diff --git a/.decisions/0397-a-tuval-route-rests-on-a-standing-text-pass.md b/.decisions/0397-a-tuval-route-rests-on-a-standing-text-pass.md new file mode 100644 index 000000000..31fb23fdd --- /dev/null +++ b/.decisions/0397-a-tuval-route-rests-on-a-standing-text-pass.md @@ -0,0 +1,115 @@ +--- +id: 0397 +title: A routed-elsewhere record rests on the text review it asserts +status: accepted +date: 2026-09-16 +tags: [fabrika, review-ui, tuval, pipeline, governance] +--- + +# 0397 — A routed-elsewhere record rests on the text review it asserts + +**What this decides:** the `review-code` verdict is a precondition of a `review-ui route`, not +commentary on it. `review-ui route` reads the verdict in force at `--sha` and refuses on a standing +FAIL; under the interim Tuval exception, where the route rests on a hand-verification, it refuses an +absent verdict too. Founder ruling on +[#9196](https://github.com/kamp-us/phoenix/issues/9196), 2026-09-15: +[the ruling comment](https://github.com/kamp-us/phoenix/issues/9196#issuecomment-5688739893), +recorded by the driver under the standing delegation of founder ruling #8807 R4.1. + +## Context + +The 2026-09-06 interim exception on [#7306](https://github.com/kamp-us/phoenix/issues/7306) lets a +Tuval PR resolve the `review-ui` namespace with a `routed-elsewhere` record instead of a rendered +verdict, and prescribes the clause that record carries, verbatim: "Tuval surface; no admissible +renderer until #7306 lands (founder ruling 2026-09-06). Text review PASS + builder hand-verification +stand in." + +That clause asserts two things, and until now the verb checked neither. +[0391](0391-hand-verification-binds-ui-content.md) mechanized the second half: the +hand-verification's currency became `--verified-at`, landed on +[#9178](https://github.com/kamp-us/phoenix/issues/9178). The first half stayed written text. +`runRoute` in [`route-verb.ts`](../packages/fabrika-cli/src/review-ui/route-verb.ts) read the PR, the +live head, the `ui` class and the `--verified-at` range, and no verdict marker at all — so it would +post the clause at a head where the text gate stood FAIL, and exit 0. + +The exception's own text is ambiguous about whether that PASS is required. The bullet after the +clause names exactly one thing as required — the builder's hand-verification — while the clause +asserts the conjunction, and the verb resolved the ambiguity permissively. + +The pipeline has read it as a conjunction every time it was used: across #7306's sunset list of +80-plus entries no route was posted over a standing text FAIL, and on +[#8878](https://github.com/kamp-us/phoenix/pull/8878), +[#8804](https://github.com/kamp-us/phoenix/pull/8804) and +[#8802](https://github.com/kamp-us/phoenix/pull/8802) the text gate FAILed, the builder repaired, and +the route landed only at the subsequent PASS head. The gap surfaced on +[#9193](https://github.com/kamp-us/phoenix/pull/9193), where every mechanical precondition held at +the head, `review-code` stood FAIL at that exact head, and the gate ended CANT-SEE rather than post +a clause it could not defend. + +The failure is 0391's quiet kind. The `routed-elsewhere` format carries no polarity and no attached +captures, so a record resting on a text PASS and one resting on a standing FAIL read identically, and +`ship gate` resolves both as `routed`. Nothing wrong merges — `ship` gates on `review-code` +independently — so what it costs is a false statement on the permanent record and on the sunset list +the first real Tuval UI review is meant to sweep against. + +## Decision + +**The text review the clause asserts is read by the verb, at the head the record binds.** + +1. **A standing FAIL refuses the route.** The `review-code` verdict in force at `--sha` is resolved + before anything is composed or posted, and a FAIL is exit `20` with the comment named. The route + returns when the text gate passes on a head; nothing about the record changes in the meantime. +2. **Absence refuses exactly where the record claims a PASS.** A route carrying `--verified-at` + stands in for the render under the #7306 exception, whose clause names both halves, so no text + verdict binding `--sha` is the same `20`. A route with no hand-verification — a prose-only diff + under a declared `uiSurfaces` prefix — asserts nothing about the text lane, so an absent verdict + is stated on stderr and in the answer's `textReview` field rather than refused. This is the + narrow arm of the ruling's direction: it makes the clause's assertion unpostable without its + evidence, without ordering the two gates on every PR that has no such clause to make. +3. **The reader is `review verdicts`', and the ordering `ship gate`'s.** The claims are the + `verdict-marker` first line and the §CP advisory carrier, ordered by `inForce` and judged current + by `bindToContent` + ([`text-verdict.ts`](../packages/fabrika-cli/src/review-ui/text-verdict.ts)). A condition each + reader derives for itself is a condition each reader derives differently — 0391 §3's rule, applied + to the other half of the same clause. A verdict the head has moved past is not in force, so a + content-bound PASS that survives a rebase survives here too. +4. **GitHub's native review fold stays out.** `ship gate` folds an `APPROVED` or `CHANGES_REQUESTED` + review into `review-code` because it is the merge authority. This verb judges only whether its own + clause states something true, and the fold costs a second API surface for a carrier the text gate + does not emit. The merge gate still reads it. +5. **The exception's text carries the condition.** It lands on #7306 as a dated amendment below the + 2026-09-06 ruling, never as an edit to it, so the sunset list stays readable against the text each + entry was posted under — 0391 §5's rule, unchanged. Landed 2026-09-16: + [the amendment comment](https://github.com/kamp-us/phoenix/issues/7306#issuecomment-5701952969), + appended to the issue body in the same shape, stating both halves of the clause as required. + +**Rejected: dropping the assertion from the clause instead.** It is the cheapest change and it +weakens what the sunset sweep can rely on: the first real Tuval UI review reads those entries back, +and an entry that claims nothing about the text lane tells it nothing. The founder was asked whether +a ui record may stand over a failed text review and answered no, so the clause stays and the verb +earns it. + +**Rejected: refusing an absent verdict on every route.** It orders the two gates on PRs whose record +makes no claim about the text lane at all, and a review-ui lane that runs before the text one parks +on a fact about sequencing rather than about the PR. + +**Sunset.** This decision lives exactly as long as the exception it conditions, as 0391 does. When +#7306 lands a trusted evidence path, the clause goes and this record is retired with it. + +## Consequences + +- A Tuval route over a standing text FAIL is refused at `20` rather than posted, and the reviewer's + move is the one the pipeline already made by hand: let the text lane repair, route at the passing + head. +- A Tuval route now needs the text verdict landed first. The reviewer reads what stands with + `fabrika review verdicts ` instead of deriving it from a comment scan. +- A prose-only route is unchanged in its outcome and louder in its answer: `textReview` says whether + a text verdict backed it, so the sunset sweep can tell the two kinds of entry apart. +- A text verdict carried only by a GitHub native review does not clear the route. The refusal names + the reader, so a lane in that shape sees why. +- #7306's sunset list gains the same fact per entry as 0391 gave it: which text verdict the record + rested on, beside the hand-verification's head. + +## Records + +no vocabulary impact diff --git a/claude-plugins/fabrika/skills/review-ui/SKILL.md b/claude-plugins/fabrika/skills/review-ui/SKILL.md index 02538e2db..7c4006a7f 100644 --- a/claude-plugins/fabrika/skills/review-ui/SKILL.md +++ b/claude-plugins/fabrika/skills/review-ui/SKILL.md @@ -72,6 +72,16 @@ the route can post; exit `11` naming the ceiling, or naming the two heads as div flag where there is no hand-verification, and never derive the range by hand — a condition you check by eye is one the next gate checks differently. +**The route also rests on the text gate's verdict, and the verb reads that for you.** Exit `20` +means the `review-code` verdict in force at `--sha` is a **FAIL**: the record would assert a text +PASS that is not there, and the polarity-free format leaves no later reader able to falsify it. That +is not yours to route around — the text lane repairs, and you route at the head it passes. The same +`20` covers an **absent** text verdict on a `--verified-at` route, because the exception's clause +names both halves; a prose-only route with no text verdict posts, and the answer's `textReview` +field says which of the two it rested on. The verdict is read before the `--verified-at` range, so a +route that is both spent at `--verified-at` and standing-FAIL at `--sha` meets `20` rather than +`12` — the text lane is the move to make first, and the desk run is re-run after it. + Exit `7` covers four different facts, and only one of them is a clean end — **read the message before you pick a terminal.** `raises no ui class` means nothing required your namespace and there is nothing to route: end ROUTED-ELSEWHERE with no write. The other three — the PR proven absent diff --git a/claude-plugins/fabrika/skills/review-ui/contract.md b/claude-plugins/fabrika/skills/review-ui/contract.md index 9707016c0..46c1645d3 100644 --- a/claude-plugins/fabrika/skills/review-ui/contract.md +++ b/claude-plugins/fabrika/skills/review-ui/contract.md @@ -173,6 +173,7 @@ sibling's numerals is not a goal the doctrine sets. | `16` | proven: no preview deployment exists for this PR — the announced-preview convention resolves to nothing; the skill's CANT-SEE route | | `17` | proven: at least one evidence upload or upload-verification failed — **nothing was posted** | | `18` | refused: the write would retire a standing verdict of the **opposite polarity** at this head and `--supersede` was not passed — nothing posted | +| `20` | refused, proven: the text review a `route` record rests on is not a standing PASS at the head it binds — the `review-code` verdict in force at `--sha` is a FAIL, or a route resting on a hand-verification has no text verdict binding that head — nothing posted | | `127` | the verb never ran at all (unresolved binary) | **`7` versus `11`** is the package's spine: a 404 is a fact about the repository, an unreachable @@ -758,9 +759,11 @@ The reasoning arrives on **stdin only**, for the same reason as `post` and `note | stdin | markdown | yes | — | which files changed and why none of them renders anything | **Output** — machine. One JSON object: -`{"answer":"routed","namespace":"review-ui","sha":"6c6fe226…","uiFiles":2,"verifiedAt":null,"upsert":"created","commentUrl":"…"}`. +`{"answer":"routed","namespace":"review-ui","sha":"6c6fe226…","uiFiles":2,"verifiedAt":null,"textReview":"absent","upsert":"created","commentUrl":"…"}`. `verifiedAt` is the `--verified-at` head the range was cleared over, and `null` where the route -rested on no hand-verification. +rested on no hand-verification. `textReview` is the `review-code` verdict this record rests on: +`pass` where one stands in force at `--sha`, `absent` where none binds that head — and `absent` is +reachable only on a route carrying no `--verified-at`, because one that does is refused at `20`. **Why it exists.** `ship scope` raises the `ui` class from a path test that cannot see whether pixels moved, so a PR whose only change under a declared `uiSurfaces` prefix is prose requires this namespace — and @@ -779,7 +782,13 @@ diff was read, and is re-read rather than re-bound. Read the changed-file list; the gate is meanwhile blocking. Refuse a diff that raises no `ui` class (`7`) — nothing required this namespace, so there is nothing to route; the predicate is `review/classes.ts`'s own `isUiSurface`, over the same declared `uiSurfaces` prefixes the gate raised the class from, never a -second copy. With `--verified-at`, compare that head to `--sha` and refuse on `12` when any file in +second copy. Read the PR's comments and resolve the `review-code` verdict in force at `--sha`; a +standing FAIL is `20`, and so is an absent verdict on a route carrying `--verified-at`. That read +runs **before** the `--verified-at` comparison below, so a route that is both spent at +`--verified-at` and standing-FAIL at `--sha` exits `20`, not `12` — a record asserting a text PASS +that is not there is unpostable at any head, while a spent hand-verification is cleared by re-running +it, so the text lane is the move to name first. With `--verified-at`, compare that head to `--sha` +and refuse on `12` when any file in the range raises the `ui` class — the hand-verification is then spent and a fresh one is owed at `--sha`; a comparison that came back at GitHub's 300-file ceiling is `11`, because the compare declares no total and a capped list can only ever hide a `ui`-class file, and so is one whose two @@ -799,6 +808,23 @@ head to compare; where a hand-verification exists, naming it is what makes the r Nothing about the `routed-elsewhere` bytes changes either way: the record stays head-bound to `--sha` and carries no evidence field. +**The text review the record rests on.** An interim exception that lets a hand-verification stand +in for a render prescribes the clause such a record carries, and that clause asserts a text review +PASS beside the hand-verification. The verb used to post it while reading neither half, so a record +over a standing text FAIL and one over a PASS read identically — and the `routed-elsewhere` format +carries no polarity for a later reader to tell them apart. The text PASS is a precondition, not +commentary, and the verb reads it: it resolves the `review-code` verdict in force at `--sha` through +`review verdicts`' own two carriers — the `verdict-marker` first line and the §CP advisory — ordered +by `ship gate`'s `inForce`, judged current by the same `bindToContent`, and given the advisory's +polarity by the one `advisoryPolarity` its sibling readers call, so no two readers hold different +rules. That last one was a copy before it was shared, and the copy diverged: a `[FAIL]` row inside an +advisory is an invalid emission, and it cleared this route while `ship gate` refused on the same +comment. A standing FAIL refuses on `20`. An **absent** verdict refuses on `20` only where +`--verified-at` is passed: that route asserts the conjunction, while a prose-only route asserts +nothing about the text lane and says so on stderr instead of blocking. The host's native review fold +is `ship gate`'s widening and is not read here — the merge gate still reads it, and this verb only +judges what its own clause claims. + **What this verb does not decide.** Whether the diff renders anything. That is the skill's judgment over `review diff`'s refusal-guarded bytes. Narrowing the `ui` path class instead was proposed and rejected: no path test can decide whether pixels moved, so a verb that tried would just @@ -817,6 +843,7 @@ relocate the defect. This verb takes the judgment as `--clause` plus a body and | `10` | `--sha` or `--verified-at` is not a head SHA, or `--clause` is blank | | `11` | a precondition read failed, the changed-file list came back truncated, or the `--verified-at` comparison came back at the 300-file ceiling or between two diverged heads — nothing was posted | | `12` | the live head moved past `--sha` — the diff you read is gone; or a `ui`-class file changed between `--verified-at` and `--sha`, so the hand-verification is spent | +| `20` | the `review-code` verdict in force at `--sha` is a FAIL, or a route resting on `--verified-at` has no `review-code` verdict binding that head | **Errors** @@ -839,6 +866,8 @@ relocate the defect. This verb takes the judgment as `--clause` plus a body and | `review-ui route: cannot read for #: — nothing was posted.` | 11 | refusal | | `review-ui route: the live head is , not — the diff you read is gone; re-read at .` | 12 | refusal | | `review-ui route: raise the ui class in .. — the hand-verification at is spent; re-run it at .` | 12 | refusal | +| `review-ui route: review-code stands FAIL at (comment ) — this record would assert a text PASS that is not there; repair the finding and route at the head the text gate passes.` | 20 | refusal | +| `review-ui route: no standing review-code verdict binds , and a route resting on a hand-verification asserts one — land the text verdict first, and read what stands with fabrika review verdicts .` | 20 | refusal | **Scope** — one PR, one comment write, the caller's stdin. @@ -851,7 +880,7 @@ $ fabrika review-ui route 6326 --sha 6c6fe226 \ export or type changed. `design-token-lint.config.json` rewrites two note strings; the guard's data fields are byte-identical. No component, route, token or style is touched. EOF -{"answer":"routed","namespace":"review-ui","sha":"6c6fe226","uiFiles":2,"verifiedAt":null,"upsert":"created","commentUrl":"https://github.com///pull/6326#issuecomment-5123990412"} +{"answer":"routed","namespace":"review-ui","sha":"6c6fe226","uiFiles":2,"verifiedAt":null,"textReview":"absent","upsert":"created","commentUrl":"https://github.com///pull/6326#issuecomment-5123990412"} ``` A route resting on a hand-verification names the head it ran at, and the range decides whether it @@ -863,7 +892,7 @@ $ fabrika review-ui route 4471 --sha fb01065b --verified-at 8efd315a \ The desk run at `8efd315a` drove every readout this diff touches. `8efd315a..fb01065b` is one commit under `packages//`, so the composition is byte-identical. EOF -{"answer":"routed","namespace":"review-ui","sha":"fb01065b","uiFiles":3,"verifiedAt":"8efd315a","upsert":"created","commentUrl":"https://github.com///pull/4471#issuecomment-5598041887"} +{"answer":"routed","namespace":"review-ui","sha":"fb01065b","uiFiles":3,"verifiedAt":"8efd315a","textReview":"pass","upsert":"created","commentUrl":"https://github.com///pull/4471#issuecomment-5598041887"} $ fabrika review-ui route 4471 --sha fb01065b --verified-at 8efd315a --clause "…" < why.md review-ui route: scanned 2 files changed in 8efd315a..fb01065b; 2 raise the ui class. @@ -875,6 +904,13 @@ $ fabrika review-ui route 4471 --sha 9c40aa71 --verified-at 8efd315a --clause " review-ui route: 8efd315a is diverged of 9c40aa71, not an ancestor — the comparison answers from their merge base, so 8efd315a..9c40aa71 was never read. Re-run the hand-verification at 9c40aa71. # exit 11 + +$ fabrika review-ui route 4471 --sha fb01065b --verified-at fb01065b --clause "…" < why.md +review-ui route: scanned 6 comments. +review-ui route: scanned 2 review-code claims; FAIL at fb01065b via the marker carrier. +review-ui route: review-code stands FAIL at fb01065b (comment 5598219196) — this record would +assert a text PASS that is not there; repair the finding and route at the head the text gate passes. +# exit 20 ``` **Grounding** @@ -890,6 +926,9 @@ their merge base, so 8efd315a..9c40aa71 was never read. Re-run the hand-verifica - **A hand-verification's currency is the verb's judgment, not the gate's**, and it binds the `ui` files' content rather than the record's own head — a commit that moves no rendered surface keeps the evidence rather than spending a desk session to re-prove it. +- **The text PASS a hand-verification clause asserts is read, not assumed** — a standing + `review-code` FAIL at the record's head refuses the route, and the reader is `review verdicts`' + own, so the two gates cannot answer one question differently. - **The head binding**, and why a moved head is re-read rather than re-bound. --- diff --git a/packages/fabrika-cli/docs/verb-reference.md b/packages/fabrika-cli/docs/verb-reference.md index 9595796e7..6c500d059 100644 --- a/packages/fabrika-cli/docs/verb-reference.md +++ b/packages/fabrika-cli/docs/verb-reference.md @@ -1055,7 +1055,7 @@ Judge a UI pull request over its preview deployment. Contract: | `review-ui render` | the named surfaces captured from a PR's preview deployment — a route, or a route plus a realized tier state (`/pano:auth` renders as the yazar test account, `/pano:auth-caylak` as the çaylak one), refusing on `11` unless the preview's own session read proves the shot came back signed in *and* at the tier the surface named. `--flag =` forces a dark-shipped flag for the run, refusing on `10` unless every surface names a tier state and on `11` unless the preview's own evaluation says the key took. `--viewport ` picks the widths, over the closed set `desktop` (1280×800) and `mobile` (390×844), crossed with `--surface` and defaulting to `desktop` alone; a name outside the set or repeated is `10`, and a shot whose PNG width reads back as another width is `19` | | `review-ui post` | the `review-ui` verdict on stdin, appended into this namespace's one comment | | `review-ui note` | a typed blocker note when the surfaces cannot be seen | -| `review-ui route` | a head-bound `routed-elsewhere` record: this PR renders nothing, so no verdict is owed. `--verified-at ` names the head a hand-verification standing in for the render ran at, and the route is refused on `12` when any file in the range to `--sha` raises the `ui` class — the evidence is spent — or on `11` when the range went unread — the comparison came back at GitHub's 300-file ceiling, or between two heads that have diverged, where the platform answers from their merge base instead | +| `review-ui route` | a head-bound `routed-elsewhere` record: this PR renders nothing, so no verdict is owed. `--verified-at ` names the head a hand-verification standing in for the render ran at, and the route is refused on `12` when any file in the range to `--sha` raises the `ui` class — the evidence is spent — or on `11` when the range went unread — the comparison came back at GitHub's 300-file ceiling, or between two heads that have diverged, where the platform answers from their merge base instead. The `review-code` verdict in force at `--sha` is read too: a standing FAIL refuses on `20`, as does an absent verdict on a `--verified-at` route, because the record's clause asserts that PASS | **Exit codes.** The shared table (with `4` a required file that does not parse or violates its schema), plus `12` the artifact is not the PR's current tree · `13` a surface threw an uncaught diff --git a/packages/fabrika-cli/src/lane/prove-verb.ts b/packages/fabrika-cli/src/lane/prove-verb.ts index 03c5424a3..820eac4c4 100644 --- a/packages/fabrika-cli/src/lane/prove-verb.ts +++ b/packages/fabrika-cli/src/lane/prove-verb.ts @@ -51,7 +51,7 @@ import {governedRootsOr, uiSurfacesOr} from "../config/paths.ts"; import {getIssue, listComments} from "../io/issues.ts"; import {isRecord, parseJson} from "../io/json.ts"; import {getPullRequest, listPullFiles} from "../io/pulls.ts"; -import {readAdvisory} from "../review/advisory.ts"; +import {advisoryPolarity, readAdvisory} from "../review/advisory.ts"; import {partitionWithUi, ROUTED_NAMESPACES, shipNamespacesOf} from "../review/classes.ts"; import {bindRange, contentDigestAt, rangeContentAt} from "../review/content-binding.ts"; import {bindHead} from "../review/head.ts"; @@ -784,9 +784,9 @@ const readNamespaceRows = ( advisories.push({ claim: { namespace: advisory.namespace, - // The advisory carrier is PASS-only; a [FAIL] row inside one is an - // invalid emission — treated as fail below, never read as a pass. - polarity: /\[FAIL\]/.test(comment.body) ? "FAIL" : "PASS", + // An invalid [FAIL] emission inside an advisory is treated as fail below, + // never read as a pass — the carrier's own predicate, one copy. + polarity: advisoryPolarity(comment.body), commentId: comment.id, sha: advisory.sha, // The advisory withholds a content binding by design — head-bound only. diff --git a/packages/fabrika-cli/src/review-ui/codes.ts b/packages/fabrika-cli/src/review-ui/codes.ts index bef60b36d..adc76287f 100644 --- a/packages/fabrika-cli/src/review-ui/codes.ts +++ b/packages/fabrika-cli/src/review-ui/codes.ts @@ -101,3 +101,16 @@ export const SUPERSEDES_VERDICT = 18; * viewport label would make the narrow half of the design law answerable from desktop pixels. */ export const WRONG_VIEWPORT = 19; +/** + * Refused, proven: the text review this route rests on is not a standing PASS at the record's head. + * + * Two triggers, one meaning and one caller move — the `review-code` verdict in force at `--sha` is a + * FAIL, or a route resting on a hand-verification has no text verdict binding that head at all. + * Either way the `routed-elsewhere` clause would assert a PASS nobody formed, and the format carries + * no polarity for a later reader to tell a true assertion from a false one. Its own seat rather than + * {@link STALE_TREE}: nothing here is stale — the tree is the one the reviewer read, and what is + * missing is the other gate's verdict over it. + * + * @ruling https://github.com/kamp-us/phoenix/issues/9196#issuecomment-5688739893 + */ +export const TEXT_REVIEW_UNMET = 20; diff --git a/packages/fabrika-cli/src/review-ui/command.ts b/packages/fabrika-cli/src/review-ui/command.ts index b781e988c..738974550 100644 --- a/packages/fabrika-cli/src/review-ui/command.ts +++ b/packages/fabrika-cli/src/review-ui/command.ts @@ -230,7 +230,7 @@ const route = leafCommand( ).pipe( Command.withShortDescription("Record that this PR renders nothing, so no verdict is owed."), Command.withDescription( - "Record, bound to the head whose diff you read, that this PR moves no pixels — so review-ui owes it no verdict and ship's gate resolves the namespace as routed. The reasoning arrives on STDIN, the record's first line is composed through the `routed-elsewhere` wire format, and both are leak-scanned, upserted as one comment and read back. It is not a verdict: the format carries no polarity, the record is head-bound so any push voids it, and no capture evidence is involved either way. Whether the diff renders anything is your judgment over `review diff`, never a verb's. Where the route rests on a hand-verification instead, --verified-at names the head that ran at and the route is refused when any file in the range to --sha raises the ui class — the same isUiSurface over the same prefixes, and a comparison at GitHub's 300-file ceiling, or one whose two heads have diverged, is UNKNOWN rather than a cleared range. Prints one JSON object. Exits 3 (empty stdin), 5 (machine-local path), 6 (bare @ reference), 7 (PR absent, closed, empty, or its diff raises no ui class — nothing to route), 8 (the post failed — UNKNOWN), 9 (the record does not read back as sent), 10 (bad --sha or --verified-at, or a blank --clause), 11 (a precondition read failed, the file list was truncated, or the --verified-at comparison came back capped or diverged — nothing was posted), 12 (the live head moved past --sha, or a ui-class file changed since --verified-at). Example: fabrika review-ui route 6326 --sha 6c6fe226 --clause \"no rendered delta; both files are prose only\" < why.md", + "Record, bound to the head whose diff you read, that this PR moves no pixels — so review-ui owes it no verdict and ship's gate resolves the namespace as routed. The reasoning arrives on STDIN, the record's first line is composed through the `routed-elsewhere` wire format, and both are leak-scanned, upserted as one comment and read back. It is not a verdict: the format carries no polarity, the record is head-bound so any push voids it, and no capture evidence is involved either way. Whether the diff renders anything is your judgment over `review diff`, never a verb's. Where the route rests on a hand-verification instead, --verified-at names the head that ran at and the route is refused when any file in the range to --sha raises the ui class — the same isUiSurface over the same prefixes, and a comparison at GitHub's 300-file ceiling, or one whose two heads have diverged, is UNKNOWN rather than a cleared range. The review-code verdict in force at --sha is read as well, through the same carriers and ordering review verdicts and ship gate use: a standing FAIL refuses, and so does an absent verdict on a --verified-at route, whose record asserts that PASS. Prints one JSON object. Exits 3 (empty stdin), 5 (machine-local path), 6 (bare @ reference), 7 (PR absent, closed, empty, or its diff raises no ui class — nothing to route), 8 (the post failed — UNKNOWN), 9 (the record does not read back as sent), 10 (bad --sha or --verified-at, or a blank --clause), 11 (a precondition read failed, the file list was truncated, or the --verified-at comparison came back capped or diverged — nothing was posted), 12 (the live head moved past --sha, or a ui-class file changed since --verified-at), 20 (review-code stands FAIL at --sha, or no review-code verdict binds it on a --verified-at route). Example: fabrika review-ui route 6326 --sha 6c6fe226 --clause \"no rendered delta; both files are prose only\" < why.md", ), ); diff --git a/packages/fabrika-cli/src/review-ui/route-verb.ts b/packages/fabrika-cli/src/review-ui/route-verb.ts index 374a6ceed..0a08ce544 100644 --- a/packages/fabrika-cli/src/review-ui/route-verb.ts +++ b/packages/fabrika-cli/src/review-ui/route-verb.ts @@ -34,6 +34,15 @@ * are UNKNOWN. * Left off, the range is never read and the route behaves as it always did: a prose-only diff under * a declared prefix rests on the body alone and has no head to compare against. + * + * **The record also rests on the text gate's verdict, so this verb reads that too.** The `review-code` + * verdict in force at `--sha` is a precondition: a standing FAIL refuses the route outright, and a + * route resting on a hand-verification refuses when no text verdict binds that head at all, because + * the clause the interim exception prescribes asserts the conjunction. A prose-only route asserts + * nothing about the text lane, so an absent verdict there is stated on stderr rather than refused. + * The reader is `review verdicts`'s and the ordering `ship gate`'s — see `./text-verdict.ts`. + * + * @ruling https://github.com/kamp-us/phoenix/issues/9196#issuecomment-5688739893 */ import {Effect} from "effect"; import type {ChildProcessSpawner} from "effect/unstable/process"; @@ -49,6 +58,7 @@ import type {StdinRead} from "../io/stdin.ts"; import {normalizeForReadback} from "../report/compose.ts"; import {type AuthoredSurface, leakRefusal, readAuthored} from "../review/authored.ts"; import {isUiSurface} from "../review/classes.ts"; +import {headContentFor} from "../review/head-content.ts"; import {openPull, resolveTargetRepo, scannedLine} from "../review/target.ts"; import {answer, FAILED, refuse, type VerbOutcome} from "../verb.ts"; import { @@ -62,10 +72,12 @@ import { PRECONDITION_UNKNOWN, READBACK_MISMATCH, STALE_TREE, + TEXT_REVIEW_UNMET, WRITE_UNKNOWN, ZERO_SCOPE, } from "./codes.ts"; import {NAMESPACE} from "./post-verb.ts"; +import {standingTextVerdict, TEXT_NAMESPACE, textClaims} from "./text-verdict.ts"; const VERB = "review-ui route"; @@ -191,6 +203,58 @@ export const runRoute = ( ); } + // The clause's other half. `ship gate` reads the two namespaces independently, so a route over + // a standing text FAIL merges nothing wrong — what it costs is a permanent record asserting a + // PASS nobody formed, which the polarity-free wire format gives a later reader no way to + // falsify. One comments read serves this and the upsert below. + const comments = yield* listComments(repo, pr); + if (comments._tag === "Failure") return unreadable("the comments", pr, comments.reason); + diagnostics.push(scannedLine(VERB, comments.value.length, "comment")); + const claims = textClaims(comments.value); + const headContent = yield* headContentFor( + VERB, + repo, + pr, + target.pull, + options.sha, + claims, + inspected, + ); + diagnostics.push(...headContent.diagnostics); + const text = standingTextVerdict(claims, inspected, headContent.digest); + diagnostics.push( + scannedLine( + VERB, + claims.length, + `${TEXT_NAMESPACE} claim`, + text === null + ? `none in force at ${inspected}` + : `${text.polarity} at ${text.sha} via the ${text.carrier} carrier`, + ), + ); + if (text !== null && text.polarity === "FAIL") { + return refuse( + TEXT_REVIEW_UNMET, + `${VERB}: ${TEXT_NAMESPACE} stands FAIL at ${inspected} (comment ${text.commentId}) — this record would assert a text PASS that is not there; repair the finding and route at the head the text gate passes.`, + diagnostics, + ); + } + // Absence refuses exactly where the record claims a PASS: a route resting on a + // hand-verification stands in for the render under an interim exception whose prescribed + // clause names both halves. A prose-only route claims neither, so it says so instead. + if (text === null && verified !== null) { + return refuse( + TEXT_REVIEW_UNMET, + `${VERB}: no standing ${TEXT_NAMESPACE} verdict binds ${inspected}, and a route resting on a hand-verification asserts one — land the text verdict first, and read what stands with fabrika review verdicts ${pr}.`, + diagnostics, + ); + } + if (text === null) { + diagnostics.push( + `${VERB}: no standing ${TEXT_NAMESPACE} verdict binds ${inspected} — this record rests on the diff alone and asserts nothing about the text lane.`, + ); + } + if (verified !== null) { const range = `${verified}..${inspected}`; const compared = yield* compareFiles(repo, verified, inspected); @@ -245,9 +309,6 @@ export const runRoute = ( // would leave `ship gate` picking between two claims about one question. const me = yield* viewerLogin; if (me._tag === "Failure") return unreadable("the authenticated user", pr, me.reason); - const comments = yield* listComments(repo, pr); - if (comments._tag === "Failure") return unreadable("the comments", pr, comments.reason); - diagnostics.push(scannedLine(VERB, comments.value.length, "comment")); const mine = comments.value .filter( (comment) => @@ -317,6 +378,7 @@ export const runRoute = ( sha: inspected, uiFiles: ui.length, verifiedAt: verified, + textReview: text === null ? "absent" : "pass", upsert: mine === undefined ? "created" : "edited", commentUrl: landed.url, }), diff --git a/packages/fabrika-cli/src/review-ui/route-verb.unit.test.ts b/packages/fabrika-cli/src/review-ui/route-verb.unit.test.ts index 607527706..8a5b817fc 100644 --- a/packages/fabrika-cli/src/review-ui/route-verb.unit.test.ts +++ b/packages/fabrika-cli/src/review-ui/route-verb.unit.test.ts @@ -7,13 +7,20 @@ import {describe, expect, it} from "vitest"; import {fakeSeams, type HttpReply, type Scripted} from "../fakes.test-support.ts"; import {COMPARE_FILE_CAP} from "../io/pulls.ts"; import type {StdinRead} from "../io/stdin.ts"; -import {read as readVerdict} from "../wire/verdict-marker.ts"; +import {emitAdvisory, reviewedHeadLine} from "../review/advisory.ts"; +import { + emit as emitVerdict, + headSha, + read as readVerdict, + clause as toClause, +} from "../wire/verdict-marker.ts"; import { EMPTY_STDIN, OFF_VOCABULARY, PRECONDITION_UNKNOWN, READBACK_MISMATCH, STALE_TREE, + TEXT_REVIEW_UNMET, ZERO_SCOPE, } from "./codes.ts"; import {runRoute} from "./route-verb.ts"; @@ -73,6 +80,20 @@ const options = { const composed = (sha = HEAD, clause = CLAUSE): string => `routed-elsewhere: review-ui @ ${sha} — ${clause}\n\n${BODY.replace(/\n+$/, "")}\n`; +/** A `review-code` verdict comment, composed through the wire format the route reads it back with. */ +const textVerdict = (polarity: "PASS" | "FAIL", sha = HEAD): Record => { + const head = headSha(sha); + const clause = toClause("merge-ready"); + if (head === null || clause === null) throw new Error(`unusable fixture: ${sha}`); + return { + id: 4001, + user: {login: "reviewer"}, + created_at: "2026-09-14T00:00:00Z", + updated_at: "2026-09-14T00:00:00Z", + body: emitVerdict({namespace: "review-code", polarity, sha: head, content: null, clause}), + }; +}; + const happy = (): ReadonlyArray => [ [PULL, pull()], [FILES, PROSE_UI], @@ -187,7 +208,8 @@ describe("review-ui route", () => { [FILES, PROSE_UI], [COMPARE, reply], [USER, served({login: "reviewer"})], - [COMMENTS, {status: 200, body: "[]"}], + // A route resting on a hand-verification asserts the text PASS, so one has to stand. + [COMMENTS, served([textVerdict("PASS")])], [CREATE, {status: 201, body: JSON.stringify({id: 512399, html_url: URL})}], [READBACK, served({body: composed()})], ]; @@ -209,7 +231,7 @@ describe("review-ui route", () => { [FILES, PROSE_UI], [SAME, served({status: "identical", total_commits: 0, files: []})], [USER, served({login: "reviewer"})], - [COMMENTS, {status: 200, body: "[]"}], + [COMMENTS, served([textVerdict("PASS")])], [CREATE, {status: 201, body: JSON.stringify({id: 512399, html_url: URL})}], [READBACK, served({body: composed()})], ], @@ -277,6 +299,109 @@ describe("review-ui route", () => { }); }); + // The clause a hand-verification route carries asserts a text review PASS beside it, and the verb + // asserted that while reading neither half. + describe("the text review the record rests on", () => { + const VERIFIED = "8efd315a1f2e3d4c5b6a7988776655443322110f"; + const COMPARE = new RegExp(`^GET \\S+/repos/o/r/compare/${VERIFIED}\\.\\.\\.${HEAD}$`); + const CLEAR = served({status: "ahead", files: [{filename: "docs/notes.md"}]}); + + const withComments = (...comments: ReadonlyArray): ReadonlyArray => [ + [PULL, pull()], + [FILES, PROSE_UI], + [USER, served({login: "reviewer"})], + [COMMENTS, served(comments)], + [CREATE, {status: 201, body: JSON.stringify({id: 512399, html_url: URL})}], + [READBACK, served({body: composed()})], + ]; + + const onHandVerification = (...comments: ReadonlyArray): ReadonlyArray => [ + [PULL, pull()], + [FILES, PROSE_UI], + [COMPARE, CLEAR], + [USER, served({login: "reviewer"})], + [COMMENTS, served(comments)], + [CREATE, {status: 201, body: JSON.stringify({id: 512399, html_url: URL})}], + [READBACK, served({body: composed()})], + ]; + + it("refuses on 20 over a standing FAIL at this head, posting nothing", async () => { + const {outcome, requests} = await run(withComments(textVerdict("FAIL"))); + expect(outcome.code).toBe(TEXT_REVIEW_UNMET); + expect(outcome.stderr.join("\n")).toContain("review-code stands FAIL"); + expect(requests.some((request) => CREATE.test(request) || PATCH.test(request))).toBe(false); + }); + + it("refuses on 20 over a standing FAIL even where the hand-verification is current", async () => { + const {outcome} = await run(onHandVerification(textVerdict("FAIL")), { + verifiedAt: VERIFIED, + }); + expect(outcome.code).toBe(TEXT_REVIEW_UNMET); + }); + + it("posts over a standing PASS at this head and records which half it read", async () => { + const {outcome} = await run(withComments(textVerdict("PASS"))); + expect(outcome.code).toBe(0); + expect(JSON.parse(outcome.stdout)).toMatchObject({answer: "routed", textReview: "pass"}); + }); + + // The exception's clause names both halves, so the evidence path is where absence refuses. + it("refuses on 20 when a hand-verification route has no text verdict at this head", async () => { + const {outcome, requests} = await run(onHandVerification(), {verifiedAt: VERIFIED}); + expect(outcome.code).toBe(TEXT_REVIEW_UNMET); + expect(outcome.stderr.join("\n")).toContain("no standing review-code verdict"); + expect(requests.some((request) => CREATE.test(request) || PATCH.test(request))).toBe(false); + }); + + // A prose-only route asserts nothing about the text lane, so it says so rather than blocking. + it("posts with no text verdict at all, stating that the record asserts none", async () => { + const {outcome} = await run(withComments()); + expect(outcome.code).toBe(0); + expect(JSON.parse(outcome.stdout)).toMatchObject({textReview: "absent"}); + expect(outcome.stderr.join("\n")).toContain("asserts nothing about the text lane"); + }); + + // A FAIL the head has moved past is not in force - `ship gate` reads it stale too. + it("does not refuse over a FAIL bound to another head", async () => { + const {outcome} = await run(withComments(textVerdict("FAIL", MOVED))); + expect(outcome.code).toBe(0); + expect(JSON.parse(outcome.stdout)).toMatchObject({textReview: "absent"}); + }); + + it("reads the control-plane advisory carrier as the PASS it is", async () => { + const {outcome} = await run( + onHandVerification({ + id: 4002, + user: {login: "owner"}, + created_at: "2026-09-14T00:00:00Z", + updated_at: "2026-09-14T00:00:00Z", + body: `${emitAdvisory("review-code", "merge-ready")}\n${reviewedHeadLine(HEAD)}\n`, + }), + {verifiedAt: VERIFIED}, + ); + expect(outcome.code).toBe(0); + expect(JSON.parse(outcome.stdout)).toMatchObject({textReview: "pass"}); + }); + + // `ship gate` and `lane prove` both read a `[FAIL]` row inside an advisory as a fail; a third + // reader that read it as a pass is how one comment cleared this route and refused at the gate. + it("reads a [FAIL] row inside an advisory as the FAIL its sibling readers read", async () => { + const {outcome, requests} = await run( + onHandVerification({ + id: 4003, + user: {login: "owner"}, + created_at: "2026-09-14T00:00:00Z", + updated_at: "2026-09-14T00:00:00Z", + body: `${emitAdvisory("review-code", "merge-ready")}\n${reviewedHeadLine(HEAD)}\n\n- [FAIL] review-code\n`, + }), + {verifiedAt: VERIFIED}, + ); + expect(outcome.code).toBe(TEXT_REVIEW_UNMET); + expect(outcome.stderr.join("\n")).toContain("review-code stands FAIL"); + expect(requests.some((request) => CREATE.test(request) || PATCH.test(request))).toBe(false); + }); + }); + it("refuses on 9 when the read-back is not the record that was sent", async () => { const {outcome} = await run([ [PULL, pull()], diff --git a/packages/fabrika-cli/src/review-ui/text-verdict.ts b/packages/fabrika-cli/src/review-ui/text-verdict.ts new file mode 100644 index 000000000..f06175050 --- /dev/null +++ b/packages/fabrika-cli/src/review-ui/text-verdict.ts @@ -0,0 +1,106 @@ +/** + * The standing `review-code` verdict a `routed-elsewhere` record rests on. + * + * The interim exception that lets a hand-verification stand in for a render prescribes a clause + * asserting a text review PASS beside it, and `review-ui route` asserted that conjunction while + * reading neither half. The ruling below settles it: the text PASS is a precondition of the route, + * not commentary on it, so the verb reads it here. + * + * **The reader is `review verdicts`'s, never a second one.** A claim is the `verdict-marker` first + * line or the §CP advisory carrier, exactly the two carriers that sweep resolves; the in-force + * ordering is `ship gate`'s own {@link inForce}, currency is {@link bindToContent}'s, and the + * advisory's polarity is {@link advisoryPolarity}'s. Three copies of one rule is how a marker reads + * current to one gate and stale to the next — the failure the hand-verification's own currency check + * was mechanized to stop, and the polarity predicate reached it first: held in triplicate, a + * `[FAIL]` row inside an advisory cleared this route while `ship gate` refused on the same comment. + * + * The advisory is admitted here whatever the lane's control-plane state, which is `lane prove`'s + * rule rather than `ship gate`'s — that one reads the carrier only under `--cp`, gating on an + * approval this verb does not judge. Reading it unconditionally can only make the route stricter, + * since an advisory the §CP fence would have excluded is still a text claim about this head. + * + * What is **not** read here is the host's native review fold. `ship gate` folds an `APPROVED` or + * `CHANGES_REQUESTED` review into `review-code` because it is the merge authority; this verb only + * judges whether the record's own clause states something true, and the fold costs a second API + * surface for a carrier the pipeline's text gate does not emit. A route's refusal line names + * `review verdicts`, so a lane whose text verdict lives only in a native review sees which reader + * answered. + * + * @ruling https://github.com/kamp-us/phoenix/issues/9196#issuecomment-5688739893 + */ +import type {CommentRecord} from "../io/issues.ts"; +import {advisoryPolarity, readAdvisory} from "../review/advisory.ts"; +import {inForce} from "../ship/gate-verb.ts"; +import {bindToContent, read as readMarker} from "../wire/verdict-marker.ts"; + +/** The namespace whose verdict the route rests on — the text gate's, fixed. */ +export const TEXT_NAMESPACE = "review-code"; + +/** One comment's claim about {@link TEXT_NAMESPACE}, before ordering or currency is applied. */ +export interface TextClaim { + readonly namespace: string; + readonly polarity: "PASS" | "FAIL"; + readonly sha: string; + /** The content the claim binds, or `null` for a carrier that emits none. */ + readonly content: string | null; + readonly carrier: "marker" | "advisory"; + readonly stamp: string; + readonly commentId: number; +} + +/** + * Every `review-code` claim the comments carry. + * + * A verdict retired below `review/supersede.ts`'s fence is not a claim: the surviving verdict holds + * the comment's first line, which is the only line either carrier is read from. + */ +export const textClaims = (comments: ReadonlyArray): ReadonlyArray => { + const claims: TextClaim[] = []; + for (const comment of comments) { + const marker = readMarker(comment.body); + if (marker._tag === "Found") { + if (marker.value.namespace !== TEXT_NAMESPACE) continue; + claims.push({ + namespace: marker.value.namespace, + polarity: marker.value.polarity, + sha: marker.value.sha, + content: marker.value.content, + carrier: "marker", + stamp: comment.updatedAt === "" ? comment.createdAt : comment.updatedAt, + commentId: comment.id, + }); + continue; + } + const advisory = readAdvisory(comment.body); + if (advisory === null || advisory.namespace !== TEXT_NAMESPACE) continue; + claims.push({ + namespace: advisory.namespace, + // The carrier's own predicate, the one `ship gate` and `lane prove` read: a `[FAIL]` + // row inside an advisory is an invalid emission, never the PASS this route rests on. + polarity: advisoryPolarity(comment.body), + sha: advisory.sha, + // It withholds a content binding by design, so it stays head-bound. + content: null, + carrier: "advisory", + stamp: comment.updatedAt === "" ? comment.createdAt : comment.updatedAt, + commentId: comment.id, + }); + } + return claims; +}; + +/** + * The text verdict in force at `head`, or `null` where none binds it. + * + * `null` folds two facts a route treats alike — no claim at all, and a claim the head moved past — + * because both leave the record's clause with no PASS to assert. The caller's stderr names which. + */ +export const standingTextVerdict = ( + claims: ReadonlyArray, + head: string, + digest: string | null, +): TextClaim | null => { + const winner = inForce(claims, head); + if (winner === null) return null; + return bindToContent(winner, head, digest)._tag === "Current" ? winner : null; +}; diff --git a/packages/fabrika-cli/src/review/advisory.ts b/packages/fabrika-cli/src/review/advisory.ts index b578e1ccf..b023237c8 100644 --- a/packages/fabrika-cli/src/review/advisory.ts +++ b/packages/fabrika-cli/src/review/advisory.ts @@ -43,3 +43,17 @@ export const readAdvisory = (body: string): AdvisoryCarrier | null => { const sha = bound?.[1] === undefined ? null : headSha(bound[1]); return sha === null ? null : {namespace: first[1].toLowerCase(), sha}; }; + +const FAIL_ROW = /\[FAIL\]/; + +/** + * The polarity a read advisory carries — `PASS` unless its body holds a `[FAIL]` row. + * + * The carrier is PASS-only by construction, which is why the rule looks redundant: `review post` + * refuses a §CP FAIL through it on `10`. It is not, because a `[FAIL]` row inside an advisory is an + * invalid emission every reader has to answer the same way — a hand-written comment reaches this + * parse too. Three readers holding the predicate in triplicate is how one gate read such a comment + * as a pass while the next read it as a fail; the rule lives here so there is one answer to read. + */ +export const advisoryPolarity = (body: string): "PASS" | "FAIL" => + FAIL_ROW.test(body) ? "FAIL" : "PASS"; diff --git a/packages/fabrika-cli/src/ship/gate-verb.ts b/packages/fabrika-cli/src/ship/gate-verb.ts index 0070eb520..3572f3868 100644 --- a/packages/fabrika-cli/src/ship/gate-verb.ts +++ b/packages/fabrika-cli/src/ship/gate-verb.ts @@ -35,7 +35,7 @@ import type {ChildProcessSpawner} from "effect/unstable/process"; import {governedRootsOr} from "../config/paths.ts"; import {type CommentRecord, listComments} from "../io/issues.ts"; import {listPullFiles, permissionFor} from "../io/pulls.ts"; -import {readAdvisory} from "../review/advisory.ts"; +import {advisoryPolarity, readAdvisory} from "../review/advisory.ts"; import {SHIP_NAMESPACES, touchesGovernanceRoot} from "../review/classes.ts"; import {headContentFor} from "../review/head-content.ts"; import {answer, refuse, type VerbOutcome} from "../verb.ts"; @@ -146,9 +146,9 @@ const candidateOf = (comment: CommentRecord, cp: boolean): Candidate | null => { ? null : { namespace: advisory.namespace, - // The advisory carrier is PASS-only. A `[FAIL]` row inside one is an invalid - // emission, caught below and reported — never read as a pass. - polarity: /\[FAIL\]/.test(comment.body) ? "FAIL" : "PASS", + // An invalid `[FAIL]` emission inside an advisory is caught below and reported — + // never read as a pass. The predicate is the carrier's own, shared by every reader. + polarity: advisoryPolarity(comment.body), sha: advisory.sha, // The §CP advisory withholds a content binding by design: the human-approval half of the // binding question is answered where head-binding is ruled, not here. So an advisory @@ -188,9 +188,14 @@ export const requiredWithFloor = ( * The in-force verdict for one namespace: head-bound candidates first, then newest write stamp. * * Exported so the ordering is testable without a PR: the two rules interact, and "head-bound - * outranks recency" is only checkable against a stale-but-newer counterexample. + * outranks recency" is only checkable against a stale-but-newer counterexample. It is generic in the + * claim so a caller carrying a narrower one — `review-ui route`'s two-polarity text claim — gets its + * own type back and needs no cast to read a field this module does not know about. */ -export const inForce = (candidates: ReadonlyArray, sha: string): Candidate | null => { +export const inForce = ( + candidates: ReadonlyArray, + sha: string, +): T | null => { const ordered = [...candidates].sort((a, b) => { const aBound = prefixMatch(a.sha, sha) ? 1 : 0; const bBound = prefixMatch(b.sha, sha) ? 1 : 0;