From d030f3ff793a958c7cf4d4173a3d9ae6d23c8dc2 Mon Sep 17 00:00:00 2001 From: zfy0701 <1646270+zfy0701@users.noreply.github.com> Date: Tue, 6 Oct 2026 08:18:39 +0800 Subject: [PATCH] fix(daemon): skip a locally edited installed skill instead of failing every turn An agent that edited one of its own installed skills wedged the workspace: the next reinstall planned a replacement of the edited bundle, the confined helper refused it, and recovery then failed verifying the same bundle, so the ledger stayed in `applying` and every later turn hit the same error. Planning now checks each recorded bundle against its receipt. One that no longer matches is given up and left in place: skipped and reported as a conflict like a foreign bundle when still desired, left alone rather than removed when not. Recovery gives up a prior edited where it stands, with no quarantine or tombstone of its operation beside it, instead of discarding or restoring it, so a workspace already stuck in `applying` recovers on its own. Refs #2802 Co-Authored-By: Claude Opus 5.5 --- .../daemon/src/skills/skill-install-ledger.ts | 47 +++++- .../daemon/test/skill-install-ledger.test.ts | 151 ++++++++++++++++-- packages/daemon/test/unified-skills.test.ts | 15 +- 3 files changed, 185 insertions(+), 28 deletions(-) diff --git a/packages/daemon/src/skills/skill-install-ledger.ts b/packages/daemon/src/skills/skill-install-ledger.ts index 2fc5fc85e..6abdfcbf9 100644 --- a/packages/daemon/src/skills/skill-install-ledger.ts +++ b/packages/daemon/src/skills/skill-install-ledger.ts @@ -294,8 +294,14 @@ export async function recoverSkillLedger( const priorByPath = new Map(ledger.prior.map((entry) => [entry.relativeRoot, entry])) const vanished = new Set() + const operationRoots = new Set(ledger.operations.map((operation) => operation.relativeRoot)) for (const operation of [...ledger.operations].reverse()) { const prior = priorByPath.get(operation.relativeRoot) + // Edited where it stands and never moved, so nothing of ours is there to discard or restore: give it up and leave it. + if (prior && (await editedInPlace(cwd, prior, operation))) { + vanished.add(prior.relativeRoot) + continue + } await mutate( { action: 'discard', @@ -337,9 +343,11 @@ export async function recoverSkillLedger( ) } } - const operationRoots = new Set(ledger.operations.map((operation) => operation.relativeRoot)) for (const prior of ledger.prior) { - if (!operationRoots.has(prior.relativeRoot) && !(await destinationOccupied(cwd, prior.relativeRoot))) { + if ( + !operationRoots.has(prior.relativeRoot) && + (!(await destinationOccupied(cwd, prior.relativeRoot)) || (await editedInPlace(cwd, prior))) + ) { vanished.add(prior.relativeRoot) } } @@ -410,6 +418,17 @@ async function destinationOccupied(cwd: string, relativeRoot: string): Promise { + if (operation) { + const parent = operation.relativeRoot.split('/').slice(0, -1).join('/') + for (const name of [operation.quarantineName, operation.tombstoneName]) { + if (await destinationOccupied(cwd, `${parent}/${name}`)) return false + } + } + return (await destinationOccupied(cwd, prior.relativeRoot)) && !(await installedBundlesIntact(cwd, [prior])) +} + /** Whether an operation's prior bundle is at neither of the two addresses recovery can find it at: its own path, or the quarantine a mutation moves it to. */ async function priorHasNoAddress(cwd: string, operation: JournalOperation): Promise { const parent = operation.relativeRoot.split('/').slice(0, -1).join('/') @@ -483,9 +502,17 @@ async function reconcileSkillBundlesLocked( const recorded = await canonicalizeOwned(options.cwd, ledger?.owned ?? []) // An absent recorded bundle is not stale executable content — nothing to quarantine, nothing foreign to protect — so plan its path as a first install. const prior: OwnedSkillBundle[] = [] + const modified = new Set() for (const entry of recorded) { - if (await destinationOccupied(options.cwd, entry.relativeRoot)) prior.push(entry) - else options.warn?.(`skills: recorded bundle ${entry.relativeRoot} is gone from the workspace; reinstalling`) + if (!(await destinationOccupied(options.cwd, entry.relativeRoot))) { + options.warn?.(`skills: recorded bundle ${entry.relativeRoot} is gone from the workspace; reinstalling`) + } else if (await installedBundlesIntact(options.cwd, [entry])) { + prior.push(entry) + } else { + // Edited since install, so no longer ours to replace or remove: give it up and leave it in place. + modified.add(entry.relativeRoot) + options.warn?.(`skills: recorded bundle ${entry.relativeRoot} was modified in the workspace; leaving it as is`) + } } const priorByPath = new Map(prior.map((entry) => [entry.relativeRoot, entry])) const legacyHints = new Set() @@ -518,9 +545,11 @@ async function reconcileSkillBundlesLocked( continue } } - const detail = legacyHints.has(candidate.relativeRoot) - ? 'legacy workspace marker is not trusted; remove or migrate it explicitly' - : 'the path is not owned by this daemon ledger' + const detail = modified.has(candidate.relativeRoot) + ? 'it was modified after installation; remove it to reinstall' + : legacyHints.has(candidate.relativeRoot) + ? 'legacy workspace marker is not trusted; remove or migrate it explicitly' + : 'the path is not owned by this daemon ledger' // Skipping leaves the path untouched, which is what refusing wanted; one foreign bundle must not stop every other skill. conflicts.push(candidate.relativeRoot) options.warn?.(`skills: skipped unowned skill ${candidate.relativeRoot}: ${detail}`) @@ -688,7 +717,9 @@ async function reconcileSkillBundlesLocked( return { installed: candidates.map((entry) => entry.relativeRoot), // Replaced and vanished prior bundles count as removed; untouched bundles do not. - removed: recorded.filter((entry) => !kept.has(entry.relativeRoot)).map((entry) => entry.relativeRoot), + removed: recorded + .filter((entry) => !kept.has(entry.relativeRoot) && !modified.has(entry.relativeRoot)) + .map((entry) => entry.relativeRoot), skipped: null, conflicts, owned diff --git a/packages/daemon/test/skill-install-ledger.test.ts b/packages/daemon/test/skill-install-ledger.test.ts index a75ac1b43..82c586758 100644 --- a/packages/daemon/test/skill-install-ledger.test.ts +++ b/packages/daemon/test/skill-install-ledger.test.ts @@ -1,5 +1,5 @@ import { createHash, randomUUID } from 'node:crypto' -import { existsSync, readFileSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync, writeFileSync } from 'node:fs' import { lstat, mkdir, mkdtemp, readdir, readFile, rename, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' @@ -533,38 +533,139 @@ describe.skipIf(process.platform === 'win32')('skill install ledger over a re-ch expect(existsSync(join(cwd, ...BUNDLE.split('/'), 'SKILL.md'))).toBe(true) }) - it('still refuses a recorded path that now holds content it does not own', async () => { + it('skips a recorded path that now holds content it does not own', async () => { await reconcile('v1') const target = join(cwd, ...BUNDLE.split('/')) await rm(target, { recursive: true, force: true }) await mkdir(target, { recursive: true }) await writeFile(join(target, 'manual'), 'do not delete') - await expect(reconcile('v1')).rejects.toThrow(SkillLedgerSafetyError) + const skipped = await reconcile('v1') + + expect(skipped.conflicts).toEqual([BUNDLE]) + expect(skipped.owned).toEqual([]) expect(await readFile(join(target, 'manual'), 'utf8')).toBe('do not delete') + expect((await readSkillLedger(await skillLedgerLocation(cwd, stateDir)))?.phase).toBe('ready') }) // The recorded inode is not proof on its own: a filesystem that recycles inode numbers can seat a // re-checked-out directory on the recorded one, and then only the receipt walk tells the two apart. - it('refuses a recorded path whose identity still matches but whose content no longer does', async () => { + it('skips a recorded path whose identity still matches but whose content no longer does', async () => { await reconcile('v1') const target = join(cwd, ...BUNDLE.split('/')) const recorded = await pathIdentity(target) await writeFile(join(target, 'manual'), 'do not delete') expect(await pathIdentity(target)).toEqual(recorded) - await expect(reconcile('v1')).rejects.toThrow(/installation failure: .*refusing to replace unowned skill/) + const skipped = await reconcile('v1') + + expect(skipped.conflicts).toEqual([BUNDLE]) + expect(skipped.owned).toEqual([]) expect(await readFile(join(target, 'manual'), 'utf8')).toBe('do not delete') }) - it('names the installation failure alongside the recovery failure', async () => { + it('skips an installed skill the agent edited and still updates the others', async () => { + const editedRoot = '.claude/skills/edited' + const options = { cwd, stateDir, agentId: 'a1', runtime: 'claude', cliVersion: '1.5.21' } + await reconcileSkillBundles({ + ...options, + fingerprint: 'v1', + candidates: [ + { ...oneFileReceipt(BUNDLE, 'fixture:v1'), sourceDir }, + { ...oneFileReceipt(editedRoot, 'edited:v1'), sourceDir } + ] + }) + const edited = join(cwd, ...editedRoot.split('/')) + await writeFile(join(edited, 'SKILL.md'), `${BODY}Local notes\n`) + await mkdir(join(edited, 'references')) + await writeFile(join(edited, 'references/notes.md'), 'kept') + const nextSource = join(root, 'next-source') + const nextBody = `${BODY}Updated content\n` + await mkdir(nextSource) + await writeFile(join(nextSource, 'SKILL.md'), nextBody) + const files = [{ path: 'SKILL.md', mode: 0o600, size: Buffer.byteLength(nextBody), sha256: sha256(nextBody) }] + const next = (root: string, sourceKey: string) => ({ + relativeRoot: root, + sourceKey, + sourceDir: nextSource, + files, + treeDigest: treeDigest(files) + }) + const warnings: string[] = [] + + const result = await reconcileSkillBundles({ + ...options, + fingerprint: 'v2', + candidates: [next(BUNDLE, 'fixture:v2'), next(editedRoot, 'edited:v2')], + warn: (message) => warnings.push(message) + }) + + expect(result.installed).toEqual([BUNDLE]) + expect(result.removed).toEqual([BUNDLE]) + expect(result.conflicts).toEqual([editedRoot]) + expect(result.owned.map((entry) => entry.relativeRoot)).toEqual([BUNDLE]) + expect(warnings).toContainEqual(expect.stringMatching(/skipped unowned skill .*edited: it was modified/)) + expect(await readFile(join(cwd, BUNDLE, 'SKILL.md'), 'utf8')).toBe(nextBody) + expect(await readFile(join(edited, 'SKILL.md'), 'utf8')).toBe(`${BODY}Local notes\n`) + expect(await readFile(join(edited, 'references/notes.md'), 'utf8')).toBe('kept') + const location = await skillLedgerLocation(cwd, stateDir) + expect((await readSkillLedger(location))?.phase).toBe('ready') + expect( + (await reconcileSkillBundles({ ...options, fingerprint: 'v2', candidates: [next(BUNDLE, 'fixture:v2')] })) + .conflicts + ).toEqual([]) + expect(await readFile(join(edited, 'references/notes.md'), 'utf8')).toBe('kept') + }) + + it('leaves an edited skill in place when it is no longer desired', async () => { await reconcile('v1') const target = join(cwd, ...BUNDLE.split('/')) - await rm(target, { recursive: true, force: true }) - await mkdir(target, { recursive: true }) - await writeFile(join(target, 'manual'), 'do not delete') + await writeFile(join(target, 'SKILL.md'), 'modified') - await expect(reconcile('v1')).rejects.toThrow(/installation failure: .*refusing to replace unowned skill/) + const result = await reconcileSkillBundles({ + cwd, + stateDir, + agentId: 'a1', + runtime: 'claude', + cliVersion: '1.5.21', + fingerprint: 'v2', + candidates: [] + }) + + expect(result.removed).toEqual([]) + expect(result.owned).toEqual([]) + expect(await readFile(join(target, 'SKILL.md'), 'utf8')).toBe('modified') + }) + + it('names the installation failure alongside the recovery failure', async () => { + await reconcile('v1') + const skills = join(cwd, '.claude/skills') + const nextSource = join(root, 'next-source') + const nextBody = `${BODY}Updated content\n` + await mkdir(nextSource) + await writeFile(join(nextSource, 'SKILL.md'), nextBody) + const files = [{ path: 'SKILL.md', mode: 0o600, size: Buffer.byteLength(nextBody), sha256: sha256(nextBody) }] + + await expect( + reconcileSkillBundles({ + cwd, + stateDir, + agentId: 'a1', + runtime: 'claude', + cliVersion: '1.5.21', + fingerprint: 'v2', + candidates: [ + { relativeRoot: BUNDLE, sourceKey: 'fixture:v2', sourceDir: nextSource, files, treeDigest: treeDigest(files) } + ], + assertMutationAuthority: () => { + const quarantine = readdirSync(skills).find((name) => name.startsWith('.agentconnect-skill-old-')) + if (!quarantine) return + // Tamper with the quarantined prior so recovery cannot put it back either. + writeFileSync(join(skills, quarantine, 'extra'), 'tampered') + throw new Error('publication authority lost after quarantine') + } + }) + ).rejects.toThrow(/could not be restored \(installation failure: .*publication authority lost after quarantine/) }) it('recovers an interrupted journal by restoring the prior it quarantined', async () => { @@ -635,26 +736,44 @@ describe.skipIf(process.platform === 'win32')('skill install ledger over a re-ch expect(existsSync(join(cwd, ...BUNDLE.split('/'), 'SKILL.md'))).toBe(true) }) - it('refuses modified retained bundles but recovers and reinstalls them after cleanup', async () => { + it('gives up a modified retained bundle in recovery and reinstalls it once removed', async () => { const installed = await reconcile('v1') const location = await writeApplyingOver(installed.owned, []) const applying = await readSkillLedger(location) const target = join(cwd, BUNDLE, 'SKILL.md') await writeFile(target, 'modified') - await expect(recover(location, applying!)).rejects.toThrow(/could not restore the prior receipt set/) - expect(await readFile(target, 'utf8')).toBe('modified') - expect(await readSkillLedger(location)).toEqual(applying) - - await rm(join(cwd, '.claude'), { recursive: true, force: true }) const recovered = await recover(location, applying!) expect(recovered.owned).toEqual([]) expect(recovered.fingerprint).toBeUndefined() + expect(await readFile(target, 'utf8')).toBe('modified') + expect((await reconcile('v1')).conflicts).toEqual([BUNDLE]) + expect(await readFile(target, 'utf8')).toBe('modified') + + await rm(join(cwd, '.claude'), { recursive: true, force: true }) const retried = await reconcile('v1') expect(retried.installed).toEqual([BUNDLE]) expect(await readFile(target, 'utf8')).toBe(BODY) }) + + // A journal left by a daemon whose reservation was refused before anything moved: recovery must not wedge on it. + it('recovers a journal whose prior was edited in place and never quarantined', async () => { + const installed = await reconcile('v1') + const target = join(cwd, BUNDLE) + await mkdir(join(target, 'references')) + await writeFile(join(target, 'references/notes.md'), 'kept') + const location = await writeApplyingOver(installed.owned, [operation(BUNDLE)]) + const applying = await readSkillLedger(location) + + const recovered = await recover(location, applying!) + + expect(recovered.phase).toBe('ready') + expect(recovered.owned).toEqual([]) + expect(recovered.fingerprint).toBeUndefined() + expect(await readFile(join(target, 'references/notes.md'), 'utf8')).toBe('kept') + expect((await reconcile('v1')).conflicts).toEqual([BUNDLE]) + }) }) describe.skipIf(process.platform !== 'win32')('skill install ledger on Windows', () => { diff --git a/packages/daemon/test/unified-skills.test.ts b/packages/daemon/test/unified-skills.test.ts index bf051f212..1d789e01a 100644 --- a/packages/daemon/test/unified-skills.test.ts +++ b/packages/daemon/test/unified-skills.test.ts @@ -565,7 +565,7 @@ describe.skipIf(!hasBwrap)('unified isolated skill installation', () => { expect(await readFile(join(cwd, '.runtime/skills/recreated/SKILL.md'), 'utf8')).toBe('manual replacement') }) - it('checks installed bytes rather than trusting a matching plan fingerprint and preserves tampering', async () => { + it('checks installed bytes rather than trusting a matching plan fingerprint and skips a tampered skill', async () => { const sourceDir = await writeSkill(sources, 'repair', 'expected') const cli = fakeCli(() => '.runtime') const localSkills: LocalSkillSource[] = [{ kind: 'dream', key: 'dream:repair', name: 'repair', sourceDir }] @@ -580,9 +580,16 @@ describe.skipIf(!hasBwrap)('unified isolated skill installation', () => { expect(cli.calls).toHaveLength(1) await writeFile(join(cwd, '.runtime/skills/repair/SKILL.md'), 'tampered') - await expect( - installSkills({ id: 'a1', runtime: 'claude', skills: [] }, cwd, { stateDir, localSkills, runCli: cli.run }) - ).rejects.toThrow(/could not be restored|mutation was refused/i) + // An edited skill is skipped as a conflict rather than failing the whole preparation. + const skipped = await installSkills({ id: 'a1', runtime: 'claude', skills: [] }, cwd, { + stateDir, + localSkills, + runCli: cli.run + }) + expect(skipped.errors).toEqual([ + { source: '.runtime/skills/repair', error: 'destination is not owned by this daemon ledger; skill skipped' } + ]) + expect(skipped.owned).toEqual([]) expect(cli.calls).toHaveLength(2) expect(await readFile(join(cwd, '.runtime/skills/repair/SKILL.md'), 'utf8')).toBe('tampered') }, 120_000)