fix: retain the rolling pair window across successful extractions - #962
Closed
gorkem2020 wants to merge 13 commits into
Closed
fix: retain the rolling pair window across successful extractions#962gorkem2020 wants to merge 13 commits into
gorkem2020 wants to merge 13 commits into
Conversation
captureAssistant: false filters assistant turns out of extraction input entirely, protecting against misattribution but starving the extractor of disambiguating context (a user reply like "yes exactly, that one" extracts poorly with no idea what "that" refers to). Add a middle mode, captureAssistant: "context": assistant turns are routed into the extraction prompt as clearly marked, non-extractable context, while remaining fully capture-ineligible. Chosen as a union literal on the existing boolean field (captureAssistant?: boolean | "context") rather than a sibling flag, since it composes naturally with the current true/false normalization (cfg.captureAssistant === "context" ? "context" : cfg.captureAssistant === true) and needed no new config surface. true and false keep byte-identical semantics, verified directly against parsePluginConfig. Assistant-context texts are collected into their own array during the auto-capture message loop (never merged into eligibleTexts), rolled across turns in a new autoCaptureRecentAssistantTexts map (mirroring the existing autoCaptureRecentTexts pattern), and cleared on successful extraction. They never touch autoCaptureSeenTextCount, so eligibility counting and the auto-capture watermark are provably unaffected — a hook-level regression test proves cumulative counting stays 1-then-2 across two user+assistant turns, not 2-then-4. SmartExtractor.extractAndPersist() gains an assistantContextTexts option threaded down to the prompt assembly, which appends the marked lines under an "Assistant Context" heading. The extraction prompt's criteria gained a matching rule: candidates may only assert facts sourced from user-authored lines, never from an assistant-marked line alone. This is prompt-level guidance only (no deterministic storage-side gate exists for this, unlike the grounding work) — coverage here is structural (the instruction text is present) plus the prompt-assembly behavior (marked lines appear only when assistantContextTexts is non-empty). Note for integration: fix/autocapture-watermark-reset (67ef600, not yet merged) touches the same post-success reset block this change touches (the line right after autoCaptureSeenTextCount.set(sessionKey, ...)). Resolving that merge needs to keep both: the flow-aware reset value and autoCaptureRecentAssistantTexts.delete(sessionKey).
…ario runAssistantContextModeScenario's turn 2 intentionally sends only that turn's delta messages, not full accumulated history. On this branch's current counter model (issue CortexReach#417 Fix CortexReach#4/CortexReach#5), that is correct: the watermark only resets on a successful extraction and never rolls back on a skip, so per-turn deltas already accumulate correctly. This is documentation only, no fixture behavior changed. A prior assembly integration test surfaced that a *rolled-back* counter model (issue CortexReach#417 Fix CortexReach#9) requires this fixture to send full accumulated history instead, so the note pins down which fixture style belongs here and why, to prevent that failure mode from resurfacing silently if this branch is later combined with Fix CortexReach#9's rollback logic.
configSchema.properties.captureAssistant was still the pre-feature plain boolean, even though this branch's own captureAssistant: "context" mode (and its runtime normalization in index.ts) has existed since this feature landed. The oneOf fix had only ever been applied as a post-assembly commit on the old deploy branch, never ported back here. Pinned exact shape from the live fleet manifest. Red-first regression test added to plugin-manifest-regression.mjs (none existed for this key). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the index.ts conflict from issue CortexReach#417 Fix CortexReach#9's flow-aware watermark reset (already on master) landing alongside this branch's own autoCaptureRecentAssistantTexts.delete(sessionKey) cleanup -- keeps both, per the integration note left in the original captureAssistant commit message.
…onversation-turns transcript The extraction prompt's chat history was two disjoint blocks: user texts flattened under "## Recent Conversation", then (in captureAssistant: "context" mode) a separate "## Assistant Context (for disambiguation only)" block appended after, each assistant line individually labeled "Assistant (context only -- do not extract from these lines)". Replace both with one "## Recent conversation turns" section: a header, a single description sentence carrying the "extract from user turns only" rule, then a continuous oldest-first transcript of "User: text" / "Assistant: text" line-groups with no other headers or per-turn metadata. The user label is the configured extractor user name when set, falling back to "User". New pure functions in src/auto-capture-cleanup.ts: - formatConversationTranscript(turns, userLabel?) -- pure rendering. - buildConversationTurnsForExtraction(...) -- assembles the ordered turn sequence from this call's true message-loop order, consuming (never recomputing) the watermark/rolling-window decisions already made by the auto-capture hook: drops already-extracted leading user turns when newUserTexts is a narrower tail-slice of eligibleTexts, keeps every assistant-context turn from this call, prepends assistant context rolled over from a prior call, and falls back to flat user turns (still preceded by rolled-over context) when newUserTexts didn't come from a tail-slice of eligibleTexts at all (pending-ingress replay has no per-message role correlation available). Orthogonal to eligibility: it never touches autoCaptureSeenTextCount, minMessages, or any counting decision, only consumes their results for rendering. index.ts's auto-capture message loop now also collects role-tagged turns (both eligible and context-only messages, true chronological order) alongside the existing eligibleTexts/assistantContextTexts arrays, then narrows against cleanTexts (the POST noise-filter list, not the pre-filter texts) so the rendered transcript never re-includes content the embedding-noise filter already dropped from the actual extraction. SmartExtractor.extractCandidates prefers the caller's conversationTurns when given, falling back to one user turn from conversationText plus assistantContextTexts as trailing assistant turns for callers that don't have per-message role data. Fixed three tests asserting on the old two-block format (the label text, the old "## Recent Conversation" heading, and a hardcoded-heading prompt-capture filter in a third test's own LLM mock) to match; the eligibility-counting assertions in all three were untouched and still pass, confirming the watermark/minMessages behavior this task was scoped to leave alone is unaffected. New test/auto-capture-cleanup.test.mjs coverage (was previously unregistered) and this file are now both registered in package.json's test chain and scripts/ci-test-manifest.mjs. index.ts and scripts/ci-test-manifest.mjs carry this repo's known mixed CRLF/LF encoding; both were edited with a latin1 Buffer splice (never Edit) to avoid normalizing the files, per this fork's CLAUDE.md -- the first ci-test-manifest.mjs attempt via Edit did trigger the documented trap (55 lines flagged instead of 1) and was discarded and redone via the splice script.
A live-incident investigation initially suspected captureAssistant:
"context" of starving the auto-capture watermark gate by halving the
eligible-message count after a restart. That mechanism was falsified:
captureAssistantEligible = (value === true) is false for both false and
"context", so isEligibleRole reduces to role === "user" either way --
"context" mode only adds assistant-role text to a separate
assistantContextTexts array for prompt context, which never touches
eligibleTexts or the cumulative watermark count. The actual restart
-survivability defect was unrelated to captureAssistant (see the
fix/watermark-restart-survival branch).
Encode the four-way harness that proved this (captureAssistant {false,
"context"} x payload shape {full-history, delta-only}, each run across a
simulated restart) as a permanent test, so a future change can't silently
reintroduce a counting/watermark difference between the two modes without
a test noticing.
Test-only: no production code change, since the invariant already holds.
Validated the test is meaningful by temporarily reintroducing the
originally-suspected bug (captureAssistantEligible also true for
"context") locally, confirming both trace-symmetry assertions fail with a
clear diff, then reverting -- this file's diff contains only the new test
plus its two registrations.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Messages extractMinMessages now bounds the extraction transcript in user<->assistant PAIRS: the window keeps the newest N user turns with their assistant replies interleaved in true order (or all of this call's unextracted user turns when there are more), and captureAssistant context rides the same window instead of an independent 6-slot assistant carry. Consumed pairs drop together, removing the asymmetry where assistant history was effectively unbounded while user history was cut to the watermark tail. Also hardens the extraction prompt's assistant-lines rule with a live-caught counterexample (the assistant greeting the user by name is not the user introducing themselves). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tant captureAssistant true makes assistant-authored lines eligible grounding sources (concrete durable uncorrected facts only, user line wins on a tie); the default and 'context' modes keep assistant lines as disambiguation context that can never ground a candidate on its own. Previously the config reached message eligibility but the extraction prompt never branched.
…ves turns A below-threshold deferral keeps content alive on two independent paths: the rolling pair buffer, and the watermark rollback (or pending-ingress re-queue) whose next slice re-includes the same turns. When the deferred turn finally fires, window assembly concatenated both copies and the extraction transcript carried the same exchange twice, back to back (observed live: window inflation plus mild extraction bias toward the duplicated line). Collapse duplicates at assembly time, by user text at pair granularity: a pair-shaped copy beats a flat re-queued copy, copies of an identical exchange collapse to the latest, and a repeated user text whose replies differ is a real conversation and is kept whole. The deferral itself lives in the regex-fallback gating change (CortexReach#934); with both applied the window stays duplicate-free on every path.
…xtractions Deleting autoCaptureRecentPairTurns after every successful extraction meant steady-state captures (one extraction per turn) always saw a bare current pair; the context window only ever materialized across deferred or valid-empty turns. Keep the buffer on success — the set-time trimTurnsToUserCap bound still caps it, the watermark keeps retained context from re-becoming an extraction source, and dedupePairWindow guards double-includes. Also promote the warm-up skip line to info so a deferring session is visible in default fleet logs.
Contributor
Author
|
Superseded by #966, which recuts the pair-window retention together with the autoCaptureContextTurns knob that sizes it (0 preserves stock behavior). Closing this draft in favor of the new stack. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The extraction transcript's rolling pair window (
autoCaptureRecentPairTurns, introduced with the pair-shaped context work) was deleted after every successful extraction. In steady state, history-carrying sessions extract every turn, so the window never survived long enough to provide context: each extraction call saw only the current pair, and the multi-pair "context continuity" the window exists for only ever materialized across deferred or valid-empty turns. Observed live: withextractMinMessages: 3, a turn following a successful extraction carried a bare 1-pair transcript; a turn following a valid-empty extraction carried 2 pairs (the asymmetry made the window appear and disappear).Change
Keep the buffer on success (remove the delete). The window stays bounded and safe:
trimTurnsToUserCapalready trims at set time tomax(extractMinMessages, this call's new user turns)user turns.dedupePairWindowalready guards double-includes.Also promote the warm-up skip line ("skipped smart extraction... cumulative=N < minMessages=M") from debug to info: it is the only signal that a turn was deferred by the gate, and at debug a deferring session reads as a stall in default logs.
Tests
New
test/pair-window-retention.test.mjs(full-plugin harness, mock embedding/LLM servers): a successful extraction must carry the previous pair into the next extraction's transcript, and the window stays trimmed to the cap across repeated successes (oldest pairs drop, long-dropped pairs never resurface). Red before the change (second extraction carried only the current pair), green after. Full suite green.Stacked on #939 (the pair-window home); the diff includes that branch until it merges.