Prepare the plugin for its move into acyclic-labs/sdk - #24
Conversation
|
| TARGET_DIR="${CARGO_TARGET_DIR:-$ROOT/target}" | ||
| [ -d "$TARGET_DIR" ] || TARGET_DIR="$ROOT/../target" |
There was a problem hiding this comment.
In a clean standalone checkout, $ROOT/target does not exist when this initialization runs, so the fallback selects $ROOT/../target. The later Cargo build writes to $ROOT/target, but the acceptance suite is pointed at the parent directory and fails because it cannot find the binaries. Detect the SDK layout from workspace metadata instead of checking whether a target directory already exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/ci-local.sh
Line: 21-22
Comment:
**Wrong clean-checkout target**
In a clean standalone checkout, `$ROOT/target` does not exist when this initialization runs, so the fallback selects `$ROOT/../target`. The later Cargo build writes to `$ROOT/target`, but the acceptance suite is pointed at the parent directory and fails because it cannot find the binaries. Detect the SDK layout from workspace metadata instead of checking whether a target directory already exists.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @@ -74,8 +73,6 @@ jobs: | |||
| persist-credentials: false | |||
| - name: Product name is single-sourced (product.toml) | |||
| run: bash scripts/check-product-name.sh | |||
| - name: No forbidden files or credential patterns | |||
| run: bash scripts/check-no-secrets.sh | |||
| - name: Code quality (line width, TODO format, comment blocks, duplication) | |||
| run: bash scripts/check-code-quality.sh | |||
| - uses: EmbarkStudios/cargo-deny-action@3c6349835b2b7b196a839186cb8b78e02f7b5f25 # v2.1.1 | |||
There was a problem hiding this comment.
This removes the repository's credential and forbidden-file scan without invoking the stated SDK replacement from this workflow. While this repository still handles pull requests and pushes to main, a committed credential or private-key file can therefore pass its CI and enter Git history.
How this was verified: The remaining deny job runs only product-name, code-quality, and cargo-deny checks after the sole credential-pattern scanner was deleted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 65-78
Comment:
**Credential scanning removed**
This removes the repository's credential and forbidden-file scan without invoking the stated SDK replacement from this workflow. While this repository still handles pull requests and pushes to `main`, a committed credential or private-key file can therefore pass its CI and enter Git history.
**How this was verified:** The remaining deny job runs only product-name, code-quality, and cargo-deny checks after the sole credential-pattern scanner was deleted.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| step "no secrets or forbidden files" bash scripts/check-no-secrets.sh | ||
| step "code quality (width, TODOs, comment blocks, duplication)" bash scripts/check-code-quality.sh | ||
| step "cargo deny" cargo deny --locked check | ||
| step "cargo deny" cargo deny --locked check licenses |
There was a problem hiding this comment.
Limiting this command to licenses omits the advisory, banned-package, and source-policy checks that GitHub CI still runs. A dependency with an allowed license but a known advisory, yanked version, or disallowed source can pass ci-local.sh, so local validation no longer reproduces CI.
| step "cargo deny" cargo deny --locked check licenses | |
| step "cargo deny" cargo deny --locked check |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/ci-local.sh
Line: 56
Comment:
**Local deny checks narrowed**
Limiting this command to `licenses` omits the advisory, banned-package, and source-policy checks that GitHub CI still runs. A dependency with an allowed license but a known advisory, yanked version, or disallowed source can pass `ci-local.sh`, so local validation no longer reproduces CI.
```suggestion
step "cargo deny" cargo deny --locked check
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| target_dir() { | ||
| if [ -n "${CARGO_TARGET_DIR:-}" ]; then printf '%s\n' "$CARGO_TARGET_DIR"; return; fi | ||
| (cd "$ROOT" && cargo metadata --no-deps --format-version 1) | sed -n 's/.*"target_directory":"\([^"]*\)".*/\1/p' | ||
| } |
There was a problem hiding this comment.
Relative target path misresolved
If CARGO_TARGET_DIR is relative and this script is invoked outside the repository, Cargo resolves the path from $ROOT, while the later cp resolves it from the caller's working directory. The release can then fail to find the built binary or copy a stale artifact from an unrelated directory. Normalize relative configured paths against $ROOT before using them.
| target_dir() { | |
| if [ -n "${CARGO_TARGET_DIR:-}" ]; then printf '%s\n' "$CARGO_TARGET_DIR"; return; fi | |
| (cd "$ROOT" && cargo metadata --no-deps --format-version 1) | sed -n 's/.*"target_directory":"\([^"]*\)".*/\1/p' | |
| } | |
| target_dir() { | |
| if [ -n "${CARGO_TARGET_DIR:-}" ]; then | |
| case "$CARGO_TARGET_DIR" in | |
| /*) printf '%s\n' "$CARGO_TARGET_DIR" ;; | |
| *) printf '%s/%s\n' "$ROOT" "$CARGO_TARGET_DIR" ;; | |
| esac | |
| return | |
| fi | |
| (cd "$ROOT" && cargo metadata --no-deps --format-version 1) | sed -n 's/.*"target_directory":"\([^"]*\)".*/\1/p' | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packaging/npm/release-local.sh
Line: 62-65
Comment:
**Relative target path misresolved**
If `CARGO_TARGET_DIR` is relative and this script is invoked outside the repository, Cargo resolves the path from `$ROOT`, while the later `cp` resolves it from the caller's working directory. The release can then fail to find the built binary or copy a stale artifact from an unrelated directory. Normalize relative configured paths against `$ROOT` before using them.
```suggestion
target_dir() {
if [ -n "${CARGO_TARGET_DIR:-}" ]; then
case "$CARGO_TARGET_DIR" in
/*) printf '%s\n' "$CARGO_TARGET_DIR" ;;
*) printf '%s/%s\n' "$ROOT" "$CARGO_TARGET_DIR" ;;
esac
return
fi
(cd "$ROOT" && cargo metadata --no-deps --format-version 1) | sed -n 's/.*"target_directory":"\([^"]*\)".*/\1/p'
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.cfc168f to
9062da1
Compare
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
9062da1 to
b5140eb
Compare
The crates will live at plugin/ in the sdk workspace and release from that repository as plugin-v<version> tags, so everything that assumes this repo is its own root or its own release source learns both layouts now, while this repo's CI can still prove it. - acyclic-fs is pinned to the sdk main line (sdk PR #99 carries the Darwin unpaired-rename, subtree-removal and O_EXCL fixes this plugin needed). The guard forwards the new capture_host_subtree mount hook. - scripts/install.sh targets plugin-v* releases on acyclic-labs/sdk and reads the current version from plugin/LATEST on main instead of the repository's "latest" release, which in a multi-family repo is not ours. product.toml gains release_tag_prefix and check-product-name.sh guards it. - The acceptance harness, ci-local.sh, docker-linux.sh and release-local.sh find the cargo target directory and lint, test and build only the plugin's own crates, in either layout. - FUSE-T is no longer needed on macOS: acyclic-fs mounts through its vendored darwinfuse server and the release binary does not link libfuse. The pkg-config shim, rpath flag and CI installer step are gone. - Edition 2024 and the sdk's lint set: the env mutation in the speculation config test carries an explicit unsafe allowance, nested ifs became let chains, and the crate roots allow missing_docs with a reason until the per-item docs land. - check-no-secrets.sh is dropped; the sdk's gitleaks and boundary scans cover it. The Windows ACL import is aliased so the boundary scanner does not read `Authorization::` as a credential header. - Version 0.0.3.
b5140eb to
e82be0b
Compare
Step "PR 0.5" of the consolidation into
acyclic-labs/sdk(the plugin becomesplugin/there; sdk PR #99 lands the acyclic-fs fixes it depends on).What changes, all validated by this repo's own CI:
acyclic-fspinned to sdk PR #99's head (main line plus the Darwin unpaired-rename, subtree-removal andO_EXCLfixes). The guard forwards the newcapture_host_subtreehook.scripts/install.shtargetsplugin-v*releases onacyclic-labs/sdkand reads the version fromplugin/LATESTon main.product.tomlgainsrelease_tag_prefix;check-product-name.shguards it.ci-local.sh,docker-linux.sh,release-local.shwork in both layouts (workspace target dir, plugin crates only).otool -L).unsafeallowance in one test, let chains,missing_docsallowances with reasons at the crate roots).check-no-secrets.shdropped in favour of the sdk's gitleaks and boundary scans; the WindowsAuthorizationimport is aliased so the sdk boundary scanner does not flag it.Validation on macOS:
cargo fmt --check,cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo test --workspace,check-product-name.sh,check-code-quality.sh, and the full acceptance suite (run-all.sh, all 13 scripts green) against the pinned acyclic-fs.Summary by cubic
Prepares the plugin for its move into
acyclic-labs/sdkasplugin/by making install, release, and local validation work in both the standalone repo and the sdk workspace. Installs now come fromplugin-v*releases there, and macOS no longer requires FUSE-T.Release and install
scripts/install.shreads the version fromplugin/LATESTon sdk main and usesrelease_tag_prefixfromproduct.toml;check-product-name.shguards it.acyclic-fsis pinned to the sdk main line (Darwin unpaired-rename, subtree-removal, andO_EXCLfixes), and the guard forwards the newcapture_host_subtreemount hook.F_BARRIERFSYNCis absent.Validation and tooling
release-local.shbuild, lint, and test only the plugin crates and find the cargo target dir in either layout.missing_docs.check-no-secrets.shis replaced by the sdk's gitleaks and boundary scans; the WindowsAuthorizationimport is aliased so the scanner does not flag it.Written for commit e82be0b. Summary will update on new commits.