[orchestrator-v2] Fix shell cache hydration and multi-env project grouping#3640
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This PR introduces substantial new logic for shell cache hydration and multi-environment project grouping. Unresolved review comments identify potential bugs including a high-severity issue where cache decoding may silently fail for projects, plus concerns about the persistence loop and identity clearing logic. You can customize Macroscope's approvability policy. Learn more. |
8866ebd to
191408f
Compare
191408f to
d6a0e95
Compare
Dismissing prior approval to re-evaluate d6a0e95
d6a0e95 to
275c738
Compare
275c738 to
a9a74af
Compare
fcce425 to
daa9283
Compare
b65e9fa to
e096da2
Compare
| export type OrchestrationV2ThreadShellSnapshotJson = | ||
| typeof OrchestrationV2ThreadShellSnapshotJson.Type; | ||
|
|
||
| export const OrchestrationV2ShellSnapshotJson = OrchestrationV2ShellSnapshot.mapFields( |
There was a problem hiding this comment.
🟠 High src/orchestrationV2.ts:1601
OrchestrationV2ShellSnapshotJson overrides threads and archivedThreads with JSON variants but leaves projects pointing at the original OrchestrationProjectShell, whose createdAt and updatedAt fields are Schema.DateTimeUtc. When a cache row is decoded via Schema.fromJsonString, those fields are ISO strings, so any non-empty projects array fails decoding and the snapshot is discarded. This silently breaks fast-start hydration. Add a OrchestrationProjectShellJson variant that uses Schema.DateTimeUtcFromString for its datetime fields and override projects in OrchestrationV2ShellSnapshotJson.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/orchestrationV2.ts around line 1601:
`OrchestrationV2ShellSnapshotJson` overrides `threads` and `archivedThreads` with JSON variants but leaves `projects` pointing at the original `OrchestrationProjectShell`, whose `createdAt` and `updatedAt` fields are `Schema.DateTimeUtc`. When a cache row is decoded via `Schema.fromJsonString`, those fields are ISO strings, so any non-empty `projects` array fails decoding and the snapshot is discarded. This silently breaks fast-start hydration. Add a `OrchestrationProjectShellJson` variant that uses `Schema.DateTimeUtcFromString` for its datetime fields and override `projects` in `OrchestrationV2ShellSnapshotJson`.
| return items.map((candidate, candidateIndex) => (candidateIndex === index ? item : candidate)); | ||
| } | ||
|
|
||
| function retainRepositoryIdentity( |
There was a problem hiding this comment.
🟡 Medium state/shellReducer.ts:16
retainRepositoryIdentity treats every incoming null repositoryIdentity as a transient enrichment miss and restores the previous value, so a legitimate removal of a repository remote can never clear the identity while the project id and workspace root stay the same. After git remote -v finds no fetch remote, the resolver returns null, but both mergeShellSnapshotProjects and applyShellStreamEvent route that through this helper, so the stale identity stays displayed and persisted indefinitely. Consider distinguishing a real null resolution from a missing/in-flight value, or document why all null values should be suppressed here.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/shellReducer.ts around line 16:
`retainRepositoryIdentity` treats every incoming `null` `repositoryIdentity` as a transient enrichment miss and restores the previous value, so a legitimate removal of a repository remote can never clear the identity while the project id and workspace root stay the same. After `git remote -v` finds no fetch remote, the resolver returns `null`, but both `mergeShellSnapshotProjects` and `applyShellStreamEvent` route that through this helper, so the stale identity stays displayed and persisted indefinitely. Consider distinguishing a real `null` resolution from a missing/in-flight value, or document why all `null` values should be suppressed here.
1980b38 to
0f3ceb5
Compare
e096da2 to
1c3140f
Compare
1c3140f to
94b925b
Compare
94b925b to
52f6436
Compare
2ff5876 to
293a498
Compare
293a498 to
c7a1a1f
Compare
c7a1a1f to
749ff38
Compare
749ff38 to
3ebbf5e
Compare
1e58e65 to
a286c60
Compare
3ebbf5e to
dcb3561
Compare
Ignore Grok ACP JSON-RPC responses with non-numeric ids before they reach Effect RpcClient so agent-internal messages such as skills-reload do not break provider startup. Persist and round-trip the latest live shell snapshot through IndexedDB so project and thread shells can hydrate from cache before live synchronization catches up. Await repository-identity resolution when building shell snapshots (HTTP and subscribeShell), and re-emit an enriched snapshot on every subscribe attach including afterSequence resumes. Preserve previously resolved repositoryIdentity across project deltas and full snapshot reloads for the same workspace root so multi-environment grouping does not split during enrichment churn.
Persisted shell snapshots encode settledAt as an ISO string. Decode it through DateTimeUtcFromString and cover the non-null cache round trip. Co-authored-by: codex <codex@users.noreply.github.com>
dcb3561 to
6764a41
Compare
| const latestSnapshot = yield* Ref.get(latestLiveSnapshot); | ||
| // Same sequence can still mean newer content (e.g. repository enrichment). | ||
| if (Option.isNone(latestSnapshot) || latestSnapshot.value === nextSnapshot) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Medium state/shell.ts:96
The persist loop never terminates while shell events keep arriving: after each cache.saveShell completes, it reads latestLiveSnapshot and compares by reference equality. Every new snapshot is a fresh object allocation, so the comparison always fails and the loop starts another save immediately, bypassing the 500 ms debounce and producing continuous storage writes for the entire session. Consider limiting the re-save to only the same-sequence race (stop once the sequence differs from what was just saved) or draining via the queue rather than chasing an indefinitely advancing live reference.
const latestSnapshot = yield* Ref.get(latestLiveSnapshot);
// Same sequence can still mean newer content (e.g. repository enrichment).
- if (Option.isNone(latestSnapshot) || latestSnapshot.value === nextSnapshot) {
+ if (
+ Option.isNone(latestSnapshot) ||
+ latestSnapshot.value === nextSnapshot ||
+ latestSnapshot.value.snapshotSequence !== nextSnapshot.snapshotSequence
+ ) {
return;
}🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/shell.ts around lines 96-100:
The `persist` loop never terminates while shell events keep arriving: after each `cache.saveShell` completes, it reads `latestLiveSnapshot` and compares by reference equality. Every new snapshot is a fresh object allocation, so the comparison always fails and the loop starts another save immediately, bypassing the 500 ms debounce and producing continuous storage writes for the entire session. Consider limiting the re-save to only the same-sequence race (stop once the sequence differs from what was just saved) or draining via the queue rather than chasing an indefinitely advancing live reference.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6764a41. Configure here.
| initial, | ||
| tail: Stream.concat(Stream.concat(replay, completionMarker), liveFrom(highWater)), | ||
| enrichment: enrichmentRefreshes, | ||
| }); |
There was a problem hiding this comment.
Stale shell snapshot after sequence reset
Medium Severity
subscribeShell now loads the initial shell snapshot before reading highWater. If the application sequence moves backward between those reads (server DB rebuild/reset), the already-captured snapshot can stay ahead of the new high water. That stale frame is still emitted as the authoritative prefix, so the client can adopt ghost projects/threads and ignore the post-reset replay and live deltas.
Reviewed by Cursor Bugbot for commit 6764a41. Configure here.
aee7ff3
into
pingdotgg:t3code/codex-turn-mapping


Summary
Problem and Fix
afterSequenceresumes, then continue with replay and live deltas.Defensive Fixes
snapshotmember and add optional metadata. Effect's legacy struct decoder accepts and strips the excess field, covered by a compatibility test.Validation
vp test run packages/contracts/src/orchestrationV2.test.ts packages/client-runtime/src/state/shellReducer.test.ts packages/client-runtime/src/state/shell-sync.test.ts apps/server/src/orchestration-v2/ShellStream.test.ts apps/server/src/project/ProjectEnrichmentService.test.ts: 49 tests passvp fmt --checkandvp lintfor the touched files: pass, with pre-existing inline-schema warnings only@t3tools/contracts,@t3tools/client-runtime, andt3: pass, with pre-existing Effect suggestions onlyNote
Fix shell cache hydration and multi-env project grouping in orchestrator-v2
mergeShellSnapshotProjectsto shellReducer.ts to merge incoming snapshots with prior state, preservingrepositoryIdentityfor unchanged roots in authoritative snapshots and avoiding structural rollbacks for enrichment snapshots at or below the cached sequence.EnvironmentShellStatein shell.ts to track the latest live snapshot and re-save it if a newer same-sequence snapshot arrives while a prior save is in flight; flushes on disconnect and scope teardown without blocking state updates.resolvedRepositoryIdentityRoots, before any enrichment refresh can appear.*Jsonvariants) in orchestrationV2.ts and updates orchestrationCache.ts to use them for persisted shell and thread snapshots, affecting date/timestamp serialization.repositoryIdentityResolvedtoProjectEnrichmentin ProjectEnrichmentService.ts to distinguish resolved (including cached null) from pending/failed identity resolution.Macroscope summarized 6764a41.
Note
Medium Risk
Touches live shell sync, WebSocket ordering, and client cache persistence across server resets and same-sequence enrichment; mistakes could mis-group projects or serve stale shell state, but behavior is heavily test-covered.
Overview
Fixes orchestrator v2 shell sync so clients hydrate from cache reliably, group projects by git identity across environments, and stay correct when server sequence resets or enrichment races the initial snapshot.
Server: HTTP and WebSocket shell paths use non-blocking
getAvailablefor project enrichment and exposerepositoryIdentityResolved. Shell subscribe always rehydrates a full snapshot (includingafterSequenceresumes), emits an unmarked authoritative frame first, optionally a same-sequence frame withresolvedRepositoryIdentityRoots, then merges batched enrichment refreshes only after that prefix viacomposeShellStreamWithEnrichment.Client:
mergeShellSnapshotProjectsdistinguishes authoritative resets (lower sequence still replaces structure) from enrichment-only updates (identity patches, no structural rollback). Shell state tracks the latest live snapshot, flushes on disconnect/scope close without blocking UI, and re-persists when a same-sequence enriched snapshot lands during an in-flight save. Orchestration cache v3 uses JSON-safe snapshot schemas for date fields.ACP: Drops JSON-RPC responses with non-numeric ids before they hit the Effect RPC client (Grok agent-internal messages).
Reviewed by Cursor Bugbot for commit 6764a41. Bugbot is set up for automated code reviews on this repo. Configure here.