docs(ingest): spec for speaker-gate + sentiment-routing + SVO floor (#785) - #786
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR adds a feature specification document ( ChangesIngest speaker-gate specification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
robotrocketscience
added
author-mondragon
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/feature-ingest-speaker-gate.md`:
- Around line 19-31: The fenced code block that begins with the lines "agent
narrates progress" is missing a language tag which triggers MD040; update the
opening fence from ``` to ```text (or another appropriate language like ```text
or ```none) so the block is explicitly tagged, e.g., change the opening ``` to
```text for the block containing "agent narrates progress" through "agent sees
its own old narration as 'context' and continues the pattern".
- Around line 151-157: Update the acceptance checklist entry that reads "All six
unit tests in §1, §2, §3 pass." to reflect the actual number of tests defined in
the spec (change to "All eight unit tests in §1, §2, §3 pass."), ensuring the
acceptance criteria matches the enumerated tests; no other changes to the
checklist items are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7d6a726-5012-493b-8343-f52844f191a5
📒 Files selected for processing (1)
docs/feature-ingest-speaker-gate.md
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
[claim:review:pascal:2026-05-14T15:19:35Z] |
|
LGTM — well-structured spec, ready to merge after a rebase. Verification:
Substantive notes:
Nit (non-blocking): the implementation PR (separate, per the body) is the load-bearing one. This spec PR establishes the contract reviewers will measure against — once it lands, the implementation PR should reference Blocker before merge-train fires:
Rebasing + force-pushing on mondragon's behalf, then adding |
…785) Three coordinated additions across transcript_logger.py, ingest.py, and hook.py that close the narration-bloat feedback loop in belief ingest. Subtractive on the ingest path; no retrieval/scoring change. §1 skips assistant-role messages for belief creation. §2 routes sentiment-matched UPS text to feedback_history only (single source of truth at the hook). §3 adds MIN_BELIEF_CONTENT_CHARS=80 with edge-anchor demotion for sub-floor candidates. Spec only; implementation follows in a separate PR closing #785.
af13734 to
d5c8434
Compare
robotrocketscience
added
ready-to-merge
|
[release:review:pascal:2026-05-14T15:21:07Z] |
|
[claim:review:curie:2026-05-14T15:21:15Z] |
|
merge-train: blocked 1 review thread(s) are unresolved on these files: docs/feature-ingest-speaker-gate.md. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
|
Reviewed and pushed a small follow-up commit (
Both CR threads resolved. Verification:
Spec itself: well-structured, three subtractive changes are independent and individually testable, determinism (#605) preserved, prereqs (#606, #290) shipped. Ready for the implementation PR that closes #785. Re-adding |
robotrocketscience
added
ready-to-merge
|
[release:review:curie:2026-05-14T15:24:40Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 8de2f86 → |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/feature-ingest-speaker-gate.md (2)
19-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tag to fenced code block.
The fenced code block is missing a language identifier, triggering MD040 linting.
📝 Suggested fix
-``` +```text agent narrates progress ↓🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/feature-ingest-speaker-gate.md` around lines 19 - 31, The fenced code block in the markdown snippet (the block that starts with ``` and contains the narrative lines like "agent narrates progress" and arrows) lacks a language tag which triggers MD040; update the opening fence from ``` to ```text so the block is explicitly marked as plain text; ensure any other similar fences in docs/feature-ingest-speaker-gate.md are also tagged the same way to satisfy the linter.
156-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect test count in acceptance criterion.
Line 156 states "All six unit tests" but the specification defines eight tests: 2 in §1 (lines 65-66), 3 in §2 (lines 96-98), and 3 in §3 (lines 130-132).
📝 Suggested fix
-- [ ] All six unit tests in §1, §2, §3 pass. +- [ ] All eight unit tests in §1, §2, §3 pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/feature-ingest-speaker-gate.md` at line 156, The acceptance criterion incorrectly states "All six unit tests in §1, §2, §3 pass." — update that checklist item text to reflect the actual total of eight tests (2 in §1, 3 in §2, 3 in §3) so it reads something like "All eight unit tests in §1, §2, §3 pass."; locate and edit the checklist line containing "All six unit tests in §1, §2, §3 pass." in docs/feature-ingest-speaker-gate.md (the checklist item string) to correct the test count.
🧹 Nitpick comments (1)
docs/feature-ingest-speaker-gate.md (1)
104-117: ⚡ Quick winDocument edge schema prerequisite for
anchor_textfield.The contract references attaching sub-floor clauses as
anchor_texton edges (line 115), but the specification doesn't note whether the edge schema currently supports this field or if it requires creation.📋 Suggestion
Consider adding a note to either the §3 header or the "Substrate prereqs" section (line 5) clarifying:
**Schema prerequisite:** Edge records must support an `anchor_text` text field to store demoted sub-floor clauses. If not present, add during implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/feature-ingest-speaker-gate.md` around lines 104 - 117, Add a short schema prerequisite noting that edge records must include an anchor_text text field to hold demoted sub‑floor clauses referenced by MIN_BELIEF_CONTENT_CHARS; update the §3 header or the "Substrate prereqs" section to state that if the current edge schema lacks anchor_text it must be added during implementation and define its type/constraints (e.g., text, nullable, max length) so consumers know how to store the attached clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/feature-ingest-speaker-gate.md`:
- Around line 72-88: Add an explicit "Interface contract" subsection that
defines the archived-message marking and how ingest reads it: when the sentiment
classifier in hook.py matches a UserPromptSubmit, hook.py must set
archived_message.retention_class = "feedback_only" (or explicitly name an
alternative field if not reusing retention_class); ingest.py must check
archived_message.retention_class and, if it equals "feedback_only", skip belief
creation but still apply the existing feedback_history bump logic; include the
exact field name and allowed value(s) so hook.py and ingest.py can be
implemented independently.
---
Duplicate comments:
In `@docs/feature-ingest-speaker-gate.md`:
- Around line 19-31: The fenced code block in the markdown snippet (the block
that starts with ``` and contains the narrative lines like "agent narrates
progress" and arrows) lacks a language tag which triggers MD040; update the
opening fence from ``` to ```text so the block is explicitly marked as plain
text; ensure any other similar fences in docs/feature-ingest-speaker-gate.md are
also tagged the same way to satisfy the linter.
- Line 156: The acceptance criterion incorrectly states "All six unit tests in
§1, §2, §3 pass." — update that checklist item text to reflect the actual total
of eight tests (2 in §1, 3 in §2, 3 in §3) so it reads something like "All eight
unit tests in §1, §2, §3 pass."; locate and edit the checklist line containing
"All six unit tests in §1, §2, §3 pass." in docs/feature-ingest-speaker-gate.md
(the checklist item string) to correct the test count.
---
Nitpick comments:
In `@docs/feature-ingest-speaker-gate.md`:
- Around line 104-117: Add a short schema prerequisite noting that edge records
must include an anchor_text text field to hold demoted sub‑floor clauses
referenced by MIN_BELIEF_CONTENT_CHARS; update the §3 header or the "Substrate
prereqs" section to state that if the current edge schema lacks anchor_text it
must be added during implementation and define its type/constraints (e.g., text,
nullable, max length) so consumers know how to store the attached clause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb670d62-b10c-4c86-80da-426422148b8f
📒 Files selected for processing (1)
docs/feature-ingest-speaker-gate.md
| ### Contract | ||
|
|
||
| When a UserPromptSubmit's text matches a sentiment pattern (per the #606 classifier) *and* the same text would otherwise become a belief via the post-compaction ingest path: | ||
|
|
||
| - The sentiment match continues to bump `feedback_history` on prior beliefs (existing #606 behavior, unchanged). | ||
| - Belief creation is suppressed for that message text. | ||
|
|
||
| ### Implementation choice — single source of truth | ||
|
|
||
| Two structural options were considered: | ||
|
|
||
| | Option | Description | Trade-off | | ||
| |---|---|---| | ||
| | **A. Sentiment-first (recommended)** | Sentiment classifier runs before ingest archives the user message. On match, mark the archived entry as feedback-only. Ingest reads the mark and skips belief creation. | Classifier stays authoritative in one place. | | ||
| | B. Ingest-side mirror | Re-run the sentiment classifier inside `ingest.py` on each user-role message and skip belief creation on match. | Simpler wiring, but duplicates the classifier. | | ||
|
|
||
| **Adopt Option A.** A single source of truth on sentiment classification keeps classifier-update churn contained to the hook surface. |
There was a problem hiding this comment.
Specify the marking mechanism for sentiment-matched text.
The contract states that the hook should "mark the archived entry as feedback-only" and that "Ingest reads the mark," but the specification does not define:
- Which field or attribute carries this mark in the archived message structure
- What values distinguish feedback-only from normal belief candidates
- Whether this reuses the
retention_classcolumn from prereq#290or introduces a new field
For a specification document, the interface contract between hook.py and ingest.py must be explicit enough for independent implementation of both sides.
📋 Recommendation
Add an "Interface contract" subsection to §2 that specifies:
### Interface contract
When the sentiment classifier matches, `hook.py` shall set the archived message field:
- `retention_class = "feedback_only"` (or define alternative field if not using retention_class)
When `ingest.py` processes archived messages, it shall:
- Check the retention_class (or specified field)
- If marked feedback_only, skip belief creation while preserving feedback_history bump logic
Or specify the actual field and values you intend to use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/feature-ingest-speaker-gate.md` around lines 72 - 88, Add an explicit
"Interface contract" subsection that defines the archived-message marking and
how ingest reads it: when the sentiment classifier in hook.py matches a
UserPromptSubmit, hook.py must set archived_message.retention_class =
"feedback_only" (or explicitly name an alternative field if not reusing
retention_class); ingest.py must check archived_message.retention_class and, if
it equals "feedback_only", skip belief creation but still apply the existing
feedback_history bump logic; include the exact field name and allowed value(s)
so hook.py and ingest.py can be implemented independently.
1 participant
Summary
Spec for #785 — closes the narration-bloat feedback loop in belief ingest with three coordinated subtractive changes:
transcript_logger.py) — skiprole == "assistant"messages for belief creation; edge construction unchanged.feedback_historyrouting (hook.py↔ ingest) — single source of truth at the hook; ingest reads a feedback-only mark and skips belief creation.ingest.py) —MIN_BELIEF_CONTENT_CHARS = 80; sub-floor candidates demote to edgeanchor_textor are rejected.Determinism (#605) preserved. No retrieval-side or scoring-side change. No back-purge of existing rows (deferred to a separate stratum-aware cleanup campaign once these gates land).
Test plan
Tracks #785.
Summary by CodeRabbit