Skip to content

follow-up to #619: enqueue and rerun config resolution fixes#621

Open
cpcloud wants to merge 4 commits intoroborev-dev:mainfrom
cpcloud:followup-619-config-resolution-fixes
Open

follow-up to #619: enqueue and rerun config resolution fixes#621
cpcloud wants to merge 4 commits intoroborev-dev:mainfrom
cpcloud:followup-619-config-resolution-fixes

Conversation

@cpcloud
Copy link
Copy Markdown
Contributor

@cpcloud cpcloud commented Apr 3, 2026

Why This PR Exists

PR #619 merged before the last two follow-up commits from that branch were pushed. Those changes are not part of main today.

This PR ports only that remaining delta onto current main.

What Was Left Out

  • enqueue requests from linked worktrees still resolved reasoning and workflow config from the main repo root instead of the active checkout
  • reruns still re-resolved model and provider through repo config instead of preserving requested overrides
  • fix and rerun paths still hard-failed when stored worktree metadata was stale instead of falling back to the main repo checkout
  • explicit CLI reasoning could still mask a parseable but invalid repo reasoning override

What This Changes

  • uses the worktree path for enqueue config resolution when the request originates from a linked worktree
  • preserves requested model and provider on rerun before repo config validation and model resolution
  • restores repo-root fallback when stored worktree metadata is stale for fix and rerun flows
  • validates repo reasoning overrides even when an explicit reasoning flag is supplied
  • adds regression coverage for the enqueue, rerun, fix, and reasoning-resolution paths above

Context

This is a direct follow-up to #619. The reason for a third PR is only that the last two fixes were still local when #619 merged, so they were not included in the changes that landed on main.

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (70b4d36)

Summary verdict: Changes introduce 2 high-severity and 1 medium-severity issue that should be addressed before merge.

High

  • internal/daemon/server.go:2497
    handleFixJob no longer fails on a stale parentJob.WorktreePath and silently falls back to parentJob.RepoPath. For reviews created from a separate worktree or branch, this can generate fixes against a different checkout than the one originally reviewed, causing edits to land on the wrong branch.

  • internal/config/config.go (validateRepoReasoningOverride)
    validateRepoReasoningOverride passes repoValue(repoCfg) into NormalizeReasoning without first checking whether the configured value is empty. If a repo has a valid .roborev.toml but does not set a reasoning override, an explicit CLI override like --reasoning fast can be rejected incorrectly.

Medium

  • internal/daemon/server.go:748
    resolveRerunModelProvider now falls back to job.RepoPath when job.WorktreePath is stale, but this only affects config/model resolution. The rerun job can still be re-queued with the stale WorktreePath, so it may later fail during execution or run against an unintended checkout.

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

@cpcloud
Copy link
Copy Markdown
Contributor Author

cpcloud commented Apr 3, 2026

Addressed in 9a752fc.

Changes in this update:

  • restore stale-worktree rejection for background fix jobs
  • reject reruns when the stored worktree path is stale, while still preserving requested model/provider when config is invalid
  • ignore empty repo reasoning values when validating explicit reasoning overrides
  • normalize the enqueue worktree-path assertion for macOS (/var vs /private/var)

Local verification:

  • go test -race -shuffle=on -tags integration ./internal/daemon
  • go test ./...
  • go vet ./...

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (9a752fc)

Summary verdict: Changes are mostly sound, but there is one medium-severity regression that should be addressed before merge.

Medium

  • internal/daemon/server.go:753
    resolveRerunModelProvider now returns early when job.RequestedModel is set, which skips agent.ResolveWorkflowConfig(...). This removes upfront validation that job.Agent still resolves under the current config, so reruns for removed or renamed agents can proceed through setup and fail later at execution time instead of being rejected immediately.
    Suggested fix: still run workflow/agent resolution for validation, then override the resolved model/provider with job.RequestedModel and job.RequestedProvider.

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

@cpcloud
Copy link
Copy Markdown
Contributor Author

cpcloud commented Apr 3, 2026

Addressed in 8ac85c5.

resolveRerunModelProvider now validates the stored rerun agent before honoring RequestedModel / RequestedProvider, so reruns with unknown or renamed agents fail immediately instead of carrying the stale agent name forward.

This keeps the earlier invalid-config behavior: requested model/provider overrides are still preserved when repo config is malformed, but known-agent validation still happens up front.

Local verification:

  • go test ./internal/daemon -run TestResolveRerunModelProvider|TestHandleRerunJob
  • go test -race -shuffle=on -tags integration ./internal/daemon
  • go test ./...
  • go vet ./...

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (8ac85c5)

Verdict: Changes are mostly sound, but there is one medium-severity correctness gap in rerun model override handling.

Medium

  • internal/daemon/server.go:756: resolveRerunModelProvider still calls agent.ResolveWorkflowConfig(...) before checking job.RequestedModel. That means a rerun with stored model/provider overrides can still fail because of unrelated, parseable repo-config errors, which undermines the intended "preserve requested overrides" behavior.
    • Why it matters: reruns that should succeed using the stored override path can be blocked by invalid workflow config that should have been bypassed.
    • Suggested fix: if job.RequestedModel is set, validate the worktree path and stored agent first, then return the requested model/provider without calling ResolveWorkflowConfig; alternatively, add explicit coverage for parseable invalid repo config and handle that case before workflow resolution.

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

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