Skip to content

feat(remote-crew): run a local session on a connected crew - #7693

Merged
bolichen97 merged 1 commit into
mainfrom
feat/remote-crew-local-session
Sep 4, 2026
Merged

feat(remote-crew): run a local session on a connected crew#7693
bolichen97 merged 1 commit into
mainfrom
feat/remote-crew-local-session

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

"New chat on crew" did not create a local session. It reached into the connected crew, created a session there, and force-navigated the tab to that crew's pane. The session then lived only on the peer: absent from the local session list, absent from local history, absent from local search. Returning to it meant remembering which crew it was on and navigating there by hand.

Why it matters

The crew picker in the composer is a where does this run control, not a leave this app control. Every other choice on that shelf — agent, model, effort, project — changes how the next turn executes while the session stays where the user put it. Making one of them teleport the user breaks that expectation, and silently drops the session out of the two surfaces people use to find work again (the sidebar list and search).

What changed (motivation → approach → change)

Goal. A session that lives in the local list — searchable, resumable, badged with the crew's name — whose turns execute on that crew, and whose header offers only what that crew can actually do.

Approach, and the alternative rejected. Session ownership was a single thing: one gateway both listed a session and ran it. The only way to run on a peer was to let the peer own the session outright, which is what produced the symptom. The alternative — keep peer ownership and mirror the peer's rows into the local list — answers "which machine is it on" but leaves the session's real home elsewhere; it cannot be resumed, retitled, or folded locally. So ownership is split into two layers instead: the slot lives here, a binding says who executes it.

What was built.

The binding. _ChatSlot gains executor / instance_id / remote_slot (state.py). is_remote requires the whole triple, so a half-binding is fail-closed — api_chat answers 409 rather than silently running the turn locally on a session the user believes is remote (chat_handlers.py). The binding is set at birth in api_chat_slot_create: the peer slot is opened first, so a failure over there leaves no orphan slot here. It is never patched onto an existing slot — name addressing any slot that already exists is refused 409 remote_already_bound before the peer is contacted, whatever that slot is. An already-bound one would be pointed at a second peer session and orphan the first; an existing local one is the destructive half, since its transcript would stay here while its execution moved to an empty peer slot, so the next turn would run with none of the conversation on screen. Refusing there also closes an ownership hole: the slot's ownership check runs after the stamp, so a caller with no right to that slot would otherwise have created a peer session and rewritten somebody else's executor before seeing its 404. Persistence writes all three keys together or none, and rehydration drops an incomplete binding, returning the session as an ordinary local one (chat_persistence.py). Both save routes write them, including the empty-window metadata merge — a bound newborn has no messages until the first relayed row lands, so for the whole of that gap the merge is the only writer its binding ever sees; skipping it there would bring the session back local after a restart and run the next turn on this machine instead of the crew the user picked. All three are added to SLOT_OWNED_META_KEYS (history.py) — without that, carry_unowned_metadata resurrects a cleared binding from disk. slot_projection.py ships executor and instance_id on every slot so the frontend branches on an always-present field rather than inferring from absence. remote_slot is deliberately not projected: it is the peer's own slot key, meaningful only inside a request routed back through that instance, and no browser code has any use for it.

Relaying the turn. POST /api/chat without ?ws=1 streams only appended transcript rows; tool_call, tool_result, chat_segment, chat_status and chat_done normally travel over the WebSocket. remote_relay.py drives the peer turn and re-emits it locally: SSE record framing, row replay, chunk resequencing, trailing-chunk finalize, stop forwarding, and error rows that always terminate with chat_done so the composer cannot hang. remote_mirror.py lets a reader opt in with ?relay=1, after which the peer mirrors its WS frames in band. That mirror is governed by a denylist of five names (chat_message, chat_chunk, chat_thinking, context_usage, chat_done — each either already drained by the SSE reader or produced by the local side, chat_done by the relay's own finally), not an allowlist, so a frame type added later is mirrored by default instead of silently dropped.

Redacting what the peer sends. A relayed row lands on the same local surfaces a locally-run one does — this machine's transcript window, its WebSocket, its ConversationLog — and a local turn never reaches them without redact_exfiltration_urls then redact_credentials (chat_runner applies the pair before slot.append("assistant", …)). So the relay applies it on this side of the wire rather than trusting the peer to have done it: _redact_relayed for a row's content, and _redact_deep for its meta and for every string in a mirrored frame — the frames carrying tool inputs and outputs, and the ones whose shape is open-ended because the mirror is a denylist. Both redactors return their input unchanged when nothing matches, so the healthy case (a peer that ran the same pass) is byte-identical and the cost is a scan.

Reading the peer's capabilities. A bound session's shelf must offer the peer's agents, models, effort levels and workspaces; the local ones describe a machine that is not answering, so a wrong pick is accepted by the picker and refused on send. GET /api/instances/{id}/capabilities (handlers_instances.py) fetches them over a new peer_capability carrier on SshTunnelManager. It is a separate carrier with a closed path set rather than a widening of proxy_request's allowlist: that allowlist is ("api","chat") / ("api","stream") matched as a prefix, so admitting api/agents would also grant the peer's mutating PUT /api/agents/{name}. The carrier accepts only the five paths in _PEER_CAPABILITY_PATHS and refuses everything else, with a byte cap enforced before JSON decode (constants.py). The four peer endpoints do not agree on reply shape — /api/agents and /api/workspaces wrap their list in a dict while /api/models and /api/effort-levels answer bare lists — so _cap_list unwraps by key before clamping; without it a healthy peer's agent picker renders empty with no error. The peer's own default_agent is carried beside its roster, because a bound session deliberately stores no agent (below) and the header would otherwise fall back to a crew from this machine's roster.

Applying a pick on the machine that runs the turn. A picker whose selection never leaves this gateway is a display-only control: the header would show reviewer while the peer answered as its own default. forward_peer_selection (remote_relay.py) POSTs the pick to the peer slot, and _apply_remote_pick (chat_handlers.py) mirrors it locally only after the peer accepts — writing the local field first would leave the user looking at a pick the peer refused. A refusal surfaces as 502 remote_pick_failed carrying the peer's own error string (clamped to 200 chars; nothing else in a peer reply is trusted for display), never a silent success. The four routes are a closed map (_PEER_CONTROL_SEGMENTS) rather than an f-string over the caller's word, since the segment is interpolated into a proxied URL. Each branch sits after the existing validation, so bad input is still 400 locally, and version parity is re-checked before every forwarded write. An accepted pick is also persisted immediately via conversation_log.update_metadata, exactly as the local agent switch does, rather than left to the periodic dirty-slot flush: the peer commits the value the moment it answers, so a restart inside the flush window would restore a local field the crew no longer agrees with — and the crew is the side that runs the next turn. (A local-only pick can only ever disagree with itself, which is why the local model/effort/workspace routes can leave it to the flush and this one cannot.) The workspace pick deliberately leaves slot.project alone — that is a path on this machine. Agent and model picks ride the create rather than following it, so a second round-trip cannot fail after the peer session already exists and leave a bound session running a crew the user did not choose. For the same reason a bound create skips both this machine's default-agent stamping and resolve_agent_bindings: those answer from this machine's roster and bindings, so they would stamp an agent the peer may not have and resolve it to a local workspace.

Version gate. Remote execution requires identical gateway versions. /api/health deliberately withholds version from non-direct-local callers, so rather than reverse that decision with a public endpoint, this adds a cookie-authenticated GET /api/version (handlers/core.py) — absent from token_auth._BYPASS_EXACT and origin.PROBE_PATHS, so it needs the dashboard credential and a served Host.

Frontend. useRemoteCapabilities reads the binding once per session. ChatPage.tsx substitutes (never merges) the peer's agent and model rosters into the shelf's pickers — a union would let the user pick a local-only model and could not say which side it came from — supplies a context-window fallback until the first relayed context_usage frame arrives, and resolves the header's agent label through the peer's default_agent for a bound session. ReasoningEffortDropdown gains levelsOverride and skips its local /api/effort-levels query entirely for a bound session, since that endpoint only knows this gateway's models. While the capability read is in flight the lists are empty, not local: a brief empty picker is honest, whereas briefly showing this machine's models invites exactly the wrong pick. RemoteCrewChip is one shared component, now rendering on both the federated-search row and the new live session row. The sidebar's crew-create mutation deliberately sends no agent, unlike every sibling create entry: defaultAgent names a crew from this machine's roster and the backend forwards any agent it is given straight to the peer, so sending it would either be refused over there or bind a different crew than the name implies. Omitting it lets the peer apply its own default, which is what the header then reads back from the peer's default_agent.

Restart and mode safety (this round, closing the GPT review blockers). Four narrowly-scoped guards, none of them resume-attach:

  • A restart no longer silently truncates a turn. relay_remote_turn now persists a _relay_in_flight marker (a new _ChatSlot field, written with the binding in both save routes) before the peer stream opens and clears it when the turn ends. On reload, a slot still carrying the marker gets an explicit "interrupted by a restart" row appended at the tail (chat_persistence.py rehydrate), so a truncated conversation reads as interrupted rather than complete. The peer keeps running — that is still the split's upside — but the local side is honest about what it did not receive.
  • A remote session is locked while its tunnel is down. peer_is_connected (remote_relay.py) reads the tunnel state defensively, and api_chat answers 409 remote_not_connected for a bound send until the tunnel is CONNECTED, so a just-restarted gateway cannot fire a turn into a half-open tunnel and lose it.
  • The peer 409s a relay=1 send to a busy slot. On the peer a relayed slot is an ordinary local slot (is_remote is false there), so the busy branch's existing remote refusal did not cover it; it would have queued and drained without the mirror, losing the answer. The busy branch now refuses relay=1 too, which the owner's reader surfaces as a reconnect prompt.
  • A crew-bound session is plain-chat-only. A non-plain mode (crew / orchestrator / design-critique) is consumed by an earlier dispatch branch in api_chat that runs its tools and filesystem work on this machine, not the peer. api_chat_slot_create rejects a non-plain mode when instance_id is set (before the peer write), and api_chat_slot_mode refuses switching a bound slot into one — closing both the birth and the post-create path.

docs/feature-map/README.md gains the Remote-bound session row, per the map's maintenance contract for a feature-adding PR.

Subtractions. remote_mirror.is_mirrored and the mirror_slot context manager are deleted — both had zero non-test callers (production attaches and detaches through remote_mirror.attach / detach), and the tests that used them now assert the mirror's behaviour instead of its internal state. remote_slot is dropped from the wire projection and from the ChatSlot TypeScript type for the reason above.

Tests

  • test/test_remote_crew_execution.py (new, 167 tests) — locks in: a fresh slot runs locally; each of the three binding fields missing individually leaves is_remote false and makes dispatch refuse rather than fall back to local; the mirror emits nothing until a reader attaches, and handles overlapping readers; SSE record framing and chunk resequencing; the version-equality gate rejecting a mismatched peer; relay row replay; the binding surviving a persistence round-trip (plus being dropped when incomplete on disk); both directions of the empty-window merge; that an explicit pick rides the create while an unpicked one is omitted rather than sent as ""; each of the four picks reaching its own peer route with the right body, an unlisted control raising before proxy_request is touched, a version-skewed peer never being written to, the peer's own refusal being what the user reads (with unusable replies falling back to the status and a 900-char message truncated to exactly 200), and an unreachable peer leaking no port; that a bound create records no agent and forwards none while a local create still stamps this machine's default; and that an accepted pick is mirrored, a refused one is not, a model pick bumps _model_pick_gen, and a workspace pick leaves slot.project untouched. Plus, from this round: the peer's slot key is never projected; an existing local slot is not converted to remote (409, peer never called, transcript and executor untouched); each of the four accepted picks is readable from the ConversationLog immediately while a refused one persists nothing; and a peer row carrying a credential and an exfiltration URL is redacted before it reaches slot.messages, the chat_chunk / chat_thinking broadcast, or a row's meta, with a mirrored frame's nested strings redacted before broadcast_ws and clean prose passed through byte-identical. This round: peer_is_connected reading the tunnel state defensively (connected → true; disconnected / no-status / a raising manager / no manager → false) and a bound send being refused 409 remote_not_connected while the tunnel is down with the peer never reached; a completed relay clearing the in-flight marker while a crash mid-turn comes back with an "interrupted" row at the tail (and the runtime marker left unset so a second restart cannot append it twice); a relay=1 send to a busy local slot being refused 409 remote_turn_busy rather than queued; and a bound slot refusing a switch to a non-plain mode.
  • test/test_remote_crew_capabilities.py (new, 28 tests) — pins the two halves that make the peer's shelf honest: the reply shape each peer endpoint actually uses (the dict-wrapped agents case is the regression that would otherwise empty the picker silently on every healthy peer) and the clamping applied to a peer's reply on its way to the browser — unlisted fields dropped, strings truncated, True rejected as a context window, row floods capped. Plus the peer's default_agent being carried and reported as "" when the roster read fails, per-control degradation, the owner/flag/origin gates, and the capability carrier's closed path set raising before any target is resolved.
  • test/test_chat_slot_facade_contract.py — the pinned projection key tuple now asserts executor and instance_id ship on every slot; remote_slot's absence is asserted positively in test_remote_crew_execution.py.
  • test/test_session_control.py — the empty-window merge's drift guard enumerates SLOT_OWNED_META_KEYS, so the three new keys join forked_from / linked_session_key in its excluded set: like those, they are conditional-identity fields a plain local newborn does not carry. The bound case they do apply to is asserted positively in test_remote_crew_execution.py rather than left uncovered.
  • website/src/test/RemoteCrewChip.test.tsx (new, 5 tests) — the chip renders the crew name, carries its testid, and titles itself from the name when no explicit title is given.
  • website/src/test/ChatSidebar.createMenu.test.tsx — rewritten to assert the new intent (create a local slot carrying instanceId). It previously pinned the jump-to-pane behaviour this PR removes, so it had to change for the fix to be expressible. Two cases added this round make the no-agent omission load-bearing: with a default agent configured, the crew create still sends none, while the ordinary local create still stamps it. Without the configured default the earlier assertion passed either way.
  • Two stop-path test doubles (test_stop_addresses_linked_session.py, test_stop_handler_idempotent.py) gained the binding fields and is_remote, which the stop-forwarding branch now reads.

Verified on this branch at c0787bcd4 (rebased onto current origin/main), targeted rather than full-suite runs. This round: pytest test/test_remote_crew_execution.py -n0184 passed (plus the _build_stream_chunk regression files); isort / flake8 / black / mypy clean on the changed backend modules. This round resolved two relay data-integrity findings: a pre-stream refusal (version skew / non-2xx / connection error, before the peer received the turn) now rolls back the user row appended before dispatch, so a retry no longer duplicates local history the peer never saw — a mid-stream truncation still keeps the row (the peer is running it); and the relay SSE now carries a tool row's durable meta (tool input/output, call id) via an opt-in include_row_meta on _build_stream_chunk, so a local refresh no longer loses tool correlation (the peer's per-gateway mid is stripped so the local row mints its own). The ordinary / OpenAI-compat SSE keeps its no-row-meta contract. Earlier rounds resolved: session_send refusal + the _run_chat chokepoint guard; the effort-dropdown override; relay cancellation preserving the in-flight marker; Continue/Rewind refusal ordered after app-ownership; chat_done on the mirror denylist; the marker being slot-owned; refuse-before-record on send; the four turn-restarting refusals; the pre-peer member- gate; and the mirror detach on a prepare() failure. Earlier rounds resolved: relay cancellation preserving the in-flight marker; Continue/Rewind refusal ordered after app-ownership; chat_done on the mirror denylist; the marker being slot-owned and its drift-guard exclusion; refuse-before-record on send; the four turn-restarting refusals; the pre-peer member- gate; and the mirror detach on a prepare() failure. The earlier-round evidence below stands for the surfaces this round did not touch:

  • pytest test/test_remote_crew_execution.py test/test_remote_crew_capabilities.py -n0137 passed.
  • The four adjacent files whose guards this diff touches (test_chat_turn_timeout_consistency.py, test_stop_addresses_linked_session.py, test_stop_handler_idempotent.py, test_chat_slot_facade_contract.py) — 53 passed.
  • test_session_control.py — 140 passed, 2 pre-existing failures (test_the_created_agent_name_is_sanitized_before_storage, test_the_audit_write_does_not_run_on_the_event_loop). Both pass in isolation and fail only when the file runs whole: the session-creation rate limiter carries state between tests. Reproduced identically (2 failed / 140 passed) on a clean origin/main worktree with this diff absent, so it is not caused by this PR — flagging rather than folding an unrelated fix in.
  • remote_relay.py line coverage 99% (210 statements, 1 miss).
  • flake8 clean on the changed backend modules; mypy clean; the repo's own scripts/check_black_formatting.py gate passes with 44 files in scope.
  • Frontend: tsc --noEmit clean, eslint src/ at 601 warnings (budget 603, 0 errors), 17 component tests pass.

Manual verification

Not verified end-to-end, and the reason is structural: remote execution gates on identical gateway versions on both sides, and every peer reachable from this machine runs an older build. The relay has therefore been exercised only against the test doubles, not a live peer. A reviewer with two same-build gateways should confirm: create a session via "New chat on crew", verify it appears in the local list with the crew's chip, send a turn, confirm tool calls and streaming text arrive locally, and change the agent/model from the header and confirm the next turn runs under the new pick.

Screenshots / video

Captured against the real built SPA (website/dist) by a scripted harness, website/scripts/capture-remote-crew-local-session.mjs, with every /api/** call answered from fixtures — no gateway, no kiro-cli, and deliberately no peer: the chip renders off the executor / instance_id fields of the session projection plus the crew roster, so the at-rest surface is reproducible on a machine with no reachable crew. The harness asserts what the frames claim before it writes them (chip present on exactly the 2 rows whose executor is remote, label resolved from the roster rather than falling back to the raw id, chip inside its own max-width cap with no row overflow, submenu offering every connected crew).

1. A bound session in the local sidebar — two peer-bound rows carrying the info-tinted Server chip (nobita, and gian-eu-west-1 truncating at the 7rem cap), beside three ordinary local rows including one whose strip already holds incognito + Autopilot, so the runs-elsewhere marker is seen competing for the same space rather than alone.

A local session bound to a remote crew, chip visible in the sidebar

2. The entry point that creates the binding — the create-caret menu with New chat on crew open and its submenu listing this machine's connected crews. Picking one creates a session that stays in this list instead of switching to that crew's iframe pane, which is the behaviour change the feature exists for.

The New chat on crew submenu listing connected crews

What these frames do NOT show, and I would rather name it than let the images imply it: a relayed turn streaming back. That needs two hosts on the same gateway build — the same constraint described under Manual verification — so both frames are at-rest surfaces. The streaming path is covered by tests, not by a picture.

Known gaps

Both are deliberate scope calls, documented where a reader lands rather than left to be discovered:

  • Resume-attach is deferred to a follow-up PR — but a restart is no longer silent. If the local gateway restarts mid-turn, the peer keeps running and the local transcript still stops at its last row; reopening the session now appends an explicit "interrupted by a restart" row at the tail rather than showing a complete-looking transcript, and the session is locked (409 remote_not_connected) until the tunnel is back rather than accepting a turn into a half-open one. Re-joining the peer's in-flight tail (true resume-attach) is the follow-up.
  • A tool the peer wants approved stalls the turn there. The approval card is rendered from the slot projection (pending_approval / approval_id), not from a streamed frame, so it is built from local slot state a relayed turn never populates: the card does not appear locally, and api_chat_slot_approve would find no local future to resolve. Closing it needs the peer's pending approval mirrored onto this slot's projection and the decision forwarded back — a second mechanism, deferred alongside resume-attach rather than half-built. Noted in relay_remote_turn's docstring. Until then, point peer-bound sessions at a crew whose approval policy does not stop for the tools you expect to use.

Related Issues

no linked issue: this came from directly reported behaviour on "New chat on crew" rather than a filed issue.

Two open PRs overlap this one — flagging both for a maintainer call:

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — no user-facing doc covers the crew picker's execution semantics yet; happy to add one if wanted
  • No secrets, credentials, or internal references in the diff

@iamwhatever
iamwhatever requested a review from a team September 1, 2026 18:19
@iamwhatever
iamwhatever requested a review from a team as a code owner September 1, 2026 18:19
@iamwhatever
iamwhatever requested a review from dwu96 September 1, 2026 18:19
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

The happy path is honest and legible, but every failure the feature routinely produces — create refused, crew unreachable, peer awaiting approval — is silent on screen.

Watch

  • "New chat on crew" fails silently. createRemoteChatMutation has no onError and createSlot.rejected only clears creatingSlot (chatSlice.ts:5569) — yet a bound create is designed to refuse on version skew or a down tunnel (the PR itself says every reachable peer fails the gate today). User clicks a crew, the menu closes, nothing appears, no reason given. High frequency × dead-end confusion × every attempt: surface the refusal via ErrorNotice near the create menu.
  • Capability failure renders as a permanently empty picker. ChatPage maps remoteCrew.capabilities?.agents ?? [] and never reads failed/isLoading/unavailable, and the hook sets retry: false; the client.ts docstring promises version_match and unavailable let "the UI explain a refusal before the user types," but no component consumes either. A disconnected crew's session shows empty agent/model dropdowns with zero explanation for up to 5 minutes. Render an "crew unreachable" row in the affected pickers from failed/unavailable.
  • A peer-side approval stalls the turn with no local feedback (disclosed known gap): the approval card never renders locally and there is nothing to act on, so the first tool-gated turn just spins forever. Until resume of the follow-up, at minimum surface a static "waiting for approval on {crew}" hint or note the limitation on the create submenu.

Suggestions

  • Add the RemoteCrewChip (or equivalent "Runs on {name}" marker) to the chat header/composer shelf — the only runs-elsewhere indicator is the sidebar row, invisible on mobile or with the sidebar collapsed, while the send action executes on another machine.

[UX-REVIEWED] c2526a3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound relay architecture with fail-closed bindings, but a bound session silently hangs on the peer's first tool-approval — a mainstream path disclosed only in a code comment.

Watch

  • Approval dead-end: relay_remote_turn's own KNOWN GAP note says the approval card "is built from local slot state that a relayed turn never populates: the card does not appear here, and api_chat_slot_approve would find no local future." On a peer with a default (non-yolo) approval policy, the first tool call stalls the turn with nothing visible locally until CHAT_TURN_TIMEOUT. The PR description names resume-attach as out of scope but not this; users will read the hang as breakage, not a documented limit.
  • Exact-version fence: "remote execution requires string-equal gateway versions" means every release of either end disables all bound sessions until both are updated in lockstep. Defensible for an unversioned frame vocabulary, but it is an operational treadmill humans should knowingly accept — and parity is re-read over the tunnel on every send and every header pick, adding a round-trip per message.
  • Second-class session type: a bound slot refuses regenerate, edit-resend, rewind, continue, queueing while busy, cross-session send, and OpenAI-compat (all honest 409s). Acceptable staging, but the feature ships with most turn-affordances absent; expect the follow-up pressure to land as an executor-aware dispatch layer rather than more per-endpoint refusals.

Suggestions

  • Disclose the approval stall where the user meets it — a notice on the crew chip/shelf (or refuse binding to a peer whose policy will stop for tools), mirroring how the Claude-backend caveats are surfaced on the Agent Backend panel.
  • The per-endpoint remote_bound_refusal guards plus the _run_chat chokepoint are the right belt-and-braces now, but the comment's own observation ("the reviewer kept finding one more each round") argues for routing all turn dispatch through one executor-aware entry point in the follow-up.

[DESIGN-REVIEWED] c2526a3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c2526a316e65db1fdf002d94acf56f5375ba61be — 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 evidence is gathered. Here is the review.

First-Principles-Verdict: CONCERNS

The binding, relay, and peer-roster shelf all earn their place; but 6 of the capabilities reply's 10 fields — workspaces, version banner, per-field failures — have zero frontend consumers.

What this change ships

Intent: start a chat that runs on a connected crew without the session vanishing from the local list — an ADDITION (a new ownership split: local session, remote execution), honestly titled feat.

  1. "New chat on crew" now creates a local session — listed, searchable, resumable — whose turns run on that crew — justified
  2. Creating one no longer switches the tab to the crew's pane — justified
  3. Bound rows carry a "Runs on {crew}" chip, shared with the federated-search chip it deduplicates — justified
  4. Agent, model, and effort pickers on a bound session offer the crew's options; picks apply on the crew — justified
  5. Workspace picker still offers this machine's list while the reply's workspaces/default_workspace go unread — zero consumers
  6. Capabilities reply's version/local_version/version_match/unavailable fields — zero consumers
  7. Regenerate, edit-resend, rewind, continue, mode switch, and queueing refuse on a bound session — justified (fail-closed, chokepoint at _run_chat fixes the cause, not just call sites)
  8. A send to a version-mismatched crew or a down tunnel gets a visible 409 — justified (unversioned wire vocabulary)
  9. A restart mid-turn leaves an explicit "interrupted" row on reload — justified
  10. New authenticated GET /api/version, ?relay=1 mirror flag, binding persisted and projected on every slot — justified, consumers counted

(Screenshots and the capture script are the PR-template convention — 416 sibling capture-*.mjs scripts — not riders.)

Watch

  • The description claims the shelf offers "the peer's agents, models, effort levels and workspaces… a wrong pick is accepted by the picker and refused on send" — but no frontend hunk reads capabilities.workspaces, so the workspace picker still lists local workspaces and the wrong-pick-refused-on-send failure survives for that one control. Same gap for version_match: the client comment promises "the UI can explain a refusal before the user types," and nothing renders it.
  • The tool-approval gap (peer turn stalls, no local card, no way to approve) is disclosed only in a relay_remote_turn docstring; a user on an approval-gated crew meets it as a silent hang.

Subtractions

  • Shrink the /api/instances/{id}/capabilities payload to the four fields the frontend reads (agents, default_agent, models, effort_levels). Grepped every website/ hunk: workspaces, default_workspace, version, local_version, version_match, unavailable each count 0 non-test consumers — and the dispatch gate uses peer_version directly, so nothing backend depends on them either. Reinstate each field in the PR that wires its control.

[FIRST-PRINCIPLES-REVIEWED] c2526a3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates fail falsification:

Candidate 1 — The only in-memory route to a marker-only remote binding (executor="remote" with empty instance_id/remote_slot) is _rehydrate_slot_from_history reading such a state off disk. But the save path (chat_persistence.py:2769 and :3057) always writes all three fields as one dict, and no code path anywhere sets executor="remote" while clearing the targets (grep confirms: writers set all three together). A truncated write cuts the JSON line mid-object ({…,"executor":"remote" — unparseable, skipped on load), so it cannot yield a valid line carrying the marker without its targets. That leaves only a deliberate hand-edit of the history JSONL — not an input that occurs in practice, and an agent able to edit that file could write a fully-local slot directly, so no boundary is crossed that isn't already open. Fails bar (a). This is defense-in-depth completeness on a corruption-recovery path, not a grounded defect.

Candidate 2 — A ValueError from get_or_create_slot after create_peer_slot (concurrent create with a differing memory_mode) returns 409 at chat_handlers.py:2436 and orphans the peer session. But the immediately-following accepted branch (if remote_slot_key and not is_new_slot, line 2453) produces the identical outcome for the same-memory_mode concurrent case — it deliberately leaves the peer session "to the crew" and does not release it. The alleged wrong outcome is the exact self-limiting orphan the authors already accept, so no distinct observable failure. Fails bar (c). (Candidate self-rates confidence low for this reason.)

No self-originated findings met the bar.

No findings.

[OPUS-REVIEWED] c2526a3

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

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

@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from d4e8d1b to 8c8ce39 Compare September 1, 2026 19:37
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c2526a316e65db1fdf002d94acf56f5375ba61be and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c2526a3

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from 8c8ce39 to d010314 Compare September 2, 2026 00:44
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE], src/kiro_crew/dashboard/chat_handlers.py:2075 (span 8c8ce393e): valid, fixed in d01031402.

The finding was correct as written. create_peer_slot is a write on another machine spending the owner's tunnel credential, and on 8c8ce393e it ran before both the App Kit ownership check and the already-bound check. An app token plus a known instance id created a session on the peer even when the request was then refused locally — the refusal came too late to matter.

Both gates now run before the peer is touched (chat_handlers.py:2079-2100):

  • an app-scoped caller cannot bind at all — the attempt is written to log_api_access with outcome="denied", source="app_isolation", and answered 404 slot_not_found (the same code the ownership path returns, so the response is not an existence oracle for instance ids);
  • a name that already addresses a slot with executor == "remote" is answered 409 remote_already_bound rather than being re-pointed at a second peer slot.

create_peer_slot is only reached at chat_handlers.py:2105, after both.

Six tests in test/test_remote_crew_execution.py::TestBindingAuthorization drive the real handler through TestClient(TestServer(...)) with create_peer_slot monkeypatched to an AsyncMock spy, and every refusal path asserts peer.assert_not_awaited() — the ordering, not just the status code, is what is pinned. The two remaining cases cover the allowed dashboard path and a 502 remote_bind_failed leaving no local slot behind.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — Opus 4.8, src/kiro_crew/dashboard/handlers_instances.py:1327 (span 8c8ce393e): valid, fixed in d01031402.

Confirmed by reading the four peer handlers rather than trusting the shapes I assumed, and the review was right — but the problem was one endpoint wider than the diagnosis. The peer endpoints do not agree on an envelope:

peer endpoint shape source
/api/agents {"agents": [...], "default_agent": ...} routes/agents.py:33api_kirocrew_agents
/api/models bare list handlers/agents.py:1975
/api/effort-levels bare list handlers/agents.py:2154
/api/workspaces {"workspaces": [...], "default": ...} handlers/files.py:1440

_cap_rows opens if not isinstance(payload, list): return [], so a wrapped reply degraded to an empty picker with no error anywhere — indistinguishable in the UI from a crew that genuinely has no agents, which is precisely the failure a reviewer cannot see in a screenshot.

Rather than patch the one read, _cap_list(payload, key) (handlers_instances.py:1201) normalizes both shapes and is applied at all four list reads. That is deliberate: a per-read unwrap leaves the next shape mismatch free to empty a different picker just as silently.

test/test_remote_crew_capabilities.py (26 tests) pins it. _all_ok() encodes each endpoint's real shape, so the headline regression — test_a_dict_wrapped_agents_reply_still_populates_the_picker — fails on the pre-fix code, and TestUnwrapHelper covers bare / wrapped / wrapper-missing-key / scalar.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 2, 2026
@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from d010314 to 3db3ceb Compare September 2, 2026 01:19
@iamwhatever

iamwhatever commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Posted as a plain comment, not an ai-review-disposition record: it answers an advisory finding and claims no span=, and a record without one blocks PR Readiness.

Disposition — GPT advisory, src/kiro_crew/dashboard/remote_relay.py:415

"message" and "slot" are the only peer writes, so remote selections remain local and relayed approvals/questions cannot resolve on the peer → Fix: remove remote-session controls and creation until their writes are forwarded to remote_slot.

The finding names two distinct gaps. The selections half was legitimate and is fixed. The approvals half is legitimate and is now a documented deferral, not silence. I did not take the prescribed fix (removing the controls and creation) for either — see the last paragraph.

1. Selections — fixed. The reviewer was right that a picker whose selection never leaves this gateway is display-only: the header would read reviewer while the peer answered as its own default. Now:

  • forward_peer_selection (remote_relay.py) POSTs the pick to the peer slot; _apply_remote_pick (chat_handlers.py) mirrors it locally only after the peer accepts, so the header can never show a pick the peer refused.
  • All four controls forward — agent, model, workspace, reasoning effort — through a closed map (_PEER_CONTROL_SEGMENTS), not an f-string over the caller's word, since the segment is interpolated into a proxied URL.
  • A refusal is a 502 remote_pick_failed carrying the peer's own error string (clamped to 200 chars; nothing else in a peer reply is trusted for display), never a silent success — a control that reports success while changing nothing is the worse failure.
  • Each branch sits after the existing local validation, so bad input is still 400 here, and ensure_version_parity is re-checked before every forwarded write.
  • Agent and model ride the create rather than following it, so a second round-trip cannot fail after the peer session already exists and leave a bound session running a crew the user did not pick. For the same reason a bound create now skips this machine's default-agent stamping and resolve_agent_bindings — both answer from this machine's roster and bindings. The peer's own default_agent is carried on /api/instances/{id}/capabilities so the header has an honest fallback.
  • Tests: the TestForwardPeerSelection, TestBoundCreateDefaults and TestRemotePickApplication classes in test/test_remote_crew_execution.py (now 96 tests) cover each of the four routes and bodies, an unlisted control raising before proxy_request is touched, a version-skewed peer never being written to, the peer's refusal being what the user reads, truncation at exactly 200 chars, an unreachable peer leaking no port, a bound create forwarding no agent while a local create still stamps the default, mirror-only-on-accept, the _model_pick_gen bump, and slot.project being left alone by a workspace pick. test/test_remote_crew_capabilities.py (28 tests) covers the default_agent carry and its "" fallback.

2. Approvals — confirmed, and deferred as a documented known gap. I went looking for the write to forward and found the gap is not a missing POST. The approval card is rendered from the slot projection (slot_projection.py: pending_approval / pending_approval_info / approval_id), not from a streamed frame, so it is built from local slot state that a relayed turn never populates. The card therefore never appears locally, and api_chat_slot_approve would find no local future to resolve — forwarding the POST alone would be a control nobody can reach. Closing it properly needs the peer's pending approval mirrored onto this slot's projection and the decision forwarded back: a second mechanism, the same shape and size as the resume-attach work already scoped to a follow-up PR. Rather than half-build it, it is now recorded in two places a reader actually lands: relay_remote_turn's docstring and the PR body's Known gaps section, with the practical mitigation (point peer-bound sessions at a crew whose approval policy does not stop for the tools you expect to use).

On the prescribed fix. Removing the controls and the creation path would remove the feature, not the defect — the requirement this PR implements is specifically that a bound session's agent, workspace, context window, model and effort match what the crew offers, so a shelf with no controls does not satisfy it. The selections gap is closed instead, and the approvals gap is bounded and disclosed rather than shipped silently. Happy to reopen if a reviewer would rather see the approval mirroring land in this PR than the next one.

Verified at 3db3ceba1 (rebased onto 31bcdd3b7): 124 tests pass across the two remote-crew files, remote_relay.py at 99% coverage, flake8/mypy/black-gate clean, tsc --noEmit clean.

@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from 3db3ceb to f66ac7a Compare September 2, 2026 05:53
Comment thread src/kiro_crew/dashboard/remote_mirror.py Fixed
Comment thread src/kiro_crew/dashboard/remote_mirror.py Fixed
Comment thread src/kiro_crew/dashboard/remote_relay.py Fixed
@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from f66ac7a to 97f069a Compare September 2, 2026 06:33

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description / code mismatch

The Description describes the peer's context reading as a relayed frame that populates the shelf's context meter and as a name the mirror denylist can safely exclude, but nothing on the local side ever turns that reading into a context_usage frame — it falls through the row handler into the transcript as a durable raw-JSON row.

1. The relayed context_usage reading is never re-emitted as a local frame, and lands in the transcript as a raw JSON row

The Description says

ChatPage.tsx substitutes (never merges) the peer's agent and model rosters into the shelf's pickers — a union would let the user pick a local-only model and could not say which side it came from — supplies a context-window fallback until the first relayed context_usage frame arrives, and resolves the header's agent label through the peer's default_agent for a bound session.

and, on the mirror denylist:

That mirror is governed by a denylist of four names (chat_message, chat_chunk, chat_thinking, context_usage — each either already drained by the SSE reader or pushed by the local side), not an allowlist, so a frame type added later is mirrored by default instead of silently dropped.

The code does — the peer's context reading does cross the wire in band, as an SSE row with type: "context_usage", so the denylist's stated reason for excluding it from the WS mirror is accurate. What is missing is the other half: _apply_row receives that payload and has no branch for it, so it is never translated into a local context_usage WS frame under the local slot key the way _replay_mirrored_frame rewrites the frames it does handle — src/kiro_crew/dashboard/remote_relay.py:306 (with the denylist at src/kiro_crew/dashboard/remote_mirror.py:50). Because context_usage is also absent from _SKIP_ROLES, the row does not merely get dropped: it takes the generic durable-row path. The frontend's fallback is therefore never superseded — website/src/pages/ChatPage.tsx:5547.

Risk — two effects, both permanent rather than transient.

The context meter never populates for a peer-bound session. contextTokens stays undefined, so contextPct and contextUsedTokens render empty and contextWindowTokens is pinned either to remoteContextWindow (the pre-turn capability fallback, which is 0 for auto) or to this machine's provider.getContextWindow — the local-knowledge answer the fallback exists specifically to avoid. The user gets no signal that auto-compaction pressure is building on the peer.

Separately, every context reading the peer takes during a relayed turn is appended to the local window as a durable row whose role is context_usage and whose content is the raw JSON payload. It is persisted into local history, counted in total_messages, and broadcast as a chat_message that the store pushes onto state.messages (website/src/store/chatSlice.ts:4785 — unknown roles fall through to the generic push). Of the two, this is the more serious: the empty meter is a missing signal, whereas this silently writes machine payloads into the user's transcript and inflates the message count on every relayed turn. Neither effect is covered by the two new test files, which assert only the roles _apply_row special-cases.

Required change — handle context_usage in _apply_row: decode the row's content and re-broadcast it as a local context_usage frame under the local slot key, the same rewrite _replay_mirrored_frame performs. That satisfies the claimed handoff and keeps the payload out of the transcript in one step. If the handoff is out of scope for this PR, then add context_usage to _SKIP_ROLES so it cannot land as a durable row, and correct both the Description and the comment at website/src/pages/ChatPage.tsx:5546-5548 to state that the peer's real context reading is not relayed yet.

@iamwhatever
iamwhatever force-pushed the feat/remote-crew-local-session branch from 97f069a to 5813184 Compare September 2, 2026 15:07
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — Opus 4.8 src/kiro_crew/dashboard/chat_handlers.py:2383 (span=d085816e0bea): valid, fixed in 0c62be6d4.

  • Orphaned peer slot on a member- crew create — a member--prefixed name on a crew-binding create passed every pre-peer gate, opened the peer slot via create_peer_slot, then get_or_create_slot raised ValueError, leaving the peer session orphaned with nothing local to release it. Fixed by adding the member- reservation to the pre-peer validation block, so it returns 409 member_slot_reserved before the peer write. Locked by TestBindingAuthorization::test_a_member_name_never_reaches_the_peer.

reaches get_or_create_slot, which raises ValueError ... after create_peer_slot already opened a peer [slot]

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE] src/kiro_crew/dashboard/remote_relay.py:736 (span=16688a917388): valid, fixed in 5e461b60f.

  • Cancellation falsely marks an unfinished relay complete — the finally cleared slot._relay_in_flight and broadcast chat_done on ANY exit, so an asyncio.CancelledError (graceful restart / CHAT_TURN_TIMEOUT, peer still running detached) recorded a finished turn and a reload saw no interruption. Fixed by catching CancelledError, flagging it, and gating the marker-clear + save + chat_done on a terminal outcome only; on cancel the marker (already persisted true at relay start) survives and the error re-raises. Locked by TestInterruptedTurnSurvivesRestart::test_cancellation_preserves_the_in_flight_marker.

Graceful restart or timeout -> relay cancellation while the detached peer continues -> partial history persists without an interruption marker.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE] src/kiro_crew/dashboard/chat_handlers.py:3334 (span=78d6e248f252): valid, fixed in 5e461b60f.

  • Remote refusal precedes app ownership in Continue and Rewindremote_bound_refusal returned its 409 before the app-ownership 404, so a foreign app naming a crew-bound slot could tell it apart from a missing one (CWE-204). Fixed by moving the refusal below the existing app-ownership 404 in both api_chat_slot_continue and chat_rewind.py, so the anti-enumeration 404 wins for a non-owner. Locked by TestBoundSlotRefusesTurnRestartingActions::test_a_foreign_app_gets_the_anti_enumeration_404_not_the_409 (continue + rewind).

Foreign app names a remote slot -> 409 occurs before the ownership guard's 404 -> foreign slot existence is disclosed.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — Opus 4.8 src/kiro_crew/dashboard/remote_mirror.py:1351 (span=0e5a6670a701): valid, fixed in 5e461b60f.

  • chat_done mirrored, so a relayed turn broadcast it twicechat_done was absent from MIRROR_SKIP_EVENTS, so the peer's frame was replayed as a local chat_done in addition to the one relay_remote_turn's own finally already broadcasts. Fixed by adding chat_done to the denylist (the local finally owns turn-end), leaving exactly one. Locked by the existing test_already_in_band_frames_are_never_mirrored, now parametrized over chat_done too.

chat_done is absent from MIRROR_SKIP_EVENTS, so on a successful relayed turn the peer's broadcast_ws("chat_done", …) is mirrored ... then the peer's

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE] src/kiro_crew/dashboard/chat_handlers.py:2455 (span=78d6e248f252): valid, fixed in 7504a5210.

  • Remote-bound targets bypass the relay through session_sendsend_to_target handed _run_chat to enqueue_or_run_prompt with no remote branch, so a cross-session send ran the crew's turn on this machine. Fixed two ways (below).

This span has now taken 3 rounds (send 919 → turn-restart actions 2413session_send 2455): the reviewer keeps finding one more local-dispatch entry point. Rather than patch a fourth in isolation, the fix adds the invariant — a slot with executor == "remote" never runs _run_chat locally, guarded at the top of _run_chat itself (chat_runner.py), so every entry point is covered at the chokepoint. send_to_target also refuses with a clear 409 remote_target_unsupported at the API boundary.

Every turn-starting entry point and how each now handles a bound slot:

entry point handling
api_chat (send) relays via relay_remote_turn; incomplete-binding / tunnel-down refuse 409
regenerate / edit-resend / rewind / continue remote_bound_refusal 409 (after app-ownership 404)
session_send (send_to_target) 409 remote_target_unsupported before dispatch
queue drain (_start_next_queued_turn) unreachable — busy remote send is refused 409, never queued
orchestrator stages unreachable — non-plain mode refused at create + mode-switch
OpenAI-compat / subagent synthesis / any future caller caught by the _run_chat chokepoint guard

Tests: TestRemoteSlotNeverRunsLocally::test_run_chat_refuses_a_remote_slot_before_any_execution (the invariant) and test_session_control.py::test_send_to_a_remote_bound_target_is_refused_not_run_locally.

Remote-bound target + enabled session_send -> send_to_target passes _run_chat to enqueue_or_run_prompt -> local tools execute and local/peer transcripts diverge.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 src/kiro_crew/dashboard/remote_mirror.py:28 (span=3c2cf2799ec4): valid, fixed in 7504a5210.

  • Two stale comments — the mirror docstring said "the 4 exclusions" but a prior fix added chat_done (now 5), and remote_relay.py:763 said a busy relayed send is "queued … and never executed" when the busy branch now returns 409 remote_turn_busy. Both were left stale by earlier-round fixes. Updated: the docstring reads "5 exclusions", and the relay comment now describes the 409 refusal and why a queued follow-up is not drained locally. Comment-only; no behaviour change.

"Naming the 4 exclusions" names five entries, and remote_relay.py:763 says busy sends queue although chat_handlers.py returns 409

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 website/src/pages/ChatPage.tsx:8870 (span=083e7a6856df): valid, fixed in 7504a5210.

  • Standalone ReasoningEffortDropdown offered local effort levels for a remote session — the portal usage omitted levelsOverride, so a bound session showed this machine's effort levels and a peer-incompatible pick would fail on send. Fixed by passing levelsOverride={remoteCrew.isRemote ? (remoteCrew.capabilities?.effort_levels ?? []) : undefined}, matching the ModelEffortDropdown on the same shelf (the component already consumes the override; only this call site omitted it). tsc -b clean.

the standalone ReasoningEffortDropdown omits levelsOverride, so remote sessions offer local effort levels and peer-incompatible picks fail

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE] src/kiro_crew/dashboard/remote_relay.py:783 (span=16688a917388): valid, fixed in c0787bcd4.

  • Pre-stream refusals persist prompts the peer never receivedapi_chat appends the user row and then dispatches the relay, so a refusal raised before the peer received the turn (version-parity skew, a non-2xx status, a connection error) left the user row in local history; a retry then duplicated it while only the retry reached the peer. Fixed: relay_remote_turn tracks whether the peer's stream yielded any byte and, on a pre-stream RemoteTurnError/exception, rolls back the just-appended user row (_drop_unsent_user_row). A mid-stream truncation keeps the row — the peer is running that turn. Tests: TestPreStreamRefusalRollsBackTheUserRow (both the rollback and the truncation-keeps-it cases).

Version skew or busy peer -> local user row already appended -> retry duplicates local history and diverges it from peer context.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 [BLOCK-MERGE] src/kiro_crew/dashboard/remote_relay.py:348 (span=16688a917388): valid, fixed in c0787bcd4.

  • Production relay drops tool-row metadata_build_stream_chunk emitted meta only for permission rows, so a tool/assistant row's durable meta (tool input/output, call id) was omitted from the relay SSE and the locally-replayed row lost that correlation on reload. Fixed: an opt-in include_row_meta on _build_stream_chunk carries the redacted row meta, turned on only by the relay drain (relay_mode), so the ordinary/OpenAI-compat SSE keeps its no-row-meta contract; _apply_row strips the peer's per-gateway mid so the local row mints its own. Tests: TestRelayCarriesToolRowMeta.

Tool call -> peer stores input/output/call identity in row metadata -> SSE omits it -> local refresh permanently loses tool details and correlation.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

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

Relationship findings

  • PR #7181 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 #7181: KEEP. Unrelated capabilities (running a session on a connected remote crew vs. attaching a portable Project) that extend the same three slot-contract structures. No behavioral collision, only a merge-order conflict. Files: src/kiro_crew/dashboard/slot_projection.py, src/kiro_crew/history.py, test/test_chat_slot_facade_contract.py.
  • PR #7255 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 #7255: REBASE. Cross-referenced on 7255's timeline (2026-09-01). Direct conflict over the composer's agent/model bindings; ordering must be agreed. Files: website/src/pages/ChatPage.tsx.
  • PR #7308 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 #7308: CONTINUE_DEVELOPMENT. Disjoint goals in the same handler bodies and the same two stop tests; both are wanted, so agree an order and rebase the second. Files: src/kiro_crew/dashboard/chat_handlers.py.
  • PR #7444 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 #7444: MERGE_DISCUSSION. This overlap is already on the record as a maintainer decision, not a latent one: 7693's author submitted a CHANGES_REQUESTED review on 7444 (2026-09-03) naming 7693 and asking 'Should we go with that route, seems more natural', and 7444's author answered 'Per offline discussion, these two look complementary rather than duplicative. And it's under the preview gate.' Neither is a duplicate of the other and neither should close, but the CHANGES_REQUESTED is still open and the two heads cannot both land unreconciled — one hard conflict in client.ts and one duplicate instance_id declaration in ChatSidebar's Slot interface. Record the decision on the PR, resolve the review, and name which PR owns the remote-row model. Files: website/src/api/client.ts, website/src/pages/ChatSidebar.tsx, website/src/hooks/useInstanceSessions.ts.
  • This PR is OVERLAPPING with PR #6874. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7693: MERGE_DISCUSSION. Thematically adjacent within the remote-crew work, materially different in scope and consumer; the two peer reads are independent. Files: src/kiro_crew/dashboard/handlers_instances.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • A zero-byte 2xx stream deletes a prompt the peer acceptedfixed in e3112bc (span=16688a917388)

The rollback was gated on received_bytes, which only flips once a chunk arrives, so a 2xx response that closed before emitting a byte hit the truncation path with received_bytes=False and dropped the user row the peer had already accepted. Fixed by distinguishing ACCEPTANCE from bytes: _peer_turn_chunks now emits an empty acceptance sentinel the instant the peer answers 2xx, and relay_remote_turn gates the rollback on a peer_reached flag set by that sentinel. Rollback now fires only on failures proven BEFORE acceptance (version-parity skew, non-2xx status, or a connection error raised before any yield); a zero-byte or truncated stream after 2xx keeps the row. New regression test test_a_zero_byte_2xx_stream_keeps_the_user_row drives the real peer path.

A zero-byte 2xx stream deletes a prompt the peer accepted

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Incomplete remote metadata fails open to local executionfixed in e3112bc (span=1ce50744f47d)

Rehydrate restored the executor="remote" marker only when the full triple was present; an on-disk truncation or hand-edit that left the marker without its target dropped the whole binding, so the session came back as an ordinary local slot and its next send ran the crew turn on THIS machine — silent wrong-host execution. Fixed by restoring the marker INDEPENDENTLY of its target fields and populating only the valid ones. An incomplete binding now keeps executor="remote" with is_remote=False, which the existing api_chat guard (slot.executor == "remote" and not slot.is_remote -> 409 remote_binding_incomplete) and the _run_chat chokepoint (keyed on executor, not is_remote) refuse — fail closed, with a message the user can act on. Test test_an_incomplete_stored_binding_fails_closed_not_open locks the new contract.

Incomplete remote metadata fails open to local execution

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Remote-session ownership omits changed page and handlersfixed in e3112bc (span=2f6082500a24)

The Remote-bound session row now lists the page owners this PR changed (pages/ChatPage.tsx and components/RemoteCrewChip.tsx alongside pages/ChatSidebar.tsx), the handler owners (handlers_instances.py and handlers/core.py alongside chat_handlers.py, remote_relay.py, remote_mirror.py), and the authenticated GET /api/version endpoint that backs the version-parity fence.

Remote-session ownership omits changed page and handlers

A session created from "New chat on crew" now lives in THIS machine's sidebar,
transcript, history and search, carrying a chip that names the crew its turns
run on. It previously POSTed straight to the peer and switched to that crew's
iframe pane, because the local list had nowhere to show a peer-owned session.

Binding: `_ChatSlot` gains executor / instance_id / remote_slot, projected on
every slot so "runs locally" is a positive value rather than an absent key. An
incomplete binding is never treated as local — that would run on this machine
work the user asked a named crew to do — so `api_chat` refuses with 409 and the
rehydrate path drops a truncated binding rather than resurrecting a session that
cannot send.

Relay: the peer runs the turn in SSE mode and `remote_relay` replays its stream
locally — chunks as `chat_chunk`, transcript rows as local appends, so history
comes for free. The SSE transport carries only transcript rows, so a relay reader
opts in with `?relay=1` and the peer also queues its WebSocket frames in band
(`remote_mirror`); a 4-name denylist keeps the already-in-band frames from
doubling, so a frame type added later is mirrored by default.

Capabilities: the shelf's agent, model, effort and context-window controls source
from the BOUND CREW, via a new narrow `peer_capability` carrier with a closed path
set. The generic proxy was not widened: its fence matches prefixes, so reaching
`api/agents` would have granted the peer's mutating PUT in the same stroke.

Version gate: remote execution requires string-equal gateway versions, read over
the tunnel from a new authenticated `GET /api/version`. Deliberately NOT public —
that would reverse the fingerprint-minimization on the probe boundary, and the
existing session-search and session-import carriers prove an authenticated
non-probe route traverses the tunnel.

Resume-attach is out of scope: a local gateway restart leaves the peer running but
loses this reader, and rejoining needs the peer's in-flight tail.
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • OpenAI-compatible requests to remote slots hang after recording unsent turnsfixed in c2526a3 (span=db37e66c9e96)

The /v1/chat/completions endpoint accepted a remote-bound slot by id, mutated it (slot.agent = agent), appended the user prompt, and dispatched — reaching the _run_chat chokepoint, which emits only a WebSocket chat_done. The HTTP collector reads local chunk/assistant rows, never that WS frame, so it waited forever while history kept a turn the peer never received. Fixed by refusing a remote-bound slot in openai_compat BEFORE any mutation: right after the slot is resolved (ahead of slot.agent =, slot.append, and slot.task =), a getattr(slot, "executor", "") == "remote" guard returns 409 remote_slot_unsupported. Keyed on executor (not is_remote) so a half-open binding is refused too, matching the chokepoint and the api_chat guard. A freshly-created slot is always local, so only an existing remote target is rejected. Test test_a_remote_bound_slot_is_refused_before_any_mutation asserts the 409 and that no prompt was appended.

OpenAI-compatible requests to remote slots hang after recording unsent turns

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • _finalize_streamed_segment backward walk stops at the first non-chunk rowfixed in c2526a3 (span=e1c585cfae0d)

A stop pressed on the peer relays a system stop_event row (its dict cls is stripped to "" crossing the relay, so it lands as a plain system row) that sits between the trailing chunk deltas and the finalized assistant row. The old walk stopped at that row, stranding every chunk, so the answer rendered twice — once streamed, once finalized. Fixed by stepping OVER transient rows while dropping only the chunk rows, and stopping at the first finished message (_SEGMENT_BOUNDARY_ROLES = assistant/tool_call/tool_result/user/error) so a prior segment is never touched. Test test_a_stop_row_between_chunks_and_final_does_not_strand_them locks it: the stop row is kept, the chunks are dropped, the finalized answer appears once.

_finalize_streamed_segment backward walk stops at the first non-chunk row

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants