Compare commits

...
Author SHA1 Message Date
sysadmin cc3ad0870a Merge pull request 'fix(validator): align final-report validator with canonical schema (Closes #698)' (#705) from fix/issue-698-report-validator-schema into master 2026-07-13 17:26:11 -05:00
sysadminandClaude Fable 5 ee90a5e7a2 fix(validator): align final-report validator with canonical schema (Closes #698)
Fixes the final-report validator defects from Issue #698 (original lead
plus the independent reproduction recorded during the PR #703 formal
review, comment 11246):

- Legacy fields: the review/merger required-field tables no longer demand
  'Pinned reviewed head', 'Scratch worktree used', 'Worktree path',
  'Worktree dirty', 'Mutations', 'Next', 'Issue/PR', 'Branch/SHA', or
  'Files changed' — names the canonical review-merge-final-report schema
  forbids or replaces. The canonical names ('Reviewed head SHA', 'Review
  worktree path/dirty', 'Safe next action', mutation categories) are
  required instead, with backward-compatible aliases where the schema
  permits them.
- Structured proof: workflow-load helper results are recognized in
  colon, key=value, and JSON renderings; validation pass proof is
  accepted anywhere in the Validation field value (for example
  'focused 50 passed; full 2665 passed').
- Mutation inference: review mutations are inferred only from
  authoritative evidence (performed=true and not gated); read-only
  diagnostics and pre-API rejections no longer count as mutations.
- Lease release vs cleanup: canonical reviewer lease release (release
  tool call or terminal phase=released marker) is lease lifecycle, not
  post-merge cleanup, and no longer triggers the branch/worktree cleanup
  checklist; genuine delete/remove claims still require full proof.
- Blocked reports: a legitimately blocked run that states an explicit
  'Reviewed/Candidate head SHA: none' with no verdict, merge, or started
  validation owes no head proofs; approval-time and merge-time live-head
  proofs are demanded only once the corresponding phase begins, and a
  report that states no head at all still fails closed.
- action_log robustness: malformed (non-dict) entries and non-list logs
  are reported as clear sanitized findings (position and type only, no
  content echo) instead of crashing with AttributeError; a defective
  validator rule now fails closed with a sanitized block finding rather
  than raising a secondary exception.

27 new regression tests, including canonical fixtures modeled on the
PR #703 formal-review handoff and the blocked preflight report from the
prior #698 reproductions. Two legacy-field test fixtures updated to the
canonical schema. Full suite: 2663 passed, 6 skipped, 161 subtests.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-13 17:37:13 -04:00
sysadmin 237656702f Merge pull request 'feat(lease): guarded cleanup for obsolete reviewer comment leases (Closes #691)' (#692) from fix/issue-691-obsolete-reviewer-lease-cleanup into master 2026-07-12 22:28:59 -05:00
sysadmin 6d86db793b feat(lease): guarded cleanup for obsolete reviewer comment leases (Closes #691)
Add fail-closed assessment and MCP tool for expired/superseded-head
comment-backed reviewer leases that lack a control-plane lease_id.

- assess_obsolete_reviewer_comment_lease_cleanup eligibility matrix
- diagnose classifications: foreign_active_current_head,
  foreign_expired_current_head, foreign_completed_superseded_head,
  foreign_expired_superseded_head, orphaned_owner_missing,
  ambiguous_conflicting_evidence
- gitea_cleanup_obsolete_reviewer_comment_lease with confirmation
  CLEANUP OBSOLETE REVIEWER LEASE <n> and controller_recovery_authorized
- preserve audit history via append-only phase=released marker
- never repoint, transfer validation, use PID ownership, or steal
  active current-head foreign leases
- regression for PR #688 lease comment 10749 after recorded expiry
2026-07-12 23:09:20 -04:00
sysadmin 56f1230a10 Merge pull request 'feat(guard): harden workflow against unattributed root WIP and pytest-disabled production guards (Closes #683)' (#684) from fix/issue-683-workflow-guard-hardening into master 2026-07-12 18:58:55 -05:00
sysadmin d302602567 feat(guard): harden workflow against unattributed root WIP and pytest-disabled production guards (Closes #683)
Add shared workflow_scope_guard with typed blockers (blocker_kind +
exact_next_action) and force-on production enforcement under pytest.
Wire root/branches/scope checks through verify_preflight_purity and
mutation entrypoints (create_issue, comment_issue) without adopting the
rejected 300a4ca patterns (test-mode early-return, porcelain *.py filter).

Regression suite covers out-of-scope issue ownership, root diagnostic
edits, worktree bind success path, force-on under pytest, porcelain
integrity, monkeypatch resistance, real entrypoint fail-closed proof,
and durable failure recording.
2026-07-12 13:15:58 -04:00
19 changed files with 3737 additions and 200 deletions
+94 -9
View File
@@ -134,16 +134,22 @@ _TARGET_BRANCH_SHA_RE = re.compile(
r"target branch sha\s*:\s*[0-9a-f]{40}",
re.IGNORECASE,
)
# #698: structured proof is rendered in several equivalent shapes —
# `workflow_hash: abc...`, `workflow_hash=abc...`, or JSON
# `"workflow_hash": "abc..."`. Recognize all of them; demanding one exact
# punctuation style rejects legitimate structured workflow-load proof.
_WORKFLOW_LOAD_HELPER_RE = re.compile(
r"workflow[- ]load helper result\s*:",
r"workflow[-_ ]load[-_ ]helper[-_ ]result\s*[:=]",
re.IGNORECASE,
)
_WORKFLOW_LOAD_HASH_RE = re.compile(
r"workflow[- ]load helper result[\s\S]{0,400}?workflow[_ ]hash\s*:\s*[0-9a-f]{12}",
r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?"
r"workflow[_ ]hash\"?\s*[:=]\s*\"?[0-9a-f]{12}",
re.IGNORECASE,
)
_WORKFLOW_LOAD_BOUNDARY_RE = re.compile(
r"workflow[- ]load helper result[\s\S]{0,400}?boundary[_ ]status\s*:\s*(?:clean|violation)",
r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?"
r"boundary[_ ]status\"?\s*[:=]\s*\"?(?:clean|violation)",
re.IGNORECASE,
)
_WORKFLOW_FILE_VIEW_NARRATIVE_RE = re.compile(
@@ -244,6 +250,59 @@ def _normalize_task_kind(task_kind: str | None) -> str:
return _TASK_KIND_ALIASES.get(raw, raw)
def _iter_action_entries(action_log: list | None) -> list[dict]:
"""Yield only structured (dict) action-log entries (#698).
Callers must never crash on malformed entries (strings, numbers, null)
that reach the validator from LLM-composed or partially parsed logs;
:func:`sanitize_action_log` reports them separately.
"""
return [e for e in (action_log or []) if isinstance(e, dict)]
def sanitize_action_log(
action_log: list | None,
) -> tuple[list[dict], list[dict[str, str]]]:
"""Split an action log into structured entries and sanitized findings (#698).
Malformed entries become clear, sanitized ``warning`` findings — the
offending value's content is never echoed back (only its position and
type), so secrets or garbage in a broken log cannot leak into validation
errors, and validation itself proceeds without secondary exceptions.
"""
if action_log is None:
return [], []
if not isinstance(action_log, (list, tuple)):
return [], [
validator_finding(
"shared.action_log_malformed",
"downgrade",
"Action log",
"action_log is not a list of structured entries "
f"(got {type(action_log).__name__}); it was ignored",
"pass action_log as a list of dict entries",
)
]
entries: list[dict] = []
findings: list[dict[str, str]] = []
for index, entry in enumerate(action_log):
if isinstance(entry, dict):
entries.append(entry)
continue
findings.append(
validator_finding(
"shared.action_log_malformed",
"downgrade",
"Action log",
f"action_log entry {index} is not a structured mapping "
f"(got {type(entry).__name__}); the entry was ignored",
"repair the malformed action_log entry or drop it before "
"revalidating",
)
)
return entries, findings
def validator_finding(
rule_id: str,
severity: str,
@@ -421,7 +480,7 @@ def _rule_shared_canonical_comment_post_claim(
rejected_in_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text))
rejected_in_log = False
if action_log:
for entry in action_log:
for entry in _iter_action_entries(action_log):
validation = entry.get("canonical_comment_validation") or {}
if validation.get("allowed") is False:
rejected_in_log = True
@@ -475,9 +534,13 @@ def _rule_reviewer_vague_mutations_none(
action_log: list[dict] | None = None,
mutations_observed: bool = False,
) -> list[dict[str, str]]:
# #698: infer review mutations only from authoritative evidence — an
# entry proves a mutation only when it affirmatively records
# performed=true and was not gated. Read-only diagnostics and pre-API
# rejections (entries without a performed flag) are not mutations.
performed = any(
e.get("performed") is not False and not e.get("gated_rejected")
for e in (action_log or [])
e.get("performed") is True and not e.get("gated_rejected")
for e in _iter_action_entries(action_log)
)
if not (mutations_observed or performed):
return []
@@ -536,7 +599,7 @@ def _rule_reviewer_git_fetch_readonly(
text = report_text or ""
fetch_observed = any(
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
for e in (action_log or [])
for e in _iter_action_entries(action_log)
) or _GIT_FETCH_RE.search(text)
if not fetch_observed:
return []
@@ -977,7 +1040,7 @@ def _rule_reviewer_target_branch_freshness(
fields = _handoff_fields(text)
fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any(
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
for e in (action_log or [])
for e in _iter_action_entries(action_log)
)
target_sha_reported = bool(_TARGET_BRANCH_SHA_RE.search(text)) or any(
"target branch" in key and "sha" in key and _FULL_SHA_RE.search(value)
@@ -1735,6 +1798,12 @@ def assess_final_report_validator(
checks: dict[str, Any] = {}
findings: list[dict[str, str]] = []
# #698: malformed action_log data must never crash validation with a
# secondary exception; malformed entries surface as sanitized findings.
sanitized_action_log, action_log_findings = sanitize_action_log(action_log)
action_log = sanitized_action_log
findings.extend(action_log_findings)
if normalized_kind == "issue_filing" and issue_filing_lock is not None:
checks["issue_filing"] = assess_issue_filing_final_report(
report_text,
@@ -1763,7 +1832,23 @@ def assess_final_report_validator(
}
for rule in _RULES_BY_TASK.get(normalized_kind, ()):
findings.extend(_call_rule(rule, report_text, normalized_kind, rule_kwargs))
try:
findings.extend(
_call_rule(rule, report_text, normalized_kind, rule_kwargs)
)
except Exception as exc: # #698: fail closed with a sanitized error
findings.append(
validator_finding(
"shared.validator_rule_error",
"block",
"Validator",
f"validator rule '{getattr(rule, '__name__', 'unknown')}' "
f"failed with {type(exc).__name__} (details withheld; "
"sanitized)",
"file a validator defect with the rule name; do not "
"bypass final-report validation",
)
)
grade, blocked, downgraded = _aggregate_grade(findings)
reasons = [f"{f['rule_id']}: {f['reason']}" for f in findings]
+572 -92
View File
@@ -623,101 +623,280 @@ def verify_preflight_purity(
remote: str | None = None,
worktree_path: str | None = None,
task: str | None = None,
*,
target_issue_number: int | None = None,
require_author_lock: bool = False,
):
"""Verify that identity and capability were verified prior to session edits."""
"""Verify identity/capability order, then production workspace guards.
#683: pytest/unittest must not skip production root/branches/scope
enforcement when force-on signals request production behavior. The
early return below only skips *preflight-order* purity checks under
pure unit-test isolation never when production guards are active.
"""
global _preflight_reviewer_violation_files
in_test = _preflight_in_test_mode()
if in_test and not (
os.environ.get("GITEA_TEST_FORCE_DIRTY")
or os.environ.get("GITEA_TEST_PORCELAIN") is not None
):
return
production_active = workflow_scope_guard.production_guards_active(
in_test_mode=in_test
)
# Pure unit-test isolation: skip purity-order unless legacy dirty/porcelain
# force flags request the dirtiness path. #683 FORCE_PRODUCTION_GUARDS alone
# runs production root/branches/scope without requiring whoami/capability.
skip_purity_order = in_test and not workflow_scope_guard.purity_order_forced()
if not _preflight_whoami_called:
raise RuntimeError(
"Pre-flight order violation: Identity (gitea_whoami) has not been verified (fail closed)"
)
if not _preflight_capability_called:
raise RuntimeError(
"Pre-flight order violation: Task capability (gitea_resolve_task_capability) has not been resolved (fail closed)"
)
if (
task is not None
and _preflight_resolved_task is not None
and task != _preflight_resolved_task
):
raise RuntimeError(
"Pre-flight task mismatch: "
f"resolved '{_preflight_resolved_task}' but mutation requires "
f"'{task}' (fail closed)"
)
# #671: block review/merge/close/completion mutations while the session is
# contaminated by a direct stable-branch push attempt (reconciler-exempt).
_enforce_stable_branch_contamination_gate(task, remote)
ctx = _resolve_namespace_mutation_context(worktree_path)
workspace = ctx["workspace_path"]
canonical_root = ctx["canonical_repo_root"]
process_root = ctx["process_project_root"]
real_workspace = os.path.realpath(workspace)
role = ctx.get("workspace_role_kind") or _effective_workspace_role()
if real_workspace != process_root:
if not _preflight_in_test_mode():
membership = author_mutation_worktree.assess_workspace_repo_membership(
workspace_path=workspace,
canonical_repo_root=canonical_root,
if not skip_purity_order:
if not _preflight_whoami_called:
raise RuntimeError(
"Pre-flight order violation: Identity (gitea_whoami) has not been verified (fail closed)"
)
if membership["block"]:
if not _preflight_capability_called:
raise RuntimeError(
"Pre-flight order violation: Task capability (gitea_resolve_task_capability) has not been resolved (fail closed)"
)
if (
task is not None
and _preflight_resolved_task is not None
and task != _preflight_resolved_task
):
raise RuntimeError(
"Pre-flight task mismatch: "
f"resolved '{_preflight_resolved_task}' but mutation requires "
f"'{task}' (fail closed)"
)
# #671: block review/merge/close/completion mutations while the session is
# contaminated by a direct stable-branch push attempt (reconciler-exempt).
_enforce_stable_branch_contamination_gate(task, remote)
ctx = _resolve_namespace_mutation_context(worktree_path)
workspace = ctx["workspace_path"]
canonical_root = ctx["canonical_repo_root"]
process_root = ctx["process_project_root"]
real_workspace = os.path.realpath(workspace)
role = ctx.get("workspace_role_kind") or _effective_workspace_role()
if real_workspace != process_root:
if not _preflight_in_test_mode():
membership = author_mutation_worktree.assess_workspace_repo_membership(
workspace_path=workspace,
canonical_repo_root=canonical_root,
)
if membership["block"]:
raise RuntimeError(
author_mutation_worktree.format_workspace_repo_membership_error(
membership
)
)
dirty_files = sorted(
_parse_porcelain_entries(_get_workspace_porcelain(workspace))
)
if dirty_files:
raise RuntimeError(
author_mutation_worktree.format_workspace_repo_membership_error(
membership
nwb.format_namespace_workspace_binding_error(
role_kind=role,
workspace_path=workspace,
binding_source=ctx.get("workspace_binding_source")
or "unknown binding source",
dirty_files=dirty_files,
ignored_bindings=ctx.get("ignored_bindings"),
)
)
dirty_files = sorted(_parse_porcelain_entries(_get_workspace_porcelain(workspace)))
if dirty_files:
raise RuntimeError(
nwb.format_namespace_workspace_binding_error(
role_kind=role,
workspace_path=workspace,
binding_source=ctx.get("workspace_binding_source")
or "unknown binding source",
dirty_files=dirty_files,
ignored_bindings=ctx.get("ignored_bindings"),
)
)
else:
if _preflight_whoami_violation:
raise RuntimeError(
"Pre-flight order violation: Workspace file edits occurred before "
f"gitea_whoami verification (fail closed). Offending files: "
f"{_format_preflight_files(_preflight_whoami_violation_files)}"
)
if _preflight_capability_violation:
raise RuntimeError(
"Pre-flight order violation: Workspace file edits occurred before "
f"gitea_resolve_task_capability verification (fail closed). Offending files: "
f"{_format_preflight_files(_preflight_capability_violation_files)}"
)
if role in {"reviewer", "merger"}:
current = _get_workspace_porcelain()
baseline = _preflight_capability_baseline_porcelain or ""
reviewer_delta = _new_tracked_changes_since(baseline, current)
_preflight_reviewer_violation_files = reviewer_delta
if reviewer_delta:
else:
if _preflight_whoami_violation:
raise RuntimeError(
f"{role.title()} role violation: profile is forbidden from modifying "
"tracked workspace files (fail closed). Offending files: "
f"{_format_preflight_files(reviewer_delta)}"
"Pre-flight order violation: Workspace file edits occurred before "
f"gitea_whoami verification (fail closed). Offending files: "
f"{_format_preflight_files(_preflight_whoami_violation_files)}"
)
if _preflight_capability_violation:
raise RuntimeError(
"Pre-flight order violation: Workspace file edits occurred before "
f"gitea_resolve_task_capability verification (fail closed). Offending files: "
f"{_format_preflight_files(_preflight_capability_violation_files)}"
)
_enforce_root_checkout_guard(worktree_path)
_enforce_branches_only_author_mutation(worktree_path)
_clear_preflight_capability_state()
if role in {"reviewer", "merger"}:
current = _get_workspace_porcelain()
baseline = _preflight_capability_baseline_porcelain or ""
reviewer_delta = _new_tracked_changes_since(baseline, current)
_preflight_reviewer_violation_files = reviewer_delta
if reviewer_delta:
raise RuntimeError(
f"{role.title()} role violation: profile is forbidden from modifying "
"tracked workspace files (fail closed). Offending files: "
f"{_format_preflight_files(reviewer_delta)}"
)
# Historical path: root + branches after purity-order when dirty paths live.
_enforce_root_checkout_guard(worktree_path)
_enforce_branches_only_author_mutation(worktree_path)
_enforce_issue_scope_guard(
worktree_path,
task=task,
target_issue_number=target_issue_number,
require_author_lock=require_author_lock,
)
_clear_preflight_capability_state()
return
# #683: under pytest unit isolation, FORCE_PRODUCTION_GUARDS still runs
# production root + branches + issue scope (no silent no-op of guards).
if production_active:
_enforce_root_checkout_guard(worktree_path)
_enforce_branches_only_author_mutation(worktree_path)
_enforce_issue_scope_guard(
worktree_path,
task=task,
target_issue_number=target_issue_number,
require_author_lock=require_author_lock,
)
def _session_issue_lock_snapshot(
workspace_path: str | None = None,
) -> dict:
"""Return session lock fields relevant to #683 scope enforcement.
Branch-vs-lock comparison uses the live workspace branch only when the
lock's worktree matches the mutation workspace. That prevents a foreign
or leftover session lock from poisoning unrelated test worktrees while
still fail-closing when the bound worktree drifts to another issue.
"""
lock = issue_lock_store.read_session_issue_lock() or {}
raw = lock.get("issue_number")
locked: int | None
try:
locked = int(raw) if raw is not None else None
except (TypeError, ValueError):
locked = None
lock_wt = (lock.get("worktree_path") or "").strip()
workspace = (workspace_path or "").strip()
worktrees_match = False
if lock_wt and workspace:
try:
worktrees_match = os.path.realpath(lock_wt) == os.path.realpath(workspace)
except OSError:
worktrees_match = False
return {
"locked_issue_number": locked,
"lock_branch_name": (lock.get("branch_name") or "").strip() or None,
"lock_worktree_path": lock_wt or None,
"worktrees_match": worktrees_match,
}
def _session_locked_issue_number() -> int | None:
"""Return the active session issue lock number when present (#683)."""
return _session_issue_lock_snapshot().get("locked_issue_number")
def _enforce_issue_scope_guard(
worktree_path: str | None = None,
*,
task: str | None = None,
target_issue_number: int | None = None,
require_author_lock: bool = False,
) -> None:
"""#683: fail closed on missing/out-of-scope issue ownership for mutations."""
ctx = _resolve_namespace_mutation_context(worktree_path)
workspace = ctx["workspace_path"]
git_state = issue_lock_worktree.read_worktree_git_state(workspace)
# Honour actual profile role as well as poisoned task role (#540 / #683):
# comment_issue preflight stamps required_role_kind=author, which must not
# strip a genuine reconciler of control-checkout exemptions.
role = ctx.get("workspace_role_kind") or _effective_workspace_role()
actual = _actual_profile_role()
if actual in nwb.NON_AUTHOR_ROLES:
role = actual
snap = _session_issue_lock_snapshot(workspace)
# Scope uses the lock's recorded branch for issue-number matching.
# Live workspace branch can inherit the parent control checkout's branch
# name when a temp branches/ dir is not its own worktree tip — that must
# not invent a false out-of-scope failure. Live branch drift is enforced
# by issue_lock_store.verify_lock_for_mutation elsewhere.
branch_for_scope = snap.get("lock_branch_name")
if (
snap.get("worktrees_match")
and workflow_scope_guard.production_guards_forced()
):
live_branch = git_state.get("current_branch")
live_issue = workflow_scope_guard.extract_issue_number_from_branch(
live_branch
)
locked = snap.get("locked_issue_number")
if (
live_issue is not None
and locked is not None
and live_issue != locked
):
branch_for_scope = live_branch
# Author implementation / source-adjacent mutations need ownership when forced.
authorish = role == "author" or (
task
in {
"create_issue",
"comment_issue",
"lock_issue",
"create_pr",
"commit_files",
"gitea_commit_files",
"mark_issue",
}
)
require_lock = bool(require_author_lock) or (
authorish
and workflow_scope_guard.production_guards_forced()
and role == "author"
)
assessment = workflow_scope_guard.assess_production_mutation_guards(
workspace_path=workspace,
canonical_repo_root=ctx["canonical_repo_root"],
porcelain_status=git_state.get("porcelain_status") or "",
current_branch=branch_for_scope,
locked_issue_number=snap.get("locked_issue_number"),
target_issue_number=target_issue_number,
role_kind=role,
require_author_lock=require_lock,
in_test_mode=_preflight_in_test_mode(),
)
workflow_scope_guard.raise_if_blocked(assessment)
def _production_guard_block_from_exc(exc: BaseException, **extra) -> dict | None:
"""Map production-guard exceptions to typed tool block responses (#683)."""
if isinstance(exc, workflow_scope_guard.ProductionGuardError):
return workflow_scope_guard.block_response(exc, **extra)
text = str(exc)
if "Workflow scope guard (#683)" in text or "Root checkout guard (#475)" in text:
kind = workflow_scope_guard.BLOCKER_PRODUCTION_GUARD
if "root_diagnostic_edit" in text or "tracked source or test edits" in text:
kind = workflow_scope_guard.BLOCKER_ROOT_DIAGNOSTIC_EDIT
elif "Branches-only mutation guard" in text or "stable control checkout" in text:
kind = workflow_scope_guard.BLOCKER_MISSING_WORKTREE
elif "out-of-scope" in text or "locked to issue" in text:
kind = workflow_scope_guard.BLOCKER_OUT_OF_SCOPE_ISSUE
elif "no owning issue" in text:
kind = workflow_scope_guard.BLOCKER_MISSING_ISSUE_SCOPE
return workflow_scope_guard.block_response(
blocker_kind=kind,
reasons=[text],
**extra,
)
if "Branches-only mutation guard" in text:
return workflow_scope_guard.block_response(
blocker_kind=workflow_scope_guard.BLOCKER_MISSING_WORKTREE,
reasons=[text],
**extra,
)
if "Root checkout guard" in text:
return workflow_scope_guard.block_response(
blocker_kind=workflow_scope_guard.BLOCKER_ROOT_DIAGNOSTIC_EDIT,
reasons=[text],
**extra,
)
return None
def _verify_role_mutation_workspace(
@@ -727,7 +906,12 @@ def _verify_role_mutation_workspace(
worktree: str | None = None,
task: str | None = None,
) -> str:
"""Bind reviewer/merger mutations to the active namespace workspace (#510)."""
"""Bind reviewer/merger mutations to the active namespace workspace (#510).
#683: must NOT early-return solely because pytest/unittest is loaded.
Production workspace binding always runs; test isolation uses explicit
env fixtures / force-on flags, never a production short-circuit here.
"""
# Check running runtimes to prevent stale mutations
try:
@@ -928,6 +1112,7 @@ import merge_approval_gate # noqa: E402
import already_landed_reconcile # noqa: E402
import author_mutation_worktree # noqa: E402
import root_checkout_guard # noqa: E402
import workflow_scope_guard # noqa: E402 # #683 production scope / force-on guards
import stable_branch_push_guard # noqa: E402
import remote_repo_guard # noqa: E402
import issue_claim_heartbeat # noqa: E402
@@ -1896,7 +2081,15 @@ def gitea_create_issue(
)
if blocked:
return blocked
verify_preflight_purity(remote, worktree_path=worktree_path, task="create_issue")
try:
verify_preflight_purity(
remote, worktree_path=worktree_path, task="create_issue"
)
except Exception as exc:
typed = _production_guard_block_from_exc(exc, number=None)
if typed is not None:
return typed
raise
content_gate = issue_content_gate.pre_create_issue_content_gate(
title,
body,
@@ -7771,11 +7964,12 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
org: str | None = None,
repo: str | None = None,
) -> dict:
"""Read-only: diagnose open-PR reviewer lease handoff and emit next action (#599).
"""Read-only: diagnose open-PR reviewer lease handoff and emit next action (#599, #691).
Classifies no-lease / own / foreign / instructed-lease-missing-with-replacement
and worktree binding mismatch. Returns a canonical ``next_action`` without
mutating Gitea state. Never steals foreign leases.
Classifies no-lease / own / foreign / superseded-head / expired / orphan /
instructed-lease-missing-with-replacement and worktree binding mismatch.
Returns a canonical ``next_action`` (including guarded obsolete-lease cleanup)
without mutating Gitea state. Never steals foreign leases.
"""
read_block = _profile_operation_gate("gitea.read")
if read_block:
@@ -7786,7 +7980,7 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
}
comments = _fetch_pr_comments(
pr_number, remote=remote, host=host, org=org, repo=repo)
h, _o, _r = _resolve(remote, host, org, repo)
h, o, r = _resolve(remote, host, org, repo)
try:
username = _authenticated_username(h)
except Exception:
@@ -7800,6 +7994,64 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
# Prefer in-session lease id when present; otherwise diagnose as a fresh
# caller without inventing ownership of an on-thread lease.
session_id = (session_lease.get("session_id") or "").strip() or None
# Live PR head + formal reviews for #691 superseded/completed classification.
current_head_sha = None
formal_reviews: list[dict] = []
try:
auth = _auth(h)
pr_live = api_request(
"GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth
) or {}
head = pr_live.get("head") or {}
current_head_sha = head.get("sha") or pr_live.get("head_commit_sha")
try:
reviews = api_request(
"GET",
f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews",
auth,
) or []
for rev in reviews if isinstance(reviews, list) else []:
formal_reviews.append({
"verdict": rev.get("state") or rev.get("verdict"),
"reviewed_head_sha": rev.get("commit_id") or rev.get(
"reviewed_head_sha"
),
"dismissed": bool(rev.get("dismissed")),
"reviewer": ((rev.get("user") or {}).get("login")),
"submitted_at": rev.get("submitted_at"),
})
except Exception:
formal_reviews = []
except Exception:
current_head_sha = None
lease_wt = None
active = reviewer_pr_lease.find_newest_nonterminal_lease(
comments, pr_number=pr_number, include_expired=True
)
if active:
lease_wt = (active.get("worktree") or "").strip() or None
worktree_exists = None
worktree_clean = None
if lease_wt:
worktree_exists = os.path.isdir(lease_wt)
if worktree_exists:
try:
import subprocess
st = subprocess.run(
["git", "-C", lease_wt, "status", "--porcelain"],
capture_output=True,
text=True,
timeout=30,
check=False,
)
if st.returncode == 0:
worktree_clean = not bool((st.stdout or "").strip())
except Exception:
worktree_clean = None
diagnosis = reviewer_pr_lease.diagnose_reviewer_pr_lease_handoff(
comments,
pr_number=pr_number,
@@ -7809,6 +8061,11 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
env_bound_worktree=env_wt,
instructed_session_id=instructed_session_id,
instructed_comment_id=instructed_comment_id,
current_head_sha=current_head_sha,
formal_reviews=formal_reviews,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
owner_process_alive=None, # PID never proves ownership; leave unset
)
diagnosis["success"] = True
diagnosis["remote"] = remote
@@ -7936,6 +8193,212 @@ def gitea_cleanup_post_merge_moot_lease(
return report
@mcp.tool()
def gitea_cleanup_obsolete_reviewer_comment_lease(
pr_number: int,
confirmation: str = "",
apply: bool = False,
controller_recovery_authorized: bool = False,
expected_lease_comment_id: int | None = None,
expected_session_id: str | None = None,
expected_leased_head: str | None = None,
owner_process_alive: bool | None = None,
current_head_review_in_progress: bool = False,
remote: str = "dadeschools",
host: str | None = None,
org: str | None = None,
repo: str | None = None,
) -> dict:
"""Guarded cleanup for obsolete comment-backed reviewer leases on open PRs (#691).
Neutralises an expired and/or superseded-head foreign lease by posting a
terminal ``phase: released`` lease marker (append-only audit). Never deletes
comments, never repoints the lease to the new head, never transfers
validation/decision/workflow state, never uses PID equality as ownership,
and never steals a genuinely active foreign lease on the current head.
Read-first when ``apply`` is false. Apply requires:
- ``controller_recovery_authorized=True`` (explicit operator/controller
recovery capability not implied by profile alone)
- ``confirmation`` exactly ``CLEANUP OBSOLETE REVIEWER LEASE <pr_number>``
- full eligibility evidence from
``reviewer_pr_lease.assess_obsolete_reviewer_comment_lease_cleanup``
Comment-backed leases need no control-plane DB ``lease_id``.
"""
read_block = _profile_operation_gate("gitea.read")
if read_block:
return {
"success": False,
"cleanup_performed": False,
"no_steal_or_adoption": True,
"reasons": read_block,
"permission_report": _permission_block_report("gitea.read"),
}
h, o, r = _resolve(remote, host, org, repo)
auth = _auth(h)
expected_repo = f"{o}/{r}"
comments = _fetch_pr_comments(
pr_number, remote=remote, host=host, org=org, repo=repo
)
pr_live = api_request(
"GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth
) or {}
head = pr_live.get("head") or {}
current_head_sha = head.get("sha") or pr_live.get("head_commit_sha")
formal_reviews: list[dict] = []
try:
reviews = api_request(
"GET",
f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews",
auth,
) or []
for rev in reviews if isinstance(reviews, list) else []:
formal_reviews.append({
"verdict": rev.get("state") or rev.get("verdict"),
"reviewed_head_sha": rev.get("commit_id")
or rev.get("reviewed_head_sha"),
"dismissed": bool(rev.get("dismissed")),
"reviewer": ((rev.get("user") or {}).get("login")),
"submitted_at": rev.get("submitted_at"),
})
except Exception:
formal_reviews = []
lease = reviewer_pr_lease.find_newest_nonterminal_lease(
comments, pr_number=pr_number, include_expired=True
)
lease_wt = ((lease or {}).get("worktree") or "").strip() or None
worktree_exists = None
worktree_clean = None
worktree_has_unpreserved_work = None
if lease_wt:
worktree_exists = os.path.isdir(lease_wt)
if worktree_exists:
try:
import subprocess
st = subprocess.run(
["git", "-C", lease_wt, "status", "--porcelain"],
capture_output=True,
text=True,
timeout=30,
check=False,
)
if st.returncode == 0:
dirty = bool((st.stdout or "").strip())
worktree_clean = not dirty
worktree_has_unpreserved_work = dirty
except Exception:
worktree_clean = None
session = reviewer_pr_lease.get_session_lease() or {}
assessment = reviewer_pr_lease.assess_obsolete_reviewer_comment_lease_cleanup(
comments,
pr_number=pr_number,
current_head_sha=current_head_sha,
formal_reviews=formal_reviews,
repo=expected_repo,
expected_repo=expected_repo,
requesting_session_id=(session.get("session_id") or "").strip() or None,
controller_recovery_authorized=controller_recovery_authorized,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
worktree_has_unpreserved_work=worktree_has_unpreserved_work,
owner_process_alive=owner_process_alive,
owner_pid_observed=None,
requesting_pid=os.getpid(),
current_head_review_in_progress=current_head_review_in_progress,
expected_lease_comment_id=expected_lease_comment_id,
expected_session_id=expected_session_id,
expected_leased_head=expected_leased_head,
confirmation=confirmation,
apply=apply,
)
report = {
"success": True,
"pr_number": pr_number,
"pr_state": pr_live.get("state"),
"cleanup_allowed": assessment.get("cleanup_allowed"),
"cleanup_performed": False,
"classification": assessment.get("classification"),
"blocker_kind": assessment.get("blocker_kind"),
"exact_next_action": assessment.get("exact_next_action"),
"mutation_eligibility": assessment.get("mutation_eligibility"),
"leased_head": assessment.get("leased_head"),
"current_head": assessment.get("current_head"),
"expires_at": assessment.get("expires_at"),
"terminal_review": assessment.get("terminal_review"),
"worktree_state": assessment.get("worktree_state"),
"owner_session_evidence": assessment.get("owner_session_evidence"),
"cleanup_tool": assessment.get("cleanup_tool"),
"required_confirmation": assessment.get("required_confirmation"),
"active_lease": assessment.get("active_lease"),
"no_steal_or_adoption": True,
"no_repoint": True,
"no_validation_transfer": True,
"mode": "apply" if apply else "read_only",
"reasons": assessment.get("reasons") or [],
"forbidden": assessment.get("forbidden") or [],
}
if not apply:
return report
if not assessment.get("cleanup_allowed"):
report["success"] = False
report["cleanup_skipped_reason"] = (
assessment.get("fail_closed_reasons")
or assessment.get("reasons")
or ["cleanup not allowed"]
)
return report
comment_block = _profile_operation_gate("gitea.pr.comment")
if comment_block:
report["success"] = False
report["reasons"] = comment_block
report["permission_report"] = _permission_block_report("gitea.pr.comment")
return report
verify_preflight_purity(remote)
# Terminal lease marker (preserves history; does not delete comment 10749-style
# entries — newest released marker neutralises the ledger).
release_body = assessment.get("release_body") or ""
audit_body = assessment.get("audit_comment_body") or ""
comment_url = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments"
with _audited(
"comment_pr",
host=h,
remote=remote,
org=o,
repo=r,
pr_number=pr_number,
request_metadata={
"source": "cleanup_obsolete_reviewer_comment_lease",
"classification": assessment.get("classification"),
},
):
posted_release = api_request(
"POST", comment_url, auth, {"body": release_body}
)
posted_audit = None
if audit_body.strip():
posted_audit = api_request(
"POST", comment_url, auth, {"body": audit_body}
)
# Never seed session lease from cleaned foreign lease.
report["cleanup_performed"] = True
report["released_comment_id"] = (posted_release or {}).get("id")
report["audit_comment_id"] = (posted_audit or {}).get("id")
report["session_lease_seeded"] = False
return report
@mcp.tool()
def gitea_release_reviewer_pr_lease(
pr_number: int,
@@ -8167,9 +8630,26 @@ def gitea_create_issue_comment(
with the reveal opt-in); on a permission block or empty body,
'success'/'performed' False and 'reasons' with no API call made
(permission blocks also carry a structured 'permission_report',
#142).
#142). On production-guard blocks (#683): 'blocker_kind' and
'exact_next_action' with no API side effect.
"""
verify_preflight_purity(remote, worktree_path=worktree_path, task="comment_issue")
try:
# Do not pass target_issue_number: comments on other issues remain
# allowed while an author holds a different implementation lock.
# Scope ownership for source edits is enforced via branch/worktree
# binding + root diagnostic checks (#683).
verify_preflight_purity(
remote,
worktree_path=worktree_path,
task="comment_issue",
)
except Exception as exc:
typed = _production_guard_block_from_exc(
exc, issue_number=issue_number
)
if typed is not None:
return typed
raise
gate_reasons = _profile_operation_gate("gitea.issue.comment")
reasons = list(gate_reasons)
if not (body or "").strip():
+1
View File
@@ -13,6 +13,7 @@ from typing import Any
AUTHORIZED_CLEANUP_TOOLS = frozenset({
"gitea_cleanup_post_merge_moot_lease",
"gitea_cleanup_obsolete_reviewer_comment_lease",
"gitea_reconcile_merged_cleanups",
"gitea_delete_branch",
"gitea_cleanup_merged_pr_branch",
+27 -8
View File
@@ -86,6 +86,22 @@ _WRONG_BRANCH_RE = re.compile(
r"deleted branch (?:does not match|!=|differs from) (?:merged )?pr head",
re.IGNORECASE,
)
# #698: canonical reviewer lease lifecycle operations are NOT post-merge
# cleanup. Releasing a reviewer PR lease (or posting its terminal
# phase=released marker) happens after every review — merged or not — and
# must never trigger the post-merge branch/worktree cleanup checklist.
_LEASE_LIFECYCLE_RE = re.compile(
r"(?:gitea_release_reviewer_pr_lease|gitea_abandon_workflow_lease|"
r"release[d]? (?:the )?(?:reviewer|workflow) (?:pr )?lease|"
r"reviewer (?:pr )?lease release[d]?|"
r"lease (?:marker|comment).{0,40}phase\s*[:=]\s*released|"
r"phase\s*[:=]\s*released)",
re.IGNORECASE,
)
_CLEANUP_MUTATIONS_VALUE_RE = re.compile(
r"cleanup mutations\s*:\s*([^\n]+)",
re.IGNORECASE,
)
def _claims_remote_delete(text: str) -> bool:
@@ -204,16 +220,19 @@ def assess_post_merge_cleanup_proof(
for field in _worktree_cleanup_fields_present(text)
)
if (remote_delete or worktree_remove) and not (remote_delete or worktree_remove):
pass
if not remote_delete and not worktree_remove:
cleanup_mutations = re.search(
r"cleanup mutations\s*:\s*(?!none\b)\S",
text,
re.IGNORECASE,
value_match = _CLEANUP_MUTATIONS_VALUE_RE.search(text)
value = (value_match.group(1).strip() if value_match else "")
value_lower = value.lower()
substantive = bool(value) and value_lower not in {
"none", "n/a", "not applicable",
}
# #698: reviewer lease release / terminal lease markers are lease
# lifecycle, not post-merge cleanup — no checklist owed.
lease_lifecycle_only = substantive and bool(
_LEASE_LIFECYCLE_RE.search(value)
)
if cleanup_mutations:
if substantive and not lease_lifecycle_only:
reasons.append(
"cleanup mutations reported without post-merge cleanup proof checklist"
)
+67 -6
View File
@@ -403,10 +403,37 @@ _REVIEWER_ACTIVE_RE = re.compile(
r"whether any reviewer was active\s*:\s*(yes|no|true|false)",
re.IGNORECASE,
)
# #698 phase detection for phase-specific head proofs.
_NO_REVIEWED_HEAD_RE = re.compile(
r"(?:reviewed head sha|candidate head sha)\s*:\s*none\b",
re.IGNORECASE,
)
_VERDICT_RECORDED_RE = re.compile(
r"review decision\s*:\s*(?:approve[d]?|request[_ ]changes)\b"
r"|review_status\s*:\s*(?:approved|request_changes)\b"
r"|terminal review mutation\s*:\s*(?!none\b)\S",
re.IGNORECASE,
)
_MERGE_ATTEMPTED_RE = re.compile(
r"merge result\s*:\s*(?:merged|success|performed|failed|attempted)\b"
r"|merge mutations\s*:\s*(?!none\b|not applicable\b)\S",
re.IGNORECASE,
)
_VALIDATION_STARTED_RE = re.compile(
r"validation\s*:\s*(?!none\b|not run\b|not applicable\b|not started\b)"
r"[^\n]*(?:pass|fail|ran|executed|\d+\s+passed)",
re.IGNORECASE,
)
def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]:
"""Final-report proof for reviewed vs live head SHAs (#399 AC 6)."""
"""Final-report proof for reviewed vs live head SHAs (#399 AC 6).
#698: head proofs are phase-specific. A legitimately blocked run that
never began validation (no reviewed head, no formal verdict, no merge)
owes none of them; approval-time and merge-time live-head proofs are
owed only once the corresponding phase actually begins.
"""
text = report_text or ""
reasons: list[str] = []
reviewed = _normalize_sha(_REVIEWED_HEAD_RE.search(text).group(1) if _REVIEWED_HEAD_RE.search(text) else None)
@@ -422,15 +449,49 @@ def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]:
)
push_during = _PUSH_DURING_VALIDATION_RE.search(text)
if not reviewed:
# Phase detection from the report's own claims.
no_head_stated = bool(_NO_REVIEWED_HEAD_RE.search(text))
verdict_recorded = bool(_VERDICT_RECORDED_RE.search(text))
merge_attempted = bool(_MERGE_ATTEMPTED_RE.search(text))
validation_started = bool(reviewed) or bool(_VALIDATION_STARTED_RE.search(text))
blocked_before_validation = (
no_head_stated
and not reviewed
and not verdict_recorded
and not merge_attempted
and not validation_started
)
if blocked_before_validation:
return {
"proven": True,
"block": False,
"reasons": [],
"reviewed_head_sha": None,
"live_head_sha_before_approval": None,
"live_head_sha_before_merge": None,
"push_during_validation": (
push_during.group(1).lower() if push_during else None
),
"phase": "blocked_before_validation",
}
if not reviewed and not no_head_stated:
# The head must always be STATED — either a SHA or an explicit
# 'none'. Silence is not a phase claim and fails closed.
reasons.append(
"reviewed head SHA not stated in final report "
"(state the SHA or an explicit 'none')"
)
elif not reviewed and (validation_started or verdict_recorded or merge_attempted):
reasons.append("reviewed head SHA not stated in final report")
if not live_approval:
if verdict_recorded and not live_approval:
reasons.append("final live head SHA before approval not stated")
if not live_merge:
if merge_attempted and not live_merge:
reasons.append("final live head SHA before merge not stated")
if not push_during:
if validation_started and not push_during:
reasons.append("whether push occurred during validation not stated")
elif reviewed and live_approval and reviewed != live_approval:
if reviewed and live_approval and reviewed != live_approval:
reasons.append("live head before approval differs from reviewed head SHA")
elif reviewed and live_merge and reviewed != live_merge:
reasons.append("live head before merge differs from reviewed head SHA")
+6 -1
View File
@@ -25,8 +25,13 @@ _REVIEWED_HEAD_RE = re.compile(
r"(?:pinned reviewed head|reviewed head sha)\s*:\s*([0-9a-f]{7,40})",
re.IGNORECASE,
)
# #698: validation pass proof appears in several legitimate shapes —
# "Validation: pass", "Validation: focused 50 passed; full 2665 passed",
# or structured "validation_status: pass". Accept pass evidence anywhere in
# the Validation field's value, not only as its first token.
_VALIDATION_PASS_RE = re.compile(
r"validation\s*:\s*(?:pass|passed|strong|ok|green)",
r"validation(?:_status)?\s*:[^\n]{0,300}?"
r"(?:\bpass(?:ed)?\b|\bstrong\b|\bok\b|\bgreen\b|\d+\s+passed)",
re.IGNORECASE,
)
_MERGED_CLAIM_RE = re.compile(
+36 -9
View File
@@ -858,9 +858,16 @@ _WALKTHROUGH_ARTIFACT_RE = re.compile(r"walkthrough\.md", re.I)
def _performed_file_mutations(action_log: list[dict] | None) -> list[dict]:
"""Return performed local file mutations, excluding gated rejections."""
"""Return performed local file mutations, excluding gated rejections.
Non-dict entries (malformed JSON, LLM mistakes) are ignored instead of
raising ``AttributeError`` (#698): a malformed ledger entry can never be
authoritative mutation evidence.
"""
performed: list[dict] = []
for entry in action_log or []:
if not isinstance(entry, dict):
continue
if entry.get("gated_rejected") or entry.get("performed") is False:
continue
action = (entry.get("action") or "").strip().lower()
@@ -2228,24 +2235,31 @@ HANDOFF_REVIEW_MUTATION_FIELDS = (
)
HANDOFF_ROLE_FIELDS = {
# #698: the review/merger required-field sets must stay aligned with the
# canonical schema (skills/llm-project-workflow/schemas/
# review-merge-final-report.md). The schema explicitly FORBIDS the legacy
# fields 'Pinned reviewed head', 'Scratch worktree used', and 'Workspace
# mutations' — a validator must never demand a field the schema bans.
"review": (
("Selected PR", ("selected pr",)),
("Reviewer eligibility", ("reviewer eligibility", "eligibility")),
("Pinned reviewed head", ("pinned reviewed head", "pinned head")),
("Worktree path", ("worktree path", "starting worktree path")),
("Worktree dirty", ("worktree dirty", "whether worktree was dirty")),
("Scratch worktree used", ("scratch worktree used", "scratch clone used",
"scratch worktree")),
("Reviewed head SHA", ("reviewed head sha", "candidate head sha")),
("Review worktree path", ("review worktree path", "worktree path",
"starting worktree path")),
("Review worktree dirty", ("review worktree dirty", "worktree dirty",
"whether worktree was dirty")),
("Unrelated local mutations", ("unrelated local mutations",
"unrelated files modified")),
"unrelated files modified",
"file edits by reviewer")),
("Review decision", ("review decision", "decision")),
("Merge result", ("merge result",)),
("Linked issue status", ("linked issue status", "linked issue")),
("Cleanup status", ("cleanup status", "cleanup")),
("Safe next action", ("safe next action", "next")),
) + HANDOFF_REVIEW_MUTATION_FIELDS,
"merger": (
("Selected PR", ("selected pr",)),
("Pinned reviewed head", ("pinned reviewed head", "pinned head")),
("Reviewed head SHA", ("reviewed head sha", "candidate head sha")),
("Active profile", ("active profile",)),
("Role kind", ("role kind",)),
("Merge capability source", ("merge capability source",)),
@@ -2433,9 +2447,22 @@ def assess_controller_handoff(report_text, role=None, local_edits=False):
# Issue #320: reviewer and merger handoffs use the precise mutation categories
# in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous
# "Workspace mutations" field, which is rejected below.
# Issue #698: the canonical review-merge schema has no 'Mutations',
# 'Next', 'Issue/PR', 'Branch/SHA', or 'Files changed' fields — their
# content lives in the precise mutation categories, 'Safe next
# action', 'Selected PR'/'Linked issue', head-SHA fields, and 'Files
# reviewed'. Requiring the legacy names rejects canonical reports.
_non_canonical_for_review = {
"Workspace mutations",
"Mutations",
"Next",
"Issue/PR",
"Branch/SHA",
"Files changed",
}
required = [
field for field in required
if field[0] != "Workspace mutations"
if field[0] not in _non_canonical_for_review
]
if any(label.startswith("workspace mutations") for label in labels):
return {
+657 -43
View File
@@ -524,13 +524,26 @@ def assess_lease_inventory(
"reclaimable_review_leases": reclaimable,
}
# Canonical next-action vocabulary for reviewer lease handoff (#599).
# Canonical next-action vocabulary for reviewer lease handoff (#599, #691).
NEXT_ACTION_ACQUIRE = "acquire"
NEXT_ACTION_WAIT = "wait"
NEXT_ACTION_RESUME_EXACT_OWNER_SESSION = "resume_exact_owner_session"
NEXT_ACTION_RELEASE_EXPIRED_LEASE = "release_expired_lease"
NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP = "operator_authorized_cleanup"
NEXT_ACTION_REPAIR_WORKTREE_BINDING = "repair_worktree_binding"
NEXT_ACTION_CLEANUP_OBSOLETE_LEASE = "cleanup_obsolete_reviewer_comment_lease"
CLEANUP_OBSOLETE_LEASE_TOOL = "gitea_cleanup_obsolete_reviewer_comment_lease"
CLEANUP_OBSOLETE_LEASE_CONFIRMATION_PREFIX = "CLEANUP OBSOLETE REVIEWER LEASE "
# Formal terminal review verdicts that complete a leased-head workflow (#691).
_TERMINAL_REVIEW_VERDICTS = frozenset({
"APPROVED",
"REQUEST_CHANGES",
"approved",
"request_changes",
"REQUESTCHANGES",
})
_HANDOFF_CLASSIFICATIONS = frozenset({
"no_lease",
@@ -539,6 +552,12 @@ _HANDOFF_CLASSIFICATIONS = frozenset({
"foreign_active",
"foreign_reclaimable",
"foreign_expired",
"foreign_active_current_head",
"foreign_expired_current_head",
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"orphaned_owner_missing",
"ambiguous_conflicting_evidence",
"instructed_lease_missing_with_replacement",
"worktree_binding_mismatch",
})
@@ -551,6 +570,476 @@ def _norm_path(value: str | None) -> str:
return os.path.normpath(text.rstrip("/"))
def _normalize_verdict(value: str | None) -> str:
text = (value or "").strip().upper().replace("-", "_").replace(" ", "_")
if text in {"REQUESTCHANGES", "REQUEST_CHANGES", "CHANGES_REQUESTED"}:
return "REQUEST_CHANGES"
if text in {"APPROVED", "APPROVE"}:
return "APPROVED"
return text
def formal_terminal_review_for_head(
formal_reviews: list[dict] | None,
*,
leased_head: str | None,
) -> dict[str, Any] | None:
"""Return the latest undismissed terminal formal review for *leased_head*."""
head = _normalize_sha(leased_head)
if not head:
return None
matches: list[dict[str, Any]] = []
for review in formal_reviews or []:
if review.get("dismissed"):
continue
verdict = _normalize_verdict(
review.get("verdict") or review.get("state") or review.get("body_state")
)
if verdict not in {"APPROVED", "REQUEST_CHANGES"}:
continue
rhead = _normalize_sha(
review.get("reviewed_head_sha")
or review.get("commit_id")
or review.get("head_sha")
)
if rhead != head:
continue
matches.append({**review, "verdict": verdict, "reviewed_head_sha": rhead})
if not matches:
return None
return matches[-1]
def find_newest_nonterminal_lease(
comments: list[dict],
*,
pr_number: int,
now: datetime | None = None,
include_expired: bool = True,
) -> dict[str, Any] | None:
"""Newest non-terminal lease marker, optionally including expired ones (#691).
Unlike ``find_active_reviewer_lease``, this retains expired non-terminal
markers so diagnosis/cleanup can still name the obsolete lease after
``expires_at`` (comment-backed ledger; no control-plane ``lease_id``).
"""
now = now or datetime.now(timezone.utc)
entries = list(reversed(_lease_entries(comments, pr_number=pr_number)))
for entry in entries:
phase = (entry.get("phase") or "").strip().lower()
if phase in _TERMINAL_PHASES:
# Newest terminal ends the ledger chain for active acquisition, but
# an older non-terminal is not "active". Stop at newest marker.
return None
expired = _lease_expired(entry, now=now)
if expired and not include_expired:
return None
lease = dict(entry)
lease["freshness"] = classify_lease_freshness(lease, now=now)
lease["expired"] = expired
return lease
return None
def cleanup_confirmation_for_pr(pr_number: int) -> str:
return f"{CLEANUP_OBSOLETE_LEASE_CONFIRMATION_PREFIX}{int(pr_number)}"
def assess_obsolete_reviewer_comment_lease_cleanup(
comments: list[dict],
*,
pr_number: int,
current_head_sha: str | None,
formal_reviews: list[dict] | None = None,
repo: str | None = None,
expected_repo: str | None = None,
requesting_session_id: str | None = None,
controller_recovery_authorized: bool = False,
worktree_exists: bool | None = None,
worktree_clean: bool | None = None,
worktree_has_unpreserved_work: bool | None = None,
owner_process_alive: bool | None = None,
owner_pid_observed: int | None = None,
requesting_pid: int | None = None,
current_head_review_in_progress: bool = False,
expected_lease_comment_id: int | None = None,
expected_session_id: str | None = None,
expected_leased_head: str | None = None,
confirmation: str | None = None,
apply: bool = False,
now: datetime | None = None,
) -> dict[str, Any]:
"""Guarded cleanup eligibility for obsolete comment-backed reviewer leases (#691).
Cleanup is never transfer/adoption/repoint and never uses PID equality as
ownership. Eligible only when evidence proves the lease is past expiry and/or
pinned to a superseded head with a completed formal terminal review, and
every safety boundary holds.
"""
now = now or datetime.now(timezone.utc)
reasons: list[str] = []
fail_closed_reasons: list[str] = []
current_head = _normalize_sha(current_head_sha)
req_session = (requesting_session_id or "").strip()
lease = find_newest_nonterminal_lease(
comments, pr_number=pr_number, now=now, include_expired=True
)
if not lease:
# Fall back to active finder (unexpired only) for report consistency.
lease = find_active_reviewer_lease(comments, pr_number=pr_number, now=now)
classification = "no_lease"
cleanup_allowed = False
release_body: str | None = None
audit_comment_body: str | None = None
terminal_review = None
leased_head = None
head_superseded = False
past_expiry = False
expires_parseable = True
if not lease:
fail_closed_reasons.append(
f"PR #{pr_number}: no non-terminal comment-backed reviewer lease to clean"
)
classification = "no_lease"
else:
leased_head = _normalize_sha(lease.get("candidate_head"))
expires_raw = lease.get("expires_at")
expires_at = _parse_timestamp(expires_raw)
if expires_raw and expires_at is None:
expires_parseable = False
fail_closed_reasons.append(
"lease expires_at cannot be parsed or trusted (fail closed)"
)
past_expiry = bool(expires_at and expires_at <= now)
freshness = lease.get("freshness") or classify_lease_freshness(lease, now=now)
owner_session = (lease.get("session_id") or "").strip()
is_requesting_owner = bool(
req_session and owner_session and req_session == owner_session
)
# Identity pins (repo / PR / session / head / comment) must match when
# the caller supplies expected evidence.
lease_repo = (lease.get("repo") or "").strip()
exp_repo = (expected_repo or repo or "").strip()
if exp_repo and lease_repo and exp_repo != lease_repo:
fail_closed_reasons.append(
f"repository identity mismatch: lease repo={lease_repo!r} "
f"expected={exp_repo!r}"
)
if lease.get("pr_number") not in (None, pr_number):
fail_closed_reasons.append(
f"PR identity mismatch: lease pr={lease.get('pr_number')} "
f"requested={pr_number}"
)
if expected_lease_comment_id is not None:
cid = lease.get("comment_id")
if cid is None or int(cid) != int(expected_lease_comment_id):
fail_closed_reasons.append(
"lease comment_id does not match expected evidence "
f"(lease={cid}, expected={expected_lease_comment_id})"
)
if expected_session_id:
if owner_session != expected_session_id.strip():
fail_closed_reasons.append(
"lease session_id does not match expected evidence "
f"(lease={owner_session}, expected={expected_session_id})"
)
if expected_leased_head:
exp_head = _normalize_sha(expected_leased_head)
if not exp_head or exp_head != leased_head:
fail_closed_reasons.append(
"leased head does not match expected evidence "
f"(lease={leased_head}, expected={exp_head})"
)
# PID equality is never ownership proof (#691).
if (
owner_pid_observed is not None
and requesting_pid is not None
and int(owner_pid_observed) == int(requesting_pid)
):
reasons.append(
"observed PID equals requesting PID but PID equality is NOT "
"ownership proof; ignored for authorization"
)
if is_requesting_owner:
fail_closed_reasons.append(
"requesting session owns the lease; use "
"gitea_release_reviewer_pr_lease (owner path), not non-owner cleanup"
)
if current_head_review_in_progress:
fail_closed_reasons.append(
"a current-head review submission is in progress; cleanup denied"
)
if not current_head:
fail_closed_reasons.append(
"current PR head SHA missing or unparseable (fail closed)"
)
if not leased_head:
fail_closed_reasons.append(
"lease candidate_head missing or unparseable (fail closed)"
)
head_superseded = bool(
current_head and leased_head and current_head != leased_head
)
head_matches_current = bool(
current_head and leased_head and current_head == leased_head
)
terminal_review = formal_terminal_review_for_head(
formal_reviews, leased_head=leased_head
)
has_terminal = terminal_review is not None
# Worktree safety.
if worktree_has_unpreserved_work is True or worktree_clean is False:
fail_closed_reasons.append(
"lease worktree is dirty or has unpreserved work; cleanup denied"
)
if (
worktree_exists is True
and worktree_clean is None
and worktree_has_unpreserved_work is None
):
# Unknown cleanliness when path exists — fail closed.
fail_closed_reasons.append(
"lease worktree exists but cleanliness is unknown (fail closed)"
)
# Classification matrix (#691).
if head_matches_current and freshness in {"active", "stale_warning"}:
classification = "foreign_active_current_head"
fail_closed_reasons.append(
"genuinely active foreign lease on the current PR head; "
"cleanup denied (fail closed)"
)
elif head_matches_current and past_expiry:
classification = "foreign_expired_current_head"
# Expired on current head: owner-missing / orphan path may apply.
if owner_process_alive is True:
fail_closed_reasons.append(
"lease expired on current head but owner process still alive "
"with plausible activity; cleanup denied"
)
elif worktree_clean is False:
fail_closed_reasons.append(
"owner process absent but worktree dirty; cleanup denied"
)
elif owner_process_alive is False and (
worktree_clean is True or worktree_exists is False
):
if controller_recovery_authorized:
classification = "orphaned_owner_missing"
else:
fail_closed_reasons.append(
"controller/recovery capability not authorized for orphan cleanup"
)
else:
fail_closed_reasons.append(
"insufficient orphan evidence for expired current-head lease "
"(need owner_process_alive=false and clean/absent worktree)"
)
elif head_superseded and has_terminal and past_expiry:
classification = "foreign_expired_superseded_head"
elif head_superseded and has_terminal and not past_expiry:
classification = "foreign_completed_superseded_head"
elif head_superseded and not has_terminal:
classification = "ambiguous_conflicting_evidence"
fail_closed_reasons.append(
"lease head is superseded but no formal terminal review exists "
"for the leased head (fail closed)"
)
elif not head_superseded and not past_expiry and freshness in {
"active", "stale_warning"
}:
classification = "foreign_active_current_head"
fail_closed_reasons.append(
"foreign lease still active on current head; wait or use owner release"
)
elif past_expiry and not head_superseded:
classification = "foreign_expired_current_head"
else:
classification = "ambiguous_conflicting_evidence"
fail_closed_reasons.append(
"lease evidence is ambiguous; cleanup denied (fail closed)"
)
# Recent owner progress on a non-superseded active lease blocks cleanup.
minutes = _minutes_since_activity(lease, now=now)
if (
head_matches_current
and freshness == "active"
and minutes is not None
and minutes < STALE_WARNING_MINUTES
):
fail_closed_reasons.append(
"owner session has recent authenticated progress on current head; "
"cleanup denied"
)
# Authority + confirmation for apply.
if not controller_recovery_authorized:
if classification in {
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"foreign_expired_current_head",
"orphaned_owner_missing",
}:
fail_closed_reasons.append(
"controller/recovery capability required "
"(controller_recovery_authorized=true)"
)
expected_conf = cleanup_confirmation_for_pr(pr_number)
if apply:
if (confirmation or "").strip() != expected_conf:
fail_closed_reasons.append(
f"confirmation must equal exactly {expected_conf!r}"
)
# Superseded + terminal path does not require past_expiry (AC1).
eligible_class = classification in {
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"orphaned_owner_missing",
"foreign_expired_current_head",
}
# foreign_expired_current_head needs orphan/clean worktree + authority.
if classification == "foreign_expired_current_head":
if worktree_clean is not True and worktree_exists is not False:
if "worktree" not in " ".join(fail_closed_reasons):
fail_closed_reasons.append(
"expired current-head cleanup requires proven-clean "
"worktree or absent worktree"
)
if owner_process_alive is True:
fail_closed_reasons.append(
"owner process still alive on expired current-head lease"
)
cleanup_allowed = (
eligible_class
and expires_parseable
and not fail_closed_reasons
and not is_requesting_owner
)
if cleanup_allowed:
release_body = format_lease_body(
repo=lease.get("repo") or exp_repo or "",
pr_number=pr_number,
issue_number=lease.get("issue_number"),
reviewer_identity=lease.get("reviewer_identity") or "",
profile=lease.get("profile") or "unknown",
session_id=owner_session or "unknown",
worktree=lease.get("worktree") or "",
phase="released",
candidate_head=leased_head,
target_branch=lease.get("target_branch") or "master",
target_branch_sha=lease.get("target_branch_sha"),
last_activity=now,
blocker="obsolete-superseded-or-expired-lease",
)
audit_comment_body = (
"## Canonical obsolete reviewer lease cleanup (#691)\n\n"
f"- pr: #{pr_number}\n"
f"- lease_comment_id: {lease.get('comment_id')}\n"
f"- session_id: {owner_session}\n"
f"- leased_head: {leased_head}\n"
f"- current_head: {current_head}\n"
f"- expires_at: {lease.get('expires_at')}\n"
f"- classification: {classification}\n"
f"- terminal_review_verdict: "
f"{(terminal_review or {}).get('verdict')}\n"
f"- tool: {CLEANUP_OBSOLETE_LEASE_TOOL}\n"
"- action: posted terminal phase=released lease marker; "
"did not transfer validation, decision, or workflow proof; "
"did not repoint lease to the new head; did not delete history\n"
)
reasons.append(
f"cleanup eligible ({classification}); post terminal released "
"marker via sanctioned tool"
)
else:
reasons.extend(fail_closed_reasons)
return {
"pr_number": pr_number,
"classification": classification,
"blocker_kind": classification if not cleanup_allowed else "none",
"cleanup_allowed": cleanup_allowed,
"mutation_allowed": False, # never grants review mutation
"mutation_eligibility": "prohibited" if not cleanup_allowed else "cleanup_only",
"exact_next_action": (
NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
if cleanup_allowed
else (
NEXT_ACTION_WAIT
if classification
in {
"foreign_active_current_head",
"ambiguous_conflicting_evidence",
}
else NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP
)
),
"cleanup_tool": CLEANUP_OBSOLETE_LEASE_TOOL,
"required_confirmation": cleanup_confirmation_for_pr(pr_number),
"leased_head": leased_head,
"current_head": current_head,
"head_superseded": head_superseded,
"past_expiry": past_expiry,
"expires_at": (lease or {}).get("expires_at") if lease else None,
"expires_parseable": expires_parseable,
"terminal_review": terminal_review,
"terminal_review_present": terminal_review is not None,
"worktree_state": {
"path": (lease or {}).get("worktree") if lease else None,
"exists": worktree_exists,
"clean": worktree_clean,
"has_unpreserved_work": worktree_has_unpreserved_work,
},
"owner_session_evidence": {
"session_id": (lease or {}).get("session_id") if lease else None,
"reviewer_identity": (
(lease or {}).get("reviewer_identity") if lease else None
),
"process_alive": owner_process_alive,
"pid_observed": owner_pid_observed,
"pid_is_not_ownership_proof": True,
"requesting_session_is_owner": bool(
lease
and req_session
and (lease.get("session_id") or "").strip() == req_session
),
},
"active_lease": lease,
"release_body": release_body,
"audit_comment_body": audit_comment_body,
"controller_recovery_authorized": controller_recovery_authorized,
"reasons": reasons if reasons else fail_closed_reasons,
"fail_closed_reasons": fail_closed_reasons,
"forbidden": [
"manual comment deletion",
"database edits",
"mtime manipulation",
"PID-based ownership",
"direct session-state seeding",
"lease stealing or adoption",
"repointing old lease to new head",
"transfer of validation or decision state",
"worktree reuse by replacement reviewer",
],
}
def diagnose_reviewer_pr_lease_handoff(
comments: list[dict],
*,
@@ -561,40 +1050,50 @@ def diagnose_reviewer_pr_lease_handoff(
env_bound_worktree: str | None = None,
instructed_session_id: str | None = None,
instructed_comment_id: int | None = None,
current_head_sha: str | None = None,
formal_reviews: list[dict] | None = None,
worktree_exists: bool | None = None,
worktree_clean: bool | None = None,
owner_process_alive: bool | None = None,
now: datetime | None = None,
) -> dict[str, Any]:
"""Classify open-PR reviewer lease handoff and emit a canonical next action (#599).
"""Classify open-PR reviewer lease handoff and emit a canonical next action (#599, #691).
Read-only diagnosis. Never steals, releases, or adopts a foreign lease.
Fail-closed acquisition rules for active foreign leases remain intact.
Returns a structured diagnosis with:
- classification
- next_action (one of wait / resume_exact_owner_session /
release_expired_lease / operator_authorized_cleanup /
repair_worktree_binding / acquire)
- classification (including #691 superseded/expired distinctions)
- next_action (including cleanup_obsolete_reviewer_comment_lease)
- active_lease identity fields when present
- worktree_binding match result
- instructed-lease mismatch flags
- cleanup tool/confirmation when cleanup-eligible
"""
now = now or datetime.now(timezone.utc)
session_id = (current_session_id or "").strip()
identity = (current_reviewer_identity or "").strip()
instructed_sid = (instructed_session_id or "").strip()
reasons: list[str] = []
current_head = _normalize_sha(current_head_sha)
active = find_active_reviewer_lease(comments, pr_number=pr_number, now=now)
# Also surface expired non-terminal newest marker for #691 diagnosis.
newest_nt = find_newest_nonterminal_lease(
comments, pr_number=pr_number, now=now, include_expired=True
)
lease_for_class = active or newest_nt
session_lease = get_session_lease()
# Worktree binding: env-bound vs proposed vs active lease worktree.
env_wt = _norm_path(env_bound_worktree)
prop_wt = _norm_path(proposed_worktree)
lease_wt = _norm_path((active or {}).get("worktree") if active else None)
lease_wt = _norm_path((lease_for_class or {}).get("worktree") if lease_for_class else None)
binding_mismatch = False
binding_details: dict[str, Any] = {
"env_bound_worktree": env_bound_worktree or None,
"proposed_worktree": proposed_worktree or None,
"lease_worktree": (active or {}).get("worktree") if active else None,
"lease_worktree": (lease_for_class or {}).get("worktree") if lease_for_class else None,
"match": True,
}
paths = [p for p in (env_wt, prop_wt, lease_wt) if p]
@@ -608,13 +1107,13 @@ def diagnose_reviewer_pr_lease_handoff(
# Instructed lease gone while a different lease is active (PR #592-style).
instructed_missing_with_replacement = False
if instructed_sid or instructed_comment_id is not None:
if not active:
if not lease_for_class:
reasons.append(
"instructed lease is gone and no active replacement lease remains"
)
else:
owner = (active.get("session_id") or "").strip()
cid = active.get("comment_id")
owner = (lease_for_class.get("session_id") or "").strip()
cid = lease_for_class.get("comment_id")
sid_mismatch = bool(instructed_sid and owner and owner != instructed_sid)
cid_mismatch = (
instructed_comment_id is not None
@@ -631,35 +1130,94 @@ def diagnose_reviewer_pr_lease_handoff(
# Classification + next_action.
classification = "no_lease"
next_action = NEXT_ACTION_ACQUIRE
cleanup_hint: dict[str, Any] | None = None
if active:
owner = (active.get("session_id") or "").strip()
freshness = active.get("freshness") or classify_lease_freshness(
active, now=now
if lease_for_class:
owner = (lease_for_class.get("session_id") or "").strip()
freshness = lease_for_class.get("freshness") or classify_lease_freshness(
lease_for_class, now=now
)
owner_identity = (active.get("reviewer_identity") or "").strip()
owner_identity = (lease_for_class.get("reviewer_identity") or "").strip()
is_own = bool(session_id and owner and owner == session_id)
# Same identity alone is NOT ownership for resume; session_id must match.
same_identity = bool(
identity and owner_identity and identity == owner_identity
)
leased_head = _normalize_sha(lease_for_class.get("candidate_head"))
head_superseded = bool(
current_head and leased_head and current_head != leased_head
)
head_current = bool(
current_head and leased_head and current_head == leased_head
)
past_expiry = bool(lease_for_class.get("expired")) or freshness == "expired"
if not past_expiry:
past_expiry = _lease_expired(lease_for_class, now=now)
terminal = formal_terminal_review_for_head(
formal_reviews, leased_head=leased_head
)
if is_own and freshness in {"active", "stale_warning"}:
if is_own and freshness in {"active", "stale_warning"} and not past_expiry:
classification = "own_active"
next_action = NEXT_ACTION_RESUME_EXACT_OWNER_SESSION
elif is_own and freshness in {"reclaimable", "expired"}:
elif is_own and (freshness in {"reclaimable", "expired"} or past_expiry):
classification = "own_expired"
next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE
reasons.append(
f"own lease freshness is '{freshness}'; release via "
"gitea_release_reviewer_pr_lease then re-acquire"
)
elif not is_own and freshness in {"active", "stale_warning"}:
classification = "foreign_active"
elif not is_own and head_superseded and terminal and past_expiry:
classification = "foreign_expired_superseded_head"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign lease expired and pinned to superseded head with "
"formal terminal review; use guarded obsolete-lease cleanup"
)
elif not is_own and head_superseded and terminal and not past_expiry:
classification = "foreign_completed_superseded_head"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign lease pinned to superseded head with completed formal "
"review; use guarded obsolete-lease cleanup (do not wait indefinitely)"
)
elif not is_own and head_superseded and not terminal:
classification = "ambiguous_conflicting_evidence"
next_action = NEXT_ACTION_WAIT
reasons.append(
"lease head superseded but no formal terminal review for leased "
"head; fail closed / wait (no indefinite steal)"
)
elif not is_own and head_current and past_expiry:
classification = "foreign_expired_current_head"
if owner_process_alive is False and worktree_clean is True:
classification = "orphaned_owner_missing"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign expired lease on current head; owner process absent "
"and worktree clean — guarded orphan cleanup eligible"
)
elif owner_process_alive is False and worktree_clean is False:
classification = "ambiguous_conflicting_evidence"
next_action = NEXT_ACTION_WAIT
reasons.append(
"owner process absent but worktree dirty; cleanup denied"
)
else:
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign expired lease on current head; use guarded cleanup "
"when worktree/process evidence permits"
)
elif not is_own and freshness in {"active", "stale_warning"} and not past_expiry:
classification = (
"foreign_active_current_head" if head_current or not current_head
else "foreign_active"
)
next_action = NEXT_ACTION_WAIT
reasons.append(
f"foreign active reviewer lease (session_id={owner}, "
f"phase={active.get('phase')}, freshness={freshness}); "
f"phase={lease_for_class.get('phase')}, freshness={freshness}); "
"do not submit; do not steal"
)
if same_identity:
@@ -674,11 +1232,12 @@ def diagnose_reviewer_pr_lease_handoff(
f"foreign reclaimable lease (session_id={owner}); clear only via "
"sanctioned gitea_release_reviewer_pr_lease when reclaimable"
)
elif not is_own and freshness == "expired":
elif not is_own and (freshness == "expired" or past_expiry):
classification = "foreign_expired"
next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE
reasons.append(
f"foreign expired lease (session_id={owner}); use sanctioned release"
f"foreign expired lease (session_id={owner}); use sanctioned release "
f"or {CLEANUP_OBSOLETE_LEASE_TOOL}"
)
else:
classification = "foreign_active"
@@ -691,13 +1250,26 @@ def diagnose_reviewer_pr_lease_handoff(
if instructed_missing_with_replacement and classification.startswith(
"foreign"
):
classification = "instructed_lease_missing_with_replacement"
# Foreign active still means wait; reclaimable still release.
if next_action == NEXT_ACTION_WAIT:
reasons.append(
"replacement foreign lease is active — wait; "
"operator_authorized_cleanup only with explicit operator authority"
)
# Preserve #691 cleanup path when superseded/expired; otherwise
# keep the PR #592 instructed-missing label.
if next_action != NEXT_ACTION_CLEANUP_OBSOLETE_LEASE:
classification = "instructed_lease_missing_with_replacement"
if next_action == NEXT_ACTION_WAIT:
reasons.append(
"replacement foreign lease is active — wait; "
"operator_authorized_cleanup only with explicit operator authority"
)
if next_action == NEXT_ACTION_CLEANUP_OBSOLETE_LEASE:
cleanup_hint = {
"cleanup_tool": CLEANUP_OBSOLETE_LEASE_TOOL,
"required_confirmation": cleanup_confirmation_for_pr(pr_number),
"controller_recovery_authorized_required": True,
"leased_head": leased_head,
"current_head": current_head,
"expires_at": lease_for_class.get("expires_at"),
"terminal_review_verdict": (terminal or {}).get("verdict"),
}
else:
classification = "no_lease"
next_action = NEXT_ACTION_ACQUIRE
@@ -720,29 +1292,55 @@ def diagnose_reviewer_pr_lease_handoff(
)
lease_summary = None
if active:
if lease_for_class:
lease_summary = {
"comment_id": active.get("comment_id"),
"session_id": active.get("session_id"),
"phase": active.get("phase"),
"candidate_head": active.get("candidate_head"),
"expires_at": active.get("expires_at"),
"last_activity": active.get("last_activity"),
"freshness": active.get("freshness")
or classify_lease_freshness(active, now=now),
"reviewer_identity": active.get("reviewer_identity"),
"profile": active.get("profile"),
"worktree": active.get("worktree"),
"blocker": active.get("blocker"),
"comment_id": lease_for_class.get("comment_id"),
"session_id": lease_for_class.get("session_id"),
"phase": lease_for_class.get("phase"),
"candidate_head": lease_for_class.get("candidate_head"),
"expires_at": lease_for_class.get("expires_at"),
"last_activity": lease_for_class.get("last_activity"),
"freshness": lease_for_class.get("freshness")
or classify_lease_freshness(lease_for_class, now=now),
"reviewer_identity": lease_for_class.get("reviewer_identity"),
"profile": lease_for_class.get("profile"),
"worktree": lease_for_class.get("worktree"),
"blocker": lease_for_class.get("blocker"),
}
return {
"pr_number": pr_number,
"classification": classification,
"blocker_kind": classification,
"next_action": next_action,
"exact_next_action": next_action,
"active_lease": lease_summary,
"session_lease": session_lease,
"worktree_binding": binding_details,
"leased_head": (lease_summary or {}).get("candidate_head"),
"current_head": current_head,
"expires_at": (lease_summary or {}).get("expires_at"),
"terminal_review_state": (
formal_terminal_review_for_head(
formal_reviews,
leased_head=(lease_summary or {}).get("candidate_head"),
)
if lease_summary
else None
),
"worktree_state": {
"exists": worktree_exists,
"clean": worktree_clean,
"path": (lease_summary or {}).get("worktree"),
},
"owner_session_evidence": {
"session_id": (lease_summary or {}).get("session_id"),
"process_alive": owner_process_alive,
"pid_is_not_ownership_proof": True,
},
"cleanup": cleanup_hint,
"cleanup_tool": (cleanup_hint or {}).get("cleanup_tool"),
"required_confirmation": (cleanup_hint or {}).get("required_confirmation"),
"instructed_session_id": instructed_session_id,
"instructed_comment_id": instructed_comment_id,
"instructed_lease_missing_with_replacement": instructed_missing_with_replacement,
@@ -751,6 +1349,19 @@ def diagnose_reviewer_pr_lease_handoff(
and not binding_mismatch
and bool(session_lease)
),
"mutation_eligibility": (
"allowed"
if (
next_action == NEXT_ACTION_RESUME_EXACT_OWNER_SESSION
and not binding_mismatch
and bool(session_lease)
)
else (
"cleanup_only"
if next_action == NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
else "prohibited"
)
),
"reasons": reasons,
"forbidden": [
"manual lock deletion",
@@ -758,5 +1369,8 @@ def diagnose_reviewer_pr_lease_handoff(
"mtime manipulation",
"direct _SESSION_LEASE seeding",
"silent foreign lease steal",
"PID-based ownership",
"repointing old lease to new head",
"transfer of validation or decision state",
],
}
+10
View File
@@ -76,6 +76,16 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = {
"permission": "gitea.pr.comment",
"role": "reviewer",
},
# #691: guarded non-owner cleanup of obsolete comment-backed reviewer leases.
# Apply path posts lease release + audit comments (gitea.pr.comment).
"cleanup_obsolete_reviewer_comment_lease": {
"permission": "gitea.pr.comment",
"role": "reviewer",
},
"gitea_cleanup_obsolete_reviewer_comment_lease": {
"permission": "gitea.pr.comment",
"role": "reviewer",
},
"blind_pr_queue_review": {
"permission": "gitea.pr.review",
"role": "reviewer",
+39 -10
View File
@@ -57,9 +57,23 @@ class TestCreateIssueWorkspaceGuard(unittest.TestCase):
# Without worktree_path/env hints, workspace resolves to PROJECT_ROOT. When that
# path is the stable control checkout (not under branches/), mutation must fail.
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue(title="Test issue", body="body text")
self.assertIn("stable control checkout", str(ctx.exception))
try:
res = srv.gitea_create_issue(title="Test issue", body="body text")
except RuntimeError as exc:
self.assertIn("stable control checkout", str(exc))
else:
# #683: production guards return typed blockers at entrypoints
self.assertFalse(res.get("success"))
self.assertFalse(res.get("performed"))
blob = " ".join(res.get("reasons") or []) + " " + str(
res.get("blocker_kind") or ""
)
self.assertTrue(
"stable control checkout" in blob
or "missing_issue_worktree" in blob
or "control checkout" in blob.lower()
)
self.assertTrue(res.get("exact_next_action"))
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
@patch("gitea_mcp_server._profile_permission_block", return_value=None)
@@ -105,11 +119,17 @@ class TestCreateIssueWorkspaceGuard(unittest.TestCase):
)
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue(
try:
res = srv.gitea_create_issue(
title="Test issue", body="body", worktree_path=missing_path
)
self.assertIn("does not exist (fail closed)", str(ctx.exception))
except RuntimeError as exc:
self.assertIn("does not exist", str(exc))
else:
self.assertFalse(res.get("success"))
blob = " ".join(res.get("reasons") or [])
self.assertIn("does not exist", blob)
self.assertTrue(res.get("exact_next_action") or res.get("reasons"))
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
@patch("gitea_mcp_server._profile_permission_block", return_value=None)
@@ -142,11 +162,20 @@ class TestCreateIssueWorkspaceGuard(unittest.TestCase):
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
with patch("gitea_mcp_server._get_workspace_porcelain", return_value=""):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue(
title="Test issue", body="body", worktree_path=wrong_repo_path
try:
res = srv.gitea_create_issue(
title="Test issue",
body="body",
worktree_path=wrong_repo_path,
)
self.assertIn("does not belong to the target repository", str(ctx.exception))
except RuntimeError as exc:
self.assertIn(
"does not belong to the target repository", str(exc)
)
else:
self.assertFalse(res.get("success"))
blob = " ".join(res.get("reasons") or [])
self.assertIn("does not belong to the target repository", blob)
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
@patch("gitea_mcp_server._profile_permission_block", return_value=None)
+11 -2
View File
@@ -267,10 +267,19 @@ class TestReconcilerCommentThroughCanonicalPath(unittest.TestCase):
with patch.dict(os.environ, {}, clear=False):
os.environ.pop("GITEA_AUTHOR_WORKTREE", None)
os.environ.pop("GITEA_ACTIVE_WORKTREE", None)
with self.assertRaises(RuntimeError):
srv.gitea_create_issue_comment(
try:
res = srv.gitea_create_issue_comment(
515, "author note", remote="prgs"
)
except RuntimeError:
pass # legacy raise path
else:
# #683: typed blocker at mutation entrypoint
self.assertFalse(res.get("success"))
self.assertFalse(res.get("performed"))
self.assertTrue(
res.get("blocker_kind") or res.get("reasons")
)
mock_api.assert_not_called()
@@ -0,0 +1,472 @@
"""#683: block unattributed root WIP; pytest cannot disable production guards.
Regression coverage required by issue #683:
1. Session locked to issue A blocks unrelated target issue B until B is selected.
2. Diagnostic source edit on the root checkout is blocked.
3. Same legitimate edit succeeds after issue ownership + isolated worktree bind.
4. Running under pytest does not deactivate production guards when force-on.
5. Dirty tracked Python files remain visible to porcelain consumers.
6. Monkeypatching one helper cannot silently turn the full guard path into a no-op.
7. Real mutation entrypoint proves production guards run before side effects.
8. Same-issue edits in a valid isolated worktree remain unaffected.
9. Blocker includes stable reason + exact recovery action.
"""
from __future__ import annotations
import os
import sys
import tempfile
import textwrap
import unittest
from pathlib import Path
from unittest.mock import MagicMock, patch
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import gitea_mcp_server as mcp_server # noqa: E402
import issue_lock_worktree # noqa: E402
import workflow_scope_guard as wsg # noqa: E402
CONTROL_ROOT = str(Path(__file__).resolve().parent.parent)
if "branches" in Path(__file__).resolve().parts:
# Running from a worktree under branches/ — parent of branches is control.
parts = Path(__file__).resolve().parts
idx = parts.index("branches")
CONTROL_ROOT = str(Path(*parts[:idx])) if idx > 0 else CONTROL_ROOT
class TestProductionGuardsForceOn(unittest.TestCase):
def tearDown(self):
for key in (
wsg.FORCE_PRODUCTION_GUARDS_ENV,
"GITEA_TEST_FORCE_DIRTY",
"GITEA_TEST_PORCELAIN",
"GITEA_AUTHOR_WORKTREE",
"GITEA_ACTIVE_WORKTREE",
):
os.environ.pop(key, None)
wsg.clear_workflow_failure_ledger()
def test_force_on_under_pytest_keeps_production_active(self):
self.assertTrue(wsg.production_guards_active(in_test_mode=False))
self.assertFalse(wsg.production_guards_active(in_test_mode=True))
os.environ[wsg.FORCE_PRODUCTION_GUARDS_ENV] = "1"
self.assertTrue(wsg.production_guards_active(in_test_mode=True))
self.assertTrue(wsg.production_guards_forced())
def test_no_early_return_in_verify_role_mutation_workspace_source(self):
src = Path(mcp_server.__file__).read_text(encoding="utf-8")
# Rejected 300a4ca pattern must not exist.
self.assertNotIn(
"if _preflight_in_test_mode():\n return _resolve_preflight_workspace_path",
src,
)
# Docstring contract for #683.
self.assertIn("#683", src)
self.assertIn("must NOT early-return solely because pytest", src)
class TestPorcelainIntegrity(unittest.TestCase):
def test_read_worktree_git_state_surfaces_dirty_py(self):
with tempfile.TemporaryDirectory() as tmp:
# Use a real git repo so porcelain is truthful.
import subprocess
subprocess.run(["git", "init"], cwd=tmp, check=True, capture_output=True)
subprocess.run(
["git", "config", "user.email", "[email protected]"],
cwd=tmp,
check=True,
capture_output=True,
)
subprocess.run(
["git", "config", "user.name", "t"],
cwd=tmp,
check=True,
capture_output=True,
)
py_path = Path(tmp) / "sample_mod.py"
py_path.write_text("x = 1\n", encoding="utf-8")
subprocess.run(["git", "add", "sample_mod.py"], cwd=tmp, check=True)
subprocess.run(
["git", "commit", "-m", "init"],
cwd=tmp,
check=True,
capture_output=True,
)
py_path.write_text("x = 2\n", encoding="utf-8")
state = issue_lock_worktree.read_worktree_git_state(tmp)
porcelain = state.get("porcelain_status") or ""
self.assertIn("sample_mod.py", porcelain)
self.assertTrue(any(line.strip().endswith(".py") for line in porcelain.splitlines()))
def test_production_reader_source_rejects_pytest_py_filter(self):
src = Path(issue_lock_worktree.__file__).read_text(encoding="utf-8")
findings = wsg.assert_no_pytest_porcelain_filter(src)
self.assertEqual(findings, [])
# Negative: the rejected 300a4ca pattern is detected.
rejected = textwrap.dedent(
"""
porcelain = status_res.stdout or ""
import sys
if "pytest" in sys.modules or "unittest" in sys.modules:
porcelain = "\\n".join(
line for line in porcelain.splitlines()
if not line.strip().endswith(".py")
)
"""
)
self.assertTrue(wsg.assert_no_pytest_porcelain_filter(rejected))
class TestIssueScopeOwnership(unittest.TestCase):
def test_out_of_scope_issue_blocked_until_selected(self):
result = wsg.assess_issue_scope_ownership(
locked_issue_number=100,
target_issue_number=200,
branch_name="fix/issue-100-example",
role_kind="author",
)
self.assertTrue(result["block"])
self.assertEqual(result["blocker_kind"], wsg.BLOCKER_OUT_OF_SCOPE_ISSUE)
self.assertIn("exact_next_action", result)
self.assertIn("owning issue", result["exact_next_action"].lower())
self.assertTrue(result["reasons"])
def test_same_issue_scope_allowed(self):
result = wsg.assess_issue_scope_ownership(
locked_issue_number=100,
target_issue_number=100,
branch_name="fix/issue-100-example",
role_kind="author",
)
self.assertFalse(result["block"])
self.assertEqual(result["exact_next_action"], "proceed")
def test_missing_lock_when_required(self):
result = wsg.assess_issue_scope_ownership(
locked_issue_number=None,
target_issue_number=None,
role_kind="author",
require_lock_for_author=True,
)
self.assertTrue(result["block"])
self.assertEqual(result["blocker_kind"], wsg.BLOCKER_MISSING_ISSUE_SCOPE)
def test_branch_issue_mismatch(self):
result = wsg.assess_issue_scope_ownership(
locked_issue_number=50,
branch_name="fix/issue-99-other",
role_kind="author",
)
self.assertTrue(result["block"])
self.assertEqual(result["blocker_kind"], wsg.BLOCKER_OUT_OF_SCOPE_ISSUE)
class TestRootDiagnosticEdit(unittest.TestCase):
def test_dirty_root_source_blocked(self):
result = wsg.assess_root_source_mutation(
workspace_path=CONTROL_ROOT,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M gitea_mcp_server.py\n M tests/test_x.py\n",
role_kind="author",
)
self.assertTrue(result["block"])
self.assertEqual(result["blocker_kind"], wsg.BLOCKER_ROOT_DIAGNOSTIC_EDIT)
self.assertIn("gitea_mcp_server.py", result["dirty_source_files"])
self.assertIn("exact_next_action", result)
self.assertIn("branches/", result["exact_next_action"])
def test_isolated_worktree_same_issue_unaffected(self):
wt = f"{CONTROL_ROOT}/branches/issue-100-example"
result = wsg.assess_root_source_mutation(
workspace_path=wt,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M helper.py\n",
current_branch="fix/issue-100-example",
locked_issue_number=100,
role_kind="author",
)
self.assertFalse(result["block"])
self.assertTrue(result["under_branches"])
def test_legitimate_after_ownership_and_worktree(self):
wt = f"{CONTROL_ROOT}/branches/issue-683-workflow-guard-hardening"
composed = wsg.assess_production_mutation_guards(
workspace_path=wt,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M workflow_scope_guard.py\n",
current_branch="fix/issue-683-workflow-guard-hardening",
locked_issue_number=683,
target_issue_number=683,
role_kind="author",
require_author_lock=True,
in_test_mode=True,
)
# Force-on required for production path under pytest.
os.environ[wsg.FORCE_PRODUCTION_GUARDS_ENV] = "1"
try:
composed = wsg.assess_production_mutation_guards(
workspace_path=wt,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M workflow_scope_guard.py\n",
current_branch="fix/issue-683-workflow-guard-hardening",
locked_issue_number=683,
target_issue_number=683,
role_kind="author",
require_author_lock=True,
in_test_mode=True,
)
self.assertFalse(composed["block"])
self.assertFalse(composed.get("skipped"))
finally:
os.environ.pop(wsg.FORCE_PRODUCTION_GUARDS_ENV, None)
class TestTypedBlockerResponse(unittest.TestCase):
def test_block_response_has_stable_kind_and_next_action(self):
assessment = wsg.assess_issue_scope_ownership(
locked_issue_number=1,
target_issue_number=2,
role_kind="author",
)
resp = wsg.block_response(assessment)
self.assertFalse(resp["success"])
self.assertFalse(resp["performed"])
self.assertEqual(resp["blocker_kind"], wsg.BLOCKER_OUT_OF_SCOPE_ISSUE)
self.assertIsInstance(resp["exact_next_action"], str)
self.assertTrue(resp["exact_next_action"])
self.assertTrue(resp["reasons"])
def test_production_guard_error_roundtrip(self):
err = wsg.ProductionGuardError(
"blocked",
blocker_kind=wsg.BLOCKER_ROOT_DIAGNOSTIC_EDIT,
reasons=["dirty root"],
)
resp = wsg.block_response(err, issue_number=683)
self.assertEqual(resp["blocker_kind"], wsg.BLOCKER_ROOT_DIAGNOSTIC_EDIT)
self.assertEqual(resp["issue_number"], 683)
self.assertIn("exact_next_action", resp)
class TestDurableFailureRecording(unittest.TestCase):
def setUp(self):
wsg.clear_workflow_failure_ledger()
def tearDown(self):
wsg.clear_workflow_failure_ledger()
def test_record_before_source_mutation(self):
pending = wsg.assess_durable_failure_recorded(
require_record=True, pending_source_mutation=True
)
self.assertTrue(pending["block"])
self.assertEqual(pending["blocker_kind"], wsg.BLOCKER_UNRECORDED_FAILURE)
wsg.record_workflow_failure(
kind="transport_eof",
detail="EOF during review session (#584 cluster)",
issue_number=683,
task="comment_issue",
)
after = wsg.assess_durable_failure_recorded(
require_record=True, pending_source_mutation=True
)
self.assertFalse(after["block"])
self.assertEqual(len(wsg.workflow_failure_ledger()), 1)
class TestMonkeypatchCannotNoopFullPath(unittest.TestCase):
def tearDown(self):
os.environ.pop(wsg.FORCE_PRODUCTION_GUARDS_ENV, None)
def test_patching_branches_only_still_blocks_dirty_root_scope(self):
"""Monkeypatching branches-only must not silence root diagnostic block."""
os.environ[wsg.FORCE_PRODUCTION_GUARDS_ENV] = "1"
with patch.object(
mcp_server, "_enforce_branches_only_author_mutation", lambda *a, **k: None
):
with patch.object(
mcp_server, "_enforce_root_checkout_guard", lambda *a, **k: None
):
# Even if both legacy helpers are patched, issue-scope composition
# still sees dirty root source via assess_production_mutation_guards.
assessment = wsg.assess_production_mutation_guards(
workspace_path=CONTROL_ROOT,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M gitea_mcp_server.py\n",
role_kind="author",
in_test_mode=True,
)
self.assertTrue(assessment["block"])
self.assertEqual(
assessment["blocker_kind"], wsg.BLOCKER_ROOT_DIAGNOSTIC_EDIT
)
class TestRealEntrypointProductionGuard(unittest.TestCase):
"""Real mutation entrypoint: production guard before side effects (#683)."""
def setUp(self):
os.environ[wsg.FORCE_PRODUCTION_GUARDS_ENV] = "1"
for key in ("GITEA_AUTHOR_WORKTREE", "GITEA_ACTIVE_WORKTREE"):
os.environ.pop(key, None)
self._orig_whoami = mcp_server._preflight_whoami_called
self._orig_cap = mcp_server._preflight_capability_called
mcp_server._preflight_whoami_called = False
mcp_server._preflight_capability_called = False
mcp_server._preflight_resolved_role = None
mcp_server._preflight_resolved_task = None
def tearDown(self):
os.environ.pop(wsg.FORCE_PRODUCTION_GUARDS_ENV, None)
for key in ("GITEA_AUTHOR_WORKTREE", "GITEA_ACTIVE_WORKTREE"):
os.environ.pop(key, None)
mcp_server._preflight_whoami_called = self._orig_whoami
mcp_server._preflight_capability_called = self._orig_cap
mcp_server._preflight_resolved_role = None
mcp_server._preflight_resolved_task = None
def test_comment_issue_blocks_dirty_root_before_api(self):
api_mock = MagicMock()
with patch.object(mcp_server, "api_request", api_mock), patch.object(
mcp_server,
"_actual_profile_role",
return_value="author",
), patch.object(
mcp_server,
"_effective_workspace_role",
return_value="author",
), patch.object(
mcp_server,
"get_profile",
return_value={
"profile_name": "prgs-author",
"allowed_operations": [
"gitea.issue.comment",
"gitea.read",
"gitea.pr.create",
"gitea.branch.push",
],
"forbidden_operations": [],
},
), patch.object(
issue_lock_worktree,
"read_worktree_git_state",
side_effect=lambda path, **kw: {
"current_branch": "master",
"porcelain_status": (
" M gitea_mcp_server.py\n"
if os.path.realpath(path) == os.path.realpath(CONTROL_ROOT)
or path == CONTROL_ROOT
else ""
),
"head_sha": "a" * 40,
"base_equivalent": True,
},
), patch.object(
mcp_server,
"_resolve_namespace_mutation_context",
return_value={
"workspace_path": CONTROL_ROOT,
"canonical_repo_root": CONTROL_ROOT,
"process_project_root": CONTROL_ROOT,
"workspace_role_kind": "author",
"workspace_binding_source": "process root",
"ignored_bindings": [],
},
), patch.object(
mcp_server,
"_resolve_author_mutation_context",
return_value={
"workspace_path": CONTROL_ROOT,
"canonical_repo_root": CONTROL_ROOT,
"process_project_root": CONTROL_ROOT,
"roots_aligned": True,
},
), patch.object(
mcp_server,
"_session_locked_issue_number",
return_value=None,
):
result = mcp_server.gitea_create_issue_comment(
issue_number=683,
body="diagnostic note",
remote="prgs",
org="Scaled-Tech-Consulting",
repo="Gitea-Tools",
worktree_path=CONTROL_ROOT,
)
api_mock.assert_not_called()
self.assertFalse(result.get("success"))
self.assertFalse(result.get("performed"))
self.assertIn(result.get("blocker_kind"), wsg.BLOCKER_KINDS)
self.assertTrue(result.get("exact_next_action"))
self.assertTrue(result.get("reasons"))
def test_comment_issue_succeeds_structure_after_worktree_bind(self):
"""Same-issue isolated worktree is not blocked by root diagnostic path."""
wt = f"{CONTROL_ROOT}/branches/issue-683-workflow-guard-hardening"
os.environ["GITEA_AUTHOR_WORKTREE"] = wt
assessment = wsg.assess_production_mutation_guards(
workspace_path=wt,
canonical_repo_root=CONTROL_ROOT,
porcelain_status=" M workflow_scope_guard.py\n",
current_branch="fix/issue-683-workflow-guard-hardening",
locked_issue_number=683,
target_issue_number=683,
role_kind="author",
require_author_lock=True,
in_test_mode=True,
)
self.assertFalse(assessment["block"], assessment)
class TestVerifyPreflightForceOn(unittest.TestCase):
def tearDown(self):
os.environ.pop(wsg.FORCE_PRODUCTION_GUARDS_ENV, None)
for key in ("GITEA_AUTHOR_WORKTREE", "GITEA_ACTIVE_WORKTREE"):
os.environ.pop(key, None)
def test_force_on_runs_production_guards_under_pytest(self):
os.environ[wsg.FORCE_PRODUCTION_GUARDS_ENV] = "1"
called = {"root": 0, "branches": 0, "scope": 0}
def _root(*a, **k):
called["root"] += 1
def _branches(*a, **k):
called["branches"] += 1
def _scope(*a, **k):
called["scope"] += 1
with patch.object(mcp_server, "_enforce_root_checkout_guard", _root), patch.object(
mcp_server, "_enforce_branches_only_author_mutation", _branches
), patch.object(mcp_server, "_enforce_issue_scope_guard", _scope):
# No whoami/capability — purity-order skipped; production still runs.
mcp_server.verify_preflight_purity(task="comment_issue")
self.assertEqual(called["root"], 1)
self.assertEqual(called["branches"], 1)
self.assertEqual(called["scope"], 1)
def test_without_force_on_pytest_skips_production_only_for_unit_isolation(self):
called = {"root": 0}
def _root(*a, **k):
called["root"] += 1
with patch.object(mcp_server, "_enforce_root_checkout_guard", _root), patch.object(
mcp_server, "_enforce_branches_only_author_mutation", lambda *a, **k: None
), patch.object(mcp_server, "_enforce_issue_scope_guard", lambda *a, **k: None):
mcp_server.verify_preflight_purity(task="comment_issue")
self.assertEqual(called["root"], 0)
if __name__ == "__main__":
unittest.main()
@@ -0,0 +1,599 @@
"""Guarded cleanup for obsolete comment-backed reviewer leases (#691).
Reproduces PR #688 lease comment 10749 class of defect: completed
REQUEST_CHANGES on head A, author pushes head B, foreign lease remains
pinned to A (and after expiry still has no non-owner cleanup path that
diagnosis can prescribe beyond indefinite wait).
"""
from __future__ import annotations
import sys
import unittest
from datetime import datetime, timedelta, timezone
from pathlib import Path
from unittest.mock import patch
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import merger_lease_adoption as mla # noqa: E402
import reviewer_pr_lease as leases # noqa: E402
# PR #688 / comment 10749 reproduction fixtures
HEAD_A = "c7a444eb4b41cf916fdbd20a4999ffd78af496d0"
HEAD_B = "4a6357800364718a27a36ebc73578d4b929ff4aa"
SESSION_FOREIGN = "25883-7d4c6e6ebd53"
COMMENT_10749 = 10749
REPO = "Scaled-Tech-Consulting/Gitea-Tools"
PR = 688
ISSUE = 687
EXPIRES_10749 = "2026-07-13T04:23:00Z"
def _utc(dt: datetime) -> datetime:
return dt if dt.tzinfo else dt.replace(tzinfo=timezone.utc)
def _lease_comment(
*,
pr_number: int = PR,
session_id: str = SESSION_FOREIGN,
phase: str = "claimed",
candidate_head: str = HEAD_A,
comment_id: int = COMMENT_10749,
last_activity: datetime | None = None,
expires_at: datetime | None = None,
worktree: str = "branches/review-pr688-feat-issue-687-reconciler-branch-delete",
repo: str = REPO,
) -> dict:
now = last_activity or datetime.now(timezone.utc)
body = leases.format_lease_body(
repo=repo,
pr_number=pr_number,
issue_number=ISSUE,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id=session_id,
worktree=worktree,
phase=phase,
candidate_head=candidate_head,
target_branch="master",
target_branch_sha="b" * 40,
last_activity=now,
expires_at=expires_at,
)
return {
"id": comment_id,
"body": body,
"user": {"login": "sysadmin"},
"author": "sysadmin",
"created_at": now.isoformat(),
"updated_at": now.isoformat(),
}
def _reviews_for_head(head: str, verdict: str = "REQUEST_CHANGES") -> list[dict]:
return [
{
"verdict": verdict,
"reviewed_head_sha": head,
"dismissed": False,
"reviewer": "sysadmin",
}
]
def _assess(
comments,
*,
current_head=HEAD_B,
formal_reviews=None,
controller=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
requesting_session="fresh-controller",
apply=False,
confirmation=None,
**kwargs,
):
return leases.assess_obsolete_reviewer_comment_lease_cleanup(
comments,
pr_number=PR,
current_head_sha=current_head,
formal_reviews=formal_reviews if formal_reviews is not None else _reviews_for_head(HEAD_A),
repo=REPO,
expected_repo=REPO,
requesting_session_id=requesting_session,
controller_recovery_authorized=controller,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
worktree_has_unpreserved_work=not worktree_clean if worktree_exists else False,
owner_process_alive=owner_process_alive,
owner_pid_observed=kwargs.get("owner_pid_observed"),
requesting_pid=kwargs.get("requesting_pid", 99999),
current_head_review_in_progress=kwargs.get(
"current_head_review_in_progress", False
),
expected_lease_comment_id=kwargs.get("expected_lease_comment_id"),
expected_session_id=kwargs.get("expected_session_id"),
expected_leased_head=kwargs.get("expected_leased_head"),
confirmation=confirmation
if confirmation is not None
else (leases.cleanup_confirmation_for_pr(PR) if apply else ""),
apply=apply,
now=kwargs.get("now"),
)
class TestIssue691SupersededHeadCleanup(unittest.TestCase):
def setUp(self):
leases.clear_session_lease()
def test_1_request_changes_then_push_expired_cleanup_succeeds(self):
"""Completed REQUEST_CHANGES on A, push B, lease A expires → cleanup OK."""
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
last_activity=expires - timedelta(hours=2),
expires_at=expires,
)
result = _assess(
[claim],
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
now=now,
apply=True,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "foreign_expired_superseded_head")
self.assertEqual(
result["exact_next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
self.assertIn("phase: released", result["release_body"])
self.assertIn("obsolete-superseded-or-expired-lease", result["release_body"])
self.assertIsNotNone(result["audit_comment_body"])
self.assertEqual(result["cleanup_tool"], leases.CLEANUP_OBSOLETE_LEASE_TOOL)
def test_2_approve_then_push_cleanup_policy(self):
"""Completed APPROVE on A, push B → cleanup eligible (completed superseded)."""
now = datetime(2026, 7, 13, 1, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc) # not yet expired
claim = _lease_comment(
last_activity=now - timedelta(minutes=10),
expires_at=expires,
)
result = _assess(
[claim],
formal_reviews=_reviews_for_head(HEAD_A, "APPROVED"),
now=now,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "foreign_completed_superseded_head")
def test_3_active_current_head_cleanup_denied(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=5),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_B, "REQUEST_CHANGES"),
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertEqual(result["classification"], "foreign_active_current_head")
def test_4_foreign_owner_recent_progress_denied(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=2),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
owner_process_alive=True,
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(
any("recent authenticated progress" in r for r in result["reasons"])
or any("current PR head" in r for r in result["reasons"])
)
def test_5_owner_absent_worktree_dirty_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=expires - timedelta(hours=1),
expires_at=expires,
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
worktree_clean=False,
owner_process_alive=False,
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("dirty" in r for r in result["reasons"]))
def test_6_owner_absent_worktree_clean_orphan_ok(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=expires - timedelta(hours=1),
expires_at=expires,
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
worktree_clean=True,
owner_process_alive=False,
now=now,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "orphaned_owner_missing")
def test_7_pid_reuse_not_ownership(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim],
now=now,
owner_pid_observed=4242,
requesting_pid=4242,
)
self.assertTrue(
any("PID equality" in r or "NOT ownership" in r for r in result["reasons"])
or result["owner_session_evidence"]["pid_is_not_ownership_proof"]
)
# Still eligible via superseded+terminal+expired despite matching PID.
self.assertTrue(result["cleanup_allowed"], result["reasons"])
def test_8_comment_backed_no_db_lease_id(self):
"""Cleanup uses comment ledger only — no control-plane lease_id required."""
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
# Explicitly no lease_id field anywhere.
self.assertNotIn("lease_id", claim)
result = _assess([claim], now=now)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["active_lease"]["comment_id"], COMMENT_10749)
self.assertIsNone(result["active_lease"].get("lease_id"))
def test_9_cleanup_posts_durable_audit_bodies(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], now=now, apply=True)
self.assertTrue(result["cleanup_allowed"])
self.assertIn("#691", result["audit_comment_body"])
self.assertIn(str(COMMENT_10749), result["audit_comment_body"])
self.assertIn(HEAD_A, result["audit_comment_body"])
self.assertIn(HEAD_B, result["audit_comment_body"])
def test_10_fresh_acquire_after_cleanup_marker(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
expires_at=expires,
last_activity=expires - timedelta(hours=1),
comment_id=COMMENT_10749,
)
assessed = _assess([claim], now=now, apply=True)
self.assertTrue(assessed["cleanup_allowed"])
release = {
"id": 99999,
"body": assessed["release_body"],
"user": {"login": "sysadmin"},
}
# Newest terminal marker ends active lease.
active = leases.find_active_reviewer_lease(
[claim, release], pr_number=PR, now=now
)
self.assertIsNone(active)
acq = leases.assess_acquire_lease(
[claim, release],
pr_number=PR,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id="fresh-reviewer-1",
repo=REPO,
issue_number=ISSUE,
worktree="branches/review-pr-688-fresh",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=now,
)
self.assertTrue(acq["acquire_allowed"], acq["reasons"])
def test_11_no_validation_state_transfer(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], now=now, apply=True)
self.assertTrue(result["cleanup_allowed"])
# Release body keeps old candidate_head (no repoint).
self.assertIn(f"candidate_head: {HEAD_A}", result["release_body"])
self.assertNotIn(HEAD_B, result["release_body"].split("candidate_head:")[1].split("\n")[0])
self.assertIn("did not transfer validation", result["audit_comment_body"])
self.assertIn("did not repoint", result["audit_comment_body"])
# Session lease must remain unset by assessment (tool never seeds).
self.assertIsNone(leases.get_session_lease())
def test_12_repeated_cleanup_idempotent(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
first = _assess([claim], now=now, apply=True)
self.assertTrue(first["cleanup_allowed"])
release = {"id": 100001, "body": first["release_body"], "user": {"login": "x"}}
second = _assess([claim, release], now=now, apply=True)
self.assertFalse(second["cleanup_allowed"])
self.assertEqual(second["classification"], "no_lease")
def test_13_malformed_expiry_fails_closed(self):
claim = _lease_comment()
# Corrupt expires_at in the body.
claim["body"] = claim["body"].replace(
claim["body"].split("expires_at:")[1].split("\n")[0].strip(),
"not-a-timestamp",
)
result = _assess([claim], formal_reviews=_reviews_for_head(HEAD_A))
self.assertFalse(result["cleanup_allowed"])
self.assertFalse(result["expires_parseable"])
self.assertTrue(any("expires_at" in r for r in result["reasons"]))
def test_14_wrong_identity_evidence_fails_closed(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
bad_repo = _assess(
[claim],
now=now,
# force repo mismatch via expected_repo
)
# Patch via direct call with wrong expected_repo
bad = leases.assess_obsolete_reviewer_comment_lease_cleanup(
[claim],
pr_number=PR,
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A),
expected_repo="Other-Org/Other-Repo",
requesting_session_id="fresh",
controller_recovery_authorized=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
now=now,
)
self.assertFalse(bad["cleanup_allowed"])
self.assertTrue(any("repository identity" in r for r in bad["reasons"]))
bad_pr = leases.assess_obsolete_reviewer_comment_lease_cleanup(
[claim],
pr_number=999,
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A),
expected_repo=REPO,
requesting_session_id="fresh",
controller_recovery_authorized=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
expected_lease_comment_id=COMMENT_10749,
now=now,
)
self.assertFalse(bad_pr["cleanup_allowed"])
bad_head = _assess(
[claim],
now=now,
expected_leased_head="0" * 40,
)
self.assertFalse(bad_head["cleanup_allowed"])
bad_session = _assess(
[claim],
now=now,
expected_session_id="wrong-session",
)
self.assertFalse(bad_session["cleanup_allowed"])
def test_15_current_head_active_cannot_be_displaced(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=1),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_B),
now=now,
current_head_review_in_progress=True,
)
self.assertFalse(result["cleanup_allowed"])
# Acquire also blocked by foreign active lease.
acq = leases.assess_acquire_lease(
[claim],
pr_number=PR,
reviewer_identity="other",
profile="prgs-reviewer",
session_id="thief",
repo=REPO,
issue_number=ISSUE,
worktree="branches/steal",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=now,
)
self.assertFalse(acq["acquire_allowed"])
def test_regression_pr688_comment_10749_after_expiry(self):
"""Exact 10749 reproduction after recorded expires_at 2026-07-13T04:23:00Z."""
now = datetime(2026, 7, 13, 4, 30, 0, tzinfo=timezone.utc)
expires = datetime.fromisoformat(EXPIRES_10749.replace("Z", "+00:00"))
claim = _lease_comment(
session_id=SESSION_FOREIGN,
candidate_head=HEAD_A,
comment_id=COMMENT_10749,
last_activity=expires - timedelta(hours=2),
expires_at=expires,
)
# Diagnosis must not prescribe indefinite wait-only for this case.
diag = leases.diagnose_reviewer_pr_lease_handoff(
[claim],
pr_number=PR,
current_session_id="fresh-reviewer",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
instructed_session_id=SESSION_FOREIGN,
instructed_comment_id=COMMENT_10749,
now=now,
)
self.assertEqual(diag["classification"], "foreign_expired_superseded_head")
self.assertEqual(
diag["next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
self.assertEqual(diag["cleanup_tool"], leases.CLEANUP_OBSOLETE_LEASE_TOOL)
self.assertIn("CLEANUP OBSOLETE REVIEWER LEASE 688", diag["required_confirmation"])
self.assertEqual(diag["mutation_eligibility"], "cleanup_only")
self.assertFalse(diag["mutation_allowed"])
# Assessment still returns the lease as active/stale class of block for acquire
# when unexpired path... here it's expired so find_active is None; acquire free
# only after cleanup if somehow still active. With expired, find_active is None:
active = leases.find_active_reviewer_lease([claim], pr_number=PR, now=now)
self.assertIsNone(active)
# But superseded unexpired still blocks acquire and diagnosis points to cleanup:
unexpired_now = datetime(2026, 7, 13, 1, 0, 0, tzinfo=timezone.utc)
claim2 = _lease_comment(
session_id=SESSION_FOREIGN,
candidate_head=HEAD_A,
comment_id=COMMENT_10749,
last_activity=unexpired_now - timedelta(minutes=45),
expires_at=expires,
)
active2 = leases.find_active_reviewer_lease(
[claim2], pr_number=PR, now=unexpired_now
)
self.assertIsNotNone(active2)
self.assertEqual(active2["freshness"], "stale_warning")
diag2 = leases.diagnose_reviewer_pr_lease_handoff(
[claim2],
pr_number=PR,
current_session_id="fresh-reviewer",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
worktree_exists=True,
worktree_clean=True,
now=unexpired_now,
)
self.assertEqual(diag2["classification"], "foreign_completed_superseded_head")
self.assertEqual(
diag2["next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
acq = leases.assess_acquire_lease(
[claim2],
pr_number=PR,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id="fresh-reviewer",
repo=REPO,
issue_number=ISSUE,
worktree="branches/review-pr-688-new",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=unexpired_now,
)
self.assertFalse(acq["acquire_allowed"])
def test_missing_controller_auth_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], controller=False, now=now)
self.assertFalse(result["cleanup_allowed"])
def test_wrong_confirmation_denied_on_apply(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim], now=now, apply=True, confirmation="MERGE PR 688"
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("confirmation" in r for r in result["reasons"]))
def test_owner_session_must_use_owner_release(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim],
now=now,
requesting_session=SESSION_FOREIGN,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("owns the lease" in r for r in result["reasons"]))
def test_superseded_without_terminal_review_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], formal_reviews=[], now=now)
self.assertFalse(result["cleanup_allowed"])
self.assertEqual(result["classification"], "ambiguous_conflicting_evidence")
class TestIssue691DiagnoseClassifications(unittest.TestCase):
def setUp(self):
leases.clear_session_lease()
def test_foreign_active_current_head_still_wait(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=5),
expires_at=now + timedelta(hours=1),
)
claim["id"] = 1
result = leases.diagnose_reviewer_pr_lease_handoff(
[claim],
pr_number=PR,
current_session_id="other",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=[],
now=now,
)
self.assertEqual(result["classification"], "foreign_active_current_head")
self.assertEqual(result["next_action"], leases.NEXT_ACTION_WAIT)
self.assertFalse(result["mutation_allowed"])
if __name__ == "__main__":
unittest.main()
@@ -0,0 +1,461 @@
"""Regression tests for #698: final-report validator vs canonical schema.
Covers the original #698 lead plus the independent reproduction recorded
during the PR #703 formal review (issue #698 comment 11246):
1. non-dict ``action_log`` entries must fail structured, never crash;
2. the validator must not demand legacy fields the canonical schema forbids
(``Pinned reviewed head``, ``Scratch worktree used``, ``Worktree path``,
``Worktree dirty``, ``Mutations``, ``Next``);
3. a legitimately blocked report (``Candidate head SHA: none``, no formal
verdict) must not owe approval/merge live-head proofs;
4. canonical reviewer lease release must not be misclassified as post-merge
cleanup;
5. structured workflow-load and validation proof must be recognized;
6. review mutations are inferred only from authoritative evidence.
"""
from __future__ import annotations
import sys
import unittest
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import final_report_validator as frv # noqa: E402
import post_merge_cleanup_proof as pmcp # noqa: E402
import pr_work_lease as pwl # noqa: E402
import review_proofs as rp # noqa: E402
from review_final_report_schema import ( # noqa: E402
assess_review_final_report_schema,
)
REVIEWED_HEAD = "a" * 40
LIVE_HEAD = "a" * 40
def _pr703_style_report(
*,
decision: str = "request_changes",
cleanup_mutations: str = (
"released reviewer PR lease via gitea_release_reviewer_pr_lease "
"(terminal lease marker phase=released posted)"
),
) -> str:
"""Canonical-schema report modeled on the PR #703 formal review handoff."""
return f"""
Formal review completed with a REQUEST_CHANGES verdict submitted and read
back via the native review API.
## Controller Handoff
- Task: review-merge-pr
- Repo: Scaled-Tech-Consulting/Gitea-Tools
- Role: reviewer
- Identity: sysadmin / prgs-reviewer
- Active profile: prgs-reviewer
- Runtime context: neutral workspace binding
- Selected PR: 703
- Linked issue: #702 open
- Eligibility class: reviewable
- Queue ordering policy: oldest eligible first
- Inventory pagination proof: has_more=false, total_count=8
- Earlier PRs skipped: none
- Candidate head SHA: {REVIEWED_HEAD}
- Reviewed head SHA: {REVIEWED_HEAD}
- Target branch: master
- Target branch SHA: {"2" * 40}
- Already-landed gate: not landed
- Author-safety result: pass (author differs from reviewer)
- Prior request-changes state: none
- Review worktree used: true
- Review worktree path: branches/review-pr-703-independent
- Review worktree inside branches: true
- Review worktree HEAD state: detached at pinned head
- Review worktree dirty before validation: clean
- Review worktree dirty after validation: clean
- Baseline worktree used: false
- Baseline worktree path: none
- Files reviewed: 4
- Validation: focused 50 passed; related 94 passed; full 2665 passed, 6 skipped
- Official validation integrity status: intact
- Terminal review mutation: one REQUEST_CHANGES review submitted and read back
- Review decision: {decision}
- Merge preflight: not run
- Merge result: none
- Linked issue status: open (live fetch proof: gitea_view_issue)
- Main checkout branch: master
- Main checkout dirty state: clean
- Main checkout updated: false
- File edits by reviewer: none
- Worktree/index mutations: none
- Git ref mutations: git fetch prgs (recorded)
- MCP/Gitea mutations: review submission and lease comments only
- Review mutations: one formal REQUEST_CHANGES verdict
- Merge mutations: none
- Cleanup mutations: {cleanup_mutations}
- External-state mutations: none
- Read-only diagnostics: gitea_view_pr, gitea_get_pr_review_feedback
- Blockers: findings F1-F6 recorded on the PR thread
- Current status: review complete; author remediation required
- Safe next action: author addresses findings and pushes a new head
- Safety statement: no merge attempted; no self-review; no root-checkout edits
- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean
- Live head SHA before approval: {LIVE_HEAD}
- Pushes occurred during validation: no
"""
def _blocked_preflight_report() -> str:
"""Blocked-run report modeled on the #702 comment 11164 reproduction."""
return """
Fresh review preflight stopped before any worktree or validation work.
## Controller Handoff
- Task: review-merge-pr
- Repo: Scaled-Tech-Consulting/Gitea-Tools
- Role: reviewer
- Identity: sysadmin / prgs-reviewer
- Active profile: prgs-reviewer
- Runtime context: stale workspace binding detected
- Selected PR: 701
- Linked issue: #699 open
- Eligibility class: blocked-before-validation
- Queue ordering policy: oldest eligible first
- Inventory pagination proof: has_more=false, total_count=8
- Earlier PRs skipped: none
- Candidate head SHA: none
- Reviewed head SHA: none
- Target branch: master
- Target branch SHA: none
- Already-landed gate: not run
- Author-safety result: not run
- Prior request-changes state: none
- Review worktree used: false
- Review worktree path: none
- Review worktree inside branches: not applicable
- Review worktree HEAD state: not applicable
- Review worktree dirty before validation: not applicable
- Review worktree dirty after validation: not applicable
- Baseline worktree used: false
- Baseline worktree path: none
- Files reviewed: 0
- Validation: not run
- Official validation integrity status: not applicable
- Terminal review mutation: none
- Review decision: none
- Merge preflight: not run
- Merge result: none
- Linked issue status: open (live fetch proof: gitea_view_issue)
- Main checkout branch: master
- Main checkout dirty state: clean
- Main checkout updated: false
- File edits by reviewer: none
- Worktree/index mutations: none
- Git ref mutations: none
- MCP/Gitea mutations: none
- Review mutations: none
- Merge mutations: none
- Cleanup mutations: none
- External-state mutations: none
- Read-only diagnostics: gitea_view_pr, gitea_get_runtime_context
- Blockers: runtime bound to a foreign task worktree; mutation prohibited
- Current status: stopped before validation began
- Next actor: operator
- Next action: repair the runtime workspace binding, then rerun the full
review workflow in a fresh reviewer session
- Next prompt: Act as REVIEWER for PR 701 after the operator repairs the
runtime binding; acquire the lease before any validation.
- Safe next action: operator repairs runtime binding, then a fresh reviewer
reruns the full workflow
- Safety statement: no lease acquired; no verdict recorded; no source edits
- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean
"""
class TestActionLogRobustness(unittest.TestCase):
"""#698 original lead: non-dict action_log must not crash validation."""
MALFORMED = [
"git fetch prgs",
42,
None,
{"action": "edit", "path": "x.py", "performed": True, "tracked": True},
]
def test_assess_final_report_validator_survives_malformed_entries(self):
result = frv.assess_final_report_validator(
_pr703_style_report(),
"review_pr",
action_log=self.MALFORMED,
)
self.assertIsInstance(result, dict)
rule_ids = {f["rule_id"] for f in result["findings"]}
self.assertIn("shared.action_log_malformed", rule_ids)
def test_malformed_entry_errors_are_sanitized(self):
_entries, findings = frv.sanitize_action_log(["secret-token-abc123"])
self.assertEqual(len(findings), 1)
reason = findings[0]["reason"]
self.assertNotIn("secret-token-abc123", reason)
self.assertIn("str", reason)
self.assertIn("entry 0", reason)
def test_non_list_action_log_is_reported_not_raised(self):
entries, findings = frv.sanitize_action_log("not-a-list")
self.assertEqual(entries, [])
self.assertEqual(len(findings), 1)
self.assertIn("not a list", findings[0]["reason"])
def test_performed_file_mutations_skips_non_dict_entries(self):
performed = rp._performed_file_mutations(
["oops", {"action": "edited", "path": "a.py"}]
)
self.assertEqual(len(performed), 1)
self.assertEqual(performed[0]["path"], "a.py")
def test_schema_entrypoint_survives_string_only_log(self):
result = assess_review_final_report_schema(
_pr703_style_report(),
action_log=["just a string", "another string"],
)
self.assertIsInstance(result, dict)
class TestLegacyFieldRequirementsRemoved(unittest.TestCase):
"""#698: prohibited legacy fields must not be REQUIRED of reports."""
PROHIBITED = (
"Pinned reviewed head",
"Scratch worktree used",
"Worktree path",
"Worktree dirty",
"Workspace mutations",
"Mutations",
"Next",
"Issue/PR",
"Branch/SHA",
"Files changed",
)
def test_review_role_field_table_has_no_prohibited_requirements(self):
names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["review"]]
for prohibited in ("Pinned reviewed head", "Scratch worktree used",
"Worktree path", "Worktree dirty"):
self.assertNotIn(prohibited, names)
def test_merger_role_field_table_has_no_pinned_reviewed_head(self):
names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["merger"]]
self.assertNotIn("Pinned reviewed head", names)
def test_canonical_report_missing_fields_never_include_prohibited(self):
result = rp.assess_controller_handoff(
_pr703_style_report(), role="review"
)
for prohibited in self.PROHIBITED:
self.assertNotIn(prohibited, result.get("missing_fields") or [])
def test_canonical_pr703_report_satisfies_required_fields(self):
result = rp.assess_controller_handoff(
_pr703_style_report(), role="review"
)
self.assertEqual(result.get("missing_fields") or [], [])
self.assertEqual(result.get("verdict"), "complete")
class TestBlockedReportAccepted(unittest.TestCase):
"""#698: blocked run with no reviewed head / verdict is legitimate."""
def test_stale_head_proof_waived_before_validation(self):
result = pwl.assess_reviewer_stale_head_final_report(
_blocked_preflight_report()
)
self.assertTrue(result["proven"])
self.assertEqual(result.get("phase"), "blocked_before_validation")
def test_blocked_report_passes_schema_validation(self):
result = assess_review_final_report_schema(_blocked_preflight_report())
blocking = [
f for f in result["findings"] if f["severity"] == "block"
]
self.assertEqual(blocking, [], blocking)
def test_verdict_phase_still_demands_approval_head_proof(self):
report = _blocked_preflight_report().replace(
"- Review decision: none",
"- Review decision: approve",
).replace(
"- Candidate head SHA: none",
f"- Candidate head SHA: {REVIEWED_HEAD}",
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
joined = " ".join(result["reasons"])
self.assertIn("before approval", joined)
def test_merge_phase_still_demands_merge_head_proof(self):
report = _pr703_style_report().replace(
"- Merge result: none",
"- Merge result: merged",
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
self.assertIn(
"final live head SHA before merge not stated",
result["reasons"],
)
def test_validation_phase_demands_push_disclosure(self):
report = _pr703_style_report().replace(
"- Pushes occurred during validation: no\n", ""
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
self.assertIn(
"whether push occurred during validation not stated",
result["reasons"],
)
class TestLeaseReleaseVsPostMergeCleanup(unittest.TestCase):
"""#698 (PR #703 review reproduction): lease release is not cleanup."""
def test_lease_release_cleanup_mutations_do_not_demand_checklist(self):
result = pmcp.assess_post_merge_cleanup_proof(_pr703_style_report())
self.assertFalse(result["block"], result["reasons"])
def test_release_tool_name_alone_is_recognized(self):
report = _pr703_style_report(
cleanup_mutations="gitea_release_reviewer_pr_lease comment 11244"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertFalse(result["block"], result["reasons"])
def test_substantive_non_lease_cleanup_still_demands_checklist(self):
report = _pr703_style_report(
cleanup_mutations="deleted stale scratch directory manually"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertTrue(result["block"])
def test_remote_branch_delete_claims_still_demand_full_proof(self):
report = _pr703_style_report(
cleanup_mutations="gitea_delete_branch removed the remote branch"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertTrue(result["block"])
self.assertTrue(
any("remote branch deletion missing" in r for r in result["reasons"])
)
def test_full_schema_run_accepts_lease_release_report(self):
result = assess_review_final_report_schema(_pr703_style_report())
lease_cleanup_blocks = [
f for f in result["findings"]
if f["rule_id"] == "reviewer.post_merge_cleanup_proof"
]
self.assertEqual(lease_cleanup_blocks, [], lease_cleanup_blocks)
class TestStructuredProofRecognition(unittest.TestCase):
"""#698: structured workflow-load and validation proof must be accepted."""
def test_key_value_workflow_proof_recognized(self):
findings = frv._rule_reviewer_workflow_load_boundary(
_pr703_style_report()
)
self.assertEqual(findings, [], findings)
def test_colon_form_workflow_proof_still_recognized(self):
report = _pr703_style_report().replace(
"- Workflow-load helper result: workflow_hash=da045d1e1f1f "
"boundary_status=clean",
"- Workflow-load helper result: workflow_hash: da045d1e1f1f, "
"boundary_status: clean",
)
findings = frv._rule_reviewer_workflow_load_boundary(report)
self.assertEqual(findings, [], findings)
def test_incomplete_structured_proof_still_blocks(self):
report = _pr703_style_report().replace(
"workflow_hash=da045d1e1f1f boundary_status=clean",
"workflow_hash=da045d1e1f1f",
)
findings = frv._rule_reviewer_workflow_load_boundary(report)
self.assertTrue(findings)
self.assertIn("boundary_status", findings[0]["reason"])
def test_validation_counts_accepted_as_pass_proof(self):
# "Validation: focused 50 passed; ..." must satisfy the reviewed-head
# validation-proof rule (PR #703 reproduction).
result = assess_review_final_report_schema(_pr703_style_report())
head_blocks = [
f for f in result["findings"]
if f["rule_id"] == "reviewer.reviewed_head_without_validation"
]
self.assertEqual(head_blocks, [], head_blocks)
class TestAuthoritativeMutationInference(unittest.TestCase):
"""#698: review mutations inferred only from authoritative evidence."""
def test_read_only_entries_do_not_imply_mutations(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[
{"action": "gitea_view_pr"},
{"action": "gitea_get_pr_review_feedback", "performed": False},
],
)
self.assertEqual(findings, [], findings)
def test_performed_mutation_still_blocks_vague_none(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[{"action": "edit", "path": "a.py", "performed": True}],
)
self.assertTrue(findings)
def test_gated_rejection_is_not_a_mutation(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[
{"action": "edit", "path": "a.py", "performed": True,
"gated_rejected": True},
],
)
self.assertEqual(findings, [], findings)
class TestValidatorRuleErrorContainment(unittest.TestCase):
"""#698: a defective rule fails closed with a sanitized error."""
def test_rule_exception_becomes_sanitized_block_finding(self):
def _boom(report_text):
raise ValueError("raw secret detail that must not leak")
original = frv._RULES_BY_TASK["review_pr"]
frv._RULES_BY_TASK["review_pr"] = [_boom]
try:
result = frv.assess_final_report_validator(
"report body", "review_pr"
)
finally:
frv._RULES_BY_TASK["review_pr"] = original
self.assertTrue(result["blocked"])
finding = next(
f for f in result["findings"]
if f["rule_id"] == "shared.validator_rule_error"
)
self.assertNotIn("raw secret detail", finding["reason"])
self.assertIn("ValueError", finding["reason"])
self.assertIn("_boom", finding["reason"])
if __name__ == "__main__":
unittest.main()
+27 -6
View File
@@ -108,13 +108,24 @@ class TestIssueCommentWorkspaceGuard(unittest.TestCase):
"gitea_mcp_server.issue_lock_worktree.read_worktree_git_state",
side_effect=self._git_state(valid_worktree),
):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue_comment(
try:
res = srv.gitea_create_issue_comment(
issue_number=557,
body="evidence comment",
remote="prgs",
)
self.assertIn("stable control checkout", str(ctx.exception))
except RuntimeError as exc:
self.assertIn("stable control checkout", str(exc))
else:
# #683 typed blocker at mutation entrypoint
self.assertFalse(res.get("success"))
self.assertFalse(res.get("performed"))
blob = " ".join(res.get("reasons") or [])
self.assertTrue(
"stable control checkout" in blob
or res.get("blocker_kind")
)
self.assertTrue(res.get("exact_next_action") or res.get("reasons"))
mock_api.assert_not_called()
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
@@ -184,14 +195,24 @@ class TestIssueCommentWorkspaceGuard(unittest.TestCase):
side_effect=self._subprocess(valid_worktree, outside_worktree),
):
with patch.dict(os.environ, self.AUTHOR_ENV, clear=True):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue_comment(
try:
res = srv.gitea_create_issue_comment(
issue_number=557,
body="evidence comment",
remote="prgs",
worktree_path=outside_worktree,
)
self.assertIn("does not belong to the target repository", str(ctx.exception))
except RuntimeError as exc:
self.assertIn(
"does not belong to the target repository",
str(exc),
)
else:
self.assertFalse(res.get("success"))
blob = " ".join(res.get("reasons") or [])
self.assertIn(
"does not belong to the target repository", blob
)
mock_api.assert_not_called()
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
+7 -3
View File
@@ -76,13 +76,17 @@ class TestPreflightReadSurvival(unittest.TestCase):
self.assertIn("task mismatch", str(ctx.exception))
def test_capability_consumed_after_mutation_gate(self):
# Use reconciler/close_pr so this purity-order test does not require a
# branches/ worktree (author create_issue would hit #274/#683 guards).
# Test isolation stays explicit; production author guards remain live
# under force-on (see tests/test_issue_683_workflow_scope_guards.py).
mcp_server.record_preflight_check("whoami")
mcp_server.record_preflight_check(
"capability", resolved_role="author", resolved_task="create_issue"
"capability", resolved_role="reconciler", resolved_task="close_pr"
)
mcp_server.verify_preflight_purity(task="create_issue")
mcp_server.verify_preflight_purity(task="close_pr")
with self.assertRaises(RuntimeError) as ctx:
mcp_server.verify_preflight_purity(task="create_issue")
mcp_server.verify_preflight_purity(task="close_pr")
self.assertIn("has not been resolved", str(ctx.exception))
def test_whoami_recovery_after_violation_clears_capability(self):
+15 -3
View File
@@ -86,9 +86,21 @@ class TestReconcilerCloseWorkspaceGuard(unittest.TestCase):
):
srv._preflight_resolved_role = "author"
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
with self.assertRaises(RuntimeError) as ctx:
srv.gitea_create_issue(title="Test", body="body")
self.assertIn("stable control checkout", str(ctx.exception))
try:
res = srv.gitea_create_issue(title="Test", body="body")
except RuntimeError as exc:
self.assertIn("stable control checkout", str(exc))
else:
# #683 typed blocker at mutation entrypoint
self.assertFalse(res.get("success"))
blob = " ".join(res.get("reasons") or []) + str(
res.get("blocker_kind") or ""
)
self.assertTrue(
"stable control checkout" in blob
or "missing_issue_worktree" in blob
or "control checkout" in blob.lower()
)
if __name__ == "__main__":
+10 -8
View File
@@ -957,17 +957,20 @@ class TestControllerHandoff(unittest.TestCase):
if not line.startswith("- Workspace mutations:"))
result = assess_controller_handoff(review_base, role="review")
self.assertEqual(result["verdict"], "incomplete")
self.assertIn("Pinned reviewed head", result["missing_fields"])
self.assertIn("Worktree path", result["missing_fields"])
# #698: the canonical schema forbids the legacy fields, so the
# validator must demand the canonical names instead.
self.assertIn("Reviewed head SHA", result["missing_fields"])
self.assertIn("Review worktree path", result["missing_fields"])
self.assertIn("Merge result", result["missing_fields"])
for legacy in ("Pinned reviewed head", "Scratch worktree used"):
self.assertNotIn(legacy, result["missing_fields"])
complete = review_base + "\n" + "\n".join([
"- Selected PR: #999",
"- Reviewer eligibility: passed",
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Worktree path: /repo/branches/review-pr-999",
"- Worktree dirty: no",
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Review worktree path: /repo/branches/review-pr-999",
"- Review worktree dirty before validation: no",
"- Unrelated local mutations: none",
"- Review decision: approve",
"- Merge result: merged",
@@ -1125,10 +1128,9 @@ class TestReviewHandoffPreciseMutationCategories(unittest.TestCase):
"- Safety: no self-review; no self-merge; no secrets",
"- Selected PR: #999",
"- Reviewer eligibility: passed",
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Worktree path: /repo/branches/review-pr-999",
"- Worktree dirty: no",
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
"- Unrelated local mutations: none",
"- Review decision: approve",
"- Merge result: none",
+626
View File
@@ -0,0 +1,626 @@
"""Workflow scope ownership and production-guard hardening (#683).
Implements fail-closed enforcement so sessions cannot:
* mutate source/tests on the root/control checkout (including temporary
diagnostic edits) without binding an issue-backed ``branches/`` worktree;
* continue out-of-scope source work while locked to a different issue;
* disable, skip, or conceal production root/branches/porcelain guards solely
because pytest/unittest is loaded.
This module is pure assessment + small durable ledger helpers. Callers gather
live facts (lock, branch, porcelain, worktree path) and pass them in. Existing
root_checkout_guard / author_mutation_worktree assessors remain authoritative;
this module composes typed blockers with exact recovery actions.
Do **not** reintroduce the rejected #681 / ``300a4ca`` patterns:
* early-return from workspace verification under ``_preflight_in_test_mode()``
* porcelain filtering that strips ``*.py`` lines under pytest
"""
from __future__ import annotations
import os
import re
import threading
from typing import Any
import author_mutation_worktree
from reviewer_worktree import parse_dirty_tracked_files
# ── force-on / test isolation ────────────────────────────────────────────────
# When set, production root/branches/scope guards MUST run even under pytest.
# Unit tests that only need preflight-order isolation leave this unset and
# use GITEA_TEST_PORCELAIN / fixtures; real-entrypoint proof sets this to "1".
FORCE_PRODUCTION_GUARDS_ENV = "GITEA_TEST_FORCE_PRODUCTION_GUARDS"
# Existing force signals also mean "exercise production dirtiness paths".
_FORCE_DIRTY_ENV = "GITEA_TEST_FORCE_DIRTY"
_FORCE_PORCELAIN_ENV = "GITEA_TEST_PORCELAIN"
# ── typed blocker kinds ──────────────────────────────────────────────────────
BLOCKER_ROOT_DIAGNOSTIC_EDIT = "root_diagnostic_edit"
BLOCKER_MISSING_ISSUE_SCOPE = "missing_issue_scope"
BLOCKER_OUT_OF_SCOPE_ISSUE = "out_of_scope_issue"
BLOCKER_MISSING_WORKTREE = "missing_issue_worktree"
BLOCKER_UNRECORDED_FAILURE = "unrecorded_workflow_failure"
BLOCKER_PRODUCTION_GUARD = "production_guard_violation"
BLOCKER_KINDS = frozenset(
{
BLOCKER_ROOT_DIAGNOSTIC_EDIT,
BLOCKER_MISSING_ISSUE_SCOPE,
BLOCKER_OUT_OF_SCOPE_ISSUE,
BLOCKER_MISSING_WORKTREE,
BLOCKER_UNRECORDED_FAILURE,
BLOCKER_PRODUCTION_GUARD,
}
)
_NEXT_ACTIONS: dict[str, str] = {
BLOCKER_ROOT_DIAGNOSTIC_EDIT: (
"Stop editing the control/root checkout. Preserve or discard root WIP "
"durably, restore root to clean master, lock or create the owning issue, "
"bind branches/issue-<N>-*, set GITEA_AUTHOR_WORKTREE to that worktree, "
"then re-run the mutation."
),
BLOCKER_MISSING_ISSUE_SCOPE: (
"Select or create the owning Gitea issue, claim/lock it "
"(gitea_mark_issue + gitea_lock_issue), bind branches/issue-<N>-* "
"from clean master, then re-run the mutation from that worktree."
),
BLOCKER_OUT_OF_SCOPE_ISSUE: (
"Stop. The active issue lock does not own this work. Release or finish "
"the current issue lease, then select/create and lock the correct "
"owning issue, bind its branches/issue-<N>-* worktree, and re-run."
),
BLOCKER_MISSING_WORKTREE: (
"Bind an issue-backed worktree under branches/ (scripts/worktree-start "
"or git worktree add branches/issue-<N>-*), set GITEA_AUTHOR_WORKTREE / "
"worktree_path to that path, keep the control checkout clean on master, "
"then re-run the mutation."
),
BLOCKER_UNRECORDED_FAILURE: (
"Record the workflow/tool failure durably first (issue comment or "
"workflow_scope_guard.record_workflow_failure), then continue only "
"inside the owning issue-backed worktree."
),
BLOCKER_PRODUCTION_GUARD: (
"Resolve the production guard violation: clean or isolate the control "
"checkout, bind the owning issue worktree under branches/, and re-run "
"with production guards active."
),
}
_ISSUE_IN_BRANCH_RE = re.compile(r"issue-(\d+)", re.IGNORECASE)
# In-process durable failure ledger (also written via optional sink callback).
_ledger_lock = threading.Lock()
_failure_ledger: list[dict[str, Any]] = []
class ProductionGuardError(RuntimeError):
"""Fail-closed production guard with typed blocker metadata (#683)."""
def __init__(
self,
message: str,
*,
blocker_kind: str,
exact_next_action: str | None = None,
reasons: list[str] | None = None,
details: dict[str, Any] | None = None,
) -> None:
super().__init__(message)
kind = (blocker_kind or "").strip()
if kind not in BLOCKER_KINDS:
kind = BLOCKER_PRODUCTION_GUARD
self.blocker_kind = kind
self.exact_next_action = (
(exact_next_action or "").strip() or _NEXT_ACTIONS[kind]
)
self.reasons = list(reasons or [message])
self.details = dict(details or {})
def production_guards_forced() -> bool:
"""True when the explicit #683 force-on flag requests production guards."""
return (os.environ.get(FORCE_PRODUCTION_GUARDS_ENV) or "").strip().lower() in {
"1",
"true",
"yes",
"on",
}
def purity_order_forced() -> bool:
"""True when tests force preflight-order dirtiness paths (legacy flags)."""
if os.environ.get(_FORCE_DIRTY_ENV):
return True
# GITEA_TEST_PORCELAIN present (even empty) means dirtiness paths are live.
if os.environ.get(_FORCE_PORCELAIN_ENV) is not None:
return True
return False
def production_guards_active(*, in_test_mode: bool) -> bool:
"""Whether production root/branches/scope guards must execute.
Production (non-test) always active. Under pytest, active when either the
explicit #683 force-on flag or legacy dirty/porcelain force signals are
set — never skip production enforcement solely because tests are running
when force-on is requested.
"""
if production_guards_forced() or purity_order_forced():
return True
return not bool(in_test_mode)
def extract_issue_number_from_branch(branch_name: str | None) -> int | None:
"""Return the first issue-N number embedded in a branch name, if any."""
text = (branch_name or "").strip()
if not text:
return None
match = _ISSUE_IN_BRANCH_RE.search(text)
if not match:
return None
try:
return int(match.group(1))
except ValueError:
return None
def is_source_or_test_path(path: str) -> bool:
"""True for tracked source/test paths that must not land as root WIP."""
p = (path or "").replace("\\", "/").lstrip("./")
if not p:
return False
if p.startswith("tests/") or "/tests/" in f"/{p}":
return True
if p.endswith((".py", ".pyi", ".toml", ".cfg", ".ini", ".sh")):
return True
if p in {"requirements.txt", "pyproject.toml", "setup.py", "setup.cfg"}:
return True
return False
def dirty_source_files(porcelain_status: str) -> list[str]:
"""Tracked dirty paths that count as source/test contamination."""
dirty = parse_dirty_tracked_files(porcelain_status or "")
return [p for p in dirty if is_source_or_test_path(p)]
def assess_issue_scope_ownership(
*,
locked_issue_number: int | None,
target_issue_number: int | None = None,
branch_name: str | None = None,
role_kind: str | None = None,
require_lock_for_author: bool = False,
) -> dict[str, Any]:
"""Fail closed when the session issue lock does not own the attempted work.
* Author sessions that require a lock fail when none is held.
* When a lock exists, the target issue (tool argument) and/or the issue
number embedded in the branch must match the locked issue.
* Reviewer/merger/reconciler roles are not issue-scope owners of author
implementation work and skip the author lock requirement.
"""
role = (role_kind or "").strip().lower()
locked = locked_issue_number
if isinstance(locked, str) and locked.isdigit():
locked = int(locked)
if locked is not None:
try:
locked = int(locked)
except (TypeError, ValueError):
locked = None
target = target_issue_number
if target is not None:
try:
target = int(target)
except (TypeError, ValueError):
target = None
branch_issue = extract_issue_number_from_branch(branch_name)
reasons: list[str] = []
blocker_kind: str | None = None
# Non-author roles do not take author issue locks for implementation.
if role in {"reviewer", "merger", "reconciler"}:
return _scope_ok(locked, target, branch_issue)
if require_lock_for_author and locked is None:
reasons.append(
"no owning issue lock is bound for this author session; "
"source/test mutation requires selecting or creating an owning issue first"
)
blocker_kind = BLOCKER_MISSING_ISSUE_SCOPE
if locked is not None and target is not None and locked != target:
reasons.append(
f"session is locked to issue #{locked} but mutation targets issue "
f"#{target}; out-of-scope until the owning issue is selected"
)
blocker_kind = BLOCKER_OUT_OF_SCOPE_ISSUE
if locked is not None and branch_issue is not None and locked != branch_issue:
reasons.append(
f"session is locked to issue #{locked} but workspace branch is for "
f"issue #{branch_issue}; bind the matching issue-backed worktree"
)
blocker_kind = BLOCKER_OUT_OF_SCOPE_ISSUE
if reasons:
kind = blocker_kind or BLOCKER_MISSING_ISSUE_SCOPE
return {
"proven": False,
"block": True,
"blocker_kind": kind,
"exact_next_action": _NEXT_ACTIONS[kind],
"reasons": reasons,
"locked_issue_number": locked,
"target_issue_number": target,
"branch_issue_number": branch_issue,
}
return _scope_ok(locked, target, branch_issue)
def assess_root_source_mutation(
*,
workspace_path: str,
canonical_repo_root: str,
porcelain_status: str,
current_branch: str | None = None,
locked_issue_number: int | None = None,
role_kind: str | None = None,
) -> dict[str, Any]:
"""Fail closed for diagnostic/source edits on the control/root checkout.
Allowed only when the active workspace is under ``branches/``. Dirty
tracked source/test files on the control checkout always block, including
temporary/diagnostic/test-only intent.
"""
role = (role_kind or "").strip().lower()
if role == "reconciler":
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
"dirty_source_files": [],
}
root = os.path.realpath(canonical_repo_root or "")
workspace = os.path.realpath(workspace_path or root or ".")
under_branches = author_mutation_worktree.is_path_under_branches(workspace, root)
dirty_src = dirty_source_files(porcelain_status)
reasons: list[str] = []
blocker_kind: str | None = None
if not under_branches and workspace == root and dirty_src:
# Root workspace with source dirtiness is unattributed root WIP.
# (Clean-root author binding is enforced by branches-only #274.)
reasons.append(
"control/root checkout has tracked source or test edits "
f"(dirty files: {', '.join(dirty_src)}); diagnostic or temporary "
"edits on the root checkout are forbidden"
)
blocker_kind = BLOCKER_ROOT_DIAGNOSTIC_EDIT
if (
not under_branches
and workspace == root
and not dirty_src
and role == "author"
):
# Explicit missing-worktree signal for force-on author entrypoints.
reasons.append(
"author source/test mutation from the stable control checkout is "
"forbidden; bind an issue-backed worktree under branches/ first"
)
blocker_kind = BLOCKER_MISSING_WORKTREE
if reasons:
kind = blocker_kind or BLOCKER_ROOT_DIAGNOSTIC_EDIT
return {
"proven": False,
"block": True,
"blocker_kind": kind,
"exact_next_action": _NEXT_ACTIONS[kind],
"reasons": reasons,
"dirty_source_files": dirty_src,
"workspace_path": workspace,
"canonical_repo_root": root,
"under_branches": under_branches,
"locked_issue_number": locked_issue_number,
}
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
"dirty_source_files": dirty_src,
"workspace_path": workspace,
"canonical_repo_root": root,
"under_branches": under_branches,
"locked_issue_number": locked_issue_number,
}
def assess_production_mutation_guards(
*,
workspace_path: str,
canonical_repo_root: str,
porcelain_status: str,
current_branch: str | None = None,
locked_issue_number: int | None = None,
target_issue_number: int | None = None,
role_kind: str | None = None,
require_author_lock: bool = False,
in_test_mode: bool = False,
) -> dict[str, Any]:
"""Compose root + scope production guards when they must be active (#683)."""
if not production_guards_active(in_test_mode=in_test_mode):
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
"skipped": True,
"skip_reason": "production guards not active (test isolation without force-on)",
}
root_assess = assess_root_source_mutation(
workspace_path=workspace_path,
canonical_repo_root=canonical_repo_root,
porcelain_status=porcelain_status,
current_branch=current_branch,
locked_issue_number=locked_issue_number,
role_kind=role_kind,
)
if root_assess["block"]:
return {**root_assess, "skipped": False}
scope_assess = assess_issue_scope_ownership(
locked_issue_number=locked_issue_number,
target_issue_number=target_issue_number,
branch_name=current_branch,
role_kind=role_kind,
require_lock_for_author=require_author_lock,
)
if scope_assess["block"]:
return {**scope_assess, "skipped": False}
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
"skipped": False,
"root": root_assess,
"scope": scope_assess,
}
def raise_if_blocked(assessment: dict[str, Any]) -> None:
"""Raise :class:`ProductionGuardError` when *assessment* blocks."""
if not assessment or not assessment.get("block"):
return
kind = assessment.get("blocker_kind") or BLOCKER_PRODUCTION_GUARD
reasons = list(assessment.get("reasons") or ["production guard violation"])
message = (
f"Workflow scope guard (#683) [{kind}]: {'; '.join(reasons)}. "
f"exact_next_action: {assessment.get('exact_next_action') or _NEXT_ACTIONS.get(kind, '')}"
)
raise ProductionGuardError(
message,
blocker_kind=kind,
exact_next_action=assessment.get("exact_next_action"),
reasons=reasons,
details={
k: v
for k, v in assessment.items()
if k
not in {
"proven",
"block",
"blocker_kind",
"exact_next_action",
"reasons",
}
},
)
def block_response(
assessment: dict[str, Any] | ProductionGuardError | None = None,
*,
blocker_kind: str | None = None,
reasons: list[str] | None = None,
exact_next_action: str | None = None,
**extra: Any,
) -> dict[str, Any]:
"""Structured fail-closed tool response with typed blocker fields."""
if isinstance(assessment, ProductionGuardError):
kind = assessment.blocker_kind
reason_list = list(assessment.reasons)
next_action = assessment.exact_next_action
extra = {**assessment.details, **extra}
elif isinstance(assessment, dict) and assessment.get("block"):
kind = assessment.get("blocker_kind") or BLOCKER_PRODUCTION_GUARD
reason_list = list(assessment.get("reasons") or [])
next_action = assessment.get("exact_next_action") or _NEXT_ACTIONS.get(
kind, _NEXT_ACTIONS[BLOCKER_PRODUCTION_GUARD]
)
else:
kind = (blocker_kind or BLOCKER_PRODUCTION_GUARD).strip()
if kind not in BLOCKER_KINDS:
kind = BLOCKER_PRODUCTION_GUARD
reason_list = list(reasons or ["production guard violation"])
next_action = exact_next_action or _NEXT_ACTIONS[kind]
if kind not in BLOCKER_KINDS:
kind = BLOCKER_PRODUCTION_GUARD
if not reason_list:
reason_list = ["production guard violation"]
next_action = (next_action or "").strip() or _NEXT_ACTIONS[kind]
out: dict[str, Any] = {
"success": False,
"performed": False,
"blocker_kind": kind,
"exact_next_action": next_action,
"reasons": reason_list,
}
for key, value in extra.items():
if key not in out and value is not None:
out[key] = value
return out
def format_production_guard_error(assessment: dict[str, Any]) -> str:
"""Single RuntimeError string carrying kind + exact next action."""
kind = assessment.get("blocker_kind") or BLOCKER_PRODUCTION_GUARD
reasons = "; ".join(assessment.get("reasons") or ["production guard violation"])
next_action = assessment.get("exact_next_action") or _NEXT_ACTIONS.get(
kind, _NEXT_ACTIONS[BLOCKER_PRODUCTION_GUARD]
)
return (
f"Workflow scope guard (#683) [{kind}]: {reasons}. "
f"exact_next_action: {next_action}"
)
# ── durable failure recording ────────────────────────────────────────────────
def record_workflow_failure(
*,
kind: str,
detail: str,
issue_number: int | None = None,
task: str | None = None,
sink: Any | None = None,
) -> dict[str, Any]:
"""Record a workflow/tool failure before source edits continue (#683 AC8).
*sink* may be a callable ``sink(record)`` (e.g. tests) or omitted for the
in-process ledger only. Returns the durable record.
"""
record = {
"kind": (kind or "workflow_failure").strip() or "workflow_failure",
"detail": (detail or "").strip(),
"issue_number": issue_number,
"task": task,
"pid": os.getpid(),
}
with _ledger_lock:
_failure_ledger.append(dict(record))
if callable(sink):
sink(record)
return record
def clear_workflow_failure_ledger() -> None:
"""Test helper: reset the in-process failure ledger."""
with _ledger_lock:
_failure_ledger.clear()
def workflow_failure_ledger() -> list[dict[str, Any]]:
"""Copy of durable in-process failure records."""
with _ledger_lock:
return [dict(r) for r in _failure_ledger]
def assess_durable_failure_recorded(
*,
require_record: bool,
pending_source_mutation: bool,
) -> dict[str, Any]:
"""Block source mutation when a workflow failure was not recorded first."""
if not require_record or not pending_source_mutation:
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
}
with _ledger_lock:
has_record = bool(_failure_ledger)
if has_record:
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
}
return {
"proven": False,
"block": True,
"blocker_kind": BLOCKER_UNRECORDED_FAILURE,
"exact_next_action": _NEXT_ACTIONS[BLOCKER_UNRECORDED_FAILURE],
"reasons": [
"workflow/tool failure triggered a need for source changes but no "
"durable failure record exists yet"
],
}
def porcelain_preserves_python_paths(porcelain_status: str) -> bool:
"""Regression helper: dirty ``*.py`` lines must remain visible (#683)."""
text = porcelain_status or ""
for line in text.splitlines():
stripped = line.strip()
if stripped.endswith(".py") or ".py " in stripped or stripped.endswith(".py"):
# Any py path present proves no silent strip of all *.py lines.
if " M " in f" {stripped}" or stripped[:1] in "MADRCTU" or len(line) >= 4:
return True
# Empty porcelain is fine; integrity means we did not strip when present.
return ".py" not in text
def assert_no_pytest_porcelain_filter(source_text: str) -> list[str]:
"""Static check: production reader must not strip ``*.py`` under pytest."""
findings: list[str] = []
lowered = source_text or ""
if "endswith(\".py\")" in lowered or "endswith('.py')" in lowered:
if "pytest" in lowered and "porcelain" in lowered.lower():
findings.append(
"production porcelain reader must not filter *.py under pytest "
"(rejected 300a4ca pattern)"
)
if "if \"pytest\" in sys.modules" in lowered and "porcelain" in lowered.lower():
if ".py" in lowered and ("join" in lowered or "endswith" in lowered):
findings.append(
"test-mode porcelain filtering of source files is forbidden (#683)"
)
return findings
def _scope_ok(
locked: int | None,
target: int | None,
branch_issue: int | None,
) -> dict[str, Any]:
return {
"proven": True,
"block": False,
"blocker_kind": None,
"exact_next_action": "proceed",
"reasons": [],
"locked_issue_number": locked,
"target_issue_number": target,
"branch_issue_number": branch_issue,
}