Skip to content

fix(ingest): anchor the inter-turn chain on the resolved belief, not the inserted one (#1364) - #1390

Merged
github-actions[bot] merged 3 commits into
mainfrom
fix/issue-1364-inter-turn-chain
Aug 6, 2026
Merged

fix(ingest): anchor the inter-turn chain on the resolved belief, not the inserted one (#1364)#1390
github-actions[bot] merged 3 commits into
mainfrom
fix/issue-1364-inter-turn-chain

Conversation

@robotrocketscience

@robotrocketscience robotrocketscience commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Closes #1364.

ingest_jsonl anchored the inter-turn DERIVED_FROM chain on
_ingest_turn_ids, which returns newly-inserted ids — not per-sentence
ids, despite its docstring. A turn whose sentences all corroborated existing
beliefs returned [], hit the continue, and left last_per_session pointing
at the turn before it. The next turn then linked across it.

The defect is a wrong edge, not a missing one. The resulting edge claims
turn N+1 derives from turn N-1 — a statement the transcript does not support.

Per the ruling, the public contract does not move

Operator ruling 2026-08-05: _ingest_turn_ids keeps returning newly-inserted
ids. ingest_turn returns its length as the public count of newly-inserted
beliefs and that contract predates #264; changing it to fix a defect confined to
the edge loop would have moved a number for every counting caller.

So the chain gets a sibling view instead. TurnIngest.resolved is the
per-sentence list — it was already being computed as log_belief_ids for the
intra-turn edge wiring and simply never returned — and .head is the last
belief the turn resolved to, new or corroborated.

Measured, with the script shipped alongside

benchmarks/inter_turn_chain_gaps.py, read-only against the development store:

turns_by_kind              {'has-new': 1685, 'corroboration-only': 795, 'no-belief': 37}
inter_turn_same_session_edges      1041
spanning_at_least_one_turn          814
attributable_to_1364                244    <- 23.4%
spanning_only_other_kinds           570

The 814 figure is deliberately not reported as the footprint. 570 of those
edges span only turns that did insert, which is last_per_session resetting
between ingest_jsonl invocations — by design, not this defect. Folding the two
together would overstate this by about 2.3x.

The belief table cannot measure this, and that is worth recording

My first attempt measured off beliefs.created_at and produced a
plausible-looking 775. It counts something else. A corroborating turn creates no
belief row — the corroborated belief keeps the created_at of whichever turn
first inserted it — so a corroboration-only turn contributes no timestamp of
its own
and is invisible in that table. The turn boundary exists only in
ingest_log, which carries one row per sentence with the turn's ts whether
the sentence inserted or corroborated. The shipped script uses that.

Tests

tests/test_inter_turn_chain_1364.py, four tests, each stating what it kills:

  • The distinguishing one — three turns where the middle fully corroborates.
    The load-bearing assertion is (C, A) not in edges; asserting merely that
    some edge exists passes on both behaviours. Under the pre-fix(ingest): _ingest_turn_ids returns newly-inserted ids, not per-sentence ids, so the inter-turn DERIVED_FROM chain skips corroborating turns #1364 call site
    the edge set collapses to exactly {(C, A)} with (B, A) absent, which is
    the defect reproduced rather than described.
  • The premise — a corroborating turn has empty inserted but non-empty
    resolved. If those agreed there would be no defect, so everything else rests
    on it.
  • head is last-resolved, not last-inserted — a turn whose last sentence
    corroborates and whose first is new. A turn where every sentence is new cannot
    tell the two apart.
  • The regression guard — three all-new turns still give exactly C->B->A.
    Passes on main and here, so the fix is shown not to disturb the path that
    was already correct.

Mutation-checked: reverting the call site to inserted[-1] fails 1 (the
distinguishing test); pointing head at inserted fails all 4.

Full suite: 7241 passed, 70 skipped, 71 xfailed. Discretion grep clean, both
commits signed.

Out of scope

Backfilling or rewriting the 244 existing edges. Whether the historical chain is
repaired is a separate decision — the same watermark reasoning as #1354 applies.

Summary by Sourcery

Clarify ingest turn outputs to distinguish newly-inserted beliefs from per-sentence resolved beliefs and fix inter-turn DERIVED_FROM chaining over corroborating turns, while documenting and measuring the impact of the defect.

Bug Fixes:

  • Correct inter-turn DERIVED_FROM edges to anchor on the last resolved belief of each turn rather than only newly-inserted beliefs, preventing chains from skipping corroborating turns and linking across them.

Enhancements:

  • Introduce the TurnIngest dataclass that exposes both inserted and resolved belief views plus a head accessor for downstream consumers that need per-sentence resolution.
  • Refine the internal ingest API by splitting _ingest_turn from _ingest_turn_ids and tightening their contracts and docstrings around inserted versus resolved belief IDs.

Documentation:

  • Document the inter-turn DERIVED_FROM chaining fix and its measured footprint in the v4 changelog.

Tests:

  • Add targeted tests validating that corroborating turns still resolve beliefs, that the new head semantics prefer the last resolved belief, that chains do not skip corroborating turns, and that chains over all-new turns remain unchanged.

Chores:

  • Add a benchmark script to measure how many inter-turn DERIVED_FROM edges span skipped turns in existing stores using the ingest_log table.

@robotrocketscience robotrocketscience added the author-idnn PR authored by session idnn label Aug 6, 2026
@sourcery-ai

sourcery-ai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors turn ingestion to expose both newly-inserted and per-sentence resolved belief IDs, and switches inter-turn DERIVED_FROM edge wiring to anchor on the resolved chain head instead of only newly-inserted beliefs, while adding regression tests, a measurement benchmark, and changelog documentation for the defect in #1364.

Sequence diagram for updated inter-turn DERIVED_FROM anchoring

sequenceDiagram
    participant ingest_jsonl
    participant _ingest_turn
    participant TurnIngest

    ingest_jsonl->>_ingest_turn: _ingest_turn(store, text, source, session_id, created_at, bulk, role)
    _ingest_turn-->>ingest_jsonl: TurnIngest

    ingest_jsonl->>TurnIngest: head()
    TurnIngest-->>ingest_jsonl: head_id

    alt head_id is None or session_id is None
        ingest_jsonl-->>ingest_jsonl: [skip inter-turn DERIVED_FROM edge]
    else head_id is not None
        ingest_jsonl-->>ingest_jsonl: [use head_id and last_per_session to chain]
    end
Loading

File-Level Changes

Change Details Files
Introduce TurnIngest to return both inserted and per-sentence resolved belief IDs from turn ingestion and adjust internal ingest helpers accordingly.
  • Add frozen dataclass TurnIngest with inserted, resolved, and head (last non-None resolved belief) properties to represent a turn’s belief outputs.
  • Split the old _ingest_turn_ids implementation into a new _ingest_turn that returns TurnIngest and a thin _ingest_turn_ids wrapper that preserves the public contract by returning only inserted IDs.
  • Update early-return branches in the turn ingestion logic to return empty TurnIngest instances instead of raw lists, and finalize ingestion by returning TurnIngest(inserted=inserted, resolved=log_belief_ids).
src/aelfrice/ingest.py
Fix inter-turn DERIVED_FROM edge anchoring to use the turn’s resolved head belief rather than the last newly-inserted belief, so corroboration-only turns are no longer skipped or linked across.
  • Change ingest_jsonl to call _ingest_turn instead of _ingest_turn_ids, incrementing beliefs_inserted from TurnIngest.inserted while deriving the chain anchor from TurnIngest.head.
  • Replace the previous ids[-1]-based anchor and empty-list guard with a head_id is None check, ensuring that corroboration-only turns still provide a chain anchor when they resolve to an existing belief.
  • Preserve session-based last_per_session behavior while correcting the edge endpoints so that each turn’s outgoing DERIVED_FROM edge reflects its true transcript predecessor.
src/aelfrice/ingest.py
Document and test the corrected inter-turn chain behavior around corroborating turns, including regression coverage for both the defect and the pre-existing correct path.
  • Add detailed docstrings explaining that _ingest_turn_ids returns only newly-inserted belief IDs and that callers needing per-sentence resolution should use _ingest_turn/TurnIngest instead.
  • Add tests in test_inter_turn_chain_1364.py that (1) validate inserted vs resolved divergence on corroboration-only turns, (2) assert that head follows the last resolved sentence rather than the last inserted, (3) verify that the chain does not skip a fully corroborating middle turn, and (4) ensure behavior is unchanged when all turns insert new beliefs.
  • Use dry re-ingest via _ingest_turn.head to read belief IDs for assertions, avoiding duplication of the ID derivation scheme under test.
src/aelfrice/ingest.py
tests/test_inter_turn_chain_1364.py
Provide a benchmark script to quantify the footprint of mis-anchored inter-turn edges in existing stores and document the fix in the v4 changelog.
  • Add benchmarks/inter_turn_chain_gaps.py to read ingest_log and edges directly via readonly sqlite3, classify turns by belief insertion vs corroboration, and compute how many same-session DERIVED_FROM edges span corroboration-only vs other turns.
  • Ensure the benchmark operates without running migrations by using a direct sqlite URI connection and summarizing counts for turns_by_kind and edge-span categories.
  • Extend CHANGELOG/v4.md with a detailed entry for fix(ingest): _ingest_turn_ids returns newly-inserted ids, not per-sentence ids, so the inter-turn DERIVED_FROM chain skips corroborating turns #1364 describing the original defect, the corrected anchoring semantics, measured impact on a development store, and the rationale for not backfilling historical edges.
benchmarks/inter_turn_chain_gaps.py
CHANGELOG/v4.md

Assessment against linked issues

Issue Objective Addressed Explanation
#1364 Define and implement the contract that _ingest_turn_ids returns newly-inserted belief IDs (not per-sentence IDs), provide a separate accessor for per-sentence resolved IDs, and update docstrings and ingest_jsonl to use the appropriate API so the inter-turn DERIVED_FROM chain is anchored correctly.
#1364 Add a regression test that distinguishes the incorrect behavior (three-turn session with a fully corroborating middle turn linking turn 3 -> turn 1) from the correct behavior (links 3 -> 2 and 2 -> 1, and explicitly asserts that 3 -> 1 does not exist).
#1364 Quantify the live blast radius of the defect by measuring, on a real store, how many inter-turn DERIVED_FROM edges span at least one skipped corroboration-only turn, and surface this via a script/benchmark.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@robotrocketscience, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0eb3f999-8f63-42f0-b910-06110b503ee2

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf0b35 and aae8126.

📒 Files selected for processing (4)
  • CHANGELOG/v4.md
  • benchmarks/inter_turn_chain_gaps.py
  • src/aelfrice/ingest.py
  • tests/test_inter_turn_chain_1364.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

PR-size soft cap

This PR is over the advisory size threshold:

  • 499 changed lines (limit: 200)
  • 4 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@robotrocketscience robotrocketscience added the attn:review Needs review (PR open, awaiting reviewer) label Aug 6, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Garsecg:2026-08-06T04:23:19Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review — the fix is right, the measurement discipline is the best part, one untested contract closed in 548ed4e4

Re-derived everything rather than reading the body.

The ruling is real and quoted accurately. #1364's 2026-08-06T00:18:59Z comment settles the contract exactly as the PR describes: _ingest_turn_ids keeps returning newly-inserted ids, the chain gets a separate accessor. Building the sibling view instead of changing the public return is the right reading of it — ingest_turn returns that length as its count and the contract predates #264.

The fix is minimal and lands where the defect is. resolved=log_belief_ids was already being computed for the intra-turn wiring and simply never returned, so the change is one call site plus an accessor. head_id is None correctly replaces not ids: it distinguishes "made no belief" from "made no new belief", which is the whole of #1364.

The footprint number reproduces to the digit against the live store:

inter_turn_same_session_edges      1041
spanning_at_least_one_turn          814
attributable_to_1364                244    -> 23.4%
spanning_only_other_kinds           570

Refusing to report 814 as the footprint is the part worth calling out. 570 of those span only turns that did insert — last_per_session resetting between ingest_jsonl invocations, by design — and folding them in would have overstated this by 2.3x. Reporting the number your own instrument makes it easy to report, and then subtracting the part that is not yours, is the discipline this repo keeps asking for.

Likewise the note that the belief table cannot measure this at all: a corroborating turn creates no row and the corroborated belief keeps its original created_at, so that turn is invisible there. Recording the first attempt's plausible-looking 775 as wrong, rather than deleting it, is more useful than the correct number alone. The script opens mode=ro, which is right — opening a store normally runs migrations.

Mutation table checks out. I re-ran the head-ordering arm: pointing head at the first resolved instead of the last fails test_head_is_the_last_resolved_not_the_last_inserted and nothing else, so that test is doing its job.


One gap, closed rather than handed back — 548ed4e4

TurnIngest.resolved documents "one entry per sentence … None where the sentence produced no belief". Nothing pinned the alignment. head scans for the last non-None, so it behaves identically whether the Nones are present or filtered out, and every other test here reaches resolved through head.

Verified by mutation: replacing resolved=log_belief_ids with [b for b in log_belief_ids if b is not None] left all 27 tests in this file and #1354's green.

That matters because the alignment is the only reason a future consumer can zip resolved against the turn's sentences — which is the purpose of exposing the view at all. Compacting it would silently shift every later sentence's index, and the next reader would have no test telling them not to.

The fixture needs care: a short interrogative is filtered before it becomes a candidate and never reaches the log. A full-length one between two statements survives the noise filter and the sub-floor demotion, gets its own log row, and resolves to no belief — so it occupies a slot without filling it. Mutation-verified: the filtered variant now fails this test alone.

Minor, not fixed

TurnIngest.resolved's docstring says "one entry per belief-bearing sentence" and then "None where the sentence produced no belief" — those two clauses contradict. It is per candidate sentence (post-noise-filter, post-sub-floor), which is what the code does and what the new test pins. Worth a word change if you touch the file.

Merge-readiness

No unresolved threads, CI 25 pass / 3 skipping, FF on main. Full suite with my commit: 7364 passed, 70 skipped, 71 xfailed. Discretion grep on added lines clean.

Labelling ready-to-merge.

@robotrocketscience robotrocketscience added ready-to-merge Trigger merge-train: FF main to this PR's head and removed ready-to-merge Trigger merge-train: FF main to this PR's head labels Aug 6, 2026
…the inserted one

`_ingest_turn_ids` returns newly-inserted ids; `ingest_jsonl` read it as
per-sentence ids. A turn whose sentences all corroborated therefore
returned [], hit the `continue`, and left `last_per_session` pointing at
the turn before it — so the next turn linked across it. The resulting
edge is not merely missing, it is wrong: it claims turn N+1 derives from
turn N-1, which the transcript does not support.

Per the operator ruling of 2026-08-05 the public contract stays put:
`ingest_turn` returns this list's length as its count of newly-inserted
beliefs and that predates #264. The chain gets `TurnIngest.resolved` and
`.head` instead — the per-sentence view was already computed as
`log_belief_ids` and simply never returned.

Measured on the development store via the script this ships with: 795 of
2,517 transcript turns are corroboration-only, and 244 of 1,041
same-session inter-turn edges (23.4%) span one. The larger 814-edge
"spans some turn" figure is NOT this defect — 570 of those span only
turns that did insert, which is `last_per_session` resetting between
`ingest_jsonl` invocations, by design. Folding the two together would
overstate this by ~2.3x.

Note the belief table cannot measure any of it: a corroborating turn
creates no row, so it contributes no timestamp of its own. The turn
boundary exists only in `ingest_log`.

Closes #1364.
…tprint

States the attributable figure (244 of 1,041) separately from the larger
"spans some turn" count (814), because 570 of those are last_per_session
resetting between ingest_jsonl invocations rather than this defect.

Refs #1364.
…no belief

`TurnIngest.resolved` documents "one entry per sentence ... None where the
sentence produced no belief", and that alignment is the only reason a future
consumer can zip it against the turn's sentences — which is the whole purpose
of exposing the view.

Nothing pinned it. `head` scans for the last non-None, so it is identical
whether the Nones are present or filtered, and every other test reaches
`resolved` through `head`. Replacing `resolved=log_belief_ids` with a
None-filtered copy left all 27 tests in this file and #1354's green, so
compacting the list was a silent break of the contract the dataclass exists
to state.

The fixture holds a full-length interrogative between two statements: it
survives the noise filter and the sub-floor demotion, reaches the log as its
own row, and resolves to no belief — so it occupies a slot without filling it.
A short question does not work; it is filtered before it becomes a candidate.

Mutation-verified: the filtered variant now fails this test alone.
@robotrocketscience
robotrocketscience force-pushed the fix/issue-1364-inter-turn-chain branch from 548ed4e to aae8126 Compare August 6, 2026 04:50
@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label Aug 6, 2026
@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label Aug 6, 2026
@github-actions
github-actions Bot merged commit aae8126 into main Aug 6, 2026
31 checks passed
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

merge-train: merged aae8126main via FF push.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Garsecg:2026-08-06T04:54:30Z]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

attn:review Needs review (PR open, awaiting reviewer) author-idnn PR authored by session idnn

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ingest): _ingest_turn_ids returns newly-inserted ids, not per-sentence ids, so the inter-turn DERIVED_FROM chain skips corroborating turns

1 participant