fix(cli): capture response from claude-code in maestro-cli send#679
fix(cli): capture response from claude-code in maestro-cli send#679pedramamini merged 3 commits intoRunMaestro:rcfrom
Conversation
Claude Code emits response text in assistant messages, not in the result message's `result` field (which is empty). The CLI spawner only checked `msg.result`, so it always returned `response: null`. Now accumulates text from assistant messages as a fallback and flushes the JSON line buffer on process close to handle output that lacks a trailing newline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes per-message parsing in agent spawners, accumulates assistant text as a fallback to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Key changes:
Minor observations (P2, non-blocking):
Confidence Score: 5/5Safe to merge — the fix is targeted and well-tested; remaining feedback is P2 style/quality. No P0 or P1 findings. Both observations (missing text separator and duplicated parse logic) are style/maintainability concerns that do not affect correctness for the primary use-case (single-turn CLI queries where Claude Code emits one final assistant message). The 5 new tests clearly cover all added code paths and the existing suite continues to pass. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as maestro-cli send
participant Spawner as spawnClaudeAgent
participant Claude as claude (child process)
CLI->>Spawner: spawnAgent('claude-code', cwd, prompt)
Spawner->>Claude: spawn(claude, [--print, --verbose, --output-format stream-json, ...])
Claude-->>Spawner: stdout data: {"type":"assistant","message":{"content":[{"type":"text","text":"..."}]}}
Note over Spawner: Accumulate text block → assistantText += text
Claude-->>Spawner: stdout data: {"type":"result","result":"","total_cost_usd":0.02}
Note over Spawner: result field is empty → result stays undefined
Claude-->>Spawner: close(0) [possibly without trailing newline]
Note over Spawner: Flush jsonBuffer remnant if any
Note over Spawner: finalResult = result ∥ assistantText ∥ undefined
Spawner-->>CLI: {success:true, response: assistantText}
Reviews (1): Last reviewed commit: "fix(cli): capture response from claude-c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli/services/agent-spawner.ts (1)
293-326: Don't swallow non-parse failures in the close-buffer flush.This
trynow wrapsaggregateModelUsage()and the assistant-content walk too, so an unexpected shape/runtime bug there gets silently discarded and the command resolves with stale data. Only theJSON.parsefailure is expected here; handle anything after parsing explicitly instead of absorbing it in the remnant-JSON fallback.As per coding guidelines, "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production. Only catch and handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR)."
🤖 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 293 - 326, The current try-catch around JSON.parse also swallows runtime errors from post-parse processing (e.g., in aggregateModelUsage, assistant content traversal, or result/session handling); refactor so only JSON.parse is inside a try that catches and ignores parse errors, then process the parsed msg outside that catch (so exceptions in handling—references to resultEmitted/result, assistantText content assembly, sessionIdEmitted/sessionId, and aggregateModelUsage(msg.modelUsage, msg.usage, msg.total_cost_usd)—are allowed to throw and bubble up); keep all existing null/shape checks but remove the broad try that currently hides unexpected runtime issues.src/__tests__/cli/services/agent-spawner.test.ts (1)
1067-1166: Cover the JSON-line close-flush path too.These additions only exercise the Claude branch. The new JSON-line flush logic at Line 530 in
src/cli/services/agent-spawner.tsstill has no direct test, and the "session_id and usage" case never asserts the bufferedusageStatsvalue, so a regression in the other half of this change would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/cli/services/agent-spawner.test.ts` around lines 1067 - 1166, Add a unit test that exercises the JSON-line close-flush path in spawnAgent so the JSON-buffer-flush logic in agent-spawner.ts is covered: simulate writing JSON fragments to mockStdout (e.g., session_id and usage/total_cost_usd emitted as lines without trailing newline or split across emits), then emit mockChild.close and await the spawnAgent promise, and assert both result.agentSessionId (session_id) and the buffered usage/total_cost_usd/usageStats value are correctly set on the returned result; locate this behavior around the JSON flush logic in agent-spawner.ts and use the spawnAgent helper and mockStdout/mockChild used in other tests to reproduce the close-flush scenario.
🤖 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/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 1067-1166: Add a unit test that exercises the JSON-line
close-flush path in spawnAgent so the JSON-buffer-flush logic in
agent-spawner.ts is covered: simulate writing JSON fragments to mockStdout
(e.g., session_id and usage/total_cost_usd emitted as lines without trailing
newline or split across emits), then emit mockChild.close and await the
spawnAgent promise, and assert both result.agentSessionId (session_id) and the
buffered usage/total_cost_usd/usageStats value are correctly set on the returned
result; locate this behavior around the JSON flush logic in agent-spawner.ts and
use the spawnAgent helper and mockStdout/mockChild used in other tests to
reproduce the close-flush scenario.
In `@src/cli/services/agent-spawner.ts`:
- Around line 293-326: The current try-catch around JSON.parse also swallows
runtime errors from post-parse processing (e.g., in aggregateModelUsage,
assistant content traversal, or result/session handling); refactor so only
JSON.parse is inside a try that catches and ignores parse errors, then process
the parsed msg outside that catch (so exceptions in handling—references to
resultEmitted/result, assistantText content assembly,
sessionIdEmitted/sessionId, and aggregateModelUsage(msg.modelUsage, msg.usage,
msg.total_cost_usd)—are allowed to throw and bubble up); keep all existing
null/shape checks but remove the broad try that currently hides unexpected
runtime issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 465ed86a-d7ba-473e-a502-1ccd8aca9f0f
📒 Files selected for processing (2)
src/__tests__/cli/services/agent-spawner.test.tssrc/cli/services/agent-spawner.ts
…tors Address review feedback: - Extract processMessage/processEvent helpers to deduplicate parsing logic between stdout data handler and close handler buffer flush - Add newline separators between accumulated assistant text blocks to prevent concatenation without whitespace Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 1083-1103: The test name says it should flush session_id and usage
on close but never asserts usage; update the spec that calls spawnAgent (the
resultPromise/result variable) to also assert the parsed usage from the final
chunk (total_cost_usd: 0.05) is returned by the agent API—e.g. after awaiting
resultPromise add an expectation verifying the usage field on result (match
0.05), using the same result object checked for success/response/agentSessionId
so the test verifies both session_id and usage are flushed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72b6e20a-a934-4678-81c3-1e76f5b78a05
📒 Files selected for processing (2)
src/__tests__/cli/services/agent-spawner.test.tssrc/cli/services/agent-spawner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/services/agent-spawner.ts
…est assertion - Narrow try/catch in close handler to only catch JSON.parse failures, letting unexpected errors in processMessage bubble up - Fix trailing whitespace caught by CI prettier - Add missing usage assertion in buffer flush test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
maestro-cli sendreturnedresponse: nullfor claude-code agents because Claude Code emits the response text inassistantmessages, not in theresultmessage'sresultfield (which is empty string)assistantmessages as a fallback whenresultfield is emptyspawnClaudeAgentandspawnJsonLineAgent)Test plan
maestro-cli send <agent-id> "Respond with exactly: CLI capture test OK"now returnssuccess: truewith correct response text🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests