Skip to content

fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit - #5933

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/slots-snapshot-across-await
Open

fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit#5933
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/slots-snapshot-across-await

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

api_chat_folder_delete and api_chat_tag_delete each walk every slot and persist it. Since that persist became await save_slot_off_loop(...), each iteration now contains a yield point — but both still iterated the live state._slots.values() view: the unfile loop in src/kiro_crew/dashboard/chat_folders.py and the tag-strip loop in src/kiro_crew/dashboard/chat_tags.py, each with the await nested directly inside.

state._slots is mutated by other coroutines that can run during that yield — session_transfer.py and session_control.py pop keys, openai_compat.py pops on its cleanup paths, and get_or_create_slot assigns. So deleting a folder or a tag while any session closes concurrently raises RuntimeError: dictionary changed size during iteration out of the handler.

Before the persist was awaited, these loop bodies had no yield point and the iteration was effectively atomic against other coroutines, so the live view was safe. Adding the await is what made it reachable.

Why it matters

The raise is worse than a 500, because both loops mutated before they persisted, so it landed with the work half-applied:

  • Folder delete abandoned the unfile partway — some conversations left with folder_id cleared, others not.
  • Tag delete had already removed the tag row from the vocabulary by that point, so slots past the raise kept a tag id whose vocabulary entry was gone.

Both leave durable state inconsistent, and the trigger is ordinary concurrent use rather than anything exotic: one tab closing while the user tidies folders or tags in another.

Chasing that down surfaced three further ordering defects on the same two handlers, each reachable by ordinary concurrent use and each producing a durable wrong value rather than a crash. They are fixed here too, and described below in the order they were found.

What changed (motivation → approach → change)

Everything this change ships, in one list (each item is expanded below):

  1. Snapshot the slot view in both delete sweeps — list(state._slots.values()), so the
    awaited persist cannot mutate the iteration.
  2. Concurrent-close arbitration — the persist is withheld when the slot is no longer the
    live _slots entry, so a close that committed inside the window is not erased.
  3. The vocabulary removal commits FIRST, so a crash cannot leave the sweep's effect
    without the delete that justified it.
  4. The sweep is ONE round of two passes, with no bounded repeat.
  5. The matching slots are captured before the commit, so a slot created during the
    window is still reached.
  6. Metadata-only merge instead of a full save in the sweeps — a full save rebuilds every
    SLOT_OWNED_META_KEYS entry and erases a concurrent close's closed flag.
  7. The identity re-check lives inside persist_swept_slot_meta, not duplicated at each
    call site, so a future sweep site cannot omit it.
  8. Committed-vocabulary snapshots (_committed_folder_ids / _committed_tag_ids)
    published only after a confirmed write, with None = UNKNOWN (fail open) and
    frozenset() = KNOWN-EMPTY (prune) kept distinct.

Scope note on the restore-time pruning (declared, not incidental). Pruning a dangling
folder_id when a mid-session path rebuilds a slot from persisted metadata is a behaviour
change rather than a pure bug fix
: before this change those paths adopted the persisted value
verbatim, so a conversation filed into a since-deleted folder kept naming it until something
else rewrote the field. It rides along with the crash fix because the same validator is what the
producers now route through, and it is the half that makes publish_committed_* correctness
load-bearing on every boot. The tag side carries the same protection: tag_ids_for_restore
takes a committed_before withholding channel — the per-id form of the folder validator's
was_committed — so a readable-but-stale tags.json cannot strip tags applied after its
snapshot. Its cold-start readers adopt persisted tags verbatim, and the mid-session sites that
hold a pre-await observation thread it, so a tag is pruned only on a transition that site
watched happen. docs/system-specs/modules/history.md records the rule and the one site that
keeps the plain membership test on purpose.

  1. One reader for the tag prunestate.tag_ids_for_restore, replacing four
    hand-synced copies of the fail-open rule; _tags_authoritative is retired entirely.

  2. Publication routed through _commit_tags_snapshot, unconditionally after a
    confirmed write, and gated on the tags file actually existing.

  3. Eight AST gate families — no awaited loop over a live _slots view, and no tag-snapshot
    write off the _commit_tags_snapshot chain.

  4. Cancellation atomicity — a new module, snapshot_commit.py, carrying
    sweep_to_completion_despite_cancellation and a publish-exactly-once commit helper, plus
    capture-and-re-raise choreography in BOTH delete handlers: a client disconnect mid-delete
    still finishes the unfile sweep and still emits the operation's only audit line, and the
    cancellation is re-raised once, after every durable consequence.

  5. Parked slots revalidate before re-entering the registry — the two app-teardown
    restores in spec_builder/backend/runtime.py re-registered a whole slot object into
    state._slots, which no attribute-write gate can see, so a slot parked BEFORE the
    sweep's capture was reached by neither pass. Folder and tag ids are now revalidated
    against committed membership read before the await.

  6. Channel default filing refuses a folder that died mid-window — the default-filing
    branch of surface_channel_session no longer files a slot into a folder a committed
    delete removed while the arrival was in flight.

  7. A commit-window rebind cannot move a sweep's write off the record that carries the id
    — routing can change before pass one reads it, so each sweep retains every matching slot
    together with the transcript key it held BEFORE the commit, and pass two pins that key as
    expected_history_key (required, not defaulted). Without the pin the write lands on
    whichever transcript routing moved to, leaving the deleted id durable on the original.

  8. The COLD-START paths are deliberately NOT touched_rehydrate_slot_from_history,
    _apply_recent_session, api_chat_slot_resume and the meta["folder_id"] branch of
    surface_channel_session adopt the persisted folder_id VERBATIM, exactly as the base does.
    An earlier revision of this PR routed all four through state.folder_id_for_restore for a
    shape-only check; that rider was REMOVED, because its only effect was rejecting a malformed
    stored id the base kept harmlessly — the sidebar renders any unknown id in the Unfiled bucket
    (website/src/utils/groupHistoryByFolder.ts:40), so the crash it prevented was one the
    validator itself introduced. The validator's remaining call sites are all race closures: they
    prune only on a committed-present → committed-absent transition a caller observed itself, plus
    the delete sweep's own superseded-merge adoption.
    Residual hazard, stated here and not only in the spec. A readable-but-stale
    folders.json — a restored backup, a half-synced data home — parses as KNOWN, so the
    None=UNKNOWN fail-open rule cannot catch it. Because the cold-start paths now validate
    nothing, that hazard cannot reach them: a stale store can no longer unfile a filing made after
    its snapshot. One sibling remains declared rather than closed: the DEFAULT-FILING branch of
    surface_channel_session, which has no transition evidence to withhold on. The tag
    vocabulary is not one of them. Its cold-start readers adopt persisted tags verbatim,
    and every mid-session site holding a pre-await observation threads it as
    committed_before, so a tag is pruned only on a committed-present -> committed-absent
    transition that site watched happen. api_chat_slot_resume keeps the plain membership
    test on purpose: its KNOWN-EMPTY arm is what stops a crash mid-delete resurrecting a
    dangling id forever. No durable on-disk evidence file backs any of this, deliberately:
    the dangling folder_id such a file would purge is one the tree already absorbs (the sidebar
    buckets it as Unfiled and resume clears it under the lock-held existence verdict), and the file
    would add a second forgeable input in the same directory as the vocabulary. A prune licence
    comes only
    from a transition an in-process caller observed itself. See history.md.

  9. chat_utils.run_config_write now awaits the shared snapshot_commit.drain_shielded instead of carrying its own copy of the shield-drain-reraise loop — subtractive, deleting a third spelling of one protocol. The caller still derives its own outcome, because a config write publishes nothing.

Observed symptom: dictionary changed size during iteration from two delete handlers under concurrent session churn. Root cause: a yield point was introduced inside an iteration over a live dict view whose backing dict other coroutines mutate. The change addresses that cause directly — iterate a snapshot, so the loop's view cannot be invalidated by a concurrent pop:

for slot in list(state._slots.values()):

This is the form the slot-wide loops in state.py already use, so it is the codebase's existing convention rather than a new pattern.

Second — concurrent-close arbitration, and it does alter the success path. The snapshot holds slot objects, not keys, so it stops the crash but not a durability bug behind it: across the await, another task can CLOSE a slot, which pops it from state._slots and persists closed=True. Force-saving the pre-close object then writes it back over that close, and because closed is slot-owned metadata where an absent field means "cleared", the close is erased and the dismissed tab returns on the next restore. Each sweep therefore re-checks identity before persisting, and withholds the persist when the key is absent or rebound:

if state._slots.get(slot.key) is not slot:
    slot._dirty = True
    continue

is rather than is not None because the key can be REBOUND to a different object (get_or_create_slot assigns), and mutating the replacement is the same bug wearing a live key. This is a behaviour change on the success path: a sweep can now skip a persist it would previously have performed, which is the intended effect. Two sites carry the gate — the folder delete's post-commit sweep and the tag delete's strip sweep.

The gate is deliberately on the PERSIST only, never on the in-memory clear. The two have opposite requirements and one guard cannot serve both: an in-memory clear writes nothing, so it can never erase a persisted closed=True — only the force-save can — while skipping the clear as well would leave a slot that was merely transiently absent (popped by an in-flight close whose save then fails, so the close handler restores it) still naming the deleted folder or tag.

Third — the folder removal commits FIRST. The unfile used to run before mutate_folders, which meant a folder write that failed had already mutated slot objects a concurrent close might be serialising: the close persisted the cleared folder_id, and the rollback was withheld on the same identity test that withheld the original persist, so nothing repaired it. Folder still present, conversation durably Unfiled. Committing first means a failed write has mutated nothing at all, which is also the order api_chat_tag_delete already used. The unfile is now the post-commit sweep, which covers a slot filed into the folder mid-flight as well. This removes the pre-commit unfile loop, the withheld deferral list and _restore_unfiled — with the commit first there is nothing to defer and nothing to roll back.

Fourth — the sweep is ONE round of two passes.

One round, no bound. An earlier iteration of this change repeated the sweep up to a fixed bound, and it is worth recording why that is gone rather than silently dropping it. One residual producer could hand a deleted id to a slot the sweep had already passed: DEFAULT FILING, the fourth copy site in surface_channel_session. That branch does validate, on the same shared helper — the gap was never a missing check. It was that the helper had to fail OPEN while the folder-store lock was held, because a locked store may be showing a removal that is about to be rolled back, so an arrival inside that window kept the deleted id:

elif folder_id and needs_default_filing(meta):
    slot.folder_id = folder_id

That value is the channel's configured folder, handed in by reconcile_channel_slots, which resolves it across one await (lookup_channel_folder, read-only and under the store lock, so it sees committed state) and persists it across another before surfacing. A delete committing inside that window cannot be seen by a value that was already resolved and already correct when it was read, so no check at the source can help — and the slot is created by get_or_create_slot AFTER this handler took its snapshot, so the snapshot cannot see it either.

That window no longer exists, which is what retired the repeat. folder_id_for_restore validates against a COMMITTED vocabulary snapshot: a frozenset of folder ids that mutate_folders swaps in only AFTER its write confirms, and never on the rollback path. So the helper cannot observe uncommitted state and needs no lock probe to protect itself from it — the probe, and the store-wide fail-open it caused, are both gone. Because this handler's own commit runs first, the folder is out of the committed vocabulary before any arrival can be assigned it, and a default-filed arrival is therefore pruned at assignment rather than chased afterwards. With no residual producer left, one round is enough: there is no bound, and nothing for a second round to find.

Two passes. The persist is an await, so anything that COPIES slot metadata while it runs — a fork — reads whatever a single interleaved loop has not reached yet, and writes that stale value into a NEW record outside the snapshot. Pass one therefore clears every matching slot and contains no yield point at all, so no copier can observe a half-swept set; pass two does all the awaiting, by which point every slot in the set already reads cleared. That yield-free property is the fork-copy guarantee, and it is why the split survives the collapse to one round: it was never a termination proof. With a single round there is nothing to terminate, and pass two over an empty set is already a no-op.

No third pass, in either delete handler. An earlier revision of this change swept the live view once more after pass two, to catch a writer that put the deleted id back on a live slot — or onto a slot that did not exist when pass one ran — inside pass two's await window. That sweep is NOT in this diff. The only producer that could reach it was api_chat_slot_fork, and this change validates the fork at its source instead, so the sweep had no remaining producer to catch and both handlers drop it. docs/system-specs/modules/history.md is the durable statement of the protocol, and it records what a NEW writer must do in the sweep's absence: validate at its own source, or keep its vocabulary check and its assignment in one synchronous run.

Which producers exist, measured rather than assumed. The one producer a downstream sweep would have had to chase is api_chat_slot_fork: it registers its child in state._slots and then copies the parent's folder_id and tags, and at the base it did so through neither validator nor any read of the committed vocabulary, so a delete landing in its await window made a dangling id durable on a record no snapshot ever saw. This change routes both copies through folder_id_for_restore and tag_ids_for_restore, which removes the producer instead of sweeping after it. The other demonstrated producers were the two REFUSED-MOVE reverts, which capture previous before an await and restore it verbatim when the target turns out to be gone: api_chat_slot_folder and api_chat_slot_create; both are fixed at source the same way. Every remaining writer of slot.folder_id or slot.tags is safe for a stated structural reason rather than by being swept — it holds the delete handler's own write lock, or its vocabulary read and its assignment have no await between them, which covers api_chat_slot_folder's target check and create_session's lock-held folder check alike.

Fifth — the matching slots are captured before the commit. Commit-first put the sweep's snapshot after the folder write, and a concurrent close pops its slot for the whole of that write, so such a slot is in no snapshot the sweep can take. The bounded repeat does not rescue it either: the first pass finds nothing left to clear and stops. When the close's own save then fails, the handler puts that same object back into state._slots still naming a folder that has just been deleted. So the handler now holds the matching slot objects across the commit:

closing: list[Any] = [s for s in state._slots.values() if s.folder_id == fid]

Capturing is safe where clearing was not, and that distinction is the whole reason this does not reopen the third defect: a capture only READS, so a failed folder write still mutates nothing. The clear pass then walks the captured objects and the live view together. A slot in both is visited twice and that is harmless — the first visit clears folder_id, so the second takes the continue.

Also adds eight AST gate families, because these defects are invisible at the call site. Two of the eight carry a maintained list and are the whole of the permanent contributor cost: the 13-entry _UNVALIDATED_VOCABULARY_WRITERS adopter allowlist, and the _FORCE_SAVE_CLOBBER_SITES census (an exact pin: removing a site reddens the build too, so the count is updated with it — deliberately, because the pin is the forcing function for removal). docs/system-specs/modules/history.md enumerates all eight and states that cost. The first: nothing about for slot in state._slots.values() looks wrong until you notice the await nested inside it, and the next slot-wide loop someone writes will read just as naturally. It flags any for ... in <x>._slots.values() whose body contains an await in the same frame, and covers .values(), .items() and .keys() alike — the hazard is a property of the lazy view, not of which projection the loop happens to read, so a gate covering only the spellings already in the tree would stop the defect that was written and not the one that will be. It matches the attribute chain rather than a receiver name, so state, self and ds are all covered. It ships with NO suppression mechanism: there are no sanctioned awaiting live-view loops in src/, so an escape hatch with zero users would be dead surface that only weakens the gate.

The second gate pins the tag-snapshot write chain. Every prune path trusts _committed_tag_ids absolutely, and what keeps that set truthful is publication immediately after a confirmed write inside _commit_tags_snapshot. Folders get this structurally because every folder write routes through mutate_folders; the tag side does not, so the gate asserts that nothing under src/ reaches save_tags_snapshot or _write_tags_snapshot off the sanctioned _commit_tags_snapshot_write_tags_snapshotsave_tags_snapshot chain. It matches bare name references as well as calls, because the sanctioned hop is asyncio.to_thread(_write_tags_snapshot, ...) and a call-only detector would miss both that and any bypass spelled the same way.

Bounds on the claim

  • The identity gate closes the window where the slot is already gone when the sweep reaches it. It does not cover a close that commits while a persist is in flight — a check taken before the await cannot. The persist is no longer a full slot save: these two sites now use a metadata-only merge (persist_swept_slot_meta, which delegates the merge step to _merge_slot_meta) that writes just folder_id / tags under the record's own cross-process lock, so a close committing during the persist is left standing instead of being rebuilt away. The merge's existence check is re-taken INSIDE that lock via update_metadata_if, with a caller guard confirming the record still names the folder / carries the tag being deleted — so a session deleted in the window is not upserted back, and a reassignment that already reached disk is not overwritten. The other force=True call sites in the repo are unchanged.
  • A slot whose close SUCCEEDS is out of state._slots for good, so arming _dirty on it is inert and its persisted line keeps the dangling id until something else saves it. Read-side validation of folder_id — mirroring the tag vocabulary prune the loader already performs — is the right root fix, and this PR now ships it. Of the four sites that copy a folder_id onto a slot, ONE prunes it against the folder vocabulary — the default-filing branch of surface_channel_session in channel_slots.py. The other three adopt the persisted value VERBATIM, as item 12 states: both restore paths in chat_persistence.py and the meta["folder_id"] branch of surface_channel_session. An earlier revision routed all four through the validator; that rider was removed, and this sentence had not been updated with it. Four further readers route through the same helper — the superseded-merge adoption in api_chat_folder_delete, the History-resume path in chat_handlers.py, and the refused-move reverts in BOTH api_chat_slot_folder and api_chat_slot_create — twelve call sites in total once the fork producer and the reverts are counted. The two reverts are siblings and are fixed together here: the create path performs no target pre-check at all, relying solely on _unhide_folder's lock-held verdict, so its revert is the only place its stale capture can be caught. Each fails OPEN when the vocabulary is unknown (folders.json unreadable), since pruning then would unfile every conversation; a legitimately-empty vocabulary is authoritative and does prune. That unknown-vocabulary case is the ONLY fail-open that survives: the helper validates against a committed vocabulary snapshot, published only after a folder write confirms, so it has no uncommitted state to guard against and the unfile sweep is a single round with no residual producer to chase.

Accepted and deferred, with the layer-level option named so the next reader inherits the decision. Three seams are deliberately not in this PR. (1) The folder side's _adopt_observed_placement and the tag side's _adopt_observed_tags both validate their observed value: the tag one against pre_delete_committed, the committed vocabulary captured at transaction start, under the same None-fails-open rule. (2) Ten save_slot_off_loop(..., force=True) sites remain unconverted, so the full-save-erases-a-concurrent-close hazard is closed at the two sweep sites and not at the others. That count is AST-measured: nine are in dashboard/ and a tenth is in crew_chat.py; three of the ten span multiple lines, which a single-line save_slot_off_loop(.*force=True grep misses. All ten are annotated in place, and the count is pinned by a gate so the deferral cannot grow unnoticed. That hazard is a property of the SAVE FORMAT — SLOT_OWNED_META_KEYS is rebuilt wholesale and an absent closed reads as cleared — and converting sites one at a time costs a bespoke guard/adopt closure each, which scales linearly in complexity. So before repeating this at the remaining ten, the follow-up should weigh fixing the layer once instead: either a merge-aware save, or persisting closed POSITIVELY so absence is no longer clearing. That is the decision to take first, not after the seventh conversion. (3) A third repo gate could pin the metadata-only-merge invariant structurally — a function-scoped AST walk asserting neither delete handler contains a save_slot_off_loop(..., force=True) call — which is implementable but would add machinery to answer a concern about machinery, and two behavioural tests already cover that invariant. The sweep-merge protocol no longer relies on prose to be read end to end: the three-way outcome is a named SweepMergeOutcome at the callback boundary, the disposition dispatch in persist_swept_slot_meta is exhaustive over it -- its final arm assigns to a NoReturn-annotated name, which only type-checks once every member is handled -- so a member without a disposition fails type-checking on every supported interpreter, and a guard returning a non-bool is rejected at the boundary rather than truthy-tested. Three tests pin that protocol independently of the two existing sweep sites.

  • Where a rejected placement was already persisted before the slot surfaced, correcting the object is not enough on its own — both revalidating branches of surface_channel_session arm _dirty after the window rebuild so the flush rewrites the record, and neither arms when the placement stands.
  • The sibling folder_id restore paths outside these two handlers now route through the same validator. The History-resume path additionally keeps its own _unhide_folder prune, which acts on an existence verdict taken inside the folder-store lock and scoped to the id that verdict was computed for; the shared helper is an unlocked reader and cannot replace it, so the two compose.

Tests

test/test_slots_snapshot_across_await.py (89 tests):

  • Structural gatetest_no_slot_wide_loop_awaits_over_a_live_view fails the build on any live-view slot loop containing an await, plus test_the_gate_scanned_a_non_empty_tree as a positive control so an empty scan cannot let the gate pass vacuously.
  • Behavioural, on the real handlerstest_folder_delete_survives_a_concurrent_slot_pop and test_tag_delete_survives_a_concurrent_slot_pop drive the actual aiohttp routes with a save_slot_off_loop stand-in that pops a different slot during the awaited save. That models the real concurrent popper deterministically rather than racing for it. Each asserts the pop genuinely fired before asserting the outcome — otherwise the test could pass by never opening the hazard window — and then asserts the surviving slots were fully unfiled / stripped.
  • Identity gate — that a folder or tag delete does not erase a concurrent close, that a transiently absent slot is still cleared / stripped in memory, that a committed unfile is armed for the flush, and test_every_slot_wide_mutating_loop_carries_the_identity_recheck as a structural check that no slot-wide mutating loop is missing the re-check.
  • Commit orderingtest_failed_folder_delete_does_not_durably_unfile_a_closing_slot fails the folder store write after a modelled close has popped and serialised the slot, and asserts the conversation is not left recorded as Unfiled while its folder survives.
  • Repeat and two-passtest_folder_delete_sweeps_every_slot_before_the_first_save_awaits and its tag-side twin have a fork copy an unswept slot's metadata during the first save's await and assert the copy reads cleared; test_folder_delete_resweeps_for_a_default_filed_slot_surfaced_during_persistence default-files a channel slot into the folder during the persist pass, through the real surfacing helper, and asserts the repeat catches it — with a path control proving the arrival took the default-filing branch, and its sibling test_folder_delete_prunes_a_metadata_surfaced_slot_at_the_source covering the branch the prune does close; test_slot_published_after_the_snapshot_is_still_unfiled covers the mid-flight file.
  • Pre-commit capturetest_folder_delete_clears_a_slot_popped_during_the_folder_write pops the slot during mutate_folders, lets the removal commit, then models the close's save failing and restoring that same object, and asserts it comes back unfiled with the flush armed.
  • Detector meta-tests — the gate fires on the live-view shape and on a non-state receiver; stays quiet on the snapshot shape, on a live view with no await, and on an await inside a nested scope.

Every behavioural test above carries at least one negative control that also holds on unfixed code — that the pop fired, that the fork ran inside the await window and wrote a record it could copy from, that the removal committed — so a vacuous green is detectable rather than silent.

Observed on this revision: test_slots_snapshot_across_await.py + test_chat_tags.py103 passed, 0 failed; the folder / rate-limit set (21 files, test_*folder*.py + test_*rate_limit*.py) → 648 passed, 0 failed; test_persist_off_loop.py plus the folder ownership / app-isolation / audit-origin suites and test_channel_slots.py130 passed, 0 failed. flake8 and mypy (both at the versions pinned in pyproject.toml) are clean on every changed file, and the black formatting gate reports no new offender.

Manual verification

N/A — unit coverage sufficient. The original failure is a deterministic RuntimeError on a specific interleaving, and each of the four ordering defects is likewise a specific interleaving; the behavioural tests reproduce those interleavings exactly against the real routes, which is stronger than a manual attempt to hit the races by hand.

Related Issues

  • Make the slot save merge-aware so a full save cannot erase a concurrent close #8361 — make the slot save merge-aware so a full save cannot erase a concurrent
    close. This change defers that layer fix and bridges it with persist_swept_slot_meta, the
    _FORCE_SAVE_CLOBBER_SITES census gate (10) and the _UNVALIDATED_VOCABULARY_WRITERS adopter
    allowlist (13). All three are interim and retire together when that issue lands; each now points
    at it in-source, so the retirement plan is anchored outside a doc paragraph.

Pattern harvest

Rule candidate: review-checklist (argued below against a static rule, with counts)

Pattern: a handler reads a value out of shared state, awaits, and then writes that
value back or acts on it without re-checking that the world still supports it. The
suspension is the whole defect: whatever made the value correct when it was read can
be undone by another writer while the coroutine is parked, and nothing on the resume
path re-asks.

Why it generalises — recurrence, not a single instance. The identical shape
occurred TWICE in this change, in two different handlers, as a refused-move revert:
api_chat_slot_folder and api_chat_slot_create each capture the slot's current
folder_id before await _unhide_folder(...) and restore it verbatim when that
await reports the target gone. Both then leave the slot naming a folder a concurrent
delete removed inside the window. What makes this recurrence rather than one bug
counted twice: only the first was reported. The second was found by enumerating every
writer of slot.folder_id after the first was cited — so a fix scoped to the reported
site would have shipped with its twin live. The same family also motivates the rest of
this change: the original crash was iterating a live _slots view across an await,
the identity re-check had to move inside persist_swept_slot_meta because a check
taken before the await cannot speak for the write after it, and
_adopt_observed_placement has to treat its observed value as pre-await evidence
rather than truth.

Outside this PR, one further instance of the same family is present in the tree at
src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_pool.py:362
(_ensure_runtime_locked, self._runtime = rt), measured both on this branch and on
upstream main, so it predates this change and is not something this PR introduced.

Should it become a static rule? Measured no — and the measurement is the argument.
I wrote an AST detector and ran it over all 1233 parsed files under src/kiro_crew/,
in two strengths:

  • A BLANKET form — any local assigned before an await and read after it — fires in
    688 functions across 1053 read-sites. That is ordinary correct async code, so as
    a gate it is unusable; it would be turned off within a day.
  • A NARROW form — assign an attribute into a local, await, assign that same local
    back onto an attribute — fires once tree-wide, and the one hit is the
    review_pool.py instance above. It does not find either motivating instance,
    including on upstream main where both are unfixed. The reason is structural, not a
    tuning problem: both reverts sit inside if not await _unhide_folder(...):, so they
    are nested one block below the statement level a simple walker inspects.

A deeper walker could reach nested blocks, but that only relocates the difficulty. The
predicate that separates a defect from correct code here is whether the captured
value's VALIDITY can be revoked by another writer during the suspension — which is a
question about the domain (is this id a key into a store another handler can delete
from?), not about syntax. A rule keyed on syntax alone would flag the 688 and still
miss the two that mattered.

So the durable artifact is a review question rather than a lint: when a handler
restores or re-uses a value it captured before an await, ask what invalidates that
value during the suspension, and re-validate on the resume path rather than trusting
the capture.
In this codebase the re-validation already exists as
state.folder_id_for_restore / state.tag_ids_for_restore, which is why both fixes
are one call rather than new machinery. The checkable half is mechanical and cheap:
when a reviewer cites one such site, enumerate every writer of that field before
fixing, because this change is evidence that these arrive in pairs.

Checklist

  • At most two commits (one is the norm), 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 behaviour change
  • No secrets, credentials, or internal references in the diff

Alternative weighed: pessimistic mutual exclusion

One serialising lock over vocabulary deletes versus slot close / fork / surface was not
weighed above, and it deserves to be, because it collapses most of what this change builds.

What it collapses. If no close, fork or surface can interleave with a delete, then the
pre-commit key capture, the two-pass yield-free sweep, the post-await identity re-check, the
placement counter's ABA detection all become unnecessary:
each exists only because a slot can rebind or be popped inside the delete's await window.
That is the large majority of this change's machinery.

What it costs. The lock has to span commit and sweep, not just the commit — holding it
for the commit alone leaves the gap between the halves, which is the window that produced the
original defect. So its hold time scales with the number of slots filed into the folder, and
it is held across a disk write. The operations it blocks are the interactive ones — closing a
tab, forking a conversation, a channel message arriving — while the operation it protects is
rare. That inverts the usual trade: it makes a rare operation cheap to reason about by making
frequent, user-visible operations occasionally wait on a multi-write critical section.

Why the optimistic protocol was still chosen here. Not because the lock is wrong — it is a
coherent design and would be simpler to verify. It is because the lock and the deferred layer
fix solve the same problem, and the layer fix dominates: a merge-aware save (or positive
closed persistence) removes the root cause — a full save rebuilding every owned metadata key
and erasing a concurrent close — without making any interactive path wait. A lock introduced
now would be superseded by that work rather than composed with it, so it would be a second
temporary structure rather than a replacement for this one.

The honest residual. If the layer fix is rejected or deferred indefinitely, the lock, not
this protocol, is the better permanent shape, and the comparison above should be re-run at that
point rather than treated as settled by this change.

Subtracted this round: the rebind-window transcript scrub

An earlier revision carried a SECOND key-addressed write on top of that pin: having found the
orphaned key, it re-visited that record to blank the vocabulary id, with its own guard shape because
the tag side derives its surviving list rather than writing a constant. The pre-commit key capture
itself is NOT what was removed — it is live in both sweeps and is what aims the write they already
make.

It is removed. The state it purged is the one this change's own ruling already accepts — a
persisted id naming no live folder, absorbed in four places, the third of which names this record
exactly ("a dangling folder_id left on the old transcript is ignored on the next load"). The one
behavioural consequence of a dangling id, a withheld auto-file suggestion from
maybe_suggest_folder, cannot arise for an orphaned record at all, because no slot routes to it
for that code to read. Keeping the scrub meant a durable write and a second guard shape (the tag
side needed a stricter one, since its write is derived rather than a constant blank) for a state
the tree does not treat as damage.

Also declared, previously omitted from the enumeration above: chat_utils.run_config_write now
awaits the shared snapshot_commit.drain_shielded instead of its own copy of the
shield-drain-reraise loop. That deletes a third spelling of one protocol; the caller still derives
its own outcome, because a config write publishes nothing.

NEXT STEP, tracked rather than deferred. The merge-aware-save layer decision is #8361, and it is the tracked successor to this change rather than an open-ended maybe: the guard/adopt surface, the adopter allowlist, the force-save census and the gate families all retire with it. Two standing constraints hold until it lands — convert the remaining force=True slot-metadata sites ONLY via that layer fix, never by repeating the bespoke guard/adopt pattern per site; and treat a red force-save census as the prompt to take the decision rather than to raise the pin.

@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 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 github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@dwu96

dwu96 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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

@rnoack1
rnoack1 marked this pull request as ready for review August 25, 2026 20:02
@rnoack1
rnoack1 requested a review from a team as a code owner August 25, 2026 20:02
@rnoack1
rnoack1 requested a review from buluoray August 25, 2026 20:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of bf4f60f1ab3a3f3465f5d465fffa94985866719d via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have enough grounding: the core mechanics (snapshot, commit-first ordering, identity-gated persist, the snapshot_commit module, the committed-vocabulary readers) all match the description, and the spec (history.md) is updated in the same change. The design assessment follows.

Design-Verdict: CONCERNS

Real corruption bugs, correctly fixed — but most of the diff is self-declared interim scaffolding bridging to #8361, and the PR's own lock-rejection argument applies to it.

Watch

  • The PR rejects the serializing-lock alternative because it "would be superseded by [Make the slot save merge-aware so a full save cannot erase a concurrent close #8361] rather than composed with it, so it would be a second temporary structure" — yet the guard/adopt closures, persist_swept_slot_meta, the 13-entry adopter allowlist, the 10-site force-save census, and the eight AST gate families are, by the PR's own text, exactly that: a temporary structure that "all retire together when that issue lands." If Make the slot save merge-aware so a full save cannot erase a concurrent close #8361 stalls, this large optimistic protocol calcifies as the permanent shape the author explicitly says is not the preferred one ("the lock, not this protocol, is the better permanent shape"). The crash fix + commit-first ordering + identity gate is small; the remaining ~90% of machinery exists only because the save format clobbers closed.
    Clears when: #8361 (merge-aware save / positive closed persistence) has an owner and a decision, or a human maintainer explicitly accepts this protocol as permanent.
  • Two hand-maintained pinned counts (_UNVALIDATED_VOCABULARY_WRITERS, 13 entries; _FORCE_SAVE_CLOBBER_SITES, exact pin of 10) now redden the build on any contributor's unrelated slot-save or vocabulary-write change until they re-derive this PR's reasoning from a 470-line spec section. That cost is acknowledged, but it is paid by every future touch of dashboard/ while the bridge stands.
    Clears when: a maintainer confirms the census/allowlist cost is acceptable for the expected lifetime of the bridge, or the gates are narrowed to the two sweep handlers.

Suggestions

[DESIGN-REVIEWED] bf4f60f

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] bf4f60f

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of bf4f60f1ab3a3f3465f5d465fffa94985866719d via the fork AI-review pipeline — 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 source hunks read, key claims verified against the base tree (the live-view awaited loops at chat_folders.py:1094 and chat_tags.py:409 are real on base, 19 _slots.pop sites can mutate during the yield, update_metadata_if pre-exists so the merge helper builds on it rather than duplicating it, and _placement_seq has exactly one real consumer). Emitting the review.

First-Principles-Verdict: CONCERNS

The folder_id property + _placement_seq per-write counter guards a double-race with exactly one reader — the heaviest single rider to weigh.

Not justified as shipped

  • Item 8 — one consumer, generalized: grep _placement_seq outside tests finds one reader (_adopt_observed_placement, chat_folders.py); a per-write counter, a property on every slot, and a ninth AST scan policing the counter's own retirement all exist for it.
  • Item 9 — rides along: consolidation of run_config_write into drain_shielded is absent from the visible (truncated) description; harm-free, inventory only.

What this change ships

Intent: FIX — deleting a folder or tag while a session closes concurrently must not crash mid-sweep and leave conversations half-unfiled or tags half-stripped. (More than 10 items; the 10 most visible kept.)

  1. Delete sweeps iterate a snapshot, not the live slot view; repo-wide AST gate closes the class — justified
  2. Folder delete commits the vocabulary removal first; the rollback/restore path is deleted — justified
  3. Sweep persists write only the swept field, so a concurrent close's closed flag survives — justified
  4. Client disconnect mid-delete still finishes the sweep and emits the only audit line — justified
  5. Committed-vocabulary snapshots, None=UNKNOWN vs empty=KNOWN, published only after confirmed writes — justified
  6. Mid-session restore/fork/park/close paths now prune folder and tag ids a watched delete removed — rides along (declared behaviour change)
  7. Cold-start tag prune removed at two rehydrate paths; dangling ids now survive boot — rides along (deleted pin reversed with recorded evidence)
  8. Per-write _placement_seq counter + folder_id property gating sweep adoption — one consumer, generalized
  9. run_config_write refolded onto shared drain_shielded — rides along
  10. Eight AST gate families + two maintained lists (13-entry allowlist; exact pin _FORCE_SAVE_CLOBBER_SITES = 10) — justified

Watch

  • Item 8's harm is a user re-filing a conversation away and back to unfiled inside one sweep's await window — a double-race. The author names and rejects the smaller refuse-on-SUPERSEDED form; a human should affirm that trade, since the counter, property, and its retirement scan are permanent until then. Clears when: the merge-aware save (Make the slot save merge-aware so a full save cannot erase a concurrent close #8361) retires the guard/adopt surface, or the adopt is changed to refuse and _placement_seq, the property, and the ninth scan are deleted together.
  • Item 6 makes publish_committed_* correctness load-bearing for user filings on every fork/close/park; the declared open siblings are counted at 2 (resume's tag prune still fires against a readable-but-stale store; surface_channel_session's default-filing branch has no transition evidence). Clears when: the tag-side withholding change the spec defers lands, or a human accepts the recorded residual.

Subtractions

  • If the Watch trade goes the other way: delete the folder_id property, _placement_seq, the seq gate in _adopt_observed_placement, and test scan nine as one unit — the plain attribute returns.

[FIRST-PRINCIPLES-REVIEWED] bf4f60f

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] bf4f60f

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 5fcb630 to 3040073 Compare August 25, 2026 22:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 25, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 3040073 to 30431ab Compare August 25, 2026 22:46
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 25, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 30431ab to 30b2a43 Compare August 25, 2026 23:43
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 25, 2026
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Aug 26, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 9a29748 to 957ef40 Compare August 26, 2026 09:14
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 26, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 957ef40 to 1b8720e Compare August 26, 2026 10:33
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 26, 2026
@rnoack1
rnoack1 force-pushed the fix/slots-snapshot-across-await branch from 1b8720e to 37bb9c2 Compare August 26, 2026 11:03
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 26, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator

Problem: This branch still deletes against the pre-#5432 handlers. Main already landed folder tags on the same four files (chat_folders.py, chat_tags.py, chat_handlers.py, channel_slots.py) plus the Chat Folders spec line. GitHub is CONFLICTING because of that, not because of a scrape.

Fix: Rebase onto current main and put #5432’s three new paths on this PR’s vocabulary, don’t restore _tags_authoritative.

  1. validate_folder_tag_ids fail-opens when _committed_tag_ids is None, not on the deleted bool.
  2. Tag delete still strips folders, then runs the slot sweep through persist_swept_slot_meta.
  3. Channel first-filing still inherits folder tags and re-applies crash-durable meta tags; both folder_id assignments go through folder_id_for_restore.

Add the commit-first unfile and restore-time folder_id prune to the Chat Folders sentence #5432 already edited. Leave the remaining force=True full-saves for a follow-up.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@kyleseaman re-verified each item at the current head 6bd438c5df4d246409da9829a8d34610bb78dd26 — all four are settled, so nothing here needs another pass from you:

  1. "Still deletes against the pre-feat(folders): folders carry tags; new chats in a tagged folder inherit them #5432 handlers." The premise is void at this head: feat(folders): folders carry tags; new chats in a tagged folder inherit them #5432 (55ef5c080) is an ancestor of HEAD (git merge-base --is-ancestor exits 0), so this branch is built on top of it, not beside it.
  2. Folder tags on the delete handler — present: folder_id appears 37 times in src/kiro_crew/dashboard/chat_folders.py, and the sweep routes through persist_swept_slot_meta (4 references in that file).
  3. "Don't reinstate _tags_authoritative." Honoured — 0 occurrences anywhere under src/kiro_crew/. It is retired, replaced by the single state.tag_ids_for_restore reader.
  4. "Leave the remaining force=True full saves for a follow-up." Honoured — the census is still pinned at 10 (_FORCE_SAVE_CLOBBER_SITES = 10), so this change removed the 3 it routes through the metadata-only helper and grew nothing. That deferral now has a tracked destination in Make the slot save merge-aware so a full save cannot erase a concurrent close #8361, referenced from the census gate, the adopter allowlist, persist_swept_slot_meta and the spec.

One item from your comment is not closed, and I am not claiming it is: the spec sentence you asked for about commit-first unfile and restore-time pruning belongs in a Chat Folders doc this PR does not touch (its only docs file is docs/system-specs/modules/history.md), so it needs a scope call rather than a quiet edit here.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is OVERLAPPING with PR #4904. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. The gate is intentionally a ratchet that puts the deferred merge-aware-save decision in front of whoever adds a caller, so PR #4904 landing after PR #5933 is exactly the case it is designed to stop. Neither PR is wrong; the ordering and whether the two new sites justify raising the bound need a human call. Files: src/kiro_crew/dashboard/chat_merge_back.py.
  • This PR is OVERLAPPING with PR #6813. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. Nearest code-near candidate by shared files and by both depending on update_metadata_if merge behaviour, but materially different scope and no measured interaction. Files: src/kiro_crew/dashboard/chat_persistence.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #7212. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. No conflict measured (the two new force=True calls in PR #7212 are in its test file, so the census is unaffected, and rows_only=True reuses the one existing src call site). But two partial-save protocols on one seam, arriving separately, is the drift PR #5933's own docs argue against -- the authors should agree whether persist_swept_slot_meta should be built on rows_only rather than beside it. Files: src/kiro_crew/dashboard/chat_persistence.py.
  • This PR is OVERLAPPING with PR #7714. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. PR #7714 is merged and does not implement any of PR #5933's behaviour; PR #5933 is the later, wider fix on the same code and rebases cleanly onto it (git merge-tree against live origin/main reports no conflict). The one thing a reviewer should ask for is a replacement assertion that the sweep merge still targets the pre-await transcript key, since PR #7714's explicit pin assertions are removed at both sites. Files: src/kiro_crew/dashboard/chat_folders.py.
  • This PR is OVERLAPPING with PR #7779. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. No goal or code overlap, but a hard ordering dependency: three separate breakages in whichever direction these land. The _FakeState AttributeError is a test failure neither PR's CI can see today, and the force=True census is deliberately a design prompt rather than a number to bump, so the two authors should agree the order and who pays for the adaptation. Files: src/kiro_crew/dashboard/session_directive_apply.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #8072. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. One new force=True caller trips PR #5933's ratchet. PR #8072's need is legitimate (a full save is what reaches disk for state _dirty does not track), which is precisely the signal the gate exists to surface, so the bound should be raised deliberately or the site converted -- not silently adjusted by whichever PR lands second. Files: src/kiro_crew/dashboard/chat_handlers.py.
  • PR #7669 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7669: KEEP. Independent, both worth landing, but they textually conflict in api_chat_tag_delete. Agree an order: if 5933 lands first, 7669's per-holder enqueue moves into the two-pass stripped list; if 7669 lands first, 5933's rewrite must carry the SessionLaneDelta accumulation forward or lane deletion silently stops firing. Files: src/kiro_crew/dashboard/chat_tags.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — taking the one code-level ask in the audit, and leaving the ordering calls to you.

The #7714 item is satisfied at head 10ffc07b5aadd5ebc08dd8ff6ba76344df51cd31. The audit asked for "a replacement assertion that the sweep merge still targets the pre-await transcript key, since #7714's explicit pin assertions are removed at both sites". That was correct when written; it has since been fixed, via a different route (a blocking review finding on the same window), so here is the evidence rather than another change:

  • src/kiro_crew/dashboard/chat_folders.py:1032 — pass one records the key alongside the slot: cleared.append((slot, slot_history_key(slot))).
  • src/kiro_crew/dashboard/chat_folders.py:1084 — pass two passes it: expected_history_key=pinned_key.
  • src/kiro_crew/dashboard/chat_tags.py:554 and :630 — the same two steps on the tag side.
  • src/kiro_crew/dashboard/chat_persistence.py:3540 — the merge now uses the pinned value (key = expected_history_key) instead of re-resolving routing at write time. Grepping the merge body for a late key = slot_history_key(slot) returns 0.
  • src/kiro_crew/dashboard/chat_persistence.py:3460 and :3574expected_history_key is a required parameter on both the merge and persist_swept_slot_meta, so a new sweep site cannot omit it. Adoption is also suppressed when routing moved, since the observed record is then one the slot has left.

The replacement assertion is test/test_slots_snapshot_across_await.py:1336, and it is control-verified rather than merely green: reverting the merge to re-resolve the key late makes it fail with "the merge targeted … rather than the transcript pass one found … a rebind across the pass-two await retargeted the scrub"; restoring the pin makes it pass.

One thing worth flagging, because it supports the audit's framing: #7714's own test_forced_save_history_key_pin.py (12 tests) passes under that same reverted control. Those assertions cover the forced-save sites, not the sweep path, so they could not have caught this — which is exactly why a replacement was needed rather than reuse.

Not touched, deliberately. The census bound is still _FORCE_SAVE_CLOBBER_SITES = 10. The audit is right that it is a design prompt rather than a number to bump, so I have not adjusted it for #4904, #8072 or #7779 — raising it silently is the thing the gate exists to prevent. The ordering questions (#4904, #7212 on whether the sweep helper should be built on rows_only rather than beside it, #7669's textual conflict in api_chat_tag_delete, #7779's _FakeState interaction) are all human calls between authors and I am not deciding them unilaterally. For what it is worth on scope: neither session_directive_apply.py nor chat_merge_back.py is among this PR's 18 files.

Separately, and not a request for action from you: the three red lanes on this head (Backend Tests (3.12, 4), Backend Tests (Windows) (4), and the derivative Coverage Gate) are main-borne, not from this diff. test/test_slot_close_recreation_race.py arrived with #7212 and its test_cleanup_handover_* cases time out at 120s; I reproduced the identical failing set — same test names, same 10-failed/32-passed split — in a clean worktree at the upstream tip with this PR's commit provably absent.

@jeeshofone

Copy link
Copy Markdown
Contributor

Coordination from the #7779 side, following the maintainer's 2026-09-04 relationship audit (it flagged a hard ordering dependency between these two PRs in session_directive_apply.py, with breakage in either landing order):

Proposal: land #5933 first, and #7779 pays the adaptation. The _FakeState test stubs the audit says would break are #7779's to extend, and the identity-gated persist direction is one #7779's applier already leans into (it pins its persist to the entry-time transcript key). If #7779 happens to land first instead, the offer stands in reverse — ping on #7779 and the adaptation commit follows within a round.

No action needed from you unless you'd rather sequence it differently.

@rnoack1

rnoack1 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Agreed on the sequencing — no objection to this landing first, and thanks for offering to carry the adaptation.

I re-measured the two claims against my current head 68678f830cb5cf3664ae9680d3fe21fa060e253c so the ordering call rests on something checkable rather than on my recollection. Both are clean from this side, which I think makes the adaptation smaller than the audit implies:

  • session_directive_apply.py is untouched here. It is not among this PR's 18 changed files, and at 856 lines it carries zero references to any symbol this PR introduces or re-signatures (folder_id_for_restore, tag_ids_for_restore, persist_swept_slot_meta, expected_history_key, prune_unknown, sweep_to_completion_despite_cancellation). Its only cross-module dashboard import is has_dashboard_surface.
  • No _FakeState stub is affected. _FakeState appears in 23 test modules; none of them are in this PR's diff, and none stub a signature this PR changes.

So there is no file-level collision to resolve — which suggests the dependency the audit flagged is semantic rather than textual, and on that point your read of the direction is right. The persist is identity-gated and pinned to the pre-await transcript key, not re-resolved late: expected_history_key is a required parameter on persist_swept_slot_meta (src/kiro_crew/dashboard/chat_persistence.py:3650, parameter at :3658), and the gate itself is if slot_history_key(slot) == expected_history_key: at :3719. Making it required rather than optional was deliberate, so no sweep site can silently omit the pin. If your applier already pins to the entry-time key, the two agree by construction.

One caveat so it does not surprise you: the required-parameter shape is the part most likely to need a touch on your side, since any call into the sweep-persist path must now pass the key explicitly. That surface is also interim — it is slated to retire with the deferred merge-aware-save layer decision, tracked in the linked issue — so it is worth pinning against the issue rather than against this shape.

@bolichen97

Copy link
Copy Markdown
Collaborator

@rnoack1 This is still the only place the live-state._slots await bug is fixed, so I want it to land, but it cannot land unsequenced. Audited at 6ec76ce (head is now e40cb3f; the 19-file list is unchanged).

Two open PRs add code inside the api_chat_tag_delete strip loop you replace with a two-pass sweep, so whichever lands second must re-anchor into pass one or its feature silently stops firing: #7669 (yours) accumulates SessionLaneDelta using authorized_history_key and the per-iteration save result this PR removes; #8185 (@Pearcekieser) bumps tags_revision after each slot.tags = ..., which also needs re-inserting into _adopt_observed_tags.

Three add a new src save_slot_off_loop(..., force=True) site and break the exact _FORCE_SAVE_CLOBBER_SITES = 10 pin: #8072 (@RohanK6, three in chat_handlers.py), #4904 (@jeeshofone, two in chat_merge_back.py), #8613 (@CrysisDeu, one in state.py). The pin is a deliberate design prompt, so please raise it explicitly or convert those sites rather than let it be bumped silently. #4904 also adds a state._slots.values() scan that test_no_slot_wide_loop_awaits_over_a_live_view will fail repo-wide if it awaits in-frame.

Smaller re-anchors: #7779 (@jeeshofone, already agreed to land after this one) in api_chat_tag_delete, #7353 (@hungtnvu) whose _CHAT_FOLDER_ICON_EPOCHS.pop sits on the _restore_unfiled block you delete, and #6823 which edits the other arm of the same close_slot try/except. #6813 shares files but no interacting code.

One rebase question: merged #8906 added chat_utils.drained_to_thread, so item 17's "third spelling" claim is stale. Should that copy route through snapshot_commit.drain_shielded too?

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

…oops

Awaiting inside `for slot in state._slots.values()` raises "dictionary changed size during iteration" when another coroutine pops a slot, so iterate a snapshot.
The snapshot holds objects rather than keys, so each loop also re-checks identity immediately before mutating and withholds the persist when the key is absent or REBOUND: a full save rebuilds the SLOT_OWNED_META_KEYS line where an absent field means cleared, so force-saving a pre-close object drops closed=True and resurrects a dismissed tab. Folder delete arms the withheld clears only once `mutate_folders` has committed, and re-scans afterwards for a conversation filed into the folder mid-flight, because the loader reads folder_id back without validating it against the folder list. Also adds an AST build gate that flags any live-view slot loop containing an await, with a `# loop-ok: <reason>` suppression that must state a reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants