Skip to content

fix(dashboard): make edit-resend a real conversation boundary - #8431

Merged
bolichen97 merged 2 commits into
mainfrom
fix/edit-resend-conversation-boundary
Sep 6, 2026
Merged

fix(dashboard): make edit-resend a real conversation boundary#8431
bolichen97 merged 2 commits into
mainfrom
fix/edit-resend-conversation-boundary

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

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.py to api_chat_slot_edit_resend in src/kiro_crew/dashboard/chat_regenerate.py.

Thirteen of the fifteen blocking review findings raised across ten rounds are fixed here, each pinned by a test that goes red without it — see Blocking findings: resolved. A fourteenth (round nine's "fence the transaction" residual) is no longer raised: its actionable half became the three-axis commit fence in item 4. Design Review, First Principles Review, Opus 4.8 Review and UX Review are green. One finding remains open: it is a pre-existing defect in the Mochi renderer, in a file this PR does not touch, and closing it turns a backend-endpoint PR into a frontend one — see The one open finding. Duplicate-PR triage against #8421 is resolved in Relationship to #8421.

Problem

api_chat_slot_edit_resend was the counted sibling of the rewind path with the same root cause, left out of PR #5395's scope:

  • It did not discard the native conversation before dispatching the replacement turn, so the replaced suffix could still reach the new turn via native ACP session resume (discarded-suffix leak).
  • Its history-rewrite failure was log-and-continue (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_regenerate and api_chat_slot_switch_variant are byte-for-byte unchanged.

  • New boundary flow: capture session_key/expected_history_key before any mutation; build the truncated+edited window on a copy.copy(slot) prospective slot; discard_conversation(session_key, skip_if_busy=True) + aflush() to durably clear the native conversation BEFORE the history rewrite; persist via save_slot_off_loop(..., expected_history_key=..., best_effort=False) wrapped in asyncio.ensure_future + asyncio.shield; commit the live slot and dispatch _run_chat only after every boundary commits.
  • Retryable refusal codes: 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).
  • The prospective copy is severed, not merely copied — _queue, _pending, _question_pending, _on_question_retired, event.
  • A row that arrives on the live slot during the boundary is carried onto the committed window and pending queue rather than replaced away, and the question map is intersected rather than adopted.
  • A busy session is refused as well as a busy slot: 409 slot_orchestrating when a plan is mid-stage, and 409 slot_subagents_running through the shared subagents_attached predicate, because the discard releases the runtime the parent's children run on.
  • The slot is reserved before the boundary awaits via a dispatch_ready-gated task, with a _start_next_queued_turn handoff on abort.
  • App callers are authorized through the shared _check_slot_app_ownership gate plus _reauthorize_after_await across the body-read await.
  • 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.
  • Client-disconnect during the save: the shield keeps the handler alive to observe the worker's real outcome and, if the rewrite landed on the authorized transcript, commits the live slot to match disk and still dispatches, closing a disk-ahead desync window.
  • Second commit hardens the request boundary the same way the sibling rewind already does: a present non-string content is a 400 invalid_content instead of reaching .strip() as a 500 ({"content": 123} was a 500 on main and on the first commit here), and a content over _MAX_EDIT_CONTENT_CHARS (32768, the cap chat_rewind and chat_fork both apply) is a 400 content_too_long. Both refuse before the destructive boundary, so an oversize edit cannot discard the native conversation first. A missing or null content still answers content_required — that is what an empty composer sends, and it is not a type error.
  • Docs updated in the same commits: docs/system-specs/modules/session.md (the boundary section now covers edit-resend and states the eight load-bearing properties, naming the four where rewind is still exposed) and docs/system-specs/modules/learn-cron-dashboard.md (the endpoint's contract and refusal codes; it previously read "in-place truncation of slot.messages", which is no longer what the endpoint does).
  • The first commit also prunes test/test_chat_regenerate_cov80.py from .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 Hygiene caps a PR at two commits, so this rides with the change that caused it rather than as its own chore.)

Blocking findings: resolved

Three lanes raised four blocking findings against 29d6198c; GPT 5.6 Review raised two more against 67e69e7e, then one each against ea90522, 86935db, cfec4cd, 2980159, 455b903, 5aa4c34, 0068bb2, and two against b9192a5e — 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.

  1. The prospective copy is now fully severed. copy.copy is shallow, so reassigning messages alone left _queue, _pending, _question_pending, _on_question_retired and event aliased to the live slot — and _ChatSlot.append writes through four of them (state.py:4272-4273 for _pending.append + event.set(), state.py:4181-4196 for the question filter and the _on_question_retired fire). 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 by test_edit_resend_refusal_leaves_the_live_pending_and_cards_alone (refusal side) and test_edit_resend_commit_adopts_the_prepared_pending_and_retires_cards (commit side).
  2. App callers are authorized, through the shared gate. Since this PR is what introduces a destructive native-session capability on this endpoint, the missing guard became load-bearing. Rather than a second hand-rolled spelling of rewind's two inline checks, it calls chat_handlers._check_slot_app_ownership — which 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 covered by one check with nothing to keep in sync — plus _reauthorize_after_await across the body-read await, since linked_session_key is rebound on already-live slots with no running gate. Denials are SEL-logged 404s (anti-enumeration, CWE-204). Pinned by test_edit_resend_denies_an_app_that_does_not_own_the_slot, test_edit_resend_denies_an_app_reaching_a_channel_linked_session, and test_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).
  3. The slot is reserved across the boundary awaits. slot.running derives from slot.task and the send path is not serialized on slot._lock, so a send arriving during the discard/flush/save saw an idle slot and dispatched a competing turn the commit then erased. A dispatch_ready-gated task now publishes the reservation with no await between the idle check and the assignment; the turn runs only once dispatch_commit is set, and on abort the task hands a send it diverted to _start_next_queued_turn so nothing is stranded. An entry queued before the reservation keeps its own trigger. Pinned by test_edit_resend_reserves_the_slot_so_a_concurrent_send_queues, test_edit_resend_abort_hands_a_diverted_send_to_the_queue_drain, and test_edit_resend_abort_leaves_a_pre_existing_queue_entry_waiting.
  4. The commit's target is re-checked on the success path, not only under cancellation — and on all three axes that can move. One predicate, shared by both paths, so neither can check a different subset (that asymmetry is what let a rebind through in the first place). (a) The transcript key: a cron or workflow injection re-links 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. (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 taken slot.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 503 edit_resend_slot_rebound. Pinned by test_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.
  5. A row arriving during the boundary is carried, not replaced away. workflow_inject and cron_inject append through append_and_surface / slot.append on the event loop and take no slot._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 wholesale slot.messages = prospective_slot.messages then drops it — and the rewrite save cannot restore it, because an explicit-snapshot save implies rewrite (chat_persistence.py:2608-2609) and rewrite skips 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 moment append's own trim drops leading rows, and a restore-path row carries no meta.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_ts only 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 by test_edit_resend_commit_keeps_a_row_injected_during_the_boundary and test_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 ts seam. That was wrongmonotonic_transcript_ts is dominated by wall-clock now, so the later append stamps later. I checked the claim before posting it, found it false, and fixed the finding instead.

  1. The mid-save cancellation test's cross-thread gate is bounded and released in a finally. Upheld against no-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 inner Event.wait is the load-bearing half, because cancelling wait_for abandons the future but cannot interrupt a worker already sitting in Event.wait(). The outer timeout=2 is 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.
  2. A busy SESSION is refused, not just a busy slot. slot.running tracks only this slot's own task, while discard_conversation is a full teardown that also releases the shared sub-agent runtime — so running reads 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 sibling reset-conversation route already applies before the same call, in the same order and with the same codes rather than new ones (chat_handlers.py:4419-4442): 409 slot_orchestrating on slot._in_stage_execution, and 409 slot_subagents_running via _subagents_attached_response → the shared chat_utils.subagents_attached predicate, probed on the effective session key since that is the runtime being released. skip_if_busy=True stays the atomic backstop for a turn admitted after both answered False. Pinned by test_edit_resend_refuses_while_a_plan_is_mid_stage, test_edit_resend_refuses_while_subagents_are_attached, and test_edit_resend_refuses_when_the_subagent_probe_fails — the last one pins that an unreadable probe means unknown children rather than zero.
  3. The commit advances slot.total_messages. It is a lifetime counter that survives trimming (state.py:3374), and the prospective append bumped 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_workspace picks the max-counter slot to resolve which workspace's lessons /api/lessons loads, and chat_slack.py:132/269 compares 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 by test_edit_resend_commit_advances_the_lifetime_message_counter, which also asserts a refused edit leaves it alone.
  4. The cancellation drain survives REPEATED cancellation. The worker thread cannot be interrupted, so once the rewrite starts it lands whether the handler lives or not — the handler has to learn the outcome and commit to match. But CancelledError is a BaseException, so a second cancellation (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 save_task then 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 by test_edit_resend_repeated_cancellation_still_commits_the_landed_rewrite, which cancels twice. rewind still drains with a bare await (chat_rewind.py:459-466) and is noted in the Pattern harvest.
  5. The periodic dirty-slot flush is excluded for the whole rewrite. 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. (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 through chat_persistence.save_slot_off_loop instead of a bare asyncio.to_thread: with an expected_history_key it 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 "this unpinned periodic writer" off a slot with a guarded write pending — reused rather than respelled. best_effort=False so 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 by test_edit_resend_excludes_the_periodic_flush_during_the_rewrite, which runs a real flush_slot_now from inside the save and asserts it declined.
  6. No await sits between the commit and the dispatch release — and the way that was fixed is by withdrawing an addition, not by adding a guard. The previous revision made a carried row durable with a second guarded save after the commit. GPT 5.6 was right that this is unsafe: dispatch_commit is already True at that point and only dispatch_ready.set() in the finally remains, so a cron completion rebinding the slot during that await releases _run_chat against 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 shipped rewind and fix(history): pair a frozen save snapshot with its prefix boundary #8421 both do. Pinned by test_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. rewind still awaits its orphan-session cleanup in that same gap (chat_rewind.py:537-544: dispatch_commit = True, then await _delete_orphan_kiro_session(...), then dispatch_ready.set()), so it is exposed on main today — recorded in the Pattern harvest rather than fixed here.
  7. docs/system-specs/modules/session.md said "Five properties" above seven bullets (non-blocking, origin validation). 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 that rewind was still exposed; it now says which four rewind does not carry, so nobody reads the list as already shared.

One implementation note worth flagging for review: the chat_handlers import is function-local by necessity, not by preference. kiro_crew.dashboard.chat_handlers cannot be the first module of the package to import (its transitive validationartifacts cycle resolves only once something else has pulled those in), so hoisting it to module scope makes import kiro_crew.dashboard.chat_regenerate fail on its own. session_control.py and handlers/core.py reach it the same way. The comment at the import site says so, so a later simplification pass does not hoist it back.

Testing

  • Rebased onto current main (04d38a783). That is what cleared Backend Tests (3.12, 3), Backend Tests (Windows) (3) and Coverage Gate: those nine failures were main's own push-arity regression in test_push_branch_gate.py / test_security.py, fixed upstream by c791f0f1d (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"), which git merge-base --is-ancestor confirms was in origin/main and not in the previous base 6d1b51704. Verified locally on the rebased tree: test_push_branch_gate.py 127 passed, test_security.py 1877 passed / 1 skipped, with no change to either file from this branch. Coverage Gate fails 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 reverting chat_regenerate.py alone and re-running: both fail ({"content": 123} is a 500 without the type check; the oversize body reaches discard_conversation without the cap).
  • Local gates clean on the committed range: black formatting, subprocess-encoding, isort, 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 under LANG=en_US.UTF-8 on a /home/tést string in test/test_atomic_write_named_duplicates.py that is present on origin/main (ae457328e) and untouched here; under CI's C.UTF-8 it passes, which matches the green De-Amazon Scrub Lint lane.
  • No test was weakened, no retry or sleep added, nothing baseline-suppressed. The one previously-asserted behaviour that changed — 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 Review is 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-1014 truncates the panel's local transcript optimistically (prev.slice(0, idx) on both messages and allHistory) and then, on !result?.ok, falls back to api.sendMessage(text). The server still holds the untouched suffix, so the edited text lands after it and the two transcripts diverge. Three checks:

  • It is reachable on origin/main today, unchanged by this PR. main's api_chat_slot_edit_resend already answers 404 slot_not_found, the remote-bound 409, 400 invalid_json (twice), 400 content_required, 409 slot_running, 400 user_message_not_found, 400 index_not_user_message and 400 index_or_ts_required. Every one of those is a !result?.ok, and the fallback corrupts on each. git diff origin/main --name-only on this branch lists no path under website/.
  • What this PR changes is the server's honesty, in the safe direction. main returned {"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.
  • Fixing it is a different PR, and the repo's own gates say so. The remedy is in the renderer: snapshot both arrays before the optimistic cut, restore them on !result?.ok, put the composer back in edit mode with the text, and surface the failure through the existing apps.mochi.chat.send_failed banner the way sendText already does at ChatPanel.tsx:915-926 — instead of sending normally. That touches website/src/apps/**, which makes Screenshot Evidence required, 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/srcapi/client.ts:3036 exports it, panelBridge.ts:1473 wraps it, ChatPanel.tsx:1008 is 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.

  1. 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_retired and event aliased 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-256 reassigns all five immediately after copy.copy; this PR's first revision reassigned only messages, and both review lanes caught it. A rule that flags copy.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.
  2. A "prospective copy" commit that REPLACES a live container wholesale instead of reconciling it. The severing above stops the copy leaking into the live slot; this is the mirror defect on the way back out. slot.messages = prospective_slot.messages silently 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 no slot._lock, so holding it is not protection. chat_rewind.py:391-395 already reasons this way for _queue ("an entry queued while the boundaries were pending belongs to the NEW timeline") while replacing messages at :390 and _pending/_question_pending at :396-397 wholesale, which is what makes this a class rather than a slip. Open on chat_rewind.py:390-398, and on regenerate / fork through the same collect_foreign=not rewrite rewrite-save path.
  3. An except around a destructive persist whose only action is a logger.* call. _save_slot_to_history failing must refuse the request, not fall through to a dispatch; the original edit-resend bug in this PR's title is precisely a try: save / except: logger.warning(...) with no return. Flag except blocks that wrap a call to a _save_*_to_history-shaped function and contain no return/raise. Two more instances of this exact shape remain in the same file and are deliberately out of this PR's scopeapi_chat_slot_regenerate (chat_regenerate.py:114-118, which truncates the live window at :104-112 before the unchecked save) and api_chat_slot_switch_variant (:213-217).

Three items this PR names and does not own, so they are not lost:

  • rewind awaits inside the commit-to-dispatch gap (chat_rewind.py:537-544), which is finding 11 above, still open on main.
  • rewind drains a cancelled save with a bare await (chat_rewind.py:459-466), which is finding 9.
  • The Mochi renderer turns any edit-resend refusal into a divergent transcript (ChatPanel.tsx:996-1014), which is the open finding above and affects main today.

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 b0ec299 rather than its body:

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, touching chat_persistence.py + chat_rewind.py + test_dashboard_chat_rewind.py + history.mdzero files in common with this PR, so it lands independently with no conflict, and it fixes a duplicated-transcript bug reachable on rewind on main today. 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 forward expected_disk_older_count. Once #8421's persistence half lands, threading the paired boundary through save_slot_off_loop gives edit-resend the same cap-trim duplication protection rewind will have.

@bolichen97
bolichen97 requested a review from a team as a code owner September 4, 2026 09:55
@bolichen97
bolichen97 requested a review from CrysisDeu September 4, 2026 09:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Both load-bearing claims verify: Mochi's !result?.ok fallback re-sends the edit as a normal message (ChatPanel.tsx:1009-1014), and chat_rewind.py still carries the bare drain (:464) and the commit-gap await (:537-544) that this PR fixes only on edit-resend. I have what I need.

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

  • New refusals feed the known-corrupting client fallback. This PR adds routine refusals (409 edit_resend_session_busy/slot_orchestrating/slot_subagents_running, 503s) → Mochi, the endpoint's only caller, treats any !result?.ok by re-sending the edit as a plain message after an optimistic local truncate (ChatPanel.tsx:996-1014) → users may see divergent transcripts more often post-merge, on busy sessions especially. The renderer follow-up the description promises must be filed and land close behind, not left "worth filing."
  • Second divergent hand-copy of a subtle concurrency transaction. Edit-resend now carries four safety properties rewind lacks (bare drain at chat_rewind.py:464, commit-gap await at :537-544), while regenerate/switch_variant in the same file keep the original log-and-continue save bug, and fix(history): pair a frozen save snapshot with its prefix boundary #8421 is a third independent copy. The ten review rounds this took are evidence the shape is too fragile to keep re-deriving per endpoint.

Suggestions

  • Extract the boundary transaction (sever/reserve/discard/save/fence/commit) into a shared helper in a follow-up, and re-base rewind and the regenerate/switch_variant fixes on it, before a third inline copy lands.

[DESIGN-REVIEWED] 3783a54

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3783a54208b3e124c5a97688a86a13ae9fd878ab — 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.

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 ships

Intent: 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.

  1. Edit-resend clears the agent's native conversation before re-running — justified (the reported leak)
  2. A failed history save is a retryable 503, not silent success — justified (the reported desync)
  3. Edits refused while a plan is mid-stage or subagents run (two 409s) — justified, derived from the new destructive discard
  4. A busy session refused 409 instead of torn down — justified
  5. App callers can no longer edit-resend slots they don't own (404) — justified, reuses the shared gate
  6. A message sent during the edit now queues instead of running — justified, derived from the new awaits
  7. Non-string content is a 400 instead of a 500 — rides along, declared, named defect on main
  8. New 32768-char content cap — rides along, declared; third spelling of one cap
  9. Disconnect mid-save still commits and dispatches — justified (worker thread is uninterruptible)
  10. Black-baseline entry pruned for the rewritten test — rides along, declared, ratchet-shrinking

Watch

  • Second spelling of the boundary transaction. chat_rewind.py already holds the identical inline sequence (discard → aflushsave_slot_off_loop with expected_history_key → drain → commit fence; chat_rewind.py:233, 332, 456, 467, 514). This PR ports ~350 lines rather than sharing, and divergence is already documented by the PR itself: four properties where "rewind is still exposed" (bare-await drain, wholesale replace, post-commit await, injected-row loss — the last also open in regenerate and fork, 3 counted unfixed siblings). Declared and deferred, so accepted-and-deferred, but the shared-helper extraction is now the third time this transaction will be needed.
  • The 32768 cap has three spellings: _MAX_EDIT_CONTENT_CHARS (chat_regenerate.py:29) plus inline 32_768 at chat_rewind.py:124 and chat_fork.py:208 (grepped 32_768, 3 hits). The constant's own comment says the point is that the endpoints agree — three independent literals is how they stop agreeing.
  • Rewind's app gate stays hand-rolled (two inline checks, chat_rewind.py:87-88 and 217) while this endpoint uses the shared _check_slot_app_ownership — 1 unfixed sibling of the same root cause.

Subtractions

  • Replace the inline 32_768 literals at chat_rewind.py:124 and chat_fork.py:208 with the one constant this PR names, so the cap has a single owning module per the code-style rule.

[FIRST-PRINCIPLES-REVIEWED] 3783a54

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 3783a54208b3e124c5a97688a86a13ae9fd878ab; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt 3783a54208b3e124c5a97688a86a13ae9fd878ab: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3783a54208b3e124c5a97688a86a13ae9fd878ab — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 3783a54

Verdict parsed from the review's SHA-scoped output markers for commit 3783a54208b3e124c5a97688a86a13ae9fd878ab.

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

@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from 05497bf to 29d6198 Compare September 5, 2026 03:57
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

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 discard_conversation here is what makes rewind's two isolation guards load-bearing.

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 (_queue/_pending/_question_pending/_on_question_retired/event), both SEL-logged 404 app-isolation guards, the dispatch_ready slot reservation with the _start_next_queued_turn handoff and queued-entry preservation, and the success-path slot_history_key(slot) == expected_history_key re-check — plus a docs/system-specs/modules/session.md update this PR lacks. Porting them here would produce a line-for-line duplicate of that PR rather than an independent fix.

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 rewind_* codes; this PR mints edit_resend_*), which is the smaller question. I have deliberately not made that call.

Green-lane work done in the meantime, since it is orthogonal to the decision:

  • Rebased onto main @ f59b865bb (picks up the remote_bound_refusal guard from feat(remote-crew): run a local session on a connected crew #7693); the PR's own added/removed lines are byte-for-byte unchanged by the rebase, and the 78 targeted tests still pass.
  • Backend Lint & Type Check (3.12) was failing only on the black gate — test/test_chat_regenerate_cov80.py became black-clean and its .github/black-baseline.txt entry had to be pruned. Done in its own chore commit (2 commits total, within the hygiene cap).
  • Backend Tests (3.12, 1) was not a test failure: setup-uv aborted after 15s fetching astral-sh/versions/main/v1/uv.ndjson (##[error]The operation was aborted due to timeout), so zero tests were collected. Coverage Gate was failing closed downstream of it. Both should clear on this run.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from 29d6198 to 67e69e7 Compare September 5, 2026 05:04
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from 67e69e7 to ea90522 Compare September 5, 2026 05:26
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Response to the two GPT 5.6 Review blocking findings on 67e69e7e

Both mechanisms are real — I traced each to code rather than take either side on faith. One is fixed in ea90522; the other I am escalating rather than patching, with reasons below. (Full detail in the PR body: Blocking findings: resolved item 5, and The one open finding.)

Fixed — "Concurrent transcript injections are overwritten"

Correct, and reachable: workflow_inject appends through append_and_surface on the event loop with no slot._lock (workflow_inject.py:172), so holding the lock is not protection. The wholesale commit dropped the injected row, and the rewrite save could not put it back — an explicit-snapshot save implies rewrite (chat_persistence.py:2608-2609) and rewrite skips the cross-process-append scan (collect_foreign=not rewrite, :3265) — so the row ended up in neither the window nor the file.

ea90522 carries arrived rows (identified by row object: a positional cut breaks the moment append's trim drops leading rows, and a restore-path row has no meta.mid) onto the committed window and _pending, which is what lets the next ordinary flush re-persist them. 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. Pinned by test_edit_resend_commit_keeps_a_row_injected_during_the_boundary and test_edit_resend_commit_does_not_resurrect_a_card_retired_meanwhile; both fail against the previous head.

Worth recording: I first drafted a rebuttal to this one, arguing that appending arrived rows would produce a descending ts seam. That was wrongmonotonic_transcript_ts only ever moves a row forward (history.py:1062-1066), so the later append stamps later. I checked before posting, found my own argument false, and fixed the finding instead.

Open — "Channel turns can recreate the discarded session"

Also correct, and worse than stated: discard_conversation's default is replay=True, and at session_lifecycle.py:770-773 that branch does self._suppress_replay.discard(key) — it permits replay. A successor cold-started during the following two awaits therefore primes from persisted history, which in that window is still the pre-rewrite transcript.

It is not this PR's shape. chat_rewind.py runs the identical sequence with the identical default — discard_conversation(session_key, skip_if_busy=True) at :332, aflush() at :366, _save_slot_to_history at :452. No replay= argument at either site, no session-claim exclusion at either site. The window exists on shipped rewind right now, same key, same duration; #8421 carries it identically.

Why I am not patching it locally. skip_if_busy probes only at pop time; closing the window needs a claim exclusion or session-generation check spanning pop → rewrite → dispatch, which lives in session_lifecycle.py and must cover every boundary caller or the endpoints diverge on the very property being fixed. It also needs a call this PR cannot make — what a linked-channel message arriving mid-boundary should do (block, be refused, or be queued), each a visible behaviour change for channel users. The two cheap mitigations are both worse than saying so: replay=False suppresses replay permanently, so the edited turn loses all prior context (the opposite of the documented contract); re-discarding a successor after the save fixes only the not-busy case and adds a fourth await inside the window it is shrinking.

I am deliberately not posting /ai-review override gpt — waiving a security-class finding is a maintainer call, and so is scoping the fix. I would recommend it land as a separate change covering rewind, regenerate, fork and edit-resend together, since three of those four are already exposed on main; happy to implement it either way. Every claim above is a line citation, so if one is wrong I would rather be corrected than have the finding waived.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from ea90522 to 86935db Compare September 5, 2026 05:42
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

1a9a4ec — the carried row is now durable, and a note on where this loop lands

Fixed — "Arriving rows are acknowledged before becoming durable"

Correct, and it is the direct consequence of the previous round's fix rather than an independent defect: carrying an arrived row into the committed window kept it live, but the rewrite had persisted the prospective window only and dropped that row's own durable copy on the way (a rewrite skips the cross-process-append scan). Between the commit and the next periodic flush tick the row existed in memory alone, so a crash there lost a workflow result the user had already been shown.

1a9a4ec gives the merged window its own guarded durability pass before the request returns. Three details are deliberate:

  • Only when something actually arrived. carried_rows is empty on every normal edit, so the common path adds neither a second history write nor a second await. test_edit_resend_skips_the_extra_persist_when_nothing_arrived asserts exactly one write there.
  • After dispatch_commit, not before. This mirrors rewind's post-commit cleanup await (chat_rewind.py:534-537): a cancellation landing on this write must not abort the reserved dispatch once the commit has already happened, which would strand a persisted edit whose turn never runs.
  • Best-effort, and that is not the shape this PR's own Pattern harvest warns about. The except: logger.warning pattern is a defect when it swallows a destructive persist and falls through to a dispatch. Here the boundary has already committed, the write is purely additive, and a failure re-arms _dirty so the periodic flush retries — the same durability, just later. Making it fatal would be strictly worse: it would unwind nothing (the rewrite has landed) while turning a transient lock timeout into a 503 on a request that succeeded.

test_edit_resend_commit_keeps_a_row_injected_during_the_boundary now also asserts the persisted window is the merged one, and fails against the previous head.

Where this leaves the lane

Nine rounds, thirteen blocking findings, twelve fixed on merit with a test that goes red without each one. I have not rebutted by assertion once, and where I was wrong I said so — the ts-ordering argument I drafted against finding 5 was false, and I checked it before posting and fixed the finding instead.

Two observations a maintainer should weigh, offered as information rather than as a request to stop reviewing:

  1. Every finding since ea90522 has been anchored residual/crash-data-loss-corruption on the same code region, and several have been consequences of the previous round's fix (this one is a consequence of finding 5; finding 5 was a consequence of the copy design that findings 1–4 required). That is a sign the endpoint is converging, but it also means a green GPT 5.6 Review may not be reachable by iteration alone — the underlying object is the shared rewrite-boundary contract, and rewind / regenerate / fork carry most of these same residuals on main today.
  2. The one finding I have consistently escalated rather than patched — fencing rebind/close for the whole transaction — is the root of that class. I would implement it as a separate change covering all four callers, and I would rather do that than keep narrowing it one endpoint at a time.

I have not posted /ai-review override gpt at any point and will not: waiving a security-class finding, and deciding whether this PR or #8421 is the vehicle, are both maintainer calls.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

1a9a4ec: the lane has returned to the escalated finding

For the record, since this closes the loop rather than opening a new one: GPT 5.6 Review's finding on this head — "Post-save rebind check cannot undo a landed rewrite" — is the same residual it raised as "Rebind refusal follows the destructive rewrite" on ea90522, "Rebind refusal happens after history corruption" on 86935db, and "Live-slot mutation escapes the conversation boundary" on cfec4cd. Same line, same anchor, same mechanism, four spellings. Its actionable slivers were taken each time (the three-axis commit-target predicate is one of them); what is left is the root.

I am not going to iterate on it again, and I want to be explicit that this is a deliberate stop rather than fatigue:

  • The mechanism is real and I verified it, rather than dismissing it. This branch is reachable only when the save returned True, i.e. the rewrite landed on the transcript the request was authorized against, truncated as the user asked, with the dropped tail archived first (chat_persistence.py:3312-3316). What is lost is the dispatch, not the history.
  • The only alternative to refusing is to commit onto a conversation the caller never read, which is finding fix(app-sdk): harden scoped-API guard against path traversal and 204 bodies #4 from 29d6198c — the defect this same lane blocked on. chat_rewind.py:514 makes the identical check in the identical position, with a comment calling it load-bearing.
  • Both suggested fixes are boundary-contract changes, not endpoint patches. "Validate routing atomically with the rewrite" requires cron_inject / workflow_inject to take a fence they do not take today (workflow_inject.py:172 appends straight on the event loop) — a behaviour change for every injection consumer. "Reconcile a landed rewrite before refusing" requires an un-truncate operation that does not exist, and built naively it races the dirty-slot flush, re-opening the desync this PR closes. Either lands across rewind, regenerate, fork and edit-resend; three of those four carry this residual on main right now.

So this needs a maintainer decision, not another push: scope the boundary-contract fix (I will implement it across all four callers), or accept the residual on the grounds that it matches shipped rewind, or decide that #8421 is the vehicle and close this. I have not posted /ai-review override gpt at any point and will not — waiving a security-class finding is not mine to do.

Everything else on this head is green, including all Linux and Windows backend shards, the coverage gate, E2E, and the Design Review, First Principles Review, Opus 4.8 Review and UX Review lanes, which have been green since the first round of fixes.

@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from 1a9a4ec to d90d510 Compare September 5, 2026 07:58
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

d90d510 — the Windows shard failure was mine, and it is fixed

Two Windows shards were red on 1a9a4ec. They are two different things and only one is this PR's.

Backend Tests (Windows) (1) — mine, fixed

A real cross-platform bug in an assertion I added:

AssertionError: assert '2026-09-05T06:11:29.945181+00:00' < '2026-09-05T06:11:29.945181+00:00'

The arrived-row ordering test asserted ts[0] < ts[1]. monotonic_transcript_ts only ever moves a row forward, so an arrived row can never be stamped earlier than the edited one — but it can be stamped identically. The edited row is floored on the (empty) truncated prefix and the arrived row on the original window tail, so both resolve to plain now; on a coarse clock — Windows advances the system clock in ~15.6 ms steps, which is precisely why monotonic_transcript_ts exists — the two reads return the same instant. My assertion over-specified the guarantee and Linux's microsecond clock hid it.

d90d510 asserts what actually holds (<=, "never earlier"), with list order separating the tie and the content assertion above it pinning that order. This is not a loosened assertion to get green: the strict form was factually wrong, and the property the merge has to preserve is that an arrived row never sorts before the edited one.

I reproduced it locally rather than guessing. A pytest plugin quantizing datetime.now() to a 15.6 ms grid fails the strict form 9 times in 20 and passes the corrected form 20/20; the whole module is green under it. Worth recording for anyone doing this again: a frozen clock does not reproduce it. Freezing also freezes the setup rows, which changes the floor relationship and makes the strict assertion pass — the faithful emulation is quantization, not stasis. The source comment and session.md both said "stamps later"; both now say "never earlier, possibly identical", so the next reader does not re-derive this.

Backend Tests (Windows) (3) — not this PR's

It fails in test_push_branch_gate.py and test_security.py on shell-grammar shapes: 'git push origin @(main)' was not read as a wildcard shape, git push origin my-feature > >(tee log.txt), git-publish-push-bare missing from a rule set. This diff is five files — chat_regenerate.py, its test, session.md, learn-cron-dashboard.md, and the black baseline — and none of them is security.py or either of those suites. Both suites pass on this branch on Linux (test_push_branch_gate.py: 59 passed), so this is a Windows-only regression already on main, most plausibly from c5b83f22c ("scope the separator-run collapse to shell-grammar subjects") or 46c9d409c ("give a source body's command-line passes a command-line subject"). Flagging it rather than absorbing it — it will red every PR whose shard 3 draws those files until someone owns it.

The GPT 5.6 Review position is unchanged from my previous comment: it has returned to the one residual I have consistently escalated, and that needs a maintainer decision rather than another push.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from d90d510 to b9192a5 Compare September 5, 2026 08:43
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

b9192a5 — rebased onto current main, and the shard-3 red is main's, proven against a pristine checkout

Backend Tests (3.12, 3) and Backend Tests (Windows) (3): not this PR's

I chased this properly rather than asserting it, and the giveaway was that the failing line numbers do not exist in this branch. CI's pull_request checkout tests the PR merged with current main, and since this PR's previous base (5f5e3dec4) main has moved 52 commits, growing test/test_push_branch_gate.py from 675 to 1424 lines via:

So the failures at test_push_branch_gate.py:939/962/1077/1127/1207/1287 were main's own new tests running against main's own new security.py — neither of which this diff touches (it is five files: chat_regenerate.py, its test, session.md, learn-cron-dashboard.md, the black baseline).

Proof, on a pristine origin/main worktree at 6d1b51704 with nothing of mine in it:

FAILED test/test_push_branch_gate.py::TestUnrecognisedOptionsReadProtectively::test_extglob_patterns_are_wildcards_too
FAILED ...::test_a_heredoc_strip_operator_consumes_its_delimiter
FAILED ...::test_a_glued_redirection_keeps_the_precise_positional_identity
FAILED ...::test_a_glued_all_branches_flag_keeps_its_identity
FAILED ...::test_a_background_operator_reads_protectively
FAILED ...::test_every_bash_metacharacter_is_accounted_for
FAILED test/test_security.py::TestGitPublishSubshellGluing::test_a_redirect_is_skipped_not_treated_as_the_end_of_the_args
FAILED ...::test_path_qualified_feature_pushes_are_not_over_blocked
FAILED ...::test_quoted_operator_ref_names_stay_pushable
9 failed, 1971 passed, 1 skipped

The rebased branch produces the identical set and the identical counts — 9 failed, 1971 passed, 1 skipped. main is red on shard 3 right now and will red every PR whose shard draws those files; it needs an owner in the git-publish floor-tag / shell-payload extraction area, not a patch from here.

(Two red herrings worth recording so nobody re-walks them: running those two files together on either tree passes when the shard composition differs, and #8421's shard 3 is green — both are artifacts of pytest-split balancing by test count with no committed .test_durations, so which shard draws which file shifts with any test-count change. Neither means the failure is composition-caused; it is a straightforward main regression that only some shard layouts surface.)

This PR is now rebased onto 6d1b51704

Worth doing regardless: the base was 52 commits stale, so my local gates were validating a tree CI was not testing. Post-rebase, on the tree CI will actually see:

  • Gates clean: black formatting, subprocess-encoding, brand-name, harness-parity, changelog-history, docs-lint, scrub-lint (--no-history), flake8 src/kiro_crew test, mypy --platform linux src/kiro_crew (1293 files).
  • test_chat_regenerate_cov80.py + test_dashboard_chat_rewind.py + test_kiro_prerequisite.py + test_dashboard_chat.py + test_gateway_appkit_endpoints.py + test_persist_off_loop.py + test_dashboard_chat_pins.py: 1315 passed.

Backend Tests (Windows) (1), the one that genuinely was mine, went green on d90d510 after the clock-granularity fix. GPT 5.6 Review's position is unchanged from my earlier comment: it has returned to the single residual I have consistently escalated, which needs a maintainer decision rather than another push.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
bolichen97 added a commit that referenced this pull request Sep 5, 2026
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.
@bolichen97
bolichen97 force-pushed the fix/edit-resend-conversation-boundary branch from b9192a5 to 6e52ab7 Compare September 5, 2026 19:39
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Response to the two GPT 5.6 Review findings on b9192a5e, at head 6e52ab767.

chat_regenerate.py:831 — "Post-commit await can dispatch into a rebound session": correct, and fixed by withdrawing the addition.

The finding is exactly right. dispatch_commit was already True at that await, so only dispatch_ready.set() in the finally remained — a cron completion rebinding the slot during the await releases _run_chat against the new session. The suggested remedy (check the save result and re-run _commit_target_intact() before releasing dispatch) cannot rescue it: the live slot has already adopted the truncated window, so refusing there leaves a truncation with no turn.

So the extra save is gone rather than guarded. A carried row now 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, and what shipped rewind and #8421 both do. test_edit_resend_commit_keeps_a_row_injected_during_the_boundary now asserts the boundary writes exactly once (saved_windows == [["edited"]]) with the arrival still carried; its arrival-retention assertions are unchanged. test_edit_resend_skips_the_extra_persist_when_nothing_arrived is deleted because the code it described no longer exists, and the property it protected is now asserted in the harder case where something did arrive.

Worth recording: rewind has the identical exposure on main todaychat_rewind.py:537 sets dispatch_commit = True, :543-544 awaits _delete_orphan_kiro_session(...), and :548 then sets dispatch_ready. Named in the Pattern harvest; not fixed here.

chat_regenerate.py:502 — "Retryable refusals corrupt Mochi conversations": real, pre-existing, and not in this diff.

The client defect is at website/src/apps/mochi/src/renderer/ChatPanel.tsx:996-1014: it cuts messages and allHistory optimistically, then on !result?.ok calls api.sendMessage(text), so the edited text lands after the server's untouched suffix. Three checks a reviewer can run:

  1. Already reachable on origin/main. git show origin/main:src/kiro_crew/dashboard/chat_regenerate.pyapi_chat_slot_edit_resend answers 404 slot_not_found, the remote-bound 409, 400 invalid_json (twice), 400 content_required, 409 slot_running, 400 user_message_not_found, 400 index_not_user_message, 400 index_or_ts_required. Each is a !result?.ok, and the fallback corrupts on every one. git diff origin/main --name-only on this branch lists nothing under website/.
  2. This PR moves the server in the safe direction. main returned {"ok": true} after a failed save and dispatched anyway, so the client was told the edit landed and had nothing to recover from. A retryable refusal replaces a lie. The new codes do make a refusal more routine — a busy session is not a caller error — which is a fair reading; what they do not do is create the client bug.
  3. The fix is a frontend PR, and the repo's gates say so. The remedy is to snapshot both arrays before the cut, restore them on !result?.ok, put the composer back in edit mode with the text, and surface the failure through the existing apps.mochi.chat.send_failed banner exactly as sendText already does at ChatPanel.tsx:915-926. That touches website/src/apps/**, so Screenshot Evidence becomes required, and the change has a genuine visual delta (restored rows plus a banner) — the <!-- no-visual-delta --> waiver would be a false claim, so it needs a real capture of the Mochi failure path.

Mochi's bridge is the only consumer of this endpoint (api/client.ts:3036 exports editResend, panelBridge.ts:1473 wraps it, ChatPanel.tsx:1008 is the sole caller), so it is worth a follow-up rather than leaving it to be rediscovered. Happy to do that as its own PR with screenshots, or to fold it in here if a maintainer prefers that scope — folding a frontend UX change plus visual evidence into a backend endpoint PR is a scope call I should not make unilaterally.

I am deliberately not posting /ai-review override gpt: that is a repository writer's action, and this finding describes a real corruption path even though the code carrying it is not in this diff.

@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 5, 2026
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.
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Head 3783a5420 — CI status and the one remaining red.

All 59 other checks are green. The rebase onto current main (04d38a783) cleared Backend Tests (3.12, 3), Backend Tests (Windows) (3) and Coverage Gate with no code change, as expected: those nine failures were main's own push-arity regression, fixed upstream by c791f0f1d (#8712). All four Linux shards, all four Windows shards, Coverage Gate and PR Hygiene are green. PR Readiness reports exactly 1 blocking readiness item(s), which is GPT 5.6 Review.

The lane's finding changed between heads, so the body's "The one open finding" section is one round behind. At b9192a5e it raised the Mochi renderer defect (answered in the previous comment; still a real pre-existing bug worth a follow-up, and no longer flagged). At 3783a5420 it raised the boundary-contract residual instead — the same finding round nine raised under a different title:

BLOCKING — chat_regenerate.py:816 — A late rebind leaves the original transcript corrupted. if not _commit_target_intact(): — unbound cron slot → api_cron_to_chat rebinds after save snapshots routing → old transcript is truncated and misbound, then this returns 503 without dispatch. Fix: prevent routing changes across save-and-commit, or roll back the landed rewrite before returning.

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"):

  1. The dropped tail IS recoverable — the adjudicator's "recovery: none" is factually wrong. The rewrite archives before it truncates: chat_persistence.py:3312-3316 runs _archive_dropped_lines(state, history_key, old_lines, new_lines) under if rewrite and path.exists(), and an explicit-snapshot save always implies rewrite. That lands the dropped lines in archive/<key>.<YYYYMMDD-HHMMSS>.jsonl via history.py:850 _archive_lines(..., reason="compact"), keyed by the same history_key, with an atomic exclusive-create so two archives in one second cannot clobber each other. So on this path the authorized transcript holds exactly the truncation the user asked for, the discarded suffix is on disk beside it, and the file is internally consistent. What is genuinely lost is the dispatch: the edited turn never runs and the caller gets a retryable 503.
  2. This is main's shipped boundary shape, not this PR's invention. git show origin/main:src/kiro_crew/dashboard/chat_rewind.py makes the identical check in the identical position — line 514, if slot_history_key(slot) != expected_history_key: after saved and before _commit_live_state(), returning 503 rewind_slot_rebound at :524 — with a comment calling it load-bearing ("a late commit here would silently replace it … this loop-side check is the one that can"). regenerate and fork reach the same rewrite-save path. So the residual covers four endpoints, three of which carry it on main today.
  3. Committing instead is the defect this same lane blocked on in an earlier round. The only alternative to refusing here is to commit onto a slot a cron or workflow injection has since rebound — which is finding 4 in the PR body, raised by this lane and fixed by adding this very check. The two findings ask for opposite behaviour at the same line; only fencing the rebinding satisfies both.

Why neither suggested fix is a patch to this endpoint. "Prevent routing changes across save-and-commit" means api_cron_to_chat / cron_inject / workflow_inject taking a boundary lock they do not take today (workflow_inject.py:172 appends straight on the event loop) — a behaviour change for every injection consumer, and it has to cover rewind/regenerate/fork too or the hole simply moves. "Roll back the landed rewrite" needs an un-truncate that does not exist: save_slot_off_loop's expected_history_key guard refuses precisely because the slot no longer routes there, so a rollback would need a write that bypasses the guard the transaction depends on — and it would itself be a destructive write racing the dirty-slot flush, reopening the desync this PR closes.

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 edit-resend / rewind / regenerate / fork) before merging, or whether that lands separately — the shape I would recommend, since three of the four carry it on main today and this PR strictly improves the fourth. Relative to main, edit-resend previously had no rebind check at all on this path: it truncated, log-and-continued past a failed save, and dispatched the edited prompt against whatever the slot had been rebound to. This PR replaces that with an archived truncation plus a retryable refusal.

The finding is fenced (Anchor: residual/… always fences, and the adjudication returned UPHOLD-FENCED), so adjudication cannot clear it and the only path is a repository writer's /ai-review override gpt 3783a54208b3e124c5a97688a86a13ae9fd878ab: …. I am deliberately not posting that — waiving a security-class finding is a writer's action that requires independently verifying the rationale, and the three checks above are what I would want verified. If any of them is wrong I would rather be corrected than have the finding waived.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

/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.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 3783a54208b3e124c5a97688a86a13ae9fd878ab.

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.

This decision applies only to this commit. A new push requires a new judgment.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. The periodic flush cannot undo the rewrite. Routing through save_slot_off_loop(..., expected_history_key=..., best_effort=False) raises _metadata_persist_inflight for 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 bare to_thread would reopen #7838.
  4. 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.
  5. 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 no meta.mid) and carried onto both the window and _pending; total_messages advances by one; the question map is intersected tightest-wins. The id() keys are sound because every pre-await row stays referenced -- the prefix by prospective_slot.messages, the suffix by the live slot.messages.
  6. Nothing awaits between commit and dispatch release. Withdrawing the second guarded save rather than guarding it was the right call: dispatch_commit is already True there, and refusing after the live slot adopted the truncated window would leave a truncation with no turn.
  7. Cancellation is handled against an uninterruptible worker. The bounded re-shield drain reads the outcome off the settled task, so a second CancelledError (a BaseException) cannot walk away from a landed rewrite; giving up leaves the live slot untouched, which is the safe half.
  8. Silent desync is gone. log-and-continue becomes retryable 503/409 with machine-readable code fields 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants