diff --git a/packages/agent-core/src/agent/injection/skill-refresh.ts b/packages/agent-core/src/agent/injection/skill-refresh.ts index 7c5dbe0f4..c96b550c3 100644 --- a/packages/agent-core/src/agent/injection/skill-refresh.ts +++ b/packages/agent-core/src/agent/injection/skill-refresh.ts @@ -23,16 +23,20 @@ export class SkillRefreshInjector extends DynamicInjector { override getInjection(): string | undefined { const registry = this.agent.skills?.registry; if (registry === undefined) return undefined; - // No baseline captured means the agent never rendered a profile prompt with - // a skill listing (e.g. a resumed agent replaying its prompt, or a bare test - // agent). Without a baseline we cannot tell what the model already sees, so - // surface nothing rather than risk a spurious reminder. const baseline = this.agent.systemPromptSkillListing; - if (baseline === undefined) return undefined; const current = registry.getModelSkillListing(); + if (current.length === 0) return undefined; + // Native resume replays the system prompt from records without calling + // useProfile, so no baseline exists. In that case, compare against the + // replayed prompt string itself: if it already contains the current listing, + // there is nothing to surface; otherwise a plugin was added after the prompt + // was recorded and the model needs the fresh listing. + if (baseline === undefined && this.agent.config?.systemPrompt.includes(current)) { + return undefined; + } // While the live listing still matches the one baked into the system // prompt, there is nothing extra to surface. - if (current === baseline) return undefined; + if (baseline !== undefined && current === baseline) return undefined; // The listing drifted from the prompt baseline. Surface it once; only // re-surface if it changed again or scrolled out of context via compaction // (the base class nulls `injectedAt` in that case). diff --git a/packages/agent-core/src/session/index.ts b/packages/agent-core/src/session/index.ts index 8cdd81916..314bfd8d7 100644 --- a/packages/agent-core/src/session/index.ts +++ b/packages/agent-core/src/session/index.ts @@ -39,6 +39,7 @@ import { resolveSkillRoots, SkillRegistry, summarizeSkill, + type SkillDefinition, type SkillRoot, type SkillSummary, } from '../skill'; @@ -110,6 +111,10 @@ export class Session { // recorded digest — or that drops a recorded server — means the live session // is stale and only a new session can reconcile it. private readonly loadedPluginMcpDigests = new Map(); + // Digest of every plugin skill actually loaded into this session. The map is + // additive for the same reason as the registry: removed or changed entries are + // stale until a new session starts with a fresh registry. + private readonly loadedPluginSkillDigests = new Map(); metadata: SessionMeta = { createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), @@ -190,14 +195,6 @@ export class Session { if (main !== undefined && profile !== undefined && main.config.systemPrompt === '') { await this.bootstrapAgentProfile(main, profile); } - // Native resume replays the system prompt from the wire without calling - // useProfile, so the skill-listing baseline is never captured. Seed it now - // (it matches the replayed prompt) so the SkillRefreshInjector can detect - // when a later /plugins reload makes the listing stale. - const skillListing = this.skills.getModelSkillListing(); - for (const agent of this.agents.values()) { - agent.systemPromptSkillListing ??= skillListing; - } await this.triggerSessionStart('resume'); return { warning: resumeWarning }; } @@ -343,11 +340,11 @@ export class Session { /** * Hot-apply a plugin runtime snapshot to this live session. Additive only: - * newly enabled plugin skills are loaded and the main agent's system prompt - * is re-rendered so the model sees them; the `Skill` builtin tool is - * refreshed so it appears when skills first become available; and newly - * enabled plugin MCP servers are connected (their tools register via the MCP - * status subscription and are picked up on the next turn). Disabled/removed + * newly enabled plugin skills are loaded and surfaced to the model by the + * SkillRefreshInjector; the `Skill` builtin tool is refreshed so it appears + * when skills first become available; and newly enabled plugin MCP servers are + * connected (their tools register via the MCP status subscription and are + * picked up on the next turn). Disabled/removed * capabilities are NOT torn down and sessionStart skills are NOT injected * mid-conversation — those are reported via `needsNewSession`. */ @@ -357,8 +354,10 @@ export class Session { await this.skillsReady; // Skills: re-resolve roots with the snapshot's plugin roots and merge them - // in. loadRoots skips already-loaded roots, so only new ones are scanned. + // in additively. Existing skills are not replaced; if a plugin changes a + // loaded skill in place, `needsNewSession` reports the stale runtime instead. const before = new Set(this.skills.listSkills().map((skill) => skill.name)); + const desiredSkillDigests = await this.desiredPluginSkillDigests(snapshot); const roots = await resolveSkillRoots({ paths: { userHomeDir: this.options.skills?.userHomeDir ?? homedir(), @@ -370,7 +369,12 @@ export class Session { mergeAllAvailableSkills: this.options.skills?.mergeAllAvailableSkills, builtinDir: this.options.skills?.builtinDir, }); - await this.skills.loadRoots(roots); + await this.skills.loadRoots(roots, { replace: false }); + for (const [key, digest] of desiredSkillDigests) { + if (!this.loadedPluginSkillDigests.has(key)) { + this.loadedPluginSkillDigests.set(key, digest); + } + } const addedSkills = this.skills .listSkills() .map((skill) => skill.name) @@ -407,27 +411,32 @@ export class Session { (name) => this.mcp.get(name)?.status === 'connected', ); - // The skill names the plugins currently declare (re-discovered from the - // snapshot roots). Compared against the additive registry, this detects a - // skill that was removed or renamed by a plugin update. - const desiredSkillKeys = await this.desiredPluginSkillKeys(snapshot); - return { addedSkills, addedMcpServers, - needsNewSession: this.pluginRuntimeNeedsNewSession(snapshot, main, desiredSkillKeys), + needsNewSession: this.pluginRuntimeNeedsNewSession(snapshot, main, desiredSkillDigests), }; } - /** `${pluginId}\0${skillName}` for every skill the snapshot's plugins currently declare. */ - private async desiredPluginSkillKeys(snapshot: PluginRuntimeSnapshot): Promise> { - if (snapshot.pluginSkillRoots.length === 0) return new Set(); - const discovered = await discoverSkills({ roots: snapshot.pluginSkillRoots }); - const keys = new Set(); + /** Current digest for every skill the snapshot's plugins declare. */ + private async desiredPluginSkillDigests( + snapshot: PluginRuntimeSnapshot, + ): Promise> { + return this.pluginSkillDigests(snapshot.pluginSkillRoots); + } + + private async pluginSkillDigests( + roots: readonly SkillRoot[] | undefined, + ): Promise> { + if (roots === undefined || roots.length === 0) return new Map(); + const discovered = await discoverSkills({ roots }); + const digests = new Map(); for (const skill of discovered) { - if (skill.plugin?.id !== undefined) keys.add(pluginSkillKey(skill.plugin.id, skill.name)); + if (skill.plugin?.id !== undefined) { + digests.set(pluginSkillKey(skill.plugin.id, skill.name), pluginSkillDigest(skill)); + } } - return keys; + return digests; } /** @@ -436,9 +445,8 @@ export class Session { * are hot-loaded and do NOT count; this detects capabilities that were * removed or CHANGED IN PLACE by a plugin update, which the running session * cannot tear down or hot-swap: - * - a loaded plugin skill (in the additive registry) that the plugin no - * longer declares — i.e. the plugin was disabled/removed, or it renamed - * or dropped that skill; + * - a loaded plugin skill that the plugin no longer declares or now declares + * with different content; * - a connected plugin MCP server that is gone, or whose config (command / * args / env / cwd / …) changed since we connected it; * - a drift between the desired and currently-active sessionStart injections. @@ -446,15 +454,14 @@ export class Session { private pluginRuntimeNeedsNewSession( snapshot: PluginRuntimeSnapshot, main: Agent | undefined, - desiredSkillKeys: ReadonlySet, + desiredSkillDigests: ReadonlyMap, ): boolean { // Skills: the registry is additive (no unload), so any loaded plugin skill - // the plugins no longer declare is stale — covers disable/remove of a plugin - // and remove/rename of an individual skill. New skills are simply absent - // from the registry until loaded, so they never trip this. - const staleSkill = this.skills.listSkills().some((skill) => { - const pluginId = skill.plugin?.id; - return pluginId !== undefined && !desiredSkillKeys.has(pluginSkillKey(pluginId, skill.name)); + // the plugins no longer declare — or now declare with different content — + // is stale. New skills are recorded after additive loading and do not trip this. + const staleSkill = [...this.loadedPluginSkillDigests].some(([key, digest]) => { + const desired = desiredSkillDigests.get(key); + return desired === undefined || desired !== digest; }); if (staleSkill) return true; @@ -490,6 +497,15 @@ export class Session { }); await this.skills.loadRoots(roots); registerBuiltinSkills(this.skills); + await this.recordLoadedPluginSkillDigests(this.options.skills?.pluginSkillRoots); + } + + private async recordLoadedPluginSkillDigests( + roots: readonly SkillRoot[] | undefined, + ): Promise { + for (const [key, digest] of await this.pluginSkillDigests(roots)) { + this.loadedPluginSkillDigests.set(key, digest); + } } private async loadMcpServers(): Promise { @@ -670,7 +686,17 @@ export class Session { export * from './subagent-host'; function pluginSkillKey(pluginId: string, skillName: string): string { - return `${pluginId}${skillName}`; + return `${pluginId}\0${skillName}`; +} + +function pluginSkillDigest(skill: SkillDefinition): string { + return stableStringify({ + path: skill.path, + description: skill.description, + content: skill.content, + metadata: skill.metadata, + instructions: skill.plugin?.instructions, + }); } /** diff --git a/packages/agent-core/src/skill/registry.ts b/packages/agent-core/src/skill/registry.ts index a03d36c45..89fa375f6 100644 --- a/packages/agent-core/src/skill/registry.ts +++ b/packages/agent-core/src/skill/registry.ts @@ -21,6 +21,10 @@ export interface SkillRegistryOptions { readonly sessionId?: string; } +export interface LoadSkillRootsOptions { + readonly replace?: boolean; +} + export class SkillRegistry { private readonly byName = new Map(); private readonly byPluginAndName = new Map(); @@ -36,7 +40,11 @@ export class SkillRegistry { this.sessionId = options.sessionId; } - async loadRoots(roots: readonly SkillRoot[]): Promise { + async loadRoots( + roots: readonly SkillRoot[], + options: LoadSkillRootsOptions = {}, + ): Promise { + const replace = options.replace ?? true; for (const root of roots) { if (!this.roots.includes(root.path)) this.roots.push(root.path); } @@ -46,12 +54,12 @@ export class SkillRegistry { onWarning: this.onWarning, onSkippedByPolicy: (skill) => this.skipped.push(skill), onDiscoveredSkill: (skill) => { - this.indexPluginSkill(skill); + this.indexPluginSkill(skill, { replace }); }, } satisfies DiscoverSkillsOptions); for (const skill of skills) { - this.byName.set(normalizeSkillName(skill.name), skill); + this.register(skill, { replace }); } } diff --git a/packages/agent-core/test/harness/plugin-reload-session.test.ts b/packages/agent-core/test/harness/plugin-reload-session.test.ts index c0d171bca..641f71114 100644 --- a/packages/agent-core/test/harness/plugin-reload-session.test.ts +++ b/packages/agent-core/test/harness/plugin-reload-session.test.ts @@ -230,23 +230,83 @@ describe('plugin reload hot-apply to a live session', () => { expect(second.applied?.needsNewSession).toBe(false); }); - it('captures the skill-listing baseline on resume so a later reload can still surface skills', async () => { + it('flags needsNewSession and keeps the loaded skill when a plugin changes an existing skill body', async () => { + const { core, rpc } = await createTestRpc(); + const created = await rpc.createSession({ id: 'ses_skill_body_change', workDir }); + + await rpc.installPlugin({ + source: await makePlugin('editpack', { + skillNames: ['edit-skill'], + skillContents: { 'edit-skill': 'old body' }, + }), + }); + await rpc.reloadPlugins({ sessionId: created.id }); + const session = core.sessions.get(created.id)!; + expect(session.skills.getSkill('edit-skill')?.content).toContain('old body'); + + await rpc.installPlugin({ + source: await makePlugin('editpack', { + skillNames: ['edit-skill'], + skillContents: { 'edit-skill': 'new body' }, + }), + }); + const second = await rpc.reloadPlugins({ sessionId: created.id }); + + expect(second.applied?.needsNewSession).toBe(true); + expect(session.skills.getSkill('edit-skill')?.content).toContain('old body'); + }); + + it('does not duplicate the skill listing on resume when the replayed prompt already has it', async () => { const first = await createTestRpc(); + await first.rpc.installPlugin({ + source: await makePlugin('prepack', { skillNames: ['preloaded-skill'] }), + }); const created = await first.rpc.createSession({ id: 'ses_resume_base', workDir }); + const original = first.core.sessions.get(created.id)?.agents.get('main')?.config.systemPrompt; + expect(original).toContain('preloaded-skill'); await first.core.sessions.get(created.id)?.flushMetadata(); // A fresh process resumes the session: useProfile is NOT called, but the - // baseline must still be seeded (otherwise SkillRefreshInjector goes silent). + // replayed prompt already contains the current listing, so no reminder is needed. const second = await createTestRpc(); await second.rpc.resumeSession({ sessionId: created.id }); const main = second.core.sessions.get(created.id)?.agents.get('main'); - expect(main?.systemPromptSkillListing).toBeDefined(); + await main?.injection.inject(); + const injected = main?.context.history.some((message) => + message.content.some((part) => part.type === 'text' && part.text.includes('preloaded-skill')), + ); + expect(injected).toBe(false); + }); + + it('surfaces plugin skills installed after a session was saved when that session is resumed', async () => { + const first = await createTestRpc(); + const created = await first.rpc.createSession({ id: 'ses_resume_after_install', workDir }); + const original = first.core.sessions.get(created.id)?.agents.get('main')?.config.systemPrompt; + expect(original).not.toContain('resume-hot-skill'); + await first.core.sessions.get(created.id)?.flushMetadata(); + + await first.rpc.installPlugin({ + source: await makePlugin('resumepack', { skillNames: ['resume-hot-skill'] }), + }); + + const second = await createTestRpc(); + await second.rpc.resumeSession({ sessionId: created.id }); + const main = second.core.sessions.get(created.id)?.agents.get('main'); + expect(main?.config.systemPrompt).not.toContain('resume-hot-skill'); + + await main?.injection.inject(); + + const injected = main?.context.history.some((message) => + message.content.some((part) => part.type === 'text' && part.text.includes('resume-hot-skill')), + ); + expect(injected).toBe(true); }); async function makePlugin( name: string, options: { readonly skillNames?: readonly string[]; + readonly skillContents?: Readonly>; readonly mcpServers?: Record; readonly sessionStartSkill?: string; } = {}, @@ -258,7 +318,7 @@ describe('plugin reload hot-apply to a live session', () => { await mkdir(join(root, 'skills', skillName), { recursive: true }); await writeFile( join(root, 'skills', skillName, 'SKILL.md'), - `---\nname: ${skillName}\ndescription: A hot-loaded skill\n---\nbody`, + `---\nname: ${skillName}\ndescription: A hot-loaded skill\n---\n${options.skillContents?.[skillName] ?? 'body'}`, 'utf8', ); }