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