fix(session): a deleted chat's side rows go with it, and its id is never minted again

[Broccolito]

⚠ Security-sensitive: privacy approval inheritance. Wants human review.
cross_affiliation_grants is a standing authorisation, not a record — the
first-crossing approval (DR-26) the user answered once. This PR is about a chat
inheriting one it was never shown.

The bug

create_session minted <prefix>_<N> with N = MAX(N) + 1 over the rows that
still existed. So deleting the newest chat of the day handed its id to the
next one, and emptying sessions restarted the ids at 1 outright (#51 W3's ABA).
Everything keyed by a chat id then read the deleted chat's state as the new
chat's.

The previous commit on this branch (a507667d) removes what this file keeps
under that id. It cannot reach what anything else keeps — the daemon's cached
agent, the knowledge base's per-chat selection, the renderer's chat state — and
classification_audit survives deletion by design (privacy-tiers §12.5). Closing
the id reuse is the other half.

What the WIP commit contained, and what survived

47fdd4ec was made by the campaign coordinator, not by this branch's author, and
its own message says it was never compiled or reviewed. It did compile — but
it broke five migration tests, and the allocator that its tests and docs describe
did not exist anywhere in the tree (git grep high_water returned zero hits on
the branch and on origin/main).

I kept all of it and added the missing pieces:

WIP content	Verdict
classification_audit.session_incarnation (DDL + additive ALTER from the reconcile)	Kept unchanged
LEDGER_SESSION_INCARNATION subquery on both ledger writers	Kept unchanged
NOT_DECLASSIFIED_BY_USER incarnation clause + its two shape tests	Kept unchanged
The five doc files	Kept unchanged — they already described the allocator correctly; this PR makes them true
Guarding classification_audit.session_incarnation in the backfill arm	Kept, and completed — see below
The high-water allocator its tests and docs assume	Did not exist. Written here.

The WIP's commit message is not carried into history — it is replaced by one
that describes what actually landed. 61e5b45b and a507667d, the author's two
finished commits, are untouched.

The defect the WIP shipped

The new guard names two columns. The WIP added the missing one on
classification_audit and missed the one on sessions. sessions.incarnation
arrives from ensure_session_incarnation_schema, in the reconcile — which runs
after the numbered migration arms that the backfill runs from. So every
database upgrading from schema ≤ 17 reached the backfill without that column and
died at startup with no such column: sessions.incarnation.

No fresh-database test can see this; the five that caught it are migration tests.
The backfill now calls the idempotent helper, exactly as it already did for the
ledger's own column.

Where the high-water mark lives, and why nothing can reset it

session_id_high_water(prefix TEXT PRIMARY KEY, last_n INTEGER NOT NULL) — a
table in sessions.db, one row per id prefix, holding the largest N ever
minted under it.

It survives each of the three paths the tests name because nothing lowers it:

• A delete. delete_session names the tables it clears one by one; this is
  not among them.
• A History reset. clear_all_sessions likewise, and deliberately — it sits
  beside the existing comment explaining why the AUTOINCREMENT counters are left
  standing. A reset that lowered it would hand the first chat afterwards the id
  of one the user had just wiped.
• A reopen. It is on disk, and ensure_session_id_high_water_schema runs
  from reconcile_loop_schema on every open.

CLAIM_NEXT_SESSION_N is one upsert returning MAX(mark, surviving MAX(N)) + 1,
so the mark is a floor under the old rule, never a ceiling: an id another
writer has already put on disk can never be minted twice. It is the transaction's
first statement, so racing creators serialise on that row rather than on a read
that must later upgrade its lock. SEED_SESSION_ID_HIGH_WATER runs from the
reconcile on every open, which closes the one-mint window on upgrade (a database
at _1..5 with no mark would otherwise let its first delete lower MAX(N)
before anything had recorded 5) and keeps the mark abreast of an older build
sharing the file.

The mark is defence in depth, not the barrier. A build without it, a restored
backup or a hand-edited database can still hand an id out twice, so every
existing guard still has to hold without it. That is why the fixtures below force
the reuse rather than assuming it is gone.

Fail-before evidence

Baseline is 47fdd4ec (the WIP as committed), cargo test -p biorouter --lib -- session:: → 273 passed, 7 failed.

Behaviour	Test	Before	After
A deleted chat's id goes to the next chat	deleting_the_newest_chat_does_not_hand_its_id_to_the_next_one	FAILED	ok
A reset + reopen restarts the ids	neither_a_history_reset_nor_a_reopen_restarts_the_ids	FAILED	ok
Upgrade dies on the guard's other column	5 migration tests, all no such column: sessions.incarnation	FAILED	ok
A reissued id inherits grant / recall text / checkpoint	a_reissued_id_inherits_no_grant_no_recall_text_and_no_checkpoint	FAILED (see below)	ok
An id only a leftover checkpoint row names is reissued (first-upgrade window)	the_mark_rises_above_an_id_that_only_a_leftover_checkpoint_names	FAILED — 56bc72ab_1 minted twice	ok

The five migration tests were migrates_v7_db_to_v8_external_key,
migrates_v15_db_to_v16_message_blobs,
migration_11_preserves_unknown_model_history_and_is_idempotent,
migration_12_adds_nullable_accounting_columns_and_is_idempotent and
migration_15_backfills_fts_index.

Two tests on the branch pass before and after, and should not be read as
evidence — I would rather say so than have a reviewer discover it:

• the_next_id_is_above_every_id_an_older_build_minted — MAX(N) + 1 over
  surviving rows already yields 8 there. It is a real guard (it goes red if the
  mark is implemented as a ceiling, ignoring rows another writer added), but it
  is not a fail-before.
• a_deleted_chats_subagent_runs_are_kept_and_never_adopted — it passed
  vacuously: the subagent's own row survived the parent's delete and kept
  MAX(N) high enough that the id did not come back anyway.

The inheritance test

⚠ Scope of this claim, stated precisely: a reissued id inherits nothing inside
sessions.db. It is not a whole-machine claim, and an earlier draft of this
section overclaimed it. Two windows outside the database remain open — see
"Two windows outside sessions.db" below.

a_reissued_id_inherits_no_grant_no_recall_text_and_no_checkpoint is the
whole-chat statement the per-table tests cannot make between them — each of those
passes while the chat as a whole still carries over. One chat is given a
cross_affiliation_grants row, messages_fts text and a checkpoint; all three
are asserted present; the chat is deleted; the id is forced back; and then:

• no grant, twice over — the row is gone, and grant::is_granted, the
  function every gate actually calls, answers false;
• no recall text — the row is gone, and an FTS MATCH for the text finds
  nothing anywhere in the file, so nothing is left for a later reader or a backup;
• no checkpoints.

What that fail-before actually measured is worth a reviewer's attention. With
delete_chat_side_rows disabled, the test fails on the is_granted
assertion — not on a row count. GRANT_IS_THE_CHATS_OWN compares
datetime(g.granted_at) >= datetime(s.created_at), which is second-granular, so
a grant recorded in the same second the replacement chat is created ties, and
a tie reads as "the chat's own". The sibling test in privacy::grant backdates
its fixture by an hour precisely to avoid that; a real user deleting a chat and
starting another needs no backdating. So the read-time guard alone does not
close this — deleting the row is what does, and the guard is the second line for
rows this build never saw. That is recorded on the test.

Scope of the guarantee, and two windows outside sessions.db

Raised by an independent review; both are real and neither is closed by this PR.

The mark is database-scoped, deliberately, and that is the strongest scope
available. create_schema — the fresh-database path — creates the table and
does not run the seed. That is correct in isolation: the seed reads
sessions, which create_schema has just created empty, so calling it there
would insert zero rows. Adding the call would be a no-op, so "seed on the fresh
path too" does not by itself buy anything.

Moving the mark outside the database does not help either, and would cost the
property that makes it trustworthy. The claim is made in the same transaction
as the row it names, so a rolled-back create_session un-claims its id. A mark
in a sibling file could not be transactional with that insert — it would
over-claim on every rollback — and it would be lost by the same rm that loses
the database, while additionally being a file an agent with developer__shell
can edit (the argument the privacy master switch already records). So the honest
guarantee is: an id is minted once per session database.

One of the two windows is now closed. The seed reads checkpoints as well as
sessions. An id an older build minted and deleted has no sessions row, so
sessions alone could not raise the mark above it and the next chat took it back
— inheriting the shadow repository at <data>/checkpoints/<id>/, the one piece of
that chat's state no per-table delete reaches. Its checkpoints rows do
outlive it (that is the leak the startup sweep exists to clean up), so they are
evidence the id was minted. This works only because the seed runs before
retire_side_rows_of_deleted_chats retires those rows, and that ordering is now
pinned by a test. Once raised, the mark stays raised.

messages_fts and cross_affiliation_grants are deliberately not read by the
seed, for a plain reason rather than a principled one: both are created later in
the same reconcile, so at that point they need not exist.

What remains open. Per-id state outside sessions.db, blocked only by the
allocator and deleted by nothing:

State	Crosses when
<knowledge root>/.active-kb-sessions/<sha256(id)> (a chat's primary KB — its write target)	sessions.db lost/recreated; or the first-upgrade window for an id that left no checkpoint row
<knowledge root>/.hidden-kb-sessions/<sha256(id)> (a chat's visible KB set)	same
<data>/checkpoints/<id>/ left by an older build's delete	closed by the checkpoints seed above

Why the KB files are not closed here, and what closes them. They are
reachable — biorouter depends on biorouter-mcp, and paths::primary_kb_sessions_dir,
paths::hidden_kb_sessions_dir and raw::hash_bytes are all pub — so the fix
is a post-commit removal in delete_session mirroring
remove_checkpoint_repository. It is a follow-up rather than a late addition
here because it writes into the knowledge tree from the session store and wants
its own tests; that is a layering decision, not a one-liner. A directory sweep
for orphaned checkpoints/<id>/ is explicitly not the answer either: removing
every directory whose id has no row would, on a lost or fresh sessions.db,
delete every checkpoint snapshot on the machine. The safe form drives removal
from the ids whose rows the sweep actually just retired.

Sequencing

#264 must merge before #267. PR #267 (biorouter web's reach gate) keys its
gate on a session id, so until this allocator lands a reissued id defeats
that gate.

The test seam, and why it is pub

Ten fixtures — in the biorouter lib's own tests, its
conversation_writeback_stress integration binary, and biorouter-cli — build
an ABA by deleting or wiping and then relying on the id coming back. With the
allocator it does not, so each now forces it through
SessionManager::forget_minted_session_ids_for_test, which reproduces exactly
the state a build without the mark, a restored backup or a hand-edited database
leaves behind. This keeps every guard being tested for the case that still
matters, instead of quietly going vacuous.

It is pub only because two of those tests sit outside the lib's cfg(test)
scope — one in an integration binary of this crate, one in biorouter-cli —
where a #[cfg(test)] item is invisible. It is #[doc(hidden)], named for what
it is, and the_id_reuse_seam_is_only_reached_from_tests walks
crates/*/{src,tests} and pins the five files that may name it, one row per file
saying which guard it exercises there.

Side rows: what does and does not follow a deleted chat

Everything in this table lives in sessions.db; for state outside it see the
section above.

Row	Follows the chat?	Where
messages, message_blobs	Yes	pre-existing
token_events (folded into deleted_chat_usage first)	Yes	PR #224 / F10
messages_fts	Yes	a507667d
checkpoints + the shadow git repository	Yes	a507667d
cross_affiliation_grants	Yes	a507667d, plus GRANT_IS_THE_CHATS_OWN refusing at read time
classification_audit	No — by design (privacy-tiers §12.5)	its backfill guard now matches sessions.incarnation, so an entry cannot shield a later chat under the same id
Subagent runs	No — by design	kept and never adopted; a run can hold text the user typed

Verification

Merged origin/main (19 commits) first, then:

BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter --lib -- session:: privacy::
#   → 526 passed; 0 failed
BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter --test privacy_capability --test privacy_guard_wiring
#   → 4 passed; 3 passed
cargo fmt --check                                        # clean

And the three suites this PR edits that are outside that list — each holds a
fixture that now has to force the id reuse, so none of them is incidental:

cargo test -p biorouter --lib -- agents::session_skills            # 10 passed
cargo test -p biorouter --test conversation_writeback_stress       #  9 passed
HOME=$(mktemp -d) CARGO_HOME=… RUSTUP_HOME=… \
  cargo test -p biorouter-cli --lib -- commands::term              #  5 passed

./scripts/clippy-lint.sh is red on a clean main for an unrelated reason
(send_prompt_turn / too_many_lines), owned elsewhere.

🤖 Generated with Claude Code