mirror of
https://github.com/rowboatlabs/rowboat.git
synced 2026-07-12 21:02:17 +02:00
fix(x): gate session skill carry-forward on the skillCarryForward trait
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 <noreply@anthropic.com>
This commit is contained in:
parent
b6c2905af7
commit
05545ba283
4 changed files with 90 additions and 36 deletions
|
|
@ -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<typeof Agent>> | z.infer<typeof Agent>;
|
||||
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<string, BuiltinAgentDefinition> = {
|
||||
|
|
@ -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<z.infer<typeof Agent>> {
|
||||
// Object.hasOwn: a plain lookup would traverse Object.prototype, so a
|
||||
// user agent named "constructor"/"toString" would hit an inherited
|
||||
|
|
|
|||
49
apps/x/packages/core/src/runtime/assembly/traits.ts
Normal file
49
apps/x/packages/core/src/runtime/assembly/traits.ts
Normal file
|
|
@ -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<string, AgentTraits> = {
|
||||
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");
|
||||
}
|
||||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue