From 05545ba283b333f82516f23452964f426d6504f5 Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:44:56 +0530 Subject: [PATCH] fix(x): gate session skill carry-forward on the skillCarryForward trait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit withActiveSkills injected carried-forward skills into every non-inline agent request, while the resolver only honors them for agents with the skillCarryForward trait (real-agent-resolver). The mismatch persisted an ever-growing activeSkills list into the requested composition of every turn for agents that would never use it. The gate now lives in withActiveSkills, mirroring the resolver. That required a structural fix: sessions cannot import assembly/registry.js (its agent builders transitively reach di/container, which imports sessions — the module cycle broke SessionsImpl registration under test). Traits move to assembly/traits.ts, a zero-import leaf, with registry re-exporting them so existing assembly-level importers are unchanged. Regression test pins the non-trait agent case. Co-Authored-By: Claude Fable 5 --- .../core/src/runtime/assembly/registry.ts | 45 ++++------------- .../core/src/runtime/assembly/traits.ts | 49 +++++++++++++++++++ .../src/runtime/sessions/sessions.test.ts | 18 +++++++ .../core/src/runtime/sessions/sessions.ts | 14 +++++- 4 files changed, 90 insertions(+), 36 deletions(-) create mode 100644 apps/x/packages/core/src/runtime/assembly/traits.ts diff --git a/apps/x/packages/core/src/runtime/assembly/registry.ts b/apps/x/packages/core/src/runtime/assembly/registry.ts index 00372cf8..a874893a 100644 --- a/apps/x/packages/core/src/runtime/assembly/registry.ts +++ b/apps/x/packages/core/src/runtime/assembly/registry.ts @@ -14,24 +14,20 @@ import { lazyResolve } from "../../di/lazy-resolve.js"; import type { IAgentsRepo } from "./repo.js"; // The registry of built-in agents: one table instead of the historical -// if (id === ...) ladder. An entry owns its builder and its traits; traits -// replace stringly-typed id comparisons ("copilot" || "rowboatx") scattered -// across the assembly layer. User-defined agents are the fallthrough, -// fetched from the agents repo. +// if (id === ...) ladder. An entry owns its builder. Traits (which replace +// stringly-typed id comparisons scattered across the assembly layer) live in +// traits.ts — a leaf module, because this file's builders transitively reach +// di/container and upstream trait readers would cycle. Re-exported here so +// assembly-level consumers keep one import. -export interface AgentTraits { - // Receives workspace context — agent notes and the user work directory — - // composed into its system prompt. - workspaceContext?: boolean; - // Session-loaded skills (activeSkills) re-attach their tools on later - // turns. Distinct from workspaceContext: a trait per concern, so neither - // silently inherits the other's meaning. - skillCarryForward?: boolean; -} +export { + carriesSkillsForward, + hasWorkspaceContext, + type AgentTraits, +} from "./traits.js"; interface BuiltinAgentDefinition { build: () => Promise> | z.infer; - traits?: AgentTraits; } // Prompt-file agents: instructions ship as a raw string whose optional YAML @@ -55,7 +51,6 @@ function promptFileAgent(id: string, getRaw: () => string): BuiltinAgentDefiniti // definition object. const COPILOT: BuiltinAgentDefinition = { build: buildCopilotAgent, - traits: { workspaceContext: true, skillCarryForward: true }, }; const builtinAgents: Record = { @@ -75,26 +70,6 @@ export function builtinAgentIds(): string[] { return Object.keys(builtinAgents); } -// Trait lookups for assembly decisions. Unknown/user agents have no traits. -function hasTrait( - agentId: string | null | undefined, - trait: keyof AgentTraits, -): boolean { - return ( - agentId != null && - Object.hasOwn(builtinAgents, agentId) && - builtinAgents[agentId].traits?.[trait] === true - ); -} - -export function hasWorkspaceContext(agentId: string | null | undefined): boolean { - return hasTrait(agentId, "workspaceContext"); -} - -export function carriesSkillsForward(agentId: string | null | undefined): boolean { - return hasTrait(agentId, "skillCarryForward"); -} - export async function loadAgent(id: string): Promise> { // Object.hasOwn: a plain lookup would traverse Object.prototype, so a // user agent named "constructor"/"toString" would hit an inherited diff --git a/apps/x/packages/core/src/runtime/assembly/traits.ts b/apps/x/packages/core/src/runtime/assembly/traits.ts new file mode 100644 index 00000000..2cf8a600 --- /dev/null +++ b/apps/x/packages/core/src/runtime/assembly/traits.ts @@ -0,0 +1,49 @@ +// Built-in agent traits: static metadata with deliberately zero imports. +// The registry (registry.ts) owns the id -> builder table, but its builders +// drag in the copilot instruction chain, which transitively reaches +// di/container — so upstream layers (sessions) reading a trait through the +// registry would create a module cycle back into themselves. Traits live in +// this leaf so trait checks are safe to import from anywhere. + +export interface AgentTraits { + // Receives workspace context — agent notes and the user work directory — + // composed into its system prompt. + workspaceContext?: boolean; + // Session-loaded skills (activeSkills) re-attach their tools on later + // turns. Distinct from workspaceContext: a trait per concern, so neither + // silently inherits the other's meaning. + skillCarryForward?: boolean; +} + +// "rowboatx" is a legacy alias for the copilot: both ids share one traits +// object. Agents absent from this table have no traits (user agents, and +// builtins that need none). +const COPILOT_TRAITS: AgentTraits = { + workspaceContext: true, + skillCarryForward: true, +}; + +const agentTraits: Record = { + copilot: COPILOT_TRAITS, + rowboatx: COPILOT_TRAITS, +}; + +// Trait lookups for assembly decisions. Unknown/user agents have no traits. +function hasTrait( + agentId: string | null | undefined, + trait: keyof AgentTraits, +): boolean { + return ( + agentId != null && + Object.hasOwn(agentTraits, agentId) && + agentTraits[agentId][trait] === true + ); +} + +export function hasWorkspaceContext(agentId: string | null | undefined): boolean { + return hasTrait(agentId, "workspaceContext"); +} + +export function carriesSkillsForward(agentId: string | null | undefined): boolean { + return hasTrait(agentId, "skillCarryForward"); +} diff --git a/apps/x/packages/core/src/runtime/sessions/sessions.test.ts b/apps/x/packages/core/src/runtime/sessions/sessions.test.ts index e1252a43..6ce9332b 100644 --- a/apps/x/packages/core/src/runtime/sessions/sessions.test.ts +++ b/apps/x/packages/core/src/runtime/sessions/sessions.test.ts @@ -943,6 +943,24 @@ describe("active-skill carry-forward", () => { }); }); + it("does not inject activeSkills for agents without the skillCarryForward trait", async () => { + // The resolver would ignore them anyway (real-agent-resolver gates on + // the same trait); this pins that sessions doesn't persist an + // ever-growing list into the requested composition either. + const { sessions, fake } = makeSessions(); + const sessionId = await sessions.createSession(); + const { turnId } = await sessions.sendMessage(sessionId, user("one"), { + agent: { agentId: "my-user-agent" }, + }); + await flush(); + fake.setLog(turnId, skillLoadLog(turnId, sessionId, { agentId: "my-user-agent" })); + + await sessions.sendMessage(sessionId, user("two"), { + agent: { agentId: "my-user-agent" }, + }); + expect(fake.createTurnInputs[1].agent).toEqual({ agentId: "my-user-agent" }); + }); + it("accumulates across turns and unions with caller-supplied skills, preserving order", async () => { const { sessions, fake } = makeSessions(); const sessionId = await sessions.createSession(); diff --git a/apps/x/packages/core/src/runtime/sessions/sessions.ts b/apps/x/packages/core/src/runtime/sessions/sessions.ts index 97162ee1..5487735c 100644 --- a/apps/x/packages/core/src/runtime/sessions/sessions.ts +++ b/apps/x/packages/core/src/runtime/sessions/sessions.ts @@ -28,6 +28,10 @@ import type { TurnExternalInput, TurnOutcome, } from "../turns/api.js"; +// traits.js, not registry.js: the registry's builders transitively reach +// di/container, which imports this module — a cycle. The traits leaf exists +// exactly so upstream layers can gate on traits. +import { carriesSkillsForward } from "../assembly/traits.js"; import type { IClock } from "../turns/clock.js"; import { type ISessions, @@ -461,7 +465,15 @@ function withActiveSkills( agent: SendMessageConfig["agent"], activeSkills: string[], ): SendMessageConfig["agent"] { - if (isInlineAgentRequest(agent) || activeSkills.length === 0) { + // Mirrors the resolver's carriesSkillsForward gate (real-agent-resolver): + // the resolver would ignore activeSkills for a non-trait agent anyway, + // but injecting them here would still persist an ever-growing list into + // every turn's requested composition. + if ( + isInlineAgentRequest(agent) || + activeSkills.length === 0 || + !carriesSkillsForward(agent.agentId) + ) { return agent; } const composition = agent.overrides?.composition;