fix(review-pr): make review lock release token-independent and TTL crash-safe#49
fix(review-pr): make review lock release token-independent and TTL crash-safe#49Sayt-0 wants to merge 1 commit into
Conversation
…ash-safe The concurrent-review lock was broken in both directions: - TTL (600s) was shorter than the agent timeout (2700s), so requests arriving mid-review saw a stale lock and started duplicate reviews. - Release relied on DELETE /actions/caches, which needs actions:write; the review job token lacked it, so the delete failed silently and every request within 600s of a completed review was silently skipped. Release now saves a "-released" marker cache entry that shadows the lock entry on prefix restore (cache saves use the runner cache-service token, independent of github.token scopes). The REST DELETE remains as logged, best-effort cleanup. The TTL becomes a crash-only fallback raised to 3600s, above the agent timeout. The review job now requests actions: write, which callers already grant per the documented setup.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The lock lifecycle logic is sound. The new released-marker mechanism correctly addresses both failure modes described in the PR:
- Fail-open (duplicate reviews): TTL raised to 3600 s — safely above the 2700 s agent timeout plus overhead, ensuring an in-flight review is never treated as stale.
- Fail-closed (silent skips): Release no longer depends on
actions:write— the cache-service token writes the-releasedmarker entry, which the restore step picks up as the newest prefix match on the next run.
Specific points checked:
- Step ID cross-references:
steps.lock-checkandsteps.acquire-lockcorrectly match theid: lock-checkandid: acquire-lockfields added in this diff. Hyphens in step IDs are valid in GitHub Actions expressions. rm -f .cache/review-lock/releasedin acquire: Correctly prevents the newly-saved lock entry from being misread as a release by concurrent runs. Thereleasedfile is only removed from the runner's local disk; the previous release marker still exists in the cache service and remains restorable by other runs.for CACHE_ID in $CACHE_IDSloop: Empty-string expansion in bash produces zero iterations — safe whengh apireturns no results or fails.- Restore-keys prefix match: The
-releasedsuffix on the marker key still starts with the restore prefix (pr-review-lock-<repo>-<pr>-), so the restore step correctly picks it up as the newest entry after a completed review. - DELETE-before-save ordering: A successful DELETE followed by a failed marker save leaves no lock entry in cache, so a concurrent run finds no match and proceeds to acquire — correct behavior.
- Re-run TTL trade-off: The 3600 s TTL means a re-run after a crash must wait up to 60 min (vs. 10 min with the old 600 s TTL). This is an intentional, explicitly documented design decision: the TTL must exceed the 2700 s agent timeout to prevent a long-running review from being treated as stale. The
run_attempt-based save key allows attempt N to save its own lock once it proceeds past the check.
derekmisler
left a comment
derekmisler
left a comment
There was a problem hiding this comment.
🤖 Automated PR Review — this comment was posted by the pr-reviewer bot, not by a human or the implementer agent
Assessment: 🟡 NEEDS ATTENTION
The core design is sound: the marker-based release mechanism cleanly sidesteps the actions:write dependency for the lock itself, the run_attempt key addition is correct, the steps.acquire-lock.outcome == 'success' guard is an improvement over the prior skip != 'true' check, and raising the TTL to 3600 s (safely above the 2700 s agent timeout) is the right call. Two medium-severity issues found in the new code:
- Race window in the two-step release — the DELETE→marker-save sequence introduces a new release-phase race not present in the original design (details below on the marker step).
actions: writegrants more than needed — the permission covers workflow-run cancellation/deletion in addition to cache cleanup and artifact management; the artifact delete use-case is a real functional dependency but the blast radius on fork-PRworkflow_runjobs is worth noting (details below onreview-pr.yml).
| fi | ||
|
|
||
| mkdir -p .cache/review-lock | ||
| touch .cache/review-lock/released |
There was a problem hiding this comment.
🤖 Automated PR Review — this comment was posted by the pr-reviewer bot, not by a human or the implementer agent
[MEDIUM] Release-phase race: marker can shadow a concurrently acquired lock
The two-step release (DELETE in "Release review lock" → cache save here) introduces a new race window that the original single-step release did not have:
- Run A finishes, executes DELETE at T1 (removes its lock entry)
- Run D starts at T2, sees no lock, acquires its own lock entry
...D-1(T1 < T2) - Run A saves this marker
...A-1-releasedat T3 (T2 < T3) - Run E starts at T4 and restores with the prefix → cache returns A's marker (newest by creation time) → sees
releasedfile → proceeds - Run D and Run E are now concurrent — duplicate review
The original code (single DELETE step) only had a start-time race (before any lock existed). This adds a release-phase race: the window between A's DELETE completing and A's marker being saved is a gap where a new lock can be established and then immediately shadowed. Reviews are idempotent so the impact is a redundant PR comment rather than data corruption, but it is a genuine regression in the concurrency guarantees.
Possible mitigations:
- Write the
releasedfile and save the marker in one atomic step (movetouch .cache/review-lock/releasedinto the bash block that also does themkdir, then save the marker before the DELETE). With this order: marker is saved first → subsequent restores always see marker → then DELETE cleans up old entries; the fresh marker is never removed because it was saved after the list was captured. - Alternatively, store the releasing
run_idin thereleasedfile. In the evaluate step, treat areleasedmarker as "proceed" only if the releasing run's lock entry (...-{releasing_run_id}-{run_attempt}) is also found (or the entry is absent). This lets concurrent readers distinguish "A released" from "D locked + A released later".
| id-token: write | ||
| checks: write | ||
| actions: write # cache delete for review-lock release cleanup; feedback artifact list/delete | ||
| outputs: |
There was a problem hiding this comment.
🤖 Automated PR Review — this comment was posted by the pr-reviewer bot, not by a human or the implementer agent
[MEDIUM] actions: write grants workflow-cancellation scope beyond cache management
actions: write on github.token covers more than cache and artifact management. It also grants:
POST /repos/{owner}/{repo}/actions/runs/{run_id}/cancel— cancel any workflow runDELETE /repos/{owner}/{repo}/actions/runs/{run_id}— delete workflow run history
actions/cache/save (the actual lock release mechanism — the marker step) uses the runner's built-in ACTIONS_RUNTIME_TOKEN (the cache service token), not github.token. So actions: write on github.token is NOT required for the cache marker to work.
The permission is needed here for two things: (a) the best-effort gh api DELETE cache-entry cleanup (already stated to "fail harmlessly" without it) and (b) the feedback artifact list/delete in the learning loop — that's a real functional dependency that can't simply be dropped.
The concern is in self-review-pr.yml specifically: that workflow uses a workflow_run trigger to process untrusted fork-PR content (PR titles, bodies, code diffs) through the LLM agent, and GH_TOKEN is exposed to the agent. With actions: write, a successful prompt-injection attack gains the ability to cancel or delete any workflow run in the repo. The attack requires compromising the agent, which is behind sanitization, but the blast radius is materially larger than actions: read.
Possible mitigations without breaking feedback artifact deletion:
- Split permissions so artifact delete uses a separate step with a scoped token (e.g. use
GITHUB_TOKENwithactions: writeonly in a post-step that runs after the agent exits, not during it). - Accept the risk with a comment documenting it explicitly — the feedback artifact dependency is real and removing
actions: writewould break the learning loop. At minimum, documenting this trade-off inSECURITY.mdwould be valuable.
This also applies to self-review-pr.yml (line 36) and test-e2e-reviewer.yml (line 37), which call this reusable workflow with actions: write at the call site.
3 participants
Summary
Fixes bug B4: the concurrent-review lock was broken in both directions, producing duplicate reviews during long runs and silent skips ("review does not start") after short runs.
Root causes
DELETE /actions/caches, which requiresactions: write; the review job token had noactionsscope, so the delete failed silentlyFix
Release no longer depends on token scopes: a completed run saves a
-releasedmarker cache entry under the same key prefix. Cache saves go through the runner's cache-service token, so they work regardless ofgithub.tokenscopes. The REST DELETE remains as logged, best-effort cleanup. The TTL becomes a crash-only fallback, raised to 3600 s (above the 2700 s agent timeout plus setup/posting overhead, and above the 50 min job cap).Hardening details:
releasedfile, so a run's own lock entry cannot be misread as a release by concurrent runs.steps.acquire-lock.outcome == 'success', so a run that never took the lock cannot release another run's lock.run_attempt, so re-run attempts can save their own entries (cache keys are immutable).Changes
review-pr/action.yml.github/workflows/review-pr.ymlactions: writeon the review job (also fixes the silently failing feedback-artifact list/delete calls).github/workflows/self-review-pr.yml,.github/workflows/test-e2e-reviewer.ymlactions: writesince the called job now requests itAGENTS.md,SECURITY.mdValidation
actionlintpasses (only pre-existing warning about unbuiltdist/)bash -e -o pipefailwith a stubbedghacross 10 scenarios: fresh, stale, released and corrupt lock states, plus release with list-fail and delete-fail tokens; all pass and the marker is always writtentests/*.shpassCompatibility
actions: writeat the call site, as thereview-pr/README.mdand SKILL templates already instruct. Non-compliant callers get an explicit permissions validation error instead of silent lock breakage.