fix(knowledge): scope the on-loop DB guard out of store construction (#8231) - #8331
fix(knowledge): scope the on-loop DB guard out of store construction (#8231)#8331bolichen97 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Scoped ContextVar opt-out mirrors the established history.py pattern, pins its one sanctioned use with an AST test, and honestly tracks the real boot-stall as #8329. [DESIGN-REVIEWED] 21b9b0e |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All counts verified. Writing the review. First-Principles-Verdict: PASS A guard that cannot represent a vetted take warns falsely every boot and burns its 60s throttle window; this fixes that at the guard, cause filed for the rest. What this change shipsIntent: stop the on-loop DB warning that fires on every gateway launch for the store construction that is deliberate (#8231) — a FIX.
The fix sits at the mechanism of the reported defect (the guard had no way to mark a sanctioned take); the deeper loop-stall cause is declared out of scope and filed (#8329), which the contract accepts. Delete-option checked: not reading the store at route registration is not a deletion — Subtractions
[FIRST-PRINCIPLES-REVIEWED] 21b9b0e |
…8231) setup_knowledge_routes() reads the gateway's lazy knowledge_store property at route registration, before the socket binds, so KnowledgeStore.__init__ (schema init, migrations, graph load) runs on the event-loop thread on every launch -- by the constructor's documented design. OnLoopDBGuard fires at the db accessor for every caller, so each boot logged a spurious on-loop warning for a take that is deliberate. Give OnLoopDBGuard a ContextVar-scoped allow_on_loop() opt-out (mirroring history.allow_on_loop_persist, but per guard instance so one store's vetted take cannot mute another's diagnostic) and wrap exactly the constructor's three init calls in it. The suppression ends with the with block: the six non-constructor _load_graph() call sites and every reader/writer path stay fully guarded, and strict mode still raises there. Sanctioned is not free: _migrate() runs an unconditional writer-locked orphan sweep and _load_graph() full-scans two tables on that same boot path. Moving that work off the loop needs a design decision and is filed as #8329; this change only removes the spurious diagnostic. Regression tests pin both directions: construction on the loop is silent (and does not raise under strict) while a genuine on-loop reader take immediately after construction still warns/raises; the opt-out resets on exception; an AST mutation guard pins the with block's body to exactly the three init calls, so a call dedented out or a fourth call joining in both fail the test. Closes #8231
159a113 to
21b9b0e
Compare
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @bolichen97 overrides the GPT 5.6 finding for 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: |
github-actions
Bot
added
readiness: action required
|
Disposition: REBUT (override) — span=src/kiro_crew/knowledge/store.py:387 "Constructor bypasses the on-loop database guard"
|
Human judgment recorded@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
1 participant
Summary
On every gateway launch the knowledge store logged a spurious on-loop DB warning (#8231).
setup_knowledge_routes()reads the gateway's lazyknowledge_storeproperty at route registration, before the socket binds, soKnowledgeStore.__init__— schema init, migrations, graph load — runs on the event-loop thread on every boot, by the constructor's own documented design.OnLoopDBGuard.check()fires at thedbaccessor for every caller, so the deliberate construction take warned on each start.What changed
src/kiro_crew/on_loop_db.py:OnLoopDBGuardgains aContextVar-scopedallow_on_loop()context manager (mirrorshistory.allow_on_loop_persist, but per guard instance so one store's vetted take cannot mute another's diagnostic).check()returns early only inside such a block; nothing about its global behaviour changes. Module docstring updated (the history opt-out is now mirrored here), ContextVar name slugified, module-level-singleton requirement documented (aContextVarseen by anyContextis never collected).src/kiro_crew/knowledge/store.py: the constructor wraps exactly_init_schema()/_migrate()/_load_graph()in the opt-out. The suppression ends with thewithblock — the six non-constructor_load_graph()call sites and every reader/writer path stay fully guarded, and strict mode still raises there.test/test_knowledge_store_onloop_db.py: newTestConstructionOnLoopIsSanctioned— construction on the loop is silent and does not raise under strict, a genuine on-loop reader take immediately after construction still warns, the opt-out resets on exception (exact== 1warn count), and an AST mutation guard pins thewithblock body to exactly the three init calls (a call dedented out AND a fourth call joining in both red).test/test_knowledge_ingestion_off_loop.py:_RecordingGuardstub mirrors the new API and its suppression semantics (load-bearing: without it the store constructor raisesAttributeErrorunder the monkeypatch).docs/system-specs/modules/knowledge.md: guard section documents the constructor opt-out, naming the REAL call site (route registration, pre-bind) and the real cost.What this deliberately does NOT do
The sanctioned take is not free:
_migrate()runs an unconditional writer-locked orphan sweep and_load_graph()full-scansentities/entity_relationson that same pre-bind boot path, so a large knowledge profile can still stall the loop there. Moving that work off the boot path (defer the graph load like the FTS rebuild, or construct the store off route registration) needs a design decision and is filed as #8329. This PR only removes the spurious diagnostic for the take that is deliberate; it does not weakencheck()globally nor silence the warning category.Review
Two pre-push model-pinned lanes ran. GPT (gpt-5.6-sol): NO-FINDINGS with scenario evidence (ContextVar task/thread isolation, suppression scope,
__slots__, exception safety, cross-OS, recorder fidelity). Opus (claude-opus-5): 1 BLOCKING + 4 CONCERNS, all verified real and all addressed — the BLOCKING was the first draft's FALSE rationale ("lazily from an async handler", "cheap") written into comments and the spec; comments/docs now name the real call site and cost, and the underlying boot-path work is #8329. The four CONCERNS (module-docstring contradiction, textual-order mutation guard, over-broad caplog assert, ContextVar naming/lifetime) are fixed in this head.Tests
python -m pytest test/test_knowledge_store_onloop_db.py test/test_knowledge_ingestion_off_loop.py test/test_no_blocking_call_on_loop.py test/test_auto_research_onloop_db.py test/test_folder_watcher_off_loop_guard.py test/test_knowledge.py— 302 passed (plus the 82-test focused set re-run after review fixes).withblock reds the AST guard.Pattern harvest
Rule candidate: a diagnostic-suppression change must record WHY the suppressed signal is safe to lose, verified against the real call graph — a plausible-but-unchecked rationale ("lazy", "cheap") written into docs is worse than no rationale, because the next reader builds on it.
Class: comment-contradicts-code / false-rationale-in-spec.
Closes #8231