From 9e1b0dcb6236f6691e633c51774267334840207b Mon Sep 17 00:00:00 2001 From: fargo Date: Mon, 17 Aug 2026 13:14:20 -0500 Subject: [PATCH] fix(store): refuse unmarked targets by default; reclaim only under --reclaim (W-F4 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the review finding on c23a71d7: 'store add' silently deleted a markerless target directory and reported it as recovered-partial, but the code cannot distinguish its own interrupted-write debris from content the operator placed by hand — and the USER root's entire contract is that tooling never destroys operator content. - addStoreEntry now throws typed STORE_TARGET_UNMARKED on an unmarked target; deletion happens only when the caller passes { reclaim: true }. - CLI: 'store add' gains --reclaim ('replace an existing UNMARKED target directory; refuses without this flag'). - Status renamed recovered-partial -> reclaimed-unmarked so even the opted-in path names what it did (fix 2 folded into fix 1). - Spec: default-refusal test asserts operator content SURVIVES; opt-in test asserts replacement; two CLI tests cover exit codes. - Ordering test (second review round): 'reclaim can never destroy a marked, vetted entry' — adds a vetted entry, re-adds with reclaim:true, asserts STORE_ALREADY_PRESENT AND the original content + marker survive on disk. Pins marker-check-before-reclaim-check against the guard-clause-migrates-upward refactor; discrimination proven by sabotaging the order (1 failed, exactly this test) and restoring (49/49). - TOCTOU note added at assertSourceTreeHasNoSymlinks per review (known check-then-use window, accepted for a local operator-run CLI). Gates (settled set, rc-honest): store spec 49/49; package vitest 87 files / 1596 tests; package build+lint rc0; root build 25/25 + typecheck 45/45; prettier --check rc0 on all four touched files. c23a71d7 remains the reviewed object, untouched. --- packages/mosaic/src/commands/store.spec.ts | 71 ++++++++++++++++++++-- packages/mosaic/src/commands/store.ts | 38 ++++++++++-- 2 files changed, 97 insertions(+), 12 deletions(-) diff --git a/packages/mosaic/src/commands/store.spec.ts b/packages/mosaic/src/commands/store.spec.ts index 91a005be..5dbea60f 100644 --- a/packages/mosaic/src/commands/store.spec.ts +++ b/packages/mosaic/src/commands/store.spec.ts @@ -171,10 +171,22 @@ describe('vetted user store (W-F4)', () => { ]); }); - it('reclaims a partial write (directory without marker) and reports recovery', () => { - const partial = join(paths.userRoot, 'skills', 'demo', '1.0.0'); - mkdirSync(partial, { recursive: true }); - writeFileSync(join(partial, 'SKILL.md'), '# torn write\n'); + it('refuses an unmarked target directory by default and preserves its content', () => { + const unmarked = join(paths.userRoot, 'skills', 'demo', '1.0.0'); + mkdirSync(unmarked, { recursive: true }); + writeFileSync(join(unmarked, 'SKILL.md'), '# operator content\n'); + expectStoreError( + () => addStoreEntry('skill', 'demo', '1.0.0', createSource('demo'), 'op', undefined, paths), + 'STORE_TARGET_UNMARKED', + ); + // The operator's hand-placed content survives the refusal. + expect(readFileSync(join(unmarked, 'SKILL.md'), 'utf-8')).toBe('# operator content\n'); + }); + + it('reclaims an unmarked target only under explicit reclaim opt-in', () => { + const unmarked = join(paths.userRoot, 'skills', 'demo', '1.0.0'); + mkdirSync(unmarked, { recursive: true }); + writeFileSync(join(unmarked, 'SKILL.md'), '# torn write\n'); const result = addStoreEntry( 'skill', 'demo', @@ -183,9 +195,28 @@ describe('vetted user store (W-F4)', () => { 'op', undefined, paths, + { reclaim: true }, ); - expect(result.status).toBe('recovered-partial'); - expect(readFileSync(join(partial, 'SKILL.md'), 'utf-8')).toBe('# demo\n'); + expect(result.status).toBe('reclaimed-unmarked'); + expect(readFileSync(join(unmarked, 'SKILL.md'), 'utf-8')).toBe('# demo\n'); + }); + + it('reclaim can never destroy a marked, vetted entry (append-only holds under --reclaim)', () => { + addStoreEntry('skill', 'demo', '1.0.0', createSource('demo'), 'op', undefined, paths); + const marked = join(paths.userRoot, 'skills', 'demo', '1.0.0'); + const vettedContent = readFileSync(join(marked, 'SKILL.md'), 'utf-8'); + expectStoreError( + () => + addStoreEntry('skill', 'demo', '1.0.0', createSource('demo'), 'op', undefined, paths, { + reclaim: true, + }), + 'STORE_ALREADY_PRESENT', + ); + // Load-bearing half: the throw alone does not prove nothing was deleted + // before it. Pins the marker-check-before-reclaim-check ordering against + // the guard-clause-migrates-upward refactor (review finding on b2124c6). + expect(readFileSync(join(marked, 'SKILL.md'), 'utf-8')).toBe(vettedContent); + expect(existsSync(join(marked, 'store-entry.json'))).toBe(true); }); it('refuses a missing source with a typed error', () => { @@ -391,6 +422,34 @@ describe('vetted user store (W-F4)', () => { ).rejects.toThrow(/--by/); }); + it('add exits nonzero on an unmarked target without --reclaim, preserving content', async () => { + const unmarked = join(paths.userRoot, 'skills', 'demo', '1.0.0'); + mkdirSync(unmarked, { recursive: true }); + writeFileSync(join(unmarked, 'SKILL.md'), '# operator\n'); + await parse(['add', 'skill', 'demo', '1.0.0', '--from', createSource('demo'), '--by', 'op']); + expect(process.exitCode).toBe(1); + expect(readFileSync(join(unmarked, 'SKILL.md'), 'utf-8')).toBe('# operator\n'); + }); + + it('add --reclaim replaces the unmarked target and succeeds', async () => { + const unmarked = join(paths.userRoot, 'skills', 'demo', '1.0.0'); + mkdirSync(unmarked, { recursive: true }); + writeFileSync(join(unmarked, 'SKILL.md'), '# torn\n'); + await parse([ + 'add', + 'skill', + 'demo', + '1.0.0', + '--from', + createSource('demo'), + '--by', + 'op', + '--reclaim', + ]); + expect(process.exitCode).toBeUndefined(); + expect(readFileSync(join(unmarked, 'SKILL.md'), 'utf-8')).toBe('# demo\n'); + }); + it('list exits 0 on an empty store', async () => { await parse(['list']); expect(process.exitCode).toBeUndefined(); diff --git a/packages/mosaic/src/commands/store.ts b/packages/mosaic/src/commands/store.ts index 6227e1f2..b58cfc36 100644 --- a/packages/mosaic/src/commands/store.ts +++ b/packages/mosaic/src/commands/store.ts @@ -25,8 +25,11 @@ import { DEFAULT_MOSAIC_USER_HOME } from '../constants.js'; * boundary: content lands here only through an explicit `store add` carrying a * named vetting attribution, and every entry is versioned * (`store/s///`) with a `store-entry.json` marker written - * LAST — a version directory without its marker is a partial write, never a - * usable entry. + * LAST — a version directory without its marker is never a usable entry, and + * an unmarked target is REFUSED by default: it may be this tool's own debris + * from an interrupted add, or content the operator placed by hand, and the + * code cannot tell those apart — so deletion happens only under an explicit + * `--reclaim` opt-in, and the result status names what was done. * * Deferred by design (W-F6 and later): activation/symlink-install into agent * homes, `current`-pointer pinning, network acquisition. `add` accepts a local @@ -59,7 +62,7 @@ export interface StoreEntryMeta { notes?: string; } -export type StoreAddStatus = 'added' | 'recovered-partial'; +export type StoreAddStatus = 'added' | 'reclaimed-unmarked'; export interface StoreAddResult { kind: StoreKind; @@ -209,6 +212,11 @@ function isInsideRoot(candidate: string, root: string): boolean { /** * Refuse any symlink in the source tree — the vetting boundary copies real * content only, so a vetted entry can never carry a link that escapes it. + * + * NOTE: known check-then-use window between this walk and the `cpSync` below: + * a symlink created concurrently with the add could slip through. Accepted + * for a local, operator-run CLI; revisit before any unattended or networked + * acquisition path exists. */ function assertSourceTreeHasNoSymlinks(sourcePath: string): void { const stack: string[] = [sourcePath]; @@ -243,6 +251,7 @@ export function addStoreEntry( vettedBy: string, notes: string | undefined, paths: StorePaths = getDefaultStorePaths(), + options: { reclaim?: boolean } = {}, ): StoreAddResult { validateStoreKind(kind); validateStoreName(name); @@ -285,9 +294,19 @@ export function addStoreEntry( `${kind} "${name}" version "${version}" is already present at ${target}; stores are append-only — add a new version instead.`, ); } - // Partial write (no marker): safe to reclaim. + // Unmarked target: either this tool's own debris from an interrupted add, + // or content the operator placed by hand — indistinguishable on disk. The + // USER root's contract is that tooling never destroys operator content, + // so deletion requires the explicit --reclaim opt-in (review finding on + // c23a71d7: silent rmSync under a benign-sounding status). + if (!options.reclaim) { + throw new StoreError( + 'STORE_TARGET_UNMARKED', + `Target exists without ${STORE_ENTRY_MARKER}: ${target}. Refusing to delete unmarked content — if this is debris from an interrupted add, re-run with --reclaim to replace it.`, + ); + } rmSync(target, { recursive: true, force: true }); - status = 'recovered-partial'; + status = 'reclaimed-unmarked'; } mkdirSync(target, { recursive: true }); @@ -451,6 +470,10 @@ export function registerStoreCommand( .requiredOption('--from ', 'Local source directory to vet (no network acquisition)') .requiredOption('--by ', 'Name of the operator vouching for this content') .option('--notes ', 'Vetting notes recorded in the entry metadata') + .option( + '--reclaim', + 'Replace an existing UNMARKED target directory (e.g. debris from an interrupted add); refuses without this flag', + ) .action( async ( kind: string, @@ -460,6 +483,7 @@ export function registerStoreCommand( from: string; by: string; notes?: string; + reclaim: boolean; }, ) => { try { @@ -471,8 +495,10 @@ export function registerStoreCommand( opts.by, opts.notes, paths, + { reclaim: opts.reclaim }, ); - const suffix = result.status === 'recovered-partial' ? ' (recovered partial write)' : ''; + const suffix = + result.status === 'reclaimed-unmarked' ? ' (replaced unmarked directory)' : ''; console.log( `${result.kind} ${displayStoreName(result.name)} ${result.version}: added${suffix}`, );