fix(orchestrator): surface AI error results instead of silent drop (fixes #1076)#1084
Closed
kagura-agent wants to merge 2 commits intocoleam00:devfrom
Closed
fix(orchestrator): surface AI error results instead of silent drop (fixes #1076)#1084kagura-agent wants to merge 2 commits intocoleam00:devfrom
kagura-agent wants to merge 2 commits intocoleam00:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors AI client -> provider surface (new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Web UI)
participant Orch as Orchestrator
participant Prov as Agent Provider
participant Plat as Platform Adapter
User->>Orch: send message
Orch->>Prov: getAgentProvider(...).sendQuery(requestOptions)
Prov-->>Orch: yield events (assistant/tool/result)
alt provider yields result isError (no assistant text)
Orch->>Orch: capture errorResult (errorSubtype)
Orch->>Plat: sendMessage(user-facing error guidance)
Plat-->>User: display error message
else provider yields normal assistant output
Orch->>Plat: forward assistant chunks
Plat-->>User: display assistant response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 895-903: Replace the hardcoded "Claude credentials" text with an
assistant-specific auth hint by checking the conversation.ai_assistant_type when
building the hint used in the error handling before calling
platform.sendMessage; use errorResult.subtype and conversation.ai_assistant_type
to pick an appropriate message (e.g., "Check your <assistant> credentials or use
/reset.") and fall back to the generic "Check server logs for details." Ensure
this change is applied in the same block referencing errorResult.subtype and
platform.sendMessage and the analogous block around the other occurrence (lines
~1045-1053) so both error messages reflect the actual ai_assistant_type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04c8cd53-1e71-436b-8c29-8aa786fcdeb8
📒 Files selected for processing (2)
packages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.ts
Contributor
Author
|
Good catch @coderabbitai — updated to use |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 52 🏁 Script executed: Length of output: 48052
Stream mode ( const providerLabel = aiClient.getType() === 'claude' ? 'Claude' : 'AI';
const hint =
errorResult.subtype === 'authentication_error'
? `Check your ${providerLabel} credentials or use /reset.`
: 'Check server logs for details.';Batch mode ( const providerLabel = aiClient.getType() === 'claude' ? 'Claude' : 'AI';
const hint =
errorResult.subtype === 'authentication_error'
? `Check your ${providerLabel} credentials or use /reset.`
: 'Check server logs for details.';Both paths are symmetric and correctly use If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator-agent.ts (1)
906-907: Align log event names with repository structured logging format.
no_ai_responsedoes not follow the{domain}.{action}_{state}convention required by the repo guidelines.♻️ Suggested rename
- getLog().debug({ conversationId, errorResult }, 'no_ai_response'); + getLog().debug({ conversationId, errorResult }, 'orchestrator.ai_response_missing'); ... - getLog().debug({ conversationId, errorResult }, 'no_ai_response'); + getLog().debug({ conversationId, errorResult }, 'orchestrator.ai_response_missing');As per coding guidelines, "Use structured logging with Pino; event naming format:
{domain}.{action}_{state}; always pair_startedwith_completedor_failed; include context IDs and error details".Also applies to: 1057-1058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 906 - 907, Replace the non-conforming log event name 'no_ai_response' in the getLog().debug call with a structured event following the repo format (for example 'orchestrator.ai_response_failed' or 'orchestrator.get_response_failed'), keep the existing context variables (conversationId, errorResult) in the payload, and ensure this failure event is paired with corresponding '_started' or '_completed' events elsewhere (see the other getLog() calls around the same method). Target the getLog().debug invocation that logs { conversationId, errorResult } in orchestrator-agent.ts and update the event string to follow {domain}.{action}_{state}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 906-907: Replace the non-conforming log event name
'no_ai_response' in the getLog().debug call with a structured event following
the repo format (for example 'orchestrator.ai_response_failed' or
'orchestrator.get_response_failed'), keep the existing context variables
(conversationId, errorResult) in the payload, and ensure this failure event is
paired with corresponding '_started' or '_completed' events elsewhere (see the
other getLog() calls around the same method). Target the getLog().debug
invocation that logs { conversationId, errorResult } in orchestrator-agent.ts
and update the event string to follow {domain}.{action}_{state}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2a58df4-9d98-4c91-aab2-404b2e45d8ec
📒 Files selected for processing (1)
packages/core/src/orchestrator/orchestrator-agent.ts
kagura-agent
force-pushed
the
fix/stream-error-silent-drop
branch
from
67b6a56 to
511c326
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator-agent.ts (1)
895-904: Extract the AI error formatter into one helper.Line 895 and Line 1046 now build the same provider label, hint, and final message string. Pulling that into a small helper will keep stream and batch behavior from drifting again on the next wording or subtype tweak.
♻️ Suggested refactor
+function formatAiErrorMessage( + aiClient: ReturnType<typeof getAssistantClient>, + errorResult: { subtype?: string } +): string { + const providerLabel = aiClient.getType() === 'claude' ? 'Claude' : 'AI'; + const hint = + errorResult.subtype === 'authentication_error' + ? `Check your ${providerLabel} credentials or use /reset.` + : 'Check server logs for details.'; + return `AI error${errorResult.subtype ? ` (${errorResult.subtype})` : ''}. ${hint}`; +}Then reuse it in both branches:
- const providerLabel = aiClient.getType() === 'claude' ? 'Claude' : 'AI'; - const hint = - errorResult.subtype === 'authentication_error' - ? `Check your ${providerLabel} credentials or use /reset.` - : 'Check server logs for details.'; - await platform.sendMessage( - conversationId, - `AI error${errorResult.subtype ? ` (${errorResult.subtype})` : ''}. ${hint}` - ); + await platform.sendMessage(conversationId, formatAiErrorMessage(aiClient, errorResult));Also applies to: 1046-1055
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 895 - 904, The code builds the same provider label, hint, and message in two places (when handling errorResult before streaming and later at lines around the batch branch); extract that logic into a single helper like formatAIErrorMessage(errorResult, aiClient) which returns the full string, then replace the repeated construction in both places (the blocks using aiClient.getType(), errorResult.subtype, and platform.sendMessage) to call this helper so wording and subtype handling stay consistent across stream and batch flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 906-907: The log event string "no_ai_response" does not follow our
`{domain}.{action}_{state}` convention—update the two log calls that use this
literal (the debug call at getLog().debug({ conversationId, errorResult },
'no_ai_response') and the other instance around lines 1057–1058) to a
domain-prefixed event like "orchestrator.ai_response_failed" so they are
queryable with other structured logs; keep the same metadata object ({
conversationId, errorResult }) and use the existing getLog() logger (created via
createLogger('orchestrator') elsewhere in this module) when replacing the event
name.
---
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 895-904: The code builds the same provider label, hint, and
message in two places (when handling errorResult before streaming and later at
lines around the batch branch); extract that logic into a single helper like
formatAIErrorMessage(errorResult, aiClient) which returns the full string, then
replace the repeated construction in both places (the blocks using
aiClient.getType(), errorResult.subtype, and platform.sendMessage) to call this
helper so wording and subtype handling stay consistent across stream and batch
flows.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c8fa93e-7a38-49d3-9a53-b759214a0a0c
📒 Files selected for processing (3)
homebrew/archon.rbpackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.ts
✅ Files skipped from review due to trivial changes (1)
- homebrew/archon.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/orchestrator/orchestrator-agent.test.ts
Comment on lines
+906
to
907
| getLog().debug({ conversationId, errorResult }, 'no_ai_response'); | ||
| return; |
There was a problem hiding this comment.
Rename this log event to match the repo convention.
no_ai_response does not follow the {domain}.{action}_{state} format used elsewhere in the project. Please rename both entries to something like orchestrator.ai_response_failed so they stay queryable with the rest of the structured logs.
As per coding guidelines, "Use createLogger('domain') from @archon/paths for structured Pino logging with event naming format {domain}.{action}_{state} and standard states: _started, _completed, _failed, _validated, _rejected".
Also applies to: 1057-1058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 906 - 907,
The log event string "no_ai_response" does not follow our
`{domain}.{action}_{state}` convention—update the two log calls that use this
literal (the debug call at getLog().debug({ conversationId, errorResult },
'no_ai_response') and the other instance around lines 1057–1058) to a
domain-prefixed event like "orchestrator.ai_response_failed" so they are
queryable with other structured logs; keep the same metadata object ({
conversationId, errorResult }) and use the existing getLog() logger (created via
createLogger('orchestrator') elsewhere in this module) when replacing the event
name.
0dffb6d to
c5908df
Compare
…ixes coleam00#1076) When the Claude OAuth refresh token is expired, the result message has is_error=true but session_id=undefined. Both handleStreamMode and handleBatchMode required sessionId to be truthy to process result messages, causing errors to be silently dropped. Changes: - Split result handler condition to process sessionId and isError independently in both stream and batch modes - Send descriptive error message to user when AI errors produce no assistant messages (with auth-specific hint for authentication_error) - Include errorResult in debug log for observability Tests: 5 new tests covering stream/batch × auth/generic/no-subtype
Address CodeRabbit review: replace hardcoded 'Claude credentials' with provider-aware label from aiClient.getType() so error messages are accurate for non-Claude assistants.
kagura-agent
force-pushed
the
fix/stream-error-silent-drop
branch
from
c5908df to
0476be6
Compare
Collaborator
|
@kagura-agent this PR looks similar to the already-merged #1089 (merged April 16, 2026). Please check whether your change is still needed — rebase on main and confirm. |
Contributor
Author
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
2 participants
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.
Problem
When the Claude OAuth refresh token is expired (or any AI error occurs with no
session_id), the chat UI shows no response at all — no error message, no indication of failure.Root Cause
Both
handleStreamModeandhandleBatchModeinorchestrator-agent.tshad a condition that requiredmsg.sessionIdto be truthy to processresultmessages:When OAuth fails, the result message has
is_error: truebutsession_id: undefined, so:allMessages.length === 0Fix
sessionIdandisErrorindependentlyAI error (authentication_error). Check your Claude credentials or use /reset.AI error (<subtype>). Check server logs for details.errorResultin debug log for observabilityApplied to both stream and batch mode paths.
Validation
bun run type-checkclean across all packageseslint+prettiervia lint-staged cleanCloses #1076
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation