fix(cli): disable background tasks for Claude Code batch spawns#862
Conversation
Adds a new `batchModeEnvVars` field to agent definitions that is read only by the CLI spawners (spawnClaudeAgent, spawnJsonLineAgent), not by the desktop UI path or --live mode. Populates Claude Code with CLAUDE_CODE_DISABLE_BACKGROUND_TASKS=1 so run_in_background tool calls do not silently fail when the short-lived `maestro-cli send` batch session exits before the async task completes. Shell-env-wins precedence matches the existing defaultEnvVars pattern, so users can still opt out by setting the var in their shell. Fixes RunMaestro#861 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change introduces automatic injection of environment variables for batch-mode agent spawns by adding an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
Greptile SummaryThis PR fixes a silent data-loss issue (#861) where Confidence Score: 5/5Safe to merge — targeted fix with no P0/P1 issues found. The change is well-scoped: a single new optional field on the agent definition type, applied only in the two CLI spawner functions, with three targeted tests covering normal/read-only/shell-override cases. The "shell env wins" falsy-check is consistent with the existing No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(cli): disable background tasks for C..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/services/agent-spawner.ts (1)
179-186: Optional: tighten the "shell env wins" check and deduplicate.
if (!env[k])treats any falsy string (e.g., an intentionally emptyCLAUDE_CODE_DISABLE_BACKGROUND_TASKS=) as unset and overwrites it. While'0'is truthy in JS (so the existing test passes), a stricter presence check better matches the stated "shell env wins" contract and is consistent across future env vars. The same block is duplicated inspawnClaudeAgentandspawnJsonLineAgent— worth a tiny helper.♻️ Proposed helper
+function applyEnvDefaults(env: NodeJS.ProcessEnv, vars?: Record<string, string>) { + if (!vars) return; + for (const k of Object.keys(vars)) { + if (!(k in env)) env[k] = vars[k]; + } +}Then replace both blocks:
- if (def?.batchModeEnvVars) { - for (const k of Object.keys(def.batchModeEnvVars)) { - if (!env[k]) env[k] = def.batchModeEnvVars[k]; - } - } + applyEnvDefaults(env, def?.batchModeEnvVars);Note: the existing
defaultEnvVarsloop at lines 407-411 uses the same!env[k]pattern; applying the helper there too keeps behavior consistent.Also applies to: 413-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 179 - 186, The batch-mode env merge currently uses "if (!env[k])" which treats empty strings or other falsy values as unset; change this to a strict presence check (e.g., test env[k] === undefined or use Object.prototype.hasOwnProperty.call(env, k)) so the shell-provided empty or falsy values are preserved, and extract the logic into a small helper (e.g., applyAgentEnvDefaults(def, env) or mergeBatchModeEnvVars) and call it from where getAgentDefinition('claude-code') is applied as well as from spawnClaudeAgent and spawnJsonLineAgent; also consider reusing the same helper for the defaultEnvVars loop to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 179-186: The batch-mode env merge currently uses "if (!env[k])"
which treats empty strings or other falsy values as unset; change this to a
strict presence check (e.g., test env[k] === undefined or use
Object.prototype.hasOwnProperty.call(env, k)) so the shell-provided empty or
falsy values are preserved, and extract the logic into a small helper (e.g.,
applyAgentEnvDefaults(def, env) or mergeBatchModeEnvVars) and call it from where
getAgentDefinition('claude-code') is applied as well as from spawnClaudeAgent
and spawnJsonLineAgent; also consider reusing the same helper for the
defaultEnvVars loop to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02e85a1c-a6a8-4878-b1d8-2acf0183da32
📒 Files selected for processing (4)
src/__tests__/cli/services/agent-spawner.test.tssrc/__tests__/main/agents/definitions.test.tssrc/cli/services/agent-spawner.tssrc/main/agents/definitions.ts
|
Thanks for the clean fix, @scriptease! 🙏 Reviewed the diff and I'm happy with this:
The CodeRabbit nitpick about extracting a helper / using a strict presence check is reasonable but not worth blocking on — staying consistent with the existing Approving. 🟢 |
Adds a new
batchModeEnvVarsfield to agent definitions that is read only by the CLI spawners (spawnClaudeAgent, spawnJsonLineAgent), not by the desktop UI path or --live mode. Populates Claude Code with CLAUDE_CODE_DISABLE_BACKGROUND_TASKS=1 so run_in_background tool calls do not silently fail when the short-livedmaestro-cli sendbatch session exits before the async task completes.Shell-env-wins precedence matches the existing defaultEnvVars pattern, so users can still opt out by setting the var in their shell.
The original issues it that maestro-cli requests can cause background tasks to spawn that then never report back and the result is lost.
The setting is documented here https://claudelog.com/faqs/what-is-disable-background-tasks-in-claude-code/
But Claude Opus 4.7 couldn't figure it out and had to be carried to the finish line.
Fixes #861
Summary by CodeRabbit