HOMELAB-2524: fix(forgejo): enforce the base branch's named required CI contexts #91
No reviewers
ld-reviewer
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/pi-extensions!91
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-2524-ci-contexts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The incident (GITS-368, epic HOMELAB-2518)
On
Homelab/ghostty-in-the-shell#37the head's combined commit status readsuccess— five green contexts — while two requiredemucontexts were absent from the head tree entirely. Leg 1 decided oninput.ci.state === "success"and used the contexts only to report which ones were non-green. An absent context is not a red context: the combined status cannot see a context that never ran, so an incomplete head was evaluated as green.The fix
Leg 1 now also reads the PR base branch's branch-protection rule and enforces its
status_check_contextsby name on the exact head.forgejo/api.ts—listBranchProtectionsnow carriesstatus_check_contextsandenable_status_check; newreadRequiredCiContexts()wrapper returns either{read: "rules", rules}or{read: "unreadable", reason}. A200with a non-array body now throws instead of reading as "no rules". All GETs; branch protection is never written (G6).forgejo/landing.ts— new pureassessRequiredContexts()/ruleCoversBranch(),LandingLockInput.requiredContexts, and the leg-1 check. Rule selection matchesbase.refexactly, plus a*pattern rule (release/*governsrelease/2.0) so a patterned rule cannot silently stop covering a branch. Another branch's rule is never borrowed — there is no "the only rule there is" fallback.forgejo/index.ts—collectLandingEvidencereads the base ref and the protection list once, shared by the merge gate and the census; bothevaluateLandingLockcall sites pass it through.docs/forgejo.md— leg 1 documents the named-context enforcement and why the combined status alone is insufficient.The four cases
enable_status_check: true) → every named context must be present on the head. Missing ⇒ refuse, naming each one.enable_status_check: false, contextsnull) → no named requirement. This is the live shape ofghostty-in-the-shell/main.Homelab/pi-extensions.Present-but-red keeps the existing non-green refusal wording (
CI is not green on the head commit … non-green contexts: …); the absent path never fires for it, so there is one refusal per fact and no duplicated message.Why unreadable does not refuse— WITHDRAWN in round 2 (see "Round 2" above: B1)The API does let us separate "readable, no rule" (
200 []) from "unreadable" (any throw), and this PR keeps that distinction in the types, the record and the note — an unreadable read is never reported as an unprotected repo, and no context name is ever fabricated.But a failed GET cannot say which failure it was (403 vs 404 vs 5xx vs a transient network error), so neither "the branch is protected" nor "the branch is unprotected" is established. The lock has no override, so refusing there would turn an infrastructure read failure into a permanent, unappealable merge blocker on every repo whose protections list the session's credential cannot read — while adding nothing that leg 1's combined-status check does not already cover. Chosen behaviour: skip the named check, record and say the gap in the landing note (
… could not be read … were NOT enforced), and leave the combined-status refusal untouched — a red head is still refused. This is pinned by two tests. The ticket's stricter reading (refuse when the caller cannot establish that the base has no rule) was rejected for that wedge reason; sincepi-extensionsanswers200 []live, either choice leaves this repo unblocked, but the stricter one would regress the whole existing gate suite and block every landing behind an unreadable endpoint.Round 2 — the review ruling, implemented (commit
c9eaa10)One reviewer approved round 1 and one requested changes; the orchestrator ruled for the change request and against the approval's reasoning. Every finding is addressed below. The wedge argument in round 1's "Why unreadable does not refuse" section is WITHDRAWN — see B1.
B1 — an unreadable protection read now REFUSES.
Round 1's defence ("refusing would wedge every repo whose protection we cannot read") was measured with the wrong identity: the operator token (
FORGEJO_API_KEY, loginaaron,is_admin: false), for which 7jesus/*-style repos answer 403 — repos aaron cannot write. The lock's reads run with the landing credential, andpi-landinghaswriteon every repo it can land in (ghostty-in-the-shell,infra-core,lab-director,prod,pi-extensions), and is absent from the twoSendrast/*repos where it is also outsidemerge_whitelist.GET /branch_protectionsneeds writer, not admin (jesus/Transponder:push: true, admin: falsereads 200). The wedge set is empty, so the refusal is affordable. The refusal names the base branch, says the named set is UNKNOWN, not empty, and gives both remedies (retry; or a human merges from the Forgejo UI).F1 — an absent/unparseable
base.refrefuses. A rule cannot be selected without a branch, and "no rule" is not a claim that can be made about a branch nothing named. Round 1 reportedno branch-protection rule for "(unknown base)"and permitted — and in one probe went as far as issuing a merge POST.F2 — the queue shows it. The census row carries
requiredCiand prints it asREQUIRED-CI …on every row, refused ones included:7/7 on "live",5/7 ABSENT …, orUNKNOWN — … could not be read.LandingLockResult.requiredContextscarries the assessment on every result (permit, leg-1 refusal, and refusals from later legs).F3 — rule selection. An exact-name rule now wins over a glob that also matches (Forgejo's own order), so a pattern cannot steal the decision from the branch's own rule. The glob model implements
*/**only; a pattern using?,[...]or{...}reports the model's limit and refuses, so it can never read as "no requirement".F4 — the helper's preview.
forgejo/verdict.ts's lock preview now carriesrequiredContexts, and the field is required onLandingLockInputand onLockState— an omitted field used to mean "the named-context check does not run", which is exactly how the preview went silent on an absent-context refusal.verdict-clireads the base branch's protection for that state (a GET) and the report printsrequired-ci: ….F5 — tests. Malformed bodies (200 with a non-array body ⇒ refused, never "no rules") and the empty-
status_check_contextsrule (enforcing, names nothing ⇒ permit, and the note says so) are both covered.Round-1 gains kept: the absent-context refusal naming every missing context, the 7/7 permit, the present-but-red refusal in its pre-existing wording, base-branch selection that never borrows another branch's rule, the
200 []no-requirement path byte-identical, andlistBranchProtectionsthrowing on a malformed body. Legs 2/3/4/5, the session panel, the legacy owner panel and hold withdrawal are untouched; the only forge call this feature adds is still a GET (G6).Red → green:
working-files/homelab-2518/HOMELAB-2524-red2.log(15 failed | 48 passed across the two touched files, at round-1 head17c2a5f) and…-green2.log(tsc --noEmitclean, 21 files / 601 tests passed; round 1 was 21 / 587).Round 2 refusal — an unreadable protection read
Round 2 refusal — unknown base branch
Round 2 refusal — a pattern the lock cannot evaluate
Round 2 behaviour notes worth a reviewer's attention
CI is not green on the head commit …), and a refusal is only ever added to a head that would otherwise have passed.LockState.requiredContextsbeing required forced every fixture in the suite to declare what it read. Fixtures that are not about branch protection now use an exportedNO_PROTECTION({ baseRef: "main", read: "rules", rules: [] }) — thepi-extensionsshape — rather than an omission.still refuses when the whitelist cannot be read) can no longer be reached with a protections GET that never worked, because the lock now refuses first. It is driven by a transient failure instead (branchProtectionsFailAfter: 1) so thewhitelist could not be readwording keeps its coverage, and the reason is written beside the fixture.Red → green evidence
Logs:
working-files/homelab-2518/HOMELAB-2524-red.logand…-green.log.Red (test file against unmodified
origin/main72be4ec):8 failed | 3 passed. Failing assertions, verbatim:The two "refuse" lines are the ticket's red case: on
mainthe merge was allowed with a required context absent.The 3 that passed on
mainare the deliberate no-regression guards: present-but-red keeps the existing wording, and a non-green head still refuses with no rule and with an unreadable read. The 6 pure-selector tests came with the implementation (they import symbols that did not exist onmainand would fail as import errors, not assertions).Green:
npx tsc --noEmitclean;npx vitest run→ 21 files / 587 tests passed (baseline onmain: 20 / 570, so 0 regressions, +17 new).Exact refusal for a missing context
Live protection facts (read-only GETs, this session)
GET /api/v1/repos/{owner}/{repo}/branch_protections:enable_status_checkstatus_check_contextsHomelab/pi-extensions200 []Homelab/ghostty-in-the-shelllive200trueci / pr-title (pull_request),ci / check (pull_request),ci / scripts (pull_request),ci / assemble (pull_request),secret-probe / secret-probe (pull_request),emu / emu (pull_request),emu / battery (pull_request)Homelab/ghostty-in-the-shellmain200falsenullA nonexistent repo answers
404. The fixture default intests/fake.tswas changed from404to200 []so the fake matches thepi-extensionsreality;branchProtections: nullremains the explicit unreadable case.Collision check
Open PRs on
Homelab/pi-extensionsat start of work: #68 only (plane/HOMELAB-2321-poll-loop-guard), diffpi-swarm-suite/src/core/index.ts— does not touchforgejo/landing.ts,forgejo/api.ts,forgejo/index.tsordocs/forgejo.md. No collision.Scope
Untouched: legs 2/3/4/5, the dangerous-class session panel, the owner-scoped legacy panel, hold withdrawal, and the existing combined-status refusal. No branch protection was modified; no Plane ticket state changed; no merge, no review, no verdict, no deployment performed by this worker.
Branch
plane/HOMELAB-2524-ci-contexts, commit17c2a5f009c9ffaa77bf91b3f4262480144270c7(model: launched pinned toomniroute/deepseek-flash).Round 3 — the second review's blockers (commit
fa52db9)One reviewer approved round 2, one requested changes. Both blockers and all five non-blocking findings are addressed in a single commit on the same branch (fast-forward push, no rebase, no force). The two refusals from round 2 are unchanged in behaviour — only the text changed.
BLOCKER H2524-B3 — the verdict helper no longer aborts on a 403
verdict-cliread the base branch's protection for its lock preview and re-threwAuthRefused, so a 401/403 exited 2 before composing:Deployed, that is fatal:
GET /branch_protectionsrequires repository admin, andld-revieweris not an admin ofpi-extensionsorghostty-in-the-shell— so the fleet's only reviewer identity could post no verdict at all. No verdict, no landing.Mechanism.
readRequiredContexts()no longer has anAuthRefusedre-throw at all. Every outcome of that read — 401/403, 404/5xx, a non-array body, a rule that names no branch, or a timeout — is classifiedread: "unreadable"and returned. The helper then composes and posts exactly as it does on a repo with a readable rule, and the unknown is surfaced:required-ci: branch protection on "main" could not be read (… 403 …): the named required CI contexts are UNKNOWN, not emptywarning: the landing lock would still refuse after this comment: … UNKNOWN, not empty …The merge gate still refuses on an unreadable read (round 2, B1). Only the preview degrades.
End-to-end proof (fixture, reviewer token path, no real review posted).
tests/verdict-694-replay.test.ts,FakeOpts.protectionsHttp: 403, run throughrunCliwith--token-file -(the reviewer token FILE path — the token arrives on stdin, never argv) and--post:## Verdict: APPROVED,commit_id= the head);required-ci: … UNKNOWN, not emptyin the report lines, the lock-preview warning present,lock.requiredContexts.unknown === "protection-unreadable";authentication refusedappears nowhere on stderr.A companion test pins the other half: with the PR read itself refused (
denyAuth), the helper still exits 2 and posts nothing — only the protection read degrades.BLOCKER H2524-B2 — the refusal now states only what was measured
Round 2 claimed the read "runs with the landing credential, which has write on every repo it can land in, and GET /branch_protections needs writer (not admin)". Both halves were wrong: the read uses
FORGEJO_API_KEY(the session's Forgejo credential), and the endpoint requires admin —jesus/Transponderanswers 403, not 200. That paragraph is gone.Reads I made this session to justify the new text (read-only GETs, no writes, nothing printed from the credential):
GET /api/v1/user200,login: aaron,is_admin: falseGET repos/Homelab/pi-extensions/branch_protections200 []GET repos/Homelab/ghostty-in-the-shell/branch_protections200(rulelive, 7 contexts)GET repos/Homelab/infra-core/branch_protections200GET repos/Homelab/lab-director/branch_protections200GET repos/Homelab/prod/branch_protections200GET repos/jesus/Transponder/branch_protections403—user should be an owner or a collaborator with admin write of a repositoryThe new rationale (verbatim, from the refusal):
The words landing credential, needs writer and has write on every repo are gone, and no identity other than the one I measured is named. A test asserts exactly that (
not.toMatch(/landing credential/),/needs writer/,/pi-landing/,/ld-reviewer/,/has write on every repo/) and that the measured facts are present.Non-blocking findings
requiredContextsreally is on every result now. It was documented "on every result" but carried only on the leg-1 refusals, so the census segment vanished on rows that refused earlier (red/pending head) or later (no verdict yet). The field is now required onLandingLockResultand every return path carries it — pending-CI, no-status, non-green, no head sha, undateable head, no-verdict, unreadable-file-list and dangerous-class refusals included (plus theirrecord). Pinned by tests, including a census row that refused on a red head and still printsREQUIRED-CI UNKNOWN — ….base.ref, never a display label. NewbaseRefFromPR(): the ref wins when present; a label is accepted only when it already is a branch name (noowner:qualifier).Homelab:main— the Gitea label shape — now answers"", so F1's absent-base-ref refusal fires instead of "no rule matched ⇒ permit". Used by both the merge gate (index.ts) and the helper; the merge-level and CLI-level tests are added.200 [{}]is malformed, not "no rules". A rule object that names no branch (or a non-object entry) is treated exactly like the non-array body:listBranchProtectionsthrows, the helper returnsunreadable, and the lock refuses.200 []still permits, byte-identical (pinned by its own test).assessRequiredContextsno longer returnsundefinedfor a missing read. A dropped field, areadvalue that is neitherrulesnorunreadable, or arulesfield that is not a list is assessed as UNKNOWN ("the lock was called without a branch-protection read") and refuses. The old test that asserted the no-op (toBeUndefined()) is replaced by one that asserts the refusal; a further test feedsnull/strings/garbage and requires a refusal without a crash.request()(extension) andapiCall()(helper) now bound each request withAbortSignal.timeout(FORGEJO_REQUEST_TIMEOUT_MS), default 30 s. A hung protection GET therefore becomes a failed read ⇒unreadable⇒ refusal, not a hang. In the extension the bound is applied to reads only (GET/HEAD): a write that times out has an ambiguous outcome — it may have landed — and aborting a merge POST would turn "I do not know" into "it failed", which is exactly what the merge read-back exists to prevent (HOMELAB-2526). In the helper it applies to every call, because the write path already classifies an in-flight failure as "the review MAY exist; reconcile by id" (HOMELAB-2566). Proved by two tests that stub a fetch which never settles: with the bound the lock refuses and the helper composes, in ~60 ms.Round-2 behaviour kept
B1's refusal, F1/F3's fail-closed shapes, exact-name-beats-glob, the red-head ordering (
CI is not green on the head commit …still speaks first, and its wording is unchanged),200 []still permitting, and no protection write anywhere — the feature adds oneGETand nothing else (G6). Legs 2–5, the session panel, the legacy owner panel and hold withdrawal are untouched.Red → green
Logs:
working-files/homelab-2518/HOMELAB-2524-red3.logand…-green3.log.c9eaa10):19 failed | 45 passedacross the two touched files — including the B3 test exiting 2 withauthentication refused, and both N5 tests hitting their 5 s timeout because the read had no bound at all.tsc --noEmitclean;npx vitest run→ 21 files / 621 tests passed (round 2 was 21/601, so +20 tests, 0 regressions). The same suite in the deployed shape (scripts/test-in-deployed-shape.sh,npm cifrom the package lock):21/21 files loaded and passed, 621/621 tests.required-ci: no branch-protection rule for "main" …, and exited 0.Scope
No merge, no review, no verdict and no deployment were performed by this worker; no comment was edited; no branch protection was read beyond the GETs listed above, and none was written. Branch
plane/HOMELAB-2524-ci-contexts, headfa52db9(model: launched pinned toomniroute/deepseek-flash).Verdict: APPROVED
model: omniroute/deepseek-flash
head:
17c2a5f009session: 01a1a5e7-883d-7446-a658-25084ff9de9e
Independent seat 1. The completeness leg is real, the wording split is honest, and I reproduced GITS-368 through the shipped code with live data rather than through the PR's own tests. Recorded non-blocking findings below; none of them stops this landing.
Ruling on the unreadable-read behaviour: acceptable, non-blocking — and I reversed my own starting position after measuring. I began from fail-closed, because this same leg refuses when the commit-status read fails ("The landing lock fails closed when it cannot read CI state") and two policies for one class of evidence is normally a defect. Then I measured the thing the design turns on: with the exact credential the leg uses (
FORGEJO_API_KEY, loginaaron, non-admin) I readGET /repos/{owner}/{repo}/branch_protectionsacross all 45 repos that credential can see — 38 answer 200, and 7 answer 403:jesus/infra-core,hamilton/infra-core,jesus/dyno,jesus/Transponder,jesus/hermes-automation,hamilton/hermes-automation,jesus/project_dino. A 403 there is a stable permission state, not a blip: the credential is not the owner/admin of those repos, so the named check can never succeed on them, and a fail-closed rule would permanently refuse every agent landing on 15% of the fleet — the legacy-owner repos this policy exists to protect among them, where the sanctioned escape (a human merging in the Forgejo UI) is exactly what the fleet is trying to stop needing. With that measurement the skip dominates: it costs nothing where the read works (38/45, verified live including theliverule this ticket is about) and it preserves today's gate byte-for-byte where enforcement is impossible. The two legs are also not the same shape, which is what I had missed: an unreadable CI read leaves no evidence at all, while an unreadable protection read leaves the combined status, the exact-head verdict panel, the danger class and the open holds intact. I checked that the degrade is stated rather than dressed up — a passing landing readsbranch protection on "live" could not be read (Forgejo API GET .../branch_protections: 404), so the named required CI contexts were NOT enforced — leg 1 decided on the combined status alone, the record carriesread: "unreadable", and a red or empty head is still refused in every probe. The one improvement I would still ask for is visibility, not policy: see finding H2524-F2.The enforcement is real; I reproduced GITS-368 against the live rule and the live head. I fetched
Homelab/ghostty-in-the-shellbranch protection live (live→enable_status_check: truewith 7 contexts,main→false/null) and the live combined status of PR #37's head2f7bb87b80b583c8ce41c7820b6efe97b94bf554(success, 5 contexts,emu / emuandemu / batteryabsent). Fed through the shippedreadRequiredCiContexts+evaluateLandingLock, that head is REFUSED and bothemucontexts are named. The same live rule with a complete 7-context head permits, with the noterequired CI contexts on "live": 7/7 present on the head, and the livemainrule permits withdoes not enforce status checks. I also checked for name drift, which is the way this kind of check breaks production: the six most recentlive-based heads carry the 7 required contexts verbatim, so enforcement will not refuse a complete head.Four behaviour probes, driven end-to-end through the registered
forgejo_merge_prtool (route-table fake), each verified by merge POST count. (1) combinedsuccesswith 2 of 7 required contexts absent → REFUSED, 0 POSTs, both absent contexts named, message states that a combined status cannot see a context that never ran. (2) all 7 present and green → PERMITTED, 1 POST, note 7/7. (3) a required context present but red → REFUSED with the pre-existingCI is not green on the head commit (state: failure, source: combined)plusnon-green contexts: emu / battery (pull_request) (failure), and never the absent wording; a required context present but pending likewise keeps the oldCI is still runningwording. (4) base-branch selection: a rule forlive-2on a PR based onliveis not borrowed (permitted), baselivein a repo whose other rule ismainrefuses and names all 6 absent contexts, the same repo with basemainand status checks off permits, and arelease/*pattern governsrelease/2.0but notmain. Extras: two absent plus one present-but-red refuses with the absent wording and lists the non-green context; a malformed 200 protections body is treated as unreadable rather than as "no rules"; a head with no statuses at all is still refused even when the protection read failed.No-protection regression: this repo's own shape is unchanged.
Homelab/pi-extensionsanswers200 []live, and both probes match the pre-ticket gate — combined success permits (no branch-protection rule for "main": no named required CI contexts, the combined status decided leg 1), combined failure refuses with the existing non-green message. No new refusal can reach the fleet's busiest repo, because an empty list yieldsrequired: []and the completeness check is skipped.Nothing else moved.
landing.tsgains one optional input field, one optional record field, one note line and the completeness block placed ahead of the existing state check; legs 2-5, the session panel, the legacy owner panel and hold withdrawal are untouched hunk for hunk.api.tsonly adds two rule fields, the read-only reader, the exports and the non-array throw, anddescribeMergeWhitelistalready catches that throw and keeps reporting unread honestly. There is no branch-protection write anywhere: the only protection request is the GET inlistBranchProtections, and the new test asserts zero non-GET calls to/branch_protections. No secret in any argv, no writes outside the worktree, nothing in~/.pi/agent/extensions, no Plane or GITS write.Tests and red-first evidence.
npx tsc --noEmitclean;npx vitest run21 files / 587 tests green;git statusclean at17c2a5f0. The red log shows 11 tests, 8 failing, 3 passing: the 3 that pass are exactly the tests that assert refusals the old code already made (present-but-red, non-green under no protection, non-green under a failed read), and the 8 are genuine assertions of the missing behaviour — two "the lock permitted where it must refuse", five note-wording assertions, one "zero protection GETs". The 6 selector tests are absent from that log, which matches the worker's disclosure that they would have been module-level import errors; that is honest red-first evidence for the behavioural leg, and the selector block simply has no red evidence.Note — H2524-F1: an absent base ref is reported as "no rule", a negative the lock has not established.
When
pr.basecarries neitherrefnorlabel,baseRefbecomes""and the lock permits withno branch-protection rule for "(unknown base)": no named required CI contexts, the combined status decided leg 1; probe P10 issued the merge POST. Unlike the 403 repos, an absent base ref is not a permission wall — it is a malformed PR read, and the same file already refuses a missing head sha ("the API reported no head sha, so CI and the verdict record cannot be checked"). Suggest refusing for an empty base ref, or at minimum wording itthe base branch could not be determined, so no rule could be selectedinstead of claiming no rule exists.Note — H2524-F2: the degradation is invisible where coverage matters most.
RequiredContextsAssessment.read: "unreadable"reaches the landing note and the record, but the census row shape inindex.tshas no field for it, so on the 7 repos that answer 403 the queue showscanMerge: truewith no hint that the named check was skipped. Adegraded/exactnessUnknowncolumn (or a line in the row's reason) would put the fleet's weakest coverage where the operator actually looks.Note — H2524-F3: rule selection takes the first match and models only
*.findover the rule list means a repo holding bothrelease/*andrelease/2.0enforces one of them rather than the union, and a selector shaperuleCoversBranchdoes not model reads as "no rule for this branch" rather than "a rule exists but did not match". Neither is wrong for today's shapes (both live rules use exact branch names) and both fail in the permit direction, so they deserve a documented limit or a union test before a pattern rule is used in anger.Note — H2524-F4: the reviewer helper's lock preview does not run the new leg.
verdict.tscallsevaluateLandingLockwithoutrequiredContexts, so the helper's "the landing lock would still refuse after this comment" preview stays silent on an absent-context refusal and the reviewer meets it only at merge time. Advisory only, since the helper never gates posting, but threading the same evidence through it would make the preview honest.Verdict: REQUEST_CHANGES
model: omniroute/deepseek-flash
head:
17c2a5f009session: 01a1a5e7-886c-7446-a658-250b4d3cffa9
What I measured at this head
npx tsc --noEmitis clean andnpx vitest runis green at 21 files / 587 tests in/home/aaron/repos/pi-extensions/.worktrees/HOMELAB-2524-ci-contexts/forgejo, withgit status --porcelainempty before and after my run, and CI run #236 is green on 14/14 contexts at this exact head.The live shapes the design rests on are real:
Homelab/pi-extensionsanswers200 [], andHomelab/ghostty-in-the-shellcarrieslivewithenable_status_check: trueand exactly the seven contexts the tests pin, plusmainwithenable_status_check: falseandstatus_check_contexts: null.The exactness of the name comparison is right, and I could not break it: the combined
/statusof a real ghostty PR head (cb63530, PR #155) reportsci / pr-title (pull_request),ci / check (pull_request),ci / scripts (pull_request),ci / assemble (pull_request),secret-probe / secret-probe (pull_request),emu / emu (pull_request),emu / battery (pull_request)— verbatim identical to the protection rule'sstatus_check_contexts, spaces and(pull_request)qualifier included, so an exact trimmed comparison is the correct rule on this forge and any normalisation would be a guess in the wrong direction.Blocker — H2524-B1: an unreadable protection read permits the landing, and the reason given for that fail-open does not survive measurement
assessRequiredContextsreturnsenforced: falsewhenreadis"unreadable", so a 403, a 404 or a 5xx onGET /repos/{o}/{r}/branch_protectionsdegrades leg 1 to the combined status alone and the merge proceeds — pinned by the test "permits but records that the named requirement was NOT enforced", which assertsmergeCalls(calls)is 1.BLOCKER OPEN: H2524-B1
The documented justification is that refusing "would turn an infrastructure read failure into a permanent merge blocker on every repo whose list the session's credential cannot read", with no override to escape it; both halves are false in this fleet and I measured them.
Writer, not admin, is what the endpoint requires:
jesus/Transponderanswers200to my credential withpermissions: {admin: false, push: true}, whilehamilton/hermes-automation,hamilton/infra-core,jesus/hermes-automationandjesus/infra-coreall answer403"user should be an owner or a collaborator with admin write of a repository" to the same credential withpush: false.The landing identity is a writer on every repository it can land in:
pi-landingispermission: "write"onHomelab/ghostty-in-the-shell,Homelab/infra-core,Homelab/lab-director,Homelab/prodandHomelab/pi-extensions, andpermission: "none"onSendrast/sendrast-webandSendrast/api-sendrast, where it is also absent frommerge_whitelist_usernames: ["aaron","jesus"]and therefore could not land even if the lock permitted.So the set of repositories where the protection read can fail while a landing could still happen is empty, and the wedge this design is built to avoid has no member in this fleet: a credential that cannot read the list is a credential that cannot land.
A refusal is not unappealable either, because the escape already exists for every other refusal this lock can make — this project's own docs name it, "a human can still merge from the Forgejo UI" (
docs/forgejo.md), and the forge itself enforcesenable_status_checkat merge for a non-admin landing identity underapply_to_admins: falseon all four protected Homelab repositories, which is how "Not all required status checks successful" reaches this fleet's records at all.Against that, the cost of permitting is exactly the thing the epic exists to kill: GITS-368 is leg 1 deciding green from a combined status that could not see the contexts that never ran, and an unreadable protection list is the one state in which this ticket cannot tell whether it just made the same call again.
What I would implement instead
Return a refusal when
input.read === "unreadable", in the shape the missing-context refusal already has: name the base branch, carry the one-line reason, say plainly that the named set is unknown rather than empty, and give the two remedies — retry once the read answers, and the human merging from the Forgejo UI.Keep the readable-but-empty list (
200 [], the pi-extensions shape) exactly as it behaves today, because that one is a measured fact and not a gap.Two tests: unreadable plus green combined status means refusal with zero merge POSTs; unreadable plus red combined status keeps the existing non-green wording.
Findings that are not blocking
H2524-F1— nothing in the PR, the tests or the ticket shows that the restricted landing identity can read the protection list at all;pi-landingisrestricted: trueand I cannot authenticate as it, so if that read 403s in production then every protected landing takes the "NOT enforced" path and the ticket delivers nothing while saying so quietly in a note nobody reads — refusing under H2524-B1 is what makes that visible on the first landing instead of never.H2524-F2— rule selection isrules.find(...), that is, list order, while Forgejo resolves a matching exact-name rule ahead of a pattern rule, so a repository carrying*with checks off ordered beforelivewith checks on would enforce nothing here and seven contexts at the forge; no live repository has overlapping rules today (the two-rule sets are{main, epic/**}and{live, main}, disjoint), so this is latent rather than live.H2524-F3— the glob translation turns*into.*, which is looser than the forge in one direction and narrower in another:release/*coversrelease/2.0/hotfixhere but not at the forge (over-match, the fail-closed direction),epic/**coversepic/HOMELAB-123correctly (verified against the two live Sendrast rules), and a rule written with?or[...]and no*covers nothing, which is the fail-open direction.H2524-F4—verdict.ts:727calls the sameevaluateLandingLockwithoutrequiredContexts, so the reviewer-side "what the merge gate would say about this PR once the comment is on it" preview keeps the pre-2524 leg 1, and becauserequiredContextsis optional onLandingLockInputno future caller is forced to supply it; the preview can only under-report, but a preview that stays silent while the gate refuses is the same class of record problem this ticket is otherwise careful about.H2524-F5— two handled states are unpinned: a malformed 200 whose body is not an array, which the new throw inlistBranchProtectionsturns into the H2524-B1 path, andenable_status_check: truewith an emptystatus_check_contexts, which the code correctly reads as "names no contexts"; neither has a test, and the fake cannot produce a non-array body at all.Routes I tried to slip a missing context through
A different qualifier form (
ci/checkagainstci / check (pull_request)) — does not slip, the comparison is exact and I verified the two sides are character-identical on a live head, so this is the strongest part of the change.A required context present under a slightly different name — does not slip, for the same reason, and the trim is the only normalisation and it is harmless.
A base branch with no rule while the default branch has one — does not slip into a false green, it permits, and that permit is the forge's own answer because a rule for another branch names nothing for this one (live: ghostty
mainis checks-off andliveis checks-on, so a PR tomainmay land on one green context and Forgejo agrees).Protection only on
mainwhile the PR targetslive— permits, and again matches the forge, because themainrule does not coverliveand the forge requires nothing there either; this is the one shape where permitting is correct and it is documented as such.Empty
status_check_contextswithenable_status_check: true— permits, correctly, and the summary says "enforces status checks but names no contexts" rather than claiming an enforcement that does not exist.A malformed protection body — slips, but into the same gap as H2524-B1 rather than past the new check: the throw in
api.tsis real and does turn a fabricated[]into an honest "unreadable", yet the gate outcome is unchanged, which is precisely why the fix belongs inassessRequiredContextsand not in the reader.An in-flight head whose required contexts are queued and not yet reporting is refused as "absent from the head", which is the safe direction, but the wording says "absent" where "not yet reported" would be true — a message nit, not a hole.
Nothing else moved
Legs 2 to 5, the session panel, the legacy owner panel and hold withdrawal are untouched: every hunk in
landing.tsis additive around leg 1,requiredContextsis only carried into the record and the landing note, and no rule, hold, ledger or panel function changed — which the untouched existing suites confirm (587 tests, includinglanding-hold-session,landing-blockers,landing-sessions,landing-model-supersede).No branch-protection write exists in the code or the tests: the only new forge call is a
GET .../branch_protections, and the G6 test asserts exactly one GET on that path and zero non-GET calls touching it — I re-ran it at this head and it passes.Red evidence
The log at
HOMELAB-2524-red.logis fair red-first evidence and not padding: 11 tests, 8 failed, and the three that passed are regression guards (a non-green combined status still refuses, with and without a readable protection list), not selector imports — there is no import error anywhere in it, and two of the red cases show the pre-fix lock actually permitting the merge of an incomplete head with zero merge POSTs asserted after.The file has grown to 17 tests since that run, and the added cases strengthen the claim rather than dilute it.
Hygiene
Read-only throughout: no code edit, no merge, no deploy, no branch-protection write, no Plane or GITS write, no write to
~/.pi/agent/extensions, and the review credential was read from OpenBao into a 0600 file inside a 0700 directory that is removed afterwards — never in argv, never printed.Round 2 of the ticket, implementing the orchestrator's ruling on the review. Round 1 read the base branch's `status_check_contexts` and enforced them by name, but it permitted on the combined status whenever that read failed, and it reported "no rule for (unknown base)" for a base branch it never learned. B1 — an unreadable protection read REFUSES. Round 1's defence was a wedge argument measured with the wrong credential: the operator token (`aaron`, `is_admin: false`), for which 7 `jesus/*`-style repos answer 403. The lock's reads run with the landing credential, and `pi-landing` has `write` on every repo it can land in (`ghostty-in-the-shell`, `infra-core`, `lab-director`, `prod`, `pi-extensions`); `GET /branch_protections` needs writer, not admin (`jesus/Transponder`: push true, admin false, reads 200). The wedge set is empty, so the refusal is affordable. It names the base branch, states that the named required set is UNKNOWN rather than empty, and gives the two remedies (retry; or a human merges from the Forgejo UI). F1 — an absent/blank `base.ref` REFUSES. A rule cannot be selected without a branch, and "no rule" is not a claim the lock can make about a branch nothing named. Round 1 reported "no rule for (unknown base)" and permitted, and in one probe went as far as issuing a merge POST. F3 — rule selection: an exact-name rule now wins over a glob that also matches, which is Forgejo's own order. The glob model implements `*`/`**` only, so a pattern using `?`, `[...]` or `{...}` reports the model's LIMIT and refuses; it can never read as "no requirement". F4 — the verdict helper's lock preview carries `requiredContexts`, and the field is REQUIRED on `LandingLockInput` and `LockState`: an omitted field used to mean "the named-context check does not run", which is exactly how the preview went silent on an absent-context refusal. `verdict-cli` now reads the base branch's protection for that state. F2/F5 — the census row carries `requiredCi` (`7/7`, `5/7 ABSENT …`, or `UNKNOWN — …`) and prints it as `REQUIRED-CI …` even on a refused row; malformed (200, non-array) bodies and the empty-`status_check_contexts` rule are covered by tests. Round-1 gains are kept: the absent-context refusal and its naming of every missing context, the 7/7 permit, the present-but-red refusal in its pre-existing wording, base-branch selection that never borrows another branch's rule, the `200 []` no-requirement path byte-identical, and `listBranchProtections` throwing on a malformed body. Legs 2/3/4/5, the session panel, the legacy owner panel and hold withdrawal are untouched, and the only forge call this feature adds is still a GET (G6). Tests: 587 -> 601 (red2 captured before the change, green2 after); tsc clean.Verdict: APPROVED
model: omniroute/deepseek-flash
head:
c9eaa1027csession: 01a1a61d-45de-7446-a658-2515468e24dc
Round-2 seat 2, independent of the other seat. I re-derived every round-2 claim from the code, from my own probes against the real
evaluateLandingLock/assessRequiredContexts/readRequiredCiContexts, and from read-only GETs to the live forge, and I did not take the description's measurements on trust.The central question was whether any route still turns "the named set is unknown" into "no requirement, permit". I could not find one that fires against this forge, and the three routes B1 named are closed; five residual gaps are listed as non-blocking findings below, none of which issues a merge the live API can reach.
GITS-368 reproduced by measurement, not by description
GET /repos/Homelab/ghostty-in-the-shell/pulls/37→ head2f7bb87;GET /repos/.../commits/2f7bb87/status→state: successover exactly five contexts —ci / pr-title (pull_request),ci / scripts (pull_request),ci / check (pull_request),ci / assemble (pull_request),secret-probe / secret-probe (pull_request)— whileGET /repos/.../branch_protectionsreports rulelivewithenable_status_check: trueand sevenstatus_check_contexts, the twoemuones absent from that status set.The required strings and the reported strings are byte-identical (both carry the
(pull_request)suffix), so exact-name matching is the right predicate here and this refusal is the one that would have caught #37; a fuzzy or suffix-stripping match would have been the wrong fix.Homelab/pi-extensionsreally does answer200 [], andghostty-in-the-shellreally does carry a second rulemainwithenable_status_check: false, so the two no-requirement shapes the design leans on are the live shapes and not fixture conveniences.HOMELAB-2524-B1 is resolved
H2524-B1 is resolved by
c9eaa10: an unreadable protection read now yieldsread: "unreadable"andunknown: "protection-unreadable", and leg 1 refuses on it instead of degrading to the combined status.blockers resolved
Measured over the transport surface, not just the 404 the tests use: 403, 401, 404, 500, a fetch rejection (the shape a timeout or a dropped connection takes), a 200 whose body is a non-array object, a 200 with an empty body under
content-type: application/json, a 200 with an empty body and no content type, a 200 whose body isnull, and a204all classifyread: "unreadable"and all refuse; only200 []classifiesread: "rules"and permits. I did not find a write path — every protection call the extension makes is a GET, asserted in my probes and in the suite's G6 test.The two legs do not contradict
Proven by probe on the real evaluator: a red head with every required context present keeps the pre-existing
CI is not green on the head commit …wording; a red head whose protection read ALSO failed keeps that same red wording (the UNKNOWN refusal is suppressed, exactly as the ordering intends); a green head missing a required context gets the absent-context refusal naming each missing context; a green head whose protection read failed gets the UNKNOWN refusal; a green head with an absent base ref gets the base-ref-absent refusal; a green head with every context present permits.One nuance worth stating rather than hiding: the round-1 ABSENT refusal sits BEFORE the combined-status check, so a head that is both red and incomplete gets the absent-context wording (it still prints the
non-green contexts:detail), notCI is not green. That is deliberate — one refusal per GITS-368 fact — and it does not contradict the round-2 claim, which is only about the two NEW refusals sitting after the combined-status check.200 []still permitsEnd-to-end probe with the
pi-extensionsshape (readable empty list, green head, fresh verdict) issues the merge POST once and the note records that the combined status decided. No regression on the busiest repo, which was the risk that would have blocked every landing in the fleet.Table — unknown-shaped input → outcome (all measured)
unreadable→ REFUSE (UNKNOWN)unreadable→ REFUSE{"rules":[]}unreadable→ REFUSEunreadable→ REFUSEnullunreadable→ REFUSEunreadable→ REFUSE[]rules, no rule → PERMIT[{}](array of non-objects)rules, branch""→ PERMIT (finding N3)release/*vs basemain*li?e/rel{a,b}ase/[mn]ainpattern-unevaluable)live(checks off) + unevaluable siblingli?emainchecks off +*sibling carrying contextsbase.ref, no labelbase-ref-absent)base.ref=""or whitespace onlybase-ref-absent) — trim handles the blank casebase.ref, label presentMainvs rulemainrefs/heads/mainvs rulemainrule_name: "live","main")requiredContextsomitted at runtimeNon-blocking findings (no
BLOCKER OPENline for any of these)H2524-N1 — the census and preview do not carry the named-CI state on every result, contrary to the record.
LandingLockResult.requiredContextsis documented as present "on EVERY result", anddocs/forgejo.mdsays "A refused row also prints leg 1's named-CI state"; measured, it is absent on the three combined-status refusals (red, still-running, no-status) and on the leg-2+ refusals, which is why a census row for an ordinary unreviewed PR prints noREQUIRED-CIsegment even when the protection read failed, and why the deployed helper's live preview on #91 prints norequired-ci:line. The specific state this hides is "red head AND the protection read never worked", which is exactly the state F2 exists to expose; it is under-reporting only (no false permit, no false claim of enforcement), and the fix is to attach the assessment where it is computed instead of on selected returns.H2524-N2 — the base ref falls back to
base.label, which makes F1's "an absent base.ref refuses" conditional.index.tsandverdict-cli.tsboth derivepr?.base?.ref ?? pr?.base?.label ?? ""; whenrefis absent and a label exists the lock no longer refuses, and with a label that is not the branch name (upstream Gitea'sowner:branchshape) the derived ref matches no rule and the head permits — my probe with labelHomelab:liveissued the merge POST with two of four required contexts absent. On THIS forge the fallback is harmless because I measuredbase.labelequal tobase.ref(main,live), so this is a latent hazard and not a live fail-open, and the F1 test covers onlybase: undefined, the one shape where the fallback cannot fire; recommend dropping the fallback or refusing when the derived ref cannot be a branch name, and pinning the label-present shape.H2524-N3 — an array of non-objects still reads as "rules". A
200whose body is a non-array refuses (correct, and pinned), but200 [{}]maps to one rule with an empty branch name and therefore to "no rule" and a permit; same "malformed is not empty" class as the case the PR does pin, worth the same treatment.H2524-N4 — "required" is a compile-time property only.
LandingLockInput.requiredContextsandLockState.requiredContextsare required fields, which is a real improvement, but a runtime caller (a parsed JSON state, a future JS caller) that omits them getsassessRequiredContexts(undefined) → undefinedand leg 1 silently skipped — the exact behaviour F4 was added to remove; a runtime refusal when the field is absent would close it.H2524-N5 — a hung protection GET hangs the lock.
request()has no timeout, so a connection that never answers waits rather than refusing; this is pre-existing for every read the lock makes and a hang is not a permit, so it is an observation, not a defect of this change.Ruling on the fixture collateral (the change I was asked to check hardest)
The re-driven HOMELAB-2578 test has NOT been hollowed out. Driven through the same fake OpenBao credential the test file uses: with the transient failure (
branchProtectionsFailAfter: 1) the refusal containswhitelist could not be read, namesaaronandpi-landing, and issues zero mutating requests; with the failure removed the wording is absent while the identity refusal is still present, so the assertion is load-bearing rather than vacuous; and with the ORIGINAL shape (branchProtections: null, protections never readable) the gate now refuses at leg 1 with the UNKNOWN wording and zero writes, so that shape is covered too — by the new leg-1 test rather than by the identity leg. The only coverage that moved is the conjunction "permanently unreadable protections AND identity mismatch", which is unreachable by design now and is documented beside the fixture, and every other suite that needed the field declaresNO_PROTECTION, which is a declared read rather than an omission.Tests, CI and hygiene
npx tsc --noEmitclean andnpx vitest run21 files / 601 tests passed atc9eaa10in the worktree,git statusclean before and after my probes (I added no file that survives this review); CI run #238 is green 14/14 on this exact head.The red log is honest: 15 failed / 48 passed across the two touched files at round-1 head, and the failures are behavioural rather than import accidents —
expected 'live*' to be 'live'shows the round-1 glob-first ordering really did let a pattern steal the rule, and the leg-1 refusals really did permit. I verified the fix's own claim on the live forge rather than assuming it, which is how I found N2.No secret is in argv anywhere in my run, no write went outside the worktree, nothing was written to the deployed extension tree, and I made no Plane or GITS write and no branch-protection write of any kind.
Verdict: REQUEST_CHANGES
model: omniroute/deepseek-flash
head:
c9eaa1027csession: 01a1a61d-45b2-7446-a658-2512dbf509ab
Independent seat 1 at
c9eaa10. The B1 fail-open is genuinely gone and F1/F3 cannot fail open in any shape I could construct, so the enforcement this ticket exists for is real. Two things block the landing, and neither is the enforcement: the rationale the refusal now ships is contradicted by measurement (H2524-B2), and F4's new protection read stops the reviewer helper from posting at all on the repo under review (H2524-B3, measured end-to-end with the sanctioned reviewer credential).What I measured, so the rest can be checked.
npx tsc --noEmitclean andnpx vitest rungreen at 21 files / 601 tests in.worktrees/HOMELAB-2524-ci-contexts/forgejo,git status --porcelainempty before and after my run, CI run #238 green on 14/14 contexts at the full head. I drove the pureforgejo/landing.tsdirectly undernode --experimental-strip-types(an inline probe, nothing written into the tree) with a real verdict-of-record so a "permit" is a true permit and not a leg-2 fallthrough, and I made live read-only GETs with the operator credential and with the sanctioned reviewer credential read from OpenBaokv/data/ld/forgejo-reviewerinside a 0700mktemp -d(bao token self-revoked, token file 0600, never in argv).B1's gate behaviour is real now, and I could not turn an unreadable read back into a permit. unreadable →
unknown: "protection-unreadable"→ refusal naming the base branch withUNKNOWN, not empty, the 404 text and both remedies;200 [](thepi-extensionsshape, read live again this session) still permits end-to-end withok: trueand the noteno branch-protection rule for "main": no named required CI contexts, the combined status decided leg 1; an enforcing rule with 7/7 still permits with7/7 present; 5/7 still refuses naming bothemucontexts. Structurally:requiredContextsis required onLandingLockInputand onLockState, all threeevaluateLandingLockcall sites (index.tsmerge gate,index.tscensus,verdict.tspreview) pass it,readRequiredCiContextsreturns a definitereadstate on every path, andassessRequiredContextsreturnsmissing: []for every UNKNOWN state so the completeness check cannot pre-empt the unknown refusal. The merge tool has no override parameter, so the gate cannot be bypassed. H2524-B1 is resolved byc9eaa10: an unreadable protection read now REFUSES instead of permitting on the combined status, and no caller path I found omits the field that makes it refuse.Blocker — H2524-B2: the refusal's rationale states three facts that measurement contradicts, one of them cited as its own evidence
BLOCKER OPEN: H2524-B2
The shipped refusal says
This read runs with the landing credential, which has write on every repo it can land in, and GET /branch_protections needs writer (not admin), anddocs/forgejo.mdrepeats both halves as the reason the refusal is affordable. Every part of that is wrong on this forge, and I measured it rather than inferring it.First, the read does not run with the landing credential.
listBranchProtectionsgoes throughapi()→headers(cfg)withcfg = loadConfig(), which isFORGEJO_API_KEY(forgejo/api.ts:48-76); the landing credential is resolved only at merge time, for the write, byresolveLandingCredentialAtRuntime. So the identity that reads protections is whateverFORGEJO_API_KEYis in the session — the operator credential in this fleet — and the landing identity never touches this GET.Second, the endpoint needs admin, not writer. The PR cites
jesus/Transponderas proof of the opposite (push: true, admin: false→ 200). I read it:GET /repos/jesus/Transponder/branch_protectionswith the operator credential answers 403 —{"message":"user should be an owner or a collaborator with admin write of a repository"}— and so dojesus/infra-core,hamilton/infra-core,jesus/dyno,jesus/hermes-automation,hamilton/hermes-automationandjesus/project_dino. Independently, with the reviewer credential the fleet actually uses for verdicts:ld-reviewerreportspermissions: {admin: false, push: true, pull: true}onHomelab/pi-extensionsand still gets 403 with the same admin-write message.pi-landingis documented in this very PR aspermission: "write"— that is exactly the class the endpoint refuses.Third, the affordability conclusion survives, but for a reason the PR does not state: I re-ran the sweep with the credential the lock actually uses and all seven 403 repositories have zero pull requests (
state=all→[]on each), so there is no landing there to refuse, while the five Homelab landing repos (pi-extensions,ghostty-in-the-shell,infra-core,prod,lab-director) all answer 200 to that same credential. The wedge is empty because the 403 repos are idle, not because the landing identity is a writer.This is a blocker rather than a nit because the paragraph is the operator-facing explanation of an unappealable refusal, and it directs remediation the wrong way ("a fault to fix, not an entitlement gap", "retry"). Round 1 was blocked on precisely this failure mode — an argument that did not survive measurement — and this head replaces it with an argument whose own cited live evidence is inverted. The fix does not touch the behaviour the orchestrator ruled for: state the credential the read really uses, state that the endpoint requires admin, and keep the (correct) conclusion with its real reason, so the next ticket does not quote
writer (not admin)as measured fact the way round 1's15% of the fleetwas quoted.Blocker — H2524-B3: F4's preview read aborts the reviewer helper before it can post a verdict
BLOCKER OPEN: H2524-B3
Measured, end-to-end, with the sanctioned reviewer credential and this PR's own unmodified code:
node --experimental-strip-types <worktree>/forgejo/verdict-cli.ts --repo Homelab/pi-extensions --pr 91 --head-sha c9eaa102… --event APPROVED --model omniroute/deepseek-flash --session <mine> --body-file <probe> --token-file <ld-reviewer token> --dry-runexits 2 withForgejo API GET repos/Homelab/pi-extensions/branch_protections: 403 Forbidden — authentication refused, stoppingand composes nothing. The deployed helper (pre-round-2, no protection read) with the same token composes the same body and reportsparse: COUNTED — approve at head c9eaa10 (model omniroute/deepseek-flash, author ld-reviewer).The cause is in
forgejo/verdict-cli.ts:api()throwsAuthRefusedon 401/403, andreadRequiredContextsrethrows it (if (err instanceof AuthRefused) throw err;), so a 403 on a read that only feeds an informational preview killscollectStateand with it the whole run. Becauseld-reviewerisadmin: false(measured above: push onpi-extensions, pull-only onghostty-in-the-shell), the read is 403 on both repos the fleet reviews, so with this code deployed the fleet's only trusted reviewer identity can post no verdict at all on them — and with no verdict of record, nothing there can land either. The comment beside the new call saysA failed read — including a 200 whose body is not a list — is unreadable, which is true for 5xx and for a malformed body but false for the most likely failure, the 403.The fix belongs in the helper, not in the entitlement: a preview-only read must never be able to stop the post. Classify 401/403 on this GET as
{ read: "unreadable" }(or skip the read when it cannot succeed) and let the preview state that it is blind — the merge gate's own read, made with the operator credential on the paths it actually lands, stays the enforcement. Granting the reviewer admin to make a preview work would trade a control for a warning.F1 and F3 do not fail open in any shape I tried. Absent
base.ref, a whitespace-onlybase.ref,li?e,li[vo]e,{live,main}each refuse with their ownunknowncause and their own first line; a legitimately matchinglive*and arelease/*rule for a different branch both read as "no named requirement" and reach leg 2 normally; an exact-name rule beats a matching glob (7/7 permits where the glob names a context the head lacks); an enforcing rule with an empty context list permits and saysnames no contexts;"live"vs" live "still matches. Non-falseunknownis the only value that permits, andmissingis empty for every non-falseunknown, so the two refusal paths cannot shadow each other.Ordering holds. Red, pending and empty-state heads keep their pre-existing wording under every UNKNOWN cause —
CI is not green on the head commit (state: failure…),CI is still running on the head commit (state: pending),no CI status is recorded for the head commit— withprotection-unreadable,base-ref-absentandpattern-unevaluableall present in the input; the new refusals only fire where round 1 would have permitted. The one place a round-1 refusal becomes a permit is deliberate and is F3 itself: a glob no longer steals the decision from the branch's own rule (I reproduce round 1's wrong refusalbranch-protection rule on "live*" requires 1 … absent, and it is gone). That direction is safe: no live rule in this fleet is a pattern at all — I swept protections on every repo I can read and all ten rules are exact branch names (ghostty live/main,infra-core live/main,prod live/main,lab-director live) — and where a forge applies a different precedence the merge is still refused by Forgejo's ownenable_status_checkenforcement. The base→head diff oflanding.tsis insertions in leg 1 only: no hunk lands inside legs 2-5, the session panel, the legacy owner panel or hold withdrawal, and the only new forge call anywhere is a GET (noPOST/PATCH/PUT/DELETEtobranch_protectionsexists in the tree).The HOMELAB-2578 re-drive preserves the test's teeth — I judge this honestly rather than waving it through. The assertions are byte-identical to round 1 (
aaron,pi-landing,/whitelist could not be read/i, zero mutating writes); only the trigger changed, from a protections GET that never worked tobranchProtectionsFailAfter: 1so the lock's own read succeeds and the gate's second read — the whitelist it names on a refusal — is the one that fails. All three surviving assertions can only hold together if the lock passed leg 1, the identity leg refused with the live whitelist unreadable, and no write was attempted, so the security property that leg guards (a wrong landing identity never merges, and its refusal still says what it could not read) is still exercised. What it no longer covers is "a repo whose protections read never worked still reaches the identity leg", and that state was removed by design in round 2, not by the test edit — the oldbranchProtections: nullfixture would now be answered by the lock itself, so keeping it would have tested the UNKNOWN refusal twice and the identity leg not at all. TheNO_PROTECTIONfixture sweep is the same kind of change: declared, commented, and adding no behaviour (I diffed every touched test file — the only non-requiredContextsedits are three import lines).The red evidence is genuine, with one honesty nit.
HOMELAB-2524-red2.logshows 15 failed / 48 passed at the round-1 head and the failures are the real ones — fourexpected null not to be nullfrom therefusal()helper (round 1 permitted where a refusal is required), the glob stealing the decision,undefinedwhereunknownshould be, and the missing census column. One assertion in that log readsr.lock.record?.requiredContexts, while the committed test assertsr.lock.requiredContexts, so the log was captured against a slightly earlier draft of the test file than the one at this head; the failure causes are identical and are not vacuous, but the log is not literally the committed file (H2524-N4).Non-blocking findings, for the record. H2524-N1:
LandingLockResult.requiredContextsis carried on the absent-context refusal, the UNKNOWN refusal, the hold refusal, the blocker refusal and the permit, but not on the three combined-status refusals or the two early leg-2 refusals or theno exact-head verdict covers the current diffrefusal — so a census row refused for red/pending CI, or an unreviewed row, prints noREQUIRED-CIsegment even when the named set was read or was UNKNOWN, and the PR body's "on every row, refused ones included" and the doc's "A refused row also prints leg 1's named-CI state" overstate what the queue shows (attaching the already-computed value to those returns would close it). H2524-N2:pr?.base?.ref ?? pr?.base?.label ?? ""substitutesbase.labelfor the base branch in bothindex.tsandverdict-cli.ts; a label likeowner:branchreads asno branch-protection rule for "owner:branch"and permits, which is the F1 shape in miniature — refusing on an absentbase.refrather than substitutinglabelwould close it. H2524-N4: the red-log draft drift above. H2524-N5:describeMergeWhitelistkeeps its pre-existingrules.length === 1 ? rules[0]fallback; it is a reporting helper, not the gate, so no named requirement is borrowed by the lock, but it is worth remembering when the census learns to show whitelists.Hygiene. Read-only throughout: no branch protection was written (the feature's only new call is a GET), nothing was written outside whatever my scratch
mktemp -dheld,~/.pi/agent/extensionsuntouched, no Plane or GITS write, no code edited, and this is one formal review on this head. I deliberately do not carry the blanket all-clear sentence: B1's gate defect is repaired on this head, while H2524-B2 and H2524-B3 are still open, so a sentence clearing everything would be false. Both are small, local changes — one message and onetry/catchclassification — and neither weakens the enforcement this ticket exists to add.Verdict: APPROVED
model: omniroute/deepseek-flash
head:
fa52db98fbsession: 01a1a665-6f59-7446-a658-251e9e59ccf0
Reviewed round 3 at exact head
fa52db98fb3917cb4e529bc9e9a2582e7d076312(basemain72be4ec), reading the round-3 commit diffc9eaa10..fa52db9and the whole-PR diff againstmain; this was a read-only review — I pushed nothing, retriggered nothing, closed or reopened nothing, and took no CI-triggering action of any kind under the fleet-wide CI pause.CI as I observed it, honestly: Forgejo Actions run #240 at this head reports 12 of 14 job contexts green (ask-user-question, forgejo, forgejo-ci, nats-intercom, pi-friction, pi-otel-telemetry, plane, pr-title, redact, refocus, skill-args, todo) with two red —
mcp-bridge(/usr/bin/env: 'node': Text file busy, runner contention) andpi-swarm-suite(tests/core/task-store-wiring.test.ts,ENOTDIR '/dev/null/nope', the flake that passed on rerun) — and neither package is touched by this diff, which changes onlyforgejo/*anddocs/forgejo.md; run #241 at the same sha is green (python). The green-CI requirement and the landing decision are the orchestrator's, not mine.H2524-B3, direction A (the fix must work). Against an independent stub Forgejo on loopback and this PR's own
verdict-cli.ts(not the repo's test harness), a 403 onGET repos/Homelab/pi-extensions/branch_protectionsgave exit 0, exactly ONE POST, and that POST was the helper's own formal review withcommit_idequal to the head; stdout carriedrequired-ci: branch protection on "main" could not be read (… 403 Forbidden …): the named required CI contexts are UNKNOWN, not emptypluswarning: the landing lock would still refuse after this comment:, and stderr carried no authentication abort.H2524-B3, direction B (the fix must not become an auth bypass). In the same stub, a 403 on
GET /userstill exits 2 with zero POSTs, and a readable but different token identity (aaron) with--author ld-reviewerexits 1 with zero POSTs and the GITS-312 wording — so only the protection read degrades; the helper's PR read, status read, date chain and file read keep their own rethrows, which I confirmed by reading each catch site, and the companion test pins the same shape.H2524-B3 is resolved by
fa52db9: every outcome of the protection read is classifiedunreadableand surfaced asrequired-ci: … UNKNOWN, not emptyplus a lock-preview warning while the verdict is composed and posted exactly once, and the identity refusal path still exits non-zero with no POST.H2524-B2 (the refusal must state only what was measured). The unmeasured paragraphs are gone: the words landing credential, needs writer, writer (not admin),
pi-landing,ld-reviewerand has write on every repo no longer appear, and a test asserts their absence alongside the presence of the measured facts (sessionFORGEJO_API_KEY, the endpoint needs repository admin, the five landing repos reading 200, remedies retry / Forgejo UI).I re-measured those facts myself with read-only GETs and a 0600 curl header file:
GET /user→login: aaron,is_admin: false;branch_protections→ 200 onHomelab/pi-extensions(0 rules),Homelab/ghostty-in-the-shell(2),Homelab/infra-core(2),Homelab/lab-director(1),Homelab/prod(2) and 404 on a nonexistent repo; andHomelab-externaljesus/Transponder→ 403user should be an owner or a collaborator with admin write of a repositorywhile that repo itself readspush: true, admin: false, which is exactly the admin claim the new text makes.H2524-B2 is resolved by
fa52db9: the two assertions round 2 had never measured are removed from the refusal, and what remains is what I independently re-measured at review time.The gate did not soften.
200 []still permits (thepi-extensionsshape), a rule withenable_status_check: falsestill permits, an enforcing rule with every context present still permits, an absent required context still refuses naming it, a present-but-red head still refuses withCI is not green on the head commit …in its existing wording, an unreadable read still refuses at merge time withUNKNOWN, not empty, and an exact-name rule still wins over a glob that also matches.The ordering is what the PR describes: a red head without a missing context keeps its old
CI is not green …refusal and carries the unknown on the result rather than in the message; a red head that ALSO misses a required context gets the completeness refusal naming the missing context; on a green head the unknown and the missing set are what decide.Unknown → outcome, as I probed it directly on
evaluateLandingLock(permits and refusals come from the same two-verdict, dangerous-class fixture):read: unreadable→ refuse,UNKNOWN, not empty200with an object) →listBranchProtectionsthrows → refuse200 null→ refuse204→data: undefined→ non-array → refuse200 [{}](rule naming no branch) → refuse (malformed, not "no rules")base.ref→ refuse,base-ref-absentbase.labelshapedowner:branchwith nobase.ref→baseRefFromPRanswers""→ refuse,base-ref-absentrelease/*vsmain) → permit, "no rule for main"release/*vsrelease/2.0) → refuse, naming the contextli?evslive) → refuse,pattern-unevaluablerefs/heads/main(orMain) against a rule namedmain→ permit, "no rule for refs/heads/main" — see the advisory note belowrequiredContextsomitted ornull→ refuse ("the lock was called without a branch-protection read")read= a value other thanrules/unreadable→ refuserulespresent but not a list → refuseN1 holds:
requiredContextsis required onLandingLockResultand every return path carries it (pending-CI, no-status, non-green, no head sha, undateable head, no verdict, unreadable file list, dangerous-class incomplete panel), so the censusREQUIRED-CIsegment no longer vanishes on a row that refused for another reason — I saw it on a red-head census row.N2 holds: the base ref is
base.ref; a label is accepted only when it already is a branch name, andHomelab:mainnow answers""at both the merge gate and the helper.N3 holds:
200 [{}]and a non-array body both reachunreadableinapi.tsand in the helper, and200 []stays byte-identical as a permit.N4 holds at runtime: a dropped field, a garbled
readstate or a non-listrulesis assessed as UNKNOWN and refuses instead of skipping the check (probed withundefined,null,"unreadable",42and"not a list"); the check can no longer be switched off by a caller forgetting a field.N5 holds and is bounded in the right direction:
request()signalsAbortSignal.timeoutonly when the method isGET/HEAD, and no caller passes an explicit override, so every read is bounded and every write (including the merge POST) keeps its existing unbounded behaviour; in the helper the bound applies to all calls, and a hung POST I simulated produced exactly one attempt and the "the review MAY exist — do not repost" classification rather than a retry, so a timed-out write stays ambiguous as HOMELAB-2526 requires.Nothing else moved: legs 2–5, the session panel, the legacy owner-scoped panel and the hold-withdrawal code are untouched in this commit (the round-3 landing.ts diff is leg 1 plus
requiredContextsplumbing), and the feature adds exactly one GET —branch_protectionsis read in one place and never written (G6).Tests:
npx tsc --noEmitclean andnpx vitest run→ 21 files / 621 tests passed inforgejo/,git statusclean at this head; the red3 log is honest — 19 failed / 45 passed across the two touched files, with the B3 test exiting 2 at the round-2 head, the B2 test catching the old landing credential / needs writer text, and both N5 tests hitting their timeout because the read had no bound.Hygiene: no credential reached any argv — the read-only GETs used a 0600
curl -H @fileheader, and the reviewer credential came from OpenBaokv/data/ld/forgejo-reviewerthrough thesa-operatorpath into a 0600 token file consumed as--token-file, withGET /userasserted to beld-reviewerbefore the write; the Bao token was self-revoked and the scratch directory removed.Advisory, not a landing condition: a
read: "rules"input whoserulesfield is absent is read as an empty list and permits — neither production reader can emit that shape (both always setruleson the success path), so it is a hand-built-caller gap rather than a live one, and treating a missingruleslike a non-listruleswould close it.Advisory: rule selection is name equality plus the
*/**model, so a ref that arrives in another spelling (refs/heads/main, differing case) reads as "no rule" and permits; Forgejo reportsbase.refas a bare branch name, so I did not find this reachable from its API, but canonicalising arefs/heads/prefix would remove the residual.Advisory: the refusal embeds a dated measurement ("Measured (2026-11-04) …"), which is honest today and which I re-verified, but it will drift as credentials and protections change; that is text maintenance, not behaviour.
All five round-3 findings and both blockers are addressed, the fail-closed directions I could reach all refuse, and the one place the PR deliberately degrades — the helper's preview on an unreadable protection read — posts its verdict once and tells the reader the merge gate would still refuse.
blockers resolved
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
fa52db98fbsession: 01a1a665-6f2e-7446-a658-251d3d043049
Independent seat 1 at
fa52db9. I took none of the round-3 write-up on report: every claim below is my own measurement at this head, in a 0700mktemp -d, with the sanctioned reviewer credential read from OpenBaokv/data/ld/forgejo-reviewer(Bao token self-revoked immediately after the KV read, reviewer token staged in a 0600 file, nothing in argv) and the operator credential used only for read-only GETs through acurl -H @fileheader file. The enforcement this ticket exists for was already real at round 2; round 3's job was the two blockers and five nits, and all seven land without softening the gate.B3, direction one — a refused protection read must compose and post. I drove the real
runCliundernode --experimental-strip-typesagainst my own fake Forgejo route table (an inline probe in the scratch dir, nothing written into the tree) with the protection read answering 403user should be an owner or a collaborator with admin write of a repository: the helper printedrequired-ci: branch protection on "main" could not be read (… 403 …): the named required CI contexts are UNKNOWN, not empty, reportedlock.requiredContexts.unknown = protection-unreadable, printed the lock-preview warning, issued exactly one POST (POST …/pulls/91/reviews,event=APPROVED,commit_id= the head, body opening## Verdict: APPROVED), read it back through the reviews list, and exited 0. The same run repeated for a 401 on that read only, a 500, a 200 whose body is not a list, and200 [{}]: each composes, posts once, exits 0, and says UNKNOWN. The post-write verification callscollectStatea second time, so the second protection read is refused too — round 2 would have died there even if the first one had been survived.B3, direction two — a refused identity must still stop. With
GET /useranswering 403 while every other route worked, the helper exits 2, printsauthentication refused, and issues zero non-GET calls; with the whole credential refused (401 everywhere) it does the same. So the fix is scoped to the one read it was meant for:collectState's PR/status reads andloginOfkeep theirAuthRefusedrethrow, and there is no path I could find where "the protection read failed" buys a verdict a refused identity could not.The B3 fix is also live, not only at fixture level. With the reviewer credential on the real PR I get
GET /user→ld-reviewer,permissions {admin:false, push:true, pull:true}onHomelab/pi-extensionsand{admin:false, push:false, pull:true}onHomelab/ghostty-in-the-shell, and 403 with the admin message onbranch_protectionsfor both — exactly the two repos the fleet reviews, so round 2's code would have left the only reviewer identity unable to post any verdict there. This head's helper at--dry-runonHomelab/pi-extensions#91printedrequired-ci: … UNKNOWN, not empty,parse: COUNTED — approve at head fa52db9, the lock warning, and exited 0.B2 — the shipped text now asserts only measured facts, and I re-measured each one.
GET /userwithFORGEJO_API_KEYanswerslogin: "aaron",is_admin: false;branch_protectionsanswers200 []onHomelab/pi-extensionsand200onghostty-in-the-shell,infra-core,lab-director,prod;jesus/Transponderanswers403 user should be an owner or a collaborator with admin write of a repository. That is the refusal's three facts, in the refusal's own words, and the paragraph now names no identity other than the one whose read was made.landing credential,needs writer,writer (not admin),has write on every repoand both identity names are gone from the shipped refusal and fromdocs/forgejo.md, and a test pins their absence.H2524-B2 is resolved by
fa52db9: the unreadable-protection refusal now states only the measured credential, endpoint requirement and 200 responses, and the retracted landing-credential and writer claims are gone from the shipped text and the doc.H2524-B3 is resolved by
fa52db9: a 401/403, 404/5xx, malformed body, branchless rule or timeout on the helper's preview read is classified unreadable and surfaced, while the merge gate still refuses on it and a refused identity still exits 2 with no POST.blockers resolved
The gate did not soften; I drove
evaluateLandingLockdirectly with a real composed verdict so a permit is a permit. Unreadable →ok:false,unknown: protection-unreadable, refusal naming base branch"main"withUNKNOWN, not emptyand both remedies.200 []→ok:truewith the noteno branch-protection rule for "main": no named required CI contexts, the combined status decided leg 1. An enforcing rule with 2/2 present → permit2/2 present on the head; 1/2 → refusal namingemu / bootwith the required/present lists. An exact-namemainrule beats a matching*rule that names a context the head lacks.baseRef: ""and ali?epattern each refuse with their own cause and their own first line. A red head with an unreadable protection read still refuses withCI is not green on the head commit (state: failure, source: combined)— the new refusal is only ever added to a head that would otherwise have passed — and a pending head keepsCI is still running on the head commit (state: pending).N1 — the assessment really is on every result. The field is required on
LandingLockResult, the return type is annotated sotscsees every path, and my probes read it off eight distinct refusal shapes (leg 1 unreadable, absent base ref, unevaluable pattern, absent context, red, pending, no verdict, unreadable file list) plus the permit. The census reads it straight off the result, so theREQUIRED-CIsegment no longer vanishes on the rows that refused earliest.N2/N3/N4 — the three ways the check could have been read as satisfied.
baseRefFromPRanswers""for a label-onlyHomelab:main,"live"for{ref:"live", label:"Homelab:live"}, and"main"for a label that already is a branch name; the helper's preview at{label:"Homelab:main"}reportedbaseRef=""andunknown: base-ref-absent. A protected-rule object naming no branch is unreadable in bothlistBranchProtectionsand the helper (rule #1 names no branch (malformed body)), so200 [{}]can no longer read as "no requirement" while200 []still permits. At runtime,assessRequiredContextsrefuses onundefined,null, a bare string,read:"maybe", andread:"rules"with a non-listrules— I fed all six and each producedprotection-unreadableandok:false, with no crash.N5 — the bound exists and it fails closed.
request()attachesAbortSignal.timeoutonly forGET/HEAD(isRead), so no write is bounded there, which is the safe choice for the merge POST. In the helper the bound is attached to every call, and the ambiguity a bounded write creates is handled where it belongs:submitReview's catch funnels any thrown call intoreviewMayExist("the POST failed in flight … may exist … do not repost"), so a timed-out review POST can never be reported as a plain failure. I confirmed the read bound end-to-end: withFORGEJO_REQUEST_TIMEOUT_MS=80and a protection route that never settles, the helper printedrequired-ci: … could not be read (aborted): … UNKNOWN, not emptyand composed in 165 ms (a probe whose event loop held a live handle, as a real socket does — worth knowing, sinceAbortSignal.timeout's timer does not itself keep the loop alive). The committed tests pin the gate side: a hung protection GET becomes a refusal with zero POSTs, and the helper composes rather than hanging.Nothing else moved. The base→head diff of
forgejo/landing.tshas 18 hunks, all insideevaluateLandingLock(leg 1 and therequiredContextscarry), plus the two interface blocks and the new leg-1 helper block — no hunk lands in legs 2–5, the session panel, the legacy owner panel or hold withdrawal, and a hold at the head still refuses exactly as before. The only new forge call the feature adds anywhere is a GET: noPOST/PUT/PATCH/DELETEtobranch_protectionsexists in the tree, andindex.ts's only round-3 change is passingbaseRefFromPR(pr)instead ofbase.ref ?? base.label.Tests.
npx tsc --noEmitclean andnpx vitest rungreen at 21 files / 621 tests in.worktrees/HOMELAB-2524-ci-contexts/forgejo;git status --porcelainempty before and after my run. I sampledHOMELAB-2524-red3.log:19 failed | 45 passedover the two touched files with the round-2 code, and the failures are the real ones —expected 'Homelab:main' to be '', the oldlanding credential … needs writer (not admin)paragraph still printing, the helper exiting 2 on the 403, and both N5 tests timing out. That is tests-first evidence, not a re-labelled green.CI state, stated honestly. At this head
fa52db98…run #240 is 12 of 14 contexts green:mcp-bridgefailed with/usr/bin/env: 'node': Text file busy(RUN fork/exec …/node/bin/node: text file busyon the shared tool cache) andpi-swarm-suitefailedtests/core/task-store-wiring.test.ts(ENOTDIR: not a directory, mkdir '/dev/null/nope'), a suite this PR does not touch; the orchestrator reports the same test failed once before on this repo and passed on its rerun, which I did not re-measure myself. Neither package is touched by this PR — it changesdocs/forgejo.mdandforgejo/**only — and the eleven other contexts —forgejo,forgejo-ci,redact,plane,pr-titleamong them — are green. A fleet-wide CI pause is in force from 11:23Z, so no rerun is possible now; I pushed nothing, retriggered nothing and closed nothing. My verdict is about the code: this head needs the two red contexts green after a rerun before it lands, and that is the orchestrator's call, not mine.Non-blocking findings, for the next ticket. First, the preview line quotes
AuthRefused's own message, so a 403 arrives asrequired-ci: … (… 403 Forbidden — authentication refused, stopping — this credential may read the pull request but not its branch protection …): the wordsstoppingare now false in that context, and the very next clause explains why it did not stop — worth one dedicated sentence for this path instead of the rethrow's wording. Second, the retracted round-2 rationale still survives as a test comment inforgejo/tests/landing-required-contexts.test.tsaround line 417:Measured with the LANDING credential, the wedge … does not exist: pi-landing has write on every repo it can land in, and GET /branch_protections needs writer, not admin (jesus/Transponder: push true, admin false, reads 200)— the assertions around it are correct, but that sentence is exactly the thing the next ticket will quote as measured fact, which is the hazard B2 named. Third, the PR body's round-2 B1 paragraph still statesneeds writer, not adminandpi-landing has write on every repowithout a superseded marker; the round-3 section retracts it, so a one-line marker would finish the job. Fourth, no test pins the helper's bounded-WRITE branch (submitReview→reviewMayExist); it is correct by inspection and unchanged in behaviour, but a bounded write was previously impossible, so a fixture that hangs the review POST would keep it that way.Hygiene. Read-only throughout: GETs on
branch_protectionsonly, no branch protection written, nothing written outside my scratch dirs, the worktree tree untouched (git statusclean), no comment edited or deleted, no Plane or GITS write, no code edited, no push, no merge, no deploy, and this is one formal review on this head. Both blockers are resolved, and I found nothing in the round-3 change that trades a control for a warning.