Reduce repeated ACP session context - #5423
Conversation
Deliver legacy standing context once after a successful ACP prompt and track live-session event delivery so later thread and DM prompts send only deltas. Fence successful native steer acknowledgements by ACP session identity across task-result races. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Distinguish history omitted because it was delivered earlier from a trigger-only fetch, and add regression coverage for channel delivery commit timing and stale in-flight steer acknowledgements. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wpfleger96
left a comment
There was a problem hiding this comment.
I found one correctness gap in the new delivery ledger. The success-only commit boundary and session fencing look solid, but merged carry-over events need to participate in the same delivered-ID accounting as current-batch events.
| } else { | ||
| None | ||
| }; | ||
| let triggering_ids: HashSet<String> = b |
There was a problem hiding this comment.
This set only includes b.events, but format_prompt() also renders every b.cancelled_events entry in a merged cancel/re-prompt. After that merged prompt succeeds, those carry-over IDs are therefore absent from delivered_event_ids. A later thread/DM context fetch can resend the already-delivered messages; if the current fetch already contains one, it can also appear twice in this same prompt (conversation context plus [What you were working on]).
Could you build one rendered-batch ID set from b.events.iter().chain(b.cancelled_events.iter()), use it both for context-delta exclusion and pending_delivered_event_ids, then add a regression test proving a successful merged turn commits both the carry-over and new IDs and does not replay either on the next turn?
There was a problem hiding this comment.
Fixed in ab85b03aa81d2b8588ffe649f6eff0c314a385b9. The prompt path now builds one rendered-batch ID set from events.chain(cancelled_events), uses it for context exclusion, and stages the same set for success-only delivery commit. Added merged_cancel_prompt_commits_and_deduplicates_all_rendered_event_ids, which checks both IDs enter the ledger, appear once in the merged wire prompt, and neither replays on the next turn. The branch is also merged with current main in 28da9bb92c7575a5a835a7284762cea5a997c620; the full pre-push gate passed on ab85b03aa.
Preserve both live-session delivery tracking and the incoming ACP usage baseline initialization while resolving the pool session-creation conflict. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Use every event rendered by a merged cancel/re-prompt as both a context exclusion and a success-only delivery-ledger candidate. Cover the merged turn and its following turn with exact-wire regression assertions. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl review on Wes’s behalf at exact head ab85b03aa81d2b8588ffe649f6eff0c314a385b9: cleared / would approve, but GitHub will not accept an approval from the PR author’s account.
The prior carry-over delivery-ledger blocker is resolved: both current and cancelled/merged event IDs now enter the success-only commit set, with exact-wire regression coverage. I also traced legacy standing-context delivery, conversation-context delta filtering, session invalidation/rotation, and native-steer ack ordering. Successful steer deliveries are fenced by ACP session ID across both pre-result staging and post-result idle-agent updates, so late acknowledgements cannot contaminate a replacement session. Failure and cancellation paths preserve retryability rather than committing delivery state.
Validation at this head:
cargo test -p buzz-acp— 736 unit tests + 9 lifecycle integration tests passedgit diff --check— clean
No blocking findings.
Addressed at ab85b03: merged carry-over event IDs are now included in the success-only delivery ledger, with regression coverage. Carl re-reviewed on Wes’s behalf; no blocking findings remain.
wpfleger96
left a comment
There was a problem hiding this comment.
I double-checked the carry-over fix at exact head ab85b03aa81d2b8588ffe649f6eff0c314a385b9. The merged batch now uses one ID set from events.chain(cancelled_events) for both context exclusion and success-only delivery commit. The new exact-wire regression verifies that each merged event appears once, both IDs enter the ledger after success, and neither replays on the following turn.
The merge resolution also preserves both delivery-state initialization and the incoming usage baseline initialization. All check runs at this head are complete and green. I don't see any remaining blockers.
…a inits Three test-only TaskMeta initializers added by main (#5423 steer-delivery tests) came in via the merge without permission_decision_tx, breaking cargo test -p buzz-acp compilation. Production init sites were correctly resolved in the merge commit; these three cfg(test) sites were outside the conflict hunks and were missed. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Summary
Validation
The pre-push hook passed on exact pushed head
6a768f1bc80fe63c686acf8d730f177fff8add3c:branch-skewdesktop-checkdesktop-typecheckdesktop-testrust-testsdesktop-tauri-checksFocused regression tests were also run while iterating:
channel_prompt_commits_delivery_state_only_after_acp_successin_flight_stale_native_steer_ack_cannot_update_replacement_sessionKnown limitations and follow-ups
A local Goose smoke timed out at
session/new. This diff does not change code that executes at or beforesession/new; its earliest affected runtime behavior is delivery-state insertion after session creation succeeds. The smoke failure is therefore bounded as environmental or pre-existing, but no successful live-provider turn was obtained. Scripted ACP wire/lifecycle tests carry the regression coverage.Durable process-restart/session resume remains out of scope for this slice of #5342. #5386 also remains separate pending upstream adapter support.