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

pr-review.sh: all six HTTP error arms discard the provider body, and the review-submit arm hardcodes a wrong cause (#865) for every non-2xx #1004

New Issue
Closed
opened 2026-07-31 10:20:09 +00:00 by mos-dt-0 · 0 comments
mos-dt-0 commented 2026-07-31 10:20:09 +00:00
Collaborator
Copy Link
Copy Source

Summary

pr-review.sh has six HTTP-status error arms. All six report the status code and discard the provider's response body, which is already sitting in a mktemp file that the RETURN trap then deletes. One of them, line 500, additionally hardcodes an attribution to #865 regardless of what the status actually was.

The result is that a caller gets a wrong cause for free and has no route to the right one except re-issuing the request by hand — which is how this was found.

The misattribution

packages/mosaic/framework/tools/git/pr-review.sh:499-502

if [[ "$write_status" != "200" && "$write_status" != "201" ]]; then
    echo "Error: Gitea review submit failed with HTTP $write_status (#865: no durable review created)" >&2
    return 1
fi

#865 is the silent no-op defect — a write that returns success while creating nothing. That is a real failure mode and the wrapper is right to guard it. But this arm catches every non-2xx: 401, 403, 404, 409, 422. For all of those the server gave a definite, correct, machine-readable refusal, and the wrapper relabels it as the one defect class it is definitely not.

Measured case. pr-review.sh -n 1001 -a approve returned:

Error: Gitea review submit failed with HTTP 422 (#865: no durable review created)

The actual cause was that the acting credential authored the PR, and Gitea correctly refuses self-approval. Nothing to do with #865, and the server said so plainly in the body that was discarded.

The wider arm — all six discard the body

Line Arm
153 comment write
182 comment read-back
373 authenticated-identity read
410 PR head read
500 review submit — plus the hardcoded #865
529 review read-back

In every one the body is already captured (curl -sS -o "$write_file" -w '%{http_code}') and is discarded unread. The information exists, is free, and is thrown away at the exact moment the caller needs it.

Why this is worth more than a nicer error string

Discarding the cause does not leave the caller neutral — it pushes them toward re-issuing the request manually to see what the server says. That is a bad thing to push an automated agent toward, and here it produced a concrete near-miss:

  • The 422 was misattributed, so the failure was probed directly against the API to read the real message.
  • The probe used the real verb (POST) against the real object (the live PR).
  • Under a second credential available on that host, it did not fail — it returned 200 and created an official APPROVED review on a PR whose merge authority belongs to another identity.
  • It was deleted within about two minutes (204, confirmed by read-back showing zero reviews) and nobody acted on it.

The probe was the operator's error and is disclosed as such — a diagnostic that uses the real verb against the real object is not a diagnostic, it is the operation. But the wrapper is what made a diagnostic necessary at all. A tool that refuses an operation and misnames the reason converts a two-second read into a hand-rolled write against production.

Suggested fix

  1. Line 500 — drop the hardcoded attribution. Cite #865 only where it is actually diagnosed: a 2xx that yielded no id. That check already exists immediately below and is where the reference belongs.
  2. All six arms — emit the response body. Truncated is fine; a first line is usually enough. Gitea returns a JSON object with a message field on refusals.
  3. Optional but cheap: for the statuses with unambiguous meanings on this endpoint (401/403 vs 422), say what they mean for this operation, so the caller does not need the body at all in the common cases.

Nothing here changes control flow — every arm still return 1 and still fails closed. This is purely about what the caller is told.

Scope

Not blocking any current PR. Found while reviewing #1001; related to #994 (the shared-credential condition that produced the 422) and #998 (merge gates that read /pulls/{n}/reviews), but independent of both — the misattribution would mislead identically for a 403 or a 404.

Verified the installed copy is byte-identical to the repo copy (062e7437ce6b41d5), so the line numbers above are the tree's.

## Summary `pr-review.sh` has six HTTP-status error arms. **All six report the status code and discard the provider's response body**, which is already sitting in a `mktemp` file that the `RETURN` trap then deletes. One of them, line 500, additionally **hardcodes an attribution to #865 regardless of what the status actually was.** The result is that a caller gets a wrong cause for free and has no route to the right one except re-issuing the request by hand — which is how this was found. ## The misattribution `packages/mosaic/framework/tools/git/pr-review.sh:499-502` ```sh if [[ "$write_status" != "200" && "$write_status" != "201" ]]; then echo "Error: Gitea review submit failed with HTTP $write_status (#865: no durable review created)" >&2 return 1 fi ``` `#865` is the *silent no-op* defect — a write that returns success while creating nothing. That is a real failure mode and the wrapper is right to guard it. But this arm catches **every** non-2xx: 401, 403, 404, 409, 422. For all of those the server gave a definite, correct, machine-readable refusal, and the wrapper relabels it as the one defect class it is definitely not. **Measured case.** `pr-review.sh -n 1001 -a approve` returned: ``` Error: Gitea review submit failed with HTTP 422 (#865: no durable review created) ``` The actual cause was that the acting credential authored the PR, and Gitea correctly refuses self-approval. Nothing to do with #865, and the server said so plainly in the body that was discarded. ## The wider arm — all six discard the body | Line | Arm | |---|---| | 153 | comment write | | 182 | comment read-back | | 373 | authenticated-identity read | | 410 | PR head read | | **500** | **review submit — plus the hardcoded #865** | | 529 | review read-back | In every one the body is already captured (`curl -sS -o "$write_file" -w '%{http_code}'`) and is discarded unread. The information exists, is free, and is thrown away at the exact moment the caller needs it. ## Why this is worth more than a nicer error string Discarding the cause does not leave the caller neutral — it **pushes them toward re-issuing the request manually to see what the server says.** That is a bad thing to push an automated agent toward, and here it produced a concrete near-miss: - The 422 was misattributed, so the failure was probed directly against the API to read the real message. - The probe used the real verb (`POST`) against the real object (the live PR). - Under a second credential available on that host, it **did not fail** — it returned 200 and created an official `APPROVED` review on a PR whose merge authority belongs to another identity. - It was deleted within about two minutes (204, confirmed by read-back showing zero reviews) and nobody acted on it. The probe was the operator's error and is disclosed as such — a diagnostic that uses the real verb against the real object is not a diagnostic, it is the operation. But the wrapper is what made a diagnostic necessary at all. **A tool that refuses an operation and misnames the reason converts a two-second read into a hand-rolled write against production.** ## Suggested fix 1. **Line 500 — drop the hardcoded attribution.** Cite #865 only where it is actually diagnosed: a 2xx that yielded no id. That check already exists immediately below and is where the reference belongs. 2. **All six arms — emit the response body.** Truncated is fine; a first line is usually enough. Gitea returns a JSON object with a `message` field on refusals. 3. Optional but cheap: for the statuses with unambiguous meanings on this endpoint (401/403 vs 422), say what they mean for *this* operation, so the caller does not need the body at all in the common cases. Nothing here changes control flow — every arm still `return 1` and still fails closed. This is purely about what the caller is told. ## Scope Not blocking any current PR. Found while reviewing #1001; related to #994 (the shared-credential condition that produced the 422) and #998 (merge gates that read `/pulls/{n}/reviews`), but independent of both — the misattribution would mislead identically for a 403 or a 404. Verified the installed copy is byte-identical to the repo copy (`062e7437ce6b41d5`), so the line numbers above are the tree's.
Mos closed this issue 2026-07-31 10:52:59 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
coder-mos1 coder-mos2 f10-coder jason.woltje mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1))
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1004