Enforce vitest strict equality in tests (Fixes #1569)#1901
Open
Enforce vitest strict equality in tests (Fixes #1569)#1901
Conversation
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
Summary by CodeRabbit
WalkthroughThis PR refactors a2a-server agent task handling by extracting helper utilities into a new module, modernizes test assertions to use strict equality, introduces type-only imports across the codebase for compilation optimization, adjusts ESLint rules, and applies optional chaining and nullish coalescing patterns throughout. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
LLxprt PR Review – PR #1901Issue AlignmentPR #1901 correctly addresses issue #1569 lint hardening:
The PR description accurately reflects the changes made. Side EffectsUnrelated code changes bundled with test migration:
These additions/refactors are substantial and were not mentioned in the issue. The PR description says "keep behavior unchanged" but these non-test changes could affect behavior. They appear to be refactoring for maintainability but are outside the scope of "enforce strict equality." Positive side effects:
Code Quality
Tests and Coverage
Verdict: Needs WorkRationale: While the core strict-equality migration is correctly implemented, the PR bundles ~2200 lines of unrelated code changes (a2a-server additions, policy config refactor, tool output limiter changes) that were not part of issue #1569 scope. This violates scope discipline and creates risk of regressions unrelated to the stated goal. The PR should either:
The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/a2a-server/src/agent/task-support.ts`:
- Around line 468-473: The comment about "!==
ToolConfirmationOutcome.ModifyWithEditor does not work!" is misleading; update
it to explain why comparing against the enum member fails and clarify the
correct check used: explain that confirmationOutcome is a runtime string (not
the enum type) so we must compare to the literal 'modify_with_editor' rather
than the enum member, or alternatively compare to
ToolConfirmationOutcome.ModifyWithEditor if you change the caller to pass the
enum value; reference the variables confirmationOutcome and
context.pendingToolConfirmationDetails and mention
ToolConfirmationOutcome.ModifyWithEditor so reviewers know where to look.
- Around line 729-731: The AnsiOutput type import currently appears at the
bottom of the file after its usage by convertAnsiOutputToString; move the import
"AnsiOutput" from '@vybestack/llxprt-code-core' into the main import block at
the top alongside the other imports from that module (so
convertAnsiOutputToString clearly has its type available near related imports),
then remove the duplicate bottom import lines that are currently at the end of
the file.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9277bde3-adc1-415f-bc53-91b0908ca88f
⛔ Files ignored due to path filters (9)
project-plans/issue1569c/BATCH_INVENTORY.mdis excluded by!project-plans/**project-plans/issue1569c/BATCH_TEMPLATE.mdis excluded by!project-plans/**project-plans/issue1569c/PLAN.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-01-test-discipline.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-02-type-imports.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-03-readability.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-04-boolean-nullish.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-05-complexity.mdis excluded by!project-plans/**project-plans/issue1569c/rule-family-06-sonar-maintainability.mdis excluded by!project-plans/**
📒 Files selected for processing (13)
eslint.config.jspackages/a2a-server/src/agent/executor.tspackages/a2a-server/src/agent/task-support.tspackages/a2a-server/src/agent/task.test.tspackages/a2a-server/src/agent/task.tspackages/a2a-server/src/commands/extensions.test.tspackages/a2a-server/src/commands/extensions.tspackages/a2a-server/src/commands/restore.test.tspackages/a2a-server/src/config/config.tspackages/a2a-server/src/utils/testing_utils.tspackages/core/src/config/config.test.tspackages/core/src/policy/config.test.tspackages/core/src/providers/anthropic/AnthropicApiExecution.ts
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/a2a-server/src/agent/task-support.ts`:
- Around line 693-716: The applyReplacement function should accept an optional
expectedReplacements: number = 1 and only perform a single replacement when
expectedReplacements === 1 (to match the behavior in
packages/core/src/tools/edit.ts); update the signature of
applyReplacement(currentContent, oldString, newString, isNewFile,
expectedReplacements = 1) and implement conditional logic: if isNewFile return
newString, if currentContent is null handle defensively as today, if oldString
is empty return currentContent, then if expectedReplacements === 1 call
currentContent.replace(oldString, newString), if expectedReplacements > 1
perform up-to-N replacements (e.g., loop using indexOf/substring to replace at
most expectedReplacements occurrences), and if you want full replace-all when
expectedReplacements is infinite or a sentinel use replaceAll; keep the unique
function name applyReplacement to locate the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 050000ed-e160-42bd-9ff3-b01f342acc1c
📒 Files selected for processing (1)
packages/a2a-server/src/agent/task-support.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (32)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1767
File: packages/cli/src/ui/components/shared/golden-snapshot.test.ts:64-65
Timestamp: 2026-03-24T21:07:40.805Z
Learning: In `packages/cli/src/ui/components/shared/golden-snapshot.test.ts` (vybestack/llxprt-code PR `#1767`), `parseAction` uses `actionStr.split(':')` to parse action corpus entries. The action corpus (`project-plans/issue1577/action-corpus.json`) has been confirmed to contain zero multi-colon action strings, so the current two-element destructuring is correct for all existing entries. Do not flag this as a truncation bug in future reviews — the corpus format is validated and the parsing matches it.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1870
File: packages/cli/src/config/postConfigRuntime.ts:248-252
Timestamp: 2026-04-02T11:09:58.796Z
Learning: In vybestack/llxprt-code, packages/cli/src/config/postConfigRuntime.ts reapplyCliOverrides() intentionally uses truthiness checks for bootstrapArgs.keyOverride/keyfileOverride/baseurlOverride to preserve pre-existing behavior; empty-string clears are not re-applied in this step. Do not flag this in lint-only/refactor PRs; propose a dedicated follow-up if behavior should change to presence-based checks.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1771
File: packages/core/src/core/AgentHookManager.ts:78-96
Timestamp: 2026-03-25T19:21:43.266Z
Learning: In `packages/core/src/core/AgentHookManager.ts` (vybestack/llxprt-code PR `#1771`), `cleanupOldHookState(newPromptId, oldPromptId)` is always called at the start of a new prompt turn (when `prompt_id` changes), after the previous prompt's stream has been fully consumed by `MessageStreamOrchestrator`. Because JS is single-threaded, there is no concurrent execution path where an in-flight `fireAfterAgentHookSafe` for the old `prompt_id` could race with the cleanup. Do not flag this pattern as a race condition in future reviews — the sequential flow guarantees: previous prompt finishes → new prompt starts → cleanup runs → fresh hook state created.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:198-208
Timestamp: 2026-03-26T03:04:09.288Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`), `applyToolWhitelistToEphemerals` treats an explicit empty `ephemerals['tools.allowed']` array as "no restriction" (falling back to `normalizedWhitelist`) because it checks `existingAllowed.length > 0` rather than `Array.isArray(ephemerals['tools.allowed'])`. This is pre-existing behavior faithfully preserved from the original `subagent.ts` (~line 370). Do not flag the empty-allowlist pass-through as a bug in decomposition or refactoring PRs — any fix to distinguish "absent" from "explicitly empty" would be a behavioral change requiring a dedicated follow-up PR.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1767
File: packages/cli/src/ui/components/shared/buffer-operations.ts:177-186
Timestamp: 2026-03-24T21:07:36.335Z
Learning: In `packages/cli/src/ui/components/shared/buffer-operations.ts` (vybestack/llxprt-code PR `#1767`), the missing blank line between `getLineRangeOffsets` and `pushUndo` is intentional pre-existing style moved verbatim from `text-buffer.ts`. Do not flag missing blank-line separators between function boundaries in this file during decomposition/refactoring PRs.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagent.ts:552-597
Timestamp: 2026-03-26T02:05:48.262Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code), `processNonInteractiveText` intentionally calls `this.ctx.onMessage(filtered.displayText)` with the raw `textResponse` *before* `parseTextToolCalls` strips tool-call syntax into `cleanedText`. This ordering is pre-existing behavior faithfully preserved from the original `runNonInteractive` implementation. Do not flag the early `onMessage` emission (before tool-call stripping) as a bug in decomposition or refactoring PRs — any change to emit only `cleanedText` would be a behavioral improvement, not a structural fix, and should be tracked as a dedicated follow-up issue.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:100-116
Timestamp: 2026-03-26T02:22:19.732Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts` (vybestack/llxprt-code PR `#1780`), the `filteredPendingTools` predicate in `mergePendingToolGroupsForDisplay` only removes overlapping shell entries from the scheduler side, leaving overlapping non-shell tool callIds potentially duplicated. This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 100-140). Do not flag this as a dedup bug in decomposition or refactoring PRs — any improvement to deduplicate non-shell overlapping callIds would be a separate enhancement, not a structural fix.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-81
Timestamp: 2026-03-25T18:17:57.109Z
Learning: In `packages/cli/src/ui/hooks/useCoreEventHandlers.ts` (vybestack/llxprt-code PR `#1768`), the `useEffect` for `ConsolePatcher` intentionally registers cleanup via BOTH `registerCleanup(consolePatcher.cleanup)` (process-level teardown) AND `return () => consolePatcher.cleanup()` (React unmount lifecycle). This dual registration is deliberate: process-level cleanup persists beyond React unmount, and the React return handles component lifecycle. `consolePatcher.cleanup` is idempotent, so double execution is safe. Do not flag this as redundant double cleanup in future reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:198-208
Timestamp: 2026-03-26T03:06:11.693Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`), `applyToolWhitelistToEphemerals` treats an explicit empty `ephemerals['tools.allowed']` array as "no restriction" (falling back to `normalizedWhitelist`) because it checks `existingAllowed.length > 0` rather than `Array.isArray(ephemerals['tools.allowed'])`. This is pre-existing behavior faithfully preserved from the original `subagent.ts` `createToolExecutionConfig` function (lines 322–331 on main, ~line 370 of the monolith). Do not flag the empty-allowlist pass-through as a bug in decomposition or refactoring PRs — any fix to distinguish "absent" from "explicitly empty" would be a behavioral change requiring a dedicated follow-up PR.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/approvalModeParity.test.ts:351-393
Timestamp: 2026-03-27T01:00:29.058Z
Learning: In `packages/cli/src/config/__tests__/approvalModeParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted approval-mode resolution logic in `approvalModeResolver.ts` produces identical results to the original inline logic. Adding new combination scenarios (e.g., admin-disabled YOLO combined with an untrusted folder) is considered scope expansion beyond parity coverage. The ordering of admin checks before trust-fallback checks is preserved from the original code. Do not flag missing cross-branch combination tests in this file as a gap in refactoring PRs.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:168-172
Timestamp: 2026-03-26T03:04:10.186Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), `createToolExecutionConfig` hardcodes `getExcludeTools: () => []`, and `createSchedulerConfig` preferentially delegates to that empty implementation over `foregroundConfig.getExcludeTools()`. This faithfully preserves the original `subagent.ts` behavior (line 344: `getExcludeTools: () => []`) and the `createSchedulerConfig()` fallback pattern (lines 1518–1521). Do not flag the empty `getExcludeTools` or the resulting masking of parent excluded tools as a regression introduced by decomposition PRs — any fix to forward `foregroundConfig.getExcludeTools()` or merge `snapshot.tools.disabled` into the returned list would be a behavioral change requiring a dedicated follow-up PR.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/parseArgumentsParity.test.ts:7-16
Timestamp: 2026-03-27T00:46:42.630Z
Learning: In `packages/cli/src/config/__tests__/parseArgumentsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted `parseArguments` function produces identical output to the original inline parsing. It deliberately does NOT cover subcommand exit behavior (mcp, hooks, extensions) via `handleSubcommandExit()` — that function is a direct mechanical extraction and adding integration tests for it would be new coverage beyond the refactoring scope. Do not flag the absence of subcommand-exit test cases as a gap in this file or in refactoring PRs that extract it.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1260
File: packages/core/src/tools/task.ts:645-894
Timestamp: 2026-02-11T15:12:48.092Z
Learning: In packages/core/src/tools/task.ts, async tasks intentionally pass `undefined` (not the foreground signal) to orchestrator.launch() so background tasks run independently of foreground agent lifecycle (Ctrl+C, turn completion). They use their own asyncAbortController for manual cancellation via /task end and setTimeout for timeout handling. This is documented at lines 748-754 and was fixed in commit c00f12740.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/parseArgumentsParity.test.ts:7-16
Timestamp: 2026-03-27T00:46:42.685Z
Learning: In `packages/cli/src/config/__tests__/parseArgumentsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite intentionally does NOT cover subcommand exit behavior (`mcp`, `hooks`, `extensions` via `handleSubcommandExit()`). The suite's scope is limited to verifying that the extracted `parseArguments` function produces identical results to the original inline parsing. `handleSubcommandExit()` is a direct mechanical extraction and adding integration tests for it would be new coverage beyond the refactor's scope. Do not flag missing subcommand-exit test cases in this file as a gap in future reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/scheduler/tool-executor.ts:99-112
Timestamp: 2026-03-25T22:22:12.030Z
Learning: In `packages/core/src/scheduler/tool-executor.ts` (vybestack/llxprt-code), `applyAfterHookModifications` uses `const appendText = systemMessage || additionalContext || ''` — this is intentional pre-existing behavior from the original code on main. When both `systemMessage` and `getAdditionalContext()` are present, `systemMessage` wins via short-circuit OR and `additionalContext` is silently dropped. This is not a bug introduced by any extraction/decomposition PR; any fix would be a behavioral change requiring a dedicated PR. Do not flag this as a bug in decomposition reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentToolExecution.ts:279-339
Timestamp: 2026-03-26T02:06:14.142Z
Learning: In `packages/core/src/core/subagentToolExecution.ts` (vybestack/llxprt-code PR `#1779`), `executeToolCalls` is exclusively for the non-interactive path and intentionally checks only `toolResponse.error` and `toolResponse.errorType` (via `isFatalToolError()`), never `CompletedToolCall.status`. This faithfully preserves the original `processFunctionCalls` method (pre-refactor subagent.ts ~line 1380). `CompletedToolCall.status` is only relevant in the interactive path, handled by `buildPartsFromCompletedCalls`. Do not flag the absence of `status` propagation in `executeToolCalls` as a regression — the two paths are intentionally separate.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1741
File: packages/core/src/config/configBase.ts:217-230
Timestamp: 2026-03-24T23:37:09.309Z
Learning: In `packages/core/src/config/configBase.ts` (vybestack/llxprt-code PR `#1741`, decomposed from the original `config.ts`), the `setEphemeralSetting` hot-update path for the `'task-max-async'` key maps `undefined`, `null`, or unparseable string values to `0` (via `isNaN(parsed) ? 0 : parsed` and the `else { normalizedValue = 0 }` branch) before calling `asyncTaskManager.setMaxAsyncTasks(normalizedValue)`. This is pre-existing behavior faithfully moved verbatim from the original `packages/core/src/config/config.ts` on `main`. Do not flag the 0-fallback as a regression or decomposition bug in this or future PRs — any fix to restore the default of `5` instead of `0` would be a behavioral change and should be addressed in a dedicated issue/PR.
📚 Learning: 2026-03-26T02:05:51.733Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentChatSetup.ts:88-120
Timestamp: 2026-03-26T02:05:51.733Z
Learning: In `packages/core/src/core/subagentChatSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), the tool name normalization inconsistency between `validateToolsAgainstRuntime()` (which normalizes via `moduleNormalizeToolName` for the allowlist check but uses the raw name for `toolRegistry.getTool`) and `buildRuntimeFunctionDeclarations` (which calls `getToolMetadata(entry)` with the raw name) is pre-existing behavior faithfully moved from the original `subagent.ts`. Do not flag this inconsistency as a bug introduced by decomposition PRs — any fix to canonicalize tool names before both validation and declaration lookup should be addressed in a dedicated follow-up PR.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:06:14.142Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentToolExecution.ts:279-339
Timestamp: 2026-03-26T02:06:14.142Z
Learning: In `packages/core/src/core/subagentToolExecution.ts` (vybestack/llxprt-code PR `#1779`), `executeToolCalls` is exclusively for the non-interactive path and intentionally checks only `toolResponse.error` and `toolResponse.errorType` (via `isFatalToolError()`), never `CompletedToolCall.status`. This faithfully preserves the original `processFunctionCalls` method (pre-refactor subagent.ts ~line 1380). `CompletedToolCall.status` is only relevant in the interactive path, handled by `buildPartsFromCompletedCalls`. Do not flag the absence of `status` propagation in `executeToolCalls` as a regression — the two paths are intentionally separate.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-04-12T05:38:22.797Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1899
File: packages/core/src/core/subagent.test.ts:2923-2925
Timestamp: 2026-04-12T05:38:22.797Z
Learning: In `packages/core/src/core/subagent.test.ts` (vybestack/llxprt-code), `MessageBus` is used as a type annotation (e.g., `const mockMessageBus = {} as MessageBus`) without a direct import. This is valid because `ToolRegistry` (imported from `../tools/tool-registry.js`) transitively re-exports `MessageBus` types, so TypeScript resolves the name without an explicit import. `tsc --noEmit` passes clean. Do not flag the missing `MessageBus` import in this file as a compile error in future reviews.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T03:04:10.186Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:168-172
Timestamp: 2026-03-26T03:04:10.186Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), `createToolExecutionConfig` hardcodes `getExcludeTools: () => []`, and `createSchedulerConfig` preferentially delegates to that empty implementation over `foregroundConfig.getExcludeTools()`. This faithfully preserves the original `subagent.ts` behavior (line 344: `getExcludeTools: () => []`) and the `createSchedulerConfig()` fallback pattern (lines 1518–1521). Do not flag the empty `getExcludeTools` or the resulting masking of parent excluded tools as a regression introduced by decomposition PRs — any fix to forward `foregroundConfig.getExcludeTools()` or merge `snapshot.tools.disabled` into the returned list would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:22:19.732Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:100-116
Timestamp: 2026-03-26T02:22:19.732Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts` (vybestack/llxprt-code PR `#1780`), the `filteredPendingTools` predicate in `mergePendingToolGroupsForDisplay` only removes overlapping shell entries from the scheduler side, leaving overlapping non-shell tool callIds potentially duplicated. This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 100-140). Do not flag this as a dedup bug in decomposition or refactoring PRs — any improvement to deduplicate non-shell overlapping callIds would be a separate enhancement, not a structural fix.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T23:28:47.345Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:172-175
Timestamp: 2026-03-26T23:28:47.345Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), `createToolExecutionConfig` passes only `messageBus` (and not `toolRegistry`) into the dependency bag forwarded to `foregroundConfig.getOrCreateScheduler()`. This faithfully preserves the original `subagent.ts` behavior (lines 349–352 on main), where the scheduler creation similarly did not forward the runtime-scoped tool registry, allowing `Config.getOrCreateScheduler()` to fall back to `this.getToolRegistry()`. Do not flag the missing `toolRegistry` forwarding in `createToolExecutionConfig` as a regression introduced by decomposition PRs — any fix to inject the runtime-scoped registry would be a behavioral change requiring a dedicated follow-up PR with its own behavioral tests.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-02-11T15:12:48.092Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1260
File: packages/core/src/tools/task.ts:645-894
Timestamp: 2026-02-11T15:12:48.092Z
Learning: In packages/core/src/tools/task.ts, async tasks intentionally pass `undefined` (not the foreground signal) to orchestrator.launch() so background tasks run independently of foreground agent lifecycle (Ctrl+C, turn completion). They use their own asyncAbortController for manual cancellation via /task end and setTimeout for timeout handling. This is documented at lines 748-754 and was fixed in commit c00f12740.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:05:48.262Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagent.ts:552-597
Timestamp: 2026-03-26T02:05:48.262Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code), `processNonInteractiveText` intentionally calls `this.ctx.onMessage(filtered.displayText)` with the raw `textResponse` *before* `parseTextToolCalls` strips tool-call syntax into `cleanedText`. This ordering is pre-existing behavior faithfully preserved from the original `runNonInteractive` implementation. Do not flag the early `onMessage` emission (before tool-call stripping) as a bug in decomposition or refactoring PRs — any change to emit only `cleanedText` would be a behavioral improvement, not a structural fix, and should be tracked as a dedicated follow-up issue.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-25T22:22:12.030Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/scheduler/tool-executor.ts:99-112
Timestamp: 2026-03-25T22:22:12.030Z
Learning: In `packages/core/src/scheduler/tool-executor.ts` (vybestack/llxprt-code), `applyAfterHookModifications` uses `const appendText = systemMessage || additionalContext || ''` — this is intentional pre-existing behavior from the original code on main. When both `systemMessage` and `getAdditionalContext()` are present, `systemMessage` wins via short-circuit OR and `additionalContext` is silently dropped. This is not a bug introduced by any extraction/decomposition PR; any fix would be a behavioral change requiring a dedicated PR. Do not flag this as a bug in decomposition reviews.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:06:03.086Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentChatSetup.ts:339-453
Timestamp: 2026-03-26T02:06:03.086Z
Learning: In `packages/core/src/core/subagentChatSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), the `assembleChatSession` function's `try/catch` block wraps only `new GeminiChat(...)` and not the preceding `await ctx.environmentContextLoader(...)` or `await getCoreSystemPromptAsync(...)` calls. This partial error boundary is pre-existing behavior faithfully preserved from the original `createChatObject` method in `subagent.ts`. Do not flag the missing async-await coverage as a decomposition regression in future reviews — it is an acknowledged pre-existing limitation, and any fix would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-25T23:07:18.966Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/scheduler/confirmation-coordinator.test.ts:593-645
Timestamp: 2026-03-25T23:07:18.966Z
Learning: In `packages/core/src/scheduler/confirmation-coordinator.ts` (vybestack/llxprt-code), `_applyInlineModify` short-circuits early when `confirmationDetails.type !== 'edit'` (around line 677). `setArgs` is only called for edit-type tools that implement `isModifiableDeclarativeTool`. The test "inline modify with newContent payload → updates args, schedules" in `confirmation-coordinator.test.ts` intentionally uses an exec-type confirmation and does NOT assert `setArgs` — that would be wrong. The edit-type inline modify path (which does call `setArgs`) is covered by integration tests in `coreToolScheduler.interactiveMode.test.ts`. Do not flag the absence of a `setArgs` assertion in this test as a bug.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-27T01:00:28.649Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/toolGovernanceParity.test.ts:458-472
Timestamp: 2026-03-27T01:00:28.649Z
Learning: In `packages/cli/src/config/__tests__/toolGovernanceParity.test.ts` (vybestack/llxprt-code PR `#1785`), the "non-interactive DEFAULT with explicit --allowed-tools" and corresponding YOLO test cases intentionally use `read_file` (a tool already in `READ_ONLY_TOOL_NAMES`) as the explicit `--allowed-tools` value. The tests are scoped to verifying that existing allowlist behavior is preserved through the refactor, not to proving union/exclusion semantics with non-read-only tools. Do not flag the choice of `read_file` as insufficient for proving merging behavior in parity or refactoring PRs — improving assertion specificity is new test design work beyond the refactoring scope.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-25T22:20:06.968Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/scheduler/confirmation-coordinator.ts:574-592
Timestamp: 2026-03-25T22:20:06.968Z
Learning: In `packages/core/src/scheduler/confirmation-coordinator.ts` (vybestack/llxprt-code PR `#1775`), `handleModifyWithEditor` builds policy context via `getPolicyContextFromInvocation(waitingToolCall.invocation, waitingToolCall.request)` after `this.statusMutator.setArgs(callId, updatedParams)`, using the pre-mutation snapshot. This is intentional — it faithfully preserves the original `main` behavior (coreToolScheduler.ts ~line 1076-1078). Additionally, `getPolicyContextFromInvocation` uses `invocation.getPolicyContext()` for `BaseToolInvocation` instances (extracts `toolName`/`serverName`, not `args`), so the stale `request.args` only affects the fallback path. Do not flag this as a stale-snapshot bug in future reviews.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T03:06:11.693Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:198-208
Timestamp: 2026-03-26T03:06:11.693Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`), `applyToolWhitelistToEphemerals` treats an explicit empty `ephemerals['tools.allowed']` array as "no restriction" (falling back to `normalizedWhitelist`) because it checks `existingAllowed.length > 0` rather than `Array.isArray(ephemerals['tools.allowed'])`. This is pre-existing behavior faithfully preserved from the original `subagent.ts` `createToolExecutionConfig` function (lines 322–331 on main, ~line 370 of the monolith). Do not flag the empty-allowlist pass-through as a bug in decomposition or refactoring PRs — any fix to distinguish "absent" from "explicitly empty" would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T03:04:09.288Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:198-208
Timestamp: 2026-03-26T03:04:09.288Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`), `applyToolWhitelistToEphemerals` treats an explicit empty `ephemerals['tools.allowed']` array as "no restriction" (falling back to `normalizedWhitelist`) because it checks `existingAllowed.length > 0` rather than `Array.isArray(ephemerals['tools.allowed'])`. This is pre-existing behavior faithfully preserved from the original `subagent.ts` (~line 370). Do not flag the empty-allowlist pass-through as a bug in decomposition or refactoring PRs — any fix to distinguish "absent" from "explicitly empty" would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T20:52:08.720Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:140-149
Timestamp: 2026-03-26T20:52:08.720Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), the `applyThoughtToState` function's `setPendingHistoryItem` updater casts `item?.type as 'gemini' | 'gemini_content'` and uses `item?.text || ''` — this is intentional pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 1110–1111 on main). The risk that a `tool_group` pending item type could be preserved is a known pre-existing pattern; fixing it requires careful testing of all thinking block rendering paths and is a behavioral change beyond decomposition scope. Do not flag this type cast as a bug in decomposition or refactoring PRs.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-25T22:20:10.399Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/scheduler/confirmation-coordinator.ts:311-326
Timestamp: 2026-03-25T22:20:10.399Z
Learning: In `packages/core/src/scheduler/confirmation-coordinator.ts` (vybestack/llxprt-code PR `#1775`), the MessageBus response handler intentionally passes `waitingToolCall.confirmationDetails.onConfirm` (the already-wrapped callback) to `handleConfirmationResponse`. This is identical to the original `coreToolScheduler.ts` on main (line ~359-366). The `processedConfirmations` set prevents double-invocation as the deduplication guard. Do not flag this wrapping pattern as a bug — it is correct pre-existing behavior preserved by the decomposition.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:06:06.881Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentToolExecution.ts:145-209
Timestamp: 2026-03-26T02:06:06.881Z
Learning: In `packages/core/src/core/subagentToolExecution.ts` (vybestack/llxprt-code PR `#1779`), `handleEmitValueCall` uses a truthiness guard (`if (variableName && variableValue)`) to validate both `emit_variable_name` and `emit_variable_value` before accepting the call. This conflicts with the `getScopeLocalFuncDefs` schema, which declares `emit_variable_value` as a required `string` (allowing `""`), meaning an empty-string value is schema-valid but rejected at runtime. This inconsistency is pre-existing behavior faithfully preserved from the original `handleEmitValueCall` implementation in the monolithic `subagent.ts`. Do not flag this as a regression introduced by the decomposition PR — a dedicated follow-up issue should address it.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T23:28:48.119Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentRuntimeSetup.ts:310-337
Timestamp: 2026-03-26T23:28:48.119Z
Learning: In `packages/core/src/core/subagentRuntimeSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), `buildChatSystemPrompt` returns `''` early when `promptConfig.systemPrompt` is absent, silently dropping `outputConfig` (including `self_emitvalue` instructions and the terminal "stop calling tools" rule) on the `initialMessages` path. This is pre-existing behavior faithfully preserved from the original `subagent.ts` (L2090-2095 and L1866 on main). A comment in the original code notes: "createChatObject normally guards this path; this branch keeps the prompt builder defensive." Do not flag this early-return or the `initialMessages + outputConfig` silent-drop as a regression in decomposition or refactoring PRs — any fix would be a behavioral change requiring a dedicated follow-up PR with its own tests.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-02-16T16:11:14.944Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1434
File: packages/core/src/tools/delete_line_range.ts:204-254
Timestamp: 2026-02-16T16:11:14.944Z
Learning: In packages/core/src/tools (ast-edit.ts, delete_line_range.ts, insert_at_line.ts, edit.ts, write-file.ts, apply-patch.ts), the LSP diagnostics collection logic (~50 lines for checkFile, filter by includeSeverities, limit per maxDiagnosticsPerFile, format with <diagnostics> tags) is intentionally duplicated across six files. A follow-up issue has been suggested to extract this into a shared helper function (e.g., collectLspDiagnosticsBlock) that would add Promise.race timeout handling and fix the hardcoded "LSP errors" label to reflect actual severities.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-25T19:21:43.266Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1771
File: packages/core/src/core/AgentHookManager.ts:78-96
Timestamp: 2026-03-25T19:21:43.266Z
Learning: In `packages/core/src/core/AgentHookManager.ts` (vybestack/llxprt-code PR `#1771`), `cleanupOldHookState(newPromptId, oldPromptId)` is always called at the start of a new prompt turn (when `prompt_id` changes), after the previous prompt's stream has been fully consumed by `MessageStreamOrchestrator`. Because JS is single-threaded, there is no concurrent execution path where an in-flight `fireAfterAgentHookSafe` for the old `prompt_id` could race with the cleanup. Do not flag this pattern as a race condition in future reviews — the sequential flow guarantees: previous prompt finishes → new prompt starts → cleanup runs → fresh hook state created.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-24T21:07:36.335Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1767
File: packages/cli/src/ui/components/shared/buffer-operations.ts:177-186
Timestamp: 2026-03-24T21:07:36.335Z
Learning: In `packages/cli/src/ui/components/shared/buffer-operations.ts` (vybestack/llxprt-code PR `#1767`), the missing blank line between `getLineRangeOffsets` and `pushUndo` is intentional pre-existing style moved verbatim from `text-buffer.ts`. Do not flag missing blank-line separators between function boundaries in this file during decomposition/refactoring PRs.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-19T23:27:48.689Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIStreamProcessor.ts:110-123
Timestamp: 2026-03-19T23:27:48.689Z
Learning: In `packages/core/src/providers/openai/OpenAIStreamProcessor.ts` (extracted from the original `generatePipelineChatCompletionImpl` in `OpenAIProvider.ts`, lines 2050-2160), text-parsed tool calls from `extractKimiToolCallsFromText()` and `deps.textToolParser.parse()` (GemmaToolCallParser) are intentionally yielded directly as `ToolCallBlock[]` without being routed through `deps.toolCallPipeline.addFragment()`. The `toolCallPipeline` is exclusively used for delta-based streaming tool calls coming from the OpenAI API. This two-path design is deliberate — do not flag missing `addFragment()` calls for text-parsed tool calls as a bug.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-19T22:50:26.075Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIRequestBuilder.ts:194-205
Timestamp: 2026-03-19T22:50:26.075Z
Learning: In `packages/core/src/providers/openai/OpenAIRequestBuilder.ts` (extracted from the original `OpenAIProvider.ts` around line 1051-1067), when building assistant messages in `processAssistantMessage`, the `content` property is intentionally omitted entirely from `baseMessage` when `tool_calls` are present. This is CRITICAL for Mistral API compatibility (issue `#760`): Mistral's OpenAI-compatible API requires that no `content` property is included at all (not even `null`) when `tool_calls` exist on an assistant message. Do not flag this as a bug or suggest adding `content` alongside `tool_calls`.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-19T22:50:00.853Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIRequestBuilder.test.ts:316-395
Timestamp: 2026-03-19T22:50:00.853Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/openai/OpenAIRequestBuilder.ts`, previously `OpenAIProvider.ts` around line 1250), `validateToolMessageSequence` intentionally does NOT deduplicate tool messages by `tool_call_id`. Contiguous tool messages sharing the same `tool_call_id` are legitimate in conversation history (e.g., replay artifacts or adapter-level splitting across message boundaries). The function only removes *orphaned* tool messages (those whose `tool_call_id` does not match any declared tool call in the preceding assistant message). Eager deduplication of same-id tool messages has historically caused strict-provider HTTP 400 errors and must not be introduced.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T20:01:50.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:0-0
Timestamp: 2026-03-26T20:01:50.052Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` reserves the `callId` in `checkpointedCallIds` *before* calling `await createToolCheckpoint(...)` to prevent concurrent duplicate attempts. On failure (exception or false/no-write return), the `callId` is deleted from the set so the next effect run can retry. This reserve-before-await, delete-on-failure pattern was introduced in commit a41b97759.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-26T02:22:12.153Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:145-153
Timestamp: 2026-03-26T02:22:12.153Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` intentionally does NOT deduplicate checkpoint writes by `callId`. Each effect re-run may re-checkpoint the same `awaiting_approval` tool call (producing multiple snapshot files/git snapshots). This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts`. Adding per-callId deduplication would be a new feature beyond the decomposition scope. Do not flag the absence of a checkpointed-callId Set as a bug in decomposition or refactoring PRs.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-27T01:40:09.382Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1790
File: packages/core/src/core/compression/MiddleOutStrategy.ts:307-325
Timestamp: 2026-03-27T01:40:09.382Z
Learning: In `packages/core/src/core/compression/MiddleOutStrategy.ts` (vybestack/llxprt-code), `IContent` messages with `speaker === 'human'` contain only `TextBlock` types by definition in the conversation model. Tool call and tool response blocks appear exclusively on `speaker === 'ai'` and `speaker === 'tool'` messages. Therefore, `extractTextFromMessage` (which filters for `TextBlock`) captures the full content of any human message, and calling `estimateTokens` on its output is a complete token estimate for human messages. Do not flag token estimation for human messages as incomplete due to missing non-text block handling.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-31T15:03:03.633Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-31T15:03:03.633Z
Learning: In vybestack/llxprt-code, `buildContinuationMessages` in `packages/core/src/providers/openai/OpenAIRequestBuilder.ts` (line 524) always appends `{ role: 'assistant', tool_calls: toolCalls }` to the history. Passing an empty `[]` array produces an invalid assistant message with `tool_calls: []` that most OpenAI-compatible APIs reject with HTTP 400. Do NOT call `requestContinuationAfterToolCalls` with an empty tool calls array for thinking-only continuation — a separate `requestContinuationAfterThinking` method is needed.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/a2a-server/src/agent/task-support.ts
🔇 Additional comments (11)
packages/a2a-server/src/agent/task-support.ts (11)
12-42: LGTM!Imports are well-organized with clear separation between external packages and internal modules. The
AnsiOutputtype is now properly imported at the top alongside other types from@vybestack/llxprt-code-core.
44-64: LGTM!The
UnionKeystype utility correctly distributes over union types to extract all possible keys. ThepickFieldsfunction properly guards withfield in frombefore assignment, making it safe for union types where not all keys exist on all variants.
66-115: LGTM!Clean extraction of serializable ToolCall properties using
pickFields, avoiding circular reference issues. The message structure properly conforms to the A2A SDKMessagetype.
117-233: LGTM!Good use of
ReadonlySetfor immutable event type classification at module level. The type guardisInformationalAgentEventcorrectly narrows the event type, andlogInformationalEventprovides clean dispatch with appropriate log levels per event category.
235-291: LGTM!Clean helper functions for confirmation handling. The type guard
isInteractiveConfirmationDetailscorrectly uses the presence ofonConfirmto discriminate between interactive and serializable confirmation details. The outcome string mapping covers all expected values with a safeundefinedreturn for unknown outcomes.
293-361: LGTM!The extracted interfaces clearly document the dependencies needed by each handler, avoiding tight coupling to the
Taskclass. Error handling correctly extracts messages fromErrorinstances and publishes appropriate status updates.
492-613: LGTM!Stream event handlers follow a consistent pattern: log the event, cancel pending tools, and publish a state update. The distinction between handlers that transition to
'input-required'(timeout, user cancelled) vs. those that preserve current state (invalid stream, error) is appropriate for their semantics.
615-650: LGTM!Clean normalization logic that provides
DEFAULT_AGENT_IDfallback and computesnewContentfor'replace'tool calls. The function correctly creates new objects rather than mutating the input.
652-691: LGTM!Good use of atomic write pattern (temp file + rename) for crash safety. The graceful degradation on write failures—logging warnings rather than throwing—is appropriate since checkpoints are an optimization, not a requirement.
718-771: LGTM!Clean utility functions for ANSI output conversion and message creation. The
convertAnsiOutputToStringcorrectly handles both string and structuredAnsiOutputformats. Message factories properly construct A2A SDK-compliantMessageobjects with unique IDs.
423-467: No action needed—the GCP environment variable manipulation pattern (GOOGLE_CLOUD_PROJECT,GOOGLE_APPLICATION_CREDENTIALSsave/delete/restore) is pre-existing behavior faithfully preserved from the originaltask.tsand is acceptable for this decomposition PR.
Comment on lines
+693
to
+716
| /** | ||
| * Applies a string replacement to content. | ||
| * Pure helper function used by getProposedContent. | ||
| */ | ||
| export function applyReplacement( | ||
| currentContent: string | null, | ||
| oldString: string, | ||
| newString: string, | ||
| isNewFile: boolean, | ||
| ): string { | ||
| if (isNewFile) { | ||
| return newString; | ||
| } | ||
| if (currentContent === null) { | ||
| // Should not happen if not a new file, but defensively return empty or newString if oldString is also empty | ||
| return oldString === '' ? newString : ''; | ||
| } | ||
| // If oldString is empty, do not modify the content. At this point isNewFile is always false | ||
| // due to the early return, so no need to check it again. | ||
| if (oldString === '') { | ||
| return currentContent; | ||
| } | ||
| return currentContent.replaceAll(oldString, newString); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the 'replace' tool defines replacement semantics in core
ast-grep --pattern 'replaceAll($_, $_)'
rg -n "replaceAll|\.replace\(" packages/core/src/tools/replaceRepository: vybestack/llxprt-code
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Find files related to the replace tool
fd -i "replace" packages/core --type f | head -20
ls -la packages/core/src/tools/ 2>/dev/null || echo "tools dir not found, checking alternatives"
find packages/core/src -name "*replace*" -type fRepository: vybestack/llxprt-code
Length of output: 7875
🏁 Script executed:
#!/bin/bash
# Search for replace tool semantics
rg -n "replace.*tool" packages/core/src/tools/ -i | head -20
# Check fuzzy-replacer for clues about replace semantics
head -100 packages/core/src/tools/fuzzy-replacer.ts
# Search for where applyReplacement is called
rg -n "applyReplacement" packages/a2a-server/src/
# Look for replaceAll usage in the entire core tools
rg -n "replaceAll" packages/core/src/tools/Repository: vybestack/llxprt-code
Length of output: 5517
🏁 Script executed:
#!/bin/bash
# Check how applyReplacement is called in task.ts
sed -n '500,530p' packages/a2a-server/src/agent/task.ts
# Check if expected_replacements is used anywhere
rg -n "expected_replacements" packages/a2a-server/src/
# Look at EditToolParams to understand the expected contract
sed -n '1,100p' packages/core/src/tools/edit.ts | grep -A 50 "interface.*Params\|type.*Params"Repository: vybestack/llxprt-code
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Search for expected_replacements handling in edit.ts
rg -n "expected_replacements" packages/core/src/tools/edit.ts -A 5 -B 5
# Check the EditToolParams interface
rg -n "interface EditToolParams|type EditToolParams" packages/core/src/tools/edit.ts -A 20
# Check how edit.ts handles single vs multiple replacements
sed -n '120,160p' packages/core/src/tools/edit.tsRepository: vybestack/llxprt-code
Length of output: 7458
🏁 Script executed:
#!/bin/bash
# Find ALL applyReplacement function definitions
rg -n "export.*function applyReplacement|function applyReplacement" -A 5
# Check if edit.ts imports from a different location
head -50 packages/core/src/tools/edit.ts | grep -i "import.*applyReplacement\|from"Repository: vybestack/llxprt-code
Length of output: 2900
applyReplacement always replaces all occurrences, not just the first one.
The function uses replaceAll() on line 715, which replaces all matching occurrences of oldString. This behavior differs from the 'replace' tool in packages/core/src/tools/edit.ts, which defaults to replacing only a single occurrence (see line 127: "For single replacement, use replace() instead of replaceAll()") and only replaces all when an expectedReplacements count is explicitly specified.
The task-support.ts implementation is missing the optional expectedReplacements parameter that the edit.ts version supports (see packages/core/src/tools/edit.ts line 75). Without this parameter, the function cannot distinguish between single and multiple replacements. Update the signature to include expectedReplacements: number = 1 and implement the conditional logic to use replace() for single replacements instead of always using replaceAll().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/a2a-server/src/agent/task-support.ts` around lines 693 - 716, The
applyReplacement function should accept an optional expectedReplacements: number
= 1 and only perform a single replacement when expectedReplacements === 1 (to
match the behavior in packages/core/src/tools/edit.ts); update the signature of
applyReplacement(currentContent, oldString, newString, isNewFile,
expectedReplacements = 1) and implement conditional logic: if isNewFile return
newString, if currentContent is null handle defensively as today, if oldString
is empty return currentContent, then if expectedReplacements === 1 call
currentContent.replace(oldString, newString), if expectedReplacements > 1
perform up-to-N replacements (e.g., loop using indexOf/substring to replace at
most expectedReplacements occurrences), and if you want full replace-all when
expectedReplacements is infinite or a sentinel use replaceAll; keep the unique
function name applyReplacement to locate the change.
Rules disabled with rationale per Issue #1569c plan: - arrow-function-convention: conflicts with Prettier parens handling - no-duplicate-string: 3-occurrence threshold is pure noise - shorthand-property-grouping: pure ordering preference, no correctness value - elseif-without-else: pure style; conflicts with early-return pattern - max-union-size: discriminated unions legitimately exceed arbitrary limit - no-alphabetical-sort: heuristic is false-positive prone on typed arrays - prefer-regexp-exec: String.match and RegExp.exec are both idiomatic - function-name: conflicts with TS class/method naming conventions - prefer-immediate-return: named intermediates improve readability/debuggability - pseudo-random: CLI context uses Math.random for IDs/jitter, not crypto Warning count: 30,504 -> 14,092 (-16,412 pure-noise warnings). Refs #1569
…de (#1569) Applied via per-package 'npx eslint --fix' with consistent-type-imports disabled during autofix (type-imports will be handled in a later batch with proper type-info). Covers: prefer-optional-chain, prefer-nullish-coalescing, no-unnecessary- condition, and other purely mechanical rewrites. No behavior changes. Verified: typecheck clean, build clean. Tests: all passing except one pre-existing flaky timeout (subagentRuntimeSetup.test.ts beforeAll hook) that fails on main too. Smoke: starts fine; API call blocked by env-level credit balance only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/config/toolGovernance.ts`:
- Line 14: The import for GeminiCLIExtension is currently a runtime binding but
it is a type-only export; change the import to a type-only import (use import
type for GeminiCLIExtension) so the compiler/tree-shaker knows it’s not a
runtime value, and confirm the only usage (the type annotation at the location
where GeminiCLIExtension is referenced) remains a type position.
In `@packages/cli/src/ui/utils/MarkdownDisplay.tsx`:
- Around line 264-270: The condition at the else-if in MarkdownDisplay that
checks "line.trim().length === 0 && !inCodeBlock" is redundant because
inCodeBlock is already handled earlier (returns when true); update the else-if
to just check "line.trim().length === 0" so the spacer push logic
(contentBlocks.push(...), lastLineEmpty handling) executes correctly without the
needless inCodeBlock guard; locate this in the MarkdownDisplay component where
lastLineEmpty and contentBlocks are managed and remove the "&& !inCodeBlock"
part.
In `@packages/core/src/tools/__tests__/ast-edit-characterization.test.ts`:
- Line 615: The optional chaining refactor for the guard is correct—leave the
check as confirmationDetails?.onConfirm to preserve prior behavior; no
functional change required. Optionally (future PR) strengthen the test by adding
an explicit assertion that confirmationDetails is truthy and that
confirmationDetails.onConfirm is a function to make the contract explicit.
In `@packages/core/src/tools/grep.ts`:
- Line 172: The abort event listener registration in
signal.addEventListener('abort', onUserAbort) should include the { once: true }
option to match the pattern used elsewhere (e.g., shell.ts) so the listener
auto-removes after firing; update the call to pass the options object ({ once:
true }) when registering onUserAbort to ensure it cannot fire multiple times and
to improve robustness.
In `@packages/core/src/utils/toolOutputLimiter.ts`:
- Around line 257-280: Duplicate token-slicing/truncation logic is present in
two places (the `truncate` branch and the block using `getEncoder()`), so
extract that shared logic into a single helper (e.g., a function referenced by
both branches) that accepts the inputs used here — `encodedContent`, `encoder`
(from `getEncoder()`), `utf8Decoder`, `content`, `targetTokenCount`, and
`originalTokens` — and returns { truncatedContent, truncatedTokenCount } (or
similar). Replace the duplicated code in both the earlier `truncate` branch and
this block with calls to that helper, and keep the final return shape (adding
the truncation marker and `wasTruncated`/`originalTokens`/`message`) unchanged.
- Around line 240-254: The sampler currently pushes the next line into
sampledLines before checking token cost, which can return content that exceeds
effectiveLimit; update the loop in limitOutputTokens (the block using lines,
sampledLines, estimateTokens and effectiveLimit) so you compute the token cost
of sampledLines plus the candidate line first and only push the line if the
estimate remains <= effectiveLimit (otherwise break), ensuring content,
wasTruncated, originalTokens and message still reflect truncation and the
reported limits.maxTokens; also ensure you don't return an empty sampledLines
payload (handle the first-line-too-large case) and keep the final content
assembly logic 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cab0022-eec9-4487-9d23-39aa3e67cad9
📒 Files selected for processing (124)
eslint.config.jspackages/a2a-server/src/http/app.tspackages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.tspackages/cli/src/auth/__tests__/oauthManager.proactive-renewal.test.tspackages/cli/src/auth/__tests__/proactive-renewal-manager.spec.tspackages/cli/src/auth/__tests__/token-access-coordinator.spec.tspackages/cli/src/auth/oauth-manager.issue1468.spec.tspackages/cli/src/auth/oauth-manager.refresh-race.spec.tspackages/cli/src/auth/oauth-manager.runtime-messagebus.spec.tspackages/cli/src/auth/proactive-renewal-manager.tspackages/cli/src/auth/provider-usage-info.tspackages/cli/src/auth/proxy/credential-store-factory.tspackages/cli/src/auth/token-access-coordinator.tspackages/cli/src/commands/extensions/settings.tspackages/cli/src/config/extensions/github.tspackages/cli/src/config/extensions/settingsIntegration.tspackages/cli/src/config/extensions/settingsStorage.tspackages/cli/src/config/interactiveContext.tspackages/cli/src/config/mcpServerConfig.tspackages/cli/src/config/postConfigRuntime.tspackages/cli/src/config/settings-validation.tspackages/cli/src/config/settings.tspackages/cli/src/config/toolGovernance.tspackages/cli/src/extensions/extensionAutoUpdater.tspackages/cli/src/runtime/runtimeAccessors.tspackages/cli/src/services/todo-continuation/todoContinuationService.tspackages/cli/src/ui/commands/authCommand.tspackages/cli/src/ui/commands/chatCommand.tspackages/cli/src/ui/commands/compressCommand.tspackages/cli/src/ui/commands/hooksCommand.tspackages/cli/src/ui/commands/logoutCommand.tspackages/cli/src/ui/commands/lspCommand.tspackages/cli/src/ui/commands/mouseCommand.tspackages/cli/src/ui/commands/policiesCommand.tspackages/cli/src/ui/commands/subagentCommand.tspackages/cli/src/ui/commands/toolkeyCommand.tspackages/cli/src/ui/commands/toolkeyfileCommand.tspackages/cli/src/ui/components/DebugProfiler.tsxpackages/cli/src/ui/components/Help.tsxpackages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/LoggingDialog.tsxpackages/cli/src/ui/components/OAuthCodeDialog.tsxpackages/cli/src/ui/components/SettingsDialog.tsxpackages/cli/src/ui/components/SubagentManagement/ProfileAttachmentWizard.tsxpackages/cli/src/ui/components/ToolsDialog.tsxpackages/cli/src/ui/components/shared/MaxSizedBox.tsxpackages/cli/src/ui/components/shared/text-buffer.test.tspackages/cli/src/ui/components/shared/word-navigation.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppBootstrap.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppDialogs.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppInput.tspackages/cli/src/ui/containers/AppContainer/hooks/useRecordingInfrastructure.tspackages/cli/src/ui/contexts/KeypressContext.test.tsxpackages/cli/src/ui/hooks/atCommandProcessor.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/useExtensionUpdates.tspackages/cli/src/ui/hooks/useTimer.tspackages/cli/src/ui/hooks/useTodoContinuation.tspackages/cli/src/ui/themes/color-utils.tspackages/cli/src/ui/utils/ConsolePatcher.tspackages/cli/src/ui/utils/MarkdownDisplay.tsxpackages/cli/src/ui/utils/fuzzyFilter.tspackages/cli/src/ui/utils/mouse.tspackages/cli/src/ui/utils/secureInputHandler.tspackages/cli/src/ui/utils/terminalSetup.tspackages/cli/src/ui/utils/tokenFormatters.tspackages/cli/src/utils/gitUtils.tspackages/cli/src/utils/sandbox.tspackages/cli/src/utils/skillUtils.tspackages/cli/src/utils/stdinSafety.tspackages/cli/src/zed-integration/zedIntegration.tspackages/core/src/auth/precedence.tspackages/core/src/auth/token-store.refresh-race.spec.tspackages/core/src/config/endpoints.tspackages/core/src/config/storage.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.tspackages/core/src/core/client.test.tspackages/core/src/core/prompts.tspackages/core/src/core/subagentRuntimeSetup.tspackages/core/src/hooks/__tests__/hookSemantics.test.tspackages/core/src/hooks/hookRunner.tspackages/core/src/policy/config.tspackages/core/src/prompt-config/prompt-installer.tspackages/core/src/prompt-config/prompt-service.tspackages/core/src/providers/BaseProvider.tspackages/core/src/providers/LoggingProviderWrapper.tspackages/core/src/providers/anthropic/AnthropicMessageNormalizer.tspackages/core/src/providers/kimi/usageInfo.tspackages/core/src/providers/openai/OpenAIProvider.emptyResponseRetry.test.tspackages/core/src/providers/openai/OpenAIRequestBuilder.tspackages/core/src/providers/openai/ToolCallCollector.tspackages/core/src/providers/openai/ToolCallNormalizer.tspackages/core/src/providers/openai/ToolNameValidator.tspackages/core/src/providers/openai/syntheticToolResponses.tspackages/core/src/providers/openai/toolNameUtils.tspackages/core/src/providers/synthetic/usageInfo.tspackages/core/src/providers/zai/usageInfo.tspackages/core/src/services/history/HistoryService.tspackages/core/src/skills/skillLoader.tspackages/core/src/telemetry/uiTelemetry.test.tspackages/core/src/test-utils/providerCallOptions.tspackages/core/src/tools/ToolFormatter.tspackages/core/src/tools/__tests__/ast-edit-characterization.test.tspackages/core/src/tools/apply-patch.tspackages/core/src/tools/edit.test.tspackages/core/src/tools/edit.tspackages/core/src/tools/glob.tspackages/core/src/tools/google-web-search-invocation.tspackages/core/src/tools/grep.tspackages/core/src/tools/lsp-diagnostics-helper.tspackages/core/src/tools/mcp-client.tspackages/core/src/tools/memoryTool.tspackages/core/src/tools/shell.tspackages/core/src/tools/task.tspackages/core/src/tools/write-file.tspackages/core/src/utils/asyncIterator.test.tspackages/core/src/utils/errors.tspackages/core/src/utils/extensionLoader.tspackages/core/src/utils/partUtils.tspackages/core/src/utils/quotaErrorDetection.tspackages/core/src/utils/retry.tspackages/core/src/utils/toolOutputLimiter.tspackages/vscode-ide-companion/src/open-files-manager.ts
💤 Files with no reviewable changes (2)
- packages/cli/src/ui/components/shared/text-buffer.test.ts
- packages/core/src/core/client.test.ts
Comment on lines
+264
to
270
| } else if (line.trim().length === 0 && !inCodeBlock) { | ||
| if (!lastLineEmpty) { | ||
| contentBlocks.push( | ||
| <Box key={`spacer-${index}`} height={EMPTY_LINE_HEIGHT} />, | ||
| ); | ||
| lastLineEmpty = true; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant condition check.
The && !inCodeBlock check at line 264 is redundant. Lines 90-115 already handle the inCodeBlock case and return early at line 114, so inCodeBlock is guaranteed to be false by the time execution reaches line 264.
♻️ Simplify the condition
- } else if (line.trim().length === 0 && !inCodeBlock) {
+ } else if (line.trim().length === 0) {
if (!lastLineEmpty) {📝 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
| } else if (line.trim().length === 0 && !inCodeBlock) { | |
| if (!lastLineEmpty) { | |
| contentBlocks.push( | |
| <Box key={`spacer-${index}`} height={EMPTY_LINE_HEIGHT} />, | |
| ); | |
| lastLineEmpty = true; | |
| } | |
| } else if (line.trim().length === 0) { | |
| if (!lastLineEmpty) { | |
| contentBlocks.push( | |
| <Box key={`spacer-${index}`} height={EMPTY_LINE_HEIGHT} />, | |
| ); | |
| lastLineEmpty = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/utils/MarkdownDisplay.tsx` around lines 264 - 270, The
condition at the else-if in MarkdownDisplay that checks "line.trim().length ===
0 && !inCodeBlock" is redundant because inCodeBlock is already handled earlier
(returns when true); update the else-if to just check "line.trim().length === 0"
so the spacer push logic (contentBlocks.push(...), lastLineEmpty handling)
executes correctly without the needless inCodeBlock guard; locate this in the
MarkdownDisplay component where lastLineEmpty and contentBlocks are managed and
remove the "&& !inCodeBlock" part.
| ); | ||
|
|
||
| if (confirmationDetails && confirmationDetails.onConfirm) { | ||
| if (confirmationDetails?.onConfirm) { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - Mechanical refactor to optional chaining is correct.
The updated guard confirmationDetails?.onConfirm is semantically identical to the previous confirmationDetails && confirmationDetails.onConfirm pattern and correctly preserves behavior.
💡 Optional test robustness improvement (out of scope for this PR)
The test currently passes silently if confirmationDetails is falsy or lacks onConfirm, without asserting their presence. Consider adding an explicit assertion in a future test improvement PR:
const confirmationDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
);
+expect(confirmationDetails).toBeDefined();
+expect(confirmationDetails?.onConfirm).toBeDefined();
+
if (confirmationDetails?.onConfirm) {
await confirmationDetails.onConfirm(
ToolConfirmationOutcome.ProceedAlways,
);
expect(setApprovalModeSpy).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT);
}This would make test failures more explicit if the API contract changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/tools/__tests__/ast-edit-characterization.test.ts` at line
615, The optional chaining refactor for the guard is correct—leave the check as
confirmationDetails?.onConfirm to preserve prior behavior; no functional change
required. Optionally (future PR) strengthen the test by adding an explicit
assertion that confirmationDetails is truthy and that
confirmationDetails.onConfirm is a function to make the contract explicit.
| } else { | ||
| signal.addEventListener('abort', onUserAbort); | ||
| } | ||
| signal.addEventListener('abort', onUserAbort); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding { once: true } option for consistency and robustness.
The abort listener registration should include the { once: true } option to match the pattern used in shell.ts (line 301). This prevents the listener from firing multiple times if somehow registered more than once, and automatically removes itself after firing.
🔒 Proposed fix to add the `once` option
- signal.addEventListener('abort', onUserAbort);
+ signal.addEventListener('abort', onUserAbort, { once: true });📝 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
| signal.addEventListener('abort', onUserAbort); | |
| signal.addEventListener('abort', onUserAbort, { once: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/tools/grep.ts` at line 172, The abort event listener
registration in signal.addEventListener('abort', onUserAbort) should include the
{ once: true } option to match the pattern used elsewhere (e.g., shell.ts) so
the listener auto-removes after firing; update the call to pass the options
object ({ once: true }) when registering onUserAbort to ensure it cannot fire
multiple times and to improve robustness.
Comment on lines
+240
to
+254
| for (let i = 0; i < lines.length; i += step) { | ||
| sampledLines.push(lines[i]); | ||
| if (estimateTokens(sampledLines.join('\n')) > effectiveLimit) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| content: | ||
| sampledLines.join('\n') + | ||
| `\n\n[Sampled ${sampledLines.length} of ${lines.length} lines due to token limit]`, | ||
| wasTruncated: true, | ||
| originalTokens, | ||
| message: `Output sampled to fit within ${limits.maxTokens} token limit`, | ||
| }; | ||
| } else { | ||
| // Single line or non-line content, fall back to truncate with escape buffer | ||
| const encoder = getEncoder(); | ||
| const targetTokenCount = Math.max(0, Math.min(effectiveLimit, tokens)); | ||
|
|
||
| let truncatedContent = ''; | ||
| let truncatedTokenCount = 0; | ||
|
|
||
| if (encodedContent && encoder) { | ||
| const truncatedTokens = encodedContent.subarray(0, targetTokenCount); | ||
| truncatedContent = utf8Decoder.decode(encoder.decode(truncatedTokens)); | ||
| truncatedTokenCount = truncatedTokens.length; | ||
| } else { | ||
| const ratio = | ||
| originalTokens > 0 | ||
| ? Math.min(1, targetTokenCount / originalTokens) | ||
| : 0; | ||
| const approxChars = Math.floor(content.length * ratio); | ||
| truncatedContent = content.slice(0, approxChars); | ||
| truncatedTokenCount = Math.ceil(truncatedContent.length / 3); | ||
| } | ||
| return { | ||
| content: | ||
| sampledLines.join('\n') + | ||
| `\n\n[Sampled ${sampledLines.length} of ${lines.length} lines due to token limit]`, | ||
| wasTruncated: true, | ||
| originalTokens, | ||
| message: `Output sampled to fit within ${limits.maxTokens} token limit`, | ||
| }; |
Contributor
There was a problem hiding this comment.
Prevent sample mode from returning over-limit content.
Lines 240-243 add the next line before checking effectiveLimit, so this branch can still return a content payload that exceeds the configured cap. That defeats the limiter in the exact case it is supposed to protect.
💡 Proposed fix
- for (let i = 0; i < lines.length; i += step) {
- sampledLines.push(lines[i]);
- if (estimateTokens(sampledLines.join('\n')) > effectiveLimit) {
- break;
- }
- }
-
- return {
- content:
- sampledLines.join('\n') +
- `\n\n[Sampled ${sampledLines.length} of ${lines.length} lines due to token limit]`,
- wasTruncated: true,
- originalTokens,
- message: `Output sampled to fit within ${limits.maxTokens} token limit`,
- };
+ for (let i = 0; i < lines.length; i += step) {
+ const candidateLines = [...sampledLines, lines[i]];
+ if (estimateTokens(candidateLines.join('\n')) > effectiveLimit) {
+ break;
+ }
+ sampledLines.push(lines[i]);
+ }
+
+ if (sampledLines.length > 0) {
+ return {
+ content:
+ sampledLines.join('\n') +
+ `\n\n[Sampled ${sampledLines.length} of ${lines.length} lines due to token limit]`,
+ wasTruncated: true,
+ originalTokens,
+ message: `Output sampled to fit within ${limits.maxTokens} token limit`,
+ };
+ }Based on learnings: packages/core/src/tools/shell.ts applies limitOutputTokens directly to model-facing llmContent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/toolOutputLimiter.ts` around lines 240 - 254, The
sampler currently pushes the next line into sampledLines before checking token
cost, which can return content that exceeds effectiveLimit; update the loop in
limitOutputTokens (the block using lines, sampledLines, estimateTokens and
effectiveLimit) so you compute the token cost of sampledLines plus the candidate
line first and only push the line if the estimate remains <= effectiveLimit
(otherwise break), ensuring content, wasTruncated, originalTokens and message
still reflect truncation and the reported limits.maxTokens; also ensure you
don't return an empty sampledLines payload (handle the first-line-too-large
case) and keep the final content assembly logic unchanged.
Comment on lines
+257
to
+280
| const encoder = getEncoder(); | ||
| const targetTokenCount = Math.max(0, Math.min(effectiveLimit, tokens)); | ||
|
|
||
| return { | ||
| content: `${truncatedContent}\n\n[Output truncated due to token limit]`, | ||
| wasTruncated: true, | ||
| originalTokens, | ||
| message: `Output truncated from ${originalTokens} to ${truncatedTokenCount} tokens`, | ||
| }; | ||
| } | ||
| let truncatedContent = ''; | ||
| let truncatedTokenCount = 0; | ||
|
|
||
| if (encodedContent && encoder) { | ||
| const truncatedTokens = encodedContent.subarray(0, targetTokenCount); | ||
| truncatedContent = utf8Decoder.decode(encoder.decode(truncatedTokens)); | ||
| truncatedTokenCount = truncatedTokens.length; | ||
| } else { | ||
| const ratio = | ||
| originalTokens > 0 ? Math.min(1, targetTokenCount / originalTokens) : 0; | ||
| const approxChars = Math.floor(content.length * ratio); | ||
| truncatedContent = content.slice(0, approxChars); | ||
| truncatedTokenCount = Math.ceil(truncatedContent.length / 3); | ||
| } | ||
|
|
||
| return { | ||
| content: `${truncatedContent}\n\n[Output truncated due to token limit]`, | ||
| wasTruncated: true, | ||
| originalTokens, | ||
| message: `Output truncated from ${originalTokens} to ${truncatedTokenCount} tokens`, | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the shared truncation path into a helper.
Lines 257-280 now duplicate the same token-slicing logic already implemented in the truncate branch above. Keeping two copies makes future fixes to markers, decoding, or token accounting easy to miss in one path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/toolOutputLimiter.ts` around lines 257 - 280,
Duplicate token-slicing/truncation logic is present in two places (the
`truncate` branch and the block using `getEncoder()`), so extract that shared
logic into a single helper (e.g., a function referenced by both branches) that
accepts the inputs used here — `encodedContent`, `encoder` (from
`getEncoder()`), `utf8Decoder`, `content`, `targetTokenCount`, and
`originalTokens` — and returns { truncatedContent, truncatedTokenCount } (or
similar). Replace the duplicated code in both the earlier `truncate` branch and
this block with calls to that helper, and keep the final return shape (adding
the truncation marker and `wasTruncated`/`originalTokens`/`message`) unchanged.
Mechanical sed-driven rewrite: s/.toEqual(/.toStrictEqual(/g across all test files flagged by vitest/prefer-strict-equal. Eight files were deliberately skipped and reverted because their tests passed under the loose toEqual but fail under toStrictEqual (actual value has explicit undefined properties from spread; expected value omits them). Those files will be handled individually in a later batch by updating each test expectation. Warning count: 13,903 -> 11,572 (-2,331, one per rewrite site). vitest/prefer-strict-equal: 2,468 -> 137 (the 8 reverted files remain). Reverted (left at toEqual pending per-test fixup): packages/core/src/code_assist/converter.test.ts packages/core/src/core/turn.test.ts packages/core/src/mcp/oauth-utils.test.ts packages/core/src/services/environmentSanitization.test.ts packages/core/src/auth/proxy/__tests__/proxy-token-store.test.ts packages/cli/src/ui/commands/schema/argumentResolver.test.ts packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx packages/cli/src/ui/hooks/useSlashCompletion.test.ts Verified: typecheck clean, lint clean (0 errors), format applied, per-package test suites pass. The pre-existing flaky tests (subagentRuntimeSetup.test.ts and settings-remediation.test.ts Performance Integration) are unchanged and not caused by this commit. Refs #1569
…s autofix (Fixes #1569) Ran eslint --fix for @typescript-eslint/consistent-type-imports across packages/core and packages/cli (in addition to the a2a-server and vscode-ide-companion touches done earlier in the branch). The rule's autofixer converts value imports of types into "import type", which is mechanical and safe when applied per-file followed by a typecheck gate. Verification path used: 1. Built lists of flagged files from /tmp/cti-cli.txt (435) and /tmp/cti-core.txt (262). 2. Ran autofix against both packages in parallel. 3. Ran npm run typecheck against the whole workspace. Core was clean. CLI produced 512 TS errors (476 TS1361 + 36 TS2206) in 36 files where the autofix-introduced "import type" collided with a value usage or a nested type modifier on an already type-only import. 4. Reverted those 36 files via git checkout --. Re-ran typecheck: 0 errors. 5. Ran npm run format to reflow the affected files. Lint warning delta: @typescript-eslint/consistent-type-imports: 1192 -> 384 Total warnings: 11572 -> 10764 Deferred (to be handled manually in a later batch): the 36 CLI files listed in /tmp/cti-broken-abs.txt. Each needs either (a) split of the import into a value-import + type-import pair, or (b) removal of the type modifier where it conflicts with import type. Tests: full npm run test run shows one pre-existing flake (subagentRuntimeSetup) and one timeout in ToolResultDisplay that passes on re-run in isolation (host was heavily loaded). No regressions attributable to this change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/a2a-server/src/agent/task.ts`:
- Around line 456-458: The conditional block checks if
(!this.skipFinalTrueAfterInlineEdit.value) then assigns
this.skipFinalTrueAfterInlineEdit.value = false, which is a redundant no-op;
remove the assignment inside the if so the code only relies on the condition,
i.e., delete the write to this.skipFinalTrueAfterInlineEdit.value while keeping
the surrounding logic intact (look for the conditional using
skipFinalTrueAfterInlineEdit.value in the task handling code).
In `@packages/cli/src/providers/provider-switching.integration.test.ts`:
- Around line 73-75: The assertion mixes toStrictEqual with
expect.arrayContaining which is contradictory; update the test "without
auto-registering gemini" to assert exact equality instead of a partial match:
replace
expect(providers).toStrictEqual(expect.arrayContaining(['provider1','provider2']))
with an exact-array assertion such as
expect(providers).toStrictEqual(['provider1', 'provider2']) (or use toEqual if
order is not guaranteed) so the variable providers is verified to contain
exactly those two entries.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d180c1da-2ab6-4b82-b498-e864ce04f9da
📒 Files selected for processing (193)
packages/a2a-server/src/agent/task.tspackages/cli/src/auth/__tests__/oauth-manager.issue913.spec.tspackages/cli/src/auth/__tests__/oauth-manager.user-declined.spec.tspackages/cli/src/auth/anthropic-oauth-provider.test.tspackages/cli/src/auth/anthropic-oauth-provider.tspackages/cli/src/auth/gemini-oauth-provider.tspackages/cli/src/auth/migration.tspackages/cli/src/auth/oauth-manager.issue1317.spec.tspackages/cli/src/auth/oauth-manager.spec.tspackages/cli/src/auth/oauth-manager.token-reuse.spec.tspackages/cli/src/auth/provider-registry.tspackages/cli/src/auth/proxy/__tests__/proxy-oauth-adapter.test.tspackages/cli/src/auth/proxy/__tests__/refresh-flow.spec.tspackages/cli/src/auth/qwen-oauth-provider.test.tspackages/cli/src/auth/qwen-oauth-provider.tspackages/cli/src/commands/extensions.tsxpackages/cli/src/commands/extensions/uninstall.tspackages/cli/src/commands/mcp/add.tspackages/cli/src/commands/mcp/list.test.tspackages/cli/src/commands/mcp/list.tspackages/cli/src/config/cliEphemeralSettings.test.tspackages/cli/src/config/config.integration.test.tspackages/cli/src/config/extensions/extensionSettings.test.tspackages/cli/src/config/sandboxConfig.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/coreToolToggle.test.tspackages/cli/src/gemini.test.tsxpackages/cli/src/integration-tests/base-url-behavior.integration.test.tspackages/cli/src/integration-tests/cli-args.integration.test.tspackages/cli/src/integration-tests/compression-settings-apply.integration.test.tspackages/cli/src/integration-tests/compression-todo.integration.test.tspackages/cli/src/integration-tests/ephemeral-settings.integration.test.tspackages/cli/src/integration-tests/profile-keyfile.integration.test.tspackages/cli/src/integration-tests/profile-system.integration.test.tspackages/cli/src/integration-tests/provider-switching.integration.test.tspackages/cli/src/integration-tests/security.integration.test.tspackages/cli/src/integration-tests/test-utils.test.tspackages/cli/src/integration-tests/test-utils.tspackages/cli/src/nonInteractiveCli.test.tspackages/cli/src/providers/credentialPrecedence.test.tspackages/cli/src/providers/logging/git-stats-service-impl.tspackages/cli/src/providers/logging/git-stats.tspackages/cli/src/providers/logging/multi-provider-logging.integration.test.tspackages/cli/src/providers/oauth-provider-registration.tspackages/cli/src/providers/provider-gemini-switching.test.tspackages/cli/src/providers/provider-switching.integration.test.tspackages/cli/src/runtime/providerMutations.tspackages/cli/src/runtime/providerSwitch.tspackages/cli/src/runtime/runtimeAccessors.spec.tspackages/cli/src/runtime/runtimeLifecycle.spec.tspackages/cli/src/runtime/settingsResolver.tspackages/cli/src/services/CommandService.tspackages/cli/src/services/FileCommandLoader.test.tspackages/cli/src/services/FileCommandLoader.tspackages/cli/src/services/McpPromptLoader.test.tspackages/cli/src/services/McpPromptLoader.tspackages/cli/src/services/prompt-processors/argumentProcessor.tspackages/cli/src/services/prompt-processors/shellProcessor.test.tspackages/cli/src/services/prompt-processors/shellProcessor.tspackages/cli/src/services/prompt-processors/types.tspackages/cli/src/services/todo-continuation/todoContinuationService.spec.tspackages/cli/src/services/todo-continuation/todoContinuationService.tspackages/cli/src/services/types.tspackages/cli/src/storage/ConversationStorage.test.tspackages/cli/src/test-utils/customMatchers.tspackages/cli/src/test-utils/mockCommandContext.tspackages/cli/src/test-utils/responsive-testing.test.tsxpackages/cli/src/test-utils/responsive-testing.tsxpackages/cli/src/ui/App.test.tsxpackages/cli/src/ui/AppContainer.tsxpackages/cli/src/ui/AppContainerRuntime.tsxpackages/cli/src/ui/IdeIntegrationNudge.tsxpackages/cli/src/ui/colors.tspackages/cli/src/ui/commands/aboutCommand.test.tspackages/cli/src/ui/commands/aboutCommand.tspackages/cli/src/ui/commands/authCommand.codex.test.tspackages/cli/src/ui/commands/authCommand.test.tspackages/cli/src/ui/commands/authCommand.tspackages/cli/src/ui/commands/baseurlCommand.tspackages/cli/src/ui/commands/chatCommand.test.tspackages/cli/src/ui/commands/chatCommand.tspackages/cli/src/ui/commands/clearCommand.test.tspackages/cli/src/ui/commands/compressCommand.test.tspackages/cli/src/ui/commands/compressCommand.tspackages/cli/src/ui/commands/copyCommand.test.tspackages/cli/src/ui/commands/copyCommand.tspackages/cli/src/ui/commands/debugCommands.tspackages/cli/src/ui/commands/diagnosticsCommand.tspackages/cli/src/ui/commands/directoryCommand.test.tsxpackages/cli/src/ui/commands/directoryCommand.tsxpackages/cli/src/ui/commands/dumpcontextCommand.tspackages/cli/src/ui/commands/helpCommand.tspackages/cli/src/ui/commands/ideCommand.test.tspackages/cli/src/ui/commands/initCommand.tspackages/cli/src/ui/commands/keyCommand.tspackages/cli/src/ui/commands/keyfileCommand.tspackages/cli/src/ui/commands/loggingCommand.tspackages/cli/src/ui/commands/logoutCommand.tspackages/cli/src/ui/commands/lspCommand.tspackages/cli/src/ui/commands/mcpCommand.test.tspackages/cli/src/ui/commands/mcpCommand.tspackages/cli/src/ui/commands/memoryCommand.test.tspackages/cli/src/ui/commands/modelCommand.tspackages/cli/src/ui/commands/permissionsCommand.tspackages/cli/src/ui/commands/privacyCommand.tspackages/cli/src/ui/commands/profileCommand.tspackages/cli/src/ui/commands/providerCommand.tspackages/cli/src/ui/commands/restoreCommand.test.tspackages/cli/src/ui/commands/restoreCommand.tspackages/cli/src/ui/commands/schema/types.tspackages/cli/src/ui/commands/setCommand.tspackages/cli/src/ui/commands/settingsCommand.tspackages/cli/src/ui/commands/setupCommand.tspackages/cli/src/ui/commands/statsCommand.tspackages/cli/src/ui/commands/subagentCommand.tspackages/cli/src/ui/commands/terminalSetupCommand.test.tspackages/cli/src/ui/commands/terminalSetupCommand.tspackages/cli/src/ui/commands/test/subagentCommand.test.tspackages/cli/src/ui/commands/test/useSlashCompletion.schema.test.tspackages/cli/src/ui/commands/themeCommand.tspackages/cli/src/ui/commands/toolkeyCommand.tspackages/cli/src/ui/commands/toolkeyfileCommand.tspackages/cli/src/ui/commands/vimCommand.tspackages/cli/src/ui/components/AboutBox.tsxpackages/cli/src/ui/components/AnsiOutput.tsxpackages/cli/src/ui/components/AppHeader.tsxpackages/cli/src/ui/components/AuthDialog.tsxpackages/cli/src/ui/components/AuthInProgress.tsxpackages/cli/src/ui/components/AutoAcceptIndicator.tsxpackages/cli/src/ui/components/BucketAuthConfirmation.tsxpackages/cli/src/ui/components/CacheStatsDisplay.tsxpackages/cli/src/ui/components/Composer.tsxpackages/cli/src/ui/components/ConsoleSummaryDisplay.tsxpackages/cli/src/ui/components/ContextIndicator.ui.test.tsxpackages/cli/src/ui/components/DebugProfiler.tsxpackages/cli/src/ui/components/DetailedMessagesDisplay.tsxpackages/cli/src/ui/components/EditorSettingsDialog.tsxpackages/cli/src/ui/components/ErrorBoundary.tsxpackages/cli/src/ui/components/FolderTrustDialog.tsxpackages/cli/src/ui/components/GeminiRespondingSpinner.tsxpackages/cli/src/ui/components/Header.tsxpackages/cli/src/ui/components/Help.tsxpackages/cli/src/ui/components/HistoryItemDisplay.tsxpackages/cli/src/ui/components/HookStatusDisplay.tsxpackages/cli/src/ui/components/InputPrompt.paste.spec.tsxpackages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/LBStatsDisplay.tsxpackages/cli/src/ui/components/LayoutManager.tsxpackages/cli/src/ui/components/LoadProfileDialog.tsxpackages/cli/src/ui/components/LoadingIndicator.tsxpackages/cli/src/ui/components/LoggingDialog.tsxpackages/cli/src/ui/components/MemoryUsageDisplay.tsxpackages/cli/src/ui/components/ModelDialog.tsxpackages/cli/src/ui/components/OAuthCodeDialog.tsxpackages/cli/src/ui/components/PermissionsModifyTrustDialog.test.tsxpackages/cli/src/ui/components/PermissionsModifyTrustDialog.tsxpackages/cli/src/ui/components/PrepareLabel.tsxpackages/cli/src/ui/components/ProfileCreateWizard/AdvancedParamsStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/AuthenticationStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/BaseUrlConfigStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/NavigationMenu.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ProfileSaveStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ProfileSuccessSummary.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ProviderSelectStep.tsxpackages/cli/src/ui/components/ProfileCreateWizard/TextInput.tsxpackages/cli/src/ui/components/ProfileCreateWizard/index.tsxpackages/cli/src/ui/components/ProfileDetailDialog.tsxpackages/cli/src/ui/components/ProfileInlineEditor.tsxpackages/cli/src/ui/components/SecureKeyInput.tsxpackages/cli/src/ui/components/SessionBrowserDialog.tsxpackages/cli/src/ui/components/SessionSummaryDisplay.test.tsxpackages/cli/src/ui/components/SessionSummaryDisplay.tsxpackages/cli/src/ui/components/ShellModeIndicator.tsxpackages/cli/src/ui/components/StatsDisplay.test.tsxpackages/cli/src/ui/components/StatusDisplay.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentCreationWizard.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentDeleteDialog.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentEditForm.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentListMenu.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentMainMenu.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentManagerDialog.tsxpackages/cli/src/ui/components/SubagentManagement/SubagentShowView.tsxpackages/cli/src/ui/components/ThemeDialog.test.tsxpackages/cli/src/ui/components/ThemeDialog.tsxpackages/cli/src/ui/components/ThemedGradient.tsxpackages/cli/src/ui/components/Tips.tsxpackages/cli/src/ui/components/TodoPanel.responsive.test.tsxpackages/cli/src/ui/components/TodoPanel.semantic.test.tsxpackages/cli/src/ui/components/TodoPanel.tsxpackages/cli/src/ui/components/ToolStatsDisplay.test.tsxpackages/cli/src/ui/components/ToolStatsDisplay.tsxpackages/cli/src/ui/components/ToolsDialog.tsx
Comment on lines
+456
to
+458
| !this.skipFinalTrueAfterInlineEdit.value | ||
| ) { | ||
| this.skipFinalTrueAfterInlineEdit = false; | ||
| this.skipFinalTrueAfterInlineEdit.value = false; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the redundant skip-flag write.
Inside if (!this.skipFinalTrueAfterInlineEdit.value), assigning this.skipFinalTrueAfterInlineEdit.value = false is a no-op and adds noise.
✂️ Proposed cleanup
if (
isAwaitingApproval &&
allPendingAreStable &&
!this.skipFinalTrueAfterInlineEdit.value
) {
- this.skipFinalTrueAfterInlineEdit.value = false;
-
// We don't need to send another message, just a final status update.
this.setTaskStateAndPublishUpdate(
'input-required',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/a2a-server/src/agent/task.ts` around lines 456 - 458, The
conditional block checks if (!this.skipFinalTrueAfterInlineEdit.value) then
assigns this.skipFinalTrueAfterInlineEdit.value = false, which is a redundant
no-op; remove the assignment inside the if so the code only relies on the
condition, i.e., delete the write to this.skipFinalTrueAfterInlineEdit.value
while keeping the surrounding logic intact (look for the conditional using
skipFinalTrueAfterInlineEdit.value in the task handling code).
Comment on lines
+73
to
75
| expect(providers).toStrictEqual( | ||
| expect.arrayContaining(['provider1', 'provider2']), | ||
| ); |
Contributor
There was a problem hiding this comment.
Clarify assertion intent: toStrictEqual with arrayContaining is semantically contradictory.
The combination of toStrictEqual (which implies exact matching) with expect.arrayContaining (which allows extra elements) creates semantic confusion. Based on the test name "without auto-registering gemini", the intent appears to be verifying the list contains exactly the two registered providers and nothing else.
🎯 Proposed fix for exact matching
- expect(providers).toStrictEqual(
- expect.arrayContaining(['provider1', 'provider2']),
- );
+ expect(providers).toStrictEqual(['provider1', 'provider2']);If the test should allow additional providers (contradicting the test name), revert to the original toEqual with arrayContaining:
- expect(providers).toStrictEqual(
- expect.arrayContaining(['provider1', 'provider2']),
- );
+ expect(providers).toEqual(
+ expect.arrayContaining(['provider1', 'provider2']),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/providers/provider-switching.integration.test.ts` around
lines 73 - 75, The assertion mixes toStrictEqual with expect.arrayContaining
which is contradictory; update the test "without auto-registering gemini" to
assert exact equality instead of a partial match: replace
expect(providers).toStrictEqual(expect.arrayContaining(['provider1','provider2']))
with an exact-array assertion such as
expect(providers).toStrictEqual(['provider1', 'provider2']) (or use toEqual if
order is not guaranteed) so the variable providers is verified to contain
exactly those two entries.
…ports (Fixes #1569) The initial Tier A.2 commit (406de04) deferred 36 CLI files where the default autofix (fixStyle: separate-type-imports) converted runtime-used enum imports to "import type", producing TS1361 errors. Fix: apply the same rule with fixStyle: inline-type-imports. Each specifier is prefixed with the type keyword while the import declaration stays a value-import, so enums used as values continue to compile. Applied via: npx eslint --fix --rule '{"@typescript-eslint/consistent-type-imports": ["error", {"prefer": "type-imports", "fixStyle": "inline-type-imports"}]}' Verification: - typecheck: 0 errors - build: clean - tests: 9378 passed / 1 pre-existing flake (subagentRuntimeSetup.test.ts, confirmed also failing on main) - prettier: clean - lint: 10764 -> 10696 total warnings (-68); consistent-type-imports 384 -> 316 warnings (-68) The remaining 316 cti warnings are in other files and in a different sub-case (import() type annotations), to be addressed in later tiers.
Three import-block conflicts resolved:
- packages/core/src/core/geminiChat.ts
Kept HEAD's `import type { PerformCompressionResult }` and added
main's `import type { Config }` (Config is type-only here).
- packages/core/src/core/subagent.ts
Kept HEAD's `import type { Config }` (regression on main would have
made it a value import). Dropped HEAD's TURN_STREAM_IDLE_TIMEOUT_MS
import now that main replaced that usage with
resolveStreamIdleTimeoutMs(this.config).
- packages/core/src/core/turn.test.ts
Kept HEAD's `import type { GenerateContentResponse, Part, Content,
FinishReason }` (all used as types) and added main's
`import { DEFAULT_STREAM_IDLE_TIMEOUT_MS }`.
Verified: npm run build (clean), npm run typecheck (0 errors).
#1569) - Add ts-morph codemod (scripts/codemod-import-type-annotations.mjs) to rewrite import() type annotations and hoist type-only named imports to import type declarations. - Disable consistent-type-imports in the vitest test-file block; this is the correct scope for the rule and removes ~263 noisy warnings from test files. - Apply codemod across 22 source files in packages/core and packages/cli (auth, config, extensions, UI layouts/contexts, hooks, compression, subagent, ide-client, RetryOrchestrator, shellExecutionService, nonInteractiveToolExecutor, configTypes, GeminiProvider). - Hand-fix codemod aftermath: - Circular self-aliases in lspIntegration.ts and token-access-coordinator.ts - Name collision in settingsIntegration.ts (ExtensionSetting -> ExtensionConfigSetting alias) plus Array<X> -> X[] rewrites - Global-name shadowing in ide-client.ts (undici RequestInit -> UndiciRequestInit alias) - Array<X> -> X[] in UIStateContext.tsx and cli/ui/types.ts - typeof import() pattern preserved in profile-utils.ts, GeminiProvider.ts, and wrapped with eslint-disable in test-utils/runtime.ts (vitest devDep must not be statically imported from src). - Revert value import for ToolRegistry in subagent.test.ts that an earlier autofix wrongly converted to a type-only import; merge-from-main reintroduced 4 new ToolRegistry(...) call sites. - Auto-format sweep brought in prettier-only changes to turn.test.ts, RetryOrchestrator.ts, and shellExecutionService.ts. Result: lint reports 0 errors, 10435 warnings (from 10727 baseline). Typecheck clean; build clean; full test suite passes except two pre-existing failures unrelated to this change: - useSlashCompletion > git-aware filtering (reproducible on stashed clean branch) - ToolResultDisplay > truncates extremely long strings (passes in isolation; load-related flake) Rule stays at warn for now; will be promoted to error once the remaining ~26 consistent-type-imports warnings are hand-patched in a follow-up.
- Widen vitest eslint block to cover *.spec.{ts,tsx} alongside *.test.{ts,tsx}
- Add targeted eslint-disable for two legitimate vitest patterns surfaced
by the widened glob (pre-advance-timer expect capture + aliased it/it.skip)
- Promote @typescript-eslint/consistent-type-imports from warn to error now
that the rule is clean repo-wide.
acoliver
changed the title
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)
packages/cli/src/config/extensions/settingsIntegration.ts (1)
9-17:⚠️ Potential issue | 🟡 MinorConsolidate
ExtensionSettingtype imports to use a single source.The file imports
ExtensionSettingfrom two different modules with different definitions:
- Line 11: from
./extensionSettings.js— derived fromExtensionSettingSchema(schema fields: name, description, envVar, sensitive)- Line 16: from
../extension.js(aliased asExtensionConfigSetting) — interface with an additionalrequired?: booleanfieldIn
getMissingSettings(), items loaded as the first type are pushed into an array typed as the second type. While TypeScript accepts this due to structural typing, maintaining two type names for nearly-identical concepts creates confusion and maintenance risk. Therequiredfield in the interface type is unused in this file and semantically misaligned with the schema-derived type.Consider using the schema-based
ExtensionSettingtype consistently throughout this file, or align both type definitions if therequiredfield has intentional semantics elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/config/extensions/settingsIntegration.ts` around lines 9 - 17, The file imports two different ExtensionSetting types causing confusion; update getMissingSettings and any local type annotations to use the schema-derived ExtensionSetting from ./extensionSettings.js (and remove the aliased import ExtensionConfigSetting from ../extension.js), so the array typed for missing settings and any pushes/reference use the single schema-based type (also remove the now-unused import symbol EXTENSIONS_CONFIG_FILENAME_FALLBACK if it was only pulled in alongside the aliased type). Ensure you update any type annotations, function signatures, and variable declarations referencing ExtensionConfigSetting to the schema ExtensionSetting and delete the duplicate import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/ui/commands/schema/argumentResolver.test.ts`:
- Around line 784-785: Replace the loose toMatchObject assertion with a strict
equality on normalized forms so extra fields are caught: normalize both schema
and originalSchema (e.g., by serializing/deserializing to drop undefineds) and
assert they are exactly equal using toStrictEqual; update the test that
currently uses expect(schema).toMatchObject(originalSchema) to compare
JSON-normalized schema and originalSchema (reference symbols: schema,
originalSchema, createCompletionHandler).
---
Outside diff comments:
In `@packages/cli/src/config/extensions/settingsIntegration.ts`:
- Around line 9-17: The file imports two different ExtensionSetting types
causing confusion; update getMissingSettings and any local type annotations to
use the schema-derived ExtensionSetting from ./extensionSettings.js (and remove
the aliased import ExtensionConfigSetting from ../extension.js), so the array
typed for missing settings and any pushes/reference use the single schema-based
type (also remove the now-unused import symbol
EXTENSIONS_CONFIG_FILENAME_FALLBACK if it was only pulled in alongside the
aliased type). Ensure you update any type annotations, function signatures, and
variable declarations referencing ExtensionConfigSetting to the schema
ExtensionSetting and delete the duplicate import.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a592287-638b-416f-9aa3-664270acdf84
📒 Files selected for processing (38)
eslint.config.jspackages/cli/src/__tests__/sessionBrowserE2E.spec.tspackages/cli/src/auth/BucketFailoverHandlerImpl.spec.tspackages/cli/src/auth/__tests__/OAuthBucketManager.spec.tspackages/cli/src/auth/__tests__/anthropic-oauth-provider.fallback.spec.tspackages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.tspackages/cli/src/auth/__tests__/auth-status-service.spec.tspackages/cli/src/auth/__tests__/behavioral/bridge-ui.behavioral.spec.tspackages/cli/src/auth/__tests__/behavioral/subagent-isolation.behavioral.spec.tspackages/cli/src/auth/__tests__/multi-bucket-auth.spec.tspackages/cli/src/auth/__tests__/provider-registry.spec.tspackages/cli/src/auth/__tests__/provider-usage-info.spec.tspackages/cli/src/auth/__tests__/token-access-coordinator.spec.tspackages/cli/src/auth/anthropic-oauth-provider.local-flow.spec.tspackages/cli/src/auth/anthropic-oauth-provider.no-refresh-on-gettoken.spec.tspackages/cli/src/auth/auth-flow-orchestrator.tspackages/cli/src/auth/codex-oauth-provider.spec.tspackages/cli/src/auth/local-oauth-callback.spec.tspackages/cli/src/auth/oauth-manager.auth-lock.spec.tspackages/cli/src/auth/oauth-manager.failover-wiring.spec.tspackages/cli/src/auth/oauth-manager.issue1468.spec.tspackages/cli/src/auth/oauth-manager.runtime-messagebus.spec.tspackages/cli/src/auth/oauth-manager.spec.tspackages/cli/src/auth/oauth-manager.wiring.spec.tspackages/cli/src/auth/profile-utils.tspackages/cli/src/auth/token-access-coordinator.tspackages/cli/src/auth/types.tspackages/cli/src/config/extensions/settingsIntegration.tspackages/cli/src/integration-tests/__tests__/oauth-buckets.integration.spec.tspackages/cli/src/nonInteractiveCli.tspackages/cli/src/runtime/__tests__/profileApplication.bucket-failover.spec.tspackages/cli/src/runtime/providerMutations.spec.tspackages/cli/src/runtime/providerSwitch.spec.tspackages/cli/src/runtime/runtimeRegistry.spec.tspackages/cli/src/runtime/runtimeSettings.proactive-wiring.lb.spec.tspackages/cli/src/services/__tests__/performResume.spec.tspackages/cli/src/services/todo-continuation/todoContinuationService.spec.tspackages/cli/src/ui/commands/schema/argumentResolver.test.ts
Comment on lines
+784
to
+785
| // Use toMatchObject since JSON.stringify drops undefined properties | ||
| expect(schema).toMatchObject(originalSchema); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
In Vitest 3.x, what are the behavioral differences between expect(a).toMatchObject(b) and expect(a).toStrictEqual(b), specifically regarding extra object properties and nested array/object equality?
💡 Result:
In Vitest 3.x, expect(a).toMatchObject(b) and expect(a).toStrictEqual(b) differ in handling extra object properties and nested equality as follows: Extra object properties: - toMatchObject(b): Passes if a contains all properties and values from b (recursively), ignoring extra properties in a. Designed for partial/subset matching. - toStrictEqual(b): Fails if a has any extra properties not in b. Requires exact match including no extras. Nested array/object equality: Both use deep recursive comparison similar to toEqual, but toStrictEqual adds strictness: - Checks for undefined keys: {a: undefined, b: 2} ≠ {b: 2}. - Array sparseness: [, 1] ≠ [undefined, 1]. - Object types/prototypes: Class instance ≠ plain object with same fields. toMatchObject applies subset logic recursively to nested objects/arrays in b, allowing extras in corresponding nested parts of a. No behavioral changes specific to these matchers in Vitest 3.x release notes; behaviors match Jest/Chai foundations with documented differences.
Citations:
- 1: https://main.vitest.dev/guide/learn/matchers
- 2: https://vitest.dev/api/expect.html
- 3: https://v3.vitest.dev/api/expect.html
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
wc -l argumentResolver.test.ts && \
sed -n '750,810p' argumentResolver.test.tsRepository: vybestack/llxprt-code
Length of output: 2573
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '700,760p' argumentResolver.test.tsRepository: vybestack/llxprt-code
Length of output: 2380
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '1,50p' argumentResolver.test.ts | head -40Repository: vybestack/llxprt-code
Length of output: 1037
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '35,80p' argumentResolver.test.tsRepository: vybestack/llxprt-code
Length of output: 1438
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
grep -n "createCompletionHandler" index.ts | head -20Repository: vybestack/llxprt-code
Length of output: 141
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '534,620p' index.tsRepository: vybestack/llxprt-code
Length of output: 1397
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '534,620p' index.ts && \
echo "---" && \
grep -n "suggestForValue\|computeHintForValue\|suggestForLiterals\|inferLiteralHint" index.ts | head -10Repository: vybestack/llxprt-code
Length of output: 1797
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '430,520p' index.tsRepository: vybestack/llxprt-code
Length of output: 2671
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
grep -n "function resolveActiveStep\|export.*resolveActiveStep" index.tsRepository: vybestack/llxprt-code
Length of output: 96
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '179,250p' index.tsRepository: vybestack/llxprt-code
Length of output: 1837
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
grep -n "function mergeSchemas\|function gatherLiteralGroup" index.tsRepository: vybestack/llxprt-code
Length of output: 155
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '67,110p' index.tsRepository: vybestack/llxprt-code
Length of output: 1340
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '250,330p' index.tsRepository: vybestack/llxprt-code
Length of output: 2493
🏁 Script executed:
cd packages/cli/src/ui/commands/schema && \
sed -n '395,430p' index.tsRepository: vybestack/llxprt-code
Length of output: 843
Consider a stricter assertion to catch potential field additions to the schema object.
The current toMatchObject comparison only verifies that schema contains all properties from originalSchema, allowing extra fields that might be introduced by future changes. While createCompletionHandler doesn't currently mutate the schema, using toStrictEqual on normalized forms would be more defensive:
Suggested fix
- // Property 2: Schema should not be mutated
- // Use toMatchObject since JSON.stringify drops undefined properties
- expect(schema).toMatchObject(originalSchema);
+ // Property 2: Schema should not be mutated
+ // Compare normalized forms strictly to catch field additions
+ expect(JSON.parse(JSON.stringify(schema))).toStrictEqual(
+ originalSchema,
+ );📝 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
| // Use toMatchObject since JSON.stringify drops undefined properties | |
| expect(schema).toMatchObject(originalSchema); | |
| // Property 2: Schema should not be mutated | |
| // Compare normalized forms strictly to catch field additions | |
| expect(JSON.parse(JSON.stringify(schema))).toStrictEqual( | |
| JSON.parse(JSON.stringify(originalSchema)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/commands/schema/argumentResolver.test.ts` around lines
784 - 785, Replace the loose toMatchObject assertion with a strict equality on
normalized forms so extra fields are caught: normalize both schema and
originalSchema (e.g., by serializing/deserializing to drop undefineds) and
assert they are exactly equal using toStrictEqual; update the test that
currently uses expect(schema).toMatchObject(originalSchema) to compare
JSON-normalized schema and originalSchema (reference symbols: schema,
originalSchema, createCompletionHandler).
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
What changed
Verification