refactor(agent): split runtime and harden loop#1959
Merged
Conversation
Extract AgentRuntimePresenter into focused services, fix multi-round usage/terminal outcomes, add no-progress and MCP abort guards, subagent run limits, and a CI native-agent eval baseline.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds a deterministic native-agent evaluation harness and CI command, expands terminal accounting and no-progress detection, propagates cancellation through agent, MCP, provider, and compaction flows, and adds subagent deadline/capacity guardrails. It also updates shared metadata contracts, persistence paths, architecture documentation, and extensive runtime tests. ChangesNative Agent Evaluation and Runtime Guardrails
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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 |
Keep DeepChatLoopEngine architecture while porting multi-round usage, terminal outcomes, no-progress guards, MCP abort, subagent limits, and native-agent eval onto the layered runtime.
Drop the obsolete runtime facade size guard, remove a retired message-store mock symbol, and apply format cleanup.
![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: 18
🧹 Nitpick comments (6)
test/main/presenter/agentRuntimePresenter/streamLifecycleService.test.ts (1)
771-844: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert request tracing never persists authorization secrets.
The fixture supplies
Bearer secret, but the assertions pass even if that value reachesinsertMessageTrace.Proposed regression assertion
expect(fixture.messageStore.insertMessageTrace).toHaveBeenCalledWith( expect.objectContaining({ sessionId: 's1', messageId: 'assistant-1', requestSeq: 1, endpoint: 'https://api.example.invalid/chat' }) ) + expect( + JSON.stringify(fixture.messageStore.insertMessageTrace.mock.calls[0]?.[0]) + ).not.toContain('Bearer secret')🤖 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 `@test/main/presenter/agentRuntimePresenter/streamLifecycleService.test.ts` around lines 771 - 844, Strengthen the insertMessageTrace assertion in the stream lifecycle test to verify persisted trace data does not contain the authorization header or Bearer secret supplied through traceConfig.persist. Keep the existing request metadata assertions and assert the sensitive value is absent from the recorded call.test/main/presenter/agentRuntimePresenter/generationControlService.test.ts (1)
31-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that replacing a run aborts the superseded controller.
The test permits same-session replacement but only checks ID uniqueness. Verify that the first controller cannot continue executing after ownership moves to the second run.
Proposed test coverage
- const first = service.registerActiveGeneration('s1', 'm1', new AbortController()) - const second = service.registerActiveGeneration('s1', 'm2', new AbortController()) + const firstController = new AbortController() + const secondController = new AbortController() + const first = service.registerActiveGeneration('s1', 'm1', firstController) + const second = service.registerActiveGeneration('s1', 'm2', secondController) expect(first.runId).toMatch(/^s1:[A-Za-z0-9_-]+$/) expect(second.runId).toMatch(/^s1:[A-Za-z0-9_-]+$/) expect(second.runId).not.toBe(first.runId) + expect(firstController.signal.aborted).toBe(true) + expect(secondController.signal.aborted).toBe(false)🤖 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 `@test/main/presenter/agentRuntimePresenter/generationControlService.test.ts` around lines 31 - 38, Update the repeated-run test around registerActiveGeneration to retain the first AbortController, register the second run for the same session, and assert that the first controller’s signal is aborted after replacement. Preserve the existing run ID format and uniqueness assertions.src/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.ts (1)
68-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNested string values inside tool results bypass volatile normalization.
normalizeResultValue's string branch returns the value unchanged instead of applyingnormalizeVolatileString. Only top-level string content (parse failure or bare-string JSON) gets timestamp/UUID stripping innormalizeToolResult. Any nested string field carrying an embedded timestamp/UUID (not matching aVOLATILE_RESULT_KEYSname) will make the fingerprint differ across otherwise-identical batches, weakening no-progress detection (false negatives).♻️ Suggested fix
if (typeof value === 'string') { - return value + return normalizeVolatileString(value) }🤖 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/noProgressToolLoopGuard.ts` around lines 68 - 92, Update normalizeResultValue so every string value, including nested array and object entries, passes through normalizeVolatileString before returning. Preserve the existing VOLATILE_RESULT_KEYS handling and recursive traversal so keyed volatile values still normalize to <volatile>.test/main/presenter/agentRuntimePresenter/accumulator.test.ts (1)
237-266: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a second-round test for
commitRoundUsage.Only a single round is exercised here. Given "cumulative multi-round usage accounting" is a headline reliability goal of this PR, add a case that calls
accumulate+commitRoundUsagetwice (simulating two provider rounds) to confirm totals accumulate correctly across rounds androundUsagedoesn't leak stale values into the next round.🤖 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 `@test/main/presenter/agentRuntimePresenter/accumulator.test.ts` around lines 237 - 266, Add a second-round scenario to the existing “usage keeps the latest round snapshot and commits it to metadata once” test, calling accumulate and commitRoundUsage twice with distinct usage values. Assert metadata totals accumulate across both rounds and verify roundUsage is cleared after each commit so values from the first round do not leak into the second.test/main/presenter/agentRuntimePresenter/turnPreparationService.test.ts (1)
351-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both sides of the ACP-subagent conjunction.
Add cases for an ACP regular session and a non-ACP subagent. The current positive case would still pass if the implementation incorrectly bypassed tools for either condition independently.
🤖 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 `@test/main/presenter/agentRuntimePresenter/turnPreparationService.test.ts` around lines 351 - 364, Add separate test cases covering the negative sides of the ACP-subagent condition: an ACP-backed regular session and a non-ACP subagent session. In each case, verify local tools and prompt layers are not bypassed, using the existing loadToolDefinitionsForSession and buildSystemPromptWithSkills assertions and mocks.test/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.test.ts (1)
107-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive coverage for the hard-stop path.
These assertions prove weak acknowledgements never terminate, but no test drives substantive identical results until
shouldTerminatebecomestrue. A regression disabling repeated-tool-loop termination would remain green.🤖 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 `@test/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.test.ts` around lines 107 - 124, Add a test case alongside the existing NoProgressToolLoopGuard coverage that repeatedly observes identical substantive tool results until the guard reaches its hard-stop threshold, then assert the returned observation has shouldTerminate: true. Keep the acknowledgement-only scenario unchanged and use the existing guard, toolCall, and toolMessage helpers to exercise the termination path.
🤖 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 `@docs/architecture/agent-system.md`:
- Around line 74-99: Update the agentRuntimePresenter directory tree in the
architecture documentation to include the existing accumulator.ts,
tapeService.ts, and types.ts modules, preserving the current entries and tree
formatting.
In `@src/main/presenter/agentRuntimePresenter/generationControlService.ts`:
- Around line 82-94: The registerActiveGeneration flow must dispose of any
existing same-session generation before replacing ownership: abort or reject the
prior active/preparation run, then store the new generation and controller.
Update src/main/presenter/agentRuntimePresenter/generationControlService.ts
lines 82-94 accordingly, and add coverage in
test/main/presenter/agentRuntimePresenter/generationControlService.test.ts lines
31-38 asserting the first controller is aborted while the newly registered
controller remains active.
In `@src/main/presenter/agentRuntimePresenter/interactionResumeService.ts`:
- Around line 340-387: Update normalizeReviewDecision to require a valid
normalized riskLevel before honoring any allow/auto_allow decision. When
riskLevel is missing or invalid, return ask_user with the existing actionHash
and rationale handling; only permit execution after accepted risk
classification, while preserving critical/high risk restrictions.
In `@src/main/presenter/agentRuntimePresenter/memoryCompactionService.ts`:
- Around line 244-254: Update cancelManualCompaction and its corresponding
cancellation path to restore the persisted session summary state after
invalidateManualCompaction changes the epoch. Refresh the deleted compaction
message using the current epoch/state, while ensuring the invalidated run cannot
overwrite a newer compaction run; ensure sessionCompactionStates no longer
remains 'compacting' after cancellation.
- Around line 1062-1085: Update extractPlainTextFromRecord to return the
original record.content when JSON parsing fails, preserving raw plain-text
messages for memory ingestion while keeping the existing parsed user and
assistant-content extraction behavior unchanged.
In `@src/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.ts`:
- Around line 167-183: Update appendCorrection to clone the selected
lastToolMessage before modifying its content, then replace the corresponding
message in messages with that clone. Preserve the existing correction formatting
and return behavior while ensuring the original ChatMessage objects remain
unchanged for consumers of the input array.
In `@src/main/presenter/agentRuntimePresenter/sessionLifecycleService.ts`:
- Around line 61-64: Update the initSession logging in sessionLifecycleService
to stop including the full projectDir value. Replace it with a boolean or
equivalent indicator showing only whether a project directory is configured,
while preserving the existing session, provider, model, and permission fields.
- Around line 255-263: Update resolveProjectDir to load the persisted previous
project directory when sessionProjectDirs has no cached entry, before comparing
it with the normalized incoming directory. Preserve null handling for sessions
without a persisted directory, and ensure the existing prompt and tool profile
cache invalidation runs when the persisted and incoming directories differ.
- Around line 304-330: Update the session-state handling in the runtimeState and
rebuilt-state branches so pending interactions only override the status on the
returned session state, without mutating or caching status as “generating” in
runtimeSharedState.runtimeState. Preserve the underlying cached/rebuilt status,
and apply the interaction-derived overlay when constructing the return value.
In `@src/main/presenter/agentRuntimePresenter/sessionSettingsService.ts`:
- Around line 78-85: Update setPermissionMode to call
sessionStore.updatePermissionMode before mutating
runtimeSharedState.runtimeState, so failed persistence leaves in-memory state
unchanged. In the model-switching flow around the lines 109-124, transactionally
persist both the model and sanitized replacement settings before updating
runtime/cache state. In the generation-settings update flow around lines
160-166, complete persistence before replacing cached generation settings.
- Around line 59-61: Update normalizePermissionMode so unknown, null, or
undefined values fall back to the least-permissive default mode, default,
instead of full_access; preserve the existing handling of default and
auto_approve.
In `@src/main/presenter/agentRuntimePresenter/streamLifecycleService.ts`:
- Around line 318-321: Update the processMessage logging around the
DeepChatAgent info call to remove normalizedInput.text content entirely; retain
only non-sensitive metadata such as the input length alongside the existing
sessionId and projectDir fields.
In `@src/main/presenter/agentRuntimePresenter/turnPreparationService.ts`:
- Around line 262-315: Make every pre-stream await that does not accept an
AbortSignal abort-responsive by wrapping it with the existing awaitWithAbort
helper, including session settings, active-skill resolution, tool definitions,
system-prompt construction, and the corresponding manual-compaction path.
Preserve the existing signal propagation for operations that already accept it,
and keep the post-await throwIfAbortRequested checks.
- Around line 253-256: Update the logging in the prepareNewTurn flow to remove
raw prompt content and the resolved projectDir from the DeepChatAgent log.
Replace them with non-sensitive metadata such as normalized input length and a
boolean indicating whether projectDir is present, while preserving the session
identifier and existing logging context.
In `@src/main/presenter/mcpPresenter/toolManager.ts`:
- Around line 489-491: Update the refresh flow around getAllToolDefinitions and
the related logic at the additional referenced location so an aborted or
superseded refresh cannot commit cachedToolDefinitions or toolNameToTargetMap.
Track the active refresh generation or signal, capture it when starting the
refresh, and validate it remains current and non-aborted immediately before
shared-state mutation; discard stale results otherwise while preserving the
existing abort behavior.
In `@test/main/presenter/agentRuntimePresenter/memoryCompactionService.test.ts`:
- Around line 205-214: Update the test for service.applyCompactionIntent so it
asserts the standardized cancellation outcome when the signal is aborted before
the provider throws, rather than expecting the late “late failure” error.
Preserve the existing setup and verify that the transient message is removed and
prior state is restored as indicated by the test name.
In `@test/main/presenter/agentRuntimePresenter/tapeAccessService.test.ts`:
- Around line 96-104: Update the test for discardSubagentTape to assert call
order, not just invocation counts: verify tapeService.ensureSessionTapeReady
completes before tapeService.recordExternalForkDiscard is called. Keep the
existing argument assertions and ensure the test still covers readiness for the
parent session only.
In `@test/main/presenter/agentSessionPresenter/integration.test.ts`:
- Around line 361-364: Update the integration mock’s updateMetadata callback to
refresh the message’s updated_at timestamp whenever metadata is changed,
matching production behavior while preserving the existing messagesStore lookup
and metadata assignment in updateMetadata.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.ts`:
- Around line 68-92: Update normalizeResultValue so every string value,
including nested array and object entries, passes through
normalizeVolatileString before returning. Preserve the existing
VOLATILE_RESULT_KEYS handling and recursive traversal so keyed volatile values
still normalize to <volatile>.
In `@test/main/presenter/agentRuntimePresenter/accumulator.test.ts`:
- Around line 237-266: Add a second-round scenario to the existing “usage keeps
the latest round snapshot and commits it to metadata once” test, calling
accumulate and commitRoundUsage twice with distinct usage values. Assert
metadata totals accumulate across both rounds and verify roundUsage is cleared
after each commit so values from the first round do not leak into the second.
In `@test/main/presenter/agentRuntimePresenter/generationControlService.test.ts`:
- Around line 31-38: Update the repeated-run test around
registerActiveGeneration to retain the first AbortController, register the
second run for the same session, and assert that the first controller’s signal
is aborted after replacement. Preserve the existing run ID format and uniqueness
assertions.
In `@test/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.test.ts`:
- Around line 107-124: Add a test case alongside the existing
NoProgressToolLoopGuard coverage that repeatedly observes identical substantive
tool results until the guard reaches its hard-stop threshold, then assert the
returned observation has shouldTerminate: true. Keep the acknowledgement-only
scenario unchanged and use the existing guard, toolCall, and toolMessage helpers
to exercise the termination path.
In `@test/main/presenter/agentRuntimePresenter/streamLifecycleService.test.ts`:
- Around line 771-844: Strengthen the insertMessageTrace assertion in the stream
lifecycle test to verify persisted trace data does not contain the authorization
header or Bearer secret supplied through traceConfig.persist. Keep the existing
request metadata assertions and assert the sensitive value is absent from the
recorded call.
In `@test/main/presenter/agentRuntimePresenter/turnPreparationService.test.ts`:
- Around line 351-364: Add separate test cases covering the negative sides of
the ACP-subagent condition: an ACP-backed regular session and a non-ACP subagent
session. In each case, verify local tools and prompt layers are not bypassed,
using the existing loadToolDefinitionsForSession and buildSystemPromptWithSkills
assertions and mocks.
🪄 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: 4bacd920-c9b7-471d-9876-9074f3d79fea
📒 Files selected for processing (87)
.github/workflows/prcheck.ymldocs/ARCHITECTURE.mddocs/architecture/agent-runtime-presenter-split/plan.mddocs/architecture/agent-runtime-presenter-split/spec.mddocs/architecture/agent-runtime-presenter-split/state-map.mddocs/architecture/agent-runtime-presenter-split/tasks.mddocs/architecture/agent-system.mddocs/architecture/native-agent-evaluation/plan.mddocs/architecture/native-agent-evaluation/spec.mddocs/architecture/native-agent-evaluation/tasks.mddocs/architecture/subagent-run-guardrails/plan.mddocs/architecture/subagent-run-guardrails/spec.mddocs/architecture/subagent-run-guardrails/tasks.mddocs/features/mock-long-chat-debug-data/spec.mddocs/issues/agent-multiround-usage/spec.mddocs/issues/agent-repeated-tool-loop/spec.mddocs/issues/agent-terminal-outcome-consistency/spec.mddocs/issues/mcp-tool-cancellation/spec.mdpackage.jsonscripts/agent-cleanup-guard.mjssrc/main/lib/awaitWithAbort.tssrc/main/lib/toolCallImagePreviews.tssrc/main/presenter/agentRuntimePresenter/accumulator.tssrc/main/presenter/agentRuntimePresenter/compactionService.tssrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/generationControlService.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/interactionResumeService.tssrc/main/presenter/agentRuntimePresenter/memoryCompactionService.tssrc/main/presenter/agentRuntimePresenter/messageHistoryService.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.tssrc/main/presenter/agentRuntimePresenter/pendingInputService.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/agentRuntimePresenter/runtimeSharedState.tssrc/main/presenter/agentRuntimePresenter/sessionLifecycleService.tssrc/main/presenter/agentRuntimePresenter/sessionSettingsService.tssrc/main/presenter/agentRuntimePresenter/streamLifecycleService.tssrc/main/presenter/agentRuntimePresenter/tapeAccessService.tssrc/main/presenter/agentRuntimePresenter/tapeService.tssrc/main/presenter/agentRuntimePresenter/turnPreparationService.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/llmProviderPresenter/index.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/mcpClient.tssrc/main/presenter/mcpPresenter/toolManager.tssrc/main/presenter/sqlitePresenter/tables/deepchatMessages.tssrc/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/src/components/chat/messageListItems.tssrc/shared/chat.d.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat.tssrc/shared/types/presenters/core.presenter.d.tssrc/shared/types/presenters/tool.presenter.d.tstest/main/evals/nativeAgent/harness.tstest/main/evals/nativeAgent/nativeAgentBehavior.eval.test.tstest/main/evals/nativeAgent/scenarios.tstest/main/lib/awaitWithAbort.test.tstest/main/lib/toolCallImagePreviews.test.tstest/main/presenter/agentRuntimePresenter/accumulator.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/compactionService.test.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/agentRuntimePresenter/generationControlService.test.tstest/main/presenter/agentRuntimePresenter/interactionResumeService.test.tstest/main/presenter/agentRuntimePresenter/memoryCompactionService.test.tstest/main/presenter/agentRuntimePresenter/messageHistoryService.test.tstest/main/presenter/agentRuntimePresenter/messageStore.test.tstest/main/presenter/agentRuntimePresenter/noProgressToolLoopGuard.test.tstest/main/presenter/agentRuntimePresenter/pendingInputService.test.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/agentRuntimePresenter/serviceConstruction.test.tstest/main/presenter/agentRuntimePresenter/sessionLifecycleService.test.tstest/main/presenter/agentRuntimePresenter/sessionSettingsService.test.tstest/main/presenter/agentRuntimePresenter/streamLifecycleService.test.tstest/main/presenter/agentRuntimePresenter/tapeAccessService.test.tstest/main/presenter/agentRuntimePresenter/turnPreparationService.test.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/llmProviderPresenter.test.tstest/main/presenter/mcpClient.test.tstest/main/presenter/mcpPresenter/toolManager.test.tstest/main/presenter/openaiCodexAuth.test.tstest/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.tstest/main/presenter/toolPresenter/toolPresenter.test.tstest/main/routes/debug/createMockChatSession.test.tstest/main/scripts/architectureGuard.test.ts
Comment on lines
74
to
99
| ```text | ||
| input/context preparation | ||
| -> register LoopRun | ||
| -> enter provider round | ||
| -> provider attempt: context gate -> ViewManifest -> rate gate -> stream | ||
| -> update output projection | ||
| -> execute typed tool batch | ||
| -> message commit -> TapeRecorder tool facts | ||
| -> continue or settle | ||
| -> terminal projection / pending drain / Memory observer | ||
| agentRuntimePresenter/ | ||
| ├── index.ts | ||
| ├── runtimeSharedState.ts | ||
| ├── sessionSettingsService.ts | ||
| ├── generationControlService.ts | ||
| ├── sessionLifecycleService.ts | ||
| ├── turnPreparationService.ts | ||
| ├── streamLifecycleService.ts | ||
| ├── interactionResumeService.ts | ||
| ├── memoryCompactionService.ts | ||
| ├── messageHistoryService.ts | ||
| ├── tapeAccessService.ts | ||
| ├── process.ts | ||
| ├── dispatch.ts | ||
| ├── contextBuilder.ts | ||
| ├── sessionStore.ts | ||
| ├── messageStore.ts | ||
| ├── pendingInputStore.ts | ||
| ├── pendingInputCoordinator.ts | ||
| ├── pendingInputService.ts | ||
| ├── compactionService.ts | ||
| ├── noProgressToolLoopGuard.ts | ||
| ├── echo.ts | ||
| └── toolOutputGuard.ts | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include all runtime modules in the directory tree.
The tree omits existing accumulator.ts, tapeService.ts, and types.ts, so it no longer accurately documents the directory layout.
Proposed documentation fix
agentRuntimePresenter/
├── index.ts
+├── types.ts
+├── accumulator.ts
├── runtimeSharedState.ts
...
├── tapeAccessService.ts
+├── tapeService.ts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```text | |
| input/context preparation | |
| -> register LoopRun | |
| -> enter provider round | |
| -> provider attempt: context gate -> ViewManifest -> rate gate -> stream | |
| -> update output projection | |
| -> execute typed tool batch | |
| -> message commit -> TapeRecorder tool facts | |
| -> continue or settle | |
| -> terminal projection / pending drain / Memory observer | |
| agentRuntimePresenter/ | |
| ├── index.ts | |
| ├── runtimeSharedState.ts | |
| ├── sessionSettingsService.ts | |
| ├── generationControlService.ts | |
| ├── sessionLifecycleService.ts | |
| ├── turnPreparationService.ts | |
| ├── streamLifecycleService.ts | |
| ├── interactionResumeService.ts | |
| ├── memoryCompactionService.ts | |
| ├── messageHistoryService.ts | |
| ├── tapeAccessService.ts | |
| ├── process.ts | |
| ├── dispatch.ts | |
| ├── contextBuilder.ts | |
| ├── sessionStore.ts | |
| ├── messageStore.ts | |
| ├── pendingInputStore.ts | |
| ├── pendingInputCoordinator.ts | |
| ├── pendingInputService.ts | |
| ├── compactionService.ts | |
| ├── noProgressToolLoopGuard.ts | |
| ├── echo.ts | |
| └── toolOutputGuard.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 `@docs/architecture/agent-system.md` around lines 74 - 99, Update the
agentRuntimePresenter directory tree in the architecture documentation to
include the existing accumulator.ts, tapeService.ts, and types.ts modules,
preserving the current entries and tree formatting.
Comment on lines
+82
to
+94
| registerActiveGeneration( | ||
| sessionId: string, | ||
| messageId: string, | ||
| abortController: AbortController | ||
| ): ActiveGeneration { | ||
| const generation: ActiveGeneration = { | ||
| runId: `${sessionId}:${this.createRunToken()}`, | ||
| messageId, | ||
| abortController | ||
| } | ||
| this.runtimeSharedState.activeGenerations.set(sessionId, generation) | ||
| this.runtimeSharedState.abortControllers.set(sessionId, abortController) | ||
| return generation |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Dispose of superseded same-session generations before transferring ownership.
Repeated registration currently drops the previous controller without aborting it, allowing unreachable work to continue concurrently.
src/main/presenter/agentRuntimePresenter/generationControlService.ts#L82-L94: abort or reject the existing active/preparation run before replacing the map entries.test/main/presenter/agentRuntimePresenter/generationControlService.test.ts#L31-L38: assert that replacement aborts the first controller while leaving the new controller active.
📍 Affects 2 files
src/main/presenter/agentRuntimePresenter/generationControlService.ts#L82-L94(this comment)test/main/presenter/agentRuntimePresenter/generationControlService.test.ts#L31-L38
🤖 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/generationControlService.ts` around
lines 82 - 94, The registerActiveGeneration flow must dispose of any existing
same-session generation before replacing ownership: abort or reject the prior
active/preparation run, then store the new generation and controller. Update
src/main/presenter/agentRuntimePresenter/generationControlService.ts lines 82-94
accordingly, and add coverage in
test/main/presenter/agentRuntimePresenter/generationControlService.test.ts lines
31-38 asserting the first controller is aborted while the newly registered
controller remains active.
Comment on lines
+340
to
+387
| function normalizeReviewDecision(rawText: string, actionHash: string): ToolPermissionReviewResult { | ||
| const jsonText = extractJsonObjectText(rawText) | ||
| if (!jsonText) { | ||
| return { decision: 'ask_user', rationale: 'Auto-review did not return JSON.', actionHash } | ||
| } | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(jsonText) as Record<string, unknown> | ||
| const rawDecision = parsed.decision ?? parsed.outcome | ||
| const riskLevel = normalizeRiskLevel(parsed.riskLevel ?? parsed.risk_level) | ||
| const userAuthorization = normalizeUserAuthorization( | ||
| parsed.userAuthorization ?? parsed.user_authorization | ||
| ) | ||
| const echoedActionHash = | ||
| typeof parsed.actionHash === 'string' | ||
| ? parsed.actionHash | ||
| : typeof parsed.action_hash === 'string' | ||
| ? parsed.action_hash | ||
| : undefined | ||
| const rationale = | ||
| typeof parsed.rationale === 'string' | ||
| ? parsed.rationale | ||
| : typeof parsed.reason === 'string' | ||
| ? parsed.reason | ||
| : undefined | ||
|
|
||
| if (echoedActionHash !== actionHash) { | ||
| return { | ||
| decision: 'ask_user', | ||
| riskLevel, | ||
| userAuthorization, | ||
| rationale: 'Auto-review action hash mismatch.', | ||
| actionHash | ||
| } | ||
| } | ||
|
|
||
| let decision: ToolPermissionReviewResult['decision'] | ||
| if (rawDecision === 'auto_allow' || rawDecision === 'allow') { | ||
| decision = 'auto_allow' | ||
| } else if (rawDecision === 'block' || rawDecision === 'deny') { | ||
| decision = riskLevel === 'critical' ? 'block' : 'ask_user' | ||
| } else { | ||
| decision = 'ask_user' | ||
| } | ||
| if (riskLevel === 'critical') decision = 'block' | ||
| else if (riskLevel === 'high') decision = 'ask_user' | ||
|
|
||
| return { decision, riskLevel, userAuthorization, rationale, actionHash } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when the reviewer omits its risk classification.
A hash-matching {"decision":"allow"} response with no valid riskLevel currently becomes auto_allow. Require an accepted risk level before authorizing execution; malformed model output must fall back to ask_user.
Proposed fix
- if (rawDecision === 'auto_allow' || rawDecision === 'allow') {
+ if (
+ (rawDecision === 'auto_allow' || rawDecision === 'allow') &&
+ (riskLevel === 'low' || riskLevel === 'medium')
+ ) {
decision = 'auto_allow'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function normalizeReviewDecision(rawText: string, actionHash: string): ToolPermissionReviewResult { | |
| const jsonText = extractJsonObjectText(rawText) | |
| if (!jsonText) { | |
| return { decision: 'ask_user', rationale: 'Auto-review did not return JSON.', actionHash } | |
| } | |
| try { | |
| const parsed = JSON.parse(jsonText) as Record<string, unknown> | |
| const rawDecision = parsed.decision ?? parsed.outcome | |
| const riskLevel = normalizeRiskLevel(parsed.riskLevel ?? parsed.risk_level) | |
| const userAuthorization = normalizeUserAuthorization( | |
| parsed.userAuthorization ?? parsed.user_authorization | |
| ) | |
| const echoedActionHash = | |
| typeof parsed.actionHash === 'string' | |
| ? parsed.actionHash | |
| : typeof parsed.action_hash === 'string' | |
| ? parsed.action_hash | |
| : undefined | |
| const rationale = | |
| typeof parsed.rationale === 'string' | |
| ? parsed.rationale | |
| : typeof parsed.reason === 'string' | |
| ? parsed.reason | |
| : undefined | |
| if (echoedActionHash !== actionHash) { | |
| return { | |
| decision: 'ask_user', | |
| riskLevel, | |
| userAuthorization, | |
| rationale: 'Auto-review action hash mismatch.', | |
| actionHash | |
| } | |
| } | |
| let decision: ToolPermissionReviewResult['decision'] | |
| if (rawDecision === 'auto_allow' || rawDecision === 'allow') { | |
| decision = 'auto_allow' | |
| } else if (rawDecision === 'block' || rawDecision === 'deny') { | |
| decision = riskLevel === 'critical' ? 'block' : 'ask_user' | |
| } else { | |
| decision = 'ask_user' | |
| } | |
| if (riskLevel === 'critical') decision = 'block' | |
| else if (riskLevel === 'high') decision = 'ask_user' | |
| return { decision, riskLevel, userAuthorization, rationale, actionHash } | |
| function normalizeReviewDecision(rawText: string, actionHash: string): ToolPermissionReviewResult { | |
| const jsonText = extractJsonObjectText(rawText) | |
| if (!jsonText) { | |
| return { decision: 'ask_user', rationale: 'Auto-review did not return JSON.', actionHash } | |
| } | |
| try { | |
| const parsed = JSON.parse(jsonText) as Record<string, unknown> | |
| const rawDecision = parsed.decision ?? parsed.outcome | |
| const riskLevel = normalizeRiskLevel(parsed.riskLevel ?? parsed.risk_level) | |
| const userAuthorization = normalizeUserAuthorization( | |
| parsed.userAuthorization ?? parsed.user_authorization | |
| ) | |
| const echoedActionHash = | |
| typeof parsed.actionHash === 'string' | |
| ? parsed.actionHash | |
| : typeof parsed.action_hash === 'string' | |
| ? parsed.action_hash | |
| : undefined | |
| const rationale = | |
| typeof parsed.rationale === 'string' | |
| ? parsed.rationale | |
| : typeof parsed.reason === 'string' | |
| ? parsed.reason | |
| : undefined | |
| if (echoedActionHash !== actionHash) { | |
| return { | |
| decision: 'ask_user', | |
| riskLevel, | |
| userAuthorization, | |
| rationale: 'Auto-review action hash mismatch.', | |
| actionHash | |
| } | |
| } | |
| let decision: ToolPermissionReviewResult['decision'] | |
| if ( | |
| (rawDecision === 'auto_allow' || rawDecision === 'allow') && | |
| (riskLevel === 'low' || riskLevel === 'medium') | |
| ) { | |
| decision = 'auto_allow' | |
| } else if (rawDecision === 'block' || rawDecision === 'deny') { | |
| decision = riskLevel === 'critical' ? 'block' : 'ask_user' | |
| } else { | |
| decision = 'ask_user' | |
| } | |
| if (riskLevel === 'critical') decision = 'block' | |
| else if (riskLevel === 'high') decision = 'ask_user' | |
| return { decision, riskLevel, userAuthorization, rationale, actionHash } |
🤖 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/interactionResumeService.ts` around
lines 340 - 387, Update normalizeReviewDecision to require a valid normalized
riskLevel before honoring any allow/auto_allow decision. When riskLevel is
missing or invalid, return ask_user with the existing actionHash and rationale
handling; only permit execution after accepted risk classification, while
preserving critical/high risk restrictions.
Comment on lines
+244
to
+254
| cancelManualCompaction(sessionId: string): boolean { | ||
| const controller = this.manualCompactionControllers.get(sessionId) | ||
| if (!controller || controller.signal.aborted) return false | ||
| this.invalidateManualCompaction(sessionId) | ||
| if ( | ||
| !this.dependencies.runtimeSharedState.activeGenerations.has(sessionId) && | ||
| !this.dependencies.runtimeSharedState.abortControllers.has(sessionId) | ||
| ) { | ||
| this.host.setSessionStatus(sessionId, 'idle') | ||
| } | ||
| return true |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore compaction state after cancellation invalidates the epoch.
Cancellation makes the catch-path epoch check fail, so the deleted compaction message is not refreshed and sessionCompactionStates remains 'compacting'. Subsequent getSessionCompactionState() calls return that stale state indefinitely. Explicitly restore the persisted summary state on cancellation and refresh the deleted message without allowing the old run to overwrite a newer run.
Also applies to: 308-316
🤖 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/memoryCompactionService.ts` around
lines 244 - 254, Update cancelManualCompaction and its corresponding
cancellation path to restore the persisted session summary state after
invalidateManualCompaction changes the epoch. Refresh the deleted compaction
message using the current epoch/state, while ensuring the invalidated run cannot
overwrite a newer compaction run; ensure sessionCompactionStates no longer
remains 'compacting' after cancellation.
Comment on lines
+1062
to
+1085
| private extractPlainTextFromRecord(record: Pick<ChatMessageRecord, 'role' | 'content'>): string { | ||
| try { | ||
| const parsed = JSON.parse(record.content) as unknown | ||
| if (record.role === 'user') { | ||
| const text = (parsed as { text?: unknown })?.text | ||
| return typeof text === 'string' ? text.trim() : '' | ||
| } | ||
| if (Array.isArray(parsed)) { | ||
| return parsed | ||
| .map((block) => { | ||
| const candidate = block as { type?: string; content?: unknown } | ||
| if (candidate?.type === 'content' && typeof candidate.content === 'string') { | ||
| return candidate.content | ||
| } | ||
| return '' | ||
| }) | ||
| .filter(Boolean) | ||
| .join(' ') | ||
| .trim() | ||
| } | ||
| return '' | ||
| } catch { | ||
| return '' | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve plain-text messages during memory extraction.
Non-JSON content currently becomes an empty string, although raw message text is supported elsewhere in this PR. This silently excludes legacy/plain-text messages from memory ingestion.
Proposed fix
try {
const parsed = JSON.parse(record.content) as unknown
+ if (typeof parsed === 'string') {
+ return parsed.trim()
+ }
if (record.role === 'user') {
const text = (parsed as { text?: unknown })?.text
return typeof text === 'string' ? text.trim() : ''
}
@@
} catch {
- return ''
+ return record.content.trim()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private extractPlainTextFromRecord(record: Pick<ChatMessageRecord, 'role' | 'content'>): string { | |
| try { | |
| const parsed = JSON.parse(record.content) as unknown | |
| if (record.role === 'user') { | |
| const text = (parsed as { text?: unknown })?.text | |
| return typeof text === 'string' ? text.trim() : '' | |
| } | |
| if (Array.isArray(parsed)) { | |
| return parsed | |
| .map((block) => { | |
| const candidate = block as { type?: string; content?: unknown } | |
| if (candidate?.type === 'content' && typeof candidate.content === 'string') { | |
| return candidate.content | |
| } | |
| return '' | |
| }) | |
| .filter(Boolean) | |
| .join(' ') | |
| .trim() | |
| } | |
| return '' | |
| } catch { | |
| return '' | |
| } | |
| private extractPlainTextFromRecord(record: Pick<ChatMessageRecord, 'role' | 'content'>): string { | |
| try { | |
| const parsed = JSON.parse(record.content) as unknown | |
| if (typeof parsed === 'string') { | |
| return parsed.trim() | |
| } | |
| if (record.role === 'user') { | |
| const text = (parsed as { text?: unknown })?.text | |
| return typeof text === 'string' ? text.trim() : '' | |
| } | |
| if (Array.isArray(parsed)) { | |
| return parsed | |
| .map((block) => { | |
| const candidate = block as { type?: string; content?: unknown } | |
| if (candidate?.type === 'content' && typeof candidate.content === 'string') { | |
| return candidate.content | |
| } | |
| return '' | |
| }) | |
| .filter(Boolean) | |
| .join(' ') | |
| .trim() | |
| } | |
| return '' | |
| } catch { | |
| return record.content.trim() | |
| } |
🤖 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/memoryCompactionService.ts` around
lines 1062 - 1085, Update extractPlainTextFromRecord to return the original
record.content when JSON parsing fails, preserving raw plain-text messages for
memory ingestion while keeping the existing parsed user and assistant-content
extraction behavior unchanged.
Comment on lines
+262
to
+315
| const generationSettings = | ||
| await this.dependencies.sessionSettingsService.getEffectiveGenerationSettings(sessionId) | ||
| this.logSlowPreStreamStep(sessionId, 'generation-settings', stepStartedAt) | ||
| const modelConfig = this.dependencies.configPresenter.getModelConfig( | ||
| state.modelId, | ||
| state.providerId | ||
| ) | ||
| const useContextBudget = this.shouldUseDeepChatContextBudget( | ||
| state.providerId, | ||
| modelConfig, | ||
| state.modelId | ||
| ) | ||
| this.throwIfAbortRequested(signal) | ||
| const interleavedReasoning = this.resolveInterleavedReasoningConfig( | ||
| state.providerId, | ||
| state.modelId, | ||
| generationSettings | ||
| ) | ||
| const contextBudgetLength = this.resolveDeepChatContextBudgetLength( | ||
| state.providerId, | ||
| generationSettings.contextLength, | ||
| modelConfig, | ||
| state.modelId | ||
| ) | ||
| const maxTokens = capAgentRequestMaxTokens(generationSettings.maxTokens, contextBudgetLength) | ||
|
|
||
| stepStartedAt = Date.now() | ||
| this.resetRuntimeActivatedSkills(sessionId) | ||
| this.setRuntimeActivatedSkills(sessionId, normalizedInput.activeSkills ?? []) | ||
| const sessionActiveSkillNames = await this.resolveActiveSkillNamesForToolProfile(sessionId) | ||
| const effectiveActiveSkillNames = this.resolveEffectiveActiveSkillNames( | ||
| sessionActiveSkillNames, | ||
| sessionId | ||
| ) | ||
| this.logSlowPreStreamStep(sessionId, 'active-skills', stepStartedAt) | ||
|
|
||
| stepStartedAt = Date.now() | ||
| const tools = await this.loadToolDefinitionsForSession( | ||
| sessionId, | ||
| projectDir, | ||
| effectiveActiveSkillNames | ||
| ) | ||
| this.logSlowPreStreamStep(sessionId, 'tool-definitions', stepStartedAt) | ||
| const toolReserveTokens = estimateToolReserveTokens(tools) | ||
| this.throwIfAbortRequested(signal) | ||
|
|
||
| stepStartedAt = Date.now() | ||
| const baseSystemPrompt = await this.buildSystemPromptWithSkills( | ||
| sessionId, | ||
| generationSettings.systemPrompt, | ||
| tools, | ||
| effectiveActiveSkillNames | ||
| ) | ||
| this.logSlowPreStreamStep(sessionId, 'system-prompt', stepStartedAt) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make pre-stream collaborator waits abort-responsive.
throwIfAbortRequested() runs only after each await. If settings, skill, tool, or prompt resolution never settles, cancellation cannot finish the turn. Wrap promises that do not accept a signal with awaitWithAbort, including the manual-compaction path.
Proposed approach
+import { awaitWithAbort } from '`@/lib/awaitWithAbort`'
- const generationSettings =
- await this.dependencies.sessionSettingsService.getEffectiveGenerationSettings(sessionId)
+ const generationSettings = await awaitWithAbort(
+ this.dependencies.sessionSettingsService.getEffectiveGenerationSettings(sessionId),
+ signal
+ )
- const sessionActiveSkillNames = await this.resolveActiveSkillNamesForToolProfile(sessionId)
+ const sessionActiveSkillNames = await awaitWithAbort(
+ this.resolveActiveSkillNamesForToolProfile(sessionId),
+ signal
+ )
- const tools = await this.loadToolDefinitionsForSession(
- sessionId,
- projectDir,
- effectiveActiveSkillNames
+ const tools = await awaitWithAbort(
+ this.loadToolDefinitionsForSession(sessionId, projectDir, effectiveActiveSkillNames),
+ signal
)Also applies to: 513-540
🤖 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/turnPreparationService.ts` around
lines 262 - 315, Make every pre-stream await that does not accept an AbortSignal
abort-responsive by wrapping it with the existing awaitWithAbort helper,
including session settings, active-skill resolution, tool definitions,
system-prompt construction, and the corresponding manual-compaction path.
Preserve the existing signal propagation for operations that already accept it,
and keep the post-await throwIfAbortRequested checks.
Comment on lines
+205
to
+214
| it('removes the transient message and restores prior state when cancellation wins', async () => { | ||
| const controller = new AbortController() | ||
| compactionService.applyCompaction.mockImplementation(async () => { | ||
| controller.abort() | ||
| throw new Error('late failure') | ||
| }) | ||
|
|
||
| await expect( | ||
| service.applyCompactionIntent('s1', intent(), { signal: controller.signal }) | ||
| ).rejects.toThrow('late failure') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the cancellation outcome rather than the late provider error.
The signal is aborted before late failure is thrown, yet the test requires that late error to escape. This contradicts both the test name and the PR’s standardized abort-outcome objective.
Proposed assertion
await expect(
service.applyCompactionIntent('s1', intent(), { signal: controller.signal })
- ).rejects.toThrow('late failure')
+ ).rejects.toMatchObject({ name: 'AbortError' })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('removes the transient message and restores prior state when cancellation wins', async () => { | |
| const controller = new AbortController() | |
| compactionService.applyCompaction.mockImplementation(async () => { | |
| controller.abort() | |
| throw new Error('late failure') | |
| }) | |
| await expect( | |
| service.applyCompactionIntent('s1', intent(), { signal: controller.signal }) | |
| ).rejects.toThrow('late failure') | |
| it('removes the transient message and restores prior state when cancellation wins', async () => { | |
| const controller = new AbortController() | |
| compactionService.applyCompaction.mockImplementation(async () => { | |
| controller.abort() | |
| throw new Error('late failure') | |
| }) | |
| await expect( | |
| service.applyCompactionIntent('s1', intent(), { signal: controller.signal }) | |
| ).rejects.toMatchObject({ name: 'AbortError' }) |
🤖 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 `@test/main/presenter/agentRuntimePresenter/memoryCompactionService.test.ts`
around lines 205 - 214, Update the test for service.applyCompactionIntent so it
asserts the standardized cancellation outcome when the signal is aborted before
the provider throws, rather than expecting the late “late failure” error.
Preserve the existing setup and verify that the transient message is removed and
prior state is restored as indicated by the test name.
Comment on lines
+96
to
+104
| it('ensures only the parent before recording a subagent discard', async () => { | ||
| await service.discardSubagentTape('parent', 'child', { reason: 'abandoned' }) | ||
|
|
||
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledTimes(1) | ||
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledWith('parent', messageStore) | ||
| expect(tapeService.recordExternalForkDiscard).toHaveBeenCalledWith('parent', 'child', 'child', { | ||
| reason: 'abandoned' | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert readiness occurs before recording the discard.
The test title promises ordering, but only verifies that both calls occurred.
Proposed test fix
expect(tapeService.recordExternalForkDiscard).toHaveBeenCalledWith('parent', 'child', 'child', {
reason: 'abandoned'
})
+ expectCalledBefore(
+ tapeService.ensureSessionTapeReady,
+ tapeService.recordExternalForkDiscard
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('ensures only the parent before recording a subagent discard', async () => { | |
| await service.discardSubagentTape('parent', 'child', { reason: 'abandoned' }) | |
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledTimes(1) | |
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledWith('parent', messageStore) | |
| expect(tapeService.recordExternalForkDiscard).toHaveBeenCalledWith('parent', 'child', 'child', { | |
| reason: 'abandoned' | |
| }) | |
| }) | |
| it('ensures only the parent before recording a subagent discard', async () => { | |
| await service.discardSubagentTape('parent', 'child', { reason: 'abandoned' }) | |
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledTimes(1) | |
| expect(tapeService.ensureSessionTapeReady).toHaveBeenCalledWith('parent', messageStore) | |
| expect(tapeService.recordExternalForkDiscard).toHaveBeenCalledWith('parent', 'child', 'child', { | |
| reason: 'abandoned' | |
| }) | |
| expectCalledBefore( | |
| tapeService.ensureSessionTapeReady, | |
| tapeService.recordExternalForkDiscard | |
| ) | |
| }) |
🤖 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 `@test/main/presenter/agentRuntimePresenter/tapeAccessService.test.ts` around
lines 96 - 104, Update the test for discardSubagentTape to assert call order,
not just invocation counts: verify tapeService.ensureSessionTapeReady completes
before tapeService.recordExternalForkDiscard is called. Keep the existing
argument assertions and ensure the test still covers readiness for the parent
session only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/presenter/agentRuntimePresenter/process.ts`:
- Around line 583-585: Update the NoProgressToolLoopGuard construction to pass
state.metadata.noProgressToolLoop directly, matching the constructor’s expected
AgentNoProgressToolLoopMetadata type; remove the unnecessary initialSnapshot
wrapper.
🪄 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: e39ffe25-4502-4df3-a72b-4def1eb51fd6
📒 Files selected for processing (7)
package.jsonscripts/agent-cleanup-guard.mjssrc/main/presenter/agentRuntimePresenter/accumulator.tssrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/agentRuntimePresenter/tapeService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/presenter/agentRuntimePresenter/tapeService.ts
- package.json
- src/main/presenter/agentRuntimePresenter/messageStore.ts
zerob13
requested changes
Jul 13, 2026
zerob13
left a comment
zerob13
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes for runtime correctness and a red repository test suite.
Validation at 9d90fb7b:
pnpm run typecheck:node: passedpnpm run test:agent:eval: passedpnpm exec vitest --run --config vitest.config.ts test/main: failed (10 tests in 3 files; 3,837 passed)
The green PR workflow currently runs the native-agent eval but not test/main, so it does not expose these failures. In addition to the inline lifecycle failure and swapped ACP assertions, six interaction tests fail because the new deepchatMessagesTable.updateMetadata call was not added to the presenter test fixture, and the deferred-subagent cancellation test still expects the old resolved-error contract.
| activeSkillNames: controls?.getActiveSkillNames?.(), | ||
| enabledSkillNames: controls?.getEnabledSkillNames?.() | ||
| }) | ||
| io.abortSignal.throwIfAborted() |
Collaborator
There was a problem hiding this comment.
[P1] Persist a returned tool result before honoring late cancellation
A tool can finish its side effect and resolve while also aborting the parent signal. This post-await check discards the returned result, so executeTools never applies it and afterRoundPersisted never writes its tool_call / tool_result Tape facts. The new persists the completed tool batch before a post-tool abort test reproduces this and currently fails. Stage and commit a successfully returned result first, then let the outer batch settle the run as aborted.
| return | ||
| } | ||
| deferredToolCallCounted = true | ||
| resumeAccounting = incrementToolCallAccounting(resumeAccounting) |
Collaborator
There was a problem hiding this comment.
[P1] Enforce the global cap before running a deferred call
At a post-call permission pause, the probe is already persisted as the 128th call (process.test.ts explicitly covers 127 → 128). Granting that interaction reaches this increment and executeDeferredToolCall without any cap check, so the side effect executes and metadata becomes 129. The loop guard only rejects a later provider-emitted batch and cannot protect this path. Reject or reserve the deferred execution before invoking it, and never persist a count above 128.
| error && typeof error === 'object' && 'name' in error | ||
| ? (error as { name?: unknown }).name | ||
| : undefined | ||
| return signal.aborted || errorName === 'AbortError' || errorName === 'CanceledError' |
Collaborator
There was a problem hiding this comment.
[P1] Do not infer parent cancellation from an error name
AbortError is also used for model request timeouts (BaseLLMProvider.createModelRequestTimeoutError), and agent tools can invoke those provider paths. With a live io.abortSignal, this helper still classifies that tool-local timeout as parent cancellation; the error escapes the tool batch and processStream persists runOutcome: aborted / runStopReason: user_stop. Treat it as parent cancellation only when the run signal is aborted (or the thrown value is its exact reason); otherwise preserve it as a tool/provider error.
|
|
||
| expect(settlement).toEqual({ status: 'completed', stopReason: 'complete' }) | ||
| expect(message?.status).toBe('sent') | ||
| expect(settlement).toEqual({ |
Collaborator
There was a problem hiding this comment.
[P1] Restore the ACP success/failure expectations
The settlement/status expectations for this complete() case and the adapter.fail() case below were swapped. complete() returns completed/sent, while fail() returns error/error; the surrounding event and Tape assertions already reflect those outcomes. The full main suite currently fails both tests. Restore the original success assertions here and the error assertions in the failure case.
This was referenced Jul 14, 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
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
AgentRuntimePresenterfrom a multi-thousand-line monolith into a thin facade plus focused services (session lifecycle, pending input, generation control, stream lifecycle, interaction resume, memory/compaction, turn preparation, etc.), preserving the public IPC/presenter contract.runOutcome/stopReason, no-progress repeated-tool-loop guard, MCP tool call cancellation viaAbortSignal, and subagent run timeouts/concurrency/handoff guardrails.pnpm run test:agent:eval) and wire it into PR CI.Context / docs
docs/architecture/agent-runtime-presenter-split/,docs/architecture/native-agent-evaluation/,docs/architecture/subagent-run-guardrails/docs/issues/Test plan
pnpm run test:agent:evalagentRuntimePresenter/*,mcpClient,toolManager,subagentOrchestratorTool)pnpm run lint/ architecture cleanup guard (lint:agent-cleanup) if available in local workflowSummary by CodeRabbit
New Features
Documentation
Tests