Model registry: deprecation fallback, mode presets, reasoning-effort, alias patterns - #107
Conversation
… alias patterns Module 2 of the runtime-core split (off main, after #105). Scope: internal/modelregistry only — fully additive (the whole tree builds unchanged against it). - Regex MatchPatterns aliases + pattern-based Resolve. - DeprecationRule with FallbackID; ResolveWithFallback returns the active fallback model + a user notice for deprecated ids; NewRegistry validates every Deprecation.FallbackID resolves (fail-fast on misconfig). - ReasoningEffort support: per-model ReasoningEfforts + DefaultReasoningEffort (validated to be a member of the model's set) + EffectiveReasoningEffort(). - Mode presets (modes.go: smart/deep/fast/large/precise -> model id + reasoning effort). These are the registry-capability layer; the CLI/agent adoption (--model deprecation-fallback, --mode, -r/--reasoning-effort, enhanced 'zero models') lands in a later module to keep PRs reviewable. Build/vet/full-suite/-race green; no new deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughThis PR extends the model registry with deprecation support, fuzzy resolution via regex patterns, preset operating modes, and per-model default reasoning effort. ModelEntry gains three new fields (DefaultReasoningEffort, MatchPatterns, Deprecation), the Registry compiles patterns at construction time and validates cross-entry dependencies, and a new Modes system provides five presets (smart, deep, fast, large, precise) with integrated reasoning effort and tool filter management. ChangesModel Registry Deprecation & Preset Modes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/modelregistry/modes_test.go (1)
41-50: ⚡ Quick winExtend test to verify slice field defensive copying.
Test only mutates
Name, butcloneModespecifically deep-copiesEnabledToolsandDisabledTools. Should verify those are also independent.Proposed extension
func TestModesReturnsIndependentCopies(t *testing.T) { modes := Modes() if len(modes) == 0 { t.Fatal("Modes() returned no presets") } + // Test struct field mutation modes[0].Name = "mutated" if again := Modes(); again[0].Name == "mutated" { t.Fatal("Modes() should return defensive copies, not shared state") } + // Test slice field mutation (even though current modes have nil slices, + // cloneMode explicitly handles this) + modes = Modes() + modes[0].EnabledTools = []string{"injected"} + modes[0].DisabledTools = []string{"injected"} + again := Modes() + if len(again[0].EnabledTools) > 0 || len(again[0].DisabledTools) > 0 { + t.Fatal("Modes() should deep-copy tool filter slices") + } }🤖 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 `@internal/modelregistry/modes_test.go` around lines 41 - 50, Update TestModesReturnsIndependentCopies to also verify that slice fields are defensively copied: after calling modes := Modes(), mutate modes[0].EnabledTools and modes[0].DisabledTools (e.g., append new entries and/or modify existing elements) and then call again := Modes() and assert that again[0].EnabledTools and again[0].DisabledTools have not been changed; this ensures cloneMode's deep copy of EnabledTools and DisabledTools is effective.
🤖 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.
Nitpick comments:
In `@internal/modelregistry/modes_test.go`:
- Around line 41-50: Update TestModesReturnsIndependentCopies to also verify
that slice fields are defensively copied: after calling modes := Modes(), mutate
modes[0].EnabledTools and modes[0].DisabledTools (e.g., append new entries
and/or modify existing elements) and then call again := Modes() and assert that
again[0].EnabledTools and again[0].DisabledTools have not been changed; this
ensures cloneMode's deep copy of EnabledTools and DisabledTools is effective.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 657bd2e6-7d8f-4ed6-ba90-735f53b3dcaa
📒 Files selected for processing (7)
internal/modelregistry/catalog.gointernal/modelregistry/catalog_test.gointernal/modelregistry/models.gointernal/modelregistry/modes.gointernal/modelregistry/modes_test.gointernal/modelregistry/resolve.gointernal/modelregistry/resolve_test.go
anandh8x
left a comment
There was a problem hiding this comment.
What's good
-
Decorator pattern keeps the catalog terse.
decorateModelDepthseparates the base catalog (id, name, capabilities, cost) from the depth metadata (patterns, defaults, deprecation). The baseDefaultModelEntriesstays readable, the depth wiring is easy to audit, and the layering is explicit. Adding a new depth field (e.g., per-model cost tier overrides) is a one-entry change in the depth map. -
Cross-entry validation in
NewRegistryis the right place. "Deprecation fallback must resolve to a real model" is checked at construction time, so a typo or missing model fails loudly at startup rather than silently redirecting to nothing at runtime. The error message includes both the deprecated model and the missing fallback id, which makes the failure mode actionable. -
DeprecationRule.Clone()is a real defensive helper. Deep-copying a struct with a pointer field requires explicit cloning.cloneModelEntryusesentry.Deprecation.Clone()to ensure catalog callers can't mutate the registry's internal state by holding onto a returnedDeprecation. TheTestModesReturnsIndependentCopiestest (for the siblingModetype) confirms the same defensive-copy pattern works. -
ResolvevsResolveWithFallbackseparation is correct.Resolveis the pure "find the model" function (exact id/api-model/alias, then regex pattern).ResolveWithFallbackadds the deprecation redirect and the user-facing notice. Callers who don't want a redirect (e.g., a UI listing active models) useResolve; callers who want the redirect (e.g., the CLI exec command) useResolveWithFallback. Both are tested independently. -
EffectiveReasoningEffortcascade is right. Requested if supported → model default → first supported →ReasoningEffortNone. The four-step cascade handles every case.TestEffectiveReasoningEffortasserts: high supported returns high; xhigh unsupported falls back to default low; empty requested falls back to default low. The cascade is also exercised byTestEveryModeResolvesToRealRegistryModel— every mode's requested effort must be supported by the resolved model, so a mode never silently downgrades on apply. -
Fuzzy match patterns are anchored regexes. Patterns like
(?i)^sonnet[^a-z0-9]*4[\.\s]?5$are anchored to start and end, so "sonnet 4.5 turbo" wouldn't match.[^a-z0-9]*handles spaces, dots, dashes, and other delimiters.[.\s]?allows "4.5" or "4 5".TestResolveRegexAliasconfirms "claude-sonnet-4-5" (id), "sonnet-4.5" (alias), "Sonnet 4.5" (mixed case), and "sonnet4.5" (no separator) all resolve. -
Default reasoning effort is validated to be in the model's supported list.
Validate()checks at construction time, andTestDefaultRegistryReasoningModelsHaveDefaultEffortasserts the runtime contract: sonnet-4.5's default is in its supported efforts, etc. A typo in the decorator would fail atNewRegistrytime, not at user request time. -
Mode presets are data-driven.
defaultModesis a slice ofModestructs. Adding a new mode is one struct literal. The 5 modes cover the realistic use cases (smart daily driver, deep reasoning, fast edits, long context, precise work) and each has a description. -
Mode.MaxTurns = 0semantics are documented as "leave the configured/default turn budget untouched". ThecloneModehelper preserves the field on copy. A future slice that actually applies modes will need to respect this contract. -
The notice logic in
ResolveWithFallbackis correct. Three branches: redirected (uses deprecation warning or generates one), deprecated with warning but no fallback (returns warning), active (empty notice).TestResolveWithFallbackActiveNoNoticeconfirms active models get no notice. The fallback warning is generated ifWarningMsgis empty:"X is deprecated; using Y instead". -
ModelStatusDeprecatedis the trigger for redirect. A model withStatus: ModelStatusActiveand aDeprecationrule wouldn't redirect — only the status drives behavior. That's the right call: a rule is just metadata; the status is the action. -
Test coverage is comprehensive. 13 new test functions across 3 new test files. Every new API is tested. The decorator is tested transitively via the integration tests. The defensive copy is tested. The case-insensitive + trimmed lookup is tested. The deprecation redirect is tested. The fuzzy match patterns are tested. The unsupported effort fallback is tested.
-
The
cloneModelEntryandcloneModehelpers are correctly used inDefaultModelEntriesandModes(). The pattern is consistent: build a private catalog, deep-copy before returning. -
Doc comments on the new types are clear and actionable.
DeprecationRulesays "FallbackID is required; the date/warning fields are advisory".Modesays "Effort of '' means 'let the model's effective default apply'".EffectiveReasoningEffortsays "Requested if the model supports it, otherwise the model's default (or first supported, or none)". A future maintainer can read the doc and understand the contract. -
The catalog depth wiring addresses a real product need. Deprecation of
claude-haiku-3.5andgpt-4-turbowas a flag in my prior PR review (#56) — the deprecation was set but no fallback. This slice wires the fallback and the notice, and theTestDefaultRegistryDeprecatedModelsRedirecttest locks it. The fuzzy aliases ("sonnet 4.5") are also a real UX improvement over requiring exact canonical ids.
Observations (non-blocking)
-
Fuzzy match patterns are catalog-only, not user-extensible. A user can't add their own pattern for a custom model.
NewRegistryaccepts patterns viaModelEntry.MatchPatterns, but the canonical catalog is hard-coded. A future enhancement could let users add patterns via config. -
Deprecation dates are strings (ISO-8601), not
time.Time.SoftDateandHardDateare documentation only — no logic checks againsttime.Now(). A future enhancement could add "past soft date? warn but resolve" vs "past hard date? error". Currently both are advisory. -
DeprecationRule.HardDateis present but unused in the logic. A future slice could enforce it (e.g., return an error fromResolveif past hard date). -
No test for active model with a deprecation warning (no redirect). The current
TestResolveWithFallbackActiveNoNoticecovers active-no-deprecation. An active model with aDeprecation.WarningMsg(but noModelStatusDeprecated) would return the warning as a notice. A test would lock the contract. -
No test for
Resolvewith overlapping patterns across models. The current catalog has non-overlapping patterns, so order doesn't matter. A future validation could detect overlaps atNewRegistrytime (or at least a test that demonstrates the first-match-wins behavior). -
EffectiveReasoningEffortcould be a method onModelEntry. A methodentry.EffectiveReasoningEffort(requested)would be more discoverable than a free function. The cascade is data-bound to the entry, so method semantics fit. -
reasoningEffortAllowedInhelper is duplicated. The test file defines a local helper for the same logic thatValidateuses inline. AmodelHasReasoningEffort(model, effort)helper would deduplicate. -
No benchmark for
ResolveorResolveWithFallback. These are called on every model lookup, so a small benchmark would catch future regressions where someone adds a linear scan over patterns. -
The
defaultModesorder is not asserted by name.TestModeNamesMatchCatalogOrderasserts the order matchesModes()but doesn't lock a specific order (smart, deep, fast, large, precise). A future test could assert the exact order so a reorder is intentional. -
The
smartandprecisemodes both useclaude-sonnet-4.5. The difference is the effort (Medium vs High). A doc note explaining the distinction would help users pick. -
Mode.EnabledTools/DisabledToolsare present but unused in the default modes. ThecloneModealready deep-copies them. A future slice could wire modes to tool filters. -
The
decorateModelDepthfunction mutates entries in place. A future refactor could return a new slice for safety. The current code is correct (the slice is the local one fromDefaultModelEntries), but the mutation pattern is subtle. -
The notice logic in
ResolveWithFallbackhas 3 branches (redirected, deprecated-with-warning, neither). A future refactor could collapse to 2 by treating "redirected" as a special case of "deprecated with fallback". -
No test for
Resolvewith empty input.strings.TrimSpace("")is"", so the regex^$would match a pattern like^$(none of the default patterns do). A test could lock the behavior (no match). -
No test for
Resolvewith a model that hasMatchPatternsbut no aliases. A model with patterns but no aliases would still be resolvable by regex. The current tests use both. A focused test would document the contract. -
The doc comment on
decorateModelDepthcould mention it's catalog-only — user registries from a custom[]ModelEntrywouldn't get the depth wiring unless they call a publicDecorateDepthhelper. -
The
MatchPatternsare checked in registration order. A future validation could detect overlaps atNewRegistrytime. The current code accepts overlaps silently. -
The
TestResolveRegexAliastest covers 4 input variants (id, alias, mixed case, no separator). A test for partial-match rejection (e.g., "sonnet 4.5 turbo" should NOT match) would lock the anchor behavior. -
The
MatchPatternstest uses(?i)sonnet[^a-z0-9]*4[.\s]?5(no anchors) inresolveTestRegistry. This is intentional (the helper tests the pattern in isolation, not the anchored catalog pattern). The catalog uses anchored patterns. -
The
Decoratefunction is not exposed as a public helper. A future slice could exportDecorateModelDepth(entries []ModelEntry) []ModelEntryfor users building custom catalogs. -
The
cloneModehelper is similar tocloneModelEntrybut for a different type. A future refactor could unify them, but they have different field shapes (Mode has no costs/context limits). -
The
ModelEntry.Validatechecks theDefaultReasoningEffortis inReasoningEffortsvia a manual loop. AmodelHasReasoningEfforthelper (or inlining the test helper) would deduplicate.
Approving — this is a well-architected slice. The decorator pattern keeps the catalog terse, the cross-entry validation catches typos at startup, the Resolve/ResolveWithFallback separation is correct, the EffectiveReasoningEffort cascade handles every case, the fuzzy match patterns are anchored, and the mode presets are data-driven. The test coverage is comprehensive (13 new tests across 3 new test files). The deprecation wiring closes a real gap from prior slices (PR #56 had deprecation but no fallback). The follow-ups are minor (user-extensible patterns, time.Time dates, active model with deprecation notice contract, order lock test, benchmark).
…eractive-command detection (#109) * Sandbox hardening: destructive/network/installer classification + interactive-command detection Module 3 of the runtime-core split (off main, after #107). Scope: internal/sandbox only — clean additive extract; no new deps; whole tree builds + full suite/-race green. LIVE (Classify is already called by sandbox.Evaluate and agent/loop.go, so this hardens existing behavior immediately): - rm -rf targeting / $HOME ~ * — now tolerant of surrounding quotes, ${HOME} braces, combined/reordered -r/-f flags, and an optional -- separator; chmod 777 only flagged when recursive or root-targeted (single-file chmod no longer false-positives). - network-command + piped-installer detection (curl|sh incl |zsh and other shells); command resolved across command/cmd/script/shell aliases so an alias key can't bypass the gate. FORWARD (compiles now, wired by the later tools/bash module): DetectInteractiveCommand flags interactive programs (vim/less/...) behind wrappers (sudo/nice/timeout, and sh -c/bash -c payloads), hardened against absolute-path/quote/escape/substitution bypasses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address #109 review (CodeRabbit): fix sandbox detection over/under-reach - [critical] rm long-flag root: --no-preserve-root -rf -- "/" and ... -rf "/" now caught (quote + -- handling restored across short AND long flags). - chmod 777 abs-path narrowed to root or a sensitive SYSTEM tree (/, /etc, /usr, …); a single-file abs path like chmod 777 /tmp/build.sh is no longer a false positive. - piped_installer now requires a remote fetch (curl/wget/fetch/aria2c) before the pipe; a local 'cat x | bash' / 'printf | sh' is no longer mis-flagged (engine_test updated to assert the corrected semantics). - nonInteractiveREPLFlags adds mongo/mongosh (--eval/-f/--file) so 'mongo --eval ...' isn't flagged interactive. - programIndex now normalizes tokens (basename/quote-strip/lowercase) like firstProgram, so full-path invocations (/usr/bin/python script.py, /bin/bash -c 'vim …') classify correctly (no false positives / missed nested detections). TDD for each; build/vet/-race/windows cross-compile/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: KRATOS <kratos@KRATOSs-Mac-mini.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Module 2 of N — Model registry
Second reviewable slice of the runtime-core split (off
main, after #105 merged). Scope:internal/modelregistryonly (7 files, ~556 lines incl. tests). Fully additive — the whole tree builds unchanged against it; no new deps.What's in it
MatchPatterns+ pattern-basedResolveso models resolve from provider-qualified ids, short aliases, and patterns.DeprecationRule{FallbackID, WarningMsg};ResolveWithFallbackreturns the active fallback model plus a user-facing notice for deprecated ids.NewRegistryvalidates that everyDeprecation.FallbackIDresolves to a real model — a misconfigured rule fails at startup instead of being silently ignored.ReasoningEfforts+DefaultReasoningEffort(validated to be a member of the model's supported set) +EffectiveReasoningEffort().modes.go):smart/deep/fast/large/precise→ concrete model id + reasoning effort.Scope note
This is the registry-capability layer. The CLI/agent adoption of it —
--modeldeprecation-fallback, the--modeflag,-r/--reasoning-effort, and the enhancedzero modelstable — lands in a later module so each PR stays reviewable. The new functions are exercised by the package's own tests (resolve_test.go,modes_test.go,catalog_test.go).Testing
go build ./...,go vet ./...,go test ./..., andgo test -race ./internal/modelregistry/all green.Part of decomposing #101 (draft) into reviewable PRs; subagents excluded.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests