Draft: Resolve #3584407 "Ai eca interceptor"

Closes #3584407

AI-Generated: Yes (Used OpenCode with Claude to adjudicate the review findings against ECA source, implement the fixes, and write the accompanying tests. All conclusions were verified against eca/ai module source and reviewed by a human before pushing.)

Latest round — review feedback from @joshua1234511 (cf8556ab)

All three findings confirmed and addressed. Details are in the four inline discussion threads; summary here.

  1. Tag normalization was incomplete. getTags() becomes a mixed array once SetTagAction runs on a request that already carried tags, and the previous array_is_list() branch filtered the original list tags back out. Extracted a shared TagNormalizer::normalize() that resolves each entry independently, matching the union semantics HasTagCondition already implements on the read side. Also handles two traps the naive loop gets wrong: PHP casting numeric-string tag names to integer array keys, and de-duplication via array keys re-casting them back.

  2. cleanupAfterSuccessors() weakened the guard it reinforced. Removed CleanupInterface from RerouteAction and FailoverToProviderAction. ECA calls it from an unconditional finally in a recursive method (Processor.php:388-394, :381) and creates a fresh plugin instance per successor (Eca.php:542), so a nested run cleared a guard the outer run still depended on. Went further and removed the root cause: both guards moved out of private static state, into a new ai_eca_interceptor.failover_guard service and a subscriber instance property, following eca_user's AccountSwitcher.

  3. SubscriberPriorityTest missed ECA's own dispatch priority. Now declared rather than merely observed — subscriber_priority: 0 on all four event derivatives, per the RenderEvent.php:152 precedent — and asserted with an exact value plus the relative ordering. Additionally closed a second hole: ECA registers at 0, tying with core's GuardrailsEventSubscriber, and Symfony breaks ties by registration order, which assertGreaterThan can never detect. Added a real listener-ordering assertion.

Also in this round

  • access() on both actions now runs token replacement, so a token resolving to an empty value denies access instead of granting it and then silently doing nothing.
  • Every access denial carries an explicit reason, so ECA can log it (EcaAction.php:112-117).
  • RerouteAction's re-entrancy bail moved from execute() into access(), making the guard visible in the ECA process debugger.
  • Failover success and failure are recorded in event metadata, so a downstream ECA condition can branch on them instead of the failure being swallowed.
  • Fixed modules/agents/tests/src/Kernel/AiEcaAgentsKernelTestBase.php, which imported a ContentTypeCreationTrait that does not exist in ECA. Whole-module PHPUnit discovery fatalled on it; discovery now finds 99 tests.

Test coverage

Test count for the interceptor went from 27 to 68 (+41): TagNormalizerTest (13), RerouteFailoverAccessTest (11), RecursionGuardStateTest (7), FailoverGuardTest (5), TagPreservationTest (2), SubscriberPriorityTest (+3). RecursionGuardStateTest asserts the inverse of finding 2 — that neither action implements CleanupInterface — so it cannot silently regress.

Pipeline status

The pipeline on cf8556a surfaced 54 problems. Two were ours; the rest were pre-existing failures unrelated to the interceptor. All are fixed in f029fe1:

  • Ours: access() resolved tokens with ECA's replace(), which leaves a token that has no replacement value as a literal string — so the emptiness check could never fail and access was still granted. Switched to replaceClear() in access() and execute() on both actions, since the two disagreeing is the bug class this was meant to fix. The test's own premise guard caught it and now pins both behaviors.
  • Pre-existing (15 errors): DataProviderInterface had no service alias, so DebugDataProviderCommand's autowired constructor broke container compilation.
  • Pre-existing (8 errors): AiActionTestBase and EcaModelSchemaTest enabled eca but not modeler_api, so eca.processor could not resolve modeler_api.template_token_resolver.
  • Pre-existing (29 phpstan errors): all from ai_agents' deprecated AiAgentBase, ignored in phpstan.neon scoped by path to the two affected files in the agents submodule. The rest of the module stays strict. Migration is a separate issue — the deprecation message is an unedited placeholder, so no replacement is documented (ai_agents work item 3546513).

Fixing the modeler_api omission also unmasked a genuine pre-existing breakage: EcaModelSchemaTest had never reached its assertion, and Schema\Eca::getEntityTypeId() returns eca_model, which Schemata routes as schemata.eca_model. That route only existed because ECA used to ship an eca_model config entity type, removed in eca commit 782086f9. So the generated schema id is a dangling URL in production too, not only in tests. Skipped conditionally, with a todo — it resumes by itself if such an entity type returns, and cannot rot into a permanently dead test. The real fix belongs in its own issue.

Full module suite now green: 100 tests, 963 assertions, 0 errors, 0 failures, 1 documented conditional skip. phpcs, phpstan level 6 and cspell all clean. Verified locally as well as in CI — the AI module checkout was moved to 1.5.x, which unblocked the kernel tests that previously could not bootstrap.

Edited by Jürgen Haas

Merge request reports

Loading