sysadminandClaude Opus 4.8
3edeba4d7f
A durable author issue lock records the PID of the MCP session that took it.
When that process exits, assess_lock_freshness marks the lock stale (live=False)
even while its lease is still within TTL, so every ownership check that needs a
live lock fails closed -- including gitea_update_pr_branch_by_merge.
Re-taking the lock was unreachable for real work. assess_issue_lock_worktree
requires the worktree to be base-equivalent to master/main/dev, and a branch
that already carries commits is ahead of its base by construction. The existing
assess_expired_lock_reclaim affordance does not apply either: it is only
consulted once the lease has expired, so a dead PID under an unexpired lease
never reaches it. assess_own_branch_adoption already speaks of "lock recovery",
but it runs after the base-equivalence gate and so was never reached.
This adds issue_lock_recovery, a pure assessor that grants a narrow waiver only
when every element of durable ownership still matches exactly and the recorded
process is demonstrably dead: same remote/org/repo/issue, same branch (and the
worktree actually on it), same registered worktree, clean worktree, local head
== remote head == open PR head, same claimant identity/profile, no competing
live lock or lease, and no ambiguous branch claims. A malformed or incomplete
lock record can never prove ownership.
The waiver suppresses base-equivalence and nothing else. Cleanliness and every
other precondition still apply, and brand-new issue claims keep the full
requirement. A refused assessment never raises: it withholds the waiver and
lets the pre-existing guard fail closed exactly as before, so recovery can only
ever add permission, never remove a guard. Refusal reasons are appended to the
block message so a caller sees the exact missing evidence.
A completed recovery is recorded on the lock as dead_session_recovery with the
prior and replacement session PIDs, heads, and claimant, so the takeover is
auditable and never looks like an original claim. Rebinding sets the live
session PID, so the recovered lock satisfies verify_lock_for_mutation and the
downstream PR update paths.
Validation:
* new tests/test_issue_753_dead_pid_lock_recovery.py -- 33 passed
* issue-lock, adoption, store, provenance, registration, duplicate-gate,
worktree, create-issue-guard suites -- 120 passed
* MCP server, commit payloads, handoff ledger, PR ownership, branch cleanup
suites -- 286 passed
* full suite -- 3498 passed, 6 skipped, 2 failed
* the same 2 failures reproduce identically on pristine master 0425bf9a
(test_issue_702_review_findings_f1_f6 F1 recovery-before-probe and
test_reconciler_supersession_close org/repo forwarding), so they are
pre-existing and unrelated
* git diff --check clean
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01L9jMhtvTjm5EajofqqaR3F
365 lines
13 KiB
Python
365 lines
13 KiB
Python
"""Dead-session author issue-lock recovery (#753).
|
|
|
|
Covers the narrow recovery path that lets an author re-acquire a durable lock
|
|
after the MCP session that recorded it exits, plus every rejection condition
|
|
that must keep failing closed.
|
|
"""
|
|
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
import unittest
|
|
from datetime import datetime, timedelta, timezone
|
|
|
|
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
|
|
|
import issue_lock_recovery # noqa: E402
|
|
import issue_lock_store # noqa: E402
|
|
import issue_lock_worktree # noqa: E402
|
|
|
|
ISSUE = 4242
|
|
BRANCH = f"fix/issue-{ISSUE}-demo"
|
|
WORKTREE = "/scratch/wt"
|
|
HEAD = "a" * 40
|
|
OTHER_SHA = "b" * 40
|
|
IDENTITY = "example-user"
|
|
PROFILE = "example-author"
|
|
|
|
|
|
def dead_pid() -> int:
|
|
"""A PID that has certainly exited (spawned, then reaped)."""
|
|
proc = subprocess.Popen([sys.executable, "-c", "pass"])
|
|
proc.wait()
|
|
return proc.pid
|
|
|
|
|
|
def future_ts(hours: int = 4) -> str:
|
|
return (
|
|
(datetime.now(timezone.utc) + timedelta(hours=hours))
|
|
.isoformat()
|
|
.replace("+00:00", "Z")
|
|
)
|
|
|
|
|
|
def make_lock(**overrides):
|
|
lock = {
|
|
"issue_number": ISSUE,
|
|
"branch_name": BRANCH,
|
|
"worktree_path": WORKTREE,
|
|
"remote": "prgs",
|
|
"org": "ExampleOrg",
|
|
"repo": "ExampleRepo",
|
|
"session_pid": dead_pid(),
|
|
"work_lease": {
|
|
"operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE,
|
|
"issue_number": ISSUE,
|
|
"branch": BRANCH,
|
|
"worktree_path": WORKTREE,
|
|
"claimant": {"username": IDENTITY, "profile": PROFILE},
|
|
"expires_at": future_ts(),
|
|
},
|
|
}
|
|
lock.update(overrides)
|
|
return lock
|
|
|
|
|
|
def assess(lock=None, **overrides):
|
|
kwargs = {
|
|
"issue_number": ISSUE,
|
|
"branch_name": BRANCH,
|
|
"worktree_path": WORKTREE,
|
|
"remote": "prgs",
|
|
"org": "ExampleOrg",
|
|
"repo": "ExampleRepo",
|
|
"identity": IDENTITY,
|
|
"profile": PROFILE,
|
|
"current_branch": BRANCH,
|
|
"porcelain_status": "",
|
|
"head_sha": HEAD,
|
|
"remote_head_sha": HEAD,
|
|
"pr_head_sha": HEAD,
|
|
"pr_number": 99,
|
|
"competing_live_locks": [],
|
|
"candidate_branches": [BRANCH],
|
|
"current_pid": os.getpid(),
|
|
}
|
|
kwargs.update(overrides)
|
|
return issue_lock_recovery.assess_dead_session_lock_recovery(
|
|
make_lock() if lock is None else lock, **kwargs
|
|
)
|
|
|
|
|
|
class TestDeadSessionRecoveryGranted(unittest.TestCase):
|
|
def test_dead_pid_with_exact_evidence_recovers(self):
|
|
result = assess()
|
|
self.assertTrue(result["recovery_sanctioned"], result["reasons"])
|
|
self.assertEqual(result["outcome"], issue_lock_recovery.RECOVERY_SANCTIONED)
|
|
|
|
def test_recovery_still_granted_when_no_open_pr_exists(self):
|
|
# A locked branch need not have a PR yet; absence must not block.
|
|
result = assess(pr_head_sha=None, pr_number=None)
|
|
self.assertTrue(result["recovery_sanctioned"], result["reasons"])
|
|
|
|
def test_lease_expiry_is_not_required_for_recovery(self):
|
|
# The defining condition is PID death, not TTL expiry (the #601 gap).
|
|
lock = make_lock()
|
|
self.assertFalse(issue_lock_store.is_lease_expired(lock))
|
|
self.assertFalse(issue_lock_store.assess_lock_freshness(lock)["live"])
|
|
self.assertTrue(assess(lock)["recovery_sanctioned"])
|
|
|
|
|
|
class TestDeadSessionRecoveryRefused(unittest.TestCase):
|
|
def assert_refused(self, result, needle):
|
|
self.assertFalse(result["recovery_sanctioned"])
|
|
self.assertEqual(result["outcome"], issue_lock_recovery.REFUSED)
|
|
self.assertTrue(
|
|
any(needle in reason for reason in result["reasons"]),
|
|
f"expected {needle!r} in {result['reasons']}",
|
|
)
|
|
|
|
def test_live_prior_pid_refused(self):
|
|
lock = make_lock(session_pid=os.getpid(), pid=os.getpid())
|
|
# Distinct current pid so the refusal is attributable to liveness.
|
|
self.assert_refused(assess(lock, current_pid=os.getpid() + 1), "still alive")
|
|
|
|
def test_different_author_identity_refused(self):
|
|
self.assert_refused(
|
|
assess(identity="someone-else"), "does not match active identity"
|
|
)
|
|
|
|
def test_different_profile_refused(self):
|
|
self.assert_refused(
|
|
assess(profile="other-profile"), "does not match active profile"
|
|
)
|
|
|
|
def test_different_branch_refused(self):
|
|
lock = make_lock(branch_name=f"fix/issue-{ISSUE}-other")
|
|
self.assert_refused(assess(lock), "does not match requested")
|
|
|
|
def test_worktree_parked_on_another_branch_refused(self):
|
|
self.assert_refused(assess(current_branch="master"), "not the locked branch")
|
|
|
|
def test_detached_head_worktree_refused(self):
|
|
self.assert_refused(assess(current_branch=None), "detached HEAD")
|
|
|
|
def test_different_worktree_refused(self):
|
|
self.assert_refused(
|
|
assess(worktree_path="/scratch/elsewhere"), "does not match declared"
|
|
)
|
|
|
|
def test_dirty_worktree_refused(self):
|
|
self.assert_refused(
|
|
assess(porcelain_status=" M gitea_mcp_server.py\n"), "requires a clean"
|
|
)
|
|
|
|
def test_local_head_differing_from_remote_refused(self):
|
|
self.assert_refused(
|
|
assess(remote_head_sha=OTHER_SHA), "does not match remote branch head"
|
|
)
|
|
|
|
def test_pr_head_differing_refused(self):
|
|
self.assert_refused(assess(pr_head_sha=OTHER_SHA), "does not match local head")
|
|
|
|
def test_missing_remote_head_refused(self):
|
|
self.assert_refused(assess(remote_head_sha=None), "remote head")
|
|
|
|
def test_competing_live_lock_refused(self):
|
|
competing = [
|
|
{
|
|
"issue_number": ISSUE,
|
|
"branch_name": BRANCH,
|
|
"worktree_path": "/scratch/other-wt",
|
|
"pid": os.getpid(),
|
|
}
|
|
]
|
|
self.assert_refused(
|
|
assess(competing_live_locks=competing), "competing live lock"
|
|
)
|
|
|
|
def test_unrelated_live_lock_does_not_block(self):
|
|
unrelated = [
|
|
{
|
|
"issue_number": 999,
|
|
"branch_name": "fix/issue-999-unrelated",
|
|
"worktree_path": "/scratch/unrelated",
|
|
"pid": os.getpid(),
|
|
}
|
|
]
|
|
self.assertTrue(assess(competing_live_locks=unrelated)["recovery_sanctioned"])
|
|
|
|
def test_multiple_candidate_branches_refused(self):
|
|
self.assert_refused(
|
|
assess(candidate_branches=[BRANCH, f"feat/issue-{ISSUE}-rival"]),
|
|
"multiple branches claim this issue",
|
|
)
|
|
|
|
def test_repository_scope_mismatch_refused(self):
|
|
self.assert_refused(assess(repo="OtherRepo"), "does not match requested")
|
|
|
|
def test_malformed_lock_missing_worktree_refused(self):
|
|
lock = make_lock()
|
|
lock.pop("worktree_path")
|
|
self.assert_refused(assess(lock), "incomplete")
|
|
|
|
def test_malformed_lock_missing_pid_refused(self):
|
|
lock = make_lock()
|
|
lock.pop("session_pid", None)
|
|
lock.pop("pid", None)
|
|
self.assert_refused(assess(lock), "incomplete")
|
|
|
|
def test_lock_without_claimant_refused(self):
|
|
lock = make_lock()
|
|
lock["work_lease"] = dict(lock["work_lease"])
|
|
lock["work_lease"].pop("claimant")
|
|
self.assert_refused(assess(lock), "claimant identity/profile")
|
|
|
|
|
|
class TestNotACandidate(unittest.TestCase):
|
|
def test_absent_lock_is_not_a_candidate(self):
|
|
result = issue_lock_recovery.assess_dead_session_lock_recovery(
|
|
None,
|
|
issue_number=ISSUE,
|
|
branch_name=BRANCH,
|
|
worktree_path=WORKTREE,
|
|
remote="prgs",
|
|
org="ExampleOrg",
|
|
repo="ExampleRepo",
|
|
identity=IDENTITY,
|
|
profile=PROFILE,
|
|
current_branch=BRANCH,
|
|
porcelain_status="",
|
|
head_sha=HEAD,
|
|
remote_head_sha=HEAD,
|
|
)
|
|
self.assertEqual(result["outcome"], issue_lock_recovery.NO_CANDIDATE)
|
|
self.assertFalse(result["recovery_sanctioned"])
|
|
self.assertFalse(result["is_candidate"])
|
|
|
|
def test_lock_for_a_different_issue_is_not_a_candidate(self):
|
|
result = assess(make_lock(issue_number=7777))
|
|
self.assertEqual(result["outcome"], issue_lock_recovery.NO_CANDIDATE)
|
|
self.assertFalse(result["recovery_sanctioned"])
|
|
|
|
|
|
class TestWorktreeGateWaiver(unittest.TestCase):
|
|
def test_new_issue_claim_still_requires_base_equivalence(self):
|
|
result = issue_lock_worktree.assess_issue_lock_worktree(
|
|
worktree_path=WORKTREE,
|
|
current_branch=BRANCH,
|
|
porcelain_status="",
|
|
base_equivalent=False,
|
|
)
|
|
self.assertTrue(result["block"])
|
|
self.assertFalse(result["base_equivalence_waived"])
|
|
|
|
def test_sanctioned_recovery_waives_base_equivalence(self):
|
|
result = issue_lock_worktree.assess_issue_lock_worktree(
|
|
worktree_path=WORKTREE,
|
|
current_branch=BRANCH,
|
|
porcelain_status="",
|
|
base_equivalent=False,
|
|
recovery_sanctioned=True,
|
|
)
|
|
self.assertTrue(result["proven"], result["reasons"])
|
|
self.assertTrue(result["base_equivalence_waived"])
|
|
|
|
def test_recovery_never_waives_cleanliness(self):
|
|
result = issue_lock_worktree.assess_issue_lock_worktree(
|
|
worktree_path=WORKTREE,
|
|
current_branch=BRANCH,
|
|
porcelain_status=" M gitea_mcp_server.py\n",
|
|
base_equivalent=False,
|
|
recovery_sanctioned=True,
|
|
)
|
|
self.assertTrue(result["block"])
|
|
self.assertTrue(
|
|
any("tracked file edits" in reason for reason in result["reasons"])
|
|
)
|
|
|
|
def test_unproven_base_equivalence_still_blocks_without_recovery(self):
|
|
result = issue_lock_worktree.assess_issue_lock_worktree(
|
|
worktree_path=WORKTREE,
|
|
current_branch=BRANCH,
|
|
porcelain_status="",
|
|
base_equivalent=None,
|
|
)
|
|
self.assertTrue(result["block"])
|
|
|
|
|
|
class TestRecoveryRecordAndDownstream(unittest.TestCase):
|
|
def test_recovery_record_preserves_truthful_provenance(self):
|
|
assessment = assess()
|
|
prior = assessment["evidence"]["prior_session_pid"]
|
|
record = issue_lock_recovery.build_recovery_record(
|
|
assessment, recovered_at="2026-07-18T23:21:40Z"
|
|
)
|
|
self.assertTrue(record["recovered"])
|
|
self.assertEqual(record["prior_session_pid"], prior)
|
|
self.assertEqual(record["replacement_session_pid"], os.getpid())
|
|
self.assertNotEqual(
|
|
record["prior_session_pid"], record["replacement_session_pid"]
|
|
)
|
|
self.assertFalse(record["prior_pid_alive"])
|
|
self.assertEqual(record["recovered_at"], "2026-07-18T23:21:40Z")
|
|
self.assertEqual(record["branch_name"], BRANCH)
|
|
self.assertEqual(record["local_head"], HEAD)
|
|
self.assertEqual(record["identity"], IDENTITY)
|
|
self.assertTrue(record["proof"])
|
|
|
|
def test_recovery_record_carries_no_secret_material(self):
|
|
record = issue_lock_recovery.build_recovery_record(
|
|
assess(), recovered_at="2026-07-18T23:21:40Z"
|
|
)
|
|
blob = repr(record).lower()
|
|
for banned in ("token", "password", "authorization", "secret", "api_key"):
|
|
self.assertNotIn(banned, blob)
|
|
|
|
def test_recovered_lock_satisfies_update_by_merge_ownership(self):
|
|
# After recovery the lock is rebound to the live session, so the
|
|
# ownership re-check used by gitea_update_pr_branch_by_merge passes.
|
|
assessment = assess()
|
|
recovered_lock = make_lock(session_pid=os.getpid(), pid=os.getpid())
|
|
recovered_lock["dead_session_recovery"] = (
|
|
issue_lock_recovery.build_recovery_record(
|
|
assessment, recovered_at="2026-07-18T23:21:40Z"
|
|
)
|
|
)
|
|
freshness = issue_lock_store.assess_lock_freshness(recovered_lock)
|
|
self.assertTrue(freshness["live"], freshness)
|
|
|
|
verdict = issue_lock_store.verify_lock_for_mutation(
|
|
recovered_lock,
|
|
issue_number=ISSUE,
|
|
branch_name=BRANCH,
|
|
worktree_path=WORKTREE,
|
|
)
|
|
self.assertTrue(verdict["proven"], verdict["reasons"])
|
|
self.assertFalse(verdict["block"])
|
|
|
|
def test_pre_recovery_lock_fails_ownership_check(self):
|
|
# Guards against a false positive above: the dead-PID lock must fail.
|
|
verdict = issue_lock_store.verify_lock_for_mutation(
|
|
make_lock(),
|
|
issue_number=ISSUE,
|
|
branch_name=BRANCH,
|
|
worktree_path=WORKTREE,
|
|
)
|
|
self.assertTrue(verdict["block"])
|
|
self.assertTrue(any("not live" in reason for reason in verdict["reasons"]))
|
|
|
|
def test_no_manual_file_seeding_required(self):
|
|
# The whole decision is reachable from the durable record plus live
|
|
# observation; nothing is written to disk to reach a verdict.
|
|
self.assertTrue(assess()["recovery_sanctioned"])
|
|
|
|
def test_refusal_message_is_fail_closed(self):
|
|
message = issue_lock_recovery.format_recovery_refusal(
|
|
assess(porcelain_status=" M x.py\n")
|
|
)
|
|
self.assertIn("fail closed", message)
|
|
self.assertIn("recovery refused", message.lower())
|
|
|
|
|
|
if __name__ == "__main__":
|
|
unittest.main()
|