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/issue-acceptance-gate.md b/docs/issue-acceptance-gate.md new file mode 100644 index 0000000..522825b --- /dev/null +++ b/docs/issue-acceptance-gate.md @@ -0,0 +1,52 @@ +# Controller Issue-Acceptance Gate + +A merged PR does not automatically prove an issue is fully satisfied. After +merge, a controller must audit the linked issue against its acceptance criteria +and post a durable handoff before the issue is treated as complete. + +## Workflow position + +1. Author implements the issue and opens a PR. +2. Reviewer reviews the PR. +3. Merger merges the approved PR. +4. **Controller performs issue-acceptance audit.** +5. Controller posts a `## Controller Issue Acceptance` comment with either: + - `STATE: accepted` and checked criteria, or + - a rejection path (`more-work-required`, `needs-tests`, `needs-docs`, etc.) + with `MISSING_WORK` and a paste-ready `NEXT_PROMPT`. + +Gitea may auto-close an issue via `Closes #N` in the PR body. That closure is +merge mechanics only. Controller acceptance is still required before any final +report or queue controller treats the issue as complete. + +## Template + +Use `issue_acceptance_gate.render_controller_acceptance_template()` or the +copy in +[`skills/llm-project-workflow/templates/controller-issue-acceptance.md`](../skills/llm-project-workflow/templates/controller-issue-acceptance.md). + +## Final-report rules + +Final reports must not claim `issue complete` solely because a PR merged. +Either: + +- include a valid `## Controller Issue Acceptance` block with + `STATE: accepted`, or +- explicitly state `controller acceptance pending` and identify the controller + as the next actor. + +`final_report_validator` enforces this through +`issue_acceptance_gate.validate_final_report_issue_acceptance()`. + +## Role boundaries + +- Authors must not mark their own issues accepted. +- Reviewers must not mark issue acceptance unless acting under controller + capability. +- Mergers merge PRs; they do not substitute for controller acceptance. + +## Related + +- #495 — canonical next-action comment templates +- #496 — fail-closed canonical comment validation before posting +- #303 — controller handoff schema for reconciliation workflows \ No newline at end of file diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 9f820ad..4115b83 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -410,6 +410,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. @@ -718,6 +740,27 @@ merger handoff. - **Prompt (normal):** `After verifying master contains the merge of PR #N using post-merge file-presence verification, close issue #M and delete the merged branch. Include verification details in the report.` - **Prompt (reconcile):** `Reconcile closed-not-merged PR #N by verifying if its content landed on master.` +### Post-merge merged cleanup ownership (#523) + +Post-merge **local worktree / remote branch cleanup** is **reconciler** work, not +author work. Do not switch from `prgs-reconciler` to `prgs-author` only to run +`gitea_reconcile_merged_cleanups`. + +- **Profile:** `prgs-reconciler` (task `reconcile_merged_cleanups` / + `reconciliation_cleanup`). +- **Namespace:** reconciler MCP server; stable control checkout is allowed for + this role (branches-only author guard does not apply). +- **Steps:** + 1. `gitea_whoami` + `gitea_resolve_task_capability(task="reconcile_merged_cleanups")`. + 2. Dry-run first: `gitea_reconcile_merged_cleanups(dry_run=True)`. + 3. Execute only after audit/authorization gates when remote branch delete or + worktree removal is required (`dry_run=False`, `execute_confirmed=True`, + and `gitea.branch.delete` when deleting remotes). +- **Fail closed:** unmerged/open heads, mismatched worktrees, and non-merged + closed PRs must not be cleaned. +- **Reports:** label cleanup actions as reconciler cleanup (not author mutation). +- **Prompt:** `As prgs-reconciler, dry-run then execute gitea_reconcile_merged_cleanups for recently merged PRs without switching to prgs-author.` + ### Stop on blocker - **Any profile.** If a required gate cannot be satisfied — identity @@ -931,6 +974,7 @@ When posting a Canonical Thread Handoff after a binding blocker: ## Related documents +- [`issue-acceptance-gate.md`](issue-acceptance-gate.md) — controller issue-acceptance audit after PR merge (#500). - [`../skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md) — portable cross-project LLM workflow skill. - [`gitea-execution-profiles.md`](gitea-execution-profiles.md) — the profile model. - [`gitea-dual-namespace-deployment.md`](gitea-dual-namespace-deployment.md) — static author/reviewer namespace deployment (#139 decision). @@ -940,6 +984,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/docs/webui-local-dev.md b/docs/webui-local-dev.md index 0a5b935..96225b6 100644 --- a/docs/webui-local-dev.md +++ b/docs/webui-local-dev.md @@ -46,7 +46,8 @@ Optional environment variables: | `/runtime` | Stub — MCP runtime health (#430) | | `/audit` | Stub — report audit paste (#431) | | `/worktrees` | Stub — hygiene dashboard (#432) | -| `/leases` | Stub — lease visibility (#433) | +| `/leases` | Lease and collision visibility (#433) | +| `/api/leases` | JSON lease/collision export | All routes are GET-only. POST/PUT/PATCH/DELETE return `405` with `read-only-mvp`. @@ -82,8 +83,16 @@ credentials. The UI surfaces pagination proof (returned count, pages fetched, If credentials are missing or the fetch fails, the page shows an explicit error instead of an empty queue (fail closed). +## Lease visibility (#433) + +`/leases` surfaces read-only lease and collision state: local issue lock file, +in-progress claim inventory (#268), reviewer PR lease comments when present +(``, #407), duplicate open PRs per issue (#400), +and duplicate local branches per issue. Links to collision-history backend +issues (#267, #268, #400, #407) are included. No lease acquire/release from UI. + ## Tests ```bash -pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py -q +pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py tests/test_webui_lease_visibility.py -q ``` \ No newline at end of file diff --git a/final_report_validator.py b/final_report_validator.py index dd499d5..d368f91 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -11,6 +11,7 @@ import inspect import re from typing import Any, Callable +import issue_acceptance_gate import issue_lock_provenance from mcp_native_cleanup_proof import assess_mcp_native_cleanup_proof from post_merge_cleanup_proof import assess_post_merge_cleanup_proof @@ -23,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({ @@ -334,6 +336,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"): @@ -714,6 +748,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, *, @@ -960,6 +1013,69 @@ def _rule_reconcile_linked_issue_live( return [] +_PR_CLOSE_NEGATIVE = frozenset({"", "none", "n/a", "na", "0", "no", "not closed", "not performed"}) +_ANCESTOR_AFFIRMATIVE_RE = re.compile( + r"ancestor|passed|true|verified|confirmed", re.IGNORECASE +) + + +def _reconciler_pr_close_performed(fields: dict[str, str], lock: dict) -> bool: + """True when the report/session indicates a reconciler PR close happened.""" + if lock.get("pr_closed") is True: + return True + value = (fields.get("prs closed", "") or "").strip().lower() + if value in _PR_CLOSE_NEGATIVE: + return False + # A closed PR is reported by number (e.g. "#99") or an affirmative result. + return bool(re.search(r"#\s*\d+|\b(?:closed|success|done)\b", value)) + + +def _rule_reconcile_close_proof( + report_text: str, + *, + reconciler_close_lock: dict | None = None, +) -> list[dict[str, str]]: + """#306: a reconciler PR close must carry exact proof fields. + + Read-only/comment-only reconciliations are untouched. Once a PR close is + reported (via the ``PRs closed`` field or a session close lock), the + handoff must prove the close capability, ancestor landing, PR close + result, and the linked-issue result — narrative alone fails closed. + """ + fields = _handoff_fields(report_text) + lock = reconciler_close_lock or {} + if not _reconciler_pr_close_performed(fields, lock): + return [] + + missing: list[str] = [] + capabilities = fields.get("capabilities proven", "") + if "gitea.pr.close" not in capabilities.lower(): + missing.append("close capability proof (gitea.pr.close)") + ancestor = fields.get("ancestor proof", "") + if not _ANCESTOR_AFFIRMATIVE_RE.search(ancestor): + missing.append("ancestor proof") + prs_closed = (fields.get("prs closed", "") or "").strip().lower() + if prs_closed in _PR_CLOSE_NEGATIVE and lock.get("pr_closed") is True: + missing.append("PR close result") + linked = fields.get("linked issue live status", "") or fields.get("issues closed", "") + if not linked.strip(): + missing.append("linked issue result") + + if not missing: + return [] + return [ + validator_finding( + "reconcile.close_proof_fields", + "block", + "Reconciler close proof", + "reconciler PR close reported without required proof field(s): " + + ", ".join(missing), + "include close capability proof, ancestor proof, PR close result, " + "and linked issue result in the handoff", + ) + ] + + def _rule_reconcile_pagination_proof(report_text: str) -> list[dict[str, str]]: text = report_text or "" if not _INVENTORY_COMPLETE_RE.search(text): @@ -1046,6 +1162,22 @@ def _rule_shared_manual_lock_pr_override(report_text: str) -> list[dict[str, str ) +def _rule_shared_issue_acceptance_gate(report_text: str) -> list[dict[str, str]]: + result = issue_acceptance_gate.validate_final_report_issue_acceptance(report_text) + if not result.get("applicable") or result.get("valid"): + return [] + return _findings_from_reasons( + "shared.issue_acceptance_gate", + result.get("reasons") or [], + field="Controller acceptance", + severity="block", + safe_next_action=( + "add Controller Issue Acceptance proof or state that controller " + "acceptance is pending; do not claim issue complete from PR merge alone" + ), + ) + + def _rule_shared_author_reviewer_same_run(report_text: str) -> list[dict[str, str]]: result = issue_lock_provenance.assess_author_reviewer_same_run_report(report_text) if result.get("proven"): @@ -1202,6 +1334,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, @@ -1214,6 +1347,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, @@ -1230,13 +1364,16 @@ _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, _rule_reconcile_stale_author_fields, _rule_reconcile_eligible_reviewed, _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, @@ -1244,31 +1381,37 @@ _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, _rule_reviewer_vague_mutations_none, _rule_conflict_fix_push_proof, _rule_worktree_cleanup_audit_proof, ], "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, ], @@ -1334,6 +1477,7 @@ def assess_final_report_validator( issue_filing_lock: dict | None = None, session_pr_opened: bool = False, validation_session: dict | None = None, + reconciler_close_lock: dict | None = None, ) -> dict[str, Any]: """Validate final-report text against task-specific proof rules (#327). @@ -1390,6 +1534,7 @@ def assess_final_report_validator( "local_edits": local_edits, "session_pr_opened": session_pr_opened, "validation_session": validation_session, + "reconciler_close_lock": reconciler_close_lock, } for rule in _RULES_BY_TASK.get(normalized_kind, ()): diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 0a5f812..9c84ead 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -696,6 +696,39 @@ def _verify_role_mutation_workspace( task: str | None = None, ) -> str: """Bind reviewer/merger mutations to the active namespace workspace (#510).""" + # Check running runtimes to prevent stale mutations + try: + if "PYTEST_CURRENT_TEST" not in os.environ or "GITEA_FORCE_MCP_RUNTIME_CHECK" in os.environ: + config = gitea_config.load_config() + required_permission = task_capability_map.required_permission(task) if task else None + matching_profiles = [] + if required_permission and config and "profiles" in config: + for p_name, p_data in config["profiles"].items(): + p_allowed = p_data.get("allowed_operations") or [] + p_forbidden = p_data.get("forbidden_operations") or [] + p_allowed_n = [] + for op in p_allowed: + try: + p_allowed_n.append(gitea_config.normalize_operation(op)) + except Exception: + pass + p_forbidden_n = [] + for op in p_forbidden: + try: + p_forbidden_n.append(gitea_config.normalize_operation(op)) + except Exception: + pass + ok, _ = gitea_config.check_operation(required_permission, p_allowed_n, p_forbidden_n) + if ok: + matching_profiles.append(p_name) + runtime_reasons = _check_mcp_runtimes_diagnostics(task or "unknown", matching_profiles) + if runtime_reasons: + raise RuntimeError("; ".join(runtime_reasons)) + except Exception as exc: + if "stale-runtime:" in str(exc): + raise RuntimeError(str(exc)) + pass + role = _effective_workspace_role() git_state = issue_lock_worktree.read_worktree_git_state( _resolve_preflight_workspace_path(worktree_path) @@ -771,6 +804,7 @@ import task_capability_map # noqa: E402 import review_proofs # noqa: E402 import review_workflow_boundary # noqa: E402 import review_workflow_load # noqa: E402 +import mcp_session_state # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import issue_lock_provenance # noqa: E402 @@ -794,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 @@ -2447,37 +2488,110 @@ _REVIEW_ACTIONS = { _TERMINAL_REVIEW_ACTIONS = frozenset({"approve", "request_changes"}) -# In-process only (#211): never persist to /tmp — host-global files are -# spoofable by any local process and go stale across sessions. +# Durable across MCP daemon process pools (#559), but never under /tmp (#211): +# host-global temp files are spoofable. Persistence uses the user-private +# cache directory from mcp_session_state (mode 0o700 / files 0o600), keyed by +# remote + profile identity with TTL. _REVIEW_DECISION_LOCK: dict | None = None +def _decision_lock_binding(lock: dict | None = None) -> dict: + """Resolve key fields for durable decision-lock storage.""" + profile = get_profile() + profile_name = (profile.get("profile_name") or "").strip() + env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() + stored_lock = ((lock or {}).get("session_profile_lock") or "").strip() + session_lock = env_lock or stored_lock or profile_name + remote = ((lock or {}).get("remote") or "").strip() or None + org = ((lock or {}).get("org") or (lock or {}).get("ready_org") or "").strip() or None + repo = ((lock or {}).get("repo") or (lock or {}).get("ready_repo") or "").strip() or None + return { + "session_profile": profile_name, + "session_profile_lock": session_lock, + "profile_identity": mcp_session_state.current_profile_identity( + profile_name=profile_name, + session_profile_lock=session_lock, + ), + "remote": remote, + "org": org, + "repo": repo, + } + + def _load_review_decision_lock(): + """Load decision lock from memory, falling back to durable session state.""" global _REVIEW_DECISION_LOCK + if _REVIEW_DECISION_LOCK is not None: + return _REVIEW_DECISION_LOCK + binding = _decision_lock_binding() + # Profile-keyed durable file; remote/org/repo validated from payload (#559). + durable = mcp_session_state.load_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity=binding.get("profile_identity"), + ) + if durable is not None: + _REVIEW_DECISION_LOCK = dict(durable) return _REVIEW_DECISION_LOCK def _save_review_decision_lock(data): + """Persist decision lock to memory + durable shared state (#559).""" global _REVIEW_DECISION_LOCK - _REVIEW_DECISION_LOCK = data + if data is None: + binding = _decision_lock_binding(_REVIEW_DECISION_LOCK) + mcp_session_state.clear_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity=binding.get("profile_identity"), + ) + _REVIEW_DECISION_LOCK = None + return + payload = dict(data) + binding = _decision_lock_binding(payload) + payload.setdefault("session_pid", os.getpid()) + payload["writer_pid"] = os.getpid() + payload["session_profile"] = binding["session_profile"] or payload.get( + "session_profile" + ) + payload["session_profile_lock"] = binding["session_profile_lock"] + payload["profile_identity"] = binding["profile_identity"] + if binding.get("remote") and not payload.get("remote"): + payload["remote"] = binding["remote"] + persisted = mcp_session_state.save_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + payload=payload, + remote=payload.get("remote"), + org=payload.get("org") or payload.get("ready_org"), + repo=payload.get("repo") or payload.get("ready_repo"), + profile_identity=payload.get("profile_identity"), + ) + _REVIEW_DECISION_LOCK = dict(persisted or payload) def _review_decision_session_reasons(lock: dict | None) -> list[str]: - """Reject locks that do not belong to this MCP process/session.""" + """Reject locks that do not belong to this MCP session identity (#559). + + Different daemon PIDs in the same IDE session pool are allowed when the + profile identity and remote match. Spoofed/stale locks still fail closed. + """ if lock is None: return [] reasons = [] - if lock.get("session_pid") != os.getpid(): - reasons.append( - "review decision lock was created in a different process " - "(fail closed)" - ) env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() - stored_lock = (lock.get("session_profile_lock") or "").strip() + stored_lock = (lock.get("session_profile_lock") or lock.get("profile_identity") or "").strip() if env_lock and stored_lock and env_lock != stored_lock: reasons.append( "review decision lock session profile lock mismatch (fail closed)" ) + # TTL / identity checks from durable envelope fields. + for reason in mcp_session_state.identity_match_reasons( + lock, + remote=lock.get("remote"), + org=lock.get("org") or lock.get("ready_org"), + repo=lock.get("repo") or lock.get("ready_repo"), + profile_identity=env_lock or stored_lock, + ): + if "profile identity mismatch" in reason or "expired" in reason or "future" in reason or "missing recorded_at" in reason: + reasons.append(reason) return reasons @@ -2488,11 +2602,12 @@ def init_review_decision_lock(remote: str | None, task: str | None, force: bool if not force: lock = _load_review_decision_lock() if lock is not None: - if lock.get("remote") == remote and lock.get("session_pid") == os.getpid(): - env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() - stored_lock = (lock.get("session_profile_lock") or "").strip() - if not env_lock or not stored_lock or env_lock == stored_lock: - return + env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() + stored_lock = (lock.get("session_profile_lock") or "").strip() + same_remote = lock.get("remote") == remote + same_profile = (not env_lock or not stored_lock or env_lock == stored_lock) + if same_remote and same_profile and not _review_decision_session_reasons(lock): + return review_workflow_load.clear_review_workflow_load() profile = get_profile() profile_name = (profile.get("profile_name") or "").strip() @@ -2507,6 +2622,10 @@ def init_review_decision_lock(remote: str | None, task: str | None, force: bool "session_pid": os.getpid(), "session_profile": profile_name, "session_profile_lock": session_lock, + "profile_identity": mcp_session_state.current_profile_identity( + profile_name=profile_name, + session_profile_lock=session_lock, + ), "final_review_decision_ready": False, "ready_pr_number": None, "ready_action": None, @@ -4625,6 +4744,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, @@ -4632,6 +4779,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: @@ -4675,6 +4824,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 @@ -5435,15 +5592,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: @@ -7547,6 +7730,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("/") @@ -7554,6 +7755,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, @@ -8601,6 +8839,102 @@ def gitea_route_task_session( ) +def _check_mcp_runtimes_diagnostics(task: str, matching_profiles: list[str]) -> list[str]: + """Check running runtimes and return errors if they are missing or stale.""" + import subprocess + import re + from datetime import datetime + + reasons = [] + code_path = os.path.join(PROJECT_ROOT, "gitea_mcp_server.py") + if not os.path.exists(code_path): + return reasons + + code_mtime = datetime.fromtimestamp(os.path.getmtime(code_path)) + + try: + proc = subprocess.run( + ["ps", "-o", "pid,lstart,command", "-ax"], + capture_output=True, text=True, check=True + ) + except Exception as exc: + return [f"stale-runtime: failed to list running processes: {exc}"] + + self_pid = os.getpid() + self_stale = False + + running_profiles = {} + for line in proc.stdout.splitlines()[1:]: + line = line.strip() + if not line or "mcp_server.py" not in line: + continue + + parts = line.split(None, 6) + if len(parts) < 7: + continue + pid_str = parts[0] + lstart_str = " ".join(parts[1:6]) + + try: + pid = int(pid_str) + start_time = datetime.strptime(lstart_str, "%a %b %d %H:%M:%S %Y") + except Exception: + continue + + try: + env_proc = subprocess.run( + ["ps", "eww", str(pid)], + capture_output=True, text=True, check=True + ) + env_out = env_proc.stdout + except Exception: + continue + + profile = "gitea-default" + match = re.search(r'\bGITEA_MCP_PROFILE=([^\s]+)', env_out) + if match: + profile = match.group(1) + + is_stale = start_time < code_mtime + if pid == self_pid and is_stale: + self_stale = True + + if profile not in running_profiles or start_time > running_profiles[profile]["start_time"]: + running_profiles[profile] = { + "pid": pid, + "start_time": start_time, + "is_stale": is_stale + } + + if self_stale: + reasons.append( + "stale-runtime: The active Gitea MCP server process is stale (running code from before changes were merged). " + "Please fully restart the Gitea MCP server (e.g. touch /Users/jasonwalker/.gemini/config/mcp_config.json) and retry." + ) + + if matching_profiles: + any_running = False + any_fresh = False + for mp in matching_profiles: + if mp in running_profiles: + any_running = True + if not running_profiles[mp]["is_stale"]: + any_fresh = True + break + if not any_running: + reasons.append( + f"stale-runtime: None of the matching profiles for task '{task}' ({matching_profiles}) are running in the OS. " + "Please restart the MCP server to ensure they are spawned." + ) + elif not any_fresh: + reasons.append( + f"stale-runtime: All matching profiles for task '{task}' ({matching_profiles}) are running but stale. " + "Please fully restart the Gitea MCP server and retry." + ) + + return reasons + + @mcp.tool() def gitea_resolve_task_capability( task: str, @@ -8711,6 +9045,16 @@ def gitea_resolve_task_capability( configured = len(matching_profiles) > 0 available_in_session = allowed_in_current_session + if "PYTEST_CURRENT_TEST" not in os.environ or "GITEA_FORCE_MCP_RUNTIME_CHECK" in os.environ: + runtime_reasons = _check_mcp_runtimes_diagnostics(task, matching_profiles) + if runtime_reasons: + restart_required = True + reason_msg = "; ".join(runtime_reasons) + next_safe_action = ( + "stale-runtime: Gitea MCP runtime conflict or missing process detected. " + "Please fully restart the Gitea MCP server and retry." + ) + if not allowed_in_current_session: if configured and switching: restart_required = True diff --git a/issue_acceptance_gate.py b/issue_acceptance_gate.py new file mode 100644 index 0000000..cd731c2 --- /dev/null +++ b/issue_acceptance_gate.py @@ -0,0 +1,300 @@ +"""Controller issue-acceptance gate helpers (#500). + +Pure validation for controller acceptance comments and final-report claims +that an issue is complete. Does not post comments or close issues. +""" + +from __future__ import annotations + +import re + +ACCEPTANCE_HEADING = "controller issue acceptance" + +REQUIRED_FIELDS = ( + "STATE", + "WHO_IS_NEXT", + "NEXT_ACTION", + "NEXT_PROMPT", + "ISSUE", + "MERGED_PR", + "MERGE_COMMIT", + "ACCEPTANCE_CRITERIA_CHECKED", + "VALIDATION_REVIEWED", + "CONTROLLER_DECISION", + "WHY", +) + +ACCEPTED_STATES = frozenset({"accepted"}) +REJECTION_STATES = frozenset({ + "more-work-required", + "more_work_required", + "needs-tests", + "needs_tests", + "needs-docs", + "needs_docs", + "needs-feature-enhancement", + "needs_feature_enhancement", + "needs-follow-up-issue", + "needs_follow_up_issue", + "blocked", +}) + +ALLOWED_NEXT_ACTORS = frozenset({ + "controller", + "author", + "reviewer", + "merger", + "reconciler", + "user", +}) + +_FIELD_RE = re.compile(r"^\s*(?:[-*]\s*)?([A-Z][A-Z0-9_ ]+)\s*:\s*(.*)$") +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) +_ISSUE_REF_RE = re.compile(r"#\d+") +_PR_REF_RE = re.compile(r"#\d+") +_CHECKED_ITEM_RE = re.compile(r"\[[xX]\]") +_UNCHECKED_ITEM_RE = re.compile(r"\[[\s]\]") + +_CLAIMS_ISSUE_COMPLETE_RE = re.compile( + r"\bissue\s+(?:is\s+)?(?:complete|completed|accepted|closed\s+as\s+complete|fully\s+satisfied)\b|" + r"\bissue\s+acceptance\s*:\s*accepted\b|" + r"\bcontroller\s+acceptance\s*:\s*(?:accepted|complete)\b", + re.IGNORECASE, +) +_MERGE_ONLY_COMPLETE_RE = re.compile( + r"(?:pr\s+merged|merged\s+pr|merge\s+result\s*:\s*merged).{0,120}" + r"(?:issue\s+(?:is\s+)?(?:complete|closed|accepted)|issue\s+complete)", + re.IGNORECASE | re.DOTALL, +) +_CLAIMS_CONTROLLER_ACCEPTANCE_RE = re.compile( + r"controller\s+issue\s+acceptance|controller\s+acceptance\s+(?:posted|complete|pending)", + re.IGNORECASE, +) +_PENDING_ACCEPTANCE_RE = re.compile( + r"controller\s+acceptance\s+(?:pending|required|not\s+(?:yet\s+)?(?:performed|complete))", + re.IGNORECASE, +) + + +def render_controller_acceptance_template() -> str: + """Return the canonical controller issue-acceptance comment template.""" + return """## Controller Issue Acceptance + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +ISSUE: +#... + +MERGED_PR: +#... + +MERGE_COMMIT: +<40-character SHA> + +ACCEPTANCE_CRITERIA_CHECKED: +- [x] ... +- [ ] ... + +VALIDATION_REVIEWED: + + +CONTROLLER_DECISION: + + +WHY: + + +MISSING_WORK: + + +FOLLOW_UP_ISSUES: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +""" + + +def extract_acceptance_fields(text: str | None) -> dict[str, str]: + """Return upper-case labeled fields from a controller acceptance block.""" + fields: dict[str, str] = {} + current_key: str | None = None + for line in (text or "").splitlines(): + match = _FIELD_RE.match(line) + if match: + current_key = match.group(1).strip().upper().replace(" ", "_") + fields[current_key] = match.group(2).strip() + continue + stripped = line.strip() + if current_key and stripped: + existing = fields.get(current_key, "") + fields[current_key] = ( + f"{existing}\n{stripped}" if existing else stripped + ) + return fields + + +def contains_acceptance_block(text: str | None) -> bool: + return ACCEPTANCE_HEADING in (text or "").lower() + + +def _empty_or_placeholder(value: str | None) -> bool: + value = (value or "").strip().lower() + return not value or value in {"none", "n/a", "unknown", "tbd", "<...>", "..."} + + +def _normalize_state(value: str | None) -> str: + return (value or "").strip().lower().replace(" ", "_").replace("-", "_") + + +def validate_controller_acceptance_comment(text: str | None) -> dict: + """Validate a controller issue-acceptance comment.""" + body = text or "" + if not contains_acceptance_block(body): + return { + "valid": False, + "fields": {}, + "reasons": ["missing Controller Issue Acceptance heading"], + } + + fields = extract_acceptance_fields(body) + reasons: list[str] = [] + + for field in REQUIRED_FIELDS: + if _empty_or_placeholder(fields.get(field)): + reasons.append(f"missing required controller acceptance field: {field}") + + state = _normalize_state(fields.get("STATE")) + if state and state not in ACCEPTED_STATES and state not in REJECTION_STATES: + reasons.append( + "STATE must be accepted or a rejection path " + "(more-work-required, needs-tests, needs-docs, " + "needs-feature-enhancement, needs-follow-up-issue, blocked)" + ) + + actor = (fields.get("WHO_IS_NEXT") or "").strip().lower() + if actor and actor not in ALLOWED_NEXT_ACTORS: + reasons.append( + "WHO_IS_NEXT must be one of: " + + ", ".join(sorted(ALLOWED_NEXT_ACTORS)) + ) + + if not _ISSUE_REF_RE.search(fields.get("ISSUE") or ""): + reasons.append("ISSUE must cite an issue number (#N)") + if not _PR_REF_RE.search(fields.get("MERGED_PR") or ""): + reasons.append("MERGED_PR must cite a merged PR number (#N)") + if not _FULL_SHA_RE.search(fields.get("MERGE_COMMIT") or ""): + reasons.append("MERGE_COMMIT must include a full 40-character SHA") + + criteria = fields.get("ACCEPTANCE_CRITERIA_CHECKED") or "" + if not _CHECKED_ITEM_RE.search(criteria) and not _UNCHECKED_ITEM_RE.search(criteria): + reasons.append( + "ACCEPTANCE_CRITERIA_CHECKED must list checked/unchecked criteria items" + ) + + decision = (fields.get("CONTROLLER_DECISION") or "").strip().lower() + if state in ACCEPTED_STATES: + if decision not in {"accepted", "accept"}: + reasons.append("accepted STATE requires CONTROLLER_DECISION: accepted") + if not _CHECKED_ITEM_RE.search(criteria): + reasons.append( + "accepted STATE requires at least one checked acceptance criterion" + ) + if _empty_or_placeholder(fields.get("WHY")): + reasons.append("accepted STATE requires WHY with acceptance rationale") + elif state in REJECTION_STATES: + if decision not in {"rejected", "reject", "more_work_required"}: + reasons.append( + "rejection STATE requires CONTROLLER_DECISION: rejected" + ) + if _empty_or_placeholder(fields.get("NEXT_PROMPT")): + reasons.append( + "rejection STATE requires a paste-ready NEXT_PROMPT for the next actor" + ) + missing = fields.get("MISSING_WORK") or "" + if _empty_or_placeholder(missing): + reasons.append( + "rejection STATE requires MISSING_WORK describing what is still needed" + ) + + return { + "valid": not reasons, + "fields": fields, + "reasons": reasons, + } + + +def claims_issue_complete(text: str | None) -> bool: + """Return True when text claims an issue is complete/accepted.""" + return bool(_CLAIMS_ISSUE_COMPLETE_RE.search(text or "")) + + +def claims_merge_only_issue_complete(text: str | None) -> bool: + """Return True when text treats PR merge as issue completion.""" + return bool(_MERGE_ONLY_COMPLETE_RE.search(text or "")) + + +def claims_controller_acceptance_update(text: str | None) -> bool: + return bool(_CLAIMS_CONTROLLER_ACCEPTANCE_RE.search(text or "")) + + +def notes_controller_acceptance_pending(text: str | None) -> bool: + return bool(_PENDING_ACCEPTANCE_RE.search(text or "")) + + +def validate_final_report_issue_acceptance(report_text: str | None) -> dict: + """Validate issue-completion and controller-acceptance claims in final reports.""" + text = report_text or "" + reasons: list[str] = [] + + complete_claim = claims_issue_complete(text) + merge_only = claims_merge_only_issue_complete(text) + acceptance_claim = claims_controller_acceptance_update(text) + pending_noted = notes_controller_acceptance_pending(text) + has_block = contains_acceptance_block(text) + + if merge_only and not (has_block and validate_controller_acceptance_comment(text)["valid"]): + reasons.append( + "final report treats PR merge as issue completion without controller acceptance proof" + ) + + if complete_claim and not pending_noted: + if not has_block: + reasons.append( + "final report claims issue complete but includes no Controller Issue Acceptance block" + ) + else: + result = validate_controller_acceptance_comment(text) + if not result["valid"]: + reasons.extend(result["reasons"]) + else: + state = _normalize_state(result["fields"].get("STATE")) + if state not in ACCEPTED_STATES: + reasons.append( + "final report claims issue complete but controller STATE is not accepted" + ) + + if acceptance_claim and has_block: + result = validate_controller_acceptance_comment(text) + if not result["valid"]: + reasons.extend(result["reasons"]) + + applicable = complete_claim or merge_only or acceptance_claim or pending_noted + return { + "applicable": applicable, + "valid": not reasons, + "reasons": reasons, + } \ No newline at end of file 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/mcp_session_state.py b/mcp_session_state.py new file mode 100644 index 0000000..3f93784 --- /dev/null +++ b/mcp_session_state.py @@ -0,0 +1,391 @@ +"""Durable MCP session validation state shared across daemon process pools (#559). + +The IDE often routes sequential MCP tool calls to different daemon processes. +Session-scoped proofs (workflow load, review decision lock) must therefore +survive process boundaries while remaining fail-closed against spoofing. + +Security notes (extends #211): +- Never store under host-global ``/tmp`` (world-writable, spoofable). +- Default root is ``~/.cache/gitea-tools/session-state`` (mode ``0o700``). +- Files are written atomically with mode ``0o600``. +- Records are keyed by remote + org + repo + profile identity, not by PID. +- TTL prevents indefinitely stale reuse across unrelated sessions. +""" + +from __future__ import annotations + +import fcntl +import json +import os +import re +import tempfile +from contextlib import contextmanager +from datetime import datetime, timedelta, timezone +from typing import Any + +STATE_DIR_ENV = "GITEA_MCP_SESSION_STATE_DIR" +DEFAULT_STATE_DIR = os.path.expanduser("~/.cache/gitea-tools/session-state") +TTL_HOURS_ENV = "GITEA_MCP_SESSION_STATE_TTL_HOURS" +DEFAULT_TTL_HOURS = 4.0 + +KIND_WORKFLOW_LOAD = "review_workflow_load" +KIND_DECISION_LOCK = "review_decision_lock" + +_SAFE_SEGMENT_RE = re.compile(r"[^A-Za-z0-9._+-]+") +SESSION_PROFILE_LOCK_ENV = "GITEA_SESSION_PROFILE_LOCK" + + +def default_state_dir() -> str: + raw = (os.environ.get(STATE_DIR_ENV) or DEFAULT_STATE_DIR).strip() + return raw or DEFAULT_STATE_DIR + + +def ttl_hours() -> float: + raw = (os.environ.get(TTL_HOURS_ENV) or "").strip() + if not raw: + return DEFAULT_TTL_HOURS + try: + value = float(raw) + except ValueError: + return DEFAULT_TTL_HOURS + return value if value > 0 else DEFAULT_TTL_HOURS + + +def _sanitize_segment(value: str) -> str: + text = (value or "").strip() + if not text: + return "_" + return _SAFE_SEGMENT_RE.sub("_", text) + + +def current_profile_identity( + profile_name: str | None = None, + session_profile_lock: str | None = None, + profile_identity: str | None = None, +) -> str: + """Resolve the session profile identity used as the durable key.""" + env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() + explicit = (profile_identity or session_profile_lock or "").strip() + lock = (explicit or env_lock or "").strip() + name = (profile_name or "").strip() + return lock or name or "unknown-profile" + + +def state_key( + *, + kind: str, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, +) -> str: + """Build durable filename key. + + Session proofs are one-active-per-profile (workflow load / decision lock), + so the primary key is kind + profile identity. Remote/org/repo are stored + inside the payload and validated on load (#559), which lets a later daemon + process recover state without already knowing the remote argument. + """ + # Keep remote/org/repo parameters for API stability / future kinds; they are + # intentionally not part of the filename for session-scoped proofs. + _ = (remote, org, repo) + return "-".join( + _sanitize_segment(part) + for part in ( + kind, + profile_identity or "unknown-profile", + ) + ) + + +def state_file_path( + *, + kind: str, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, + state_dir: str | None = None, +) -> str: + root = (state_dir or default_state_dir()).strip() + name = state_key( + kind=kind, + remote=remote, + org=org, + repo=repo, + profile_identity=profile_identity, + ) + return os.path.join(root, f"{name}.json") + + +def _ensure_state_dir(state_dir: str | None = None) -> str: + root = (state_dir or default_state_dir()).strip() + os.makedirs(root, mode=0o700, exist_ok=True) + return root + + +def _now_utc() -> datetime: + return datetime.now(timezone.utc) + + +def _parse_iso(value: str | None) -> datetime | None: + text = (value or "").strip() + if not text: + return None + if text.endswith("Z"): + text = text[:-1] + "+00:00" + try: + parsed = datetime.fromisoformat(text) + except ValueError: + return None + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +@contextmanager +def _exclusive_file_lock(lock_path: str): + os.makedirs(os.path.dirname(lock_path) or ".", exist_ok=True) + fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield fd + finally: + try: + fcntl.flock(fd, fcntl.LOCK_UN) + finally: + os.close(fd) + + +def _read_json(path: str) -> dict[str, Any] | None: + if not path or not os.path.exists(path): + return None + try: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError): + return None + return data if isinstance(data, dict) else None + + +def _write_json(path: str, data: dict[str, Any]) -> None: + parent = os.path.dirname(path) or "." + os.makedirs(parent, mode=0o700, exist_ok=True) + payload = json.dumps(data, indent=2, sort_keys=True) + "\n" + fd, temp_path = tempfile.mkstemp(prefix=".session-", suffix=".json", dir=parent) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(payload) + handle.flush() + os.fsync(handle.fileno()) + os.chmod(temp_path, 0o600) + os.replace(temp_path, path) + try: + os.chmod(path, 0o600) + except OSError: + pass + finally: + if os.path.exists(temp_path): + try: + os.remove(temp_path) + except OSError: + pass + + +def identity_match_reasons( + record: dict[str, Any] | None, + *, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, +) -> list[str]: + """Return fail-closed reasons when durable record identity does not match.""" + if record is None: + return [] + reasons: list[str] = [] + expected_profile = current_profile_identity(profile_identity=profile_identity) + stored_profile = ( + (record.get("session_profile_lock") or record.get("profile_identity") or "") + .strip() + ) + if stored_profile and expected_profile and stored_profile != expected_profile: + if expected_profile != "unknown-profile": + reasons.append( + "session state profile identity mismatch " + f"(stored={stored_profile!r}, active={expected_profile!r}; fail closed)" + ) + + for field, expected in ( + ("remote", remote), + ("org", org), + ("repo", repo), + ): + want = (expected or "").strip() + have = (str(record.get(field) or "")).strip() + if want and have and want != have: + reasons.append( + f"session state {field} mismatch " + f"(stored={have!r}, expected={want!r}; fail closed)" + ) + + recorded_at = _parse_iso(record.get("recorded_at") or record.get("updated_at")) + if recorded_at is None: + reasons.append("session state missing recorded_at timestamp (fail closed)") + else: + age = _now_utc() - recorded_at + if age > timedelta(hours=ttl_hours()): + reasons.append( + f"session state expired after {ttl_hours():g}h (fail closed)" + ) + if age < timedelta(0): + reasons.append("session state recorded_at is in the future (fail closed)") + return reasons + + +def load_state( + *, + kind: str, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, + state_dir: str | None = None, +) -> dict[str, Any] | None: + """Load durable state payload when identity checks pass.""" + profile = current_profile_identity(profile_identity=profile_identity) + path = state_file_path( + kind=kind, + remote=remote, + org=org, + repo=repo, + profile_identity=profile, + state_dir=state_dir, + ) + lock_path = f"{path}.lock" + with _exclusive_file_lock(lock_path): + envelope = _read_json(path) + if not envelope: + return None + payload = envelope.get("payload") + if not isinstance(payload, dict): + return None + # Identity fields live on both envelope and payload for convenience. + merged = dict(payload) + for key in ( + "kind", + "remote", + "org", + "repo", + "profile_identity", + "session_profile_lock", + "recorded_at", + "updated_at", + "writer_pid", + ): + if key in envelope and key not in merged: + merged[key] = envelope[key] + reasons = identity_match_reasons( + merged, + remote=remote, + org=org, + repo=repo, + profile_identity=profile, + ) + if reasons: + return None + return merged + + +def save_state( + *, + kind: str, + payload: dict[str, Any] | None, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, + state_dir: str | None = None, +) -> dict[str, Any] | None: + """Persist or clear durable state for the given session identity key.""" + profile = current_profile_identity( + profile_name=payload.get("session_profile") if payload else None, + session_profile_lock=( + (payload or {}).get("session_profile_lock") or profile_identity + ), + ) + # Prefer explicit args over payload fields for key location. + key_remote = remote if remote is not None else (payload or {}).get("remote") + key_org = org if org is not None else (payload or {}).get("org") + key_repo = repo if repo is not None else (payload or {}).get("repo") + + root = _ensure_state_dir(state_dir) + path = state_file_path( + kind=kind, + remote=key_remote, + org=key_org, + repo=key_repo, + profile_identity=profile, + state_dir=root, + ) + lock_path = f"{path}.lock" + with _exclusive_file_lock(lock_path): + if payload is None: + for candidate in (path, lock_path): + if os.path.exists(candidate): + try: + os.remove(candidate) + except OSError: + pass + return None + + now = _now_utc().isoformat().replace("+00:00", "Z") + body = dict(payload) + body.setdefault("session_pid", os.getpid()) + body["writer_pid"] = os.getpid() + body["profile_identity"] = profile + if not (body.get("session_profile_lock") or "").strip(): + body["session_profile_lock"] = profile + body["recorded_at"] = body.get("recorded_at") or now + body["updated_at"] = now + if key_remote is not None: + body["remote"] = key_remote + if key_org is not None: + body["org"] = key_org + if key_repo is not None: + body["repo"] = key_repo + + envelope = { + "kind": kind, + "remote": key_remote, + "org": key_org, + "repo": key_repo, + "profile_identity": profile, + "session_profile_lock": body.get("session_profile_lock"), + "recorded_at": body["recorded_at"], + "updated_at": body["updated_at"], + "writer_pid": body["writer_pid"], + "payload": body, + } + _write_json(path, envelope) + return dict(body) + + +def clear_state( + *, + kind: str, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, + profile_identity: str | None = None, + state_dir: str | None = None, +) -> None: + save_state( + kind=kind, + payload=None, + remote=remote, + org=org, + repo=repo, + profile_identity=profile_identity, + state_dir=state_dir, + ) 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/review_workflow_load.py b/review_workflow_load.py index 9cee3d0..8deb622 100644 --- a/review_workflow_load.py +++ b/review_workflow_load.py @@ -1,4 +1,4 @@ -"""Canonical review-merge workflow load proof for reviewer mutations (#389, #403).""" +"""Canonical review-merge workflow load proof for reviewer mutations (#389, #403, #559).""" from __future__ import annotations @@ -7,6 +7,7 @@ import os import re from pathlib import Path +import mcp_session_state import review_workflow_boundary as boundary WORKFLOW_REL_PATH = ( @@ -88,17 +89,79 @@ def assess_prompt_conflict(prompt_text: str | None) -> tuple[bool, list[str]]: return bool(reasons), reasons +def _session_binding_fields() -> dict: + """Capture profile identity used to share state across daemon processes.""" + env_lock = (os.environ.get(mcp_session_state.SESSION_PROFILE_LOCK_ENV) or "").strip() + profile_name = (os.environ.get("GITEA_MCP_PROFILE") or "").strip() + remote = (os.environ.get("GITEA_MCP_REMOTE") or "").strip() or None + identity = mcp_session_state.current_profile_identity( + profile_name=profile_name, + session_profile_lock=env_lock, + ) + return { + "session_profile": profile_name or identity, + "session_profile_lock": env_lock or identity, + "profile_identity": identity, + "remote": remote, + } + + +def _persist_workflow_load(record: dict | None) -> dict | None: + """Write durable workflow-load proof (or clear it).""" + binding = _session_binding_fields() + if record is None: + mcp_session_state.clear_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote=binding.get("remote"), + profile_identity=binding.get("profile_identity"), + ) + return None + payload = dict(record) + payload.update({ + k: v for k, v in binding.items() if v is not None + }) + return mcp_session_state.save_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + payload=payload, + remote=payload.get("remote"), + org=payload.get("org"), + repo=payload.get("repo"), + profile_identity=payload.get("profile_identity"), + ) + + +def _load_durable_workflow_load() -> dict | None: + binding = _session_binding_fields() + return mcp_session_state.load_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote=binding.get("remote"), + profile_identity=binding.get("profile_identity"), + ) + + +def _active_workflow_load() -> dict | None: + """Prefer in-process cache; fall back to durable shared state (#559).""" + global _REVIEW_WORKFLOW_LOAD + if _REVIEW_WORKFLOW_LOAD is not None: + return _REVIEW_WORKFLOW_LOAD + durable = _load_durable_workflow_load() + if durable is not None: + _REVIEW_WORKFLOW_LOAD = dict(durable) + return _REVIEW_WORKFLOW_LOAD + + def record_review_workflow_load( project_root: str, *, prompt_text: str | None = None, ) -> dict: - """Record in-process workflow load proof for the current MCP session.""" + """Record workflow load proof for the current MCP session (durable + memory).""" global _REVIEW_WORKFLOW_LOAD meta = build_canonical_workflow_metadata( project_root, prompt_text=prompt_text) boundary_state = boundary.assess_boundary_status(project_root) - _REVIEW_WORKFLOW_LOAD = { + binding = _session_binding_fields() + record = { **meta, "session_pid": os.getpid(), "loaded": True, @@ -107,7 +170,10 @@ def record_review_workflow_load( "pre_review_command_count": boundary_state.get("pre_review_command_count"), "boundary_violation_count": boundary_state.get("boundary_violation_count"), "boundary_reasons": list(boundary_state.get("reasons") or []), + **{k: v for k, v in binding.items() if v is not None}, } + persisted = _persist_workflow_load(record) + _REVIEW_WORKFLOW_LOAD = dict(persisted or record) return dict(_REVIEW_WORKFLOW_LOAD) @@ -115,12 +181,13 @@ def clear_review_workflow_load() -> None: """Test helper and review_pr session reset.""" global _REVIEW_WORKFLOW_LOAD _REVIEW_WORKFLOW_LOAD = None + _persist_workflow_load(None) boundary.clear_pre_review_commands() def workflow_load_status(project_root: str | None = None) -> dict: """Non-throwing status for capability/runtime reports.""" - load = _REVIEW_WORKFLOW_LOAD + load = _active_workflow_load() if load is None: return { "workflow_load_proof_present": False, @@ -148,6 +215,8 @@ def workflow_load_status(project_root: str | None = None) -> dict: "prompt_conflicts_with_workflow": load.get( "prompt_conflicts_with_workflow"), "session_pid": load.get("session_pid"), + "writer_pid": load.get("writer_pid"), + "profile_identity": load.get("profile_identity"), "boundary_status": load.get("boundary_status"), "boundary_clean": load.get("boundary_clean"), "workflow_load_helper_result": boundary.workflow_load_helper_result( @@ -160,13 +229,47 @@ def _session_validation_reasons( load: dict, project_root: str | None, ) -> list[str]: + """Validate durable/in-memory load proof for this session identity (#559).""" reasons: list[str] = [] - if load.get("session_pid") != os.getpid(): + # Cross-process daemon pools are allowed when profile identity matches. + # Reject only when the stored profile identity conflicts with this process. + binding = _session_binding_fields() + stored_identity = ( + load.get("session_profile_lock") + or load.get("profile_identity") + or "" + ).strip() + active_identity = (binding.get("profile_identity") or "").strip() + if ( + stored_identity + and active_identity + and stored_identity != active_identity + and active_identity != "unknown-profile" + and stored_identity != "unknown-profile" + ): reasons.append( - "workflow load proof was recorded in a different process " - "(fail closed)" + "workflow load proof profile identity mismatch " + f"(stored={stored_identity!r}, active={active_identity!r}; fail closed)" ) return reasons + + # Expired durable records are treated as absent. + identity_reasons = mcp_session_state.identity_match_reasons( + load, + remote=binding.get("remote") or load.get("remote"), + org=load.get("org"), + repo=load.get("repo"), + profile_identity=active_identity or stored_identity, + ) + # Filter out remote-mismatch noise when remote was not bound at load time. + for reason in identity_reasons: + if "missing recorded_at" in reason or "expired" in reason or "future" in reason: + reasons.append(reason) + elif "profile identity mismatch" in reason: + reasons.append(reason) + if reasons: + return reasons + if load.get("prompt_conflicts_with_workflow"): reasons.extend(load.get("prompt_conflict_reasons") or [ "active prompt conflicts with loaded review-merge workflow" @@ -196,7 +299,8 @@ def review_workflow_load_blockers( ) -> list[str]: """Reasons reviewer mutations must fail closed.""" boundary_reasons = boundary.boundary_blockers(project_root) - if boundary_reasons and _REVIEW_WORKFLOW_LOAD is None: + load = _active_workflow_load() + if boundary_reasons and load is None: return boundary_reasons status = workflow_load_status(project_root) if not status.get("workflow_load_proof_present"): @@ -214,4 +318,4 @@ def recovery_handoff_without_replay() -> list[str]: "Do not call gitea_submit_pr_review, gitea_mark_final_review_decision, " "or gitea_merge_pr until workflow-load proof is present.", "Do not include approve/merge replay commands in the recovery handoff.", - ] \ No newline at end of file + ] diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index d5aee2d..7c5729b 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -211,4 +211,16 @@ See [`templates/release-tag.md`](templates/release-tag.md) and - No code path in the canonical workflows allows continuation past a declared required step without the BLOCKED report. - See also: Global LLM Worktree Rule, Shell Spawn Hard-Stop Rule, Identity and profile safety, Subagent Tool-Budget Guardrails, and the explicit prohibition list in Universal rules. -Tests / proof docs updated in this change + runbooks. Full relevant test runs (see PR handoff) pass; `git diff --check` clean. Missing-step cases are now documented to fail closed before mutation. \ No newline at end of file +Tests / proof docs updated in this change + runbooks. Full relevant test runs (see PR handoff) pass; `git diff --check` clean. Missing-step cases are now documented to fail closed before mutation. + +## 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/controller-issue-acceptance.md b/skills/llm-project-workflow/templates/controller-issue-acceptance.md new file mode 100644 index 0000000..148ef30 --- /dev/null +++ b/skills/llm-project-workflow/templates/controller-issue-acceptance.md @@ -0,0 +1,68 @@ +# Controller issue-acceptance prompt + +Use after a PR merges when auditing whether the linked issue is truly complete. + +```text +Audit issue # against its acceptance criteria after merged PR #. +Post a Controller Issue Acceptance comment with checked criteria, validation +reviewed, controller decision, next actor, and paste-ready next prompt. +Do not mark the issue accepted unless every required criterion is satisfied. +``` + +## Comment template + +```text +## Controller Issue Acceptance + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +ISSUE: +#... + +MERGED_PR: +#... + +MERGE_COMMIT: +<40-character SHA> + +ACCEPTANCE_CRITERIA_CHECKED: +- [x] ... +- [ ] ... + +VALIDATION_REVIEWED: + + +CONTROLLER_DECISION: + + +WHY: + + +MISSING_WORK: + + +FOLLOW_UP_ISSUES: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +``` + +## Rejection paths + +When rejecting completion, `STATE` must name the gap (`needs-tests`, +`needs-docs`, `more-work-required`, etc.), `MISSING_WORK` must be explicit, +and `NEXT_PROMPT` must be ready for the next author session. \ No newline at end of file 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 b17381a..c7954a3 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). @@ -127,4 +128,14 @@ including the review/merge role fields: Selected PR, Reviewer eligibility, Pinned reviewed head, Review decision, Merge result, Linked issue status, Cleanup status. If you could not merge, name the exact gate. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). + +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/reconcile-landed-pr.md b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md index a9f4cd1..c58f3fe 100644 --- a/skills/llm-project-workflow/workflows/reconcile-landed-pr.md +++ b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md @@ -375,6 +375,24 @@ Include: * confirmation that no normal review, approval, request-changes, or merge was performed +## 18A. Reconciler close proof is enforced (#306) + +When a reconciler run closes a PR, the final-report validator +(`final_report_validator` rule `reconcile.close_proof_fields`) **blocks** the +handoff unless it carries all four close proofs. The prompt is guidance; the +MCP validator is the authority. + +A close is detected from `PRs closed: #` (or a session close lock). Once a +close is reported, the handoff must include: + +* `Capabilities proven:` naming `gitea.pr.close` — the exact close capability +* `Ancestor proof:` — the landed/ancestor proof for the closed PR +* `PRs closed:` — the PR close result (the closed PR number) +* `Linked issue live status:` (or `Issues closed:`) — the linked-issue result + +Comment-only and blocked reconciliations (no PR close) are unaffected: the rule +returns no finding when nothing was closed. + ## 19. Local artifact and report consistency rule Do not create local walkthrough, notes, markdown, JSON, or report artifacts diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 2dcc8d2..983c812 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -274,6 +274,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/task_capability_map.py b/task_capability_map.py index c86ce2a..8d3daf2 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -106,11 +106,11 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { }, "reconcile_merged_cleanups": { "permission": "gitea.read", - "role": "author", + "role": "reconciler", }, "reconciliation_cleanup": { "permission": "gitea.branch.delete", - "role": "author", + "role": "reconciler", }, "work_issue": { "permission": "gitea.pr.create", diff --git a/test_mcp_conn.py b/test_mcp_conn.py new file mode 100644 index 0000000..7481ce9 --- /dev/null +++ b/test_mcp_conn.py @@ -0,0 +1,129 @@ +#!/usr/bin/env python3 +"""Live health-check script to verify Gitea MCP namespace connections. + +Spawns the MCP server processes as defined in the IDE's global config, +performs the JSON-RPC handshake, and queries the tools list to verify +that the connection is fully operational and doesn't return EOF. +""" +import json +import os +import subprocess +import sys + +def test_connection(name, config): + print(f"Testing MCP connection for '{name}'...") + command = config.get("command") + args = config.get("args", []) + env = config.get("env", {}) + + # Merge current environment + run_env = os.environ.copy() + run_env.update(env) + + # Spawn subprocess + try: + proc = subprocess.Popen( + [command] + args, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + bufsize=1, + env=run_env + ) + except Exception as e: + print(f" [FAIL] Failed to spawn process: {e}") + return False + + # Send initialize request + init_req = { + "jsonrpc": "2.0", + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "healthcheck", "version": "1.0"} + }, + "id": 1 + } + + try: + proc.stdin.write(json.dumps(init_req) + "\n") + proc.stdin.flush() + + # Read response + line = proc.stdout.readline() + if not line: + stderr_content = proc.stderr.read() + print(f" [FAIL] Received EOF from process. Stderr:\n{stderr_content}") + proc.terminate() + return False + + print(f" [OK] Received initialize response: {line.strip()[:150]}...") + + # Send initialized notification + init_notif = { + "jsonrpc": "2.0", + "method": "notifications/initialized" + } + proc.stdin.write(json.dumps(init_notif) + "\n") + proc.stdin.flush() + + # Send tools/list request + list_req = { + "jsonrpc": "2.0", + "method": "tools/list", + "params": {}, + "id": 2 + } + proc.stdin.write(json.dumps(list_req) + "\n") + proc.stdin.flush() + + line = proc.stdout.readline() + if not line: + print(" [FAIL] Received EOF on tools/list request.") + proc.terminate() + return False + + res = json.loads(line) + if "error" in res: + print(f" [FAIL] Server returned error: {res['error']}") + proc.terminate() + return False + + tools = res.get("result", {}).get("tools", []) + tool_names = [t.get("name") for t in tools] + print(f" [OK] Successfully retrieved {len(tool_names)} tools: {tool_names[:5]}...") + + proc.terminate() + return True + except Exception as e: + print(f" [FAIL] Error during handshake: {e}") + proc.terminate() + return False + +def main(): + config_path = "/Users/jasonwalker/.gemini/config/mcp_config.json" + try: + with open(config_path) as f: + mcp_config = json.load(f) + except Exception as e: + print(f"Failed to load mcp_config.json: {e}") + sys.exit(1) + + servers = mcp_config.get("mcpServers", {}) + failed = False + for name in ["gitea-author", "gitea-reviewer"]: + if name in servers: + if not test_connection(name, servers[name]): + failed = True + else: + print(f"Server '{name}' not found in mcp_config.json") + + if failed: + sys.exit(1) + else: + print("All Gitea MCP connection tests passed!") + +if __name__ == "__main__": + main() diff --git a/tests/conftest.py b/tests/conftest.py index 7bbf78c..1c34986 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,14 +18,25 @@ def _reset_mutation_authority(monkeypatch): explicitly. """ monkeypatch.delenv("GITEA_SESSION_PROFILE_LOCK", raising=False) + # Isolate durable session-state files so tests never share host cache (#559). + import tempfile + + _state_tmp = tempfile.TemporaryDirectory(prefix="gitea-session-state-") + monkeypatch.setenv("GITEA_MCP_SESSION_STATE_DIR", _state_tmp.name) try: import mcp_server except Exception: + _state_tmp.cleanup() yield return monkeypatch.setattr(mcp_server, "_MUTATION_AUTHORITY", None) monkeypatch.setattr(mcp_server, "_IDENTITY_CACHE", {}) monkeypatch.setattr(mcp_server, "_REVIEW_DECISION_LOCK", None) + try: + import review_workflow_load + review_workflow_load._REVIEW_WORKFLOW_LOAD = None + except Exception: + pass try: import capability_stop_terminal capability_stop_terminal.clear() @@ -37,3 +48,13 @@ def _reset_mutation_authority(monkeypatch): capability_stop_terminal.clear() except Exception: pass + try: + import review_workflow_load + review_workflow_load._REVIEW_WORKFLOW_LOAD = None + except Exception: + pass + try: + mcp_server._REVIEW_DECISION_LOCK = None + except Exception: + pass + _state_tmp.cleanup() diff --git a/tests/test_audit_reconciliation_mode.py b/tests/test_audit_reconciliation_mode.py index a117fab..e429ddf 100644 --- a/tests/test_audit_reconciliation_mode.py +++ b/tests/test_audit_reconciliation_mode.py @@ -284,7 +284,7 @@ class TestTaskCapabilityMap(unittest.TestCase): required_permission("reconciliation_cleanup"), "gitea.branch.delete", ) - self.assertEqual(required_role("reconciliation_cleanup"), "author") + self.assertEqual(required_role("reconciliation_cleanup"), "reconciler") if __name__ == "__main__": 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 88c5b23..4701458 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", @@ -498,6 +504,81 @@ class TestCanonicalReconcileSchema(unittest.TestCase): ) +class TestReconcilerCloseProof(unittest.TestCase): + """Issue #306: a reconciler PR close must carry its proof fields.""" + + def _closed_report(self, **kwargs): + # A report that claims PR #99 was closed via the reconciler path. + report = ( + _reconcile_handoff(**kwargs) + .replace( + "- Capabilities proven: gitea.read, gitea.pr.comment", + "- Capabilities proven: gitea.read, gitea.pr.comment, gitea.pr.close", + ) + .replace("- Missing capabilities: gitea.pr.close", "- Missing capabilities: none") + .replace("- PRs closed: none", "- PRs closed: #99") + .replace( + "- Blocker: missing gitea.pr.close capability", "- Blocker: none" + ) + ) + return report + + def test_full_proof_close_passes(self): + result = assess_final_report_validator( + self._closed_report(), "reconcile_already_landed" + ) + self.assertEqual(result["grade"], "A") + self.assertFalse( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_capability_proof_blocks(self): + # Claims PRs closed: #99 but never proves gitea.pr.close capability. + report = _reconcile_handoff().replace("- PRs closed: none", "- PRs closed: #99") + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_ancestor_proof_blocks(self): + report = self._closed_report(drop=("Ancestor proof",)) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_linked_issue_result_blocks(self): + report = self._closed_report(drop=("Linked issue live status", "Issues closed")) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_lock_requires_proof_even_if_text_says_none(self): + # Session lock proves a PR was closed; the report omits close proof. + result = assess_final_report_validator( + _reconcile_handoff(), + "reconcile_already_landed", + reconciler_close_lock={"pr_closed": True}, + ) + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_comment_only_reconcile_needs_no_close_proof(self): + result = assess_final_report_validator( + _reconcile_handoff(), "reconcile_already_landed" + ) + self.assertEqual(result["grade"], "A") + self.assertFalse( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + class TestEntryPoint(unittest.TestCase): def test_unknown_task_kind_blocks(self): result = assess_final_report_validator("report", "unknown_mode") diff --git a/tests/test_issue_acceptance_gate.py b/tests/test_issue_acceptance_gate.py new file mode 100644 index 0000000..ead154d --- /dev/null +++ b/tests/test_issue_acceptance_gate.py @@ -0,0 +1,183 @@ +"""Tests for controller issue-acceptance gate (#500).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import issue_acceptance_gate # noqa: E402 +from final_report_validator import assess_final_report_validator # noqa: E402 + + +def _accepted_comment(**overrides): + lines = [ + "## Controller Issue Acceptance", + "", + "STATE:", + "accepted", + "", + "WHO_IS_NEXT:", + "controller", + "", + "NEXT_ACTION:", + "Close the tracker follow-up after verification.", + "", + "NEXT_PROMPT:", + "Verify deployment checklist for issue #500 and post acceptance.", + "", + "ISSUE:", + "#500", + "", + "MERGED_PR:", + "#503", + "", + "MERGE_COMMIT:", + "0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "", + "ACCEPTANCE_CRITERIA_CHECKED:", + "- [x] documentation added", + "- [x] validator tests added", + "", + "VALIDATION_REVIEWED:", + "pytest tests/test_issue_acceptance_gate.py -q", + "", + "CONTROLLER_DECISION:", + "accepted", + "", + "WHY:", + "All acceptance criteria satisfied with proof.", + "", + "MISSING_WORK:", + "none", + "", + "FOLLOW_UP_ISSUES:", + "none", + "", + "BLOCKERS:", + "none", + "", + "LAST_UPDATED_BY:", + "jcwalker3 / prgs-controller / 2026-07-08", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + text = text.replace(f"{key}:\n", f"{key}:\n{value}\n", 1) + return text + + +def _rejection_comment(state="needs-tests"): + text = _accepted_comment() + text = text.replace("STATE:\naccepted", f"STATE:\n{state}") + text = text.replace( + "CONTROLLER_DECISION:\naccepted", + "CONTROLLER_DECISION:\nrejected", + ) + text = text.replace( + "ACCEPTANCE_CRITERIA_CHECKED:\n- [x] documentation added\n- [x] validator tests added", + "ACCEPTANCE_CRITERIA_CHECKED:\n- [ ] regression tests for rejection paths", + ) + text = text.replace( + "MISSING_WORK:\nnone", + "MISSING_WORK:\nAdd regression tests for controller rejection paths.", + ) + text = text.replace( + "NEXT_PROMPT:\nVerify deployment checklist for issue #500 and post acceptance.", + "NEXT_PROMPT:\nImplement the missing tests for issue #500 and reopen the PR.", + ) + return text + + +class TestControllerAcceptanceComment(unittest.TestCase): + def test_accepted_comment_valid(self): + result = issue_acceptance_gate.validate_controller_acceptance_comment( + _accepted_comment() + ) + self.assertTrue(result["valid"], result["reasons"]) + + def test_missing_who_is_next_rejected(self): + text = _accepted_comment().replace("WHO_IS_NEXT:\ncontroller\n", "WHO_IS_NEXT:\n\n") + result = issue_acceptance_gate.validate_controller_acceptance_comment(text) + self.assertFalse(result["valid"]) + self.assertTrue(any("WHO_IS_NEXT" in r for r in result["reasons"])) + + def test_missing_next_prompt_on_rejection(self): + text = _rejection_comment() + text = text.replace( + "NEXT_PROMPT:\nImplement the missing tests for issue #500 and reopen the PR.\n", + "NEXT_PROMPT:\n\n", + ) + result = issue_acceptance_gate.validate_controller_acceptance_comment(text) + self.assertFalse(result["valid"]) + self.assertTrue(any("NEXT_PROMPT" in r for r in result["reasons"])) + + def test_vague_merge_only_completion_detected(self): + text = "PR merged. Issue complete." + self.assertTrue(issue_acceptance_gate.claims_merge_only_issue_complete(text)) + + def test_rejection_paths_require_missing_work(self): + for state in ( + "more-work-required", + "needs-tests", + "needs-docs", + "needs-feature-enhancement", + "needs-follow-up-issue", + ): + text = _rejection_comment(state=state).replace( + "MISSING_WORK:\nAdd regression tests for controller rejection paths.\n", + "MISSING_WORK:\n\n", + ) + result = issue_acceptance_gate.validate_controller_acceptance_comment(text) + self.assertFalse(result["valid"], state) + self.assertTrue(any("MISSING_WORK" in r for r in result["reasons"])) + + def test_accepted_requires_checked_criteria(self): + text = _accepted_comment().replace( + "ACCEPTANCE_CRITERIA_CHECKED:\n- [x] documentation added\n- [x] validator tests added\n", + "ACCEPTANCE_CRITERIA_CHECKED:\n- [ ] documentation added\n", + ) + result = issue_acceptance_gate.validate_controller_acceptance_comment(text) + self.assertFalse(result["valid"]) + self.assertTrue(any("checked acceptance criterion" in r for r in result["reasons"])) + + +class TestFinalReportIntegration(unittest.TestCase): + def test_merge_only_complete_blocks_work_issue_report(self): + report = ( + "## Controller Handoff\n\n" + "- Task: work issue #500\n" + "- Merge result: merged\n" + "- Current status: PR merged; issue complete\n" + ) + result = assess_final_report_validator(report, "work_issue") + self.assertTrue( + any( + f["rule_id"] == "shared.issue_acceptance_gate" + for f in result["findings"] + ), + result["findings"], + ) + + def test_pending_acceptance_allowed(self): + report = ( + "## Controller Handoff\n\n" + "- Task: work issue #500\n" + "- Merge result: merged\n" + "- Current status: controller acceptance pending\n" + ) + result = assess_final_report_validator(report, "work_issue") + self.assertFalse( + any( + f["rule_id"] == "shared.issue_acceptance_gate" + for f in result["findings"] + ), + result["findings"], + ) + + def test_accepted_block_allows_complete_claim(self): + report = _accepted_comment() + "\n\nIssue is complete." + gate = issue_acceptance_gate.validate_final_report_issue_acceptance(report) + self.assertTrue(gate["valid"], gate["reasons"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file 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_mcp_session_state.py b/tests/test_mcp_session_state.py new file mode 100644 index 0000000..ebfc0ce --- /dev/null +++ b/tests/test_mcp_session_state.py @@ -0,0 +1,207 @@ +"""Tests for durable MCP session state shared across daemon processes (#559).""" + +from __future__ import annotations + +import os +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +sys_path_root = str(Path(__file__).resolve().parent.parent) +import sys + +if sys_path_root not in sys.path: + sys.path.insert(0, sys_path_root) + +import mcp_session_state +import review_workflow_load +import mcp_server + + +class TestMcpSessionStateStore(unittest.TestCase): + def setUp(self): + self._tmpdir = tempfile.TemporaryDirectory() + self.state_dir = self._tmpdir.name + self._env = patch.dict( + os.environ, + { + mcp_session_state.STATE_DIR_ENV: self.state_dir, + mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-reviewer", + "GITEA_MCP_PROFILE": "prgs-reviewer", + }, + clear=False, + ) + self._env.start() + + def tearDown(self): + self._env.stop() + self._tmpdir.cleanup() + + def test_round_trip_same_identity(self): + saved = mcp_session_state.save_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + payload={"loaded": True, "workflow_hash": "abc123def456"}, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertIsNotNone(saved) + self.assertEqual(saved["workflow_hash"], "abc123def456") + self.assertIn("recorded_at", saved) + + loaded = mcp_session_state.load_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertIsNotNone(loaded) + self.assertEqual(loaded["workflow_hash"], "abc123def456") + self.assertEqual(loaded["profile_identity"], "prgs-reviewer") + + def test_profile_mismatch_returns_none(self): + mcp_session_state.save_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + payload={"final_review_decision_ready": True, "remote": "prgs"}, + remote="prgs", + ) + with patch.dict( + os.environ, + {mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-author"}, + clear=False, + ): + loaded = mcp_session_state.load_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + remote="prgs", + ) + self.assertIsNone(loaded) + + def test_clear_removes_file(self): + mcp_session_state.save_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + payload={"loaded": True}, + remote="prgs", + ) + path = mcp_session_state.state_file_path( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote="prgs", + profile_identity="prgs-reviewer", + state_dir=self.state_dir, + ) + self.assertTrue(os.path.exists(path)) + mcp_session_state.clear_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote="prgs", + profile_identity="prgs-reviewer", + ) + self.assertFalse(os.path.exists(path)) + + def test_files_are_private_mode(self): + mcp_session_state.save_state( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + payload={"loaded": True}, + remote="prgs", + ) + path = mcp_session_state.state_file_path( + kind=mcp_session_state.KIND_WORKFLOW_LOAD, + remote="prgs", + profile_identity="prgs-reviewer", + state_dir=self.state_dir, + ) + mode = os.stat(path).st_mode & 0o777 + self.assertEqual(mode, 0o600) + + +class TestWorkflowLoadCrossProcess(unittest.TestCase): + def setUp(self): + self._tmpdir = tempfile.TemporaryDirectory() + self._env = patch.dict( + os.environ, + { + mcp_session_state.STATE_DIR_ENV: self._tmpdir.name, + mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-reviewer", + "GITEA_MCP_PROFILE": "prgs-reviewer", + "GITEA_MCP_REMOTE": "prgs", + }, + clear=False, + ) + self._env.start() + review_workflow_load.clear_review_workflow_load() + + def tearDown(self): + review_workflow_load.clear_review_workflow_load() + self._env.stop() + self._tmpdir.cleanup() + + def test_different_pid_same_profile_accepted(self): + root = str(Path(__file__).resolve().parent.parent) + recorded = review_workflow_load.record_review_workflow_load(root) + self.assertTrue(recorded["loaded"]) + + # Simulate another daemon process: clear memory, keep durable file, + # and change apparent PID identity only (profile remains the same). + review_workflow_load._REVIEW_WORKFLOW_LOAD = None + status = review_workflow_load.workflow_load_status(root) + self.assertTrue(status["workflow_load_proof_present"]) + self.assertTrue( + status["workflow_load_valid"], + msg=status.get("reasons"), + ) + + def test_profile_mismatch_blocks(self): + root = str(Path(__file__).resolve().parent.parent) + review_workflow_load.record_review_workflow_load(root) + review_workflow_load._REVIEW_WORKFLOW_LOAD = None + with patch.dict( + os.environ, + {mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-author"}, + clear=False, + ): + # Durable load uses active identity; mismatched profile key misses. + status = review_workflow_load.workflow_load_status(root) + self.assertFalse(status["workflow_load_proof_present"]) + + +class TestDecisionLockCrossProcess(unittest.TestCase): + def setUp(self): + self._tmpdir = tempfile.TemporaryDirectory() + self._env = patch.dict( + os.environ, + { + mcp_session_state.STATE_DIR_ENV: self._tmpdir.name, + mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-reviewer", + }, + clear=False, + ) + self._env.start() + mcp_server._save_review_decision_lock(None) + review_workflow_load.clear_review_workflow_load() + + def tearDown(self): + mcp_server._save_review_decision_lock(None) + review_workflow_load.clear_review_workflow_load() + self._env.stop() + self._tmpdir.cleanup() + + def test_decision_lock_survives_memory_clear(self): + with patch.object(mcp_server, "get_profile", return_value={ + "profile_name": "prgs-reviewer", + }): + mcp_server.init_review_decision_lock("prgs", "review_pr", force=True) + lock = mcp_server._load_review_decision_lock() + self.assertIsNotNone(lock) + self.assertEqual(lock["remote"], "prgs") + self.assertFalse(lock["final_review_decision_ready"]) + + # New process: memory empty, durable state remains. + mcp_server._REVIEW_DECISION_LOCK = None + restored = mcp_server._load_review_decision_lock() + self.assertIsNotNone(restored) + self.assertEqual(restored["remote"], "prgs") + reasons = mcp_server._review_decision_session_reasons(restored) + self.assertEqual(reasons, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_stale_runtime.py b/tests/test_mcp_stale_runtime.py new file mode 100644 index 0000000..19310c2 --- /dev/null +++ b/tests/test_mcp_stale_runtime.py @@ -0,0 +1,71 @@ +import os +import unittest +from unittest.mock import patch, MagicMock +from datetime import datetime + +import gitea_mcp_server + +class TestMcpStaleRuntime(unittest.TestCase): + @patch("subprocess.run") + @patch("os.path.getmtime") + @patch("os.path.exists") + @patch("os.getpid") + @patch.dict("os.environ", {"GITEA_FORCE_MCP_RUNTIME_CHECK": "1"}) + def test_stale_and_missing_runtimes(self, mock_getpid, mock_exists, mock_getmtime, mock_run): + # Setup mocks + mock_getpid.return_value = 12345 + mock_exists.return_value = True + + # Code modification time: Jul 8 2026, 14:00:00 + code_time = datetime(2026, 7, 8, 14, 0, 0) + mock_getmtime.return_value = code_time.timestamp() + + # Mock ps -ax output + # PID 12345 is self (started at 13:00:00 - stale) + # PID 54321 is prgs-author (started at 15:00:00 - fresh) + # prgs-reviewer is missing + ps_output = ( + " PID LSTART COMMAND\n" + "12345 Wed Jul 8 13:00:00 2026 /path/to/python mcp_server.py\n" + "54321 Wed Jul 8 15:00:00 2026 /path/to/python mcp_server.py\n" + ) + + mock_run_ps = MagicMock() + mock_run_ps.stdout = ps_output + + # Mock env output for ps eww + mock_run_env12345 = MagicMock() + mock_run_env12345.stdout = "GITEA_MCP_PROFILE=prgs-reconciler" + + mock_run_env54321 = MagicMock() + mock_run_env54321.stdout = "GITEA_MCP_PROFILE=prgs-author" + + def side_effect(args, **kwargs): + if args[0] == "ps" and "eww" in args: + pid = args[2] + if pid == "12345": + return mock_run_env12345 + elif pid == "54321": + return mock_run_env54321 + elif args[0] == "ps": + return mock_run_ps + raise ValueError(f"Unexpected subprocess run args: {args}") + + mock_run.side_effect = side_effect + + # Test 1: required reviewer role matching prgs-reviewer (which is missing) + reasons = gitea_mcp_server._check_mcp_runtimes_diagnostics("review_pr", ["prgs-reviewer"]) + + self.assertTrue(any("stale-runtime: The active Gitea MCP server process is stale" in r for r in reasons)) + self.assertTrue(any("stale-runtime: None of the matching profiles for task" in r for r in reasons)) + + # Test 2: required author role matching prgs-author (which is fresh) + reasons_author = gitea_mcp_server._check_mcp_runtimes_diagnostics("create_issue", ["prgs-author"]) + + # Still contains self stale error + self.assertTrue(any("stale-runtime: The active Gitea MCP server process is stale" in r for r in reasons_author)) + # But does NOT contain missing author profile error + self.assertFalse(any("None of the matching profiles for task" in r for r in reasons_author)) + +if __name__ == "__main__": + unittest.main() 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_reconciler_cleanup_integration.py b/tests/test_reconciler_cleanup_integration.py new file mode 100644 index 0000000..7637602 --- /dev/null +++ b/tests/test_reconciler_cleanup_integration.py @@ -0,0 +1,263 @@ +"""Integration tests for reconciler merged cleanup (#523).""" +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import mcp_server +import task_capability_map +from audit_reconciliation_mode import clear_phase +from task_capability_map import required_role + +RECONCILER_PROFILE = { + "profile_name": "prgs-reconciler", + "context": "prgs", + "role": "reconciler", + "username": "sysadmin", + "base_url": "https://gitea.prgs.cc", + "allowed_operations": [ + "gitea.read", + "gitea.pr.close", + "gitea.pr.comment", + "gitea.issue.comment", + ], + "forbidden_operations": [], +} + +AUTHOR_PROFILE = { + "profile_name": "prgs-author", + "context": "prgs", + "role": "author", + "username": "jcwalker3", + "base_url": "https://gitea.prgs.cc", + "allowed_operations": [ + "gitea.read", + "gitea.pr.create", + "gitea.branch.create", + "gitea.branch.push", + "gitea.repo.commit", + ], + "forbidden_operations": [], +} + +CONTROL_ROOT = "/Users/jasonwalker/Development/Gitea-Tools" + + +class TestReconcilerCleanupIntegration(unittest.TestCase): + def setUp(self): + clear_phase() + mcp_server._preflight_whoami_called = False + mcp_server._preflight_capability_called = False + mcp_server._preflight_resolved_role = None + mcp_server._preflight_resolved_task = None + mcp_server._preflight_whoami_violation = False + mcp_server._preflight_capability_violation = False + mcp_server._preflight_whoami_violation_files = [] + mcp_server._preflight_capability_violation_files = [] + mcp_server._preflight_capability_baseline_porcelain = "" + self.mock_api = patch("gitea_auth.api_request").start() + self.mock_auth = patch( + "mcp_server.get_auth_header", return_value="token test" + ).start() + + def tearDown(self): + patch.stopall() + clear_phase() + mcp_server._IDENTITY_CACHE.clear() + + def test_capability_map_routes_merged_cleanup_to_reconciler(self): + self.assertEqual(required_role("reconcile_merged_cleanups"), "reconciler") + self.assertEqual(required_role("reconciliation_cleanup"), "reconciler") + self.assertEqual( + task_capability_map.required_permission("reconcile_merged_cleanups"), + "gitea.read", + ) + + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_reconciler_can_run_reconcile_merged_cleanups_directly(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) + + self.mock_api.side_effect = [ + [ # closed pulls page + { + "number": 100, + "title": "merged feature", + "body": "Closes #100", + "merged": True, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "deadbeef", + "head": {"ref": "feat/issue-100", "sha": "cafebabe"}, + }, + { + # closed but NOT merged — must not become a cleanup entry + "number": 101, + "title": "abandoned", + "body": "Closes #101", + "merged": False, + "merged_at": None, + "head": {"ref": "feat/issue-101", "sha": "badcafe"}, + }, + ], + [], # open pulls + ] + + with patch("mcp_server._remote_branch_exists", return_value=True): + with patch( + "mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", + return_value=True, + ): + result = mcp_server.gitea_reconcile_merged_cleanups( + dry_run=True, + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertTrue(result["dry_run"]) + entry_numbers = [e["issue_number"] for e in result["entries"]] + self.assertEqual(entry_numbers, [100]) + self.assertNotIn(101, entry_numbers) + + @patch.dict( + os.environ, + { + "GITEA_PROFILE_NAME": "prgs-reconciler", + # Force preflight purity to evaluate (disable test short-circuit). + "GITEA_TEST_PORCELAIN": "", + }, + clear=True, + ) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_reconciler_role_bypasses_branches_only_mutation_guard(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) + + with patch("mcp_server.PROJECT_ROOT", CONTROL_ROOT): + with patch( + "mcp_server.root_checkout_guard.resolve_remote_master_sha", + return_value="abc123", + ): + with patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={ + "current_branch": "master", + "head_sha": "abc123", + "porcelain_status": "", + }, + ): + try: + mcp_server.verify_preflight_purity( + remote="prgs", + task="reconcile_merged_cleanups", + ) + except RuntimeError as e: + self.fail( + "verify_preflight_purity raised RuntimeError " + f"unexpectedly for reconciler: {e}" + ) + + @patch.dict( + os.environ, + {"GITEA_PROFILE_NAME": "prgs-author", "GITEA_TEST_PORCELAIN": ""}, + clear=True, + ) + @patch("mcp_server.get_profile", return_value=AUTHOR_PROFILE) + def test_author_role_remains_blocked_on_root_checkout(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="author", + resolved_task="reconcile_merged_cleanups", + ) + + with patch("mcp_server.PROJECT_ROOT", CONTROL_ROOT): + with patch( + "mcp_server.root_checkout_guard.resolve_remote_master_sha", + return_value="abc123", + ): + with patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={ + "current_branch": "master", + "head_sha": "abc123", + "porcelain_status": "", + }, + ): + with self.assertRaises(RuntimeError) as ctx: + mcp_server.verify_preflight_purity( + remote="prgs", + task="reconcile_merged_cleanups", + ) + message = str(ctx.exception) + self.assertTrue( + "stable control checkout" in message + or "author mutation blocked" in message + or "branches/" in message, + msg=message, + ) + + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_open_unmerged_pr_heads_are_not_safe_cleanup_targets(self, _profile): + """AC5: active/unmerged author work must remain blocked from cleanup.""" + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) + + open_pr = { + "number": 200, + "title": "active work", + "body": "Closes #200", + "merged": False, + "state": "open", + "head": {"ref": "feat/issue-200", "sha": "livehead1"}, + } + # Same head branch still open on another PR while a closed/merged PR + # used the same branch name historically — open head must block delete. + closed_merged = { + "number": 199, + "title": "older merge same branch name", + "body": "Closes #199", + "merged": True, + "merged_at": "2026-07-01T12:00:00Z", + "merge_commit_sha": "deadbeef", + "head": {"ref": "feat/issue-200", "sha": "oldhead99"}, + } + self.mock_api.side_effect = [ + [closed_merged], # closed + [open_pr], # open + ] + + with patch("mcp_server._remote_branch_exists", return_value=True): + with patch( + "mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", + return_value=True, + ): + result = mcp_server.gitea_reconcile_merged_cleanups( + dry_run=True, + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertEqual(len(result["entries"]), 1) + remote_assessment = result["entries"][0].get("remote_branch") or {} + self.assertFalse( + remote_assessment.get("safe_to_delete_remote"), + msg=f"open head must block remote delete: {remote_assessment}", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_workflow_load.py b/tests/test_review_workflow_load.py index e59bf1a..8d1e42a 100644 --- a/tests/test_review_workflow_load.py +++ b/tests/test_review_workflow_load.py @@ -33,12 +33,38 @@ class TestReviewWorkflowLoadModule(unittest.TestCase): self.assertTrue(status["workflow_load_proof_present"]) self.assertTrue(status["workflow_load_valid"]) - def test_stale_session_pid_blocks(self): + def test_profile_identity_mismatch_blocks(self): + """#559: different daemon PIDs are OK; profile identity mismatch is not.""" + import tempfile + from unittest.mock import patch + + import mcp_session_state + root = str(__import__("pathlib").Path(__file__).resolve().parent.parent) - review_workflow_load.record_review_workflow_load(root) - review_workflow_load._REVIEW_WORKFLOW_LOAD["session_pid"] = 0 - blockers = review_workflow_load.review_workflow_load_blockers(root) - self.assertTrue(any("different process" in b for b in blockers)) + with tempfile.TemporaryDirectory() as tmp: + with patch.dict( + os.environ, + { + mcp_session_state.STATE_DIR_ENV: tmp, + mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-reviewer", + }, + clear=False, + ): + review_workflow_load.clear_review_workflow_load() + review_workflow_load.record_review_workflow_load(root) + # Corrupt the in-memory profile identity while keeping PID. + review_workflow_load._REVIEW_WORKFLOW_LOAD[ + "session_profile_lock" + ] = "other-profile" + review_workflow_load._REVIEW_WORKFLOW_LOAD[ + "profile_identity" + ] = "other-profile" + blockers = review_workflow_load.review_workflow_load_blockers(root) + self.assertTrue( + any("profile identity mismatch" in b for b in blockers), + msg=blockers, + ) + review_workflow_load.clear_review_workflow_load() def test_prompt_conflict_detected(self): conflict, reasons = review_workflow_load.assess_prompt_conflict( 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/tests/test_webui_lease_visibility.py b/tests/test_webui_lease_visibility.py new file mode 100644 index 0000000..a515311 --- /dev/null +++ b/tests/test_webui_lease_visibility.py @@ -0,0 +1,94 @@ +"""Tests for web UI lease visibility (#433).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from starlette.testclient import TestClient + +from webui.app import create_app +from webui.lease_loader import ( + _duplicate_branch_warnings, + _duplicate_pr_warnings, + load_lease_snapshot, + parse_reviewer_lease_comment, + snapshot_to_dict, +) + + +class TestLeaseLoader(unittest.TestCase): + def test_parse_reviewer_lease_comment(self): + body = ( + "\n" + "pr: #42\n" + "reviewer_identity: sysadmin\n" + "profile: prgs-reviewer\n" + "phase: validating\n" + "expires_at: 2026-07-07T20:00:00Z\n" + ) + parsed = parse_reviewer_lease_comment(body) + self.assertIsNotNone(parsed) + assert parsed is not None + self.assertEqual(parsed["pr_number"], 42) + self.assertEqual(parsed["phase"], "validating") + + def test_duplicate_pr_warnings(self): + raw_prs = [ + {"number": 1, "title": "Closes #99", "body": ""}, + {"number": 2, "title": "fixes #99", "body": ""}, + ] + warnings = _duplicate_pr_warnings(raw_prs) + self.assertEqual(len(warnings), 1) + self.assertEqual(warnings[0].issue_number, 99) + self.assertEqual(warnings[0].pr_numbers, (1, 2)) + + def test_duplicate_branch_warnings(self): + warnings = _duplicate_branch_warnings([ + "feat/issue-12-a", + "feat/issue-12-b", + "master", + ]) + self.assertEqual(len(warnings), 1) + self.assertEqual(warnings[0].issue_number, 12) + + def test_load_snapshot_with_injected_fetch(self): + def fetch_prs(_h, _o, _r, _a): + return ([], None) + + def fetch_issues(_h, _o, _r, _a): + return ([], None) + + snapshot = load_lease_snapshot( + fetch_prs=fetch_prs, + fetch_issues=fetch_issues, + fetch_comments=lambda *_a, **_k: [], + issue_lock_path=str(Path("/nonexistent/lock.json")), + ) + data = snapshot_to_dict(snapshot) + self.assertIn("claim_inventory", data) + self.assertIn("collision_history", data) + + +class TestLeaseRoutes(unittest.TestCase): + def setUp(self): + self.client = TestClient(create_app()) + + def test_leases_page_renders(self): + response = self.client.get("/leases") + self.assertEqual(response.status_code, 200) + self.assertIn("Leases", response.text) + self.assertIn("Collision warnings", response.text) + self.assertNotIn("child issue", response.text.lower()) + + def test_api_leases_json(self): + response = self.client.get("/api/leases") + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertIn("claim_inventory", data) + self.assertIn("duplicate_prs", data) + self.assertIn("reviewer_leases", data) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_webui_skeleton.py b/tests/test_webui_skeleton.py index 5dd23a1..953b368 100644 --- a/tests/test_webui_skeleton.py +++ b/tests/test_webui_skeleton.py @@ -46,10 +46,14 @@ class TestWebuiSkeleton(unittest.TestCase): self.assertEqual(response.status_code, 200) self.assertIn("Gitea-Tools", response.text) + def test_leases_is_implemented(self): + response = self.client.get("/leases") + self.assertEqual(response.status_code, 200) + self.assertIn("Leases", response.text) + self.assertIn("Collision warnings", response.text) + def test_extra_stub_routes(self): - for path in ("/worktrees", "/leases"): - with self.subTest(path=path): - self.assertEqual(self.client.get(path).status_code, 200) + self.assertEqual(self.client.get("/worktrees").status_code, 200) def test_post_is_rejected(self): response = self.client.post("/health") @@ -62,10 +66,10 @@ class TestWebuiSkeleton(unittest.TestCase): self.assertIn("Live queue", response.text) def test_nav_links_on_all_pages(self): - for path in ("/", "/queue", "/projects", "/prompts", "/runtime", "/audit"): + for path in ("/", "/queue", "/projects", "/prompts", "/runtime", "/audit", "/leases"): with self.subTest(path=path): text = self.client.get(path).text - for href in ("/queue", "/projects", "/prompts", "/runtime", "/audit"): + for href in ("/queue", "/projects", "/prompts", "/runtime", "/audit", "/leases"): self.assertIn(f'href="{href}"', text) diff --git a/webui/app.py b/webui/app.py index a97d1ad..4f4cb0f 100644 --- a/webui/app.py +++ b/webui/app.py @@ -14,6 +14,8 @@ from webui.project_registry import find_project, load_registry, registry_to_dict from webui.project_views import render_project_detail, render_projects_list from webui.prompt_library import find_prompt, library_to_dict from webui.prompt_views import render_prompt_detail, render_prompts_page +from webui.lease_loader import load_lease_snapshot, snapshot_to_dict as lease_snapshot_to_dict +from webui.lease_views import render_leases_page from webui.queue_loader import load_queue_snapshot, snapshot_to_dict from webui.queue_views import render_queue_page @@ -141,10 +143,12 @@ async def worktrees(_request: Request) -> HTMLResponse: async def leases(_request: Request) -> HTMLResponse: - return _stub_page( - "Leases", - "Lease visibility will show active issue and reviewer PR leases.", - ) + snapshot = load_lease_snapshot() + return HTMLResponse(render_leases_page(snapshot)) + + +async def api_leases(_request: Request) -> JSONResponse: + return JSONResponse(lease_snapshot_to_dict(load_lease_snapshot())) async def method_not_allowed(request: Request, _exc: Exception) -> Response: @@ -175,6 +179,7 @@ def create_app() -> Starlette: Route("/audit", audit, methods=["GET"]), Route("/worktrees", worktrees, methods=["GET"]), Route("/leases", leases, methods=["GET"]), + Route("/api/leases", api_leases, methods=["GET"]), ], exception_handlers={405: method_not_allowed}, ) \ No newline at end of file diff --git a/webui/lease_loader.py b/webui/lease_loader.py new file mode 100644 index 0000000..548aec3 --- /dev/null +++ b/webui/lease_loader.py @@ -0,0 +1,331 @@ +"""Lease and collision visibility for the web UI (#433).""" + +from __future__ import annotations + +import os +import re +import subprocess +from dataclasses import dataclass +from typing import Any, Callable +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 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 + +_REVIEWER_LEASE_MARKER = "" +_REVIEWER_FIELD_RE = re.compile( + r"^\s*([a-z_]+)\s*:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_ISSUE_BRANCH_RE = re.compile(r"issue-(\d+)", re.IGNORECASE) + +_COLLISION_HISTORY = ( + {"number": 267, "title": "Author work leases"}, + {"number": 268, "title": "Issue claim heartbeat leases"}, + {"number": 400, "title": "Early duplicate-work detection"}, + {"number": 407, "title": "Per-PR reviewer leases"}, +) + + +@dataclass(frozen=True) +class CollisionWarning: + kind: str + message: str + issue_number: int | None = None + pr_numbers: tuple[int, ...] = () + + +@dataclass(frozen=True) +class LeaseSnapshot: + project_id: str + repo_label: str + issue_lock: dict[str, Any] | None + claim_inventory: dict[str, Any] + reviewer_leases: tuple[dict[str, Any], ...] + duplicate_prs: tuple[CollisionWarning, ...] + duplicate_branches: tuple[CollisionWarning, ...] + collision_history: tuple[dict[str, Any], ...] + fetch_error: str | None = None + + +def _repo_root() -> str: + override = (os.environ.get("WEBUI_REPO_ROOT") or "").strip() + if override: + return os.path.realpath(override) + return os.path.realpath(os.path.join(os.path.dirname(__file__), "..")) + + +def _host_from_url(remote_host: str) -> str: + parsed = urlparse(remote_host.strip()) + return parsed.netloc or remote_host.strip().rstrip("/") + + +def _parse_pr_ref(value: str | None) -> int | None: + digits = re.sub(r"[^\d]", "", value or "") + return int(digits) if digits.isdigit() else None + + +def parse_reviewer_lease_comment(body: str) -> dict[str, Any] | None: + text = body or "" + if _REVIEWER_LEASE_MARKER not in text: + return None + fields: dict[str, str] = {} + for match in _REVIEWER_FIELD_RE.finditer(text): + fields[match.group(1).strip().lower()] = match.group(2).strip() + if not fields: + return None + return { + "pr_number": _parse_pr_ref(fields.get("pr")), + "issue_number": _parse_pr_ref(fields.get("issue")), + "reviewer_identity": fields.get("reviewer_identity"), + "profile": fields.get("profile"), + "phase": (fields.get("phase") or "").strip().lower() or None, + "expires_at": fields.get("expires_at"), + "blocker": fields.get("blocker"), + } + + +def _fetch_comments( + host: str, + org: str, + repo: str, + auth: str, + *, + issue_number: int, +) -> list[dict]: + url = f"{repo_api_url(host, org, repo)}/issues/{issue_number}/comments" + comments: list[dict] = [] + page = 1 + while page <= 10: + raw_page, meta = api_fetch_page(url, auth, page=page, limit=50) + comments.extend(raw_page) + if meta.get("is_final_page"): + break + page += 1 + return comments + + +def _list_local_branch_names(project_root: str) -> list[str]: + result = subprocess.run( + ["git", "-C", project_root, "branch", "--list", "--format=%(refname:short)"], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + return [] + return [line.strip() for line in (result.stdout or "").splitlines() if line.strip()] + + +def _duplicate_pr_warnings(raw_prs: list[dict]) -> list[CollisionWarning]: + issue_to_prs: dict[int, list[int]] = {} + for pr in raw_prs: + linked = _extract_linked_issue(pr.get("title"), pr.get("body")) + if linked is None: + continue + issue_to_prs.setdefault(linked, []).append(int(pr["number"])) + warnings: list[CollisionWarning] = [] + for issue_number, pr_numbers in sorted(issue_to_prs.items()): + if len(pr_numbers) > 1: + warnings.append( + CollisionWarning( + kind="duplicate-pr", + issue_number=issue_number, + pr_numbers=tuple(sorted(pr_numbers)), + message=( + f"Issue #{issue_number} has {len(pr_numbers)} open PRs: " + f"{', '.join(f'#{n}' for n in sorted(pr_numbers))} (#400)" + ), + ) + ) + return warnings + + +def _duplicate_branch_warnings(branch_names: list[str]) -> list[CollisionWarning]: + issue_to_branches: dict[int, list[str]] = {} + for name in branch_names: + match = _ISSUE_BRANCH_RE.search(name) + if not match: + continue + issue_num = int(match.group(1)) + issue_to_branches.setdefault(issue_num, []).append(name) + warnings: list[CollisionWarning] = [] + for issue_number, branches in sorted(issue_to_branches.items()): + if len(branches) > 1: + warnings.append( + CollisionWarning( + kind="duplicate-branch", + issue_number=issue_number, + message=( + f"Issue #{issue_number} has {len(branches)} local branches: " + f"{', '.join(branches)}" + ), + ) + ) + return warnings + + +def _extract_reviewer_leases( + raw_prs: list[dict], + *, + fetch_comments: Callable[[int], list[dict]], +) -> list[dict[str, Any]]: + leases: list[dict[str, Any]] = [] + for pr in raw_prs: + pr_number = int(pr["number"]) + for comment in fetch_comments(pr_number): + parsed = parse_reviewer_lease_comment(comment.get("body") or "") + if not parsed: + continue + leases.append( + { + **parsed, + "pr_number": parsed.get("pr_number") or pr_number, + "comment_id": comment.get("id"), + "author": (comment.get("user") or {}).get("login"), + "created_at": comment.get("created_at"), + } + ) + active_phases = {"claimed", "validating", "approved", "request-changes", "merging"} + return [lease for lease in leases if (lease.get("phase") or "") in active_phases] + + +def load_lease_snapshot( + *, + project_id: str | None = None, + project_root: str | None = None, + issue_lock_path: str | None = None, + fetch_prs: Callable | None = None, + fetch_issues: Callable | None = None, + fetch_comments: Callable[[str, str, str, str, int], list[dict]] | None = None, +) -> LeaseSnapshot: + registry = load_registry() + project: ProjectRecord | None = None + if project_id: + project = next((p for p in registry.projects if p.id == project_id), None) + else: + project = registry.projects[0] if registry.projects else None + + root = project_root or _repo_root() + lock = read_issue_lock(issue_lock_path if issue_lock_path is not None else ISSUE_LOCK_FILE) + + if project is None: + return LeaseSnapshot( + project_id=project_id or "", + repo_label="", + issue_lock=lock, + claim_inventory={"entries": [], "counts": {}}, + reviewer_leases=(), + duplicate_prs=(), + duplicate_branches=(), + collision_history=_COLLISION_HISTORY, + fetch_error="project not found in registry", + ) + + host = _host_from_url(project.remote_host) + pr_fetch = fetch_prs or _fetch_prs + issue_fetch = fetch_issues or _fetch_issues + comment_fetch = fetch_comments or _fetch_comments + using_live = fetch_prs is None or fetch_issues is None + auth = get_auth_header(host) if using_live else "test-auth" + + if using_live and not auth: + return LeaseSnapshot( + project_id=project.id, + repo_label=f"{project.gitea_owner}/{project.repo_name}", + issue_lock=lock, + claim_inventory={"entries": [], "counts": {}}, + reviewer_leases=(), + duplicate_prs=(), + duplicate_branches=_duplicate_branch_warnings(_list_local_branch_names(root)), + collision_history=_COLLISION_HISTORY, + fetch_error=( + f"Gitea credentials unavailable for {host}; " + "remote lease artifacts cannot be loaded" + ), + ) + + try: + raw_prs, _ = pr_fetch(host, project.gitea_owner, project.repo_name, auth) + raw_issues, _ = issue_fetch(host, project.gitea_owner, project.repo_name, auth) + except Exception as exc: # noqa: BLE001 + return LeaseSnapshot( + project_id=project.id, + repo_label=f"{project.gitea_owner}/{project.repo_name}", + issue_lock=lock, + claim_inventory={"entries": [], "counts": {}}, + reviewer_leases=(), + duplicate_prs=(), + duplicate_branches=_duplicate_branch_warnings(_list_local_branch_names(root)), + collision_history=_COLLISION_HISTORY, + fetch_error=f"Gitea fetch failed: {exc}", + ) + + comments_by_issue: dict[int, list[dict]] = {} + for issue in raw_issues: + if not any(lb.get("name") == "status:in-progress" for lb in issue.get("labels", [])): + continue + number = int(issue["number"]) + comments_by_issue[number] = comment_fetch( + host, project.gitea_owner, project.repo_name, auth, issue_number=number + ) + + branch_names = _list_local_branch_names(root) + inventory = build_claim_inventory( + issues=raw_issues, + comments_by_issue=comments_by_issue, + open_prs=raw_prs, + branch_names=branch_names, + ) + + def _pr_comments(pr_number: int) -> list[dict]: + return comment_fetch( + host, project.gitea_owner, project.repo_name, auth, issue_number=pr_number + ) + + reviewer_leases = tuple(_extract_reviewer_leases(raw_prs, fetch_comments=_pr_comments)) + + return LeaseSnapshot( + project_id=project.id, + repo_label=f"{project.gitea_owner}/{project.repo_name}", + issue_lock=lock, + claim_inventory=inventory, + reviewer_leases=reviewer_leases, + duplicate_prs=tuple(_duplicate_pr_warnings(raw_prs)), + duplicate_branches=tuple(_duplicate_branch_warnings(branch_names)), + collision_history=_COLLISION_HISTORY, + ) + + +def snapshot_to_dict(snapshot: LeaseSnapshot) -> dict[str, Any]: + return { + "project_id": snapshot.project_id, + "repo_label": snapshot.repo_label, + "issue_lock": snapshot.issue_lock, + "claim_inventory": snapshot.claim_inventory, + "reviewer_leases": list(snapshot.reviewer_leases), + "duplicate_prs": [ + { + "kind": w.kind, + "message": w.message, + "issue_number": w.issue_number, + "pr_numbers": list(w.pr_numbers), + } + for w in snapshot.duplicate_prs + ], + "duplicate_branches": [ + { + "kind": w.kind, + "message": w.message, + "issue_number": w.issue_number, + } + for w in snapshot.duplicate_branches + ], + "collision_history": list(snapshot.collision_history), + "fetch_error": snapshot.fetch_error, + } \ No newline at end of file diff --git a/webui/lease_views.py b/webui/lease_views.py new file mode 100644 index 0000000..a998dea --- /dev/null +++ b/webui/lease_views.py @@ -0,0 +1,130 @@ +"""HTML views for lease and collision visibility (#433).""" + +from __future__ import annotations + +import html +import json + +from webui.layout import render_page +from webui.lease_loader import LeaseSnapshot + + +def _escape(text: str) -> str: + return html.escape(text, quote=True) + + +def _warnings_block(snapshot: LeaseSnapshot) -> str: + warnings = list(snapshot.duplicate_prs) + list(snapshot.duplicate_branches) + if not warnings: + return "

No duplicate PR/branch collisions detected in current inventory.

" + items = "".join(f"
  • {_escape(w.message)}
    • " for w in warnings) + return f"
      {items}
    " + + +def _lock_block(snapshot: LeaseSnapshot) -> str: + lock = snapshot.issue_lock + if not lock: + return "

    No active local issue lock file.

    " + return ( + "
    "
+        f"{_escape(json.dumps(lock, indent=2, sort_keys=True))}"
+        "
    " + ) + + +def _claims_table(snapshot: LeaseSnapshot) -> str: + entries = snapshot.claim_inventory.get("entries") or [] + if not entries: + return "

    No in-progress issue claims in fetched inventory.

    " + rows = [] + for entry in entries: + rows.append( + "" + f"#{entry.get('issue_number')}" + f"" + f"{_escape(str(entry.get('status')))}" + f"{_escape(str(entry.get('linked_open_pr') or '—'))}" + f"{entry.get('heartbeat_count', 0)}" + f"{_escape(', '.join(entry.get('matching_branches') or []) or '—')}" + f"{_escape('; '.join(entry.get('reasons') or []))}" + "" + ) + return ( + "" + "" + "" + f"{''.join(rows)}
    IssueClaim statusOpen PRHeartbeatsBranchesNotes
    " + ) + + +def _reviewer_leases_table(snapshot: LeaseSnapshot) -> str: + if not snapshot.reviewer_leases: + return ( + "

    No active reviewer PR lease comments found " + "(marker <!-- mcp-review-lease:v1 -->; see #407).

    " + ) + rows = [] + for lease in snapshot.reviewer_leases: + rows.append( + "" + f"#{lease.get('pr_number')}" + f"{_escape(str(lease.get('reviewer_identity') or '—'))}" + f"{_escape(str(lease.get('profile') or '—'))}" + f"{_escape(str(lease.get('phase') or '—'))}" + f"{_escape(str(lease.get('expires_at') or '—'))}" + "" + ) + return ( + "" + "" + "" + f"{''.join(rows)}
    PRReviewerProfilePhaseExpires
    " + ) + + +def _history_links(snapshot: LeaseSnapshot) -> str: + items = "".join( + f"
  • #{item['number']} — {_escape(item['title'])}
    • " + for item in snapshot.collision_history + ) + return f"
      {items}
    " + + +LEASE_PAGE_STYLES = """ + +""" + + +def render_leases_page(snapshot: LeaseSnapshot) -> str: + error_block = "" + if snapshot.fetch_error: + error_block = ( + '

    Partial load: ' + f"{_escape(snapshot.fetch_error)}

    " + ) + body = ( + "

    Leases & collisions

    " + "

    Read-only visibility for issue claims, reviewer PR leases, and " + "duplicate-work risks. Does not acquire or release leases.

    " + f"{error_block}" + f"

    Project: {_escape(snapshot.repo_label)}

    " + "

    Collision warnings

    " + f"{_warnings_block(snapshot)}" + "

    Local issue lock

    " + f"{_lock_block(snapshot)}" + "

    In-progress issue claims (#268)

    " + f"{_claims_table(snapshot)}" + "

    Reviewer PR leases (#407)

    " + f"{_reviewer_leases_table(snapshot)}" + "

    Collision / lease history issues

    " + f"{_history_links(snapshot)}" + "

    JSON API

    " + f"{LEASE_PAGE_STYLES}" + ) + return render_page(title="Leases", body_html=body) \ No newline at end of file