Skip to content

fix(review-pr): make review lock release token-independent and TTL crash-safe #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sayt-0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(review-pr): make review lock release token-independent and TTL crash-safe #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/review-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

@derekmisler derekmisler Jul 6, 2026

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.

🤖 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 run
  • DELETE /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_TOKEN with actions: write only 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: write would break the learning loop. At minimum, documenting this trade-off in SECURITY.md would 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.

exit-code: ${{ steps.run-review.outputs.exit-code }}

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/self-review-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) || '' }}
2 changes: 1 addition & 1 deletion .github/workflows/test-e2e-reviewer.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-<repo>-<pr>-*` 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-<repo>-<pr>-*` 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-<repo>-<job>-<run_id>` 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: `<!-- docker-agent-review -->` on review comments, `<!-- docker-agent-review-reply -->` on agent replies (including mention-reply responses). **Don't change these strings** — workflows in consumer repos grep for them.
Expand Down
5 changes: 3 additions & 2 deletions SECURITY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-<repo>-<pr>-*`, 600 s
TTL) prevents concurrent reviews from racing on the same PR.
- The existing in-action **cache lock** (`pr-review-lock-<repo>-<pr>-*`, 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

Expand Down
102 changes: 75 additions & 27 deletions review-pr/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}-
Expand All @@ -170,35 +175,48 @@ 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
if: steps.lock-check.outputs.skip != 'true'
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'
Expand Down Expand Up @@ -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 }}
Expand All @@ -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

@derekmisler derekmisler Jul 6, 2026

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.

🤖 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:

  1. Run A finishes, executes DELETE at T1 (removes its lock entry)
  2. Run D starts at T2, sees no lock, acquires its own lock entry ...D-1 (T1 < T2)
  3. Run A saves this marker ...A-1-released at T3 (T2 < T3)
  4. Run E starts at T4 and restores with the prefix → cache returns A's marker (newest by creation time) → sees released file → proceeds
  5. 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 released file and save the marker in one atomic step (move touch .cache/review-lock/released into the bash block that also does the mkdir, 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_id in the released file. In the evaluate step, treat a released marker 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".

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'
Expand Down