>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3593077. --> Reported by: [camoa](https://www.drupal.org/user/2448054) >>> <h3 id="summary-problem-motivation">Problem/Motivation</h3> <p>A full-module code review of <code>ai_metering</code> 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: <code>phpcs</code> (Drupal + DrupalPractice) reports 0 errors/warnings across 52 files, and <code>phpstan</code> at the module's configured level 2 passes. The items below are substantive findings that automated tooling does not catch.</p> <p><strong>Security note:</strong> A small number of security-sensitive findings were also identified during this review. In line with Drupal's security disclosure policy they are intentionally <em>not</em> described in this public issue; they will be communicated to the maintainer directly.</p> <h4 id="summary-steps-reproduce">Steps to reproduce</h4> <p>Per-finding reproduction details are included inline below.</p> <h3>Findings</h3> <p><strong>1. Functional &mdash; fallback operation dispatch is dead for multi-word operations (Major)</strong></p> <p>In <code>AiPreGenerateSubscriber::reroute()</code> the fallback provider is invoked with <code>$provider-&amp;gt;{$operationType}(...)</code>. <code>PreGenerateResponseEvent::getOperationType()</code> returns the snake_case operation name (set via <code>ProviderProxy::camelToSnake()</code>), so the dynamic call resolves to e.g. <code>translate_text</code> rather than <code>translateText</code> and <code>ProviderProxy::__call()</code> throws <code>AiOperationTypeMissingException</code>. Only single-word operations (<code>chat</code>, <code>embeddings</code>) dispatch natively; <code>translate_text</code> 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 &rarr; camelCase before the dynamic call.</p> <p><strong>2. Architecture &mdash; hardcoded role gate (Major)</strong></p> <p><code>AiPreGenerateSubscriber</code> restricts all AI calls to users with the <code>administrator</code> or <code>editor</code> role and throws <code>AiQuotaException</code> otherwise (the code already flags this <code>@todo</code> as temporary). <code>editor</code> 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.</p> <p><strong>3. Config &mdash; missing schema entry and install default for <code>quota.translate_output_ratio</code> (Major)</strong></p> <p><code>quota.translate_output_ratio</code> is read in <code>TokenEstimator</code> and written by <code>MeteringSettingsForm</code>, but it is absent from both <code>config/install/ai_metering.settings.yml</code> and <code>config/schema/ai_metering.schema.yml</code>. After the settings form is first saved this produces a <code>config:export</code> diff, and the undeclared key can fail strict typed-config validation. Fix: add <code>translate_output_ratio: 1.10</code> to the <code>quota</code> section of the install config and a matching <code>type: float</code> entry to the schema.</p> <p><strong>4. Config &mdash; runtime counters written to config on every sync (Normal)</strong></p> <p><code>ModelPricingService::syncFromLitellm()</code> writes <code>pricing_sync.last_synced</code> and <code>pricing_sync.synced_count</code> into <code>ai_metering.settings</code> on every sync. These are runtime counters (already stored in State), and writing them to config dirties every subsequent <code>config:export</code>. Fix: keep them in State only, read them from State in <code>getLastSynced()</code>/<code>getSyncedCount()</code>, and remove the <code>pricing_sync</code> key from install/schema (with an update hook to clean existing sites).</p> <p><strong>5. API correctness &mdash; Anthropic token counter sends an unsupported parameter (Normal)</strong></p> <p><code>AnthropicTokenCountingAdapter</code> includes <code>max_tokens: 1</code> in the request body to <code>/v1/messages/count_tokens</code>. <code>max_tokens</code> is not part of the count-tokens request and should be removed.</p> <p><strong>6. UX &mdash; quota-exceeded notification has no de-duplication (Normal)</strong></p> <p><code>hook_ai_metering_quota_exceeded()</code> 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 <code>Y-m</code>) so the email is sent at most once per user per month.</p> <p><strong>7. Packaging &mdash; external CDN asset loaded outside the library system (Normal)</strong></p> <p>The dashboard template (<code>templates/ai-metering-dashboard.html.twig</code>) loads Chart.js with a raw <code>&amp;lt;script src="https://cdn.jsdelivr.net/..."&amp;gt;</code> tag. For Drupal.org packaging and CSP compatibility it should be declared in <code>ai_metering.libraries.yml</code> and attached as a dependency (ideally bundled locally, or declared as an external library with an SRI hash), consistent with the existing htmx library.</p> <p><strong>8. Code quality &mdash; PHPStan level 6 hygiene (Minor)</strong></p> <p>At level 6 there are 8 <code>missingType.iterableValue</code> docblock gaps (missing array value types on several service/controller/form methods) plus one <code>array_filter(..., 'strlen')</code> callback-type warning in <code>MeteringSettingsForm</code>. The module commits to level 2 today; tightening these keeps higher levels clean.</p> <p><strong>Lower-priority observations</strong></p> <ul> <li><code>QuotaManager::getDashboardData()</code> loads all of a month's usage rows into PHP and aggregates in code; pushing aggregation to SQL (<code>GROUP BY uid</code>) would scale better on busy sites.</li> <li><code>rerouteTranslationViaChat()</code> reads <code>ProviderProxy::$plugin</code> via Reflection, which will break silently if the upstream property is renamed; an upstream accessor would be more robust.</li> </ul> <h3 id="summary-proposed-resolution">Proposed resolution</h3> <p>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.</p> <h3 id="summary-remaining-tasks">Remaining tasks</h3> <ul> <li>Maintainer review/triage of the findings above.</li> <li>Handle the security-sensitive findings separately (to be sent to the maintainer directly).</li> <li>Implement fixes and add/extend tests; add an update hook and change record where config changes apply.</li> </ul> <h3 id="summary-ui-changes">User interface changes</h3> <p>None, other than the dashboard Chart.js asset moving into the library system (no visual change expected).</p> <h3 id="summary-api-changes">API changes</h3> <p>None to public APIs. The <code>pricing_sync</code> config removal and the role-gate change should ship with an update hook and a brief change record.</p> <h3 id="summary-data-model-changes">Data model changes</h3> <p>None. Config schema gains <code>quota.translate_output_ratio</code> and loses <code>pricing_sync</code>; there is no database schema change.</p>