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.
2026-05-01 23:01:32 -07:00
parent ec19622133
commit a9c8ecfb0b
1 changed files with 97 additions and 0 deletions
@@ -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 |