Repository navigation
fix: a turn that strands a background task must not report success - #70
Merged
Merged
Conversation
subtype:'success' is the one rung the auto-continue ladder does not cover. A run that started something with run_in_background and then ended its turn saying it would wait and continue reports a clean end_turn, so the ladder — which fires only on NON-success — never saw it. Nothing resumes a headless `claude -p`: the process exits and takes the background shell with it. The chat printed "Done" over work that had not happened, and the continuation the agent promised could never arrive. run-continuation.js holds both post-run decisions, pure so they can be pinned without a server or an SSH host: - shouldNudgeBackgroundWait() spends ONE run harvesting the task, and only for a launch in the last run. Detection is structural (a Bash call carrying run_in_background), never textual — user-facing prose is written in the UI language, so an English regex is dead on a French install. - describeTurnBudgetAnomaly() names an error_max_turns that did not come from our budget. Measured on CLI 2.1.231 a run capped at N reports num_turns === N + 1, so a stop at 3 of 50 is a cap imposed elsewhere and "raise Max turns" is actively wrong advice (#67 follow-up). BACKGROUND_TASK_INSTRUCTION states the constraint in the system prompt, so the harvest run is the second line of defence rather than the first. Also: the SSH auto-continue path now logs (it logged nothing at all, on either branch) and names the budget and what was spent, which is what the local CLI loop has always done.
Two independent reviewers (codex, agy) and a live A/B run all landed on the same defect in ebf439f: `bgLaunched` was a one-way latch, so every agent that OBEYED BACKGROUND_TASK_INSTRUCTION — launch, collect, finish — was charged a rescue run for its obedience. Worse than the bug it fixed. - Both loops now count bgLaunches/bgHarvests per run and nudge only when launches > harvests. Verified live: an obedient agent reports launches=1 harvests=1 and is not nudged; one that walks away reports launches=1 harvests=0 and is. - isBackgroundHarvest() cannot key on BashOutput alone. Measured on CLI 2.1.231, a background launch answers "Output is being written to: <hash>/<uuid>/tasks/<id>.output" and the agent collects by READING that file; BashOutput is never called. - isBackgroundLaunch() reads the flag off a PARSED object. As a substring match, `grep -rn '"run_in_background": true' *.js` — a real command in this repo — registered as a launch. The regex survives only for input that does not parse at all. - A bounded nudge needs an honest ending: when the rescue run also walks away, describeStrandedBackgroundTask() says what was left running and the loop returns completed:false, so taskWorker does not mark a Kanban task done over unfinished work. - describeTurnBudgetAnomaly() is applied to the FIRST retry notice too. "hit the 50-turn limit (used 3)" contradicts itself, and a user who stops reading after the first retry never reached the corrected sentence. Known limit, documented: shell-syntax backgrounding (`cmd &`, `nohup`) inside a foreground Bash call is not detected — separating it from `a && b` and `2>&1` needs a shell parser. The system-prompt rule covers it in words.
Round 2 of independent review (codex, grok) found the bound from 87c8da8 was
dead in the loops, and a live two-run test reproduced it: the counters lived
inside runOnce, so the rescue run started from zero. Its most likely shape is a
text-only answer that touches no tool at all -> launches=0, harvests=0 ->
describeStrandedBackgroundTask() stayed silent and the turn reported success.
That is the original "Done over unfinished work", one run later.
- The debt now lives in the loop body: bgLaunches, a Set of harvested shell ids,
and an anonymous-harvest counter, read through bgOutstanding(). Verified live:
run 1 launches and walks away -> outstanding=1 -> nudge; run 2 answers in text
and touches nothing -> outstanding still 1 -> stranded notice instead of Done.
- Harvests dedupe by shell id (from BashOutput/KillShell, or out of the
tasks/<id>.output path). Two polls of one job used to cancel a second launch
that really was abandoned. backgroundOutstanding() clamps at zero so a poll of
something started in an earlier turn cannot mask a later launch.
- `View` is accepted alongside `Read`: it is the name in this project's own
allowedTools lists, so an agent using it looked like it collected nothing.
- The stranded notice is a real `---` status line. statusLineKind()
(public/index.html:7215) needs that fence, or the SPA paints its own Done badge
over the warning.
- completed:false is REVERTED. It has no consumer that does the right thing: the
chat path discards it, the API Kanban worker never calls these functions, and
the one reader (server.js:1859, the subscription branch) turns it into
{subtype:'error'} -> taskStatusForStop -> 'failed', which auto-retries a chain
and re-runs every side effect. CLAUDE.md claimed otherwise; corrected.
Round 3 of independent review (codex, grok) found the batch total
max(0, launches - harvests) banked a harvest that had no launch behind it.
Reading a leftover tasks/<id>.output from an earlier turn — often the first
thing a resumed session does — then launching a shell and walking away came
out at zero owed: no rescue, no warning, "Done" over a running job. The
sibling case is launch A and B, collect only B.
The debt is now incremental and the whole rule lives in the module:
applyBackgroundTool(state, name, input) folds one tool call into {debt, seen},
a harvest decrements only an EXISTING debt and only once per shell id, and a
surplus is dropped. Both loops call that one function instead of open-coding
the accounting, which is how the local and remote paths drift apart.
An unidentifiable harvest (BashOutput with no readable bash_id) now returns
null and does not pay the debt. bash_id is required, so it only happens on a
malformed payload; crediting it re-opened the raw-call-count hole. One extra
rescue run is the cheaper mistake.
Verified live against the real CLI, all three shapes in one script:
obedient (launch + collect) -> debt=0, no nudge
walk away, then text-only rescue -> debt=1, stranded notice, not "Done"
launch two, collect one -> debt=1, nudge
Also restores the KNOWN LIMIT note (shell-syntax backgrounding: `cmd &`,
nohup) to the module header, where it was lost in the previous rewrite.
Round 4 (codex FAIL / grok PASS) converged on one residual hole and one stale sentence. Neither is a regression; both are now written down where the next reader will hit them. - A harvest is not PAIRED to a launch: the shell id lives only in the tool RESULT, and onTool sees tool_use blocks. So a Read of some OTHER tasks/<id>.output after a launch pays that launch's debt. The mirror case, a leftover read BEFORE the launch, is closed by the no-banking rule. Closing this one means threading tool_result through both transports and changing the stream contract, for a case that needs the agent to read an unrelated shell log in the same turn it abandons a real one. - CLAUDE.md still named bgLaunches/bgHarvestIds/bgAnonHarvests in the ask-user-question closure list; the test passes bgState.
buildSystemPrompt() is called from exactly two places — the chat path and processTelegramChat. taskWorker (Kanban/scheduled), the multi-agent DAG members and the bots/dispatch path each build their own system prompt AND run their own cli.send loop, so they got neither the instruction nor the harvest rescue. An unattended task that walks away from a background job is the worst version of this bug: nobody is reading that chat to notice the "Done". Those three now get BACKGROUND_TASK_INSTRUCTION — one concatenation each, no behavioural risk. The rescue itself stays in runCliSingle/runSshSingle: extending it would mean duplicating the debt accounting into a third and fourth loop, which is exactly how the local and remote paths would drift apart. Documented as uneven coverage rather than left implicit.
Round 5 (codex) showed the previous commit was a no-op for two of the three paths it claimed to cover. claude-cli.js:252 drops --system-prompt whenever there is a session to resume, and: - a multi-agent worker starts from the orchestrator's session id (agentSessionId = currentSessionId), so agentSp never reaches it; - a bot keeps a persistent session per chat, so botSp reaches it on the first turn only. That path already had the fix for this shape — the `standing` block re-sent in the USER turn — and the instruction now goes there. The multi-agent instruction moves from agentSp to agentPrompt for the same reason. taskWorker's taskBotSp keeps it as a system prompt: that path does send one on a fresh run. Documented as still NOT covered: a Kanban task on the subscription engine passes systemPrompt:'' to runInteractiveSingle, and changing that respawns the tmux session — out of scope for this fix rather than silently claimed. Not a defect, checked: BOT_PROMPT_MAX (8192) validates the user-supplied field at the API boundary, not the assembled prompt, which has always been larger.
Round 6 (codex) found taskWorker coverage was true only for a bot-owned task on a fresh session. taskBotSp is undefined for a task with no bot_id, and claude-cli.js:252 drops --system-prompt when the task resumes an existing session — so an ordinary scheduled task got nothing at all. BACKGROUND_TASK_INSTRUCTION moves to the task prompt, next to TASK_VERIFICATION_SUFFIX, which lives in that channel for exactly this reason. Removed from taskBotSp, where it would now duplicate on the one path that has both. Net effect of rounds 5-6: no path gets this as a system prompt except chat and Telegram, the two that go through buildSystemPrompt. Everything else takes the user turn, which --resume always delivers.
Round 7 (grok) found the code holds but three doc claims did not, and — the part that matters — that nothing pinned WHICH expression the constant is concatenated onto. Rounds 5-7 each shipped a no-op green: appended to a system prompt that --resume drops, to one that is undefined for a bot-less task, and to one a bot sees only on its first turn. test/run-continuation.test.js now reads server.js as source text (the technique test/render/script-scope.test.mjs already uses, because the property is textual) and pins all five channels: buildSystemPrompt, the task prompt, the multi-agent worker prompt, the bots standing block, and the NEGATIVE case — taskBotSp must NOT carry it, since its presence there reads as coverage it cannot deliver. Doc corrections: - "chat and Telegram are the only paths that get it as a system prompt" was false: a bot on a FRESH session gets it via botSp too. - "the subscription Kanban engine is still not covered" became false with the round-6 move. It passes systemPrompt:'' but runInteractiveSingle types the prompt into the tmux pane (claude-interactive.js:441), so the user-turn channel reaches it like every other. - server.js:1823 -> 1829 after the comment added in round 6.
Round 8 verdict was PASS, with one leftover of the same family it had just confirmed closed: botSp is documented as a delivery channel and was the only one not pinned. Deleting the constant from it leaves a brand-new bot with an empty `standing` block and no copy anywhere, and the suite still went green — which is exactly the failure mode rounds 5-7 kept producing. Also corrects a stale citation: the `completed` reader is server.js:1865, not 1859 (that line is now the fullText branch above it).
Behaviour-preserving. runCliSingle/runSshSingle each carried thirteen copies of
the same three lines — append to fullText, append to chatBuffers with a
MAX_CHAT_BUFFER tail-trim, ws.send a {type:'text'} frame with a conditional
tabId spread. Every copy had to get the trim and the spread right; one that
missed either grew the buffer without bound or delivered into the wrong tab.
emitTurnNotice() now owns the buffer write and the frame. The caller still does
its own `fullText += notice` first, so the ORDER of effects is unchanged —
folding that in moved the append after the send, which two reviewers flagged as
a real (if currently unobservable) difference.
restartAvailable is a named boolean, not a free-form `extra` object. An extra
spread sat between `text` and `tabId` and would have let a future caller
overwrite `type`/`text` or leak a tabId the frame is not meant to carry; the
frame is now built key by key, so its order is fixed by construction.
Deliberately NOT converted, and said so in the header:
- the onText handlers, which stream model output rather than a status notice and
also write stmts.setPartialText every fifth chunk;
- runners outside these two loops (taskWorker, multi-agent members, bots), which
buffer into taskBuffers, tag frames with `agent:`, or persist only.
Also: _anom and _anomaly were two names for one value in one function; the
backgroundOutstanding(bgState) call is hoisted into _owed where it was read
twice on adjacent lines.
notice boilerplate blocks 28 -> 2 (the two that remain are onText)
MAX_CHAT_BUFFER repeats 56 -> 4
lines across both loops 698 -> 674
Reviewed in two rounds by codex and grok (non-Claude). Round 1 found the leaky
`extra`, a half-done conversion leaving two styles in one function, the moved
fullText append, and a comment that claimed more than was done. All four fixed;
round 2 returned PASS from both.
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
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.
What this fixes
Two defects in the single-agent run loops, reported against v7.9.0.
1. A turn that strands a background task reported success
An agent that started something with
run_in_backgroundand then ended its turn — "I'll wait for it and continue" — reports a cleanend_turn, i.e.subtype:'success'. That is the one rung the auto-continue ladder does not cover: the ladder fires only on NON-success. Nothing else resumes a headlessclaude -p— the process exits and takes the background shell with it — so the chat printed✅ Doneover work that had not happened, and the continuation the agent promised could never arrive.run-continuation.jsadds two pure decisions, and the loops spend at most one extra run collecting the task.Detection is structural, never textual. A
Bashcall carryingrun_in_backgroundis the same JSON on every install; user-facing prose is written in the UI language (buildSystemPromptpins that explicitly), so a regex over English phrases like "I'll check back" is dead on a French or Ukrainian one.The debt is incremental and turn-level. Four simpler shapes were tried first and every one of them shipped green while still being broken — they are listed in the module header and each has a test named after it:
tasks/<id>.outputpaid for a launch that came afterwardsapplyBackgroundTool(state, name, input)folds each tool call into{debt, seen}: a harvest decrements only an existing debt, once per shell id, and a surplus is dropped.The harvest side cannot key on
BashOutput. Measured on CLI 2.1.231, a background launch answersOutput is being written to: <hash>/<uuid>/tasks/<id>.outputand the agent collects by reading that file —BashOutputis never called.A bounded nudge needs an honest ending. When the rescue run also walks away, the turn says what is still running, as a real
---status line (statusLineKind()needs the fence, or the SPA paints its own Done badge over the warning).2.
error_max_turnsfar below the requested budget no longer says "raise Max turns"Measured against CLI 2.1.231, a run capped at N reports
num_turns === N + 1, so a genuine exhaustion lands at the cap. A stop at 3 turns against a 50-turn dial is a cap imposed somewhere else — a different CLI version, asettings.json, a hook on the machine the agent runs on — and sending that user to the dial is actively wrong advice. Applied to the first retry notice as well as the exhausted one:hit the 50-turn limit (used 3)contradicts itself.The SSH auto-continue path also now logs (it logged nothing at all, on either branch) and names the budget and what was spent, which the local CLI loop has always done. Relates to #67.
Delivery channels
--system-promptis dropped whenever there is a session to resume (claude-cli.js:252), and is absent entirely for a task with no bot. So each runner takes the channel that actually reaches it:buildSystemPromptTASK_VERIFICATION_SUFFIXagentPrompt(they resume the orchestrator's session)botSpon a fresh session, thestandinguser block once resumingThe harvest rescue stays in
runCliSingle/runSshSingle; duplicating the debt accounting into a third and fourth loop is how the local and remote paths drift apart. The unattended runners get the instruction only — which is the half that matters most there, since nobody is reading that chat.Known limits, both documented in the module header
onToolsees tool_use blocks. The mirror case (a leftover read before the launch) is closed by the no-banking rule.cmd &,nohup) inside a foregroundBashcall is not detected — separating it froma && band2>&1needs a shell parser. The system-prompt rule covers it in words.Verification
npm test→ 70 files, exit 0. Newtest/run-continuation.test.js(69 checks); every wrong shape above has a test named after it.BACKGROUND_TASK_INSTRUCTION, plus a negative pin thattaskBotSpmust not. Three commits in this branch were green no-ops before those pins existed.debt=0, no nudge; walk away then text-only rescue →debt=1, stranded notice instead of "Done"; launch two collect one →debt=1, nudge.