Homelab/pi-extensions
6
0
Fork 0
Code Issues Pull requests 4 Projects Releases Packages Wiki Activity Actions

HOMELAB-1679: fix(forgejo): FORGEJO_LANDING_REVIEWERS cannot widen the trusted set on gated repos (LR-20261111-017, Amendment 15) #130

Closed
pi-landing wants to merge 1 commit from lr-20261111-017-n6split into main
pull from: lr-20261111-017-n6split
merge into: Homelab:main
Conversation 3 Commits 1 Files changed 4 +236 -8
pi-landing commented 2026-11-11 16:16:46 +00:00
Collaborator
Copy link

Spec 017, AMENDMENT 15 — N6 split out ahead of #125. Small and surgical; gate-changing, so two pane seats (via forgejo_comment_as as ld-reviewer) plus homelab-88's file review land before it merges. Delivered as soon as it lands — not held by Amendment 10.

The rule

On the two repos named in code (GO_GATED_REPOS: Homelab/infra-core, Homelab/prod) the landing lock's trusted reviewer set stays the default ["ld-reviewer"]:

  • FORGEJO_LANDING_REVIEWERS is ignored there, and
  • its presence is logged once per call (landing policy: FORGEJO_LANDING_REVIEWERS is set and ignored on GO-gated repo <owner/name> …).

Same class as B3: a value a session can set is not a gate. Non-gated repos keep today's behavior (the env widens the set as before). The repoRef is threaded from every caller (merge gate, census, verdict-cli's two read paths); the policy seam's second parameter is optional, so existing callers and tests compile unchanged.

Proven

  • New forgejo/tests/landing-gated-reviewers-n6.test.ts (4 tests): gated + env set → bogus identity NOT trusted + exactly one log line; non-gated + env set → bogus identity trusted, no log line; gated + env unset → default, silent. The two new-behavior tests fail on the old code (red→green shown by stashing landing.ts).
  • vitest run: 53 files / 1230 pass; the 1 failure is tests/merge-identity.test.ts (ambient landing token), identical on the untouched base — pre-existing. tsc --noEmit clean.
  • no independent walk: implementer-run checks only.

Lab-Run: LR-20261111-017

Spec 017, **AMENDMENT 15** — N6 split out ahead of #125. Small and surgical; gate-changing, so two pane seats (via `forgejo_comment_as` as ld-reviewer) plus homelab-88's file review land before it merges. **Delivered as soon as it lands** — not held by Amendment 10. ## The rule On the two repos named in code (`GO_GATED_REPOS`: `Homelab/infra-core`, `Homelab/prod`) the landing lock's trusted reviewer set stays the default `["ld-reviewer"]`: - `FORGEJO_LANDING_REVIEWERS` is **ignored** there, and - its presence is **logged once per call** (`landing policy: FORGEJO_LANDING_REVIEWERS is set and ignored on GO-gated repo <owner/name> …`). Same class as B3: a value a session can set is not a gate. Non-gated repos keep today's behavior (the env widens the set as before). The repoRef is threaded from every caller (merge gate, census, verdict-cli's two read paths); the policy seam's second parameter is optional, so existing callers and tests compile unchanged. ## Proven - New `forgejo/tests/landing-gated-reviewers-n6.test.ts` (4 tests): gated + env set → bogus identity NOT trusted + exactly one log line; non-gated + env set → bogus identity trusted, no log line; gated + env unset → default, silent. The two new-behavior tests **fail on the old code** (red→green shown by stashing `landing.ts`). - `vitest run`: 53 files / 1230 pass; the 1 failure is `tests/merge-identity.test.ts` (ambient landing token), identical on the untouched base — pre-existing. `tsc --noEmit` clean. - **no independent walk: implementer-run checks only.** Lab-Run: LR-20261111-017
fix(forgejo): FORGEJO_LANDING_REVIEWERS cannot widen the trusted set on gated repos (LR-20261111-017, Amendment 15)
All checks were successful
CI / pr-title (pull_request) Successful in 2s
Details
CI / forgejo-ci (pull_request) Successful in 54s
Details
CI / mcp-bridge (pull_request) Successful in 20s
Details
CI / pi-otel-telemetry (pull_request) Successful in 1m13s
Details
tests / python (pull_request) Successful in 11s
Details
CI / ask-user-question (pull_request) Successful in 39s
Details
CI / nats-intercom (pull_request) Successful in 30s
Details
CI / todo (pull_request) Successful in 59s
Details
CI / skill-args (pull_request) Successful in 1m2s
Details
CI / plane (pull_request) Successful in 1m21s
Details
CI / refocus (pull_request) Successful in 1m0s
Details
CI / pi-friction (pull_request) Successful in 55s
Details
CI / redact (pull_request) Successful in 1m5s
Details
CI / forgejo (pull_request) Successful in 1m56s
Details
CI / pi-swarm-suite (pull_request) Successful in 4m26s
Details
fc83914cfd 
On the two GO-gated repos (Homelab/infra-core, Homelab/prod — named in code;
#125 owns the wider gate) the trusted reviewer set stays the default
[ld-reviewer]: the FORGEJO_LANDING_REVIEWERS env is ignored there, and its
presence is logged once per call. Same class as B3: a value a session can set
is not a gate. Non-gated repos unchanged. Tests both ways, failing on the old
code.

Lab-Run: LR-20261111-017
ld-reviewer commented 2026-11-11 16:20:11 +00:00
Collaborator
Copy link

LD-VERDICT APPROVED head=fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 session=LR-20261111-017-seatA-20261111T1617Z

Head confirmed: the PR's current head is fc83914cfd (base 51daf8842e), so the pin holds. Diff read against origin/main: 4 files, +236/-8 (forgejo/index.ts, forgejo/landing.ts, forgejo/verdict-cli.ts, new forgejo/tests/landing-gated-reviewers-n6.test.ts).

  1. The gated path cannot be widened by the env. resolveLandingPolicy's reviewerIdentities is (gated ? null : reviewersOverride) ?? DEFAULT_LANDING_POLICY.reviewerIdentities (landing.ts:3053-3054): when gated the override is not merely ignored, it is not reachable. Traced every caller into resolveLandingPolicy — merge gate index.ts:440 (evidence.repoRef, built from the same owner/repo as the PR read, index.ts:324), census index.ts:962 (repoRef from parseRepoRef, index.ts:954), verdict-cli collectState verdict-cli.ts:619 (args.repo) and the ledger fallback verdict-cli.ts:948 (collected.state.repoRef). In every path the value used as the gate key is literally the same value used to address the API: the CLI validates --repo to exactly owner/name (verdict-cli.ts:151) after stripping URL/.git/trailing slash, so repoKey cannot resolve to a different key than the repo the call touches. Also confirmed the seam is not bypassable elsewhere: no consumer outside forgejo/ references reviewerIdentities, and verdict.ts falls back to DEFAULT_LANDING_POLICY when no policy is supplied (verdict.ts:596), i.e. fail-closed. repoKey/repo isGoGatedRepo normalise case, URL, .git, trailing slash and query/fragment; lookalikes ("Homelab/prod-fork") are correctly not gated.

  2. The log fires once per gated call and never for non-gated. Single log site, guarded by gated && reviewersOverride (landing.ts:3043-3050); resolution happens once per merge (assertLandingLock called once, index.ts:787), once per census, and once per collectState. Gated+unset and non-gated+set are both silent, as specified.

  3. The tests genuinely fail on the old code. Scratch check in my own clone: git checkout origin/main -- forgejo/landing.ts, run the new file → 2 failed / 2 passed, exactly the two new-behavior tests red ("gated repo + env set: the extra identity is NOT trusted…" and "the gate recognises the repo in the shapes a caller actually holds"); restoring the PR version → 4/4 green. Full suite on the head: 53 files, 1230 pass, the only failure is tests/merge-identity.test.ts ("merges with the dedicated landing credential…"), the ambient-landing-token case that fails identically on the untouched base — pre-existing, not a hold. tsc --noEmit exit 0.

  4. Nothing else changed. The landing.ts hunks stop at the policy seam (GO_GATED_REPOS, repoKey, isGoGatedRepo, policyLine, resolveLandingPolicy); the only other edits are the two index.ts call sites and the two verdict-cli.ts call sites plus the new test file. Non-gated behaviour is byte-for-byte today's.

Holds: none.

Non-blocking notes (neither changes a verdict):

  1. forgejo/verdict-cli.ts:868 and forgejo/verdict-cli.ts:1096 — the review-post path calls collectState twice (post, then live re-read), so on a gated repo with the env set the policy line prints twice in one CLI invocation. "Once per call" holds per resolution, not per invocation; noise, not a widening.
  2. docs/forgejo.md:127, 754, 930 still describe FORGEJO_LANDING_REVIEWERS as an unconditional override; that is now inaccurate for the two gated repos. Follow-up, not this diff's scope.

model: omniroute/deepseek-flash
session: LR-20261111-017-seatA-20261111T1617Z

LD-VERDICT APPROVED head=fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 session=LR-20261111-017-seatA-20261111T1617Z Head confirmed: the PR's current head is fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 (base 51daf8842e7c6cf81b5b9a47602030028a03fd4e), so the pin holds. Diff read against origin/main: 4 files, +236/-8 (forgejo/index.ts, forgejo/landing.ts, forgejo/verdict-cli.ts, new forgejo/tests/landing-gated-reviewers-n6.test.ts). 1. The gated path cannot be widened by the env. resolveLandingPolicy's reviewerIdentities is `(gated ? null : reviewersOverride) ?? DEFAULT_LANDING_POLICY.reviewerIdentities` (landing.ts:3053-3054): when gated the override is not merely ignored, it is not reachable. Traced every caller into resolveLandingPolicy — merge gate index.ts:440 (`evidence.repoRef`, built from the same owner/repo as the PR read, index.ts:324), census index.ts:962 (`repoRef` from parseRepoRef, index.ts:954), verdict-cli collectState verdict-cli.ts:619 (`args.repo`) and the ledger fallback verdict-cli.ts:948 (`collected.state.repoRef`). In every path the value used as the gate key is literally the same value used to address the API: the CLI validates `--repo` to exactly owner/name (verdict-cli.ts:151) after stripping URL/.git/trailing slash, so repoKey cannot resolve to a different key than the repo the call touches. Also confirmed the seam is not bypassable elsewhere: no consumer outside forgejo/ references reviewerIdentities, and verdict.ts falls back to DEFAULT_LANDING_POLICY when no policy is supplied (verdict.ts:596), i.e. fail-closed. repoKey/repo isGoGatedRepo normalise case, URL, .git, trailing slash and query/fragment; lookalikes ("Homelab/prod-fork") are correctly not gated. 2. The log fires once per gated call and never for non-gated. Single log site, guarded by `gated && reviewersOverride` (landing.ts:3043-3050); resolution happens once per merge (assertLandingLock called once, index.ts:787), once per census, and once per collectState. Gated+unset and non-gated+set are both silent, as specified. 3. The tests genuinely fail on the old code. Scratch check in my own clone: `git checkout origin/main -- forgejo/landing.ts`, run the new file → 2 failed / 2 passed, exactly the two new-behavior tests red ("gated repo + env set: the extra identity is NOT trusted…" and "the gate recognises the repo in the shapes a caller actually holds"); restoring the PR version → 4/4 green. Full suite on the head: 53 files, 1230 pass, the only failure is tests/merge-identity.test.ts ("merges with the dedicated landing credential…"), the ambient-landing-token case that fails identically on the untouched base — pre-existing, not a hold. `tsc --noEmit` exit 0. 4. Nothing else changed. The landing.ts hunks stop at the policy seam (GO_GATED_REPOS, repoKey, isGoGatedRepo, policyLine, resolveLandingPolicy); the only other edits are the two index.ts call sites and the two verdict-cli.ts call sites plus the new test file. Non-gated behaviour is byte-for-byte today's. Holds: none. Non-blocking notes (neither changes a verdict): 1. forgejo/verdict-cli.ts:868 and forgejo/verdict-cli.ts:1096 — the review-post path calls collectState twice (post, then live re-read), so on a gated repo with the env set the policy line prints twice in one CLI invocation. "Once per call" holds per resolution, not per invocation; noise, not a widening. 2. docs/forgejo.md:127, 754, 930 still describe FORGEJO_LANDING_REVIEWERS as an unconditional override; that is now inaccurate for the two gated repos. Follow-up, not this diff's scope. model: omniroute/deepseek-flash session: LR-20261111-017-seatA-20261111T1617Z
ld-reviewer commented 2026-11-11 16:20:38 +00:00
Collaborator
Copy link

LD-VERDICT APPROVED head=fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 session=LR-20261111-017-seatB-20261111T1617Z

PR head from the API is exactly fc83914cfd (base 51daf884). Judged the diff and the checks only.

  1. The gated path cannot be widened by the env. Every resolveLandingPolicy call site in the tree is threaded: merge gate forgejo/index.ts:443 with evidence.repoRef, built at forgejo/index.ts:327 as ${owner}/${repo} from parseRepoRef (index.ts:261-273 — canonical, handles owner/repo, .git, and a full URL); census forgejo/index.ts:979 with ${parsed.owner}/${parsed.repo} (:971); verdict-cli's two read paths forgejo/verdict-cli.ts:619 and :948. There is no other read of FORGEJO_LANDING_REVIEWERS anywhere outside landing.ts, and evaluateLandingLock only consumes a policy, so no second, ungated resolution exists. verdict.ts:596 falls back to DEFAULT_LANDING_POLICY (narrow set) when none is injected — fail-closed. Traced end to end.

  2. The log fires once per gated call, never on non-gated. Single site forgejo/landing.ts:3047-3055, guarded by gated && reviewersOverride, and resolveLandingPolicy runs once per merge attempt / census / CLI run (verdict-cli :948 is a fallback only when state.policy is unset, which collectState always sets). The tests count it through an injected sink rather than eyeballing stderr: 1 line for gated+set, [] for non-gated+set and for gated+unset.

  3. The tests are genuinely red on the old code. I re-ran the new test file against untouched origin/main (51daf88) in a scratch worktree with no landing.ts change: 2 failed | 2 passed — exactly the two new-behavior tests (gated+set, and the repo-shape test), with the two unchanged-behavior tests green. On the head they are 4/4. Red→green holds.

  4. Nothing else changed — after accounting for a stale branch. The branch is based on b4b4478, one merge behind main (51daf88, PR #128). So main..head displays a revert of #128 (the forgeMessage/body 409 distinction and the pin lowercasing). It does not actually change that code: I merged fc83914 into origin/main locally — clean, no conflict — and the resulting tree differs from main by exactly 4 files, +236/−8 (landing.ts, index.ts, verdict-cli.ts, and the new test). #128 survives, and its behaviour is untouched by this branch's own diff (b4b4478..fc83914, 4 files, +236/−8).

Holds: none.

Non-blocking notes:

  • NOTE (merge method): land this as a MERGE commit, never squash. A squash would take the head's tree verbatim and silently revert #128's 409-body distinction and pin normalisation. The merge-commit path is clean and preserves both.
  • forgejo/landing.ts:2986-2992 repoKey takes the last two segments, while forgejo/verdict-cli.ts:619 passes the raw args.repo — --repo Homelab/prod/x keys as prod/x (not gated) even though verdict-cli.ts:449 still addresses Homelab/prod. The merge gate and census are unaffected (both pass canonical owner/repo), so no landing can be widened; at worst the CLI's own policy/preview warning disagrees with the gate. Cheap follow-up: normalise in the CLI, or key on the first two segments.
  • docs/forgejo.md:127 and :708 still describe FORGEJO_LANDING_REVIEWERS as an unconditional override, with no mention of the two gated repos. Fine to fold into #125.

Checks I ran on the merged head: vitest run → 53 files, 1234 passed, 1 failed = tests/merge-identity.test.ts:144 (ambient landing token), which fails identically on untouched main (1 failed | 24 passed) — pre-existing, not a hold. tsc --noEmit clean.

model: omniroute/deepseek-flash
session: LR-20261111-017-seatB-20261111T1617Z

LD-VERDICT APPROVED head=fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 session=LR-20261111-017-seatB-20261111T1617Z PR head from the API is exactly fc83914cfd3ac572d3599cd14ac81c0a9b899bb0 (base 51daf884). Judged the diff and the checks only. 1. The gated path cannot be widened by the env. Every `resolveLandingPolicy` call site in the tree is threaded: merge gate `forgejo/index.ts:443` with `evidence.repoRef`, built at `forgejo/index.ts:327` as `${owner}/${repo}` from `parseRepoRef` (`index.ts:261-273` — canonical, handles `owner/repo`, `.git`, and a full URL); census `forgejo/index.ts:979` with `${parsed.owner}/${parsed.repo}` (`:971`); verdict-cli's two read paths `forgejo/verdict-cli.ts:619` and `:948`. There is no other read of `FORGEJO_LANDING_REVIEWERS` anywhere outside `landing.ts`, and `evaluateLandingLock` only *consumes* a policy, so no second, ungated resolution exists. `verdict.ts:596` falls back to `DEFAULT_LANDING_POLICY` (narrow set) when none is injected — fail-closed. Traced end to end. 2. The log fires once per gated call, never on non-gated. Single site `forgejo/landing.ts:3047-3055`, guarded by `gated && reviewersOverride`, and `resolveLandingPolicy` runs once per merge attempt / census / CLI run (verdict-cli `:948` is a fallback only when `state.policy` is unset, which `collectState` always sets). The tests count it through an injected sink rather than eyeballing stderr: 1 line for gated+set, `[]` for non-gated+set and for gated+unset. 3. The tests are genuinely red on the old code. I re-ran the new test file against untouched `origin/main` (51daf88) in a scratch worktree with no `landing.ts` change: **2 failed | 2 passed** — exactly the two new-behavior tests (gated+set, and the repo-shape test), with the two unchanged-behavior tests green. On the head they are 4/4. Red→green holds. 4. Nothing else changed — after accounting for a stale branch. The branch is based on `b4b4478`, one merge behind main (`51daf88`, PR #128). So `main..head` *displays* a revert of #128 (the `forgeMessage`/`body` 409 distinction and the pin lowercasing). It does not actually change that code: I merged `fc83914` into `origin/main` locally — clean, no conflict — and the resulting tree differs from main by exactly 4 files, +236/−8 (`landing.ts`, `index.ts`, `verdict-cli.ts`, and the new test). #128 survives, and its behaviour is untouched by this branch's own diff (`b4b4478..fc83914`, 4 files, +236/−8). Holds: none. Non-blocking notes: - **NOTE (merge method): land this as a MERGE commit, never squash.** A squash would take the head's tree verbatim and silently revert #128's 409-body distinction and pin normalisation. The merge-commit path is clean and preserves both. - `forgejo/landing.ts:2986-2992` `repoKey` takes the *last two* segments, while `forgejo/verdict-cli.ts:619` passes the raw `args.repo` — `--repo Homelab/prod/x` keys as `prod/x` (not gated) even though `verdict-cli.ts:449` still addresses Homelab/prod. The merge gate and census are unaffected (both pass canonical `owner/repo`), so no landing can be widened; at worst the CLI's own policy/preview warning disagrees with the gate. Cheap follow-up: normalise in the CLI, or key on the first two segments. - `docs/forgejo.md:127` and `:708` still describe `FORGEJO_LANDING_REVIEWERS` as an unconditional override, with no mention of the two gated repos. Fine to fold into #125. Checks I ran on the merged head: `vitest run` → 53 files, 1234 passed, 1 failed = `tests/merge-identity.test.ts:144` (ambient landing token), which fails identically on untouched main (1 failed | 24 passed) — pre-existing, not a hold. `tsc --noEmit` clean. model: omniroute/deepseek-flash session: LR-20261111-017-seatB-20261111T1617Z
pi-landing commented 2026-11-11 17:21:35 +00:00
Author
Collaborator
Copy link

Superseded per El Director's AMENDMENT 18: N6 consolidated with the server-name rule and B8 into ONE lock-hardening PR — #131 (base 51daf88, head c4e93bb), commit 1 being this PR's exact change plus the four-knob pin keyed off the server's repo name. This PR's seats and SEATSID OK do not transfer to the new head; #131 carries its own seat round and review. Closing unmerged.

Superseded per El Director's AMENDMENT 18: N6 consolidated with the server-name rule and B8 into ONE lock-hardening PR — #131 (base 51daf88, head c4e93bb), commit 1 being this PR's exact change plus the four-knob pin keyed off the server's repo name. This PR's seats and SEATSID OK do not transfer to the new head; #131 carries its own seat round and review. Closing unmerged.
pi-landing closed this pull request 2026-11-11 17:22:03 +00:00
All checks were successful
CI / pr-title (pull_request) Successful in 2s
Details
CI / forgejo-ci (pull_request) Successful in 54s
Details
CI / mcp-bridge (pull_request) Successful in 20s
Details
CI / pi-otel-telemetry (pull_request) Successful in 1m13s
Details
tests / python (pull_request) Successful in 11s
Details
CI / ask-user-question (pull_request) Successful in 39s
Details
CI / nats-intercom (pull_request) Successful in 30s
Details
CI / todo (pull_request) Successful in 59s
Details
CI / skill-args (pull_request) Successful in 1m2s
Details
CI / plane (pull_request) Successful in 1m21s
Details
CI / refocus (pull_request) Successful in 1m0s
Details
CI / pi-friction (pull_request) Successful in 55s
Details
CI / redact (pull_request) Successful in 1m5s
Details
CI / forgejo (pull_request) Successful in 1m56s
Details
CI / pi-swarm-suite (pull_request) Successful in 4m26s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Homelab/pi-extensions!130
No description provided.