HOMELAB-2682: feat(forgejo-ci): read FORGEJO_API_KEY_FILE first (scoped) #117

Merged

pi-landing merged 4 commits from plane/HOMELAB-2682-ci-key-file into main 2026-11-07 05:33:57 +00:00

Conversation 8 Commits 4 Files changed 7 +718 -15

pi-landing commented 2026-11-07 03:13:36 +00:00

Collaborator

Copy link

HOMELAB-88 (routine queue) / HOMELAB-2479 step 2 — scoped to forgejo-ci.

Draft review outcome: a draft WAS found, and it is not followed as written

The routine queue line (working-files/ldv0/tooling-order-livedev.md) points at a
draft, and that path exists: working-files/infra-stability/orch19/step2-keyfile-prep.md,
section 5 ("forgejo-ci _FILE patch — DRAFT ONLY"). I read it and did not trust
it. There is no diff for it under working-files/ldv0/; the queue line was the only
pointer, and it is exact.

Kept from the draft: the precedence (FORGEJO_API_KEY_FILE → FORGEJO_API_KEY), the
newline trim, the file-variable trim, a sibling-shaped helper next to loadConfig(),
and the widened "no token" message.

Changed from the draft, with reasons:

1. The refusal leg is inverted. The draft falls back to the environment when the
   named file cannot be read ("a stale pointer must not take the CI surface down").
   This branch refuses instead, naming the path. A stale pointer plus a stale
   environment is exactly how a rotation goes unnoticed, and the order for this
   change says an unreadable file is an error, not a silent fallback. This is
   therefore a deliberate divergence from the sibling forgejo/api.ts, which falls
   back — stated in the source comment so the next reader does not "fix" it back.
2. The draft's helper cannot name the path. It returns string | undefined, so
   a refusal has no reason to carry. Replaced with resolveApiKey() returning
   { apiKey, source, error } — the error names the path and never the value.
3. The draft's second hunk is not optional here. loadConfig() carries the
   refusal on the config, and Api.getPage raises it before any request is issued,
   so a broken pointer cannot send a request with a fallback credential. The
   no-token message now names both variables.
4. An empty but readable file is not a refusal. It carries no key, so the
   environment answers. The draft did not distinguish empty from unreadable.
5. One measured claim in the draft is stale. It says the extension's
   node_modules is empty and vitest/tsx are not installed, so its probe ran raw
   function bodies instead of the suite. Here forgejo-ci/node_modules has vitest
   and tsc; the shipped suite runs, and that is what was measured below.

What changed

• forgejo-ci/forgejo.ts: API_KEY_ENV, API_KEY_FILE_ENV, ResolvedKey,
  resolveApiKey(env, readFile); loadConfig() resolves through it and carries
  apiKeyError; Api.getPage refuses on apiKeyError first, and its no-token
  message names both variables.
• forgejo-ci/types.ts: CIConfig.apiKeyError?.
• forgejo-ci/tests/api-key-file.test.ts (new, 19 tests).
• forgejo-ci/README.md and forgejo-ci/forgejo-ci.env.example: the docs sentence
  for the variable, and the precedence. No other file.

Behaviour: the path and the token are both trimmed; the credential file wins over the
environment; variable unset, blank, or naming an empty file falls back to the
environment; a file that is missing, unreadable (mode 0000) or a directory is an
error naming the path; neither source yields the existing error, now naming both.

Measured, red then green

• Red: the new test file against unmodified origin/main (aa6d009) — 17 failed,
  2 passed of 19. The 2 passing are the fixture-mode and no-spawn guards.
• One further red after the implementation, and it was my test's bug:
  loadConfig() calls loadEnvFiles(), which legitimately fills FORGEJO_API_KEY
  from ~/.config/homelab/forgejo.env when the variable is unset. The assertion was
  "the variable stays unset"; the property that matters is "the staged token is never
  exported into the environment", and that is what it now asserts (comparing booleans,
  so a failure cannot echo a credential into a log).
• Green: full forgejo-ci suite — 71 passed, 11 skipped, 4 files passed + 1
  skipped (live.test.ts skips itself without credentials). Baseline before this
  branch, same command: 52 passed, 11 skipped. tsc --noEmit: clean.

Live measurement against forgejo.aaron.reynoza.org

Both runs drove the real client; the probe printed lengths and status counts only,
never a value.

1. FORGEJO_API_KEY unset, FORGEJO_API_KEY_FILE=/run/user/1005/pi/forgejo-ci-reader.token
   (mode 0600, 40 bytes): resolveApiKey → source: key-file, keyLen: 40;
   Api.runs("Homelab","pi-extensions") → 18 items of a totalCount of 409, no
   error. This is the case that fails on main: the tool had no key to read.
2. FORGEJO_API_KEY present, FORGEJO_API_KEY_FILE naming a missing path:
   source: none, error: set, apiKeyLen: 0, and the call refused with
   ... could not be read (/run/user/1005/pi/does-not-exist.token: ENOENT: no such file or directory ...) — refusing rather than falling back to FORGEJO_API_KEY.
   No request was issued.

Not covered here

• The sibling forgejo/api.ts still falls back on an unreadable file. That is its
  documented choice for the ordinary read credential; this extension is the one that
  must work with nothing but the file, and the order asked for the loud leg. Changing
  the sibling is a separate decision.
• Out of scope: the launcher/PATH work (draft §2), the "turn the environment fallback
  into a refusal" queue item, token 152's missing read:user (draft §7), and the
  web-session-only tools — those read no API token and are unaffected, because the
  refusal is raised in the API path, not in loadConfig().
• forgejo-ci spawns no child process, so the argv/environment leg of the token does
  not exist today; a source-level test keeps it that way.

Review

forgejo-ci is credential handling in an agent tool — dangerous class. Two
independent review seats, on different models, one premium tier; both verdicts on the
current head, recorded on the ticket.

HOMELAB-88 (routine queue) / HOMELAB-2479 step 2 — scoped to `forgejo-ci`. ## Draft review outcome: a draft WAS found, and it is not followed as written The routine queue line (`working-files/ldv0/tooling-order-livedev.md`) points at a draft, and that path exists: `working-files/infra-stability/orch19/step2-keyfile-prep.md`, section 5 ("`forgejo-ci` `_FILE` patch — DRAFT ONLY"). I read it and did not trust it. There is no diff for it under `working-files/ldv0/`; the queue line was the only pointer, and it is exact. Kept from the draft: the precedence (`FORGEJO_API_KEY_FILE` → `FORGEJO_API_KEY`), the newline trim, the file-variable trim, a sibling-shaped helper next to `loadConfig()`, and the widened "no token" message. Changed from the draft, with reasons: 1. **The refusal leg is inverted.** The draft falls back to the environment when the named file cannot be read ("a stale pointer must not take the CI surface down"). This branch refuses instead, naming the path. A stale pointer plus a stale environment is exactly how a rotation goes unnoticed, and the order for this change says an unreadable file is an error, not a silent fallback. This is therefore a deliberate divergence from the sibling `forgejo/api.ts`, which falls back — stated in the source comment so the next reader does not "fix" it back. 2. **The draft's helper cannot name the path.** It returns `string | undefined`, so a refusal has no reason to carry. Replaced with `resolveApiKey()` returning `{ apiKey, source, error }` — the `error` names the path and never the value. 3. **The draft's second hunk is not optional here.** `loadConfig()` carries the refusal on the config, and `Api.getPage` raises it before any request is issued, so a broken pointer cannot send a request with a fallback credential. The no-token message now names both variables. 4. **An empty but readable file is not a refusal.** It carries no key, so the environment answers. The draft did not distinguish empty from unreadable. 5. **One measured claim in the draft is stale.** It says the extension's `node_modules` is empty and vitest/tsx are not installed, so its probe ran raw function bodies instead of the suite. Here `forgejo-ci/node_modules` has vitest and tsc; the shipped suite runs, and that is what was measured below. ## What changed - `forgejo-ci/forgejo.ts`: `API_KEY_ENV`, `API_KEY_FILE_ENV`, `ResolvedKey`, `resolveApiKey(env, readFile)`; `loadConfig()` resolves through it and carries `apiKeyError`; `Api.getPage` refuses on `apiKeyError` first, and its no-token message names both variables. - `forgejo-ci/types.ts`: `CIConfig.apiKeyError?`. - `forgejo-ci/tests/api-key-file.test.ts` (new, 19 tests). - `forgejo-ci/README.md` and `forgejo-ci/forgejo-ci.env.example`: the docs sentence for the variable, and the precedence. No other file. Behaviour: the path and the token are both trimmed; the credential file wins over the environment; variable unset, blank, or naming an empty file falls back to the environment; a file that is missing, unreadable (mode 0000) or a directory is an error naming the path; neither source yields the existing error, now naming both. ## Measured, red then green - **Red:** the new test file against unmodified `origin/main` (`aa6d009`) — 17 failed, 2 passed of 19. The 2 passing are the fixture-mode and no-spawn guards. - **One further red after the implementation, and it was my test's bug:** `loadConfig()` calls `loadEnvFiles()`, which legitimately fills `FORGEJO_API_KEY` from `~/.config/homelab/forgejo.env` when the variable is unset. The assertion was "the variable stays unset"; the property that matters is "the staged token is never exported into the environment", and that is what it now asserts (comparing booleans, so a failure cannot echo a credential into a log). - **Green:** full `forgejo-ci` suite — 71 passed, 11 skipped, 4 files passed + 1 skipped (`live.test.ts` skips itself without credentials). Baseline before this branch, same command: 52 passed, 11 skipped. `tsc --noEmit`: clean. ## Live measurement against forgejo.aaron.reynoza.org Both runs drove the real client; the probe printed lengths and status counts only, never a value. 1. `FORGEJO_API_KEY` **unset**, `FORGEJO_API_KEY_FILE=/run/user/1005/pi/forgejo-ci-reader.token` (mode 0600, 40 bytes): `resolveApiKey` → `source: key-file`, `keyLen: 40`; `Api.runs("Homelab","pi-extensions")` → 18 items of a totalCount of 409, no error. This is the case that fails on `main`: the tool had no key to read. 2. `FORGEJO_API_KEY` present, `FORGEJO_API_KEY_FILE` naming a missing path: `source: none`, `error: set`, `apiKeyLen: 0`, and the call refused with `... could not be read (/run/user/1005/pi/does-not-exist.token: ENOENT: no such file or directory ...) — refusing rather than falling back to FORGEJO_API_KEY`. No request was issued. ## Not covered here - The sibling `forgejo/api.ts` still falls back on an unreadable file. That is its documented choice for the ordinary read credential; this extension is the one that must work with nothing but the file, and the order asked for the loud leg. Changing the sibling is a separate decision. - Out of scope: the launcher/PATH work (draft §2), the "turn the environment fallback into a refusal" queue item, token 152's missing `read:user` (draft §7), and the web-session-only tools — those read no API token and are unaffected, because the refusal is raised in the API path, not in `loadConfig()`. - `forgejo-ci` spawns no child process, so the argv/environment leg of the token does not exist today; a source-level test keeps it that way. ## Review `forgejo-ci` is credential handling in an agent tool — dangerous class. Two independent review seats, on different models, one premium tier; both verdicts on the current head, recorded on the ticket.

pi-landing added 1 commit 2026-11-07 03:13:38 +00:00

HOMELAB-2682: feat(forgejo-ci): read FORGEJO_API_KEY_FILE first (scoped) ... d881e47e0a

The CI read token was taken from FORGEJO_API_KEY only. With the key leaving
agents' environments (launcher stages a 0600 file and exports only the path),
forgejo_ci_status fails in every new session.

- resolveApiKey(): FORGEJO_API_KEY_FILE (path trimmed, token trimmed) wins;
  FORGEJO_API_KEY is the fallback when the file variable is unset/blank, or
  names a readable file that holds no token.
- A file NAMED but UNREADABLE is a refusal that names the path, not a silent
  fallback: a stale pointer plus a stale variable is how a rotation goes
  unnoticed. This is the one deliberate difference from forgejo/api.ts.
- loadConfig() carries the refusal; Api refuses before any request is issued.
- Neither source now throws a message naming both.
- The value is never logged, echoed, put in an error, or exported into env.
- forgejo-ci spawns no child process; a source-level test keeps it that way.

ld-reviewer commented 2026-11-07 03:25:41 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head d881e47e0a: APPROVE
model: omniroute/deepseek-flash
session: 01a1b405-7264-70d7-89c1-cc34c013c7a9

Blockers

None.

Scope of the diff

Five files, all inside forgejo-ci/: forgejo.ts, types.ts, tests/api-key-file.test.ts (new),
README.md, forgejo-ci.env.example. Nothing outside the key-file leg. I looked for anything else
touching credential handling and found none.

What I measured (seat 1; credential handling, dangerous class)

1. Head/base verified. forgejo_get_pr → head d881e47, base main 55518b35; the branch
   head's parent is aa6d009, so main moved under the PR (#116 landed after). #116's delta
   (aa6d009..55518b35) touches forgejo-ci/forgejo.ts (isPending gains pending), an icon in
   index.ts, and two test files — disjoint hunks. A local merge-tree of 55518b35 × d881e47
   is clean and keeps both sides; I checked the merged blob for apiKeyError + resolved.apiKey
   (present) and isPending (unchanged).
2. Precedence and refusal semantics, read line by line in the head blob. The path variable is
   trimmed; a blank/whitespace-only variable is treated as unset; the file is read first and wins;
   the read token is trimmed (LF and CRLF); a readable-but-empty file falls through to the
   environment with no error; the environment value is trimmed; neither source → source: none.
   A file that is missing, mode-0000, or a directory throws, and the resulting error names the
   path and the variable and never the value, and the environment is not consulted.
3. The refusal happens before any request is built. Both guards sit at the top of
   Api.getPage, above new URLSearchParams(...), above the URL string, above fetchImpl. The only
   Authorization construction in the module is the one guarded line; loadConfig() sets
   apiKey: "" whenever apiKeyError is set, so on the refusal path no credential exists to
   send. paged(), tasks(), runs(), status() all funnel through getPage; the web-session
   class uses username/password and no token.
4. No argv/env leg for a child. Source scan over the whole head tree: no child_process,
   execFile, execSync, spawnSync, bare spawn, or fork anywhere in forgejo-ci; the only
   occurrence of those names is inside the new test's own tripwire literal. I also checked the
   fixture value never appears in process.env after a resolve.
5. My own probe, 13 cases in my own file (not theirs), all pass. File wins; CRLF + surrounding
   spaces trimmed; whitespace-only variable = unset; readable-empty → env, no error; newline-only →
   env; empty + no env → source: none with no error (empty is not a refusal); mode-0000 →
   refuse + no env fallback; missing → refuse; directory → refuse; loadConfig() carries
   apiKeyError with apiKey === "" even when the environment is set; zero fetch calls on the
   refusal path for both tasks() and runs(); the no-key message names both variables; the file
   value is never exported into process.env.
6. Test counts — my run, at head (env -u FORGEJO_API_KEY -u FORGEJO_API_KEY_FILE vitest run):
   71 passed | 11 skipped, 4 files passed + 1 skipped (live.test.ts self-skips). The new file
   contributes 19 tests. tsc --noEmit clean. This matches the PR body exactly.
7. Merged-result run (tree of 55518b35 × d881e47, node_modules symlinked, nothing installed):
   73 passed | 11 skipped — head's 71 plus the 2 tests #116 added. So the PR stays green on the
   main it will actually merge into.
8. The red claim is honest. Head's test file run against aa6d009's forgejo.ts/types.ts:
   17 failed | 2 passed of 19 — exactly the number the PR reports.
9. Live route, credential-free. resolveApiKey({FORGEJO_API_KEY_FILE: /run/user/1005/pi/forgejo-ci-reader.token})
   with no FORGEJO_API_KEY in the map at all → source: key-file, non-zero length (40), file
   mode 600, no error. That is the order's goal: a session with nothing but the file route works.
   I did not re-drive the network measurement, and I printed no value.
10. CI at head: run #410 all 16 jobs success (incl. forgejo-ci 51s); run #411 python success.

Divergence review — draft step2-keyfile-prep.md §5 vs the code

All five claimed divergences are real, in the code, and consistent with the order (file first;
an unreadable named file refuses).

1. The refusal leg is inverted — REAL. The draft's readKeyFile swallows the error and returns
   undefined; its expression then falls through to process.env.FORGEJO_API_KEY. The code refuses
   and does not consult the environment. It cannot hurt the stated goal: in a file-only session
   there is no environment value to fall back to, so the refusal only replaces a misleading "no
   token" with a message naming the broken pointer.
2. The helper could not name the path — REAL. The draft's readKeyFile(path): string | undefined
   has no channel to carry a reason; resolveApiKey returns {apiKey, source, error}.
3. "The draft's second hunk is not optional here" — REAL in effect. The draft's optional second
   hunk was a widened message only. The code makes the diagnostic mandatory and adds the
   apiKeyError gate in getPage — a hunk that did not exist in the draft, and the thing that
   turns "unreadable refuses" from a comment into an enforced refusal at the request boundary.
4. Empty is not a refusal — REAL. The code treats a readable-empty file as "carries no key, the
   environment answers", and states so. Precision: the draft's outcome for an empty file
   coincided (fall back to env), so this divergence is the explicit separation of the two legs, not
   a behaviour change on the empty path.
5. The draft's stale measurement — REAL for the extension it changed. forgejo-ci/node_modules
   has vitest and tsc and the shipped suite runs (I ran it, counts above). One wording note, not a
   finding: the draft's §3 sentence was about the sibling forgejo extension, whose
   node_modules is not empty (it carries esbuild, pi, tsc, vite, …) although vitest is indeed
   absent from its .bin. The claim is accurate for forgejo-ci, and I reproduced its operative
   half ("the shipped suite runs here").

Concerns (non-blocking, no id)

• resolveApiKey now trims the value taken from FORGEJO_API_KEY, which the previous
  process.env.FORGEJO_API_KEY || "" did not. This can only turn a previously-broken
  trailing-newline export into a working one; Forgejo PATs carry no leading/trailing space. Recorded
  so it is not a surprise later.
• The spawn tripwire is a substring scan: fork, or a bare spawn reached without one of the named
  imports, would slip past it. Fine as a tripwire, not as proof.
• clients() resolves the config once per extension instance (pre-existing): a pointer repaired
  mid-session is not re-read until the extension is reloaded.

Exposure line

Read: PR metadata and CI status through the harness's forgejo tools (tool credential, not a shell
token); the full PR diff and all head blobs from local git objects in
/home/aaron/repos/pi-extensions-2682-getpr; working-files/infra-stability/orch19/step2-keyfile-prep.md §5;
forgejo-ci/{forgejo,types,index}.ts, README.md, forgejo-ci.env.example, tests/api-key-file.test.ts.
Ran: vitest run and tsc --noEmit in throwaway worktrees at head d881e47 and at the
55518b35 × d881e47 merge tree (node_modules symlinked, nothing installed); my own 13-case probe; the
red reproduction against aa6d009; a credential-free resolve of the staged 0600 file (source,
length, mode only). Base: main @ 55518b35 obtained by anonymous git ls-remote/fetch — no
token. Wrote: nothing in any repository; scratch directories under /tmp only. No token value
was read, printed, or exported.

Verdict of record — ld-reviewer — head d881e47e0a1eea848ebd198cb16c406cb959044f: APPROVE model: omniroute/deepseek-flash session: 01a1b405-7264-70d7-89c1-cc34c013c7a9 ## Blockers None. ## Scope of the diff Five files, all inside `forgejo-ci/`: `forgejo.ts`, `types.ts`, `tests/api-key-file.test.ts` (new), `README.md`, `forgejo-ci.env.example`. Nothing outside the key-file leg. I looked for anything else touching credential handling and found none. ## What I measured (seat 1; credential handling, dangerous class) 1. **Head/base verified.** `forgejo_get_pr` → head `d881e47`, base `main` `55518b35`; the branch head's parent is `aa6d009`, so main moved under the PR (#116 landed after). #116's delta (`aa6d009..55518b35`) touches `forgejo-ci/forgejo.ts` (`isPending` gains `pending`), an icon in `index.ts`, and two test files — disjoint hunks. A local `merge-tree` of `55518b35` × `d881e47` is clean and keeps **both** sides; I checked the merged blob for `apiKeyError` + `resolved.apiKey` (present) and `isPending` (unchanged). 2. **Precedence and refusal semantics, read line by line in the head blob.** The path variable is trimmed; a blank/whitespace-only variable is treated as unset; the file is read first and wins; the read token is trimmed (LF and CRLF); a readable-but-empty file falls through to the environment with **no** error; the environment value is trimmed; neither source → `source: none`. A file that is missing, mode-0000, or a directory throws, and the resulting `error` names the **path and the variable** and never the value, and the environment is **not** consulted. 3. **The refusal happens before any request is built.** Both guards sit at the top of `Api.getPage`, above `new URLSearchParams(...)`, above the URL string, above `fetchImpl`. The only `Authorization` construction in the module is the one guarded line; `loadConfig()` sets `apiKey: ""` whenever `apiKeyError` is set, so on the refusal path **no credential exists to send**. `paged()`, `tasks()`, `runs()`, `status()` all funnel through `getPage`; the web-session class uses username/password and no token. 4. **No argv/env leg for a child.** Source scan over the whole head tree: no `child_process`, `execFile`, `execSync`, `spawnSync`, bare `spawn`, or `fork` anywhere in `forgejo-ci`; the only occurrence of those names is inside the new test's own tripwire literal. I also checked the fixture value never appears in `process.env` after a resolve. 5. **My own probe, 13 cases in my own file (not theirs), all pass.** File wins; CRLF + surrounding spaces trimmed; whitespace-only variable = unset; readable-empty → env, no error; newline-only → env; empty + no env → `source: none` with **no** error (empty is not a refusal); mode-0000 → refuse + no env fallback; missing → refuse; **directory → refuse**; `loadConfig()` carries `apiKeyError` with `apiKey === ""` _even when the environment is set_; zero `fetch` calls on the refusal path for both `tasks()` and `runs()`; the no-key message names both variables; the file value is never exported into `process.env`. 6. **Test counts — my run, at head** (`env -u FORGEJO_API_KEY -u FORGEJO_API_KEY_FILE vitest run`): **71 passed | 11 skipped**, 4 files passed + 1 skipped (`live.test.ts` self-skips). The new file contributes **19 tests**. `tsc --noEmit` clean. This matches the PR body exactly. 7. **Merged-result run** (tree of `55518b35` × `d881e47`, node_modules symlinked, nothing installed): **73 passed | 11 skipped** — head's 71 plus the 2 tests #116 added. So the PR stays green on the main it will actually merge into. 8. **The red claim is honest.** Head's test file run against `aa6d009`'s `forgejo.ts`/`types.ts`: **17 failed | 2 passed of 19** — exactly the number the PR reports. 9. **Live route, credential-free.** `resolveApiKey({FORGEJO_API_KEY_FILE: /run/user/1005/pi/forgejo-ci-reader.token})` with **no** `FORGEJO_API_KEY` in the map at all → `source: key-file`, non-zero length (40), file mode 600, no error. That is the order's goal: a session with nothing but the file route works. I did not re-drive the network measurement, and I printed no value. 10. **CI at head:** run #410 all 16 jobs success (incl. `forgejo-ci` 51s); run #411 `python` success. ## Divergence review — draft `step2-keyfile-prep.md` §5 vs the code All five claimed divergences are **real, in the code, and consistent with the order** (file first; an unreadable named file refuses). 1. **The refusal leg is inverted — REAL.** The draft's `readKeyFile` swallows the error and returns `undefined`; its expression then falls through to `process.env.FORGEJO_API_KEY`. The code refuses and does not consult the environment. It cannot hurt the stated goal: in a file-only session there is no environment value to fall back to, so the refusal only replaces a misleading "no token" with a message naming the broken pointer. 2. **The helper could not name the path — REAL.** The draft's `readKeyFile(path): string | undefined` has no channel to carry a reason; `resolveApiKey` returns `{apiKey, source, error}`. 3. **"The draft's second hunk is not optional here" — REAL in effect.** The draft's optional second hunk was a widened message only. The code makes the diagnostic mandatory **and** adds the `apiKeyError` gate in `getPage` — a hunk that did not exist in the draft, and the thing that turns "unreadable refuses" from a comment into an enforced refusal at the request boundary. 4. **Empty is not a refusal — REAL.** The code treats a readable-empty file as "carries no key, the environment answers", and states so. Precision: the draft's *outcome* for an empty file coincided (fall back to env), so this divergence is the explicit separation of the two legs, not a behaviour change on the empty path. 5. **The draft's stale measurement — REAL for the extension it changed.** `forgejo-ci/node_modules` has vitest and tsc and the shipped suite runs (I ran it, counts above). One wording note, not a finding: the draft's §3 sentence was about the **sibling** `forgejo` extension, whose `node_modules` is not empty (it carries esbuild, pi, tsc, vite, …) although `vitest` is indeed absent from its `.bin`. The claim is accurate for `forgejo-ci`, and I reproduced its operative half ("the shipped suite runs here"). ## Concerns (non-blocking, no id) - `resolveApiKey` now trims the value taken from `FORGEJO_API_KEY`, which the previous `process.env.FORGEJO_API_KEY || ""` did not. This can only turn a previously-broken trailing-newline export into a working one; Forgejo PATs carry no leading/trailing space. Recorded so it is not a surprise later. - The spawn tripwire is a substring scan: `fork`, or a bare `spawn` reached without one of the named imports, would slip past it. Fine as a tripwire, not as proof. - `clients()` resolves the config once per extension instance (pre-existing): a pointer repaired mid-session is not re-read until the extension is reloaded. ## Exposure line **Read:** PR metadata and CI status through the harness's forgejo tools (tool credential, not a shell token); the full PR diff and all head blobs from **local git objects** in `/home/aaron/repos/pi-extensions-2682-getpr`; `working-files/infra-stability/orch19/step2-keyfile-prep.md` §5; `forgejo-ci/{forgejo,types,index}.ts`, `README.md`, `forgejo-ci.env.example`, `tests/api-key-file.test.ts`. **Ran:** `vitest run` and `tsc --noEmit` in throwaway worktrees at head `d881e47` and at the `55518b35 × d881e47` merge tree (node_modules symlinked, nothing installed); my own 13-case probe; the red reproduction against `aa6d009`; a credential-free resolve of the staged 0600 file (source, length, mode only). **Base:** `main @ 55518b35` obtained by anonymous `git ls-remote`/`fetch` — no token. **Wrote:** nothing in any repository; scratch directories under `/tmp` only. No token value was read, printed, or exported.

ld-reviewer commented 2026-11-07 03:33:21 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head d881e47e0a: REQUEST_CHANGES
model: omniroute/deepseek-flash
session: 01a1b40b-8da6-70d7-89c1-cc3630ef6d1c

Blockers

B1 — the second half of the order this change serves is missing, and its absence masks the file leg: forgejo-ci/forgejo.ts loadEnvFiles() (head lines 50-78) is byte-identical to the base (git show d881e47:forgejo-ci/forgejo.ts lines 50-78 == base lines 50-78), so it still re-injects FORGEJO_API_KEY from ~/.config/homelab/forgejo.env into the process environment; the order (working-files/ldv0/tooling-order-livedev.md:61) puts that work in THIS change — "WITH THE FILE ROUTE, same PR … loadEnvFiles() re-injects ~/.config/homelab/forgejo.env into pi's environment, so pi gets Aaron's key back however the launcher starts it. Stop that. Both changes together finish key removal for pi. Read that a new session's environment has no FORGEJO_API_KEY and forgejo_ci_status still works" — and the PR body's "Not covered here" list does not name it, so the omission is silent rather than an argued deferral. Measured on the head with env -u FORGEJO_API_KEY -u FORGEJO_API_KEY_FILE (vite-node, loadConfig() from the head worktree; only booleans and lengths printed): key absent at import → after loadEnvFiles() FORGEJO_API_KEY is in process.env → loadConfig() answers with a 40-char key, apiKeyError unset; with FORGEJO_API_KEY_FILE deleted the same 40-char key still answers. Consequence for acceptance: the order's completion read ("a new session's environment has no FORGEJO_API_KEY and forgejo_ci_status still works") cannot be performed against this head — a keyless session still gets a key from the env file, so the tool would answer even if the file leg were broken, which is exactly the masking the order says to remove.

Measurements (this seat, independent)

Diff scope, base = current main 55518b3504e1867024427bc8a35999b7c1ed0b88 (merge-base aa6d009, head d881e47): 5 files, all inside forgejo-ci/ — forgejo.ts (+79/-7), types.ts (+6), tests/api-key-file.test.ts (new, 244), README.md (+12/-2), forgejo-ci.env.example (+7/-2). Nothing outside the key-file leg; no scope creep.

Main moved under this branch (#116): git merge-tree --write-tree origin/main d881e47 → clean, single tree 7e2bb01, no conflicts; the merged forgejo.ts carries both #116's isPending("pending") and this change's apiKeyError guard at getPage.

Precedence and refusal semantics, traced in the merged tree rather than assumed: resolveApiKey() trims the variable, opens with readFileSync, trims the token; a file that opens and yields text wins over the environment (source: "key-file"); a throw from the open (missing, mode 0000, directory) returns {apiKey: undefined, source: "none", error} naming the path and the first line of the OS message, consulting nothing else; a readable-but-empty file falls through to the environment, exactly as the README says; neither source → source: "none" with no error of its own. Api.getPage raises cfg.apiKeyError before URLSearchParams, before the URL, before fetchImpl — my own probe (injected fetch counting calls) got the refusal thrown, message identical to the carried string, 0 requests issued; the no-token path likewise names both variables with 0 requests issued. apiKey reaches exactly one place in the whole module — the Authorization header inside getPage (grep of the head) — so no authenticated path escapes the guard; Api.get (web/cookie transport) and the log/artifact tools never read it. No child process: no child_process/spawn/execSync/execFile in non-test sources (the only cross-spawn hits are package-lock.json transitive deps), so there is no argv or env leg.

Divergences claimed against the draft (working-files/infra-stability/orch19/step2-keyfile-prep.md §5, read in full), each checked against the code: (1) the refusal leg IS inverted — the draft's readKeyFile catches and returns undefined so "a file that cannot be read … FALLS BACK", the head refuses; consistent with the order's loud leg. (2) The helper shape IS as claimed, and it does match the sibling: forgejo/api.ts:93-107 returns {apiKey, source, note}; the head returns {apiKey, source, error} — same shape, reason-carrying. (3) The draft's "optional second hunk" IS promoted and does the work: CIConfig.apiKeyError + the getPage guard, so no credential is left in the config to send. (4) Empty ≠ unreadable IS real and is documented (README: "An empty (but readable) file is not an error"). (5) The draft's "node_modules is empty, hence a raw-function probe" IS stale: forgejo-ci/node_modules has vitest 2.1.9 and tsc, both run. No divergence breaks the new-session goal: with only FORGEJO_API_KEY_FILE present the file leg resolves and answers (that leg is what B1 leaves unproven in production, not broken).

Tests at the head, run by me in the head worktree (.worktrees/HOMELAB-2682-ci-key-file, node_modules linked from the main clone because a fresh worktree has none): vitest run → 71 passed, 11 skipped, 5 files (4 passed, live.test.ts self-skips) — api-key-file.test.ts 19, forgejo.test.ts 24, tools.test.ts 12, regression.test.ts 16; tsc --noEmit clean (no output). Test fixtures: both token literals are 20 and 19 chars and appear in no other file at the head — local fakes, not a reused real credential.

Non-blocking, for the fix worker's same pass: forgejo-ci/index.ts:9 still lists "Env: FORGEJO_BASE_URL, FORGEJO_API_KEY," without the file variable, and forgejo-ci.env.example:8 says the variable is the fallback "when that variable is unset/blank/empty" without the unreadable-file refusal that README:118-122 states correctly.

Exposure — what I read and ran, and nothing else: this seat read the diff and merged tree from local git objects only in ~/repos/pi-extensions (no FORGEJO_API_KEY used for any read; no token printed, no token in any argv), read the draft §5, the order line 61, the builder's PR body and ci-key-file-build-findings.md, and forgejo/api.ts; ran vitest run and tsc --noEmit in the head worktree and two vite-node probes under /tmp/rv117 (printing booleans, lengths, request counts only, with the credential variables unset). Base 55518b3504e1867024427bc8a35999b7c1ed0b88, merge-base aa6d00972ad9e4d2df4e9c68f24b6d19ccf69ba9, head d881e47e0a1eea848ebd198cb16c406cb959044f. Reviewer only: no file edited, no commit, no push.

Verdict of record — ld-reviewer — head d881e47e0a1eea848ebd198cb16c406cb959044f: REQUEST_CHANGES model: omniroute/deepseek-flash session: 01a1b40b-8da6-70d7-89c1-cc3630ef6d1c ## Blockers B1 — the second half of the order this change serves is missing, and its absence masks the file leg: `forgejo-ci/forgejo.ts` `loadEnvFiles()` (head lines 50-78) is byte-identical to the base (`git show d881e47:forgejo-ci/forgejo.ts` lines 50-78 == base lines 50-78), so it still re-injects `FORGEJO_API_KEY` from `~/.config/homelab/forgejo.env` into the process environment; the order (`working-files/ldv0/tooling-order-livedev.md:61`) puts that work in THIS change — "WITH THE FILE ROUTE, same PR … `loadEnvFiles()` re-injects `~/.config/homelab/forgejo.env` into pi's environment, so pi gets Aaron's key back however the launcher starts it. Stop that. Both changes together finish key removal for pi. Read that a new session's environment has no FORGEJO_API_KEY and forgejo_ci_status still works" — and the PR body's "Not covered here" list does not name it, so the omission is silent rather than an argued deferral. Measured on the head with `env -u FORGEJO_API_KEY -u FORGEJO_API_KEY_FILE` (vite-node, `loadConfig()` from the head worktree; only booleans and lengths printed): key absent at import → after `loadEnvFiles()` `FORGEJO_API_KEY` is in `process.env` → `loadConfig()` answers with a 40-char key, `apiKeyError` unset; with `FORGEJO_API_KEY_FILE` deleted the same 40-char key still answers. Consequence for acceptance: the order's completion read ("a new session's environment has no FORGEJO_API_KEY and forgejo_ci_status still works") cannot be performed against this head — a keyless session still gets a key from the env file, so the tool would answer even if the file leg were broken, which is exactly the masking the order says to remove. ## Measurements (this seat, independent) Diff scope, base = current `main` `55518b3504e1867024427bc8a35999b7c1ed0b88` (merge-base `aa6d009`, head `d881e47`): 5 files, all inside `forgejo-ci/` — `forgejo.ts` (+79/-7), `types.ts` (+6), `tests/api-key-file.test.ts` (new, 244), `README.md` (+12/-2), `forgejo-ci.env.example` (+7/-2). Nothing outside the key-file leg; no scope creep. Main moved under this branch (`#116`): `git merge-tree --write-tree origin/main d881e47` → clean, single tree `7e2bb01`, no conflicts; the merged `forgejo.ts` carries both `#116`'s `isPending("pending")` and this change's `apiKeyError` guard at `getPage`. Precedence and refusal semantics, traced in the merged tree rather than assumed: `resolveApiKey()` trims the variable, opens with `readFileSync`, trims the token; a file that opens and yields text wins over the environment (`source: "key-file"`); a throw from the open (missing, mode 0000, directory) returns `{apiKey: undefined, source: "none", error}` naming the path and the first line of the OS message, consulting nothing else; a readable-but-empty file falls through to the environment, exactly as the README says; neither source → `source: "none"` with no error of its own. `Api.getPage` raises `cfg.apiKeyError` before `URLSearchParams`, before the URL, before `fetchImpl` — my own probe (injected fetch counting calls) got the refusal thrown, message identical to the carried string, **0 requests issued**; the no-token path likewise names both variables with **0 requests issued**. `apiKey` reaches exactly one place in the whole module — the `Authorization` header inside `getPage` (grep of the head) — so no authenticated path escapes the guard; `Api.get` (web/cookie transport) and the log/artifact tools never read it. No child process: no `child_process`/`spawn`/`execSync`/`execFile` in non-test sources (the only `cross-spawn` hits are `package-lock.json` transitive deps), so there is no argv or env leg. Divergences claimed against the draft (`working-files/infra-stability/orch19/step2-keyfile-prep.md` §5, read in full), each checked against the code: (1) the refusal leg IS inverted — the draft's `readKeyFile` catches and returns `undefined` so "a file that cannot be read … FALLS BACK", the head refuses; consistent with the order's loud leg. (2) The helper shape IS as claimed, and it does match the sibling: `forgejo/api.ts:93-107` returns `{apiKey, source, note}`; the head returns `{apiKey, source, error}` — same shape, reason-carrying. (3) The draft's "optional second hunk" IS promoted and does the work: `CIConfig.apiKeyError` + the `getPage` guard, so no credential is left in the config to send. (4) Empty ≠ unreadable IS real and is documented (README: "An empty (but readable) file is not an error"). (5) The draft's "`node_modules` is empty, hence a raw-function probe" IS stale: `forgejo-ci/node_modules` has vitest 2.1.9 and tsc, both run. No divergence breaks the new-session goal: with only `FORGEJO_API_KEY_FILE` present the file leg resolves and answers (that leg is what B1 leaves unproven in production, not broken). Tests at the head, run by me in the head worktree (`.worktrees/HOMELAB-2682-ci-key-file`, `node_modules` linked from the main clone because a fresh worktree has none): `vitest run` → **71 passed, 11 skipped**, 5 files (4 passed, `live.test.ts` self-skips) — `api-key-file.test.ts` 19, `forgejo.test.ts` 24, `tools.test.ts` 12, `regression.test.ts` 16; `tsc --noEmit` clean (no output). Test fixtures: both token literals are 20 and 19 chars and appear in no other file at the head — local fakes, not a reused real credential. Non-blocking, for the fix worker's same pass: `forgejo-ci/index.ts:9` still lists "Env: FORGEJO_BASE_URL, FORGEJO_API_KEY," without the file variable, and `forgejo-ci.env.example:8` says the variable is the fallback "when that variable is unset/blank/empty" without the unreadable-file refusal that README:118-122 states correctly. Exposure — what I read and ran, and nothing else: this seat read the diff and merged tree from **local git objects only** in `~/repos/pi-extensions` (no `FORGEJO_API_KEY` used for any read; no token printed, no token in any argv), read the draft §5, the order line 61, the builder's PR body and `ci-key-file-build-findings.md`, and `forgejo/api.ts`; ran `vitest run` and `tsc --noEmit` in the head worktree and two `vite-node` probes under `/tmp/rv117` (printing booleans, lengths, request counts only, with the credential variables unset). Base `55518b3504e1867024427bc8a35999b7c1ed0b88`, merge-base `aa6d00972ad9e4d2df4e9c68f24b6d19ccf69ba9`, head `d881e47e0a1eea848ebd198cb16c406cb959044f`. Reviewer only: no file edited, no commit, no push.

aaron referenced this pull request from a commit 2026-11-07 03:41:24 +00:00

HOMELAB-2682: fix(forgejo-ci): stop forgejo.env re-injecting the API key (scoped)

aaron added 1 commit 2026-11-07 03:41:24 +00:00

HOMELAB-2682: fix(forgejo-ci): stop forgejo.env re-injecting the API key (scoped) ... a502276e67

B1 on PR #117: the order puts two changes in one PR — the credential-file
route AND stopping `loadEnvFiles()` from putting `FORGEJO_API_KEY` back into
`process.env` from `~/.config/homelab/forgejo.env`. Only the first was here.

Measured on the previous head (`d881e47`) with neither variable exported:
after `loadEnvFiles()` the key was back in `process.env` (40 chars) and
`resolveApiKey()` answered `source: "env"`. So the file route could be
entirely broken and every tool would still answer, and the order's completion
read ("a new session's environment has no FORGEJO_API_KEY and
forgejo_ci_status still works") could not be performed at all.

Mechanism: `ENV_FILE_NEVER_INJECT` — `loadEnvFiles()` skips `FORGEJO_API_KEY`
when copying KEY=VALUE lines. Skipping, rather than recording provenance and
teaching the resolver to ignore env-file values, because provenance would
still leave the value in `process.env`, inherited by every child process and
readable at `/proc/<pid>/environ` — the leak the file route exists to remove.

Scoped deliberately: `FORGEJO_API_KEY_FILE` is a path, not a credential, and
stays eligible; `FORGEJO_WEB_PASSWORD` / `FORGEJO_SESSION_COOKIE` stay eligible
because the web-route log/artifact tools have no other source for them. A
genuinely EXPORTED `FORGEJO_API_KEY` still works as the fallback — the file is
denied as a source, the environment is not.

`loadEnvFiles(files = ENV_FILES)` takes the list as an optional argument so the
loader can be exercised against a staged fixture instead of `$HOME`.

Tests (new file `tests/env-file-key.test.ts`, 7): env-file key is neither
injected nor a source; the real `~/.config/homelab` files do not put a key
back; the files still load the web session and base URL; an exported key is
still the fallback; the file route still wins over an env file; the unreadable
-file refusal still fires with an env file present and issues no request.
Value comparisons are boolean throughout, so a failure cannot echo a credential.

Red then green: 4 failed | 3 passed of 7 before the fix (the 3 are the
regression guards, which already held), 7 passed after. Full package suite:
78 passed | 11 skipped (was 71 + 11; the 19 key-file tests still pass).
`tsc --noEmit` clean.

Live, with neither variable exported: file route -> `source: key-file`, key not
in `process.env`, `runs("Homelab","pi-extensions")` -> 6 items of 411; no file
route -> `source: none`, key not in `process.env`, refusal naming both
variables, no request issued.

ld-reviewer commented 2026-11-07 04:47:06 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head a502276e67: APPROVE

model: deepseek-flash
session: 01a1b452-3e28-702c-a118-436124958b51

Blockers

None.

A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER : line.

What I verified at this head (git objects only)

Order item 1 — FORGEJO_API_KEY_FILE first.

• resolveApiKey(env, readFile) trims the path ((env[API_KEY_FILE_ENV] ?? "").trim()), reads the file, and trims the token ((text ?? "").trim()) before using it. When the path is non-blank and yields a token, it returns inside the if (filePath) block — FORGEJO_API_KEY is not read on that path.
• A failed read is caught and returns {apiKey: undefined, source: "none", error} before the environment is consulted; there is no fallback leg out of that branch. The message interpolates the trimmed path, the variable name, and the first line of the reader's error only — never file contents, never the token.
• readFileSync throws for a missing path (ENOENT), an unreadable path (EACCES) and a directory (EISDIR), so all three reach that refusal; the new tests exercise the three as separate cases.
• Blank/whitespace path, or a readable file whose trimmed content is empty, falls through to FORGEJO_API_KEY — the code order puts the env read after the if (filePath) block, and the tests assert that fall-through for both.
• The no-token message names both variables: no CI read token: point FORGEJO_API_KEY_FILE at a readable 0600 file, or set FORGEJO_API_KEY — ....
• Refusal precedes the request: Api.getPage checks this.cfg.apiKeyError and then !this.cfg.apiKey before the query string, the URL, or any fetch call. paged() reaches fetch only through getPage; in the whole file apiKey is used only inside getPage, and only in the Authorization: token … header — never in a URL. loadConfig() carries resolved.error into cfg.apiKeyError (added to CIConfig in types.ts as optional).

Order item 2 — loadEnvFiles() stops re-injecting the key.

• ENV_FILE_NEVER_INJECT = new Set(["FORGEJO_API_KEY"]) and the loader continues for that key, so no env file can put the value into process.env; every other key is still copied (a new test asserts FORGEJO_WEB_USERNAME, FORGEJO_WEB_PASSWORD, FORGEJO_BASE_URL still load from a staged file — i.e. this is not "stop reading the files").
• The loader now takes the file list as an optional parameter defaulting to the real ENV_FILES, which is what makes a staged-fixture test possible; the default path is still exercised by a separate test that calls loadEnvFiles() with no argument.
• Tests pin the behaviour with value-sensitive assertions written as booleans (expect(process.env[KEY_VAR] === undefined).toBe(true), expect(cfg.apiKey === ENV_FILE_TOKEN).toBe(false)), so a failing assertion cannot echo a key; the token-shaped literals in the tests are placeholders.

Deviation I am recording, not blocking on. The set holds FORGEJO_API_KEY only — FORGEJO_API_KEY_FILE is deliberately not skipped, while the order's second item names "(and the file variable)". The source comment states the reason (a path is not a credential; the loader keeps exporting the web credentials for the same reason). Consequence I verified in the code: on a host whose forgejo.env itself names FORGEJO_API_KEY_FILE, a session launched with neither variable would still resolve a token from that path through the loader — the credential value still never enters the environment, because the loader has no path that sets FORGEJO_API_KEY from a file. What that scenario does mask is a broken launcher pointer, which is the argument used to deny the key itself. I judged this a documented deviation that exposes no credential, not a blocker; the orchestrator should either accept it as stated or ask for the path variable to be skipped too.

No credential on any output path. forgejo.ts contains no console.*, no process.stdout write and no JSON.stringify; the only use of the key is the request header. The refusal message carries the path and the reader's first error line. A source-scanning test fails if child_process/execFile/execSync/spawnSync/Bun.spawn ever appear in forgejo.ts, which is the argv leg. The web-session login posts user_name/password in a request body (pre-existing code, unchanged by this diff).

Base and merge observation. The head's merge-base with main is aa6d00972ad9e4d2df4e9c68f24b6d19ccf69ba9, not 55518b35: the branch predates the merge of PR #116. So the two-dot diff shows isPending losing "pending", STATUS_ICON losing pending: "⏳" and two #116 tests disappearing — and neither commit in the range touches those lines (git show of each commit). git merge-tree --write-tree 55518b35 a502276 completes with no conflict, and in the merged tree isPending still lists "pending", index.ts still has pending: "⏳", and tests/forgejo.test.ts still holds the treats a commit-status pending as pending case. I record this because the diff a reader sees says the opposite of what a three-way merge produces, and any landing path that applies the two-dot diff literally would revert #116.

Test hygiene note (non-blocking). env-file-key.test.ts → "the REAL homelab env files do not put a key back either" calls loadEnvFiles() with the default list, so it reads the operator's own ~/.config/homelab/*.env into the vitest process environment (its web credentials included) for that test; afterEach restores the watched names. The test's own comment acknowledges the assertion is trivially satisfiable where those files are absent.

I did not execute the suites: this checkout has no node_modules.

Exposure: git objects of Homelab/pi-extensions at 55518b35..a502276e only; no PR comments, census, or ticket thread read; my only write is this one verdict posted as ld-reviewer, preceded by one read of this PR's branch_protections/main.

Verdict of record — ld-reviewer — head a502276e671c948328ecc91a1fe7b0f6c6aa4701: APPROVE model: deepseek-flash session: 01a1b452-3e28-702c-a118-436124958b51 ## Blockers None. A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER <id>: line. ### What I verified at this head (git objects only) **Order item 1 — `FORGEJO_API_KEY_FILE` first.** - `resolveApiKey(env, readFile)` trims the path (`(env[API_KEY_FILE_ENV] ?? "").trim()`), reads the file, and trims the token (`(text ?? "").trim()`) before using it. When the path is non-blank and yields a token, it returns inside the `if (filePath)` block — `FORGEJO_API_KEY` is not read on that path. - A failed read is caught and returns `{apiKey: undefined, source: "none", error}` **before** the environment is consulted; there is no fallback leg out of that branch. The message interpolates the trimmed path, the variable name, and the first line of the reader's error only — never file contents, never the token. - `readFileSync` throws for a missing path (ENOENT), an unreadable path (EACCES) and a directory (EISDIR), so all three reach that refusal; the new tests exercise the three as separate cases. - Blank/whitespace path, or a readable file whose trimmed content is empty, falls through to `FORGEJO_API_KEY` — the code order puts the env read after the `if (filePath)` block, and the tests assert that fall-through for both. - The no-token message names both variables: `no CI read token: point FORGEJO_API_KEY_FILE at a readable 0600 file, or set FORGEJO_API_KEY — ...`. - Refusal precedes the request: `Api.getPage` checks `this.cfg.apiKeyError` and then `!this.cfg.apiKey` before the query string, the URL, or any `fetch` call. `paged()` reaches `fetch` only through `getPage`; in the whole file `apiKey` is used only inside `getPage`, and only in the `Authorization: token …` header — never in a URL. `loadConfig()` carries `resolved.error` into `cfg.apiKeyError` (added to `CIConfig` in `types.ts` as optional). **Order item 2 — `loadEnvFiles()` stops re-injecting the key.** - `ENV_FILE_NEVER_INJECT = new Set(["FORGEJO_API_KEY"])` and the loader `continue`s for that key, so no env file can put the value into `process.env`; every other key is still copied (a new test asserts `FORGEJO_WEB_USERNAME`, `FORGEJO_WEB_PASSWORD`, `FORGEJO_BASE_URL` still load from a staged file — i.e. this is not "stop reading the files"). - The loader now takes the file list as an optional parameter defaulting to the real `ENV_FILES`, which is what makes a staged-fixture test possible; the default path is still exercised by a separate test that calls `loadEnvFiles()` with no argument. - Tests pin the behaviour with value-sensitive assertions written as booleans (`expect(process.env[KEY_VAR] === undefined).toBe(true)`, `expect(cfg.apiKey === ENV_FILE_TOKEN).toBe(false)`), so a failing assertion cannot echo a key; the token-shaped literals in the tests are placeholders. **Deviation I am recording, not blocking on.** The set holds `FORGEJO_API_KEY` only — `FORGEJO_API_KEY_FILE` is deliberately *not* skipped, while the order's second item names "(and the file variable)". The source comment states the reason (a path is not a credential; the loader keeps exporting the web credentials for the same reason). Consequence I verified in the code: on a host whose `forgejo.env` itself names `FORGEJO_API_KEY_FILE`, a session launched with neither variable would still resolve a token from that path through the loader — the credential value still never enters the environment, because the loader has no path that sets `FORGEJO_API_KEY` from a file. What that scenario does mask is a *broken launcher pointer*, which is the argument used to deny the key itself. I judged this a documented deviation that exposes no credential, not a blocker; the orchestrator should either accept it as stated or ask for the path variable to be skipped too. **No credential on any output path.** `forgejo.ts` contains no `console.*`, no `process.stdout` write and no `JSON.stringify`; the only use of the key is the request header. The refusal message carries the path and the reader's first error line. A source-scanning test fails if `child_process`/`execFile`/`execSync`/`spawnSync`/`Bun.spawn` ever appear in `forgejo.ts`, which is the argv leg. The web-session login posts `user_name`/`password` in a request body (pre-existing code, unchanged by this diff). **Base and merge observation.** The head's merge-base with `main` is `aa6d00972ad9e4d2df4e9c68f24b6d19ccf69ba9`, not `55518b35`: the branch predates the merge of PR #116. So the two-dot diff shows `isPending` losing `"pending"`, `STATUS_ICON` losing `pending: "⏳"` and two #116 tests disappearing — and neither commit in the range touches those lines (`git show` of each commit). `git merge-tree --write-tree 55518b35 a502276` completes with no conflict, and in the merged tree `isPending` still lists `"pending"`, `index.ts` still has `pending: "⏳"`, and `tests/forgejo.test.ts` still holds the `treats a commit-status pending as pending` case. I record this because the diff a reader sees says the opposite of what a three-way merge produces, and any landing path that applies the two-dot diff literally would revert #116. **Test hygiene note (non-blocking).** `env-file-key.test.ts` → "the REAL homelab env files do not put a key back either" calls `loadEnvFiles()` with the default list, so it reads the operator's own `~/.config/homelab/*.env` into the vitest process environment (its web credentials included) for that test; `afterEach` restores the watched names. The test's own comment acknowledges the assertion is trivially satisfiable where those files are absent. I did not execute the suites: this checkout has no `node_modules`. Exposure: git objects of Homelab/pi-extensions at 55518b35..a502276e only; no PR comments, census, or ticket thread read; my only write is this one verdict posted as ld-reviewer, preceded by one read of this PR's `branch_protections/main`.

ld-reviewer commented 2026-11-07 04:49:20 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head a502276e67: REQUEST_CHANGES

model: omniroute/deepseek-flash
session: 01a1b452-3df6-702c-a118-435fda35a6c3

Blockers

• B1: blocker — loadEnvFiles() still injects FORGEJO_API_KEY_FILE from the env files. ENV_FILE_NEVER_INJECT (forgejo-ci/forgejo.ts:86) holds FORGEJO_API_KEY only, so a session with neither variable exported still acquires a usable CI token from ~/.config/homelab/forgejo-ci.env / forgejo.env. Item 2 of the order names the file variable too, and no test pins it either way. Verified at this head with a staged env file naming a 0600 token: loadEnvFiles([envFile]) left FORGEJO_API_KEY absent from process.env, injected the path, and resolveApiKey(process.env) then returned source: "key-file" carrying the staged token with no error. That is the "pi gets the key back however the launcher starts it" path the order removes, now carried by the pointer instead of the value — and it is the exact line the PR's own forgejo-ci.env.example and forgejo-ci/README.md now tell the operator to write. The comment added at forgejo.ts:93-94 states the invariant "a credential must arrive from the caller's real environment or from the 0600 file named by FORGEJO_API_KEY_FILE, never from a file this module reads on its own"; the loader does not enforce it.

What I verified at this head

Item 1 (file first), by reading the head and the new tests:

• precedence FORGEJO_API_KEY_FILE → FORGEJO_API_KEY in resolveApiKey: the file wins when both are present and answers alone when the variable is absent.
• the path and the token are both .trim()ed (trailing spaces in the variable; LF, CRLF and surrounding spaces in the token).
• missing / unreadable (mode 0000) / directory named by the variable: apiKey undefined, source: "none", error set and naming the path, the environment not consulted. Three separate tests, one per case.
• readable-but-empty (and newline-only) file: falls through to the environment, no error.
• neither source: the thrown message names both FORGEJO_API_KEY_FILE and FORGEJO_API_KEY.
• the refusal is raised in getPage before the URL is assembled and before fetchImpl is called; the tests assert the fake server saw zero calls. The Authorization header is set at exactly one place in this file (getPage), and WebSession authenticates by cookie and never reads apiKey, so a broken pointer cannot reach the wire some other way. The swallow in Api.runByNumber is followed in the same loop by api.runs, which re-raises the refusal, so forgejo_ci_status and forgejo_ci_failures surface it instead of reporting "no runs found".

Item 2 (env files), by reading the head and by running this head's module against a staged fixture:

• FORGEJO_API_KEY named in an env file is skipped, never reaches process.env, and resolveApiKey then reports source: "none". Pinned by tests.
• the files still supply their non-key settings (base URL, web session), a genuinely exported key is still the fallback, and the key file still wins over an env-file key.
• B1 above is the part of item 2 that is not done.

Hygiene:

• no credential value on any path I read: the refusal text is built from the path and err.message only, and the tests assert both token values are absent from it; the env-file suite asserts booleans so a failing assertion cannot echo a token.
• no argv and no URL: the module contains no child_process / exec* / spawn* (a test pins that statically), and the token reaches the wire only as an Authorization: token … header — nothing is placed in a query string.

Not blocking (recorded, no effect on the vote)

• docs/forgejo-ci.md:34 still reads FORGEJO_API_KEY … "(~/.config/homelab/forgejo.env also supplies it)", which this change makes false. The extension's own README and env example were updated; that page was not.
• tests/env-file-key.test.ts, "the REAL homelab env files do not put a key back either", calls loadEnvFiles() with the real default file list and asserts resolveApiKey(process.env).source === "none". It is not hermetic: on a machine whose env file names FORGEJO_API_KEY_FILE — the line the env example now documents — the loader injects that path and the assertion fails. The same test encodes the expectation B1 declines to implement.
• FORGEJO_WEB_PASSWORD / FORGEJO_SESSION_COOKIE remain injectable from the env files by design (the added comment argues the log/artifact tools have no other source). The order named only the key and I do not vote on it; noted because it is a credential that still enters process.env from those files.

Branch rule

GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main with the staged ld-reviewer credential answered HTTP 403 — user should be an owner or a collaborator with admin write of a repository. I therefore did not read required_approvals myself, and I state that value as unverified by me. This verdict is posted as a PR comment, the route the sanctioned posting template records for this repository.

Diff-scope note (git objects, not a finding)

The branch's merge-base with main is aa6d0097 (#109); main has since moved to 55518b35 (#116, commit 72c18ab), which this branch does not contain. The two-dot diff 55518b35..a502276e therefore shows isPending losing "pending" and STATUS_ICON losing pending. That region is unchanged by this branch relative to aa6d0097 (git diff aa6d0097..a502276e has no such hunk): it is main's commit showing up as a removal because the branch is behind. Landing must not put the branch's tree in place of main's, or #116 would be dropped.

A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER : line.

Exposure: git objects of Homelab/pi-extensions only — 55518b35..a502276e, the merge-base aa6d0097 for comparison, and file contents at a502276e — plus that head's forgejo.ts executed from a throwaway /tmp copy against a staged fixture; the only file read under working-files/ was the posting template, copied with REPO/PR changed; no PR comments, no landing census, no ticket thread read; the only API reads were GET /api/v1/user and GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main with the staged ld-reviewer credential; my only write is this one verdict posted as ld-reviewer.

Verdict of record — ld-reviewer — head a502276e671c948328ecc91a1fe7b0f6c6aa4701: REQUEST_CHANGES model: omniroute/deepseek-flash session: 01a1b452-3df6-702c-a118-435fda35a6c3 ## Blockers - B1: blocker — `loadEnvFiles()` still injects `FORGEJO_API_KEY_FILE` from the env files. `ENV_FILE_NEVER_INJECT` (forgejo-ci/forgejo.ts:86) holds `FORGEJO_API_KEY` only, so a session with neither variable exported still acquires a usable CI token from `~/.config/homelab/forgejo-ci.env` / `forgejo.env`. Item 2 of the order names the file variable too, and no test pins it either way. Verified at this head with a staged env file naming a 0600 token: `loadEnvFiles([envFile])` left `FORGEJO_API_KEY` absent from `process.env`, injected the path, and `resolveApiKey(process.env)` then returned `source: "key-file"` carrying the staged token with no error. That is the "pi gets the key back however the launcher starts it" path the order removes, now carried by the pointer instead of the value — and it is the exact line the PR's own `forgejo-ci.env.example` and `forgejo-ci/README.md` now tell the operator to write. The comment added at forgejo.ts:93-94 states the invariant "a credential must arrive from the caller's real environment or from the 0600 file named by `FORGEJO_API_KEY_FILE`, never from a file this module reads on its own"; the loader does not enforce it. ## What I verified at this head Item 1 (file first), by reading the head and the new tests: - precedence `FORGEJO_API_KEY_FILE` → `FORGEJO_API_KEY` in `resolveApiKey`: the file wins when both are present and answers alone when the variable is absent. - the path and the token are both `.trim()`ed (trailing spaces in the variable; LF, CRLF and surrounding spaces in the token). - missing / unreadable (mode 0000) / directory named by the variable: `apiKey` undefined, `source: "none"`, `error` set and naming the path, the environment not consulted. Three separate tests, one per case. - readable-but-empty (and newline-only) file: falls through to the environment, no error. - neither source: the thrown message names both `FORGEJO_API_KEY_FILE` and `FORGEJO_API_KEY`. - the refusal is raised in `getPage` before the URL is assembled and before `fetchImpl` is called; the tests assert the fake server saw zero calls. The `Authorization` header is set at exactly one place in this file (`getPage`), and `WebSession` authenticates by cookie and never reads `apiKey`, so a broken pointer cannot reach the wire some other way. The swallow in `Api.runByNumber` is followed in the same loop by `api.runs`, which re-raises the refusal, so `forgejo_ci_status` and `forgejo_ci_failures` surface it instead of reporting "no runs found". Item 2 (env files), by reading the head and by running this head's module against a staged fixture: - `FORGEJO_API_KEY` named in an env file is skipped, never reaches `process.env`, and `resolveApiKey` then reports `source: "none"`. Pinned by tests. - the files still supply their non-key settings (base URL, web session), a genuinely exported key is still the fallback, and the key file still wins over an env-file key. - B1 above is the part of item 2 that is not done. Hygiene: - no credential value on any path I read: the refusal text is built from the path and `err.message` only, and the tests assert both token values are absent from it; the env-file suite asserts booleans so a failing assertion cannot echo a token. - no argv and no URL: the module contains no `child_process` / `exec*` / `spawn*` (a test pins that statically), and the token reaches the wire only as an `Authorization: token …` header — nothing is placed in a query string. ## Not blocking (recorded, no effect on the vote) - `docs/forgejo-ci.md:34` still reads `FORGEJO_API_KEY` … "(`~/.config/homelab/forgejo.env` also supplies it)", which this change makes false. The extension's own README and env example were updated; that page was not. - `tests/env-file-key.test.ts`, "the REAL homelab env files do not put a key back either", calls `loadEnvFiles()` with the real default file list and asserts `resolveApiKey(process.env).source === "none"`. It is not hermetic: on a machine whose env file names `FORGEJO_API_KEY_FILE` — the line the env example now documents — the loader injects that path and the assertion fails. The same test encodes the expectation B1 declines to implement. - `FORGEJO_WEB_PASSWORD` / `FORGEJO_SESSION_COOKIE` remain injectable from the env files by design (the added comment argues the log/artifact tools have no other source). The order named only the key and I do not vote on it; noted because it is a credential that still enters `process.env` from those files. ## Branch rule `GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main` with the staged ld-reviewer credential answered HTTP 403 — `user should be an owner or a collaborator with admin write of a repository`. I therefore did not read `required_approvals` myself, and I state that value as unverified by me. This verdict is posted as a PR comment, the route the sanctioned posting template records for this repository. ## Diff-scope note (git objects, not a finding) The branch's merge-base with main is `aa6d0097` (#109); main has since moved to `55518b35` (#116, commit `72c18ab`), which this branch does not contain. The two-dot diff `55518b35..a502276e` therefore shows `isPending` losing `"pending"` and `STATUS_ICON` losing `pending`. That region is unchanged by this branch relative to `aa6d0097` (`git diff aa6d0097..a502276e` has no such hunk): it is main's commit showing up as a removal because the branch is behind. Landing must not put the branch's tree in place of main's, or #116 would be dropped. A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER <id>: line. Exposure: git objects of Homelab/pi-extensions only — 55518b35..a502276e, the merge-base aa6d0097 for comparison, and file contents at a502276e — plus that head's `forgejo.ts` executed from a throwaway /tmp copy against a staged fixture; the only file read under working-files/ was the posting template, copied with REPO/PR changed; no PR comments, no landing census, no ticket thread read; the only API reads were `GET /api/v1/user` and `GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main` with the staged ld-reviewer credential; my only write is this one verdict posted as ld-reviewer.

aaron referenced this pull request from a commit 2026-11-07 04:55:27 +00:00

HOMELAB-2682: fix(forgejo-ci): loadEnvFiles skips FORGEJO_API_KEY_FILE re-injection

aaron added 1 commit 2026-11-07 04:55:27 +00:00

HOMELAB-2682: fix(forgejo-ci): loadEnvFiles skips FORGEJO_API_KEY_FILE re-injection ... 5edf088990

B1 on PR #117: loadEnvFiles() skipped FORGEJO_API_KEY but still injected
FORGEJO_API_KEY_FILE from ~/.config/homelab/forgejo.env. A pointer is a route:
a session started with neither variable exported still resolved a working token
(source "key-file", no error) — "pi gets the key back however the launcher
starts it". The skip list now names both variables, and a comment records why
the path must stay listed so it is not "fixed" back.

Tests pin the behaviour first (3 legs failed at a502276e): a readable pointer in
an env file is not injected and resolves nothing; both credential variables are
skipped while every other key still loads; the real homelab env files inject
neither. A pointer the caller exported still configures the route.

docs/forgejo-ci.md: the env-file sentence now says both variables are skipped
and that the file route is chosen by the caller's exported environment.

aaron added 1 commit 2026-11-07 04:58:49 +00:00

HOMELAB-2682: docs(forgejo-ci): credential variables are caller-exported, not env-file 6fb394ce43

ld-reviewer commented 2026-11-07 05:06:20 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 6fb394ce43: APPROVE

model: omniroute/deepseek-flash
session: 01a1b463-a4aa-702c-a118-436c17115330

Blockers

None.

Fixed text (standing rule)

Fixed text, verbatim: "A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER : line."

Branch rule

GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main with the staged ld-reviewer credential answered HTTP 403 ("user should be an owner or a collaborator with admin write of a repository"), so required_approvals on this repo could not be read by me. Per the documented state of pi-extensions (no branch protection on main) I posted this as a comment rather than a formal review.

What I verified

1. forgejo-ci/forgejo.ts credential resolution. resolveApiKey() trims the pointer path and tries FORGEJO_API_KEY_FILE first; a readable file whose content trims to non-empty answers source: "key-file". FORGEJO_API_KEY is consulted only when the pointer is unset/blank, or when the file is readable and empty (including newline-only) — I checked both cases in the source and they are covered by tests I ran. An unreadable, missing, or directory path returns {apiKey: undefined, source: "none", error} whose message names the path and the variable, and the environment is not consulted — the message contains only the path, no credential value. The no-token message names both FORGEJO_API_KEY_FILE and FORGEJO_API_KEY. Api.getPage() raises apiKeyError before it builds the query string or calls fetch, so nothing is sent behind a broken pointer; all REST calls in the module go through getPage (the other fetchImpl uses are in WebSession, which sends a cookie, not the token), and runByNumber's catch returns null only until the following runs call re-raises the same error.
2. loadEnvFiles(). ENV_FILE_NEVER_INJECT = {FORGEJO_API_KEY, FORGEJO_API_KEY_FILE} is consulted after the KEY=VALUE match and before the "existing process env wins" guard, so both variables are skipped (including export -prefixed lines) while the remaining keys still load. I extracted this head's forgejo-ci tree and ran its own suite: tests/env-file-key.test.ts and tests/api-key-file.test.ts pass, 30 tests. The same two test files run against commit a502276 (the commit immediately before the FILE skip was added) fail 3 tests, including "a FILE pointer in an env file is not injected, so a keyless session resolves nothing" and "skips BOTH credential variables while every other key still loads"; against d881e47 7 of the 11 fail. Credential assertions on those paths are boolean comparisons, not echoed values.
3. Docs. At this head, forgejo-ci/README.md, forgejo-ci/forgejo-ci.env.example and docs/forgejo-ci.md no longer present FORGEJO_API_KEY (or FORGEJO_API_KEY_FILE) as an env-file setting; each states that the loader skips both and that the credential is exported by the caller, with the _FILE route named as the preferred one. docs/forgejo-ci.md shows only FORGEJO_API_KEY as a table row and names FORGEJO_API_KEY_FILE in that row's meaning.
4. No path to a printed credential. forgejo-ci/forgejo.ts and forgejo-ci/index.ts at this head contain no console.*, process.stdout, or process.stderr write, no child_process/spawn/exec use (the head also carries a static tripwire test for that), and the token reaches only the Authorization request header — it is not interpolated into a URL, a query string, or argv. tsc --noEmit on the head's forgejo-ci tree is clean, and the suite excluding tests/live.test.ts passes, 82 tests; I ran nothing that needs a live Forgejo or a real token.

Non-blocking observations

• forgejo-ci/tests/env-file-key.test.ts carries a docstring line saying "The PR's own env example tells operators to write FORGEJO_API_KEY_FILE into ~/.config/homelab/forgejo.env", which contradicts this head's forgejo-ci.env.example (it says neither variable belongs there). It is an internal test comment, not an operator-facing instruction.
• resolveApiKey() trims only the ends of the file content, so a file with more than one non-empty line yields a token containing an interior newline; the request then fails as an invalid header rather than naming the file. Outside the order's words, and it cannot expose the value.
• The two tests that read the real ~/.config/homelab/*.env files pass trivially on a machine where those files do not name the credential variables; the fixture tests beside them are the ones that pin the behaviour.

Exposure: git objects of Homelab/pi-extensions at aa6d0097..6fb394ce433a589cba395fa7e55da7c529ad238e (fetch, diff, log, and file contents at the head), plus that head's forgejo-ci tree extracted to a temp directory to run its own vitest and tsc; the branch-protection read and this verdict were made with the staged ld-reviewer credential; I read no PR comments, no landing census, and no Plane ticket, and never touched FORGEJO_API_KEY; my only write is this one verdict posted as ld-reviewer.

Verdict of record — ld-reviewer — head 6fb394ce433a589cba395fa7e55da7c529ad238e: APPROVE model: omniroute/deepseek-flash session: 01a1b463-a4aa-702c-a118-436c17115330 ## Blockers None. ### Fixed text (standing rule) Fixed text, verbatim: "A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER <id>: line." ### Branch rule GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main with the staged ld-reviewer credential answered HTTP 403 ("user should be an owner or a collaborator with admin write of a repository"), so `required_approvals` on this repo could not be read by me. Per the documented state of pi-extensions (no branch protection on main) I posted this as a comment rather than a formal review. ### What I verified 1. **`forgejo-ci/forgejo.ts` credential resolution.** `resolveApiKey()` trims the pointer path and tries `FORGEJO_API_KEY_FILE` first; a readable file whose content trims to non-empty answers `source: "key-file"`. `FORGEJO_API_KEY` is consulted only when the pointer is unset/blank, or when the file is readable and empty (including newline-only) — I checked both cases in the source and they are covered by tests I ran. An unreadable, missing, or directory path returns `{apiKey: undefined, source: "none", error}` whose message names the path and the variable, and the environment is not consulted — the message contains only the path, no credential value. The no-token message names both `FORGEJO_API_KEY_FILE` and `FORGEJO_API_KEY`. `Api.getPage()` raises `apiKeyError` before it builds the query string or calls fetch, so nothing is sent behind a broken pointer; all REST calls in the module go through `getPage` (the other `fetchImpl` uses are in `WebSession`, which sends a cookie, not the token), and `runByNumber`'s catch returns null only until the following `runs` call re-raises the same error. 2. **`loadEnvFiles()`.** `ENV_FILE_NEVER_INJECT = {FORGEJO_API_KEY, FORGEJO_API_KEY_FILE}` is consulted after the KEY=VALUE match and before the "existing process env wins" guard, so both variables are skipped (including `export `-prefixed lines) while the remaining keys still load. I extracted this head's `forgejo-ci` tree and ran its own suite: `tests/env-file-key.test.ts` and `tests/api-key-file.test.ts` pass, 30 tests. The same two test files run against commit `a502276` (the commit immediately before the FILE skip was added) fail 3 tests, including "a FILE pointer in an env file is not injected, so a keyless session resolves nothing" and "skips BOTH credential variables while every other key still loads"; against `d881e47` 7 of the 11 fail. Credential assertions on those paths are boolean comparisons, not echoed values. 3. **Docs.** At this head, `forgejo-ci/README.md`, `forgejo-ci/forgejo-ci.env.example` and `docs/forgejo-ci.md` no longer present `FORGEJO_API_KEY` (or `FORGEJO_API_KEY_FILE`) as an env-file setting; each states that the loader skips both and that the credential is exported by the caller, with the `_FILE` route named as the preferred one. `docs/forgejo-ci.md` shows only `FORGEJO_API_KEY` as a table row and names `FORGEJO_API_KEY_FILE` in that row's meaning. 4. **No path to a printed credential.** `forgejo-ci/forgejo.ts` and `forgejo-ci/index.ts` at this head contain no `console.*`, `process.stdout`, or `process.stderr` write, no `child_process`/`spawn`/`exec` use (the head also carries a static tripwire test for that), and the token reaches only the `Authorization` request header — it is not interpolated into a URL, a query string, or argv. `tsc --noEmit` on the head's `forgejo-ci` tree is clean, and the suite excluding `tests/live.test.ts` passes, 82 tests; I ran nothing that needs a live Forgejo or a real token. ### Non-blocking observations - `forgejo-ci/tests/env-file-key.test.ts` carries a docstring line saying "The PR's own env example tells operators to write `FORGEJO_API_KEY_FILE` into `~/.config/homelab/forgejo.env`", which contradicts this head's `forgejo-ci.env.example` (it says neither variable belongs there). It is an internal test comment, not an operator-facing instruction. - `resolveApiKey()` trims only the ends of the file content, so a file with more than one non-empty line yields a token containing an interior newline; the request then fails as an invalid header rather than naming the file. Outside the order's words, and it cannot expose the value. - The two tests that read the real `~/.config/homelab/*.env` files pass trivially on a machine where those files do not name the credential variables; the fixture tests beside them are the ones that pin the behaviour. Exposure: git objects of Homelab/pi-extensions at aa6d0097..6fb394ce433a589cba395fa7e55da7c529ad238e (fetch, diff, log, and file contents at the head), plus that head's `forgejo-ci` tree extracted to a temp directory to run its own vitest and tsc; the branch-protection read and this verdict were made with the staged ld-reviewer credential; I read no PR comments, no landing census, and no Plane ticket, and never touched FORGEJO_API_KEY; my only write is this one verdict posted as ld-reviewer.

ld-reviewer commented 2026-11-07 05:06:34 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 6fb394ce43: APPROVE

model: omniroute/deepseek-flash
session: 01a1b463-a478-702c-a118-436bdb529972

Blockers

None.

Fixed text (standing rule)

"A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER : line."

What this seat read

Local git objects of Homelab/pi-extensions only: a fetch, the range aa6d0097..6fb394ce (the branch's own work; aa6d0097 is its merge-base with main), the four commit messages, and file contents at the head — forgejo-ci/forgejo.ts, forgejo-ci/types.ts, forgejo-ci/tests/api-key-file.test.ts, forgejo-ci/tests/env-file-key.test.ts, forgejo-ci/tests/fake.ts, forgejo-ci/README.md, forgejo-ci/forgejo-ci.env.example, docs/forgejo-ci.md, plus forgejo/api.ts at the head for the sibling comparison the order names.

Order item 1 — FORGEJO_API_KEY_FILE read first

• Precedence: resolveApiKey (forgejo.ts:175-199) trims the pointer ((env[API_KEY_FILE_ENV] ?? "").trim()), reads the file, and only reaches env[API_KEY_ENV] when the pointer is unset/blank or the file yields no token. File-before-env, as in forgejo/api.ts.
• Token trimming: (text ?? "").trim() — a trailing newline, a CRLF, and surrounding spaces all leave the same token; the path is trimmed too, so a trailing space in the pointer is harmless.
• Unreadable file REFUSES and does not fall back: the catch builds a failure string and returns { apiKey: undefined, source: "none", error } — the function returns before the fromEnv read, so the environment is not consulted on that path. The message names the pointer variable, the path, and the first line of the errno detail only. A directory named by the pointer raises EISDIR from readFileSync and is caught by the same catch; a missing file is ENOENT, likewise caught.
• Empty (but readable) file falls back to the environment: if (token) is false, control falls through to fromEnv. A file holding only a newline behaves the same.
• Both variable names in the no-token error: Api.getPage throws `no CI read token: point ${API_KEY_FILE_ENV} at a readable 0600 file, or set ${API_KEY_ENV} …`.
• Refusal before any request: the apiKeyError throw and the missing-key throw are the first two statements of Api.getPage, ahead of URLSearchParams, the URL string, and this.fetchImpl. paged, tasks, runs, runByNumber and commitStatuses all reach the network only through getPage; I found no other fetchImpl call site inside Api.
• The divergence from forgejo/api.ts — which falls back to the environment on an unreadable file (api.ts:78-118) — is the divergence the order's own wording requires, and the code comment and README both state it.

Order item 2 — loadEnvFiles() no longer re-injects the credential

• ENV_FILE_NEVER_INJECT = new Set(["FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"]), and loadEnvFiles applies if (ENV_FILE_NEVER_INJECT.has(key)) continue; immediately after key extraction — before the "existing process env wins" check and before any assignment, so neither variable from an env file can reach process.env.
• Every other key still loads: FORGEJO_BASE_URL, FORGEJO_SESSION_COOKIE, FORGEJO_WEB_USERNAME and FORGEJO_WEB_PASSWORD pass the same loop unchanged (the skip is a two-name set, not a stop-reading-the-files change), and the code comment states why those two web-session values stay eligible.
• Tests pin both skips with value-safe assertions: tests/env-file-key.test.ts "an env file may not name the key FILE either" asserts process.env[FILE_VAR] === undefined, resolveApiKey(process.env).source === "none", and resolved.apiKey === FILE_TOKEN is false (boolean form) for a file the test has itself filled with a token; the sibling block asserts the same for the value variable, and "skips BOTH credential variables while every other key still loads" asserts the three non-credential keys do load.
• The sanctioned route survives: "a FILE pointer the CALLER exported still configures the route" exports FORGEJO_API_KEY_FILE itself, calls loadConfig(), and gets source: "key-file" — the pointer is denied only as an env-file source; "a genuinely exported key is still the fallback" pins the exported-value path.

Order item 3 — Docs

• forgejo-ci/README.md: the auth table now reads FORGEJO_API_KEY_FILE, else FORGEJO_API_KEY; the Configure section states "The API credential is not an env-file setting: export it in the caller's own shell/environment", shows the two exports (FORGEJO_API_KEY_FILE=/path/to/token first, the value as fallback), and states that both are skipped by loadEnvFiles() so an entry in forgejo.env or forgejo-ci.env is inert.
• forgejo-ci/forgejo-ci.env.example: both credential lines are gone from the block; the header says both must be EXPORTED, that loadEnvFiles() skips both, and that a value written in the file for either is inert; the commented placeholder now reads "Neither credential variable belongs here".
• docs/forgejo-ci.md: the variable table says "Export it, or export FORGEJO_API_KEY_FILE naming a 0600 file", and a new paragraph says the env-file read "is for non-credential settings only" with both names skipped.
• None of the three instructs an operator to write either variable into a ~/.config/homelab/*.env file; all three name the caller's environment as where the route is chosen.

Order item 4 — behaviour tests, no value exposure

• The legs assert observable behaviour, not implementation: which source wins, what source reports, what a broken pointer does to process.env-backed resolution, and expect(calls).toHaveLength(0) — the injected fetchImpl is never invoked behind a broken pointer (two separate tests, in both files).
• No credential value can be printed: the fixture constants are short synthetic markers (20/20/16 and 21/20 characters, dash-bearing, not hex- or token-shaped) written by the test itself into a mkdtemp 0700 directory as a 0600 file; tests/env-file-key.test.ts compares with booleans throughout, and the value comparisons in tests/api-key-file.test.ts use only those synthetic constants. No test reads a real credential.
• No token reaches argv or a URL: tests/fake.ts records pathname + search per call and the credential travels in the Authorization header the module has always used; a grep of the whole forgejo-ci/ tree at this head for token=, access_token, api_key= and private_token returns nothing. A tripwire test asserts forgejo.ts contains none of child_process, execFile, execSync, spawnSync, Bun.spawn.

Test evidence I produced

I exported the head tree to a throwaway directory under /tmp (no write to the repository) and ran vitest run tests/api-key-file.test.ts tests/env-file-key.test.ts there: 30 tests passed, 2 files passed. The three remaining test files could not import typebox in that throwaway copy — that package is absent from this checkout's node_modules, so those three do not run here at all; it is a property of my staging, not of the diff.

Observation, not a blocker

Api.runByNumber wraps its call in try { … } catch { return null }, so the credential refusal is swallowed there and the lookup falls through to api.runs(...), which throws it; the refusal is therefore still reported, one call later. Worth knowing, nothing hidden.

Branch rule

I could not read GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main as ld-reviewer: HTTP 403, "user should be an owner or a collaborator with admin write of a repository" — the same refusal a prior seat recorded, and pi-extensions is documented as having no branch protection. So this verdict is posted on the comment route and is not a formal review.

Exposure: git objects of Homelab/pi-extensions at aa6d0097..6fb394ce only, plus file contents at that head, the single canonical posting template, and a throwaway /tmp export of the head tree for the two test files; no PR comments, no landing census, no Plane ticket, no other working-files path, no images; my only write is this one verdict posted as ld-reviewer.

Verdict of record — ld-reviewer — head 6fb394ce433a589cba395fa7e55da7c529ad238e: APPROVE model: omniroute/deepseek-flash session: 01a1b463-a478-702c-a118-436bdb529972 ## Blockers None. ### Fixed text (standing rule) "A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER <id>: line." ### What this seat read Local git objects of `Homelab/pi-extensions` only: a fetch, the range `aa6d0097..6fb394ce` (the branch's own work; `aa6d0097` is its merge-base with `main`), the four commit messages, and file contents at the head — `forgejo-ci/forgejo.ts`, `forgejo-ci/types.ts`, `forgejo-ci/tests/api-key-file.test.ts`, `forgejo-ci/tests/env-file-key.test.ts`, `forgejo-ci/tests/fake.ts`, `forgejo-ci/README.md`, `forgejo-ci/forgejo-ci.env.example`, `docs/forgejo-ci.md`, plus `forgejo/api.ts` at the head for the sibling comparison the order names. ### Order item 1 — `FORGEJO_API_KEY_FILE` read first - Precedence: `resolveApiKey` (forgejo.ts:175-199) trims the pointer (`(env[API_KEY_FILE_ENV] ?? "").trim()`), reads the file, and only reaches `env[API_KEY_ENV]` when the pointer is unset/blank or the file yields no token. File-before-env, as in `forgejo/api.ts`. - Token trimming: `(text ?? "").trim()` — a trailing newline, a CRLF, and surrounding spaces all leave the same token; the path is trimmed too, so a trailing space in the pointer is harmless. - Unreadable file REFUSES and does not fall back: the `catch` builds a `failure` string and returns `{ apiKey: undefined, source: "none", error }` — the function returns *before* the `fromEnv` read, so the environment is not consulted on that path. The message names the pointer variable, the path, and the first line of the errno detail only. A directory named by the pointer raises `EISDIR` from `readFileSync` and is caught by the same `catch`; a missing file is `ENOENT`, likewise caught. - Empty (but readable) file falls back to the environment: `if (token)` is false, control falls through to `fromEnv`. A file holding only a newline behaves the same. - Both variable names in the no-token error: `Api.getPage` throws `` `no CI read token: point ${API_KEY_FILE_ENV} at a readable 0600 file, or set ${API_KEY_ENV} …` ``. - Refusal before any request: the `apiKeyError` throw and the missing-key throw are the first two statements of `Api.getPage`, ahead of `URLSearchParams`, the URL string, and `this.fetchImpl`. `paged`, `tasks`, `runs`, `runByNumber` and `commitStatuses` all reach the network only through `getPage`; I found no other `fetchImpl` call site inside `Api`. - The divergence from `forgejo/api.ts` — which falls back to the environment on an unreadable file (api.ts:78-118) — is the divergence the order's own wording requires, and the code comment and README both state it. ### Order item 2 — `loadEnvFiles()` no longer re-injects the credential - `ENV_FILE_NEVER_INJECT = new Set(["FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"])`, and `loadEnvFiles` applies `if (ENV_FILE_NEVER_INJECT.has(key)) continue;` immediately after key extraction — before the "existing process env wins" check and before any assignment, so neither variable from an env file can reach `process.env`. - Every other key still loads: `FORGEJO_BASE_URL`, `FORGEJO_SESSION_COOKIE`, `FORGEJO_WEB_USERNAME` and `FORGEJO_WEB_PASSWORD` pass the same loop unchanged (the skip is a two-name set, not a stop-reading-the-files change), and the code comment states why those two web-session values stay eligible. - Tests pin both skips with value-safe assertions: `tests/env-file-key.test.ts` "an env file may not name the key FILE either" asserts `process.env[FILE_VAR] === undefined`, `resolveApiKey(process.env).source === "none"`, and `resolved.apiKey === FILE_TOKEN` is `false` (boolean form) for a file the test has itself filled with a token; the sibling block asserts the same for the value variable, and "skips BOTH credential variables while every other key still loads" asserts the three non-credential keys *do* load. - The sanctioned route survives: "a FILE pointer the CALLER exported still configures the route" exports `FORGEJO_API_KEY_FILE` itself, calls `loadConfig()`, and gets `source: "key-file"` — the pointer is denied only as an env-file source; "a genuinely exported key is still the fallback" pins the exported-value path. ### Order item 3 — Docs - `forgejo-ci/README.md`: the auth table now reads `FORGEJO_API_KEY_FILE`, else `FORGEJO_API_KEY`; the Configure section states "The API credential is **not** an env-file setting: export it in the caller's own shell/environment", shows the two exports (`FORGEJO_API_KEY_FILE=/path/to/token` first, the value as fallback), and states that both are skipped by `loadEnvFiles()` so an entry in `forgejo.env` or `forgejo-ci.env` is inert. - `forgejo-ci/forgejo-ci.env.example`: both credential lines are gone from the block; the header says both must be EXPORTED, that `loadEnvFiles()` skips both, and that a value written in the file for either is inert; the commented placeholder now reads "Neither credential variable belongs here". - `docs/forgejo-ci.md`: the variable table says "Export it, or export `FORGEJO_API_KEY_FILE` naming a 0600 file", and a new paragraph says the env-file read "is for non-credential settings only" with both names skipped. - None of the three instructs an operator to write either variable into a `~/.config/homelab/*.env` file; all three name the caller's environment as where the route is chosen. ### Order item 4 — behaviour tests, no value exposure - The legs assert observable behaviour, not implementation: which source wins, what `source` reports, what a broken pointer does to `process.env`-backed resolution, and `expect(calls).toHaveLength(0)` — the injected `fetchImpl` is never invoked behind a broken pointer (two separate tests, in both files). - No credential value can be printed: the fixture constants are short synthetic markers (20/20/16 and 21/20 characters, dash-bearing, not hex- or token-shaped) written by the test itself into a `mkdtemp` 0700 directory as a 0600 file; `tests/env-file-key.test.ts` compares with booleans throughout, and the value comparisons in `tests/api-key-file.test.ts` use only those synthetic constants. No test reads a real credential. - No token reaches argv or a URL: `tests/fake.ts` records `pathname + search` per call and the credential travels in the `Authorization` header the module has always used; a grep of the whole `forgejo-ci/` tree at this head for `token=`, `access_token`, `api_key=` and `private_token` returns nothing. A tripwire test asserts `forgejo.ts` contains none of `child_process`, `execFile`, `execSync`, `spawnSync`, `Bun.spawn`. ### Test evidence I produced I exported the head tree to a throwaway directory under `/tmp` (no write to the repository) and ran `vitest run tests/api-key-file.test.ts tests/env-file-key.test.ts` there: 30 tests passed, 2 files passed. The three remaining test files could not import `typebox` in that throwaway copy — that package is absent from this checkout's `node_modules`, so those three do not run here at all; it is a property of my staging, not of the diff. ### Observation, not a blocker `Api.runByNumber` wraps its call in `try { … } catch { return null }`, so the credential refusal is swallowed there and the lookup falls through to `api.runs(...)`, which throws it; the refusal is therefore still reported, one call later. Worth knowing, nothing hidden. ### Branch rule I could not read `GET /api/v1/repos/Homelab/pi-extensions/branch_protections/main` as `ld-reviewer`: HTTP 403, "user should be an owner or a collaborator with admin write of a repository" — the same refusal a prior seat recorded, and `pi-extensions` is documented as having no branch protection. So this verdict is posted on the comment route and is not a formal review. Exposure: git objects of Homelab/pi-extensions at aa6d0097..6fb394ce only, plus file contents at that head, the single canonical posting template, and a throwaway /tmp export of the head tree for the two test files; no PR comments, no landing census, no Plane ticket, no other working-files path, no images; my only write is this one verdict posted as ld-reviewer.

ld-reviewer commented 2026-11-07 05:16:41 +00:00

Collaborator

Copy link

Closure of record — ld-reviewer — HOMELAB-2682 PR #117 — head 6fb394ce43
model: omniroute/deepseek-flash
session: 01a1b469-73a2-702c-a118-436e6cae7995

Closure seat. No verdict is posted here, nothing was edited, committed or pushed. Both findings that the landing lock still reads as open on this head were re-verified by this seat against the code and tests at 6fb394ce, and each is closed below with its own line naming the comment that raised it.

Finding raised in comment #23094 — verdict of record at head d881e47e, REQUEST_CHANGES, finding labelled B1

Claimed: loadEnvFiles() was byte-identical to the base and still re-injected FORGEJO_API_KEY from ~/.config/homelab/forgejo.env into the process environment, so a keyless session got the key back and the file route could not be proven at all.

Verified by this seat at head 6fb394ce (local git objects and an independent probe, booleans and lengths only):

• The raising head had no skip list at all — loadEnvFiles() carried only the "existing process env wins" rule. At 6fb394ce it skips every key in ENV_FILE_NEVER_INJECT (forgejo-ci/forgejo.ts:100 = {"FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"}, the skip at :121, the single process.env write at :127), so a value named in an env file never reaches the environment.
• Probe with both variables unset and a staged env file naming FORGEJO_API_KEY: the key is absent after loadEnvFiles(), resolveApiKey() answers source: "none" with no error, loadConfig().apiKey is empty and apiKeyError unset.
• The REAL ~/.config/homelab/forgejo.env (which does name FORGEJO_API_KEY; this seat read key NAMES only, never values) with the default file list: the key is still absent and the source is none.
• The pin is real, not incidental: emptying the deny set in a throwaway copy of this head turns 7 of the 11 tests in tests/env-file-key.test.ts red.

resolved #23094
resolved B1 — the finding raised in comment #23094 is closed: verified fixed at head 6fb394ce43

Finding raised in comment #23120 — verdict of record at head a502276e, REQUEST_CHANGES, finding labelled B1

Claimed: ENV_FILE_NEVER_INJECT held FORGEJO_API_KEY only, so FORGEJO_API_KEY_FILE was still injected from the env files and a session with neither variable exported still acquired a working token (source: "key-file", no error), carried by the pointer instead of the value; no test pinned it.

Verified by this seat at head 6fb394ce:

• At the raising head the set was ["FORGEJO_API_KEY"] (a502276e:forgejo-ci/forgejo.ts:86); at 6fb394ce it is ["FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"].
• Probe with a staged env file naming a readable 0600 file that holds a token: the pointer is absent after loadEnvFiles(), and resolveApiKey() answers source: "none", apiKey undefined, no error. Naming both credential variables in one file denies both while the rest of that file still loads (FORGEJO_BASE_URL applies).
• The sanctioned route is intact, not deleted: a pointer the CALLER exported still resolves source: "key-file" with the token and no error, and an unreadable pointer still refuses, naming the path, with a key named in an env file not rescuing it.
• The pin is real: dropping FORGEJO_API_KEY_FILE back out of the deny set in a throwaway copy of this head turns 3 of the 11 tests in tests/env-file-key.test.ts red.

resolved #23120
resolved B1 — the finding raised in comment #23120 is closed: verified fixed at head 6fb394ce43

Other measurements at this head

• vitest run in forgejo-ci at 6fb394ce: 82 passed, 11 skipped (5 files pass; live.test.ts self-skips). tsc --noEmit: clean.
• Both raising comments key their finding B1, so the lock's id-ed closure arm cannot tell one from the other by label alone. That is why each finding above carries its own line naming the comment that raised it.
• Not a finding, stated because it is what this seat measured and did not cure: assessFindingGrammar at this head reports one live malformed-blockers problem on comment #23118 (its ## Blockers section carries the standing fixed text, which the lock reads as template vocabulary). This closure does not cure it — its lawful exit is a later exact-head verdict from the same reviewer, which only a reviewer posts — and the reason its two later exact-head verdicts do not supersede it is that #23118 declares model: deepseek-flash while #23132/#23133 declare model: omniroute/deepseek-flash, so the lock's own-later-word test does not match them.

A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER : line.

Exposure — what this seat read and ran, and nothing else: the two raising comments (#23094, #23120) and PR #117's full comment set through the staged ld-reviewer credential (the comment set is what the dry-run needs); local git objects of Homelab/pi-extensions at 6fb394ce, d881e47e and a502276e (git show, git log), plus a throwaway copy of this head under /tmp for the two mutation checks; key NAMES of ~/.config/homelab/forgejo.env and forgejo-ci.env (values never printed); the deployed lock compiled from ~/.pi/agent/extensions/forgejo/landing.ts for the dry-run; vitest run and tsc --noEmit in the head worktree. No token was printed, put in argv, or placed in a URL. This seat edited no file, made no commit and no push, and its only write is this one closure comment posted as ld-reviewer.

Closure of record — ld-reviewer — HOMELAB-2682 PR #117 — head 6fb394ce433a589cba395fa7e55da7c529ad238e model: omniroute/deepseek-flash session: 01a1b469-73a2-702c-a118-436e6cae7995 Closure seat. No verdict is posted here, nothing was edited, committed or pushed. Both findings that the landing lock still reads as open on this head were re-verified by this seat against the code and tests at 6fb394ce, and each is closed below with its own line naming the comment that raised it. ### Finding raised in comment #23094 — verdict of record at head d881e47e, REQUEST_CHANGES, finding labelled B1 Claimed: `loadEnvFiles()` was byte-identical to the base and still re-injected `FORGEJO_API_KEY` from `~/.config/homelab/forgejo.env` into the process environment, so a keyless session got the key back and the file route could not be proven at all. Verified by this seat at head 6fb394ce (local git objects and an independent probe, booleans and lengths only): - The raising head had no skip list at all — `loadEnvFiles()` carried only the "existing process env wins" rule. At 6fb394ce it skips every key in `ENV_FILE_NEVER_INJECT` (`forgejo-ci/forgejo.ts:100` = `{"FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"}`, the skip at `:121`, the single `process.env` write at `:127`), so a value named in an env file never reaches the environment. - Probe with both variables unset and a staged env file naming `FORGEJO_API_KEY`: the key is absent after `loadEnvFiles()`, `resolveApiKey()` answers `source: "none"` with no error, `loadConfig().apiKey` is empty and `apiKeyError` unset. - The REAL `~/.config/homelab/forgejo.env` (which does name `FORGEJO_API_KEY`; this seat read key NAMES only, never values) with the default file list: the key is still absent and the source is `none`. - The pin is real, not incidental: emptying the deny set in a throwaway copy of this head turns 7 of the 11 tests in `tests/env-file-key.test.ts` red. resolved #23094 resolved B1 — the finding raised in comment #23094 is closed: verified fixed at head 6fb394ce433a589cba395fa7e55da7c529ad238e ### Finding raised in comment #23120 — verdict of record at head a502276e, REQUEST_CHANGES, finding labelled B1 Claimed: `ENV_FILE_NEVER_INJECT` held `FORGEJO_API_KEY` only, so `FORGEJO_API_KEY_FILE` was still injected from the env files and a session with neither variable exported still acquired a working token (`source: "key-file"`, no error), carried by the pointer instead of the value; no test pinned it. Verified by this seat at head 6fb394ce: - At the raising head the set was `["FORGEJO_API_KEY"]` (`a502276e:forgejo-ci/forgejo.ts:86`); at 6fb394ce it is `["FORGEJO_API_KEY", "FORGEJO_API_KEY_FILE"]`. - Probe with a staged env file naming a readable 0600 file that holds a token: the pointer is absent after `loadEnvFiles()`, and `resolveApiKey()` answers `source: "none"`, `apiKey` undefined, no error. Naming both credential variables in one file denies both while the rest of that file still loads (`FORGEJO_BASE_URL` applies). - The sanctioned route is intact, not deleted: a pointer the CALLER exported still resolves `source: "key-file"` with the token and no error, and an unreadable pointer still refuses, naming the path, with a key named in an env file not rescuing it. - The pin is real: dropping `FORGEJO_API_KEY_FILE` back out of the deny set in a throwaway copy of this head turns 3 of the 11 tests in `tests/env-file-key.test.ts` red. resolved #23120 resolved B1 — the finding raised in comment #23120 is closed: verified fixed at head 6fb394ce433a589cba395fa7e55da7c529ad238e ### Other measurements at this head - `vitest run` in `forgejo-ci` at 6fb394ce: 82 passed, 11 skipped (5 files pass; `live.test.ts` self-skips). `tsc --noEmit`: clean. - Both raising comments key their finding `B1`, so the lock's id-ed closure arm cannot tell one from the other by label alone. That is why each finding above carries its own line naming the comment that raised it. - Not a finding, stated because it is what this seat measured and did not cure: `assessFindingGrammar` at this head reports one live `malformed-blockers` problem on comment #23118 (its `## Blockers` section carries the standing fixed text, which the lock reads as template vocabulary). This closure does not cure it — its lawful exit is a later exact-head verdict from the same reviewer, which only a reviewer posts — and the reason its two later exact-head verdicts do not supersede it is that #23118 declares `model: deepseek-flash` while #23132/#23133 declare `model: omniroute/deepseek-flash`, so the lock's own-later-word test does not match them. A problem only blocks if it is a REQUEST_CHANGES verdict, a line under ## Blockers, or an exact BLOCKER <id>: line. Exposure — what this seat read and ran, and nothing else: the two raising comments (#23094, #23120) and PR #117's full comment set through the staged ld-reviewer credential (the comment set is what the dry-run needs); local git objects of Homelab/pi-extensions at 6fb394ce, d881e47e and a502276e (`git show`, `git log`), plus a throwaway copy of this head under /tmp for the two mutation checks; key NAMES of `~/.config/homelab/forgejo.env` and `forgejo-ci.env` (values never printed); the deployed lock compiled from `~/.pi/agent/extensions/forgejo/landing.ts` for the dry-run; `vitest run` and `tsc --noEmit` in the head worktree. No token was printed, put in argv, or placed in a URL. This seat edited no file, made no commit and no push, and its only write is this one closure comment posted as ld-reviewer.

ld-reviewer commented 2026-11-07 05:28:32 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 6fb394ce43: APPROVE

model: deepseek-flash
session: 01a1b478-2e8c-702c-a118-4379d67a4732

Blockers

None.

Exposure

git objects of Homelab/pi-extensions at aa6d0097..6fb394ce; the comment set read for the dry-run via the staged ld-reviewer route; my only write is this verdict posted as ld-reviewer.

Verdict of record — ld-reviewer — head 6fb394ce433a589cba395fa7e55da7c529ad238e: APPROVE model: deepseek-flash session: 01a1b478-2e8c-702c-a118-4379d67a4732 ## Blockers None. ### Exposure git objects of Homelab/pi-extensions at aa6d0097..6fb394ce; the comment set read for the dry-run via the staged ld-reviewer route; my only write is this verdict posted as ld-reviewer.

pi-landing merged commit 2acbdbd8da into main 2026-11-07 05:33:57 +00:00

pi-landing referenced this pull request from a commit 2026-11-07 05:33:59 +00:00

Merge pull request 'HOMELAB-2682: feat(forgejo-ci): read FORGEJO_API_KEY_FILE first (scoped)' (#117) from plane/HOMELAB-2682-ci-key-file into main

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

3 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!117

No description provided.