diff --git a/docs/bootstrap-review-path.md b/docs/bootstrap-review-path.md new file mode 100644 index 0000000..33293d2 --- /dev/null +++ b/docs/bootstrap-review-path.md @@ -0,0 +1,190 @@ +# Bootstrap Review Path for Self-Hosted MCP Workflow Fixes (#557) + +## Purpose + +Gitea-Tools MCP workflow fixes can create a **bootstrap deadlock**: the live +MCP daemons still run the broken code from `master`, so canonical reviewer / +merger tools cannot complete the review that would land the fix. + +This document defines a **narrow, controller-authorized bootstrap path** so +such fixes can land without weakening normal review/merge gates. + +It is **not** a general bypass. Normal PRs must still use the full +reviewer → merger MCP workflow. + +## When this path applies + +All of the following must be true: + +1. The PR changes **Gitea-Tools MCP workflow / preflight / lease / gate** code + that the live daemon must load to review itself (self-hosted control plane). +2. Canonical review or merge tools **fail closed** because of that defect + (documented tool errors, not “inconvenience”). +3. A **controller** records an explicit bootstrap authorization on the PR or + linked issue (see below). +4. The PR source is clean, testable, and free of unrelated scope. + +Example (historical): PR #553 / Issue #546 (reviewer preflight capability/lease +deadlock). Live daemons on unpatched `master` could not complete canonical +review of the fix PR. + +## Durable authorization (required) + +**No manual remote merge, force-merge, or API merge of a bootstrap PR is +allowed** unless a controller has posted a durable authorization record on +Gitea. + +### Authorization record (issue or PR comment) + +The controller comment **must** include a machine-readable marker and the +fields below: + +```text +## BOOTSTRAP REVIEW AUTHORIZATION (#557) + +Status: APPROVED +PR: +Issue: +Controller: +Reason: live MCP runtime cannot canonically review this self-hosted workflow fix +Scope: narrow bootstrap only — does not weaken normal PR gates + +Validation evidence: +- git diff --check: clean +- targeted pytest: → pass +- full suite attribution (if non-zero): baseline compared to prgs/master + +Duplicate-PR audit: +Mutation ledger audit: +Contamination audit: + +Allowed verification actions used: +Forbidden actions NOT used: root checkout edits; raw curl mutation; direct +module import of gitea_mcp_server for mutations; in-memory gate restoration + +Post-land plan: +- restart MCP daemons for author/reviewer/merger/reconciler profiles +- re-verify with canonical tools (see Post-bootstrap steps) +``` + +Without this record, bootstrap merge is **forbidden**. Agents must stop with +`BLOCKED + DIAGNOSE` rather than improvise a merge. + +## Review gates and proof (still required) + +Bootstrap does **not** skip technical review. It only allows verification and +landing when the **live MCP mutation path** cannot complete the review. + +Before authorization, the controller (or a delegated read-only verifier in an +isolated worktree) must have: + +| Gate | Requirement | +|------|-------------| +| Diff hygiene | `git diff --check` clean on the PR tip vs `prgs/master` | +| Tests | Targeted tests for the fix pass; full suite either green or failures attributed to baseline `prgs/master` | +| Duplicate PR | Open-PR inventory shows no competing open PR for the same issue (or supersession is documented) | +| Mutation ledger | Any claimed mutations are ledger-consistent; no silent root edits | +| Contamination audit | PR/issue comments and leases classified as clean vs workflow-contaminated | +| Scope | Diff limited to the bootstrap issue; no drive-by refactors | + +### Contamination audit (comments / leases) + +Comments or leases created via **raw curl**, **direct Python import of MCP +modules**, or **token extraction** are **workflow-contaminated**. They must be +listed and must **not** be treated as canonical review proof. + +Only comments posted via **sanctioned MCP reviewer tools** (after the fix is +landed and daemons restarted, or during a clean path that did not bypass gates) +count as canonical review state. + +Historical example on PR #553: + +| Comment | Disposition | +|---------|-------------| +| #7247 test / raw API | contaminated | +| #7251 lease metadata / raw API | contaminated | +| #7252 lease metadata / direct import | contaminated | +| #7265 lease metadata / MCP reviewer tools | workflow-clean | + +## Allowed verification actions + +These may run **outside** the broken live mutation path to establish facts: + +- Read-only MCP tools (`gitea_view_pr`, `gitea_list_prs`, `gitea_whoami`, + `gitea_get_profile`, inventory, eligibility **reads**). +- Isolated `branches/` worktree checkout of the PR head (never root). +- `git fetch`, `git log`, `git diff`, `git merge-tree` (read-only analysis). +- Targeted `pytest` / `py_compile` inside that worktree. +- Controller posting of the bootstrap authorization comment via a healthy + MCP profile **if available**; if comment mutation is also deadlocked, a + **human operator** posts the authorization in the Gitea UI (still durable on + the thread). + +## Forbidden actions (always) + +Even under bootstrap: + +- Editing or committing in the **project root / control checkout**. +- Treating root as a worktree for implementation or review mutations. +- Raw `curl` / REST mutation with extracted tokens as a normal workflow. +- `import gitea_mcp_server` (or sibling modules) from a shell to call mutation + helpers and bypass preflight. +- In-memory restoration of capability / lease / decision state to “finish” + a blocked chain. +- Force-push, history rewrite, or deleting evidence comments. +- Weakening normal gates in code “temporarily” without a tracked issue/PR. +- Self-review or self-merge by the PR author identity. + +## Landing procedure (after APPROVED authorization) + +1. Confirm the authorization record is present and complete on the PR or issue. +2. Merge **only** with an independent merger identity when possible. + - Preferred: restart/replace the MCP merger daemon with a build that includes + the fix (or a one-shot process started from the PR worktree binary) so + `gitea_merge_pr` can run under normal gates. + - If the live merger daemon still cannot load the fix, a **human operator** + may merge via the Gitea UI **only** after the authorization record exists. +3. Never merge from contaminated proof alone. + +## Post-bootstrap steps + +### 1. Restart MCP daemons + +After the fix lands on `prgs/master`: + +1. Stop author / reviewer / merger / reconciler MCP server processes for this + repo (IDE MCP pool + any long-lived terminals). +2. Confirm no stale PID still serves the old code. +3. Restart each profile so it loads the updated `master` tree (or installed + package path). +4. Call `gitea_whoami` / `gitea_get_profile` on each namespace and record + profile name + identity. + +### 2. Re-verify with canonical tools + +Example pattern after PR #553-class fixes (lease release / #550-style follow-up): + +1. From a **reviewer** MCP session: acquire/release or inspect the relevant + lease with canonical tools only. +2. Confirm no direct-import or raw-API path is required. +3. Post a short controller or reconciler note: bootstrap complete; normal gates + restored. + +If verification still requires a bypass, open a new issue — do **not** extend +this bootstrap authorization silently. + +## Explicit non-goals + +- This path does **not** authorize skipping tests, duplicate audits, or + contamination audits. +- This path does **not** authorize root checkout implementation work. +- This path does **not** replace BLOCKED + DIAGNOSE when a non-bootstrap + failure occurs (#552). +- This path does **not** grant permanent “operator exception” culture. + +## Related + +- Root checkout policy: `docs/llm-workflow-runbooks.md` (Global LLM Worktree Rule, #475) +- BLOCKED + DIAGNOSE: issue #552 / skill workflows +- Reviewer lease / preflight deadlock class: issues #546, #548, #550 +- Durable session proofs across daemon pools: issue #559 diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 630bb69..0653dca 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -408,6 +408,28 @@ The guard blocks when the **control checkout** (repository root) is: `branches/...` directories are disposable role worktrees; the root checkout is the stable orchestration surface only. +## Bootstrap Review Path for self-hosted MCP fixes (#557) + +When a PR fixes Gitea-Tools MCP workflow code that the **live daemon** still +runs from broken `master`, canonical review can deadlock on itself. + +Do **not** improvise raw API, direct imports, root edits, or gate bypasses. + +Use the narrow controller-authorized path documented in: + +- [`docs/bootstrap-review-path.md`](bootstrap-review-path.md) + +Hard rules: + +- No merge without a durable `BOOTSTRAP REVIEW AUTHORIZATION (#557)` record on + the PR or linked issue. +- Validation, duplicate-PR, mutation-ledger, and contamination audits remain + mandatory. +- Root checkout stays read-only orchestration only; verification runs in + `branches/` worktrees. +- After land: restart MCP daemons and re-verify with canonical tools only. +- This never weakens normal reviewer/merger gates for ordinary PRs. + ## Shell Spawn Hard-Stop Rule Symptom: a shell tool call returns `exit_code: -1` with empty stdout/stderr. @@ -939,6 +961,21 @@ When posting a Canonical Thread Handoff after a binding blocker: - [`credential-isolation.md`](credential-isolation.md) — credential handling. - [`release-workflows.md`](release-workflows.md) — release/merge workflow. - [`../README.md`](../README.md) — canonical config, thin launchers, the menu. +- [`state-handoff-ledger.md`](state-handoff-ledger.md) — canonical state comments and next-action handoff (#494). + +## Canonical state handoff ledger (#494) + +Gitea comments and final reports must make continuation obvious without chat +history. See [`state-handoff-ledger.md`](state-handoff-ledger.md) for: + +- discussion → issue → PR → review → merge → reconcile lifecycle +- templates for discussion, issue, PR, and queue-controller state comments +- final-report requirements: Current status, Next actor, Next action, Next prompt +- queue-controller priority order and discussion ≥5-comment rule (urgent/trivial + exceptions) + +Helpers live in `state_handoff_ledger.py`; final-report enforcement is wired +through `assess_final_report_validator`. ## PR-only queue cleanup mode (#390) diff --git a/docs/mcp-menu.md b/docs/mcp-menu.md index 0b028f9..e028a40 100644 --- a/docs/mcp-menu.md +++ b/docs/mcp-menu.md @@ -41,7 +41,7 @@ The script must be executable (`chmod +x mcp-menu.sh`). It uses bash with |--------|-------------| | Project status / root checkout health | Shows cwd, branch, `git status --short --branch`, HEAD SHA, `prgs/master` SHA, and warnings when the root checkout is dirty or off `master`. | | Author workflow prompts | Ready-to-copy prompts for issue work, conflict-fix sessions, and root checkout recovery. | -| Reviewer workflow prompts | PR review prompt (review-only; no merge). | +| Reviewer workflow prompts | Standard PR review prompt, and a skip-already-reviewed-stale-`REQUEST_CHANGES` prompt that hands off to the author without a duplicate terminal mutation (review-only; no merge). | | Merger workflow prompts | PR merge prompt (merge gates and explicit approval). | | Reconciler workflow prompts | Already-landed / closed PR reconciliation prompt. | | Onboarding new project | Checklist prompt for adding a repository to the MCP workflow. | diff --git a/docs/state-handoff-ledger.md b/docs/state-handoff-ledger.md new file mode 100644 index 0000000..fa0ce76 --- /dev/null +++ b/docs/state-handoff-ledger.md @@ -0,0 +1,86 @@ +# Canonical state handoff ledger (#494) + +Gitea is the system of record for continuation. Every discussion, issue, and PR +should answer: current state, last proof, who acts next, and the exact prompt for +the next role. + +## Lifecycle + +1. Controller opens a discussion. +2. Discussion accumulates substantive comments (default minimum: five). +3. Controller posts a discussion summary when ready. +4. Controller creates linked issues from the summary. +5. Author locks issue, implements, opens PR. +6. Reviewer reviews at current head. +7. Merger merges after formal approval. +8. Reconciler closes superseded or already-landed PRs. +9. Issue/PR/discussion state comments make the next action obvious at every step. + +## Discussion rules + +- Do **not** convert a discussion into issues until it has at least **five + substantive comments**, unless the discussion state comment marks + `URGENCY: urgent` or `URGENCY: trivial`. +- Substantive comment types: proposal, risk/concern, acceptance criteria, + implementation approach, dependency/sequence, summary. +- Before issue creation, post a **discussion summary** with decision, issues to + create, non-goals, unresolved questions, and next prompt. +- Created issues must link back to the discussion; the discussion must link + forward to created issues. + +## State comment templates + +Use `state_handoff_ledger.py` helpers or copy the canonical blocks: + +- `render_discussion_state_comment(...)` +- `render_discussion_summary_comment(...)` +- `render_issue_state_comment(...)` +- `render_pr_state_comment(...)` +- `render_queue_controller_report(...)` + +Post state comments as the **latest canonical update** on the object. Do not +bury state inside PR bodies only. + +## Final report requirements + +Every final report must include a `Controller Handoff` section with: + +- **Current status** — live state after this session +- **Next actor** — `author`, `reviewer`, `merger`, `reconciler`, or `controller` +- **Next action** — one imperative step +- **Next prompt** — ready-to-paste prompt for the next role + +`assess_final_report_next_action_handoff` and +`assess_contradictory_state_handoff` enforce these fields and reject +contradictory claims (for example, ready-to-merge without approval, issue done +without PR proof, discussion complete without summary). + +## Queue controller selection (priority order) + +1. Merge clean approved PRs at current head. +2. Review PRs needing review. +3. Reconcile superseded or already-landed duplicates. +4. Continue blocked-but-now-unblocked issues. +5. Create issues from completed discussions. +6. Start new author work only when higher-priority queue items are clear. + +For each candidate object, read the **latest canonical state comment** before +choosing an action. Output the exact next-role prompt in the controller report. + +## Example workflow states + +| State | Next actor | Typical next action | +|-------|------------|---------------------| +| Discussion needs more comments | controller | Facilitate discussion until five substantive comments or urgent/trivial exception | +| Issue ready for author | author | Lock issue and implement in `branches/` worktree | +| PR needs review | reviewer | Review at pinned head in reviewer worktree | +| PR approved at head | merger | Merge with merger profile after eligibility proof | +| PR superseded | reconciler | Close duplicate/already-landed PR with proof | +| Issue blocked | controller | Post issue state comment with blockers and next prompt | + +## Non-goals + +- Chat history is not the source of truth. +- Do not weaken author/reviewer/merger/reconciler separation. +- Do not replace Gitea issues, PRs, or canonical workflow files under + `skills/llm-project-workflow/`. \ No newline at end of file diff --git a/final_report_validator.py b/final_report_validator.py index e05784d..5df3f1c 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -24,6 +24,7 @@ from review_proofs import ( assess_review_mutation_final_report, assess_validation_report, ) +from state_handoff_ledger import assess_state_handoff_ledger_report from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ @@ -339,6 +340,38 @@ def _rule_shared_controller_handoff( ) +def _rule_shared_state_handoff_next_action(report_text: str) -> list[dict[str, str]]: + result = assess_state_handoff_ledger_report(report_text) + if result.get("complete"): + return [] + findings: list[dict[str, str]] = [] + next_action = result.get("next_action") or {} + contradictions = result.get("contradictions") or {} + if next_action.get("block"): + findings.extend( + _findings_from_reasons( + "shared.state_handoff_next_action", + next_action.get("reasons") or [], + field="Next action handoff", + severity="block", + safe_next_action=next_action.get("safe_next_action") + or "add next-action handoff fields to Controller Handoff", + ) + ) + if contradictions.get("block"): + findings.extend( + _findings_from_reasons( + "shared.state_handoff_contradiction", + contradictions.get("reasons") or [], + field="State handoff", + severity="block", + safe_next_action=contradictions.get("safe_next_action") + or "resolve contradictory state claims before submission", + ) + ) + return findings + + def _rule_shared_email_disclosure(report_text: str) -> list[dict[str, str]]: result = assess_email_disclosure(report_text) if result.get("proven"): @@ -719,6 +752,25 @@ def _rule_reviewer_main_checkout_baseline(report_text: str) -> list[dict[str, st ] +def _rule_reviewer_premerge_baseline_proof(report_text: str) -> list[dict[str, str]]: + from premerge_baseline_proof import assess_premerge_baseline_proof + + result = assess_premerge_baseline_proof(report_text) + if not result.get("block"): + return [] + return _findings_from_reasons( + "reviewer.premerge_baseline_proof", + result.get("reasons") or [], + field="Pre-merge baseline proof", + severity="block", + safe_next_action=result.get("safe_next_action") + or ( + "prove the failure on the PR pre-merge base commit or a known-failure " + "record predating the PR; current-master reproduction is not baseline proof" + ), + ) + + def _rule_reviewer_validation_status_vocabulary( report_text: str, *, @@ -1286,6 +1338,7 @@ _SHARED_CLEANUP_PROOF_RULES = ( _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "review_pr": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_legacy_workspace_mutations, @@ -1298,6 +1351,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_premerge_baseline_proof, _rule_reviewer_validation_status_vocabulary, _rule_reviewer_main_checkout_baseline, _rule_reviewer_main_checkout_path, @@ -1325,6 +1379,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, *_SHARED_CLEANUP_PROOF_RULES, @@ -1333,6 +1388,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reconcile_linked_issue_live, _rule_reconcile_close_proof, _rule_reconcile_pagination_proof, + _rule_reviewer_premerge_baseline_proof, _rule_reviewer_git_fetch_readonly, _rule_reviewer_legacy_workspace_mutations, _rule_reviewer_vague_mutations_none, @@ -1340,12 +1396,14 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "author_issue": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_vague_mutations_none, ], "work_issue": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_shared_issue_acceptance_gate, @@ -1355,17 +1413,20 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "issue_filing": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, ], "inventory": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reconcile_pagination_proof, ], "issue_selection": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, ], diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 8d4b21e..2c92be7 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -828,6 +828,13 @@ import audit_reconciliation_mode # noqa: E402 import review_merge_state_machine # noqa: E402 import pr_work_lease # noqa: E402 import native_mcp_preference # noqa: E402 +import master_parity_gate # noqa: E402 + +# Master-parity baseline (#420): the commit this server process started at. +# Captured once so that, at mutation time, we can detect when the on-disk +# master has advanced past the running code and fail closed until restart. +# Read-only operations are never blocked by staleness. +_STARTUP_PARITY = master_parity_gate.capture_startup_parity(PROJECT_ROOT) import worktree_cleanup_audit # noqa: E402 @@ -4761,6 +4768,34 @@ def gitea_reconcile_merged_cleanups( ) delete_capability_allowed = not _profile_operation_gate("gitea.branch.delete") + + # #534: discover reviewer scratch trees and active leases for those PRs. + scratch_candidates = merged_cleanup_reconcile.discover_reviewer_scratch_worktrees( + PROJECT_ROOT + ) + active_reviewer_leases: dict[int, bool] = {} + pr_states: dict[int, dict] = {} + for scratch in scratch_candidates: + pr_num = int(scratch["pr_number"]) + if pr_num in active_reviewer_leases: + continue + try: + comments = api_get_all(f"{base}/issues/{pr_num}/comments", auth) + except Exception: + comments = [] + lease = reviewer_pr_lease.find_active_reviewer_lease( + comments, pr_number=pr_num + ) + active_reviewer_leases[pr_num] = bool(lease) + try: + pr_live = api_request("GET", f"{base}/pulls/{pr_num}", auth) + except Exception: + pr_live = {} + pr_states[pr_num] = { + "merged": bool((pr_live or {}).get("merged") or (pr_live or {}).get("merged_at")), + "closed": (pr_live or {}).get("state") == "closed", + } + report = merged_cleanup_reconcile.build_reconciliation_report( project_root=PROJECT_ROOT, closed_prs=merged_closed, @@ -4768,6 +4803,8 @@ def gitea_reconcile_merged_cleanups( remote_branch_exists=remote_branch_exists, head_on_master=head_on_master, delete_capability_allowed=delete_capability_allowed, + active_reviewer_leases=active_reviewer_leases, + pr_states=pr_states, ) if dry_run: @@ -4811,6 +4848,14 @@ def gitea_reconcile_merged_cleanups( ) actions.append({"action": "remove_local_worktree", **result}) + for scratch in report.get("reviewer_scratch_entries") or []: + if not scratch.get("safe_to_remove_worktree"): + continue + result = merged_cleanup_reconcile.remove_reviewer_scratch_worktree( + PROJECT_ROOT, scratch.get("worktree_path") or "" + ) + actions.append({"action": "remove_reviewer_scratch_worktree", **result}) + report["dry_run"] = False report["executed"] = True report["actions"] = actions @@ -5571,15 +5616,41 @@ def _try_auto_switch_for_operation(op: str, host: str | None = None) -> bool: return False +def _current_master_parity() -> dict: + """Assess this process's code against the on-disk master HEAD (#420).""" + current_head = master_parity_gate.read_git_head(PROJECT_ROOT) + return master_parity_gate.assess_master_parity(_STARTUP_PARITY, current_head) + + +def _master_parity_block(op: str) -> list[str]: + """Fail-closed staleness reasons for a mutation *op* (#420). + + Read-only operations (``gitea.read``) are never blocked: a stale server can + still be inspected. Any other (mutating) operation is refused when the + on-disk master has definitively advanced past the running code, since newly + merged capability gates would not yet be loaded in memory. + """ + if op == "gitea.read": + return [] + return master_parity_gate.parity_block_reasons(_current_master_parity()) + + def _profile_operation_gate(op: str) -> list[str]: - """Profile permission check for a single gated operation (#126, #216). + """Profile permission check for a single gated operation (#126, #216, #420). Issue discussion comments are gated separately from the gitea.pr.* review/merge family: listing requires ``gitea.read``, creating requires ``gitea.issue.comment``. Closing a PR requires the distinct ``gitea.pr.close`` (#216). Returns a list of block reasons (empty = allowed); an unreadable profile fails closed. + + A mutating operation is additionally refused when the server code is stale + relative to master (#420), so a long-running process cannot bypass a + capability gate that has since been merged. """ + stale_reasons = _master_parity_block(op) + if stale_reasons: + return stale_reasons try: profile = get_profile() except Exception as exc: @@ -7693,6 +7764,24 @@ def gitea_get_runtime_context( PROJECT_ROOT), } + parity = _current_master_parity() + result["master_parity"] = { + "in_parity": parity["in_parity"], + "stale": parity["stale"], + "restart_required": parity["restart_required"], + "startup_head": parity["startup_head"], + "current_head": parity["current_head"], + "summary": master_parity_gate.format_parity(parity), + "mutation_gate_enforced": not master_parity_gate.gate_disabled(), + } + if parity["stale"] and not master_parity_gate.gate_disabled(): + safe_next_action = ( + "Server code is stale relative to master; restart the Gitea MCP " + "server to load current capability gates before mutating. " + f"({master_parity_gate.format_parity(parity)})" + ) + result["safe_next_action"] = safe_next_action + if reveal and h: result["server"] = gitea_url(h, "").rstrip("/") @@ -7700,6 +7789,43 @@ def gitea_get_runtime_context( @mcp.tool() +def gitea_assess_master_parity( + remote: str = "dadeschools", + host: str | None = None, +) -> dict: + """Read-only: is the running server code in parity with the on-disk master? + + The MCP server loads its capability-gate code into memory at startup; + ``master`` advancing (e.g. a newly merged security gate) does not take + effect until the process restarts. This tool compares the commit the + process started at against the current workspace ``HEAD`` and reports + whether a restart is required before mutations are safe again (#420). + + Never mutates and makes no network calls. Read-only operations are never + blocked by staleness; only mutating operations fail closed while stale. + + Returns: + dict with 'in_parity', 'stale', 'restart_required', 'startup_head', + 'current_head', 'mutation_gate_enforced', 'summary', 'reasons', and a + 'report' recovery payload when stale. + """ + parity = _current_master_parity() + enforced = not master_parity_gate.gate_disabled() + out = { + "in_parity": parity["in_parity"], + "stale": parity["stale"], + "restart_required": parity["restart_required"], + "determinable": parity["determinable"], + "startup_head": parity["startup_head"], + "current_head": parity["current_head"], + "mutation_gate_enforced": enforced, + "summary": master_parity_gate.format_parity(parity), + "reasons": parity["reasons"], + "process_root": PROJECT_ROOT, + } + if parity["stale"] and enforced: + out["report"] = master_parity_gate.parity_report(parity) + return out def gitea_record_pre_review_command( command: str, cwd: str | None = None, diff --git a/master_parity_gate.py b/master_parity_gate.py new file mode 100644 index 0000000..efb7314 --- /dev/null +++ b/master_parity_gate.py @@ -0,0 +1,168 @@ +"""Master-parity staleness gate (#420). + +The Gitea MCP server loads its capability-gate code and workflow logic into +memory when the process starts. When ``master`` advances -- for example a newly +merged security gate such as the branch-delete capability gate (#408/#410) -- +the running process keeps executing the *old* code until it is restarted. A +stale server can therefore still perform a mutation that the updated codebase +would forbid. + +Runtime profile/config data is already read live from disk on every call +(``gitea_config.load_config`` re-reads the JSON file each time), so profile +``allowed_operations`` changes take effect immediately without a restart. The +gap this module closes is *code* parity: it captures the server process's +source-tree commit at startup and detects, at mutation time, when the on-disk +``master`` HEAD has advanced past it. Detected staleness fails closed with a +restart-required recovery report, while read-only operations stay allowed so a +stale server can still be inspected. + +The core assessment is pure -- callers inject the observed HEAD SHAs -- so the +logic is fully unit-testable without a git checkout. +""" + +from __future__ import annotations + +import os +import subprocess + +# Environment escape hatches (ops + tests): +# GITEA_MCP_DISABLE_PARITY_GATE -> disable enforcement entirely (fail open). +# GITEA_TEST_CURRENT_HEAD -> force the "current" HEAD read, for tests. +ENV_DISABLE = "GITEA_MCP_DISABLE_PARITY_GATE" +ENV_TEST_CURRENT_HEAD = "GITEA_TEST_CURRENT_HEAD" + + +def read_git_head(root: str) -> str | None: + """Return the current ``HEAD`` commit SHA of *root*, or ``None``. + + ``None`` means the SHA could not be determined (not a git checkout, git + unavailable, or an error). A test override via ``GITEA_TEST_CURRENT_HEAD`` + takes precedence so the gate can be exercised deterministically. + """ + forced = os.environ.get(ENV_TEST_CURRENT_HEAD) + if forced is not None: + return forced.strip() or None + if not root: + return None + try: + res = subprocess.run( + ["git", "-C", root, "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=False, + ) + except Exception: + return None + if res.returncode != 0: + return None + return (res.stdout or "").strip() or None + + +def capture_startup_parity(root: str, head: str | None = None) -> dict: + """Capture the process source-tree baseline once at server startup. + + *head* may be injected (tests); otherwise it is read from *root*. The result + is an opaque baseline handed back to :func:`assess_master_parity`. + """ + startup_head = head if head is not None else read_git_head(root) + return {"root": root, "startup_head": startup_head} + + +def _short(sha: str | None) -> str: + return sha[:12] if sha else "unknown" + + +def assess_master_parity(startup: dict | None, current_head: str | None) -> dict: + """Compare the startup baseline against the current on-disk ``HEAD``. + + Pure: both HEADs are supplied by the caller. Returns a structured result: + + - ``in_parity`` -- server code matches the on-disk master (or parity + could not be determined, which is not treated as stale). + - ``stale`` -- the on-disk master has definitively advanced past the + running process. + - ``restart_required`` -- alias of ``stale``; the recovery action. + - ``determinable`` -- whether both HEADs were known well enough to compare. + - ``startup_head`` / ``current_head`` / ``reasons``. + """ + startup_head = (startup or {}).get("startup_head") + reasons: list[str] = [] + + if startup_head is None: + reasons.append( + "startup commit was not captured; code parity cannot be enforced") + return _result(True, False, False, startup_head, current_head, reasons) + + if current_head is None: + reasons.append( + "current workspace HEAD could not be read; code parity cannot be " + "enforced") + return _result(True, False, False, startup_head, current_head, reasons) + + if startup_head == current_head: + return _result(True, False, True, startup_head, current_head, reasons) + + reasons.append( + f"MCP server started at commit {_short(startup_head)} but the workspace " + f"master is now {_short(current_head)}; restart the server to load the " + f"current capability gates") + return _result(False, True, True, startup_head, current_head, reasons) + + +def _result(in_parity, stale, determinable, startup_head, current_head, reasons): + return { + "in_parity": in_parity, + "stale": stale, + "restart_required": stale, + "determinable": determinable, + "startup_head": startup_head, + "current_head": current_head, + "reasons": list(reasons), + } + + +def gate_disabled() -> bool: + """Whether the parity gate is disabled by env escape hatch.""" + return bool((os.environ.get(ENV_DISABLE) or "").strip()) + + +def parity_block_reasons(assessment: dict) -> list[str]: + """Block reasons for a mutation gate (empty when the mutation may proceed). + + A disabled gate or an in-parity / non-determinable assessment yields no + reasons; only a definitively stale server blocks. + """ + if gate_disabled(): + return [] + if assessment.get("stale"): + return list(assessment.get("reasons") or + ["server code is stale relative to master (fail closed)"]) + return [] + + +def parity_report(assessment: dict) -> dict: + """Structured stale-server report for permission-block payloads.""" + return { + "kind": "server_stale", + "restart_required": True, + "startup_head": assessment.get("startup_head"), + "current_head": assessment.get("current_head"), + "reasons": list(assessment.get("reasons") or []), + "recovery": [ + "The running MCP server is executing code older than the current " + "master and may not enforce newly merged capability gates.", + "Restart the Gitea MCP server so it reloads master's capability " + "gates and execution profiles before retrying the mutation.", + ], + } + + +def format_parity(assessment: dict) -> str: + """One-line human summary for logs / runtime context.""" + if assessment.get("stale"): + return (f"STALE: started {_short(assessment.get('startup_head'))}, " + f"master now {_short(assessment.get('current_head'))} " + f"(restart required)") + if not assessment.get("determinable"): + return "parity indeterminate (baseline or current HEAD unknown)" + return f"in parity at {_short(assessment.get('current_head'))}" diff --git a/mcp-menu.sh b/mcp-menu.sh index 4d124f2..215f017 100755 --- a/mcp-menu.sh +++ b/mcp-menu.sh @@ -115,7 +115,15 @@ Workflow: } show_reviewer_prompts() { - print_prompt_block "Reviewer — PR review" \ + while true; do + printf '\n--- Reviewer workflow prompts ---\n' + printf ' 1) Standard PR review\n' + printf ' 2) Skip already-reviewed stale REQUEST_CHANGES PR and hand off to author\n' + printf ' 0) Back\n' + read -r -p 'Choice: ' choice + case "$choice" in + 1) + print_prompt_block "Reviewer — PR review" \ "You are the REVIEWER session for /. Goal: review PR #

only — do not merge unless explicitly switched to merger mode. @@ -126,6 +134,35 @@ Workflow: 3. gitea_view_pr, validate scope, run required checks in the correct worktree. 4. gitea_review_pr with approve, request-changes, or comment as warranted. 5. Final report: PR head SHA, verdict, validation evidence, mutation ledger. No merge in reviewer-only runs." + ;; + 2) + print_prompt_block "Reviewer — skip already-reviewed stale REQUEST_CHANGES PR and hand off to author" \ +"You are the REVIEWER session for /. + +Goal: review PR #

only far enough to determine whether the current head already has a non-stale REQUEST_CHANGES verdict. If it does, do NOT submit another terminal review mutation — produce an author handoff and stop. + +Workflow: +1. gitea_view_pr; pin the current head SHA. +2. Load prior reviews; find the latest REQUEST_CHANGES and the head SHA it was bound to. +3. If that REQUEST_CHANGES is still bound to the current head (non-stale), do not submit another terminal review mutation. Confirm the binding and summarize the blockers. +4. Run only the diagnostics needed to produce a useful author handoff — no full re-review. +5. Duplicate/supersession check: confirm no newer canonical PR or superseding head changes the decision. +6. Stop — no new review mutation. + +Return: +- selected PR and issue +- current head SHA +- prior REQUEST_CHANGES proof (review id + bound head SHA) +- duplicate/supersession analysis +- validation summary +- why no new review mutation was submitted +- corrected mutation ledger, including local worktree create/remove if used +- author-ready fix prompt" + ;; + 0) return ;; + *) printf 'Invalid choice.\n' ;; + esac + done } show_merger_prompts() { diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index bf81258..e6e7236 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -17,6 +17,11 @@ import issue_lock_store PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) +# Reviewer scratch folders: review-pr or review-pr-submit (#534). +REVIEWER_SCRATCH_NAME_RE = re.compile( + r"^review-pr(?P\d+)(?:-submit)?$", + re.IGNORECASE, +) def extract_linked_issue(title: str, body: str) -> int | None: @@ -36,6 +41,148 @@ def resolve_worktree_path(project_root: str, branch: str) -> str: return os.path.join(project_root, "branches", branch_worktree_folder(branch)) +def _git_worktree_porcelain(project_root: str) -> list[dict[str, str | None]]: + """Parse ``git worktree list --porcelain`` into structured records.""" + res = subprocess.run( + ["git", "-C", project_root, "worktree", "list", "--porcelain"], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return [] + + records: list[dict[str, str | None]] = [] + current: dict[str, str | None] = {} + for line in res.stdout.splitlines(): + line = line.strip() + if not line: + if current.get("worktree"): + records.append(current) + current = {} + continue + if line.startswith("worktree "): + if current.get("worktree"): + records.append(current) + current = {"worktree": line[9:].strip(), "branch": None, "head": None} + elif line.startswith("branch ") and current: + ref = line[7:].strip() + current["branch"] = ( + ref[11:] if ref.startswith("refs/heads/") else ref + ) + elif line.startswith("HEAD ") and current: + current["head"] = line[5:].strip() + elif line == "detached" and current: + current["branch"] = None + if current.get("worktree"): + records.append(current) + return records + + +def discover_local_worktrees(project_root: str) -> dict[str, str]: + """Return a mapping from branch name to absolute worktree path (#528).""" + mapping: dict[str, str] = {} + for record in _git_worktree_porcelain(project_root): + branch_name = record.get("branch") + path = record.get("worktree") + if branch_name and path: + mapping[str(branch_name)] = str(path) + return mapping + + +def parse_reviewer_scratch_folder(folder_name: str) -> int | None: + """Return PR number for a reviewer scratch folder name, else None (#534).""" + match = REVIEWER_SCRATCH_NAME_RE.match((folder_name or "").strip()) + if not match: + return None + return int(match.group("pr")) + + +def discover_reviewer_scratch_worktrees(project_root: str) -> list[dict[str, Any]]: + """Discover detached reviewer scratch worktrees under ``branches/`` (#534). + + Matches folder names ``review-pr`` and ``review-pr-submit`` only. + These are never treated as author worktree paths. + """ + root = os.path.realpath(project_root) + branches_root = os.path.realpath(os.path.join(root, "branches")) + found: list[dict[str, Any]] = [] + for record in _git_worktree_porcelain(project_root): + path = record.get("worktree") + if not path: + continue + real = os.path.realpath(path) + parent = os.path.dirname(real) + if parent != branches_root: + continue + folder = os.path.basename(real) + pr_number = parse_reviewer_scratch_folder(folder) + if pr_number is None: + continue + found.append( + { + "pr_number": pr_number, + "worktree_path": real, + "folder_name": folder, + "current_branch": record.get("branch"), + "head_sha": record.get("head"), + "worktree_kind": "reviewer_scratch", + } + ) + return found + + +def assess_reviewer_scratch_cleanup( + *, + pr_number: int, + worktree_path: str, + folder_name: str, + pr_merged: bool, + pr_closed: bool, + worktree_state: dict[str, Any], + active_reviewer_lease: bool, +) -> dict[str, Any]: + """Assess whether a reviewer scratch worktree is safe to remove (#534).""" + reasons: list[str] = [] + exists = bool(worktree_state.get("exists")) + if not (pr_merged or pr_closed): + reasons.append("associated PR is still open") + if not exists: + reasons.append("reviewer scratch worktree not present") + if exists and worktree_state.get("clean") is False: + dirty = worktree_state.get("dirty_files") or [] + reasons.append( + "reviewer scratch worktree has tracked edits" + + (f" ({', '.join(dirty)})" if dirty else "") + ) + if active_reviewer_lease: + reasons.append("active reviewer lease still requires this worktree") + current_branch = worktree_state.get("current_branch") + if current_branch: + # Scratch trees are expected detached; a named branch is ambiguous ownership. + reasons.append( + f"reviewer scratch worktree is on branch '{current_branch}' " + "(expected detached HEAD for review-pr scratch trees)" + ) + + safe = exists and not reasons + return { + "pr_number": pr_number, + "worktree_path": worktree_path, + "folder_name": folder_name, + "worktree_kind": "reviewer_scratch", + "worktree_exists": exists, + "worktree_clean": worktree_state.get("clean"), + "safe_to_remove_worktree": safe, + "block_reasons": reasons, + "recommended_action": ( + "remove_reviewer_scratch_worktree" if safe else "keep_reviewer_scratch_worktree" + ), + # Never treat as author worktree cleanup. + "author_worktree_cleanup": False, + } + + def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: if path: return issue_lock_store.read_lock_file(path.strip()) @@ -216,6 +363,7 @@ def build_pr_cleanup_entry( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + worktree_map: dict[str, str] | None = None, ) -> dict[str, Any]: pr_number = int(pr["number"]) head_branch = pr.get("head") or "" @@ -224,7 +372,16 @@ def build_pr_cleanup_entry( title = pr.get("title") or "" body = pr.get("body") or "" merged = bool(pr.get("merged_at")) - worktree_path = resolve_worktree_path(project_root, head_branch) + + discovered_path = worktree_map.get(head_branch) if worktree_map else None + derived_path = resolve_worktree_path(project_root, head_branch) + if discovered_path: + worktree_path = discovered_path + match_type = "branch" + else: + worktree_path = derived_path + match_type = "path" + worktree_state = read_local_worktree_state(worktree_path) worktree_state["worktree_path"] = worktree_path active_lock = has_active_issue_lock(head_branch, issue_lock_path) @@ -247,6 +404,8 @@ def build_pr_cleanup_entry( worktree_state=worktree_state, active_lock=active_lock, ) + local["match_type"] = match_type + return { "pr_number": pr_number, "issue_number": extract_linked_issue(title, body), @@ -270,8 +429,11 @@ def build_reconciliation_report( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + active_reviewer_leases: dict[int, bool] | None = None, + pr_states: dict[int, dict[str, Any]] | None = None, ) -> dict[str, Any]: open_heads = collect_open_pr_heads(open_prs) + worktree_map = discover_local_worktrees(project_root) entries: list[dict[str, Any]] = [] for pr in closed_prs or []: if not pr.get("merged_at"): @@ -290,19 +452,63 @@ def build_reconciliation_report( delete_capability_allowed=delete_capability_allowed, issue_lock_path=issue_lock_path, protected_branches=protected_branches, + worktree_map=worktree_map, ) ) + + # #534: reviewer scratch worktrees are reported separately from author cleanup. + lease_map = active_reviewer_leases or {} + state_map = pr_states or {} + open_pr_numbers = { + int(pr["number"]) for pr in (open_prs or []) if pr.get("number") is not None + } + closed_by_number = { + int(pr["number"]): pr for pr in (closed_prs or []) if pr.get("number") is not None + } + scratch_entries: list[dict[str, Any]] = [] + for scratch in discover_reviewer_scratch_worktrees(project_root): + pr_number = int(scratch["pr_number"]) + state_info = state_map.get(pr_number) or {} + closed_pr = closed_by_number.get(pr_number) + if closed_pr is not None: + pr_merged = bool(closed_pr.get("merged_at") or closed_pr.get("merged")) + pr_closed = True + elif pr_number in open_pr_numbers: + pr_merged = False + pr_closed = False + else: + pr_merged = bool(state_info.get("merged")) + pr_closed = bool(state_info.get("closed") or state_info.get("merged")) + worktree_path = scratch["worktree_path"] + worktree_state = read_local_worktree_state(worktree_path) + worktree_state["worktree_path"] = worktree_path + # Prefer live branch from state (git) over porcelain branch field. + assessment = assess_reviewer_scratch_cleanup( + pr_number=pr_number, + worktree_path=worktree_path, + folder_name=scratch["folder_name"], + pr_merged=pr_merged, + pr_closed=pr_closed, + worktree_state=worktree_state, + active_reviewer_lease=bool(lease_map.get(pr_number)), + ) + scratch_entries.append(assessment) + return { "project_root": os.path.realpath(project_root), "merged_pr_count": len(entries), "entries": entries, + "reviewer_scratch_entries": scratch_entries, + "reviewer_scratch_count": len(scratch_entries), "dry_run": True, "executed": False, } def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: - worktree_path = resolve_worktree_path(project_root, branch) + worktree_map = discover_local_worktrees(project_root) + discovered_path = worktree_map.get(branch) + worktree_path = discovered_path or resolve_worktree_path(project_root, branch) if not os.path.isdir(worktree_path): return { "success": False, @@ -326,4 +532,65 @@ def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: "performed": True, "message": f"removed worktree {worktree_path}", "worktree_path": worktree_path, + } + + +def remove_reviewer_scratch_worktree( + project_root: str, + worktree_path: str, +) -> dict[str, Any]: + """Remove a reviewer scratch worktree by absolute path (#534). + + Fail closed unless the path is a direct ``branches/review-pr*`` child of + *project_root*. Never routes through author branch-name derivation. + """ + root = os.path.realpath(project_root) + branches_root = os.path.realpath(os.path.join(root, "branches")) + real = os.path.realpath((worktree_path or "").strip()) + folder = os.path.basename(real) + if os.path.dirname(real) != branches_root: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": ( + "reviewer scratch path must be a direct child of " + f"{branches_root}; got {real}" + ), + } + if parse_reviewer_scratch_folder(folder) is None: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": f"path is not a reviewer scratch folder: {folder}", + } + if not os.path.isdir(real): + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": f"worktree not found: {real}", + } + res = subprocess.run( + ["git", "-C", project_root, "worktree", "remove", real], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": (res.stderr or res.stdout or "worktree remove failed").strip(), + "worktree_path": real, + } + return { + "success": True, + "performed": True, + "worktree_kind": "reviewer_scratch", + "message": f"removed reviewer scratch worktree {real}", + "worktree_path": real, + "author_worktree_cleanup": False, } \ No newline at end of file diff --git a/pr_queue_cleanup.py b/pr_queue_cleanup.py index c1237aa..d6091ca 100644 --- a/pr_queue_cleanup.py +++ b/pr_queue_cleanup.py @@ -132,6 +132,13 @@ def check_cleanup_task_allowed(task: str) -> tuple[bool, list[str]]: _SELECTED_PR_RE = re.compile( r"^\s*[-*]?\s*selected pr\s*:\s*#?(\d+)", re.IGNORECASE | re.MULTILINE ) +_SELECTED_PR_NONE_RE = re.compile( + r"^\s*[-*]?\s*selected pr\s*:\s*(?:none|n/a)\b", re.IGNORECASE | re.MULTILINE +) +_EMPTY_QUEUE_RE = re.compile( + r"\b0 open pr|\bno open pr|\bno eligible pr|\bempty (?:review )?queue|\binventory empty|\btrusted_empty\b", + re.IGNORECASE, +) _NEXT_SUGGESTED_RE = re.compile( r"^\s*[-*]?\s*next suggested pr\s*:", re.IGNORECASE | re.MULTILINE ) @@ -176,7 +183,11 @@ def assess_pr_queue_cleanup_report(report_text: str) -> dict: selected = _SELECTED_PR_RE.findall(text) if len(selected) == 0: - reasons.append("report must name exactly one Selected PR") + if _SELECTED_PR_NONE_RE.search(text): + if not _EMPTY_QUEUE_RE.search(text): + reasons.append("Selected PR is 'none' but no valid empty-queue claim found") + else: + reasons.append("report must name exactly one Selected PR") elif len(set(selected)) > 1: reasons.append( "cleanup run selected multiple PRs " @@ -184,6 +195,9 @@ def assess_pr_queue_cleanup_report(report_text: str) -> dict: "PR per run" ) + if len(selected) > 0 and _SELECTED_PR_NONE_RE.search(text): + reasons.append("report contains both a selected PR and a claim of none selected") + terminal = _TERMINAL_MUTATION_RE.findall(text) if len(terminal) > 1: reasons.append( diff --git a/premerge_baseline_proof.py b/premerge_baseline_proof.py new file mode 100644 index 0000000..cf9b971 --- /dev/null +++ b/premerge_baseline_proof.py @@ -0,0 +1,176 @@ +"""Pre-merge baseline proof verifier for non-zero validation exits (#533). + +A controller/reviewer may reproduce a failing test on *current* master and +describe it as proof of a "known baseline failure". Reproducing a failure on +post-merge master only proves the failure exists on current master — it does +NOT prove the failure pre-existed the PR under review. A PR can introduce or +preserve a regression, then once merged the failure appears on master and gets +mislabeled "baseline", weakening validation gates. + +This module distinguishes four canonical validation outcomes: + +- ``CLEAN_PASS`` — the validation command exited zero. +- ``CURRENT_MASTER_FAILURE_REPRODUCED`` — the same failure reproduces on + current (post-merge) master. Not sufficient as baseline proof. +- ``PREMERGE_BASELINE_PROVEN_FAILURE`` — the failure is proven on the PR's + pre-merge base commit, or by a documented known-failure record that predates + the PR. +- ``UNRESOLVED_REGRESSION_RISK`` — a non-zero exit that is neither a clean pass + nor proven pre-existing. + +A report may only call a non-zero validation exit a "baseline"/"pre-existing" +failure when it carries pre-merge proof. +""" + +from __future__ import annotations + +import re +from typing import Any + +CLEAN_PASS = "clean pass" +CURRENT_MASTER_FAILURE_REPRODUCED = "current-master failure reproduced" +PREMERGE_BASELINE_PROVEN_FAILURE = "pre-merge baseline-proven failure" +UNRESOLVED_REGRESSION_RISK = "unresolved regression risk" + +# A non-zero validation exit is claimed somewhere in the report. +_NONZERO_EXIT_RE = re.compile( + r"(?:exit(?:\s*(?:status|code))?\s*[:=]?\s*(?:[1-9]\d*)" + r"|\b\d+\s+failed\b" + r"|\bnon[- ]zero\s+(?:exit|validation))", + re.IGNORECASE, +) +# The report claims the failure is pre-existing / a baseline failure. +_BASELINE_CLAIM_RE = re.compile( + r"(?:pre[- ]?existing(?:\s+(?:baseline|failure))?" + r"|baseline(?:[- ]proven)?\s+failure" + r"|known[- ]baseline" + r"|failure\s+is\s+baseline" + r"|already\s+fail(?:s|ing)\s+on\s+master)", + re.IGNORECASE, +) +# Only-current-master reproduction language (insufficient on its own). +_CURRENT_MASTER_RE = re.compile( + r"(?:current[- ]master\s+(?:reproduc|failure)" + r"|reproduc\w*\s+on\s+(?:current\s+)?master" + r"|post[- ]merge\s+master" + r"|fails\s+on\s+(?:current\s+)?master\s+(?:now|too|as\s+well))", + re.IGNORECASE, +) +# Pre-merge base proof: a base commit tested before the PR landed. +_PREMERGE_BASE_RE = re.compile( + r"(?:pre[- ]merge\s+base(?:\s+commit)?" + r"|pr\s+base\s+commit" + r"|merge[- ]base(?:\s+commit)?" + r"|base\s+commit\s+before\s+(?:the\s+)?pr)", + re.IGNORECASE, +) +# Documented known-failure record predating the PR. +_KNOWN_FAILURE_RECORD_RE = re.compile( + r"known[- ]failure\s+(?:record|reference|ledger)\b.{0,80}?" + r"(?:predat|before\s+(?:the\s+)?pr|pre[- ]existing|prior\s+to\s+(?:the\s+)?pr)", + re.IGNORECASE | re.DOTALL, +) + +# Required proof fields for a pre-merge baseline claim. +_FIELD_PATTERNS: dict[str, re.Pattern[str]] = { + "base commit": re.compile( + r"(?:pre[- ]merge\s+)?base\s+commit\s*[:=]\s*([0-9a-f]{7,40})", re.IGNORECASE + ), + "tested commit": re.compile( + r"tested\s+commit\s*[:=]\s*([0-9a-f]{7,40})", re.IGNORECASE + ), + "command": re.compile(r"command\s*[:=]\s*\S", re.IGNORECASE), + "exit status": re.compile(r"exit\s*(?:status|code)?\s*[:=]\s*\d+", re.IGNORECASE), + "failure signature": re.compile( + r"failure\s+signature\s*[:=]\s*\S", re.IGNORECASE + ), +} + + +def assess_premerge_baseline_proof(report_text: str) -> dict[str, Any]: + """Assess whether a claimed baseline failure carries pre-merge proof. + + Returns a dict with ``proven``, ``block``, ``label``, ``reasons``, + ``skipped`` and ``safe_next_action``. Only blocks when the report claims a + non-zero validation exit is baseline/pre-existing without valid pre-merge + proof. + """ + text = report_text or "" + + has_failure = bool(_NONZERO_EXIT_RE.search(text)) + claims_baseline = bool(_BASELINE_CLAIM_RE.search(text)) + + # No failure claimed, or no baseline assertion — nothing to enforce here. + if not has_failure and not claims_baseline: + return { + "proven": True, + "block": False, + "label": CLEAN_PASS, + "reasons": [], + "skipped": True, + "safe_next_action": "", + } + + if not claims_baseline: + # A non-zero exit not asserted as baseline — surface as regression risk, + # but do not block (the report never claimed a clean/baseline pass). + return { + "proven": True, + "block": False, + "label": UNRESOLVED_REGRESSION_RISK, + "reasons": [], + "skipped": False, + "safe_next_action": "", + } + + has_premerge_base = bool(_PREMERGE_BASE_RE.search(text)) + has_known_record = bool(_KNOWN_FAILURE_RECORD_RE.search(text)) + only_current_master = bool(_CURRENT_MASTER_RE.search(text)) + + reasons: list[str] = [] + + if not (has_premerge_base or has_known_record): + if only_current_master: + reasons.append( + "baseline failure claimed from current-master reproduction only; " + "that proves the failure on current master, not that it pre-existed " + "the PR — provide pre-merge base proof or a known-failure record" + ) + else: + reasons.append( + "baseline/pre-existing failure claimed without pre-merge proof " + "(no pre-merge base commit and no documented known-failure record " + "predating the PR)" + ) + + # Require the concrete proof fields regardless of proof source. + missing = [name for name, pat in _FIELD_PATTERNS.items() if not pat.search(text)] + if missing: + reasons.append( + "pre-merge baseline claim missing required proof field(s): " + + ", ".join(missing) + ) + + if reasons: + return { + "proven": False, + "block": True, + "label": UNRESOLVED_REGRESSION_RISK, + "reasons": reasons, + "skipped": False, + "safe_next_action": ( + "prove the failure on the PR pre-merge base commit (or cite a " + "known-failure record predating the PR) and state base commit, " + "tested commit, command, exit status, and failure signature; " + "current-master reproduction alone is not baseline proof" + ), + } + + return { + "proven": True, + "block": False, + "label": PREMERGE_BASELINE_PROVEN_FAILURE, + "reasons": [], + "skipped": False, + "safe_next_action": "", + } diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..73b9f8d 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -182,4 +182,17 @@ Ready-to-copy task prompts live in [`templates/`](templates/): Releases follow SemVer from remote `master` only, after full test suite passes. See [`templates/release-tag.md`](templates/release-tag.md) and -`scripts/release-tag`. \ No newline at end of file +`scripts/release-tag`. + +## Bootstrap Review Path (#557) + +Self-hosted MCP workflow fixes can deadlock live review daemons. Do not bypass +gates with raw API, direct imports, or root checkout edits. + +If and only if a controller posts a durable `BOOTSTRAP REVIEW AUTHORIZATION +(#557)` record, follow: + +`docs/bootstrap-review-path.md` + +Otherwise stop with BLOCKED + DIAGNOSE. Bootstrap never weakens normal PR gates. + diff --git a/skills/llm-project-workflow/templates/pr-queue-cleanup.md b/skills/llm-project-workflow/templates/pr-queue-cleanup.md index d3913f1..1031a2a 100644 --- a/skills/llm-project-workflow/templates/pr-queue-cleanup.md +++ b/skills/llm-project-workflow/templates/pr-queue-cleanup.md @@ -16,7 +16,7 @@ Rules: reviewing a second PR after a terminal mutation in this run. - After REQUEST_CHANGES: stop. After APPROVED: merge only this PR and only if operator explicitly authorized merge for this PR in this run. -- Report Next suggested PR without continuing to it. +- Report Next suggested PR without continuing to it. If the PR queue is empty, look at approvals, or issues next. Operator PR list (optional): Merge authorized for selected PR in this run: diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 668ee4c..11a8d7e 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -28,7 +28,8 @@ Repo name disambiguation (Gitea-Tools blind review hardening): `pr_inventory_trust_gate.status`, trust-gate reasons, corroboration, remote/owner/repo/state filter, and the inventory MCP profile. A recent merge commit is not valid corroboration. Author-bound sessions must not present - reviewer queue inventory as a reviewer decision. + reviewer queue inventory as a reviewer decision. If the PR queue is empty, + look at approvals, or issues next. Load the canonical workflow first: `skills/llm-project-workflow/workflows/review-merge-pr.md` (task mode: review-merge-pr). @@ -129,4 +130,14 @@ Cleanup status. If you could not merge, name the exact gate. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + +Baseline failure proof (#533): if a validation command exits non-zero, do NOT +call it a clean pass. Only label a failure "baseline"/"pre-existing" with +pre-merge proof — the failure reproduced on the PR's pre-merge base commit, or a +documented known-failure record predating the PR. Reproducing on current +(post-merge) master is "current-master failure reproduced", NOT baseline proof. +State: base commit, tested commit, command, exit status, failure signature. +Use one label: clean pass / current-master failure reproduced / pre-merge +baseline-proven failure / unresolved regression risk +(final_report_validator: reviewer.premerge_baseline_proof). ``` diff --git a/skills/llm-project-workflow/workflows/pr-queue-cleanup.md b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md index 8a1be98..f43b968 100644 --- a/skills/llm-project-workflow/workflows/pr-queue-cleanup.md +++ b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md @@ -55,6 +55,10 @@ claim. Select exactly one PR according to project queue ordering rules PRs only with live per-PR proof. No multi-PR validation and no batch report may substitute for per-PR proof. +If the open PR queue is empty: +* First, look at **Approvals** next: check if there are open PRs with pending/completed approvals requiring attention or merge. +* Next, look at **Issues** next: check if there are unresolved open issues requiring action/fixes. + ## 4. Terminal mutation chain `pr_queue_cleanup.resolve_cleanup_run_state` is the authority: diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 5a90ea5..95cee0e 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -272,6 +272,10 @@ If queue ordering cannot be proven, stop and produce a recovery handoff. ## 10. Select the next actionable PR using project rules +If the open PR queue is empty: +* First, look at approvals next to see if there are pending approvals or approved PRs that need attention/merge. +* Next, look at issues next to see if there are open issues requiring action/fixes. + Do not review your own PR. Do not review stale, draft, blocked, duplicate, already-owned, dependency-blocked, already-landed, already-requested-changes work unless the rules explicitly allow it. diff --git a/state_handoff_ledger.py b/state_handoff_ledger.py new file mode 100644 index 0000000..809043e --- /dev/null +++ b/state_handoff_ledger.py @@ -0,0 +1,374 @@ +"""Canonical state handoff ledger for discussions, issues, and PRs (#494).""" + +from __future__ import annotations + +import re +from typing import Any + +HANDOFF_HEADING = "Controller Handoff" + +ISSUE_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "STATUS", + "RELATED_PRS", + "BRANCH", + "HEAD_SHA", + "VALIDATION", + "BLOCKERS", + "DUPLICATES_OR_SUPERSEDES", + "LAST_UPDATED_BY", +) + +PR_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "ISSUE", + "BASE", + "HEAD", + "HEAD_SHA", + "REVIEW_STATUS", + "VALIDATION", + "BLOCKERS", + "SUPERSEDES", + "SUPERSEDED_BY", + "MERGE_READY", + "LAST_UPDATED_BY", +) + +DISCUSSION_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "COMMENT_COUNT", + "SUBSTANTIVE_COMMENTS", + "URGENCY", + "BLOCKERS", + "LAST_UPDATED_BY", +) + +DISCUSSION_SUMMARY_FIELDS = ( + "DECISION", + "ISSUES_TO_CREATE", + "NON_GOALS", + "UNRESOLVED_QUESTIONS", + "NEXT_PROMPT", + "LAST_UPDATED_BY", +) + +QUEUE_CONTROLLER_FIELDS = ( + "QUEUE_STATE", + "SELECTED_OBJECT", + "SELECTED_OBJECT_TYPE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "EVIDENCE", + "LAST_UPDATED_BY", +) + +FINAL_REPORT_NEXT_ACTION_FIELDS = ( + ("Current status", ("current status",)), + ("Next actor", ("next actor", "next_actor")), + ("Next action", ("next action", "next_action")), + ("Next prompt", ("next prompt", "next_prompt")), +) + +STATE_COMMENT_KINDS = frozenset({ + "issue_state", + "pr_state", + "discussion_state", + "discussion_summary", + "queue_controller", +}) + +_FIELDS_BY_KIND = { + "issue_state": ISSUE_STATE_FIELDS, + "pr_state": PR_STATE_FIELDS, + "discussion_state": DISCUSSION_STATE_FIELDS, + "discussion_summary": DISCUSSION_SUMMARY_FIELDS, + "queue_controller": QUEUE_CONTROLLER_FIELDS, +} + +_FIELD_LINE_RE = re.compile( + r"^([A-Z][A-Z0-9_]+)\s*:\s*(.*)$", + re.MULTILINE, +) +_HANDOFF_SECTION_RE = re.compile(r"^##\s*Controller Handoff\s*$", re.I | re.M) +_READY_TO_MERGE_RE = re.compile( + r"\b(?:ready[_ ]to[_ ]merge|merge[_ ]ready\s*:\s*(?:yes|true))\b", + re.IGNORECASE, +) +_APPROVAL_PROOF_RE = re.compile( + r"\b(?:review decision\s*:\s*approve|approved at|approval at current head|" + r"formal approval|review_status\s*:\s*approved)\b", + re.IGNORECASE, +) +_EXTERNAL_NONE_RE = re.compile( + r"^\s*[-*]?\s*external[- ]state mutations\s*:\s*none\s*$", + re.IGNORECASE | re.MULTILINE, +) +_LOCK_MUTATION_RE = re.compile( + r"\b(?:gitea_lock_issue|issue-locks/|lock file edited)\b", + re.IGNORECASE, +) +_ISSUE_DONE_RE = re.compile( + r"\b(?:issue (?:done|closed)\b|current status\s*:\s*(?:done|closed)\b|" + r"current status\s*:\s*issue complete\b)", + re.IGNORECASE, +) +_PR_MERGE_PROOF_RE = re.compile( + r"\b(?:pr (?:merged|number)|merge result\s*:\s*merged|pr mutations\s*:\s*created)\b", + re.IGNORECASE, +) +_DISCUSSION_COMPLETE_RE = re.compile( + r"\b(?:discussion complete|discussion ready for issue creation)\b", + re.IGNORECASE, +) +_DISCUSSION_SUMMARY_PROOF_RE = re.compile( + r"\b(?:discussion summary|issues to create|issues created|linked issues)\b", + re.IGNORECASE, +) + +MIN_DISCUSSION_COMMENTS_DEFAULT = 5 + + +def _render_fields(fields: tuple[str, ...], values: dict[str, Any]) -> str: + lines = ["```text"] + for name in fields: + raw = values.get(name, values.get(name.lower(), "none")) + text = str(raw).strip() if raw is not None else "none" + lines.append(f"{name}: {text or 'none'}") + lines.append("```") + return "\n".join(lines) + + +def render_issue_state_comment(**values: Any) -> str: + """Render canonical issue state comment body.""" + return _render_fields(ISSUE_STATE_FIELDS, values) + + +def render_pr_state_comment(**values: Any) -> str: + """Render canonical PR state comment body.""" + return _render_fields(PR_STATE_FIELDS, values) + + +def render_discussion_state_comment(**values: Any) -> str: + """Render canonical discussion state comment body.""" + return _render_fields(DISCUSSION_STATE_FIELDS, values) + + +def render_discussion_summary_comment(**values: Any) -> str: + """Render discussion-to-issue conversion summary comment.""" + return _render_fields(DISCUSSION_SUMMARY_FIELDS, values) + + +def render_queue_controller_report(**values: Any) -> str: + """Render queue-controller selection report.""" + return _render_fields(QUEUE_CONTROLLER_FIELDS, values) + + +def parse_state_comment(text: str) -> dict[str, str]: + """Parse KEY: value lines from a canonical state comment.""" + parsed: dict[str, str] = {} + for match in _FIELD_LINE_RE.finditer(text or ""): + parsed[match.group(1)] = match.group(2).strip() + return parsed + + +def _handoff_field_map(report_text: str) -> dict[str, str]: + section_match = _HANDOFF_SECTION_RE.search(report_text or "") + if not section_match: + return {} + section = (report_text or "")[section_match.end():] + fields: dict[str, str] = {} + for line in section.splitlines(): + stripped = line.strip().lstrip("-*").strip() + if ":" not in stripped: + continue + key, value = stripped.split(":", 1) + fields[key.strip().lower()] = value.strip() + return fields + + +def assess_state_comment(text: str, *, kind: str) -> dict[str, Any]: + """Validate a canonical Gitea state comment for *kind*.""" + normalized = (kind or "").strip().lower() + if normalized not in STATE_COMMENT_KINDS: + return { + "complete": False, + "block": True, + "missing_fields": [], + "reasons": [f"unknown state comment kind '{kind}'"], + "safe_next_action": "use a supported state comment kind", + } + + required = _FIELDS_BY_KIND[normalized] + parsed = parse_state_comment(text) + missing = [name for name in required if name not in parsed or not parsed[name].strip()] + reasons = [f"state comment missing field: {name}" for name in missing] + + if normalized == "discussion_state": + urgency = parsed.get("URGENCY", "").strip().lower() + count_raw = parsed.get("COMMENT_COUNT", "").strip() + try: + count = int(count_raw) + except (TypeError, ValueError): + count = -1 + if ( + urgency not in {"urgent", "trivial"} + and count >= 0 + and count < MIN_DISCUSSION_COMMENTS_DEFAULT + ): + reasons.append( + f"discussion has {count} comments; need at least " + f"{MIN_DISCUSSION_COMMENTS_DEFAULT} substantive comments " + "or URGENCY: urgent|trivial" + ) + + block = bool(missing or reasons) + return { + "complete": not block, + "block": block, + "missing_fields": missing, + "parsed_fields": parsed, + "reasons": reasons, + "safe_next_action": ( + "fill all required state comment fields before posting" + if block + else "proceed" + ), + } + + +def assess_final_report_next_action_handoff(report_text: str) -> dict[str, Any]: + """Require current status plus next actor/action/prompt in Controller Handoff.""" + fields = _handoff_field_map(report_text) + if not fields: + return { + "complete": False, + "block": True, + "missing_fields": [name for name, _ in FINAL_REPORT_NEXT_ACTION_FIELDS], + "reasons": [f"final report has no section titled exactly '{HANDOFF_HEADING}'"], + "safe_next_action": f"add a complete {HANDOFF_HEADING} section", + } + + missing = [] + for name, aliases in FINAL_REPORT_NEXT_ACTION_FIELDS: + value = "" + for alias in aliases: + if alias in fields: + value = fields[alias] + break + if not value or value.lower() in {"none", "n/a", "not verified in this session", ""}: + missing.append(name) + + reasons = [f"handoff missing next-action field: {name}" for name in missing] + block = bool(missing) + return { + "complete": not block, + "block": block, + "missing_fields": missing, + "reasons": reasons, + "safe_next_action": ( + "add Current status, Next actor, Next action, and Next prompt to Controller Handoff" + if block + else "proceed" + ), + } + + +def assess_contradictory_state_handoff(report_text: str) -> dict[str, Any]: + """Fail closed on contradictory queue/state claims in final reports.""" + text = report_text or "" + fields = _handoff_field_map(text) + reasons: list[str] = [] + + review_decision = fields.get("review decision", fields.get("review status", "")) + has_approval = ( + review_decision.lower() in {"approve", "approved"} + or bool(_APPROVAL_PROOF_RE.search(text)) + ) + if _READY_TO_MERGE_RE.search(text) and not has_approval: + reasons.append( + "report claims ready to merge without approval-at-current-head proof" + ) + + lock_val = fields.get("external-state mutations", "") + mutation_lines = " ".join( + fields.get(key, "") + for key in ( + "issue mutations", + "mcp/gitea mutations", + "external-state mutations", + "claim/lock state", + ) + ) + if ( + (_EXTERNAL_NONE_RE.search(text) or lock_val.lower() == "none") + and _LOCK_MUTATION_RE.search(mutation_lines) + ): + reasons.append( + "report claims External-state mutations: none while lock " + "activity is documented in mutation fields" + ) + + if _ISSUE_DONE_RE.search(text) and not _PR_MERGE_PROOF_RE.search(text): + reasons.append( + "report claims issue done/complete without PR or merge proof" + ) + + if _DISCUSSION_COMPLETE_RE.search(text) and not _DISCUSSION_SUMMARY_PROOF_RE.search(text): + reasons.append( + "report claims discussion complete without summary or issue-creation proof" + ) + + review_status = fields.get("review status", fields.get("review decision", "")) + merge_ready = fields.get("merge ready", "") + if ( + merge_ready.lower() in {"yes", "true", "ready"} + and review_status.lower() not in {"approve", "approved"} + and not _APPROVAL_PROOF_RE.search(text) + ): + reasons.append( + "MERGE_READY or merge-ready claim without approved review status" + ) + + block = bool(reasons) + return { + "complete": not block, + "block": block, + "reasons": reasons, + "safe_next_action": ( + "correct contradictory state claims or add missing proof" + if block + else "proceed" + ), + } + + +def assess_state_handoff_ledger_report(report_text: str) -> dict[str, Any]: + """Composite #494 assessment for final reports.""" + next_action = assess_final_report_next_action_handoff(report_text) + contradictions = assess_contradictory_state_handoff(report_text) + reasons = list(next_action.get("reasons") or []) + list(contradictions.get("reasons") or []) + block = bool(next_action.get("block") or contradictions.get("block")) + return { + "complete": not block, + "block": block, + "next_action": next_action, + "contradictions": contradictions, + "reasons": reasons, + "safe_next_action": ( + next_action.get("safe_next_action") + if next_action.get("block") + else contradictions.get("safe_next_action") + if contradictions.get("block") + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_bootstrap_review_path_docs.py b/tests/test_bootstrap_review_path_docs.py new file mode 100644 index 0000000..71c5c14 --- /dev/null +++ b/tests/test_bootstrap_review_path_docs.py @@ -0,0 +1,81 @@ +"""Doc-contract checks for the bootstrap review path (#557). + +Self-hosted MCP workflow fixes can deadlock live review daemons. These tests +pin the narrow controller-authorized bootstrap path so it cannot silently +regress into a general gate bypass. +""" + +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +DOC = REPO_ROOT / "docs" / "bootstrap-review-path.md" +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +SKILL = REPO_ROOT / "skills" / "llm-project-workflow" / "SKILL.md" + + +def _normalize(text: str) -> str: + return " ".join(text.split()) + + +def test_bootstrap_doc_exists_and_names_authorization_marker(): + text = DOC.read_text(encoding="utf-8") + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + assert "bootstrap deadlock" in text.lower() or "Bootstrap deadlock" in text + + +def test_bootstrap_doc_requires_validation_and_audits(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "git diff --check", + "Duplicate-PR audit", + "Mutation ledger audit", + "Contamination audit", + "workflow-contaminated", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + + +def test_bootstrap_doc_lists_allowed_and_forbidden_actions(): + text = _normalize(DOC.read_text(encoding="utf-8")) + lower = text.lower() + for phrase in ( + "Allowed verification actions", + "Forbidden actions", + "project root", + "direct Python import", + "branches/", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + assert "curl" in lower + assert "raw" in lower + + +def test_bootstrap_doc_forbids_general_gate_weakening(): + text = _normalize(DOC.read_text(encoding="utf-8")).lower() + assert "never weakens normal" in text or "does not weaken normal" in text + assert "general bypass" in text or "general gate" in text + + +def test_bootstrap_doc_post_land_restart_and_verify(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "Restart MCP daemons", + "canonical tools", + "gitea_whoami", + ): + assert phrase in text, f"post-bootstrap guidance missing: {phrase!r}" + + +def test_runbook_links_bootstrap_path(): + text = _normalize(RUNBOOK.read_text(encoding="utf-8")) + assert "Bootstrap Review Path for self-hosted MCP fixes (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + + +def test_skill_links_bootstrap_path(): + text = _normalize(SKILL.read_text(encoding="utf-8")) + assert "Bootstrap Review Path (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BLOCKED + DIAGNOSE" in text or "BLOCKED" in text diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 3e93135..03b8a67 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -35,6 +35,9 @@ def _review_handoff(**overrides): "- External-state mutations: none", "- Read-only diagnostics: issue and PR metadata inspected", "- Current status: PR open", + "- Next actor: author", + "- Next action: address review feedback", + "- Next prompt: Fix PR #203 per reviewer request_changes and push updated head", "- Blockers: none", "- Next: await author", "- Safety: no self-review; no self-merge", @@ -92,6 +95,9 @@ def _reconcile_handoff(drop=(), **overrides): "- Read-only diagnostics: gitea_view_pr, gitea_view_issue", "- Reconciliation mutations: PR comment posted", "- Current status: reconciliation complete", + "- Next actor: reconciler", + "- Next action: close PR #99 after capability proof", + "- Next prompt: Reconcile already-landed PR #99 with prgs-reconciler profile", "- Blocker: missing gitea.pr.close capability", "- Safe next action: hand off to a profile with gitea.pr.close", "- No review/merge confirmation: confirmed", diff --git a/tests/test_master_parity_gate.py b/tests/test_master_parity_gate.py new file mode 100644 index 0000000..33086f0 --- /dev/null +++ b/tests/test_master_parity_gate.py @@ -0,0 +1,152 @@ +"""Tests for the master-parity staleness gate (#420). + +Covers the pure assessment logic, the mutation-block helper, the env escape +hatches, and the server-side wiring: reads are never blocked by staleness, a +mutation is refused when the on-disk master has advanced, and the read-only +gitea_assess_master_parity tool reports the stale state. +""" +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import master_parity_gate as mp # noqa: E402 + + +SHA_A = "a" * 40 +SHA_B = "b" * 40 + + +class TestAssessMasterParity(unittest.TestCase): + def test_in_parity_when_heads_match(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_A) + self.assertTrue(res["in_parity"]) + self.assertFalse(res["stale"]) + self.assertFalse(res["restart_required"]) + self.assertTrue(res["determinable"]) + + def test_stale_when_master_advanced(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + self.assertFalse(res["in_parity"]) + self.assertTrue(res["stale"]) + self.assertTrue(res["restart_required"]) + self.assertTrue(res["determinable"]) + self.assertTrue(any("restart" in r for r in res["reasons"])) + + def test_missing_startup_head_not_determinable(self): + res = mp.assess_master_parity({"startup_head": None}, SHA_B) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + self.assertTrue(res["in_parity"]) + + def test_missing_current_head_not_determinable(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, None) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + self.assertTrue(res["in_parity"]) + + def test_none_startup_baseline(self): + res = mp.assess_master_parity(None, SHA_A) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + + +class TestBlockReasonsAndReport(unittest.TestCase): + def test_stale_produces_block_reasons(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + self.assertTrue(mp.parity_block_reasons(res)) + + def test_in_parity_no_block_reasons(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_A) + self.assertEqual(mp.parity_block_reasons(res), []) + + def test_disable_env_suppresses_block(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + with patch.dict(os.environ, {mp.ENV_DISABLE: "1"}): + self.assertTrue(mp.gate_disabled()) + self.assertEqual(mp.parity_block_reasons(res), []) + + def test_report_shape_when_stale(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + report = mp.parity_report(res) + self.assertEqual(report["kind"], "server_stale") + self.assertTrue(report["restart_required"]) + self.assertEqual(report["startup_head"], SHA_A) + self.assertEqual(report["current_head"], SHA_B) + self.assertTrue(report["recovery"]) + + +class TestReadGitHead(unittest.TestCase): + def test_test_override_takes_precedence(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + self.assertEqual(mp.read_git_head("/nonexistent"), SHA_B) + + def test_blank_override_is_none(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: " "}): + self.assertIsNone(mp.read_git_head("/nonexistent")) + + def test_empty_root_is_none(self): + # No override set; empty root cannot resolve a HEAD. + env = {k: v for k, v in os.environ.items() + if k != mp.ENV_TEST_CURRENT_HEAD} + with patch.dict(os.environ, env, clear=True): + self.assertIsNone(mp.read_git_head("")) + + +class TestServerWiring(unittest.TestCase): + """Integration with the gate choke point in the server namespace.""" + + def setUp(self): + import mcp_server + self.srv = mcp_server + # Pin a known startup baseline so the current-HEAD override can diverge. + self._saved = self.srv._STARTUP_PARITY + self.srv._STARTUP_PARITY = {"root": self.srv.PROJECT_ROOT, + "startup_head": SHA_A} + + def tearDown(self): + self.srv._STARTUP_PARITY = self._saved + + def test_reads_not_blocked_when_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + self.assertEqual(self.srv._master_parity_block("gitea.read"), []) + + def test_mutation_blocked_when_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + reasons = self.srv._master_parity_block("gitea.pr.create") + self.assertTrue(reasons) + # The profile-operation choke point surfaces the same block. + self.assertTrue(self.srv._profile_operation_gate("gitea.pr.create")) + + def test_mutation_allowed_when_in_parity(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_A}): + self.assertEqual( + self.srv._master_parity_block("gitea.pr.create"), []) + + def test_disable_env_lets_mutation_through(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B, + mp.ENV_DISABLE: "1"}): + self.assertEqual( + self.srv._master_parity_block("gitea.pr.create"), []) + + def test_assess_tool_reports_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + out = self.srv.gitea_assess_master_parity(remote="prgs") + self.assertTrue(out["stale"]) + self.assertTrue(out["restart_required"]) + self.assertEqual(out["startup_head"], SHA_A) + self.assertEqual(out["current_head"], SHA_B) + self.assertIn("report", out) + + def test_assess_tool_reports_in_parity(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_A}): + out = self.srv.gitea_assess_master_parity(remote="prgs") + self.assertFalse(out["stale"]) + self.assertTrue(out["in_parity"]) + self.assertNotIn("report", out) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_menu_script.py b/tests/test_mcp_menu_script.py index a447648..b52ecad 100644 --- a/tests/test_mcp_menu_script.py +++ b/tests/test_mcp_menu_script.py @@ -105,6 +105,19 @@ class TestMcpMenuScript(unittest.TestCase): self.assertIn("./mcp-menu.sh", docs_text) self.assertIn("placeholder", docs_text.lower()) + def test_reviewer_skip_stale_request_changes_prompt_discoverable(self): + # #482: the skip-already-reviewed-stale-REQUEST_CHANGES reviewer prompt + # must be reachable from the reviewer menu and documented. + label = "Skip already-reviewed stale REQUEST_CHANGES PR and hand off to author" + reviewer_fn = self._extract_function("show_reviewer_prompts") + self.assertIn(label, reviewer_fn, "new reviewer prompt label must appear in the reviewer menu") + # The prompt must instruct: no duplicate terminal review mutation for a + # non-stale REQUEST_CHANGES head, and must carry the author handoff. + self.assertIn("do NOT submit another terminal review mutation", reviewer_fn) + self.assertIn("author-ready fix prompt", reviewer_fn) + docs_text = DOCS.read_text(encoding="utf-8") + self.assertIn("REQUEST_CHANGES", docs_text) + def _extract_function(self, name: str) -> str: marker = f"{name}() {{" start = self.content.index(marker) diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py index 779df64..e90d5ab 100644 --- a/tests/test_merged_cleanup_reconcile.py +++ b/tests/test_merged_cleanup_reconcile.py @@ -4,7 +4,7 @@ import os import sys import tempfile import unittest -from unittest.mock import patch +from unittest.mock import patch, MagicMock sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) @@ -154,6 +154,290 @@ class TestMergedCleanupReport(unittest.TestCase): finally: os.remove(lock_path) + @patch("subprocess.run") + def test_discover_local_worktrees(self, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + res = mcr.discover_local_worktrees("/repo/Gitea-Tools") + self.assertEqual(res, {"feat/issue-11-x": "/repo/Gitea-Tools/branches/custom-name"}) + + @patch("subprocess.run") + @patch("merged_cleanup_reconcile.read_local_worktree_state") + def test_build_report_discovers_non_canonical_worktree_by_branch(self, mock_state, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + mock_state.return_value = { + "exists": True, + "clean": True, + "current_branch": "feat/issue-11-x", + "head_sha": "cafebabe", + "dirty_files": [], + } + + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 11, + "title": "merged", + "body": "Closes #11", + "head": {"ref": "feat/issue-11-x"}, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-11-x": True}, + head_on_master={11: True}, + delete_capability_allowed=True, + ) + self.assertEqual(report["merged_pr_count"], 1) + entry = report["entries"][0] + local = entry["local_worktree"] + self.assertEqual(local["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") + self.assertEqual(local["match_type"], "branch") + + @patch("subprocess.run") + def test_remove_local_worktree_uses_discovered_path(self, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + # First call is discover_local_worktrees, second is git worktree remove + mock_run.side_effect = [ + MagicMock(returncode=0, stdout=mock_output), + MagicMock(returncode=0, stdout="", stderr=""), + ] + + with patch("os.path.isdir", return_value=True): + result = mcr.remove_local_worktree("/repo/Gitea-Tools", "feat/issue-11-x") + self.assertTrue(result["success"]) + self.assertTrue(result["performed"]) + self.assertEqual(result["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") + # Assert git worktree remove was called with the custom path + mock_run.assert_any_call( + ["git", "-C", "/repo/Gitea-Tools", "worktree", "remove", "/repo/Gitea-Tools/branches/custom-name"], + capture_output=True, + text=True, + check=False, + ) + + +class TestReviewerScratchCleanup(unittest.TestCase): + """#534: authorized cleanup path for reviewer scratch worktrees.""" + + def test_parse_reviewer_scratch_folder(self): + self.assertEqual(mcr.parse_reviewer_scratch_folder("review-pr512-submit"), 512) + self.assertEqual(mcr.parse_reviewer_scratch_folder("review-pr99"), 99) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("feat-issue-11-x")) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("review-feat-issue-11")) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("review-pr512-extra-tail")) + + def test_assess_safe_clean_detached_after_merge(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=False, + ) + self.assertTrue(assessment["safe_to_remove_worktree"]) + self.assertFalse(assessment["author_worktree_cleanup"]) + self.assertEqual( + assessment["recommended_action"], "remove_reviewer_scratch_worktree" + ) + self.assertEqual(assessment["worktree_kind"], "reviewer_scratch") + + def test_assess_blocks_dirty_scratch(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": False, + "current_branch": None, + "head_sha": "abc", + "dirty_files": ["foo.py"], + }, + active_reviewer_lease=False, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue( + any("tracked edits" in r for r in assessment["block_reasons"]) + ) + + def test_assess_blocks_active_lease(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=True, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue( + any("active reviewer lease" in r for r in assessment["block_reasons"]) + ) + + def test_assess_blocks_open_pr(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=False, + pr_closed=False, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=False, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue(any("still open" in r for r in assessment["block_reasons"])) + + @patch("subprocess.run") + @patch("merged_cleanup_reconcile.read_local_worktree_state") + def test_report_lists_scratch_separately_from_author(self, mock_state, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/feat-issue-11-x\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD deadbeef\n\n" + "worktree /repo/Gitea-Tools/branches/review-pr512-submit\n" + "HEAD abcdef0\n" + "detached\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + + def _state(path): + if "review-pr512" in path: + return { + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abcdef0", + "dirty_files": [], + } + return { + "exists": True, + "clean": True, + "current_branch": "feat/issue-11-x", + "head_sha": "deadbeef", + "dirty_files": [], + } + + mock_state.side_effect = _state + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 11, + "title": "merged author", + "body": "Closes #11", + "head": {"ref": "feat/issue-11-x"}, + "merged_at": "2026-07-06T12:00:00Z", + }, + { + "number": 512, + "title": "merged reviewed", + "body": "Closes #512", + "head": {"ref": "feat/issue-512-x"}, + "merged_at": "2026-07-06T12:00:00Z", + }, + ], + open_prs=[], + remote_branch_exists={ + "feat/issue-11-x": False, + "feat/issue-512-x": False, + }, + head_on_master={11: True, 512: True}, + delete_capability_allowed=True, + active_reviewer_leases={512: False}, + ) + self.assertEqual(report["merged_pr_count"], 2) + self.assertEqual(report["reviewer_scratch_count"], 1) + scratch = report["reviewer_scratch_entries"][0] + self.assertEqual(scratch["pr_number"], 512) + self.assertEqual(scratch["worktree_kind"], "reviewer_scratch") + self.assertFalse(scratch["author_worktree_cleanup"]) + self.assertTrue(scratch["safe_to_remove_worktree"]) + # Author entries must not swallow the scratch path as local_worktree. + author_paths = { + (e.get("local_worktree") or {}).get("worktree_path") + for e in report["entries"] + } + self.assertNotIn( + "/repo/Gitea-Tools/branches/review-pr512-submit", author_paths + ) + + @patch("subprocess.run") + def test_remove_reviewer_scratch_worktree_path_guard(self, mock_run): + mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + with patch("os.path.isdir", return_value=True): + bad = mcr.remove_reviewer_scratch_worktree( + "/repo/Gitea-Tools", + "/repo/Gitea-Tools/branches/feat-issue-11-x", + ) + self.assertFalse(bad["performed"]) + good = mcr.remove_reviewer_scratch_worktree( + "/repo/Gitea-Tools", + "/repo/Gitea-Tools/branches/review-pr512-submit", + ) + self.assertTrue(good["success"]) + self.assertTrue(good["performed"]) + self.assertFalse(good["author_worktree_cleanup"]) + mock_run.assert_any_call( + [ + "git", + "-C", + "/repo/Gitea-Tools", + "worktree", + "remove", + "/repo/Gitea-Tools/branches/review-pr512-submit", + ], + capture_output=True, + text=True, + check=False, + ) + if __name__ == "__main__": unittest.main() \ No newline at end of file diff --git a/tests/test_pr_queue_cleanup.py b/tests/test_pr_queue_cleanup.py index e26a026..caff6cc 100644 --- a/tests/test_pr_queue_cleanup.py +++ b/tests/test_pr_queue_cleanup.py @@ -265,5 +265,38 @@ class TestCleanupReportVerifier(unittest.TestCase): self.assertEqual(direct["proven"], wrapped["proven"]) +class TestCleanupEmptyQueue(unittest.TestCase): + def test_empty_queue_report_passes(self): + report = _clean_report( + selected="Selected PR: none", + decision="Review decision: comment", + next="Next suggested PR: approvals (queue empty)", + pagination="PR inventory pagination proof: inventory_complete=true, total_count 0", + ) + report += "\npr_inventory_trust_gate.status: trusted_empty" + result = assess_pr_queue_cleanup_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + def test_empty_queue_without_evidence_fails(self): + report = _clean_report( + selected="Selected PR: none", + ) + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("no valid empty-queue claim" in r for r in result["reasons"]), + result["reasons"] + ) + + def test_contradictory_selected_pr_fails(self): + report = _clean_report() + "\nSelected PR: none" + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("contains both a selected PR and a claim of none selected" in r for r in result["reasons"]), + result["reasons"] + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_premerge_baseline_proof.py b/tests/test_premerge_baseline_proof.py new file mode 100644 index 0000000..865f093 --- /dev/null +++ b/tests/test_premerge_baseline_proof.py @@ -0,0 +1,112 @@ +import unittest + +from premerge_baseline_proof import ( + CLEAN_PASS, + PREMERGE_BASELINE_PROVEN_FAILURE, + UNRESOLVED_REGRESSION_RISK, + assess_premerge_baseline_proof, +) +from final_report_validator import assess_final_report_validator + +_FIELDS = ( + "Pre-merge base commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Tested commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" +) + +# Current-master-only reproduction carries no pre-merge base proof (that is the +# whole defect #533 guards against): a command/exit/signature but no base commit. +_CURRENT_ONLY_FIELDS = ( + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" +) + + +class TestPremergeBaselineProof(unittest.TestCase): + def test_clean_pass_not_blocked(self): + result = assess_premerge_baseline_proof( + "Validation: full suite passed, exit status 0. Clean pass." + ) + self.assertFalse(result["block"]) + self.assertTrue(result["skipped"]) + self.assertEqual(result["label"], CLEAN_PASS) + + def test_nonzero_exit_not_claimed_baseline_not_blocked(self): + result = assess_premerge_baseline_proof( + "Full suite: 1 failed. Investigating the regression; not yet classified." + ) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], UNRESOLVED_REGRESSION_RISK) + + def test_current_master_only_reproduction_rejected(self): + report = ( + "Full suite: 1 failed. This is a pre-existing baseline failure — " + "reproduced on current master after the PR merged.\n" + _CURRENT_ONLY_FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertTrue(result["block"]) + self.assertTrue( + any("current-master reproduction only" in r for r in result["reasons"]) + ) + + def test_baseline_claim_missing_fields_blocked(self): + report = ( + "Full suite: 1 failed. This failure is baseline / pre-existing. " + "Verified against the pre-merge base commit." + ) + result = assess_premerge_baseline_proof(report) + self.assertTrue(result["block"]) + self.assertTrue( + any("missing required proof field" in r for r in result["reasons"]) + ) + + def test_premerge_base_proof_accepted(self): + report = ( + "Full suite: 1 failed. This is a pre-existing baseline failure, proven " + "on the PR pre-merge base commit.\n" + _FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertFalse(result["block"]) + self.assertTrue(result["proven"]) + self.assertEqual(result["label"], PREMERGE_BASELINE_PROVEN_FAILURE) + + def test_documented_known_failure_record_accepted(self): + report = ( + "Full suite: 1 failed. Baseline failure: cited known-failure record #529 " + "which predates the PR.\n" + _FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], PREMERGE_BASELINE_PROVEN_FAILURE) + + +class TestPremergeBaselineProofValidatorIntegration(unittest.TestCase): + def _has_rule(self, report): + result = assess_final_report_validator(report, "review_pr") + return any( + f["rule_id"] == "reviewer.premerge_baseline_proof" + for f in result["findings"] + ) + + def test_review_report_current_master_only_flagged(self): + report = ( + "## Reviewer final report\n" + "Full suite: 1 failed. Pre-existing baseline failure — reproduced on " + "post-merge master.\n" + _CURRENT_ONLY_FIELDS + ) + self.assertTrue(self._has_rule(report)) + + def test_review_report_premerge_proof_clean(self): + report = ( + "## Reviewer final report\n" + "Full suite: 1 failed. Pre-existing baseline failure proven on the " + "pre-merge base commit.\n" + _FIELDS + ) + self.assertFalse(self._has_rule(report)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_state_handoff_ledger.py b/tests/test_state_handoff_ledger.py new file mode 100644 index 0000000..08f5c5f --- /dev/null +++ b/tests/test_state_handoff_ledger.py @@ -0,0 +1,243 @@ +"""Tests for canonical state handoff ledger (#494).""" +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 state_handoff_ledger import ( # noqa: E402 + ISSUE_STATE_FIELDS, + assess_contradictory_state_handoff, + assess_final_report_next_action_handoff, + assess_state_comment, + assess_state_handoff_ledger_report, + parse_state_comment, + render_issue_state_comment, + render_pr_state_comment, + render_queue_controller_report, +) + + +def _work_issue_handoff(**overrides): + lines = [ + "## Controller Handoff", + "", + "- Task: work-issue", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: author", + "- Identity: jcwalker3 / prgs-author", + "- Selected issue: #494", + "- Current status: implementation complete; PR opened awaiting review", + "- Next actor: reviewer", + "- Next action: review PR at pinned head", + "- Next prompt: Review PR #500 for issue #494 at current head in reviewer worktree", + "- Safe next action: switch to reviewer session", + "- External-state mutations: none", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + needle = f"- {key}:" + if needle in text: + prefix = text.split(needle)[0] + rest = text.split(needle, 1)[1] + suffix = rest.split("\n", 1)[1] if "\n" in rest else "" + text = f"{prefix}{needle} {value}\n{suffix}".rstrip() + else: + text += f"\n- {key}: {value}" + return text + + +class TestStateCommentTemplates(unittest.TestCase): + def test_render_issue_state_includes_all_fields(self): + body = render_issue_state_comment( + STATE="issue_ready_for_author", + NEXT_ACTOR="author", + NEXT_ACTION="lock and implement", + NEXT_PROMPT="Find issue #494 and open a PR", + LAST_UPDATED_BY="jcwalker3", + ) + parsed = parse_state_comment(body) + for field in ISSUE_STATE_FIELDS: + self.assertIn(field, parsed) + + def test_render_pr_state_parses(self): + body = render_pr_state_comment( + STATE="needs_review", + NEXT_ACTOR="reviewer", + NEXT_ACTION="review at head", + NEXT_PROMPT="Review PR #500", + ISSUE="#494", + HEAD_SHA="a" * 40, + LAST_UPDATED_BY="sysadmin", + ) + parsed = parse_state_comment(body) + self.assertEqual(parsed["STATE"], "needs_review") + self.assertEqual(parsed["NEXT_ACTOR"], "reviewer") + + def test_queue_controller_template(self): + body = render_queue_controller_report( + QUEUE_STATE="open_prs_need_review", + SELECTED_OBJECT="PR #500", + SELECTED_OBJECT_TYPE="pr", + NEXT_ACTOR="reviewer", + NEXT_ACTION="start review session", + NEXT_PROMPT="Review PR #500", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(body, kind="queue_controller") + self.assertTrue(result["complete"]) + + +class TestStateCommentValidation(unittest.TestCase): + def test_complete_issue_state_comment(self): + body = render_issue_state_comment( + STATE="in_progress", + NEXT_ACTOR="author", + NEXT_ACTION="push branch", + NEXT_PROMPT="Continue issue #494", + STATUS="open", + RELATED_PRS="none", + BRANCH="feat/issue-494-state-handoff-ledger", + HEAD_SHA="b" * 40, + VALIDATION="pytest passed", + BLOCKERS="none", + DUPLICATES_OR_SUPERSEDES="none", + LAST_UPDATED_BY="jcwalker3", + ) + result = assess_state_comment(body, kind="issue_state") + self.assertTrue(result["complete"]) + self.assertFalse(result["block"]) + + def test_missing_fields_blocked(self): + body = "STATE: open\nNEXT_ACTOR: author" + result = assess_state_comment(body, kind="issue_state") + self.assertFalse(result["complete"]) + self.assertIn("NEXT_ACTION", result["missing_fields"]) + + def test_discussion_requires_five_comments_by_default(self): + body = render_issue_state_comment( + STATE="collecting_feedback", + NEXT_ACTOR="controller", + NEXT_ACTION="facilitate discussion", + NEXT_PROMPT="Add acceptance criteria", + STATUS="open", + RELATED_PRS="none", + BRANCH="none", + HEAD_SHA="none", + VALIDATION="none", + BLOCKERS="none", + DUPLICATES_OR_SUPERSEDES="none", + LAST_UPDATED_BY="controller", + ) + # Re-map to discussion fields manually for comment-count test + from state_handoff_ledger import render_discussion_state_comment + + disc = render_discussion_state_comment( + STATE="collecting_feedback", + NEXT_ACTOR="controller", + NEXT_ACTION="wait for comments", + NEXT_PROMPT="Continue discussion", + COMMENT_COUNT="2", + SUBSTANTIVE_COMMENTS="yes", + URGENCY="normal", + BLOCKERS="none", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(disc, kind="discussion_state") + self.assertFalse(result["complete"]) + self.assertTrue(any("5" in reason for reason in result["reasons"])) + + def test_discussion_urgent_exception(self): + from state_handoff_ledger import render_discussion_state_comment + + disc = render_discussion_state_comment( + STATE="ready_for_summary", + NEXT_ACTOR="controller", + NEXT_ACTION="summarize", + NEXT_PROMPT="Post summary", + COMMENT_COUNT="2", + SUBSTANTIVE_COMMENTS="yes", + URGENCY="urgent", + BLOCKERS="none", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(disc, kind="discussion_state") + self.assertTrue(result["complete"]) + + +class TestFinalReportNextAction(unittest.TestCase): + def test_complete_next_action_handoff(self): + report = _work_issue_handoff() + result = assess_final_report_next_action_handoff(report) + self.assertTrue(result["complete"]) + self.assertFalse(result["block"]) + + def test_missing_next_prompt_blocked(self): + report = _work_issue_handoff(**{"Next prompt": "none"}) + result = assess_final_report_next_action_handoff(report) + self.assertFalse(result["complete"]) + self.assertIn("Next prompt", result["missing_fields"]) + + def test_missing_handoff_section_blocked(self): + result = assess_final_report_next_action_handoff("no handoff here") + self.assertTrue(result["block"]) + + +class TestContradictoryState(unittest.TestCase): + def test_ready_to_merge_without_approval_blocked(self): + report = _work_issue_handoff( + **{ + "Current status": "PR ready to merge", + "Review decision": "not attempted", + } + ) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_ready_to_merge_with_approval_allowed(self): + report = _work_issue_handoff( + **{ + "Current status": "PR ready to merge", + "Review decision": "approve", + } + ) + result = assess_contradictory_state_handoff(report) + self.assertFalse(result["block"]) + + def test_issue_done_without_pr_proof_blocked(self): + report = _work_issue_handoff(**{"Current status": "issue complete"}) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_external_none_with_lock_activity_blocked(self): + report = _work_issue_handoff( + **{ + "External-state mutations": "none", + } + ) + report += "\n- Issue mutations: gitea_lock_issue adopted branch" + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_discussion_complete_without_summary_blocked(self): + report = _work_issue_handoff(**{"Current status": "discussion complete"}) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + +class TestFinalReportValidatorIntegration(unittest.TestCase): + def test_work_issue_validator_flags_missing_next_prompt(self): + report = _work_issue_handoff(**{"Next prompt": ""}) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("shared.state_handoff_next_action", rule_ids) + + def test_work_issue_validator_accepts_complete_handoff(self): + report = _work_issue_handoff() + result = assess_state_handoff_ledger_report(report) + self.assertTrue(result["complete"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/webui/lease_loader.py b/webui/lease_loader.py index 717feef..548aec3 100644 --- a/webui/lease_loader.py +++ b/webui/lease_loader.py @@ -11,7 +11,8 @@ from urllib.parse import urlparse from gitea_auth import api_fetch_page, get_auth_header, repo_api_url from issue_claim_heartbeat import build_claim_inventory -from merged_cleanup_reconcile import ISSUE_LOCK_FILE, read_issue_lock +from merged_cleanup_reconcile import read_issue_lock +from issue_lock_provenance import ISSUE_LOCK_FILE from webui.project_registry import ProjectRecord, load_registry from webui.queue_loader import _extract_linked_issue, _fetch_issues, _fetch_prs