Fix #1748: BeforeToolSelectionHook: empty allowedFunctionNames array treated as no restriction#1910
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent 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). (5)
🧰 Additional context used🧠 Learnings (31)📓 Common learnings📚 Learning: 2026-03-21T17:07:21.182ZApplied to files:
📚 Learning: 2026-03-21T15:18:11.019ZApplied to files:
📚 Learning: 2026-03-26T02:22:19.732ZApplied to files:
📚 Learning: 2026-03-19T23:27:48.689ZApplied to files:
📚 Learning: 2026-03-26T03:06:11.693ZApplied to files:
📚 Learning: 2026-03-26T03:04:09.288ZApplied to files:
📚 Learning: 2026-03-22T04:06:53.600ZApplied to files:
📚 Learning: 2026-03-26T21:40:39.044ZApplied to files:
📚 Learning: 2026-03-21T15:18:51.729ZApplied to files:
📚 Learning: 2026-03-26T03:04:10.186ZApplied to files:
📚 Learning: 2026-03-27T01:00:28.649ZApplied to files:
📚 Learning: 2026-02-06T15:52:42.315ZApplied to files:
📚 Learning: 2026-02-15T21:44:56.598ZApplied to files:
📚 Learning: 2026-02-16T19:18:56.265ZApplied to files:
📚 Learning: 2026-03-21T17:07:10.889ZApplied to files:
📚 Learning: 2026-03-31T02:12:43.093ZApplied to files:
📚 Learning: 2026-03-21T15:18:11.794ZApplied to files:
📚 Learning: 2026-03-21T17:07:28.742ZApplied to files:
📚 Learning: 2026-03-21T15:19:53.168ZApplied to files:
📚 Learning: 2026-03-25T22:22:12.030ZApplied to files:
📚 Learning: 2026-03-26T02:05:51.733ZApplied to files:
📚 Learning: 2026-03-26T02:05:48.262ZApplied to files:
📚 Learning: 2026-02-28T23:18:15.496ZApplied to files:
📚 Learning: 2026-04-22T08:28:33.938ZApplied to files:
📚 Learning: 2026-02-26T19:06:23.993ZApplied to files:
📚 Learning: 2026-03-27T00:46:42.630ZApplied to files:
📚 Learning: 2026-03-26T02:06:06.881ZApplied to files:
📚 Learning: 2026-03-27T01:00:29.058ZApplied to files:
📚 Learning: 2026-04-23T23:33:02.521ZApplied to files:
📚 Learning: 2026-04-22T08:29:29.173ZApplied to files:
🔇 Additional comments (3)
Summary by CodeRabbit
WalkthroughThis PR tightens tool-selection filtering in DirectMessageProcessor and StreamProcessor to require Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Contributor
LLxprt PR Review – PR #1910Issue AlignmentIssue #1748 correctly identifies the bug: The fix changes both if (allowedFunctions?.length)to: if (Array.isArray(allowedFunctions))This correctly applies filtering whenever the property is an array—including empty arrays—while preserving the no-filter behavior when the property is omitted or undefined. Issue requirement satisfied. Side EffectsMinimal risk. The change is surgical and isolated to two guard conditions. No shared state, config changes, or downstream mutations. The hook aggregator ( Code QualityCorrect and idiomatic. Using Tests and CoverageCoverage impact: Increase. A new 184-line test file
The tests use VerdictReady. The fix is correct, minimal, and well-tested. Issue #1748 is resolved. The test suite covers the behavioral cases for both processors including the empty-array edge case that was the original bug. |
github-actions
Bot
added
the
maintainer:e2e:ok
![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: 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/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts`:
- Around line 121-171: Add a regression test inside the existing describe.each
block that passes a non-array value (e.g., a string like 'beta') for the
allowedFunctionNames option to applyToolSelectionHook and asserts that the
returned tools equal the original toolsFromConfig; specifically, add an it(...)
that calls applyToolSelectionHook({ allowedFunctionNames: 'beta' },
toolsFromConfig) (using the existing createTools helper) and
expect(result).toStrictEqual(toolsFromConfig) to confirm the runtime array guard
leaves tools unchanged when allowedFunctionNames is malformed.
🪄 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: 8679634a-ad75-4709-8d4f-4489908dde2e
📒 Files selected for processing (3)
packages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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). (5)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:322-336
Timestamp: 2026-03-21T17:07:21.182Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `if (allowedFunctions?.length)` guard inside `_applyToolSelectionHook` (around lines 323-337) is intentional preserved behavior from the original `geminiChat.ts` on `main`. An empty `allowedFunctionNames` array (`[]`) is treated as "no restriction" rather than "deny all" — this is the upstream semantic. Do not flag this as a bug in decomposition PRs; any behavioral change to honor an empty list as a deny-all policy would be a feature modification, not a refactoring. A follow-up issue (`#1748` in vybestack/llxprt-code) has been filed to address this upstream bug.
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: 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: 1907
File: packages/cli/src/ui/App.test.tsx:1583-1586
Timestamp: 2026-04-23T23:33:15.915Z
Learning: Repo vybestack/llxprt-code (PR `#1907`): For tests like packages/cli/src/ui/App.test.tsx, maintainers prefer titles that describe the feature under test (e.g., “auto-send queued messages”), even if the specific assertion covers a guard path (e.g., empty queue ⇒ no auto-send). Renaming for perfect alignment is optional and out of scope for lint-enforcement batches.
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.
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.
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: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:360-366
Timestamp: 2026-03-21T15:18:11.019Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `_handleBeforeModelHook` intentionally returns only `modifiedContents` (i.e., `modifiedRequest.contents`) from the BeforeModel hook result, dropping any hook changes to `tools`, `config`, or other request fields. This is faithfully preserved from the original `geminiChat.ts` behavior on the main branch and is not a regression from the decomposition. Do not flag this partial-field forwarding as a bug in future reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1899
File: packages/core/src/core/geminiChat.runtime.test.ts:837-1007
Timestamp: 2026-04-12T05:38:21.192Z
Learning: In `packages/core/src/core/geminiChat.runtime.test.ts` (vybestack/llxprt-code PR `#1899`), the `stream idle timeout behavioral tests for TurnProcessor and DirectMessageProcessor` describe block intentionally tests only config plumbing and resolver integration (verifying `chat.getConfig()` surfaces the ephemeral setting and `resolveStreamIdleTimeoutMs` returns the correct value). The actual stalled-stream watchdog abort behavior is covered in dedicated consumer suites, e.g., `turn.test.ts` line ~787. Do not flag these as missing stalled-stream behavioral coverage — adding full stalled-stream tests for every consumer variant is considered scope expansion.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:79-88
Timestamp: 2026-03-21T15:19:53.168Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `logApiRequest()` is intentionally called before `_applyPreSendHooks()` in `generateDirectMessage()`. This preserves the original `geminiChat.ts` behavior where `this._logApiRequest()` was called at line ~1195 with the initial requestContents, prior to BeforeModel/BeforeToolSelection hooks being applied. Logging post-hook state would be an improvement but is not a regression. Do not flag this ordering as a bug in decomposition reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:430-467
Timestamp: 2026-03-21T15:18:11.794Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. Do not flag this as a hook-sanitization bypass — it is established upstream behavior, and any change would be a behavioral modification, not a decomposition fix.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:15.661Z
Learning: In `packages/core/src/core/ConversationManager.ts`, `packages/core/src/core/TurnProcessor.ts`, `packages/core/src/core/StreamProcessor.ts`, and `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`, commit 870e85a91), `makePositionMatcher()` was hoisted outside all per-Content loops so that a single position matcher is shared across the entire batch. This supersedes the prior preserved-behavior pattern (original `geminiChat.ts` called it per-Content). The unmatched-tool queue is now consumed in order across all items in a batch, which is the correct behavior.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:431-468
Timestamp: 2026-03-21T17:07:28.742Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. The pre-existing issue where the pre-hook aggregatedText is used after an AfterModel hook rewrites the response is tracked in issue `#1749` for a dedicated fix. Do not flag this as a hook-sanitization bypass in decomposition reviews — it is established upstream behavior.
📚 Learning: 2026-03-21T17:07:21.182Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:322-336
Timestamp: 2026-03-21T17:07:21.182Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `if (allowedFunctions?.length)` guard inside `_applyToolSelectionHook` (around lines 323-337) is intentional preserved behavior from the original `geminiChat.ts` on `main`. An empty `allowedFunctionNames` array (`[]`) is treated as "no restriction" rather than "deny all" — this is the upstream semantic. Do not flag this as a bug in decomposition PRs; any behavioral change to honor an empty list as a deny-all policy would be a feature modification, not a refactoring. A follow-up issue (`#1748` in vybestack/llxprt-code) has been filed to address this upstream bug.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-02-28T23:18:15.496Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1642
File: packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx:528-578
Timestamp: 2026-02-28T23:18:15.496Z
Learning: In `packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx`, tests use mocked child components (ToolConfirmationMessage, ToolMessage) for isolation and consistency with upstream gemini-cli patterns. Tests for permanent tool approval settings (`enablePermanentToolApproval`) use this mock-based approach and pass successfully as part of the cherry-picked upstream test suite.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-21T15:18:11.019Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:360-366
Timestamp: 2026-03-21T15:18:11.019Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `_handleBeforeModelHook` intentionally returns only `modifiedContents` (i.e., `modifiedRequest.contents`) from the BeforeModel hook result, dropping any hook changes to `tools`, `config`, or other request fields. This is faithfully preserved from the original `geminiChat.ts` behavior on the main branch and is not a regression from the decomposition. Do not flag this partial-field forwarding as a bug in future reviews.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-27T01:00:29.058Z
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.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-27T00:46:42.630Z
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.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-02-26T19:06:23.993Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1627
File: integration-tests/run_shell_command.test.ts:550-557
Timestamp: 2026-02-26T19:06:23.993Z
Learning: In `integration-tests/run_shell_command.test.ts`, the "rejects invalid shell expressions" test intentionally does not assert `toolRequest.success === false` for shell syntax errors because issue `#1625` tracks that `run_shell_command` currently reports `success: true` for commands that fail with non-zero exit codes (it only sets `result.error` on spawn failures, not non-zero exits). The test uses FakeProvider to script model responses but executes the real tool, so asserting `success === false` would fail until `#1625` is resolved. The test correctly verifies the tool was invoked with the invalid syntax and the model responded with FAIL.
<!--
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/StreamProcessor.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-02-15T21:44:56.598Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1407
File: packages/core/src/core/geminiChatHookTriggers.ts:56-65
Timestamp: 2026-02-15T21:44:56.598Z
Learning: Enforce the canonical speaker-to-role mapping used by GeminiChat hooks: in IContent.speaker, which is strictly typed as 'human | ai | tool' (no 'system'), map 'human' to the 'user' role, 'ai' to the 'model' role, and 'tool' to the 'user' role in all hook payloads. This pattern should be applied across related hook files within packages/core/src/core/ (not just the single file) to ensure consistent role assignment.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-02-16T19:18:56.265Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1436
File: packages/core/src/core/nonInteractiveToolExecutor.ts:0-0
Timestamp: 2026-02-16T19:18:56.265Z
Learning: Guideline: In the core scheduler architecture, the system runs in a single mode at a time—either interactive or non-interactive, never both on the same scheduler instance. Non-interactive (CLI one-shot) runs without any interactive session; interactive mode subagents run within the parent's interactive context and inherit its mode. When reviewing code, ensure non-interactive tool executions (e.g., in nonInteractiveToolExecutor.ts) create and use a fresh completionResolver per executeToolCall, and that there is no race with interactive sessions since they cannot coexist on the same scheduler instance. This pattern applies across files in packages/core/src/core/. Only apply to relevant files, not globally.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-22T04:06:53.600Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:06:53.600Z
Learning: When computing `lastPromptTokenCount` (e.g., in the streaming path like `_convertIContentStream`/equivalent), ensure it includes the full prompt token footprint: `lastPromptTokenCount = promptTokens + cache_read_input_tokens + cache_creation_input_tokens`. Do not use `promptTokens` alone, because cached context would otherwise cause `CompressionHandler.shouldCompress()` to underestimate context usage and may incorrectly suppress needed compression. Keep this combined computation consistent with the non-streaming path (e.g., `TurnProcessor._executeProviderCall`), and do not treat the presence of cache-token additions as redundant—both token types are required for correctness when cached context is active.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-21T15:18:11.794Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:430-467
Timestamp: 2026-03-21T15:18:11.794Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. Do not flag this as a hook-sanitization bypass — it is established upstream behavior, and any change would be a behavioral modification, not a decomposition fix.
Applied to files:
packages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T17:07:28.742Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:431-468
Timestamp: 2026-03-21T17:07:28.742Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. The pre-existing issue where the pre-hook aggregatedText is used after an AfterModel hook rewrites the response is tracked in issue `#1749` for a dedicated fix. Do not flag this as a hook-sanitization bypass in decomposition reviews — it is established upstream behavior.
Applied to files:
packages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T15:19:53.168Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:79-88
Timestamp: 2026-03-21T15:19:53.168Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `logApiRequest()` is intentionally called before `_applyPreSendHooks()` in `generateDirectMessage()`. This preserves the original `geminiChat.ts` behavior where `this._logApiRequest()` was called at line ~1195 with the initial requestContents, prior to BeforeModel/BeforeToolSelection hooks being applied. Logging post-hook state would be an improvement but is not a regression. Do not flag this ordering as a bug in decomposition reviews.
Applied to files:
packages/core/src/core/DirectMessageProcessor.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/core/src/core/DirectMessageProcessor.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/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T17:07:10.889Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:10.889Z
Learning: In these core processor modules, ensure `makePositionMatcher()` is created once and reused across the entire batch (i.e., hoist it outside any per-`content` loops) rather than rebuilding it for each content item. Also ensure the `unmatched-tool` queue is consumed in-order across all batch items (not reset/handled independently per item), since that preserves correct batch-wide behavior.
Applied to files:
packages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.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/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-21T03:05:57.496Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:600-660
Timestamp: 2026-03-21T03:05:57.496Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), both classes have a `_enrichSchemaDepthError` method with similar names but different data sources: `TurnProcessor._enrichSchemaDepthError` reads from cached `this.generationConfig.tools`, while `StreamProcessor._enrichSchemaDepthError` uses a freshly-resolved provider's tool state. They are intentionally separate implementations, not duplicate code. Do not suggest extracting them into a shared utility.
Applied to files:
packages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T21:40:39.044Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:521-525
Timestamp: 2026-03-26T21:40:39.044Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), `processGeminiStreamEvents` previously always returned `StreamProcessingStatus.Completed` even when a `UserCancelled` or `Error` event was received — this was pre-existing dead code from the original `useGeminiStream.ts` (line 1371 checked for UserCancelled but the function always returned Completed). Fixed in commit 4a28363bc by adding a `processingResult` variable that tracks the status through the event loop, setting it to `StreamProcessingStatus.UserCancelled` or `StreamProcessingStatus.Error` as appropriate, and returning it at the end instead of always returning `Completed`.
Applied to files:
packages/core/src/core/StreamProcessor.ts
🔇 Additional comments (3)
packages/core/src/core/DirectMessageProcessor.ts (1)
387-395: Good hardening onallowedFunctionNamestype-check before filtering.Line 387 correctly gates filtering behind
Array.isArray, which avoids applying.includeslogic to malformed hook output while preserving expected allowlist behavior.packages/core/src/core/StreamProcessor.ts (1)
424-432: Parity and safety update looks correct.Line 424 brings the same array-guard behavior to
StreamProcessor, keeping tool-selection semantics consistent withDirectMessageProcessor.packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts (1)
121-171: Nice cross-processor coverage for the key allowlist states.The parameterized matrix cleanly validates omitted/
undefined/empty/populatedallowedFunctionNamesbehavior in both processors.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts (1)
122-173:⚠️ Potential issue | 🟡 MinorAdd the malformed-input regression case.
The suite covers omitted,
undefined, empty, and populated arrays, but it still misses the non-array case that proves the new runtime guard is actually enforcing shape.Suggested test addition
it('returns no tools when allowedFunctionNames is an empty array', async () => { const toolsFromConfig = createTools(); const result = await applyToolSelectionHook( { allowedFunctionNames: [] }, toolsFromConfig, ); expect(result).toStrictEqual([]); }); + + it('leaves tools unchanged when allowedFunctionNames is non-array', async () => { + const toolsFromConfig = createTools(); + + const result = await applyToolSelectionHook( + { allowedFunctionNames: 'beta' as unknown as string[] }, + toolsFromConfig, + ); + + expect(result).toStrictEqual(toolsFromConfig); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts` around lines 122 - 173, Add a regression test that passes a malformed non-array value for allowedFunctionNames to prove the runtime guard rejects bad shapes: within the same describe.each block add an it() that calls applyToolSelectionHook({ allowedFunctionNames: 'beta' }, createTools()) and assert that the promise rejects (e.g., await expect(...).rejects.toThrow()) so the test references applyToolSelectionHook, createTools, and the allowedFunctionNames option to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts`:
- Around line 122-173: Add a regression test that passes a malformed non-array
value for allowedFunctionNames to prove the runtime guard rejects bad shapes:
within the same describe.each block add an it() that calls
applyToolSelectionHook({ allowedFunctionNames: 'beta' }, createTools()) and
assert that the promise rejects (e.g., await expect(...).rejects.toThrow()) so
the test references applyToolSelectionHook, createTools, and the
allowedFunctionNames option to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56672315-02aa-47b3-be94-a57006547b7d
📒 Files selected for processing (3)
packages/core/src/core/DirectMessageProcessor.tspackages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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). (5)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:322-336
Timestamp: 2026-03-21T17:07:21.182Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `if (allowedFunctions?.length)` guard inside `_applyToolSelectionHook` (around lines 323-337) is intentional preserved behavior from the original `geminiChat.ts` on `main`. An empty `allowedFunctionNames` array (`[]`) is treated as "no restriction" rather than "deny all" — this is the upstream semantic. Do not flag this as a bug in decomposition PRs; any behavioral change to honor an empty list as a deny-all policy would be a feature modification, not a refactoring. A follow-up issue (`#1748` in vybestack/llxprt-code) has been filed to address this upstream bug.
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: 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: 1907
File: packages/cli/src/ui/App.test.tsx:1583-1586
Timestamp: 2026-04-23T23:33:15.915Z
Learning: Repo vybestack/llxprt-code (PR `#1907`): For tests like packages/cli/src/ui/App.test.tsx, maintainers prefer titles that describe the feature under test (e.g., “auto-send queued messages”), even if the specific assertion covers a guard path (e.g., empty queue ⇒ no auto-send). Renaming for perfect alignment is optional and out of scope for lint-enforcement batches.
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.
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.
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: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:360-366
Timestamp: 2026-03-21T15:18:11.019Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `_handleBeforeModelHook` intentionally returns only `modifiedContents` (i.e., `modifiedRequest.contents`) from the BeforeModel hook result, dropping any hook changes to `tools`, `config`, or other request fields. This is faithfully preserved from the original `geminiChat.ts` behavior on the main branch and is not a regression from the decomposition. Do not flag this partial-field forwarding as a bug in future reviews.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1899
File: packages/core/src/core/geminiChat.runtime.test.ts:837-1007
Timestamp: 2026-04-12T05:38:21.192Z
Learning: In `packages/core/src/core/geminiChat.runtime.test.ts` (vybestack/llxprt-code PR `#1899`), the `stream idle timeout behavioral tests for TurnProcessor and DirectMessageProcessor` describe block intentionally tests only config plumbing and resolver integration (verifying `chat.getConfig()` surfaces the ephemeral setting and `resolveStreamIdleTimeoutMs` returns the correct value). The actual stalled-stream watchdog abort behavior is covered in dedicated consumer suites, e.g., `turn.test.ts` line ~787. Do not flag these as missing stalled-stream behavioral coverage — adding full stalled-stream tests for every consumer variant is considered scope expansion.
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.
📚 Learning: 2026-03-21T17:07:21.182Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:322-336
Timestamp: 2026-03-21T17:07:21.182Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `if (allowedFunctions?.length)` guard inside `_applyToolSelectionHook` (around lines 323-337) is intentional preserved behavior from the original `geminiChat.ts` on `main`. An empty `allowedFunctionNames` array (`[]`) is treated as "no restriction" rather than "deny all" — this is the upstream semantic. Do not flag this as a bug in decomposition PRs; any behavioral change to honor an empty list as a deny-all policy would be a feature modification, not a refactoring. A follow-up issue (`#1748` in vybestack/llxprt-code) has been filed to address this upstream bug.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-21T15:18:11.019Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:360-366
Timestamp: 2026-03-21T15:18:11.019Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `_handleBeforeModelHook` intentionally returns only `modifiedContents` (i.e., `modifiedRequest.contents`) from the BeforeModel hook result, dropping any hook changes to `tools`, `config`, or other request fields. This is faithfully preserved from the original `geminiChat.ts` behavior on the main branch and is not a regression from the decomposition. Do not flag this partial-field forwarding as a bug in future reviews.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T21:40:39.044Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:521-525
Timestamp: 2026-03-26T21:40:39.044Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), `processGeminiStreamEvents` previously always returned `StreamProcessingStatus.Completed` even when a `UserCancelled` or `Error` event was received — this was pre-existing dead code from the original `useGeminiStream.ts` (line 1371 checked for UserCancelled but the function always returned Completed). Fixed in commit 4a28363bc by adding a `processingResult` variable that tracks the status through the event loop, setting it to `StreamProcessingStatus.UserCancelled` or `StreamProcessingStatus.Error` as appropriate, and returning it at the end instead of always returning `Completed`.
Applied to files:
packages/core/src/core/StreamProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-02-15T21:44:56.598Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1407
File: packages/core/src/core/geminiChatHookTriggers.ts:56-65
Timestamp: 2026-02-15T21:44:56.598Z
Learning: Enforce the canonical speaker-to-role mapping used by GeminiChat hooks: in IContent.speaker, which is strictly typed as 'human | ai | tool' (no 'system'), map 'human' to the 'user' role, 'ai' to the 'model' role, and 'tool' to the 'user' role in all hook payloads. This pattern should be applied across related hook files within packages/core/src/core/ (not just the single file) to ensure consistent role assignment.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-02-16T19:18:56.265Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1436
File: packages/core/src/core/nonInteractiveToolExecutor.ts:0-0
Timestamp: 2026-02-16T19:18:56.265Z
Learning: Guideline: In the core scheduler architecture, the system runs in a single mode at a time—either interactive or non-interactive, never both on the same scheduler instance. Non-interactive (CLI one-shot) runs without any interactive session; interactive mode subagents run within the parent's interactive context and inherit its mode. When reviewing code, ensure non-interactive tool executions (e.g., in nonInteractiveToolExecutor.ts) create and use a fresh completionResolver per executeToolCall, and that there is no race with interactive sessions since they cannot coexist on the same scheduler instance. This pattern applies across files in packages/core/src/core/. Only apply to relevant files, not globally.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T17:07:10.889Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:10.889Z
Learning: In these core processor modules, ensure `makePositionMatcher()` is created once and reused across the entire batch (i.e., hoist it outside any per-`content` loops) rather than rebuilding it for each content item. Also ensure the `unmatched-tool` queue is consumed in-order across all batch items (not reset/handled independently per item), since that preserves correct batch-wide behavior.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-22T04:06:53.600Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:06:53.600Z
Learning: When computing `lastPromptTokenCount` (e.g., in the streaming path like `_convertIContentStream`/equivalent), ensure it includes the full prompt token footprint: `lastPromptTokenCount = promptTokens + cache_read_input_tokens + cache_creation_input_tokens`. Do not use `promptTokens` alone, because cached context would otherwise cause `CompressionHandler.shouldCompress()` to underestimate context usage and may incorrectly suppress needed compression. Keep this combined computation consistent with the non-streaming path (e.g., `TurnProcessor._executeProviderCall`), and do not treat the presence of cache-token additions as redundant—both token types are required for correctness when cached context is active.
Applied to files:
packages/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.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/core/src/core/StreamProcessor.tspackages/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-02-28T23:18:15.496Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1642
File: packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx:528-578
Timestamp: 2026-02-28T23:18:15.496Z
Learning: In `packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx`, tests use mocked child components (ToolConfirmationMessage, ToolMessage) for isolation and consistency with upstream gemini-cli patterns. Tests for permanent tool approval settings (`enablePermanentToolApproval`) use this mock-based approach and pass successfully as part of the cherry-picked upstream test suite.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-04-22T08:28:33.938Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/core/src/tools/edit.test.ts:1009-1012
Timestamp: 2026-04-22T08:28:33.938Z
Learning: In `packages/core/src/tools/edit.test.ts` (vybestack/llxprt-code), the `// eslint-disable-next-line vitest/no-conditional-in-test -- intentional: narrowing/filter/parameterized-test context` comment before `if (confirmation && 'onConfirm' in confirmation)` in the IDE mode test is intentional type-narrowing behavior for a lint-enforcement PR (`#1907`). Do not suggest replacing this pattern with assertion-based narrowing in lint-promotion or refactoring PRs — that refactor is considered out of scope for those PRs.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 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/core/src/core/toolSelectionHook.allowedFunctionNames.test.tspackages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-27T00:46:42.630Z
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.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-27T01:00:29.058Z
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.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.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/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-04-23T23:33:02.521Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/cli/src/ui/components/Footer.test.tsx:198-214
Timestamp: 2026-04-23T23:33:02.521Z
Learning: In `packages/cli/src/ui/components/Footer.test.tsx` (vybestack/llxprt-code PR `#1907`), the responsive breakpoint truncation test in the `"should adapt branch truncation length based on breakpoint"` test uses a regex contract (`/feature\/.+\.\.\..+/`) to validate truncation rather than asserting per-breakpoint `expectedMaxLength` values. Do not flag the unused `expectedMaxLength` field or suggest adding per-breakpoint length assertions — that is considered a test-coverage enhancement outside the scope of lint enforcement PRs.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-27T19:31:00.236Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/config/settings.ts:295-322
Timestamp: 2026-03-27T19:31:00.236Z
Learning: In `packages/cli/src/config/settings.ts` (vybestack/llxprt-code PR `#1797`), `mergeSettings()` does NOT deep-merge `hooksConfig` or `hooks` across scopes — both are left to the top-level spread, meaning a higher-precedence scope that defines only one nested key (e.g., `hooksConfig.disabled`) can silently drop inherited keys from lower-precedence scopes. This shallow-merge limitation for `hooksConfig` and `hooks` is a known pre-existing issue tracked in follow-up issue `#1802`. Do not re-flag this as a new bug in reviews of this file until `#1802` is resolved.
Applied to files:
packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts
📚 Learning: 2026-03-21T15:18:11.794Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:430-467
Timestamp: 2026-03-21T15:18:11.794Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. Do not flag this as a hook-sanitization bypass — it is established upstream behavior, and any change would be a behavioral modification, not a decomposition fix.
Applied to files:
packages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T17:07:28.742Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:431-468
Timestamp: 2026-03-21T17:07:28.742Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), the `_processDirectResponse` method intentionally appends `aggregatedText` to the candidate parts and defines `directResponse.text` even when `triggerAfterModelHook` returns a modified response via `getModifiedResponse()`. This faithfully preserves the original behavior from `geminiChat.ts` on the main branch. The pre-existing issue where the pre-hook aggregatedText is used after an AfterModel hook rewrites the response is tracked in issue `#1749` for a dedicated fix. Do not flag this as a hook-sanitization bypass in decomposition reviews — it is established upstream behavior.
Applied to files:
packages/core/src/core/DirectMessageProcessor.ts
📚 Learning: 2026-03-21T15:19:53.168Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:79-88
Timestamp: 2026-03-21T15:19:53.168Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`), `logApiRequest()` is intentionally called before `_applyPreSendHooks()` in `generateDirectMessage()`. This preserves the original `geminiChat.ts` behavior where `this._logApiRequest()` was called at line ~1195 with the initial requestContents, prior to BeforeModel/BeforeToolSelection hooks being applied. Logging post-hook state would be an improvement but is not a regression. Do not flag this ordering as a bug in decomposition reviews.
Applied to files:
packages/core/src/core/DirectMessageProcessor.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/core/src/core/DirectMessageProcessor.ts
🔇 Additional comments (2)
packages/core/src/core/StreamProcessor.ts (1)
424-432: Correctly tighten the guard to real arrays.This now distinguishes an explicit empty allowlist from omitted or malformed config without changing the surrounding hook flow.
packages/core/src/core/DirectMessageProcessor.ts (1)
387-395: Correctly tighten the guard to real arrays.This preserves the existing direct-message flow while fixing the allowlist handling for empty arrays.
…treated as no restriction
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. |
Collaborator
Author
|
Addressed CodeRabbit feedback by adding a malformed non-array allowedFunctionNames regression case to packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts. Also updated the PR description to link Fixes #1748 and include the focused verification plan. All PR checks are now passing. |
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.
TLDR
Fixes #1748 by treating an explicit empty
allowedFunctionNamesarray from BeforeToolSelection hooks as an instruction to block all tool functions, while preserving the existing no-filter behavior when the property is omitted or undefined.Dive Deeper
Both
DirectMessageProcessorandStreamProcessorpreviously gated filtering onallowedFunctions?.length, so an explicit empty array skipped filtering and behaved like no restriction. The processors now apply filtering wheneverallowedFunctionNamesis an array, including an empty array.The new focused test suite exercises both processors for omitted, undefined, empty-array, populated-array, and malformed non-array inputs.
Reviewer Test Plan
npm run typecheck --workspace @vybestack/llxprt-code-corenpm run test --workspace @vybestack/llxprt-code-core -- DirectMessageProcessor StreamProcessor geminiChat.hook-control hookAggregatornpx prettier --check packages/core/src/core/DirectMessageProcessor.ts packages/core/src/core/StreamProcessor.ts packages/core/src/core/toolSelectionHook.allowedFunctionNames.test.ts packages/core/src/core/geminiChat.hook-control.test.ts packages/core/src/hooks/hookAggregator.test.tsTesting Matrix
Fixes #1748