Reviewer lease handoff should expose a canonical active-foreign-session resolution path #599

New Issue

2026-07-09T15:44:04-05:00

jcwalker3 commented

2026-07-09 15:44:04 -05:00

Problem

A reviewer submit task for PR #592 correctly stopped because the instructed lease #8640 / session 23139-da018dab43ba was gone, while a different active foreign reviewer lease #8647 / session 51515-2bfb70f0685f blocked canonical acquisition.

The workflow currently has:

gitea_assess_reviewer_pr_lease — read-only presence of an active lease (#407), but no canonical next-action resolution for foreign ownership
gitea_release_reviewer_pr_lease — release only when the current session owns the lease, or when the lease is expired/reclaimable
gitea_cleanup_post_merge_moot_lease — only for already-merged/closed PRs (#515)
gitea_adopt_merger_pr_lease — merger-side adoption after approval (#536), not a reviewer-safe path for open-PR foreign leases

There is no reviewer-safe adoption/release/diagnose path for an active foreign reviewer lease on an open PR, even when the intended decision is already known and the PR is conflicted. Agents are left without a sanctioned next step and must not invent cleanup (manual lock deletion, raw API bypass, mtime manipulation, or direct MCP internals).

Related prior work (do not reopen): #407 (per-PR reviewer leases), #515 (post-merge moot cleanup), #536/#537 (merger lease adoption), #577 (lease release newest-wins), #578 (release auth identity), #510 (namespace workspace binding).

Observed state (PR #592 incident)

Field	Value
PR	#592
Head SHA	`b810e512fd9f50b7c0106305914ad79553faee12`
PR state	open, `mergeable=false` / conflicted
Existing formal reviews	none
Active lease	comment #8647, session `51515-2bfb70f0685f`, phase `claimed`, candidate_head matches, `expires_at` `2026-07-09T22:25:49Z`
Prior instructed lease	#8640 / session `23139-da018dab43ba` — gone
Env binding mismatch	reviewer env reported `branches/review-pr-538`, while target PR lease/worktree was `branches/review-pr-592-issue-590`

Required outcome

Canonical reviewer-safe diagnose path for active foreign reviewer leases (open PRs), distinct from merger adoption and post-merge moot cleanup.
Tool output must return an explicit next action, one of:
- wait — foreign lease is active and not reclaimable; do not submit
- resume_exact_owner_session — same identity/session should continue
- release_expired_lease — use sanctioned release tooling only
- operator_authorized_cleanup — only with explicit operator authority; never automatic steal
Forbidden paths remain forbidden: no manual lock deletion, raw API bypass, mtime manipulation, or direct MCP internals / _SESSION_LEASE seeding.
Runtime/worktree binding mismatch is a first-class blocker (e.g. env bound to review-pr-538 while lease targets review-pr-592-…) with a clear repair path before any formal review submit.
Tests/proofs for:
- active foreign lease (PR #592-style replacement lease)
- expired lease → sanctioned release next step
- missing instructed lease while a different active lease exists
- wrong worktree / env binding mismatch

Design constraints

Fail-closed acquisition for active foreign leases must remain intact (no silent steal).
Expired/abandoned leases may be cleared only through existing/sanctioned tools (gitea_release_reviewer_pr_lease when reclaimable, or an explicitly authorized operator path if added).
Diagnosis output must identify at minimum: blocking lease comment id, session_id, phase, candidate_head, expires_at / freshness, and the exact canonical next step.
Worktree binding mismatch must be detected before gitea_submit_pr_review / formal decision.
No path may allow a reviewer to submit a formal decision without owning a valid reviewer lease for the current PR head.

Acceptance criteria

Reviewer acquisition fail-closed behavior remains intact for active foreign leases.
Expired/abandoned leases can be cleared only through sanctioned tooling.
Tool output identifies the blocking lease comment, session_id, phase, candidate_head, expiry, and exact canonical next step.
Reviewer worktree binding mismatch is detected before review submission.
No path allows a reviewer to submit a formal decision without owning a valid reviewer lease.
Regression tests cover PR #592-style replacement lease behavior (instructed lease gone + newer foreign active lease).

Suggested implementation sketch (non-binding)

Extend or add a reviewer-facing assess tool (e.g. expand gitea_assess_reviewer_pr_lease or add gitea_diagnose_reviewer_pr_lease_handoff) that classifies:
- no lease / own active / own expired / foreign active / foreign expired / instructed-lease-missing-with-replacement
- worktree/env binding match vs mismatch
Emit structured next_action + reasons + lease identity fields.
Wire submit path to refuse with the same structured diagnosis when lease or binding is wrong.
Keep merger adoption (#536) and post-merge moot cleanup (#515) unchanged in scope.

Source / incident pointer

Durable incident: PR #592 reviewer submit blocked by foreign lease #8647 after instructed lease #8640 disappeared; worktree env mismatch review-pr-538 vs review-pr-592-issue-590.

## Problem A reviewer submit task for PR #592 correctly stopped because the instructed lease #8640 / session `23139-da018dab43ba` was gone, while a different **active foreign reviewer lease** #8647 / session `51515-2bfb70f0685f` blocked canonical acquisition. The workflow currently has: - `gitea_assess_reviewer_pr_lease` — read-only presence of *an* active lease (#407), but no **canonical next-action** resolution for foreign ownership - `gitea_release_reviewer_pr_lease` — release only when the current session owns the lease, or when the lease is expired/reclaimable - `gitea_cleanup_post_merge_moot_lease` — only for already-merged/closed PRs (#515) - `gitea_adopt_merger_pr_lease` — merger-side adoption after approval (#536), not a reviewer-safe path for open-PR foreign leases There is **no reviewer-safe adoption/release/diagnose path** for an active foreign reviewer lease on an open PR, even when the intended decision is already known and the PR is conflicted. Agents are left without a sanctioned next step and must not invent cleanup (manual lock deletion, raw API bypass, mtime manipulation, or direct MCP internals). Related prior work (do not reopen): #407 (per-PR reviewer leases), #515 (post-merge moot cleanup), #536/#537 (merger lease adoption), #577 (lease release newest-wins), #578 (release auth identity), #510 (namespace workspace binding). ## Observed state (PR #592 incident) | Field | Value | | --- | --- | | PR | #592 | | Head SHA | `b810e512fd9f50b7c0106305914ad79553faee12` | | PR state | open, `mergeable=false` / conflicted | | Existing formal reviews | none | | Active lease | comment #8647, session `51515-2bfb70f0685f`, phase `claimed`, candidate_head matches, `expires_at` `2026-07-09T22:25:49Z` | | Prior instructed lease | #8640 / session `23139-da018dab43ba` — **gone** | | Env binding mismatch | reviewer env reported `branches/review-pr-538`, while target PR lease/worktree was `branches/review-pr-592-issue-590` | ## Required outcome 1. **Canonical reviewer-safe diagnose path** for active foreign reviewer leases (open PRs), distinct from merger adoption and post-merge moot cleanup. 2. Tool output must return an **explicit next action**, one of: - `wait` — foreign lease is active and not reclaimable; do not submit - `resume_exact_owner_session` — same identity/session should continue - `release_expired_lease` — use sanctioned release tooling only - `operator_authorized_cleanup` — only with explicit operator authority; never automatic steal 3. **Forbidden paths remain forbidden**: no manual lock deletion, raw API bypass, mtime manipulation, or direct MCP internals / `_SESSION_LEASE` seeding. 4. **Runtime/worktree binding mismatch** is a first-class blocker (e.g. env bound to `review-pr-538` while lease targets `review-pr-592-…`) with a clear repair path before any formal review submit. 5. **Tests/proofs** for: - active foreign lease (PR #592-style replacement lease) - expired lease → sanctioned release next step - missing instructed lease while a different active lease exists - wrong worktree / env binding mismatch ## Design constraints - Fail-closed acquisition for active foreign leases **must remain intact** (no silent steal). - Expired/abandoned leases may be cleared **only** through existing/sanctioned tools (`gitea_release_reviewer_pr_lease` when reclaimable, or an explicitly authorized operator path if added). - Diagnosis output must identify at minimum: blocking lease comment id, `session_id`, `phase`, `candidate_head`, `expires_at` / freshness, and the exact canonical next step. - Worktree binding mismatch must be detected **before** `gitea_submit_pr_review` / formal decision. - No path may allow a reviewer to submit a formal decision without owning a valid reviewer lease for the current PR head. ## Acceptance criteria 1. Reviewer acquisition fail-closed behavior remains intact for active foreign leases. 2. Expired/abandoned leases can be cleared only through sanctioned tooling. 3. Tool output identifies the blocking lease comment, `session_id`, phase, `candidate_head`, expiry, and exact canonical next step. 4. Reviewer worktree binding mismatch is detected before review submission. 5. No path allows a reviewer to submit a formal decision without owning a valid reviewer lease. 6. Regression tests cover PR #592-style replacement lease behavior (instructed lease gone + newer foreign active lease). ## Suggested implementation sketch (non-binding) - Extend or add a reviewer-facing assess tool (e.g. expand `gitea_assess_reviewer_pr_lease` or add `gitea_diagnose_reviewer_pr_lease_handoff`) that classifies: - no lease / own active / own expired / foreign active / foreign expired / instructed-lease-missing-with-replacement - worktree/env binding match vs mismatch - Emit structured `next_action` + `reasons` + lease identity fields. - Wire submit path to refuse with the same structured diagnosis when lease or binding is wrong. - Keep merger adoption (#536) and post-merge moot cleanup (#515) unchanged in scope. ## Source / incident pointer Durable incident: PR #592 reviewer submit blocked by foreign lease #8647 after instructed lease #8640 disappeared; worktree env mismatch `review-pr-538` vs `review-pr-592-issue-590`.