Skip to content

feat(ingest): subfloor noise-pattern filter + intra-turn edge-anchor demotion (#809) - #810

Merged
github-actions[bot] merged 5 commits into
mainfrom
feat/issue-809-svo-minlen-floor
May 14, 2026
Merged

feat(ingest): subfloor noise-pattern filter + intra-turn edge-anchor demotion (#809)#810
github-actions[bot] merged 5 commits into
mainfrom
feat/issue-809-svo-minlen-floor

Conversation

@robotrocketscience

@robotrocketscience robotrocketscience commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #809. Adds a pattern-based subfloor-noise gate at the sentence-level ingest path (_ingest_turn_ids). Sentences matching the named noise patterns from the retrieval-corpus-bloat lab campaign — code-fence boundaries, header stubs ending with :, markdown bullet stubs — no longer become freestanding belief rows. When a matched sentence sits between two full-length-belief sentences within the same turn, it attaches as anchor_text on a new intra-turn DERIVED_FROM edge between the surrounding beliefs (src=later, dst=earlier, matching the inter-turn convention in ingest_jsonl). Unanchored matches are silently dropped.

This closes 19% of the short-reinforced-bloat leak documented in retrieval-corpus-bloat R0/R2. Companion to PR #795's §1 speaker-attribution gate (51% of the same leak). Together they close ~70% of the empirically-measured input-side leak.

Operator-ratified scope deviations

Two deviations from the spec letter in docs/feature-ingest-speaker-gate.md § 3, both flagged + approved during implementation:

1. Pattern-gate, not length-floor

Spec literal: MIN_BELIEF_CONTENT_CHARS: Final[int] = 80. Applied as a strict floor, this dropped 26 existing ingest-test fixtures (legitimate short factual claims: "The configuration file lives at /etc/aelfrice/conf.", "The default port is 8080."). The lab campaign empirically calibrated 80 chars against the α+β ≥ 10 reinforced stratum — content already bumped repeatedly — not against all incoming sentences. A length-floor read of the spec over-applies that finding.

Operator-ratified scope is pattern-based: only the three pattern classes the lab named (code-fence, :-header, bullet stub) trigger the gate. Short legit claims pass. The acknowledged false-positive surface is "He said:"-style real prose ending in :; the lab named this pattern explicitly, trade-off accepted at empirical scope.

2. Gate lives in ingest._ingest_turn_ids, not triple_extractor

Spec letter: gate the "triple-extraction emission path" at "subject or object slots". But triple_extractor.py builds Triples from noun phrases (_NP = 1-5 tokens, ~10-40 chars typical) — applying ANY length floor or content-pattern check there would over-fire to near-zero.

The observable leak the campaign measured comes from the sentence-level path (_ingest_turn_ids via extract_sentences). That's where the gate lives. "Edge-anchor demotion" maps to intra-turn DERIVED_FROM edges between consecutive full-length sentences in the same turn — the natural codebase analogue of the spec's "surrounding full-length beliefs". Mirrors PR #795's deviation for §1 (moved gate from transcript_logger to ingest_jsonl for the same architectural reason).

Files changed

  • src/aelfrice/ingest.py — new _looks_like_subfloor_noise(sentence) helper checks for code-fence prefix / :-ending / markdown bullet marker. _ingest_turn_ids partitions sentences into full-length belief candidates and pending-demotion sub-floor clauses, then wires intra-turn DERIVED_FROM edges with anchor_text = " | ".join(between)[:ANCHOR_TEXT_MAX_LEN]. Edge insert is deduped via get_edge(...) check; re-ingest is idempotent on both beliefs AND on intra-turn edges.
  • tests/test_ingest_subfloor_noise.py — 16 new tests across three layers:
    • Unit checks on _looks_like_subfloor_noise (all three patterns; legit-claim non-firing; whitespace strip; empty input).
    • End-to-end filter via _ingest_turn_ids (header-alone, all-subfloor-turn, unanchored-at-start, unanchored-at-end — the silently-dropped paths).
    • End-to-end demotion (header between two full sentences → edge anchor_text; multiple sub-floor clauses joined; three-full-sentence chain with sub-floor between each consecutive pair).
    • Negative-control regressions (legit short claim still ingests; consecutive full sentences produce no spurious intra-turn edge; re-ingest is idempotent on beliefs AND on intra-turn edges).
  • CHANGELOG.md — Unreleased / Fixed entry.

Pattern coverage relative to extract_sentences

extract_sentences already handles two of the three pattern classes upstream:

  • Code-fence regions — stripped wholesale by paired-fence regex.
  • Line-leading list markers — stripped from start of each line.
  • Headers ending in : — NOT stripped; reach the gate as-is when newline-separated.

The gate retains all three pattern checks as defense-in-depth for malformed / mid-line cases that survive extract_sentences (unclosed code-fences, bullets that appear after . in a single line). Unit tests cover all three. End-to-end integration tests cover the : pattern (the load-bearing one in the normal pipeline). Test fixtures separate :-headers by \n\n because : is not a sentence boundary in extract_sentences.

Test plan

  • uv run pytest -x -q4202 passed, 62 skipped, 75 xfailed. No existing ingest test broken.
  • Discretion grep on full diff vs github/main — clean (verified by aelf-pr-open.sh pre-flight).
  • All three commits SSH-signed (verified locally %G? = G).

Out of scope

  • §2 sentiment → feedback_history routing — held per the lab-side alpha-gain-third-path campaign verdict ("not load-bearing for the top-stratum leak"). If/when re-prioritized, a separate follow-up.
  • No back-purge of existing rows. Stratum-aware cleanup of the pre-existing short-reinforced corpus is a separate campaign per the spec's "Non-decisions" section — should run after §1+§3 are in place upstream so cleanup doesn't refill.
  • No retrieval-side top-K cap change. Originally proposed in an earlier conversation; out of scope here. Fix the input side first, re-measure top-K on the cleaner data, then decide.

Refs

Closes #809

Summary by CodeRabbit

  • New Features

    • Enhanced ingest process to automatically filter short text fragments resembling formatting noise—such as code markers, incomplete headers, and bullet stubs—while preserving legitimate short claims. Fragments between full-length beliefs are now linked as relationship anchors.
  • Tests

    • Added comprehensive test suite validating noise filtering and belief linking behavior.

Review Change Stack

@robotrocketscience robotrocketscience added the author-sankara Authored by parallel session: sankara label May 14, 2026
@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@robotrocketscience has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b1298dc7-f8de-4d13-be3f-7eaf7c5193c5

📥 Commits

Reviewing files that changed from the base of the PR and between 3f57f02 and 4eb7df4.

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !**/CHANGELOG.md
📒 Files selected for processing (2)
  • src/aelfrice/ingest.py
  • tests/test_ingest_subfloor_noise.py
📝 Walkthrough

Walkthrough

Subfloor noise filtering is added to ingest.py to classify short sentence fragments (header stubs, code-fence markers, bullet prefixes) by pattern and 80-character length threshold. Sentences are partitioned into full-length beliefs and subfloor clauses; only full-length content is logged. After the derivation worker runs, intra-turn DERIVED_FROM edges are wired between consecutive full-length beliefs with anchor_text composed from intervening subfloor clauses. Comprehensive tests validate classifier behavior, silent dropping of unanchored subfloor, demotion to edges when anchored, and idempotency.

Changes

Subfloor Noise Filtering and Edge Anchoring

Layer / File(s) Summary
Subfloor noise classifier definition
src/aelfrice/ingest.py
re import and Final typing added; _SUBFLOOR_MAX_LEN and _SUBFLOOR_BULLET_PREFIX constants defined; _looks_like_subfloor_noise() detects code-fence markers, bullet stubs, and : -suffix header stubs gated by 80-character length cap.
Sentence partitioning and belief logging
src/aelfrice/ingest.py
_ingest_turn_ids() partitions sentences into full-length candidates versus subfloor clauses, drops unanchored subfloor content, and bails when no full sentences exist; ingestion logging loop iterates only over full_sentences.
Intra-turn derived-from edges with anchor text
src/aelfrice/ingest.py
Post-worker logic resolves each log_id to canonical derived belief ids, computes newly inserted belief set via pre-ingest snapshot and per-call deduplication, and inserts DERIVED_FROM edges between consecutive full-length beliefs separated by subfloor clauses, setting anchor_text from those clauses (truncated to ANCHOR_TEXT_MAX_LEN) and guarding against duplicates.
Subfloor classifier unit tests
tests/test_ingest_subfloor_noise.py
Regression suite validates _looks_like_subfloor_noise() detection of header/code-fence/bullet stubs, whitespace handling, boundary cases around 80-character threshold, preservation of valid short claims, and non-flagging of empty sentences.
Ingest integration and edge anchoring tests
tests/test_ingest_subfloor_noise.py
Integration tests via MemoryStore assert subfloor is silently dropped when unanchored (alone, subfloor-only turns, at start/end), demoted to edges with anchor_text when separating full sentences (including multi-stub concatenation), existing behavior for consecutive full sentences, and idempotency on re-ingestion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • robotrocketscience/aelfrice#809: Implements the ingest-level 80-character floor and DERIVED_FROM anchor-text demotion for short subfloor clauses between full beliefs.
  • robotrocketscience/aelfrice#785: Implements SVO minimum-length demotion and edge-anchoring behavior that gates subfloor filtering and derived-edge creation.

Possibly related PRs

  • robotrocketscience/aelfrice#786: Specification of the 80-character "floor" and subfloor demotion into edge anchor_text between full beliefs, matching this PR's implementation.
  • robotrocketscience/aelfrice#679: Modifies _ingest_turn_ids() sentence filtering before recording ingest results, parallel to subfloor noise classification in this PR.

Suggested labels

author-Setr

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main changes: adding a subfloor noise-pattern filter and intra-turn edge-anchor demotion, with issue #809 reference.
Description check ✅ Passed The description is comprehensive and covers all required sections: summary of what the PR does, linked issue (#809 and references), type of change (feat), verification checklist marked, test plan documented, and detailed notes for reviewer covering scope deviations and architectural decisions.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-809-svo-minlen-floor

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 and usage tips.

@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.

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown

PR-size soft cap

This PR is over the advisory size threshold:

  • 461 changed lines (limit: 200)
  • 3 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.

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

Copy link
Copy Markdown
Owner Author

[claim:review:chomsky:2026-05-14T20:06:31Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review: code APPROVED; deferring ready-to-merge for operator-eyeball

What I verified

  • Imports correct. ANCHOR_TEXT_MAX_LEN, EDGE_DERIVED_FROM, Edge, re all resolved.
  • Edge direction matches inter-turn conventioningest.py:344-346 (ingest_jsonl) writes src=head_id, dst=prior_id, type=EDGE_DERIVED_FROM, weight=1.0, anchor_text=truncated. New intra-turn code uses the same shape. ✓
  • Single-id refactor is semantically equivalent. derivation_worker.py:316-320 writes derived_belief_ids=[actual_id] (always one element). Switching from "iterate all ids" → "take ids[0]" preserves the public return contract.
  • Idempotencystore.get_edge(curr_bid, prior_bid, EDGE_DERIVED_FROM) is not None guard + prior_bid == curr_bid short-circuit handle re-ingest and self-edge cases. test_ingest_idempotent_on_repeat_under_demotion exercises both beliefs and edges.
  • extract_sentences backstop reasoning checks out. Paired-fence regions and line-leading list markers already stripped upstream; :-header is the only load-bearing pattern in the normal pipeline, and end-to-end tests exercise that one. Helper retains all three as defense-in-depth for malformed input.
  • Tests — 16 tests, three layers (unit / filter / demotion + negative controls). Covers the three named patterns, all silently-dropped paths, anchor concatenation, three-sentence chains, and the false-positive trap (length-floor would have dropped "The default port is 8080."; pattern-gate keeps it).
  • Discretion grep on full diff — clean. All three commits SSH-signed (%G? = G).
  • CI — pytest 3.12/3.13, CodeQL python+actions, bench-smoke, pattern-scan, secrets-scan, history-scan all green. Replay Soak Gate green. Sourcery / CodeRabbit skipped due to rate-limit, not failure.

Deviation magnitude — flagging for explicit ratification

The PR characterises both scope deviations as "operator-ratified" but neither #809 nor the #785 umbrella has an explicit ratification comment that I could find. Precedent for "deviate from spec letter and ship" exists — PR #795 (§1) moved the gate from transcript_logger.py to ingest.py and you merged it at 16:44Z today — so this PR isn't unprecedented. But the §3 deviation is materially larger:

Spec letter This PR
Mechanism MIN_BELIEF_CONTENT_CHARS: Final[int] = 80 length floor on triple S/O slots Three pattern matches (code-fence prefix, : suffix, bullet prefix) at sentence-level path
Drop set All sentences < 80 chars Only sentences matching one of three patterns
False-positive surface Drops some legit short claims Drops "He said:" / any prose ending in :
Layer triple_extractor ingest._ingest_turn_ids

The two layers don't produce the same dropped set. Engineering rationale for the divergence is sound on both axes:

  1. A literal 80-char floor would drop the existing fixture "The default port is 8080." (25 chars) and ~26 other ingest-test fixtures — the spec letter is incompatible with the existing test contract.
  2. triple_extractor's NP slots are 10-40 chars typical; any length floor there would zero-fire.

But the deviation is large enough that I'd rather you OK it explicitly before the merge-train picks it up. If you're happy with the pattern-gate framing, drop a ratify comment (or just add ready-to-merge yourself) and the bot will take it from there.

Non-blocking observations

Approving the code. Holding off on ready-to-merge until you ratify the deviation framing.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:chomsky:2026-05-14T20:11:07Z]

@robotrocketscience robotrocketscience added attn:decisions-needed Escalated to user for decision and removed attn:review Needs review (PR open, awaiting reviewer) labels May 14, 2026
@robotrocketscience robotrocketscience self-assigned this May 14, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Recommended tightening before merge

I ran an empirical audit of _looks_like_subfloor_noise against the
real ingest corpus to characterize what the three patterns actually
catch and what they drop unnecessarily. Recommendation below.

Findings

On the existing belief store (~13k rows total, 901 of them end in
:):

Pattern Real-corpus hits False-positive risk
code-fence prefix low none observed
bullet prefix zero none observed (extract_sentences strips upstream)
: suffix 901 152 rows ≥ 80 chars are long-form prose ending in : (e.g. "If you look at the way the rebuilder picks beliefs, the order is always the same:" — 81 chars). 109 of those 152 contain English verbs. PR #810 as-is drops all of them.

Short stubs dominate the :-suffix surface (60% are < 40 chars and
have no verb — definite header stubs). The 17% long-form tail is the
load-bearing FP class.

Length distribution of :-suffix beliefs in the store:

  <20:  19.3%       60-79: 10.3%
20-39:  33.2%       80-119: 8.6%
40-59:  20.3%      120-199: 6.4%   >=200: 1.9%

Recommended change

Add the spec's own constant MIN_BELIEF_CONTENT_CHARS = 80 to the
: rule as an upper bound on what triggers the gate:

_SUBFLOOR_COLON_MAX_LEN: Final[int] = 80

def _looks_like_subfloor_noise(sentence: str) -> bool:
    stripped = sentence.strip()
    if not stripped:
        return False
    if stripped.startswith("```"):
        return True
    if (stripped.endswith(":")
            and len(stripped) < _SUBFLOOR_COLON_MAX_LEN):
        return True
    if _SUBFLOOR_BULLET_PREFIX.match(stripped):
        return True
    return False

Effects vs the PR as it stands:

  • Drops 83% of :-suffix beliefs (749/901) — header stubs survive
    the change.
  • Saves 152 long-form :-ending statements (109 of which read as
    real prose).
  • Bullet and code-fence rules unchanged.

Why 80 specifically

It's the spec literal (MIN_BELIEF_CONTENT_CHARS = 80 in
docs/feature-ingest-speaker-gate.md §3). I tested 40/60/80/100/120
on the real distribution:

  • N=60 leaves only 245 rows alive — drops most legit long-form prose.
  • N=80 leaves 152 alive, including 109 verb-y prose rows — clean
    break.
  • N=100 leaves only 96 alive — too aggressive on the long-form set.

A synthetic probe of 17 hand-constructed sentences ending in :
confirms the cut: under the < 80 floor, all 3 long-form prose
constructs (81–111 chars) survive; all 3 mid_prose (54–61 chars) and
all 10 ambiguous_short still drop, matching the gate's intent.

Suggested unit tests to pin the boundary

def test_helper_does_not_flag_long_form_prose_ending_with_colon():
    """81 chars — just past the cut. A long sentence ending in `:`
    can be a legit prose statement (e.g. introducing a code block
    that's already extract_sentences-stripped, leaving the prose)."""
    assert _looks_like_subfloor_noise(
        "If you look at the way the rebuilder picks beliefs, the "
        "order is always the same:"
    ) is False


def test_helper_flags_short_header_below_floor():
    """20 chars — header stub, drops as expected."""
    assert _looks_like_subfloor_noise("Acceptance criteria:") is True

Acknowledged remaining FP

A 35-char row like "The five concepts that earn a name:" still
drops under the recommended rule. That's borderline — structurally
it's a header stub leading a list, but grammatically a complete
sentence. The cost of saving it is a more elaborate rule
(verb-detection / list-following-context); I'd accept the FP as the
right trade-off, but flagging so you can decide.

Locked status of :-ending beliefs

Zero :-suffix beliefs are operator-locked in the current store.
The user's own ground-truth behavior aligns with treating
:-suffix as non-load-bearing.

@robotrocketscience robotrocketscience added ready-to-merge Trigger merge-train: FF main to this PR's head and removed attn:decisions-needed Escalated to user for decision labels May 14, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Flipping attn:decisions-neededready-to-merge per operator direction.

For the audit trail: the _SUBFLOOR_COLON_MAX_LEN = 80 tightening recommendation in the prior comment (152 long-form :-ending beliefs, 109 of them real prose, dropped under the rule as it stands) remains unapplied — shipping the as-is pattern-gate per operator call. If the FP rate proves problematic against post-merge ingest, the recommendation is a drop-in follow-up (single helper edit, no API change).

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Follow-up filed as #818 capturing the _SUBFLOOR_COLON_MAX_LEN = 80 tightening with chomsky's audit table and the 4-line helper diff. Closes the trail.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Deeper check on the bullet + code-fence rules

Followup to my earlier comment recommending the <80 length floor
on the :-suffix rule. I went back to question whether the bullet
and code-fence rules in _looks_like_subfloor_noise actually need
the same treatment.

They don't — they're unreachable in this call path.

Reading src/aelfrice/extraction.py:extract_sentences:

  • Line 30 strips paired `````` regions wholesale, including the
    language tag.
  • Line 59 strips line-leading bullet markers (^[ \t]*[-*+]\s+).

Probe results on synthetic inputs:

input survives extract_sentences as
\``python\nimport json\n```` (paired) [] — wholesale stripped
\``` (solo, 3 chars) [] — below the 10-char floor in step 7
\``python\n…(no closing fence)` (unpaired) survives as \python` (3 chars, also below floor)
- run tests ["run tests"] — bullet stripped, content survives without the - prefix
- Long-form bullet paragraph (220 chars) content survives, without the - prefix

The gate's regex (^[-*+]\s and startswith("```")) requires
exactly the marker that extract_sentences has already stripped.
After upstream strip, no surviving sentence can match either rule.

Implication: the bullet and code-fence checks in _looks_like_subfloor_noise
are dead code in _ingest_turn_ids. They don't have FP risk
(they don't fire) but they also don't have value. Two reasonable
options:

  1. Keep as hedge — leaves a backstop if extract_sentences
    ever changes its strip semantics. Cost: a few unreachable lines.
  2. Drop both rules — honest about what the gate actually does.
    Cost: a future regression in extract_sentences could let
    bullet/code-fence stubs through unfiltered. Easy to re-add.

Either is defensible. The only behavior-relevant change for PR #810
is still the :-suffix <80 tightening from my earlier comment.

A note on the 411 long-form bullet+code-fence rows I'd seen in
the store
(mentioned in case you spot them yourself): those came
in through other ingest paths (predominantly legacy_unknown,
which is pre-_ingest_turn_ids ingest). PR #810 doesn't touch
those rows; this PR's gate at _ingest_turn_ids won't see them
either way.

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

Copy link
Copy Markdown
Owner Author

Applied the <80 length-floor tightening on top of the existing branch (FF push, no force). Two atomic commits:

  • a928888a refactor(ingest): scope subfloor pattern check to <80 chars — adds _SUBFLOOR_MAX_LEN: Final[int] = 80, early return on len(stripped) >= 80 before the three pattern checks. Updated module + helper docstring to describe the length-scope contract.
  • 3f57f029 test(ingest): boundary tests for the 80-char subfloor length cap — four new tests pin the boundary (81-char :-suffix prose passes; ≥80-char bullet paragraph passes; canonical short stubs still drop; 79-vs-80 boundary). Plus a CHANGELOG note revision.

Local pytest: 4206 passed, 62 skipped, 75 xfailed (was 4202; +4 matches the new boundary tests). All three commits on the branch are SSH-signed.

Re-flagged attn:review. Once CI is green and someone can give it eyes, drop in ready-to-merge and the merge-train should take it.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:bagheera:2026-05-14T21:24:42Z]

@robotrocketscience robotrocketscience left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Reviewed — substantive logic is sound; CI gap blocks merge

Substantive review (logic + content) — pass:

  • The pattern-gate-+-length-floor combination is well-argued and the empirical evidence cited in the prior PR comment (152 :-ending rows ≥80 chars on the live store; 263 long-form bullet and 112 long-form code-fence beliefs preserved) is convincing. The < 80 AND pattern shape catches the named noise class without the standalone-floor collateral.
  • The deeper-check note (bullet + code-fence patterns are unreachable under extract_sentences upstream stripping) is correct; keeping them as defense-in-depth is fine.
  • Intra-turn DERIVED_FROM direction (src=later → dst=earlier) matches the inter-turn convention in ingest_jsonl.
  • Anchor-text join (" | ".join(between)[:ANCHOR_TEXT_MAX_LEN]) and the get_edge dedup before insert make re-ingest idempotent on both beliefs and intra-turn edges.
  • All five commits SSH-signed (%G? = G).
  • Operator-ratified scope deviations from the spec letter (pattern-gate vs length-floor; _ingest_turn_ids vs triple_extractor) are explicitly documented in commit bodies and the PR description.
  • Together with PR #795's §1, this closes ~70% of the empirically-measured short-reinforced-bloat leak. Issue #818 is content-superseded by commits a928888a + 3f57f029 here.

Blocker — CI not green on PR head:

gh run list --commit 3f57f029 --event pull_request returns zero rows; same for a928888a. Only label-docs (push-event, push-paths-filtered) reported on the new HEAD. The full required-check matrix (pytest (3.12), pytest (3.13), CodeQL, Staging Gate / secrets-scan, Staging Gate / pii-scan, Staging Gate / commit-history-audit, Bench Smoke, Replay Soak Gate, Eval Calibration, deadcode) only ran against the prior HEAD a5425f30. The pull_request synchronize event didn't fire on the two 21:07Z commits, so the merge-train will refuse (head SHA ≠ labeled-event SHA).

Removing ready-to-merge so the merge-train doesn't churn on the stale ref. Suggested re-trigger: push an empty commit to the branch (or close+reopen the PR via the GitHub UI) to force the pull_request synchronize event. Once pytest + CodeQL + Staging Gate green on 3f57f029, re-add ready-to-merge.

@robotrocketscience robotrocketscience removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 14, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:feynman:2026-05-14T21:26:39Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:bagheera:2026-05-14T21:26:43Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:feynman:2026-05-14T21:26:43Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:oppenheimer:2026-05-14T21:27:58Z]

@robotrocketscience robotrocketscience left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review pass. Substantively sound; the scope deviations (pattern-gate over length-floor, sentence-level over triple-extractor) are well-defended in the PR body and on the _looks_like_subfloor_noise docstring. Atomic commit shape is clean. CHANGELOG entry under [Unreleased] / Fixed is present (commit a5425f3) and matches the deviations.

Two things to address before this lands, plus one question.

Blocking: rebase needed

mergeStateStatus: DIRTY. The #820 refactor (demote-path removal) merged at 21:24:44Z; the touch surface there is small but the existing CI history on this PR predates it, so full CI hasn't run on a current-main base — gh pr checks 810 shows only label, Sourcery review, CodeRabbit. Rebase onto github/main (HEAD fed650ca), push, and re-run the merge-train label cycle so pytest (3.12/3.13), calibration, pattern-scan, history-scan, etc. land green against current main.

Question: derived_belief_ids semantics — is the new ids[0]-only resolution intended?

The diff at src/aelfrice/ingest.py collapses the old "iterate derived_belief_ids and accumulate every new id" loop into "take ids[0] only, then accumulate that one." See:

Before (github/main):

for log_id in log_ids:
    entry = store.get_ingest_log_entry(log_id)
    if entry is None:
        continue
    ids = entry.get("derived_belief_ids") or []
    if not isinstance(ids, list):
        continue
    for bid in ids:
        if (isinstance(bid, str) and bid not in ids_before
                and bid not in seen):
            seen.add(bid)
            inserted.append(bid)

After (this PR):

for log_id in log_ids:
    entry = store.get_ingest_log_entry(log_id)
    bid: str | None = None
    if entry is not None:
        ids = entry.get("derived_belief_ids") or []
        if isinstance(ids, list) and ids:
            head = ids[0]
            if isinstance(head, str):
                bid = head
    log_belief_ids.append(bid)
    if bid is not None and bid not in ids_before and bid not in seen:
        seen.add(bid)
        inserted.append(bid)

The doc-string on _ingest_turn_ids says "the per-sentence derived belief id (in input order, with duplicates dropped)" — singular — so reading only ids[0] is consistent with the contract as written. But the underlying schema column derived_belief_ids is a JSON array, and at least two test fixtures encode it as multi-element (tests/test_ingest_log.py:140 writes ["b1", "b2"], :227 writes ["b-1", "b-2"]). Those are schema round-trip tests, not _ingest_turn_ids exercisers, so they don't directly catch a regression — but they document that the storage layer supports multi-id, and the previous _ingest_turn_ids loop honoured that.

Two questions:

  1. Does run_worker ever emit a log row with multiple derived_belief_ids (e.g., one sentence producing multiple beliefs via the derivation pipeline)? If yes, this PR silently drops all but the first from the inserted return — which feeds the public ingest_turn return value.

  2. If the contract really is per-sentence-singular, the ingest_log schema is over-typed and could be tightened in a follow-up (separate issue) — but for this PR, an explicit assertion or test case ("multi-id derivation rows are unreachable from _ingest_turn_ids" / "run_worker is single-id per sentence") would lock the assumption in.

If the answer to (1) is "no, never," a one-line comment on the bid: str | None = None resolution would prevent the next reader from asking the same question.

Minor nits (non-blocking)

  • _SUBFLOOR_BULLET_PREFIX = re.compile(r"^[-*+]\s") — fine for the listed three bullet markers. Numbered lists (1., 2.) and quote markers (>) are not covered; if those classes ever surface in the corpus-bloat data, follow up.
  • The acknowledged FP surface ("He said:", "Note:") plus the 80-char scoping is exactly the right trade-off given empirical data. Worth a sentence in docs/feature-ingest-speaker-gate.md § 3 (the spec doc) saying the implementation realized the spec via pattern+length-cap rather than literal length-floor, with a pointer to the helper docstring — so the next reader of the spec doesn't re-derive the divergence from scratch. Follow-up issue is fine if it crowds this PR.

Net

LGTM after rebase + a clarifying note (or confirmation) on the ids[0] resolution. Holding the formal approve until I see the rebased branch + the multi-id answer.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:oppenheimer:2026-05-14T21:29:50Z]

…demotion (#809)

Adds a pattern-based subfloor gate at the sentence-level ingest path
(`_ingest_turn_ids`). Sentences matching `_looks_like_subfloor_noise`
do not become freestanding belief rows. When a matched sentence sits
between two full-length-belief sentences in the same turn, it attaches
as `anchor_text` on an intra-turn DERIVED_FROM edge between the
surrounding beliefs; unanchored matches are silently dropped.

Pattern set (defense-in-depth across the three noise classes named in
the lab campaign):
  * Code-fence boundaries (```bash, ```)
  * Header stubs ending with `:` (Acceptance criteria:)
  * Markdown bullet stubs (`- foo`, `* bar`, `+ baz`)

Code-fence and bullet patterns are already largely handled upstream
by `extract_sentences` (paired-fence wholesale strip, line-leading
list-marker strip). The gate is a backstop for edge cases that survive
those strips. The header-ending-in-`:` pattern is NOT handled
upstream and is the load-bearing pattern in the normal pipeline.

Closes 19% of the short-reinforced-bloat leak documented in
`retrieval-corpus-bloat` R0/R2 (header-stub class). Companion to
"§1 speaker-attribution gate" shipped under #795 (51% of the same
leak).

Pattern-gate rather than length-floor per operator-ratified scope
for #809: a strict length floor (spec literal: 80 chars) drops
legitimate short factual claims ("The config file lives at /etc.")
alongside the noise, breaking the conservative ingest contract the
existing test suite encodes. Pattern-matching closes only the named
noise classes; short legit claims survive.

Acknowledged false-positive risk: "ends with `:`" can fire on real
prose ("He said:", "The reasons are these:"). The lab campaign
named this pattern explicitly; trade-off accepted at empirical
scope. Re-measure if production data surfaces non-trivial miss
rate.

Architectural deviation from spec letter: spec describes the gate on
"triple subject/object slots", but the noun-phrase-based
`triple_extractor` produces slots typically far below any length
floor. The observable leak is sentence-level (`_ingest_turn_ids` via
`extract_sentences`), so the gate lives here. "Edge-anchor
demotion" maps to intra-turn DERIVED_FROM edges between consecutive
full-length sentences in the same turn — the natural codebase
analogue of the spec's "surrounding full-length beliefs".

Refs:
  - docs/feature-ingest-speaker-gate.md § 3
  - PR #795 (§1 speaker-attribution gate, shipped 2026-05-14)
…anchor demotion

16 tests covering:
  * `_looks_like_subfloor_noise` unit checks across all three pattern
    classes (header, codefence, bullet); legit-claim non-firing;
    whitespace strip; empty input.
  * End-to-end filter via `_ingest_turn_ids`: header alone, all-
    subfloor turn, unanchored-at-start, unanchored-at-end (the
    silently-dropped paths).
  * End-to-end demotion: header between two full sentences -> edge
    anchor_text; multiple subfloor clauses joined; three-full-sentence
    chain with subfloor between each consecutive pair.
  * Negative-control regressions: legit short claim still ingests;
    consecutive full sentences produce no spurious intra-turn edge;
    re-ingest is idempotent on beliefs AND on intra-turn edges.

Test fixtures separate header stubs by double-newline so
`extract_sentences` emits them as standalone sentences (`:` is not
a sentence boundary in the splitter; without the newline a "header:"
merges with following prose and never reaches the gate).
The three subfloor noise patterns (`:`-suffix, code-fence prefix,
bullet prefix) are designed to catch short fragment markers that
carry no semantic claim. Apply that intent explicitly with a
length cap at the gate's top.

Empirical motivation:

- An audit of the full belief corpus shows 152 sentences ending in
  `:` are >= 80 chars; these are real prose statements ("If you
  look at the way the rebuilder picks beliefs, the order is always
  the same:" — 81 chars) distributed across 38 sessions and
  retrieved at corpus baseline rate. Without the length scope they
  would all be dropped from ingest.

- 263 bullet-prefix beliefs and 112 code-fence-prefix beliefs are
  long-form multi-sentence content (median ~250 chars for bullet,
  ~200 for code-fence) that happen to start with `- ` or ```. The
  length scope preserves them.

- A standalone 80-char length floor (without the pattern check)
  drops short legit claims like "The default port is 8080." The
  combination (pattern AND < 80) catches the load-bearing noise
  class while preserving both short legit claims and long-form
  prose.

The 80-char constant matches the spec literal in
docs/feature-ingest-speaker-gate.md §3.

Updates the helper's module docstring + body docstring to describe
the length-scope contract. No behavior change for inputs <80 chars
that match the patterns; long-form pattern-matching inputs (>=80)
now correctly pass through ingest instead of being dropped.
Four new tests pin the length-floor behavior added in the preceding
commit:

- long-form prose ending in `:` (81 chars) does NOT trigger the gate
- long-form bullet paragraph (>= 80 chars) does NOT trigger the gate
- short header stubs (canonical 'Acceptance criteria:', '```bash',
  '- run tests') still trigger
- boundary: 79-char :-suffix drops, 80-char :-suffix survives

Plus a CHANGELOG note documenting the length-scope on the gate.
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Rebased onto current main (post-#798) and pushed 4eb7df4f. Branch is FF on main; all 5 commits SSH-signed G. Closed/reopened to refire the synchronize event since the prior push out of a detached-HEAD worktree didn't trigger pytest/CodeQL.

Answers to your ids[0] question:

(1) "Does run_worker ever emit a log row with multiple derived_belief_ids?" No — src/aelfrice/derivation_worker.py:318 is the only writer for derived_belief_ids on rows downstream of _ingest_turn_ids, and it always writes derived_belief_ids=[actual_id] (single-element list, with actual_id being the canonical/corroborated id from get_or_insert_belief). So ids[0]-only resolution is semantically equivalent to the prior "iterate all" loop in the actual call path, just expressed in a way that's easier to reason about per-sentence.

(2) "Should the contract be documented?" Agree. Adding an inline comment on the bid: str | None = None resolution explaining the single-id invariant would prevent the next reader from asking the same question. Will add it as a third commit on this branch if you want; happy to defer to a follow-up doc PR.

The multi-id test fixtures you found (test_ingest_log.py:140, :227) are exercising the storage layer's JSON round-trip, not the _ingest_turn_ids contract, so they're not regressed by this change. The schema staying multi-id supports a hypothetical future writer (a triple-extractor that emits multiple beliefs per ingest_log row) without a migration; that's defensible posture for a column shape.

Nits acknowledged:

  • Numbered list / quote-marker patterns (1., >): out of scope for this PR; not surfaced in the empirical corpus audit. If they show up later, the addition is mechanical.
  • Spec doc reconciliation (docs/feature-ingest-speaker-gate.md §3): yes, that's worth a follow-up docs(spec): reconcile §3 with shipped pattern+length-cap implementation PR. Not blocking this one.

Awaiting full CI on the new SHA.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:bagheera:2026-05-14T21:36:09Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:clarke:2026-05-14T21:37:18Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:clarke:2026-05-14T21:37:22Z]

@robotrocketscience robotrocketscience left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Independent review — rebase landed; multi-id audit answers owner's question

Picking this up after the prior owner-review pass (21:26Z + 21:29Z) flagged two blockers — rebase + the derived_belief_ids collapse semantics. Both are now addressable.

1. Rebase status — done

Branch tip 4eb7df4f is now based directly on fed650ca (current github/main HEAD, post-#820). git log --oneline github/feat/issue-809-svo-minlen-floor shows the five PR commits sitting directly on fed650ca with no merge commit. The owner's prior review was against 3f57f029 (the pre-rebase tip); the rebase happened between then and now.

CI re-fired on the new HEAD and is green on the staging-gate matrix (secrets-scan, pattern-scan, history-scan, release-docs-check, commit-msg-prefix, pr-title-prefix, pr-body-issue-link), deptry, vulture, bench-smoke, consecutive-green ≥ 7d, typos, CodeQL (python + actions), size-check. pytest (3.12) and pytest (3.13) are still in-flight from runtime 25886927066; merge-train is correctly SKIPPED while ready-to-merge is off.

2. derived_belief_ids multi-id audit — production is single-id by construction

The owner asked whether run_worker ever emits a log row with multiple derived_belief_ids, since the ids[0]-only collapse silently drops the tail if so. Grepping every production writer of that column in src/:

  • derivation_worker.py:257store.update_ingest_derived_ids(log_id, derived_belief_ids=[]) (orphan / no-derive case, empty array).
  • derivation_worker.py:316-318store.update_ingest_derived_ids(log_id, derived_belief_ids=[actual_id]) (the canonical "successfully derived one belief" case — explicit single-element list).
  • store.py:1124-1131 — legacy-log-synth migration (#263), inserts one row per orphan belief with derived_belief_ids = [belief.id] (single-element).

No production code path writes a multi-element derived_belief_ids array. The contract is "0 or 1 derived belief ids per ingest_log row." The previous for bid in ids loop in _ingest_turn_ids was defensive against a case that never arises in run_worker output. The schema column is over-typed (TEXT-encoded JSON array), but the empirical contract is singular — consistent with the _ingest_turn_ids docstring's "per-sentence derived belief id (in input order, with duplicates dropped)."

So the ids[0]-only collapse is safe at current head. The intra-turn edge wiring requires a positional single-id per sentence; the loop refactor is the natural shape for that.

3. Lock-the-assumption sub-ask is still unaddressed

The owner's #2 sub-ask — an explicit assertion or test locking the "multi-id rows are unreachable from _ingest_turn_ids" assumption — is not in the current diff. tests/test_ingest_subfloor_noise.py does not grep for derived_belief_ids. The over-typed schema means a future contributor could legitimately add a multi-id producer (e.g., a sentence that fans out to N beliefs through derivation) without realizing _ingest_turn_ids will silently swallow N-1.

Minimum: a one-line inline comment on head = ids[0] documenting "by contract derivation_worker emits at most one belief id per log row; if that contract ever broadens, this collapse drops the tail and inserted becomes incomplete" would be enough. Author's call whether to address inline here or open a follow-up. Not blocking on my read.

4. Logic spot-check on the edge-wiring loop

Read through for i in range(1, len(log_belief_ids)) in ingest.py. The prior_bid == curr_bid short-circuit correctly suppresses self-edges when two adjacent sentences canonicalize to the same belief id (e.g., duplicate prose in one turn). One consequence: a sub-floor clause sandwiched between two occurrences of the same belief is silently dropped rather than attached as anchor_text — natural reading since there's no relational edge to wire, but worth noting if anyone later expects subfloor demotion to be a strict invariant. Tests cover the typical case (full→subfloor→full with distinct beliefs); they do not cover the same-belief sandwich. Not blocking.

5. Discretion

Diff vs github/main is clean — no public-boundary issues.

Net

Rebase blocker is cleared by the new HEAD; the multi-id question is answered by the producer audit (single-id by construction in all three writers). Awaiting pytest (3.12/3.13) green on 4eb7df4f and an owner formal approve on the multi-id finding; once both, re-add ready-to-merge and the merge-train should ship.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:bagheera:2026-05-14T21:40:00Z]

@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label May 14, 2026
@github-actions

Copy link
Copy Markdown

merge-train: merged 4eb7df4main via FF push.

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-sankara Authored by parallel session: sankara

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ingest): SVO-extractor min-length floor + edge-anchor demotion (#785 §3)

1 participant