From 51273a378b5755e5fe46db6df3cff7166948b7e1 Mon Sep 17 00:00:00 2001 From: Sayt-0 Date: Mon, 6 Jul 2026 18:05:50 +0200 Subject: [PATCH] fix(review-pr): make review lock release token-independent and TTL crash-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. --- .github/workflows/review-pr.yml | 1 + .github/workflows/self-review-pr.yml | 2 +- .github/workflows/test-e2e-reviewer.yml | 2 +- AGENTS.md | 2 +- SECURITY.md | 5 +- review-pr/action.yml | 102 +++++++++++++++++------- 6 files changed, 82 insertions(+), 32 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 50d5623..f8fc8ab 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -195,6 +195,7 @@ jobs: issues: write id-token: write checks: write + actions: write # cache delete for review-lock release cleanup; feedback artifact list/delete outputs: exit-code: ${{ steps.run-review.outputs.exit-code }} diff --git a/.github/workflows/self-review-pr.yml b/.github/workflows/self-review-pr.yml index acb0add..8b5ef83 100644 --- a/.github/workflows/self-review-pr.yml +++ b/.github/workflows/self-review-pr.yml @@ -31,6 +31,6 @@ jobs: issues: write # Create security incident issues if secrets detected checks: write # (Optional) Show review progress as a check run id-token: write # Required for OIDC authentication to AWS Secrets Manager - actions: read # Download artifacts from trigger workflow + actions: write # Cache cleanup for review-lock release and feedback artifact deletion; also covers trigger artifact download with: trigger-run-id: ${{ github.event_name == 'workflow_run' && format('{0}', github.event.workflow_run.id) || '' }} diff --git a/.github/workflows/test-e2e-reviewer.yml b/.github/workflows/test-e2e-reviewer.yml index 3949b43..3bdc00b 100644 --- a/.github/workflows/test-e2e-reviewer.yml +++ b/.github/workflows/test-e2e-reviewer.yml @@ -33,7 +33,7 @@ jobs: issues: write id-token: write checks: write - actions: read + actions: write # Cache cleanup for review-lock release and feedback artifact deletion with: pr-number: ${{ inputs.pr_number }} secrets: diff --git a/AGENTS.md b/AGENTS.md index 402cbd9..ef58b14 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -172,7 +172,7 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an ### `review-pr` action specifics -- Uses a **best-effort cache lock** (`pr-review-lock---*` cache key) to avoid concurrent reviews on the same PR. Lock TTL is 600s; the agent execution timeout is 1800s (30 min) — these are intentionally decoupled. Reviews are idempotent so the small race window is acceptable. +- Uses a **best-effort cache lock** (`pr-review-lock---*` cache key) to avoid concurrent reviews on the same PR. Completed runs release the lock by saving a `-released` marker cache entry that shadows their lock entry (cache saves work regardless of token scopes; the REST cache DELETE is best-effort cleanup only). The 3600s TTL is a fallback for crashed holders and must stay above the 2700s agent timeout so an in-flight review is never treated as stale. Reviews are idempotent so the small race window is acceptable. - **Memory persistence** uses `actions/cache` keyed by `pr-review-memory---` with prefix-based restore. The DB lives at `${{ github.workspace }}/.cache/pr-review-memory.db`. - **Feedback loop**: the `reply-to-feedback` job in `.github/workflows/review-pr.yml` (which runs the `pr-review-reply.yaml` agent) uploads a `pr-review-feedback` artifact on every reply via its "Upload feedback artifact" step. The next review run downloads all such artifacts, runs `pr-review-feedback.yaml` to call `add_memory(...)` for each, then deletes the artifacts. - **Bot reply detection** uses HTML markers: `` on review comments, `` on agent replies (including mention-reply responses). **Don't change these strings** — workflows in consumer repos grep for them. diff --git a/SECURITY.md b/SECURITY.md index 0580771..f1d1596 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -117,8 +117,9 @@ bound request frequency: gated (they are org-gated and per-PR serialized), though their replies still count toward the window. The check **fails open** so an API error never blocks a legitimate review. -- The existing in-action **cache lock** (`pr-review-lock---*`, 600 s - TTL) prevents concurrent reviews from racing on the same PR. +- The existing in-action **cache lock** (`pr-review-lock---*`, released + on completion via a cache marker, 3600 s TTL fallback for crashed runs) + prevents concurrent reviews from racing on the same PR. ## Security Modules diff --git a/review-pr/action.yml b/review-pr/action.yml index 8d365af..809a5b0 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -148,17 +148,22 @@ runs: DEFAULT_TOKEN: ${{ github.token }} # Concurrent review guard: best-effort lock using GitHub Actions cache. - # There is a narrow race window where two runs starting within seconds - # can both pass the check before either saves its lock. This is accepted - # because reviews are idempotent (duplicate reviews are harmless) and - # the window is small enough that it rarely occurs in practice. + # Lifecycle: acquire saves a lock entry; release saves a newer "released" + # marker entry under the same key prefix (restore returns the newest match, + # so the marker shadows the lock). The TTL below is only a fallback for + # runs that crashed before releasing. There is a narrow race window where + # two runs starting within seconds can both pass the check before either + # saves its lock. This is accepted because reviews are idempotent + # (duplicate reviews are harmless) and the window is small enough that it + # rarely occurs in practice. - name: Check for concurrent review id: review-lock uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 with: path: .cache/review-lock - # Exact key will never match (saves include run_id), so this always - # falls through to restore-keys prefix match — finding the most recent lock + # Exact key will never match (saves append run identifiers), so this always + # falls through to restore-keys prefix match — finding the most recent + # lock or release-marker entry key: pr-review-lock-${{ github.repository }}-${{ steps.resolve-context.outputs.pr-number }}-exact restore-keys: | pr-review-lock-${{ github.repository }}-${{ steps.resolve-context.outputs.pr-number }}- @@ -170,25 +175,37 @@ runs: # cache-hit is only true on exact key match; prefix matches via restore-keys # set cache-matched-key but leave cache-hit as false if [ -n "${{ steps.review-lock.outputs.cache-matched-key }}" ]; then - LOCK_TIME=$(cat .cache/review-lock/timestamp 2>/dev/null || echo "0") - NOW=$(date +%s) - AGE=$(( NOW - LOCK_TIME )) - if [ "$AGE" -lt 600 ]; then # 600 s lock TTL — intentionally decoupled from the review timeout (now 1800 s) - echo "⏭️ Review already in progress (started ${AGE}s ago) — skipping" - echo "skip=true" >> $GITHUB_OUTPUT - echo "skip-reason=concurrent" >> $GITHUB_OUTPUT - echo "lock-age=${AGE}" >> $GITHUB_OUTPUT - exit 0 + if [ -f .cache/review-lock/released ]; then + # Newest matching entry is a release marker — the previous review finished + echo "🔓 Previous review released its lock — proceeding" + else + LOCK_TIME=$(cat .cache/review-lock/timestamp 2>/dev/null || echo "0") + NOW=$(date +%s) + AGE=$(( NOW - LOCK_TIME )) + # TTL is a fallback for lock holders that crashed before releasing. + # It must exceed the agent timeout (2700 s) plus setup/posting + # overhead so an in-flight review is never treated as stale. + if [ "$AGE" -lt 3600 ]; then + echo "⏭️ Review already in progress (started ${AGE}s ago) — skipping" + echo "skip=true" >> $GITHUB_OUTPUT + echo "skip-reason=concurrent" >> $GITHUB_OUTPUT + echo "lock-age=${AGE}" >> $GITHUB_OUTPUT + exit 0 + fi + echo "🔓 Stale lock (${AGE}s old — holder likely crashed) — proceeding" fi - echo "🔓 Stale lock (${AGE}s old) — proceeding" fi echo "skip=false" >> $GITHUB_OUTPUT - name: Acquire review lock + id: acquire-lock if: steps.lock-check.outputs.skip != 'true' shell: bash run: | mkdir -p .cache/review-lock + # Drop any restored release marker so this run's lock entry is not + # itself read as a release by concurrent runs + rm -f .cache/review-lock/released date +%s > .cache/review-lock/timestamp - name: Save review lock @@ -196,9 +213,10 @@ runs: uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 with: path: .cache/review-lock - # Use run_id in save key so each run can save (cache keys are immutable) - # The restore step uses the fixed key with no run_id, so it matches via prefix - key: pr-review-lock-${{ github.repository }}-${{ steps.resolve-context.outputs.pr-number }}-${{ github.run_id }} + # Use run_id/run_attempt in save key so each attempt can save (cache keys + # are immutable). The restore step uses the fixed prefix, so it matches + # the newest entry regardless of the suffix. + key: pr-review-lock-${{ github.repository }}-${{ steps.resolve-context.outputs.pr-number }}-${{ github.run_id }}-${{ github.run_attempt }} - name: Ensure cache directory exists if: steps.lock-check.outputs.skip != 'true' @@ -867,7 +885,7 @@ runs: with: agent: ${{ env.ACTION_PATH }}/agents/pr-review.yaml prompt: ${{ steps.context.outputs.review_prompt }} - timeout: "2700" # 45 min — allows complex reviews to complete; lock TTL (600 s) is intentionally shorter + timeout: "2700" # 45 min — allows complex reviews to complete; must stay below the 3600 s lock TTL so an in-flight review is never treated as stale anthropic-api-key: ${{ inputs.anthropic-api-key }} openai-api-key: ${{ inputs.openai-api-key }} google-api-key: ${{ inputs.google-api-key }} @@ -885,21 +903,51 @@ runs: auth-org: ${{ inputs.auth-org }} skip-auth: ${{ inputs.skip-auth }} + # Lock release is two-fold: + # 1. Best-effort DELETE of this PR's lock cache entries to keep the cache + # tidy. This needs actions:write on the token and fails harmlessly on + # tokens without it. + # 2. Save a "released" marker entry under the same restore prefix (next + # step). Restore picks the newest match, so the marker shadows any + # surviving lock entry. Cache saves go through the runner's cache + # service token, so this works regardless of the github-token scopes. + # Cleanup runs first so a successful DELETE cannot remove the fresh marker. + # If both fail (e.g. runner crash), the lock expires via the 3600 s TTL. - name: Release review lock - if: always() && steps.lock-check.outputs.skip != 'true' - continue-on-error: true # Release failures are safe — stale locks expire via TTL (600s) + if: always() && steps.acquire-lock.outcome == 'success' + continue-on-error: true shell: bash env: GH_TOKEN: ${{ steps.resolve-token.outputs.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }} run: | - # Delete all cache entries matching this PR's lock prefix - gh api --paginate "repos/$REPO/actions/caches?key=pr-review-lock-${REPO}-${PR_NUMBER}-" \ - --jq '.actions_caches[].id' 2>/dev/null | while read -r CACHE_ID; do - gh api "repos/$REPO/actions/caches/$CACHE_ID" -X DELETE 2>/dev/null || true + CACHE_IDS=$(gh api --paginate "repos/$REPO/actions/caches?key=pr-review-lock-${REPO}-${PR_NUMBER}-" \ + --jq '.actions_caches[].id' 2>/dev/null) || CACHE_IDS="" + DELETED=0 + FAILED=0 + for CACHE_ID in $CACHE_IDS; do + if gh api "repos/$REPO/actions/caches/$CACHE_ID" -X DELETE --silent 2>/dev/null; then + DELETED=$((DELETED + 1)) + else + FAILED=$((FAILED + 1)) + fi done - echo "🔓 Released review lock for PR #$PR_NUMBER" + if [ "$FAILED" -gt 0 ]; then + echo "ℹ️ Could not delete $FAILED lock cache entries — token likely lacks actions:write; the release marker below still unlocks" + fi + + mkdir -p .cache/review-lock + touch .cache/review-lock/released + echo "🔓 Releasing review lock for PR #$PR_NUMBER (deleted $DELETED cache entries)" + + - name: Save review lock release marker + if: always() && steps.acquire-lock.outcome == 'success' + continue-on-error: true # If this fails the lock expires via the TTL fallback (3600 s) + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: .cache/review-lock + key: pr-review-lock-${{ github.repository }}-${{ steps.resolve-context.outputs.pr-number }}-${{ github.run_id }}-${{ github.run_attempt }}-released - name: Save reviewer memory if: always() && steps.lock-check.outputs.skip != 'true'