Issue #3586026: Reject AI Agent IDs that collide with code-defined AiAgent plugins

Ahmad Khalilrequested to merge
issue/ai_agents-3586026:3586026-ai-agent-entity-form into 1.4.x

Closes #3586026 (https://www.drupal.org/i/3586026).

AiAgentManager merges two types of plugin definitions under a single ID namespace: code-defined AiAgent plugins discovered via parent::findDefinitions(), and ai_agent config entities merged in afterwards. When an admin saves a config entity whose machine name matches an existing code-plugin ID, findDefinitions() silently drops the config entity (the if (!isset(definitions[id])) guard at src/PluginManager/AiAgentManager.php:196). The entity becomes invisible to createInstance(), and any downstream consumer expecting ConfigAiAgentInterface (notably ai_assistant_api's AgentRunner — see #3586449 (https://www.drupal.org/i/3586449)) crashes with Call to undefined
method.

This MR closes the gap from both sides:

  • AiAgentForm — the machine_name exists callback now rejects IDs already owned by a non-config code plugin, alongside the existing config-entity check. New collisions can no longer be created through the entity form.
  • AiAgentManager::findDefinitions() — when discovery drops a config entity due to a code-plugin collision, log a warning to the ai_agents channel naming the colliding ID and suggesting a rename (@id_config is the convention this module already uses for shipped defaults like taxonomy_agent_config). Pre-existing collisions on legacy sites are now surfaced in watchdog instead of being silently broken.

The runtime fatal in AgentRunner is handled separately in the companion MR against the ai project (#3586449 (https://www.drupal.org/i/3586449)) so each project's maintainers can review their own surface.

Backwards compatibility

AiAgentManager::__construct() takes one new required argument (LoggerChannelFactoryInterface $loggerFactory). Any project subclassing AiAgentManager and overriding the constructor will need to add the matching parameter and
pass it to parent::__construct(). No public method signatures change.

The form-level validation is strictly new behavior — IDs that were valid before remain valid; only IDs that would have produced a silently-broken collision are now rejected at save time.

Verified locally

  • phpcs --standard=Drupal,DrupalPractice — clean on all changed files.
  • phpstan analyse --configuration=phpstan.neon — clean on all changed files (level 1).
  • Form validator rejects taxonomy_agent, node_content_type_agent, field_type_agent (code-plugin IDs) and existing config-entity IDs; allows new non-colliding IDs.
  • Watchdog warning fires at severity Warning in channel ai_agents when discovery runs against a database with a colliding config entity, with the configured ID substituted into the message.
  • Plugin manager instantiates and discovers cleanly after the DI refactor (verified via \Drupal::service('plugin.manager.ai_agents')->getDefinitions() returning the expected definitions on a test site).
  • No existing kernel/unit tests cover the touched code paths in this module; the form validator and discovery warning are exercised through the runtime smoke tests above. Adding test coverage is a reasonable follow-up if the
    maintainers want it before merge.

Suggested follow-up (out of scope for this MR)

The if (!isset(definitions[id])) semantics in findDefinitions() are preserved here — code plugins continue to win the collision and the config entity remains dropped. That's the right call short-term (won't break sites where a code plugin's tools/behaviors are doing real work under an ID that happens to also exist as a config entity), but the longer-term contract is debatable. Worth a separate issue if the maintainers want to revisit, but not necessary to resolve before this MR lands.

Merge request reports