fix(dashboard): make edit-resend a real conversation boundary - #8431
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Both load-bearing claims verify: Mochi's Design-Verdict: CONCERNS Sound fix to a real desync class, but it multiplies refusals into a client that corrupts on every refusal, and forks the boundary transaction a second time. Watch
Suggestions
[DESIGN-REVIEWED] 3783a54 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All facts verified. The review contract's output follows. First-Principles-Verdict: CONCERNS The fix is real and aimed at the named cause, but it lands as a second hand-written copy of rewind's boundary transaction that has already diverged from it in both directions. What this change shipsIntent: stop an edited-and-resent dashboard message from leaking the replaced suffix into the new turn or silently desyncing the live window from disk (issue #7838) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3783a54 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @bolichen97 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
05497bf to
29d6198
Compare
|
Both blocking lanes are correct on all four findings — I verified each against the code rather than rebutting any of them. They are now enumerated in the PR body under Open blocking findings, and the body's earlier "app-ownership is deliberately out of scope" claim is explicitly withdrawn: base edit-resend also never cleared a native conversation, so this PR introducing They are not fixed on this branch for one reason: #8421 already implements all four, on the same endpoint, for the same issue (#7838). Its diff carries the copy-severing ( So the remaining work on this PR is a vehicle decision, not a code change: one of #8421 and this PR should be closed. The only substantive difference between them is the error-code vocabulary (#8421 reuses rewind's Green-lane work done in the meantime, since it is orthogonal to the decision:
|
29d6198 to
67e69e7
Compare
67e69e7 to
ea90522
Compare
Response to the two
|
ea90522 to
86935db
Compare
|
|
1a9a4ec to
d90d510
Compare
|
d90d510 to
b9192a5
Compare
|
The rewind boundary transaction freezes a window snapshot on the event loop and then awaits the save in a worker thread, where _save_slot_to_history read the LIVE _disk_older_count. A trim at the window cap in that gap credits the trimmed rows to the frozen prefix while the snapshot still holds them at its head, so the rewrite emitted them twice -- a silently duplicated transcript, reachable on origin/main today. The endpoint now passes the boundary it observed in the same synchronous stretch as the snapshot. The save refuses on drift (nothing written, the retryable 503 the transaction already contracts for) and the commit re-adopts that boundary, because the commit puts the pre-trim window prefix back on the live window. The durable position base (_disk_older_durable_count) is re-adopted with it: the trim advances the two together, and a row counted as having left the window front while it is back inside it refuses a valid absolute cursor or repeats rows. The post-write witnesses are gated on the routing for the same reason: they describe the file just written but live on the slot, which the loop can rebind mid-write. Stamping then cleared a _pending_rewrite the new transcript still owed and claimed its unsaved rows as persisted, which a later flush believed. Every witness left at its pre-save value is the conservative reading of that witness. Split out of the edit-resend port (#8431 carries the endpoint); both defects live in the shared save and are reachable on rewind without it.
b9192a5 to
6e52ab7
Compare
|
Response to the two
The finding is exactly right. So the extra save is gone rather than guarded. A carried row now reaches disk the ordinary way — the commit sets Worth recording:
The client defect is at
Mochi's bridge is the only consumer of this endpoint ( I am deliberately not posting |
Port the post-#5395 rewind transaction shape into api_chat_slot_edit_resend: prepare the truncated+edited window on a copy.copy(slot) prospective slot, durably clear the native ACP conversation (discard_conversation(skip_if_busy=True) + aflush) before the history rewrite, and mutate the live slot + dispatch the replacement turn only after both boundaries commit. A discard/flush failure now returns 503 (edit_resend_prepare_failed), a busy native session returns 409 (edit_resend_session_busy), and a history-write failure or refused save returns 503 (edit_resend_save_failed) instead of the old log-and-continue 200 that dispatched a turn from never-persisted state. The regenerate and switch-variant siblings are unchanged. The history write is also shielded. The worker thread cannot be interrupted, so a client disconnect during a bare await would abandon a completed destructive rewrite: history truncated on disk while the live slot still holds the full original window, which the next dirty-slot flush then re-serializes back over the truncated file. On cancellation the handler now waits for the worker's real outcome and, if the rewrite landed on the transcript it authorized, commits the live slot to match disk and dispatches the edited prompt before propagating the cancel. Four properties the ported shape has to carry, because each fails toward the permissive answer if it is left out. Every one is pinned by a test that goes red without it. The prospective slot is SEVERED, not merely copied. copy.copy is shallow, so reassigning messages leaves _queue, _pending, _question_pending, _on_question_retired and event aliased to the live slot -- and _ChatSlot.append writes through four of them. Un-severed, the prospective append pushes the edited row into the live pending queue, wakes the stream reader, and announces the live question cards as retired before any of the five refusal paths can reject the edit, leaving a phantom row and a card-less "needs input" behind. The commit is the one place that prepared state becomes live, and it re-announces the retirement through the callback the copy was denied. App callers are authorized. Discarding the native conversation is a destructive capability, so an app token reaching a slot it does not own destroys a resume identity it has no claim on. It runs through the shared _check_slot_app_ownership gate plus _reauthorize_after_await across the body-read window rather than a second hand-rolled spelling: that gate already authorizes the _app binding, the effective session key and the transcript key, so a channel-linked slot and an unbound channel-origin slot are both covered by one check. The chat_handlers import is function-local by necessity -- that module cannot be the package's first import, so hoisting it breaks importing chat_regenerate on its own. The slot is reserved before the awaits. slot.running derives from slot.task and the send path is not serialized on slot._lock, so a send arriving during the three durable boundaries would see an idle slot and dispatch a competing turn the commit then erases. The reservation diverts it to the queue and, on abort, hands it to the canonical successor dispatch so it is never stranded; an entry queued before the reservation keeps its own trigger. The transcript key is re-checked on the success path, not only under cancellation. A cron or workflow injection can re-link the slot mid-persistence; the snapshot froze the old routing, so the save's own expected_history_key guard cannot see the live slot move and only this loop-side check can (edit_resend_slot_rebound). A fifth property, raised on the first revision and fixed the same way: a row that arrives during the boundary is CARRIED, not replaced away. workflow_inject and cron_inject append on the event loop and take no slot._lock, so a completion landing mid-boundary reaches the live window while this handler holds the lock. A wholesale slot.messages = prospective_slot.messages drops it, and the rewrite save cannot restore it either because a rewrite deliberately skips the cross-process-append scan (collect_foreign = not rewrite) -- leaving the row in neither the window nor the file. Arrived rows are identified by row object (the window front can be trimmed, and a restore-path row carries no meta.mid) and carried onto the committed window and pending queue, which is what lets the next ordinary flush re-persist them. Ordering is safe because monotonic_transcript_ts only ever moves a row forward. The question map is intersected rather than adopted, so a card retired by either the edit or an arrived row stays retired and is announced once. The mid-save cancellation test's cross-thread gate is bounded and released in a finally. It blocks a thread in the DEFAULT executor, which the interpreter joins at exit, so an unreleased gate did not fail the test -- it hung interpreter shutdown for the whole xdist worker. Bounding the INNER Event.wait is the load-bearing half: cancelling wait_for abandons the future but cannot interrupt a worker already sitting in Event.wait. The outer 2s bound is unchanged, so a save that never starts still fails fast. A busy SESSION is not the same question as a busy slot, and the discard is a full teardown that also releases the shared sub-agent runtime. slot.running tracks only this slot's own task, so it reads False both BETWEEN an autopilot plan's stages and while the parent's children keep running after the parent turn ended. Apply the same two guards the sibling reset-conversation teardown applies before the same call, in the same order and with the same codes: slot_orchestrating on _in_stage_execution, and slot_subagents_running through the shared chat_utils.subagents_attached predicate, which fails closed on an unreadable probe. skip_if_busy stays the atomic backstop for a turn admitted after both answered False. The commit's target check is one predicate covering all three axes that move across the boundary awaits, shared by the success and cancellation paths so neither can check a different subset. Beyond the transcript key: the slot OBJECT, because a close-and-recreate under one name is a different conversation that leaves the transcript key unchanged; and the dispatch reservation, because if something else has taken slot.task then committing runs this handler's turn alongside whatever now owns the slot -- two concurrent turns writing one window. The commit also advances slot.total_messages. It is a LIFETIME counter and the prospective append bumped only the copy's int, so the edited row was invisible to every reader of it: _get_active_workspace picks the max-counter slot to resolve which workspace's lessons to load, and the Slack mirror compares the counter against its own start value to decide whether anything happened. Incremented by one at the commit rather than adopted from the copy, whose value predates the arrived rows that bumped the live counter themselves. The cancellation drain survives REPEATED cancellation. CancelledError is a BaseException, so a second cancel -- a gateway shutdown reaching a handler already unwinding from a client disconnect -- is not absorbed by the except Exception around the drain, and a bare await on the save task then abandons a rewrite the worker finishes anyway: disk truncated, live slot still holding the discarded suffix, next flush pushing that stale window back over the truncated file. Re-shield the drain a bounded number of times and read the outcome off the SETTLED task instead of awaiting it, so a cancel landing between the two cannot lose it. Giving up leaves the live slot untouched, the safe half of the desync. The history rewrite goes through save_slot_off_loop rather than a bare to_thread, which excludes the periodic dirty-slot flush for its duration. Because the live slot keeps the FULL window until the commit, a flush tick could snapshot that stale window, block behind the rewrite on the per-session history lock, and then write the snapshot back on top -- restoring every message the rewrite just discarded. The helper raises slot._metadata_persist_inflight around the write and lowers it in a finally, which is the flag flush_slot_now already honours to keep the unpinned periodic writer off a slot with a guarded write pending. Shielding the wrapper rather than the inner future is what holds the exclusion: a cancellation reaching the shield leaves the coroutine running, so its finally cannot release the flag early. best_effort=False so a failure reaches the 503 instead of being swallowed and re-armed as a dirty retry. Nothing awaits between the commit and the dispatch release. Once the live slot has adopted the truncated window, dispatch_commit is already True and only dispatch_ready.set() in the finally remains, so an await in that gap lets a cron or workflow completion rebind the slot and the released dispatch then runs the edited prompt against ANOTHER conversation. Re-checking the commit target after such an await cannot rescue it either: refusing once the live slot holds the truncated window would leave a truncation with no turn. So a row the commit carried reaches disk the ordinary way -- the commit sets _dirty and the next periodic flush writes the merged window, the same durability gap every ordinary append has -- rather than through a second guarded save the boundary would have to await. rewind still awaits its orphan-session cleanup in that gap. The arrived-row ordering test asserts "never earlier" rather than "strictly later". monotonic_transcript_ts only ever moves a row forward, so an arrived row cannot be stamped before the edited one -- but it CAN be stamped identically, because on a coarse clock (Windows ticks in ~15.6 ms steps) both appends read the same instant. The strict form passed on Linux and reded on Windows for exactly that reason; list order is what separates the tie, which is why the merge order matters rather than a re-sort. Also prune test/test_chat_regenerate_cov80.py from .github/black-baseline.txt: this commit rewrote most of that file and it is black-clean now, so the entry is stale and the ratchet moves in the shrinking direction.
A present non-string `content` reached `.strip()` and raised
AttributeError, so `{"content": 123}` answered 500 instead of a 400
naming the field, and an unbounded replacement prompt passed the request
boundary that the sibling `rewind` and `fork` endpoints both cap. Both
now refuse with a machine-readable code before the destructive boundary
runs, so an oversize edit cannot discard the native conversation first.
A missing or null `content` keeps answering `content_required`: that is
what an empty composer sends, and it is not a type error.
|
Head All 59 other checks are green. The rebase onto current The lane's finding changed between heads, so the body's "The one open finding" section is one round behind. At
The mechanism is correct and I am not disputing it. Three checks a maintainer can run before deciding, because two of them contradict the adjudicator's severity reasoning ("no rollback, no dispatch, and no self-correcting flush … recovery: none"):
Why neither suggested fix is a patch to this endpoint. "Prevent routing changes across save-and-commit" means What I am asking for. A maintainer decision on scope: whether this PR absorbs a shared-boundary-contract change (fencing rebind for the transaction, across The finding is fenced ( |
|
/ai-review override gpt 3783a54: pre-existing cross-endpoint residual this PR narrows -- main's edit-resend saves with no expected_history_key and dispatches anyway, main's rewind ships the identical check at chat_rewind.py:514, and the dropped tail is best-effort archived by _archive_dropped_lines (chat_persistence.py:3312). The real fix is a rebind boundary lock across four endpoints. |
Human judgment recorded@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
bolichen97
left a comment
There was a problem hiding this comment.
Tech Lead review — APPROVE on the merits (recorded as a comment: GitHub blocks self-approval)
I read the 554-line chat_regenerate.py diff rather than the body, and the conversation-boundary semantics are correct on the eight properties that matter:
- Nothing live mutates before every irreversible boundary commits. The truncated+edited window is built on a
copy.copy(slot)whose five shared mutables (_queue,_pending,_question_pending,_on_question_retired,event) are severed, so all six refusal paths leave the live slot byte-identical. Verified against_ChatSlot.append's four write-throughs. - Ordering is right.
discard_conversation(skip_if_busy=True)+aflush()precede the history rewrite, so the replaced suffix cannot re-enter the new turn via native resume. A save failure after the discard leaves history intact and the native session rebuilt from it -- the safe direction. - The periodic flush cannot undo the rewrite. Routing through
save_slot_off_loop(..., expected_history_key=..., best_effort=False)raises_metadata_persist_inflightfor the whole write, and shielding the wrapper (not the inner future) is what holds that exclusion under cancellation. This is the load-bearing anti-corruption mechanism given the live slot keeps the full window until commit; a bareto_threadwould reopen #7838. - The commit fence is symmetric and complete. One
_commit_target_intact()shared by the success and cancellation paths, covering transcript key, slot object identity, and the dispatch reservation -- so a close-and-recreate under the same name and a displaced reservation both refuse. No await between the fence and the mutations. - No message loss. Rows arriving mid-boundary are identified by object identity (correct:
append's own trim breaks a positional cut, and restore-path rows carry nometa.mid) and carried onto both the window and_pending;total_messagesadvances by one; the question map is intersected tightest-wins. Theid()keys are sound because every pre-await row stays referenced -- the prefix byprospective_slot.messages, the suffix by the liveslot.messages. - Nothing awaits between commit and dispatch release. Withdrawing the second guarded save rather than guarding it was the right call:
dispatch_commitis alreadyTruethere, and refusing after the live slot adopted the truncated window would leave a truncation with no turn. - Cancellation is handled against an uninterruptible worker. The bounded re-shield drain reads the outcome off the settled task, so a second
CancelledError(aBaseException) cannot walk away from a landed rewrite; giving up leaves the live slot untouched, which is the safe half. - Silent desync is gone.
log-and-continuebecomes retryable 503/409 with machine-readablecodefields per AGENTS.md's backend-string rule, and the destructive boundary is now behind the type check and the 32768 cap rather than in front of them.
Scope is disciplined. api_chat_slot_regenerate and api_chat_slot_switch_variant are byte-for-byte unchanged, and their surviving try: save / except: logger shape is filed in the Pattern harvest rather than quietly folded in. docs/system-specs/modules/session.md and learn-cron-dashboard.md are updated in the same commits, which is what AGENTS.md § Specification management requires. The .github/black-baseline.txt prune is one deletion, ratchet-shrinking, and declared.
On the overridden GPT 5.6 finding -- I did not take the author's override on faith. The residual is a rebind boundary spanning four endpoints; this PR adds expected_history_key where main had none and a loop-side three-axis fence with no await between check and mutation, which is as tight as a single-endpoint change can be. The override reasoning holds on the diff.
One residual I did not see raised by any lane, recorded so it is not rediscovered: the commit replaces slot._pending wholesale with the pre-await snapshot plus arrived rows, so if the live stream reader drained _pending during the boundary those rows are resurrected and re-delivered. Low severity (duplicate stream rows; slot.messages stays correct) and identical to what shipped rewind does at chat_rewind.py:396-397, so it is not a regression here -- it belongs to Pattern harvest item 2, which already files it as an open class.
Both CONCERNS verdicts are advisory and I agree with both as follow-ups, not blockers. The shared-helper extraction (Design, First Principles) is the right next move before a third inline copy lands, and the three spellings of the 32768 cap should collapse onto the constant this PR names. The Mochi renderer's !result?.ok fallback is reachable on main today and this PR touches no file under website/; it does make refusals more routine, so the follow-up should land close behind -- but holding a server-side data-loss fix hostage to a frontend PR keeps the worse bug live.
Merging as squash.
Closes the discarded-suffix leak and silent-desync class on the dashboard edit-resend path (issue #7838), by porting the post-#5395 rewind conversation-boundary transaction shape from
chat_rewind.pytoapi_chat_slot_edit_resendinsrc/kiro_crew/dashboard/chat_regenerate.py.Problem
api_chat_slot_edit_resendwas the counted sibling of the rewind path with the same root cause, left out of PR #5395's scope:logger.warning("edit-resend: failed to persist")) rather than a retryable 503, so a failed save silently dispatched a replacement turn from state that was never persisted (silent desync of the live window from disk).Changes
Scope is edit-resend only. Siblings
api_chat_slot_regenerateandapi_chat_slot_switch_variantare byte-for-byte unchanged.session_key/expected_history_keybefore any mutation; build the truncated+edited window on acopy.copy(slot)prospective slot;discard_conversation(session_key, skip_if_busy=True)+aflush()to durably clear the native conversation BEFORE the history rewrite; persist viasave_slot_off_loop(..., expected_history_key=..., best_effort=False)wrapped inasyncio.ensure_future+asyncio.shield; commit the live slot and dispatch_run_chatonly after every boundary commits.edit_resend_prepare_failed(discard/flush exception, 503),edit_resend_session_busy(busy session, 409),edit_resend_save_failed(save exception or guard-refused, 503),edit_resend_slot_rebound(slot moved to another transcript mid-persistence, 503)._queue,_pending,_question_pending,_on_question_retired,event.slot_orchestratingwhen a plan is mid-stage, and 409slot_subagents_runningthrough the sharedsubagents_attachedpredicate, because the discard releases the runtime the parent's children run on.dispatch_ready-gated task, with a_start_next_queued_turnhandoff on abort._check_slot_app_ownershipgate plus_reauthorize_after_awaitacross the body-read await.dispatch_commitis alreadyTrueand onlydispatch_ready.set()in thefinallyremains, so an await in that gap lets a cron or workflow completion rebind the slot and the released dispatch then runs the edited prompt against another conversation.rewindalready does: a present non-stringcontentis a 400invalid_contentinstead of reaching.strip()as a 500 ({"content": 123}was a 500 onmainand on the first commit here), and acontentover_MAX_EDIT_CONTENT_CHARS(32768, the capchat_rewindandchat_forkboth apply) is a 400content_too_long. Both refuse before the destructive boundary, so an oversize edit cannot discard the native conversation first. A missing ornullcontentstill answerscontent_required— that is what an empty composer sends, and it is not a type error.docs/system-specs/modules/session.md(the boundary section now covers edit-resend and states the eight load-bearing properties, naming the four whererewindis still exposed) anddocs/system-specs/modules/learn-cron-dashboard.md(the endpoint's contract and refusal codes; it previously read "in-place truncation ofslot.messages", which is no longer what the endpoint does).test/test_chat_regenerate_cov80.pyfrom.github/black-baseline.txt: it rewrote most of that file and it is black-clean now, so the entry is stale and the ratchet moves in the shrinking direction. (PR Hygienecaps a PR at two commits, so this rides with the change that caused it rather than as its ownchore.)Blocking findings: resolved
Three lanes raised four blocking findings against
29d6198c;GPT 5.6 Reviewraised two more against67e69e7e, then one each againstea90522,86935db,cfec4cd,2980159,455b903,5aa4c34,0068bb2, and two againstb9192a5e— narrowing as the actionable ones were fixed. Thirteen of the fifteen were correct and are fixed here; round nine's "fence the transaction" residual is no longer raised once its actionable half landed as item 4, and the fifteenth is below. Each fix was verified to be a real regression guard by reverting the source and re-running.copy.copyis shallow, so reassigningmessagesalone left_queue,_pending,_question_pending,_on_question_retiredandeventaliased to the live slot — and_ChatSlot.appendwrites through four of them (state.py:4272-4273for_pending.append+event.set(),state.py:4181-4196for the question filter and the_on_question_retiredfire). Un-severed, the prospective append pushed the edited row into the live pending queue and announced the live question cards as retired before any of the five refusal paths could reject the edit, leaving a phantom row and a card-less "needs input" behind. The commit is now the one place that prepared state becomes live, and it re-announces the retirement through the callback the copy was denied. Pinned bytest_edit_resend_refusal_leaves_the_live_pending_and_cards_alone(refusal side) andtest_edit_resend_commit_adopts_the_prepared_pending_and_retires_cards(commit side).chat_handlers._check_slot_app_ownership— which already authorizes the_appbinding, the effective SESSION key and the TRANSCRIPT key, so a channel-linked slot and an unbound channel-origin slot are covered by one check with nothing to keep in sync — plus_reauthorize_after_awaitacross the body-read await, sincelinked_session_keyis rebound on already-live slots with norunninggate. Denials are SEL-logged 404s (anti-enumeration, CWE-204). Pinned bytest_edit_resend_denies_an_app_that_does_not_own_the_slot,test_edit_resend_denies_an_app_reaching_a_channel_linked_session, andtest_edit_resend_reauthorizes_the_slot_after_the_body_read(which also shows the guard is not app-only: a dashboard caller must not land a destructive edit on a slot that was replaced across the await either).slot.runningderives fromslot.taskand the send path is not serialized onslot._lock, so a send arriving during the discard/flush/save saw an idle slot and dispatched a competing turn the commit then erased. Adispatch_ready-gated task now publishes the reservation with no await between the idle check and the assignment; the turn runs only oncedispatch_commitis set, and on abort the task hands a send it diverted to_start_next_queued_turnso nothing is stranded. An entry queued before the reservation keeps its own trigger. Pinned bytest_edit_resend_reserves_the_slot_so_a_concurrent_send_queues,test_edit_resend_abort_hands_a_diverted_send_to_the_queue_drain, andtest_edit_resend_abort_leaves_a_pre_existing_queue_entry_waiting.expected_history_keyguard cannot see the live slot move and only this loop-side check can. (b) The slot object: a close-and-recreate under the same name is a different conversation that leaves the transcript key unchanged, so nothing but object identity catches it. (c) The dispatch reservation: if something else has takenslot.task, committing would run this handler's turn alongside whatever now owns the slot — two concurrent turns writing one window. Any of the three refuses with a retryable 503edit_resend_slot_rebound. Pinned bytest_edit_resend_refuses_the_commit_when_the_slot_is_rebound,..._is_replaced, and..._the_reservation_is_displaced; each stubs the save to accept the write so only the commit-side fence can refuse.workflow_injectandcron_injectappend throughappend_and_surface/slot.appendon the event loop and take noslot._lock(workflow_inject.py:172), so a workflow or cron completion landing mid-boundary reaches the live window while this handler holds the lock. A wholesaleslot.messages = prospective_slot.messagesthen drops it — and the rewrite save cannot restore it, because an explicit-snapshot save impliesrewrite(chat_persistence.py:2608-2609) andrewriteskips the cross-process-append scan (collect_foreign=not rewrite,:3265), so the row ends up in neither the window nor the file. Arrived rows are now identified by row object — a positional cut breaks the momentappend's own trim drops leading rows, and a restore-path row carries nometa.mid— and carried onto the committed window and_pending, which is what lets the next ordinary flush re-persist them. Ordering is safe rather than lucky:monotonic_transcript_tsonly ever moves a row forward (history.py:1062-1066), so a row appended after the edited one stamps after it. The question map is intersected rather than adopted, so a card retired by either the edit or an arrived row stays retired and is announced exactly once. Pinned bytest_edit_resend_commit_keeps_a_row_injected_during_the_boundaryandtest_edit_resend_commit_does_not_resurrect_a_card_retired_meanwhile.I want to be explicit that I initially drafted a rebuttal to this one, on the grounds that appending arrived rows after the prospective window would produce a descending
tsseam. That was wrong —monotonic_transcript_tsis dominated by wall-clocknow, so the later append stamps later. I checked the claim before posting it, found it false, and fixed the finding instead.finally. Upheld againstno-test-side-effects(AUTOSDE.yaml:192-193,blocking: true) and correct: the gate blocks a thread in the default executor, which the interpreter joins at exit, so an unreleased gate did not fail the test — it hung interpreter shutdown for the whole xdist worker. Bounding the innerEvent.waitis the load-bearing half, because cancellingwait_forabandons the future but cannot interrupt a worker already sitting inEvent.wait(). The outertimeout=2is deliberately unchanged: it is the real assertion boundary. The 30s backstop is a deadlock guard, never a synchronisation point, so a loaded shared runner cannot turn it into a flake.slot.runningtracks only this slot's own task, whilediscard_conversationis a full teardown that also releases the shared sub-agent runtime — sorunningreads False both between an autopilot plan's stages and while the parent's children keep going after the parent turn ends.main's edit-resend never discarded anything, so this PR is what makes the omission load-bearing. Fixed by applying the two guards the siblingreset-conversationroute already applies before the same call, in the same order and with the same codes rather than new ones (chat_handlers.py:4419-4442): 409slot_orchestratingonslot._in_stage_execution, and 409slot_subagents_runningvia_subagents_attached_response→ the sharedchat_utils.subagents_attachedpredicate, probed on the effective session key since that is the runtime being released.skip_if_busy=Truestays the atomic backstop for a turn admitted after both answered False. Pinned bytest_edit_resend_refuses_while_a_plan_is_mid_stage,test_edit_resend_refuses_while_subagents_are_attached, andtest_edit_resend_refuses_when_the_subagent_probe_fails— the last one pins that an unreadable probe means unknown children rather than zero.slot.total_messages. It is a lifetime counter that survives trimming (state.py:3374), and the prospectiveappendbumped only the copy's int — so the edited row was invisible to every reader of it. Two real consumers:handlers/_shared.py:369_get_active_workspacepicks the max-counter slot to resolve which workspace's lessons/api/lessonsloads, andchat_slack.py:132/269compares the counter against its own start value to decide whether anything happened during a mirrored turn. Incremented by one at the commit rather than adopted from the copy, whose value predates the arrived rows that bumped the live counter themselves; truncation deliberately does not roll it back. Pinned bytest_edit_resend_commit_advances_the_lifetime_message_counter, which also asserts a refused edit leaves it alone.CancelledErroris aBaseException, so a second cancellation (a gateway shutdown reaching a handler already unwinding from a client disconnect) is not absorbed by theexcept Exceptionaround the drain, and a bareawait save_taskthen walks away from a landed rewrite: disk truncated, live slot still holding the discarded suffix, next flush pushing that stale window back over the truncated file. The drain now re-shields a bounded number of times (_SAVE_DRAIN_ATTEMPTS) and reads the outcome off the settled task rather than awaiting it, so a cancel landing between the two cannot lose it; giving up leaves the live slot untouched, which is the safe half of the desync. Pinned bytest_edit_resend_repeated_cancellation_still_commits_the_landed_rewrite, which cancels twice.rewindstill drains with a bareawait(chat_rewind.py:459-466) and is noted in the Pattern harvest.main's edit-resend truncated the live window before saving, so its flush would have snapshotted the already-truncated window; the copy design is what opened this.) Fixed by saving throughchat_persistence.save_slot_off_loopinstead of a bareasyncio.to_thread: with anexpected_history_keyit raisesslot._metadata_persist_inflightaround the write and lowers it in afinally, which is the flagflush_slot_nowalready honours to keep "this unpinned periodic writer" off a slot with a guarded write pending — reused rather than respelled.best_effort=Falseso a failure still reaches the 503 rather than being swallowed and re-armed as a dirty retry. Shielding the wrapper rather than the inner future is what holds the exclusion for the whole write. Pinned bytest_edit_resend_excludes_the_periodic_flush_during_the_rewrite, which runs a realflush_slot_nowfrom inside the save and asserts it declined.GPT 5.6was right that this is unsafe:dispatch_commitis alreadyTrueat that point and onlydispatch_ready.set()in thefinallyremains, so a cron completion rebinding the slot during that await releases_run_chatagainst the new session. Its suggested remedy (re-check the save result and_commit_target_intact()before releasing dispatch) cannot rescue it either: the live slot has already adopted the truncated window, so refusing there would leave a truncation with no turn. The extra save is therefore gone. A carried row reaches disk the ordinary way — the commit sets_dirty, so the next periodic flush writes the merged window, which is the same durability gap every ordinary append already has, and it is what shippedrewindand fix(history): pair a frozen save snapshot with its prefix boundary #8421 both do. Pinned bytest_edit_resend_commit_keeps_a_row_injected_during_the_boundary, which now asserts the boundary writes exactly once (saved_windows == [["edited"]]) with the arrival still carried in the live window; the arrival-retention assertions themselves are unchanged.rewindstill awaits its orphan-session cleanup in that same gap (chat_rewind.py:537-544:dispatch_commit = True, thenawait _delete_orphan_kiro_session(...), thendispatch_ready.set()), so it is exposed onmaintoday — recorded in the Pattern harvest rather than fixed here.docs/system-specs/modules/session.mdsaid "Five properties" above seven bullets (non-blocking, originvalidation). Corrected — and there are eight now, since the no-await-before-dispatch property above earned its own bullet. The sentence also claimed all of them were "load-bearing on both endpoints" while three bullets said in their own text thatrewindwas still exposed; it now says which fourrewinddoes not carry, so nobody reads the list as already shared.One implementation note worth flagging for review: the
chat_handlersimport is function-local by necessity, not by preference.kiro_crew.dashboard.chat_handlerscannot be the first module of the package to import (its transitivevalidation↔artifactscycle resolves only once something else has pulled those in), so hoisting it to module scope makesimport kiro_crew.dashboard.chat_regeneratefail on its own.session_control.pyandhandlers/core.pyreach it the same way. The comment at the import site says so, so a later simplification pass does not hoist it back.Testing
main(04d38a783). That is what clearedBackend Tests (3.12, 3),Backend Tests (Windows) (3)andCoverage Gate: those nine failures weremain's own push-arity regression intest_push_branch_gate.py/test_security.py, fixed upstream byc791f0f1d(fix(security): hand the push arity scan raw words, not cut ones #8712, "Fixes the main-wide push-gate breakage blocking every open PR's shard"), whichgit merge-base --is-ancestorconfirms was inorigin/mainand not in the previous base6d1b51704. Verified locally on the rebased tree:test_push_branch_gate.py127 passed,test_security.py1877 passed / 1 skipped, with no change to either file from this branch.Coverage Gatefails closed behind a failed shard and never evaluated a floor, so it is one failure with the shard rather than two.test_chat_regenerate_cov80.py+test_dashboard_chat_rewind.py+test_dashboard_chat.py+test_dashboard_chat_pins.py+test_kiro_prerequisite.py: 1150 passed. The two new input-validation tests were verified to be real guards by revertingchat_regenerate.pyalone and re-running: both fail ({"content": 123}is a 500 without the type check; the oversize body reachesdiscard_conversationwithout the cap).flake8 src/kiro_crew test(whole tree),mypy --platform linux src/kiro_crew(1294 files, clean), brand-name, harness-parity, changelog-history, docs-lint, scrub-lint (--no-history). One note for reproducers: scrub-lint reds locally underLANG=en_US.UTF-8on a/home/téststring intest/test_atomic_write_named_duplicates.pythat is present onorigin/main(ae457328e) and untouched here; under CI'sC.UTF-8it passes, which matches the greenDe-Amazon Scrub Lintlane.test_edit_resend_skips_the_extra_persist_when_nothing_arrived— was deleted because the code it described is gone, and the property it protected ("exactly one write on the normal path") is now asserted by the arrived-row test in the harder case where something did arrive.The one open finding
GPT 5.6 Reviewis red on one finding, and I am not clearing it with/ai-review override gpt— that is a repository writer's decision, not mine.The finding is real, and it is not in this diff.
website/src/apps/mochi/src/renderer/ChatPanel.tsx:996-1014truncates the panel's local transcript optimistically (prev.slice(0, idx)on bothmessagesandallHistory) and then, on!result?.ok, falls back toapi.sendMessage(text). The server still holds the untouched suffix, so the edited text lands after it and the two transcripts diverge. Three checks:origin/maintoday, unchanged by this PR.main'sapi_chat_slot_edit_resendalready answers 404slot_not_found, the remote-bound 409, 400invalid_json(twice), 400content_required, 409slot_running, 400user_message_not_found, 400index_not_user_messageand 400index_or_ts_required. Every one of those is a!result?.ok, and the fallback corrupts on each.git diff origin/main --name-onlyon this branch lists no path underwebsite/.mainreturned{"ok": true}after a failed save and dispatched anyway — the client was lied to and had nothing to recover from. This PR replaces that with a retryable refusal. The new codes do make a refusal more routine (a busy session is not a caller error), which is a fair reading of the finding; what they do not do is create the client bug.!result?.ok, put the composer back in edit mode with the text, and surface the failure through the existingapps.mochi.chat.send_failedbanner the waysendTextalready does atChatPanel.tsx:915-926— instead of sending normally. That toucheswebsite/src/apps/**, which makesScreenshot Evidencerequired, and it has a genuine visual delta (the restored rows and the banner), so it needs a real capture of the Mochi failure path — not the<!-- no-visual-delta -->waiver, which would be a false claim. Folding a frontend UX change plus visual evidence into a backend endpoint PR is a scope call for a maintainer.Mochi's bridge is the only consumer of this endpoint (
grep -rn 'editResend' website/src→api/client.ts:3036exports it,panelBridge.ts:1473wraps it,ChatPanel.tsx:1008is the sole caller), so the follow-up is worth filing rather than leaving to be rediscovered. I am happy to do it as a follow-up PR with screenshots, or to fold it in here if a maintainer prefers that scope.Pattern harvest
Rule candidate: semgrep — three patterns, all of which this PR is a data point for.
copy.copy(<slot-like>)treated as an isolation boundary without severing the shared mutable attributes. A shallow copy leaves_pending,_queue,_question_pending,_on_question_retiredandeventaliased to the original, so a "prospective"append()on the copy mutates the live object — and it does so before the rejection paths the copy exists to make safe, which is what makes the failure invisible in the happy path and wrong only when the request is refused.chat_rewind.py:244-256reassigns all five immediately aftercopy.copy; this PR's first revision reassigned onlymessages, and both review lanes caught it. A rule that flagscopy.copy(x)followed by a mutating call on the copy without an intervening reassignment of every mutable-container attribute the class declares would have caught it at authoring time.slot.messages = prospective_slot.messagessilently discards anything that reached the live window while the boundary was pending, and the injectors that can do so —workflow_inject,cron_inject— append on the event loop and take noslot._lock, so holding it is not protection.chat_rewind.py:391-395already reasons this way for_queue("an entry queued while the boundaries were pending belongs to the NEW timeline") while replacingmessagesat:390and_pending/_question_pendingat:396-397wholesale, which is what makes this a class rather than a slip. Open onchat_rewind.py:390-398, and onregenerate/forkthrough the samecollect_foreign=not rewriterewrite-save path.exceptaround a destructive persist whose only action is alogger.*call._save_slot_to_historyfailing must refuse the request, not fall through to a dispatch; the original edit-resend bug in this PR's title is precisely atry: save / except: logger.warning(...)with noreturn. Flagexceptblocks that wrap a call to a_save_*_to_history-shaped function and contain noreturn/raise. Two more instances of this exact shape remain in the same file and are deliberately out of this PR's scope —api_chat_slot_regenerate(chat_regenerate.py:114-118, which truncates the live window at:104-112before the unchecked save) andapi_chat_slot_switch_variant(:213-217).Three items this PR names and does not own, so they are not lost:
rewindawaits inside the commit-to-dispatch gap (chat_rewind.py:537-544), which is finding 11 above, still open onmain.rewinddrains a cancelled save with a bareawait(chat_rewind.py:459-466), which is finding 9.ChatPanel.tsx:996-1014), which is the open finding above and affectsmaintoday.Relationship to #8421
Resolved: this PR is the vehicle and #8421 shrinks to its unique half. #8421 ("fix: clear the native context boundary before edit-resend rewrites (#7838)") is an independent full rewrite of the same function toward the same contract — the two conflict wholesale and neither can merge before the other is withdrawn. Both bodies said so; this section records the outcome rather than asking for one.
Where this PR is wider, verified against #8421's head
b0ec299rather than its body:chat_handlers._check_slot_app_ownership(four conditions) +_reauthorize_after_await. fix(history): pair a frozen save snapshot with its prefix boundary #8421 hand-rolls three checks (not slot._app,request_app != slot._app, and alinked_session_keypresence test) and never comparesslot_history_key(slot)against_history_key_for(slot.key). The shared gate's own docstring names the case that leaves open: an unboundchannel_originslot has an emptylinked_session_key, so the effective-session comparison is a value against itself whileslot_history_keyresolves onto the channel's transcript. Under fix(history): pair a frozen save snapshot with its prefix boundary #8421 an app-owned channel-origin slot passes and edit-resend truncates and discards the native identity of a foreign channel conversation.slot._in_stage_execution(409slot_orchestrating) and attached sub-agents (409slot_subagents_running) before the discard. fix(history): pair a frozen save snapshot with its prefix boundary #8421 has neither, relying only onskip_if_busy=True— which cannot see either, becauseslot.runningreads False in both states.save_slot_off_loop(..., best_effort=False), which raisesslot._metadata_persist_inflightfor the whole write. fix(history): pair a frozen save snapshot with its prefix boundary #8421 uses a bareasyncio.to_thread(_save_slot_to_history, ...), which never raises it — andflush_slot_nowdoes not takeslot._lock, so that flag is its only defence. Since the prospective-copy design leaves the live slot holding the full pre-edit window until the commit, a flush tick inside fix(history): pair a frozen save snapshot with its prefix boundary #8421's transaction writes that stale window back over the truncated file, reopening the exact desync edit_resend shares the rewind context-boundary root cause fixed for rewind in PR #5395 #7838 was filed about.await save_taskinsideexcept CancelledError.Two claims in the previous version of this section were stale and are corrected: #8421 does retain arrived window rows (via a lifetime-counter delta, not a wholesale replace) — that assessment was made against its older head
0cac476— and its app isolation is a three-check version missing the transcript-key condition, not an absence of isolation.What #8421 uniquely owns and should keep: its second commit,
fix(history): pair a frozen save snapshot with its prefix boundary, touchingchat_persistence.py+chat_rewind.py+test_dashboard_chat_rewind.py+history.md— zero files in common with this PR, so it lands independently with no conflict, and it fixes a duplicated-transcript bug reachable onrewindonmaintoday. Its input hardening lived in the file this PR rewrites, so it is absorbed here as the second commit above rather than lost.One follow-up neither PR covers: this PR's edit-resend saves through
save_slot_off_loop, which does not forwardexpected_disk_older_count. Once #8421's persistence half lands, threading the paired boundary throughsave_slot_off_loopgives edit-resend the same cap-trim duplication protectionrewindwill have.