feat(retrieval): graph-expansion fact-readback (bi-temporal Phase 1, default-off) - #191
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| "derived": True, | ||
| "metadata": {"derived": True, "kind": "kg_facts", "triples": expanded}, | ||
| } | ||
| return [*results, derived] |
There was a problem hiding this comment.
WARNING: Derived block is not project/agent scoped, so it can leak cross-project KG facts into a scoped answer.
This block is built from the global TemporalKnowledgeGraph (which has no project dimension; query_entity never receives one) and is appended here before _filter_project_scope. Because the block's metadata has no project/agent key, that filter never drops it (hit_project stays None). The primary served hits are project-filtered, but the KG readback surfaces facts about the discovered entities regardless of which project learned them. Under a project-scoped query, the derived block can expose facts sourced from other projects' turns.
Consider threading project/search_agents into _append_graph_expansion and filtering format_expanded_context/the expanded triples by project scope, or document explicitly that the derived block intentionally crosses project boundaries.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "rank": len(results), | ||
| "source_score": top_confidence, | ||
| "derived": True, | ||
| "metadata": {"derived": True, "kind": "kg_facts", "triples": expanded}, |
There was a problem hiding this comment.
SUGGESTION: metadata["triples"] stores the full expanded list (up to max_expanded=10) while text is truncated to the token budget by format_expanded_context.
The visible block and the structured triples can disagree: text may show 1 fact but metadata.triples exposes up to 10. Any downstream consumer that reads metadata.triples (e.g. a claims gate or provenance checker) will see facts that were never surfaced in text, undermining the "provenance-honest / what you see is what's served" guarantee. Store only the budget-truncated subset, or explicitly document that triples is the superset.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if not block: | ||
| return results | ||
| # Keep confidence: surface the strongest currently-valid triple's score. | ||
| top_confidence = max( |
There was a problem hiding this comment.
SUGGESTION: top_confidence is the max confidence across all expanded triples, not just the ones that fit the budget and appear in text.
format_expanded_context truncates the visible facts to max_tokens*4 chars, but source_score here reflects the strongest triple in the whole (pre-truncation) expanded list. The score attached to the derived result can therefore overstate the confidence of the facts actually shown. Compute top_confidence from only the triples included in block (e.g. have format_expanded_context return the kept subset) for an honest source_score.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| id="graph_expansion", label="Graph-expansion fact readback", | ||
| category="quality", scope="runtime", type="int", | ||
| config_key="controls.graph_expansion", default=0, int_range=(0, 2000), | ||
| cost="one KG read over the served hits' entities plus the added context tokens; the value is the derived block's token budget", |
There was a problem hiding this comment.
SUGGESTION: Cost text says "one KG read over the served hits' entities", but expand_from_results issues a query_entity call per seed entity (capped at 10 via unique_entities[:10]) plus BFS follow-ups (default max_hops=2, max_expanded=10).
So a single enabled query can perform ~10+ KG lookups, not one. The cost description (and the README/doc entries derived from it) understates the read amplification. Worth rephrasing to reflect the per-entity fan-out, or capping seed entities lower for the readback path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Note: the previous review SHA Fix these issues in Kilo Cloud Previous Review Summary (commit 37c35bc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 37c35bc)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by hy3-20260706:free · Input: 60.2K · Output: 4.7K · Cached: 227.1K |
…default-off) Wire the orphaned graph_expansion.expand_from_results into retrieve() behind a new default-off control. When graph_expansion is enabled and a "kg" source is present, the entities in the served hits are looked up in the already-populated bi-temporal knowledge graph and their currently-valid facts are appended as one derived-facts block (source "kg_expansion", derived=True), reading the fact store back into the answer context. Phase 1 is valid-time only (query_entity defaults to as_of=now); no as-of query surface is exposed (that is Phase 2). No new storage, schema, or extraction change. Provenance-honest: the block is labelled derived with confidence carried from the strongest triple, the underlying turns stay the primary evidence, and the claims gate is untouched. Fail-open: a missing, empty, or erroring KG is a silent no-op, and with the control off the retrieval path is byte-for-byte unchanged. The graph_expansion control is an int token budget (0 = off, the default), wired through the runtime controls into search() like adjacent_turns. EventQA validation (E-025) is pending before any default flip.
37c35bc to
39c7d65
Compare
…MemEval with an instrument-validity gate Phase-1 feasibility done before the design, per the N-026 lesson. The LongMemEval runner does not call retrieve() today (hand-assembles context at lines 358-403), extraction is regex-only, and the encounter-order seed cap lands 2.3 of 10 seeds on the KG. But the corpus coverage claim holds (untruncated per-turn extraction, 482 triples per question) and a synthetic end-to-end probe through PR #191 fires 6/6 with the served context differing 6/6 at 0.111 saturation, against the 0.72 that doomed EventQA. So the instrument can see the effect, conditional on wiring the runner first. Instrument-validity gate and kill criterion both fixed verbatim before any arm runs. No results, no arm run, PR #191 unmerged, defaults unchanged.
1 participant
What this wires
The bi-temporal fact store was write-only at answer time: facts are extracted, validity-windowed, superseded, and never read back into what the generator sees. This PR wires the previously orphaned
graph_expansion.expand_from_results(built, referenced nowhere inretrieval.py/api.py/controls.py) intoretrieve(), behind a new default-off control.When
graph_expansionis on and a"kg"source is present, the entities in the served hits are looked up in the already-populated bi-temporal knowledge graph (query_entity) and their currently-valid facts are formatted (format_expanded_context) into one additional derived-facts block appended to the results. This is almost entirely wiring: the store layer (query_entity(as_of=...)withvalid_from/valid_to/superseded_byfiltering) and the integration layer (expand_from_results) already existed.Design
graph_expansion(runtime, quality) added totaosmd/controls.py. Typeint: the value is the derived block's token budget (max_tokens), and0= off. Default0(off). Wired through the runtime controls intosearch()exactly likeadjacent_turns -> adjacent_neighbors.retrieve()gains agraph_expansion: int = 0parameter (mirrorsadjacent_neighbors). In all four strategies (thorough / fast / minimal / custom), after the source merge, truncation, and neighbour attachment,_append_graph_expansion(results, sources.get("kg"), graph_expansion)appends the derived block as additional context (not counted againstlimit), following theapply_temporal_stageinjection pattern.source == "kg_expansion",derived == True,metadata.kind == "kg_facts"), confidence carried from the strongest currently-valid triple. The underlying turns remain the primary evidence; the claims gate is untouched.Phase 1 vs Phase 2 boundary
expand_from_resultspassesas_of=None, soquery_entityreads currently-valid facts (as_of=now). Noas-ofparameter is exposed on the public search / HTTP surface.as_ofon search/retrieve and HTTP, resolved from the query's temporal expression, calling the already-presentquery_entity(as_of=)path. Pre-registered separately (E-026).Default-off / byte-identical
With the control off (the default), the retrieval path is byte-for-byte unchanged: the
if graph_expansion:guard is false,_append_graph_expansionis never called, and existing callers are unaffected by the new keyword-only default. A regression test asserts the default run and an explicitgraph_expansion=0run are equal with nokg_expansionsource.Tests
TDD, added to
tests/test_retrieval.py: off-is-noop regression guard; on-appends-one-derived-block; empty-KG / erroring-KG / no-kg-source no-ops (fail-open); and a token-budget test (tight budget drops facts and stays withinmax_tokens*4chars, generous budget keeps more). Full suite: 1086 passed.Validation pending
EventQA validation (E-025, pre-registered) is a separate GPU step and is pending. The feature is left ready for it. Do not flip the default until E-025 clears its kill criterion (> standard-error improvement on EventQA overall, <= 20% context-token increase).
HOLD for review — do not merge.