fix(dashboard): pin expected_history_key at the remaining tags/folders forced saves (#7519)

[chenmingwei23]

Problem / Motivation

save_slot_off_loop / _save_slot_to_history resolve their target transcript from live routing (linked_session_key) at write time. A rebind during the persist await -- a cron completion or workflow injection rebinds already-live slots with no running gate -- can therefore redirect a durable write to a transcript the caller never authorized against. PR #7346 added the expected_history_key refuse-if-moved pin (the save returns False, writing nothing, when the live key has moved off the pinned one) but wired it only at the autocompact endpoint. The remaining forced-save sites in the tags/folders handlers shared the same rebind window and were still unpinned.

Why it matters

A user action aimed at one conversation (tagging it, filing it, pinning it, switching its mode, deleting a tag or folder) could durably mutate a different conversation's transcript metadata if the slot rebinds mid-request. That is a cross-session write-integrity hole: the caller's authorization covered one transcript and the bytes landed on another, with no error and no audit trail.

What changed (motivation -> approach -> change)

Thread the pin through the 8 remaining forced-save sites, following the autocompact precedent:

• chat_tags.py: the tag-delete slot strip loop, PUT /slots/{slot}/tags, and the drag-drop status reassign (POST /slots/{slot}/drop).
• chat_folders.py: the folder-delete unfile loop and its _restore_unfiled rollback, PATCH /slots/{slot}/folder, PATCH /slots/{slot}/pin, and PATCH /slots/{slot}/mode.

The five request endpoints capture the authorized key before their first await and re-check it (plus slot object identity, mirroring _reauthorize_after_await) after the last await before mutating, so a rebind during body parsing, lock waits, or the mode endpoint's busy probes is refused before anything mutates. The persist window itself is covered by the save's pin.

Refusal handling matches each caller's existing convention rather than one uniform policy:

• Direct mutation endpoints (tags PUT, folder, pin, mode) roll back the live field and return 409 session_gone -- the autocompact disposition.
• The drag-drop endpoint answers in its own rejection shape (ok: false + reason, SEL rejected), so the card stays put.
• The best-effort cleanup loops (tag-delete strip, folder-delete unfile, restore rollback) mark the slot dirty for the periodic flush and keep going, matching their existing failure tolerance.

Hardenings from review: each endpoint's re-check/mutate/persist/rollback span is serialized -- folder/pin/mode under a new per-transcript _slot_meta_txn_lock (the autocompact txn-lock shape), and the drag-drop reassign under the module's existing tags_write_lock that every other slot.tags writer already holds -- so a rollback can only ever undo its own write (value-based rollback cannot tell "my write survived" from "someone else wrote the same value"). Rollbacks additionally stay compare-and-set as defense for the non-endpoint writers that do not take these locks (the folder-delete unfile loop), and the folder endpoint restores the prior _folder_changed breadcrumb latch instead of clearing it (a pending re-injection from an earlier successful move survives a later refusal).

Reviewed and deliberately NOT changed:

• A rebind landing after the save's routing snapshot leaves the durable write correctly on the authorized transcript; the slot's own metadata then follows the slot to its new home on later flushes. That is the ordinary data model for slot-owned meta keys (autocompact's post-save check protected its SessionManager live-map seeding, which has no analogue here).
• The _unhide_folder write is not compensated on the new 409 path: re-hiding could erase a concurrent legitimate unhide, and a visible empty folder is benign and user-correctable.
• Same-class force=True sites outside this issue's tags/folders scope (slot-recreate in chat_handlers.py, chat_auto_tag.py, crew_chat.py) are left for a follow-up issue.

Tests

• New test/test_forced_save_history_key_pin.py (11 tests): two real-save tests drive endpoints through the real _save_slot_to_history with routing rebound mid-persist (409, rollback, nothing written to either transcript -- mutation-sensitive: an unpinned save reddens the foreign-meta assertion); per-site disposition tests for all 8 sites assert the refusal handling and that the pin kwarg equals the pre-request key; hardening tests cover the preserved breadcrumb latch, the concurrent-writer-wins rollback guard, and a real lock-window rebind refused by the post-await re-check before any mutation.
• Two existing test stubs that returned None (now falsy at the new refusal branches) return True, restoring their documented fidelity.
• Ran: the new file (11 passed) plus the 10 related suites (300 passed); isort, flake8, the baselined black gate, and full mypy (1,243 files) all clean.

Manual verification

Not applicable -- backend-only; behavior on the success path is byte-identical at every site (verified in review), and the refusal paths are only reachable through the mid-request rebind race, which the real-save tests exercise deterministically.

Screenshots / video

Not applicable (no UI change).

Related Issues

Closes #7519

Pattern harvest

Rule candidate: a mutate/save/rollback span with awaits inside must be serialized per resource (txn lock), with compare-and-set rollback as the fallback where a writer cannot take the lock: value-based restore cannot tell "my write survived" from "someone else committed after me", so an unconditional restore erases a concurrent writer's acknowledged commit. The same class produced _autocompact_txn_lock and the guarded rollback in _restore_unfiled; candidate for the recurring-defect list if it recurs once more.

Checklist

• Tests added/updated for the change
• Local gates run (isort / flake8 / mypy / pytest)
• Docs updated where behavior they document changed (no spec documents these endpoints' persist semantics; the pin's contract lives in chat_persistence.py docstrings from feat(chat): per-session auto-compact threshold slider in the context popover #7346)

Contribution License Agreement

By submitting this pull request, I confirm my contribution is made under the terms of the project's license.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Extends the proven #7346 refuse-if-moved pin to the remaining sites with sound serialization and CAS rollback; scope deferrals are explicit and reasonable.

Suggestions

• The capture-key → re-authorize → pinned-save → CAS-rollback → 409 span is hand-expanded five times across the endpoints; extracting one shared async helper (context manager or wrapper) would make the invariant impossible to drift per-site and shrink this PR's own duplication.
• Since this is the second round of fixing the same unpinned-force=True class (and three known sites remain), a CI/lint gate on new unpinned force=True saves would close the class rather than instances — worth filing with the deferred-sites follow-up.

[DESIGN-REVIEWED] 94c0bf9

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 94c0bf9909fd3a23f605a82f4c56f518679533d5 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. The mechanism pre-exists in chat_persistence.py (#7346), the three deferred siblings are exactly where the PR says (chat_handlers.py:2300, chat_auto_tag.py:137, crew_chat.py:1119), the new lock mirrors but is functionally distinct from the existing tags_write_lock (which auto-tag, a machine-driven writer, holds — coupling mode's crew probe under it would stall that), and every hardening in the diff is declared in the description. Final review:

First-Principles-Verdict: CONCERNS

Every item is a declared, derived fix for #7519 — but it threads the pin through 8 call sites while the cause-level fix (force-implies-pin inside the save) stays deferred with 3 counted siblings.

What this change ships

Intent: stop a sidebar action (tag/file/pin/mode) from durably writing to a different conversation when the slot rebinds mid-request — a FIX, extending #7346's existing pin.

1. Tag/folder/pin/mode writes land only on the conversation the user acted on (8 sites) — justified
2. Four endpoints can now answer 409 "session gone" mid-request — justified, declared
3. Board drag-drop bounces the card back in its own rejection shape — justified, declared
4. Tag/folder-delete cleanup retries via periodic flush on a refused save — justified, declared
5. Requests re-checked after body-parse/lock awaits, refused before mutating — justified, same cause
6. Folder/pin/mode requests serialized under a new per-state txn lock — declared, derived from the rollback's own semantics
7. Drag-drop can no longer resurrect a tag deleted while it waited — rides along, real race, declared
8. Mode's busy probe moved inside the lock (timing) — declared
9. Refused folder move preserves a pending [FOLDER] breadcrumb — declared
10. Two test stubs now return True — declared, test-fidelity

Watch

Point pattern with counted, declared siblings: grepping save_slot_off_loop(... force=True leaves 3 unpinned sites (chat_handlers.py:2300, chat_auto_tag.py:137, crew_chat.py:1119), which the description defers ("left for a follow-up issue"). The candidate general fix is one line at the cause: save_slot_off_loop captures slot_history_key(slot) at entry and pins whenever force=True, retiring the kwarg-threading at all 11 sites instead of extending it file by file. Deciding whether any forced caller must follow a rebind is the open question the follow-up should answer before mechanically copying this pattern — accepted-and-deferred, not a demand.

[FIRST-PRINCIPLES-REVIEWED] 94c0bf9

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for 94c0bf9909fd3a23f605a82f4c56f518679533d5; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The single candidate is self-rated "low" confidence and explicitly unconfirmed. Verifying its (c) — observable wrong outcome:

The _dirty = True refusal path re-enrolls the slot in the ordinary periodic flush (_flush_dirty_slots → flush_slot_now), which writes to the slot's current routing without a pin. But that flush is a pre-existing, always-on mechanism that writes every dirty slot's own metadata to whatever transcript it currently routes to. After a legitimate rebind, the slot is that transcript, and the metadata written is the slot's own rolled-back state — not a foreign conversation's data being corrupted with this request's value. Pre-PR, the endpoint save wrote the new value directly to the foreign transcript; this PR refuses that and only leaves the standard convergence mark. There is no re-derivable cross-transcript write that this diff introduces beyond what the flush already does for every dirty slot, so (c) does not hold at 80+. The candidate dies under falsification.

No further grounded findings surfaced in Step 2 — the lock acquisition, CAS rollback guards (if slot.tags == written_tags, etc.), and under-lock vocabulary resolution are all internally consistent.

No findings.

[OPUS-REVIEWED] 94c0bf9

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

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

[chenmingwei23]

GPT 5.6 round 1 disposition -- FIXED in 85ec27e.

Finding: value-only rollback guards can erase an EQUAL concurrent commit (same-value write + mid-save rebind: the newer save succeeds, the refused request's compare-and-set cannot distinguish its own surviving write from the equal one, and restores stale state).

Fix, per the prescribed remedy: the whole re-check/mutate/persist/rollback span is now serialized -- PATCH folder/pin/mode under a new per-transcript _slot_meta_txn_lock (same shape and rationale as chat_handlers._autocompact_txn_lock, WeakValueDictionary keyed by transcript so alias slots serialize too), and POST drop under the module's existing tags_write_lock that every other slot.tags writer (PUT tags, tag-delete strip, auto-tag) already holds. Under the lock exactly one request is inside the span per transcript, so a rollback can only undo its own write. The compare-and-set guards remain as defense in depth for the non-endpoint writers that do not take these locks (the folder-delete unfile loop, which never rolls back -- it dirty-marks).

Verified: 11 pin tests + 300 related tests green; isort/flake8/black gate/loop-bound-locks gate/mypy clean.

[chenmingwei23]

GPT 5.6 round 2 disposition -- all three FIXED in 1de9745.

1. Rebind refusals bypass SEL: every new session_gone denial now emits log_api_access(outcome=denied) before returning -- PUT tags (both paths), PATCH folder (both, via _audit_origin like its allowed path), PATCH pin (both), PATCH mode (both). The drop endpoint already logged its rejections.

2. Lock wait makes the mode busy guard stale: the fail-closed running/pending-work/crew-live-work checks now run INSIDE _slot_meta_txn_lock, after acquisition and after the identity re-check, so the decision is fresh at mutation time.

3. Drop persists a tag deleted while awaiting the lock: the column lookup and tag_index are now resolved UNDER the tags write lock; a target deleted while the request waited is absent from the vocabulary, reads as 'column is not a status lane', and the drop is a rejected no-op. New test pins this (drop with a dangling column tag id leaves slot tags untouched and never calls the save).

Verified: 12 pin tests + 300 related tests green; isort/flake8/black gate/mypy clean.

[chenmingwei23]

GPT 5.6 round 3 disposition -- FIXED in f084270.

Finding: the txn lock was keyed by the transcript key, which is exactly the identity a rebind CHANGES -- so PATCH A waiting in its save and same-value PATCH B arriving after a rebind would acquire different locks and the spans interleave anyway (A's rollback could erase B's acknowledged live change before B persists).

Fix, per the prescribed remedy: _slot_meta_txn_lock is now keyed by the STATE (one shared metadata lock, WeakKeyDictionary[state, LoopBoundLock]) -- the same rebind-stable identity chat_tags._TAGS_WRITE_LOCKS already uses for every tags writer, so the folder/pin/mode spans serialize regardless of routing changes. These are rare human-driven sidebar operations, so a single per-state lock does not contend. LoopBoundLock replaces the raw asyncio.Lock for loop-rebind safety (issue 4800 convention).

Verified: 122 tests across the pin file + folder/mode suites green; isort/flake8/black gate/loop-bound-locks gate/mypy clean.

[chenmingwei23]

GPT 5.6 round 4 disposition -- FIXED FORWARD in a051ee8 (the prescribed revert would reopen the cross-transcript write hole this PR exists to close).

Finding (real): a slot left dirty can be picked up by the UNPINNED periodic flush while the endpoint's pinned save awaits; the flush persists the provisional in-memory value to the slot's current transcript, then the endpoint's save refuses and rolls back -- leaving the durable record holding a value the caller was told did not apply.

Fix: every endpoint refusal path now marks the slot dirty after its rollback, so the next periodic flush re-persists the rolled-back live state to wherever the slot routes and the durable record reconverges within one flush interval. This is the same reconvergence mechanism the cleanup loops (tag-delete strip, folder-delete unfile, restore rollback) already use, now uniform across all 8 sites. The provisional-value window is inherent to the slot model's in-memory-first design (the flush has always written live fields unpinned); the invariant this PR holds is that the REQUEST's own durable write never lands on an unauthorized transcript, and the flush-side divergence now self-heals.

Verified: 12 pin tests + 313 related tests green; black gate/flake8/mypy clean.

[chenmingwei23]

CI note: the Backend Tests (Windows) (3) red on a051ee8 is MAIN-OWNED, not this PR's. The failing assertion is test_security_posture.py TestGateSideLogRedactorSpelling (slack/gateway.py: 7 gate-side log sites vs census 6). This PR touches zero lines in slack/gateway.py or the census; reproduced the identical failure against pure origin/main (a492b65) in a detached worktree: 1 failed. Expect the same red on any sibling shard that carries this test until main lands the census fix; this PR will rebase onto settled main to cut a fresh merge ref once that happens. All five AI review lanes are green on this head (GPT converged after 4 fix rounds).

[chenmingwei23]

Rebased onto main 213f805 (the gate-side census fix has landed on main -- verified by running the census test against pure origin/main in a detached worktree: 1 passed). New head 94c0bf9 is the same single commit, byte-identical diff (4 files); scoped gates re-ran green locally (black gate / flake8 / mypy / 104 tests). This cuts a fresh merge ref past the main-owned red; all review lanes re-roll on the new head.

[chenmingwei23]

/ai-review override gpt 94c0bf9: Zero-delta vs base 213f805: the mutate-then-await shape and the unpinned periodic flush exist verbatim on main (identical shape in the merged autocompact endpoint), so this finding is a pre-existing hole belonging to another issue; this PR strictly narrows that window (pin + reauth + state lock + dirty reconvergence) on a diff byte-identical to a051ee8, which this lane passed. Residual class filed as issue 7772.

[chenmingwei23]

GPT 5.6 round 5 (post-rebase re-roll on the byte-identical diff) disposition:

• BLOCKING (provisional metadata escapes via periodic flush, crash before retry): NOT APPLICABLE TO THIS PR -- measured zero delta against base. Main's own folder endpoint mutates the live slot then awaits (_unhide_folder + a fully UNPINNED save), and the merged autocompact endpoint has the same mutate-then-persist shape; the flush has always written live fields unpinned. This diff strictly narrows the window (pre-await capture, identity re-check, state-wide txn lock, refusal rollback + dirty reconvergence). The remaining residual is a slot-model staging decision, filed as issue 7772 (item 2). Realized via /ai-review override per the zero-delta rule; this lane previously passed this exact diff on a051ee8.

• FINDING (bool coercion treats JSON string "false" as true on /pin): real class, pre-existing on main VERBATIM -- this PR did not add or move that line's semantics (prior_pinned/new_pinned wrap the same expression). Folding a validation change into a converged security PR re-rolls every lane for no correctness gain on this PR's scope; deferred to issue 7772 (item 3).

[github-actions]

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 94c0bf9909fd3a23f605a82f4c56f518679533d5.

Zero-delta vs base 213f805: the mutate-then-await shape and the unpinned periodic flush exist verbatim on main (identical shape in the merged autocompact endpoint), so this finding is a pre-existing hole belonging to another issue; this PR strictly narrows that window (pin + reauth + state lock + dirty reconvergence) on a diff byte-identical to a051ee8, which this lane passed. Residual class filed as issue 7772.

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

[chenmingwei23]

First Principles CONCERNS disposition -- ACCEPTED AND DEFERRED (matching the review's own framing). The cause-level candidate (force-implies-pin inside save_slot_off_loop, retiring the kwarg threading at all 11 sites) is now recorded on issue #7772 together with the design question that gates it: whether any forced caller must legitimately follow a rebind. This PR deliberately stays at the call-site pattern #7346 established because answering that question changes the save's contract for ALL callers -- an architecture decision for the follow-up, not a mechanical extension of this diff. The three counted unpinned siblings are item 1 of the same issue.

[chenmingwei23]

Review-ready on head 94c0bf9: PR Readiness = success (readiness: passed label applied), all 68 deduped check-run lanes green including CodeQL, and all five AI review lanes verdict-clean -- Opus/Design/UX pass, First Principles CONCERNS dispositioned (accepted-and-deferred to issue #7772), GPT via the recorded human override on this head after 4 fixed rounds + 1 zero-delta round.

Round ledger: r1 rollback-erases-equal-commit (FIXED: state txn lock + tags lock serialization), r2 SEL-on-denials + stale busy guard + drop resurrects deleted tag (all FIXED), r3 lock keyed by rebindable transcript key (FIXED: state-keyed LoopBoundLock), r4 provisional value escapes via unpinned flush (FIXED FORWARD: dirty reconvergence; prescribed revert rejected as reopening the hole), r5 same finding re-raised on a byte-identical diff the lane had passed (zero-delta vs base, overridden; residual + siblings + pin bool-coercion filed as issue #7772). Branch untouched since the override (push voids it).

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause -- the remaining forced-save call sites in chat_folders.py/chat_tags.py did not pin expected_history_key, so a concurrent write could clobber history; the fix pins it at each of them (#7519).

[bolichen97]

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

• PR #5933 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 #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.

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