From 30468c7f23716e2948922e6a504516d5e03f1d67 Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 22 Aug 2026 14:13:16 -0600 Subject: [PATCH 1/2] Fix #29, #30, #31: stop codev update reporting things it never checked Three bugs, one defect wearing three hats. Each states a conclusion nothing established. #31 --dry-run printed "no files will be changed" and then wrote CLAUDE.md.codev-new and AGENTS.md.codev-new. Only the skills call was guarded; copyRootFiles was not. A dry run that writes is worse than no dry run, because it is the one mode an operator trusts specifically on the promise that it touches nothing. copyRootFiles now takes dryRun and reports what it WOULD do. #30 A conflict was declared whenever the destination merely existed, with the reason "Content differs from template" -- while the only test performed was fs.existsSync. Every update handed over a merge task that was usually a no-op. It now compares, after template substitution, and returns `unchanged` for a byte-identical file. An unreadable destination counts as differing: that direction surfaces the file for a human instead of calling it clean. #29 needed more than a comparison. Once a skill directory existed its contents were frozen at install time forever, and --force never helped (that branch only wrapped copyRootFiles). The comment said "without replacing customizations", but with nothing compared the code could not tell a customization from a stale copy, so it preserved both -- which in practice preserved rot. Real cost: a vendored afx skill claiming --branch does not exist, and agents burning turns on it. Comparing against the CURRENT skeleton does not fix that; it only says "same or different", not "customized or stale". Answering the actual question needs provenance, so copySkills now keeps a per-provider .codev-skill-manifest.json of the hash it installed: dest == skeleton -> already current, backfill the hash dest == installed hash -> unmodified but stale, REFRESH dest != installed hash -> local edits, leave alone and SAY SO no manifest entry / unread -> cannot tell, leave alone and SAY SO The last row is the one that matters. "I cannot tell" must not be spelled the same way as "safe to overwrite", or the first update after this ships eats a year of someone's local edits. Customized skills are now reported rather than silently skipped: a customization that is quietly blocking every update is exactly what an operator needs told. init/adopt keep the old semantics -- only update opts in via refreshUnmodified. 15 tests. The #30 and #31 ones fail against the old implementation. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/update-truthfulness.test.ts | 233 ++++++++++++++++++ packages/codev/src/commands/update.ts | 40 ++- packages/codev/src/lib/scaffold.ts | 194 ++++++++++++++- 3 files changed, 451 insertions(+), 16 deletions(-) create mode 100644 packages/codev/src/__tests__/update-truthfulness.test.ts diff --git a/packages/codev/src/__tests__/update-truthfulness.test.ts b/packages/codev/src/__tests__/update-truthfulness.test.ts new file mode 100644 index 000000000..46dd4b323 --- /dev/null +++ b/packages/codev/src/__tests__/update-truthfulness.test.ts @@ -0,0 +1,233 @@ +/** + * Issues #29, #30, #31 — `codev update` saying things that were not checked. + * + * All three are the same defect wearing different clothes: the code reports a + * conclusion it never established. + * + * #31 `--dry-run` announced "no files will be changed" and then wrote. + * #30 A conflict was reported with the reason "Content differs from template" + * when the only test performed was `fs.existsSync`. + * #29 Skills were preserved "without replacing customizations" by a guard + * that tested the DIRECTORY, so it could not tell a customization from a + * stale copy and preserved both. + * + * Each test below fails against the old implementation. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { + copyRootFiles, + copySkills, + SKILL_MANIFEST_FILENAME, +} from '../lib/scaffold.js'; + +// Not under os.tmpdir(): same reasoning as the write-guard fixtures. +const FIXTURE_HOME = path.join(path.resolve(__dirname, '..', '..'), 'node_modules', '.update-fixtures'); + +let base: string; +let target: string; +let skeleton: string; + +beforeEach(() => { + fs.mkdirSync(FIXTURE_HOME, { recursive: true }); + base = fs.mkdtempSync(path.join(FIXTURE_HOME, 'upd-')); + target = path.join(base, 'project'); + skeleton = path.join(base, 'skeleton'); + fs.mkdirSync(path.join(skeleton, 'templates'), { recursive: true }); + fs.mkdirSync(target, { recursive: true }); +}); + +afterEach(() => { + fs.rmSync(FIXTURE_HOME, { recursive: true, force: true }); +}); + +function writeTemplate(file: string, content: string): void { + fs.writeFileSync(path.join(skeleton, 'templates', file), content); +} + +function writeSkill(root: string, provider: string, name: string, files: Record): void { + const dir = path.join(root, `.${provider}`, 'skills', name); + fs.mkdirSync(dir, { recursive: true }); + for (const [rel, content] of Object.entries(files)) { + const abs = path.join(dir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } +} + +describe('#30: copyRootFiles must compare content before calling it a conflict', () => { + it('reports an identical file as unchanged, not as a conflict', () => { + writeTemplate('CLAUDE.md', 'same content\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'same content\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true }); + + expect(r.unchanged).toContain('CLAUDE.md'); + expect(r.conflicts).not.toContain('CLAUDE.md'); + }); + + it('writes NO .codev-new sibling for an identical file', () => { + // The visible cost of the old behavior: a merge task that was a no-op. + writeTemplate('CLAUDE.md', 'same content\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'same content\n'); + + copyRootFiles(target, skeleton, 'proj', { handleConflicts: true }); + + expect(fs.existsSync(path.join(target, 'CLAUDE.md.codev-new'))).toBe(false); + }); + + it('still reports a genuine difference as a conflict, and writes the sibling', () => { + writeTemplate('CLAUDE.md', 'new content\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'old content\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true }); + + expect(r.conflicts).toContain('CLAUDE.md'); + expect(fs.readFileSync(path.join(target, 'CLAUDE.md.codev-new'), 'utf-8')).toBe('new content\n'); + }); + + it('compares AFTER template substitution, so a substituted file is not a false conflict', () => { + writeTemplate('CLAUDE.md', 'project: {{PROJECT_NAME}}\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'project: proj\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true }); + + expect(r.unchanged).toContain('CLAUDE.md'); + expect(r.conflicts).toHaveLength(0); + }); +}); + +describe('#31: --dry-run must not write', () => { + it('writes no .codev-new sibling on a dry run', () => { + writeTemplate('CLAUDE.md', 'new content\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'old content\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true, dryRun: true }); + + expect(r.conflicts).toContain('CLAUDE.md'); + expect(fs.existsSync(path.join(target, 'CLAUDE.md.codev-new'))).toBe(false); + }); + + it('creates no new file on a dry run, but still reports what it would create', () => { + writeTemplate('AGENTS.md', 'fresh\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true, dryRun: true }); + + expect(r.copied).toContain('AGENTS.md'); + expect(fs.existsSync(path.join(target, 'AGENTS.md'))).toBe(false); + }); + + it('a dry run followed by a real run produces the same reported outcome', () => { + // The point of a dry run is that it predicts the real one. + writeTemplate('CLAUDE.md', 'new\n'); + fs.writeFileSync(path.join(target, 'CLAUDE.md'), 'old\n'); + + const dry = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true, dryRun: true }); + const real = copyRootFiles(target, skeleton, 'proj', { handleConflicts: true }); + + expect(dry.conflicts).toEqual(real.conflicts); + expect(dry.copied).toEqual(real.copied); + expect(dry.unchanged).toEqual(real.unchanged); + }); +}); + +describe('#29: skills must be refreshable, and customizations must survive', () => { + it('records a manifest when it first installs a skill', () => { + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + const manifestPath = path.join(target, '.claude', 'skills', SKILL_MANIFEST_FILENAME); + expect(fs.existsSync(manifestPath)).toBe(true); + expect(JSON.parse(fs.readFileSync(manifestPath, 'utf-8'))).toHaveProperty('afx'); + }); + + it('REFRESHES a vendored skill that is unmodified but stale', () => { + // The whole point. Under the old guard this was frozen forever. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2 — now documents --branch\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.refreshed).toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')) + .toBe('v2 — now documents --branch\n'); + }); + + it('LEAVES a locally edited skill alone, and says so', () => { + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + fs.writeFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'my local notes\n'); + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.customized).toContain('.claude/skills/afx/'); + expect(r.refreshed).not.toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')) + .toBe('my local notes\n'); + }); + + it('treats a skill of UNKNOWN provenance as customized, never as refreshable', () => { + // Installed before manifests existed. "I cannot tell" must not be spelled + // the same way as "safe to overwrite" — that would eat real local work. + writeSkill(target, 'claude', 'afx', { 'SKILL.md': 'vendored long ago\n' }); + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.customized).toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')) + .toBe('vendored long ago\n'); + }); + + it('backfills the manifest for an already-current skill, so the NEXT update can tell', () => { + // Unknown provenance but identical content: safe to record, and recording + // it is what lets the following release refresh instead of stalling. + writeSkill(target, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.refreshed).toContain('.claude/skills/afx/'); + }); + + it('notices a change in a nested file, not just the top-level one', () => { + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n', 'references/flags.md': 'a\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n', 'references/flags.md': 'b\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.refreshed).toContain('.claude/skills/afx/'); + }); + + it('still installs a brand-new skill that the project does not have', () => { + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + writeSkill(skeleton, 'claude', 'porch', { 'SKILL.md': 'p1\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'consult', { 'SKILL.md': 'c1\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.copied).toContain('.claude/skills/consult/'); + }); + + it('without refreshUnmodified, behaves exactly as before', () => { + // init/adopt still want the old semantics; only update opts in. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + copySkills(target, skeleton, { skipExisting: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + const r = copySkills(target, skeleton, { skipExisting: true }); + + expect(r.skipped).toContain('.claude/skills/afx/'); + expect(r.refreshed).toHaveLength(0); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')).toBe('v1\n'); + }); +}); diff --git a/packages/codev/src/commands/update.ts b/packages/codev/src/commands/update.ts index e2bfecdfb..92f707f84 100644 --- a/packages/codev/src/commands/update.ts +++ b/packages/codev/src/commands/update.ts @@ -220,13 +220,28 @@ export async function update(options: UpdateOptions = {}): Promise const templatesDir = getTemplatesDir(); - // Add missing provider-native skills without replacing customizations. + // Add missing provider-native skills, refresh stale ones, and leave local + // edits alone (#29). `skipExisting` on its own tested only whether the skill + // DIRECTORY existed, so a vendored skill was frozen at install time forever + // — it could not tell a customization from rot, and preserved both. if (!dryRun) { - const skillsResult = copySkills(targetDir, templatesDir, { skipExisting: true }); + const skillsResult = copySkills(targetDir, templatesDir, { + skipExisting: true, + refreshUnmodified: true, + }); for (const skill of skillsResult.copied) { result.newFiles.push(skill); log(chalk.green(' + (new)'), skill); } + for (const skill of skillsResult.refreshed) { + result.updated.push(skill); + log(chalk.blue(' ~ (refreshed)'), skill); + } + // Say which skills were held back and why. Silently skipping a customized + // skill is how one drifts a year behind without anyone noticing. + for (const skill of skillsResult.customized) { + log(chalk.yellow(' ! (local edits, not refreshed)'), skill); + } } // Update root files (CLAUDE.md, AGENTS.md) @@ -238,10 +253,22 @@ export async function update(options: UpdateOptions = {}): Promise log(chalk.blue(' ~ (updated)'), file); } } else { - const rootResult = copyRootFiles(targetDir, templatesDir, projectName, { handleConflicts: true }); + // #31: pass dryRun through. This call was previously unguarded, so + // `--dry-run` printed "no files will be changed" and then wrote + // `CLAUDE.md.codev-new` and `AGENTS.md.codev-new` to disk. + const rootResult = copyRootFiles(targetDir, templatesDir, projectName, { + handleConflicts: true, + dryRun, + }); for (const file of rootResult.copied) { result.newFiles.push(file); - log(chalk.green(' + (new)'), file); + log(dryRun ? chalk.dim(' + (would create)') : chalk.green(' + (new)'), file); + } + // #30: `unchanged` exists because these used to be reported as conflicts + // with the reason "Content differs from template" — while nothing had + // compared any content. Most updates handed over a no-op merge task. + for (const file of rootResult.unchanged) { + log(chalk.dim(' = (unchanged)'), file); } for (const file of rootResult.conflicts) { result.rootConflicts.push({ @@ -250,7 +277,10 @@ export async function update(options: UpdateOptions = {}): Promise reason: 'Content differs from template', }); log(chalk.yellow(' ! (conflict)'), file); - log(chalk.dim(' New version saved as:'), `${file}.codev-new`); + log( + chalk.dim(dryRun ? ' New version would be saved as:' : ' New version saved as:'), + `${file}.codev-new`, + ); } } diff --git a/packages/codev/src/lib/scaffold.ts b/packages/codev/src/lib/scaffold.ts index 4256b20d5..4983e6980 100644 --- a/packages/codev/src/lib/scaffold.ts +++ b/packages/codev/src/lib/scaffold.ts @@ -7,6 +7,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; +import { createHash } from 'node:crypto'; interface CreateUserDirsOptions { skipExisting?: boolean; @@ -247,11 +248,28 @@ export function copyColdTierDefaults( interface CopyRootFilesOptions { handleConflicts?: boolean; + /** + * Issue #31: when true, report what WOULD happen and write nothing. + * + * `codev update --dry-run` printed "no files will be changed" and then wrote + * `.codev-new` siblings, because only the skills call was guarded and this one + * was not. A dry run that writes is worse than no dry run: it is the one mode + * an operator trusts specifically because it promised not to touch anything. + */ + dryRun?: boolean; } interface CopyRootFilesResult { copied: string[]; conflicts: string[]; + /** + * Issue #30: files that exist and are byte-identical to the template. + * + * Previously these were reported as conflicts with the reason "Content differs + * from template" — a claim nothing had checked, since existence was the only + * test. Every update handed the operator a merge task that was usually a no-op. + */ + unchanged: string[]; } /** @@ -263,9 +281,10 @@ export function copyRootFiles( projectName: string, options: CopyRootFilesOptions = {} ): CopyRootFilesResult { - const { handleConflicts = false } = options; + const { handleConflicts = false, dryRun = false } = options; const copied: string[] = []; const conflicts: string[] = []; + const unchanged: string[] = []; const rootFiles = ['CLAUDE.md', 'AGENTS.md']; for (const file of rootFiles) { @@ -280,19 +299,39 @@ export function copyRootFiles( .replace(/\{\{PROJECT_NAME\}\}/g, projectName); if (fs.existsSync(destPath)) { + // Issue #30: actually compare before claiming the content differs. An + // unreadable destination counts as "differs" — that is the direction that + // surfaces the file for a human to look at, rather than silently calling + // it clean. + let current: string | null = null; + try { + current = fs.readFileSync(destPath, 'utf-8'); + } catch { + current = null; + } + + if (current === content) { + unchanged.push(file); + continue; + } + if (handleConflicts) { - // Create .codev-new for merge - fs.writeFileSync(destPath + '.codev-new', content); conflicts.push(file); + // Issue #31: a dry run reports the conflict but writes no sibling. + if (!dryRun) { + fs.writeFileSync(destPath + '.codev-new', content); + } } // Skip if exists and not handling conflicts } else { - fs.writeFileSync(destPath, content); copied.push(file); + if (!dryRun) { + fs.writeFileSync(destPath, content); + } } } - return { copied, conflicts }; + return { copied, conflicts, unchanged }; } interface CreateProjectsDirOptions { @@ -346,6 +385,20 @@ function copyDirRecursive(src: string, dest: string): void { interface CopySkillsOptions { skipExisting?: boolean; + /** + * Issue #29: refresh a vendored skill whose content still matches an older + * shipped version, and leave a locally-edited one alone. + * + * `skipExisting` alone tests the skill DIRECTORY, so once a skill exists its + * contents are frozen at install time forever. `--force` never helped: that + * branch only wrapped `copyRootFiles`. The comment said "without replacing + * customizations", but with no comparison the code could not tell a + * customization from a stale copy, so it preserved both — which in practice + * means it preserved rot. Real cost: vendored skills stating the OPPOSITE of + * current behavior (the `afx` skill claiming `--branch` does not exist), and + * agents burning turns on flags that were removed releases ago. + */ + refreshUnmodified?: boolean; } interface CopySkillsResult { @@ -353,10 +406,73 @@ interface CopySkillsResult { copied: string[]; /** Project-relative provider-qualified paths, including a trailing slash. */ skipped: string[]; + /** + * Skills refreshed because their content was unmodified but stale (#29). + * Project-relative provider-qualified paths, including a trailing slash. + */ + refreshed: string[]; + /** + * Skills left alone because they carry local edits (#29). Reported rather + * than silently skipped: a customization that is now blocking an update is + * exactly what an operator needs told. + */ + customized: string[]; /** Project-relative provider skill roots that were created. */ directoriesCreated: string[]; } +/** Filename of the per-provider skill manifest (#29). */ +export const SKILL_MANIFEST_FILENAME = '.codev-skill-manifest.json'; + +/** + * Content hash of a skill directory: every file's relative path and bytes. + * + * Returns null when the tree cannot be read. Callers treat null as "I could not + * tell", never as "unchanged". + */ +function hashSkillDir(dir: string): string | null { + const files: string[] = []; + const visit = (d: string, prefix: string): void => { + for (const entry of fs.readdirSync(d, { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name))) { + const rel = prefix ? `${prefix}/${entry.name}` : entry.name; + const abs = path.join(d, entry.name); + if (entry.isDirectory()) visit(abs, rel); + else if (entry.isFile()) files.push(`${rel}${fs.readFileSync(abs, 'utf-8')}`); + } + }; + try { + visit(dir, ''); + } catch { + return null; + } + return createHash('sha256').update(files.join('')).digest('hex'); +} + +/** + * Read the skill manifest for a provider's skills dir. + * + * Absent or unparseable manifest yields an empty map, which makes every skill + * "unknown provenance" rather than "unmodified" — the direction that leaves + * local work alone. + */ +function readSkillManifest(skillsDir: string): Record { + try { + const raw = fs.readFileSync(path.join(skillsDir, SKILL_MANIFEST_FILENAME), 'utf-8'); + const parsed: unknown = JSON.parse(raw); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + return parsed as Record; + } + } catch { /* absent or unreadable — treat as empty */ } + return {}; +} + +function writeSkillManifest(skillsDir: string, manifest: Record): void { + fs.writeFileSync( + path.join(skillsDir, SKILL_MANIFEST_FILENAME), + JSON.stringify(manifest, null, 2) + '\n', + ); +} + export const SKILL_PROVIDERS = ['claude', 'codex'] as const; /** @@ -371,9 +487,11 @@ export function copySkills( skeletonDir: string, options: CopySkillsOptions = {} ): CopySkillsResult { - const { skipExisting = false } = options; + const { skipExisting = false, refreshUnmodified = false } = options; const copied: string[] = []; const skipped: string[] = []; + const refreshed: string[] = []; + const customized: string[] = []; const directoriesCreated: string[] = []; for (const provider of SKILL_PROVIDERS) { @@ -389,23 +507,77 @@ export function copySkills( // Older/corrupt skeletons may not provide every configured provider. if (!fs.existsSync(srcDir)) continue; + // Issue #29: provenance for "is this vendored copy modified, or just old?" + const manifest = refreshUnmodified ? readSkillManifest(skillsDir) : {}; + let manifestChanged = false; + for (const entry of fs.readdirSync(srcDir, { withFileTypes: true })) { if (!entry.isDirectory()) continue; + const srcSkillDir = path.join(srcDir, entry.name); const destSkillDir = path.join(skillsDir, entry.name); const relativeSkillDir = `${relativeSkillsDir}/${entry.name}/`; - if (skipExisting && fs.existsSync(destSkillDir)) { - skipped.push(relativeSkillDir); - continue; + if (fs.existsSync(destSkillDir)) { + if (!refreshUnmodified) { + if (skipExisting) { + skipped.push(relativeSkillDir); + continue; + } + } else { + const srcHash = hashSkillDir(srcSkillDir); + const destHash = hashSkillDir(destSkillDir); + const installedHash = manifest[entry.name]; + + // Already current. Record the hash if we never had it, so the NEXT + // update can tell modified from stale without another guess. + if (srcHash !== null && destHash === srcHash) { + skipped.push(relativeSkillDir); + if (installedHash !== srcHash) { + manifest[entry.name] = srcHash; + manifestChanged = true; + } + continue; + } + + // Unknown provenance (installed before manifests, or unreadable) or a + // local edit. Both leave the copy alone — but they are REPORTED, not + // silently skipped, because a customization now blocking an update is + // exactly what an operator needs told. + if (destHash === null || installedHash === undefined || destHash !== installedHash) { + customized.push(relativeSkillDir); + continue; + } + + // Vendored copy still matches what we installed, and the skeleton has + // moved: it is stale, not customized. Refresh it. + copyDirRecursive(srcSkillDir, destSkillDir); + refreshed.push(relativeSkillDir); + if (srcHash !== null) { + manifest[entry.name] = srcHash; + manifestChanged = true; + } + continue; + } } - copyDirRecursive(path.join(srcDir, entry.name), destSkillDir); + copyDirRecursive(srcSkillDir, destSkillDir); copied.push(relativeSkillDir); + if (refreshUnmodified) { + const srcHash = hashSkillDir(srcSkillDir); + if (srcHash !== null) { + manifest[entry.name] = srcHash; + manifestChanged = true; + } + } + } + + if (refreshUnmodified && manifestChanged) { + writeSkillManifest(skillsDir, manifest); } } - return { copied, skipped, directoriesCreated }; + return { copied, skipped, refreshed, customized, directoriesCreated }; } interface CopyRolesOptions { From e1652b8f6593be74a5aacf1913d648fb141ab2d3 Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 22 Aug 2026 16:55:49 -0600 Subject: [PATCH 2/2] [Fix #29] Replace instead of overlay, and stop freezing init-only projects The review found a path that re-creates the exact freeze #29 exists to fix, and verified it by running the code rather than reading it. copyDirRecursive is additive -- it never removes destination files absent from the source. So after a refresh where the skeleton DELETED a file: - the deleted file survives in the vendored skill, which is the stale-doc symptom #29 is about, surviving the fix meant to remove it - the recorded hash is the skeleton's, but the destination now hashes to src+leftover, so the NEXT update reads destHash != installedHash, calls the skill customized, and freezes it forever with zero local edits Silent, unrecoverable without deleting the directory by hand, and it lands on the first release that removes a skill file. The refresh branch now rmSync's the destination first -- safe there and only there, because destHash === installedHash was just proven, so nothing local is being discarded. Also from the review: --dry-run --force still wrote. I guarded the conflict branch and left the force branch calling copyRootFiles with no options at all, so the exact combination of two real CLI flags still printed "no files will be changed" and then created the file. init and adopt never wrote a manifest, so a project installed at v1 that never happened to run `update` at v1 arrived at v2 as unknown provenance and was held back from every future refresh -- the fix would never have reached the projects most in need of it. A separate recordManifest option (implied by refreshUnmodified) leaves provenance without changing init/adopt semantics: adopt still refuses to rewrite an existing project's skills. hashSkillDir read files as utf-8, which collapses invalid bytes to U+FFFD, so two different binary assets hashed identically and a modified one would have read as unmodified and been overwritten. Now hashes raw Buffers with length-framed fields, which also stops a+bc colliding with ab+c. The held-back message now names the remedy, since --force does not reach skills. 9 more tests, including the two-update sequence that proves a skeleton deletion no longer poisons provenance. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/update-truthfulness.test.ts | 146 ++++++++++++++++++ packages/codev/src/commands/adopt.ts | 5 +- packages/codev/src/commands/init.ts | 6 +- packages/codev/src/commands/update.ts | 11 +- packages/codev/src/lib/scaffold.ts | 49 +++++- 5 files changed, 208 insertions(+), 9 deletions(-) diff --git a/packages/codev/src/__tests__/update-truthfulness.test.ts b/packages/codev/src/__tests__/update-truthfulness.test.ts index 46dd4b323..b11c81658 100644 --- a/packages/codev/src/__tests__/update-truthfulness.test.ts +++ b/packages/codev/src/__tests__/update-truthfulness.test.ts @@ -119,6 +119,18 @@ describe('#31: --dry-run must not write', () => { expect(fs.existsSync(path.join(target, 'AGENTS.md'))).toBe(false); }); + it('does not write on a dry run through the FORCE branch either', () => { + // The force branch was missed the first time: `copyRootFiles` was called + // with no options at all, so `--dry-run --force` announced "no files will + // be changed" and then created the file. Both are real CLI flags. + writeTemplate('AGENTS.md', 'fresh\n'); + + const r = copyRootFiles(target, skeleton, 'proj', { dryRun: true }); + + expect(r.copied).toContain('AGENTS.md'); + expect(fs.existsSync(path.join(target, 'AGENTS.md'))).toBe(false); + }); + it('a dry run followed by a real run produces the same reported outcome', () => { // The point of a dry run is that it predicts the real one. writeTemplate('CLAUDE.md', 'new\n'); @@ -231,3 +243,137 @@ describe('#29: skills must be refreshable, and customizations must survive', () expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')).toBe('v1\n'); }); }); + +describe('#29 follow-up: a file deleted from the skeleton must not freeze the skill', () => { + it('removes a file the skeleton dropped, instead of leaving it behind', () => { + // copyDirRecursive is additive. Overlaying leaves the deleted file in the + // vendored skill — the exact stale-doc symptom #29 exists to fix, surviving + // the fix that was supposed to remove it. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n', 'references/old.md': 'removed in v2\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + fs.rmSync(path.join(skeleton, '.claude/skills/afx/references'), { recursive: true, force: true }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(fs.existsSync(path.join(target, '.claude/skills/afx/references/old.md'))).toBe(false); + }); + + it('stays refreshable on the NEXT update after a skeleton deletion', () => { + // The compounding half. An overlay records the skeleton's hash while the + // destination hashes to src+leftover, so the following update reads + // destHash !== installedHash, calls it customized, and freezes it forever + // with zero local edits. Silent and unrecoverable without deleting the dir. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n', 'references/old.md': 'gone in v2\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + fs.rmSync(path.join(skeleton, '.claude/skills/afx/references'), { recursive: true, force: true }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v3\n' }); + const third = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(third.customized).toHaveLength(0); + expect(third.refreshed).toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')).toBe('v3\n'); + }); + + it('a local edit still survives a skeleton deletion', () => { + // The removal is scoped to the branch that already proved the copy is + // unmodified. A customized skill must never reach it. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n', 'references/old.md': 'a\n' }); + copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + fs.writeFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'MY NOTES\n'); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + fs.rmSync(path.join(skeleton, '.claude/skills/afx/references'), { recursive: true, force: true }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.customized).toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')).toBe('MY NOTES\n'); + expect(fs.existsSync(path.join(target, '.claude/skills/afx/references/old.md'))).toBe(true); + }); +}); + +describe('#29 follow-up: init/adopt must leave provenance behind', () => { + it('recordManifest writes a manifest WITHOUT refreshing anything', () => { + // adopt's shape: it must never rewrite an existing project's skills, but it + // should still leave enough behind for a later update to tell stale from + // customized. + writeSkill(target, 'claude', 'afx', { 'SKILL.md': 'pre-existing\n' }); + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + + const r = copySkills(target, skeleton, { skipExisting: true, recordManifest: true }); + + expect(r.refreshed).toHaveLength(0); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')) + .toBe('pre-existing\n'); + }); + + it('a skill installed with recordManifest is refreshable on the next update', () => { + // The whole reason this option exists. Installed at v1 via init, never + // updated at v1 — under the original fix that project would be classified + // unknown-provenance and frozen out of every future refresh. + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + copySkills(target, skeleton, { recordManifest: true }); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.refreshed).toContain('.claude/skills/afx/'); + expect(r.customized).toHaveLength(0); + }); + + it('recordManifest still respects a local edit made after install', () => { + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v1\n' }); + copySkills(target, skeleton, { recordManifest: true }); + fs.writeFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'MINE\n'); + + writeSkill(skeleton, 'claude', 'afx', { 'SKILL.md': 'v2\n' }); + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.customized).toContain('.claude/skills/afx/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/afx/SKILL.md'), 'utf-8')).toBe('MINE\n'); + }); +}); + +describe('#29 follow-up: hashing raw bytes', () => { + const BYTES_A = Buffer.from([0xff, 0xfe, 0x01]); + const BYTES_B = Buffer.from([0xff, 0xfe, 0x02]); + + function installBinarySkill(bytes: Buffer): void { + writeSkill(skeleton, 'claude', 'assets', { 'SKILL.md': 'v1\n' }); + fs.writeFileSync(path.join(skeleton, '.claude/skills/assets/asset.bin'), bytes); + } + + it('sees a one-byte binary change that utf-8 decoding would flatten', () => { + // Both byte strings decode to the same U+FFFD sequence, so a utf-8 hash + // rated them identical — a locally modified binary would have read as + // unmodified and been silently overwritten. + installBinarySkill(BYTES_A); + copySkills(target, skeleton, { recordManifest: true }); + + // Local edit: same length, one byte different, invalid utf-8 either way. + fs.writeFileSync(path.join(target, '.claude/skills/assets/asset.bin'), BYTES_B); + writeSkill(skeleton, 'claude', 'assets', { 'SKILL.md': 'v2\n' }); + fs.writeFileSync(path.join(skeleton, '.claude/skills/assets/asset.bin'), BYTES_A); + + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.customized).toContain('.claude/skills/assets/'); + expect(fs.readFileSync(path.join(target, '.claude/skills/assets/asset.bin'))).toEqual(BYTES_B); + }); + + it('still refreshes when the binary is untouched and only text moved', () => { + installBinarySkill(BYTES_A); + copySkills(target, skeleton, { recordManifest: true }); + + writeSkill(skeleton, 'claude', 'assets', { 'SKILL.md': 'v2\n' }); + fs.writeFileSync(path.join(skeleton, '.claude/skills/assets/asset.bin'), BYTES_A); + + const r = copySkills(target, skeleton, { skipExisting: true, refreshUnmodified: true }); + + expect(r.refreshed).toContain('.claude/skills/assets/'); + }); +}); diff --git a/packages/codev/src/commands/adopt.ts b/packages/codev/src/commands/adopt.ts index 3e915a36f..986fad1f4 100644 --- a/packages/codev/src/commands/adopt.ts +++ b/packages/codev/src/commands/adopt.ts @@ -133,7 +133,10 @@ export async function adopt(options: AdoptOptions = {}): Promise { // Existing local copies in codev/ are left in place — they take precedence via the resolution chain. // Copy provider-native skills, preserving existing skill directories. - const skillsResult = copySkills(targetDir, skeletonDir, { skipExisting: true }); + // #29: recordManifest, not refreshUnmodified -- adopt must not rewrite an + // existing project's skills. It only leaves provenance for the ones it + // installs, so a later `update` can tell stale from customized. + const skillsResult = copySkills(targetDir, skeletonDir, { skipExisting: true, recordManifest: true }); for (const directory of skillsResult.directoriesCreated) { console.log(chalk.green(' +'), directory); fileCount++; diff --git a/packages/codev/src/commands/init.ts b/packages/codev/src/commands/init.ts index 4dccd5e16..67d953559 100644 --- a/packages/codev/src/commands/init.ts +++ b/packages/codev/src/commands/init.ts @@ -97,7 +97,11 @@ export async function init(projectName?: string, options: InitOptions = {}): Pro // They resolve at runtime from the installed npm package via the unified file resolver. // Copy provider-native skills (must exist on disk for tool discovery). - const skillsResult = copySkills(targetDir, skeletonDir); + // #29: record provenance for what we install. Without it, a project that + // never happens to run `update` at this version reaches the next one with + // no manifest, is classified "unknown provenance", and is held back from + // every future skill refresh permanently. + const skillsResult = copySkills(targetDir, skeletonDir, { recordManifest: true }); for (const directory of skillsResult.directoriesCreated) { console.log(chalk.green(' +'), directory); fileCount++; diff --git a/packages/codev/src/commands/update.ts b/packages/codev/src/commands/update.ts index 92f707f84..0e7c9cad5 100644 --- a/packages/codev/src/commands/update.ts +++ b/packages/codev/src/commands/update.ts @@ -242,12 +242,21 @@ export async function update(options: UpdateOptions = {}): Promise for (const skill of skillsResult.customized) { log(chalk.yellow(' ! (local edits, not refreshed)'), skill); } + // Naming the state without the remedy is half an answer, and `--force` + // does not reach skills. Say how to take the shipped version. + if (skillsResult.customized.length > 0) { + log(chalk.dim(' To take the shipped version of one, delete its directory and re-run.')); + } } // Update root files (CLAUDE.md, AGENTS.md) const projectName = path.basename(targetDir); if (force) { - const rootResult = copyRootFiles(targetDir, templatesDir, projectName); + // #31 again: this branch was missed the first time, so + // `codev update --dry-run --force` printed "no files will be changed" + // and then created a missing CLAUDE.md / AGENTS.md. Both are real CLI + // flags and combining them is an obvious thing to do. + const rootResult = copyRootFiles(targetDir, templatesDir, projectName, { dryRun }); for (const file of rootResult.copied) { result.updated.push(file); log(chalk.blue(' ~ (updated)'), file); diff --git a/packages/codev/src/lib/scaffold.ts b/packages/codev/src/lib/scaffold.ts index 4983e6980..f842df436 100644 --- a/packages/codev/src/lib/scaffold.ts +++ b/packages/codev/src/lib/scaffold.ts @@ -399,6 +399,18 @@ interface CopySkillsOptions { * agents burning turns on flags that were removed releases ago. */ refreshUnmodified?: boolean; + /** + * Record provenance for every skill installed, without enabling refresh. + * + * For init/adopt. Without it, a project initialized at v1 that never happens + * to run `update` at v1 reaches v2 with no manifest, is classified "unknown + * provenance", and is held back permanently -- so the #29 fix would never + * reach the projects that need it most. Init knows exactly what it installed, + * so the provenance is free there. + * + * Implied by `refreshUnmodified`. + */ + recordManifest?: boolean; } interface CopySkillsResult { @@ -431,13 +443,24 @@ export const SKILL_MANIFEST_FILENAME = '.codev-skill-manifest.json'; * tell", never as "unchanged". */ function hashSkillDir(dir: string): string | null { - const files: string[] = []; + const parts: Buffer[] = []; const visit = (d: string, prefix: string): void => { for (const entry of fs.readdirSync(d, { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name))) { const rel = prefix ? `${prefix}/${entry.name}` : entry.name; const abs = path.join(d, entry.name); if (entry.isDirectory()) visit(abs, rel); - else if (entry.isFile()) files.push(`${rel}${fs.readFileSync(abs, 'utf-8')}`); + else if (entry.isFile()) { + // Raw bytes, length-framed. Reading as utf-8 collapses invalid bytes to + // U+FFFD, so two different binary assets can hash identically and a + // modified one would read as unmodified and be overwritten. Framing each + // field by length also stops a+bc colliding with ab+c. + const name = Buffer.from(rel, 'utf-8'); + const content = fs.readFileSync(abs); + const header = Buffer.alloc(8); + header.writeUInt32BE(name.length, 0); + header.writeUInt32BE(content.length, 4); + parts.push(header, name, content); + } } }; try { @@ -445,7 +468,9 @@ function hashSkillDir(dir: string): string | null { } catch { return null; } - return createHash('sha256').update(files.join('')).digest('hex'); + const hash = createHash('sha256'); + for (const part of parts) hash.update(part); + return hash.digest('hex'); } /** @@ -488,6 +513,7 @@ export function copySkills( options: CopySkillsOptions = {} ): CopySkillsResult { const { skipExisting = false, refreshUnmodified = false } = options; + const recordManifest = options.recordManifest ?? refreshUnmodified; const copied: string[] = []; const skipped: string[] = []; const refreshed: string[] = []; @@ -508,7 +534,7 @@ export function copySkills( if (!fs.existsSync(srcDir)) continue; // Issue #29: provenance for "is this vendored copy modified, or just old?" - const manifest = refreshUnmodified ? readSkillManifest(skillsDir) : {}; + const manifest = recordManifest ? readSkillManifest(skillsDir) : {}; let manifestChanged = false; for (const entry of fs.readdirSync(srcDir, { withFileTypes: true })) { @@ -551,6 +577,17 @@ export function copySkills( // Vendored copy still matches what we installed, and the skeleton has // moved: it is stale, not customized. Refresh it. + // + // REPLACE, don't overlay. `copyDirRecursive` is additive — it never + // removes destination files absent from the source. Overlaying leaves + // a file the skeleton deleted sitting in the vendored skill (exactly + // the stale-doc symptom #29 exists to fix), AND poisons provenance: + // the recorded hash is the skeleton's, but the destination now hashes + // to src+leftover, so the NEXT update reads `destHash !== installed`, + // calls the skill customized, and freezes it forever with zero local + // edits. Safe to remove here and only here, because `destHash === + // installedHash` was just proven: nothing local is being discarded. + fs.rmSync(destSkillDir, { recursive: true, force: true }); copyDirRecursive(srcSkillDir, destSkillDir); refreshed.push(relativeSkillDir); if (srcHash !== null) { @@ -563,7 +600,7 @@ export function copySkills( copyDirRecursive(srcSkillDir, destSkillDir); copied.push(relativeSkillDir); - if (refreshUnmodified) { + if (recordManifest) { const srcHash = hashSkillDir(srcSkillDir); if (srcHash !== null) { manifest[entry.name] = srcHash; @@ -572,7 +609,7 @@ export function copySkills( } } - if (refreshUnmodified && manifestChanged) { + if (recordManifest && manifestChanged) { writeSkillManifest(skillsDir, manifest); } }