fix(dashboard): size transfer-bundle tail by message id (#4684)

[chenmingwei23]

Problem / Motivation

build_transfer_bundle (src/kiro_crew/dashboard/session_transfer.py) sizes the un-flushed tail of an exported session bundle as slot.messages[slot._disk_window_len:]. That counter advances only on the save and load paths, but a durable injector (cron_inject.py, workflow_inject.py, crew_chat.py) appends the same row to the resident window AND persists it to disk via append_if_absent with one shared meta.mid, without going through a save. The disk read then already returns the row while the counter has not moved, so the slice starts one row too early and the bundle carries the injection twice. PR #4137 rejected this exact estimator for this exact job on the read path, measured: substituting it broke test_transient_window_row_does_not_duplicate_the_tail.

Reachability, stated plainly: the shipped export endpoint (handlers_instances.api_instances_send_session) routes through build_transfer_bundle_async, which is NOT exposed -- an injector's slot.append marks the slot dirty (state.py:2291), the async builder flushes a dirty slot before snapshotting its tail (session_transfer.py), and the save folds the append_if_absent copy into the window (chat_persistence.py:1440-1452) and advances the boundary (:2113), so its counter is honest by the time the tail is cut. The sync builder is the documented entry for tests and off-loop callers; this PR removes the desynced estimator from that path rather than fixing a duplication reachable through today's send button.

Why it matters

Any present or future off-loop caller of the sync builder -- the module's own docstring invites them -- silently ships a transcript with a duplicated turn after a cron/workflow injection, and the receiving instance materialises the corrupted copy. It is the same trap the read path already paid to measure and remove (#4137); leaving it in the sibling function is how it gets reintroduced. Two estimator strategies for one job, one of them known-wrong, is also standing drift.

What changed (motivation -> approach -> change)

Symptom: an injected row appears twice in the exported bundle. Root cause: the tail is sized by a window-length counter that durable injectors structurally desync (they write disk + window with no save). Change: route the sync builder's tail through chat_handlers._append_unflushed_tail -- the id-based merge #4137 established -- which matches window rows against the disk read by meta.mid (the id an injector stamps on both copies), falls back to the ordered body walk when the disk window region has missing/mixed ids, and merges in only the rows disk does not already hold. The counter dependency disappears from this path, along with the now-dead if not all_messages fallback (and its identical dead twin in _read_and_assemble). The async sibling deliberately keeps its boundary slice -- its pre-snapshot flush is what makes that counter honest -- and its docstring now says so instead of claiming the two builders are the same. A layering comment at the new import records the direction constraint (session_transfer may import chat_handlers, never the reverse).

Tests

All in test/test_session_transfer.py, each red against the previous slice (mutation-verified by restoring the old estimator in place):

• test_durably_injected_row_is_not_duplicated_in_the_bundle -- the id-matching arm, the one an injector's rows actually take.
• test_durably_injected_row_is_not_duplicated_when_older_rows_have_no_id -- the ordered-comparison arm (pre-id-era transcripts).
• test_durably_injected_row_with_a_frozen_prefix_is_not_duplicated -- non-zero _disk_older_count, pinning the window-region offset arithmetic the sync path newly depends on.
• test_genuinely_owed_row_still_travels_after_an_injection -- control: a fix that merely stopped appending would pass the three above and fail this.
• test_bundle_ignores_the_resume_count_and_ships_each_turn_once -- the prior boundary test, rewritten to the invariant that survives the estimator change.
• The fake-slot helper gains an overridable _disk_older_count; the long-history test now models the realistic frozen prefix (disk_older=8).

Gates: isort / flake8 / mypy clean; test_session_transfer.py 134/134 and test_dashboard_chat.py 645/645 green; full suite's 82 fails + 2 errors reproduced byte-identical on pristine base (host-env: sandbox confinement, AF_UNIX path length), none touching this area.

Manual verification

N/A -- unit coverage sufficient: the changed function is pure data-flow over a fake-able slot/log pair, and the shipped async endpoint is intentionally unchanged.

Closes #4684

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0d76467

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reading the contract, intent, patch, and the surrounding code (session_transfer.py, chat_handlers._append_unflushed_tail, and consumer counts for both builders) is complete. Emitting the review.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but it lands on a builder with zero production callers — retiring that builder was the unweighed, deeper subtraction.

What this change ships

Intent: stop an exported session bundle from carrying a cron/workflow-injected turn twice — a FIX.

1. Sync bundle no longer duplicates a durably-injected turn — justified; reuses existing _append_unflushed_tail, cause level.
2. Dead if not all_messages fallback deleted from the sync builder — rides along; pure deletion, verified dead.
3. Identical dead fallback deleted from _read_and_assemble — rides along; pure deletion, declared.
4. Async builder's docstring now states the two builders size tails differently — declared.
5. Sync builder's history param now demands the full chained corpus — tightened caller contract, declared.
6. New import edge session_transfer → chat_handlers, direction pinned by comment — declared.
7. Test fake gains disk_older; four new tests, one rewritten — declared.

Watch

• Reachability: grep build_transfer_bundle( under src/ — 1 hit, the definition; the only shipped path is build_transfer_bundle_async (handlers_instances.py:854), unchanged. The description concedes it: "rather than fixing a duplication reachable through today's send button." The named harm ("any present or future off-loop caller") is prophylactic; session_transfer sizes the un-flushed tail with _disk_window_len, which a durable injector leaves behind #4684 gives it a filed defect, which is why this is CONCERNS, not BLOCK.
• The delete option was never weighed: with zero production consumers, removing the sync builder removes the same drift without keeping two tail estimators correct forever.

Subtractions

• Defer-then-retire build_transfer_bundle: 0 production consumers (grepped build_transfer_bundle( — def site plus 17 test-only calls); pointing tests at the async builder deletes the second estimator instead of maintaining it.

[FIRST-PRINCIPLES-REVIEWED] 0d76467

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Consolidates onto the proven #4137 id-merge at the true root cause; honest about reachability; async path deliberately and correctly untouched.

Suggestions

• Two modules now consume _append_unflushed_tail, yet it stays a private symbol in chat_handlers, guarded only by a layering comment that already names its correct home — do the move to chat_persistence (with _snapshot_slot_window and its body-compare collaborators) now, while the second consumer is fresh, rather than after a cycle forces it.

[DESIGN-REVIEWED] 0d76467

[chenmingwei23]

Disposition for the First Principles CONCERNS (advisory) on 0d76467:

Accepted and deferred. The subtraction is real: build_transfer_bundle has zero production callers (the shipped export path is build_transfer_bundle_async), so retiring it and pointing the 17 test-only call sites at the async builder would delete the second tail estimator instead of maintaining it. Deferring rather than folding it in because (a) it is a scope change beyond #4684 -- that issue files a defect in the estimator, and this PR fixes exactly that; (b) rewiring 17 sync test call sites onto an async builder changes what those tests exercise (the async path's flush/retry loop), which deserves its own review rather than riding a converged fix; (c) the module docstring documents the sync builder as the off-loop/test entry, so removal is an API decision for a maintainer-scoped follow-up. The drift risk in the meantime is bounded: both builders now either share the id-based helper or document why not, and the layering comment pins the import direction. Will file the retire-or-keep decision as a follow-up item rather than expanding this diff.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34 — this comment is updated in place on each push.

Review details

The candidate list contains no candidates — the discovery pass found nothing. I verified the change independently:

• The sync build_transfer_bundle is test-only in production (only build_transfer_bundle_async is reachable via handlers_instances), so the switch to _append_unflushed_tail carries no production data-loss path.
• The removed _read_and_assemble fallback (if not history: history = list(tail)) is a genuine no-op after history.extend(tail).
• The removed sync fallback (if not all_messages: all_messages = list(slot.messages)) is subsumed: with an empty disk read, _append_unflushed_tail routes every window row to owed_rows and returns them all.
• _append_unflushed_tail's id-arm/ordered-arm logic is exercised by the new tests and matches the changed call site; _snapshot_slot_window reads _disk_older_count which the test fakes now supply.
• No import cycle: chat_handlers does not reference session_transfer in its transitive graph.

No grounded (a)/(b)/(c) defect survives, and I found no new one to add.

No findings.

[OPUS-REVIEWED] 0d76467

Verdict parsed from the review's SHA-scoped output markers for commit 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0d76467b29c67a5bd2c31f4d4bb6341ae98c8f34: <one-sentence reason>

[bolichen97]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL both present, success, 0 findings), security checklist all-NO, AI reviewers green. Category: fix with clear root cause — transfer-bundle tail sized by _disk_window_len re-appended a durably-injected row already on disk (mirrors #4137); now sized by message id via _append_unflushed_tail.

[bolichen97]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: clear root cause (#4684) — sync transfer-bundle tail sized by meta.mid identity via _append_unflushed_tail instead of the _disk_window_len boundary a durable injector never advances.

[iamwhatever]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep+CodeQL success, no alerts), security checklist all-NO, AI reviewers green. Category: sizes the transfer-bundle unflushed tail by message id instead of the _disk_window_len boundary, fixing duplicated injector rows (regression #4137) in the sync bundle builder.