Skip to content

Post forge-native GitHub reviews - #196

Open
artyomsv wants to merge 34 commits into
masterfrom
feat/github-reviews
Open

artyomsv wants to merge 34 commits into
masterfrom
feat/github-reviews

Conversation

@artyomsv

Copy link
Copy Markdown
Owner

The reviewer posts a GitHub pull request review (Approve / Request changes / Comment) with the inline findings inside it; the factory requests the bot as reviewer; a per-repository "Reviewer may approve" setting.

Spec: docs/superpowers/specs/2026-10-15-github-reviews-design.md
Plan: docs/superpowers/plans/2026-10-15-github-reviews.md

Part of #115

The reviewer posts a pull request review on GitHub: Approve, Request
changes or Comment, with the inline findings inside it. The orchestrator
decides the verdict from the same open-blocker count the land gate
reads; the worker posts it through one new port method, falling back to
today's comments on forges without native reviews. A per-repository
setting keeps the bot from changing merge state, and the factory
requests the reviewer on its pull requests.

Part of #115
The adversarial review of the design against the code found four claims
the build would have been wrong on, and fifteen gaps:

- a Comment review does not clear a reviewer's Request changes, so the
  setting-off rule needs a dismissal (D11);
- the REVIEW idempotency claim cannot find a half-posted review, so the
  worker looks the review up before posting and marks it at once;
- the worker never received the assessment it had to put in words, so
  the command carries assessment, verdict and the setting;
- the land-gate guard is the recorded machine actor, not review.human(),
  so a reviewer without an identity never gets an Approve.

Part of #115
Fourteen tasks (0-13): contract, GitHub adapter, worker native path,
the repository setting, projection, verdict decision, factory reviewer
request, guard tests, dashboard, docs, mutation checks and the live
proof. Tests first in every task, CI as the only test runner.

Part of #115
PostComments carries the orchestrator's assessment, the verdict to post
and the repository setting; CommentsPosted carries what was posted.
CommentSink gains find/post/update/dismiss defaults for a forge-native
review and PullRequestSink gains requestReviewers (ADR-047).

Part of #115
GitHubReviewApi carries the review calls (ADR-047 §5.2): one POST with
verdict, body and inline comments; the inline ids read back by anchor;
the bot's review for a head found by header; a body update; and the
dismissal of the bot's own standing changes-requested review.

Part of #115
A review:<id> thread ref replies as a top-level comment, reads as the
review body plus the comments after it, and refuses an in-place update.
GitHubPullRequestSink.requestReviewers maps a 422 to
reviewer_not_collaborator (ADR-047 D8, D9).

Part of #115
… verdict

The native path claims every key first, looks for a review an earlier
attempt left on the forge, posts once, marks the claim at once, reads the
inline ids back, falls back on a refused verdict or a rejected anchor,
and dismisses the bot's own hold when the repository setting is off.

Part of #115
V88 adds repository.reviewer_may_approve (default on) and the two
posted-review columns on review_status. The registry reads, writes and
defaults the setting and answers it to the saga (ADR-047 §4.2).

Part of #115
openBlockers runs the same openCounts the land gate reads;
recordPostedVerdict writes under the recordPosted commit guard; the
summary and detail records carry the posted verdict slug and the forge
link (ADR-047 §4.3).

Part of #115
ReviewVerdictDecision is the pure rule (ADR-047 §4.1): degraded gives
Comment, an open BLOCKER gives Request changes, else Approve; an unknown
repository, the setting off, or a reviewer without a bot identity turn
the posted verdict into Comment. ResultSaga carries the decision on
PostComments and records the posted verdict from CommentsPosted.

Part of #115
CoreIsProviderNeutralTest refused a provider name in the projection: how
a forge addresses a review is adapter knowledge. PostedReview and
CommentsPosted now carry the link the forge reports (GitHub's html_url),
and review_status stores it as given.

ResultSaga reads the registries through lambdas: a method reference on a
field dereferences it when built, which a saga fake that set neither
registry showed as an NPE on every ReviewGenerated.

Part of #115
Once per attempt and pull request, with a factory-channel receipt so a
restart does not ask twice; skipped once the bot has posted for this
head; a refusal or fault is a receipt, never a wait (ADR-047 D9).

Part of #115
The review page says "Posted as Approved / Changes requested / Comment"
with the forge's link; the work item review step links to that page;
the repository form and detail carry the setting (ADR-047 §4.4).

Part of #115
ADR-047, the SCM mapping for a review as the summary, the UNVERIFIED
entries for the fallbacks and the dismissal, the CLAUDE.md limit line,
and the spec and plan amendments made during execution (the review link
comes from the forge; the inline refs are keyed by anchor).

Part of #115
A new head must clear the posted verdict; a crash after a body-only
review must still post the findings inline on redelivery; a comment
whose line moved must match on its original line.

Part of #115
…on redelivery

A new run clears posted_verdict, posted_review_ref and posted_review_url
with the other outcome columns, so an old head's "Posted as" never sits
under "reviewing". A review found on the forge that carries none of this
run's comments was left by a body-only attempt that died: the findings
are posted one by one, as that ladder would have done. A comment whose
line GitHub nulled after the head moved matches on its original line.

Two tests pin what mutation checks t1-a and t1-b showed unpinned: the
wither keeps the verdict fields and the legacy constructor leaves them
null.

Part of #115
Mutation check t10-b survived: the edit-form test starts from a saved
value, so only the registration path sees the form's default.

Part of #115
@artyomsv
artyomsv marked this pull request as ready for review October 16, 2026 17:55
Live on spire-test PR 53 the review's own comment list came back in the
legacy shape (path and position only, line and side null), so no inline
comment matched and a comment that was on GitHub was reported as not
posted. Part of #115.
GitHub answers a review's own comment list in the legacy shape: path and
position only, with line, side and start_line null. The read-back matched
nothing, so a comment that was on GitHub was reported as not posted and
the orchestrator recorded no inline thread (spire-test PR 53). The pull
request's comment list carries line, side and pull_request_review_id;
walk that and keep this review's rows. Part of #115.
A re-run on the same head found the earlier run's review by its header
and reused it (spire-test PR 55): nothing new was posted and the hold was
dismissed as superseded by itself. Part of #115.
The worker looks for a review of the head before posting, so a crash
between the post and the mark does not post twice. A dashboard re-run
clears the worker's claims, and that look then found the earlier run's
review by its header, reused it and, with the setting off, dismissed it
as superseded by itself (spire-test PR 55). Each claimed run and re-run
now gets an opaque run_token on review_status; PostComments carries it
and the worker writes it on the header line as an HTML comment, so the
look matches this run's review only. A retry and a metadata refresh keep
the token. Part of #115.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant