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] 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