Skip to content

fix(cli): preserve current browser effort - #323

Merged
steipete merged 3 commits into
steipete:mainfrom
jung0han:codex/current-model-ignore-inherited-effort
Jul 13, 2026
Merged

fix(cli): preserve current browser effort#323
steipete merged 3 commits into
steipete:mainfrom
jung0han:codex/current-model-ignore-inherited-effort

Conversation

@jung0han

Copy link
Copy Markdown
Contributor

Summary

  • do not inherit browser.thinkingTime from config when the CLI explicitly requests --browser-model-strategy current
  • preserve an explicit --browser-thinking-time and config-defined modelStrategy: current plus thinkingTime
  • document the precedence rule and cover it with direct and real Commander source tests

Reproduction

With a global browser default such as:

{
  "browser": {
    "thinkingTime": "extended"
  }
}

this command intends to keep the current browser state:

oracle --engine browser --browser-model-strategy current -p "..." --file "..."

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 passed
  • pnpm vitest run — 144 files passed, 18 skipped; 1,563 tests passed, 43 skipped
  • pnpm check
  • pnpm build
  • signed-in Chrome smoke with config thinkingTime: extended, explicit --browser-model-strategy current, and no thinking-time flag
    • no thinking-time selection step ran
    • request completed with exact marker CURRENT MODEL NO INHERITED EFFORT OK
  • Oracle external review after requested revisions
    • BLOCKERS: None
    • TEST_GAPS: None material
    • VERDICT: APPROVE

This is intentionally separate from #320: it changes CLI config precedence for the current strategy rather than GPT-5.6 Sol/Pro picker compatibility.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 6:07 PM ET / 22:07 UTC.

Summary
The PR prevents an explicit CLI --browser-model-strategy current from inheriting browser.thinkingTime, while preserving explicit thinking-time flags and config-defined strategy/effort pairs, with documentation and regression tests.

Reproducibility: yes. Current-main source gives a high-confidence reproduction path: an explicit CLI current strategy leaves effort unset, then config defaulting injects browser.thinkingTime and downstream browser execution attempts that unintended effort selection.

Review metrics: 2 noteworthy metrics.

  • Runtime scope: 1 file, 8 changed lines. The behavioral change is isolated to the shared browser config-default helper rather than picker or execution logic.
  • Focused coverage: 6 regression cases added. The tests cover the fixed path and neighboring precedence cases that must remain compatible.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Next step before merge

  • No automated repair is needed; ordinary maintainer review and completion of the remaining required check are the only next steps.

Security
Cleared: The diff changes local option precedence, tests, and documentation without adding dependencies, permissions, secret access, downloads, lifecycle hooks, or other supply-chain surface.

Review details

Best 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 current strategy leaves effort unset, then config defaulting injects browser.thinkingTime and downstream browser execution attempts that unintended effort selection.

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 changes

Label changes:

  • add P2: The PR fixes a bounded browser CLI workflow that can fail before prompt submission, with limited blast radius.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body records an after-fix signed-in Chrome run that directly observed the changed behavior—no effort-selection step—and the successful response marker.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body records an after-fix signed-in Chrome run that directly observed the changed behavior—no effort-selection step—and the successful response marker.

Label justifications:

  • P2: The PR fixes a bounded browser CLI workflow that can fail before prompt submission, with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body records an after-fix signed-in Chrome run that directly observed the changed behavior—no effort-selection step—and the successful response marker.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body records an after-fix signed-in Chrome run that directly observed the changed behavior—no effort-selection step—and the successful response marker.
Evidence reviewed

What I checked:

  • Current-main bug path: The current defaults helper applies configured browser.thinkingTime whenever browserThinkingTime is unset, without considering that browserModelStrategy may be explicitly CLI-sourced as current. (src/cli/browserDefaults.ts:148, ec2e99bf57b4)
  • Narrow implementation: The branch adds a CLI-source check for current before inheriting thinking time and leaves the other browser defaults unchanged. (src/cli/browserDefaults.ts:60, e827942f78da)
  • Regression coverage: The tests cover explicit CLI current without effort, explicit effort preservation, config-defined current plus effort, select/ignore inheritance, and actual Commander option-source behavior. (tests/cli/browserDefaults.test.ts:127, e827942f78da)
  • Real behavior proof: The PR body reports a signed-in Chrome run with configured extended effort and explicit current strategy; no effort-selection step ran and the request completed with the exact marker CURRENT MODEL NO INHERITED EFFORT OK. (e827942f78da)
  • Repository-specific policy: Oracle's policy calls for careful browser smoke validation and changelog coverage for user-visible changes; the PR includes both a browser smoke report and an unreleased changelog entry. (AGENTS.md:9, ec2e99bf57b4)
  • Current checks: GitGuardian, macOS, and Ubuntu checks passed; Windows remained pending at inspection time and is routine merge gating rather than a patch defect. (e827942f78da)

Likely related people:

  • Peter Steinberger: Current-main blame and file history tie the shared CLI browser-default helper to the v0.16.0 release commit, making him the strongest available routing candidate for its precedence contract. (role: recent area contributor; confidence: medium; commits: 54acb79dc716; files: src/cli/browserDefaults.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@jung0han

Copy link
Copy Markdown
Contributor Author

Oracle independent review completed against head e827942f.

  • BLOCKERS: None
  • TEST_GAPS: No material test gaps
  • VERDICT: APPROVE

Oracle confirmed the source-sensitive guard preserves the intended matrix: explicit CLI current suppresses only inherited config thinking time; explicit CLI thinking time is retained; config-defined current + thinkingTime remains intact; and explicit select/ignore still inherit the config value.

Non-blocking suggestions only: optionally spell out select/ignore inheritance in docs and add a future root-command config integration test. The review considered the current focused/full tests, cross-platform CI, build/check, and signed-in browser smoke sufficient for merge readiness.

@steipete
steipete merged commit 8e3456e into steipete:main Jul 13, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants