refactor(agent): tighten message, permission, and stop boundaries#1973
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds architecture contracts and aligns runtime behavior around three boundaries: canonical ChangesCanonical message boundary
Permission-mode ownership
Provider terminal contract
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SessionTurnCoordinator
participant TurnCoordinator
participant AgentRuntime
participant PendingInputStore
SessionTurnCoordinator->>SessionTurnCoordinator: normalize input once
SessionTurnCoordinator->>TurnCoordinator: SendMessageInput
TurnCoordinator->>AgentRuntime: send or queue SendMessageInput
AgentRuntime->>PendingInputStore: persist structured payload
PendingInputStore-->>AgentRuntime: validated decoded payload
sequenceDiagram
participant Provider
participant TerminalMapper
participant Accumulator
participant CompatibilityAdapter
Provider->>TerminalMapper: provider stop reason
TerminalMapper->>Accumulator: terminal events
Accumulator->>Accumulator: assign stream stop reason
Accumulator->>CompatibilityAdapter: settled stream state
CompatibilityAdapter-->>Provider: preserved terminal outcome
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentRuntimePresenter/contextBuilder.ts (1)
1088-1100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the legacy string overload here.
buildContextWithMetadataalready takesSendMessageInput, butbuildContextstill acceptsstring | SendMessageInputand normalizes strings inline. Narrow this toSendMessageInputand update the remaining raw-string test calls intest/main/presenter/agentRuntimePresenter/contextBuilder.test.tsandtest/main/presenter/agentRuntimePresenter/tapeService.test.ts.🤖 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/main/presenter/agentRuntimePresenter/contextBuilder.ts` around lines 1088 - 1100, Remove the string overload from buildContext by changing its newUserContent parameter to SendMessageInput and passing it directly to buildContextWithMetadata without inline normalization. Update remaining buildContext callers in contextBuilder.test.ts and tapeService.test.ts to provide SendMessageInput objects instead of raw strings.
🧹 Nitpick comments (2)
src/main/presenter/agentRuntimePresenter/process.ts (1)
562-594: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated
max_tool_callschecks.The
outcome.type === 'max_tool_calls'condition is evaluated twice here, separated by the abort check. Consolidating these blocks into a single branch improves readability and keeps the logic tightly coupled.If the intention is to skip marking tool calls as aborted when a user explicitly cancels the generation, placing the consolidated block immediately after the abort check handles this cleanly.
♻️ Proposed refactor
- if (outcome.type === 'max_tool_calls') { - logger.info( - `[ProcessStream] max tool calls reached (${outcome.attemptedToolCount} > ${outcome.limit}), stopping` - ) - state.planTerminalReason = 'max_steps' - markUnexecutedToolCallsForLimit(state) - } - if (io.abortSignal.aborted) { stampRunOutcome(state, 'aborted', 'user_stop') finalizeUserCanceledErrorIfNeeded(state, io) return { status: 'aborted', stopReason: 'user_stop', errorMessage: USER_CANCELED_GENERATION_ERROR, usage: buildUsageSnapshot(state) } } if (outcome.type === 'max_tool_calls') { + logger.info( + `[ProcessStream] max tool calls reached (${outcome.attemptedToolCount} > ${outcome.limit}), stopping` + ) + state.planTerminalReason = 'max_steps' + markUnexecutedToolCallsForLimit(state) stampRunOutcome(state, 'completed', 'max_tool_calls') outputSink.complete({ runId: run.runId,🤖 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/main/presenter/agentRuntimePresenter/process.ts` around lines 562 - 594, Consolidate the two outcome.type === 'max_tool_calls' blocks in the process stream flow into one branch positioned after the io.abortSignal.aborted handling. Preserve the existing max-tool-call logging, planTerminalReason assignment, markUnexecutedToolCallsForLimit call, completion signaling, and return values, while ensuring user cancellation is handled first.src/main/presenter/agentRuntimePresenter/index.ts (1)
873-876: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract repeated inline normalization to a helper.
The fallback logic coercing
string | SendMessageInputto{ text, files: [] }is duplicated across four methods. Consider extracting this into a private helper to keep the code DRY.
src/main/presenter/agentRuntimePresenter/index.ts#L873-L876: Extract to a private methodtoSendMessageInput(content: string | SendMessageInput): SendMessageInput.src/main/presenter/agentRuntimePresenter/index.ts#L907-L908: Call the helper here.src/main/presenter/agentRuntimePresenter/index.ts#L967-L968: Call the helper here.src/main/presenter/agentRuntimePresenter/index.ts#L1045-L1046: Call the helper here.🤖 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/main/presenter/agentRuntimePresenter/index.ts` around lines 873 - 876, Extract the duplicated string-or-input normalization into a private toSendMessageInput(content: string | SendMessageInput): SendMessageInput helper in src/main/presenter/agentRuntimePresenter/index.ts, preserving the existing text and empty-files fallback. Replace the inline normalization at lines 873-876, 907-908, 967-968, and 1045-1046 with calls to this helper; all four sites require the same change.
🤖 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 `@src/main/agent/acp/runtime/acpContentMapper.ts`:
- Around line 11-28: Add a default branch to the stop-reason switch in the ACP
content-mapping function so unexpected protocol values always return a valid
event array. Preserve existing mappings and use the established error-and-stop
event pattern for the fallback, preventing callers from receiving undefined.
In `@test/main/presenter/usageStatsService.test.ts`:
- Line 439: Update the SessionRow type and the deepchatSessionsTable.create mock
to include a permissionMode field and accept the fourth argument, respectively.
Store the provided permissionMode when creating session rows so the mock matches
the table API and schema used by the test setup.
---
Outside diff comments:
In `@src/main/presenter/agentRuntimePresenter/contextBuilder.ts`:
- Around line 1088-1100: Remove the string overload from buildContext by
changing its newUserContent parameter to SendMessageInput and passing it
directly to buildContextWithMetadata without inline normalization. Update
remaining buildContext callers in contextBuilder.test.ts and tapeService.test.ts
to provide SendMessageInput objects instead of raw strings.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 873-876: Extract the duplicated string-or-input normalization into
a private toSendMessageInput(content: string | SendMessageInput):
SendMessageInput helper in src/main/presenter/agentRuntimePresenter/index.ts,
preserving the existing text and empty-files fallback. Replace the inline
normalization at lines 873-876, 907-908, 967-968, and 1045-1046 with calls to
this helper; all four sites require the same change.
In `@src/main/presenter/agentRuntimePresenter/process.ts`:
- Around line 562-594: Consolidate the two outcome.type === 'max_tool_calls'
blocks in the process stream flow into one branch positioned after the
io.abortSignal.aborted handling. Preserve the existing max-tool-call logging,
planTerminalReason assignment, markUnexecutedToolCallsForLimit call, completion
signaling, and return values, while ensuring user cancellation is handled first.
🪄 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: 801bd9ea-f56e-4796-803e-db57b95655d8
📒 Files selected for processing (61)
docs/architecture/agent-system.mddocs/architecture/permission-mode-policy-owner/spec.mddocs/architecture/provider-round-stop-contract/spec.mddocs/architecture/send-message-canonical-boundary/spec.mddocs/issues/agent-terminal-outcome-consistency/spec.mdsrc/main/agent/acp/instance/acpAgentInstance.tssrc/main/agent/acp/instance/acpAgentRuntime.tssrc/main/agent/acp/instance/ports.tssrc/main/agent/acp/runtime/acpContentMapper.tssrc/main/agent/acp/runtime/index.tssrc/main/agent/deepchat/deepChatAgentRepository.tssrc/main/agent/deepchat/pending/pendingInputCoordinator.tssrc/main/agent/deepchat/pending/pendingInputStore.tssrc/main/agent/manager/deepChatAgentBackend.tssrc/main/agent/manager/sessionHandles.tssrc/main/agent/shared/agentSessionHandle.tssrc/main/presenter/agentRuntimePresenter/accumulator.tssrc/main/presenter/agentRuntimePresenter/acpCompatibilityAdapters.tssrc/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.tssrc/main/presenter/agentRuntimePresenter/compactionService.tssrc/main/presenter/agentRuntimePresenter/contextBuilder.tssrc/main/presenter/agentRuntimePresenter/deferredToolExecutor.tssrc/main/presenter/agentRuntimePresenter/generationSettings.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.tssrc/main/presenter/agentRuntimePresenter/sessionStore.tssrc/main/presenter/agentRuntimePresenter/tapeViewPolicy.tssrc/main/presenter/agentRuntimePresenter/turnCoordinator.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/llmProviderPresenter/aiSdk/streamAdapter.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/sessionApplication/agentAssignmentPolicy.tssrc/main/presenter/sessionApplication/turnCoordinator.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/shared/contracts/routes/sessions.routes.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/llm-events.tstest/main/agent/acp/instance/acpAgentRuntime.test.tstest/main/agent/acp/runtime/acpContentMapper.test.tstest/main/agent/deepchat/pending/pendingInputStore.test.tstest/main/agent/manager/deepChatAgentBackend.test.tstest/main/agent/manager/directAcpAgentBackend.test.tstest/main/presenter/agentRepository.test.tstest/main/presenter/agentRuntimePresenter/accumulator.test.tstest/main/presenter/agentRuntimePresenter/acpCompatibilityAdapters.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/compactionService.test.tstest/main/presenter/agentRuntimePresenter/contextBuilder.test.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/agentRuntimePresenter/tapeService.test.tstest/main/presenter/agentRuntimePresenter/tapeViewAssembler.test.tstest/main/presenter/agentRuntimePresenter/tapeViewPolicy.test.tstest/main/presenter/llmProviderPresenter/aiSdkStreamAdapter.test.tstest/main/presenter/sessionApplication/agentAssignmentPolicy.test.tstest/main/presenter/sessionApplication/runtimeIntegration.test.tstest/main/presenter/sessionApplication/turnCoordinator.test.tstest/main/presenter/sqlitePresenter.test.tstest/main/presenter/sqlitePresenter/deepchatSessionsTable.test.tstest/main/presenter/usageStatsService.test.tstest/main/routes/contracts.test.ts
💤 Files with no reviewable changes (1)
- docs/issues/agent-terminal-outcome-consistency/spec.md
zerob13
changed the title
zhangmo8
approved these changes
Jul 15, 2026
zhangmo8
left a comment
zhangmo8
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed current head 7d66ae8.
No blocking findings. The renderer is unchanged, and activeSkills/inlineItems remain preserved through send, steer, initial-turn, queue, persistence, and ACP paths, so normal skill chips in the composer are unaffected. Pending-input validation only changes behavior for malformed or corrupt payloads.
Validation performed:
- 222 focused main-process tests passed
- 26 ChatInputBox and MessageItemUser renderer tests passed
- typecheck:node and typecheck:web passed
- PR build and memory checks are green
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.
Summary
Why
Repeated normalization and lossy stop-reason mapping made downstream modules distrust typed callers and allowed invalid or bounded outcomes to be hidden. This change narrows internal contracts and keeps compatibility handling at owned boundaries. Pending payload corruption is logged but fails open so conversation continuity is not blocked.
Validation
Summary by CodeRabbit
max_turn_requests) are preserved; tool-use cancellations/refusals produce correct error+stop sequences without “successful completion” fallbacks.