feat(orchestrator): introduce new orchestrator#2829
Conversation
Co-authored-by: codex <codex@users.noreply.github.com>
- Initialize provider as unchecked in a pending state - Update initial probe message to reflect session-local status
- Type the runtime effect with `Scope` - Build the ACP session runtime without wrapping it in `Effect.scoped`
- Use strict TurnId and ProviderItemId parsing in Codex session routing - Decode in-memory stdio chunks in streaming mode to avoid split UTF-8 corruption
- Transfer session-owned scopes into adapter state - Ensure runtime scopes close on stop and startup failure - Add regression coverage for scoped lifecycle cleanup
- Close the managed native event logger when the adapter layer tears down - Make session runtime close idempotent with an atomic closed flag - Add coverage for flushing thread native logs on shutdown
- Use codex app-server snapshots for auth, models, and skills - Remove legacy CLI/config discovery paths and related helpers - Update tests for the new provider status flow
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
- Document the target orchestration graph, IDs, lifecycles, and capability model - Add Codex app-server probe fixtures and update the probe test harness
- Introduce orchestration v2 service interfaces and error types - Add replay runtime, fixtures, and integration coverage - Update shared contracts and probe transcripts Co-authored-by: codex <codex@users.noreply.github.com>
- Add Codex adapter and replay harness wiring - Introduce in-memory orchestration projections and provider registry - Expand orchestration contracts for turn and runtime events
Co-authored-by: codex <codex@users.noreply.github.com>
- Add context transfer IDs, schemas, and projections - Support cheap fork creation and Codex native fork rollback - Cover fork idempotency and replay behavior in tests
- Track remaining projection, context transfer, rollback, capability, and subagent work - Clarify current V2 baseline and debugger-only follow-ups
- Map fork and merge-back turns into stored handoffs and transfer resolutions - Add shell snapshot projection support plus coverage tests - Update replay fixtures and web contracts for the new turn flow
Co-authored-by: codex <codex@users.noreply.github.com>
- Move Codex replay recording into `apps/server` - Add Claude Agent SDK replay fixtures and test harness - Update orchestration-v2 fixture scenarios and docs
- Move Claude provider runtime logic into its own module - Share the SDK query runner between live and replay paths - Add replay driver error wrapping for unexpected failures
Port orchestration V2 provider adapter wiring to the provider-instance driver registry. Co-authored-by: codex <codex@users.noreply.github.com>
- persist the selected model on run records - surface run model selection in the debug UI - update replay fixtures and contracts for the new field
- Record Claude SDK transcripts across multiple prompts and restart/query modes - Add approval and tool-call replay coverage for new orchestration fixtures - Update Claude adapter testkit to model open/prompt/permission frames
- Derive Claude SDK query options from runtime policy - Add read-only replay fixture and policy mapping tests - Reuse shared approval-policy fixtures across orchestrator tests Co-authored-by: codex <codex@users.noreply.github.com>
- add active steering and interrupt-restart replay fixtures - update Claude adapter/orchestrator turn handling for steering - refresh replay and integration test coverage
- add interrupt and mid-tool replay fixtures for Claude and Codex - log Claude Agent SDK protocol frames to native event traces - project Codex commandExecution start events into orchestration updates
- Map Cursor SDK agents and runs to V2 thread and turn lifecycles - Update MCP capability, tool, and testing guidance for SDK-based injection
- Define four implementation shapes for the V2-only migration - Add the plan to the plans index
- Separate provider driver identity from configured provider instances - Add durable event, effect, checkpoint, and turn-position persistence - Route turn lifecycle and recovery through shared orchestration services
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const worker = yield* OrchestrationEffectWorkerV2; | ||
| yield* Effect.forever( | ||
| worker.runOnce.pipe( | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/EffectWorker.ts:341
Effect.catchCause catches all causes including Interrupt. When the scope closes during normal shutdown, the forked fiber is interrupted, and this handler logs a misleading "Orchestration effect worker failed" warning for what is expected behavior. Worse, it swallows the interrupt and returns false, causing the loop to attempt waiting for more work instead of terminating promptly. Consider re-raising interrupt causes so the fiber shuts down cleanly.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/EffectWorker.ts around line 341:
`Effect.catchCause` catches all causes including `Interrupt`. When the scope closes during normal shutdown, the forked fiber is interrupted, and this handler logs a misleading "Orchestration effect worker failed" warning for what is expected behavior. Worse, it swallows the interrupt and returns `false`, causing the loop to attempt waiting for more work instead of terminating promptly. Consider re-raising interrupt causes so the fiber shuts down cleanly.
Evidence trail:
apps/server/src/orchestration-v2/EffectWorker.ts lines 335-352 (the daemonLayer with catchCause, forkScoped), pnpm-workspace.yaml line 24 (effect: 4.0.0-beta.78), Effect v4 migration docs at https://github.com/Effect-TS/effect-smol/blob/main/migration/error-handling.md confirming catchCause = catchAllCause catches all cause types including Interrupt, Effect documentation at https://effect.website/docs/error-management/expected-errors/ confirming catchAll only catches recoverable errors while catchAllCause/catchCause catches all causes.
| return Effect.fail( | ||
| unsupported( | ||
| input, | ||
| input.capabilities.turns.supportsInterrupt ? "interrupt_restart_steering" : "active_steering", |
There was a problem hiding this comment.
🟢 Low orchestration-v2/CommandPolicy.ts:206
When forceRestart is true and supportsInterrupt is false, the error reports "active_steering" as the missing capability. Since the caller explicitly requested a restart, the error should name "interrupt_restart_steering" instead. Consider updating the capability selection to account for forceRestart.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/CommandPolicy.ts around line 206:
When `forceRestart` is `true` and `supportsInterrupt` is `false`, the error reports `"active_steering"` as the missing capability. Since the caller explicitly requested a restart, the error should name `"interrupt_restart_steering"` instead. Consider updating the capability selection to account for `forceRestart`.
Evidence trail:
apps/server/src/orchestration-v2/CommandPolicy.ts lines 193-210 at REVIEWED_COMMIT. The function `decideSteeringExecution`: line 194 skips active_steering when forceRestart=true; line 198-199 checks supportsInterrupt; line 206 ternary `supportsInterrupt ? "interrupt_restart_steering" : "active_steering"` does not consider forceRestart when choosing the error capability name.
| driver = json_extract(payload_json, '$.driver'), |
There was a problem hiding this comment.
🟡 Medium Migrations/035_OrchestrationV2Foundation.ts:87
Line 87 updates driver using json_extract(payload_json, '$.driver') without a COALESCE fallback to provider, so rows where the payload lacks a driver field are overwritten with NULL instead of the existing provider value. This is inconsistent with the corresponding updates for orchestration_v2_projection_provider_sessions and orchestration_v2_projection_provider_threads. Use COALESCE(json_extract(payload_json, '$.driver'), provider) to match the established pattern.
| driver = json_extract(payload_json, '$.driver'), | |
| driver = COALESCE(json_extract(payload_json, '$.driver'), provider), |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/persistence/Migrations/035_OrchestrationV2Foundation.ts around line 87:
Line 87 updates `driver` using `json_extract(payload_json, '$.driver')` without a `COALESCE` fallback to `provider`, so rows where the payload lacks a `driver` field are overwritten with `NULL` instead of the existing `provider` value. This is inconsistent with the corresponding updates for `orchestration_v2_projection_provider_sessions` and `orchestration_v2_projection_provider_threads`. Use `COALESCE(json_extract(payload_json, '$.driver'), provider)` to match the established pattern.
Evidence trail:
apps/server/src/persistence/Migrations/035_OrchestrationV2Foundation.ts lines 61, 74, 87 (COALESCE present on lines 61 and 74 for provider_sessions and provider_threads respectively, missing on line 87 for subagents). apps/server/src/persistence/Migrations/034_OrchestrationV2Subagents.ts line 13 confirms `provider TEXT NOT NULL` column exists on the subagents table. Commit: REVIEWED_COMMIT.
| export class CodexReplayTranscriptDecodeError extends Schema.TaggedErrorClass<CodexReplayTranscriptDecodeError>()( | ||
| "CodexReplayTranscriptDecodeError", | ||
| { | ||
| driver: Schema.optional(Schema.String), |
There was a problem hiding this comment.
🟢 Low Adapters/CodexAdapterV2.testkit.ts:28
CodexReplayTranscriptDecodeError expects driver, but metadataFromTranscript returns provider. When the error is constructed with ...metadataFromTranscript(transcript), driver is always undefined and the transcript's provider value is discarded. Rename the schema field to provider to align with the metadata helper.
| driver: Schema.optional(Schema.String), | |
| provider: Schema.optional(Schema.String), |
Also found in 3 other location(s)
apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.testkit.ts:75
Field name mismatch:
metadataFromTranscriptreturns{ provider, protocol, scenario }butAcpReplayTranscriptDecodeErrorexpectsdrivernotprovider. When spreading...metadataFromTranscript(transcript)into the error constructor at lines 75 and 84, theproviderproperty is ignored (extra property) and thedriverfield remainsundefined. The error message includesdriver: Schema.optional(Schema.String)but never receives the actual provider value.
apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts:45
The
driverfield inOpenCodeReplayTranscriptDecodeErrorwill always beundefinedbecausetranscriptMetadata()returns an object with keyprovider, notdriver. When spreading{...transcriptMetadata(transcript), cause}into the constructor, theprovidervalue is passed but ignored (no matching schema field), whiledriverremains unset.
apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts:61
The
driverfield inCursorReplayTranscriptDecodeErrorwill always beundefinedbecausemetadataFromTranscript()returns an object with keyprovider, notdriver. When spreading{...metadataFromTranscript(transcript), cause}into the constructor, theprovidervalue is passed but ignored (no matching schema field), whiledriverremains unset.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts around line 28:
`CodexReplayTranscriptDecodeError` expects `driver`, but `metadataFromTranscript` returns `provider`. When the error is constructed with `...metadataFromTranscript(transcript)`, `driver` is always `undefined` and the transcript's `provider` value is discarded. Rename the schema field to `provider` to align with the metadata helper.
Evidence trail:
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts line 28: `driver: Schema.optional(Schema.String)` — error class expects field named `driver`
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts lines 46-56: `metadataFromTranscript` returns `{ provider, protocol, scenario }` — uses `provider`, not `driver`
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts line 198: `...metadataFromTranscript(transcript)` spread into `new CodexReplayTranscriptDecodeError(...)` — property name mismatch causes `driver` to be `undefined` and `provider` to be silently discarded
Also found in 3 other location(s):
- apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.testkit.ts:75 -- Field name mismatch: `metadataFromTranscript` returns `{ provider, protocol, scenario }` but `AcpReplayTranscriptDecodeError` expects `driver` not `provider`. When spreading `...metadataFromTranscript(transcript)` into the error constructor at lines 75 and 84, the `provider` property is ignored (extra property) and the `driver` field remains `undefined`. The error message includes `driver: Schema.optional(Schema.String)` but never receives the actual provider value.
- apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts:45 -- The `driver` field in `OpenCodeReplayTranscriptDecodeError` will always be `undefined` because `transcriptMetadata()` returns an object with key `provider`, not `driver`. When spreading `{...transcriptMetadata(transcript), cause}` into the constructor, the `provider` value is passed but ignored (no matching schema field), while `driver` remains unset.
- apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts:61 -- The `driver` field in `CursorReplayTranscriptDecodeError` will always be `undefined` because `metadataFromTranscript()` returns an object with key `provider`, not `driver`. When spreading `{...metadataFromTranscript(transcript), cause}` into the constructor, the `provider` value is passed but ignored (no matching schema field), while `driver` remains unset.
| providerTurnId: null, | ||
| nativeItemRef: null, | ||
| parentItemId: null, | ||
| ordinal: ordinal * 100, |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/Orchestrator.ts:1683
The ordinal * 100 formula here (and at lines 2511 and 2539) can produce ordinals that collide with or fall behind ordinals already assigned by nextTurnItemOrdinal(projection) in the normal dispatch path (line 1929). For example, if a thread has accumulated 350 turn items via nextTurnItemOrdinal, a queued run with run ordinal 3 computes 3 * 100 = 300, placing the new item before 50 existing items. Consider using nextTurnItemOrdinal(projection) consistently for all turn item ordinals, with sequential assignment for multi-item cases (e.g., handoff = N, user message = N + 1).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Orchestrator.ts around line 1683:
The `ordinal * 100` formula here (and at lines 2511 and 2539) can produce ordinals that collide with or fall behind ordinals already assigned by `nextTurnItemOrdinal(projection)` in the normal dispatch path (line 1929). For example, if a thread has accumulated 350 turn items via `nextTurnItemOrdinal`, a queued run with run ordinal 3 computes `3 * 100 = 300`, placing the new item *before* 50 existing items. Consider using `nextTurnItemOrdinal(projection)` consistently for all turn item ordinals, with sequential assignment for multi-item cases (e.g., handoff = N, user message = N + 1).
Evidence trail:
- apps/server/src/orchestration-v2/Orchestrator.ts line 163-165: `nextRunOrdinal` returns `projection.runs.length + 1`
- apps/server/src/orchestration-v2/Orchestrator.ts line 194-196: `nextTurnItemOrdinal` returns `Math.max(0, ...projection.turnItems.map(item => item.ordinal)) + 1`
- apps/server/src/orchestration-v2/Orchestrator.ts line 1577: `ordinal = nextRunOrdinal(projection)` for queued run
- apps/server/src/orchestration-v2/Orchestrator.ts line 1683: `ordinal: ordinal * 100` for queued run's turn item
- apps/server/src/orchestration-v2/Orchestrator.ts line 1929: `ordinal: nextTurnItemOrdinal(projection)` for normal dispatch
- apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.ts line 863-869: `nextWithinTurn` counter is unbounded, `ordinal = context.input.runOrdinal * 100 + nextWithinTurn`
- apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts line 713-719: same unbounded pattern
- apps/server/src/orchestration-v2/ProjectionStore.ts line 424-455: `sortMessagesByTurnItemOrder` sorts by ordinal, confirming ordinals affect display order
- Track explicit thread-to-session bindings and detach without stopping siblings - Apply runtime settings per Codex thread and filter multiplexed events by turn - Add migration, projection, lifecycle, and concurrency coverage
| steps.push({ type: "await", key: `run:${messageIndex - 1}` }); | ||
| steps.push({ type: "await_thread_idle", threadId: ids.threadId }); |
There was a problem hiding this comment.
🟢 Low fixtures/shared.ts:475
In the "queue_message" case, the await step for run:${messageIndex - 1} is pushed at line 475 but the corresponding key is never removed from activeRunDispatchKeys. The steer, restart, and interrupt cases all call activeRunDispatchKeys.delete(...) when they emit an await step. Because the key remains in the set, the guard at line 649 (activeRunDispatchKeys.size > 0) fires and appends a redundant await_all + await_thread_idle pair even though that run was already awaited.
steps.push({ type: "await", key: `run:${messageIndex - 1}` });
+ activeRunDispatchKeys.delete(`run:${messageIndex - 1}`);
steps.push({ type: "await_thread_idle", threadId: ids.threadId });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/testkit/fixtures/shared.ts around lines 475-476:
In the `"queue_message"` case, the `await` step for `run:${messageIndex - 1}` is pushed at line 475 but the corresponding key is never removed from `activeRunDispatchKeys`. The `steer`, `restart`, and `interrupt` cases all call `activeRunDispatchKeys.delete(...)` when they emit an `await` step. Because the key remains in the set, the guard at line 649 (`activeRunDispatchKeys.size > 0`) fires and appends a redundant `await_all` + `await_thread_idle` pair even though that run was already awaited.
Evidence trail:
apps/server/src/orchestration-v2/testkit/fixtures/shared.ts lines 419-426 (shouldRunInBackground set true when next step is queue_message), line 445 (activeRunDispatchKeys.add(key)), line 475 (await step pushed but no delete from activeRunDispatchKeys), line 558 (steer case deletes from set), line 592 (restart case deletes), line 619 (interrupt case deletes), line 649 (guard fires if set not empty).
| ); | ||
| }), | ||
| ), | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟠 High orchestration-v2/RunExecutionService.ts:532
When startTurn fails (line 579), its error handler interrupts providerEventFiber (line 584) and then writes final run events with "failed" status (lines 587–598). However, Effect.catchCause on line 532 catches all causes — including interrupt causes — so the interrupted fiber also logs a warning and writes its own set of final run events (run.updated, node.updated, provider-thread.updated) with "failed" status (lines 539–549). This produces duplicate final run events in the event sink.
The catchCause handler should check whether the cause is interrupt-only (e.g. via Cause.isInterruptedOnly) and re-propagate it with Effect.failCause(cause) instead of executing the recovery logic that writes final events.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 532:
When `startTurn` fails (line 579), its error handler interrupts `providerEventFiber` (line 584) and then writes final run events with `"failed"` status (lines 587–598). However, `Effect.catchCause` on line 532 catches *all* causes — including interrupt causes — so the interrupted fiber also logs a warning and writes its own set of final run events (`run.updated`, `node.updated`, `provider-thread.updated`) with `"failed"` status (lines 539–549). This produces duplicate final run events in the event sink.
The `catchCause` handler should check whether the cause is interrupt-only (e.g. via `Cause.isInterruptedOnly`) and re-propagate it with `Effect.failCause(cause)` instead of executing the recovery logic that writes final events.
Evidence trail:
- apps/server/src/orchestration-v2/RunExecutionService.ts lines 480-561: `providerEventFiber` pipeline with `Effect.catchCause` at line 532 that writes final events on ANY cause
- apps/server/src/orchestration-v2/RunExecutionService.ts lines 564-608: `startTurn` error handler that interrupts the fiber (line 584) and then also writes final events (lines 587-598)
- pnpm-workspace.yaml line 24: `effect: 4.0.0-beta.78` (confirms Effect v4)
- Effect-TS docs (https://effect.website/docs/data-types/cause): `Cause` includes `Interrupt` variant, and `Effect.catchCause` catches all causes
- Effect Platform Runtime (https://github.com/Effect-TS/effect/blob/main/packages/platform/src/Runtime.ts): uses `Cause.isInterruptedOnly` to distinguish interrupt-only causes, confirming the pattern exists for this purpose
- github.com/Effect-TS/effect-smol/blob/main/migration/cause.md: Confirms v4 Cause has Fail, Die, and Interrupt reason types
There was a problem hiding this comment.
One Effect error-convention issue: a wrapper message is being derived from cause.message. See inline comment.
Posted via Macroscope — Effect Service Conventions
| new ProviderAdapterDriverCreateError({ | ||
| driver: CODEX_DRIVER_KIND, | ||
| instanceId, | ||
| detail: cause.message, | ||
| cause, |
There was a problem hiding this comment.
detail: cause.message makes the wrapper's caller-visible message derive from cause.message — ProviderAdapterDriverCreateError.message is built from this.detail (Failed to create orchestration-v2 provider adapter ... : ${this.detail}). The conventions require the wrapper message to come only from stable structural attributes, with the real failure preserved via cause (already done here). Use a static descriptive detail like the other drivers (ClaudeAdapterV2, CursorAdapterV2, etc.) instead of copying cause.message.
| new ProviderAdapterDriverCreateError({ | |
| driver: CODEX_DRIVER_KIND, | |
| instanceId, | |
| detail: cause.message, | |
| cause, | |
| new ProviderAdapterDriverCreateError({ | |
| driver: CODEX_DRIVER_KIND, | |
| instanceId, | |
| detail: "Failed to materialize the Codex shadow home.", | |
| cause, |
Posted via Macroscope — Effect Service Conventions
| yield* Ref.set(assistantSegmentRef, { nextSegmentIndex: 0 }); |
There was a problem hiding this comment.
🟡 Medium acp/AcpSessionRuntime.ts:486
The adoptSession function resets assistantSegmentRef to { nextSegmentIndex: 0 } without first calling closeActiveAssistantSegment, so any active assistant segment is silently discarded when switching sessions via loadSession, resumeSession, or forkSession. This leaves consumers with an orphaned AssistantItemStarted event that never gets a matching AssistantItemCompleted. Consider closing the active segment before resetting the ref.
| yield* Ref.set(assistantSegmentRef, { nextSegmentIndex: 0 }); | |
| yield* closeActiveAssistantSegment({ queue: eventQueue, assistantSegmentRef }); | |
| yield* Ref.set(assistantSegmentRef, { nextSegmentIndex: 0 }); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 486:
The `adoptSession` function resets `assistantSegmentRef` to `{ nextSegmentIndex: 0 }` without first calling `closeActiveAssistantSegment`, so any active assistant segment is silently discarded when switching sessions via `loadSession`, `resumeSession`, or `forkSession`. This leaves consumers with an orphaned `AssistantItemStarted` event that never gets a matching `AssistantItemCompleted`. Consider closing the active segment before resetting the ref.
Evidence trail:
apps/server/src/provider/acp/AcpSessionRuntime.ts lines 467-489 (adoptSession resets ref at line 486), lines 1017-1037 (closeActiveAssistantSegment emits AssistantItemCompleted), lines 788-804 (prompt properly calls closeActiveAssistantSegment), lines 903-906 (handleSessionUpdate properly calls closeActiveAssistantSegment), lines 719/735/751 (callers of adoptSession), lines 369-377 (async notification handler), lines 935-939 (ensureActiveAssistantSegment sets activeItemId)
Retain the existing event-sourced application data plane for projects and shell state while making orchestration v2 the sole agent runtime. Adapt v2 events and receipts to the shared infrastructure and remove the v1 provider reactors.
juliusmarminge
force-pushed
the
t3code/codex-turn-mapping
branch
from
562fbfc to
7a40fa1
Compare
| const parts = expanded.split("<any>"); | ||
| let offset = 0; | ||
| for (const part of parts) { | ||
| const index = actual.indexOf(part, offset); | ||
| if (index === -1) return false; | ||
| offset = index + part.length; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🟡 Medium scripts/acp-replay-agent.ts:92
When expected is "abc<any>def", matchesExpected incorrectly matches an actual value like "XXXabcYYYdef" because indexOf("abc", 0) finds it at index 3 rather than requiring it at index 0. The first literal segment (before the first <any>) must be anchored to the start of the string.
| const parts = expanded.split("<any>"); | |
| let offset = 0; | |
| for (const part of parts) { | |
| const index = actual.indexOf(part, offset); | |
| if (index === -1) return false; | |
| offset = index + part.length; | |
| } | |
| return true; | |
| const parts = expanded.split("<any>"); | |
| let offset = 0; | |
| for (const [i, part] of parts.entries()) { | |
| const index = actual.indexOf(part, offset); | |
| if (index === -1 || (i === 0 && index !== 0)) return false; | |
| offset = index + part.length; | |
| } | |
| return true; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/scripts/acp-replay-agent.ts around lines 92-99:
When `expected` is `"abc<any>def"`, `matchesExpected` incorrectly matches an `actual` value like `"XXXabcYYYdef"` because `indexOf("abc", 0)` finds it at index 3 rather than requiring it at index 0. The first literal segment (before the first `<any>`) must be anchored to the start of the string.
Evidence trail:
apps/server/scripts/acp-replay-agent.ts lines 87-99 at REVIEWED_COMMIT. The `matchesExpected` function splits on `<any>` and uses `indexOf(part, offset)` in a loop (line 95), never checking that the first segment starts at index 0. `"XXXabcYYYdef".indexOf("abc", 0)` returns 3 (not 0), so the first literal segment is not anchored, producing a false positive match.
| projection.runs.some((run) => run.status === "running" || run.status === "waiting") |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderRuntimeRecoveryService.ts:450
The .some check inside recoverProjection only looks for running or waiting runs, so when a projection has only starting runs and no sessions are resumed, terminalize is skipped and those runs remain stuck in starting indefinitely. Consider adding || run.status === "starting" to align with the activeRuns helper.
- projection.runs.some((run) => run.status === "running" || run.status === "waiting")
+ projection.runs.some((run) => run.status === "running" || run.status === "waiting" || run.status === "starting")🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts around line 450:
The `.some` check inside `recoverProjection` only looks for `running` or `waiting` runs, so when a projection has only `starting` runs and no sessions are resumed, `terminalize` is skipped and those runs remain stuck in `starting` indefinitely. Consider adding `|| run.status === "starting"` to align with the `activeRuns` helper.
Evidence trail:
apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts lines 242-245 (activeRuns includes 'starting'), lines 448-450 (.some guard omits 'starting'), lines 259-264 (terminalize uses activeRuns), line 458 (returns 0 terminalizedRuns when guard is false)
- Define Shape 4.0 for the client cutover and product parity - Define Shape 4.5 for V2-native UI capabilities - Defer existing V1 state migration to an out-of-scope Stage 5
- Migrate web and mobile state, commands, and persistence to V2 projections - Render conversations from authoritative visible turn items - Harden runtime policy, launch handling, attachments, and cache invalidation
| @@ -2017,12 +2037,22 @@ function ChatViewContent(props: ChatViewProps) { | |||
| } | |||
| return [...serverMessagesWithPreviewHandoff, ...pendingMessages]; | |||
| }, [attachmentPreviewHandoffByMessageId, displayServerMessages, optimisticUserMessages]); | |||
| const timelineEntries = useMemo( | |||
| const serverTimelineEntries = useMemo( | |||
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:2040
For server threads, serverTimelineEntries (line 2040) resolves attachment preview URLs exclusively from serverAttachmentUrlById, which is populated by useAssetUrls. The attachmentPreviewHandoffByMessageId overlay — which bridges blob preview URLs from optimistic messages to server-committed messages — is only applied in the draftTimelineEntries path via timelineMessages. This means that when a server thread commits a user message with images, the optimistic message is removed (blob URLs handed off), but serverAttachmentUrlById hasn't resolved the server asset URL yet (the async result is still pending for newly committed attachment IDs). During that window the images render with no preview URL, causing a visible flash.
Consider merging the handoff blob URLs into the attachmentUrlById map passed to deriveTimelineEntriesFromVisibleTurnItems so that serverTimelineEntries falls back to the handoff blob URL for any attachment whose server URL hasn't resolved yet.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 2040:
For server threads, `serverTimelineEntries` (line 2040) resolves attachment preview URLs exclusively from `serverAttachmentUrlById`, which is populated by `useAssetUrls`. The `attachmentPreviewHandoffByMessageId` overlay — which bridges blob preview URLs from optimistic messages to server-committed messages — is only applied in the `draftTimelineEntries` path via `timelineMessages`. This means that when a server thread commits a user message with images, the optimistic message is removed (blob URLs handed off), but `serverAttachmentUrlById` hasn't resolved the server asset URL yet (the async result is still pending for newly committed attachment IDs). During that window the images render with no preview URL, causing a visible flash.
Consider merging the handoff blob URLs into the `attachmentUrlById` map passed to `deriveTimelineEntriesFromVisibleTurnItems` so that `serverTimelineEntries` falls back to the handoff blob URL for any attachment whose server URL hasn't resolved yet.
Evidence trail:
apps/web/src/components/ChatView.tsx line 2040-2048 (serverTimelineEntries uses only serverAttachmentUrlById), line 2049-2053 (draftTimelineEntries uses timelineMessages which has handoff applied), line 1987-2038 (timelineMessages applies attachmentPreviewHandoffByMessageId), line 2054 (isServerThread selects serverTimelineEntries), line 1877-1886 (serverAttachmentUrlById from useAssetUrls), line 3213-3239 (handoff triggered on optimistic→server transition). apps/web/src/session-logic.ts line 470-473 (deriveTimelineEntriesFromVisibleTurnItems resolves previewUrl only from attachmentUrlById map).
| if (left === null) return right; | ||
| if (right === null) return left; | ||
| return Date.parse(right) > Date.parse(left) ? right : left; |
There was a problem hiding this comment.
🟢 Low lib/threadActivity.ts:258
When left is an invalid ISO string, Date.parse(left) returns NaN, and Date.parse(right) > NaN evaluates to false, causing the function to return left (the invalid string) instead of right (the valid one). Consider restoring the Number.isFinite checks so invalid timestamps are rejected and the valid argument is returned.
- if (left === null) return right;
- if (right === null) return left;
- return Date.parse(right) > Date.parse(left) ? right : left;
+ if (left === null) return right;
+ if (right === null) return left;
+ const leftMs = Date.parse(left);
+ const rightMs = Date.parse(right);
+ if (!Number.isFinite(leftMs)) return right;
+ if (!Number.isFinite(rightMs)) return left;
+ return rightMs > leftMs ? right : left;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/lib/threadActivity.ts around lines 258-260:
When `left` is an invalid ISO string, `Date.parse(left)` returns `NaN`, and `Date.parse(right) > NaN` evaluates to `false`, causing the function to return `left` (the invalid string) instead of `right` (the valid one). Consider restoring the `Number.isFinite` checks so invalid timestamps are rejected and the valid argument is returned.
Evidence trail:
apps/mobile/src/lib/threadActivity.ts lines 257-261 (REVIEWED_COMMIT): `maxIsoTimestamp` without `Number.isFinite` checks. apps/web/src/components/chat/MessagesTimeline.logic.ts lines 20-28 (REVIEWED_COMMIT): the web version of `maxIsoTimestamp` WITH `Number.isFinite` checks (lines 25-26). JavaScript spec: any comparison with NaN returns false, confirming the behavioral claim.
1 participant
Summary
Validation
Notes
Note
Wire orchestration v2 provider adapters, contracts, replay testkit, and WebSocket RPCs
runtimeLayer.tsand exposed via the server layer.CodexAdapterV2.ts,ClaudeAdapterV2.ts) with capability matrices, protocol loggers, and provider adapter registry.dispatchCommand,getThreadProjection,subscribeShell,subscribeThread) exposed through the server WS layer and fronted by the web RPC client andEnvironmentApi.031_OrchestrationV2.ts) creating orchestration v2 event, command receipt, and projection tables (threads, runs, run attempts, nodes, provider sessions).orchestrationV2.tscovering commands, domain events, projections, stream items, and RPC schemas, plus new branded entity IDs inbaseSchemas.ts.📊 Macroscope summarized 79031a1. 31 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.