diff --git a/final_report_validator.py b/final_report_validator.py index 3fbac0f..d8bd96f 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -490,6 +490,45 @@ def _rule_reviewer_validation_failure_history( ] +def _rule_reviewer_stale_head_proof(report_text: str) -> list[dict[str, str]]: + from pr_work_lease import assess_reviewer_stale_head_final_report + + result = assess_reviewer_stale_head_final_report(report_text) + if result.get("proven"): + return [] + return _findings_from_reasons( + "reviewer.stale_head_proof", + result.get("reasons") or [], + field="Stale-head proof", + severity="block", + safe_next_action=( + "state reviewed head SHA, live head before approval/merge, and " + "whether any push occurred during validation" + ), + ) + + +def _rule_conflict_fix_push_proof(report_text: str) -> list[dict[str, str]]: + from pr_work_lease import assess_conflict_fix_final_report + + text = report_text or "" + if "conflict-fix" not in text.lower() and "conflict fix" not in text.lower(): + return [] + result = assess_conflict_fix_final_report(text) + if result.get("proven"): + return [] + return _findings_from_reasons( + "author.conflict_fix_push_proof", + result.get("reasons") or [], + field="Conflict-fix push proof", + severity="block", + safe_next_action=( + "state branch head before/after push, reviewer lease status, " + "fast-forward status, and whether any reviewer was active" + ), + ) + + def _rule_reviewer_validation_command(report_text: str) -> list[dict[str, str]]: text = report_text or "" if not _BARE_PYTEST_RE.search(text): @@ -909,6 +948,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_target_branch_freshness, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, + _rule_reviewer_stale_head_proof, ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, @@ -930,6 +970,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_controller_handoff, _rule_shared_email_disclosure, _rule_reviewer_vague_mutations_none, + _rule_conflict_fix_push_proof, ], "issue_filing": [ _rule_shared_controller_handoff, diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b1a5e0c..be9b235 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -507,6 +507,7 @@ import merged_cleanup_reconcile # noqa: E402 import reconciler_profile # noqa: E402 import reconciliation_workflow # noqa: E402 import review_merge_state_machine # noqa: E402 +import pr_work_lease # noqa: E402 import native_mcp_preference # noqa: E402 @@ -2197,6 +2198,50 @@ def gitea_get_pr_review_feedback( } +def _list_pr_lease_comments( + pr_number: int, + *, + remote: str, + host: str | None, + org: str | None, + repo: str | None, + limit: int = 100, +) -> list[dict]: + """Fetch PR/issue thread comments used for reviewer/conflict-fix leases.""" + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + api = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments" + comments = api_request("GET", api, auth) or [] + return list(comments[:limit]) + + +def _pr_work_lease_reviewer_block( + *, + pr_number: int, + reviewed_head_sha: str | None, + live_head_sha: str | None, + mutation: str, + remote: str, + host: str | None, + org: str | None, + repo: str | None, +) -> dict: + comments = _list_pr_lease_comments( + pr_number, + remote=remote, + host=host, + org=org, + repo=repo, + ) + return pr_work_lease.assess_reviewer_mutation_blocked( + pr_number=pr_number, + comments=comments, + reviewed_head_sha=reviewed_head_sha, + live_head_sha=live_head_sha, + mutation=mutation, + ) + + def _evaluate_pr_review_submission( pr_number: int, action: str, @@ -2281,6 +2326,11 @@ def _evaluate_pr_review_submission( lock = _load_review_decision_lock() or {} if live and lock.get("ready_expected_head_sha"): pinned_sha = lock.get("ready_expected_head_sha") + if live and not pinned_sha: + reasons.append( + "reviewed head SHA required before live review mutation (fail closed, #399)" + ) + return result if pinned_sha and actual_sha and pinned_sha != actual_sha: reasons.append( "expected head SHA does not match current PR head (fail closed)" @@ -2290,6 +2340,21 @@ def _evaluate_pr_review_submission( reasons.append("PR head SHA unavailable (fail closed)") return result + lease_block = _pr_work_lease_reviewer_block( + pr_number=pr_number, + reviewed_head_sha=pinned_sha, + live_head_sha=actual_sha, + mutation=action, + remote=remote, + host=host, + org=org, + repo=repo, + ) + if lease_block.get("block"): + reasons.extend(lease_block.get("reasons") or []) + result["pr_work_lease"] = lease_block + return result + result["would_perform"] = True if not live: reasons.append( @@ -2432,6 +2497,39 @@ def gitea_mark_final_review_decision( f"{sorted(_REVIEW_ACTIONS)}" ], } + if not (expected_head_sha or "").strip(): + return { + "marked_ready": False, + "reasons": [ + "expected_head_sha required before marking final review " + "decision (fail closed, #399)" + ], + } + elig = gitea_check_pr_eligibility( + pr_number=pr_number, + action="review", + remote=remote, + host=None, + org=org, + repo=repo, + ) + live_head = elig.get("head_sha") + lease_block = _pr_work_lease_reviewer_block( + pr_number=pr_number, + reviewed_head_sha=expected_head_sha, + live_head_sha=live_head, + mutation="mark_ready", + remote=remote, + host=None, + org=org, + repo=repo, + ) + if lease_block.get("block"): + return { + "marked_ready": False, + "reasons": lease_block.get("reasons") or [], + "pr_work_lease": lease_block, + } if action == "request_changes": # Duplicate request-changes suppression (#332): an unresolved # REQUEST_CHANGES at the current head must not be duplicated. @@ -3055,8 +3153,27 @@ def gitea_merge_pr( result["permission_report"] = elig["permission_report"] return result - # Gate 4 — head SHA must match if the caller pinned a reviewed SHA. + # Gate 4 — reviewed head SHA is mandatory and must match live PR head (#399). actual_sha = result["head_sha"] + if not (expected_head_sha or "").strip(): + reasons.append( + "expected_head_sha required before merge (fail closed, #399)" + ) + return result + lease_block = _pr_work_lease_reviewer_block( + pr_number=pr_number, + reviewed_head_sha=expected_head_sha, + live_head_sha=actual_sha, + mutation="merge", + remote=remote, + host=host, + org=org, + repo=repo, + ) + if lease_block.get("block"): + reasons.extend(lease_block.get("reasons") or []) + result["pr_work_lease"] = lease_block + return result if expected_head_sha and actual_sha and expected_head_sha != actual_sha: reasons.append( "expected head SHA does not match current PR head (fail closed)" @@ -6068,6 +6185,110 @@ def gitea_post_heartbeat( ) +@mcp.tool() +def gitea_acquire_conflict_fix_lease( + pr_number: int, + branch: str, + worktree_path: str, + head_before: str, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Acquire a conflict-fix lease on a PR branch before pushing (#399).""" + blocked = _profile_permission_block( + task_capability_map.required_permission("comment_issue")) + if blocked: + return blocked + verify_preflight_purity(remote, worktree_path=worktree_path) + comments = _list_pr_lease_comments( + pr_number, + remote=remote, + host=host, + org=org, + repo=repo, + ) + reviewer_lease = pr_work_lease.find_active_reviewer_lease( + comments, pr_number=pr_number) + if reviewer_lease: + return { + "acquired": False, + "reasons": [ + f"active reviewer lease on PR #{pr_number}; cannot acquire " + "conflict-fix lease (fail closed)" + ], + "active_reviewer_lease": reviewer_lease, + } + profile_name = get_profile().get("profile_name") or "unknown" + body = pr_work_lease.format_conflict_fix_lease_body( + pr_number=pr_number, + branch=branch, + worktree=worktree_path, + profile=profile_name, + head_before=head_before, + reviewer_active=bool(reviewer_lease), + ) + posted = _post_structured_issue_comment( + issue_number=pr_number, + body=body, + remote=remote, + host=host, + org=org, + repo=repo, + audit_op="conflict_fix_lease_acquire", + ) + return { + "acquired": posted.get("success", False), + "pr_number": pr_number, + "branch": branch, + "worktree_path": worktree_path, + "head_before": head_before, + "comment_id": posted.get("comment_id"), + "active_reviewer_lease": reviewer_lease, + "reasons": [] if posted.get("success") else ["lease comment post failed"], + } + + +@mcp.tool() +def gitea_assess_conflict_fix_push( + pr_number: int, + branch_head_before: str, + branch_head_after: str, + worktree_path: str, + push_cwd: str, + is_fast_forward: bool = True, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Read-only pre-push gate for author conflict-fix sessions (#399).""" + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "push_allowed": False, + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + comments = _list_pr_lease_comments( + pr_number, + remote=remote, + host=host, + org=org, + repo=repo, + ) + return pr_work_lease.assess_conflict_fix_push( + pr_number=pr_number, + comments=comments, + branch_head_before=branch_head_before, + branch_head_after=branch_head_after, + worktree_path=worktree_path, + push_cwd=push_cwd, + is_fast_forward=is_fast_forward, + ) + + @mcp.tool() def gitea_reconcile_issue_claims( state: str = "open", diff --git a/pr_work_lease.py b/pr_work_lease.py new file mode 100644 index 0000000..e2b3f21 --- /dev/null +++ b/pr_work_lease.py @@ -0,0 +1,482 @@ +"""Conflict-fix and reviewer PR work leases (#399, #407 reader). + +Structured PR/issue comments prove exclusive phases so author conflict-fix +pushes cannot race reviewer validation/approval/merge on the same head. +""" + +from __future__ import annotations + +import re +from datetime import datetime, timedelta, timezone +from typing import Any + +REVIEWER_LEASE_MARKER = "" +CONFLICT_FIX_LEASE_MARKER = "" + +_FULL_SHA = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE) + +_FIELD_RE = re.compile( + r"^\s*([a-z_]+)\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) + +_TERMINAL_REVIEWER_PHASES = frozenset({"done", "released", "blocked"}) +_ACTIVE_REVIEWER_PHASES = frozenset({ + "claimed", + "validating", + "approved", + "request-changes", + "merging", +}) +_TERMINAL_CONFLICT_FIX_PHASES = frozenset({"released", "blocked", "done"}) +_ACTIVE_CONFLICT_FIX_PHASES = frozenset({"claimed", "pushing", "pushed"}) + +DEFAULT_CONFLICT_FIX_TTL_MINUTES = 120 +DEFAULT_REVIEWER_LEASE_TTL_MINUTES = 120 + + +def _parse_timestamp(value: str | None) -> datetime | None: + if not value: + return None + text = value.strip() + if text.endswith("Z"): + text = text[:-1] + "+00:00" + try: + parsed = datetime.fromisoformat(text) + except ValueError: + return None + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +def _normalize_sha(value: str | None) -> str | None: + text = (value or "").strip().lower() + if not text: + return None + return text if _FULL_SHA.match(text) else None + + +def _parse_pr_ref(value: str | None) -> int | None: + digits = re.sub(r"[^\d]", "", value or "") + return int(digits) if digits.isdigit() else None + + +def _parse_marker_comment(body: str, marker: str) -> dict[str, str] | None: + text = body or "" + if marker not in text: + return None + fields: dict[str, str] = {} + for match in _FIELD_RE.finditer(text): + fields[match.group(1).strip().lower()] = match.group(2).strip() + return fields or None + + +def parse_reviewer_lease_comment(body: str) -> dict[str, Any] | None: + fields = _parse_marker_comment(body, REVIEWER_LEASE_MARKER) + if not fields: + return None + return { + "lease_kind": "reviewer", + "pr_number": _parse_pr_ref(fields.get("pr")), + "issue_number": _parse_pr_ref(fields.get("issue")), + "reviewer_identity": fields.get("reviewer_identity"), + "profile": fields.get("profile"), + "session_id": fields.get("session_id"), + "worktree": fields.get("worktree"), + "phase": (fields.get("phase") or "").strip().lower() or None, + "candidate_head": _normalize_sha(fields.get("candidate_head")), + "target_branch": fields.get("target_branch"), + "target_branch_sha": _normalize_sha(fields.get("target_branch_sha")), + "last_activity": fields.get("last_activity"), + "expires_at": fields.get("expires_at"), + "blocker": fields.get("blocker"), + "raw_fields": fields, + } + + +def parse_conflict_fix_lease_comment(body: str) -> dict[str, Any] | None: + fields = _parse_marker_comment(body, CONFLICT_FIX_LEASE_MARKER) + if not fields: + return None + ff = (fields.get("fast_forward") or "").strip().lower() + reviewer_active = (fields.get("reviewer_active") or "").strip().lower() + return { + "lease_kind": "conflict_fix", + "pr_number": _parse_pr_ref(fields.get("pr")), + "branch": fields.get("branch"), + "worktree": fields.get("worktree"), + "profile": fields.get("profile"), + "session_id": fields.get("session_id"), + "phase": (fields.get("phase") or "").strip().lower() or None, + "head_before": _normalize_sha(fields.get("head_before")), + "head_after": _normalize_sha(fields.get("head_after")), + "expires_at": fields.get("expires_at"), + "reviewer_active": reviewer_active in {"yes", "true", "1"}, + "fast_forward": ff in {"yes", "true", "1"}, + "raw_fields": fields, + } + + +def _comment_entries(comments: list[dict], *, pr_number: int | None) -> list[dict]: + entries: list[dict] = [] + for comment in comments or []: + body = comment.get("body") or "" + for parser in (parse_reviewer_lease_comment, parse_conflict_fix_lease_comment): + parsed = parser(body) + if not parsed: + continue + if pr_number is not None and parsed.get("pr_number") not in (None, pr_number): + continue + entries.append({ + **parsed, + "comment_id": comment.get("id"), + "author": (comment.get("user") or {}).get("login") or comment.get("author"), + "created_at": comment.get("created_at"), + "updated_at": comment.get("updated_at"), + }) + break + return entries + + +def _lease_expired(lease: dict, *, now: datetime) -> bool: + expires_at = _parse_timestamp(lease.get("expires_at")) + return bool(expires_at and expires_at <= now) + + +def _lease_phase_active(lease: dict, *, active_phases: frozenset[str]) -> bool: + phase = (lease.get("phase") or "").strip().lower() + if phase in _TERMINAL_REVIEWER_PHASES or phase in _TERMINAL_CONFLICT_FIX_PHASES: + return False + return phase in active_phases or bool(phase and phase not in ( + _TERMINAL_REVIEWER_PHASES | _TERMINAL_CONFLICT_FIX_PHASES + )) + + +def find_active_reviewer_lease( + comments: list[dict], + *, + pr_number: int, + now: datetime | None = None, +) -> dict[str, Any] | None: + """Return the newest unexpired reviewer lease for *pr_number*, if any.""" + now = now or datetime.now(timezone.utc) + candidates = [ + entry for entry in _comment_entries(comments, pr_number=pr_number) + if entry.get("lease_kind") == "reviewer" + ] + for lease in reversed(candidates): + if _lease_expired(lease, now=now): + continue + phase = (lease.get("phase") or "").strip().lower() + if phase in _TERMINAL_REVIEWER_PHASES: + continue + if phase in _ACTIVE_REVIEWER_PHASES or phase: + return lease + return None + + +def find_active_conflict_fix_lease( + comments: list[dict], + *, + pr_number: int, + now: datetime | None = None, +) -> dict[str, Any] | None: + """Return the newest unexpired conflict-fix lease for *pr_number*, if any.""" + now = now or datetime.now(timezone.utc) + candidates = [ + entry for entry in _comment_entries(comments, pr_number=pr_number) + if entry.get("lease_kind") == "conflict_fix" + ] + for lease in reversed(candidates): + if _lease_expired(lease, now=now): + continue + phase = (lease.get("phase") or "").strip().lower() + if phase in _TERMINAL_CONFLICT_FIX_PHASES: + continue + if phase in _ACTIVE_CONFLICT_FIX_PHASES or phase: + return lease + return None + + +def format_conflict_fix_lease_body( + *, + pr_number: int, + branch: str, + worktree: str, + profile: str, + head_before: str, + phase: str = "claimed", + session_id: str = "unknown", + expires_at: datetime | None = None, + reviewer_active: bool = False, +) -> str: + expires = expires_at or ( + datetime.now(timezone.utc) + timedelta(minutes=DEFAULT_CONFLICT_FIX_TTL_MINUTES) + ) + expires_text = expires.astimezone(timezone.utc).replace(microsecond=0).isoformat().replace( + "+00:00", "Z" + ) + lines = [ + CONFLICT_FIX_LEASE_MARKER, + f"pr: #{pr_number}", + f"branch: {branch}", + f"worktree: {worktree}", + f"profile: {profile}", + f"session_id: {session_id}", + f"phase: {phase}", + f"head_before: {head_before}", + f"expires_at: {expires_text}", + f"reviewer_active: {'yes' if reviewer_active else 'no'}", + ] + return "\n".join(lines) + + +def assess_head_sha_equality( + reviewed_head_sha: str | None, + live_head_sha: str | None, +) -> dict[str, Any]: + """Fail closed when reviewed and live PR heads differ.""" + reviewed = _normalize_sha(reviewed_head_sha) + live = _normalize_sha(live_head_sha) + reasons: list[str] = [] + if not reviewed or not live: + reasons.append( + "reviewed/live head SHA missing or not full 40-hex; fail closed" + ) + elif reviewed != live: + reasons.append( + "PR head changed after validation; re-pin and re-validate before " + "approval or merge" + ) + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "reviewed_head_sha": reviewed, + "live_head_sha": live, + "head_changed": bool(reviewed and live and reviewed != live), + } + + +def assess_conflict_fix_push( + *, + pr_number: int, + comments: list[dict], + branch_head_before: str | None, + branch_head_after: str | None, + worktree_path: str | None, + push_cwd: str | None, + is_fast_forward: bool | None, + now: datetime | None = None, +) -> dict[str, Any]: + """Author pre-push gate: block when a reviewer holds an active lease.""" + now = now or datetime.now(timezone.utc) + reasons: list[str] = [] + reviewer_lease = find_active_reviewer_lease(comments, pr_number=pr_number, now=now) + conflict_lease = find_active_conflict_fix_lease(comments, pr_number=pr_number, now=now) + + if reviewer_lease: + reasons.append( + f"active reviewer lease on PR #{pr_number} " + f"(phase={reviewer_lease.get('phase')}); author push blocked" + ) + + head_before = _normalize_sha(branch_head_before) + head_after = _normalize_sha(branch_head_after) + if not head_before: + reasons.append("branch head before push missing or invalid SHA") + if head_after and head_before and head_before == head_after: + reasons.append("branch head unchanged; no push to perform") + + worktree = (worktree_path or "").strip() + cwd = (push_cwd or "").strip() + if not worktree: + reasons.append("worktree path required for conflict-fix push proof") + elif cwd and worktree and not cwd.rstrip("/").endswith(worktree.rstrip("/").split("/")[-1]): + if worktree not in cwd: + reasons.append( + f"push cwd '{cwd}' does not match session worktree '{worktree}'" + ) + + if is_fast_forward is False: + reasons.append("non-fast-forward push rejected for conflict-fix (fail closed)") + + if conflict_lease and conflict_lease.get("phase") == "pushing": + owner = conflict_lease.get("worktree") + if owner and worktree and owner != worktree: + reasons.append( + f"sibling conflict-fix lease active from worktree '{owner}'" + ) + + push_allowed = not reasons + return { + "push_allowed": push_allowed, + "block": not push_allowed, + "reasons": reasons, + "active_reviewer_lease": reviewer_lease, + "active_conflict_fix_lease": conflict_lease, + "branch_head_before": head_before, + "branch_head_after": head_after, + "reviewer_was_active": bool(reviewer_lease), + "fast_forward": is_fast_forward, + } + + +def assess_reviewer_mutation_blocked( + *, + pr_number: int, + comments: list[dict], + reviewed_head_sha: str | None, + live_head_sha: str | None, + mutation: str, + now: datetime | None = None, +) -> dict[str, Any]: + """Reviewer gate: block when conflict-fix lease active or head moved.""" + now = now or datetime.now(timezone.utc) + reasons: list[str] = [] + conflict_lease = find_active_conflict_fix_lease(comments, pr_number=pr_number, now=now) + if conflict_lease and (conflict_lease.get("phase") or "") in _ACTIVE_CONFLICT_FIX_PHASES: + reasons.append( + f"active conflict-fix lease on PR #{pr_number} " + f"(phase={conflict_lease.get('phase')}); reviewer {mutation} blocked" + ) + + head_check = assess_head_sha_equality(reviewed_head_sha, live_head_sha) + if head_check["block"]: + reasons.extend(head_check["reasons"]) + + if not _normalize_sha(reviewed_head_sha): + reasons.append( + f"reviewed head SHA required before reviewer {mutation} (fail closed)" + ) + + allowed = not reasons + return { + "mutation_allowed": allowed, + "block": not allowed, + "reasons": reasons, + "active_conflict_fix_lease": conflict_lease, + "head_check": head_check, + "reviewed_head_sha": head_check.get("reviewed_head_sha"), + "live_head_sha": head_check.get("live_head_sha"), + "push_during_validation": bool( + conflict_lease and conflict_lease.get("phase") in {"pushing", "pushed"} + ), + } + + +_REVIEWED_HEAD_RE = re.compile( + r"reviewed head sha\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_LIVE_HEAD_BEFORE_APPROVAL_RE = re.compile( + r"(?:live head sha before approval|final live head sha before approval)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_LIVE_HEAD_BEFORE_MERGE_RE = re.compile( + r"(?:live head sha before merge|final live head sha before merge)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_PUSH_DURING_VALIDATION_RE = re.compile( + r"push(?:es)? occurred during validation\s*:\s*(yes|no|true|false)", + re.IGNORECASE, +) +_CONFLICT_HEAD_BEFORE_RE = re.compile( + r"branch head before push\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_CONFLICT_HEAD_AFTER_RE = re.compile( + r"branch head after push\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_REVIEWER_LEASE_STATUS_RE = re.compile( + r"active reviewer lease status\s*:\s*(.+)$", + re.IGNORECASE | re.MULTILINE, +) +_FAST_FORWARD_RE = re.compile( + r"whether push was fast-forward\s*:\s*(yes|no|true|false)", + re.IGNORECASE, +) +_REVIEWER_ACTIVE_RE = re.compile( + r"whether any reviewer was active\s*:\s*(yes|no|true|false)", + 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).""" + 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) + live_approval = _normalize_sha( + _LIVE_HEAD_BEFORE_APPROVAL_RE.search(text).group(1) + if _LIVE_HEAD_BEFORE_APPROVAL_RE.search(text) + else None + ) + live_merge = _normalize_sha( + _LIVE_HEAD_BEFORE_MERGE_RE.search(text).group(1) + if _LIVE_HEAD_BEFORE_MERGE_RE.search(text) + else None + ) + push_during = _PUSH_DURING_VALIDATION_RE.search(text) + + if not reviewed: + reasons.append("reviewed head SHA not stated in final report") + if not live_approval: + reasons.append("final live head SHA before approval not stated") + if not live_merge: + reasons.append("final live head SHA before merge not stated") + if not push_during: + reasons.append("whether push occurred during validation not stated") + elif 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") + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "reviewed_head_sha": reviewed, + "live_head_sha_before_approval": live_approval, + "live_head_sha_before_merge": live_merge, + "push_during_validation": (push_during.group(1).lower() if push_during else None), + } + + +def assess_conflict_fix_final_report(report_text: str) -> dict[str, Any]: + """Final-report proof for conflict-fix push sessions (#399 AC 7).""" + text = report_text or "" + reasons: list[str] = [] + head_before = _normalize_sha( + _CONFLICT_HEAD_BEFORE_RE.search(text).group(1) + if _CONFLICT_HEAD_BEFORE_RE.search(text) + else None + ) + head_after = _normalize_sha( + _CONFLICT_HEAD_AFTER_RE.search(text).group(1) + if _CONFLICT_HEAD_AFTER_RE.search(text) + else None + ) + if not head_before: + reasons.append("branch head before push not stated") + if not head_after: + reasons.append("branch head after push not stated") + if not _REVIEWER_LEASE_STATUS_RE.search(text): + reasons.append("active reviewer lease status not stated") + if not _FAST_FORWARD_RE.search(text): + reasons.append("whether push was fast-forward not stated") + if not _REVIEWER_ACTIVE_RE.search(text): + reasons.append("whether any reviewer was active not stated") + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "branch_head_before": head_before, + "branch_head_after": head_after, + } \ No newline at end of file diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 5b0e10a..fe784ca 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -732,6 +732,24 @@ The final report must identify: * whether same-PR merge continuation was allowed * whether the run stopped as required +## 26B. Conflict-fix lease and stale-head protection (#399) + +Before validating, approving, or merging a PR: + +1. Check for an active conflict-fix lease on the PR; stop if one is active. +2. Pin `expected_head_sha` before validation and pass it to + `gitea_mark_final_review_decision`, `gitea_submit_pr_review`, and + `gitea_merge_pr`. +3. Re-fetch live PR head immediately before approval and merge; refuse when + live head differs from the reviewed SHA. + +Final reports must state: + +* reviewed head SHA +* final live head SHA before approval +* final live head SHA before merge +* whether any push occurred during validation + ## 27. Merge rules Before merge, rerun fresh live checks: diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index f450cd7..0ee51a1 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -577,6 +577,29 @@ After push, report: If push fails, stop and produce a recovery handoff. +## 20A. Conflict-fix lease and push gate (#399) + +When pushing to an existing PR branch to resolve merge conflicts: + +1. Call `gitea_acquire_conflict_fix_lease` before any push. +2. Call `gitea_assess_conflict_fix_push` immediately before `git push` with: + * branch head before push + * branch head after push (local) + * session worktree path + * push cwd + * whether the push is fast-forward +3. Do not push when a reviewer holds an active lease on the same PR. +4. Do not force-push. +5. Do not push from the main checkout or wrong cwd. + +Conflict-fix final reports must state: + +* branch head before push +* branch head after push +* active reviewer lease status +* whether push was fast-forward +* whether any reviewer was active + ## 21. PR creation rules Create a PR only if implementation and validation pass, unless project policy explicitly allows draft PRs with documented validation failures. diff --git a/tests/test_pr_work_lease.py b/tests/test_pr_work_lease.py new file mode 100644 index 0000000..8dbcb33 --- /dev/null +++ b/tests/test_pr_work_lease.py @@ -0,0 +1,207 @@ +#!/usr/bin/env python3 +"""Regression tests for conflict-fix and reviewer PR work leases (#399).""" + +from __future__ import annotations + +import os +import sys +import unittest +from datetime import datetime, timedelta, timezone + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from pr_work_lease import ( # noqa: E402 + CONFLICT_FIX_LEASE_MARKER, + REVIEWER_LEASE_MARKER, + assess_conflict_fix_final_report, + assess_conflict_fix_push, + assess_head_sha_equality, + assess_reviewer_mutation_blocked, + assess_reviewer_stale_head_final_report, + format_conflict_fix_lease_body, + parse_conflict_fix_lease_comment, + parse_reviewer_lease_comment, +) + +HEAD_A = "a" * 40 +HEAD_B = "b" * 40 +NOW = datetime(2026, 7, 7, 15, 0, tzinfo=timezone.utc) + + +def _reviewer_lease_body(*, phase: str = "validating", expires_minutes: int = 60) -> str: + expires = (NOW + timedelta(minutes=expires_minutes)).isoformat().replace("+00:00", "Z") + return "\n".join([ + REVIEWER_LEASE_MARKER, + "pr: #376", + "phase: " + phase, + f"candidate_head: {HEAD_A}", + f"expires_at: {expires}", + "profile: prgs-reviewer", + ]) + + +def _conflict_fix_body(*, phase: str = "claimed", worktree: str = "branches/fix-376") -> str: + expires = (NOW + timedelta(minutes=60)).isoformat().replace("+00:00", "Z") + return "\n".join([ + CONFLICT_FIX_LEASE_MARKER, + "pr: #376", + f"phase: {phase}", + f"worktree: {worktree}", + f"head_before: {HEAD_A}", + f"expires_at: {expires}", + "profile: prgs-author", + ]) + + +class TestLeaseParsing(unittest.TestCase): + def test_parse_reviewer_lease(self): + parsed = parse_reviewer_lease_comment(_reviewer_lease_body()) + self.assertEqual(parsed["pr_number"], 376) + self.assertEqual(parsed["phase"], "validating") + self.assertEqual(parsed["candidate_head"], HEAD_A) + + def test_parse_conflict_fix_lease(self): + parsed = parse_conflict_fix_lease_comment(_conflict_fix_body()) + self.assertEqual(parsed["pr_number"], 376) + self.assertEqual(parsed["phase"], "claimed") + + +class TestConflictFixPushGate(unittest.TestCase): + def test_blocks_push_during_active_reviewer_lease(self): + comments = [{"body": _reviewer_lease_body()}] + result = assess_conflict_fix_push( + pr_number=376, + comments=comments, + branch_head_before=HEAD_A, + branch_head_after=HEAD_B, + worktree_path="branches/fix-376", + push_cwd="/proj/branches/fix-376", + is_fast_forward=True, + now=NOW, + ) + self.assertFalse(result["push_allowed"]) + self.assertTrue(any("reviewer lease" in r for r in result["reasons"])) + + def test_rejects_non_fast_forward(self): + result = assess_conflict_fix_push( + pr_number=376, + comments=[], + branch_head_before=HEAD_A, + branch_head_after=HEAD_B, + worktree_path="branches/fix-376", + push_cwd="/proj/branches/fix-376", + is_fast_forward=False, + now=NOW, + ) + self.assertFalse(result["push_allowed"]) + self.assertTrue(any("non-fast-forward" in r for r in result["reasons"])) + + def test_wrong_cwd_push_attempt(self): + result = assess_conflict_fix_push( + pr_number=376, + comments=[], + branch_head_before=HEAD_A, + branch_head_after=HEAD_B, + worktree_path="branches/fix-376", + push_cwd="/proj/master", + is_fast_forward=True, + now=NOW, + ) + self.assertFalse(result["push_allowed"]) + self.assertTrue(any("cwd" in r.lower() for r in result["reasons"])) + + def test_sibling_conflict_fix_collision(self): + comments = [{"body": _conflict_fix_body(phase="pushing", worktree="branches/other")}] + result = assess_conflict_fix_push( + pr_number=376, + comments=comments, + branch_head_before=HEAD_A, + branch_head_after=HEAD_B, + worktree_path="branches/fix-376", + push_cwd="/proj/branches/fix-376", + is_fast_forward=True, + now=NOW, + ) + self.assertFalse(result["push_allowed"]) + self.assertTrue(any("sibling conflict-fix" in r for r in result["reasons"])) + + +class TestReviewerMutationGate(unittest.TestCase): + def test_blocks_review_during_conflict_fix(self): + comments = [{"body": _conflict_fix_body(phase="pushing")}] + result = assess_reviewer_mutation_blocked( + pr_number=376, + comments=comments, + reviewed_head_sha=HEAD_A, + live_head_sha=HEAD_A, + mutation="approve", + now=NOW, + ) + self.assertFalse(result["mutation_allowed"]) + self.assertTrue(any("conflict-fix lease" in r for r in result["reasons"])) + + def test_stale_head_blocks_approval(self): + result = assess_reviewer_mutation_blocked( + pr_number=376, + comments=[], + reviewed_head_sha=HEAD_A, + live_head_sha=HEAD_B, + mutation="merge", + now=NOW, + ) + self.assertFalse(result["mutation_allowed"]) + self.assertTrue(result["head_check"]["head_changed"]) + + def test_head_equality_required_fields(self): + result = assess_head_sha_equality(HEAD_A, HEAD_B) + self.assertFalse(result["proven"]) + self.assertTrue(result["head_changed"]) + + +class TestFinalReportProof(unittest.TestCase): + def test_reviewer_stale_head_report_requires_fields(self): + result = assess_reviewer_stale_head_final_report("no head proof here") + self.assertFalse(result["proven"]) + + def test_reviewer_stale_head_report_passes(self): + report = "\n".join([ + f"Reviewed head SHA: {HEAD_A}", + f"Final live head SHA before approval: {HEAD_A}", + f"Final live head SHA before merge: {HEAD_A}", + "Push occurred during validation: no", + ]) + result = assess_reviewer_stale_head_final_report(report) + self.assertTrue(result["proven"]) + + def test_conflict_fix_report_requires_fields(self): + result = assess_conflict_fix_final_report("incomplete") + self.assertFalse(result["proven"]) + + def test_conflict_fix_report_passes(self): + report = "\n".join([ + f"Branch head before push: {HEAD_A}", + f"Branch head after push: {HEAD_B}", + "Active reviewer lease status: none", + "Whether push was fast-forward: yes", + "Whether any reviewer was active: no", + ]) + result = assess_conflict_fix_final_report(report) + self.assertTrue(result["proven"]) + + +class TestFormatLease(unittest.TestCase): + def test_format_conflict_fix_lease_includes_marker(self): + body = format_conflict_fix_lease_body( + pr_number=376, + branch="feat/x", + worktree="branches/fix-376", + profile="prgs-author", + head_before=HEAD_A, + ) + self.assertIn(CONFLICT_FIX_LEASE_MARKER, body) + parsed = parse_conflict_fix_lease_comment(body) + self.assertEqual(parsed["pr_number"], 376) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file