Skip to content

fix: preserve requested model provenance on rerun#581

Merged
wesm merged 5 commits intomainfrom
evaluate-issue-556
Mar 30, 2026
Merged

fix: preserve requested model provenance on rerun#581
wesm merged 5 commits intomainfrom
evaluate-issue-556

Conversation

@mariusvniekerk
Copy link
Copy Markdown
Collaborator

Summary

  • add requested_model / requested_provider alongside effective model / provider
  • recompute effective model/provider on rerun from requested intent plus current config/defaults
  • expose requested model/provider as optional hidden queue columns in the TUI
  • extend SQLite, PostgreSQL, and sync paths for the new provenance fields

Why

Issue #556 came from reruns replaying stale effective OpenCode model values. The old schema only stored the effective model/provider, so rerun could not tell whether a value was explicitly requested or merely resolved from config/defaults.

Ambiguous migration choice

Existing rows are ambiguous: the historical model / provider values do not tell us whether they were explicitly requested or implicitly resolved.

I considered storing a per-row creation/model-schema version marker so rerun could special-case older rows, but that adds more schema and branching complexity than the problem warrants.

This change instead chooses the simpler behavior:

  • migrate existing rows with requested_model = NULL and requested_provider = NULL
  • do not backfill requested fields from historical effective values
  • treat NULL requested fields as "no explicit request recorded; reevaluate"

That means pre-migration rows will rerun against current defaults/config rather than freezing an old ambiguous effective value.

Validation

  • go test ./internal/storage ./internal/daemon ./cmd/roborev/tui
  • go vet ./...

Exact scenario checked

I also re-ran the exact configuration discussed in #556:

  • default_agent = "opencode"
  • default_model = "opencode/nemotron-3-super-free"

Before this change, that config stored the resolved effective model in review_jobs.model, which rerun then replayed as if it were original intent. After this change, explicit requested intent is tracked separately, and config-derived runs leave requested fields empty so rerun can reevaluate.

Preserve explicit requested model/provider separately from effective execution settings so reruns can reevaluate current defaults when no request was pinned.

Also exposes requested model/provider as optional hidden TUI queue columns and updates SQLite/PostgreSQL sync schema accordingly.

Validation:
- go test ./internal/storage ./internal/daemon ./cmd/roborev/tui
- go vet ./...

🤖 Generated with [OpenAI Codex](https://openai.com/codex)
Co-authored-by: OpenAI Codex <noreply@openai.com>
Fix the PostgreSQL UpsertJob INSERT to match the target column count.

This addresses the GitHub Actions integration failure in TestIntegration_UpsertJob_BackfillsModel.

Validation:
- go test ./internal/storage ./internal/daemon ./cmd/roborev/tui
- go vet ./...

🤖 Generated with [OpenAI Codex](https://openai.com/codex)
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 27, 2026

roborev: Combined Review (686ddbe)

Verdict: Changes are close, but there is 1 high-severity migration regression and 2 medium-severity correctness issues to address before merge.

High

  • internal/storage/db.go:371
    The SQLite rebuild migration no longer marks the existing agentic column as present. With hasAgentic = true missing from case "agentic":, rebuild migrations will omit that column from baseCols, which can permanently drop/reset existing agentic state for users upgrading in place.

Medium

  • internal/daemon/server.go:658
    resolveRerunModelProvider resolves config against job.RepoPath / an empty checkout root instead of the original checkout/worktree path used when the job was enqueued. Jobs created from linked worktrees can therefore rerun with the wrong config, producing the wrong effective model/provider. Resolution should use job.WorktreePath when set, otherwise job.RepoPath.

  • internal/storage/postgres.go in UpsertJob and internal/storage/sync.go in UpsertPulledJob
    The sync upsert path still uses COALESCE(EXCLUDED..., existing...) together with nullString("") for model/provider. That prevents intentional clearing of those fields during reruns, so synced jobs can retain stale requested model/provider values instead of reflecting the new implicit configuration state.


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

Addresses the three issues raised on PR #581:
- preserve agentic during SQLite rebuild migration
- resolve rerun config from worktree path when present
- allow sync upserts to clear model/provider/requested fields

Validation:
- go test ./internal/storage ./internal/daemon ./cmd/roborev/tui
- go vet ./...

🤖 Generated with [OpenAI Codex](https://openai.com/codex)
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 27, 2026

roborev: Combined Review (fce6437)

Verdict: Changes are mostly sound, but there is one medium-severity backward-compatibility regression in rerun behavior.

Medium

  • internal/daemon/server.go:657: resolveRerunModelProvider only reads job.RequestedProvider, so jobs created before requested_provider existed can rerun with an empty provider even when the original explicit override still exists in job.Provider. That changes rerun behavior for legacy rows after upgrade. Fix by falling back to job.Provider when RequestedProvider is empty, or by backfilling requested_provider during migration before treating it as the only source of truth.

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

@mariusvniekerk
Copy link
Copy Markdown
Collaborator Author

@wesm I think we probably don't want to do that much backwards compat support here since we'll have to guess either way the upgrade goes. This path makes old records behave like they do now.

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 29, 2026

roborev: Combined Review (ae6d92f)

Verdict: 3 medium-severity issues should be addressed before merge to avoid incorrect rerun model/provider resolution.

Medium

  • Legacy reruns can lose explicit model/provider overrides

    • Location: internal/daemon/server.go:659, internal/daemon/server.go:1726
    • Problem: Reruns now rely only on requested_model / requested_provider to preserve explicit overrides. For pre-migration jobs, those fields are empty even when the original run used an explicit --model or --provider, so rerunning a legacy job can silently fall back to the current default model/provider instead of preserving the original override.
    • Suggested fix: Treat empty requested fields on legacy jobs as unknown provenance and fall back to persisted job.Model / job.Provider, or add a compatibility path/migration for older jobs.
  • Fix jobs are routed through the wrong workflow on rerun

    • Location: internal/daemon/server.go:645
    • Problem: workflowForJob maps compact jobs to "fix" but leaves JobTypeFix on the default "review" path, so rerunning completed fix jobs can resolve model/provider using the wrong workflow configuration.
    • Suggested fix: Include storage.JobTypeFix in the "fix" branch and add rerun coverage for fix jobs.
  • Branch-specific config is ignored during rerun model resolution

    • Location: internal/daemon/server.go:663
    • Problem: resolveRerunModelProvider passes "" as the branch argument to agent.ResolveWorkflowConfig, which skips branch-specific overrides and can change the resolved model/provider for reruns.
    • Suggested fix: Pass job.Branch to ResolveWorkflowConfig instead of an empty string.

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

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 30, 2026

roborev: Combined Review (b55b5f2)

Verdict: One medium-severity regression needs to be addressed before merge.

Medium

  • Legacy reruns can lose originally requested model/provider overrides
    Locations: internal/daemon/server.go#L653, internal/daemon/server.go#L1727
    Reruns now resolve from requested_model / requested_provider only. Older jobs created before these fields existed will have them empty, so rerunning a legacy job can silently drop an explicit --model or --provider from the original run and fall back to current defaults or NULL, changing behavior.
    Suggested fix: Fall back to job.Model / job.Provider when RequestedModel / RequestedProvider are empty, or backfill the new requested fields for legacy rows during migration.

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

@wesm
Copy link
Copy Markdown
Collaborator

wesm commented Mar 30, 2026

As intended.

@wesm wesm merged commit d2d671f into main Mar 30, 2026
8 checks passed
@wesm wesm deleted the evaluate-issue-556 branch March 30, 2026 03:06
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.

2 participants