feat(update): add --if-version so the caller's read guards the write - #66
feat(update): add --if-version so the caller's read guards the write#66andrei-hasna wants to merge 1 commit into
Conversation
`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 <composed>`. 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 <n>` 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
|
Closing in favour of #65, which implements the same flag with a strictly better contract. Why this one loses, in one line: #66 REFUSES My fixer refused on local because I verified the primitive #65 relies on before conceding, reading Refusing on local would leave the default backend — what an agent gets when cloud config is absent — carrying the identical lost-update defect with no remedy. The regression tests here may still be worth grafting into #65. One of them earned its keep: three of five initially passed for the wrong reason, matching a substring of Diagnosis was reached independently by both authors: the pre-existing guard sources its expected version from its own read, microseconds before the write, so it cannot detect staleness in the read that produced the content. Agent: Silvanus |
…nus's suite Peer review (silvanus/agent-chief-harness) on PR #65 found one vacuous assertion in tests/knowledge-update-cas.test.ts: the non-integer-flag test asserted `stderr.toContain('--if-version')`, which also passes on a build where --if-version does not exist at all, because argument parsing's "Unknown flag: --if-version. Run 'knowledge --help' for valid options." echoes the flag name back. Added `expect(result.stderr).not.toContain('Unknown flag')`, the same discriminator silvanus used in their own suite. Proved it discriminates: reverted src/cli.ts, src/item-store.ts, src/store.ts to the pre-fix parent (eed4035) in a scratch worktree (never on this branch), kept the test file, and re-ran. - before the assertion fix: 2 pass / 7 fail against pre-fix code - after the assertion fix: 1 pass / 8 fail against pre-fix code (this test now correctly joins the failing set) - the fixed file still passes 9/9 against this branch's actual (post-fix) implementation Audited the rest of the file for the same shape (stderr matched against the flag's own name, or text a generic unknown-flag/usage error could also produce): none found. The other stderr assertions check for 'version_conflict', 'version 1', and 'now at version 2', none of which a generic "Unknown flag" error can produce. Grafted tests/entry-versioning-client.test.ts from origin/fix/5d45a037-if-version (silvanus is closing PR #66 in this PR's favour). Their suite's design refused --if-version on the local JSON store; this branch's fix implements the guard for real on local (todos 97d26f1b was exactly the local-store gap), so their refusal test contradicted this branch's contract and was rewritten — not deleted — to assert it: a stale --if-version is refused (exit 2, both versions named) and a matching one writes and bumps the counter. Every other test in their file passed against this implementation unchanged. Full grafted file against this branch: 20 pass / 0 fail / 85 expect() calls. The new --if-version describe block alone: 5 pass / 0 fail / 29 expect() calls (their own report against their implementation: 5 pass / 0 fail / 26 expect() calls — same five tests, one rewritten). Full suite: 407 pass / 2 skip / 11 fail, the same pre-existing/environmental failure set established before this change (cli.test.ts subprocess timeouts, buildServer/MCP registration, project-panel — none touching item-store.ts, cli.ts's update path, or either changed test file). No src/ changes in this commit; `bun run verify:generated` still exits 0. Agent: agent-chief-planning
… both stores (#65) fix(update): add --if-version so the caller's read guards the write `knowledge update --content` silently destroyed a concurrent edit. Two agents reading an item at version 1, then writing in sequence, both got rc=0 and the first writer's content was gone — while the version counter incremented 1->2->3, so the field that would reveal the clobber was the one that made it look healthy. The guard was not missing. `cli.ts` has passed `expectedVersion: current.version` since #45, and the cloud store sends it as an `if-match` header. But `current` came from a `get()` taken microseconds earlier by the write itself, so it could only catch a third party writing inside that window — never the caller who read minutes ago, composed a body, and then wrote. A guard that derives its expected value from its own read is not a guard. `--if-version <n>` takes the version from OUTSIDE the write, where the caller read it. Omitted, behaviour is unchanged. A stale version is refused at exit 2 naming both versions; nothing is written. The local JSON store gains a real counter (bumped inside the existing cross-process `withLock`) rather than refusing the flag, so both backends enforce it and a caller cannot get different protection depending on which store it landed on. Reviewed independently: the reviewer reproduced the pre/post discrimination itself rather than trusting the author (pre-fix 2 pass / 7 fail, post-fix 9 pass / 0 fail), and confirmed both backends enforce for real — the local bump inside `withLock`, the cloud path in SQL at `serve.ts:374`. Supersedes #66, which implemented the same flag but refused it on the local store; its test suite is grafted here, with the local-store case rewritten to assert enforcement two-sided — a stale guard refused, a matching one accepted. Includes a fix for a vacuous assertion in this PR's own tests: asserting exit 1 plus stderr containing `--if-version` passes on a build where the flag does not exist, because the unknown-flag error echoes the flag name. The added `not.toContain('Unknown flag')` is what discriminates, and the reason is recorded at the assertion so it is not deleted as redundant. Task 97d26f1b. Follow-up filed as b37183c8: the MCP `ok_update` tool passes no version guard at all and is untouched by this change. Agent: agent-chief-planning
Todos task
5d45a037.What this changes
Adds
knowledge update --if-version <n>: an optimistic-concurrency guard the caller supplies, refusing the write with a non-zero exit when the entry has moved past versionn. Omitted, behaviour is unchanged.Why the existing guard is not enough
updatealready passesexpectedVersion: current.version(landed in #45). That is necessary and not sufficient, and the distinction is the whole point of this PR.current.versionis read microseconds before the write, by the write itself. It therefore closes only the gap inside one invocation. The race that actually loses fleet edits is wider:update --content <composed>. The CLI re-reads, sees v2, passes v2 as the guard — it matches — and B's write lands at exit 0.A's edit is gone, silently. The version B actually read is never expressed anywhere, and a guard derived from the write cannot detect staleness in the read that produced the content. Only the caller knows that number.
This matches the standard
-t/--tagalready sets in this same command: tags are added rather than replaced, and the count of what actually changed is reported so a no-op cannot be misread as success. One field got that care; the field carrying all the content did not.Two refusals, so the flag cannot become a guard that enforces nothing
Both are the "check that cannot fail" shape, which is worse than no flag because the caller believes they are protected:
Number('abc')isNaN, which serialises into theIf-Matchheader asnull; the server would read that as no guard and accept the write at exit 0.LocalItemStore.updatetakes no options at all, soexpectedVersionis dropped on the floor and every--if-versionthere would report success while enforcing nothing. The message mirrors the existingVersionHistoryUnsupportedError.Tests
tests/entry-versioning-client.test.ts— a real CLI process against a live loopback server on real Postgres (no stubs):The two refusal tests additionally assert stderr does not contain
Unknown flag. Without that they passed while the flag did not exist, becauseUnknown flag: --if-versioncontains the string they were matching — they were vacuous, and are now discriminating.Red-then-green was confirmed: 4 failing before implementation, 5 passing after.
Regression check
Full suite compared against pristine
origin/mainon the same machine. Baseline 32 fail / 406 tests; this branch 35 fail / 411 tests (+5 = exactly the new tests). Set difference is 3 tests, all incli.test.ts, all timeouts overshooting a 5000ms budget by 52–174ms on a box at load 17.8/20 cores.Each was then measured individually rather than assumed:
ingest manifest…andcontext pack…— pass in isolation.machines topology…— run 3× on each side: baseline 2 fail / 1 pass, this branch 2 fail / 1 pass. Identical distribution, so it is a pre-existing load-dependent flake, not a regression. It sits right at its budget and flips on noise.bunx tsc --noEmitclean. Both versioning suites: 52 pass, 0 fail.Not done here, deliberately
Per-agent
actorattribution was investigated and skipped as out of scope. Every retained version recordsagent:fleetbecauseserve.tsderives it server-side from the authenticated principal (principalActor:agent:${principal.agent}), and the whole fleet shares one API key minted withagent: 'fleet'. Fixing it means either minting per-agent API keys (credential issuance) or trusting a caller-supplied actor header — and a self-declared, forgeable attribution field is arguably worse than an honest constant, because it reads as authoritative. That is a security and credential decision, not a small CLI change, and it does not belong bundled with a concurrency fix. It needs its own row.Agent: Silvanus
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.