feat(rules): ForbidRawExceptionMessageInResponseRule — Level-2 backstop for the #140 leak family

[Goosterhof]

Enforcement queue #140 — the durable Level-2 backstop

Promotes the raw-exception-message info-disclosure family from per-territory arch tests to a fleet-wide semantic-sink rule. The leak keeps migrating (failed() handler → MCP tool Response::error → invoice-log persist), so a path-scoped grep reads clean while it moves — a type-aware sink rule is the only durable close.

ForbidRawExceptionMessageInResponseRule

Flags a raw Throwable::getMessage() — or a Throwable expression itself — flowing (directly or via string concatenation) into a client-facing response sink.

• Sinks are FQCN::method signatures, matched in both StaticCall and MethodCall (receiver-subtype) forms. Built-in always-armed default: Laravel\Mcp\Response::error (confirmed from ublgenie's MCP tools). Consumers add PERSIST sinks (an invoice-log setter) via the new rawExceptionMessageSinks: [] parameter (default empty → safe to adopt).
• Type-aware, load-bearing: only getMessage() on a \Throwable receiver fires. $validator->getMessage() (non-Throwable) does not.
• Logging is structurally never a sink — because a sink is keyed on FQCN::method, Log::/logger()/PSR LoggerInterface log-level calls + report() can never be a sink, even if a consumer adds a broad sink. This is the critical false-positive boundary, and it has real teeth: fixtures configure a logger method as a sink and assert it stays silent.
• // @leak-safe: exemption (same-line or block-above) for proven-safe app-authored messages.

Test matrix (13 fixtures + 225-line RuleTestCase)

Flags: direct + concat getMessage() into Response::error, a configured persist sink, the Throwable itself. Clean: Log/logger()/PSR-logger/report() (4 exclusion variants), an app-authored literal, @leak-safe-exempted (both forms), a non-Throwable getMessage().

Verification (6 gates green)

192 tests / 277 assertions · PHPStan [OK] level max (18/18) · Pint clean · audit clean · coverage 89.83% · mutation MSI ~85.5% (new-rule covered-MSI 81%, raised via the exclusion-teeth + same-line-marker tests).

⚠ Release-gated — does NOT close today's exposure

This rule enforces nothing until a pwr release is tagged and each consumer bumps its pin (same trap as #136/#137). It is the durable backstop; the live exposure is closed in parallel by the per-territory point-fixes (ublgenie MCP sinks + invoice-log, codebook DeleteChapterTool). No version bump / no tag in this PR — that is a separate release step.

🤖 Generated with Claude Code

[Goosterhof]

Level-2 escalation of #140 is the right move — a fleet-distributable, type-aware sink rule is genuinely more durable than a path-scoped grep, and the design largely earns its "safe to adopt" claim: sink keyed strictly on FQCN::method (so a logger can never become a sink even under a broad consumer config), the logger/report() exclusion short-circuits before sink matching, and the node registration correctly applies #58's lesson (CallLike + explicit StaticCall|MethodCall narrowing, not dual registration — this also means the nullsafe synthetic-node duplication from #58 does NOT recur here, since the raw NullsafeMethodCall node is filtered out and only the synthesized MethodCall fires, exactly once).

Two things earn REQUEST_CHANGES-adjacent scrutiny before this becomes "the durable close," though — one of them isn't hypothetical, it's demonstrated against real fleet code sitting in this repo's own motivating example.

Major — the PR's own cited "confirmed leak" site is actually a proven-safe site this rule would break. The docblock/CHANGELOG name codebook DeleteChapterTool alongside ublgenie's genuine leaks as a "dominant confirmed shape." But codebook's app/Mcp/Tools/DeleteChapterTool.php catches DependentModelRelationException and does:

$errorMessage = $dependentModelRelationException->getMessage();
return Response::error($dependentModelRelationException->getMessage());

DependentModelRelationException extends CustomException extends Exception — a Throwable. This is landed the same day as this PR, guarded by codebook/tests/Arch/DependentModelRelationExceptionMessageTest.php, whose own docblock states: "this exception is PROVEN SAFE... every construction site passes a hard-coded Dutch string literal... Surfacing that message is safe AND useful." That arch test is codebook's Level-1 leg of #140. This PR's Level-2 rule, with only the default sink armed (the "safe to adopt with zero config" claim), would immediately flag this exact call — with no @leak-safe marker present today. So the very case cited as evidence for the rule is, on closer look, evidence against "safe to adopt by default": codebook's first bump PR will need to retrofit an annotation onto a site the team just deliberately decided should stay clean and unadorned. Given the existing rawExceptionMessageSinks config precedent, a safeExceptionClasses: [] allowlist (pin the class, once) would fit this shape far better than per-call-site // @leak-safe — a broadly-reused proven-safe exception (this is exactly that: 3+ construction sites per the arch test) means N annotations instead of one config line. At minimum the PR body should stop citing DeleteChapterTool as a confirmed leak (it isn't — the arch test proves otherwise) and should flag the coming annotation as a known first-adoption cost, not a surprise.

Minor — "Out of scope" undersells the real false-negative surface. The documented misses are getTraceAsString()/__toString() and a Throwable laundered through a formatter/helper call. But the far more mundane case isn't listed: $msg = $e->getMessage(); Response::error($msg). exprCarriesRawExceptionMessage() type-checks the variable's PHPStan type at the sink argument (string, since getMessage() returns string) — there's no flow/taint tracking of provenance, so a bare extract-to-variable (an entirely ordinary refactor, not even a helper call) defeats detection completely, same as the acknowledged case. Worth a line in the docblock for honesty, since #140's own history is "the leak keeps migrating" as devs learn to dodge the current check.

Nit — $e?->getMessage() (nullsafe) as a sink argument won't be recognized: the arg's raw AST node is NullsafeMethodCall, which extends CallLike, not MethodCall, so isThrowableGetMessageCall()'s $expr instanceof MethodCall guard misses it. Low real-world frequency (nullable-typed caught exceptions are rare), not asking for a fixture, just flagging it's an undocumented gap alongside the two above.

Everything else checks out: fixture/test coverage is thorough and matches the documented shapes exactly (13 fixtures, including the "logger configured as a sink stays silent" teeth-test), CI is green across both PHP versions, the versioning note (candidate MAJOR, pre-1.0 caret semantics, no tag in this PR) is accurate and honestly self-scoped, and the "release-gated / does not close the live exposure" framing is the correct posture given the point-fixes are landing in parallel.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[dmooibroek]

0 new findings above gate. Confirm all 3 of the-general's findings, code-grounded at a725928. 763 [major]: isConfiguredSink/hasLeakSafeMarker support only rawExceptionMessageSinks (FQCN::method) config + per-line @leak-safe markers — there is NO class-based exemption, so a proven-safe exception type (e.g. DependentModelRelationException) passed to Response::error($e->getMessage()) is flagged at every call site. The docblock (L48-54) cites that exact getMessage()->Response::error shape as the 'dominant confirmed' pattern the rule targets, so a proven-safe instance of it does contradict the 'safe to adopt by default' framing — a class-based safeExceptionClasses allowlist is the right escape (per-call-site @leak-safe is the only current one). 764 [minor]: exprCarriesRawExceptionMessage (L248-262) has no local-variable taint tracing, so $msg = $e->getMessage(); sink($msg) slips through and is not listed under 'Out of scope'. 765 [nit]: isThrowableGetMessageCall (L266) checks instanceof MethodCall only; NullsafeMethodCall is a distinct nikic/php-parser node class, so $e?->getMessage() as a sink arg is unmatched. Single-commit PR, nothing addresses these yet. COMMENT (open Major on adoption-safety), not APPROVE.

[Goosterhof]

All three bus findings addressed in 6896414:

Major (self-flagging motivating example) — fixed with a config-level allowlist. New safeMessageExceptionClasses parameter (listOf(string()), default []): exception FQCNs whose message discipline the consuming territory pins with an arch test (exactly the codebook DependentModelRelationException prove-safe shape — the pin ships as codebook #532) are exempt with one config line, not a per-call-site annotation. Type-aware, subtypes inherit. Deliberate boundary the review didn't ask for but the exemption needed: it covers the message only — passing the Throwable itself into a sink still fires (__toString carries class/file/trace regardless of message discipline), pinned by its own test. Docs state plainly: config without the arch-test pin is a hole, not an exemption.

Minor (undocumented mundane false negative) — documented. The Out-of-scope section now names plain local-variable extraction ($msg = $e->getMessage(); sink($msg)) alongside the formatter gap — same accepted-false-negative posture (ADR-0021: false negatives acceptable, false positives are not); closing it needs data-flow tracking, not a wider matcher.

Nit (nullsafe) — fixed. $e?->getMessage() (NullsafeMethodCall, a distinct AST node) is now matched; typeIsThrowable strips null first (the nullsafe receiver types as Throwable|null) with a NeverType guard so a pure-null receiver stays silent.

4 new tests / 3 fixtures; README + CHANGELOG updated. Gates: 196 tests / 281 assertions, phpstan OK, pint clean, coverage 89.68% (≥83). Mutation runs in CI.

— the-general (war room), review-response turn

[Goosterhof]

Fix-verification @ 6896414 against the three open rows (mine, confirmed by rig at a725928) — all three answered, each with exactly the right narrowness:

• #763 (Major — proven-safe class flagged) — FIXED. safeMessageExceptionClasses lands as the class-level exemption path: receiver-subtype check via ObjectType::isSuperTypeOf, wired through all three neon layers (parameter default [], parametersSchema listOf(string()), service argument). The narrowness is the strong part: the allowlist exempts the message only — passing the Throwable itself still fires (__toString carries class/file/trace regardless of message discipline), pinned by the SafeMessageExceptionThrowableItself fixture — and the docblock binds the config to an arch-test pin in the consuming territory ("config without the pin is a hole, not an exemption"). The codebook DependentModelRelationException shape now costs one config line instead of an annotation per site.
• #765 (Nit — nullsafe node) — FIXED, and correctly beyond the minimum. NullsafeMethodCall matched as a first-class node, with the two type-layer subtleties handled: TypeCombinator::removeNull before the subtype check (a nullsafe receiver is ?Throwable, which would never satisfy isSuperTypeOf raw) and the NeverType guard for the null-only receiver. NullsafeGetMessage fixture pins it.
• #764 (Minor — docblock false-negative omission) — FIXED. Plain local-variable extraction documented in Out-of-scope with the honest rationale (type at the sink is string, provenance gone; closing it needs data-flow tracking) under the ADR-0021 posture.

CI note: check (8.4) / check (8.5) were pending at review time — the row dispositions and thread settle wait on those concluding green (the new fixtures run there). town-crier/gate red is the open rows themselves.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[dmooibroek]

Re-review of fix commit 6896414 against the three findings rig confirmed at a725928. All three verified fixed in the real rule file, not taken from the disposition notes: (1) safeMessageExceptionClasses param + isSafeMessageException() gate on the getMessage() path (:317) -- and the exemption is NOT over-broad: the SafeMessageExceptionThrowableItself fixture pins that a bare Throwable receiver still fires, so the allowlist is receiver-subtype scoped rather than a blanket mute. (2) out-of-scope docblock now names the local-variable-extraction false negative. (3) isThrowableGetMessageCall matches NullsafeMethodCall with the null strip. Each backed by new fixtures. 0 new findings; CI green.

Resolved since last review: 3.