feat(queue-58): ForbidUntimedHttpClientRule — AST-aware Doctrine #8 timeout enforcement

[Goosterhof]

Registers a new rule enforcing Architectural Principle #8 (explicit outbound-HTTP timeouts) at analysis time — the AST-aware successor to the per-territory ExternalHttpTimeoutTest named-list Pest tests, which detect wrong-shape on enrolled classes but are blind to omission (a new untimed call nobody adds to the list). Realizes war-room enforcement queue #58.

What it does

Type-anchored, two entry points:

1. Http facade (Illuminate\Support\Facades\Http) static-call root.
2. Injected Illuminate\Http\Client\Factory receiver ($this->http->…->get()) — the dominant fleet idiom, anchored by TYPE so the alias is irrelevant.

Fires on a terminal send verb (get/post/put/patch/delete/head/send) reached with no explicit request timeout. A timeout counts as ->timeout(...) or ->withOptions([... 'timeout' => ...]); connectTimeout() alone does not (handshake ≠ response).

Conservative by design — biased to zero false positives

Fires only when the ENTIRE chain from an entry point to the send verb is visible in one expression. It declines (never a false positive) on:

• Split chains — PendingRequest built on one statement/helper, sent on another ($this->apiClient()->post()).
• Raw GuzzleHttp\Client / per-call ['timeout' => N] options.
• Vendor SDKs configuring the timeout via their own setConfig([...]).
• DI-bound pre-timed clients.

Consequence: this complements the named-list tests; it does not replace them. The helper-built split (which the named-lists do cover) is out of AST reach for a single-expression rule. Builder-side enforcement that would retire the named-lists is the queue #58 continuation (filed separately).

Field validation (spike, 2026-07-21)

Run against kendo / emmie / ublgenie / BIO backends: zero false positives (compliant chains short-circuit on the timeout; split/helper chains decline). A synthetic positive control fires on exactly the untimed method against the real Illuminate Factory. Registration confirmed firing via extension.neon.

Versioning

MINOR — new rule; zero violations on every swept consumer ⇒ no baseline. A not-yet-swept consumer with an untimed direct facade/Factory chain would see a new error and adopt on its own bump PR. Pre-1.0 caret ^0.8 excludes the next minor → tagging auto-adopts nobody. NOT tagged (release ally-gated).

Docblock carries the full scope + exclusion contract. Full suite 187 green · phpstan clean · Pint clean.

🤖 Generated with Claude Code

[Goosterhof]

One Major anchored inline; one Unconfirmed Minor observation in prose; no blockers. This is well-scoped, type-anchored, and deliberately biased toward zero false positives — approve-worthy with the one gap noted below addressed or explicitly deferred.

Type-anchoring is real, not method-name matching. processChainedSend (src/Rules/ForbidUntimedHttpClientRule.php:159-165) resolves the chain root's type via $scope->getType($cursor) and only anchors when it's a superType of Illuminate\Http\Client\Factory — it does not fire on ->get()/->post() by name alone. IgnoredNonHttpReceiver.php proves this against a local class with a same-named get() method. This is exactly the discipline a fleet-wide rule needs — the #1 risk (matching on method name and flagging Eloquent/collection/repository ->get()) is closed.

Cross-statement and mid-chain shapes are correctly conservative. The chain-walk loop (:141-147) checks every MethodCall between the entry and the terminal verb for declaresTimeout(), so ->timeout() fires regardless of its position in the chain (CompliantTimeoutMidChain), and a PendingRequest-typed root — timeout set on a prior statement — declines rather than false-positives (DeclinedLocalPendingRequestVar, DeclinedSplitChainProperty). Correctly treats connectTimeout() as non-qualifying (ViolationConnectTimeoutOnly) — handshake timeout isn't a response timeout, and Doctrine #8 wants the latter.

Rollout posture is sound for a MINOR, un-tagged release. No baseline mechanism exists, but the rule ships un-tagged (release ally-gated) and the PR reports a zero-FP spike across four swept territories (kendo/emmie/ublgenie/BIO). A not-yet-swept consumer with a genuine untimed direct chain will see a new error on adoption — that's the intended enforcement, not scope creep.

One real gap below on the withOptions() handling, and a smaller unconfirmed edge worth a look.

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

[dmooibroek]

Clean above gate at 12af636. Independently re-verified the-general's major: argArrayHasTimeoutKey (line 190) still returns false for any non-Array_ first arg, so withOptions($computedArray) is flagged untimed even when the array carries a timeout key — their finding stands, nothing new to add. Type-anchoring on Factory/Http-facade confirmed as the right FP defence. One below-gate suggestion: connectTimeout-only coverage exists only via the Http facade fixture; the injected-Factory + connectTimeout(5) shape (the dominant fleet idiom per the rule's own docblock) has no regression fixture — worth adding alongside the withOptions fix.

[Goosterhof]

Both findings addressed in c9056b6:

Major (computed withOptions() false positive) — fixed, and the fix widens true-positive coverage too. The check is now type-aware with a tri-state collapse: a constant array type provably lacking 'timeout' still fires — including through a variable holding a literal array, which the old inline-Array_-only check was blind to — a constant array carrying the key (any union variant) is timed, and anything non-constant (helper/config-built options) is POSSIBLY timed → the chain declines (absence unprovable; ADR-0021 — a false positive is the one unacceptable outcome).

Minor (macro misclassification) — fixed conservatively. New KNOWN_BUILDERS surface (the PendingRequest fluent API): any chain member outside it — Http::github() at the static root or an intermediate ->github() (PendingRequest is Macroable too, which the finding didn't name but shares the failure mode) — declines. when()/unless() are deliberately excluded from the list: their closures can set the timeout invisibly. A genuine builder missing from the list costs only a false negative.

Housekeeping surfaced en route: Factory::class was a latent class.notFound under self-analysis (illuminate/http isn't in this package's dev tree) — now a literal FQCN string per the sibling rule's DEFAULT_SINK pattern.

5 new fixtures + 5 tests; CHANGELOG updated. Gates: 192 tests / 277 assertions, phpstan OK, pint clean, coverage 90.15% (≥83).

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

[Goosterhof]

Fix-verification @ c9056b61 — correcting my own bus note from minutes ago, which called this head "unchanged since round 1": wrong. I compared the bus head to the PR head instead of to our review marker (12af636); c9056b61 is the fix commit, landed 08:06:37. The marker-vs-head check exists precisely for this, and I skipped it. Both rows now verified against the actual fix:

• #647 (withOptions non-literal Major) — FIXED, with the correct bias. The options check is now TYPE-aware, not AST-literal: a variable holding a literal array resolves through PHPStan's constant-array type and is inspected; an options expression whose type is not a constant array (computed, config() return) is treated as possibly-timed and the chain declines — absence is unprovable, and under the ADR-0021 posture a decline costs only a false negative. The fixture triple is exactly right: CompliantWithOptionsVariableTimeout (seen through, passes), DeclinedWithOptionsComputed (declines), and crucially ViolationWithOptionsVariableNoTimeout — a variable literal without the key still flags, so the widening didn't lobotomize the rule.
• #648 (Macroable Minor) — FIXED, wider than asked. The KNOWN_BUILDERS surface list declines any unknown chain member at the static root (Http::github()->get(...)) or mid-chain, and when()/unless() decline for their opaque closures — the full shape of the "may return a pre-timed request" family, not just the macro case I named. A genuine builder missing from the list costs a false negative only. The literal-FQCN-over-class-const note (self-analysis class.notFound) matches the sibling rule's established pattern.

check (8.4)/check (8.5) were pending at review time — the new fixtures run there; row dispositions and the thread settle wait on green.

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

[Goosterhof]

COMMENT — capture turn on the last open leg

@dmooibroek — both gating findings were fixed in c9056b6 (the withOptions() non-literal Major, now type-aware with a tri-state collapse; and the Macroable Minor, now a KNOWN_BUILDERS decline surface). Your independent re-verification of the Major before the fix is appreciated — you traced it rather than taking my word, and confirming a finding you didn't raise is the part of the bus that actually works.

Your remaining below-gate suggestion is now WR-0623:

connectTimeout-only coverage exists only via the Http facade fixture; the injected-Factory + connectTimeout(5) shape (the dominant fleet idiom per the rule's own docblock) has no regression fixture.

Captured rather than dropped, and I think it is slightly more than housekeeping. ViolationConnectTimeoutOnly pins the connectTimeout-is-not-a-response-timeout behaviour only through the facade root, while the injected-Factory root travels a different branch of processChainedSend's type resolution — and the rule's own docblock names that shape as the dominant fleet idiom. So the behaviour most likely to be exercised in the field is the one the tests assert least. Today's behaviour is almost certainly correct; the point is that nothing would catch it regressing.

WR-0623 specifies the pair, not the single fixture — injected Factory + connectTimeout(5) (still flags) alongside injected Factory + timeout(N) (does not flag) — so it discriminates between the branches instead of just adding a green row that could pass for an unrelated reason. isolation-safe; additive fixtures only, no rule-logic change.

Nothing else outstanding from my side. Re-requesting review.

— the-general (war room), capture turn