fix: move _old_item_ids reads into the off-loop dupe-gate hop (#4441)

[CrysisDeu]

Summary

Fixes #4441: four synchronous full-source reads (SELECT id FROM items WHERE source_id = ?) resolving _old_item_ids ran directly on the asyncio event loop inside async ingest_file (replace-all and existing-local-file paths) and ingest_text (changed-existing and replace-all paths). On a large library each materializes ~20k rows — over a second of blocking SQLite per call. This is the same cost profile PR #4443 retired for the created-ids reads, but these reads compute the pre-existing group to DELETE, so the created-ids callback substitution cannot cover them; #4443 explicitly scoped them out.

What changed

• src/kiro_crew/knowledge/ingestion.py — new _resolve_old_item_ids helper carries the query; each ingest path's duplicate-gate hop (run_to_completion(_gate)) now resolves the group on the worker thread (thread-local sqlite connection) just ahead of the gate's own BEGIN IMMEDIATE, returning (dupe_job, ids) so the resolved group still reaches both the gate's in-txn delete and _finalize's delete_items_batch. The read keeps its logical position relative to the gate/finalize hops — closing the read-to-delete slip window is a separate shape change tracked by knowledge: transactional validation for rollback/ownership writes vs concurrent dedup and cascade (from #4443 round-5 review) #4457 and deliberately out of scope here.
• test/test_knowledge_ingest_created_ids.py — the occurrence ratchet is repointed: ingest_file and ingest_text now pin zero query literals, _resolve_old_item_ids pins exactly one. New TestOldGroupReadRunsOffLoop adds one test per original read site: each spies on the resolver, asserts it ran off the loop thread, and pins that the resolved group still drives the replace (stale items deleted).
• test/test_knowledge_delete_off_loop.py — new AST ratchet test_resolve_old_item_ids_is_never_called_on_the_event_loop: any future call site added straight to a coroutine body fails the suite instead of reintroducing the stall.

Verification

• Targeted: 10 knowledge test files, 385 passed + the new/repointed tests (29 in the two touched files).
• Mutation-verified twice: moving the resolution back on-loop fails all 4 new behavioral tests AND the new AST ratchet; the real fix passes both.
• Local gates: black-check, subprocess-encoding, isort, flake8 (full), mypy (full), brand-name gate, harness-parity gate — all green.
• Pre-push review: GPT lane (gpt-5.6-sol) no findings; Opus lane (claude-opus-5) 0 blocking, 5 advisory — adopted the on-loop-call-site ratchet and a misleading test name fix; declined the store-method query consolidation and str-narrowing as out-of-scope surface (both call sites are truthy-guarded; noted for follow-up).

No UI changes (backend-only), so no screenshots.

Closes #4441

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 07241cf76f4b2680d6a05671e6fb5e8b3147ee30 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change moves the expensive full-source reads into the existing off-loop gate hop, threads the resolved ids back via a tuple so both the in-transaction delete and _finalize still receive them, and explicitly defers the read-to-delete consistency window to a tracked follow-up (#4457). Nothing between the old read sites and the gate hop consumes _old_item_ids, so the deferred resolution is semantics-preserving; the remaining on-loop reads are cheap point lookups. Tests pin both the mechanism (off-loop thread) and the semantics (stale group still deleted), plus an AST ratchet against regression.

Design-Verdict: PASS

Blocking reads folded into the already-off-loop gate hop is the root-cause fix, correctly scoped, with the slip window explicitly deferred to #4457.

[DESIGN-REVIEWED] 07241cf

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 07241cf76f4b2680d6a05671e6fb5e8b3147ee30 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 07241cf

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 07241cf76f4b2680d6a05671e6fb5e8b3147ee30 — this comment is updated in place on each push.

Review details

The refactor is behavior-preserving: _gate returns (dupe_job, ids), run_to_completion forwards the tuple intact, the duplicate path still returns early, and _old_item_ids is rebound to the resolved ids only on the fall-through path where it's consumed. No UnboundLocalError risk (the closure reads the outer _old_item_ids without assigning it), and resolve_old_group is only set where source_id is non-None. I could not ground any concrete input → path → wrong-outcome defect in the changed lines.

No findings.

[OPUS-REVIEWED] 07241cf

Verdict parsed from the review's SHA-scoped output markers for commit 07241cf76f4b2680d6a05671e6fb5e8b3147ee30.

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 07241cf76f4b2680d6a05671e6fb5e8b3147ee30 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change checks out against the contract: I read the intent file, the patch, and the surrounding ingestion.py, store.py, and both test files, and ran the greps the lenses require.

First-Principles-Verdict: PASS

Four on-loop full-source reads collapse into one off-loop helper — a declared mechanism-level fix, net-subtractive (4 query copies → 1), with recurrence pinned by an AST ratchet.

What this change ships

Intent: stop a re-ingest of a changed document from freezing the agent's event loop for seconds on a large library (#4441) — a FIX.

1. Ingest no longer stalls the loop on the four replace-all/changed-content paths — justified (the reported defect).
2. New internal helper _resolve_old_item_ids carries the query once — justified; 2 consumers (the two _gate closures), grepped SELECT id FROM items WHERE source_id in src/: 4 down to 1 occurrence in ingestion.py.
3. Gate hop now returns (dupe_job, ids) so the resolved group still reaches the in-txn delete and finalize — justified, mechanism of the fix.
4. Ratchet repointed: ingest_file/ingest_text pin zero query literals, the helper pins one — declared.
5. New AST ratchet fails any future on-loop call site — declared; guards the decision gap, not just this instance.
6. Four behavioral off-loop tests, one per original read site — declared.

Watch

• store.py:1075 keeps its own inline copy of the identical query inside delete_source_cascade (count: 2 spellings repo-wide post-change). Pre-existing, in-txn, and the description records declining consolidation — accepted-and-deferred, but it is the one place the two spellings can now diverge.
• The read-to-delete slip window is explicitly deferred to knowledge: transactional validation for rollback/ownership writes vs concurrent dedup and cascade (from #4443 round-5 review) #4457; the description says so, which is the smaller finding lens 6 asks for — nothing further owed here.

[FIRST-PRINCIPLES-REVIEWED] 07241cf

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (CodeQL+Semgrep success 0-ann, no alerts), security checklist all-NO, AI reviewers green. Category: fix with clear root cause — the full-source _old_item_ids read (one row per item, seconds of blocking SQLite on a large library) ran on the event loop in the replace-all ingest paths; moved into the existing off-loop run_to_completion duplicate-gate hop (#4441) via _resolve_old_item_ids, with a ratchet test pinning it off-loop. Same query/ids, no behaviour change — concurrency offload only, no security surface.

[chenmingwei23]

Tier 1 auto-approve: fix (3 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 -- moves the full-source _old_item_ids SELECT off the event loop into the off-loop duplicate-gate hop; clear root cause, behavior-preserving, test + coverage files only besides the one source file.

[iamwhatever]

Tier 1 auto-approve: fix (3 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: moves the full-source _old_item_ids read (one row per item, seconds of blocking SQLite on a large library) off the event loop into the existing off-loop duplicate-gate hop, closing #4441 with no change to gate ordering.