Conversation
startsBackgroundWork() decides whether a turn's CLI process is held open past `result`. It matched Bash and the deferred-work set, but not Agent, so a turn whose only outstanding work was a background subagent scored as having nothing pending. stdin was released immediately, the CLI read that EOF as print wind-down, and the agent was killed mid-run — surfacing on the next message as "No completion record was found for background agent". This is a gap in the fix that shipped for siteboon#1113, not a regression: that work predates background agents, and its own notes list subagents as finishing inside a turn, which was true then. Agent gets its own arm rather than a place in DEFERRED_WORK_TOOLS, because that set would also match foreground agents, which never push a follow-up turn and would pin a process for the full BG_WAIT_CEILING_MS. The test is `!== false` rather than `=== true` since `run_in_background` is optional on AgentInput and agents background by default. Workflow does belong in the set: WorkflowInput has no foreground option at all, so every call returns a task id immediately and reports back later. Fixes siteboon#1268 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe Claude runtime now detects background ChangesClaude background work
Suggested reviewers: Priority: ➖ Normal — Schedule this Claude runtime fix because background agents and workflows can be terminated before deferred work completes, a medium-severity product defect. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Background Agent and Workflow calls now keep the CLI alive as intended, while explicitly foreground Agents remain excluded. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each tool, Comment |
There was a problem hiding this comment.
🟡 Changes recommended
It introduces/extends backend-module export and file-type conventions (export-at-declaration and JS-in-server/modules/) that should be addressed to comply with the repository’s backend module standards.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an issue in the Claude provider where turns that launch background work via the Agent tool were not recognized as “background work pending,” causing the CLI stdin to be released immediately after result and killing the background agent mid-run (issue #1268). It also extends the same “hold CLI open” behavior to Workflow, which always reports back in a later turn.
Changes:
- Update
startsBackgroundWork()to treatAgenttool calls as background work unlessrun_in_background === false. - Add
Workflowto the deferred-work tool set so workflow task IDs keep the CLI open until follow-up. - Add a focused unit test suite covering background/foreground
Bash,Agent,Workflow, and regressions for existing deferred tools.
File summaries
| File | Description |
|---|---|
server/modules/providers/list/claude/claude-runtime.provider.js |
Extends background-work detection for Agent and Workflow, and exports the helper for unit testing. |
server/modules/providers/tests/claude-background-work.test.ts |
Adds unit coverage for the background-work scoring logic across key tool scenarios. |
Review details
Suppressed comments (1)
server/modules/providers/list/claude/claude-runtime.provider.js:1230
- If
startsBackgroundWorkis exported at its declaration, it should be removed from the end-of-file export list to avoid duplicate exports and to keep the file’s public API surface explicit at the definition site.
extractTokenBudget,
extractCumulativeTokenBudget,
startsBackgroundWork
};
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Tool calls that leave work running past the end of a turn. Bash and Agent only | ||
| // count when they are backgrounded; the rest defer or watch work by nature. | ||
| // Workflow belongs here rather than in a branch of its own: its input schema has | ||
| // no foreground option at all, so every call returns a task id immediately and | ||
| // reports back in a later turn. | ||
| const DEFERRED_WORK_TOOLS = new Set(['Monitor', 'ScheduleWakeup', 'CronCreate', 'TaskCreate', 'Workflow']); |
There was a problem hiding this comment.
I'd rather not do this one here, and I think the premise is slightly off.
It's correct that this is the only .js file in list/claude/. But it isn't an isolated straggler — all three runtime providers are .js:
| File | Lines |
|---|---|
list/claude/claude-runtime.provider.js |
1233 |
list/opencode/opencode-runtime.provider.js |
434 |
list/cursor/cursor-runtime.provider.js |
387 |
That's 5 of the 5 non-TS files under server/modules/ accounted for by the runtime layer and the notification orchestrator. Migrating this one alone would make it the odd provider out rather than bringing it in line.
On the standard itself (.agents/skills/backend-module-standards/SKILL.md), the two relevant rules are:
Use TypeScript for every file inside
server/modules/. Do not add JavaScript backend module files.
This PR adds no .js file — the new test is .ts.
When touched JavaScript utilities belong to the work, migrate them to TypeScript. Place a one-use utility in its sole component; place a utility used in at least two locations in
server/shared/utils.ts.
The placement guidance scopes this to utilities. A 1,233-line runtime provider isn't one, and there's nowhere for it to go under either branch of that rule.
Against that, CONTRIBUTING.md asks to keep PRs focused. A 1,233-line JS→TS migration would dwarf a 17-line bug fix and make the actual behaviour change hard to review — on a file whose failure mode is silently killed background work.
Taking the follow-up option you offered: happy to open an issue to migrate all three runtime providers together, or to send it as a separate PR if the maintainers would prefer. Just say which.
| // A backgrounded subagent outlives the turn exactly like a backgrounded | ||
| // Bash does, so the process has to be held open for it to report back. | ||
| // Agents background by default — `run_in_background` is optional and only | ||
| // an explicit `false` opts out — hence `!== false` rather than `=== true`. | ||
| // A foreground agent must stay out of DEFERRED_WORK_TOOLS: it never pushes |
There was a problem hiding this comment.
Done in a8eb8aa — export function startsBackgroundWork() at the declaration, removed from the end-of-file block, with a Used by ... note naming the consumer per the standard's rule on documenting exported components.
Worth noting the precedent holds in the other direction too: isSubagentPromptEcho is exported the same way and its only cross-file consumer is also a test, so this isn't a new kind of export for the module.
I left the nine remaining symbols in the end-of-file block alone — they're pre-existing and unrelated to this fix.
Backend module standards require exports at the declaration rather than collected at the end of an implementation file, and a note naming the consumer. Matches isSubagentPromptEcho, the module's other test-facing predicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed this fix against 1.37.3 (cherry-picked onto it, 10/10 of the new tests pass, 171/172 of the providers suite with 1 pre-existing skip). Two observations from diagnosing the same failure, in case they are useful — the second one suggests a follow-up is still needed. 1. The CLI child does not exit at wind-down — it detaches and keeps working.
The user-visible result is the same and the fix here is still correct — but the agent's work usually is not lost, it lands in the transcript with no client attached. It also means two CLI processes can hold one session id and append to one JSONL concurrently. 2. A second teardown path survives this fix.
// A new turn supersedes any earlier one still holding this session's process
// open, so held runs cannot stack up across a conversation.
if (sessionKey()) {
getSession(sessionKey())?.releaseInput?.();
}Since every turn builds a fresh That reading is from the code path plus the process evidence above rather than an A/B test with this patch applied, so I may be missing something — but if it holds, the complete fix needs the new turn delivered into the held run's input stream ( |
|
Heads-up on an interaction with #1296, which touches the same twenty lines of this file — and on a measurement that supports your change. Where they meet. #1296 adds a second condition to the same hold, so the process is kept open while the CLI's own list of outstanding work is non-empty, not only when this turn's tool calls scored function shouldHoldForBackgroundWork(pendingFromThisTurn, outstandingTasks) {
return Boolean(pendingFromThisTurn) || outstandingTasks.length > 0;
}That list arrives as Neither replaces the other, and yours is the one that belongs at the root:
Caveat on my evidence, since it is partial: I confirmed the The practical bit. #1296 inserts two helpers immediately above |
Fixes #1268.
The bug
startsBackgroundWork()decides whether a turn's CLI process is held open past itsresult. It matchesBashand theDEFERRED_WORK_TOOLSset, but nothing matchesAgent— so a turn whose only outstanding work is a background subagent is scored as having nothing pending.releasePromptStream()runs immediately, the SDK closes stdin, the CLI reads that EOF as print wind-down, and the agent is killed mid-run.The Node server never restarts and the browser stays on the same transcript, so the only visible trace is on the next message:
As #1268 notes, this is a gap in the fix that shipped for #1113 rather than a regression: that work predates background agents, and its own notes list subagents as finishing inside a turn, which was true at the time.
Reproduce
The change
Agentgets its own arm rather than a place inDEFERRED_WORK_TOOLS, for the reason given in the issue: that set would also match foreground agents, which never push a follow-up turn and would therefore pin a CLI process for the full 30-minuteBG_WAIT_CEILING_MS.The test is
!== falserather than=== truebecauserun_in_backgroundis optional onAgentInputand agents background by default — an omitted field means background, so=== truewould still drop the common case.Workflowis added toDEFERRED_WORK_TOOLS, and that is deliberate:WorkflowInputhas no foreground option at all, so every call returns a task id immediately and reports back in a later turn. It has the identical failure mode in the identical function. Happy to split it out if you would rather keep this PR to the letter of the issue.Tests
New
server/modules/providers/tests/claude-background-work.test.ts, following the existingclaude-subagent-echo.test.tspattern;startsBackgroundWorkis exported for it the same wayisSubagentPromptEchois.Ten cases, including guards against the two ways this could be got wrong — a foreground
Agentmust not hold (or it pins the process for the ceiling), and the existingBash/Monitorbehaviour must not shift.Verified the tests fail without the fix rather than assuming it: reverting just the two logic changes fails exactly the 4 fix-dependent cases and leaves the other 6 green.
Checks
npm test— 421 pass / 4 fail. The 4 failures areclaude-cli-path.test.tsand are pre-existing on unmodifiedmain: this machine has a native/usr/bin/claude, which those tests assume absent. Confirmed by stashing and re-running.npm run build— passes.oxlint— clean on both changed files.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes