Give hosts a seam onto the engine's models and controllers - #474
Merged
Merged
Conversation
A host that mounts the engine has concerns its own models and controllers already carry — a connection-switching concern, a session module — and the only way onto the engine's classes was to reopen them from a to_prepare block: undocumented, order-sensitive, and wrong twice over, because TelemetryTrace inherited ActiveRecord::Base rather than ApplicationRecord and had to be patched on its own. TelemetryTrace now inherits ApplicationRecord like every other engine model. Its table name stays fixed rather than derived from the prefix, because the telemetry migration creates the table under its literal name. ActionAgent.model_concerns is included into ApplicationRecord as it loads, after abstract_class is set, so a concern's included block sees no table name and its inherited hook sees every model; ActionAgent.controller_concerns is included into ApplicationController ahead of its own callbacks, so a concern's before_action runs before the dashboard authenticates. Entries are modules or names resolved when the class loads, so a typo fails at boot naming itself. The ingest endpoint inherits ActionController::API and is not touched. Assigning the never-consumed base_controller_class now warns and points at controller_concerns. The new test boots the dummy app in a child process with both lists configured and reads what the classes looked like on their first load; reopening ApplicationController in the test process would reorder every loaded subclass's callback chain. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R333pNcMuXB4CZPfPKao4n
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KfFzc97pARf86bj8LNR59B
The dashboard API now verifies CSRF tokens (#461), so asserting that Api::BaseController skips it would be wrong once that lands; it also failed on Rails main, where a skip leaves differently named callbacks in the chain. ApiForgeryProtectionTest exercises the behaviour instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KfFzc97pARf86bj8LNR59B
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KfFzc97pARf86bj8LNR59B
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #463.
A host that mounts the engine inside an existing Rails app usually has concerns its own models and controllers already carry — a connection-switching concern that pins tables to one database, a session module its admin controllers read through. Until now the only way onto the engine's classes was to reopen them from a
to_prepareblock: undocumented, order-sensitive, and wrong twice over, becauseTelemetryTracedid not inheritApplicationRecordand had to be patched on its own.What changes
ActionAgent::TelemetryTraceinheritsActionAgent::ApplicationRecordlike the other 20 engine models, dropping its duplicateAdapterAwareinclude and gaining the ownership API (owner_association,for_owner) from the same place. Its table name stays fixed atactive_agent_telemetry_tracesrather than derived fromtable_name_prefix: the install generator's telemetry migration creates the table under that literal name whatever prefix the host sets (the dashboard-tables migration is the one that follows the prefix).config.model_concernsandconfig.controller_concerns. Arrays of modules or class-name strings.ApplicationRecordincludes the first as it loads, so every engine model carries them;ApplicationControllerincludes the second as it loads, ahead ofprotect_from_forgeryandbefore_action :authenticate_dashboard!, so a host concern's own callbacks run before the dashboard authenticates. A name isconstantized when the class loads, so an initializer can name a constant the host has not autoloaded yet, and a typo raisesNameErrorat boot naming it rather than being skipped. The ingest endpoint,Api::TracesController, inheritsActionController::APIwith its own bearer-token authentication and is not touched. InApplicationRecordthe include sits afterabstract_class = true, because a concern whoseincludedblock readstable_name(a connection-switching concern does) would otherwise register a table for the abstract base.base_controller_class=warns throughActionAgent.deprecatorand points atcontroller_concerns. The accessor stays until 2.0 so initializers that set it keep booting.Tested against the host that motivated it
actionagent/test/host_integration_test.rbpins the host-facing contract in the shape the motivating install uses. Because the test process has long since loaded the engine's classes, the controller half comes from a child process that boots the dummy app with both lists configured (test/support/first_load_script.rb) and reports what the classes looked like on their first load: a concern'sincludedblock sees the abstract base with no table name and itsinheritedhook sees every model's table, the trace table among them; a concern'sbefore_actionprecedesauthenticate_dashboard!on every dashboard controller whileApi::BaseController's forgery skip andApi::MCPController's authentication skip still hold; the ingest endpoint carries nothing; and acurrent_user_resolverreaches the concern's session reader while the engine's own privatecurrent_userkeeps precedence over the concern's public one. In-process tests cover name resolution, theModuleform, theNameErroron a misnamed concern, the hierarchy (everyActionAgent::Active Record model inheritsApplicationRecord, eager-loaded through Zeitwerk sinceEngine#eager_load!is a no-op),reset!and the deprecation.The companion change in that host replaces its four
to_preparereopenings with the two settings. The one it keeps isApi::BaseController.protect_from_forgery, which is #461.Verification
actionagent/test/**undergemfiles/rails8.gemfile: 453 runs, 0 failures.test/**(framework) runs green apart from the provider tests that need API keys, which fail identically without this change.🤖 Generated with Claude Code
https://claude.ai/code/session_01R333pNcMuXB4CZPfPKao4n
Generated by Claude Code