feat(seamless-p2): concurrency guards + collaboration semantics (Phase 2)#41
Conversation
…h bindings, gated dispatch Cross-cutting completion of the Phase 2 backend: same-transaction paste-ack matcher (NativeImportContext + bound_queued_message_id), foreign-writer detection, cli-fresh auto-bind with quarantine fallback, writer-probe and paste-transport impls, busy-wait bootstrap (resume-ready file + TTL), fail-closed sid guard, launch-time pane bindings + spawn reservations, and follow_up routed through the dispatch gate (DispatchOutcome API). Known gaps closed in follow-up commits: ingest-ack test suite (helpers staged), cli_collab unit tests, clippy strictness, shared/types.ts regeneration.
Reviewer handoff (chief run bc-lanes-2026-07-19) — READY, NOT AUTO-MERGEDWhat it does: Phase 2 of the owner-approved CLI↔UI design (#38) — the collaboration write-side. A UI message sent while someone is on the CLI is pasted into their live pane and auto-submitted (one writer, both people see the same turn); messages sent during a run queue durably and auto-drain; sending over an occupied slot returns an explicit conflict showing whose message and what text; attaching the CLI mid-run shows a busy notice and auto-resumes when the run finishes. Why this is ready-for-review rather than mergedThe boss (orchestrator) holds the merge deliberately, despite standing merge authorization, because: it carries 3 DB migrations, it modifies dispatch paths that touch real work, and it is by a wide margin the highest-risk change of this run. One word from the owner merges it. Live-proven (real backend, real Claude API, real tmux)
Four rounds of review found and fixed 8 real defects, including two severe
Verification disciplineEvery fix was verified by disabling it and confirming its test goes red — 11 revert-checks total. This caught two hollow guards (correct fixes whose tests bypassed the real call site) that would otherwise have passed forever. Two revert-check techniques for sqlx-cached queries are documented in the PR body for future use. Known limitations (documented, tracked as Phase 2.1)Gate-bypass on CI green on |
1 participant
feat(seamless-p2): concurrency guards + collaboration semantics (Phase 2)
Contract
Implements Phase 2 ONLY of the owner-approved design (merged via #38, approved 2026-07-20 with all §9 recommendations):
docs/superpowers/specs/2026-07-20-cli-ui-seamless-sync-design.md— §6 (Design C), §8 Phase 2 scope + verification, §9 decisions F1 (paste-into-TUI, auto-submit), F2 (fork "bring these back"), F3 (busy-notice), F6 (explicit replace + durable slot), F7 (auto-attribute), F11 (foreign-writer detect + banner). Phase 1 base: #39. Phase 3 items (F10/F12/F13 — nicknames, presence chips, idle-surface notifications, composing awareness) are explicitly NOT here.What ships (backend — pushed; frontend — in progress)
--resumecmdline evidence) → free; per-session critical sections; the 2s poller cache is never authority. DB/probe errors fail CLOSED (flips the previous fail-open sid guard,unwrap_or(false)→ withhold-on-error).queued→pasting→pasted→imported): imported is acknowledged in the same SQLite transaction that imports the matching native user record (prompt equality + sid + time-window matcher, executor-linked records excluded). Timeout/pane-death returns the message to the persisted slot + a visible notice — never dropped, and (post-review hardening) never blind re-pasted: requeue is evidence-conditioned (pane binding dead/replaced, or 15-min hard cap), auto-re-paste is suppressed while the original pane lives, and a late ack binds a requeued row straight toimported(blocking duplicate dispatch).session_queued_messages) replaces the in-memory DashMap (survives restarts; consumed viaon_executor_finished+ a lease-aware drain loop). Explicit replace contract: enqueue over an occupied slot withoutreplace: true⇒ 409 carrying the queued message's text/source; in-flight deliveries are never silently replaced.--resume <sid>— no more silent--continue || freshfork.cli_pane_bindings, cli-resume/cli-fresh) + fenced per-workspace spawn reservations (workspace_spawn_reservations, TTL) close the spawn-visibility TOCTOU; §4 cli-fresh auto-bind (new sid file + only-live-pane evidence) with quarantine as the unchanged fallback.foreign_writer_seen_at→ surfaced in feed health for the banner. Detection only, fail-safe on probe failure.POST /sessions/{id}/fork-recoveryre-sends a dropped branch's user turns through the lease-aware dispatch gate.follow_upnow returnsDispatchOutcome(started | queued | routed_to_cli | conflict) so the composer can tell the truth about where a message went.DISABLE_CLI_COLLAB_ROUTINGdisables paste-routing/auto-dispatch; durable queue, replace contract, fail-closed guards, and detection stay unconditional.Merge notes
20260721090000_add_session_queued_messages.sql: session_queued_messages, cli_pane_bindings, workspace_spawn_reservations, +cli_native_records.bound_queued_message_id,claude_session_links.foreign_writer_seen_at) — never-auto-merge class. Head verified non-colliding against main at ce97ca6.Evidence
Backend unit/integration (DONE) — 555 tests green across the workspace (verified by the lane driver, not just the executor), including: closed-DB-pool fail-closed lease test; scoped-reservation lifecycle observed via recording dispatcher (held during dispatch, released after, released+requeued on dispatcher failure); paste-ack atomicity proven with an injected mid-transaction ABORT trigger (ack rolls back with the failed import); matcher window/exclusion bounds; evidence-conditioned requeue suite (long live paste survives past the old timeout with zero re-pastes; dead-pane requeues with notice; late ack imports the requeued slot and blocks duplicate claim; 15-min hard cap); foreign-writer fail-safe matrix; cli-fresh auto-bind vs quarantine; busy-notice resume-ready file lifecycle; Phase 1 read-only fixture + T4 canary stayed green.
Frontend tests run locally via
npx vitest run(tsconfig excludes *.test.ts from CI by repo convention).Live e2e (DONE) — scratch instance (dev_assets/ inside this worktree, scratch tmux socket
bc-seamp2-e2e), real backend, real installedclaudeCLI, real Anthropic API calls, two-browser + raw-terminal-writer methodology matching Phase 1's:execution_processdispatched (status=running, realCodingAgentInitialRequest); a second send while it ran returnedoutcome=queuedwith a real DB-backed row; replace-without-flag correctly 409s showing the queued text+source; the row auto-drained and dispatched the instant the executor freed up.pasting→pasted→importedcycle completed and the queue returned to empty.--continue/fresh fork occurred. A real bug was found live: after the blocking executor genuinely finished, the pane never auto-resumed (polled 60+ seconds, no resume-ready signal). Root cause (confirmed, not guessed): a no-repository-change coding-agent completion took a "skip cleanup" branch incontainer.rsthat bypassed the collaboration finish hook entirely (fixed in96bee803). Fix hardened further with a periodic drain-loop reconciliation (reconcile_waiting_panes,cli_collab.rs) that self-heals any missed/late finish-hook call by scanning active pane bindings against completed-writer evidence (fixed indc7917d4), plus error propagation on the resume-signal write path (previously silent-failure-prone) with retry. Verified two ways: (1) regression-test discipline — manually reverted just thereconcile_waiting_panescall, watcheddrain_recovers_busy_resume_pane_when_finish_hook_was_missedgo genuinely red (left: []vs the expected signal), restored it, watched it go green again; (2) live re-verification against the exact original stuck pane — rebuilt the binary, restarted the backend cleanly, found the SAME pane that had been frozen on the busy-notice text for 30+ minutes now showing a live, resumed claude conversation, and confirmed it was genuinely interactive (not just stale text) by sending it a fresh command and getting a real response.routed_to_cli(pasted); loser gotconflictshowing the winner's exact text and source.claude -p --resume <sid>from a plain terminal, completely outside the app, against a session whose app-attached CLI pane's claude process was still alive (never explicitly detached/killed). The write landed and imported correctly (content visible, correct attribution) — the ingester's read/import path is writer-agnostic as designed.claude_session_links.foreign_writer_seen_atstayed NULL and no banner appeared; the actual gating logic (app_pane_absent = !(probe_failed || (pane_session_exists && agent_running == true)),cli_native_record.rs:~574-600/claude_transcript_ingest.rs:~761-762) means this is CORRECT behavior for this test: a write landing while the app's own pane has a live agent is definitionally ambiguous (could be that very pane), so the classifier rightly does not flag it. F11's actual target case — a raw write to a session with NO live app agent — was not isolated in this session, so no gap is confirmed either way. My first pass at this drew the wrong conclusion (guessed the gate checked binding-row existence rather than live-agent state) and was corrected before being written here. Follow-up: a dedicated live test with the app-side agent genuinely stopped before the raw write.error_datacarries{outcome:"conflict", status:{...queued text, source...}}exactly per contract.pasted_atset,acked_atnull). Backend stayed completely down for 30 seconds while claude answered unsupervised in the live tmux pane. On restart, startup reconciliation correctly discovered and bound the native record (acked_atlands exactly at the restart moment) — fullpasted→importedcycle survived a genuine mid-flight crash, message present in the transcript, zero duplicate dispatch.e647b0e3…). Ran it against a real session that went through three different writers (CLI-mode creation, a UI-send routed via paste while the CLI pane was genuinely alive — the precise T4 collision shape, and a raw external write): zero forks, one root, one leaf — the app-routed paths (paste, executor dispatch, racing sends) provably do not fork. Nuance recorded: a later fork DID appear once a raw-terminal write (row 6 above) was followed by continued app-routed use of the same pane — confirming, not contradicting, the design's explicit scope boundary that raw-terminal writers bypass the app-side lease entirely (§6) and are a detection-only case (F11).cargo fmt, CI-exactclippy -D warnings,cargo test --workspace(583 passed / 0 failed),prepare-db,generate-types:check, and the full frontend mirror (local-web lint+format+build, remote-web, ui, web-core, i18n, unused-i18n, legacy-paths).Known limitations (deliberate, tracked for Phase 2.1)
start_review(crates/server/src/routes/sessions/review.rs) and the PR-description auto-follow-up (crates/server/src/routes/workspaces/pr.rs) start a coding agent — including resuming the latest Claude session id — without consulting the writer lease or spawn reservation. A live CLI owning sid X plus one of these starting--resume Xcan still fork the conversation. This is pre-existing, not a regression from this PR:git diff origin/main...HEADtouches neither file, and their last modifications predate this branch. Phase 2's scope (§8) was the lease on follow-up dispatch + terminal attach; retrofitting these surfaces is Phase 2.1.resume_signal_attempts/last_signaled_atcolumn or a "panes awaiting resume" gauge would make it diagnosable.mark_consumed/requeue, the row keeps that still-alive process's claim owner and stays protected from reconcile until the process restarts (a new owner uuid on restart requeues it normally — no permanent wedge, no data loss, no duplicate). This is a deliberate risk trade, not an oversight: (1) it's a liveness issue, not a correctness one — the message is preserved and never duplicated, worst case is delayed-until-restart; (2) it requires a panic/cancellation specifically, since the normal failure path (dispatch_executor_locked'sErrbranch) already requeues explicitly; (3) the reconcile/claim-ownership clause has already been rewritten three times across this review, and that clause is precisely where the last two real defects were found — a fourth edit to close a liveness-only edge case was judged higher-risk than the edge case itself.Post-review fix stack (rounds 2-3) — read this before reviewing the diff
A full review gauntlet (council +
/simplify+ a 10-angle deep review) after the initial implementation found a P0 and six data-loss/correctness bugs that the live e2e had NOT caught. All are fixed, each with a regression test that was individually verified to fail when its fix is reverted (see "Revert-check discipline" below). The two most severe, for reviewer attention:d7eceafb(+ real testfa1ecd77): the writer probe fabricatedConfirmedResume(expected)purely from DB agreement (expected_sid == binding.claude_session_id) without checking the live process's actual--resumearg. That inverts the design's own authority order (§6: live re-probe is authority, the poller cache is a hint) and could paste a UI message into the wrong live conversation when a pane was relaunched onto a different sid — the exact T4-class silent misroute this feature exists to prevent. Nowsid_evidencetrusts live cmdline evidence only; a DB/live disagreement resolvesCliAmbiguous→ the safe queue path.a145c4f3(executor side) +2267390e(paste side): the 1s reconcile loop requeued anypastingrow older than a 5s grace while holding no session lock, but an executor spawn is allowed 30s and a paste issues ≥3 sequential tmux ops (each bounded 5s). A slow-but-healthy delivery was therefore requeued mid-flight, itsmark_consumed/mark_pastedthen matched 0 rows and was silently discarded, and the row was re-dispatched later — running the same user prompt twice. Now both delivery kinds take a process-scoped claim (executor_claim_owner/paste_claim_owner) that reconcile respects, a zero-row consume/paste-ack is a hard error instead of a silent no-op, and a legitimately-abandonedpastingrow keepspasted_at+ thePASTED_REQUEUEmarker so a late ack can still bind it.Design note — why the claim owner is per-process, not per-dispatch (do not "simplify" this without reading this note):
executor_claim_owner/paste_claim_ownerare a singleUuid::new_v4()generated once atCliCollabServiceconstruction, not a fresh id per individual claim. This is deliberate.reconcileis a global bulkUPDATEacross all sessions, but it takes only one active-owner value per call (cli_collab.rs:1182-1183). If the owner were minted per-dispatch instead, a single reconcile tick could only recognize one in-flight claim as "mine" and would requeue every other concurrent in-flight claim in the same process out from under it — reintroducing the exact duplicate-submit bug this column exists to close, just with a narrower blast radius. Per-process ownership means every claim this process is actively holding is protected by the same comparison simultaneously, no matter how many sessions are dispatching at once. The tradeoff this accepts (see "Known limitations" below) is that a claim abandoned by a panic/cancellation within a still-alive process stays protected — i.e. un-recoverable — until that process restarts, rather than being individually reclaimable.Also fixed: destructive retry (a 409 conflict was swallowed after the git reset had already run, silently dropping the prompt while the UI reported success —
5dcb0a88; retry also lost its transcript truncation, restored + threaded throughd83a5ea5); a killed/failed agent still auto-running its queued follow-up (5945c87a); queue lost-updates from deciding success by state re-read instead ofrows_affected, plus routes that bypassed the per-session lock (6125487e); and the finish hook being scattered across multiple completion branches that each had to remember to call it — collapsed to one unconditional idempotent call with a shared writer predicate (bf6418bb,af398038).Revert-check discipline (how these tests were validated)
Every fix's regression test was verified to actually bite: disable the fix mechanism, confirm the test goes red, restore, confirm green. This caught two tests that passed both ways (a correct fix with a hollow guard — one tested an extracted helper that bypassed the real call site, the other used a fake probe so the production type never ran); both were replaced with tests that exercise the real path.
Technique note for sqlx-cached queries: you cannot edit the SQL text of a
sqlx::query!to disable a protective branch — the offline cache won't compile the modified query. Instead shadow the bound parameter (e.g.let active_executor_claim_owner: Option<&str> = None;), which makes that protective branch always-match and reproduces the pre-fix behavior exactly, without touching the cached query. This isolates precisely one mechanism per check.What the gauntlet checked and found SOUND (not only what it found broken)
The paste-ack matcher is genuinely single-transaction (candidate select → native-record insert →
importedtransition → outbox → cursor advance all inside one caller-committed transaction), with ack-wins-before-age ordering; its bind order, 15-min/-5s window and recursive-CTE are correct. Nostd::sync::Mutexis held across an.awaitin production paths. The single-active-slot and single-ack invariants are enforced by real partial unique indexes, not app convention. Foreign keys are ON at runtime, so the migrations' CASCADE/SET NULL clauses are effective. Spawn reservations are race-safe via PK + fence + TTL. TheDispatchOutcome/QueueStatusunions are exhaustively handled on both sides.Review rounds (so far)
$bc_siddouble-quoted; numerics sanitized; guarded rm; TTL + future-timestamp checks).Translation provenance (read before merging)
This PR adds 28 new i18n keys (queue/delivery states, replace-confirm dialog, foreign-writer banner, fork-recovery toasts) and needed them in all 6 non-English locales (es/fr/ja/ko/zh-Hans/zh-Hant) to pass
check-i18n.sh. Those ~168 non-English strings are machine-generated (Codex) and have NOT been reviewed by a native speaker of any of the 6 languages. Verification performed so far is mechanical only:check-i18n.sh/check-unused-i18n-keys.mjspass (key parity, no dupes, no unused keys), and a programmatic diff confirmed zero strings are byte-identical to their English source (no lazy English-as-placeholder leakage) in any locale. That is necessary but not sufficient — it proves the strings are translations, not that they are correct or natural ones.conversation.queue.replaceTitle/replaceUiMessage/replaceRecoveryMessage(the replace-confirmation dialog) — this is the one safety-relevant string set; a confusing translation here could lead someone to overwrite the wrong queued message.conversation.queue.queued/pasting/pasted(novel state-machine concept with no prior art elsewhere in the app) andconversation.foreignWriter.message(an unusual English idiom — "being edited outside the app" — worth checking it reads naturally rather than just literally in each language).en/*.json) is authoritative and unaffected;config.tsdefinesfallbackLng, so any future correction to a non-English string is a pure content fix with no code risk.Deviations (recorded)
ClaudeTranscriptIngest::spawnsignature ripples db→services→local-deployment→server); it was salvaged intact from an executor run killed by a harness restart after it had fully compiled. A granular split would have produced non-compiling intermediate commits.pasted → importedin the schema — the ingester ack IS the import commit; UI copy keeps the doc's "not yet submitted" wording.