feat(skill): extract review-merge-pr workflow from monolithic SKILL.md
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) <[email protected]>
This commit is contained in:
Claude Opus 4.8
@@ -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 <branch>`), 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 <repo> 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 -- <file>`, 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 <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
|
||||
|
||||
Reference in New Issue
Block a user