Tell a delegating audit domain to block, not end its turn - #700
dormouse-bot wants to merge 3 commits into
Conversation
Run 35327271988's application-security domain fanned out to eight nested agents, issued its wait loop with run_in_background: true, said it was holding for four outstanding work streams, and ended its turn at 09:11:56. Ending the turn is how a subagent completes, so the orchestrator saw the task finish with no sentinel; all four streams returned by 09:19:29, inside a deadline that ran to 09:33:24, and every result was discarded. That is the orchestrator's own section 2 failure one level down, and a domain never reads the file carrying that rule. Put the rule in _preamble.md, which every domain does read, and widen the spec's audited version past the orchestrator. Also stop the merge placeholder and the Summary from naming a cause nobody observed: this run published "cut off at the 32-minute deadline" twenty-one minutes wrong.
Deploying mouseterm with
|
| Latest commit: |
3ad55ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99f3656a.mouseterm.pages.dev |
| Branch Preview URL: | https://daily-review-runs-3534430187.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full review. The rule is the right one and the transcript evidence behind it holds up; node --test scripts/security-audit.test.mjs is 40/40 here and the 1900 → 1950 re-baseline matches the ratchet (1916 words, ceil to 1950). Three things I'd change.
The delegate wait loop has no bound. §2 of orchestrator.md persists its deadline to $RUNNER_TEMP/audit-deadline, and this spec's first Orchestration FAIL IF requires exactly that — "re-issued under a bounded 32-minute deadline persisted to a file … rather than recomputed from now". The preamble's template has only the per-call CALL_END, and tells the domain to re-issue "for as long as your caller is still waiting on you" — which the domain cannot observe. A domain that follows this literally keeps re-issuing until the job's timeout-minutes: 40 cancels the run: it never rewrites its VERDICT: line and never writes the sentinel, so its synthesis dies exactly as run 35327271988's did, just from the other side of the same wait. The inline suggestion mirrors §2; the bound itself is your call, but it has to land far enough inside the orchestrator's 32 minutes that the domain can still synthesize and close.
The block returns no answer on the success path. §2 ends with echo "$ANSWER" and the prose "The call's last line is its answer, and it is what you act on"; this template prints only on the STILL WAITING break, so a call where every delegate finished comes back with empty output — which is also what a call moved to the background returns. §2 carries a whole paragraph on telling those two apart precisely because the agent can't. Same suggestion covers it.
The workflow's fallback arm still asserts the cause this PR removes from orchestrator.md. In .github/workflows/security-audit.yaml, the no-merged-report arm writes _Incomplete — this domain was still writing when the run ended._ — the same unobservable claim, on the path that publishes when the orchestrator itself never reached §3. Its own comment above the line calls it "the same marker §3's emit writes", and after this PR that is no longer true. Fixing it means updating the two security-audit.test.mjs fixtures that pin the wording (the '## audit-ci-secrets.md\n\n_Incomplete — …' note and its counts key, plus the regex in the no merged report publishes the fragments arm). I haven't pushed anything since this is a draft — say the word and I'll take it.
The wait template had only a per-call break, so a domain following it literally re-issues until timeout-minutes cancels the job - the same lost synthesis from the other side of the wait. Mirror section 2: a deadline persisted to $RUNNER_TEMP, 25 minutes so it lands inside the orchestrator's 32 with room to close the fragment, and an answer line on every path so ALL FINISHED is distinguishable from a backgrounded call that printed nothing. Also name the until loop as sanctioned where the bare-sleep ban would otherwise read as forbidding it, stop claiming the transcript shows which loop body was backgrounded, and carry the placeholder fix into security-audit.yaml's own no-merged-report arm, which asserted the same unobservable cause. Both fixtures that pin its wording move with it; reverting either turns the suite red.
|
All three taken in
|
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full review. The three earlier points are taken, and the checks reproduce here: node --test scripts/security-audit.test.mjs is 40/40, node scripts/spec-lint.mjs is clean apart from the two standalone/sidecar/node_modules hits, and reverting the workflow arm's sentence does turn the suite red. Three things about the new template's deadline, all of them in the four lines that compute it.
One deadline file for every domain. $RUNNER_TEMP/delegate-deadline is a fixed path, and all three domains are subagents of one headless session in one job — same runner, same $RUNNER_TEMP, same CWD. The rule is addressed to every domain ("If you delegate…"), so whichever delegates first writes the deadline the others then read. A domain that starts delegating after that file's 25 minutes have passed breaks on DEADLINE in its first iteration and writes up with nothing from its delegates — the loss this PR exists to stop, arriving through the fix. Naming the file after the fragment separates them; that's in the first suggestion.
The 25 minutes is anchored at the wrong clock. The caller's 32 runs from the orchestrator's first wait call; the domain's 25 from its own, which comes after the orchestrator spawned it and it fanned out. In the run this PR is built on that gap was ten minutes: the orchestrator's first wait was 09:01:24 (deadline 09:33:24) and application-security issued its wait at 09:11:50, so a 25-minute bound would have landed at 09:36:50 — three and a half minutes past the caller, not "inside your caller's own bound with room left to rewrite your verdict line and close the fragment". The bound that holds is the caller's own, and it is on disk at the path the spec's first Orchestration FAIL IF already names ($RUNNER_TEMP/audit-deadline); reading it and subtracting the close-out margin makes the sentence true, with the 25 minutes left as the fallback for the case where the orchestrator has not started waiting yet. Two suggestions, one on the block and one on the prose that describes it.
$RUNNER_TEMP is unset when the preamble runs locally. scripts/security-audit-local.sh feeds _preamble.md straight to claude -p and exports only GITHUB_REPOSITORY, so DEADLINE_FILE becomes /delegate-deadline. For a non-root operator the write fails, DEADLINE is empty, [ "$NOW" -ge "" ] errors and tests false, and the wait has no bound at all — every call returns STILL WAITING and the prose says to re-issue it. For a root one (a container) the write lands at the filesystem root and survives between hand-runs, handing the next run an expired deadline: precisely the failure §2's comment rejects a repo-root fallback for. §2 can argue $RUNNER_TEMP needs no fallback because "this file only ever runs there" — the preamble is the file that doesn't have that property. I'd fix it in the local script rather than the template: export RUNNER_TEMP="${RUNNER_TEMP:-$(mktemp -d)}" beside the GITHUB_REPOSITORY export gives a hand-run the same fresh-per-run directory Actions gives, with nothing surviving into the next one. It's outside the diff, so no suggestion for it — say the word and I'll push it with the rest.
The template's deadline file was a fixed $RUNNER_TEMP/delegate-deadline, and all three domains are subagents of one headless session on one runner - so whichever delegates first would set the bound for the rest, and a domain starting later would break on DEADLINE in its first iteration and write up with nothing from its delegates. Name it after the fragment. Its 25 minutes was also anchored at the wrong clock: the caller's 32 runs from the orchestrator's first wait, the domain's from its own, which is after the spawn and the fan-out. In run 35327271988 that gap was ten minutes, so 25 would have landed three and a half minutes past the caller. Read the caller's deadline from the path section 2 already persists it to, less three minutes to close the fragment, and keep the 25 as the fallback for a caller that has not started waiting yet. The prose that described the old anchor moves with it. Both properties are pinned in security-audit.test.mjs. Mutation-checked: a fixed deadline name or a dropped caller read each turn the suite red (40 pass -> 39 pass / 1 fail). security-audit-local.sh now exports RUNNER_TEMP when Actions has not, since it feeds _preamble.md straight to claude -p. Without it the path is /delegate-deadline-...: unwritable for a normal user, which leaves the wait unbounded, and persistent across hand-runs for a root one, which is what section 2 rejects a repo-root fallback for. A fresh mktemp -d per run gives a hand-run what Actions gives.
|
All three taken in
Both new properties are pinned in |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full review. The caller-anchored deadline is the right fix and its arithmetic holds: running the block with its placeholders filled, a seeded $RUNNER_TEMP/audit-deadline at now + 1920 persists exactly caller - 180, no caller file persists now + 1500, and an expired caller answers DEADLINE on the first iteration. node --test scripts/security-audit.test.mjs is 40/40, node scripts/spec-lint.mjs reports only the two standalone/sidecar/node_modules hits, and the 1900 → 1950 re-baseline is required (1916 words, ceil to 1950). Two things, one of them a comment.
The delegate block is the one shipped block nothing executes, and the bound is what that costs. The spec's Source of truth line says "the wait and merge blocks run as shipped in scripts/security-audit.test.mjs", and the test file's own comment says "Every consumer below is pinned by executing the shipped text" — but this block is pinned by five text regexes, so its wording is checked and its behavior is not. Two mutations, each reinstating a bug an earlier round of this PR found, leave the suite at 40/40:
CALLER - 180→CALLER + 180puts the domain's deadline after its caller's. That is the wrong-clock failure3ad55ff4exists to fix, and it comes back silently.- Deleting
[ "$NOW" -ge "$DEADLINE" ] && { ANSWER="DEADLINE"; break; }restores the unbounded loop from the first round — the domain re-issues until the job'stimeout-minutes: 40cancels the run.
Contrary to the new comment's "The block is a template, not runnable", it is runnable as shipped: it has exactly two placeholders, and filling them gives the same kind of text runWait already executes for §2.
// And run it: the regexes above pin the rule's wording, not the bound. Filling
// the block's two placeholders makes it the same runnable text §2's wait is
// tested as, which is what catches the margin pointing the wrong way or the
// `DEADLINE` break going missing.
test('the delegate wait bounds itself by its caller\'s deadline', (t) => {
const block = promptShellBlock(readFileSync(join(repo, '.github/audit/_preamble.md'), 'utf8'), 'delegate-deadline-')
.replace('<your fragment>', 'audit-application.md')
.replace("<every delegate's output file is complete>", '[ -f delegates-done ]');
const runDelegateWait = (caller, done) => {
const dir = tempDir(t, 'dormouse-audit-delegate-');
if (caller !== undefined) writeFileSync(join(dir, 'audit-deadline'), `${caller}\n`);
if (done) writeFileSync(join(dir, 'delegates-done'), '');
const r = spawnSync('bash', ['-c', block], { cwd: dir, encoding: 'utf8', env: { ...process.env, RUNNER_TEMP: dir }, timeout: 10_000 });
assert.equal(r.status, 0, r.stderr);
return { answer: r.stdout.trim().split('\n').at(-1),
persisted: Number(readFileSync(join(dir, 'delegate-deadline-audit-application.md'), 'utf8')) };
};
const now = Math.floor(Date.now() / 1000);
// The caller's own bound, three minutes early — not the domain's own clock.
assert.deepEqual(runDelegateWait(now + 1920, true), { answer: 'ALL FINISHED', persisted: now + 1920 - 180 });
// No caller deadline on disk yet: 25 minutes from here.
assert.equal(runDelegateWait(undefined, true).persisted - now, 1500);
// An expired caller means stop now and write up, not wait out the call cap.
assert.equal(runDelegateWait(now - 1, false).answer, 'DEADLINE');
});I ran all three states here: added alongside the existing test it is green as shipped (41/41) and red on each mutation above (40 pass / 1 fail each). Say the word and I'll push it — the branch is untouched since this is a draft.
The second is inline: the new RUNNER_TEMP export guards a path no local run reaches, and its comment says otherwise.
| # `_preamble.md`'s delegate wait persists its deadline under `$RUNNER_TEMP`, | ||
| # which only Actions sets. A fresh directory per run gives a hand-run the same | ||
| # property CI has — nothing from the last run survives into this one — where a | ||
| # repo-root fallback would hand the next run an expired deadline. |
There was a problem hiding this comment.
run_domain passes --disallowed-tools "Task,Agent,Workflow" on both paths, and §2 of orchestrator.md records that this script "runs the domains directly and never the orchestrator" — so no local domain can delegate, nothing here ever reads $RUNNER_TEMP/audit-deadline, and the delegate block this export exists for is unreachable locally. As written the comment claims a hand-run needs it, which sends the next reader looking for a local failure that cannot happen; it also leaves an empty mktemp -d behind on every run, including the early exits above. Keeping the export as insurance against that tool list changing is reasonable — the comment just has to say that is what it is. Dropping both is the other defensible option.
| # `_preamble.md`'s delegate wait persists its deadline under `$RUNNER_TEMP`, | |
| # which only Actions sets. A fresh directory per run gives a hand-run the same | |
| # property CI has — nothing from the last run survives into this one — where a | |
| # repo-root fallback would hand the next run an expired deadline. | |
| # `_preamble.md`'s delegate wait persists its deadline under `$RUNNER_TEMP`, | |
| # which only Actions sets. No domain reaches that block here — `run_domain` | |
| # denies `Task`/`Agent`, so a local domain cannot delegate, and the | |
| # orchestrator never runs locally — but a fresh directory per run keeps the | |
| # wait bounded if that ever changes, where a repo-root fallback would hand the | |
| # next run an expired deadline. |
1 participant
The nightly
security-audithas held the release gate shut for tenconsecutive days. #682 fixed
the part where the
application-securitydomain lost everything it had found;this run shows what is left. Its fragment now carries 23 KB of real findings —
that fix works — but it still has no sentinel and no verdict, and the cause is
not the deadline.
What happened in 35327271988
application-securityfanned out to eight nested agents. At 09:11:50 it issuedits wait loop with
run_in_background: true, which returns an id and blocksnothing; at 09:11:52 it wrote "Four work streams are still running … I'm
holding for them before assembling the final fragment"; at 09:11:56 it ended
its turn. Ending the turn is how a subagent completes, so the orchestrator's
task_notificationfor it readcompletedwith the sentinel never written.The four streams it was holding for returned at 09:13:00, 09:13:04, 09:13:45
and 09:19:29 — all inside a deadline that did not expire until 09:33:24. None
of their results reached the report.
That is the run 32618922852 failure that
orchestrator.md§2 exists toprevent, occurring one level down. §1 tells the orchestrator not to read the
domain files, and a domain is given
_preamble.mdplus its own file — so therule that would have caught this is in the one file the failing agent never
sees.
The change
.github/audit/_preamble.mdgains the rule, in the file every domaindoes read: if you delegate, block in a Bash call, never with
run_in_background, break under the ten-minute cap, re-issue withtimeout: 600000, and bound the whole wait with a deadline persisted to$RUNNER_TEMP— 25 minutes, so it lands inside the orchestrator's 32 withroom to close the fragment. The block prints its answer on every path, as
§2's does.
docs/specs/security-audit.mdwidens the Orchestration statement pastthe orchestrator and extends the domain-prompt
FAIL IFto cover adelegating domain that waits by ending its turn or backgrounding its loop.
Evidence in the rationale; budget re-baselined 1900 → 1950 in this PR.
scripts/security-audit.test.mjspins all three parts of the new rule.Verified by mutation: weakening the preamble sentence turns the suite red
(40 pass → 39 pass / 1 fail).
.github/audit/orchestrator.mdand.github/workflows/security-audit.yamlstop asserting a cause nobodyobserved. The merge placeholder said "still writing when the deadline
passed", the workflow's own no-merged-report arm said "still writing when
the run ended", and the Summary published "cut off … at the 32-minute
deadline" — twenty-one minutes wrong, for a domain that had ended its own
turn.
node --test scripts/security-audit.test.mjsis 40/40 andnode scripts/spec-lint.mjsis clean apart from the twostandalone/sidecar/node_modulespath hits that every dependency-free checkout reports.
Opened as a draft per
AGENTS.md. Found by the nightlyreview-runssweep'slive-work scan; tracker: #598.
Transcript evidence
From
audit-transcript/_temp/claude-execution-output.json:subagent_stats: 13 spawned,max_depth: 3, 10 spawned by subagents.application-securitytool call, 09:11:50.922Z:{"description": "Wait until all audit fragments stop changing", "timeout": 600000, "run_in_background": true}→"Command running in background with ID: bcokmjllb".task_notificationforaa7328a3c2c624016(application-security),09:11:56.294Z,
status: "completed".09:19:29.976Z.
gave up at 09:33:36.718Z.
audit-application.md: 23,687 bytes,VERDICT: INCONCLUSIVEon line 1, zerooccurrences of
<!-- END OF REPORT -->.