Skip to content

fix(pr-review): force-fetch + verify worktree HEAD matches PR head#175

Merged
jacsamell merged 2 commits into
mainfrom
fix/pr-review-worktree-force-sync
May 19, 2026
Merged

fix(pr-review): force-fetch + verify worktree HEAD matches PR head#175
jacsamell merged 2 commits into
mainfrom
fix/pr-review-worktree-force-sync

Conversation

@jacsamell
Copy link
Copy Markdown
Contributor

@jacsamell jacsamell commented May 19, 2026

Symptom

Roy reported judges posting 10 "new" findings on aetheronhq/aetheron-connect-v2#1266 that were all against pre-rewrite line numbers and content — referencing files/tables/triggers that no longer existed in the PR.

Root cause

PR was force-pushed (rebase / squash) several times. The review worktree at `~/.cube/worktrees//pr-review-/` was synced via `git fetch origin ` without `--force`. On force-push, the local ref refused to fast-forward and silently stayed stale. `git reset --hard origin/` then reset to the old ref. Judges Read pre-rewrite files. We logged a warning and proceeded.

Confirmed live: worktree HEAD was at `fda636a3` (May 19 12:29) while the actual PR head was `62e9fbd1` — 6 commits behind including the rewrites that obsoleted the cube findings.

Fix

  1. `git fetch --force` — handles force-pushes. Failure is now fatal.
  2. `git clean -fdx` after reset — leftover untracked files from a prior round could mask file moves.
  3. Verify worktree HEAD == `pr.head_sha` after sync. Abort loud on mismatch with an actionable hint.
  4. Log the synced SHA in the banner so this regression is visible in run output.

Test plan

  • Force-push a PR, re-run `cube pr-review ` — worktree updates to the new head
  • Manually pin worktree to an old commit, run cube — fails loud with SHA mismatch
  • Normal (non-force-pushed) re-runs still work

🤖 Generated with Claude Code

Summary

This PR fixes a critical issue where judges posted incorrect findings after PR force-pushes due to stale review worktrees. The root cause was that git fetch without --force failed to update local refs during force-pushes, causing subsequent resets to stale commits.

Changes

peer_review.py – Strengthened worktree synchronisation to be deterministic and fail-fast:

  • Use git fetch --force (failures now fatal)
  • Execute git clean -fdx after reset to remove stale untracked files
  • Verify synced worktree HEAD matches expected PR head SHA; abort loudly if mismatched with actionable guidance
  • Log the synced SHA in output for visibility

auto_approve.py – Improved cube identity resolution:

  • When a GitHub App is configured, resolve bot identity via the App rather than falling back to the operator
  • Fail closed if the App is configured but login cannot be fetched
  • Added caching of derived bot login

app_auth.py – Added GitHub App bot login discovery:

  • New get_app_login() function that fetches and caches the App's bot login via /app endpoint
  • Returns None gracefully if not configured or if the API response lacks required fields

Outcome

Eliminates the risk of running judges against stale PR content after force-pushes, ensuring findings map to correct line numbers and content. Force-push detection is now explicit and actionable rather than silent and error-prone.

Review Change Stack

jacsamell and others added 2 commits May 19, 2026 16:17
…rator

The self-approve guard exists so the cube bot can't approve its own
PRs. But `_cube_login()` called `gh api user` against the operator's
gh-auth (e.g. jacsamell) and used that as the cube identity. Result:
the operator was blocked from getting auto-approve on any PR they
authored, even though reviews are posted by `the-agent-cube[bot]` and
have nothing to do with the operator's GitHub account.

When the GitHub App is configured, the cube identity is the bot, not
the operator. New `get_app_login()` in `app_auth.py` derives the bot
login from the App's slug via the JWT-authenticated `/app` endpoint
(GitHub Apps can't call `/user`). Result is cached in-process.

`_cube_login()` now:
  * Returns the App bot login (e.g. `the-agent-cube[bot]`) when the
    App is configured.
  * Falls back to `gh api user` only when no App is set up.
  * Fails closed when the App is configured but its login can't be
    fetched — better than pretending the operator is the bot.

Verified live: `_cube_login()` returns `'the-agent-cube[bot]'` on a
configured workstation.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: judges kept posting "new" findings against code Roy had
already rewritten. Walking through aetheronhq/aetheron-connect-v2#1266,
all 10 most recent comments were against pre-rewrite line numbers and
content — referenced files/tables/triggers that no longer existed.

Root cause: PR was force-pushed (rebase / squash) several times. The
review worktree synced via `git fetch origin <head>` got a non-fast-
forward and the local ref refused to update. Then `git reset --hard
origin/<head>` reset to a stale ref. Judges Read pre-rewrite files
and re-raised findings. We logged a warning but proceeded.

Three changes:
  1. `git fetch --force origin <head> <base>` — handles force-pushes.
     Failure is now fatal instead of a warning.
  2. After reset, `git clean -fdx` — leftover untracked files from a
     prior round (e.g. moved files) could mask file-list changes.
  3. Verify worktree HEAD matches `pr.head_sha` after sync. Abort
     loud on mismatch with an actionable hint to delete the worktree.

Banner now logs the head SHA being synced to so this regression is
visible in the run output.

Verified live: PR #1266 worktree was stuck at fda636a3 (6 commits
behind 62e9fbd1). `git fetch --force` brought it current; HEAD
verification would have caught the stale state.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: be2b4bc2-8807-4375-9452-669522191b99

📥 Commits

Reviewing files that changed from the base of the PR and between 9dfe6e3 and 40eea58.

📒 Files selected for processing (3)
  • python/cube/commands/auto_approve.py
  • python/cube/commands/peer_review.py
  • python/cube/github/app_auth.py

Walkthrough

This PR enhances two independent workflows: it introduces GitHub App bot identity caching for auto-approve gating logic, and strengthens the peer-review worktree synchronisation with deterministic cleanup and commit SHA validation.

Changes

GitHub App Bot Identity Resolution

Layer / File(s) Summary
GitHub App bot login discovery and caching
python/cube/github/app_auth.py
New get_app_login() function discovers the configured GitHub App's slug, derives the bot login as <slug>[bot], caches the result, and returns None on configuration or API failures.
Auto-approve cube identity via GitHub App configuration
python/cube/commands/auto_approve.py
_gh_json now accepts optional env parameter for subprocess execution, and _cube_login uses get_app_login() to determine cube identity from configured App credentials with fail-closed semantics.

Deterministic PR Review Worktree Sync

Layer / File(s) Summary
Worktree sync parameter and safety checks
python/cube/commands/peer_review.py
_ensure_pr_review_worktree adds expected_head_sha parameter, uses forced fetch with failure handling, hard-resets and cleans existing worktrees, and validates synced HEAD SHA against the expected value, aborting on mismatch.
PR review workflow integration and error handling
python/cube/commands/peer_review.py
_run_pr_review logs the target PR head SHA, calls the updated worktree helper with expected SHA, converts errors to user-facing output with exit(1), and displays the head SHA in worktree reporting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • aetheronhq/agent-cube#153: Both PRs refine the auto-approve gating flow in auto_approve.py—this PR updates cube identity resolution using the GitHub App credentials, whilst the retrieved PR adds the --allow-approve gate that posts APPROVE based on those identity conditions.

Poem

🐰 Whiskers twitch with delight,
App logins cached, worktrees synced tight,
No stale commits shall pass the gate,
Identity true, approval's fate.
Fresh as morning dew, ready to rate!


Comment @coderabbitai help to get the list of available commands and usage tips.

@jacsamell jacsamell merged commit 09bf3d4 into main May 19, 2026
0 of 2 checks passed
@jacsamell jacsamell deleted the fix/pr-review-worktree-force-sync branch May 19, 2026 12:42
jacsamell added a commit that referenced this pull request May 19, 2026
…med judges

Symptom: cube panel kept posting findings against pre-rewrite code on
aetheronhq/aetheron-connect-v2#1266 even after PR #175's --force fetch
landed. Same 10+ findings against lines/tables that no longer exist.

Two more layers of the bug surfaced:

(1) Duplicate worktrees, only one synced.
    PR reviews historically used `~/.cube/worktrees/<project>/pr-<n>/`
    (legacy, created by `_prefetch_worktrees`). PR #172 added a
    second one at `pr-review-<n>/` for the new file-routing flow.
    Both worktrees existed in parallel; only the new one had the
    --force fetch + HEAD verification from #175. The other stayed
    pinned to whatever it was last synced to.

    Worse: `build_peer_review_prompt` told judges to Read from the
    LEGACY path, while `_apply_file_scope` told them to Read from the
    NEW path. Same prompt, two locations.

    Confirmed live: both worktrees existed on PR #1266; the legacy
    one was stuck at fda636a3 (6 commits behind 62e9fbd1).

(2) Resumed judges had stale session memory.
    Sessions persist across panel runs. On the next run, the LLM
    continues its prior conversation — including its prior reasoning
    about file contents, line numbers, function shapes. Even when the
    resumed judge Reads fresh files, its reasoning anchors on the old
    memory. The previous "RE-REVIEW" hint was too soft to displace it.

Fixes:
  * Skip `_prefetch_worktrees` in PR-review mode when a dedicated
    `review_worktree` was passed. The new sync is authoritative; the
    legacy sync just made a second stale copy.
  * Pipe `review_worktree` through `build_peer_review_prompt` so the
    prompt's worktree path matches the file-scope block. One location.
  * Replace the meek "verify commit" hint with a hard "CURRENT HEAD"
    callout that names the SHA, says "your prior reasoning may be out
    of date", and lists 4 explicit steps: Re-Read fresh, verify each
    prior finding still applies, add new ones, drop the rest.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jacsamell added a commit that referenced this pull request May 19, 2026
…med judges (#176)

* fix(pr-review): eliminate stale legacy worktree + signal HEAD to resumed judges

Symptom: cube panel kept posting findings against pre-rewrite code on
aetheronhq/aetheron-connect-v2#1266 even after PR #175's --force fetch
landed. Same 10+ findings against lines/tables that no longer exist.

Two more layers of the bug surfaced:

(1) Duplicate worktrees, only one synced.
    PR reviews historically used `~/.cube/worktrees/<project>/pr-<n>/`
    (legacy, created by `_prefetch_worktrees`). PR #172 added a
    second one at `pr-review-<n>/` for the new file-routing flow.
    Both worktrees existed in parallel; only the new one had the
    --force fetch + HEAD verification from #175. The other stayed
    pinned to whatever it was last synced to.

    Worse: `build_peer_review_prompt` told judges to Read from the
    LEGACY path, while `_apply_file_scope` told them to Read from the
    NEW path. Same prompt, two locations.

    Confirmed live: both worktrees existed on PR #1266; the legacy
    one was stuck at fda636a3 (6 commits behind 62e9fbd1).

(2) Resumed judges had stale session memory.
    Sessions persist across panel runs. On the next run, the LLM
    continues its prior conversation — including its prior reasoning
    about file contents, line numbers, function shapes. Even when the
    resumed judge Reads fresh files, its reasoning anchors on the old
    memory. The previous "RE-REVIEW" hint was too soft to displace it.

Fixes:
  * Skip `_prefetch_worktrees` in PR-review mode when a dedicated
    `review_worktree` was passed. The new sync is authoritative; the
    legacy sync just made a second stale copy.
  * Pipe `review_worktree` through `build_peer_review_prompt` so the
    prompt's worktree path matches the file-scope block. One location.
  * Replace the meek "verify commit" hint with a hard "CURRENT HEAD"
    callout that names the SHA, says "your prior reasoning may be out
    of date", and lists 4 explicit steps: Re-Read fresh, verify each
    prior finding still applies, add new ones, drop the rest.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* revert(pr-review): use the original pr-<n>/ worktree, drop pr-review-<n>/

PR #172 invented `pr-review-<n>/` as a parallel worktree to hold the
file-routing flow's check-out. The original `pr-<n>/` (managed by
`_prefetch_worktrees` for the cube panel + `_get_cli_review_worktrees`
for CodeRabbit) kept existing alongside it. Result: two worktrees per
PR, only one kept synced by the new code, prompts pointing at the
wrong one, judges Reading stale content.

Reverting the path so the file-routing flow reuses `pr-<n>/`. Combined
with the #176 skip-`_prefetch_worktrees`-when-already-synced logic,
this gives one worktree per PR with the new --force fetch + HEAD
verification applied to it. CodeRabbit and cube judges both read
from the same place.

`pr-review-<n>/` orphan dirs deleted from the local cache; `git
worktree prune` cleaned the stale refs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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