fix: preserve the auto-capture cursor after successful history-flow extractions - #932
Conversation
|
After #919 was merged, this PR now has merge conflicts with the latest |
67ef600 to
137ff6e
Compare
app3apps
left a comment
There was a problem hiding this comment.
Reviewed head 137ff6e. The targeted tests and full suite pass, and the patch fixes the reported cursor reset for the covered monotonically growing history flow.
Two non-blocking follow-ups are worth tightening:
- Auto-capture work is fire-and-forget. An older, shorter capture can finish after a newer one and overwrite the newer watermark, causing already-consumed messages to be submitted again.
- When the next eligible history is equal in length or shorter, the strict growth check leaves the entire history in
newTexts; unchanged retries or rolling windows can therefore be reprocessed wholesale.
A per-session generation/serialization guard plus equal-length and out-of-order completion regressions would make the cursor robust. These edge cases do not outweigh the concrete improvement in this PR, so approving.
|
This PR has developed merge conflicts again after the latest changes landed on Please rebase onto the current |
…ction Two auto-capture flows share the autoCaptureSeenTextCount counter. In the ingress flow (message_received feeds pendingIngressTexts and each agent_end carries only the newest message) the counter is a pure accumulator of new texts toward extractMinMessages, and resetting it to 0 after a successful extraction is the intended windowing from issue message history each turn) the same counter is also the slice cursor into that history; resetting it to 0 made the very next capture re-read and re-extract the entire session: repeated LLM cost and repeated admission and dedup evaluations over already-extracted content. Make the post-success reset flow-aware: ingress-fed turns keep the reset to 0 (the existing counter-reset scenario in test/smart-extractor-branches.mjs still passes unchanged), while history-carrying turns record the consumed history length so the next capture only sees the delta. Red-proved: the new regression test fails against the previous code with turn 1 content re-read in turn 2, and passes after the change. Wire the suite into the local npm test chain and the core-regression CI group.
137ff6e to
eadeaf2
Compare
app3apps
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head eadeaf2. The targeted tests and full suite pass, and the monotonic-history cursor fix remains sound for the primary flow.
Non-blocking follow-ups: equal or shorter history redelivery can still be treated as entirely new, and cursor updates around extraction/concurrent completion could inflate or regress the saved position. Overlap, redelivery, and concurrency tests would tighten those edges.
Approving.
Problem
After every successful auto-capture write, the per-session cursor is reset to zero (
autoCaptureSeenTextCount.set(sessionKey, 0)). In history-carrying sessions, whereagent_enddelivers the whole session each turn, the cursor doubles as the slice offset, so the turn after any successful extraction re-reads the entire session history as new. Observed live: a later extraction's input re-contained turns already extracted minutes earlier. Costs grow with session length, and re-rolling the same content through extraction and admission multiplies the chances of a low-quality item eventually slipping through.The reset is not simply a bug: the commit trail and a pinning test show it is intentional for ingress-fed sessions (per-message channels feeding
pendingIngressTexts), where the counter acts as a pure new-text accumulator forextractMinMessageswindowing and a length-based reset would wedge the window closed.Fix
Flow-aware reset:
pendingIngressTexts.length > 0 ? 0 : eligibleTexts.length. Ingress sessions keep the accumulator semantics; history sessions record the consumed length so subsequent turns slice only the delta.Tests
Red-proven regression (turn 2 re-read turn 1's content before the fix; only the delta after) plus the pre-existing ingress counter-reset scenario passing unchanged.
Verification on a live gateway
A four-turn session produced extraction inputs containing each turn exactly once; the previous whole-history re-read signature is gone.