HOMELAB-2578: fix(forgejo): resolve landing credential at runtime #82

Merged

pi-landing merged 2 commits from plane/HOMELAB-2578-runtime-landing-credential into main 2026-11-03 16:53:40 +00:00

Conversation 6 Commits 2 Files changed 5 +1233 -79

aaron commented 2026-11-03 05:36:29 +00:00

Owner

Copy link

What

forgejo_merge_pr could only write with a landing credential the process already held at startup (FORGEJO_LANDING_TOKEN_FILE or FORGEJO_LANDING_TOKEN). An orchestrator session that started without either could never acquire one, so the measured failure stood: on Homelab/ghostty-in-the-shell#126 — formal review 1650, all seven required contexts green — the merge authenticated as aaron and the live merge whitelist ["pi-landing"] answered 405.

The merge leg now resolves the sanctioned path at merge time:

FORGEJO_LANDING_TOKEN_FILE -> FORGEJO_LANDING_TOKEN -> OpenBao -> refuse

• resolveLandingCredentialAtRuntime reads kv/pi/forgejo-landing through the sanctioned CLI (bao kv get -format=json <path>, or the helper named by FORGEJO_LANDING_OPENBAO_CLI). argv carries a path, never a token; the child env is an allowlist (PATH/HOME/namespaces + BAO_ADDR/VAULT_ADDR), so the CLI takes its login material from its own $HOME/.vault-token (0600 file). stdout is parsed and never echoed; a failure quotes only one scrubbed stderr line.
• The token is staged by materialiseLandingToken as a 0600 file inside a 0700 directory, the modes are read back from disk, and the merge credential is resolved from that file. The directory is removed in a finally on every exit path (success, identity refusal, merge-write failure), with a process-exit drain as a last resort.
• The staged credential feeds the existing branded proof unchanged: assertMergeIdentity (GET /user) runs immediately before the write, and mergeAsLanding takes the token from that proof. No raw-API merge path was added.
• A read that is configured but fails refuses with zero mutating requests. It never degrades to the operator's FORGEJO_API_KEY (the GITS-359 substitution).
• A wrong or unreadable identity now names the authenticated login and the live merge whitelist (read-only GET .../branch_protections) in the refusal.
• The landing lock is untouched and still evaluated first: a PR that cannot land never causes the sealed secret to be read.

Red first

forgejo/tests/landing-runtime-credential.test.ts, driving the real forgejo_merge_pr handler over the route-table fake plus a fake OpenBao CLI (0700 script in a 0700 temp dir that answers the KV read the way bao kv get -format=json does, with a sentinel proving whether the runtime path ran). No network, no live OpenBao, no real credential.

At 9747346: 9 of 11 failed

× resolves pi-landing from OpenBao at merge time and merges with it
  → Refusing to merge …#79: the merge would be attributed to "aaron", but the landing
    identity is "pi-landing" (credential from FORGEJO_API_KEY (configured-token)).
× proves GET /user = pi-landing with the runtime credential immediately before the merge
× reports the 0600 file and 0700 dir it materialised, and removes the dir after the merge
× never prints the runtime token in the result
× refuses when the sanctioned read fails … → expected … to contain 'kv/pi/forgejo-landing'
× refuses when the OpenBao payload carries no token field
× refuses when the OpenBao CLI is not runnable at all → expected … to match /OpenBao/i
× refuses with the authenticated login and the live merge whitelist, zero writes
× reads the configured field (FORGEJO_LANDING_OPENBAO_FIELD)

Green

At this head, rebased onto origin/main e2b6f6e:

• npx tsc --noEmit — clean
• npx vitest run — 15 files / 406 tests passed (baseline at 9747346: 14 / 372)
• new file: 16 tests — runtime resolution, one merge, GET /user immediately before it, 0600/0700 read back from disk, cleanup on success / identity refusal / merge-write failure, no OpenBao read when the lock refuses, OpenBao failure and missing field and un-runnable CLI all refuse with zero mutating requests, wrong identity refuses naming aaron + ["pi-landing"], ambiguous field name configurable, explicit token file still wins and never invokes the helper, and the token never appears in the result.

No live credential was read by this worker or by any test: the shared tests/fake.ts installs an inert OpenBao helper at import, so no suite can reach kv/pi/forgejo-landing.

Files

forgejo/api.ts, forgejo/index.ts, forgejo/types.ts, forgejo/tests/fake.ts (additive: branch-protections route, mergeHttp, inert OpenBao helper), forgejo/tests/landing-runtime-credential.test.ts (new).

docs/forgejo.md is not touched (collides with #81), forgejo/landing.ts is not touched, verdict-cli.ts/verdict.ts are not touched, ~/.pi/agent/extensions is not touched.

Deployment note (owner action, not code)

The runtime read needs an OpenBao address. This host's pi processes carry FORGEJO_API_KEY/FORGEJO_BASE_URL and no BAO_ADDR/VAULT_ADDR (checked by env key names only), so on this box the read will refuse until the address (a non-secret URL) or a sanctioned helper is available to the session — either BAO_ADDR in the pi host's unit env, or FORGEJO_LANDING_OPENBAO_CLI pointing at the existing sanctioned mint+read helper. The refusal names exactly that. The 405 class the ticket reports is fixed at the tool level; the deployment wiring is a one-line, non-secret change outside this repo's allowed files.

Scope

HOMELAB-2518 / HOMELAB-2578 only. No merge, no verdict, no review, no Done by this worker. GITS untouched (read-only).

## What `forgejo_merge_pr` could only write with a landing credential the process **already held at startup** (`FORGEJO_LANDING_TOKEN_FILE` or `FORGEJO_LANDING_TOKEN`). An orchestrator session that started without either could never acquire one, so the measured failure stood: on `Homelab/ghostty-in-the-shell#126` — formal review 1650, all seven required contexts green — the merge authenticated as `aaron` and the live merge whitelist `["pi-landing"]` answered **405**. The merge leg now resolves the sanctioned path **at merge time**: ``` FORGEJO_LANDING_TOKEN_FILE -> FORGEJO_LANDING_TOKEN -> OpenBao -> refuse ``` - `resolveLandingCredentialAtRuntime` reads `kv/pi/forgejo-landing` through the sanctioned CLI (`bao kv get -format=json <path>`, or the helper named by `FORGEJO_LANDING_OPENBAO_CLI`). argv carries a **path, never a token**; the child env is an allowlist (`PATH`/`HOME`/namespaces + `BAO_ADDR`/`VAULT_ADDR`), so the CLI takes its login material from its own `$HOME/.vault-token` (0600 file). stdout is parsed and never echoed; a failure quotes only one scrubbed stderr line. - The token is staged by `materialiseLandingToken` as a **0600 file inside a 0700 directory**, the modes are **read back from disk**, and the merge credential is resolved **from that file**. The directory is removed in a `finally` on every exit path (success, identity refusal, merge-write failure), with a process-exit drain as a last resort. - The staged credential feeds the **existing branded proof unchanged**: `assertMergeIdentity` (`GET /user`) runs immediately before the write, and `mergeAsLanding` takes the token from that proof. No raw-API merge path was added. - A read that is configured but fails **refuses** with zero mutating requests. It never degrades to the operator's `FORGEJO_API_KEY` (the GITS-359 substitution). - A wrong or unreadable identity now names the **authenticated login and the live merge whitelist** (read-only `GET .../branch_protections`) in the refusal. - The landing lock is untouched and still evaluated first: a PR that cannot land never causes the sealed secret to be read. ## Red first `forgejo/tests/landing-runtime-credential.test.ts`, driving the real `forgejo_merge_pr` handler over the route-table fake plus a **fake OpenBao CLI** (0700 script in a 0700 temp dir that answers the KV read the way `bao kv get -format=json` does, with a sentinel proving whether the runtime path ran). No network, no live OpenBao, no real credential. At `9747346`: **9 of 11 failed** ``` × resolves pi-landing from OpenBao at merge time and merges with it → Refusing to merge …#79: the merge would be attributed to "aaron", but the landing identity is "pi-landing" (credential from FORGEJO_API_KEY (configured-token)). × proves GET /user = pi-landing with the runtime credential immediately before the merge × reports the 0600 file and 0700 dir it materialised, and removes the dir after the merge × never prints the runtime token in the result × refuses when the sanctioned read fails … → expected … to contain 'kv/pi/forgejo-landing' × refuses when the OpenBao payload carries no token field × refuses when the OpenBao CLI is not runnable at all → expected … to match /OpenBao/i × refuses with the authenticated login and the live merge whitelist, zero writes × reads the configured field (FORGEJO_LANDING_OPENBAO_FIELD) ``` ## Green At this head, rebased onto `origin/main` `e2b6f6e`: - `npx tsc --noEmit` — clean - `npx vitest run` — **15 files / 406 tests passed** (baseline at `9747346`: 14 / 372) - new file: **16 tests** — runtime resolution, one merge, `GET /user` immediately before it, 0600/0700 read back from disk, cleanup on success / identity refusal / merge-write failure, no OpenBao read when the lock refuses, OpenBao failure and missing field and un-runnable CLI all refuse with **zero mutating requests**, wrong identity refuses naming `aaron` + `["pi-landing"]`, ambiguous field name configurable, explicit token file still wins and never invokes the helper, and the token never appears in the result. No live credential was read by this worker or by any test: the shared `tests/fake.ts` installs an inert OpenBao helper at import, so no suite can reach `kv/pi/forgejo-landing`. ## Files `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/types.ts`, `forgejo/tests/fake.ts` (additive: branch-protections route, `mergeHttp`, inert OpenBao helper), `forgejo/tests/landing-runtime-credential.test.ts` (new). `docs/forgejo.md` is **not** touched (collides with #81), `forgejo/landing.ts` is not touched, `verdict-cli.ts`/`verdict.ts` are not touched, `~/.pi/agent/extensions` is not touched. ## Deployment note (owner action, not code) The runtime read needs an OpenBao address. This host's pi processes carry `FORGEJO_API_KEY`/`FORGEJO_BASE_URL` and **no** `BAO_ADDR`/`VAULT_ADDR` (checked by env *key* names only), so on this box the read will refuse until the address (a non-secret URL) or a sanctioned helper is available to the session — either `BAO_ADDR` in the pi host's unit env, or `FORGEJO_LANDING_OPENBAO_CLI` pointing at the existing sanctioned mint+read helper. The refusal names exactly that. The 405 class the ticket reports is fixed at the tool level; the deployment wiring is a one-line, non-secret change outside this repo's allowed files. ## Scope HOMELAB-2518 / HOMELAB-2578 only. No merge, no verdict, no review, no Done by this worker. GITS untouched (read-only).

aaron added 1 commit 2026-11-03 05:36:29 +00:00

HOMELAB-2578: fix(forgejo): resolve landing credential at runtime ... 49ac697f95

`forgejo_merge_pr` could only write with a landing credential the process
already held at startup (`FORGEJO_LANDING_TOKEN_FILE` or
`FORGEJO_LANDING_TOKEN`). An orchestrator session that started without either
could never acquire one, so the measured failure stood: on
Homelab/ghostty-in-the-shell#126 — formal review 1650, all seven required
contexts green — the merge authenticated as `aaron` and the live whitelist
`["pi-landing"]` answered 405.

The merge leg now resolves the sanctioned path AT MERGE TIME:

    FORGEJO_LANDING_TOKEN_FILE -> FORGEJO_LANDING_TOKEN -> OpenBao -> refuse

- `resolveLandingCredentialAtRuntime` reads `kv/pi/forgejo-landing` through the
  sanctioned CLI (`bao kv get -format=json <path>`, or the helper named by
  `FORGEJO_LANDING_OPENBAO_CLI`). argv carries a path, never a token; the child
  env is an allowlist (PATH/HOME/namespaces + `BAO_ADDR`/`VAULT_ADDR`), so the
  CLI takes its login material from its own `$HOME/.vault-token`. stdout is
  parsed and never echoed; a failure quotes only a scrubbed stderr line.
- The token is staged by `materialiseLandingToken` as a 0600 file inside a 0700
  directory, the modes are READ BACK from disk, and the merge credential is
  resolved FROM that file. The directory is removed in a `finally` for every
  exit path, with a process-exit drain as a last resort.
- The staged credential feeds the existing branded proof unchanged:
  `assertMergeIdentity` (`GET /user`) runs immediately before the write and
  `mergeAsLanding` takes the token from that proof.
- A read that is configured but fails REFUSES with zero mutating requests; it
  never degrades to the operator's `FORGEJO_API_KEY`.
- A wrong or unreadable identity now names the authenticated login AND the live
  merge whitelist (read-only `GET .../branch_protections`) in the refusal.
- The landing lock is untouched and still evaluated first: a PR that cannot
  land never causes the sealed secret to be read.

Tests are red-first (`landing-runtime-credential.test.ts`: 9 of 11 failed at
9747346, "resolves pi-landing from OpenBao at merge time" refused as `aaron`).
The OpenBao transport is a fake 0700 CLI script under `os.tmpdir()`, and the
shared fake installs an inert helper so no suite can read a live secret or
touch the network. No raw-API merge path, no landing-lock change, no docs change.

aaron added 1 commit 2026-11-03 05:55:28 +00:00

HOMELAB-2578: fix(forgejo): default the landing OpenBao address ... 05e8b72851

Resolving `kv/pi/forgejo-landing` at merge time is not sufficient on a host
whose session carries no address. The measured shape: this host has no
`BAO_ADDR`, no `VAULT_ADDR` and no `~/.bao_address`, so the sanctioned `bao`
CLI has no endpoint, refuses the read, and `forgejo_merge_pr` refuses with zero
writes. The runtime path existed; production still could never land.

The merge leg now supplies the sanctioned non-secret endpoint
`https://openbao.aaron.reynoza.org` to the child env whenever the session names
none, behind the existing overrides — `FORGEJO_LANDING_OPENBAO_ADDR`, then
`BAO_ADDR`, then `VAULT_ADDR`:

- `resolveLandingOpenBaoAddress(env)` is the single precedence rule and never
  returns empty; `runOpenBaoCli` always sets `BAO_ADDR`/`VAULT_ADDR` in the
  allowlisted child env from it.
- An address is a server URL, never a credential: no new credential is
  required, the CLI still authenticates from its own `$HOME/.vault-token`, and
  nothing new enters argv. The 0600/0700 staging, the branded identity proof
  immediately before the write, the refusal-not-degrade rule and the landing
  lock are unchanged.
- Refusals and the reported origin now name the endpoint alongside the path
  (`OpenBao <path> at <endpoint>`), so a wrong endpoint is diagnosable.

Tests are red-first in `landing-runtime-credential.test.ts`: a fake CLI that
refuses when the child env carries no address (this host's shape) fails on
9747346-style code and passes now; overrides are pinned in precedence order.
The fake is a local `/bin/sh` script — no suite reads the live secret or
touches the network.

No docs, no landing-lock, no verdict change, no raw-API merge path.

aaron commented 2026-11-03 06:01:22 +00:00

Author

Owner

Copy link

Non-verdict worker note (HOMELAB-2578 / HOMELAB-2518) — head 05e8b728519c9bdda5b5f4450faf4a8991e0776a

Follow-up on the review finding: resolving kv/pi/forgejo-landing at merge time still refused on this host, because a running session carries no BAO_ADDR/VAULT_ADDR and there is no ~/.bao_address, so the sanctioned bao CLI had no endpoint.

Change (runtime files only):

• forgejo/api.ts — DEFAULT_LANDING_OPENBAO_ADDR = https://openbao.aaron.reynoza.org plus resolveLandingOpenBaoAddress(env) with precedence FORGEJO_LANDING_OPENBAO_ADDR → BAO_ADDR → VAULT_ADDR → default (never empty); runOpenBaoCli now always sets BAO_ADDR/VAULT_ADDR in the allowlisted child env. Refusals and the reported origin name the endpoint (OpenBao <path> at <endpoint>).
• forgejo/index.ts — refusal text and merge-tool guidance name the address env and its default.
• forgejo/tests/landing-runtime-credential.test.ts — 4 new cases.

Red-first: the new fake CLI refuses when the child env carries no BAO_ADDR (this host's shape). Before the fix, 2 of 20 failed: the merge refused (OpenBao kv/pi/forgejo-landing could not be read … no OpenBao address is configured), zero mutating requests, and FORGEJO_LANDING_OPENBAO_ADDR was ignored.

Green: targeted 20/20; full forgejo suite 15 files / 410 tests; tsc --noEmit clean; CI run #196 forgejo job success on this head (~/tmp/h2578-addr-red.log, /tmp/h2578-addr-full.log).

Unchanged: the address is a server URL, not a credential — no new credential required, the CLI still authenticates from its own $HOME/.vault-token, nothing new in argv, no secret printed, 0600/0700 staging and the branded GET /user proof before the write are intact, refusal never degrades to FORGEJO_API_KEY, landing lock untouched. Tests use a local /bin/sh fake only: no live OpenBao read, no network. No docs/verdict/landing-lock edits. Not merged, no verdict posted.

Non-verdict worker note (HOMELAB-2578 / HOMELAB-2518) — head `05e8b728519c9bdda5b5f4450faf4a8991e0776a` Follow-up on the review finding: resolving `kv/pi/forgejo-landing` at merge time still refused on this host, because a running session carries no `BAO_ADDR`/`VAULT_ADDR` and there is no `~/.bao_address`, so the sanctioned `bao` CLI had no endpoint. **Change (runtime files only):** - `forgejo/api.ts` — `DEFAULT_LANDING_OPENBAO_ADDR = https://openbao.aaron.reynoza.org` plus `resolveLandingOpenBaoAddress(env)` with precedence `FORGEJO_LANDING_OPENBAO_ADDR` → `BAO_ADDR` → `VAULT_ADDR` → default (never empty); `runOpenBaoCli` now always sets `BAO_ADDR`/`VAULT_ADDR` in the allowlisted child env. Refusals and the reported origin name the endpoint (`OpenBao <path> at <endpoint>`). - `forgejo/index.ts` — refusal text and merge-tool guidance name the address env and its default. - `forgejo/tests/landing-runtime-credential.test.ts` — 4 new cases. **Red-first:** the new fake CLI refuses when the child env carries no `BAO_ADDR` (this host's shape). Before the fix, 2 of 20 failed: the merge refused (`OpenBao kv/pi/forgejo-landing could not be read … no OpenBao address is configured`), zero mutating requests, and `FORGEJO_LANDING_OPENBAO_ADDR` was ignored. **Green:** targeted 20/20; full `forgejo` suite 15 files / 410 tests; `tsc --noEmit` clean; CI run #196 `forgejo` job success on this head (~/tmp/h2578-addr-red.log, /tmp/h2578-addr-full.log). **Unchanged:** the address is a server URL, not a credential — no new credential required, the CLI still authenticates from its own `$HOME/.vault-token`, nothing new in argv, no secret printed, 0600/0700 staging and the branded `GET /user` proof before the write are intact, refusal never degrades to `FORGEJO_API_KEY`, landing lock untouched. Tests use a local `/bin/sh` fake only: no live OpenBao read, no network. No docs/verdict/landing-lock edits. Not merged, no verdict posted.

aaron commented 2026-11-03 06:02:12 +00:00

Author

Owner

Copy link

Addendum: CI run #196 on 05e8b728519c9bdda5b5f4450faf4a8991e0776a is now pass — all 14 jobs green (incl. forgejo 1m24s and pi-swarm-suite 5m41s). Still non-verdict; not merged.

Addendum: CI run #196 on `05e8b728519c9bdda5b5f4450faf4a8991e0776a` is now **pass** — all 14 jobs green (incl. `forgejo` 1m24s and `pi-swarm-suite` 5m41s). Still non-verdict; not merged.

ld-reviewer approved these changes 2026-11-03 06:40:32 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: 05e8b72851

Independent general review of the exact head 05e8b728519c9bdda5b5f4450faf4a8991e0776a
("default the landing OpenBao address" on top of 49ac697, base e2b6f6e). Re-fetched before
the verdict; the head had not moved. CI run #196 on this sha: 14/14 jobs success. Re-run locally
on the same tree: npx tsc --noEmit clean, npx vitest run → 15 files / 410 tests passed. No
blocker found; the findings below are documentation, design-note, receipt-honesty and coverage
items.

What I verified myself (not taken from the worker log)

Fail-closed is structural, not incidental. resolveLandingCredentialAtRuntime returns at
step 1 unless the answer is configured-token, and step 2 either refuses or returns
source: "openbao". There is no reachable return of configured-token on the merge leg, and
index.ts no longer imports resolveLandingCredential at all — the substitution that produced
GITS-359 is removed rather than merely discouraged.

No secret in argv or in the child environment. execFile(cli, ["kv","get","-format=json", path])
— argv carries a path. The child env is an explicit allowlist (PATH, HOME, BAO_NAMESPACE,
VAULT_NAMESPACE) plus BAO_ADDR/VAULT_ADDR, which are URLs. execFile is used with no
shell: true, so the env-configurable path and CLI are not an injection surface. stdout is parsed
and never echoed; the only stderr text that can escape is one line passed through
scrubSecretShapes (any [A-Za-z0-9_\-+/=]{32,} run collapses to <redacted>, then 300 chars).
No production console.*/JSON.stringify of credential material appears in the diff.

The 0600/0700 contract is read back from disk. materialiseLandingToken does mkdtempSync →
chmodSync(dir, 0o700) → writeFileSync(…, {mode: 0o600}) → chmodSync(file, 0o600) (the
explicit chmod defeats umask) → statSync on both, and on any mismatch removes the directory and
refuses rather than use it. dispose() is flag-guarded and idempotent, with a process.once("exit")
drain as a last resort.

Ordering is correct and least-privilege. assertLandingLock runs before any credential
resolution, so a PR that cannot land never causes the sealed secret to be read — proven by a
sentinel in the new suite, not by inspection alone. The identity proof (GET /user) sits inside
try, immediately before mergeAsLanding, and carries the same branded proof/config, so the write
uses exactly the credential that was just proved. finally { credential.dispose?.() } covers
success, identity refusal and merge-write failure; all three are asserted against the real
filesystem.

The refusal evidence is honest. listBranchProtections is a read-only GET …/branch_protections; non-2xx throws and describeMergeWhitelist converts that into "could not
be read", never into "no whitelist". A disabled whitelist is reported as DISABLED. The
identity refusal names the authenticated login, the requirement, and the live list.

Tests use fakes only. tests/fake.ts installs an inert /bin/sh helper at import (0700 script
in a 0700 mkdtemp dir) through FORGEJO_LANDING_OPENBAO_CLI, so no suite in this package can
reach a live kv/pi/forgejo-landing or the network. The fixture token is a lowercase placeholder,
not credential-shaped. I performed no live OpenBao read and used no landing credential.

Findings (non-blocking)

F1 — the PR body is stale after the default-address commit. The "Deployment note (owner
action, not code)" section still says the runtime read "will refuse until the address … is
available to the session", and the Green section cites 406 tests with 16 in the new file. At this
head a sanctioned default endpoint exists (DEFAULT_LANDING_OPENBAO_ADDR), so that read no longer
refuses for want of an address, and the suite is actually 410 tests / 20 in the new file. The code
and the tool description/promptGuidelines are correct and current; only the prose lags.
Updating the body stops the next reader from planning work that is already done.

F2 — the default endpoint redirects ambient CLI login material (design note). runOpenBaoCli
sets BAO_ADDR/VAULT_ADDR unconditionally and forwards HOME, so the CLI presents its own
$HOME/.vault-token to the resolved address. On a host that holds a vault token for a different
cluster and sets no BAO_ADDR, that token is presented to the hardcoded host. Here the endpoint is
the sanctioned one and this is the commit's explicit intent, and all three override variables are
covered by tests, so this does not block — but it deserves a sentence in the docs when
docs/forgejo.md is next touched (currently excluded because PR #81 owns it). Related: the
allowlist does not forward HTTPS_PROXY/NO_PROXY/BAO_CACERT/BAO_SKIP_VERIFY, so a host
behind a proxy or with a private CA fails closed and must supply a wrapper through
FORGEJO_LANDING_OPENBAO_CLI. Failing closed is the right direction.

F3 — details.runtimeCredential.removed asserts a fact it did not observe. The handler sets
removed: true before the finally runs, and removeRuntimeDir swallows a failed rmSync. The
mode fields are genuinely read back, but removed is a prediction — the only receipt field in this
change not derived from a read-back, in a package whose other receipts (merged sha, comment
identity) are all observed. Reporting an existsSync result after disposal, or dropping the field,
would close the gap. Minor: the control itself is sound and removal is separately asserted by three
tests.

F4 — coverage gap for an unreadable runtime identity. The suite covers a runtime credential
that resolves to a mismatched login (aaron, ld-reviewer) but not one whose GET /user is
unreadable (non-2xx). The code path refuses correctly — assertMergeIdentity returns
ok: false with "the merge identity could not be read …" before any write, and that branch is
exercised in merge-identity.test.ts — so this is coverage, not a defect.

Scope

Diff is confined to forgejo/api.ts, forgejo/index.ts, forgejo/types.ts, forgejo/tests/fake.ts
and the new forgejo/tests/landing-runtime-credential.test.ts. No docs/forgejo.md, no
landing.ts, no verdict*.ts, no package.json change, no new dependency, nothing under
~/.pi/agent/extensions. No raw-API merge path was introduced and the landing lock is untouched.

## Verdict: APPROVED model: omniroute/deepseek-flash head: 05e8b728519c9bdda5b5f4450faf4a8991e0776a Independent general review of the exact head `05e8b728519c9bdda5b5f4450faf4a8991e0776a` ("default the landing OpenBao address" on top of `49ac697`, base `e2b6f6e`). Re-fetched before the verdict; the head had not moved. CI run #196 on this sha: 14/14 jobs `success`. Re-run locally on the same tree: `npx tsc --noEmit` clean, `npx vitest run` → 15 files / 410 tests passed. No blocker found; the findings below are documentation, design-note, receipt-honesty and coverage items. ## What I verified myself (not taken from the worker log) **Fail-closed is structural, not incidental.** `resolveLandingCredentialAtRuntime` returns at step 1 unless the answer is `configured-token`, and step 2 either refuses or returns `source: "openbao"`. There is no reachable return of `configured-token` on the merge leg, and `index.ts` no longer imports `resolveLandingCredential` at all — the substitution that produced GITS-359 is removed rather than merely discouraged. **No secret in argv or in the child environment.** `execFile(cli, ["kv","get","-format=json", path])` — argv carries a path. The child env is an explicit allowlist (`PATH`, `HOME`, `BAO_NAMESPACE`, `VAULT_NAMESPACE`) plus `BAO_ADDR`/`VAULT_ADDR`, which are URLs. `execFile` is used with no `shell: true`, so the env-configurable path and CLI are not an injection surface. stdout is parsed and never echoed; the only stderr text that can escape is one line passed through `scrubSecretShapes` (any `[A-Za-z0-9_\-+/=]{32,}` run collapses to `<redacted>`, then 300 chars). No production `console.*`/`JSON.stringify` of credential material appears in the diff. **The 0600/0700 contract is read back from disk.** `materialiseLandingToken` does `mkdtempSync` → `chmodSync(dir, 0o700)` → `writeFileSync(…, {mode: 0o600})` → `chmodSync(file, 0o600)` (the explicit chmod defeats umask) → `statSync` on both, and on any mismatch removes the directory and refuses rather than use it. `dispose()` is flag-guarded and idempotent, with a `process.once("exit")` drain as a last resort. **Ordering is correct and least-privilege.** `assertLandingLock` runs before any credential resolution, so a PR that cannot land never causes the sealed secret to be read — proven by a sentinel in the new suite, not by inspection alone. The identity proof (`GET /user`) sits inside `try`, immediately before `mergeAsLanding`, and carries the same branded proof/config, so the write uses exactly the credential that was just proved. `finally { credential.dispose?.() }` covers success, identity refusal and merge-write failure; all three are asserted against the real filesystem. **The refusal evidence is honest.** `listBranchProtections` is a read-only `GET …/branch_protections`; non-2xx throws and `describeMergeWhitelist` converts that into "could not be read", never into "no whitelist". A disabled whitelist is reported as DISABLED. The identity refusal names the authenticated login, the requirement, and the live list. **Tests use fakes only.** `tests/fake.ts` installs an inert `/bin/sh` helper at import (0700 script in a 0700 `mkdtemp` dir) through `FORGEJO_LANDING_OPENBAO_CLI`, so no suite in this package can reach a live `kv/pi/forgejo-landing` or the network. The fixture token is a lowercase placeholder, not credential-shaped. I performed no live OpenBao read and used no landing credential. ## Findings (non-blocking) **F1 — the PR body is stale after the default-address commit.** The "Deployment note (owner action, not code)" section still says the runtime read "will refuse until the address … is available to the session", and the Green section cites 406 tests with 16 in the new file. At this head a sanctioned default endpoint exists (`DEFAULT_LANDING_OPENBAO_ADDR`), so that read no longer refuses for want of an address, and the suite is actually 410 tests / 20 in the new file. The code and the tool `description`/`promptGuidelines` are correct and current; only the prose lags. Updating the body stops the next reader from planning work that is already done. **F2 — the default endpoint redirects ambient CLI login material (design note).** `runOpenBaoCli` sets `BAO_ADDR`/`VAULT_ADDR` unconditionally and forwards `HOME`, so the CLI presents its own `$HOME/.vault-token` to the resolved address. On a host that holds a vault token for a different cluster and sets no `BAO_ADDR`, that token is presented to the hardcoded host. Here the endpoint is the sanctioned one and this is the commit's explicit intent, and all three override variables are covered by tests, so this does not block — but it deserves a sentence in the docs when `docs/forgejo.md` is next touched (currently excluded because PR #81 owns it). Related: the allowlist does not forward `HTTPS_PROXY`/`NO_PROXY`/`BAO_CACERT`/`BAO_SKIP_VERIFY`, so a host behind a proxy or with a private CA fails closed and must supply a wrapper through `FORGEJO_LANDING_OPENBAO_CLI`. Failing closed is the right direction. **F3 — `details.runtimeCredential.removed` asserts a fact it did not observe.** The handler sets `removed: true` before the `finally` runs, and `removeRuntimeDir` swallows a failed `rmSync`. The mode fields are genuinely read back, but `removed` is a prediction — the only receipt field in this change not derived from a read-back, in a package whose other receipts (merged sha, comment identity) are all observed. Reporting an `existsSync` result after disposal, or dropping the field, would close the gap. Minor: the control itself is sound and removal is separately asserted by three tests. **F4 — coverage gap for an unreadable runtime identity.** The suite covers a runtime credential that resolves to a mismatched login (`aaron`, `ld-reviewer`) but not one whose `GET /user` is unreadable (non-2xx). The code path refuses correctly — `assertMergeIdentity` returns `ok: false` with "the merge identity could not be read …" before any write, and that branch is exercised in `merge-identity.test.ts` — so this is coverage, not a defect. ## Scope Diff is confined to `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/types.ts`, `forgejo/tests/fake.ts` and the new `forgejo/tests/landing-runtime-credential.test.ts`. No `docs/forgejo.md`, no `landing.ts`, no `verdict*.ts`, no `package.json` change, no new dependency, nothing under `~/.pi/agent/extensions`. No raw-API merge path was introduced and the landing lock is untouched.

ld-reviewer approved these changes 2026-11-03 06:59:05 +00:00

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/k3.1-pro
head: 05e8b72851

tier: premium

Independent premium review of exact head 05e8b728519c9bdda5b5f4450faf4a8991e0776a. Re-fetched immediately before submission and unchanged. CI run #196 on this SHA is green in 14/14 jobs. On the clean exact-head worktree I independently ran npx tsc --noEmit (clean) and npx vitest run (15 files / 410 tests passed, including 20 runtime-credential tests), using only the route-table Forgejo fake and local fake OpenBao CLI.

Verified directly: runtime resolution cannot return the configured operator credential for a merge; OpenBao/identity/staging failures refuse without operator fallback. Address precedence is FORGEJO_LANDING_OPENBAO_ADDR -> BAO_ADDR -> VAULT_ADDR -> the sanctioned default. execFile carries only the KV path in argv, uses no shell, and gives the child an allowlisted environment with no credential value. The runtime token is staged 0600 inside 0700, modes are read back from disk, and the credential is re-read from that file. The landing lock precedes secret resolution; runtime GET /user immediately precedes the branded merge write, which uses the same proved credential. finally disposes on success, identity refusal, and merge failure, with normal-path removal asserted by tests.

No blocker found. Four non-blocking findings:

F1 — stale PR prose. The PR body still says no default OpenBao address exists and reports 406 total / 16 new tests. This head has the default endpoint and verifies at 410 total / 20 new tests. Code and tool guidance are current; only the description lags.

F2 — ambient CLI authority and omitted connection variables. With no override, forwarding HOME while forcing the default address presents ambient $HOME/.vault-token material to that host, even if the token originated for another cluster. This is intended and overrideable in this Homelab deployment, but should be documented. Proxy/CA variables are omitted, so hosts needing them fail closed and require a sanctioned wrapper.

F3 — cleanup receipt is not read back. details.runtimeCredential.removed is set true before finally runs. removeRuntimeDir swallows removal errors and drops the directory from the live set even on failure, so a failed removal would be misreported and not retried by the exit drain. Normal-path cleanup is test-proven; this is a failure-path receipt/retention defect.

F4 — unreadable runtime identity coverage. The new suite covers wrong login but not non-2xx runtime GET /user. The shared identity branch is directly tested with 403 and zero mutations in merge-identity.test.ts, and inspection confirms the runtime path uses it; add the runtime-specific regression case for completeness.

Evidence: /home/aaron/repos/homelab/working-files/homelab-2518/HOMELAB-2578-review-kimi-final.md.

## Verdict: APPROVED model: omniroute/k3.1-pro head: 05e8b728519c9bdda5b5f4450faf4a8991e0776a tier: premium Independent premium review of exact head `05e8b728519c9bdda5b5f4450faf4a8991e0776a`. Re-fetched immediately before submission and unchanged. CI run #196 on this SHA is green in 14/14 jobs. On the clean exact-head worktree I independently ran `npx tsc --noEmit` (clean) and `npx vitest run` (15 files / 410 tests passed, including 20 runtime-credential tests), using only the route-table Forgejo fake and local fake OpenBao CLI. Verified directly: runtime resolution cannot return the configured operator credential for a merge; OpenBao/identity/staging failures refuse without operator fallback. Address precedence is `FORGEJO_LANDING_OPENBAO_ADDR` -> `BAO_ADDR` -> `VAULT_ADDR` -> the sanctioned default. `execFile` carries only the KV path in argv, uses no shell, and gives the child an allowlisted environment with no credential value. The runtime token is staged 0600 inside 0700, modes are read back from disk, and the credential is re-read from that file. The landing lock precedes secret resolution; runtime `GET /user` immediately precedes the branded merge write, which uses the same proved credential. `finally` disposes on success, identity refusal, and merge failure, with normal-path removal asserted by tests. No blocker found. Four non-blocking findings: **F1 — stale PR prose.** The PR body still says no default OpenBao address exists and reports 406 total / 16 new tests. This head has the default endpoint and verifies at 410 total / 20 new tests. Code and tool guidance are current; only the description lags. **F2 — ambient CLI authority and omitted connection variables.** With no override, forwarding `HOME` while forcing the default address presents ambient `$HOME/.vault-token` material to that host, even if the token originated for another cluster. This is intended and overrideable in this Homelab deployment, but should be documented. Proxy/CA variables are omitted, so hosts needing them fail closed and require a sanctioned wrapper. **F3 — cleanup receipt is not read back.** `details.runtimeCredential.removed` is set `true` before `finally` runs. `removeRuntimeDir` swallows removal errors and drops the directory from the live set even on failure, so a failed removal would be misreported and not retried by the exit drain. Normal-path cleanup is test-proven; this is a failure-path receipt/retention defect. **F4 — unreadable runtime identity coverage.** The new suite covers wrong login but not non-2xx runtime `GET /user`. The shared identity branch is directly tested with 403 and zero mutations in `merge-identity.test.ts`, and inspection confirms the runtime path uses it; add the runtime-specific regression case for completeness. Evidence: `/home/aaron/repos/homelab/working-files/homelab-2518/HOMELAB-2578-review-kimi-final.md`.

ld-reviewer approved these changes 2026-11-03 07:24:24 +00:00

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: 05e8b72851

Fresh panel seat on Homelab/pi-extensions PR #82, exact head 05e8b72. I re-fetched the branch head before writing this (it has not moved from 05e8b728519c9bdda5b5f4450faf4a8991e0776a), worked from a clean worktree of that exact commit, and derived everything below from the tree rather than from the PR body, the worker log, or any earlier review on this PR. The earlier reviews are not cited, edited, or reused as this verdict.

What the diff contains: five files, +1233/−79 — forgejo/api.ts, forgejo/index.ts, forgejo/types.ts, forgejo/tests/fake.ts, and a new forgejo/tests/landing-runtime-credential.test.ts. docs/forgejo.md, forgejo/landing.ts, verdict*.ts, package.json, and ~/.pi/agent/extensions are all untouched, and git diff --check is clean.

Independent verification on that exact head: npx tsc --noEmit exits 0 with no diagnostics; npx vitest run reports 15 files / 410 tests passed, with 20 tests in the new file. CI run #196 on 05e8b72851 shows 14/14 jobs success, including forgejo, forgejo-ci, and pr-title.

The property the ticket exists to guarantee holds by construction, not by convention: resolveLandingCredentialAtRuntime returns early unless the answer is configured-token, and after that point every branch either returns source: "openbao" or a refusal, so there is no reachable path that hands the merge write the shared operator credential. resolveLandingCredential is no longer imported by index.ts at all (only the runtime variant is), which means the substitution GITS-359 described was removed rather than left available beside the new path.

No secret reaches argv or a child environment: the only child process in the production diff is a single execFile(cli, ["kv", "get", "-format=json", path], …) whose fourth argv slot is a KV path, and the child env is an explicit allowlist of PATH, HOME, BAO_NAMESPACE, VAULT_NAMESPACE plus BAO_ADDR/VAULT_ADDR, which are server URLs. The secret exists only as a local variable after stdout is parsed. There is no shell: true anywhere in api.ts or index.ts, so the configurable path and CLI are not an injection surface, and a grep of the two production files finds no console.*, process.stdout, or process.stderr write at all.

Failure reasons are scrubbed by construction: scrubSecretShapes collapses any 32-plus-character token-shaped run to <redacted> and truncates to 300 characters, and it is applied both to the quoted CLI stderr line and to the final refusal. parseOpenBaoPayload errors carry the path only.

The staging control is honest about being a control: materialiseLandingToken does mkdtempSync → chmodSync(dir, 0o700) → writeFileSync(…, {mode: 0o600}) → chmodSync(file, 0o600) (the explicit chmod defeats umask) → statSync on both, and on any mode mismatch it removes the directory and refuses instead of using it. The merge credential is then re-resolved through resolveLandingCredential against that staged file, so the token that authorises the write is the one read back off disk.

Ordering is least-privilege: assertLandingLock is awaited before resolveLandingCredentialAtRuntime, so a PR that cannot land never causes the sealed secret to be read, and the new suite proves it with a sentinel file the fake CLI touches when it runs. assertMergeIdentity (GET /user) sits inside the cleanup try, immediately before the branded mergeAsLanding write, using the same credential — the new suite asserts the GET /user index is strictly before the merge POST index and that the merge POST carries the OpenBao token rather than the operator token.

Cleanup is on every exit path: the staged credential is disposed in a finally, so success, wrong identity, and merge-write failure all remove the forgejo-landing-* directory, and each of those three outcomes is asserted against a sandboxed TMPDIR. Disposal is idempotent and a process.once("exit") drain covers a throw past the finally.

The refusal evidence is read-only and honest: listBranchProtections is a GET repos/{owner}/{repo}/branch_protections, and describeMergeWhitelist reports an unreadable list as "could not be read" rather than as an empty allow-list, and a disabled whitelist as DISABLED. The test suite drives a route-table fetch fake and a local 0700 /bin/sh fake CLI, and tests/fake.ts installs an inert helper at import, so no suite in this package can reach the live kv/pi/forgejo-landing or the network; I read no live credential and read no live secret.

I confirm the four findings carried forward are real and non-blocking and I add no blocker of my own: F1 (the PR body still describes the pre-05e8b72 world with no default address and reports 406/16 rather than the actual 410/20), F2 (the default address means an ambient $HOME/.vault-token is presented to the sanctioned host, and proxy/CA variables are not forwarded, so affected hosts fail closed), F3 (details.runtimeCredential.removed: true is a prediction set before the finally, and removeRuntimeDir also drops the directory from the exit-drain set even when rmSync failed, so a failed removal would be misreported and not retried), and F4 (no runtime-specific regression test for a non-2xx GET /user, though that branch is covered in merge-identity.test.ts). All four are behaviours I traced in the code, not claims I took on trust.

No blocker open.

Read-only: no code edit, no merge, no deployment, no Done transition, no ~/.pi/agent/extensions write, no live OpenBao secret read into this session. Reviewer identity was taken from the sanctioned OpenBao reviewer path only (never the operator login, never pi-landing), GET /user on it answered ld-reviewer before anything was posted, and the minted OpenBao token was revoked with the revoke verified by HTTP status, not by parsing a JSON body.

## Verdict: APPROVED model: omniroute/deepseek-flash head: 05e8b728519c9bdda5b5f4450faf4a8991e0776a Fresh panel seat on `Homelab/pi-extensions` PR #82, exact head `05e8b72`. I re-fetched the branch head before writing this (it has not moved from `05e8b728519c9bdda5b5f4450faf4a8991e0776a`), worked from a clean worktree of that exact commit, and derived everything below from the tree rather than from the PR body, the worker log, or any earlier review on this PR. The earlier reviews are not cited, edited, or reused as this verdict. What the diff contains: five files, +1233/−79 — `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/types.ts`, `forgejo/tests/fake.ts`, and a new `forgejo/tests/landing-runtime-credential.test.ts`. `docs/forgejo.md`, `forgejo/landing.ts`, `verdict*.ts`, `package.json`, and `~/.pi/agent/extensions` are all untouched, and `git diff --check` is clean. Independent verification on that exact head: `npx tsc --noEmit` exits 0 with no diagnostics; `npx vitest run` reports 15 files / 410 tests passed, with 20 tests in the new file. CI run #196 on `05e8b72851` shows 14/14 jobs `success`, including `forgejo`, `forgejo-ci`, and `pr-title`. The property the ticket exists to guarantee holds by construction, not by convention: `resolveLandingCredentialAtRuntime` returns early unless the answer is `configured-token`, and after that point every branch either returns `source: "openbao"` or a refusal, so there is no reachable path that hands the merge write the shared operator credential. `resolveLandingCredential` is no longer imported by `index.ts` at all (only the runtime variant is), which means the substitution GITS-359 described was removed rather than left available beside the new path. No secret reaches argv or a child environment: the only child process in the production diff is a single `execFile(cli, ["kv", "get", "-format=json", path], …)` whose fourth argv slot is a KV *path*, and the child env is an explicit allowlist of `PATH`, `HOME`, `BAO_NAMESPACE`, `VAULT_NAMESPACE` plus `BAO_ADDR`/`VAULT_ADDR`, which are server URLs. The secret exists only as a local variable after stdout is parsed. There is no `shell: true` anywhere in `api.ts` or `index.ts`, so the configurable path and CLI are not an injection surface, and a grep of the two production files finds no `console.*`, `process.stdout`, or `process.stderr` write at all. Failure reasons are scrubbed by construction: `scrubSecretShapes` collapses any 32-plus-character token-shaped run to `<redacted>` and truncates to 300 characters, and it is applied both to the quoted CLI stderr line and to the final refusal. `parseOpenBaoPayload` errors carry the path only. The staging control is honest about being a control: `materialiseLandingToken` does `mkdtempSync` → `chmodSync(dir, 0o700)` → `writeFileSync(…, {mode: 0o600})` → `chmodSync(file, 0o600)` (the explicit chmod defeats umask) → `statSync` on *both*, and on any mode mismatch it removes the directory and refuses instead of using it. The merge credential is then re-resolved through `resolveLandingCredential` against that staged file, so the token that authorises the write is the one read back off disk. Ordering is least-privilege: `assertLandingLock` is awaited *before* `resolveLandingCredentialAtRuntime`, so a PR that cannot land never causes the sealed secret to be read, and the new suite proves it with a sentinel file the fake CLI touches when it runs. `assertMergeIdentity` (`GET /user`) sits inside the cleanup `try`, immediately before the branded `mergeAsLanding` write, using the same credential — the new suite asserts the `GET /user` index is strictly before the merge POST index and that the merge POST carries the OpenBao token rather than the operator token. Cleanup is on every exit path: the staged credential is disposed in a `finally`, so success, wrong identity, and merge-write failure all remove the `forgejo-landing-*` directory, and each of those three outcomes is asserted against a sandboxed `TMPDIR`. Disposal is idempotent and a `process.once("exit")` drain covers a throw past the `finally`. The refusal evidence is read-only and honest: `listBranchProtections` is a `GET repos/{owner}/{repo}/branch_protections`, and `describeMergeWhitelist` reports an unreadable list as "could not be read" rather than as an empty allow-list, and a disabled whitelist as DISABLED. The test suite drives a route-table fetch fake and a local 0700 `/bin/sh` fake CLI, and `tests/fake.ts` installs an inert helper at import, so no suite in this package can reach the live `kv/pi/forgejo-landing` or the network; I read no live credential and read no live secret. I confirm the four findings carried forward are real and non-blocking and I add no blocker of my own: F1 (the PR body still describes the pre-`05e8b72` world with no default address and reports 406/16 rather than the actual 410/20), F2 (the default address means an ambient `$HOME/.vault-token` is presented to the sanctioned host, and proxy/CA variables are not forwarded, so affected hosts fail closed), F3 (`details.runtimeCredential.removed: true` is a prediction set before the `finally`, and `removeRuntimeDir` also drops the directory from the exit-drain set even when `rmSync` failed, so a failed removal would be misreported and not retried), and F4 (no runtime-specific regression test for a non-2xx `GET /user`, though that branch is covered in `merge-identity.test.ts`). All four are behaviours I traced in the code, not claims I took on trust. No blocker open. Read-only: no code edit, no merge, no deployment, no Done transition, no `~/.pi/agent/extensions` write, no live OpenBao secret read into this session. Reviewer identity was taken from the sanctioned OpenBao reviewer path only (never the operator login, never `pi-landing`), `GET /user` on it answered `ld-reviewer` before anything was posted, and the minted OpenBao token was revoked with the revoke verified by HTTP status, not by parsing a JSON body.

ld-reviewer approved these changes 2026-11-03 07:25:59 +00:00

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: 05e8b72851

Fresh panel seat on Homelab/pi-extensions PR #82, exact head 05e8b72. I re-fetched the branch head before writing this (it has not moved from 05e8b728519c9bdda5b5f4450faf4a8991e0776a), worked from a clean worktree of that exact commit, and derived everything below from the tree rather than from the PR body, the worker log, or any earlier review on this PR. The earlier reviews are not cited, edited, or reused as this verdict.

What the diff contains: five files, +1233/−79 — forgejo/api.ts, forgejo/index.ts, forgejo/types.ts, forgejo/tests/fake.ts, and a new forgejo/tests/landing-runtime-credential.test.ts. docs/forgejo.md, forgejo/landing.ts, verdict*.ts, package.json, and ~/.pi/agent/extensions are all untouched, and git diff --check is clean.

Independent verification on that exact head: npx tsc --noEmit exits 0 with no diagnostics; npx vitest run reports 15 files / 410 tests passed, with 20 tests in the new file. CI run #196 on 05e8b72851 shows 14/14 jobs success, including forgejo, forgejo-ci, and pr-title.

The property the ticket exists to guarantee holds by construction, not by convention: resolveLandingCredentialAtRuntime returns early unless the answer is configured-token, and after that point every branch either returns source: "openbao" or a refusal, so there is no reachable path that hands the merge write the shared operator credential. resolveLandingCredential is no longer imported by index.ts at all (only the runtime variant is), which means the substitution GITS-359 described was removed rather than left available beside the new path.

No secret reaches argv or a child environment: the only child process in the production diff is a single execFile(cli, ["kv", "get", "-format=json", path], …) whose fourth argv slot is a KV path, and the child env is an explicit allowlist of PATH, HOME, BAO_NAMESPACE, VAULT_NAMESPACE plus BAO_ADDR/VAULT_ADDR, which are server URLs. The secret exists only as a local variable after stdout is parsed. There is no shell: true anywhere in api.ts or index.ts, so the configurable path and CLI are not an injection surface, and a grep of the two production files finds no console.*, process.stdout, or process.stderr write at all.

Failure reasons are scrubbed by construction: scrubSecretShapes collapses any 32-plus-character token-shaped run to <redacted> and truncates to 300 characters, and it is applied both to the quoted CLI stderr line and to the final refusal. parseOpenBaoPayload errors carry the path only.

The staging control is honest about being a control: materialiseLandingToken does mkdtempSync → chmodSync(dir, 0o700) → writeFileSync(…, {mode: 0o600}) → chmodSync(file, 0o600) (the explicit chmod defeats umask) → statSync on both, and on any mode mismatch it removes the directory and refuses instead of using it. The merge credential is then re-resolved through resolveLandingCredential against that staged file, so the token that authorises the write is the one read back off disk.

Ordering is least-privilege: assertLandingLock is awaited before resolveLandingCredentialAtRuntime, so a PR that cannot land never causes the sealed secret to be read, and the new suite proves it with a sentinel file the fake CLI touches when it runs. assertMergeIdentity (GET /user) sits inside the cleanup try, immediately before the branded mergeAsLanding write, using the same credential — the new suite asserts the GET /user index is strictly before the merge POST index and that the merge POST carries the OpenBao token rather than the operator token.

Cleanup is on every exit path: the staged credential is disposed in a finally, so success, wrong identity, and merge-write failure all remove the forgejo-landing-* directory, and each of those three outcomes is asserted against a sandboxed TMPDIR. Disposal is idempotent and a process.once("exit") drain covers a throw past the finally.

The refusal evidence is read-only and honest: listBranchProtections is a GET repos/{owner}/{repo}/branch_protections, and describeMergeWhitelist reports an unreadable list as "could not be read" rather than as an empty allow-list, and a disabled whitelist as DISABLED. The test suite drives a route-table fetch fake and a local 0700 /bin/sh fake CLI, and tests/fake.ts installs an inert helper at import, so no suite in this package can reach the live kv/pi/forgejo-landing or the network; I read no live credential and read no live secret.

I confirm the four findings carried forward are real and non-blocking and I add no blocker of my own: F1 (the PR body still describes the pre-05e8b72 world with no default address and reports 406/16 rather than the actual 410/20), F2 (the default address means an ambient $HOME/.vault-token is presented to the sanctioned host, and proxy/CA variables are not forwarded, so affected hosts fail closed), F3 (details.runtimeCredential.removed: true is a prediction set before the finally, and removeRuntimeDir also drops the directory from the exit-drain set even when rmSync failed, so a failed removal would be misreported and not retried), and F4 (no runtime-specific regression test for a non-2xx GET /user, though that branch is covered in merge-identity.test.ts). All four are behaviours I traced in the code, not claims I took on trust.

No blocker open.

Read-only: no code edit, no merge, no deployment, no Done transition, no ~/.pi/agent/extensions write, no live OpenBao secret read into this session. Reviewer identity was taken from the sanctioned OpenBao reviewer path only (never the operator login, never pi-landing), GET /user on it answered ld-reviewer before anything was posted, and the minted OpenBao token was revoked with the revoke verified by HTTP status, not by parsing a JSON body.

## Verdict: APPROVED model: omniroute/deepseek-flash head: 05e8b728519c9bdda5b5f4450faf4a8991e0776a Fresh panel seat on `Homelab/pi-extensions` PR #82, exact head `05e8b72`. I re-fetched the branch head before writing this (it has not moved from `05e8b728519c9bdda5b5f4450faf4a8991e0776a`), worked from a clean worktree of that exact commit, and derived everything below from the tree rather than from the PR body, the worker log, or any earlier review on this PR. The earlier reviews are not cited, edited, or reused as this verdict. What the diff contains: five files, +1233/−79 — `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/types.ts`, `forgejo/tests/fake.ts`, and a new `forgejo/tests/landing-runtime-credential.test.ts`. `docs/forgejo.md`, `forgejo/landing.ts`, `verdict*.ts`, `package.json`, and `~/.pi/agent/extensions` are all untouched, and `git diff --check` is clean. Independent verification on that exact head: `npx tsc --noEmit` exits 0 with no diagnostics; `npx vitest run` reports 15 files / 410 tests passed, with 20 tests in the new file. CI run #196 on `05e8b72851` shows 14/14 jobs `success`, including `forgejo`, `forgejo-ci`, and `pr-title`. The property the ticket exists to guarantee holds by construction, not by convention: `resolveLandingCredentialAtRuntime` returns early unless the answer is `configured-token`, and after that point every branch either returns `source: "openbao"` or a refusal, so there is no reachable path that hands the merge write the shared operator credential. `resolveLandingCredential` is no longer imported by `index.ts` at all (only the runtime variant is), which means the substitution GITS-359 described was removed rather than left available beside the new path. No secret reaches argv or a child environment: the only child process in the production diff is a single `execFile(cli, ["kv", "get", "-format=json", path], …)` whose fourth argv slot is a KV *path*, and the child env is an explicit allowlist of `PATH`, `HOME`, `BAO_NAMESPACE`, `VAULT_NAMESPACE` plus `BAO_ADDR`/`VAULT_ADDR`, which are server URLs. The secret exists only as a local variable after stdout is parsed. There is no `shell: true` anywhere in `api.ts` or `index.ts`, so the configurable path and CLI are not an injection surface, and a grep of the two production files finds no `console.*`, `process.stdout`, or `process.stderr` write at all. Failure reasons are scrubbed by construction: `scrubSecretShapes` collapses any 32-plus-character token-shaped run to `<redacted>` and truncates to 300 characters, and it is applied both to the quoted CLI stderr line and to the final refusal. `parseOpenBaoPayload` errors carry the path only. The staging control is honest about being a control: `materialiseLandingToken` does `mkdtempSync` → `chmodSync(dir, 0o700)` → `writeFileSync(…, {mode: 0o600})` → `chmodSync(file, 0o600)` (the explicit chmod defeats umask) → `statSync` on *both*, and on any mode mismatch it removes the directory and refuses instead of using it. The merge credential is then re-resolved through `resolveLandingCredential` against that staged file, so the token that authorises the write is the one read back off disk. Ordering is least-privilege: `assertLandingLock` is awaited *before* `resolveLandingCredentialAtRuntime`, so a PR that cannot land never causes the sealed secret to be read, and the new suite proves it with a sentinel file the fake CLI touches when it runs. `assertMergeIdentity` (`GET /user`) sits inside the cleanup `try`, immediately before the branded `mergeAsLanding` write, using the same credential — the new suite asserts the `GET /user` index is strictly before the merge POST index and that the merge POST carries the OpenBao token rather than the operator token. Cleanup is on every exit path: the staged credential is disposed in a `finally`, so success, wrong identity, and merge-write failure all remove the `forgejo-landing-*` directory, and each of those three outcomes is asserted against a sandboxed `TMPDIR`. Disposal is idempotent and a `process.once("exit")` drain covers a throw past the `finally`. The refusal evidence is read-only and honest: `listBranchProtections` is a `GET repos/{owner}/{repo}/branch_protections`, and `describeMergeWhitelist` reports an unreadable list as "could not be read" rather than as an empty allow-list, and a disabled whitelist as DISABLED. The test suite drives a route-table fetch fake and a local 0700 `/bin/sh` fake CLI, and `tests/fake.ts` installs an inert helper at import, so no suite in this package can reach the live `kv/pi/forgejo-landing` or the network; I read no live credential and read no live secret. I confirm the four findings carried forward are real and non-blocking and I add no blocker of my own: F1 (the PR body still describes the pre-`05e8b72` world with no default address and reports 406/16 rather than the actual 410/20), F2 (the default address means an ambient `$HOME/.vault-token` is presented to the sanctioned host, and proxy/CA variables are not forwarded, so affected hosts fail closed), F3 (`details.runtimeCredential.removed: true` is a prediction set before the `finally`, and `removeRuntimeDir` also drops the directory from the exit-drain set even when `rmSync` failed, so a failed removal would be misreported and not retried), and F4 (no runtime-specific regression test for a non-2xx `GET /user`, though that branch is covered in `merge-identity.test.ts`). All four are behaviours I traced in the code, not claims I took on trust. No blocker open. Read-only: no code edit, no merge, no deployment, no Done transition, no `~/.pi/agent/extensions` write, no live OpenBao secret read into this session. Reviewer identity was taken from the sanctioned OpenBao reviewer path only (never the operator login, never `pi-landing`), `GET /user` on it answered `ld-reviewer` before anything was posted, and the minted OpenBao token was revoked with the revoke verified by HTTP status, not by parsing a JSON body.

aaron referenced this pull request 2026-11-03 07:37:13 +00:00

HOMELAB-2586: fix(forgejo): honor wrapped blocker negations #84

aaron referenced this pull request 2026-11-03 08:11:33 +00:00

HOMELAB-2586: fix(forgejo): honor wrapped blocker negations #84

aaron referenced this pull request 2026-11-03 08:48:21 +00:00

HOMELAB-2586: fix(forgejo): honor wrapped blocker negations #84

pi-landing merged commit 93cb2b3ae8 into main 2026-11-03 16:53:40 +00:00

pi-landing referenced this pull request from a commit 2026-11-03 16:53:42 +00:00

Merge pull request 'HOMELAB-2578: fix(forgejo): resolve landing credential at runtime' (#82) from plane/HOMELAB-2578-runtime-landing-credential into main

Sign in to join this conversation.

Reviewers

No reviewers

ld-reviewer

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

Subscribe

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!82

No description provided.