Skip to content

follow-up to #618: config, worktree, and CI reasoning fixes#619

Merged
wesm merged 9 commits intoroborev-dev:mainfrom
cpcloud:followup-618-config-fixes
Apr 3, 2026
Merged

follow-up to #618: config, worktree, and CI reasoning fixes#619
wesm merged 9 commits intoroborev-dev:mainfrom
cpcloud:followup-618-config-fixes

Conversation

@cpcloud
Copy link
Copy Markdown
Contributor

@cpcloud cpcloud commented Apr 3, 2026

Why This PR Exists

PR #618 ("Honor global reasoning defaults") merged first, but the original working branch still had a set of review-driven follow-up fixes that landed after the squash merge. Those fixes never reached main.

This PR ports that missing follow-up series onto current main without dragging the old pre-merge branch history forward.

Motivation

The merge of #618 fixed the top-level global reasoning default issue, but it left a set of related edge cases and regressions unresolved:

  • malformed global and repo config could still be surfaced too late or in the wrong places
  • explicit reasoning handling could still interact incorrectly with repo-config validation
  • background fix jobs and reruns could resolve config from stale or invalid worktree paths
  • CI review fallback had been widened too far and could mask unexpected config-load failures instead of only malformed config

The net effect was that reasoning/config behavior was still inconsistent across refine, enqueue, fix, rerun, batch, backup-resolution, and CI paths.

What This Changes

  • validates repo config at the intended CLI and daemon entrypoints instead of letting explicit reasoning or shared workflow resolution bypass those errors
  • preserves explicit reasoning overrides while still surfacing invalid repo/global config when that config must be consulted
  • makes background fix jobs honor the active fix reasoning/model config
  • keeps batch, CI, and backup-resolution behavior consistent when repo config is malformed
  • validates fix-job worktree paths before using them for config resolution
  • clears stale worktree metadata so reruns do not reintroduce invalid config resolution later
  • narrows CI fallback so only malformed repo config falls back to global/default settings; unexpected config-load failures still fail fast
  • carries the Windows test cleanup needed for the enqueue reasoning test

Testing

  • go test ./...
  • go vet ./...

Context

This is a direct follow-up to #618, created because the refined fixes were still on the original branch after #618 had already merged.

@cpcloud cpcloud changed the title config: port post-merge reasoning follow-ups follow-up to #618: config, worktree, and CI reasoning fixes Apr 3, 2026
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (0666baf)

Summary: Changes are mostly sound, but there are 2 correctness issues that should be addressed before merge.

High

  • internal/daemon/server.go:2490 (handleFixJob)
    When a parent review job references a worktree that no longer validates, the new flow silently clears WorktreePath and falls back to parentJob.RepoPath. That can redirect a fix job from the original isolated checkout into the user’s main repository checkout, causing edits against the wrong branch/worktree instead of failing fast.
    Recommended fix: If parentJob.WorktreePath is set but validatedWorktreePath(...) returns empty, reject the fix request with a clear error rather than enqueueing the job against RepoPath.

Medium

  • internal/daemon/server.go:744 and internal/daemon/server.go:1851 (resolveRerunModelProvider, handleRerunJob)
    Rerun model/provider resolution now ignores an invalid worktree for config lookup, but the rerun still operates on the original job record. If that stored WorktreePath is stale, the request can be accepted using config from the repo root while the job still targets the stale checkout, leading to inconsistent behavior and likely a later execution failure.
    Recommended fix: Validate the stored worktree during rerun handling and either fail immediately or update/clear the rerun job’s WorktreePath consistently with the path used for model resolution.

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

@cpcloud
Copy link
Copy Markdown
Contributor Author

cpcloud commented Apr 3, 2026

Addressed the stale-worktree review findings in 1f6136a.

Changes:

  • handleFixJob now rejects a parent job whose stored WorktreePath no longer validates, instead of silently falling back to the main repo checkout.
  • reruns now also fail fast on stale worktree paths via resolveRerunModelProvider, so model/config resolution and eventual execution stay aligned.
  • added regressions for both fix and rerun rejection paths.

Validation:

  • go test ./...
  • go vet ./...

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (1f6136a)

No medium-or-higher findings; the reviewed changes look clean.

All reported findings were below the requested reporting threshold, and no agent identified any Medium, High, or Critical issues.


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

@wesm wesm merged commit 4c7ad82 into roborev-dev:main Apr 3, 2026
8 checks passed
@cpcloud cpcloud deleted the followup-618-config-fixes branch April 3, 2026 15:09
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