fix(knowledge): take created ids from writes, not source diffs (#4431)

[bolichen97]

Problem / Motivation

Three knowledge-ingestion sites still derive "what this call wrote" by snapshotting the source's full item-id set before the work and diffing a second snapshot afterwards. Each snapshot is a SELECT id FROM items WHERE source_id = ? materializing one row per item in the SOURCE (~20k rows, over a second of blocking SQLite on a large library), and the "before" read runs ON the asyncio event loop, once per ingested file — the same per-file loop stall open PR #3397 removes from the folder-scan caller:

1. ingestion.py::_ingest_file_body — _before_ids captured on the loop, consumed only by the partial-failure branch of _finalize.
2. ingestion.py::ingest_text — same shape, and this function did not yet collect the ids it writes.
3. artifact_ingest.py::ingest_artifact — full before/after diff feeding _set_state, while the pipeline it calls already reports created ids through on_committed (the contract agent_source.py consumes today).

Why it matters

Beyond the loop stall, the diff is wrong, not just slow: import_bundle writes into the same aggregate source in its own transaction under no shared lock, so anything it commits while an ingest is awaiting gets attributed to that ingest — handing a document delete authority over knowledge it never created. The comment at the site 1 collection already records this; sites 2 and 3 still had the defect.

What changed (motivation → approach → change)

Ids are now collected at the write, never inferred from a source snapshot — the same conversion #3397 applies to the folder-scan caller, extended to the remaining three sites:

• Site 1 (_ingest_file_body): deleted the _before_ids capture; the partial-failure branch passes list(created_item_ids) (already collected immediately after add_item, before add_source_location/_store_entities/_embed_item, so a chunk that raises partway is still captured) instead of after_ids - _before_ids, dropping the after_ids re-read too.
• Site 2 (ingest_text): added created_item_ids collection mirroring site 1's ordering exactly; deleted the _before_ids capture and the after_ids re-read.
• Site 3 (ingest_artifact): deleted both snapshot reads; the ids come from pipeline.ingest_file(..., on_committed=_record_ownership), which fires inside the finalize hop only on the processed == total branch — the same branch that reports job status completed, which is the only path that consumes new_ids. The callback also persists _set_state there (GPT round-1 finding): the awaits between the pipeline's commit and the caller's own state write (temp-file cleanup, job-status read) are cancellation points, and a shutdown landing there would orphan the committed group; the write is fail-safe (a swallowed error defers to the caller's idempotent post-ingest write). The duplicate and partial/failed early returns are untouched.

Strictly out of scope (deliberately not widened): the _old_item_ids reads in ingest_file/ingest_text share the query text but mean "the pre-existing item group this call must delete" — no callback can supply them, and moving them off the loop changes the transactional shape of the delete. Filed as follow-up #4441. Also untouched: folder_watcher.py and test/test_knowledge_ingest_scan_off_loop.py (owned by open PR #3397 — this diff has zero file overlap with it), the processed == total success path and the duplicate gate. The partial-failure rollback calls deliberately do NOT pass owner_source_id (adjudicated across GPT rounds 2-3, which prescribed opposite fixes for this span): the rolled-back items are chunks of an INCOMPLETE write, and detaching them to a concurrent duplicate-gate attacher would leave that attacher holding a truncated document whose recorded whole-document hash reads as satisfied, so no rescan ever repairs it. Destroying them outright leaves the attacher's claim naming an empty group, which the doc-state recovery paths already treat as re-attempt. This matches the pre-existing rollback semantics (the old after_ids - before_ids delete was also unscoped). The success path's own owner_source_id usage is untouched.

Tests

New test/test_knowledge_ingest_created_ids.py (all six fail on main before this change, pass after):

• Sites 1 & 2 — rollback precision: partial-failure ingest (second chunk's embed raises) with a pre-existing sibling item group AND a foreign item committed mid-ingest (injected inside extract_batch, i.e. after the point the old "before" snapshot was taken — the import_bundle misattribution case). Asserts the rollback deletes exactly what this call created: both foreign items survive, sync_status goes error. On main the mid-ingest foreign item is destroyed.
• Ratchet — the snapshot query cannot creep back: AST scan pinning occurrences of the SELECT id FROM items WHERE source_id literal per function (nested _finalize closures included, so re-adding the read on- or off-loop trips it): _ingest_file_body = 0, ingest_artifact = 0, ingest_text = exactly 2 (the out-of-scope _old_item_ids reads tracked by knowledge: _old_item_ids full-source reads still run on the event loop (ingest_file/ingest_text) #4441 — the message tells the future fixer to repoint the ratchet). Counting occurrences, not timing, per the shape fix(knowledge): take ingested item ids from the commit callback #3397 established.
• Site 3 — state records what the pipeline reported: a spy wraps on_committed and asserts _set_state stored exactly the reported ids, non-empty on the completed path (an empty list would silently lose the slug's claim/ownership record), and asserts ingest_artifact still passes on_committed at all.

Local gates: isort / flake8 / mypy clean; full pytest 56640 passed, 296 skipped, 6 xfailed.

Manual verification

N/A — unit coverage sufficient: the converted paths are exercised end-to-end through the real IngestionPipeline and SQLite store in the new tests, including the concurrent-writer interleaving.

Related Issues

Closes #4431
Follow-up for the out-of-scope on-loop reads: #4441

Checklist

• Single commit with a Conventional Commits title
• Tests added/updated for the change
• Docs untouched — docs/system-specs/modules/knowledge.md already documents the collected-at-the-write contract this PR extends; no documented behavior changed

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 2342533abf698a648f41c009a37e8e36a3d8e3c7; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Advisory premise-level review of 2342533abf698a648f41c009a37e8e36a3d8e3c7 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

Both data files read; I've verified the claims against the repository (the on_committed contract's three call sites, the remaining snapshot-query occurrences, the black gate's prune mechanics). Final review follows.

First-Principles-Verdict: CONCERNS

Sound fix at cause level — but its own fail-safe rationale indicts an untouched sibling callback, and one baseline prune rides along undeclared.

What this change ships

Intent: stop an ingest from deleting or claiming knowledge a concurrent import committed, by taking created ids from the writes themselves — a FIX.

1. File-ingest partial failure now deletes only its own items; concurrent writers' items survive — justified.
2. Text-ingest partial failure likewise, with the id collection newly added — justified.
3. Artifact ownership record lists only ids the pipeline actually committed — justified; adopts the existing on_committed contract instead of a second spelling.
4. Artifact ownership now persists during the commit itself, surviving a shutdown just after — declared rider, named harm.
5. Two full-source id reads per ingest removed from the event loop — justified.
6. New fallback branch: post-ingest ownership write runs only when the in-commit write failed — declared rider, named harm.
7. Ratchet test pins the snapshot-query count per function — justified guard.
8. mcp_gateway/preflight.py pruned from the black baseline — undeclared rider; gate-mandated (the graduated-entry check in check_black_formatting.py fails unscoped).

Watch

• Unfixed sibling of the rider's own cause: the diff's comment says "A raise escaping here would poison the finalize hop AFTER the group committed" and swallows accordingly. Grep on_committed=: 3 callback sites that persist state inside the hop — folder_watcher.py:938 and artifact_ingest.py:414 swallow; agent_source.py:350 _record_ownership calls set_state bare, so the same writer-lock raise makes a committed document report failed. Unfixed and unfiled (the description's follow-up knowledge: _old_item_ids full-source reads still run on the event loop (ingest_file/ingest_text) #4441 covers only the _old_item_ids reads).

[FIRST-PRINCIPLES-REVIEWED] 2342533

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix on the established seam: ids collected at the write (the existing on_committed contract from #3397/agent_source), with a ratchet test preventing snapshot re-introduction.

Watch

• The .github/black-baseline.txt prune of mcp_gateway/preflight.py is unrelated to this PR's files and unmentioned in the description — likely --update-baseline fallout; confirm it's intentional or move it to its own commit per the baseline convention.

[DESIGN-REVIEWED] 2342533

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2342533abf698a648f41c009a37e8e36a3d8e3c7 — this comment is updated in place on each push.

Review details

I've independently verified the three conversion sites and the cross-file on_committed contract:

• _ingest_file_body (ingestion.py:668): created_item_ids.append(item_id) runs immediately after add_item, before the location/entity/embed steps that can raise — so a partway chunk failure still captures the created id for the partial-failure delete at line 726. The completed branch fires on_committed(list(created_item_ids)) inside the uncancellable _finalize hop.
• ingest_text (ingestion.py:842): same pattern, collection newly added; partial-failure delete at line 888 uses created_item_ids.
• artifact_ingest: on_committed=_record_ownership fires only on the processed == total branch — the same branch that yields status "completed". committed_ids is extended before the guarded _set_state, so the if not ownership_persisted fallback (reached only when the hop write raised-and-was-swallowed) still has the ids. Duplicate/partial/failed statuses return before the fallback. Cross-thread visibility of ownership_persisted/committed_ids is established by the await join on the shielded worker in run_to_completion.

No path where status == "completed" skips both the hop write and the fallback; no dangling references to the removed _before_ids/after_ids. The candidate list is empty and nothing survives to Step 2.

No findings.

[OPUS-REVIEWED] 2342533

Verdict parsed from the review's SHA-scoped output markers for commit 2342533abf698a648f41c009a37e8e36a3d8e3c7.

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

[bolichen97]

Adjudication for the partial-failure rollback span (ingestion.py _finalize, elif processed < total branch), which received two OPPOSITE blocking prescriptions:

• Round 2 (head 2522af3): "Pass owner_source_id=source_id at both rollback calls" -- adopted in e65736c.
• Round 3 (head e65736c): "Remove owner_source_id=source_id from both partial-failure rollback calls" -- adopted in bc76f6f.

Resolution: round 3 is correct and is now final. The rolled-back items are chunks of an INCOMPLETE write. A concurrent identical ingest whose duplicate gate attached to them recorded the whole-document content_hash as satisfied in its own state row; if rollback DETACHES the truncated items to that attacher (owner-scoped path), the attacher keeps a truncated document that no rescan ever repairs, because its hash reads unchanged -- permanent silent corruption. If rollback DESTROYS them (unscoped path), the attacher's claim names an empty group, which the doc-state recovery paths already treat as "re-attempt", so the next scan restores a complete copy -- transient and self-healing. Destroy-outright also matches the pre-existing rollback semantics (the old after_ids - before_ids delete was unscoped).

Disposition: the round-2 prescription is REJECTED as harmful; the round-3 prescription is ADOPTED. Both rollback calls stay unscoped, with the rationale recorded in code comments at both sites. Please treat this span as adjudicated: further findings prescribing owner-scoping here should reference and rebut the truncated-document argument above rather than re-raising the round-2 form.

[bolichen97]

Adjudication for the round-5 findings on head 24addd2 (operator decision, option b from the escalation comment):

Both round-5 findings (artifact ownership persist vs concurrent dedup collapse; partial-rollback delete vs cascade reassignment) are adjudicated as a pre-existing residual race class, deferred to follow-up #4457 (filed, cross-linked with #4441 since both change the transactional shape of the same hops):

• Both interleavings exist IDENTICALLY in pre-PR main, with strictly WIDER windows: the old after_ids - before_ids rollback deleted a stale id set with no ownership validation AND additionally swept concurrent writers' items (the defect this PR fixes); the old artifact state write happened later still, after the post-ingest awaits, from a snapshot diff.
• This PR strictly narrows both windows. The prescribed fixes (read-validate-delete/persist under BEGIN IMMEDIATE) introduce transactional machinery that belongs with knowledge: _old_item_ids full-source reads still run on the event loop (ingest_file/ingest_text) #4441's transactional-shape work, per the same scoping boundary the PR body already declares.

Disposition: REBUT-AS-OUT-OF-SCOPE with tracked follow-up #4457. Findings in this class on subsequent heads of this PR should be reported as advisory referencing #4457, not BLOCKING, unless the finding shows the window is NEWLY INTRODUCED or WIDENED by this diff.

[bolichen97]

/ai-review override gpt 2342533: residual pre-existing race class predating this PR, adjudicated and tracked in follow-up #4457

Context: the remaining finding (fallback _set_state vs concurrent dedup collapse) is the same class adjudicated in the round-5 disposition comment above: the window exists identically and WIDER in pre-PR main (state written after all post-ingest awaits, from a snapshot diff), this PR strictly narrows it (primary write now inside the finalize hop; the fallback only runs when that write failed), and the prescribed read-validate-write transaction belongs with #4457/#4441's transactional-shape work. Operator-authorized override (option b from the escalation).

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 2342533abf698a648f41c009a37e8e36a3d8e3c7.

residual pre-existing race class predating this PR, adjudicated and tracked in follow-up #4457

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

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix -- sources knowledge-ingest created ids from write responses instead of source diffs, fixing id tracking.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #2937 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #2937: CONTINUE_DEVELOPMENT. One of the PR's two halves has been partly solved on main by a different mechanism after this branch's merge base (e2f569a, Aug 14; PR #4443 merged Aug 19). The intent/sweep machinery is still the only repair for pre-existing residue and for a hard kill, but it must be rebuilt on top of the on_committed seam rather than on the window it assumes. Files: src/kiro_crew/knowledge/artifact_ingest.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.