diff --git a/packages/core/src/agents/forkedAgent.cache.test.ts b/packages/core/src/agents/forkedAgent.cache.test.ts index 5601a3cfc53..0dc14fe8c97 100644 --- a/packages/core/src/agents/forkedAgent.cache.test.ts +++ b/packages/core/src/agents/forkedAgent.cache.test.ts @@ -10,6 +10,7 @@ import { getCacheSafeParams, getCacheSafeParamsSessionId, clearCacheSafeParams, + createForkedChat, runForkedAgent, } from './forkedAgent.js'; import type { Content, GenerateContentConfig } from '@google/genai'; @@ -228,6 +229,26 @@ describe('CacheSafeParams', () => { }); }); +describe('createForkedChat', () => { + beforeEach(() => { + clearCacheSafeParams(); + vi.mocked(GeminiChat).mockReset(); + }); + + it('marks the fork so its history rewrites skip parent skill tracking', () => { + const forked = {} as unknown as GeminiChat; + vi.mocked(GeminiChat).mockImplementation(() => forked); + + saveCacheSafeParams({ systemInstruction: 'si' }, [], 'test-model'); + const chat = createForkedChat( + {} as unknown as Config, + getCacheSafeParams()!, + ); + + expect(chat.isForkedChat).toBe(true); + }); +}); + describe('runForkedAgent (cache path)', () => { beforeEach(() => { clearCacheSafeParams(); diff --git a/packages/core/src/agents/forkedAgent.ts b/packages/core/src/agents/forkedAgent.ts index 69d3fc86ac4..4390a3a62bf 100644 --- a/packages/core/src/agents/forkedAgent.ts +++ b/packages/core/src/agents/forkedAgent.ts @@ -223,7 +223,7 @@ export function createForkedChat( ? params.history.slice(-maxHistoryEntries) : params.history; - return new GeminiChat( + const forkedChat = new GeminiChat( config, { ...params.generationConfig, @@ -235,6 +235,10 @@ export function createForkedChat( undefined, // no chatRecordingService undefined, // no telemetryService ); + // The fork shares the parent's ToolRegistry; its history rewrites must + // not touch the parent's loaded-skill tracking. + forkedChat.isForkedChat = true; + return forkedChat; } interface ForkedModelRuntime { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 0d74be6e37f..e840f597ca2 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -2470,6 +2470,7 @@ export class GeminiClient { const m = mcResult.meta; const changed = m.tokensSaved > 0; if (changed) { + // setHistory conservatively clears loaded-skill tracking. this.getChat().setHistory(mcResult.history); await this.disarmFileReadCacheAfterEviction(m, 'microcompaction'); } @@ -2768,6 +2769,8 @@ export class GeminiClient { this.getChat().addHistory(entry); } } + // Loaded-skill tracking was conservatively cleared by the strip + // above; restored bodies simply re-inject on their next invoke. strippedRetryEntries = []; }; diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index e03f3014bd3..d88829c3349 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -303,6 +303,107 @@ describe('GeminiChat', async () => { } as unknown as GenerateContentResponse; } + describe('history-rewrite loaded-skill tracking', () => { + // Destructive rewrites (compaction, truncation, orphan stripping) + // conservatively clear the SkillTool's loaded-skill tracking so an + // evicted body can never stay stuck behind the dedup guard. The + // trade-off is at most one duplicate body on the next invoke. + const wireSkillTracker = () => { + const skillTool = { clearLoadedSkills: vi.fn() }; + vi.mocked(mockConfig.getToolRegistry).mockReturnValue({ + getTool: vi.fn().mockReturnValue(skillTool), + } as unknown as ReturnType); + return skillTool; + }; + + it('setHistory clears tracking on wholesale replacement', () => { + const skillTool = wireSkillTracker(); + chat.setHistory([{ role: 'user', parts: [{ text: 'hi' }] }]); + expect(skillTool.clearLoadedSkills).toHaveBeenCalled(); + }); + + it('tryCompress clears tracking through its setHistory', async () => { + const skillTool = wireSkillTracker(); + vi.spyOn( + ChatCompressionService.prototype, + 'compress', + ).mockResolvedValueOnce({ + newHistory: [{ role: 'user', parts: [{ text: 'summary' }] }], + info: { + originalTokenCount: 100_000, + newTokenCount: 30_000, + compressionStatus: CompressionStatus.COMPRESSED, + }, + }); + + await chat.tryCompress('prompt-skill-clear', true); + + expect(skillTool.clearLoadedSkills).toHaveBeenCalled(); + }); + + it('tryCompress leaves tracking untouched on NOOP', async () => { + const skillTool = wireSkillTracker(); + vi.spyOn( + ChatCompressionService.prototype, + 'compress', + ).mockResolvedValueOnce({ + newHistory: null, + info: { + originalTokenCount: 1_000, + newTokenCount: 1_000, + compressionStatus: CompressionStatus.NOOP, + }, + }); + + await chat.tryCompress('prompt-skill-noop', true); + + expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled(); + }); + + it('truncateHistory clears tracking when entries were dropped', () => { + const skillTool = wireSkillTracker(); + chat.addHistory({ role: 'user', parts: [{ text: 'a' }] }); + chat.addHistory({ role: 'model', parts: [{ text: 'b' }] }); + chat.truncateHistory(1); + expect(skillTool.clearLoadedSkills).toHaveBeenCalled(); + }); + + it('truncateHistory leaves tracking when nothing was dropped', () => { + const skillTool = wireSkillTracker(); + chat.addHistory({ role: 'user', parts: [{ text: 'a' }] }); + chat.truncateHistory(5); + expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled(); + }); + + it('stripOrphanedUserEntriesFromHistory clears tracking when it strips', () => { + const skillTool = wireSkillTracker(); + chat.addHistory({ role: 'model', parts: [{ text: 'ack' }] }); + chat.addHistory({ role: 'user', parts: [{ text: 'orphan' }] }); + chat.stripOrphanedUserEntriesFromHistory(); + expect(skillTool.clearLoadedSkills).toHaveBeenCalled(); + }); + + it('stripOrphanedUserEntriesFromHistory leaves tracking when nothing is stripped', () => { + const skillTool = wireSkillTracker(); + chat.addHistory({ role: 'model', parts: [{ text: 'ack' }] }); + chat.stripOrphanedUserEntriesFromHistory(); + expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled(); + }); + + it('forked chats never touch the shared parent tracker', () => { + const skillTool = wireSkillTracker(); + chat.isForkedChat = true; + + chat.setHistory([{ role: 'model', parts: [{ text: 'ack' }] }]); + chat.addHistory({ role: 'user', parts: [{ text: 'orphan' }] }); + chat.stripOrphanedUserEntriesFromHistory(); + chat.addHistory({ role: 'model', parts: [{ text: 'ack2' }] }); + chat.truncateHistory(1); + + expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled(); + }); + }); + describe('system instruction helpers', () => { it('replaces prior session-start context instead of appending indefinitely', () => { const isolatedChat = new GeminiChat( @@ -16400,7 +16501,10 @@ describe('GeminiChat', async () => { mockFileSystem.set(planFile, PLAN); try { const chat = new GeminiChat( - { getPlanFilePath: () => planFile } as unknown as Config, + { + getPlanFilePath: () => planFile, + getToolRegistry: () => undefined, + } as unknown as Config, {}, [], ); @@ -16432,6 +16536,7 @@ describe('GeminiChat', async () => { const chat = new GeminiChat( { getPlanFilePath: () => '/plans/never-written.md', + getToolRegistry: () => undefined, } as unknown as Config, {}, [], diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 7313c48c0f3..74535bce082 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -56,6 +56,7 @@ import { } from './tokenLimits.js'; import { hasCycleInSchema } from '../tools/tools.js'; import { ToolNames, canonicalToolName } from '../tools/tool-names.js'; +import { clearLoadedSkillTracking } from '../tools/skill-utils.js'; import * as fs from 'node:fs'; import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js'; import { isManagedMemoryPath } from '../memory/paths.js'; @@ -1973,6 +1974,15 @@ export class GeminiChat { private userContentPushCount = 0; private manualPlanExitNoticesEnabled = false; + /** + * True for forked/speculative chats built by `createForkedChat` on the + * parent's Config. They share the parent's ToolRegistry (and the single + * SkillTool tracking instance) while rewriting only a copy of a parent + * history slice, so their rewrites must not touch loaded-skill tracking — + * only the chat owning the authoritative session may. + */ + isForkedChat = false; + /** * Reset both partial-push markers in lockstep. Every history-mutation * site uses this — single-field resets are a bug because the fields @@ -2395,6 +2405,8 @@ export class GeminiChat { // the session-token-limit gate blocks a prompt that fits the // compressed history (#9506). this.tokenCountsByRouteKey.clear(); + // Loaded-skill tracking was conservatively cleared by the setHistory + // above — no second sync here. this.setLastPromptTokenCount( info.newTokenCount, info.newTokenCountIsEstimated, @@ -2873,6 +2885,8 @@ export class GeminiChat { // state. The JSONL compression checkpoint is intentionally not // written because the send is about to be rejected. this.setHistory(historyBeforeHardRescue); + // setHistory conservatively cleared loaded-skill tracking; the + // restored bodies re-arm it on their next invoke. this.lastPromptTokenCount = lastPromptTokenCountBeforeHardRescue; this.lastPromptTokenCountIsEstimated = lastPromptTokenCountWasEstimatedBeforeHardRescue; @@ -4884,9 +4898,18 @@ export class GeminiChat { // stash too: its referent (the model turn at the old index) is gone. this.clearPendingPartialState(); this.redactApprovedPlansFromLoadedHistory(); + // Wholesale replacement can drop resident skill bodies (compression, + // /restore, session-manager load_history, ACP restoreSessionHistory + // all land here). Conservatively clear the tracking so an evicted + // skill never stays stuck behind the dedup guard; a still-resident + // body costs at most one duplicate injection on the next invoke. + if (!this.isForkedChat) { + clearLoadedSkillTracking(this.config.getToolRegistry(), 'setHistory'); + } } truncateHistory(keepCount: number): void { + const prevLen = this.history.length; this.history = this.history.slice(0, keepCount); // Truncation can drop the entry the partial-push marker points at, // or leave it valid but shift the meaning of nearby indices. Reset @@ -4894,6 +4917,14 @@ export class GeminiChat { // ephemeral, so losing them across a truncate is safe (the // sendMessageStream that pushed them has already finished or will // start fresh on the next call). + if (this.history.length < prevLen && !this.isForkedChat) { + // Truncation may have dropped a skill body; conservative clear + // re-arms reload (see setHistory for the trade-off). + clearLoadedSkillTracking( + this.config.getToolRegistry(), + 'truncateHistory', + ); + } this.clearPendingPartialState(); } @@ -4950,6 +4981,16 @@ export class GeminiChat { // `sendMessageStream` would otherwise leave a stale marker that // happens to line up with whatever model entry is at that index // in the meanwhile. + if (strippedEntries.length > 0 && !this.isForkedChat) { + // The stripped entries may have carried a skill body; conservative + // clear re-arms reload (see setHistory for the trade-off). A forked + // chat shares the parent's tracker while holding only a tail slice, + // so only the authoritative session's chat may clear. + clearLoadedSkillTracking( + this.config.getToolRegistry(), + 'stripOrphanedUserEntries', + ); + } this.clearPendingPartialState(); return strippedEntries; } diff --git a/packages/core/src/hooks/registerSkillHooks.test.ts b/packages/core/src/hooks/registerSkillHooks.test.ts index fdf51e06f8c..d1c04e28c3b 100644 --- a/packages/core/src/hooks/registerSkillHooks.test.ts +++ b/packages/core/src/hooks/registerSkillHooks.test.ts @@ -226,4 +226,149 @@ describe('registerSkillHooks', () => { expect(hooks).toHaveLength(1); expect(hooks[0].skillRoot).toBe(skillRoot); }); + + it('should not duplicate hooks when the same skill registers again (skill reload)', () => { + // Skill unload (/unskill, eviction sync) never unregisters session hooks, + // so a reload must not push duplicate entries — otherwise the hook fires + // once per unload/reload cycle. + const skill: SkillConfig = { + name: 'test-skill', + description: 'Test skill', + level: 'user', + filePath: '/path/to/skill/SKILL.md', + skillRoot, + body: 'Test body', + hooks: { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { + type: HookType.Command, + command: 'echo "checking command"', + }, + ], + }, + ], + }, + }; + + expect(registerSkillHooks(sessionHooksManager, sessionId, skill)).toBe(1); + expect(registerSkillHooks(sessionHooksManager, sessionId, skill)).toBe(0); + + const hooks = sessionHooksManager.getHooksForEvent( + sessionId, + HookEventName.PreToolUse, + ); + expect(hooks).toHaveLength(1); + }); + + it('still registers a same-command hook from a different skill', () => { + const makeSkill = (name: string, root: string): SkillConfig => ({ + name, + description: 'Test skill', + level: 'user', + filePath: `${root}/SKILL.md`, + skillRoot: root, + body: 'Test body', + hooks: { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { + type: HookType.Command, + command: 'echo "checking command"', + }, + ], + }, + ], + }, + }); + + expect( + registerSkillHooks( + sessionHooksManager, + sessionId, + makeSkill('skill-a', '/path/to/a'), + ), + ).toBe(1); + expect( + registerSkillHooks( + sessionHooksManager, + sessionId, + makeSkill('skill-b', '/path/to/b'), + ), + ).toBe(1); + + const hooks = sessionHooksManager.getHooksForEvent( + sessionId, + HookEventName.PreToolUse, + ); + expect(hooks).toHaveLength(2); + }); + + it('registers same-command hooks that differ only in timeout (R1-1)', () => { + const skill: SkillConfig = { + name: 'test-skill', + description: 'Test skill', + level: 'user', + filePath: '/path/to/skill/SKILL.md', + skillRoot, + body: 'Test body', + hooks: { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { + type: HookType.Command, + command: 'echo hi', + timeout: 10, + }, + { + type: HookType.Command, + command: 'echo hi', + timeout: 30, + }, + ], + }, + ], + }, + }; + + expect(registerSkillHooks(sessionHooksManager, sessionId, skill)).toBe(2); + }); + + it('registers same-URL http hooks that differ only in headers (R1-1)', () => { + const skill: SkillConfig = { + name: 'test-skill', + description: 'Test skill', + level: 'user', + filePath: '/path/to/skill/SKILL.md', + skillRoot, + body: 'Test body', + hooks: { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { + type: HookType.Http, + url: 'http://gw.local/hook', + headers: { Authorization: 'Bearer a' }, + }, + { + type: HookType.Http, + url: 'http://gw.local/hook', + headers: { Authorization: 'Bearer b' }, + }, + ], + }, + ], + }, + }; + + expect(registerSkillHooks(sessionHooksManager, sessionId, skill)).toBe(2); + }); }); diff --git a/packages/core/src/hooks/registerSkillHooks.ts b/packages/core/src/hooks/registerSkillHooks.ts index bac45b89fab..1274e6359f6 100644 --- a/packages/core/src/hooks/registerSkillHooks.ts +++ b/packages/core/src/hooks/registerSkillHooks.ts @@ -18,6 +18,7 @@ import type { SkillHooksSettings, SkillConfig } from '../skills/types.js'; import { HookType, type HookEventName, + type HookConfig, type CommandHookConfig, type HttpHookConfig, } from './types.js'; @@ -71,6 +72,25 @@ export function registerSkillHooks( skill.skillRoot, ); + // Skip hooks this skill already registered earlier in the session. + // Unloading a skill body (/unskill, eviction sync) never unregisters + // its session hooks, so without this dedup every unload/reload cycle + // would push a duplicate entry and the hook would fire once per cycle. + const alreadyRegistered = sessionHooksManager + .getHooksForEvent(sessionId, eventName) + .some( + (entry) => + entry.matcher === matcherPattern && + entry.skillRoot === skill.skillRoot && + hookConfigKey(entry.config) === hookConfigKey(hookConfig), + ); + if (alreadyRegistered) { + debugLogger.debug( + `Hook for ${eventName} with matcher '${matcherPattern}' from skill '${skill.name}' already registered; skipping duplicate`, + ); + continue; + } + sessionHooksManager.addSessionHook( sessionId, eventName, @@ -96,6 +116,18 @@ export function registerSkillHooks( return registeredCount; } +/** + * Identity key for dedup: the whole prepared config. Keying on only + * type + command/url silently drops distinct hooks the frontmatter + * admits per matcher (same command with different timeout/shell, same + * URL with different headers) — the second of the pair is skipped even + * on first registration. Prepared configs from frontmatter carry no + * functions, so a structural key is stable across reload cycles. + */ +function hookConfigKey(hook: HookConfig): string { + return `${hook.type}:${JSON.stringify(hook)}`; +} + /** * Prepares hook config with skillRoot environment variable. * diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 37694cd08e9..15ba82ba3f4 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -3199,6 +3199,15 @@ describe('PermissionManager', () => { expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); + it('addSessionAllowRule deduplicates identical rules', () => { + pm.addSessionAllowRule('Bash(git *)'); + pm.addSessionAllowRule('Bash(git *)'); + expect( + (pm as unknown as { sessionRules: { allow: unknown[] } }).sessionRules + .allow, + ).toHaveLength(1); + }); + it('malformed session allow rule is silently ignored', async () => { pm.addSessionAllowRule('Bash(git commit'); // 'git commit' is not readonly, so default is 'ask'. diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index f6aa226b820..5c7bf94b364 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -1255,6 +1255,13 @@ export class PermissionManager { ); return; } + // Deduplicate on raw string — mirrors addPersistentRule and the + // dangerous-stash branch above. Reload cycles (e.g. /unskill + + // re-invoke) re-run applySkillAllowedTools; without this guard the + // skill's allowedTools list would accumulate on every cycle. + if (this.sessionRules.allow.some((r) => r.raw === rule.raw)) { + return; + } this.sessionRules.allow.push(rule); } } diff --git a/packages/core/src/telemetry/uiTelemetry.test.ts b/packages/core/src/telemetry/uiTelemetry.test.ts index ecee0f9be17..bff65f5c2b4 100644 --- a/packages/core/src/telemetry/uiTelemetry.test.ts +++ b/packages/core/src/telemetry/uiTelemetry.test.ts @@ -1409,9 +1409,7 @@ describe('UiTelemetryService', () => { service.addEvent(makeApiEvent('model-a', 77), SESSION_B); expect(service.getMetricsForSession(SESSION_B).models).toEqual({}); // ...but the aggregate still counts it. - expect(service.getMetrics().models['model-a']?.api.totalRequests).toBe( - 2, - ); + expect(service.getMetrics().models['model-a']?.api.totalRequests).toBe(2); }); it('keeps the bySource null prototype across restore', () => { diff --git a/packages/core/src/tools/skill-utils.test.ts b/packages/core/src/tools/skill-utils.test.ts index 82d29cfc726..aaacd96f24b 100644 --- a/packages/core/src/tools/skill-utils.test.ts +++ b/packages/core/src/tools/skill-utils.test.ts @@ -9,7 +9,10 @@ import { applySkillAllowedTools, collectAvailableSkillEntries, clearCollectedSkillEntriesCache, + clearLoadedSkillTracking, } from './skill-utils.js'; +import { ToolNames } from './tool-names.js'; +import type { ToolRegistry } from './tool-registry.js'; import type { PermissionManager } from '../permissions/permission-manager.js'; import type { SkillManager } from '../skills/skill-manager.js'; import type { Config } from '../config/config.js'; @@ -147,3 +150,30 @@ describe('collectAvailableSkillEntries memoize cache', () => { expect(sm.listSkills).toHaveBeenCalledTimes(2); }); }); + +describe('clearLoadedSkillTracking', () => { + it('clears the SkillTool tracker when one is registered', () => { + const clearLoadedSkills = vi.fn(); + const registry = { + getTool: vi.fn().mockReturnValue({ clearLoadedSkills }), + } as unknown as ToolRegistry; + + clearLoadedSkillTracking(registry, 'test-boundary'); + + expect(registry.getTool).toHaveBeenCalledWith(ToolNames.SKILL); + expect(clearLoadedSkills).toHaveBeenCalledTimes(1); + }); + + it('no-ops when the registry or tracker is missing', () => { + expect(() => + clearLoadedSkillTracking(undefined, 'test-boundary'), + ).not.toThrow(); + + const registry = { + getTool: vi.fn().mockReturnValue(undefined), + } as unknown as ToolRegistry; + expect(() => + clearLoadedSkillTracking(registry, 'test-boundary'), + ).not.toThrow(); + }); +}); diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index 4cee298e979..fecbf0c9f6f 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -8,7 +8,12 @@ import type { PermissionManager } from '../permissions/permission-manager.js'; import type { Config } from '../config/config.js'; import type { SkillManager } from '../skills/skill-manager.js'; import type { SkillConfig, SkillLevel } from '../skills/types.js'; +import type { ToolRegistry } from './tool-registry.js'; +import { ToolNames } from './tool-names.js'; import { escapeXml } from '../utils/xml.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('SKILL'); /** * Builds the LLM-facing content string when a skill body is injected. @@ -291,3 +296,29 @@ export function applySkillAllowedTools( permissionManager.addSessionAllowRule(rule); } } + +/** + * Conservatively drop ALL loaded-skill tracking after a destructive + * history rewrite (compaction, truncation, orphan stripping). The rewrite + * may have removed a skill body; the dedup guard must not leave that + * skill permanently unreloadable behind "already loaded in context". + * Over-clearing is the safe direction: a still-resident body costs at + * most one duplicate injection on the next invoke, while a stale entry + * makes the body unrecoverable until session restart. + * + * Duck-typed (mirroring `clearCommand`'s existing `clearLoadedSkills` + * call) so history-rewrite sites don't need a runtime import of the + * SkillTool class. + */ +export function clearLoadedSkillTracking( + toolRegistry: ToolRegistry | undefined, + logTag: string, +): void { + const tool = toolRegistry?.getTool(ToolNames.SKILL); + if (tool && 'clearLoadedSkills' in tool) { + (tool as { clearLoadedSkills(): void }).clearLoadedSkills(); + debugLogger.debug( + `[SKILL_TRACKING] conservatively cleared loaded-skill tracking after ${logTag}`, + ); + } +} diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index b6c390c23f1..f4c0af67279 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -1408,6 +1408,10 @@ describe('SkillTool', () => { const llmText = partToString(result.llmContent); expect(llmText).toBe('Prompt content from MCP'); expect(result.returnDisplay).toBe('Executed command: mcp-prompt-a'); + // Command delegations are NOT tracked: the result is raw command + // text, not a skill body, so a tracked name here would block a + // later same-named file skill behind the dedup guard. + expect([...skillTool.getLoadedSkillNames()]).toEqual([]); }); it('should fall through to not-found error when executor returns null', async () => { diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 6804b69b3c7..afc4dc74afc 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -328,8 +328,10 @@ export class SkillTool extends BaseDeclarativeTool { } /** - * Clears the loaded-skills tracking. Should be called when the session - * is reset (e.g. /clear) so that stale body-token data is not shown. + * Clears the loaded-skills tracking. Called when the session is reset + * (e.g. /clear) and conservatively at destructive history-rewrite + * boundaries (compaction, truncation, orphan stripping), so a skill + * whose body was evicted never stays stuck behind the dedup guard. */ clearLoadedSkills(): void { this.loadedSkillNames.clear(); @@ -549,7 +551,11 @@ class SkillToolInvocation extends BaseToolInvocation { this.config, new SkillLaunchEvent(this.params.skill, true, this.promptId), ); - this.onSkillLoaded(this.params.skill); + // Don't track via `onSkillLoaded` (mirrors the disabled + // branch above): the result is raw command text, not a + // skill body, so a tracked name here would block a later + // same-named file skill behind the dedup guard even though + // no body is resident. return { llmContent: [{ text: commandResult }], returnDisplay: `Executed command: ${this.params.skill}`,