fix(gate): complete owning-PR renewal enforcement coverage (#945)
Addresses review 623 on PR #946 (B1 blocker, F2 medium, F3 minor).
B1 - the wiring this branch exists to install had no regression coverage.
The existing suite exercised owning_pr_renewal_from_lock,
_owning_pr_continuation_from_lock and the duplicate gate directly, but never
drove an enforcement path, so reverting any of the three call sites left the
whole repository green. Add tests/test_issue_945_enforcement_path_wiring.py,
which drives the real mcp_server._enforce_locked_issue_duplicate_recheck (the
shared recheck behind gitea_commit_files and gitea_create_pr),
mcp_server.gitea_assess_work_issue_duplicate and
mcp_server._prove_author_ownership_for_pr against a renewal-bearing lock, and
asserts each grants the exemption. Reverting the commit/create-PR recheck to
the recovery-only rebuild now fails 8 tests and 4 subtests; reverting the
assessor or the push prover fails 2 each. The suite also keeps the fail-closed
matrix on the real paths: an open PR alone, a second PR, a different PR,
branch, issue or head, identity and profile mismatch, ungranted and malformed
renewal blocks, and sequential-task non-inheritance are all still refused.
F2 - the claimant check compares lease_renewal.identity/profile against the
claimant recorded on the same lock file. Both sides are server-written fields
of one document, so it is an internal-consistency check, not verification of
the live authenticated caller. Correct the docstring and the inline comment to
say so, and document the binding that actually prevents cross-session reuse:
the enforcement paths load the lock through _load_existing_issue_lock() with no
issue coordinates, which resolves issue_lock_store.read_session_issue_lock() to
the session pointer at session-{os.getpid()}.json, so lock selection is scoped
to the operating-system process. Its limits are stated too - per-process rather
than per-authenticated-user, silent on locks reached by explicit coordinates,
and silent on two roles sharing one process. Live identity and profile stay
enforced by the mutation-authority and profile gates, not by this rebuild.
F3 - _owning_pr_continuation_from_lock previously fell through to renewal when
a dead_session_recovery block was present but failed to rebuild, so a recovery
record naming one PR could be bypassed by renewal evidence naming another.
Present-but-unusable recovery evidence is now ambiguous rather than absent and
fails closed. Because an expired lease whose recorded owner has also died
satisfies both dispositions in one gitea_lock_issue call, a sanctioned pair is
a reachable state; when both rebuild, they must agree on issue, PR, branch and
every head, or no continuation authority is returned. Recovery-only and
renewal-only locks keep their existing behaviour exactly.
Validation of the resulting token against live PR state remains untouched and
solely owned by issue_work_duplicate_gate._assess_owning_pr_exemption. No
public signature, MCP tool schema, refusal shape or reason code changes.
Tests: focused #945/#755/#760 suites 137 passed, 19 subtests. Full suite in a
branches/ worktree: 30F/5619P/6S/1013 subtests at this head vs 30F/5572P/6S/1002
at a clean checkout of 79334d48, with byte-identical failing test id sets - the
+47 passes and +11 subtests are exactly the new coverage.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Q8RUznLXEA4JoK48sTZiSK
This commit is contained in:
+29
-2
@@ -459,6 +459,31 @@ def owning_pr_renewal_from_lock(
|
||||
Renewal has no descendant case — the assessor required the local, remote and
|
||||
PR heads to be equal — so that equality is re-checked here, and the record
|
||||
must still name the claimant the lock records.
|
||||
|
||||
**What the claimant check below is, and what it is not.** It compares
|
||||
``lease_renewal.identity``/``profile`` against the claimant recorded on the
|
||||
*same* lock file. Both sides are server-written fields of one document, so
|
||||
this is an internal-consistency check: it rejects a lock whose renewal block
|
||||
and claimant disagree. It does **not** consult the live authenticated caller
|
||||
and therefore does not, on its own, prove that the session invoking a later
|
||||
gate is the session the renewal was granted to.
|
||||
|
||||
The binding that actually keeps one session from using another's renewal is
|
||||
structural, and it lives in the caller rather than here. The enforcement
|
||||
paths load the lock through ``_load_existing_issue_lock()`` with no issue
|
||||
coordinates, which resolves ``issue_lock_store.read_session_issue_lock()``
|
||||
→ the session pointer at ``session-{os.getpid()}.json``. Lock *selection* is
|
||||
scoped to the operating-system process, so a caller cannot aim the recheck
|
||||
at a lock some other process bound. Its limits follow from what that scope
|
||||
is: it is per-process, not per-authenticated-user; it says nothing about a
|
||||
lock reached by explicit issue coordinates rather than the session pointer,
|
||||
and nothing about two roles sharing one process. Live identity and profile
|
||||
are enforced separately, by the mutation-authority and profile gates each
|
||||
mutating path already runs — not by this rebuild.
|
||||
|
||||
This function is therefore strictly a re-read with an added consistency
|
||||
requirement. It narrows what a persisted lock can authorize; it never widens
|
||||
it, and it never substitutes for a caller-identity gate.
|
||||
"""
|
||||
if not isinstance(lock_record, Mapping):
|
||||
return None
|
||||
@@ -488,8 +513,10 @@ def owning_pr_renewal_from_lock(
|
||||
return None
|
||||
# Renewal is refused outright unless the durable lock records both a
|
||||
# claimant username and profile, so a sanctioned record always carries them.
|
||||
# Requiring them to still agree keeps a renewal block from being reused
|
||||
# under an identity or profile the lock no longer names.
|
||||
# Requiring them to still agree rejects a lock whose renewal block and
|
||||
# claimant disagree. Both values are read from this one server-written
|
||||
# document: this is internal consistency, not a check against the live
|
||||
# authenticated caller — see the docstring for the binding that is.
|
||||
claimant = lock_record.get("claimant")
|
||||
if not isinstance(claimant, Mapping):
|
||||
lease = lock_record.get("work_lease")
|
||||
|
||||
Reference in New Issue
Block a user