Skip to content

feat(orchestrator): introduce new orchestrator - #2829

Open
juliusmarminge wants to merge 214 commits into
mainfrom
t3code/codex-turn-mapping
Open

feat(orchestrator): introduce new orchestrator#2829
juliusmarminge wants to merge 214 commits into
mainfrom
t3code/codex-turn-mapping

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented May 27, 2026

Copy link
Copy Markdown
Member

Summary

  • wire orchestration V2 provider adapter registry/factory flow for Codex and Claude provider instances
  • add Claude replay/query primitives, native fork/rollback fixtures, subagent fixture coverage, and provider replay harness updates
  • update debugger model/provider picker and improve user-facing orchestration errors

Validation

  • bun fmt
  • bun lint
  • bun typecheck
  • bun run test -- src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts -t claudeAgent
  • bun run test -- src/orchestration-v2/testkit/ClaudeReplayFixtures.integration.test.ts
  • bun run test -- src/orchestration-v2/testkit/ThreadFork.integration.test.ts -t Claude

Notes

  • Draft PR for review of current branch state. Codex all-provider replay still needs schema alignment with latest app-server behavior before it can be treated as a full-suite signal.

Note

Introduce orchestration V2 runtime with new provider adapters, MCP toolkits, and project service

  • Adds a full orchestration V2 runtime layer with event sourcing, projection store, effect outbox, checkpoint capture/rollback, and run execution services wired together in runtimeLayer.ts
  • Introduces V2 provider adapters for Claude, Codex, Cursor, Grok, OpenCode, and ACP Registry, each implementing ProviderAdapterV2Shape with session/turn lifecycle, subagent support, and structured error types
  • Adds MCP toolkits for orchestrator (delegate_task, thread lifecycle, scheduled tasks) and worktree (handoff, status) operations, registered in McpHttpServer.ts
  • Introduces ProjectService with enrichment (favicon, repository identity), ScheduledTaskService with interval/fixed-time scheduling, and HTTP endpoints for both in the environment API
  • Migrates IPC and cache schemas to V2 types (OrchestrationV2ShellSnapshot, OrchestrationV2ThreadProjection), adds V2 atoms for shell/thread streams, and updates sidebar/composer/thread-detail UI components to use runtime-based state
  • Adds 9 database migrations (035–043) covering V2 events, projections, effect outbox, provider session bindings, scheduled tasks, legacy import state, and application event source consolidation
  • Risk: IPC API is breaking — /api/orchestration/snapshot removed, orchestration method names changed to V2 variants, and thread snapshot cache key now uses projection.thread.id; clients must be updated simultaneously

Macroscope summarized a73a9ff.


Note

High Risk
High risk from a large orchestration cutover touching run lifecycle, provider sessions, approvals, forks/merge-back, and persisted client cache schema—any regression affects core agent workflows and reconnect behavior.

Overview
This PR lands Orchestration V2 as the live orchestration path (per the broader branch) and, in the shown diff, rewires mobile to V2 shell/thread projections, shared cache schema versioning, and run-centric semantics (runtime vs legacy session, RunId vs turn ids).

Mobile thread experience now consumes OrchestrationV2ShellSnapshot / thread detail projections, persists them with ORCHESTRATION_CACHE_SCHEMA_VERSION, and drives the chat from V2 feed/activity models. New UI includes queue reorder/promote-to-steer, a thread lineage / merge-back / detach banner, fork-from-assistant-response, expandable activity inspectors with checkpoint rollback, and user-message intent badges. Approvals and user-input cards respect responseCapability when the provider process is gone. Archive rules distinguish queued vs active provider work; the list can show a Done state from lastVisitedAt vs updatedAt.

Supporting changes: CI installs build-essential so ACP process-tree live tests compile fixtures instead of soft-skipping. Desktop exposes userDataDirName / legacy alpha dirname. Marketing updates the Cursor harness label.

Large .plans documents record thread-lineage/context-transfer implementation status and the multi-shape V2 application integration roadmap (mostly documentation in this diff slice).

Reviewed by Cursor Bugbot for commit a73a9ff. Bugbot is set up for automated code reviews on this repo. Configure here.

juliusmarminge and others added 30 commits April 17, 2026 17:29
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
Integrate the native resource diagnostics rewrite with orchestration V2, including shared provider event logging, RPC authorization, and background-policy wiring.

Co-authored-by: codex <codex@users.noreply.github.com>
environmentId: Schema.String,
threadId: Schema.String,
snapshot: OrchestrationThreadDetailSnapshot,
snapshot: OrchestrationV2ThreadDetailSnapshot,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mobile V2 cache uses wrong schemas

High Severity

Mobile shell/thread cache still defines local schemas at versions 1/2 over OrchestrationV2ShellSnapshot and OrchestrationV2ThreadDetailSnapshot, then round-trips them with Schema.fromJsonString. Those types use DateTimeUtc, so JSON loads see ISO strings and fail decode; the store discards the record as corrupt. Web already uses the shared ORCHESTRATION_CACHE_SCHEMA_VERSION (3) with OrchestrationV2ShellSnapshotJson / projection JSON fields for this reason. Offline shell and thread restore on mobile effectively never stick after the V2 cutover.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5065ad0. Configure here.

Comment thread apps/server/src/orchestration-v2/testkit/fixtures/todo_list/grok_output.ts Outdated
Comment thread apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts Outdated
juliusmarminge and others added 2 commits July 29, 2026 18:21
Use shared cache schemas, close the OpenCode abort race, and assert fixture collection sizes correctly.

Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the Orchestration V2 service definitions, layers, and provider adapter/driver wiring against the Effect service conventions.

The previously flagged detail: cause.message in CodexAdapterV2 is resolved — the driver now passes static descriptive detail values and preserves the real failure as cause.

One remaining pattern in the new V2 services: several service-level failures distinguish semantically distinct outcomes only by a prose sentence stored in the Schema.Defect() cause slot, with no message getter derived from structural attributes. Those strings are also what surfaces to callers through orchestration-v2/UserFacingErrors.ts (messageFrom reads string cause values), so the caller-visible message is unstructured data rather than derived state, and cause no longer holds a real underlying failure. Inline comments cover CheckpointRollbackService.ts and RuntimeRequestService.ts; the same shape also appears in CheckpointCaptureService.ts (line 89), ProviderTurnControlService.ts (lines 112, 266) and TurnItemPositionStore.ts (line 82) if you want to normalize them together.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/orchestration-v2/RuntimeRequestService.ts
Comment thread apps/server/src/orchestration-v2/CheckpointRollbackService.ts Outdated
<Text className="mb-1 pr-1 font-t3-medium text-2xs text-foreground-muted opacity-60">
Sent by another agent
</Text>
) : null}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Queued messages lack intent labels

Medium Severity

The ThreadFeed on mobile doesn't visually distinguish messages with queued_turn or steer intents. This causes confusion because these messages appear as ordinary sent bubbles in the chat timeline while also being listed in the ThreadQueueControl, creating a double display. The web client already handles this distinction with intent badges.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6643f4e. Configure here.

juliusmarminge and others added 2 commits July 29, 2026 18:45
Distinguish queued and steering user messages in the mobile thread feed, matching the web timeline.

Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Comment thread apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
Co-authored-by: codex <codex@users.noreply.github.com>

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the new Effect service code in this PR against the service conventions. One new finding on ThreadManagementService.ts error modeling; two previously flagged error-modeling findings (CheckpointRollbackService.ts, RuntimeRequestService.ts) are still open and were not re-commented.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/orchestration-v2/ThreadManagementService.ts Outdated
Comment thread apps/server/src/provider/acp/AcpSessionRuntime.ts
Comment thread apps/server/src/mcp/OrchestratorMcpService.ts Outdated
juliusmarminge and others added 2 commits July 29, 2026 19:22
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Comment thread apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts
Comment thread apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
relativePath,
link.line,
),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inherited file links switch threads

Medium Severity

File links in the new activity inspector resolve paths with the current thread’s workspaceRoot, but navigate to ThreadFile using row.sourceThreadId. Thread selection is driven by threads/:environmentId/:threadId route params, so tapping a file on an inherited row switches the selected thread to the source thread instead of staying on the current one.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bb78a0a. Configure here.

Co-authored-by: codex <codex@users.noreply.github.com>
Comment thread apps/server/src/orchestration-v2/testkit/fixtures/shared.ts
Comment on lines +131 to +155
const trimmed = value.trim();
const next: ProviderInstanceEnvironmentVariable[] = [];
let found = false;

for (const variable of environment ?? []) {
if (variable.name !== field.name) {
next.push(variable);
continue;
}
found = true;
if (trimmed.length > 0) {
next.push({
name: variable.name,
value: trimmed,
sensitive: field.sensitive ?? true,
});
}
}

if (!found && trimmed.length > 0) {
next.push({
name: field.name,
value: trimmed,
sensitive: field.sensitive ?? true,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Low settings/ProviderInstanceCard.tsx:131

nextProviderEnvironmentWithFieldValue trims the value before storing it, so dedicated environment-field values with leading or trailing whitespace are silently altered. A credential such as " token " is stored as "token", which can break authentication or other downstream behavior. Unlike the generic environment table, which trims only the variable name, this trims the actual payload. Consider preserving the raw value for dedicated fields.

-  const trimmed = value.trim();
-  const next: ProviderInstanceEnvironmentVariable[] = [];
+  const next: ProviderInstanceEnvironmentVariable[] = [];
-    if (trimmed.length > 0) {
+    if (value.length > 0) {
-        value: trimmed,
+        value,
-  if (!found && trimmed.length > 0) {
+  if (!found && value.length > 0) {
-      value: trimmed,
+      value,
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/settings/ProviderInstanceCard.tsx around lines 131-155:

`nextProviderEnvironmentWithFieldValue` trims the value before storing it, so dedicated environment-field values with leading or trailing whitespace are silently altered. A credential such as `" token "` is stored as `"token"`, which can break authentication or other downstream behavior. Unlike the generic environment table, which trims only the variable name, this trims the actual payload. Consider preserving the raw value for dedicated fields.

@goggi

goggi commented Jul 29, 2026

Copy link
Copy Markdown

Scheduled task reports success while its new-thread launch fails in background

Tested on aec85c546e597dbcf78db81b134e7438d9a5ada3 with a scheduled task configured to create a new worktree. Reproduced twice with fresh threads.

Observed

The scheduled-task record ends with lastRunStatus: succeeded, but the generated thread has ordinal 1 failed before provider start:

Workspace preparation failed during generate metadata:
Text generation failed in generateBranchName: Failed to write temp file

ENOENT: no such file or directory, open
/tmp/t3code-codex-schema-208069-6MZJpS/919af017300b

The first reproduction used a custom TMPDIR; I restarted the desktop/server with TMPDIR=/tmp and confirmed the active server process inherited it. The second reproduction failed identically, so this is not caused by that local temp configuration, permissions, or available space.

Status propagation

ScheduledTaskService.runTask treats threadLaunch.launch(...) returning as completion and immediately derives lastRunStatus from that Exit. ThreadLaunchService.launch returns successfully after schedulePreparation(...) forks prepareInBackground into preparationScope. When generateBranchName later fails, the prepared run/thread is marked failed, but that failure cannot update the already-completed scheduled-task run.

This creates two user-visible problems:

  1. A recoverable branch-name metadata failure prevents the scheduled task from starting at all.
  2. The scheduled task still reports success because it tracks dispatch rather than background preparation outcome.

Local workaround confirmed from the launch conditions: choose Use the project checkout or Use a specific checkout; both bypass the implicit worktree branch-name generation path. New-worktree scheduled tasks remain broken in this environment.

Suggested direction: await a preparation receipt/outcome before completing the scheduled-task run, and either fall back to fallbackBranchName(threadId) when branch-name generation fails or otherwise make metadata failure non-fatal.

juliusmarminge and others added 2 commits July 29, 2026 20:06
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Comment thread apps/server/src/orchestration-v2/ProviderAdapterRegistry.ts Outdated
juliusmarminge and others added 2 commits July 29, 2026 20:43
Co-authored-by: codex <codex@users.noreply.github.com>
- Publish migration lifecycle events during startup
- Show a web toast while legacy threads are restored
toastManager.close(toastIdRef.current);
toastIdRef.current = null;
}
}, [migration]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Migration toast can stick open

Medium Severity

The new migration toast opens with timeout: 0 on running and only closes when the atom leaves that status. subscribeServerLifecycle reads the snapshot before the PubSub live leg is attached, so a complete published in that gap is dropped for that subscriber. The atom can stay on running and the loading toast never dismisses until remount or reconnect.

Additional Locations (1)
Fix in Cursor Fix in Web

Triggered by learned rule: PubSub subscribe-before-snapshot to avoid change-stream gaps

Reviewed by Cursor Bugbot for commit 1490bf9. Configure here.

}
}, [migration]);

useEffect(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium components/LegacyThreadMigrationToast.tsx:35

The unmount cleanup closes toastIdRef.current but never resets it to null. Under React.StrictMode, the development effect replay runs setup → cleanup → setup again: the cleanup closes the toast added by the first setup, but the ref stays non-null, so the second setup hits the toastIdRef.current !== null early-return and never re-adds it. The running migration notification is absent in development. Reset toastIdRef.current to null after closing it in the cleanup, matching what the status-change effect already does.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/LegacyThreadMigrationToast.tsx around line 35:

The unmount cleanup closes `toastIdRef.current` but never resets it to `null`. Under `React.StrictMode`, the development effect replay runs setup → cleanup → setup again: the cleanup closes the toast added by the first setup, but the ref stays non-null, so the second setup hits the `toastIdRef.current !== null` early-return and never re-adds it. The running migration notification is absent in development. Reset `toastIdRef.current` to `null` after closing it in the cleanup, matching what the status-change effect already does.

Comment on lines +1367 to +1372
const completed = await recordMessagesUntilTurnResults({
iterator,
entries: input.entries,
scenario: input.scenario,
resultCount: input.prompts.length,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 High Adapters/ClaudeAdapterV2.testkit.ts:1367

recordClaudeActiveSteeringQuery waits for input.prompts.length result events from the SDK, but the SDK emits one result per turn — not one per queued message. Since all prompts are offered before the turn runs, they are coalesced into a single turn that produces only one result. recordMessagesUntilTurnResults then blocks forever waiting for the remaining result events that never arrive, so every active_steering recording (which requires ≥2 prompts) hangs indefinitely. Consider setting resultCount to 1 so the loop waits for the single steered turn to complete.

    const completed = await recordMessagesUntilTurnResults({
      iterator,
      entries: input.entries,
      scenario: input.scenario,
-      resultCount: input.prompts.length,
+      resultCount: 1,
    });
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around lines 1367-1372:

`recordClaudeActiveSteeringQuery` waits for `input.prompts.length` `result` events from the SDK, but the SDK emits one `result` per turn — not one per queued message. Since all prompts are offered before the turn runs, they are coalesced into a single turn that produces only one `result`. `recordMessagesUntilTurnResults` then blocks forever waiting for the remaining `result` events that never arrive, so every `active_steering` recording (which requires ≥2 prompts) hangs indefinitely. Consider setting `resultCount` to `1` so the loop waits for the single steered turn to complete.

- Project retry progress and recovery for Codex and Claude turns
- Sync thread completion markers through server-side visit timestamps

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit a73a9ff. Configure here.

environmentId: serverThread.environmentId,
input: { threadId: serverThread.id, visitedAt: serverThread.updatedAt },
});
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Visit watermark misses run completions

High Severity

thread.visit records visitedAt from shell updatedAt, while the Done marker compares against latestRun.completedAt. Shell SQL updatedAt comes from the thread payload and is not advanced by run completion, so opening a finished thread often sets a watermark still older than completedAt. Done then stays lit after the user has already viewed the work, and the early lastVisitedAt >= updatedAt guard can block further visit dispatches forever.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a73a9ff. Configure here.

} finally {
setBusyRunId(null);
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cancelled queue reappears as pending

High Severity

Cancelling a queued run drops it from server queue state, but the matching optimistic queued_turn message stays in optimisticUserMessages. QueuedRunsControl only hides those while a queued run still claims the message id, so after cancel the row comes back as a stuck pending item until the thread is remounted. Optimistic cleanup only runs when a visible turn item appears, which queued/cancelled runs never create.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a73a9ff. Configure here.

if (match === null) continue;
const ownerPid = Number(match[1]);
if (!Number.isSafeInteger(ownerPid) || ownerPid <= 1 || ownerPid === currentPid) continue;
if (isProcessAlive(ownerPid)) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Low acp/AcpSessionRuntime.ts:319

sweepStaleLinuxCgroupSiblings skips removal of an empty cgroup whenever the PID encoded in its name belongs to any live process. After the original owner exits and Linux reuses that PID for an unrelated process, the empty t3-acp-* cgroup is never swept for as long as the unrelated process is alive, so repeated PID reuse can leave stale directories accumulating indefinitely. The check on line 319 only tests whether the PID is alive, not whether the current process is the original lease owner. Consider encoding an owner identity that survives PID reuse (e.g., process start time) into the lease name, or sweeping empty leases regardless of PID liveness.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 319:

`sweepStaleLinuxCgroupSiblings` skips removal of an empty cgroup whenever the PID encoded in its name belongs to any live process. After the original owner exits and Linux reuses that PID for an unrelated process, the empty `t3-acp-*` cgroup is never swept for as long as the unrelated process is alive, so repeated PID reuse can leave stale directories accumulating indefinitely. The check on line 319 only tests whether the PID is alive, not whether the current process is the original lease owner. Consider encoding an owner identity that survives PID reuse (e.g., process start time) into the lease name, or sweeping empty leases regardless of PID liveness.

targetRunId: RunId,
fromProviderThreadIds: Schema.Array(ProviderThreadId),
toProviderThreadId: ProviderThreadId,
coveredRunOrdinals: Schema.Struct({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Low src/orchestrationV2.ts:544

coveredRunOrdinals accepts { from: 5, to: 2 } because it only validates each endpoint as a PositiveInt without enforcing from <= to, unlike TurnCountRange. A decoded handoff with a reversed range passes validation and produces a nonsensical covered-run range downstream instead of rejecting corrupt input. Consider adding a refinement that enforces from <= to, or document why reversed ranges are permitted.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/contracts/src/orchestrationV2.ts around line 544:

`coveredRunOrdinals` accepts `{ from: 5, to: 2 }` because it only validates each endpoint as a `PositiveInt` without enforcing `from <= to`, unlike `TurnCountRange`. A decoded handoff with a reversed range passes validation and produces a nonsensical covered-run range downstream instead of rejecting corrupt input. Consider adding a refinement that enforces `from <= to`, or document why reversed ranges are permitted.

Comment on lines +32 to +43
migratedThreadKeys.add(threadKey);
const local = useUiStateStore.getState().threadLastVisitedAtById[threadKey];
if (!local) continue;
const localMs = Date.parse(local);
if (!Number.isFinite(localMs)) continue;
const serverMs = thread.lastVisitedAt === null ? null : Date.parse(thread.lastVisitedAt);
if (serverMs !== null && Number.isFinite(serverMs) && serverMs >= localMs) continue;
void visitThreadMutation({
environmentId: thread.environmentId,
input: { threadId: thread.id, visitedAt: local },
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium hooks/useThreadVisitedMigration.ts:32

When the visitThreadMutation call on line 39 fails transiently (e.g. the server connection is unavailable at page load), the thread's local visited watermark is never migrated for the rest of the page session. The key is added to migratedThreadKeys before the mutation resolves, and the promise result is discarded with void, so every subsequent effect run skips the thread even after connectivity recovers. Add the key to migratedThreadKeys only after the mutation succeeds, or remove it on failure so the migration retries on the next run.

Suggested change
migratedThreadKeys.add(threadKey);
const local = useUiStateStore.getState().threadLastVisitedAtById[threadKey];
if (!local) continue;
const localMs = Date.parse(local);
if (!Number.isFinite(localMs)) continue;
const serverMs = thread.lastVisitedAt === null ? null : Date.parse(thread.lastVisitedAt);
if (serverMs !== null && Number.isFinite(serverMs) && serverMs >= localMs) continue;
void visitThreadMutation({
environmentId: thread.environmentId,
input: { threadId: thread.id, visitedAt: local },
});
}
const threadKey = scopedThreadKey(scopeThreadRef(thread.environmentId, thread.id));
if (migratedThreadKeys.has(threadKey)) continue;
const local = useUiStateStore.getState().threadLastVisitedAtById[threadKey];
if (!local) continue;
const localMs = Date.parse(local);
if (!Number.isFinite(localMs)) continue;
const serverMs = thread.lastVisitedAt === null ? null : Date.parse(thread.lastVisitedAt);
if (serverMs !== null && Number.isFinite(serverMs) && serverMs >= localMs) continue;
void visitThreadMutation({
environmentId: thread.environmentId,
input: { threadId: thread.id, visitedAt: local },
}).then((result) => {
if (result._tag === "success") {
migratedThreadKeys.add(threadKey);
}
});
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/hooks/useThreadVisitedMigration.ts around lines 32-43:

When the `visitThreadMutation` call on line 39 fails transiently (e.g. the server connection is unavailable at page load), the thread's local visited watermark is never migrated for the rest of the page session. The key is added to `migratedThreadKeys` before the mutation resolves, and the promise result is discarded with `void`, so every subsequent effect run skips the thread even after connectivity recovers. Add the key to `migratedThreadKeys` only after the mutation succeeds, or remove it on failure so the migration retries on the next run.

Comment on lines +263 to +269
const dispatchKey = `${selectedThreadKey}:${selectedThreadUpdatedAt}`;
if (lastDispatchedVisitRef.current === dispatchKey) return;
lastDispatchedVisitRef.current = dispatchKey;
void visitThread({
environmentId: props.environmentId,
input: { threadId: selectedThreadId, visitedAt: selectedThreadUpdatedAt },
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium threads/ThreadDetailScreen.tsx:263

lastDispatchedVisitRef is set before visitThread resolves, and the command result is discarded with void. If the visit request fails transiently, subsequent effect runs for the same updatedAt watermark are permanently deduplicated for this mounted screen, so the server-side visited watermark never updates and the cross-device “Done” marker stays stale until the thread changes or the screen remounts. Set the dedupe marker only after the command succeeds, or clear it on failure.

-    lastDispatchedVisitRef.current = dispatchKey;
-    void visitThread({
-      environmentId: props.environmentId,
-      input: { threadId: selectedThreadId, visitedAt: selectedThreadUpdatedAt },
-    });
+    void visitThread({
+      environmentId: props.environmentId,
+      input: { threadId: selectedThreadId, visitedAt: selectedThreadUpdatedAt },
+    }).then((result) => {
+      if (result._tag === "Success") {
+        lastDispatchedVisitRef.current = dispatchKey;
+      }
+    });
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadDetailScreen.tsx around lines 263-269:

`lastDispatchedVisitRef` is set before `visitThread` resolves, and the command result is discarded with `void`. If the visit request fails transiently, subsequent effect runs for the same `updatedAt` watermark are permanently deduplicated for this mounted screen, so the server-side visited watermark never updates and the cross-device “Done” marker stays stale until the thread changes or the screen remounts. Set the dedupe marker only after the command succeeds, or clear it on failure.

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

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants