Repository navigation
security(kb): backfill ownerless ingested documents as SYSTEM, frozen to the facts present at deploy (#16693) - #16723
Conversation
… to the facts present at deploy (#16693) Owner decisions on #16693: ingested documents keep their reach, now as explicit SYSTEM, and every other ownerless fact stays private. - knowledge/ingestion_visibility.py recognises each ingestion class by a positive marker that only its writer sets; source_type cannot, since every unmarked fact defaults to manual_upload. - Migration 20260914_092 marks the facts that have no owner and no visibility at deploy and logs the count. - At startup, after adopt_legacy_facts, the backfill changes only marked facts, through update_fact. - index_ownership files an ownerless SYSTEM fact in kb:system:facts. - Trusted ingestion sites stamp SYSTEM themselves: connectors, the population router, machine-synced and parsed man pages, the repo sync's documentation, and the admin upload.
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (18)
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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…ption observes it and decided facts drop out (#16693) - migration-tests failed on 51ab542: probe-ladder observability found 20260914_092 unobservable, because it only wrote a metadata key. - An unobservable intermediate revision re-runs on baseline adoption. A re-run would have marked facts stored after deploy, which is the spoofing window the freeze exists to close. - The candidate flag is now a nullable knowledge_facts.visibility_backfill_candidate column. The add_column is a structural marker the probe observes. The flag is set only in the run that creates the column, so a re-run can never flag a fact stored after deploy. - Review LOW: the backfill clears the flag on every decided candidate (updated, kept private, skipped), so a later start revisits only failed updates instead of re-walking kept-private facts forever. The flag is row-only, so clearing it moves no projection.
|
Review at 16535f6: approve. Went through all four areas, traced each to actual call sites rather than trusting the description: Per-site stamps, not a chokepoint — the only 4 callers of `stamp_if_document`/`system_backfill_class` are `api/knowledge.py:739` (the `/upload` route's `_store_fact_in_kb`, admin-gated — verified `Depends(check_admin_permission)` directly on the route), `connectors/base.py:604` (server-controlled connector metadata — `source_type` is set by `.update()` after copying the connector's own content metadata, so nothing the source could supply overrides it), `fact_projection.py` (the backfill, operating on already-stored facts), and `knowledge_sync_incremental.py` (the repo sync). None of `/extract`, storefact, or MCP add call this classification at all — the risk the module docstring names is closed by exclusion, not by a filter that could have a hole in it. `_store_fact_in_kb`'s `type == "file"` guard — grepped the whole file: `"type": "file"` is written exactly once, as a hardcoded literal in the `/upload` handler's own `fact_metadata` dict (never merged from caller-supplied JSON), immediately before the only call site that ever sets it. The other 4 callers of `_store_fact_in_kb` never set `type` at all, so the check can't fire for them regardless of what a caller sends elsewhere. The migration as a structural marker — `upgrade()`'s own guard (`if not has_table(...) or has_column(...): return`) means the one-time `UPDATE ... SET visibility_backfill_candidate = TRUE` runs exactly once, ever, on the deploy that adds the column. Any later migration run — including a fresh baseline adoption — sees the column already exists and never re-flags, so a fact stored after first deploy structurally cannot be picked up by the startup backfill, closing the spoof-then-restart window by construction rather than by a runtime check that could be bypassed. `_project_fact_to_redis`'s condition — this drops `facts.py`'s `and owner_id` requirement, and I verified why that's required, not just that it changed: `ownership_index.py`'s `index_ownership` now indexes an ownerless-but-`visibility=SYSTEM` fact directly into `kb:system:facts` (bypassing `set_owner`, which needs an owner). Without dropping the outer `owner_id` check, a freshly-stamped SYSTEM document from any of the trusted ingestion sites would never even reach `index_ownership`, and would silently never appear in the system-facts index. Confirmed the new branch only fires for `visibility == SYSTEM` explicitly — any other ownerless fact still correctly returns `False` without touching the system index. `_backfill_one_document`'s double-check — re-reads the fact fresh via `_read_fact_for_write` and re-runs `system_backfill_class` against that copy before writing, specifically to catch a fact that changed between the SQL-row read and the actual update (comment: "drift between them must never make an owned or scoped fact SYSTEM"). Good defense-in-depth on top of the migration's deploy-time freeze. Ratchet baseline: all 4 touched entries move down (3477→3471, 1987→1979, 1547→1546, 678→676), no ceiling raises. CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable. Noted the pending rebase after #16705 for the shared `facts.py` line. |
…TEM facts indexed (#16693) Conflicts after vehicles 2e/2f: - knowledge/facts.py: #16705 added "and owner_id" to the ownership-index call's guard. Kept this branch's guard without it: index_ownership itself returns False for an ownerless fact and adds an ownerless SYSTEM fact to kb:system:facts, which the owner_id check would skip. A missing ownership manager still takes the #689 fallback (#16685's test covers it). - python_file_size ratchet and known-large lists: every entry set to the merged tree's wc -l (all 488 entries in each file checked).
1 participant
Refs #16693, #16654, #16664
Once #16664 (T2) scopes chat grounding, a fact with no owner and no visibility would stop grounding anyone's chat. This PR delivers the owner decisions recorded on #16693: ingested documents keep their reach, now explicitly SYSTEM and auditable, and every other ownerless fact stays private. It uses
Refs, notCloses, because AC3 (before and after counts from a deployment) is host evidence that only exists after deploy.Thinking Path
source_typecan't identify documents._apply_provenance_defaultsstampsmanual_uploadon every fact that sets none. Agent and LLC diaries, fact extraction and non-admin MCP adds all carry it, the same as document uploads. So each ingestion class is recognised by a positive marker that only its own writer sets. A census of every production KB writer confirmed each marker has exactly one writer./extractauto-store, and thestorefacttool. So new ingestion is stamped at each trusted, admin-only site. The backfill has the same risk: if it re-ran over every ownerless fact at each startup, it would promote an imitation at the next restart. So a migration freezes the candidates at deploy time.kb:system:factstogether. So the migration flags the candidates and logs the count. The startup chain, afteradopt_legacy_facts, changes them throughupdate_fact, as AC1 asks.What Changed
knowledge/ingestion_visibility.py(new): the owner's classes as one predicate.source_type: connector); admin file upload (type: filewith a filename); man pages (the population router, the man-page parser, orcategory: system/manpageswith amachine_id); the documentation KB; the system-command reference; and the repo sync's documentation categories.atomic_facts, chat, and anything owned or scoped.reports,securityandsource-codecategories stay private.stamp_if_documentapplies the predicate to metadata a trusted site built itself.Migration
20260914_092. Adds the nullable columnknowledge_facts.visibility_backfill_candidateand sets it on every fact with no owner and no visibility at deploy, logging the count.models/knowledge_fact.pydeclares it.FactProjectionMixin.backfill_document_visibility:fact_store.backfill_candidates;iter_factsgains a keyword-onlywhere; newcount_facts);update_factwill merge onto, so drift between Redis and the row can never make an owned fact SYSTEM;update_factwithvisibility: systemplus avisibility_backfill: 16693tag, so each change can be found and reversed;clear_backfill_candidates, a row-only update), so a later start revisits only failed updates;source_type/category.knowledge_factorychains it afteradopt_legacy_facts.kb:system:factsfor ownerless facts.index_ownershipnow files an ownerless SYSTEM fact there; before, it returned early without an owner._project_fact_to_rediscalls it whenever the manager exists. fix(kb): ID lists read back as lists, explicit read APIs pass the admin input, MCP add refuses non-admin platform-wide reach, and fact store/delete survive a missing ownership manager (#16662, #16663, #16685) #16705 touches the same line, and I'll resolve it on rebase.Trusted-site stamps (AC2):
stamp_if_document, which respects an owner or visibility the source already set);_store_fact_in_kb, keyed on thetype: filethat only the upload route writes.Size ratchet. The frozen files shrank, so their entries are lowered:
api/knowledge.py3477→3471,knowledge_maintenance1987→1979,knowledge_population1547→1546,connectors/base678→676,knowledge_sync_incremental894→884.facts.pystays at 1799.Acceptance criteria
kb:system:facts) through theupdate_factpath; idempotent. The scope is ingestion-sourced facts, per the owner.backfill_document_visibilitymakes the changes.test_the_backfill_write_reaches_the_row_redis_chromadb_and_the_system_indexdrives the realupdate_fact.test_candidates_are_flagged_by_the_migration_and_still_unowned_and_unsetpins the selection and the structural marker. Decided facts lose the flag and failures keep it, as the four backfill tests show.visibility=systemexplicitly from then on. Tested.api/kb_ingestion_sites_system_16693_test.pycovers every site, including the upload route, and checks that the backfill's predicate recognises what each site writes.test_a_new_ownerless_system_fact_is_projected_into_the_system_indexcovers the projection.owner_id IS NULL, and the predicate rejects any owner or scope key.test_a_fact_whose_live_copy_has_an_owner_is_never_touchedcovers drift between Redis and the row.Not in this PR
autobot_docsChromaDB collection. The documentation indexer writes there, and it has no facts-store rows, so a row backfill can't reach it. Noted on security(kb): chat and agent grounding read the knowledge base as the chatting user #16664 for T2.Verification
16535f6bf:all relevant tests pass, overapi/kb_ingestion_sites_system_16693_test.py,api/knowledge_test.py,knowledge/fact_projection_backfill_16693_test.py,knowledge/ingestion_visibility_16693_test.py,knowledge/ownership_index_16663_test.pyandknowledge_sync_incremental_test.py. Pre-commit passed.51ab54282, root-caused and fixed in16535f6bf.migration-testsfailed ontest_probe_ladder_selfcheck.py::test_observability_coverage: the revision only wrote a metadata key, so the probe ladder couldn't observe it. Allowlisting it wasn't safe. An unobservable intermediate revision re-runs on baseline adoption, and a re-run would have marked facts stored after deploy. The column design is observable and re-run-safe.51ab54282: APPROVE. Verified: every_store_fact_in_kbcaller, both admin-gated routers, the connector gate, and the migration's head and SQL. Findings:16535f6bf: every decided candidate loses its flag._cleanup_fact_mappingsstill usedhasattr(self, "ownership_manager"). Already fixed by fix(kb): ID lists read back as lists, explicit read APIs pass the admin input, MCP add refuses non-admin platform-wide reach, and fact store/delete survive a missing ownership manager (#16662, #16663, #16685) #16705.Model Used
Claude Opus 5 (
claude-opus-5).🤖 Generated with Claude Code