fix(daemon): skip a locally edited installed skill instead of failing every turn

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<string>()
+    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<b
   }
 }
 
+/** Whether a prior still at its own path, with no artifact of its operation beside it, no longer matches its receipt. */
+async function editedInPlace(cwd: string, prior: OwnedSkillBundle, operation?: JournalOperation): Promise<boolean> {
+  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<boolean> {
   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<string>()
     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<string>()
@@ -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

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', () => {

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)