fix(knowledge): reconcile artifacts on every start, not on source-row creation

[bolichen97]

Follow-up to #2448, fixing the blocking finding GPT 5.6 raised on it. #2448 was merged while CI was still running, so the finding was never acted on; dispositions are recorded here.

Problem

Artifacts silently stop appearing in the Knowledge Library, permanently, with the toggle reading ON.

ArtifactKnowledgeSync.start() gated its catch-up pass on whether it had just inserted the aggregate source row:

source_id, created = ensure_artifact_source(self.kstore)
if created:
    self._backfill_task = asyncio.create_task(self._run_backfill(source_id))

That row outlives the feature being switched off. So on any install that ever had auto-ingest on — which, before #2448, was every install, since it shipped on by default:

1. knowledge.auto_ingest_artifacts goes off (an upgrade past fix(knowledge): make the three auto-ingest paths opt-in #2448, or the user flips it) → _start_artifact_ingest_async returns early → the change-listener is never registered
2. artifacts are created / edited / deleted during that window → nothing mirrors them into the Library
3. the user opts back in → ensure_artifact_source finds the row → created=False → the catch-up never runs
4. the drift from step 2 is never repaired

The only recovery was to delete the "Artifacts" source by hand so the row would be re-inserted on the next boot — which nobody would guess.

The created flag was documented as a deliberate design choice ("the row's existence is the idempotency marker … no separate flag needed"), justified by "nothing writes the store while the gateway is down, and there is no out-of-process writer, so no recurring reconcile is needed". Making the feature opt-in invalidated that premise: the gateway now runs, and writes artifacts, with this listener switched off.

Why it matters

It is silent and it is not self-healing. The switch says ON, the artifact exists, the Library does not have it, and nothing in the UI or the logs tells the user which of those to distrust. Search then returns confidently incomplete results — worse than an obvious failure, because the user has no reason to doubt it.

Fix (symptoms → root cause → change)

Root cause: the catch-up pass keyed off a proxy (row insertion) for the thing it actually needed to know (does the Library match the store?). The proxy was only ever valid while the feature could not be off.

Change: replace it with a real comparison, reconcile_artifacts, and run it on every start():

• ingest what the store has and artifact_item_state lacks or disagrees with
• remove state for artifacts that no longer exist
• created is now reported for logging only, and ensure_artifact_source's docstring says so, so the trap is not re-set later

This repairs drift from any cause — the feature having been off, a crash mid-ingest, a restore from backup — not just this one. That is why it was chosen over the alternative (a config write-back migration that preserves the old effective value): the migration fixes the one path that produced this bug, while reconcile fixes the class.

Three properties that make running it on every start safe:

1. Converged costs nothing. ingest_artifact already returns None for unchanged content, so the steady state spends zero extraction calls and logs at debug rather than writing a line every boot.
2. Removals are judged against every artifact, not the eligible kinds. Narrowing auto_ingest_artifact_kinds makes an artifact ineligible, not absent — reaping on that basis would delete content the user never deleted. Removals run first and are unbudgeted, since they are pure state deletes with no token cost.
3. Ingests are bounded per run by RECONCILE_INGEST_BUDGET (a module constant — no new config key, no i18n or settings surface). ArtifactStore.list is newest-first, so a backlog from a long off-window drains across successive starts with the most recent artifacts landing first, rather than arriving as one unbounded burst of billed extraction calls. This also closes the second item I disclosed on fix(knowledge): make the three auto-ingest paths opt-in #2448 ("the first-enable artifact backfill is unbounded"). Unchanged artifacts never consume budget, so a converged store never defers.

Both the filesystem walk and the state read are asyncio.to_thread-offloaded, as the previous pass already was.

Tests

test/test_knowledge_artifact_ingest.py — TestBackfill becomes TestReconcile, plus the regression:

• test_start_still_reconciles_when_the_source_row_already_exists — the regression this PR exists for. Pre-creates the row (as an upgrade has), adds content while "off", and asserts start() still reconciles. Proven red against the pre-fix gate:

  E  AssertionError: start() skipped the reconcile because the source row already existed
  E  assert None is not None
  

• test_a_converged_store_costs_nothing — second pass over identical content returns (0, 0, 0). This is the property that makes every-start safe; without it the fix would trade a correctness bug for a cost bug.
• test_reconcile_drops_state_for_artifacts_deleted_while_off — a delete during the off-window leaves searchable text with no artifact behind it; reconcile must drop it.
• test_an_ineligible_kind_is_not_treated_as_deleted — narrowing kinds must not reap.
• test_budget_defers_the_remainder_and_a_later_run_finishes_it — the budget defers rather than drops, and the next start finishes the backlog.
• test_reconcile_ingests_eligible_only / test_reconcile_empty_kinds_noop — carried over from TestBackfill.

Manual verification

Gate	Result
pytest (full backend)	39915 passed, 17 failed
isort --check-only / flake8	clean
mypy src/kiro_crew/	3 errors, all in untouched hooks.py
scripts/docs-lint.sh	190 files, pass

The 17 failures are the same pre-existing macOS-host set documented on #2448 (Seatbelt sandbox spawn, nested-pytest harness, a macOS temp-path redaction artifact, a __CF_USER_TEXT_ENCODING env leak) — verified against a clean origin/main checkout there, and byte-identical node IDs here. Passing count rose 39814 → 39915 with the new tests; no new failure appeared.

No frontend gates run: this change has no website/ diff.

Screenshots

N/A — no user-visible UI change. The fix is entirely in the gateway's startup reconcile path; the Settings toggle and its copy are untouched by this PR (they were shipped in #2448).

Review round 2 (commit 09cbb7764)

Three blocking findings from the first round, all legitimate, all fixed — see the disposition comment for the full reasoning:

1. remove_artifact ran on the event loop (GPT + Opus). remove_artifact -> delete_items_batch -> _load_graph is a graph rebuild in a SQLite transaction; the sibling live-delete in _handle already offloads the identical call. Now await asyncio.to_thread(...). This is the same defect class as fix(knowledge): run per-file dedup off the event loop #2175 / fix(knowledge): run the ingest-path item deletes off the event loop #2336 — and the existing test_knowledge_delete_off_loop.py ratchet missed it, because it scans for the literal delete_items_batch inside async def bodies and my call reached it via remove_artifact.
2. if not kinds: return sat above the removal loop, so an empty allowlist left deleted content searchable. Removals now run first, unconditionally.
3. An unreadable meta.json was treated as a deletion. ArtifactStore.list omits such an artifact, so a transient read error would have deleted a live artifact's indexed content. _artifact_is_really_gone now confirms per slug and removes only on ArtifactNotFoundError.

Three new tests, one per finding; the off-loop one asserts the thread and is proven red against the pre-fix code.

Still open from #2448

Not fixed here, and deliberately: the artifacts toggle still needs a gateway restart to take effect. _start_artifact_ingest_async reads auto_ingest_artifacts once at startup, while its two neighbours in the same Settings card re-read config every watcher sweep and apply immediately. This PR makes the restart repair the drift correctly, but the user still has to know to restart. Fixing that means either a live config re-read on this path or an explicit hint on the toggle — a separate change, and the UX reviewer flagged it on #2448 too.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound root-cause fix, but the repair loop is restart-gated: a deferred backlog on a long-running gateway silently stays incomplete for weeks.

Watch

The budget "defers the remainder … drains across successive starts" — but reconcile is only ever armed from start(). A personal gateway can run for weeks between restarts, so an opt-in against a >RECONCILE_INGEST_BUDGET backlog leaves search incomplete (toggle ON, log line only) for the whole uptime — the exact harm class this PR exists to kill, now bounded but still restart-coupled, compounding the acknowledged "toggle needs a restart" gap.

Suggestions

• Re-arm _run_reconcile in-process when deferred > 0 (a delayed retask, not a new config surface), so a backlog drains without the user knowing to restart.

[DESIGN-REVIEWED] 7c18b88

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/knowledge/artifact_ingest.py:752 -- extractor failures hit "except Exception" without consuming the budget, allowing every backlog artifact to be attempted in one start -> Fix: increment the budget counter in this exception path.
[GPT-REVIEWED] 7c18b88

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates are self-rated low confidence and framed as scalability/accuracy concerns. I verified the mechanisms against the actual code.

Candidate 1 (failed extraction consumes budget → starves older artifacts): The mechanism is real — a non-raising failed/partial ingest returns a truthy job_id with a non-completed, non-duplicate status (_job_status returns 'failed'/'partial', ingestion.py:213-218), so the reconcile loop does ingested += 1 (artifact_ingest.py:591) and never records state, retrying every start. But the harmful outcome — older ingestible artifacts permanently starved — requires ≥budget (50) newest artifacts persistently failing extraction without raising (a raising failure hits the loop's except and does not consume budget). A transient/backend-wide failure hits all artifacts (no differential starvation and self-heals next start); permanent deterministic per-content non-raising failure of the newest 50 specifically is a condition I cannot confirm occurs in practice, and the candidate could not either. Outcome (c) resolves to "could," not "does." Not a crash/data-loss/corruption/removed-guard class either. Drops below the bar.

Candidate 2 (O(N) get() reads + name-refresh writes every start): Confirmed the loops are unconditional per tracked artifact (artifact_ingest.py:487-538), but the work runs off-loop via to_thread and produces no observable wrong outcome — it is a scalability note, and the "costs nothing" claim it cites is explicitly scoped to extraction calls, which the candidate concedes. No (c). This is exactly the performance/accuracy-of-claim category the pass does not report.

Neither survives falsification at ≥80.

No findings.

[OPUS-REVIEWED] 7c18b88

Verdict parsed from the review's SHA-scoped output markers for commit 7c18b88d8669eab4c0cf602536db195502a8994b.

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

[bolichen97]

All three findings on c97545f6c190e8761af911554d133280fde4c9bc were legitimate and are fixed in 09cbb7764. None were rebutted.

1. GPT + Opus (both) — BLOCKING, artifact_ingest.py:443, synchronous removal blocks the event loop — FIXED.

remove_artifact → delete_items_batch → store._load_graph is a full in-memory graph rebuild inside a SQLite transaction, once per slug deleted during an off-window, and I called it straight from the coroutine body. Opus's note that the sibling live-delete in _handle already offloads this identical call for this identical reason is the decisive evidence — I introduced an inconsistency with the very path I was mirroring.

Now await asyncio.to_thread(remove_artifact, ...), with a comment naming the mechanism so the next edit does not undo it.

Worth flagging for maintainers: this is the same defect class as #2175 and #2336, and the existing ratchet did not catch it. test/test_knowledge_delete_off_loop.py scans for the literal name delete_items_batch inside async def bodies; my call reached it through remove_artifact, one level of indirection, so the scan sailed past a textbook instance of what it guards. Widening that ratchet to the wrapper functions that reach _load_graph (remove_artifact, delete_item, delete_source_cascade, import_bundle) would close the gap. I have deliberately left that out of this PR to keep it reviewable, but it is the durable fix and I am happy to open it.

New test: test_removals_run_off_the_event_loop asserts the thread remove_artifact lands on, not the call shape, so a refactor that keeps asyncio.to_thread in the source but hands it an already-invoked result still fails. Proven red against the pre-fix code:

E  AssertionError: remove_artifact ran on the event-loop thread; it must be handed to asyncio.to_thread

2. GPT — BLOCKING, artifact_ingest.py:428, empty kinds skip deletion reconciliation — FIXED.

if not kinds: return 0, 0, 0 sat above the removal loop, so an empty allowlist left a deleted artifact's text answering searches forever. An empty allowlist means "ingest nothing", not "let deleted content stay searchable". Removals now run first and unconditionally; only the ingest loop is gated on kinds.

New test: test_reconcile_empty_kinds_still_drops_deleted_state.

3. GPT — BLOCKING, artifact_ingest.py:441, unreadable artifacts treated as deleted — FIXED, and this was the most serious of the three.

ArtifactStore.list silently omits an artifact whose meta.json cannot be read, so absence from the listing is not proof of deletion — a transient file-read error would have deleted a live artifact's indexed content. Added _artifact_is_really_gone, which confirms each candidate with an offloaded art_store.get and removes only on ArtifactNotFoundError; any other exception logs a warning and keeps the state, because a stale item group is recoverable on the next start and deleted items are not.

New test: test_an_unreadable_artifact_is_not_treated_as_deleted.

Verification on 09cbb7764: backend 39918 passed (17 pre-existing macOS-host failures, unchanged set, documented on #2448), isort / flake8 clean, mypy clean outside the untouched hooks.py. No website/ diff.

The Windows shard-2 failure on the previous SHA was test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state ("DID NOT RAISE OSError") — unrelated to this diff, which touches only artifact ingest, its handler docstring, its test, and a spec doc. This push re-tests it.

[bolichen97]

Both findings on 09cbb7764 were legitimate and are fixed in 85711b099. Neither was rebutted. Opus passed this round.

1. GPT — BLOCKING, artifact_ingest.py:489, deduplicated artifacts permanently exhaust the ingest budget — FIXED.

Confirmed the mechanism in code before fixing. On a DUPLICATE_JOB_STATUS refusal ingest_artifact returns a truthy job_id, so my if await ingest_artifact(...): ingested += 1 counted it. Worse, that path writes _set_state(..., [], status="deduped") with an empty group, and the per-slug short-circuit requires prev_hash == content_hash AND old_item_ids — so with an empty group it falls through and re-attempts on every start. A budget's worth of duplicates therefore starves every artifact behind them permanently, exactly as reported.

Reconcile now reads the job status and continues on DUPLICATE_JOB_STATUS without spending budget. Refusals cost no extraction call (the gate is a pre-ingest hash lookup), so this is free.

New test: test_a_duplicate_refusal_does_not_spend_the_budget — forces every job to report duplicate, runs with budget=1, asserts (ingested, deferred) == (0, 0). Proven red against the pre-fix code.

2. GPT — BLOCKING, artifact_ingest.py:489, empty artifacts retain stale indexed content — FIXED.

ingest_artifact early-returns on not text.strip(), so an artifact emptied after being ingested kept its previous chunks answering searches. Fixed at the source rather than in reconcile, because the live _handle("upsert") path has the identical bug — an artifact emptied while sync is ON also left stale chunks. The empty-content branch now drops the tracked group when one exists, via await asyncio.to_thread(remove_artifact, ...) (offloaded for the same graph-rebuild reason as round 1's finding).

New test: test_an_emptied_artifact_drops_its_indexed_group. Proven red against the pre-fix code.

---

On the Windows shard-2 failure — not from this diff, and main is red on it.

test/test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state — "DID NOT RAISE <class 'OSError'>".

The test makes a file unreadable with chmod(0o000) and asserts the read raises OSError. Windows ignores the read bit for the file's owner, so the read succeeds and pytest.raises(OSError) never fires. It is a platform assumption in the test, deterministic rather than flaky, and it has no reachable connection to this diff (which touches artifact ingest, its handler docstring, its test, and a spec doc).

It was added today in #1676 (feat(chat): add message-level pinning, commit 3c7c23ed2) and is not listed in test/windows-expected-failures.txt, so main carries this failure. It needs its own fix — either a skipif for Windows or a permission-denial mechanism that works there. Happy to open that separately; it should not gate this PR.

[bolichen97]

Accepted GPT 5.6's blocking finding on artifact_ingest.py:467 (off-window metadata drift) and fixed it in 7476652a.

Two unbudgeted repair passes now run inside reconcile_artifacts, after removals and before the budgeted ingest loop:

• Kind drift. A tracked artifact whose own kind has no _KIND_EXT entry (markdown -> svg while sync was off) is reaped. That condition is proof of a post-ingest kind change, because an unsupported kind can never have been ingested under any config, so this cannot over-delete. It deliberately does NOT touch supported-but-config-excluded kinds: narrowing kinds makes an artifact ineligible, not stale, which is the existing over-deletion guard. Runs even with an empty allowlist, same rule as deletions.
• Name drift. Every tracked artifact's stored group label is refreshed to its current redacted name via the existing refresh_artifact_name, mirroring the live rename path. Metadata-only: content hashes untouched, so a converged store still spends zero budget and zero extraction calls.

3 new tests, each verified failing on the unmodified source: kind-change-to-unsupported reaps, kind drift reaps under an empty allowlist, and rename-while-off refreshes the label without re-ingesting (asserts item ids unchanged and (0, 0, 0) counts). Full file: 44 passed. flake8, isort, mypy clean. Spec docs/system-specs/modules/artifacts.md updated with both contracts.

Also rebased onto current main (was 50+ commits behind).

[bolichen97]

Accepted GPT 5.6's round-2 finding (artifact_ingest.py:496 -- supported-but-excluded kind change left stale content) and fixed it in 4ee8e094, but not by treating "current kind outside kinds" as stale. That predicate cannot distinguish two different situations:

• the artifact changed kind after ingest -> its group came from the old kind's reader, so it IS stale
• the user narrowed auto_ingest_artifact_kinds -> the artifact is untouched, so reaping deletes content they never changed (the existing over-deletion guard, test_an_ineligible_kind_is_not_treated_as_deleted)

So reconcile now decides from the kind recorded at ingest instead. artifact_item_state gains a kind column (create + ALTER TABLE migration in store.py), written by _set_state on both the completed and deduped paths; _known_slugs became _known_kinds returning slug -> ingested kind. Drift is recorded_kind is not None and recorded_kind != art.kind, which covers GPT's markdown -> excluded-text case and the earlier markdown -> svg case uniformly, while a narrowed allowlist alone still reaps nothing. A legacy row predating the column carries NULL = "cannot tell", left alone (a stale group is recoverable, deleted items are not); its next ingest backfills the column.

Tests: 6 reconcile tests now cover kind change to unsupported, kind change into an excluded kind, narrowing-alone-never-reaps across repeated starts, NULL legacy row left alone, drift reaped under an empty allowlist, and rename-while-off refreshing the label without re-ingesting. Suite: 47 passed in the file, 1793 passed across all knowledge/artifact tests. flake8, isort, mypy clean. Spec doc updated with the recorded-kind rationale.

On Fable's advisory CONCERNS (in-process retry for a >50 deferred backlog): leaving as-is for this PR, since it changes when billed extraction runs rather than fixing a correctness gap, and the PR is already a follow-up to a merged change. Happy to take it as a separate issue.

[bolichen97]

Both round-7 findings addressed in 1d87792d.

Nonempty fallback snapshots overwrite newer content (:304) -- accepted as written. The guard moved above the empty check and now returns on source_missing unconditionally: a fallback snapshot is not evidence about the live file in either direction, so acting on it either destroys a valid index or replaces newer text with older. Test asserts an older non-blank snapshot does not displace the newer indexed body.

Startup retry duplicates committed items (:693) -- the diagnosis is correct and I reproduced it: with the state row deleted to simulate the crash window, the next reconcile leaves two searchable copies of the same body. But the proposed fix ("revert the unconditional startup retry") reverts the defect this PR exists to fix, and it is not needed, because the residue is reconstructible: ownership is per-source, so within the aggregate Artifacts source an item listed by no artifact_item_state row is unreachable from every artifact and can only be crash residue.

_sweep_orphan_items() now runs first in reconcile_artifacts, unbudgeted, deleting exactly those items. Scoping notes: deduped rows own no items in this source (their content lives under the winning source) and every other ingest path writes its own source_id, so neither is caught; callers hold the sync lock, so no in-flight ingest can be mistaken for residue; and an unparseable item_ids group aborts the sweep rather than risking a false orphan. This also repairs residue from crashes that predate this PR, which the previous behaviour left searchable forever.

The probe test is kept as a regression test: it deletes the state row post-ingest and asserts exactly one copy of the body survives the next reconcile (it fails without the sweep, showing 2).

Suite: 56 passed in the file, 1802 across all knowledge/artifact tests. flake8, isort, mypy clean. Spec doc updated with both rules.

[bolichen97]

Round-8 finding is correct, and I have removed the orphan sweep in 3ec7ddd4. Verified the mechanism: export_all carries sources, items, entities, relations, source_locations and mentions, but none of the per-document state tables, so imported items genuinely arrive with no ownership row and the sweep would have deleted them.

That leaves the round-7 duplication unfixed, which I want to be explicit about rather than paper over. The two findings are in direct tension: reconstructing ownership requires treating unowned items as residue, and that is only safe once imported items arrive owned. The root cause is that import_bundle does not restore artifact_item_state (nor folder_file_state / agent_item_state), which is outside this PR's diff.

Scope call: the duplication window is reachable from the live listener path too (crash, then any upsert event for that artifact), so it predates this PR and removing the sweep restores the status quo rather than regressing it. Filed #2670 with both halves, the reproduction, and the suggested direction (carry the state tables in export/import, then fence the crash window, after which the sweep becomes safe and also repairs pre-existing residue).

Everything else from rounds 1-7 stands. Suite: 55 passed in the file, 1801 across all knowledge/artifact tests. flake8, isort, mypy clean.

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 7c18b88d8669eab4c0cf602536db195502a8994b touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[bolichen97]

Accepted the round-9 finding, fixed in 7c18b88d. The emptied-body drop costs no extraction calls, so putting it behind the extraction budget was the wrong side of the line: an emptied artifact buried under a backlog of newer changes stayed searchable for as many restarts as that backlog took to drain.

There is now an unbudgeted pre-pass before the ingest loop that removes tracked artifacts whose live body is blank. Scoping:

• only tracked artifacts are read, since an untracked one has no group to drop
• it stands down on source_missing for exactly the reason ingest_artifact does: a blank snapshot behind an unreadable live source proves nothing about the real content
• dropped slugs are skipped by the later name-refresh and ingest loops, so nothing re-reads them
• the extra get is a disk read (the same one ingest_artifact would have done), never an extraction

Two tests: with budget=1 and a newer artifact consuming the budget, the emptied one is still dropped and its obsolete text is gone while the newer body lands; and an emptied body behind a dead source pointer is left alone with its index intact. The first is verified failing on 3ec7ddd4.

Suite: 57 passed in the file, 1803 across all knowledge/artifact tests. flake8, isort, mypy clean. Spec doc updated.

For context on where this stands: all 48 CI checks were green on the previous head, with this review the only blocker.