fix(scope): harden merged tool and skill boundaries

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> | 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> | 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.
 

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> | 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> | 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
   }
 
   /**

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<SkillDefinition>({
+      ...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()
   })
 })