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

[Sayt-0]

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

Direction	Cause	Effect
Fail-open	Lock TTL (600 s) < agent timeout (2700 s)	Any request between minute 10 and 45 of a running review saw a "stale" lock and started a duplicate concurrent review
Fail-closed	Release used DELETE /actions/caches, which requires actions: write; the review job token had no actions scope, so the delete failed silently	Every request within 600 s of a completed review was skipped silently (green run, no message on the PR)

Fix

Release no longer depends on token scopes: a completed run saves a -released marker cache entry under the same key prefix. Cache saves go through the runner's cache-service token, so they work regardless of github.token scopes. 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).

acquire:  save  pr-review-lock-<repo>-<pr>-<run_id>-<run_attempt>            (timestamp)
release:  save  pr-review-lock-<repo>-<pr>-<run_id>-<run_attempt>-released   (timestamp + released file)
check:    restore newest entry matching prefix pr-review-lock-<repo>-<pr>-
            |- has "released" file       -> proceed (previous review finished)
            |- timestamp < 3600 s old    -> skip (review genuinely in progress)
            '- older / missing timestamp -> proceed (holder crashed, TTL fallback)

Hardening details:

• The acquire step deletes any restored released file, so a run's own lock entry cannot be misread as a release by concurrent runs.
• Release steps are gated on steps.acquire-lock.outcome == 'success', so a run that never took the lock cannot release another run's lock.
• Save keys include run_attempt, so re-run attempts can save their own entries (cache keys are immutable).
• Failed cache deletes are now logged instead of fully silenced.

Changes

File	Change
review-pr/action.yml	Lock evaluate/acquire/release logic, TTL 600 s to 3600 s, release-marker step, updated comments
.github/workflows/review-pr.yml	actions: write on the review job (also fixes the silently failing feedback-artifact list/delete calls)
.github/workflows/self-review-pr.yml, .github/workflows/test-e2e-reviewer.yml	Callers grant actions: write since the called job now requests it
AGENTS.md, SECURITY.md	Lock documentation updated (previous text also claimed an outdated 1800 s agent timeout)

Validation

• actionlint passes (only pre-existing warning about unbuilt dist/)
• All modified YAML parses
• Lock bash blocks exercised under bash -e -o pipefail with a stubbed gh across 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 written
• Existing tests/*.sh pass

Compatibility

• Consumers calling the reusable workflow must grant actions: write at the call site, as the review-pr/README.md and SKILL templates already instruct. Non-compliant callers get an explicit permissions validation error instead of silent lock breakage.
• Legacy lock entries saved by the old code are handled by the TTL branch; a marker entry read by old code behaves like the old 600 s dead zone at worst. No duplicate reviews can result from version mixing during rollout.

[docker-agent]

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 -released marker 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-check and steps.acquire-lock correctly match the id: lock-check and id: acquire-lock fields added in this diff. Hyphens in step IDs are valid in GitHub Actions expressions.
• rm -f .cache/review-lock/released in acquire: Correctly prevents the newly-saved lock entry from being misread as a release by concurrent runs. The released file 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_IDS loop: Empty-string expansion in bash produces zero iterations — safe when gh api returns no results or fails.
• Restore-keys prefix match: The -released suffix 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]

lock release logic looks solid. the marker-shadows-lock pattern is clean and the acquire-lock gate on release is a nice hardening touch. ci is all green, approving.