Code Review: PR #338

Commit: f856085b13e0240c136e985c5a881388d0cd2924

Summary

Ships /mutation-test as the framework's behaviour-quality sensor, closing the gap between "did the test execute this line?" (coverage %) and "would the test catch a behaviour change?" (mutation score). Language-dispatched across four runners (Stryker, MutPy, go-mutesting, mutant), graceful-degrades exit-3 + advisory when no runner is installed (sibling to /pdf and /process), wires in as the 10th dimension of /launch-check with WARN-not-FAIL semantics, and documents the five-axis decision space in AgDR-0045. 1,120 additions across 8 files; 34/34 smoke assertions PASS at this SHA; all 4 CI checks green.

Checklist Results

• Architecture & Design: Pass — clean dispatch boundary between detect.sh (mechanical: language + runner check) and SKILL.md (model-driven flow); helpers are sourceable + CLI-dispatched without leakage.
• Code Quality: Pass — set -uo pipefail, scoped local, deliberate shellcheck disable=SC2086 only on the $prune find-args expansion (documented inline as a glob), and the CLI dispatch uses BASH_SOURCE for sourced-mode detection.
• Testing: Pass — 7 test blocks across language detection (5 languages + AC fixture), TS-wins-over-JS, empty/docs-only dirs, node_modules exclusion, runner name validation (4 valid + 2 invalid), graceful-degrade under stripped PATH (asserts exit 3 + 4 per-language install advisories), --check-only, --advisory, and the six-section report shape contract.
• Security: Pass — no secrets, no network at smoke time, no user-supplied data piped to a shell. Stripped-PATH fixture symlinks only POSIX binaries.
• Performance: Pass — detection is bounded find with explicit prune list; no git ls-files (works on un-tracked dirs); 90-min runner timeout configurable.
• PR Description & Glossary: Pass — narrative bullets (each carries what+why per pr-quality.md), 8-term glossary covering mutation score, survived/equivalent mutants, threshold, behaviour harness, and the four runners. Ticket linked (Closes #299).
• Technical Decisions (AgDR):Pass — AgDR-0045 documents five decision axes (runner-dispatch, threshold, cadence, runner-missing behaviour, report location), uses the body-H1-only convention with no YAML frontmatter, and cites sibling AgDRs (0034 for /pdf exit-3 shape, 0025 for /process graceful-degrade, 0043 for /launch-check sibling-skill pattern, 0019 for audit persistence).
• Adopter Handbooks: N/A — diff doesn't touch **/*.{ts,tsx} source, migration paths, or commit-message-author code; only architecture/general handbooks would always-load and none apply to a new advisory skill.

Issues Found

None blocking.

Handbook Findings

No handbook violations. Public handbooks scanned (migration-safety blocking, clean-architecture-layers / commit-message-quality / typescript/strict-mode advisory) — diff doesn't touch migration paths, doesn't introduce architecture layer violations (skill code is in its own bounded helper), and doesn't add TS sources. The shell helpers stay disciplined.

Suggestions

1. detect.sh check_runner doesn't recognise the cosmic-ray override example. SKILL.md § "Config" shows {"mutation": {"runner": {"python": "cosmic-ray"}}} as a per-project override, but the dispatch in detect.sh only knows the four shipped runner names — invoking with cosmic-ray would return exit 2 ("unknown runner"). Two options: (a) treat the SKILL.md example as illustrative-only and add a note clarifying that adding a new runner requires extending detect.sh; or (b) loosen check_runner to fall back to command -v "$runner" for unknown names so the override path actually works without a code change. Either is fine — pick one and tighten the doc.

2. .cjs extension isn't bucketed with JS. The js_count glob covers *.js + *.jsx + *.mjs, but CommonJS module files in .cjs form (used in Node.js dual-package configs) won't count. Probably rare enough to defer; flag here so the gap is visible. Adding -o -name '*.cjs' to the find line covers it.

3. TS-wins-over-JS rule could be over-eager on a single legacy .tsx file. If a JS-dominant project has a single .tsx migration sample sitting in docs/examples/ (because docs/examples/ isn't in the prune list), the project would auto-flip to TS detection and pick Stryker against a JS codebase. Mitigations exist (explicit --language=js override, configurable mutation.runner), but the heuristic comment in detect.sh could acknowledge this trade-off: "TS-presence wins" is the deliberate call, with the docs-sample caveat being why --language=... exists.

4. The smoke test never exercises .tsx / .jsx detection. The TS fixture uses .ts only; the JS fixture uses .js only; the mixed fixture mixes .ts and .js. Adding one assertion that a fixture of .tsx-only files reports ts would lock the JSX-handling contract.

5. AgDR-0045 § "Artifacts" line "PR: " — common pattern in the framework, but worth back-filling to PR #338 after merge for trend-mining ergonomics (operator follow-up; not a Rex-blocker).

6. Optional: surface the AgDR-0044 token-efficiency invariant change in the PR body. The dynamic skill-count update in test_token_efficiency_wave1.sh is a quietly nice improvement (no more hardcoded 52 to drift); a future reader scanning the test would benefit from the rationale. Could be done by adding a one-line bullet to the Summary or leaving the diff's inline comment to do the work.

Verdict

APPROVED

This is a clean polyglot-skill addition with disciplined boundaries:

• The graceful-degrade shape is consistent with /pdf and /process (verified — /pdf SKILL.md uses exit-3, /process SKILL.md uses the same install-advisory pattern).
• WARN-not-FAIL for launch-check integration is defended explicitly in AgDR-0045 axis 3 (mutation testing is leading indicator, not blocker) and aligns with the framework's milestone-boundary cadence rather than per-PR.
• The smoke test exercises every claimed contract — including the stripped-PATH graceful-degrade path the PR explicitly calls out (covered in test block 5).
• File-count detection with TS-presence override is a sensible heuristic; configured mutation.runner correctly wins per AgDR-0045 axis 1.
• AgDR-0045 follows the body-H1 / no-frontmatter convention (consistent with the framework drift from templates/agdr.md per feedback memory).
• 90-min timeout cap, dated reports with -NN collision suffix, per-language shallow-merge override semantics all match the framework's broader config conventions.

The six suggestions above are all polish (.cjs glob, .tsx smoke coverage, check_runner extensibility narrative, the SKILL.md cosmic-ray example consistency, AgDR PR back-fill, body-bullet for the dynamic skill-count fix). None block merge; each is a one-line-of-code or one-line-of-prose fix.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: f856085b13e0240c136e985c5a881388d0cd2924