fix(dashboard): stop rewind's commit undoing mid-boundary writes

[chenmingwei23]

Problem / Motivation

rewind and edit-resend are the two edit context-boundary endpoints, and they run the same transaction: freeze a prospective window, destroy the native conversation, rewrite persisted history, then adopt the prepared state on the live slot. Anything that writes to the live slot while those boundaries are pending is racing the commit. Two of those writes were being thrown away, and a third outcome was leaving no record. Measured at main 7dd090fb62e858cc1d752e404deae44913d48de1, and re-verified on the rebase onto 56f67aa43f00f9484c346a8d1669b39102a63c78.

1. A row that arrives mid-boundary is silently erased. rewind's commit did slot.messages = prospective_slot.messages -- a wholesale replace. A workflow or cron completion appends straight on the event loop without taking slot._lock (workflow_inject -> append_and_surface), so a row can land between the pre-await snapshot and the commit. The replace drops it, and the rewrite cannot put it back because a rewrite deliberately skips the cross-process-append scan (collect_foreign=not rewrite). The same loss happens one field over in slot._pending, which is what an open client's stream reader drains.

2. A row already delivered mid-boundary is queued again. _pending is the un-drained buffer an open client's stream reader empties, and drain() does slot._pending.clear() on the live list. The commit did slot._pending = prospective_slot._pending + arrived_pending, and that frozen copy still holds every row the drain delivered -- so a client that read mid-boundary is handed the same rows a second time.

3. An answered question card comes back. Answering a card mutates the live dict: clear_question_pending pops the id from slot._question_pending. The commit did slot._question_pending = prospective_slot._question_pending, which replaces that whole dict with a copy frozen before any await -- so the pop is undone. A blocking card is the shape that reaches this, because an append never retires one: state.py retires on _QUESTION_RETIRING_ROLES = {"user", "nudge"} and only if not rec.get("blocking"). The user sees a card asking for an answer they already gave, and the slot reports needs_input against a round-trip that has completed.

4. A failure after the native discard left no audit record. Once the native session is torn down it is unrecoverable. SEL carried this endpoint's denials and its successful commits, but not this outcome -- so in the trail, "destroyed the user's conversation context and then failed" was indistinguishable from a denial that touched nothing. True of both endpoints, and true of seven exits rather than the four that are obvious: the four 503 returns plus a cancellation landing on any of the three awaits that run with destruction already done. None of those three is reachable by an except Exception, because CancelledError derives from BaseException, and they are the worst of the seven because the client is never told at all -- the cancellation propagates instead of a response, so SEL is the only place the outcome can be attributed from.

Why it matters

(1) is silent data loss in a user-visible path: the arrived row is gone from the window, and because it never returns to the window no later flush re-persists it either. (2) is the mirror -- nothing is lost, something is duplicated, and the client sees a row it has already been shown. (3) is worse than either, because it strands consent. A blocking card is a question that gated what happened next. Bringing it back either asks the user twice for one decision, or leaves the session waiting on a question nobody knows is pending -- with the answer channel already gone, no later round-trip can retire it. (4) is the one that cannot be recovered after the fact: the destruction is irreversible and, without a record, unattributable.

What changed (motivation -> approach -> change)

The four defects above lose to one thing: the commit replaces a whole container with one frozen before the boundary. So there is one rule -- a commit edits the live container by the delta the transaction owns; it never assigns a frozen copy over it -- and it is now applied to all three fields, in both endpoints.

• Arrival retention. pre_await_row_ids / pre_await_pending_ids are captured beside the existing pre_await_disk_older_count, in the same synchronous stretch as the snapshot. The window becomes prospective_slot.messages + arrived_rows. Identity rather than a length, because an append at the window cap trims the front and a positional slice would re-adopt trimmed rows or miss the arrived one. Arrivals go after the prospective window: monotonic_transcript_ts only ever moves a row forward, so an arrived row must never sort before the edit, and on a coarse clock (Windows ticks in ~15.6 ms steps) both appends can read the same instant -- list order is what separates that tie.

  Identity by id() needs the objects kept alive, and that is a correctness requirement rather than a detail. An id() is an integer that says nothing about lifetime, and at a low-index edit nothing else pins the leading rows -- at index 0 the prospective copy is empty. A cap trim during the awaits would free a leading row, CPython could hand its address to a newly appended arrival, and the commit would read that arrival as "not new" and drop it. So the snapshot retains the lists (pre_await_rows, pre_await_pending) and derives the id sets from them. meta.mid is not usable as the identity instead: append skips it for restored rows (mint_mid=False) and for the wire-only roles, so it is not present on every row.

• _pending is edited, not replaced. A pre-await row survives only if it is still live, so a row the drain already delivered is dropped instead of requeued; the edit's own row and anything that arrived are kept unconditionally. Order is unchanged -- pre-await rows, then the edit, then arrivals -- which is the same monotonic argument as the window.

  delivered_pending_ids = pre_await_pending_ids - {id(row) for row in slot._pending}
  slot._pending[:] = [
      row
      for row in prospective_slot._pending + arrived_pending
      if id(row) not in delivered_pending_ids
  ]

• The answer is made authoritative by construction. The commit stops assigning _question_pending at all. It deletes exactly the ids the edit retired, in place, and announces only the ids it actually removed:

  announce_retired = [
      question_id
      for question_id in retired_question_ids
      if slot._question_pending.pop(question_id, None) is not None
  ]

  A census of every _question_pending write site in src/kiro_crew returns eight, and exactly one adds an id: mark_pending. So once an answer has popped a card, nothing in the commit can bring it back -- the commit no longer carries a competing copy of the container, which means there is no reconciliation for a later edit to this function to forget. It also leaves a card that arrived mid-boundary alone. edit-resend used to keep a prospective INTERSECT live set here, which retired an answered card correctly but erased an arrived one, because the intersection is keyed on the frozen copy. That set is deleted and both endpoints now retire in place.

flowchart LR
  subgraph Before
    A1["answer pops id from live dict"]:::ctx --> B1["commit assigns frozen copy"]:::removed --> C1["card is back, unanswerable"]:::removed
  end
  subgraph After
    A2["answer pops id from live dict"]:::ctx --> B2["commit pops only the edit's ids"]:::added --> C2["card stays answered"]:::added
  end
  classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px
  classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3
  classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E
  linkStyle 0,1 stroke:#DC2626,stroke-dasharray:4 3
  linkStyle 2,3 stroke:#16A34A,stroke-width:2px

Loading

Legend: green = added by this change, red = removed by it, blue = unchanged.

The commit used to hand the slot a dict copied before the answer existed; now it edits the dict the answer wrote to, so the answer wins.

• SEL on post-discard failure. A local _sel_native_destroyed(reason) in each endpoint, called before every exit that leaves destroyed context uncommitted. outcome="error", resources carrying slot=<key>,native_cleared=1, and a distinct reason per site. The spelling is not invented: outcome="error" is settled 98 times across src/kiro_crew/dashboard/.

  Every exit in the destroyed-context window, and what native_cleared ends up as. The window opens at the first instruction inside discard_conversation that changes state and closes at the commit. Both endpoints are identical; the reasons are prefixed per endpoint in the log.

  #	exit	what is known	native_cleared
  1	discard cancelled, drain settles with a raise	nothing either way	unknown
  2	discard cancelled, drain settles True	it happened	1
  3	discard cancelled, drain settles False	provably nothing -- skip_if_busy returns before the pop	no record
  4	discard cancelled, never settles within 8 passes	nothing either way	unknown
  5	discard raises ordinarily, 503 *_prepare_failed	nothing either way -- the raise can land on either side of the pop	unknown
  6	discard returns False, 409 *_session_busy	provably nothing -- refusal precedes the pop	no record
  7	aflush() cancelled	it happened -- the discard already returned True	1
  8	aflush() raises, 503 *_prepare_failed	it happened	1
  9	save cancelled and the rewrite did not land	it happened	1
  10	save raises, 503 *_save_failed	it happened	1
  11	save refuses, 503 *_save_failed	it happened	1
  12	commit target moved, 503 *_slot_rebound	it happened	1
  13	success -- the commit runs	it happened and was committed	no record; the success is audited separately

  Rows 3 and 6 are where unknown would be the wrong word, and they are the reason the field is three-valued rather than two: discard_conversation evaluates if skip_if_busy and ... semaphore.locked(): return False before owner._sessions.pop(...), so a refusal is positive knowledge that nothing was torn down. Writing unknown there would discard a fact. Rows 1, 4 and 5 are the opposite: genuinely nothing is known, and the field says so instead of picking whichever of the two facts is cheaper. Rows 5 and 12's ordinary-failure siblings were the last two exits to write nothing at all; both now write, which closes the enumeration.

  The broad except Exception around the drain is deliberate and the code says why. provider.shutdown() is provider transport and its failure modes are not enumerable from a caller, and letting an arbitrary error escape a CancelledError handler would replace the client's cancellation with an unrelated exception on a teardown path. It narrows where it matters -- CancelledError, KeyboardInterrupt and SystemExit still surface -- and the breadth is harmless now that it no longer manufactures a false state.

  The drain is a bounded re-shield, not a single await, because that await is itself a cancellation point: one further cancel would abandon it and lose the record. Eight passes, matching _SAVE_DRAIN_ATTEMPTS, whose comment in this file names the same hazard. The settled task is inspected rather than the await's value.

  One direction stays open and it is a callee's contract, not a branch: discard_conversation never reports whether it passed its own destruction point, which is why rows 1, 4 and 5 can only say unknown rather than the truth. Folded into Both edit-boundary endpoints validate slot identity only after the destructive rewrite has landed #8988 with a census of all eight call sites.

  Two other rewrite-save callers are deliberately not closed: regenerate and fork have the same injected-row exposure and are untouched by this change. The spec now says so, so the fixed subset -- exactly rewind and edit-resend -- is discoverable rather than inferred.

Disclosed: all three cancellation paths were added in review, and each reviewer was right. The first revision covered only the four 503 returns. Opus flagged the history save. GPT then flagged aflush(). On the next round GPT flagged the discard itself, and its adjudication is what corrected the reasoning here: I had argued in a disposition that a cancelled discard destroys nothing and so should not be recorded, which is wrong -- session_lifecycle.py pops the session and calls clear_sid before three further awaits, so destruction is already true while those run. The table above replaces that claim.

Subtracted in review: two extra commit-time identity axes, and the measurement is why. An earlier revision added a _commit_target_intact() predicate to rewind carrying three axes -- the routing key main already checked, plus state._slots.get(name) is slot and slot.task is task. A review finding on the object-identity axis is correct and its consequence is larger than the axis: every one of those axes is evaluated after await asyncio.shield(save_task) has already rewritten the shared history file, so no axis at that point in the sequence can protect the file. A same-name close-and-recreate keeps the same history key, the save's routing guard passes it, and the replacement conversation's transcript is truncated with no archive and no recovery -- the 503 tells the client to retry the rewind, not to restore the other conversation. Adding or dropping a commit-time axis does not move that by a single instruction. Closing it means pre-write validation inside _save_slot_to_history, which is a shared write path serving both endpoints and a second, larger purpose than this change. So the two added axes are dropped and main's own commit-time routing check is restored verbatim at both call sites. Nothing that exists on main is removed -- this declines to add, it does not delete a protection. The corruption is filed as #8988 with the remedy shape.

The sibling is fixed rather than declared, because the reviewers were right that a declaration was not enough. An earlier revision fixed rewind only and named edit-resend's intersect as out of scope. Design Review and First Principles Review each landed on the same objection from opposite ends: the PR derives a rule and then leaves a live violation of it, in a file the diff already opens, when the fix is the same six lines. So surviving_questions is deleted, edit-resend retires in place, and the two endpoints now spell one job one way instead of two. This makes the diff wider by one function and the concept narrower by one special case.

slot.total_messages is also not taken. edit-resend increments this lifetime counter for the edited row; rewind does not, and prospective_slot.append bumps only the shallow copy's int. That looks like a real defect, is not reachable from any gap above, and would not be tested by anything here.

Tests

Thirteen new tests in test/test_dashboard_chat_rewind.py and two in test/test_chat_regenerate_cov80.py, all driving the real endpoint through TestClient with the racing event injected inside the awaited boundary via discard_conversation's side effect -- the harness both files already use.

Test	Locks in
..._keeps_a_row_that_arrived_during_the_boundaries	the arrived row survives, sits after the edited row, and the truncated suffix is not resurrected
..._keeps_a_pending_row_that_arrived_during_the_boundaries	the same row survives in _pending
..._keeps_a_blocking_card_answered_during_the_boundaries	a blocking card answered inside the boundary stays retired through the commit
..._does_not_requeue_a_row_drained_during_the_boundaries	a row the client already drained is not handed to it twice, while the edit's own row still arrives
..._records_a_sel_event_when_the_native_context_is_destroyed	one SEL record, outcome="error", native_cleared=1, naming the slot and the reason
..._cancelled_without_a_landed_rewrite_still_records_the_destruction	the post-save cancellation exit records the destruction, asserting the no-commit precondition so it cannot pass via the landed branch
..._retains_the_pre_await_rows_so_their_ids_cannot_recycle	a row trimmed mid-boundary is still held by a list, asked of gc.get_referrers because rows are plain dicts and cannot be weak-referenced, plus the behavioural half that the arrival still lands
..._cancelled_on_the_sid_flush_still_records_the_destruction	the aflush cancellation records it too, asserting the discard returned True first so the destruction really happened
..._cancelled_inside_the_discard_still_records_the_destruction	a cancellation delivered while the discard's own awaits are in flight records discard_cancelled; the handler task is captured on its own stack so the cancellation lands deterministically
..._cancelled_discard_that_refused_records_no_destruction	a skip_if_busy refusal destroyed nothing and must leave no record, so the drain cannot simply record on every cancellation
..._cancelled_discard_that_raises_records_an_unknown_outcome	a drain that raises records exactly one native_cleared=unknown and zero native_cleared=1, so it cannot be satisfied by over-claiming in either direction
..._second_cancellation_on_the_drain_still_records	cancelling the handler twice -- once onto the shielded await, once onto the drain -- still leaves exactly one record; asserts the second cancel landed so it cannot pass on the first alone
..._discard_failure_records_an_unknown_outcome	an ordinary discard raise records exactly one native_cleared=unknown and zero native_cleared=1, and leaves the original branch intact
test_edit_resend_commit_keeps_a_blocking_card_answered_meanwhile	the sibling endpoint keeps an answered card retired
test_edit_resend_commit_keeps_a_card_that_arrived_meanwhile	the sibling endpoint keeps a card raised during its own boundary -- the erasure the deleted intersect caused

Each test asserts its own precondition so it cannot pass vacuously: the arrival tests append through append_and_surface, the real door a workflow completion uses; the drain test asserts the drain delivered something and seeds the buffer through that same door, since the fixture drains; and the card tests assert the clear returned True and use a blocking card, which is the axis-isolating condition -- a non-blocking card would be retired by the edit's own user append and the test would never touch the race.

Proved rather than asserted: prepare-pr's prove.py --base main reverts the production hunks in a throwaway worktree while keeping the test hunks, re-runs the changed test files, and reports PROVEN: an assertion failed with the bug reintroduced. Individual fixes were also mutation-checked by hand -- restoring destroyed = False, and dropping the retained row list -- each reddening its own test on its own assertion message, which is what separates a test from a decoration.

An existing test is the control that the deleted intersect lost no coverage: test_edit_resend_commit_does_not_resurrect_a_card_retired_meanwhile pins the case the intersect was written for -- an arrived user row retiring every non-blocking card, with no second announcement from the commit -- and it passes unchanged against the in-place retire.

Gates run individually rather than through a wrapper, on the changed files: black --target-version py310 --check clean, isort --check-only clean, flake8 clean, mypy src/kiro_crew/dashboard/chat_rewind.py reporting only two pre-existing errors in src/kiro_crew/transcribe.py:1337 reached transitively (identical count on an unmodified sibling, so not from this diff). Suites at -n0, one file at a time: test_dashboard_chat_rewind.py 51 passed (38 before), test_chat_regenerate_cov80.py 66 (64 before), test_chat_regenerate_refusal_codes.py 3, plus every spec that reads the symbols this diff touches -- test_slot_needs_input_status.py 30, test_ask_question_roundtrip.py 63, test_chat_slot_facade_contract.py 5, test_steer_requeue.py 52.

Manual verification

N/A -- unit coverage sufficient. All four defects are concurrency-shaped and reachable only by interleaving an event with an awaited boundary, which the tests drive directly and deterministically; the SEL record is asserted from the recorded call rather than from a log.

Related Issues

Closes #8975
Refs #8419
Refs #8988

Pattern harvest

The defect class is a commit that adopts prepared state by replacing a container rather than editing it. Every field lost this way lost the same way: messages, _pending, and _question_pending were each assigned a copy frozen before an await, so any in-place write that landed during the await was discarded without a trace. Replacement is the easy thing to write and it is silently destructive exactly in proportion to how long the boundary is open. The tell is a plain assignment of one object's field to another object's field inside a commit that follows an await.

Rule candidate: review-prompt
Pattern: inside a post-await commit, live.field = frozen_copy.field discards every concurrent in-place write to live.field -- prefer editing the live container by the delta the transaction owns.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) -- docs/system-specs/modules/session.md reconciled: its "Edit rewind context boundary" bullets described the pre-fix behaviour
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

All claims verified against the diff and tests. I have what I need for the design review.

Design-Verdict: CONCERNS

Sound delta-commit fix, but the intricate discard-drain/SEL machinery is now duplicated verbatim across both endpoints — the exact sibling-divergence class this PR just paid for.

Watch

• ~130 lines of delicate, near-identical logic (_sel_native_destroyed, the shielded discard, the bounded re-shield drain, the three-valued outcome inspection) are copy-pasted between chat_rewind.py and chat_regenerate.py, differing only in prefix strings. The PR's own history shows the cost of two spellings of one job (the surviving_questions intersect it deletes was sibling drift), and Both edit-boundary endpoints validate slot identity only after the destructive rewrite has landed #8988's discard-contract work will have to patch both copies in lockstep or one endpoint silently regresses. A shared boundary helper serving both endpoints is the durable shape; if deferred, name it in Both edit-boundary endpoints validate slot identity only after the destructive rewrite has landed #8988 explicitly.
• The same silent injected-row loss stays live in regenerate and fork ("deliberately still open" per the spec update). Documented, but it is the identical user-visible data loss defect (1) fixes — make sure a tracked issue owns closing them, not just a spec sentence.

[DESIGN-REVIEWED] eb3ea26

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of eb3ea26edc01142db8291181c5cbcbb999159854 — 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 claims are now verified. The description's counted claims hold up (one _question_pending adder at interaction_coordinator.py:218; pre-existing SEL denial/success logs in both endpoints; regenerate/fork deferral declared in the session.md hunk). Two genuine premise risks remain: the shield changes teardown semantics while its comment denies it, and reset-conversation carries the same unshielded discard.

First-Principles-Verdict: CONCERNS

The shield quietly turns an abandonable native teardown into run-to-completion while its own comment denies any change, and reset-conversation keeps both gaps.

What this change ships

Intent: keep an edit-boundary commit from erasing concurrent writes and leave an audit record when it destroys context and fails — a FIX (four declared defects).

1. A workflow/cron row landing during rewind survives the commit — justified
2. That arrived row also survives in the open client's stream buffer — justified
3. A row the client already drained is no longer delivered twice — justified
4. A blocking card answered mid-boundary stays answered (retire-in-place) — justified
5. A card raised mid-boundary survives edit-resend; the intersection set is deleted — justified, net deletion
6. Pre-await rows retained so a recycled id() cannot drop an arrival — justified (CPython lifetime rule)
7. Seven destroyed-without-commit exits now leave a SEL record (native_cleared three-valued) — justified, audit attribution
8. Client disconnect no longer cancels the teardown mid-flight (shield + 8-pass drain) — undeclared behavior change
9. Rewind announces only retirements the commit actually performed — justified, follows from 4
10. session.md updated same-commit; regenerate/fork deferral declared — mandated by AGENTS.md

Watch

• Item 8's framing is contradicted by the diff: the comment says "The shield does not make the teardown slower -- it was always going to run to completion," but the removed code was a bare await state.sessions.discard_conversation(...), so a disconnect previously threw CancelledError into the teardown and abandoned provider.shutdown()/release_subagent_runtime mid-flight. The shield is a real second fix (resource release under disconnect) shipping as "no change."
• One unfixed sibling of that same cause: chat_handlers.py:4650 (reset-conversation) still bare-awaits discard_conversation in a request handler (grepped discard_conversation; the only other handler-level caller), so a cancel there still abandons the teardown and can destroy with no "completed" SEL record. The session.md deferral hunk names regenerate/fork for row loss but not this.

[FIRST-PRINCIPLES-REVIEWED] eb3ea26

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I have enough to complete the validation. Let me verify one detail about how the double-cancel scenario is treated as real in this file's discard drain (already confirmed: _DISCARD_DRAIN_ATTEMPTS loop at lines 441-449 and regenerate's _SAVE_DRAIN_ATTEMPTS loop at 902-913 both explicitly defend against a second cancel, while rewind's save drain at 719 does not).

My analysis:

• Candidate 1: Grounded and real. In chat_rewind.py, the discard drain (added by this PR) and chat_regenerate.py's save drain (902-913) both use a bounded re-shield loop precisely because a single await on the task is defeated by one more cancellation. But rewind's own save drain at line 719 is a single unshielded await save_task. A second CancelledError there propagates past the if landed…/else block, skipping the PR's new _sel_native_destroyed("request_cancelled") (line 738) and _commit_live_state(). However, the single-await line is pre-existing (context, not + in the diff); the diff neighbors it. The desync is inherited, and the missing-record-on-double-cancel is an incomplete improvement, not a regression with irreversible loss. Fix requires editing the pre-existing drain block to mirror regenerate → demotes to FINDING.

• Candidate 2: Log/resource hygiene ("Task exception was never retrieved"), speculative ("task later completes with an exception"), no functional wrong outcome. Below the 80 bar. Drop.

A second cancellation on rewind's history-save drain (chat_rewind.py:719) is not re-shielded like the discard drain above it or chat_regenerate's save drain, so the PR's own request_cancelled audit record is lost and live state desyncs from the landed rewrite.

FINDING — src/kiro_crew/dashboard/chat_rewind.py:719 — the cancellation-drain uses a single landed = bool(await save_task), itself a cancellation point; a second cancel (gateway shutdown reaching a handler already unwinding from a client disconnect) raises CancelledError past the except Exception, skipping both _commit_live_state() and the new _sel_native_destroyed("request_cancelled") at line 738 — the exact double-cancel gap the sibling drains (_DISCARD_DRAIN_ATTEMPTS above, _SAVE_DRAIN_ATTEMPTS in chat_regenerate.py:902) exist to close → Fix: replace the single await save_task with the same bounded for _ in range(_SAVE_DRAIN_ATTEMPTS): await asyncio.shield(save_task) loop and read the outcome off the settled task, mirroring chat_regenerate.py:902-913.

[OPUS-REVIEWED] eb3ea26

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] eb3ea26

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

[chenmingwei23]

Disposition of the GPT 5.6 blocking finding (F1, chat_rewind.py:455)

The hazard is real and I am not disputing it. A close-and-recreate under the same name keeps
the same history key, the save's own guards do not test slot-object identity, and the truncating
write therefore lands before _commit_target_intact() can refuse the commit. The 503 is honest
about the commit and silent about the write.

What the measurement adds is that this change did not introduce it, and improves it. All four
facts read at main 0d65dc969d420e26f409a55a14c8451409521321:

1. chat_persistence.py contains zero occurrences of _slots.get -- the save has no notion
   of which object owns a name. Its only opt-in guards are expected_history_key (:2683) and
   expected_disk_older_count (:2639), and a same-name recreate changes neither. The reviewer is
   correct on this point.
2. edit-resend on main has the identical ordering: save_slot_off_loop( :728,
   shield(save_task) :737, _commit_target_intact() :773 and :816 -- both after the write. The
   hazard is a property of the shared transaction shape, already merged and reviewed in the
   sibling, not of this port.
3. On main, rewind reaches the same end state and reports success. Write at :495-505, then
   slot_history_key(slot) != expected_history_key alone at :559, which a same-name recreate
   passes. So main truncates the file and returns 200. This PR turns that into a 503: same
   input, same write, strictly better outcome, plus a SEL record (commit_target_moved,
   native_cleared=1) that main does not emit at all. The guard this finding names is what made
   the hazard visible.
4. The harm does reach another conversation -- slot_history_key is documented as "the file its
   conversation is stored in" -- and there is no recovery: _archive_dropped_lines (:2562)
   archives the OLD slot's dropped tail, not the replacement's overwritten rows.

Why the prescribed remedy does not belong in this PR. "Validate slot and reservation identity
within the serialized save before writing" needs a new opt-in guard on _save_slot_to_history, a
shared function whose other callers -- the periodic dirty-slot flush among them -- legitimately
write for slots that may be detached; an unconditional check changes their behaviour, so it has to
be a parameter, which is a contract change to the persistence module. It must also land in both
endpoints or the two shapes diverge again, which is the specific thing #8419 exists to prevent. And
the save runs on a worker thread while slot._lock is an asyncio.Lock, so the fix cannot be a
lock -- it has to be another value comparison threaded through, like the two guards already there.

Filed as its own issue against main rather than folded in here: #8988, with the ordering table, the
absent-guard measurement, the shared-file property, the missing archive path and two candidate
remedies. Filed against main specifically because the same shape is already shipped in
edit-resend, where nothing is watching it.

I am not asking for an override and have not requested one. The finding is security-class and
withheld from adjudication, which is exactly where an override would be least defensible: it would
score a real hazard as answered on the strength of an argument about scope. The blocking verdict
stands until a maintainer decides whether this PR should grow to carry the shared-save guard or
whether #8988 owns it.

[chenmingwei23]

Status: blocking finding accepted, remedy tracked elsewhere

The GPT 5.6 blocking finding (F1, chat_rewind.py:455) is accepted as real. A same-name
close-and-recreate keeps the same history key, the save applies no slot-object identity guard, and
the truncating write therefore lands before the commit-side check can refuse. There is no recovery
path for the replacement conversation's overwritten rows.

It predates this change, and it is present identically on the edit-resend path on main:
chat_regenerate.py dispatches its save at :728, awaits it at :737, and calls
_commit_target_intact() at :773 and :816 -- all after the write. It is a property of the shared
transaction shape rather than of this port.

The remedy is tracked in #8988 rather than here, because it requires a new opt-in identity
parameter on the shared _save_slot_to_history -- other callers, the periodic dirty-slot flush
among them, legitimately write for slots that may be detached, so an unconditional check would
change their behaviour -- plus a matching change in both endpoints so the two shapes do not diverge
again.

This PR is therefore left as it stands, for a human to decide between holding it and landing it. I
am not arguing the finding down and I am not claiming this PR is unaffected by it.

[chenmingwei23]

self-added: yes
mechanism: none added this round -- the round REMOVED _commit_target_intact and restored main's inline routing check at both commit sites.

• replacement guard runs after the destructive rewrite span=7cc321177e08

Fixed by removal, and ruled at class level so it covers this finding wherever
it moves. _commit_target_intact was added by THIS PR -- every line of it is
a + against main -- and it is gone at 2e2ec1f
along with both axes it introduced.
The class: EVERY commit-time identity axis in this handler is evaluated after
await asyncio.shield(save_task) has already rewritten the shared history
file, so no axis at that point can protect the file. That applies verbatim to
the reservation axis and to any future third one, not just to the flagged line.
Main already guarded both commit sites on
slot_history_key(slot) == expected_history_key, and that check is restored
verbatim -- nothing present on main is removed by this round.
The corruption the finding describes therefore pre-exists this PR and is
unmoved by it in either direction. Pre-write validation belongs INSIDE
_save_slot_to_history, beside the two optional guards it already carries
(expected_disk_older_count, expected_history_key), both of which refuse
with return False and write nothing. That is a shared write path serving
both edit-boundary endpoints and a larger purpose than this PR; filed as
#8988 with the remedy shape and the incarnation-vs-name caveat.

The subtraction was the maintainer's ruling after both candidates were costed: repair the mechanism (thread identity into the shared save, about twenty lines across three files) versus remove it. Removal won because the disk clobber happens before either added axis in both worlds, so those axes protect live state only and the prescribed fix is a new data-safety mechanism in a shared write path rather than a repair of this one. The two tests that asserted the removed axes were deleted with it, and the import they alone used was dropped.

[chenmingwei23]

self-added: yes
mechanism: native_cleared is now three-valued (1 / absent / unknown); the drain records discard_cancelled_outcome_unknown and logs the cause instead of assuming nothing was destroyed.

• post-clear discard failures suppress destruction handling in both edit boundaries span=814c72439d3e

Fixed in both endpoints. The finding is correct and the defect was mine, added in
the round that introduced this drain, so it is worth naming precisely rather than
just patched: except Exception: destroyed = False ASSERTED A FACT IT DID NOT
HAVE. The drained call raised, so the truth was "undetermined", and the code
wrote "not destroyed" -- which then suppressed the audit event on the one path
the audit exists for.
The root cause was the TYPE, not the branch. A boolean has room for two answers
and the teardown has three: it happened, it refused and destroyed nothing, or the
check that would have told us raised. So native_cleared is now three-valued and
the third writes unknown, with the exception logged at warning rather than
swallowed. Nothing claims a destruction it cannot prove, and nothing goes silent
on one it cannot rule out.
Class-level, and this is the rule I would want applied to the next finding of
this shape anywhere in these handlers: a boolean that has to represent an
unknown will be made to lie, so widen the field instead of choosing which lie is
cheaper. An audit that records every determinate outcome and goes quiet on the
indeterminate one is worse than no audit, because its silence reads as nothing to
report.
On narrowing the catch, since the remedy invites it: it stays Exception
deliberately and the code says why. provider.shutdown() is provider transport
and its failure modes are not enumerable from a caller, and letting an arbitrary
error escape a CancelledError handler would REPLACE the client's cancellation
with an unrelated exception on a teardown path. It does narrow where it matters --
Exception leaves CancelledError, KeyboardInterrupt and SystemExit free to
surface -- and the broad catch is now harmless because it no longer manufactures a
false state.
Pinned by ..._cancelled_discard_that_raises_records_an_unknown_outcome, which
asserts both halves: exactly one native_cleared=unknown record, and ZERO
native_cleared=1 records, so a future change cannot satisfy it by over-claiming.
Restoring destroyed = False turns it red on its own message.

[chenmingwei23]

• items 7-8 fix a different harm than the title, are half the diff, and a two-commit split is the natural shape

Needs a decision, and I am putting it to the maintainer rather than choosing
unilaterally, because the cost is not in the split itself.
The observation is correct and I am not arguing it. The title names the commit's
writes; the shield and the SEL audit fix attributability, which is a different
harm, and a git log reader sees only the subject line. AGENTS.md allows two
commits, so the shape is available and would make both purposes visible where the
description cannot reach.
What it costs: splitting rewrites history, which is a new head, which re-runs all
64 lanes and re-arms all five review lanes on a diff that has just converged after
six rounds. The content would be byte-identical -- this buys log legibility, not
correctness -- and the same push also discards the current green board.
The question, plainly: is log legibility worth one more full CI round on an
otherwise converged diff, or does the description carry it? I will split on a
word and it is a mechanical change (git reset --soft onto the base, then two
commits: the container/delta commit, then the audit commit).
Class-level note for whichever way it goes: the reason the two halves travelled
together is that the SEL work was one of the three gaps the originating issue
named, so it was round-0 intent rather than something that accreted during review.
A PR that fixes several gaps of one issue will keep hitting this, and the honest
fix is to split at OPEN time by gap, not at round six by reviewer request.

[chenmingwei23]

• the audit fix's root cause has 6 sibling discard_conversation sites this change leaves unrecorded

Answered by counting the consumers rather than by claiming context exempts them,
and the count does not come out uniformly in my favour -- one sibling has the same
exposure and I am naming it instead of arguing it away.
I resolved the enclosing function of every call site. chat_handlers.py:4650 is
inside async def api_chat_slot_reset_conversation(request: web.Request): a
client-cancellable HTTP handler, the same shape as the two endpoints this PR
fixes, so a browser disconnect can land on its bare await after irreversible
teardown exactly as it could here. That one is a real, unfixed sibling.
The others differ in the precondition rather than in importance:
chat_runner.py:4009 (_consume_pending_reset) is runner-internal, and the
channel sites (slack/handler.py:2384 _handle_compact_command,
telegram/transport_dispatch.py:3220 _handle_compact, and the Discord path) are
driven by inbound events with no HTTP client that can abort mid-await. They can
still be cancelled at shutdown, so the exposure is narrower, not absent.
Class-level, and it is the same conclusion three rounds of this PR reached from
other directions: this is not N call sites each needing a guard, it is ONE missing
callee contract. Every fix at a caller re-derives whether discard_conversation
passed its own destruction point, which the caller cannot know -- so a per-caller
guard would be wrong in the same way six times. #8988 owns that contract and now
carries this analysis.
Not fixed here on purpose: extending to api_chat_slot_reset_conversation means a
third endpoint in a PR whose scope is already the subject of the sibling watch item
above, and it would ship a fourth copy of a mechanism the reviewer has just asked
me to stop duplicating.

[chenmingwei23]

self-added: yes
mechanism: bounded re-shield drain (_DISCARD_DRAIN_ATTEMPTS / _SAVE_DRAIN_ATTEMPTS, 8 passes) around the discard in both endpoints, then the settled task is inspected.

• a second cancellation at await discard_task bypasses SEL after teardown span=efa9be2cb212

Fixed in both endpoints, and fixed rather than deferred as advisory because the
remedy already exists in this file. chat_regenerate.py bounds its
history-rewrite drain with _SAVE_DRAIN_ATTEMPTS = 8, and its comment names this
exact hazard: each retry "absorbs ONE further cancellation (a gateway shutdown
landing on a handler already unwinding from a client disconnect)". My drain was a
single await, which is defeated by exactly one more cancel -- so this was my
mechanism being LESS complete than its sibling in the same function, not new
scope.
The drain now re-shields up to the bound, breaks as soon as the task is done, then
inspects the settled task rather than trusting the await's return: an exception
records the undetermined outcome, a True result records the destruction, a refusal
records nothing, and never settling within the bound also records undetermined.
Class-level, and it generalises past this await: an await inside a
CancelledError handler is itself cancellable, so any single-await drain in a
cancellation path is one cancel away from losing whatever it was draining for.
Where the outcome is irreversible, the drain must be bounded-retried and the TASK
inspected, not the await's value.
Pinned by ..._second_cancellation_on_the_drain_still_records, which cancels the
handler twice -- once onto the shielded await, once onto the drain -- and asserts
its own precondition that the second cancel landed, so it cannot pass by only
exercising the first. Replacing the loop with a single await turns it red on its
own message.
Scope note, because the same shape exists one file over and I am not touching it:
chat_rewind's history-rewrite drain is a single await save_task on main and
stays that way. That asymmetry is pre-existing, unrelated to the containers this
PR fixes, and belongs to whoever reconciles the two save paths.

[chenmingwei23]

self-added: no
mechanism: the ordinary discard-failure branch in both endpoints now records discard_failed_outcome_unknown with native_cleared="unknown"; the full exit enumeration is in the PR body.

• normal discard failures bypass destruction auditing in both edit boundaries span=7cc321177e08

Fixed in both endpoints, and this is the same defect as the cancellation one on
the branch I had not touched -- it predates the shield work rather than being
created by it, so it is completing the mechanism rather than repairing a repair.
Rather than answer this exit alone, here is EVERY exit in the destroyed-context
window with what the field ends up as, so a further instance of this class inside
these two handlers cannot exist:
1 discard cancelled + drain raises -> unknown. 2 drain settles True -> 1.
3 drain settles False -> no record. 4 drain never settles in 8 -> unknown.
5 discard raises ordinarily -> unknown (THIS FIX). 6 discard returns False,
409 -> no record. 7 aflush cancelled -> 1. 8 aflush raises -> 1. 9 save
cancelled without landing -> 1. 10 save raises -> 1. 11 save refuses -> 1.
12 commit target moved -> 1. 13 success -> committed, audited separately.
Two of those rows are where unknown would be the WRONG value, and they decide
the field's shape: discard_conversation evaluates
if skip_if_busy and ... semaphore.locked(): return False BEFORE
owner._sessions.pop(...), so rows 3 and 6 are positive knowledge that nothing was
torn down. Writing unknown there would discard a fact. Rows 1, 4 and 5 are the
reverse, and the field says so rather than picking whichever fact is cheaper.
Class-level: the rule is that this field never asserts what the code does not
know, in either direction -- not a false destruction and not a false absence. Every
exit above is now classified by what is KNOWN at it, so the enumeration is the
invariant and no individual branch is load-bearing.
Pinned by ..._discard_failure_records_an_unknown_outcome, asserting exactly one
native_cleared=unknown AND zero native_cleared=1, plus that the 503 leaves the
original branch intact. Deleting the record turns it red on its own message.

[chenmingwei23]

Verification record for this head, posted as a comment rather than in the
description so the review lanes are not re-triggered on a settled board.

Commands run, one file at a time:

timeout 900 python3 -m pytest -n0 test/test_dashboard_chat_rewind.py -q </dev/null      -> 49 passed
timeout 900 python3 -m pytest -n0 test/test_chat_regenerate_cov80.py -q </dev/null      -> 66 passed
timeout 900 python3 -m pytest -n0 test/test_chat_regenerate_refusal_codes.py -q </dev/null -> 3 passed

Plus flake8, black --target-version py310 --check and isort --check-only
clean on the four changed files, and prove.py --base origin/main reporting
PROVEN.

The three-valued-outcome test was mutation-verified: restoring destroyed = False
turns it red on its own message, so it fails for the reason it claims to test
rather than incidentally.

Main moved to 12ecd4512 mid-work and the branch was rebased and re-verified
before pushing, so these numbers are against the current head.