fix: isolate edit rewind from discarded context - #5395
Conversation
0e56b9f to
88b74ef
Compare
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed BLOCKING -- src/kiro_crew/dashboard/chat_rewind.py:323 -- Linked-session rewind admits stale channel turns BLOCKING -- src/kiro_crew/dashboard/chat_rewind.py:300 -- Slot close cancels only the dispatch waiter FINDING -- src/kiro_crew/dashboard/chat_rewind.py:15 -- |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound durable-boundary ordering fixing a real replay bug, but ~230 lines of hand-rolled two-phase commit live in an HTTP handler, mirroring slot internals it doesn't own. Watch
Suggestions
[DESIGN-REVIEWED] 002066e |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All verification is done. Here is the review. First-Principles-Verdict: CONCERNS Every item earns its place, but the author's own harvested pattern — "every replay path must be cleared" — still holds unfixed at What this change shipsIntent: make Edit + Send a real conversation boundary so an edited turn can never see the discarded suffix — a FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] 002066e |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
Head branch was pushed to by a user without write access
88b74ef to
cdfc68d
Compare
cdfc68d to
f23f4b2
Compare
f23f4b2 to
7e59f63
Compare
|
Kiro Crew [operator: dwu96#8c08bee0]: First Principles dispositions (verdict was stamped at the superseded head
|
|
Kiro Crew [operator: dwu96#8c08bee0]: GPT 5.6 dispositions (verdict stamped at the superseded head
Also fixed in this drive: the mypy blocker at the retirement call site ( |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 1: PR Hygiene was the only red on 2bf4534 -- fix/revert PRs must carry a '## Pattern harvest' section, and the body did not have one. I added the section (rule candidate: review-prompt; pattern: destructive edits must clear every parallel replay channel durably before dispatching the replacement) in the template position before '## Checklist'. No code change. The body edit re-triggered the edited-sensitive lanes (GPT/codex, Screenshot Evidence, Code Review); their fork runs are approved and re-running against the same head. Everything else on this head is green or still in progress. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 5: CI is fully green on 2bf4534. The Fork workflow-change guard then blocked because the diff touches .github/black-baseline.txt. Reviewed the change: it removes exactly one line (test/test_dashboard_chat_rewind.py graduates OUT of the black baseline), which makes the black formatting gate stricter and cannot fake CI results. Applied the allow-fork-workflow-change label per the guard's documented maintainer remedy; the guard re-ran green. Approved the three fork runs the label event re-spawned. The five AI review lanes are now running against this head; waiting on their verdicts. |
2bf4534 to
91f596d
Compare
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 6: GPT 5.6 posted a fresh verdict on 2bf4534 with two blocking findings. Both verified real and fixed; new head is 91f596d (amended into the single commit, Premshay remains the git author, Kiro Crew added as co-author).
Tests added: test_rewind_rejects_when_the_save_is_refused (False return -> 503, slot intact) and test_rewind_reserves_the_slot_and_keeps_concurrent_queue_entries (slot.running True inside the awaited boundary; an entry queued mid-rewind survives the commit while snapshot entries are removed). Local gates all green: isort, flake8, mypy (1262 files), black baseline, pytest rewind 29 passed + session/context/chat 1122 passed. |
|
Kiro Crew [operator: dwu96#8c08bee0]: Stand-down + one correction. Correction to my First Principles disposition above (the Stand-down: this drive's premise (author inactive 7+ days) no longer holds — the author is actively pushing, and operator chenmingwei23 is mid-drive (both fresh GPT findings on Good luck with the remaining lanes. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 17: GPT round 3 on 91f596d raised three blocking findings. All three verified real and fixed; new head is 0baf550 (same single amended commit, Premshay remains git author).
Tests: three new cases (channel-linked app slot -> 404 with discard never awaited; aflush failure -> 503 with flush awaited once; mid-save rebind -> 503 rewind_slot_rebound with slot intact). docs/system-specs/modules/session.md updated to match (durable flush, rebind refusal, app link denial). Local gates green: isort, flake8, mypy, black baseline, 1154 backend tests across the four suites. Also for the record: the earlier CI red on this PR was two Windows shard flakes (rate-limiter timing tests and a perf-scaling assertion, no overlap with this diff); they passed on re-run attempt 3 with zero code change. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 20: Backend Tests (3.10, 2) on 0baf550 failed legitimately -- the error-code contract ratchet (test_error_code_contract.py) caps un-coded error responses in chat_rewind.py at 15, and the new app channel-link 404 made it 16. Fixed on 3ba66a5 by giving all three "not found" 404s in the file the shared code "slot_not_found" (same convention chat_regenerate.py uses); the three bodies stay byte-identical, so the anti-enumeration property is preserved -- a missing slot, an unowned slot, and a channel-linked slot remain indistinguishable to an app caller. The baseline regenerated per the anti-stale test (chat_rewind.py 15 -> 13, a net improvement; never raised). Contract test + rewind suite + full gate set green locally. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] GPT round 4 on 3ba66a5 raised two blocking findings. Both verified real and fixed; new head is 366d554 (same single amended commit, Premshay remains git author).
Tests: the flush-choreography test updated to the new save contract (live slot + expected_history_key -- its old mock asserted the prospective copy and deadlocked a worker thread once the contract changed), the rebind test now accepts either fence (save-side rewind_save_failed or commit-side rewind_slot_rebound; both mean the commit never happened), and a new 409 test locks in the busy refusal with discard asked exactly once with skip_if_busy=True and no flush afterwards. Local gates green: isort, flake8, mypy, black baseline, 33 rewind tests + 1128 across session/context/chat/error-contract suites. Convergence note: rounds 2-4 have produced 2+3+2 blocking findings, each round on the code the previous round added. All seven were verified real before fixing (none rebutted), so this is the review working, not thrashing -- but if round 5 blocks again on this round's additions I will escalate to the operator rather than continue expanding the diff. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Cycle 33: main moved again (#7855 chat-nav error codes, #7856 crew-companion escape-listener test fix) and the PR went CONFLICTING -- which also explains why no CI batch spawned on the previous two heads: GitHub creates no pull_request runs without a merge ref, only the base-context trio. Rebased onto 4b27ec9; the only conflict was error-code-baseline.json (generated file, resolved by regenerating on the rebased tree: chat_rewind.py 15 -> 13 entries preserved, totals now 1207). No source changes in this push beyond the rebase. All gates green locally (39 rewind+contract tests, 1122 session/context/chat, isort/flake8/mypy/black). New head c7d0980; PR is MERGEABLE again, CI batch spawned and approved. Note #7856 also fixes the CrewCompanionPanel Escape flake that hit this PR's frontend shard earlier. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] GPT round 5 on c7d0980: ONE blocking finding, verified real and fixed on 702f170 (same single amended commit, Premshay remains git author). Cancellation can commit a rewind without dispatching it. Correct: aiohttp cancels the handler task on client disconnect, and asyncio.to_thread propagates that cancellation while the worker thread finishes the destructive rewrite regardless -- leaving persisted history rewound, live slot state stale, and the edited prompt never dispatched. Fix per the prescribed shape: the save runs as a retained task awaited through asyncio.shield; on cancellation the handler awaits the worker's real outcome, and if the rewrite landed (and the slot still routes to the authorized transcript) it completes the same synchronous live-state commit the success path uses and lets the reserved dispatch task run the edited prompt, before propagating the cancellation. The commit mutations are extracted into one closure shared by both paths so they cannot drift; the only success-path extra is the best-effort orphan-file cleanup, deliberately skipped on the cancel path (cosmetic, kiro-cli GC reclaims it). Test added: gated save + handler-task cancel mid-save -> the landed rewrite is committed (slot window is the truncated prefix plus the edited row) and the dispatch task still runs the edited prompt. Local gates green: isort, flake8, mypy, black baseline, 34 rewind tests + 1128 across session/context/chat/error-contract. Convergence: rounds 2-5 produced 2 -> 3 -> 2 -> 1 blocking findings; round 5's finding is against the PR's original save-await structure, not the round-4 additions, and the trend is narrowing -- so this round was fixed rather than escalated. If round 6 blocks again, I stop and hand the thread to the operator with the full history. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] GPT round 6 on 702f170 posted two blocking findings. Disposition:
ESCALATION -- convergence history for the operator and the author: rounds 2 through 6 produced 2 -> 3 -> 2 -> 1 -> 2 blocking findings, all on this PR's successive additions; ten were verified real and fixed, none rebutted. Round 6's finding 2 contradicts round 4's demanded direction, which says the review has reached the boundary of what iteration inside this PR can converge on. State as of ceb01f8: CI fully green, Design Review CONCERNS (advisory), First Principles CONCERNS (advisory), UX skipped, Opus no blocking findings on the prior head, GPT blocking on the narrow rebind-during-write residue only. Options for a human decision: (a) accept the residue as documented behavior for this PR (it is strictly SMALLER than what the PR replaces -- main's rewind has no rebind fence at all) and override the lane; (b) scope a follow-up issue for serializing rebinding with persistence and land this PR with the residue documented; (c) have the author restructure persistence under the slot lock. I am not choosing among these unilaterally: (a) needs a maintainer override call, (b) and (c) change scope or design. Standing down per the drive-to-green escalation protocol. The drive-to-green label and work dir /tmp/kc-drive-5395 are left in place for whoever picks this up. |
Rebuild a rewound session from persisted retained history after clearing the native resume sid, so discarded turns cannot re-enter the replacement context. Maintainer drive on top of Premshay's rebase, folding in two GPT review rounds (five blocking findings total): - Reserve the slot before the awaited durable boundaries: publish the dispatch task (gated on an internal event) before the first await, so a concurrent send takes the queue path instead of starting a competing turn the commit would erase; the commit removes only the pre-await queue snapshot, and a rejected rewind hands arrived entries to the canonical queue drain. - Check _save_slot_to_history's return value: its guards refuse the write (False) when the session was deleted or rebound mid-save; rewind now 503s instead of dispatching from state that was never persisted. - Deny app-authenticated rewind of a channel-linked slot: its effective session is a foreign conversation the app does not own, and the rewind would clear that session's native identity (404, anti-enumeration). - Force the session-map durability point endpoint-side after the discard (state.sessions.aflush, 503 on failure), so a gateway exit before the debounced flush cannot resurrect the discarded conversation; the shared discard keeps its existing semantics for its other callers. - Refuse the commit when the slot was rebound to another transcript while persistence was in flight, so a late commit cannot overwrite state injected by a concurrent rebinding. The save itself goes through the live slot with expected_history_key, so a rebind visible at write time refuses the write before it can rewrite the former transcript. - Ask the discard with skip_if_busy: an inbound channel turn holds the session semaphore while slot.running reads False, and an unconditional discard would tear down its provider mid-reply; the refusal surfaces as a retryable 409 rewind_session_busy. - Give the three identical anti-enumeration 404s the shared machine code slot_not_found (error-code contract ratchet; baseline regenerated, chat_rewind.py 15 -> 13). - Shield the history save against handler cancellation: the worker thread finishes the destructive rewrite regardless, so a client disconnect mid-save now waits for the worker's outcome and completes the matching live-state commit (and the reserved dispatch still runs the edited prompt) instead of abandoning a landed rewrite with stale live state. Original change authored by Premshay. Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Operator ruling received: accept the narrow rebind-during-write residue for this PR (it is strictly smaller than main's rewind, which has no rebind fence at all) and clear the GPT lane. Mechanics on a FORK PR: the /ai-review override marker does not force-pass the Stage-2 fork lane -- it triggers a fresh review roll -- and the PR had also gone CONFLICTING again overnight, which voids any SHA-pinned override anyway. So the sequence is: rebase first (done -- new head 002066e on f8b3203; only conflict was the regenerated error-code-baseline.json; all gates green locally, 1164 tests), let CI conclude and GPT re-roll on this head, and only if GPT re-blocks on the same accepted residue post the override to realize the operator's disposition. Fork runs approved; guard label re-applied. |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Disposition of the two advisory CONCERNS verdicts on 002066e (no code push -- both lanes are advisory and the PR has converged; pushing would re-roll every lane): Design Review, both Watch items and the Suggestion (extract prepare/commit onto _ChatSlot; unify edit_resend): agreed on the merits, deferred as a tracked follow-up rather than folded in. The two-phase boundary grew handler-local across six review rounds because each round demanded a narrower fence at this call site; moving it onto _ChatSlot is a refactor of slot-owned persistence bookkeeping that widens this fix PR's blast radius and touches surfaces (regenerate, fork) this PR does not otherwise change. Captured as backlog item f-20260903-01: extract snapshot+adopt onto _ChatSlot, let the save report the bookkeeping it wrote, and migrate chat_regenerate's edit_resend (truncate-first, swallow-save-failure, no replay clear) onto the same boundary. The rewind docstring already documents the edit_resend divergence as accepted-and-deferred. First Principles, Watch item 1 (unfixed edit_resend sibling): same disposition -- accepted-and-deferred, tracked in the same follow-up above; the Pattern harvest sentence is the follow-up's charter, not a claim this PR fixed that path. First Principles, Watch item 2 (app/channel-link refusal appears only in session.md, not the description): confirming intent here on the record -- the refusal is intended API behavior. It was added for GPT round 3's blocking security finding (an app-owned slot with a linked_session_key would otherwise clear the native identity of a channel conversation the app does not own); the 404 shape matches the existing ownership check for anti-enumeration. Editing the PR body to declare it would re-trigger the edited-sensitive GPT lane on a converged head, so this comment serves as the declaration; the author can fold a line into the description at merge time if desired. |
|
/ai-review override gpt 002066e: Operator-accepted residue class: narrow races vs the non-atomic rewind rewrite (channel cold-start after discard; slot-close vs in-flight rewrite). Outcomes recoverable: suffix archived, canonical history retained per spec. Rounds 5-8 each landed a different variant prescribing feature removal or cross-module serialization, out of this fix PR's scope; serialization deferred to a tracked follow-up (f-20260903-01). |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
Human judgment recorded, but the following reviewer lane(s) could not be re-run automatically: GPT 5.6. Re-run the lane's latest workflow run for |
|
Kiro Crew [operator: chenmingwei23#pr-drive-5395] Superseded by #8145 (same-repo branch fix/rewind-context-boundary-mainline, identical commit content rebased onto current main; @Premshay remains the git author with a Kiro Crew co-author trailer). Why: this fork PR converged on the code -- 12 verified blocking findings fixed across six GPT rounds, CI green, Opus clean, Design/First-Principles advisory-only and dispositioned -- but could not converge on process. The recorded human override (marker target=gpt head=002066e35) is not honored by the fork review lanes, which re-review from scratch on every roll: two independent samples blocked on the identical accepted-residue pair, and every rebase (main moves several times a day; this PR went CONFLICTING again within hours) voids the SHA-pinned marker. Same-repo lanes honor the override mechanism, so #8145 is the deterministic path to done. Precedent: #5864 superseding fork PR #3420. This PR can be closed once #8145 lands; leaving that to the maintainer or the author. |
Open PR relationship auditThis 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. SUPERSEDED: closing this PRCurrent origin/main already contains this PR's entire change, and in a strictly better form: merged PR #8145 (merge commit 2a69ac9, an ancestor of origin/main) is the same single commit rebased onto a same-repo branch with Premshay kept as git author, touching the same 7 files, and its only code difference is a correction of a defect this PR head still carries (restoring pre-save persistence witnesses in _commit_live_state, which re-arms _pending_rewrite and moves _disk_tail_ts backwards). Rebasing this fork PR would produce an empty-to-worse diff: a read-only merge-tree against origin/main conflicts in chat_rewind.py, test_dashboard_chat_rewind.py and error-code-baseline.json, and resolving those conflicts means discarding the fork's side. The recorded reason the fork PR could not land itself is process, not code -- fork review lanes do not honor the human override marker and every rebase voids the SHA-pinned override -- and the drive operator's own note on 2026-09-03T09:47:49Z asks for closure once PR #8145 lands. The remaining follow-up work is tracked outside this PR: issue Issue #7838 (port the same boundary to edit_resend in chat_regenerate.py) and backl…
The audit recommendation is to close this PR; the relationship target above is the implementation to retain or the merged implementation that already covers it. No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation
Edit + Send visually rewound a dashboard conversation, but the next turn could still receive the discarded suffix through native ACP session resume, generic session-history reconstruction, or queued successors.
Why it matters
Editing a prompt must create a real conversation boundary: the replacement turn may use only the retained prefix and revised prompt, including after a process restart or in another open client.
What changed (motivation → approach → change)
The rewind request is now one-shot and is built only from the retained slot prefix. It clears the native conversation identity durably before saving the rewritten history, rejects the request if that boundary cannot be persisted, removes queued successors, and notifies other clients to remove their stale queue cards.
Tests
pytest -q test/test_dashboard_chat_rewind.py test/test_session.py -k "rewind or TestDiscardConversation"— 30 passedpytest -q test/test_context.py test/test_dashboard_chat.py -k "rewind or build_session_context or build_message"— 11 passedmypy— passednpm run build— passedops_mission_control, with none in changed paths.npm run checkexits non-zero after its passing dashboard suite because this Linux install lacks optional Electron packages required by unrelated Electron contract tests (electron,electron-updater,electron-store, andapp-builder-lib).Manual verification
The focused endpoint tests cover durable-boundary failure, native-session discard, retained-prefix reconstruction, and queue removal. The local gateway deployment is queued after this PR; manual Edit + Send verification remains appropriate for a live dashboard session.
Why no screenshot: No rendered UI changed; this fixes prompt/session state assembly.
Related Issues
no linked issue: no existing issue matches this bug fix
Pattern harvest
Rule candidate: review-prompt
Pattern: a destructive edit that removes content in one layer while the data stays reachable through a parallel replay channel (native session resume, history reconstruction, queued successors) -- every replay path must be cleared, durably, before the replacement is dispatched.
Checklist
main