fix: the audit's remaining findings, and doctor learns whether jobs detach - #224
Merged
Conversation
…etach Five findings from the six-persona sweep and the acceptance passes, plus a commitment I made and dropped. doctor now reports whether dist/job-runner.js is present. resolveRunnerPath() returning undefined is not an error — it is the signal to run jobs IN-PROCESS, which is right for an unbuilt checkout and wrong everywhere else, because the concurrency cap is enforced by the supervisor pool. So the setting that exists because of a measured OOM silently stops applying, with one stderr line at dispatch time and nothing checking it. I said I would add this after the first auditor raised it and did not; an audit noticing twice is what it took. Dead workspace locks are swept. A lock whose holder is gone IS reclaimed — but only when something contends for that exact directory, and nothing contends for a path you dispatched against once. A resource audit measured a seven-day-old lock still sitting there. The sweep reuses isDead(), so a file is removed under exactly the rule that would let a waiter steal it; a looser second rule here could delete a lock the acquire path still considers live. `usage` with no routes printed a bare header. That reads as a broken command, which is the one thing it does not mean. It now says there are no routes and names both ways to get one. `--json` is a promise about the shape of the output and was kept only on the success path: a bad --config made `doctor --json` print a sentence, so anything parsing it got a parse error instead of the reason. Same message, correct envelope; without --json the plain line is unchanged, and a test pins both directions. README: the configure paragraph was one 180-word block covering four separate concerns, which a new-user audit could not follow. Split under labelled headings, with the job-runner check named and `--live` marked as spending quota. Carried and NOT fixed, with reasons: stream-subprocess has no backpressure and kills a healthy child on queue overflow — a real design change to the path every dispatch takes, and the module already documents the behaviour honestly; MCP JSON-RPC error frames bypass the redaction sinks on the HTTP transport, structural and never demonstrated carrying a credential; two weak test assertions that are redundant rather than wrong. npm run check: 1258 passed, 8 skipped, exit 0.
Merged
fstubner
added a commit
that referenced
this pull request
Sep 6, 2026
Version bump, changelog heading, and the acceptance record for the release. Also the seven findings from the final acceptance pass, which returned CONDITIONAL: - The release notes declared `doctor --json` still printed text on a load failure. It does not; this same release fixed it. - PRs #223 and #224 had no changelog entries at all — eight user-visible changes, including doctor gaining a check that makes it exit 1 in an unbuilt checkout, and a lock sweep that runs before every job start. - The `usage` empty state and the doctor job-runner check were pinned by no test: removing both left the suite byte-identical at 1258 passed. - pruneDeadWorkspaceLocks deleted an unreadable lock on sight, measured at 25ms against the acquire path's deliberate 1018ms grace, while its docblock, its commit message and its test all asserted the two rules were the same. It now honours the grace. `wx` creation leaves a real empty-file window, and this runs before every job start, so losing that race deletes a live holder's lock and lets two dispatches edit one shared_locked workspace. - Removing the sweep's call from start.ts failed no test. - `--json` was honoured only for the exact token `--json`, and four error shapes bypassed the envelope entirely; `unknown command` printed the help block to stdout, so `frobnicate --json | jq` was fed usage text. All four emit the envelope now and usage moved to stderr, which also means main() throws for a missing prompt rather than returning 1. - The redaction sink inventory omitted result.md and both installOutputRedaction calls; removing the first left that file 20/20 green. Live dispatch on this build: cursor_cli returned pong; codex_cli refused as rate-limited, which costs nothing and counts as a refusal. npm run check: 1269 passed, 8 skipped, exit 0, green in one full run.
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
1 participant
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.
Five findings from the six-persona sweep and the acceptance passes — plus a commitment I made in review and dropped.
doctornow reports whether jobs will actually detachresolveRunnerPath()returning undefined is not an error — it is the signal to run jobs in-process, which is right for an unbuilt checkout and wrong everywhere else. The concurrency cap is enforced by the supervisor pool, so in-process mode silently removes the bound that exists because of a measured OOM. One stderr line at dispatch time, and nothing checking it.An auditor raised this, I said I would fold it in, and I did not. It took a second audit noticing.
Dead workspace locks are swept
A lock whose holder is gone is reclaimed — but only when something contends for that exact directory, and nothing contends for a path you dispatched against once and moved on from. A resource audit measured a seven-day-old lock still sitting there.
The sweep reuses
isDead(), so a file is removed under exactly the rule that would let a waiter steal it. A looser second rule here would be a way to delete a lock the acquire path still considers live.usagewith no routes printed a bare headerWhich reads as a broken command — the one thing it does not mean. It now says there are no routes and names both ways to get one. Carried across three releases as cosmetic; it is the first thing a user with no routes sees.
--jsonnow holds on the error path--jsonis a promise about the shape of the output, and it was kept only on the success path: a bad--configmadedoctor --jsonprint a sentence, so anything parsing it got a parse error instead of the reason.{ "ok": false, "error": "config path ... is a directory, not a file. Point --config at the config.yaml inside it." }Without
--jsonthe plain line is unchanged. Tests pin both directions; sabotaging the envelope fails only the JSON one.README
The
configureparagraph was one 180-word block covering four separate concerns, which the new-user audit could not follow. Split under labelled headings, with the job-runner check named and--livemarked as spending quota.Carried and NOT fixed, with reasons
stream-subprocesshas no backpressure and kills a healthy child on queue overflow. A real design change to the path every dispatch takes, and the module already documents the behaviour honestly rather than claiming otherwise.boundary-strictness, codex/cursortoBeDefined) — redundant rather than wrong.Repo hygiene
Tracked repo is clean: no build artifacts, explicit
filesfield, 1.89 MB published tarball, no stray or backup files, and the site is not shipped. Ignored working-tree clutter noted separately — two items predate the project rename.npm run check: 1258 passed, 8 skipped, exit 0.