Add test-plan-executor subagent to pr-review by LeanAndMean · Pull Request #138 · LeanAndMean/mach10

LeanAndMean · 2026-05-08T20:18:42Z

Summary

Adds agents/test-plan-executor.md, a read-only subagent that runs each item in a PR's ## Test plan checklist, classifies the outcome (PASS / FAIL / BLOCKED), applies a destructive-pattern safety filter, and returns structured findings.
Wires the executor into commands/pr-review.md (Step 2 Skill bullet to invoke it in parallel; Step 3 owns the canonical ## Test plan results table format) so FAIL items merge into the existing F<n> finding flow consumable by pr-review-fix.
Strengthens the test plan template in commands/pr-create.md with an HTML guidance block plus three example bullets covering specificity, backticked commands, single-concern items, and setup/action/teardown ordering.
Updates commands/pr-pre-merge.md Step 5d to coordinate with the existing  marker via epoch-second timestamp comparison (fresh: surface prior summary and skip the suite; stale: prompt re-run / accept-stale / run suite directly).
Adds a Test-plan-results section sync entry to CLAUDE.md, disambiguates the executor from pr-review-toolkit:pr-test-analyzer in README.md, and bumps the plugin version 1.29.0 -> 1.30.0.

Test plan

Run /mach10:pr-review against this PR and confirm the executor is invoked, the ## Test plan results table renders, and any FAIL items receive F<n> identifiers in the standard findings list.
On a fixture PR with a backticked passing command (e.g., `git --version`), confirm the item is reported PASS with the command shown.
On a fixture PR with a deliberately failing command (e.g., `false`), confirm the item is reported FAIL with exit code and a truncated last-30-lines output excerpt, and that it appears as an F<n> finding.
On a fixture PR containing a destructive item like `rm -rf /`, confirm it is BLOCKED with filtered for safety: ... and is NOT executed.
On a fixture PR with a prose-only human-perception item (e.g., "Verify the page renders correctly"), confirm it is reported BLOCKED with the perception reason.
On a PR whose body has no ## Test plan section, confirm the executor surfaces a single suggestion-level note rather than running.
After a fresh pr-review run, run /mach10:pr-pre-merge and confirm Step 5d surfaces the prior results and skips the auto-detected suite.
Add a commit on top of a previously-reviewed PR, run /mach10:pr-pre-merge, and confirm it offers the three-option staleness AskUserQuestion (re-run pr-review / accept stale / run suite directly).
Sanity-check the strengthened pr-create.md template by running /mach10:pr-create on a scratch branch and confirming the HTML guidance comment plus example bullets render as expected in the draft body.

Fixes #136

Implements the revised plan for issue 136 as a single stage. Adds a read-only `mach10:test-plan-executor` subagent that runs the items in a PR's `## Test plan` checklist when invoked from `/mach10:pr-review`, classifying each as PASS / FAIL / BLOCKED. Failures flow back through the existing F-identifier convention so `pr-review-fix` handles remediation; blocked items appear in a dedicated `## Test plan results` table in the review comment. Three integration touchpoints land alongside the agent: - `pr-create.md` strengthens the test plan template with an HTML guidance block and 3 example bullets. - `pr-pre-merge.md` Step 5d reads the most recent  comment and skips the test suite when fresh results exist; uses a timezone-safe epoch-second comparison for staleness. - `README.md` notes test plan execution on the pr-review row and disambiguates the new agent from `pr-review-toolkit:pr-test-analyzer` (static coverage analysis vs dynamic execution). CLAUDE.md adds a Test-plan-results section sync entry tracking the cross-file convention between pr-review.md (emit) and pr-pre-merge.md (consume). Plugin version 1.29.0 -> 1.30.0. Co-Authored-By: Claude <noreply@anthropic.com>

LeanAndMean · 2026-05-08T20:30:56Z

PR Review: Add test-plan-executor subagent to pr-review

This is an automated review composed from several specialized review agents (code-reviewer, comment-analyzer, silent-failure-hunter, mach10:feature-completeness-checker, plugin-dev:plugin-validator). Findings are merged across agents with inline source attribution where useful. Critical/Important findings use sequential F<n> identifiers; Suggestions use a separate S<n> counter.

Critical Findings

F1: Destructive-pattern rm regex does not block the most realistic destructive invocations (per code-reviewer and comment-analyzer).

Location: agents/test-plan-executor.md:64
Pattern: \brm\b[^|;&\n]*\s(/|\$HOME|~|\*|\$\{?HOME\}?)(\s|$)
The trailing (\s|$) requires the destructive target to be a standalone token, so common subpath forms slip through unfiltered: rm -rf /etc, rm -rf /var/log, rm -rf $HOME/.ssh, rm -rf ~/.config, rm -rf /*. The accompanying prose ("any rm (with any flag combination) targeting root, home, tilde, or wildcard") overstates what the regex matches. The implementation also diverges from the revised plan, which committed to \brm\s+-rf?\s+(/|$HOME|~|\*|${?HOME}?) (per feature-completeness-checker).
Suggested fix: replace the trailing (\s|$) with (/|\s|$) so subpath targets are also matched, or anchor on a leading \s and accept anything after the destructive root token. Update the prose on the same line to honestly describe the matched set.

F2: Placeholder degenerate case targets an obsolete sentinel string after the pr-create template was rewritten in this same PR (per code-reviewer, comment-analyzer, and silent-failure-hunter).

Locations: agents/test-plan-executor.md:58 and commands/pr-create.md:129-131
The agent looks for the literal <bulleted checklist...> placeholder. That string was the old template. The new template uses different placeholders (<test command>, <expected outcome>, <service>, <observable behavior>, <teardown command>).
Result: a PR author who runs /mach10:pr-create with the new template and submits without replacing the example bullets will not trip the placeholder degenerate case. Instead, the executor will extract the literal backticked <test command> and try to run it -- producing a "command not found" FAIL finding that masks the real problem (unfilled template).
Suggested fix: detect the new placeholder tokens (or any backticked <...> placeholder of the form "angle-bracketed unfilled token"). Add a CLAUDE.md sync entry so the placeholder list stays aligned between pr-create.md and agents/test-plan-executor.md.

Important Findings

F3: Agent is missing a tools: frontmatter field, so the read-only safety boundary is advisory rather than enforced (per feature-completeness-checker).

Location: agents/test-plan-executor.md:1-6
The revised plan explicitly committed to "Tools needed: Bash, Read, Grep, Glob only. No Edit, no Write, no Skill." Without an explicit tools: field, the agent inherits pr-review.md's allowed-tools, which currently includes Skill. The agent's prose ("Read-only", line 12) is purely advisory.
Mitigating context: pr-review.md's allowed-tools does not include Edit or Write, so those mutating tools cannot leak via inheritance. But Skill is inherited, contradicting the plan's commitment.
Suggested fix: add tools: Bash, Read, Grep, Glob to the agent frontmatter (the existing feature-completeness-checker.md follows the same omit-the-field pattern, so consider whether this should be applied to both agents for consistency).

F4: Procedural-check path can absorb failures as PASS without requiring evidence (per silent-failure-hunter).

Location: agents/test-plan-executor.md:29, 38, 41
Procedural items have no objective signal (unlike automatable items' exit code). The classification rules say "procedural check succeeded" is PASS, but nothing forces the agent to (a) record what it did to verify, or (b) define what counts as evidence of success. The principle on line 14 ("Refusal is informative, never silent") is contradicted by the procedural path, which has no analogous "I couldn't verify" exit.
Suggested fix: require procedural items to record both the verification action taken (file read, grep run) and the observable evidence supporting PASS. Add: "If you cannot identify a concrete observable to check for the procedural item, classify it as BLOCKED with reason 'procedural step too ambiguous to verify' rather than guessing PASS."

F5: dd if=... of=/dev/sd*, mkfs, shred, and parted against block devices bypass the destructive filter (per code-reviewer).

Location: agents/test-plan-executor.md:68
The pattern >\s*(/dev/sd[a-z]|/dev/disk) only matches shell-redirect writes. The canonical disk-destroying commands use program flags rather than redirects: dd if=/dev/zero of=/dev/sda, mkfs.ext4 /dev/sda, shred /dev/sda, parted /dev/sda mklabel gpt -- all unfiltered.
Suggested fix: add a pattern like \b(dd|mkfs(\.[a-z0-9]+)?|shred|parted)\b[^|;&\n]*/dev/(sd[a-z]|disk|nvme) to the refused-pattern list (case-insensitive).

F6: pr-pre-merge Step 5d date parse failure can silently mark stale results as fresh (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:180-187
Neither date -u -d "<createdAt>" +%s nor git log -1 --format=%ct failure is handled. If date fails (BSD date without -d, malformed input, surprising offset format), the substitution may yield empty string. The comparison comment_epoch >= branch_epoch then degenerates: an empty value treated as 0 may resolve to either branch depending on shell semantics, and the fail-open case (treated as fresh) silently skips the suite.
Suggested fix: add an explicit validation step. Require the agent to verify both epoch values are non-empty integers (e.g., [[ "$comment_epoch" =~ ^[0-9]+$ ]]) before comparing. On failure, treat the result as stale and surface a note: "Could not parse comment timestamp; treating as stale."

F7: Default 120s timeout misclassification for long suites that do not match the bump regex (per silent-failure-hunter).

Location: agents/test-plan-executor.md:34, 36-39
A test plan item like Run \make check`has nofull|all|integration|suite|e2e|end.to.endtoken, so the timeout is not bumped. On a slow project this can timeout at 120s. The strict reading classifies this asFAIL(non-zero exit), but the agent's "best-judgment" + "prefer over-blocking" guidance creates a real risk it will downgrade toBLOCKED("test took too long") -- in which case noFfinding is created andpr-review-fix` never sees the failure.
Suggested fix: add an explicit rule: "A command that exits due to timeout (e.g., exit 124) is FAIL, not BLOCKED. Note the timeout in the output excerpt: command exceeded <Nms> timeout. Do not silently re-run with a longer timeout."

F8: Malformed ## Test plan results table is accepted as fresh in pr-pre-merge Step 5d (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:180, 200
The conditional is binary on the heading's literal presence. A review comment with the marker and ## Test plan results heading but a malformed/empty/truncated table beneath it (e.g., upstream failure) takes the fresh path. The agent then surfaces "no items" as the prior summary and skips the suite.
Suggested fix: add a parse check on the table content. If the section is present but contains no parseable per-item rows, fall through to running the suite and surface a note: "Prior review's test plan results section was malformed; ran suite directly."

F9: "Multiple sections" degenerate case silently drops content from non-first sections (per silent-failure-hunter and comment-analyzer).

Location: agents/test-plan-executor.md:57
"Use the first match; ignore subsequent sections" violates "Refusal is informative, never silent." If a PR body has two ## Test plan sections (iterative drafting; stale section above an updated one), the executor silently runs only the first. The intended checklist may be the second.
Suggested fix: surface duplicate-sections as an observable note alongside the per-item table: "PR body contains N ## Test plan sections; ran only the first. Other sections were ignored -- consolidate them."

Suggestions

S1: PR test plan does not name actual fixture PRs, despite the revised plan committing to identifying two (per feature-completeness-checker).

The revised plan's "Test approach" said: "Identify two existing PRs whose test plans cover varied cases and document them in the implementation PR description as the validation fixtures." The current PR description's Test plan section reads as "On a fixture PR with..." for nine items but never names a fixture PR. A reviewer cannot reproduce the manual validation as written.

S2: HTML guidance block lifecycle is undefined in pr-create.md (per comment-analyzer).

The HTML comment block (commands/pr-create.md:121-128) is invisible in rendered GitHub view but visible in raw view. Nothing in pr-create.md instructs Claude to keep, remove, or rewrite it during the "Modify" loop. Either outcome (preserved as raw-view noise in merged PRs, or silently stripped) is arguable; neither is documented.
Add a one-line instruction to Step 2 specifying whether the guidance block should persist or be stripped before PR creation.

S3: HTML guidance rules contradict the example bullets they ship alongside (per comment-analyzer).

commands/pr-create.md:122-127 tells the author to "split 'Run X and verify Y' into two checkboxes." But the example bullet "Run <test command> and confirm " is exactly the "Run X and verify Y" shape. An author following the example violates the rule; following the rule diverges from the example.
Pick one and align the other.

S4: New sync entry undercounts the locations it should track (per comment-analyzer).

CLAUDE.md:73 lists three locations for the ## Test plan results heading. But commands/pr-pre-merge.md references the heading at two lines (180 in the marker check, 200 in the fall-through condition). The sync entry should call out both lines explicitly, matching the meticulous counting style of nearby entries (e.g., the No-background-agent sync entry counts nine sites across six files).
Also consider whether the descriptive prose at agents/test-plan-executor.md:8 ("test plan results table") is load-bearing enough to require synchronization.

S5: README "dynamic vs static" disambiguation undersells the more useful difference (per comment-analyzer).

README.md:265 contrasts dynamic execution against static analysis. Both descriptions are accurate, but the more useful disambiguation for a reader is what each agent operates on: the executor consumes the PR description's ## Test plan checklist; pr-test-analyzer consumes the diff's test files and source. Two readers seeing only "dynamic vs static" may not realize the agents read entirely different inputs.
Reword to emphasize input source as well as execution mode.

S6: Add a one-line rationale for the >= equality case in the freshness comparison (per comment-analyzer).

commands/pr-pre-merge.md:187 defines fresh as "comment epoch >= branch epoch." A future maintainer rereading this may flip it to > without realizing the equality case is intentional. Add: "Use >= so a review run immediately after the final commit is treated as fresh."

S7: "P passed, F failed, B blocked" abbreviations are introduced without definition (per comment-analyzer).

commands/pr-pre-merge.md:236 uses P/F/B shorthand with no definition and no precedent elsewhere in the codebase. Use full words ("3 passed, 1 failed, 0 blocked") or define the abbreviations on first use.

S8: BLOCKED reason enum is closed but doesn't cover all paths the rules allow (per silent-failure-hunter).

agents/test-plan-executor.md:83 defines the BLOCKED reason as either human perception required: <category> or filtered for safety: <pattern>. Other legitimate BLOCKED scenarios (timeout, malformed prior result) have no enum slot, so the agent will pick one of the two even when the fit is poor.
Either lock BLOCKED to exactly the two enumerated reasons (and require FAIL for anything else), or expand the enum to include other legitimate paths.

S9: Output truncation rule is asymmetric in a way that can hide context for BLOCKED items (per silent-failure-hunter).

agents/test-plan-executor.md:15, 41, 82 -- output is captured only for FAIL. For BLOCKED items where partial output exists (a destructive command that was almost run, or a procedural check that produced a partial trace), nothing is recorded.
Allow (but do not require) a brief note field for BLOCKED items where partial output materially helps the reviewer. Keep the 30-line cap.

S10: Step 2 "no test plan section" fallback depends on the downstream Skill honoring the instruction (per silent-failure-hunter).

commands/pr-review.md:72 -- the "if no test plan section, surface a single S<n> finding" fallback lives inside a Skill-instruction string. If the downstream Skill paraphrases or drops it, the silent-no-test-plan case becomes silent indeed.
Consider an after-the-fact verification in Step 3: when composing the comment body, if the PR body has no ## Test plan section, ensure the Suggestions list contains the "PR description does not contain a test plan section" item, regardless of what the Skill returned.

S11: pr-pre-merge "Run suite directly" path doesn't handle the no-runner-detected case (per silent-failure-hunter).

commands/pr-pre-merge.md:200-209 -- the auto-detect comment lists Python / JavaScript / etc. test runners but doesn't say what to do if no runner can be detected (docs-only repo, config-only PR, project ships no tests). The agent may improvise (false signal in either direction) or silently skip with no recorded status.
Add: "If no test runner can be auto-detected, report this explicitly and record no test runner detected in the Step 7 report."

Strengths

The epoch-second comparison logic in pr-pre-merge.md Step 5d is well-reasoned: it correctly avoids ISO 8601 string comparison with mixed timezone offsets, uses committer time (which updates on rebase/cherry-pick), and the inline rationale ("string comparison of ISO 8601 timestamps with mixed timezone offsets gives the wrong answer") will help future maintainers understand why.
The new "Test-plan-results section sync" entry in CLAUDE.md is well-formed and matches the tone/format of surrounding sync entries (the location-count nit in S4 notwithstanding).
The README "Note" callout cleanly disambiguates mach10:test-plan-executor (dynamic execution) from pr-review-toolkit:pr-test-analyzer (static analysis).
The new agent file omits allowed-tools and argument-hint, consistent with the project's "Writing Agents" convention.
The agent's "Core Principles" section (agents/test-plan-executor.md:10-15) is exemplary: each principle states the behavior and the rationale, and contrasts with the inverse behavior. This kind of "why" comment ages well.
The ## Test plan heading regex (^#{2,}\s*test\s*plan\s*$) correctly excludes ## Test plan results due to the trailing \s*$ anchor -- no risk of self-matching the review comment's results section.
Convention adherence is excellent: the new agent file mirrors the existing feature-completeness-checker.md template (same frontmatter shape, same single-line escaped description with <example> blocks, similar body section organization).
Plugin manifest validation passes: valid JSON, version 1.30.0 is a correct semver bump (minor for a new feature), all required fields present, no malformed entries.
Several sync entries audited and confirmed not requiring updates from this PR: No-background-agent sync (the new spawn site is delegated through the existing pr-review Skill pass-through), Step 0 bootstrap sync, --comments two-call sync (the new fetch uses --json comments, a different mechanism), and Finding-identifier sync.

Reviewed by Claude Opus 4.7

LeanAndMean · 2026-05-08T20:35:05Z

Independent Assessment of PR Review

For each finding from the prior review, I read the actual code at the cited paths and independently verified whether the issue exists. Findings are classified as Genuine, Nitpick, False positive, or Deferred.

Critical Findings

F1: Destructive rm regex misses common subpath forms — Genuine

Verified at agents/test-plan-executor.md:64. The trailing (\s|$) requires the destructive target to be a complete token, so rm -rf /etc, rm -rf /var/log, rm -rf $HOME/.ssh, and rm -rf ~/.config all fail to match. Only the bare-token forms (rm -rf /, rm -rf ~, rm -rf *) match. This contradicts both the line's prose and the revised plan's commitment.

F2: Placeholder check targets obsolete sentinel string — Genuine

Verified at agents/test-plan-executor.md:58 and commands/pr-create.md:129-131. The agent looks for <bulleted checklist...>; the new template uses <test command>, <expected outcome>, <service>, <observable behavior>, <teardown command>. A user submitting an unfilled template will trip a "command not found" FAIL rather than the intended placeholder notice. Internal mismatch shipped in this PR.

Important Findings

F3: Missing tools: frontmatter on the agent — Nitpick

commands/pr-review.md's allowed-tools excludes Edit/Write, so only Skill could leak via inheritance. The existing agents/feature-completeness-checker.md follows the same omit-the-field convention. Worth adding for defense in depth in a follow-up but not a blocker.

F4: Procedural-check path can absorb failures as PASS without evidence — Nitpick

Tone section (line 99) and Core Principle 2 (line 13) already say "prefer marking BLOCKED over guessing." The classification rules are tight enough; the reviewer's suggested explicit "record evidence" clause is polish, not a fix.

F5: dd/mkfs/shred/parted against block devices bypass filter — Genuine

Verified at agents/test-plan-executor.md:68. The pattern >\s*(/dev/sd[a-z]|/dev/disk) only matches shell-redirect writes; the canonical disk-destroying commands using program flags slip through. Same severity class as F1.

F6: date parse failure can mark stale results as fresh — Genuine

Verified at commands/pr-pre-merge.md:180-187. Neither date -u -d nor git log -1 --format=%ct failure has an explicit guard. If date returns empty, bash arithmetic resolves the comparison to use 0, mis-classifying as fresh. A single [[ "$x" =~ ^[0-9]+$ ]] validation guards both inputs cheaply.

F7: 120s timeout misclassification for long suites — Nitpick

The rules at agents/test-plan-executor.md:36-39 already say FAIL is "non-zero exit code" — exit 124 is non-zero, so the strict reading is correct. The reviewer's concern that the LLM will downgrade to BLOCKED is speculative; an explicit "timeout = FAIL" clause is a useful nudge but not a bug.

F8: Malformed ## Test plan results table accepted as fresh — Genuine

Verified at commands/pr-pre-merge.md:175-180. The freshness gate is a binary contains("## Test plan results") check. A review comment with the heading but a malformed/empty table beneath it takes the fresh path with zero parseable rows. Concrete silent-skip path; a "if no parseable rows, fall through" check fixes it.

F9: "Multiple sections" silently drops content from non-first sections — Nitpick

Defensible deterministic behavior — duplicate ## Test plan sections are rare. The reviewer's suggested observable note is a useful enhancement but the silent-skip risk is low.

Suggestions

S1: PR test plan does not name actual fixture PRs — Nitpick

About the PR description, not the code. Reasonable polish; tracker hygiene rather than a defect.

S2: HTML guidance block lifecycle is undefined — Nitpick

Documentation gap at commands/pr-create.md:121-128. Neither outcome (preserved or stripped) is incorrect. Doc-rot risk only.

S3: HTML guidance rules contradict the example bullets — Genuine

Verified at commands/pr-create.md:122-127 and :129-131. The rule "split 'Run X and verify Y' into two checkboxes" is contradicted by the example "Run <test command> and confirm ." Author-facing contradiction shipped in this PR; small but actively misleading.

S4: New sync entry undercounts locations — Nitpick

The entry at CLAUDE.md:73 is technically accurate but doesn't enumerate both commands/pr-pre-merge.md line references (180 and 200) the way nearby meticulous entries do. Style consistency only.

S5: README disambiguation undersells input-source difference — Nitpick

Easy reword at README.md:265. Not a defect.

S6: Add rationale for >= equality case — Nitpick

One-line maintenance polish at commands/pr-pre-merge.md:187.

S7: P/F/B abbreviations used without definition — Nitpick

Pure clarity polish at commands/pr-pre-merge.md:236.

S8: BLOCKED reason enum is closed but doesn't cover all paths — Nitpick

Forward-looking polish at agents/test-plan-executor.md:83. Pairs well with F4/F7/F8 fixes.

S9: Output truncation rule is asymmetric — Nitpick

BLOCKED items by definition were not run; the asymmetry is intentional and reasonable.

S10: Step 2 fallback depends on downstream Skill honoring instruction — Nitpick

Belt-and-suspenders verification in Step 3 is sensible defense-in-depth; the existing instruction is explicit and the Skill is trusted.

S11: "Run suite directly" path doesn't handle no-runner-detected — Nitpick

One-line instruction at commands/pr-pre-merge.md:200-209 closes the gap. Polish.

Tally

Genuine: 6 (F1, F2, F5, F6, F8, S3)
Nitpick: 14 (F3, F4, F7, F9, S1, S2, S4, S5, S6, S7, S8, S9, S10, S11)
False positive: 0
Deferred: 0

Staged Implementation Plan

Stage 1 (Required): Tighten the destructive-pattern safety filter

Addresses F1 and F5.

Files: agents/test-plan-executor.md
Update the rm regex at line 64 so subpath targets are also matched (e.g., replace trailing (\s|$) with (/|\s|$), or anchor on \brm\s+(-[a-zA-Z]*\s+)*(/|\$\{?HOME\}?|~|\*)). Update prose on the same line to honestly describe the matched set.
Add a new pattern covering block-device destruction by program flag rather than redirect: \b(dd|mkfs(\.[a-z0-9]+)?|shred|parted)\b[^|;&\n]*/dev/(sd[a-z]|disk|nvme).

Stage 2 (Required): Fix placeholder detection alignment with the new template

Addresses F2.

Files: agents/test-plan-executor.md, CLAUDE.md
Change the placeholder check at line 58 to detect the new template tokens (or generically detect any backticked <...> placeholder of the form "angle-bracketed unfilled token").
Add a CLAUDE.md sync entry pairing commands/pr-create.md template placeholders with agents/test-plan-executor.md's placeholder check.

Stage 3 (Required): Harden pr-pre-merge freshness gate

Addresses F6 and F8.

Files: commands/pr-pre-merge.md
Validate that both comment_epoch and branch_epoch are non-empty integers (e.g., [[ "$x" =~ ^[0-9]+$ ]]); on failure, treat as stale and surface a parse-failure note.
Add a parse check on the ## Test plan results table contents; if the section is present but contains no parseable per-item rows, fall through to running the suite and surface a "prior section was malformed" note.

Stage 4 (Required): Resolve template rule/example contradiction

Addresses S3.

Files: commands/pr-create.md
Either rewrite the example bullets at lines 129-131 to be already-split (one concern per bullet) OR soften the rule wording at lines 122-127. Pick a direction and align both.

Stage 5 (Optional): Behavioral defensiveness polish

Addresses F4, F7, F9, S8, S10.

Files: agents/test-plan-executor.md, commands/pr-review.md
Require procedural items to record verification action + observable evidence; if none can be identified, classify BLOCKED.
Add explicit "timeout = FAIL" rule.
Surface duplicate ## Test plan sections as an observable note.
Expand or lock down the BLOCKED reason enum.
Add a Step 3 belt-and-suspenders verification for the "no test plan section" Suggestion.

Stage 6 (Optional): Documentation and clarity polish

Addresses F3, S1, S2, S4, S5, S6, S7, S11.

Files: agents/test-plan-executor.md, agents/feature-completeness-checker.md, commands/pr-create.md, commands/pr-pre-merge.md, CLAUDE.md, README.md, PR description.
Add tools: Bash, Read, Grep, Glob frontmatter to both agents (F3).
Name two concrete fixture PRs in the description's Test plan (S1).
Add Step 2 instruction in pr-create.md for the HTML guidance block lifecycle (S2).
Enumerate both pr-pre-merge.md line references in the sync entry (S4).
Reword README Note callout to emphasize input source (S5).
Add inline rationale comment for >= equality case (S6).
Spell out P/F/B abbreviations or define on first use (S7).
Add explicit "no test runner detected" handling (S11).

Assessed by Claude Opus 4.7

- F1: Tighten rm destructive-pattern regex so subpath targets (/etc, $HOME/.ssh, ~/.config, /*) are matched, and update prose to match. - F2: Replace obsolete <bulleted checklist...> sentinel with a generic angle-bracketed-token detector that covers the new pr-create template placeholders. Add a CLAUDE.md sync entry pairing the convention with both files. - F5: Add a refused-pattern bullet for direct block-device destruction via program flags (dd of=/dev/sd*, mkfs.* /dev/sd*, shred /dev/sd*, parted /dev/sd*) so they no longer bypass the redirect-only filter. - F6: Validate that comment_epoch and branch_epoch are non-empty integers before comparing in pr-pre-merge Step 5d. On failure, treat as stale and surface a parse-failure note. - F8: Require at least one parseable per-item table row beneath the ## Test plan results heading. If the section is malformed, fall through to running the suite directly with an explicit note. - S3: Soften the "one concern per item" rule in the pr-create test plan template guidance so it no longer contradicts the example bullets that pair an action with its expected outcome. Co-Authored-By: Claude <noreply@anthropic.com>

LeanAndMean · 2026-05-08T21:10:39Z

Review fixes applied (commit `146893b`)

Addressed the six findings the assessment classified as Genuine: F1, F2, F5, F6, F8, S3.

Files modified:

agents/test-plan-executor.md — finding 1 (rm regex + prose), finding 2 (placeholder degenerate case rewritten as a generic angle-bracketed-token detector), finding 5 (new dd/mkfs/shred/parted block-device pattern)
commands/pr-pre-merge.md — finding 6 (epoch integer validation; stale fall-through on parse failure), finding 8 (parseable-row gate; malformed-section fall-through)
commands/pr-create.md — suggestion 3 (softened "one concern per item" rule to align with the example bullets)
CLAUDE.md — finding 2 (new "Test-plan-template placeholder sync" entry pairing pr-create.md template tokens with the agent's check)

Notable decisions:

finding 1: dropped the trailing (\s|$) constraint entirely rather than the reviewer's literal first suggestion (/|\s|$). The literal suggestion would have still missed rm -rf /etc because e after / matches none of /, \s, $. Dropping the constraint is the reviewer's second suggestion ("anchor on a leading \s and accept anything after the destructive root token") and catches every subpath form the finding called out.
suggestion 3: softened the rule rather than splitting the example bullets. The action+outcome pairing in each example is a reasonable single test step; a hard "split" rule would have produced more redundant bullets than clarity.
finding 2: chose a generic angle-bracket detector in the agent over enumerating the five placeholder tokens, so future template additions don't drift. The new sync entry calls out that the angle-bracket convention itself (not the specific token list) is the load-bearing piece.
finding 8: defined "parseable table data row" precisely (line beginning with |, three or more pipe-delimited cells, excluding header and separator) so a future implementer can mechanically check the gate.

The fourteen findings the assessment classified as Nitpick (F3, F4, F7, F9, S1, S2, S4, S5, S6, S7, S8, S9, S10, S11) were intentionally not addressed in this batch.

LeanAndMean · 2026-05-11T15:13:06Z

PR Review: Add test-plan-executor subagent to pr-review (re-review)

This is an automated review of PR 138 in its current state after commit 146893b. Findings are aggregated from code-reviewer, comment-analyzer, silent-failure-hunter, mach10:feature-completeness-checker, plugin-dev:plugin-validator, plugin-dev:skill-reviewer, and mach10:test-plan-executor. Critical/Important findings use sequential F<n> identifiers; Suggestions use a separate S<n> counter.

The previously-fixed findings (F1, F2, F5, F6, F8, S3) were independently verified in the current code and are correctly resolved.

Critical Findings

F1: Step 3 merge can poison the Step 5d freshness comparison (per code-reviewer).

Location: commands/pr-pre-merge.md:187
pr-pre-merge Step 3 may merge origin/<default-branch> into the PR branch and push (clean-merge path on line 109, version-file auto-resolve on line 113). Step 5d then computes branch_epoch via git log -1 --format=%ct, which returns the committer time of the newly-created merge commit. The comparison comment_epoch >= branch_epoch will treat a perfectly fresh mach10-review comment as stale simply because the freshness-check itself rebased the branch. The user is then forced through the three-option AskUserQuestion even though no substantive change occurred.
Suggested fix: use the latest non-merge commit timestamp (e.g., git log -1 --format=%ct --first-parent --no-merges, or the max %at of non-merge commits) so a Step 3 merge does not invalidate prior test plan results. Document the rationale inline.

F2: gh pr view failure in Step 5d Step 1 is silently conflated with "no marker" (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:172-178, 206
The instruction says the jq query "returns the comment object, or null if none was found." But gh pr view can also fail entirely (auth expired, rate limit, network error, JSON parse error). In every failure case the variable is empty/error, not null. The downstream branching keys off "Marker found", so any gh failure silently falls through to Step 3 ("No marker..."), running the full auto-detected suite when a fresh review may have already exercised the test plan.
Suggested fix: capture the gh pr view exit code; on non-zero exit, surface a CLI note like "Could not fetch PR comments (gh exit N); running suite as a precaution" and proceed -- making the failure visible rather than silently masquerading as "no prior review."

Important Findings

F3: ## Test plan results section sync entry describes the consumer inaccurately (per code-reviewer and comment-analyzer).

Location: CLAUDE.md:73
The entry says the heading is consumed by commands/pr-pre-merge.md Step 5d "via a contains("## Test plan results") check." The current code does not use a jq contains("## Test plan results") filter -- the jq query on pr-pre-merge.md:175 filters by the  marker only, and the heading is referenced in three distinct ways at lines 180 (Step 5d Step 2 conditional), 182 (section extraction between heading and next ^#), and 206 (fall-through condition). Future maintainers searching for the literal contains("...") to update on rename will not find it, and the entry undersells the extraction logic in line 182.
Suggested fix: broaden the sync entry wording to "consumed by commands/pr-pre-merge.md Step 5d via section-presence detection (line 180), body-extraction between the heading and the next ^# (line 182), and a fall-through branch condition (line 206)."

F4: "Generic angle-bracketed-token detector" sync entry contradicts the agent prose, which enumerates only five tokens (per comment-analyzer and skill-reviewer).

Locations: CLAUDE.md:74 and agents/test-plan-executor.md:58
The sync entry says the agent uses "a generic angle-bracketed-token detector rather than enumerating each token, so adding new placeholders to the template is safe." But the agent prose says: "Detect both backticked tokens (e.g., `<test command>`, `<service>`, `<teardown command>`) and bare angle-bracketed placeholder phrases (e.g., <expected outcome>, <observable behavior>)." There are five enumerated examples and no detection rule/regex. A maintainer implementing the agent would reasonably check only the five examples; adding a new placeholder to pr-create.md may then fail to trip the degenerate case. The same gap is noted by the skill-reviewer.
Suggested fix: add an explicit detection rule to the agent prose, consistent with the parsing-section regex style. For example: "Match the captured item text against <[a-zA-Z][a-zA-Z0-9 _-]*> to flag placeholders, both inside and outside backticks." Either keep the CLAUDE.md sync entry as-is (now backed by the regex) or weaken it to "the agent detects backticked and bare angle-bracketed placeholders by pattern."

F5: Destructive-pattern claim "Refusal is informative, never silent" overstates what the filter enforces (per silent-failure-hunter).

Locations: agents/test-plan-executor.md:14, 62-71
The filter at lines 62-71 catches direct-form destructive commands, but obfuscated equivalents execute silently:
- eval "rm -rf $HOME" -- line 67's \beval\s+<\s*\( only catches eval <(...), not eval "...".
- Heredoc piped to shell: cat <<EOF | sh\nrm -rf /\nEOF -- the extracted command is cat, not rm.
- Interpreter -c arguments: python -c "import os; os.system('rm -rf /')" -- the outer command is python.
The line-14 claim "Refusal is informative, never silent" is therefore an overstatement -- the agent will silently execute these forms.
Suggested fix: weaken the line-14 claim to "Refusal of matched patterns is informative" and add a one-line acknowledgement at line 71 that the filter is a floor (not a ceiling) -- obfuscated destructive commands embedded inside shell strings, heredocs, or interpreter -c arguments will execute without being refused. (Tightening the filter for all these obfuscations is out of scope; making the limit honest is in scope.)

F6: Severity assignment rule for FAIL items has a gap for procedural failures (per skill-reviewer).

Location: agents/test-plan-executor.md:86-89
The Critical/Important severity rules only cover command-extracted failures ("hard build or test-suite failure" vs "single test or validation failed"). Procedural items can also FAIL (e.g., reading a file and not finding the expected string), but the severity rules say nothing about them. The agent has no guidance for whether a procedural FAIL is Critical or Important, so the F-prefixed identifier assignment in the calling review session is ambiguous.
Suggested fix: add a third rule: "Procedural FAIL -- the prose-interpreted check failed (file did not contain expected string, command output did not match expected pattern, etc.); default to Important."

Suggestions

S1: ## Test plan results extraction is not anchored, so a nested heading or fenced block could match (per code-reviewer and skill-reviewer).

Location: commands/pr-pre-merge.md:182
The spec says "extract the body between the ## Test plan results heading and the next ^# heading." A naive substring match for ## Test plan results would also match ### Test plan results subsection (nested heading) or be triggered by the heading appearing inside a code fence. Tighten the extraction anchor to ^##\s+Test plan results\s*$ (and ignore matches inside ``` fences).

S2: Quoted absolute paths bypass the rm filter (per code-reviewer).

Location: agents/test-plan-executor.md:64
rm "/etc/passwd" and rm '/etc/passwd' are not refused because the pattern requires \s(/|...) immediately before the dangerous prefix; the quote intervenes. The executor runs commands via Bash, which will strip the quotes and execute. The same gap exists in the block-device line 69 for quoted /dev/... targets.
Suggested fix: add ["']? (or a character class) before each dangerous prefix in the alternation.

S3: Placeholder detector will false-positive on angle-bracketed autolink URLs (per code-reviewer).

Location: agents/test-plan-executor.md:58
A bare <[^>]+> detector (once F4's detection rule is added) matches legitimate Markdown autolinks like <http://localhost:3000> or <https://example.com/path>. Constrain placeholders to lowercase words/hyphens/spaces only (e.g., <[a-z][a-z0-9 _-]*>) or explicitly exclude <https?://...>.

S4: Test-plan-results section sync entry undercounts pr-pre-merge.md references (per comment-analyzer).

Location: CLAUDE.md:73
The entry cites a single Step 5d check, but the heading appears at three distinct lines in pr-pre-merge.md (180, 182, 206). The meticulous "No-background-agent sync" entry counts nine sites across six files; this entry should follow the same pattern. (Coupled with F3 above -- a single fix addresses both.)

S5: HTML guidance block in pr-create.md Step 2 has no lifecycle instruction (per comment-analyzer).

Location: commands/pr-create.md:121-128, 153
The HTML comment block is invisible in rendered GitHub view but visible in the body source. Nothing in Step 2 instructs Claude to keep, remove, or rewrite it during the "Modify" loop, and Step 3 posts the body verbatim. Every PR created via the command will therefore carry the guidance block in its source unless the user explicitly removes it.
Suggested fix: add a one-line instruction to Step 2 specifying whether the HTML guidance block should be stripped before final approval or kept as-is for downstream PR editors. Pick one and document it.

S6: Capture mechanism for comment_epoch / branch_epoch is not pinned (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:186-187
The spec says "comment epoch: date -u -d "<createdAt>" +%s" and "branch epoch: git log -1 --format=%ct" but never specifies how to capture each into a variable. Bash command substitution $(...) strips trailing newlines (so the =~ ^[0-9]+$ validation passes); other capture mechanisms (heredoc, read) preserve newlines and would falsely fail validation. Stderr handling is also unspecified.
Suggested fix: pin the capture to comment_epoch=$(date -u -d "$createdAt" +%s 2>/dev/null) and similarly for branch_epoch, so both newline-stripping and stderr suppression are explicit.

S7: Stale fall-through asymmetry (no-confirmation vs three-option AskUserQuestion) is undocumented (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:182, 194-204
Two distinct paths converge on Step 3: the malformed-section path (line 182) explicitly says "fall through to step 3 below" with no user interaction; the stale path's "Run suite directly" option (line 204) also says "fall through" but only after the user explicitly chose it. The control flow is correct, but a future maintainer reading both paths may notice the asymmetry and "fix" it by adding/removing a confirmation gate from one side.
Suggested fix: add a one-line rationale at line 182 ("no confirmation needed; the prior result was unusable") and at line 198 ("user explicitly chose this") to lock the asymmetry in place.

S8: "Parseable table data row" exclusion rule is under-specified (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:182
The spec excludes "the header row (| Item | Status | Notes | or similar)" and "the separator row (|---|---|---|)" but defines neither structurally. Edge cases (alignment-marker separators like | :--- | :---: | ---: |; lines with two-cell prose like | Note | this is fine, ignore; a multi-table section with two headers) will give different results across sessions. Tighten to: "a line matching ^\s*\|[^|]*\|[^|]*\|[^|]*\|, excluding rows whose cells are all empty/whitespace or contain only -/:, and excluding the first matching row if its cells contain only header keywords."

S9: Malformed-section / parse-failure CLI notes are not surfaced in Step 7's pre-merge report (per silent-failure-hunter).

Location: commands/pr-pre-merge.md:182, 189, 238
The "section was malformed" and "Could not parse comment timestamp" notes are emitted to the CLI but never recorded in the Step 7 report. A re-run of pr-pre-merge repeats the same failure silently, with no audit trail. Either include the note in Step 7's Tests bullet (e.g., "from pr-review: parse-failed -- ran suite") or post a one-line follow-up comment on the PR when either path fires.

S10: Destructive-pattern filter omits other realistic risky patterns (per skill-reviewer).

Location: agents/test-plan-executor.md:62-71
chmod -R, chown -R, git push --force, git reset --hard, docker system prune, kubectl delete, npm publish are all plausibly-occurring destructive or state-mutating commands that pass the current filter. Even a read-only executor should consider these. At minimum, document the scope decision (filesystem destruction only, not state mutation) in a "Non-goals" note so future readers understand why the filter is intentionally narrow.

S11: pr-test-analyzer disambiguation cue absent from the agent's <example> commentaries (per skill-reviewer).

Location: agents/test-plan-executor.md:3 (the <example> blocks)
The description distinguishes the executor from pr-review-toolkit:pr-test-analyzer by verb ("execute", "runs"), but neither <example> block contains a contrast cue. When two test-plan agents coexist, orchestrator selection depends on the example commentaries. Add one sentence in a commentary like: "Unlike pr-test-analyzer, which evaluates plan adequacy, this agent actually executes the checked items."

S12: PR description Test plan section references unnamed fixture PRs and is not executable (per comment-analyzer and the test-plan-executor's own run).

Location: PR 138 body, Test plan bullets
The new template (introduced in this same PR) says: "Be specific: include the command and the observable pass/fail criterion." But the PR's own Test plan reads "On a fixture PR with..." for six items without naming any fixture PR. The mach10:test-plan-executor agent run on this PR classified all nine items as BLOCKED (no PASS, no FAIL coverage). The PR introducing the executor is therefore self-dogfooding-incompatible.
Suggested fix: either link to specific fixture PR/branch numbers used during manual verification, or note in the body that these are manually-verified one-offs (and consider that the test plan template's own "be specific" guidance does not apply to a meta-PR like this one).

S13: Output Format section omits a concrete example of the aggregate return shape (per skill-reviewer).

Location: agents/test-plan-executor.md:73-91
Per-item fields are specified, but no example block shows the full structure for one PASS, one FAIL, and one BLOCKED, nor names the load-bearing ## Test plan results section heading that the caller uses for the rendered table. A minimal example block would make the contract precise.

S14: Parsed-item list handoff contract is left undefined (per skill-reviewer).

Location: agents/test-plan-executor.md:21
The line "If the parent provides a parsed item list, use it" never names the parameter or shape. The caller cannot discover how to pass it, and the agent cannot discover how to detect it. Either remove the optional path (the otherwise branch already parses the body) or specify the handoff contract explicitly.

Strengths

All six previously-fixed findings (F1 rm regex, F2 generic angle-bracket detection, F5 block-device pattern, F6 epoch validation, F8 parseable-row gate, S3 softened template rule) are correctly implemented in the current code and verified independently.
The new agent file is structurally clean: well-formed frontmatter (name/description/model/color), no spurious allowed-tools/argument-hint fields, and mirrors the existing feature-completeness-checker.md pattern.
Plugin manifest validates cleanly: valid JSON, correct semver bump 1.29.0 -> 1.30.0 (minor bump for a new feature).
The feature-completeness check passed: every deliverable from the revised plan, every key design decision, and every degenerate case enumerated in the agent spec is reflected in the PR diff. The two intentional plan departures (the additional dd/mkfs block-device pattern and the new CLAUDE.md sync entry) are net additions documented in the implementation comment.
The new agent's "Core Principles" and "Tone" sections cleanly separate the executor's responsibilities from pr-review-fix -- the boundary is explicit, which is unusual and welcome.
The Step 5d epoch-second comparison logic is the right primitive for this gate; ISO 8601 string comparison with mixed timezone offsets would have been wrong.
The README "Note" callout at line 265 disambiguating mach10:test-plan-executor (dynamic execution) from pr-review-toolkit:pr-test-analyzer (static analysis) is the kind of forward-looking note that prevents future confusion.

Test plan results

The mach10:test-plan-executor agent was invoked against this PR's own Test plan section. All nine items classified as BLOCKED -- the PR's test plan is composed entirely of interactive-workflow or fixture-PR-dependent items the agent cannot drive non-interactively.

Item	Status	Notes
Run `/mach10:pr-review` against this PR and confirm the executor is invoked, the `## Test plan results` table renders, and any FAIL items receive `F<n>` identifiers in the standard findings list.	BLOCKED	human perception required: interactive slash-command workflow
On a fixture PR with a backticked passing command (e.g., `git --version`), confirm the item is reported `PASS` with the command shown.	BLOCKED	human perception required: fixture PR not named
On a fixture PR with a deliberately failing command (e.g., `false`), confirm the item is reported `FAIL` with exit code and a truncated last-30-lines output excerpt, and that it appears as an `F<n>` finding.	BLOCKED	human perception required: fixture PR not named
On a fixture PR containing a destructive item like `rm -rf /`, confirm it is `BLOCKED` with `filtered for safety: ...` and is NOT executed.	BLOCKED	human perception required: fixture PR not named
On a fixture PR with a prose-only human-perception item (e.g., "Verify the page renders correctly"), confirm it is reported `BLOCKED` with the perception reason.	BLOCKED	human perception required: fixture PR not named
On a PR whose body has no `## Test plan` section, confirm the executor surfaces a single suggestion-level note rather than running.	BLOCKED	human perception required: fixture PR not named
After a fresh `pr-review` run, run `/mach10:pr-pre-merge` and confirm Step 5d surfaces the prior results and skips the auto-detected suite.	BLOCKED	human perception required: interactive workflow sequencing
Add a commit on top of a previously-reviewed PR, run `/mach10:pr-pre-merge`, and confirm it offers the three-option staleness `AskUserQuestion` (re-run pr-review / accept stale / run suite directly).	BLOCKED	human perception required: AskUserQuestion is an interactive UI surface
Sanity-check the strengthened `pr-create.md` template by running `/mach10:pr-create` on a scratch branch and confirming the HTML guidance comment plus example bullets render as expected in the draft body.	BLOCKED	human perception required: interactive workflow, rendering verification

Summary: 0 passed, 0 failed, 9 blocked.

This dogfooding outcome is the trigger for S12 -- the PR's own test plan is not executable, so the executor agent cannot validate the executor agent on its introducing PR.

Reviewed by Claude Opus 4.7

LeanAndMean · 2026-05-11T15:15:47Z

Independent Assessment of PR Re-Review

For each finding from the prior re-review, I read the actual code at the cited paths and independently verified whether the issue exists. Findings are classified as Genuine, Nitpick, False positive, or Deferred.

Critical Findings

F1: Step 3 merge poisons Step 5d freshness comparison — Genuine

Confirmed at commands/pr-pre-merge.md:103-109 (Step 3 may git merge and push a merge commit) and line 187 (git log -1 --format=%ct returns HEAD's committer time). A merge created seconds before the freshness check will out-stamp any prior review comment, forcing the user through the stale path unnecessarily.

F2: gh pr view failure silently conflated with "no marker" — Genuine

Confirmed at commands/pr-pre-merge.md:172-178, 206. The jq pipeline prints null on success-with-no-match and produces empty/error output on gh failure; the downstream branching treats both identically and falls into "run suite directly." Other steps in the same command (Step 2 line 66, Step 3 line 83) explicitly handle gh failures; Step 5d should too.

Important Findings

F3: ## Test plan results sync entry describes consumers inaccurately — Genuine

Verified at CLAUDE.md:73 vs commands/pr-pre-merge.md:175, 180, 182, 206. The sync entry says "via a contains(...) check," but line 175's jq filter is on the  marker (not the section heading); the heading is referenced at three other lines in three different shapes (membership, body-slicing boundary, fall-through trigger). The entry is misleading and undercounts consumers.

F4: Sync entry "generic detector" claim doesn't match agent prose — Nitpick

Verified at CLAUDE.md:74 and agents/test-plan-executor.md:58. The agent prose is example-based ("e.g., ..., e.g., ...") which a reasonable reader can interpret as illustrative rather than exhaustive. Ambiguous wording, not a contradiction. Worth tightening but not blocking.

F5: "Refusal is informative, never silent" overstates the filter — Genuine

Confirmed at agents/test-plan-executor.md:14, 62-71. The patterns require literal prefix tokens — eval "rm -rf $HOME" (no <(...), line 67 requires <\s*\() and python -c "import os; os.system('rm -rf /')" (outer command is python, not rm) both bypass the filter and execute silently. The line-14 claim overstates coverage; rephrasing it (cheap) is sufficient.

F6: Procedural FAIL has no severity rule — Genuine

Verified at agents/test-plan-executor.md:86-89. Both rules describe command-extracted failures only (build/suite vs single-test). A procedural-item FAIL (file read, prose check) leaves the agent without guidance for Critical-vs-Important assignment. Add a third bullet defaulting procedural FAIL to Important.

Suggestions

S1: Heading extraction not anchored — Nitpick

commands/pr-pre-merge.md:182 already says "between the heading and the next ^#," which is a workable anchor. Tighter wording (heading-level anchor, fenced-block awareness) is polish, not correctness.

S2: Quoted absolute paths bypass rm filter — Genuine

Verified at agents/test-plan-executor.md:64. The leading \s requires whitespace immediately before the dangerous prefix; rm "/etc/passwd" has " between space and /. Bash strips quotes before execution, so the rm runs. Allow optional ["'\s] before each prefix.

S3: Placeholder detector false-positive on autolink URLs — Genuine (reclassified from Deferred by user decision; bundled with F4 in Stage 7, which is now Required)

Pre-condition: F4 needs to add a generic detection rule first. Once F4 is addressed, <https://...> autolinks would false-positive without an explicit exclusion. Handled in the same edit as F4 (Stage 7 below).

S4: Sync entry undercounts pr-pre-merge.md references — Genuine (consolidated with F3)

Same root cause as F3 (CLAUDE.md:73). Single edit fixes both.

S5: HTML guidance block lifecycle undefined — Nitpick

commands/pr-create.md:121-128 is user-facing guidance; the author edits the body during the Modify loop. A lifecycle note would be polish, not correctness.

S6: Epoch capture mechanism not pinned — Nitpick

commands/pr-pre-merge.md:186-187 describes the commands but not the bash capture mechanism. Bash conventions ($(...) strips trailing newlines) resolve it in practice.

S7: Stale fall-through asymmetry undocumented — Nitpick

The asymmetry is defensible: malformed = unparseable (no choice to offer); stale = parseable-but-old (user can decide). Worth a one-line rationale but not a bug.

S8: "Parseable table data row" rule under-specified — Nitpick

The "three or more pipe-delimited cells, excluding header/separator" rule is workable. Edge cases (alignment-marker separators, multi-table sections) are vanishingly rare in this context.

S9: Malformed-section / parse-failure notes not in Step 7 report — Nitpick

commands/pr-pre-merge.md:238 template covers fresh / stale / direct-suite but not "malformed prior section." Useful polish for audit trail, but the CLI note still surfaces it in the moment.

S10: Filter omits other risky patterns — Deferred

The filter is intentionally narrow (filesystem-destruction floor). Expanding to chmod -R, git push --force, kubectl delete, etc. is a design choice for a follow-up hardening pass.

S11: No pr-test-analyzer disambiguation cue in <example> — Nitpick

The description already differentiates by verb ("execute", "runs"). The README Note callout at line 265 carries the explicit disambiguation. Adding it to the agent example is redundant defense-in-depth.

S12: PR test plan references unnamed fixture PRs — Deferred

The PR description's Test plan is intentionally manual-verification-only for a meta-PR. The executor correctly classifying all nine items as BLOCKED is the agent demonstrating its placeholder/missing-section handling, not a code defect. Out of scope for this PR.

S13: Output Format lacks aggregate-shape example — Nitpick

Lines 73-91 describe per-item fields adequately. A concrete example would aid clarity but isn't load-bearing.

S14: Parsed-item list handoff contract undefined — Nitpick

Line 21 says "If the parent provides a parsed item list, use it." The caller (pr-review.md) does not currently pass one; the agent parses the body itself per lines 47-49. Dead optional path; either remove it or specify the contract, but neither is urgent.

Tally (updated after deferred-item disposition)

Genuine: 8 (F1, F2, F3, F5, F6, S2, S3, S4)
Nitpick: 10 (F4, S1, S5, S6, S7, S8, S9, S11, S13, S14)
False positive: 0
Deferred: 2 (S10, S12) — both skipped by user decision

Reclassified items: S3 moved from Deferred to Genuine; bundled with F4 in Stage 7 (now Required).

Staged Implementation Plan

Stage 1 (Required): Fix Step 5d freshness comparison

Addresses F1.

Files: commands/pr-pre-merge.md
At line 187, compute branch_epoch from the latest non-merge commit (e.g., git log -1 --format=%ct --no-merges, or filter --first-parent to ignore the merge into the PR branch).
Add an inline comment explaining why merge commits are excluded (Step 3 may produce a fresh merge whose timestamp is younger than any prior review).

Stage 2 (Required): Handle `gh pr view` failure in Step 5d

Addresses F2.

Files: commands/pr-pre-merge.md
Capture gh pr view exit code at lines 172-178. On non-zero exit, surface a CLI note ("Could not fetch PR comments; running suite as a precaution") and proceed deliberately rather than falling through silently. Match the explicit-failure pattern from Step 2 line 66 and Step 3 line 83.

Stage 3 (Required): Correct the CLAUDE.md sync entry

Addresses F3 and S4 (same root cause).

Files: CLAUDE.md
Rewrite the "Test-plan-results section sync" entry at line 73 to describe the real consumers: jq filter on  at line 175, plus heading-anchored body extraction at lines 180, 182, and fall-through trigger at 206. Match the location-count meticulousness of the "No-background-agent sync" entry.

Stage 4 (Required): Narrow the "never silent" claim or extend the filter

Addresses F5.

Files: agents/test-plan-executor.md
Cheap path: rephrase line 14 to "Refusal of matched patterns is informative" and add a one-line caveat at line 71 that the filter is a floor (obfuscated forms execute).
Expensive path: extend lines 62-71 with patterns for eval "...", heredoc-piped-to-shell, and interpreter -c "..." strings. Recommend the cheap path.

Stage 5 (Required): Add procedural-failure severity rule

Addresses F6.

Files: agents/test-plan-executor.md
Add a third bullet to the severity rules at lines 86-89: "Procedural FAIL -- the prose-interpreted check failed; default to Important unless the check explicitly gates the build."

Stage 6 (Required): Allow quoted paths in the destructive-rm pattern

Addresses S2.

Files: agents/test-plan-executor.md
At line 64, change \s(/|\$HOME|~|\*|\$\{?HOME\}?) to ["'\s](/|\$HOME|~|\*|\$\{?HOME\}?) (or equivalent) so quoted paths are also caught. Apply the same fix to line 69 for quoted /dev/... targets.

Stage 7 (Required): Tighten placeholder rule and pre-handle URL autolinks

Addresses F4 (nitpick coupled with S3) and S3 (genuine, reclassified from deferred).

Files: agents/test-plan-executor.md
At line 58, replace the example list with an explicit rule: "Any unfilled <token> within item text triggers this case, except <https?://...> autolinks." Resolves the F4 ambiguity and the S3 autolink regression risk in one edit.
Promoted from Optional to Required because S3 is now genuine; F4 rides along as a free improvement.

Stage 8 (Optional): Polish

Addresses S1, S5, S6, S7, S8, S9, S11, S13, S14.

Files: agents/test-plan-executor.md, commands/pr-pre-merge.md, commands/pr-create.md
Editorial improvements: explicit lifecycle note for the HTML guidance template, capture-mechanism hint for epoch values, rationale lines for the stale-vs-malformed asymmetry, malformed-prior-section bullet in the Step 7 report template, concrete aggregate-output example, removal-or-specification of the optional parsed-item-list handoff path.

Deferred (skipped by user decision)

S10: Broader destructive-pattern catalog (chmod -R, git push --force, kubectl delete, npm publish). Skipped; current narrow scope is sufficient.
S12: PR description's Test plan is an intentional manual-verification fixture for a meta-PR. Skipped; the executor's all-BLOCKED outcome on this PR is the bootstrap case.

Assessed by Claude Opus 4.7

LeanAndMean · 2026-05-11T17:00:44Z

Deferred findings processed after the re-review:

S3 (autolink false-positive) — Reclassified as genuine. Bundled with F4 in Stage 7 (now Required) and will be addressed in the fix handoff.
S10 (broader destructive-pattern catalog) — Skipped. Current narrow scope (filesystem destruction only) is intentional; no follow-up issue created.
S12 (unnamed fixture PRs in PR Test plan) — Skipped. Intentional bootstrap state for the meta-PR introducing the executor; no follow-up issue created.

…#136) Fix freshness comparison and gh failure handling in pr-pre-merge Step 5d (F1, F2), accurately describe Test-plan-results consumers in CLAUDE.md sync entry (F3, S4), tighten the test-plan-executor safety claims and severity rules (F5, F6), and harden the destructive-pattern filter and placeholder detector against quoted paths and autolink URLs (S2, S3). Co-Authored-By: Claude <noreply@anthropic.com>

LeanAndMean · 2026-05-11T17:09:33Z

Review fixes: F1, F2, F3, F5, F6, S2, S3, S4

Commit: 0e675af

Critical

F1 (commands/pr-pre-merge.md): Step 5d branch-epoch now uses git log -1 --format=%ct --no-merges so a Step 3 merge commit no longer makes a fresh review appear stale. Added inline rationale.
F2 (commands/pr-pre-merge.md): Step 5d now checks the gh pr view exit code and surfaces a CLI note ("Could not fetch PR comments (gh exit N); running suite as a precaution") on failure, then falls through to the auto-detected suite instead of silently masquerading as "no prior review."

Important

F3 + S4 (CLAUDE.md): Rewrote the Test-plan-results section sync entry to describe the three real consumer sites in pr-pre-merge.md (line 180 presence detection, line 182 body-extraction, line 206 fall-through trigger) plus the  marker filter at line 175. Corrected the location count to five total (two in pr-review.md, three in pr-pre-merge.md).
F5 (agents/test-plan-executor.md): Weakened the line-14 claim from "Refusal is informative, never silent" to "Refusal of matched patterns is informative, not silent." Added a safety-floor caveat after the filter table acknowledging that obfuscated destructive forms (eval "...", heredoc-piped-to-shell, interpreter -c strings) will execute.
F6 (agents/test-plan-executor.md): Added a third severity rule defaulting procedural FAILs to Important (e.g., "file did not contain expected string", "command output did not match expected pattern").

Suggestions

S2 (agents/test-plan-executor.md): Added ["']? before dangerous prefixes in three filter patterns -- the rm pattern (line 64), the shell-redirect block-device pattern (line 68), and the dd/mkfs/shred/parted pattern (line 69) -- so quoted absolute paths like rm "/etc/passwd" and dd of="/dev/sda" are now caught.
S3 (agents/test-plan-executor.md, CLAUDE.md): Replaced the example-based placeholder description with an explicit <[a-z][a-z0-9 _-]*> regex (case-insensitive) and an explicit exclusion for <https?://...> Markdown autolinks. Updated the Test-plan-template placeholder sync entry in CLAUDE.md to reflect the new detection mechanism.

Notable decisions

F1 used --no-merges rather than --first-parent to directly target the bug (any merge commit, not just first-parent lineage).
F5 took the assessment-recommended cheap path (weaken claim + add caveat) instead of extending the filter to cover obfuscated forms.
S2 was extended to the shell-redirect pattern (line 68) in addition to the two patterns called out in the review, since the same quote-bypass applies there.
Findings F4, S1, S5-S14 were classified as Nitpick or Deferred in the assessment and were not addressed in this session.

Verify with /clear then /mach10:pr-review 138.

LeanAndMean · 2026-05-11T17:19:46Z

PR Review: Add test-plan-executor subagent to pr-review

Critical Issues

F1: Incorrect terminology "allowlist" for what is actually a denylist/blocklist.

File: agents/test-plan-executor.md, line 14
Core Principle 3 reads: "A small allowlist of destructive command patterns is refused." An allowlist is a list of things that are permitted. The patterns listed are things that are refused -- this is a denylist. The inverted terminology could confuse the agent's execution behavior, since the sentence is self-contradictory.
Fix: Change "allowlist" to "denylist" on line 14.
Sources: code-reviewer, comment-analyzer

F2: Destructive-pattern filter does not block rm -rf . or rm -rf ./ (relative paths).

File: agents/test-plan-executor.md, lines 63-64
The first filter pattern requires the rm argument to begin with /, $HOME, ~, *, or ${HOME}. The command rm -rf . or rm -rf ./ uses a relative path that matches none of these prefixes. In the context of a PR review, the working directory is the repository root -- running rm -rf . would delete the entire checkout. This is not an obfuscated command; it is a straightforward, common destructive pattern that falls within the filter's stated scope.
Fix: Add . and ./ to the refused prefix list. Suggested pattern addition: \.\.?/? as an alternative in the prefix group.
Source: silent-failure-hunter

F3: date -u -d is GNU-specific and will always fail on macOS (BSD date), systematically degrading the feature for macOS users.

File: commands/pr-pre-merge.md, line 186
The instruction uses date -u -d "<createdAt>" +%s to convert ISO 8601 timestamps. The -d flag is GNU coreutils-specific and does not exist on BSD/macOS date. The fallback validation guard catches the failure and routes to the stale path, but macOS users will always be routed to the stale path and asked three-way questions even when the review was just posted. They will never get the "fresh" fast path.
Fix: Replace with a portable alternative, e.g., python3 -c "import datetime,sys; print(int(datetime.datetime.fromisoformat(sys.argv[1].replace('Z','+00:00')).timestamp()))" "$createdAt". Or detect the platform and use the appropriate date syntax.
Sources: silent-failure-hunter, comment-analyzer

Important Issues

F4: The null return from the jq query (exit 0, no review comment found) is not explicitly routed to step 3.

File: commands/pr-pre-merge.md, lines 175-178
gh pr view returns exit 0 even when the jq filter produces null (no matching comment). The instruction says to check the exit code and proceed to step 2 on success, but step 2's guard clause ("Marker found and the comment body contains a ## Test plan results section...") implicitly excludes null. Claude must infer from the step 2 heading that null means "marker not found" and jump to step 3, but could instead attempt to parse null's body and createdAt fields.
Fix: Add after the exit code check: "If the command succeeded but returned null (no comment matched the marker), skip directly to step 3."
Sources: code-reviewer, silent-failure-hunter

F5: Epoch validation failure note blames the wrong timestamp source.

File: commands/pr-pre-merge.md, line 189
The validation failure path says "Could not parse comment timestamp; treating as stale" regardless of whether the comment_epoch or branch_epoch validation failed. If git log -1 --format=%ct --no-merges returns empty (e.g., branch with only merge commits), the note would blame the comment timestamp, misleading the user.
Fix: Split into two variants: "Could not parse comment timestamp" (for comment_epoch failure) and "Could not determine branch commit timestamp" (for branch_epoch failure).
Source: silent-failure-hunter

F6: Section extraction terminator ^# truncates at sub-headings inside the section.

File: commands/pr-pre-merge.md, line 182
The instruction says "Extract the body between the ## Test plan results heading and the next ^# heading." The regex ^# matches any heading level, including ### Sub-heading lines that might appear inside the results section. This would cause premature truncation.
Fix: Change from "the next ^# heading" to "the next heading of the same or higher level (^#{1,2}\s)" since ## Test plan results is a level-2 heading.
Source: silent-failure-hunter

F7: Timestamp-only freshness check cannot detect commit rewriting (force-push after review).

File: commands/pr-pre-merge.md, line 187
If a developer rebases after review, the latest non-merge commit might have an older committer timestamp. The comparison would show "fresh" when the code has changed substantively. The epoch comparison checks timestamps, not commit SHAs.
Fix: Consider storing the HEAD SHA in the review comment and verifying it matches current HEAD, or add a note documenting this known limitation.
Source: silent-failure-hunter

F8: No-background-agent sync entry not updated to mention the new Skill pass-through at pr-review.md line 72.

File: CLAUDE.md, line 75
The existing sync note says "Note: commands/pr-review.md Step 2 (line 67) contains a separate Skill pass-through instruction..." but line 72 is also a Skill pass-through that delegates agent invocation. The sync entry should mention both sites.
Fix: Update the parenthetical to reference both lines: "Step 2 (lines 67 and 72) contains separate Skill pass-through instructions..."
Source: code-reviewer

Suggestions

S1: Destructive filter doesn't cover chmod, chown, kill, truncate, and other system-altering commands. The filter's design explicitly acknowledges it is "a safety floor, not a ceiling," so this is a known design boundary rather than an oversight. Consider expanding the floor to cover the most common system-state commands.

Source: silent-failure-hunter

S2: The rm regex example note on line 73 says "filtered for safety: matched destructive rm -rf pattern" but the regex catches any rm invocation targeting dangerous paths, not just rm -rf. A plain rm /etc/passwd would also match. The example could mislead a maintainer into thinking only rm -rf is caught.

Source: comment-analyzer

S3: The CLAUDE.md test-plan-template placeholder sync entry (line 74) says the regex is <[a-z][a-z0-9 _-]*> but omits the "(case-insensitive)" qualifier that appears in the agent file. A reader might assume the regex only matches lowercase.

Source: comment-analyzer

S4: The HTML comment guidance block in pr-create.md (lines 121-128) will be invisible when rendered on GitHub but visible when editing. This is harmless but slightly noisy. Consider adding "Remove this comment block when the test plan is complete" or documenting that it is intended to persist.

Sources: code-reviewer, comment-analyzer

S5: The heading regex ^#{2,}\s*test\s*plan\s*$ uses \s* between "test" and "plan", making the space optional. "Testplan" (no space) would match, which is likely unintentional since all documented variants have a space.

Source: silent-failure-hunter

S6: The item regex ^\s*[-*+]\s+\[[ xX]\]\s+(.+)$ does not match numbered checklist items (1. [ ] item). Users who use numbered items would get "Test plan section is empty" with no explanation.

Source: silent-failure-hunter

S7: The Step 7 report template uses P, F, B as count placeholders ("P passed, F failed, B blocked") which are inconsistent with the N convention used elsewhere ("N failures noted").

Source: silent-failure-hunter

S8: The test-plan-executor agent has no instruction for what to do when gh pr view itself fails (auth expired, PR not found). Consider adding a degenerate case: "Fetch failure -- return a single note."

Source: silent-failure-hunter

S9: The pr-review.md Step 2 instruction tells the Skill to invoke mach10:test-plan-executor, but provides no fallback if the agent cannot be resolved (plugin not installed). Consider adding a fallback clause.

Source: silent-failure-hunter

S10: The heading-level termination rule ("same or higher level") combined with the variable heading match (^#{2,}) creates ambiguity -- an implementer must track the matched heading level to know what "same or higher" means. Consider pinning the heading level to ## since the template always uses ##.

Source: comment-analyzer

Strengths

Thorough edge case handling: The pr-pre-merge.md Step 5d logic has documented fallback paths for every failure mode (CLI failure, missing marker, malformed section, unparseable timestamps). The rationale for epoch-second comparison over ISO string comparison is a valuable "why" comment.
Well-structured agent definition: The test-plan-executor.md agent has clear sections (Core Principles, Inputs, Per-Item Process, Degenerate Cases, Destructive-Pattern Filter, Output Format, Tone) with explicit classification rules and output format specifications.
Accurate sync entries: The CLAUDE.md sync entries (test-plan-results section sync, test-plan-template placeholder sync) correctly identify all cross-file sites with accurate line numbers and counts.
Safety-conscious design: The "safety floor, not a ceiling" disclaimer explicitly documents the limitation boundary and enumerates evasion vectors, preventing overestimation of the filter's coverage.
Full feature completeness: The PR fully implements all requirements from issue 136's revised implementation plan. Both new sync entries are defensible additions beyond the plan's specification.
Clean plugin structure: Plugin validation passes all checks. The new agent has complete and correctly formatted YAML frontmatter with well-structured example blocks.

Test plan results

Item	Status	Notes
Run `/mach10:pr-review` against this PR and confirm executor invocation	BLOCKED	human perception required: multi-session integration test
Backticked passing command (`git --version`) reports PASS	PASS	exit code 0, output verified
Deliberately failing command (`false`) reports FAIL with exit code and excerpt	PASS	exit code 1 confirmed; agent spec covers truncation and F-identifier merge
Destructive `rm -rf /` is BLOCKED with safety note	PASS	regex matched; all destructive patterns verified
Prose-only human-perception item reports BLOCKED	PASS	agent spec defines classification and BLOCKED reason format
Missing `## Test plan` section surfaces suggestion-level note	PASS	degenerate case handling verified in agent and pr-review.md
After fresh pr-review, pr-pre-merge surfaces prior results	BLOCKED	human perception required: multi-session interactive workflow
Staleness AskUserQuestion with three options	BLOCKED	human perception required: multi-step interactive workflow
Strengthened pr-create template rendering	BLOCKED	human perception required: visual rendering judgment

Reviewed by Claude Opus 4.6. This is an automated review -- findings should be verified by a human reviewer.

LeanAndMean · 2026-05-11T17:26:20Z

Independent Assessment of PR Review

For each finding from the review, I read the actual code at the cited paths and independently verified whether the issue exists. I also cross-referenced against prior PR discussion (two prior review cycles with fixes at commits 146893b and 0e675af).

Critical Findings

F1: "allowlist" should be "denylist" -- Genuine issue
Confirmed at agents/test-plan-executor.md:14. Line reads "A small allowlist of destructive command patterns is refused." The term "allowlist" means a list of permitted items; these are refused/blocked, making them a denylist. Simple one-word fix. Not addressed in prior review rounds.

F2: Destructive filter doesn't block rm -rf . or rm -rf ./ -- Genuine issue
Confirmed at agents/test-plan-executor.md:64. The regex \brm\b[^|;&\n]*\s["']?(/|\$HOME|~|\*|\$\{?HOME\}?) requires arguments beginning with /, $HOME, ~, *, or ${HOME}. Relative paths . and ./ don't match. Running rm -rf . in the repo root would delete the entire checkout. While documented as "a safety floor," rm -rf . is a catastrophic common case that should be on the floor.

F3: date -u -d is GNU-only; macOS users always get stale path -- Nitpick (reclassified)
Confirmed at commands/pr-pre-merge.md:186. However, line 189 explicitly handles this: "If either validation fails (BSD date without -d support, malformed createdAt...)." The fallback is documented and intentional. macOS users get a slightly degraded experience (always prompted with three-way question) but the command doesn't break. Previously raised in round 2 as a nitpick and not addressed. Quality-of-life improvement, not a correctness bug.

Important Findings

F4: null jq result has no explicit branch instruction -- Nitpick (reclassified)
Confirmed at commands/pr-pre-merge.md:175-178. When exit 0 with null, step 2's condition ("Marker found AND the comment body contains a ## Test plan results section") fails because null has no section. Step 3 lists "No marker" as the first fall-through case. The null case is implicitly handled by the conjunction failing. Cleanliness improvement, not a correctness fix.

F5: Epoch validation failure note blames wrong timestamp source -- Genuine issue
Confirmed at commands/pr-pre-merge.md:189. The note says "Could not parse comment timestamp" but the validation applies to both comment_epoch and branch_epoch. If branch_epoch fails (e.g., branch with only merge commits), the error message blames the comment timestamp, misleading for debugging.

F6: Section extraction terminator ^# truncates at sub-headings -- Genuine issue
Confirmed at commands/pr-pre-merge.md:182. "Extract the body between the ## Test plan results heading and the next ^# heading" -- ^# matches any heading level including ### inside the section. If the results section contains sub-headings, extraction would truncate prematurely. The correct terminator should match same-or-higher level: ^#{1,2}\s.

F7: Timestamp-only freshness check can't detect commit rewriting -- Nitpick (reclassified)
commands/pr-pre-merge.md:187. Force-pushes are rare in this methodology's workflow. --no-merges already handles the common false-stale case. The three-way question provides a user escape hatch. Theoretical edge case.

F8: No-background-agent sync entry not updated for line 72 -- False positive
CLAUDE.md:75. The sync entry explicitly distinguishes "Skill delegation" from "direct Task call." Line 72 is a Skill pass-through instruction (same as line 67), not a direct agent-spawn site. The sync entry's Note already covers this: "Step 2 (line 67) contains a separate Skill pass-through instruction... that is a different mechanism and is not part of this sync." Line 72 falls under the same Skill delegation umbrella and is not a new spawn site.

Suggestions

S1: Missing chmod/chown/kill/truncate in destructive filter -- Nitpick (by design, filter is explicitly "a safety floor, not a ceiling")

S2: rm regex example note says "rm -rf pattern" but regex catches any rm -- Genuine issue (minor) -- the example on line 73 should say "rm pattern" rather than "rm -rf pattern"

S3: CLAUDE.md sync entry omits "case-insensitive" qualifier -- Genuine issue (minor) -- line 74 should add "(case-insensitive)" after the regex

S4: HTML comment guidance in pr-create.md visible in raw PR body -- Nitpick (standard practice, HTML comments are invisible in rendered view)

S5: Heading regex optionally matches "Testplan" (no space) -- Genuine issue (minor) -- \s* should be \s+ between "test" and "plan"

S6: Item regex doesn't match numbered checklist items -- Nitpick (GitHub checkbox syntax uses - [ ], not 1. [ ]; the pr-create template generates the correct format)

S7: P/F/B count placeholders inconsistent with N convention -- Nitpick (P/F/B are intentional mnemonics for Pass/Fail/Blocked)

S8: No error handling for gh pr view fetch failure in agent -- Nitpick (parent session already fetches PR data; agent's gh call is a fallback path)

S9: No fallback if test-plan-executor agent can't be resolved -- Nitpick (Skill invocation failure would surface naturally as an error)

S10: Heading-level termination ambiguity -- Genuine issue (minor) (prose description relies on the implementer tracking the matched heading level)

Tally

Genuine: 8 (F1, F2, F5, F6, S2, S3, S5, S10)
Nitpick: 9 (F3, F4, F7, S1, S4, S6, S7, S8, S9)
False positive: 1 (F8)
Deferred: 0

Staged Implementation Plan

Stage 1 (Required): Critical and Important genuine fixes

Addresses F1, F2, F5, F6.

agents/test-plan-executor.md: F1 (change "allowlist" to "denylist" on line 14), F2 (add \. to rm regex prefix group on line 64 so rm -rf . and rm -rf ./ are caught)
commands/pr-pre-merge.md: F5 (change "Could not parse comment timestamp" to "Could not parse timestamp" on line 189), F6 (change terminator from ^# to ^#{1,2}\s on line 182)
CLAUDE.md: Update test-plan-results sync entry to reflect the changed terminator from F6

Stage 2 (Optional): Minor genuine fixes

Addresses S2, S3, S5, S10.

agents/test-plan-executor.md: S2 (change "rm -rf pattern" to "rm pattern" on line 73), S5 (change \s* to \s+ between test and plan on line 47), S10 (add regex example to termination rule on line 48)
CLAUDE.md: S3 (add "case-insensitive" to placeholder sync entry on line 74)

Optional: Nitpick improvements (not required for merge)

F3: Portable date command (cross-platform polish)
F4: Explicit null routing after jq query (cleanliness)
Other nitpicks: editorial improvements

Assessed by Claude Opus 4.6

#136) Co-Authored-By: Claude <noreply@anthropic.com>

LeanAndMean · 2026-05-11T18:17:33Z

Addressed review findings F1, F2, F5, F6, S2, S3, S5, S10 in commit 30628cd.

agents/test-plan-executor.md:

finding F1: "allowlist" -> "denylist" (line 14)
finding F2: added \.\.?/? to rm regex prefix group so rm -rf ., rm -rf ./, rm -rf .., rm -rf ../ are caught; updated description
suggestion S2: "rm -rf pattern" -> "rm pattern" in example note (line 73)
suggestion S5: \s* -> \s+ between "test" and "plan" in heading regex (line 47)
suggestion S10: pinned section termination to ^#{1,2}\s instead of ambiguous "same or higher level" (line 48)

commands/pr-pre-merge.md:

finding F5: split validation failure message into separate variants for comment_epoch and branch_epoch failures (line 189)
finding F6: changed section extraction terminator from ^# to ^#{1,2}\s so sub-headings inside the section don't cause premature truncation (line 182)

CLAUDE.md:

Updated test-plan-results section sync entry to reflect F6's new ^#{1,2}\s terminator (line 73)
suggestion S3: added "(case-insensitive)" qualifier to placeholder sync entry regex (line 74)

Findings deferred (per the assessment's classification): F3, F4, F7, F8, S1, S4, S6, S7, S8, S9 were judged nitpicks or false positives and were not addressed.

LeanAndMean · 2026-05-11T19:37:05Z

PR Review: Add test-plan-executor subagent to pr-review

Critical Issues (1)

F1: Section-ending regex inconsistency with heading-acceptance regex in agents/test-plan-executor.md (lines 47-48). The heading-match regex ^#{2,}\s*test\s+plan\s*$ accepts headings at depth 3+ (e.g., ### Test plan), but the section-termination regex ^#{1,2}\s only terminates on # or ## headings. If a PR uses ### Test plan, a subsequent ### Notes sibling heading would NOT terminate the section -- the parser would continue past it and potentially parse and execute unrelated checklist items from adjacent sections. The simplest fix is restricting the heading match to ^#{2}\s*test\s+plan\s*$ (only ## level, matching what pr-create.md actually emits), or making the section-end pattern dynamic based on the matched heading level.

File: agents/test-plan-executor.md:47-48
Source: per comment-analyzer, per silent-failure-hunter

Important Issues (5)

F2: Placeholder detection regex <[a-z][a-z0-9 _-]*> in agents/test-plan-executor.md (line 58) matches common HTML tags (<details>, <summary>, <code>, <pre>) that may appear in test plans with collapsible sections. This would trigger a false "Test plan still uses the template placeholder" early return, preventing execution of a legitimate test plan. The actual template placeholders all contain spaces or underscores (e.g., <test command>, <expected outcome>), so requiring a space or underscore in the match would distinguish them from HTML tags.

File: agents/test-plan-executor.md:58
Source: per silent-failure-hunter

F3: No error handling instruction for gh pr view failure in agents/test-plan-executor.md (line 21). When fetching the PR body, if gh pr view fails (auth expired, network error, rate limit), the agent has no defined behavior. Compare to pr-pre-merge.md line 178 which explicitly handles this case. The agent should return a single BLOCKED finding with the error details so the calling session can surface it.

File: agents/test-plan-executor.md:21
Source: per silent-failure-hunter

F4: Epoch comparison in commands/pr-pre-merge.md Step 5d (lines 184-189) crosses clock domains -- comment_epoch comes from the GitHub server (createdAt) while branch_epoch comes from local git (%ct). With no skew tolerance, if the committer's clock is behind GitHub's, a commit pushed after the review could appear to predate it, making a stale review look fresh. Adding a small tolerance window (e.g., 60 seconds) or surfacing a note when the delta is small would mitigate this.

File: commands/pr-pre-merge.md:184-189
Source: per silent-failure-hunter

F5: The jq filter in commands/pr-pre-merge.md (line 175) returns null in two fundamentally different situations: (1) no review comment exists, and (2) the PR has 100+ comments and the review comment was truncated by GitHub's API pagination. The fallback behavior (run suite) is safe, but the user gets no diagnostic to distinguish "no prior review" from "review lost to pagination." A comment-count check after null would clarify.

File: commands/pr-pre-merge.md:175
Source: per silent-failure-hunter

F6: The rm destructive-pattern description in agents/test-plan-executor.md (line 64) claims to catch relative dot-paths (., ./, .., ../), but the regex [^|;&\n]* greedy quantifier can consume whitespace needed by the subsequent \s anchor, causing false negatives for edge cases like bare rm .. Interior .. path traversal (e.g., rm -rf "some dir/../../") is also not caught since \.\.?/? only matches at the start of the path argument. The documentation slightly overstates the pattern's coverage.

File: agents/test-plan-executor.md:64
Source: per comment-analyzer, per silent-failure-hunter

Suggestions (10)

S1: The eval|source destructive pattern (\b(eval|source)\s+<\s*\() only catches process substitution, not common dangerous forms like eval "$var" or source /path/to/untrusted.sh. The acknowledged limitations paragraph covers eval "..." but not plain source /path. Consider expanding the limitation acknowledgment or the pattern.

File: agents/test-plan-executor.md:67
Source: per comment-analyzer

S2: Timeout escalation regex \b(full|all|integration|suite|e2e|end.to.end)\b over-matches common words. "Verify all fields are populated" would escalate to 600s even for a quick check. Consider requiring keyword co-occurrence (e.g., full.suite) or surfacing a note when timeout is bumped.

File: agents/test-plan-executor.md:34
Source: per silent-failure-hunter

S3: date -u -d in commands/pr-pre-merge.md (line 186) is not portable to macOS (BSD date). While the validation fallback correctly catches this and treats as stale, macOS users will always hit the stale path even with a fresh review. Consider adding a BSD-compatible alternative (e.g., try date -j -f first, fall back to date -u -d).

File: commands/pr-pre-merge.md:186
Source: per comment-analyzer, per silent-failure-hunter

S4: No explicit routing for "gh exit 0 but jq returned null" between sub-steps 1 and 2 in pr-pre-merge.md Step 5d. Claude will infer the correct behavior from the step 3 header, but an explicit sentence ("If the command succeeded but the jq filter returned null, skip step 2 and fall through to step 3") would improve clarity.

File: commands/pr-pre-merge.md:175-178
Source: per silent-failure-hunter

S5: Consider adding a sync entry for the severity-classification coupling between pr-review.md Step 2 invocation instruction (line 72, severity rules for FAIL items) and agents/test-plan-executor.md Output Format section (lines 88-94). If someone changes severity classification in one but not the other, they would diverge.

File: CLAUDE.md
Source: per comment-analyzer

S6: The autolink URL exclusion note in agents/test-plan-executor.md (line 58) implies a separate filtering step is needed, but <https://...> is naturally excluded because : and / are not in the character class [a-z0-9 _-]. Rewording to clarify this is a natural regex property would prevent implementers from adding redundant URL-filtering code.

File: agents/test-plan-executor.md:58
Source: per comment-analyzer

S7: The pr-pre-merge.md Step 7 report template (line 242) has no variant for the "all items BLOCKED" case. When P=0, F=0, B=N, reporting "0 passed, 0 failed, 5 blocked" doesn't communicate that no automated verification occurred. Consider adding: [from pr-review: all N items blocked (no automated verification)].

File: commands/pr-pre-merge.md:242
Source: per silent-failure-hunter

S8: --no-merges on a merge-only branch in pr-pre-merge.md (line 187) produces empty output. The fallback CLI note "Could not determine branch commit timestamp; treating as stale" is correct but doesn't explain why. When branch_epoch is empty but git log -1 --format=%ct (without --no-merges) produces a value, a more specific note would help.

File: commands/pr-pre-merge.md:187
Source: per silent-failure-hunter

S9: The HTML comment guidance block in commands/pr-create.md (lines 121-128) references "the example bullets below" -- if template bullets change without updating the comment, the guidance becomes misleading. Consider making the reference more generic or adding a note to the sync entry.

File: commands/pr-create.md:121-128
Source: per comment-analyzer

S10: No error handling specified in pr-review.md (line 72) for when the test-plan-executor agent crashes or returns malformed output. The review might be posted without a test plan results section and without a note explaining the absence. Consider adding: "If the executor fails, omit the section and add a note."

File: commands/pr-review.md:72
Source: per silent-failure-hunter

Strengths

Thorough cross-file synchronization: Both new CLAUDE.md sync entries are verified accurate -- all referenced line numbers, file locations, and descriptions match actual code.
Defensive documentation: The destructive-pattern filter's "safety floor, not a ceiling" caveat with specific evasion vectors is excellent defensive documentation.
Robust edge-case handling: Step 5d in pr-pre-merge.md handles missing markers, malformed sections, epoch validation failures, and BSD date incompatibility with clear fallback paths.
Consistent vocabulary: The PASS/FAIL/BLOCKED status vocabulary is identical across test-plan-executor.md, pr-review.md, and pr-pre-merge.md.
Full requirements coverage: PR fully implements all requirements from issue 136's revised implementation plan. Version bump (1.29.0 -> 1.30.0) is appropriate for new capability.
Clean plugin structure: All YAML frontmatter valid, all cross-references synchronized, no missing fields in agent definition.
Well-placed rationale comments: The --no-merges parenthetical explains the non-obvious interaction between Step 3 merge behavior and Step 5d freshness comparison.

Test plan results

Item	Status	Notes
Run `/mach10:pr-review` against this PR and confirm the executor is invoked	BLOCKED	Requires full interactive Skill workflow orchestration
Passing command (`git --version`) -> PASS with command shown	PASS	Exit 0, `git version 2.25.1`. Agent lines 28-29, 37 correctly classify.
Failing command (`false`) -> FAIL with exit code and output	PASS	Exit 1. Agent lines 38, 84-85 correctly classify and capture output.
Destructive `rm -rf /` -> BLOCKED, not executed	PASS	Regex at line 64 matches `rm -rf /`. Lines 32-33, 72-73 confirm BLOCKED with safety note.
Prose human-perception item -> BLOCKED with reason	PASS	Agent line 30 classifies subjective items as human-perception. Line 86 specifies reason format.
No `## Test plan` section -> suggestion-level note	PASS	Agent line 55 handles missing-section degenerate case. pr-review.md line 72 confirms S-level finding.
Fresh pr-review then pr-pre-merge surfaces prior results	BLOCKED	Requires full interactive `/mach10:pr-review` + `/mach10:pr-pre-merge` workflow
Stale review -> three-option AskUserQuestion	BLOCKED	Requires prior review state + new commit. Procedurally verified: lines 194-204 implement correctly.
pr-create template HTML guidance renders as expected	BLOCKED	Requires interactive `/mach10:pr-create` on scratch branch. Procedurally verified: lines 120-131 correct.

This is an automated review. Reviewed by Claude Opus 4.6.

LeanAndMean · 2026-05-11T19:42:04Z

Independent Assessment of PR Review

For each finding from the review, I read the actual code at the referenced locations and independently verified whether the issue exists in the current state of the code (after 3 prior fix cycles: commits 146893b, 0e675af, 30628cd).

Per-Finding Classification

Finding	Classification	Reasoning
F1	Genuine issue	Section-end regex `^#{1,2}\s` does not terminate `###`-level siblings when `### Test plan` is parsed. Heading match accepts `^#{2,}` but terminator only covers depth 1-2.
F2	Nitpick	HTML tags in checkbox item text are extremely rare. The `<service>` placeholder (single word, no space) prevents fixing without a design tradeoff -- requiring spaces would break template detection for `<service>`.
F3	Genuine (minor)	Missing error handling for `gh pr view` failure in agent. Inconsistent with `pr-pre-merge.md` line 178 which explicitly handles this. One-sentence addition would fix.
F4	Nitpick	Clock skew is negligible with NTP. Stale path provides safe escape hatch. Theoretical concern only.
F5	False positive	`gh` CLI auto-paginates `--json` output via GraphQL. The 100-comment truncation scenario does not apply.
F6	False positive	Regex backtracking correctly handles `rm .` and `rm -rf .`. Traced the match step by step: `[^
S1	Nitpick	Limitation already acknowledged in adjacent caveat paragraph (line 71).
S2	Nitpick	Timeout ceiling over-matching is harmless -- commands finish when they finish regardless of the timeout max.
S3	Nitpick	Already raised and intentionally handled in cycle 2. BSD date fallback is documented and functional.
S4	Nitpick	Implicit null routing via step 3 header is correct. Already deferred in prior cycle.
S5	Genuine (minor)	Severity classification rules are coupled between `pr-review.md` line 72 and `test-plan-executor.md` lines 88-94 without a sync entry.
S6	Nitpick	Note is technically correct; rewording is cosmetic.
S7	Genuine (minor)	All-blocked case (P=0, F=0, B=N) should be communicated distinctly since no automated verification occurred.
S8	Nitpick	Diagnostic is correct but not maximally specific. Behavior is safe.
S9	Nitpick	Reference is positionally correct; fragility is theoretical.
S10	Genuine (minor)	Silent omission of test-plan-results on executor crash. Explicit failure instruction would make behavior deterministic.

Totals: 5 genuine (1 real bug: F1; 4 minor: F3, S5, S7, S10), 2 false positive (F5, F6), 9 nitpick (F2, F4, S1, S2, S3, S4, S6, S8, S9), 0 deferred

Staged Implementation Plan

Stage 1 (Required): Fix section-end regex mismatch -- F1

File: agents/test-plan-executor.md

Restrict the heading-match regex from ^#{2,}\s*test\s+plan\s*$ to ^#{2}\s*test\s+plan\s*$ (only ## level). This matches exactly what pr-create.md emits and eliminates the mismatch with the ^#{1,2}\s terminator. Update the "Variants like..." list on line 47 to remove ### Test plan.

Stage 2 (Required): Add gh pr view error handling to agent -- F3

File: agents/test-plan-executor.md

Add to the Inputs section (after line 21): "If gh pr view fails (non-zero exit), return a single BLOCKED finding: 'Could not fetch PR body (gh exit N)' and stop."

Stage 3 (Optional): Add sync entry for severity classification -- S5

File: CLAUDE.md

Add a sync entry documenting the coupling between commands/pr-review.md Step 2 Skill instruction (line 72, severity rules for FAIL items) and agents/test-plan-executor.md Output Format section (lines 88-94).

Stage 4 (Optional): Add all-blocked variant to report template -- S7

File: commands/pr-pre-merge.md

Add a variant to the Step 7 Tests template for the all-blocked case: [from pr-review: all N items blocked (no automated verification)].

Stage 5 (Optional): Add executor failure handling to pr-review -- S10

File: commands/pr-review.md

Add to the Skill invocation instruction (line 72): "If the executor agent fails or returns malformed output, omit the ## Test plan results section and surface a single S-level finding ('Test plan executor failed') in Suggestions."

Assessed by Claude Opus 4.6

Co-Authored-By: Claude <noreply@anthropic.com>

LeanAndMean · 2026-05-11T19:53:27Z

Review-fix progress (commit `3234246`)

Addressed five findings from the review:

F1 (agents/test-plan-executor.md): Restricted heading-match regex from ^#{2,} to ^#{2} and removed ### Test plan from the variants list, eliminating the mismatch with the ^#{1,2}\s section terminator.
F3 (agents/test-plan-executor.md): Added explicit error handling for gh pr view failure -- returns a single BLOCKED finding with the exit code and stops.
S5 (CLAUDE.md): Added a Severity-classification sync entry documenting the coupling between pr-review.md line 72 and test-plan-executor.md lines 88-94.
S7 (commands/pr-pre-merge.md): Added an all-blocked variant (from pr-review: all N items blocked (no automated verification)) to the Step 7 Tests report template.
S10 (commands/pr-review.md): Added executor-failure handling to the Skill invocation -- if the executor agent fails or returns malformed output, omit the test plan results section and surface an S-level finding.

Findings F2, F4, F5, F6, and S1-S4, S6, S8, S9 were not addressed -- per the assessment they were classified as nitpicks or false positives.

Address PR 138 review findings F1, F2, F5, F6, S2, S3, S5, S10 (Part of

30628cd

#136) Co-Authored-By: Claude <noreply@anthropic.com>

Address PR 138 review findings F1, F3, S5, S7, S10 (Part of #136)

3234246

Co-Authored-By: Claude <noreply@anthropic.com>

Conversation

LeanAndMean commented May 8, 2026

Summary

Test plan

Uh oh!

LeanAndMean commented May 8, 2026

PR Review: Add test-plan-executor subagent to pr-review

Critical Findings

Important Findings

Suggestions

Strengths

Uh oh!

LeanAndMean commented May 8, 2026

Independent Assessment of PR Review

Critical Findings

Important Findings

Suggestions

Tally

Staged Implementation Plan

Stage 1 (Required): Tighten the destructive-pattern safety filter

Stage 2 (Required): Fix placeholder detection alignment with the new template

Stage 3 (Required): Harden pr-pre-merge freshness gate

Stage 4 (Required): Resolve template rule/example contradiction

Stage 5 (Optional): Behavioral defensiveness polish

Stage 6 (Optional): Documentation and clarity polish

Uh oh!

LeanAndMean commented May 8, 2026

Review fixes applied (commit 146893b)

Uh oh!

LeanAndMean commented May 11, 2026

PR Review: Add test-plan-executor subagent to pr-review (re-review)

Critical Findings

Important Findings

Suggestions

Strengths

Test plan results

Uh oh!

LeanAndMean commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Independent Assessment of PR Re-Review

Critical Findings

Important Findings

Suggestions

Tally (updated after deferred-item disposition)

Staged Implementation Plan

Stage 1 (Required): Fix Step 5d freshness comparison

Stage 2 (Required): Handle gh pr view failure in Step 5d

Stage 3 (Required): Correct the CLAUDE.md sync entry

Stage 4 (Required): Narrow the "never silent" claim or extend the filter

Stage 5 (Required): Add procedural-failure severity rule

Stage 6 (Required): Allow quoted paths in the destructive-rm pattern

Stage 7 (Required): Tighten placeholder rule and pre-handle URL autolinks

Stage 8 (Optional): Polish

Deferred (skipped by user decision)

Uh oh!

LeanAndMean commented May 11, 2026

Uh oh!

LeanAndMean commented May 11, 2026

Review fixes: F1, F2, F3, F5, F6, S2, S3, S4

Critical

Important

Suggestions

Notable decisions

Uh oh!

LeanAndMean commented May 11, 2026

PR Review: Add test-plan-executor subagent to pr-review

Critical Issues

Important Issues

Suggestions

Strengths

Test plan results

Uh oh!

LeanAndMean commented May 11, 2026

Independent Assessment of PR Review

Critical Findings

Important Findings

Suggestions

Tally

Staged Implementation Plan

Stage 1 (Required): Critical and Important genuine fixes

Review fixes applied (commit `146893b`)

LeanAndMean commented May 11, 2026 •

edited

Loading

Stage 2 (Required): Handle `gh pr view` failure in Step 5d

Review-fix progress (commit `3234246`)