Conductor hardening: close six reported defects and twelve found auditing them (GH #6) - #7
Merged
Merged
Conversation
+ audit) Wave 1 of the hardening reported in #6. Three defects that all fail in the same direction — a gate that says green when it has not checked. 1. Reviewer fixes could escape write_scope (#6 comment 2, security boundary). runFixLoop() staged and amended the repair, THEN the attempt's scope check ran against `git status --porcelain` — empty on a committed tree, so the violation was invisible. A reviewer-triggered repair had strictly more write authority than the maker session whose identical violation is caught. The gate now runs on the dirty repair tree before `git add`; a violation is an ordinary red attempt, the diff is preserved, and it never reaches runtime. 2. A chatty model self-approved (found while auditing; not in the report). APPROVED_RE and RUNTIME_PASS_RE were unanchored .test(body) calls, and the reviewer/runtime prompts contain "VERDICT: APPROVED" and "RUNTIME: PASS" as instructions. A model that restated its instructions was read as approving regardless of the verdict it reached: reproduced, a document ending "VERDICT: CHANGES REQUESTED" read as APPROVED. readVerdict() now takes the LAST line-anchored verdict, treats a missing verdict as blocking, and fails closed on a line naming both outcomes. classifyRuntimeVerdict() likewise. 3. Post-approval code changes landed unreviewed (found while auditing). Round 3 runs as the coder agent with the worktree writable; "do not edit implementation files" is an instruction, not a gate. An edit inside write_scope passed the scope re-check and was folded into the closed commit by an unguarded amend. Any non-document change after the last approving review now invalidates that approval and fails the attempt. Also: the run lock is placed via `git rev-parse --git-common-dir` (#6 comment 1) — `<root>/.git` is a FILE in a linked worktree, so the old path died ENOTDIR before any gate ran. Resolved against ROOT, not cwd, and acquired with 'wx' so two conductors starting at once cannot both win the check-then-write race. Tests: new scripts/conductor/conductor.hardening.test.mjs — negative controls that pass silently when the defect is present. Verified they fail against HEAD aaea3c7 (3 fail) and pass on this commit (5 pass). Existing conductor + resume suites: 19 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
…dings (GH #6) Wave 2 of #6 — items 1, 3 and 4. 1. --max-processed (item 3). --max-tickets bounds SUCCESSES: `landed` only advances on a verified success, so on a board with a low success rate `--max-tickets 1` could claim, run and fail an unbounded number of tickets. A success target is not a work budget. --max-processed is charged on the CLAIM, whatever the outcome. Startup orphans are reported as `resumed` and do not consume it — they were claimed by a previous invocation, and charging them would make the flag mean two things depending on how the last run died. conductor.end now reports landed/processed/resumed/stopReason. Every numeric flag now goes through intOpt(). `Number(opt(...))` failed silently in two directions: a typo became NaN, so `while (landed < NaN)` was false on the first evaluation and the run exited reporting landed=0 — indistinguishable from an empty board — and a bare flag became `true`, which Number()s to 1. 2. Timeout descendants (item 4). spawnSync's timeout signals the direct child only; a session is opencode plus whatever it shells out to. Those descendants outlived the timeout and a same-worktree retry raced them. Sessions now spawn into their own process group and a timeout kills the GROUP — SIGTERM, bounded grace, SIGKILL, then a liveness check. Retry is permitted only once the group is observably empty. Containment fails closed: no pid, a group surviving SIGKILL, or a platform without group semantics all refuse the retry and return 124. Windows retries zero times. Timeouts are now classified from both shapes Node reports them in. The `res.error` return sat ABOVE the signal check, so ETIMEDOUT was reported as a generic session failure — the same timeout, two outcomes, by platform. Containment lives in scripts/lib/session-containment.mjs because conductor.mjs calls main() at import, so nothing in it is unit-testable — the same reason runtime-verdict.mjs and attempt-outcome.mjs exist. 3. Findings cross attempt boundaries (item 1). A red fix loop returned only reviewer NAMES, so the next fresh attempt was told who objected and never what was wrong. reviewFailureFeedback() carries the terminal findings: non-approved verdicts only, bounded per-document and overall with truncation announced, `(missing)` for an unreadable review (never inferred as approval), and fenced in BEGIN/END UNTRUSTED markers — a review quotes the source it reviewed, so it can carry instruction-shaped text into the prompt of the agent about to rewrite that source. Also wires conductor.hardening.test.mjs into Pass 53 and corrects the stale "not wired into scripts/test.ts" headers (untrue since v3.1.2 — R-15). Tests: 11 hardening cases, all verified to fail without their fix. Pass 53 now runs 30 conductor tests (was 19). Full suite: 724 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
None of these are in the report. All four are error paths that fail in the direction of looking fine. 1. A failed merge crashed the run mid-merge. land()'s `git merge` was a bare call and `sh` throws on non-zero git exit, so the throw escaped land() and main(); main().catch logged conductor.fatal and exited 1, leaving the target's main branch with MERGE_HEAD set and the merge staged. The NEXT run then refused to start on "working tree not clean" — blaming a dirty tree the conductor itself created — and the ticket was stranded in in_review having been accept()ed in memory only. The merge is now an outcome: abort, log, comment, push the BRANCH (main never got the work, so the branch is the only copy), return false. The in-memory accept() is discarded by re-loading the board rather than persisted — "Done" for work that never reached main is a lie the board would tell every future reader. 2. Scope-violation evidence could dirty the target. preserveAttemptEvidence() wrote a self-ignoring .gitignore into the evidence directory; captureScopeEvidence() wrote into the same directory and did not. So whether a scope violation poisoned the next run depended on whether some earlier ticket had happened to preserve attempt evidence first. Both callers now go through ensureEvidenceDir(). 3. runVerifyDirect() had no maxBuffer. The default is 1MB; a verbose test suite overflows it, spawnSync kills the process, and `status` comes back null — which the function reported as a failure. Its one job is to overturn an unsubstantiated runtime FAIL by re-running the ticket's own verify, so an overflow made it uphold the FAIL it exists to check. Now 256MB (as runBaselinePreflight already used), and a run that produces no exit code is reported `inconclusive` rather than as a failing verify — a harness problem must not be attributed to the candidate's code. 4. pushRemotes() could not push a branch under --merge. Mode-derived ref only; the new conflict path needs the branch explicitly. Added `forceRef`. Tests: the merge-conflict path is reproduced deterministically with a pre-merge-commit hook that refuses — same shape as a content conflict (non-zero exit, MERGE_HEAD set, merge staged). Verified it fails against the previous commit with "a failed merge must be an outcome, not a fatal crash". Conductor suites: 31 passed, 0 failed. Full suite: 724 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
… item 2)
A deterministic runtime failure used to discard a candidate that had already
passed scope and independent review, restarting the next attempt from main —
throwing away the coding AND the review effort over what is frequently one
mechanical mistake. --runtime-fix-iterations (default 1) allows a bounded
repair instead.
The repair is NOT a shortcut past the gates. It is a new candidate that
re-earns all of them:
runtime FAIL -> repair -> no-op check -> scope -> reviewer RECOMPUTE
-> fresh review -> bounded review-fix -> fresh runtime
-> scope -> close
Three rules make that safe, each because its absence launders unreviewed code
into a closed ticket:
1. Any code change invalidates ALL prior approval — the re-review is not
limited to reviewers who objected before.
2. Reviewers are RECOMPUTED from the post-repair main...branch diff. A repair
can touch a newly security-sensitive path, and the reviewer set that never
saw that path is the wrong one.
3. Freshness is PROVEN. runReviewRound() now archives and removes each review
document before its session, so an exit-zero reviewer that writes nothing
reads as "no review" instead of silently re-using its earlier APPROVED.
(Archived first: the round that TRIGGERED a repair would otherwise be gone
from the evidence by the time the attempt failed.)
A no-op repair is still a failure; an out-of-scope repair is rejected with its
diff preserved; the repair path never calls close, accept, merge or release.
DELIBERATE DEVIATION from the reported spec: it asks that the failed runtime
report be committed separately so a no-op repair cannot look like a source
change. The candidate is meant to land as a single commit, so the no-op check
instead asks which NON-DOCUMENT files the repair touched — documents are
excluded by construction. Same guarantee, no extra commit.
Also fixes an asymmetry the repair loop exposed: "evidence outranks the claim"
applied to FAIL only. A claimed PASS was taken at face value even when the same
document quoted a non-zero exit and a failing test — the round scrutinised a
pessimistic model and trusted an optimistic one. runtime-verdict.mjs had stated
the rule ("prose never overrides exit codes") and implemented it in
classifyRuntimeVerdict() since P-A9, but no gate path ever called it: a rule
written and left unwired. close() would still have caught the bad candidate,
but only after the repair budget was spent and after the receipts recorded PASS
for work that does not build.
Tests: 7 acceptance cases on a stateful fixture (the same role is invoked
repeatedly and must behave differently each time) — happy path asserted on
event ORDER not final status, stale-approval reuse, no-op repair, out-of-scope
repair, --runtime-fix-iterations 0 compatibility, repair bound respected, and
the self-contradicting PASS. All five new negative controls verified to fail
against the previous commit.
Pass 53 now runs 38 conductor tests. Full suite: 724 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
… failures close() is documented as "the load-bearing verb" — it runs the ticket's own verify command from outside the session and refuses to advance on non-zero. It ran that command with execSync's DEFAULTS, and both are wrong here. 1. NO TIMEOUT. An unattended run hung forever on a verify that hangs — a watch-mode test runner, a command that prompts, a wedged container. Every other loop in this executor is bounded (sessions, provider retries, fix iterations, attempts, and now tickets processed); the one that was not bounded was the gate itself. Now 30 minutes by default, overridable per project via conductor.config.json `verifyTimeoutMs` and threaded through both the conductor and the resume path. 2. 1MB maxBuffer. execSync THROWS ENOBUFS when a command that exits ZERO simply prints a lot, and that throw landed in the same catch as a real failure — so a fully green ticket was refused. Verified directly: a command printing 2MB and exiting 0 throws ENOBUFS. Now 256MB, matching what conductor.mjs already uses for the same command shape (and the same defect fixed in runVerifyDirect one commit ago — same root cause, two call sites). 3. All three outcomes reported "verify gate did not exit 0", and for the two above that sentence is false: there was no exit code at all. An operator reading it goes looking for a failing test that does not exist. Timeout and buffer-overflow now say what actually happened and that the ticket is NOT verified; a real non-zero exit still reports exactly as before. Checked and NOT a defect, recorded so it is not re-litigated: the JIRA board driver cannot supply `verify` or `write_scope`. Both come from a checked-in TICKET_SCOPE_MAP precisely so board content never becomes a command — the deliberate counterpart to the session env allowlist. Tests: three library-level cases. The two negative controls fail against the previous commit — the hang one by running the full 60s test timeout, which is the defect stated as plainly as it can be. Conductor suites: 41 passed, 0 failed. Full suite: 724 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
…nd two silent review-gate gaps
Continuing the audit past the reported issue. Four more, all of the same
family: something that should stop, block, or recruit, silently didn't.
1. Exhausting the provider-limit retries CRASHED THE RUN. runSession ended
with `throw new Error('limit retries exhausted')`, and the throw escaped
runSession, executeTicket AND main() — main().catch logged conductor.fatal
and exited 1 with the ticket still claimed and owned, released by nobody. It
now returns a non-zero code, which routes into the existing
blockWithoutExhausting() path: released with a reason, evidence preserved,
and a provider outage does not consume the feature's coding attempts.
2. STOP was ignored for hours. The provider backoff doubles 5m -> 60m and can
total well over two hours across its retries, and STOP was only ever read
BETWEEN tickets — so an operator who touched STOP during a rate-limit pause
was ignored for the rest of the backoff. STOP is the only stop mechanism
this executor has. The backoff now polls it every 5s and abandons the
session when it appears.
3. The two retry budgets shared one counter. GH #6 item 4 asks that
--session-timeout-retries be kept separate from the provider rate-limit
retries and both bounded; the flag shipped last commit but both still
consumed the same `attempt` loop, so a timeout retry silently spent a
rate-limit retry. Now separate counters, with --session-limit-retries
(default 5, preserving the historical 6-iteration loop) and
--limit-backoff-minutes (default 5).
4. Reviewer selection under-recruited, twice — in a module whose own header
says it is "biased toward firing: a false negative ships unreviewed auth".
- DELETIONS WERE INVISIBLE. It read only `+++ b/<path>`, and git renders a
deleted file as `+++ /dev/null` with the real path on the `--- a/` line.
So deleting src/auth/session.ts recruited nobody but code-reviewer, while
adding one line to it recruited security. Removing an auth check is not
the lower-risk change. Both header sides are now read, /dev/null filtered.
- AN UNROUTABLE DECLARED REVIEWER VANISHED. A board declaring
`reviews: ["securty"]` asked for a security review, got none, and nothing
anywhere recorded it — the run looked like a normally reviewed ticket.
triggeredReviewers() now returns `dropped`, and the conductor logs it with
the routable names. Deliberately NOT fatal: the routable set is small and
a board may name reviewers a different executor implements. Making it
refuse the ticket instead is a reasonable stricter call, left open.
Tests: 6 new cases. The review-trigger and provider-limit-exhaustion controls
were verified red against the previous commit. The STOP-during-backoff control
could not be run against the unfixed code to completion — it sleeps out the
full 5-minute backoff and hits the test harness timeout, which is the defect
demonstrated about as plainly as it can be.
Pass 53 now runs 45 conductor tests. Full suite: 724 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
… repo Three findings in the scope gate — the containment boundary the whole executor rests on. 1. CONTAINMENT BYPASS. matches_scope() enforced "a bare '*' / '**' is refused outright (would authorise the whole repo)" — its own stated intent — with a blacklist of four LITERAL strings. Every other spelling walked through. Measured against the real function: '**/*', '*/**', '*/*', '?*/**' and '[a-z]*/**' each matched "src/auth/session.ts", i.e. authorised every nested path in the repository, while only the two exact strings were refused. A blacklist of spellings cannot express "names no path". Replaced with the intent: the FIRST path segment must be a literal name. 'a/**' and 'src/features/x/*.test.ts' stay anchored and allowed; '**/*' and friends are refused, as is '*.ts' — which spans every tree in the repo, the exact thing a write_scope exists to prevent. Verified end to end against a real git repo: the old script ALLOWED an out-of-scope write under '**/*'; the new one refuses it, and every previously-passing pattern still passes. 2. RENAMES WERE CHECKED ON ONE SIDE. The enumerator printed only a rename's destination, so `R src/auth/important.ts -> a/moved.ts` passed a write_scope of 'a/**' while having DELETED a file outside it. The destination being in scope says nothing about the source. Both sides are now checked. Reachable only if the session stages the rename itself — which it is told never to do, and which is precisely what a gate must not depend on. Also: paths are read with core.quotePath=false and surrounding quotes stripped, so a correctly-scoped file no longer fails the gate because its name is non-ASCII. 3. THE MATCHER'S TEST RAN NOWHERE, AND TESTED A COPY. validate-scope.match.test.sh has existed since v3.8.0, is referenced only in docs/RELEASE_TRACKER.md as proof the matcher is "fixture-backed", and no harness has ever executed it. It also carried its own copy-pasted matches_scope, so it could not fail when the product changed — only when the copy did; the two had already diverged. The matcher now lives once, in _scope-match.sh, sourced by both; the fixture suite gained the six unanchored-pattern cases; and Pass 53 runs it, refusing a zero-assertion run as a pass exactly as it does for node --test. This is scripts/test-conductor-suite.ts's own lesson — "a test that no harness executes is not a test; it is a file that resembles one" — recurring one directory over, in the file that guards the containment boundary. Matcher fixtures: 21 passed, 0 failed (was 15, all still passing). Pass 53 now runs 48 conductor tests. Full suite: 724 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
A sweep for test files no harness executes, prompted by finding validate-scope.match.test.sh in that state last commit. Six more: scripts/lib/jira-tickets.test.mjs (6 tests) scripts/lib/jira-tickets-parity.test.mjs (4 tests) scripts/lib/jira-tickets-integration.test.mjs (1 test) scripts/lib/tests/annotate.test.mjs (11 tests) scripts/lib/tests/img-gate.test.mjs (16 tests) scripts/log-hop.test.mjs (7 tests) 45 tests, referenced by nothing but themselves. The JIRA three are the whole board driver's coverage — a 342-line module that the conductor can be pointed at with CONDUCTOR_BOARD=jira — with no harness behind it. All six are GREEN as found, so nothing was broken. They were unprotected, which is the state conductor.test.mjs was in while it stayed RED across v3.1.0, v3.1.1 and earlier — the exact history that caused Pass 53 to be written. The fix is not a longer list. Pass 53 now DISCOVERS every *.test.mjs under scripts/, because a list has to be updated by whoever adds a suite and fails silently when they don't, whereas discovery cannot be forgotten. Two vacuity guards: zero discovered files fails the Pass (discovery broken), and zero passing tests already failed it (an empty match must never read as green). Pass 53: 9 suites, 93 tests (was 3 suites, 48 tests). Full suite: 724 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ
10 tasks
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.
Closes #6.
All six reported items, plus twelve defects found while auditing them. Eight
commits, each independently landable; the runtime-repair feature is isolated in
its own commit because it is the largest behavioural change here.
The two findings that outrank most of the report
Both affect any deployed version, independently of whether this branch merges.
1. A chatty model self-approved.
APPROVED_REandRUNTIME_PASS_REwereunanchored
.test(body)calls, and the reviewer/runtime prompts contain"VERDICT: APPROVED"and"RUNTIME: PASS"as instructions. Any model thatrestated its instructions was read as approving. Reproduced against the real
regexes: a document ending
VERDICT: CHANGES REQUESTEDread as APPROVED.readVerdict()now takes the last line-anchored verdict, treats a missing oneas blocking, and fails closed when a line names both outcomes.
2. Post-approval code landed unreviewed. Round 3 runs as the coder agent
with the worktree writable; "do not edit implementation files" is an
instruction, not a gate. An edit inside
write_scopepassed the scope re-checkand was folded into the closed commit by an unguarded amend — violating the
report's own stated invariant that modified code always invalidates prior
review. Any non-document change after the last approving review now invalidates
it.
Reported items
write_scope(comment 2)git addENOTDIR(comment 1)--git-common-dir, resolved againstROOT(it returns a relative path from the repo root; resolving against cwd passes tests and misplaces the lock in the field). Acquired with'wx'so two conductors cannot both win the check-then-write racereviewFailureFeedback(): non-approved only, bounded per-document and overall,(missing)never inferred as approval, fenced inBEGIN/END UNTRUSTEDmarkers--max-ticketsbounds successes--max-processedadded, charged on the claim;landed/processed/resumed/stopReasoninconductor.endOne deliberate deviation, documented in the commit and README: the report
asks that the failed runtime report be committed separately so a no-op repair
cannot masquerade as a source change. The candidate must land as a single
commit, so the no-op check instead asks which non-document files the repair
touched — documents are excluded by construction. Same guarantee, no extra
commit.
Found while auditing
matches_scope()enforced "a bare*/**is refused outright" with a blacklist of four literal strings.
**/*,*/**,*/*,?*/**and[a-z]*/**each authorised every nested path inthe repo. Now the first path segment must be a literal name.
R src/auth/important.ts -> a/moved.tspassed a
write_scopeofa/**while having deleted a file outside it.close()— the load-bearing gate — was the one unbounded loop. Notimeout (an unattended run hangs forever on a watch-mode runner), and a 1MB
maxBufferthat makesexecSyncthrow when a command exiting zerosimply prints a lot — refusing a green ticket and reporting "did not exit 0",
which was untrue: there was no exit code at all.
throwescapedrunSession,executeTicketandmain(), leaving the ticket claimed andreleased by nobody. And
STOPwas read only between tickets, so it wasignored for the duration of a backoff that doubles to 60m.
rate-limit retry (item 4 asks that they be separate and both bounded).
is "biased toward firing": deletions were invisible (git renders them
+++ /dev/null, so deleting an auth file recruited nobody), and anunroutable declared reviewer vanished silently.
own non-zero exit was trusted while the equivalent FAIL got a deterministic
re-run.
classifyRuntimeVerdict()had implemented the rule since P-A9 and nogate path ever called it.
MERGE_HEADset — so the nextrun refused to start on a dirty tree the conductor itself created.
runVerifyDirecthad nomaxBuffer;a typo'd numeric flag became
NaNand exited reportinglanded=0,indistinguishable from an empty board.
Test harness
validate-scope.match.test.shhas existed since v3.8.0, is cited inRELEASE_TRACKER.mdas proof the matcher is "fixture-backed", and nothing ranit — while carrying its own copy of
matches_scope, so it could only failwhen the copy changed. A sweep found six more orphaned suites (45 tests),
including the JIRA board driver's entire unit, parity and integration coverage.
All green as found, so nothing was broken; they were unprotected — the state
conductor.test.mjswas in while it stayed red across three releases, which isthe history that caused Pass 53 to exist.
Pass 53 now discovers every
*.test.mjsunderscripts/, with two vacuityguards. A list has to be updated by whoever adds a suite and fails silently when
they don't.
Evidence
new concurrency; working tree clean after.
this branch — every prior conductor fixture runs
--rounds 1. The newconductor.hardening.test.mjsdrives the real 3-round loop with a statefulstub.
one exception being the STOP-during-backoff case, which against unfixed code
sleeps out the full 5-minute backoff and hits the harness timeout.
Not included: no version bump or CHANGELOG entry.
CHANGELOG.mdstops at 3.5.4while
package.jsonsays 3.9.0, so recent releases clearly go throughdocs/RELEASE_TRACKER.mdas a separate deliberate act — that call is themaintainer's.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JDSdRmtXCgkHtZ9XVgLXoZ