From c426679aad9221492decce260fd977199ff80e6c Mon Sep 17 00:00:00 2001 From: justinhelmer <1403438+justinhelmer@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:58:18 +0000 Subject: [PATCH] fix(review): recover workspaces at pull request heads Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com> --- docs/reference/specs/agent-review.md | 6 +- docs/reference/specs/tracing.md | 2 +- src/core/dispatch/authorize.test.ts | 106 +++++++++++++--- src/core/dispatch/authorize.ts | 117 +++++++++++++---- src/core/dispatch/provision.ts | 6 +- src/core/dispatch/reply.test.ts | 4 +- src/core/dispatch/reply.ts | 2 +- src/core/dispatcher.test.ts | 183 ++++++++++++++++++++++----- src/core/refusal.ts | 2 +- src/core/reviewRound.test.ts | 65 +++++++++- src/core/reviewRound.ts | 100 +++++++++++---- src/execution/factory.ts | 7 +- 12 files changed, 484 insertions(+), 116 deletions(-) diff --git a/docs/reference/specs/agent-review.md b/docs/reference/specs/agent-review.md index b666075a5..bda6bdeed 100644 --- a/docs/reference/specs/agent-review.md +++ b/docs/reference/specs/agent-review.md @@ -27,12 +27,12 @@ Reviews a PR with the full change in context and reports ranked, evidence-anchor 7. **Resident-path variant** ([resident-repos.md](resident-repos.md) §31): in a resident repo environment the dispatcher swaps in `REVIEW_SYSTEM_RESIDENT` via `RunOptions.system` — same gather-once discipline, but against the ready worktree (already on the branch under review, deps installed; no cloning) using git directly, since `gh` is not in the resident image. 8. **Reviewed-head guard — a review is posted to a PR only if it is a review OF that PR's head** ([`reviewedHead.ts`](../../../src/core/reviewedHead.ts)): immediately before the write, the post-step re-reads the pull request's open head; a merged, closed or unreadable pull request has no postable head, so the verdict is recorded as Slack-only and no review lands after terminal state. With the resident's per-thread bindings, pool users and exec routing all holding, an agent can still fetch and check out *another* PR's branch because the PR under review references it, review that tree, call `submit_verdict approve`, and have the deterministic post-step put `LGTM:` on the wrong PR — which the org's auto-approve workflow then approves on wrong evidence. Every other layer does its job; this one asks "is this review of this PR?". (a) after the model finishes and **before** the workspace is released (post-release a resident re-attach would show the ref's *current* tip, not what was reviewed), the dispatcher runs `git rev-parse HEAD` in the run's workspace itself — the **observed** head, authoritative; (b) `submit_verdict` requires `head` (the agent's own `git rev-parse HEAD`) — the **reported** head, consulted only when nothing was observed (the cold sandbox's cwd is the workspace root, not the clone); (c) `checkReviewedHead({expected: RepoContext.headSha, observed, reported})` must pass or the post is skipped with a thread note that names the source (`workspace-observed` or `model-reported`), prints both usable SHA values in full and identifies their first differing hex position — never two identical seven-character prefixes — before saying the verdict is Slack-only. Fail-closed on every unknown: no `headSha` at resolution ("PR head unknown"), no observed and no reported head ("reviewed head unknown"), and a matching *reported* head never rescues a mismatching *observed* one. A posted review is therefore always pinned (`commit_id` = the verified head) — the unpinned post path is gone. Both review prompts now say: review the PR's own head, never fetch/check out another branch or PR even when referenced; if the change depends on unmerged work, say so as a finding. Upstream of this guard, the resident attaches at the resolved head itself ([resident-repos.md](resident-repos.md) item 51: the bot passes `RepoContext.headSha` to `/attach` and the resident fetches a mirror whose ref tip lags it), so a mirror that has not yet fetched a push is not what the guard catches — the guard stays the backstop for an agent that strays, not the outcome of an ordinary re-review. 9. **The agent is told its target — deterministically, from resolved facts** ([`reviewTarget.ts`](../../../src/core/reviewTarget.ts); the upstream complement of item 8, which is the backstop): the dispatcher already resolves `repo`, `pr`, head branch (`ref`), head commit (`headSha`) and now the base branch (`RepoContext.baseRef`, from the same `GET /pulls/{n}` call, validated as a ref) before the model turn — and until this item used them only *after* the run (to post and to guard) while the model got a Slack sentence with a URL and a prompt saying the worktree was "typically" the branch under review. Now a `review` run with a resolved PR gets a `REVIEW TARGET` block appended to its system prompt on both paths: repository, PR number + URL, head branch, head commit, base branch (an unresolved head branch/commit is named as unknown, never invented; an unknown base renders as "the repository's default branch" and the diff target falls back to `origin/HEAD`). Resident path: "the worktree is already at that head; FIRST command `git rev-parse HEAD`, which must equal the head commit — if not, STOP, report the two heads as an infrastructure failure, submit no finding or verdict, fetch/check out nothing"; `origin/` is already in the clone, so the diff is against it and `git fetch` is not run (the resident read-only attach removes the ability anyway). Cold sandbox path: Switchboard provisions the checkout at the resolved PR head before the model starts, then the same HEAD check; the agent neither clones nor checks out a ref. Both: pass that commit as `head` to `submit_verdict`. The resident review prompt no longer hedges ("typically the branch under review" is gone — the block states the branch) and no longer instructs a fetch. Coding runs and PR-less reviews get no block — their prompts are byte-identical to before. Because the block and the item 8 guard read the same `RepoContext`, they can disagree only when the agent strays — and then the guard refuses the post. -10. **The workspace is named and checked on every backend before the run — the agent has nothing to go looking for**: item 9's "FIRST command `git rev-parse HEAD`" assumed the model would run it where its shell starts. Left to find the worktree itself, an agent whose shell starts elsewhere `cd`s around until it lands in the resident's own warm default-branch checkout, runs `git rev-parse HEAD` there, reports *that* HEAD as a mismatch and submits `request_changes` — with the worktree attached at the PR head all along. Two fixes, both from facts the bot already holds. (a) **The attach answer's `workspace` rides into the prompt**: `ResidentBinding.workspace` (from `POST /attach`, [resident-repos.md](resident-repos.md) item 16) → `ExecutorSelection.binding` → the resident preamble ("Your shell starts in the worktree `` on every bash call …") and the REVIEW TARGET block, which now pins the first command "from the current directory, no `cd`" and states that any other checkout on the host — the resident's default-branch checkout included — is NOT the PR; an attach answer without the path names no path (nothing invented) and keeps the no-`cd` rule. (b) **The dispatcher proves the workspace HEAD against the PR head itself on every backend**, before any model turn: the resident binding is the resident source; a seeded sandbox uses its named checkout; an unseeded cold backend first initializes a clean workspace, fetches `origin/HEAD`, the resolved base and `refs/pull//head` with the run's read identity, checks out the resolved PR-head SHA, and only then runs `git rev-parse HEAD` there. Equal (≥7-hex prefix match, `sameCommit`) → the block says Switchboard verified it before the run; attached = the PR's current head after one re-read → adopt that raced push; different or unreadable → the run is **not started**: card `🔀 … not started (branch moved)`, ONE reply naming the source and full heads (or the unreadable source), the workspace released, no provider call. This setup outcome is infrastructure, never a finding or verdict, and the post-step is unreachable, so nothing is posted to GitHub. Review-only: a coding run attached elsewhere still runs (it branches off whatever it has). The resident needs no change — its binding, exec cwd (`cd && …`) and attach answer are already correct in that failure. +10. **The workspace is named and checked on every backend before the run — the agent has nothing to go looking for**: item 9's "FIRST command `git rev-parse HEAD`" assumed the model would run it where its shell starts. Left to find the worktree itself, an agent whose shell starts elsewhere `cd`s around until it lands in the resident's own warm default-branch checkout, runs `git rev-parse HEAD` there, reports *that* HEAD as a mismatch and submits `request_changes` — with the worktree attached at the PR head all along. Two fixes, both from facts the bot already holds. (a) **The attach answer's `workspace` rides into the prompt**: `ResidentBinding.workspace` (from `POST /attach`, [resident-repos.md](resident-repos.md) item 16) → `ExecutorSelection.binding` → the resident preamble ("Your shell starts in the worktree `` on every bash call …") and the REVIEW TARGET block, which now pins the first command "from the current directory, no `cd`" and states that any other checkout on the host — the resident's default-branch checkout included — is NOT the PR; an attach answer without the path names no path (nothing invented) and keeps the no-`cd` rule. (b) **The dispatcher attaches and proves the pull request's checkout, never the unit's or thread's branch**: whenever a unit or direct run reviews a pull request, the attach binds to that pull request's head branch and expected commit, even when the unit's own contract branch or an older sticky resident binding names another ref. The guard runs `git rev-parse HEAD` through the attached executor and compares that observed checkout HEAD with the head commit being reviewed; neither `ResidentBinding.sha` nor `Executor.moveTo().sha` is proof of what is checked out. A seeded sandbox uses its named checkout; an unseeded cold backend first initializes a clean workspace, fetches `origin/HEAD`, the resolved base and `refs/pull//head` with the run's read identity, checks out the resolved PR-head SHA, and only then runs `git rev-parse HEAD` there. Equal (≥7-hex prefix match, `sameCommit`) → the block says Switchboard verified it before the run. When they differ, one current-head read may adopt a raced push only when the workspace is already at that current head; otherwise the dispatcher reprovisions the workspace at the expected reviewed head and checks the resulting HEAD once. Only a failed reprovision or second mismatch refuses: card `🔀 … not started (workspace head mismatch)`, ONE reply quoting the full workspace and expected SHAs (or the unreadable source), without asserting why they differ or calling the condition a bug, the workspace released, no provider call. This setup outcome is infrastructure, never a finding or verdict, and the post-step is unreachable, so nothing is posted to GitHub. Review-only: a coding run attached elsewhere still runs (it branches off whatever it has). 10. **Head moved mid-run — the thread is told** ([`headMoved.ts`](../../../src/core/headMoved.ts)): a review is posted pinned to the head it examined (item 8), and the org's auto-approve workflow skips a review whose `commit_id` is not the PR head — so a push that lands *while the run is in flight* can never get unreviewed code approved. That protection is silent: the thread sees "review posted" while GitHub shows the review against an outdated commit and nothing auto-approves. After a successful post the dispatcher fetches the PR head once more (`CoreDeps.fetchPrHead`, default `currentPrHeadSha` — one REST GET, best-effort) and, when it is not the reviewed commit, replies `ℹ️ # moved during the run: reviewed , head is now . The review was posted pinned to and will not auto-approve — re-request to review .` The post itself is unchanged (still pinned to the reviewed head — the review IS of that commit). Unknown current head (fetch failed, malformed) → no note, never a false alarm; nothing posted (guard refused, opt-out, hard stop) → no fetch. Distinct from item 8: the guard is about what the agent *reviewed*; this is about what happened to the PR *afterwards*. 11. **Unknown head — the run is not started**: a re-review reading "re-review: rebuilt **on main** after the base landed…" binds `ref: "main"` from the prose; a resolver that *skips* the PR head fetch whenever a ref is bound then leaves `RepoContext.headSha` undefined, the attach carries no `sha` for the resident to refresh to, the worktree stays at the previous head, the agent spends its turns establishing that the new commit is not in its checkout and writes a `request_changes` "cannot review" verdict, and item 8 refuses the post ("PR head unknown at resolution time"). Two fixes. (a) **A PR named in the message is always resolved** ([resident-repos.md](resident-repos.md) §29): the head fetch runs whenever the current message names a PR of the resolved repo, whatever ref phrasing sits beside it; the PR's head branch *is* the ref for that message (prose "on X" — even `on branch X` — cannot redirect a PR review and binds nothing when the fetch fails). **The head sha is the head branch's ref tip, not only the PR object's `head.sha`**: after a force-push GitHub's pull-request object (`head.sha`, `commits`) lags the branch ref — observed live for minutes while the new commit was already fetchable by sha — and a review attached at the lagging `head.sha` reviews a head nobody asked about and refuses with a mismatch. So every PR-head read (`prHead` behind `resolveRepoContext` and `currentPrHeadSha`; `fetchPullRequestFacts` for ship) also reads `GET /repos/{repo}/git/ref/heads/{branch}` when the head lives on the base repo and prefers the ref's tip when the two disagree, logging both shas (`[pr-head] #: GitHub's PR object reports head X while refs/heads/ is at Y — using the ref`); an unreadable ref, or one that does not point at a commit object, keeps the PR object's sha, and a cross-fork head (no ref on the base repo) never asks. (b) **A review with an unknown head is refused before any attach or model turn**: `review` agent + a PR in the message with no `headSha`, or an inherited PR whose head was `unreachable` → `🔀 Review of # not started: I could not resolve the PR's current head from GitHub … Re-send the request in a moment…`, card `· not started (PR head unknown)`, `[review] … not started: PR head unknown` log. Everything downstream of an unknown head is a guaranteed refusal, so nothing is spent on it. The gate and the post-step decide "is this verdict meant to be posted" by ONE predicate, `reviewPostIntended` in [`reviewPost.ts`](../../../src/core/reviewPost.ts) (review agent + no opt-out) — a review-like agent or a new opt-out phrasing changes both at once. An explicit "slack only" opt-out (item 6) is exempt — an unpinned Slack-only verdict is what was asked for; a *closed* inherited PR still runs Slack-only with the `ℹ️ Review not posted … the PR is closed` note (item 8) — its head is known, only the post target is gone. -12. **Head moved during the run — carried across a rebase, re-reviewed otherwise; the current head is consulted before either guard refuses** ([`headMoved.ts`](../../../src/core/headMoved.ts)): when the head moves mid-run by a rebase onto main, item 10 alone would post the review pinned to the old head with the re-request note, and the re-request would spend a second full run reaching the same verdict — items 8 and 10 are tuned for the unsafe case (a stray review, unreviewed code) and would treat every move as one. So, after the model finishes and before anything is posted, the dispatcher fetches the PR head once and compares it with the head the agent reviewed (observed, else reported — the guard's own authority order). (a) **Reviewed = current ≠ resolved** (a mid-run re-attach after an eviction landed on the ref's newer tip): the review IS of the PR's head — adopted, posted pinned to it, no refusal. (b) **Reviewed = resolved ≠ current** — the PR moved under the review: the move is classified from GitHub's compare lists (`GET /repos/{repo}/compare/{base}...{sha}` for the reviewed and the current head, `CoreDeps.fetchPrCommits`, default `prCommitsSince`; parallel, best-effort). **Rebase**: same number of commits, same messages in order, same set of touched files (300-file cap on either side → messages decide) — a rebase onto main, conflict resolutions included — the review applies unchanged: posted pinned to the **new** head with the footer `_Reviewed at ; the head moved to during the review — a rebase of the same N commits — so this review is posted against ._` and the thread note `ℹ️ #: review carried to — a rebase of the same N commits (reviewed ).` — an acknowledgement, sent at `verbose` and above ([routing-and-config.md](routing-and-config.md) item 28) — (an `LGTM:` auto-approves as usual). **Substantive** (new/dropped/reordered/reworded commits, or same messages reaching new files — the cheap guard against a message-preserving `--amend`): the **same run re-reviews** at the new head — no new run, no re-request. In order: a `run_note` of kind `head_moved` on the run stream, the [card](../vocabulary.md#card) label gains `· head moved → `, the thread gets `🔀 # moved during the run: reviewed , head is now — B → A commits (+ “subject”, − “subject”, …). Re-reviewing at before posting.`; the worktree is moved to the new head where the executor can (`Executor.moveTo`, resident: one more `/attach` carrying the new sha — the item 51 fetch-on-attach recreates the tree at the ref's tip); the earlier verdict is voided; the conversation continues with the first review as an assistant turn and a [follow-up](../vocabulary.md#follow-up) user turn (`rereviewFollowUp`: both heads, both commit lists by subject, where the worktree stands — or, with no `moveTo` / a refused move, `git fetch origin && git checkout ` — and a fresh `submit_verdict` with `head` = ``); the run's `answer` event and the Slack reply are the re-review's. That one more turn is one more `prompt` on the run's own pi session through the seam the run stage hands over (`followUp`; [harness-pi.md](harness-pi.md) item 14) — the turn's own verdict capture, the settle around it, never a second process; pi reads its system prompt once, at load, so the REVIEW TARGET block is not recomposed and the follow-up alone names the new head. A run resumed as a `finish` plan ([run-history.md](run-history.md) item 37) has no session to prompt: a substantive move it finds is not re-reviewed — a `head_moved` note says the session is gone and the verdict stands for the head it reviewed — and item 10 pins the post there. The post is then pinned to the new head after the ordinary item 8 guard. **Unclassifiable** (no base branch, compare failed for either side) → item 10 exactly as before. A head that moves **again** after the re-review gets item 10's note, never a third turn. Hard stop → none of this. (c) **Reviewed ≠ both** → the agent strayed: item 8 refuses as before, having consulted the current head first. (d) **Before the run** (item 10's attach check): attached ≠ resolved now also asks GitHub once — the resident's attach fetched the mirror to the ref's tip, so a push that raced the request leaves the worktree at the PR's head NOW; attached = current → the run reviews the attached head (RepoContext adopts it, the block says the attach was verified) instead of refusing and asking for a re-send; anything else (unknown current, or a second move) → `not started (branch moved)` as before. Bounds: the re-review turn runs under the agent's budgets again (worst case one more review's worth of turns/time); the follow-up carries the first review's text, not its tool transcript (the model re-reads what it needs); the superseded first review is not in the run record (the note is). +12. **Head moved during the run — carried across a rebase, re-reviewed otherwise; the current head is consulted before either guard refuses** ([`headMoved.ts`](../../../src/core/headMoved.ts)): when the head moves mid-run by a rebase onto main, item 10 alone would post the review pinned to the old head with the re-request note, and the re-request would spend a second full run reaching the same verdict — items 8 and 10 are tuned for the unsafe case (a stray review, unreviewed code) and would treat every move as one. So, after the model finishes and before anything is posted, the dispatcher fetches the PR head once and compares it with the head the agent reviewed (observed, else reported — the guard's own authority order). (a) **Reviewed = current ≠ resolved** (a mid-run re-attach after an eviction landed on the ref's newer tip): the review IS of the PR's head — adopted, posted pinned to it, no refusal. (b) **Reviewed = resolved ≠ current** — the PR moved under the review: the move is classified from GitHub's compare lists (`GET /repos/{repo}/compare/{base}...{sha}` for the reviewed and the current head, `CoreDeps.fetchPrCommits`, default `prCommitsSince`; parallel, best-effort). **Rebase**: same number of commits, same messages in order, same set of touched files (300-file cap on either side → messages decide) — a rebase onto main, conflict resolutions included — the review applies unchanged: posted pinned to the **new** head with the footer `_Reviewed at ; the head moved to during the review — a rebase of the same N commits — so this review is posted against ._` and the thread note `ℹ️ #: review carried to — a rebase of the same N commits (reviewed ).` — an acknowledgement, sent at `verbose` and above ([routing-and-config.md](routing-and-config.md) item 28) — (an `LGTM:` auto-approves as usual). **Substantive** (new/dropped/reordered/reworded commits, or same messages reaching new files — the cheap guard against a message-preserving `--amend`): the **same run re-reviews** at the new head — no new run, no re-request. In order: a `run_note` of kind `head_moved` on the run stream, the [card](../vocabulary.md#card) label gains `· head moved → `, the thread gets `🔀 # moved during the run: reviewed , head is now — B → A commits (+ “subject”, − “subject”, …). Re-reviewing at before posting.`; the worktree is moved to the new head where the executor can (`Executor.moveTo`, resident: one more `/attach` carrying the new sha — the item 51 fetch-on-attach recreates the tree at the ref's tip); the earlier verdict is voided; the conversation continues with the first review as an assistant turn and a [follow-up](../vocabulary.md#follow-up) user turn (`rereviewFollowUp`: both heads, both commit lists by subject, where the worktree stands — or, with no `moveTo` / a refused move, `git fetch origin && git checkout ` — and a fresh `submit_verdict` with `head` = ``); the run's `answer` event and the Slack reply are the re-review's. That one more turn is one more `prompt` on the run's own pi session through the seam the run stage hands over (`followUp`; [harness-pi.md](harness-pi.md) item 14) — the turn's own verdict capture, the settle around it, never a second process; pi reads its system prompt once, at load, so the REVIEW TARGET block is not recomposed and the follow-up alone names the new head. A run resumed as a `finish` plan ([run-history.md](run-history.md) item 37) has no session to prompt: a substantive move it finds is not re-reviewed — a `head_moved` note says the session is gone and the verdict stands for the head it reviewed — and item 10 pins the post there. The post is then pinned to the new head after the ordinary item 8 guard. **Unclassifiable** (no base branch, compare failed for either side) → item 10 exactly as before. A head that moves **again** after the re-review gets item 10's note, never a third turn. Hard stop → none of this. (c) **Reviewed ≠ both** → the agent strayed: item 8 refuses as before, having consulted the current head first. (d) **Before the run** (item 10's attach check): attached ≠ resolved now also asks GitHub once — the resident's attach fetched the mirror to the ref's tip, so a push that raced the request leaves the worktree at the PR's head NOW; attached = current → the run reviews the attached head (RepoContext adopts it, the block says the attach was verified) instead of refusing and asking for a re-send. Otherwise the dispatcher reprovisions once at the resolved reviewed head and rechecks the checkout: matching → the review runs; a thrown reprovision, unreadable retry HEAD or second mismatch → `not started (workspace head mismatch)` (`workspace_head_mismatch`), with initial and retry observations kept distinct, the workspace released and no provider call. Bounds: the re-review turn runs under the agent's budgets again (worst case one more review's worth of turns/time); the follow-up carries the first review's text, not its tool transcript (the model re-reads what it needs); the superseded first review is not in the run record (the note is). 13. **The verdict reply carries its run link — projection only**: a review run's channel reply (the verdict message, rendered from the verdict per item 5b) ends with a `[Live run]()` line — joined by ` · ` to the `Posted to #` note when the post landed — appended at the reply seam when `PUBLIC_BASE_URL` is set — the verdict is what gets scanned in the review loop, and the status card that already carries the link scrolls away. Appended at the reply seam only, in standard Markdown (each adapter renders its own dialect — Slack a `` hyperlink): the run record's `answer` event and the GitHub post body stay link-free (the run record is the source of truth; channels project from it). No `PUBLIC_BASE_URL` → the bare answer, as before; non-review agents get no link on a successful answer (it is not a verdict, and their card carries it) — but ANY run's failure reply (the outer catch's `⚠️ ` line) ends with the same `[Live run]()` line when a run started and the URL exists, so a failed run's transcript is one click from the thread instead of a card that scrolled away. @@ -71,7 +71,7 @@ Reviews a PR with the full change in context and reports ranked, evidence-anchor | 5: end to end — a review whose loop ended without `submit_verdict` gets one verdict turn; the verdict the turn submits leads the posted body (`LGTM:`) over the loop's own write-up, the turn's reply is dropped, the note and the tool call are on the record; a run that still submits none posts the no-verdict line; a Slack-only opt-out is not nudged; a stop before any substantive review tool gets no verdict turn even when it lands during reviewed-head settlement, while a stop after checkout and a real verdict keeps findings-so-far | `[unit]` `src/core/dispatcher.test.ts::review post-step::a review whose loop ended without submit_verdict gets ONE verdict turn; the verdict it submits leads the posted body, the write-up stays the review's text`, `src/core/dispatcher.test.ts::review post-step::a review of a resolved PR posts the review back to the PR by default`, `src/core/dispatcher.test.ts::review post-step::an unknown head with an explicit 'slack only' opt-out still runs — the user asked for an unpinned, Slack-only verdict`, `src/core/dispatcher.test.ts::review post-step::a soft stop before review activity replies only that it stopped and runs no review tail`, `src/core/dispatcher.test.ts::review post-step::a soft stop during a substantive head fetch aborts settlement before notifications, movement, follow-up, or publication`, `src/core/dispatcher.test.ts::review post-step::a soft stop after checkout and real findings keeps the findings-so-far review behavior` | | REVIEW TARGET block (item 9): present on a resident review with a resolved PR (resident variant, carries the RepoContext head sha), present on a cold sandbox review whose checkout Switchboard already provisioned, absent for coding runs and PR-less reviews; text is pure/deterministic, unresolved head branch/commit named as unknown (base falls back to the default branch / `origin/HEAD`), both variants forbid clone/fetch/checkout movement | `[unit]` `src/core/reviewTarget.test.ts` (5); `src/core/dispatcher.test.ts::repo/ref resolution + resident prompt selection …::a resident review of a resolved PR gets the REVIEW TARGET block…`, `::a sandbox-path review of a resolved PR gets the sandbox variant…`, `::no REVIEW TARGET block for a coding run on a PR, nor for a review with no resolved PR`. `[agent]` post-deploy: the first resident review's Slack card shows the agent's first command as `git rev-parse HEAD` and its verdict `head` equals the PR head. | | Worktree named, attach checked (item 10): REVIEW TARGET names the worktree path, forbids `cd`/filesystem search, pins the first command to the current directory; no path invented when the attach lacks one; `verifiedAtAttach` sentence only on the resident path with matching shas | `[unit]` `src/core/reviewTarget.test.ts::resident path with the worktree path…`, `::resident path without a worktree path…`, `::verifiedAtAttach…`; `src/execution/resident.test.ts::records the attach result's ref@sha as the thread binding…` (+ `workspace`), `::a 200 attach answer without a string \`workspace\` still binds…`; `src/execution/factory.test.ts::warm probe → ResidentExecutor…` (`binding` on the selection, unset on the per-thread path). | -| Workspace-head mismatch never reaches the model on any backend (item 10, 12d): a cold unseeded review provisions the resolved PR checkout before observation; resident binding or workspace-observed HEAD = PR head → verified; a raced current head is adopted; mismatch or unreadable → `not started`, source + full divergence in one reply, workspace released, zero provider calls, no finding, verdict or GitHub post; coding and resumed runs are unaffected | `[unit]` `src/core/dispatch/authorize.test.ts::authorizeAttachedHead — every review workspace is at the PR head before the first model turn::*`; `src/core/reviewRound.test.ts::guardAttachedHead (before any model call)::*`; prompt backstop: `src/core/reviewTarget.test.ts::reviewTargetBlock::*` | +| Workspace-head mismatch never reaches the model on any backend (item 10, 12d): every PR review attaches on the PR head branch even when a unit or sticky binding names another ref; the checked-out workspace HEAD is read through the executor before and after recovery and compared only with the reviewed head (binding and move metadata are never proof); a raced current head is adopted, otherwise one reprovision at the expected head is rechecked; only a failed reprovision or second mismatch is `not started`, quoting both full SHAs without an unproved cause, releasing the workspace before any provider call; coding and resumed runs are unaffected | `[unit]` `src/core/dispatcher.test.ts::repo/ref resolution + resident prompt selection …::a genuine push during workspace preparation is reprovisioned at the reviewed head and the review runs`, `::a unit reviewing an adopted pull request binds its attach to the pull request branch instead of its own`, `::a failed reprovision refuses with the two heads and no cause the guard did not establish`; supporting seams: `src/core/dispatch/authorize.test.ts::authorizeAttachedHead — every review workspace is at the PR head before the first model turn::a resident binding SHA cannot verify a checkout whose observed HEAD differs`, `::resident moveTo metadata cannot verify a retry whose observed HEAD is still mismatched`, `::a successful retry publishes the observed checkout HEAD instead of moveTo metadata`, `src/core/reviewRound.test.ts::guardAttachedHead (before any model call)::*` | | `RepoContext.baseRef` from `base.ref` (explicit and inherited PR), validated as a ref, unset when malformed/missing | `[unit]` `src/core/repoContext.test.ts::PR base branch for the review target` (3) | | Resident review prompt: no "typically the branch under review" hedge, no `git fetch` instruction, diffs against `origin/` | `[unit]` `src/agents/registry.test.ts::review resident: no 'typically the branch under review' hedge, no git fetch instruction` | | Closed pull request refused before admission (item 11): merged → one line with the pull request, merge timestamp and `nothing to review`; closed-unmerged → parallel closed line without a merge time; neither claims admission, opens a card/workspace, registers a run, calls a provider or reaches the review post; the resolver carries the normalized merge timestamp | `[unit]` `src/core/dispatcher.test.ts::review post-step::a merged PR ends before admission with one nothing-to-review line and no run machinery`, `src/core/dispatcher.test.ts::review post-step::a closed-unmerged PR ends before admission with one closed line and no merge time or run machinery`, `src/core/repoContext.test.ts::resolveRepoContext: PR-source flags for ship::a merged PR URL in a plain ask yields no ref hint — the PR's facts (pr, headSha) still ride as context (issue 1860)`, `src/core/repoContext.test.ts::resolveRepoContext: PR-source flags for ship::a closed-unmerged PR contributes no ref hint either…` | diff --git a/docs/reference/specs/tracing.md b/docs/reference/specs/tracing.md index af545739d..db53ca8f7 100644 --- a/docs/reference/specs/tracing.md +++ b/docs/reference/specs/tracing.md @@ -25,7 +25,7 @@ One measurement primitive, a span, records every unit of work Switchboard does 15. **Display names.** `DISPLAY_NAMES` (`src/core/trace/displayNames.ts`) names every enumerated streamed span (`satisfies Record`; unique by test), and `displayNameOf(name)` covers the prefix families — a tool's own name, an MCP tool's own name, the resident step's label (`RESIDENT_STEP_LABELS` in `src/execution/residentSteps.ts`, the one table both sides share: the Worker's runners take a `ResidentStepName`, so a step the table lacks is a type error in the Worker, and a source-scan test keeps the table free of labels for steps the Worker no longer names) — with `a Switchboard step` for an unknown leaf. No user surface prints a raw span name: the run page's span rows and, later, the card's setup label and the timeline's ranked list all read this table. 16. **The protected head and the content count.** The registry's backlog ([live-view.md](live-view.md) item 2) keeps the events that say what a run is — `input`, `context`, `run_meta`, the root's start, the `slack.receive` and `dispatch.*` span pairs, the `mcp_unavailable`/`spans_dropped`/`cold_sandbox`/`ledger_untracked` notes — as a protected head of at most `HEAD_BUDGET_BYTES` (512 KiB), capped per event to `MAX_EVENT_BYTES` at publish, never trimmed by the count (8000) or byte bound; a fresh subscribe replays the head first. The record budget ([run-history.md](run-history.md) item 6) drops span pairs first, from the middle outward, never from the head, so spans displace no content. `stepCount` — content events, span records excluded — rides the summary, the snapshot, the record and the index row's count cell beside `eventCount`. 17. **The emitters — the harness, the model proxy, the MCP bridge, the executor, the review settle, the dispatcher.** `runPiHarnessOpen` opens `run.agent` under the run's root ([harness-pi.md](harness-pi.md) item 5); under it the model proxy opens one `model.turn` per call pi makes ([model-proxy.md](model-proxy.md) item 6; [live-view.md](live-view.md) item 15) and the bridge one `tool.` span per tool call: the call is announced inside it, so its `tool_call`/`tool_result` — and whatever a relayed tool publishes, a `skill_use` — carry its `spanId`; a relayed call's `ToolContext` carries the span and a `TracingExecutor` ([execution.md](execution.md) item 15), and the span ends with `callId`, `ok`, `exitCode?` (`error` on a failed tool, on a stop, on a gate bypass). The MCP bridge runs each remote call as `mcp..` under that span ([mcp-tools.md](mcp-tools.md) item 10). `settleReviewedHead` is one uncounted `run.settle_reviewed_head` span whose re-review `run.agent` is its child. A ship [round](../vocabulary.md#round) is a child run of its own with its own root ([agent-ship.md](agent-ship.md) item 12): nothing opens `ship.round` any more — the name stays in the stream table for the records the retired in-process pipeline wrote. The dispatcher binds each run to the request's root (item 18): the root's `span_start` and the setup spans so far backfill as the stream's first events (head material), the loop's spans follow live, and the root ends after the seal, so its end never reaches a sealing registry and every record shows the request as its one open span. Without a parent span (tests) the harness emits no span record and its stream is byte-identical. Every record carries `schema: 2`; the `💭 thought for …` card line is the bridge's `onProgress` note at each assistant message's end; there is no `turn` or `mcp_tool_use` event kind — the `model.turn` and `mcp.*` spans are the record ([live-view.md](live-view.md) item 13). The `github.token_mint`/`github.rest` spans wait for `tracedFetch` (the cross-Worker PR); `GithubApiError` and the resident's `ExecInfraError`s are classified now (item 2; [execution.md](execution.md) item 9), so the spans above them carry kind and code. The model proxy is the `model.turn` emitter: each call a run's bearer buys through it is one span under the span the bearer carries — the request root at the mint, the harness's `run.agent` once it re-parents the grant — with the attrs the native runner once set (`model`, `stopReason`, the four token counts, `ttftMs`), so a turn taken in pi's process reads on the record as the loop's did; a relayed tool's own work (its `exec.*`, its `github.*`) hangs under the bridge's tool span through the same tracing executor and client view; the bridge emits no `model.turn` — the proxy's spans are the run's turns. -18. **One request, one root — the dispatcher's spans.** `startRequestRoot(deps, { channel, receivedAt })` (`src/core/requestTrace.ts`) is the one constructor of roots: each channel adapter calls it when our process sees the message — Slack before its redelivery guard, HTTP and MCP once the caller's identity is established and the body parsed, the CLI at entry — stamps the message `receivedAt` (Slack also `originAt`, the platform's `ts`) and hands the `RequestTrace` to `dispatch()` through `DispatchOptions.trace` (the `DispatchFn` seam carries it); a dispatch without one (tests, an older caller) starts its own at entry. The root's sinks are the log sink at `tracing.log` (or `CoreDeps.sinks`, a test's recording sink), the run-stream sink, the card sink and a collector for the runless closes; `CoreDeps.clock` and `CoreDeps.tracer` are the injection points the no-gaps test uses. Every awaited step of `dispatch()` is a `span(fn)` child of the root at the site the work starts: `dispatch.history`, `dispatch.repo_context` and `dispatch.memory_read` (wrapping the promises as they are created, so their overlap is real), `dispatch.ack_card`, `dispatch.admission` (the steer ack, a superseded resume), `dispatch.refuse` (every refusal — the close and the reply as one span, `outcome` naming why: `agent_allowlist`, `repo_not_onboarded`, `repo_unverified`, `repo_access`, `pr_head_unknown`, `which_branch`, `branch_moved`, `ship_preflight`, `setup_failed`…), `dispatch.workspace.attach` (`backend`), `dispatch.gate.attached_head` (`outcome`), `dispatch.mcp_discovery`, `dispatch.compose` (the wait on the memory read; the composition is synchronous), `dispatch.channel_visibility`, `dispatch.ledger_claim`, `dispatch.ship_preflight`; `run.command` (a command run's body, `command` naming it; a runless command reply too, log-only there); the background span `run.reading_diff` (`outcome`; started by `startReviewReadingDiff` under the root, so the diff's exec — and its `http.client` hop to a sandbox — is its child rather than a stray root on the Worker); `run.observe_workspace`, `run.description_turn` (the coding run's extra model turn for a missing PR description, [pr-description.md](pr-description.md) item 5 — its `run.agent` hangs under it, as the re-review's does under `run.settle_reviewed_head`), `run.verdict_turn` (the review run's extra model turn for a missing [verdict](../vocabulary.md#verdict), [agent-review.md](agent-review.md) item 5 — its `run.agent` hangs under it the same way), `run.pr_post_step`, `run.review_post_step` (the review post-step, [agent-review.md](agent-review.md) item 18 — a review run only), `run.reading_diff_join`, `run.pr_description_join` (the join on the PR description's store lookup, [reading-diff.md](reading-diff.md) item 7); `post.card_close` and `post.reply` (between finish and seal, on the stream); log-only `post.workspace_release`, `post.ledger_finishing`, `post.followups`, `post.history_write`, `post.settled_outcome` (a late child, minutes after). Sync decisions (the repo and PR-head gates) are attrs on the refusal that follows, not spans of their own. `registry.create` carries `receivedAt` (a resume keeps its original stamps); `trace.bindRun` follows it; `run_meta` carries `traceId`, and a command run publishes `run_meta { agent: "command", traceId }` with no model and no repo. The card ticks from receipt: its clock is `receivedAt`, a 5 s setup heartbeat repaints it through setup with the card sink's label (`◐ *coding* · 42s — attaching the workspace…`) until the run loop's own heartbeat takes over; every close carries the request's elapsed time (`📦 … · not started (repo access) · 12s`, `❌ setup failed · … · 3m 04s`), and, at `debug` ([routing-and-config.md](routing-and-config.md) item 28), the shape line and the queued caption lead its detail when the card's gate passes (`cardShapeLine`: a minute of window or 15 s of getting ready, then item 5's informativeness rule; `queuedCaption` from a minute) — a runless close over the root's children so far to now, a done close over `[receivedAt, finishedAt]`. The root ends in `dispatch()`'s outermost finally with `status` (`completed`, `refused` when a `dispatch.refuse` happened, `stopped`, `failed`), after the drain and the tail and before the fresh turn, which is a request of its own: received now, `queuedBehindMs = now − the earliest follow-up's arrival` on its root and its card. A restart from its request (`prepareRestartTurn`, [run-history.md](run-history.md) item 54) is a request of its own the same way — received now, no queued numbers — and its root carries `restartOfRunId` at start instead: the page names the run it restarts rather than counting the predecessor's lifetime as a wait. **The no-gaps test** (`src/core/dispatcher.test.ts::no gaps…`): an `AsyncLocalStorage` `SpanContext` on the injected tracer, a ticking clock that advances only when a timed fake settles, every awaited fake wrapped; on each golden path every tick has a span (a `null` span is a gap) and the partition over the request's streamed spans equals the ticks per bucket with `overheadMs === the uncounted ticks + backgroundOnlyMs` (0 on these paths). A done close reads its shape from the finish-site diagnosis (`diagnosis.shape`, [run-friction.md](run-friction.md) item 3) — the same partition the record stores and the report prints — while a runless close still partitions the root's children so far. Deferred to their own PRs: `dispatch.gate.repo`/`dispatch.gate.pr_head` as spans (sync today), `post.reflection` (the scheduler returns nothing awaitable). +18. **One request, one root — the dispatcher's spans.** `startRequestRoot(deps, { channel, receivedAt })` (`src/core/requestTrace.ts`) is the one constructor of roots: each channel adapter calls it when our process sees the message — Slack before its redelivery guard, HTTP and MCP once the caller's identity is established and the body parsed, the CLI at entry — stamps the message `receivedAt` (Slack also `originAt`, the platform's `ts`) and hands the `RequestTrace` to `dispatch()` through `DispatchOptions.trace` (the `DispatchFn` seam carries it); a dispatch without one (tests, an older caller) starts its own at entry. The root's sinks are the log sink at `tracing.log` (or `CoreDeps.sinks`, a test's recording sink), the run-stream sink, the card sink and a collector for the runless closes; `CoreDeps.clock` and `CoreDeps.tracer` are the injection points the no-gaps test uses. Every awaited step of `dispatch()` is a `span(fn)` child of the root at the site the work starts: `dispatch.history`, `dispatch.repo_context` and `dispatch.memory_read` (wrapping the promises as they are created, so their overlap is real), `dispatch.ack_card`, `dispatch.admission` (the steer ack, a superseded resume), `dispatch.refuse` (every refusal — the close and the reply as one span, `outcome` naming why: `agent_allowlist`, `repo_not_onboarded`, `repo_unverified`, `repo_access`, `pr_head_unknown`, `which_branch`, `workspace_head_mismatch`, `ship_preflight`, `setup_failed`…), `dispatch.workspace.attach` (`backend`), `dispatch.gate.attached_head` (`outcome`), `dispatch.mcp_discovery`, `dispatch.compose` (the wait on the memory read; the composition is synchronous), `dispatch.channel_visibility`, `dispatch.ledger_claim`, `dispatch.ship_preflight`; `run.command` (a command run's body, `command` naming it; a runless command reply too, log-only there); the background span `run.reading_diff` (`outcome`; started by `startReviewReadingDiff` under the root, so the diff's exec — and its `http.client` hop to a sandbox — is its child rather than a stray root on the Worker); `run.observe_workspace`, `run.description_turn` (the coding run's extra model turn for a missing PR description, [pr-description.md](pr-description.md) item 5 — its `run.agent` hangs under it, as the re-review's does under `run.settle_reviewed_head`), `run.verdict_turn` (the review run's extra model turn for a missing [verdict](../vocabulary.md#verdict), [agent-review.md](agent-review.md) item 5 — its `run.agent` hangs under it the same way), `run.pr_post_step`, `run.review_post_step` (the review post-step, [agent-review.md](agent-review.md) item 18 — a review run only), `run.reading_diff_join`, `run.pr_description_join` (the join on the PR description's store lookup, [reading-diff.md](reading-diff.md) item 7); `post.card_close` and `post.reply` (between finish and seal, on the stream); log-only `post.workspace_release`, `post.ledger_finishing`, `post.followups`, `post.history_write`, `post.settled_outcome` (a late child, minutes after). Sync decisions (the repo and PR-head gates) are attrs on the refusal that follows, not spans of their own. `registry.create` carries `receivedAt` (a resume keeps its original stamps); `trace.bindRun` follows it; `run_meta` carries `traceId`, and a command run publishes `run_meta { agent: "command", traceId }` with no model and no repo. The card ticks from receipt: its clock is `receivedAt`, a 5 s setup heartbeat repaints it through setup with the card sink's label (`◐ *coding* · 42s — attaching the workspace…`) until the run loop's own heartbeat takes over; every close carries the request's elapsed time (`📦 … · not started (repo access) · 12s`, `❌ setup failed · … · 3m 04s`), and, at `debug` ([routing-and-config.md](routing-and-config.md) item 28), the shape line and the queued caption lead its detail when the card's gate passes (`cardShapeLine`: a minute of window or 15 s of getting ready, then item 5's informativeness rule; `queuedCaption` from a minute) — a runless close over the root's children so far to now, a done close over `[receivedAt, finishedAt]`. The root ends in `dispatch()`'s outermost finally with `status` (`completed`, `refused` when a `dispatch.refuse` happened, `stopped`, `failed`), after the drain and the tail and before the fresh turn, which is a request of its own: received now, `queuedBehindMs = now − the earliest follow-up's arrival` on its root and its card. A restart from its request (`prepareRestartTurn`, [run-history.md](run-history.md) item 54) is a request of its own the same way — received now, no queued numbers — and its root carries `restartOfRunId` at start instead: the page names the run it restarts rather than counting the predecessor's lifetime as a wait. **The no-gaps test** (`src/core/dispatcher.test.ts::no gaps…`): an `AsyncLocalStorage` `SpanContext` on the injected tracer, a ticking clock that advances only when a timed fake settles, every awaited fake wrapped; on each golden path every tick has a span (a `null` span is a gap) and the partition over the request's streamed spans equals the ticks per bucket with `overheadMs === the uncounted ticks + backgroundOnlyMs` (0 on these paths). A done close reads its shape from the finish-site diagnosis (`diagnosis.shape`, [run-friction.md](run-friction.md) item 3) — the same partition the record stores and the report prints — while a runless close still partitions the root's children so far. Deferred to their own PRs: `dispatch.gate.repo`/`dispatch.gate.pr_head` as spans (sync today), `post.reflection` (the scheduler returns nothing awaitable). 19. **The resident's and the sandbox's own measurements.** The resident Worker records every command it runs for one `/attach` or `/op` — `runOk`'s steps (`clone`, `install`, `worktree-clean`…), the op's command (`test`, `build`), and each wait for the mirror lock (`mutex_wait`, `waitedMs`) — into a per-request collector (`src/execution/residentStepTrace.ts`, scoped by an `AsyncLocalStorage` so a concurrent request's steps never land on the wrong answer) and returns them in the answer's `trace`: offsets from the request's start (`startMs`, `durationMs`), a `status`, `exitCode`/`timedOut` when the step was a command, bounded to 64 steps and 8 KB, names sanitized to `[a-z0-9_-]`. The bot re-validates at the parse boundary (`sanitizeGraftedSteps`, `src/execution/residentTrace.ts` — an Anti-Corruption Layer: every field rebuilt from an allowlist, error text dropped, an out-of-range exit code dropped, the bounds re-applied) and grafts the steps under the span that made the call (`Span.graft`, a child with both stamps supplied): an attach's under `dispatch.workspace.attach` as `dispatch.workspace.attach.`, an op's under the command run's `run.command` as `run.command.` (the trace rides the op's value and the chat result), each rebased so the resident's request start is the calling span's start and clipped to the bot's now, with `backend: resident`, `exitCode`, `timedOut`, `waitedMs` as attrs and an `infra` classification (never a message) on a failed step; the parent gets `clockSkewMs` — what the bot waited minus what the resident measured. A refusal carries the steps that led to it too (a failed attach's trace is the one that says which step blew the budget): the bot pins them on the error the refusal becomes (`withResidentTrace`/`residentTraceOf`, found through a wrapping `cause`), the factory's sandbox fallback hands them on as the selection's `trace`, and the dispatcher grafts them under the attach span whether the attach returned or threw. A Worker predating the trace binds and answers as before (no `trace`, no grafts). The sandbox Worker stamps `durationMs` on its `/exec` document — the sandbox's own wall time for the command, the `serverMs` a later PR puts on the `tool.*` span; nothing on the bot reads it yet, and the executor is indifferent to its presence. The resident's own roots (`resident.attach`, `resident.op`, the refresh cycle) and the sandbox's `sandbox.exec` log lines are the Worker-roots PR's. 20. **The bot's own roots, and the deploy's.** Work no request caused gets a root of its own through `startProcessRoot` (`src/core/requestTrace.ts`), on the leading sinks only — it streams to no run and paints no card, so under `tracing.log: roots` it is one JSON line when it ends: `slack.catch_up` around each reconnect catch-up pass ([slack-channel.md](slack-channel.md) item 7; `channels`, `missed`, `silenced`, `orphans`, `skipped`), `drain` around the shutdown drain (`signal`, `runs` held at the start, `handed` to the next generation, `sealed`, `abandonedRuns` at the exit). Two more roots exist for the bot's calls to the resident Worker's admin listing, which would otherwise reach the Worker with no trace to adopt and mint one root per call there: `resident.fleet_refresh` around each background read of the fleet facts ([routing-and-config.md](routing-and-config.md) item 11; `httpStatus`, `residents`), and `dashboard.residents` around each residents page request (`route` `index`/`detail`, `httpStatus`). Both hand the root to the admin client (`withSpan`), so the Worker's `resident.fetch` for `/residents` is a child on the bot's trace. The deploy runner ([release-and-deploy.md](release-and-deploy.md) item 19) starts one `deploy.step.` root per step on its own output at `slow`, `outcome` one of `live`/`deployed`/`not_live`/`failed`/`threw` (`busy` went with the deploy gate — release-and-deploy item 13), and its live gate is the step's one child, `deploy.wait_live`, whose `waitedMs` is the number the runner's `live (…; Ns after the upload)` line prints — the same value, never a second measurement. All of these are log-only names: they have no partition class and never stream. The clock ratchet (item 4) shrinks with them: the dispatcher, the composition root, the Slack adapter, the catch-up, the admission, the command registry and the deploy runner read no `Date.now` — the dispatcher through `CoreDeps.clock`, the runner through its `deps.now`, the rest through `systemClock` or a caller's `now`. The Workers' own roots (`.fetch`, `cron.`) and the remaining reads are the later PRs'. 21. **Trace context between our own Workers — the bot's side.** `tracedFetch(parent, url, init, { route })` (`src/core/trace/tracedFetch.ts`) is the one way the bot calls a Worker: one log-only `http.client` span under the caller's span, ending at the response headers (the body is the caller's to read under its own deadline), carrying `host`, the caller's closed-table `route`, `method` and `httpStatus` — never a header, a query string or a body; a transport failure is classified (`timeout` on an abort, else `transport`) and carries no message. The `traceparent` header (`00---01`) is set only when the request's host is one of ours: `internalHostsOf` (`src/core/trace/internalHosts.ts`) computes the set once at startup from the configured URLs — the resident, the sandbox, the state Worker, the schedules and overrides Workers, the public shim — matched exactly on `hostname[:port]`, never a suffix, and one `[trace] internal hosts` log line names it; a call to any other host (GitHub, Slack, a model provider, an MCP server) carries no trace context. Without a parent span there is no trace to carry and the call is a plain fetch. The parent reaches the clients explicitly, never through ambient state: the runner's `TracingExecutor` hands each `exec.*` span to the inner executor as `opts.span` (`ExecTraceOptions` on every `Executor` method), the resident and sandbox clients thread it to their one fetch, the dispatcher's `dispatch.workspace.attach` span reaches the factory's probe and attach, and `Operations.run` takes a `trace.span`. The container edges — the Slack, HTTP and MCP adapters and the live-view router — ignore an inbound `traceparent`/`tracestate`/`baggage` unconditionally: they mint their own root and have no code path that reads one. `ScheduleFiring.traceId` is the shim's trace for a firing, optional until the shim has roots. The shim's strip-and-mint, the Workers' own roots and log sinks, the cron roots and the `github.*` spans are the next PR's. diff --git a/src/core/dispatch/authorize.test.ts b/src/core/dispatch/authorize.test.ts index b6cb5bd65..86e6f2c3c 100644 --- a/src/core/dispatch/authorize.test.ts +++ b/src/core/dispatch/authorize.test.ts @@ -547,17 +547,22 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef const review = getAgent("review"); const repoCtx: RepoContext = { repo: "acme/api", pr: 41, ref: "feature/x", headSha: SHA_A }; - function selection(over: { sha?: string; resident?: boolean; observed?: string; seeded?: boolean } = {}): { + function selection(over: { sha?: string; resident?: boolean; observed?: string | string[]; seeded?: boolean } = {}): { selection: ExecutorSelection; releases: string[]; commands: string[]; } { const releases: string[] = []; const commands: string[] = []; + const observed = Array.isArray(over.observed) ? [...over.observed] : [over.observed ?? ""]; const executor = { exec: async (command: string) => { commands.push(command); - return command.includes("rev-parse HEAD") ? (over.observed ?? "") : "provisioned"; + return command.includes("rev-parse HEAD") + ? observed.length > 1 + ? observed.shift()! + : (observed[0] ?? "") + : "provisioned"; }, release: async (mode: string) => { releases.push(mode); @@ -602,7 +607,7 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef it("the worktree attached at the PR head: verified, the repo context unchanged", async () => { const s = setup(); - const { selection: sel } = selection({ sha: SHA_A }); + const { selection: sel } = selection({ sha: SHA_A, observed: `${SHA_A}\n` }); expect(await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel))).toEqual({ kind: "allowed", repoCtx, @@ -612,10 +617,23 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef expect(s.replies).toEqual([]); }); + it("a resident binding SHA cannot verify a checkout whose observed HEAD differs", async () => { + const s = setup(); + const { selection: sel, commands, releases } = selection({ sha: SHA_A, observed: `${SHA_B}\n` }); + + expect(await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel))).toEqual({ + kind: "refused", + reason: "workspace_head_mismatch", + }); + expect(commands.filter((command) => command === "git rev-parse HEAD")).toHaveLength(2); + expect(releases).toEqual(["always"]); + expect(s.replies[0]).toContain(`workspace-observed HEAD for feature/x is at ${SHA_B}`); + }); + it("a push raced the request and the worktree sits at the PR's current head: adopted — the repo context takes that head and the caller re-publishes the run meta", async () => { const s = setup(); const asked: unknown[] = []; - const { selection: sel } = selection({ sha: SHA_B }); + const { selection: sel } = selection({ sha: SHA_B, observed: `${SHA_B}\n` }); const out = await authorizeAttachedHead( { ...s.deps, @@ -635,19 +653,75 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef expect(asked).toEqual([{ repo: "acme/api", number: 41 }]); }); - it("the branch moved while the worktree was being attached: refused — the pool user released, the card closed, one named reply, no model turn", async () => { + it("the workspace stays mismatched after one reprovision: refused — the pool user released, the card closed, one evidence-only reply, no model turn", async () => { const s = setup(); - const { selection: sel, releases } = selection({ sha: SHA_B }); + const { selection: sel, releases } = selection({ sha: SHA_B, observed: `${SHA_B}\n` }); const out = await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel)); - expect(out).toEqual({ kind: "refused", reason: "branch_moved" }); - expect(s.refusals).toEqual(["branch_moved"]); + expect(out).toEqual({ kind: "refused", reason: "workspace_head_mismatch" }); + expect(s.refusals).toEqual(["workspace_head_mismatch"]); expect(releases).toEqual(["always"]); - expect(closedReasons(s.closes).join("\n")).toContain("branch moved"); - expect(s.replies[0]).toContain( - `🔀 Review of acme/api#41 not started: the resident binding for feature/x is at ${SHA_B}, but the PR head is ${SHA_A}`, + expect(closedReasons(s.closes).join("\n")).toContain("workspace head mismatch"); + expect(s.replies[0]).toContain(SHA_B); + expect(s.replies[0]).toContain(`expected reviewed head is ${SHA_A}`); + expect(s.replies[0]).not.toMatch(/branch moved|push|force-push|bug/i); + }); + + it("a resident moveTo failure refuses without describing the initial binding as the retry observation", async () => { + const s = setup(); + const { selection: sel, releases } = selection({ sha: SHA_B, observed: `${SHA_B}\n` }); + sel.executor.moveTo = async () => { + throw new Error("resident move failed"); + }; + + expect(await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel))).toEqual({ + kind: "refused", + reason: "workspace_head_mismatch", + }); + expect(releases).toEqual(["always"]); + expect(s.replies[0]).toContain(`initial workspace-observed HEAD for feature/x was at ${SHA_B}`); + expect(s.replies[0]).toContain("automatic reprovision failed before a retry HEAD could be observed"); + expect(s.replies[0]).not.toContain( + `after one automatic reprovision, the resident binding for feature/x is at ${SHA_B}`, ); }); + it("resident moveTo metadata cannot verify a retry whose observed HEAD is still mismatched", async () => { + const s = setup(); + const { selection: sel, commands, releases } = selection({ sha: SHA_B, observed: `${SHA_B}\n` }); + sel.executor.moveTo = async () => ({ sha: SHA_A }); + + expect(await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel))).toEqual({ + kind: "refused", + reason: "workspace_head_mismatch", + }); + expect(commands.filter((command) => command === "git rev-parse HEAD")).toHaveLength(2); + expect(releases).toEqual(["always"]); + expect(s.replies[0]).toContain(`workspace-observed HEAD for feature/x is at ${SHA_B}`); + }); + + it("a successful retry publishes the observed checkout HEAD instead of moveTo metadata", async () => { + const s = setup(); + const { + selection: sel, + commands, + releases, + } = selection({ + sha: SHA_B, + observed: [`${SHA_B}\n`, `${SHA_A}\n`], + }); + sel.executor.moveTo = async () => ({ sha: SHA_C }); + + expect(await authorizeAttachedHead({ ...s.deps, fetchPrHead: async () => SHA_C }, ctx(s, sel))).toEqual({ + kind: "allowed", + repoCtx, + verifiedAtAttach: true, + headAdopted: false, + }); + expect(commands.filter((command) => command === "git rev-parse HEAD")).toHaveLength(2); + expect(sel.binding?.sha).toBe(SHA_A); + expect(releases).toEqual([]); + }); + it("an unseeded non-resident backend provisions the PR checkout at the resolved head before verifying it; a mismatch is refused and released before the model", async () => { const verified = setup(); const atHead = selection({ resident: false, observed: `${SHA_A}\n` }); @@ -687,10 +761,10 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef { ...mismatched.deps, fetchPrHead: async () => SHA_C }, ctx(mismatched, elsewhere.selection), ), - ).toEqual({ kind: "refused", reason: "branch_moved" }); + ).toEqual({ kind: "refused", reason: "workspace_head_mismatch" }); expect(elsewhere.releases).toEqual(["always"]); expect(mismatched.replies[0]).toContain(`workspace-observed HEAD for feature/x is at ${SHA_B}`); - expect(mismatched.replies[0]).toContain(`PR head is ${SHA_A}`); + expect(mismatched.replies[0]).toContain(`expected reviewed head is ${SHA_A}`); }); it("an unreadable workspace head is refused fail-closed; only non-review and resumed runs skip the first-turn guard", async () => { @@ -698,10 +772,12 @@ describe("authorizeAttachedHead — every review workspace is at the PR head bef const noSha = selection({ resident: false, observed: "exit 128: not a git repository" }); expect(await authorizeAttachedHead(unknown.deps, ctx(unknown, noSha.selection))).toEqual({ kind: "refused", - reason: "branch_moved", + reason: "workspace_head_mismatch", }); expect(noSha.releases).toEqual(["always"]); - expect(unknown.replies[0]).toContain("workspace-observed HEAD for feature/x could not be read"); + expect(unknown.replies[0]).toContain( + "workspace-observed HEAD for feature/x could not be read after one automatic reprovision", + ); const asked: unknown[] = []; const deps: AuthorizeDeps = { diff --git a/src/core/dispatch/authorize.ts b/src/core/dispatch/authorize.ts index 0a9ded023..00475ca8b 100644 --- a/src/core/dispatch/authorize.ts +++ b/src/core/dispatch/authorize.ts @@ -388,7 +388,7 @@ export async function authorizePrHead( * the attach verified the worktree is at that head — or it was refused. */ export type AttachedHeadGate = | { kind: "allowed"; repoCtx: RepoContext; verifiedAtAttach: boolean; headAdopted: boolean } - | { kind: "refused"; reason: "branch_moved" }; + | { kind: "refused"; reason: "workspace_head_mismatch" }; /** * A cold review starts in an empty per-thread workspace. Provision the exact @@ -399,14 +399,24 @@ export type AttachedHeadGate = * resolved base are fetched beside the PR head because the review prompt diffs * against those remote-tracking refs. */ +function reviewCheckoutFacts(repoCtx: RepoContext & { repo: string; pr: number }): { + remote: string; + helper: string; + refspecs: string[]; +} { + return { + remote: `https://github.com/${repoCtx.repo}.git`, + helper: `!f() { test -n "$GH_TOKEN" || exit 1; printf '%s\\n' 'username=x-access-token' "password=$GH_TOKEN"; }; f`, + refspecs: [ + "+HEAD:refs/remotes/origin/HEAD", + `+refs/pull/${repoCtx.pr}/head:refs/remotes/origin/pull/${repoCtx.pr}/head`, + ...(repoCtx.baseRef ? [`+refs/heads/${repoCtx.baseRef}:refs/remotes/origin/${repoCtx.baseRef}`] : []), + ], + }; +} + function coldReviewCheckoutCommand(repoCtx: RepoContext & { repo: string; pr: number; headSha: string }): string { - const remote = `https://github.com/${repoCtx.repo}.git`; - const helper = `!f() { test -n "$GH_TOKEN" || exit 1; printf '%s\\n' 'username=x-access-token' "password=$GH_TOKEN"; }; f`; - const refspecs = [ - "+HEAD:refs/remotes/origin/HEAD", - `+refs/pull/${repoCtx.pr}/head:refs/remotes/origin/pull/${repoCtx.pr}/head`, - ...(repoCtx.baseRef ? [`+refs/heads/${repoCtx.baseRef}:refs/remotes/origin/${repoCtx.baseRef}`] : []), - ]; + const { remote, helper, refspecs } = reviewCheckoutFacts(repoCtx); return [ "set -eu", "find . -mindepth 1 -maxdepth 1 ! -name attachments -exec rm -rf -- {} +", @@ -417,13 +427,27 @@ function coldReviewCheckoutCommand(repoCtx: RepoContext & { repo: string; pr: nu ].join("\n"); } +/** Refresh an already-seeded review checkout at the same expected head. */ +function seededReviewCheckoutCommand( + repoCtx: RepoContext & { repo: string; pr: number; headSha: string }, + workspace: string, +): string { + const { helper, refspecs } = reviewCheckoutFacts(repoCtx); + const git = `git -C ${shellQuote(workspace)}`; + return [ + "set -eu", + `${git} -c credential.helper= -c credential.helper=${shellQuote(helper)} fetch --force --no-tags origin ${refspecs.map(shellQuote).join(" ")}`, + `${git} checkout --detach --force ${shellQuote(repoCtx.headSha)}`, + ].join("\n"); +} + /** * The attached-head guard (docs/reference/specs/agent-review.md item 10): every * PR review is at its resolved head before any model turn. Cold backends are - * provisioned first; seeded backends are observed at their named checkout; a - * resident's attach binding is its proof. Verified; adopted (a push raced the - * request and the workspace is at the PR's head now); or refused (mismatch or - * unreadable): the workspace is released and one named reply is sent. + * provisioned first; seeded and resident backends are observed through their + * executor. Verified; adopted (a push raced the request and the workspace is + * at the PR's head now); or refused (mismatch or unreadable): the workspace is + * released and one named reply is sent. */ export async function authorizeAttachedHead( deps: AuthorizeDeps, @@ -441,17 +465,16 @@ export async function authorizeAttachedHead( const { executor, resident, binding } = selection; let repoCtx = ctx.repoCtx; // Attach-head check (docs/reference/specs/agent-review.md item 10): every PR - // review proves the workspace's HEAD before any model turn. A resident's - // attach binding is the proof it just returned; every other backend is - // observed directly in the checkout. Unknown is a refusal, not permission - // for the model to turn an infrastructure failure into a finding. + // review proves the workspace's HEAD before any model turn. Every backend, + // including a resident whose attach returned SHA metadata, is observed + // directly in the checkout. Unknown is a refusal, not permission for the + // model to turn an infrastructure failure into a finding. let verifiedAtAttach = false; let headAdopted = false; if (!resume && agent.name === "review" && repoCtx.pr !== undefined && repoCtx.repo) { const pr = { repo: repoCtx.repo, number: repoCtx.pr }; const expectedHeadSha = repoCtx.headSha; const guard = await root.span("dispatch.gate.attached_head", async (span) => { - const source = resident && binding?.sha !== undefined ? "resident binding" : "workspace-observed"; const command = selection.seeded?.workspace ? `git -C ${shellQuote(selection.seeded.workspace)} rev-parse HEAD` : "git rev-parse HEAD"; @@ -461,19 +484,52 @@ export async function authorizeAttachedHead( { timeoutMs: BASH_TIMEOUT_MAX_MS }, ); } - const sha = - source === "resident binding" - ? binding?.sha - : await executor - .exec(command, { timeoutMs: 30_000 }) - .then(parseRevParseOutput) - .catch(() => undefined); + const observeHead = () => + executor + .exec(command, { timeoutMs: 30_000, span }) + .then(parseRevParseOutput) + .catch(() => undefined); + const sha = await observeHead(); const g = await guardAttachedHead({ pr, expectedHeadSha, - attached: { sha, ref: binding?.ref ?? repoCtx.ref, source }, + attached: { sha, ref: binding?.ref ?? repoCtx.ref, source: "workspace-observed" }, fallbackRef: repoCtx.ref, fetchPrHead: deps.fetchPrHead ?? currentPrHeadSha, + reprovision: async (headSha) => { + if (executor.moveTo) { + await executor.moveTo(headSha, { span }); + const observedSha = await observeHead(); + if (selection.binding && observedSha !== undefined) { + selection.binding = { + ...selection.binding, + ref: repoCtx.ref ?? selection.binding.ref, + sha: observedSha, + }; + } + return { + sha: observedSha, + ref: repoCtx.ref, + source: "workspace-observed" as const, + }; + } + if (selection.seeded?.workspace) { + await executor.exec( + seededReviewCheckoutCommand( + { ...repoCtx, repo: pr.repo, pr: pr.number, headSha }, + selection.seeded.workspace, + ), + { timeoutMs: BASH_TIMEOUT_MAX_MS, span }, + ); + } else { + await executor.exec(coldReviewCheckoutCommand({ ...repoCtx, repo: pr.repo, pr: pr.number, headSha }), { + timeoutMs: BASH_TIMEOUT_MAX_MS, + span, + }); + } + const retried = await observeHead(); + return { sha: retried, ref: repoCtx.ref, source: "workspace-observed" as const }; + }, logKey: msg.threadKey, }); span.setAttrs({ outcome: g.outcome }); @@ -486,13 +542,18 @@ export async function authorizeAttachedHead( verifiedAtAttach = true; headAdopted = true; } else if (guard.outcome === "refused") { - await refuse(refusalOf("branch_moved", guard.reply), async () => { + await refuse(refusalOf("workspace_head_mismatch", guard.reply), async () => { if (executor.release) await executor.release("always").catch(() => {}); await card.done( - shell.close({ kind: "not_started", icon: "🔀", reason: "branch moved", ...closeLines(clock(), false) }), + shell.close({ + kind: "not_started", + icon: "🔀", + reason: "workspace head mismatch", + ...closeLines(clock(), false), + }), ); }); - return { kind: "refused", reason: "branch_moved" }; + return { kind: "refused", reason: "workspace_head_mismatch" }; } } return { kind: "allowed", repoCtx, verifiedAtAttach, headAdopted }; diff --git a/src/core/dispatch/provision.ts b/src/core/dispatch/provision.ts index 23f579b20..f6b476c61 100644 --- a/src/core/dispatch/provision.ts +++ b/src/core/dispatch/provision.ts @@ -858,7 +858,11 @@ async function attachRound( ): Promise { const { threadKey, agent, profile, repoCtx, root, clock, reattach, stopSignal, remainingMs, requester } = ctx; const { onSetupNote } = ctx; - const ownPr = ownPrOf(repoCtx); + // A review target's PR-derived ref is authoritative. Passing `ownPr` asks + // the resident to preserve or conditionally move a sticky thread binding; + // that is right for a coding follow-up, but can keep a plan unit's branch + // when this round is reviewing an adopted pull request on another branch. + const ownPr = agent.name === "review" ? undefined : ownPrOf(repoCtx); return root.span("dispatch.workspace.attach", async (span) => { // The resident's own steps (clone, install, the mutex wait…) graft under // this span, rebased to its start (docs/reference/specs/tracing.md item 19) — on a diff --git a/src/core/dispatch/reply.test.ts b/src/core/dispatch/reply.test.ts index 27896d7ba..19239cbf1 100644 --- a/src/core/dispatch/reply.test.ts +++ b/src/core/dispatch/reply.test.ts @@ -264,7 +264,7 @@ describe("renderRefusal — the one rendering of a Refusal", () => { // lines, the references' one line — and the inventory's quote as a // literal, so a drifted builder fails here instead of shipping. Codes // whose text another module builds from live data (`pr_head_unknown` from - // `checkPrHeadPreflight`, `branch_moved` from `guardAttachedHead`, + // `checkPrHeadPreflight`, `workspace_head_mismatch` from `guardAttachedHead`, // `ship_preflight` from the ship preflight) are proven byte-identical by // those modules' own tests; the silent codes (`coordinator_thread_live`, // `workspace_lost`, `setup_failed`) and `uncaught` render nothing. @@ -414,7 +414,7 @@ describe("renderRefusal — the one rendering of a Refusal", () => { // another module's tested builder, or silent by design. const provenElsewhere: RefusalCode[] = [ "pr_head_unknown", - "branch_moved", + "workspace_head_mismatch", // (record 0054): each producer's own test proves its sentences // byte-identical — the ship preflight's ten (preflight.test.ts), the // plan hand-off's fifteen (handOff.test.ts), the resolve parser diff --git a/src/core/dispatch/reply.ts b/src/core/dispatch/reply.ts index 6fab7484a..fcbc9c782 100644 --- a/src/core/dispatch/reply.ts +++ b/src/core/dispatch/reply.ts @@ -520,7 +520,7 @@ export async function renderConfirmationOffer(io: ChannelIO, offer: Confirmation * Codes missing here carry producer-built text on the `Refusal` instead: * `profile_bounded` (`profileRefusalReply`), `follow_up_refused` and its * `elsewhere_` twin (`refusalReply` in admission.ts), `pr_head_unknown` - * (`checkPrHeadPreflight`), `branch_moved` (`guardAttachedHead`), + * (`checkPrHeadPreflight`), `workspace_head_mismatch` (`guardAttachedHead`), * `ship_preflight` (the preflight's own reply), the reference codes (the one * `REFERENCE_REFUSAL` line), the click codes (confirm.ts's lines), and the * silent codes (`coordinator_thread_live`, `workspace_lost`, `setup_failed`, diff --git a/src/core/dispatcher.test.ts b/src/core/dispatcher.test.ts index b2c78c02b..3a28470c4 100644 --- a/src/core/dispatcher.test.ts +++ b/src/core/dispatcher.test.ts @@ -1614,16 +1614,18 @@ describe("resident repo dispatch", () => { // clarifying question, no model turn burned), and the resident prompt variant // selected AFTER executor resolution via RunOptions.system. -/** Router-style fetch stub for the resident service: /status and /attach. */ +/** Router-style fetch stub for the resident service: /status, /attach and /exec. */ function residentFetchStub( handlers: { status?: () => Response; attach?: (body: Record) => Response; + exec?: (body: Record) => Response; /** GitHub's REST API, for a resolution that fetches a pull request. */ github?: (path: string) => Response; } = {}, ) { const calls: Array<{ path: string; host: string; body?: Record }> = []; + let attachedSha = "abc"; const fn = vi.fn(async (url: unknown, init?: RequestInit) => { const { pathname: path, host } = new URL(String(url)); const body = init?.body ? (JSON.parse(String(init.body)) as Record) : undefined; @@ -1633,12 +1635,28 @@ function residentFetchStub( return handlers.status?.() ?? new Response(JSON.stringify({ state: "warm", reason: "" }), { status: 200 }); } if (path === "/attach") { - return ( + const response = handlers.attach?.(body ?? {}) ?? new Response( JSON.stringify({ workspace: "/workspace/threads/t/main", ref: "main", sha: "abc", user: "worker2" }), { status: 200 }, - ) + ); + const answer = (await response + .clone() + .json() + .catch(() => undefined)) as { sha?: unknown } | undefined; + if (typeof answer?.sha === "string") attachedSha = answer.sha; + return response; + } + if (path === "/exec") { + if (handlers.exec === undefined && body?.command !== "git rev-parse HEAD") { + throw new Error(`unexpected fetch: ${String(url)}`); + } + return ( + handlers.exec?.(body ?? {}) ?? + new Response(JSON.stringify({ stdout: `${attachedSha}\n`, stderr: "", exitCode: 0, truncated: false }), { + status: 200, + }) ); } throw new Error(`unexpected fetch: ${String(url)}`); @@ -2214,13 +2232,110 @@ describe("repo/ref resolution + resident prompt selection", () => { expect(replies.some((r) => /not started/i.test(r))).toBe(false); }); - it("a resident review attached at ANOTHER commit than the PR head is not started: named reply, no model turn, worktree released", async () => { + it("a genuine push during workspace preparation is reprovisioned at the reviewed head and the review runs", async () => { vi.stubEnv("SANDBOX_TOKEN", "tok"); vi.stubEnv("RESIDENT_OPERATOR_TOKEN", "rtok"); vi.stubEnv("GITHUB_APP_ID", ""); const head = "e".repeat(40); - const attached = "4dd3832099140ee5c76022a525bbc5e7629d5ada"; - // Own stub: the shared one has no /detach route, and the release is the point here. + const previousHead = "4dd3832099140ee5c76022a525bbc5e7629d5ada"; + let attaches = 0; + const { calls } = residentFetchStub({ + attach: () => { + attaches++; + return new Response( + JSON.stringify({ + workspace: "/workspace/threads/t-9f/patch-1", + ref: "patch-1", + sha: attaches === 1 ? previousHead : head, + user: "worker3", + }), + { status: 200 }, + ); + }, + }); + const provider = capturingProvider(); + const deps = makeDeps(RESIDENT_YAML_FIXTURE, provider); + deps.resolveRepoContext = () => ({ repo: "acme/api", ref: "patch-1", pr: 42, headSha: head, baseRef: "main" }); + deps.fetchPrHead = async () => head; + deps.postReviewComment = vi.fn(async () => {}); + const { io, replies } = fakeIO(); + + await dispatch(deps, msg("agent:review https://github.com/acme/api/pull/42", "slack:UADMIN"), io); + + expect(provider.requests).toHaveLength(2); // the review and its no-verdict follow-up both ran + expect(replies.some((r) => /not started/i.test(r))).toBe(false); + expect(calls.filter((c) => c.path === "/attach").map((c) => c.body?.sha)).toEqual([head, head]); + }); + + it("a unit reviewing an adopted pull request binds its attach to the pull request branch instead of its own", async () => { + vi.stubEnv("SANDBOX_TOKEN", "tok"); + vi.stubEnv("RESIDENT_OPERATOR_TOKEN", "rtok"); + vi.stubEnv("GITHUB_APP_ID", ""); + const head = "e".repeat(40); + const unitHead = "4dd3832099140ee5c76022a525bbc5e7629d5ada"; + const { calls } = residentFetchStub({ + attach: (body) => + new Response( + JSON.stringify( + body.ownPr === undefined + ? { + workspace: "/workspace/threads/t-9f/patch-1", + ref: "patch-1", + sha: head, + user: "worker3", + } + : { + workspace: "/workspace/threads/t-9f/unit-review-u1", + ref: "feature/unit-review-u1", + sha: unitHead, + user: "worker3", + }, + ), + { status: 200 }, + ), + }); + const provider = capturingProvider(); + const deps = makeDeps(RESIDENT_YAML_FIXTURE, provider); + deps.resolveRepoContext = () => ({ + repo: "acme/api", + ref: "patch-1", + refFromPr: true, + pr: 42, + prFromRecord: true, + headSha: head, + baseRef: "main", + }); + deps.fetchPrHead = async () => head; + deps.postReviewComment = vi.fn(async () => {}); + const contract = contractFromPlan({ + planMarkdown: "### U10. review the adopted pull request\n\nreview the adopted pull request\n", + unitId: "U10", + readSpec: () => undefined, + rebase: { branch: "feature/unit-review-u1", onto: "main" }, + }); + const { io, replies } = fakeIO(); + + await dispatch(deps, msg("agent:review https://github.com/acme/api/pull/42", "slack:UADMIN"), io, { contract }); + + expect(provider.requests).toHaveLength(2); + expect(replies.some((r) => /not started/i.test(r))).toBe(false); + expect(calls.find((c) => c.path === "/attach")?.body).toMatchObject({ + refHint: "patch-1", + sha: head, + readonly: true, + }); + expect(calls.find((c) => c.path === "/attach")?.body).not.toHaveProperty("ownPr"); + }); + + it("a failed reprovision refuses with the two heads and no cause the guard did not establish", async () => { + vi.stubEnv("SANDBOX_TOKEN", "tok"); + vi.stubEnv("RESIDENT_OPERATOR_TOKEN", "rtok"); + vi.stubEnv("GITHUB_APP_ID", ""); + const expectedHead = "e".repeat(40); + const firstHead = "4dd3832099140ee5c76022a525bbc5e7629d5ada"; + const reprovisionedHead = "5dd3832099140ee5c76022a525bbc5e7629d5adb"; + let attaches = 0; + let checkedOutHead = firstHead; const calls: Array<{ path: string; body?: Record }> = []; vi.stubGlobal( "fetch", @@ -2230,16 +2345,22 @@ describe("repo/ref resolution + resident prompt selection", () => { calls.push({ path, body }); if (path === "/status") return new Response(JSON.stringify({ state: "warm", reason: "" }), { status: 200 }); if (path === "/attach") { + attaches++; + checkedOutHead = attaches === 1 ? firstHead : reprovisionedHead; return new Response( JSON.stringify({ workspace: "/workspace/threads/t-9f/patch-1", ref: "patch-1", - sha: attached, + sha: checkedOutHead, user: "worker3", }), - { - status: 200, - }, + { status: 200 }, + ); + } + if (path === "/exec") { + return new Response( + JSON.stringify({ stdout: `${checkedOutHead}\n`, stderr: "", exitCode: 0, truncated: false }), + { status: 200 }, ); } if (path === "/detach") return new Response(JSON.stringify({ released: true }), { status: 200 }); @@ -2248,25 +2369,27 @@ describe("repo/ref resolution + resident prompt selection", () => { ); const provider = capturingProvider(); const deps = makeDeps(RESIDENT_YAML_FIXTURE, provider); - const ctx = { repo: "acme/api", ref: "patch-1", pr: 42, headSha: head, baseRef: "main" }; - deps.resolveRepoContext = () => ctx; - const post = vi.fn(async () => {}); - deps.postReviewComment = post; + deps.resolveRepoContext = () => ({ + repo: "acme/api", + ref: "patch-1", + pr: 42, + headSha: expectedHead, + baseRef: "main", + }); + deps.fetchPrHead = async () => "6dd3832099140ee5c76022a525bbc5e7629d5adc"; + deps.postReviewComment = vi.fn(async () => {}); const { io, replies, statuses } = fakeIO(); + await dispatch(deps, msg("agent:review https://github.com/acme/api/pull/42", "slack:UADMIN"), io); - expect(provider.requests).toHaveLength(0); // no model turn burned on a guaranteed-refused review - expect(post).not.toHaveBeenCalled(); + + expect(provider.requests).toHaveLength(0); const reply = replies.find((r) => /not started/i.test(r)) ?? ""; - expect(reply).toContain("acme/api#42"); - expect(reply).toContain(attached); // what the resident attached - expect(reply).toContain(head); // what the PR head is - expect(reply).toContain("This is a bug: the workspace was not reprovisioned automatically at the new head"); - const last = statuses[statuses.length - 1]; - expect(last.title).toMatch(/not started/); - // Before refusing, the PR's current head is asked once (item 12) — here the - // GET fails (unknown) — then the pool user goes back. - expect(calls.map((c) => c.path)).toEqual(["/status", "/attach", "/repos/acme/api/pulls/42", "/detach"]); - expect(calls[3]?.body).toMatchObject({ force: true }); + expect(reply).toContain(reprovisionedHead); + expect(reply).toContain(expectedHead); + expect(reply).not.toMatch(/branch moved|push|force-push|bug/i); + expect(statuses.at(-1)?.title).toMatch(/not started.*workspace head mismatch/i); + expect(calls.filter((c) => c.path === "/attach").map((c) => c.body?.sha)).toEqual([expectedHead, expectedHead]); + expect(calls.at(-1)).toMatchObject({ path: "/detach", body: { force: true } }); }); // agent-review.md item 12: the resident's attach fetches the mirror to the @@ -2331,10 +2454,8 @@ describe("repo/ref resolution + resident prompt selection", () => { ); expect(system).not.toContain(`Head commit: ${resolvedHead}`); expect(replies.some((r) => /not started/i.test(r))).toBe(false); - // The shared stub has no /exec route, so the workspace HEAD is unobservable - // and no head was reported: the guard fails closed as always. - expect(replies.some((r) => r.includes("reviewed head unknown"))).toBe(true); - expect(post).not.toHaveBeenCalled(); + expect(replies.some((r) => r.includes("reviewed head unknown"))).toBe(false); + expect(post).toHaveBeenCalledTimes(1); }); // When the PR head goes unresolved at resolution time and the run starts @@ -3222,7 +3343,7 @@ describe("review post-step", () => { expect(reviewTurns(provider)).toHaveLength(1); // no re-review expect(ex.moves).toEqual([]); expect(ex.probeSpans.length).toBeGreaterThan(0); - expect(new Set(ex.probeSpans)).toEqual(new Set(["none", "run.settle_reviewed_head"])); + expect(new Set(ex.probeSpans)).toEqual(new Set(["dispatch.gate.attached_head", "run.settle_reviewed_head"])); expect(spy.calls).toHaveLength(1); expect(spy.calls[0].target).toEqual({ repo: "acme/api", number: 42, commitId: OTHER_HEAD }); expect(spy.calls[0].body).toMatch( diff --git a/src/core/refusal.ts b/src/core/refusal.ts index 545ee9015..960ed18fa 100644 --- a/src/core/refusal.ts +++ b/src/core/refusal.ts @@ -47,7 +47,7 @@ const CAUSE_OF = { repo_not_onboarded: "request", repo_access: "policy", pr_head_unknown: "system", - branch_moved: "system", + workspace_head_mismatch: "system", coordinator_thread_live: "system", live_agent_allowlist: "policy", follow_up_refused: "request", diff --git a/src/core/reviewRound.test.ts b/src/core/reviewRound.test.ts index a1423abec..8646f565c 100644 --- a/src/core/reviewRound.test.ts +++ b/src/core/reviewRound.test.ts @@ -224,16 +224,19 @@ describe("guardAttachedHead (before any model call)", () => { // ever invoking a provider — no provider handle even reaches the unit. it("attached at the PR head → verified, no lookup", async () => { const fetchPrHead = vi.fn(async () => THIRD); + const reprovision = vi.fn(async () => ({ sha: THIRD })); const r = await guardAttachedHead({ pr: { repo: "acme/api", number: 42 }, expectedHeadSha: HEAD, attached: { sha: HEAD, ref: "patch-1" }, fallbackRef: undefined, fetchPrHead, + reprovision, logKey: "t", }); expect(r).toEqual({ outcome: "verified" }); expect(fetchPrHead).not.toHaveBeenCalled(); + expect(reprovision).not.toHaveBeenCalled(); }); it("attached at another commit that IS the PR's current head → adopted with that head", async () => { @@ -243,6 +246,9 @@ describe("guardAttachedHead (before any model call)", () => { attached: { sha: OTHER, ref: "patch-1" }, fallbackRef: undefined, fetchPrHead: async () => OTHER, + reprovision: async () => { + throw new Error("must not reprovision an adopted current head"); + }, logKey: "t", }); expect(r).toEqual({ outcome: "adopted", headSha: OTHER }); @@ -255,14 +261,15 @@ describe("guardAttachedHead (before any model call)", () => { attached: { sha: OTHER, ref: "patch-1" }, fallbackRef: undefined, fetchPrHead: async () => THIRD, + reprovision: async () => ({ sha: OTHER, ref: "patch-1", source: "workspace-observed" }), logKey: "t", }); expect(r.outcome).toBe("refused"); if (r.outcome === "refused") { expect(r.reply).toContain("acme/api#42"); expect(r.reply).toContain(`workspace-observed HEAD for patch-1 is at ${OTHER}`); - expect(r.reply).toContain(`PR head is ${HEAD}`); - expect(r.reply).toContain("This is a bug: the workspace was not reprovisioned automatically at the new head"); + expect(r.reply).toContain(`expected reviewed head is ${HEAD}`); + expect(r.reply).not.toMatch(/branch moved|push|force-push|bug/i); expect(r.reply).toContain("It is not a finding, and nothing was posted to GitHub"); } }); @@ -276,15 +283,60 @@ describe("guardAttachedHead (before any model call)", () => { fetchPrHead: async () => { throw new Error("GitHub down"); }, + reprovision: async () => ({ sha: OTHER, ref: "patch-1", source: "workspace-observed" }), logKey: "t", }); expect(r.outcome).toBe("refused"); if (r.outcome === "refused") expect(r.reply).toContain("workspace-observed HEAD for patch-1"); }); + it("a failed reprovision keeps the initial observation separate from the retry failure", async () => { + const r = await guardAttachedHead({ + pr: { repo: "acme/api", number: 42 }, + expectedHeadSha: HEAD, + attached: { sha: OTHER, ref: "patch-1", source: "resident binding" }, + fallbackRef: undefined, + fetchPrHead: async () => THIRD, + reprovision: async () => { + throw new Error("resident moveTo failed"); + }, + logKey: "t", + }); + expect(r.outcome).toBe("refused"); + if (r.outcome === "refused") { + expect(r.reply).toContain(`initial resident binding for patch-1 was at ${OTHER}`); + expect(r.reply).toContain("automatic reprovision failed before a retry HEAD could be observed"); + expect(r.reply).not.toContain(`after one automatic reprovision, the resident binding for patch-1 is at ${OTHER}`); + } + }); + + it("an invalid retry head names the initial observation and the unreadable retry separately", async () => { + const r = await guardAttachedHead({ + pr: { repo: "acme/api", number: 42 }, + expectedHeadSha: HEAD, + attached: { sha: OTHER, ref: "patch-1", source: "resident binding" }, + fallbackRef: undefined, + fetchPrHead: async () => THIRD, + reprovision: async () => ({ sha: "not-a-sha", ref: "patch-1", source: "resident binding" }), + logKey: "t", + }); + expect(r.outcome).toBe("refused"); + if (r.outcome === "refused") { + expect(r.reply).toContain(`initial resident binding for patch-1 was at ${OTHER}`); + expect(r.reply).toContain("resident binding for patch-1 could not be read after one automatic reprovision"); + expect(r.reply).not.toContain(`after one automatic reprovision, the resident binding for patch-1 is at ${OTHER}`); + } + }); + it("an invalid expected head stays with the earlier preflight; an unreadable workspace head is refused as infrastructure", async () => { const fetchPrHead = vi.fn(async () => HEAD); - const base = { pr: { repo: "acme/api", number: 42 }, fallbackRef: undefined, fetchPrHead, logKey: "t" }; + const base = { + pr: { repo: "acme/api", number: 42 }, + fallbackRef: undefined, + fetchPrHead, + reprovision: async () => ({ sha: undefined }), + logKey: "t", + }; expect( await guardAttachedHead({ ...base, expectedHeadSha: undefined, attached: { sha: OTHER, ref: "b" } }), ).toEqual({ outcome: "unverified" }); @@ -292,11 +344,14 @@ describe("guardAttachedHead (before any model call)", () => { ...base, expectedHeadSha: HEAD, attached: { sha: "not-a-sha", ref: "b", source: "workspace-observed" }, + reprovision: async () => ({ sha: undefined, ref: "b", source: "workspace-observed" }), }); expect(unreadable.outcome).toBe("refused"); if (unreadable.outcome === "refused") { - expect(unreadable.reply).toContain("workspace-observed HEAD for b could not be read"); - expect(unreadable.reply).toContain("This is a bug: the infrastructure failure was not retried automatically"); + expect(unreadable.reply).toContain( + "workspace-observed HEAD for b could not be read after one automatic reprovision", + ); + expect(unreadable.reply).not.toContain("This is a bug"); expect(unreadable.reply).toContain("not a finding, and nothing was posted to GitHub"); } expect(fetchPrHead).not.toHaveBeenCalled(); diff --git a/src/core/reviewRound.ts b/src/core/reviewRound.ts index aabc546ae..5ba3282d6 100644 --- a/src/core/reviewRound.ts +++ b/src/core/reviewRound.ts @@ -275,14 +275,14 @@ export type AttachHeadGuard = | { outcome: "refused"; reply: string }; /** - * Compare the workspace head with the PR head the round resolved, BEFORE any - * model call, on every backend. Equal → verified. Different well-formed shas - * get one current-head lookup so an attach that raced a push may adopt the - * current head; otherwise the run is refused and its workspace released. An - * unreadable workspace head is refused too: without proof that the checkout is - * the PR head, an infrastructure failure must never become a review finding or - * a GitHub post. Only an invalid expected head remains `unverified`; the - * earlier PR-head gate owns that refusal. + * Compare the checked-out workspace head with the PR head the round will + * review, BEFORE any model call, on every backend. Equal → verified. Different + * well-formed shas get one current-head lookup so an attach that raced a push + * may adopt the current head. Every other mismatch (and an unreadable first + * observation) gets one reprovision at the expected reviewed head and one + * re-check; only that failed recovery is refused. The refusal reports the two + * facts the guard established and no inferred cause. Only an invalid expected + * head remains `unverified`; the earlier PR-head gate owns that refusal. */ export async function guardAttachedHead(input: { pr: { repo: string; number: number }; @@ -297,45 +297,95 @@ export async function guardAttachedHead(input: { /** Named in the refusal when the attach answered no ref. */ fallbackRef: string | undefined; fetchPrHead: FetchPrHead; + /** Recreate the review checkout at the expected head and report its HEAD. */ + reprovision: (expectedHeadSha: string) => Promise<{ + sha: string | undefined; + ref?: string | undefined; + source?: "resident binding" | "workspace-observed"; + }>; logKey: string; }): Promise { const expected = normalizeHead(input.expectedHeadSha); if (!expected) return { outcome: "unverified" }; - const attached = normalizeHead(input.attached.sha); - const source = input.attached.source ?? "workspace-observed"; const where = `${input.pr.repo}#${input.pr.number}`; - const branch = input.attached.ref ?? input.fallbackRef; - const namedSource = - source === "resident binding" + type ObservedHead = { + sha: string | undefined; + ref?: string | undefined; + source?: "resident binding" | "workspace-observed"; + }; + const namedSource = (observed: ObservedHead): string => { + const source = observed.source ?? "workspace-observed"; + const branch = observed.ref ?? input.fallbackRef; + return source === "resident binding" ? branch ? `${source} for ${branch}` : source : branch ? `${source} HEAD for ${branch}` : `${source} HEAD`; - if (!attached) { - console.log(`[review] ${input.logKey} not started: ${namedSource} unreadable, PR head ${expected} (${where})`); + }; + + const first = normalizeHead(input.attached.sha); + if (first && sameCommit(expected, first)) return { outcome: "verified" }; + if (first) { + const current = normalizeHead(await input.fetchPrHead(input.pr).catch(() => undefined)); + if (current && sameCommit(first, current)) { + console.log( + `[review] ${input.logKey} PR head moved since resolution: ${expected} → ${current}; the ${namedSource(input.attached)} is at the current head — reviewing it (${where})`, + ); + return { outcome: "adopted", headSha: current }; + } + } + + let retried: ObservedHead; + let reprovisionFailed = false; + try { + retried = await input.reprovision(expected); + } catch { + reprovisionFailed = true; + retried = { sha: undefined }; + } + const checked = normalizeHead(retried.sha); + if (checked && sameCommit(expected, checked)) { + console.log(`[review] ${input.logKey} workspace reprovisioned at reviewed head ${expected} (${where})`); + return { outcome: "verified" }; + } + + const initial = first + ? `the initial ${namedSource(input.attached)} was at ${first}` + : `the initial ${namedSource(input.attached)} could not be read`; + if (reprovisionFailed) { + console.log( + `[review] ${input.logKey} not started: ${initial}; reprovision failed before retry HEAD observation, expected reviewed head ${expected} (${where})`, + ); return { outcome: "refused", reply: - `🔀 Review of ${where} not started: ${namedSource} could not be read, so the workspace cannot be verified against PR head ${expected}. ` + - `This is a bug: the infrastructure failure was not retried automatically. It is not a finding, and nothing was posted to GitHub.`, + `🔀 Review of ${where} not started: ${initial}, and the automatic reprovision failed before a retry HEAD could be observed; the expected reviewed head is ${expected}. ` + + `It is not a finding, and nothing was posted to GitHub.`, }; } - if (sameCommit(expected, attached)) return { outcome: "verified" }; - const current = normalizeHead(await input.fetchPrHead(input.pr).catch(() => undefined)); - if (current && sameCommit(attached, current)) { + + const source = namedSource(retried); + if (!checked) { console.log( - `[review] ${input.logKey} PR head moved since resolution: ${expected} → ${current}; the ${namedSource} is at the current head — reviewing it (${where})`, + `[review] ${input.logKey} not started: ${initial}; ${source} unreadable after reprovision, expected reviewed head ${expected} (${where})`, ); - return { outcome: "adopted", headSha: current }; + return { + outcome: "refused", + reply: + `🔀 Review of ${where} not started: ${initial}, and the ${source} could not be read after one automatic reprovision, so it could not be verified; the expected reviewed head is ${expected}. ` + + `It is not a finding, and nothing was posted to GitHub.`, + }; } - console.log(`[review] ${input.logKey} not started: ${namedSource} ${attached}, PR head ${expected} (${where})`); + console.log( + `[review] ${input.logKey} not started: ${source} ${checked} after reprovision, expected reviewed head ${expected} (${where})`, + ); return { outcome: "refused", reply: - `🔀 Review of ${where} not started: the ${namedSource} is at ${attached}, but the PR head is ${expected} — the branch moved while the workspace was being prepared (a push or force-push). ` + - `This is a bug: the workspace was not reprovisioned automatically at the new head. It is not a finding, and nothing was posted to GitHub.`, + `🔀 Review of ${where} not started: after one automatic reprovision, the ${source} is at ${checked}, but the expected reviewed head is ${expected}. ` + + `It is not a finding, and nothing was posted to GitHub.`, }; } diff --git a/src/execution/factory.ts b/src/execution/factory.ts index 75ee049d7..3f03824f3 100644 --- a/src/execution/factory.ts +++ b/src/execution/factory.ts @@ -314,9 +314,10 @@ export interface ExecutorSelection { trace?: ResidentStep[]; /** The resident's own total for the attach, for the clock-skew attr. */ attachMs?: number; - /** The resident's attach answer (ref, sha, worktree path) on the resident - * path — the dispatcher names the path to the model and checks the sha - * against the PR head before a review runs. Unset on every other path. */ + /** The resident's attach answer (ref, advisory sha, worktree path) on the + * resident path — the dispatcher names the path to the model and observes + * the checkout HEAD through the executor before a review runs. Unset on + * every other path. */ binding?: ResidentBinding; /** The drain's share of a failed attach's wait (issue 2101): on the sandbox * fallback after a drained fleet refused the run, so the dispatcher still