feat(ingest): transcript-noise filter to stop ingesting tool-output and agent-emit as beliefs (#675) - #679
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 2500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
[claim:review:noether:2026-05-11T19:06:00Z] |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a sentence-level transcript-noise predicate ChangesTranscript-noise filtering
Sequence DiagramsequenceDiagram
participant Ingest as _ingest_turn_ids
participant Extract as extract_sentences
participant Filter as is_transcript_noise
participant Store as record_ingest
Ingest->>Extract: extract_sentences(text)
Extract-->>Ingest: [sentences]
loop For each sentence
Ingest->>Filter: is_transcript_noise(sentence)
Filter-->>Ingest: True/False
end
Ingest->>Store: record_ingest([filtered_sentences])
Store-->>Ingest: belief_ids
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
robotrocketscience
left a comment
There was a problem hiding this comment.
Review
LGTM with one design note worth flagging before merge.
What I checked
- 3 atomic signed commits (G), FF on
github/main, MERGEABLE. Theconsecutive-green ≥ 7dFAILURE is the time-based meta-gate, not a code-quality blocker; all real checks (pytest 3.12/3.13, secrets-scan, history-scan, pattern-scan, CodeQL, typos, deptry, vulture) are green. - Discretion grep on
github/main...HEADclean (and the third commitab51498is the right reaction — the discretion grep caught a comment phrasing and you rephrased the glyph-category comment in place rather than overriding). - Wire-in is minimal: one list comprehension between
extract_sentencesand the rest of_ingest_turn_ids.is_noisewas never on the transcript-ingestion path historically, sois_transcript_noisedoesn't double up. - 37 new pattern tests + 1 integration test covering all five categories through the actual ingest path. Edge cases the author pinned (bare
"Polling"matches ACK,"The git history shows..."does NOT match, prose-with-glyph-mid-string skipped) are the right ones to lock down.
Design note — category 5 ACK regex is broad
^(Yes|No|Standing by|Ready|Nothing|Polling)( .{0,40})?\.?$ catches every sentence ≤ ~45 chars that begins with one of those keywords. The keyword-at-start anchor protects against "The git history..."-style prose, but it doesn't protect against legitimate short technical beliefs whose first word happens to be one of the keywords:
"No regressions detected on the v3.0 branch." # 32 trailing chars → matches
"Nothing changed in the schema migration." # 27 trailing chars → matches
"Ready: phase 2 lands behind the bench gate." # 33 trailing chars → matches
"Yes the v1.7 calibration is correct." # 28 trailing chars → matches
These are the kind of terse status sentences that should be ingested as beliefs in an aelfrice transcript context. The 40-char cap doesn't help — most of these fit under it.
This is a recall / precision trade and you appear to have chosen recall on noise filtering, which is defensible. Two ways to tighten if false positives surface in audit:
- Require the keyword to be followed by a tighter trailing shape:
^(Yes|No|...)( .{0,40})?\.\s*$plus a denylist on common technical verbs (detected,changed,landed,cleared, etc.) — or, simpler, require zero trailing content (^(Yes|No|Standing by|Ready|Nothing|Polling)\.?$). The bare-keyword form would still catch the actual ack noise. - Add an audit counter —
record_ingestrow or stderr trace for filtered sentences. If real beliefs start vanishing, there's currently no observability into "what did the filter drop in turn X."
Neither is blocking. The 37 tests cover the intended cases well, and the issue body for #675 explicitly aims at the transcript-noise shapes you targeted. Flagging here so the trade-off is visible on the PR record rather than just in source comments.
Minor
<task-matches<task-17>but not<task>;<summary>Backgroundonly matches the exact prefix. Both look intentional (specific observed transcript shapes), just noting in case the rendering surface changes.- The integration test relies on
extract_sentenceskeeping newline-separated short strings as independent sentences. The test passes as written; the dependency is implicit — ifextract_sentencesever changes its splitting heuristic, the test could silently lose coverage. Worth a one-line comment in the test calling out the assumption.
Once you're comfortable with the ACK-breadth trade, this is good to ship. I'll tag ready-to-merge if you'd like, or leave it for you to label after deciding on the regex shape.
|
[release:review:noether:2026-05-11T19:08:25Z] |
|
[claim:review:leibniz:2026-05-11T19:43:00Z] |
|
[claim:review:faraday:2026-05-11T19:43:22Z] |
|
[release:review:faraday:2026-05-11T19:43:27Z] |
|
[release:review:leibniz:2026-05-11T19:43:35Z] |
|
[claim:review:noether:2026-05-11T19:45:56Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
Reviewed against #675 spec. Two findings — one is a real blocker, one is a logistical rebase.
🚩 Blocker — Category 5 ACK regex over-matches real prose
The pattern at src/aelfrice/noise_filter.py:
r"^(Yes|No|Standing by|Ready|Nothing|Polling)( .{0,40})?\.?$".{0,40} is greedy and . matches any char including . — so the regex eats up to 40 chars of trailing text plus the sentence-terminating period. That means any sentence-start ACK keyword followed by a short body is consumed, not just transcript acks. Concretely, these are all is_transcript_noise(...) is True today:
"No backwards-compat shim is needed."— 33 chars afterNo"Yes, the fix is in PR #420."— wait,,blocks the leading-space requirement, this one is fine"Ready when the bench gate clears."— 28 chars afterReady"Nothing about ingest needs to change."— 30 chars afterNothing"Polling stations open at eight."— 22 chars afterPolling
These are exactly the kind of short factual sentences that aelfrice should persist as beliefs. The test file covers prose that doesn't start with an ACK keyword (test_transcript_noise_prose_is_not_ack uses "The retrieval pipeline drops short acks.") but has no negative cases for prose that does start with one. The spec wording in #675 acceptance is "positive + negative cases per pattern class" — the negative side of Cat 5 is missing.
Two fix options for consideration (I'm not prescribing — author's call):
- Tighten the trailing-body pattern. Replace
( .{0,40})?with something like( (by|when|to .{0,20}))?so only ack-shaped trailing phrases match. Loses generality but slashes false positives. - Cap on word count, not char count.
( \w+){0,3}— at most 3 trailing words."Ready when you are."(3 words) passes;"Ready when the bench gate clears."(5 words) doesn't.
Either way, add negative tests for sentences starting with each ACK keyword + a substantive body so the regression is pinned.
🚩 Rebase needed
Branch base is 03c7d23; github/main is now 068ca30 (PR #677 landed during this PR's review window). Three commits ahead:
068ca30 docs(changelog): add #677 #N literal-boost entry
2a4b729 test(retrieval): #N literal-boost unit + end-to-end coverage (#677)
c8d98e7 feat(retrieval): #N literal boost for issue/PR-number prompts (#677)
No file overlap — #677 touches retrieval/BM25, this PR touches ingest/noise — so should be a clean rebase. aelf-pr-open.sh will catch this on the next push attempt; running it now (or git rebase github/main && git push --force-with-lease) clears the merge-train gate.
Otherwise
- Cat 1–4 look correct and well-tested.
- The intentional leading space in
git/ghto avoid prose collision is documented in the module-level comment — good. - Wire-in at
_ingest_turn_idsis a clean two-line addition with no other code paths touched. - Integration test in
test_ingest.pyexercises each category in one turn with a real sentence — solid evidence the filter doesn't drop everything. - 3 commits, all signed.
- Discretion grep on added lines: clean.
Not adding ready-to-merge until the ACK regex concern lands a decision and the rebase ships.
|
[release:review:noether:2026-05-11T19:47:55Z] |
|
[claim:review:faraday:2026-05-11T19:48:14Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
LGTM — approve.
Five categories cleanly factored: position-0 startswith for shell prefixes (cat 1) and XML tags (cat 3), single-codepoint glyph match (cat 2 U+23FA), and two regexes for the progress/ack shapes (cat 4/5). Regexes compiled once at module load as Final. The wire-in is a literal two lines after extract_sentences in _ingest_turn_ids and doesn't touch scan_repo — the surface boundary matches the issue (transcript-ingest, not file-scan).
Tests are thorough: 37 unit tests covering positive + negative per category, plus the multi-sentence integration test in test_ingest.py that exercises one sentence per noise class against a real sentence and asserts exactly one belief survives. The "git history shows..." negative case explicitly pins the position-0 requirement that distinguishes shell-prefix from prose. The cat-4-vs-cat-5 overlap case ("Polling for results." matches ACK regex) is documented in the test and is the spec-correct behaviour.
Discretion grep clean. 3 commits, all G-signed. The ab51498 docstring rephrase commit is explicitly for the discretion grep — good hygiene.
Aligns with locked PHILOSOPHY (#605). Pure deterministic regex / startswith gates, no embeddings or ML scoring. The pipeline-level concern (catch tool-output before it becomes a belief) is solved at the ingest surface rather than retrieved-then-filtered, which is the cheaper and more correct point.
One observation — false-positive surface on cat 5 ACK regex (No|Standing by|Ready|Nothing|Polling + up to 40 trailing chars). A legitimate decision-statement like "No to that approach." or "Ready to ship." would get filtered. In transcript context these are rare and the issue spec wants them gone, so this is correct for the surface it gates — just noting the trade-off so future tuning has a reference point. The cat-5 bare-keyword-no-period match ("Polling" alone → True) is more aggressive than the docstring's \.?$-anchor reads at first glance, but is intentional per the test comment.
Branch state: REBASE-NEEDED against current main (base 03c7d23, main has #677 ahead). #677 changed only retrieval.py + CHANGELOG.md — no overlap with this PR's files (ingest.py, noise_filter.py, tests/test_*). Clean FF expected after git rebase github/main.
Adding ready-to-merge; the merge-train bot will request rebase if needed.
|
[release:review:faraday:2026-05-11T19:49:36Z] |
|
merge-train: blocked branch is not fast-forward on The |
|
merge-train: blocked required check(s) failed: The |
|
[claim:review:Noether:2026-05-11T20:50:05Z] |
|
[claim:review:faraday:2026-05-11T20:50:28Z] |
|
[release:review:faraday:2026-05-11T20:50:33Z] |
|
[release:review:schwartzchild:2026-05-12T22:49:43Z] |
Docstring claimed `\\.*$` (zero-or-more dots) but the implementation uses `\\.?$` (optional single dot). Code is correct; docstring is the drift. One-character doc fix; no behavior change. Caught by CodeRabbit on PR #679.
6f23869 to
8cb81a4
Compare
|
Rebased onto current Soak streak is now 7 (2026-05-07 → 2026-05-13) per Per operator decision, shipping with Cat 5 ACK regex as-is (broad Removing |
|
merge-train: blocked timed out waiting for CI checks to complete after 10 minutes. Retrigger once checks have settled. The |
|
merge-train: blocked branch is not fast-forward on The |
…es (#675) Add is_transcript_noise(sentence) to noise_filter.py. Five categories: shell-command prefixes (cd /, git , gh , uv run, pytest, python ), U+23FA tool-call rendering glyph, pseudo-XML structural tags, single-word progress emits (^[A-Z][a-z]+ing\.$), and agent ack emits. Regexes compiled once at module load as module-level Finals. Unit tests cover each category with positive and negative cases including all specified edge cases.
Single-line filter in _ingest_turn_ids immediately after extract_sentences: sentences = [s for s in sentences if not is_transcript_noise(s)] Import added at top of ingest.py. Integration test in test_ingest.py verifies that a turn containing one noise sentence per category plus one real sentence produces exactly 1 derived belief id with the correct content.
Docstring claimed `\\.*$` (zero-or-more dots) but the implementation uses `\\.?$` (optional single dot). Code is correct; docstring is the drift. One-character doc fix; no behavior change. Caught by CodeRabbit on PR #679.
8cb81a4 to
147b484
Compare
|
merge-train: merged 147b484 → |
Closes #675.
What ships
In-tree transcript-noise filter to stop ingesting tool-output / agent-emit sentences as beliefs.
Surface
src/aelfrice/noise_filter.py— newis_transcript_noise(sentence: str) -> boolpredicate. Five pattern categories, each documented and tested:cd /,git,gh,uv run,pytest,python).⏺/ U+23FA).<worktree,<output-file,<task-,<summary>Background).^[A-Z][a-z]+ing\.$— matchesPolling.,Running., etc.).^(Yes|No|Standing by|Ready|Nothing|Polling)( .{0,40})?\.?$).Regexes compiled once at module load as
Finalconstants. Empty / whitespace-only inputs returnFalse(handled upstream byis_noise).src/aelfrice/ingest.py:_ingest_turn_ids— two-line filter inserted immediately aftersentences = extract_sentences(text):No other ingest path is touched.
scan_repokeeps using the existingis_noiseagainst a different content surface.Acceptance bullets (#675)
tests/test_noise_filter.pyadds 37 new tests across all five categories. Edge cases pinned: bare"Polling"matches ACK (period optional);"The git history shows..."does NOT match (not at start);"Polling for results."matches ACK, not progress.tests/test_ingest.py::test_ingest_turn_ids_filters_transcript_noise_and_keeps_real_sentenceconstructs a multi-sentence transcript turn with each noise class plus one real prose sentence; asserts exactly one belief id is returned and its content matches the real sentence.Out of scope
#Nliteral boost in BM25 — covered by sister issue feat(retrieval): #N literal boost in BM25 scoring for issue/PR-number prompts #677.Notes
Two atomic signed commits (predicate + tests, then wire-in + integration test). No DB writes, no schema changes, ~80 LOC including tests.
Summary by CodeRabbit
Bug Fixes
Tests