fix(cli): preserve current browser effort - #323
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 6:07 PM ET / 22:07 UTC. Summary Reproducibility: yes. Current-main source gives a high-confidence reproduction path: an explicit CLI Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Merge the source-aware precedence rule after required checks complete, retaining explicit CLI effort overrides and config-defined current-strategy/effort pairs exactly as covered by the focused tests. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source gives a high-confidence reproduction path: an explicit CLI Is this the best way to solve the issue? Yes. Checking the Commander source in the existing shared defaults helper is the narrowest maintainable fix, and the branch explicitly protects both explicit effort overrides and config-defined strategy/effort behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ec2e99bf57b4. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Oracle independent review completed against head
Oracle confirmed the source-sensitive guard preserves the intended matrix: explicit CLI Non-blocking suggestions only: optionally spell out |
Summary
browser.thinkingTimefrom config when the CLI explicitly requests--browser-model-strategy current--browser-thinking-timeand config-definedmodelStrategy: currentplusthinkingTimeReproduction
With a global browser default such as:
{ "browser": { "thinkingTime": "extended" } }this command intends to keep the current browser state:
Before this change, the config default was still injected, so a currently selected Pro model could fail before submission while Oracle tried to confirm a separate Extended effort chip.
The new precedence rule suppresses only that inherited value. Users can still change effort explicitly with
--browser-thinking-time, and config-defined strategy/effort pairs remain unchanged.Verification
pnpm vitest run tests/cli/browserDefaults.test.ts— 18 passedpnpm vitest run— 144 files passed, 18 skipped; 1,563 tests passed, 43 skippedpnpm checkpnpm buildthinkingTime: extended, explicit--browser-model-strategy current, and no thinking-time flagCURRENT MODEL NO INHERITED EFFORT OKBLOCKERS: NoneTEST_GAPS: None materialVERDICT: APPROVEThis is intentionally separate from #320: it changes CLI config precedence for the
currentstrategy rather than GPT-5.6 Sol/Pro picker compatibility.