fix(dashboard): stop the un-flushed tail duplicating persisted rows

[rnoack1]

Problem / Motivation

A bounded slot-detail request (GET /api/chat/slots/{slot}?limit=N) can return the same message twice. The client replaces its message list with this response, so the duplicate is what the user sees in the transcript.

The endpoint serves a bounded page by reading the slot's chained history from disk and then appending any window messages that have not been written yet. It sized that tail by subtracting the disk length from the in-memory window length. Those two numbers are not in the same unit. A save drops transient roles — chunk, done, streaming, queued, permission — so the disk read omits them while the window keeps them. Each transient row in the window therefore inflated the subtraction by one, the negative slice slot.messages[-unflushed:] reached that many rows too far back, and the rows it picked up were ones the disk read had already returned.

Every streamed assistant turn puts a done row in the window, so this fires on ordinary use rather than an edge case: any slot with streaming activity in its window can serve a duplicated page.

Why it matters

The duplication is silent and it is on a read path, so nothing reports an error and nothing self-corrects. A user paginating back through a session sees repeated messages, and because the count of rows returned no longer matches the span the server consumed, the next_before cursor the client sends for the following page is computed against a page whose contents overlap the previous one.

The magnitude scales with how many transient rows sit in the window before the last persisted row, so a long streaming session degrades further rather than staying at one duplicate.

What changed (motivation → approach → change)

Observed symptom: a bounded page returns a message the previous rows already contained. Root cause: the boundary between "already on disk" and "still only in memory" was derived by comparing two lengths counted in different units, so the boundary itself was wrong.

The fix stops deriving that boundary from a subtraction at all. _append_unflushed_tail now identifies persisted window rows by message id: a save copies each window row's meta.mid to disk, so a window row whose id appears in the disk read has been persisted, and the tail is everything after the last such row. Ids are matched as a multiset — one disk occurrence consumed per window row — because a caller can post a duplicate id and a plain set would then match a row that never reached disk; and they are counted over the on-disk window region only, all_msgs[slot._disk_older_count:], so a row in the frozen prefix cannot fund a match. See the set-membership and frozen-prefix sections below. This closes the over-count direction and keeps the duplication fix, but it is not free of edges — see the known gap below. Transient rows carry no id and cannot inflate the boundary, which is the duplication above. Rows written to the same transcript by anything other than this slot's own save also carry no id, so they cannot be mistaken for a flushed window row and consume a turn that is still owed. Id matching is selected only when EVERY row in that region carries a valid id, because a durable injection puts an id-carrying copy in the window and an id-less copy on disk, so the region can legitimately be mixed. For the cases identity cannot serve — a region with no ids at all, which is what a session persisted before ids existed looks like, or a mixed region — the fallback matches the window against the disk region as an ordered subsequence, skipping rows only another writer put there rather than treating the first unmatched row as the end of the window. The subtraction is gone from both paths.

Two details worth calling out for a reviewer:

• It reuses the existing _TRANSIENT_ROLES constant in chat_persistence.py instead of introducing a second name for the same set. That constant is already documented as mirroring the save-time drop gate, and their membership is identical — verified by feeding every role through _build_message_entry_uncached and comparing the set it returns None for against the constant.
• It takes the slot and reads slot.messages itself rather than accepting a precomputed count. The disk read runs in a worker thread, so the window can grow across that await; a count captured beforehand would be stale by the time the tail is sliced.

Scope is deliberately one thing. Two adjacent improvements to the same handler are out of scope here: an in-memory fast path for bounded reads (follow-up #4134), and unblocking the slot-close broadcast, which is being raised as its own pull request rather than folded into a correctness fix.

Tests

test/test_dashboard_chat.py, in TestSlotDetailPagination:

• test_transient_window_row_does_not_duplicate_the_tail — locks in the fix. A window of four persisted rows with one done row among them, all four already on disk, so the correct tail is empty. Asserts the exact returned content sequence. Against the previous expression this fails with msg 3 returned twice:

  AssertionError: un-flushed tail sized by a length subtraction re-appended a persisted row
  assert ['msg 0', 'ms...g 3', 'msg 3'] == ['msg 0', 'ms...g 2', 'msg 3']
    Left contains one more item: 'msg 3'
  

  done is used because _prepare_messages drops it from the response, so the duplicate is visible directly in the content list rather than mixed in with a transient row.

• test_genuinely_unflushed_tail_is_still_appended_once — the negative control. Only the first two window rows reached disk, so the last two are genuinely owed to the client, and a transient row sits between them. This test passes both before and after, which is what makes the pair discriminating: a "fix" that simply stopped appending the tail would satisfy the first test and fail this one.

• test_foreign_disk_row_does_not_consume_the_unflushed_tail — the over-count direction. Three window rows are persisted through the real save path (so the disk rows carry the window's own ids), then a writer that does not mirror into the window appends one row to the same transcript, then a new turn arrives in memory. Against the previous expression that foreign row counts as a fourth persisted window row, the forward walk steps past the new turn, and the response comes back as ['msg 0', 'msg 1', 'msg 2', 'foreign row'] with the owed turn missing. It persists through _save_slot_to_history rather than writing disk rows by hand, because a hand-written disk row carries no id and so exercises a shape no real save produces.

• test_non_string_message_id_does_not_crash_the_bounded_read — a type guard. meta on an inbound message comes from the HTTP caller and is checked only for being a dict, so its values keep whatever type arrived, and an id is minted only when one is absent — a truthiness test, so a truthy non-string id is preserved deliberately (a row replayed from disk must keep its id). Posting meta: {"mid": ["not-a-string"]} therefore lands a list on disk, and hashing it returned TypeError: unhashable type: 'list' as an HTTP 500. Both sides of the match now require a string, following the same shape the pin loader already uses at state.py:4973.

• test_foreign_row_does_not_consume_the_tail_of_an_id_less_session — the same over-count on the id-less fallback, which the test above cannot reach because it persists through the real save path and so puts ids on disk. Here the disk rows are written by ConversationLog.append, which persists no meta, so the read carries no ids and the fallback runs. A foreign row is then appended and a new turn arrives in memory. Against the previous expression it fails with AssertionError: over-count dropped the owed turn, and it also asserts no row is duplicated, so a boundary that walks short fails it too.

• test_id_less_boundary_matches_a_row_redacted_on_load — a unit test on _append_unflushed_tail covering the half of the match that content equality cannot carry. Restore copies ts verbatim but redacts non-user content, so a persisted row can differ from its disk copy in content alone; matching on content only ends the run there and re-appends rows the disk read already returned.

• test_repeated_caller_supplied_id_does_not_hide_an_unflushed_row — drives the duplicate id directly rather than waiting for state to settle, which is what makes it discriminating: it posts an id, saves so that id is on disk exactly once, posts the SAME id again, and reads bounded. Against a set membership test it fails with got ['first'] — the unflushed row is missing. It asserts the fixture still reproduces the duplicate (both window rows carry the id), so it fails loudly rather than passing vacuously if id preservation ever changes. A control that removes only the occurrence-consuming line restores set semantics and fails this test and nothing else.

• test_interleaved_foreign_row_does_not_duplicate_the_persisted_suffix — built through the real save path rather than a synthesised region: save a window row, let another writer append, save again so _interleave_foreign_lines merges that row in time order, then add an un-flushed turn. It asserts the disk really reads ['w1', 'foreign row', 'w2'] before asserting anything about the response, so a fixture that produced no interleave fails loudly instead of passing vacuously. Against the stop-at-first-divergence behaviour the response comes back ['w1', 'foreign row', 'w2', 'w2', 'w3'].

  The window timestamps are set explicitly and far apart rather than left to the clock, and that is load-bearing rather than cosmetic. With all three stamps clock-minted the test was green on Linux and failed on windows-latest with disk=['w1', 'w2'] — the foreign row gone. The cause is a coarse clock, not buffering or line endings: monotonic_transcript_ts (history.py:1179-1219) returns previous + 1µs when the clock has not advanced, and its own docstring cites Windows' ~15.6 ms steps. The slot floors against the same previous row (state.py:1764-1771), so on a coarse host BOTH writers independently derive w1 + 1µs and produce an identical ts. Pass 1 then resolves w1 against itself, leaving exactly one unmatched window entry and exactly one unmatched disk line sharing that stamp — the save's UNAMBIGUOUS in-place-edit case (chat_persistence.py:1573-1587) — so the foreign row is dropped before any interleave can happen. Reproduced on Linux under a frozen clock: the clock-minted shape yields ['w1', 'w2'], the explicit-stamp shape yields ['w1', 'foreign row', 'w2']. The precondition self-check was kept, not relaxed, and the control that reverts the production skip still fails this test.

Full test/test_dashboard_chat.py passes at 601. The wider set of every test file touching chat_handlers, /api/chat/slots/, _disk_older_count, _disk_window_len, session transfer, persistence, channel slots, backfill, rewind, open-slot restore, resume, the cron/workflow injectors, foreign-append handling and history locking runs 4633 passed / 11 skipped / 0 failed. That set deliberately includes the restore and resume paths, which establish a non-zero frozen prefix (test_rehydrate_slot_loads_full_500_message_window, test_resume_from_disk_sends_the_older_history_cursor), the injector paths, which are the writers a mixed disk window comes from, and the history-locking suite that covers the cross-process append the interleave comes from. isort, flake8 and mypy (the three blocking backend gates) are clean — mypy reports no issues across 982 source files.

One unrelated failure was seen and dismissed only after checking: test_gateway_lock_diagnosis.py::test_flock_is_held_by_a_fork_orphan (assert 284433 == 1) exercises gateway_lock / platform_compat flock and /proc identity, nothing this change touches. With both changed files stashed it fails identically on the unmodified commit, same pid and same assertion, and the stash restore was checksum-verified. It is a sandbox /proc reparenting artifact.

Manual verification

N/A — the bug is observable purely through the endpoint's JSON response, which both tests assert on directly.

Note on the failing Windows shard

Backend Tests (Windows) (2) was cancelled at its 40-minute cap (19:33:57Z -> 20:14:10Z) while every other job in the run succeeded. Progress reached [ 99%] at 19:52:17Z and then went silent for 22 minutes. It is a hang, not a failing assertion, and it is not caused by this change. The chain, measured rather than inferred:

• A pre-existing test is borderline against the per-test timeout. On PR perf(history): rebuild the tab-id index from the metadata cache #3998's passing Windows shard 2 the durations report reads 104.11s call test/test_design_tweak_backend.py::TestWhatIsWrittenStaysReadable::test_anything_the_writer_accepts_the_reader_returns, against the shard's --timeout=180. That is 58% of the budget in one test, and 5x the next slowest at 20.59s. It grows a record 1000 bytes at a time and re-writes the whole record each iteration until the 2 MiB MAX_RECORD_BYTES ceiling refuses it, so it performs roughly 2091 cycles of os.fsync + os.replace + os.path.exists + os.unlink and writes about 2 GiB in total. It costs 8-9 s on Linux.
• On this run it crossed the line and took a worker down with it. pytest-timeout fired at 19:42:16Z, dumped every thread with that test on top of the MainThread stack, and the worker went [gw1] node down: Not properly terminated. xdist replaced it and finished the tests to 99%. The controller then wedged, and no per-test timeout applies to the controller, which is why nothing interrupted the remaining 22 minutes and no second stack dump appears. On PRs perf(history): rebuild the tab-id index from the metadata cache #3998, fix(chat): keep a code-block user bubble inside a phone viewport #4116 and feat(sel): bound the Security Event Log with rotation and retention #4000 that same test ran in the same Windows shard 2 with zero timeout banners and zero node-down lines, in about 11 minutes each.
• This change's tests are not in the region that hung, and they shut down cleanly. Within shard 2's ordering the two tests added here sit at positions 1275 and 1276 of 13037, about 9.8% in, immediately after the pre-existing test_message_arriving_during_the_disk_read_is_not_dropped which uses the same TestClient/TestServer pattern. The hang is at 99%, which in this ordering is test_kiro_usage_api.py. Run locally under -n auto with the 180 s timeout, the whole of test/test_dashboard_chat.py gives 579 passed in 5.25 s with no node down and no crashed worker; the two new tests alongside that neighbour give 3 passed in 3.92 s.
• Nothing here is asynchronous, so nothing can be awaited. This change adds zero and removes zero occurrences of await, async def, asyncio., Lock(, .acquire(, .join(, Event( and .wait(; it replaces eight synchronous lines with a call to one synchronous helper. _ChatSlot.drain(), which the new tests call, is four synchronous lines that copy a list and clear an event, so it cannot be awaited and cannot block.
• The shard split is not affected in a way that matters. pytest-split reports No test durations found and splits evenly by collection index, so adding tests does shift later boundaries. Measured directly, the borderline test resolves to group 2 both with and without the two tests added here, and it is independently present in shard 2 on the three other pull requests above, so its shard membership is stable and not a consequence of this change.

Classification: an environment-level hang triggered by a pre-existing test that carries 58% of the per-test budget on Windows. The remedy is to re-run that single job, and separately to bring that test's Windows cost down; no commit here can clear it.

Two earlier statements in this section were wrong and are corrected above. It previously said that test "exceeds the 180 s per-test timeout" on Windows as a general property — it does not, it takes 104.11 s and passes. And it presented that timeout as the hang itself — it is a recovered timeout at roughly 36% through the shard, whereas the 22-minute hang is in the controller after the worker died.

Note on the two failure directions

The GPT 5.6 Review lane raised the foreign-row case as blocking, and it was right. It is fixed here rather than argued with, and this note records the distinction so the next reader does not have to re-derive it, because two review lanes examined the opposite direction and passed.

The old boundary, max(0, len(all_msgs) - slot._disk_older_count), can be wrong in either direction, and the two are not symmetric.

Under-count happens when the window holds rows a save drops. The tail is sized too large and the response repeats rows the disk read already returned. That is the duplication this pull request set out to fix, and it fails loudly: the user sees a doubled message.

Over-count happens when the disk read grows without any window row having been flushed — a row appended to the same transcript by anything other than this slot's own save. The boundary walks past rows that are genuinely still owed, and they are dropped from a response the client uses to replace its message list. That fails silently, and it loses a message rather than repeating one.

slot._disk_older_count is a snapshot: it is set when a session is restored or trimmed and is never recomputed while serving a request, whereas len(all_msgs) is read live from disk on every bounded page. Any growth in the second that the first does not account for becomes over-count.

Both directions are pre-existing rather than introduced here — the hunk this pull request removed derived the identical value from the identical two quantities, and reverting only the production change while keeping the new test reproduces the same dropped turn. Matching by message id removes the need for the subtraction on the primary path, and ordered row matching removes it on the id-less fallback, so both directions are closed on both paths.

Second round: the same over-count on the id-less fallback. The GPT 5.6 Review lane raised this against head 42e5c44a and it was also right. The first round fixed the over-count only where the disk read carries ids; the fallback still sized the boundary as len(all_msgs) - slot._disk_older_count, so on a session whose disk rows have no ids a foreign append still walked one row too far and dropped the owed turn. Reproduced before fixing, in test_foreign_row_does_not_consume_the_tail_of_an_id_less_session: against the previous expression it fails with AssertionError: over-count dropped the owed turn, while the four tests from the first round still pass — so the new test isolates the fallback rather than re-testing the identity path.

The fallback matches the window against the disk region as an ordered subsequence, starting at slot._disk_older_count. A row matches on role plus content, where content is compared after applying the load redaction — _same_persisted_body takes the disk body, runs the transform a restore runs on non-user content (chat_persistence.py:708-709, mirrored byte-for-byte by channel_slots._redact_assistant, whose docstring notes one transcript cannot have two redaction policies), and compares that to the window body.

That replaced an earlier content OR ts predicate, and the reason is a real defect rather than tidying. ts was there to tolerate the restore case, where a persisted row's content legitimately differs from its window copy. But ts is not an identity: on a coarse clock two writers flooring off the same previous row both emit previous + 1µs (history.py:1179-1219, which names Windows' ~15.6 ms steps), so a foreign disk row and an un-flushed window row of the same role can carry an identical stamp while holding different messages. Accepting ts alone then treated the foreign row as the window's own, advanced the boundary past the un-flushed row, and omitted it from a payload the client uses to replace its transcript — the message simply disappears from view. Redaction-aware content comparison covers the restore case for the right reason and leaves genuinely different rows unmatched.

Pinned by test_shared_stamp_foreign_row_does_not_swallow_an_unflushed_row, which asserts the collision and the same-role precondition before asserting anything else; a control restoring the ts clause fails that test and nothing else.

Fixing this also exposed a fictional fixture, worth recording because it had been passing for the wrong reason. test_id_less_boundary_matches_a_row_redacted_on_load used a hand-written pair, token=SECRET on disk against token=[redacted] in the window. The real redactor leaves token=SECRET untouched, so that pair is not a redaction at all and the test only ever passed via the ts branch. It now derives both sides from the real redactor (AKIAIOSFODNN7EXAMPLE → [REDACTED: credential]) and asserts the transform actually changed the string, so it cannot drift back to fiction.

Interleaved foreign rows — why the scan skips rather than stops

This walk originally stopped at the first unmatched row. That conflates two different conditions: "we have run off the end of the flushed window" and "this row is not one of mine". The disk region can genuinely contain rows of the second kind, sitting between rows of the window.

chat_persistence.py:1411-1421 documents why: the save is non-destructive against a cross-process append. It captures its window snapshot before taking the session lock, so another process — the docstring names subagent, cron and CLI — can append and release in that gap; a bare meta + frozen + window replace would silently delete that acknowledged message, so those rows are preserved. They are then merged back in time order by _interleave_foreign_lines (chat_persistence.py:1337-1357), whose own docstring says such rows "genuinely happened BETWEEN the window's turns". So the on-disk window region really can read [window, foreign, window].

Stopping there left every persisted row after the foreign row in the tail, and _append_unflushed_tail returns list(all_msgs) + list(tail) — so an already-persisted suffix was appended a second time. Measured end to end through the real save path in test_interleaved_foreign_row_does_not_duplicate_the_persisted_suffix: the response came back ['w1', 'foreign row', 'w2', 'w2', 'w3'].

The scan now skips a row that does not match the window row under consideration. Two properties make that safe rather than merely lenient. It cannot pass over a row that should have matched, because both sequences are chronological — the save's merge preserves each side's internal order — so a later window row's persisted copy cannot precede the current row's. And it is bounded: the disk cursor only moves forward, so the scans total O(window + region), and the first window row with no match anywhere in the remaining region ends the walk, which is the genuine end of the flushed prefix rather than an arbitrary stop. The test asserts the fixture actually interleaved before asserting anything else, so it cannot pass vacuously if the timestamps tie; a control restoring the stop-at-first-divergence behaviour fails it and nothing else.

One correction to the finding as filed: the line it cites is a docstring line, not the expression. The expression it quotes was three lines below it.

The set-membership finding: one half refuted, one half real

The GPT 5.6 Review lane raised if isinstance(mid, str) and mid in disk_mids: as "collapses valid repeated and non-string IDs", so a repeated id drops an unflushed row and a mixed-type id re-appends a persisted one. An earlier revision of this description dismissed both halves. That was wrong about the repeated-id half, and the argument it used is corrected below. The line numbers drift in every citation: the predicate was at chat_handlers.py:1054 on head 42e5c44a and 1068 on 1e71993b, against cited 1051 and 1061.

Message ids are server-minted but NOT server-enforced. The only mint site is state.py:1806-1807, which produces f"m-{uuid.uuid4().hex[:16]}" — always a str, 64 bits of entropy. But minting is conditional: state.py:1801-1803 mints only when an id is absent, and state.py:1150-1153 gives the reason (a row replayed from disk must keep its id or a post-restart redelivery would not be recognisable). A posted id therefore survives verbatim — chat_handlers.py:460 passes meta=_redact_meta(user_meta) into slot.append, and _redact_meta (chat_utils.py:1276) is {k: _redact_value(v) for k, v in list(meta.items())}, which has no allowlist and preserves the key set. Nothing uniquifies a caller-supplied mid.

No product path supplies one. The frontend builds the POST /api/chat meta explicitly at ChatPage.tsx:3944-3952 from files, dirs, pastes, knowledge, origin and sendId; there is no mid key, and the comment above it states the echo carries "the server-minted mid". Every other non-test mid: in website/src is the pins API (api/pins.ts, useChatPins.ts), which reads a server id and posts to /api/chat/pins. Channel replay cannot supply one either: it forwards meta only when the disk row has a dict (channel_slots.py:492) and ConversationLog.append takes no meta parameter. chat_fork.py:235 does copy source ids into a forked slot, but appends in source order, so it cannot place an on-disk id after an unflushed row.

Repeated id — REAL, and fixed. Reproduced through the endpoint, not argued: post first with meta:{"mid":"m-deadbeefdeadbeef"}, save, post second with the same id, then GET ?limit=10 returns ['first']. The unflushed row is gone from a response the client uses to replace its list. Only ONE disk row is needed — the lane's "two on-disk rows sharing one id" overstates the precondition.

The earlier argument for dismissing this was: the walk has no break, so it takes the last window row whose id is on disk, and since flushes follow window order the persisted rows are a prefix, so last-match equals the true boundary. That silently assumed each id appears at most once in the window. Drop the assumption and last-match-wins is the defect: the second row satisfies the same single set entry and drags the boundary past a row that was never persisted. The code's correctness rested on an unstated uniqueness invariant.

The fix counts disk occurrences and consumes one per matched window row, bounding the match to as many rows as really reached disk. Behaviour is unchanged in the unique case. A "break on the first miss" fix was rejected: queued and permission rows carry ids (they are in _TRANSIENT_ROLES but not _WIRE_ONLY_ROLES) and are never persisted, so an early break would stop at one and re-append everything after it.

Non-string id — still refuted, still unfixed by design. Reproducing it needs a literal meta: {"mid": ["not-a-string"]}, and the failure direction is the opposite of the lane's crash-data-loss-corruption anchor: an unmatched id means the persisted row is not recognised as flushed, so it is RE-APPENDED — a visible duplicate in one bounded read, not a silent loss. The isinstance(mid, str) guard on both sides is what makes that a duplicate instead of TypeError: unhashable type: 'list' as an HTTP 500, which is why it stays.

Reachability for both halves is still caller-controlled only, so neither is a live user-facing bug. The repeated-id fix is defence-in-depth that removes an unstated invariant, and it is cheap enough that arguing the reachability was the wrong trade.

Frozen-prefix scoping — a regression this diff introduced, now fixed

The id counting was built over the WHOLE chained disk read, which includes all_msgs[:slot._disk_older_count] — the frozen prefix, meaning on-disk rows OLDER than the window, none of which is in slot.messages. An id occurrence existing only in that prefix therefore funded a consumption for a window row that was never flushed, the boundary walked past it, and the bounded response silently omitted a real message.

This one is worth naming as a regression rather than an inherited edge. At base the arithmetic was already scoped to the current session — current_session_disk = max(0, disk_len - slot._disk_older_count) — and no id matching existed at all. Introducing the identity path dropped that scoping. Two further pieces of evidence that this was an oversight and not a deliberate widening: the id-less fallback in the same function already starts its disk cursor at min(slot._disk_older_count, len(all_msgs)), so the two branches disagreed about where the window's on-disk region begins; and chat_handlers.py:4158-4160 names all_msgs[:_disk_older_count] "the frozen prefix saves never rewrite".

The fix counts only all_msgs[slot._disk_older_count:]. It is a strict narrowing: _disk_older_count is max(0, disk_total - len(window)) (chat_handlers.py:4135) and is "set at restore/resume, never drifts with new messages" (chat_handlers.py:1152), so the slice is exactly the on-disk window region, and a never-resumed slot has _disk_older_count == 0, which makes the slice the whole list and changes nothing in the common case. Narrowing can only reduce counts, so it can only move the boundary earlier — it can never turn a duplicate into a loss.

Why the existing tests missed it: all seven prior tests run with no frozen prefix (_disk_older_count == 0), so the duplicate-id case they exercised had both occurrences inside the current session, which the multiset already handled. test_frozen_prefix_id_does_not_fund_a_window_match covers the untested variant, and asserts the fixture establishes a non-zero prefix so it cannot pass vacuously against the defect. It fails before the narrowing with "a frozen-prefix id funded a match and hid the owed turn" and passes after; a control that reverts only the slice fails that test and nothing else.

Known gap, measured rather than assumed. Identity only helps where the disk row carries an id, and only this slot's own save writes one — ConversationLog.append has no id parameter at all. So a row that is mirrored into the window and written to the same transcript through that append path (a workflow or crew result) has an id in the window and none on disk. Where the disk read holds no ids at all this is now handled, because the ordered fallback matches on role plus content or ts rather than on an id. The gap that remains is the mixed case: a session with ids on disk from an earlier save and an injector-written row. There the identity walk runs, does not match that row, treats it as still owed, and re-appends it, so a bounded page duplicates it until the next flush rewrites the window with ids. The previous count-based boundary happened to get that case right. The trade is therefore a rare silent loss exchanged for a rarer, visible and self-healing duplicate; closing it properly means giving the append path a way to carry the window row's id.

Mixed id and id-less disk window — the ALL vs ANY quantifier

A third blocking item, distinct from the two above and not a set-vs-multiset error: the gate that selected id matching was if disk_mid_counts:, which is truthy when ANY row in the disk window carries an id. That region can legitimately be MIXED, and the reachability is a fact about the two writers rather than about the matcher:

• slot.append mints an id for any role outside _WIRE_ONLY_ROLES (state.py:1801-1807).
• ConversationLog.append_if_absent (history.py:2204-2245) has no meta parameter and delegates to append, which has none either, so the durable copy reaches disk with no id at all.
• Both writers run for a single row by design. cron_inject.py:142 and workflow_inject.py:192 call slot.append and then append_if_absent_off_loop, and workflow_inject.py's own comment states the periodic slot save "may serialize it to disk before this durable copy runs" — append_if_absent no-ops in that order. In the other order the durable copy lands first, id-less, alongside earlier saved rows that do carry ids.

Confirmed rather than reasoned: test_mixed_id_and_id_less_disk_window_does_not_duplicate_an_injection drives both real writers and asserts the disk window really is mixed before it asserts anything else, so it cannot pass on a fixture neither writer emits. That guard passes, and against the ANY gate the test then fails with "the durable injection was appended twice" — id matching is applied to a row that structurally cannot match, the row reads as un-flushed, and the injection is appended a second time.

The fix requires EVERY row in the disk window to carry a valid id before selecting id matching; a mixed region takes the ordered path, which compares role plus content or ts — fields both writers do record. A control reverting only the quantifier fails that test and nothing else, and all eight earlier boundary tests pass under both quantifiers, so this is not a behaviour change for any previously covered shape.

Note on the advisory Design Review

Recorded rather than actioned. Its Watch items — the divergent flush-boundary estimators, the three copies of the load-redaction pipeline, and an id parameter on ConversationLog.append as the root fix — are all real and all deferred by design. The last of those is the write-side fix that would retire the content matching discussed above, which is why it is the one worth doing next. None is clearable inside this change, and the lane self-declares that it does not block merge.

Why identity rather than the existing _disk_window_len boundary

A fair question, since the repo already has a flush boundary and already uses it for this same job — slot.messages[slot._disk_window_len:] at session_transfer.py:771. It would also be immune to the foreign row, because the save path maintains it rather than deriving it from len(all_msgs). So this is a trade rather than a clear win, and it is worth naming the case it gets wrong.

That boundary can run ahead of the window it indexes. session_transfer.py:688-694 documents the mechanism: the save sets _disk_window_len = len(window) over the raw window, streaming chunk rows included, and _flush_segment then reassigns slot.messages to drop that trailing chunk run and append the finalised assistant message without adjusting the boundary. When that happens the slice slot.messages[_disk_window_len:] silently yields nothing, so the un-flushed tail is dropped — the same loss this pull request is trying to prevent, arriving by a different route. state.py:1453-1454 says as much where the field is declared: it is a trim watermark and "NOT a fragile 'what to append' counter".

The existing consumer handles that case by refusing: session_transfer.py:695-699 raises SnapshotUnstable("the persisted boundary is ahead of the resident window"). A snapshot can abort and be retried. A GET on this read path cannot — it has to return a page — so refusing is not an option here, which is what makes the boundary usable there and not here. Identity has no equivalent staleness, because it is derived from the disk rows the request just read.

It also fails in the opposite direction, which was measured by actually substituting it for the id-less fallback rather than reasoned about. _disk_window_len is advanced only by the save path, so a row a durable injector wrote (cron_inject.py:142, workflow_inject.py:192 — each calls slot.append and append_if_absent_off_loop) is on disk while the counter has not moved. Substituting it makes test_transient_window_row_does_not_duplicate_the_tail and test_genuinely_unflushed_tail_is_still_appended_once fail, because it under-counts and re-appends persisted rows — the duplication this pull request exists to fix. So both candidate counters are wrong for the id-less case for opposite reasons, which is why that path matches rows rather than counting them.

Three sibling sites, checked and deliberately left alone

The same disk-count-plus-window-count shape appears elsewhere. Both were checked against whether they can reintroduce the duplication fixed here, and neither can, so both are follow-ups rather than scope for this change.

chat_handlers.py:4132, in api_chat_slot_resume, computes next_before = _disk_older_count + (total - len(recent)). The second term counts in-memory rows including transient ones while the first counts disk rows excluding them, so the cursor can read high and the following page can overlap rows already shown. That is a cursor defect on the same endpoint family, not a tail-append defect: it cannot duplicate or drop rows within a single response, and correcting it means changing what api_chat_slot_resume hands the client, which needs its own test.

chat_rewind.py:131 computes chained_len = disk_older + len(msgs) and uses it only to validate an index (raw_index >= chained_len) and to word the 400 error. The same unit mix can make that ceiling read a shade high, so a just-out-of-range index could be accepted, but nothing here appends a tail, so no response can gain or lose a row.

session_transfer.py:771 computes new_msgs = slot.messages[slot._disk_window_len :] and sizes the same un-flushed tail with _disk_window_len — the estimator this change rejects for exactly this job, for the reason given above. The mechanism reaches: _disk_window_len advances only on the save and load paths, so a durable injector writes its row to disk without moving it while the window copy grows slot.messages, and the slice then re-includes a row the disk read already returned. The SnapshotUnstable guard does not cover it. That guard reads if slot._disk_window_len > len(slot.messages) at session_transfer.py:695, which fires only when the boundary runs AHEAD of the window; the failure here is the boundary running BEHIND, which the guard cannot see. So this site is genuinely undeclared rather than defended, and it is a follow-up rather than scope here — correcting it changes what an exported bundle contains and needs its own test. Verified by reading the guard and the assignment sites; the duplicate is mechanism-verified, not observed, because the export path was not executed.

Asymmetric redaction on the id-less path — fixed

_same_persisted_body redacted only the DISK side before comparing. That is right for one direction and wrong for the other, and the wrong one duplicates.

A restore redacts non-user content on load (chat_persistence.py:708-709), so after a restore the WINDOW holds the redacted text and the disk may hold raw — redacting the disk side converges. But a save also redacts every non-user role on the way out (chat_persistence.py:1282-1284) while the window keeps the text verbatim (state.py:2107), so in a session that was never restored the DISK holds the redacted text and the window holds raw. Redacting only the disk side cannot converge on that pair, because redacting an already-redacted body just reproduces it. The row then reads as un-flushed, the walk stops, and the persisted suffix is appended a second time.

Reproduced through the real writers rather than argued: with one id-less foreign row present to select the ordered path, a saved assistant row containing a credential came back twice — ['q1', 'foreign row', 'key [REDACTED: credential] here', 'key [REDACTED: credential] here']. The reachability detail worth stating is that this only bites on the id-less fallback; an all-id window matches on meta.mid and never consults body text at all, which is why the first attempt at reproducing it showed nothing.

The fix compares both sides through the transform. The pair is idempotent on already-redacted text (measured), so the restore direction keeps working. The redaction sequence is now applied through one _load_redacted helper rather than inlined again, which is a small step toward the advisory Design lane's concern about that pipeline having several copies.

Honest residual: two bodies that redact to the same text now compare equal, so a distinct row differing only inside a redacted span can be consumed. Measured — two different 20-character AWS-style keys both redact to [REDACTED: credential] and do match. This is the same class as the content-equality residual below, it already applied in the restore direction under the old code, and it is far narrower than the duplication it removes, which fired for every redaction-sensitive row on this path.

New test test_save_redacted_row_does_not_duplicate_the_persisted_suffix drives the real writers and asserts three preconditions before asserting the outcome: the disk window region is genuinely mixed, the disk side really is the redacted form, and the window side really is raw. Reverting only the comparison fails that test and nothing else — 60 other boundary tests pass either way.

The same edit corrects a docstring that had drifted: _append_unflushed_tail claimed "A row matches on role plus EITHER content or ts", while the loop contains no ts comparison at all. The ts arm was removed earlier in this pull request and the prose was not updated with it.

Content equality and a foreign duplicate — reachable, pre-existing, and the suggested remedy is worse

The GPT 5.6 Review lane blocks on chat_handlers.py:1236, the ordered-walk predicate row.get("role", "assistant") == role and _same_persisted_body(row.get("content", ""), body, role). Its claim is that a row written by a different process which happens to share the same role and the same body is consumed by this matcher, the boundary advances past it, and a genuinely un-flushed window message is then left out of a response the client uses to replace its transcript. That claim is correct, and it reproduces.

The concrete input: save a slot so q1 and a1 are on disk, append one more assistant row ok to the window without saving, then have a different process append its own unrelated assistant row that also reads ok — ConversationLog.append, which is how a subagent, cron or CLI writes into a session (chat_persistence.py:1411-1421). Disk then holds ['q1', 'a1', 'ok'] and the window holds ['q1', 'a1', 'ok'], but those two ok rows are two different messages, so a correct response contains ok twice. The measured response contains it once, so one copy is lost.

Two further measurements decide what to do about it, and they point the same way.

First, this is not a regression. Running the identical scenario against the base commit c7e2564f4, whose slot-detail handler sized the tail by subtracting lengths instead of matching rows, produces the same single ok. The behaviour predates this change; this pull request neither introduces it nor makes it worse.

Second, the lane's suggested remedy — treat a content match with a differing timestamp as ambiguous and keep the window tail — was applied and measured rather than argued about, and it reopens the duplication this pull request exists to fix. Under it, test_mixed_id_and_id_less_disk_window_does_not_duplicate_an_injection fails with "the durable injection was appended twice". The reason is that a durable injection's disk copy legitimately carries a different timestamp from its window row: the injector calls slot.append and then append_if_absent, and those two mint their stamps independently — measured 0.6 ms apart, 2026-08-18T14:29:40.694745+00:00 in the window against ...695337+00:00 on disk. Requiring the stamps to be equal therefore refuses to match the durable copy, that row reads as un-flushed, and it is appended a second time — on every cron and workflow injection, not rarely.

That leaves whether some other field could separate the two cases, and none can. A durable copy and an unrelated foreign row are written by the same function, since append_if_absent delegates to ConversationLog.append (history.py:2204-2245). Both therefore carry no meta.mid, both share the window row's role and body, and both differ from it in ts. On the fields that reach disk today the two are indistinguishable, so this matcher cannot do better than choose which error to make. It currently assumes the row is the durable copy, which loses one copy of a duplicate-looking line only when a foreign writer emits an exact body collision against an un-flushed row. The alternative duplicates every durable injection. The current choice is the smaller error, and switching sides without a new field would be a straight downgrade.

The real fix is on the write side, and the advisory Design Review lane already names it: give ConversationLog.append an id parameter so a durable copy carries its window row's id. The id path is exact, needs neither body nor timestamp comparison, and removes the ambiguity instead of trading between its two failure modes. That is a change to history.py and its callers, so it is a follow-up rather than scope here, and it is recorded rather than coded around.

Status note: the blocking GPT 5.6 Review verdict re-raises this same item at the head that already carried this section. The finding is conceded and reproduced here; what is refuted is its prescribed remedy. A further commit does not change that analysis, so clearing the lane is a human decision rather than something another revision buys.

The id walk dropped an un-flushed row that sat before a persisted one — fixed

The id path recorded a prefix boundary: on a match it set start = i + 1, and a miss simply did not advance it. That is only correct if every persisted row comes before every un-flushed one. When it does not, a later match moves the boundary PAST an un-flushed row, and the bounded response leaves that row out altogether. The harm is a drop — a message the reader should see is missing — which is worse than the duplication this change was written to prevent.

Measured, not argued. With q1 and a1 saved so both disk rows carry ids, and an un-flushed assistant row placed between them in the window, the response came back ['q1', 'a1'] — the owed turn simply gone.

The obvious fix is wrong, and it is worth writing down why. Ending the walk at the first miss makes the boundary a true prefix, but a transient row is dropped by the save and so can never match an id: the walk would stop there and re-append every persisted row after it. That is the duplication bug again. Traced on the same fixture, break-on-miss leaves start at 1 and returns a1 twice.

So the id path now selects the owed rows by MEMBERSHIP instead. An id present in the disk window region proves that row reached disk, so the owed set is the rows whose id did not, kept in window order. Transient roles are skipped outright, exactly as the ordered path already does at the equivalent point. Where the persisted rows really are a prefix this produces the same answer, which makes it a strict generalisation rather than a behaviour change.

Two tests, and the second one earned its place. test_interleaved_unmatched_row_is_not_dropped_by_the_id_walk asserts the owed row arrives AND that the persisted row is not duplicated, so it fails against both the old boundary and against break-on-miss. test_transient_window_row_does_not_truncate_the_id_prefix is the negative control, driven through the real writers: queued sits outside the mint exclusion list so it carries an id that can never reach disk. That control caught a genuine regression in my first attempt — selecting purely by membership surfaced the transient row in the response, where the boundary had excluded it — which is why the transient skip is there. Reverting only the production change fails the first test and nothing else; 603 other tests pass either way.

One honest limit on reachability. The interleaved ordering is constructed at the unit level. I did not identify a producer that emits it: the queue reorder at chat_handlers.py:2650 moves transient rows to the END of the window, where a miss is harmless, and I did not audit every segment-finalisation path. The fix is a strict generalisation and costs nothing where the ordering never arises, so it is worth having regardless; but the drop is demonstrated against the function's contract rather than observed in a live flow.

Escalated: the id-presence discriminator for the content path cannot be adopted

The blocking lane also asked that the content-matching path refuse to treat a window row that carries an id as persisted when the only evidence is a role-and-content match against a disk row that carries none. That is a different proposal from the timestamp variant answered further down, and it was tested on its own terms rather than dismissed by reference to that answer.

It cannot be adopted, and the reason is a measurement of the legitimate case. A durable injection puts the row in the window through slot.append, which mints an id, and persists the durable copy through append_if_absent, which records no meta at all. Measured on that path, the window row carries an id and the disk copy carries none — precisely the shape the proposed rule refuses. Implementing it makes test_mixed_id_and_id_less_disk_window_does_not_duplicate_an_injection fail with "the durable injection was appended twice", so adopting it reinstates the duplication this change exists to remove.

That is the same conclusion the write-side note below reaches from the other direction: while the durable copy carries no id, nothing on disk distinguishes it from an unrelated row with the same text, so no rule reading only the persisted fields can separate them. Giving ConversationLog.append an id parameter would; that is a change to history.py and its callers.

This item is escalated to rnoack rather than dispositioned here. It is not being argued away: it is a real ambiguity, it has no safe fix inside this change, and the write-side fix that would resolve it is out of scope for this pull request.

A pending approval prompt was dropped by the bounded read — fixed

_TRANSIENT_ROLES documents itself as being about a window-region disk line — see its comment at chat_persistence.py:1320-1322 — and chat_persistence.py:1571 uses it exactly that way, against lines read off disk. The owed-set loop was applying the same set to slot.messages, which are in-memory window rows, so it was answering a different question: not "is this disk line a real message" but "does the client still need this row". A pending permission row is actionable, and the client reads it straight out of the transcript (selectSlotPendingApproval in chatSlice.ts), so dropping it makes the approval bar disappear while the server is still waiting for an answer.

Skipping it also bought nothing. permission is never persisted — chat_persistence.py:1274 returns None for it — so a permission row can never have a disk counterpart for the id dedup to match. The skip could only ever remove it.

Reproduced before fixing. test_pending_approval_survives_a_bounded_read drives the real producer's call shape (the approval metadata rides in cls as JSON, as at chat_runner.py:5966) and asserts four preconditions first — every disk row carries an id so the owed-set loop runs, the permission row is absent from disk, and present in the window — then asserts the response carries exactly one permission row. It fails before the change and passes after. Reverting only the predicate fails that test and nothing else; 604 other tests pass either way.

Single delivery is safe rather than assumed: the client REPLACES its transcript from this payload rather than appending (state.messages = next in chatSlice.ts), so returning the row once cannot double it. The test asserts the count is exactly one, not merely non-zero, and also asserts the persisted row is not duplicated.

The fix is a call-site predicate, _UNOWED_WINDOW_ROLES, rather than an edit to the shared frozenset. Changing the frozenset would alter persistence and the cross-process append logic at chat_persistence.py:1571, which is far more than this needs. queued deliberately stays excluded: the client rebuilds those bubbles from the payload's own queue field.

The mixed-window fixture was not platform-honest — fixed

Backend Tests (Windows) (2) failed on the previous head, in the redaction test's own precondition guard: has_id=[True, True]. The give-away is the LENGTH — two entries, not three — so the id-less foreign row was absent from the chained read entirely rather than mislabelled.

Located by execution under a frozen clock, which is the coarse-clock extreme. The foreign row is present immediately after the append and disappears in the SECOND save, so the loss is in _save_slot_to_history, not in the append and not in the chained read. The window row derived its stamp as previous-plus-one-microsecond off q1, and the foreign row derived the SAME stamp off the same previous row; a stamp carried by exactly one unmatched window entry and one unmatched disk line is the save's unambiguous in-place-edit case, so the window's version won and the disk line was dropped. On a coarse clock that collision is the default rather than a rarity, which is why POSIX was green and Windows was not.

Fixed in the fixture only, by pinning the two window rows to explicit stamps far apart so the foreign row's real "now" sorts strictly between them and can equal neither — deterministic at any clock resolution. The sibling interleave test already pins stamps for exactly this reason; this test was the only other one with the same foreign-append-then-save shape, confirmed by scanning the file. No skip, so Windows keeps its coverage of the ordered path. Nothing in the product's timestamping or dedup was touched, and the guard was neither weakened nor removed — the guard is the reason the problem surfaced instead of passing vacuously.

Verified with an instrument proven to detect the defect: under the frozen clock the old clock-minted fixture reproduces has_id=[True, True] and the pinned fixture gives has_id=[True, False, True]. An earlier pytest-level freeze attempt was discarded because it failed its own positive control — the pre-fix test passed under it, so it could not have detected anything.

Negative control, as required: reverting the redaction fix this test covers still fails it with "the save-redacted row was appended twice", so the fixture change did not make it vacuous.

A note for the record rather than a change here: the underlying product behaviour — a cross-process foreign append whose stamp coincides with exactly one unflushed window row being dropped by the save's in-place-edit arm — is real, and on a coarse-clock host the coincidence is the common case rather than a rare one. It lives in chat_persistence.py, which this change does not touch, and it is the same root pressure as the write-side identity item already recorded above.

A foreign row that merely redacts alike no longer consumes an un-flushed row

The redaction-equivalent compare treated any two bodies that redact to the same text as the same row. Two DIFFERENT credentials redact to the same placeholder, so a foreign row could be consumed as the window's own and the un-flushed message dropped from a payload the client uses as a replacement. That residual used to be written down here as accepted; it is now closed.

The redaction branch additionally requires the stamps to match, and the measurement is what makes that safe rather than a guess. The branch only ever fires for a row and its own persisted copy — they differ precisely because one side was redacted — and both the save and the load copy ts verbatim, so the legitimate pair carries an IDENTICAL stamp (measured: same value on both sides to the microsecond). A foreign writer's row carries its own stamp and no longer matches.

It cannot reinstate the duplication fixed earlier in this change, and that is checked the same way. A durable injection is byte-identical to its window row, so it returns at the plain-equality branch and never reaches the stamp check — which matters because that pair genuinely does carry different stamps, the two writers minting independently. The requirement is on the redaction branch alone; plain content equality is untouched.

New test test_redaction_equal_foreign_row_does_not_consume_an_unflushed_row asserts the fixture's own preconditions first — two distinct bodies, redacting alike, on a mixed disk window with our row un-flushed — then asserts both rows arrive. Reverting only the stamp requirement fails that test and nothing else; 606 others pass either way. The two bodies are deliberately different credential KINDS — an access-key id, and a labelled secret-key assignment whose value is a single character — because both collapse to the same redaction tag. That keeps the collision real while leaving only one key-shaped literal in the fixture, the canonical documentation example; an invented near-miss variant of that key is what a secret scanner flags.

The tail match runs off the event loop

The tail match walks the window against the whole disk window region and applies the redaction transform to both sides of every candidate compare, so on a large mixed or id-less history the cost is real, and running it inline blocks every other request on the loop. The disk read immediately above it already went through asyncio.to_thread, so one async function had the I/O off-thread and the CPU-heavy scan on it. That asymmetry is now gone.

Moving it to a worker introduces a hazard the inline version did not have: the loop can mutate the window while the scan reads it. So the scan works from a snapshot of the window list and the disk offset taken before any scanning. That snapshot's first form read the two values back to back, which is NOT atomic once the scan is off-loop — corrected below. This keeps the property the function already documented — it reads the window itself rather than trusting a count a caller captured before an await — while making the read consistent for the whole walk.

New test test_bounded_read_runs_the_tail_match_off_the_event_loop compares the thread the match runs on against the loop thread, and guards that the match ran at all so it cannot pass by never reaching the code. Reverting only the dispatch fails that test and nothing else.

Residual: the ordered path can still drop a pending approval

Only the owed-set site was changed. The ordered path has the same skip, but the two are not the same defect and the same edit is wrong there — measured, not reasoned.

In the owed-set path the skip excludes the row from the result outright, so un-skipping delivers it exactly once. In the ordered path the skip only stops the row from advancing the prefix boundary; the row can still reach the tail through the slice, which is what happens in the ordinary case where nothing follows a pending approval. Applying the same predicate there makes the permission row find no disk match, so the walk ends at it, the prefix stops short, and every persisted row after it is re-appended: the measured response became ['user', 'assistant', 'permission', 'assistant'] — the approval delivered, but the persisted assistant row duplicated. The full unit file stayed green under that change, so the suite would not have caught it.

The ordered path does drop a pending approval when a persisted row follows it, measured on an id-less disk window. Fixing it correctly means replacing that path's prefix slice with the same membership selection the id path now uses, which is the un-flushed-tail predicate work already escalated rather than scope here. Recorded for rnoack rather than fixed in this change.

A live streaming chunk was dropped by the bounded read — fixed

While the agent is answering, each piece of text it produces is appended to the window as a chunk row. Those rows were on the list of roles a bounded read hands back nothing for, on the reasoning that they are transient noise. That reasoning was wrong, and the mistake is in what happens downstream: _prepare_messages does not throw chunk rows away, it concatenates a run of them and emits a single streaming row. That row is the only way the answer-in-progress reaches the client through this endpoint, because the client discards raw chunk rows itself. So skipping them upstream meant the collapse step had nothing to work with and the partial answer was simply absent — the user watching a reply arrive would see it vanish on any bounded refresh.

chunk and streaming are now excluded from that skip list, leaving done (which _prepare_messages discards regardless) and queued (which the client rebuilds from the response's own queue field, and which an existing test requires to stay out of the transcript). Only the owed-set path needed the change: on the ordered path the skip does not remove a row from the result, it only stops it advancing the boundary, so chunk rows already reach the tail there.

New test test_live_chunk_row_survives_a_bounded_read drives the real producer's call shape and asserts the response carries a streaming row holding the partial text. It fails before this change with no streaming row at all.

The window snapshot could pair a pre-trim window with a post-trim count — fixed

Two facts have to agree for the scan to be correct: the window rows, and the count of persisted rows that sit before them. Once the scan moved to a worker thread, an append on the event loop could land between the two reads, trim the front of the window and raise that count. The window copy would then be from before the trim while the count was from after it. The count is used to decide which part of the disk read corresponds to the window, so an inflated count made that region too short, the trimmed rows' identifiers were missing from it, and those already-persisted rows were treated as still owed and appended a second time.

The slot's lock is an asyncio lock and cannot be taken from a worker thread, so the fix is the same bounded re-read the save path already uses for this exact race: read the count, copy the window, then confirm the count has not moved, retrying a small fixed number of times and falling through to a final read. It reuses that path's retry limit rather than introducing a second one, so the two cannot drift apart. The loop is bounded, so there is no spin.

New test test_window_snapshot_survives_a_trim_between_the_two_reads fires a trim at the moment the window copy finishes and asserts no persisted row comes back twice. Before the change it returns ['q1', 'a1', 'q2', 'q1', 'a1', 'owed'].

A resolved approval was re-appended after later turns — fixed

A permission row is never persisted, so it is always "owed" by the owed-set walk and therefore always lands in the tail — after every row that did reach disk. For a pending approval that is the right place: it is the newest row, and nothing can follow it because the agent is blocked waiting on the answer. For an answered one it is wrong. The agent has since produced turns that are on disk, so tail-appending moves the approval after them and the rendered transcript no longer matches what happened. Measured before the fix, a slot holding q1 → approval (approved) → a1 returned ['user', 'assistant', 'permission'].

The decision is written into the row's cls JSON in place, so the row stays in the window and its position cannot distinguish it. The owed-set walk now skips a permission row that carries a decision. It tests truthiness rather than key presence, matching the client's own !meta.resolved check, so an empty decision still counts as pending and an actionable approval is never dropped — that is the direction that would hide a live approval bar, and the pending case is covered by its own test which still passes.

Related Issues

Kept out of this pull request so it stays a single correctness fix:

• perf(dashboard): serve a bounded slot-detail page from the in-memory window #4134 — serve a bounded slot-detail page from the in-memory window when it already holds enough rows
• Unblocking the slot-close broadcast in api_chat_slot_delete is being raised as its own pull request. It touches this same file but a different function, so the two are independent; whichever merges second will need a mechanical rebase.

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, no user-facing docs describe this internal boundary
• No secrets, credentials, or internal references in the diff

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[bolichen97]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 6f9cc7da0f3015be3c9e5e44bd5d604bdda8ac05 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 6f9cc7d

[github-actions]

Design Review (Fable 5, fork) — ⚠️ could not complete

The design review did not produce a verdict for 6f9cc7da0f3015be3c9e5e44bd5d604bdda8ac05 (the model call errored or returned no verdict header). See the Fork Design Review job logs. Advisory — does not block merge.

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 6f9cc7da0f3015be3c9e5e44bd5d604bdda8ac05 via the fork AI-review pipeline; updated in place on each push.

Review details

I have enough to decide. Let me confirm both candidates' reachability claims against the code I've opened.

CANDIDATE 1 (non-string disk content crashes _load_redacted): The redaction branch in _same_persisted_body only runs when role != "user" (line 72-76). For a non-user row to reach disk, it passes through ConversationLog.append → _redact_at_write_boundary (history.py:2759), which calls redact_exfiltration_urls(content) → _URL_RE.finditer(content) and would itself raise TypeError on a non-string before it ever reaches disk. Moreover, the unchanged _prepare_messages (chat_utils.py:1671-1673) already redacts every non-user disk row on the render path and has always done so — so the old legacy path crashed on the same input, making the "newly reachable" claim false. The author self-rates this "low: could not confirm a non-string content reaches disk through any shipped writer." Reachability (a) is unconfirmed and (c)'s "new failure" is false. Killed.

CANDIDATE 2 (two ts-less rows conflated): For the redaction branch, disk_ts == window_ts must hold with both empty. But _ChatSlot.append (state.py:2129) always sets "ts": ts or monotonic_transcript_ts(...), so a genuine window row never has an empty ts — it is either caller-supplied or a minted monotonic stamp. window_ts is thus never empty, so the "both stamps empty" collision cannot occur for a real window row. The author self-rates this "low: reachable only via a foreign/corrupt row with no ts" — but the window side is always stamped. Reachability (a) does not occur in practice. Killed.

Both candidates rest on unconfirmed foreign/corrupt-transcript inputs and are falsified by the write-boundary redaction, the always-present window stamp, and the pre-existing _prepare_messages behavior. Neither reaches 80+. No grounded new defect surfaced during falsification (the non-string mid path is guarded by isinstance(mid, str) at diff lines 264 and 294).

No findings.

[OPUS-REVIEWED] 6f9cc7d

[github-actions]

First Principles Review (Fable 5, fork) — ⏭️ skipped

Revision 6f9cc7da0f3015be3c9e5e44bd5d604bdda8ac05 ships no reviewable capability, so there is nothing to inventory. Advisory — does not block merge.

[chenmingwei23]

approved the workflow to run