From a30a3ce4c37b2dde725301bef8b9ef8e04160088 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 22 Jul 2026 01:10:48 -0400 Subject: [PATCH] fix(lock): make the exact-owner renewal waiver survive downstream gates (#760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review #499 on PR #791. F1 — the renewal sanction was computed and then discarded twice. `assess_issue_lock_worktree` waived base-equivalence only for `recovery_sanctioned`, and `gitea_lock_issue` never passed the renewal waiver into it. A branch being renewed always carries committed work, so it is never base-equivalent, and #753 recovery refuses when the recorded PID is alive — which is the defining condition of a renewal. Every real renewal was therefore granted by the assessor and then rejected one gate later. Thread `renewal_sanctioned` into `assess_issue_lock_worktree` alongside `recovery_sanctioned`, waiving base-equivalence on the same grounds and nothing else. Cleanliness is evaluated before the waiver and is never relaxed; the assessment now reports which of the two waivers applied. The MCP-level regression then exposed a second discard point: the duplicate-work gate rejected the renewal with "open PR already covers issue" — the very PR the lock being renewed already owns. Add `owning_pr_renewal_evidence`, the mirror of `issue_lock_recovery.owning_pr_recovery_evidence` (#755), and carry it into the gate only when renewal was granted. It re-checks that the PR, local, and remote heads agree, so truncated or hand-built evidence cannot authorize an exemption. Neither waiver is caller-supplied and `gitea_lock_issue` still gains no parameter. Absolute wall-clock expiry is unchanged. No #790 heartbeat, sliding expiry, fencing-token, or shared-lifecycle behavior is introduced, and #753 recovery behavior is untouched. F2 — add tests/test_issue_760_mcp_renewal_path.py, 10 cases driving the native `gitea_lock_issue` path against a real git repository and a real durable lock: expired lease, live recorded PID, committed non-base-equivalent branch. Proves the renewal completes, records prior and replacement lease evidence, advances the generation exactly once, produces a live lock that satisfies `verify_lock_for_mutation`, and does not claim dead-session recovery. Negative companions prove a foreign claimant, foreign profile, unpublished branch, or mismatched PR head cannot use the waiver, that a dirty worktree still fails closed, and that base-equivalence still applies with no waiver at all. Tests: new MCP suite 10 passed; combined #760 suites 50 passed; lock/lease regression set 200 passed with 2 subtests; full suite 4240 passed, 11 failed, 6 skipped, 493 subtests passed — the same 11 pre-existing failures as the master baseline at 3d0c13fa, no new failures. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ti8deB36iWcjHmE9cuxour --- gitea_mcp_server.py | 18 ++ issue_lock_renewal.py | 56 ++++ issue_lock_worktree.py | 28 +- tests/test_issue_760_mcp_renewal_path.py | 342 +++++++++++++++++++++++ 4 files changed, 441 insertions(+), 3 deletions(-) create mode 100644 tests/test_issue_760_mcp_renewal_path.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index d7ebb76..0ac7b4b 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4103,6 +4103,14 @@ def gitea_lock_issue( if recovery_sanctioned else None ) + # #760: a sanctioned renewal owns its open PR for the same reason, so it + # needs the same exemption. Without it the duplicate-work gate rejects every + # renewal with "open PR already covers issue", which is the PR the lock + # being renewed already owns. Withheld unless renewal was granted. + if recovered_owning_pr is None and renewal_sanctioned: + recovered_owning_pr = issue_lock_renewal.owning_pr_renewal_evidence( + renewal_assessment + ) lock_assessment = issue_lock_worktree.assess_issue_lock_worktree( worktree_path=resolved_worktree, current_branch=git_state.get("current_branch"), @@ -4111,6 +4119,10 @@ def gitea_lock_issue( inspected_git_root=git_state.get("inspected_git_root"), base_branch=git_state.get("base_branch"), recovery_sanctioned=recovery_sanctioned, + # #760: without this the renewal waiver was computed and then discarded + # here — the exact-owner branch always carries commits, so it can never + # be base-equivalent, and every real renewal failed at this gate. + renewal_sanctioned=renewal_sanctioned, ) if lock_assessment["block"]: reasons = list(lock_assessment.get("reasons") or []) @@ -4120,6 +4132,12 @@ def gitea_lock_issue( reasons.append( issue_lock_recovery.format_recovery_refusal(recovery_assessment) ) + # #760: same courtesy for a refused renewal, so an exact owner blocked + # at this gate sees which piece of ownership evidence was missing. + if renewal_assessment and renewal_assessment.get("is_candidate"): + reasons.append( + issue_lock_renewal.format_renewal_refusal(renewal_assessment) + ) raise RuntimeError( issue_lock_worktree.format_issue_lock_worktree_error( {**lock_assessment, "reasons": reasons} diff --git a/issue_lock_renewal.py b/issue_lock_renewal.py index 14d6f99..ec8a436 100644 --- a/issue_lock_renewal.py +++ b/issue_lock_renewal.py @@ -380,6 +380,62 @@ def assess_exact_owner_lease_renewal( ) +def owning_pr_renewal_evidence( + assessment: Mapping[str, Any] | None, +) -> dict[str, Any] | None: + """Server-derived proof of the open PR a sanctioned renewal already owns. + + The mirror of ``issue_lock_recovery.owning_pr_recovery_evidence`` (#755) for + the renewal disposition. An exact-owner renewal of a published branch is, by + construction, renewal of work that already has an open PR — so the + duplicate-work gate's linked-open-PR blocker would otherwise discard every + sanctioned renewal, exactly as it once discarded every sanctioned recovery. + + Returns ``None`` unless renewal was actually granted and the evidence names + one owning PR whose head agrees with both the local and remote heads the + assessor accepted. Nothing is caller-supplied: every field is copied from + evidence built out of durable lock state plus live git/Gitea observation. + + Renewal has no descendant case — it requires the local, remote, and PR heads + to be equal — so there is only one head to report. + """ + if not isinstance(assessment, Mapping): + return None + if assessment.get("outcome") != RENEWAL_SANCTIONED: + return None + if not assessment.get("renewal_sanctioned"): + return None + + evidence = assessment.get("evidence") or {} + branch_name = _text(evidence.get("branch_name")) + pr_head = _text(evidence.get("pr_head_sha")) + local_head = _text(evidence.get("head_sha")) + remote_head = _text(evidence.get("remote_head_sha")) + raw_pr_number = evidence.get("pr_number") + + if raw_pr_number is None or not branch_name or not pr_head: + return None + # The assessor already required these to agree. Re-check, so a truncated or + # hand-built evidence map can never authorize an exemption. + if pr_head != local_head or pr_head != remote_head: + return None + try: + pr_number = int(raw_pr_number) + issue_number = int(evidence.get("issue_number")) + except (TypeError, ValueError): + return None + + return { + "issue_number": issue_number, + "pr_number": pr_number, + "branch_name": branch_name, + "head_sha": pr_head, + "recorded_head": pr_head, + "accepted_head": pr_head, + "head_relation": "equal", + } + + def build_renewal_record( assessment: Mapping[str, Any] | None, *, diff --git a/issue_lock_worktree.py b/issue_lock_worktree.py index e84b8d8..de52ff7 100644 --- a/issue_lock_worktree.py +++ b/issue_lock_worktree.py @@ -285,6 +285,7 @@ def assess_issue_lock_worktree( base_branch: str | None = None, base_branches: frozenset[str] | None = None, recovery_sanctioned: bool = False, + renewal_sanctioned: bool = False, ) -> dict: """Fail closed when lock preconditions are not met on the declared worktree. @@ -296,6 +297,19 @@ def assess_issue_lock_worktree( by construction and could never satisfy it. Every other precondition — notably worktree cleanliness — still applies unchanged, and brand-new issue claims keep the full base-equivalence requirement. + + ``renewal_sanctioned`` waives base-equivalence on exactly the same grounds + for the other proven-ownership case (#760): ``issue_lock_renewal`` has shown + that an *expired* lease is being renewed by its exact recorded owner — same + remote, org, repo, issue, operation, branch, realpath-normalized worktree, + claimant username and profile — with the local head matching the remote head + and any owning PR head. Such a branch carries committed work for the same + reason a recovered one does, so it can never be base-equivalent either. + + Both waivers relax this one requirement and nothing else. Neither is + caller-supplied: each is computed server-side from durable lock state plus + live observation. With both False every precondition applies exactly as + before. """ bases = base_branches or BASE_BRANCHES reasons: list[str] = [] @@ -314,9 +328,12 @@ def assess_issue_lock_worktree( f"(dirty files: {', '.join(dirty_files)})" ) - if recovery_sanctioned: + if recovery_sanctioned or renewal_sanctioned: # Base-equivalence intentionally not evaluated: ownership was proven - # against the durable lock record instead (#753). + # against the durable lock record instead — by dead-session recovery + # (#753) or by exact-owner renewal of an expired lease (#760). Every + # other precondition above and below still applies; cleanliness in + # particular is checked before this branch and is never waived. pass elif base_equivalent is False: reasons.append( @@ -347,6 +364,7 @@ def assess_issue_lock_worktree( base_branch=base_branch, base_equivalent=base_equivalent, recovery_sanctioned=recovery_sanctioned, + renewal_sanctioned=renewal_sanctioned, ) @@ -406,6 +424,7 @@ def _assessment( base_branch: str | None = None, base_equivalent: bool | None = None, recovery_sanctioned: bool = False, + renewal_sanctioned: bool = False, ) -> dict: return { "proven": proven, @@ -418,7 +437,10 @@ def _assessment( "base_branch": base_branch, "base_equivalent": base_equivalent, "recovery_sanctioned": recovery_sanctioned, - "base_equivalence_waived": bool(recovery_sanctioned), + "renewal_sanctioned": renewal_sanctioned, + # Either proven-ownership waiver relaxes base-equivalence; the two are + # reported separately so an audit can tell which one applied. + "base_equivalence_waived": bool(recovery_sanctioned or renewal_sanctioned), } diff --git a/tests/test_issue_760_mcp_renewal_path.py b/tests/test_issue_760_mcp_renewal_path.py new file mode 100644 index 0000000..f33b84a --- /dev/null +++ b/tests/test_issue_760_mcp_renewal_path.py @@ -0,0 +1,342 @@ +"""MCP-level exact-owner lease renewal through ``gitea_lock_issue`` (#760). + +The unit suite in ``test_issue_760_exact_owner_lease_renewal`` proves the +renewal *disposition*. It cannot prove the disposition survives the rest of the +tool, and it did not: the waiver was computed and then discarded before +``assess_issue_lock_worktree``, so every real renewal still failed on +base-equivalence. A branch being renewed always carries committed work, so it is +never base-equivalent by construction — exactly the argument #753 already makes +for recovery. + +These tests drive the public tool end to end against a real git repository and a +real durable lock file, composing every gate in the production order. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from datetime import datetime, timedelta, timezone +from unittest.mock import patch + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +from mutation_profile_fixture import shared_mutation_env # noqa: E402 + +import issue_lock_provenance # noqa: E402 +import issue_lock_store # noqa: E402 +import mcp_server # noqa: E402 + +ISSUE = 9760 +BRANCH = f"fix/issue-{ISSUE}-renewal-mcp" +IDENTITY = "example-user" +PROFILE = "test-author-prgs" +ORG = "Scaled-Tech-Consulting" +REPO = "Gitea-Tools" + + +def _past_ts(hours: int = 1) -> str: + return ( + (datetime.now(timezone.utc) - timedelta(hours=hours)) + .isoformat() + .replace("+00:00", "Z") + ) + + +class _RenewalMcpBase(unittest.TestCase): + """Real git repo + durable expired lock owned by a live PID. + + The recorded PID is ``os.getpid()`` — unambiguously alive. That is the whole + point of #760: the PID belongs to the long-lived MCP daemon, so its liveness + says nothing about whether the authoring task still holds the work. + """ + + def setUp(self): + self.lock_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.lock_dir.cleanup) + self.repo = tempfile.mkdtemp(prefix="issue760-mcp-") + self.addCleanup(lambda: subprocess.run(["rm", "-rf", self.repo], check=False)) + self._init_worktree() + self.remotes = patch.dict( + mcp_server.REMOTES, + {"prgs": {"host": "gitea.prgs.cc", "org": ORG, "repo": REPO}}, + ) + self.remotes.start() + self.addCleanup(patch.stopall) + mcp_server._IDENTITY_CACHE.clear() + + def _git(self, *args): + return subprocess.run( + ["git", "-C", self.repo, *args], + capture_output=True, + text=True, + check=True, + ) + + def _init_worktree(self): + self._git("init", "-q", "-b", "master") + self._git("config", "user.email", "test@example.com") + self._git("config", "user.name", "Test") + with open(os.path.join(self.repo, "seed.txt"), "w") as fh: + fh.write("seed\n") + self._git("add", "seed.txt") + self._git("commit", "-q", "-m", "seed") + self.base_sha = self._git("rev-parse", "HEAD").stdout.strip() + # The branch carries committed work, so it is NOT base-equivalent. + self._git("checkout", "-q", "-b", BRANCH) + with open(os.path.join(self.repo, "work.txt"), "w") as fh: + fh.write("author work\n") + self._git("add", "work.txt") + self._git("commit", "-q", "-m", "author work") + self.head_sha = self._git("rev-parse", "HEAD").stdout.strip() + self.worktree = os.path.realpath(self.repo) + + def write_expired_lock(self, **overrides): + path = issue_lock_store.lock_file_path( + remote="prgs", + org=ORG, + repo=REPO, + issue_number=ISSUE, + lock_dir=self.lock_dir.name, + ) + claimant = {"username": IDENTITY, "profile": PROFILE} + pid = overrides.pop("session_pid", os.getpid()) + overrides.pop("pid", None) + lease_overrides = overrides.pop("work_lease", {}) + data = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "remote": "prgs", + "org": ORG, + "repo": REPO, + "worktree_path": self.worktree, + "session_pid": pid, + "pid": pid, + "lock_generation": 3, + "work_lease": { + "operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE, + "issue_number": ISSUE, + "pr_number": None, + "branch": BRANCH, + "worktree_path": self.worktree, + "claimant": claimant, + "created_at": _past_ts(5), + "last_heartbeat_at": _past_ts(5), + "expires_at": _past_ts(), # already expired + }, + "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( + tool="gitea_lock_issue", + claimant=claimant, + ), + } + data["work_lease"].update(lease_overrides) + data.update(overrides) + data["session_pid"] = pid + data["pid"] = pid + data["lock_file_path"] = path + issue_lock_store.save_lock_file(path, data) + return path + + def _tool_env(self): + env = shared_mutation_env( + PROFILE, + include_example_repo=True, + GITEA_ISSUE_LOCK_DIR=self.lock_dir.name, + ) + env["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + return env + + def _git_state(self, *, porcelain="", branch=BRANCH, head=None): + return { + "current_branch": branch, + "porcelain_status": porcelain, + # The decisive fact: a branch carrying work is never base-equivalent. + "base_equivalent": False, + "head_sha": head or self.head_sha, + "inspected_git_root": self.worktree, + "base_branch": "master", + } + + def run_lock_issue( + self, + *, + branch_entries=None, + open_prs=None, + git_state=None, + identity=IDENTITY, + profile=PROFILE, + ): + """Drive the public tool for the published exact-owner renewal shape.""" + if branch_entries is None: + branch_entries = [{"name": BRANCH, "commit": {"id": self.head_sha}}] + if open_prs is None: + open_prs = [{"number": 4242, "head": {"ref": BRANCH, "sha": self.head_sha}}] + if git_state is None: + git_state = self._git_state() + env = self._tool_env() + with patch( + "mcp_server.api_get_all", return_value=list(branch_entries) + ), patch( + "mcp_server._list_open_pulls", return_value=list(open_prs) + ), patch( + "mcp_server.get_auth_header", return_value="token x" + ), patch( + "mcp_server._work_lease_claimant", + return_value={"username": identity, "profile": profile}, + ), patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value=git_state, + ), patch( + "mcp_server.issue_duplicate_context_fetcher", + side_effect=lambda h, o, r, auth, issue_number: ( + list(open_prs), + [b.get("name") for b in branch_entries if isinstance(b, dict)], + {"status": "not_claimed"}, + ), + ), patch.dict(os.environ, env, clear=True): + os.environ["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + return mcp_server.gitea_lock_issue( + issue_number=ISSUE, + branch_name=BRANCH, + remote="prgs", + worktree_path=self.worktree, + ) + + +class TestRenewalReachableThroughTool(_RenewalMcpBase): + """F1: the sanctioned renewal must survive every downstream gate.""" + + def test_expired_lease_live_pid_exact_owner_renews_through_the_tool(self): + prior = issue_lock_store.read_lock_file(self.write_expired_lock()) + self.assertTrue(issue_lock_store.is_lease_expired(prior)) + self.assertTrue(issue_lock_store.is_process_alive(prior["session_pid"])) + + result = self.run_lock_issue() + + self.assertTrue(result["success"], result) + self.assertEqual(result["issue_number"], ISSUE) + self.assertEqual(result["branch_name"], BRANCH) + # The renewal is reported natively, so no lock-file inspection is needed. + self.assertIn("lease_renewal", result) + self.assertTrue(result["lease_renewal"]["renewed"]) + self.assertIn("Renewed the expired", result["message"]) + + def test_renewed_lock_records_prior_and_replacement_evidence(self): + prior = issue_lock_store.read_lock_file(self.write_expired_lock()) + prior_expiry = prior["work_lease"]["expires_at"] + prior_generation = issue_lock_store.lock_generation(prior) + + result = self.run_lock_issue() + written = issue_lock_store.read_lock_file(result["lock_file_path"]) + + renewal = written["lease_renewal"] + self.assertTrue(renewal["renewed"]) + self.assertEqual(renewal["prior_pid"], prior["session_pid"]) + self.assertTrue(renewal["prior_pid_alive"]) + self.assertEqual(renewal["prior_expires_at"], prior_expiry) + self.assertEqual(renewal["identity"], IDENTITY) + self.assertEqual(renewal["profile"], PROFILE) + self.assertEqual(renewal["head_sha"], self.head_sha) + self.assertTrue(renewal["proof"]) + # New expiry is a fresh absolute stamp, later than the one it replaced. + self.assertEqual(renewal["new_expires_at"], written["work_lease"]["expires_at"]) + self.assertGreater(renewal["new_expires_at"], prior_expiry) + # Compare-and-swap advanced the generation exactly once. + self.assertEqual( + issue_lock_store.lock_generation(written), prior_generation + 1 + ) + + def test_renewed_lock_is_live_and_satisfies_mutation_ownership(self): + self.write_expired_lock() + result = self.run_lock_issue() + written = issue_lock_store.read_lock_file(result["lock_file_path"]) + + self.assertTrue(issue_lock_store.assess_lock_freshness(written)["live"]) + verdict = issue_lock_store.verify_lock_for_mutation( + written, + issue_number=ISSUE, + branch_name=BRANCH, + worktree_path=self.worktree, + ) + self.assertTrue(verdict["proven"], verdict) + self.assertFalse(verdict["block"]) + + def test_recovery_record_is_not_written_for_a_live_owner_renewal(self): + """#753 recovery must not be claimed when the recorded PID is alive.""" + self.write_expired_lock() + result = self.run_lock_issue() + written = issue_lock_store.read_lock_file(result["lock_file_path"]) + self.assertNotIn("dead_session_recovery", written) + + +class TestRenewalWaiverIsNarrow(_RenewalMcpBase): + """The waiver relaxes base-equivalence and nothing else.""" + + def test_dirty_worktree_still_blocks_a_would_be_renewal(self): + """Cleanliness is never waived; the renewal assessor refuses first. + + A dirty worktree makes the renewal refuse, so no waiver is issued and + the lease-conflict gate fails closed ahead of the worktree gate. The + refusal names the uncommitted files, so the owner still learns why. + """ + self.write_expired_lock() + with self.assertRaises(Exception) as ctx: + self.run_lock_issue( + git_state=self._git_state(porcelain=" M gitea_mcp_server.py\n") + ) + message = str(ctx.exception) + self.assertIn("Recovery review is required before takeover", message) + self.assertIn("worktree has uncommitted tracked changes", message) + self.assertIn("gitea_mcp_server.py", message) + + def test_foreign_claimant_cannot_use_the_waiver(self): + """A near-match owner gets no renewal and no base-equivalence waiver.""" + self.write_expired_lock() + with self.assertRaises(Exception) as ctx: + self.run_lock_issue(identity="someone-else") + message = str(ctx.exception) + self.assertIn("Recovery review is required before takeover", message) + # The refusal names the missing ownership evidence (#760 diagnostics). + self.assertIn("does not match active identity", message) + + def test_foreign_profile_cannot_use_the_waiver(self): + self.write_expired_lock() + with self.assertRaises(Exception) as ctx: + self.run_lock_issue(profile="other-author") + self.assertIn( + "Recovery review is required before takeover", str(ctx.exception) + ) + + def test_unpublished_branch_cannot_use_the_waiver(self): + """No remote head to agree with, so exact-owner renewal is refused.""" + self.write_expired_lock() + with self.assertRaises(Exception) as ctx: + self.run_lock_issue(branch_entries=[], open_prs=[]) + self.assertIn( + "Recovery review is required before takeover", str(ctx.exception) + ) + + def test_pr_head_mismatch_cannot_use_the_waiver(self): + self.write_expired_lock() + other = "9" * 40 + with self.assertRaises(Exception) as ctx: + self.run_lock_issue( + open_prs=[{"number": 4242, "head": {"ref": BRANCH, "sha": other}}] + ) + self.assertIn( + "Recovery review is required before takeover", str(ctx.exception) + ) + + def test_non_base_equivalent_branch_still_blocks_without_any_waiver(self): + """No durable lock at all: the ordinary base-equivalence rule applies.""" + with self.assertRaises(Exception) as ctx: + self.run_lock_issue() + self.assertIn("must be base-equivalent", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main()