Skip to content

fix(reviewer-bot): mutation-intent routing and git-ref CAS state lock#405

Merged
PLeVasseur merged 3 commits into
Safety-Critical-Rust-Consortium:mainfrom
PLeVasseur:fix/reviewer-bot-burst-lock-and-triage-approval
Feb 18, 2026
Merged

fix(reviewer-bot): mutation-intent routing and git-ref CAS state lock#405
PLeVasseur merged 3 commits into
Safety-Critical-Rust-Consortium:mainfrom
PLeVasseur:fix/reviewer-bot-burst-lock-and-triage-approval

Conversation

@PLeVasseur

@PLeVasseur PLeVasseur commented Feb 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes the reviewer-bot CI regression caused by unsupported conditional writes (If-Match) on Issue PATCH.
  • Replaces issue-body lease locking with a git-ref CAS lock so all mutating reviewer-bot paths are serialized safely.
  • Switches lock gating from event-name based to mutation-intent based, so fork review events can defer cleanly while trusted reconcile remains lock-protected.

Root Cause

  • The prior hardening attempted lease acquire with If-Match against the state issue update endpoint.
  • GitHub rejects conditional headers for this unsafe request path, which caused lock acquire to fail before event handling.
  • For fork pull_request_review events, that failure happened before deferral/handoff logic, so reconcile never ran.

What This PR Does Now

  • Adds mutation-intent routing in main() with explicit intents:
    • mutating
    • non_mutating_defer
    • non_mutating_readonly
  • Acquires lock only for mutating paths.
  • Implements git-ref CAS locking in scripts/reviewer_bot.py:
    • lock ref bootstrap/read (refs/heads/reviewer-bot-state-lock)
    • lock/unlock commits with owner token and expiry metadata
    • fast-forward ref update (force=false) with retry/backoff on conflicts
    • lease freshness renewal before state persistence
  • Removes lock-time dependence on issue If-Match; state saves now occur under lock without unsupported conditional headers.
  • Keeps fail-closed behavior for mutating paths if lock or save fails.
  • Updates .github/workflows/reviewer-bot-reconcile.yml to contents: write so reconcile can participate in lock acquire/release.
  • Removes workflow-level per-item concurrency from .github/workflows/reviewer-bot.yml so serialization is lock-driven and does not rely on pending-run replacement.
  • Retains existing triage escalation behavior from earlier commits in this PR.

Fork PR Approval Flow (Current)

  1. Fork pull_request_review/submitted run is non-mutating and defers.
  2. First-hop run succeeds and publishes reconcile context.
  3. Trusted workflow_run reconcile acquires CAS lock, mutates state issue #314, and releases lock.
  4. /rectify remains available as manual fallback.

Validation

@netlify

netlify Bot commented Feb 10, 2026

Copy link
Copy Markdown

Deploy Preview for scrc-coding-guidelines ready!

Name Link
🔨 Latest commit 71d3cc6
🔍 Latest deploy log https://app.netlify.com/projects/scrc-coding-guidelines/deploys/6995d98b5057ad00095eef15
😎 Deploy Preview https://deploy-preview-405--scrc-coding-guidelines.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@felix91gr

Copy link
Copy Markdown
Collaborator

@plaindocs would you be able to review this? I don't know much about GH Actions' concurrency primitives

@plaindocs

Copy link
Copy Markdown
Collaborator

@felix91gr I'm in the same boat. But if we need a scapegoat to click "approve" and hope I'm game :-D

@felix91gr

Copy link
Copy Markdown
Collaborator

I don't know much about GH Actions' concurrency primitives

Now I do - they suck 🤣 that's why Pete has to implement a mutex and a whole data structure just to make GHA behave.

@felix91gr

Copy link
Copy Markdown
Collaborator

I thought about it and I believe we're probably better off just doing this, since the vanilla GHA behavior is so broken. It's a trade of one broken thing for one thing that... in the worst case scenario, will be broken as well.

And we have no real way to test this other than testing it in prod 🫠

Let's see how we do! 🧯

@felix91gr felix91gr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see my last comment; I think this is the best course of action even in the worst case)

@plaindocs

Copy link
Copy Markdown
Collaborator

@felix91gr agreed, although I'm not sure what the broken build is about. :-p

@felix91gr

Copy link
Copy Markdown
Collaborator

@plaindocs huh, you're right

CC @PLeVasseur there's a failing build:

Run uv run python scripts/reviewer_bot.py
   (details omitted)
Installed 56 packages in 50ms
ERROR: Unexpected status while acquiring reviewer-bot lease lock (status 400): {"message":"Bad Request","documentation_url":"https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api#use-conditional-requests-if-appropriate","errors":["Conditional request headers are not allowed in unsafe requests unless supported by the endpoint"],"status":"400"}
Event: pull_request_review, Action: submitted
Error: Process completed with exit code 1.

@PLeVasseur PLeVasseur changed the title fix: harden reviewer-bot state locking and triage escalation fix(reviewer-bot): mutation-intent routing and git-ref CAS state lock Feb 18, 2026
@PLeVasseur
PLeVasseur added this pull request to the merge queue Feb 18, 2026
Merged via the queue into Safety-Critical-Rust-Consortium:main with commit 2f51959 Feb 18, 2026
15 checks passed
@PLeVasseur
PLeVasseur deleted the fix/reviewer-bot-burst-lock-and-triage-approval branch February 18, 2026 18:59
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.

3 participants