Batch automatic post-commit reviews

[wesm]

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. Each entry records its pending range — the last reviewed boundary and the branch head last observed — so a range is tied to a history, not just a branch name. 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; a pushed branch with no checked-out worktree is flushed from the pushing worktree, and explicit branch flushes run even during a rebase.

There is deliberately no reflog or rename 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. A rename leaves its entry orphaned and the branch whose first-parent chain holds the recorded range adopts it; a branch name reused after a rename hands the old range to the branch that now holds it. The worst case is a redundant review, never a silently skipped commit.

[wesm]

Replaces #1044 (accidentally closed by a faulty force-push to its fork branch; fork push access was lost when it closed). Same squashed commit, review history lives in #1044.

[roborev-ci]

roborev: Combined Review (6819d43)

Pending reviews can be bypassed in branch-rename and non-branch push scenarios.

Medium

• Stale destination checkpoints can skip pending commits after a branch rename — cmd/roborev/postcommit_batch.go:197
  Rename migration is attempted only when the destination has no checkpoint or its checkpoint is off-chain. A stale checkpoint left by a deleted destination branch may lie on the renamed branch’s first-parent chain, causing the source branch’s earlier pending commits to be permanently skipped. This also affects disabled and pre-push planners. Consult rename evidence before trusting an existing destination checkpoint, preferring a usable source checkpoint, and cover normal planning, disabled draining, and push flushing.

• Tag-only and raw-revision pushes can bypass pending-review flushing — cmd/roborev/postcommit.go:261
  Pre-push flushing ignores non-deletion sources other than refs/heads/* and HEAD. Consequently, tag-only pushes and pushes from raw revisions can publish commits in pending batches without first flushing them. Retain every pushed object SHA for ancestor-boundary discovery while limiting direct branch flushes to named branch sources.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m12s

[roborev-ci]

roborev: Combined Review (18ab7e3)

Medium-severity checkpoint migration and history-rewrite gaps could permanently skip commits during post-commit review.

Medium

• cmd/roborev/postcommit_batch.go:197 and cmd/roborev/postcommit_batch.go:409 — Rename migration accepts a destination checkpoint whenever it lies on the renamed branch’s first-parent chain. If that checkpoint is stale but newer than the source checkpoint, earlier pending source commits are permanently skipped. Reconcile rename evidence before accepting any destination checkpoint, and test an on-chain destination checkpoint newer than the source checkpoint.

• cmd/roborev/postcommit_batch.go:570 — When rewritten history has no merge base with the stored checkpoint, recovery reviews only head and preserves the unusable checkpoint. Following a root rebase or unrelated-history rewrite, other pending commits can remain permanently unreviewed. Remap the checkpoint using post-rewrite data or review the complete empty-tree/root-to-head range before advancing it.

---

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

[roborev-ci]

roborev: Combined Review (505b6ed)

Code review found two medium-severity correctness issues in post-commit batching.

Medium

• Rename evidence can be reused indefinitely — cmd/roborev/postcommit_batch.go:284
  If a renamed source branch is recreated, gains a checkpoint, and is later deleted, the renamed branch can adopt that unrelated checkpoint during a subsequent hook. This may overwrite its valid checkpoint and silently skip pending commits. Persist consumed rename migrations or otherwise distinguish the original rename checkpoint from later state under a reused source name. Add coverage for recreate-then-delete before the renamed branch’s next hook.

• Branch and HEAD are resolved inconsistently — cmd/roborev/postcommit_batch.go:175
  The current branch and HEAD are captured by separate Git commands. A concurrent checkout between them can combine one branch’s name with another branch’s commit, enqueueing the wrong range and advancing the wrong checkpoint. Resolve the captured symbolic branch ref directly, or obtain both values through one consistent Git operation.

---

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

[roborev-ci]

roborev: Combined Review (54e0069)

Code review found two medium-severity correctness issues in branch batching and pre-push flushing.

Medium

• cmd/roborev/postcommit.go:261 — Valid non-branch push sources are ignored. Pre-push input is discarded unless its source is refs/heads/* or exactly HEAD. Pushes using sources such as HEAD~1, raw commit refspecs, or tags can transfer commits while leaving pending branch batches unreviewed. Retain every non-deletion pushed commit as an ancestor-flush candidate by resolving or peeling it to a commit; use branch names only for direct checkpoint attribution. Add behavioral coverage for a non-branch source carrying pending commits.

• cmd/roborev/postcommit_batch.go:208 — Checkpoints survive branch deletion and can be incorrectly reused. Because checkpoints are keyed only by branch name, recreating a branch with the same name adopts the previous branch incarnation’s checkpoint. This can produce an incorrect pending count and trigger an immediate, overly broad review. Persist sufficient ref-incarnation or last-observed-head state to detect deletion and recreation and initialize a fresh checkpoint, while preserving explicit rename migration. Add coverage for same-name recreation from related and divergent histories.

---

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

[roborev-ci]

roborev: Combined Review (a164a11)

Review found two medium-severity correctness issues in cross-branch configuration and checkpoint handling.

Medium

• cmd/roborev/postcommit.go:285 — Incorrect configuration used when flushing another branch

  Cross-branch flushing uses the pushing checkout’s root for configuration resolution and daemon requests. When .roborev.toml differs between branches or linked worktrees, the target branch can receive the wrong review mode, agent, exclusions, or workflow. A mismatched excluded_branches policy may return HTTP 200 and advance the checkpoint without reviewing the pending commits.

  Suggested fix: Resolve and retain the target branch’s worktree/config provenance while planning its batch, then use that context consistently for enqueue policy and workflow resolution.

• cmd/roborev/postcommit_batch.go:341 — Reflog expiry can discard pending commits

  A saved identity missing from a readable reflog is treated as definitive evidence that the branch was recreated. Normal reflog expiry or pruning can also remove old identities, causing a dormant pending batch to be discarded and reseeded at HEAD^1. This permanently omits earlier pending commits.

  Suggested fix: Treat a missing reflog identity as inconclusive and preserve, validate, or recover the existing checkpoint. Discard it only when branch recreation is positively established, or use an incarnation marker that does not expire with reflogs.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 18m17s

[roborev-ci]

roborev: Combined Review (798e75a)

Review verdict: Two medium-severity correctness issues could allow commits to bypass post-commit review.

Medium

• Branch renames can silently skip pending commits — cmd/roborev/postcommit_batch.go:301

  When the prior reflog identity has expired or been pruned, the code treats the resulting inconclusive match as a mismatch, discards the candidate checkpoint, and deletes its source state. The renamed branch then seeds at HEAD^1, potentially skipping pending pre-rename commits.

  Suggested fix: Preserve or migrate candidates that remain compatible with the renamed branch’s history when reflog identity is unavailable. Delete source state only when branch recreation is definitively established.

• Unchecked-out branches can be pushed without review — cmd/roborev/postcommit.go:285

  Pushing an explicitly named branch that is not checked out in any worktree defers its batch flush but still allows the push. Its commits can reach the remote unreviewed, with no guaranteed later event to flush them, contradicting the documented pre-push behavior.

  Suggested fix: Flush the branch using its pushed ref and branch-specific configuration, either by resolving configuration from that branch or by retaining the necessary configuration context with its checkpoint.

---

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

[roborev-ci]

roborev: Combined Review (65467fd)

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

Medium

• Recreated branch names can permanently skip pre-rename commits — cmd/roborev/postcommit_batch.go:291
  Ignoring checkpoints whose branch name remains live mishandles branch renames followed by recreating the old name. The renamed branch may seed at the previous tip while the recreated source retains the earlier checkpoint, leaving a pre-rename commit unreviewed by either branch. Preserve branch-incarnation or pending-tip information, or conservatively copy an on-chain checkpoint to the renamed branch without removing it from the recreated source.

• Rebase guard prevents explicit pre-push flushes — cmd/roborev/postcommit.go:83
  The rebase guard also rejects the internal --flush --flush-branch invocation. If an unchecked-out branch is pushed while the fallback worktree is rebasing, the flush is skipped and pending commits can be pushed without review. Restrict the guard to ordinary post-commit invocations and allow flush requests carrying an explicitly captured branch and SHA.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 14m2s

[roborev-ci]

roborev: Combined Review (f4dcf1d)

The batching change has two medium-severity branch-history bugs that can silently skip pending reviews.

Medium

• cmd/roborev/postcommit_batch.go:243 — Existing batch entries do not refresh Tip on ordinary below-threshold commits. After multiple pending commits and a branch rename, a branch containing the stale tip can adopt and delete the orphaned entry despite lacking later pending commits, causing those commits to disappear from batch state and be pushed without review. Update the current branch entry’s Tip to the captured head after every successful normal batch plan while preserving its checkpoint. Add a test covering a rename after multiple pending commits where a branch forked at the former tip runs first.

• cmd/roborev/postcommit_batch.go:674 — Ancestor flushing ignores saved entries whose original branch name no longer resolves. After a branch rename or deletion, pushing pending commits through a tag, SHA, or another non-branch source skips the orphaned entry. Derive the carried boundary from the stored entry tip, or map that tip to a live containing branch, rather than requiring refs/heads/<savedBranch> to exist. Add a rename-plus-tag-push behavioral test.

---

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

[roborev-ci]

roborev: Combined Review (00bb3e5)

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

Medium

• Partial ancestor flush can leave later commits unreviewed — cmd/roborev/postcommit_batch.go:815

  When a pending range A..C is partially flushed through a child fork at B, the state records both the checkpoint and tip as B. A later tag or SHA push containing C then appears to have no pending commits, leaving B..C unreviewed.

  Preserve the recorded descendant tip when flushing an older boundary, advancing only the checkpoint. Add a behavioral test covering a partial child push followed by a tag/SHA push of the original tip.

---

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

[roborev-ci]

roborev: Combined Review (380bd40)

Review identified a medium-severity correctness gap in branch checkpoint flushing.

Medium

• cmd/roborev/postcommit_batch.go:702 — Ancestor flushing accepts any merge base after the checkpoint, including boundaries reachable only through the saved branch tip’s second parent. Pushing a merged side branch can therefore advance the parent branch’s checkpoint onto side history. If the parent branch is later renamed, its first-parent commits and merge commit may become unadoptable and be pushed without review. Restrict flushing to boundaries on the saved branch tip’s first-parent chain, and add a regression test covering a side-branch push followed by renaming and pushing the merge branch.

---

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

[roborev-ci]

roborev: Combined Review (576f2d3)

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

Medium

• cmd/roborev/postcommit_batch.go:680 — Rewritten branch history can bypass pending reviews. Ancestor flushing uses the live branch tip whenever the branch still resolves, ignoring the recorded tip. If a branch is reset or recreated onto divergent history, pushing a tag or SHA containing its old pending commits finds no carried range, allowing those commits to be pushed without review. Detect when entry.Tip is no longer on the live branch’s first-parent chain and flush the recorded range separately, preserving both histories when pushed simultaneously.

• cmd/roborev/postcommit_batch.go:106 — Lock acquisition can block indefinitely. Retries use the command’s unbounded background context, so a stuck or suspended hook holding the lock can indefinitely block later commits and pushes. Use a short timeout for lock acquisition, then log and return; a later hook can recover the range from the unchanged checkpoint.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 9m47s

[roborev-ci]

roborev: Combined Review (3c76878)

Review verdict: Two medium-severity reliability gaps could allow commits to escape review.

Medium

• Orphaned commits can lose their review checkpoint after divergent history — cmd/roborev/postcommit_batch.go:406

  After a branch with pending commits is reset to divergent history, the next post-commit hook replaces its recorded old tip even when handOffPostCommitRange finds no branch containing that tip. A later tag or SHA push of the abandoned tip can therefore push those commits without triggering review.

  Suggested fix: Preserve the old checkpoint as a separate orphaned range until reviewed or flushed, while tracking the rewritten branch separately. Add coverage for reset → new commit → old-tip push.

• Lock contention can permanently skip immediate commit reviews — cmd/roborev/postcommit.go:94

  Every post-commit invocation acquires the batch lock, even when batching is disabled and no batch state exists. If a slow or stuck holder causes a timeout, the hook skips review without recording a checkpoint. Under the default commit-review mode, a later hook reviews only its own HEAD, so the skipped commit is never retried.

  Suggested fix: Bypass the shared lock for non-batched immediate reviews when no state needs draining, or record retryable state before returning after lock failure.

---

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

[roborev-ci]

roborev: Combined Review (4d657d4)

The post-commit batching change has two medium-severity correctness issues involving divergent branch histories and pending review ranges.

Medium

• cmd/roborev/postcommit_batch.go:406 — propagateEscapedPostCommitRange replaces the recorded tip even when handOffPostCommitRange finds no live first-parent branch to receive it. If a branch is force-moved to divergent history, the old pending range is forgotten and a later tag or SHA push will not flush it.

  • Fix: Have the handoff report whether the old range was transferred or covered. Otherwise, preserve it as a separate orphan/incarnation entry before tracking the branch’s new history.

• cmd/roborev/postcommit_batch.go:755 — An abandoned recorded tip is considered only when FirstParentDistance returns onChain=false without an error. Unrelated histories return an error, so force-moving a live branch to an orphan history can hide its old pending tip and prevent review when that tip is later pushed by tag or SHA.

  • Fix: Distinguish unrelated/off-chain histories from unresolved or transient Git failures, and include any resolvable recorded tip that is not on the live branch’s first-parent chain.

---

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

[roborev-ci]

roborev: Combined Review (cdde231)

One medium-severity correctness issue remains in post-commit batching.

Medium

• cmd/roborev/postcommit_batch.go:406 — When a branch is reset or force-moved away from a pending tip and later receives another commit, handOffPostCommitRange may find no live branch containing the old tip, yet the caller still replaces the recorded Tip with the new head. This loses the abandoned pending range, allowing a later tag or SHA push of the old tip to bypass the pre-push flush and leave those commits unreviewed.
  • Suggested fix: Preserve the old range as a separate orphaned entry unless handoff confirms coverage. Add a regression test covering reset → new commit → old-tip push.

---

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