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

fix(framework): drop unsupported --comment from tea pr approve/reject; route review body via durable comment (#835) #857

Merged
jason.woltje merged 1 commits from fix/835-pr-review-approve-reject-comment-flag into main 2026-07-20 09:19:49 +00:00
Conversation 2 Commits 1 Files Changed 2 +169 -101
jason.woltje commented 2026-07-20 08:59:42 +00:00
Owner
Copy Link

Defect (#835, p3-low)

packages/mosaic/framework/tools/git/pr-review.sh's Gitea approve/reject paths passed a --comment flag that tea v0.11.1 does not define. tea pr approve / tea pr reject errored with flag provided but not defined: -comment whenever a review body was supplied, breaking the durable formal-review record-of-review path fleet-wide (fails closed, not a security hole).

Fix (minimal, shell-tooling only)

  • Removed --comment/-comment from both tea pr approve and tea pr reject invocations.
  • When a review body is supplied, it is now posted via the same verified Gitea comments REST API path the comment action already uses (#812) — write, then provider read-back verification of ID/body/repo/PR before reporting success.
  • Extracted the shared logic into a gitea_post_verified_comment() helper reused by all three actions (approve, request-changes, comment) instead of duplicating it. The pre-existing comment action's behavior and output are unchanged (still calls the same underlying write+read-back sequence, now via the shared helper).
  • If no comment body is supplied to approve, behavior is unchanged (plain approve, no comment call).

Red-first test

Extended the existing #812 regression harness (packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh), which already exercises approve/reject/comment against stubbed tea/curl. The tea stub now rejects any -comment/--comment flag on pr approve/pr reject exactly like real tea v0.11.1 (flag provided but not defined: -comment, exit 1) so it authentically reproduces the bug.

  • RED (manual reproduction against pre-fix pr-review.sh, and against the updated harness before the fix): tea pr approve/tea pr reject invocations including --comment fail with flag provided but not defined: -comment, exit 1.
  • GREEN (after the fix): tea pr approve/tea pr reject run without --comment and succeed; when a body is supplied it is posted and read-back-verified via REST, reporting Added and verified review comment on Gitea PR #<n> (comment ID <id>).
  • All 7 packages/mosaic/framework/tools/git/test-*.sh harnesses pass, including the extended one.

Gates (repo root)

  • pnpm typecheck — PASS (42/42 tasks)
  • pnpm lint — PASS (23/23 tasks)
  • pnpm format:check — PASS
  • pnpm test — PASS (43/43 turbo tasks; Mosaic 75 files/1434 tests, Gateway 56 files/628 tests, DB 2/3 files, plus framework-shell python/bash suites)
  • shellcheck -x packages/mosaic/framework/tools/git/pr-review.sh — PASS (0 findings; plain shellcheck without -x reports only the pre-existing informational SC1091 for the source of a sibling script, matching this file's existing # shellcheck source= directive convention)
  • shellcheck -x on the updated test harness — PASS

No pre-existing failures were observed in any of the above; nothing new was introduced.

Integrity

  • No --no-verify, no weakened/skipped tests, no fabricated results.
  • Change is confined to framework tooling shell scripts and their test harness — no production application code touched.
  • Minimal scope: only the approve/reject --comment handling and shared-helper extraction; the comment action's own logic/output is unchanged (only reuses the extracted helper).

Closes #835. NO self-merge — awaiting named-executor merge clearance per authority model. Author lane: ms-835-build.

## Defect (#835, p3-low) `packages/mosaic/framework/tools/git/pr-review.sh`'s Gitea approve/reject paths passed a `--comment` flag that `tea` v0.11.1 does not define. `tea pr approve` / `tea pr reject` errored with `flag provided but not defined: -comment` whenever a review body was supplied, breaking the durable formal-review record-of-review path fleet-wide (fails closed, not a security hole). ## Fix (minimal, shell-tooling only) - Removed `--comment`/`-comment` from both `tea pr approve` and `tea pr reject` invocations. - When a review body is supplied, it is now posted via the same verified Gitea comments REST API path the `comment` action already uses (#812) — write, then provider read-back verification of ID/body/repo/PR before reporting success. - Extracted the shared logic into a `gitea_post_verified_comment()` helper reused by all three actions (`approve`, `request-changes`, `comment`) instead of duplicating it. The pre-existing `comment` action's behavior and output are unchanged (still calls the same underlying write+read-back sequence, now via the shared helper). - If no comment body is supplied to `approve`, behavior is unchanged (plain approve, no comment call). ## Red-first test Extended the existing #812 regression harness (`packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh`), which already exercises approve/reject/comment against stubbed `tea`/`curl`. The `tea` stub now rejects any `-comment`/`--comment` flag on `pr approve`/`pr reject` exactly like real `tea` v0.11.1 (`flag provided but not defined: -comment`, exit 1) so it authentically reproduces the bug. - RED (manual reproduction against pre-fix `pr-review.sh`, and against the updated harness before the fix): `tea pr approve`/`tea pr reject` invocations including `--comment` fail with `flag provided but not defined: -comment`, exit 1. - GREEN (after the fix): `tea pr approve`/`tea pr reject` run without `--comment` and succeed; when a body is supplied it is posted and read-back-verified via REST, reporting `Added and verified review comment on Gitea PR #<n> (comment ID <id>)`. - All 7 `packages/mosaic/framework/tools/git/test-*.sh` harnesses pass, including the extended one. ## Gates (repo root) - `pnpm typecheck` — PASS (42/42 tasks) - `pnpm lint` — PASS (23/23 tasks) - `pnpm format:check` — PASS - `pnpm test` — PASS (43/43 turbo tasks; Mosaic 75 files/1434 tests, Gateway 56 files/628 tests, DB 2/3 files, plus framework-shell python/bash suites) - `shellcheck -x packages/mosaic/framework/tools/git/pr-review.sh` — PASS (0 findings; plain `shellcheck` without `-x` reports only the pre-existing informational SC1091 for the `source` of a sibling script, matching this file's existing `# shellcheck source=` directive convention) - `shellcheck -x` on the updated test harness — PASS No pre-existing failures were observed in any of the above; nothing new was introduced. ## Integrity - No `--no-verify`, no weakened/skipped tests, no fabricated results. - Change is confined to framework tooling shell scripts and their test harness — no production application code touched. - Minimal scope: only the approve/reject `--comment` handling and shared-helper extraction; the `comment` action's own logic/output is unchanged (only reuses the extracted helper). Closes #835. NO self-merge — awaiting named-executor merge clearance per authority model. Author lane: ms-835-build.
jason.woltje added 1 commit 2026-07-20 08:59:43 +00:00
fix(framework): drop unsupported --comment from tea pr approve/reject; route review body via durable comment (#835)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
2e8e8aa83b 
tea v0.11.1 defines no --comment/-comment flag on `pr approve` or `pr
reject`, so pr-review.sh's Gitea approve/reject paths errored with
"flag provided but not defined: -comment" whenever a review body was
supplied, breaking the durable formal-review path fleet-wide.

Remove --comment from both tea invocations and, when a review body is
present, post it via the same verified Gitea comments REST API path
the `comment` action already uses (#812) — extracted into a shared
gitea_post_verified_comment() helper so all three actions share one
write+read-back-verified implementation instead of duplicating it.

Extends the existing #812 regression harness
(test-pr-review-gitea-comment.sh) with a `tea` stub that rejects
unknown flags exactly like real tea v0.11.1, so it fails RED against
the pre-fix wrapper and passes GREEN after.

Closes #835.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jason.woltje commented 2026-07-20 09:11:14 +00:00
Author
Owner
Copy Link

Durable exact-head Reviewer-of-Record — APPROVE (do not merge without coordinator named-executor clearance)

Exact head bound: 2e8e8aa83b4d4810d01e156438914b7798e8dee4

Independent CODE review — author lane ms-835-build != reviewer lane ms-857-review (distinct network-capable lane; conducted READ-ONLY via Gitea API + a read-only local clone of the PR head; no commits/pushes/approvals via any tool). Persisted here as a durable comment because the tea pr approve durable path is broken by exactly the defect this PR fixes (#835). VERDICT: APPROVE.

  • Scope (verified /pulls/857/files): exactly 2 files, both framework-shell — packages/mosaic/framework/tools/git/pr-review.sh (+132/-97) and test-pr-review-gitea-comment.sh (+37/-4). No production application code; no unrelated files.
  • Fix correctness: tea pr approve (L213) and tea pr reject (L230) no longer pass --comment/-comment. A shared helper gitea_post_verified_comment() (L69-176) is extracted verbatim from the pre-existing comment-action logic (POST → parse id → GET read-back → verify id/body/issue_url) and reused by approve (only when a body is supplied), request-changes (unconditional, -z COMMENT guard unchanged), and comment. The comment action's output string is byte-for-byte identical to pre-PR → no #812 regression.
  • Fails closed: every helper failure path (token/URL/repo resolution, curl transport, non-201 write, malformed/missing id, non-200 read-back, id/body/issue_url mismatch) → stderr + return 1; all three call sites use $(...) || exit 1. No silent-success path. shellcheck -x pr-review.sh = 0 findings.
  • Injection-safe: comment body is JSON-encoded via python3 os.environ, never interpolated into shell/JSON literals; curl args quoted. Regression test round-trips a body with embedded quotes, a leading --, and a newline.
  • Red-first genuine (independently reproduced): reviewer checked out pull/857/head (rev-parse == the bound head), ran the harness = PASS; paired the NEW harness with the pre-fix pr-review.sh (merge-base 11d2818) = genuine RED (exit 1) at the added approve-with-body case. All pre-existing #812 assertions present unmodified — nothing weakened/removed/repointed; no --no-verify/skip.
  • CI: Woodpecker pipeline #1942 (pull_request @ 2e8e8aa8) = success, all 9 steps green (clone, ci-postgres, install, sanitization, upgrade-guard, typecheck, lint, format, test).

Non-blocking nit (operational, not a defect): splitting approve/reject from the comment POST is two non-atomic tea calls; if approve/reject succeeds but the comment POST fails, the PR is approved/rejected with no comment and the script exits non-zero (fails loud). Unavoidable given tea v0.11.1 has no atomic option; handled as well as possible.

Author != reviewer preserved (author ms-835-build, reviewer ms-857-review). Merge authority is the coordinator's (Mos) — NO self-merge; awaiting a NAMED-EXECUTOR clearance.

### Durable exact-head Reviewer-of-Record — APPROVE (do not merge without coordinator named-executor clearance) **Exact head bound:** `2e8e8aa83b4d4810d01e156438914b7798e8dee4` **Independent CODE review** — author lane `ms-835-build` != reviewer lane `ms-857-review` (distinct network-capable lane; conducted READ-ONLY via Gitea API + a read-only local clone of the PR head; no commits/pushes/approvals via any tool). Persisted here as a durable comment because the `tea pr approve` durable path is broken by exactly the defect this PR fixes (#835). **VERDICT: APPROVE.** - **Scope (verified `/pulls/857/files`):** exactly 2 files, both framework-shell — `packages/mosaic/framework/tools/git/pr-review.sh` (+132/-97) and `test-pr-review-gitea-comment.sh` (+37/-4). No production application code; no unrelated files. - **Fix correctness:** `tea pr approve` (L213) and `tea pr reject` (L230) no longer pass `--comment`/`-comment`. A shared helper `gitea_post_verified_comment()` (L69-176) is extracted verbatim from the pre-existing `comment`-action logic (POST → parse id → GET read-back → verify id/body/issue_url) and reused by approve (only when a body is supplied), request-changes (unconditional, `-z COMMENT` guard unchanged), and comment. The `comment` action's output string is byte-for-byte identical to pre-PR → **no #812 regression**. - **Fails closed:** every helper failure path (token/URL/repo resolution, curl transport, non-201 write, malformed/missing id, non-200 read-back, id/body/issue_url mismatch) → stderr + `return 1`; all three call sites use `$(...) || exit 1`. No silent-success path. `shellcheck -x pr-review.sh` = 0 findings. - **Injection-safe:** comment body is JSON-encoded via `python3 os.environ`, never interpolated into shell/JSON literals; curl args quoted. Regression test round-trips a body with embedded quotes, a leading `--`, and a newline. - **Red-first genuine (independently reproduced):** reviewer checked out `pull/857/head` (rev-parse == the bound head), ran the harness = PASS; paired the NEW harness with the pre-fix `pr-review.sh` (merge-base `11d2818`) = genuine RED (exit 1) at the added `approve`-with-body case. All pre-existing #812 assertions present unmodified — nothing weakened/removed/repointed; no `--no-verify`/skip. - **CI:** Woodpecker pipeline **#1942** (pull_request @ `2e8e8aa8`) = **success**, all 9 steps green (clone, ci-postgres, install, sanitization, upgrade-guard, typecheck, lint, format, test). **Non-blocking nit (operational, not a defect):** splitting approve/reject from the comment POST is two non-atomic `tea` calls; if approve/reject succeeds but the comment POST fails, the PR is approved/rejected with no comment and the script exits non-zero (fails loud). Unavoidable given tea v0.11.1 has no atomic option; handled as well as possible. Author != reviewer preserved (author `ms-835-build`, reviewer `ms-857-review`). Merge authority is the coordinator's (Mos) — NO self-merge; awaiting a NAMED-EXECUTOR clearance.
jason.woltje commented 2026-07-20 09:19:35 +00:00
Author
Owner
Copy Link

Named-executor merge — PROVENANCE STAMP (pre-mutation)

Executor: Mos (coordinator / merge-authority). Host web1 · tmux pane mos-claude:0.0 · PID 3935025 · UTC 2026-07-20T09:19:35Z.

Clearance — all 6 checks verified primary-source by the executor, plus dual independent lane verification (MS-LEAD read-back 091339Z + homelab independent 091633Z):

  1. PR #857 open / unmerged / mergeable / base main, 2 framework-shell files, no production code.
  2. CI pipeline #1942 terminal success at exact head 2e8e8aa83b4d4810d01e156438914b7798e8dee4 (all steps green).
  3. Durable Reviewer-of-Record comment #18366 = APPROVE, author-lane ms-835-build ≠ reviewer-lane ms-857-review, bound to 2e8e8aa8, no REQUEST-CHANGES.
  4. reviewed-SHA == CI-SHA == PR head, all full-40 2e8e8aa83b4d4810d01e156438914b7798e8dee4.
  5. Body carries Closes #835.
  6. CI queue-guard clear (exit 0).

Merging now via squash. This comment is written BEFORE the merge mutation per the named-executor provenance rule (shared-token attribution hardening; xref #855).

### Named-executor merge — PROVENANCE STAMP (pre-mutation) **Executor:** Mos (coordinator / merge-authority). Host `web1` · tmux pane `mos-claude:0.0` · PID `3935025` · UTC 2026-07-20T09:19:35Z. **Clearance — all 6 checks verified primary-source by the executor, plus dual independent lane verification** (MS-LEAD read-back 091339Z + homelab independent 091633Z): 1. PR #857 open / unmerged / mergeable / base `main`, 2 framework-shell files, no production code. 2. CI pipeline #1942 terminal **success** at exact head `2e8e8aa83b4d4810d01e156438914b7798e8dee4` (all steps green). 3. Durable Reviewer-of-Record comment #18366 = **APPROVE**, author-lane `ms-835-build` ≠ reviewer-lane `ms-857-review`, bound to `2e8e8aa8`, no REQUEST-CHANGES. 4. reviewed-SHA == CI-SHA == PR head, all full-40 `2e8e8aa83b4d4810d01e156438914b7798e8dee4`. 5. Body carries `Closes #835`. 6. CI queue-guard clear (exit 0). Merging now via **squash**. This comment is written BEFORE the merge mutation per the named-executor provenance rule (shared-token attribution hardening; xref #855).
jason.woltje merged commit 3b70c66c07 into main 2026-07-20 09:19:49 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#857
Delete Branch "fix/835-pr-review-approve-reject-comment-flag"

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?