From 9157c6125bda3c015c443eea92d63a2fe016fdcc Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Mon, 3 Aug 2026 07:57:02 +0300 Subject: [PATCH] feat(update): add --if-version so the CALLER's read guards the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update` already passed `expectedVersion: current.version`, but that number comes from a read the command performs microseconds before its own write, so it only ever closes the gap inside one invocation. The race that actually loses fleet edits is wider than one invocation. An agent reads an entry, spends minutes composing a body against what it read, and then runs `update --content `. The command 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. A guard derived from the write cannot detect staleness in the read that produced the content; only the caller's own version can. `--if-version ` lets the caller supply the version it read. Omitted, behaviour is byte-identical for every installed caller, which is asserted. Two refusals exist so the flag cannot become a guard that silently enforces nothing — the failure mode that is worse than having no flag: - A non-numeric value is rejected before any write. `Number('abc')` is NaN and serialises into the If-Match header as `null`, which the server reads as no guard at all, so the caller would ask for protection and get none at exit 0. - `--if-version` against the local JSON store is refused, naming the way to a store that can honour it. That store's `update` takes no options, so `expectedVersion` is dropped on the floor and every such call would report success while enforcing nothing. Tests (tests/entry-versioning-client.test.ts, real CLI process against a live loopback server on real Postgres): a stale writer is refused and the earlier edit survives; a current version is accepted, so the guard is not always-on; omitting the flag still writes; a bad value writes nothing; the local store refuses. The two refusal tests additionally assert stderr does NOT contain "Unknown flag", because without that they pass while the flag does not exist. Agent: Silvanus --- src/cli.ts | 78 ++++++++++++++- tests/entry-versioning-client.test.ts | 138 +++++++++++++++++++++++++- 2 files changed, 213 insertions(+), 3 deletions(-) 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); +});