Respect explicit Codex model provider config#191
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPrevents the unauthenticated OpenCode Zen fallback from overriding explicit ChangesFree-mode fallback respects explicit provider configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Review Summary by QodoRespect explicit Codex model provider config and skip Zen fallback
WalkthroughsDescription• Respect explicit top-level model_provider in config.toml during startup • Skip OpenCode Zen fallback when custom provider is configured • Add TOML comment stripping and top-level config detection logic • Expand test coverage for explicit and nested provider configurations • Update manual test documentation with custom-provider verification steps Diagramflowchart LR
startup["Startup: ensureDefaultFreeModeStateForMissingAuthSync"]
checkAuth{"Has usable Codex auth?"}
checkDefault{"Should create default state?"}
checkConfig{"Has explicit model_provider in config.toml?"}
returnNull["Return null"]
returnCurrent["Return current state"]
returnZen["Return OpenCode Zen state"]
startup --> checkAuth
checkAuth -->|Yes| returnNull
checkAuth -->|No| checkDefault
checkDefault -->|No| returnCurrent
checkDefault -->|Yes| checkConfig
checkConfig -->|Yes| returnCurrent
checkConfig -->|No| returnZen
File Changes1. src/server/codexAppServerBridge.archive.test.ts
|
Code Review by Qodo
Context used 1. Caches config scan failures
|
|
All good. Free mode doesn't get enabled with this fix. Thanks! |
4f6fa76 to
f108186
Compare
|
/review |
|
Persistent review updated to latest commit f108186 |
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/server/codexAppServerBridge.ts`:
- Around line 3720-3805: The TOML scanner in
hasExplicitCodexModelProviderConfigSync (and helper stripTomlComment) fails to
detect quoted keys and mis-parses multiline strings because state is reset per
line; update stripTomlComment or replace it with a small TOML-aware line scanner
that preserves string state across lines (track inSingleQuote, inDoubleQuote,
inMultilineBasic, inMultilineLiteral, and escaped across lines) so comments
inside multiline strings are ignored, and update the key-detection in
hasExplicitCodexModelProviderConfigSync to accept quoted keys (e.g., handle
"model_provider" and 'model_provider' in addition to unquoted) when matching the
left-hand side before '='; ensure explicitCodexModelProviderConfigCache usage
remains the same and add regression tests for a quoted key `"model_provider" =
"azure"` (should return true) and for a multiline string containing
`model_provider = "azure"` (should return false).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e50847e4-507c-4514-82b2-3d94b3d108f6
📒 Files selected for processing (3)
src/server/codexAppServerBridge.archive.test.tssrc/server/codexAppServerBridge.tstests/providers-models/android-published-cli-loads-codex-app-server-models-through-local-proxy.md
✅ Files skipped from review due to trivial changes (2)
- src/server/codexAppServerBridge.archive.test.ts
- tests/providers-models/android-published-cli-loads-codex-app-server-models-through-local-proxy.md
f108186 to
1917880
Compare
|
/review |
|
Persistent review updated to latest commit 1917880 |
Summary
Fixes #190
Verification
Performance
Summary by CodeRabbit
Bug Fixes
Tests
Documentation