fix: write orchestrator failure reasons to task_logs.json (#1978)#2005
fix: write orchestrator failure reasons to task_logs.json (#1978)#2005octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
…Mik90#1978) When the SpecOrchestrator or BuildOrchestrator fails (e.g. "Model completed session without making any tool calls — expected files not created: context.json"), the error string in outcome.error was emitted as a PLANNING_FAILED task event but never written to task_logs.json. This caused the Logs tab to show red "error" badges with no message text. Fix by writing outcome.error to the log writer as an 'error' entry before closing the active phase, in both runSpecOrchestrator and runBuildOrchestrator. These orchestrator-level failures don't flow through the onEvent stream callback, so they need to be written explicitly at the point where outcome.error is first available.
|
octo-patch seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds error logging to orchestrator functions in the agent worker. Before closing log phases in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the build and spec orchestrators to log failure reasons directly to the UI when an outcome is unsuccessful, ensuring that build-level and spec-level validation errors are visible in the Logs tab. I have reviewed the changes and suggest consolidating the phase-mapping logic in the build orchestrator into a helper function to reduce duplication with the existing terminal state cleanup block.
| if (!outcome.success && outcome.error) { | ||
| const data = logWriter.getData(); | ||
| const activePhase = (['validation', 'coding', 'planning'] as const).find( | ||
| (p) => data.phases[p]?.status === 'active' | ||
| ); | ||
| const errorPhase: 'qa' | 'coding' | 'planning' = | ||
| activePhase === 'validation' ? 'qa' : (activePhase ?? 'planning'); | ||
| logWriter.logText(outcome.error, errorPhase, 'error'); | ||
| } |
There was a problem hiding this comment.
The logic for identifying the active phase and mapping it to a Phase type is correct and ensures that orchestrator-level errors are attributed to the correct log section. However, note that this logic for finding and mapping the active phase is partially duplicated in the terminal state cleanup block (lines 699-705). While not a bug, consolidating this into a helper or a shared variable within the if (logWriter) block would improve maintainability.
2 participants
Fixes #1978
Problem
After the Vercel AI SDK v6 migration, planning phase errors show as red "error" badges in the Logs tab with completely empty detail messages. When
SpecOrchestratororBuildOrchestratorfails (e.g., "Model completed session without making any tool calls — expected files not created: context.json"), the error string inoutcome.errorwas emitted as aPLANNING_FAILEDtask event but never written totask_logs.json.Solution
Explicitly write
outcome.errorto the log writer as anerrorentry before closing the active phase, in bothrunSpecOrchestratorandrunBuildOrchestratorinworker.ts. These orchestrator-level failures don't flow through theonEventstream callback, so they need to be written at the point whereoutcome.erroris first available.Testing
Summary by CodeRabbit