From 3976f886e478a25853ebf22b2f1f1f7ffc2f470f Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 03:32:59 -0400 Subject: [PATCH 1/7] feat(skill): extract review-merge-pr workflow from monolithic SKILL.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #334 - Add workflows/review-merge-pr.md with reviewer-only gates and steps - Add schemas/review-merge-final-report.md with mutation ledger fields - Turn SKILL.md into task-mode router; replace inlined §F/§G with links - Point review/merge templates at extracted workflow - Add doc-contract tests for split structure Part of #333. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/llm-project-workflow/SKILL.md | 140 +++------- .../schemas/review-merge-final-report.md | 113 ++++++++ .../templates/merge-pr.md | 4 + .../templates/review-pr.md | 4 + .../workflows/review-merge-pr.md | 243 ++++++++++++++++++ tests/test_llm_workflow_split.py | 47 ++++ 6 files changed, 447 insertions(+), 104 deletions(-) create mode 100644 skills/llm-project-workflow/schemas/review-merge-final-report.md create mode 100644 skills/llm-project-workflow/workflows/review-merge-pr.md create mode 100644 tests/test_llm_workflow_split.py diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index b95f2a9..ed00208 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -17,6 +17,30 @@ The core promise: **an LLM never does unsafe or untracked work.** Every change is tracked by an issue, isolated in its own worktree, reviewed by a different identity, and cleaned up only after a real merge. +## Task mode router (#333) + +**Identify task mode before any mutation.** Load the matching workflow file and +final-report schema; do not apply rules from other modes. + +| Task mode | Workflow | Final report schema | +|-----------|----------|---------------------| +| `review-merge-pr` | [`workflows/review-merge-pr.md`](workflows/review-merge-pr.md) | [`schemas/review-merge-final-report.md`](schemas/review-merge-final-report.md) | +| `create-issue` | `workflows/create-issue.md` (planned — #333) | `schemas/create-issue-final-report.md` (planned) | +| `work-issue` | `workflows/work-issue.md` (planned — #333) | `schemas/work-issue-final-report.md` (planned) | + +Mode isolation: + +- **review-merge-pr** — reviewer namespace only; no issue creation, no code edits, no approve/merge replay after terminal review +- **create-issue** — author namespace; duplicate search + `create_issue` capability; no PR review/merge +- **work-issue** — author namespace; implement, validate, commit, PR; no approve/merge + +Shared gate references (all modes): `gates/identity-capability-rules.md`, +`gates/mutation-ledger-rules.md`, `gates/proof-wording-rules.md` (planned — #333). + +Until `create-issue.md` and `work-issue.md` land, author rules remain in this +file (§A, §E) and review rules live in +[`workflows/review-merge-pr.md`](workflows/review-merge-pr.md). + --- @@ -283,110 +307,16 @@ Worktree folder = branch with `/` replaced by `-` ## F. Review workflow -1. Use a separate review worktree (`scripts/worktree-review `), detached. -2. Verify your authenticated identity. -3. Verify the PR author — **you must not be the author.** Self-review - contamination must be *evidence-backed* (#173): state the authenticated - reviewer identity, the PR author identity, whether this session - authored/touched the PR branch, and the evidence source for any - "same-session author" claim. If the evidence is missing, report the - status as **unknown** — never declare contamination by assumption — and - choose another PR or stop (`review_proofs.assess_self_review_contamination`). -4. Verify the worktree is clean. -5. **Checkout proof (#173):** before reviewing or validating, prove and - state: the selected PR head SHA from Gitea (pinned), the local checkout - SHA (`git rev-parse HEAD`), that `HEAD ==` the pinned PR head SHA, and - that the diff base is the PR base branch. If `HEAD` does not match the - pinned head, **stop before review/merge** - (`review_proofs.verify_pinned_head_checkout`). -6. **Inventory proof (#173 + repo disambiguation hardening):** a blind queue - review must prove listing completeness before claiming "only PRs found". - Use repo-name disambiguation: - - "Gitea-Tools" / "gitea tool" / "MCP Gitea tool" / "gitea MCP tool" / - "gitea-tools repo" resolve **only** to `Scaled-Tech-Consulting/Gitea-Tools`. - - "mcp-control-plane" resolves only to `Scaled-Tech-Consulting/mcp-control-plane`. - - Ambiguous ("open PRs", no explicit repo, "MCP Gitea tooling") → inventory - **both** configured repos. - Report must state exactly which repo(s) were checked. If only one checked: - "Only was checked. Other configured repos were not checked. This is - not a complete queue inventory." Never let a single-repo zero hide PRs in - the other. - Both configured repos must be reported with state filter, pagination proof, - and open-PR count (`review_proofs.assess_inventory_completeness` and - `resolve_repos_from_user_reference`). Before inventory, reconcile the - operator-supplied PR backlog against the target repo - (`review_proofs.reconcile_queue_target`); never report `trusted_empty` - for one repo while ignoring contradictory supplied PR numbers in another. -7. **Role-boundary proof (#175):** a reviewer queue task must not silently - become author implementation. If no eligible PR exists, stop with the - queue report. Do not claim issues, create branches, commit, push, or open - PRs unless the operator explicitly retasks the run as author work. Mixed - reviewer+author namespace use must be reported with a justification, and - scratch-only notes are not durable evidence unless posted or committed - intentionally (`review_proofs.assess_role_boundary`). -8. Inspect the full diff; confirm scope matches the linked issue; flag unrelated files. -9. Run the tests. Validation reporting must include the exact command and - exact results: pass/fail, counts of tests passed/skipped/failed, any - ignored paths and why they are safe to ignore, and whether the command - differs from the repository's canonical validation command. Only claim a - validation result after the command has completed and its output has - been read (`review_proofs.assess_validation_report`). - During validation, review work is **read-only**: use - `gitea_dry_run_pr_review` to prove submission mechanics — never post live - APPROVE, REQUEST_CHANGES, or review comments to probe tool paths. After - validation completes, call `gitea_mark_final_review_decision`, then submit - exactly one live review via - `gitea_submit_pr_review(..., final_review_decision_ready=True)`. - After submitting, re-read `gitea_get_pr_review_feedback` and confirm the - verdict is visible (`approval_visible` true for APPROVE; PENDING drafts do - not count — #244). Do not merge until a visible APPROVED review exists. - Final reports must list exactly one review mutation - (`review_proofs.assess_review_mutation_final_report`) unless an - operator-approved correction flow was invoked and explained. -10. **Do not merge if checks fail. Do not merge if the reviewer is the author.** -11. **#179 A-bar proofs** (all fail closed when missing — - `review_proofs.assess_capability_evidence`, `assess_sweep_evidence`, - `assess_live_state_recheck`, `assess_role_boundary`): - - Capability claims must cite exact `gitea_resolve_task_capability` - output (or runtime context); a bare "capability checks passed" is - downgraded. - - The secret/provenance sweep must state the exact command/script/ - pattern/named method and the scope scanned. - - Immediately before submitting a review verdict (and again before any - merge), re-read live PR state and prove: still open, live head == - pinned head, base unchanged, no unresolved blocking review state. - - Reviewer runs stay in the reviewer namespace; any author-namespace - call requires an explicit justification in the report. -12. The final report must distinguish (`review_proofs.build_final_report`): - identity eligible; PR author different from reviewer; session - contamination absent (with evidence); validation performed on the pinned - head; capability evidence; sweep verdict; live-state recheck; role - boundary; merge performed; issue status verified. If any proof is - missing, stop or downgrade the result instead of merging confidently. +Moved to [`workflows/review-merge-pr.md`](workflows/review-merge-pr.md) (#334). +Load that file for PR inventory, validation, review mutation, merge preflight, +cleanup, and reviewer hard walls. Final report: +[`schemas/review-merge-final-report.md`](schemas/review-merge-final-report.md). ## G. Merge / cleanup workflow -Only an eligible (non-author) reviewer merges. Before merging: always verify -the authenticated identity **and** the PR author; cite exact capability -evidence for merge_pr (#179); respect runtime profile gates; run independent -validation (do not trust the author's reported results); perform the **final -live-state recheck** (#179 — PR still open, live head == pinned head, base -unchanged, no unresolved blocking review state) immediately before the merge -mutation; and merge with a **pinned head SHA** and, where supported, the -**expected changed-file set**, so a moved head or widened diff refuses the -merge. After a real merge: - -1. Confirm remote `master` actually contains the merge commit or expected squashed changes via post-merge file-presence verification (A PR is not done just because `master` moved or is marked "closed". Verify that expected files added/modified in the PR are actually present on `master` using `git pull`, `git log --oneline -- `, or `git merge-base --is-ancestor`; linked issues are closed; `status:in-progress` is removed). -2. Close/release the issue. -3. Whenever an issue is closed, check for `status:in-progress`: remove it, or report why it could not be removed. -4. Do not delete the remote source branch until: PR `merged=true`, or reconciliation confirms content is safely landed, or the issue owner explicitly abandons the work. -5. Remove the local branch. -6. Remove the branch worktree folder (`scripts/worktree-clean --delete-branch `). Branches/worktrees are cleaned only after the above is verified. -7. Fetch/prune. -8. Confirm the main checkout is clean and current (`0 0` vs remote). -9. Final merge/reconciliation reports must include: PR metadata (state, merged flag, merge commit/hash), Git content (remote master hash, expected content present or not), and the exact post-merge verification method used & results. - -Never run cleanup before the merge is confirmed on remote `master`. +Moved to [`workflows/review-merge-pr.md`](workflows/review-merge-pr.md) §6–§7 +and [`templates/worktree-cleanup.md`](templates/worktree-cleanup.md). Load the +workflow file before any merge mutation. ## H. Fail-closed cases @@ -479,9 +409,11 @@ Compact format (default, canonical field set per issue #182): Role-specific fields (append to the compact block): -- review/merge tasks: `Selected PR:`, `Reviewer eligibility:`, - `Pinned reviewed head:`, `Review decision:`, `Merge result:`, - `Linked issue status:`, `Cleanup status:` +- review/merge tasks: use + [`schemas/review-merge-final-report.md`](schemas/review-merge-final-report.md) + (includes `Selected PR:`, `Reviewer eligibility:`, `Pinned reviewed head:`, + `Review decision:`, `Merge result:`, `Linked issue status:`, `Cleanup status:`, + and precise mutation categories — not `Workspace mutations`) - issue-filing tasks (#191): `Issue created or updated:`, `Related issues:`; body must cite exact issue number/title, duplicate-search summary (issues searched, closest matches, why update rejected / new issue justified), full diff --git a/skills/llm-project-workflow/schemas/review-merge-final-report.md b/skills/llm-project-workflow/schemas/review-merge-final-report.md new file mode 100644 index 0000000..40a2999 --- /dev/null +++ b/skills/llm-project-workflow/schemas/review-merge-final-report.md @@ -0,0 +1,113 @@ +# Review-merge final report schema + +**Task mode:** `review-merge-pr` + +Every review or merge run must end with a section titled exactly +`Controller Handoff` validated by `review_proofs.assess_controller_handoff`. + +Use this schema — not author or issue-filing fields (`PR number opened`, +`Claim/comment status`, `Workspace mutations`). + +## Required header + +```md +## Controller Handoff + +- Task: review-merge-pr +- Repo: +- Role/profile: +- Identity: +- Mode: review-merge-pr +``` + +Identity format: `username / profile` (e.g. `sysadmin / prgs-reviewer`). Do not +include personal email unless required for disambiguation (#305). + +## Selection and eligibility + +```md +- Selected PR: +- PR live state: +- Pinned reviewed head: +- Reviewer eligibility: +- Repositories checked: +- Open PR counts: +- Inventory completeness: +- Inventory pagination proof: +- Selected PR or reason none selected: +- Linked issue: +- Linked issue live status: +``` + +Linked issue number must match across all sections (#314). If not verified live: +`Linked issue status: not verified in this session`. + +## Validation proof (#311, #323) + +```md +- Validation command: +- Validation working directory: +- Validation executable proof: +- Validation result: +- Baseline SHA: (required if full suite failed on PR head) +- Baseline validation command/result: (required if approving despite full-suite failure) +- Matching failure list: +``` + +Reject vague claims like "same as master" without baseline worktree proof. + +## Review and merge outcome + +```md +- Review decision: +- Review mutations: +- Merge result: +- Merge commit: +- Cleanup status: +``` + +`Review mutations` must list exactly one live terminal review unless +operator-approved correction is documented. + +## Mutation ledger (#319, #320) + +Use precise categories — **not** `Workspace mutations` or bare `Mutations: None`. + +```md +- File edits by reviewer: none +- Worktree/index mutations: +- Git ref mutations: +- MCP/Gitea mutations: +- Review mutations: +- Merge mutations: +- Cleanup mutations: +- External-state mutations: +- Read-only diagnostics: +``` + +Rules: + +- `git fetch` → Git ref mutations +- `git worktree add/remove` → Worktree/index mutations +- merge simulation → Worktree/index mutations (#317) +- MCP review/merge calls → MCP/Gitea mutations + Review/Merge mutations + +## Proof wording (#330) + +Do not claim "live proof" for blockers unless revalidated this session. Prior +feedback at unchanged head: say `prior blocker reused` or `prior request-changes +still applies`. + +Reject unsupported phrases: `inventory complete`, `same as master`, `live proof`, +`file edits none` — unless matching evidence is present. + +## Blockers and safety + +```md +- Blockers: +- Safe next action: +- Safety: no self-review; no self-merge; no author mutations; no code edits +``` + +Long format (§K in SKILL.md) is required when merge occurred, validation failed, +permissions blocked, or multiple repos were involved. \ No newline at end of file diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index bee3b75..ae99544 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -5,6 +5,10 @@ Copy, fill the `<...>` fields, and paste as the task prompt. ```text Task: merge PR # for issue # if it is eligible and checks pass. +Load the canonical workflow first: +`skills/llm-project-workflow/workflows/review-merge-pr.md` (task mode: review-merge-pr). +Final report schema: `schemas/review-merge-final-report.md`. + Rules (llm-project-workflow): - Only an eligible, NON-author reviewer merges. If authenticated user == PR author → STOP. diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 088243b..391f962 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -30,6 +30,10 @@ Repo name disambiguation (Gitea-Tools blind review hardening): commit is not valid corroboration. Author-bound sessions must not present reviewer queue inventory as a reviewer decision. +Load the canonical workflow first: +`skills/llm-project-workflow/workflows/review-merge-pr.md` (task mode: review-merge-pr). +Final report schema: `schemas/review-merge-final-report.md`. + Rules (llm-project-workflow): - Review in a SEPARATE detached review worktree, never the author's folder. - Worktree safety (#233): before checkout, diff, validation, review, or merge, diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md new file mode 100644 index 0000000..3057afc --- /dev/null +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -0,0 +1,243 @@ +# Review and merge PR workflow + +**Task mode:** `review-merge-pr` + +Load this file before any PR review or merge mutation. Parent router: +[`../SKILL.md`](../SKILL.md). Final report schema: +[`../schemas/review-merge-final-report.md`](../schemas/review-merge-final-report.md). + +## Mode isolation (hard walls) + +Review mode **must not**: + +- create issues or edit issue bodies +- edit repository files, run formatting, or fix code +- claim issues, create branches, commit, push, or open PRs +- use author MCP namespace (`gitea-tools` / `prgs-author`) for mutations +- improvise raw MCP repair, shell fallbacks, or background task tools + +Review mode **must**: + +- use reviewer MCP namespace (`gitea-reviewer` / `prgs-reviewer`) +- prove identity, profile, runtime context, and exact capabilities before mutation +- validate only in a session-owned worktree under `branches/` +- keep the main checkout on `master`/`main`/`dev` for orchestration only + +If the authenticated user equals the PR author → **stop** (no self-review, no self-merge). + +--- + +## Global worktree rule (reviewer) + +The main project checkout is orchestration only. All review validation happens +under `branches/` — typically a detached review worktree from +`scripts/worktree-review `. + +Before validation, prove: + +1. project root +2. working directory (must be under `branches/`) +3. worktree path +4. worktree clean status +5. pinned PR head SHA == `git rev-parse HEAD` + +Never run `git stash`, `git checkout --`, `git reset`, or `git clean` on +another session's dirty files. If the shared worktree is dirty, create a scratch +review worktree instead. + +Merge simulation (`git merge --no-commit`, `git merge --abort`) is a +**worktree/index mutation**, not read-only diagnostics (#317). + +--- + +## 1. Pre-flight gates + +Run before any PR mutation: + +1. `gitea_whoami` — prove reviewer identity (not PR author) +2. `gitea_get_profile` / `gitea_get_runtime_context` +3. `gitea_resolve_task_capability` for `review_pr`, `approve_pr`, `merge_pr` +4. `gitea_check_pr_eligibility` for the intended action on the selected PR + +Capability claims must cite exact resolver output (#179); bare "checks passed" is +downgraded. + +Stay in the reviewer namespace. Any author-namespace call requires explicit +justification in the final report. + +--- + +## 2. PR inventory and queue selection + +### Repo disambiguation + +- `Gitea-Tools` / `gitea tool` / `MCP Gitea tool` → `Scaled-Tech-Consulting/Gitea-Tools` +- `mcp-control-plane` → `Scaled-Tech-Consulting/mcp-control-plane` +- Ambiguous requests → inventory **both** configured repos + +Report must state exactly which repo(s) were checked. A single-repo zero must not +be reported as a global empty queue if the other repo was not inventoried. + +### Pagination proof (#330) + +PR inventory completeness requires explicit final-page/no-next-page evidence, +total-count evidence, or traversal until an empty/final page. Do not prove +completeness by assuming the default Gitea API page size. + +### Trust gate (#196, #198) + +Before reporting "no open PRs" or "queue empty": + +- run `pr_inventory_trust_gate` (or `review_proofs.assess_reviewer_queue_inventory`) +- only `trusted_empty` allows a clean empty-queue stop +- a bare `[]` from `gitea_list_prs` is never sufficient + +Empty-queue reports must include `pr_inventory_trust_gate.status`, reasons, +corroboration, remote/owner/repo/state filter, and inventory MCP profile. + +### Queue ordering (#321, #318) + +Select the lowest eligible open PR unless a documented blocker requires skipping. +Skipping requires live proof the blocker still applies — not stale handoff text. + +### Already-landed gate (#292, #294, #295) + +If the PR head is already on the target branch, stop review/approve/merge. +Redirect to reconciliation workflow; do not treat as review-eligible. + +--- + +## 3. Review worktree and pinned head + +1. `scripts/worktree-review ` → detached under `branches/review-*` +2. Prove checkout (#173): + - pinned PR head SHA from Gitea + - local `git rev-parse HEAD` + - `HEAD ==` pinned head + - diff base == PR base branch +3. If head moved → re-pin and restart review +4. Confirm worktree clean; inspect full diff; confirm scope matches linked issue + +Linked issue consistency (#314): fetch/verify the linked issue live. Report the +same issue number in all sections. If not verified: +`Linked issue status: not verified in this session`. + +Secret/provenance sweep (#179): state exact command/script/pattern and scope. + +--- + +## 4. Validation + +Run tests from the review worktree. Report must include (#311): + +- exact command as executed +- working directory +- exact result (pass/fail counts) +- executable path proof for bare commands like `pytest` (`which`, `--version`) + +If full suite fails, default action is `REQUEST_CHANGES` (#323). Approval with +failures requires baseline worktree proof under `branches/`, not main checkout. + +During validation, use `gitea_dry_run_pr_review` — never live APPROVE/REQUEST_CHANGES +to probe tool paths. + +--- + +## 5. Review mutation (terminal) + +After validation completes: + +1. Re-fetch PR; verify head SHA unchanged +2. `gitea_mark_final_review_decision` with pinned head +3. `gitea_submit_pr_review(..., final_review_decision_ready=True)` — exactly one live review +4. `gitea_get_pr_review_feedback` — confirm visible verdict (`approval_visible` for APPROVE; PENDING drafts do not count — #244) + +Final reports must list exactly one review mutation +(`review_proofs.assess_review_mutation_final_report`). + +Hard-stop after terminal review mutation (#332): do not perform additional review +mutations in the same session unless operator-approved correction. + +--- + +## 6. Merge preflight and execution + +Merge only when: + +- eligible non-author reviewer +- visible APPROVED review at pinned head +- no undismissed REQUEST_CHANGES +- PR open and mergeable +- live head == pinned reviewed head (re-fetch immediately before merge) + +Use `gitea_merge_pr` with `confirmation="MERGE PR "` and `expected_head_sha`. + +Gated merge refuses moved heads and widened diffs when `expected_changed_files` is set. + +--- + +## 7. Post-merge cleanup + +After confirmed merge on remote `master`: + +1. Post-merge file-presence verification on `master` +2. Close/release linked issue; remove `status:in-progress` +3. Delete remote branch (when safe) +4. `scripts/worktree-clean --delete-branch ` +5. Fetch/prune; confirm main checkout clean and `0 0` vs remote + +Never run cleanup before merge is confirmed on remote `master`. + +See also [`../templates/worktree-cleanup.md`](../templates/worktree-cleanup.md). + +--- + +## 8. Fail-closed stops + +Stop and report (no mutating action) when: + +- identity/capability cannot be proven +- self-author detected +- worktree state unclear +- pinned head mismatch +- inventory incomplete or untrusted +- tests fail without baseline proof +- PR closed-not-merged (`merged=false`) — run reconciliation instead +- infra_stop or MCP transport failure — recovery handoff only + +When blocked, emit recovery handoff per +[`../templates/recover-bad-state.md`](../templates/recover-bad-state.md). + +--- + +## 9. Role-boundary proof (#175) + +A reviewer queue task must not silently become author implementation. If no +eligible PR exists, stop with the queue report. Do not claim issues, branch, +commit, push, or open PRs unless the operator explicitly retasks as author work. + +Use `review_proofs.assess_role_boundary`; downgrade or stop if not clean. + +--- + +## 10. A-bar proofs (#179) + +All required before A-grade report or merge: + +- `assess_capability_evidence` +- `assess_sweep_evidence` +- `assess_live_state_recheck` (immediately before review submit and merge) +- `assess_role_boundary` +- `assess_review_mutation_final_report` +- `build_final_report` + +Missing proof → downgrade or block merge. + +--- + +## Prompt templates + +Short triggers still work via templates that reference this workflow: + +- [`../templates/review-pr.md`](../templates/review-pr.md) +- [`../templates/merge-pr.md`](../templates/merge-pr.md) \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py new file mode 100644 index 0000000..f485066 --- /dev/null +++ b/tests/test_llm_workflow_split.py @@ -0,0 +1,47 @@ +"""Doc-contract checks for task-specific LLM workflow split (#333, #334).""" +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +SKILL_DIR = REPO_ROOT / "skills" / "llm-project-workflow" + + +def test_review_merge_workflow_file_exists(): + path = SKILL_DIR / "workflows" / "review-merge-pr.md" + text = path.read_text(encoding="utf-8") + assert "Task mode:" in text + assert "review-merge-pr" in text + assert "gitea-reviewer" in text + assert "scripts/worktree-review" in text + + +def test_review_merge_final_report_schema_exists(): + path = SKILL_DIR / "schemas" / "review-merge-final-report.md" + text = path.read_text(encoding="utf-8") + assert "Controller Handoff" in text + assert "Worktree/index mutations" in text + assert "Workspace mutations" in text # documented as rejected + + +def test_skill_router_points_to_review_merge_workflow(): + skill = (SKILL_DIR / "SKILL.md").read_text(encoding="utf-8") + assert "## Task mode router" in skill + assert "workflows/review-merge-pr.md" in skill + assert "schemas/review-merge-final-report.md" in skill + assert "Identify task mode before any mutation" in skill + + +def test_skill_still_declares_controller_handoff_contract(): + skill = (SKILL_DIR / "SKILL.md").read_text(encoding="utf-8") + assert "## Controller Handoff" in skill + assert "assess_controller_handoff" in skill + assert "## Global LLM Worktree Rule" in skill + + +def test_review_pr_template_references_extracted_workflow(): + text = (SKILL_DIR / "templates" / "review-pr.md").read_text(encoding="utf-8") + assert "workflows/review-merge-pr.md" in text + + +def test_merge_pr_template_references_extracted_workflow(): + text = (SKILL_DIR / "templates" / "merge-pr.md").read_text(encoding="utf-8") + assert "workflows/review-merge-pr.md" in text \ No newline at end of file From c05b2f4c05ee44fcf3cc5f80f779d9f6f8bccaec Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 03:37:42 -0400 Subject: [PATCH 2/7] docs(skill): load canonical review-merge prompt into workflow file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace summary workflow with full §0–§38 canonical PR review/merge rules. Align review-merge-final-report schema with §37 controller handoff fields. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/llm-project-workflow/SKILL.md | 2 +- .../schemas/review-merge-final-report.md | 143 +- .../workflows/review-merge-pr.md | 1232 ++++++++++++++--- tests/test_llm_workflow_split.py | 12 +- 4 files changed, 1136 insertions(+), 253 deletions(-) diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index ed00208..8391e02 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -24,7 +24,7 @@ final-report schema; do not apply rules from other modes. | Task mode | Workflow | Final report schema | |-----------|----------|---------------------| -| `review-merge-pr` | [`workflows/review-merge-pr.md`](workflows/review-merge-pr.md) | [`schemas/review-merge-final-report.md`](schemas/review-merge-final-report.md) | +| `review-merge-pr` | [`workflows/review-merge-pr.md`](workflows/review-merge-pr.md) (canonical; §0–§38) | [`schemas/review-merge-final-report.md`](schemas/review-merge-final-report.md) | | `create-issue` | `workflows/create-issue.md` (planned — #333) | `schemas/create-issue-final-report.md` (planned) | | `work-issue` | `workflows/work-issue.md` (planned — #333) | `schemas/work-issue-final-report.md` (planned) | diff --git a/skills/llm-project-workflow/schemas/review-merge-final-report.md b/skills/llm-project-workflow/schemas/review-merge-final-report.md index 40a2999..237d2f2 100644 --- a/skills/llm-project-workflow/schemas/review-merge-final-report.md +++ b/skills/llm-project-workflow/schemas/review-merge-final-report.md @@ -1,80 +1,56 @@ -# Review-merge final report schema +# Review-merge controller handoff schema **Task mode:** `review-merge-pr` -Every review or merge run must end with a section titled exactly -`Controller Handoff` validated by `review_proofs.assess_controller_handoff`. +End every review/merge run with a section titled exactly `Controller Handoff`. +Use this canonical field set. Do not omit fields — use `none` or +`not verified in this session` where appropriate. -Use this schema — not author or issue-filing fields (`PR number opened`, -`Claim/comment status`, `Workspace mutations`). - -## Required header +Do not use legacy fields: `Pinned reviewed head`, `Scratch worktree used`, +`Workspace mutations`, `Mutations: None` (when mutations occurred). ```md ## Controller Handoff -- Task: review-merge-pr +- Task: - Repo: -- Role/profile: +- Role: - Identity: -- Mode: review-merge-pr -``` - -Identity format: `username / profile` (e.g. `sysadmin / prgs-reviewer`). Do not -include personal email unless required for disambiguation (#305). - -## Selection and eligibility - -```md +- Active profile: +- Runtime context: - Selected PR: -- PR live state: -- Pinned reviewed head: -- Reviewer eligibility: -- Repositories checked: -- Open PR counts: -- Inventory completeness: -- Inventory pagination proof: -- Selected PR or reason none selected: - Linked issue: -- Linked issue live status: -``` - -Linked issue number must match across all sections (#314). If not verified live: -`Linked issue status: not verified in this session`. - -## Validation proof (#311, #323) - -```md -- Validation command: -- Validation working directory: -- Validation executable proof: -- Validation result: -- Baseline SHA: (required if full suite failed on PR head) -- Baseline validation command/result: (required if approving despite full-suite failure) -- Matching failure list: -``` - -Reject vague claims like "same as master" without baseline worktree proof. - -## Review and merge outcome - -```md +- Eligibility class: +- Queue ordering policy: +- Inventory pagination proof: +- Earlier PRs skipped: +- Candidate head SHA: +- Reviewed head SHA: +- Target branch: +- Target branch SHA: +- Already-landed gate: +- Author-safety result: +- Prior request-changes state: +- Review worktree used: +- Review worktree path: +- Review worktree inside branches: +- Review worktree HEAD state: +- Review worktree dirty before validation: +- Review worktree dirty after validation: +- Baseline worktree used: +- Baseline worktree path: +- Files reviewed: +- Validation: +- Official validation integrity status: +- Terminal review mutation: - Review decision: -- Review mutations: +- Merge preflight: - Merge result: -- Merge commit: -- Cleanup status: -``` - -`Review mutations` must list exactly one live terminal review unless -operator-approved correction is documented. - -## Mutation ledger (#319, #320) - -Use precise categories — **not** `Workspace mutations` or bare `Mutations: None`. - -```md -- File edits by reviewer: none +- Linked issue status: +- Main checkout branch: +- Main checkout dirty state: +- Main checkout updated: +- File edits by reviewer: - Worktree/index mutations: - Git ref mutations: - MCP/Gitea mutations: @@ -83,31 +59,24 @@ Use precise categories — **not** `Workspace mutations` or bare `Mutations: Non - Cleanup mutations: - External-state mutations: - Read-only diagnostics: -``` - -Rules: - -- `git fetch` → Git ref mutations -- `git worktree add/remove` → Worktree/index mutations -- merge simulation → Worktree/index mutations (#317) -- MCP review/merge calls → MCP/Gitea mutations + Review/Merge mutations - -## Proof wording (#330) - -Do not claim "live proof" for blockers unless revalidated this session. Prior -feedback at unchanged head: say `prior blocker reused` or `prior request-changes -still applies`. - -Reject unsupported phrases: `inventory complete`, `same as master`, `live proof`, -`file edits none` — unless matching evidence is present. - -## Blockers and safety - -```md - Blockers: +- Current status: - Safe next action: -- Safety: no self-review; no self-merge; no author mutations; no code edits +- Safety statement: ``` -Long format (§K in SKILL.md) is required when merge occurred, validation failed, -permissions blocked, or multiple repos were involved. \ No newline at end of file +### Already-landed handoff overrides + +When eligibility class is `ALREADY_LANDED_RECONCILE_REQUIRED`: + +- Reviewed head SHA: `none` +- Review worktree used: `false` +- Review worktree path: `none` +- Review decision: `none` +- Merge result: `none` + +Identity format: `username / profile` (not personal email unless required — #305). + +Narrative final report and controller handoff must agree on eligibility class, +candidate/reviewed head SHA, mutation state, worktree usage, review decision, +terminal review mutation, merge result, and linked issue status. \ No newline at end of file diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 3057afc..86eb359 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -1,243 +1,1153 @@ -# Review and merge PR workflow +--- +task_mode: review-merge-pr +canonical: true +final_report_schema: ../schemas/review-merge-final-report.md +--- + +# Review and merge PR workflow (canonical) **Task mode:** `review-merge-pr` -Load this file before any PR review or merge mutation. Parent router: -[`../SKILL.md`](../SKILL.md). Final report schema: -[`../schemas/review-merge-final-report.md`](../schemas/review-merge-final-report.md). +This file is the canonical PR review/merge workflow for Gitea-Tools. Load it +before any PR mutation. Final report schema: +[`schemas/review-merge-final-report.md`](../schemas/review-merge-final-report.md). -## Mode isolation (hard walls) +**Default task prompt:** -Review mode **must not**: +> Review the next eligible open PR in this project. Merge it only if every +> identity, capability, author-safety, inventory, queue-ordering, worktree, +> validation, already-landed, prior-review, final-report, mutation-ledger, +> proof-wording, and merge gate passes. -- create issues or edit issue bodies -- edit repository files, run formatting, or fix code -- claim issues, create branches, commit, push, or open PRs -- use author MCP namespace (`gitea-tools` / `prgs-author`) for mutations -- improvise raw MCP repair, shell fallbacks, or background task tools - -Review mode **must**: - -- use reviewer MCP namespace (`gitea-reviewer` / `prgs-reviewer`) -- prove identity, profile, runtime context, and exact capabilities before mutation -- validate only in a session-owned worktree under `branches/` -- keep the main checkout on `master`/`main`/`dev` for orchestration only - -If the authenticated user equals the PR author → **stop** (no self-review, no self-merge). +Do not improvise around the gates. Follow project skills, MCP gates, and +workflow rules exactly. --- -## Global worktree rule (reviewer) +## 0. Load the canonical workflow first -The main project checkout is orchestration only. All review validation happens -under `branches/` — typically a detached review worktree from -`scripts/worktree-review `. +Before starting PR work, check whether the project provides a canonical PR review/merge workflow through a project skill, runbook, or MCP helper. -Before validation, prove: +If available, load it first and report: -1. project root -2. working directory (must be under `branches/`) -3. worktree path -4. worktree clean status -5. pinned PR head SHA == `git rev-parse HEAD` +* workflow source +* workflow version, commit, or hash +* whether this prompt conflicts with the loaded workflow -Never run `git stash`, `git checkout --`, `git reset`, or `git clean` on -another session's dirty files. If the shared worktree is dirty, create a scratch -review worktree instead. +If the canonical workflow cannot be loaded and the project requires it, stop and produce a recovery handoff only. -Merge simulation (`git merge --no-commit`, `git merge --abort`) is a -**worktree/index mutation**, not read-only diagnostics (#317). +## 1. Start with live identity, profile, runtime, and capability checks ---- +Prove: -## 1. Pre-flight gates +* authenticated identity +* active reviewer profile +* repo/project +* runtime context +* exact capabilities needed for inventory, review, request-changes, approve, comment, and merge -Run before any PR mutation: +A nearby capability does not count. -1. `gitea_whoami` — prove reviewer identity (not PR author) -2. `gitea_get_profile` / `gitea_get_runtime_context` -3. `gitea_resolve_task_capability` for `review_pr`, `approve_pr`, `merge_pr` -4. `gitea_check_pr_eligibility` for the intended action on the selected PR +Examples: -Capability claims must cite exact resolver output (#179); bare "checks passed" is -downgraded. +* `create_issue` does not authorize `issue_comment` +* `review_pr` does not authorize `merge_pr` +* `issue_comment` does not authorize `create_issue` +* `review_pr` does not authorize `request_changes` unless the capability explicitly covers that operation +* `review_pr` does not authorize `approve` unless the capability explicitly covers that operation -Stay in the reviewer namespace. Any author-namespace call requires explicit -justification in the final report. +If capability cannot be proven, stop and produce a recovery handoff only. ---- +## 2. Stop immediately on blocked infrastructure -## 2. PR inventory and queue selection +If any of the following appears, stop immediately: -### Repo disambiguation +* `infra_stop` +* MCP reconnect failure +* stale capability state +* dirty control checkout +* dirty review worktree +* missing capability +* workspace mismatch +* stale target branch state +* broken canonical workflow loading +* failed required preflight +* capability resolver warning that says the current state may be unsafe +* stale or inconsistent runtime context -- `Gitea-Tools` / `gitea tool` / `MCP Gitea tool` → `Scaled-Tech-Consulting/Gitea-Tools` -- `mcp-control-plane` → `Scaled-Tech-Consulting/mcp-control-plane` -- Ambiguous requests → inventory **both** configured repos +Do not continue PR selection, validation, review, approval, merge, cleanup, or reconciliation. -Report must state exactly which repo(s) were checked. A single-repo zero must not -be reported as a global empty queue if the other repo was not inventoried. +Produce an executable recovery handoff only. -### Pagination proof (#330) +Blocked recovery handoffs must not include direct approve or merge replay commands. -PR inventory completeness requires explicit final-page/no-next-page evidence, -total-count evidence, or traversal until an empty/final page. Do not prove -completeness by assuming the default Gitea API page size. +Blocked handoffs must say to rerun the full workflow after the blocker clears. -### Trust gate (#196, #198) +Do not preserve stale `ready to merge`, `approved`, or `all gates passed` state across MCP failures, reconnects, or capability re-resolution. -Before reporting "no open PRs" or "queue empty": +## 3. Author safety -- run `pr_inventory_trust_gate` (or `review_proofs.assess_reviewer_queue_inventory`) -- only `trusted_empty` allows a clean empty-queue stop -- a bare `[]` from `gitea_list_prs` is never sufficient +If you are the PR author, stop immediately. -Empty-queue reports must include `pr_inventory_trust_gate.status`, reasons, -corroboration, remote/owner/repo/state filter, and inventory MCP profile. +Do not approve, request changes, comment as reviewer, merge, close, or mutate the PR. -### Queue ordering (#321, #318) +Produce a recovery handoff only. -Select the lowest eligible open PR unless a documented blocker requires skipping. -Skipping requires live proof the blocker still applies — not stale handoff text. +If author identity cannot be proven, stop and produce a recovery handoff only. -### Already-landed gate (#292, #294, #295) +If authenticated identity changes during the run, restart identity/profile/capability checks before any further mutation. -If the PR head is already on the target branch, stop review/approve/merge. -Redirect to reconciliation workflow; do not treat as review-eligible. +## 4. Main checkout rule ---- +The main project checkout must stay on `master`, `main`, or `dev`. -## 3. Review worktree and pinned head +Do not review, test, fix, merge, resolve conflicts, or cleanup from the main checkout. -1. `scripts/worktree-review ` → detached under `branches/review-*` -2. Prove checkout (#173): - - pinned PR head SHA from Gitea - - local `git rev-parse HEAD` - - `HEAD ==` pinned head - - diff base == PR base branch -3. If head moved → re-pin and restart review -4. Confirm worktree clean; inspect full diff; confirm scope matches linked issue +Do not run tests in the main checkout. -Linked issue consistency (#314): fetch/verify the linked issue live. Report the -same issue number in all sections. If not verified: -`Linked issue status: not verified in this session`. +Do not run baseline validation in the main checkout. -Secret/provenance sweep (#179): state exact command/script/pattern and scope. +All PR validation and baseline comparison work must happen under the project’s `branches/` directory. ---- +No exceptions for small fixes, docs, tests, cleanup, conflict resolution, emergencies, or “just one file.” -## 4. Validation +Updating the main checkout is allowed only after a successful merge and only if the project workflow explicitly allows stable-branch update. -Run tests from the review worktree. Report must include (#311): +Do not update the main checkout if merge failed, was blocked, or produced reconciliation-only status. -- exact command as executed -- working directory -- exact result (pass/fail counts) -- executable path proof for bare commands like `pytest` (`which`, `--version`) +If the main checkout is dirty before selection, stop and produce a recovery handoff. -If full suite fails, default action is `REQUEST_CHANGES` (#323). Approval with -failures requires baseline worktree proof under `branches/`, not main checkout. +If the main checkout becomes dirty during the run, stop and produce a recovery handoff unless the only change is the explicitly allowed post-merge stable-branch fast-forward. -During validation, use `gitea_dry_run_pr_review` — never live APPROVE/REQUEST_CHANGES -to probe tool paths. +## 5. No raw MCP repair during normal review ---- +Do not run `pkill`, kill MCP processes, edit MCP config, restart servers, or perform control-checkout repair during normal PR review. -## 5. Review mutation (terminal) +If MCP repair is required, stop PR review and produce a separate `CONTROL-CHECKOUT REPAIR MODE` handoff. -After validation completes: +Do not mix MCP repair mode with PR review/merge mode. -1. Re-fetch PR; verify head SHA unchanged -2. `gitea_mark_final_review_decision` with pinned head -3. `gitea_submit_pr_review(..., final_review_decision_ready=True)` — exactly one live review -4. `gitea_get_pr_review_feedback` — confirm visible verdict (`approval_visible` for APPROVE; PENDING drafts do not count — #244) +Do not use successful repair as permission to resume the same review flow. After repair, rerun the full workflow from the beginning. -Final reports must list exactly one review mutation -(`review_proofs.assess_review_mutation_final_report`). +## 6. No background task tools -Hard-stop after terminal review mutation (#332): do not perform additional review -mutations in the same session unless operator-approved correction. +Do not use `schedule`, `manage_task`, background jobs, async waits, or delayed task tools during PR review/merge. ---- +Use direct commands and MCP tools only. -## 6. Merge preflight and execution +If a required action cannot complete synchronously, stop and produce a recovery handoff. -Merge only when: +Long synchronous commands, such as a test suite, are allowed only if they are run directly and reported with exact command, working directory, and result. -- eligible non-author reviewer -- visible APPROVED review at pinned head -- no undismissed REQUEST_CHANGES -- PR open and mergeable -- live head == pinned reviewed head (re-fetch immediately before merge) +Do not say “I will check later,” “I will monitor,” or “I will continue in the background.” -Use `gitea_merge_pr` with `confirmation="MERGE PR "` and `expected_head_sha`. +## 7. No local Gitea fallback during normal review -Gated merge refuses moved heads and widened diffs when `expected_changed_files` is set. +During normal reviewer workflows, do not read Gitea profile secret files. ---- +Do not inspect or open files such as: -## 7. Post-merge cleanup +* `profiles.json` +* local token stores +* credential files +* local Gitea auth/profile config files +* `.env` files containing Gitea credentials +* keychain dumps +* token helper outputs -After confirmed merge on remote `master`: +Do not run local Gitea helper scripts when MCP tools are available. -1. Post-merge file-presence verification on `master` -2. Close/release linked issue; remove `status:in-progress` -3. Delete remote branch (when safe) -4. `scripts/worktree-clean --delete-branch ` -5. Fetch/prune; confirm main checkout clean and `0 0` vs remote +Use MCP tools for Gitea operations. -Never run cleanup before merge is confirmed on remote `master`. +Local fallback is allowed only in explicit recovery mode when MCP is unavailable and identity/profile/capability can be independently proven. -See also [`../templates/worktree-cleanup.md`](../templates/worktree-cleanup.md). +If local fallback is used, report: ---- +* why MCP was unavailable +* exact identity proof +* exact profile proof +* exact repo proof +* exact capability proof +* exact local command used -## 8. Fail-closed stops +Do not use local fallback to bypass MCP gates. -Stop and report (no mutating action) when: +## 8. Build a complete live PR inventory -- identity/capability cannot be proven -- self-author detected -- worktree state unclear -- pinned head mismatch -- inventory incomplete or untrusted -- tests fail without baseline proof -- PR closed-not-merged (`merged=false`) — run reconciliation instead -- infra_stop or MCP transport failure — recovery handoff only +List all open PRs. -When blocked, emit recovery handoff per -[`../templates/recover-bad-state.md`](../templates/recover-bad-state.md). +Follow pagination until the tool proves there are no more pages. ---- +Do not assume inventory is complete. -## 9. Role-boundary proof (#175) +Do not claim `oldest eligible PR`, `next eligible PR`, or complete queue inventory unless pagination is proven. -A reviewer queue task must not silently become author implementation. If no -eligible PR exists, stop with the queue report. Do not claim issues, branch, -commit, push, or open PRs unless the operator explicitly retasks as author work. +Pagination proof must not rely on assumed default API page size. -Use `review_proofs.assess_role_boundary`; downgrade or stop if not clean. +Inventory is complete only if one of the following is proven: ---- +* the MCP response explicitly says there is no next page / `has_more=false` / final page +* the workflow traversed pages until an empty page or explicit final page was returned +* the tool response includes total-count or pagination metadata proving all open PRs were returned +* the request explicitly set `page` / `limit` / `per_page`, and the response explicitly proves the server honored that page size and did not truncate results -## 10. A-bar proofs (#179) +Do not say “inventory complete” merely because the result count is less than an assumed default page size. -All required before A-grade report or merge: +If the tool returns exactly the requested page size, assume there may be more pages unless the tool proves otherwise. -- `assess_capability_evidence` -- `assess_sweep_evidence` -- `assess_live_state_recheck` (immediately before review submit and merge) -- `assess_role_boundary` -- `assess_review_mutation_final_report` -- `build_final_report` +If pagination metadata is absent and the tool cannot page, report `INVENTORY_PAGINATION_UNPROVEN` and stop unless the canonical workflow explicitly permits continuing with an incomplete inventory. -Missing proof → downgrade or block merge. +For each open PR, identify: ---- +* PR number +* title +* author +* base branch +* head branch +* draft/WIP state +* mergeability +* linked issue +* current head SHA +* whether the PR appears already landed -## Prompt templates +Final report must include pagination/final-page proof. -Short triggers still work via templates that reference this workflow: +## 9. Queue ordering proof rule -- [`../templates/review-pr.md`](../templates/review-pr.md) -- [`../templates/merge-pr.md`](../templates/merge-pr.md) \ No newline at end of file +State the queue ordering policy before selecting a PR. + +If the project uses oldest-first review, explicitly sort or reason by PR number or created date. + +Do not rely on the API response order unless the tool proves that order matches the project policy. + +If newer PRs appear before the selected PR in the API response, explain that they are newer and not earlier in the queue. + +Final report must include: + +* queue ordering policy used +* whether API order matched that policy +* selected PR’s position under that policy +* earlier PRs considered +* earlier PRs skipped and why + +If queue ordering cannot be proven, stop and produce a recovery handoff. + +## 10. Select the next actionable PR using project rules + +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. + +Do not skip an earlier actionable PR without documenting why. + +Use these terms precisely: + +* `Oldest open PR requiring action` +* `Eligibility class` +* `Next review/merge eligible PR` + +Do not call an already-landed PR `eligible for review` or `eligible for merge`. + +If the oldest open PR is already landed, classify it as: + +`ALREADY_LANDED_RECONCILE_REQUIRED` + +If the oldest open PR is `ALREADY_LANDED_RECONCILE_REQUIRED`, stop normal review immediately unless project policy explicitly allows skipping reconciliation blockers. + +Report: + +`Queue blocked by already-landed PR requiring reconciliation` + +Do not keep rerunning normal review on the same already-landed PR. + +If the selected PR is not the oldest open PR requiring action, report why every earlier PR was not actionable. + +## 11. Skip-prior-PR rule + +Do not skip an earlier open PR without live proof or explicitly labeled prior proof from current-session review feedback. + +If skipping because of prior `REQUEST_CHANGES`, fetch current review feedback and verify the head SHA has not changed since the blocking review. + +If skipping because of non-mergeable status, fetch live PR state and provide conflict proof. + +Report for every skipped earlier PR: + +* skipped PR number +* current head SHA +* blocking category +* blocking review decision, if applicable +* blocking review head SHA, if applicable +* mergeability result, if applicable +* conflict proof command/tool, if applicable +* conflicting files, if available +* whether the head changed after the blocker +* reason it remains blocked + +If blocker proof cannot be fetched, classify the PR as: + +`BLOCKER_STATUS_UNVERIFIED` + +Then stop or produce a recovery handoff according to project policy. + +Do not skip an earlier PR based only on memory from a previous session. + +## 11A. Skipped PRs are read-only + +Skipping an earlier PR must be a read-only classification step. + +Do not submit `REQUEST_CHANGES`, approve, comment, merge, close, or otherwise mutate a skipped PR. + +If an earlier PR already has unresolved `REQUEST_CHANGES` at the same head SHA, do not duplicate the review. Fetch the review feedback, verify the head SHA is unchanged, classify the PR as blocked, and move on only if project policy allows skipping blocked PRs. + +If the head SHA changed after the blocking review, the PR is not safely skippable based on the old blocker. Re-evaluate it according to queue policy. + +The final report must distinguish: + +* prior blocker reused +* blocker revalidated live in this session +* blocker status unverified + +Do not claim live proof unless the proof command or MCP tool ran in the current session. + +If you mutate a skipped PR, stop immediately and report a workflow violation. + +## 12. Non-mergeable skip proof rule + +Do not skip a non-mergeable earlier PR without live proof. + +For every skipped non-mergeable PR, report: + +* PR number +* current head SHA +* mergeability result +* command/tool used to prove conflict +* conflicting files, if available +* whether head changed since any prior blocker + +If conflict proof cannot be retrieved, classify the PR as: + +`MERGEABILITY_UNVERIFIED` + +Then stop or produce a recovery handoff according to project policy. + +Conflict proof must be obtained in a session-owned diagnostic worktree under `branches/`, unless the MCP tool itself provides exact conflict files and proof. + +Do not perform conflict proof in the main checkout. + +## 13. Prior request-changes override rule + +Before approving a PR, fetch current PR review feedback. + +If the PR has a prior `REQUEST_CHANGES` review, report: + +* blocking reviewer +* blocking review timestamp +* blocking review head SHA +* current head SHA +* blocker text +* whether the head changed since the blocker + +If the head changed after the request-changes review, verify the new head addresses the blocker before approving. + +If the head did not change after the request-changes review, do not approve unless you prove one of: + +* the earlier blocker was incorrect +* the validation environment was wrong +* the issue was resolved externally in a way that does not require PR head changes +* project policy explicitly permits overriding that blocker with proof + +If this proof is missing, do not approve. + +If the selected PR still has unresolved `REQUEST_CHANGES` at the same head SHA and no override proof exists, submit `REQUEST_CHANGES` only if no equivalent unresolved request-changes review already exists at that same head SHA. Otherwise stop with a recovery handoff or classify it blocked according to project policy. + +Final report must include the prior blocker, current head, whether head changed, and why approval is safe. + +## 14. Pin the PR head SHA before validation + +Re-fetch the selected PR. + +Record the current head SHA. + +Call this the `candidate head SHA`. + +Do not call it the `reviewed head SHA` until validation and diff review have actually passed. + +If the head SHA changes before review submission or merge, restart review or stop. + +If the head SHA changes after approval but before merge, do not merge. Restart review or stop. + +Final report must include: + +* candidate head SHA +* reviewed head SHA, only if validation and diff review passed +* whether head changed during the run + +## 15. Refresh target branch before ancestry checks + +Fetch the PR target branch from the remote before checking whether the PR head is already landed. + +Do not rely on stale local `master`, `main`, or `dev`. + +Record the fetched target branch SHA used for ancestry checks. + +If the target branch cannot be fetched or verified, stop and produce a recovery handoff. + +`git fetch`, `git remote update`, and any command that updates refs must be reported under `Git ref mutations`, not read-only diagnostics. + +## 16. Already-landed PR gate + +After pinning the PR head SHA and refreshing the target branch, check whether the PR head SHA is already an ancestor of the target branch. + +If the PR head SHA is already landed on the target branch, stop normal review/merge flow. + +Do not approve the PR. + +Do not request changes unless there is a real code/review blocker. + +Do not call the merge API. + +Produce an `ALREADY_LANDED_RECONCILE_REQUIRED` handoff instead. + +The handoff must include: + +* PR number/title +* eligibility class +* candidate head SHA +* reviewed head SHA: `none` +* target branch +* target branch SHA +* ancestor proof +* linked issue status, if verified +* recommended reconciliation action +* required capabilities for reconciliation + +Any close/comment/issue mutation requires separate exact capability proof. + +Do not tell the user or another agent to manually close a PR or issue unless the handoff clearly says capability must be proven first or an authorized human must do it. + +## 17. Linked issue verification + +If the PR claims to close or link an issue, fetch the linked issue live before reporting its status. + +If the linked issue was not fetched live in the current session, report: + +`Linked issue status: not verified in this session` + +Do not claim `issue open`, `issue closed`, or `issue resolved` without live proof. + +Any PR close, issue close, or comment recommendation must say exact capability must be proven first, unless an authorized human performs it. + +The linked issue number in diagnostics, final report, and controller handoff must match the selected PR. + +Do not include stale issue numbers from prior PRs. + +If linked issue status changes after merge, fetch the issue again before reporting post-merge status. + +## 18. Review worktree ownership rule + +Prefer a fresh session-owned review worktree under `branches/`, named like: + +`branches/review-pr-` + +Do not reuse a branch-named or existing worktree unless safe-reuse proof passes. + +Safe-reuse proof must include: + +* exact worktree path +* worktree is inside `branches/` +* worktree is not the main checkout +* worktree is not owned by another active task/session +* clean tracked state +* clean untracked state +* current branch/head before reset +* reset target SHA +* explicit project policy allowing reuse/reset + +Do not run `git reset --hard`, `git clean`, checkout, or other destructive worktree commands unless the worktree is session-owned or safe-reuse proof passes. + +If safe-reuse proof cannot be produced, create a fresh session-owned review worktree. + +## 19. Reviewer worktree no-edit rule + +The PR validation worktree is for validating the submitted PR head only. + +Do not edit files in the PR validation worktree. + +Do not apply local fixes in the PR validation worktree. + +Do not run official validation after modifying the PR validation worktree. + +If you need to test a possible fix, create a separate diagnostic scratch worktree and label it clearly: + +`Diagnostic local experiment — not PR-head validation` + +Any diagnostic edits must be reported under `File edits by reviewer`. + +Diagnostic test results must not be reported as official PR validation results. + +Do not say `File edits by reviewer: none` if any file was edited in any review, diagnostic, baseline, or artifact worktree. + +## 20. Validate in a session-owned worktree under `branches/` + +Prove: + +* exact review worktree path +* worktree is inside `branches/` +* worktree is clean before validation +* PR head is checked out in that worktree +* review worktree HEAD matches candidate head SHA + +Run required validation: + +* tests +* compile checks +* lint checks +* diff checks +* secret/provenance checks +* dangerous artifact checks +* project-specific validation + +If validation fails, request changes with exact blockers, unless an equivalent unresolved request-changes review already exists at the same head SHA. + +If an equivalent unresolved request-changes review already exists at the same head SHA, do not duplicate it. Stop with a final report or recovery handoff according to project policy. + +If validation cannot run, do not approve or merge. + +## 21. Official validation integrity rule + +Record the PR head SHA before validation. + +Prove the validation worktree is clean before validation. + +Run validation against the unmodified PR head. + +After validation, prove the validation worktree is still clean. + +If validation fails, submit `REQUEST_CHANGES` using the unmodified PR-head failure, unless an equivalent unresolved request-changes review already exists at the same head SHA. + +If the worktree was edited after validation for diagnosis, report it separately and do not use post-edit test results as official validation. + +If official validation was contaminated by a local edit, stop and produce a recovery handoff. + +Official validation integrity status must be one of: + +* `passed` +* `failed` +* `not run` +* `contaminated — recovery required` + +Do not invent a softer status. + +## 22. Baseline validation rule + +Do not run tests in the main checkout. + +If the PR full suite fails and you need to compare failures against `master`, create a clean baseline worktree under `branches/`, such as: + +`branches/baseline-master-pr` + +Baseline comparison must include: + +* baseline worktree path +* baseline target SHA +* PR head SHA +* exact command run on both worktrees +* baseline failures +* PR failures +* proof the failure signatures match +* proof the baseline worktree was clean before and after validation +* proof the PR worktree was clean before and after validation + +Do not approve a PR with full-suite failures unless: + +* project policy allows baseline comparison +* baseline proof is complete +* failure signatures match +* new/changed tests pass +* failures are proven unrelated to the PR + +If full-suite failures differ or proof is incomplete, submit `REQUEST_CHANGES`, unless an equivalent unresolved request-changes review already exists at the same head SHA. + +Do not claim “same as master” unless the clean baseline worktree proof is included. + +Do not claim “full-suite failures are pre-existing” unless baseline proof is complete and the failure signatures match. + +## 23. Validation command proof rule + +Report the exact validation command as executed. + +Report the working directory where validation ran. + +If using bare `pytest`, also report: + +* `which pytest` +* `pytest --version` +* whether it resolves to the project venv + +Prefer the project venv executable when available, for example: + +`/Users/jasonwalker/Development/Gitea-Tools/venv/bin/pytest tests/` + +Do not summarize validation as just `pytest passed` unless the exact command, working directory, and result are included. + +For every validation command, report: + +* command +* working directory +* exit code or pass/fail result +* summary count if available +* whether it was official PR-head validation, baseline validation, or diagnostic-only validation + +## 24. Local merge simulation rule + +A local merge simulation is not read-only. + +Commands such as the following must be reported as worktree/index mutations: + +* `git merge --no-commit` +* `git merge --abort` +* `git reset` +* `git checkout` +* `git clean` +* `git worktree add` +* `git worktree remove` + +If merge simulation is needed, prefer a separate session-owned diagnostic worktree. + +If merge simulation is run in the review worktree, prove: + +* worktree clean before simulation +* exact merge command +* result/conflict status +* abort/reset command +* worktree clean after abort + +Do not list merge simulation under read-only diagnostics. + +Do not call the worktree clean after simulation unless clean status was actually checked after abort/reset. + +If conflicting files are reported, they must come from current-session proof. + +## 25. Review the actual diff + +Check: + +* correctness +* tests +* scope +* security boundaries +* workflow rule compliance +* whether the PR really closes or satisfies the linked issue +* unrelated changes +* dangerous generated artifacts +* secrets +* provenance markers +* temporary agent files +* whether the PR is redundant because the diff is already present on the target branch +* whether the PR modifies reviewer workflow, MCP gates, profiles, routing, or authorization boundaries +* whether documentation and tests match behavior + +Do not approve based only on test pass/fail status. Review the diff. + +Report files reviewed. + +## 26. Review decision + +If blockers exist, submit `REQUEST_CHANGES`, unless an equivalent unresolved request-changes review already exists at the same head SHA. + +If non-blocking notes exist, comment clearly only if exact comment capability is proven. + +If everything passes, approve only if exact approve/review capability is proven. + +Do not approve and merge unless both actions are separately authorized. + +Submit review only after dry-run review passes if the tool supports dry run. + +Do not approve already-landed, redundant, stale, or reconciliation-only PRs. + +Do not approve a PR with unresolved prior `REQUEST_CHANGES` unless the prior-request-changes override rule is satisfied. + +Do not submit review mutations on skipped PRs. + +## 26A. Terminal review mutation hard-stop + +A live review mutation is terminal for the current reviewer run except for same-PR merge continuation after approval. + +Terminal review mutations include: + +* `REQUEST_CHANGES` +* `APPROVED` +* review comment submission +* PR comment submission used as review feedback +* any tool that records final review decision state +* any tool that changes PR review state +* any tool that consumes a session review-mutation budget + +After `REQUEST_CHANGES` or blocking feedback, stop immediately and produce the final report. + +After `APPROVED`, continue only to merge preflight and merge for the same selected PR, if merge is authorized and all merge gates pass. + +Do not mutate one PR and then continue reviewing, approving, requesting changes, commenting on, or merging another PR in the same run. + +Do not consume a live review mutation on an earlier skipped PR. + +If a live mutation happens on a non-selected or skipped PR, stop immediately and report the workflow violation. + +If a tool blocks further review mutation because a live mutation already occurred, stop immediately. Do not continue validation of another PR. + +The final report must identify: + +* the terminal mutation +* PR number affected by the terminal mutation +* whether same-PR merge continuation was allowed +* whether the run stopped as required + +## 27. Merge rules + +Before merge, rerun fresh live checks: + +* whoami +* active profile/runtime +* exact merge capability +* author safety +* PR re-fetch +* reviewed head SHA unchanged +* target branch freshly fetched +* PR still open +* PR still mergeable +* PR head is not already landed +* required checks still pass +* linked issue handling is correct +* review worktree is clean + +Merge only after all gates pass. + +Do not merge if: + +* MCP reconnect failed +* capability state is stale +* worktree is dirty +* PR head changed +* validation failed +* inventory was incomplete +* PR is already landed +* target branch is stale +* you are the PR author +* prior request-changes override proof is missing +* terminal review mutation occurred on a different PR earlier in the run +* final decision was not marked for the selected PR, if the workflow requires final-decision marking + +If the merge tool requires an explicit confirmation string, report: + +* first gated/no-op call, if any +* confirmation string used +* which call actually performed the merge + +Rejected, dry-run, or confirmation-gated merge calls must not be reported as performed merge mutations. + +## 28. After merge + +Confirm: + +* merge result +* target branch updated +* merged commit/SHA, if available +* selected PR head SHA is now ancestor of target branch +* linked issue status if the PR claims to close an issue + +Clean only the session-owned `branches/` review worktree if the project workflow explicitly allows cleanup. + +Do not delete or mutate unrelated branches/worktrees. + +Do not touch the main checkout except to update the stable branch after merge if explicitly allowed by the workflow. + +Any stable-branch update after merge is a git mutation and must be reported. + +Do not update the main checkout if merge failed, was blocked, or produced reconciliation-only status. + +If any local artifact is created after final cleanup, run and report a new final status check. + +## 29. Recovery handoff rules + +If blocked, produce a recovery handoff with: + +* exact blocker +* failed tool/function, if any +* repo/project +* selected PR, if one was safely selected +* eligibility class +* candidate head SHA, if known +* reviewed head SHA, only if validation and diff review passed +* target branch and target branch SHA, if known +* linked issue status, or `not verified in this session` +* worktree path, if created +* exact state reached before stopping +* safe next action +* statement that no unsafe mutation was attempted + +Blocked handoffs must not include direct approve or merge replay commands. + +Blocked handoffs must say to rerun the full workflow after the blocker clears. + +If the blocker is a terminal review mutation already consumed in the current run, the handoff must say that the next run must start from the beginning and must not reuse stale ready/approved state. + +## 30. Final report must be precise + +Include: + +* canonical workflow source/version/hash, if available +* authenticated identity/profile +* repo/project +* complete PR inventory proof, including pagination/final-page proof +* queue ordering policy used +* selected PR number/title/author +* eligibility class +* author-safety result +* skipped earlier PRs and live blocker proof or prior blocker proof, if any +* prior request-changes proof, if any +* candidate head SHA +* target branch and target branch SHA used for ancestry checks +* reviewed head SHA, only if validation and diff review passed +* already-landed gate result +* linked issue proof/status, if applicable +* main checkout branch +* main checkout dirty state +* review worktree used: true/false +* review worktree path +* review worktree inside `branches/`: true/false +* review worktree HEAD state: detached/branch/none +* review worktree dirty before validation: true/false/not applicable +* review worktree dirty after validation: true/false/not applicable +* baseline validation worktree, if used +* baseline validation result, if used +* files reviewed +* validation commands and results +* official validation integrity status +* diagnostic scratch experiments, if any +* local merge simulation result, if any +* review decision +* terminal review mutation, if any +* merge preflight results, if merge was attempted +* merge result, if merged +* cleanup result +* blockers, if stopped +* confirmation that the main checkout was not used for task work +* final proof that no unsupported proof wording is used + +If the report and actual tool/command log disagree, fix the report before final output. + +## 31. Final report must distinguish mutation types + +Do not use the legacy field `Workspace mutations`. + +Use only precise categories: + +* File edits by reviewer: +* Worktree/index mutations: +* Git ref mutations: +* MCP/Gitea mutations: +* Review mutations: +* Merge mutations: +* Cleanup mutations: +* External-state mutations: +* Read-only diagnostics: + +`git fetch`, `git remote update`, and any command that updates refs must be listed under `Git ref mutations`, not read-only diagnostics. + +If `git reset --hard`, checkout, clean, worktree add/remove, merge simulation, merge abort, or similar commands occurred, report them under `Worktree/index mutations`. + +Use precise wording: + +* `File edits by reviewer: none` +* `Worktree/index mutations: ...` +* `Git ref mutations: ...` +* `MCP/Gitea mutations: ...` + +Do not collapse review, merge, cleanup, or external-state mutations into vague wording. + +## 31A. Local artifact and report consistency rule + +Do not create local walkthrough, notes, markdown, JSON, or report artifacts during reviewer runs unless the canonical workflow or operator explicitly requires it. + +If any file is edited, created, generated, or written, report it under `File edits by reviewer`. + +For each file write, report: + +* exact path +* whether it was inside the repo +* whether it was tracked or untracked +* why it was created +* whether final `git status` was run after the write + +Do not say `File edits by reviewer: none` if any file write occurred. + +Do not write files after the final clean-status check unless you rerun and report a new final clean-status check. + +If the action log says `Edited`, `Created`, `Wrote`, or equivalent, the mutation ledger must include that file. + +If the file was created outside the repo, still report it under `File edits by reviewer` and label it `outside repo`. + +If the file was created inside the repo, report whether it is tracked or untracked. + +If an artifact was created only for operator handoff, state that explicitly. + +Default behavior: do not create `walkthrough.md` or similar artifacts unless the workflow explicitly requires it. + +## 32. Controller handoff consistency rule + +The controller handoff must use the same canonical schema and facts as the final report. + +If the already-landed gate fires, the controller handoff must include: + +* Eligibility class: `ALREADY_LANDED_RECONCILE_REQUIRED` +* Candidate head SHA: +* Reviewed head SHA: `none` +* Review worktree used: `false` +* Review worktree path: `none` +* Git ref mutations: +* Review decision: `none` +* Merge result: `none` + +Do not use these legacy fields: + +* `Pinned reviewed head` +* `Scratch worktree used` +* `Workspace mutations` +* `Mutations: None` when any git ref, MCP, Gitea, review, merge, cleanup, or external-state mutation occurred + +The narrative report and controller handoff must agree on: + +* eligibility class +* candidate head SHA +* reviewed head SHA +* mutation state +* worktree usage +* review decision +* terminal review mutation +* merge result +* linked issue status + +If they disagree, fix the report before final output. + +## 33. Forbidden final-report claims unless proven + +Do not claim: + +* `oldest eligible PR` +* `next eligible PR` +* `reviewed head SHA` +* `ready to merge` +* `workspace mutations none` +* `scratch worktree false` +* `all gates passed` +* `inventory complete` +* `already landed` +* `merged` +* `issue closed` +* `manual close required` +* `target branch up to date` +* `read-only merge simulation` +* `no file edits by reviewer` +* `same as master` +* `full-suite failures are pre-existing` +* `prior request-changes resolved` +* `local fallback was safe` +* `live proof` +* `live blocker proof` +* `skipped PR` +* `terminal mutation none` +* `no unsafe mutation` + +unless the corresponding proof is included. + +If anything blocks safe review or merge, stop immediately and produce an executable recovery handoff. + +Do not improvise around the gates. + +Do not preserve stale `ready to merge` state across MCP failures or reconnects. + +Do not include approve or merge replay commands in a blocked handoff. Say to restart the full workflow after the blocker clears. + +## 34. Proof wording enforcement + +The following phrases are forbidden unless directly supported by current-session evidence: + +* live proof +* live blocker proof +* inventory complete +* same as master +* no file edits +* full-suite failures are pre-existing +* issue closed +* merged +* target branch up to date +* all gates passed +* next eligible PR +* oldest eligible PR +* no unsafe mutation +* worktree clean +* linked issue verified +* PR still mergeable +* head unchanged +* final page +* no next page + +If the proof comes from prior review feedback rather than a command/tool run in the current session, label it as prior proof, not live proof. + +If relying on prior review feedback at unchanged head SHA, say: + +`Prior blocker reused; head SHA unchanged since blocking review.` + +Do not say: + +`Live blocker proof` + +unless the blocker was actually revalidated in the current session. + +If a tool call was rejected, confirmation-gated, dry-run-only, or no-op, report it separately from performed mutations. + +A dry run is not a mutation. + +A confirmation-gated rejected call is not a performed mutation. + +A successful merge call is a merge mutation. + +A successful review submission is a review mutation. + +A final-decision marking call is a review mutation if it records durable review state or consumes review-mutation budget. + +## 35. Duplicate request-changes prevention + +Before submitting `REQUEST_CHANGES`, fetch current review feedback. + +If an unresolved `REQUEST_CHANGES` review already exists at the same head SHA with the same blocker, do not submit another `REQUEST_CHANGES`. + +Report: + +* prior blocking reviewer +* prior blocking timestamp +* prior blocking head SHA +* current head SHA +* blocker text +* why the blocker still applies +* that no duplicate review mutation was submitted + +If the head SHA changed or the old blocker is not equivalent, evaluate normally. + +Do not consume the session’s review mutation budget just to duplicate an existing blocker. + +## 36. Final self-check before output + +Before final output, check the report for contradictions. + +Verify: + +* if any file was edited, `File edits by reviewer` is not `none` +* if any worktree was added/removed, `Worktree/index mutations` lists it +* if any fetch happened, `Git ref mutations` lists it +* if any review was submitted, `Review mutations` lists it +* if any merge was performed, `Merge mutations` lists it +* if any cleanup happened, `Cleanup mutations` lists it +* if any issue/PR external state changed, `External-state mutations` lists it +* if pagination is claimed complete, final-page proof is present +* if live proof is claimed, the proof ran in the current session +* if same-as-master is claimed, baseline proof is complete +* if issue closed is claimed, linked issue was fetched after merge +* if merged is claimed, merge result and target ancestry proof are present +* if selected PR is claimed next eligible, every earlier PR has proof-backed skip reasoning +* if a terminal mutation happened, no other PR was reviewed or mutated afterward + +If any contradiction exists, fix the final report before output. + +## 37. Controller handoff schema + +End every run with a controller handoff using this schema. + +Do not omit fields. Use `none` or `not verified in this session` where appropriate. + +Controller Handoff: + +* Task: +* Repo: +* Role: +* Identity: +* Active profile: +* Runtime context: +* Selected PR: +* Linked issue: +* Eligibility class: +* Queue ordering policy: +* Inventory pagination proof: +* Earlier PRs skipped: +* Candidate head SHA: +* Reviewed head SHA: +* Target branch: +* Target branch SHA: +* Already-landed gate: +* Author-safety result: +* Prior request-changes state: +* Review worktree used: +* Review worktree path: +* Review worktree inside branches: +* Review worktree HEAD state: +* Review worktree dirty before validation: +* Review worktree dirty after validation: +* Baseline worktree used: +* Baseline worktree path: +* Files reviewed: +* Validation: +* Official validation integrity status: +* Terminal review mutation: +* Review decision: +* Merge preflight: +* Merge result: +* Linked issue status: +* Main checkout branch: +* Main checkout dirty state: +* Main checkout updated: +* File edits by reviewer: +* Worktree/index mutations: +* Git ref mutations: +* MCP/Gitea mutations: +* Review mutations: +* Merge mutations: +* Cleanup mutations: +* External-state mutations: +* Read-only diagnostics: +* Blockers: +* Current status: +* Safe next action: +* Safety statement: + +## 38. Stop conditions summary + +Stop immediately and produce a recovery handoff if: + +* canonical workflow is required but cannot be loaded +* identity/profile/capability cannot be proven +* authenticated identity is the PR author +* runtime context is blocked +* infra stop appears +* MCP reconnect fails +* capability state is stale +* inventory pagination cannot be proven +* queue ordering cannot be proven +* earlier PR blocker status cannot be verified +* selected PR is already landed +* target branch cannot be fetched +* linked issue cannot be verified when required for decision +* review worktree is dirty +* validation cannot run +* validation fails and request-changes cannot be safely submitted +* baseline comparison is required but incomplete +* prior request-changes override proof is missing +* PR head changes before review or merge +* merge preflight fails +* terminal review mutation occurs on a different PR +* any report contradiction cannot be resolved + +Blocked handoffs must not include direct approve or merge replay commands. + +Blocked handoffs must say to rerun the full workflow after the blocker clears. + +Do not improvise around the gates. diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index f485066..b71babf 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -8,17 +8,21 @@ SKILL_DIR = REPO_ROOT / "skills" / "llm-project-workflow" def test_review_merge_workflow_file_exists(): path = SKILL_DIR / "workflows" / "review-merge-pr.md" text = path.read_text(encoding="utf-8") - assert "Task mode:" in text + assert "canonical: true" in text assert "review-merge-pr" in text - assert "gitea-reviewer" in text - assert "scripts/worktree-review" in text + assert "## 0. Load the canonical workflow first" in text + assert "## 26A. Terminal review mutation hard-stop" in text + assert "## 37. Controller handoff schema" in text + assert "INVENTORY_PAGINATION_UNPROVEN" in text + assert "ALREADY_LANDED_RECONCILE_REQUIRED" in text def test_review_merge_final_report_schema_exists(): path = SKILL_DIR / "schemas" / "review-merge-final-report.md" text = path.read_text(encoding="utf-8") assert "Controller Handoff" in text - assert "Worktree/index mutations" in text + assert "Candidate head SHA:" in text + assert "Terminal review mutation:" in text assert "Workspace mutations" in text # documented as rejected From 14f5c865b3580dfa53c2d06ad69d323831062373 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:23:45 -0400 Subject: [PATCH 3/7] feat: add create-issue final-report verifier (#337) Extend review_proofs with assess_create_issue_final_report to enforce workflow-source proof, create-issue handoff schema fields, and rejection of work-issue/review-merge cross-mode handoff fields. Closes #337 --- review_proofs.py | 187 +++++++++++++++++++++++++++++++ tests/test_llm_workflow_split.py | 8 +- tests/test_review_proofs.py | 118 +++++++++++++++++++ 3 files changed, 312 insertions(+), 1 deletion(-) diff --git a/review_proofs.py b/review_proofs.py index be71582..c820d08 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1571,8 +1571,55 @@ HANDOFF_ROLE_FIELDS = { )), ("Related issues", ("related issues",)), ), + "create_issue": ( + ("Active profile", ("active profile",)), + ("Runtime context", ("runtime context",)), + ("Requested issue task", ("requested issue task",)), + ("Workflow source", ("workflow source",)), + ("Capability proof", ("capability proof",)), + ("Duplicate search terms", ("duplicate search terms",)), + ("Duplicate search pagination proof", ( + "duplicate search pagination proof", + )), + ("Duplicates found", ("duplicates found",)), + ("Issues created", ("issues created",)), + ("Issues commented", ("issues commented",)), + ("Issues edited", ("issues edited",)), + ("Issues skipped as duplicates", ( + "issues skipped as duplicates", + )), + ("File edits by issue creator", ("file edits by issue creator",)), + ("Safety statement", ("safety statement",)), + ), } +CREATE_ISSUE_WORKFLOW_MARKERS = ( + "workflows/create-issue.md", + "create-issue.md", +) + +CREATE_ISSUE_TASK_MARKERS = ( + "create-issue", + "create issue", +) + +CREATE_ISSUE_FORBIDDEN_CROSS_MODE_FIELDS = ( + ("Selected issue", ("selected issue",)), + ("Branch name", ("branch name",)), + ("PR number", ("pr number",)), + ("PR URL", ("pr url",)), + ("Commit SHA", ("commit sha",)), + ("Push result", ("push result",)), + ("Baseline comparison", ("baseline comparison",)), + ("Selected PR", ("selected pr",)), + ("Review decision", ("review decision",)), + ("Merge result", ("merge result",)), + ("Merge preflight", ("merge preflight",)), + ("Already-landed gate", ("already-landed gate",)), + ("Pinned reviewed head", ("pinned reviewed head",)), + ("Terminal review mutation", ("terminal review mutation",)), +) + def _handoff_section_lines(report_text): """Return the lines of the Controller Handoff section, or None.""" @@ -1623,6 +1670,11 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): fields_dict[label] = v.strip() required = list(HANDOFF_BASE_FIELDS) + if role == "create_issue": + required = [ + field for field in required + if field[0] not in ("Workspace mutations", "Mutations") + ] required.extend(HANDOFF_ROLE_FIELDS.get(role or "", ())) missing = [] @@ -2647,6 +2699,141 @@ def assess_issue_filing_duplicate_summary( } +def assess_create_issue_workflow_source(report_text: str) -> dict: + """#337: create-issue reports must cite the canonical workflow file.""" + lower = (report_text or "").lower() + reasons = [] + if not any(marker in lower for marker in CREATE_ISSUE_WORKFLOW_MARKERS): + reasons.append( + "create-issue report missing workflow source " + "(workflows/create-issue.md)" + ) + if not any(marker in lower for marker in CREATE_ISSUE_TASK_MARKERS): + reasons.append( + "create-issue report missing task mode declaration " + "(create-issue)" + ) + return { + "complete": not reasons, + "downgraded": bool(reasons), + "reasons": reasons, + } + + +def assess_create_issue_mode_isolation(report_text: str) -> dict: + """#337: reject work-issue/review-merge handoff fields in create-issue runs.""" + section = _handoff_section_lines(report_text) + reasons = [] + if section is None: + return { + "complete": True, + "downgraded": False, + "reasons": [], + } + + labels = [] + for line in section: + stripped = line.strip().lstrip("-*").strip() + if ":" in stripped: + labels.append(stripped.split(":", 1)[0].strip().lower()) + + for name, aliases in CREATE_ISSUE_FORBIDDEN_CROSS_MODE_FIELDS: + if any( + label.startswith(alias) + for label in labels + for alias in aliases + ): + reasons.append( + f"create-issue handoff must not include cross-mode field " + f"'{name}'" + ) + + legacy_workspace = any( + label.startswith("workspace mutations") for label in labels + ) + precise_categories = any( + label.startswith("file edits by issue creator") + for label in labels + ) + if legacy_workspace and not precise_categories: + reasons.append( + "create-issue handoff must use precise mutation categories " + "instead of legacy 'Workspace mutations'" + ) + + return { + "complete": not reasons, + "downgraded": bool(reasons), + "reasons": reasons, + } + + +def assess_create_issue_final_report( + report_text: str, + *, + issue_number: int | None = None, + issue_title: str | None = None, + action: str = "created", + mutations: list[str] | None = None, + resolved_capabilities: dict | None = None, + issues_searched: int | None = None, + closest_matches: list[dict] | None = None, + duplicate_result: dict | None = None, + performed_mutations: list[str] | None = None, +) -> dict: + """#337: composite verifier for create-issue final reports.""" + checks = { + "workflow_source": assess_create_issue_workflow_source(report_text), + "mode_isolation": assess_create_issue_mode_isolation(report_text), + "controller_handoff": assess_controller_handoff( + report_text, role="create_issue" + ), + "issue_reference": assess_issue_filing_issue_reference( + report_text, + issue_number=issue_number, + issue_title=issue_title, + action=action, + ), + "mutation_capability": assess_issue_filing_mutation_capability( + report_text, + mutations or performed_mutations, + resolved_capabilities, + ), + "mutation_scope": assess_issue_filing_mutation_scope( + report_text, + performed_mutations=performed_mutations or mutations, + ), + "duplicate_summary": assess_issue_filing_duplicate_summary( + report_text, + issues_searched=issues_searched, + closest_matches=closest_matches, + duplicate_result=duplicate_result, + ), + } + + reasons = [] + downgraded = False + for name, result in checks.items(): + verdict = result.get("verdict") + if verdict in ("missing", "incomplete"): + downgraded = True + reasons.extend(result.get("reasons") or []) + elif result.get("downgraded") or not result.get("complete", True): + downgraded = True + reasons.extend( + f"{name}: {r}" for r in (result.get("reasons") or []) + ) + + grade = "A" if not downgraded else "downgraded" + return { + "grade": grade, + "downgraded": downgraded, + "checks": checks, + "reasons": reasons, + "complete": not downgraded, + } + + def assess_issue_filing_final_report( report_text: str, *, diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 327b17d..3e23692 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -97,4 +97,10 @@ def test_merge_pr_template_references_workflow(): def test_start_issue_template_references_work_issue_workflow(): text = (SKILL_DIR / "templates" / "start-issue.md").read_text(encoding="utf-8") - assert "workflows/work-issue.md" in text \ No newline at end of file + assert "workflows/work-issue.md" in text + + +def test_create_issue_final_report_verifier_exported(): + from review_proofs import assess_create_issue_final_report + + assert callable(assess_create_issue_final_report) \ No newline at end of file diff --git a/tests/test_review_proofs.py b/tests/test_review_proofs.py index 7166cca..b4683dc 100644 --- a/tests/test_review_proofs.py +++ b/tests/test_review_proofs.py @@ -29,6 +29,9 @@ from review_proofs import ( # noqa: E402 assess_contradictory_no_pr_claim, assess_continuation_mode_report, assess_controller_handoff, + assess_create_issue_final_report, + assess_create_issue_mode_isolation, + assess_create_issue_workflow_source, assess_edited_pr_inventory_coverage, assess_empty_queue_report, assess_fresh_issue_selection, @@ -2042,5 +2045,120 @@ class TestIssueFilingFinalReport(unittest.TestCase): self.assertTrue(result["downgraded"]) +class TestCreateIssueFinalReport(unittest.TestCase): + """#337: create-issue final-report verifier coverage.""" + + ISSUE_TITLE = "Extract create-issue workflow from monolithic LLM project skill" + CAPABILITIES = { + "create_issue": { + "requested_task": "create_issue", + "required_operation_permission": "gitea.issue.create", + "allowed_in_current_session": True, + }, + } + CLOSEST = [{"number": 333, "title": "Split monolithic workflow", "state": "closed"}] + + def _good_report(self): + return "\n".join([ + "Task mode: create-issue", + "Workflow source: skills/llm-project-workflow/workflows/create-issue.md", + f"Updated issue #337 — `{self.ISSUE_TITLE}` (remaining verifier AC).", + "Duplicate check: searched 54 open issues.", + "Closest existing: #333 — Split monolithic workflow (partial overlap).", + "Why new issue justified: dedicated create-issue extraction.", + "Only mutation: issue creation (gitea.issue.create).", + "Confirm no labels, comments, PRs, reviews, merges, or closes.", + "## Controller Handoff", + "- Task: create-issue", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: author", + "- Identity: jcwalker3 / prgs-author", + "- Active profile: prgs-author", + "- Runtime context: prgs-author on prgs", + "- Requested issue task: extract create-issue workflow verifier", + "- Workflow source: workflows/create-issue.md @ fd396df", + "- Capability proof: create_issue allowed (gitea.issue.create)", + "- Duplicate search terms: create-issue, workflow extract", + "- Duplicate search pagination proof: 54 open issues < limit 500", + "- Duplicates found: #333 partial only", + "- Issues created: none (implementation PR for remaining AC)", + "- Issues commented: none", + "- Issues edited: none", + "- Issues skipped as duplicates: none", + "- File edits by issue creator: review_proofs.py, tests", + "- Worktree/index mutations: feat/issue-337 worktree created", + "- Git ref mutations: fetch prgs/master", + "- MCP/Gitea mutations: gitea_mark_issue start on #337", + "- Issue mutations: claimed #337", + "- Label/assignment/milestone mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: whoami, list issues", + "- Blockers: none", + "- Current status: implementing verifier", + "- Safe next action: open PR", + "- Safety statement: no review/merge/repo main-checkout edits", + "- Issue/PR: #337", + "- Branch/SHA: feat/issue-337-create-issue-verifier", + "- Files changed: review_proofs.py, tests", + "- Validation: pytest tests/test_review_proofs.py", + "- Mutations: create_issue capability resolved; mark_issue only", + "- Next: open PR", + "- Safety: no review/merge", + ]) + + def test_complete_create_issue_report_earns_a(self): + result = assess_create_issue_final_report( + self._good_report(), + issue_number=337, + issue_title=self.ISSUE_TITLE, + action="updated", + mutations=["create_issue"], + resolved_capabilities=self.CAPABILITIES, + issues_searched=54, + closest_matches=self.CLOSEST, + performed_mutations=["create_issue"], + ) + self.assertEqual(result["grade"], "A") + self.assertFalse(result["downgraded"]) + + def test_missing_workflow_source_downgrades(self): + report = self._good_report().replace("workflows/create-issue.md", "SKILL.md only") + result = assess_create_issue_workflow_source(report) + self.assertTrue(result["downgraded"]) + + def test_work_issue_field_in_handoff_downgrades(self): + report = self._good_report().replace( + "- Safety statement:", + "- Selected issue: #123\n- Safety statement:", + ) + result = assess_create_issue_mode_isolation(report) + self.assertTrue(result["downgraded"]) + self.assertTrue( + any("Selected issue" in r for r in result["reasons"]) + ) + + def test_review_merge_field_in_handoff_downgrades(self): + report = self._good_report().replace( + "- Safety statement:", + "- Review decision: APPROVE\n- Safety statement:", + ) + result = assess_create_issue_mode_isolation(report) + self.assertTrue(result["downgraded"]) + + def test_legacy_workspace_mutations_without_precise_categories_downgrades(self): + report = self._good_report().replace( + "- File edits by issue creator:", + "- Workspace mutations: none\n- File edits by issue creator:", + ) + lines = report.splitlines() + cleaned = [ + line for line in lines + if not line.strip().startswith("- File edits by issue creator:") + ] + report = "\n".join(cleaned) + result = assess_create_issue_mode_isolation(report) + self.assertTrue(result["downgraded"]) + + if __name__ == "__main__": unittest.main() From 89292862767cdee0998bb0ab8a28d2dd2e0ed299 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:25:02 -0400 Subject: [PATCH 4/7] feat: add pagination metadata to gitea_list_prs (#340) Return wrapped {prs, pagination} from gitea_list_prs with has_more, inventory_complete, and page traversal via api_fetch_page. Route gitea_review_pr inventory through gitea_list_prs so trust gates can prove pagination finality. Add assess_list_prs_pagination_proof verifier. Closes #340 --- gitea_auth.py | 37 ++++++ gitea_mcp_server.py | 158 ++++++++++++++++++++----- review_proofs.py | 102 +++++++++++++++- tests/test_capability_stop_terminal.py | 10 +- tests/test_list_prs_pagination.py | 153 ++++++++++++++++++++++++ tests/test_mcp_server.py | 45 ++++--- tests/test_pr_queue_inventory.py | 122 ++++++++++--------- 7 files changed, 520 insertions(+), 107 deletions(-) create mode 100644 tests/test_list_prs_pagination.py diff --git a/gitea_auth.py b/gitea_auth.py index 52a34c6..5e30213 100644 --- a/gitea_auth.py +++ b/gitea_auth.py @@ -420,6 +420,43 @@ def api_get_all(url, auth_header, *, limit=None, page_size=50, max_pages=100, return results +def api_fetch_page(url, auth_header, *, page=1, limit=50, **kwargs): + """Fetch one page from a Gitea list endpoint with explicit pagination metadata. + + Returns ``(items, pagination)`` where *pagination* includes ``has_more``, + ``next_page``, and ``is_final_page`` derived from the returned page length. + """ + page = max(1, int(page)) + limit = max(1, min(50, int(limit))) + page_url = _add_query(url, page=page, limit=limit) + data = api_request("GET", page_url, auth_header, **kwargs) + if data is None: + pagination = { + "page": page, + "per_page": limit, + "returned_count": 0, + "has_more": False, + "next_page": None, + "is_final_page": True, + } + return [], pagination + if not isinstance(data, list): + raise RuntimeError( + f"expected a list page from Gitea, got {type(data).__name__}" + ) + returned = len(data) + has_more = returned >= limit + pagination = { + "page": page, + "per_page": limit, + "returned_count": returned, + "has_more": has_more, + "next_page": page + 1 if has_more else None, + "is_final_page": not has_more, + } + return data, pagination + + def gitea_url(host, path): """Build a full URL for *host* and *path*, using http for loopback and https for others.""" if not path.startswith("/"): diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 7eea703..88bc9ac 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -461,6 +461,7 @@ from gitea_auth import ( # noqa: E402 get_auth_header, api_request, api_get_all, + api_fetch_page, repo_api_url, get_profile, gitea_url, @@ -1118,6 +1119,56 @@ def gitea_create_pr( return _with_optional_url({"number": data["number"]}, data.get("html_url")) +def _format_list_pr_entry(pr: dict) -> dict: + head_obj = pr.get("head") or {} + base_obj = pr.get("base") or {} + entry = { + "number": pr["number"], + "title": pr["title"], + "state": pr["state"], + "head": head_obj.get("ref") if isinstance(head_obj, dict) else head_obj, + "base": base_obj.get("ref") if isinstance(base_obj, dict) else base_obj, + "mergeable": pr.get("mergeable"), + "updated_at": pr.get("updated_at"), + "head_sha": head_obj.get("sha") if isinstance(head_obj, dict) else None, + "author": (pr.get("user") or {}).get("login"), + "labels": [ + label["name"] + for label in pr.get("labels", []) + if isinstance(label, dict) and label.get("name") + ], + "body": pr.get("body") or "", + } + return _with_optional_url(entry, pr.get("html_url")) + + +def _list_prs_pagination_envelope( + prs: list[dict], + *, + page: int, + per_page: int, + pages_fetched: int, + is_final_page: bool, + has_more: bool, + next_page: int | None, + inventory_complete: bool, +) -> dict: + return { + "prs": prs, + "pagination": { + "page": page, + "per_page": per_page, + "returned_count": len(prs), + "has_more": has_more, + "next_page": next_page, + "is_final_page": is_final_page, + "pages_fetched": pages_fetched, + "inventory_complete": inventory_complete, + "total_count": len(prs) if inventory_complete else None, + }, + } + + @mcp.tool() def gitea_list_prs( state: str = "open", @@ -1125,7 +1176,9 @@ def gitea_list_prs( host: str | None = None, org: str | None = None, repo: str | None = None, -) -> list[dict]: + page: int | None = None, + per_page: int = 50, +) -> dict: """List pull requests on a Gitea repository. Args: @@ -1134,11 +1187,16 @@ def gitea_list_prs( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + page: Optional 1-based page for explicit single-page fetch. When + omitted, all pages are traversed and ``inventory_complete`` is set. + per_page: Page size (1–50; Gitea caps at 50). Returns: - List of dicts with 'number', 'title', 'state', 'head', 'base', - 'mergeable', 'updated_at' ('url' only with the reveal opt-in). - The additional 'updated_at' aids stale/conflicting queue detection. + Dict with ``prs`` (list of PR summaries) and ``pagination`` metadata + (``has_more``, ``next_page``, ``is_final_page``, ``inventory_complete``, + ``pages_fetched``, ``total_count`` when complete). Each PR entry carries + ``number``, ``title``, ``state``, ``head``, ``base``, ``mergeable``, + ``updated_at`` ('url' only with the reveal opt-in). """ allowed, block_reasons = capability_stop_terminal.check_reviewer_queue_tool( "list_prs" @@ -1148,20 +1206,48 @@ def gitea_list_prs( h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) url = f"{repo_api_url(h, o, r)}/pulls?state={state}" - prs = api_get_all(url, auth) - results = [] - for pr in prs: - entry = { - "number": pr["number"], - "title": pr["title"], - "state": pr["state"], - "head": pr["head"]["ref"], - "base": pr["base"]["ref"], - "mergeable": pr.get("mergeable"), - "updated_at": pr.get("updated_at"), - } - results.append(_with_optional_url(entry, pr.get("html_url"))) - return results + + if page is not None: + raw_page, pagination = api_fetch_page( + url, auth, page=page, limit=per_page + ) + prs = [_format_list_pr_entry(pr) for pr in raw_page] + return _list_prs_pagination_envelope( + prs, + page=pagination["page"], + per_page=pagination["per_page"], + pages_fetched=1, + is_final_page=pagination["is_final_page"], + has_more=pagination["has_more"], + next_page=pagination["next_page"], + inventory_complete=False, + ) + + all_raw: list[dict] = [] + current_page = 1 + pages_fetched = 0 + last_meta: dict = {} + while pages_fetched < 100: + raw_page, last_meta = api_fetch_page( + url, auth, page=current_page, limit=per_page + ) + pages_fetched += 1 + all_raw.extend(raw_page) + if last_meta["is_final_page"]: + break + current_page += 1 + + prs = [_format_list_pr_entry(pr) for pr in all_raw] + return _list_prs_pagination_envelope( + prs, + page=1, + per_page=per_page, + pages_fetched=pages_fetched, + is_final_page=bool(last_meta.get("is_final_page")), + has_more=False, + next_page=None, + inventory_complete=bool(last_meta.get("is_final_page")), + ) @mcp.tool() @@ -2928,19 +3014,25 @@ def gitea_review_pr( if can_read and auth: inventory_attempted = True try: - url = f"{repo_api_url(h, o, r)}/pulls?state=open" - prs = api_get_all(url, auth) - prs_found_count = len(prs) - for pr in prs: - labels = [l["name"] for l in pr.get("labels", [])] + prs = gitea_list_prs( + state="open", + remote=remote, + host=h, + org=o, + repo=r, + ) + prs_list = prs.get("prs") or [] + prs_found_count = len(prs_list) + for pr in prs_list: + labels = list(pr.get("labels") or []) body_text = pr.get("body") or "" linked = [] # Simple extraction of linked issues from body for match in re.findall(r'#(\d+)', body_text): linked.append(f"#{match}") - pr_head_sha = (pr.get("head") or {}).get("sha") - pr_author = (pr.get("user") or {}).get("login") + pr_head_sha = pr.get("head_sha") + pr_author = pr.get("author") # Determine review block status req_non_author = False @@ -2951,8 +3043,8 @@ def gitea_review_pr( "number": pr["number"], "title": pr["title"], "author": pr_author, - "base": pr["base"]["ref"], - "head": pr["head"]["ref"], + "base": pr["base"], + "head": pr["head"], "mergeable": pr.get("mergeable"), "linked_issues": list(set(linked)), "labels": labels, @@ -2993,6 +3085,16 @@ def gitea_review_pr( if inventory_msg: report_lines.append(inventory_msg) else: + pagination = ( + prs.get("pagination") if isinstance(prs, dict) else {} + ) or {} + has_finality_metadata = bool( + pagination.get("inventory_complete") + or ( + pagination.get("is_final_page") + and not pagination.get("has_more") + ) + ) inventory_trust_gate = review_proofs.pr_inventory_trust_gate( prs if inventory_attempted else None, remote=remote, @@ -3001,7 +3103,7 @@ def gitea_review_pr( state="open", authenticated_profile=profile, local_remote_url=_local_git_remote_url(remote), - has_finality_metadata=True, + has_finality_metadata=has_finality_metadata, ) report_lines.extend( review_proofs.format_pr_inventory_trust_gate_report( diff --git a/review_proofs.py b/review_proofs.py index be71582..7630fbe 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -968,6 +968,90 @@ def assess_mutation_ledger_report( } +def assess_list_prs_pagination_proof( + list_prs_response: dict | list | None, + *, + inventory_complete_claimed: bool = False, +) -> dict: + """#340: verify ``gitea_list_prs`` pagination metadata proves inventory scope. + + Accepts the wrapped ``{"prs": [...], "pagination": {...}}`` response from + ``gitea_list_prs``. Legacy bare lists fail closed when completeness is + claimed. Single-page fetches may not claim full inventory unless + ``inventory_complete`` or ``is_final_page`` is true. + """ + reasons: list[str] = [] + if list_prs_response is None: + reasons.append("gitea_list_prs response missing; fail closed") + return {"proven": False, "block": True, "reasons": reasons, "pagination": None} + + if isinstance(list_prs_response, list): + pagination = None + if inventory_complete_claimed: + reasons.append( + "bare PR list lacks pagination metadata; cannot prove inventory " + "completeness" + ) + return { + "proven": not reasons, + "block": bool(reasons), + "reasons": reasons, + "pagination": pagination, + } + + if not isinstance(list_prs_response, dict): + reasons.append("gitea_list_prs response is not a dict or list; fail closed") + return {"proven": False, "block": True, "reasons": reasons, "pagination": None} + + prs = list_prs_response.get("prs") + pagination = list_prs_response.get("pagination") + if not isinstance(prs, list): + reasons.append("gitea_list_prs response missing 'prs' list") + if not isinstance(pagination, dict): + reasons.append("gitea_list_prs response missing 'pagination' metadata") + return { + "proven": False, + "block": True, + "reasons": reasons, + "pagination": pagination if isinstance(pagination, dict) else None, + } + + required_keys = ("page", "per_page", "returned_count", "has_more", "is_final_page") + for key in required_keys: + if key not in pagination: + reasons.append(f"pagination metadata missing '{key}'") + + if inventory_complete_claimed: + complete = pagination.get("inventory_complete") is True + final_page = pagination.get("is_final_page") is True and pagination.get("has_more") is False + if not (complete or final_page): + reasons.append( + "inventory completeness claimed but pagination metadata does not " + "prove final page (inventory_complete or is_final_page with " + "has_more=false required)" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "pagination": pagination, + } + + +def _prs_from_list_response(list_prs_response: list | dict | None) -> list | None: + """Normalize ``gitea_list_prs`` output to a PR list.""" + if list_prs_response is None: + return None + if isinstance(list_prs_response, list): + return list_prs_response + if isinstance(list_prs_response, dict): + prs = list_prs_response.get("prs") + return prs if isinstance(prs, list) else None + return None + + def assess_role_boundary(proof=None, *, task_role=None, namespaces_used=None, justification=None): """Assess reviewer/author role separation for blind queue workflows. @@ -1810,14 +1894,19 @@ def pr_inventory_trust_gate( "queue_target_lock": lock_status, } - if list_prs_response is None or not isinstance(list_prs_response, list): + prs_list = _prs_from_list_response(list_prs_response) + pagination_meta = ( + list_prs_response.get("pagination") + if isinstance(list_prs_response, dict) else {} + ) or {} + if prs_list is None: return { "status": "inventory_error", - "reasons": ["PR list response is invalid (not a list or None)"], + "reasons": ["PR list response is invalid (missing prs list or None)"], "corroborated": False, } - if len(list_prs_response) > 0: + if len(prs_list) > 0: return { "status": "trusted_nonempty", "reasons": [], @@ -1848,6 +1937,13 @@ def pr_inventory_trust_gate( corroborated = False if has_finality_metadata: corroborated = True + elif pagination_meta.get("inventory_complete") is True: + corroborated = True + elif ( + pagination_meta.get("is_final_page") is True + and pagination_meta.get("has_more") is False + ): + corroborated = True elif corroboration_open_pr_counter == 0: corroborated = True else: diff --git a/tests/test_capability_stop_terminal.py b/tests/test_capability_stop_terminal.py index 5588f33..4100630 100644 --- a/tests/test_capability_stop_terminal.py +++ b/tests/test_capability_stop_terminal.py @@ -99,12 +99,16 @@ class TestCapabilityStopTerminal(unittest.TestCase): self.assertIn("Cannot perform reviewer task", str(ctx.exception)) self.assertIn("review_pr", str(ctx.exception)) - @patch("mcp_server.api_get_all", return_value=[]) + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request", return_value={"login": "jcwalker3"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_list_prs_allowed_after_author_task_clears_stale_denial( - self, _auth, _api, _get_all, + self, _auth, _api, mock_fetch, ): + mock_fetch.return_value = ([], { + "page": 1, "per_page": 50, "returned_count": 0, + "has_more": False, "next_page": None, "is_final_page": True, + }) with patch.dict(os.environ, self._env()): denied = mcp_server.gitea_resolve_task_capability( task="review_pr", remote="prgs" @@ -120,7 +124,7 @@ class TestCapabilityStopTerminal(unittest.TestCase): self.assertFalse(capability_stop_terminal.is_active()) prs = mcp_server.gitea_list_prs(remote="prgs") - self.assertEqual(prs, []) + self.assertEqual(prs["prs"], []) @patch("mcp_server.api_request", return_value={"login": "jcwalker3"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") diff --git a/tests/test_list_prs_pagination.py b/tests/test_list_prs_pagination.py new file mode 100644 index 0000000..2f8b78a --- /dev/null +++ b/tests/test_list_prs_pagination.py @@ -0,0 +1,153 @@ +"""Tests for gitea_list_prs pagination metadata (#340).""" +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import gitea_auth # noqa: E402 +from mcp_server import gitea_list_prs # noqa: E402 +from review_proofs import assess_list_prs_pagination_proof # noqa: E402 + +FAKE_AUTH = "Basic dGVzdDp0ZXN0" +URL = "https://gitea.example.com/api/v1/repos/o/r/pulls?state=open" + +SAMPLE_PR = { + "number": 1, + "title": "PR 1", + "state": "open", + "head": {"ref": "branch1"}, + "base": {"ref": "master"}, + "mergeable": True, + "updated_at": "2026-07-05T12:00:00Z", +} + + +class TestApiFetchPage(unittest.TestCase): + @patch("gitea_auth.api_request") + def test_final_page_metadata(self, mock_req): + mock_req.return_value = [SAMPLE_PR] + items, meta = gitea_auth.api_fetch_page(URL, FAKE_AUTH, page=1, limit=50) + self.assertEqual(len(items), 1) + self.assertFalse(meta["has_more"]) + self.assertTrue(meta["is_final_page"]) + self.assertIsNone(meta["next_page"]) + + @patch("gitea_auth.api_request") + def test_full_page_has_more(self, mock_req): + mock_req.return_value = [{"id": i} for i in range(2)] + items, meta = gitea_auth.api_fetch_page(URL, FAKE_AUTH, page=1, limit=2) + self.assertEqual(len(items), 2) + self.assertTrue(meta["has_more"]) + self.assertFalse(meta["is_final_page"]) + self.assertEqual(meta["next_page"], 2) + + +class TestGiteaListPrsPagination(unittest.TestCase): + @patch("mcp_server.api_fetch_page") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_explicit_page_returns_has_more(self, _auth, mock_fetch): + mock_fetch.return_value = ( + [SAMPLE_PR], + { + "page": 1, + "per_page": 50, + "returned_count": 1, + "has_more": False, + "next_page": None, + "is_final_page": True, + }, + ) + with patch.dict(os.environ, {}, clear=True): + result = gitea_list_prs(page=1, per_page=50) + self.assertEqual(len(result["prs"]), 1) + self.assertFalse(result["pagination"]["inventory_complete"]) + self.assertTrue(result["pagination"]["is_final_page"]) + + @patch("mcp_server.api_fetch_page") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_multi_page_fetch_all_marks_inventory_complete(self, _auth, mock_fetch): + mock_fetch.side_effect = [ + ( + [dict(SAMPLE_PR, number=i) for i in (1, 2)], + { + "page": 1, + "per_page": 2, + "returned_count": 2, + "has_more": True, + "next_page": 2, + "is_final_page": False, + }, + ), + ( + [dict(SAMPLE_PR, number=3)], + { + "page": 2, + "per_page": 2, + "returned_count": 1, + "has_more": False, + "next_page": None, + "is_final_page": True, + }, + ), + ] + with patch.dict(os.environ, {}, clear=True): + result = gitea_list_prs(per_page=2) + self.assertEqual([p["number"] for p in result["prs"]], [1, 2, 3]) + self.assertTrue(result["pagination"]["inventory_complete"]) + self.assertEqual(result["pagination"]["pages_fetched"], 2) + self.assertEqual(result["pagination"]["total_count"], 3) + + +class TestListPrsPaginationProof(unittest.TestCase): + def test_wrapped_final_page_passes_completeness_claim(self): + response = { + "prs": [], + "pagination": { + "page": 1, + "per_page": 50, + "returned_count": 0, + "has_more": False, + "next_page": None, + "is_final_page": True, + "inventory_complete": True, + "pages_fetched": 1, + "total_count": 0, + }, + } + result = assess_list_prs_pagination_proof( + response, inventory_complete_claimed=True + ) + self.assertTrue(result["proven"]) + + def test_bare_list_blocks_completeness_claim(self): + result = assess_list_prs_pagination_proof( + [{"number": 1}], inventory_complete_claimed=True + ) + self.assertFalse(result["proven"]) + self.assertIn("pagination metadata", " ".join(result["reasons"])) + + def test_single_page_without_complete_blocks_claim(self): + response = { + "prs": [{"number": 1}], + "pagination": { + "page": 1, + "per_page": 50, + "returned_count": 1, + "has_more": True, + "next_page": 2, + "is_final_page": False, + "inventory_complete": False, + "pages_fetched": 1, + "total_count": None, + }, + } + result = assess_list_prs_pagination_proof( + response, inventory_complete_claimed=True + ) + self.assertFalse(result["proven"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 0b01a54..18ebb80 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -399,40 +399,49 @@ class TestMirrorRefs(unittest.TestCase): # --------------------------------------------------------------------------- class TestListPRs(unittest.TestCase): - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_list_prs(self, _auth, mock_api): - mock_api.return_value = [ - { + def test_list_prs(self, _auth, mock_fetch): + mock_fetch.return_value = ( + [{ "number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1"}, "base": {"ref": "main"}, "html_url": "http://url1", "mergeable": True, "updated_at": "2026-07-05T12:00:00Z" - } - ] + }], + { + "page": 1, "per_page": 50, "returned_count": 1, + "has_more": False, "next_page": None, "is_final_page": True, + }, + ) with patch.dict(os.environ, {}, clear=True): result = gitea_list_prs() - self.assertEqual(len(result), 1) - self.assertEqual(result[0]["number"], 1) - self.assertEqual(result[0]["head"], "branch1") - self.assertNotIn("url", result[0]) + self.assertEqual(len(result["prs"]), 1) + self.assertEqual(result["prs"][0]["number"], 1) + self.assertEqual(result["prs"][0]["head"], "branch1") + self.assertNotIn("url", result["prs"][0]) + self.assertTrue(result["pagination"]["inventory_complete"]) # Staleness fields for queue reconciliation (#166) - self.assertEqual(result[0]["updated_at"], "2026-07-05T12:00:00Z") + self.assertEqual(result["prs"][0]["updated_at"], "2026-07-05T12:00:00Z") - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_list_prs_reveal_opt_in_includes_url(self, _auth, mock_api): - mock_api.return_value = [ - { + def test_list_prs_reveal_opt_in_includes_url(self, _auth, mock_fetch): + mock_fetch.return_value = ( + [{ "number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1"}, "base": {"ref": "main"}, "html_url": "http://url1", "mergeable": True, "updated_at": "2026-07-05T12:00:00Z" - } - ] + }], + { + "page": 1, "per_page": 50, "returned_count": 1, + "has_more": False, "next_page": None, "is_final_page": True, + }, + ) with patch.dict(os.environ, {"GITEA_MCP_REVEAL_ENDPOINTS": "1"}, clear=True): result = gitea_list_prs() - self.assertEqual(result[0]["url"], "http://url1") + self.assertEqual(result["prs"][0]["url"], "http://url1") # --------------------------------------------------------------------------- diff --git a/tests/test_pr_queue_inventory.py b/tests/test_pr_queue_inventory.py index 82998a4..aeb8bc4 100644 --- a/tests/test_pr_queue_inventory.py +++ b/tests/test_pr_queue_inventory.py @@ -16,38 +16,50 @@ import gitea_config FAKE_AUTH = "Basic dGVzdDp0ZXN0" + +def _final_page_fetch(items): + return (items, { + "page": 1, + "per_page": 50, + "returned_count": len(items), + "has_more": False, + "next_page": None, + "is_final_page": True, + }) + + class TestPRQueueInventory(unittest.TestCase): - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_author_profile_can_list_open_prs(self, mock_get_profile, _auth, mock_get_all): + def test_author_profile_can_list_open_prs(self, mock_get_profile, _auth, mock_fetch): mock_get_profile.return_value = { "profile_name": "gitea-author", "allowed_operations": ["read", "gitea.pr.comment"], "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [ + mock_fetch.return_value = _final_page_fetch([ {"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1"}, "base": {"ref": "master"}, "mergeable": True} - ] + ]) result = gitea_list_prs(state="open", remote="prgs") - self.assertEqual(len(result), 1) - self.assertEqual(result[0]["number"], 1) + self.assertEqual(len(result["prs"]), 1) + self.assertEqual(result["prs"][0]["number"], 1) - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") @patch("gitea_config.is_runtime_switching_enabled", return_value=True) - def test_author_profile_can_produce_pending_reviewer_queue_report(self, mock_switch, mock_get_profile, _auth, mock_api, mock_get_all): + def test_author_profile_can_produce_pending_reviewer_queue_report(self, mock_switch, mock_get_profile, _auth, mock_api, mock_fetch): mock_get_profile.return_value = { "profile_name": "gitea-author", "allowed_operations": ["read"], "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [ + mock_fetch.return_value = _final_page_fetch([ { "number": 1, "title": "PR 1", @@ -72,7 +84,7 @@ class TestPRQueueInventory(unittest.TestCase): "body": "", "updated_at": "2026-07-05T11:00:00Z" } - ] + ]) mock_api.return_value = {"login": "jcwalker3"} # whoami result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs") @@ -86,18 +98,18 @@ class TestPRQueueInventory(unittest.TestCase): # updated_at surfaced for reconciliation (#166) self.assertIn("Updated: 2026-07-05T10:00:00Z", result["message"]) - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_author_profile_never_calls_mutation_tools_during_inventory(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_author_profile_never_calls_mutation_tools_during_inventory(self, mock_get_profile, _auth, mock_api, mock_fetch): mock_get_profile.return_value = { "profile_name": "gitea-author", "allowed_operations": ["read"], "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [] + mock_fetch.return_value = _final_page_fetch([]) mock_api.return_value = {"login": "jcwalker3"} gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs") @@ -105,20 +117,20 @@ class TestPRQueueInventory(unittest.TestCase): method = call.args[0] self.assertEqual(method, "GET") - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_reviewer_profile_can_proceed_from_inventory_to_review_gates(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_reviewer_profile_can_proceed_from_inventory_to_review_gates(self, mock_get_profile, _auth, mock_api, mock_fetch): mock_get_profile.return_value = { "profile_name": "gitea-reviewer", "allowed_operations": ["read", "approve", "review"], "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [ + mock_fetch.return_value = _final_page_fetch([ {"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "other_user"}} - ] + ]) # mock_api: inventory whoami, eligibility whoami, eligibility PR, # POST review (#244: state + visible-verdict GET reviews). mock_api.side_effect = [ @@ -152,19 +164,19 @@ class TestPRQueueInventory(unittest.TestCase): self.assertIn("Repository:", result["message"]) self.assertIn("Successfully submitted review", result["message"]) - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") @patch("gitea_config.is_runtime_switching_enabled", return_value=False) - def test_static_runtime_rejects_or_hides_profile_activation(self, mock_switch, mock_get_profile, _auth, mock_api, mock_get_all): + def test_static_runtime_rejects_or_hides_profile_activation(self, mock_switch, mock_get_profile, _auth, mock_api, mock_fetch): mock_get_profile.return_value = { "profile_name": "gitea-author", "allowed_operations": ["read"], "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [] + mock_fetch.return_value = _final_page_fetch([]) mock_api.return_value = {"login": "jcwalker3"} result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs") @@ -200,11 +212,11 @@ class TestPRQueueInventory(unittest.TestCase): self.assertFalse(result["eligible"]) self.assertEqual(mock_api.call_count, 0) - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_inventory_failure_output_is_redacted_no_raw_exception_leak(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_inventory_failure_output_is_redacted_no_raw_exception_leak(self, mock_get_profile, _auth, mock_api, mock_fetch): """Inventory failure path must use redaction and not leak raw exception text.""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -213,7 +225,7 @@ class TestPRQueueInventory(unittest.TestCase): "base_url": None, } # Exception contains something that should be redacted or not leaked raw - mock_get_all.side_effect = Exception("connection error token supersecrettoken123 at https://internal.example/repo") + mock_fetch.side_effect = Exception("connection error token supersecrettoken123 at https://internal.example/repo") mock_api.return_value = {"login": "jcwalker3"} result = gitea_review_pr(pr_number=42, event="APPROVE", remote="prgs") @@ -228,11 +240,11 @@ class TestPRQueueInventory(unittest.TestCase): # uses project's redaction pattern (token prefix -> [REDACTED]) # even if url leaks, the exception is redacted per pattern - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_inventory_failure_output_redacts_real_service_urls(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_inventory_failure_output_redacts_real_service_urls(self, mock_get_profile, _auth, mock_api, mock_fetch): """Failure paths must fully redact real service hostnames and URLs from inventory output.""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -240,7 +252,7 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.side_effect = Exception("failed to connect to https://gitea.prgs.cc/api/v1/repos/x") + mock_fetch.side_effect = Exception("failed to connect to https://gitea.prgs.cc/api/v1/repos/x") mock_api.return_value = {"login": "jcwalker3"} result = gitea_review_pr(pr_number=42, event="APPROVE", remote="prgs") @@ -250,11 +262,11 @@ class TestPRQueueInventory(unittest.TestCase): self.assertIn("[REDACTED_URL]", msg) self.assertNotIn("gitea.prgs.cc", msg) - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_author_profiles_can_perform_read_only_pr_inventory(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_author_profiles_can_perform_read_only_pr_inventory(self, mock_get_profile, _auth, mock_api, mock_fetch): """Author profiles with read can perform read-only PR inventory (report produced, no mutations).""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -262,9 +274,9 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [ + mock_fetch.return_value = _final_page_fetch([ {"number": 7, "title": "Some PR", "state": "open", "head": {"ref": "f", "sha": "def"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "someone"}, "labels": [], "body": ""} - ] + ]) mock_api.return_value = {"login": "jcwalker3"} result = gitea_review_pr(pr_number=7, event="APPROVE", remote="prgs") @@ -278,12 +290,12 @@ class TestPRQueueInventory(unittest.TestCase): self.assertEqual(call.args[0], "GET") @patch("mcp_server._local_git_remote_url") - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") def test_empty_inventory_runs_trust_gate_and_blocks_without_trusted_empty( - self, mock_get_profile, _auth, mock_api, mock_get_all, mock_local_url + self, mock_get_profile, _auth, mock_api, mock_fetch, mock_local_url ): mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -291,7 +303,7 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [] + mock_fetch.return_value = _final_page_fetch([]) mock_api.return_value = {"login": "jcwalker3"} mock_local_url.return_value = None @@ -309,12 +321,12 @@ class TestPRQueueInventory(unittest.TestCase): self.assertNotIn("Open PRs found: 0 (trusted_empty)", msg) @patch("mcp_server._local_git_remote_url") - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") def test_empty_inventory_trusted_empty_when_gate_passes( - self, mock_get_profile, _auth, mock_api, mock_get_all, mock_local_url + self, mock_get_profile, _auth, mock_api, mock_fetch, mock_local_url ): mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -322,7 +334,7 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [] + mock_fetch.return_value = _final_page_fetch([]) mock_api.return_value = {"login": "jcwalker3"} mock_local_url.return_value = ( "https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools.git" @@ -341,11 +353,11 @@ class TestPRQueueInventory(unittest.TestCase): self.assertIn("Open PRs found: 0 (trusted_empty)", msg) @patch("mcp_server._local_git_remote_url") - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_author_profiles_cannot_approve_request_changes_merge_or_bypass_gates(self, mock_get_profile, _auth, mock_api, mock_get_all, mock_local_url): + def test_author_profiles_cannot_approve_request_changes_merge_or_bypass_gates(self, mock_get_profile, _auth, mock_api, mock_fetch, mock_local_url): """Author profiles still cannot approve, request_changes, merge, or bypass gates even with inventory.""" mock_local_url.return_value = ( "https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools.git" @@ -357,7 +369,7 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [] + mock_fetch.return_value = _final_page_fetch([]) mock_api.return_value = {"login": "jcwalker3"} result = gitea_review_pr(pr_number=1, event=event, remote="prgs") @@ -377,10 +389,10 @@ class TestPRQueueInventory(unittest.TestCase): # Tests use direct list/view calls + inventory path to ensure fields and # live data are available for detection without trusting prior handoffs. - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_r5_s1_stale_prior_handoff_merged_but_live_pr_open(self, mock_get_profile, _auth, mock_get_all): + def test_r5_s1_stale_prior_handoff_merged_but_live_pr_open(self, mock_get_profile, _auth, mock_fetch): """Scenario 1: stale prior handoff says merged but live PR list says open.""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -388,17 +400,17 @@ class TestPRQueueInventory(unittest.TestCase): "forbidden_operations": [], "base_url": None, } - mock_get_all.return_value = [{ + mock_fetch.return_value = _final_page_fetch([{ "number": 99, "title": "Stale merge claim", "state": "open", "head": {"ref": "f99", "sha": "sha99"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "other"}, "labels": [], "body": "Fixes #200", "updated_at": "2026-07-05T22:00:00Z" - }] + }]) prs = gitea_list_prs(state="open", remote="prgs") - self.assertEqual(len(prs), 1) - self.assertEqual(prs[0]["state"], "open") - self.assertIn("updated_at", prs[0]) # enables staleness check vs prior "merged" + self.assertEqual(len(prs["prs"]), 1) + self.assertEqual(prs["prs"][0]["state"], "open") + self.assertIn("updated_at", prs["prs"][0]) # enables staleness check vs prior "merged" @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @@ -424,11 +436,11 @@ class TestPRQueueInventory(unittest.TestCase): self.assertIsNotNone(pr.get("closed_at")) # live closed would contradict prior "open" handoff - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_r5_s3_list_prs_and_view_pr_disagree(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_r5_s3_list_prs_and_view_pr_disagree(self, mock_get_profile, _auth, mock_api, mock_fetch): """Scenario 3: gitea_list_prs and gitea_view_pr disagree on state/details.""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -437,12 +449,12 @@ class TestPRQueueInventory(unittest.TestCase): "base_url": None, } # list says open - mock_get_all.return_value = [{ + mock_fetch.return_value = _final_page_fetch([{ "number": 77, "title": "Disagree", "state": "open", "head": {"ref": "f77", "sha": "s77"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "x"}, "labels": [], "body": "", "updated_at": "2026-07-05T23:00:00Z" - }] + }]) # view says closed (simulating inconsistency) mock_api.return_value = { "number": 77, "title": "Disagree", "state": "closed", @@ -453,7 +465,7 @@ class TestPRQueueInventory(unittest.TestCase): } listed = gitea_list_prs(state="open", remote="prgs") viewed = gitea_view_pr(pr_number=77, remote="prgs") - self.assertEqual(listed[0]["state"], "open") + self.assertEqual(listed["prs"][0]["state"], "open") self.assertEqual(viewed["state"], "closed") # caller must reconcile; mismatch is hard blocker per rules @@ -483,11 +495,11 @@ class TestPRQueueInventory(unittest.TestCase): self.assertIsNone(pr.get("merge_commit_sha")) # post-merge view must surface missing commit for detection - @patch("mcp_server.api_get_all") + @patch("mcp_server.api_fetch_page") @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @patch("mcp_server.get_profile") - def test_r5_s5_linked_issue_remains_open_after_claimed_merge(self, mock_get_profile, _auth, mock_api, mock_get_all): + def test_r5_s5_linked_issue_remains_open_after_claimed_merge(self, mock_get_profile, _auth, mock_api, mock_fetch): """Scenario 5: linked issue remains open after claimed merge.""" mock_get_profile.return_value = { "profile_name": "gitea-author", @@ -496,12 +508,12 @@ class TestPRQueueInventory(unittest.TestCase): "base_url": None, } # PR claims merge, links #300 - mock_get_all.return_value = [{ + mock_fetch.return_value = _final_page_fetch([{ "number": 300, "title": "PR for #300", "state": "closed", "head": {"ref": "f300", "sha": "s3"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "u"}, "labels": [], "body": "Fixes #300", "updated_at": "2026-07-05T20:20:00Z" - }] + }]) # But linked issue (via list_issues or view) still open - here we use view_pr body to confirm link # For this test we assert the PR data shows link, caller must check issue state live mock_api.return_value = { From c41a698a08342366de4717ef6fea4184c262cd71 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:26:58 -0400 Subject: [PATCH 5/7] feat(reports): classify git fetch as a git ref mutation (closes #297) Reviewer reports treated 'git fetch prgs master' as read-only diagnostics while claiming 'Git/worktree mutations: None'. Fetch edits no files but updates remote-tracking refs, so hiding it under diagnostics understates what the run mutated. Add git_ref_mutating_commands() classifying fetch, pull, push, merge, update-ref, remote update, branch -d/-D, and reset --hard as git ref mutations, and assess_git_ref_mutation_report() which fails a report when ref-updating commands ran but the report lacks a non-none 'Git ref mutations' entry, omits 'fetched /' for a targeted fetch, still claims 'Git/worktree mutations: None', or lists the command under read-only diagnostics. Tests cover fetch-only review, missing ledger entry, missing fetch target, contradictory none-claim, read-only misclassification, worktree creation (not a ref mutation), merge, cleanup branch deletion, no-mutation blocked handoff, and dict command-log entries. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 116 ++++++++++++++++++++++++++ tests/test_git_ref_mutation_report.py | 101 ++++++++++++++++++++++ 2 files changed, 217 insertions(+) create mode 100644 tests/test_git_ref_mutation_report.py diff --git a/review_proofs.py b/review_proofs.py index be71582..b031532 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -2816,3 +2816,119 @@ def assess_email_disclosure( for email in emails ], } + + +# --------------------------------------------------------------------------- +# Git ref mutations (#297) +# --------------------------------------------------------------------------- + +_GIT_REF_MUTATIONS_FIELD_RE = re.compile( + r"^\s*git ref mutations\s*:\s*(.+?)\s*$", re.I | re.M) +_GIT_WORKTREE_MUTATIONS_NONE_RE = re.compile( + r"^\s*git/worktree mutations\s*:\s*none\b", re.I | re.M) + +_GIT_REF_MUTATING_FIRST_WORDS = frozenset( + {"fetch", "pull", "push", "merge", "update-ref"}) +_GIT_REF_MUTATING_PREFIXES = ("remote update", "branch -d", "branch -D", + "reset --hard") + + +def _command_text(entry): + if isinstance(entry, dict): + return str(entry.get("command") or "").strip() + return str(entry or "").strip() + + +def _git_subcommand_line(command): + tokens = command.split() + if not tokens or tokens[0] != "git": + return None + return " ".join(tokens[1:]) + + +def git_ref_mutating_commands(command_log): + """Return logged commands that update git refs (#297). + + ``git fetch`` and friends edit no files but move refs; they are never + read-only diagnostics. + """ + out = [] + for entry in command_log or []: + command = _command_text(entry) + rest = _git_subcommand_line(command) + if rest is None: + continue + first = rest.split()[0] if rest.split() else "" + if first in _GIT_REF_MUTATING_FIRST_WORDS: + out.append(command) + continue + if any(rest.startswith(prefix) for prefix in _GIT_REF_MUTATING_PREFIXES): + out.append(command) + return out + + +def _fetch_targets(ref_commands): + """Extract ``/`` targets from git fetch commands.""" + targets = [] + for command in ref_commands: + rest = _git_subcommand_line(command) or "" + tokens = rest.split() + if not tokens or tokens[0] != "fetch": + continue + args = [t for t in tokens[1:] if not t.startswith("-")] + if len(args) >= 2: + targets.append(f"{args[0]}/{args[1]}") + return targets + + +def assess_git_ref_mutation_report(report_text, *, command_log=None): + """#297: ref updates are Git ref mutations, not read-only diagnostics. + + When the command log holds ref-updating commands, the report must carry + a non-none ``Git ref mutations`` entry (naming ``fetched + /`` for targeted fetches), may not claim ``Git/worktree + mutations: None``, and may not list the command as read-only. + """ + text = report_text or "" + lower = text.lower() + reasons = [] + ref_commands = git_ref_mutating_commands(command_log) + fetch_targets = _fetch_targets(ref_commands) + + if ref_commands: + field = _GIT_REF_MUTATIONS_FIELD_RE.search(text) + value = field.group(1).strip().lower() if field else None + if not value or value == "none": + reasons.append( + "ref-updating commands ran but the report has no " + "'Git ref mutations' entry" + ) + for target in fetch_targets: + if "fetched" not in lower or target.lower() not in lower: + reasons.append( + "git fetch ran; report must state " + f"'Git ref mutations: fetched {target}'" + ) + if _GIT_WORKTREE_MUTATIONS_NONE_RE.search(text): + reasons.append( + "'Git/worktree mutations: None' rejected: ref-updating " + "commands ran" + ) + for line in text.splitlines(): + if "read-only" not in line.lower(): + continue + for command in ref_commands: + if command.lower() in line.lower(): + reasons.append( + "read-only diagnostics may not include ref-updating " + f"command '{command}'" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "ref_mutating_commands": ref_commands, + "fetch_targets": fetch_targets, + } diff --git a/tests/test_git_ref_mutation_report.py b/tests/test_git_ref_mutation_report.py new file mode 100644 index 0000000..c8ce29c --- /dev/null +++ b/tests/test_git_ref_mutation_report.py @@ -0,0 +1,101 @@ +"""Git ref mutations must be reported, never hidden as diagnostics (#297). + +``git fetch`` updates remote-tracking refs even though it edits no files. +Reports that ran fetch (or any ref-updating command) must carry a +``Git ref mutations`` entry and may not claim ``Git/worktree mutations: +None`` or classify the fetch as read-only diagnostics. +""" +import sys +import unittest + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import review_proofs + + +def _assess(report, commands): + return review_proofs.assess_git_ref_mutation_report( + report, command_log=commands) + + +class TestGitRefMutationDetection(unittest.TestCase): + def test_fetch_only_review_with_proper_ledger_passes(self): + report = "\n".join([ + "Git ref mutations: fetched prgs/master", + "Review decision: APPROVE", + ]) + res = _assess(report, ["git fetch prgs master"]) + self.assertTrue(res["proven"], res["reasons"]) + self.assertEqual(res["ref_mutating_commands"], ["git fetch prgs master"]) + + def test_fetch_without_ref_mutation_line_is_blocked(self): + report = "Review decision: APPROVE\nMutations: None\n" + res = _assess(report, ["git fetch prgs master"]) + self.assertFalse(res["proven"]) + self.assertTrue( + any("git ref mutations" in r.lower() for r in res["reasons"])) + + def test_fetch_with_remote_branch_requires_fetched_target_in_report(self): + report = "Git ref mutations: some refs updated\n" + res = _assess(report, ["git fetch prgs master"]) + self.assertFalse(res["proven"]) + self.assertTrue( + any("prgs/master" in r for r in res["reasons"])) + + def test_git_worktree_mutations_none_rejected_when_fetch_occurred(self): + report = "\n".join([ + "Git ref mutations: fetched prgs/master", + "Git/worktree mutations: None", + ]) + res = _assess(report, ["git fetch prgs master"]) + self.assertFalse(res["proven"]) + self.assertTrue( + any("git/worktree mutations" in r.lower() for r in res["reasons"])) + + def test_fetch_classified_as_read_only_diagnostics_rejected(self): + report = "\n".join([ + "Git ref mutations: fetched prgs/master", + "Read-only diagnostics: git fetch prgs master, git status", + ]) + res = _assess(report, ["git fetch prgs master"]) + self.assertFalse(res["proven"]) + self.assertTrue( + any("read-only" in r.lower() for r in res["reasons"])) + + def test_worktree_creation_is_not_a_ref_mutation(self): + report = "Worktree mutations: created branches/review-pr9\n" + res = _assess(report, ["git worktree add branches/review-pr9 prgs/master"]) + self.assertTrue(res["proven"], res["reasons"]) + self.assertEqual(res["ref_mutating_commands"], []) + + def test_merge_command_counts_as_ref_mutation(self): + report = "Review decision: APPROVE\n" + res = _assess(report, ["git merge --no-ff feat/x"]) + self.assertFalse(res["proven"]) + self.assertIn("git merge --no-ff feat/x", res["ref_mutating_commands"]) + + def test_cleanup_branch_delete_counts_as_ref_mutation(self): + report = "Cleanup mutations: deleted branch feat/x\n" + res = _assess(report, ["git branch -d feat/x"]) + self.assertFalse(res["proven"]) + self.assertIn("git branch -d feat/x", res["ref_mutating_commands"]) + + def test_no_mutation_blocked_handoff_passes(self): + report = "\n".join([ + "Git/worktree mutations: None", + "Current status: blocked handoff, no mutations performed", + ]) + res = _assess(report, ["git status --porcelain", "git log --oneline -1"]) + self.assertTrue(res["proven"], res["reasons"]) + self.assertEqual(res["ref_mutating_commands"], []) + + def test_command_log_dict_entries_supported(self): + report = "Git ref mutations: fetched prgs/master\n" + res = _assess(report, [{"command": "git fetch prgs master"}]) + self.assertTrue(res["proven"], res["reasons"]) + self.assertEqual( + res["ref_mutating_commands"], ["git fetch prgs master"]) + + +if __name__ == "__main__": + unittest.main() From 873bcd06e50108645f1329e61e192901ad47c1fe Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:27:02 -0400 Subject: [PATCH 6/7] docs: pin GlitchTip filing orchestrator contract (Closes #153) --- .../glitchtip-to-gitea-workflow-design.md | 46 +++++++++++++++---- docs/mcp-client-registration.md | 11 ++++- tests/test_external_mcp_registration_docs.py | 38 +++++++++++++++ 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/docs/architecture/glitchtip-to-gitea-workflow-design.md b/docs/architecture/glitchtip-to-gitea-workflow-design.md index 08e4571..3106b7d 100644 --- a/docs/architecture/glitchtip-to-gitea-workflow-design.md +++ b/docs/architecture/glitchtip-to-gitea-workflow-design.md @@ -1,21 +1,34 @@ -# GlitchTip-to-Gitea Issue Filing Workflow Design +# GlitchTip-to-Gitea Issue Filing Workflow Contract -- **Status:** Design (no implementation in this repo) -- **Issue:** #74 (parent umbrella: #75) +- **Status:** Contract for the library-only implementation in + `Scaled-Tech-Consulting/mcp-control-plane` (#57) +- **Issue:** #153 (supersedes #74/#78 as the Gitea-Tools tracker) - **Related:** #78 (deduplication design, child of #74) -- **Date:** 2026-07-02 +- **Date:** 2026-07-07 ## 1. Boundary and Orchestration * **GlitchTip-to-Gitea filing is NOT a GlitchTip MCP capability.** The `glitchtip-mcp` boundary remains strictly read-only per ADR-0001. -* The filing capability lives in an **orchestrator / runbook / release workflow**. It **composes** separate GlitchTip **read** tools (from the `glitchtip-mcp` server) and Gitea **issue** tools (from the `gitea-mcp` server). +* The filing capability lives in a **library-only orchestrator** in + `mcp-control-plane` and is not exposed through `glitchtip-mcp`. +* The orchestrator composes the real GlitchTip read path with Gitea + issue-write tools. It must not use mocked GlitchTip issue data in production + filing paths. * The orchestrator **must not centralize credentials** into a single server. The GlitchTip MCP holds only GlitchTip tokens, and the Gitea MCP holds only Gitea tokens. +* If a future MCP surface exposes filing, it must be a separate write-boundary + server/profile with explicit Gitea issue-write permission and the same audit + gates. It must not be added to the read-only `glitchtip-mcp` surface. ## 2. Invocation and Safety * **Explicit invocation only:** There is no automatic, unsupervised filing in phase 1. A human or an explicitly-triggered automation must initiate the workflow. * **Dry-run / Preview required:** The orchestrator must present a preview of the drafted Gitea issue (title, body, labels) and obtain explicit confirmation before calling the Gitea mutation tool to file the issue. -* **Gitea Profile Checks & Audit Logging:** The actual Gitea issue creation relies on `gitea-mcp`, and therefore inherently subjects the mutation to Gitea profile checks and audit logging as standard. +* **Gitea Profile Checks & Audit Logging:** The actual Gitea issue creation + relies on `gitea-mcp`, and therefore must pass Gitea profile checks and + fail-closed mutation audit before create/link/comment actions. +* **Deduplication before create:** The orchestrator must run the + GlitchTip-to-Gitea dedup/linking logic before any Gitea create action. Create, + link, and skip decisions must be represented in tests. ## 3. Gitea Issue Format @@ -51,6 +64,23 @@ To prevent PII or secret leakage into Gitea, the orchestrator and the underlying The principle is: **"Link, don't dump"**. The generated issue acts as an alert/pointer, while the raw context remains protected inside GlitchTip. -## 5. Deduplication and Linking (Deferred) +## 5. Deduplication and Linking -Deduplication logic (e.g. searching existing Gitea issues, managing GlitchTip issue IDs, and race condition handling) is specifically handled by **Issue #78** and will augment this design. +Deduplication logic (e.g. searching existing Gitea issues, managing GlitchTip +issue IDs, and race condition handling) is integrated into the library-only +filing orchestrator in `mcp-control-plane`. The orchestrator must reuse that +dedup/linking path instead of duplicating or bypassing it. + +## 6. Gitea-Tools Acceptance Contract for #153 + +Gitea-Tools does not host the filing implementation. This repository owns the +operator-facing contract: + +* `glitchtip-mcp` remains read-only and exposes only GlitchTip inspection tools. +* Filing is implemented in `mcp-control-plane`, not in this Gitea MCP runtime. +* Filing uses the real GlitchTip read path, not mocked issue data. +* Dedup runs before any Gitea create action. +* Create/link/skip decisions and audit failure are covered by orchestrator tests. +* Mutation audit fails closed before any Gitea create, link, or comment mutation. +* Any future MCP exposure requires a separate write-boundary server/profile and + must not add Gitea write credentials to `glitchtip-mcp`. diff --git a/docs/mcp-client-registration.md b/docs/mcp-client-registration.md index e83d5f3..f0477dd 100644 --- a/docs/mcp-client-registration.md +++ b/docs/mcp-client-registration.md @@ -95,7 +95,14 @@ back to shell commands, raw service APIs, or unrelated MCP servers. - Do not register `jenkins-write-mcp` until an operator approves a trigger profile; no shipped profile carries trigger capability by default. - `glitchtip-mcp` remains read-only. It must not file or mutate Gitea issues. -- GlitchTip-to-Gitea filing is a separate orchestrator that composes GlitchTip - read tools with Gitea issue-write tools. +- GlitchTip-to-Gitea filing is a separate library-only orchestrator in + `mcp-control-plane` that composes the real GlitchTip read path with Gitea + issue-write tools. +- The filing orchestrator must run GlitchTip/Gitea dedup before any Gitea + create action, and its create/link/skip decisions must be covered by tests. +- The filing orchestrator must fail closed on mutation-audit failure before + any Gitea create, link, or comment mutation. +- If filing is ever MCP-exposed, it must use a separate write-boundary + server/profile. It must not be exposed by `glitchtip-mcp`. - Gitea credentials never enter Jenkins or GlitchTip runtimes. - Jenkins and GlitchTip credentials never enter the Gitea MCP runtime. diff --git a/tests/test_external_mcp_registration_docs.py b/tests/test_external_mcp_registration_docs.py index 40c8eba..8225bb6 100644 --- a/tests/test_external_mcp_registration_docs.py +++ b/tests/test_external_mcp_registration_docs.py @@ -59,6 +59,22 @@ def test_registration_doc_preserves_boundaries(): assert phrase in text +def test_registration_doc_pins_glitchtip_filing_boundary(): + text = " ".join(_doc_text().split()) + for phrase in ( + "GlitchTip-to-Gitea filing is a separate library-only orchestrator in " + "`mcp-control-plane`", + "real GlitchTip read path", + "dedup before any Gitea create action", + "create/link/skip decisions", + "fail closed on mutation-audit failure before any Gitea create, link, " + "or comment mutation", + "must use a separate write-boundary server/profile", + "must not be exposed by `glitchtip-mcp`", + ): + assert phrase in text + + def test_registration_doc_separates_jenkins_trigger_from_read_surface(): text = _doc_text() assert "jenkins_trigger_build" in text @@ -100,3 +116,25 @@ def test_related_docs_keep_jenkins_trigger_off_read_surface(): assert "jenkins-write-mcp" in text assert "jenkins_mcp.write_server" in text assert "jenkins-mcp" in text + + +def test_glitchtip_filing_contract_doc_covers_issue_153_acceptance(): + text = ( + REPO_ROOT / "docs" / "architecture" / + "glitchtip-to-gitea-workflow-design.md" + ).read_text(encoding="utf-8") + flattened = " ".join(text.split()) + for phrase in ( + "**Status:** Contract for the library-only implementation in " + "`Scaled-Tech-Consulting/mcp-control-plane` (#57)", + "**Issue:** #153", + "not exposed through `glitchtip-mcp`", + "real GlitchTip read path", + "must not use mocked GlitchTip issue data", + "separate write-boundary server/profile", + "fail-closed mutation audit before create/link/comment actions", + "Deduplication before create", + "Create, link, and skip decisions must be represented in tests", + "Mutation audit fails closed", + ): + assert phrase in flattened From 456e46d3fc39d5cbdaffcac50bfabcaf2182fbb8 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:32:15 -0400 Subject: [PATCH 7/7] feat: enforce session hard-stop after terminal review mutation (#332) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the single-terminal-decision machinery (#211) to session-level hard-stop semantics: - terminal_review_hard_stop_reasons: after a live terminal verdict, only the merge sequence for the same approved PR may continue; REQUEST_CHANGES stops the run; operator-approved corrections re-open the review path only, never a cross-PR merge. - gitea_merge_pr: new Gate 2b consults the hard-stop before any API call, so a run can never verdict one PR and merge another. - gitea_mark_final_review_decision: hard-stop check with explicit stop guidance naming the consumed terminal mutation; plus duplicate REQUEST_CHANGES suppression — an unresolved request-changes at the current head SHA refuses a second request-changes (fail closed when feedback cannot be verified). - tests/test_terminal_review_hard_stop.py: 13 tests covering the acceptance criteria; existing TestSubmitPrReview setups stub the new feedback fetch. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 83 +++++++++++ tests/test_mcp_server.py | 21 ++- tests/test_terminal_review_hard_stop.py | 187 ++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 tests/test_terminal_review_hard_stop.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 7eea703..e8cbf46 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -1677,6 +1677,51 @@ def record_live_review_mutation(pr_number: int, action: str, review_id: int | No _save_review_decision_lock(lock) +def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[str]: + """Session hard-stop after a terminal live review mutation (#332). + + After a terminal verdict is consumed in this run, the only permitted + continuation is the merge sequence for the same PR that was approved. + A REQUEST_CHANGES (or an approval of a different PR) blocks every + further review/mark-ready/merge mutation. An operator-approved + correction (#211) re-opens the review path only — never a cross-PR + merge. + + *operation* is one of 'merge', 'mark_ready', or 'review'. + Returns [] when the operation may proceed. + """ + lock = _load_review_decision_lock() + if lock is None: + return [] + terminals = [ + m for m in (lock.get("live_mutations") or []) + if m.get("action") in _TERMINAL_REVIEW_ACTIONS + ] + if not terminals: + return [] + last = terminals[-1] + if ( + operation == "merge" + and last.get("action") == "approve" + and last.get("pr_number") == pr_number + ): + return [] + if operation in ("mark_ready", "review") and lock.get("correction_authorized"): + return [] + if last.get("action") == "approve": + guidance = ( + f"only the merge sequence for approved PR " + f"#{last.get('pr_number')} may continue" + ) + else: + guidance = "the session must stop and produce a final report" + return [ + "terminal review mutation already consumed in this run " + f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} " + "(fail closed, #332)" + ] + + # Patterns scrubbed from any surfaced error text so a credential can never leak. _SECRET_PREFIXES = ("token ", "Basic ") @@ -2092,6 +2137,9 @@ def gitea_mark_final_review_decision( } org = resolved_org repo = resolved_repo + hard_stop = terminal_review_hard_stop_reasons(pr_number, "mark_ready") + if hard_stop: + return {"marked_ready": False, "reasons": hard_stop} if lock.get("live_mutations") and not lock.get("correction_authorized"): return { "marked_ready": False, @@ -2109,6 +2157,33 @@ def gitea_mark_final_review_decision( f"{sorted(_REVIEW_ACTIONS)}" ], } + if action == "request_changes": + # Duplicate request-changes suppression (#332): an unresolved + # REQUEST_CHANGES at the current head must not be duplicated. + feedback = gitea_get_pr_review_feedback( + pr_number, remote=remote, org=org, repo=repo) + if not feedback.get("success"): + return { + "marked_ready": False, + "reasons": [ + "could not verify existing request-changes state for " + f"PR #{pr_number}; refusing to submit a possibly " + "duplicate REQUEST_CHANGES (fail closed, #332)" + ], + } + if ( + feedback.get("has_blocking_change_requests") + and not feedback.get("review_feedback_stale") + ): + return { + "marked_ready": False, + "reasons": [ + f"PR #{pr_number} already has an unresolved " + "REQUEST_CHANGES at the current head SHA; do not " + "duplicate the request-changes review (fail closed, " + "#332)" + ], + } lock["final_review_decision_ready"] = True lock["ready_pr_number"] = pr_number lock["ready_action"] = action @@ -2678,6 +2753,14 @@ def gitea_merge_pr( ) return result + # Gate 2b — session hard-stop after a terminal review mutation (#332): + # merge may only continue for the same PR that was just approved. + # Local in-process check; zero API calls. + hard_stop = terminal_review_hard_stop_reasons(pr_number, "merge") + if hard_stop: + reasons.extend(hard_stop) + return result + # Gate 3 — reuse #14 eligibility (identity + profile + merge-allowed + # author + self-merge block + open + mergeable/unknown fail-closed). # Read-only GETs only. diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 0b01a54..d61f658 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -47,6 +47,21 @@ import mcp_server FAKE_AUTH = "Basic dGVzdDp0ZXN0" +_NO_BLOCKER_FEEDBACK = { + "success": True, + "has_blocking_change_requests": False, + "review_feedback_stale": False, +} + + +def _mark_request_changes_ready(pr_number=8, **kwargs): + """Mark a request_changes decision ready with the #332 duplicate- + suppression feedback fetch stubbed to 'no existing blocker'.""" + with patch("mcp_server.gitea_get_pr_review_feedback", + return_value=dict(_NO_BLOCKER_FEEDBACK)): + return gitea_mark_final_review_decision( + pr_number, "request_changes", **kwargs) + def _formal_review(reviewer, verdict, sha="abc123", review_id=1): return { @@ -1775,7 +1790,7 @@ class TestSubmitPrReview(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_request_changes_succeeds_when_eligible(self, _auth, mock_api): - gitea_mark_final_review_decision(8, "request_changes", remote="prgs") + _mark_request_changes_ready(remote="prgs") mock_api.side_effect = [ {"login": "reviewer-bot"}, self._pr("author-bot"), {"id": 9, "state": "REQUEST_CHANGES"}, @@ -1798,7 +1813,7 @@ class TestSubmitPrReview(unittest.TestCase): self.assertEqual(post_calls[0].args[3]["event"], "REQUEST_CHANGES") def test_request_changes_blocked_without_eligibility(self): - gitea_mark_final_review_decision(8, "request_changes", remote="prgs") + _mark_request_changes_ready(remote="prgs") with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) as _a, \ patch("mcp_server.api_request") as mock_api: mock_api.side_effect = [{"login": "reviewer-bot"}, self._pr("author-bot")] @@ -2080,7 +2095,7 @@ class TestSubmitPrReview(unittest.TestCase): reason="operator approved correcting mistaken approve", operator_authorized=True, ) - gitea_mark_final_review_decision(8, "request_changes", remote="prgs") + _mark_request_changes_ready(remote="prgs") second = gitea_submit_pr_review( pr_number=8, action="request_changes", remote="prgs", final_review_decision_ready=True, diff --git a/tests/test_terminal_review_hard_stop.py b/tests/test_terminal_review_hard_stop.py new file mode 100644 index 0000000..bd5109b --- /dev/null +++ b/tests/test_terminal_review_hard_stop.py @@ -0,0 +1,187 @@ +"""Tests for the session hard-stop after a terminal review mutation (#332). + +Extends the single-terminal-decision machinery (#211): after a live +REQUEST_CHANGES the run must stop; after an APPROVE only the merge sequence +for that same PR may continue; a different PR can never be reviewed or +merged in the same run; duplicate REQUEST_CHANGES at an unchanged head is +suppressed. +""" +import os +import unittest +from unittest.mock import patch + +import mcp_server + + +def _lock(mutations=None, correction=False): + return { + "task": "review_pr", + "remote": "prgs", + "session_pid": os.getpid(), + "session_profile": "prgs-reviewer", + "session_profile_lock": "", + "final_review_decision_ready": False, + "ready_pr_number": None, + "ready_action": None, + "ready_expected_head_sha": None, + "ready_remote": None, + "ready_org": None, + "ready_repo": None, + "live_mutations": list(mutations or []), + "correction_authorized": correction, + "correction_reason": None, + } + + +def _seed(mutations=None, correction=False): + mcp_server._save_review_decision_lock(_lock(mutations, correction)) + + +APPROVED_A = {"pr_number": 5, "action": "approve", "review_id": 1, + "review_state": "approve"} +RC_A = {"pr_number": 5, "action": "request_changes", "review_id": 2, + "review_state": "request_changes"} + + +class TestTerminalHardStopReasons(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + + def test_no_lock_no_reasons(self): + mcp_server._save_review_decision_lock(None) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(5, "merge"), []) + + def test_no_terminal_mutation_no_reasons(self): + _seed() + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(5, "merge"), []) + + def test_request_changes_blocks_everything(self): + _seed([RC_A]) + for op in ("merge", "mark_ready", "review"): + for pr in (5, 6): + reasons = mcp_server.terminal_review_hard_stop_reasons(pr, op) + self.assertTrue(reasons, f"{op}/{pr}") + self.assertIn("request_changes on PR #5", reasons[0]) + self.assertIn("#332", reasons[0]) + + def test_approve_allows_same_pr_merge_only(self): + _seed([APPROVED_A]) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(5, "merge"), []) + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(6, "merge")) + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(6, "mark_ready")) + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(6, "review")) + + def test_correction_reopens_review_path_only(self): + _seed([RC_A], correction=True) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(5, "mark_ready"), []) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(5, "review"), []) + # correction never opens a cross-PR merge + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(6, "merge")) + + +class TestMergeHardStopWiring(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + + def test_merge_blocked_after_request_changes(self): + _seed([RC_A]) + result = mcp_server.gitea_merge_pr( + pr_number=6, confirmation="MERGE PR 6", remote="prgs") + self.assertFalse(result["performed"]) + self.assertTrue( + any("#332" in r for r in result["reasons"]), result["reasons"]) + + def test_merge_of_other_pr_blocked_after_approve(self): + _seed([APPROVED_A]) + result = mcp_server.gitea_merge_pr( + pr_number=6, confirmation="MERGE PR 6", remote="prgs") + self.assertFalse(result["performed"]) + self.assertTrue( + any("#332" in r for r in result["reasons"]), result["reasons"]) + + +class TestMarkFinalHardStopWiring(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + + def test_mark_ready_blocked_after_terminal_mutation(self): + _seed([RC_A]) + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="approve", remote="prgs") + self.assertFalse(result["marked_ready"]) + self.assertTrue( + any("#332" in r for r in result["reasons"]), result["reasons"]) + + +def _feedback(blocking, stale=False, success=True): + return { + "success": success, + "has_blocking_change_requests": blocking, + "review_feedback_stale": stale, + "current_head_sha": "abc123", + } + + +class TestDuplicateRequestChangesSuppression(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + + def test_duplicate_request_changes_blocked_at_same_head(self): + _seed() + with patch.object(mcp_server, "gitea_get_pr_review_feedback", + return_value=_feedback(blocking=True, stale=False)): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="request_changes", remote="prgs") + self.assertFalse(result["marked_ready"]) + self.assertTrue( + any("duplicate" in r for r in result["reasons"]), + result["reasons"]) + + def test_request_changes_allowed_when_prior_blocker_stale(self): + _seed() + with patch.object(mcp_server, "gitea_get_pr_review_feedback", + return_value=_feedback(blocking=True, stale=True)): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="request_changes", remote="prgs") + self.assertTrue(result["marked_ready"], result.get("reasons")) + + def test_request_changes_allowed_when_no_blocker(self): + _seed() + with patch.object(mcp_server, "gitea_get_pr_review_feedback", + return_value=_feedback(blocking=False)): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="request_changes", remote="prgs") + self.assertTrue(result["marked_ready"], result.get("reasons")) + + def test_request_changes_fails_closed_when_feedback_unavailable(self): + _seed() + with patch.object(mcp_server, "gitea_get_pr_review_feedback", + return_value=_feedback(blocking=False, + success=False)): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="request_changes", remote="prgs") + self.assertFalse(result["marked_ready"]) + self.assertTrue( + any("could not verify" in r for r in result["reasons"]), + result["reasons"]) + + def test_approve_does_not_fetch_feedback(self): + _seed() + with patch.object(mcp_server, "gitea_get_pr_review_feedback", + side_effect=AssertionError("must not be called")): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="approve", remote="prgs") + self.assertTrue(result["marked_ready"], result.get("reasons")) + + +if __name__ == "__main__": + unittest.main()