fix: wrap extraction transcript turns in speaker tags to stop assistant misattribution - #964
Conversation
rwmjhb
left a comment
There was a problem hiding this comment.
The speaker-tag direction is sound, and both the full suite and a clean source/dist build pass. One user-facing regression blocks merge:
- For the supported two-step flow where a user states a fact and then says
remember this, the code prepends the prior message only totexts.buildConversationTurnsForExtraction()still receives the originalnewTexts, andSmartExtractorprefers any nonemptyconversationTurnsover the flatconversationText. The actual extraction prompt therefore contains onlyremember this, not the fact it references; if no candidate is produced, the fallback is skipped as well.
Please build the tagged turns from the final text sequence (or explicitly reconstruct the prepended prior turn) and add an integration assertion against the real extraction prompt for this flow.
Two follow-ups are worth covering in the same area: session compression currently changes texts after the turn list is built, so the tagged prompt can bypass the compressor selection; and a single turn longer than extractMaxChars returns a suffix with a closing tag but no opening speaker tag. Neither should be allowed to undermine the new attribution invariant.
|
All three items are addressed on the new head:
All three regressions fail on the previous head and pass on this one; tsc, the touched suites, and the full chain are green, and dist is rebuilt at the head. |
f1a8f32 to
f56499e
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head f56499e. The new tagged transcript fixes the default user-only attribution path, and the focused/full suites pass, but the previous remember-this and selection fixes remain incomplete when the supported captureAssistant=true mode is enabled.
The remember-this reconstruction still loses role information. A normal new delta can contain both the user command and the assistant acknowledgement, so the existing texts.length === 1 prepend branch does not run. When it does run, priorRecentTexts contains mixed-role strings and the recovered prior entry is unconditionally rebuilt as { role: "user" }. A prior assistant statement can therefore be presented to extraction as user-authored, recreating the attribution failure this PR is intended to prevent. Please retain recent ConversationTurn objects, detect the remember command among new user turns independently of accompanying assistant turns, and prepend the referenced turn with its original role. Add an actual-prompt integration test with captureAssistant=true.
The final turn reconciliation also retains every assistant turn via turn.role !== "user" || keptUserTexts.has(turn.text). Assistant text removed by compression or noise filtering is therefore reintroduced into the tagged prompt and can evict selected content during tail trimming. Please reconcile selection for both roles using stable turn identity or ordered occurrence counts, including duplicate-text coverage.
The fixed opener added after truncation also exceeds extractMaxChars by a small constant; that can be handled alongside the boundary tests. Requesting changes for the two role/selection paths above.
|
All three addressed, and our own adversarial pre-push passes surfaced adjacent defects on the same paths, fixed in this round as well. Role-preserving remember prepend. The recents singleton now stores Both-role reconciliation. The user-only Set filter is replaced by Truncation. Rather than patching the opener overshoot in place, the rendered-string trim is replaced by a turn-layer bound: Lifecycle of the new recents window (from the same passes). The window is cleared only on true session terminals ( Speaker-tag normalization. Envelope stripping (final pass). With stripping now running per turn ahead of the turn-layer bound, the stripper's own cost profile mattered more than before: its labeled-section and keyed-block regexes rescanned toward end-of-input for every fenced A second consequence we note openly alongside the markup-inside-the-ceiling one: extractMaxChars is charged for speaker-tag markup while session compression selects content by raw character count, so under compression a selected oldest text can still be dropped by the rendered-transcript bound. We read both as the documented price of strict-ceiling attribution; operators who compress aggressively can raise extractMaxChars to compensate. One neutralizer artifact we are noting openly rather than changing: prose that uses All new tests were written first and confirmed failing against the prior head; full suite green. Follow-up in 45879d3, one defect our own post-push pass found on this head. The prepend puts the referent at the OLDEST end of the transcript while the budget walk keeps turns newest-first, so at the default ceiling a verbose reply between the fact and the command could evict the fact, and the prepend was still logged as delivered. The referent run now carries a protected share of |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 45879d3. The latest changes give the prepended referent transcript budget and register the compressor contract coverage; the focused and full suites pass. One merge-blocking remember-path issue remains.
With captureAssistant=false, assistant turns are omitted and index.ts identifies a multi-block referent only by walking backward across adjacent user turns. Distinct user messages therefore become indistinguishable from blocks of one message. At the default four-message threshold, three earlier user messages can remain unextracted; when the fourth is remember this, the walk reaches the start of the window and submits all three for their first extraction. A direct probe prepended both an older unrelated preference and the immediately preceding fact, so deduplication does not prevent unintended memories from being created.
Please retain a stable message/group identity on each ConversationTurn and extend a referent only across blocks sharing that identity; otherwise use only the immediately referenced message. Add a production prompt-path regression with multiple distinct prior user messages followed by remember this, asserting that unrelated messages never enter extraction.
The pending-ingress omission of assistant turns and assistant-referent budget targeting are worthwhile follow-ups, but the separate-message conflation is the blocker here.
|
Head Message identity. Every Both run-extension walks are id-scoped. The remember-referent walk and the window-pinning walk now extend a run only across adjacent user turns sharing the referent's id, and a turn without an id never extends a run, which is the fall-back-to-the-immediately-referenced-message behavior you asked for. Envelope-only blocks inside one message share that message's id, so a contentless block still does not break a genuine contiguous run. Regressions. A production prompt-path case reproduces your probe: three distinct prior user messages followed by The pending-ingress assistant-turn omission and the assistant-referent budget targeting remain as follow-ups per your note. The branch currently shows a conflict against master from this morning's unrelated merge; we rebase in our normal cycle, and this fix is independent of that motion. Full suite, typecheck, and a fresh dist are green on the new head. |
|
Thanks for the follow-up. Current head |
7b15bcf to
850904f
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 850904f. The source-message identity change addresses the prior user-only referent conflation, but the rebased head removes two package scripts still required by CI.
package.json no longer defines test:cli-smoke or test:core-regression, while .github/workflows/ci.yml still runs both commands. Executing either exact command on this head returns Missing script, so the cli-smoke and core-regression required jobs fail before running any tests. Please restore both scripts using the current scripts/run-ci-tests.mjs groups while retaining the newly registered transcript/compressor suites, then run the exact two npm commands.
The message_received ingress path still omits assistant turns under captureAssistant=true, and assistant-authored remember referents are not protected under transcript pressure; those remain worthwhile follow-ups. The missing CI entry points are the merge blocker on this head.
|
Restored in 7c519d4: the rebase dropped the test:cli-smoke and test:core-regression script keys (they share one physical line with the main test chain, and the conflict resolution reconstructed that line without its trailing keys). Both are back on the current run-ci-tests.mjs groups, the newly registered transcript and compressor suites stay in the chain, and the two exact npm commands were executed on this head and pass. The captureAssistant ingress omission and assistant-authored referent protection are tracked as follow-ups on our side. |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 7c519d4. The prior merge blocker is fixed: test:cli-smoke and test:core-regression are restored and both exact commands, targeted tests, the full suite, and repository CI pass. The role-tagged transcript/prompt changes address the core assistant-to-user misattribution issue, and I found no new HIGH/CRITICAL regression attributable to this head. Approving.
Known follow-ups remain: the pre-existing message_received pending-ingress path can still omit assistant turns with captureAssistant=true; assistant-authored remember referents can be displaced under the default transcript budget; and unknown session_end reason values with successor IDs currently retain the old remember window.
|
Thanks for the update. After the recent merges, current head |
|
Rebased onto current master (post #952) as requested. The review-round history is squashed into a single commit for a clean re-verification; the extraction-policy and grounding features from master are composed with the tagged-transcript changes (policy resolution and the grounding rejudge now feed the same bounded transcript the extractor sees). Focused and full suites green, dist rebuilt. |
9441658 to
d2e8772
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head d2e8772 after the rebase. The tagged-transcript attribution fix remains intact, focused coverage and the full suite pass, CI is green, and the extraction-policy/grounding integration introduces no merge-blocking regression. Known follow-ups remain around captureAssistant pending ingress, assistant-authored remember-reference budgeting, lifecycle reason typing, and further spoof-neutralizer hardening.
|
Thanks for the update. After #934 merged, current head |
|
Rebased onto current master (post #934). This one needed a real composition, documented here for review: The deferral machinery merged in #934 rolls the seen cursor back on below-threshold turns so a later turn re-includes the deferred texts, while this branch's remember-referent design requires that a distinct older message never gets its first extraction smuggled in by an unrelated remember command (pinned by the existing sweep test here). Both now hold: the cursor rollback stays exactly as merged, and when a delta ends in an explicit remember command, texts re-swept by the rollback (precisely those already sitting in the deferred-flush bucket) are dropped from that run and stay deposited for their own consumer, a later plain turn or the terminal flush. Messages genuinely delivered alongside the command are not in the bucket and remain in the delta, so multi-message rounds ending in a remember keep extracting whole. The snapshot-identity guard also reads this branch's tagged recent-turns window now. All of #934's deferral, settled-consume, and gating tests pass unchanged alongside this branch's full watermark/referent suite. Typecheck, build (dist recommitted), manifest verifier, and the full suite (51 files, 0 failures) are green. Mergeable again. |
d2e8772 to
c1c9db5
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head c1c9db5. The tagged-transcript attribution fix remains covered and the focused/full suites plus GitHub CI are green, but the #934 composition introduces one lifecycle regression that blocks this head.
The priority-10 session_end hook intentionally returns for idle, daily, compaction, or successor rollovers so autoCaptureRecentTurns survives across the continuation boundary. A separate session_end hook still runs for every reason and invokes agentEndAutoCaptureHook with __autoCaptureTerminalFlush: true. When pending or deferred text exists, that path reaches the unconditional autoCaptureRecentTurns.delete(...) in the terminal-flush branch. Queued rollover ingress is explicitly expected by the surrounding comments, so an awaited rollover can erase the preserved referent and boundary state; the next remember this then has no intended context.
Please propagate the lifecycle classification into the flush path. Continuation rollovers may flush pending/deferred input, but they must retain or rebuild the recent-turn window; only true terminal boundaries should delete it. Add a regression that queues rollover ingress, awaits all session_end handlers, and verifies the subsequent remember command still receives its referent.
Requesting changes.
|
Addressed in e0a0cca. Confirmed: the flush hook runs for every The lifecycle classification is now a single shared helper ( Regression added as requested: queued ingress plus an idle |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head e0a0cca. The lifecycle classification fix closes the reported rollover-window deletion, and the focused checks plus GitHub CI are green. One attribution blocker remains in the terminal deferred-flush path.
autoCaptureDeferredFlushTexts still stores deferred work as plain string[]. With captureAssistant=true, a below-threshold user/assistant exchange is flattened into that queue. On session_end, the flush invokes agentEndAutoCaptureHook with messages: []; buildConversationTurnsForExtraction() cannot correlate those strings to original turns and takes its fallback, assigning every item role user. Because terminal flush bypasses the normal message threshold, assistant-authored content reaches the extraction prompt inside user_message tags. A direct reproduction confirms this.
Please preserve deferred state as ConversationTurn[] (role, messageId, occurrence identity), or otherwise reconstruct the terminal flush from role-bearing turns rather than flat strings. Add an end-to-end regression with captureAssistant=true, a below-threshold multi-paragraph assistant response, and session_end.
Requesting changes.
|
Addressed in 9d261f4, taking the preferred shape: deferred-flush state now stores role-bearing Regression added as requested: |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 9d261f4. The deferred terminal-flush state now preserves ConversationTurn role and occurrence identity, and the new captureAssistant=true regression closes the assistant-to-user misattribution blocker from the previous round. Targeted tests, the full suite, direct type/dist verification, and GitHub CI are green.
The remaining sessionId-only cleanup, duplicate-text correlation, tight-budget remember-referent, and empty-input retry cases are non-blocking follow-ups.
Approved.
|
PR #941 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes. |
|
Rebased onto current master (post #941); the round history is squashed into one commit. Real overlaps composed: the prompt builders keep master's single-sourced stage identities with this branch's tagged-transcript machinery on top, the LLM client keeps master's shared default system prompt, and the mock detectors adopt master's identity-string form (shrinking this diff). Registration files re-unioned, typecheck and the full suite pass, dist rebuilt in-commit. |
9a17ece to
2c60ed4
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 2c60ed4. The conflict resolution preserves the previously approved role-tagged extraction and deferred-flush behavior; targeted tests, the full suite, and GitHub CI are green.
The remaining session-lifecycle ordering, empty-input classification, and text-only dedup cases are non-blocking follow-ups.
Approved.
|
PR #944 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes. |
…ant misattribution
2c60ed4 to
0474fb1
Compare
Problem
The extraction transcript renders turns as
User:/Assistant:line prefixes, so only the FIRST line of a message carries a speaker marker. A multi-paragraph assistant reply sheds its marker after the first paragraph, and the extractor attributes assistant-authored plans, preferences, and self-descriptions to the user and stores them as user memories. Live example that motivated this: an assistant's multi-paragraph explanation of how memory layers work came back as "User believes manual notes are only for rare big items" style rows.Change
<user_message>/<assistant_message>blocks, built from the capture hook's role-tagged message-loop order, so every line has an unambiguous owner.{system, user}split: the transcript-format teaching and grounding rules ride the system half, the tagged transcript rides the user half.completeJsongains an optional per-call system prompt to carry it (the default generic system message is unchanged for all other callers).captureAssistant: withfalse(default) assistant lines never enter the transcript and memories may only be grounded in<user_message>blocks; withtrueassistant blocks are attributable sources with explicit attribution rules ("attribute every memory to whoever actually said it").extractMaxCharstruncation snaps to a tag boundary, so a sliced transcript never opens with headless text.Tests
New
test/extraction-transcript-speaker-tags.test.mjs(16 tests): whole-message wrapping, multi-paragraph containment, chronology, spoof neutralization, boundary trimming, turn assembly (watermark tail-slice, mixed-role alignment, pair-aware skip, ingress replay fallback), and prompt teaching in both modes. Existing extraction suites updated for the new prompt shape; registered in the test chain and CI manifest.