fix(store): refuse unmarked targets by default; reclaim only under --reclaim (W-F4 review)
ci/woodpecker/pr/ci Pipeline failed
ci/woodpecker/pr/ci Pipeline failed
Resolves the review finding onc23a71d7: '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.c23a71d7remains the reviewed object, untouched.
This commit is contained in:
@@ -171,10 +171,22 @@ describe('vetted user store (W-F4)', () => {
|
|||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('reclaims a partial write (directory without marker) and reports recovery', () => {
|
it('refuses an unmarked target directory by default and preserves its content', () => {
|
||||||
const partial = join(paths.userRoot, 'skills', 'demo', '1.0.0');
|
const unmarked = join(paths.userRoot, 'skills', 'demo', '1.0.0');
|
||||||
mkdirSync(partial, { recursive: true });
|
mkdirSync(unmarked, { recursive: true });
|
||||||
writeFileSync(join(partial, 'SKILL.md'), '# torn write\n');
|
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(
|
const result = addStoreEntry(
|
||||||
'skill',
|
'skill',
|
||||||
'demo',
|
'demo',
|
||||||
@@ -183,9 +195,28 @@ describe('vetted user store (W-F4)', () => {
|
|||||||
'op',
|
'op',
|
||||||
undefined,
|
undefined,
|
||||||
paths,
|
paths,
|
||||||
|
{ reclaim: true },
|
||||||
);
|
);
|
||||||
expect(result.status).toBe('recovered-partial');
|
expect(result.status).toBe('reclaimed-unmarked');
|
||||||
expect(readFileSync(join(partial, 'SKILL.md'), 'utf-8')).toBe('# demo\n');
|
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', () => {
|
it('refuses a missing source with a typed error', () => {
|
||||||
@@ -391,6 +422,34 @@ describe('vetted user store (W-F4)', () => {
|
|||||||
).rejects.toThrow(/--by/);
|
).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 () => {
|
it('list exits 0 on an empty store', async () => {
|
||||||
await parse(['list']);
|
await parse(['list']);
|
||||||
expect(process.exitCode).toBeUndefined();
|
expect(process.exitCode).toBeUndefined();
|
||||||
|
|||||||
@@ -25,8 +25,11 @@ import { DEFAULT_MOSAIC_USER_HOME } from '../constants.js';
|
|||||||
* boundary: content lands here only through an explicit `store add` carrying a
|
* boundary: content lands here only through an explicit `store add` carrying a
|
||||||
* named vetting attribution, and every entry is versioned
|
* named vetting attribution, and every entry is versioned
|
||||||
* (`store/<kind>s/<name>/<version>/`) with a `store-entry.json` marker written
|
* (`store/<kind>s/<name>/<version>/`) with a `store-entry.json` marker written
|
||||||
* LAST — a version directory without its marker is a partial write, never a
|
* LAST — a version directory without its marker is never a usable entry, and
|
||||||
* usable entry.
|
* 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
|
* Deferred by design (W-F6 and later): activation/symlink-install into agent
|
||||||
* homes, `current`-pointer pinning, network acquisition. `add` accepts a local
|
* homes, `current`-pointer pinning, network acquisition. `add` accepts a local
|
||||||
@@ -59,7 +62,7 @@ export interface StoreEntryMeta {
|
|||||||
notes?: string;
|
notes?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
export type StoreAddStatus = 'added' | 'recovered-partial';
|
export type StoreAddStatus = 'added' | 'reclaimed-unmarked';
|
||||||
|
|
||||||
export interface StoreAddResult {
|
export interface StoreAddResult {
|
||||||
kind: StoreKind;
|
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
|
* 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.
|
* 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 {
|
function assertSourceTreeHasNoSymlinks(sourcePath: string): void {
|
||||||
const stack: string[] = [sourcePath];
|
const stack: string[] = [sourcePath];
|
||||||
@@ -243,6 +251,7 @@ export function addStoreEntry(
|
|||||||
vettedBy: string,
|
vettedBy: string,
|
||||||
notes: string | undefined,
|
notes: string | undefined,
|
||||||
paths: StorePaths = getDefaultStorePaths(),
|
paths: StorePaths = getDefaultStorePaths(),
|
||||||
|
options: { reclaim?: boolean } = {},
|
||||||
): StoreAddResult {
|
): StoreAddResult {
|
||||||
validateStoreKind(kind);
|
validateStoreKind(kind);
|
||||||
validateStoreName(name);
|
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.`,
|
`${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 });
|
rmSync(target, { recursive: true, force: true });
|
||||||
status = 'recovered-partial';
|
status = 'reclaimed-unmarked';
|
||||||
}
|
}
|
||||||
|
|
||||||
mkdirSync(target, { recursive: true });
|
mkdirSync(target, { recursive: true });
|
||||||
@@ -451,6 +470,10 @@ export function registerStoreCommand(
|
|||||||
.requiredOption('--from <path>', 'Local source directory to vet (no network acquisition)')
|
.requiredOption('--from <path>', 'Local source directory to vet (no network acquisition)')
|
||||||
.requiredOption('--by <operator>', 'Name of the operator vouching for this content')
|
.requiredOption('--by <operator>', 'Name of the operator vouching for this content')
|
||||||
.option('--notes <notes>', 'Vetting notes recorded in the entry metadata')
|
.option('--notes <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(
|
.action(
|
||||||
async (
|
async (
|
||||||
kind: string,
|
kind: string,
|
||||||
@@ -460,6 +483,7 @@ export function registerStoreCommand(
|
|||||||
from: string;
|
from: string;
|
||||||
by: string;
|
by: string;
|
||||||
notes?: string;
|
notes?: string;
|
||||||
|
reclaim: boolean;
|
||||||
},
|
},
|
||||||
) => {
|
) => {
|
||||||
try {
|
try {
|
||||||
@@ -471,8 +495,10 @@ export function registerStoreCommand(
|
|||||||
opts.by,
|
opts.by,
|
||||||
opts.notes,
|
opts.notes,
|
||||||
paths,
|
paths,
|
||||||
|
{ reclaim: opts.reclaim },
|
||||||
);
|
);
|
||||||
const suffix = result.status === 'recovered-partial' ? ' (recovered partial write)' : '';
|
const suffix =
|
||||||
|
result.status === 'reclaimed-unmarked' ? ' (replaced unmarked directory)' : '';
|
||||||
console.log(
|
console.log(
|
||||||
`${result.kind} ${displayStoreName(result.name)} ${result.version}: added${suffix}`,
|
`${result.kind} ${displayStoreName(result.name)} ${result.version}: added${suffix}`,
|
||||||
);
|
);
|
||||||
|
|||||||
Reference in New Issue
Block a user