Repository navigation
feat: pi-parity for chat + agent LLM loops (shared stream layer, reasoning round-trip, developer role) - #116
Merged
Merged
Conversation
Centralize the OpenAI-compatible streaming/message-prep layer (sse.ts consumer, request body, truncation guard, reasoning round-trip) into a shared llm-stream module so chat-loop and pi-agent-loop can't drift again. - system prompt sent as role:"system" message (never top-level system:), incl. compaction - finish_reason capture + refuse truncated/interrupted tool calls in both loops - reasoning round-trips to the same model under its native field; converts to text cross-model; never baked into content (pi behaviour) - empty assistant turns filtered from outgoing requests - reasoning models get role:"developer" on supported providers (models.dev reasoning flag + pi detectCompat denylist); model catalog now parses it
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesReasoning-aware chat and agent flows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatOrAgent
participant ContextPreparation
participant LLMProvider
participant StreamConsumer
participant ToolExecutor
ChatOrAgent->>ContextPreparation: prepare model-aware messages
ContextPreparation->>LLMProvider: send chat-completion request
LLMProvider->>StreamConsumer: stream content, reasoning, and tool calls
StreamConsumer->>ChatOrAgent: return parsed turn and finish reason
ChatOrAgent->>ToolExecutor: execute complete tool calls
ToolExecutor->>ChatOrAgent: return tool results or truncation errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
electron/lib/pi-agent-loop.ts (1)
754-766: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the execution path with this index mapping.
The guard resolves chip call IDs through
turn.toolCallIndexes[i], which converts an array position into the streamtool_calls[].index. The execution path at Line 808 usesstreamCallIds.get(tcIdx)and therefore assumes the two are identical. If a provider emits non-contiguous indexes, that lookup returns the wrong ID orundefined, and the pending chip created byonToolPendingnever updates. Use the same mapping in both places.♻️ Proposed change at Line 808
- const pendingCallId = streamCallIds.get(tcIdx); + const pendingCallId = streamCallIds.get(turn.toolCallIndexes[tcIdx] ?? tcIdx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/pi-agent-loop.ts` around lines 754 - 766, Update the execution path near onToolPending to resolve call IDs through turn.toolCallIndexes using the tool-call array position, matching callIdFor in failToolCallsFromTruncatedMessage. Replace the direct streamCallIds.get(tcIdx) lookup while preserving the existing behavior for contiguous and non-contiguous provider indexes.src/components/chat/chat-panel/QuestionForm.tsx (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate the prompt text with the textarea for assistive technology.
The prompt moved out of the
placeholder, so screen-reader users no longer receive it through the field itself. Link the<p>to the textarea witharia-describedby, and give the textarea an accessible name.♻️ Proposed accessibility wiring
- <p className="text-[0.714rem] text-[var(--text-tertiary)] leading-relaxed">{q.prompt}</p> + <p id={`q-${q.id}-prompt`} className="text-[0.714rem] text-[var(--text-tertiary)] leading-relaxed">{q.prompt}</p> <textarea + aria-label={q.label} + aria-describedby={`q-${q.id}-prompt`} value={answers[q.id] ?? ""}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/chat/chat-panel/QuestionForm.tsx` around lines 44 - 56, Update the question field rendering around the prompt <p> and textarea: assign each prompt a unique id derived from q.id, reference it from the textarea via aria-describedby, and add an accessible name to the textarea so assistive technologies identify the field and its associated prompt.electron/lib/llm-stream.ts (2)
268-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a data-driven denylist.
The condition mixes provider ids and URL fragments in one long boolean chain. A table makes additions safer and testable.
const NON_STANDARD_PROVIDERS = new Set(["nvidia", "cerebras", "xai", "together", "zai", "moonshotai", "moonshotai-cn", "ant-ling", "localllm"]); const NON_STANDARD_URL_FRAGMENTS = ["integrate.api.nvidia.com", "cerebras.ai", "api.x.ai", /* … */];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/llm-stream.ts` around lines 268 - 289, Refactor supportsDeveloperRole to replace the long isNonStandard boolean chain with data-driven NON_STANDARD_PROVIDERS and NON_STANDARD_URL_FRAGMENTS collections, then derive the denylist match from the provider and baseUrl. Preserve all existing provider IDs, URL fragments, OpenRouter model handling, and return behavior.
96-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the
trytoJSON.parseonly.The
catchat line 162 is documented as a malformed-JSON guard. It currently also covers the callback invocations (onUsage,onToken,onThought,onToolPending) and the tool-call buffering. An error thrown by a consumer callback is silently discarded, and the remainder of that chunk is skipped. Parse first, then process outside thetry.♻️ Proposed scoping
- try { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const chunk = JSON.parse(jsonStr) as any; - + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` + let chunk: any; + try { + chunk = JSON.parse(jsonStr); + } catch { continue; /* skip malformed SSE JSON line */ }The closing
} catch { ... }at line 162 is then removed and the block body de-indented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/llm-stream.ts` around lines 96 - 163, Restrict the try/catch in the SSE chunk processing flow to JSON.parse(jsonStr) only, retaining the malformed-JSON skip behavior. Move usage handling, finish-reason extraction, delta processing, reasoning accumulation, tool buffering, and callback invocations outside the catch so consumer errors propagate instead of being silently discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelogs/v2.6.5.md`:
- Line 5: Update the changelog entry to describe both agent-loop and
context-compaction paths as using chat’s provider-aware system-role resolution,
with system as the fallback, rather than claiming they always send role
"system".
In `@electron/lib/chat-loop.ts`:
- Around line 293-294: Restrict the streamInterrupted calculation and check in
the turn-finish handling to executions using the streaming branch, not the
provider === "localllm" non-streaming path. Add a local flag set only in the
streaming else branch and require it when evaluating streamInterrupted, while
preserving the existing "length" handling and tool-call flow.
In `@electron/lib/llm-stream.ts`:
- Around line 250-256: The tool-call result mapping must handle entries where
tc.id is empty or missing, as produced by consumeAssistantStream truncation. In
the map around opts.emitStart and opts.emitEnd, skip id-less tool calls or
synthesize a valid identifier, and ensure the same identifier is used
consistently for the emitted tool result and corresponding assistant message.
- Around line 351-398: Update prepareContextMessages so reasoningByMsg does not
depend on object identity: associate reasoning with a stable message index or
identifier and restore it correctly after pruner transformations. Alternatively,
explicitly enforce and document that the pruner must preserve references if no
stable key exists. Also update the documentation near prepareContextMessages to
state that the system prompt role is controlled by systemRole and may be
developer, not always system.
In `@electron/lib/llm.ts`:
- Line 80: Update calculatePromptBreakdown so messages with role "developer"
follow the same system-token accounting branch as role "system", preventing the
system prompt from also contributing to conversationTokens. Preserve existing
handling for tool, assistant, and user messages.
In `@electron/lib/pi-agent-loop.test.ts`:
- Line 498: Replace the vacuous error assertions in
electron/lib/pi-agent-loop.test.ts at lines 498-498 and 526-526 with a predicate
that checks whether any log entry starts with "error:" and asserts the result is
false; update both sites identically.
In `@electron/lib/pi-agent-loop.ts`:
- Around line 577-581: Pass provider through the agent configuration flow from
electron/ipc/pi-agent.ts into AgentLLMConfig, then include it in the
resolveSystemRole call in the agent loop alongside isReasoningModel, baseUrl,
and modelId. Preserve the existing shared role-resolution behavior used by the
chat path.
In `@src/components/agent/AgentChatPane.tsx`:
- Around line 529-533: Ensure the model catalog is prewarmed or subscribed to
through the established catalog-loading flow before the `getModelInfo` call that
sets `isReasoningModel` in `AgentChatPane`. Update `handleApprovePlan` to
propagate the same `isReasoningModel` value in its config so plan-approval turns
use the same system-role behavior as normal prompt turns.
---
Nitpick comments:
In `@electron/lib/llm-stream.ts`:
- Around line 268-289: Refactor supportsDeveloperRole to replace the long
isNonStandard boolean chain with data-driven NON_STANDARD_PROVIDERS and
NON_STANDARD_URL_FRAGMENTS collections, then derive the denylist match from the
provider and baseUrl. Preserve all existing provider IDs, URL fragments,
OpenRouter model handling, and return behavior.
- Around line 96-163: Restrict the try/catch in the SSE chunk processing flow to
JSON.parse(jsonStr) only, retaining the malformed-JSON skip behavior. Move usage
handling, finish-reason extraction, delta processing, reasoning accumulation,
tool buffering, and callback invocations outside the catch so consumer errors
propagate instead of being silently discarded.
In `@electron/lib/pi-agent-loop.ts`:
- Around line 754-766: Update the execution path near onToolPending to resolve
call IDs through turn.toolCallIndexes using the tool-call array position,
matching callIdFor in failToolCallsFromTruncatedMessage. Replace the direct
streamCallIds.get(tcIdx) lookup while preserving the existing behavior for
contiguous and non-contiguous provider indexes.
In `@src/components/chat/chat-panel/QuestionForm.tsx`:
- Around line 44-56: Update the question field rendering around the prompt <p>
and textarea: assign each prompt a unique id derived from q.id, reference it
from the textarea via aria-describedby, and add an accessible name to the
textarea so assistive technologies identify the field and its associated prompt.
🪄 Autofix
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 Plus
Run ID: 489e754c-d8a6-4957-8f59-987e6f7f3f0b
📒 Files selected for processing (18)
changelogs/v2.6.5.mdelectron/ipc/chat.tselectron/ipc/pi-agent.tselectron/lib/chat-loop.tselectron/lib/compaction.tselectron/lib/llm-stream.test.tselectron/lib/llm-stream.tselectron/lib/llm.tselectron/lib/pi-agent-loop.test.tselectron/lib/pi-agent-loop.tselectron/lib/tool-schemas.tselectron/lib/tools.tsshared/models/model-catalog.test.tsshared/models/model-catalog.tssrc/components/agent/AgentChatPane.tsxsrc/components/chat/chat-panel/QuestionForm.tsxsrc/components/chat/chat-panel/index.tsxsrc/hooks/useChatStream.ts
- chat-loop: interrupted-stream guard restricted to the SSE streaming branch (localllm null finish_reason is a normal response) - llm.ts: calculatePromptBreakdown counts role:'developer' under the system bucket so it isn't double-counted as conversation - llm-stream: consumeAssistantStream synthesizes ids for tool calls cut before the id chunk; failToolCallsFromTruncatedMessage same-id defense; SSE try/catch narrowed to JSON.parse so consumer errors propagate; supportsDeveloperRole is data-driven; prepareContextMessages docs cover systemRole + pruner identity - pi-agent: provider plumbed into AgentLLMConfig (localllm derived from local baseUrl) and used in resolveSystemRole; onToolPending callId resolves through turn.toolCallIndexes for non-contiguous provider indexes - AgentChatPane: prewarms the model catalog; plan-approval config propagates isReasoningModel - QuestionForm: accessible textarea naming + aria-describedby for the prompt - tests: vacuous not.toContain error assertions replaced with real predicates; new tests for id synthesis and truncation id consistency
This was referenced Aug 7, 2026
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
1 participant
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.
What does this PR do?
Brings the chat and agent tool-call loops to parity with pi and fixes several bugs the two loops had silently drifted on.
electron/lib/llm-stream.tsconsumed by bothchat-loopandpi-agent-loop, so they can't diverge again.role: "system"message (never a top-levelsystem:field) — fixesunknown parameter systemfailures on strict providers, incl. the agent's context compaction.finish_reasonis now captured; tool calls from alength-truncated turn or an interrupted stream (no finish_reason) are refused instead of executed with possibly-truncated arguments.reasoning_content/reasoning/reasoning_text), is converted to plain text cross-model, and is never baked into reply content (pi behaviour). Empty reasoning-only turns are filtered out so they can't 400 the next request.developersystem role on providers that support it (models.devreasoningflag + pi'sdetectCompatdenylist).Type of change
Screenshots / recording
UI change (ask-questions form) — no screenshot; the form now shows the question as persistent text above the answer field instead of a placeholder that disappears on typing.
Checklist
npm run type-check:allpassesnpm run lintpassesnpm testpasses (runsnpm run compilefirst soelectron/bundle-guard.test.tsactually executes —npm run test:bundleto run just that)npm run test:e2epasses (run before merging UI changes or cutting a release)scripts/features.config.jsso it appears in the "What's New" modal (generated JSON — runnode scripts/generate-features.js) — N/A: not a headline feature; covered by thev2.6.5changelog.var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>— N/A: no new IPC handlers.schema.ts— N/A: no migrations.electron/db/queries.ts— single source of truth — N/A: no new SQL.dependenciesordevDependenciesadded toROLE_MAPinscripts/generate-licenses.jsandlicenses.jsonregenerated — N/A: no new deps.electron/mcp/tools/index.tsdispatch +electron/lib/tool-schemas.tsZod schema — N/A: no new MCP tools (schema edit only, existingask_questionsdescription).--external:<pkg>flag in thecompilescript: updated the appropriate allowlist group — N/A: no flag changes.Notes for reviewer
llm-stream.tsis now the single source of truth for SSE parsing (reasoning-field capture,finish_reason, tool-call buffering), request-body construction, the truncation guard, and message prep. Both loops keep their own orchestration (tool execution is parallel in both; the agent additionally has approval gates / plan mode / subagents).developerrole is gated on pi'ssupportsDeveloperRoledenylist (DeepSeek, NVIDIA, Cerebras, xAI, Together, chutes.ai, ZAI, Moonshot, Cloudflare, Ant Ling, on-device Llama; OpenRouter only foranthropic/*/openai/*) so it can't 400 an unsupported provider. No manual Auto/On/Off override yet — flagged as follow-up on the task card if mis-cataloged models need it.pendingQuestions?.length(scalar) rather than the array to avoid a React deps-size dev warning.