diff --git a/src/cli.ts b/src/cli.ts index 5e92261..247197b 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -83,6 +83,21 @@ interface Flags { to?: string; /** Entry version for `diff`. Named --rev because -v/--version is taken. */ rev?: number; + /** + * Optimistic concurrency guard for `update`: the version the CALLER read. + * + * Distinct from the version `update` reads for itself immediately before + * writing. That internal read only ever covers the microseconds inside one + * invocation, so it cannot see the race that actually loses edits — an agent + * reads an entry, composes a new body over the following minutes, and by the + * time it types `update` the CLI's own re-read has already absorbed whatever + * landed in between and the guard passes. Only the caller knows the version + * its content was written against, so only the caller can supply it. + * + * Held as the raw argv string so a malformed value can be refused by name + * rather than becoming NaN. + */ + ifVersionRaw?: string; since?: string; topic?: string; dedupe?: boolean; @@ -240,6 +255,11 @@ function parseArgs(argv: string[]): ParseResult { // --version (print the package version), and re-pointing it at an entry // version would silently break every existing `knowledge -v` invocation. case '--rev': flags.rev = Number(argv[i + 1]); i += 1; break; + // Parsed as a raw string and validated at use, not with `Number()` here. + // `Number('abc')` is NaN, and NaN silently serialises to `null` in the + // If-Match header — the guard would then be dropped and the write would + // land at exit 0, which is worse than having no flag at all. + case '--if-version': flags.ifVersionRaw = argv[i + 1]; i += 1; break; case '--since': flags.since = argv[i + 1]; i += 1; break; case '--topic': flags.topic = argv[i + 1]; i += 1; break; case '--dedupe': flags.dedupe = true; break; @@ -457,6 +477,10 @@ Update Options: --content New content --url New source URL -t, --tag Add a tag + --if-version Refuse (non-zero) if the entry moved past version n. + Pass the version you read BEFORE composing the edit; + without it the guard is this command's own re-read, + which cannot see a change that landed while you wrote. Delete Options: --id Item id @@ -474,7 +498,7 @@ function printCommandHelp(command: string): void { if (command === 'add') { console.log('Usage: knowledge add <content> [--url <url>] [-t <tag>]... [--json]\n -t/--tag is repeatable and accepts comma-separated values: -t a -t b == -t "a,b"'); return; } if (command === 'list' || command === 'ls') { console.log('Usage: knowledge list|ls [--format table|json] [-p <page>] [-l <limit>] [-s <search>] [-t <tag>]... [--sort created|title] [--desc] [--archived] [--include-archived] [--verbose] [--json]\n -s/--search is a CASE-INSENSITIVE LITERAL SUBSTRING filter over id, title and content — not a\n tokenised or semantic search, so a word order that never appears verbatim matches nothing. It\n resolves an item by its slug because the id is included. For meaning-based lookup use `knowledge\n search <query>`, which is a different index and will find items this filter cannot.\n -t/--tag is repeatable and accepts comma-separated values; repeated -t narrows (an item must carry every tag).\n Each value matches an item carrying the whole value OR all of its comma-split names — a union, so\n `-t "a,b,c"` finds items carrying a legacy literal "a,b,c" tag as well as items carrying the three\n names separately. (`untag` differs on purpose: it stops at the whole-value match.)\n Use --json to tell those two shapes apart; the table renders them near-identically.\n Archived items are excluded by default; add --include-archived to sweep both.\n If both --archived and --include-archived are passed, --archived wins (archived items only).'); return; } if (command === 'get') { console.log('Usage: knowledge get --id <id> [--json]'); return; } - if (command === 'update' || command === 'edit') { console.log('Usage: knowledge update|edit --id <id> [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--json]\n -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced.\n With -t the output reports how many tags were actually added, so 0 added is not read as 3.'); return; } + if (command === 'update' || command === 'edit') { console.log('Usage: knowledge update|edit --id <id> [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--if-version <n>] [--json]\n -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced.\n With -t the output reports how many tags were actually added, so 0 added is not read as 3.\n --if-version <n> refuses the write, non-zero, if the entry has moved past version n.\n Pass the version you read BEFORE composing this edit (knowledge get --id <id> --json).\n Without it the guard is the version this command re-reads for itself, which cannot\n see an edit that landed while you were composing, so a stale write still wins.\n On conflict, re-read the entry, reconcile against what changed, and re-run — there is\n deliberately no automatic retry.'); return; } if (command === 'archive') { console.log('Usage: knowledge archive --id <id> [--json]'); return; } if (command === 'restore' || command === 'unarchive') { console.log('Usage: knowledge restore|unarchive --id <id> [--json]'); return; } if (command === 'upsert') { console.log('Usage: knowledge upsert [title] [content] [--id <id>] [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--json]\n -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced.\n With -t the output reports how many tags were actually added, on both the create and update paths.'); return; } @@ -834,6 +858,45 @@ function requireId(flags: Flags): asserts flags is Flags & { id: string } { if (!flags.id) throw new Error('Missing required --id. Example: knowledge get --id <id>'); } +/** + * Validate `--if-version` and confirm the resolved store can actually enforce + * it. Returns undefined when the flag was not passed, which leaves the existing + * read-then-write default in place for every installed caller. + * + * Both refusals below exist because the alternative is a guard that cannot + * fail. `Number('abc')` is NaN and would serialise into the If-Match header as + * `null`, so the server would see no guard and accept the write at exit 0 — the + * caller asked for protection and silently received none. The local JSON store + * is the same failure with a different cause: its `update` takes no options at + * all, so `expectedVersion` is dropped on the floor and every `--if-version` + * against it would report success while enforcing nothing. + */ +function parseIfVersion(flags: Flags, store: ItemStore): number | undefined { + const raw = flags.ifVersionRaw; + if (raw === undefined) return undefined; + + const parsed = Number(raw); + if (!Number.isInteger(parsed) || parsed < 1) { + throw new Error( + `Invalid --if-version value ${JSON.stringify(raw)}. It must be a positive whole number — ` + + 'the version you read before composing this edit, as reported by ' + + '`knowledge get --id <id> --json` or `knowledge versions --id <id> --json`.', + ); + } + + if (!store.supportsVersions) { + throw new Error( + '--if-version cannot be honoured by the local JSON knowledge store. It keeps no version ' + + 'line, so the guard would be accepted and enforced nowhere — reporting success while ' + + 'another writer is overwritten is worse than refusing. Optimistic concurrency lives in ' + + 'the Postgres-backed store: point this CLI at it (HASNA_KNOWLEDGE_STORAGE_MODE=postgres ' + + 'plus the API url/key) and re-run.', + ); + } + + return parsed; +} + function sortItems(items: KnowledgeItem[], flags: Flags): { sorted: KnowledgeItem[]; sort: string; direction: string } { const sort = flags.sort ?? 'created'; if (sort !== 'created' && sort !== 'title') { @@ -2116,7 +2179,18 @@ async function run(argv: string[]): Promise<void> { // versions; there is deliberately NO automatic retry, because re-applying // without comparing the fields that moved is how you overwrite a colleague // while believing you handled the conflict. - const item = await itemStore.update(current.id, patch, { expectedVersion: current.version }); + // + // That default is necessary and NOT sufficient, which is why --if-version + // exists. `current.version` is read microseconds before the write, so it + // only ever closes the gap inside this one invocation. The race that loses + // real edits is wider than one invocation: an agent reads an entry, spends + // minutes composing a body against what it read, and by the time it runs + // `update` this re-read has already absorbed the other writer's change. The + // guard then matches, the write is accepted, and the earlier edit is gone at + // exit 0. A guard derived from the write cannot detect staleness in the + // read that produced the content — only the caller's own version can. + const expectedVersion = parseIfVersion(flags, itemStore); + const item = await itemStore.update(current.id, patch, { expectedVersion: expectedVersion ?? current.version }); // When -t was asked for, report how many tags were actually added. Without this, // "added 3" and "added none, they were all already there" both print `Updated <id>` // at exit 0 and carry the count nowhere — not in `message`, not in JSON — the same diff --git a/tests/entry-versioning-client.test.ts b/tests/entry-versioning-client.test.ts index cba4750..b9efde8 100644 --- a/tests/entry-versioning-client.test.ts +++ b/tests/entry-versioning-client.test.ts @@ -15,7 +15,7 @@ * bodies. */ import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; -import { mkdtempSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { ApiKeyStore, mintApiKey, verifyApiKey } from '@hasna/contracts/auth'; @@ -295,3 +295,139 @@ describe('ItemStore (local transport) — a store with no history says so', () = expect(message).toContain('HASNA_KNOWLEDGE_STORAGE_MODE=postgres'); }); }); + +// --------------------------------------------------------------------------- +// `update --if-version` — the guard the CLI's own re-read cannot provide. +// +// The update command already passes `expectedVersion: current.version`, but that +// number comes from a read the CLI performs microseconds before its own write. +// It therefore guards only the gap inside one invocation. The race that actually +// loses fleet edits is wider: an agent reads an entry, spends minutes composing a +// new body, and then types `knowledge update --content <composed>`. The CLI +// re-reads, sees whatever landed in the meantime, passes THAT as the guard, and +// the write is accepted — so a second writer holding a stale read still clobbers +// the first at exit 0. The version the agent actually read is never expressed, +// and a guard derived from the write itself cannot express it. +// +// `--if-version <N>` is how the caller supplies the version IT read. Omitted, +// behaviour is unchanged, because many installed callers pass nothing. +// --------------------------------------------------------------------------- + +describe('knowledge update --if-version — caller-supplied concurrency guard', () => { + test('a writer holding a stale read is REFUSED instead of silently clobbering', async () => { + const store = cloudStore(); + const created = await store.create({ title: 'Contended entry', content: 'BASE-LINE-ZERO' }); + const cliEnv = cloudEnv as Record<string, string>; + + // Both agents read the same version — the read an agent really performs, + // well before it composes an edit. + const readA = await store.get(created.id); + const readB = await store.get(created.id); + expect(readA!.version).toBe(1); + expect(readB!.version).toBe(1); + + const a = await runCli( + ['update', '--id', created.id, '--content', 'BASE-LINE-ZERO\nAAA', '--if-version', String(readA!.version), '--json'], + cliEnv, + ); + expect(a.exitCode).toBe(0); + + // B still holds version 1 while the stored entry is at 2. + const b = await runCli( + ['update', '--id', created.id, '--content', 'BASE-LINE-ZERO\nBBB', '--if-version', String(readB!.version), '--json'], + cliEnv, + ); + expect(b.exitCode).not.toBe(0); + expect(b.stderr).toContain('version_conflict'); + // Both numbers must be named, or the operator cannot tell what to re-read. + expect(b.stderr).toContain('version 1'); + expect(b.stderr).toContain('version 2'); + + // The decisive assertion: A's edit survived and B's never landed. + const after = await store.get(created.id); + expect(after!.version).toBe(2); + expect(after!.content).toContain('AAA'); + expect(after!.content).not.toContain('BBB'); + }, 60_000); + + test('a matching --if-version is accepted, so the guard is not simply always-on', async () => { + // Positive control for the test above: same flag, same path, a current + // version instead of a stale one, and the write must land. + const store = cloudStore(); + const created = await store.create({ title: 'Uncontended entry', content: 'first' }); + const cliEnv = cloudEnv as Record<string, string>; + + const result = await runCli( + ['update', '--id', created.id, '--content', 'second', '--if-version', String(created.version), '--json'], + cliEnv, + ); + expect(result.exitCode).toBe(0); + + const after = await store.get(created.id); + expect(after!.version).toBe(2); + expect(after!.content).toBe('second'); + }, 60_000); + + test('OMITTING --if-version leaves existing callers working exactly as before', async () => { + // Back-compat is the reason the flag is opt-in. Many installed callers pass + // nothing, and this asserts they are not broken by the addition. + const store = cloudStore(); + const created = await store.create({ title: 'Unguarded entry', content: 'first' }); + const cliEnv = cloudEnv as Record<string, string>; + + const result = await runCli(['update', '--id', created.id, '--content', 'second', '--json'], cliEnv); + expect(result.exitCode).toBe(0); + + const after = await store.get(created.id); + expect(after!.version).toBe(2); + expect(after!.content).toBe('second'); + }, 60_000); + + test('a non-numeric --if-version is rejected before anything is written', async () => { + const store = cloudStore(); + const created = await store.create({ title: 'Bad guard', content: 'untouched' }); + const cliEnv = cloudEnv as Record<string, string>; + + const result = await runCli( + ['update', '--id', created.id, '--content', 'clobbered', '--if-version', 'abc', '--json'], + cliEnv, + ); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain('--if-version'); + // Without this the test passes while the flag does not exist at all, because + // `Unknown flag: --if-version` also contains the string above. It would then + // be a check that cannot fail. + expect(result.stderr).not.toContain('Unknown flag'); + + // A guard that parsed to NaN and then wrote anyway would be worse than none. + const after = await store.get(created.id); + expect(after!.content).toBe('untouched'); + expect(after!.version).toBe(1); + }, 60_000); + + test('--if-version against the local JSON store REFUSES rather than passing vacuously', async () => { + // The local store drops expectedVersion on the floor. Accepting the flag + // there would return exit 0 while enforcing nothing — a guard that cannot + // fail, which is the failure mode this whole file exists to prevent. + const dir = mkdtempSync(join(tmpdir(), 'ok-ifversion-local-')); + const path = join(dir, 'db.json'); + writeFileSync( + path, + JSON.stringify({ + items: [{ id: 'k_local', short_id: 'local', title: 'T', content: 'c', url: null, tags: [], created_at: 'x', updated_at: 'x' }], + }), + ); + + const result = await runCli(['update', '--id', 'k_local', '--content', 'new', '--if-version', '1', '--store', path, '--json']); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain('--if-version'); + // Same discriminator as above: an unparsed flag would satisfy the assertion + // above without the store ever refusing anything. + expect(result.stderr).not.toContain('Unknown flag'); + // Name the way out, exactly as VersionHistoryUnsupportedError does. + expect(result.stderr).toContain('HASNA_KNOWLEDGE_STORAGE_MODE=postgres'); + + // And the write must not have happened behind the refusal. + expect(JSON.parse(readFileSync(path, 'utf8')).items[0].content).toBe('c'); + }, 60_000); +});