fix(commands): stop teaching the discarded-stderr shape, and gate it (v1.40.0)

[lapc506]

The toolkit was blocking what its own commands taught

It ships a discard-stderr rule that refuses a command an agent runs. That rule has no reach at all over the same shape written into a command or a skill — and those files exist to be copied and executed.

Measured across the tree:

total discard sites	164
in a refused form	152
in commands/ skills/ agents/ — the instructing surface	34
already correct (reverse order)	12

A discarded stream makes a failing command indistinguishable from a succeeding one that printed nothing, and the empty result then reads as "none found" rather than "it errored".

The sweep

All 34 now append stderr to ${MNM_LOG:-/tmp/make-no-mistakes.log} — self-contained, so no site depends on a variable some earlier step was supposed to define.

Two files were left alone deliberately, and neither is an oversight:

• commands/parallelize.md states the rule in prose. It already told readers not to do this.
• skills/merge-advisor/SKILL.md uses the legal reverse order.

The mechanism, because a sweep with no gate undoes itself

scripts/check-discard-stderr.mjs, wired into npm run check-discard-stderr and prepublishOnly, and asserted by the suite. Not a gate that nothing runs.

The pattern is NOT defined in the checker

It is read from the shared discard-stderr rule in hooks/rules/rules.yaml, via the rules.json the build already generates and CI already guards against drift.

The first draft carried its own copy of the regex — a second implementation of one measurement. That drifts: the rule tightens in one place and the other keeps passing what it started refusing, silently, which is the exact class of defect this checker exists to catch, committed inside the checker. Adding a form now means editing rules.yaml; both consumers inherit it and neither can disagree with the other.

The only transformation is a POSIX bracket class to its JS equivalent, exported as a named function so it can be asserted rather than hidden in an inline replace. A missing rule throws rather than falling back to a private pattern, and exits 2 (could-not-determine), never 1 (found-violations) — a checker that substituted its own copy would report PASS while checking something nobody approved.

What the checker does own: which lines are commands

The rule genuinely cannot answer that — a hook receives one command string, this receives a document.

In markdown only fenced blocks count. Inline backticks are prose, because a rule that says "never write X" has to be able to write X. Not hypothetical: building this, a sibling guard blocked the command that was counting the violations, and separately blocked a PR body that merely quoted a redirect while explaining why its order is the safe one. Four such blocks across the session, every one reported rather than routed around and never with a marker — a guard that refuses ordinary work gets bypassed, and a bypassed guard carries no information at all.

Outside markdown every line is a command, comments included: a comment demonstrating the bad form is still the line a reader copies.

Order is the whole distinction

The suite's central assertion is that the two spellings sharing a token set get opposite verdicts: duplicating stderr to the original stdout before stdout is retargeted survives and is unmatched by the rule; retargeting stdout first loses both and is matched.

The bare stdout form is not in this toolkit's rule. A consuming repo may hold a stricter local policy, and a test pins that divergence so adopting it stays a deliberate edit to rules.yaml — which changes the hook too — instead of a silent difference between two consumers.

Scope is split on purpose

scope	sites	treatment
commands/ skills/ agents/	0	hard gate
hooks/ scripts/	113	counted, reported, not enforced

A gate that reds the repo it guards is one that gets deleted. Turning the second half into a ratchet on added lines is the next step, named here as intended, not as existing.

An unreadable file is reported as a finding rather than skipped, so a permissions problem cannot read as a passing scan — the same collapse the checker exists to prevent, one level up.

Verification

• npm run check-discard-stderr → gate 0 findings, PASS, pattern sourced from rules.json#discard-stderr
• npx vitest run → 77/77, 13 files (17 new)
• npm run build-rules → regenerates rules.json with no drift
• Mutation-tested, because a de-duplication that is claimed and not measured is just a comment: loosening the SHARED rule breaks 6 tests (the checker really does inherit); restoring a private copy inside the checker breaks 7; unmutated control 17/17

A finding this PR does NOT fix

build-rules.mjs carries an IP-leak guard whose header says client/org names do not belong "anywhere in committed code". It scans only the serialized rules (build-rules.mjs:184-190), so scripts/, src/, commands/, skills/ and CHANGELOG.md are outside its reach entirely.

Found the hard way: three comments added in this branch named a specific consuming repository, and nothing stopped them. They are gone — all three now say "a consuming repo may hold a stricter local policy", which is the portable statement and the more accurate one for a general-purpose plugin. Widening the guard is the rule owner's decision, not a side effect of this PR.

It is the same shape as the defect above: a control whose stated scope is wider than its mechanism.

Ordering note — a live example of the sibling PR

Cut from main and bumps to 1.40.0, assuming #57 (merge-advisor, 1.39.0) lands first. Both PRs touch package.json, plugin.json, marketplace.json and CHANGELOG.md.

Neither conflicts with main today; they conflict with each other. That is exactly the latent-conflict class #57's predicate 5 exists to surface, and it is invisible to gh pr list on both. If this one merges first, #57 needs a rebase and a renumber to 1.40.0, and this becomes 1.39.0.

Created by Claude Code on behalf of @lapc506

[dojo-code-reviewer]

✅ Approved

Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.

Walkthrough

⚠️ Governance Notice: This PR targets the main branch directly, bypassing the standard GitFlow pipeline (feature → develop → main). While the implementation itself is robust and correct, please ensure this direct merge to main is intended or whether it should be retargeted to develop.

Walkthrough

This PR addresses an inconsistency where the toolkit blocked agents from running commands with discarded standard error (2>/dev/null) while its own instructing surface (commands/, skills/, and agents/) taught the bad practice. It performs a comprehensive sweep of 34 instances, replacing them with safe redirection to MNM_LOG (defaulting to /tmp/make-no-mistakes.log), and introduces a strict build-time and pre-publish verification script (scripts/check-discard-stderr.mjs) to permanently prevent future regressions.

Areas Reviewed

• Instructing Surface: Markdown files in commands/, skills/, and agents/ to verify redirection correctness.
• Checker Mechanism: scripts/check-discard-stderr.mjs to evaluate how commands are isolated from prose in markdown, and how rules are loaded dynamically from rules.json.
• Testing: src/audit/discard-stderr.test.ts to ensure coverage of both regex translation and document parsing logic.
• Configuration: Integration of the checker in package.json (prepublishOnly and standard scripts).

Safety Rationale

The PR is safe to merge because it eliminates silent command execution errors without changing functional code, introduces a local test harness that passes perfectly (77/77), and separates gating (strictly enforced for user instructions) from ratcheting (reported but not blocking for internal shell scripts).

Verdict

Approved — 0 blockers, 1 P3.

🔵 P3 — Minor

• src/audit/discard-stderr.test.ts:53 — 🔵 P3 (minor) — Executing execFileSync('cat', ...) makes the test suite dependent on the Unix cat utility. This utility is not natively available on Windows environments (without WSL/Git Bash), which causes test failures for developers working on Windows. Using Node's native readFileSync instead makes the test fully cross-platform.

[pass 1]

---

Total findings: 1 business context (1 total)

[dojo-code-reviewer]

🔵 P3 (minor) — Executing execFileSync('cat', ...) makes the test suite dependent on the Unix cat utility. This utility is not natively available on Windows environments (without WSL/Git Bash), which causes test failures for developers working on Windows. Using Node's native readFileSync instead makes the test fully cross-platform.

[pass 1]