From a9c8ecfb0bc160f936aff403bed5472d28e7305b Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 23:01:32 -0700 Subject: [PATCH] docs: add review update strategy with state transition diagram Explains the edit-in-place approach, state transition rules, worst-wins escalation, and inline comment lifecycle. Includes a Mermaid state diagram for visual reference. --- docs/REVIEW_STRATEGY.md | 97 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 docs/REVIEW_STRATEGY.md diff --git a/docs/REVIEW_STRATEGY.md b/docs/REVIEW_STRATEGY.md new file mode 100644 index 0000000..270d124 --- /dev/null +++ b/docs/REVIEW_STRATEGY.md @@ -0,0 +1,97 @@ +# Review Update Strategy + +review-bot uses an **edit-in-place** strategy for updating reviews. Reviews are never deleted — this preserves conversation threads on inline comments. + +## State Transition Diagram + +```mermaid +stateDiagram-v2 + [*] --> NoExistingReview: First run + + NoExistingReview --> POST_Review: Generate findings + event + POST_Review --> PostEscalationCheck: event == APPROVED? + + PostEscalationCheck --> Done: No sibling blocks + PostEscalationCheck --> Supersede_And_Repost: Sibling has REQUEST_CHANGES + Supersede_And_Repost --> Done: Posted as REQUEST_CHANGES + + [*] --> ExistingReviewFound: Subsequent run (sentinel match) + + ExistingReviewFound --> CheckEscalation: Determine final event + CheckEscalation --> CompareState: Apply worst-wins if needed + + CompareState --> SameState: existing.state == new event + CompareState --> StateChange: existing.state != new event + + SameState --> Skip: Body unchanged + SameState --> PatchBody: Body changed → PATCH in place + + StateChange --> Escalate: APPROVED → REQUEST_CHANGES + StateChange --> Downgrade: REQUEST_CHANGES → APPROVED + + Escalate --> Supersede: PATCH old body → "Superseded" + Supersede --> POST_New_RC: POST new REQUEST_CHANGES + + Downgrade --> POST_New_Approve: POST new APPROVED (old stays intact) + + Skip --> Done + PatchBody --> Done + POST_New_RC --> Done + POST_New_Approve --> Done +``` + +## Rules + +| Scenario | Action | Reason | +|----------|--------|--------| +| No existing review | POST new | First run | +| Same state, same body | Skip | Nothing changed — preserve threads | +| Same state, body changed | PATCH body | Update findings without losing threads | +| APPROVED → REQUEST_CHANGES | Supersede old + POST new | Can always escalate; old APPROVED is no longer valid | +| REQUEST_CHANGES → APPROVED | POST new APPROVED | Can't edit state; old REQUEST_CHANGES stays as historical record | +| Sibling has REQUEST_CHANGES (worst-wins) | Escalate to REQUEST_CHANGES | PR must stay blocked if ANY reviewer blocks | + +## Key Constraints + +1. **Review state is immutable after POST** — Gitea has no API to change APPROVED ↔ REQUEST_CHANGES +2. **Never delete reviews** — Deleting cascades to inline comments and reply threads +3. **"Last review per user" wins** — Gitea uses the most recent review from a user for merge decisions +4. **REQUEST_CHANGES reviews are never touched** — Their inline comments and threads are preserved as historical record +5. **APPROVED reviews can be superseded** — When escalation is needed, mark old as superseded and POST new + +## Worst-Wins (Shared Token) + +When multiple reviewer roles share a token (e.g., `sonnet` and `security` both use `sonnet-review-bot`): + +``` +CI Matrix Run: + sonnet → REQUEST_CHANGES (findings) + security → APPROVED (no security issues) + ↓ + security sees sibling REQUEST_CHANGES + ↓ + security escalates → REQUEST_CHANGES + ↓ + PR stays blocked ✓ +``` + +The **first-run case** (no existing review to read login from) uses a post-posting fallback: +POST APPROVED → check siblings → if blocked, supersede own APPROVED → re-POST as REQUEST_CHANGES. + +## Edit Mechanism + +Reviews are edited via `PATCH /repos/{owner}/{repo}/issues/comments/{id}`: + +- **Review body**: ID obtained from the timeline API (`/issues/{index}/timeline`, type `"review"`) +- **Inline comments**: IDs obtained from `/pulls/{index}/reviews/{id}/comments` +- **Both are editable** by the token that created them +- **ListReviews always returns the original body** (reads from review table, not comment table) — sentinel matching works regardless of edits + +## Inline Comments Lifecycle + +| Event | Inline comments behavior | +|-------|--------------------------| +| First POST | Created on specific diff lines | +| PATCH body (same state) | Unchanged — still current findings | +| Supersede (state change) | Old inline comments stay (readable but on outdated code) | +| New POST after supersede | Fresh inline comments on current diff |