Repository navigation
ci: verify published CLI with an owned fixture - #253
Merged
Merged
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| process.removeListener("SIGINT", onINT) | ||
| process.removeListener("SIGTERM", onTERM) | ||
| report.finishedUTC = new Date().toISOString() | ||
| if (evidence) { |
There was a problem hiding this comment.
🟡 Evidence-write failure in finally loses report and exit.
Suggested change
| if (evidence) { | |
| Wrap the evidence writes in their own try/catch: on failure set `pendingFailure ??= error`, push the message to `report.cleanupErrors`, set `report.status = "fail"`, and still fall through to the `console.log(report)` and the `if (pendingFailure)` exit-code block so the in-memory report is always emitted. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:382-389):
Problem: Evidence-write failure in finally loses report and exit
Detail: In the top-level `finally`, the evidence writes (`mkdir`/`writeFile` of report.json and the per-run stdout/stderr captures, lines 382-389) and the final `console.log(JSON.stringify(report))` (line 390) run before the `if (pendingFailure)` exit-code block (392-394), with no error handling of their own. If any evidence write rejects (ENOSPC, EACCES, evidence path occupied by a regular file), the exception escapes the finally block: the in-memory report — including the failure root cause already captured in `report.error` — is never printed or persisted, and the runner dies on an unhandled top-level rejection whose message is the I/O error, not the smoke result. The original test failure is masked exactly when the evidence is most needed. Repro: make --evidence-dir point at an existing regular file and force any assertion failure — the process crashes with the mkdir/write error instead of reporting the failed assertion.
Suggested fix: Wrap the evidence writes in their own try/catch: on failure set `pendingFailure ??= error`, push the message to `report.cleanupErrors`, set `report.status = "fail"`, and still fall through to the `console.log(report)` and the `if (pendingFailure)` exit-code block so the in-memory report is always emitted.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
In the top-level finally, the evidence writes (mkdir/writeFile of report.json and the per-run stdout/stderr captures, lines 382-389) and the final console.log(JSON.stringify(report)) (line 390) run before the if (pendingFailure) exit-code block (392-394), with no error handling of their own. If any evidence write rejects (ENOSPC, EACCES, evidence path occupied by a regular file), the exception escapes the finally block: the in-memory report — including the failure root cause already captured in report.error — is never printed or persisted, and the runner dies on an unhandled top-level rejection whose message is the I/O error, not the smoke result. The original test failure is masked exactly when the evidence is most needed. Repro: make --evidence-dir point at an existing regular file and force any assertion failure — the process crashes with the mkdir/write error instead of reporting the failed assertion.
process.removeListener("SIGINT", onINT)
process.removeListener("SIGTERM", onTERM)
report.finishedUTC = new Date().toISOString()
if (evidence) {
await mkdir(evidence, { recursive: true })
await writeFile(path.join(evidence, "report.json"), JSON.stringify(report, null, 2) + "\n")
for (const record of runs) {
await writeFile(path.join(evidence, `${record.label}.stdout`), record.stdout)
await writeFile(path.join(evidence, `${record.label}.stderr`), record.stderr)
}
}
console.log(JSON.stringify(report, null, 2))| let pendingFailure | ||
| const report = { | ||
| recipe: marker, | ||
| expectedVersion: "0.5.4", |
There was a problem hiding this comment.
🟡 Version and SHA pins spread across four files.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:39):
Problem: Version and SHA pins spread across four files
Detail: The 0.5.4 pin appears as literals in both workflow files (the npm spec and the inline `node -e` version check, ~2 literals each), in the README prose and snippet, and twice in smoke.mjs (`expectedVersion` and the `--version` stdout assert at line 288); the binary SHA-256 f12a5425… is likewise repeated in both workflows and the README. That is roughly eight literals across four files for one release bump. smoke.mjs fails loudly if its own pin drifts, but the README and the two workflows can drift silently against each other — e.g. a bump that updates the active workflow but misses workflow.example.yml ships an example that installs the wrong release.
Suggested fix: Centralize the pins in one consumed source, e.g. examples/released-smoke/pins.json ({version, linuxX64BaselineSha256}) read by smoke.mjs (reuse expectedVersion in the --version assert instead of a second literal) and by both workflow snippets via jq/env. The inline `node -e` version check in the workflows then becomes redundant with smoke.mjs's own assertion and can be dropped.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The 0.5.4 pin appears as literals in both workflow files (the npm spec and the inline node -e version check, ~2 literals each), in the README prose and snippet, and twice in smoke.mjs (expectedVersion and the --version stdout assert at line 288); the binary SHA-256 f12a5425… is likewise repeated in both workflows and the README. That is roughly eight literals across four files for one release bump. smoke.mjs fails loudly if its own pin drifts, but the README and the two workflows can drift silently against each other — e.g. a bump that updates the active workflow but misses workflow.example.yml ships an example that installs the wrong release.
const report = {
recipe: marker,
expectedVersion: "0.5.4",
sourceCommit: "6c661f4cf3e3d555a0f290bc22e071c6de4bd19b",
startedUTC: new Date().toISOString(),| const report = { | ||
| recipe: marker, | ||
| expectedVersion: "0.5.4", | ||
| sourceCommit: "6c661f4cf3e3d555a0f290bc22e071c6de4bd19b", |
There was a problem hiding this comment.
🟡 Hardcoded sourceCommit drifts from running source.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:40):
Problem: Hardcoded sourceCommit drifts from running source
Detail: report.sourceCommit is the frozen literal 6c661f4c… while report.sourceHashes is recomputed from disk on every run. The active workflow re-runs on every future push touching examples/released-smoke/** on main, so after the first follow-up edit the evidence report will claim a source revision that no longer describes the running script — undermining the report's provenance story. The literal also does not match this PR's head SHA (0d8be2c9…); if it denotes the v0.5.4 release commit, neither the field name nor the report says so.
Suggested fix: Either derive the running revision at runtime (e.g. `process.env.GITHUB_SHA ?? "unset"`) or rename the field to make its meaning explicit, e.g. `releaseSourceCommit: "6c661f4c…" // v0.5.4 release tag`, so it is clearly a property of the pinned release rather than of this recipe file.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
report.sourceCommit is the frozen literal 6c661f4… while report.sourceHashes is recomputed from disk on every run. The active workflow re-runs on every future push touching examples/released-smoke/** on main, so after the first follow-up edit the evidence report will claim a source revision that no longer describes the running script — undermining the report's provenance story. The literal also does not match this PR's head SHA (0d8be2c…); if it denotes the v0.5.4 release commit, neither the field name nor the report says so.
recipe: marker,
expectedVersion: "0.5.4",
sourceCommit: "6c661f4cf3e3d555a0f290bc22e071c6de4bd19b",
startedUTC: new Date().toISOString(),
nodeVersion: process.version,| }, 30_000) | ||
| controller.signal.addEventListener("abort", term, { once: true }) | ||
| const collect = (name, data) => { | ||
| record[name] += data.toString("utf8") |
There was a problem hiding this comment.
🟡 2 MiB capture bound aborts but never stops buffering.
Suggested change
| record[name] += data.toString("utf8") | |
| Once the bound trips, hard-cap capture: set a flag so subsequent `collect` calls return without appending (or `child.stdout.destroy()` / removeListener), keeping the captured string at ~2 MiB regardless of how long the child survives before SIGKILL. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:106-110):
Problem: 2 MiB capture bound aborts but never stops buffering
Detail: collect() appends every chunk to record.stdout/stderr and calls abort() once the 2 MiB bound is exceeded, but abort() only sends SIGTERM — SIGKILL is deferred up to 5 s by term()'s killTimer, and nothing detaches the stream listeners, destroys the streams, or stops appending. A child (or grandchild) that keeps printing while ignoring/trapping SIGTERM grows the captured string unbounded during the grace window, so the documented "2 MiB capture bound" does not actually bound capture: runner memory spikes and the oversized string is later written into the evidence files and stringified into report.json. Repro: given a released binary that prints continuously and traps SIGTERM, stdout passes 2 MiB → abort() fires → SIGKILL only lands 5 s later while `record.stdout` keeps growing the whole time.
Suggested fix: Once the bound trips, hard-cap capture: set a flag so subsequent `collect` calls return without appending (or `child.stdout.destroy()` / removeListener), keeping the captured string at ~2 MiB regardless of how long the child survives before SIGKILL.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
collect() appends every chunk to record.stdout/stderr and calls abort() once the 2 MiB bound is exceeded, but abort() only sends SIGTERM — SIGKILL is deferred up to 5 s by term()'s killTimer, and nothing detaches the stream listeners, destroys the streams, or stops appending. A child (or grandchild) that keeps printing while ignoring/trapping SIGTERM grows the captured string unbounded during the grace window, so the documented "2 MiB capture bound" does not actually bound capture: runner memory spikes and the oversized string is later written into the evidence files and stringified into report.json. Repro: given a released binary that prints continuously and traps SIGTERM, stdout passes 2 MiB → abort() fires → SIGKILL only lands 5 s later while record.stdout keeps growing the whole time.
const timer = setTimeout(() => {
deadline = true
term()
}, 30_000)
controller.signal.addEventListener("abort", term, { once: true })
const collect = (name, data) => {
record[name] += data.toString("utf8")
if (Buffer.byteLength(record[name]) > 2 * 1024 * 1024) {
abort(`${label} ${name} exceeds the fixture's 2 MiB capture bound`)
}
}| clearTimeout(killTimer) | ||
| controller.signal.removeEventListener("abort", term) | ||
| if (pid) { | ||
| await cleanupGroup(pid) |
There was a problem hiding this comment.
🟡 cleanupGroup throw in finally masks the primary error.
Suggested change
| await cleanupGroup(pid) | |
| Mirror the outer finally's pattern: `try { await cleanupGroup(pid) } catch (error) { pendingFailure ??= error; report.status = "fail"; report.cleanupErrors.push(error.message) }` then `groups.delete(pid)` — the primary error propagates unchanged and cleanup failures still fail the run without masking it. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:129-131):
Problem: cleanupGroup throw in finally masks the primary error
Detail: In run()'s finally, `await cleanupGroup(pid)` can throw ("Process group N still exists after SIGKILL"). A throw from a finally block supersedes the error already thrown from the try — the 30-second deadline breach (line 121) or the abort reason rethrown at line 122 — so the outer catch stores only the cleanup error in `report.error` and the `[owned CLI smoke]` stderr summary: the actual failure cause disappears from the report. Repro: child hangs past the 30 s deadline AND a lingering group member survives the post-SIGKILL 1 s poll window → the report shows only "still exists after SIGKILL", hiding the timeout. The file's own outer finally already models the right pattern (try/catch around cleanupGroup, appending to report.cleanupErrors).
Suggested fix: Mirror the outer finally's pattern: `try { await cleanupGroup(pid) } catch (error) { pendingFailure ??= error; report.status = "fail"; report.cleanupErrors.push(error.message) }` then `groups.delete(pid)` — the primary error propagates unchanged and cleanup failures still fail the run without masking it.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
In run()'s finally, await cleanupGroup(pid) can throw ("Process group N still exists after SIGKILL"). A throw from a finally block supersedes the error already thrown from the try — the 30-second deadline breach (line 121) or the abort reason rethrown at line 122 — so the outer catch stores only the cleanup error in report.error and the [owned CLI smoke] stderr summary: the actual failure cause disappears from the report. Repro: child hangs past the 30 s deadline AND a lingering group member survives the post-SIGKILL 1 s poll window → the report shows only "still exists after SIGKILL", hiding the timeout. The file's own outer finally already models the right pattern (try/catch around cleanupGroup, appending to report.cleanupErrors).
} finally {
clearTimeout(timer)
clearTimeout(killTimer)
controller.signal.removeEventListener("abort", term)
if (pid) {
await cleanupGroup(pid)
groups.delete(pid)
}
}
}| - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 | ||
| with: | ||
| node-version: 20.20.2 | ||
| - name: Install pinned public release without package scripts |
There was a problem hiding this comment.
🟡 Example workflow duplicates active workflow steps verbatim.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/workflow.example.yml:23-36):
Problem: Example workflow duplicates active workflow steps verbatim
Detail: The install, version-check, smoke-run and artifact steps (lines 23-44) are verbatim copies of .github/workflows/released-smoke.yml lines 24-51; only the `on:` triggers differ. The README local-run snippet is a third copy of the same npm-install + smoke.mjs invocation. Every future change — version bump, SHA-256 rotation, flag change, step fix — must be applied identically in three places; a missed edit silently makes the copyable example (and the README instructions) test something different from the active CI. This PR itself demonstrates the cost: the 0.5.4 pin and f12a5425… SHA already appear in all three.
Suggested fix: Extract the shared body (npm install + version check + smoke.mjs invocation) into a single script — e.g. examples/released-smoke/run-ci.sh — or a reusable composite action under .github/actions/released-smoke, so both workflow files (and the README) contain only their differing triggers and reference the same steps.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The install, version-check, smoke-run and artifact steps (lines 23-44) are verbatim copies of .github/workflows/released-smoke.yml lines 24-51; only the on: triggers differ. The README local-run snippet is a third copy of the same npm-install + smoke.mjs invocation. Every future change — version bump, SHA-256 rotation, flag change, step fix — must be applied identically in three places; a missed edit silently makes the copyable example (and the README instructions) test something different from the active CI. This PR itself demonstrates the cost: the 0.5.4 pin and f12a5425… SHA already appear in all three.
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4
with:
node-version: 20.20.2
- name: Install pinned public release without package scripts
shell: bash
run: |
npm install --prefix "$RUNNER_TEMP/aictrl-owned-cli" \
--ignore-scripts --no-audit --no-fund \
@aictrl/cli@0.5.4 @aictrl/cli-linux-x64-baseline@0.5.4| TERM: "dumb", | ||
| } | ||
| report.childEnvKeys = Object.keys(env).sort() | ||
| report.config = config |
There was a problem hiding this comment.
⚪ Evidence embeds apiKey field and request bodies.
Suggested change
| report.config = config | |
| Serialize a redacted view before persisting: replace provider options.apiKey with "[redacted]" (structuredClone the config first), and either drop record.body or store request-shape booleans (model/stream/tools-allowlist checks, marker presence) instead of full message contents — the assertions only need those booleans. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:285):
Problem: Evidence embeds apiKey field and request bodies
Detail: report.json — uploaded as a 7-day workflow artifact and printed to the job log — embeds the full provider config including the `apiKey` option field (report.config = config, line 285) plus every captured model request body (record.body = body, line 223). Today's values are inert by construction (placeholder key "fixture-not-a-credential", public fixture bytes) and the README documents that, but workflow.example.yml and the README market this recipe as a copyable template: anyone pointing it at a real provider will ship their real API key and private prompt content into a downloadable artifact. Redacting by default would make the safe pattern ship with the template.
Suggested fix: Serialize a redacted view before persisting: replace provider options.apiKey with "[redacted]" (structuredClone the config first), and either drop record.body or store request-shape booleans (model/stream/tools-allowlist checks, marker presence) instead of full message contents — the assertions only need those booleans.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
report.json — uploaded as a 7-day workflow artifact and printed to the job log — embeds the full provider config including the apiKey option field (report.config = config, line 285) plus every captured model request body (record.body = body, line 223). Today's values are inert by construction (placeholder key "fixture-not-a-credential", public fixture bytes) and the README documents that, but workflow.example.yml and the README market this recipe as a copyable template: anyone pointing it at a real provider will ship their real API key and private prompt content into a downloadable artifact. Redacting by default would make the safe pattern ship with the template.
report.childEnvKeys = Object.keys(env).sort()
report.config = config
const version = await run("version", ["--version"], env, workspace)| push: | ||
| branches: | ||
| - main | ||
| - ci/released-consumer-smoke-20261021 |
There was a problem hiding this comment.
⚪ Dated PR branch left in push trigger after merge.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, .github/workflows/released-smoke.yml:8):
Problem: Dated PR branch left in push trigger after merge
Detail: The push trigger permanently lists ci/released-consumer-smoke-20261021 — this PR's dated head branch, needed only so the branch could run its own workflow pre-merge (the PR body confirms the branch-push run). Once the PR merges, `main` plus the paths filter already covers every future change and the dated entry is dead config that will never match another push. Leaving it in the permanent trigger list invites confusion about which maintainer branches are expected to run this workflow.
Suggested fix: Drop the dated branch entry as part of landing this PR (the pre-merge run already happened), or replace it with a stable maintainer-branch pattern (e.g. ci/released-smoke-*) and note in the comment that any dated entry is temporary.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The push trigger permanently lists ci/released-consumer-smoke-20261021 — this PR's dated head branch, needed only so the branch could run its own workflow pre-merge (the PR body confirms the branch-push run). Once the PR merges, main plus the paths filter already covers every future change and the dated entry is dead config that will never match another push. Leaving it in the permanent trigger list invites confusion about which maintainer branches are expected to run this workflow.
on:
push:
branches:
- main
- ci/released-consumer-smoke-20261021
paths:
- .github/workflows/released-smoke.yml
- examples/released-smoke/**
workflow_dispatch:
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 6 · ⚪ 2 · 0/8 resolved
🤖 Fix all 8 open findings with your agent📋 Out-of-diff findings (8)
Reviewed 5 files · 0 inline · view all 8 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
Review response — PR #253Reviewed all eight findings from Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
Test plan
|
| if (!signalGroup(pid, 0)) return | ||
| await pause(50) | ||
| } | ||
| throw new Error(`Process group ${pid} still exists after SIGKILL`) |
There was a problem hiding this comment.
🟡 Zombie group member fails an otherwise green run.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:84):
Problem: Zombie group member fails an otherwise green run
Detail: kill(-pgid, 0) succeeds while any group member still exists as an unreaped zombie, so under an init that does not reap (e.g. container PID 1 without a reaper), cleanupGroup can poll past both its SIGTERM and post-SIGKILL windows and throw "Process group N still exists after SIGKILL". In run()'s finally the rethrow fires precisely when there is no primary failure, converting a run where every assertion passed into overall status "fail" with exit 1 — a false negative for the smoke.
Suggested fix: Treat zombie-only groups as cleaned (e.g. check /proc/<pid>/stat state before declaring failure), or when the child run already succeeded, record the residual-group error in report.cleanupErrors without failing the exit code.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
kill(-pgid, 0) succeeds while any group member still exists as an unreaped zombie, so under an init that does not reap (e.g. container PID 1 without a reaper), cleanupGroup can poll past both its SIGTERM and post-SIGKILL windows and throw "Process group N still exists after SIGKILL". In run()'s finally the rethrow fires precisely when there is no primary failure, converting a run where every assertion passed into overall status "fail" with exit 1 — a false negative for the smoke.
signalGroup(pid, "SIGKILL")
const killedUntil = Date.now() + 1000
while (Date.now() < killedUntil) {
if (!signalGroup(pid, 0)) return
await pause(50)
}
throw new Error(`Process group ${pid} still exists after SIGKILL`)
}| report.cleanupErrors.push(error.message) | ||
| } | ||
| report.cleanup = { processGroupsRemaining: groups.size, temporaryRootRemoved: removed } | ||
| process.removeListener("SIGINT", onINT) |
There was a problem hiding this comment.
🟡 Signal handlers removed before evidence write.
Suggested change
| process.removeListener("SIGINT", onINT) | |
| Keep the handlers installed until after the evidence writes and final console.log complete — move the removeListener calls to the very end of the finally block, or make the handlers idempotent (abort once, ignore repeats) instead of removing them early. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:402-403):
Problem: Signal handlers removed before evidence write
Detail: The SIGINT/SIGTERM listeners are removed (lines 402-403) before the evidence report is written (405-413) and printed (420). A second Ctrl+C — or a CI cancellation signal, which GitHub sends on job cancel — landing in that window reverts to the default disposition and kills the runner instantly, so no report.json is produced for the run even though --evidence-dir was supplied, and the workflow's upload step (if-no-files-found: error) then surfaces a confusing artifact error instead of the smoke result.
Suggested fix: Keep the handlers installed until after the evidence writes and final console.log complete — move the removeListener calls to the very end of the finally block, or make the handlers idempotent (abort once, ignore repeats) instead of removing them early.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The SIGINT/SIGTERM listeners are removed (lines 402-403) before the evidence report is written (405-413) and printed (420). A second Ctrl+C — or a CI cancellation signal, which GitHub sends on job cancel — landing in that window reverts to the default disposition and kills the runner instantly, so no report.json is produced for the run even though --evidence-dir was supplied, and the workflow's upload step (if-no-files-found: error) then surfaces a confusing artifact error instead of the smoke result.
} catch (error) {
pendingFailure ??= error
report.status = "fail"
report.cleanupErrors.push(error.message)
}
report.cleanup = { processGroupsRemaining: groups.size, temporaryRootRemoved: removed }
process.removeListener("SIGINT", onINT)
process.removeListener("SIGTERM", onTERM)
report.finishedUTC = new Date().toISOString()
try {
if (evidence) {| const file = path.join(workspace, "fixture.txt") | ||
| await writeFile(file, fixture) | ||
| await writeFile(path.join(workspace, "invalid.schema.json"), "[]\n") | ||
| overallTimer = setTimeout(() => abort("Recipe exceeded its external 60-second deadline"), 60_000) |
There was a problem hiding this comment.
🟡 60 s deadline armed only after unbounded setup.
Suggested change
| overallTimer = setTimeout(() => abort("Recipe exceeded its external 60-second deadline"), 60_000) | |
| Arm overallTimer as the first statement inside the try block (before the readFile hashing and the ancestor walk), or document that setup-phase hangs rely on the external job timeout. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:234):
Problem: 60 s deadline armed only after unbounded setup
Detail: The overall 60-second abort timer is armed at line 234, after hashing the recipe files and the native binary (potentially tens of MB), the ancestor lstat walk up to /, and temp-tree creation. A hang in any of those setup steps is outside the recipe's own deadline, so the README's "the whole run has 60 seconds" guarantee does not bound the whole run — only the workflow's 5-minute job timeout catches it, and a local run has no bound at all.
Suggested fix: Arm overallTimer as the first statement inside the try block (before the readFile hashing and the ancestor walk), or document that setup-phase hangs rely on the external job timeout.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The overall 60-second abort timer is armed at line 234, after hashing the recipe files and the native binary (potentially tens of MB), the ancestor lstat walk up to /, and temp-tree creation. A hang in any of those setup steps is outside the recipe's own deadline, so the README's "the whole run has 60 seconds" guarantee does not bound the whole run — only the workflow's 5-minute job timeout catches it, and a local run has no bound at all.
const file = path.join(workspace, "fixture.txt")
await writeFile(file, fixture)
await writeFile(path.join(workspace, "invalid.schema.json"), "[]\n")
overallTimer = setTimeout(() => abort("Recipe exceeded its external 60-second deadline"), 60_000)
server = createServer(async (request, response) => {
const record = { method: request.method, path: request.url, atUTC: new Date().toISOString() }| const args = process.argv.slice(2) | ||
| const options = new Map() | ||
| for (let i = 0; i < args.length; i += 2) { | ||
| assert(["--binary", "--evidence-dir", "--expected-sha256"].includes(args[i]), `Unknown option ${args[i]}`) |
There was a problem hiding this comment.
⚪ Usage errors skip evidence report entirely.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:15-23):
Problem: Usage errors skip evidence report entirely
Detail: Option parsing and validation asserts run before the main try/finally, so a bad invocation (unknown option, odd argument count, relative --binary) dies as an uncaught exception with no report.json written to --evidence-dir, unlike every other failure path. In CI the upload step then fails with if-no-files-found: error, so the job log shows an artifact error rather than a diagnostic report naming the misuse.
Suggested fix: Move argv parsing inside the main try block, or wrap it so that a parseable --evidence-dir still receives a minimal status:"fail" report describing the usage error.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Option parsing and validation asserts run before the main try/finally, so a bad invocation (unknown option, odd argument count, relative --binary) dies as an uncaught exception with no report.json written to --evidence-dir, unlike every other failure path. In CI the upload step then fails with if-no-files-found: error, so the job log shows an artifact error rather than a diagnostic report naming the misuse.
const options = new Map()
for (let i = 0; i < args.length; i += 2) {
assert(["--binary", "--evidence-dir", "--expected-sha256"].includes(args[i]), `Unknown option ${args[i]}`)
assert(args[i + 1] && !options.has(args[i]), `Missing or duplicate option ${args[i]}`)
options.set(args[i], args[i + 1])
}
const binary = options.get("--binary")
assert(binary && path.isAbsolute(binary), "Supply --binary with the absolute released native binary path")
assert(process.platform !== "win32", "This recipe requires POSIX process groups")
const evidence = options.get("--evidence-dir")| report.childEnvKeys = Object.keys(env).sort() | ||
| report.config = config | ||
| const version = await run("version", ["--version"], env, workspace) | ||
| assert.equal(version.code, 0) |
There was a problem hiding this comment.
⚪ Bare asserts lack context messages.
Suggested change
| assert.equal(version.code, 0) | |
| Add message arguments: assert.equal(version.code, 0, "Version command exit"), assert.equal(negative.code, 2, "Invalid schema must exit 2"), assert.equal(negative.signal, null, "Invalid schema run signal"). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:311):
Problem: Bare asserts lack context messages
Detail: Assertion call sites are inconsistent about human-readable messages: most wire/server asserts and the positive run pass explanatory labels ("Released native version", "Only two model turns are scripted", "Positive command failed: …"), but the outcome checks at lines 311, 353 and 354 use bare assert.equal. When one of the unlabeled asserts fires, the evidence report gets only a generic expected/actual diff without naming which contract was violated (version exit code vs invalid-schema exit code/signal).
Suggested fix: Add message arguments: assert.equal(version.code, 0, "Version command exit"), assert.equal(negative.code, 2, "Invalid schema must exit 2"), assert.equal(negative.signal, null, "Invalid schema run signal").
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Assertion call sites are inconsistent about human-readable messages: most wire/server asserts and the positive run pass explanatory labels ("Released native version", "Only two model turns are scripted", "Positive command failed: …"), but the outcome checks at lines 311, 353 and 354 use bare assert.equal. When one of the unlabeled asserts fires, the evidence report gets only a generic expected/actual diff without naming which contract was violated (version exit code vs invalid-schema exit code/signal).
const version = await run("version", ["--version"], env, workspace)
assert.equal(version.code, 0)
assert.equal(version.stdout.trim(), "0.5.4", "Released native version")
assert.equal(requests.length, 0, "Version command must not call the model")| ["home", "data", "config", "cache", "state"].map((name) => [name, path.join(root, name)]), | ||
| ) | ||
| await Promise.all([workspace, ...Object.values(dirs)].map((dir) => mkdir(dir))) | ||
| // v0.5.4 still discovers nested instruction files despite the config flag. |
There was a problem hiding this comment.
⚪ Workaround comment lacks #250 issue reference.
Suggested change
| // v0.5.4 still discovers nested instruction files despite the config flag. | |
| Change to: `// v0.5.4 still discovers nested instruction files despite the config flag (#250).` |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #253, examples/released-smoke/smoke.mjs:216):
Problem: Workaround comment lacks #250 issue reference
Detail: The comment justifying the manual ancestor AGENTS.md/.opencode/AGENTS.md sweep is a release-behavior workaround, and the README ties this exact behavior to issue #250, but the comment itself carries no issue reference — a future reader cannot tell when the workaround can be removed. The repo convention (no workaround/TODO comments without an issue reference) is followed everywhere else here.
Suggested fix: Change to: `// v0.5.4 still discovers nested instruction files despite the config flag (#250).`
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The comment justifying the manual ancestor AGENTS.md/.opencode/AGENTS.md sweep is a release-behavior workaround, and the README ties this exact behavior to issue #250, but the comment itself carries no issue reference — a future reader cannot tell when the workaround can be removed. The repo convention (no workaround/TODO comments without an issue reference) is followed everywhere else here.
await Promise.all([workspace, ...Object.values(dirs)].map((dir) => mkdir(dir)))
// v0.5.4 still discovers nested instruction files despite the config flag.
for (let directory = workspace; ; directory = path.dirname(directory)) {
for (const name of ["AGENTS.md", ".opencode/AGENTS.md"]) {
const instruction = path.join(directory, name)
const absent = await lstat(instruction).then(
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 3 · ⚪ 3 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 5 files · 0 inline · view all 6 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
Review response — PR #253Responding to this review. Judged against Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed hereNo findings are deferred from this review. Optional observations retained without a code change:
VerificationEight whole-runner cases/89 checks cover actual first/repeated SIGINT and SIGTERM during paused evidence writes, primary-error preservation and normal released-0.5.4 execution. Hosted CI: 1,829 passed, six skipped, zero failed; CodeQL passed. Downloaded release-smoke artifact passes 15 checks, including the released binary hash, actual fixture exchange, exit codes and cleanup. This is a scripted-provider smoke, not model-quality evidence. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No linked issue.
Intent
Repository CI builds source, while consumers install a published npm artifact. Add a deterministic consumer-contract check against pinned CLI 0.5.4 so release packaging and headless integration are exercised independently of the source build.
Expected Impact on Users
Maintainers can detect a broken published CLI artifact separately from a successful source build. The credential-free fixture exercises an actual file read, event output, error classification and cleanup using pinned version 0.5.4, and gives integrators a reproducible diagnostic. This adds integration evidence; it does not change the CLI's runtime behavior or prove a real model completes an adopter's task.
Expected Outcomes
Implementation
Use a scripted loopback provider, an inert fixture, fresh HOME/XDG state, closed stdin, a read-only wire tool catalog and an explicit child environment without host credentials. Verify the native artifact version and SHA-256 before invocation.
The active workflow runs on path-filtered pushes to
mainand this exact maintainer branch, plus manual dispatch. It never checks out an unknown PR or runs on PR events. Official action revisions, Node 20.20.2 and npm packages 0.5.4 are pinned; package scripts are disabled. Checkout receives onlycontents: readand does not persist credentials. Uploads contain only inert fixture evidence.Scope Caveat
The scripted provider proves harness integration, not paid-provider quality, successful structured-output generation or untrusted-checkout safety. npm setup uses network access and the CI job is not an egress sandbox. Released stdin, instruction-discovery and subprocess-environment constraints remain documented. No merge, release or production deployment is included.
Test Plan
Verification — current corrective head
Current corrective head:
31f2b60ca9ece9732204634ba0c4fffbd8588419. Idempotent signal handlers remain installed through evidence writes and final report output; a first signal records nonzero interruption and preserves any primary failure. README states the execution-phase 60-second timer and enclosing five-minute CI timeout accurately.Historical verification and earlier corrective round
Verification
Independent local QA passed the final formatted public-layout runner in 1.302 seconds: native 0.5.4 and expected hash; two model requests with actual fixture bytes; successful NDJSON/exit 0; invalid-schema exit 2 without extra requests; zero remaining process groups and removed temporary roots. Separate local fault injection verified SIGKILL escalation and cleanup; ancestor instruction guards rejected before CLI/model execution.
Installed Prettier, JavaScript syntax, YAML/bash structure and source/hash checks passed. Normal pre-push checks passed all six Turbo tasks (cache hits). The actual branch-push consumer smoke run passed on commit
0d8be2c97378f8ee54d8c3c9728e70af419806f9. Its downloaded artifact confirms native version/hash, exactly two wire requests with actual fixture bytes, exit codes 0/0/2, all runner assertions and clean process/state cleanup; all four recipe hashes match the reviewed source. All five PR checks also passed for this exact head:verify,Analyze (actions),Analyze (javascript-typescript),CodeQLandsmoke. Six polls completed the 30-minute review window at 2026-10-21T02:22:46Z; no reviews or comments arrived. This satisfies the bounded monitoring protocol and does not imply review approval. The PR is ready for code review; aictrl-bot review was triggered on 21 October.Corrective review follow-up — current head
Commit
51f2c2f6932f52c69c277231512781cd92858730bounds each captured stream to2MiB while draining, preserves primary failures through cleanup/evidence-write errors, reports cleanup failures independently, and names the released tag sourcereleaseSourceCommit. Original review response:4TRUE/FIX,3TRUE/IGNORE style tradeoffs,1FALSE/IGNORE fake-credential claim;8exact recorded verdicts written/readbackverified.Independent targeted QA:12cases/104checks, including real released0.5.4 positive paths, overflow, deliberately injected cleanup failures and report-path failures. Fresh CI passed1829tests/6skipped/0failed and typechecking. GitHub's actual merge checkout has the same tree as the corrective head. Released consumer smoke passed; its downloaded artifact passes15readback checks, with all4recipe hashes matching this head, actual fixture bytes and exits0/0/2. CodeQL checks also succeeded.
New-push monitoring ended10:52:15UTC with all6checksSUCCESS and fresh exact-head review5598518143. Its six new findings are under source verification; this PR is not yet clean-review or merge-ready. The original verification above remains historical proof at0d8be2c9.