Repository navigation
feat(compact): add compactModel config to override compaction model - #1446
laurentftech wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix broken step numbering in system prompt (glob/grep as sub-bullets) - Remove unnecessary wrapper in getSystemPrompt - Drop unused beforeEach/afterEach lifecycle in tests - Remove redundant registration test (beforeAll already throws) - Fix CLAUDE_CONFIG_DIR leak in beforeAll (restore in finally) - Replace @ts-ignore with explicit ToolUseContext cast - Use placeholder in README agentRouting example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o gpt-4o)\n\nReplace placeholder model with gpt-4o so the example is immediately usable.
Allows using a fast API model for compaction when the main loop runs a slow local reasoning model. Skips the forked-agent cache-sharing path automatically when models differ (different model = cache miss). Closes Twigpine#1445 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I took a pass through the PR intent, the linked issue, the changed paths, and the related open code-reviewer-agent PR, and found issues that need to be addressed before this is ready.
Findings
-
[P2] Remove the unrelated duplicate code-reviewer agent changes
src/tools/AgentTool/built-in/codeReviewerAgent.ts:72
This PR is titled and described as addingcompactModelfor compaction, but it also adds the built-incode-revieweragent, its tests, README routing entry, and an.openlore/ignore rule. That agent work is already being reviewed separately in #1381, and this branch contains an older copy of it: for example, #1381 has since added shell-tool denials after review, while this PR still leavesBash/PowerShellavailable through wildcard agent tools despite the prompt's read-only guarantee. Please drop the code-reviewer/README/OpenLore-related changes from this compact-model PR and keep that work in the dedicated agent PR. -
[P2] Check tool-search support against the model that will actually compact
src/services/compact/compact.ts:1270
The streaming fallback now sends the request withmodel: compactModel, butisToolSearchEnabled()is still called withcontext.options.mainLoopModel. When the main model supports tool references and the configured compact model does not, this can enableToolSearchTooland deferred MCP-tool handling for a request that is then sent to the unsupported compact model. That makes the primary use case fragile: switching compaction to a fast override model can fail or shape the request incorrectly based on the unrelated main-loop model. Please make the tool-search decision usecompactModelconsistently for the compact request. -
[P2] Add a supported way to clear
compactModel
src/tools/ConfigTool/supportedSettings.ts:62
The linked issue says unsetcompactModelshould restore the existing behavior of usingmainLoopModel, and the PR body says "Unset to revert to default". The config tool path added here only supports get vs set; omittingvaluereads the setting, empty strings failvalidateModel, and the only"default"deletion path is hard-coded forremoteControlAtStartup. After a user setscompactModel, they cannot return to the default behavior through the documented config command without manually editing the global config file. Please add an explicit clear/default/unset path for this setting and cover it with a focused test.
- Drop code-reviewer agent (belongs in dedicated PR Twigpine#1381) - Fix isToolSearchEnabled to use compactModel, not mainLoopModel - Add clearable support so /config compactModel default unsets the key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the detailed review. Addressed all three P2 findings in 8499c32: [P2] Code-reviewer agent removed — dropped [P2] [P2] |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Resolve compactModel aliases before sending compaction requests
src/services/compact/compact.ts:1159
compactModelis validated with the samevalidateModelpath as the main model setting, and that validator explicitly accepts aliases such assonnet,opus, andhaikuas valid. Unlike the main-loop model path, though, this value is read straight from global config and passed toqueryModelWithStreamingasmodel: compactModel, and the API request only strips context tags rather than expanding aliases. That means/config set compactModel sonnetsucceeds, but the next compaction sendsmodel: "sonnet"instead of the resolved model ID, so the documented config surface can fail at runtime. Please normalize/parse the stored compact model the same way the main model path does before comparing it tomainLoopModeland before passing it to the API. -
[P3] Return the cleared value in the config-tool result
src/tools/ConfigTool/ConfigTool.ts:205
The new clearable-setting branch successfully deletescompactModel, but it returns onlyvalue: null. Both the visible ConfigTool UI andmapToolResultToToolResultBlockParamrender successful set operations fromcontent.newValue, so/config set compactModel defaultreportsSet compactModel to undefinedto the user and to the model even though the key was cleared. Please return the same result shape that the set path expects, and add a focused regression test for the clear/default case so the reset behavior and confirmation stay in sync.
Resolve compactModel aliases (sonnet/opus/haiku) via parseUserSpecifiedModel before sending to the API — raw aliases caused runtime failures since the API does not expand them. Fix clearable-setting result shape: return newValue:null instead of value:null so mapToolResultToToolResultBlockParam renders "Set compactModel to null" rather than "Set compactModel to undefined". Add ConfigTool regression tests covering set/clear/get/error rendering paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Both findings addressed in ed1aad0. [P2] Alias resolution — [P3] Clearable result shape — the |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P3] Coerce compactModel values before validation
src/tools/ConfigTool/supportedSettings.ts:68
The ConfigTool input schema still allowsvalueto be a string, boolean, or number, but the newcompactModelsetting passesv as stringdirectly intovalidateModel. That cast does not convert the runtime value, andvalidateModelimmediately callsmodel.trim(), so a malformed tool call such as{ "setting": "compactModel", "value": true }throwsTypeError: model.trim is not a functioninstead of returning a normal validation error. The existingmodelsetting avoids this by callingvalidateModel(String(v)); please apply the same coercion or explicitly reject non-string values before validation, and add a small regression test for the bad-value path.
|
closing as abandoned and newer #1629 has overlap |
Closes #1445
Summary
compactModel?: stringtoGlobalConfig— optional override for the model used during compaction/config set compactModel <model-id>with model validationcompactModeldiffers frommainLoopModel(different model = guaranteed cache miss)Motivation
When running a slow local reasoning model (e.g. Qwen3.6 27B) as the main loop, compaction blocks on that same slow model — even though summarization doesn't require reasoning. This allows routing compaction to a fast API model (e.g. Gemma 4) without affecting the main conversation model.
No cache tradeoff for local-model users: local inference has no prompt cache anyway.
Usage
Unset to revert to default (uses
mainLoopModel).Files changed
src/utils/config.tscompactModel?: stringfield +GLOBAL_CONFIG_KEYSregistrationsrc/tools/ConfigTool/supportedSettings.tsvalidateModelsrc/services/compact/compact.tscompactModelfrom config, disable forked path when model differs, pass override toqueryModelWithStreaming🤖 Generated with Claude Code