diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index c19aa0a..2345b3d 100644 --- a/.claude-plugin/plugin.json +++ b/.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" diff --git a/CLAUDE.md b/CLAUDE.md index fbbcaa7..a175b91 100644 --- a/CLAUDE.md +++ b/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 `` 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., ``, ``, ``, ``, ``) 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 `` 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 diff --git a/README.md b/README.md index 55e1a6c..ecc45e8 100644 --- a/README.md +++ b/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 [aspects]` | Run comprehensive PR review, post results, then independently assess each finding | +| `/mach10:pr-review [aspects]` | Run comprehensive PR review, execute any documented `## Test plan` checklist, post results, then independently assess each finding | | `/mach10:pr-review-fix [--review-comment ] [--assessment-comment ] [findings] [context]` | Fix specific review findings via feature-dev | | `/mach10:pr-ci-fix [context]` | Diagnose and fix failing CI checks via feature-dev | | `/mach10:doc-review [scope]` | Review and update documentation based on PR changes | | `/mach10:pr-pre-merge ` | Run pre-merge checklist (branch freshness, docs, version, CHANGELOG, tests) | | `/mach10:pr-merge ` | 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 | diff --git a/agents/test-plan-executor.md b/agents/test-plan-executor.md new file mode 100644 index 0000000..47c2fa0 --- /dev/null +++ b/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\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\n\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\n\n\n\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\n\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\n +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` 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 `. 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: `. 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 `` autolink URLs -- these are legitimate Markdown, not placeholders. The current `commands/pr-create.md` template seeds tokens like ``, ``, ``, ``, ``; 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: ` or `filtered for safety: `. + +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. diff --git a/commands/pr-create.md b/commands/pr-create.md index 09147b2..dc91230 100644 --- a/commands/pr-create.md +++ b/commands/pr-create.md @@ -118,7 +118,17 @@ Compose a PR title and body based on the gathered context. - ## Test plan -- [ ] + +- [ ] Run `` and confirm +- [ ] Start `` and verify +- [ ] Run `` and confirm clean shutdown Fixes # diff --git a/commands/pr-pre-merge.md b/commands/pr-pre-merge.md index 505c675..c12a79e 100644 --- a/commands/pr-pre-merge.md +++ b/commands/pr-pre-merge.md @@ -167,16 +167,52 @@ Using the PR context gathered in Step 4, work through each item. For each, repor ### 5d. Tests -Run the project's test suite: +Check whether `/mach10:pr-review` recently ran the test plan and surface those results when fresh, instead of running the suite again. -``` -# Auto-detect test runner -# Python: pytest, unittest -# JavaScript: npm test, jest -# etc. -``` +1. Fetch the most recent `` comment on the PR: + + ``` + gh pr view --json comments --jq '[.comments[] | select(.body | contains(""))] | last' + ``` + + The query returns the comment object (with `createdAt` and `body`), or `null` if none was found. Check the `gh pr view` exit code: on non-zero exit (auth expired, rate limit, network error, JSON parse error), surface a CLI note "Could not fetch PR comments (gh exit N); running suite as a precaution" and fall through to step 3 below. Only proceed to step 2 when the command succeeded (exit 0). + +2. **Marker found and the comment body contains a `## Test plan results` section with at least one parseable per-item table row:** compare the comment's `createdAt` to the latest commit timestamp on the branch. Convert both to Unix epoch seconds before comparing -- string comparison of ISO 8601 timestamps with mixed timezone offsets gives the wrong answer. + + First, validate the section is well-formed. Extract the body between the `## Test plan results` heading and the next heading of the same or higher level (`^#{1,2}\s`, or end of comment), and require at least one parseable table data row -- a line beginning with `|` containing three or more pipe-delimited cells, excluding the header row (`| Item | Status | Notes |` or similar) and the separator row (`|---|---|---|`). If the section heading is present but no parseable rows are found, treat this as a malformed prior result: surface a CLI note "Prior review's test plan results section was malformed; ran suite directly", and fall through to step 3 below. + + Then capture the epoch values: + + - Comment epoch: `date -u -d "" +%s` (where `` is the ISO 8601 string from the JSON above). + - Branch epoch: `git log -1 --format=%ct --no-merges` (committer time of the latest non-merge commit; `--no-merges` excludes merge commits that Step 3 may have created, which would otherwise make a fresh review appear stale). + + Validate that both values are non-empty integers (e.g., `[[ "$comment_epoch" =~ ^[0-9]+$ ]]` and `[[ "$branch_epoch" =~ ^[0-9]+$ ]]`) before comparing. If `comment_epoch` validation fails (BSD `date` without `-d` support, malformed `createdAt`, etc.), treat the result as stale and surface a CLI note "Could not parse comment timestamp; treating as stale". If `branch_epoch` validation fails (unexpected `git log` output, branch with only merge commits, etc.), treat the result as stale and surface a CLI note "Could not determine branch commit timestamp; treating as stale". In either case, proceed to the stale path below -- do not skip the suite based on an invalid comparison. + + Then: + + - **Fresh** (comment epoch >= branch epoch): surface the prior summary line and per-item statuses in CLI output and skip running the auto-detected suite. Record the outcome for the Step 7 report. + - **Stale** (branch epoch > comment epoch, or epoch validation failed above): use `AskUserQuestion` to ask how to proceed: + + - **Re-run pr-review (stops the checklist)**: "Stop this session and run a fresh review (recovery: `/clear` then `/mach10:pr-review `)" + - **Accept stale results**: "Trust the prior summary even though the branch has new commits since" + - **Run suite directly**: "Skip the test plan and run only the auto-detected test suite" + + If the user selects **Re-run pr-review (stops the checklist)**, stop the session and instruct the user to run the recovery command. Leave Step 5 as `in_progress`. + + If the user selects **Accept stale results**, surface the prior summary in CLI output and skip running the suite. Record the outcome for the Step 7 report. + + If the user selects **Run suite directly**, fall through to step 3 below. + +3. **No marker, marker without a `## Test plan results` section, marker with a malformed (no parseable rows) section, or user selected "Run suite directly":** run the project's test suite: + + ``` + # Auto-detect test runner + # Python: pytest, unittest + # JavaScript: npm test, jest + # etc. + ``` -Report results. If tests fail, investigate and report — do NOT silently ignore failures. + Report results. If tests fail, investigate and report -- do NOT silently ignore failures. Mark Step 5 complete. @@ -203,7 +239,7 @@ Present a summary of what was done: - [ ] Documentation: [updated / no changes needed] - [ ] Version: [bumped to X.Y.Z / no version tracking / no changes needed] - [ ] CHANGELOG: [updated / no changelog maintained / no changes needed] -- [ ] Tests: [all passing / N failures noted] +- [ ] Tests: [from pr-review: P passed, F failed, B blocked / from pr-review: all N items blocked (no automated verification) / stale results accepted (P passed, F failed, B blocked) / suite passing / N failures noted] Recommend next step: `/clear` then `/mach10:pr-merge ` diff --git a/commands/pr-review.md b/commands/pr-review.md index 3e0c2c7..61b90ec 100644 --- a/commands/pr-review.md +++ b/commands/pr-review.md @@ -69,6 +69,7 @@ Use the Skill tool to invoke `/pr-review-toolkit:review-pr` with the appropriate - **Always** include this instruction in the Skill invocation: "If the PR has a linked issue (look for issue references like 'Fixes #N', 'Closes #N', 'Resolves #N', 'Part of #N', 'Issue #N', or a bare '#N' in the PR description), include the `feature-completeness-checker` agent alongside the other review agents. This agent verifies that the PR fully implements the requirements from the linked issue's acceptance criteria and implementation plan. Do not launch this agent if no linked issue is detected." - **Always** include this instruction in the Skill invocation: "Label each Critical and Important finding with a sequential F-prefixed identifier (F1, F2, F3, ...) numbered continuously across both sections. Label each Suggestion with a sequential S-prefixed identifier (S1, S2, S3, ...) using a separate counter. Use bold prefixes in the output (e.g., `**F1:** Missing null check`, `**S1:** Consider extracting helper`)." - **Always** include this instruction in the Skill invocation: "IMPORTANT: Create at least one sub-task to track your progress. Use `"Step 2.M: "` as the subject format where M is the sequence number. Use best judgment on granularity based on the review scope." +- **Always** include this instruction in the Skill invocation: "If the PR description contains a `## Test plan` section, also invoke the `mach10:test-plan-executor` agent in parallel with the other review agents. The executor runs each test plan item, classifies outcomes (`PASS` / `FAIL` / `BLOCKED`), and returns findings. Merge `FAIL` items into the standard Critical/Important findings list under sequential `F` identifiers (Critical for hard build/test-suite failures, Important for single-test or validation failures). Render the per-item table in the `## Test plan results` section using the format defined in Step 3 below. If the PR description has no `## Test plan` section, do not invoke the executor; instead surface a single `S` finding ('PR description does not contain a test plan section') in Suggestions and omit the `## Test plan results` section. If the executor agent fails or returns malformed output, omit the `## Test plan results` section and surface a single `S` finding ('Test plan executor failed') in Suggestions." Do NOT attempt to fix any issues -- this session is for review only. @@ -88,6 +89,7 @@ The comment must include: - `` as the very first line of the comment body (this invisible HTML marker enables reliable identification in future sessions) - The complete review findings (Critical, Important, Suggestions, Strengths), including any findings from supplementary agents merged into the appropriate severity categories with inline source attribution (e.g., "per plugin-dev:skill-reviewer") - F/S identifiers on every finding -- Critical and Important findings use `F` numbered sequentially across both sections, Suggestions use `S` with a separate counter (e.g., `**F1:** ...`, `**F2:** ...`, `**S1:** ...`) +- A `## Test plan results` section (positioned after Strengths, before model attribution) when a `## Test plan` section exists in the PR body. The section contains a per-item Markdown table with columns Item / Status / Notes. Statuses are `PASS`, `FAIL`, or `BLOCKED`. Every item appears in the table (so the section reflects full test plan coverage); `FAIL` items also appear in the standard Critical/Important findings list under their `F` identifiers. Omit this section entirely when the PR body has no `## Test plan` section -- in that case, surface a single `S` finding ("PR description does not contain a test plan section") in Suggestions instead. - Model attribution at the bottom (e.g., "Reviewed by Claude Opus 4.6") - A note that this is an automated review