perf(ingest): incremental per-turn edge detection (#1000) - #1003
Conversation
Reviewer's GuideImplements incremental, per-turn semantic edge detection by scoping candidate-pair generation and write-gating to newly inserted beliefs while preserving full-store behavior, determinism, and per-belief caps, and updates ingest to use the new incremental path with comprehensive tests. Sequence diagram for incremental per-turn semantic edge detectionsequenceDiagram
participant Ingest as ingest__ingest_turn_ids
participant Store
participant RelDet as write_semantic_edges
participant Audit as relationships_audit
participant Dedup as _jaccard_prefiltered_pairs
Ingest->>RelDet: write_semantic_edges(Store, new_belief_ids=inserted)
activate RelDet
RelDet->>RelDet: restrict = frozenset(new_belief_ids)
RelDet->>Audit: relationships_audit(Store, restrict_to_ids=restrict)
activate Audit
Audit->>Dedup: _jaccard_prefiltered_pairs(beliefs, max_pairs, restrict_to_ids=restrict)
activate Dedup
Dedup-->>Audit: candidates, raw_count, truncated
deactivate Dedup
Audit-->>RelDet: RelationshipsAuditReport(pairs)
deactivate Audit
RelDet->>RelDet: high_pairs = [p for p in report.pairs if p.relationship == EDGE_CONTRADICTS]
RelDet->>Store: edges_for_beliefs(endpoint_ids)
Store-->>RelDet: existing_edges
loop for each high_pair
RelDet->>Store: get_edge(src_id, dst_id, EDGE_CONTRADICTS)
alt edge_missing
RelDet->>Store: insert_edge(Edge(src, dst, EDGE_CONTRADICTS))
else edge_exists
RelDet->>RelDet: [increment budget only if full-store mode]
end
end
RelDet-->>Ingest: SemanticEdgeWriteReport
deactivate RelDet
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 38 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
robotrocketscience
added
the
attn:review
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The meaning of
raw_countin_jaccard_prefiltered_pairshas changed from "all O(n²) pairs visited" to "pairs actually scored", and now depends onrestrict_to_ids; if any callers rely on the previous interpretation, consider explicitly documenting this behavioral change at the public call sites (e.g.,relationships_audit) or renaming the field to avoid ambiguity. - The tests in
test_relationship_detector_incremental.pyreach intostore._conndirectly to inspect edges; consider adding a small public helper onMemoryStore(or using existing APIs) to expose this information so the tests don’t depend on private attributes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The meaning of `raw_count` in `_jaccard_prefiltered_pairs` has changed from "all O(n²) pairs visited" to "pairs actually scored", and now depends on `restrict_to_ids`; if any callers rely on the previous interpretation, consider explicitly documenting this behavioral change at the public call sites (e.g., `relationships_audit`) or renaming the field to avoid ambiguity.
- The tests in `test_relationship_detector_incremental.py` reach into `store._conn` directly to inspect edges; consider adding a small public helper on `MemoryStore` (or using existing APIs) to expose this information so the tests don’t depend on private attributes.
## Individual Comments
### Comment 1
<location path="tests/test_relationship_detector_incremental.py" line_range="78-84" />
<code_context>
+ return b
+
+
+def _contradicts_edges(store: MemoryStore) -> list[tuple[str, str, str]]:
+ """Return all CONTRADICTS edges as sorted (src, dst, type) tuples."""
+ rows = store._conn.execute( # type: ignore[attr-defined]
+ "SELECT src, dst, type FROM edges WHERE type = ? ORDER BY src, dst",
+ (EDGE_CONTRADICTS,),
+ ).fetchall()
+ return [(r[0], r[1], r[2]) for r in rows]
+
+
</code_context>
<issue_to_address>
**nitpick:** Avoid reaching into the store’s private connection in tests if there’s a public helper available, or add a small wrapper for clarity.
`_contradicts_edges` is reaching into `store._conn` directly. Prefer using a public API for fetching edges (or add a small test-only wrapper that calls `store.edges_for_beliefs`) so the tests don’t depend on the store’s internal schema/attributes. If no such API exists, at least add a TODO noting that changes to `_conn` or the edges schema will require updating these tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
[claim:review:idnn:2026-06-23T18:13:47Z] |
|
Needs rebase — conflicts with just-merged #1002. #1002 (issue #999, SUPPORTS/SUPERSEDES edge writers) landed on The overlap is concentrated at the ingest auto-detect gate in Please rebase onto current |
robotrocketscience
added
attn:merge-conflict
|
[release:review:idnn:2026-06-23T18:14:07Z] |
robotrocketscience
added
ready-to-merge
|
Needs a rebase on current Pulled |
…airs When restrict_to_ids is provided (a frozenset of belief IDs), skip any pair where neither endpoint appears in the set before scoring. raw_count counts only pairs actually scored (not skipped). Default None preserves byte-identical behaviour.
…hips_audit Adds keyword-only restrict_to_ids param (set/frozenset/None). When provided, passes it as a frozenset to _jaccard_prefiltered_pairs so candidate-pair generation is scoped to pairs touching the given IDs. Default None preserves byte-identical full-store audit behaviour.
…_semantic_edges with store-seeded write-gate budget Adds keyword-only new_belief_ids param (Sequence[str] | None). When provided, scopes the audit to pairs touching at least one new belief (delta from the current turn), which is provably equivalent to the full-store audit for discovering new edges. In incremental mode the write-gate budget per endpoint is seeded from existing CONTRADICTS edges in the store before the write loop so the hard per-belief cap is enforced across turns, not just within one call. The existing-skip branch only increments edges_per_belief in full-store mode to avoid double-counting the seeded values. Default new_belief_ids=None preserves byte-identical full-store behaviour.
…ntal edge detection Changes write_semantic_edges(store) to write_semantic_edges(store, new_belief_ids=inserted) so the edge audit is scoped to pairs touching the beliefs newly inserted that turn rather than re-scanning the whole store O(n²) every turn.
…lence, determinism, hard-cap, restrict) Five test groups: 1. Incremental per-turn calls produce the same final CONTRADICTS edge set as a single full-store audit (equivalence proof). 2. Two incremental builds on identical input are byte-equal (determinism). 3. Full-store mode (new_belief_ids=None) is byte-identical to the original path (backward-compat / no-op parity, idempotency). 4. Hub belief never exceeds max_edges_per_belief across turns (hard-cap enforcement via store-seeded write-gate budget). 5. _jaccard_prefiltered_pairs restrict_to_ids unit test: every returned pair has at least one endpoint in the set; None returns the superset.
4d44668 to
46a6c86
Compare
robotrocketscience
added
attn:review
|
Rebased onto |
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: tests/test_relationship_detector_incremental.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
|
[claim:review:gylf:2026-06-23T18:24:38Z] |
Remove unused 'import pytest' and 'DEFAULT_MAX_EDGES_PER_BELIEF' from the incremental edge-detection test module. Both were imported but never referenced; CodeQL flagged them as code-scanning alerts, blocking the merge-train on unresolved review threads. The store._conn access the Sourcery nitpick flagged is consistent with established convention across the test suite (test_auditor, test_clamp_ghosts, etc.) and is left as-is.
|
[claim:review:Setr:2026-06-23T18:28:04Z] |
|
[release:review:Setr:2026-06-23T18:28:08Z] |
|
Review complete. The incremental delta-scoping is sound: scoping candidate-pair generation to pairs touching at least one One semantics note for the record (not blocking): incremental == full-store edge set holds without cap pressure; under cap pressure the two modes may select different surviving edges (incremental writes turn-by-turn, full-store sees all pairs in one global sort), though both honor the ≤cap bound. Test 1 correctly scopes its equivalence assertion to the no-cap case. Cleared the blockers: removed two CodeQL-flagged unused imports ( |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 5fec7eb → |
|
[release:review:gylf:2026-06-23T18:31:44Z] |
|
Review: APPROVE (with one scoping note for the operator) Verified against post-#1002
Scoping note (operator call, not a blocker): this PR makes CONTRADICTS only incremental. SUPPORTS/SUPERSEDES still run a full-store audit every turn, tracked as the follow-up #1004. Per a fresh #1001 LoCoMo10 measurement on the post-#1002 substrate, SUPPORTS dominates the edge set 18,707 to 1,659 CONTRADICTS (866 SUPERSEDES) — so the bulk of the per-turn O(n²) cost #1000 cites is in the writers this PR does not yet scope. The latency win here is real but partial; #1000's 25–40 min/conv goal isn't met until #1004 lands. Flagging so the #1000↔#1004 issue linkage is a conscious choice (close #1000 now with #1004 as the tracked remainder, vs. keep #1000 open until #1004). Code is correct regardless of that linkage choice. Adding |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 5fec7eb → |
2 participants
Summary
Makes the
#988semantic-edge build incremental so per-turn auto-relationship ingest no longer re-audits the whole store on every turn.Closes #1000.
Problem
write_semantic_edges(store)audits the entire store via an O(n²) candidate-pair scan capped atmax_candidate_pairs(5000). Wired into ingest per-turn, this produced two unsatisfactory build paths (both measured on LoCoMo10):Same root cause: candidate-pair generation rescans the whole store instead of scoping to what changed.
Fix
Scope candidate-pair generation to pairs touching at least one belief newly inserted that turn (the delta
insertedalready computed iningest._ingest_turn_ids).Why this is equivalent to the full audit: every old-old high-confidence pair (both endpoints from prior turns) was already generated and processed in the turn when the later of its two endpoints was first inserted. So the only new edges a full audit writes each turn are exactly the delta-involving pairs — restricting to them discovers the same edges without the whole-store rescan. Because each turn adds a bounded number of pairs, the per-turn build never hits the 5000-pair cap, recovering the ~57% the single pass lost.
Write-gate preserved across turns
max_edges_per_belief(default 8) caps how many CONTRADICTS edges a belief accretes. The full audit enforced this by re-encountering a belief's prior edges each call. In incremental mode those old-old pairs aren't generated, so each endpoint's write-gate budget is seeded from its existing CONTRADICTS edge count in the store before the write loop — making the cap a true hard per-belief bound across turns. Full-store mode (new_belief_ids=None) is byte-identical to before.Changes (atomic commits)
refactor(dedup)—restrict_to_idsfilter on_jaccard_prefiltered_pairs(skip before Jaccard;None⇒ unchanged).feat(relationship_detector)— threadrestrict_to_idsthroughrelationships_audit.feat(relationship_detector)—new_belief_idsincremental mode inwrite_semantic_edgeswith store-seeded write-gate budget.feat(ingest)— pass the per-turn delta (inserted) towrite_semantic_edges.test(relationship_detector)— 7 tests: incremental==full equivalence, determinism, full-store/idempotent backward-compat, hard-cap across turns, andrestrict_to_idsunit coverage.Determinism / scope
Stdlib-only, no
random/embeddings/LLM — #605 posture holds. Sort and canonicalization unchanged, so edge tables stay byte-stable. Default-off flag (AELFRICE_AUTO_RELATIONSHIPS) unchanged; a fresh install still writes no edges and full-store mode is byte-identical.Acceptance
max_candidate_pairs.LoCoMo10 wall-clock and exact-count reproduction against the per-turn baseline are a lab-side verification (the corpus lives lab-side); the equivalence and hard-cap properties are proven here by unit tests on synthetic stores.
Summary by Sourcery
Make semantic edge detection incremental by scoping per-turn candidate generation to newly inserted beliefs, preserving determinism and full-store behavior while avoiding whole-store rescans.
Enhancements:
Tests: