fix: make model customizations configurable and update models to latest#572
Conversation
- Fix UNKNOWN_N identifier mapping in systemPromptSync that caused "UNKNOWN_3 is not defined" ReferenceError when entering plan mode - Add settings.misc.enableModelCustomizations toggle (default true) - Move model-customizations and show-more-items-in-select-menus to Misc Configurable group, gated by the new setting - Update model catalog (add Sonnet 4.6, Haiku 4.5) and model descriptions from 4.5 to 4.6 - Widen modelSelector lookback window and generalize declaration pattern
📝 WalkthroughWalkthroughThis PR introduces a new configurable feature gate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/patches/opusplan1m.ts (1)
214-230:⚠️ Potential issue | 🟠 MajorPreserve wrapper semantics when injecting the
opusplan[1m]branch.Lines 214-230 match both wrapped and unwrapped return forms, but the injected branch always returns a raw array. For wrapped builds (e.g.,
return v1A([...])), this changes runtime behavior and can break selector construction.Proposed fix
- const pattern = - /(if\s*\(\s*([$\w]+)\s*===\s*"opusplan"\s*\)\s*return\s*(?:[$\w]+\()?\[\s*\.\.\.([$\w]+)\s*,\s*([$\w]+)\(\)\s*\]\)?;)/; + const pattern = + /(if\s*\(\s*([$\w]+)\s*===\s*"opusplan"\s*\)\s*return\s*(?:([$\w]+)\()?\[\s*\.\.\.([$\w]+)\s*,\s*([$\w]+)\(\)\s*\]\)?;)/; - const [fullMatch, , varName, listVar] = match; + const [fullMatch, , varName, wrapperFn, listVar] = match; - const replacement = - fullMatch + - `if(${varName}==="opusplan[1m]")return[...${listVar},{value:"opusplan[1m]",label:"Opus Plan Mode 1M",description:"Use Opus 4.6 in plan mode, Sonnet 4.6 (1M context) otherwise"}];`; + const injectedList = `[...${listVar},{value:"opusplan[1m]",label:"Opus Plan Mode 1M",description:"Use Opus 4.6 in plan mode, Sonnet 4.6 (1M context) otherwise"}]`; + const injectedReturn = wrapperFn + ? `${wrapperFn}(${injectedList})` + : injectedList; + const replacement = + fullMatch + `if(${varName}==="opusplan[1m]")return${injectedReturn};`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/opusplan1m.ts` around lines 214 - 230, The injected branch always returns a raw array, breaking builds that use a wrapper like return wrapperName([...]) captured by the original regex; update the replacement to preserve wrapper semantics: inspect fullMatch to see if it contains a wrapper call (e.g. match return\s*([$\w]+)\(\s*\[\.\.\.${listVar}) and if present inject the new element inside that wrapper (return wrapperName([...${listVar}, {value:"opusplan[1m]",label:"Opus Plan Mode 1M",description:"..."}]);), otherwise keep the raw array return (return [...${listVar}, {...}]); use the existing variables varName, listVar and fullMatch to build the conditional replacement so wrapped and unwrapped forms behave identically.
🧹 Nitpick comments (3)
src/tests/config.test.ts (1)
169-188: Add a regression test for explicitfalsepreservation.Lines 169-188 cover missing-key backfill, but a companion case should assert that an explicitly configured
enableModelCustomizations: falseremainsfalseafterreadConfigFile().Based on learnings: Applies to **/*.test.{ts,tsx} : Test edge cases and error conditions in test files.Suggested test addition
+ it('should preserve enableModelCustomizations=false when explicitly set', async () => { + const mockConfig = { + ccVersion: '1.0.0', + ccInstallationPath: null, + lastModified: '2024-01-01', + changesApplied: true, + settings: { + ...DEFAULT_SETTINGS, + misc: { + ...DEFAULT_SETTINGS.misc, + enableModelCustomizations: false, + }, + }, + }; + + vi.spyOn(fs, 'readFile').mockResolvedValue(JSON.stringify(mockConfig)); + const result = await readConfigFile(); + + expect(result.settings.misc.enableModelCustomizations).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/config.test.ts` around lines 169 - 188, Add a new test that mirrors the missing-key backfill case but ensures an explicit false is preserved: create a mockConfig using DEFAULT_SETTINGS where settings.misc.enableModelCustomizations is set to false, spy on fs.readFile to return that config, call readConfigFile(), and assert result.settings.misc.enableModelCustomizations === false; reference readConfigFile, DEFAULT_SETTINGS and the misc.enableModelCustomizations field to locate where to add the test.src/patches/modelCustomizationsToggle.test.ts (2)
92-95: Deduplicate repeated patch-id arrays into a shared constant.The same patch list is repeated in three tests; extracting it once will
reduce drift when IDs change.Also applies to: 117-120, 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/modelCustomizationsToggle.test.ts` around lines 92 - 95, Extract the repeated patch-id array used in the three tests into a single shared constant (e.g. const PATCH_IDS = ['model-customizations','show-more-items-in-select-menus']) at the top of the test suite and replace the inline array arguments passed to applyCustomization(config, ccInstInfo, [...]) in each test with PATCH_IDS; ensure all three occurrences (the calls that assign { results } from applyCustomization) use the new constant and run tests to confirm no breakage.
88-147: Add explicit error-path coverage for patch pipeline failures.Current tests cover enabled/disabled paths, but not failure conditions
(e.g., Line 129/Line 130 patch writers returningnull, or plumbing
failures around Line 144-Line 146). Add at least one failure-path test
to lock expected fallback behavior and avoid silent regressions.As per coding guidelines, "Test edge cases and error conditions in test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/modelCustomizationsToggle.test.ts` around lines 88 - 147, Add a new test case that simulates a failure path by mocking writeModelCustomizations and/or writeShowMoreItemsInSelectMenus to return null (or throw) and/or mocking plumbing helpers (restoreClijsFromBackup, applySystemPrompts, updateConfigFile) to throw; then call applyCustomization and assert the corresponding result entries (found via results.find) have failed: true and skipped:false (or applied:false as appropriate), verify that replaceFileBreakingHardLinks is still invoked with the expected args as the fallback, and verify plumbing teardown/restore behavior (restoreClijsFromBackup, applySystemPrompts, updateConfigFile) is called or handled as expected; use the same identifiers from the diff (applyCustomization, writeModelCustomizations, writeShowMoreItemsInSelectMenus, replaceFileBreakingHardLinks, restoreClijsFromBackup, applySystemPrompts, updateConfigFile) so the test pins down error-path behavior and prevents silent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/patches/opusplan1m.ts`:
- Around line 214-230: The injected branch always returns a raw array, breaking
builds that use a wrapper like return wrapperName([...]) captured by the
original regex; update the replacement to preserve wrapper semantics: inspect
fullMatch to see if it contains a wrapper call (e.g. match
return\s*([$\w]+)\(\s*\[\.\.\.${listVar}) and if present inject the new element
inside that wrapper (return wrapperName([...${listVar},
{value:"opusplan[1m]",label:"Opus Plan Mode 1M",description:"..."}]);),
otherwise keep the raw array return (return [...${listVar}, {...}]); use the
existing variables varName, listVar and fullMatch to build the conditional
replacement so wrapped and unwrapped forms behave identically.
---
Nitpick comments:
In `@src/patches/modelCustomizationsToggle.test.ts`:
- Around line 92-95: Extract the repeated patch-id array used in the three tests
into a single shared constant (e.g. const PATCH_IDS =
['model-customizations','show-more-items-in-select-menus']) at the top of the
test suite and replace the inline array arguments passed to
applyCustomization(config, ccInstInfo, [...]) in each test with PATCH_IDS;
ensure all three occurrences (the calls that assign { results } from
applyCustomization) use the new constant and run tests to confirm no breakage.
- Around line 88-147: Add a new test case that simulates a failure path by
mocking writeModelCustomizations and/or writeShowMoreItemsInSelectMenus to
return null (or throw) and/or mocking plumbing helpers (restoreClijsFromBackup,
applySystemPrompts, updateConfigFile) to throw; then call applyCustomization and
assert the corresponding result entries (found via results.find) have failed:
true and skipped:false (or applied:false as appropriate), verify that
replaceFileBreakingHardLinks is still invoked with the expected args as the
fallback, and verify plumbing teardown/restore behavior (restoreClijsFromBackup,
applySystemPrompts, updateConfigFile) is called or handled as expected; use the
same identifiers from the diff (applyCustomization, writeModelCustomizations,
writeShowMoreItemsInSelectMenus, replaceFileBreakingHardLinks,
restoreClijsFromBackup, applySystemPrompts, updateConfigFile) so the test pins
down error-path behavior and prevents silent regressions.
In `@src/tests/config.test.ts`:
- Around line 169-188: Add a new test that mirrors the missing-key backfill case
but ensures an explicit false is preserved: create a mockConfig using
DEFAULT_SETTINGS where settings.misc.enableModelCustomizations is set to false,
spy on fs.readFile to return that config, call readConfigFile(), and assert
result.settings.misc.enableModelCustomizations === false; reference
readConfigFile, DEFAULT_SETTINGS and the misc.enableModelCustomizations field to
locate where to add the test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/defaultSettings.tssrc/patches/index.tssrc/patches/modelCustomizationsToggle.test.tssrc/patches/modelSelector.tssrc/patches/opusplan1m.tssrc/systemPromptSync.tssrc/tests/config.test.tssrc/types.tssrc/ui/components/MiscView.tsx
- Detect wrapper function (e.g. v1A) in fullMatch and preserve it in the injected opusplan[1m] return expression - Extract repeated patch ID array into shared PATCH_IDS constant - Add failure-path test for when patch functions return null - Add config test preserving explicit false for enableModelCustomizations
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/patches/modelCustomizationsToggle.test.ts (1)
93-173: Add explicit coverage for theunsettoggle state.Current tests validate
trueandfalse, but not a missing
enableModelCustomizationskey in runtime config for this flow.🧪 Suggested test case
+ it('treats missing enableModelCustomizations as enabled', async () => { + const config = baseConfig(); + delete (config.settings.misc as Record<string, unknown>) + .enableModelCustomizations; + + const { results } = await applyCustomization(config, ccInstInfo, [ + ...PATCH_IDS, + ]); + + expect(results.find(r => r.id === 'model-customizations')).toMatchObject({ + applied: true, + skipped: false, + }); + expect( + results.find(r => r.id === 'show-more-items-in-select-menus') + ).toMatchObject({ + applied: true, + skipped: false, + }); + });As per coding guidelines: Test edge cases and error conditions in test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/modelCustomizationsToggle.test.ts` around lines 93 - 173, Add a new test case that verifies behavior when config.settings.misc.enableModelCustomizations is unset: create a config from baseConfig() and delete or omit the enableModelCustomizations key, call applyCustomization(config, ccInstInfo, [...PATCH_IDS]), then assert that the 'model-customizations' and 'show-more-items-in-select-menus' results are skipped/ not applied, that writeModelCustomizations and writeShowMoreItemsInSelectMenus were not called, and that replaceFileBreakingHardLinks was still invoked as in the other "skips" test; reference symbols: enableModelCustomizations, applyCustomization, PATCH_IDS, writeModelCustomizations, writeShowMoreItemsInSelectMenus, replaceFileBreakingHardLinks.src/patches/opusplan1m.ts (1)
113-115: Tighten the description regex for deterministic patch targeting.The
.{0,20}fragments are broader than needed and can increase accidental
matches in minified targets. Prefer explicit version-token matching.♻️ Proposed refactor
- const pattern = - /(if\s*\(\s*([$\w]+)\s*===\s*"opusplan"\s*\)\s*return\s*"Opus .{0,20} in plan mode, else Sonnet .{0,20}";)/; + const pattern = + /(if\s*\(\s*([$\w]+)\s*===\s*"opusplan"\s*\)\s*return\s*"Opus \d+(?:\.\d+){0,2} in plan mode, else Sonnet \d+(?:\.\d+){0,2}";)/;Based on learnings: In patches under
src/patches/, patch regexes should be strict and match the minified format exactly (ignoring whitespace/newlines).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/opusplan1m.ts` around lines 113 - 115, The current regex in the pattern variable uses .{0,20} for both "Opus" and "Sonnet" descriptions which is too permissive; update the regex literal (the pattern constant) to replace each .{0,20} with a strict version-token class such as v?\d+(?:\.\d+)* or a limited charset like [A-Za-z0-9._-]{1,20} so it only matches version-like tokens (keep the existing surrounding structure, parentheses and \s* to tolerate minification/whitespace); ensure both occurrences are tightened and the overall regex still uses the same delimiter and flags so it continues to match the minified format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/patches/modelCustomizationsToggle.test.ts`:
- Around line 93-173: Add a new test case that verifies behavior when
config.settings.misc.enableModelCustomizations is unset: create a config from
baseConfig() and delete or omit the enableModelCustomizations key, call
applyCustomization(config, ccInstInfo, [...PATCH_IDS]), then assert that the
'model-customizations' and 'show-more-items-in-select-menus' results are
skipped/ not applied, that writeModelCustomizations and
writeShowMoreItemsInSelectMenus were not called, and that
replaceFileBreakingHardLinks was still invoked as in the other "skips" test;
reference symbols: enableModelCustomizations, applyCustomization, PATCH_IDS,
writeModelCustomizations, writeShowMoreItemsInSelectMenus,
replaceFileBreakingHardLinks.
In `@src/patches/opusplan1m.ts`:
- Around line 113-115: The current regex in the pattern variable uses .{0,20}
for both "Opus" and "Sonnet" descriptions which is too permissive; update the
regex literal (the pattern constant) to replace each .{0,20} with a strict
version-token class such as v?\d+(?:\.\d+)* or a limited charset like
[A-Za-z0-9._-]{1,20} so it only matches version-like tokens (keep the existing
surrounding structure, parentheses and \s* to tolerate minification/whitespace);
ensure both occurrences are tightened and the overall regex still uses the same
delimiter and flags so it continues to match the minified format.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/patches/modelCustomizationsToggle.test.tssrc/patches/opusplan1m.tssrc/tests/config.test.ts
|
Hi @liafonx, thank you for these fixes! The Everything else—the model selector changes, the configuration option for them, the opusplan[1m] changes—looks great; thank you! |
Hi @bl-ue , I am using v2.1.63. The reason for that fix is that when I enable opusplan mode (even without [1m]), then switch to plan mode and send a message, my request gets silently dropped with an error:
Strangely, this only happens when I switch to plan mode. Even after switching back, changing the model from opus to sonnet, the issue persists. Only restarting Claude Code resolves it. After investigating with Claude by checking logs and tweaked binary, I found that was the cause of the issue. However, as you said, since I haven’t examined the entire injection system, The |
|
@liafonx Okay, thank you, that's helpful. To help me debug, will you please trying using vanilla tweakcc without your patch and produce the broken binary, and then run Will you please also revert the |
liafonx
changed the title
|
Hi @bl-ue, done — reverted the UNKNOWN_* fallback in systemPromptSync.ts as requested. The PR now only contains the model customizations toggle and model catalog updates. Here is the reproduction as requested: Environment: CC v2.1.63, vanilla Steps: Error output: {"type":"result","subtype":"error_during_execution","is_error":true,"errors":["UNKNOWN_3 is not defined"]}Broken JS (extracted via The gist contains the full 11MB patched JS. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/patches/index.ts (1)
608-610: Remove newly added inline comments in this TS file.Please drop these new comments to match repo style rules.
Suggested diff
- // Keep model list customization and select-menu size behavior in sync. - // Disabling model customizations should restore both selectors to vanilla CC behavior. const modelCustomizationsEnabled = config.settings.misc?.enableModelCustomizations ?? true;As per coding guidelines "Do not add comments unless explicitly requested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/index.ts` around lines 608 - 610, Remove the newly added inline comment lines related to model list customization near the declaration of modelCustomizationsEnabled; specifically delete the "// Keep model list customization and select-menu size behavior in sync." and "// Disabling model customizations should restore both selectors to vanilla CC behavior." comments so the code conforms to the repo guideline "Do not add comments unless explicitly requested" and leave only the code (the const modelCustomizationsEnabled = ...) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/patches/index.ts`:
- Around line 608-610: Remove the newly added inline comment lines related to
model list customization near the declaration of modelCustomizationsEnabled;
specifically delete the "// Keep model list customization and select-menu size
behavior in sync." and "// Disabling model customizations should restore both
selectors to vanilla CC behavior." comments so the code conforms to the repo
guideline "Do not add comments unless explicitly requested" and leave only the
code (the const modelCustomizationsEnabled = ...) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b12a8cd-dd53-45e7-8117-64f074cee2e8
📒 Files selected for processing (4)
src/defaultSettings.tssrc/patches/index.tssrc/types.tssrc/ui/components/MiscView.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/MiscView.tsx
- src/defaultSettings.ts
- src/types.ts
2 participants
Why
Model customizations had no config/TUI opt-out.
What changed
settings.misc.enableModelCustomizations(defaulttrue) and wired Misc UI toggle.model-customizationsandshow-more-items-in-select-menusfrom Always Applied to Misc Configurable, gated by the new setting.let|var|const) for minifier variance.Validation
npx vitest run src/— 168 passed, 4 skippedpnpm lint— cleanbrew reinstall claude-codeReviewer checklist
enableModelCustomizationswork end-to-endtrue/false/unset)Summary by CodeRabbit
New Features
Tests