Conversation
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 (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a provider-call preflight for token budgeting with a 256-token safety margin and context-pressure recovery (compaction/trim) when output capacity would drop below 4000 tokens, and centralizes POSIX shell fallback plus spawn-cwd validation with enriched spawn error messages and tests. ChangesAgent Tool Context Budget - Second Increment
Background Exec Shell Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test/main/presenter/agentRuntimePresenter/toolOutputGuard.test.ts (1)
4-27: ⚡ Quick winThe
tokenxmock is not exercised by this test.
hasContextBudget()only callsapproximateTokenSize()fortoolDefinitions, but this case passestoolDefinitions: []. The 3744/3745 boundary is therefore still tied toestimateMessagesTokens(), so the test can drift when the message estimator changes.Either add a non-empty
toolDefinitionsfixture so the mock participates, or remove the mock and derive the boundary from the same message-token estimator the production code uses.🤖 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/toolOutputGuard.test.ts` around lines 4 - 27, The test's tokenx mock never runs because hasContextBudget() only calls approximateTokenSize() for toolDefinitions and the test passes toolDefinitions: []; update the test (in toolOutputGuard.test.ts) to include a small non-empty toolDefinitions fixture (e.g., one ToolDefinition object with a predictable description or prompt text) so that approximateTokenSize (the vi.mock for 'tokenx') is exercised, and adjust the message lengths (the 'x'.repeat(...) values) as needed to preserve the same true boundary when the mocked approximateTokenSize is applied; ensure references to ToolOutputGuard and hasContextBudget remain unchanged.
🤖 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/lib/agentRuntime/shellEnvHelper.ts`:
- Around line 266-292: The current isAvailableShell/resolveShellPath logic only
checks existence via fs.existsSync which can return true for non-executable
files; update isAvailableShell to verify executability as well by using
fs.statSync and fs.accessSync(shellPath, fs.constants.X_OK) (or equivalent
permission checks) and return false when access or stat fails; keep
resolveShellPath behavior but call the new isAvailableShell so non-executable
candidates are skipped and the fallback search continues.
In `@src/main/presenter/agentRuntimePresenter/contextBudget.ts`:
- Around line 163-171: The code reports a positive effectiveMaxTokens even when
fitsWithinContext is false (remainingOutputTokens <= 0); change the logic so
effectiveMaxTokens is zero whenever the request does not fit. Concretely, update
the computation of effectiveMaxTokens (alongside fitsWithinContext) so that when
hasFiniteContext and remainingOutputTokens <= 0 you set effectiveMaxTokens = 0;
otherwise keep the existing Math.max(Math.min(requestedMaxTokens,
remainingOutputTokens), AGENT_MIN_EFFECTIVE_OUTPUT_TOKENS) behavior, and ensure
totalRequestTokens uses that adjusted effectiveMaxTokens. This ensures
effectiveMaxTokens is never positive when fitsWithinContext is false.
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 1877-1901: The recovery path currently only patches the leading
system prompt via replaceLeadingSystemPromptInPlace, leaving requestMessages
(the mutable array later reused) unchanged when recoverContextPressure returns a
trimmed/compacted list; update requestMessages in-place whenever
recovered.messages exists (replace its contents with recovered.messages) so
subsequent loops and provider.coreStream see the persisted, trimmed message
list, and ensure requestPreflight (from preflightRequestContext) remains
consistent with that mutated requestMessages; perform this update alongside the
existing recovered.systemPrompt handling after recoverContextPressure returns.
In `@src/main/presenter/llmProviderPresenter/aiSdk/messageMapper.ts`:
- Around line 20-35: The sanitizeProviderOptions function currently only checks
typeof === 'object' and !Array.isArray, letting Date/URL/class instances
through; update the filter in sanitizeProviderOptions to only accept plain
object payloads by using a robust "is plain object" check (e.g., ensure
Object.prototype.toString.call(value) === '[object Object]' or
Object.getPrototypeOf(value) === Object.prototype) in the entry type guard for
the Object.entries filter so exotic objects (Date, URL, class instances) are
rejected and the function returns undefined for non-plain values.
In `@test/main/lib/agentRuntime/backgroundExecSessionManager.test.ts`:
- Line 55: The test currently calls
vi.mocked(fs.existsSync).mockReturnValue(...), but vi.mocked is only a TS type
helper and not a mock creator; replace those calls with spies: use vi.spyOn(fs,
'existsSync').mockReturnValue(...) for the occurrences around where existsSync
is used (refer to the test file and the symbol fs.existsSync), and apply the
same change to the other two occurrences flagged in the comment (the calls at
the other test locations). Ensure you create the spy before invoking the mock
behavior and restore the spy after the test if necessary.
In `@test/main/lib/agentRuntime/shellEnvHelper.test.ts`:
- Line 68: Tests call vi.mocked(fs.existsSync) but the 'fs' module is never
mocked or spied on, causing runtime failures; fix by either adding a
module-level mock with vi.mock('fs') (consistent with other mocks like
'child_process' and 'electron') or by creating spies in the test setup (e.g., in
beforeEach call vi.spyOn(fs, 'existsSync') and then use .mockReturnValue(...) /
.mockImplementation(...) for each test), and update all usages of
vi.mocked(fs.existsSync) (including the other occurrences) to operate on the
mocked/spied function.
In
`@test/main/presenter/toolPresenter/agentTools/agentBashHandlerEncoding.test.ts`:
- Line 68: The test currently uses vi.mocked(fs.existsSync) which doesn't create
runtime stubs and leaves resolveUsableSpawnCwd() calling the real filesystem;
replace that with runtime spies/stubs for both fs.existsSync and fs.statSync
(the functions used by resolveUsableSpawnCwd in spawnGuard.ts) — e.g., spy on
fs.existsSync to return true and spy on fs.statSync to return a Stats-like
object whose isDirectory() returns true, and ensure you restore the spies after
the test so other tests are unaffected.
---
Nitpick comments:
In `@test/main/presenter/agentRuntimePresenter/toolOutputGuard.test.ts`:
- Around line 4-27: The test's tokenx mock never runs because hasContextBudget()
only calls approximateTokenSize() for toolDefinitions and the test passes
toolDefinitions: []; update the test (in toolOutputGuard.test.ts) to include a
small non-empty toolDefinitions fixture (e.g., one ToolDefinition object with a
predictable description or prompt text) so that approximateTokenSize (the
vi.mock for 'tokenx') is exercised, and adjust the message lengths (the
'x'.repeat(...) values) as needed to preserve the same true boundary when the
mocked approximateTokenSize is applied; ensure references to ToolOutputGuard and
hasContextBudget remain unchanged.
🪄 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: 15826431-0492-44b8-a7f8-c2aa409fe82f
📒 Files selected for processing (22)
docs/issues/agent-tool-context-budget/plan.mddocs/issues/agent-tool-context-budget/spec.mddocs/issues/agent-tool-context-budget/tasks.mddocs/issues/background-exec-shell-fallback/plan.mddocs/issues/background-exec-shell-fallback/spec.mddocs/issues/background-exec-shell-fallback/tasks.mdsrc/main/lib/agentRuntime/backgroundExecSessionManager.tssrc/main/lib/agentRuntime/shellEnvHelper.tssrc/main/lib/agentRuntime/spawnGuard.tssrc/main/presenter/agentRuntimePresenter/compactionService.tssrc/main/presenter/agentRuntimePresenter/contextBudget.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/toolOutputGuard.tssrc/main/presenter/llmProviderPresenter/aiSdk/messageMapper.tssrc/main/presenter/toolPresenter/agentTools/agentBashHandler.tstest/main/lib/agentRuntime/backgroundExecSessionManager.test.tstest/main/lib/agentRuntime/shellEnvHelper.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/contextBudget.test.tstest/main/presenter/agentRuntimePresenter/toolOutputGuard.test.tstest/main/presenter/llmProviderPresenter/aiSdkMessageMapper.test.tstest/main/presenter/toolPresenter/agentTools/agentBashHandlerEncoding.test.ts
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/lib/agentRuntime/shellEnvHelper.ts`:
- Around line 318-323: The current selection for variable shell may return
fallbackShells[0] even if resolveShellPath rejected it; update the logic in
shellEnvHelper (around getFallbackShellCandidates and resolveShellPath usage
that computes shell) so that you only accept a candidate if
resolveShellPath(candidate) returns a truthy/valid path, otherwise fall back to
a guaranteed-safe default like '/bin/sh'; in practice remove the unconditional
fallbackShells[0] branch and change the expression to use the first resolved
candidate from fallbackShells (from fallbackShells.map(...).find(Boolean)) or
'/bin/sh' so spawn() won’t receive an unchecked/previously-rejected path.
🪄 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: f25af7a0-5a7e-4554-9eb4-6fbf0723d7f6
📒 Files selected for processing (18)
docs/issues/agent-tool-context-budget/plan.mddocs/issues/agent-tool-context-budget/spec.mddocs/issues/agent-tool-context-budget/tasks.mddocs/issues/background-exec-shell-fallback/plan.mddocs/issues/background-exec-shell-fallback/spec.mddocs/issues/background-exec-shell-fallback/tasks.mdsrc/main/lib/agentRuntime/shellEnvHelper.tssrc/main/presenter/agentRuntimePresenter/contextBudget.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/llmProviderPresenter/aiSdk/messageMapper.tstest/main/lib/agentRuntime/backgroundExecSessionManager.test.tstest/main/lib/agentRuntime/shellEnvHelper.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/contextBudget.test.tstest/main/presenter/agentRuntimePresenter/toolOutputGuard.test.tstest/main/presenter/llmProviderPresenter/aiSdkMessageMapper.test.tstest/main/presenter/toolPresenter/agentTools/agentBashHandlerEncoding.test.tstest/setup.ts
✅ Files skipped from review due to trivial changes (4)
- docs/issues/background-exec-shell-fallback/tasks.md
- docs/issues/agent-tool-context-budget/spec.md
- docs/issues/background-exec-shell-fallback/plan.md
- docs/issues/agent-tool-context-budget/tasks.md
🚧 Files skipped from review as they are similar to previous changes (10)
- test/main/presenter/agentRuntimePresenter/toolOutputGuard.test.ts
- test/main/presenter/agentRuntimePresenter/contextBudget.test.ts
- test/main/presenter/toolPresenter/agentTools/agentBashHandlerEncoding.test.ts
- src/main/presenter/agentRuntimePresenter/contextBudget.ts
- src/main/presenter/llmProviderPresenter/aiSdk/messageMapper.ts
- docs/issues/agent-tool-context-budget/plan.md
- test/main/lib/agentRuntime/backgroundExecSessionManager.test.ts
- test/main/presenter/llmProviderPresenter/aiSdkMessageMapper.test.ts
- src/main/presenter/agentRuntimePresenter/index.ts
- test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts
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
Documentation
Tests