Skip to content

fix: wrap extraction transcript turns in speaker tags to stop assistant misattribution - #964

Open
gorkem2020 wants to merge 1 commit into
CortexReach:masterfrom
gorkem2020:fix/extraction-speaker-attribution
Open

fix: wrap extraction transcript turns in speaker tags to stop assistant misattribution#964
gorkem2020 wants to merge 1 commit into
CortexReach:masterfrom
gorkem2020:fix/extraction-speaker-attribution

Conversation

@gorkem2020

Copy link
Copy Markdown
Contributor

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

  • Each message is wrapped wholly in <user_message>/<assistant_message> blocks, built from the capture hook's role-tagged message-loop order, so every line has an unambiguous owner.
  • The extraction prompt becomes a {system, user} split: the transcript-format teaching and grounding rules ride the system half, the tagged transcript rides the user half. completeJson gains an optional per-call system prompt to carry it (the default generic system message is unchanged for all other callers).
  • Two prompt modes, matching captureAssistant: with false (default) assistant lines never enter the transcript and memories may only be grounded in <user_message> blocks; with true assistant blocks are attributable sources with explicit attribution rules ("attribute every memory to whoever actually said it").
  • Literal speaker tags typed INSIDE a message are neutralized with guillemets, so content can neither fake a block boundary nor defeat tag-boundary trimming.
  • extractMaxChars truncation 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.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 to texts. buildConversationTurnsForExtraction() still receives the original newTexts, and SmartExtractor prefers any nonempty conversationTurns over the flat conversationText. The actual extraction prompt therefore contains only remember 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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

All three items are addressed on the new head:

  1. Remember-this flow: the prepended prior message is now reconstructed as a tagged user turn (the tagged transcript carries the same final sequence as the flat text path), so the real extraction prompt contains both the referenced fact and the command. A new integration test asserts against the captured extraction prompt for exactly this two-step flow.
  2. Compression selection: the tagged transcript now mirrors the final extraction input via a single kept-set filter (a user turn survives only if its text survived session compression and the noise filter alike), so selector-dropped texts can no longer re-enter through the tags. Covered by an integration test where a compression-dropped filler must not reach the prompt.
  3. Truncation: a single turn longer than the budget is re-headed with the opener matching its closing tag, so the attribution invariant survives truncation; genuinely untagged input still passes through unchanged (existing behavior pinned by the prior test).

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.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch from f1a8f32 to f56499e Compare July 24, 2026 06:34

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@gorkem2020

gorkem2020 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

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 ConversationTurn objects rather than flat strings. Detection counts the new USER turns only (exactly one, an explicit remember command, prior history present), so an assistant acknowledgement in the same delta no longer masks the command, and the referenced prior turns are prepended with their original roles to both the flat extraction input and the tagged transcript. The prepend window runs from the nearest substantive user-authored prior turn to the end of the recents rather than taking the positionally last turn, which under captureAssistant=true could be the assistant's ack of the fact instead of the fact itself; a multi-block assistant reply cannot exhaust the walk, a prior remember command is never chosen as the referent (a repeated "remember this" reaches back to the fact), and in user-only capture it degenerates to exactly the previous single-text behavior. A window-filling assistant reply can no longer evict the fact either: when the bounded recents would otherwise contain no substantive user turn, the newest one is pinned at the front. The requested integration coverage runs the real prompt path with captureAssistant=true for the assistant-fact, ack-walk, multi-block, and window-eviction shapes.

Both-role reconciliation. The user-only Set filter is replaced by reconcileTurnsWithKeptTexts, so compression- or noise-dropped assistant text stays out of the tagged prompt and duplicate texts keep per-copy multiplicity. Our final pass upgraded the selection from value matching to positional: the compression and noise-filter selectors now report the indices they kept (CompressResult.keptIndices, filterNoiseByEmbeddingWithIndices), the call site threads them through, and the transcript selects turns by position, which keeps a byte-identical text uttered by both roles attributed to the copy that actually survived (value matching consumed occurrences front-to-back and could emit the survivor under the earlier turn's role whenever a selector kept the later duplicate, which the last-index auto-admit in compression does at the default budget). Ordered occurrence counting remains as the fallback whenever positional alignment is unavailable. Unit-pinned for the assistant-drop, duplicate-multiplicity, ordering, cross-role duplicate, three-copy middle-survivor, and misalignment-fallback cases.

Truncation. Rather than patching the opener overshoot in place, the rendered-string trim is replaced by a turn-layer bound: buildBoundedTranscript keeps whole turns from the end and tail-slices only the TEXT of the oldest turn that partially fits, tags intact. extractMaxChars is enforced as a strict absolute ceiling on the transcript, matching the previous flat-text path's slice(-maxChars) contract with no overshoot of any size. Our review pass found the string-surgery approach had worse problems than the small constant: a turn straddling the cut was discarded whole whenever any shorter turn followed (reducing a remember-this transcript to just the bare command), and a cut landing inside a closing tag could re-head with the wrong speaker and emit an unterminated block. The turn-layer bound removes that failure class structurally. The helper also reports the untruncated render length, so the over-budget path renders the turns once rather than twice, and a debug line reports whenever bounding drops content. Regressions pin the strict cap across many turns, straddle survival, single over-limit turns, whole-turn drops on tiny remainders, and block well-formedness under arbitrary budgets. One consequence we are noting openly: the tag markup now lives inside the ceiling, so a many-turn history spends a few percent of the budget on structure that the flat path spent on content; we read the knob as a bound on what is sent, matching your reading of the overshoot, and judged a strict ceiling safer than any variant that can exceed it.

Lifecycle of the new recents window (from the same passes). The window is cleared only on true session terminals (new, reset, deleted, shutdown, restart). The host also rolls sessions mid-conversation for idle and daily budgets and for compaction: those emissions keep the same session key and announce a successor id, and clearing there would drop the referent at exactly the moment it left the visible transcript, so they are preserved. An unrecognized or absent reason is preserved only when a successor is announced, and clears otherwise. The pending-ingress queue is deliberately left alone by this teardown, since it is conversation-scoped and shared by every agent bound to the conversation, and the rollover-triggering inbound is already queued when the boundary fires; it stays bounded by its own per-conversation cap. The shared "unknown" session-key fallback never receives a prepend, closing a cross-session leak for unattributable sessions.

Speaker-tag normalization. neutralizeSpeakerTagSpoof is now a single forward scan rather than a regular expression. Two rounds of quantified matching kept going superlinear on adversarial input (first the whitespace run around the optional slash, then the attribute arm), and a bounded whitespace budget let longer padding through unnormalized. The scan never revisits a character, accepts unlimited padding, and covers case variants plus attribute-bearing and self-closing forms. Zero-width format characters (U+200B through U+200D, U+2060) are treated as padding as well, since they are invisible and absent from the JavaScript whitespace class. Visibly malformed near-tags are left untouched on purpose: they do not read as tags, and matching arbitrary content before the name would mangle ordinary prose. Scaling tests hold the helper to a tight millisecond ceiling on pathological inputs.

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 json block, going superlinear on fence-dense messages, and the key lookaheads could strip a keyless block whenever the envelope keys appeared anywhere later in the text. Both patterns are replaced by a single forward fence scan with block-local checks (balanced-object body, envelope keys inside the block, label immediately before it), held to a millisecond ceiling on fence-dense input by a scaling test. A turn that strips to nothing is dropped rather than rendered as a contentless speaker block, and a delta that strips entirely to envelope metadata skips the extraction call. The recents window treats a multi-block user message as one contiguous run at both the eviction pin and the remember walk, so a fact block can no longer be evicted while its trailer block survives. The window is also agent-scoped now: hosts that hand several agents the same literal session key (session.scope global) no longer share one remember window across agents. Teardown deliberately stays session-scoped rather than agent-scoped: a session_end context cannot reliably name the agent that wrote a window (the host rebuilds its agentId from the session key and resolves the default agent on keys with no agent segment), and a terminal boundary ends the session for every agent riding the key, so a terminal sweeps every window under that key. Referent anchoring and the eviction pin additionally require a turn to still carry content after envelope stripping, so a contentless envelope turn cannot swallow the referent, while run-extension walks stay role-based so an envelope block inside a multi-block message does not break the contiguous run. The tag-padding walk accepts the full invisible format-character class (bidi marks and overrides included) rather than four enumerated codepoints, envelope-block brace counting skips JSON string literals so an unpaired brace in a chat string cannot shield a block from stripping, and a transcript emptied by bounding skips the extraction call.

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 < as a comparison operator directly against the literal identifier user_message or assistant_message, with a later > in the same message, is rewritten to bracket look-alikes. The rewrite is fail-safe (two visible characters, never a structural change), and narrowing the padded-form acceptance enough to leave it alone would reopen the padded-spoof surface the scan exists to close, so we chose the cosmetic artifact over the exposure.

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 extractMaxChars (fair split: whichever side needs less than half gets what it needs, the other takes the remainder), and only the run itself is protected, since protecting the whole prepended window would spend that share on the replies that follow the fact and evict it anyway. A ceiling too small to carry the referent is reported rather than silently dropped. Alongside it: the request's system and user halves are now pinned by an assertion, the session-compressor contract test is registered in both the npm chain and the CI manifest (it was executed by neither), and a delta that skips the model no longer charges the hourly extraction quota.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Head a63be7b closes the separate-message conflation with the identity model you specified.

Message identity. Every ConversationTurn now carries a messageId stamped in the agent_end message loop: all text blocks of one source message share it, and distinct messages never do. The counter is monotonic across capture calls, since the recents window mixes turns from several calls and per-call indices could collide. Pending-ingress replay turns, which have no correlated source message, are synthesized with fresh ids each, so they can never read as blocks of one message.

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 remember this at the default threshold, asserting the immediately referenced fact is prepended and the unrelated earlier messages never enter extraction (red before the fix). A companion case pins the feature the walk exists for: one user message with two content blocks still gets both blocks prepended. A unit case pins the fallback-lane contract: distinct replayed texts never share an identity.

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.

@rwmjhb

rwmjhb commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the follow-up. Current head a63be7b has merge conflicts with the latest base, so I cannot verify the new fix against the current code yet. Please rebase onto the latest master, resolve the conflicts, rerun the focused and full suites, and push the resolved head for re-review.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch 2 times, most recently from 7b15bcf to 850904f Compare July 28, 2026 11:18

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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 rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@rwmjhb

rwmjhb commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the update. After the recent merges, current head 7c519d4 now has merge conflicts with the latest master, so the approved revision cannot be merged as-is. Please rebase onto the latest master, resolve the conflicts, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch from 9441658 to d2e8772 Compare August 1, 2026 13:20

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@rwmjhb

rwmjhb commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the update. After #934 merged, current head d2e8772 now has merge conflicts with the latest master, so the approved revision cannot be merged as reviewed. Please rebase onto the latest master, resolve the conflicts while preserving the reviewed tagged-transcript and message-identity behavior, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch from d2e8772 to c1c9db5 Compare August 2, 2026 09:51

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Addressed in e0a0cca. Confirmed: the flush hook runs for every session_end reason, so a continuation rollover with queued or deferred text reached the unconditional recent-turns delete and erased the window the priority-10 sweep had deliberately preserved.

The lifecycle classification is now a single shared helper (isTerminalSessionBoundary): the priority-10 sweep uses it unchanged, and the flush invocation passes its verdict alongside __autoCaptureTerminalFlush as __autoCaptureTerminalBoundary. Inside the capture hook, the flush semantics (consume pending and deferred input) stay keyed to the flush flag, while the recent-turns delete is keyed to the boundary flag: only true terminal boundaries wipe the window, and a continuation flush instead folds the flushed ingress into it through the existing window-update branch (a flush without the boundary flag is treated as terminal, so direct invocations keep their old behavior).

Regression added as requested: queued ingress plus an idle session_end with a successor, all handler promises awaited; it asserts the flush consumed the queued text, and that the subsequent remember command still extracts with its referent (the flushed inbound, the newest substantive user turn) in the prompt. Red before the fix (the remember turn had no referent and did not extract), green after. Typecheck, the focused file, and the full suite pass.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Addressed in 9d261f4, taking the preferred shape: deferred-flush state now stores role-bearing ConversationTurn[] (role, message id, occurrence order) instead of flat strings. The terminal flush dedups by text with first occurrence winning (mirroring the previous string-set union), synthesizes user turns only for pending ingress (user-authored by construction), and feeds the stored turns to the transcript directly, bypassing the no-correlation fallback that re-tagged everything as user turns. Every other touch point keeps the turn shape: the below-threshold deposit and barren deferral select the role-bearing turns for the deposited texts, the failed-run restore hands back turns, and consumed-text retirement filters on turn text.

Regression added as requested: captureAssistant=true, a below-threshold exchange with a multi-paragraph assistant response, then session_end with all handler promises awaited. It asserts the flushed extraction keeps the assistant answer inside assistant_message tags, that no assistant-authored content appears inside user_message tags, and that the user question keeps its tag. Red before the fix (the assistant answer arrived as a user turn), green after. Typecheck, the focused suites, and the full suite pass.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@rwmjhb

rwmjhb commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch 2 times, most recently from 9a17ece to 2c60ed4 Compare August 3, 2026 13:52

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@rwmjhb

rwmjhb commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (post #944 and #986), registration union resolved, full gates green. Mergeable again.

@gorkem2020
gorkem2020 force-pushed the fix/extraction-speaker-attribution branch from 2c60ed4 to 0474fb1 Compare August 4, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants