feat(chat): merge a forked session back into its parent - #4904
Conversation
79bb2d5 to
347c713
Compare
347c713 to
be89cfa
Compare
be89cfa to
1eddcf3
Compare
1eddcf3 to
ef06640
Compare
ef06640 to
584654c
Compare
|
Round 32 disposition (fixes on F1 (security — app-id equality does not authorize the parent transcript): FIXED as suggested. The round-11 parent check compared F2 (security — fork title can escape the pending-context frame): FIXED, taking the omit option. The title is model/user-influenced text and was interpolated into the frame LABEL — ahead of the untrusted-data preamble that guards only the content. Verified: merge suite 55/55, black gate, flake8, mypy. |
|
Round 33 disposition (fix on F1 (fenced — crew completions stranded during merge): FIXED as suggested. The crew terminal marks the agent done BEFORE
Consolidation (announced in round 31, triggered by this fourth member): Tests: crew-pending → 409 → quiescent → same POST succeeds end-to-end; a direct orchestrator test pins both books of CI note: the Backend Lint & Type Check failure on the previous head does not reproduce locally — black gate, flake8 (full src), isort, and mypy over all 1303 source files all pass here; this push re-runs the job, and I will read its log if it stays red on an otherwise-green head. |
|
Round 34 disposition (fixes on GPT F1 (fenced — merge discards accepted pending context): FIXED as suggested, expiry-aware. Live Opus F1 (blocking — Backend Lint & Type Check (red on the last two heads): root-caused and fixed. CI's flake8 covers Merge suite 58/58, crew suite 164/164, black gate (crew_chat.py deliberately kept on the known-unformatted baseline — the counter edit is formatted in the file's existing style rather than graduating 170 unrelated lines into this PR), mypy clean. |
|
Round 35 disposition (fixes on F1 (fenced — reserved parent permits doomed auto-turn prompts): FIXED as suggested, both halves. A workflow completion's result row legitimately lands on a
Tests: a reserved slot queues the prompt (returns False, no user row, dispatch coroutine asserted never called); an end-to-end merge whose summarizer injects a queued prompt mid-reservation asserts the kick fires for the parent after a 200. The first version of the kick test parked the prompt BEFORE the merge and correctly got 409 F2 (fenced — paused workflow treated as finished): FIXED as suggested. Backend Tests (3.12, 4) red on the previous head: the failure is Merge suite 62/62, guardrail suites green, black gate, flake8 (CI scope incl. test/), mypy clean. |
|
Round 36 disposition (fixes on F1 (fenced — merging forks accept prompts that archival discards): FIXED as suggested, and it is a correction to my round-35 breadth. Round 35's queue condition used F2 (fenced — parent reservation strands subagent completions): FIXED as suggested. The subagent completion dispatcher's Tests: the round-35 unit test now also drives the round-36 fork branch (merging fork → The Backend Lint red on the previous head remains annotation-opaque (".github:16 exit 1") and does not reproduce locally against CI's exact flake8/black/mypy scope — I'll pull the completed run's log next cycle and root-cause it there if this head's run stays red. |
|
Status + CI fixes on All three AI lanes are GREEN on the round-36 head (
No production code changed beyond the isort reorder. If this head comes back clean, #4904 is fully green on rounds 26–36 inclusive. |
|
Status note — GPT formally CLEAR; rate-limited run retriggered The GPT lane's comment on the round-36 head ( No production or test changes on this push. |
|
Round 37 disposition (fixes on F1 (fenced — cron delivery to reserved parent silently discarded): FIXED as suggested. The cron-delivery idle branch in Proactive audit — rounds 21/35/36/37 are one recurring shape (a direct dispatcher missing the reservation), so instead of waiting for round 38 I enumerated every
The remaining audited sites are clean by construction: chat_runner's two dispatches run FROM the drain (now held); chat_handlers' site is the user-send path behind Tests: drain-hold test (reserved → False, entry NOT consumed); merge suite 63/63; cron routing 38/38; queue-family suite 34/34. Black gate, flake8 (CI scope), isort (src+test), mypy clean. Backend shard 2's |
|
Round 38 disposition (fixes on F1 (fenced — parent rebind bypasses app isolation): FIXED as suggested, both halves. The ownership gate authorized one observation of the parent's routing, then the 10–30s summarization await left a window where cron/workflow delivery could rebind
The recheck catches the 10–30s window cheaply; the pin closes the residual gap between recheck and flush atomically inside the guard the persistence layer built for exactly this race. Test: wrapping summarizer rebinds the parent mid-merge → 409 CI shard-4 red on the previous head was the doubles class again ( |
|
CI note on |
|
Re-sync #15 + shard-2 fix ( The shard-2 red on the previous head was the MagicMock-doubles class in a THIRD file: Merge suite 64/64; black/flake8 (CI scope)/isort/mypy clean. |
|
Round 39 disposition (fixes on F1 (fenced — bulk cleanup can remove a reserved parent mid-merge): FIXED as suggested, both halves.
Sibling audit (the round-37 lesson applied proactively): every other Test: reserved parent + merging fork + plain stale slot, all 10 days idle → cleanup archives ONLY the plain one, both transition slots survive with their flags intact. Cleanup + merge + queue-drain suites 76/76. Black gate (one wrap in the new test, applied), flake8 (CI scope), isort, mypy clean. |
|
Coverage Gate fix ( |
|
Round 40 disposition (fixes on F1 (fenced — rollback can discard a concurrent parent delivery): FIXED as suggested. The tension is between two deliberate designs: the round-25 rollback unpublishes a parent this call rehydrated when the merge fails pre-commit, and the round-6 reservation deliberately keeps accepting one-shot background delivery appends on that parent (only its TURNS are blocked). A heartbeat/cron row landing during the summarization await therefore lives only in memory on the freshly rehydrated slot — and the unconditional pop discarded it. The pop is now gated on an unchanged rehydrated baseline: Tests: (1) delivery lands mid-summarization + summarizer refuses → 409, parent survives carrying the delivered row, fork untouched/retry-safe; (2) counterpart — no delivery, pre-commit failure → parent still rolled back exactly as round 25 specified. Merge suite 67/67; black/flake8 (CI scope)/isort/mypy clean. For the record: the rollback family is now triple-guarded — creation witness (r26: only pop what this call minted), commit witness (r27: never pop post-commit), unchanged baseline (r40: never pop new content). |
|
Re-sync #16 ( Verified: tsc, 84 merged-fork/menu frontend tests, all 10 i18n gates, backend merge suite 67/67 (including the round-40 rollback-baseline pair), black/flake8 (CI scope)/isort clean. The round-40 content is unchanged — this head is that fix rebased. |
|
Re-sync #17 ( |
|
Round 41 disposition (fixes on F1 (fenced — parent reservation can attach to a closed, stale slot): FIXED as suggested. Parent resolution runs before the lock acquire, so a close (or variant switch) can pop the parent from the registry in that window; the round-25 lock-atomic reservation then landed on the popped OBJECT, and the merge's durable save would write the closed session back open. The reservation block now verifies Chose abort-retryably over the suggestion's re-resolve alternative deliberately: re-resolving under the lock would attach the merge to a slot object the round-13/32 authorization gates never examined. Test: hold the parent's lock, start the merge (blocks at acquire), pop the parent and publish a replacement under the same key, release → 409 |
|
Round 42 disposition + re-sync #18 (fixes on F1 (fenced — script cron result lost during fork merge): FIXED as suggested at BOTH delivery sites. Round 37 taught the cron dispatchers about the reserved PARENT; this round is the FORK side of the same delivery: a merging fork's transcript is frozen by the round-28 write gate, so a direct append raises Doubles: the two new flag reads were modeled ( Re-sync #18 folded into this push: main moved again; single conflict was (The adjudication section of the review comment shows an infra error this round — "Claude Code 2.1.240 does not support this model" — so the verdict stood unadjudicated; noting for the maintainer, fixed on merit regardless.) |
|
Round 43 disposition (fixes on F1 (fenced — merge can discard accepted parent context): FIXED as suggested. The round-6 model-visibility enqueue pushes the merge summary through the parent's pending-context buffer, which FIFO-evicts at The preflight sits immediately before the commit (the block append + durable save), per the suggested shape: expiry-aware live count ( Test: parent filled with 50 live entries → 409 (Adjudication lane errored again this round — same "Claude Code 2.1.240" infra failure as round 42 — so the verdict stood unadjudicated; fixed on merit.) |
|
Round 44 disposition (fixes on F1 (fenced — reserved parents still accept context writes): FIXED as suggested. Round 43's capacity preflight counts live entries immediately before the merge's commit, but The delivery/merge context family is now closed at every seam: producers gate on merged/merging (r24) and reserved (r44), the merge preflights capacity pre-commit (r43), delivery dispatchers park or fall back (r35–r42), and the drain holds during reservations (r37). Windows shard-2 red on the previous head was the doubles class again: Tests: reserved parent POST /context → 409 |
|
Round 45 disposition (fixes on F1 (fenced — reservation check races the offloaded deletion): FIXED as suggested, full claim protocol. The deletion paths' round-5/22 reserved-keys checks run on the event loop, but the unlink itself is offloaded to a thread — a merge could reserve-and-save in that gap, and the deletion then unlinked the transcript the merge had just durably written (fork archives, summary lost). The suggested three-part protocol is implemented exactly:
Both spellings are claimed for the same reason the merge reserves both (round 6): the History surface and Test: a claim on the parent's history key → merge answers 409 (Adjudication infra note: #9216 has been filed and claimed upstream for the "Claude Code 2.1.240" adjudicator failure — rounds 42–45 stood unadjudicated.) |
|
Round 46 disposition (fixes on F1 (fenced — failed nested merge resurrects an archived parent): FIXED as suggested. The round-25 rollback wrapped only the transition body's response, so a refusal issued AFTER the rehydrate but BEFORE the transition — the busy/merged 409 on an archived merged parent (the named case), the round-32 app-ownership 404, and the round-45 deletion-claims 409 — left the rehydrated session published: the failed merge's one visible effect was reopening a session the user had archived. The rollback is now a shared helper ( Test: archived MERGED parent + child merge → 409 Re-sync #19 folded in (clean, no conflicts). Note on the previous head's Windows shard-1 red: |
|
Round 47 disposition (fixes on F1 (fenced — overlapping deletes can release another request's claim): FIXED as suggested. The round-45 claims were a plain set, so two concurrent deletes of the same key each
Tests: the new refcount test walks the exact failing interleaving — two overlapping claims on one key, first release leaves the key CLAIMED, last release clears it, over-release is a no-op; the round-45 merge-refusal test updated to the mapping shape. Merge/deletion suite 76/76, sessions memory+clear 26/26; black gate, flake8 (CI scope), isort, mypy (3 files) clean. (Windows shard-4 red on the previous head was |
|
Round 48 disposition (fixes on F1 (fenced — overlapping merges can prematurely release a shared reservation): FIXED as suggested (refcount option). F2 (fenced — receipt reuse accepts corrupted summary content): FIXED as suggested. The Tests: (F1) the exact interleaving — shared key, two reservations, first release leaves it protected, over-release no-op; (F2) commit + archive-fail, tamper the block body, retry → fresh summarization runs (counted) instead of reuse; the intact-receipt reuse test still passes unchanged. Merge/deletion/receipt suite 82/82, sessions 26/26; black gate, flake8 (CI scope), isort, mypy (3 files) clean. |
iamwhatever
left a comment
There was a problem hiding this comment.
Merge-back is terminal, and the fork loop dead-ends
The design ruling in #3816 (archive the fork after merging) makes merge-back a one-way door, and I think the resulting lifecycle is narrower than the "branch out, conclude, fold back in" loop the description promises. Three concrete points:
1. A merged fork cannot be continued at all. After the merge, merged_slot_response refuses send / continue / regenerate / variant switch / edit-resend / rewind with 409 session_merged, and _ChatSlot.append refuses live rows outright. So the natural follow-up question — "I merged, then kept working in the fork, how does the new part get folded in?" — has no answer: there is no new part, because the fork is frozen. Worth stating explicitly in the description, since "archived" reads as "closed but resumable" and this is stronger than that.
2. Re-merging is impossible, and forking around it dead-ends with a misleading error. _merge_back_locked returns 409 already_merged for any merged fork outside the _archive_pending retry window, so a second merge is out. The obvious workaround — fork the archived fork and keep exploring — produces a slot whose forked_from points at the merged fork, and merging that back hits the parent guard in api_chat_slot_merge_back:
if (
getattr(parent_slot, "running", False)
or ...
or getattr(parent_slot, "_merged", False)
):
return web.json_response(
{"error": "the parent session has a turn in progress; retry when it finishes",
"code": "parent_busy"},
status=409,
)A permanently merged parent is bucketed with transient busy states and the user is told to "retry when it finishes" — it never finishes. Please split this into its own code (e.g. parent_merged) with copy that says the parent is archived and cannot receive another merge. As it stands, the fork chain cannot repeat and the failure mode lies about why.
3. The one shot can silently lose its value on restart. The known limitation already noted in the description — merged_summary is not in RECALL_ROLES, so the parent's cold replay after a gateway restart does not re-inject the summary — compounds with (1) and (2): the user gets exactly one merge, and after a restart that merge is presentation-only. One-shot plus non-durable-to-the-model is a rough combination for a feature whose whole point is not losing fork context.
Suggested direction
Preferred: don't archive on merge. Keep the fork writable, and make merge incremental — merge_key is already content-keyed and _post_fork_range already computes a range, so a second merge can diff from the last merge point and append a second card. That is the loop the description describes, and most of the plumbing is here already.
If v1 must stay terminal, then at minimum:
- give merged-parent its own 409 code and accurate copy (point 2 — this one is a bug, not a scope call);
- say "this fork becomes read-only and cannot be continued or merged again" in the confirm dialog, not just in the PR body.
|
Re-sync #20 — main moved (~40 commits, to For the record: the previous head |
|
Thanks — taking this in three parts (fixes pushed in Point 2 ( Point 1 (frozen is stronger than "archived" reads) — dialog copy strengthened. The confirm now says: "This is one-way: the fork becomes read-only and cannot be continued or merged again." (It previously said only "will become read-only".) I'll mirror the same sentence into the PR description's lifecycle section. Point 3 + suggested direction (incremental merge instead of archive-on-merge) — agreed this is the better loop, but it reverses the #3816 design ruling (archive the fork after merging), which this PR implemented deliberately. You're right that the plumbing is close: |
|
Round 49 disposition (fix on F1 (fenced — reserved-parent prompts are silently discarded): TAKEN, via the "reject visibly" arm of the suggested fix. The turn gate is the funnel every dispatch shape passes through, and a prompt that reaches it was already ACCEPTED — its user row appended by the caller — so the silent return stranded linked-Slack and OpenAI-compat surfaces exactly as described: no reply, no completion. The gate now refuses visibly, mirroring the refused-remote-slot shape directly above it: on a reserved parent (which is writable) it appends a retryable error row ("a fork is being merged into this session right now … send it again in a moment") and broadcasts Why not the "queue" arm: the queue drain re-appends its message when it dispatches (that is how parked deliveries work), so re-queueing a prompt whose row is already in the transcript would double the user row; and the reservation window is the summarization pass (seconds), so a visible retryable refusal is the honest shape rather than a hidden delay. The r37 park-in-queue behavior for dispatcher-routed deliveries is unchanged — this round only covers the direct-funnel paths that bypass those dispatchers. Also this push — shard 2 red (both OSes) taken with the same seam: Tests: new |
|
Re-sync #21 ( |
POST /api/chat/slots/{slot}/merge-back on a fork resolves the parent from
forked_from, summarizes the fork with the existing session summarizer, and
appends a visible merged_summary block to the parent's tail (with a gap
note when the parent advanced). The fork is archived: merged/merged_into
metadata implies closed, restore paths keep it readable but non-continuable,
and api_chat rejects turns on a merged slot. UI: MergedSummaryCard renderer,
a merge-back action on the fork breadcrumb, i18n in all locales.
Hardened per local review (GPT + Opus mirrors):
- both transcript reads run via asyncio.to_thread (no event-loop stall)
- the transition is serialized on the slot's _fork_lock; a stale summary
(refused/failed regeneration) is rejected rather than committed
- archive failure keeps _merged and marks _archive_pending so a retry
re-runs ONLY the archive - the parent can never gain a duplicate block
(belt-and-braces: an existing block for the same fork is never re-appended)
- the parent block is broadcast only after the durable save confirms
- _save_slot_to_history folds slot._merged into closed on periodic saves
so a resumed merged fork cannot be revived as an open tab
Implements the maintainer-approved design on issue kirodotdev#3816: visible block,
archive after merge, summary only, append at tail with gap note.
Problem / Motivation
Forking a session is a one-way door. A fork exists to try something without polluting the parent — a side investigation, a risky refactor conversation, an alternative approach — but once the experiment pays off there is no way to bring its outcome home. The user is left with two permanently diverged sessions: the parent never learns what the fork concluded, and the fork lingers in the open-tab list as a stale near-duplicate that has to be closed by hand and re-found in History later.
Why it matters
Fork-heavy workflows (issue #3816) currently punish exactly the users who adopt them: every successful side-quest costs a manual copy-paste of its conclusions back into the parent plus tab housekeeping, and every skipped copy-paste means the parent's later turns proceed without context the user already paid tokens to produce. Merge-back turns the fork lifecycle into a loop — branch out, conclude, fold the conclusion back in — which is what makes forking safe to use routinely.
What changed (motivation → approach → change)
Goal: fold a finished fork back into its parent. The maintainer ruled on the design questions in the issue thread: a visible marked block (not silent context injection), summary only (not raw transcript replay), appended at the parent's current tail with a gap note when the parent advanced, and the fork is archived after merging. This PR implements exactly that shape, reusing existing machinery at every step rather than inventing parallel paths:
src/kiro_crew/dashboard/chat_merge_back.py(new) —POST /api/chat/slots/{slot}/merge-backon a fork. Resolves the parent fromforked_from(open slot preferred so the block renders live, else rehydrated from History; handles channel-origin keys), summarizes the whole fork transcript with the existing on-demand session summarizer (force=True; the summary is read back from the sidecar rather than trusting the generator's bool), computes the fork point via an identity-based longest-common-prefix scan (per-messagemidpreferred) to produce themessage_rangeand the "parent advanced N message(s)" gap note, appends amerged_summaryblock at the parent's tail, and archives the fork._fork_lock; both transcript reads run viaasyncio.to_thread(the AUTOSDE no-blocking-call-on-event-loop rule); a stale summary (refused/failed regeneration over newer persisted turns) is rejected with 409 rather than committed; the parent block is broadcast only after its durable save confirms (no phantom rows in open tabs); and an archive-save failure keeps the merge fact (_merged=True+_archive_pending) so a retry re-runs only the archive step — the parent can never gain a duplicate block. A cross-restart belt-and-braces scan of the full on-disk parent transcript covers the marker-loss case.chat_persistence.py/history.py/state.py—merged/merged_into/merged_atbecome slot-owned metadata;mergedfolds intoclosedon every save (including periodic flushes of a History-resumed merged fork, which previously would have dropped the closed marker and revived the archived fork on restart), so restore paths keep a merged fork readable but non-continuable with no new restore-path code.chat_handlers.py—api_chatand the resume path refuse turns on a merged slot, making the archived fork read-only.MergedSummaryCardrenders the block (GitMerge icon, fork title, localized gap note, full-fork-coverage label) in both transcript surfaces (transcriptRenderers.tsxregistry +ChatPage's renderer chain); the fork breadcrumb inChatPanegains a confirm-gated "Merge back" action (hidden once merged);mergeBackChatSlotAPI client method +mergeBackmutation inuseSessionActionsthat switches to the parent on success and maps the endpoint's distinct 409 codes to specific messages.en-XAregenerated.Restructure round (head
0cdbe70ad, after three rounds of blocking findings in the same span): enforcement moved from per-endpoint checks to the mutation boundary —_ChatSlot.appendrefuses live rows on a merged/merging fork (SlotMergedError),_run_chatrefuses turns at entry, and a sharedmerged_slot_response409 gate now covers send, continue, regenerate, variant switch, edit/resend and rewind; the parent is reserved (_merging) for the whole transition with a fastparent_busy409 before the summarization spend; merge idempotency is content-keyed (merge_key= hash of the fork snapshot,content_sigtamper check,merge_receipt_corrupton mismatch) instead of positional; the pane header's split/close icons collapsed into one overflowDropdownMenuso the row stays under the two-control cap with the fork breadcrumb trigger; the breadcrumb pill itself shows "Merging…" while pending. Subtractions per review: persistedgap_notefallback,merged_intosnapshot field,retried:archiveresponse key, and the never-writtenmerged_atmeta key are gone.Review-round changes (heads
584654cab→8055e7287): the fork is held read-only for the whole merge transition (_mergingflag + locked re-check of running/queued work); the parent-block idempotency scan is range-matched so a stale block from a pre-restart merge is never accepted as current; the parent's delivery queue is no longer drained (only the block's own pending entry is removed); the summaries-disabled gate is lifted for this one explicit call so the action works on a default install; the 200 body is trimmed to{ok, parent_key};archive_failed/parent_save_failed/parent_missing/merge_in_progressmap to distinct localized alerts; the merge button shows a pending state; the gap note is built client-side from a structuredadvancedcount (i18next plurals, 12 catalogs).Mechanical ride-alongs: the locale catalogs re-sort (output of the repo's i18n sort gate) and one
routes/chat.pyline reformat (formatter gate) — no behavior change in either.Round 7 (head
3ed7976bf): the parent-close race is fully closed — the sharedclose_slotteardown now refuses (409merge_in_progress) on the parent's_merge_reservedreservation as well as the fork's_merging, so a close raced against the merge's durable save can no longer be silently undone; resume sets themergedread-only flag AFTER the history replay loop (flag-first tripped the append write gate and made every merged fork unreadable from History); the merge-back error alert mapsfork_flush_failedandnon_persistent_sessionto accurate localized copy instead of the speculative "parent may be gone" fallback; "Open parent" resumes a closed parent through the History resume path (single view AND split-pane collapse) instead of silently 404ing; the module spec (docs/system-specs/modules/learn-cron-dashboard.md) documents/merge-backand the slot-ownedmergedmeta key; and nine unrelated main-side screenshot files the rebase had silently deleted are restored.Known limitation (v1 scope): the merged summary reaches the parent's next turns via the in-memory pending-context queue; the persisted
merged_summaryrow is presentation-only (it is not inRECALL_ROLES), so after a gateway restart the parent's cold replay does not re-inject the summary into the model — the visible card remains. Making the persisted row recall-eligible touches the recall policy for all roles and is deferred to a follow-up rather than smuggled into this PR.Tests
TestMergeBackSlot(14 tests,test/test_dashboard_chat.py): head-fork happy path (block content, range, archive); tail-fork range; gap note when the parent advanced; parent-not-open rehydration; double-merge 409; running-turn 409; stale-summary 409; summary-unavailable 409; not-a-fork 409; unknown slot / missing parent 404s; non-persistent 400; parent-save-failure 503 with clean rollback; archive-failure retry (503archive_failedkeeps_merged, retry archives without duplicating the parent block).test_merged_flag_folds_into_closed_on_periodic_savelocks the persistence fold.i18n:checkgreen.tsc -b, vitest.Manual verification
Local review round (GPT + Opus mirrors of the repo's CI review contracts) found 5 blocking/advisory defects — all fixed and re-verified by a focused verification pass confirming each closed with no new Critical/High. UI screenshots below were captured from a seeded dev instance (same-origin against the built dist); component behavior is additionally locked by tests.
Screenshots / video
Captured from a seeded dev instance (parent "Retry logic investigation" + fork "Alternative cap design").
Parent session — the
MergedSummaryCardblock appended at the tail (fork title, summary body, italic gap note from the structuredadvancedcount):Fork pane (Split View) — the breadcrumb pill is the dropdown trigger holding "Merge back" (no extra header-row button; menu open):
Related Issues
Fixes #3816
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)