You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Design feedback on #435, from the repo owner while reviewing it for release. Filing so it is not lost in a PR thread — the current shape ships for testing, this is about where it should land.
The concern
Declaring tools as a config array is the weakest part of the current design:
Three things are already known to the framework, and the config restates them:
The model — TicketTools already says model Ticket.
The agent — the naming convention already maps Ticket → TicketAgent.
The schema — the columns are in the database.
So a host declares a class whose entire content is derivable, then declares it again in an initializer. Something like Reservation < ApplicationRecord being reachable by ReservationAgent with no config at all is the shape worth aiming for.
Not because it is hard. Enumerating models after eager_load! is a few lines, and the convention already works. The problem is what auto-registration would expose.
Auto-registering all of them puts User, ApiToken and CliDeviceGrant in front of a model by default. PROPOSAL_schema_tools.md called this out directly: "Nothing should appear in an agent's toolset because a table exists."
The filterable / returns allowlists are the other half of the same argument. They cannot be derived from the schema, because "which columns may a model filter on" is a judgement about exposure, not a fact about the table. Deriving them would mean either exposing every column or inventing a heuristic that quietly decides password_digest is uninteresting.
Middle grounds worth considering
Ordered by how much they keep the boundary explicit:
Drop the config array; discover SchemaTools subclasses. The declaration stays the boundary, but it is declared once instead of twice — SchemaTools.descendants after eager-loading app/agent_tools. This is what the host shim did before Schema-derived agent tools: generate a bounded tool roster from an ActiveRecord model #435's seam existed, and it worked. Smallest change, removes the duplication actually complained about.
A generator.rails g active_agent:schema_tools Reservation writes a starter class with every column commented out, so the allowlist is a review step rather than a blank page.
Opt-in auto-derivation with an explicit include list.config.schema_tools_from_models = [Reservation, Ticket] — derives the roster but still names the models. Removes the tool class for simple cases.
Full auto-registration with a denylist. Most convenient, and the one that eventually exposes a table someone forgot to deny. Would need a hard-coded refusal for anything matching the Devise/token/session shape, which is a heuristic guarding a security boundary.
(1) looks like the clear near-term win; it is strictly less config with no change to what is exposed. (3) is the interesting one if the goal is to delete tool classes entirely for CRUD-shaped models.
Not blocking the current release
#435 ships as-is for testing and demo. This is about the next iteration.
Design feedback on #435, from the repo owner while reviewing it for release. Filing so it is not lost in a PR thread — the current shape ships for testing, this is about where it should land.
The concern
Declaring tools as a config array is the weakest part of the current design:
Three things are already known to the framework, and the config restates them:
TicketToolsalready saysmodel Ticket.Ticket→TicketAgent.So a host declares a class whose entire content is derivable, then declares it again in an initializer. Something like
Reservation < ApplicationRecordbeing reachable byReservationAgentwith no config at all is the shape worth aiming for.Why it was not done that way in #435
Not because it is hard. Enumerating models after
eager_load!is a few lines, and the convention already works. The problem is what auto-registration would expose.Measured on a real host app (27 models):
Auto-registering all of them puts
User,ApiTokenandCliDeviceGrantin front of a model by default.PROPOSAL_schema_tools.mdcalled this out directly: "Nothing should appear in an agent's toolset because a table exists."The
filterable/returnsallowlists are the other half of the same argument. They cannot be derived from the schema, because "which columns may a model filter on" is a judgement about exposure, not a fact about the table. Deriving them would mean either exposing every column or inventing a heuristic that quietly decidespassword_digestis uninteresting.Middle grounds worth considering
Ordered by how much they keep the boundary explicit:
SchemaToolssubclasses. The declaration stays the boundary, but it is declared once instead of twice —SchemaTools.descendantsafter eager-loadingapp/agent_tools. This is what the host shim did before Schema-derived agent tools: generate a bounded tool roster from an ActiveRecord model #435's seam existed, and it worked. Smallest change, removes the duplication actually complained about.rails g active_agent:schema_tools Reservationwrites a starter class with every column commented out, so the allowlist is a review step rather than a blank page.config.schema_tools_from_models = [Reservation, Ticket]— derives the roster but still names the models. Removes the tool class for simple cases.(1) looks like the clear near-term win; it is strictly less config with no change to what is exposed. (3) is the interesting one if the goal is to delete tool classes entirely for CRUD-shaped models.
Not blocking the current release
#435 ships as-is for testing and demo. This is about the next iteration.