Code review hardening: fallback operation dispatch, config schema gap, pricing-sync config writes, and API correctness (1.0.x)

Problem/Motivation

A full-module code review of ai_metering 1.0.x (a maintainer-assist pass) was carried out, covering Drupal coding standards, configuration handling, and Drupal AI integration correctness. Baseline tooling is clean: phpcs (Drupal + DrupalPractice) reports 0 errors/warnings across 52 files, and phpstan at the module's configured level 2 passes. The items below are substantive findings that automated tooling does not catch.

Security note: A small number of security-sensitive findings were also identified during this review. In line with Drupal's security disclosure policy they are intentionally not described in this public issue; they will be communicated to the maintainer directly.

Steps to reproduce

Per-finding reproduction details are included inline below.

Findings

1. Functional — fallback operation dispatch is dead for multi-word operations (Major)

In AiPreGenerateSubscriber::reroute() the fallback provider is invoked with $provider->{$operationType}(...). PreGenerateResponseEvent::getOperationType() returns the snake_case operation name (set via ProviderProxy::camelToSnake()), so the dynamic call resolves to e.g. translate_text rather than translateText and ProviderProxy::__call() throws AiOperationTypeMissingException. Only single-word operations (chat, embeddings) dispatch natively; translate_text works only because it is caught and rebuilt via the chat path, and other multi-word operations fall through to a hard quota exception. Fix: convert the operation type snake_case → camelCase before the dynamic call.

2. Architecture — hardcoded role gate (Major)

AiPreGenerateSubscriber restricts all AI calls to users with the administrator or editor role and throws AiQuotaException otherwise (the code already flags this @todo as temporary). editor is not a core role, so on most sites this blocks AI for legitimate users, and an access decision is surfaced as a quota error. Recommend replacing the hardcoded list with a configurable permission, or removing the gate and relying on the AI module's own access layer plus the existing quota check.

3. Config — missing schema entry and install default for quota.translate_output_ratio (Major)

quota.translate_output_ratio is read in TokenEstimator and written by MeteringSettingsForm, but it is absent from both config/install/ai_metering.settings.yml and config/schema/ai_metering.schema.yml. After the settings form is first saved this produces a config:export diff, and the undeclared key can fail strict typed-config validation. Fix: add translate_output_ratio: 1.10 to the quota section of the install config and a matching type: float entry to the schema.

4. Config — runtime counters written to config on every sync (Normal)

ModelPricingService::syncFromLitellm() writes pricing_sync.last_synced and pricing_sync.synced_count into ai_metering.settings on every sync. These are runtime counters (already stored in State), and writing them to config dirties every subsequent config:export. Fix: keep them in State only, read them from State in getLastSynced()/getSyncedCount(), and remove the pricing_sync key from install/schema (with an update hook to clean existing sites).

5. API correctness — Anthropic token counter sends an unsupported parameter (Normal)

AnthropicTokenCountingAdapter includes max_tokens: 1 in the request body to /v1/messages/count_tokens. max_tokens is not part of the count-tokens request and should be removed.

6. UX — quota-exceeded notification has no de-duplication (Normal)

hook_ai_metering_quota_exceeded() sends the fallback notification email on every over-quota call, even though the docblock states it should notify the user "the first time" their quota is exceeded. Recommend a State-keyed guard (per uid, per Y-m) so the email is sent at most once per user per month.

7. Packaging — external CDN asset loaded outside the library system (Normal)

The dashboard template (templates/ai-metering-dashboard.html.twig) loads Chart.js with a raw <script src="https://cdn.jsdelivr.net/..."> tag. For Drupal.org packaging and CSP compatibility it should be declared in ai_metering.libraries.yml and attached as a dependency (ideally bundled locally, or declared as an external library with an SRI hash), consistent with the existing htmx library.

8. Code quality — PHPStan level 6 hygiene (Minor)

At level 6 there are 8 missingType.iterableValue docblock gaps (missing array value types on several service/controller/form methods) plus one array_filter(..., 'strlen') callback-type warning in MeteringSettingsForm. The module commits to level 2 today; tightening these keeps higher levels clean.

Lower-priority observations

• QuotaManager::getDashboardData() loads all of a month's usage rows into PHP and aggregates in code; pushing aggregation to SQL (GROUP BY uid) would scale better on busy sites.
• rerouteTranslationViaChat() reads ProviderProxy::$plugin via Reflection, which will break silently if the upstream property is renamed; an upstream accessor would be more robust.

Proposed resolution

Address the functional and config-handling items first (they affect fallback routing and config-export cleanliness), then API correctness, UX, packaging, and code-quality items. Any of these can be split into a dedicated child issue if the maintainer prefers per-bug tracking.

Remaining tasks

• Maintainer review/triage of the findings above.
• Handle the security-sensitive findings separately (to be sent to the maintainer directly).
• Implement fixes and add/extend tests; add an update hook and change record where config changes apply.

User interface changes

None, other than the dashboard Chart.js asset moving into the library system (no visual change expected).

API changes

None to public APIs. The pricing_sync config removal and the role-gate change should ship with an update hook and a brief change record.

Data model changes

None. Config schema gains quota.translate_output_ratio and loses pricing_sync; there is no database schema change.