fix: load knowledge in workbench context fallback#201
Conversation
📝 WalkthroughWalkthroughQueryService now backfills missing workbench panels when needed and returns computed knowledge_graph data in both shared-memory and database fallback paths. StateBootstrap now loads narrative knowledge into shared memory, refactors foreshadow loading, and expands bootstrap and refresh sequences. ChangesKnowledge graph backfill and loading
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/engine/services/query_service.py (1)
571-589: 🚀 Performance & Scalability | 🔵 TrivialOn-demand backfill re-runs on every request for novels that legitimately lack these panels.
The trigger
not chronicles or plot_arc is None or knowledge is None or not foreshadowscannot distinguish "not yet loaded" from "genuinely empty". For a novel with no plot arc / knowledge / foreshadows (e.g. early-stage works), this branch will hit SQLite (plus Bible + snapshots loads for the chronicles case) on every/workbench-contextcall, undermining the "memory read" fast-path this endpoint is designed around. Consider a per-novel "backfill attempted" marker in shared state, or gating the backfill so a genuinely-empty result is cached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/engine/services/query_service.py` around lines 571 - 589, The workbench context backfill in QueryService currently treats genuinely empty panels the same as unloaded ones, so the `if not chronicles or plot_arc is None or knowledge is None or not foreshadows` branch keeps re-running on every request. Update `query_service.py` around `QueryService`’s on-demand backfill logic to distinguish “not yet attempted” from “legitimately empty,” using a per-novel shared-state marker or cached empty-result flag. Ensure `_load_foreshadows`, `_load_plot_arc`, `_load_knowledge`, and the chronicles/Bible/snapshots path only run once per novel, and reuse the cached empty state on later `/workbench-context` calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/engine/services/query_service.py`:
- Around line 571-589: The workbench context backfill in QueryService currently
treats genuinely empty panels the same as unloaded ones, so the `if not
chronicles or plot_arc is None or knowledge is None or not foreshadows` branch
keeps re-running on every request. Update `query_service.py` around
`QueryService`’s on-demand backfill logic to distinguish “not yet attempted”
from “legitimately empty,” using a per-novel shared-state marker or cached
empty-result flag. Ensure `_load_foreshadows`, `_load_plot_arc`,
`_load_knowledge`, and the chronicles/Bible/snapshots path only run once per
novel, and reuse the cached empty state on later `/workbench-context` calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 754abb22-bddd-49c3-80c9-4d09e2054078
📒 Files selected for processing (2)
application/engine/services/query_service.pyapplication/engine/services/state_bootstrap.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/engine/services/query_service.py (1)
572-609: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBackfill doesn't load triples, so
knowledge_graphmay be incorrect in the shared-memory path.The backfill loads foreshadows, plot_arc, knowledge, bible, snapshots, and chronicles — but never calls
bootstrap._load_triples(novel_id). Meanwhile,knowledge_graphis computed at line 609 viaself.get_knowledge_graph(novel_id), which reads triples from shared state. If the original bootstrap didn't warm triples (the exact "older bootstrap path" scenario the comment describes), the returnedknowledge_graphwill showtotal_triples: 0even when triples exist in SQLite.Additionally,
needs_backfillat line 580 doesn't check for missing triples, so if all other panels are present but triples are absent, the backfill won't trigger at all.🔧 Proposed fix: load triples during backfill and include in eligibility check
needs_backfill = not chronicles or plot_arc is None or knowledge is None or not foreshadows + triples = self._shared.get_triples(novel_id) + needs_backfill = needs_backfill or not triples if needs_backfill and not backfill_attempted: try: from application.engine.services.state_bootstrap import StateBootstrap bootstrap = StateBootstrap() + if not triples: + bootstrap._load_triples(novel_id) if not foreshadows: foreshadows = bootstrap._load_foreshadows(novel_id) if plot_arc is None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/engine/services/query_service.py` around lines 572 - 609, The workbench backfill in query_service.py does not hydrate triples, so `get_knowledge_graph(novel_id)` can return stale/empty data in the shared-state path. Update the backfill block around `needs_backfill` and the `StateBootstrap` calls to also load triples when they are missing, and include triples in the backfill eligibility check so the backfill runs when only triples are absent. Keep the fix localized to the backfill flow that currently loads foreshadows, plot_arc, knowledge, bible, snapshots, and chronicles, and ensure the shared-memory state is updated before `get_knowledge_graph` is computed.
🧹 Nitpick comments (1)
application/engine/services/query_service.py (1)
648-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing
get_knowledge_graph()instead of manually computing.The manual aggregation duplicates the logic in
get_knowledge_graph()(lines 761–771). Sincebootstrap._load_triples(novel_id)was called on the preceding line and likely populates shared state (matching the pattern of other_load_*methods),self.get_knowledge_graph(novel_id)would return the same result with less duplication.♻️ Proposed refactor
bootstrap._load_bible(novel_id) - triples_data = bootstrap._load_triples(novel_id) + bootstrap._load_triples(novel_id) bootstrap._load_snapshots(novel_id) knowledge_data = bootstrap._load_knowledge(novel_id) chronicles_data = bootstrap._load_chronicles(novel_id) - knowledge_graph = { - "total_triples": len(triples_data), - "by_source": {}, - } - for t in triples_data: - src = t.get("source_type", "unknown") - knowledge_graph["by_source"][src] = knowledge_graph["by_source"].get(src, 0) + 1 + knowledge_graph = self.get_knowledge_graph(novel_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/engine/services/query_service.py` around lines 648 - 654, The knowledge graph aggregation in the query service is duplicated logic; replace the manual construction in this branch with a call to get_knowledge_graph(novel_id) after bootstrap._load_triples(novel_id) so the existing method handles the totals and by_source counting. Keep the surrounding flow in QueryService consistent with the other _load_* patterns and remove the local loop that recomputes the same result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@application/engine/services/query_service.py`:
- Around line 572-609: The workbench backfill in query_service.py does not
hydrate triples, so `get_knowledge_graph(novel_id)` can return stale/empty data
in the shared-state path. Update the backfill block around `needs_backfill` and
the `StateBootstrap` calls to also load triples when they are missing, and
include triples in the backfill eligibility check so the backfill runs when only
triples are absent. Keep the fix localized to the backfill flow that currently
loads foreshadows, plot_arc, knowledge, bible, snapshots, and chronicles, and
ensure the shared-memory state is updated before `get_knowledge_graph` is
computed.
---
Nitpick comments:
In `@application/engine/services/query_service.py`:
- Around line 648-654: The knowledge graph aggregation in the query service is
duplicated logic; replace the manual construction in this branch with a call to
get_knowledge_graph(novel_id) after bootstrap._load_triples(novel_id) so the
existing method handles the totals and by_source counting. Keep the surrounding
flow in QueryService consistent with the other _load_* patterns and remove the
local loop that recomputes the same result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e953d424-64a5-498d-9c3e-307b3ce24120
📒 Files selected for processing (1)
application/engine/services/query_service.py
1 participant
变更类型
fixBug 修复feat新功能refactor重构（不影响功能）perf性能优化docs文档Summary
Fixes a workbench context fallback failure where
QueryService._fallback_workbench_from_db()callsStateBootstrap._load_knowledge(), butStateBootstrapdid not implement that method.Changes
StateBootstrap._load_knowledge()to load premise lock, chapter summaries, and knowledge facts into shared state.foreshadowstable when the registry row is absent.Why
Without this fix,
/api/v1/novels/{novel_id}/workbench-contextcan return empty or failed fallback data even when knowledge and chapter summaries already exist in SQLite.Verification
py_compilefor:application/engine/services/state_bootstrap.pyapplication/engine/services/query_service.pySummary by CodeRabbit
New Features
Bug Fixes