feat(instances): carry the kiro-cli context so a sent session resumes - #2260
Conversation
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @iamwhatever overrides the GPT 5.6 finding for 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: |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound root-cause fix with honest degradation, but Layer B egress silently exits the repo's credential-redaction floor, and the diff's own docstrings still claim otherwise. Watch
Suggestions
[DESIGN-REVIEWED] a8a7cec |
UX Review (Fable 5) — 🟡 CONCERNSAdvisory UX-level review of UX-Verdict: CONCERNS A downgraded send to an older peer is transcript-only and the sender knows it — yet the row still shows the plain green "Sent" this PR exists to kill. Watch
Suggestions
[UX-REVIEWED] a8a7cec |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've reviewed the complete diff. Let me verify the two redaction-related comments against the actual code behavior, since that's the crux of this PR's deliberate design. The core source changes are in My analysis:
The stale docstring/comment claims that Layer B events are "egress-redacted" / "re-redacts the events (ingress)" contradict the byte-exact code, but that is a documentation inaccuracy with no runtime consequence chain to user/system harm — below the finding bar. No findings. [OPUS-REVIEWED] a8a7cec Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
f0f96dc to
3f09b06
Compare
3f09b06 to
f7af6a2
Compare
f7af6a2 to
7b12a03
Compare
Dispositions — prior SHAs
|
7b12a03 to
c24bd38
Compare
Dispositions —
|
c24bd38 to
a05580f
Compare
043b5d2 to
66b1ebd
Compare
Disposition —
|
66b1ebd to
aa6b2e2
Compare
Dispositions —
|
aa6b2e2 to
8091b0d
Compare
Advisory dispositions — Design & UX
|
8091b0d to
92234de
Compare
|
Both blocking findings were legitimate. Fixed at 1. Size cap checked after loading the whole event log ( Two notes on the fix beyond what was reported:
The test pins the ordering, not the return value — a post-read check also returns 2. Imported context uses default filesystem permissions ( Two deliberate narrowings, both pinned by tests:
Third issue, not reported, found while fixing #2. The permissions site used a hand-rolled Gates: 170 backend (was 165 — 5 new tests), flake8 / isort clean. Unchanged: the two-instance smoke test is still the open merge gate — neither fix is reachable by the unit suite end-to-end. |
|
Rebased onto BLOCKING — FINDING — I first tried gating on
Worth noting for the record: SAST (Semgrep) — Gates: 174 backend (was 170), Unchanged: the two-instance smoke test remains the open merge gate. |
|
Both findings from BLOCKING — Fixed by keeping in-progress slots counted, not by reverting the hunk — the retraction is load-bearing for an earlier blocking finding (a slot reachable mid-construction cold-starts a fresh context that the join written afterwards can never attach to). Two things beyond the report:
FINDING — Note for reviewers, since it affects test fidelity: this suite's Gates: 3045 passed / 8 skipped across the slot / dashboard / fork / transfer / state suites (730 in the fork+transfer set specifically), flake8 / isort clean. Unchanged: the two-instance smoke test remains the open merge gate. |
|
Rebased onto BLOCKING —
Now fails closed: both files are deleted (the pair is useless alone, and the Also worth stating plainly, since it was the stated reason for this round: there was no frontend-test failure to fix. CI was green on Gates: 179 backend (was 174), Unchanged: the two-instance smoke test remains the open merge gate. |
Layer B now travels byte-exact — reversing an earlier finding on this PR, with measurementsPushed as What was wrongAn earlier round of this PR asked for the Layer B envelope to be redacted on egress, and I implemented that (plus a per-record JSONL redactor on both boundaries). Investigating whether a sent session can actually resume on the peer, I replayed the full egress+ingress pipeline against this developer machine's 704 real kiro-cli sessions:
That one field is It is a provider signature over the thinking content, validated when the conversation is replayed. The consequence: the peer's A signature is not a credential. Redacting it protects nothing (it is a MAC over content the peer already receives) and destroys the artifact. Redaction and transplant are mutually exclusive here; the PR was trying to do both, and the defensive half was silently breaking the functional half. Why no test caught itEvery Layer B test used a synthetic What changed
Why byte-exact is acceptableThe bound is the destination, not the payload. A send goes hub → the operator's own peer instance, over a tunnel that operator authenticated, and the peer stores the files
VerificationSame 704 sessions, round-tripped through the current code (real egress read → real ingress write): New regression test Gates: 172 targeted + 1213 across the transfer/posture/slot/fork suites, flake8 / isort clean. Unchanged: the two-instance smoke test remains the open merge gate — and it is the only thing that can prove resume end-to-end, since even this verification stops at "the bytes kiro-cli would read". |
rebuilds it as a tab. That copy cannot continue where it left off. The peer shows every turn, but the next prompt starts from a condensed ~8K text prefix rather than the context the model was actually holding, so long or compacted sessions silently lose their working context on arrival. A session is two stores. Layer A is the display transcript (<data-home>/sessions/<key>.jsonl) -- what #1744 sends. Layer B is the context the model actually holds: kiro_sessions_dir()/<sid>.{json,jsonl}, outside the crew home, joined to a slot via session_map.json. With Layer A alone SessionMap.get finds no usable sid on the peer and the next turn falls back to _build_history_prefix() -- no tool state, no real context window. bundle_version 2 carries an optional layer_b, and the importer materialises it and writes the session_map join, which is also what auto-disables the prefix fallback -- so resume goes through session/load at the same fidelity as a local gateway restart. Properties that matter: - Optional. A v1 sender, or a session with no kiro-cli context, ships Layer A only. Both versions stay accepted ({1, 2}) so a v2 instance can still receive from a v1 one, and send_session_bundle downgrades once to v1 when a peer refuses v2 -- gated on the version, not on layer_b presence, since a context-free session ships v2 with no layer_b at all. - Rewritten, never replayed. Fresh sid (copy-never-move; a repeat send cannot collide), cwd and filesystem allowed_*_paths cleared (matching the existing project decision -- the session arrives unscoped), agent_name set to the target-resolved agent, conversation_metadata kept byte-for-byte because that IS the resumable context. - Threading contract honoured. All SessionMap access stays on the event loop (see subagent.py's contract: the map is an unlocked dict with whole-file saves): _resolve_layer_b_sid and _join_layer_b run on the loop, while _write_layer_b_files / _unlink_layer_b_files do blocking file IO in a worker and never receive a handle to the map. - The imported slot is unreachable until it is correct. get_or_create_slot registers it in state._slots and publishes it before returning, so it is retracted immediately and re-registered + published once, at the end. Layer B lands before the transcript work, and both failure paths undo the join and delete its files. - Redacted both directions, structure-preservingly. The events blob and the envelope both cross the host boundary and can hold a credential the redactors never saw, so both are scrubbed on egress and again on ingress. - Resume fidelity is reported end to end: import returns resume_mode (session_load|prefix), send-session forwards it, and the row renders "Sent (transcript only)" so a degraded copy is never shown as a full one. Memory, project/repo and sub-agent conversations deliberately do not travel; sub-agent results already live inside Layer B as injected context. Direction is hub -> peer. Tests: 150 in test_session_transfer.py plus security-posture and error-code-contract; 607 frontend. Two-instance smoke test still outstanding.
|
Five blocking findings on FIXED — FIXED — FIXED — REBUTTED — Two pieces of evidence:
OVERRIDDEN —
What bounds the exposure is the destination, not the payload: a send goes to the operator's own peer instance, over a tunnel that operator authenticated, stored The formal override is posted separately, per-SHA. Gates: 175 targeted, 1216 across the transfer / posture / slot / fork suites, flake8 + isort clean. Still the open gate: the two-instance smoke test. Worth recording why it has not run here — |
|
/ai-review override gpt a8a7cec: Layer B is forwarded byte-exact by the repo owner's explicit decision — its thinking-block signatures are validated on replay, so redacting it corrupted the conversation in 41% of 704 real sessions, and the destination is the operator's own peer over a tunnel they authenticated. |
Human judgment recorded@iamwhatever 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. |
Follow-on to #1744 (merged), which shipped session transfer: a peer receives the visible transcript and rebuilds it as a tab. This PR adds the second half so the copy actually resumes. Now that #1744 has landed, this branch is rebased onto it and contains only the Layer B delta (2,577 / −57 across 26 files).
Original transcript-transfer work is @CrysisDeu's; this builds on it.
Problem
A session sent to a peer arrives as browsable history that cannot continue where it left off. Every turn is displayed, but the next prompt starts from a condensed ~8K text summary rather than the conversation the model was actually holding — so long or compacted sessions silently lose their working context. The tab looks complete, which is what makes it dangerous.
Why it matters
The point of moving a session to the machine with the repo and the cores is to keep working. If the agent arrives without the real context window, the user re-establishes it by hand — most of what they were trying to avoid — and only discovers the loss mid-task.
Fix (symptom → root cause → change)
Symptom: an imported session displays fully but resumes from a lossy summary.
Root cause: a session is two stores, not one.
<data-home>/sessions/<key>.jsonl. This is what feat(instances): send a copy of a session to another instance #1744 sends.kiro_sessions_dir()/<sid>.{json,jsonl}, which lives outside the crew home and is joined to a slot throughsession_map.json.With Layer A alone,
SessionMap.getfinds no usable sid on the peer, so the next turn falls back to_build_history_prefix()— a ~8K-char text prefix with no tool state and no real context window. Nothing was broken; the second store was simply never in the bundle.Change:
bundle_version2 carries an optionallayer_b, and the importer materialises it and writes thesession_mapjoin — which is also what auto-disables the prefix fallback, so resume goes throughsession/loadat the same fidelity as a local gateway restart.Properties that matter
Optional, and backward-compatible in both directions. A v1 sender, or a session that never opened a kiro-cli context, ships Layer A only. Both versions stay accepted (
{1, 2}) so a v2 instance can still receive from a v1 one, andsend_session_bundledowngrades once to v1 when a peer refuses v2 — gated on the version, not onlayer_bpresence, because a context-free session ships v2 with nolayer_bat all.Host-naming fields rewritten; the conversation byte-exact. Fresh
sid(copy-never-move holds; a repeat send cannot collide),cwd+ filesystemallowed_*_pathscleared (matching feat(instances): send a copy of a session to another instance #1744'sprojectdecision — the session arrives unscoped),agent_nameset to the target-resolved agent, timestamps refreshed. Everything else —conversation_metadataand the whole events blob — travels byte-for-byte, which is forced rather than stylistic; see Layer B travels byte-exact below.The repo's
SessionMapthreading contract is honoured. All map access stays on the event loop (subagent.py: the map is an unlocked dict with whole-file saves, so worker-thread access races cold-starts)._resolve_layer_b_sidand_join_layer_brun on the loop;_write_layer_b_files/_unlink_layer_b_filesdo the blocking file IO in a worker and never receive a handle to the map.The imported slot is unreachable until it is correct.
get_or_create_slotregisters the slot instate._slotsand publishes it before returning, so it is retracted immediately and re-registered + published once, at the end. Layer B lands before the transcript work, and both failure paths undo the join and delete its files.Layer A is redacted on egress and ingress; Layer B is not. Assistant content, the title and the origin label are scrubbed on both boundaries (the sender is not trusted). Layer B is exempt because redacting it corrupts it — see below. Inbound Layer B is still validated structurally (parse-only, never rewritten).
Failure paths leave nothing behind. Files land
0600owner-only and fail closed if the lockdown cannot be applied; a failure on the second of the two writes unlinks the half-pair;CancelledError(aBaseException, so the ordinary handler never saw it) rolls back the join and the files before re-raising; and the size cap is checked withst_sizebefore the read so an oversized log cannot be allocated at all.The slot cap holds across both of its windows. Slots retracted for construction stay counted (
DashboardState.live_slot_count(), shared with the fork path), and the cap is re-tested immediately before creation with noawaitin the gap — body parsing and agent resolution both yield, so the entry check alone admitted every concurrent import near the cap.Resume fidelity is reported end to end. Import returns
resume_mode: session_load|prefix,send-sessionforwards it, and the sender's row renders "Sent (transcript only)" instead of a green "Sent" — so a degraded copy is never mistaken for a full one. Older peers report""and stay plain "Sent".Degradation is disclosed where it is felt, not only where it is sent. The sender's row is component state in a menu that closes; the consequence is discovered later on the receiving machine. So an import that arrived without resumable context also marks its own tab title
— transcript only, which persists with the session.An unparseable Layer B record refuses the whole blob rather than shipping malformed JSONL that would make the peer's
session/loadfail after this side already reportedsession_load.Still deliberately not travelling
spawn_continueagainst one specific sub-agent is lostDirection stays hub → peer; bidirectional is the natural follow-on.
Layer B travels byte-exact (design change, a8a7cec)
Layer B is forwarded unredacted, and that is forced rather than lax. The
envelope's thinking blocks carry a provider
signatureover their own content,validated when the conversation is replayed — so rewriting any covered byte makes
the peer's
session/loadsucceed and its next turn fail, a failure thatsurfaces far from its cause.
This was not hypothetical. An earlier revision of this PR redacted Layer B on both
boundaries; replayed against one developer machine's 704 real sessions, that
pass altered a thinking signature in 286 of them (41%). After the change, the
same 704-session round-trip alters 0 events blobs and 0 of 1619 signatures.
Redacting this artifact and transplanting it are mutually exclusive. What bounds
the exposure is the destination, not a scrub of the payload: a send goes to the
operator's own peer instance, over a tunnel that operator authenticated, and
the peer stores it
0600— Layer B never leaves the operator's trust boundary.Layer A keeps its redaction (it is rendered in a transcript and re-read by an
agent as context). Inbound Layer B is validated structurally — parse-only, never
rewritten — and refused whole if any record fails to parse.
This deliberately reverses an earlier review finding on this PR that asked for the
envelope to be scrubbed on egress.
security_posture.pyandinstances.md§14.1anow record the byte-exact stance and its reasoning.
Tests
127 in
test/test_session_transfer.py(175 with thetest_security_posture.py+test_error_code_contract.pygates; 1,216 across the transfer / posture / slot / forksuites), plus the frontend submenu spec:
cwd/allowed_*_paths, target agent, source path absent from the whole envelope, caller's dict unmutated, minimal envelope toleratedtest_import_preserves_the_thinking_signature_verbatimasserts on the file bytes kiro-cli will read, not an in-memory dict, and a shared_THINKING_ENVELOPEfixture replaces the empty-{}envelopes that let signature corruption pass unnoticedread_textto prove an oversized log is never allocated, since a post-read check also returnsNoneand would pass against the bug0600, the created dir0700, a pre-existing kiro-cli dir left untouched, and Layer B discarded entirely when the lockdown fails_read_layer_btakes only asid,_write_layer_b_filestakes nosessionshandle, the module cannot importSessionMapstate._slotsfor the whole build and present exactly once after; Layer B lands before the save; a failed save rolls back the join and its fileslayer_brejected with machine-readable codes; v2-without-layer_b still downgrades; downgrade fires at most onceresume_modereported assession_load/prefix/prefix-for-v1; the row renders a distinct transcript-only state; and the tab is marked when the sender withheld context it had (layer_b_skipped), which validation carries throughManual verification
Outstanding — this is the merge gate. The resume path has not been exercised
against two live instances. Worth recording why it cannot be automated here: a
pod gets its own empty
KIRO_HOME(pod/runtime.py:702), so a pod's kiro-cli isunauthenticated and cannot run a turn, and
kiro-cli chatcannot create a v2session at all (
--session-sourceonly pairs with--delete-session) — so no pod-or CLI-only harness can even produce a source session. It needs two real gateways
with an authenticated kiro-cli; the repo owner is running that.
Verified deterministically instead, on 704 real local kiro-cli sessions rather
than fixtures: the full egress + ingress pipeline round-trips every one of them with
0 events blobs altered and 0 of 1,619 thinking signatures altered (the same
harness measured 286/704 corrupted before the byte-exact change). The
<sid>.jsonenvelope shape was read from real sessions and the rewrite asserted field-by-field
against it, and the join key matches what
acp/client.pyreads to issuesession/load(_meta._kiro.dev/session_file).What that still does not prove: that the provider accepts the replayed
conversation on the peer. Only the two-instance test can.
Screenshots
The one user-visible change: a degraded copy no longer reads as a plain success. Compare row 1 (green ✓
Sent— Layer B landed, resumable) with row 2 (amber ⚠Sent (transcript only)— the copy landed but without resumable context). Before this PR both rendered identically.Light theme
Also visible: the mid-send spinner, the red
Failedstate, and the disablednot connectedrow.How these were produced — and what they are not. Captured with scripted Playwright against the repo's existing
website/capture/<feature>.{html,tsx}harness convention (same aspath-chips,update-card,webhooks): the realInstanceSendItemsinside a real openDropdownMenuContent, real stylesheet + theme tokens, i18next initialised exactly asmain.tsxdoes. Only the send outcome is supplied as data — the same thing a peer reports back viaresume_mode.They are not an end-to-end capture. The transcript-only state is only reachable after a real cross-instance send to a peer whose Layer B failed to materialise, which needs two live gateways and an open tunnel — the same two-instance smoke test still listed as the open merge gate below. The capture script asserts every expected row label before writing a frame, so it cannot silently emit one where the new state looks like a plain "Sent" (that assertion caught a real harness bug where every row had fallen back to idle).