fix(subagent): re-dispatch a read-only child once when its stream is cut with zero output - #796
fix(subagent): re-dispatch a read-only child once when its stream is cut with zero output#796griffinwork40 wants to merge 3 commits into
Conversation
…cut with zero output
A forked sub-agent whose model stream is closed mid-generation by an
intermediary dies for good. The provider loop re-drives such a cut twice per
tool-use round (STREAM_INCOMPLETE_MAX_RETRIES), but once that budget is spent
nothing above it retries: handle.ts:555 throws StreamIncompleteError whose own
message states the intended contract ("the parent should retry or fall back"),
and no layer implemented it. dag.ts has no retry at all. The parent just
receives an isError payload and only the parent MODEL can choose to re-call.
Forensics over 12,478 witness traces (115 failures / 74 traces, against a
2,997-child control set):
- 100% of failures ran >=360s (median 1119s ~19min); succeeding children run
a median of 184s. No failure has ever occurred under 6 minutes — this is a
duration-exposure effect, not a model or concurrency effect.
- Nested orchestrator children are 5x enriched (40.9% of failures vs 8.2% of
successes), each absorbing up to MODEL_CAP_BYTES=100_000 per nested return.
But 59% of failures are leaves, so that is a multiplier, not the mechanism.
- The rate rose ~6.7x per-dispatch on top of a 6.5x volume increase while the
orchestrator share DECLINED, and no commit touched providers/ or subagent*
in that window — consistent with an external gateway, not a regression.
Adds subagent/stream-cut-retry.ts: a pure decision module plus a wrapper that
re-runs the full executeOnce body, so a re-dispatch forks a genuinely fresh
child (new session, trace, worktree) rather than re-awaiting a spent handle.
Safety is gated twice, because "zero output" is NOT "zero side effects":
handle.ts accumulates streamed text (:413) and tool calls (:415) separately,
and :513-515 names tool-loops-without-a-final-turn as reaching this branch. A
write-capable child could therefore have written files, committed, or POSTed
before the cut while reporting zero output, and re-running its prompt would
double-fire all of it. So:
1. childWriteCapable must be false (child-config.ts:204 — already folds in
the agent tool allowlist, the parent cage, and readOnlyBash). Only
provably side-effect-free children are re-dispatched. This also removes
the stranded-dirty-worktree hazard, since isolated worktrees are created
only for write-capable children.
2. A monotonic cancel generation must not have moved. Across the retry gap
both in-flight handle maps are empty, so cancelActiveForeground() finds
nothing to cancel and never aborts call.signal; without the counter a
cancel during the settle delay would be ignored and a child forked anyway.
Buffered partials (status:'succeeded' — real findings salvaged) and
tool_use_loop_capped (a requested ceiling) are both excluded from retry.
Budget is 1, deliberately below the provider loop's 2: by this layer the
provider has already spent its per-round budget, so a re-dispatch re-runs a
whole multi-minute prompt and costs far more than a single-round re-drive.
Accepted, documented costs: SubagentStop fires once per attempt and attempt 1's
injectContext is discarded; telemetry emits one subagent.failed row per attempt
with no attempt dimension. Both are inherent to a retry being a second child.
Verified: 723 test files / 13,774 tests pass with coverage thresholds enforced;
tsc --noEmit clean; new module at 100% statements/branches/functions/lines.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19e0ae2219
ℹ️ 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".
| // whole prompt, so it is only safe when the child cannot mutate anything. | ||
| // `childWriteCapable` is the authoritative answer — it already accounts for | ||
| // the named agent's tool allowlist, the parent's cage, and readOnlyBash. | ||
| if (probe !== undefined) probe.sideEffectFree = !childWriteCapable; |
There was a problem hiding this comment.
Gate retries on every side-effecting tool
When a restricted or named child is allowed a non-file mutator such as send_telegram, config_set, create_schedule, or browser_act, it can perform that action, emit no assistant text, and then end with stream_incomplete. This assignment treats !childWriteCapable as side-effect-free, but childWriteCapable only checks write_file, edit_file, and mutating bash; the retry therefore reruns the whole prompt and can duplicate messages, remote actions, schedules, or configuration changes.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
| canRedispatch: () => | ||
| probe.sideEffectFree && this.cancelGeneration === cancelGenerationAtStart, |
There was a problem hiding this comment.
Recheck cancellation after the fresh fork
If cancelActiveForeground() is called after this gate passes but while the second executeOnce() is awaiting forkSubagent()—which can await hooks and read-scope resolution—the handle maps are still empty, so the cancellation only increments cancelGeneration. Nothing checks that generation again after the fork resolves, and the newly created child is registered and run despite the user's cancellation; recheck the generation before starting the foreground branch or make the pending fork cancellable.
AGENTS.md reference: AGENTS.md:L83-L83
Useful? React with 👍 / 👎.
|
@codex fix comments |
Summary
Testing
|
|
Wave 1.5 + shadow-verify complete. All pivotal citations re-derived at the ref; one Code Review — PR #796
Blocking1 ·
|
| claim | status | resolution |
|---|---|---|
"ESC during the retry gap forks a fresh child anyway" (raised high + medium by two agents independently) |
REFUTED | turn-handler.ts:281 fires session.interrupt() unconditionally, before the guard at :297. Chain: agent-session.ts:763-767 → anthropic-direct/query.ts:621-623 abort.requestAbort('interrupted') → abort-coordinator.ts:114-121 controller.abort() — on the same per-turn controller that owns call.signal (query.ts:359 abort.begin() → :428 signal: controller.signal → loop.ts:1014-1019 signal: input.signal). stream-cut-retry.ts:145/152 and subagent-executor.ts:462 all suppress. Residual doc/test defect kept as finding #6 at low. |
| "retry never fires for any shipped read-only agent" | REFUTED (narrowed) | git-investigator (builtins.ts:146) and Explore (builtins.ts:175) both pass the predicate. Finding #1 restated as research-agent-specific. |
agent-session.ts:766 |
path-corrected | File absent at ref; real path src/agent/session/agent-session.ts:763-767. Content correct — not fabricated, finding retained. |
sleepWithAbort rejects on abort |
disproven | Resolves early; no finding. |
What was not checked
- Citations verified inline against branch HEAD
b90271e049cdf655c1891e3ce98b3dcf9d2fc548viagit show <ref>:<path>andgit grep <ref>— everyhighcitation plus everyfile-statecitation at all severities. Two high claims additionally re-derived by independent shadow verifiers. - Stated intent: PR fix(subagent): re-dispatch a read-only child once when its stream is cut with zero output #796 title + body — spec-compliance assessed (finding test: hermetically lock AFK_INTERNAL tier in audience-gate tests #1).
- Did not run
pnpm test,pnpm lint, or coverage, so the PR's 723-files / 13,774-tests,tscclean, and 100%-new-module claims are unverified. - Did not review
dag.tsor the skill/mint dispatch paths (the PR declares them out of scope). - Did not evaluate the Ctrl+C/SIGINT hard-interrupt path (
turn-handler.ts:835-840) or Telegram/daemon soft-stop equivalents — the abort analysis covers ESC/soft-stop in the REPL only. - Did not enumerate user- or plugin-authored agent files, which can set
bash: read-only(agents/parser.ts:188) and reach finding fix(memory): make HOT.md non-fatal — truncation covenant #2.
Merge decision
DO NOT MERGE
One high finding defeats the change's stated purpose for its primary case: research-agent — named in child-config.ts:200-201 as the canonical read-only agent and mandated as subagent_type by this repo's own skills — is silently ineligible for the retry, because the authorization set is READ_ONLY_PHASE_TOOLS, whose contract is pre-approval phase gating and which excludes web_scrape and agent by design. The PR body documents a different, looser gate than the one that shipped. The fix is small and local: a dedicated retry-safety set, plus a test that runs a real registered agent through the predicate.
Done
- Reviewed PR fix(subagent): re-dispatch a read-only child once when its stream is cut with zero output #796 across security, api-compat, correctness, spec-compliance, test-coverage, and perf-observability: 2 parallel Wave 1 agents → inline Wave 1.5 citation/absence verification at the ref → 2 shadow verifiers on the
highclaims; emitted 10 findings, 1 blocking. - Evidence: reviewed ref
b90271e049cdf655c1891e3ce98b3dcf9d2fc548; diff artifact/Users/griffinlong/.afk/state/skill-preflight/0ebee9f3-404f-427b-ab67-4c3c5e25737e/pr-796.diff; every citation re-derived viagit show b90271e:<path>/git grep b90271e; verifier chains cited tofile:lineabove. - What changed in the world: nothing — this skill is read-only. No files written, no commits, no PR comments, no pushes. Working tree remains clean at
b90271e. The review itself is transcript-delivered because the skill forbids file writes; the durable anchors are the ref SHA and the diff artifact path above. - Pending: the review is not posted to the PR and no fix was applied — both are deliberately out of scope. Findings test: hermetically lock AFK_INTERNAL tier in audience-gate tests #1 and fix(skills): enforce read-only tool gating for recon skills (ground-state) #5 are the two that gate a merge; verifying the PR's own test/lint/coverage claims requires a run I did not perform.
🤖 Posted by agent-afk /review --post github
|
@codex fix comments |
Summary
Testing
|
Why
A forked sub-agent whose model stream gets closed mid-generation by an intermediary dies for good. The provider loop re-drives that cut twice per tool-use round (
STREAM_INCOMPLETE_MAX_RETRIES,loop.ts:128), but once the budget is spent nothing above it retries —handle.ts:555throwsStreamIncompleteErrorwhose own message states the intended contract ("the parent should retry or fall back") and no layer implemented it.dag.tshas no retry at all. The parent just gets anisErrorpayload; only the parent model can choose to re-call.The error message itself is not a bug — it is the deliberate anti-truncation guard from #635/#628 (
translate.ts:349) correctly refusing to present a partial as a finished answer.Forensics
12,478 witness traces: 115 failure events across 74 traces, against a 2,997-child control set. Every correlation base-rated.
MODEL_CAP_BYTES=100_000per nested return,_output-cap.ts:47) — but 59% of failures are leaves, so that is a multiplier, not the mechanism.providers/orsubagent*in that window — consistent with external gateway behaviour, not a regression.sonnet.What this does
Adds
src/agent/subagent/stream-cut-retry.ts— a pure decision module plus a wrapper that re-runs the fullexecuteOncebody, so a re-dispatch forks a genuinely fresh child (new session, trace, worktree) rather than re-awaiting a spent handle.forkSubagentcall-count is the load-bearing test assertion.Safety: "zero output" is NOT "zero side effects"
The naive version of this fix — retry whenever
partialOutputBytes == 0— would have corrupted user state, and an adversarial review caught it before merge.handle.tsaccumulates streamed text (:413) and tool calls (:415) separately, and:513-515explicitly names tool-loops-without-a-final-turn as reaching the zero-output branch. So a write-capable child can run dozens of tool calls — file writes,git commit, HTTP POSTs — emit no text, and report zero output. Re-running its prompt would double-fire all of it.Two gates therefore guard every re-dispatch:
childWriteCapable === false(child-config.ts:204, which already folds in the agent's tool allowlist, the parent cage, andreadOnlyBash). Only provably side-effect-free children are retried. This also removes the stranded-dirty-worktree hazard, since isolated worktrees exist only for write-capable children.cancelActiveForeground()finds nothing to cancel and never abortscall.signal— without the counter a user cancel during the settle delay would be silently ignored and a fresh child forked anyway.Also excluded from retry: buffered partials (
status:'succeeded'— retrying would destroy salvaged findings) andtool_use_loop_capped(a requested ceiling, not a transport failure).Budget is 1, deliberately below the provider loop's 2: by this layer the provider has already spent its per-round budget, so a re-dispatch re-runs a whole multi-minute prompt.
Accepted, documented costs
SubagentStopfires once per attempt; attempt 1'sinjectContextis discarded.subagent.failedrow per attempt with noattemptdimension.Both are inherent to a retry genuinely being a second child. Attempt-scoped trace events are kept on purpose so a rescued failure stays visible in
afk trace show.Not in scope
dag.tsnodes and the skill/mint dispatch paths still do not retry. The module is reusable; they can adopt it incrementally.Verification
pnpm test:coverage— 723 files / 13,774 tests pass, coverage thresholds enforced (74/79/82/74).pnpm lint(tsc --noEmit) clean.incompleteToolResultFieldsproducer rather than a hand-rolled fixture.Full diagnosis:
~/.afk/workspace/reports/subagent-stream-cut-2026-07-31.md