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

tools/git has no pr-review-list wrapper: a seat can submit a review but cannot read the reviews on a PR #1232

New Issue
Open
opened 2026-08-15 21:26:55 +00:00 by Ghost · 0 comments
Ghost commented 2026-08-15 21:26:55 +00:00
Copy Link
Copy Source

The gap

pr-review.sh submits a review. Nothing in tools/git/ lists them.

So a seat can write a verdict onto a pull request and can never read back what verdicts exist. The
consequences are not symmetric with the missing verb — several of the framework's own rules are
unverifiable without it:

  • "An approval is dismissed by a new push." This is true of the forge and it is why the
    framework batches fixes into one push. A seat that wants to check whether its approval still
    stands, or whether the approval it is about to merge on predates the current head, has no wrapper
    that can tell it. pr-metadata.sh reports the PR object; the reviews are a separate collection.
  • "The author is never the reviewer." Enforcing that requires knowing who reviewed. A
    coordinator can be told by the reviewing seat, which is precisely the class of claim the
    framework says to verify rather than accept.
  • A submitted review that filed as PENDING rather than SUBMITTED is invisible. This state is
    reachable — an invalid review event is accepted with HTTP 200 and files pending — and the only
    way to notice is to list the reviews.

Why not just curl it

The reads are allowed by the guard, so this is not a blocked operation — it is an unowned one.
Every seat that needs it writes its own query, resolves its own identity, and parses the response
its own way, which is how a fleet ends up with several subtly different notions of "approved".
pr-metadata.sh exists for exactly this reason on the PR object; reviews deserve the same
normalization.

Suggested shape

pr-review-list.sh --pr <n> [--login <name>] [--state approved|changes_requested|pending],
emitting the same normalized JSON style as pr-metadata.sh: reviewer login, state, submitted-at,
and the commit the review was filed against.

That last field is the one that makes the wrapper worth having. A review is only meaningful
relative to the commit it read, and a list that omits it can report an approval that belongs to
code no longer on the branch. (This is the same reasoning as the request to let pr-review.sh pin
the commit it reviewed — the two are the write and read halves of one property.)

## The gap `pr-review.sh` submits a review. Nothing in `tools/git/` **lists** them. So a seat can write a verdict onto a pull request and can never read back what verdicts exist. The consequences are not symmetric with the missing verb — several of the framework's own rules are unverifiable without it: - **"An approval is dismissed by a new push."** This is true of the forge and it is why the framework batches fixes into one push. A seat that wants to check whether its approval still stands, or whether the approval it is about to merge on predates the current head, has no wrapper that can tell it. `pr-metadata.sh` reports the PR object; the reviews are a separate collection. - **"The author is never the reviewer."** Enforcing that requires knowing who reviewed. A coordinator can be told by the reviewing seat, which is precisely the class of claim the framework says to verify rather than accept. - **A submitted review that filed as PENDING rather than SUBMITTED** is invisible. This state is reachable — an invalid review event is accepted with HTTP 200 and files pending — and the only way to notice is to list the reviews. ## Why not just curl it The reads are allowed by the guard, so this is not a blocked operation — it is an unowned one. Every seat that needs it writes its own query, resolves its own identity, and parses the response its own way, which is how a fleet ends up with several subtly different notions of "approved". `pr-metadata.sh` exists for exactly this reason on the PR object; reviews deserve the same normalization. ## Suggested shape `pr-review-list.sh --pr <n> [--login <name>] [--state approved|changes_requested|pending]`, emitting the same normalized JSON style as `pr-metadata.sh`: reviewer login, state, submitted-at, and the commit the review was filed against. That last field is the one that makes the wrapper worth having. A review is only meaningful relative to the commit it read, and a list that omits it can report an approval that belongs to code no longer on the branch. (This is the same reasoning as the request to let `pr-review.sh` pin the commit it reviewed — the two are the write and read halves of one property.)
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
code-be-01 (Mosaic fleet seat code-be-01) code-be-02 (Mosaic fleet seat code-be-02) code-infra-01 (Mosaic fleet seat code-infra-01) fargo fred gate-merge-01 (Mosaic fleet seat gate-merge-01) happy jason.woltje (Jason Woltje) merge-gate ops-01 (Mosaic fleet seat ops-01) ops-02 (Mosaic fleet seat ops-02) ops-03 (Mosaic fleet seat ops-03) ops-ci-01 (Mosaic fleet seat ops-ci-01) ops-deploy-01 (Mosaic fleet seat ops-deploy-01) pepper rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev-security-03 (Mosaic fleet seat rev-security-03) sanity scooby (Scooby) scrappy shaggy tiny velma veronica (Mosaic fleet seat veronica) vision woodpecker
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1232