Skip to content

fix(cli): --pr fetches into a unique ref and restores the checkout (#665) - #674

Open
gadievron wants to merge 2 commits into
masterfrom
fix/665-pr-head-unique-restore
Open

gadievron wants to merge 2 commits into
masterfrom
fix/665-pr-head-unique-restore

Conversation

@gadievron

@gadievron gadievron commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

What

FetchPR now fetches the PR head into a unique non-branch ref (refs/openant/pr/<N>/<pid>) and checks out DETACHED. Closes #665.

Why the previous attempt was wrong

The first version used a defer-based restore inside FetchPR. The defer fired before the caller (and the Python engine) read the tree — every --pr scan read the user's original checkout instead of the PR, and the "restore" left HEAD detached. Caught by the adversarial review + a real T1 dispatch; rewritten.

The correct fix

  • Unique non-branch ref (refs/openant/pr/<N>/<pid>): a different ref per invocation (the PID kills the shared-name race); a non-branch ref never blocks a force-fetch (the second---pr refusal is gone).
  • Detached checkout: never blocks a re-fetch; never leaves a branch behind; the tree persists until the caller moves it — which is the correct contract.
  • No defer: the checkout persists until the caller moves the tree, so scan --pr N scans the PR tree, and scan (no flags) naturally reads whatever tree is there.
  • The user-facing message and doc comments updated (the T1 round-1 finding).

Testing

The T1 round-1 executed repro (fake gh + local refs/pull/7/head):

  • Master run 2: fatal: refusing to fetch into branch 'refs/heads/pr-head' (the bug)
  • HEAD runs 1-3 (including PID-reuse with --force): all ok; ChangedFiles=[f.txt g.py] on the PR head (not the user's tree); tree content = pr

go build, go vet, gofmt all clean.

Known limitations

  • The unique refs are never deleted (harmless: not a branch, doesn't block anything; acknowledged as an accepted leak)
  • Concurrent different-PR runs on one working tree still clobber (pre-existing, not worsened by this fix)

Fixes #665

Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d

@gadievron

Copy link
Copy Markdown
Collaborator Author

Status: ON HOLD — do not merge.

The adversarial review (2026-09-22) found defects in this PR that need rewriting before it can merge. The specific finding is in the review record. The PR is being rewritten; the current branch and all evidence are preserved.

Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d

Gadi Evron added 2 commits September 22, 2026 13:55
…heckout (#665)

Rewrite of the previous attempt (which used a defer-based restore inside
FetchPR — a defect the adversarial review caught: the defer fired BEFORE
the caller and the Python engine read the tree, making every --pr scan
read the user's original checkout instead of the PR, and leaving HEAD
detached).

The correct fix: the PR head fetches into a unique non-branch ref
(refs/openant/pr/<N>/<pid>) and checks out DETACHED. A detached
checkout never blocks a re-fetch (the second --pr refusal is gone) and
never leaves a branch behind (the shared-name race is gone). NO defer —
the checkout persists until the caller moves the tree, which is the
correct contract: 'openant scan <repo> --pr N' scans the PR tree, and
'openant scan <repo>' (no flags) naturally reads whatever tree is there.

Fixes #665

Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
…rt order (#665)

F1: diff_shared.go's user-facing message and diff.go's doc comment still
said 'pr-head' — now they say what actually happens (unique ref, detached
checkout). F2: the os import order per gofmt.

Agent: ISSUES-TO-PR ses_f3e7372a5ffe3b2I6lP4Wm3w2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant