HOMELAB-1534: fix(reviewer): authenticate all git calls in the review workdir (private repos) + claim-verification verdict policy #504

Open

aaron wants to merge 2 commits from plane/HOMELAB-1534-reviewer-fetch-auth into live

pull from: plane/HOMELAB-1534-reviewer-fetch-auth

merge into: Homelab:live

Homelab:main

Homelab:live

Homelab:plane/HOMELAB-1535-outline-prestop

Homelab:plane/HOMELAB-1535-outline-ha

Homelab:plane/HOMELAB-1518-longhorn-rule-fix

Homelab:plane/HOMELAB-1527-tempo-pipeline

Homelab:plane/HOMELAB-1526-mqttx-web

Homelab:plane/HOMELAB-1511-prowlarr-qbit

Conversation 2 Commits 2 Files changed 2 +41 -13

aaron commented 2026-09-11 02:39:09 +00:00

Owner

Copy link

What

Two reviewer fixes from the HOMELAB-1526 demo post-mortem (HOMELAB-1534):

1. Private repos were never reviewable. http.extraHeader auth is per-invocation, so in the review workdir:

   • the git fetch for the base ref ran unauthenticated, and
   • because the clone is blobless (--filter=blob:none), the lazy blob fetch during git diff did too.

   Public repos (infra-core) masked both; every Homelab/prod PR failed with could not read Username (36/36 in 3 days). Now authEnv(token) is passed to clone, fetch and diff.

2. Claim-verification verdict policy in the review prompt: CHANGES-REQUESTED whenever a claimed outcome is unverifiable from the diff (runtime/deploy claims like "deployed", "pod Ready", "verified E2E", "reachable at URL") or an acceptance criterion is marked ✅ without a citable artifact. This is the HOMELAB-1526 canary class (fabricated E2E claims were approved as "nits").

Verification

• Isolated live test of the patched code against the private repo (Homelab/prod, PR #541's branch): clone + fetch + diff = 46 files / 84KB diff, 3/3 runs OK.
• Receiver restarted with the patch (to be confirmed on this PR's review — this repo is public, so use the canary PR on prod for the private path).

Related: HOMELAB-1526 (canary origin), HOMELAB-1527.

## What Two reviewer fixes from the HOMELAB-1526 demo post-mortem (HOMELAB-1534): 1. **Private repos were never reviewable.** `http.extraHeader` auth is per-invocation, so in the review workdir: - the `git fetch` for the base ref ran unauthenticated, and - because the clone is blobless (`--filter=blob:none`), the **lazy blob fetch during `git diff`** did too. Public repos (infra-core) masked both; every Homelab/prod PR failed with `could not read Username` (36/36 in 3 days). Now `authEnv(token)` is passed to clone, fetch **and** diff. 2. **Claim-verification verdict policy** in the review prompt: `CHANGES-REQUESTED` whenever a claimed outcome is unverifiable from the diff (runtime/deploy claims like "deployed", "pod Ready", "verified E2E", "reachable at URL") or an acceptance criterion is marked ✅ without a citable artifact. This is the HOMELAB-1526 canary class (fabricated E2E claims were approved as "nits"). ## Verification - Isolated live test of the patched code against the private repo (`Homelab/prod`, PR #541's branch): clone + fetch + diff = **46 files / 84KB diff, 3/3 runs OK**. - Receiver restarted with the patch (to be confirmed on this PR's review — this repo is public, so use the canary PR on prod for the private path). Related: HOMELAB-1526 (canary origin), HOMELAB-1527.

aaron added 1 commit 2026-09-11 02:39:09 +00:00

HOMELAB-1534: fix(reviewer): authenticate all git calls in the review workdir ... 55becf875f

Private repos were never reviewable: http.extraHeader auth is per-invocation,
so the fetch and — because the clone is blobless (--filter=blob:none) — the
lazy blob fetch during 'git diff' ran unauthenticated. Public repos masked
this for months; Homelab/prod PRs all failed with 'could not read Username'.

- extract authEnv(token); pass it to clone, fetch AND diff
- prompt: CHANGES-REQUESTED for unverifiable runtime/deploy claims
  (replay of the HOMELAB-1526 fabricated-E2E canary class)

Verified: clone+fetch+diff on Homelab/prod feat/jitsi-zitadel-jwt (46 files,
84KB diff), 3/3 runs.

homelab-bot approved these changes 2026-09-11 02:40:59 +00:00

homelab-bot left a comment

Collaborator

Copy link

AI Review (HOMELAB-1534)

Verdict: PASS-WITH-NITS

The code change correctly implements the diagnosed root cause (per-invocation http.extraHeader + blobless-clone lazy fetches), and the verdict-policy prompt addition matches the ticket scope item. Nits are below; the ticket's runtime verification items remain outstanding and are tracked as ⚠️, not ✅ — merging this PR does not close HOMELAB-1534.

Acceptance Criteria (HOMELAB-1534)

Criterion	Met?	Evidence
Reviewer: fix prod clone-auth failures	⚠️	Mechanism is correct by inspection: authEnv() (review.js:46-55) now applied to clone (review.js:64), base fetch (review.js:81), and git diff lazy blob fetch (review.js:82). But "prod clone works" is a runtime outcome — no canary replay or prod-PR review log cited in the diff.
Reviewer: block unverifiable "deployed E2E" claims (severity mapping, not nits)	✅	prompt.js:32-34 — explicit policy: CHANGES-REQUESTED for unverifiable runtime/deploy claims and ✅-without-artifact criteria; PASS-WITH-NITS restricted to cosmetic remarks. Directly targets the #501 failure mode.
Reviewer: canary replay	❌	Not in diff. No test, fixture, or replay script for the receiver in this PR.
Verify: preflight GO (reviewer green on infra-core AND prod)	⚠️	Runtime/deploy claim — unverifiable from diff, no citable artifact. Gate ticket closure (not necessarily this merge) on it.
Verify: reviewer blocks a #501-style fabricated-E2E PR	⚠️	Policy text exists, but the blocking behavior is itself a runtime claim requiring a decoy-PR demonstration. Unverified here.
Remaining scope items (AGENTS.md playbook, Plane 429 retry, Forgejo list_ci_runs, swarm-suite contract, project skill, rehearsal/export)	❌	Out of scope for this PR; ticket correctly remains In Progress.

Design Doc Citations

• pi-review-receiver operations runbook (Outline, §Architecture / §Credentials and rotation) — confirms the worker's clone→fetch→diff pipeline this PR patches, and that the clone token is read:repository on claude-agent. Note the runbook names the account claude-agent while authEnv defaults FORGEJO_CLONE_USER to "ci-bot" — pre-existing, but the env var must be set in secrets.env for the default not to silently produce wrong Basic-auth credentials on private repos.
• HOMELAB-1534 ticket (Scope/Verify) — this PR covers exactly one scope bullet (the reviewer one); the "no-cheating rule" (task-agnostic changes only) is satisfied — both edits are generic.

Open Issues

• [review.js:81-82] Token-masking inconsistency: cloneShallow scrubs the token from error messages (review.js:66-68) but fetchDiff does not. Git stderr rarely echoes http.extraHeader values, so this is low-risk defense-in-depth, but the two paths should behave identically — wrap fetch/diff errors with the same split(token).join("***") scrub.
• [review.js:49] authEnv returns bare process.env when token is empty — fetchDiff then silently degrades to unauthenticated fetch/diff (the pre-fix behavior). For private repos this fails late with "could not read Username" instead of a clear config error. Consider failing fast when FORGEJO_CLONE_TOKEN/FORGEJO_COMMENT_TOKEN resolve empty.
• [review.js:78] buildCuratedContext runs after fetchDiff in the same workdir; if it ever shells out to git (e.g., git log, reading non-checked-out blobs from the blobless clone), it would hit the same unauthenticated lazy-fetch path. Not visible in this diff — confirm it only reads the materialized working tree, or thread authEnv through.
• [prompt.js:32] The policy is self-applicable: this PR's ticket-level runtime claims (preflight GO, decoy-PR blocking) have no citable artifacts. Recommend the merge comment explicitly state which Verify items remain open and that HOMELAB-1534 stays In Progress until the prod canary replay + #501-style decoy test are evidenced.

Powered by Kimi k3 · advisory.

## AI Review (HOMELAB-1534) **Verdict**: PASS-WITH-NITS The code change correctly implements the diagnosed root cause (per-invocation `http.extraHeader` + blobless-clone lazy fetches), and the verdict-policy prompt addition matches the ticket scope item. Nits are below; the ticket's runtime verification items remain outstanding and are tracked as ⚠️, not ✅ — merging this PR does not close HOMELAB-1534. ### Acceptance Criteria (HOMELAB-1534) | Criterion | Met? | Evidence | |---|---|---| | Reviewer: fix prod clone-auth failures | ⚠️ | Mechanism is correct by inspection: `authEnv()` (review.js:46-55) now applied to clone (review.js:64), base fetch (review.js:81), and `git diff` lazy blob fetch (review.js:82). But "prod clone works" is a runtime outcome — no canary replay or prod-PR review log cited in the diff. | | Reviewer: block unverifiable "deployed E2E" claims (severity mapping, not nits) | ✅ | prompt.js:32-34 — explicit policy: CHANGES-REQUESTED for unverifiable runtime/deploy claims and ✅-without-artifact criteria; PASS-WITH-NITS restricted to cosmetic remarks. Directly targets the #501 failure mode. | | Reviewer: canary replay | ❌ | Not in diff. No test, fixture, or replay script for the receiver in this PR. | | Verify: preflight GO (reviewer green on infra-core AND prod) | ⚠️ | Runtime/deploy claim — unverifiable from diff, no citable artifact. Gate ticket closure (not necessarily this merge) on it. | | Verify: reviewer blocks a #501-style fabricated-E2E PR | ⚠️ | Policy text exists, but the blocking behavior is itself a runtime claim requiring a decoy-PR demonstration. Unverified here. | | Remaining scope items (AGENTS.md playbook, Plane 429 retry, Forgejo `list_ci_runs`, swarm-suite contract, project skill, rehearsal/export) | ❌ | Out of scope for this PR; ticket correctly remains In Progress. | ### Design Doc Citations - **pi-review-receiver operations runbook** (Outline, §Architecture / §Credentials and rotation) — confirms the worker's clone→fetch→diff pipeline this PR patches, and that the clone token is `read:repository` on `claude-agent`. Note the runbook names the account `claude-agent` while `authEnv` defaults `FORGEJO_CLONE_USER` to `"ci-bot"` — pre-existing, but the env var must be set in `secrets.env` for the default not to silently produce wrong Basic-auth credentials on private repos. - **HOMELAB-1534 ticket** (Scope/Verify) — this PR covers exactly one scope bullet (the reviewer one); the "no-cheating rule" (task-agnostic changes only) is satisfied — both edits are generic. ### Open Issues - **[review.js:81-82]** Token-masking inconsistency: `cloneShallow` scrubs the token from error messages (review.js:66-68) but `fetchDiff` does not. Git stderr rarely echoes `http.extraHeader` values, so this is low-risk defense-in-depth, but the two paths should behave identically — wrap fetch/diff errors with the same `split(token).join("***")` scrub. - **[review.js:49]** `authEnv` returns bare `process.env` when `token` is empty — `fetchDiff` then silently degrades to unauthenticated fetch/diff (the pre-fix behavior). For private repos this fails late with "could not read Username" instead of a clear config error. Consider failing fast when `FORGEJO_CLONE_TOKEN`/`FORGEJO_COMMENT_TOKEN` resolve empty. - **[review.js:78]** `buildCuratedContext` runs after `fetchDiff` in the same workdir; if it ever shells out to git (e.g., `git log`, reading non-checked-out blobs from the blobless clone), it would hit the same unauthenticated lazy-fetch path. Not visible in this diff — confirm it only reads the materialized working tree, or thread `authEnv` through. - **[prompt.js:32]** The policy is self-applicable: this PR's ticket-level runtime claims (preflight GO, decoy-PR blocking) have no citable artifacts. Recommend the merge comment explicitly state which Verify items remain open and that HOMELAB-1534 stays In Progress until the prod canary replay + #501-style decoy test are evidenced. _Powered by Kimi k3 · advisory._

forgejo-admin referenced this pull request from a commit 2026-09-11 02:43:05 +00:00

HOMELAB-1534: fix(reviewer): scrub tokens in fetch/diff errors + warn on missing clone token

forgejo-admin added 1 commit 2026-09-11 02:43:05 +00:00

HOMELAB-1534: fix(reviewer): scrub tokens in fetch/diff errors + warn on missing clone token ... 28e894c7b9

Addresses review nits on #504: token-masking parity across clone/fetch/diff
paths (scrubToken helper), and a loud clone_token_missing warning when only
public repos are reviewable. Verified context.js does not shell out to git.

aaron commented 2026-09-11 02:43:14 +00:00

Author

Owner

Copy link

Review nits addressed in 28e894c:

• Token-masking parity — new scrubToken() helper applied to both cloneShallow and fetchDiff error paths (fetch/diff previously masked nothing).
• Empty-token warning — runReview now logs clone_token_missing loudly when neither FORGEJO_CLONE_TOKEN nor FORGEJO_COMMENT_TOKEN resolves; we kept it a warning (not fail-fast) so public-only deployments keep working.
• context.js audit — verified it does not shell out to git (grep for spawn/exec/git: empty), so the blobless lazy-fetch exposure is confined to fetchDiff, which now carries authEnv.

Re: runbook account naming — FORGEJO_CLONE_USER is set explicitly (homelab-bot) in the receiver's secrets.env, so the ci-bot fallback is not in play; flagged to update the runbook when convenient.

Per your own recommendation: the ticket-level runtime verifications (preflight GO, decoy #501-style replay evidence) remain open on HOMELAB-1534 and will be evidenced there before the ticket moves to Done. The receiver has been restarted with this code and both commits are live (canary run earlier today: Homelab/prod#618 → CHANGES-REQUESTED in 104s).

Review nits addressed in `28e894c`: - **Token-masking parity** — new `scrubToken()` helper applied to both `cloneShallow` and `fetchDiff` error paths (fetch/diff previously masked nothing). - **Empty-token warning** — `runReview` now logs `clone_token_missing` loudly when neither `FORGEJO_CLONE_TOKEN` nor `FORGEJO_COMMENT_TOKEN` resolves; we kept it a warning (not fail-fast) so public-only deployments keep working. - **`context.js` audit** — verified it does not shell out to git (grep for spawn/exec/git: empty), so the blobless lazy-fetch exposure is confined to `fetchDiff`, which now carries `authEnv`. Re: runbook account naming — `FORGEJO_CLONE_USER` is set explicitly (`homelab-bot`) in the receiver's `secrets.env`, so the `ci-bot` fallback is not in play; flagged to update the runbook when convenient. Per your own recommendation: the ticket-level runtime verifications (preflight GO, decoy #501-style replay evidence) remain open on HOMELAB-1534 and will be evidenced there before the ticket moves to Done. The receiver has been restarted with this code and both commits are live (canary run earlier today: Homelab/prod#618 → CHANGES-REQUESTED in 104s).

All checks were successful

Hide all checks

CI Review / helm-validate (pull_request) Successful in 5s

Required

Details

CI Review / receiver-test (pull_request) Successful in 3s

Details

Lint & Validate / terraform-validate (pull_request) Successful in 1m44s

Required

Details

Lint & Validate / yaml-lint (pull_request) Successful in 8s

Required

Details

Lint & Validate / shellcheck (pull_request) Successful in 3s

Required

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin plane/HOMELAB-1534-reviewer-fetch-auth:plane/HOMELAB-1534-reviewer-fetch-auth

git switch plane/HOMELAB-1534-reviewer-fetch-auth

Sign in to join this conversation.

Reviewers

No reviewers

homelab-bot

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/infra-core!504

No description provided.