From cf255eebb168e3dcd331dedfcd513d3f28cb45fd Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:41:37 +0800 Subject: [PATCH] fix(scope): harden merged tool and skill boundaries --- docs/cordis-catalog/services.md | 4 +- .../cordis/tool-cordis/src/api-catalog.ts | 4 +- packages/core/tools/README.md | 2 +- packages/core/tools/src/index.ts | 44 +++++++++++- packages/core/tools/tests/tools.spec.ts | 60 ++++++++++++++++ packages/skill/skill/README.md | 6 +- packages/skill/skill/src/index.ts | 49 ++++++++----- packages/skill/skill/tests/skill.spec.ts | 68 +++++++++++++++++-- packages/skill/tool-skill/README.md | 2 +- packages/skill/tool-skill/package.json | 1 + packages/skill/tool-skill/src/index.ts | 31 ++++++--- .../skill/tool-skill/tests/tool-skill.spec.ts | 50 +++++++++++++- packages/skill/tool-skill/tsconfig.json | 1 + pnpm-lock.yaml | 3 + 14 files changed, 283 insertions(+), 42 deletions(-) diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index ef4a30a3c3..1dad87a861 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -227,8 +227,8 @@ Source: [`packages/core/session/src/index.ts:427`](../../packages/core/session/s Registry of skill providers. It merges provider catalogs with stable first-wins duplicate handling, exposes sorted model-visible summaries, and loads full skill bodies on demand. ```ts cordis-catalog -registerProvider(provider: SkillProvider): () => void -register(skill: SkillRegistration): () => void +registerProvider(provider: SkillProvider): () => Promise | void +register(skill: SkillRegistration): () => Promise | void async list(options: SkillLookupOptions = {}): Promise async get(name: string, options: SkillLookupOptions = {}): Promise ``` diff --git a/packages/cordis/tool-cordis/src/api-catalog.ts b/packages/cordis/tool-cordis/src/api-catalog.ts index fe04ceb610..57390710f8 100644 --- a/packages/cordis/tool-cordis/src/api-catalog.ts +++ b/packages/cordis/tool-cordis/src/api-catalog.ts @@ -169,8 +169,8 @@ export const SERVICE_API: readonly ServiceApiEntry[] = [ key: 'skills', summary: 'Registry of skill providers.', methods: [ - 'registerProvider(provider: SkillProvider): () => void', - 'register(skill: SkillRegistration): () => void', + 'registerProvider(provider: SkillProvider): () => Promise | void', + 'register(skill: SkillRegistration): () => Promise | void', 'async list(options: SkillLookupOptions = {}): Promise', 'async get(name: string, options: SkillLookupOptions = {}): Promise', ], diff --git a/packages/core/tools/README.md b/packages/core/tools/README.md index d8a3ec08e5..2a678408e0 100644 --- a/packages/core/tools/README.md +++ b/packages/core/tools/README.md @@ -39,7 +39,7 @@ The live registry pipeline has three transformable waterfalls followed by the ow - `ToolExecutionToken` — a frozen, property-free identity value assigned by the registry. It supports equality correlation only and exposes no live outer execution state. - `ToolExecution` — the pipeline-owned call: immutable `{ token, callId, name, arguments, agent?, parent? }` identity plus optional operational `signal`, which an around wrapper may add, replace, remove, and restore. A nested call's `parent` is a `ToolExecutionToken`, not an execution object. - `ToolExecutionResult` — losslessly JSON-serializable outcome: `{ callId, content, isError, error?, additionalContext?, meta? }`. The registry validates the complete post-policy value before final observation. On failure with a `HarnessError`, `error: { name, code }` carries the structured failure class alongside the model-facing text (the loop forwards it onto the `tool/result` session event for retry/sandbox plugins and replay). `additionalContext` (a `HookContext`) ferries any `tools/post-execute` context up to the loop, which buffers it and appends it as a `context/message` after all `tool/result`s in the step. `meta` is the tool's opaque presentation payload from a successful `execute` (the object return form); the loop forwards it onto the `tool/result` session event for result-card rendering. -- `PreToolDecision` — `{kind:'allow'}` | `{kind:'deny', reason}` | `{kind:'ask', reason?}`. Input rewrite (changing `arguments`) is deliberately NOT offered (it would desync the pre-execution audit/history/UI from what ran — its own proposed RFC); `ask` is serviced by [`ctx.approval`](../../ui/user-approval/README.md) when a deployment mounts it (`allowed-once` proceeds to dispatch; `rejected`/`cancelled`/`unavailable` deny with distinct reasons) and degrades to `deny` when none is mounted or the execution carries no agent. +- `PreToolDecision` — `{kind:'allow'}` | `{kind:'deny', reason}` | `{kind:'ask', reason?}`. The registry validates this exact union at runtime: a JavaScript/casted value with an unknown kind, a malformed reason, or extra fields fails closed as an `isError`; the tool body does not run and final observers still receive one result. Input rewrite (changing `arguments`) is deliberately NOT offered (it would desync the pre-execution audit/history/UI from what ran — its own proposed RFC); `ask` is serviced by [`ctx.approval`](../../ui/user-approval/README.md) when a deployment mounts it (`allowed-once` proceeds to dispatch; `rejected`/`cancelled`/`unavailable` deny with distinct reasons) and degrades to `deny` when none is mounted or the execution carries no agent. - `PostToolDecision` — `{kind:'accept', content?, additionalContext?}` (keep the call successful, optionally replacing the model-facing content) | `{kind:'block', feedback, additionalContext?}` (turn it into an `isError` whose content is the corrective feedback). Output replacement is clean because `tool/result` is logged AFTER `execute()` returns. - `ToolGuard` — `(execution) => string | undefined`; the returned string is a final monotonic denial reason evaluated after the reorderable pre-execute waterfall and before dispatch. - `ToolCallView` / `ToolResultView` — provider-neutral `card`-tagged render intents a tool returns from `presentCall` / `presentResult` to own how a UI renders ITS calls (see "Tool-owned UI presentation"). diff --git a/packages/core/tools/src/index.ts b/packages/core/tools/src/index.ts index 66dab9e2bd..71dd465dd6 100644 --- a/packages/core/tools/src/index.ts +++ b/packages/core/tools/src/index.ts @@ -91,6 +91,9 @@ declare module 'cordis' { * tool body never runs. Input rewrite is deliberately NOT offered here (see * {@link PreToolDecision}); `ask` is serviced by the `ctx.approval` seam * when one is mounted, and degrades to deny otherwise. + * The returned union is validated as an exact runtime shape before approval + * or guards run; a malformed JavaScript/casted decision fails closed as an + * `isError` result and the tool body never runs. * Scope-filtered dispatch (`@deepseek-ai/dsh-scope`) keys the carrier by `exec.agent`: a * listener registered through `agent.ctx` fires only for that agent's * calls, while a plain plugin listener fires for every call (including @@ -908,6 +911,8 @@ export class ToolRegistry extends Service { * {@link HarnessError} surfaces its `{ name, code }` on the result. Before * the final observe-only notification, the authoritative outcome must survive * a lossless JSON round trip; an invalid outcome is normalized to an error. + * A malformed runtime/casted `tools/pre-execute` decision likewise normalizes + * to an error before approval, guards, or the tool body. * Caller-owned arguments must survive lossless-JSON validation before and * after cloning; a violation normalizes to an error before policy or dispatch. * @param exec - the single-use call input; its identity is snapshotted and @@ -1002,10 +1007,10 @@ export class ToolRegistry extends Service { // carrier keys dispatch by exec.agent, so an `agent.ctx` listener gates only // its own agent's calls (agent-less calls are subject-less). const carrier = scopeTarget(this, exec.agent) - const gate = await this.ctx.waterfall( + const gate = this.snapshotPreDecision(await this.ctx.waterfall( carrier, 'tools/pre-execute', exec, () => Promise.resolve({ kind: 'allow' }), - ) + )) const decision = gate.kind === 'ask' ? await this.serviceAsk(exec, gate) : gate const denialReason = decision.kind === 'allow' ? this.guardReason(exec) @@ -1055,6 +1060,41 @@ export class ToolRegistry extends Service { return await this.postExecute(exec, result) } + /** Validate and detach the extensible gate's decision before any grant can dispatch. */ + private snapshotPreDecision(value: unknown): PreToolDecision { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + throw new TypeError('tools/pre-execute must return a PreToolDecision object') + } + const decision = value as { kind?: unknown; reason?: unknown } + const keys = Reflect.ownKeys(decision) + const hasExactKeys = (...expected: string[]): boolean => + keys.length === expected.length && expected.every(key => Object.hasOwn(decision, key)) + switch (decision.kind) { + case 'allow': + if (!hasExactKeys('kind')) { + throw new TypeError('tools/pre-execute allow decision must contain only kind') + } + return { kind: 'allow' } + case 'deny': { + const reason = decision.reason + if (!hasExactKeys('kind', 'reason') || typeof reason !== 'string') { + throw new TypeError('tools/pre-execute deny decision must contain only kind and a string reason') + } + return { kind: 'deny', reason } + } + case 'ask': { + const reason = decision.reason + if (!(hasExactKeys('kind') || hasExactKeys('kind', 'reason')) + || (reason !== undefined && typeof reason !== 'string')) { + throw new TypeError('tools/pre-execute ask decision must contain only kind and an optional string reason') + } + return { kind: 'ask', ...reason !== undefined ? { reason } : {} } + } + default: + throw new TypeError('tools/pre-execute must return an allow, deny, or ask decision') + } + } + /** Notify final-result observers without giving them a mutation/error channel into the outcome. */ private async notifyResult(exec: ToolExecution, result: ToolExecutionResult): Promise { // The pipeline is over: freeze the remaining mutable signal slot so every diff --git a/packages/core/tools/tests/tools.spec.ts b/packages/core/tools/tests/tools.spec.ts index d1c454738d..deb53b7dc7 100644 --- a/packages/core/tools/tests/tools.spec.ts +++ b/packages/core/tools/tests/tools.spec.ts @@ -229,6 +229,66 @@ describe('ToolRegistry', () => { expect(result.content[0]).toMatchObject({ text: 'Error: denied by policy' }) }) + it.each([ + { + name: 'non-object decision', + replacement: null, + message: 'tools/pre-execute must return a PreToolDecision object', + }, + { + name: 'unknown decision kind', + replacement: { kind: 'permit' }, + message: 'tools/pre-execute must return an allow, deny, or ask decision', + }, + { + name: 'allow decision carrying extra fields', + replacement: { kind: 'allow', reason: 'smuggled' }, + message: 'tools/pre-execute allow decision must contain only kind', + }, + { + name: 'deny decision without a reason', + replacement: { kind: 'deny' }, + message: 'tools/pre-execute deny decision must contain only kind and a string reason', + }, + { + name: 'deny decision with a non-string reason', + replacement: { kind: 'deny', reason: 42 }, + message: 'tools/pre-execute deny decision must contain only kind and a string reason', + }, + { + name: 'ask decision with a non-string reason', + replacement: { kind: 'ask', reason: true }, + message: 'tools/pre-execute ask decision must contain only kind and an optional string reason', + }, + { + name: 'ask decision carrying extra fields', + replacement: { kind: 'ask', cache: true }, + message: 'tools/pre-execute ask decision must contain only kind and an optional string reason', + }, + ])('fails closed on a malformed tools/pre-execute $name', async ({ replacement, message }) => { + const ctx = await setup() + let bodyCalls = 0 + const observed: ToolExecutionResult[] = [] + ctx.tools.register({ + ...echoTool, + async execute() { + bodyCalls += 1 + return [] + }, + }) + ctx.on('tools/pre-execute', async () => replacement as unknown as PreToolDecision) + ctx.on('tools/result', (_exec, result) => { observed.push(result) }) + + const result = await ctx.tools.execute({ + callId: CallId('malformed-pre'), name: 'echo', arguments: {}, + }) + + expect(result.isError).toBe(true) + expect(result.content[0]).toMatchObject({ text: `Error: ${message}` }) + expect(bodyCalls).toBe(0) + expect(observed).toEqual([result]) + }) + it('rejects a JavaScript guard that returns an async/non-string decision', async () => { const ctx = await setup() let bodyCalls = 0 diff --git a/packages/skill/skill/README.md b/packages/skill/skill/README.md index 98f787dbc7..33d16e1b69 100644 --- a/packages/skill/skill/README.md +++ b/packages/skill/skill/README.md @@ -8,10 +8,10 @@ This package owns the `ctx.skills` interface. It does not know whether skills co ### Public API -- `ctx.skills.registerProvider(provider): () => void` Registers a provider by unique `provider.name`. Duplicate provider names throw, and `runtime` is reserved for `ctx.skills.register(...)`. The registration is effect-scoped and HMR-safe. +- `ctx.skills.registerProvider(provider): () => Promise | void` Registers a provider by unique `provider.name`. Duplicate provider names throw, and `runtime` is reserved for `ctx.skills.register(...)`. The registry snapshots the name and callback identities at registration, so replacing those fields later cannot change lookup or HMR cleanup; callbacks remain bound to the original provider object and can still read its mutable state. The registration is effect-scoped and HMR-safe, and the exact Cordis disposer supports ordered composite teardown. - `ctx.skills.list({ cwd?, signal? })` Returns model-invocable skill summaries for the current workspace, merged across providers and sorted by name. - `ctx.skills.get(name, { cwd?, signal? })` Returns the full winning skill, including disabled-for-model skills. -- `ctx.skills.register(skill): () => void` Registers a runtime embedded skill. Same-name runtime registrations are first-wins: a duplicate logs a warning and gets a no-op disposer. +- `ctx.skills.register(skill): () => Promise | void` Registers a runtime embedded skill. Same-name runtime registrations are first-wins: a duplicate logs a warning and gets a no-op disposer. Successful registrations return the exact Cordis disposer for ordered composite teardown. ### Config @@ -21,7 +21,7 @@ This package owns the `ctx.skills` interface. It does not know whether skills co ## Provider Contract -A provider registers synchronously from its `apply()` and returns `SkillCandidate[]` from `list(options)` when discovery is requested. Remote setup, authentication, and discovery belong in the awaited `list()` call rather than plugin registration. Providers should stop promptly when `options.signal` aborts; the registry also stops awaiting an uncooperative provider so agent cancellation cannot hang prefix composition. The provider later receives the winning candidate back in `get(candidate, options)`. The candidate's `locator` is opaque to the registry, so a local provider can store a file path while a remote provider can store a URL, id, or version token. +A provider registers synchronously from its `apply()` and returns `SkillCandidate[]` from `list(options)` when discovery is requested. Registration copies `name` and binds the current `list` and `get` methods once; replacing those fields on the caller-owned object later does not rewrite the live registry entry, and disposal always removes the original name. Remote setup, authentication, and discovery belong in the awaited `list()` call rather than plugin registration. Providers should stop promptly when `options.signal` aborts; the registry also stops awaiting an uncooperative provider so agent cancellation cannot hang prefix composition. The provider later receives the winning candidate back in `get(candidate, options)`. The candidate's `locator` is opaque to the registry, so a local provider can store a file path while a remote provider can store a URL, id, or version token. The registry validates candidate names, descriptions, ranks, and provider ownership. Candidate contract violations fail fast because the provider plugin is malformed; a provider `list()` rejection is treated as a transient source failure, logged, skipped for that request, and not cached. Only completed catalogs are cached, and a provider/runtime revision change during discovery discards the stale result and retries. Duplicate skill names are resolved first-wins by `rank`, provider registration order, then the provider's own local order. The final summary list is sorted by skill `name` for deterministic consumers. diff --git a/packages/skill/skill/src/index.ts b/packages/skill/skill/src/index.ts index 50a8b5d0d0..20ac717e9c 100644 --- a/packages/skill/skill/src/index.ts +++ b/packages/skill/skill/src/index.ts @@ -177,31 +177,46 @@ export class SkillService extends Service { * Register a skill provider synchronously during the provider plugin's * `apply()`. Throws if another provider already owns the same provider name, * including the reserved runtime provider name. Providers that need remote - * initialization do that work inside `list()` after registration. Effect- - * scoped and HMR-safe: disposing the caller's fiber unregisters the provider - * and invalidates cached catalogs. + * initialization do that work inside `list()` after registration. The name + * and callback identities are snapshotted at registration, so later + * replacement of those fields cannot change the registry key, dispatch + * callbacks, or HMR cleanup identity. Bound callbacks retain the original + * provider object as their receiver, so provider-owned mutable state remains + * live. Effect-scoped and HMR-safe: disposing the caller's fiber unregisters + * the provider and invalidates cached catalogs. * @param provider - the provider to register by `provider.name`. - * @returns a disposer that unregisters this provider. + * @returns the exact Cordis effect disposer that unregisters this provider; + * composite effects may yield it directly to preserve teardown ordering. */ - registerProvider(provider: SkillProvider): () => void { + registerProvider(provider: SkillProvider): () => Promise | void { + // Snapshot the registration contract before entering the effect. The + // callback binding preserves the historical method receiver while making + // replacement of `provider.list`/`provider.get` after registration inert. + // In particular, cleanup must never re-read caller-owned `provider.name`: + // an HMR host may mutate or reuse that object before its old fiber unloads. + const snapshot: SkillProvider = Object.freeze({ + name: provider.name, + list: provider.list.bind(provider), + get: provider.get.bind(provider), + }) const dispose = this.ctx.effect(function* (this: SkillService) { - if (provider.name === RUNTIME_PROVIDER) { + if (snapshot.name === RUNTIME_PROVIDER) { throw new Error(`"${RUNTIME_PROVIDER}" is reserved for runtime skill registrations`) } - if (this.providers.has(provider.name)) { - throw new Error(`a skill provider named "${provider.name}" is already registered`) + if (this.providers.has(snapshot.name)) { + throw new Error(`a skill provider named "${snapshot.name}" is already registered`) } - this.providers.set(provider.name, { provider, order: this.nextProviderOrder }) + this.providers.set(snapshot.name, { provider: snapshot, order: this.nextProviderOrder }) this.nextProviderOrder += 1 this.invalidateCache() yield () => { - this.providers.delete(provider.name) + this.providers.delete(snapshot.name) this.invalidateCache() - this.ctx.emit('skill/provider-removed', provider.name) + this.ctx.emit('skill/provider-removed', snapshot.name) } - this.ctx.emit('skill/provider-added', provider) + this.ctx.emit('skill/provider-added', snapshot) }.bind(this), 'skills.registerProvider()') - return () => void dispose() + return dispose } /** @@ -210,9 +225,11 @@ export class SkillService extends Service { * registrations are first-wins: a duplicate logs a warning and gets a no-op * disposer so it cannot remove the active contribution. * @param skill - the complete skill definition to expose for discovery. - * @returns a disposer that removes this runtime contribution and invalidates caches. + * @returns the exact Cordis effect disposer that removes this runtime + * contribution and invalidates caches; composite effects may yield it + * directly to preserve teardown ordering. */ - register(skill: SkillRegistration): () => void { + register(skill: SkillRegistration): () => Promise | void { const normalized = normalizeRuntimeSkill(skill) const existing = this.runtime.get(normalized.name) if (existing !== undefined) { @@ -229,7 +246,7 @@ export class SkillService extends Service { this.invalidateCache() } }.bind(this), 'skills.register()') - return () => void dispose() + return dispose } /** diff --git a/packages/skill/skill/tests/skill.spec.ts b/packages/skill/skill/tests/skill.spec.ts index 010d161039..e3b491f690 100644 --- a/packages/skill/skill/tests/skill.spec.ts +++ b/packages/skill/skill/tests/skill.spec.ts @@ -103,10 +103,68 @@ describe('SkillService registry', () => { }, })).toThrow('reserved') - disposeMemory() + await disposeMemory() expect((await ctx.skills.list()).map(skill => skill.name)).toEqual(['same-rank-skill', 'shadowed']) }) + it('snapshots a provider registration so caller mutation cannot corrupt HMR cleanup', async () => { + const ctx = new Context() + await ctx.plugin(SkillService) + const candidate: SkillCandidate = { + name: 'stable-skill', + description: 'Stable skill', + provider: 'stable-provider', + source: 'test', + rank: 1, + locator: 'original', + } + const originalList = vi.fn(() => Promise.resolve([candidate])) + const originalGet = vi.fn((listed: SkillCandidate) => Promise.resolve({ + ...listed, + content: 'Original body.', + })) + const provider: SkillProvider = { + name: 'stable-provider', + list: originalList, + get: originalGet, + } + const added: SkillProvider[] = [] + const removed: string[] = [] + ctx.on('skill/provider-added', (registered) => { added.push(registered) }) + ctx.on('skill/provider-removed', (name) => { removed.push(name) }) + const owner = await ctx.plugin({ + name: 'mutable-provider-owner', + inject: ['skills'], + apply(pluginCtx: Context) { + pluginCtx.skills.registerProvider(provider) + }, + }) + + provider.name = 'mutated-provider' + const replacementList = vi.fn(() => Promise.resolve([])) + const replacementGet = vi.fn(() => Promise.resolve(undefined)) + provider.list = replacementList + provider.get = replacementGet + + expect(added).toHaveLength(1) + expect(added[0]).not.toBe(provider) + expect(added[0]?.name).toBe('stable-provider') + expect(Object.isFrozen(added[0])).toBe(true) + expect((await ctx.skills.list()).map(skill => skill.name)).toEqual(['stable-skill']) + expect((await ctx.skills.get('stable-skill'))?.content).toBe('Original body.') + expect(originalList).toHaveBeenCalledOnce() + expect(originalGet).toHaveBeenCalledOnce() + expect(replacementList).not.toHaveBeenCalled() + expect(replacementGet).not.toHaveBeenCalled() + + await owner.dispose() + expect(removed).toEqual(['stable-provider']) + expect(await ctx.skills.list()).toEqual([]) + const replacement = new MemoryProvider([]) + Object.defineProperty(replacement, 'name', { value: 'stable-provider' }) + expect(() => ctx.skills.registerProvider(replacement)).not.toThrow() + }) + it('validates provider candidates and invalid registry caps', async () => { const defaultedService = new SkillService(new Context()) expect(await defaultedService.list()).toEqual([]) @@ -196,7 +254,7 @@ describe('SkillService registry', () => { path: 'memory://runtime-skill', metadata: { owner: 'tests' }, }) - disposeRuntime() + await disposeRuntime() await ctx.skills.list({ cwd: '/tmp/first-cache-key' }) await ctx.skills.list({ cwd: '/tmp/second-cache-key' }) @@ -245,7 +303,7 @@ describe('SkillService registry', () => { const pending = ctx.skills.list() await started - dispose() + await dispose() release?.() expect(await pending).toEqual([]) @@ -336,9 +394,9 @@ describe('SkillService registry', () => { const disposeFirst = ctx.skills.register({ name: 'same-skill', description: 'First', source: 'runtime', content: 'first' }) const disposeSecond = ctx.skills.register({ name: 'same-skill', description: 'Second', source: 'runtime', content: 'second' }) - disposeSecond() + await disposeSecond() expect((await ctx.skills.get('same-skill'))?.description).toBe('First') - disposeFirst() + await disposeFirst() expect(await ctx.skills.get('same-skill')).toBeUndefined() }) }) diff --git a/packages/skill/tool-skill/README.md b/packages/skill/tool-skill/README.md index c55bc3693d..7e72f89000 100644 --- a/packages/skill/tool-skill/README.md +++ b/packages/skill/tool-skill/README.md @@ -6,7 +6,7 @@ Requires `ctx.tools` and `ctx.skills` (`inject: ['tools', 'skills']`). ## Session-prefix catalog -The plugin contributes one user-role `` catalog through `agent/session-prefix`. It resolves skills for the calling session's cwd, forwards the prefix abort signal to discovery, and lists only sorted `name` and `description` entries; skill bodies, paths, sources, providers, and `whenToUse` hints remain outside the catalog. The catalog is omitted when no model-invocable skills are available. +The plugin contributes one user-role `` catalog through `agent/session-prefix`. It resolves skills for the calling session's cwd, forwards the prefix abort signal to discovery, and lists only sorted `name` and `description` entries; skill bodies, paths, sources, providers, and `whenToUse` hints remain outside the catalog. The catalog is omitted when no model-invocable skills are available, and also when that agent's tool view restricts away the shipped `skill` tool or resolves a same-name scoped shadow instead. This exact-definition check keeps prompt guidance, the model-visible schema, and executable dispatch aligned. `catalogDescriptionMaxLength` controls normalized, XML-escaped catalog descriptions. Its default is `500` and values must be integers of at least `3`, which reserves room for a truncation ellipsis. The [session-prefix RFC](../../../docs/rfc/implemented/feature/2026-07-07-session-prefix.md) defines the request-only, header-logged lifecycle of this message. diff --git a/packages/skill/tool-skill/package.json b/packages/skill/tool-skill/package.json index f0d97eb3d8..bf7a77a49e 100644 --- a/packages/skill/tool-skill/package.json +++ b/packages/skill/tool-skill/package.json @@ -34,6 +34,7 @@ "devDependencies": { "@deepseek-ai/dsh-agent": "workspace:^", "@deepseek-ai/dsh-llm": "workspace:^", + "@deepseek-ai/dsh-scope": "workspace:^", "@deepseek-ai/dsh-skill": "workspace:^", "@deepseek-ai/dsh-skill-local": "workspace:^", "@deepseek-ai/dsh-tools": "workspace:^", diff --git a/packages/skill/tool-skill/src/index.ts b/packages/skill/tool-skill/src/index.ts index dc3232a13f..39fa0253b4 100644 --- a/packages/skill/tool-skill/src/index.ts +++ b/packages/skill/tool-skill/src/index.ts @@ -26,18 +26,16 @@ export const Config: z = z.object({ catalogDescriptionMaxLength: z.number().default(DEFAULT_CATALOG_DESCRIPTION_MAX_LENGTH), }) -/** Register the session-prefix skill catalog and the model-facing skill loader. */ +/** + * Register the model-facing skill loader and its visibility-matched + * session-prefix catalog. The catalog is emitted only when the calling agent + * resolves this plugin's exact tool registration; a restriction or scoped + * same-name shadow therefore removes both the schema and its call guidance. + */ export function apply(ctx: Context, config: Config = {}): void { const catalogDescriptionMaxLength = config.catalogDescriptionMaxLength ?? DEFAULT_CATALOG_DESCRIPTION_MAX_LENGTH assertPositiveInteger('catalogDescriptionMaxLength', catalogDescriptionMaxLength, 3) - ctx.on('agent/session-prefix', async (agent, _prefix, signal, next): Promise => { - const skills = await ctx.skills.list({ cwd: agent.session.header.cwd, signal }) - const rest = await next() - if (skills.length === 0) return rest - return [renderCatalogMessage(skills, catalogDescriptionMaxLength), ...rest] - }) - const skillTool = defineTool({ name: 'skill', description: 'Load the full instructions for an available skill. Call this with the exact skill name from the session skill catalog before acting on a task that names or clearly matches that skill.', @@ -62,6 +60,23 @@ export function apply(ctx: Context, config: Config = {}): void { }, }) ctx.tools.register(skillTool) + const registeredSkillTool = ctx.tools.get(skillTool.name) + /* v8 ignore next 3 -- register() publishes synchronously or throws; this guards future registry drift. */ + if (registeredSkillTool === undefined) { + throw new Error('dsh-tool-skill: registered skill tool is not visible in the global registry') + } + + // Register after the tool so reverse-order fiber teardown removes this + // guidance listener before its referenced tool. Exact definition identity is + // the shared truth for restrictions and scoped shadows: another tool merely + // named `skill` must not inherit this plugin's catalog or instructions. + ctx.on('agent/session-prefix', async (agent, _prefix, signal, next): Promise => { + if (ctx.tools.get(skillTool.name, agent) !== registeredSkillTool) return await next() + const skills = await ctx.skills.list({ cwd: agent.session.header.cwd, signal }) + const rest = await next() + if (skills.length === 0) return rest + return [renderCatalogMessage(skills, catalogDescriptionMaxLength), ...rest] + }) } function renderSkillContent(skill: SkillDefinition): string { diff --git a/packages/skill/tool-skill/tests/tool-skill.spec.ts b/packages/skill/tool-skill/tests/tool-skill.spec.ts index ccfd795144..cab7f4aa02 100644 --- a/packages/skill/tool-skill/tests/tool-skill.spec.ts +++ b/packages/skill/tool-skill/tests/tool-skill.spec.ts @@ -4,8 +4,9 @@ import { join } from 'node:path' import { tmpdir } from 'node:os' import { Context } from 'cordis' import { CallId, type Message } from '@deepseek-ai/dsh-llm' +import { createScope, type Scope } from '@deepseek-ai/dsh-scope' import SystemPrompt, { renderPrompt } from '@deepseek-ai/dsh-system-prompt' -import ToolRegistry from '@deepseek-ai/dsh-tools' +import ToolRegistry, { defineTool } from '@deepseek-ai/dsh-tools' import { agentEvents, type Agent } from '@deepseek-ai/dsh-agent' import SkillService from '@deepseek-ai/dsh-skill' import * as SkillLocal from '@deepseek-ai/dsh-skill-local' @@ -36,7 +37,10 @@ function agentForCwd(cwd: string): Agent { } async function composePrefix(ctx: Context, cwd: string, signal = new AbortController().signal): Promise { - const agent = agentForCwd(cwd) + return await composePrefixForAgent(ctx, agentForCwd(cwd), signal) +} + +async function composePrefixForAgent(ctx: Context, agent: Agent, signal = new AbortController().signal): Promise { const empty: Message[] = [] return await agentEvents(ctx, agent).waterfall( 'agent/session-prefix', empty, signal, @@ -44,6 +48,15 @@ async function composePrefix(ctx: Context, cwd: string, signal = new AbortContro ) } +async function mintAgentScope(ctx: Context, cwd: string): Promise<{ agent: Agent; scope: Scope }> { + const agent = agentForCwd(cwd) + let scope!: Scope + await ctx.plugin(Object.assign((inner: Context) => { scope = createScope(inner, agent) }, { + inject: ['tools'], + })) + return { agent, scope } +} + describe('dsh-tool-skill', () => { it('registers the skill tool schema and removes it on dispose', async () => { const ctx = new Context() @@ -154,6 +167,39 @@ describe('dsh-tool-skill', () => { expect(await composePrefix(ctx, '/workspace')).toEqual([]) }) + it('omits catalog guidance when the calling agent restricts away the shipped skill tool', async () => { + const home = await tempDir('tool-restricted-catalog') + const ctx = await setup(home) + ctx.skills.register({ name: 'listed-skill', description: 'Listed', source: 'runtime', content: 'body' }) + const { agent, scope } = await mintAgentScope(ctx, '/workspace') + scope.ctx.tools.restrict({ deny: ['skill'] }) + + expect(ctx.tools.get('skill', agent)).toBeUndefined() + expect(await composePrefixForAgent(ctx, agent)).toEqual([]) + expect(await composePrefix(ctx, '/workspace')).toHaveLength(1) + await scope.dispose() + }) + + it('does not attach shipped catalog guidance to a scoped same-name tool shadow', async () => { + const home = await tempDir('tool-shadowed-catalog') + const ctx = await setup(home) + ctx.skills.register({ name: 'listed-skill', description: 'Listed', source: 'runtime', content: 'body' }) + const { agent, scope } = await mintAgentScope(ctx, '/workspace') + scope.ctx.tools.register(defineTool({ + name: 'skill', + description: 'A scoped tool with unrelated semantics.', + parameters: {}, + execute() { + return Promise.resolve([{ type: 'text', text: 'shadow' }]) + }, + })) + + expect(ctx.tools.get('skill', agent)).not.toBe(ctx.tools.get('skill')) + expect(await composePrefixForAgent(ctx, agent)).toEqual([]) + expect(await composePrefix(ctx, '/workspace')).toHaveLength(1) + await scope.dispose() + }) + it('validates the catalog description cap', async () => { const home = await tempDir('tool-invalid-catalog-cap') const ctx = new Context() diff --git a/packages/skill/tool-skill/tsconfig.json b/packages/skill/tool-skill/tsconfig.json index 039fc641c0..52ebb8bf9d 100644 --- a/packages/skill/tool-skill/tsconfig.json +++ b/packages/skill/tool-skill/tsconfig.json @@ -9,6 +9,7 @@ { "path": "../../../vendor/cosmokit" }, { "path": "../../../vendor/cordis" }, { "path": "../../../vendor/schemastery" }, + { "path": "../../core/scope" }, { "path": "../../llm/llm" }, { "path": "../../core/agent" }, { "path": "../skill" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index aace53a37e..68ec4e3183 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -822,6 +822,9 @@ importers: '@deepseek-ai/dsh-llm': specifier: workspace:^ version: link:../../llm/llm + '@deepseek-ai/dsh-scope': + specifier: workspace:^ + version: link:../../core/scope '@deepseek-ai/dsh-skill': specifier: workspace:^ version: link:../skill