fix(core): convert media blocks to text placeholders during compression (Fixes #1875)#1892
Conversation
…on (Fixes #1875) Extend sanitizeHistoryForCompression() to handle media blocks by converting them to concise text placeholders, preventing unsupported media types (e.g., PDF file parts) from reaching the compression LLM call. Changes: - Add mediaBlockToCompressionPlaceholder() helper that uses classifyMediaBlock() to generate format [Attached <category>: <filename or mimeType>] - Update sanitizeHistoryForCompression() to detect and convert media blocks alongside existing tool block conversion - Media block messages keep their original speaker (unlike tool messages) - Add 7 new behavioral tests for media block handling This fix is provider-agnostic and benefits any provider that may not support certain media types (like PDFs) during compression, specifically addressing the Kimi 400 error: 'invalid part type: file'.
github-actions
bot
added
the
maintainer:e2e:ok
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 (1)
📜 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). (4)
🧰 Additional context used🧠 Learnings (27)📓 Common learnings📚 Learning: 2026-03-27T01:40:09.382ZApplied to files:
📚 Learning: 2026-03-22T04:07:00.235ZApplied to files:
📚 Learning: 2026-03-21T03:22:51.631ZApplied to files:
📚 Learning: 2026-03-21T15:17:37.899ZApplied to files:
📚 Learning: 2026-03-21T03:23:48.264ZApplied to files:
📚 Learning: 2026-03-29T05:46:51.084ZApplied to files:
📚 Learning: 2026-03-21T17:57:00.539ZApplied to files:
📚 Learning: 2026-03-21T15:59:45.670ZApplied to files:
📚 Learning: 2026-03-26T03:04:09.288ZApplied to files:
📚 Learning: 2026-03-25T12:57:45.842ZApplied to files:
📚 Learning: 2026-03-26T03:06:11.693ZApplied to files:
📚 Learning: 2026-03-01T17:19:16.202ZApplied to files:
📚 Learning: 2026-03-19T23:27:48.657ZApplied to files:
📚 Learning: 2026-03-27T19:31:13.642ZApplied to files:
📚 Learning: 2026-03-26T20:52:08.720ZApplied to files:
📚 Learning: 2026-03-13T14:32:11.331ZApplied to files:
📚 Learning: 2026-03-24T21:07:40.805ZApplied to files:
📚 Learning: 2026-02-28T20:19:36.726ZApplied to files:
📚 Learning: 2026-01-13T19:28:00.789ZApplied to files:
📚 Learning: 2026-03-31T15:03:03.633ZApplied to files:
📚 Learning: 2026-03-19T22:50:00.853ZApplied to files:
📚 Learning: 2026-03-19T23:27:48.689ZApplied 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-31T02:12:43.093ZApplied to files:
🔇 Additional comments (2)
Summary by CodeRabbit
WalkthroughAdds media block sanitization: media blocks are converted into standardized text placeholders via a new exported helper, and sanitization logic updated to replace media/tool blocks with text, preserve speakers for media-bearing messages, and only early-return when no tool/media blocks and speaker isn't Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #1892Issue AlignmentIssue #1875 describes a 400 error from Kimi during middle-out compression when PDF media blocks are present. The root cause: The fix is correct:
Side Effects
Code QualityCorrectness:
Error handling:
Type safety:
Tests and CoverageCoverage impact: Increase
VerdictReady — The fix correctly addresses the Kimi 400 error by converting media blocks to text placeholders during compression, similar to how tool blocks are already handled. The implementation is clean, well-tested, and maintains backward compatibility for non-media messages. |
![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/core/src/core/compression/utils.test.ts`:
- Around line 669-677: Add a regression test that covers the empty-string
filename fallback to mimeType by creating a history using
mediaBlock('image/png', '') (or equivalent) wrapped in humanMsgOnlyMedia and
asserting sanitizeHistoryForCompression returns a single block converted to text
with text '[Attached image: image/png]'; this mirrors the existing test but
supplies filename: '' so the code path in sanitizeHistoryForCompression that
checks filename fallback (referencing mediaBlock, humanMsgOnlyMedia, and
sanitizeHistoryForCompression) is exercised.
In `@packages/core/src/core/compression/utils.ts`:
- Around line 349-352: The placeholder generation in
mediaBlockToCompressionPlaceholder treats empty string filenames as valid
because it uses the nullish coalescing operator (media.filename ??
media.mimeType), producing an empty identifier; change the fallback logic to
treat empty or whitespace-only filenames as absent (e.g., use a truthy/trim
check) so identifier falls back to media.mimeType or 'unknown' instead; update
mediaBlockToCompressionPlaceholder to compute identifier using a trimmed truthy
test (or helper) rather than ?? and keep classifyMediaBlock usage 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: cc3cf5dd-f465-4235-bb92-74b8ce9d8fe1
📒 Files selected for processing (2)
packages/core/src/core/compression/utils.test.tspackages/core/src/core/compression/utils.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). (4)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
📚 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/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-22T04:07:00.235Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T03:22:51.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:327-363
Timestamp: 2026-03-21T03:22:51.631Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code), `classifyMixedParts()`, `convertPartListUnionToIContent()`, and `convertIContentToResponse()` intentionally do not handle Gemini media parts (`inlineData`/`fileData`). This matches the original `geminiChat.ts` behavior. Adding media support would be a feature addition, not a decomposition fix — do not flag the absence of `inlineData`/`fileData` handling in these functions as a bug in future reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T15:17:37.899Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T17:57:00.539Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts:294-305
Timestamp: 2026-03-21T17:57:00.539Z
Learning: In `packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts` (vybestack/llxprt-code PR `#1740`), `blocksToText` and `concatenateTextAndCodeBlocks` are intentionally kept as separate functions. They serve different callers with different contracts: `blocksToText` uses string concatenation and calls `trimStart()` on the result, while `concatenateTextAndCodeBlocks` uses an array+join pattern and guards against null `block.text`. Consolidating them is out of scope for the decomposition PR and should be done as a dedicated follow-up if desired. Do not flag this duplication in future decomposition reviews.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-21T03:23:48.264Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:297-315
Timestamp: 2026-03-21T03:23:48.264Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code PR `#1743`), `convertAllFunctionResponses` intentionally packs all `functionResponse` parts into a single `IContent` with `speaker='tool'` and multiple `tool_response` blocks. This is faithfully preserved from the original `convertPartListUnionToIContent` method in `geminiChat.ts` (pre-refactor, commit 71e63cb4, lines ~2984-3001). Do not flag this as incorrect multi-block packing — it is the established upstream behavior for this input-conversion path.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-02-28T20:19:36.726Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/anthropic/AnthropicProvider.ts:1233-1293
Timestamp: 2026-02-28T20:19:36.726Z
Learning: In the llxprt-code IContent model (packages/core/src/providers/anthropic/AnthropicProvider.ts and related message processing), each tool speaker message (IContent with speaker='tool') contains exactly one tool_response block plus its associated media blocks. Multiple tool responses are always separate IContent entries. This structural constraint is enforced by the message format, so code that filters toolResponseBlocks from a single IContent and iterates over them will only process one tool_response per iteration.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-01-13T19:28:00.789Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.789Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-13T14:32:11.331Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-13T14:32:11.331Z
Learning: In `packages/core/src/core/geminiChat.ts`, the `convertIContentToResponse` function already converts `ThinkingBlock` to `ThoughtPart` with `thought: true` (around line 3095). However, the `text` getter on the returned response object uses `parts.find((p) => 'text' in p)?.text`, which incorrectly matches `ThoughtPart` first (since `ThoughtPart` also has a `text` property) when Anthropic thinking blocks appear before text blocks. This causes the actual response text to not be surfaced. The fix is to exclude thought parts: `parts.find((p) => 'text' in p && !(p as ThoughtPart).thought)?.text || ''`.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.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/core/src/core/compression/utils.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/core/src/core/compression/utils.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T23:27:49.587Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts:199-203
Timestamp: 2026-03-19T23:27:49.587Z
Learning: In `packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts` (vybestack/llxprt-code), Scenarios 3, 7a, 7b, and 7c use `buildMessagesWithReasoning` (imported directly from `OpenAIRequestBuilder`) rather than calling `provider.generateChatCompletion`. This is intentional and pre-existing behavior: the original tests accessed the same helper via a hacky `buildMessagesWithReasoning.call(provider, ...)` private-method pattern. The PR's direct import is an improvement, not a regression. Do not flag these scenarios as insufficiently integrated — they are helper-level tests by design, and adding full provider-path coverage is out of scope for refactoring PRs.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-30T17:36:37.126Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/ui/components/shared/text-buffer.test.ts:2647-2704
Timestamp: 2026-03-30T17:36:37.126Z
Learning: In vybestack/llxprt-code, tests for the React hook useTextBuffer (packages/cli/src/ui/components/shared/text-buffer.ts) should not read internal fields like visualLayout. For cache/identity invalidation checks in packages/cli/src/ui/components/shared/text-buffer.test.ts, assert against the public allVisualLines array instead, and reserve internal-structure assertions for lower-level helpers if needed.
Applied to files:
packages/core/src/core/compression/utils.test.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/compression/utils.test.ts
📚 Learning: 2026-03-21T15:18:04.202Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:358-364
Timestamp: 2026-03-21T15:18:04.202Z
Learning: In `packages/core/src/core/ConversationManager.ts` (vybestack/llxprt-code PR `#1743`), `_addModelOutputToHistory` attaches `usageMetadata` to every `IContent` produced in the loop over `consolidatedOutputContents`. This is intentional behavior faithfully preserved from the original `recordHistory()` in `geminiChat.ts`. Multi-entry model output is limited to text/tool-call interleaving cases because `_consolidateModelOutput` already merges adjacent text parts. Do not flag the per-entry usageMetadata attachment as a double-counting bug in future reviews.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-25T22:19:49.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1775
File: packages/core/src/core/coreToolScheduler.test.ts:0-0
Timestamp: 2026-03-25T22:19:49.983Z
Learning: In `packages/core/src/core/coreToolScheduler.test.ts` (vybestack/llxprt-code), `createMockMessageBus()` does not forward `publish()` calls to registered subscribers — it only records them. Integration tests that rely on bus-driven subscriber callbacks (e.g., stale-correlation invalidation via `TOOL_CONFIRMATION_RESPONSE`) will not exercise those paths through the mock. Stale-correlation invalidation coverage for `ConfirmationCoordinator` is intentionally handled in `packages/core/src/scheduler/confirmation-coordinator.test.ts` via direct `handleMessageBusResponse` invocations, not in `coreToolScheduler.test.ts`. Do not flag this as missing coverage in the scheduler integration tests.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-26T02:47:18.478Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentExecution.test.ts:142-143
Timestamp: 2026-03-26T02:47:18.478Z
Learning: In `packages/core/src/core/subagentExecution.ts` (vybestack/llxprt-code PR `#1779`), `checkGoalCompletion` returns `Promise<Content[] | null>`, not `Promise<Part[] | null>`. `Content` has the shape `{ role: string, parts: Part[] }`, so test assertions like `result![0].parts[0]` are correct and access the first Part within the first Content object. Do not flag `result![0].parts[0]` indexing in tests for this function as an invalid shape access.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-01T17:19:16.202Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts:64-77
Timestamp: 2026-03-01T17:19:16.202Z
Learning: In llxprt-code, MediaBlock instances in the pipeline (from fileUtils, ContentConverters, etc.) use canonically lowercase MIME types per RFC 2045. MIME type normalization is enforced at the content creation layer, so case-insensitive MIME type checks (e.g., `mimeType.toLowerCase().startsWith('image/')`) are not needed in provider code. Cross-cutting MIME type normalization would be a separate architectural concern.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T22:51:20.841Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIStreamProcessor.ts:300-305
Timestamp: 2026-03-19T22:51:20.841Z
Learning: In `packages/core/src/providers/openai/OpenAIStreamProcessor.ts` (vybestack/llxprt-code), `reasoning_content` deltas from `parseStreamingReasoningDelta` are intentionally accumulated into `state.accumulatedReasoningContent` during chunk processing and emitted as a `ThinkingBlock` only at the end of the stream, co-packaged in the same `IContent` block as tool calls. This is required for DeepSeek-reasoner compatibility: the model requires `reasoning_content` and tool calls to appear together in a single `IContent` block. Do not flag this deferred/batched emission pattern as a bug — inline streaming of reasoning was a legacy-path behavior that has been removed.
Applied to files:
packages/core/src/core/compression/utils.test.ts
🔇 Additional comments (2)
packages/core/src/core/compression/utils.ts (1)
377-379: Media sanitization path looks correct.
sanitizeHistoryForCompressionnow correctly enters rewrite mode for media blocks and converts them to plain text placeholders before request building.Also applies to: 414-417
packages/core/src/core/compression/utils.test.ts (1)
654-777: Great media regression coverage.These tests cover the key conversion and speaker scenarios for issue
#1875and meaningfully reduce the chance of provider-specific 400 regressions.
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: 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/core/src/core/compression/utils.test.ts`:
- Around line 748-787: Add a regression test for a speaker='tool' message whose
tool_response block is followed by media blocks to ensure
sanitizeHistoryForCompression both placeholderizes media and re-tags the overall
message speaker to 'human'; create an IContent msg with speaker: 'tool', blocks:
[{ type: 'tool_response', ... }, mediaBlock(...)] (using existing mediaBlock
helper), call sanitizeHistoryForCompression([msg]) and assert result[0].speaker
=== 'human', that the tool_response block text contains the expected '[Tool
Response: ...]' marker and that the media block(s) were converted to text
placeholders like '[Attached image: ...]' (or PDF) in the output.
In `@packages/core/src/core/compression/utils.ts`:
- Around line 349-355: mediaBlockToCompressionPlaceholder currently drops
MediaBlock.caption; update it to include caption (MediaBlock.caption) in the
placeholder so captions/alt text aren't lost during compression. Change the
identifier logic to prefer a non-empty caption.trim() first, then
filename?.trim(), then mimeType, then 'unknown', and keep the existing category
handling (capitalize 'pdf' to 'PDF'). Ensure the returned string still uses the
same format (`[Attached ${label}: ${identifier}]`) but uses the caption when
available.
🪄 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: f89cc138-0062-45a3-aa39-7c6b2762975b
📒 Files selected for processing (2)
packages/core/src/core/compression/utils.test.tspackages/core/src/core/compression/utils.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). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (34)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:327-363
Timestamp: 2026-03-21T03:22:51.631Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code), `classifyMixedParts()`, `convertPartListUnionToIContent()`, and `convertIContentToResponse()` intentionally do not handle Gemini media parts (`inlineData`/`fileData`). This matches the original `geminiChat.ts` behavior. Adding media support would be a feature addition, not a decomposition fix — do not flag the absence of `inlineData`/`fileData` handling in these functions as a bug in future reviews.
📚 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/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-22T04:07:00.235Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T03:22:51.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:327-363
Timestamp: 2026-03-21T03:22:51.631Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code), `classifyMixedParts()`, `convertPartListUnionToIContent()`, and `convertIContentToResponse()` intentionally do not handle Gemini media parts (`inlineData`/`fileData`). This matches the original `geminiChat.ts` behavior. Adding media support would be a feature addition, not a decomposition fix — do not flag the absence of `inlineData`/`fileData` handling in these functions as a bug in future reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-29T05:46:51.084Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1827
File: packages/core/src/core/compression/CompressionHandler.ts:714-722
Timestamp: 2026-03-29T05:46:51.084Z
Learning: In `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1827`), `lastSuccessfulCompressionTime` is intentionally set in two places when fallback compression succeeds via `runCompressionWithRetryAndFallback`: once inside `performFallbackCompression` (needed for the `forceTruncationIfIneffective`-only call path) and once in `performCompression` after `runCompressionWithRetryAndFallback` returns. The duplicate assignment is harmless (both use `Date.now()`) and is deliberately left without extra branching or flags to keep the code simple. Do not flag this dual assignment as a redundancy in future reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-25T12:57:45.842Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/containers/AppContainer/hooks/useModelRuntimeSync.ts:35-38
Timestamp: 2026-03-25T12:57:45.842Z
Learning: In `packages/cli/src/ui/containers/AppContainer/hooks/useModelRuntimeSync.ts` (vybestack/llxprt-code PR `#1768`), the pattern `providerModel && providerModel.trim() !== '' ? providerModel : configModel` (checking trim() for emptiness but assigning the untrimmed value) is intentional pre-existing behavior moved verbatim from the original `AppContainer.tsx`. Do not flag the lack of normalization (using the trimmed value as `effectiveModel`) as a whitespace propagation bug in decomposition or future PRs — any fix would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-21T15:59:45.670Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/utils/toolIdNormalization.ts:98-121
Timestamp: 2026-03-21T15:59:45.670Z
Learning: In `packages/core/src/providers/utils/toolIdNormalization.ts` (vybestack/llxprt-code PR `#1740`), `normalizeToAnthropicToolId` handles empty/falsy input by returning the deterministic constant `'toolu_empty'` and logging via `debugLogger.error(...)`. This path is unreachable in practice because all IDs first pass through `normalizeToHistoryToolId` (which maps empty → `hist_tool_`, then caught by the `hist_tool_` prefix handler), but the error log surfaces it if a provider ever returns an empty tool call ID. Do not suggest a random/hash-based fallback — that was a prior bug (pairing hazard: tool_use and tool_result calling separately would get different IDs). Fixed in commit adc8e9d.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-21T17:57:00.539Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts:294-305
Timestamp: 2026-03-21T17:57:00.539Z
Learning: In `packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts` (vybestack/llxprt-code PR `#1740`), `blocksToText` and `concatenateTextAndCodeBlocks` are intentionally kept as separate functions. They serve different callers with different contracts: `blocksToText` uses string concatenation and calls `trimStart()` on the result, while `concatenateTextAndCodeBlocks` uses an array+join pattern and guards against null `block.text`. Consolidating them is out of scope for the decomposition PR and should be done as a dedicated follow-up if desired. Do not flag this duplication in future decomposition reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-01T17:19:16.202Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts:64-77
Timestamp: 2026-03-01T17:19:16.202Z
Learning: In llxprt-code, MediaBlock instances in the pipeline (from fileUtils, ContentConverters, etc.) use canonically lowercase MIME types per RFC 2045. MIME type normalization is enforced at the content creation layer, so case-insensitive MIME type checks (e.g., `mimeType.toLowerCase().startsWith('image/')`) are not needed in provider code. Cross-cutting MIME type normalization would be a separate architectural concern.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-02-28T20:19:36.726Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/anthropic/AnthropicProvider.ts:1233-1293
Timestamp: 2026-02-28T20:19:36.726Z
Learning: In the llxprt-code IContent model (packages/core/src/providers/anthropic/AnthropicProvider.ts and related message processing), each tool speaker message (IContent with speaker='tool') contains exactly one tool_response block plus its associated media blocks. Multiple tool responses are always separate IContent entries. This structural constraint is enforced by the message format, so code that filters toolResponseBlocks from a single IContent and iterates over them will only process one tool_response per iteration.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T03:23:48.264Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:297-315
Timestamp: 2026-03-21T03:23:48.264Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code PR `#1743`), `convertAllFunctionResponses` intentionally packs all `functionResponse` parts into a single `IContent` with `speaker='tool'` and multiple `tool_response` blocks. This is faithfully preserved from the original `convertPartListUnionToIContent` method in `geminiChat.ts` (pre-refactor, commit 71e63cb4, lines ~2984-3001). Do not flag this as incorrect multi-block packing — it is the established upstream behavior for this input-conversion path.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-01-13T19:28:00.789Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.789Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-13T14:32:11.331Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-13T14:32:11.331Z
Learning: In `packages/core/src/core/geminiChat.ts`, the `convertIContentToResponse` function already converts `ThinkingBlock` to `ThoughtPart` with `thought: true` (around line 3095). However, the `text` getter on the returned response object uses `parts.find((p) => 'text' in p)?.text`, which incorrectly matches `ThoughtPart` first (since `ThoughtPart` also has a `text` property) when Anthropic thinking blocks appear before text blocks. This causes the actual response text to not be surfaced. The fix is to exclude thought parts: `parts.find((p) => 'text' in p && !(p as ThoughtPart).thought)?.text || ''`.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.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/core/src/core/compression/utils.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/core/src/core/compression/utils.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T23:27:49.587Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts:199-203
Timestamp: 2026-03-19T23:27:49.587Z
Learning: In `packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts` (vybestack/llxprt-code), Scenarios 3, 7a, 7b, and 7c use `buildMessagesWithReasoning` (imported directly from `OpenAIRequestBuilder`) rather than calling `provider.generateChatCompletion`. This is intentional and pre-existing behavior: the original tests accessed the same helper via a hacky `buildMessagesWithReasoning.call(provider, ...)` private-method pattern. The PR's direct import is an improvement, not a regression. Do not flag these scenarios as insufficiently integrated — they are helper-level tests by design, and adding full provider-path coverage is out of scope for refactoring PRs.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-30T17:36:37.126Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/ui/components/shared/text-buffer.test.ts:2647-2704
Timestamp: 2026-03-30T17:36:37.126Z
Learning: In vybestack/llxprt-code, tests for the React hook useTextBuffer (packages/cli/src/ui/components/shared/text-buffer.ts) should not read internal fields like visualLayout. For cache/identity invalidation checks in packages/cli/src/ui/components/shared/text-buffer.test.ts, assert against the public allVisualLines array instead, and reserve internal-structure assertions for lower-level helpers if needed.
Applied to files:
packages/core/src/core/compression/utils.test.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/compression/utils.test.ts
📚 Learning: 2026-03-21T15:17:37.899Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.test.ts
📚 Learning: 2026-03-27T00:46:42.685Z
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.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-24T21:07:40.805Z
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.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.test.ts
📚 Learning: 2026-03-27T00:46:43.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts:347-383
Timestamp: 2026-03-27T00:46:43.700Z
Learning: In `packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the last test ("profile ephemeral folderTrust value does NOT change the trust check") intentionally omits a real profile load. Its sole purpose is to assert that `isWorkspaceTrusted` is called with the original settings object (not a profile-merged copy) in the untrusted-folder branch. The profile-merge path is covered by other parity test files. Do not suggest adding an inline profile or `LLXPRT_PROFILE` env var to this test — that would be scope expansion beyond its intended parity coverage.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-04-03T05:57:42.326Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-26T02:47:18.478Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentExecution.test.ts:142-143
Timestamp: 2026-03-26T02:47:18.478Z
Learning: In `packages/core/src/core/subagentExecution.ts` (vybestack/llxprt-code PR `#1779`), `checkGoalCompletion` returns `Promise<Content[] | null>`, not `Promise<Part[] | null>`. `Content` has the shape `{ role: string, parts: Part[] }`, so test assertions like `result![0].parts[0]` are correct and access the first Part within the first Content object. Do not flag `result![0].parts[0]` indexing in tests for this function as an invalid shape access.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T22:51:20.841Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIStreamProcessor.ts:300-305
Timestamp: 2026-03-19T22:51:20.841Z
Learning: In `packages/core/src/providers/openai/OpenAIStreamProcessor.ts` (vybestack/llxprt-code), `reasoning_content` deltas from `parseStreamingReasoningDelta` are intentionally accumulated into `state.accumulatedReasoningContent` during chunk processing and emitted as a `ThinkingBlock` only at the end of the stream, co-packaged in the same `IContent` block as tool calls. This is required for DeepSeek-reasoner compatibility: the model requires `reasoning_content` and tool calls to appear together in a single `IContent` block. Do not flag this deferred/batched emission pattern as a bug — inline streaming of reasoning was a legacy-path behavior that has been removed.
Applied to files:
packages/core/src/core/compression/utils.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/core/compression/utils.ts`:
- Around line 341-355: The JSDoc for mediaBlockToCompressionPlaceholder is out
of sync with the actual identifier precedence (the code prefers caption then
filename then mimeType), so update the function comment to reflect the real
order; edit the JSDoc above mediaBlockToCompressionPlaceholder to state the
format uses the category with the identifier chosen in precedence: caption →
filename → mimeType → "unknown" (and optionally note caption is preferred for
accessibility/context), and ensure any mention of filename or mimeType alone is
removed or rewritten to match the identifier variable logic and
classifyMediaBlock usage.
🪄 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: 812c0a15-5cf0-4473-ac2a-39ccdc52f4a7
📒 Files selected for processing (2)
packages/core/src/core/compression/utils.test.tspackages/core/src/core/compression/utils.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). (4)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (37)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:327-363
Timestamp: 2026-03-21T03:22:51.631Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code), `classifyMixedParts()`, `convertPartListUnionToIContent()`, and `convertIContentToResponse()` intentionally do not handle Gemini media parts (`inlineData`/`fileData`). This matches the original `geminiChat.ts` behavior. Adding media support would be a feature addition, not a decomposition fix — do not flag the absence of `inlineData`/`fileData` handling in these functions as a bug in future reviews.
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.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
📚 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/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-22T04:07:00.235Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:07:00.235Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the streaming path in `_convertIContentStream` (or equivalent) must compute `lastPromptTokenCount` as `promptTokens + cache_read_input_tokens + cache_creation_input_tokens`, not raw `promptTokens` alone. Using only raw `promptTokens` causes `CompressionHandler.shouldCompress()` to underestimate context usage when a cached context is active, potentially suppressing needed compression. Fixed in commit c3e68b476. This cache-token inclusion is consistent with the non-streaming path in `TurnProcessor._executeProviderCall`. Do not flag the combined computation as redundant — both token types must be included to accurately reflect the full prompt token footprint.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T03:22:51.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:327-363
Timestamp: 2026-03-21T03:22:51.631Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code), `classifyMixedParts()`, `convertPartListUnionToIContent()`, and `convertIContentToResponse()` intentionally do not handle Gemini media parts (`inlineData`/`fileData`). This matches the original `geminiChat.ts` behavior. Adding media support would be a feature addition, not a decomposition fix — do not flag the absence of `inlineData`/`fileData` handling in these functions as a bug in future reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T15:17:37.899Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T03:23:48.264Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/MessageConverter.ts:297-315
Timestamp: 2026-03-21T03:23:48.264Z
Learning: In `packages/core/src/core/MessageConverter.ts` (vybestack/llxprt-code PR `#1743`), `convertAllFunctionResponses` intentionally packs all `functionResponse` parts into a single `IContent` with `speaker='tool'` and multiple `tool_response` blocks. This is faithfully preserved from the original `convertPartListUnionToIContent` method in `geminiChat.ts` (pre-refactor, commit 71e63cb4, lines ~2984-3001). Do not flag this as incorrect multi-block packing — it is the established upstream behavior for this input-conversion path.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-29T05:46:51.084Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1827
File: packages/core/src/core/compression/CompressionHandler.ts:714-722
Timestamp: 2026-03-29T05:46:51.084Z
Learning: In `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1827`), `lastSuccessfulCompressionTime` is intentionally set in two places when fallback compression succeeds via `runCompressionWithRetryAndFallback`: once inside `performFallbackCompression` (needed for the `forceTruncationIfIneffective`-only call path) and once in `performCompression` after `runCompressionWithRetryAndFallback` returns. The duplicate assignment is harmless (both use `Date.now()`) and is deliberately left without extra branching or flags to keep the code simple. Do not flag this dual assignment as a redundancy in future reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T17:57:00.539Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts:294-305
Timestamp: 2026-03-21T17:57:00.539Z
Learning: In `packages/core/src/providers/anthropic/AnthropicMessageNormalizer.ts` (vybestack/llxprt-code PR `#1740`), `blocksToText` and `concatenateTextAndCodeBlocks` are intentionally kept as separate functions. They serve different callers with different contracts: `blocksToText` uses string concatenation and calls `trimStart()` on the result, while `concatenateTextAndCodeBlocks` uses an array+join pattern and guards against null `block.text`. Consolidating them is out of scope for the decomposition PR and should be done as a dedicated follow-up if desired. Do not flag this duplication in future decomposition reviews.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-21T15:59:45.670Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/utils/toolIdNormalization.ts:98-121
Timestamp: 2026-03-21T15:59:45.670Z
Learning: In `packages/core/src/providers/utils/toolIdNormalization.ts` (vybestack/llxprt-code PR `#1740`), `normalizeToAnthropicToolId` handles empty/falsy input by returning the deterministic constant `'toolu_empty'` and logging via `debugLogger.error(...)`. This path is unreachable in practice because all IDs first pass through `normalizeToHistoryToolId` (which maps empty → `hist_tool_`, then caught by the `hist_tool_` prefix handler), but the error log surfaces it if a provider ever returns an empty tool call ID. Do not suggest a random/hash-based fallback — that was a prior bug (pairing hazard: tool_use and tool_result calling separately would get different IDs). Fixed in commit adc8e9d.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-25T12:57:45.842Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/containers/AppContainer/hooks/useModelRuntimeSync.ts:35-38
Timestamp: 2026-03-25T12:57:45.842Z
Learning: In `packages/cli/src/ui/containers/AppContainer/hooks/useModelRuntimeSync.ts` (vybestack/llxprt-code PR `#1768`), the pattern `providerModel && providerModel.trim() !== '' ? providerModel : configModel` (checking trim() for emptiness but assigning the untrimmed value) is intentional pre-existing behavior moved verbatim from the original `AppContainer.tsx`. Do not flag the lack of normalization (using the trimmed value as `effectiveModel`) as a whitespace propagation bug in decomposition or future PRs — any fix would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-01T17:19:16.202Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts:64-77
Timestamp: 2026-03-01T17:19:16.202Z
Learning: In llxprt-code, MediaBlock instances in the pipeline (from fileUtils, ContentConverters, etc.) use canonically lowercase MIME types per RFC 2045. MIME type normalization is enforced at the content creation layer, so case-insensitive MIME type checks (e.g., `mimeType.toLowerCase().startsWith('image/')`) are not needed in provider code. Cross-cutting MIME type normalization would be a separate architectural concern.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T23:27:48.657Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIStreamProcessor.ts:358-359
Timestamp: 2026-03-19T23:27:48.657Z
Learning: In `packages/core/src/providers/openai/OpenAIStreamProcessor.ts` (vybestack/llxprt-code), `state.accumulatedText` is incremented with raw delta content before Kimi/tool-call markup or `<think>` content is stripped, and the `requestContinuation()` guards check `accumulatedText.length === 0` and `accumulatedReasoningContent.length === 0`. This is intentional pre-existing behavior faithfully preserved from the original `generatePipelineChatCompletionImpl` (lines 1824, 2019, 2542-2544, 2597-2600 in the legacy file). Do not flag this as a bug or suggest replacing these guards with a `hasVisibleText` flag tracking only yielded TextBlocks — any such change would be a behavioral modification, not a refactoring, and is out of scope for extraction PRs.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-27T19:31:13.642Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/ui/utils/highlight.ts:43-45
Timestamp: 2026-03-27T19:31:13.642Z
Learning: In `packages/cli/src/ui/utils/highlight.ts` (vybestack/llxprt-code PR `#1797`), the `highlightCache` cache key intentionally omits the `transformations` payload (only encoding `index`, `isCursorInsideTransform`, `cursorCol`, and `text`). Including the full transformations serialization was deferred due to hot-path performance tradeoffs; it should only be revisited with benchmark-backed evidence when real incorrect-highlighting reports are observed. Do not re-flag the missing transformations in the cache key in future reviews.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-02-28T20:19:36.726Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1640
File: packages/core/src/providers/anthropic/AnthropicProvider.ts:1233-1293
Timestamp: 2026-02-28T20:19:36.726Z
Learning: In the llxprt-code IContent model (packages/core/src/providers/anthropic/AnthropicProvider.ts and related message processing), each tool speaker message (IContent with speaker='tool') contains exactly one tool_response block plus its associated media blocks. Multiple tool responses are always separate IContent entries. This structural constraint is enforced by the message format, so code that filters toolResponseBlocks from a single IContent and iterates over them will only process one tool_response per iteration.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-01-13T19:28:00.789Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.789Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/core/src/core/compression/utils.ts
📚 Learning: 2026-03-13T14:32:11.331Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-13T14:32:11.331Z
Learning: In `packages/core/src/core/geminiChat.ts`, the `convertIContentToResponse` function already converts `ThinkingBlock` to `ThoughtPart` with `thought: true` (around line 3095). However, the `text` getter on the returned response object uses `parts.find((p) => 'text' in p)?.text`, which incorrectly matches `ThoughtPart` first (since `ThoughtPart` also has a `text` property) when Anthropic thinking blocks appear before text blocks. This causes the actual response text to not be surfaced. The fix is to exclude thought parts: `parts.find((p) => 'text' in p && !(p as ThoughtPart).thought)?.text || ''`.
Applied to files:
packages/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.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/core/src/core/compression/utils.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/core/src/core/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.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/compression/utils.tspackages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-30T17:36:37.126Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/ui/components/shared/text-buffer.test.ts:2647-2704
Timestamp: 2026-03-30T17:36:37.126Z
Learning: In vybestack/llxprt-code, tests for the React hook useTextBuffer (packages/cli/src/ui/components/shared/text-buffer.ts) should not read internal fields like visualLayout. For cache/identity invalidation checks in packages/cli/src/ui/components/shared/text-buffer.test.ts, assert against the public allVisualLines array instead, and reserve internal-structure assertions for lower-level helpers if needed.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-19T23:27:49.587Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts:199-203
Timestamp: 2026-03-19T23:27:49.587Z
Learning: In `packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts` (vybestack/llxprt-code), Scenarios 3, 7a, 7b, and 7c use `buildMessagesWithReasoning` (imported directly from `OpenAIRequestBuilder`) rather than calling `provider.generateChatCompletion`. This is intentional and pre-existing behavior: the original tests accessed the same helper via a hacky `buildMessagesWithReasoning.call(provider, ...)` private-method pattern. The PR's direct import is an improvement, not a regression. Do not flag these scenarios as insufficiently integrated — they are helper-level tests by design, and adding full provider-path coverage is out of scope for refactoring PRs.
Applied to files:
packages/core/src/core/compression/utils.test.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/compression/utils.test.ts
📚 Learning: 2026-03-27T00:46:42.685Z
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.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.test.ts
📚 Learning: 2026-03-24T21:07:40.805Z
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.
Applied to files:
packages/core/src/core/compression/utils.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/compression/utils.test.ts
📚 Learning: 2026-03-27T00:46:43.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts:347-383
Timestamp: 2026-03-27T00:46:43.700Z
Learning: In `packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the last test ("profile ephemeral folderTrust value does NOT change the trust check") intentionally omits a real profile load. Its sole purpose is to assert that `isWorkspaceTrusted` is called with the original settings object (not a profile-merged copy) in the untrusted-folder branch. The profile-merge path is covered by other parity test files. Do not suggest adding an inline profile or `LLXPRT_PROFILE` env var to this test — that would be scope expansion beyond its intended parity coverage.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-04-03T05:57:42.326Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Applied to files:
packages/core/src/core/compression/utils.test.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/compression/utils.test.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, `OpenAIStreamProcessor.ts` (processStreamingResponse) has a `hasToolsButNoText` guard that calls `requestContinuation()` only when `toolCallCount > 0 AND accumulatedText/textBuffer/accumulatedReasoningContent/accumulatedThinkingContent are all empty`. For Kimi K2 on the native Fireworks/OpenAI path, the model emits `<think>…</think>` content (landing in `accumulatedThinkingContent`) then stops without text. Because `accumulatedThinkingContent.length > 0`, `hasToolsButNoText` is false and no continuation fires. A separate `hasThinkingButNoText` guard is needed to call `requestContinuation([],…)` when thinking is present but text is absent and toolCallCount === 0.
Applied to files:
packages/core/src/core/compression/utils.test.ts
📚 Learning: 2026-03-26T02:47:18.478Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentExecution.test.ts:142-143
Timestamp: 2026-03-26T02:47:18.478Z
Learning: In `packages/core/src/core/subagentExecution.ts` (vybestack/llxprt-code PR `#1779`), `checkGoalCompletion` returns `Promise<Content[] | null>`, not `Promise<Part[] | null>`. `Content` has the shape `{ role: string, parts: Part[] }`, so test assertions like `result![0].parts[0]` are correct and access the first Part within the first Content object. Do not flag `result![0].parts[0]` indexing in tests for this function as an invalid shape access.
Applied to files:
packages/core/src/core/compression/utils.test.ts
🔇 Additional comments (2)
packages/core/src/core/compression/utils.ts (1)
382-423: Media placeholderization in sanitization path looks correct.Good change: media blocks are now converted to text placeholders in-order alongside tool block sanitization, which directly addresses provider-side validation failures from non-text media payloads.
packages/core/src/core/compression/utils.test.ts (1)
656-835: Excellent regression coverage for Issue#1875.These tests exercise the key behavioral matrix (filename/mime fallback, empty filename, media categories, mixed content ordering, speaker behavior, caption preference, and tool+media interplay). This materially reduces risk of reintroducing the 400-error path.
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
Fixes issue #1875 where Kimi compression fails with 400 error when media blocks (like PDFs) are sent during compression.
Problem
The error
400 the message at position 25 with role 'user' contains an invalid part type: fileoccurs during middle-out compression. This happens becausesanitizeHistoryForCompression()converts tool blocks to text but passes media blocks through unchanged. When these reach Kimi viaOpenAIRequestBuilder.ts, PDF media blocks get converted totype: 'file'parts that Kimi does not support.Solution
Extend
sanitizeHistoryForCompression()to convert media blocks to concise text placeholders, similar to how it handles tool blocks.Changes
Task 1: mediaBlockToCompressionPlaceholder() helper
classifyMediaBlockfrom../../providers/utils/mediaUtils.jsMediaBlockas inputclassifyMediaBlock()to determine category (image, pdf, audio, video, unknown)[Attached <category>: <filename or mimeType>]media.filenameif available, otherwise usesmedia.mimeTypeTask 2: Update sanitizeHistoryForCompression()
hasToolBlocksdetection to also check for media blocks:b.type === 'media'block.type === 'media'that:mediaBlockToCompressionPlaceholder()Task 3: Behavioral tests (TDD)
Added 7 new tests to
utils.test.ts:[Attached PDF: document.pdf])[Attached image: image/png])Verification
npm run test- All tests pass (9417 passed, 33 skipped)npm run lint- No new errors (59 pre-existing warnings in modified files)npm run typecheck- No TypeScript errors in core packagenpm run format- Code properly formattednpm run build- Build succeedsnode scripts/start.js --profile-load synthetic "write me a haiku and nothing else"- Smoke test passesWhy This Fix Is Provider-Agnostic
The fix benefits any provider that may not support certain media types during compression. While the issue was specifically reported for Kimi (which doesn't support PDF file parts), the same pattern applies to any provider with similar limitations. The actual media data isn't necessary for the compression LLM to understand conversation context for summarization.