From 22619e274850302a23cffbc7089ccd215014124b Mon Sep 17 00:00:00 2001 From: zfy0701 <1646270+zfy0701@users.noreply.github.com> Date: Tue, 6 Oct 2026 08:32:57 +0800 Subject: [PATCH] fix(daemon): remove a placed session's skills left at its session directory Since #2801 a placed session installs its skills into its checkout under a ledger of their own. Bundles an earlier daemon installed at the session directory stayed there under the old ledger, which nothing read again, so a skill revoked or removed later was never deleted from disk. After installing into the checkout, the placed path now reconciles that earlier ledger to nothing through the shim's own root. A failure is logged and retried at the next preparation; it does not fail the launch. Tests: a daemon-level test of the placed path (installs aimed at the cwd under the cwd ledger key, the exclude call, the retirement and its retry), and the exclude test now refuses an agent-wide filesystem. Co-Authored-By: Claude Opus 5.5 --- packages/daemon/src/daemon.ts | 47 ++++++++++- packages/daemon/src/shim/skill-client.ts | 3 +- packages/daemon/test/daemon-lifecycle.test.ts | 80 +++++++++++++++++++ packages/daemon/test/executor-plane.test.ts | 3 +- packages/daemon/test/git-exclude.test.ts | 9 ++- 5 files changed, 134 insertions(+), 8 deletions(-) diff --git a/packages/daemon/src/daemon.ts b/packages/daemon/src/daemon.ts index 9a3bd493e0..921715119f 100644 --- a/packages/daemon/src/daemon.ts +++ b/packages/daemon/src/daemon.ts @@ -6229,14 +6229,55 @@ export class Daemon { ): Promise { const reported = plane?.workspaceIncarnationFor?.(pod) const shimGeneration = plane?.shimGenerationFor?.(pod) - return await this.reconcileSandboxSkills(agent, { + const isLaunchCurrent = () => + plane?.workspaceIncarnationFor?.(pod) === reported && plane?.shimGenerationFor?.(pod) === shimGeneration + const ledger = await this.reconcileSandboxSkills(agent, { client: plane?.skillClientFor?.(pod, cwd), // Receipts are relative to the directory installed into, so installs into a cwd keep a ledger of their own. workspaceIncarnation: reported && cwd ? cwdWorkspaceIncarnation(reported, cwd) : reported, shimGeneration, - isLaunchCurrent: () => - plane?.workspaceIncarnationFor?.(pod) === reported && plane?.shimGenerationFor?.(pod) === shimGeneration + isLaunchCurrent }) + if (cwd !== undefined && reported && shimGeneration !== undefined) { + await this.retireShimRootSkills(agent, plane?.skillClientFor?.(pod), reported, shimGeneration, isLaunchCurrent) + } + return ledger + } + + /** Remove the bundles an older daemon installed at a placed session's directory, above its checkout: no runtime scans there, and no other ledger owns them. */ + private async retireShimRootSkills( + agent: Agent, + client: ClusterSkillClient | undefined, + workspaceIncarnation: string, + shimGeneration: number, + isLaunchCurrent: () => boolean + ): Promise { + const prior = await this.store.clusterSkillLedger(agent.id, workspaceIncarnation) + if (!prior?.ledger.roots.length) return + const duty = this.duties.dutyForAgent(agent.id) + const skillsAgentId = this.runtimeCatalog.entries[agent.runtime]?.skillsAgentId + if (!client || !duty || !skillsAgentId || !this.cfg.daemonId) return + try { + await new ClusterSkillCoordinator(this.store).reconcile({ + authority: { + groupId: duty.groupId, + term: duty.term, + daemonId: this.cfg.daemonId, + agentId: agent.id, + workspaceIncarnation + }, + skillsAgentId, + shimGeneration, + sources: [], + client, + isLaunchCurrent + }) + } catch (error) { + // Retried at the next preparation: the ledger still holds them. + this.log.warn( + `skills: could not remove ${agent.id}'s bundles at its session directory (${(error as Error).message})` + ) + } } private async reconcileMicrosandboxSkills(agent: Agent, cwd: string): Promise { diff --git a/packages/daemon/src/shim/skill-client.ts b/packages/daemon/src/shim/skill-client.ts index 127657ab80..eb81321500 100644 --- a/packages/daemon/src/shim/skill-client.ts +++ b/packages/daemon/src/shim/skill-client.ts @@ -30,8 +30,7 @@ import { type ClusterSkillVerifyReply } from './skill-protocol.js' -/** A shim's skills seam aimed at `cwd` rather than the shim's own workspace root: the runtimes scan for project skills - * from their cwd up to its repository root only, so bundles beside a checkout rather than in it are never found. */ +/** A shim's skills seam aimed at `cwd`, not its own root: runtimes scan for project skills only from cwd up to the repository root. */ export function cwdSkillRequester(session: ShimRequester, cwd: string): ShimRequester { return { request: (capability, request, options) => session.request(capability, { cwd, request }, options) } } diff --git a/packages/daemon/test/daemon-lifecycle.test.ts b/packages/daemon/test/daemon-lifecycle.test.ts index 79eb8dec6d..ef18a36812 100644 --- a/packages/daemon/test/daemon-lifecycle.test.ts +++ b/packages/daemon/test/daemon-lifecycle.test.ts @@ -11,7 +11,9 @@ import { buildCpClientDeps } from '../src/cp/cp-client-deps.js' import { ExecutorPlane } from '../src/execution/executor-plane.js' import { TaskViolationError } from '../src/cp/task-reader.js' import { configFilesDir } from '../src/shim/config-file-env.js' +import { ClusterSkillCoordinator } from '../src/skills/cluster-skill-coordinator.js' import { readSkillLedger, skillLedgerLocation } from '../src/skills/skill-install-ledger.js' +import { cwdWorkspaceIncarnation } from '../src/skills/workspace-incarnation.js' import { sessionKey, transcriptChannelKey } from '../src/store/local-store.js' import { NO_RESPONSE_SENTINEL } from '../src/session/no-response.js' import { localWorkspaceFiles } from '../src/workspace/workspace-files.js' @@ -626,6 +628,84 @@ describe('Daemon session lifecycle (#118)', () => { } }) + it("installs a placed session's skills into its checkout and retires those at its session directory", async () => { + const daemon = new Daemon({ root: scaffold(), hostFactory: () => quietHost() as never }) + try { + await daemon.start() + const d = daemon as any + const agent = d.agents.get('bot-a') + d.runtimeCatalog.entries.claude = { ...d.runtimeCatalog.entries.claude, skillsAgentId: 'claude' } + const cwd = '/srv/executor/sessions/leaf/workspace' + const reported = 'workspace:reported' + const atCwd = cwdWorkspaceIncarnation(reported, cwd) + const root = (path: string) => ({ + path, + sourceId: 'managed:a', + sourceKind: 'managed' as const, + digest: 'a'.repeat(64), + files: [] + }) + const clients = new Map([ + [undefined, { manifestLimits: { maxFiles: 1, maxTotalBytes: 1 } }], + [cwd, { manifestLimits: { maxFiles: 1, maxTotalBytes: 1 } }] + ]) + d.executorPlane = { + placementOf: (key: string) => + key === KEY ? { agentId: 'bot-a', sessionKey: KEY, subject: 'bot-a/leaf' } : undefined, + withEnvironment: (_subject: string, work: () => Promise) => work(), + mountFor: () => '/srv/executor', + skillClientFor: (_subject: string, at?: string) => clients.get(at), + workspaceIncarnationFor: () => reported, + shimGenerationFor: () => 1, + planeFor: () => undefined, + launched: () => [], + releaseAgent: () => {}, + stop: async () => {} + } + vi.spyOn(d.workspaces, 'prepareExecutorWorkspace').mockResolvedValue(cwd) + const exclude = vi.spyOn(d.workspaces, 'excludePlacedSessionSkills').mockResolvedValue(undefined) + vi.spyOn(d.duties, 'dutyForAgent').mockReturnValue({ groupId: 'group-a', term: '1' }) + // An older daemon installed at the shim's root, the session directory; the checkout holds nothing yet. + const ledgers = new Map([[reported, { revision: 1, ledger: { roots: [root('.claude/skills/a')] } }]]) + vi.spyOn(d.store, 'clusterSkillLedger').mockImplementation((async (_id: string, key: string) => + ledgers.get(key)) as never) + let retirementFails = true + const reconcile = vi.spyOn(ClusterSkillCoordinator.prototype, 'reconcile').mockImplementation(async (input) => { + const at = input.authority.workspaceIncarnation + if (at === reported && retirementFails) { + retirementFails = false + throw new Error('shim busy') + } + const roots = at === atCwd ? [root('.claude/skills/a')] : [] + ledgers.set(at, { revision: 2, ledger: { roots } }) + return { roots } + }) + const request = { sessionKey: KEY, isolation: 'session' } + + // A failed retirement leaves the launch alone; the next preparation retries it. + expect(await d.runAgentWorkspacePreparation(agent, request)).toBe(cwd) + expect(reconcile.mock.calls.map(([input]) => [input.authority.workspaceIncarnation, input.client])).toEqual([ + [atCwd, clients.get(cwd)], + [reported, clients.get(undefined)] + ]) + expect(reconcile.mock.calls[1]![0].sources).toEqual([]) + expect(exclude).toHaveBeenCalledWith(agent, cwd, ['.claude/skills/a', '.agentconnect/cluster-skill-state']) + + reconcile.mockClear() + await d.runAgentWorkspacePreparation(agent, request) + expect(reconcile.mock.calls.map(([input]) => input.authority.workspaceIncarnation)).toEqual([atCwd, reported]) + expect(ledgers.get(reported)?.ledger.roots).toEqual([]) + + // Nothing is left there, so later preparations ask nothing of the session directory. + reconcile.mockClear() + await d.runAgentWorkspacePreparation(agent, request) + expect(reconcile.mock.calls.map(([input]) => input.authority.workspaceIncarnation)).toEqual([atCwd]) + } finally { + vi.restoreAllMocks() + await daemon.stop() + } + }) + it.each([true, false])('installs a host runtime only for local execution (runInSandbox=%s)', async (runInSandbox) => { const root = scaffold() const host = quietHost() diff --git a/packages/daemon/test/executor-plane.test.ts b/packages/daemon/test/executor-plane.test.ts index da0ca45303..755099f604 100644 --- a/packages/daemon/test/executor-plane.test.ts +++ b/packages/daemon/test/executor-plane.test.ts @@ -254,8 +254,7 @@ describe('the skills a placed session installs', () => { return { executor, request } } - // The shim is rooted at the session directory, above the checkout the runtime scans for project skills (Codex - // stops at the repository root), so installs are aimed at the runtime's cwd. + // The shim is rooted at the session directory, above the checkout where runtimes scan for project skills. it('aims every request at the cwd it is given', async () => { const { executor, request } = bound() await executor.skillClientFor(SUBJECT, CWD)!.verify([]) diff --git a/packages/daemon/test/git-exclude.test.ts b/packages/daemon/test/git-exclude.test.ts index e4b1bd3a57..131fbd7f3a 100644 --- a/packages/daemon/test/git-exclude.test.ts +++ b/packages/daemon/test/git-exclude.test.ts @@ -248,11 +248,18 @@ describe('the workspace manager recording the bundles its skills step installs', mkdir: async () => {}, writeFile: async (path: string, content: string) => void written.push({ path, content }) } + const unscoped = { + readFileBytes: async () => { + throw new Error('asked without the path that names the session') + } + } const workspaces = new WorkspaceManager() wireTestPlane(workspaces, { workspacesOffDisk: true, gitRunnerFor: () => runner, - workspaceFsFor: () => ({ fs, mount: '/srv/executor' }) as never + // A placed session's filesystem is found by its path; an agent-wide question would reach the holder's own disk. + workspaceFsFor: (_agentId, scope) => + ({ fs: scope?.path === cwd ? fs : unscoped, mount: '/srv/executor' }) as never }) await workspaces.excludePlacedSessionSkills(agent, cwd, [BUNDLE, '.agentconnect/cluster-skill-state']) // The exclude file is the checkout's own, on the executor: nothing is written on this disk.