fix: three findings from a six-persona audit sweep - #223
Merged
Conversation
Six independent auditors with different lenses. Most of what they reported was already answered by the code; these three were not. A test whose premise never held. dispatch-log's "when the log dir is unwritable" test set the log dir to a path containing a NUL byte, which reads as obviously invalid and is not: assigning it to process.env truncates the value AT the NUL, so the log dir became a perfectly writable directory and the write succeeded. `not.toThrow()` then passed for the absence of the condition the test names. The auditor found the weak assertion; the dead premise turned up while fixing it. It now blocks the directory with a regular file, asserts the warning fires, and asserts it fires once rather than per dispatch. Deleting the warning — the auditor's own sabotage — fails it. Empty supervisor spawn logs accumulated forever. Measured here: 129 files going back three weeks, zero live heartbeats, 780 bytes between them, six bytes each. Keeping these is deliberate and the reason is sound — a supervisor that died is the one that left a stale heartbeat, and its bootstrap output is the only explanation. That covers a log with something IN it. An empty one explains nothing, and the liveness check reads this directory on every drain, which is the same permanent per-dispatch cost the heartbeat cleanup was added to stop paying. Empty logs past the staleness window are now dropped; non-empty ones and fresh ones are kept, and a test pins all three cases. The README told a brand-new user to run `doctor --live` as step three of installing, with no indication that it contacts providers and spends quota — the tool's own code describes the flag as "consume quota". Quick start now uses plain `doctor`, and `--live` is described as the deliberate step it is. Same paragraph says `configure` is optional, which the README asserted 150 lines later and not where someone deciding whether to run it would see. npm run check: 1254 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.
Six independent auditors, different lenses: security, correctness, new user, returning maintainer, resources, tests. Most of what they reported was already answered by the code. Three were not.
A test whose premise never held
dispatch-log's "when the log dir is unwritable" test set the log dir to a path containing a NUL byte. That reads as obviously invalid and is not — assigning it toprocess.envtruncates the value at the NUL, so the log dir became a perfectly writable directory and the write succeeded.not.toThrow()then passed for the absence of the condition the test names.The auditor found the weak assertion. The dead premise turned up while fixing it. The test now blocks the directory with a regular file, asserts the warning fires, and asserts it fires once rather than per dispatch. Deleting the warning — the auditor's own sabotage — fails it.
Empty spawn logs accumulated forever
Measured here: 129 files going back three weeks, zero live heartbeats, 780 bytes between them — six bytes each.
Keeping these is deliberate and the reason is sound: a supervisor that died is the one that left a stale heartbeat, and its bootstrap output is the only explanation of why. That covers a log with something in it. An empty one explains nothing — and the liveness check reads this directory on every drain, which is the same permanent per-dispatch cost the heartbeat cleanup was added to stop paying.
Empty logs past the staleness window are dropped; non-empty ones and fresh ones are kept. A test pins all three cases.
The quick start spent your quota without saying so
The README told a brand-new user to run
doctor --liveas step three of installing. The tool's own code describes that flag as "consume quota", and the acceptance procedure says it "can burn a subscription window".Quick start now uses plain
doctor, with--livedescribed as the deliberate step it is. The same paragraph notesconfigureis optional — which the README already said, 150 lines below where someone deciding whether to run it would look.What was rejected, and why
copy/git_worktreetakes its copy each dispatch is isolated. The whole-dispatch lock is the branch above, forshared_locked.start.tsruns in-process and returns before the slot queue is reached, and it warns.file-lock.ts's own comment, which namesBreakerStoreas the caller that must not drop an update.npm run check: 1254 passed, 8 skipped, exit 0.