fix(subagent): move per-role model fallback into opencodex config (#1190) - #1210
fix(subagent): move per-role model fallback into opencodex config (#1190)#1210Yuxin-Qiao wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds ChangesSub-agent fallback compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Subagent
participant OpenCodex
participant Config
participant CodexRole
participant Doctor
Subagent->>OpenCodex: request with primary model
OpenCodex->>Config: resolve subagentModelFallbackByModel
Config-->>OpenCodex: model-specific fallback chain
OpenCodex->>CodexRole: read legacy model_fallback
CodexRole-->>OpenCodex: compatibility entries
Doctor->>CodexRole: scan agent TOML files
CodexRole-->>Doctor: roles containing model_fallback
OpenCodex-->>Subagent: apply ordered fallback candidates
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. Hygiene✅ Deterministic PR hygiene checks passed. |
|
✅ Deterministic PR hygiene checks passed. |
9320131 to
63f31ae
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs-site/src/content/docs/ru/reference/configuration/agents.md`:
- Line 21: Update the Russian table entry for subagentModelFallbackByModel to
identify this OpenCodex configuration field as the supported location for
per-role fallback metadata. Describe model_fallback in $CODEX_HOME/agents/*.toml
as legacy-only and state that Codex 0.146+ skips the role when it is used,
keeping the wording consistent with the English documentation and referenced
guidance.
In `@src/cli/doctor.ts`:
- Around line 926-932: Update the scanner used by the doctor flow around
scanCodexAgentRolesWithTomlModelFallback so it reports every readable
model_fallback TOML key, including model_fallback = [], while excluding roles
without the key; do not base detection solely on
readCodexAgentModelFallback(...).length. In
tests/subagent-model-fallback.test.ts lines 990-1004, add an empty-array
model_fallback role and assert it is reported, while retaining coverage that
roles without the key are excluded.
In `@src/codex/subagent-model-fallback.ts`:
- Around line 567-570: Update scanCodexAgentRolesWithTomlModelFallback to detect
the presence of the model_fallback key rather than requiring
readCodexAgentModelFallback to return a non-empty list; add or use
hasCodexAgentModelFallbackField with the same TOML parser while preserving
readCodexAgentModelFallback for fallback execution, and add a focused Bun
regression test under tests/ covering model_fallback = [].
🪄 Autofix
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: 4fb8111f-cb52-4927-bac7-5d9056e41ae8
📒 Files selected for processing (15)
docs-site/src/content/docs/guides/sub-agent-surface.mddocs-site/src/content/docs/ja/guides/sub-agent-surface.mddocs-site/src/content/docs/ja/reference/configuration/agents.mddocs-site/src/content/docs/ko/guides/sub-agent-surface.mddocs-site/src/content/docs/ko/reference/configuration/agents.mddocs-site/src/content/docs/reference/configuration/agents.mddocs-site/src/content/docs/ru/guides/sub-agent-surface.mddocs-site/src/content/docs/ru/reference/configuration/agents.mddocs-site/src/content/docs/zh-cn/guides/sub-agent-surface.mddocs-site/src/content/docs/zh-cn/reference/configuration/agents.mdsrc/cli/doctor.tssrc/codex/subagent-model-fallback.tssrc/config.tssrc/types.tstests/subagent-model-fallback.test.ts
210e372 to
c476e83
Compare
|
Addressed all three CodeRabbit findings:
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
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/codex/subagent-model-fallback.ts`:
- Around line 567-572: Update hasCodexAgentModelFallbackField and the
fallback-reading path to reuse one TOML-aware, presence-aware parser instead of
matching raw text. Ensure it recognizes quoted model_fallback keys, ignores
occurrences inside multiline strings, and preserves distinction between an
absent field and a present field with an empty value; add coverage for both
cases in the existing subagent model fallback tests.
🪄 Autofix
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: a404ca17-a237-47ea-b4f2-bcc687bfa453
📒 Files selected for processing (3)
docs-site/src/content/docs/ru/reference/configuration/agents.mdsrc/codex/subagent-model-fallback.tstests/subagent-model-fallback.test.ts
|
@coderabbitai please re-review Addressed the TOML-awareness finding in
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR diff summary ---'
git diff --stat origin/dev...HEAD || git diff --stat HEAD~1...HEAD
echo
echo '--- Changed files ---'
git diff --name-only origin/dev...HEAD || git diff --name-only HEAD~1...HEAD
echo
echo '--- Relevant symbol map ---'
fd -a -t f '^(subagent-model-fallback|doctor)\.ts$' src tests | while IFS= read -r f; do
echo "### $f"
ast-grep outline "$f" --items all --type function --match 'parseTomlModelFallbackField|hasCodexAgentModelFallbackField|scanCodexAgentRolesWithTomlModelFallback|read.*ModelFallback|applySubagentModelFallback' || true
done
echo
echo '--- Implementation references with context ---'
rg -n -C 8 'parseTomlModelFallbackField|hasCodexAgentModelFallbackField|scanCodexAgentRolesWithTomlModelFallback|model_fallback' src/codex/subagent-model-fallback.ts tests/subagent-model-fallback.test.ts
echo
echo '--- Incremental commit diff ---'
git show --format=fuller --find-renames c29ade17 -- src/codex/subagent-model-fallback.ts tests/subagent-model-fallback.test.tsLength of output: 38871 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Parser with line numbers ---'
nl -ba src/codex/subagent-model-fallback.ts | sed -n '520,725p'
echo
echo '--- TOML dependencies and parser usage ---'
rg -n -i -C 3 '"[^"]*toml[^"]*"|from ["'\''][^"'\'']*toml|require\(["'\''][^"'\'']*toml' \
package.json bun.lockb bun.lock tests src 2>/dev/null || true
echo
echo '--- Escaped multiline-delimiter coverage ---'
rg -n -i -C 5 'escaped.*multiline|multiline.*escaped|\\{1,}""".*model_fallback|model_fallback.*\\{1,}"""' \
tests src || true
echo
echo '--- All parser call sites ---'
rg -n -C 3 '\bparseTomlModelFallbackField\b|\bscanTomlLine\b|\bfindTomlMultilineStringEnd\b' src testsLength of output: 1056 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Parser with line numbers ---'
sed -n '520,725p' src/codex/subagent-model-fallback.ts | awk '{ printf "%6d %s\n", NR + 519, $0 }'
echo
echo '--- TOML dependencies and parser usage ---'
for f in package.json bun.lockb bun.lock; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -i -C 3 'toml' "$f" || true
fi
done
rg -n -i -C 3 'from ["'\''][^"'\'']*toml|require\(["'\''][^"'\'']*toml|parse.*toml|toml.*parse' src tests || true
echo
echo '--- Escaped multiline-delimiter coverage ---'
rg -n -i -C 5 'escaped.*multiline|multiline.*escaped|\\{1,}""".*model_fallback|model_fallback.*\\{1,}"""' tests src || true
echo
echo '--- All parser call sites ---'
rg -n -C 3 '\bparseTomlModelFallbackField\b|\bscanTomlLine\b|\bfindTomlMultilineStringEnd\b' src testsLength of output: 49739
Medium — preserve multiline basic-string state across escaped delimiters In
As a result, this valid pattern can cause a false positive: description = """
An escaped delimiter: \"""
model_fallback = []
"""The scanner exits string mode at Use the same delimiter-aware helper in both scanner paths:
The shared parser and the existing quoted-key, empty-array, and basic string tests otherwise address the prior finding. ✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…dge-jun#1190) Codex 0.146+ strictly deserializes $CODEX_HOME/agents/*.toml and rejects model_fallback as an unknown field, skipping the entire role definition. Per-role fallback chains now live in config.json as subagentModelFallbackByModel, keyed by the requested primary model id, and are consulted before the legacy TOML read (kept for backwards compatibility). ocx doctor scans agent role files and warns when any still carries model_fallback, pointing at the new config home. Docs updated in all locales.
CodeRabbit review: Codex >= 0.146 rejects the unknown field regardless of value, so the doctor scan must report model_fallback = [] roles too. Add hasCodexAgentModelFallbackField and a regression test; clarify the Russian config table wording.
CodeRabbit review: hasCodexAgentModelFallbackField matched raw text, so quoted keys like "model_fallback" were missed and the same text inside a multiline string literal was a false positive. Share one TOML-aware, presence-aware parser between the doctor scan and the fallback-reading path; it recognizes quoted keys, skips string contents, and keeps absent vs. empty-array distinct. Add quoted-key and multiline-string tests.
c29ade1 to
2b6a50f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/codex/subagent-model-fallback.ts`:
- Around line 637-671: Update parseTomlStringArrayValue so commas are tracked
separately from whitespace and required between successive string elements.
Reject arrays with adjacent strings or a leading comma, while preserving valid
empty and comma-separated arrays; add tests covering missing-comma and
leading-comma inputs.
- Around line 568-574: Replace the raw triple-quote index lookup in both scanner
paths with findTomlMultilineStringEnd, including the path containing the shown
state.inMultilineString handling, so escaped delimiters do not terminate
scanning early. Add a regression test covering an escaped triple quote followed
by model_fallback = [] within the same multiline string, ensuring the embedded
text is not reported as configuration.
🪄 Autofix
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: bc6a5156-c411-4787-a7e4-888b7b274c97
📒 Files selected for processing (3)
src/codex/subagent-model-fallback.tssrc/config.tssrc/types.ts
CodeRabbit review: an escaped triple quote (\""") inside a multiline basic string closed scanner state early, and string arrays accepted adjacent elements without commas. Use findTomlMultilineStringEnd in both scanner paths and require one comma between successive array elements while keeping empty and trailing-comma arrays valid. Add regression tests for escaped delimiters and malformed arrays.
|
@coderabbitai review Addressed the two new findings in
Verification: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Commit summary and changed files ---'
git show --stat --oneline --decorate --find-renames 7325fbac
git show --format='' --name-only 7325fbac
echo
echo '--- Relevant symbol map ---'
ast-grep outline src/codex/subagent-model-fallback.ts \
--items all --type function \
--match 'findTomlMultilineStringEnd|scanTomlLine|parseTomlModelFallbackField|parseTomlStringArrayValue|hasCodexAgentModelFallbackField|readAgentModelFallback'
ast-grep outline tests/subagent-model-fallback.test.ts \
--items all --type function \
--match 'modelFallback|fallback|Toml|toml'
echo
echo '--- Incremental implementation and test diff ---'
git show --format=fuller --find-renames 7325fbac -- \
src/codex/subagent-model-fallback.ts \
tests/subagent-model-fallback.test.ts
echo
echo '--- Current parser and relevant tests with line numbers ---'
rg -n -C 14 \
'function (findTomlMultilineStringEnd|scanTomlLine|parseTomlModelFallbackField|parseTomlStringArrayValue)|findTomlMultilineStringEnd\(|escaped delimiter|missing comma|leading comma|doubled comma|model_fallback = \[\]' \
src/codex/subagent-model-fallback.ts \
tests/subagent-model-fallback.test.tsLength of output: 27697
The reported test results were not independently executed in this inspection environment. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
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/codex/subagent-model-fallback.ts`:
- Line 663: Update the array parsing logic in readCodexAgentModelFallback() so
encountering ] validates the rest of the current physical line, permitting only
horizontal whitespace, an inline # comment, or end-of-line; reject any other
trailing token without consuming subsequent lines passed by
parseTomlModelFallbackField(). Add a focused Bun regression test under tests/
verifying scanCodexAgentRolesWithTomlModelFallback() reports the malformed field
and returns no fallback entries.
🪄 Autofix
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: b1676da4-b145-4e4a-93aa-65e94dfadd37
📒 Files selected for processing (2)
src/codex/subagent-model-fallback.tstests/subagent-model-fallback.test.ts
CodeRabbit review: the value parser returned as soon as it read the closing bracket, so 'model_fallback = ["kimi/k3"] invalid' produced a fallback list. After ']' only horizontal whitespace, an inline comment, or the line end is valid; anything else makes the value malformed while presence (and thus the doctor WARN) is still reported.
|
@coderabbitai review Addressed the trailing-token finding in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Incremental commit ---'
git show --stat --oneline --find-renames 6b1ce0cd
git show --format=fuller --find-renames 6b1ce0cd -- \
src/codex/subagent-model-fallback.ts \
tests/subagent-model-fallback.test.ts
echo
echo '--- Parser and call-site map ---'
ast-grep outline src/codex/subagent-model-fallback.ts --items all --type function \
--match 'parseTomlStringArrayValue|parseTomlModelFallbackField|scanCodexAgentRolesWithTomlModelFallback|readAgentModelFallback|hasCodexAgentModelFallbackField'
ast-grep outline tests/subagent-model-fallback.test.ts --items all --type function \
--match 'fallback|Toml|toml'
echo
echo '--- Focused implementation and regression context ---'
rg -n -C 20 \
'function parseTomlStringArrayValue|parseTomlStringArrayValue\(|trailing token|inline comment|model_fallback = \["kimi/k3"\]' \
src/codex/subagent-model-fallback.ts \
tests/subagent-model-fallback.test.tsLength of output: 12712
I did not independently run the reported typecheck or test command in this environment. You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/codex/subagent-model-fallback.ts (1)
513-523: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPut global configuration before legacy TOML fallbacks.
The new selection path supplies legacy role fallbacks through the chain that
normalizedChain()inserts beforeconfig.subagentModelFallbackat Lines 127-129. A legacymodel_fallbackcan therefore override the global OpenCodex fallback configuration.Build the chain in this order: requested primary model, model-keyed configuration, global
subagentModelFallback, then legacy TOML values. Apply the existingfallbackChainKeyde-duplication across all four stages. Add a regression case with distinct model-keyed, global, and legacy entries.🤖 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/codex/subagent-model-fallback.ts` around lines 513 - 523, Update the fallback-chain construction around resolveConfiguredModelFallbackForPrimary and resolveAgentModelFallbackForPrimary so it orders the requested primary model, model-keyed configuration, global config.subagentModelFallback, then legacy TOML values. Apply the existing fallbackChainKey de-duplication across all stages, and add a regression case using distinct model-keyed, global, and legacy entries to verify that order.Source: Path instructions
🤖 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/codex/subagent-model-fallback.ts`:
- Around line 513-523: Update the fallback-chain construction around
resolveConfiguredModelFallbackForPrimary and resolveAgentModelFallbackForPrimary
so it orders the requested primary model, model-keyed configuration, global
config.subagentModelFallback, then legacy TOML values. Apply the existing
fallbackChainKey de-duplication across all stages, and add a regression case
using distinct model-keyed, global, and legacy entries to verify that order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87d049d3-7e27-4645-81ce-0921cc5c3df1
📒 Files selected for processing (2)
src/codex/subagent-model-fallback.tstests/subagent-model-fallback.test.ts
|
CI status for Run 31242590755: Local checks on the same head: Could a maintainer re-run the failed jobs (GitHub Actions "Re-run failed jobs" or |
|
Additional local evidence for the same head ( |
|
Landed on The compatibility fix was right: The correction reorders to primary → keyed → global → legacy, deduplicating every stage through Full suite green on the landed head. Thanks — moving this off the unsupported TOML field was the right call, and the doctor warnings make the migration visible to users. |
2 participants
Summary
$CODEX_HOME/agents/*.tomland rejectsmodel_fallbackas an unknown field, which skips the entire custom-agent role definition (the role never enters themulti_agent_v1registry).subagentModelFallbackByModel(Record<primaryModelId, string[]>), consulted after the requested primary model and before the globalsubagentModelFallbacklist — the same slot the TOML field used to occupy.model_fallbackread is kept for backwards compatibility (config-keyed chains take precedence on duplicates), butocx doctornow scans$CODEX_HOME/agents/*.tomland warns for every role file still carrying the field, pointing at the new config home. The scan shares one TOML-aware, presence-aware parser with the reader: quoted keys are detected, text inside (multi)line strings is not, escaped delimiters stay inside strings, and malformed array values still trigger the doctor WARN while yielding no fallback entries.Closes #1190
Verification
origin/dev(0 commits behind).bun run typecheck— clean.bun test tests/subagent-model-fallback.test.ts— 50 pass / 0 fail. New coverage: config-keyed resolution, cross-key dedupe with account selectors, config-over-TOML precedence, doctor scan presence including empty arrays, quotedmodel_fallbackkeys, single-line and multiline strings containingmodel_fallbacktext, escaped triple quotes inside multiline strings, and malformed arrays (missing/leading commas, trailing tokens) that stay reported by the scan while producing no fallback entries.bun test tests/doctor.test.ts tests/doctor-provider-apikey.test.ts tests/codex-plugins-doctor.test.ts tests/doctor-oauth.test.ts tests/config.test.ts tests/config-user-edits.test.ts— 222 pass / 0 fail.bun run privacy:scan— passed.bun run test(9079 tests): 12 failures intests/management-provider-validation.test.ts,crash-guard, and a process-serialization test; re-ran the same files on a cleanorigin/devworktree and got the identical failures — pre-existing ondev, unrelated to this change.6b1ce0cdcompleted with no new findings).Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I fixed all correct Codex and CodeRabbit findings.
My PR is ready for review.