diff --git a/author_mutation_worktree.py b/author_mutation_worktree.py index 04b6933..29dc05d 100644 --- a/author_mutation_worktree.py +++ b/author_mutation_worktree.py @@ -7,8 +7,11 @@ project's ``branches/`` directory, never from the stable control checkout. from __future__ import annotations import os +import subprocess BASE_BRANCHES = frozenset({"master", "main", "dev"}) +ACTIVE_WORKTREE_ENV = "GITEA_ACTIVE_WORKTREE" +AUTHOR_WORKTREE_ENV = "GITEA_AUTHOR_WORKTREE" def _normalize_path(path: str) -> str: @@ -48,6 +51,130 @@ def resolve_mutation_workspace( return os.path.realpath(project_root) +def _realpath_git_common_dir(workspace_path: str, common_dir: str) -> str: + """Resolve ``git rev-parse --git-common-dir`` relative to *workspace_path*.""" + raw = (common_dir or "").strip() + if not raw: + return raw + if os.path.isabs(raw): + return os.path.realpath(raw) + return os.path.realpath(os.path.join(workspace_path, raw)) + + +def resolve_canonical_repo_root(workspace_path: str, fallback_project_root: str) -> str: + """Return the stable repository root for *workspace_path* via git metadata (#460).""" + path = (workspace_path or "").strip() + fallback = os.path.realpath(fallback_project_root) + if not path: + return fallback + try: + res = subprocess.run( + ["git", "-C", path, "rev-parse", "--git-common-dir"], + capture_output=True, + text=True, + check=True, + ) + common = _realpath_git_common_dir(path, res.stdout) + except Exception: + return fallback + if common.endswith(f"{os.sep}.git"): + return os.path.dirname(common) + if os.path.basename(common) == ".git": + return os.path.dirname(common) + return fallback + + +def resolve_author_mutation_context( + worktree_path: str | None, + process_project_root: str, + *, + active_worktree_env: str | None = None, + author_worktree_env: str | None = None, +) -> dict: + """Shared workspace resolution for runtime_context and mutation guards (#460).""" + workspace = resolve_mutation_workspace( + worktree_path, + process_project_root, + active_worktree_env=active_worktree_env, + author_worktree_env=author_worktree_env, + ) + process_root = os.path.realpath(process_project_root) + # Canonical repository identity comes from the MCP process checkout (#460), + # not from the declared task workspace being validated. + canonical_root = resolve_canonical_repo_root(process_root, process_root) + return { + "workspace_path": workspace, + "process_project_root": process_root, + "canonical_repo_root": canonical_root, + "roots_aligned": canonical_root == process_root, + } + + +def assess_workspace_repo_membership( + *, + workspace_path: str, + canonical_repo_root: str, +) -> dict: + """Fail closed when *workspace_path* is not a git worktree of *canonical_repo_root*.""" + workspace = os.path.realpath(workspace_path) + root = os.path.realpath(canonical_repo_root) + reasons: list[str] = [] + + if not os.path.exists(workspace): + reasons.append(f"worktree path '{workspace}' does not exist") + return _membership_assessment(False, reasons, workspace, root, None) + + if not os.path.isdir(workspace): + reasons.append(f"worktree path '{workspace}' is not a directory") + return _membership_assessment(False, reasons, workspace, root, None) + + try: + res = subprocess.run( + ["git", "-C", workspace, "rev-parse", "--git-common-dir"], + capture_output=True, + text=True, + check=True, + ) + common_dir = _realpath_git_common_dir(workspace, res.stdout) + except Exception: + reasons.append(f"worktree '{workspace}' is not a valid git repository") + return _membership_assessment(False, reasons, workspace, root, None) + + expected_dir = os.path.realpath(os.path.join(root, ".git")) + if common_dir != expected_dir: + reasons.append( + f"worktree '{workspace}' does not belong to the target repository '{root}'" + ) + return _membership_assessment(not reasons, reasons, workspace, root, common_dir) + + +def _membership_assessment( + proven: bool, + reasons: list[str], + workspace: str, + root: str, + common_dir: str | None, +) -> dict: + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "workspace_path": workspace, + "canonical_repo_root": root, + "git_common_dir": common_dir, + } + + +def format_workspace_repo_membership_error(assessment: dict) -> str: + workspace = assessment.get("workspace_path") or "(unknown)" + root = assessment.get("canonical_repo_root") or "(unknown)" + reasons = "; ".join(assessment.get("reasons") or ["unknown repository membership violation"]) + return ( + f"Branches-only mutation guard (#274): {reasons} (fail closed). " + f"canonical repository root: {root}; workspace: {workspace}." + ) + + def assess_author_mutation_worktree( *, workspace_path: str, diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 09b4d5c..634e22c 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -274,12 +274,24 @@ is proven abandoned and the takeover is recorded. Gitea-Tools lease gates: `gitea_lock_issue` (fail-closed before author mutations), `status:in-progress`, and claim comments. `gitea_lock_issue` -records an `author_issue_work` lease in the issue-lock payload with issue -number, optional PR number, branch, worktree path, claimant identity/profile, -created timestamp, expiry timestamp, and last heartbeat timestamp. An active -same-issue/same-operation lease blocks duplicate work. An expired lease still -blocks takeover until a recovery review records why the prior work is abandoned, -completed, or unsafe to continue. +records an `author_issue_work` lease in a keyed lock file under +`GITEA_ISSUE_LOCK_DIR` (default `~/.cache/gitea-tools/issue-locks`), one file +per `remote` + `org` + `repo` + `issue_number`. The current MCP session binds +its active lock through a per-process pointer so concurrent repos/issues never +share one overwrite-prone slot (#443). + +Each lock payload includes issue number, optional PR number, branch, worktree +path, claimant identity/profile, created timestamp, expiry timestamp, and last +heartbeat timestamp. An active same-issue/same-operation lease blocks duplicate +work. An expired lease still blocks takeover until a recovery review records why +the prior work is abandoned, completed, or unsafe to continue. + +**Do not manually seed `/tmp/gitea_issue_lock.json` or any lock file as a normal +recovery path.** That global slot is deprecated and can clobber unrelated live +leases (#438). After an MCP restart, call `gitea_lock_issue` again — own-branch +adoption rebinds the session when the issue's exact branch already exists (#442). +`gitea_create_pr` resolves the durable keyed lock by session pointer or by +matching `head` branch without unsafe manual seeding. **Issue-lock recovery (#447):** Do not manually seed, restore, or delete `/tmp/gitea_issue_lock.json` as a normal recovery path. That file is global diff --git a/final_report_validator.py b/final_report_validator.py index 958a4a9..6b6edac 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -21,6 +21,7 @@ from review_proofs import ( assess_review_mutation_final_report, assess_validation_report, ) +from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ "review_pr", @@ -507,6 +508,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): @@ -615,6 +655,34 @@ def _rule_reviewer_main_checkout_baseline(report_text: str) -> list[dict[str, st ] +def _rule_reviewer_validation_status_vocabulary( + report_text: str, + *, + action_log: list[dict] | None = None, +) -> list[dict[str, str]]: + text = report_text or "" + if not re.search( + r"validation status|pr-head validation status|official validation status", + text, + re.IGNORECASE, + ): + return [] + result = assess_validation_status_vocabulary( + text, + command_log=action_log, + ) + if not result.get("block"): + return [] + return _findings_from_reasons( + "reviewer.validation_status_vocabulary", + result.get("reasons") or [], + field="Validation status", + severity="block", + safe_next_action=result.get("safe_next_action") + or "use a validation status that matches the proof path executed", + ) + + def _rule_reviewer_main_checkout_path(report_text: str) -> list[dict[str, str]]: text = report_text or "" if "baseline worktree path" not in text.lower(): @@ -1022,6 +1090,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_validation_structured, _rule_reviewer_linked_issue, _rule_reviewer_baseline_on_failure, + _rule_reviewer_validation_status_vocabulary, _rule_reviewer_main_checkout_baseline, _rule_reviewer_main_checkout_path, _rule_reviewer_already_landed_eligible, @@ -1030,6 +1099,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_workflow_load_boundary, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, + _rule_reviewer_stale_head_proof, ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, @@ -1054,6 +1124,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _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 1418780..12e456f 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -190,14 +190,22 @@ def _ensure_process_start_porcelain() -> str: def _resolve_preflight_workspace_path(worktree_path: str | None = None) -> str: """Resolve the workspace root inspected by pre-flight guards.""" - path = (worktree_path or "").strip() - if not path: - path = (os.environ.get(ACTIVE_WORKTREE_ENV) or "").strip() - if not path: - path = (os.environ.get(AUTHOR_WORKTREE_ENV) or "").strip() - if not path: - path = PROJECT_ROOT - return os.path.realpath(os.path.abspath(path)) + return author_mutation_worktree.resolve_mutation_workspace( + worktree_path, + PROJECT_ROOT, + active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), + author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), + ) + + +def _resolve_author_mutation_context(worktree_path: str | None = None) -> dict: + """Canonical workspace + repository root for runtime_context and guards (#460).""" + return author_mutation_worktree.resolve_author_mutation_context( + worktree_path, + PROJECT_ROOT, + active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), + author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), + ) def _get_git_root(path: str) -> str | None: @@ -267,21 +275,31 @@ def _format_preflight_files(files: list[str]) -> str: def _preflight_workspace_details(worktree_path: str | None, dirty_files: list[str]) -> dict: - workspace = _resolve_preflight_workspace_path(worktree_path) + ctx = _resolve_author_mutation_context(worktree_path) + workspace = ctx["workspace_path"] inspected_root = _get_git_root(workspace) - control_root = os.path.realpath(PROJECT_ROOT) + process_root = ctx["process_project_root"] + canonical_root = ctx["canonical_repo_root"] active_root = os.path.realpath(inspected_root or workspace) - if active_root == control_root: + if active_root == canonical_root: dirty_scope = "control checkout" else: dirty_scope = "active task workspace" - return { - "mcp_server_process_root": control_root, + details = { + "mcp_server_process_root": process_root, + "canonical_repository_root": canonical_root, "active_task_workspace_root": active_root, "inspected_git_root": inspected_root, "dirty_files": list(dirty_files), "dirty_scope": dirty_scope, + "workspace_roots_aligned": ctx["roots_aligned"], } + if not ctx["roots_aligned"]: + details["workspace_root_mismatch"] = ( + "runtime_context and mutation guard use canonical repository root " + f"'{canonical_root}' instead of MCP process root '{process_root}'" + ) + return details def _format_preflight_workspace_details(details: dict) -> str: @@ -402,16 +420,12 @@ def _enforce_branches_only_author_mutation(worktree_path: str | None = None) -> """#274: author mutations must run from a branches/ session worktree.""" if _preflight_resolved_role == "reviewer": return - workspace = author_mutation_worktree.resolve_mutation_workspace( - worktree_path, - PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), - ) + ctx = _resolve_author_mutation_context(worktree_path) + workspace = ctx["workspace_path"] git_state = issue_lock_worktree.read_worktree_git_state(workspace) assessment = author_mutation_worktree.assess_author_mutation_worktree( workspace_path=workspace, - project_root=PROJECT_ROOT, + project_root=ctx["canonical_repo_root"], current_branch=git_state.get("current_branch"), ) if assessment["block"]: @@ -440,43 +454,23 @@ def verify_preflight_purity(remote: str | None = None, worktree_path: str | None "Pre-flight order violation: Task capability (gitea_resolve_task_capability) has not been resolved (fail closed)" ) - workspace = author_mutation_worktree.resolve_mutation_workspace( - worktree_path, - PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), - ) + ctx = _resolve_author_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) - real_root = os.path.realpath(PROJECT_ROOT) - if real_workspace != real_root: + if real_workspace != process_root: if not _preflight_in_test_mode(): - if not os.path.exists(real_workspace): + membership = author_mutation_worktree.assess_workspace_repo_membership( + workspace_path=workspace, + canonical_repo_root=canonical_root, + ) + if membership["block"]: raise RuntimeError( - f"Branches-only mutation guard (#274): worktree path '{workspace}' does not exist (fail closed)" - ) - if not os.path.isdir(real_workspace): - raise RuntimeError( - f"Branches-only mutation guard (#274): worktree path '{workspace}' is not a directory (fail closed)" - ) - try: - res = subprocess.run( - ["git", "-C", real_workspace, "rev-parse", "--git-common-dir"], - capture_output=True, - text=True, - check=True, - ) - common_dir = os.path.realpath(res.stdout.strip()) - expected_dir = os.path.realpath(os.path.join(real_root, ".git")) - if common_dir != expected_dir: - raise RuntimeError( - f"Branches-only mutation guard (#274): worktree '{workspace}' does not belong to the target repository '{PROJECT_ROOT}' (fail closed)" + author_mutation_worktree.format_workspace_repo_membership_error( + membership ) - except Exception as e: - if isinstance(e, RuntimeError): - raise e - raise RuntimeError( - f"Branches-only mutation guard (#274): worktree '{workspace}' is not a valid git repository (fail closed)" ) dirty_files = sorted(_parse_porcelain_entries(_get_workspace_porcelain(workspace))) @@ -541,6 +535,9 @@ import review_workflow_load # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import issue_lock_provenance # noqa: E402 +import issue_lock_store # noqa: E402 +import issue_lock_adoption # noqa: E402 +import merge_approval_gate # noqa: E402 import already_landed_reconcile # noqa: E402 import author_mutation_worktree # noqa: E402 import issue_claim_heartbeat # noqa: E402 @@ -550,11 +547,13 @@ 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 -# Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, -# consumed by gitea_create_pr and scripts/worktree-start. +# Keyed issue-lock storage (#443): per remote/org/repo/issue files under +# GITEA_ISSUE_LOCK_DIR, bound to the current MCP session via a per-PID pointer. +# Legacy global path retained only for test/doc references — do not seed manually. ISSUE_LOCK_FILE = "/tmp/gitea_issue_lock.json" WORK_LEASE_TTL_HOURS = 4 AUTHOR_ISSUE_WORK_LEASE = "author_issue_work" @@ -586,15 +585,59 @@ def _parse_work_lease_timestamp(value: str | None) -> datetime | None: return None -def _load_existing_issue_lock() -> dict | None: - if not os.path.exists(ISSUE_LOCK_FILE): - return None +def _load_existing_issue_lock( + *, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + issue_number: int | None = None, +) -> dict | None: + if remote and org and repo and issue_number is not None: + return issue_lock_store.load_issue_lock( + remote=remote, + org=org, + repo=repo, + issue_number=issue_number, + ) + return issue_lock_store.read_session_issue_lock() + + +def _resolve_issue_lock_for_pr( + *, + remote: str, + org: str, + repo: str, + head: str, +) -> dict: + lock_data = issue_lock_store.read_session_issue_lock() + if not lock_data: + lock_data = issue_lock_store.find_lock_for_branch( + remote=remote, + org=org, + repo=repo, + branch_name=head, + ) + if not lock_data: + raise RuntimeError( + "Issue lock is missing (fail closed). Call gitea_lock_issue first." + ) + return lock_data + + +def _save_issue_lock(data: dict) -> str: + existing = issue_lock_store.load_issue_lock( + remote=str(data.get("remote") or ""), + org=str(data.get("org") or ""), + repo=str(data.get("repo") or ""), + issue_number=int(data.get("issue_number") or 0), + ) + overwrite_block = issue_lock_store.assess_foreign_lock_overwrite(existing, data) + if overwrite_block: + raise RuntimeError(overwrite_block) try: - with open(ISSUE_LOCK_FILE, "r", encoding="utf-8") as f: - data = json.load(f) - return data if isinstance(data, dict) else None - except Exception: - return None + return issue_lock_store.bind_session_lock(data) + except Exception as e: + raise RuntimeError(f"Could not write issue lock file: {e}") from e def _work_lease_claimant(host: str | None) -> dict: @@ -797,6 +840,19 @@ def _enforce_locked_issue_duplicate_recheck( return None +def _branch_entry_commit_sha(branch: dict | str) -> str | None: + """Best-effort head SHA for a Gitea branch entry (None when absent).""" + if not isinstance(branch, dict): + return None + commit = branch.get("commit") + if isinstance(commit, dict): + sha = commit.get("id") or commit.get("sha") + if sha: + return str(sha) + sha = branch.get("commit_sha") + return str(sha) if sha else None + + def _reveal_endpoints() -> bool: """Admin/debug opt-in (#120): include endpoint URLs and token source names in tool output. Off by default so normal LLM-facing responses @@ -1244,8 +1300,9 @@ def gitea_lock_issue( resolved_worktree = issue_lock_worktree.resolve_author_worktree_path( worktree_path, PROJECT_ROOT ) - active_lease_block = _active_work_lease_block( - _load_existing_issue_lock(), + h, o, r = _resolve(remote, host, org, repo) + active_lease_block = issue_lock_store.assess_same_issue_lease_conflict( + _load_existing_issue_lock(remote=remote, org=o, repo=r, issue_number=issue_number), issue_number=issue_number, branch_name=branch_name, worktree_path=resolved_worktree, @@ -1269,7 +1326,6 @@ def gitea_lock_issue( issue_lock_worktree.format_issue_lock_worktree_error(lock_assessment) ) - h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) duplicate_gate = _assess_issue_duplicate_gate( issue_number, @@ -1285,6 +1341,30 @@ def gitea_lock_issue( f"duplicate work gate blocked issue #{issue_number} (fail closed)" ])) + branch_url = f"{repo_api_url(h, o, r)}/branches" + try: + branches = api_get_all(branch_url, auth) + except Exception as e: + raise RuntimeError(f"Could not list branches to verify issue lock: {e}") + existing_branch_entries = [ + { + "name": _branch_entry_name(branch), + "commit_sha": _branch_entry_commit_sha(branch), + } + for branch in branches + ] + adoption = issue_lock_adoption.assess_own_branch_adoption( + issue_number=issue_number, + requested_branch=branch_name, + existing_branches=existing_branch_entries, + ) + if adoption["block"]: + competing = ", ".join(adoption["competing_branches"]) + raise ValueError( + f"Issue #{issue_number} already has matching branch '{competing}' " + "that is not the requested branch (fail closed)" + ) + work_lease = _build_author_issue_work_lease( issue_number=issue_number, branch_name=branch_name, @@ -1305,11 +1385,20 @@ def gitea_lock_issue( ), } - try: - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(data, f) - except Exception as e: - raise RuntimeError(f"Could not write issue lock file: {e}") + lock_file_path = _save_issue_lock(data) + lock_record = issue_lock_store.read_lock_file(lock_file_path) or data + freshness = issue_lock_store.assess_lock_freshness(lock_record) + competing = [ + entry + for entry in issue_lock_store.list_live_locks() + if entry.get("issue_number") != issue_number + ] + lock_proof = issue_lock_store.format_lock_proof( + lock_record, + freshness=freshness, + competing_live_locks=competing, + released=False, + ) agent_artifacts = agent_temp_artifacts.find_agent_temp_artifacts_from_porcelain( git_state.get("porcelain_status") or "" @@ -1324,7 +1413,24 @@ def gitea_lock_issue( "branch_name": branch_name, "worktree_path": resolved_worktree, "work_lease": work_lease, + "lock_file_path": lock_file_path, + "lock_freshness": freshness, + "lock_proof": lock_proof, } + if adoption["adopt"]: + result["adoption"] = issue_lock_adoption.build_adoption_proof( + issue_number=issue_number, + branch_name=branch_name, + assessment=adoption, + open_pr_checked=True, + competing_lock_checked=True, + lock_file_path=lock_file_path, + lock_file_status="written", + ) + result["message"] = ( + f"Adopted existing branch '{branch_name}' and locked issue " + f"#{issue_number} for recovery (fail-closed check complete)." + ) if agent_artifacts: result["warnings"] = [ "Agent temp artifacts at repo root (delete before implementation): " @@ -1416,15 +1522,8 @@ def gitea_create_pr( verify_preflight_purity(remote, worktree_path=worktree_path) h, o, r = _resolve(remote, host, org, repo) - # ── Issue Lock Validation (Issue #194 / #196) ── - if not os.path.exists(ISSUE_LOCK_FILE): - raise RuntimeError("Issue lock is missing (fail closed). Call gitea_lock_issue first.") - - try: - with open(ISSUE_LOCK_FILE, "r", encoding="utf-8") as f: - lock_data = json.load(f) - except Exception as e: - raise RuntimeError(f"Could not read issue lock file: {e} (fail closed)") + # ── Issue Lock Validation (Issue #194 / #196 / #443) ── + lock_data = _resolve_issue_lock_for_pr(remote=remote, org=o, repo=r, head=head) lock_provenance_check = issue_lock_provenance.assess_lock_file_for_create_pr( lock_data @@ -1449,6 +1548,15 @@ def gitea_create_pr( f"PR head branch '{head}' does not match locked branch '{locked_branch}' (fail closed)" ) + ownership = issue_lock_store.verify_lock_for_mutation( + lock_data, + issue_number=locked_issue, + branch_name=head, + worktree_path=worktree_path, + ) + if ownership["block"]: + raise ValueError(ownership["reasons"][0]) + # Check for forbidden terms anywhere in title/body forbidden_terms = ["equivalent", "related", "same as"] text_to_check = f"{title} {body}".lower() @@ -2377,6 +2485,10 @@ def gitea_get_pr_review_feedback( e for e in latest_by_reviewer.values() if e["verdict"] == "APPROVED" and not e["dismissed"] ] + approval_head = merge_approval_gate.assess_merge_approval_head( + current_head_sha=current_head, + latest_by_reviewer=latest_by_reviewer, + ) return { "success": True, "pr_number": pr_number, @@ -2387,6 +2499,9 @@ def gitea_get_pr_review_feedback( login: e["verdict"] for login, e in latest_by_reviewer.items()}, "has_blocking_change_requests": bool(blocking), "approval_visible": bool(approvals), + "approval_at_current_head": approval_head["approval_at_current_head"], + "latest_approved_head_sha": approval_head["latest_approved_head_sha"], + "stale_approval_block_reason": approval_head["stale_approval_block_reason"], "latest_reviewed_head_sha": latest_reviewed_head, "review_feedback_stale": bool( latest_reviewed_head and current_head @@ -2398,6 +2513,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, @@ -2501,6 +2660,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)" @@ -2510,6 +2674,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( @@ -2659,6 +2838,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. @@ -2979,13 +3191,7 @@ def _prepare_commit_payload_files(files: list[dict]) -> tuple[list[dict], list[d processed_files = [] source_proofs = [] - lock_data = {} - if os.path.exists(ISSUE_LOCK_FILE): - try: - with open(ISSUE_LOCK_FILE, "r", encoding="utf-8") as f: - lock_data = json.load(f) - except Exception: - pass + lock_data = issue_lock_store.read_session_issue_lock() or {} locked_worktree = lock_data.get("worktree_path") if locked_worktree: @@ -3314,8 +3520,27 @@ def gitea_merge_pr( if reasons: 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)" @@ -3370,6 +3595,9 @@ def gitea_merge_pr( result["permission_report"] = feedback["permission_report"] return result result["approval_visible"] = feedback.get("approval_visible") + result["approval_at_current_head"] = feedback.get("approval_at_current_head") + result["latest_approved_head_sha"] = feedback.get("latest_approved_head_sha") + result["review_feedback_stale"] = feedback.get("review_feedback_stale") result["has_blocking_change_requests"] = feedback.get( "has_blocking_change_requests") if feedback.get("has_blocking_change_requests"): @@ -3383,6 +3611,16 @@ def gitea_merge_pr( "completed before merge (fail closed)" ) return result + if not feedback.get("approval_at_current_head"): + reasons.append( + feedback.get("stale_approval_block_reason") + or ( + "approval does not apply to current PR head SHA " + "(fail closed); required next action: re-review PR at " + "current head before merge" + ) + ) + return result # Gate 8 — in-process mutation authority (#199): the last check before # the merge mutation, using the identity the eligibility gate proved. @@ -6676,6 +6914,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", @@ -6721,6 +7063,15 @@ def gitea_reconcile_issue_claims( heartbeat_lease_minutes=heartbeat_lease_minutes, reclaim_after_minutes=reclaim_after_minutes, ) + live_locks = issue_lock_store.list_live_locks() + inventory["live_issue_locks"] = live_locks + inventory["live_issue_lock_numbers"] = sorted( + { + int(entry["issue_number"]) + for entry in live_locks + if entry.get("issue_number") is not None + } + ) inventory["cleanup_plan"] = issue_claim_heartbeat.build_cleanup_plan(inventory) inventory["success"] = True inventory["performed"] = False diff --git a/issue_lock_adoption.py b/issue_lock_adoption.py new file mode 100644 index 0000000..8f8a8d6 --- /dev/null +++ b/issue_lock_adoption.py @@ -0,0 +1,156 @@ +"""Own-branch lock adoption / recovery for ``gitea_lock_issue`` (#442 / #443). + +When an issue's own already-pushed branch exists, lock reacquisition must be +allowed (adoption) instead of being treated as #400 duplicate competing work. +This module isolates the pure decision so it can be unit-tested apart from the +MCP server's live Gitea calls. + +Adoption is granted only for the issue's *exact* requested branch. Any other +branch that merely contains the same ``issue-`` marker is competing work and +stays fail-closed. Open-PR, competing-live-lock, capability, and worktree +safety checks are enforced by the caller before this decision is consulted; +this module additionally records whether they passed for proof purposes. +""" + +from __future__ import annotations + +import re + +ADOPT = "adopt_existing_branch" +BLOCK_COMPETING = "block_competing_branch" +NO_MATCH = "no_matching_branch" + + +def _branch_name(entry) -> str: + if isinstance(entry, dict): + return str(entry.get("name") or "") + return str(entry or "") + + +def _branch_sha(entry) -> str | None: + if isinstance(entry, dict): + sha = entry.get("commit_sha") + if sha: + return str(sha) + return None + + +def _branch_carries_issue_marker(branch_name: str, issue_number: int) -> bool: + """Return True when *branch_name* references issue *issue_number* exactly. + + Uses a numeric word-boundary so ``issue-42`` does not match inside + ``issue-420`` (AC6 / #440). + """ + name = (branch_name or "").strip() + if not name: + return False + pattern = rf"(?:^|/)issue-{int(issue_number)}(?![0-9])" + return re.search(pattern, name) is not None + + +def assess_own_branch_adoption( + *, + issue_number: int, + requested_branch: str, + existing_branches, +) -> dict: + """Decide whether an existing matching branch is adoptable. + + Args: + issue_number: The tracking issue number being locked. + requested_branch: The exact branch the caller wants to lock. + existing_branches: Iterable of remote branch entries — either names or + dicts with ``name`` and optional ``commit_sha``. + + Returns: + dict with: + * ``outcome`` — one of ADOPT / BLOCK_COMPETING / NO_MATCH + * ``adopt`` (bool), ``block`` (bool) + * ``reason`` (str) + * ``matched_branch`` (str | None), ``matched_head_sha`` (str | None) + * ``competing_branches`` (list[str]) + + ADOPT: the issue's exact branch exists and no other same-issue branch does. + BLOCK_COMPETING: at least one same-issue branch is not the requested branch. + NO_MATCH: no branch carries the issue marker — normal lock path applies. + """ + requested = (requested_branch or "").strip() + + matches: list[tuple[str, str | None]] = [] + for entry in existing_branches or []: + name = _branch_name(entry).strip() + if _branch_carries_issue_marker(name, issue_number): + matches.append((name, _branch_sha(entry))) + + competing = sorted({name for name, _ in matches if name != requested}) + exact = [(name, sha) for name, sha in matches if name == requested] + + # Fail closed whenever any non-requested same-issue branch exists, even if + # the requested branch is also present: ownership is then ambiguous. + if competing: + return { + "outcome": BLOCK_COMPETING, + "adopt": False, + "block": True, + "reason": ( + f"issue #{issue_number} already has matching branch(es) " + f"{competing} that are not the requested branch " + f"'{requested}' (fail closed)" + ), + "matched_branch": None, + "matched_head_sha": None, + "competing_branches": competing, + } + + if exact: + name, sha = exact[0] + return { + "outcome": ADOPT, + "adopt": True, + "block": False, + "reason": ( + f"existing branch '{name}' is the exact requested branch for " + f"issue #{issue_number}; adopting it for lock recovery" + ), + "matched_branch": name, + "matched_head_sha": sha, + "competing_branches": [], + } + + return { + "outcome": NO_MATCH, + "adopt": False, + "block": False, + "reason": f"no existing branch matches issue #{issue_number}", + "matched_branch": None, + "matched_head_sha": None, + "competing_branches": [], + } + + +def build_adoption_proof( + *, + issue_number: int, + branch_name: str, + assessment: dict, + open_pr_checked: bool, + competing_lock_checked: bool, + lock_file_path: str, + lock_file_status: str, +) -> dict: + """Assemble the proof block returned by ``gitea_lock_issue`` on adoption. + + Requirement #4: adoption results must carry issue number, branch name, + branch head commit, adoption reason, no-existing-PR proof, no-competing- + live-lock proof, and lock file path/status. + """ + return { + "issue_number": issue_number, + "branch_name": branch_name, + "branch_head_commit": assessment.get("matched_head_sha"), + "adoption_reason": assessment.get("reason"), + "no_existing_pr_proof": bool(open_pr_checked), + "no_competing_live_lock_proof": bool(competing_lock_checked), + "lock_file_path": lock_file_path, + "lock_file_status": lock_file_status, + } \ No newline at end of file diff --git a/issue_lock_store.py b/issue_lock_store.py new file mode 100644 index 0000000..6a7b4d7 --- /dev/null +++ b/issue_lock_store.py @@ -0,0 +1,612 @@ +"""Keyed, persistent issue-lock storage (#443) with flock hardening (#438). + +Replaces the single global ``/tmp/gitea_issue_lock.json`` slot with per-issue +lock files under ``GITEA_ISSUE_LOCK_DIR`` (default +``~/.cache/gitea-tools/issue-locks``). Each MCP session binds its active lock +via a per-process pointer file so concurrent repos/issues never clobber each +other. Acquisition is serialized per issue with ``fcntl.flock``. +""" + +from __future__ import annotations + +import errno +import fcntl +import json +import os +import re +import tempfile +from contextlib import contextmanager +from datetime import datetime, timedelta, timezone +from typing import Any + +LOCK_DIR_ENV = "GITEA_ISSUE_LOCK_DIR" +DEFAULT_LOCK_DIR = os.path.expanduser("~/.cache/gitea-tools/issue-locks") +WORK_LEASE_TTL_HOURS = 4 +AUTHOR_ISSUE_WORK_LEASE = "author_issue_work" + +_SAFE_SEGMENT_RE = re.compile(r"[^A-Za-z0-9._+-]+") + + +class LockContentionError(RuntimeError): + """Raised when an exclusive per-issue lock cannot be acquired.""" + + +def default_lock_dir() -> str: + raw = (os.environ.get(LOCK_DIR_ENV) or DEFAULT_LOCK_DIR).strip() + return raw or DEFAULT_LOCK_DIR + + +def _sanitize_segment(value: str) -> str: + text = (value or "").strip() + if not text: + return "_" + return _SAFE_SEGMENT_RE.sub("_", text) + + +def lock_key( + *, + remote: str, + org: str, + repo: str, + issue_number: int, +) -> str: + return "-".join( + _sanitize_segment(part) + for part in (remote, org, repo, str(issue_number)) + ) + + +def lock_file_path( + *, + remote: str, + org: str, + repo: str, + issue_number: int, + lock_dir: str | None = None, +) -> str: + root = (lock_dir or default_lock_dir()).strip() + return os.path.join(root, f"{lock_key(remote=remote, org=org, repo=repo, issue_number=issue_number)}.json") + + +def session_pointer_path(lock_dir: str | None = None) -> str: + root = (lock_dir or default_lock_dir()).strip() + return os.path.join(root, f"session-{os.getpid()}.json") + + +def _ensure_lock_dir(lock_dir: str | None = None) -> str: + root = (lock_dir or default_lock_dir()).strip() + os.makedirs(root, mode=0o700, exist_ok=True) + return root + + +def flock_path(json_path: str) -> str: + return f"{json_path}.lock" + + +def is_process_alive(pid: int | None) -> bool: + if not pid or pid <= 0: + return False + try: + os.kill(int(pid), 0) + return True + except OSError as exc: + return exc.errno != errno.ESRCH + except (TypeError, ValueError): + return False + + +@contextmanager +def _exclusive_file_lock(lock_path: str): + os.makedirs(os.path.dirname(lock_path) or ".", exist_ok=True) + fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o600) + try: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError as exc: + raise LockContentionError( + f"could not acquire exclusive lock on '{lock_path}'" + ) from exc + yield fd + finally: + try: + fcntl.flock(fd, fcntl.LOCK_UN) + finally: + os.close(fd) + + +def read_lock_file(path: str) -> dict[str, Any] | None: + lock_path = (path or "").strip() + if not lock_path or not os.path.exists(lock_path): + return None + try: + with open(lock_path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError): + return None + return data if isinstance(data, dict) else None + + +def save_lock_file(path: str, data: dict[str, Any]) -> None: + lock_path = (path or "").strip() + if not lock_path: + raise ValueError("lock path is required (fail closed)") + parent = os.path.dirname(lock_path) or "." + os.makedirs(parent, mode=0o700, exist_ok=True) + payload = json.dumps(data, indent=2, sort_keys=True) + "\n" + fd, temp_path = tempfile.mkstemp(prefix=".lock-", suffix=".json", dir=parent) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(payload) + handle.flush() + os.fsync(handle.fileno()) + os.replace(temp_path, lock_path) + finally: + if os.path.exists(temp_path): + try: + os.remove(temp_path) + except OSError: + pass + + +def bind_session_lock(lock_data: dict[str, Any], lock_dir: str | None = None) -> str: + """Persist a keyed lock and bind it to the current process session.""" + remote = str(lock_data.get("remote") or "") + org = str(lock_data.get("org") or "") + repo = str(lock_data.get("repo") or "") + issue_number = int(lock_data.get("issue_number") or 0) + if not remote or not org or not repo or issue_number <= 0: + raise ValueError("lock record must include remote, org, repo, and issue_number") + + root = _ensure_lock_dir(lock_dir) + path = lock_file_path( + remote=remote, + org=org, + repo=repo, + issue_number=issue_number, + lock_dir=root, + ) + record = dict(lock_data) + record["lock_file_path"] = path + record["session_pid"] = os.getpid() + record.setdefault("pid", os.getpid()) + + pointer = { + "pid": os.getpid(), + "lock_file_path": path, + "issue_number": issue_number, + "branch_name": record.get("branch_name"), + "remote": remote, + "org": org, + "repo": repo, + } + sentinel = flock_path(path) + try: + with _exclusive_file_lock(sentinel): + existing = read_lock_file(path) + overwrite_block = assess_foreign_lock_overwrite(existing, record) + if overwrite_block: + raise RuntimeError(overwrite_block) + lease_block = assess_same_issue_lease_conflict( + existing, + issue_number=issue_number, + branch_name=str(record.get("branch_name") or ""), + worktree_path=str(record.get("worktree_path") or ""), + ) + if lease_block: + raise RuntimeError(lease_block) + save_lock_file(path, record) + save_lock_file(session_pointer_path(root), pointer) + except LockContentionError as exc: + competing = read_lock_file(path) + if competing: + owner_pid = competing.get("session_pid") or competing.get("pid") + raise RuntimeError( + f"Issue #{issue_number} lock contention: {exc}; competing owner " + f"pid={owner_pid} (fail closed)" + ) from exc + raise RuntimeError(f"Issue #{issue_number} lock contention: {exc} (fail closed)") from exc + return path + + +def read_session_issue_lock(lock_dir: str | None = None) -> dict[str, Any] | None: + root = (lock_dir or default_lock_dir()).strip() + pointer = read_lock_file(session_pointer_path(root)) + if not pointer: + return None + lock_path = str(pointer.get("lock_file_path") or "").strip() + if not lock_path: + return None + return read_lock_file(lock_path) + + +def load_issue_lock( + *, + remote: str, + org: str, + repo: str, + issue_number: int, + lock_dir: str | None = None, +) -> dict[str, Any] | None: + return read_lock_file( + lock_file_path( + remote=remote, + org=org, + repo=repo, + issue_number=issue_number, + lock_dir=lock_dir, + ) + ) + + +def iter_lock_files(lock_dir: str | None = None) -> list[str]: + root = (lock_dir or default_lock_dir()).strip() + if not os.path.isdir(root): + return [] + paths: list[str] = [] + for name in os.listdir(root): + if not name.endswith(".json") or name.startswith("session-"): + continue + paths.append(os.path.join(root, name)) + return sorted(paths) + + +def find_lock_for_branch( + *, + remote: str, + org: str, + repo: str, + branch_name: str, + lock_dir: str | None = None, +) -> dict[str, Any] | None: + target = (branch_name or "").strip() + if not target: + return None + for path in iter_lock_files(lock_dir): + lock = read_lock_file(path) + if not lock: + continue + if ( + str(lock.get("remote") or "") == remote + and str(lock.get("org") or "") == org + and str(lock.get("repo") or "") == repo + and str(lock.get("branch_name") or "").strip() == target + ): + lock = dict(lock) + lock.setdefault("lock_file_path", path) + return lock + return None + + +def _lease_now(now: datetime | None = None) -> datetime: + return now or datetime.now(timezone.utc) + + +def _parse_lease_timestamp(value: str | None) -> datetime | None: + text = (value or "").strip() + if not text: + return None + try: + return datetime.fromisoformat(text.replace("Z", "+00:00")).astimezone(timezone.utc) + except ValueError: + return None + + +def lease_expires_at(lock: dict[str, Any] | None) -> datetime | None: + if not lock: + return None + lease = lock.get("work_lease") + if not isinstance(lease, dict): + return None + return _parse_lease_timestamp(lease.get("expires_at")) + + +def is_lease_expired(lock: dict[str, Any] | None, *, now: datetime | None = None) -> bool: + expires = lease_expires_at(lock) + if expires is None: + return False + return expires <= _lease_now(now) + + +def is_lease_live(lock: dict[str, Any] | None, *, now: datetime | None = None) -> bool: + return assess_lock_freshness(lock, now=now)["live"] + + +def assess_lock_freshness( + lock_data: dict[str, Any] | None, + *, + now: datetime | None = None, +) -> dict[str, Any]: + """Classify a lock as live, expired, stale, or absent.""" + current = _lease_now(now) + if not lock_data: + return { + "status": "absent", + "live": False, + "stale": False, + "reason": "no lock record", + } + + expires_at = lease_expires_at(lock_data) + lease = lock_data.get("work_lease") + heartbeat_at = _parse_lease_timestamp(lock_data.get("last_heartbeat_at")) + if heartbeat_at is None and isinstance(lease, dict): + heartbeat_at = _parse_lease_timestamp(lease.get("last_heartbeat_at")) + + pid = lock_data.get("session_pid") + if pid is None: + pid = lock_data.get("pid") + pid_alive = is_process_alive(pid) if pid is not None else False + + if expires_at and expires_at <= current: + return { + "status": "expired", + "live": False, + "stale": True, + "reason": f"lease expired at {expires_at.isoformat()}", + "pid_alive": pid_alive, + } + + if pid is not None and not pid_alive: + return { + "status": "stale", + "live": False, + "stale": True, + "reason": f"owner pid {pid} is not alive", + "pid_alive": False, + } + + return { + "status": "live", + "live": True, + "stale": False, + "reason": "lock heartbeat and lease are fresh", + "pid_alive": pid_alive, + "heartbeat_at": heartbeat_at.isoformat() if heartbeat_at else None, + "expires_at": expires_at.isoformat() if expires_at else None, + } + + +def _same_realpath(left: str | None, right: str | None) -> bool: + if not left or not right: + return False + try: + return os.path.realpath(left) == os.path.realpath(right) + except OSError: + return left == right + + +def assess_same_issue_lease_conflict( + existing_lock: dict[str, Any] | None, + *, + issue_number: int, + branch_name: str, + worktree_path: str, + operation_type: str = AUTHOR_ISSUE_WORK_LEASE, + now: datetime | None = None, +) -> str | None: + """Return a fail-closed error when a competing live lease blocks acquisition.""" + if not existing_lock: + return None + + existing_issue = existing_lock.get("issue_number") + lease = existing_lock.get("work_lease") + existing_operation = ( + lease.get("operation_type") + if isinstance(lease, dict) + else AUTHOR_ISSUE_WORK_LEASE + ) + if existing_issue != issue_number or existing_operation != operation_type: + return None + + existing_branch = existing_lock.get("branch_name") + existing_worktree = existing_lock.get("worktree_path") + same_owner = ( + existing_branch == branch_name + and _same_realpath(str(existing_worktree or ""), worktree_path) + ) + if is_lease_expired(existing_lock, now=now): + return ( + f"Issue #{issue_number} has an expired {operation_type} lease on " + f"branch '{existing_branch}' from worktree '{existing_worktree}'. " + "Recovery review is required before takeover (fail closed)" + ) + if same_owner: + return None + return ( + f"Issue #{issue_number} already has an active {operation_type} lease on " + f"branch '{existing_branch}' from worktree '{existing_worktree}' " + "(fail closed)" + ) + + +def assess_foreign_lock_overwrite( + existing_lock: dict[str, Any] | None, + incoming_lock: dict[str, Any], + *, + now: datetime | None = None, +) -> str | None: + """Block writes that would clobber an unrelated live lease on the same key.""" + if not existing_lock: + return None + + same_issue = existing_lock.get("issue_number") == incoming_lock.get("issue_number") + same_branch = existing_lock.get("branch_name") == incoming_lock.get("branch_name") + same_worktree = _same_realpath( + str(existing_lock.get("worktree_path") or ""), + str(incoming_lock.get("worktree_path") or ""), + ) + if same_issue and same_branch and same_worktree: + return None + if not is_lease_live(existing_lock, now=now): + return None + return ( + "Refusing to overwrite a live foreign issue lock " + f"(issue #{existing_lock.get('issue_number')}, " + f"branch '{existing_lock.get('branch_name')}', " + f"worktree '{existing_lock.get('worktree_path')}') (fail closed)" + ) + + +def find_live_lock_for_branch( + branch_name: str, + lock_dir: str | None = None, +) -> dict[str, Any] | None: + target = (branch_name or "").strip() + if not target: + return None + for path in iter_lock_files(lock_dir): + lock = read_lock_file(path) + if not lock: + continue + if str(lock.get("branch_name") or "").strip() != target: + continue + if not is_lease_live(lock): + continue + record = dict(lock) + record.setdefault("lock_file_path", path) + return record + return None + + +def resolve_locked_branch_for_session( + branch_name: str | None = None, + lock_dir: str | None = None, +) -> str: + if branch_name: + lock = find_live_lock_for_branch(branch_name, lock_dir) + if lock: + return str(lock.get("branch_name") or "") + lock = read_session_issue_lock(lock_dir) + return str((lock or {}).get("branch_name") or "") + + +def has_active_issue_lock( + branch: str, + *, + lock_dir: str | None = None, +) -> bool: + target = (branch or "").strip() + if not target: + return False + for path in iter_lock_files(lock_dir): + lock = read_lock_file(path) + if not lock: + continue + if str(lock.get("branch_name") or "").strip() != target: + continue + if is_lease_live(lock): + return True + return False + + +def verify_lock_for_mutation( + lock_data: dict[str, Any] | None, + *, + issue_number: int | None = None, + branch_name: str | None = None, + worktree_path: str | None = None, +) -> dict[str, Any]: + """Re-check lock ownership immediately before a mutation (#438).""" + reasons: list[str] = [] + if not lock_data: + return {"proven": False, "block": True, "reasons": ["issue lock is missing (fail closed)"]} + + freshness = assess_lock_freshness(lock_data) + if not freshness["live"]: + reasons.append(f"issue lock is not live: {freshness['reason']} (fail closed)") + + if issue_number is not None and lock_data.get("issue_number") != issue_number: + reasons.append( + f"issue lock targets #{lock_data.get('issue_number')}, expected #{issue_number} (fail closed)" + ) + + if branch_name is not None and lock_data.get("branch_name") != branch_name: + reasons.append( + f"issue lock branch '{lock_data.get('branch_name')}' does not match " + f"'{branch_name}' (fail closed)" + ) + + if worktree_path is not None: + locked = os.path.realpath(str(lock_data.get("worktree_path") or "")) + declared = os.path.realpath(worktree_path) + if locked != declared: + reasons.append( + f"issue lock worktree '{locked}' does not match declared '{declared}' (fail closed)" + ) + + return { + "proven": not reasons, + "block": bool(reasons), + "reasons": reasons, + "freshness": freshness, + "lock_proof": format_lock_proof(lock_data, freshness=freshness), + } + + +def list_live_locks( + *, + lock_dir: str | None = None, + now: datetime | None = None, +) -> list[dict[str, Any]]: + """Return live per-issue locks for queue visibility.""" + live: list[dict[str, Any]] = [] + for path in iter_lock_files(lock_dir): + record = read_lock_file(path) + if not record: + continue + freshness = assess_lock_freshness(record, now=now) + if not freshness["live"]: + continue + live.append( + { + "issue_number": record.get("issue_number"), + "branch_name": record.get("branch_name"), + "remote": record.get("remote"), + "org": record.get("org"), + "repo": record.get("repo"), + "worktree_path": record.get("worktree_path"), + "pid": record.get("session_pid") or record.get("pid"), + "claimant": ( + record.get("claimant") + or (record.get("work_lease") or {}).get("claimant") + ), + "freshness": freshness, + "lock_path": record.get("lock_file_path") or path, + } + ) + return live + + +def format_lock_proof( + lock_data: dict[str, Any] | None, + *, + freshness: dict[str, Any] | None = None, + competing_live_locks: list[dict[str, Any]] | None = None, + released: bool | None = None, +) -> str: + """Canonical issue-lock proof string for final reports.""" + if not lock_data: + return "issue lock proof: not acquired" + fresh = freshness or assess_lock_freshness(lock_data) + owner = lock_data.get("claimant") or {} + if not owner and isinstance(lock_data.get("work_lease"), dict): + owner = lock_data["work_lease"].get("claimant") or {} + parts = [ + "issue lock proof:", + f"acquired issue #{lock_data.get('issue_number')}", + f"branch {lock_data.get('branch_name')}", + f"owner {owner.get('profile') or 'unknown'}", + f"pid {lock_data.get('session_pid') or lock_data.get('pid')}", + f"freshness {fresh.get('status')}", + ] + if competing_live_locks is not None: + parts.append( + "no competing live lock" + if not competing_live_locks + else f"competing live locks {len(competing_live_locks)}" + ) + if released is True: + parts.append("lock released") + elif released is False: + parts.append("lock retained") + return "; ".join(parts) \ No newline at end of file diff --git a/merge_approval_gate.py b/merge_approval_gate.py new file mode 100644 index 0000000..08fc8ad --- /dev/null +++ b/merge_approval_gate.py @@ -0,0 +1,61 @@ +"""Merge approval must pin the current PR head SHA (#471). + +Formal APPROVED reviews that predate the live PR head must not satisfy +``gitea_merge_pr`` eligibility. Pure assessment helpers are isolated here +for hermetic unit tests apart from MCP HTTP calls. +""" + +from __future__ import annotations + + +def assess_merge_approval_head( + *, + current_head_sha: str | None, + latest_by_reviewer: dict, +) -> dict: + """Return whether a visible approval applies to the live PR head. + + Args: + current_head_sha: Current PR head commit SHA. + latest_by_reviewer: Map of reviewer login → review entry dicts with + ``verdict``, ``dismissed``, and ``reviewed_head_sha`` keys. + + Returns: + dict with ``approval_at_current_head``, ``latest_approved_head_sha``, + and ``stale_approval_block_reason`` (set when merge must fail closed). + """ + current = (current_head_sha or "").strip() + approved_entries = [ + entry + for entry in (latest_by_reviewer or {}).values() + if (entry.get("verdict") or "").upper() == "APPROVED" + and not entry.get("dismissed") + ] + at_current = any( + (entry.get("reviewed_head_sha") or "").strip() == current + for entry in approved_entries + if current + ) + latest_approved = None + if approved_entries: + latest_entry = sorted( + approved_entries, + key=lambda entry: ( + entry.get("submitted_at") or "", + entry.get("reviewed_head_sha") or "", + ), + )[-1] + latest_approved = (latest_entry.get("reviewed_head_sha") or "").strip() or None + reason = None + if approved_entries and not at_current: + reason = ( + f"stale approval: approved SHA '{latest_approved}' does not match " + f"current live PR head SHA '{current or '(unknown)'}' (fail closed); " + "required next action: re-review PR at current head before merge" + ) + + return { + "approval_at_current_head": at_current, + "latest_approved_head_sha": latest_approved, + "stale_approval_block_reason": reason, + } \ No newline at end of file diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index 60204f6..bf81258 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -13,9 +13,9 @@ import subprocess from typing import Any from reviewer_worktree import parse_dirty_tracked_files +import issue_lock_store PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) -ISSUE_LOCK_FILE = os.environ.get("GITEA_ISSUE_LOCK_FILE", "/tmp/gitea_issue_lock.json") CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) @@ -37,22 +37,18 @@ def resolve_worktree_path(project_root: str, branch: str) -> str: def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: - lock_path = (path or ISSUE_LOCK_FILE).strip() - if not lock_path or not os.path.exists(lock_path): - return None - try: - with open(lock_path, encoding="utf-8") as handle: - data = json.load(handle) - except (OSError, json.JSONDecodeError): - return None - return data if isinstance(data, dict) else None + if path: + return issue_lock_store.read_lock_file(path.strip()) + return issue_lock_store.read_session_issue_lock() def has_active_issue_lock(branch: str, lock_path: str | None = None) -> bool: - lock = read_issue_lock(lock_path) - if not lock: - return False - return (lock.get("branch_name") or "").strip() == (branch or "").strip() + if lock_path: + lock = issue_lock_store.read_lock_file(lock_path.strip()) + if not lock: + return False + return (lock.get("branch_name") or "").strip() == (branch or "").strip() + return issue_lock_store.has_active_issue_lock(branch) def collect_open_pr_heads(open_prs: list[dict[str, Any]]) -> set[str]: 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/scripts/worktree-start b/scripts/worktree-start index 09c3bee..a189164 100755 --- a/scripts/worktree-start +++ b/scripts/worktree-start @@ -38,13 +38,21 @@ fi branch="$1" start_ref="${2:-prgs/master}" +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +repo_root="$(cd "$script_dir/.." && pwd)" + # Enforce issue-linked, traceable branch names (issue → branch → worktree → PR). if [[ "$allow_unlinked" -eq 0 ]]; then - if [[ ! -f "/tmp/gitea_issue_lock.json" ]]; then - echo "Error: Issue lock file '/tmp/gitea_issue_lock.json' is missing. You must lock exactly one issue before branch creation (fail closed)." >&2 + locked_branch=$(python3 -c " +import sys +sys.path.insert(0, '$repo_root') +import issue_lock_store +print(issue_lock_store.resolve_locked_branch_for_session('$branch')) +") + if [[ -z "$locked_branch" ]]; then + echo "Error: No session issue lock is bound. Call gitea_lock_issue before branch creation (fail closed)." >&2 exit 2 fi - locked_branch=$(python3 -c "import json; print(json.load(open('/tmp/gitea_issue_lock.json')).get('branch_name', ''))") if [[ "$branch" != "$locked_branch" ]]; then echo "Error: Requested branch '$branch' does not match locked branch '$locked_branch' (fail closed)." >&2 exit 2 @@ -68,8 +76,6 @@ EOF fi fi -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -repo_root="$(cd "$script_dir/.." && pwd)" worktree_name="${branch//\//-}" worktree_path="$repo_root/branches/$worktree_name" diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 8e8dd96..0336d55 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -606,6 +606,28 @@ If the cause is unknown, do not erase the earlier failure with plain `gitea_validate_review_final_report` rejects reports that omit known earlier validation failures when `validation_session.observed_failures` is supplied. +## 21B. Validation status taxonomy (#406) + +When the final report summarizes how validation concluded, use one of these +**validation status** labels (distinct from per-command pass/fail entries): + +* `passed` — raw PR-head validation passed on the unmodified head. +* `failed` — raw PR-head validation failed and no allowed resolution path + was proven. +* `baseline-equivalent failure accepted` — only when a clean baseline + worktree under `branches/` proves matching failure signatures on the target + branch (baseline path, target SHA, exact commands, failure lists, and + `failure signatures match: true`). +* `raw-head failure resolved by merge simulation` — raw PR-head validation + failed, but merge simulation into the current target passed cleanly; report + merge simulation under `Worktree/index mutations` with full #317 proof. +* `passed after transient failure investigation` — a later run passed after an + earlier failure in the same session; document the failure history (#396). + +Do not use `baseline-equivalent failure accepted` when only merge simulation +resolved the failure. Do not use bare `passed` when raw PR-head validation +failed unless one of the resolution statuses above applies. + ## 22. Baseline validation rule Do not run tests in the main checkout. @@ -790,6 +812,24 @@ inventory before continuing. Final reports must include lease session id, acquisition proof, heartbeat status, and release/blocked status. +## 26C. 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: @@ -800,6 +840,7 @@ Before merge, rerun fresh live checks: * author safety * PR re-fetch * reviewed head SHA unchanged +* visible APPROVED review applies to the **current live PR head SHA** (`approval_at_current_head`); if the head moved after approval, re-review at the new head before merge (#471) * target branch freshly fetched * PR still open * PR still mergeable @@ -816,6 +857,7 @@ Do not merge if: * capability state is stale * worktree is dirty * PR head changed +* approval is stale (approved SHA ≠ current live head SHA) * validation failed * inventory was incomplete * PR is already landed diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index 5484259..ceef51c 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -607,6 +607,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_agent_temp_artifacts.py b/tests/test_agent_temp_artifacts.py index 1532143..7684d98 100644 --- a/tests/test_agent_temp_artifacts.py +++ b/tests/test_agent_temp_artifacts.py @@ -74,21 +74,24 @@ ISSUE_WRITE_ENV = { class TestIssueLockArtifactWarning(unittest.TestCase): def setUp(self): - self._env_patcher = patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True) + self._lock_dir = tempfile.TemporaryDirectory() + env = {**ISSUE_WRITE_ENV, "GITEA_ISSUE_LOCK_DIR": self._lock_dir.name} + self._env_patcher = patch.dict(os.environ, env, clear=True) self._env_patcher.start() def tearDown(self): self._env_patcher.stop() + self._lock_dir.cleanup() @patch( "mcp_server.issue_duplicate_context_fetcher", return_value=([], [], {"status": "not_claimed"}), ) + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server._auth", return_value="token x") @patch("mcp_server._resolve", return_value=("h", "o", "r")) - @patch("mcp_server.ISSUE_LOCK_FILE", new_callable=lambda: tempfile.mktemp()) @patch("issue_lock_worktree.read_worktree_git_state") - def test_lock_success_includes_artifact_warning(self, mock_state, _lock_file, *_mocks): + def test_lock_success_includes_artifact_warning(self, mock_state, *_mocks): mock_state.return_value = { "current_branch": "master", "porcelain_status": "?? _emit_payload.py\n", diff --git a/tests/test_audit.py b/tests/test_audit.py index 11cecd3..b233b15 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -284,21 +284,38 @@ class TestSimpleToolAudit(_AuditWiringBase): self.assertEqual(result["number"], 9) +_NO_PR_WORK_LEASE_BLOCK = {"block": False, "reasons": [], "mutation_allowed": True} + + class TestGatedToolAudit(_AuditWiringBase): def setUp(self): super().setUp() + from mcp_server import init_review_decision_lock from tests.test_mcp_server import _install_owned_reviewer_lease import reviewer_pr_lease + # init_review_decision_lock clears any prior session lease (#407). + init_review_decision_lock("prgs", "review_pr") self._lease_patch = _install_owned_reviewer_lease(8) self._lease_patch.start() self._auth_identity_patch = patch( "mcp_server._authenticated_username", return_value="reviewer-bot" ) self._auth_identity_patch.start() + self._pr_lease_comments_patch = patch( + "mcp_server._list_pr_lease_comments", return_value=[] + ) + self._pr_lease_comments_patch.start() + self._pr_work_lease_patch = patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ) + self._pr_work_lease_patch.start() self.addCleanup(self._auth_identity_patch.stop) self.addCleanup(self._lease_patch.stop) + self.addCleanup(self._pr_lease_comments_patch.stop) + self.addCleanup(self._pr_work_lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) def _pr(self, author, state="open", sha="abc123", mergeable=True): @@ -313,7 +330,8 @@ class TestGatedToolAudit(_AuditWiringBase): {"login": "merger-bot"}, self._pr("author-bot"), self._pr("author-bot"), [{"id": 1, "user": {"login": "reviewer-bot"}, "state": "APPROVED", - "submitted_at": "2026-07-06T10:00:00Z", "dismissed": False}], + "commit_id": "abc123", "submitted_at": "2026-07-06T10:00:00Z", + "dismissed": False}], {}, {"merged_commit_sha": "c1"}, ] env = self._env(GITEA_PROFILE_NAME="gitea-merger", @@ -349,7 +367,9 @@ class TestGatedToolAudit(_AuditWiringBase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_submit_review_success_audited(self, _auth, mock_api): + # mark_final_review_decision and submit each run eligibility (user + PR). mock_api.side_effect = [ + {"login": "reviewer-bot"}, self._pr("author-bot"), {"login": "reviewer-bot"}, self._pr("author-bot"), {"id": 7, "state": "APPROVED"}, [{"id": 7, "user": {"login": "reviewer-bot"}, "state": "APPROVED", @@ -358,23 +378,16 @@ class TestGatedToolAudit(_AuditWiringBase): env = self._env(GITEA_PROFILE_NAME="gitea-reviewer", GITEA_ALLOWED_OPERATIONS="read,review,approve") with patch.dict(os.environ, env, clear=True): - from mcp_server import init_review_decision_lock, gitea_mark_final_review_decision - from tests.test_mcp_server import _install_owned_reviewer_lease - import reviewer_pr_lease + from mcp_server import gitea_mark_final_review_decision - init_review_decision_lock("prgs", "review_pr") - gitea_mark_final_review_decision(8, "approve", remote="prgs") - lease_patch = _install_owned_reviewer_lease(8) - lease_patch.start() - try: - r = gitea_submit_pr_review( - pr_number=8, action="approve", - body="LGTM", remote="prgs", - final_review_decision_ready=True, - ) - finally: - lease_patch.stop() - reviewer_pr_lease.clear_session_lease() + gitea_mark_final_review_decision( + 8, "approve", expected_head_sha="abc123", remote="prgs", + ) + r = gitea_submit_pr_review( + pr_number=8, action="approve", + body="LGTM", remote="prgs", + final_review_decision_ready=True, + ) self.assertTrue(r["performed"]) recs = self._records() self.assertEqual(len(recs), 1) diff --git a/tests/test_commit_payloads.py b/tests/test_commit_payloads.py index 6307a09..d3b3e52 100644 --- a/tests/test_commit_payloads.py +++ b/tests/test_commit_payloads.py @@ -66,9 +66,12 @@ class TestCommitPayloads(unittest.TestCase): ) self.locked_worktree_path = os.path.realpath(self.locked_worktree_dir.name) - self.lock_file_path = "/tmp/gitea_issue_lock.json" + import issue_lock_store import issue_lock_provenance + self._lock_dir = tempfile.TemporaryDirectory() + os.environ["GITEA_ISSUE_LOCK_DIR"] = self._lock_dir.name + work_lease = { "operation_type": "author_issue_work", "issue_number": 263, @@ -89,8 +92,7 @@ class TestCommitPayloads(unittest.TestCase): claimant=work_lease.get("claimant"), ), } - with open(self.lock_file_path, "w", encoding="utf-8") as fh: - fh.write(json.dumps(self.lock_data)) + self.lock_file_path = issue_lock_store.bind_session_lock(self.lock_data) # Reset preflight status to bypass/pass verification in tests self.orig_whoami_called = mcp_server._preflight_whoami_called @@ -114,8 +116,7 @@ class TestCommitPayloads(unittest.TestCase): self._dir.cleanup() self.locked_worktree_dir.cleanup() - if os.path.exists(self.lock_file_path): - os.remove(self.lock_file_path) + self._lock_dir.cleanup() def _env(self, profile: str) -> dict: return { @@ -124,6 +125,7 @@ class TestCommitPayloads(unittest.TestCase): "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TEST_PORCELAIN": "", "GITEA_AUTHOR_WORKTREE": self.locked_worktree_path, + "GITEA_ISSUE_LOCK_DIR": self._lock_dir.name, } @patch("mcp_server.api_request") diff --git a/tests/test_create_issue_workspace_guard.py b/tests/test_create_issue_workspace_guard.py index 2cd9f41..8d1d1f6 100644 --- a/tests/test_create_issue_workspace_guard.py +++ b/tests/test_create_issue_workspace_guard.py @@ -106,20 +106,32 @@ class TestCreateIssueWorkspaceGuard(unittest.TestCase): @patch("gitea_mcp_server.issue_lock_worktree.read_worktree_git_state", return_value={"current_branch": "feat/issue-1"}) @patch("os.path.exists", return_value=True) @patch("os.path.isdir", return_value=True) + @patch("author_mutation_worktree.subprocess.run") @patch("subprocess.run") - def test_create_issue_wrong_repo_fails_closed(self, mock_run, mock_isdir, mock_exists, _git, _get_all, mock_api, _role, _ns, _prof, _auth): - # Mock subprocess.run for git --git-common-dir to return a different path - mock_res = MagicMock() - mock_res.stdout = "/Users/jasonwalker/Development/some-other-repo/.git\n" - mock_run.return_value = mock_res - + def test_create_issue_wrong_repo_fails_closed(self, mock_run, mock_amw_run, mock_isdir, mock_exists, _git, _get_all, mock_api, _role, _ns, _prof, _auth): wrong_repo_path = os.path.join(CONTROL_CHECKOUT_ROOT, "branches", "feat-issue-1") + def _subprocess_side_effect(cmd, *args, **kwargs): + mock_res = MagicMock(returncode=0) + if "--git-common-dir" in cmd: + cwd = cmd[cmd.index("-C") + 1] if "-C" in cmd else "" + if cwd == wrong_repo_path: + mock_res.stdout = "/Users/jasonwalker/Development/some-other-repo/.git\n" + else: + mock_res.stdout = f"{CONTROL_CHECKOUT_ROOT}/.git\n" + else: + mock_res.stdout = "" + return mock_res + + mock_run.side_effect = _subprocess_side_effect + mock_amw_run.side_effect = _subprocess_side_effect + with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT): - with self.assertRaises(RuntimeError) as ctx: - srv.gitea_create_issue( - title="Test issue", body="body", worktree_path=wrong_repo_path - ) + 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 + ) self.assertIn("does not belong to the target repository", str(ctx.exception)) @patch("gitea_mcp_server._auth", return_value=FAKE_AUTH) diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 672d82e..88c5b23 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -41,6 +41,10 @@ def _review_handoff(**overrides): "- Selected PR: #203", "- Reviewer eligibility: eligible", "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", "- Worktree path: branches/review-203", "- Worktree dirty: clean", "- Scratch worktree used: yes (branches/review-203)", diff --git a/tests/test_issue_lock_adoption.py b/tests/test_issue_lock_adoption.py new file mode 100644 index 0000000..a579cbc --- /dev/null +++ b/tests/test_issue_lock_adoption.py @@ -0,0 +1,138 @@ +"""Unit tests for own-branch lock adoption decision (#442 / #443).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from issue_lock_adoption import ( # noqa: E402 + ADOPT, + BLOCK_COMPETING, + NO_MATCH, + assess_own_branch_adoption, + build_adoption_proof, +) + +REQ = "feat/issue-420-server-code-parity" + + +class TestAssessOwnBranchAdoption(unittest.TestCase): + def test_exact_own_branch_is_adopted(self): + result = assess_own_branch_adoption( + issue_number=420, + requested_branch=REQ, + existing_branches=[{"name": REQ, "commit_sha": "934688a"}], + ) + self.assertEqual(result["outcome"], ADOPT) + self.assertTrue(result["adopt"]) + self.assertFalse(result["block"]) + self.assertEqual(result["matched_branch"], REQ) + self.assertEqual(result["matched_head_sha"], "934688a") + + def test_exact_own_branch_adopted_when_sha_missing(self): + result = assess_own_branch_adoption( + issue_number=420, requested_branch=REQ, existing_branches=[REQ] + ) + self.assertEqual(result["outcome"], ADOPT) + self.assertIsNone(result["matched_head_sha"]) + + def test_different_branch_same_issue_blocks(self): + result = assess_own_branch_adoption( + issue_number=420, + requested_branch=REQ, + existing_branches=[{"name": "feat/issue-420-other-work"}], + ) + self.assertEqual(result["outcome"], BLOCK_COMPETING) + self.assertTrue(result["block"]) + self.assertFalse(result["adopt"]) + self.assertIn("feat/issue-420-other-work", result["competing_branches"]) + self.assertIn("fail closed", result["reason"]) + + def test_own_branch_plus_competing_branch_blocks(self): + # Ambiguous ownership: fail closed even though the exact branch exists. + result = assess_own_branch_adoption( + issue_number=420, + requested_branch=REQ, + existing_branches=[{"name": REQ}, {"name": "feat/issue-420-rogue"}], + ) + self.assertEqual(result["outcome"], BLOCK_COMPETING) + self.assertTrue(result["block"]) + self.assertEqual(result["competing_branches"], ["feat/issue-420-rogue"]) + + def test_no_matching_branch_is_normal_path(self): + result = assess_own_branch_adoption( + issue_number=420, + requested_branch=REQ, + existing_branches=[{"name": "feat/issue-999-unrelated"}], + ) + self.assertEqual(result["outcome"], NO_MATCH) + self.assertFalse(result["block"]) + self.assertFalse(result["adopt"]) + + def test_empty_branch_list_is_normal_path(self): + result = assess_own_branch_adoption( + issue_number=420, requested_branch=REQ, existing_branches=[] + ) + self.assertEqual(result["outcome"], NO_MATCH) + + def test_higher_issue_number_branch_does_not_block_lower_issue_adoption(self): + # issue-420 must not be treated as competing work for issue #42. + own_branch = "feat/issue-42-widget" + result = assess_own_branch_adoption( + issue_number=42, + requested_branch=own_branch, + existing_branches=[ + {"name": own_branch, "commit_sha": "abc1234"}, + {"name": "feat/issue-420-server-code-parity"}, + ], + ) + self.assertEqual(result["outcome"], ADOPT) + self.assertTrue(result["adopt"]) + self.assertFalse(result["block"]) + self.assertEqual(result["matched_branch"], own_branch) + + def test_unrelated_higher_number_branch_is_ignored_without_own_branch(self): + result = assess_own_branch_adoption( + issue_number=42, + requested_branch="feat/issue-42-thing", + existing_branches=[{"name": "feat/issue-420-server-code-parity"}], + ) + self.assertEqual(result["outcome"], NO_MATCH) + self.assertFalse(result["block"]) + self.assertFalse(result["adopt"]) + + +class TestBuildAdoptionProof(unittest.TestCase): + def test_proof_has_all_required_fields(self): + assessment = assess_own_branch_adoption( + issue_number=420, + requested_branch=REQ, + existing_branches=[{"name": REQ, "commit_sha": "934688a"}], + ) + proof = build_adoption_proof( + issue_number=420, + branch_name=REQ, + assessment=assessment, + open_pr_checked=True, + competing_lock_checked=True, + lock_file_path="/tmp/example-lock.json", + lock_file_status="written", + ) + for key in ( + "issue_number", + "branch_name", + "branch_head_commit", + "adoption_reason", + "no_existing_pr_proof", + "no_competing_live_lock_proof", + "lock_file_path", + "lock_file_status", + ): + self.assertIn(key, proof) + self.assertEqual(proof["branch_head_commit"], "934688a") + self.assertTrue(proof["no_existing_pr_proof"]) + self.assertTrue(proof["no_competing_live_lock_proof"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_issue_lock_store.py b/tests/test_issue_lock_store.py new file mode 100644 index 0000000..d87fc73 --- /dev/null +++ b/tests/test_issue_lock_store.py @@ -0,0 +1,256 @@ +"""Unit tests for keyed issue-lock storage (#443) and flock hardening (#438).""" +import json +import os +import sys +import tempfile +import threading +import unittest +from datetime import datetime, timedelta, timezone +from pathlib import Path +from unittest import mock + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import issue_lock_store as ils # noqa: E402 + + +def _lease(expires_at: str) -> dict: + return { + "operation_type": ils.AUTHOR_ISSUE_WORK_LEASE, + "expires_at": expires_at, + "created_at": "2026-01-01T00:00:00Z", + "last_heartbeat_at": "2026-01-01T00:00:00Z", + } + + +def _lock_record(**overrides) -> dict: + record = { + "issue_number": 420, + "branch_name": "feat/issue-420-server-code-parity", + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": "Gitea-Tools", + "worktree_path": "/tmp/wt-420", + "work_lease": _lease("2999-01-01T00:00:00Z"), + } + record.update(overrides) + return record + + +class TestIssueLockStore(unittest.TestCase): + def setUp(self): + self._dir = tempfile.TemporaryDirectory() + self.lock_dir = self._dir.name + self._env = mock.patch.dict(os.environ, {"GITEA_ISSUE_LOCK_DIR": self.lock_dir}) + self._env.start() + + def tearDown(self): + self._env.stop() + self._dir.cleanup() + + def test_concurrent_repo_locks_do_not_overwrite(self): + lock_a = _lock_record( + issue_number=108, + branch_name="feat/issue-108-root-menu", + repo="mcp-control-plane", + worktree_path="/tmp/wt-108", + ) + lock_b = _lock_record( + issue_number=420, + branch_name="feat/issue-420-server-code-parity", + repo="Gitea-Tools", + worktree_path="/tmp/wt-420", + ) + path_a = ils.bind_session_lock(lock_a) + with mock.patch("os.getpid", return_value=9999): + path_b = ils.bind_session_lock(lock_b) + + self.assertNotEqual(path_a, path_b) + self.assertTrue(os.path.exists(path_a)) + self.assertTrue(os.path.exists(path_b)) + stored_a = ils.read_lock_file(path_a) + stored_b = ils.read_lock_file(path_b) + self.assertEqual(stored_a["issue_number"], 108) + self.assertEqual(stored_b["issue_number"], 420) + + def test_concurrent_issue_locks_same_repo_do_not_overwrite(self): + lock_a = _lock_record(issue_number=427, branch_name="feat/issue-427-a") + lock_b = _lock_record(issue_number=428, branch_name="feat/issue-428-b") + path_a = ils.bind_session_lock(lock_a) + with mock.patch("os.getpid", return_value=4242): + path_b = ils.bind_session_lock(lock_b) + + self.assertNotEqual(path_a, path_b) + self.assertEqual(ils.read_lock_file(path_a)["issue_number"], 427) + self.assertEqual(ils.read_lock_file(path_b)["issue_number"], 428) + + def test_foreign_live_lease_blocks_overwrite(self): + existing = _lock_record( + branch_name="feat/issue-420-other", + worktree_path="/tmp/other", + work_lease=_lease("2999-01-01T00:00:00Z"), + ) + path = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=420, + ) + ils.save_lock_file(path, existing) + + incoming = _lock_record(worktree_path="/tmp/mine") + block = ils.assess_foreign_lock_overwrite(existing, incoming) + self.assertIn("live foreign issue lock", block or "") + + def test_expired_lease_allows_takeover_with_conflict_check(self): + existing = _lock_record( + branch_name="feat/issue-420-other", + worktree_path="/tmp/other", + work_lease=_lease("2000-01-01T00:00:00Z"), + ) + incoming = _lock_record(worktree_path="/tmp/mine") + self.assertIsNone(ils.assess_foreign_lock_overwrite(existing, incoming)) + block = ils.assess_same_issue_lease_conflict( + existing, + issue_number=420, + branch_name="feat/issue-420-server-code-parity", + worktree_path="/tmp/mine", + ) + self.assertIn("Recovery review is required", block or "") + + def test_same_owner_lease_conflict_allows_refresh(self): + worktree = "/tmp/wt-420" + existing = _lock_record(worktree_path=worktree) + block = ils.assess_same_issue_lease_conflict( + existing, + issue_number=420, + branch_name="feat/issue-420-server-code-parity", + worktree_path=worktree, + ) + self.assertIsNone(block) + + def test_find_lock_for_branch_after_restart(self): + record = _lock_record() + path = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=420, + ) + ils.save_lock_file(path, record) + + with mock.patch("os.getpid", return_value=5555): + self.assertIsNone(ils.read_session_issue_lock()) + + found = ils.find_lock_for_branch( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + branch_name="feat/issue-420-server-code-parity", + ) + self.assertEqual(found["issue_number"], 420) + + def test_has_active_issue_lock_scans_keyed_store(self): + ils.bind_session_lock(_lock_record()) + self.assertTrue( + ils.has_active_issue_lock("feat/issue-420-server-code-parity") + ) + self.assertFalse(ils.has_active_issue_lock("feat/issue-999-other")) + + def test_atomic_write_preserves_unrelated_lock(self): + path_a = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=108, + ) + ils.save_lock_file(path_a, _lock_record(issue_number=108, repo="mcp-control-plane")) + path_b = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=420, + ) + ils.save_lock_file(path_b, _lock_record()) + + self.assertTrue(os.path.exists(path_a)) + self.assertTrue(os.path.exists(path_b)) + self.assertEqual(ils.read_lock_file(path_a)["issue_number"], 108) + + def test_concurrent_bind_same_issue_only_one_wins(self): + barrier = threading.Barrier(2) + results: list[str | Exception] = [] + + def worker(): + barrier.wait() + try: + ils.bind_session_lock( + _lock_record(worktree_path=f"/tmp/wt-{threading.get_ident()}") + ) + results.append("ok") + except Exception as exc: # noqa: BLE001 + results.append(exc) + + threads = [threading.Thread(target=worker) for _ in range(2)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + successes = [item for item in results if item == "ok"] + failures = [item for item in results if isinstance(item, Exception)] + self.assertEqual(len(successes), 1) + self.assertEqual(len(failures), 1) + failure_text = str(failures[0]).lower() + self.assertTrue( + "active" in failure_text or "lock contention" in failure_text, + failures[0], + ) + + def test_verify_lock_for_mutation_blocks_stale_lock(self): + record = _lock_record( + work_lease=_lease("2000-01-01T00:00:00Z"), + ) + record["pid"] = 999999 + record["session_pid"] = 999999 + result = ils.verify_lock_for_mutation( + record, + issue_number=420, + branch_name="feat/issue-420-server-code-parity", + worktree_path="/tmp/wt-420", + ) + self.assertTrue(result["block"]) + self.assertIn("not live", result["reasons"][0]) + + def test_list_live_locks_excludes_stale_records(self): + live_path = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=420, + ) + ils.save_lock_file( + live_path, + _lock_record(worktree_path="/tmp/wt-420"), + ) + stale_path = ils.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=440, + ) + ils.save_lock_file( + stale_path, + _lock_record( + issue_number=440, + branch_name="feat/issue-440-recovery", + work_lease=_lease("2000-01-01T00:00:00Z"), + worktree_path="/tmp/wt-440", + ), + ) + live = ils.list_live_locks(lock_dir=self.lock_dir) + self.assertEqual([entry["issue_number"] for entry in live], [420]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_issue_work_duplicate_gate.py b/tests/test_issue_work_duplicate_gate.py index ac4ccea..1ae3024 100644 --- a/tests/test_issue_work_duplicate_gate.py +++ b/tests/test_issue_work_duplicate_gate.py @@ -1,5 +1,4 @@ """Tests for early duplicate-work detection (#400).""" -import json import os import sys import tempfile @@ -10,6 +9,7 @@ from unittest.mock import patch sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) import issue_lock_provenance +import issue_lock_store import issue_work_duplicate_gate as dup_gate import mcp_server from issue_work_duplicate_gate import ( @@ -123,8 +123,9 @@ class TestDuplicateReportOutcome(unittest.TestCase): class TestInjectableDuplicateFetcher(unittest.TestCase): + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server.get_auth_header", return_value="token x") - def test_lock_issue_uses_injected_fetcher(self, _auth): + def test_lock_issue_uses_injected_fetcher(self, _auth, _api): seen = {} def fetcher(h, o, r, auth, issue_number): @@ -141,26 +142,29 @@ class TestInjectableDuplicateFetcher(unittest.TestCase): "porcelain_status": "", "base_equivalent": True, }, - ), patch.dict(os.environ, { - "GITEA_ALLOWED_OPERATIONS": "gitea.issue.comment", - }, clear=True): - with patch.object(mcp_server, "ISSUE_LOCK_FILE", tempfile.mktemp()): - mcp_server.gitea_lock_issue( - issue_number=400, - branch_name="feat/issue-400-duplicate-work-preflight", - remote="prgs", - ) + ): + with tempfile.TemporaryDirectory() as lock_dir: + with patch.dict(os.environ, { + "GITEA_ALLOWED_OPERATIONS": "gitea.issue.comment", + "GITEA_ISSUE_LOCK_DIR": lock_dir, + }, clear=True): + mcp_server.gitea_lock_issue( + issue_number=400, + branch_name="feat/issue-400-duplicate-work-preflight", + remote="prgs", + ) self.assertEqual(seen["issue_number"], 400) class TestMcpDuplicateRecheck(unittest.TestCase): def setUp(self): self._dir = tempfile.TemporaryDirectory() - self.lock_path = os.path.join(self._dir.name, "gitea_issue_lock.json") - self._lock_patch = patch.object( - mcp_server, "ISSUE_LOCK_FILE", self.lock_path + self._env_patch = patch.dict( + os.environ, + {"GITEA_ISSUE_LOCK_DIR": self._dir.name}, + clear=False, ) - self._lock_patch.start() + self._env_patch.start() self._remotes = patch.dict(mcp_server.REMOTES, { "prgs": {"host": "gitea.example.com", "org": "Example-Org", "repo": "Example-Repo"}, @@ -173,6 +177,7 @@ class TestMcpDuplicateRecheck(unittest.TestCase): self._dir.cleanup() def _write_lock(self, issue_number=400, branch="feat/issue-400-x"): + worktree_path = os.path.realpath(os.getcwd()) work_lease = { "operation_type": "author_issue_work", "issue_number": issue_number, @@ -180,17 +185,19 @@ class TestMcpDuplicateRecheck(unittest.TestCase): "claimant": {"username": "test-user", "profile": "test-author"}, "expires_at": "2999-01-01T00:00:00Z", } - with open(self.lock_path, "w", encoding="utf-8") as fh: - json.dump({ - "issue_number": issue_number, - "branch_name": branch, - "remote": "prgs", - "work_lease": work_lease, - "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( - tool="gitea_lock_issue", - claimant=work_lease.get("claimant"), - ), - }, fh) + issue_lock_store.bind_session_lock({ + "issue_number": issue_number, + "branch_name": branch, + "remote": "prgs", + "org": "Example-Org", + "repo": "Example-Repo", + "worktree_path": worktree_path, + "work_lease": work_lease, + "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( + tool="gitea_lock_issue", + claimant=work_lease.get("claimant"), + ), + }) @patch("mcp_server._assess_issue_duplicate_gate") @patch("mcp_server.get_profile", return_value={ @@ -254,6 +261,7 @@ class TestMcpDuplicateRecheck(unittest.TestCase): base="master", body="Closes #400", remote="prgs", + worktree_path=os.path.realpath(os.getcwd()), ) self.assertFalse(result["success"]) self.assertIsNone(result.get("number")) diff --git a/tests/test_llm_agent_sha.py b/tests/test_llm_agent_sha.py index e751567..2751d9d 100644 --- a/tests/test_llm_agent_sha.py +++ b/tests/test_llm_agent_sha.py @@ -123,16 +123,19 @@ class TestShaCannotBypassSelfReview(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_review_tool_refuses_self_approval_despite_sha(self, _auth, mock_api, mock_get_all): - mock_get_all.return_value = [{"number": 9, "title": "PR 9", "state": "open", "head": {"ref": "branch9", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}] + from mcp_server import init_review_decision_lock + from tests.test_mcp_server import FULL_HEAD_SHA, _seed_ready_review_decision + + head_sha = FULL_HEAD_SHA + mock_get_all.return_value = [{"number": 9, "title": "PR 9", "state": "open", "head": {"ref": "branch9", "sha": head_sha}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}] mock_api.side_effect = [ {"login": "jcwalker3"}, # /user (inventory) {"login": "jcwalker3"}, # /user (submit eligibility) - {"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": "abc1234"}, "mergeable": True}, # /pulls/9 + {"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": head_sha}, "mergeable": True}, # /pulls/9 ] - from mcp_server import init_review_decision_lock, gitea_mark_final_review_decision init_review_decision_lock("prgs", "review_pr") - mcp_server.gitea_load_review_workflow() - gitea_mark_final_review_decision(9, "approve", remote="prgs") + with patch("mcp_server._list_pr_lease_comments", return_value=[]): + _seed_ready_review_decision(9, "approve", sha=head_sha, remote="prgs") env = self._env(SHA_WOULD_BE_REVIEWER, "reviewer") with patch.dict(os.environ, env, clear=True): r = gitea_review_pr( diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 4399a52..f557139 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -6,6 +6,7 @@ the MCP protocol) with mocked API responses. import json import os import sys +import tempfile import unittest from unittest.mock import patch, MagicMock @@ -45,8 +46,10 @@ from gitea_auth import get_profile # noqa: E402 import gitea_config # noqa: E402 import mcp_server +import issue_lock_store FAKE_AUTH = "Basic dGVzdDp0ZXN0" +FULL_HEAD_SHA = "a" * 40 _NO_BLOCKER_FEEDBACK = { "success": True, @@ -66,6 +69,7 @@ def _mark_request_changes_ready(pr_number=8, **kwargs): suppression feedback fetch stubbed to 'no existing blocker'.""" with patch("mcp_server.gitea_get_pr_review_feedback", return_value=dict(_NO_BLOCKER_FEEDBACK)): + kwargs.setdefault("expected_head_sha", "abc123") return gitea_mark_final_review_decision( pr_number, "request_changes", **kwargs) @@ -86,6 +90,7 @@ def _visible_approval_reviews(reviewer="reviewer-bot", sha="abc123"): _DEFAULT_LEASE_SESSION = "mcp-test-reviewer-lease" +_NO_PR_WORK_LEASE_BLOCK = {"block": False, "reasons": [], "mutation_allowed": True} def _reviewer_lease_comment( @@ -143,6 +148,46 @@ def _install_owned_reviewer_lease( ) +def _seed_ready_review_decision( + pr_number, + action, + *, + sha=FULL_HEAD_SHA, + remote="prgs", + org=None, + repo=None, +): + """Mark the review-decision lock ready without mark_final API calls (#399).""" + import mcp_server as _m + + resolved_org, resolved_repo = org, repo + if remote in _m.REMOTES: + _, resolved_org, resolved_repo = _m._resolve(remote, None, org, repo) + profile_name = (_m.get_profile().get("profile_name") or "").strip() + session_lock = ( + (os.environ.get(_m.SESSION_PROFILE_LOCK_ENV) or "").strip() + or profile_name + ) + _m._save_review_decision_lock({ + "task": "review_pr", + "remote": remote, + "session_pid": os.getpid(), + "session_profile": profile_name, + "session_profile_lock": session_lock, + "final_review_decision_ready": True, + "ready_pr_number": pr_number, + "ready_action": action, + "ready_expected_head_sha": sha, + "ready_remote": remote, + "ready_org": resolved_org, + "ready_repo": resolved_repo, + "live_mutations": [], + "correction_authorized": False, + "correction_reason": None, + }) + _m.gitea_load_review_workflow() + + # Issue-write tools are profile-gated (#69). ISSUE_WRITE_ENV = { "GITEA_ALLOWED_OPERATIONS": ( @@ -180,6 +225,7 @@ def _sample_issue_lock(issue_number=123, branch_name="feat/x", **overrides): "remote": "dadeschools", "org": "Scaled-Tech-Consulting", "repo": "Gitea-Tools", + "worktree_path": "/tmp/test-worktree", "work_lease": work_lease, "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( tool="gitea_lock_issue", @@ -195,6 +241,17 @@ def _clear_duplicate_context_fetcher(*_args, **_kwargs): return [], [], {"status": "not_claimed"} +def _bind_test_lock(**overrides) -> str: + remote = overrides.get("remote", "dadeschools") + record = _sample_issue_lock(**overrides) + if remote in mcp_server.REMOTES: + profile = mcp_server.REMOTES[remote] + record.setdefault("org", profile["org"]) + record.setdefault("repo", profile["repo"]) + record["remote"] = remote + return issue_lock_store.bind_session_lock(record) + + # --------------------------------------------------------------------------- # Create Issue # --------------------------------------------------------------------------- @@ -257,18 +314,21 @@ class TestCreatePR(unittest.TestCase): return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - @patch("os.path.exists", return_value=True) - @patch("builtins.open") - def test_creates_pr(self, mock_open, mock_exists, _auth, mock_api, _role, _dup_fetcher): - lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x")) - mock_open.return_value.__enter__.return_value.read.return_value = lock_json + def test_creates_pr(self, _auth, mock_api, _role, _dup_fetcher): + worktree = os.path.realpath(os.getcwd()) mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): - result = gitea_create_pr(title="feat: X Closes #123", head="feat/x", base="main") + with tempfile.TemporaryDirectory() as lock_dir: + env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} + with patch.dict(os.environ, env, clear=True): + _bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree) + result = gitea_create_pr( + title="feat: X Closes #123", + head="feat/x", + base="main", + worktree_path=worktree, + ) self.assertEqual(result["number"], 3) self.assertNotIn("url", result) - mock_exists.assert_called_with(ISSUE_LOCK_FILE) - mock_open.assert_called_with(ISSUE_LOCK_FILE, "r", encoding="utf-8") payload = mock_api.call_args[0][3] self.assertEqual(payload["head"], "feat/x") self.assertEqual(payload["base"], "main") @@ -282,30 +342,42 @@ class TestCreatePR(unittest.TestCase): return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - @patch("os.path.exists", return_value=True) - @patch("builtins.open") - def test_create_pr_reveal_opt_in_includes_url(self, mock_open, mock_exists, _auth, mock_api, _role, _dup_fetcher): - lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x")) - mock_open.return_value.__enter__.return_value.read.return_value = lock_json + def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _dup_fetcher): + worktree = os.path.realpath(os.getcwd()) mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} - env = {**CREATE_PR_ENV, "GITEA_MCP_REVEAL_ENDPOINTS": "1"} - with patch.dict(os.environ, env, clear=True): - result = gitea_create_pr(title="feat: X Closes #123", head="feat/x", base="main") + with tempfile.TemporaryDirectory() as lock_dir: + env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir, "GITEA_MCP_REVEAL_ENDPOINTS": "1"} + with patch.dict(os.environ, env, clear=True): + _bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree) + result = gitea_create_pr( + title="feat: X Closes #123", + head="feat/x", + base="main", + worktree_path=worktree, + ) self.assertIn("pulls/3", result["url"]) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - @patch("os.path.exists", return_value=True) - @patch("builtins.open") - def test_create_pr_locked_issue_mismatch_fails(self, mock_open, mock_exists, _auth, _role): - lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x")) - mock_open.return_value.__enter__.return_value.read.return_value = lock_json - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): - with self.assertRaises(ValueError) as ctx: - gitea_create_pr(title="feat: X Closes #999", head="feat/x", base="main") + def test_create_pr_locked_issue_mismatch_fails(self, _auth, _role): + worktree = os.path.realpath(os.getcwd()) + with tempfile.TemporaryDirectory() as lock_dir: + env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} + with patch.dict(os.environ, env, clear=True): + _bind_test_lock( + issue_number=123, + branch_name="feat/x", + worktree_path=worktree, + ) + with self.assertRaises(ValueError) as ctx: + gitea_create_pr( + title="feat: X Closes #999", + head="feat/x", + base="main", + worktree_path=worktree, + ) self.assertIn("Closes #123", str(ctx.exception)) - mock_open.assert_called_with(ISSUE_LOCK_FILE, "r", encoding="utf-8") # --------------------------------------------------------------------------- @@ -616,6 +688,17 @@ class TestMergePR(unittest.TestCase): self.addCleanup(self._auth_identity_patch.stop) self.addCleanup(self._lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) + self._pr_lease_comments_patch = patch( + "mcp_server._list_pr_lease_comments", return_value=[] + ) + self._pr_lease_comments_patch.start() + self.addCleanup(self._pr_lease_comments_patch.stop) + self._pr_work_lease_patch = patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ) + self._pr_work_lease_patch.start() + self.addCleanup(self._pr_work_lease_patch.stop) def _pr(self, author, state="open", sha="abc123", mergeable=True): return { @@ -689,7 +772,8 @@ class TestMergePR(unittest.TestCase): with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( pr_number=8, confirmation=self._confirm(8), - expected_changed_files=["b.py", "a.py"], remote="prgs") + expected_changed_files=["b.py", "a.py"], + expected_head_sha="abc123", remote="prgs") self.assertTrue(r["performed"]) # -- read-back / cleanup surfacing (#98) ----------------------------------- @@ -707,8 +791,10 @@ class TestMergePR(unittest.TestCase): env = {"GITEA_PROFILE_NAME": "gitea-merger", "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8), - remote="prgs") + r = gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) # The merge itself is still reported performed/successful. self.assertTrue(r["performed"]) self.assertEqual(r["merge_result"], "PR #8 merged via 'merge'.") @@ -736,8 +822,10 @@ class TestMergePR(unittest.TestCase): env = {"GITEA_PROFILE_NAME": "gitea-merger", "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8), - remote="prgs") + r = gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) self.assertTrue(r["performed"]) self.assertEqual(r["merge_commit"], "c9") self.assertTrue(r["cleanup_status"].startswith("skipped (cleanup error:")) @@ -751,7 +839,7 @@ class TestMergePR(unittest.TestCase): env = {"GITEA_PROFILE_NAME": "gitea-merger", "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr(pr_number=8, confirmation="", remote="prgs") + r = gitea_merge_pr(pr_number=8, confirmation="", expected_head_sha="abc123", remote="prgs") self.assertFalse(r["performed"]) self.assertTrue(any("explicit confirmation required" in x for x in r["reasons"])) mock_api.assert_not_called() @@ -762,7 +850,7 @@ class TestMergePR(unittest.TestCase): env = {"GITEA_PROFILE_NAME": "gitea-merger", "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr(pr_number=8, confirmation="MERGE PR 9", remote="prgs") + r = gitea_merge_pr(pr_number=8, confirmation="MERGE PR 9", expected_head_sha="abc123", remote="prgs") self.assertFalse(r["performed"]) mock_api.assert_not_called() @@ -896,7 +984,8 @@ class TestMergePR(unittest.TestCase): with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( pr_number=8, confirmation=self._confirm(8), - expected_changed_files=["a.py", "b.py"], remote="prgs") + expected_changed_files=["a.py", "b.py"], + expected_head_sha="abc123", remote="prgs") self.assertFalse(r["performed"]) self.assertIn( "PR changed files do not match expected_changed_files (fail closed)", @@ -909,7 +998,7 @@ class TestMergePR(unittest.TestCase): def test_invalid_merge_method_rejected(self, mock_api): with patch.dict(os.environ, {}, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation="MERGE PR 8", do="octopus", remote="prgs") + pr_number=8, confirmation="MERGE PR 8", do="octopus", expected_head_sha="abc123", remote="prgs") self.assertFalse(r["performed"]) self.assertTrue(any("unknown merge method" in x for x in r["reasons"])) mock_api.assert_not_called() @@ -927,7 +1016,9 @@ class TestMergePR(unittest.TestCase): "GITEA_TOKEN": "super-secret-token"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) blob = repr(r).lower() for secret in ("super-secret-token", "authorization", "basic ", FAKE_AUTH.lower()): self.assertNotIn(secret, blob) @@ -944,12 +1035,37 @@ class TestMergePR(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) self.assertFalse(r["performed"]) blob = repr(r) self.assertIn("[REDACTED]", blob) self.assertNotIn("abc-secret-xyz", blob) + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_merge_blocked_on_stale_approval_head(self, _auth, mock_api): + old_sha = "8b61c4b41f1b49b271ed3b99657431cf06eeda3e" + new_sha = "3e4b721d60e97147ba0704773cf57cd0d42cbe31" + mock_api.side_effect = [ + {"login": "merger-bot"}, self._pr("author-bot", sha=new_sha), + self._pr("author-bot", sha=new_sha), + [_formal_review("reviewer-bot", "APPROVED", sha=old_sha)], + ] + env = {"GITEA_PROFILE_NAME": "gitea-merger", + "GITEA_ALLOWED_OPERATIONS": "read,merge"} + with patch.dict(os.environ, env, clear=True): + r = gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs") + self.assertFalse(r["performed"]) + self.assertTrue(r.get("approval_visible")) + self.assertFalse(r.get("approval_at_current_head")) + self.assertTrue(any("stale approval" in x for x in r["reasons"])) + self.assertTrue(any(old_sha in x for x in r["reasons"])) + self.assertTrue(any(new_sha in x for x in r["reasons"])) + self._assert_no_merge_call(mock_api) + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_merge_blocked_without_visible_approval(self, _auth, mock_api): @@ -962,7 +1078,9 @@ class TestMergePR(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) self.assertFalse(r["performed"]) self.assertFalse(r.get("approval_visible")) self.assertTrue(any("no visible APPROVED review" in x for x in r["reasons"])) @@ -983,7 +1101,9 @@ class TestMergePR(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha="abc123", remote="prgs", + ) self.assertFalse(r["performed"]) self.assertTrue(r.get("has_blocking_change_requests")) self.assertTrue(any("REQUEST_CHANGES" in x for x in r["reasons"])) @@ -1084,15 +1204,18 @@ class TestReviewPR(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}] + head_sha = FULL_HEAD_SHA + mock_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": head_sha}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}] # mock_api responses: 1) /user (inventory), 2) /user (eligibility), 3) /pulls/1 (eligibility) mock_api.side_effect = [ {"login": "jcwalker3"}, # /api/v1/user (inventory) {"login": "jcwalker3"}, # /api/v1/user (submit eligibility) - {"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": "abc1234"}, "mergeable": True}, # /pulls/1 + {"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": head_sha}, "mergeable": True}, # /pulls/1 ] - _init_reviewer_session("prgs") - gitea_mark_final_review_decision(1, "approve", remote="prgs") + from mcp_server import init_review_decision_lock + init_review_decision_lock("prgs", "review_pr") + with patch("mcp_server._list_pr_lease_comments", return_value=[]): + _seed_ready_review_decision(1, "approve", sha=head_sha, remote="prgs") result = gitea_review_pr( pr_number=1, event="APPROVE", @@ -1748,7 +1871,7 @@ class TestReviewDecisionValidationGate(unittest.TestCase): """Block incidental live review mutations during validation.""" PR = 203 - SHA = "abc123" + SHA = FULL_HEAD_SHA def _pr(self, author, sha=SHA): return { @@ -1773,6 +1896,18 @@ class TestReviewDecisionValidationGate(unittest.TestCase): self.addCleanup(self._auth_identity_patch.stop) self.addCleanup(self._lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) + self._pr_lease_comments_patch = patch( + "mcp_server._list_pr_lease_comments", return_value=[] + ) + self._pr_lease_comments_patch.start() + self._pr_work_lease_patch = patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ) + self._pr_work_lease_patch.start() + self.addCleanup(self._pr_lease_comments_patch.stop) + self.addCleanup(self._pr_work_lease_patch.stop) + def _env(self): return patch.dict(os.environ, { @@ -1870,15 +2005,28 @@ class TestSubmitPrReview(unittest.TestCase): import reviewer_pr_lease _init_reviewer_session("prgs") - gitea_mark_final_review_decision(8, "approve", remote="prgs") self._lease_patch = _install_owned_reviewer_lease(8) self._lease_patch.start() self._auth_identity_patch = patch( "mcp_server._authenticated_username", return_value="reviewer-bot" ) self._auth_identity_patch.start() + self._pr_lease_comments_patch = patch( + "mcp_server._list_pr_lease_comments", return_value=[] + ) + self._pr_lease_comments_patch.start() + self._pr_work_lease_patch = patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ) + self._pr_work_lease_patch.start() + gitea_mark_final_review_decision( + 8, "approve", remote="prgs", expected_head_sha="abc123", + ) self.addCleanup(self._auth_identity_patch.stop) self.addCleanup(self._lease_patch.stop) + self.addCleanup(self._pr_lease_comments_patch.stop) + self.addCleanup(self._pr_work_lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) def _pr(self, author, state="open", sha="abc123", mergeable=True): @@ -2039,10 +2187,11 @@ class TestSubmitPrReview(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_comment_succeeds_when_review_eligible(self, _auth, mock_api): - gitea_mark_final_review_decision(8, "comment", remote="prgs") mock_api.side_effect = [ + {"login": "reviewer-bot"}, self._pr("author-bot"), {"login": "reviewer-bot"}, self._pr("author-bot"), {"id": 3}, ] + gitea_mark_final_review_decision(8, "comment", remote="prgs", expected_head_sha="abc123") env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review"} with patch.dict(os.environ, env, clear=True): @@ -2058,12 +2207,15 @@ class TestSubmitPrReview(unittest.TestCase): with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH), \ patch("mcp_server.api_request") as mock_api: mock_api.side_effect = [ + {"login": "jcwalker3"}, self._pr("jcwalker3"), {"login": "jcwalker3"}, self._pr("jcwalker3"), {"id": 4}, ] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review"} with patch.dict(os.environ, env, clear=True): - gitea_mark_final_review_decision(8, "comment", remote="prgs") + gitea_mark_final_review_decision( + 8, "comment", remote="prgs", expected_head_sha="abc123", + ) r = gitea_submit_pr_review( pr_number=8, action="comment", body="note", remote="prgs", final_review_decision_ready=True, @@ -2106,14 +2258,14 @@ class TestSubmitPrReview(unittest.TestCase): with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH), \ patch("mcp_server.api_request") as mock_api: mock_api.side_effect = [ - {"login": "reviewer-bot"}, self._pr("author-bot", sha="abc123"), + {"login": "reviewer-bot"}, self._pr("author-bot", sha="deadbeef"), ] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): r = gitea_submit_pr_review( pr_number=8, action="approve", - expected_head_sha="deadbeef", remote="prgs", + expected_head_sha="abc123", remote="prgs", final_review_decision_ready=True, ) self.assertFalse(r["performed"]) @@ -2164,7 +2316,7 @@ class TestSubmitPrReview(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,review,approve", "GITEA_TOKEN": "super-secret-token"} with patch.dict(os.environ, env, clear=True): - gitea_mark_final_review_decision(5, "approve", remote="prgs") + gitea_mark_final_review_decision(5, "approve", remote="prgs", expected_head_sha="abc123") r = gitea_submit_pr_review( pr_number=5, action="approve", remote="prgs", final_review_decision_ready=True, @@ -2184,7 +2336,6 @@ class TestSubmitPrReview(unittest.TestCase): env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): - gitea_mark_final_review_decision(8, "approve", remote="prgs") r = gitea_submit_pr_review( pr_number=8, action="approve", remote="prgs", final_review_decision_ready=True, @@ -2274,7 +2425,7 @@ class TestSubmitPrReview(unittest.TestCase): def test_mark_final_decision_rejects_remote_mismatch(self): _init_reviewer_session("prgs") - r = gitea_mark_final_review_decision(8, "approve", remote="dadeschools") + r = gitea_mark_final_review_decision(8, "approve", remote="dadeschools", expected_head_sha="abc123") self.assertFalse(r["marked_ready"]) self.assertTrue(any("does not match locked remote" in x for x in r["reasons"])) @@ -2286,28 +2437,30 @@ class TestSubmitPrReview(unittest.TestCase): {"id": 42, "state": "APPROVED"}, [_formal_review("reviewer-bot", "APPROVED", review_id=42)], {"login": "reviewer-bot"}, self._pr("author-bot"), + {"login": "reviewer-bot"}, self._pr("author-bot"), {"id": 43, "state": "REQUEST_CHANGES"}, [_formal_review("reviewer-bot", "REQUEST_CHANGES", review_id=43)], ] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve,request_changes"} - gitea_mark_final_review_decision(8, "approve", remote="prgs") - with patch.dict(os.environ, env, clear=True): - first = gitea_submit_pr_review( - pr_number=8, action="approve", remote="prgs", - final_review_decision_ready=True, - ) - auth = gitea_authorize_review_correction( - prior_review_id=42, - prior_review_state="approve", - reason="operator approved correcting mistaken approve", - operator_authorized=True, - ) - _mark_request_changes_ready(remote="prgs") - second = gitea_submit_pr_review( - pr_number=8, action="request_changes", remote="prgs", - final_review_decision_ready=True, - ) + with patch("mcp_server.gitea_get_pr_review_feedback", + return_value=dict(_NO_BLOCKER_FEEDBACK)): + with patch.dict(os.environ, env, clear=True): + first = gitea_submit_pr_review( + pr_number=8, action="approve", remote="prgs", + final_review_decision_ready=True, + ) + auth = gitea_authorize_review_correction( + prior_review_id=42, + prior_review_state="approve", + reason="operator approved correcting mistaken approve", + operator_authorized=True, + ) + _mark_request_changes_ready(remote="prgs") + second = gitea_submit_pr_review( + pr_number=8, action="request_changes", remote="prgs", + final_review_decision_ready=True, + ) self.assertTrue(first["performed"]) self.assertTrue(auth["authorized"]) self.assertTrue(second["performed"]) @@ -2319,7 +2472,7 @@ class TestSubmitPrReview(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): # Mark decision for specific remote, org, repo - gitea_mark_final_review_decision(8, "approve", remote="prgs", org="MyOrg", repo="MyRepo") + gitea_mark_final_review_decision(8, "approve", remote="prgs", expected_head_sha="abc123", org="MyOrg", repo="MyRepo") # Mismatched remote r = gitea_submit_pr_review( @@ -2362,6 +2515,11 @@ class TestTrackerHygieneCleanup(unittest.TestCase): self.mock_audit = patch("gitea_audit.write_event").start() # gitea.pr.close: closing a PR via gitea_edit_pr is capability-gated (#216). patch("mcp_server.get_profile", return_value={"profile_name": "test", "allowed_operations": ["read", "merge", "edit", "close", "gitea.pr.close", "gitea.issue.close"], "audit_label": "test", "forbidden_operations": []}).start() + patch("mcp_server._list_pr_lease_comments", return_value=[]).start() + patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ).start() def tearDown(self): patch.stopall() @@ -2408,7 +2566,8 @@ class TestTrackerHygieneCleanup(unittest.TestCase): def test_merge_pr_with_closes_removes_label(self): import reviewer_pr_lease - lease_patch = _install_owned_reviewer_lease(1, head_sha="sha123") + head_sha = FULL_HEAD_SHA + lease_patch = _install_owned_reviewer_lease(1, head_sha=head_sha) lease_patch.start() self.addCleanup(lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) @@ -2417,12 +2576,12 @@ class TestTrackerHygieneCleanup(unittest.TestCase): if method == "GET" and "/user" in url: return {"login": "merger"} if method == "GET" and url.endswith("/reviews"): - return [_formal_review("reviewer", "APPROVED", sha="sha123")] + return [_formal_review("reviewer", "APPROVED", sha=head_sha)] if method == "GET" and "pulls/1" in url and "/files" not in url: return { "user": {"login": "author"}, "state": "open", - "head": {"sha": "sha123", "ref": "feat/my-branch"}, + "head": {"sha": head_sha, "ref": "feat/my-branch"}, "base": {"ref": "main"}, "mergeable": True, "merged_commit_sha": "merge123", @@ -2442,14 +2601,18 @@ class TestTrackerHygieneCleanup(unittest.TestCase): return {} self.mock_api.side_effect = api_side_effect - res = gitea_merge_pr(pr_number=1, confirmation="MERGE PR 1", do="merge") + res = gitea_merge_pr( + pr_number=1, confirmation="MERGE PR 1", do="merge", + expected_head_sha=head_sha, + ) self.assertTrue(res["performed"]) self.assertEqual(res["cleanup_status"].get(123), "released") def test_merge_pr_with_branch_name_removes_label(self): import reviewer_pr_lease - lease_patch = _install_owned_reviewer_lease(1, head_sha="sha123") + head_sha = FULL_HEAD_SHA + lease_patch = _install_owned_reviewer_lease(1, head_sha=head_sha) lease_patch.start() self.addCleanup(lease_patch.stop) self.addCleanup(reviewer_pr_lease.clear_session_lease) @@ -2458,12 +2621,12 @@ class TestTrackerHygieneCleanup(unittest.TestCase): if method == "GET" and "/user" in url: return {"login": "merger"} if method == "GET" and url.endswith("/reviews"): - return [_formal_review("reviewer", "APPROVED", sha="sha123")] + return [_formal_review("reviewer", "APPROVED", sha=head_sha)] if method == "GET" and "pulls/1" in url and "/files" not in url: return { "user": {"login": "author"}, "state": "open", - "head": {"sha": "sha123", "ref": "fix/issue-123-slug"}, + "head": {"sha": head_sha, "ref": "fix/issue-123-slug"}, "base": {"ref": "main"}, "mergeable": True, "merged_commit_sha": "merge123", @@ -2483,7 +2646,10 @@ class TestTrackerHygieneCleanup(unittest.TestCase): return {} self.mock_api.side_effect = api_side_effect - res = gitea_merge_pr(pr_number=1, confirmation="MERGE PR 1", do="merge") + res = gitea_merge_pr( + pr_number=1, confirmation="MERGE PR 1", do="merge", + expected_head_sha=head_sha, + ) self.assertTrue(res["performed"]) self.assertEqual(res["cleanup_status"].get(123), "released") @@ -3191,7 +3357,12 @@ class TestIssueLocking(unittest.TestCase): """Test issue locking and PR gating constraints.""" def setUp(self): - self._env_patcher = patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True) + self._lock_dir = tempfile.TemporaryDirectory() + env = { + **ISSUE_WRITE_ENV, + "GITEA_ISSUE_LOCK_DIR": self._lock_dir.name, + } + self._env_patcher = patch.dict(os.environ, env, clear=True) self._env_patcher.start() self._dup_fetcher_patcher = patch( "mcp_server.issue_duplicate_context_fetcher", @@ -3202,15 +3373,21 @@ class TestIssueLocking(unittest.TestCase): def tearDown(self): self._dup_fetcher_patcher.stop() self._env_patcher.stop() - if os.path.exists(ISSUE_LOCK_FILE): - os.remove(ISSUE_LOCK_FILE) + self._lock_dir.cleanup() + + def _create_pr_env(self) -> dict: + return { + **CREATE_PR_ENV, + "GITEA_ISSUE_LOCK_DIR": self._lock_dir.name, + } @patch( "mcp_server.issue_lock_worktree.read_worktree_git_state", return_value=_clean_master_git_state_for_lock(), ) + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_lock_issue_success(self, _auth, _git_state): + def test_lock_issue_success(self, _auth, _api, _git_state): res = gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertTrue(res["success"]) self.assertEqual(res["work_lease"]["operation_type"], "author_issue_work") @@ -3220,9 +3397,8 @@ class TestIssueLocking(unittest.TestCase): self.assertIn("expires_at", res["work_lease"]) self.assertIn("last_heartbeat_at", res["work_lease"]) self.assertEqual(res["work_lease"]["claimant"]["profile"], "gitea-default") - self.assertTrue(os.path.exists(ISSUE_LOCK_FILE)) - with open(ISSUE_LOCK_FILE, encoding="utf-8") as f: - lock = json.load(f) + self.assertIn("lock_file_path", res) + lock = issue_lock_store.read_lock_file(res["lock_file_path"]) self.assertIn("worktree_path", lock) self.assertIn("work_lease", lock) @@ -3278,32 +3454,81 @@ class TestIssueLocking(unittest.TestCase): gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertIn("remote branch(es) already match issue pattern", str(ctx.exception)) - def test_lock_issue_blocks_active_same_operation_lease(self): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump({ + @patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value=_clean_master_git_state_for_lock(), + ) + @patch("mcp_server.api_get_all") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_lock_issue_adopts_exact_own_branch(self, _auth, mock_api, _git_state): + branch = "feat/issue-196-mutations" + self.mock_dup_fetcher.return_value = ([], [branch], {"status": "not_claimed"}) + mock_api.return_value = [{"name": branch, "commit": {"id": "abc123"}}] + res = gitea_lock_issue(issue_number=196, branch_name=branch, remote="prgs") + self.assertTrue(res["success"]) + self.assertIn("adoption", res) + self.assertEqual(res["adoption"]["branch_head_commit"], "abc123") + + @patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value=_clean_master_git_state_for_lock(), + ) + @patch("mcp_server.api_get_all", return_value=[]) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_lock_issue_blocks_active_same_operation_lease(self, _auth, _api, _git_state): + prgs_repo = mcp_server.REMOTES["prgs"]["repo"] + issue_lock_store.save_lock_file( + issue_lock_store.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo=prgs_repo, + issue_number=196, + ), + { "issue_number": 196, "branch_name": "feat/issue-196-other-work", + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": prgs_repo, "worktree_path": "/tmp/other-worktree", "work_lease": { "operation_type": "author_issue_work", "expires_at": "2999-01-01T00:00:00Z", }, - }, f) + }, + ) with self.assertRaises(RuntimeError) as ctx: gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertIn("already has an active author_issue_work lease", str(ctx.exception)) - def test_lock_issue_blocks_expired_same_operation_lease_for_recovery(self): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump({ + @patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value=_clean_master_git_state_for_lock(), + ) + @patch("mcp_server.api_get_all", return_value=[]) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_lock_issue_blocks_expired_same_operation_lease_for_recovery(self, _auth, _api, _git_state): + prgs_repo = mcp_server.REMOTES["prgs"]["repo"] + issue_lock_store.save_lock_file( + issue_lock_store.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo=prgs_repo, + issue_number=196, + ), + { "issue_number": 196, "branch_name": "feat/issue-196-other-work", + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": prgs_repo, "worktree_path": "/tmp/other-worktree", "work_lease": { "operation_type": "author_issue_work", "expires_at": "2000-01-01T00:00:00Z", }, - }, f) + }, + ) with self.assertRaises(RuntimeError) as ctx: gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertIn("Recovery review is required before takeover", str(ctx.exception)) @@ -3370,9 +3595,7 @@ class TestIssueLocking(unittest.TestCase): return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_missing_lock_fails(self, _auth, _role): - if os.path.exists(ISSUE_LOCK_FILE): - os.remove(ISSUE_LOCK_FILE) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + with patch.dict(os.environ, self._create_pr_env(), clear=True): with self.assertRaises(RuntimeError) as ctx: gitea_create_pr(title="feat: X Closes #196", head="feat/issue-196-mutations", remote="prgs") self.assertIn("Issue lock is missing", str(ctx.exception)) @@ -3381,37 +3604,64 @@ class TestIssueLocking(unittest.TestCase): return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_branch_mismatch_fails(self, _auth, _role): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(_sample_issue_lock( - issue_number=196, branch_name="feat/issue-196-mutations"), f) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + worktree = os.path.realpath(os.getcwd()) + _bind_test_lock( + issue_number=196, + branch_name="feat/issue-196-mutations", + remote="prgs", + worktree_path=worktree, + ) + with patch.dict(os.environ, self._create_pr_env(), clear=True): with self.assertRaises(ValueError) as ctx: - gitea_create_pr(title="feat: X Closes #196", head="feat/issue-196-different", remote="prgs") + gitea_create_pr( + title="feat: X Closes #196", + head="feat/issue-196-different", + remote="prgs", + worktree_path=worktree, + ) self.assertIn("does not match locked branch", str(ctx.exception)) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_forbidden_terms_fails(self, _auth, _role): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(_sample_issue_lock( - issue_number=196, branch_name="feat/issue-196-mutations"), f) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + worktree = os.path.realpath(os.getcwd()) + _bind_test_lock( + issue_number=196, + branch_name="feat/issue-196-mutations", + remote="prgs", + worktree_path=worktree, + ) + with patch.dict(os.environ, self._create_pr_env(), clear=True): for term in ("equivalent to #196", "related to #196", "same as #196"): with self.assertRaises(ValueError) as ctx: - gitea_create_pr(title=f"feat: X {term}", head="feat/issue-196-mutations", remote="prgs") + gitea_create_pr( + title=f"feat: X {term}", + head="feat/issue-196-mutations", + remote="prgs", + worktree_path=worktree, + ) self.assertIn("contains forbidden term", str(ctx.exception)) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_missing_closes_ref_fails(self, _auth, _role): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(_sample_issue_lock( - issue_number=196, branch_name="feat/issue-196-mutations"), f) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + worktree = os.path.realpath(os.getcwd()) + _bind_test_lock( + issue_number=196, + branch_name="feat/issue-196-mutations", + remote="prgs", + worktree_path=worktree, + ) + with patch.dict(os.environ, self._create_pr_env(), clear=True): with self.assertRaises(ValueError) as ctx: - gitea_create_pr(title="feat: X refs #196", head="feat/issue-196-mutations", remote="prgs") + gitea_create_pr( + title="feat: X refs #196", + head="feat/issue-196-mutations", + remote="prgs", + worktree_path=worktree, + ) self.assertIn("must contain 'Closes #196' or 'Fixes #196' exactly", str(ctx.exception)) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", @@ -3419,13 +3669,13 @@ class TestIssueLocking(unittest.TestCase): @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_worktree_mismatch_fails(self, _auth, _role): scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-pr") - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(_sample_issue_lock( - issue_number=249, - branch_name="feat/issue-249-issue-lock-scratch-worktree", - worktree_path=scratch, - ), f) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + _bind_test_lock( + issue_number=249, + branch_name="feat/issue-249-issue-lock-scratch-worktree", + worktree_path=scratch, + remote="prgs", + ) + with patch.dict(os.environ, self._create_pr_env(), clear=True): with self.assertRaises(ValueError) as ctx: gitea_create_pr( title="feat: lock scratch worktree Closes #249", @@ -3439,22 +3689,35 @@ class TestIssueLocking(unittest.TestCase): return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_pr_manual_lock_seed_blocked(self, _auth, _role): - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump( + worktree = os.path.realpath(os.getcwd()) + with tempfile.TemporaryDirectory() as lock_dir: + env = {**self._create_pr_env(), "GITEA_ISSUE_LOCK_DIR": lock_dir} + with patch.dict(os.environ, env, clear=True): + issue_lock_store.save_lock_file( + issue_lock_store.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo=mcp_server.REMOTES["prgs"]["repo"], + issue_number=447, + lock_dir=lock_dir, + ), _sample_issue_lock( issue_number=447, branch_name="feat/issue-447-lock-provenance", + remote="prgs", + org="Scaled-Tech-Consulting", + repo=mcp_server.REMOTES["prgs"]["repo"], + worktree_path=worktree, lock_provenance=None, ), - f, - ) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): - with self.assertRaises(RuntimeError) as ctx: - gitea_create_pr( - title="feat: lock provenance Closes #447", - head="feat/issue-447-lock-provenance", - remote="prgs", ) + with self.assertRaises(RuntimeError) as ctx: + gitea_create_pr( + title="feat: lock provenance Closes #447", + head="feat/issue-447-lock-provenance", + remote="prgs", + worktree_path=worktree, + ) self.assertIn("lock provenance", str(ctx.exception).lower()) @patch("mcp_server.api_request") @@ -3464,13 +3727,13 @@ class TestIssueLocking(unittest.TestCase): def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api): scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-e2e") mock_api.return_value = {"number": 250, "html_url": "https://example/pr/250"} - with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: - json.dump(_sample_issue_lock( - issue_number=249, - branch_name="feat/issue-249-issue-lock-scratch-worktree", - worktree_path=scratch, - ), f) - with patch.dict(os.environ, CREATE_PR_ENV, clear=True): + _bind_test_lock( + issue_number=249, + branch_name="feat/issue-249-issue-lock-scratch-worktree", + worktree_path=scratch, + remote="prgs", + ) + with patch.dict(os.environ, self._create_pr_env(), clear=True): res = gitea_create_pr( title="feat: issue-lock scratch worktree Closes #249", head="feat/issue-249-issue-lock-scratch-worktree", diff --git a/tests/test_merge_approval_gate.py b/tests/test_merge_approval_gate.py new file mode 100644 index 0000000..94f11ac --- /dev/null +++ b/tests/test_merge_approval_gate.py @@ -0,0 +1,59 @@ +"""Hermetic tests for merge approval head pinning (#471).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from merge_approval_gate import assess_merge_approval_head # noqa: E402 + +HEAD_OLD = "8b61c4b41f1b49b271ed3b99657431cf06eeda3e" +HEAD_NEW = "3e4b721d60e97147ba0704773cf57cd0d42cbe31" + + +class TestMergeApprovalGate(unittest.TestCase): + def test_fresh_approval_at_current_head(self): + result = assess_merge_approval_head( + current_head_sha=HEAD_NEW, + latest_by_reviewer={ + "reviewer1": { + "verdict": "APPROVED", + "dismissed": False, + "reviewed_head_sha": HEAD_NEW, + "submitted_at": "2026-07-06T12:00:00Z", + } + }, + ) + self.assertTrue(result["approval_at_current_head"]) + self.assertIsNone(result["stale_approval_block_reason"]) + + def test_stale_approval_after_rebase(self): + result = assess_merge_approval_head( + current_head_sha=HEAD_NEW, + latest_by_reviewer={ + "reviewer1": { + "verdict": "APPROVED", + "dismissed": False, + "reviewed_head_sha": HEAD_OLD, + "submitted_at": "2026-07-06T10:00:00Z", + } + }, + ) + self.assertFalse(result["approval_at_current_head"]) + self.assertEqual(result["latest_approved_head_sha"], HEAD_OLD) + self.assertIn("stale approval", result["stale_approval_block_reason"]) + self.assertIn(HEAD_OLD, result["stale_approval_block_reason"]) + self.assertIn(HEAD_NEW, result["stale_approval_block_reason"]) + self.assertIn("re-review PR at current head", result["stale_approval_block_reason"]) + + def test_no_approval_entries(self): + result = assess_merge_approval_head( + current_head_sha=HEAD_NEW, + latest_by_reviewer={}, + ) + self.assertFalse(result["approval_at_current_head"]) + self.assertIsNone(result["latest_approved_head_sha"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_permission_reports.py b/tests/test_permission_reports.py index b317a00..279dc2b 100644 --- a/tests/test_permission_reports.py +++ b/tests/test_permission_reports.py @@ -230,8 +230,9 @@ class TestEligibilityDenialReport(PermissionReportBase): return PR_PAYLOAD mock_api.side_effect = fake_api mcp_server.init_review_decision_lock("prgs", "review_pr") - mcp_server.gitea_load_review_workflow() - mcp_server.gitea_mark_final_review_decision(42, "approve", remote="prgs") + from tests.test_mcp_server import _seed_ready_review_decision + + _seed_ready_review_decision(42, "approve", remote="prgs") with patch.dict(os.environ, self._env("author-profile")): res = mcp_server.gitea_submit_pr_review( pr_number=42, action="approve", body="lgtm", remote="prgs", @@ -273,8 +274,9 @@ class TestReviewCommentPathUsesCanonicalOp(PermissionReportBase): return PR_PAYLOAD mock_api.side_effect = fake_api mcp_server.init_review_decision_lock("prgs", "review_pr") - mcp_server.gitea_load_review_workflow() - mcp_server.gitea_mark_final_review_decision(42, "comment", remote="prgs") + from tests.test_mcp_server import _seed_ready_review_decision + + _seed_ready_review_decision(42, "comment", remote="prgs") with patch.dict(os.environ, self._env("author-profile")): res = mcp_server.gitea_submit_pr_review( pr_number=42, action="comment", body="finding", remote="prgs", diff --git a/tests/test_pr_queue_inventory.py b/tests/test_pr_queue_inventory.py index 1e5a3c5..a61cc3f 100644 --- a/tests/test_pr_queue_inventory.py +++ b/tests/test_pr_queue_inventory.py @@ -128,18 +128,29 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } + + from mcp_server import init_review_decision_lock + from tests.test_mcp_server import ( + FULL_HEAD_SHA, + _install_owned_reviewer_lease, + _seed_ready_review_decision, + ) + import reviewer_pr_lease + + head_sha = FULL_HEAD_SHA mock_fetch.return_value = _final_page_fetch([ - {"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "other_user"}} + {"number": 1, "title": "PR 1", "state": "open", + "head": {"ref": "branch1", "sha": head_sha}, + "base": {"ref": "master"}, "mergeable": True, + "user": {"login": "other_user"}} ]) - # mock_api: inventory whoami, eligibility whoami, eligibility PR, - # POST review (#244: state + visible-verdict GET reviews). mock_api.side_effect = [ {"login": "reviewer1"}, {"login": "reviewer1"}, { "user": {"login": "other_user"}, "state": "open", - "head": {"sha": "abc1"}, + "head": {"sha": head_sha}, "mergeable": True, }, {"id": 100, "state": "APPROVED"}, @@ -148,22 +159,19 @@ class TestPRQueueInventory(unittest.TestCase): "id": 100, "user": {"login": "reviewer1"}, "state": "APPROVED", - "commit_id": "abc1", + "commit_id": head_sha, "submitted_at": "2026-07-06T10:00:00Z", "dismissed": False, } ], ] - from mcp_server import init_review_decision_lock, gitea_mark_final_review_decision - from tests.test_mcp_server import _install_owned_reviewer_lease - import reviewer_pr_lease - - with patch("mcp_server._authenticated_username", return_value="reviewer1"): + with patch("mcp_server._authenticated_username", return_value="reviewer1"), \ + patch("mcp_server._list_pr_lease_comments", return_value=[]): init_review_decision_lock("prgs", "review_pr") - gitea_mark_final_review_decision(1, "approve", remote="prgs") + _seed_ready_review_decision(1, "approve", sha=head_sha, remote="prgs") lease_patch = _install_owned_reviewer_lease( - 1, head_sha="abc1", session_id="inventory-review-lease", + 1, head_sha=head_sha, session_id="inventory-review-lease", ) lease_patch.start() self.addCleanup(lease_patch.stop) 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 diff --git a/tests/test_review_feedback.py b/tests/test_review_feedback.py index 01fff0c..72cbe61 100644 --- a/tests/test_review_feedback.py +++ b/tests/test_review_feedback.py @@ -115,6 +115,22 @@ class TestPRReviewFeedbackDiscovery(unittest.TestCase): result["latest_review_state_by_reviewer"], {"reviewer1": "APPROVED"}) self.assertFalse(result["has_blocking_change_requests"]) self.assertTrue(result["approval_visible"]) + self.assertTrue(result["approval_at_current_head"]) + + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + @patch("mcp_server.get_profile") + def test_stale_approval_not_at_current_head(self, mock_get_profile, _auth, mock_api): + mock_get_profile.return_value = self._profile() + mock_api.side_effect = [ + _pr_details(head_sha="newhead3"), + [_review("reviewer1", "APPROVED", commit_id="oldhead1")], + ] + result = gitea_get_pr_review_feedback(pr_number=5, remote="prgs") + self.assertTrue(result["approval_visible"]) + self.assertFalse(result["approval_at_current_head"]) + self.assertEqual(result["latest_approved_head_sha"], "oldhead1") + self.assertIn("stale approval", result["stale_approval_block_reason"]) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) diff --git a/tests/test_review_final_report_schema.py b/tests/test_review_final_report_schema.py index fbcb5a2..3b2211e 100644 --- a/tests/test_review_final_report_schema.py +++ b/tests/test_review_final_report_schema.py @@ -19,7 +19,11 @@ def _minimal_review_report(**overrides): "- Issue/PR: #182 / PR #203", "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", "- Files changed: review_proofs.py", - "- Validation: pytest tests/test_review_proofs.py -q in branches/review-203", + "- Validation: pass: pytest tests/test_review_proofs.py -q in branches/review-203", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", "- Mutations: review only", "- File edits by reviewer: none", "- Worktree/index mutations: none", @@ -80,7 +84,7 @@ class TestReviewFinalReportSchema(unittest.TestCase): def test_reviewed_head_without_validation_blocks(self): report = _minimal_review_report().replace( - "- Validation: pytest tests/test_review_proofs.py -q in branches/review-203", + "- Validation: pass: pytest tests/test_review_proofs.py -q in branches/review-203", "- Validation: not run", ) report += "\n- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9" diff --git a/tests/test_review_workflow_load.py b/tests/test_review_workflow_load.py index b611422..e59bf1a 100644 --- a/tests/test_review_workflow_load.py +++ b/tests/test_review_workflow_load.py @@ -130,7 +130,9 @@ class TestReviewWorkflowLoadGates(unittest.TestCase): self.assertTrue(blockers) def test_dry_run_allowed_without_load(self): - with patch("mcp_server.gitea_check_pr_eligibility") as elig: + with patch("mcp_server.get_auth_header", return_value="Basic dGVzdA=="), \ + patch("mcp_server._list_pr_lease_comments", return_value=[]), \ + patch("mcp_server.gitea_check_pr_eligibility") as elig: elig.return_value = { "eligible": True, "authenticated_user": "rev", diff --git a/tests/test_terminal_review_hard_stop.py b/tests/test_terminal_review_hard_stop.py index bd5109b..4c65130 100644 --- a/tests/test_terminal_review_hard_stop.py +++ b/tests/test_terminal_review_hard_stop.py @@ -12,6 +12,8 @@ from unittest.mock import patch import mcp_server +HEAD_SHA = "a" * 40 + def _lock(mutations=None, correction=False): return { @@ -127,10 +129,20 @@ def _feedback(blocking, stale=False, success=True): "success": success, "has_blocking_change_requests": blocking, "review_feedback_stale": stale, - "current_head_sha": "abc123", + "current_head_sha": HEAD_SHA, } +def _mark(action, pr_number=6, **kwargs): + kwargs.setdefault("expected_head_sha", HEAD_SHA) + no_lease_block = {"block": False, "reasons": [], "mutation_allowed": True} + with patch("mcp_server._list_pr_lease_comments", return_value=[]), \ + patch("mcp_server._pr_work_lease_reviewer_block", return_value=no_lease_block): + return mcp_server.gitea_mark_final_review_decision( + pr_number=pr_number, action=action, remote="prgs", **kwargs + ) + + class TestDuplicateRequestChangesSuppression(unittest.TestCase): def tearDown(self): mcp_server._save_review_decision_lock(None) @@ -139,8 +151,7 @@ class TestDuplicateRequestChangesSuppression(unittest.TestCase): _seed() with patch.object(mcp_server, "gitea_get_pr_review_feedback", return_value=_feedback(blocking=True, stale=False)): - result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="request_changes", remote="prgs") + result = _mark("request_changes") self.assertFalse(result["marked_ready"]) self.assertTrue( any("duplicate" in r for r in result["reasons"]), @@ -150,16 +161,14 @@ class TestDuplicateRequestChangesSuppression(unittest.TestCase): _seed() with patch.object(mcp_server, "gitea_get_pr_review_feedback", return_value=_feedback(blocking=True, stale=True)): - result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="request_changes", remote="prgs") + result = _mark("request_changes") self.assertTrue(result["marked_ready"], result.get("reasons")) def test_request_changes_allowed_when_no_blocker(self): _seed() with patch.object(mcp_server, "gitea_get_pr_review_feedback", return_value=_feedback(blocking=False)): - result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="request_changes", remote="prgs") + result = _mark("request_changes") self.assertTrue(result["marked_ready"], result.get("reasons")) def test_request_changes_fails_closed_when_feedback_unavailable(self): @@ -167,8 +176,7 @@ class TestDuplicateRequestChangesSuppression(unittest.TestCase): with patch.object(mcp_server, "gitea_get_pr_review_feedback", return_value=_feedback(blocking=False, success=False)): - result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="request_changes", remote="prgs") + result = _mark("request_changes") self.assertFalse(result["marked_ready"]) self.assertTrue( any("could not verify" in r for r in result["reasons"]), @@ -178,8 +186,7 @@ class TestDuplicateRequestChangesSuppression(unittest.TestCase): _seed() with patch.object(mcp_server, "gitea_get_pr_review_feedback", side_effect=AssertionError("must not be called")): - result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="approve", remote="prgs") + result = _mark("approve") self.assertTrue(result["marked_ready"], result.get("reasons")) diff --git a/tests/test_validation_status_vocabulary.py b/tests/test_validation_status_vocabulary.py new file mode 100644 index 0000000..f01f3e9 --- /dev/null +++ b/tests/test_validation_status_vocabulary.py @@ -0,0 +1,198 @@ +"""Tests for validation status vocabulary (#406).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from final_report_validator import assess_final_report_validator # noqa: E402 +from validation_status_vocabulary import ( # noqa: E402 + STATUS_BASELINE_EQUIVALENT, + STATUS_FAILED, + STATUS_MERGE_SIM_RESOLVED, + STATUS_PASSED, + STATUS_TRANSIENT_PASS, + assess_validation_status_vocabulary, +) + + +def _handoff(**extra): + fields = { + "Task": "review PR #386", + "Validation status": STATUS_PASSED, + "Raw PR-head validation result": "passed", + "Merge simulation result": "not run", + "Baseline worktree used": "none", + } + fields.update(extra) + lines = ["## Controller Handoff", ""] + lines.extend(f"- {key}: {value}" for key, value in fields.items()) + return "\n".join(lines) + + +class TestValidationStatusVocabulary(unittest.TestCase): + def test_raw_head_pass_status(self): + report = _handoff() + result = assess_validation_status_vocabulary(report) + self.assertFalse(result["block"]) + self.assertEqual(result["status_claimed"], STATUS_PASSED) + + def test_raw_head_failure_with_baseline_match(self): + report = _handoff( + **{ + "Validation status": STATUS_BASELINE_EQUIVALENT, + "Raw PR-head validation result": "failed", + "Baseline worktree used": "branches/baseline-master-pr386", + "Baseline target SHA": "a" * 40, + "Baseline failures": "test_foo failed", + "PR failures": "test_foo failed", + "Failure signatures match": "true", + } + ) + result = assess_validation_status_vocabulary(report) + self.assertFalse(result["block"]) + self.assertTrue(result["baseline_proof_complete"]) + + def test_baseline_equivalent_without_baseline_proof_blocked(self): + report = _handoff( + **{ + "Validation status": STATUS_BASELINE_EQUIVALENT, + "Raw PR-head validation result": "failed", + } + ) + result = assess_validation_status_vocabulary(report) + self.assertTrue(result["block"]) + self.assertIn("baseline-equivalent", result["reasons"][0]) + + def test_merge_simulation_resolution_passes(self): + report = "\n".join([ + _handoff( + **{ + "Validation status": STATUS_MERGE_SIM_RESOLVED, + "Raw PR-head validation result": "failed", + "Merge simulation result": "passed", + } + ), + "Worktree/index mutations: merge simulation in branches/review-pr386", + "Worktree path: branches/review-pr386", + "Pre-simulation clean status: clean", + "Merge result: clean merge", + "Abort command: git merge --abort", + "Post-abort clean status: clean", + ]) + command_log = [ + {"command": "git merge --no-commit prgs/master"}, + {"command": "git merge --abort"}, + ] + result = assess_validation_status_vocabulary( + report, command_log=command_log + ) + self.assertFalse(result["block"]) + self.assertTrue(result["merge_simulation_passed"]) + + def test_merge_simulation_failure_stays_failed(self): + report = _handoff( + **{ + "Validation status": STATUS_FAILED, + "Raw PR-head validation result": "failed", + "Merge simulation result": "failed", + } + ) + result = assess_validation_status_vocabulary(report) + self.assertFalse(result["block"]) + + def test_failed_status_with_passing_merge_sim_blocked(self): + report = "\n".join([ + _handoff( + **{ + "Validation status": STATUS_FAILED, + "Raw PR-head validation result": "failed", + "Merge simulation result": "passed", + } + ), + "Worktree/index mutations: merge simulation", + "Worktree path: branches/review-pr386", + "Pre-simulation clean status: clean", + "Merge result: clean", + "Abort command: git merge --abort", + "Post-abort clean status: clean", + ]) + result = assess_validation_status_vocabulary( + report, + command_log=[{"command": "git merge --no-commit prgs/master"}], + ) + self.assertTrue(result["block"]) + + def test_transient_failure_then_pass(self): + report = _handoff( + **{ + "Validation status": STATUS_TRANSIENT_PASS, + "Raw PR-head validation result": "passed", + "Transient validation failure history": ( + "first run failed with infra flake; rerun passed" + ), + } + ) + result = assess_validation_status_vocabulary(report) + self.assertFalse(result["block"]) + + def test_transient_pass_without_history_blocked(self): + report = _handoff( + **{ + "Validation status": STATUS_TRANSIENT_PASS, + "Raw PR-head validation result": "passed", + } + ) + result = assess_validation_status_vocabulary(report) + self.assertTrue(result["block"]) + + def test_bare_passed_after_raw_failure_blocked(self): + report = _handoff( + **{ + "Validation status": STATUS_PASSED, + "Raw PR-head validation result": "failed", + } + ) + result = assess_validation_status_vocabulary(report) + self.assertTrue(result["block"]) + + def test_wrong_baseline_label_when_merge_sim_used_blocked(self): + report = "\n".join([ + _handoff( + **{ + "Validation status": STATUS_BASELINE_EQUIVALENT, + "Raw PR-head validation result": "failed", + "Merge simulation result": "passed", + } + ), + "Worktree/index mutations: merge simulation", + "Worktree path: branches/review-pr386", + "Pre-simulation clean status: clean", + "Merge result: clean", + "Abort command: git merge --abort", + "Post-abort clean status: clean", + ]) + result = assess_validation_status_vocabulary( + report, + command_log=[{"command": "git merge --no-commit prgs/master"}], + ) + self.assertTrue(result["block"]) + joined = " ".join(result["reasons"]).lower() + self.assertTrue( + "misleading" in joined or "baseline-equivalent" in joined + ) + + def test_final_report_validator_integration_blocks_misleading_label(self): + report = _handoff( + **{ + "Validation status": STATUS_BASELINE_EQUIVALENT, + "Raw PR-head validation result": "failed", + } + ) + result = assess_final_report_validator(report, "review_pr") + blocked_ids = {f["rule_id"] for f in result["findings"]} + self.assertIn("reviewer.validation_status_vocabulary", blocked_ids) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_workspace_guard_alignment.py b/tests/test_workspace_guard_alignment.py new file mode 100644 index 0000000..ddcb67f --- /dev/null +++ b/tests/test_workspace_guard_alignment.py @@ -0,0 +1,153 @@ +"""Tests for runtime_context / mutation-guard workspace alignment (#460).""" + +from __future__ import annotations + +import os +import sys +import unittest +from pathlib import Path +from unittest import mock +from unittest.mock import MagicMock + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import author_mutation_worktree as amw # noqa: E402 +import gitea_mcp_server as srv # noqa: E402 + +CONTROL_ROOT = str(Path(__file__).resolve().parents[3]) +BRANCHES_WORKTREE = str(Path(__file__).resolve().parents[1]) +MCP_PROCESS_ROOT = BRANCHES_WORKTREE + + +class TestCanonicalRepoRoot(unittest.TestCase): + @mock.patch("subprocess.run") + def test_resolves_main_repo_from_branches_worktree(self, mock_run): + mock_run.return_value = MagicMock( + returncode=0, + stdout=f"{CONTROL_ROOT}/.git\n", + ) + root = amw.resolve_canonical_repo_root(BRANCHES_WORKTREE, MCP_PROCESS_ROOT) + self.assertEqual(root, CONTROL_ROOT) + + def test_falls_back_when_git_unavailable(self): + root = amw.resolve_canonical_repo_root("/missing/path", MCP_PROCESS_ROOT) + self.assertEqual(root, os.path.realpath(MCP_PROCESS_ROOT)) + + +class TestWorkspaceRepoMembership(unittest.TestCase): + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + @mock.patch("subprocess.run") + def test_valid_branches_worktree_accepted(self, mock_run, *_exists): + mock_run.return_value = MagicMock( + returncode=0, + stdout=f"{CONTROL_ROOT}/.git\n", + ) + result = amw.assess_workspace_repo_membership( + workspace_path=BRANCHES_WORKTREE, + canonical_repo_root=CONTROL_ROOT, + ) + self.assertTrue(result["proven"]) + self.assertFalse(result["block"]) + + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + @mock.patch("subprocess.run") + def test_wrong_repo_rejected(self, mock_run, *_exists): + mock_run.return_value = MagicMock( + returncode=0, + stdout="/other/repo/.git\n", + ) + result = amw.assess_workspace_repo_membership( + workspace_path=BRANCHES_WORKTREE, + canonical_repo_root=CONTROL_ROOT, + ) + self.assertTrue(result["block"]) + self.assertIn("does not belong", result["reasons"][0]) + + @mock.patch("os.path.exists", return_value=False) + def test_missing_worktree_rejected(self, *_exists): + result = amw.assess_workspace_repo_membership( + workspace_path=f"{CONTROL_ROOT}/branches/missing-worktree", + canonical_repo_root=CONTROL_ROOT, + ) + self.assertTrue(result["block"]) + self.assertIn("does not exist", result["reasons"][0]) + + +class TestRuntimeContextGuardAlignment(unittest.TestCase): + def setUp(self): + srv._preflight_whoami_called = True + srv._preflight_capability_called = True + srv._preflight_resolved_role = "author" + self._orig_in_test = srv._preflight_in_test_mode + srv._preflight_in_test_mode = lambda: False + self._env_patch = mock.patch.dict( + os.environ, + {}, + clear=False, + ) + self._env_patch.start() + os.environ.pop("GITEA_ACTIVE_WORKTREE", None) + os.environ.pop("GITEA_AUTHOR_WORKTREE", None) + + def tearDown(self): + srv._preflight_in_test_mode = self._orig_in_test + self._env_patch.stop() + + def test_runtime_context_and_guard_share_resolved_workspace(self): + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + ctx = srv._resolve_author_mutation_context(BRANCHES_WORKTREE) + status = srv.assess_preflight_status(worktree_path=BRANCHES_WORKTREE) + self.assertEqual(ctx["workspace_path"], os.path.realpath(BRANCHES_WORKTREE)) + self.assertEqual(ctx["canonical_repo_root"], CONTROL_ROOT) + self.assertFalse(ctx["roots_aligned"]) + self.assertEqual( + status["preflight_workspace"]["active_task_workspace_root"], + os.path.realpath(BRANCHES_WORKTREE), + ) + self.assertEqual( + status["preflight_workspace"]["canonical_repository_root"], + CONTROL_ROOT, + ) + self.assertIn("workspace_root_mismatch", status["preflight_workspace"]) + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_declared_branches_worktree_passes_when_mcp_root_differs( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock( + returncode=0, + stdout=f"{CONTROL_ROOT}/.git\n", + ) + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch.dict("os.environ", {"GITEA_TEST_PORCELAIN": ""}, clear=False): + srv.verify_preflight_purity(worktree_path=BRANCHES_WORKTREE) + + def test_stable_checkout_still_rejected(self): + with mock.patch.object(srv, "PROJECT_ROOT", CONTROL_ROOT): + with mock.patch.dict("os.environ", {"GITEA_TEST_PORCELAIN": ""}, clear=False): + with self.assertRaises(RuntimeError) as ctx: + srv.verify_preflight_purity() + self.assertIn("stable control checkout", str(ctx.exception)) + + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + @mock.patch("subprocess.run") + def test_non_branches_worktree_rejected(self, mock_run, *_exists): + outside = "/tmp/outside-repo-checkout" + mock_run.return_value = MagicMock( + returncode=0, + stdout=f"{CONTROL_ROOT}/.git\n", + ) + with mock.patch.object(srv, "PROJECT_ROOT", CONTROL_ROOT): + with mock.patch.dict("os.environ", {"GITEA_TEST_PORCELAIN": ""}, clear=False): + with self.assertRaises(RuntimeError) as ctx: + srv.verify_preflight_purity(worktree_path=outside) + self.assertIn("not under", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_worktrees.py b/tests/test_worktrees.py index 45f1ac5..9ed3285 100644 --- a/tests/test_worktrees.py +++ b/tests/test_worktrees.py @@ -20,33 +20,50 @@ def run(script, *args): branch = arg break - lock_file = Path("/tmp/gitea_issue_lock.json") - created_lock = False + lock_dir_ctx = None + extra_env = os.environ.copy() if script == "worktree-start" and branch: import re - import json + import tempfile + import issue_lock_store + m = re.search(r"issue-(\d+)", branch) if not m: m = re.search(r"pr-(\d+)", branch) issue_num = int(m.group(1)) if m else 999 - lock_file.write_text(json.dumps({ + lock_dir_ctx = tempfile.TemporaryDirectory() + extra_env["GITEA_ISSUE_LOCK_DIR"] = lock_dir_ctx.name + record = { "issue_number": issue_num, "branch_name": branch, "remote": "prgs", "org": "Scaled-Tech-Consulting", - "repo": "Gitea-Tools" - }), encoding="utf-8") - created_lock = True + "repo": "Gitea-Tools", + "worktree_path": "/tmp/test-worktree", + "work_lease": { + "operation_type": "author_issue_work", + "expires_at": "2999-01-01T00:00:00Z", + }, + } + path = issue_lock_store.lock_file_path( + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + issue_number=issue_num, + lock_dir=lock_dir_ctx.name, + ) + issue_lock_store.save_lock_file(path, record) try: proc = subprocess.run( ["bash", str(SCRIPTS / script), *args], capture_output=True, text=True, cwd=str(REPO), + env=extra_env, ) return proc.returncode, proc.stdout, proc.stderr finally: - if created_lock and lock_file.exists(): - lock_file.unlink() + if lock_dir_ctx is not None: + lock_dir_ctx.cleanup() class TestWorktreeStart(unittest.TestCase): diff --git a/validation_status_vocabulary.py b/validation_status_vocabulary.py new file mode 100644 index 0000000..6f4e91a --- /dev/null +++ b/validation_status_vocabulary.py @@ -0,0 +1,205 @@ +"""Precise validation-status vocabulary for reviewer final reports (#406).""" + +from __future__ import annotations + +import re +from typing import Any + +from reviewer_merge_simulation import assess_merge_simulation_report + +STATUS_PASSED = "passed" +STATUS_FAILED = "failed" +STATUS_BASELINE_EQUIVALENT = "baseline-equivalent failure accepted" +STATUS_MERGE_SIM_RESOLVED = "raw-head failure resolved by merge simulation" +STATUS_TRANSIENT_PASS = "passed after transient failure investigation" + +ALLOWED_VALIDATION_STATUSES = frozenset({ + STATUS_PASSED, + STATUS_FAILED, + STATUS_BASELINE_EQUIVALENT, + STATUS_MERGE_SIM_RESOLVED, + STATUS_TRANSIENT_PASS, +}) + +_STATUS_FIELD_RE = re.compile( + r"^\s*[-*]?\s*(?:validation status|pr-head validation status|" + r"official validation status)\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_RAW_HEAD_RESULT_RE = re.compile( + r"^\s*[-*]?\s*raw pr-head validation result\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_MERGE_SIM_RESULT_RE = re.compile( + r"^\s*[-*]?\s*merge simulation result\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_BASELINE_WORKTREE_USED_RE = re.compile( + r"^\s*[-*]?\s*baseline (?:validation )?worktree(?: used)?\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_BASELINE_TARGET_SHA_RE = re.compile( + r"^\s*[-*]?\s*baseline target sha\s*:\s*([0-9a-f]{7,40})\s*$", + re.IGNORECASE | re.MULTILINE, +) +_FAILURE_SIGNATURE_RE = re.compile( + r"failure signatures match\s*:\s*(true|yes)", + re.IGNORECASE, +) +_BASELINE_FAILURES_RE = re.compile( + r"baseline failures\s*:", + re.IGNORECASE, +) +_TRANSIENT_HISTORY_RE = re.compile( + r"(?:transient validation failure|earlier validation failure|" + r"prior failure|failure history|failed then passed)", + re.IGNORECASE, +) +_FULL_SHA = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE) + + +def _first_match(pattern: re.Pattern[str], text: str) -> str: + match = pattern.search(text or "") + return (match.group(1).strip() if match else "") + + +def _normalize_status_label(raw: str) -> str: + text = (raw or "").strip().lower() + for status in ALLOWED_VALIDATION_STATUSES: + if text == status.lower(): + return status + return raw.strip() + + +def _baseline_proof_complete(text: str, baseline_proof: dict | None) -> bool: + proof = baseline_proof or {} + worktree = ( + (proof.get("worktree_path") or "").strip() + or _first_match(_BASELINE_WORKTREE_USED_RE, text) + ).lower() + if not worktree or worktree in {"none", "n/a", "not used", "not applicable"}: + return False + if "branches/" not in worktree and not worktree.startswith("branches/"): + return False + target_sha = (proof.get("baseline_target_sha") or "").strip() + if not target_sha: + target_sha = _first_match(_BASELINE_TARGET_SHA_RE, text) + if not _FULL_SHA.match(target_sha or ""): + return False + if proof.get("failure_signatures_match") is True: + return True + if _FAILURE_SIGNATURE_RE.search(text) and _BASELINE_FAILURES_RE.search(text): + return True + return False + + +def _merge_simulation_passed(text: str, command_log: list | None) -> bool: + merge_result = _first_match(_MERGE_SIM_RESULT_RE, text).lower() + if merge_result in {"passed", "pass", "clean", "succeeded", "success"}: + sim = assess_merge_simulation_report(text, command_log=command_log) + return sim.get("proven") and not sim.get("block") + if "pass" in merge_result and "fail" not in merge_result: + sim = assess_merge_simulation_report(text, command_log=command_log) + return sim.get("proven") and not sim.get("block") + return False + + +def _raw_head_failed(text: str) -> bool: + raw = _first_match(_RAW_HEAD_RESULT_RE, text).lower() + if raw in {"failed", "fail", "failure"}: + return True + if "fail" in raw and "pass" not in raw: + return True + return bool(re.search(r"\bfailed\b.*pr-head validation", text, re.IGNORECASE)) + + +def _raw_head_passed(text: str) -> bool: + raw = _first_match(_RAW_HEAD_RESULT_RE, text).lower() + return raw in {"passed", "pass", "success"} + + +def assess_validation_status_vocabulary( + report_text: str, + *, + command_log: list | None = None, + baseline_proof: dict | None = None, +) -> dict[str, Any]: + """Bind validation-status labels to the proof path that actually ran (#406).""" + text = report_text or "" + reasons: list[str] = [] + status_raw = _first_match(_STATUS_FIELD_RE, text) + status = _normalize_status_label(status_raw) if status_raw else "" + + if status_raw and status not in ALLOWED_VALIDATION_STATUSES: + reasons.append( + f"unknown validation status {status_raw!r}; use one of " + f"{sorted(ALLOWED_VALIDATION_STATUSES)}" + ) + + if status == STATUS_BASELINE_EQUIVALENT: + if not _baseline_proof_complete(text, baseline_proof): + reasons.append( + "baseline-equivalent failure accepted requires baseline " + "worktree path, baseline target SHA, and matching failure " + "signatures (#406)" + ) + + if status == STATUS_MERGE_SIM_RESOLVED: + if not _raw_head_failed(text): + reasons.append( + "raw-head failure resolved by merge simulation requires " + "raw PR-head validation result: failed (#406)" + ) + if not _merge_simulation_passed(text, command_log): + reasons.append( + "raw-head failure resolved by merge simulation requires " + "passing merge simulation with worktree/index mutation proof " + "(#317/#406)" + ) + + if status == STATUS_TRANSIENT_PASS: + if not _TRANSIENT_HISTORY_RE.search(text): + reasons.append( + "passed after transient failure investigation requires " + "documented earlier validation failure history (#396/#406)" + ) + + if status == STATUS_PASSED and _raw_head_failed(text): + reasons.append( + "validation status passed contradicts raw PR-head validation " + "failure; use a precise status (#406)" + ) + + if status == STATUS_BASELINE_EQUIVALENT and _merge_simulation_passed( + text, command_log + ) and not _baseline_proof_complete(text, baseline_proof): + reasons.append( + "baseline-equivalent failure accepted is misleading when only " + "merge simulation resolved the failure; use " + "'raw-head failure resolved by merge simulation' (#406)" + ) + + if status == STATUS_FAILED and _merge_simulation_passed(text, command_log): + reasons.append( + "validation status failed contradicts passing merge simulation; " + "report the precise resolution status (#406)" + ) + + block = bool(reasons) + return { + "block": block, + "proven": not block, + "status_claimed": status or None, + "raw_status_label": status_raw or None, + "raw_head_failed": _raw_head_failed(text), + "raw_head_passed": _raw_head_passed(text), + "merge_simulation_passed": _merge_simulation_passed(text, command_log), + "baseline_proof_complete": _baseline_proof_complete(text, baseline_proof), + "reasons": reasons, + "safe_next_action": ( + "use a validation status that matches the proof path executed " + "(baseline worktree, merge simulation, or transient history)" + if reasons + else "proceed" + ), + } \ No newline at end of file