mosaicstack/stack
1
0
Fork 0
Code Issues 85 Pull Requests Actions Packages Projects Releases 15 Wiki Activity

fix(framework): pr-review.sh -r/--repo + -H/--host overrides + UA + repo preflight (Patches 5/5c) #896

Merged
Mos merged 2 commits from feat/pr-review-5-family into main 2026-07-25 22:08:09 +00:00
Conversation 2 Commits 2 Files Changed 3 +428 -12
jason.woltje commented 2026-07-25 21:19:20 +00:00
Owner
Copy Link

Summary

Upstreams the pr-review.sh patch family 5 + 5b + 5c + 5d from the host-local
tooling-patches kit into the framework, against a fresh clone of origin/main
(not the stale /src/mosaic-stack checkout).

Per the reproduce-first-or-halt discipline: two of the four named sub-changes
(5b, 5d) did not reproduce against baseline — the current framework
pr-review.sh already implements a more robust fix for each, landed
independently of the host-local kit. Those two are HALTed (not ported) to
avoid regressing already-shipped hardening. Only 5 and 5c genuinely
reproduced and are included.

Patch 5 — -r/--repo override + User-Agent: mosaic-pr-review header

Reproduced against baseline: neither flag existed —
./pr-review.sh -n 1 -a comment -c x -r foo/bar failed with
Unknown option: -r; grep -c User-Agent pr-review.sh was 0.

Change:

  • -r/--repo <owner/repo> — explicit slug override, skipping git-remote slug
    inference. Mirrors the already-established -r convention of the sibling
    wrappers (pr-view.sh, pr-diff.sh, pr-ci-wait.sh; #867), rather than
    reinventing a parallel scheme. detect_platform is now tolerant of a
    missing/foreign git origin when -r is given (assumes gitea), matching the
    same sibling-wrapper convention — otherwise -r couldn't achieve its stated
    purpose (reviewer worktrees with a nonstandard/missing origin).
  • User-Agent: mosaic-pr-review added to every Gitea curl call in the file
    (comment write, comment read-back, /user identity read, PR-head read,
    review submit, review read-back, and the new preflight) — some
    Cloudflare-fronted Gitea hosts intermittently reject curl's default UA.

Patch 5c — -H/--host override + repo-exists preflight

Reproduced against baseline: ./pr-review.sh -n 1 -a comment -c x -H example.test
failed with Unknown option: -H.

Change:

  • -H/--host <host> — explicit Gitea host override, skipping remote-host
    inference, so ambient CWD/remote state can't cross-wire a review to the
    wrong instance.
  • Repo-exists preflight: when -r is used, GET .../repos/<slug> runs
    before any write. A wrong-host cross-wire now surfaces as a clear
    preflight error instead of an opaque write-404 with no residue.

Patch 5b — HALTED (does not reproduce; already superseded)

Named bug: readback verification asserts on issue_url and fails when Gitea
returns it empty on PR comments.

This does not reproduce against baseline. The current
gitea_create_comment_verified() doesn't assert on issue_url at all — it
keys off pull_request_url with full origin + path pinning (scheme, host,
port, and the entire path, not a bare endswith suffix check), explicitly
rejecting a bare issue_url for a claimed PR comment (see the inline
rationale and test-pr-review-gitea-comment.sh case 17,
comment-plain-issue), and already case-folds a mixed-case repo slug in that
same comparison (case 15b, comment-mixed-case-slug, #875). This is a
strictly stronger fix than the "fall back to html_url" patch described in the
source kit — porting the kit's simpler fallback verbatim would have weakened
an already-hardened, already-tested verification path. Not ported.

Patch 5d — HALTED (does not reproduce; already superseded)

Named bug: tea approve/reject path used instead of REST-native
/pulls/{n}/reviews.

This does not reproduce against baseline. grep -c 'tea pr' pr-review.sh
is 0 — there is no tea approve/tea reject call anywhere in the file.
gitea_submit_review_verified() already POSTs to /pulls/{n}/reviews and
verifies the created review by its exact provider-returned id, with additional
hardening not present in the source kit's description: identity-bound
verification (author == acting identity), PR-head commit_id pinning, exact
submitted-body verification (catching Gitea's pending-review reuse/lost-body
edge cases), and a current-head TOCTOU close-out (re-reads the PR head after
verification and fails closed if it advanced). Already covered by 18 cases in
test-pr-review-gitea-comment.sh. Not ported.

Verification

  • Existing test-pr-review-gitea-comment.sh: passes, unchanged.
  • New test-pr-review-repo-host-override.sh (wired into
    packages/mosaic's test:framework-shell): covers flag parsing, help text,
    detect_platform tolerance with no git origin, override winning over a
    wrong git-remote, the repo-exists preflight (pass + fail), and that every
    curl call in a full run carries the User-Agent: mosaic-pr-review header.
    Red-first confirmed: fails against the pre-patch baseline file
    (Unknown option: -r), passes against the patched file.
  • Full pnpm run test:framework-shell chain: all green (both pr-review
    tests + the rest of the wired suite).
  • shellcheck on both changed .sh files: zero new findings (the sole
    pre-existing SC1091 info-level note on the source line is unchanged
    besides its line number shifting).
  • grep -niE 'jason|woltje|jarvis' on changed files: 0 hits.
  • packages/mosaic/framework/tools/quality/scripts/verify-sanitized.sh:
    passes (sanitization gate).

Discipline notes

  • Built from a fresh clone of origin/main (mosaicstack/stack), authored
    as mosaic-coder <coder@fleet.mosaicstack.dev> — not the stale
    /src/mosaic-stack checkout.
  • No merge, no self-review performed.

Part of #891

## Summary Upstreams the `pr-review.sh` patch family **5 + 5b + 5c + 5d** from the host-local tooling-patches kit into the framework, against a **fresh clone of origin/main** (not the stale `/src/mosaic-stack` checkout). Per the reproduce-first-or-halt discipline: **two of the four named sub-changes (5b, 5d) did not reproduce against baseline** — the current framework `pr-review.sh` already implements a more robust fix for each, landed independently of the host-local kit. Those two are **HALTed** (not ported) to avoid regressing already-shipped hardening. Only **5** and **5c** genuinely reproduced and are included. ## Patch 5 — `-r/--repo` override + `User-Agent: mosaic-pr-review` header **Reproduced against baseline:** neither flag existed — `./pr-review.sh -n 1 -a comment -c x -r foo/bar` failed with `Unknown option: -r`; `grep -c User-Agent pr-review.sh` was `0`. **Change:** - `-r/--repo <owner/repo>` — explicit slug override, skipping git-remote slug inference. Mirrors the already-established `-r` convention of the sibling wrappers (`pr-view.sh`, `pr-diff.sh`, `pr-ci-wait.sh`; #867), rather than reinventing a parallel scheme. `detect_platform` is now tolerant of a missing/foreign git origin when `-r` is given (assumes gitea), matching the same sibling-wrapper convention — otherwise `-r` couldn't achieve its stated purpose (reviewer worktrees with a nonstandard/missing origin). - `User-Agent: mosaic-pr-review` added to **every** Gitea curl call in the file (comment write, comment read-back, `/user` identity read, PR-head read, review submit, review read-back, and the new preflight) — some Cloudflare-fronted Gitea hosts intermittently reject curl's default UA. ## Patch 5c — `-H/--host` override + repo-exists preflight **Reproduced against baseline:** `./pr-review.sh -n 1 -a comment -c x -H example.test` failed with `Unknown option: -H`. **Change:** - `-H/--host <host>` — explicit Gitea host override, skipping remote-host inference, so ambient CWD/remote state can't cross-wire a review to the wrong instance. - Repo-exists preflight: when `-r` is used, `GET .../repos/<slug>` runs **before any write**. A wrong-host cross-wire now surfaces as a clear preflight error instead of an opaque write-404 with no residue. ## Patch 5b — HALTED (does not reproduce; already superseded) Named bug: readback verification asserts on `issue_url` and fails when Gitea returns it empty on PR comments. **This does not reproduce against baseline.** The current `gitea_create_comment_verified()` doesn't assert on `issue_url` at all — it keys off `pull_request_url` with **full origin + path pinning** (scheme, host, port, and the entire path, not a bare `endswith` suffix check), explicitly **rejecting** a bare `issue_url` for a claimed PR comment (see the inline rationale and `test-pr-review-gitea-comment.sh` case 17, `comment-plain-issue`), and already case-folds a mixed-case repo slug in that same comparison (case 15b, `comment-mixed-case-slug`, `#875`). This is a strictly stronger fix than the "fall back to `html_url`" patch described in the source kit — porting the kit's simpler fallback verbatim would have **weakened** an already-hardened, already-tested verification path. Not ported. ## Patch 5d — HALTED (does not reproduce; already superseded) Named bug: `tea approve/reject` path used instead of REST-native `/pulls/{n}/reviews`. **This does not reproduce against baseline.** `grep -c 'tea pr' pr-review.sh` is `0` — there is no `tea approve`/`tea reject` call anywhere in the file. `gitea_submit_review_verified()` already POSTs to `/pulls/{n}/reviews` and verifies the created review by its exact provider-returned id, with additional hardening not present in the source kit's description: identity-bound verification (author == acting identity), PR-head `commit_id` pinning, exact submitted-body verification (catching Gitea's pending-review reuse/lost-body edge cases), and a current-head TOCTOU close-out (re-reads the PR head after verification and fails closed if it advanced). Already covered by 18 cases in `test-pr-review-gitea-comment.sh`. Not ported. ## Verification - Existing `test-pr-review-gitea-comment.sh`: **passes**, unchanged. - New `test-pr-review-repo-host-override.sh` (wired into `packages/mosaic`'s `test:framework-shell`): covers flag parsing, help text, `detect_platform` tolerance with no git origin, override winning over a wrong git-remote, the repo-exists preflight (pass + fail), and that every curl call in a full run carries the `User-Agent: mosaic-pr-review` header. **Red-first confirmed**: fails against the pre-patch baseline file (`Unknown option: -r`), passes against the patched file. - Full `pnpm run test:framework-shell` chain: **all green** (both pr-review tests + the rest of the wired suite). - `shellcheck` on both changed `.sh` files: **zero new findings** (the sole pre-existing SC1091 info-level note on the `source` line is unchanged besides its line number shifting). - `grep -niE 'jason|woltje|jarvis'` on changed files: **0 hits**. - `packages/mosaic/framework/tools/quality/scripts/verify-sanitized.sh`: **passes** (sanitization gate). ## Discipline notes - Built from a **fresh clone** of `origin/main` (`mosaicstack/stack`), authored as `mosaic-coder <coder@fleet.mosaicstack.dev>` — not the stale `/src/mosaic-stack` checkout. - No merge, no self-review performed. Part of #891
jason.woltje added 1 commit 2026-07-25 21:19:21 +00:00
fix(framework): pr-review.sh -r/--repo + -H/--host overrides, UA header, repo preflight (Patch 5/5c)
Some checks failed
ci/woodpecker/pr/ci Pipeline was canceled
Details
03e1cdbcd9 
Upstreams the pr-review.sh -r/--repo and -H/--host override family from the
host-local tooling-patches kit (patch 5 + 5c), adapted to the framework's
current REST-native review/comment implementation:

- -r/--repo <owner/repo>: explicit slug override, skipping git-remote slug
  inference, for reviewer worktrees whose origin is nonstandard or missing.
  Mirrors the established -r convention of the sibling wrappers (pr-view.sh,
  pr-diff.sh, pr-ci-wait.sh; #867). detect_platform is now tolerant of a
  missing/foreign git origin when -r is given (assumes gitea), matching the
  same sibling-wrapper convention.
- -H/--host <host>: explicit Gitea host override, skipping remote-host
  inference, so ambient CWD/remote state cannot cross-wire a review/comment to
  the wrong instance.
- Repo-exists preflight: when -r is used, GET .../repos/<slug> BEFORE any
  write. A wrong-host cross-wire now surfaces as a clear preflight error
  instead of an opaque write-404.
- `User-Agent: mosaic-pr-review` header added to every Gitea curl call (write,
  read-back, /user, PR-head read, preflight) — some Cloudflare-fronted Gitea
  hosts intermittently reject curl's default User-Agent.

Reproduce-first evidence (against origin/main baseline, fresh clone):
- -r/-H: `./pr-review.sh -n 1 -a comment -c x -r foo/bar` / `-H example.test`
  both failed with "Unknown option" pre-patch; both parse post-patch.
- User-Agent: `grep -c User-Agent pr-review.sh` was 0 pre-patch.
- New regression test (test-pr-review-repo-host-override.sh) fails against the
  baseline file and passes against the patched file (red-first confirmed).

Scope note — two sub-changes from the source kit's patch family (5b: an
issue_url-empty html_url readback fallback; 5d: REST-native reviews replacing
`tea approve/reject`) were investigated and found ALREADY SUPERSEDED on this
baseline by a more robust, already-tested implementation:
- 5d: the current gitea_submit_review_verified() already POSTs to
  /pulls/{n}/reviews (no `tea pr approve/reject` call exists in this file —
  `grep -c 'tea pr' pr-review.sh` is 0), with additional hardening (identity
  binding, PR-head commit pinning, current-head TOCTOU close-out) beyond what
  the source patch describes.
- 5b: comment verification already keys off `pull_request_url` (not
  `issue_url`) with full origin+path pinning (see gitea_create_comment_verified
  / test-pr-review-gitea-comment.sh cases 15b, 17), which already handles the
  Gitea empty-issue_url-on-PR-comments behavior the source patch targets, via a
  stricter mechanism than a bare html_url fallback would provide. Porting the
  source kit's simpler fallback verbatim would have been a regression.

Per HALT-on-no-repro discipline, only the two sub-changes that actually
reproduced against baseline (5, 5c) are included here.

Part of #891
jason.woltje added 1 commit 2026-07-25 21:46:20 +00:00
fix(framework): guard dispatch-level host resolution against a missing git origin (pr-review.sh)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
dac9838b26 
Independent review of PR #896 found the -r/--repo + -H/--host overrides had a
real bug that this addresses:

1. The approve/request-changes/comment dispatch bodies each independently
   called a BARE `host=$(get_remote_host)` (used only for a best-effort
   tea-login guess) that ignored -H/--host entirely and, under `set -e`, died
   SILENTLY (exit 1, zero stdout/stderr) whenever there was no git origin at
   all — exactly the "reviewer worktree with no usable origin" case -r/-H
   exist to support. This was WORSE than the pre-5/5c baseline, which at
   least failed loud at detect_platform. Fixed by preferring -H/--host and
   otherwise tolerating a missing/failing git-remote lookup for that
   best-effort guess: `host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}"`.

2. The regression test's "no git origin" fixture was a subdirectory NESTED
   inside this checkout, so `git remote get-url origin` resolved UPWARD to the
   checkout's own real origin — the "tolerates a missing origin" assertions
   never actually exercised a no-origin case. Fixed by creating that fixture
   via `mktemp -d` under the system /tmp (truly outside any .git ancestry) and
   additionally bounding it with GIT_CEILING_DIRECTORIES, plus a fixture
   self-check that fails the harness if `git remote get-url origin` ever
   resolves from inside it. Extended coverage to approve/request-changes (not
   just comment), each asserting the run is not silently dead (exit != 0 with
   zero combined output) in the true no-origin dir.

Reproduce-first evidence: with the CORRECTED fixture, re-running the
regression test against the previous (round-1) pr-review.sh reproduces the
exact reported failure — "died SILENTLY (exit 1, zero output)" — for the
`comment` action; the fix in this commit resolves it for comment, approve,
and request-changes alike.

Re-verified: pnpm run test:framework-shell (both pr-review tests + full
chain) green; shellcheck 0 new findings; grep -niE 'jason|woltje|jarvis' 0
hits; verify-sanitized.sh passed.

Part of #891
jason.woltje changed title from fix(framework): pr-review.sh repo/host overrides + UA + html_url readback fallback + REST-native reviews (Patches 5/5b/5c/5d) to fix(framework): pr-review.sh -r/--repo + -H/--host overrides + UA + repo preflight (Patches 5/5c) 2026-07-25 21:46:29 +00:00
jason.woltje commented 2026-07-25 22:07:27 +00:00
Author
Owner
Copy Link

Record of Review — PR #896 (pr-review.sh Patches 5+5c; Part of #891) — POST-FIX, VERDICT APPROVE

VERDICT: APPROVE at CI-green head. Independent re-review after a REQUEST-CHANGES fix cycle.
Reviewer: independent subagent a81636717 (ms-lead-reviewer), fresh clone, reproduced from first principles — ≠ builder (a3c08f74, commit-author mosaic-coder).
REVIEWED-HEAD: dac9838b26e35c196c6442ffe90fd2d7157f4bd1 (unmoved).

The 3 REQUEST-CHANGES items — ALL FIXED + independently verified

  1. Silent-fail bug — FIXED (PASS). All 3 dispatch sites now host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}". Reproduced from first principles with stub env in a TRUE no-origin dir: pre-fix (03e1cdb) all 3 actions died silent (exit1, zero output); post-fix all 3 emit a clear diagnostic (proceed to token lookup). Proved -H/HOST_OVERRIDE genuinely honored (trap-git stub: get_remote_host never called with -r+-H). Audited a 4th call site (line 298) — safe (set -e suspended in conditional function call). No remaining unguarded sites.
  2. Test fixture — FIXED (PASS). NO_GIT_DIR now mktemp -d under /tmp + GIT_CEILING_DIRECTORIES + self-check; coverage extended to approve/request-changes with assert_not_silently_dead. Genuine RED-FIRST: old pr-review.sh + new test = FAIL (silent-death caught); fixed = PASS.
  3. Title — FIXED (PASS). Gitea title now "(Patches 5/5c)". (Woodpecker cached-title showing 5/5b/5c/5d is a stale-webhook metadata quirk, not a defect.)

Unchanged-good confirmed

  • 5b/5d halt intact: tea pr=0; pull_request_url pinning present/unchanged. No regression to 5/5c.
  • Scope: only pr-review.sh + its test (git diff --stat).

Gates + CI

  • test:framework-shell full 19-step chain green; shellcheck 0 new; firewall jason|woltje|jarvis=0; verify-sanitized passed.
  • CI TERMINAL-GREEN: Woodpecker pipeline 2016 @ dac9838b, ALL 8 steps success incl test (the #897-flaky recovery_b1 did NOT flake this run — clean unambiguous pass, not flake-masked).
  • open, mergeable=true, up-to-date with main (merge-base==main tip), base main, body Part of #891 (maps to (b)), commit-author mosaic-coder ≠ reviewer.

GO for id-11 stamp + merge at FULL-40 head dac9838b26e35c196c6442ffe90fd2d7157f4bd1. I relay; Mos stamps id-11 + merges. Non-executor.

# Record of Review — PR #896 (pr-review.sh Patches 5+5c; Part of #891) — POST-FIX, VERDICT APPROVE **VERDICT: APPROVE** at CI-green head. Independent re-review after a REQUEST-CHANGES fix cycle. **Reviewer:** independent subagent `a81636717` (`ms-lead-reviewer`), fresh clone, reproduced from first principles — ≠ builder (`a3c08f74`, commit-author `mosaic-coder`). **REVIEWED-HEAD:** `dac9838b26e35c196c6442ffe90fd2d7157f4bd1` (unmoved). ## The 3 REQUEST-CHANGES items — ALL FIXED + independently verified 1. **Silent-fail bug — FIXED (PASS).** All 3 dispatch sites now `host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}"`. Reproduced from first principles with stub env in a TRUE no-origin dir: pre-fix (03e1cdb) all 3 actions died silent (exit1, zero output); post-fix all 3 emit a clear diagnostic (proceed to token lookup). Proved `-H`/HOST_OVERRIDE genuinely honored (trap-git stub: get_remote_host never called with -r+-H). Audited a 4th call site (line 298) — safe (set -e suspended in conditional function call). No remaining unguarded sites. 2. **Test fixture — FIXED (PASS).** NO_GIT_DIR now `mktemp -d` under /tmp + GIT_CEILING_DIRECTORIES + self-check; coverage extended to approve/request-changes with assert_not_silently_dead. Genuine RED-FIRST: old pr-review.sh + new test = FAIL (silent-death caught); fixed = PASS. 3. **Title — FIXED (PASS).** Gitea title now "(Patches 5/5c)". (Woodpecker cached-title showing 5/5b/5c/5d is a stale-webhook metadata quirk, not a defect.) ## Unchanged-good confirmed - **5b/5d halt intact:** `tea pr`=0; pull_request_url pinning present/unchanged. No regression to 5/5c. - Scope: only pr-review.sh + its test (git diff --stat). ## Gates + CI - `test:framework-shell` full 19-step chain green; shellcheck 0 new; firewall `jason|woltje|jarvis`=0; verify-sanitized passed. - **CI TERMINAL-GREEN:** Woodpecker pipeline 2016 @ `dac9838b`, ALL 8 steps success incl `test` (the #897-flaky recovery_b1 did NOT flake this run — clean unambiguous pass, not flake-masked). - open, mergeable=true, up-to-date with main (merge-base==main tip), base main, body `Part of #891` (maps to (b)), commit-author mosaic-coder ≠ reviewer. **GO for id-11 stamp + merge at FULL-40 head `dac9838b26e35c196c6442ffe90fd2d7157f4bd1`. I relay; Mos stamps id-11 + merges. Non-executor.**
Mos approved these changes 2026-07-25 22:08:07 +00:00
Mos left a comment
First-time contributor
Copy Link

GO — Gate-16 id-11 stamp, pinned to exact FULL head dac9838b26.
Basis: verbatim RoR (comment 18796) — independent re-review APPROVE after a full REQUEST-CHANGES cycle in which the reviewer reproduced both the original silent-fail (pre-fix silent death in a true no-origin dir) and its cure (clear diagnostic; -H/HOST_OVERRIDE honored, no unguarded get_remote_host sites), verified the genuinely-isolated fixture with red-first, and confirmed the 5b/5d halt decisions remain intact (baseline REST-native path untouched, zero tea invocations). CI terminal-green (pipeline 2016, all 8 steps, clean test pass). Patches 5+5c upstream; 5b/5d closed OBE — the baseline exceeded the kit. Part of #891. Named executor: Mos (id-11), squash pinned.

GO — Gate-16 id-11 stamp, pinned to exact FULL head dac9838b26e35c196c6442ffe90fd2d7157f4bd1. Basis: verbatim RoR (comment 18796) — independent re-review APPROVE after a full REQUEST-CHANGES cycle in which the reviewer reproduced both the original silent-fail (pre-fix silent death in a true no-origin dir) and its cure (clear diagnostic; -H/HOST_OVERRIDE honored, no unguarded get_remote_host sites), verified the genuinely-isolated fixture with red-first, and confirmed the 5b/5d halt decisions remain intact (baseline REST-native path untouched, zero tea invocations). CI terminal-green (pipeline 2016, all 8 steps, clean test pass). Patches 5+5c upstream; 5b/5d closed OBE — the baseline exceeded the kit. Part of #891. Named executor: Mos (id-11), squash pinned.
Mos merged commit 2483dada33 into main 2026-07-25 22:08:09 +00:00
Mos deleted branch feat/pr-review-5-family 2026-07-25 22:08:10 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Mos
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#896
Delete Branch "feat/pr-review-5-family"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?