feat(knowledge_store): make git the source of truth for workspace documents - #1649
Merged
Conversation
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.
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.
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.
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.
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.
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.
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.
Both lived in the write path the plan already claimed was shipped, so the plan should say what the live test found and why the suite was blind to it.
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.
…a turn 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.
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.
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.
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.
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.
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.
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.
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.
Line wrapping the formatter wants; code-quality gates on ruff format --check.
Names the 6 pre-existing failures and the 13 fixture-absent errors, so a future run can tell inherited red from its own.
|
@CREDO23 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (118)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
surfsense_backend/tests/unit/middleware/test_git_tree_tool_staging.py (1)
91-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the real unflipped cloud path in this test.
For
workspace_id=None,build_backend_resolver()returnsStateBackend;StateBackendis not aGitTreeBackend, sowrite_filestill createsdirty_paths. Passing the workspace ID withknowledge_store_enabled=Falseresolves toKBPostgresBackend, which matches the legacy “old path” that this assertion claims to cover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/tests/unit/middleware/test_git_tree_tool_staging.py` around lines 91 - 98, Update test_a_workspace_still_on_the_old_path_keeps_staging to construct the middleware with a non-null workspace_id and knowledge_store_enabled=False, so _tool resolves KBPostgresBackend through build_backend_resolver(). Keep the existing write_file invocation and dirty_paths assertion unchanged.surfsense_backend/app/routes/editor_routes.py (1)
287-326: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
title_is_explicitcan be True withoutdocument.titleever being updated.
provided_titleandtitle_is_explicit=bool(provided_title)are computed unconditionally, butdocument.titleis only reassigned inside theDocumentType.NOTEbranch. For any other document type, sendingdata["title"](which the docstring says is generically accepted) setstitle_is_explicit=Truewhiledocument.titlestays at its old value.record_saved_documentthen treats the unchanged, stale title as an explicit rename and takes the identity-resettingdoc_to_virtual_pathpath instead of the identity-preservingvirtual_path_ofpath, which can compute an incorrect canonical path for the document in the git-backed store.Either gate
title_is_explicitto the branch that actually applies the title, or applyprovided_titletodocument.titlefor all document types when present.🐛 Scope the explicit-rename signal to when the title actually changed
provided_title = data.get("title") if document.document_type == DocumentType.NOTE: # If the frontend sends a title, use it; otherwise extract from markdown new_title = provided_title ... if new_title: document.title = new_title.strip() else: document.title = "Untitled" + title_is_explicit = bool(provided_title) and document.document_type == DocumentType.NOTE🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/routes/editor_routes.py` around lines 287 - 326, Scope title_is_explicit to cases where the save flow actually applies a provided title, using the existing DocumentType.NOTE title-update logic. Ensure non-NOTE documents cannot report an explicit rename when document.title remains unchanged, so record_saved_document preserves the existing identity path.
🟡 Minor comments (6)
docs/adr/0001-git-native-knowledge-base.md-50-59 (1)
50-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDeclare the fenced block language.
Line 50 triggers MD040. Use
textfor this ASCII flow diagram.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0001-git-native-knowledge-base.md` around lines 50 - 59, Declare the fenced ASCII flow diagram as a text block by adding the text language identifier to the fence surrounding the diagram.Source: Linters/SAST tools
plans/git-native-kb/00c-shared-contract.md-19-19 (1)
19-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the leading space inside the code span.
Line 19 triggers MD038. Keep the space outside the backticks:
`(<doc_id>).xml`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/git-native-kb/00c-shared-contract.md` at line 19, Update the inline code span in the filename-rules bullet to remove the leading space while preserving the intended `(<doc_id>).xml` text and keeping the surrounding prose unchanged.Source: Linters/SAST tools
plans/git-native-kb/00-umbrella-plan.md-7-9 (1)
7-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the blank line inside the blockquote.
Line 8 triggers MD028; merge the two quoted paragraphs or remove the blank separator.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/git-native-kb/00-umbrella-plan.md` around lines 7 - 9, Remove the blank line between the two consecutive blockquoted paragraphs in the umbrella plan so the quoted content remains a single continuous block and satisfies the MD028 formatting rule.Source: Linters/SAST tools
plans/git-native-kb/05a-seed-runbook.md-295-297 (1)
295-297: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the seed runtime estimate.
“Measured in 1.5” has no unit or benchmark context, so operators cannot use the stated cost model. Replace it with a concrete measurement or remove the fragment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/git-native-kb/05a-seed-runbook.md` around lines 295 - 297, Complete the seed runtime estimate in the Cost model section by replacing “measured in 1.5” with a concrete value that includes units and benchmark context, or remove the fragment if no reliable measurement is available. Keep the surrounding description of git-object writes and metadata updates unchanged.plans/git-native-kb/05a-seed-runbook.md-331-333 (1)
331-333: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winDescribe repair cost accurately.
index_treeis a full-tree reconciliation, but the derived-index plan says unchanged content reuses cached embeddings. Replace “full re-embed” with “full-tree reconcile/re-chunk; unchanged content should reuse embeddings” so operators do not overestimate or incorrectly avoid repair.Also applies to: 361-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/git-native-kb/05a-seed-runbook.md` around lines 331 - 333, Update the migration runbook’s drift-monitor repair-cost wording in both referenced passages: replace “full re-embed” with language stating that repair performs a full-tree reconcile/re-chunk while unchanged content reuses cached embeddings. Preserve the existing warning and workspace-cap context.plans/git-native-kb/04-derived-index.md-44-47 (1)
44-47: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlign all operational references with the registered index task/API.
The Phase 4 plan and the production runbook use inconsistent names for the shipped whole-tree/index entry points, which can make manual repair commands ineffective.
plans/git-native-kb/04-derived-index.md#L44-L47: replaceindex_revisionwith the actual shipped trigger and document its current-revision semantics.plans/git-native-kb/05a-seed-runbook.md#L146-L149: use the same registered task name in the queue-routing check.plans/git-native-kb/05a-seed-runbook.md#L454-L454: use that canonical task name in the manual full-reindex instruction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/git-native-kb/04-derived-index.md` around lines 44 - 47, Align all references with the registered whole-tree index task/API: in plans/git-native-kb/04-derived-index.md lines 44-47, replace index_revision with the shipped trigger and document that it operates on the current revision; in plans/git-native-kb/05a-seed-runbook.md lines 146-149 and 454, use that same canonical registered task name for queue routing and manual full-reindex instructions.
🧹 Nitpick comments (6)
surfsense_backend/app/knowledge_store/index/converge.py (2)
461-479: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_revision_author_idignores itsrevisionargument.It reads
list_revisions(limit=1)(the tip) rather than the author ofrevision. Writers commit under the write lock, not the index lock, so HEAD can advance between theheadread in_runand this call — rows created forheadthen get attributed to a newer commit's author. Prefer resolving the author forrevisionexplicitly (or at least verify the tip id matches and fall back to the owner otherwise).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/knowledge_store/index/converge.py` around lines 461 - 479, Update _revision_author_id to resolve the author for its revision argument rather than unconditionally using list_revisions(limit=1). Match the requested revision explicitly, and fall back to the workspace owner when that revision cannot be found or read; preserve the existing warning behavior for lookup failures.
445-458: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_load_ownedmaterializes every indexer-owned document, content included.Each converge run — including the per-save incremental path that may touch one file — loads full
DocumentORM rows (withcontent/source_markdown) for the whole workspace. For large workspaces this dominates the run's memory and I/O.Consider deferring the large text columns, or loading only
(id, marker)pairs and fetching rows on demand for the paths actually in the plan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/knowledge_store/index/converge.py` around lines 445 - 458, The _load_owned function currently loads full Document rows, including large text fields, for every workspace document. Change its query to load only the identifiers and PATH_MARKER values needed to build the owned mapping, then fetch complete Document records on demand only for paths selected by the converge plan, preserving the existing marker-to-document behavior.surfsense_backend/app/knowledge_store/janitor.py (1)
21-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne bad workspace aborts the whole sweep.
A single failing
prune_working_copies(corrupt repo, permission error, worktree bookkeeping failure) propagates and leaves every remaining workspace unpruned, so abandoned copies accumulate on disk until someone notices. Isolate per workspace and keep going.♻️ Proposed change
for workspace_dir in sorted(root.iterdir()): if not workspace_dir.is_dir(): continue store = KnowledgeStore.for_workspace(workspace_dir.name) - ids = await store.prune_working_copies(older_than_seconds=older_than_seconds) + try: + ids = await store.prune_working_copies( + older_than_seconds=older_than_seconds + ) + except Exception: + logger.warning( + "Could not prune working copies for workspace %s", + workspace_dir.name, + exc_info=True, + ) + continue if ids: pruned[workspace_dir.name] = idsRequires a module-level
logger = logging.getLogger(__name__).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/knowledge_store/janitor.py` around lines 21 - 28, Update the workspace loop in the janitor sweep to catch exceptions from each workspace’s KnowledgeStore.for_workspace or prune_working_copies call, log the failure through a module-level logger, and continue processing subsequent workspaces. Preserve successful pruning and the existing pruned result mapping.surfsense_backend/tests/integration/conftest.py (1)
139-160: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_set()doesn't clear_flag_cache, so a flag read before flipping stays stale.
_setswaps_read_workspace_flagbut leaves_flag_cacheuntouched; only the fixture's own setup/teardown clear it. If a workspace's flag were read once beforeworkspace_flip(...)is called in the same test (or before a second flip within a test), the cached value from before the patch would win. All current call sites flip before any read, so this is dormant today, but it's worth hardening the shared fixture.♻️ Proposed fix
def _set(enabled: bool) -> None: async def read(workspace_id: int) -> bool: return enabled monkeypatch.setattr(ks_settings, "_read_workspace_flag", read) + ks_settings._flag_cache.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/tests/integration/conftest.py` around lines 139 - 160, Update the workspace_flip fixture’s _set function to clear ks_settings._flag_cache whenever the mocked _read_workspace_flag implementation is changed, before or alongside monkeypatch.setattr. Preserve the existing setup and teardown clears so each flip immediately invalidates previously cached workspace flags.surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/kb_persistence/middleware.py (1)
83-96: 📐 Maintainability & Code Quality | 🔵 TrivialConsider extracting
ensure_folder_hierarchyout of the agent middleware module.The rename itself is a clean mechanical change. But
surfsense_backend/app/knowledge_store/index/converge.pynow imports this helper directly fromkb_persistence/middleware.py— a legacy agent-persistence module. That ties the git-native derived index's folder-creation logic to the Postgres-first legacy write path's module, pulling agent-middleware machinery into what should be a framework-agnostic indexing core (per ADR 0002). Movingensure_folder_hierarchyto a small shared utility module would let both call sites depend on a neutral helper instead of one depending on the other's middleware internals.Also applies to: 193-198, 363-368, 813-818
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/kb_persistence/middleware.py` around lines 83 - 96, Extract ensure_folder_hierarchy from the agent middleware module into a small neutral shared utility, preserving its existing behavior and signature. Update both the middleware call sites and converge.py to import the helper from the new utility, so the framework-agnostic indexing core no longer depends on kb_persistence middleware internals.surfsense_backend/app/services/document_revision_recorder.py (1)
39-67: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPer-workspace enablement isn't enforced inside
record_markdown_filesitself.The module docstring states this write path sits "behind the per-workspace knowledge-store flag," but the function only checks the global
load_knowledge_store_settings().enabled. The per-workspace check is left to each caller's discipline (both callers in this file do check it correctly). Given how central this invariant is elsewhere in the PR (dual-writer prevention guards indocument_reindex_tasks.py, the sweep's flip-only scope,_index's worker-time re-check), consider enforcing it here too so a future caller can't accidentally bypass it.🛡️ Enforce the per-workspace flag centrally
+ if not await knowledge_store_enabled_for(workspace_id): + return None if (not files and not removes) or not load_knowledge_store_settings().enabled: return NonePlease confirm other callers of
record_markdown_files(e.g. the agent turn-commit path) also gate onknowledge_store_enabled_forbefore calling it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@surfsense_backend/app/services/document_revision_recorder.py` around lines 39 - 67, Enforce the per-workspace knowledge-store flag inside record_markdown_files before creating a transaction or writing files, using the existing knowledge_store_enabled_for helper with workspace_id alongside the global setting check. Also inspect every caller, including the agent turn-commit path, and ensure each gates on knowledge_store_enabled_for before invoking this function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plans/git-native-kb/00b-diagrams.md`:
- Around line 16-17: Remove the dashed Postgres-to-Git reindex arrow from the
diagram, leaving only the one-way Git-to-Postgres derivation flow. Preserve the
existing Git and Postgres nodes and the forward arrow unchanged.
- Around line 149-161: Update the Migrator sequence diagram to remove the IDX
reindex step for the seed revision and its chunk-set response. Show the flow as
seed commit, byte-parity verification against the existing index, marking the
seed revision as the indexing baseline, then flipping the workspace flag; retain
the rollback note that Postgres content remains until verification completes.
In `@plans/git-native-kb/05-migration.md`:
- Around line 30-39: plans/git-native-kb/05-migration.md lines 30-39: Add a
migration lock or final atomic Postgres change-watermark validation between
parity verification and cutover, blocking the flip if writes occurred after
scanning. plans/git-native-kb/05a-seed-runbook.md lines 268-272: revise the
live-seeding guidance to require quiesced concurrent writes or explicit
detection through the same lock/watermark mechanism; do not claim live seeding
is safe without that protection.
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py`:
- Around line 27-32: The cross-thread graph cache can associate Git persistence
with the wrong thread. In
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py
lines 27-32, update KnowledgeStorePersistenceMiddleware construction to resolve
thread_id from live runtime configuration or remove the captured constructor
identity; in
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/agent_cache.py
lines 127-130, include thread_id in the cache key whenever Git-native
persistence is enabled or disable cross-thread reuse for those graphs.
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/git_tree.py`:
- Around line 39-45: The workspace naming fallback in the thread-ID derivation
logic must not return the shared “thread-adhoc” identifier for missing or
parentless task IDs. Update this path to derive a unique turn/run-scoped
identifier, or reject Git-tree execution when no safe scope is available, while
preserving stable “thread-{root}” naming for valid parent-thread IDs.
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/mkdir/index.py`:
- Around line 44-46: The GitTreeBackend branch in the mkdir operation must not
silently create unpersisted empty directories. Add explicit directory metadata
or sentinel persistence so empty folders survive the turn commit; otherwise
return a clear unsupported-operation error for Git-backed workspaces, while
preserving the existing behavior for non-Git cloud backends.
In `@surfsense_backend/app/agents/chat/runtime/path_resolver.py`:
- Around line 58-66: Update to_virtual_path to normalize and validate
store_path, accepting only the documents root or paths beneath documents and
rejecting other repository roots such as .cache/state. In the convergence flow,
check each full-tree entry’s root before calling to_virtual_path and skip
non-document entries so they are not indexed as knowledge documents.
- Around line 248-253: Update build_path_index() to pre-seed all valid
PATH_MARKER paths and their document occupants before deriving fallback paths
for unmarked documents. When processing a persisted marker in the path resolver,
detect an existing occupant instead of overwriting it, preserving collision
handling so marked agent-authored paths take precedence over title-derived
paths.
In `@surfsense_backend/app/knowledge_store/engines/git.py`:
- Around line 45-47: Replace the module-level _open_working_copy_lock with a
per-instance lock initialized on GitContentEngine, and update open_working_copy
to acquire self._open_working_copy_lock. Keep the lock covering the existing
working-copy creation flow so conflicting opens of the same engine remain
serialized while different workspaces can proceed independently.
In `@surfsense_backend/app/knowledge_store/index/converge.py`:
- Around line 240-248: Update the content-loading and indexing flow around the
decode/blank checks so an existing indexed row at a tree path is removed or
cleared when the file is undecodable or blank. Ensure this invalidation occurs
before the successful stamp advances and remains effective even though the path
stays in plan.tree, preventing the upsert and _prune logic from preserving stale
content.
In `@surfsense_backend/app/knowledge_store/migrate.py`:
- Around line 85-105: Update the seed-and-orphan-removal block to run whenever
not dry_run, even when files is empty. Keep computing tracked paths and removing
every path absent from files, while allowing the write loop to perform no writes
for an empty mapping; preserve the existing transaction, revision assignment,
and exception handling around this flow.
In `@surfsense_backend/app/knowledge_store/transaction.py`:
- Around line 34-48: Update Transaction.resolve so a move destination cannot
remain in both writes and removes or silently overwrite an explicit write. When
processing each move, remove dst from the pending removals and detect or
otherwise preserve an existing explicit writes[dst] according to the
transaction’s conflict policy, while retaining the existing source resolution
and missing-path error behavior.
In `@surfsense_backend/app/knowledge_store/write_lock.py`:
- Around line 22-30: Update _workspace_lock() and workspace_index_lock() to
renew their Redis leases throughout the protected operation using
lock.extend(...) or lock.reacquire(), and propagate LockNotOwnedError
immediately if renewal or ownership is lost so the operation exits before
another worker acquires the lock. Replace the existing acknowledgement of this
weakness near the lock handling with the actual renewal and failure behavior,
preserving normal release semantics.
In `@surfsense_backend/app/services/document_revision_recorder.py`:
- Around line 70-152: Separate the PATH_MARKER metadata commit in
record_saved_document from the record_markdown_files git-write error path. If
session.commit() fails after a revision is returned, preserve and return that
revision, and report the metadata persistence failure distinctly for retry or
reconciliation instead of treating the entire recording as failed. Keep the
existing full-failure handling for errors occurring before the git revision is
committed.
- Around line 155-188: Update record_prepared_documents to validate that every
document has the same workspace_id as documents[0] before calling
build_path_index or record_markdown_files; reject mixed-workspace batches
without writing to any knowledge store.
In `@surfsense_backend/app/tasks/celery_tasks/knowledge_store/index_tasks.py`:
- Around line 103-140: Update _sweep to isolate each workspace iteration: catch
failures from KnowledgeStore.for_workspace(workspace_id).get_current_revision()
and log the workspace-specific error, then continue checking subsequent
workspaces. Preserve existing drift detection and enqueue behavior, while
ensuring a failed HEAD read does not abort the sweep.
In `@surfsense_backend/app/tasks/chat/streaming/agent/event_loop.py`:
- Around line 142-165: Move the await
knowledge_store_enabled_for(fallback_commit_workspace_id) call out of the
safety-net if condition and into its existing try block. Preserve the other
commit-gate conditions, and only run commit_turn_working_copy plus the state
update when the enablement check succeeds and returns true; ensure exceptions
from the lookup are handled by the existing _perf_log.warning path.
---
Outside diff comments:
In `@surfsense_backend/app/routes/editor_routes.py`:
- Around line 287-326: Scope title_is_explicit to cases where the save flow
actually applies a provided title, using the existing DocumentType.NOTE
title-update logic. Ensure non-NOTE documents cannot report an explicit rename
when document.title remains unchanged, so record_saved_document preserves the
existing identity path.
In `@surfsense_backend/tests/unit/middleware/test_git_tree_tool_staging.py`:
- Around line 91-98: Update test_a_workspace_still_on_the_old_path_keeps_staging
to construct the middleware with a non-null workspace_id and
knowledge_store_enabled=False, so _tool resolves KBPostgresBackend through
build_backend_resolver(). Keep the existing write_file invocation and
dirty_paths assertion unchanged.
---
Minor comments:
In `@docs/adr/0001-git-native-knowledge-base.md`:
- Around line 50-59: Declare the fenced ASCII flow diagram as a text block by
adding the text language identifier to the fence surrounding the diagram.
In `@plans/git-native-kb/00-umbrella-plan.md`:
- Around line 7-9: Remove the blank line between the two consecutive blockquoted
paragraphs in the umbrella plan so the quoted content remains a single
continuous block and satisfies the MD028 formatting rule.
In `@plans/git-native-kb/00c-shared-contract.md`:
- Line 19: Update the inline code span in the filename-rules bullet to remove
the leading space while preserving the intended `(<doc_id>).xml` text and
keeping the surrounding prose unchanged.
In `@plans/git-native-kb/04-derived-index.md`:
- Around line 44-47: Align all references with the registered whole-tree index
task/API: in plans/git-native-kb/04-derived-index.md lines 44-47, replace
index_revision with the shipped trigger and document that it operates on the
current revision; in plans/git-native-kb/05a-seed-runbook.md lines 146-149 and
454, use that same canonical registered task name for queue routing and manual
full-reindex instructions.
In `@plans/git-native-kb/05a-seed-runbook.md`:
- Around line 295-297: Complete the seed runtime estimate in the Cost model
section by replacing “measured in 1.5” with a concrete value that includes units
and benchmark context, or remove the fragment if no reliable measurement is
available. Keep the surrounding description of git-object writes and metadata
updates unchanged.
- Around line 331-333: Update the migration runbook’s drift-monitor repair-cost
wording in both referenced passages: replace “full re-embed” with language
stating that repair performs a full-tree reconcile/re-chunk while unchanged
content reuses cached embeddings. Preserve the existing warning and
workspace-cap context.
---
Nitpick comments:
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/kb_persistence/middleware.py`:
- Around line 83-96: Extract ensure_folder_hierarchy from the agent middleware
module into a small neutral shared utility, preserving its existing behavior and
signature. Update both the middleware call sites and converge.py to import the
helper from the new utility, so the framework-agnostic indexing core no longer
depends on kb_persistence middleware internals.
In `@surfsense_backend/app/knowledge_store/index/converge.py`:
- Around line 461-479: Update _revision_author_id to resolve the author for its
revision argument rather than unconditionally using list_revisions(limit=1).
Match the requested revision explicitly, and fall back to the workspace owner
when that revision cannot be found or read; preserve the existing warning
behavior for lookup failures.
- Around line 445-458: The _load_owned function currently loads full Document
rows, including large text fields, for every workspace document. Change its
query to load only the identifiers and PATH_MARKER values needed to build the
owned mapping, then fetch complete Document records on demand only for paths
selected by the converge plan, preserving the existing marker-to-document
behavior.
In `@surfsense_backend/app/knowledge_store/janitor.py`:
- Around line 21-28: Update the workspace loop in the janitor sweep to catch
exceptions from each workspace’s KnowledgeStore.for_workspace or
prune_working_copies call, log the failure through a module-level logger, and
continue processing subsequent workspaces. Preserve successful pruning and the
existing pruned result mapping.
In `@surfsense_backend/app/services/document_revision_recorder.py`:
- Around line 39-67: Enforce the per-workspace knowledge-store flag inside
record_markdown_files before creating a transaction or writing files, using the
existing knowledge_store_enabled_for helper with workspace_id alongside the
global setting check. Also inspect every caller, including the agent turn-commit
path, and ensure each gates on knowledge_store_enabled_for before invoking this
function.
In `@surfsense_backend/tests/integration/conftest.py`:
- Around line 139-160: Update the workspace_flip fixture’s _set function to
clear ks_settings._flag_cache whenever the mocked _read_workspace_flag
implementation is changed, before or alongside monkeypatch.setattr. Preserve the
existing setup and teardown clears so each flip immediately invalidates
previously cached workspace flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34842dde-e52a-421a-aa26-72ba2009e194
⛔ Files ignored due to path filters (1)
surfsense_backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
docs/adr/0001-git-native-knowledge-base.mddocs/adr/0002-knowledge-core-ports-and-adapters.mdplans/git-native-kb/00-umbrella-plan.mdplans/git-native-kb/00b-diagrams.mdplans/git-native-kb/00c-shared-contract.mdplans/git-native-kb/01-git-storage-core.mdplans/git-native-kb/02-git-working-tree-backend.mdplans/git-native-kb/03-commit-write-path.mdplans/git-native-kb/04-derived-index.mdplans/git-native-kb/05-migration.mdplans/git-native-kb/05a-seed-runbook.mdplans/git-native-kb/06-zero-projection.mdsurfsense_backend/.env.examplesurfsense_backend/.gitignoresurfsense_backend/alembic/versions/175_add_workspace_knowledge_store_flag.pysurfsense_backend/alembic/versions/176_add_derived_index_columns.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/graph/compile_graph_sync.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/kb_persistence/middleware.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/__init__.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/commit_message.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/commit_turn.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/middleware.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_tree/middleware.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/stack.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/agent_cache.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/factory.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/git_tree.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/local_folder.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/multi_root_local_folder.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/resolver.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/middleware/middleware.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/edit_file/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/mkdir/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/move_file/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/read_file/description.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/read_file/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/rm/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/rmdir/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/tools/write_file/index.pysurfsense_backend/app/agents/chat/multi_agent_chat/subagents/builtins/knowledge_base/middleware_stack.pysurfsense_backend/app/agents/chat/runtime/mention_resolver.pysurfsense_backend/app/agents/chat/runtime/path_resolver.pysurfsense_backend/app/agents/chat/runtime/references/documents/resolver.pysurfsense_backend/app/celery_app.pysurfsense_backend/app/config/__init__.pysurfsense_backend/app/db.pysurfsense_backend/app/indexing_pipeline/cache/cached_indexing.pysurfsense_backend/app/indexing_pipeline/chunk_reconciler.pysurfsense_backend/app/indexing_pipeline/document_chunker.pysurfsense_backend/app/indexing_pipeline/indexing_pipeline_service.pysurfsense_backend/app/knowledge_store/__init__.pysurfsense_backend/app/knowledge_store/engines/__init__.pysurfsense_backend/app/knowledge_store/engines/base.pysurfsense_backend/app/knowledge_store/engines/git.pysurfsense_backend/app/knowledge_store/identities.pysurfsense_backend/app/knowledge_store/index/__init__.pysurfsense_backend/app/knowledge_store/index/converge.pysurfsense_backend/app/knowledge_store/index/queue.pysurfsense_backend/app/knowledge_store/janitor.pysurfsense_backend/app/knowledge_store/migrate.pysurfsense_backend/app/knowledge_store/settings.pysurfsense_backend/app/knowledge_store/store.pysurfsense_backend/app/knowledge_store/store_path.pysurfsense_backend/app/knowledge_store/transaction.pysurfsense_backend/app/knowledge_store/write_lock.pysurfsense_backend/app/observability/metrics.pysurfsense_backend/app/routes/documents_routes.pysurfsense_backend/app/routes/editor_routes.pysurfsense_backend/app/services/document_revision_recorder.pysurfsense_backend/app/tasks/celery_tasks/document_reindex_tasks.pysurfsense_backend/app/tasks/celery_tasks/knowledge_store/__init__.pysurfsense_backend/app/tasks/celery_tasks/knowledge_store/drift_monitor_task.pysurfsense_backend/app/tasks/celery_tasks/knowledge_store/index_tasks.pysurfsense_backend/app/tasks/celery_tasks/knowledge_store/janitor_task.pysurfsense_backend/app/tasks/chat/streaming/agent/event_loop.pysurfsense_backend/pyproject.tomlsurfsense_backend/scripts/migrate_knowledge_store.pysurfsense_backend/tests/integration/conftest.pysurfsense_backend/tests/integration/knowledge_store/__init__.pysurfsense_backend/tests/integration/knowledge_store/conftest.pysurfsense_backend/tests/integration/knowledge_store/index/__init__.pysurfsense_backend/tests/integration/knowledge_store/index/test_converge.pysurfsense_backend/tests/integration/knowledge_store/index/test_drift_monitor.pysurfsense_backend/tests/integration/knowledge_store/index/test_sweep.pysurfsense_backend/tests/integration/knowledge_store/test_commit_turn.pysurfsense_backend/tests/integration/knowledge_store/test_document_recorder.pysurfsense_backend/tests/integration/knowledge_store/test_fleet_runner.pysurfsense_backend/tests/integration/knowledge_store/test_migrate.pysurfsense_backend/tests/integration/knowledge_store/test_migrate_placement.pysurfsense_backend/tests/integration/knowledge_store/test_paused_turn_keeps_its_copy.pysurfsense_backend/tests/integration/knowledge_store/test_store.pysurfsense_backend/tests/integration/knowledge_store/test_write_lock.pysurfsense_backend/tests/integration/knowledge_store/test_writer_guards.pysurfsense_backend/tests/integration/test_document_versioning.pysurfsense_backend/tests/unit/agents/new_chat/test_mention_resolver.pysurfsense_backend/tests/unit/agents/new_chat/test_path_resolver.pysurfsense_backend/tests/unit/indexing_pipeline/test_chunk_reconciler.pysurfsense_backend/tests/unit/indexing_pipeline/test_chunk_spans.pysurfsense_backend/tests/unit/indexing_pipeline/test_document_hashing.pysurfsense_backend/tests/unit/knowledge_store/__init__.pysurfsense_backend/tests/unit/knowledge_store/conftest.pysurfsense_backend/tests/unit/knowledge_store/engines/__init__.pysurfsense_backend/tests/unit/knowledge_store/engines/test_git.pysurfsense_backend/tests/unit/knowledge_store/index/__init__.pysurfsense_backend/tests/unit/knowledge_store/index/test_index_tasks.pysurfsense_backend/tests/unit/knowledge_store/index/test_queue.pysurfsense_backend/tests/unit/knowledge_store/test_janitor.pysurfsense_backend/tests/unit/knowledge_store/test_settings.pysurfsense_backend/tests/unit/knowledge_store/test_transaction.pysurfsense_backend/tests/unit/middleware/test_commit_message.pysurfsense_backend/tests/unit/middleware/test_filesystem_backends.pysurfsense_backend/tests/unit/middleware/test_git_tree_backend.pysurfsense_backend/tests/unit/middleware/test_git_tree_tool_staging.pysurfsense_backend/tests/unit/middleware/test_knowledge_store_persistence_builder.pysurfsense_backend/tests/unit/middleware/test_knowledge_tree.pysurfsense_backend/tests/unit/middleware/test_read_file_description.py
Comment on lines
+16
to
+17
| GIT -->|"one-way derivation (04)"| PG | ||
| PG -. "reindex(workspace) rebuilds from git (04)" .-> GIT |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the Postgres-to-Git reindex arrow.
Line 17 draws PG → GIT, contradicting the stated one-way Git → Postgres model. Reindex reads Git and rebuilds Postgres; it must not appear as a reverse data flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/git-native-kb/00b-diagrams.md` around lines 16 - 17, Remove the dashed
Postgres-to-Git reindex arrow from the diagram, leaving only the one-way
Git-to-Postgres derivation flow. Preserve the existing Git and Postgres nodes
and the forward arrow unchanged.
Comment on lines
+149
to
+161
| ```mermaid | ||
| sequenceDiagram | ||
| autonumber | ||
| participant M as Migrator (per workspace, flagged) | ||
| participant PG as Postgres (existing docs/folders) | ||
| participant GIT as New git repo | ||
| participant IDX as reindex(workspace) | ||
| M->>PG: read documents + folders (preserve unique_identifier_hash) | ||
| M->>GIT: write files + one seed commit | ||
| M->>IDX: rebuild chunks/embeddings from git HEAD | ||
| IDX-->>M: chunk set | ||
| M->>M: verify search parity vs pre-migration, then flip flag | ||
| Note over M,GIT: Postgres content kept until verified (rollback window). |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not reindex the seed revision before flipping a workspace.
This flow contradicts the adopted migration contract: the seed copies Postgres bytes, so existing chunks are already the derived index. Reindexing the seed turns every document into an addition and can trigger a workspace-wide re-embed. Show seed → byte-parity verification → mark the seed as the indexing baseline → flip instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/git-native-kb/00b-diagrams.md` around lines 149 - 161, Update the
Migrator sequence diagram to remove the IDX reindex step for the seed revision
and its chunk-set response. Show the flow as seed commit, byte-parity
verification against the existing index, marking the seed revision as the
indexing baseline, then flipping the workspace flag; retain the rollback note
that Postgres content remains until verification completes.
Comment on lines
+30
to
+39
| - **Seed commit per workspace.** Read current `folders` + `documents` | ||
| (`source_markdown`/`content`) and write the real tree into the Phase-1 repo as **one | ||
| seed commit** (`author=migration`), using the same path rules as the live write path | ||
| (C1). Streamed table scan + file writes: O(documents) I/O, no embeddings, no locks on | ||
| hot tables. Idempotent — re-seeding unchanged content is a no-op commit. | ||
| - **Preserve identity.** Keep the `unique_identifier_hash` ↔ path mapping so connector | ||
| re-syncs and existing references stay stable. | ||
| - **Parity = byte identity, not reindex.** Gate the flip on: every seeded blob's bytes | ||
| equal the document's Postgres markdown (and nothing is missing/extra). O(documents) | ||
| hashing, seconds per workspace. `reindex()` stays a disaster-recovery tool; run it once |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Migration can cut over stale Postgres content.
The seed and parity flow has no fence against legacy writes, while the runbook permits it to run live. A save can occur after scanning or parity and be omitted when Git becomes authoritative.
plans/git-native-kb/05-migration.md#L30-L39: add a migration lock or a final atomic Postgres change-watermark check before flip.plans/git-native-kb/05a-seed-runbook.md#L268-L272: do not describe live seeding as safe unless concurrent writes are quiesced or detected.
📍 Affects 2 files
plans/git-native-kb/05-migration.md#L30-L39(this comment)plans/git-native-kb/05a-seed-runbook.md#L268-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/git-native-kb/05-migration.md` around lines 30 - 39,
plans/git-native-kb/05-migration.md lines 30-39: Add a migration lock or final
atomic Postgres change-watermark validation between parity verification and
cutover, blocking the flip if writes occurred after scanning.
plans/git-native-kb/05a-seed-runbook.md lines 268-272: revise the live-seeding
guidance to require quiesced concurrent writes or explicit detection through the
same lock/watermark mechanism; do not claim live seeding is safe without that
protection.
Comment on lines
+27
to
+32
| return KnowledgeStorePersistenceMiddleware( | ||
| workspace_id=workspace_id, | ||
| created_by_id=user_id, | ||
| thread_id=thread_id, | ||
| llm=llm, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The cross-thread graph cache can bind Git persistence to the wrong thread.
The middleware receives thread_id at construction, but cross-thread graph reuse omits that identity. A cached graph can therefore commit another thread’s working copy.
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py#L27-L32: resolve the thread from live runtime configuration or remove the captured constructor identity.surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/agent_cache.py#L127-L130: includethread_idwhenever Git-native persistence is enabled, or disable cross-thread reuse for those graphs.
📍 Affects 2 files
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py#L27-L32(this comment)surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/agent_cache.py#L127-L130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py`
around lines 27 - 32, The cross-thread graph cache can associate Git persistence
with the wrong thread. In
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/builder.py
lines 27-32, update KnowledgeStorePersistenceMiddleware construction to resolve
thread_id from live runtime configuration or remove the captured constructor
identity; in
surfsense_backend/app/agents/chat/multi_agent_chat/main_agent/runtime/agent_cache.py
lines 127-130, include thread_id in the cache key whenever Git-native
persistence is enabled or disable cross-thread reuse for those graphs.
Comment on lines
+39
to
+45
| if thread_id is None: | ||
| return "thread-adhoc" | ||
| root = str(thread_id).split("::", 1)[0] | ||
| # A parentless subagent's id is a bare ``task:{id}``, naming no turn. | ||
| if not root or root.startswith("task:"): | ||
| return "thread-adhoc" | ||
| return f"thread-{root}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not share the thread-adhoc working copy across turns.
Missing thread_id values and parentless task:* IDs all map to the same workspace-local copy. Since end-of-turn persistence commits and discards that same ID, concurrent turns can mix, commit, or delete each other’s uncommitted changes. Derive a unique turn/run ID for this fallback, or reject Git-tree execution when no safely scoped ID exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@surfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/git_tree.py`
around lines 39 - 45, The workspace naming fallback in the thread-ID derivation
logic must not return the shared “thread-adhoc” identifier for missing or
parentless task IDs. Update this path to derive a unique turn/run-scoped
identifier, or reject Git-tree execution when no safe scope is available, while
preserving stable “thread-{root}” naming for valid parent-thread IDs.
Comment on lines
+22
to
+30
| LOCK_TTL_SECONDS = 30.0 | ||
| # How long a contender waits before giving up. | ||
| LOCK_WAIT_SECONDS = 10.0 | ||
|
|
||
| # Indexing a whole workspace embeds every document, so its ceiling is minutes. | ||
| INDEX_LOCK_TTL_SECONDS = 1800.0 | ||
| # A contender gives up quickly: the holder converges to the current revision | ||
| # anyway, and the drift sweep re-drives anything it missed. | ||
| INDEX_LOCK_WAIT_SECONDS = 5.0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline surfsense_backend/app/knowledge_store/write_lock.py --view expanded || true
echo "== relevant file sections =="
cat -n surfsense_backend/app/knowledge_store/write_lock.py | sed -n '1,160p'
echo "== write_lock usages =="
rg -n "acquire_(workspace|index)_lock|release_(workspace|index)_lock|WorkspaceLock|IndexLock|LOCK_TTL_SECONDS|INDEX_LOCK_TTL_SECONDS" surfsense_backend/app -SRepository: MODSetter/SurfSense
Length of output: 6165
🌐 Web query:
redis-py asyncio lock extension renew lock during work lease pattern
💡 Result:
In redis-py (including the asyncio implementation), you can manage lock extensions during long-running tasks using the extend and reacquire methods [1][2][3][4]. The extend method adds a specified amount of time to the existing TTL of an already acquired lock [3][4]. extend(additional_time: float, replace_ttl: bool = False) - additional_time: The number of seconds to add to the current lock duration [3][4]. - replace_ttl: If set to False (default), it adds the additional_time to the existing TTL. If set to True, it replaces the current TTL with the provided additional_time [3][4]. The reacquire method resets the lock's TTL back to the original timeout value specified when the lock was first initialized [3][4][5]. reacquire - This method effectively "re-ups" the lock to its full initial duration [5][6]. Important considerations for the lease/renewal pattern: 1. Ownership verification: Both methods internally verify that the current client still owns the lock using the lock's unique token [1][2][7]. If the lock has expired and been acquired by another process, these methods will raise a LockNotOwnedError [1][2][7]. 2. Error Handling: Always handle LockNotOwnedError and LockError to account for cases where the lock may have expired or was not acquired properly before an extension attempt [1][2][7]. 3. Execution safety: Because distributed locks are subject to network latency and clock drift, ensure your extension logic runs well before the actual expiration of the lock to prevent other processes from acquiring it in the interim. Example usage: async with lock: # Perform initial work await do_work # Renew lock for another 10 seconds await lock.extend(10) # Perform more work await do_work_further
Citations:
- 1: https://github.com/redis/redis-py/blob/5cd710a3/redis/asyncio/lock.py
- 2: https://github.com/redis/redis-py/blob/abd08b06/redis/asyncio/lock.py
- 3: https://redis.readthedocs.io/en/stable/lock.html
- 4: https://redis.readthedocs.io/en/latest/lock.html
- 5: https://redis.readthedocs.io/en/stable/_modules/redis/lock.html
- 6: https://redis.readthedocs.io/en/latest/_modules/redis/lock.html
- 7: https://github.com/redis/redis-py/blob/master/redis/lock.py
Keep Redis leases valid throughout the protected operation.
Both _workspace_lock() and workspace_index_lock() can let the Redis TTL expire while the caller is still mutating Git/Postgres, because lock.release() only detects loss of ownership after the protected block finishes and never renews the lease. Add lock.extend(...)/lock.reacquire() logic with LockNotOwnedError propagation so the held operation exits before another worker can acquire the lock. (Also affects the existing acknowledgement of this weakness at lines 101-102.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@surfsense_backend/app/knowledge_store/write_lock.py` around lines 22 - 30,
Update _workspace_lock() and workspace_index_lock() to renew their Redis leases
throughout the protected operation using lock.extend(...) or lock.reacquire(),
and propagate LockNotOwnedError immediately if renewal or ownership is lost so
the operation exits before another worker acquires the lock. Replace the
existing acknowledgement of this weakness near the lock handling with the actual
renewal and failure behavior, preserving normal release semantics.
Comment on lines
+70
to
+152
| async def record_saved_document( | ||
| session: AsyncSession, | ||
| *, | ||
| workspace_id: int, | ||
| doc_id: int, | ||
| title: str, | ||
| folder_id: int | None, | ||
| markdown: str, | ||
| author_user_id: str | None, | ||
| title_is_explicit: bool = False, | ||
| ) -> str | None: | ||
| """Resolve one document's canonical store path and record the save. | ||
|
|
||
| The recorded path is remembered on the row (``document_metadata``), so the | ||
| next save knows where the document used to live and can drop that file | ||
| when a retitle moves it. The marker is written only when a revision was | ||
| actually recorded: a marker without a file would make the row look | ||
| indexer-owned and a later rebuild would prune it. | ||
|
|
||
| ``title_is_explicit`` means someone chose the title, so the file follows it. | ||
| A note's title is otherwise re-read from its first heading on every save, | ||
| and letting that place the file would rename whatever the agent named — for | ||
| a name the caller never asked to change. | ||
|
|
||
| Never raises: while the store coexists with the Postgres write path | ||
| (until the Phase 5 cut), a recording failure must not fail the save | ||
| that already committed — it is logged instead. | ||
| """ | ||
| if not await knowledge_store_enabled_for(workspace_id): | ||
| return None | ||
| try: | ||
| index = await build_path_index(session, workspace_id) | ||
| document = await session.get(Document, doc_id) | ||
| metadata = document.document_metadata if document else None | ||
| previous = (metadata or {}).get(PATH_MARKER) | ||
| virtual_path = ( | ||
| doc_to_virtual_path( | ||
| doc_id=doc_id, title=title, folder_id=folder_id, index=index | ||
| ) | ||
| if title_is_explicit | ||
| else virtual_path_of( | ||
| metadata=metadata, | ||
| doc_id=doc_id, | ||
| title=title, | ||
| folder_id=folder_id, | ||
| index=index, | ||
| ) | ||
| ) | ||
| filename = virtual_path.rsplit("/", 1)[-1] | ||
| stale = _stale_store_path(previous, virtual_path) | ||
| revision = await record_markdown_files( | ||
| workspace_id=workspace_id, | ||
| files={to_store_path(virtual_path): markdown}, | ||
| message=f"docs: save {filename}", | ||
| author_user_id=author_user_id, | ||
| removes=[stale] if stale else (), | ||
| ) | ||
| if revision is not None and document is not None and previous != virtual_path: | ||
| document.document_metadata = { | ||
| **(document.document_metadata or {}), | ||
| PATH_MARKER: virtual_path, | ||
| } | ||
| # Safe to commit here: the recorder runs at the point of | ||
| # durability, after the save's own commit, so nothing else is | ||
| # pending on this session. | ||
| await session.commit() | ||
| except Exception as exc: | ||
| logger.warning( | ||
| "Knowledge store recording failed for document %s in workspace %s", | ||
| doc_id, | ||
| workspace_id, | ||
| exc_info=True, | ||
| ) | ||
| metrics.record_knowledge_store_record_outcome( | ||
| flow="editor_save", | ||
| status="failed", | ||
| error_category=metrics.categorize_exception(exc), | ||
| ) | ||
| return None | ||
| metrics.record_knowledge_store_record_outcome( | ||
| flow="editor_save", status="recorded" if revision else "noop" | ||
| ) | ||
| return revision |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A metadata-commit failure after a successful git write is reported as a total failure, but git already has the new revision.
record_markdown_files durably commits to git inside its own store.transaction() context (line 58-64) before this function ever touches document.document_metadata. If the subsequent await session.commit() (line 135) for the PATH_MARKER update raises, the outer except Exception (line 136) logs "recording failed" and returns None — but the revision was already written to git. The PATH_MARKER on the row stays stale, so the next save's previous lookup (line 104) will be wrong: _stale_store_path may fail to remove the file the document actually moved to, or may target the wrong path for removal, leaving orphaned files in the store over time.
Consider separating the metadata-persist step from the git-write step in error handling, so a metadata-commit failure is distinguishable (and retried/reconciled) rather than masking a revision that already landed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@surfsense_backend/app/services/document_revision_recorder.py` around lines 70
- 152, Separate the PATH_MARKER metadata commit in record_saved_document from
the record_markdown_files git-write error path. If session.commit() fails after
a revision is returned, preserve and return that revision, and report the
metadata persistence failure distinctly for retry or reconciliation instead of
treating the entire recording as failed. Keep the existing full-failure handling
for errors occurring before the git revision is committed.
Comment on lines
+155
to
+188
| async def record_prepared_documents( | ||
| session: AsyncSession, documents: Sequence[Document] | ||
| ) -> str | None: | ||
| """Record a sync batch's accepted markdown as one revision. | ||
|
|
||
| Called after ``prepare_for_indexing`` commits — the moment content becomes | ||
| durable — so chunking/embedding failures can never block the record. | ||
| Never raises, for the same coexistence reason as ``record_saved_document``. | ||
| """ | ||
| if not documents: | ||
| return None | ||
| workspace_id = documents[0].workspace_id | ||
| if not await knowledge_store_enabled_for(workspace_id): | ||
| return None | ||
| try: | ||
| index = await build_path_index(session, workspace_id) | ||
| files: dict[str, str] = {} | ||
| for doc in documents: | ||
| if not doc.source_markdown: | ||
| continue | ||
| virtual_path = doc_to_virtual_path( | ||
| doc_id=doc.id, title=doc.title, folder_id=doc.folder_id, index=index | ||
| ) | ||
| files[to_store_path(virtual_path)] = doc.source_markdown | ||
| revision = await record_markdown_files( | ||
| workspace_id=workspace_id, | ||
| files=files, | ||
| message=f"sync: index {len(files)} document(s)", | ||
| author_user_id=( | ||
| str(documents[0].created_by_id) | ||
| if documents[0].created_by_id is not None | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)document_revision_recorder\.py$|prepare_for_indexing' || true
echo "== file outline =="
ast-grep outline surfsense_backend/app/services/document_revision_recorder.py --view expanded || true
echo "== relevant file sections =="
wc -l surfsense_backend/app/services/document_revision_recorder.py
sed -n '1,240p' surfsense_backend/app/services/document_revision_recorder.py
echo "== callers of record_prepared_documents =="
rg -n "record_prepared_documents|prepare_for_indexing" surfsense_backend -SRepository: MODSetter/SurfSense
Length of output: 21441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== indexing pipeline service relevant sections =="
wc -l surfsense_backend/app/indexing_pipeline/indexing_pipeline_service.py
sed -n '200,370p' surfsense_backend/app/indexing_pipeline/indexing_pipeline_service.py
sed -n '560,640p' surfsense_backend/app/indexing_pipeline/indexing_pipeline_service.py
echo "== prepare_for_indexing definitions/calls =="
rg -n "def prepare_for_indexing|async def prepare_for_indexing|prepare_for_indexing\(" surfsense_backend/app/indexing_pipeline surfsense_backend/app/services surfsense_backend/app/tasks/connector_indexers -S
echo "== connector recorder test sections =="
sed -n '320,470p' surfsense_backend/tests/integration/knowledge_store/test_document_recorder.py
echo "== integration tests around batch cases =="
sed -n '80,120p' surfsense_backend/tests/integration/indexing_pipeline/test_prepare_for_indexing.py
sed -n '135,155p' surfsense_backend/tests/integration/indexing_pipeline/test_prepare_for_indexing.py
sed -n '280,305p' surfsense_backend/tests/integration/indexing_pipeline/test_prepare_for_indexing.pyRepository: MODSetter/SurfSense
Length of output: 19350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local_folder_batch_caller =="
wc -l surfsense_backend/app/tasks/connector_indexers/local_folder_indexer.py
sed -n '740,885p' surfsense_backend/app/tasks/connector_indexers/local_folder_indexer.py
echo "== connectordoc class =="
rg -n "class ConnectorDocument|ConnectorDocument" surfsense_backend/app -S | head -n 80
rg -n "workspace_id\s*=" surfsense_backend/app -S | rg "ConnectorDocument|connector_docs|connector_doc" || true
echo "== direct record_prepared_documents test coverage for mixed workspace =="
rg -n "workspace_id|mixed workspace|different workspace|record_prepared_documents" surfsense_backend/tests/integration/knowledge_store/test_document_recorder.py -SRepository: MODSetter/SurfSense
Length of output: 18843
Defend record_prepared_documents against mixed-workspace batches.
prepare_for_indexing receives user-controlled ConnectorDocuments and can return persisted Document objects from any workspace in the batch before calling this recorder. Currently it indexes by connector_docs[0].workspace_id; if one document comes from another workspace, it would write that document’s markdown into the first workspace’s git-backed knowledge store. Reject batches where doc.workspace_id != documents[0].workspace_id before building/store writing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@surfsense_backend/app/services/document_revision_recorder.py` around lines
155 - 188, Update record_prepared_documents to validate that every document has
the same workspace_id as documents[0] before calling build_path_index or
record_markdown_files; reject mixed-workspace batches without writing to any
knowledge store.
Comment on lines
+103
to
+140
| async def _sweep() -> int: | ||
| """Enqueue indexing wherever the stamp disagrees with the store's HEAD. | ||
|
|
||
| Candidates are **flipped** workspaces only (`workspaces.knowledge_store_enabled`). | ||
| A seeded-but-unflipped workspace has a repo on disk too, but Postgres is | ||
| still its write model — indexing it would fight the legacy pipeline. | ||
| """ | ||
| session_maker = get_celery_session_maker() | ||
| async with session_maker() as session: | ||
| result = await session.execute( | ||
| select(Workspace.id, Workspace.last_indexed_revision).where( | ||
| Workspace.knowledge_store_enabled.is_(True) | ||
| ) | ||
| ) | ||
| stamps = dict(result.all()) | ||
|
|
||
| enqueued = 0 | ||
| for workspace_id, stamp in stamps.items(): | ||
| if enqueued >= SWEEP_ENQUEUE_CAP: | ||
| logger.info( | ||
| "Drift sweep hit its cap of %d; the rest wait for the next run", | ||
| SWEEP_ENQUEUE_CAP, | ||
| ) | ||
| break | ||
| head = await KnowledgeStore.for_workspace(workspace_id).get_current_revision() | ||
| if head is None or head == stamp: | ||
| continue | ||
| if stamp is None: | ||
| # Never indexed: a full converge that embeds the whole tree. Route | ||
| # it with the rebuilds so a backfill can't bury user-facing saves. | ||
| reindex_knowledge_store.delay(workspace_id) | ||
| else: | ||
| index_knowledge_store_revision.delay(workspace_id) | ||
| enqueued += 1 | ||
|
|
||
| if enqueued: | ||
| logger.info("Drift sweep enqueued indexing for %d workspaces", enqueued) | ||
| return enqueued |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No per-workspace error isolation in the drift sweep.
If get_current_revision() raises for one workspace (e.g. a corrupted or unreadable repo), the exception propagates out of _sweep and aborts the entire hourly run — no subsequent workspaces get their HEAD checked or enqueued, even ones with real drift. Combined with the fact that the enqueue cap only bounds enqueues, not the number of sequential HEAD reads performed on non-drifted workspaces, a large flipped fleet can make this loop slow and fragile as rollout grows.
🛡️ Isolate per-workspace failures
for workspace_id, stamp in stamps.items():
if enqueued >= SWEEP_ENQUEUE_CAP:
logger.info(
"Drift sweep hit its cap of %d; the rest wait for the next run",
SWEEP_ENQUEUE_CAP,
)
break
- head = await KnowledgeStore.for_workspace(workspace_id).get_current_revision()
+ try:
+ head = await KnowledgeStore.for_workspace(workspace_id).get_current_revision()
+ except Exception:
+ logger.exception(
+ "Drift sweep failed to read HEAD for workspace %s; skipping", workspace_id
+ )
+ continue
if head is None or head == stamp:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def _sweep() -> int: | |
| """Enqueue indexing wherever the stamp disagrees with the store's HEAD. | |
| Candidates are **flipped** workspaces only (`workspaces.knowledge_store_enabled`). | |
| A seeded-but-unflipped workspace has a repo on disk too, but Postgres is | |
| still its write model — indexing it would fight the legacy pipeline. | |
| """ | |
| session_maker = get_celery_session_maker() | |
| async with session_maker() as session: | |
| result = await session.execute( | |
| select(Workspace.id, Workspace.last_indexed_revision).where( | |
| Workspace.knowledge_store_enabled.is_(True) | |
| ) | |
| ) | |
| stamps = dict(result.all()) | |
| enqueued = 0 | |
| for workspace_id, stamp in stamps.items(): | |
| if enqueued >= SWEEP_ENQUEUE_CAP: | |
| logger.info( | |
| "Drift sweep hit its cap of %d; the rest wait for the next run", | |
| SWEEP_ENQUEUE_CAP, | |
| ) | |
| break | |
| head = await KnowledgeStore.for_workspace(workspace_id).get_current_revision() | |
| if head is None or head == stamp: | |
| continue | |
| if stamp is None: | |
| # Never indexed: a full converge that embeds the whole tree. Route | |
| # it with the rebuilds so a backfill can't bury user-facing saves. | |
| reindex_knowledge_store.delay(workspace_id) | |
| else: | |
| index_knowledge_store_revision.delay(workspace_id) | |
| enqueued += 1 | |
| if enqueued: | |
| logger.info("Drift sweep enqueued indexing for %d workspaces", enqueued) | |
| return enqueued | |
| async def _sweep() -> int: | |
| """Enqueue indexing wherever the stamp disagrees with the store's HEAD. | |
| Candidates are **flipped** workspaces only (`workspaces.knowledge_store_enabled`). | |
| A seeded-but-unflipped workspace has a repo on disk too, but Postgres is | |
| still its write model — indexing it would fight the legacy pipeline. | |
| """ | |
| session_maker = get_celery_session_maker() | |
| async with session_maker() as session: | |
| result = await session.execute( | |
| select(Workspace.id, Workspace.last_indexed_revision).where( | |
| Workspace.knowledge_store_enabled.is_(True) | |
| ) | |
| ) | |
| stamps = dict(result.all()) | |
| enqueued = 0 | |
| for workspace_id, stamp in stamps.items(): | |
| if enqueued >= SWEEP_ENQUEUE_CAP: | |
| logger.info( | |
| "Drift sweep hit its cap of %d; the rest wait for the next run", | |
| SWEEP_ENQUEUE_CAP, | |
| ) | |
| break | |
| try: | |
| head = await KnowledgeStore.for_workspace(workspace_id).get_current_revision() | |
| except Exception: | |
| logger.exception( | |
| "Drift sweep failed to read HEAD for workspace %s; skipping", workspace_id | |
| ) | |
| continue | |
| if head is None or head == stamp: | |
| continue | |
| if stamp is None: | |
| # Never indexed: a full converge that embeds the whole tree. Route | |
| # it with the rebuilds so a backfill can't bury user-facing saves. | |
| reindex_knowledge_store.delay(workspace_id) | |
| else: | |
| index_knowledge_store_revision.delay(workspace_id) | |
| enqueued += 1 | |
| if enqueued: | |
| logger.info("Drift sweep enqueued indexing for %d workspaces", enqueued) | |
| return enqueued |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@surfsense_backend/app/tasks/celery_tasks/knowledge_store/index_tasks.py`
around lines 103 - 140, Update _sweep to isolate each workspace iteration: catch
failures from KnowledgeStore.for_workspace(workspace_id).get_current_revision()
and log the workspace-specific error, then continue checking subsequent
workspaces. Preserve existing drift detection and enqueue behavior, while
ensuring a failed HEAD read does not abort the sweep.
Comment on lines
+142
to
+165
| if ( | ||
| not pending_values | ||
| and fallback_commit_filesystem_mode == FilesystemMode.CLOUD | ||
| and fallback_commit_workspace_id is not None | ||
| and await knowledge_store_enabled_for(fallback_commit_workspace_id) | ||
| ): | ||
| try: | ||
| delta = await commit_turn_working_copy( | ||
| workspace_id=fallback_commit_workspace_id, | ||
| thread_id=fallback_commit_thread_id, | ||
| created_by_id=fallback_commit_created_by_id, | ||
| llm=None, | ||
| ) | ||
| if delta: | ||
| await agent.aupdate_state( | ||
| config, | ||
| delta, | ||
| as_node="KnowledgeStorePersistenceMiddleware.after_agent", | ||
| ) | ||
| except Exception as exc: | ||
| _perf_log.warning( | ||
| "[stream_agent_events] git-native safety-net commit failed: %s", exc | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A15 'def knowledge_store_enabled_for' surfsense_backend/app/knowledge_store/settings.pyRepository: MODSetter/SurfSense
Length of output: 894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== event_loop relevant section =="
sed -n '120,180p' surfsense_backend/app/tasks/chat/streaming/agent/event_loop.py | cat -n | sed 's/^/event_loop.py:/'
echo
echo "== settings implementations =="
sed -n '1,120p' surfsense_backend/app/knowledge_store/settings.py | cat -n | sed 's/^/settings.py:/'
echo
echo "== commit_turn_working_copy definition/usages =="
rg -n "def commit_turn_working_copy|async def commit_turn_working_copy|commit_turn_working_copy\(" surfsense_backend/app -g '*.py' | head -50Repository: MODSetter/SurfSense
Length of output: 8406
Move the enablement check inside the safety-net handler.
knowledge_store_enabled_for(fallback_commit_workspace_id) performs a live database lookup and can raise, but it is currently evaluated in the if condition before the try/except. That exception bypasses the commit-gate evaluation, contract logging, and interrupt-frame rendering that follow. Put the await inside the existing try so the safety net keeps the finalization path alive on transient failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@surfsense_backend/app/tasks/chat/streaming/agent/event_loop.py` around lines
142 - 165, Move the await
knowledge_store_enabled_for(fallback_commit_workspace_id) call out of the
safety-net if condition and into its existing try block. Preserve the other
commit-gate conditions, and only run commit_turn_working_copy plus the state
update when the enablement check succeeds and returns true; ensure exceptions
from the lookup are handled by the existing _perf_log.warning path.
4 tasks
CREDO23
added a commit
that referenced
this pull request
Jul 30, 2026
This reverts commit 366f9c1, 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.
CREDO23
added a commit
that referenced
this pull request
Jul 30, 2026
Revert "Merge pull request #1649 from CREDO23/kb_git_mvp"
4 tasks
CREDO23
added a commit
that referenced
this pull request
Jul 31, 2026
Restores the work from #1649, which was reverted on main in #1650 to keep it out of the last release. Content is identical to that merge. This is a revert of the revert (a8292f5) rather than a merge of kb_git_mvp, deliberately. Merging the branch would make its commits an ancestor of both dev and main; main's side deleted those files, so the next merge between the two branches would silently delete them again. A revert carries the content without the history, so dev and main share no ancestor that knows about these files, and the eventual dev -> main merge sees them as added on one side only and keeps them. Git becomes the source of truth for knowledge base content; Postgres and pgvector become a derived, rebuildable index. Both switches guarding the new path default to off: the KNOWLEDGE_STORE_ENABLED env var and the per-workspace knowledge_store_enabled column, so merging this changes no runtime behaviour. Migrations 175 and 176 only add columns and use ADD COLUMN IF NOT EXISTS. They are already applied on production, where alembic_version was moved back to 174 during the revert, so they will re-run harmlessly. Verified on this branch: 0 conflicts against dev, every app.* import in the restored files resolves, 2251 unit tests pass. The one collection error (platforms/google_maps) is missing a fixture that is untracked on both dev and main, and predates this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Workspace knowledge base content moves into a per-workspace git repository, and the Postgres side becomes a derived index rather than the store of record.
app/knowledge_store/— aKnowledgeStorefacade over a pluggable content engine (dulwich today). Mutations go through atransaction()unit of work, so one agent turn produces exactly one revision, whatever mix of writes, edits, moves and deletes it contains.app/knowledge_store/index/) —index_changes()walks the diff sinceworkspaces.last_indexed_revision;index_tree()reconciles the whole tree and prunes rows whose files are gone. Embeddings are reused by content hash, so re-indexing unchanged content costs nothing.app/knowledge_store/migrate.pyseeds an existing workspace's documents into git and verifies byte parity;scripts/migrate_knowledge_store.pyruns it across the fleet with dry-run, per-workspace reports and flip/unflip.Two new columns (migrations 175, 176), both nullable and additive:
workspaces.knowledge_store_enabledandworkspaces.last_indexed_revision.This is inert on merge. Git-native behaviour requires both the process-wide
KNOWLEDGE_STORE_ENABLED(defaults false) and the per-workspace flag (defaults false). With either off, every path behaves exactly as before. No workspace is flipped by this PR.Motivation and Context
Document versioning was a hand-rolled set of tables — snapshot rows, restore logic, and a content-hash scheme maintained by hand. Git already solves content-addressed storage, history, diffs and moves, and solves them better than we were going to. Making it the source of truth removes that machinery instead of extending it, and gives the agent a real working copy to edit rather than a simulated filesystem.
The second motivation is search quality: with git as truth, the chunk index becomes disposable and can be rebuilt from any revision, which is what lets citations point at a revision instead of at mutable rows.
Design and phasing are written up under
plans/git-native-kb/.API Changes
documents_routes.pyandeditor_routes.pychange internally (saves and retitles route through the store when the workspace is flipped) but no request or response shape changes.Change Type
Testing Performed
Unit and integration suites:
6 failed, 3995 passed, 13 errors. All of it is inherited frommain— 3 inautomations, 2 ingoogle_mapsparsers, 1 PAT static check, and the 13 errors aregoogle_mapstests whose captured fixture JSON isn't in the repo. Nothing inknowledge_store, and the diff touches none of those paths.New coverage is integration-first against real Postgres and Redis rather than mocks of components we own: turn commits across every operation, writer guards, the cross-process write lock (including the event-loop regression that leaked locks), converge and prune, rename following, migration seeding and parity, and the fleet runner.
Manual verification on a canary workspace with both flags on: seed and parity check, editor save, and agent turns covering create, edit, move and delete — confirming a move keeps the document's row id and version history.
Checklist
High-level PR Summary
This PR migrates workspace knowledge base content into per-workspace Git repositories, making Git the single source of truth while Postgres becomes a derived index. The implementation adds a
KnowledgeStorefacade over dulwich with transaction-based mutations, an agent write path using git working copies, and a derived index that re-chunks/re-embeds based on content hashes. The system includes drift monitoring, migration tooling with byte-parity verification, and is fully inert on merge — requiring both the globalKNOWLEDGE_STORE_ENABLEDflag and per-workspaceknowledge_store_enabledcolumn to activate. No workspace is flipped by this PR. The change replaces three hand-rolled versioning systems (DocumentVersion,DocumentRevision/FolderRevision, andAgentActionLog) with native git history, eliminating significant maintenance burden while enabling document versioning as a first-class feature.⏱️ Estimated Review Time: 3+ hours
💡 Review Order Suggestion
plans/git-native-kb/00-umbrella-plan.mdplans/git-native-kb/00c-shared-contract.mddocs/adr/0001-git-native-knowledge-base.mddocs/adr/0002-knowledge-core-ports-and-adapters.mdsurfsense_backend/alembic/versions/175_add_workspace_knowledge_store_flag.pysurfsense_backend/alembic/versions/176_add_derived_index_columns.pysurfsense_backend/app/knowledge_store/engines/base.pysurfsense_backend/app/knowledge_store/engines/git.pysurfsense_backend/app/knowledge_store/store.pysurfsense_backend/app/knowledge_store/transaction.pysurfsense_backend/app/knowledge_store/write_lock.pysurfsense_backend/app/knowledge_store/settings.pysurfsense_backend/app/agents/chat/multi_agent_chat/shared/middleware/filesystem/backends/git_tree.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_store_persistence/commit_turn.pysurfsense_backend/app/knowledge_store/index/converge.pysurfsense_backend/app/knowledge_store/migrate.pysurfsense_backend/scripts/migrate_knowledge_store.pysurfsense_backend/app/config/__init__.pysurfsense_backend/app/db.pysurfsense_backend/app/celery_app.pySummary by CodeRabbit
New Features
Documentation
Bug Fixes