From 5114ac1ab6459e54215ce4207d79b5990c2b76f5 Mon Sep 17 00:00:00 2001 From: usirin Date: Wed, 16 Sep 2026 18:06:20 -0700 Subject: [PATCH 1/2] feat(lane): retrigger child PRs after an epic assembly push (#8880) --- .../fabrika/skills/operate/SKILL.md | 30 ++ packages/fabrika-cli/docs/verb-reference.md | 1 + packages/fabrika-cli/src/io/pulls.ts | 147 ++++++++++ packages/fabrika-cli/src/lane/command.ts | 27 ++ .../fabrika-cli/src/lane/retrigger-verb.ts | 259 ++++++++++++++++++ .../src/lane/retrigger-verb.unit.test.ts | 167 +++++++++++ 6 files changed, 631 insertions(+) create mode 100644 packages/fabrika-cli/src/lane/retrigger-verb.ts create mode 100644 packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts diff --git a/claude-plugins/fabrika/skills/operate/SKILL.md b/claude-plugins/fabrika/skills/operate/SKILL.md index d02c1bd9c..f3323ff1f 100644 --- a/claude-plugins/fabrika/skills/operate/SKILL.md +++ b/claude-plugins/fabrika/skills/operate/SKILL.md @@ -595,6 +595,36 @@ drifted back into the driver's checkout is caught at the one step every publicat rather than after the fact. The remedy is never a flag: place the run's worktree with `lane assembly` and push from there. +**A push of the assembly branch leaves every open child PR on it reporting checks over the old +base, so retrigger them in the same breath.** GitHub rebuilds `refs/pull//merge` when a base +moves — a merge ref fetched over a PR whose head had not been pushed for eleven days carried that +morning's trunk tip as its first parent — but it emits no `pull_request` event for a base push: +the event fires on `opened`, `synchronize` and `reopened`, and a base push is none of them. So +nothing schedules a run, and the red the child inherited from the base you just fixed stays on the +PR until something moves its head: + +```bash +node packages/fabrika-cli/src/bin.ts lane retrigger $lane_key +``` + +It sweeps every OPEN pull request based on `epic/` and moves the head of each one that is behind +the branch, through GitHub's own branch update — a merge of the base into the head branch, which is +a `synchronize`, so the run is scheduled against a merge ref computed now. It is safe to run after +every push: a child whose head already carries the base tip is read, reported `current` and never +written to, so a second call schedules nothing. `RETRIGGER-VERDICT: NONE` says no open PR sits on +the branch at all, which is the ordinary answer under this shape — the run opens one PR and its +children open none — and it costs one board read. + +**Never close and reopen a PR to force this.** A reopen does rebuild the merge ref, and it also +tears the PR's preview stage down mid-deploy — which is the reason it is forbidden rather than +merely discouraged, and `lane retrigger`'s own reference row carries the report that fallout was +filed under. It raced the rebuild the one time it was used, so the guard re-ran against the stale +base anyway and took two rounds. Re-running the workflow is no route either: a re-run replays the original event's +`GITHUB_SHA` and `GITHUB_REF`, which is the stale merge commit — the very red you are clearing. +Exit `42` is a child the assembly branch does not merge into, and that is a repair round on that +child rather than anything to retry here; exit `8` says an update was accepted and the head had not +moved inside its window, so re-read before writing again. + **The first of those pushes also opens the run's one PR**, as a draft — a draft carries the CI signal and the board's view of the run without inviting a review the machine has not asked for. Open it yourself; no shell owns this branch. Its body carries three diff --git a/packages/fabrika-cli/docs/verb-reference.md b/packages/fabrika-cli/docs/verb-reference.md index 155752ae6..fae1fc8ca 100644 --- a/packages/fabrika-cli/docs/verb-reference.md +++ b/packages/fabrika-cli/docs/verb-reference.md @@ -710,6 +710,7 @@ snapshot. Lane state is local and never committed. | `lane refresh` | the trunk merged into an epic run's assembly branch, in the worktree `lane assembly` placed — nothing else in this package touches trunk after the first cut, so the branch drifts behind `main` while the children build and the queue ejects the tail for it. Refuse a dirty seat, `git fetch origin`, resolve `--base` to a commit, answer `CURRENT` when the branch already carries it, else `git merge --no-ff` and re-read HEAD; on exit 0 the last stdout line is `REFRESH-VERDICT: MERGED`, `CURRENT` or `DECLINED` and the line above it the head. A conflict aborts, resets through `ORIG_HEAD` and **proves** the reset by re-reading HEAD, and names `--cause assembly-conflict` as the park to record; a reset that will not take is `8`, never the clean `42`. Two callers reach it automatically and each is gated by its own `assemblyRefresh` arm, both shipped `off`: `--on-review`, typed by the driver on the tail's way into review (`onReview`), and the pre-dispatch call `lane dispatch` makes itself before it cuts a child's worktree off the branch (`onDispatch`), which is never typed. Both arms are read from the `.fabrika.jsonc` of the repository that OWNS the cwd, so a linked worktree reads the primary checkout's copy rather than its own tracked one (#9159). A driver typing the verb is never gated. It pushes nothing and records nothing | | `lane dispatch` | run one active task through Codex in a verified worktree; [contract](./codex-dispatch.md). On an epic run's child it first refreshes the assembly branch the worktree is about to be cut from — `lane refresh`'s own code, before the brief is emitted, under `assemblyRefresh.onDispatch`, read from the `.fabrika.jsonc` of the repository that OWNS the cwd while the worktree spawning below keeps reading the cwd itself. That gate refuses only `11`, a repository identity it cannot read; a cwd in no repository at all is not a refusal there and reads the shipped arm at itself. The `39` this verb still exits is the lanes-root resolution ahead of every read, on either of its two arms: `--root` absent and no repository derivable off the cwd, or a present `--root` that is relative and a cwd that itself holds neither `.fabrika` nor `.git`. A conflict there is `42` with the branch proven back where it stood and no worktree created: record the park it names rather than dispatching over the unrefreshed branch | | `lane assembly` / `push` | an epic run's assembly worktree, and its published branch. Every resume fetches and asks whether `origin/HEAD` already carries the content of the branch it found — the state a multi-phase epic lands in the moment it ships an intermediate tail. The read is squash-aware, because every landing here is a squash and ancestry alone answers "not contained" for every branch there is: ancestry is the fast path, and what survives it is settled by cumulative patch id — the branch's net diff against the trunk, matched against the patches the trunk took since their merge base, limited to the paths the branch touches. A contained branch is re-cut off the trunk in one `worktree add --no-track -B` and the note says which proof opened it; its seat is dropped first without `--force`, so git refusing a dirty tree keeps unlanded bytes out of the re-cut. Anything less than containment resumes as it always did, no board is read, and an unreadable answer is `11` | +| `lane retrigger` | fresh checks scheduled on every OPEN pull request based on an epic run's assembly branch, after `lane push` moved it. The platform was read rather than assumed: GitHub DOES recompute `refs/pull//merge` when a base moves — PR #8063's merge ref carried that morning's trunk tip as its first parent while its head had not been pushed for eleven days — but a base push emits no `pull_request` event (the event fires on `opened`, `synchronize`, `reopened`), so nothing schedules a run and every child keeps reporting checks over the tree its base held before the push; #8063's check runs were all stamped the day its head was pushed, hundreds of trunk commits ago. A re-run cannot serve, because it replays the original event's `GITHUB_SHA` and `GITHUB_REF`, which is the stale merge commit, and close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy ([#8881](https://github.com/kamp-us/phoenix/issues/8881)). So the head is moved instead, through `PUT /pulls/{n}/update-branch`: the base is merged into the head branch, which IS a `synchronize`, so the run is scheduled against a merge ref computed now, with nothing closed, nothing force-pushed and no commit rewritten. Idempotent by construction — a child whose head already carries the base tip is read, reported `current` and never written to, so a second call writes nothing — and the staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so a head a sibling moved first refuses the write rather than misaddressing it; the 422 that refusal shares with a conflict is split by re-reading the head, and a head that moved is reported as the move it was going to buy. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `CURRENT` (children exist, all already carried the base) or `NONE` (no open pull request on the branch), and the lines above it one row per child. `8` an accepted update whose head did not move inside its 60s window, or a read-back that failed after a write — UNKNOWN, so re-read before writing again; `11` the list or a comparison unread before anything was written, never an empty sweep; `42` the assembly branch does not merge into a head branch, which is that child's repair round and not this verb's. It touches no working tree, pushes nothing and records nothing | | `lane assembly-pr` | the assembly PR's title and `## About this epic` section, derived from the epic issue — `--field title` prints `feat(epic): `, whose conventional type is `build/pr-title.ts`'s map rather than a second copy of the release-please rule, and `--field about` prints the section derived from the epic's `## Pitch` **Problem** paragraph — a closing keyword swapped for a word GitHub's documented keyword list does not carry (the reference left as written), cut to its opening sentences under a word budget with `[…]` marking what a triage-length Problem left behind, and lifted as a block quote so a label or a control-plane phrase in the epic's own sentence is reproduced rather than reworded, which is the shape `build pr`'s body guard exempts. The assembled section is then re-read through that guard's own predicates so it cannot be one it refuses. Bare values, one per call, because the operate skill's `gh pr create` fence interpolates them and derives nothing itself. An unpitched epic is an ANSWER — empty stdout, the reason on stderr — so a run still publishes its PR with no section. It opens and edits no pull request | | `lane assembly-body` | the guard the epic run's single assembly PR body passes through on its way to `gh pr create` — the body arrives on STDIN and leaves on stdout unchanged when a closing keyword aims at the epic — the same bytes, plus a trailing newline when the body lacked one — so the fence pipes rather than deriving. A body reaching the epic through `Part of #`, or closing only its landed children, is refused on `58` with nothing printed, and a pipe carrying nothing opens no PR: one epic run is one branch and one PR, so a tail that does not close its epic folds the lane to `shipped` and then `complete` over an epic the board still calls open (ADR 0382). The link reader is `issueRefsOf`, the very one `lane/closure.ts` judges the merged PR with, so this refuses exactly the bodies that reader would later call partial or leave unreadable and the two seams cannot disagree. Membership, not first match — a tail carries one closing keyword per landed child, and those are not judged. Reads no board and takes no `--repo`: the number's epic-ness was established one command earlier by `lane assembly-pr`'s `56`. It opens, edits and reads back no pull request | | `lane integrate` | one reviewed child merged into that worktree, its dependencies reconciled from the merged lockfile, then judged by the repo's `codeValidators` — last stdout line on exit 0 is `INTEGRATE-VERDICT: MERGED`, the line above it the merged head; every refusal below the merge resets the branch and pushes nothing. Under `assemblyReplay.onCollision` (shipped `off`) a colliding child is replayed onto the tip instead of stopping the run: the plain keep-both hunks are kept both ways, the child's own branch is moved onto the replayed range so its next integrate is up to date rather than a second collision, and that range is merged `--no-ff` like any other landing; the verdict is `INTEGRATE-VERDICT: REPLAYED` over a machinery event naming the moved range the child owes one review round over. A hunk that is not a plain keep-both restores, proves the reset, and parks on `--cause replay-conflict`; a child branch that will not move is exit `54` with nothing merged, and a working tree standing on it is the usual reason. A refusal below a replayed merge puts the child branch back on the revision its reviewer graded as well as resetting the assembly branch, so a replay the verb did not keep moves no range — and a branch that will not go back is `8`, never the red | diff --git a/packages/fabrika-cli/src/io/pulls.ts b/packages/fabrika-cli/src/io/pulls.ts index 400dc31c4..5b764264f 100644 --- a/packages/fabrika-cli/src/io/pulls.ts +++ b/packages/fabrika-cli/src/io/pulls.ts @@ -595,3 +595,150 @@ export const pullsForBranch = ( return ok(out); }), ); + +/** + * One open pull request sitting on a base branch: exactly what judging its staleness and moving it + * needs, which is the number to address and the head to compare and to guard the write with. + */ +export interface BasePull { + readonly number: number; + readonly headSha: string; +} + +/** + * Every OPEN pull request whose base ref is `base`, paged. + * + * The mirror of {@link pullsForBranch}, which asks the other question: that one takes a head and + * reads any state, because "what pull request does this work sit on" is often about a closed one. + * This one takes a base and reads open only — a closed pull request schedules nothing and a merged + * one has nothing left to run, so widening the state would hand a caller rows it must drop again. + */ +export const openPullsForBase = ( + repo: string, + base: string, +): Shell>> => + authed((token) => + Effect.gen(function* () { + const page = yield* pagedWithLinkProof( + token, + `repos/${repo}/pulls?state=open&base=${encodeURIComponent(base)}`, + ); + if (page._tag === "Failure") return page; + if (!page.value.exhausted) { + return fail(`the open pull request list for base \`${base}\` was not read to its end`); + } + const out: BasePull[] = []; + for (const value of page.value.entries) { + const headNode = isRecord(value) ? value.head : null; + const sha = isRecord(headNode) && typeof headNode.sha === "string" ? headNode.sha : null; + if (!isRecord(value) || typeof value.number !== "number" || sha === null) { + return fail("GitHub answered 200 but one entry is not a pull request"); + } + out.push({number: value.number, headSha: sha}); + } + return ok(out); + }), + ); + +/** How a head stands to a base right now, in the platform's vocabulary and its own count. */ +export interface BaseStanding { + readonly status: CompareStatus; + /** Commits the base holds and the head does not — what makes the head's last CI run stale. */ + readonly behindBy: number; +} + +/** + * Where a pull request's head stands against its base, with no file list in the answer. + * + * Separate from {@link compareFiles} because it asks a different question and pays a different + * price: a staleness read wants the envelope's `status` and `behind_by` and nothing else, so it + * takes `per_page=1` rather than walking up to {@link COMPARE_FILE_CAP} entries per pull request. + * Both fields sit on the envelope and do not page, so the narrowed page changes no answer here. + * + * The pull request record's own `base.sha` cannot serve: the platform freezes it at the commit the + * PR was opened against, so a PR whose base has since moved a hundred commits still reads its + * original sha there. This comparison is the read that answers against the base as it stands now. + */ +export const compareStanding = ( + repo: string, + base: string, + head: string, +): Shell> => + authed((token) => + restCall(token, { + method: "GET", + path: `repos/${repo}/compare/${base}...${head}?per_page=1`, + }).pipe( + Effect.map((outcome) => + attemptOf(outcome, (body) => { + if (!isRecord(body)) { + return fail("GitHub answered 200 but its output is not a comparison"); + } + if (typeof body.status !== "string" || !COMPARE_STATUSES.includes(body.status)) { + return fail( + "GitHub answered 200 but its comparison declares no status, so where the head stands against the base is unknown", + ); + } + if (typeof body.behind_by !== "number") { + return fail( + "GitHub answered 200 but its comparison declares no behind_by, so how far the head trails the base is unknown", + ); + } + return ok({status: body.status as CompareStatus, behindBy: body.behind_by}); + }), + ), + ), + ); + +/** + * What the platform did with a branch-update request: accepted it, declined it, or never answered. + * + * `Accepted` is the documented 202 and it is not a landing — the endpoint serves "Updating pull + * request branch." and performs the merge asynchronously, so a caller proves the merge by watching + * the head move, never by reading this tag. `Declined` is the 422 the endpoint documents for every + * validation failure it has: a conflict and a stale `expected_head_sha` share that one status and + * the page names no field that tells them apart, so the reason travels verbatim and the split is + * the caller's to prove by re-reading the head ([REST, "Update a pull request + * branch"](https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#update-a-pull-request-branch)). + */ +export type BranchUpdate = + | {readonly _tag: "Accepted"} + | {readonly _tag: "Declined"; readonly reason: string} + | {readonly _tag: "Unreadable"; readonly reason: string}; + +/** + * Merge a pull request's base into its head branch through the platform's own update endpoint. + * + * `expectedHeadSha` is the concurrency guard that endpoint documents: the update is refused rather + * than applied when the head is no longer the commit the caller read. Passing it keeps the write + * addressed to the pull request the caller judged stale, instead of to whatever landed since. + */ +export const updatePullBranch = ( + repo: string, + pr: number, + expectedHeadSha: string, +): Shell => + Effect.map( + authed((token) => + Effect.map( + restCall(token, { + method: "PUT", + path: `repos/${repo}/pulls/${pr}/update-branch`, + body: {expected_head_sha: expectedHeadSha}, + }), + (outcome): Attempt => { + if (outcome._tag === "Unreachable") { + return ok({_tag: "Unreadable", reason: outcome.reason}); + } + if (outcome.status === 202) return ok({_tag: "Accepted"}); + return ok( + outcome.status === 422 + ? {_tag: "Declined", reason: refusalText(outcome)} + : {_tag: "Unreadable", reason: refusalText(outcome)}, + ); + }, + ), + ), + (attempt): BranchUpdate => + attempt._tag === "Failure" ? {_tag: "Unreadable", reason: attempt.reason} : attempt.value, + ); diff --git a/packages/fabrika-cli/src/lane/command.ts b/packages/fabrika-cli/src/lane/command.ts index 0aeab6922..6d9a9c62a 100644 --- a/packages/fabrika-cli/src/lane/command.ts +++ b/packages/fabrika-cli/src/lane/command.ts @@ -73,6 +73,7 @@ import {DEFAULT_TRUNK_REF, runRefresh} from "./refresh-verb.ts"; import {keyRefusal} from "./refusals.ts"; import {classesForEvent, PARK_CAUSE_TOKENS} from "./report.ts"; import {runReport} from "./report-verb.ts"; +import {runRetrigger} from "./retrigger-verb.ts"; import {runSeats} from "./seats-verb.ts"; import {boardReaders, runSettle} from "./settle-verb.ts"; import {BUILD_CLAIM_BUDGET_MINUTES, DISPATCH_BUDGET, SHELL_BUDGETS} from "./shell-budget.ts"; @@ -852,6 +853,31 @@ const refresh = leafCommand( ), ); +const retrigger = leafCommand( + "retrigger", + { + epic: Argument.integer("epic").pipe( + Argument.withDescription("the epic issue whose run owns the assembly branch"), + ), + repo: Flag.string("repo").pipe( + Flag.optional, + Flag.withDescription( + "the target owner/name the sweep reads against (default: $CLAUDE_PIPELINE_REPO, else $GITHUB_REPOSITORY, else the origin remote)", + ), + ), + }, + Effect.fn(function* ({epic, repo}) { + yield* emit(yield* runRetrigger({epic, repo: Option.getOrNull(repo), env: process.env})); + }), +).pipe( + Command.withShortDescription( + "Schedule fresh checks on the children an assembly push left stale.", + ), + Command.withDescription( + "Schedule a fresh CI run on every OPEN pull request based on an epic run's assembly branch — `epic/`, derived from the epic number and never taken from the caller — after `lane push` moved that branch. GitHub recomputes `refs/pull//merge` when a base moves, but emits no pull_request event for a base push (the event fires on opened, synchronize and reopened), so nothing schedules a run and every child keeps reporting checks over the tree its base had before the push. A workflow re-run cannot serve: it replays the original event's GITHUB_SHA and GITHUB_REF, which is the stale merge commit. Close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy. So the head is moved instead, through GitHub's own `PUT /pulls/{n}/update-branch`, which merges the base into the head branch and is a synchronize: a run is scheduled against a merge ref computed now, nothing is closed, nothing is force-pushed and no commit is rewritten. Idempotent by construction: a child whose head already contains the base tip is read, reported `current` and never written to, so a second call right after a first writes nothing. The staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so it is refused rather than misaddressed when a sibling moved the head first. Nothing is pushed, no working tree is touched and no lane log is written. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `RETRIGGER-VERDICT: CURRENT` (children exist and all already carried the base) or `RETRIGGER-VERDICT: NONE` (no open pull request is based on the branch), and the lines above it are one row per child: `# current`, or `# -> `. Exits 8 (an update was accepted and the head did not move inside its 60s window, or a read-back after a write failed — a merge may still be in flight, so re-read before writing again), 11 (the pull request list or a comparison could not be read before anything was written — UNKNOWN, never an empty sweep), 42 (the assembly branch does not merge into one or more of the head branches, so those children need a repair round before their checks can run at all). Example: fabrika lane retrigger 8716", + ), +); + const pushLane = leafCommand( "push", { @@ -1579,6 +1605,7 @@ export const laneCommand = Command.make("lane").pipe( integrate, refresh, pushLane, + retrigger, stale, seats, migrate, diff --git a/packages/fabrika-cli/src/lane/retrigger-verb.ts b/packages/fabrika-cli/src/lane/retrigger-verb.ts new file mode 100644 index 000000000..50847e2e0 --- /dev/null +++ b/packages/fabrika-cli/src/lane/retrigger-verb.ts @@ -0,0 +1,259 @@ +/** + * `lane retrigger` — schedule a fresh CI run on every open pull request sitting on an epic run's + * assembly branch, after that branch has moved. + * + * The platform behaviour this exists for was read against a live pull request rather than assumed. + * GitHub **does** recompute `refs/pull//merge` when the base moves: one open pull request's merge + * ref, fetched while its head had not been pushed for eleven days, carried the trunk tip of that + * morning as its first parent. What the base push does not do is emit any event — `pull_request` + * fires on + * `opened`, `synchronize` and `reopened`, and a base push is none of them + * ([GitHub, "Events that trigger + * workflows"](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request)) + * — so nothing schedules a run and the checks on that PR keep reporting a tree from before the base + * moved. The same read showed it: every check run on that head was stamped the day the head was + * pushed, with hundreds of trunk commits landed since. + * + * That rules out the two cheap levers. Re-requesting the check suite or re-running the workflow + * replays the original event: "The workflow will also use the same `GITHUB_SHA` (commit SHA) and + * `GITHUB_REF` (git ref) of the original event that triggered the workflow run" + * ([GitHub, "Re-run workflows and + * jobs"](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs)), + * which is the stale merge commit — the very red the driver is trying to clear. And close/reopen, + * the hand fix this verb retires, tears the pull request's preview stage down mid-deploy. + * + * So the head has to move, and the smallest honest move is the platform's own branch update: it + * merges the base into the head branch, which is a `synchronize`, which schedules a run against a + * merge ref computed now. Nothing is closed, nothing is force-pushed and no commit of the child's + * is rewritten. + * + * **Idempotent by construction.** A child whose head already contains the base tip is *not* stale — + * its last run judged the tree the merge ref holds today — so it is read, reported `current` and + * never written to. The staleness question is asked of the platform's own comparison, so a second + * call right after a first one writes nothing. + * + * It touches no working tree and no lane record: the branch name is derived from the epic number + * and everything else is the board's. Publishing the assembly branch stays `lane push`'s, and + * recording anything stays the driver's. + * + * @ruling https://github.com/kamp-us/phoenix/issues/8880 + */ +import {Effect} from "effect"; +import type {ChildProcessSpawner} from "effect/unstable/process"; +import {badNumber, resolveTargetRepo} from "../build/target.ts"; +import type {Shell} from "../io/git.ts"; +import { + type BasePull, + compareStanding, + getPullRequest, + openPullsForBase, + updatePullBranch, +} from "../io/pulls.ts"; +import {pollWaits} from "../ship/mergeability.ts"; +import {answer, refuse, type VerbOutcome} from "../verb.ts"; +import {epicBranch} from "../wire/lane-brief.ts"; +import {APPEND_UNKNOWN, LANE_UNREADABLE, MERGE_CONFLICT} from "./codes.ts"; + +const VERB = "fabrika lane retrigger"; + +/** The whole wall-clock one accepted update gets to move the head before it is called UNKNOWN. */ +export const RETRIGGER_WINDOW_SECONDS = 60; + +/** How many characters of a sha a report prints — enough to identify, short enough to scan. */ +const SHORT = 8; + +const short = (sha: string): string => sha.slice(0, SHORT); + +/** + * What became of one child, as four facts rather than a boolean. + * + * `Current` and `Moved` are the two answers; `Conflicted` and `Unknown` are the two ways a child + * ends a sweep unretriggered, and they are kept apart because their remedies are opposite — a + * conflict is a repair round on that child, and an unknown is a re-read before anything else is + * touched. + */ +type Outcome = + | {readonly _tag: "Current"; readonly pr: number} + | {readonly _tag: "Moved"; readonly pr: number; readonly from: string; readonly to: string} + | {readonly _tag: "Conflicted"; readonly pr: number; readonly reason: string} + | { + readonly _tag: "Unknown"; + readonly pr: number; + readonly reason: string; + readonly wrote: boolean; + }; + +/** + * Watch one child's head until it leaves `from`, within the window. + * + * The update endpoint answers 202 and merges asynchronously, so the accepted status is a receipt + * and not a landing. A head that has not moved when the window is spent is UNKNOWN rather than a + * failure: the merge may still be in flight, and reporting it as not done would send a driver to + * write again over a write that is already running. + * + * The first read costs no wait, because a small merge is routinely done before the first one would + * elapse; the waits are {@link pollWaits}' schedule rather than a second one written here. + */ +const awaitMovedHead = ( + repo: string, + pr: number, + from: string, + windowSeconds: number, +): Shell => + Effect.gen(function* () { + for (const wait of [0, ...pollWaits(windowSeconds)]) { + if (wait > 0) yield* Effect.sleep(`${wait} seconds`); + const read = yield* getPullRequest(repo, pr); + if (read._tag === "Absent") { + return { + _tag: "Unknown" as const, + pr, + reason: "the pull request is no longer there to read back", + wrote: true, + }; + } + if (read._tag === "Unknown") + return {_tag: "Unknown" as const, pr, reason: read.reason, wrote: true}; + if (read.value.headSha !== from) { + return {_tag: "Moved" as const, pr, from, to: read.value.headSha}; + } + } + return { + _tag: "Unknown" as const, + pr, + reason: `the update was accepted and the head was still ${short(from)} after ${windowSeconds}s`, + wrote: true, + }; + }); + +/** + * One child, from the staleness read through to the head it ends on. + * + * The `Declined` arm re-reads the head because the platform spends one status on two facts: a + * conflict and a head that moved since it was read both answer 422. A head that is no longer the + * commit this sweep sent is the second of those, and it is not a failure at all — whatever moved it + * emitted the `synchronize` this verb was going to buy. + */ +const retriggerOne = ( + repo: string, + base: string, + child: BasePull, + windowSeconds: number, +): Shell => + Effect.gen(function* () { + const standing = yield* compareStanding(repo, base, child.headSha); + if (standing._tag === "Failure") { + return {_tag: "Unknown" as const, pr: child.number, reason: standing.reason, wrote: false}; + } + if (standing.value.status === "identical" || standing.value.status === "ahead") { + return {_tag: "Current" as const, pr: child.number}; + } + + const update = yield* updatePullBranch(repo, child.number, child.headSha); + if (update._tag === "Unreadable") { + return {_tag: "Unknown" as const, pr: child.number, reason: update.reason, wrote: true}; + } + if (update._tag === "Declined") { + const read = yield* getPullRequest(repo, child.number); + if (read._tag !== "Present") { + return { + _tag: "Unknown" as const, + pr: child.number, + reason: `${update.reason}, and the head could not be re-read to tell a conflict from a head that moved`, + wrote: false, + }; + } + return read.value.headSha === child.headSha + ? {_tag: "Conflicted" as const, pr: child.number, reason: update.reason} + : { + _tag: "Moved" as const, + pr: child.number, + from: child.headSha, + to: read.value.headSha, + }; + } + return yield* awaitMovedHead(repo, child.number, child.headSha, windowSeconds); + }); + +const row = (outcome: Outcome): string => { + switch (outcome._tag) { + case "Current": + return `#${outcome.pr} current`; + case "Moved": + return `#${outcome.pr} ${short(outcome.from)} -> ${short(outcome.to)}`; + case "Conflicted": + return `#${outcome.pr} conflicted`; + case "Unknown": + return `#${outcome.pr} unknown`; + } +}; + +export interface RetriggerOptions { + readonly epic: number; + readonly repo: string | null; + readonly env: Readonly>; + /** The window one accepted update gets to move the head. Tests pass `0`; the adapter never does. */ + readonly windowSeconds?: number; +} + +export const runRetrigger = ({ + epic, + repo, + env, + windowSeconds = RETRIGGER_WINDOW_SECONDS, +}: RetriggerOptions): Effect.Effect => + Effect.gen(function* () { + const bad = badNumber(VERB, "an issue number", epic); + if (bad !== null) return bad; + + const target = yield* resolveTargetRepo(VERB, repo, env); + if (target._tag === "Refused") return target.outcome; + const base = epicBranch(epic); + + const listed = yield* openPullsForBase(target.repo, base); + if (listed._tag === "Failure") { + return refuse( + LANE_UNREADABLE, + `${VERB}: cannot read the open pull requests based on ${base}: ${listed.reason} — which children are stale is UNKNOWN, so nothing was written.`, + ); + } + if (listed.value.length === 0) { + return answer("RETRIGGER-VERDICT: NONE\n", [ + `${VERB}: no open pull request is based on ${base} — nothing was read further and nothing was written.`, + ]); + } + + const outcomes: Outcome[] = []; + for (const child of listed.value) { + outcomes.push(yield* retriggerOne(target.repo, base, child, windowSeconds)); + } + + const moved = outcomes.filter((outcome) => outcome._tag === "Moved"); + const rows = outcomes.map(row).join("\n"); + const note = `${VERB}: ${moved.length} of ${outcomes.length} open pull request(s) on ${base} were behind it and were updated; the rest already carried it.`; + + const unknown = outcomes.find((outcome) => outcome._tag === "Unknown"); + if (unknown !== undefined) { + return refuse( + unknown.wrote ? APPEND_UNKNOWN : LANE_UNREADABLE, + `${VERB}: #${unknown.pr}: ${unknown.reason} — whether its checks were retriggered is UNKNOWN.`, + [note, ...outcomes.map(row)], + ); + } + + const conflicted = outcomes.filter((outcome) => outcome._tag === "Conflicted"); + if (conflicted.length > 0) { + return refuse( + MERGE_CONFLICT, + `${VERB}: ${base} does not merge into ${conflicted.length} of these head branch(es), so their checks cannot be retriggered without a repair round: ${conflicted + .map((outcome) => `#${outcome.pr} (${outcome.reason})`) + .join("; ")}.`, + [note, ...outcomes.map(row)], + ); + } + + return answer( + `${rows}\nRETRIGGER-VERDICT: ${moved.length === 0 ? "CURRENT" : "RETRIGGERED"}\n`, + [note], + ); + }); diff --git a/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts b/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts new file mode 100644 index 000000000..9fc3ca390 --- /dev/null +++ b/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts @@ -0,0 +1,167 @@ +/** + * `lane retrigger` — the staleness read that makes it a no-op, the one write it makes, and the + * seats the three ways a child ends unretriggered take. + */ +import {Effect, Layer} from "effect"; +import {describe, expect, it} from "vitest"; +import {pullPayload, served} from "../build/fixtures.test-support.ts"; +import {fakeHttp, fakeShell, type HttpReply} from "../fakes.test-support.ts"; +import {APPEND_UNKNOWN, LANE_UNREADABLE, MERGE_CONFLICT} from "./codes.ts"; +import {runRetrigger} from "./retrigger-verb.ts"; + +const ENV = {CLAUDE_PIPELINE_REPO: "o/r", GITHUB_TOKEN: "ghp_scripted"} as Record< + string, + string | undefined +>; + +const EPIC = 8716; +const BEFORE = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const AFTER = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + +const LIST = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/pulls\?state=open&base=epic%2F8716/; +const COMPARE = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/compare\/epic\/8716\.\.\./; +const UPDATE = /^PUT https:\/\/api\.github\.com\/repos\/o\/r\/pulls\/8859\/update-branch$/; +const PULL = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/pulls\/8859$/; + +const child = (overrides: Record = {}): Record => ({ + number: 8859, + head: {sha: BEFORE, ref: "build/8766-commands-cannot-emit-76b00cfe"}, + ...overrides, +}); + +const listed = (rows: ReadonlyArray>): HttpReply => served(rows); + +const standing = (status: string, behindBy: number): HttpReply => + served({status, behind_by: behindBy, ahead_by: 1}); + +const ACCEPTED: HttpReply = {status: 202, body: '{"message":"Updating pull request branch."}'}; + +const run = (script: ReadonlyArray) => + Effect.runPromise( + Effect.provide( + runRetrigger({epic: EPIC, repo: null, env: ENV, windowSeconds: 0}), + Layer.mergeAll(fakeShell([]).layer, fakeHttp(script).layer), + ), + ); + +const withRequests = (script: ReadonlyArray) => { + const http = fakeHttp(script); + return Effect.runPromise( + Effect.provide( + runRetrigger({epic: EPIC, repo: null, env: ENV, windowSeconds: 0}), + Layer.mergeAll(fakeShell([]).layer, http.layer), + ), + ).then((outcome) => ({outcome, requests: http.calls, bodies: http.bodies})); +}; + +describe("lane retrigger", () => { + it("answers NONE when no open pull request sits on the assembly branch", async () => { + const out = await run([[LIST, listed([])]]); + + expect(out.code).toBe(0); + expect(out.stdout).toBe("RETRIGGER-VERDICT: NONE\n"); + }); + + it("writes nothing to a child whose head already carries the base", async () => { + const {outcome, requests} = await withRequests([ + [LIST, listed([child()])], + [COMPARE, standing("ahead", 0)], + ]); + + expect(outcome.code).toBe(0); + expect(outcome.stdout).toBe("#8859 current\nRETRIGGER-VERDICT: CURRENT\n"); + expect(requests.some((line) => line.startsWith("PUT"))).toBe(false); + }); + + it("reads the base as it stands, not the pull request's frozen base sha", async () => { + const {requests} = await withRequests([ + [LIST, listed([child()])], + [COMPARE, standing("identical", 0)], + ]); + + expect(requests).toContain( + `GET https://api.github.com/repos/o/r/compare/epic/8716...${BEFORE}?per_page=1`, + ); + }); + + it("updates a behind child through the branch-update endpoint, guarded by its head", async () => { + const {outcome, requests, bodies} = await withRequests([ + [LIST, listed([child()])], + [COMPARE, standing("behind", 3)], + [UPDATE, ACCEPTED], + [PULL, served(pullPayload({number: 8859, head: {sha: AFTER, ref: "build/8766"}}))], + ]); + + expect(outcome.code).toBe(0); + expect(outcome.stdout).toBe("#8859 aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n"); + const update = requests.findIndex((line) => line.startsWith("PUT")); + expect(update).toBeGreaterThan(-1); + expect(JSON.parse(bodies[update] ?? "{}")).toEqual({expected_head_sha: BEFORE}); + }); + + it("updates a diverged child too — it also lacks commits the base holds", async () => { + const out = await run([ + [LIST, listed([child()])], + [COMPARE, standing("diverged", 2)], + [UPDATE, ACCEPTED], + [PULL, served(pullPayload({number: 8859, head: {sha: AFTER, ref: "build/8766"}}))], + ]); + + expect(out.code).toBe(0); + expect(out.stdout).toContain("RETRIGGER-VERDICT: RETRIGGERED"); + }); + + it("is UNKNOWN, never retriggered, when an accepted update leaves the head where it was", async () => { + const out = await run([ + [LIST, listed([child()])], + [COMPARE, standing("behind", 1)], + [UPDATE, ACCEPTED], + [PULL, served(pullPayload({number: 8859, head: {sha: BEFORE, ref: "build/8766"}}))], + ]); + + expect(out.code).toBe(APPEND_UNKNOWN); + expect(out.stdout).toBe(""); + expect(out.stderr.join(" ")).toContain("#8859"); + }); + + it("refuses a child the assembly branch does not merge into, naming GitHub's own reason", async () => { + const out = await run([ + [LIST, listed([child()])], + [COMPARE, standing("behind", 1)], + [UPDATE, {status: 422, body: '{"message":"merge conflict between base and head"}'}], + [PULL, served(pullPayload({number: 8859, head: {sha: BEFORE, ref: "build/8766"}}))], + ]); + + expect(out.code).toBe(MERGE_CONFLICT); + expect(out.stderr.join(" ")).toContain("merge conflict between base and head"); + }); + + it("reads a declined update whose head has moved as the move it was going to buy", async () => { + const out = await run([ + [LIST, listed([child()])], + [COMPARE, standing("behind", 1)], + [UPDATE, {status: 422, body: '{"message":"Validation Failed"}'}], + [PULL, served(pullPayload({number: 8859, head: {sha: AFTER, ref: "build/8766"}}))], + ]); + + expect(out.code).toBe(0); + expect(out.stdout).toBe("#8859 aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n"); + }); + + it("is UNKNOWN, never an empty sweep, when the pull request list cannot be read", async () => { + const out = await run([[LIST, {status: 500, body: '{"message":"boom"}'}]]); + + expect(out.code).toBe(LANE_UNREADABLE); + expect(out.stdout).toBe(""); + }); + + it("writes nothing when the staleness comparison cannot be read", async () => { + const {outcome, requests} = await withRequests([ + [LIST, listed([child()])], + [COMPARE, {status: 502, body: '{"message":"Bad gateway"}'}], + ]); + + expect(outcome.code).toBe(LANE_UNREADABLE); + expect(requests.some((line) => line.startsWith("PUT"))).toBe(false); + }); +}); From a497629133b3e576d9ac3d1d68432144bd8c0edb Mon Sep 17 00:00:00 2001 From: usirin Date: Wed, 16 Sep 2026 18:32:08 -0700 Subject: [PATCH 2/2] fix(lane): retrigger reports behind_by and keeps exit 11's untouched-sweep promise (#8880) --- .../fabrika/skills/operate/SKILL.md | 6 ++- packages/fabrika-cli/docs/verb-reference.md | 2 +- packages/fabrika-cli/src/io/pulls.ts | 8 +++- packages/fabrika-cli/src/lane/command.ts | 2 +- .../fabrika-cli/src/lane/retrigger-verb.ts | 39 ++++++++++++++++--- .../src/lane/retrigger-verb.unit.test.ts | 26 ++++++++++++- 6 files changed, 71 insertions(+), 12 deletions(-) diff --git a/claude-plugins/fabrika/skills/operate/SKILL.md b/claude-plugins/fabrika/skills/operate/SKILL.md index f3323ff1f..2bc7985e3 100644 --- a/claude-plugins/fabrika/skills/operate/SKILL.md +++ b/claude-plugins/fabrika/skills/operate/SKILL.md @@ -622,8 +622,10 @@ filed under. It raced the rebuild the one time it was used, so the guard re-ran base anyway and took two rounds. Re-running the workflow is no route either: a re-run replays the original event's `GITHUB_SHA` and `GITHUB_REF`, which is the stale merge commit — the very red you are clearing. Exit `42` is a child the assembly branch does not merge into, and that is a repair round on that -child rather than anything to retry here; exit `8` says an update was accepted and the head had not -moved inside its window, so re-read before writing again. +child rather than anything to retry here; exit `8` says the sweep has already moved a child's head +and then lost its read — an accepted update whose head had not moved inside its window, or a read +that failed after that write — so re-read before writing again. Exit `11` is the sweep that wrote to +nothing, which is the one you can simply re-run. **The first of those pushes also opens the run's one PR**, as a draft — a draft carries the CI signal and the board's view of the run without inviting a review the machine has not asked for. diff --git a/packages/fabrika-cli/docs/verb-reference.md b/packages/fabrika-cli/docs/verb-reference.md index fae1fc8ca..181ea10a3 100644 --- a/packages/fabrika-cli/docs/verb-reference.md +++ b/packages/fabrika-cli/docs/verb-reference.md @@ -710,7 +710,7 @@ snapshot. Lane state is local and never committed. | `lane refresh` | the trunk merged into an epic run's assembly branch, in the worktree `lane assembly` placed — nothing else in this package touches trunk after the first cut, so the branch drifts behind `main` while the children build and the queue ejects the tail for it. Refuse a dirty seat, `git fetch origin`, resolve `--base` to a commit, answer `CURRENT` when the branch already carries it, else `git merge --no-ff` and re-read HEAD; on exit 0 the last stdout line is `REFRESH-VERDICT: MERGED`, `CURRENT` or `DECLINED` and the line above it the head. A conflict aborts, resets through `ORIG_HEAD` and **proves** the reset by re-reading HEAD, and names `--cause assembly-conflict` as the park to record; a reset that will not take is `8`, never the clean `42`. Two callers reach it automatically and each is gated by its own `assemblyRefresh` arm, both shipped `off`: `--on-review`, typed by the driver on the tail's way into review (`onReview`), and the pre-dispatch call `lane dispatch` makes itself before it cuts a child's worktree off the branch (`onDispatch`), which is never typed. Both arms are read from the `.fabrika.jsonc` of the repository that OWNS the cwd, so a linked worktree reads the primary checkout's copy rather than its own tracked one (#9159). A driver typing the verb is never gated. It pushes nothing and records nothing | | `lane dispatch` | run one active task through Codex in a verified worktree; [contract](./codex-dispatch.md). On an epic run's child it first refreshes the assembly branch the worktree is about to be cut from — `lane refresh`'s own code, before the brief is emitted, under `assemblyRefresh.onDispatch`, read from the `.fabrika.jsonc` of the repository that OWNS the cwd while the worktree spawning below keeps reading the cwd itself. That gate refuses only `11`, a repository identity it cannot read; a cwd in no repository at all is not a refusal there and reads the shipped arm at itself. The `39` this verb still exits is the lanes-root resolution ahead of every read, on either of its two arms: `--root` absent and no repository derivable off the cwd, or a present `--root` that is relative and a cwd that itself holds neither `.fabrika` nor `.git`. A conflict there is `42` with the branch proven back where it stood and no worktree created: record the park it names rather than dispatching over the unrefreshed branch | | `lane assembly` / `push` | an epic run's assembly worktree, and its published branch. Every resume fetches and asks whether `origin/HEAD` already carries the content of the branch it found — the state a multi-phase epic lands in the moment it ships an intermediate tail. The read is squash-aware, because every landing here is a squash and ancestry alone answers "not contained" for every branch there is: ancestry is the fast path, and what survives it is settled by cumulative patch id — the branch's net diff against the trunk, matched against the patches the trunk took since their merge base, limited to the paths the branch touches. A contained branch is re-cut off the trunk in one `worktree add --no-track -B` and the note says which proof opened it; its seat is dropped first without `--force`, so git refusing a dirty tree keeps unlanded bytes out of the re-cut. Anything less than containment resumes as it always did, no board is read, and an unreadable answer is `11` | -| `lane retrigger` | fresh checks scheduled on every OPEN pull request based on an epic run's assembly branch, after `lane push` moved it. The platform was read rather than assumed: GitHub DOES recompute `refs/pull//merge` when a base moves — PR #8063's merge ref carried that morning's trunk tip as its first parent while its head had not been pushed for eleven days — but a base push emits no `pull_request` event (the event fires on `opened`, `synchronize`, `reopened`), so nothing schedules a run and every child keeps reporting checks over the tree its base held before the push; #8063's check runs were all stamped the day its head was pushed, hundreds of trunk commits ago. A re-run cannot serve, because it replays the original event's `GITHUB_SHA` and `GITHUB_REF`, which is the stale merge commit, and close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy ([#8881](https://github.com/kamp-us/phoenix/issues/8881)). So the head is moved instead, through `PUT /pulls/{n}/update-branch`: the base is merged into the head branch, which IS a `synchronize`, so the run is scheduled against a merge ref computed now, with nothing closed, nothing force-pushed and no commit rewritten. Idempotent by construction — a child whose head already carries the base tip is read, reported `current` and never written to, so a second call writes nothing — and the staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so a head a sibling moved first refuses the write rather than misaddressing it; the 422 that refusal shares with a conflict is split by re-reading the head, and a head that moved is reported as the move it was going to buy. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `CURRENT` (children exist, all already carried the base) or `NONE` (no open pull request on the branch), and the lines above it one row per child. `8` an accepted update whose head did not move inside its 60s window, or a read-back that failed after a write — UNKNOWN, so re-read before writing again; `11` the list or a comparison unread before anything was written, never an empty sweep; `42` the assembly branch does not merge into a head branch, which is that child's repair round and not this verb's. It touches no working tree, pushes nothing and records nothing | +| `lane retrigger` | fresh checks scheduled on every OPEN pull request based on an epic run's assembly branch, after `lane push` moved it. The platform was read rather than assumed: GitHub DOES recompute `refs/pull//merge` when a base moves — PR #8063's merge ref carried that morning's trunk tip as its first parent while its head had not been pushed for eleven days — but a base push emits no `pull_request` event (the event fires on `opened`, `synchronize`, `reopened`), so nothing schedules a run and every child keeps reporting checks over the tree its base held before the push; #8063's check runs were all stamped the day its head was pushed, hundreds of trunk commits ago. A re-run cannot serve, because it replays the original event's `GITHUB_SHA` and `GITHUB_REF`, which is the stale merge commit, and close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy ([#8881](https://github.com/kamp-us/phoenix/issues/8881)). So the head is moved instead, through `PUT /pulls/{n}/update-branch`: the base is merged into the head branch, which IS a `synchronize`, so the run is scheduled against a merge ref computed now, with nothing closed, nothing force-pushed and no commit rewritten. Idempotent by construction — a child whose head already carries the base tip is read, reported `current` and never written to, so a second call writes nothing — and the staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so a head a sibling moved first refuses the write rather than misaddressing it; the 422 that refusal shares with a conflict is split by re-reading the head, and a head that moved is reported as the move it was going to buy. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `CURRENT` (children exist, all already carried the base) or `NONE` (no open pull request on the branch), and the lines above it one row per child: `# current`, or `# behind, -> `, the count read from the comparison's own `behind_by` so a driver sees whether the base drifted by one commit or a hundred. `8` an accepted update whose head did not move inside its 60s window, or a read that failed after this sweep had already moved a child's head — UNKNOWN, so re-read before writing again; `11` the list or a comparison unread while this sweep had written to no child, so the sweep is untouched and safe to re-run — never an empty sweep; `42` the assembly branch does not merge into a head branch, which is that child's repair round and not this verb's. It touches no working tree, pushes nothing and records nothing | | `lane assembly-pr` | the assembly PR's title and `## About this epic` section, derived from the epic issue — `--field title` prints `feat(epic): `, whose conventional type is `build/pr-title.ts`'s map rather than a second copy of the release-please rule, and `--field about` prints the section derived from the epic's `## Pitch` **Problem** paragraph — a closing keyword swapped for a word GitHub's documented keyword list does not carry (the reference left as written), cut to its opening sentences under a word budget with `[…]` marking what a triage-length Problem left behind, and lifted as a block quote so a label or a control-plane phrase in the epic's own sentence is reproduced rather than reworded, which is the shape `build pr`'s body guard exempts. The assembled section is then re-read through that guard's own predicates so it cannot be one it refuses. Bare values, one per call, because the operate skill's `gh pr create` fence interpolates them and derives nothing itself. An unpitched epic is an ANSWER — empty stdout, the reason on stderr — so a run still publishes its PR with no section. It opens and edits no pull request | | `lane assembly-body` | the guard the epic run's single assembly PR body passes through on its way to `gh pr create` — the body arrives on STDIN and leaves on stdout unchanged when a closing keyword aims at the epic — the same bytes, plus a trailing newline when the body lacked one — so the fence pipes rather than deriving. A body reaching the epic through `Part of #`, or closing only its landed children, is refused on `58` with nothing printed, and a pipe carrying nothing opens no PR: one epic run is one branch and one PR, so a tail that does not close its epic folds the lane to `shipped` and then `complete` over an epic the board still calls open (ADR 0382). The link reader is `issueRefsOf`, the very one `lane/closure.ts` judges the merged PR with, so this refuses exactly the bodies that reader would later call partial or leave unreadable and the two seams cannot disagree. Membership, not first match — a tail carries one closing keyword per landed child, and those are not judged. Reads no board and takes no `--repo`: the number's epic-ness was established one command earlier by `lane assembly-pr`'s `56`. It opens, edits and reads back no pull request | | `lane integrate` | one reviewed child merged into that worktree, its dependencies reconciled from the merged lockfile, then judged by the repo's `codeValidators` — last stdout line on exit 0 is `INTEGRATE-VERDICT: MERGED`, the line above it the merged head; every refusal below the merge resets the branch and pushes nothing. Under `assemblyReplay.onCollision` (shipped `off`) a colliding child is replayed onto the tip instead of stopping the run: the plain keep-both hunks are kept both ways, the child's own branch is moved onto the replayed range so its next integrate is up to date rather than a second collision, and that range is merged `--no-ff` like any other landing; the verdict is `INTEGRATE-VERDICT: REPLAYED` over a machinery event naming the moved range the child owes one review round over. A hunk that is not a plain keep-both restores, proves the reset, and parks on `--cause replay-conflict`; a child branch that will not move is exit `54` with nothing merged, and a working tree standing on it is the usual reason. A refusal below a replayed merge puts the child branch back on the revision its reviewer graded as well as resetting the assembly branch, so a replay the verb did not keep moves no range — and a branch that will not go back is `8`, never the red | diff --git a/packages/fabrika-cli/src/io/pulls.ts b/packages/fabrika-cli/src/io/pulls.ts index 5b764264f..6aa4a2628 100644 --- a/packages/fabrika-cli/src/io/pulls.ts +++ b/packages/fabrika-cli/src/io/pulls.ts @@ -643,7 +643,13 @@ export const openPullsForBase = ( /** How a head stands to a base right now, in the platform's vocabulary and its own count. */ export interface BaseStanding { readonly status: CompareStatus; - /** Commits the base holds and the head does not — what makes the head's last CI run stale. */ + /** + * Commits the base holds and the head does not — what makes the head's last CI run stale. + * + * Read by `lane retrigger`, which prints it on the row for each child it moved: the status alone + * says a child was behind, and this says by how much, which is what tells a driver whether the + * base drifted by one commit or by a hundred. + */ readonly behindBy: number; } diff --git a/packages/fabrika-cli/src/lane/command.ts b/packages/fabrika-cli/src/lane/command.ts index 6d9a9c62a..8a3010dec 100644 --- a/packages/fabrika-cli/src/lane/command.ts +++ b/packages/fabrika-cli/src/lane/command.ts @@ -874,7 +874,7 @@ const retrigger = leafCommand( "Schedule fresh checks on the children an assembly push left stale.", ), Command.withDescription( - "Schedule a fresh CI run on every OPEN pull request based on an epic run's assembly branch — `epic/`, derived from the epic number and never taken from the caller — after `lane push` moved that branch. GitHub recomputes `refs/pull//merge` when a base moves, but emits no pull_request event for a base push (the event fires on opened, synchronize and reopened), so nothing schedules a run and every child keeps reporting checks over the tree its base had before the push. A workflow re-run cannot serve: it replays the original event's GITHUB_SHA and GITHUB_REF, which is the stale merge commit. Close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy. So the head is moved instead, through GitHub's own `PUT /pulls/{n}/update-branch`, which merges the base into the head branch and is a synchronize: a run is scheduled against a merge ref computed now, nothing is closed, nothing is force-pushed and no commit is rewritten. Idempotent by construction: a child whose head already contains the base tip is read, reported `current` and never written to, so a second call right after a first writes nothing. The staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so it is refused rather than misaddressed when a sibling moved the head first. Nothing is pushed, no working tree is touched and no lane log is written. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `RETRIGGER-VERDICT: CURRENT` (children exist and all already carried the base) or `RETRIGGER-VERDICT: NONE` (no open pull request is based on the branch), and the lines above it are one row per child: `# current`, or `# -> `. Exits 8 (an update was accepted and the head did not move inside its 60s window, or a read-back after a write failed — a merge may still be in flight, so re-read before writing again), 11 (the pull request list or a comparison could not be read before anything was written — UNKNOWN, never an empty sweep), 42 (the assembly branch does not merge into one or more of the head branches, so those children need a repair round before their checks can run at all). Example: fabrika lane retrigger 8716", + "Schedule a fresh CI run on every OPEN pull request based on an epic run's assembly branch — `epic/`, derived from the epic number and never taken from the caller — after `lane push` moved that branch. GitHub recomputes `refs/pull//merge` when a base moves, but emits no pull_request event for a base push (the event fires on opened, synchronize and reopened), so nothing schedules a run and every child keeps reporting checks over the tree its base had before the push. A workflow re-run cannot serve: it replays the original event's GITHUB_SHA and GITHUB_REF, which is the stale merge commit. Close/reopen is forbidden — it tears the pull request's preview stage down mid-deploy. So the head is moved instead, through GitHub's own `PUT /pulls/{n}/update-branch`, which merges the base into the head branch and is a synchronize: a run is scheduled against a merge ref computed now, nothing is closed, nothing is force-pushed and no commit is rewritten. Idempotent by construction: a child whose head already contains the base tip is read, reported `current` and never written to, so a second call right after a first writes nothing. The staleness read is the platform's own comparison against the base as it stands, never the pull request's frozen `base.sha`. Each write carries `expected_head_sha`, so it is refused rather than misaddressed when a sibling moved the head first. Nothing is pushed, no working tree is touched and no lane log is written. On exit 0 the last stdout line is `RETRIGGER-VERDICT: RETRIGGERED`, `RETRIGGER-VERDICT: CURRENT` (children exist and all already carried the base) or `RETRIGGER-VERDICT: NONE` (no open pull request is based on the branch), and the lines above it are one row per child: `# current`, or `# behind, -> `. Exits 8 (an update was accepted and the head did not move inside its 60s window, or a read failed after this sweep had already moved a child's head — a merge may still be in flight, so re-read before writing again), 11 (the pull request list or a comparison could not be read and this sweep had written to no child — UNKNOWN over an untouched sweep, never an empty one), 42 (the assembly branch does not merge into one or more of the head branches, so those children need a repair round before their checks can run at all). Example: fabrika lane retrigger 8716", ), ); diff --git a/packages/fabrika-cli/src/lane/retrigger-verb.ts b/packages/fabrika-cli/src/lane/retrigger-verb.ts index 50847e2e0..d1b57fb59 100644 --- a/packages/fabrika-cli/src/lane/retrigger-verb.ts +++ b/packages/fabrika-cli/src/lane/retrigger-verb.ts @@ -71,10 +71,22 @@ const short = (sha: string): string => sha.slice(0, SHORT); * ends a sweep unretriggered, and they are kept apart because their remedies are opposite — a * conflict is a repair round on that child, and an unknown is a re-read before anything else is * touched. + * + * `Moved` and `Unknown` each carry whether this verb wrote to that child, because the sweep's exit + * code turns on whether *any* child was written to, and a child that ends unknown after a sibling's + * head was already moved is not the untouched sweep exit `11` promises. */ type Outcome = | {readonly _tag: "Current"; readonly pr: number} - | {readonly _tag: "Moved"; readonly pr: number; readonly from: string; readonly to: string} + | { + readonly _tag: "Moved"; + readonly pr: number; + readonly from: string; + readonly to: string; + /** How far the head trailed the base when this sweep read it, before the move. */ + readonly behindBy: number; + readonly wrote: boolean; + } | {readonly _tag: "Conflicted"; readonly pr: number; readonly reason: string} | { readonly _tag: "Unknown"; @@ -83,6 +95,10 @@ type Outcome = readonly wrote: boolean; }; +/** Whether this verb addressed a write to that child — `Current` and `Conflicted` never do. */ +const wroteTo = (outcome: Outcome): boolean => + (outcome._tag === "Moved" || outcome._tag === "Unknown") && outcome.wrote; + /** * Watch one child's head until it leaves `from`, within the window. * @@ -98,6 +114,7 @@ const awaitMovedHead = ( repo: string, pr: number, from: string, + behindBy: number, windowSeconds: number, ): Shell => Effect.gen(function* () { @@ -115,7 +132,7 @@ const awaitMovedHead = ( if (read._tag === "Unknown") return {_tag: "Unknown" as const, pr, reason: read.reason, wrote: true}; if (read.value.headSha !== from) { - return {_tag: "Moved" as const, pr, from, to: read.value.headSha}; + return {_tag: "Moved" as const, pr, from, to: read.value.headSha, behindBy, wrote: true}; } } return { @@ -170,9 +187,17 @@ const retriggerOne = ( pr: child.number, from: child.headSha, to: read.value.headSha, + behindBy: standing.value.behindBy, + wrote: false, }; } - return yield* awaitMovedHead(repo, child.number, child.headSha, windowSeconds); + return yield* awaitMovedHead( + repo, + child.number, + child.headSha, + standing.value.behindBy, + windowSeconds, + ); }); const row = (outcome: Outcome): string => { @@ -180,7 +205,7 @@ const row = (outcome: Outcome): string => { case "Current": return `#${outcome.pr} current`; case "Moved": - return `#${outcome.pr} ${short(outcome.from)} -> ${short(outcome.to)}`; + return `#${outcome.pr} ${outcome.behindBy} behind, ${short(outcome.from)} -> ${short(outcome.to)}`; case "Conflicted": return `#${outcome.pr} conflicted`; case "Unknown": @@ -232,10 +257,14 @@ export const runRetrigger = ({ const rows = outcomes.map(row).join("\n"); const note = `${VERB}: ${moved.length} of ${outcomes.length} open pull request(s) on ${base} were behind it and were updated; the rest already carried it.`; + // The code a driver reads to decide whether a re-run is safe, so it answers over the whole + // sweep rather than over the child that ended unknown: one child read badly after a sibling's + // head was already moved is an unknown with a write behind it, which is exit 8's fact, not + // exit 11's. const unknown = outcomes.find((outcome) => outcome._tag === "Unknown"); if (unknown !== undefined) { return refuse( - unknown.wrote ? APPEND_UNKNOWN : LANE_UNREADABLE, + outcomes.some(wroteTo) ? APPEND_UNKNOWN : LANE_UNREADABLE, `${VERB}: #${unknown.pr}: ${unknown.reason} — whether its checks were retriggered is UNKNOWN.`, [note, ...outcomes.map(row)], ); diff --git a/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts b/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts index 9fc3ca390..622a65605 100644 --- a/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts +++ b/packages/fabrika-cli/src/lane/retrigger-verb.unit.test.ts @@ -17,9 +17,13 @@ const ENV = {CLAUDE_PIPELINE_REPO: "o/r", GITHUB_TOKEN: "ghp_scripted"} as Recor const EPIC = 8716; const BEFORE = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; const AFTER = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; +const SIBLING = "cccccccccccccccccccccccccccccccccccccccc"; const LIST = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/pulls\?state=open&base=epic%2F8716/; const COMPARE = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/compare\/epic\/8716\.\.\./; +const COMPARE_FIRST = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/compare\/epic\/8716\.\.\.aaaa/; +const COMPARE_SIBLING = + /^GET https:\/\/api\.github\.com\/repos\/o\/r\/compare\/epic\/8716\.\.\.cccc/; const UPDATE = /^PUT https:\/\/api\.github\.com\/repos\/o\/r\/pulls\/8859\/update-branch$/; const PULL = /^GET https:\/\/api\.github\.com\/repos\/o\/r\/pulls\/8859$/; @@ -93,7 +97,9 @@ describe("lane retrigger", () => { ]); expect(outcome.code).toBe(0); - expect(outcome.stdout).toBe("#8859 aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n"); + expect(outcome.stdout).toBe( + "#8859 3 behind, aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n", + ); const update = requests.findIndex((line) => line.startsWith("PUT")); expect(update).toBeGreaterThan(-1); expect(JSON.parse(bodies[update] ?? "{}")).toEqual({expected_head_sha: BEFORE}); @@ -145,7 +151,23 @@ describe("lane retrigger", () => { ]); expect(out.code).toBe(0); - expect(out.stdout).toBe("#8859 aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n"); + expect(out.stdout).toBe( + "#8859 1 behind, aaaaaaaa -> bbbbbbbb\nRETRIGGER-VERDICT: RETRIGGERED\n", + ); + }); + + it("routes a read that fails after an earlier child was moved to the written-to code", async () => { + const out = await run([ + [LIST, listed([child(), child({number: 8860, head: {sha: SIBLING, ref: "build/8767"}})])], + [COMPARE_FIRST, standing("behind", 2)], + [UPDATE, ACCEPTED], + [PULL, served(pullPayload({number: 8859, head: {sha: AFTER, ref: "build/8766"}}))], + [COMPARE_SIBLING, {status: 502, body: '{"message":"Bad gateway"}'}], + ]); + + expect(out.code).toBe(APPEND_UNKNOWN); + expect(out.code).not.toBe(LANE_UNREADABLE); + expect(out.stderr.join(" ")).toContain("#8859 2 behind, aaaaaaaa -> bbbbbbbb"); }); it("is UNKNOWN, never an empty sweep, when the pull request list cannot be read", async () => {