Batch automatic post-commit reviews

[salmonumbrella]

roborev currently queues a review after every commit. That gets noisy fast on repos with lots of small commits.

This adds a repo-local post_commit_batch_size, so the post-commit hook waits for N commits and enqueues one review over the accumulated range. Below the threshold the hook returns without starting or contacting the daemon, so sub-threshold commits stay cheap. Commit mode reviews checkpoint..HEAD; branch mode keeps the full branch scope, just at the configured frequency. Disabling batching drains the current range instead of dropping it.

Checkpoints are kept per branch in a JSON file under the shared git common dir, so linked worktrees agree on the same pending range. Hook runs are serialized with a file lock, and a checkpoint advances only after a successful enqueue, so a failed enqueue retries the same range on the next commit. A new pre-push hook flushes pending work for pushed refs/heads branches before they leave the machine.

There is deliberately no history-rewrite tracking: after a rebase or amend the checkpoint recovers from the merge base and re-reviews from there, and any other error fails open to an immediate single-commit review. The worst case is a redundant review, never a silently skipped commit.

[roborev-ci]

roborev: Combined Review (e80c5b8)

Review identified two medium-severity correctness issues in post-commit batching.

Medium

• Branch-specific repair erases all checkpoints — cmd/roborev/postcommit_batch.go:262
  Repairing an amended/off-chain checkpoint or transient distance error for one branch resets the entire state map. This discards pending checkpoints for other branches and can cause accumulated commits to be skipped. Preserve the loaded state and replace only the affected branch’s checkpoint; reset the whole map only when the state file cannot be decoded.

• Branch renames lose pending commits — cmd/roborev/postcommit_batch.go:164
  Renaming a branch with sub-threshold pending commits leaves its checkpoint under the old branch name. The next commit initializes the renamed branch at HEAD^1, permanently excluding earlier pending commits from commit-mode reviews. Detect branch renames and migrate the checkpoint, for example by tracking the previously active branch/head and transferring its state when the old ref disappears.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 4m21s

[roborev-ci]

roborev: Combined Review (1d3e522)

Medium-severity rebase/amend handling issue requires changes before merge.

Medium

• cmd/roborev/postcommit_batch.go:198 — Rebase/amend handling does not update persisted batch checkpoints or range-shaped review refs. When a checkpoint is rewritten, the next hook treats it as off-chain, reviews only HEAD, and advances past previously pending commits. Existing checkpoint..head reviews also cannot be remapped by the exact-SHA logic.
  • Fix: Integrate batch state and range endpoints into the post-rewrite mapping flow. Add coverage for a completed batch followed by pending commits, a clean rebase, and another commit.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 4m35s

[roborev-ci]

roborev: Combined Review (e386dae)

The change has one medium-severity correctness issue in range remapping.

Medium

• Range jobs can be remapped to a non-equivalent diff — internal/storage/jobs.go:1493

  Range endpoints are remapped independently when their individual patch IDs match. After a branch is rebased onto an updated base, a stored old-base..old-head review can become old-base..new-head, potentially including upstream or otherwise changed commits absent from the original review. This can associate completed reviews with the wrong diff or cause queued reviews to inspect an unintended range.

  Fix: Apply the complete rewrite mapping atomically and remap a range only when the entire range is proven equivalent—at minimum, require validated mappings for both endpoints. Otherwise, preserve the original range.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 8m57s

[roborev-ci]

roborev: Combined Review (f17cdfa)

Range and checkpoint remapping can incorrectly associate or skip reviews after rebases.

Medium

• Range remapping may accept materially different commit ranges — internal/storage/jobs.go:1562
  Range jobs are remapped when only their endpoints have patch-equivalent mappings. An intermediate commit can change during a rebase while both endpoint patches remain unchanged, causing an old review to be associated with a materially different new range. Require range-level equivalence, such as a validated old/new range patch ID, before remapping.

• Changed batch checkpoints can cause rewritten commits to be skipped — cmd/roborev/remap.go:101
  Batch checkpoints are remapped only for patch-equivalent commits. If a checkpoint changes during a rebase, the old checkpoint becomes off-chain; the next hook reviews only HEAD and advances the checkpoint, permanently skipping pending rewritten commits. Track valid rewrite pairs separately from job-remap mappings, and move a changed checkpoint boundary to the replacement commit’s parent so the replacement and pending descendants remain eligible for review.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 5m28s

[mariusvniekerk]

One thing we can potentially also do is for the markers that the post-commit receives to basically go into a cooldown / commit count period. Since the pure n commit form can easily prevent the last commit of a PR from being reviewed.

[roborev-ci]

roborev: Combined Review (97a48fa)

Medium-severity correctness issues remain in post-commit batching and rewrite remapping.

Medium

• cmd/roborev/postcommit_batch.go:345 — Rewrite mappings can corrupt stacked-branch checkpoints. Mappings are applied to every branch whose checkpoint shares the same SHA. Rebasing one stacked branch can therefore move another branch’s checkpoint outside its history, causing the next hook to review only HEAD and skip pending commits. Remap only the affected branch, or verify that the replacement remains on each branch’s current first-parent chain. Add a stacked-branch rebase test.

• cmd/roborev/remap.go:89 — Reordered commits can permanently escape review. Mapping a checkpoint directly to its rewritten counterpart assumes commit order survives an interactive rebase. If a pending commit moves before the checkpoint, future batches exclude it. Use the complete rewrite mapping and ancestry to preserve a boundary before all previously pending commits; when that cannot be proven, reset conservatively before the earliest affected commit. Add coverage for reordered commits across a checkpoint.

• cmd/roborev/postcommit_batch.go:159 — Concurrent hooks can overlap batches and regress checkpoints. Batch processing uses an unlocked read-modify-write sequence, and enqueueing precedes another potentially stale write in advancePostCommitBatch. Overlapping hooks in one checkout can enqueue duplicate ranges and let an older invocation overwrite newer state. Serialize planning, enqueueing, and checkpoint advancement with a per-worktree lock, while preserving the checkpoint when enqueueing fails. Add a concurrent-hook behavior test.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 5m20s

[roborev-ci]

roborev: Combined Review (a6c0e44)

Code review found two medium-severity correctness issues in post-commit batching and rebase checkpoint handling.

Medium

• Pending batches may never be reviewed — cmd/roborev/postcommit.go:86
  Below-threshold batches are persisted but no later flush is scheduled. If development stops before the threshold is reached, the remaining commits—and potentially all commits on a short-lived branch—are never reviewed. Add an idle/cooldown flush or terminal trigger that enqueues the pending range.

• Rebase remapping can include upstream commits or discard pending commits — cmd/roborev/postcommit_batch.go:383
  Checkpoints absent from the rewrite map are ignored. When the checkpoint is the old branch base, rebasing onto an advanced base can leave it on the first-parent chain and cause upstream commits to enter the next batch. If the checkpoint is dropped, the hook can fall back to HEAD and permanently lose surviving pending commits. Derive the new checkpoint from the earliest surviving rewritten pending commit and test both advanced-base rebases and dropped checkpoints.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 6m17s

[roborev-ci]

roborev: Combined Review (e7fb971)

Medium-severity issues remain in Git-hook batching and rewrite remapping.

Medium

• Pre-push flush ignores pushed refs — internal/githook/githook.go:444
  The hook flushes only the checked-out branch instead of refs supplied on stdin. Pushing another branch, using --all, or pushing from another worktree can publish commits while their batch remains pending.
  Fix: Flush pending state for every pushed ref, or safely flush all pending branches/worktrees for the repository.

• Disabling batching can drop pending commits — cmd/roborev/postcommit_batch.go:283
  Switching to immediate-review mode reviews only HEAD and advances the checkpoint, permanently skipping earlier pending commits. A temporary config parse error can produce the same outcome by resolving to batch size 1.
  Fix: Enqueue the accumulated pending range before transitioning modes, and preserve pending state on config-load errors.

• Rebase range remapping requires an unchanged endpoint to be mapped — internal/daemon/server.go:2746
  Git normally reports only rewritten commits, so an unchanged range base is absent from post-rewrite mappings. First-batch and branch-mode review ranges can therefore remain tied to obsolete SHAs.
  Fix: Derive the new boundary from the earliest rewritten commit’s parent, or provide explicit old/new range mappings while retaining combined-diff patch-ID validation.

• Branch rename migration can abandon pending commits — cmd/roborev/postcommit_batch.go:256
  Migration considers only LastBranch. If another branch receives a commit before the original branch is renamed, the renamed branch initializes at HEAD^1 and loses its earlier pending range.
  Fix: Search missing saved branch entries for a unique checkpoint on the current first-parent chain instead of relying only on the last active branch.

---

Reviewers: 2 done | Synthesis: codex, 15s | Total: 6m21s

[roborev-ci]

roborev: Combined Review (8b2b9f5)

Review found three medium-severity correctness issues affecting hook interoperability, rewritten commit ranges, and branch exclusions.

Medium

• internal/githook/githook.go:498 — The embeddable pre-push snippet consumes all hook stdin before invoking the existing pre-push hook. Git LFS or custom validation hooks consequently receive no ref-update records and may silently skip required work. Buffer and replay stdin to both roborev and the existing hook, with a behavioral test verifying that downstream hook code receives the original input.

• internal/daemon/server.go:2751 — When the range base is unchanged, the code always derives a new base from the earliest rewritten commit. If only the head of a multi-commit range is amended, this drops unchanged earlier commits, causing patch validation to reject an otherwise equivalent oldBase..newHead range and leaving review jobs pinned to obsolete SHAs. Validate the original base with the new head first, then fall back to the derived base only if necessary. Add coverage for a multi-commit range where only the head is rewritten.

• cmd/roborev/postcommit.go:249 — Push flushing can target a branch other than the checked-out branch, but enqueue exclusion checks still use the worktree’s current branch. This can permanently drop reviews by advancing a target branch’s checkpoint after an incorrect exclusion, or review a target branch that should be excluded. Apply excluded_branches to the explicitly supplied target branch when present, and test flushing a branch different from the checked-out branch.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 7m5s

[roborev-ci]

roborev: Combined Review (daeeb59)

Review found three medium-severity correctness issues in hook cleanup, renamed-branch flushing, and reordered-range remapping.

Medium

• internal/githook/githook.go:498 — Embedded pre-push hook cleanup can leave an invalid script. Temporary-file commands generated in the embedded snippet are not recognized by isRoborevSnippetLine. Uninstalling or upgrading removes only part of the function, potentially leaving commands and a stray } that block pushes. Make every generated line removable—preferably with explicit block markers—and add uninstall/upgrade coverage for embedded pre-push hooks.

• cmd/roborev/postcommit_batch.go:345 — Renaming a branch can bypass pending review flushing. Push flushing searches only for state stored under the branch’s current name. If the branch was renamed after its last pending commit, the checkpoint remains under the old name and the pre-push hook silently skips the batch. Apply the existing renamed-branch migration logic in planStoredPostCommitBatch, persist the migrated state, and test rename-then-push without an intervening commit.

• internal/daemon/server.go:2744 — Reordered commits can prevent valid review-range remapping. Range remapping assumes the rewritten old head remains the new head. After an interactive rebase reorders patch-equivalent commits, the mapped commit may instead be inside the new range, causing validation of a truncated range and leaving stale SHAs. Derive the new boundaries from the ancestry and ordering of all mapped commits before comparing combined patch IDs, and add coverage for an equivalent reordered range.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 6m51s

[roborev-ci]

roborev: Combined Review (b4840f1)

The change has three medium-severity correctness and performance issues that could leave reviews stale or skip pending commits.

Medium

• internal/daemon/server.go:2805 — Range remapping may exceed the client timeout.
  Remapping makes O(n²) FirstParentDistance calls, each spawning multiple Git processes. A moderate rebase can exceed the remap client’s 10-second timeout, leaving stored range reviews tied to obsolete SHAs. Load each relevant first-parent chain once, map SHAs to positions, and derive the rewritten bounds in memory.

• cmd/roborev/postcommit.go:239 — Pre-push flushing misses some valid refspecs.
  Pushes whose source is not written as refs/heads/...—including explicit HEAD or commit-based refspecs—are ignored. Pending commits may therefore be pushed without the intended final batch review. Resolve the supplied local object or ref to its local branch when possible using its SHA and current/local branch refs.

• cmd/roborev/postcommit_batch.go:260 — Ambiguous rename migration can discard pending commits.
  Deleted branch entries are never pruned, and every on-chain entry is treated as a rename candidate. If several stale entries share an ancestral checkpoint, migration initializes the renamed branch at HEAD^, silently omitting earlier pending commits. Disambiguate using branch history or the saved last branch; when ambiguity remains, conservatively retain the oldest applicable checkpoint.

---

Reviewers: 2 done | Synthesis: codex, 16s | Total: 6m29s

[roborev-ci]

roborev: Combined Review (cf3ba0c)

Review identified one medium-severity correctness issue in post-commit batching.

Medium

• cmd/roborev/postcommit.go:263 — Duplicate review jobs when branches move between worktrees

  Batch state is stored per worktree, while pre-push flushes each branch through every worktree. If a branch with pending commits moves between worktrees, separate state files can retain different checkpoints, producing overlapping commit ranges and duplicate review jobs on subsequent pushes.

  Suggested fix: Store branch checkpoints in shared repository state, or reconcile checkpoints across all worktrees and enqueue only one range per pushed branch.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 8m34s

[roborev-ci]

roborev: Combined Review (2944c7c)

Review found two medium-severity correctness issues in post-commit batching and push flushing.

Medium

• cmd/roborev/postcommit.go:259 — Push flushing may use the wrong worktree. Each branch is flushed against every worktree, but shared batch state means the first successful invocation advances the checkpoint and skips the branch’s actual worktree. Reviews can consequently use the wrong repository config, path, and agent checkout.

  • Fix: Resolve and flush each branch from its associated worktree exactly once, falling back to the current root only for unassociated branches.

• cmd/roborev/postcommit_batch.go:321 — Disabling batching does not restore HEAD reviews. After batching has been enabled, reducing post_commit_batch_size below 2 continues emitting ranges and persisting checkpoints indefinitely because every subsequent commit has pending state.

  • Fix: Drain the accumulated range once, then remove the branch checkpoint after a successful enqueue so subsequent disabled hooks review HEAD.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m51s

[roborev-ci]

roborev: Combined Review (11a10b5)

Review identifies one medium-severity correctness issue in rebase checkpoint remapping.

Medium

• Rebase remapping can skip changed commits — cmd/roborev/postcommit_batch.go:501

  If an earlier reviewed commit changes during a rebase while the saved checkpoint remains patch-equivalent, the checkpoint advances to its rewritten SHA and the changed ancestor is never reviewed, including during pre-push flushing.

  Suggested fix: Detect non-equivalent rewritten ancestors of the checkpoint and move the boundary to the parent of the earliest affected commit, or otherwise force review of the rewritten range.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 10m16s

[wesm]

looking at this

[roborev-ci]

roborev: Combined Review (e56ddbd)

Review verdict: One medium-severity correctness issue should be addressed before merge.

Medium

• Pending checkpoint is not inherited by branches created from another pending branch — cmd/roborev/postcommit_batch.go:363

  If a new branch is created from a still-existing branch with a pending checkpoint, the new branch does not inherit that checkpoint. Pushing it may review only commits added after the fork—or nothing—allowing inherited, unreviewed commits to be pushed.

  Suggested fix: When initializing or flushing a branch, inherit any applicable ancestor branch’s pending checkpoint. Add coverage for branching from a pending branch while retaining the source branch.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m29s

[roborev-ci]

roborev: Combined Review (24dc115)

Medium-severity issues remain in pre-push planning and checkpoint inheritance.

Medium

• cmd/roborev/postcommit_batch.go:355 — Pre-push planning silently skips reviews on state errors. State-load errors, invalid checkpoints, and ancestry failures are treated as “no pending commits,” allowing pending commits to be pushed without review. Distinguish absent or empty state from failures; for errors or off-chain checkpoints, review the pushed branch head/range and repair the checkpoint, consistent with planPostCommitBatch.

• cmd/roborev/postcommit_batch.go:264 — Divergent branches can contribute stale checkpoints. Inheritance verifies that the saved checkpoint is an ancestor of the new branch but does not verify that the saved branch tip is on the new branch’s first-parent chain. This can select an ancient common checkpoint and trigger review of unrelated, already-reviewed history. Require the saved branch tip to be on the first-parent chain; for deleted branches, persist enough branch-tip or rename metadata to validate inheritance.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m31s

[roborev-ci]

roborev: Combined Review (a38d39d)

Review identified two medium-severity correctness and scalability issues in the new batching and remapping flows.

Medium

• Incorrect branch configuration during pre-push flushes — cmd/roborev/review.go:591
  Pre-push branch reviews pass the pushed SHA as headRef, causing GetBranchBase, GetUpstream, and UpstreamIsTrunk to query branch.<sha>.* instead of branch.<name>.*. Partial-batch flushes can therefore ignore the branch’s configured base/upstream and review the wrong range. Resolve branch configuration using refs/heads/<branchName>, while retaining the immutable pushed SHA for merge-base and range calculations.

• Range remapping scales with all historical jobs — internal/daemon/server.go:2729
  Each remap enumerates every historical non-running range job and invokes git log before checking whether the job intersects the rewrite. As jobs accumulate, the synchronous post-rewrite request may exceed the client’s 10-second timeout and prevent relevant recent ranges from being remapped. Query or shortlist ranges whose endpoints may be affected by the supplied mappings, then validate only those candidates.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 9m45s

[roborev-ci]

roborev: Combined Review (4c761b8)

Code review found one medium-severity issue in off-chain checkpoint recovery.

Medium

• cmd/roborev/postcommit_batch.go:430 — When a checkpoint is off-chain, the fallback reviews only Head and then advances the checkpoint to that head. If rewrite remapping failed or the post-rewrite hook was absent, previously pending commits are permanently skipped. Recover from a safe common ancestor, or preserve the old checkpoint until a conservative recovery range is queued successfully.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m1s

[roborev-ci]

roborev: Combined Review (6bb635c)

Review found one medium-severity concurrency issue.

Medium

• cmd/roborev/postcommit.go:87 — The repository-wide lock times out after 10 ms and remains held during daemon startup and the HTTP request. Concurrent commits in different worktrees may silently skip both enqueueing and state recording, even with batching disabled. Use a bounded lock wait or narrow the lock scope while ensuring each contending hook records or enqueues its commit.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 6m47s

[roborev-ci]

roborev: Combined Review (1c8086e)

Medium-severity issues found in checkpoint inheritance and rewrite performance.

Medium

• cmd/roborev/postcommit_batch.go:213 — Pending checkpoints are inherited only when the current branch has none. After rebasing onto a branch with pending commits, remapping can advance the feature checkpoint past those commits, causing them to be omitted from review when pushed. Always compare against eligible pending ancestor checkpoints and retain the oldest boundary.

• cmd/roborev/postcommit_batch.go:614 — Remapping launches multiple synchronous Git subprocesses per rewrite for every tracked branch. Large rebases or accumulated branch state could trigger thousands of subprocesses and substantially delay the post-rewrite hook. Build old/new first-parent ancestry maps once and resolve checkpoints in memory.

---

Reviewers: 2 done | Synthesis: codex, 5s | Total: 7m36s

[roborev-ci]

roborev: Combined Review (7f120f8)

Review found two medium-severity correctness issues in batched post-commit handling.

Medium

• cmd/roborev/postcommit.go:131 — Batched commit reviews become range jobs, but automatic design-review routing handles only single-commit jobs. Enabling batching silently disables auto_design_review follow-ups. Add range-aware dispatch or route each commit separately, with integration coverage for batching plus hook_enabled.

• cmd/roborev/postcommit_batch.go:290, cmd/roborev/postcommit_batch.go:798 — A child branch can omit shared pending commits when it forks before the parent advances further. Detect the common first-parent history between the saved checkpoint/tip and child head, and inherit pending commits when that intersection extends beyond the checkpoint.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m13s

[roborev-ci]

roborev: Combined Review (8d78884)

Potentially exploitable Git argument injection plus three batching/range correctness issues require fixes before merge.

High

• Git argument injection enables file clobbering — internal/daemon/server.go:2768
POST /api/remap does not validate mapping SHAs before constructing a revision range. An option-like base such as --output=/tmp/target can be interpreted by Git and create or truncate files, including through a symlink. Validate every mapping endpoint as an exact commit OID, reject option-like values, and place --end-of-options before revision arguments where supported.

Medium

• Range panel jobs bypass auto-design routing — internal/daemon/panel_enqueue.go:475
  Batched post-commit ranges using hook_review_panel fail the JobTypeReview-only gate. Permit both review and range job types and add range-panel coverage.

• Range classification checks only one commit message — internal/daemon/server_auto_design.go:201
classifierCommitMessage uses git log -1, ignoring earlier trigger messages and allowing a final skip-pattern commit to skip the entire batch. Gather all messages and apply range-aware trigger/skip semantics.

• Random branch flush order can leave commits unreviewed — cmd/roborev/postcommit.go:270
  Each map-ordered flush immediately advances shared state, so an untracked child branch can lose checkpoint inheritance when its tracked source is flushed first. Plan all branches from one locked state snapshot before advancing checkpoints.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 11m29s

[roborev-ci]

roborev: Combined Review (de04f65)

Medium-severity issues remain in batched range auto-design and rewrite-aware range remapping.

Medium

• internal/daemon/panel_enqueue.go:475 — Batched post-commit reviews using a hook panel return early for JobTypeRange, silently skipping auto-design. Allow both review and range job types in maybeDispatchPanelAutoDesign and add panel-range coverage.

• cmd/roborev/remap.go:77 — Range remapping only receives rewrites with matching, nonempty per-commit patch IDs. Ranges ending at empty or merge commits therefore cannot be shortlisted or remapped even when their combined diffs are equivalent. Send all valid rewrite pairs for range-bound discovery while retaining patch-equivalent mappings for exact commit jobs.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 11m10s

[roborev-ci]

roborev: Combined Review (36e35a2)

Review found two medium-severity issues requiring fixes.

Medium

• cmd/roborev/postcommit.go:271 — Pushing a branch without a checked-out worktree falls back to the current checkout’s configuration. Differing or malformed configuration may select the wrong review scope or agent, or skip flushing entirely. Resolve configuration from the pushed head or use an isolated checkout for the target branch.

• internal/daemon/server_auto_design.go:201 — Range auto-design examines only the tip commit message, so message-triggered design reviews in earlier batched commits are silently missed. Load each commit subject in the range and apply trigger/skip rules across the complete set.

---

Reviewers: 2 done | Synthesis: codex, 5s | Total: 7m17s

[roborev-ci]

roborev: Combined Review (63105b0)

Overall verdict: Two medium-severity issues should be addressed before merging.

Medium

• Deferred classification may use inconsistent configuration — internal/daemon/worker_classify.go:303
  Deferred classification reloads branch-scoped config from the reviewed ref, while initial routing uses the working checkout. Reviews using current or uncommitted config may therefore switch classifier or design settings mid-workflow. Persist the resolved config or an ephemeral-checkout marker so only ephemeral jobs reload from the immutable ref.

• Pre-push performs unnecessary worktree creation — cmd/roborev/postcommit.go:280
  For unchecked-out branches, pre-push creates a full temporary worktree before determining whether batch state exists. With batching disabled, unrelated pushes incur unnecessary checkout overhead. Return early when the shared batch-state file is absent and inspect available state before creating a disposable checkout.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m29s

[roborev-ci]

roborev: Combined Review (9ced5ab)

Changes need revision due to one medium-severity configuration consistency issue.

Medium

• internal/daemon/server.go:2493 — Ephemeral pre-push checkouts are discarded without preserving their configuration source. The worker then builds the queued review from the main checkout, so branch-specific settings—such as prompt limits, snapshot directory, severity, and agent options—may differ from those of the pushed commit. Persist an immutable configuration ref or snapshot with the job and resolve against it, or retain a durable exact-ref checkout for processing.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 11m0s

[roborev-ci]

roborev: Combined Review (f05a419)

Review found two medium-severity correctness issues in post-commit batching and pre-push flushing.

Medium

• Branch renames can permanently omit pending commits from review — cmd/roborev/postcommit_batch.go:197

  Checkpoints are keyed only by branch name. Renaming a branch strands its pending checkpoint under the old name; an immediate push finds no batch, while the next commit initializes a new checkpoint at HEAD^1, permanently omitting the pre-rename commits from commit-mode review.

  Suggested fix: Detect and migrate orphaned checkpoints when the old ref has disappeared and the checkpoint is an ancestor of the renamed branch, or conservatively review the unresolved pending range before establishing a new checkpoint.

• Cross-branch pre-push flushing applies exclusions to the wrong branch — cmd/roborev/postcommit.go:151

  Pre-push flushing can enqueue a branch other than the checked-out branch, but the daemon’s exclusion check examines the checkout’s current branch. This can review an excluded pushed branch or skip a non-excluded pushed branch when the checked-out branch is excluded. The checkpoint is then advanced at cmd/roborev/postcommit.go:195, silently losing the pending review.

  Suggested fix: Make enqueue branch filtering prefer req.Branch, falling back to the current branch only when it is empty, and add coverage for cross-branch pre-push cases.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 7m53s

[roborev-ci]

roborev: Combined Review (1192ec2)

Changes need revision: three medium-severity batching issues can silently omit commits from review.

Medium

• Off-chain checkpoints are discarded when batching is disabled — cmd/roborev/postcommit_batch.go:268
  After a rebase, the hook reviews only HEAD and deletes the checkpoint, silently skipping accumulated rewritten commits. Recover and drain off-chain checkpoints using the same merge-base logic as enabled batching, preserving the checkpoint if recovery fails.

• Pushed branches use the wrong worktree policy — cmd/roborev/postcommit.go:267
  Every pushed branch is flushed using the worktree where git push ran, so non-current branches are evaluated with the current branch’s config and exclusions. A skipped enqueue can then advance the pushed branch’s checkpoint without creating a review. Resolve the pushed branch’s worktree/config, or ensure enqueue policy uses the supplied branch before advancing its checkpoint.

• Branch renames strand pending checkpoints — cmd/roborev/postcommit_batch.go:197
  Because checkpoints are keyed only by branch name, a renamed branch loses access to its pending state, potentially omitting pre-rename commits permanently. Migrate uniquely matching stale checkpoints using branch-tip information, or conservatively review from the nearest applicable checkpoint when flushing an untracked branch.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m39s

[roborev-ci]

roborev: Combined Review (afb6273)

Medium-severity regressions remain in post-commit batching and branch exclusion handling.

Medium

• internal/daemon/server.go:2457 — Manual reviews can be incorrectly excluded

  Every request with an explicit branch now undergoes branch-exclusion checks, including manual roborev review --branch=... requests. This conflicts with the documented behavior that exclusions apply only to automatic reviews.

  Fix: Apply the explicit branch to exclusion checks only for post-commit requests while preserving existing insights behavior. Add coverage for a source-empty manual request targeting an excluded branch.

• cmd/roborev/postcommit.go:145 — Concurrent repository changes can skip pending work

  After batching captures immutable batch.Head and batch.Branch, the normal branch-review path re-reads live HEAD and the current branch. A concurrent commit or checkout can enqueue a different range or branch while advancing the original branch’s checkpoint, potentially skipping work.

  Fix: For enabled batches, call tryBranchReviewForRef with batch.Head and batch.Branch, and use batch.Branch in the enqueue request.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m56s

[roborev-ci]

roborev: Combined Review (e8d0b82)

Changes need adjustment: one medium-severity branch-rename issue can permanently omit commits from review.

Medium

• cmd/roborev/postcommit_batch.go:343 — Batch state is keyed strictly by branch name. If a branch is renamed after accumulating a partial batch, the pre-push lookup misses the checkpoint and skips flushing. A later commit initializes the renamed branch’s checkpoint at its immediate parent, permanently excluding the pending pre-rename commits from review.
  • Suggested fix: Recover or migrate checkpoints across branch renames—for example, select a unique stored checkpoint on the pushed head’s first-parent chain and move it to the new branch key. Add a behavioral rename-and-push test.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 6m32s

[roborev-ci]

roborev: Combined Review (c5707c8)

Code review found one medium-severity correctness issue in branch rename detection.

Medium

• Stale checkpoints can cause incorrect batching or skipped commits — cmd/roborev/postcommit_batch.go:259

  Treating a missing branch whose checkpoint is an ancestor of the current head as a rename is unreliable because deleted-branch entries are never pruned. A new branch may inherit an unrelated stale checkpoint, enqueue an unexpectedly large historical range, and reach the batch threshold prematurely. Conversely, multiple stale matches during a real rename are considered ambiguous, so pre-push flushes nothing; the next commit then seeds at HEAD^1, permanently skipping pending pre-rename commits.

  Suggested fix: Persist stronger branch identity or use reflog rename evidence. If identity remains ambiguous, preserve and conservatively review the pending range instead of reseeding at HEAD^1.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m41s

[roborev-ci]

roborev: Combined Review (fdf1c23)

Two medium-severity issues remain in checkpoint migration and best-effort pre-push behavior.

Medium

• Consecutive renames can skip pending commits — cmd/roborev/postcommit_batch.go:258
  Checkpoint migration follows only the most recent rename. If old is renamed to middle and then new before another hook runs, the state remains under old, while the helper finds only middle, silently skipping pending pre-rename commits. Walk the ordered rename chain in the current branch’s reflog until a stored checkpoint is found, and add consecutive-rename coverage to post-commit and pre-push tests.

• Hook failures can incorrectly abort pushes — internal/githook/githook.go:445
  The standalone pre-push hook returns the roborev process’s nonzero exit status to Git. A stale, broken, or unexpectedly failing binary can therefore block a push even though enqueueing is intended to be best-effort. Mask failures from the flush command, exit successfully, and add a behavioral test using a fake binary that exits nonzero.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m34s

[roborev-ci]

roborev: Combined Review (047a3c4)

Verdict: One medium-severity issue could allow inherited commits to be pushed without review.

Medium

• Inherited pending commits are skipped on new branches — cmd/roborev/postcommit_batch.go:202

  A new branch always initializes its checkpoint at HEAD^1, ignoring unreviewed commits inherited from its parent branch. If child is created from parent while parent has a pending commit, pushing only child can send that inherited change upstream without review.

  Suggested fix: During pre-push, flush pending branch ranges whose tips are ancestors of the pushed head, or initialize copied branches from the appropriate ancestor checkpoint using reliable branch-creation evidence. Add coverage for stacked-branch pushes.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m9s

[roborev-ci]

roborev: Combined Review (af62c74)

Review identified two medium-severity correctness issues in post-commit batching and pre-push flushing.

Medium

• Ancestor flushing can skip pending parent commits — cmd/roborev/postcommit_batch.go:449
  Flushing requires the saved branch tip to be on the pushed head’s first-parent chain. If a child branches at pending commit P1 and the parent advances to P2, pushing the child excludes P1 from its range but does not flush the parent because P2 is not an ancestor. P1 can therefore be pushed without review.
  Fix: Find the latest shared first-parent commit between each stored branch and the pushed head. If it is newer than the checkpoint, flush through that commit while retaining later parent commits as pending.

• Disabling batching after a branch rename can leave commits undrained — cmd/roborev/postcommit_batch.go:301
  The disabled-batching drain path does not migrate checkpoints after a rename. It reviews only the current HEAD, leaving pending pre-rename commits under the old branch name.
  Fix: When the current branch has no checkpoint, apply the same reflog-based rename migration used by the enabled and pre-push planners before calculating and clearing the pending range.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m42s

[roborev-ci]

roborev: Combined Review (8ad1320)

Batching is generally sound, but branch rename handling can skip pending commits and allow them to be pushed unreviewed.

Medium

• cmd/roborev/postcommit_batch.go:449 — Ancestor flushing misses renamed branches. Resolving refs/heads/<old-name> fails for checkpoints stranded under a renamed branch. Pushing a child of that branch then reviews only the child’s range, leaving the parent’s pending commits unreviewed. Reconcile rename checkpoints for all local branches before discovering pushed ancestors, then calculate ancestor boundaries from the migrated state.

• cmd/roborev/postcommit_batch.go:197 — Renaming to a previously used branch name can select a stale checkpoint. Migration runs only when the destination has no checkpoint, but deleted branch entries persist. If both source and destination checkpoints exist, the stale destination may override the source branch’s pending range and permanently skip commits. Check reflog rename evidence before trusting the destination entry and explicitly reconcile both checkpoints.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 13m42s

[roborev-ci]

roborev: Combined Review (34f88b0)

Medium-severity branch-state edge cases can allow pending commits to bypass review.

Medium

• cmd/roborev/postcommit_batch.go:472 — Ancestor flushing requires the saved parent branch ref to remain resolvable. If a child is created from a pending parent tip and that parent is later renamed, deleted, or reset, the parent state is skipped and its pending commit may be pushed without review. Persist the last observed pending tip or reconcile stale branch identities before ancestor discovery, with coverage for renamed or removed parent refs.

• cmd/roborev/postcommit_batch.go:271 — Rename migration unconditionally adopts and deletes state for historical source names. If an old source branch name is recreated and accumulates a new batch, planning the renamed branch can steal its live checkpoint, causing earlier pending commits on the recreated branch to be omitted. Preserve source state when its ref currently exists or use stable branch identities, and add a behavioral test for recreating the source branch before migration.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 10m27s

[roborev-ci]

roborev: Combined Review (4bbe744)

Medium-severity checkpoint edge cases can permanently skip commits from review.

Medium

• Rename migration can accept a stale destination checkpoint — cmd/roborev/postcommit_batch.go:197
  Rename migration runs only when the destination has no checkpoint or its checkpoint is off-chain. If a branch is renamed onto a deleted branch name whose stale checkpoint is on the renamed branch’s first-parent chain, earlier commits pending under the source name are permanently skipped. Reconcile usable reflog rename evidence before accepting the destination checkpoint, including the on-chain case, while preserving checkpoints for recreated live source branches.

• Renamed or deleted parent branches are omitted during ancestor flushing — cmd/roborev/postcommit_batch.go:481
  Ancestor flushing requires the saved branch name to resolve. If a parent with pending commits is renamed or deleted after a child is created, pushing only the child skips the unresolved parent entry, even though the push contains those commits. Persist the last observed branch tip alongside its checkpoint or reconcile stale saved names to renamed refs before ancestor discovery; add coverage for pushing a child after its pending parent is renamed or deleted.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 10m33s

[wesm]

declined. merging

[roborev-ci]

roborev: Combined Review (93efd79)

Review batching has three medium-severity correctness issues affecting job discovery, pre-push flushing, and renamed-branch checkpoints.

Medium

• Batched reviews cannot be found through SHA-based lookups — cmd/roborev/postcommit.go:136
  Batched jobs store <checkpoint>..<head> as GitRef, but show HEAD, wait HEAD, and refine’s review discovery require an exact git_ref = <head>. As a result, these commands cannot find pending or completed batched reviews.
  Suggested fix: Persist the range’s head SHA separately, or add a repository-scoped lookup matching the range’s resolved end commit. Add coverage for show, wait, and refine.

• Detached-HEAD pushes can bypass pending-review flushing — cmd/roborev/postcommit.go:261
  Pre-push processing discards HEAD when detached, along with other non-branch local sources. The pushed SHA is then omitted from ancestor analysis, potentially allowing pending commits to be pushed without flushing.
  Suggested fix: Retain every nonzero pushed object ID for ancestor-batch detection, even when no local branch name can be derived.

• A stale destination checkpoint can shadow the correct checkpoint after a branch rename — cmd/roborev/postcommit_batch.go:197
  Rename migration only runs when the destination lacks a checkpoint or its checkpoint is off-chain. If a deleted destination branch left an on-chain stale checkpoint, it can override the source branch’s actual checkpoint and permanently skip earlier pending commits.
  Suggested fix: Reconcile rename evidence before accepting an existing destination checkpoint, including the on-chain case, while preserving recreated-source safeguards.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 11m19s

[wesm]

some kind of a force push failure, I'm opening a new PR

[wesm]

Superseded by #1088, which carries the same squashed commit (original authorship preserved). This PR was accidentally closed by a faulty force-push that emptied the head branch.