feat(knowledge): add documents automatically, and dedup per document

[NicholasRBowers]

Problem

The Knowledge Library only grew when the user added a source by hand. Two kinds of document therefore stayed invisible to search:

• Documents the agent read while working. A design doc it fetched from a wiki, a spec someone linked, a runbook it used to answer a question — all gone at the end of the turn. The agent had no way to add anything: its Library access was read-only (local_knowledge_search, knowledge_dedup).
• The documentation of the project the user is working in. Registering it by hand is possible, but the folder-add path walks everything FileReader.SUPPORTED accepts — source code, logs, CSVs, extensionless files — because per-source filtering is denylist-only with no include allowlist. Pointing it at a repository ingests the repository, not its docs.

A config key named knowledge.auto_ingest_doc_links has existed for this and does nothing: its only consumer is a platform seam that resolves to return None.

Separately, de-duplication had a hole this feature would have walked straight into. enumerate_docs enumerated folder sources per file but fell back to one unit per SOURCE for everything else. For a source holding N documents that unit's hash was only its first item's, so a match made the whole source the loser and delete_source_cascade removed all N. The guard against that (_AGGREGATE_SOURCE_TYPES) excluded aggregate sources from dedup entirely — so artifact documents were never de-duplicated at all.

Why it matters

Every document the user has to remember to file is a document that will not be there when they search for it later — and the value of a knowledge library is exactly the things you did not think to save. Meanwhile the un-filtered folder path made the one available workaround worse than useless: ingesting a repository's source code costs one LLM extraction call per chunk and pollutes retrieval for every future query.

The dedup hole mattered immediately: a new aggregate source for agent-added documents would have been un-deduped by construction, and the un-guarded path deletes user data.

Fix (symptoms → root cause → change)

Root cause of the missing documents: there was no agent write path, and no way to say "documents only" to a folder scan.

Root cause of the dedup hole: the dedup unit was sometimes a source. A source is not a document, so treating it as one is wrong for every source holding more than one — and the carve-out that hid the symptom also disabled the feature.

1. A document filter, expressed as source properties (doc_filter.py)

One rule decides every case: auto-add prose written for humans about intent, decisions, and how things work; exclude prose written for agents, generated files, and machine-readable lists. It is expressed as the properties a folder source already understands — include_extensions, ignore_patterns, extra_skip_dirs, min_file_bytes — so the ordinary scan path applies it with no special casing.

.txt is excluded: inside a repository it is nearly always a list, not prose (verified against SOURCES.txt, scrub-allowlist.txt, windows-expected-failures.txt).

Root-anchoring is the load-bearing detail. Repository boilerplate (AGENTS.md, SECURITY.md, LICENSE*, …) is matched against the path relative to the project root with no separator in the pattern, so it can only match a top-level path. Matching those as bare basenames at any depth destroys real documents — measured deleting docs/kiro-cli/mcp/security.md and docs/system-specs/modules/security.md.

Measured on the real trees: KiroCrew 2706 walked → 270 with a document extension → 167 after the filter (3.4 MB); opportunity-planner 150 → 105. This matches the plan's predicted ~165/~105.

2. include_extensions, a size floor, and a per-sweep chunk budget (folder_watcher.py)

_walk gains an extension allowlist that can only narrow (never widen past reader support) and a size floor that reuses the stat already taken for the mtime. None means "no allowlist" — today's behaviour for every existing source — and an empty set means "nothing", a distinction that would silently change every source if inverted.

File filters bound pollution; only a chunk budget bounds cost — dropping 277 files to 209 removed only ~140 of ~1740 extraction calls, because a handful of large documents dominates. So discovered files are now ordered newest-first unconditionally, and a sweep stops once it has ingested knowledge.auto_ingest_chunk_budget chunks (default 150 ≈ one repository per hour at the 300s interval). Files not reached keep or lack their folder_file_state row, so the next sweep resumes from them — the existing status column already carries the resume point. Never applied to a folder the user added by hand: they asked for the whole folder.

_walk also now normalizes the relative path before fnmatch. Patterns are written with /, so on Windows every pattern containing a separator silently never matched — that broke the new *.egg-info/* rule and, latently, the existing sub/* style.

3. Project documents, registered without a confirmation step (project_docs.py)

Each live chat slot's project dir resolves to its nearest .git ancestor and is registered as an active local_folder source carrying the document filter.

The manual path uses pending_confirmation because an unfiltered walk is unbounded. The filter plus the budget makes it bounded, so the gate is unnecessary rather than skipped — and dismissal happens after the fact instead: deleting the source writes a tombstone that survives the delete.

Two guards worth naming:

• A repo root resolving to $HOME is refused. .git in a home directory is a common dotfiles setup; without this, any project dir under such a home would register the entire home directory.
• Containment is re-validated per sweep on the right invariant. A project repo root lives outside the workspace by design, so the drop folder's workspace-containment check is wrong here — applying it would skip every project source with a denied audit event. Instead the recorded path must still resolve to itself, which catches the directory being swapped for a link elsewhere.

4. The agent write path (agent_source.py, knowledge_add_document)

Documents land in one aggregate agent:// source ("Auto-added"), with per-document groups in a new agent_item_state table keyed by a slug derived from the path or title — not the content, so an edit replaces the group instead of accumulating copies. It routes through IngestionPipeline.ingest_file (one ingestion path), redacts content and title before they cross into the store, refuses sensitive paths, and serialises adds so the before/after item-id attribution stays correct.

Deleting this source deliberately does not tombstone it, unlike the per-path auto-sources. Those are keyed to one folder, so re-registering a folder the user removed would override an explicit choice. This row is not a place — it is the container for a feature that already has a discoverable off switch (knowledge.auto_add_documents). Making the delete a second, hidden, permanent off switch gives one intent two controls, and the one with no UI wins. So deleting it means "clear what is in here"; the toggle means "stop adding". This matches the sibling Artifacts source.

This replaces the never-built server-side link scanner rather than reviving it. knowledge.doc_ingest_hosts is deliberately not applied: it is SSRF protection for KiroCrew fetching a URL unattended, its default is [] = deny-all, and wiring it here would make the feature ingest nothing while its toggle read on. The agent already fetched the content with its own tools under its own approval; KiroCrew fetches nothing.

5. Dedup on documents, and no duplicate written at all

• enumerate_docs replaces the source-level fallback with one DocRef per (source_id, content_hash) group, so aggregate sources dedup per document like any other. _AGGREGATE_SOURCE_TYPES is gone — with no source-level unit there is nothing to guard against.
• _delete_doc deletes a document's items and marks its owning state row deduped so nothing re-ingests it. It removes the source row only once the source is provably empty (no items, no state rows, not a folder/vault) — which keeps a collapsed one-shot upload from lingering as an empty row without ever taking a source that still holds documents.
• DocRef.key now falls back to the content hash, not "". Every document in one source previously shared a key, so removing one marked them all removed and protected them all from deletion.
• dedup_document takes the just-written document's content_hash. A source id alone is ambiguous once a source holds more than one document; it was picking whichever item came back first.
• A pre-ingest exact-hash gate in IngestionPipeline refuses a byte-identical write on every path, recording a terminal skipped_duplicate job so callers can tell "already present" from "nothing to do". Refusing is not the same as doing nothing: the items the call was going to replace are deleted first, because the document's content changed to something already stored elsewhere and its old items are now superseded. A folder file refused this way is marked deduped, not done — done with an empty group looks like a successful ingest that produced nothing, and the scan would never revisit it.
• A scheduled sweep. Nothing invoked dedup_sweep automatically; it was reachable only from the CLI and an MCP tool. The watcher now runs one every knowledge.dedup_every_n_sweeps (default 12, ~hourly). Because running it on a schedule is a change in kind — deletes that used to need a human command now happen unattended — the first sweep in a process is a dry run that only logs what it would collapse. The gate and the sweep are complements: only the sweep catches a near-duplicate or a pre-existing one, and it needs embeddings so it can never run inline.

6. Config, UI, docs

auto_ingest_doc_links is renamed to auto_add_documents rather than joined by a new key: it already meant "documents encountered while working get added automatically", which is this feature. The loader accepts the legacy spelling (same idiom as agent.yolo → agent.dangerously_skip_permissions) so an existing config's value carries over instead of silently reverting.

New: auto_register_project_docs (true), auto_ingest_chunk_budget (150), dedup_every_n_sweeps (12). All five knowledge keys added to _EDITABLE_CONFIG — a key absent from that allowlist renders a toggle that then fails to save. auto_ingest_artifacts also gets its first frontend surface (it was config-file only), so all three auto-ingest toggles sit together in one Knowledge Library section.

Deviations from the implementation plan

1. enumerate_docs keeps the folder branch. The plan specified one GROUP BY source_id, content_hash for everything. That merges two identical files in the same folder into a single DocRef spanning both — losing the per-file identity _delete_doc needs to mark each row. Only the non-folder fallback is replaced; the folder branch was already per-document and correct.
2. _delete_doc may remove a provably-empty source. The plan asked for a hard assertion that delete_source_cascade is unreachable from dedup. Taken literally, a collapsed one-shot upload leaves an empty source row in the UI until the next boot reaps it. The real invariant is never delete a source that still holds documents, so that is what is enforced (and tested).
3. The items.content_hash backfill (plan stage 5b.1 / A4b) is NOT in this PR. I implemented it, and it was a data-loss bug: it grouped by source_id, so a folder source with N legacy files got one identical hash on all of them, and the newly-automatic sweep then reads them as exact duplicates and deletes N−1. That is the same source-as-document confusion this PR exists to fix, reproduced in the migration. A correct version has to derive per document group, and it is a migration that rewrites every legacy row at process start whose failure mode is silently shredding a knowledge base — so it belongs in its own change, with a dry-run and a count report, not bolted onto this one. Dropping it leaves behaviour at the status quo: legacy null-hash rows reach dedup through the filename+embedding tier exactly as they did before. What IS kept is the durable half — a test asserting every ingest path stamps the column, so the gap cannot grow.

Tests

New: test_knowledge_doc_filter.py (18), test_knowledge_project_docs.py (37), test_knowledge_agent_source.py (43), plus TestKnowledgeAutoIngest in test_config_loader.py (16).

What they lock in, beyond the happy paths:

• docs/**/security.md survives while root SECURITY.md is dropped — the exact paths an unanchored filter was measured destroying.
• The predicate and the scan cannot drift. test_walk_with_project_properties_matches_the_predicate builds a tree, walks it with the real source properties, filters it with should_ingest_doc, and asserts the two sets are equal.
• A pattern containing / still matches when the OS separator is \ — the Windows break above.
• include_extensions=None preserves today's behaviour, set() takes nothing, and an allowlist cannot widen past reader support.
• A project source outside the workspace is still scanned; one whose path was swapped for a link is skipped; a hand-added folder is never budgeted; a repo root that resolves to $HOME is refused.
• The budget stops a sweep and later sweeps finish the remainder with no file lost.
• A refused write leaves no superseded items behind, on both the folder and the aggregate path — verified non-vacuous (both tests fail with the fix reverted).
• The first scheduled dedup pass runs with apply=False and only later passes delete.
• Collapsing one document in an aggregate source leaves its siblings and the source row intact; the collapsed document's state row is marked so a second pass finds nothing.
• Every ingest path stamps items.content_hash.
• Both config spellings, canonical-wins, round-trip settles on the new name, and all five keys are dashboard-editable.

Updated to the new contracts: test_knowledge_dedup.py (three tests whose premise this PR deliberately inverts — aggregate sources are no longer carved out), test_folder_watcher.py and test_perf_boot_path.py (_ingest_file now returns (item_ids, outcome)), test_knowledge_artifact_ingest.py and test_knowledge_ingest_guard.py (_maybe_dedup takes the document's hash).

Manual verification

Full suites locally: 26 896 backend passed, 8 262 frontend passed, mypy clean (635 files, CI-parity venv with no faiss), isort / flake8 / tsc -b clean, all 11 i18n gate checks pass, and a fresh-interpreter import of every touched entry point (pytest's discovery order hides ordering cycles).

32 backend tests fail on this host — all 32 also fail on clean main (sandbox user-namespace EPERM, hardlink and root-owned-binary assumptions), verified by running the same files in the main clone.

The document filter was run over both real trees to confirm the counts still land where they were measured (167 / 105) — a filter regression shows up as a count change long before it shows up as bad search results.

Screenshots

Captured against an isolated pod serving this branch's built bundle. The spec asserted no modal overlay immediately before each shot (Playwright's to_be_visible() passes for an element underneath an overlay), verified all five expected literals, read the numeric input's value, and asserted the pre-rename label Chunk Budget Per Scan is absent so a stale bundle would fail loudly rather than be quietly photographed.

Full Settings → Chat page, in context

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 7d187917f3f9a70b9f787bda0ba4a5c031026e7b — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound dedup redesign and write path, but three unattended behaviors flip on by default at upgrade — spend, persistence, and deletion — with only log-level visibility.

Watch

• Silent upgrade opt-in. auto_add_documents defaults true (the key it renames defaulted false and was inert) and auto_register_project_docs defaults true — "On by default" — so every existing install starts LLM-extracting project repos and granting the agent a persistent write path with no user action. The chunk budget bounds rate, not the decision; a human should consciously own this default.
• Cross-session injection persistence. knowledge_add_document stores whatever the agent "already fetched" from untrusted sources into retrieval for all future sessions; redaction strips credentials, not planted instructions. The library becomes a durable prompt-injection channel — the visible "Auto-added" source and SEL trail are the only mitigations.

Suggestions

• Surface the first-pass dedup preview ("PREVIEW ONLY... will collapse them") in the dashboard, not just a log line — it is the sole human checkpoint before unattended deletes, and log-only means nobody sees it.

[DESIGN-REVIEWED] 7d18791

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7d187917f3f9a70b9f787bda0ba4a5c031026e7b and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/handlers/knowledge.py:588 -- counting "sl.source_id" gives shared sources nonzero badges, but /items?source_id= filters only i.source_id, so expansion returns no items -> Fix: count ownership only until source-scoped listing includes source_locations.
[GPT-REVIEWED] 7d18791

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7d187917f3f9a70b9f787bda0ba4a5c031026e7b: <one-sentence reason>

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

Advisory UX-level review of 7d187917f3f9a70b9f787bda0ba4a5c031026e7b — updated in place on each push; does not block merge.

UX-Verdict: CONCERNS

Solid copy overall, but toggle clicks give no visible response on slow/failed saves, and two strings quietly invert what users will expect.

Watch

• Knowledge toggles don't flip until the server round-trips, and failures announce off-screen. knowledgeMut has no optimistic onMutate (unlike sibling tipsMut/dashMut in the same file), and its onError writes to the top-of-page saveError banner while the Knowledge Library card sits well below the fold (see settings-chat-knowledge-library-in-context.png) — a failed click looks like a dead switch with no visible reason. Moderate frequency × confusion × every failure. Fix: optimistic flip + rollback, matching tipsMut.
• 0 on a field labeled "Auto-Ingest Limit Per Scan" means unlimited, stated only inside the "?" tooltip ("0 removes the bound"). A user entering 0 to stop auto-ingest gets unbounded LLM-extraction cost — opposite of intent, decision-critical info demoted to a tooltip. Low frequency × high impact × persists until the bill. Fix: put "0 = no limit" in visible hint text, or floor the input at 1 and let the toggles be "off".
• "Added documents appear in one 'Auto-added' source you can remove in a click" invites deleting the source to stop the feature — but by this PR's design the source silently repopulates and the real off switch is the toggle above. User deletes, it comes back, they feel loss of control. Fix the string: "…remove in a click (turn this toggle off to stop new additions)".

Suggestions

• Card mixes three verbs for one family — "Auto-Add Documents", "Auto-Register Project Documents", "Auto-Ingest Limit Per Scan" — and the limit's hint ("automatically-registered source") never says which toggle it bounds; align on one verb and name the governed toggle in the hint.
• In let_the_agent_add_documents_it_reads_while_workin, drop "It reads them with its own tools under your approval — Kiro Crew fetches nothing itself": the agent-vs-Kiro-Crew distinction is internal architecture a user can't parse; the first sentence carries the meaning.

[UX-REVIEWED] 7d18791

[github-actions]

Opus 5 Review — ✅ human override accepted

Reviewed 7d187917f3f9a70b9f787bda0ba4a5c031026e7b — this comment is updated in place on each push.

Human judgment by @NicholasRBowers overrides the Opus 5 finding for 7d187917f3f9a70b9f787bda0ba4a5c031026e7b; the recorded reason is authoritative for this commit.

Verdict recorded from an authorized human decision for commit 7d187917f3f9a70b9f787bda0ba4a5c031026e7b.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7d187917f3f9a70b9f787bda0ba4a5c031026e7b: <one-sentence reason>

[NicholasRBowers]

Dispositions for findings raised on 0b6c0f5daf38f93726e024886cdb0f6292a72abc, now at e5fa13436fcf8675822657fdf7f3a9548eb0c0ae.

Finding	Disposition	Evidence
store.py — legacy backfill assigns one source-wide hash, scheduled dedup then deletes unrelated documents	fixed (by removal)	_backfill_item_content_hashes and its _migrate call are deleted. Confirmed the mechanism: the driving query grouped by source_id, so every file in a folder source received one identical hash; enumerate_docs still built one DocRef per folder_file_state row, and tier-1 matched them all. A correct version must derive per document group, and it rewrites every legacy row at process start — so it is deferred to its own change rather than repaired here. Deviation #3 in the PR body states this.
ingestion.py — duplicate replacement returns before removing the document being replaced	fixed	_skip_as_duplicate now takes old_item_ids and deletes them before returning, at both gates. Two regression tests added (test_a_refused_write_does_not_leave_the_superseded_items_behind, test_a_refused_agent_add_records_state_instead_of_a_dead_group); both were verified to FAIL with the fix reverted and pass with it, so neither is vacuous. The artifact and agent item-state rows also record deduped on this path, so their syncs stop retrying a write the gate will refuse again.

Also addressed from the advisory reviews, since each was a real defect rather than a preference:

• UX — dead-end recovery instruction. undismiss_auto_source has zero callers, so the "re-enable it from the Knowledge page" copy pointed at a control that does not exist and a delete was unrecoverable. Rather than build that UI, the aggregate agent:// source no longer tombstones: the feature already has a discoverable toggle, and a second hidden permanent off switch with no UI was the actual defect. The dismissed status and its 409 are gone.
• UX — untranslated hint + pipeline jargon. The chunk-budget hint was a hardcoded English template literal in a 10-language dashboard (this was also a hard CI failure in the i18n gate). It now goes through the catalog with an interpolated default, and the label is renamed from "Chunk Budget Per Scan" to "Auto-Ingest Limit Per Scan".
• Design — the first scheduled sweep is a bulk destructive event with no dry run. The first sweep in a process now runs with apply=False and logs what it would collapse; later sweeps apply. The backfill half of that concern is moot now that the backfill is gone.

Separately, a Windows CI failure exposed a real bug in this change: _walk compared backslash-separated relative paths against forward-slash patterns, so the new *.egg-info/* rule could never match on Windows (and the pre-existing sub/* style was latently broken). The path is now normalized before fnmatch, with a regression test.

[NicholasRBowers]

Second round, now at ceacce35608af98a066111b277bdeca58a3d546a. The two blocking findings from 0b6c0f5d were already fixed in e5fa1343; this revision closes two further findings a local Opus mirror raised against the same commit.

Finding	Disposition	Evidence
dedup.py — _source_is_now_empty reads artifact_item_state / agent_item_state without the try/except its sibling _doc_labels wraps the same reads in; a raise propagates out of _delete_doc after delete_items_batch has committed	fixed	Both that helper and _delete_doc's own marker-table loop now degrade instead of raising: the emptiness check answers False (a lingering empty source row is strictly safer than guessing), and the marker loop skips one table (costing a re-ingest next pass rather than the whole sweep). New test test_a_state_table_read_failure_cannot_abort_the_sweep proxies the store to fail exactly those reads; verified non-vacuous — it raises with the guard removed.
AGENTS.md comment convention — historical narration ("was wrong", "previously", "was not actually true") instead of present-tense current behaviour	fixed	Rewrote the rationale blocks in dedup.py, watcher.py, doc_filter.py and ingestion.py to state the invariant in present tense while keeping the why the convention explicitly permits. Example: "Grouping by SOURCE was wrong for any source holding more than one document" → "A source is not a dedup unit. For an aggregate source one unit per source carries only its FIRST item's hash…". The store.py instance the reviewer cited is gone with the backfill.

On the reviewer's note about mixed provenance in content_hash: agreed that two derivations coexisting in one column is not itself the defect, and that per-document derivation would make the docstring's argument true while per-source makes it false. That is precisely why the backfill is removed from this PR rather than repaired in it — a correct version needs the per-document grouping, and it rewrites every legacy row at process start, so it belongs in a change where that is the subject and can carry a dry-run. Deviation #3 in the PR body records this.

[NicholasRBowers]

Round 3, now at 9468b3e3904c5cc4b6037d570c0fea616c2a898e.

Finding on ceacce35	Disposition	Evidence
agent_source.py:195 — path reads bypass the guarded reader; an LLM-supplied PDF/DOCX path decodes as text and stores binary garbage	fixed	Legitimate and reachable: _read_local_document called read_text(errors="replace") on whatever path it was given, so a .pdf or .docx would have been stored as replacement characters — searchable, useless, and paid for with an extraction call per chunk. Extraction now goes through FileReader, the same reader a folder source uses, so the extension decides how the file is read (pdfplumber / python-docx / html-to-prose). Redaction and the single ingest path are preserved: the reader's OUTPUT is redacted and handed to ingest_file.

Two adjacent defects that the same read path carried, fixed in the same change:

• An extension no reader handles was decoded anyway. Now refused, with a message telling the caller to pass the text as content instead of guessing at the bytes.
• The size guard was bypassed. IngestionPipeline.ingest_file deliberately checks knowledge.max_ingest_file_mb before reading, so reading the file here skipped it entirely and a multi-hundred-MB file would be pulled into memory before anything looked at its size. The check is now repeated at this read.

Four tests added (test_binary_document_is_not_stored_as_mojibake, test_pdf_text_goes_through_the_reader, test_unsupported_extension_is_refused_with_a_usable_message, test_oversized_file_is_refused_before_it_is_read). Verified non-vacuous: three of the four fail against the previous text-decoding read.

Gates on this head: 26,908 backend passed with zero new failures against this host's pre-existing baseline, mypy / isort / flake8 clean. Screenshot URLs re-pinned to the new SHA.

[NicholasRBowers]

Round 4, now at c7684f301e616a28275ed2c35a00b04596cd4e24. Both findings on 9468b3e3 accepted and fixed as proposed.

Finding	Disposition	Evidence
agent_source.py:221 — path reads bypass the TOCTOU-safe hook; a symlink swap after resolve/stat makes an auto-approved knowledge_add_document(path=...) follow and persist credential contents	fixed by removing path, as the finding recommended	This code path has now drawn a blocking finding in two consecutive rounds (binary decode, then TOCTOU), which is the signal to change the design rather than patch it again. path is gone from the MCP tool, its schema (content is now required), the HTTP route, and add_agent_document; document_slug keys on the title alone. The capability is not lost: the agent reads the file with its own tools — under their approval and audit, with no check-to-open window here — and passes the text. Documents that arrive fetched are text to begin with, and documents living in the user's project are covered by project-docs registration, which scans through the guarded folder path. Removing it also retires the extension and size-guard duplication that came with owning a reader.
project_docs.py:99 — active project scans ingest file symlinks pointing outside the repository	fixed	Reproduced before fixing: a tree containing docs/runbook.md -> ../../private/runbook.md walked the link and resolved outside the root. os.walk does not descend a directory symlink, so the hole is specifically file symlinks, which are followed on open. _walk gains confine_to_root, wired from a source property: a file whose resolved path lands outside the registered root is skipped, compared with commonpath so /repo-evil cannot pass as inside /repo. Off for a folder the user registered by hand — following a link they placed there is their choice — and on for an auto-registered source, where nobody confirmed the scope. That asymmetry is the point: this source is created without confirmation, so content from outside the registered tree is never what was asked for.

Tests: TestRootConfinement (4 tests — the escape is skipped, a hand-added folder still follows its links, project sources turn confinement on, and a sibling-prefix path is not treated as inside) plus test_there_is_no_path_parameter and test_content_is_required. Verified non-vacuous: the confinement test fails with the property flipped off.

Gates on this head: 26,897 backend passed; the only failures are this host's pre-existing sandbox set, each confirmed failing on clean main. mypy / isort / flake8 clean. Spec (docs/system-specs/modules/knowledge.md) reconciled for both changes in the same revision, and the screenshot URLs re-pinned.

[NicholasRBowers]

Now at 580b554428289a941adbbf0d334748b807b97664, rebased onto current main.

CodeQL py/clear-text-logging-sensitive-data (ingestion.py) — SUPPRESSED, with the warrant stated as a boundary invariant.

Two prior attempts did not clear it. First, log_name = _redact(display_name) or display_name carried a dead or fallback — _redact returns its input unchanged when falsy, so the fallback could only ever re-yield the same empty string while handing taint analysis a genuine unredacted edge. That is removed. Second, the repo's existing marker form (# lgtm[...], precedent at src/kiro_crew/history.py) is not honoured by the current engine, so the suppression is now # codeql[py/clear-text-logging-sensitive-data] on the flagged statement.

The warrant is deliberately this function's boundary, not caller provenance: log_name is derived once, immediately after display_name, and every message and audit sink in the function consumes it — the sensitive-path SEL event and its PermissionError, the oversized warning, and the oversized SEL event. No sink reads the raw value, so the property holds for every ingest path into the function and for any added later. A provenance-based warrant ("these callers sanitise upstream") would decay as callers are added, and the suppression is precisely what would stop the scanner reporting that decay.

Two sinks beyond the reported one were fixed earlier in the same pass and carry no suppression: the sensitive-path event and its raised message previously embedded the raw name.

Backend Tests (Windows) (3) — flake, not addressed by code. test_open_slots_persistence.py::test_flush_during_async_restore_does_not_truncate_snapshot failed assert 7 == 8. Neither that test nor session-persistence code is in this diff (git diff --name-only origin/main...HEAD), and the assertion is a count race in a concurrent flush/restore test. Superseded by this push.

Also cleared this round: the Brand Name Gate and Docs Lint gates that landed on main mid-flight. 14 added lines used the joined product name in prose (fixed at source, including the i18n catalog — which then required updating nine locale translations, since the product name is a do-not-translate term and the glossary test counts a translation that drops it); and two illustrative doc paths in comments were being read as real citations, now reworded.

Gates: pytest with no new failures, isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, tsc -b, vitest 8725 passed, i18n 7/7.

[NicholasRBowers]

Now at e11675db0417ab8e2b8ec3c9902fcc260a772c7d.

CodeQL py/clear-text-logging-sensitive-data — resolved by removing the flow, not by suppressing it.

Three suppression attempts failed and I stopped trying to silence the query. For the record, so nobody repeats them: removing the dead or display_name fallback did not clear it (though that fallback was a real defect on its own — _redact returns its input unchanged when falsy, so it could only ever re-yield the same empty string while handing taint analysis a genuine unredacted edge); the repo's existing # lgtm[...] marker form is not honoured by the current engine; and # codeql[py/clear-text-logging-sensitive-data] on the flagged statement was also not honoured — the annotation came back on the same line with the comment in place.

The query was right about the shape of the code even though _redact does sanitise. Of the three sinks the document name reached, the application log is the one with no redaction contract and the widest reach — log files, shipping aggregators, anyone with host access — while the other two are bounded: the error goes back to the caller that supplied the name in the first place, and the SEL record is the audit trail, which is where a redacted name belongs.

So the name no longer goes to the log at all. logger.warning now names the size, the configured limit and the source_id, which is what an operator needs — what to raise and which source to look at. The full name stays in the FileTooLargeError raised to the caller and in the SEL event. log_name now flows only to two sel() records and two raised exceptions; no logging sink, and no suppression marker anywhere in the file.

The test now asserts the invariant rather than the message. test_oversized_file_raises_with_actionable_warning previously asserted the filename appeared in the log warning — that was the old contract, and it failed when the flow was removed, which is how I caught it. It now asserts the security property in three parts: the name IS in the exception, the name is NOT in any log record, and the log still carries max_ingest_file_mb and source_id= so it stays worth reading. Proven non-vacuous — restoring logger.warning(msg) fails the exclusion assertion.

Suite: 47 failures, every one inside the pre-existing environmental families and zero in the knowledge area. Clean main at the same base fails 48 in the same families, 29 of them in src/kiro_crew/apps/builtins/ops_mission_control/tests/test_ledger_sync_git.py — that file landed on main a few minutes before this push and is flaky under -n auto (it passes 39/39 in isolation on both trees and does real git init/commit/push). Flagging it here because it currently makes any PR's local suite look broken; it is not this branch's.

Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 203 knowledge tests.

[NicholasRBowers]

Now at 831369cbf88ed4398e49b3c3e8a3b768ff14463d. Both blocking findings were real and are fixed. Both were in the adoption code added one push earlier, and they are the same defect wearing two faces, so they are fixed together behind one helper rather than patched where they surfaced.

1. store.py:653 — ownership transfer left the recipient's state row detached. Correct.

delete_items_batch reassigned items.source_id to a surviving holder but never told that holder's state row. item_ids is the only list a source's delete path consults, so the recipient owned an item its own row did not name — and removing that document later dropped an empty group while the content stayed searchable. That is precisely the strand this model exists to prevent, which is what makes the finding land: I had fixed this shape in the cascade's revive path and left the item-level path with the same hole.

2. store.py:813 — aggregate documents were revived with a status from the wrong vocabulary. Also correct, and the consequence named is right.

The revive branch wrote status = 'done' for all three state tables, but the vocabularies are not interchangeable and I had assumed they were. folder_file_state is scan-driven and moves pending → done; artifact_item_state and agent_item_state are push-driven, default to 'active', and only ever hold 'active' or 'deduped' — there is no 'done'. Since find_document_by_hash matches on 'active', an adopted agent document became invisible to the intra-aggregate duplicate gate, and identical content would land again under a second source_uri. Verified against the schema and every writer, not just the two call sites.

The fix. One module-level _DOC_STATE_TABLES pairs each table with its own healthy status and carries the reason they differ, and one _adopt_reassigned_item(item_id, new_source_id) does the adoption — matching on content_hash because that identifies the document independently of which source holds it, appending rather than replacing so a chunked multi-item group is not truncated to one, and clearing any deferral marker since a row that owns an item is no longer deferring. Both reassignment sites now call it, so a third such site cannot reintroduce this.

The revive branch also gained a distinction it was missing. When the content is genuinely gone, a folder row goes to 'pending' so the next walk re-ingests it — but an aggregate row has no scanner to revive it, so it keeps 'deduped' and only loses the marker. Promoting it to 'active' while it owns nothing would make the duplicate gate refuse a re-add of content the Library does not actually hold, which is the inverse of finding 2 and would have been a fresh bug.

Tests. Two added, each proven non-vacuous by reverting its own fix in isolation: test_an_adopted_aggregate_document_stays_visible_to_the_duplicate_gate (asserts 'active', and that find_document_by_hash then sees it) and test_reassigning_an_item_tells_the_new_owner_state_row (asserts the recipient's row names the item it now owns).

Suite: 48 failures, exactly matching clean main at the same base, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 205 knowledge tests.

Separately — the two shard failures on the prior head were flakes, not findings. Backend Tests (3.12, 3) and Backend Tests (Windows) (3) each failed one timing test while Backend Tests (3.10, 3) passed the identical test set (shards split by test id), and both tests pass locally on this branch and on clean main. Neither test nor its subject is in the diff, and running the knowledge tests immediately before them in one process passes 153/153, so there is no order dependence from this PR. Coverage Gate failed closed only because coverage-combine was skipped downstream of those shards. I reran the failed jobs; this push supersedes them.

[NicholasRBowers]

Now at 494647b541eec4996f20ce432f7ac0858f1a6aa4. Both blocking findings were real and are fixed. One is fixed by a different mechanism than the one suggested, for a specific reason set out below.

1. folder_watcher.py:455 — symlink race bypassed project-root confinement. Correct, fixed as described.

_walk validated the resolved path, but _ingest_file re-resolved and then checked only sensitivity, so a link retargeted in between could pull in any non-sensitive file on the host. _ingest_file now re-checks confinement against the source root as well, and the root is threaded in from the scan rather than re-derived. Per the second half of the suggestion, the pipeline is now handed the validated resolved path instead of re-deriving it downstream — re-deriving there would reopen the window the check just closed. The display name still comes from the logical path, so a symlinked document keeps the name the user sees in the folder.

2. dedup.py:577 — a deleted duplicate retained stale ownership. The problem is real. The suggested fix would have caused a worse bug, so the same hole is closed a different way.

The mechanism as described holds: a losing file's row is 'deduped' with an empty group, so _handle_deleted has no ids to detach, the source stays a location of the winner's items after its own file is gone, and a later winner deletion hands it a document with no file behind it — stale searchable content.

Storing winner.item_ids in the loser's row would fix that symptom and introduce a data-loss bug. item_ids means "the items this row owns", and dedup derives a document's identity from whatever it points at. A row naming another source's items becomes a second document over one physical item set; the two DocRefs differ only in the source element, so _match_reason's a.key == b.key guard misses, they exact-match at Tier 1 with certainty, and the collapse deletes the surviving copy. That is why the deferral marker in this PR is a source id (merged_into_source_id) and never an item list.

So the claim is released by content hash — the only handle such a row legitimately has — via a new detach_source_location_by_hash(source_id, content_hash). It deletes only source_locations rows; the items belong to the winner and are untouched. _handle_deleted calls it precisely when there is no group to detach, which is exactly the deduped case. Net effect is the one the finding asked for: after the losing file is deleted, that source is no longer a candidate to inherit the document, and deleting the winner now removes the document instead of stranding it.

Two tests, each proven non-vacuous by reverting its own fix: test_a_path_that_escapes_the_root_is_refused_at_ingest_time (asserts the pipeline is never awaited for an escaping symlink) and test_deleting_a_deduped_file_releases_its_claim_on_the_winner.

Adding the root parameter also surfaced four test doubles that stubbed _ingest_file with a fixed arity — two in test_knowledge_agent_source.py, two in test_perf_boot_path.py. They were updated to the real signature rather than the signature being bent to fit them; the failures were genuine notice that the doubles had drifted from the contract.

Suite: 47 failures, all in the pre-existing environmental families, zero in the knowledge area; clean main at the same base fails 48. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint.

On E2E (stub ACP backend, offline) — the i18n render-time gate failure is a measurement flake, not this branch. It reported artifacts.layout: 0 → 2, two layout/over-budget-truncation findings for the Gallery and Table labels at SegmentedControl.tsx:136 via ArtifactsPage.tsx:1833. Verified against the gate's own base ref (57e7df81, which is exactly this branch's merge-base): both component files are byte-identical between base and head, and both labels' en-XA values are byte-identical ([Ğàĺĺèŕý ···········], [Ţàƀĺè ········]). Identical input on both sides, different result. The mechanism is visible in the component: the label lives in an AnimatePresence/motion.span animating width: 0 → auto on a 0.2s spring, so the measured width depends on when the snapshot lands, and one of the two findings sits exactly at its budget (1.90x against a 1.9x budget). This push re-runs it.

[NicholasRBowers]

Now at 60791f6eb82338a44a2a8b311510cf023d625a08. Both blocking findings were real and are fixed.

1. mcp_core.py:5922 — an unredacted title reached chat. Correct, fixed as described.

The handler already computed audit_title for the SEL rows but interpolated the raw title into the string returned to the agent. That string is rendered into chat and persisted in the transcript, which is a wider audience than the audit log that was already taking the redacted form — so the sink with the broadest reach was the one left unredacted. Now uses audit_title. There is no cost to this: _redact returns an ordinary name unchanged, which the second test pins.

2. ingestion.py:185 — the duplicate shortcut dropped source ownership. Correct, and it is the same invariant this PR is built on.

The gate refused the write, deleted the caller's superseded items and recorded a terminal duplicate job — but never recorded that the refusing source has a copy. Under "one document, many locations" that is exactly the thing that must be written down: the copy was invisible to the reference count, so deleting the holder destroyed the only items while the second folder's file was still on disk, and nothing would bring the content back.

I fixed it by attaching the location rather than by removing the shortcut. Removing it would work, but it would spend a full extraction pass on every duplicate file, which is the specific cost the gate exists to avoid — and the auto-registration paths in this PR make duplicates common (a repo and its worktrees, the same design doc in two project folders). Attaching costs one INSERT and makes the refusal safe, so the outcome the finding asked for is reached without giving up the saving: the refusing source is now a holder, and deleting the first source moves the document to it instead of destroying it. The regression test asserts exactly that sequence.

This also composes correctly with the two mechanisms added last round: the refused source's row is the 'deduped'-with-empty-group shape, so if its file is later deleted, detach_source_location_by_hash releases the claim, and if the holder is deleted, _adopt_reassigned_item gives the row the items it inherits.

Tests: test_the_duplicate_gate_records_the_refusing_source_as_a_holder and, in a new test/test_mcp_knowledge_add_document.py, test_a_credential_in_the_title_does_not_reach_the_chat_result plus test_an_ordinary_title_is_returned_unchanged. Each proven non-vacuous by reverting its own fix in isolation.

Suite: 48 failures, matching clean main exactly at the same base, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint.

[NicholasRBowers]

Now at 62afe97c6dd3c0f48b7f816619d12f768ee7d741. Both blocking findings were real and are fixed exactly as suggested — both are the two helpers this PR already has, applied at call sites I had missed.

1. store.py:844 — the cascade reassigned without adopting. Correct.

I added _adopt_reassigned_item to the item-level path (delete_items_batch) two rounds ago and reasoned that delete_source_cascade was already covered by its revive loop. It is not: that loop only reaches rows that carry merged_into_source_id, and a row can hold a location without ever carrying a marker — the pre-ingest duplicate gate writes precisely that shape (status='deduped', empty group, no marker, plus a location on the holder's items), and it is the shape the previous round's fix made common. So the recipient owned an item its own row never named, and deleting that file then left the item searchable with no state and no locations. Now adopts at the reassignment itself.

2. artifact_ingest.py:352 — removing a deduped artifact retained its claim. Correct.

Same shape as the folder-file case fixed earlier, in the artifact path: with an empty group there was nothing to detach, so the artifact source stayed a location of the winner's items after the artifact was gone, and a later winner deletion would hand it a document with no artifact behind it. remove_artifact now calls detach_source_location_by_hash with the state row's hash when the group is empty. _prev_hash was already being read and discarded; it is now used, as suggested.

Together with the previous rounds this closes the pattern at all four sites where ownership moves or a copy goes away: item-level delete, source cascade, folder-file delete, artifact delete.

A pre-existing test of mine failed, and its premise — not the fix — was wrong. test_a_deduped_row_does_not_block_a_later_retry asserted that re-adding after the holder's deletion must land, on the stated grounds that "the Library does not hold" the document. Since the gate now records a location, deleting the holder moves the document to the agent source instead of destroying it, so "unchanged since last add" became the truthful answer and the old assertion was asserting the bug. The test now covers both: the moved case (row goes active with the inherited item, re-add correctly refused) and the guard it was written for (claim released and items genuinely gone → a stale hash must not read as unchanged, re-add lands).

Tests: test_a_gate_refused_row_adopts_the_item_the_cascade_hands_it (builds the unmarked gate shape explicitly) and test_removing_a_deduped_artifact_releases_its_claim_on_the_winner. Each proven non-vacuous by reverting its own fix in isolation.

Suite: 48 failures, matching clean main exactly at the same base, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 212 knowledge tests.

Unrelated infrastructure note: Publish readiness signal failed once on the prior head with gh: Label does not exist (HTTP 404) while transitioning to readiness: checking — two concurrent instances of that workflow racing on the same label set, not a signal about this branch.

[NicholasRBowers]

Now at 4cbe822a7aee0b8eef31aa28568871a9483fed9e. The finding was real and is fixed, plus a sibling instance you did not name.

folder_watcher.py:237 — an edited deduped document kept its claim on the old content. Correct.

A claim is specific to the content it was made for. A source that lost a dedup owns no items but IS a location of the winner's; when its own copy is then edited, that claim points at the wrong document. Deleting the holder afterwards hands this source the superseded text, which stays searchable with nothing behind it. Exactly as described.

Fixed as one rule in the store, not three checks at three ingest paths. release_stale_claim(source_id, prev_hash, new_hash, prev_item_ids) encodes the whole condition once — fire only when the row owned nothing (a row with a live group replaces its own items through the normal delete-and-reingest path, and releasing its claim there would drop a legitimate co-ownership) and only when the hash actually moved. It is now called from all three paths you named: the folder scan before an edited file is re-ingested, ingest_artifact before a changed artifact lands, and add_agent_document before a changed document under the same source_uri lands. Putting the predicate in one place is deliberate — it is the part that would rot if each path re-derived it, and this PR has already had four rounds of exactly that failure mode.

Sibling instance fixed at the same time: agent_source.remove_document. It had the identical gap remove_artifact had a round ago — if item_ids: delete_items_batch(...) with no else-branch, so removing a deduped agent document left its claim on the winner behind. Same one-line detach. That completes the sweep: every path that either moves ownership or gives up a copy now either adopts or releases.

Three tests, each proven non-vacuous by reverting its own fix: the edit case (claim released, and deleting the holder then takes the superseded text with it); a negative test pinning that the rule does NOT fire for a live group, an unchanged hash, or a missing prior hash — the three ways an over-eager version of this would destroy real co-ownership; and test_removing_a_deduped_agent_document_releases_its_claim.

Suite: 47 failures against clean main's 48 at the same base, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 194 knowledge tests.

[NicholasRBowers]

Now at fc39f91af887ec35305987c726c1ae7bb375193c. Both blocking findings were real. The first is fixed exactly as suggested; the second is fixed by a narrower mechanism that removes the same risk without disabling the feature, and the tradeoff is stated below for the repo owner.

1. dedup.py:364 — identical aggregate documents lost independent ownership. Correct, and it was this PR's own invariant broken one level down.

The comment above that query already claimed "one DocRef per DOCUMENT", but the grouping was (i.source_id, i.content_hash) — which is not per-document once an aggregate holds two distinct documents with identical text. Both fused into one reference spanning both item groups, so a later collapse treated them as a single thing and removing one artifact took the other's indexed copy with it. Same source-as-document confusion the PR exists to remove, just at the document level.

Aggregate documents are now enumerated from artifact_item_state / agent_item_state by slug and item_ids — those tables are the per-document registry, so they are the authority. Items no state row claims (legacy rows, source types with no registry) still fall back to the hash grouping over the unclaimed remainder, so coverage does not regress.

2. watcher.py:356 — the scheduled sweep applied fuzzy matches unattended. The risk is real and is now closed.

preview = not self._dedup_applied_once meant only the first scheduled pass was a preview; every later one applied. Since a collapse deletes the loser's own copy, a wrong filename-plus-cosine match costs that document its unique text with nobody watching — two same-named weekly reports can clear the threshold while stating different facts. Model A protects a document from source deletion, not from a false match, so there was nothing else standing behind this.

Fixed by making the unattended pass certainty-gated rather than preview-only. dedup_sweep gained certain_only, and the scheduled sweep passes it: exact-content matches apply, fuzzy candidates are found and reported but never acted on. The reason this is preferred over apply=False is that the duplicates automatic registration actually produces are exact ones — the same file in a repo and each of its worktrees, the same design doc in two project folders — and those are facts, not judgements. Preview-only would leave that class permanently uncollapsed and require a manual step to get any benefit from the feature, while gaining nothing on the risk that matters. Fuzzy collapses remain available deliberately through the CLI and the knowledge_dedup tool, which already default to a dry run.

Flagging the tradeoff explicitly since it is a behaviour decision on the repo owner's feature: unattended fuzzy collapse is now off, unattended exact collapse stays on. If the intent is that nothing at all is deleted without a human in the loop, the one-line change is certain_only=True → apply=False at that call site.

Tests, each proven non-vacuous by reverting its own fix: test_two_identical_artifacts_stay_independent_documents (asserts two units with disjoint item groups, where the old grouping produced one) and test_certain_only_applies_exact_matches_and_only_reports_fuzzy (asserts only the exact action collapses while both are still reported).

Suite: 46 failures against clean main's 48 at the same base, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 217 knowledge tests.

[NicholasRBowers]

Now at 1815db0227abe39e00fc8b02d25d0db09050577a. The finding was real, reproduced, and is fixed exactly as suggested.

dedup.py:479 — same-source de-dup could strand an existing file. Correct.

Reproduced before changing anything, with two identical files in one watched folder (a LICENSE and a vendored copy of it): the sweep applied one collapse, the loser's row went to deduped with an empty group, and removing the survivor from disk left nothing searchable while the duplicate file was still sitting there.

The reason it is unrecoverable is structural, and it is worth naming because it explains why a guard is the right answer rather than better bookkeeping. Every protection this PR adds is a location in another source. Within one source there is no second holder, so delete_items_batch finds nothing to hand the copy to and destroys the items; the deferral marker names the very source the row already lives in, so the revive hook — which fires when the marked source is deleted — can never run; and the mtime gate keeps an unedited file from ever being re-read. Three mechanisms that normally catch this all no-op at once.

_match_reason now returns None when a.source_id == b.source_id.

Nothing legitimate is given up. Two identical files in one folder are two files, and indexing both is honest — the space a collapse saves is not worth losing the document. Duplicates within an aggregate are already refused earlier and more cheaply by the pre-ingest gate's find_document_by_hash, which writes no state row at all, so the sweep never needed to cover that case.

Test: test_two_identical_files_in_one_folder_are_never_collapsed asserts no collapse, both item groups alive, both rows still done, and no marker naming its own source. Proven non-vacuous — with the guard disabled it fails on two files in ONE source must never be collapsed.

Rebase: this push also absorbed 22 commits of main, with one import-block conflict in dashboard/handlers/knowledge.py resolved as the union (main's data_home plus this branch's _slot_project_snapshot; both verified used).

Suite: 47 failures, all in the pre-existing environmental families, zero in the knowledge area. One extra flake appeared and was ruled out: test_brand_name_gate.py::TestUrlBoundary::test_many_brand_names_on_one_line_stay_linear is a wall-clock ratio assertion, is not in this diff, and passes 3/3 in isolation — it only fails under 16-worker load. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint, 218 knowledge tests.

[NicholasRBowers]

Now at 7983bbdc5267f28b66ab85e91eb63c0f9ebdd441. Both blocking findings were real. The second is fixed as suggested; the first is fixed by a different mechanism, because the suggested fix reverses a deliberate product decision — details and the one-line alternative below.

1. store.py:669 — hash-only adoption conflated distinct documents. Correct, and it was fallout from the previous round.

_adopt_reassigned_item looped over every state row matching (source_id, content_hash) and appended the item to each. Making aggregate documents per-document (round 18) is exactly what makes two rows legitimately share a hash, so one physical item landed in two groups and removing either document would delete it and take the other's indexed content with it.

Adoption now acts only when the hash picks out ONE row; an ambiguous hash adopts nothing. The asymmetry is deliberate: an un-adopted row leaves a stale claim, which is visible and recoverable, whereas a cross-wired group destroys content on the next delete.

The same ambiguity existed in the mirror-image helper, which you did not name, and is fixed too. detach_source_location_by_hash released the claim for every row matching the hash, so removing one of two identical documents dropped the claim the other still needed — stranding it when the winner went away. It now keeps the claim when more than one document in that source shares the content, on the same reasoning: a stale claim beats a destroyed document.

2. project_docs.py:99 — auto-registered project documents bypassed approval. The exposure is real; the fix is different.

Verified the substance first. _redact in the ingest path is documented as "Redact LLM-derived text before storing" and is applied to extraction output only — the raw document text is not scrubbed before it reaches the extraction worker. The artifact path already scrubs its body via _redact_for_ingest; the folder path does not. So a credential in a project runbook did reach the worker, and on this path the user never chose the folder.

pending_confirmation is not available as the fix: auto-registration with no confirmation step is an explicit product decision by the repository owner (the whole point of the feature is that project docs become searchable without adding the folder by hand), and autosource.py already documents why active is seeded rather than pending_confirmation — no user is present to confirm.

So the exposure is closed at the point that actually leaks: a source marked auto-added has its document text scrubbed before hashing, chunking, extraction and storage — the same scrub, in the same position, the artifact path already uses. The credential never reaches the worker or the index, and the zero-friction registration the feature exists for is preserved. Scoped by AUTO_ADDED_PROP read from the source row, so every auto path (project docs, the agent aggregate, the auto-registered drop folder) is covered and a hand-registered folder is untouched — silently rewriting the indexed text of folders users chose themselves would be a behaviour change well beyond this PR.

If the owner would rather have the confirmation gate, it is one line: seed "sync_status": "pending_confirmation" in project_source_properties(). Flagging it rather than deciding it.

Tests, each proven non-vacuous: test_an_ambiguous_hash_adopts_nothing_and_keeps_the_claim (asserts neither group is written and the claim survives); test_an_auto_added_source_scrubs_secrets_before_extraction (asserts the credential is absent from stored items and from what extract_batch was handed — the call that leaves the machine); and test_a_hand_registered_source_is_left_alone, which pins that the scrub does not widen to existing sources.

Suite: 45 failures, all in the pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 713 files, docs-lint, brand gate, scrub-lint.

Backend Tests (Windows) (1) — flake, not addressed by code. test_acp_runtime.py::TestAcpRuntimePidTracking::test_kill_untracks_pid failed assert [] == [4242]. Neither the test nor ACP runtime PID tracking is in this diff, and it is a different Windows test from the two that flaked on earlier heads (test_open_slots_persistence, test_platform_compat) — a fresh timing assertion each attempt is the loaded-runner signature, not a defect. Superseded by this push.

[NicholasRBowers]

Now at b16434cf5b07ba049c9f53f9d49ee7a8391ba14f. The finding was real and is fixed. One remaining red check is a breakage on main, not from this branch — evidence below.

dedup.py:624 — co-owned loser items survived the collapse. Correct, and it was an interaction with this PR's own mechanism.

Collapses chain. If source C lost an earlier round to B, C is a location of B's items while owning nothing. When B then loses to A, the old code attached only B's source to A's items and called delete_items_batch(loser.item_ids, owner_source_id=loser.source_id). That call means "this ONE source's copy is gone", so with C still holding the items it degraded to a detach: the loser's items stayed alive and searchable under C, and the sweep reported the duplicate as collapsed. Two live copies of identical text, which is precisely what dedup exists to remove.

Fixed as suggested. Every source that can reach the loser's copy — the loser plus any source that previously deferred to it — is now attached to the winner's items first, and only then are the loser's items hard-deleted with no owner_source_id. add_source_location is OR IGNORE, so re-attaching an existing pair (commonly the winner's own source) is a no-op. The unconditional delete is safe here precisely because item_ids means "the items this row owns" and no second state row may name them, so the only other references were the location claims just migrated.

Test: test_a_co_owned_loser_leaves_no_second_copy builds the three-source chain and asserts the loser's item is gone, not merely detached, and that C now points at the winner. Proven non-vacuous — restoring the owner_source_id argument fails it on exactly that assertion (assert 1 == 0, "the loser's duplicate item must not survive").

Backend Tests (3.10, 2) / (3.12, 2) / (Windows) (2) — a main breakage, not this branch.

All three shards fail one repo-wide guard:

test_lazy_data_home_paths.py::TestNoImportTimePathResolution::test_no_module_level_path_constants
  src/kiro_crew/apps/builtins/spec_builder/tests/test_routes.py:88
  [module] _REAL_STATE_DIR = ..._state_dir()

That file is not in this diff (git diff origin/main...HEAD -- '*spec_builder*' is empty). It arrived with the Spec Builder builtin (#518), and the guard fails on a clean main checkout at 794aeed7f with an empty working tree — verified directly, not inferred. It is red for every PR based on that commit, and this branch merely inherits it; it cannot go green here until the module-level constant is converted to the accessor pattern the guard's own message prescribes. Deliberately not fixed in this PR: it belongs to another feature and a fix here would collide with the owner's.

Local suite otherwise: 47 failures = the pre-existing environmental families plus that one guard, zero in the knowledge area. Gates: isort, flake8, mypy 719 files, docs-lint, brand gate, scrub-lint.

E2E (stub ACP backend, offline) remains the known artifacts.layout 0->2 i18n render-budget flake — both component files and both en-XA values are byte-identical between the gate's base and head; the label width is animated and one finding sits at its 1.9x budget.

[NicholasRBowers]

Now at b16434cf5b07ba049c9f53f9d49ee7a8391ba14f. One blocking finding is OPEN and NOT fixed — recording it here deliberately, because the review's own comment shows only "review incomplete" and the finding text survives nowhere but the workflow log, which ages out.

Recovered finding — store.py:678, state and item hashes use different domains. It is correct, and it is mine.

The GPT job's steps all succeeded but it never emitted a [GPT-REVIEWED] marker, so the gate failed closed and the PR comment reports no verdict. The verdict was produced; it is in the job log:

BLOCKING -- src/kiro_crew/knowledge/store.py:678 -- State and item hashes use different domains
PDF/DOCX/HTML duplicate -> state stores the input hash while items store extracted-text hash
-> source deletion cannot adopt or detach reassigned chunks, leaving stale or duplicate searchable items.

Verified directly rather than taken on faith:

• folder_file_state.content_hash is written from _hash_file() — sha256 over the raw file bytes. That is right for its actual job, change detection: deriving a text hash there would force an extraction pass on every scan, which is exactly what the mtime/hash gate exists to avoid.
• items.content_hash is sha256(text.encode()) over the extracted text.

For .md/.txt the two coincide, which is why every test covering this machinery passes — the fixtures are all plaintext. For anything the reader transforms (PDF, DOCX, HTML) they differ, and every state↔item hash lookup silently misses.

Scope, checked per call site rather than assumed:

Path	Domain	Status
agent_item_state, artifact_item_state	sha256(text) — same as items	correct
folder_watcher.py:451 → detach_source_location_by_hash	bytes hash vs items.content_hash	misses
release_stale_claim (store.py:763)	bytes hash from a state row	misses
_adopt_reassigned_item	item text hash vs all three tables	correct for aggregates, misses folder rows

So the location bookkeeping added in the earlier rounds is inert for non-plaintext folder documents. In user terms: the same PDF in two synced folders is collapsed to one copy; later removing the winning folder cannot hand the survivor to the other folder, because the two rows are keyed in different domains — the document either stays searchable with no file behind it, or vanishes from a folder that still has it on disk.

Not fixed in this push, and not going to be guessed at. The two candidate designs differ in blast radius, and picking one is a scope call for the repository owner:

1. Give folder state rows a second column holding the extracted-text hash, written at ingest when the text is already in hand, leaving content_hash as the change-detection bytes hash. Uniform and domain-correct everywhere — but a schema migration, and the existing hash-less rows need a null-tolerant path. A per-source content_hash backfill was already written and removed from this PR once for being a data-loss bug, so this is exactly the ground that has bitten before.
2. Keep comparisons within one domain by passing the state-domain hash in from callers that already hold it, and resolving items through item_ids / source_locations rather than items.content_hash.

Escalated to the owner rather than started unattended. The three shard-2 failures remain the separately-reported main breakage (test_lazy_data_home_paths naming Spec Builder's test_routes.py:88, proven failing on a clean main checkout), Windows shard 3 is the test_open_slots_persistence async-yield flake, and Coverage Gate fails closed downstream of the skipped combine.

[NicholasRBowers]

Now at 7d187917f3f9a70b9f787bda0ba4a5c031026e7b. The hash-domain finding is FIXED (it was recovered from the review job's log last round, where the verdict landed without its marker).

store.py — state and item hashes were compared across domains.

Two different quantities were both called a content hash. folder_file_state.content_hash is sha256 of the file's raw bytes, which is correct for its real job — change detection has to answer "did this file change?" before any extraction runs, and deriving it from extracted text would force an extraction pass on every scan. items.content_hash is sha256 of the extracted text. For .md/.txt the two strings are equal, so every plaintext test passed; for anything the reader transforms they differ and the lookup matched nothing. Confirmed empirically with an HTML file — the reader returns Runbook Rotate the fleet nightly. and the two hashes differ.

Folder rows now carry the text hash separately in text_hash, and ownership lookups resolve through one expression per table (_OWNERSHIP_HASH_COL): COALESCE(text_hash, content_hash) for folder rows, content_hash for the aggregates, which already stored a text hash and needed no second column. That fixed the three affected sites — _adopt_reassigned_item, detach_source_location_by_hash, and release_stale_claim, the last of which now takes both hashes because it needs the bytes pair to decide whether the file changed and the text hash to name the document.

The COALESCE is load-bearing, and a test proved it. Keying on text_hash alone regressed legacy rows: they are never backfilled, and for plaintext their ownership lookups match today via content_hash. The existing test_a_gate_refused_row_adopts_the_item_the_cascade_hands_it failed and caught exactly that, so the fallback is what keeps already-stored rows behaving as they do now. A rescan populates text_hash and the row becomes correct for transformed documents too.

Population needs no new plumbing. _update_state derives the value from the row's own items (one document's items share its hash). The one row that owns nothing — refused by the pre-ingest gate, and precisely the row whose claim must later be released — takes it from the byte-identical sibling it was refused against: equal bytes through the same reader give equal text, so it is derived, not guessed.

Deliberately no backfill. The column is nullable and legacy rows are left NULL. A content_hash backfill was written and removed from this PR once for being a data-loss bug (it grouped by source_id, so a folder of N legacy files got ONE hash); the same trap applies here, and COALESCE makes the backfill unnecessary.

Migration verified on a copy of the real 667-item database, not just on fixtures: items, sources, folder state and source_locations counts identical before and after, run twice for idempotency, all 76 existing rows left NULL as intended.

Tests, proven non-vacuous: test_ownership_lookups_work_when_text_differs_from_bytes builds a row whose bytes and text hashes differ and asserts the recipient records what it inherits, and that a detach keyed on the bytes hash matches nothing while the text hash matches. Pointing the folder entry back at content_hash fails it on that assertion.

Known narrower limitation, not addressed here. The pre-ingest duplicate gate compares a folder file's bytes hash against the aggregate tables' text hash, so a transformed folder document is not recognised as a duplicate of an artifact or agent document. That costs a redundant indexed copy rather than losing data, and closing it means hashing extracted text before the gate — which reintroduces the extraction pass the gate exists to avoid. Flagged rather than folded into this change.

Suite: 46 failures, all pre-existing environmental families, zero in the knowledge area. Gates: isort, flake8, mypy 719 files, docs-lint, brand gate, scrub-lint. Shard 2's test_lazy_data_home_paths failure remains the separately-reported main breakage.

[NicholasRBowers]

/ai-review override fable 7d18791: Opus 5 aborted with an execution error (Claude result is_error:true) and emitted no verdict or findings; its log contains no finding lines, and it passed with no blocking findings on the prior head b16434c.

[github-actions]

Human judgment recorded

@NicholasRBowers marked the fable AI finding as false positive, not applicable, or explicitly accepted for 7d187917f3f9a70b9f787bda0ba4a5c031026e7b.

Opus 5 aborted with an execution error (Claude result is_error:true) and emitted no verdict or findings; its log contains no finding lines, and it passed with no blocking findings on the prior head b16434c.

This decision applies only to this commit. A new push requires a new judgment.