Skip to content

Update default Gemini model and add model-not-found fallback#517

Merged
wesm merged 5 commits intomainfrom
fix/gemini-3.1-pro
Mar 16, 2026
Merged

Update default Gemini model and add model-not-found fallback#517
wesm merged 5 commits intomainfrom
fix/gemini-3.1-pro

Conversation

@wesm
Copy link
Copy Markdown
Collaborator

@wesm wesm commented Mar 16, 2026

Summary

  • Update default Gemini model from gemini-3.1-pro-preview to gemini-3.1-pro now that it's promoted out of preview
  • When the Gemini CLI fails with a model-not-found error, automatically retry without the -m flag to let the CLI use its own default model, making roborev resilient to Google renaming models
  • Extract runGemini helper to support the retry without duplicating pipe/parse logic

🤖 Generated with Claude Code

- Update default model from gemini-3.1-pro-preview to gemini-3.1-pro
- When Gemini CLI fails with a model-not-found error, retry without the
  -m flag to let the CLI use its own default model
- Extract runGemini helper to avoid duplicating pipe/parse logic
- Add tests for model fallback and isModelNotFoundError

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Mar 16, 2026

roborev: Combined Review (6a7f484)

Summary Verdict: Updates default Gemini model to gemini-3.1-pro and implements a retry fallback
mechanism for missing/retired models.

High Severity

  • Location: internal/agent/gemini.go (lines 157-159)
  • Issue: Data race on bytes.Buffer. The code calls stderrStr := stderr.String() before cmd .Wait() completes. Because exec.Cmd populates Stderr via a concurrent goroutine, this can observe an incomplete buffer and race with ongoing writes. Consequently, the isModelNotFoundError(stderrStr) check may miss the actual model-not-found message and skip the retry, or returned errors may lose the
    trailing stderr that explains the failure.
  • Suggested Fix: Move stderrStr := stderr.String() to immediately after waitErr := cmd.Wait(), which acts as the synchronization point guaranteeing all writes have finished. Add a regression test where stdout closes first and stderr is emitted only at process exit.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Move stderr.String() after cmd.Wait(), which is the synchronization
point guaranteeing all stderr writes have completed. Previously it was
called between parseStreamJSON and Wait, racing with the goroutine
that populates the stderr buffer.

Add regression test with late stderr (emitted after stdout closes).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Mar 16, 2026

roborev: Combined Review (7e94c58)

Verdict: The changes successfully implement the model update and fix a data race,
but introduce a few medium-severity bugs related to output streaming and user-configured model fallbacks.

Medium

  • internal/agent/gemini.go:102, internal/agent/gemini.go:149, [internal/agent/gemini.go:166](/home/roborev/repos/rob
    orev/internal/agent/gemini.go#L166):
    The first failed attempt streams stderr directly into the caller’s output writer before the fallback decision is made. If the retry succeeds, users still see/store the initial "model not found" error in live output and job logs
    even though the review completed successfully. Suggested fix: Buffer the first attempt’s streamed output separately until fallback is ruled out, or suppress forwarding stderr/stdout from the probe run and only emit output from the successful attempt.
  • [internal/agent/gemini.go:103](/home
    /roborev/repos/roborev/internal/agent/gemini.go#L103):
    The fallback triggers for any non-empty a.Model, not just the built-in default. That means an explicitly configured model typo or retired user-selected model is silently replaced with
    Gemini CLI’s default model, which can change review behavior/cost unexpectedly instead of surfacing the configuration error. Suggested fix: Only auto-fallback when the missing model matches the agent’s implicit default, or track whether the model was user-specified and fail fast for explicit overrides.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

- Revert model name back to gemini-3.1-pro-preview (still in preview)
- Track whether model was set via WithModel (user config) vs built-in
  default; only auto-fallback for the built-in default so user config
  errors fail fast instead of silently switching models
- Add test for explicit model failing fast on model-not-found

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Mar 16, 2026

roborev: Combined Review (4ba953a)

Verdict: Clean. The commits successfully refactor Gemini execution and fix a data race, with no medium or higher severity issues found
across all reviews.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Address review findings:
- Check a.Model == defaultGeminiModel instead of tracking modelExplicit
  bool, so direct field assignment of non-default models also fails fast
- Remove modelExplicit field (no longer needed)
- Add invocation-counting test that proves explicit models are invoked
  exactly once (no silent retry)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Mar 16, 2026

roborev: Combined Review (45dca86)

Verdict: The PR introduces a useful fallback and fixes a stderr race, but has medium-severity issues regarding
explicit model configuration and security risks during agentic execution retry.

Medium

1. Explicit Model Configuration Ignored in Fallback

  • Location: [internal/agent/gemini.go:107](/home/roborev/repos/roborev/internal/agent/gemini.go
    #L107), cmd/roborev/main.go:898
  • Description: The fallback is now gated only on a.Model == defaultGeminiModel, which does not distinguish the built-in default from a user-configured explicit model with the same literal value. A user who pins gemini-3.1-pro-preview in config/CLI will now silently fall back to whatever Gemini CLI chooses, even though explicit models should
    fail fast.
  • Suggested Fix: Restore explicit origin tracking (modelExplicit or equivalent) and add a regression test for WithModel(defaultGeminiModel).

2. Stderr-Controlled Fallback Security Risk in Agentic Mode

  • Location: internal/agent/gem
    ini.go
    (reviewed diff lines 106-114, 203-208)
  • Description: The retry path is triggered by a broad substring match on stderr (isModelNotFoundError(stderrStr)) coming from the Gemini subprocess. In agentic mode, the fallback preserves --yolo plus tool access, so a false positive causes the same prompt to be executed a second time with privileged local tool access. This creates a concrete security risk of duplicated local side effects and silent
    resubmission of repository contents to the CLI-selected default model rather than the pinned built-in model.
  • Suggested Fix: Gate fallback on a structured error signal from the Gemini CLI/API (e.g., a specific exit code or machine-readable error payload proving the failure happened before any tool session started
    ), not on free-form stderr text. At minimum, disable automatic fallback when agentic mode is enabled, or require explicit opt-in before retrying without -m.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Gemini CLI deprecated --allowed-tools in favor of the Policy Engine.
Use --approval-mode plan (read-only) for reviews and --approval-mode
yolo for agentic mode, dropping --allowed-tools and --yolo flags.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Mar 16, 2026

roborev: Combined Review (60dcde7)

Verdict: Ref
actors Gemini execution, fixes a stderr-buffer race, implements a retry fallback for the default Gemini model, and updates CLI flags to use --approval-mode.

Medium

  • [internal/agent/gemini.go](/home/roborev/repos/roborev/internal/agent/
    gemini.go)
    : The retry gate in Review is a.Model == defaultGeminiModel, which does not distinguish the built-in default from an explicit user/config override of the same string. If someone deliberately pins gemini-3.1-pro-preview, a 4
    04 will now silently retry without -m and switch to whatever Gemini CLI currently treats as its default. That contradicts the comment about surfacing config errors and can change review behavior unexpectedly. Suggested fix: track model provenance separately (for example, a modelExplicit bit) and only auto-fallback for the built-
    in default. Add a test for “explicit model equal to default string does not fallback”.

  • internal/agent/gemini.go: buildArgsWithModel now replaces explicit --allowed -tools scoping with only --approval-mode plan|yolo. The repo contract says non-agentic mode is read-only, but this code no longer enforces that at the Gemini boundary. Unless Gemini guarantees plan preserves read-only tooling semantics, review jobs can regress into approval-blocked runs or
    gain mutating tools unexpectedly. Suggested fix: preserve explicit tool scoping if the new CLI still supports it, or add an integration/smoke test that proves plan stays read-only and yolo retains write/shell access. The updated coverage in [internal/agent/gemini_test.go](/home
    /roborev/repos/roborev/internal/agent/gemini_test.go) only checks flag tokens, not the read-only vs agentic behavior.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Copy Markdown
Collaborator Author

wesm commented Mar 16, 2026

Good enough here. Merging

@wesm wesm merged commit 6ae4067 into main Mar 16, 2026
8 checks passed
@wesm wesm deleted the fix/gemini-3.1-pro branch March 16, 2026 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant