Guard provider lifecycle and checkpoints to primary thread/active turn#106
Merged
juliusmarminge merged 4 commits intomainfrom Feb 27, 2026
Merged
Guard provider lifecycle and checkpoints to primary thread/active turn#106juliusmarminge merged 4 commits intomainfrom
juliusmarminge merged 4 commits intomainfrom
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.githubusercontent.com/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.githubusercontent.com/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.githubusercontent.com/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)
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