Skip to content

fix(update): add --if-version CAS guard, close the lost-update gap on both stores - #65

Merged
andrei-hasna merged 3 commits into
mainfrom
fix/97d26f1b-knowledge-update-cas
Aug 3, 2026
Merged

fix(update): add --if-version CAS guard, close the lost-update gap on both stores#65
andrei-hasna merged 3 commits into
mainfrom
fix/97d26f1b-knowledge-update-cas

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a measured P1 data-loss defect (todos 97d26f1b): knowledge update --content was an unconditional whole-content overwrite, and a write computed from a stale read could silently destroy an intervening write from another agent.

  • Root cause 1 — local store had no version tracking at all. LocalItemStore.update ignored expectedVersion (didn't even accept the parameter), and local items carried no version field, so any concurrency guard was a no-op on the on-box JSON store.
  • Root cause 2 — the CLI's existing auto-guard only protects one invocation's own internal read-then-write window. update already re-reads the item and sends expectedVersion: current.version, but that re-read happens fresh at write time, so it can never catch a decision an agent made from an earlier, separate get — by write time the freshly re-read version already reflects any intervening edit, so the guard trivially "passes" against itself.

Fix

  • LocalItemStore now tracks a lock-protected version counter (bumped on every successful write, check-and-write inside one file-lock acquisition). This is distinct from version historysupportsVersions stays false, no prior bodies are retained; that's still Postgres-only.
  • knowledge update gains an optional --if-version <n> flag. Pass the version a prior get returned; the write is rejected (nothing written) if the store has moved to a different version since. Omitting it preserves the exact previous behaviour on both backends.
  • A version conflict (KnowledgeVersionConflictError, already used by the cloud/Postgres path) is now surfaced as a distinct exit code (2), separate from the CLI's generic error exit (1), with code/expected/current fields in --json output.

Regression test

tests/knowledge-update-cas.test.ts reproduces the exact repro from the bug report (two reads at the same version, two writes, second clobbers first) at both the ItemStore layer (fast, in-process) and the CLI layer (real subprocess, real local JSON file), plus:

  • a legacy-item case (no version field yet) is read as version 1, not silently unguardable
  • a positive control (fresh version accepted)
  • backward-compat (omitting --if-version still works)
  • flag validation (non-integer --if-version rejected before touching the store)
  • an isolation test: an explicit --store plus a fully-selected postgres mode pointed at a guard-refused non-loopback host still resolves local, and the default store location is never created

Confirmed failing (7/9) against the unfixed store before the fix, passing (9/9) after.

Test plan

  • Regression test written first, confirmed failing against the unfixed code (created.version undefined, no KnowledgeVersionConflictError thrown, clobber not prevented)
  • Same test passing after the fix (9/9)
  • Full suite run (bun test) with ambient HASNA_KNOWLEDGE_* vars unset: 400 pass / 2 skip / 13 fail — all 13 failures reproduce on a pristine pre-fix baseline run under the same conditions (11 baseline fails, same test names; the 2 extra are cli.test.ts/smoke-script timeouts under full-suite CPU contention, confirmed passing when run in isolation)
  • tsc --noEmit clean
  • Directly related test files (entry-versioning*.test.ts, cloud-store.test.ts, knowledge-mode.test.ts, serve.test.ts) all green (99/99) run in isolation
  • Isolation constraint honoured: every test either drives ItemStore directly with storePathOverridden: true, or spawns the CLI with a fresh temp HOME + explicit --store, stripping ambient *KNOWLEDGE* env

Notes for reviewers

  • This PR is opened as a draft and is not to be merged by the author.
  • Scope is intentionally narrow: --if-version is added to update only, per the task. archive/restore/untag/upsert keep their existing auto-derived expectedVersion (unchanged behaviour, still real protection for the single-invocation race).

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

… both stores

`knowledge update --content` was an unconditional whole-content overwrite on
the local JSON store: LocalItemStore.update ignored expectedVersion entirely
(it did not even accept the parameter) and local items carried no version
field at all, so a write computed from a stale read succeeded at rc=0 and
silently destroyed an intervening write from a second agent.

The cloud/postgres store already enforced a server-side version check, but
the CLI always re-read the item's version immediately before writing, which
only guards the instant inside one command invocation — it cannot catch a
decision an agent made from an earlier, separate `get`, because by write time
the freshly re-read version already reflects any intervening edit.

Fixes both gaps:
- LocalItemStore now tracks a lock-protected version counter (distinct from
  version HISTORY, which it still does not keep; supportsVersions stays
  false) and rejects a write via KnowledgeVersionConflictError when
  expectedVersion is supplied and stale.
- `knowledge update` gains an optional `--if-version <n>` flag: pass the
  version a prior `get` returned, and the write is rejected unless the store
  is still at exactly that version. Omit it and behaviour is unchanged.
- A version conflict is now a distinct exit code (2) from the CLI's generic
  error exit (1), with `code`/`expected`/`current` in --json output, so a
  caller can detect the guard firing without parsing prose.

Regression test: tests/knowledge-update-cas.test.ts reproduces the exact lost
update (two stale reads at the same version, two writes, second destroys
first) at both the ItemStore layer and the CLI layer, confirmed failing
against the unfixed store before this fix and passing after.

Agent: agent-chief-planning
CI (#65, run 30784802813) caught this correctly:
`bun run verify:generated` rebuilds through the package's own `build` script
and diffs the result against the committed bin/dist, and the prior commit
here edited src/store.ts, src/item-store.ts and src/cli.ts without
regenerating them.

Regenerated with `bun run build` (bun 1.3.14, matching the version pinned in
.github/workflows/ci.yml) via `bun run verify:generated`. Affects exactly the
files touched by the CAS fix's source changes:
  - dist/store.d.ts, dist/item-store.d.ts — the version/CAS doc comments and
    the new KnowledgeVersionConflictError re-export
  - dist/index.js, bin/knowledge.js, bin/knowledge-mcp.js — the bundles that
    embed item-store.ts/cli.ts

`bun run verify:generated` now exits 0 against this commit.

Agent: agent-chief-planning
…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
@andrei-hasna
andrei-hasna marked this pull request as ready for review August 3, 2026 05:37
@andrei-hasna
andrei-hasna merged commit 97fb22b into main Aug 3, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant