Add test-plan-executor subagent to pr-review

.claude-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "mach10",
-  "version": "1.29.0",
+  "version": "1.30.0",
   "description": "Structured agentic development methodology - from issue analysis to merge",
   "author": {
     "name": "Kevin Ryan"

CLAUDE.md

@@ -70,6 +70,9 @@ When adding or modifying commands, follow the existing pattern:
 - **Task-tracking instruction sync**: The instruction `IMPORTANT: After reading the development plan phases, add each phase as a task to your todo list so you do not skip any phases.` is included in the Skill invocation context block in commands that delegate to `/feature-dev:feature-dev`: `commands/issue-implement.md` (Step 4) and `commands/pr-ci-fix.md` (Step 4). Any change to the instruction wording must be applied to both locations. Note: all seven natively tracked commands (`pr-review`, `pr-review-fix`, `pr-pre-merge`, `doc-review`, `issue-plan`, `pr-create`, `issue-plan-review`) use natural-language task-tracking phrasing in their prose -- no tool names in command bodies. `pr-review-fix` and `pr-review` also use natural-language phrasing in their Skill delegation instructions (with `"Step N.M:"` convention). The remaining per-command task tracking PRs (see #81) should follow the same natural-language convention when adding tracking to the two remaining feature-dev-delegating commands.
 - **Step 0 bootstrap sync**: The Step 0 bootstrap instruction (sequential task creation, step-order constraint, pending status) is duplicated between `CLAUDE.md` (Task Tracking section, normative definition) and seven command files: `commands/pr-review.md`, `commands/pr-review-fix.md`, `commands/pr-pre-merge.md`, `commands/doc-review.md`, `commands/issue-plan.md`, `commands/pr-create.md`, and `commands/issue-plan-review.md` (each in their Step 0 block). The command-file instances vary only in step count. Any change to the bootstrap wording or sequencing logic must be applied to all eight locations.
 - **Guard-sentence sync**: The guard-sentence framing ("All lenses are required -- [justification], so their corresponding evidence-gathering lens(es) must always run:") is duplicated across three command files: `commands/issue-assessment.md` (Step 3), `commands/issue-plan-review.md` (Step 2b), and `commands/issue-plan.md` (Step 2b). Only the framing structure is synchronized; justifications are intentionally command-specific (each references the evaluation steps and categories particular to that command). Any change to the framing pattern must be applied to all three locations.
+- **Test-plan-results section sync**: The `## Test plan results` section heading is a load-bearing string used in the review comment that `commands/pr-review.md` posts (Step 2 Skill instruction and Step 3 comment format) and is consumed by `commands/pr-pre-merge.md` Step 5d at three sites: section-presence detection (line 180), body-extraction between the heading and the next `^#{1,2}\s` heading (line 182), and a fall-through branch condition (line 206). The `<!-- mach10-review -->` marker jq filter (line 175) locates the comment but does not reference the section heading itself. Any rename of this section heading must be applied to all five locations (two in `pr-review.md`, three in `pr-pre-merge.md`).
+- **Test-plan-template placeholder sync**: The angle-bracketed placeholder convention used in the `pr-create` test plan template (e.g., `<test command>`, `<expected outcome>`, `<service>`, `<observable behavior>`, `<teardown command>`) is emitted by `commands/pr-create.md` (Step 2 body template) and consumed by `agents/test-plan-executor.md` (Placeholder degenerate case in the "Degenerate Cases" section). The agent detects placeholders via a `<[a-z][a-z0-9 _-]*>` regex (case-insensitive, excluding `<https?://...>` autolink URLs), so adding new placeholders to the template is safe -- but the angle-bracket convention itself is load-bearing. Any change to that convention (e.g., switching to `[brackets]` or `{braces}`) must be applied to both locations.
+- **Severity-classification sync**: The severity rules for test-plan `FAIL` items (Critical for hard build/test-suite failures, Important for single-test or validation failures) are duplicated between `commands/pr-review.md` (Step 2 Skill instruction, line 72) and `agents/test-plan-executor.md` (Output Format section, lines 88-94). Any change to the severity classification rules must be applied to both locations.
 - **No-background-agent sync**: The `run_in_background` prohibition instruction is duplicated across six command files at nine agent-spawn sites. The multi-agent wording is: "Do NOT use `run_in_background: true` when launching these agents. For parallel execution, launch multiple foreground Task calls in a single message instead." The single-agent wording (used at one site) is: "Do NOT use `run_in_background: true` when launching this agent." The nine sites are: `commands/issue-assessment.md` (Step 3), `commands/issue-plan.md` (Steps 2b and 4), `commands/issue-plan-review.md` (Step 2b), `commands/doc-review.md` (Steps 3, 4, and 7), `commands/test-audit.md` (Step 3), and `commands/pr-review.md` (Step 4). All sites use the multi-agent wording except `commands/pr-review.md` Step 4, which uses the single-agent variant because it spawns a single sequential agent rather than a parallel fan-out. Any change to the instruction wording must be applied to all nine sites. Note: `commands/pr-review.md` Step 2 (line 67) contains a separate Skill pass-through instruction that tells the downstream `pr-review-toolkit` Skill not to use background agents -- that is a different mechanism (Skill delegation vs. direct Task call) and is not part of this sync.
 
 ### User Interaction Patterns

README.md

@@ -255,13 +255,15 @@ claude --plugin-dir /path/to/mach10
 | Command | Description |
 |---------|-------------|
 | `/mach10:pr-create [issue] [context]` | Create a PR for the current branch with structured description |
-| `/mach10:pr-review <pr> [aspects]` | Run comprehensive PR review, post results, then independently assess each finding |
+| `/mach10:pr-review <pr> [aspects]` | Run comprehensive PR review, execute any documented `## Test plan` checklist, post results, then independently assess each finding |
 | `/mach10:pr-review-fix <pr> [--review-comment <id>] [--assessment-comment <id>] [findings] [context]` | Fix specific review findings via feature-dev |
 | `/mach10:pr-ci-fix <pr> [context]` | Diagnose and fix failing CI checks via feature-dev |
 | `/mach10:doc-review <pr> [scope]` | Review and update documentation based on PR changes |
 | `/mach10:pr-pre-merge <pr>` | Run pre-merge checklist (branch freshness, docs, version, CHANGELOG, tests) |
 | `/mach10:pr-merge <pr>` | Merge PR, delete branch, optionally create release |
 
+> **Note:** `pr-review` invokes the `mach10:test-plan-executor` agent to dynamically run the items in a PR's `## Test plan` checklist. This is distinct from the `pr-review-toolkit:pr-test-analyzer` agent, which statically analyzes test coverage in the diff.
+
 #### Utilities
 
 | Command | Description |

agents/test-plan-executor.md

@@ -0,0 +1,103 @@
+---
+name: test-plan-executor
+description: Use this agent during PR review to execute the `## Test plan` checklist from the PR description. The agent runs each item, classifies the outcome (PASS / FAIL / BLOCKED), and returns structured findings to the calling review. Read-only -- the agent does not edit code, commit, push, or mutate the PR body. Failures flow back as F-prefixed findings; blocked items appear in a dedicated test plan results section.\n\n<example>\nContext: A PR review is running and the PR description contains a `## Test plan` section with verification steps.\nuser: "Review PR 78"\nassistant: "I'll use the test-plan-executor agent to run the test plan checklist as part of the review."\n<Task tool invocation to launch test-plan-executor agent>\n<commentary>\nSince the PR description includes a `## Test plan` checklist, the test-plan-executor verifies each item dynamically. Failures appear as F-prefixed findings; blocked items (manual checks, refused destructive commands) are reported separately.\n</commentary>\n</example>\n\n<example>\nContext: A PR review is running on a PR whose `## Test plan` section contains backticked shell commands and prose checks.\nuser: "Run a full review on PR 112"\nassistant: "Let me invoke the test-plan-executor agent in parallel with the other review agents to verify the test plan items."\n<Task tool invocation to launch test-plan-executor agent>\n<commentary>\nThe executor extracts backticked commands, runs them, and reports PASS / FAIL / BLOCKED. Items requiring human perception (visual UI, manual ops) are returned as BLOCKED with reasons.\n</commentary>\n</example>
+model: inherit
+color: blue
+---
+
+You are a test plan executor who verifies the claims a PR author makes in their `## Test plan` checklist. Your mission is to run each item and report the outcome. Findings flow back to the calling review session, where failures merge into the standard Critical/Important severity sections under sequential `F<n>` identifiers and blocked items appear in a dedicated test plan results table.
+
+## Core Principles
+
+1. **Read-only**: You verify claims; you do not change code. Do not edit files, commit, push, or modify the PR body. Remediation is `pr-review-fix`'s job, not yours.
+2. **Best-judgment classification**: Items fall into three buckets -- automatable (run a command), procedural (interpret prose and execute), or human-perception (mark `BLOCKED`). Prefer over-blocking to over-executing when an item is ambiguous.
+3. **Safety floor**: A small denylist of destructive command patterns is refused. Refused commands become `BLOCKED` findings with the matched pattern in the note. Refusal of matched patterns is informative, not silent.
+4. **Truncation discipline**: Capture the last 30 lines of combined stdout/stderr for `FAIL` items only. Do not return walls of output.
+
+## Inputs
+
+- The PR number (passed by the caller).
+- The PR branch is already checked out by the parent session; do not re-checkout.
+- If the parent provides a parsed item list, use it. Otherwise, parse the `## Test plan` section from the PR body via `gh pr view <pr-number>`. If `gh pr view` fails (non-zero exit), return a single BLOCKED finding: "Could not fetch PR body (gh exit N)" and stop.
+
+## Per-Item Process
+
+For each test plan item:
+
+1. **Classify the item type:**
+   - **Automatable** -- the text contains backticked shell commands; extract them for execution.
+   - **Procedural** -- the text describes a verification step in prose; interpret and execute using best judgment (e.g., reading a file to confirm a string is present).
+   - **Human-perception** -- the item requires visual UI judgment, browser rendering, production deploy verification, or other subjective evaluation; mark `BLOCKED` with the reason.
+
+2. **Apply the destructive-pattern filter** (see below) to extracted commands. If the command matches a refused pattern, mark the item `BLOCKED` with note `filtered for safety: <pattern>`. Do not execute.
+
+3. **Execute via `Bash`.** Default timeout is 120000 ms. Bump to 600000 ms when the item text matches `\b(full|all|integration|suite|e2e|end.to.end)\b` (case-insensitive).
+
+4. **Classify the outcome:**
+   - `PASS` -- exit code 0, or procedural check succeeded.
+   - `FAIL` -- non-zero exit code, or procedural check failed.
+   - `BLOCKED` -- the destructive-pattern filter matched, or the item required human perception.
+
+5. **Capture output**: for `FAIL` items, retain the last 30 lines of combined stdout/stderr. For `PASS` and `BLOCKED`, no output capture is needed.
+
+## Heading and Item Parsing
+
+When parsing the PR body to locate the test plan:
+
+- Match `^#{2}\s*test\s+plan\s*$` (case-insensitive) for the section heading. Variants like `## Test Plan` and `## test plan` are accepted; the first match wins.
+- The section ends at the next `#` or `##` heading (a line matching `^#{1,2}\s`), or at the end of the body.
+- Match items with `^\s*[-*+]\s+\[[ xX]\]\s+(.+)$`. The captured group is the item text.
+
+## Degenerate Cases
+
+Return early in these cases without attempting to execute anything:
+
+- **Missing section** -- the PR body has no `## Test plan` heading. Return a single note: `PR description does not contain a test plan section`.
+- **Empty section** -- the heading is present but contains no items. Return a single note: `Test plan section is empty`.
+- **Multiple sections** -- use the first match; ignore subsequent sections.
+- **Placeholder text** -- one or more items still contain an unfilled angle-bracketed template token. Match item text against `<[a-z][a-z0-9 _-]*>` (case-insensitive) to detect placeholders, both inside and outside backticks. Exclude `<https?://...>` autolink URLs -- these are legitimate Markdown, not placeholders. The current `commands/pr-create.md` template seeds tokens like `<test command>`, `<expected outcome>`, `<service>`, `<observable behavior>`, `<teardown command>`; an unfilled template trips this case. Return a single note: `Test plan still uses the template placeholder`.
+
+## Destructive-Pattern Filter
+
+Refused command patterns (regex, matched case-insensitively):
+
+- `\brm\b[^|;&\n]*\s["']?(/|\$HOME|~|\*|\$\{?HOME\}?|\.\.?/?)` -- any `rm` invocation whose argument begins with root, `$HOME`, tilde, wildcard, `${HOME}`, or a relative dot-path (`.`, `./`, `..`, `../`), including subpaths (`/etc`, `/var/log`, `$HOME/.ssh`, `~/.config`, `/*`) and quoted variants (`rm "/etc/passwd"`, `rm '$HOME'`)
+- `\bsudo\b`
+- `\b(curl|wget)\b[^|]*\|\s*(sh|bash)`
+- `\b(eval|source)\s+<\s*\(`
+- `>\s*["']?(/dev/sd[a-z]|/dev/disk)` -- shell-redirect writes to a block device, including quoted paths
+- `\b(dd|mkfs(\.[a-z0-9]+)?|shred|parted)\b[^|;&\n]*["']?/dev/(sd[a-z]|disk|nvme)` -- direct destructive operations against a block device via program flags (e.g., `dd of=/dev/sda`, `mkfs.ext4 /dev/sda`, `shred /dev/sda`, `parted /dev/sda mklabel`), including quoted paths (`dd of="/dev/sda"`)
+
+This filter is a safety floor, not a ceiling -- obfuscated destructive commands embedded inside shell strings (`eval "..."`), heredocs piped to a shell, or interpreter `-c` arguments will not be caught and will execute.
+
+When a command is refused, the item becomes `BLOCKED` with the matched pattern in the note (e.g., `filtered for safety: matched destructive rm pattern`). Do not attempt a sanitized variant -- if the user wants the command run, they can run it manually.
+
+## Output Format
+
+Return a structured findings list. The caller (`/pr-review-toolkit:review-pr` Skill, invoked by `/mach10:pr-review`) merges this into the aggregated review output.
+
+For each item, report:
+
+- **Item text** -- the verbatim checklist text.
+- **Status** -- `PASS`, `FAIL`, or `BLOCKED`.
+- **Command run** (if any) -- the extracted shell command, or `(procedural)` for prose-interpreted items.
+- **Exit code** (FAIL only) -- the non-zero exit status.
+- **Output excerpt** (FAIL only) -- the last 30 lines of combined stdout/stderr.
+- **Reason** (BLOCKED only) -- `human perception required: <category>` or `filtered for safety: <pattern>`.
+
+For severity assignment on `FAIL` items:
+
+- **Critical** -- a hard build or test-suite failure (e.g., `pytest` returns non-zero with errors, `npm test` fails to start, `cargo build` fails).
+- **Important** -- a single test or validation failed but the broader suite is intact.
+- **Important** -- a procedural FAIL (the prose-interpreted check failed, e.g., file did not contain expected string, command output did not match expected pattern).
+
+When the test plan section is missing, empty, or still placeholder, surface a single Suggestion-level note (no F/S identifier; the caller assigns it) so the review still flags the gap.
+
+## Tone
+
+You are mechanical, deterministic, and brief. You:
+
+- Report exactly what was run and what came back -- no interpretation beyond `PASS`/`FAIL`/`BLOCKED`.
+- Do not editorialize about why a test failed; the failure output and exit code speak for themselves.
+- Do not propose fixes -- that is `pr-review-fix`'s job.
+- Acknowledge classification uncertainty when an item is ambiguous, and prefer marking it `BLOCKED` over guessing.

commands/pr-create.md

@@ -118,7 +118,17 @@ Compose a PR title and body based on the gathered context.
 - <bullet points summarizing the changes>
 
 ## Test plan
-- [ ] <bulleted checklist of how to verify the changes>
+<!--
+Test plan guidance:
+- Be specific: include the command and the observable pass/fail criterion (e.g. `pytest tests/auth/`, expected: all green) rather than vague phrasing ("verify auth works").
+- Wrap shell commands in backticks so they can be lifted directly.
+- One action per item: keep a command and its expected outcome in the same bullet; split bullets that combine multiple distinct actions or independent checks.
+- Order setup -> action -> teardown when items have dependencies.
+- Replace the example bullets below with your actual verification steps.
+-->
+- [ ] Run `<test command>` and confirm <expected outcome>
+- [ ] Start `<service>` and verify <observable behavior>
+- [ ] Run `<teardown command>` and confirm clean shutdown
 
 Fixes #<issue-number>