Skip to content

fix: a fresh arm directory runs the resume check in silence (#109) - #122

Merged
rookslog merged 4 commits into
mainfrom
fix/review-arm-fresh-dir
Aug 16, 2026
Merged

fix: a fresh arm directory runs the resume check in silence (#109)#122
rookslog merged 4 commits into
mainfrom
fix/review-arm-fresh-dir

Conversation

@rookslog

@rookslog rookslog commented Aug 16, 2026

Copy link
Copy Markdown
Owner

The first review-baseline arm on #109 (bench/run.sh review-baseline-112-118 --prompts bench/review-prompts/112-118 --reps 2) printed no matches found on its first line of work. The resume check asks which sidecars an arm holds with ls over a glob, a fresh arm holds none, and zsh answers NOMATCH — so the line prints and the run correctly carries on with existing empty.

Corrected after this branch opened. That arm was healthy: it finished about three hours later with all eight samples, both manifests covering their plans, exit 0. The zero-byte .err and absent manifest that the arm directory held at the time are exactly what a call still OPEN looks like — stderr's redirect creates the file at spawn and the runner removes it on completion. An earlier revision of this description read them as a signal kill, on a reproduction that matched them byte for byte; that reproduction was evidence of the in-progress state and not of a termination, and the benign reading went unconsidered. The commit messages on this branch are immutable and carry the retracted account; the code, the README and this description carry the corrected one.

What the noise cost was the reading rather than a sample. An arm that prints a failure and then says nothing for hours cannot be told from a dead one, so a check that passes has to be silent as well as harmless. The glob becomes the (N) null-glob qualifier. The resume refusal is untouched, and the new test holds it: an arm re-run under a changed system prompt still exits 2.

Nothing exercised the runner, which is why nobody could settle it either way. test/bench-helpers.js builds an arm by writing the files an arm holds, which is every step except the ones only the runner takes. test/run-sh.test.js now drives bench/run.sh end to end over a stand-in claude, on a fresh directory and then on a changed configuration, and skips where zsh is absent. It cannot measure duration, and the AGENTS.md blind-spot entry says so, because the stand-in answers at once where a real arm is quiet for hours.

bench/README.md states what the calling shell supplies for a real run, including the credential route #77 ranks, and adds that an arm is slow and silence is not failure. The CI comment claiming the runner is never invoked is corrected.

npm run check exits 0 (901 tests, 0 failures). The new test fails against the previous line and passes against the fix. Measured on the last full CI run: the two runner tests execute on macOS and skip on the GitHub ubuntu image, which carries no zsh.

The first real review arm on issue #109 printed `no matches found` from the
resume check and collected nothing. The message and the abort are two events,
and reading them as one hid both.

The resume check asks which sidecars an arm already holds, with `ls` over a
glob. A fresh arm holds none, so zsh answers NOMATCH: the substitution fails,
the line prints, and the run carries on with `existing` empty, which is the
correct reading of an empty arm. The arm then reached its first live call, and
the shell was killed by a signal during it — before the line that removes an
empty `.err`, and without the EXIT trap, which is why the directory held one
zero-byte `.err` and no manifest. A stand-in `claude` reproduces each half: the
fixed runner completes a fresh arm clean, and a stand-in that signals its own
process group leaves exactly the directory the operator saw.

The glob is now the `(N)` null-glob qualifier, so no match gives no words. A
check that passes has to be silent as well as harmless, because the arm spends
a live call under it. The refusal is untouched: an arm resumed under a changed
system prompt or a changed rule set still exits 2 rather than mixing two
conditions under one name.

Nothing exercised the runner. `test/bench-helpers.js` builds an arm by writing
the files an arm holds, which is every step except the ones only the runner
takes, so the smoke path could not see this. `test/run-sh.test.js` drives
`bench/run.sh` end to end over a stand-in `claude`, on a fresh directory and
then on a changed configuration. It skips where zsh is absent and it reaches no
model.

`bench/README.md` now states what the calling shell supplies for a real run,
including the credential route, and the CI comment that said the runner is
never invoked is corrected.
@github-actions github-actions Bot added documentation Improvements or additions to documentation distribution Install pathways, npm, plugin marketplaces labels Aug 16, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b86ac47094

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread test/run-sh.test.js Outdated
`run.sh` writes under its own `bench/out/`, so a literal arm name here is a
directory in the operator's tree. Two test processes on one checkout collide
there, and anything already standing at the name would be resumed by the fresh
-arm test and then removed by its cleanup. The name now comes from the
per-test temporary directory, which is unique to the run, and the test asserts
it is a name `arm-manifest.mjs` accepts.

Two more from reading the file back. zsh is asked for by name through `PATH`
rather than at `/bin/zsh`, which is macOS's path and would have skipped the
whole file on Linux while `execFile` used it anyway. And the header no longer
says CI does not run this, which the corrected workflow comment already
contradicts.
…109)

New evidence retracts the diagnosis this branch was written under. The first
review arm completed about three hours after it launched: eight samples, both
manifests covering their plans, exit 0. So the zero-byte `.err` and the absent
manifest were a snapshot of a call still OPEN, and the reproduction that
matched them byte for byte was evidence of that state and not of a kill. A
signal was named over an artifact that not-being-finished-yet produces, and
the benign reading was never ruled out.

The fix stands and the reason for it changes. The NOMATCH was cosmetic noise
on a working run, and the cost was the reading: an arm that prints a failure
and then says nothing for hours cannot be told from a dead one, which is what
the unexercised runner left nobody able to check. A passing check has to be
silent, because silence is what the next line of output is read against.

`bench/run.sh` and `test/run-sh.test.js` carry the corrected account.
`bench/README.md` says an arm is slow and silence is not failure, with the
duration marked unaudited. The blind-spot entry in AGENTS.md now names
duration as the thing a stand-in cannot measure.
`git add -A` staged the scratch file two of these commits were written
from. The net diff against main no longer carries it.
@rookslog
rookslog merged commit 1b20406 into main Aug 16, 2026
8 checks passed
@rookslog
rookslog deleted the fix/review-arm-fresh-dir branch August 16, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

distribution Install pathways, npm, plugin marketplaces documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant