This reverts commit 366f9c18a, keeping the git-native knowledge base in
dev until the issues queued for the next release are fixed.
Nothing shipped by that merge was reachable in production: git-native
behaviour needs both the KNOWLEDGE_STORE_ENABLED env (default FALSE) and
a per-workspace column (default false), so reverting changes no runtime
behaviour.
Migrations 175 and 176 stay applied in production; alembic_version is
moved back to 174 by hand so this tree's upgrade is a no-op. Both
revisions only add columns, and they use ADD COLUMN IF NOT EXISTS, so
re-landing this work re-applies them harmlessly.
Phase 5 says what the migration is; doing it on production needs an
ordered list of commands and checks. Covers the merge target and why it
is main rather than a dev promotion, the deploy checks that can corrupt
data (split object-store volume) or fail quietly (worker queues, beat,
schema), the dry run, the seed, parity verification, and the batched
flip with its rollback.
Also ignore the fleet runner's report file: it lands in the backend cwd
and carries workspace ids and document paths.
Git reported a move as an unrelated removal and an addition, so the index
deleted the row and inserted a fresh one at the new path. The id is what
the rest of the schema holds: document_versions cascades from it, so the
document's entire version history went with it; an upload's stored original
cascades too, orphaning the blob; document_revisions is nulled, detaching
the audit trail; and citations already written into past answers name the
document and chunk ids, so every one of them dangles. move_file is an
ordinary agent operation and nothing warned — the sweep looked clean,
because git and postgres agreed on content.
Dulwich detects renames and tree_changes already accepts the detector, so
the engine now reports a move as one renamed change carrying both paths.
Convergence hands the row's marker over to the new path and the upsert
updates it in place. The fallback identity travels with the marker, or the
next file written at the old path would resolve to the row that left it —
except for an upload, which identifies by filename and would be duplicated
on re-upload.
The change window is one diff of the two snapshots now, rather than a fold
of every revision between them. A queued run can be several commits behind,
and folding lost the rename as soon as anything edited the file afterwards.
It also drops a path that came and went inside the window, which the fold
reported as a deletion.
A move that also rewrites the file has nothing left to match and still
arrives as a removal plus an addition; the marker guard in _delete is what
keeps that from deleting the row the same run just wrote.
The end-of-stream safety net ran on every stream, and a turn paused at an
approval gate ends its stream like any other. The helper it calls both
commits and discards, so the turn's work so far was cut into a revision of
its own and anything not yet committable was dropped — including a folder
the agent had just made, which git cannot restore, having no empty
directories. The write the approval was granted for then failed for want of
its parent, and the next approval repeated the whole cycle.
Convergence applies upserts before removals, and git reports a rename as
both. The upsert claims the row by its path marker; the removal then
resolves the old path through the unique-hash fallback — stale, since a
retitle moves the marker and leaves the hash behind — and lands on that same
row. The document vanished from the UI and from search while its file stayed
in the tree, healed only by the next full rebuild.
A note's title is re-read from its first heading on every save, so an
ordinary editor save reached the recorder looking like a retitle and renamed
whatever the agent had named. On the canary that moved a note out from under
the path the agent was holding, and its next turn re-created the file
instead of editing it.
Celery runs every task on a fresh event loop, and the cached client stayed
bound to the first one. It failed inside acquire — after redis had set the
key, before the reply was read — leaving the lock held by nobody for its
full TTL, which wedged indexing for the workspace.
Two writers name files and they disagree. The seeder and the revision recorder
derive a name from the title, always appending .xml; the agent's write_file
commits whatever name the model chose, usually .md. So deriving is a guess
about anything the agent authored, and the canary's parity check read that
guess as drift: missing=1 extra=1 for a single file that was never wrong.
Worse than the false alarm, a real seed acts on it — writing the derived name
and deleting the agent's file as an orphan.
virtual_path_of prefers the path recorded on the row and falls back to
derivation, which is the name the seeder gave every row that has no marker.
The seeder now stamps what it actually wrote, so a retitle can tell which file
to drop, and the readers that hand paths to the agent report where content
lives rather than where a title implies it should.
Retiring .xml from storage is the real repair; this makes the two views agree
without renaming a single existing file.
The canary's turn hung at the very end with no error: the model accepted the
request for the revision's subject and then never answered. Everything that
closes a turn waits behind that call — the commit, the working copy's discard,
the turn's outcome — and a stall raises nothing, so the existing except never
ran and the stream just sat there.
A deadline is what the fallback needed to be reachable, since the subject is a
nicety and losing it costs a nicer name, not a write.
The staging guards now read like their four siblings — one line naming the
reason — and the copy-id docstring keeps the invariant without retelling the
bug. 22 lines of prose the code already said.
Both set dirty_paths under is_cloud with no backend check, so kb_persistence
recorded the same write into Postgres as a legacy document git never hears
about. That masked the dropped delegated writes — the note showed up in the UI,
nothing errored — and would have become a double write once they landed.
The four pure-staging tools already branch on the backend; these two stage as a
side effect of a successful write, which is how they were missed. With all six
guarded, none of the five keys the legacy commit triggers on is reachable under
the git backend, so it is a true no-op on a flipped workspace. files stays: it
is the in-turn read cache, not a trigger.
Subagents run under a namespaced {parent}::task:{tool_call_id} thread id, so
they opened a copy of their own while the end-of-turn commit — which only knows
the parent thread — diffed a copy that never existed. FileNotFoundError, a
silent return, and the delegated write gone, with the copy leaked because the
discard was never reached. Delegation is the normal path for agent writes.
The copy belongs to the turn, not to the actor: resolving the root segment puts
every actor in one copy, which is also what keeps one turn to one revision.
The suite missed this because every test builds the copy id by hand, so none
exercised the two sides deriving it. The new test writes through the backend
with a ::task: runtime and commits with the parent id.
Neither .env.example nor any doc mentioned KNOWLEDGE_STORE_ENABLED, so
the flag was discoverable only by reading app/config. Names both keys,
and spells out the part that is easy to get wrong: this is the master
switch alone, a workspace also needs its own column set by the migration
runner, and the root has to be a shared volume because every web and
worker process reads the same history.
The seeder deliberately never reindexes: it copies bytes out of Postgres,
so the chunk index already matches what it wrote, and passing parity *is*
that assertion. But nothing wrote last_indexed_revision — the only writer
in the tree is converge.py, inside a convergence run — so a flipped
workspace was left with a NULL stamp.
The drift sweep reads NULL as never-indexed and enqueues a whole-tree
converge, which re-embeds everything an hour after the flip. That is
exactly the migration cost the adopt-don't-rebuild seed exists to avoid,
paid silently and unprompted.
Flip now stamps the store's head, read from head rather than the report's
seeded_revision, which is None on an idempotent re-seed. Unflip clears
it, so a workspace flipped back converges fully instead of trusting a
stamp from before the legacy pipeline owned its chunks.
Found by canary-flipping workspace 1 before touching the fleet. Verified
by mutation: dropping the stamp fails the two flip tests, and stamping
unconditionally fails the unflip test.
The restore guard had only its positive case: refused when the workspace
is git-backed. Nothing held it to being per-workspace, so rewriting it
as load_knowledge_store_settings().enabled would take restore away from
every workspace on the fleet the moment the kill switch goes on, and the
suite would not notice.
This is the asymmetric case test_writer_guards.py already keeps for the
editor reindex guard; the restore guard now has it too. Verified by
mutation: swapping in the global-flag form fails this test alone.
Each existing test wrote the same brand-new documents/note.md, so
_OPERATION_BY_KIND was only ever exercised for "added" and tx.remove
never ran at all. Mapping "removed" to write_file would have left the
suite green — and receipts are what the orchestrator treats as ground
truth for what the agent did.
Adds an edit, a delete, a move (which the store decomposes into the two
changes, so it covers both kinds at once), a mixed turn asserting one
receipt per change, and a contended commit carrying a removal so the
failed-receipt branch stops being add-only.
Verified by mutation: collapsing the kind map fails the four change-kind
tests while all eight originals pass, and hardcoding the failed-receipt
operation fails only the new contention case.
Every test in this file called record_markdown_files directly, so the
retitle test handed `removes` in ready-made and proved only that the
primitive honours it. The callers that have to *derive* the removal from
the row's path marker were never invoked: record_saved_document and
record_prepared_documents were both at zero coverage, taking the module
to 46%.
Drives both through the real git engine: retitle leaves one path and one
extra revision, no marker is left when nothing was recorded, a marker
outside /documents is swallowed rather than fatal, a document with no
source_markdown stays out of the batch, and neither caller lets a store
failure reach the save that already committed.
Module goes to 100% statements and branches. Verified by mutation: never
deriving the stale path, writing the marker without a revision, and
dropping the markdown-less skip each fail exactly one new test.
select_description ignored its mode argument, so every mode was told reads come
back as <document view="full"> with [n]-labelled passages, and to cite "the same
[n] you would use from search_knowledge_base". Only KBPostgresBackend renders
that. On a git-native workspace the model is promised labels it never sees while
holding search ordinals in context, so the likely outcome is not a citation that
drops but one that credits the wrong source — the defect class C2 exists to kill,
and the only thing about C2 that blocks flipping a workspace.
Split by read format rather than cloud-vs-desktop, which also fixes desktop
local: it has carried the same wrong description since before the flag existed.
Raw-mode text now forbids reusing a search ordinal outright.
knowledge_store_enabled threads from the middleware stack through the subagent
dependencies to build_filesystem_mw; it already keys the compiled-graph cache,
so a flip rotates the description with the backend.
The envelope itself stays deferred to its own post-flip PR: search citations are
unaffected by the flip, and C2's remaining slices need spans that only exist
once the indexer has run.
The hourly sweep compares a stored git revision against the store's HEAD, so
both sides of its predicate come from git and it cannot see drift that lives on
the Postgres side. The daily monitor does see it, but only logged — leaving the
fix to a hand-typed reindex call, which contradicts Phase 4's own "runbook steps
get forgotten; converging systems don't" for the identical class of fault.
A drift verdict now enqueues the whole-tree converge, capped at 10 per run:
fleet-wide drift is systemic, and fanning out rebuilds would compound it rather
than fix it. The error status stays alarm-only, since a store the check could
not read is not fixed by indexing it harder and its parity fields describe
nothing.
Ceiling marked in the code: drift index_tree cannot fix — an unmarked Postgres
row with no file in the tree, i.e. a writer bypassing git — costs one rebuild
per run until a human intervenes, with the alarm persisting throughout.
index_revision promised an argument it does not take — its docstring had
to open by disclaiming it — and reindex is the same verb the legacy
per-document editor path already uses, which is the writer this phase
guards against.
Fossil's rebuild was the obvious alternative and is wrong here: the run
upserts and prunes, and document ids survive it because they reach the
browser through the Zero publication. Nothing is wiped but chunks.
The two differ only in scope, so the names say scope: index_changes
folds in what moved since the stamp, index_tree reconciles against the
whole tree and is the only one that can notice a deletion it never saw.
Celery task names are untouched; they are the wire contract.
Also corrects the umbrella plan, which described this as "wipe +
rebuild" while 04-derived-index.md said document rows are never wiped.
The core package was clean apart from two files: everything under
knowledge_store imports only itself and app.config, while indexer.py
reached into app.db, the indexing pipeline and the agents middleware.
Per ADR 0002 that file is a driven consumer, not core, and nothing in
the layout said so.
Moves it to knowledge_store/index/ as converge.py + queue.py, and the
three celery entry points to celery_tasks/knowledge_store/. Task names,
queue routes and beat entries are unchanged; only module paths move.
index/__init__.py deliberately re-exports nothing. queue.py is a
writer's last step and has to stay cheap to import, so a convenience
re-export would put the ORM and the pipeline on every save's import
path. A test pins that, and fails if the re-export comes back.
This check is what replaces reading migration reports by hand once
workspaces are flipped, so an "ok" it reports has to mean git and
Postgres actually agree. Runs against real repos and rows: a seeded
workspace reads ok, an unseeded document reads drift, an unflipped
workspace is not checked, and one failing workspace still leaves the
next one checked.
Mutation-verified: collapsing _status to a constant "ok" fails.
enqueue_index deliberately swallows broker failures so a committed save
never fails on a queue problem. Untested, that swallow makes a typo
look exactly like a broker outage: nothing enqueues and nothing
complains. Covers the dispatch, the kill switch, the non-numeric id
drop, and the swallowed connection error.
The sweep is the only recovery for indexing lost to a broker or worker,
and it was the one piece of phase 4 with no test at all. Pins the
flipped-only filter, the skip when a stamp is level or a store is
missing, the enqueue cap, and the routing split where a never-indexed
workspace goes to the rebuild task rather than the fast queue.
Both of those last two were verified by mutation: dropping the flipped
filter and inverting the routing each fail a test.
