feat(server): make session messages queue safely - #5
Conversation
roughcoder
left a comment
roughcoder
left a comment
There was a problem hiding this comment.
Review: feat(server): make session messages queue safely
Verdict: REQUEST-CHANGES — the core design is better than I expected and several races I went looking for are genuinely closed. What blocks it is a set of liveness gaps around the release boundary: a queued message can be stranded indefinitely or delivered twice, and none of the concurrency behaviour is tested.
Read + reason only — per resource limits I ran no tests, no typecheck, no installs.
Verified correct (I tried to break these and couldn't)
Worth stating up front, because it changes how the rest should be read:
- No lost-wakeup on the "message arrives as the turn ends" race.
projectionPipeline.projectEventruns inside the same SQL transaction aseventStore.append(OrchestrationEngine.ts:170-205), and dispatch is serialized through one queue against the same read model the decider reads. So if the decider seesrunning, thethread.session-setthat flips it was not yet processed, and the queued row is committed before that event is published. The boundary cannot fire "between" the decision and the insert. - No duplicate release via the event worker.
makeDrainableWorkeris a singleTxQueue.take+Effect.foreverfiber (DrainableWorker.ts:47-56), andreleaseNextQueuedTurn's dispatch returns only after the transaction that deletes the queued row commits. Sequential boundary events are safe. - Persistence is well-separated.
state TEXT NOT NULLhas no CHECK constraint (005_Projections.ts:81) so'queued'inserts fine;UNIQUE (thread_id, turn_id)doesn't collide because SQLite treats NULLs as distinct, so >1 queued row per thread works;getPendingProjectionTurnandclearPendingProjectionTurnsByThreadboth filterstate = 'pending', so queued rows are neither started early nor wiped byreplacePendingTurnStart.listByThreadIdgainedstate <> 'queued'so they stay out of the UI. - FIFO across restart holds —
ORDER BY requested_at ASC, row_id ASC(ProjectionTurns.ts:221), androw_id INTEGER PRIMARY KEY AUTOINCREMENTdoes exist (005_Projections.ts:76), so the tiebreak is stable. deliveryis accurate.readEventsisfromSequenceExclusiveand dispatch returnslastSequence(OrchestrationEngine.ts:231), soreadEvents(sequence - 1, 1)really does return the last event of the command. Queue →turn-start-queued, interrupt →turn-start-queued, idle →turn-start-requested. Interrupt against an idle child correctly skips the interrupt entirely and reportsimmediate.- Schema safety ✅ —
modeis optional and typed; the new command is inInternalOrchestrationCommandso it isn't externally dispatchable;decider.ts:1513'scommand satisfies neveris satisfied by the new case. The new event type doesn't break any exhaustive switch:applyThreadDetailEventends with a forward-compatible fallback (threadReducer.ts:618), andAgentAwarenessRelay.ts:91/projector.ts:828havedefault:.
The unverified post-typecheck fix — reads correct
SessionSpawnReactor.ts:196-203. Cause and Option are imported (lines 8, 13) and nowIso is defined at line 30, so nothing dangles. Effect.interrupt | Effect.logWarning collapses the channel to never, which matches the shape start needs. Layer.provide(ProjectionTurnRepositoryLive) at line 213 is the same pattern already used at ProviderRuntimeIngestion.ts:2071, so the SqlClient requirement bubbling to server.ts:247 is idiomatic here. I could not run typecheck, but I found no reason it would fail.
One inconsistency: this boundary uses Effect.interrupt while its sibling processEventSafely uses Effect.failCause(cause) (line 155). Effect.interrupt discards the original cause and re-interrupts with the current fiber id. Prefer failCause for consistency.
Blocking
1. [major] A session that dies without a terminal status strands its queue forever — SessionSpawnReactor.ts:131 + 186-191
Release is driven purely by thread.session-set transitions. If a provider crashes, or the server is killed mid-turn, the last persisted status stays running. No further boundary event ever arrives, so the queued message is never released — and the restart sweep is no help, because it explicitly skips threads whose status is starting/running (line 188-190). The parent received delivery: "queued" and waits indefinitely with nothing surfacing the stall. The recovery sweep is the natural place to fix this: a thread with queued rows and no live provider session is exactly the case it exists to rescue.
2. [major] interrupt mode has no fallback if the interrupt doesn't take — decider.ts:1028-1069
The decider emits thread.turn-interrupt-requested optimistically and queues the replacement behind it. Nothing verifies the provider honoured it, and there's no timeout. On any adapter where interrupt is unsupported, silently dropped, or fails, the session stays running and the queued replacement strands exactly as in #1 — while the tool has already told the parent the message is queued. Given the PR explicitly defers notify because "provider steering behavior is not uniform", the same non-uniformity applies to interrupt and deserves an explicit fallback contract.
3. [major] stopped and error are treated as safe release boundaries — SessionSpawnReactor.ts:131
The guard is "not starting and not running", so idle, ready, interrupted, stopped and error all release. Two consequences worth an explicit decision rather than falling out of the predicate:
- A parent calls
stop_sessionon a child with a queued message → the stop drives a terminal status → the queued message immediately starts a new turn, resurrecting the child the parent just stopped. - A child crashes → the reactor notifies the parent of the error (line 133-142) and restarts the child with the queued message.
4. [major] Startup TOCTOU between the recovery sweep and the event worker — SessionSpawnReactor.ts:168-195
forkParked starts the stream consumer, then the sweep runs inline on a different fiber. Both do read-listQueuedTurnStarts-then-dispatch, and that pair is not atomic across fibers (the intra-worker case is safe, as noted above). If a boundary event for a queued thread arrives while the sweep is walking, both can dispatch thread.turn.start.queued for the same messageId.
The duplicate is never harmlessly absorbed, because there is no idempotency guard: the decider's queued case (decider.ts:1070-1093) only checks that the message exists and is a user message — not whether it already started a turn. So the second command either emits a second turn-start-requested (same message, two turns) or, if the session is already starting, re-emits turn-start-queued, whose projection re-inserts the queued row for another round. A message.turnId === null check in the decider, or draining the sweep through the worker, would close this.
Non-blocking
5. [minor] One bad thread aborts the whole recovery sweep — SessionSpawnReactor.ts:180-195. Effect.forEach with concurrency: 1 and the catch outside it: a failure on the first thread skips every remaining thread's queued messages until the next restart. Catch per-thread instead.
6. [minor] Queued rows leak for dead threads — ProjectionTurns.ts:100-110. clearPendingProjectionTurnsByThread filters state = 'pending', so queued rows survive thread teardown, and listQueuedTurnStarts is an unfiltered global scan that returns them forever. Tolerated at runtime (getThreadShellById → none → void) but it grows unbounded.
7. [minor] Readback miss reports optimistic success — handlers.ts:59, 407-417. Stream.runHead returning Option.none maps to "immediate". A failed lookup should not resolve to the happy answer; prefer surfacing an error.
8. [minor] Queueing is gated on deliveryMode !== undefined — decider.ts:1028. Only the MCP handler sets it, so the human UI path keeps the old racy mid-turn behaviour. Reasonable scoping, but the safety property now lives in the caller rather than the contract; any future dispatcher silently opts out.
9. [nit] handlers.test.ts collides with PR #4, which adds the same new file (with a vite-plus/test import vs. @effect/vitest here). Guaranteed add/add conflict — worth sequencing with whoever merges first.
Test quality
decider.settled.test.ts:402-479 is genuine behavioural testing — it asserts exact event sequences for queue, interrupt (including turnId propagation), and release. Good.
The gap is that every finding above lives outside what's tested. The two helpers in handlers.test.ts (normalizeSendToSessionMode, deliveryFromAcknowledgedEventType) are near-tautological — one is ?? and the other a single ternary. Nothing covers the reactor release path, double-release, stranding on a stuck running session, the enqueue/delete SQL, FIFO with more than one queued message, or restart recovery. For a change whose entire value proposition is concurrency safety, the concurrency behaviour is the part with no coverage.
Not verified
No test run, no typecheck, no lint, no live multi-session exercise. In particular the typecheck status of the post-fix reactor is reasoned from imports and an existing layer precedent, not confirmed; and findings #1-#4 are derived from reading the fiber/transaction structure rather than reproduced.
|
Fixed in Fixed:
Deferred as requested:
Validation: targeted command covering five touched suites passes 50/50. The one permitted Node 24 typecheck found four localized test/recovery typing issues; those exact diagnostics were fixed, and the 50 targeted tests pass afterward. Per the shared-machine rule, typecheck was not run twice. @codex review |
roughcoder
left a comment
roughcoder
left a comment
There was a problem hiding this comment.
Delta re-review — fbe88cc8b
Verdict: APPROVE-WITH-NITS. All four of my findings are resolved, and #4 in particular is fixed properly rather than papered over. One residual race is worth closing before this carries production traffic; everything else is nits.
Delta only (f8cd73bc7..fbe88cc8b). Read + reason, no runs.
Findings status
1. [major] Stranded queue — RESOLVED (with a caveat, below). Recovery now runs at startup and every 30s, and force-recovers a persisted starting/running session only when it has no live provider binding and its updatedAt is stale.
2. [major] Interrupt fallback — RESOLVED. Bounded at 30s from the queue entry's requestedAt, then cancel + stop + notify. Old entries compute remaining = 0 and fire on the first sweep, so a stuck interrupt that predates a restart is handled rather than waiting a fresh 30s.
3. [major] Terminal states — RESOLVED. Release is now restricted to idle/ready/interrupted; stopped/error cancel instead. No resurrection.
On your notification question: the parent can distinguish all three outcomes. Interrupted-and-replaced is silent (the message simply arrives), terminal cancellation says "N queued messages were cancelled because the spawned session entered stopped/error state", and timeout says "Interrupt delivery timed out; the queued replacement was cancelled and the spawned session was stopped." The ordering is right, too: the timeout handler cancels before stopping, so the resulting stopped event finds an empty queue and cancelTerminalQueue returns 0 without emitting a second, contradictory notification.
4. [major] TOCTOU / idempotency — RESOLVED, and this is the strongest part of the delta. Three independent mechanisms now back each other:
- Serialization covers both paths. The sweep no longer releases directly; it enqueues
{type:"recover"}into the same single-fiber worker as the stream consumer, and the interrupt timer enqueues{type:"interrupt-timeout"}there too. All three entry points are serialized. This is exactly the pairing I flagged. - Real idempotency at the decider.
queuedTurnStartsis hydrated into the authoritative command read model and maintained byprojectEvent(added on queued, removed on requested/cancelled), so a duplicate release finds no entry and fails as an invariant error instead of starting a second turn. - No more re-queue loop. The busy branch of
thread.turn.start.queuednow errors rather than re-emittingturn-start-queued, so a duplicate can't reinsert the row for another round — the failure mode I was most worried about.
5. [minor] Sweep error isolation — RESOLVED. Per-thread work moved inside the worker, where processInputSafely catches each input independently. The remaining catch wraps only the two fetches, where aborting is correct.
7. [minor] Optimistic delivery ack — RESOLVED, with a trade-off worth naming (below).
8. [minor] Contract-level queue default — RESOLVED. command.deliveryMode ?? "queue" in the decider, so the safety property no longer depends on the caller.
6. [minor] Queued-row teardown — DEFERRED, acknowledged in the reply.
Residual
[major] The liveness check is a snapshot, and staleness doesn't protect long turns. live is computed in enqueueRecovery (SessionSpawnReactor.ts:362) and consumed later inside the worker (:370), so it can be arbitrarily old by the time it's acted on — the queue may hold other work, and each recover dispatches commands synchronously through commit.
That matters more than it looks because the second guard doesn't carry much weight: stale (:261) compares session.updatedAt against 30s, but updatedAt only moves on session status transitions, so any turn longer than 30 seconds — i.e. most real agent turns — is permanently "stale". Liveness is therefore load-bearing on its own. If listSessions() runs during a provider rebind (crash-restart, adapter reconnect) the session is briefly absent, live: false sticks, and the sweep force-sets interrupted with activeTurnId: null and releases a queued turn while the original turn is still running — the double-running-turn this PR exists to prevent.
Two cheap fixes, either sufficient: re-check providerService.listSessions() inside the worker at decision time (it's in-memory and Effect<…, never>, so it costs nothing and closes the snapshot window entirely), or require two consecutive not-live observations before recovering, which is the standard failure-detector shape and makes the rebind window a non-event.
[minor] Strict ack failure re-introduces "fail after the side effect". resolveSendToSessionDelivery now fails on a missing/unexpected ack — correct in that it no longer reports the optimistic answer — but the dispatch has already happened. A transient event-store read hiccup surfaces as an error for a message that really is queued, and a parent that retries creates a duplicate (the retry carries a fresh messageId, so command-receipt dedup won't catch it). Consider carrying the threadId in that error, or a third delivery value, so the parent knows the send landed.
[minor] The contract-level default now changes the human UI path. Any thread.turn.start to a busy thread queues, including sends from the app. That's the safer behaviour and it's what I asked for, but no client affordance shows "queued" — the client reducer ignores thread.turn-start-queued via its forward-compatible fallback. A user typing into a busy thread now sees their message appear and nothing happen, with no explanation, until the boundary. Worth a follow-up on the client.
[nit] Timeout fibers are re-forked every sweep. recover forks scheduleInterruptTimeout per queued entry (:256) on every 30s pass, with no dedupe by (threadId, messageId). Self-limiting in practice — the first firing cancels the entry — but an entry that survives an early-return path accumulates a sleeper per sweep.
[nit] Date.parse NaN at :224 flows into Duration.millis(NaN). Unreachable while requestedAt comes from IsoDateTime, and the sibling staleness check fails safe on NaN; noting it only because the two behave differently.
[nit] Benign release races now log warnings. If the session goes busy between the reactor's projection read and the decider's read model, the release fails as an invariant error and logs at warning level each time. Correct outcome, noisy signal.
On the four unverified typing fixes
I checked the semantics rather than assuming the gate will catch them, and found no problems:
listSessions()exists with no arguments and no error channel, andProviderSession.threadIdis a requiredThreadId, so.map(entry => entry.threadId)is sound.- The new
ProviderServicerequirement bubbles out ofSessionSpawnReactorLive(onlyProjectionTurnRepositoryLiveis provided), but it joinsReactorLayerLivealongsideProviderCommandReactorLiveandProviderRuntimeIngestionLive, which already require it — so the composition atserver.ts:247shouldn't shift. modeis threaded consistently end-to-end: persisted as a distinctinterruptingstate, and every query (enqueue,delete,listQueued, and thelistByThreadIdexclusion) was updated toIN ('queued','interrupting'). No half-updated predicate.ThreadTurnStartQueuedPayload.modecarrieswithDecodingDefault("queue"), so already-persisted events decode.
The one I'd still watch at the gate: Effect.forkScoped inside processInput requires Scope in the worker's R. makeDrainableWorker propagates R and the layer build provides a scope, so it should hold — but it's the least locally obvious of the four.
Tests
TestClock, not real sleeps — TestClock.setTime drives both the stale-recovery and interrupt-timeout cases, so the suite stays fast and deterministic. The assertions are real: exactly one thread.turn.start.queued when a boundary event races startup recovery (a direct guard on finding 4), the FIFO head asserted by messageId, zero releases and two cancels for both stopped and error, and the timeout case pinned to the timeout path by setting live: true so it can't be satisfied by the stale-recovery branch. The double-release test is a structural regression guard rather than a concurrency stress test, which is the right bar given the design now makes the property structural.
|
Fixed in Fixed:
Deferred and documented in the PR description/internals guide:
Validation:
@codex review |
roughcoder
left a comment
roughcoder
left a comment
There was a problem hiding this comment.
Delta re-review — 9496f002e
Verdict: APPROVE — merge-ready from my side. Both residuals are closed properly, and I judge the remaining liveness risk acceptable. Only nits left.
Delta only (fbe88cc8b..9496f002e). Read + reason, no runs.
[major] Liveness snapshot — RESOLVED, and the race is genuinely closed
live is now computed inside processInput's recover branch, on the serialized worker, immediately before the decision — and the live field is gone from WorkerInput entirely, so there's no vestigial path still carrying a stale value. The window between observing !live and dispatching the recovery is now the dispatch itself rather than however long the input sat in the queue.
On your specific question — yes, the recheck hits the same structure the rebind window affects. ProviderService.listSessions() aggregates each subscribed adapter's own session list, which is exactly what a rebind mutates. So this is a true recheck, not a different source that would paper over the problem.
Two-phase observation does not exist; one recheck is deemed sufficient. I think that's the right call here, for three reasons:
- Two independent conditions must both hold — no live binding and no session status write for 30s. If a rebind writes a
startingstatus,updatedAtrefreshes,stalegoes false, and the session is protected regardless of the directory. - For a healthy session to be falsely recovered, the adapter directory would have to transiently lose an entry for a live session with no accompanying status transition. That's a much narrower window than the one you had before.
- If the process genuinely died silently, recovery is the correct action — and two-phase would delay every genuine recovery by a full 30s interval, which is the primary case this exists for.
The residual therefore rests on one assumption: absence from the directory means genuinely no binding, not a transient gap. That's worth a one-line comment at the recheck, because if that property ever stops holding, two-phase observation is the fix and nothing else in the code would signal it.
[minor] Ambiguous ack — RESOLVED, and "unknown" is a real enum value
SendToSessionResult.delivery is Schema.Literals(["immediate", "queued", "unknown"]) — declared in the contract, not a stringly escape, so it encodes into the tool's JSON Schema as a third valid variant. The readback failure is caught (Effect.catch → Option.none()), so a transient event-store hiccup no longer reports failure for a message that actually committed. The tool description documents when unknown appears. This is the right resolution of the trade-off I raised.
[nit] fixes — both RESOLVED
Timeout fibers are deduped by ${threadId}:${messageId} with check-and-add on the serialized worker (so no check-then-act race) and Effect.ensuring cleanup that also covers interruption. Date.parse NaN now fails safe at both sites — the timeout path logs and declines to schedule instead of sleeping on Duration.millis(NaN), and the staleness path treats an unparseable updatedAt as not-stale, which is the safe direction (it declines to force-interrupt).
The two post-typecheck fixes
Per the amended process rule, re-running typecheck to verify a fix is now allowed and expected — please do that rather than leaving the final state unverified. My read of the semantics found no problems:
Effect.failCause(cause)→Effect.interruptinprocessInputSafely. Sound: the branch only runs underCause.hasInterruptsOnly, so the discarded cause contained nothing but interrupts and no diagnostic value is lost. It also makes this consistent with the sweep's catch, which already usedEffect.interrupt. Presumably done to pin the error channel toneverforDrainableWorker<WorkerInput>. Behaviourally unchanged: either form kills the worker loop, and only shutdown reaches it.Parameters<typeof scheduleInterruptTimeout>[0]inforkInterruptTimeoutis the one I'd eyeball first — that's an exotic type query over anEffect.fncallable, and it's the kind of expression that resolves differently than you'd expect. The shape it needs to accept (listQueuedTurnStartsentries:threadId,messageId,mode,requestedAt) does match. Second place to check isEffect.catch(() => Effect.succeed(Option.none()))inhandlers.ts, where a bareOption.none()can inferOption<never>.
Nits
resolveSendToSessionDeliverynow returnsEffect.succeedon every path — it can be a plain function, dropping theyield*at the call site.- The dedupe key is added in
forkInterruptTimeoutbut removed in two other places (scheduleInterruptTimeout's NaN branch and theensuring). Correct, but the add/remove responsibility is spread across three sites; centralising the removal inensuringalone would be easier to keep right. mode !== "interrupt"is checked in bothforkInterruptTimeoutandscheduleInterruptTimeout. Harmless.
Tests
The new case is the right one: stale + running + live: true asserts no thread.session.set, no release, and the queued row retained — the exact false-recovery guard, and the inverse of the existing stale-recovery test, so both directions are now pinned. Still TestClock throughout.
Remaining from earlier, unchanged and non-blocking: queued-row teardown is deferred, and the contract-level queue default still has no client affordance surfacing "queued" to a user typing into a busy thread.
|
Closing items fixed in
Verification:
The reviewer-flagged |
Persist busy-child deliveries as FIFO queued turn starts and release them at provider session boundaries. Interrupt mode requests the existing provider interrupt before the queued replacement is released. Constraint: send_to_session must remain event-sourced and provider-neutral. Rejected: provider-level notify steering | adapter semantics are inconsistent and need an explicit capability contract. Confidence: high Scope-risk: moderate Directive: add notify only after every provider has an explicit steering or fallback contract. Tested: pnpm exec vp test run apps/server/src/mcp/toolkits/sessions/handlers.test.ts apps/server/src/orchestration/decider.settled.test.ts (15 passed); npm run typecheck under Node 24 found one startup error-channel leak, fixed afterward. Not-tested: full typecheck was not rerun due the session's one-run resource constraint; no browser or full-suite validation.
Make queued delivery release idempotent, recover stale persisted sessions only after checking live provider bindings, cancel terminal queues, and bound interrupt fallback with parent-visible cancellation. Constraint: queued delivery must remain event-sourced and safe across restart, provider failure, and concurrent release signals. Rejected: unconditional stop-then-send fallback | provider exit can race terminal queue cancellation and resurrect stopped work. Confidence: high Scope-risk: moderate Directive: preserve queued IDs in the command read model when changing turn projections. Tested: pnpm exec vp test run apps/server/src/orchestration/Layers/SessionSpawnReactor.test.ts apps/server/src/orchestration/decider.settled.test.ts apps/server/src/mcp/toolkits/sessions/handlers.test.ts apps/server/src/orchestration/projector.test.ts apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts (50 passed); npm run typecheck under Node 24 found four localized typing issues, fixed afterward. Not-tested: typecheck was not rerun due the one-run resource constraint; no full suite or browser validation.
Move provider liveness observation into the serialized recovery decision and make post-dispatch acknowledgement uncertainty explicit. Constraint: Shared-machine validation permits one typecheck run and targeted tests only. Rejected: Two-observation failure detection | a decision-time in-memory provider check closes the stale snapshot window directly. Confidence: high Scope-risk: narrow Directive: Keep provider liveness checks inside serialized recovery decisions. Tested: pnpm exec vp test run apps/server/src/orchestration/Layers/SessionSpawnReactor.test.ts apps/server/src/mcp/toolkits/sessions/handlers.test.ts apps/server/src/orchestration/decider.settled.test.ts (21 passed, exit 0). Not-tested: npm run typecheck was not rerun after fixing its two exact diagnostics; its single permitted run exited 1.
Record the provider-directory assumption at the recovery decision and keep recovery sweep interruption out of the error channel. Constraint: Recovery may force-release only when directory absence is authoritative. Rejected: Optional cleanup refactors | closing the reviewed invariant and compiler issue keeps this commit narrow. Confidence: high Scope-risk: narrow Directive: Revisit the recovery failure detector if provider directory absence can become transient during rebind. Tested: Node 24 npm run typecheck (exit 0); targeted SessionSpawnReactor test (6 passed, exit 0); git diff --check. Not-tested: Full test suite and browser validation were not run.
Keep grace-stop notices immediate while ordinary busy-session messages queue, and make provider reactor fixtures model a completed turn before follow-up commands. Constraint: Rebase must preserve graceful-stop deadlines and queued session delivery semantics. Rejected: Queueing grace notices | it can hide the stop deadline until the turn it must stop has already ended. Confidence: high Scope-risk: narrow Directive: Internal in-turn steering commands must opt out explicitly if future delivery defaults queue busy sessions. Tested: Node 24 npm run typecheck (exit 0); targeted provider/handlers tests (53 passed, exit 0); targeted decider/session reactor/snapshot tests (39 passed, exit 0); git diff --check. Not-tested: Full repository test suite and browser validation were not run.
e8c7555 to
f78ddd6
Compare
|
Rebased onto current Conflict resolution preserved the landed structured reports, checkout support, and graceful-stop session fields/flows. This branch adds no migration, so no Grace-stop interaction:
Verification:
|
Exercise conflicting provider turn acceptance through the intentional immediate grace-notice steer now that ordinary busy-session messages queue by default. Constraint: The ingestion test must create a pending turn start before asserting conflicting turn acceptance. Rejected: Restoring implicit busy-session steering | it would violate send_to_session's queue-by-default contract. Confidence: high Scope-risk: narrow Directive: Pending-turn ingestion tests must use an explicit immediate-start path when the thread is already running. Tested: Node 24 npm run typecheck (exit 0); targeted ProviderRuntimeIngestion, ProviderCommandReactor, SessionSpawnReactor, and decider suites (112 passed, exit 0); single regression test (exit 0); git diff --check. Not-tested: Full repository suite and browser validation were not rerun locally.
|
Gate failure diagnosed and resolved in Root cause/event flow:
The production behavior is intentional and unchanged: ordinary messages must not implicitly steer a busy provider anymore. The obsolete test setup now uses Verification:
|
#5/#6/#9/#10 Rebased onto main after #4/#5/#6/#9/#10 merged; carries the reconciliation: - Envelope delivery applies to agent AND system-origin synthetic reports (same report-posted path); formatReportMessage keeps #10's origin-aware lead and delivers >1KB summaries as an envelope with a read_report hint. - SessionReportEnvelope carries #9's structured data compactly: recommendation and completionPercent whole, findings/validation as counts; and #10's origin so a synthesized epitaph is visible at a glance. read_report returns the full findings/validation arrays with every page (bounded by the 32KB structured cap) plus origin. - findByReportId now selects abstract, structured_json, and origin and maps through the shared mapReportRow, so no column can be silently dropped by one read path; the ProjectionSnapshotQuery Struct.pick allowlist gained abstract. - Migration renumbered 043 -> 046 (043 structured, 044 stop audit, 045 origin landed first). - read_report docs record UTF-16 code-unit paging (pages can run one unit short or long at surrogate boundaries) and the unguessable-UUID timing assumption behind the reportId-only lookup path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… sibling access (#8) Rebased onto main after #4/#5/#6/#9/#10 merged; carries the reconciliation: - Envelope delivery applies to agent AND system-origin synthetic reports (same report-posted path); formatReportMessage keeps #10's origin-aware lead and delivers >1KB summaries as an envelope with a read_report hint. - SessionReportEnvelope carries #9's structured data compactly: recommendation and completionPercent whole, findings/validation as counts; and #10's origin so a synthesized epitaph is visible at a glance. read_report returns the full findings/validation arrays with every page (bounded by the 32KB structured cap) plus origin. - findByReportId now selects abstract, structured_json, and origin and maps through the shared mapReportRow, so no column can be silently dropped by one read path; the ProjectionSnapshotQuery Struct.pick allowlist gained abstract. - Migration renumbered 043 -> 046 (043 structured, 044 stop audit, 045 origin landed first). - read_report docs record UTF-16 code-unit paging (pages can run one unit short or long at surrogate boundaries) and the unguessable-UUID timing assumption behind the reportId-only lookup path. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
What
send_to_sessiondefault toqueueat the decider boundaryinterruptfallback: cancel the replacement, stop the still-running session, and notify the parent when no boundary arrivesdelivery: immediate | queued | unknown; acknowledgement uncertainty after a committed send no longer reports failurenotifyas a follow-up because provider steering behavior is not uniformWhy
Parent sessions frequently message children that are already mid-turn. Delivery must remain durable and live across provider failure, server restart, concurrent release signals, terminal session states, ignored interrupts, provider rebind windows, and graceful-stop deadlines.
Validation
origin/mainafter PRs sessions MCP: document messageLimit bounds, add structured report fields, fix stop_session null result #9, feat(sessions): support explicit spawned-session checkouts #4, and feat: let spawned sessions stop gracefully #6 mergednpm run typecheckunder Node 24 — exit 0pnpm exec vp test run apps/server/src/mcp/toolkits/sessions/handlers.test.ts apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts— 53 passed, exit 0pnpm exec vp test run apps/server/src/orchestration/decider.settled.test.ts apps/server/src/orchestration/Layers/SessionSpawnReactor.test.ts apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts— 39 passed, exit 0git diff --checkDeferred
notifyrequires an explicit cross-provider steering capability/fallback contractGenerated by GPT-5.6-sol via the Phoenix Codex harness.