Repository navigation
feat(config): add compactModel option to use a separate model for compaction (#1445) - #1629
Conversation
…paction When set and different from mainLoopModel, the forked-agent prompt-cache-sharing path is bypassed (guaranteed cache miss with different models) and the streaming fallback uses compactModel for the API call instead. Closes Twigpine#1445
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (8)
📝 WalkthroughWalkthroughAdds an optional ChangescompactModel config option and compact service wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/compact/compact.ts (1)
462-471:⚠️ Potential issue | 🟡 MinorCache-sharing gate is correct, but test coverage for model-change scenarios is missing.
The logic correctly disables prompt-cache sharing when
compactModeldiffers frommainLoopModel. The gate at lines 462–471 uses strict inequality (!==), so it will catch empty strings and prevent cache sharing.However, if
compactModelis set to an empty string"", the model selection at line 1413 usescompactModel ?? context.options.mainLoopModel— which would pass""to the API (since??treats empty strings as non-nullish). This is low-likelihood (requires explicitclaude config set compactModel ""), but the gate mitigates the cache-sharing risk; the API call risk remains.Missing: tests covering
modelChangesForCompactionwhencompactModeldiffers frommainLoopModel. The existing provider-gate tests cover Anthropic vs non-Anthropic, but not model-change scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/compact/compact.ts` around lines 462 - 471, Add test coverage for the modelChangesForCompaction scenario to verify that prompt-cache sharing is correctly disabled when compactModel differs from mainLoopModel. Create tests that set compactModel to a different value than context.options.mainLoopModel and assert that modelChangesForCompaction evaluates to true, which in turn causes promptCacheSharingEnabled to be false. Reference the existing provider-gate tests for structure and add similar test cases for model-change scenarios to ensure the cache-sharing gate behaves correctly across different model configurations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/compact/compact.ts`:
- Around line 462-471: Add test coverage for the modelChangesForCompaction
scenario to verify that prompt-cache sharing is correctly disabled when
compactModel differs from mainLoopModel. Create tests that set compactModel to a
different value than context.options.mainLoopModel and assert that
modelChangesForCompaction evaluates to true, which in turn causes
promptCacheSharingEnabled to be false. Reference the existing provider-gate
tests for structure and add similar test cases for model-change scenarios to
ensure the cache-sharing gate behaves correctly across different model
configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d72b7e1e-7465-4c52-94cb-5dcaf8e69307
📒 Files selected for processing (2)
src/services/compact/compact.tssrc/utils/config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/config.tssrc/services/compact/compact.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/config.tssrc/services/compact/compact.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/config.tssrc/services/compact/compact.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/config.tssrc/services/compact/compact.ts
🔇 Additional comments (5)
src/utils/config.ts (2)
673-676: LGTM!
781-781: LGTM!src/services/compact/compact.ts (3)
44-44: LGTM!
1193-1205: Duplicate logic matches the main path.The
compactModelread andmodelChangesForCompactioncomputation are duplicated fromcompactConversation(). Given thatstreamCompactSummary()is called independently (including frompartialCompactConversation), and the logic is only 3 lines, this duplication is acceptable — extracting it would add more overhead than value.
1413-1420: Model override and max-tokens derivation are correct.The streaming fallback correctly uses
compactModel ?? mainLoopModelfor both the model parameter and thegetMaxOutputTokensForModelcall, ensuring the output token limit matches the chosen model's constraints.Same empty-string edge case as noted in the cache-sharing block above (low likelihood).
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P2] Expose and validate
compactModelthrough the config surface
src/utils/config.ts:676
The PR wirescompactModelintoGlobalConfigand the compaction runtime, but the linked issue asks users to configure it withclaude config set compactModel ...and explicitly lists the supported setting/model-validation work as part of the implementation. In this patch,compactModelonly appears inconfig.tsandcompact.ts; the/configsettings list still jumps fromautoCompactEnabledtomaxMessagesCompactionThreshold, and there is no supported-setting/model validation entry for this key. That leaves the feature effectively unavailable through the advertised user-facing config path, while manual config edits can still pass invalid values such as an empty string into the compaction API call. Please add the missing config UI/command surface and validate or normalize the model value before using it for compaction. -
[P2] Complete CodeRabbit's request for
compactModelcoverage
src/services/compact/compact.ts:462
CodeRabbit's review item asks for coverage of themodelChangesForCompactionpath, and the current patch still has no test that setscompactModelto a model different frommainLoopModeland verifies that the forked-agent cache-sharing path is skipped while the streaming fallback uses the override model. The existing compact tests only cover provider compatibility, so a future edit could easily re-enable cache sharing for a mismatched model or forget to pass the override intoqueryModelWithStreamingwithout any regression failing. Please complete that review request with focused coverage for the different-model case.
… test coverage Addresses review feedback on Twigpine#1629: - Surface compactModel as a managedEnum setting in the /config screen, following the teammateDefaultModel pattern (submenu + ModelPicker). - Add a compact.test.ts case covering the compactModel !== mainLoopModel path: cache-sharing is skipped and the streaming compaction fallback routes model/maxOutputTokensOverride to compactModel.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Settings/Config.tsx`:
- Around line 1616-1634: Add a check before performing the config update
operations to ensure the compact model is actually changing. Before the line
that sets isDirty.current to true, compare the incoming model_2 value with the
current compactModel value from getGlobalConfig(). If they are already equal
(accounting for the null/undefined case where an unset compactModel and model_2
=== null are equivalent), skip the entire block that includes isDirty.current =
true, saveGlobalConfig, setGlobalConfig, and setChanges. This prevents marking
the dialog as dirty and recording unnecessary changes when the user selects an
option that doesn't actually change the current state.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 112e8154-7f90-42c0-a2b7-d41c41ffba3b
📒 Files selected for processing (2)
src/components/Settings/Config.tsxsrc/services/compact/compact.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/services/compact/compact.test.tssrc/components/Settings/Config.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/services/compact/compact.test.tssrc/components/Settings/Config.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/compact/compact.test.tssrc/components/Settings/Config.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/compact/compact.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/compact/compact.test.tssrc/components/Settings/Config.tsx
🔇 Additional comments (1)
src/services/compact/compact.test.ts (1)
203-208: LGTM!Also applies to: 327-342, 386-388, 679-727
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the current head and found issues that still need to be addressed.
Findings
-
[P2] Check tool-search support against the compact model
src/services/compact/compact.ts:1365
The streaming compaction path now sendsqueryModelWithStreaming()withmodel: compactModel ?? context.options.mainLoopModel, but the tool-search capability check just above it still callsisToolSearchEnabled()withcontext.options.mainLoopModel. That means a user can configure a compact model whose tool-search/tool-call surface differs from the main model, yet the compaction request will include tools chosen for the wrong model. In the bad direction this can attach unsupported tool-search tooling to the compact model request; in the other direction it can unnecessarily drop tool-search support even though the compact model supports it. Please run the tool-search eligibility check against the actual model used for compaction, and add a focused regression test forcompactModel !== mainLoopModel. -
[P3] Complete CodeRabbit's request to skip no-op compact model selections
src/components/Settings/Config.tsx:1616
CodeRabbit's current review asks for the CompactModel picker to skip the update path when the selected value is already the current value, including the unset/null case. The current handler still closes the submenu and then unconditionally marks the settings dirty, callssaveGlobalConfig, recordssetChanges, and emitstengu_compact_model_changed. Re-selecting the current compact model therefore produces a false pending settings change and a misleading analytics event. Please mirror the no-op guard used by the teammate default model path before settingisDirty.current.
Addresses round-2 review feedback on Twigpine#1629: - compact.ts: pass compactModel ?? mainLoopModel to isToolSearchEnabled so the tool-search capability check matches the model actually used for streaming compaction (not always mainLoopModel when a compact model is set) - Config.tsx: mirror the teammateDefaultModel no-op guard — return early when compactModel is unset and the picker confirms null (no-op selection) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the current head and found one issue that still needs to be addressed.
Findings
- [P3] Complete CodeRabbit's request to skip no-op compact model selections
src/components/Settings/Config.tsx:1616
CodeRabbit's review item asked the CompactModel picker to skip the update path when the selected value is already the current value, including the unset/default case. The current guard only handlescompactModel === undefined && model_2 === null; if a user opens the picker whilecompactModelis already set to a model and confirms that same model, the handler still marks the dialog dirty, callssaveGlobalConfig, records a pending change, and emitstengu_compact_model_changed. Please complete that review request by comparing the normalized current value (globalConfig.compactModel ?? null) tomodel_2before settingisDirty.current.
Compare globalConfig.compactModel ?? null against the picker's selection so re-confirming the current value (including explicit "Default"/null when a model was previously set) is treated as a no-op instead of marking settings dirty.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the current head and found one issue that still needs to be addressed.
Findings
- [P2] Resolve compactModel aliases before compaction
src/services/compact/compact.ts:1413
The new CompactModel picker reusesModelPicker, whose first-party options return alias values likesonnet,opus, andsonnet[1m]; the handler stores that value directly inglobalConfig.compactModel. The compaction path then uses that raw value forisToolSearchEnabled,getMaxOutputTokensForModel, andqueryModelWithStreaming, and the API normalizer only strips[1m]rather than expanding aliases. So a normal user selection such as Sonnet can make the next compaction sendmodel: "sonnet"instead of the resolved model ID, and the current regression test misses this because it uses a full model ID. Please normalize the stored compact model with the sameparseUserSpecifiedModelpath used by the main model before comparing or sending it, and add coverage for an alias value from the picker/config path.
ModelPicker stores alias strings (e.g. 'sonnet') directly in globalConfig.compactModel. Compact reads that value and must call parseUserSpecifiedModel() to expand it to the canonical model ID before comparing against mainLoopModel or sending to the API. Two read-sites in compactConversation and streamCompactSummary are both fixed. Test updated so the 'skips cache-sharing' case uses the resolved model ID (legacy claude-opus-4-1 remaps to current default) and a new test verifies alias expansion end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the current head and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
Cherry-picked from upstream 0c45e16. Adds: - compactModel? field on GlobalConfig (defaults to mainLoopModel) - CompactModel submenu in /config with ModelPicker - resolveCompactModel helper respecting model API config gates - parseUserSpecifiedModel integration for mainLoopModel resolution OC-specific changes: - config.ts: kept OC's logoColor/maxMessagesCompactionThreshold alongside UP's compactModel - compact.test.ts: skipped GitHub Native Anthropic mode test (legacy provider per AGENTS.md) Includes 2 new tests for compactModel override behavior.
Goal: bring `bun run typecheck` to 0 errors on main-opencc. Five files touched, no behavior changes: - config.ts: cherry-pick da7bc4a (Twigpine#1629) referenced MaxMessagesCompactionThreshold without importing the type. Add the type, threshold constant tuple, and normalize helper from PR Twigpine#1605. - Config.tsx: drop dead `&& pendingScope` clause at line 1666; showSubmenu === 'ExternalIncludes' guard already gates the dialog. - initReplBridge.ts: cast msg.message.content at line 293 — UserMessage stores response shape (ContentBlock[]) but getContentText expects request shape (ContentBlockParam[]); cast via Parameters<>[0]. - compact.test.ts: helpers must include top-level `content` per Message interface; rewrite mock to handle optional `msg.message?.content`. - sessionStorage.test.ts: 5 tests reference upstream-only goalStates (loadTranscriptFile) and `goal` arg (restoreSessionMetadata) — OC uses appState.goalSentinel instead. Convert to test.skip + no-op placeholder per AGENTS.md skip-legacy-test pattern. Build + impacted test suites verified green.
3 participants
Adds a
compactModelconfig key so compaction can use a different(e.g. cheaper or faster) model than
mainLoopModel.What changed
src/utils/config.ts: addedcompactModel?: stringtoGlobalConfigand
GLOBAL_CONFIG_KEYS.src/services/compact/compact.ts:compactModeldirectly fromgetGlobalConfig()(no threadingthrough
ToolUseContext.options— minimal footprint).modelChangesForCompactionguard in bothcompactConversation()and
streamCompactSummary(): whencompactModelis set and differsfrom
mainLoopModel, bypass the forked-agent cache-sharing path(guaranteed miss with a different model anyway).
compactModel(falling back tomainLoopModelwhen unset) toqueryModelWithStreaming(), including themaxOutputTokensOverrideguard.
Behaviour
compactModelunset → no behaviour change (existing path preserved).compactModel === mainLoopModel→ no behaviour change.compactModelset and different → cache-sharing bypassed; streamingfallback runs with
compactModel.Closes #1445
Summary by CodeRabbit