Resolve orchestration default models to latest-within-class via config table#561
Conversation
|
@codex review Focus on bugs, correctness issues, and edge cases. Do not check adherence to a spec or plan. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af8a2f3385
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-thread starts Codex review (PR #561, P2): after routing defaults through the config table, a non-orchestration `slot start` with no --model reached the codex-default fallback in startSingleThread/ensureThread, which threw when mag.orchestration.model_classes was absent — breaking plain t3code starts on minimal/legacy configs that previously opened with the built-in codex default. Reserve the loud missing-table throw for orchestration default resolution (resolveAgentModel / selectOrchestrationFlags). Add singleThreadCodexModel(): returns the table's codex value when present, else "" (provider/CLI picks its own default) — never throws, no hardcoded version string (AC2 preserved). Regression test in t3code.test.ts: absent table → "" (no throw), present → configured value, with a contrast assertion that orchestration classModel() still throws on the same table-less config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
note: branch state has drifted since this body was written (baseline: 4 commits at 2026-06-06T20:13:58Z, current: 5 commits). consider |
|
note: branch state has drifted since this body was written (baseline: 4 commits at 2026-06-06T20:14:11Z, current: 5 commits). consider |
…sses table Replace the pinned minor-version constants in src/adapters/t3code.ts (DEFAULT_MODEL=gpt-5.4, DEFAULT_CLAUDE_MODEL, CLAUDE_OPUS_MODEL=claude-opus-4-6) and the two "coder:codex:gpt-5.4"/"reviewer:codex:gpt-5.4" fallback tokens with a config-driven latest-within-class resolver. The per-class latest mapping is the single source of truth under mag.orchestration.model_classes in config.yaml; bumping a class is a config edit, not a code change. - New src/orchestration/model-defaults.ts: TRACKED_MODEL_CLASSES, resolveModelClass (throws loudly on missing/blank/non-string class — AC3), classForProvider. - t3code.ts: classModel/providerDefaultModel read orchCfg?.model_classes (the lint:config-reference Direction-4 adapter-read seam); resolveAgentModel's lowest tier resolves the class table (below all task-1fbd4edf overrides); selectOrchestrationFlags sources the claude-code suffix from the table; models are pre-resolved BEFORE createWorktrees so a missing class throws before any side effect. - templates/config.reference.yaml + templates/harness/config.yaml: documented model_classes table + how-to-bump comment (AC2/AC6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e + tests - tmux-adapter.ts: resolveAgentModel's lowest tier calls the shared providerDefaultModel (exported from t3code.ts); models pre-resolved before createWorktrees / tmux sessions so a missing class throws before side effects. Exported resolveAgentModel as a test seam. - tmux-adapter.test.ts: bare-codex resolves to the table value, reviewer_model config still wins, missing class throws loudly. - orchestration-defaults.test.ts: the two default_coder:null fakeConfigs now carry model_classes (claude-code resolution reads the table). - lint-config-reference.test.ts: world-sanity guard that model_classes is covered (not inert) against the live repo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
slotStart auto-fill drives selectOrchestrationFlagsForTask with the default claude-code coder, which now resolves through the config table; the test writeConfig() helper must carry mag.orchestration.model_classes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…clean The bottom-tier comment named the removed DEFAULT_MODEL/DEFAULT_CLAUDE_MODEL constants verbatim, so a constants-name grep still hit it. Reword to describe them generically; no behaviour change. Also (outside this repo) added mag.orchestration.model_classes to the live state-repo config ~/self-improve/harness/config.yaml so new no-explicit-model orchestration sessions resolve latest-within-class via the live loadConfigSync path (AC1/AC2 deploy step). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-thread starts Codex review (PR #561, P2): after routing defaults through the config table, a non-orchestration `slot start` with no --model reached the codex-default fallback in startSingleThread/ensureThread, which threw when mag.orchestration.model_classes was absent — breaking plain t3code starts on minimal/legacy configs that previously opened with the built-in codex default. Reserve the loud missing-table throw for orchestration default resolution (resolveAgentModel / selectOrchestrationFlags). Add singleThreadCodexModel(): returns the table's codex value when present, else "" (provider/CLI picks its own default) — never throws, no hardcoded version string (AC2 preserved). Regression test in t3code.test.ts: absent table → "" (no throw), present → configured value, with a contrast assertion that orchestration classModel() still throws on the same table-less config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c8537d3 to
47a4509
Compare
Suggest Refactor — task-c48b7beb (coder retrospective)What I'd do differently next time on this change (and adjacent cleanups worth a follow-up). Process / would-do-differently
Code-shape refactors worth a follow-up (not done here — out of scope)
What worked (keep doing)
|
Summary
Orchestration sessions that start with no explicit model previously inherited hardcoded minor-version pins in
src/adapters/t3code.ts(DEFAULT_MODEL = "gpt-5.4",CLAUDE_OPUS_MODEL = "claude-opus-4-6", fallback tokenscoder:codex:gpt-5.4/reviewer:codex:gpt-5.4). These went stale as new minors shipped. This implements approach B from the proposal: a single per-classmodel_classestable inconfig.yamlundermag.orchestrationis the source of truth, and the code resolves each unspecified default by reading it — so bumping a class is a config edit, not a code change.Implements
docs/proposals/resolve-default-models-latest-within-class.md(task-c48b7beb).What changed
src/orchestration/model-defaults.ts—TRACKED_MODEL_CLASSES,resolveModelClass(table, cls)(throws loudly on missing/blank/non-string — no silent fallback),classForProvider.src/adapters/t3code.ts— removed the three pinned constants + two fallback tokens;classModel/providerDefaultModelreadorchCfg?.model_classes(thelint:config-referenceadapter-read seam);resolveAgentModel's lowest tier resolves the table (below everytask-1fbd4edfoverride);selectOrchestrationFlagssources the claude-code suffix from the table; models are pre-resolved beforecreateWorktreesso a missing class throws before any side effect.src/adapters/tmux-adapter.ts— shares the resolver viaproviderDefaultModel; same pre-resolve ordering.templates/config.reference.yaml+templates/harness/config.yaml— documentedmodel_classestable (codex→gpt-5.5, claude-opus→claude-opus-4-8, claude-sonnet→claude-sonnet-4-6) + a "how to bump" comment.slot startpath usessingleThreadCodexModel(), which degrades to the provider default (never throws, no hardcoded version) so minimal/legacy configs without the table still open.Precedence (unchanged, no regression)
adapter-arg flag (
--coder-model/--reviewer-model) > explicit token model >coder_model/reviewer_modelconfig >model_classestable > provider default.Tests
src/orchestration/model-defaults.test.ts— per-class loud-failure enumeration (missing/blank/whitespace/non-string).src/adapters/t3code.test.ts— precedence ladder, runtime-path throw, table-sourced suffix, empty fallback tokens, single-thread non-throwing carve-out.src/adapters/tmux-adapter.test.ts— shared resolver parity + loud failure.scripts/lint-config-reference.test.ts—model_classescovered (not inert) against the live repo.src/.Verification
orch statusevidence for a no-explicit-model session resolving from the config table: coderclaude-code→claude-opus-4-8, reviewercodex→gpt-5.5(tiny →claude-sonnet-4-6).🤖 Generated with Claude Code