Skip to content

fix(sdk): build kernel + restore fixture +x before npm test (unblocks drive loop, replaces #64/#65/#68) - #69

Merged
kjgbot merged 2 commits into
mainfrom
fix/sdk-test-prep-kernel
Aug 30, 2026
Merged

fix(sdk): build kernel + restore fixture +x before npm test (unblocks drive loop, replaces #64/#65/#68)#69
kjgbot merged 2 commits into
mainfrom
fix/sdk-test-prep-kernel

Conversation

@kjgbot

@kjgbot kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Root cause

Since PR #63 the drive loop has been stalling on "19 SDK test failures". Root cause: `sdk/tests/live-kernel.test.ts` requires a built `relayflowd` binary (built via `ops/cargo.sh` into a toolchain-external target dir, per PR #38). The cloud sandbox does NOT build the kernel before running `npm test`.

Every drive step that inspects test status observes the failures:

  • assess-1 sees red tests → writes `ops/NEEDS_HUMAN.md` → `assess-gate` parks the run
  • No build/verify step ever fires

Why this replaces #64, #65, #68

This one-line `sdk/package.json` change lands the prep AT the layer every stage hits: `npm test` itself. Whoever runs npm test — the assessor, the builder, verify, or a human on a laptop — gets a built kernel first.

Diff

```diff

  • "test": "npm run build && vitest run",
  • "test:prep": "( cd ../kernel && sh ../ops/cargo.sh build ) && ( find ../testdata/preflight -name '*-cli' -type f -exec chmod +x {} + 2>/dev/null || true )",
  • "test": "npm run test:prep && npm run build && vitest run",
    ```

Test plan

  • Confirmed `npm test` invokes `test:prep` first as `npm run test:prep`
  • Confirmed package.json parses
  • Swarm review (this PR)
  • First drive run after merge unblocks (produces a PR that actually builds the GHA workflow instead of parking)

… drive loop)

Root cause the drive loop has been stalling on since PR #63 (2026-08-30):
sdk/tests/live-kernel.test.ts requires a built relayflowd binary
(ops/cargo.sh's toolchain-external target), and the cloud sandbox
does not build it before running npm test. Result: the assessor observes
red tests, writes ops/NEEDS_HUMAN.md, assess-gate parks the run, and no
build/verify step ever fires.

Fix moves the prep INSIDE npm test itself so it's correct for every
caller — the drive assessor, drive builder, verify step, and humans on
a laptop.

Superseded PRs (all closed):
- #64: separate pre-build step — steps have per-step sandboxes, prep
  invisible to build (BACKLOG.md:688-707)
- #65, #68: prep in build's task prompt — assess parks first, so build
  never runs

This lands the prep at a lower layer that all three stages hit.
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

Next included review available in 40 minutes.

View limit details

Limit details: You’ve used the included review currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: c9426509-4acc-498d-b954-d83150ae5ec2

📥 Commits

Reviewing files that changed from the base of the PR and between 5c5c05d and 240a39c.

📒 Files selected for processing (1)
  • sdk/package.json
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: 24037a99-69ce-42bf-8e93-3d2216389105

📥 Commits

Reviewing files that changed from the base of the PR and between 7369f55 and 5c5c05d.

📒 Files selected for processing (1)
  • sdk/package.json

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The SDK test workflow now runs a preparation script. The preparation builds the kernel and updates preflight CLI fixture permissions before the SDK build and Vitest execution.

Changes

SDK test workflow

Layer / File(s) Summary
Test preparation and execution
sdk/package.json
Adds test:prep for the kernel build and preflight CLI permissions. The test script runs this preparation before the SDK build and Vitest.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 5c5c0

The change runs kernel and fixture preparation before SDK tests; no actionable merge-blocking risk remains beyond normal checks and review.

Poem

A rabbit checks the kernel’s track
Then makes CLI tools executable back
The tests begin in ordered flight
Vitest hops through the night
Clean scripts keep the path bright


Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing.

Comment @coderabbitai help to get the list of available commands.

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — FAIL

Maintainability Review — PR #69 (sdk/package.json)

Blockers

None strictly, but this is close. The 2>/dev/null || true on the chmod branch (sdk/package.json:26) silently masks every failure in that subshell: missing ../testdata/preflight, missing find, permission denied, even a typo in the flag. A future developer who moves or renames the fixture directory will not learn about it from npm test — they'll get a downstream vitest failure with no signal pointing here. The stated intent ("CLIs may not all exist yet") should be expressed as [ -d ../testdata/preflight ] && find ... -exec chmod +x {} + (still tolerant of the absent dir, but any real error surfaces). As written, the contract is invisible.

Concerns

  • Silent monorepo coupling. test now cd's into ../kernel, invokes ../ops/cargo.sh, and touches ../testdata/preflight (sdk/package.json:26). A stranger reading sdk/package.json sees no comment, no README pointer, and no guard that these paths exist. If the SDK is ever consumed standalone (its package.json is a publishable-looking artifact — bin, files, exports), npm test breaks in a confusing way. At minimum add a one-line comment via an adjacent "//test:prep" key documenting why the prep step exists and what it assumes about layout.
  • test:watch skips prep (sdk/package.json:27). The two entry points now diverge: test builds the kernel and chmods fixtures; test:watch uses whatever stale binary happens to be on disk. Anyone iterating locally with test:watch after a kernel change will silently test the old binary — exactly the "test that wouldn't fail if the behavior broke" pattern the lens warns about.
  • Implicit Rust toolchain install on npm test. ops/cargo.sh may fetch and install a full rustup toolchain (ops/cargo.sh:79-121) if none is present. A first-time contributor running npm test from sdk/ gets a multi-minute network install with no warning from the npm script name. This surprising side effect needs to be flagged in sdk/README.md or the script name (e.g. pretest:kernel) — "test:prep" undersells it.
  • Unconditional chmod on every run mutates checked-in fixtures each invocation. If executability were tracked in git the prep step wouldn't be needed; if it can't be tracked (Windows checkouts?), the reason belongs in a comment. Neither is stated.

Notes

  • Splitting into test:prep is a real readability win over inlining — good direction. Consider pretest (npm's lifecycle hook) so vitest-only invocations still get prep, closing the test:watch gap.
  • Subshell isolation ( cd ../kernel && ... ) is correct; won't leak cwd. Fine as-is on POSIX; this repo appears Unix-only so not a blocker.

REVIEW_FAILED

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

history lens — FAIL

Blocker

  • sdk/package.json:26-27 reintroduces the exact failure-hiding pattern deliberately removed from PR fix(workflows): build agent preps kernel + fixtures inside its own sandbox (v2 — replaces #64, #65) #68: 2>/dev/null || true. That PR’s history says silent skips reproduce the stall being fixed. The fixtures are not optional—they are tracked executable files used by SDK CLI/live-kernel tests—so failure to find or chmod them must fail test:prep. Masking the error violates AGENTS.md’s fail-closed rule and makes the commit claim “restore fixture +x” untrue: the command only attempts restoration. Remove the redirection and || true.

Concerns

  • None beyond the blocker.

Notes

REVIEW_FAILED

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

structure lens — PASS

→ Read docs/RFC-0001-everything-is-a-relayflow.md
→ Read sdk/package.json

$ ls -la /Users/khaliqgant/AgentWorkforce/flows-ops 2>/dev/null; echo "---kernel---"; ls /Users/khaliqgant/AgentWorkforce/flows-ops/kernel 2>/dev/null || echo "no kernel dir"; echo "---ops---"; ls /Users/khaliqgant/AgentWorkforce/flows-ops/ops 2>/dev/null || echo "no ops dir"; echo "---cargo.sh---"; ls /Users/khaliqgant/AgentWorkforce/flows-ops/ops/cargo.sh 2>/dev/null || echo "no cargo.sh"
total 32
drwxr-xr-x@ 17 khaliqgant staff 544 Aug 30 05:11 .
drwxr-xr-x@ 9 khaliqgant staff 288 Aug 31 00:00 ..
drwxr-xr-x@ 3 khaliqgant staff 96 Aug 30 05:11 .claude
-rw-r--r--@ 1 khaliqgant staff 295 Aug 30 05:11 .env.example
drwxr-xr-x@ 17 khaliqgant staff 544 Aug 31 00:12 .git
-rw-r--r--@ 1 khaliqgant staff 416 Aug 30 05:11 .gitignore
-rw-r--r--@ 1 khaliqgant staff 3900 Aug 30 05:11 AGENTS.md
-rw-r--r--@ 1 khaliqgant staff 1410 Aug 30 05:11 README.md
drwxr-xr-x@ 3 khaliqgant staff 96 Aug 30 05:11 charter
drwxr-xr-x@ 5 khaliqgant staff 160 Aug 30 05:11 docs
drwxr-xr-x@ 10 khaliqgant staff 320 Aug 30 16:25 kernel
drwxr-xr-x@ 21 khaliqgant staff 672 Aug 30 20:14 ops
drwxr-xr-x@ 17 khaliqgant staff 544 Aug 30 05:11 regressions
drwxr-xr-x@ 3 khaliqgant staff 96 Aug 30 05:11 scripts
drwxr-xr-x@ 11 khaliqgant staff 352 Aug 30 21:36 sdk
drwxr-xr-x@ 23 khaliqgant staff 736 Aug 30 05:11 testdata
drwxr-xr-x@ 8 khaliqgant staff 256 Aug 30 05:11 workflows
---kernel---
Cargo.lock
Cargo.toml
DESIGN.md
README.md
package-lock.json
relayflowd
relayflowd-core
relayflowd-journal
---ops---
AUTODRIVE_BRIEF.md
AUTONOMY.md
BACKLOG.md
DIRECTIVES.md
DRIVE-LOG.md
FORBIDDEN_PATHS
HANDOFF-2026-08-28.md
IMMUTABLE_PATHS
NEXT.md
RUN-CONTRACT.md
SCOREBOARD.md
STATE.md
autodrive.sh
cargo.sh
deliver-run.sh
gen-drive-cloud.py
launch-gate.sh
open-pr.mjs
reviews
---cargo.sh---
/Users/khaliqgant/AgentWorkforce/flows-ops/ops/cargo.sh

$ ls /Users/khaliqgant/AgentWorkforce/flows-ops/testdata/preflight 2>/dev/null || echo "no testdata/preflight"; echo "---sdk test files---"; ls /Users/khaliqgant/AgentWorkforce/flows-ops/sdk/testdata 2>/dev/null || ls /Users/khaliqgant/AgentWorkforce/flows-ops/sdk 2>/dev/null
authenticated-cli
cli-declared.flow.yaml
cli-missing.flow.yaml
cli-signal.flow.yaml
cli-unauthenticated.flow.yaml
cli-unresolved.flow.yaml
counting-cli
empty-path.flow.yaml
flows.json
no-executor.flow.yaml
project-default
shared-cli.flow.yaml
signal-probe-cli
trigger-declared.flow.yaml
unauthenticated-cli
warning.flow.yaml
---sdk test files---
dist
node_modules
package-lock.json
package.json
scripts
src
tests
tsconfig.json
vitest.config.ts

$ ls -la /Users/khaliqgant/AgentWorkforce/flows-ops/testdata/preflight/ | grep -i cli; echo "---charter---"; ls /Users/khaliqgant/AgentWorkforce/flows-ops/charter; echo "---cargo.sh head---"; head -40 /Users/khaliqgant/AgentWorkforce/flows-ops/ops/cargo.sh
-rwxr-xr-x@ 1 khaliqgant staff 115 Aug 30 05:11 authenticated-cli
-rw-r--r--@ 1 khaliqgant staff 136 Aug 30 05:11 cli-declared.flow.yaml
-rw-r--r--@ 1 khaliqgant staff 129 Aug 30 05:11 cli-missing.flow.yaml
-rw-r--r--@ 1 khaliqgant staff 160 Aug 30 05:11 cli-signal.flow.yaml
-rw-r--r--@ 1 khaliqgant staff 169 Aug 30 05:11 cli-unauthenticated.flow.yaml
-rw-r--r--@ 1 khaliqgant staff 109 Aug 30 05:11 cli-unresolved.flow.yaml
-rwxr-xr-x@ 1 khaliqgant staff 232 Aug 30 05:11 counting-cli
-rw-r--r--@ 1 khaliqgant staff 272 Aug 30 05:11 shared-cli.flow.yaml
-rwxr-xr-x@ 1 khaliqgant staff 156 Aug 30 05:11 signal-probe-cli
-rwxr-xr-x@ 1 khaliqgant staff 86 Aug 30 05:11 unauthenticated-cli
---charter---
LEAD.md
---cargo.sh head---
#!/bin/sh

Cargo with a private toolchain home, so a run neither depends on nor pollutes

a machine-global cargo store — while keeping that home OUT of the repo.

It also has to FIND cargo. A cloud sandbox puts rustup's shims on PATH for

interactive/agent shells but not for the deterministic step shell, where

cargo resolved to nothing and the step died with

env: 'cargo': No such file or directory (run f18ec684, verify-1) — even

though the agent step in the same sandbox had just compiled and tested the

whole workspace. So: use cargo from PATH when it is there, fall back to the

standard rustup locations, then install one, and only then give up.

set -eu

The toolchain must live OUTSIDE the repo. It used to sit in

the repo's own .cargo-home for hermeticity, and in a cloud sandbox that was

exactly wrong: the executor propagates the workspace between steps and drops

files over a per-file size cap, so a toolchain installed in one step arrived

in the next with pieces missing —

error: Missing manifest in toolchain 'stable-x86_64-unknown-linux-gnu'

(run 4b40159d, verify-1). A partial copy is worse than no copy: it looks

installed and fails on use. Keeping it out of the propagated tree means each

step either finds a whole toolchain or installs one.

toolchain_home="${RELAYFLOWS_TOOLCHAIN_HOME:-$HOME/.relayflows-toolchain}"
export CARGO_HOME="$toolchain_home/cargo"
export RUSTUP_HOME="$toolchain_home/rustup"

Build OUTPUT must stay out of the propagated tree too, for a different and

sharper reason than the toolchain above.

kernel/target/debug is about 4900 files. With it inside the tree, the

relayfile mount flush is rejected as too large and the run keeps going:

relayfile flush failed after the command succeeded (exit 0); a later agent

step may see stale files: ... notify flush: ... http 413 payload...

The failure is non-fatal, so the workflow reports success while later steps

read stale files and the delivered patch silently loses the run's real work.

Three runs lost their work exactly this way (fdb49a9c 4127 changed files,

ad98c2c3 4175, 52fa0752 3247 — three 413s each), against one that did not

(76a4a8d1, 489 files, no flush failure). Of 52fa0752's changed paths, 4914

matched target/debug and 1 matched node_modules, so this directory is the

whole of the problem.

Structure lens — PR #69

The diff is a one-line addition of a test:prep script to sdk/package.json, wired into test before the build+vitest run. On the structure axes I own — boundaries, coupling, file size, single-purpose, RFC-0001 vocabulary fidelity — this change is small and clean.

Boundaries and kernel purity. No kernel code is touched; no provider SDK, product logic, or tenant awareness enters kernel/. The script merely calls the existing ops/cargo.sh build from outside, which is the correct direction (the SDK builds against the kernel boundary, not the reverse). The RFC's language boundary (§4 — "a binary you call over a protocol cannot absorb product logic") is respected.

Coupling. Mild concern, not a blocker. sdk/package.json:26 hard-codes ( cd ../kernel && sh ../ops/cargo.sh build ) — a relative-path assumption that sdk/, kernel/, and ops/ remain siblings at the repo root. Today they do (verified: kernel/Cargo.toml, ops/cargo.sh, testdata/preflight all resolve from the diff's paths). If the layout ever restructures, this breaks silently. A DRYer shape would be a single shared prep script in ops/ invoked from both places, but for a repo this size the inline form is acceptable.

Fail-closed / completionReason discipline. Note: the test:prep chain ends with || true on the find … -exec chmod +x … clause, swallowing the exit status. This is soft-fallback tasting — the exact pattern AGENTS.md §4 and Covenant 2 warn against. It is scoped to test scaffolding (notifying an exec-bit fix for testdata/preflight/*-cli fixtures), not a journal write, so it does not reach the "journal write that fails fails the step" bar. Still, it should be understood as a deliberate test-infra affordance, not a precedent to copy into kernel or runner code.

File size / single purpose. No file grows toward a smell threshold; package.json stays a manifest. The find … -name '*-cli' and the cargo build are two distinct prep duties bundled under one test:prep name — arguably two concerns ("build kernel", "normalize fixture mode bits") but folding them into one prep step is the idiomatic npm pattern and not a defect.

Vocabulary / RFC alignment. The preflight fixtures (cli-missing, cli-unauthenticated, cli-unresolved, no-executor) directly serve Covenant 2's preflight obligation ("refuses or warns before the run starts on anything it cannot prove") — the exec-bit normalization is what lets those fixture CLIs actually run as declared. This change strengthens, rather than undermines, the preflight story.

No blockers. No primitives added. No product logic in the kernel. No file grown past its purpose.

REVIEW_PASSED

Both lenses on #69 correctly flagged `2>/dev/null || true` as
failure-hiding: fixtures are tracked, chmod errors should surface.

Replaced with `[ ! -d ../testdata/preflight ] || find ...` — tolerant
of an absent dir, but any real error (missing find, permission denied,
typo in flag) fails test:prep loudly. Maintainability lens's exact
recommendation.
@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — PASS

Maintainability review — PR #69 (sdk/package.json)

Blockers: none.

Concerns

  1. Silent-skip failure mode on the preflight guard. sdk/package.json:26 uses [ ! -d ../testdata/preflight ] || find .... If the testdata/preflight directory is ever moved or renamed, test:prep short-circuits to success — the tests then run against whatever exec bits happen to be on disk, or fail with an obscure EACCES at test time. This directly contradicts AGENTS.md rule 4 ("fail closed") and the exact failure class the neighbouring test warns about (sdk/tests/live-kernel.test.ts:27-31: "these cases SKIP rather than fail and the suite reports a false green"). A cheaper form — no guard, let find on a missing path fail loudly — matches the "no || true on the kernel build" reasoning already applied one clause earlier.

  2. Implicit fixture-naming contract. The -name '*-cli' glob quietly binds chmod to a naming convention. Anyone adding a preflight fixture that isn't suffixed -cli (e.g. signal-probe, custom-tool) will get a green test:prep and a red vitest run. Nothing in testdata/preflight/ or sdk/tests/preflight.test.ts documents the convention. A comment in live-kernel.test.ts pointing at the coupling, or renaming existing fixtures to a stricter pattern documented in code, would keep the invariant discoverable.

  3. Duplicated prep ceremony. workflows/drive-cloud.yaml:226+ already carries an inline kernel-build/chmod prep with different mechanics (sdk/node_modules/.bin, esbuild path chmod). Now sdk/package.json:26 asserts a variant of the same invariant. Two prep paths drifting in slightly different ways is the shape of the maintenance trap this PR is meant to relieve — the cloud yaml should be updated to defer to npm test (which now does prep itself) so the invariant lives in one place.

Notes

  • No test protects test:prep itself. If a future edit deletes the kernel build clause, CI red is the only signal; consider a one-liner in bin.test.ts or a new test:prep.test.ts that asserts the script contains the two clauses (or at least that RELAYFLOWD_BIN resolves after a fresh checkout).
  • The PR body's "Diff" block shows the older || true chmod form, not the shipped [ ! -d ... ] || find form. Anyone reading the PR later to understand the code will see wrong text. Update the description or link to the actual file to avoid teaching stale rules (the same anti-pattern AGENTS.md lines 47-51 warns about).
  • ../kernel and ../testdata/preflight are hard-coded relative to sdk/. Fine today; would benefit from a one-line comment above the test script (e.g. via a README hop, since JSON has no comments) recording why test:prep exists — the drive-loop stall and the 4900-file target dir. A stranger reading this in six months will otherwise have to run git blame and read PR fix(sdk): build kernel + restore fixture +x before npm test (unblocks drive loop, replaces #64/#65/#68) #69 (whose body doesn't match the code) to find out.

REVIEW_PASSED

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

history lens — PASS

Blockers

None.

Concerns

None. The final test:prep command at sdk/package.json:26–27 propagates both kernel-build and fixture-permission failures. The branch’s follow-up commit deliberately removed the original 2>/dev/null || true, so this diff no longer repeats the repository’s documented fail-open failure class.

Notes

This change fits the recorded history:

  • Earlier attempts placed preparation in a separate workflow step or build prompt. Those failed because per-step sandboxes do not reliably share files or git objects (ops/BACKLOG.md:565–585). Putting preparation inside npm test at sdk/package.json:26–27 ensures the build and Vitest execute in the same sandbox.
  • It does not reintroduce the large in-tree kernel/target directory deliberately removed after HTTP 413 flush failures. The new script invokes ops/cargo.sh, whose CARGO_TARGET_DIR remains outside the propagated tree and keyed per worktree (ops/cargo.sh:27–52).
  • Restoring executable bits on tracked testdata/preflight/*-cli fixtures directly addresses the sandbox mode-stripping incident; using -type f avoids following symlinks, and real find/chmod failures now stop the test.
  • The change supports RFC-0001’s preflight covenant and gate-1 live-kernel acceptance requirements (docs/RFC-0001-everything-is-a-relayflow.md:33–35, 95–100); it contradicts no settled decision.
  • The commit messages are truthful. 5c5c05d adds the kernel build and fixture-mode restoration, while 240a39c accurately describes the subsequent fail-closed correction. The only minor boundary is that test:watch remains unprepared, but neither commit claims to change watch mode.

REVIEW_PASSED

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

structure lens — PASS

→ Read docs/RFC-0001-everything-is-a-relayflow.md
→ Read AGENTS.md
→ Read sdk/package.json

$ ls -la ops/cargo.sh 2>&1; echo "---"; ls -la ../flows/ops 2>&1 | head; echo "--- find testdata/preflight ---"; find ../flows/testdata/preflight -maxdepth 2 2>&1 | head -40
-rwxr-xr-x@ 1 khaliqgant staff 6604 Aug 30 05:11 ops/cargo.sh

ls: ../flows/ops: No such file or directory
--- find testdata/preflight ---
find: ../flows/testdata/preflight: No such file or directory

Structure review — PR #69 (sdk/package.json test script)

The diff is a single, small change to sdk/package.json lines 26-28: it pulls out a test:prep script that (a) builds the Rust kernel via sh ../ops/cargo.sh build and (b) chmod +x every *-cli file under ../testdata/preflight, then chains it ahead of npm run build && vitest run in test.

No product logic reaches the kernel here, no new primitive is added, and no file grows past its purpose. The change is purely test-fixture prep, so the core RFC-0001 / AGENTS.md invariants (closed kernel vocabulary, fail-closed journal, completionReason discipline, single-purpose modules) are untouched. The sdk still only speaks the journal protocol; building the kernel is a test-time concern, not a runtime one.

Concerns

  1. Cross-package coupling via a print-inline shell one-liner. test:prep reaches out of the sdk package into a sibling crate (../kernel) with a brittle relative path, and composes a subshell pipeline (( … ) && [ ! -d … ] || ( find … -exec chmod … )) directly inside package.json. This embeds multi-step orchestration where this repo's own convention is a dedicated scripts/ file — build already offloads to scripts/make-cli-executable.mjs (line 22). The chmod logic, in particular, is exactly what make-cli-executable.mjs-style scripts exist for; inlining it a second time splits the convention.

  2. Silent skip of fixture prep. [ ! -d ../testdata/preflight ] || find … short-circuits when the directory is absent, so a missing testdata/preflight tree does not fail the run — it silently leaves -cli fixtures non-executable. That is a quiet-fallthrough shape. For a test-prep step it is arguably tolerable, but it is the same "silent pass when a precondition is unmet" pattern AGENTS.md cautions against in production code.

  3. chmod +x mutates source-controlled fixtures. If *-cli files under testdata/preflight are tracked non-executable, the prep step will flip their mode bits and dirty the working tree on every npm test.

Notes

  • Shell-out to cargo.sh (not cargo directly) is consistent with the repo's ops/ wrapper, which is good.
  • The &&-chain is fail-closed at the top level (a failing cargo build aborts before vitest), which is the right shape.

No blockers.

REVIEW_PASSED

@kjgbot

kjgbot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

🎯 review-swarm: PASSED (M:pass H:pass S:pass)

Lens transcripts posted as sibling comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant