feat(update): add --if-version so the caller's read guards the write

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 <content>         New content
   --url <url>                 New source URL
   -t, --tag <tag>             Add a tag
+  --if-version <n>            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 <id>                   Item id
@@ -474,7 +498,7 @@ function printCommandHelp(command: string): void {
   if (command === 'add') { console.log('Usage: knowledge add <title> <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

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);
+});