Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a ToolOutputGuard for offloading and context-budgeting of tool outputs, integrates it into tool execution and presenter flows, surfaces terminal errors from oversized tool results, and updates tests to cover offload, budgeting, and terminal-error behaviors. Changes
Sequence DiagramsequenceDiagram
participant Executor as Tool Executor
participant Guard as ToolOutputGuard
participant FS as File System
participant Presenter as Presenter/Stream
participant Provider as LLM Provider
Executor->>Guard: prepareToolOutput(rawOutput, tool, contextLength)
alt output too large / requires offload
Guard->>FS: write offload file (session, tool)
alt write succeeds
Guard-->>Executor: {ok: offload_stub}
else write fails
Guard-->>Executor: {tool_error: "offload failed"}
end
else fits in-memory
Guard-->>Executor: {ok: guarded_content}
end
Executor->>Guard: guardToolOutput(prepResult, contextLength, maxTokens)
alt fits in conversation context
Guard-->>Executor: {ok: tool_message_content}
Executor->>Presenter: inject tool_message_content
Presenter->>Provider: resumeWithToolResult(tool_message_content)
else exceeds context
Guard-->>Executor: {terminal_error: "context overflow"}
Executor->>Presenter: surface terminal_error (finalize as error)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts`:
- Around line 577-589: The current code uses guardedResult.kind === 'tool_error'
to mark tool_call blocks, which loses the original tool failure if callTool()
returned rawData.isError === true but the guard returned ok/offloaded; compute a
preserved error flag (e.g., isToolError = guardedResult.kind === 'tool_error' ||
rawData?.isError === true) after callTool()/guarding and use that flag when
pushing the conversation entry and when calling updateToolCallBlock so the
UI/persisted block reflects the tool's original failure; update references in
this section (guardedResult, rawData, updateToolCallBlock, conversation push) to
use that combined flag.
In `@src/main/presenter/deepchatAgentPresenter/toolOutputGuard.ts`:
- Around line 15-25: The current PreparedToolOutputResult 'ok' variant drops the
offload file handle by only returning {content, offloaded}; change the 'ok'
variant to include the offload resource (e.g., offloadHandle or offloadPath and
a cleanup/close token) so callers can manage the file lifecycle, then propagate
that handle through the code paths that convert results to
tool_error/terminal_error (the locations around the earlier offload checks and
the downgrade in the later block) instead of discarding it; update any consumers
that construct or transform PreparedToolOutputResult so they close/delete the
offload when the final result is an error or when no longer needed.
- Around line 237-245: The buildOffloadStub function currently injects the raw
filesystem path (filePath) into tool output which leaks local paths; change it
to avoid returning the absolute path by using an opaque offload identifier or at
most the basename instead of the full filePath (e.g., accept or compute an
offloadId and/or use path.basename(filePath) inside buildOffloadStub) and ensure
the real filePath remains only in server-side metadata/storage (not in the
string returned by buildOffloadStub).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1093e40e-3ec9-488b-8270-b7429236f0a4
📒 Files selected for processing (8)
src/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/main/presenter/deepchatAgentPresenter/toolOutputGuard.tssrc/main/presenter/deepchatAgentPresenter/types.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.tstest/main/presenter/deepchatAgentPresenter/process.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/deepchatAgentPresenter/index.ts (1)
1848-1919:⚠️ Potential issue | 🟠 MajorClean up offloaded tool artifacts when a session is cleared or destroyed.
This path now persists successful tool outputs under the session directory, but
clearMessages()anddestroySession()still only remove DB/runtime state. Deleting a chat will leave its offloaded tool payloads on disk, which is a storage and privacy leak. Please remove the session offload directory as part of those teardown paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/index.ts` around lines 1848 - 1919, The teardown paths (clearMessages and destroySession) currently only clear DB/runtime state but leave offloaded tool artifacts on disk (created via prepareToolOutput and returned in prepared.offloadPath), causing storage/privacy leaks; update clearMessages() and destroySession() to remove the session's offload directory by resolving the session project dir (use resolveProjectDir(sessionId)) and deleting the offload folder (or the specific prepared.offloadPath children) asynchronously, handling and logging errors without throwing (so teardown continues), and ensure any helper cleanup on toolOutputGuard (if available) is invoked to avoid orphaned files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/toolOutputGuard.ts`:
- Around line 84-107: The current offload filename (from
resolveToolOffloadPath(sessionId, toolCallId)) can be overwritten when the same
toolCallId is reused; change the offload flow to produce a unique filename per
execution (e.g., append a monotonic timestamp or UUID) before creating/writing
the file and return that unique path in offloadPath and buildOffloadStub.
Concretely, generate a uniqueSuffix (timestamp or uuid) when preparing filePath,
either by extending resolveToolOffloadPath to accept a suffix or by computing
filePath = resolveToolOffloadPath(sessionId, toolCallId) + `.${uniqueSuffix}`;
then use that filePath for mkdir/writeFile and in the returned object and
buildOffloadStub so prior offloads are never overwritten.
---
Outside diff comments:
In `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 1848-1919: The teardown paths (clearMessages and destroySession)
currently only clear DB/runtime state but leave offloaded tool artifacts on disk
(created via prepareToolOutput and returned in prepared.offloadPath), causing
storage/privacy leaks; update clearMessages() and destroySession() to remove the
session's offload directory by resolving the session project dir (use
resolveProjectDir(sessionId)) and deleting the offload folder (or the specific
prepared.offloadPath children) asynchronously, handling and logging errors
without throwing (so teardown continues), and ensure any helper cleanup on
toolOutputGuard (if available) is invoked to avoid orphaned files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78d55bb1-10ea-4564-aec4-0ee6f77c6cba
📒 Files selected for processing (6)
src/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/toolOutputGuard.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.tstest/main/presenter/deepchatAgentPresenter/process.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/deepchatAgentPresenter/process.test.ts
This was referenced Mar 8, 2026
Merged
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests