feat(agent): configure output limits - #2103
Conversation
📝 WalkthroughWalkthroughThis change adds configurable per-Agent limits for file-read truncation, inline tool output, and command or skill output. The limits are validated, persisted through Agent settings, applied across runtime execution and context fitting, and covered by focused tests. ChangesConfigurable Agent output limits
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentSettings
participant AgentToolManager
participant AgentBashHandler
participant ToolOutputGuard
participant ConversationContext
AgentSettings->>AgentToolManager: persist per-Agent output limits
AgentToolManager->>AgentBashHandler: pass command preview limit
AgentToolManager->>ToolOutputGuard: provide session output limits
AgentBashHandler-->>AgentToolManager: return output and offload path
ToolOutputGuard->>ConversationContext: fit oversized tool output
ConversationContext-->>ToolOutputGuard: context budget result
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/agent/shared/process/backgroundExecSessionManager.ts (1)
172-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the configured preview limit when polling background sessions.
BackgroundExecSessionManager.poll()uses globalconfig.maxOutputChars. These callers only passoffloadThresholdChars. A backgroundexecorskill_runsession can therefore return more or less output than the Agent setting specifies.
- src/main/agent/shared/process/backgroundExecSessionManager.ts#L172-L177: add a preview-length contract to
start()orpoll(), then apply it when building poll output.- src/main/skill/skillExecutionService.ts#L101-L111: forward
outputPreviewCharsseparately from the capped offload threshold.- src/main/tool/agentTools/agentBashHandler.ts#L620-L627: forward
outputPreviewCharsseparately from the capped offload threshold.Update
AgentToolManager.callProcessTool()to resolve and pass the conversation limit forpoll. Add a focused Vitest regression test for a background session with a non-default limit. As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 172 - 177, Preserve the Agent-configured preview limit throughout background session polling. In src/main/agent/shared/process/backgroundExecSessionManager.ts:172-177, add a preview-length contract to start or poll and apply it when constructing poll output; in src/main/skill/skillExecutionService.ts:101-111 and src/main/tool/agentTools/agentBashHandler.ts:620-627, forward outputPreviewChars separately from the capped offloadThresholdChars. Update AgentToolManager.callProcessTool() to resolve and pass the conversation limit to poll, and add a focused Vitest regression test covering a background session with a non-default limit.Source: Coding guidelines
🧹 Nitpick comments (4)
src/main/agent/deepchat/runtime/toolOutputGuard.ts (3)
147-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the offload marker into a constant.
The literal
'[Tool output offloaded]'now appears in four places: the guard at Line 152, the batch guard at Line 301, and the two stub builders at Line 544 and Line 555. A change to the stub header in one builder would silently break the "already offloaded" detection. Define one constant and use it in all four places.♻️ Proposed refactor
const TOOL_OUTPUT_PREVIEW_LENGTH = 1024 +const TOOL_OUTPUT_OFFLOAD_MARKER = '[Tool output offloaded]'if (!CONTEXT_FALLBACK_OFFLOAD_TOOLS.has(params.toolName)) return null if (!params.rawContent) return null - if (params.rawContent.startsWith('[Tool output offloaded]')) return null + if (params.rawContent.startsWith(TOOL_OUTPUT_OFFLOAD_MARKER)) return null🤖 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/agent/deepchat/runtime/toolOutputGuard.ts` around lines 147 - 164, Define a shared constant for the “[Tool output offloaded]” marker and replace the duplicated literal in prepareContextFallback, the batch guard, and both stub builders. Ensure all offload detection and generated stub headers use this single constant.
166-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared fallback-attempt block.
fitExistingToolOutputat Lines 180-199 andguardToolOutputat Lines 235-254 run the same sequence: prepare the context fallback, rebuild the messages, re-check the budget, and clean up the temporary offload file on rejection. Only theToolMessageUpdateModediffers. Extract one private helper that takes the mode and returns the accepted fallback ornull. This keeps the cleanup rule in one place.♻️ Proposed helper
private async tryContextFallback( params: ContextFallbackParams & ContextBudgetParams & { toolCallId: string }, mode: ToolMessageUpdateMode ): Promise<PreparedToolOutput | null> { const fallback = await this.prepareContextFallback(params) if (fallback?.kind !== 'ok') return null const fallbackMessages = this.withToolMessage( params.conversationMessages, params.toolCallId, fallback.content, mode ) if ( this.hasContextBudget({ conversationMessages: fallbackMessages, toolDefinitions: params.toolDefinitions, contextLength: params.contextLength, maxTokens: params.maxTokens }) ) { return fallback } await this.cleanupOffloadedOutput(fallback.offloadPath) return null }Also applies to: 234-255
🤖 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/agent/deepchat/runtime/toolOutputGuard.ts` around lines 166 - 206, Extract the duplicated fallback preparation, message rebuilding, budget validation, and rejected-offload cleanup from fitExistingToolOutput and guardToolOutput into a private tryContextFallback helper. Have it accept the relevant fallback/context parameters plus toolCallId and ToolMessageUpdateMode, return the accepted PreparedToolOutput or null, and preserve each caller’s existing mode and subsequent error handling.
296-338: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the cost of repeated budget preflights.
Each iteration calls
hasContextBudget, which runspreflightRequestContextover the whole message list and all tool definitions. This new loop adds up to N passes for a batch of N results, on top of the existing downgrade loop below that adds up to N × 4 more. The early return limits this in the common case. If large tool batches on long conversations become slow, cache the token estimate for the unchanged prefix ofconversationMessagesand re-estimate only the tool messages.🤖 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/agent/deepchat/runtime/toolOutputGuard.ts` around lines 296 - 338, Optimize the repeated budget checks in the fittedResults loop by caching the token estimate for the unchanged conversation prefix and tool definitions, then re-estimating only the modified tool messages before each hasContextBudget decision. Update or invalidate the cached estimate whenever fittedResults changes, preserving the existing budget outcome and early return behavior.src/main/agent/deepchat/runtime/turnResumeContract.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the ownership difference between the two offload paths.
offloadPathnames a file thatToolOutputGuardcreated and may delete during cleanup.existingOffloadPathnames a file that the tool created, and the guard must never delete it. TheToolOutputGuard.prepareContextFallbackreuse branch relies on this distinction: it deliberately returnsoffloadPath: undefinedso thatcleanupOffloadedOutputbecomes a no-op. The field names alone do not convey the rule. Add a short comment so a future caller does not pass the tool-owned path to cleanup.♻️ Proposed comment
export type ResumeBudgetToolCall = { id: string name: string responseText?: string + /** File written by ToolOutputGuard. Safe to delete during cleanup. */ offloadPath?: string + /** File written by the tool itself. Never delete it during cleanup. */ existingOffloadPath?: string }🤖 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/agent/deepchat/runtime/turnResumeContract.ts` around lines 6 - 8, Add a concise ownership comment beside offloadPath and existingOffloadPath in the turn-resume contract, stating that offloadPath is guard-created and may be deleted during cleanup, while existingOffloadPath is tool-created and must never be deleted. Preserve the prepareContextFallback reuse behavior where it leaves offloadPath undefined.
🤖 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/features/configurable-agent-output-limits/plan.md`:
- Around line 90-96: Replace the placeholder <10 focused main test files> in the
documented Vitest command with the exact focused test file paths, so the command
is directly runnable; alternatively, explicitly label the block as pseudocode if
concrete paths cannot be provided.
In `@src/main/agent/deepchat/runtime/turnCoordinator.ts`:
- Around line 1327-1335: In the resumeBudget `kind === 'ok'` branch, call the
current run scope’s `assertCurrent()` immediately after the awaited
`fitExistingToolOutput` completes and before `updateToolCallResponse`,
`messageStore.updateAssistantContent`, or `messageProjection.refresh`. Match the
guard placement used by the sibling `tool_error` and `terminal_error` branches
so superseded or aborted runs cannot persist the fitted response.
- Line 1319: Update the resume flow around fitResumeBudgetForToolCall to pass
the derived contextBudgetLength from resolveDeepChatContextBudgetLength instead
of generationSettings.contextLength. Keep the fit decision aligned with the
context window used to build resumeContext, including the
Number.MAX_SAFE_INTEGER bypass for AC/non-chat models.
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 335-344: Cap decoded offloaded output previews to
outputPreviewChars in backgroundExecSessionManager’s persisted-output completion
path, skillExecutionService before formatting the decoded tail, and
agentBashHandler before returning the completed result; update the listed sites
in src/main/agent/shared/process/backgroundExecSessionManager.ts#L335-L344,
src/main/skill/skillExecutionService.ts#L519-L521, and
src/main/tool/agentTools/agentBashHandler.ts#L487-L492. Add a focused Vitest
regression test using a single line longer than four times the configured limit
and verify the user-visible preview never exceeds that limit.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 2772-2781: Translate every output-limit entry, including titles,
descriptions, hints, and labels, in src/renderer/src/i18n/da-DK/settings.json
lines 2772-2781 into Danish; src/renderer/src/i18n/de-DE/settings.json lines
229-238 into German; src/renderer/src/i18n/es-ES/settings.json lines 229-238
into Spanish; src/renderer/src/i18n/fa-IR/settings.json lines 2772-2781 into
Persian; src/renderer/src/i18n/fr-FR/settings.json lines 2772-2781 into French;
and src/renderer/src/i18n/he-IL/settings.json lines 2772-2781 into Hebrew, while
preserving all existing translation keys and JSON structure.
In `@src/renderer/src/i18n/id-ID/settings.json`:
- Around line 229-238: Translate the output-limit settings strings identified by
outputLimitsTitle, outputLimitsDescription, outputLimitsSafetyHint,
outputLimitsReset, outputLimitsReadFile, outputLimitsReadFileHint,
outputLimitsTool, outputLimitsToolHint, outputLimitsCommand, and
outputLimitsCommandHint in src/renderer/src/i18n/id-ID/settings.json (229-238),
it-IT/settings.json (229-238), ja-JP/settings.json (2772-2781),
ko-KR/settings.json (2772-2781), ms-MY/settings.json (229-238),
pl-PL/settings.json (229-238), pt-BR/settings.json (2772-2781), and
ru-RU/settings.json (2772-2781); preserve the existing keys and JSON structure
while replacing the English values with accurate locale-specific translations.
In `@test/renderer/components/DeepChatAgentsSettings.test.ts`:
- Around line 316-326: Update the assertions in the DeepChatAgentsSettings test
to read each input’s live element.value property instead of
getAttribute('value'), both before and after triggering the reset via
agent-output-limits-reset; preserve the existing expected values.
---
Outside diff comments:
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 172-177: Preserve the Agent-configured preview limit throughout
background session polling. In
src/main/agent/shared/process/backgroundExecSessionManager.ts:172-177, add a
preview-length contract to start or poll and apply it when constructing poll
output; in src/main/skill/skillExecutionService.ts:101-111 and
src/main/tool/agentTools/agentBashHandler.ts:620-627, forward outputPreviewChars
separately from the capped offloadThresholdChars. Update
AgentToolManager.callProcessTool() to resolve and pass the conversation limit to
poll, and add a focused Vitest regression test covering a background session
with a non-default limit.
---
Nitpick comments:
In `@src/main/agent/deepchat/runtime/toolOutputGuard.ts`:
- Around line 147-164: Define a shared constant for the “[Tool output
offloaded]” marker and replace the duplicated literal in prepareContextFallback,
the batch guard, and both stub builders. Ensure all offload detection and
generated stub headers use this single constant.
- Around line 166-206: Extract the duplicated fallback preparation, message
rebuilding, budget validation, and rejected-offload cleanup from
fitExistingToolOutput and guardToolOutput into a private tryContextFallback
helper. Have it accept the relevant fallback/context parameters plus toolCallId
and ToolMessageUpdateMode, return the accepted PreparedToolOutput or null, and
preserve each caller’s existing mode and subsequent error handling.
- Around line 296-338: Optimize the repeated budget checks in the fittedResults
loop by caching the token estimate for the unchanged conversation prefix and
tool definitions, then re-estimating only the modified tool messages before each
hasContextBudget decision. Update or invalidate the cached estimate whenever
fittedResults changes, preserving the existing budget outcome and early return
behavior.
In `@src/main/agent/deepchat/runtime/turnResumeContract.ts`:
- Around line 6-8: Add a concise ownership comment beside offloadPath and
existingOffloadPath in the turn-resume contract, stating that offloadPath is
guard-created and may be deleted during cleanup, while existingOffloadPath is
tool-created and must never be deleted. Preserve the prepareContextFallback
reuse behavior where it leaves offloadPath undefined.
🪄 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: f5631707-0b2f-463f-bdb3-bf76e073291f
📒 Files selected for processing (51)
docs/features/configurable-agent-output-limits/plan.mddocs/features/configurable-agent-output-limits/spec.mddocs/features/configurable-agent-output-limits/tasks.mdsrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/toolOutputGuard.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/turnResumeContract.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/skill/skillExecutionService.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentFileSystemHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/renderer/settings/components/DeepChatAgentsSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/domainSchemas.tssrc/shared/lib/agentOutputLimits.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/mcp.tssrc/shared/types/mcp.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/deepchat/runtime/toolOutputGuard.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/shared/agentOutputLimits.test.tstest/main/skill/skillExecutionService.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/agentTools/agentToolManagerRead.test.tstest/renderer/components/DeepChatAgentsSettings.test.ts
yyhhyyyyyy
left a comment
There was a problem hiding this comment.
I would mark this as Request changes. I found one end-to-end blocker and two correctness issues in the limit-enforcement path.
-
Blocking: the persisted values are discarded by the runtime resolver.
Both
AgentToolManagerandToolOutputGuardconsumeresolveDeepChatAgentConfig(). That path eventually reachesDeepChatAgentRepository.resolveConfig(), wheremergeDeepChatConfig()reconstructs the config from an explicit field list. The three new fields are missing from that list, soresolveAgentOutputLimits()always receivesundefinedand falls back to the defaults.This also explains why the current tests stay green: the renderer test verifies persistence, while the runtime tests mock
resolveDeepChatAgentConfig()and therefore bypass the real repository path. Please preserve the three fields inmergeDeepChatConfig()and add a repository-level regression that writes non-default values and resolves them through the production config path. -
The persisted-output readers do not enforce the configured character cap.
The
readLastCharsFromFile()implementations inbackgroundExecSessionManager.ts,skillExecutionService.ts, andagentBashHandler.tsread up tomaxChars * 4bytes for UTF-8, but return the decoded text without a final character cap. A long single-line ASCII output therefore returns almost four times the configured limit. Sinceexecis no longer covered by the generic 5,000-character guard, this can now flow inline despite the command limit.Please cap the decoded tail after partial-line handling and add a regression using a single line longer than four times the configured limit.
-
The resume path can persist stale output after an awaited fallback.
fitExistingToolOutput()may perform file I/O, but thekind === 'ok'branch updates the message store before re-checking the abort signal or current run scope. A cancellation or superseding run during that await can therefore mutate the transcript from a stale run. Please check both the signal and scope before the first mutation, and clean up any newly created guard-owned offload file if the result is abandoned.
Non-blocking follow-ups:
- The new strings remain English in 14 non-English locale files.
- The deletion ownership difference between
offloadPathandexistingOffloadPathis safety-critical but currently encoded only by naming convention. - The context-pressure loop adds repeated full-context preflights; this is worth profiling for large contexts and large parallel tool batches.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/tool/agentTools/agentBashHandler.ts (1)
457-477: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrain the offload write queue before reading the preview.
When
offloadedis true, both callbacks callreadLastCharsFromFile()beforesettle()waits foroutputWriteQueue. The final append can still be pending. The returned preview can omit the final output or be empty even though the file is completed later.Await
outputWriteQueueafter flushing the decoders and before reading the file in both paths. Add a focused regression test for an offloaded command.As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
Also applies to: 487-501
🤖 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/tool/agentTools/agentBashHandler.ts` around lines 457 - 477, Drain outputWriteQueue after outputDecoders.flush() and before readLastCharsFromFile() in both timeout and normal completion paths, ensuring offloaded previews include the final queued append. Add a focused regression test covering the returned preview for an offloaded command.Source: Coding guidelines
src/main/agent/shared/process/backgroundExecSessionManager.ts (1)
78-78: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPropagate the per-Agent preview limit through background sessions.
The background-session contract carries only the disk-spooling threshold. It does not carry the inline preview limit, so polling falls back to the global limit.
src/main/agent/shared/process/backgroundExecSessionManager.ts#L78-L78: AddpreviewCharstoBackgroundSession, initialize it from the normalized option, and use it inpoll().src/main/tool/agentTools/agentBashHandler.ts#L620-L627: Passoptions.outputPreviewCharsseparately from the cappedoffloadThresholdChars.As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
🤖 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/agent/shared/process/backgroundExecSessionManager.ts` at line 78, Extend BackgroundSession in src/main/agent/shared/process/backgroundExecSessionManager.ts at lines 78-78 with previewChars, initialize it from the normalized preview option, and make poll() use that per-session value instead of the global limit. In src/main/tool/agentTools/agentBashHandler.ts at lines 620-627, pass options.outputPreviewChars separately from the capped offloadThresholdChars. Add the smallest regression test covering the user-visible per-agent preview limit.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Line 78: Extend BackgroundSession in
src/main/agent/shared/process/backgroundExecSessionManager.ts at lines 78-78
with previewChars, initialize it from the normalized preview option, and make
poll() use that per-session value instead of the global limit. In
src/main/tool/agentTools/agentBashHandler.ts at lines 620-627, pass
options.outputPreviewChars separately from the capped offloadThresholdChars. Add
the smallest regression test covering the user-visible per-agent preview limit.
In `@src/main/tool/agentTools/agentBashHandler.ts`:
- Around line 457-477: Drain outputWriteQueue after outputDecoders.flush() and
before readLastCharsFromFile() in both timeout and normal completion paths,
ensuring offloaded previews include the final queued append. Add a focused
regression test covering the returned preview for an offloaded command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 697fde4b-5d3f-43b3-8572-160753fbea81
📒 Files selected for processing (9)
docs/features/configurable-agent-output-limits/plan.mdsrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/skill/skillExecutionService.tssrc/main/tool/agentTools/agentBashHandler.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/renderer/components/DeepChatAgentsSettings.test.tstest/setup.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/skill/skillExecutionService.ts
- test/main/agent/shared/process/backgroundExecSessionManager.test.ts
- test/renderer/components/DeepChatAgentsSettings.test.ts
- src/main/agent/deepchat/runtime/turnCoordinator.ts
yyhhyyyyyy
left a comment
There was a problem hiding this comment.
I re-reviewed the latest head (7fb0891). The persisted-output cap issue and the live input assertions are fixed, and the new scope check prevents the previously reported stale mutation in the non-offloaded success case. I still see two issues that should be addressed before merging:
-
Blocking: the production config resolver still drops all three settings.
mergeDeepChatConfig()still does not carryreadFileAutoTruncateChars,toolOutputInlineChars, orcommandOutputInlineChars. Every runtime consumer obtains these values throughresolveDeepChatAgentConfig(), so custom values are silently replaced by the defaults. The UI can save and reload the values, which makes the failure especially difficult to notice.Please preserve these fields in
mergeDeepChatConfig()and add a repository-level regression test that persists non-default values and resolves them through the actual production path. The current renderer and runtime tests do not cover this because they either verify raw persistence or mockresolveDeepChatAgentConfig(). -
The resume fallback lifecycle is still only partially protected.
The awaited fallback is not tied to
preStreamAbortSignal, and not all file side effects are guarded by cancellation and scope freshness. In thetool_errorandterminal_errorbranches,budgetToolCall.offloadPathis deleted beforescope.assertCurrent(). If the session instance is replaced during the await, the stale run can therefore delete a file still referenced by the already-persisted tool result.Conversely, if a successful fallback creates a new
resumeBudget.offloadPathand the subsequent scope check fails, that newly created file is abandoned without cleanup. The new test only coverskind: 'ok'withoffloaded: false, so neither ownership case is exercised.Please make the fitting step abort-aware, validate cancellation and scope freshness before cleanup or persistence, clean up newly created guard-owned fallback files when their result is abandoned, and never delete the tool-owned
existingOffloadPath. Focused tests should cover cancellation, stale replacement with an offloaded success result, and stale replacement in both error branches.
Non-blocking: the ten new strings are still left in English in 16 non-English locale files.
Because the first issue means the configurable values still have no runtime effect, I would keep this as Request changes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/tool/agentTools/agentBashHandler.ts (1)
431-468: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve previews after a partial offload failure.
If an early
appendFilesucceeds and a later append fails, the catch block setsoffloadedtofalseand stores only the failed chunk inoutput.settle()then returns that buffer instead of combining it with the already persisted output. It also omitsoutputFilePath.Track whether any output was persisted independently from later write failures. Build the trailing preview from persisted output plus fallback-buffer output. Keep the offload path when persisted output exists. Add a focused Vitest test with a successful first append and a failed later append.
As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
🤖 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/tool/agentTools/agentBashHandler.ts` around lines 431 - 468, Update appendOutput and settle so persisted output remains tracked independently after a later appendFile failure; combine the trailing persisted-file preview with the in-memory fallback buffer, preserve offloaded outputFilePath whenever any write succeeded, and avoid returning only the failed chunk. Add a focused Vitest regression test covering a successful first append followed by a failed later append and asserting the returned preview and outputFilePath.Source: Coding guidelines
test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts (1)
2459-2459: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicated type assertion.
Line 2459 repeats
)?.[1] as { sessionId: string; runId: string }. The second occurrence is a standalone expression continuation. TypeScript cannot parse this test file.Proposed fix
const diagnostic = loggerErrorMock.mock.calls.find( ([message]) => message === '[DeepChatAgent] Execution Journal recovery candidate parked' )?.[1] as { sessionId: string; runId: string } - )?.[1] as { sessionId: string; runId: string }🤖 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/agent/deepchat/harness/deepChatAgentHarness.test.ts` at line 2459, Remove the duplicated standalone `)?.[1] as { sessionId: string; runId: string }` continuation near the affected test expression, retaining only the complete valid access and type assertion so the test file parses correctly.src/main/agent/shared/process/backgroundExecSessionManager.ts (1)
1041-1061: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove completed host sessions out of
activeSessions.When
list()receives a non-running session from the utility host, the matching local entry remains inactiveSessions. If the utility host later exits,handleHostExit()moves that completed session tocrashedSessions. The process tool then reports a completed command as a crash.Update local tracking from each non-running
hostSessionsresult. Add a focused Vitest test that lists a completed session, exits the utility host, and confirms that the session remains completed.As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
🤖 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/agent/shared/process/backgroundExecSessionManager.ts` around lines 1041 - 1061, Update the host-session reconciliation flow around request<SessionMeta[]>('list', [conversationId]) to move each non-running host session from activeSessions into the appropriate completed-session tracking before handling host exit, preventing handleHostExit() from classifying it as crashed. Add a focused Vitest regression test that lists a completed session, exits the utility host, and verifies the session remains completed.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/main/agent/shared/process/backgroundExecSessionManager.ts`:
- Around line 1041-1061: Update the host-session reconciliation flow around
request<SessionMeta[]>('list', [conversationId]) to move each non-running host
session from activeSessions into the appropriate completed-session tracking
before handling host exit, preventing handleHostExit() from classifying it as
crashed. Add a focused Vitest regression test that lists a completed session,
exits the utility host, and verifies the session remains completed.
In `@src/main/tool/agentTools/agentBashHandler.ts`:
- Around line 431-468: Update appendOutput and settle so persisted output
remains tracked independently after a later appendFile failure; combine the
trailing persisted-file preview with the in-memory fallback buffer, preserve
offloaded outputFilePath whenever any write succeeded, and avoid returning only
the failed chunk. Add a focused Vitest regression test covering a successful
first append followed by a failed later append and asserting the returned
preview and outputFilePath.
In `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts`:
- Line 2459: Remove the duplicated standalone `)?.[1] as { sessionId: string;
runId: string }` continuation near the affected test expression, retaining only
the complete valid access and type assertion so the test file parses correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3113e24-867e-4e02-b89f-e959702616e3
📒 Files selected for processing (47)
docs/features/configurable-agent-output-limits/plan.mddocs/features/configurable-agent-output-limits/spec.mddocs/features/configurable-agent-output-limits/tasks.mdsrc/main/agent/deepchat/deepChatAgentRepository.tssrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/toolOutputGuard.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/turnResumeContract.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/skill/skillExecutionService.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentFileSystemHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/core/mcp.tssrc/shared/types/mcp.tstest/main/agent/deepchat/deepChatAgentRepository.test.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/deepchat/runtime/toolOutputGuard.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/skill/skillExecutionService.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/agentTools/agentToolManagerRead.test.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- src/renderer/src/i18n/tr-TR/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/zh-CN/settings.json
- test/main/agent/deepchat/runtime/toolAdapters.test.ts
- src/main/agent/deepchat/runtime/interactionCoordinator.ts
- src/renderer/src/i18n/it-IT/settings.json
- src/renderer/src/i18n/es-ES/settings.json
- src/renderer/src/i18n/de-DE/settings.json
- src/renderer/src/i18n/pl-PL/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- src/main/agent/deepchat/loop/ports.ts
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/ms-MY/settings.json
- docs/features/configurable-agent-output-limits/tasks.md
- src/renderer/src/i18n/zh-TW/settings.json
- src/main/agent/deepchat/harness/createDeepChatAgentHarness.ts
- src/renderer/src/i18n/id-ID/settings.json
- src/main/agent/deepchat/runtime/turnResumeContract.ts
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/shared/types/mcp.ts
- src/renderer/src/i18n/en-US/settings.json
- src/main/skill/skillExecutionService.ts
- src/renderer/src/i18n/vi-VN/settings.json
- src/main/agent/deepchat/runtime/dispatch.ts
- src/main/agent/deepchat/runtime/deferredToolExecutor.ts
- docs/features/configurable-agent-output-limits/spec.md
- test/main/agent/shared/process/backgroundExecSessionManager.test.ts
- src/shared/types/core/mcp.ts
- src/renderer/src/i18n/ko-KR/settings.json
- docs/features/configurable-agent-output-limits/plan.md
Why
Issue #2102 identifies that hardcoded file-read, tool-output, and command-output limits are too conservative for Agents using large-context models. This change exposes the effective user-visible limits per Agent while keeping internal disk-spooling details and the removed legacy web limit out of Settings.
Implementation
DeepChatAgentConfig:readFileAutoTruncateChars(default: 4,500)toolOutputInlineChars(default: 5,000)commandOutputInlineChars(default: 12,000)read.limit.ToolOutputGuard, whileexecandskill_runuse the dedicated command preview path.The configured limits remain upper bounds. The existing full-request context preflight can still offload or reject results that do not fit the model context.
UI
Before
After
Compatibility
webContentLengthLimitare intentionally not exposed.Validation
pnpm formatpnpm i18npnpm lintpnpm typecheckCloses #2102
Summary by CodeRabbit
New Features
Localization
Tests
Documentation