| author | |
| committer | |
| log | 96d610b1f82255acd0fc077917dde7a944033f23 |
| tree | eb19bb838d7f475b37dea15d35ab8b6b5ad2a460 |
| parent | 0c4c027e92bae27716d94d14e3531c390d9ecef1 |
5 files changed, 154 insertions(+), 6 deletions(-)
.plans/16c-pr89-remediation-checklist.md+6-6| ... | @@ -53,8 +53,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` | ... | @@ -53,8 +53,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` |
| 53 | - Threads: PRRT_kwDORLtfbc5wkPaL | 53 | - Threads: PRRT_kwDORLtfbc5wkPaL |
| 54 | - Audit note: SessionId-only routing can misassociate events under races/rebinds. | 54 | - Audit note: SessionId-only routing can misassociate events under races/rebinds. |
| 55 | 55 | ||
| 56 | - [ ] `C020` On `message.completed`, the message ID is added to the set and `thread.message.assistant.complete` is dispatched. On `turn.completed`, the same set is iterated and `thread.message.assistant.complete` is dispatched again for each ID—including already-completed ones. Consider removing message IDs from the set after dispatching on `message.completed`, or filtering out already-completed IDs before the `turn.completed` loop. | 56 | - [x] `C020` On `message.completed`, the message ID is added to the set and `thread.message.assistant.complete` is dispatched. On `turn.completed`, the same set is iterated and `thread.message.assistant.complete` is dispatched again for each ID—including already-completed ones. Consider removing message IDs from the set after dispatching on `message.completed`, or filtering out already-completed IDs before the `turn.completed` loop. |
| 57 | - Status: `TODO` | 57 | - Status: `DONE` |
| 58 | - Verdict: `partially-valid` | 58 | - Verdict: `partially-valid` |
| 59 | - Severity: `Medium` | 59 | - Severity: `Medium` |
| 60 | - Area: `Runtime resilience and failure handling` | 60 | - Area: `Runtime resilience and failure handling` |
| ... | @@ -80,8 +80,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` | ... | @@ -80,8 +80,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` |
| 80 | - Threads: PRRT_kwDORLtfbc5wyTaW, PRRT_kwDORLtfbc5wzli3 (+1 duplicate thread(s)) | 80 | - Threads: PRRT_kwDORLtfbc5wyTaW, PRRT_kwDORLtfbc5wzli3 (+1 duplicate thread(s)) |
| 81 | - Audit note: Same unhandled rejection path remains in WS message handler. | 81 | - Audit note: Same unhandled rejection path remains in WS message handler. |
| 82 | 82 | ||
| 83 | - [ ] `C042` Duplicated `resolveThreadWorkspaceCwd` across three files | 83 | - [x] `C042` Duplicated `resolveThreadWorkspaceCwd` across three files |
| 84 | - Status: `TODO` | 84 | - Status: `DONE` |
| 85 | - Verdict: `partially-valid` | 85 | - Verdict: `partially-valid` |
| 86 | - Severity: `Low` | 86 | - Severity: `Low` |
| 87 | - Area: `Runtime resilience and failure handling` | 87 | - Area: `Runtime resilience and failure handling` |
| ... | @@ -98,8 +98,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` | ... | @@ -98,8 +98,8 @@ Counts: active `53` (`valid=34`, `partially-valid=19`), closed-invalid `5` |
| 98 | - Threads: PRRT_kwDORLtfbc5wnWwM, PRRT_kwDORLtfbc5w1C3-, PRRT_kwDORLtfbc5w1HGZ (+2 duplicate thread(s)) | 98 | - Threads: PRRT_kwDORLtfbc5wnWwM, PRRT_kwDORLtfbc5w1C3-, PRRT_kwDORLtfbc5w1HGZ (+2 duplicate thread(s)) |
| 99 | - Audit note: Workspace CWD resolution duplication still present across modules. | 99 | - Audit note: Workspace CWD resolution duplication still present across modules. |
| 100 | 100 | ||
| 101 | - [ ] `C044` Checkpoint reactor swallows diff errors silently for `turn.completed` | 101 | - [x] `C044` Checkpoint reactor swallows diff errors silently for `turn.completed` |
| 102 | - Status: `TODO` | 102 | - Status: `DONE` |
| 103 | - Verdict: `partially-valid` | 103 | - Verdict: `partially-valid` |
| 104 | - Severity: `Low` | 104 | - Severity: `Low` |
| 105 | - Area: `Runtime resilience and failure handling` | 105 | - Area: `Runtime resilience and failure handling` |
apps/server/src/orchestration/Layers/CheckpointReactor.test.ts+48| ... | @@ -379,6 +379,54 @@ describe("CheckpointReactor", () => { | ... | @@ -379,6 +379,54 @@ describe("CheckpointReactor", () => { |
| 379 | ).toBe("v2\n"); | 379 | ).toBe("v2\n"); |
| 380 | }); | 380 | }); |
| 381 | 381 | ||
| 382 | it("appends capture failure activity when turn diff summary cannot be derived", async () => { | ||
| 383 | const harness = await createHarness({ seedFilesystemCheckpoints: false }); | ||
| 384 | const createdAt = new Date().toISOString(); | ||
| 385 | |||
| 386 | await Effect.runPromise( | ||
| 387 | harness.engine.dispatch({ | ||
| 388 | type: "thread.session.set", | ||
| 389 | commandId: CommandId.makeUnsafe("cmd-session-set-missing-baseline-diff"), | ||
| 390 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 391 | session: { | ||
| 392 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 393 | status: "ready", | ||
| 394 | providerName: "codex", | ||
| 395 | providerSessionId: asSessionId("sess-1"), | ||
| 396 | providerThreadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 397 | activeTurnId: null, | ||
| 398 | lastError: null, | ||
| 399 | updatedAt: createdAt, | ||
| 400 | }, | ||
| 401 | createdAt, | ||
| 402 | }), | ||
| 403 | ); | ||
| 404 | |||
| 405 | harness.provider.emit({ | ||
| 406 | type: "turn.completed", | ||
| 407 | eventId: EventId.makeUnsafe("evt-turn-completed-missing-baseline"), | ||
| 408 | provider: "codex", | ||
| 409 | sessionId: asSessionId("sess-1"), | ||
| 410 | createdAt: new Date().toISOString(), | ||
| 411 | threadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 412 | turnId: asTurnId("turn-missing-baseline"), | ||
| 413 | status: "completed", | ||
| 414 | }); | ||
| 415 | |||
| 416 | await waitForEvent(harness.engine, (event) => event.type === "thread.turn-diff-completed"); | ||
| 417 | const thread = await waitForThread( | ||
| 418 | harness.engine, | ||
| 419 | (entry) => | ||
| 420 | entry.checkpoints.length === 1 && | ||
| 421 | entry.activities.some((activity) => activity.kind === "checkpoint.capture.failed"), | ||
| 422 | ); | ||
| 423 | |||
| 424 | expect(thread.checkpoints[0]?.checkpointTurnCount).toBe(1); | ||
| 425 | expect( | ||
| 426 | thread.activities.some((activity) => activity.kind === "checkpoint.capture.failed"), | ||
| 427 | ).toBe(true); | ||
| 428 | }); | ||
| 429 | |||
| 382 | it("captures pre-turn baseline from project workspace root when thread worktree is unset", async () => { | 430 | it("captures pre-turn baseline from project workspace root when thread worktree is unset", async () => { |
| 383 | const harness = await createHarness({ | 431 | const harness = await createHarness({ |
| 384 | hasSession: false, | 432 | hasSession: false, |
apps/server/src/orchestration/Layers/CheckpointReactor.ts+8| ... | @@ -244,6 +244,14 @@ const make = Effect.gen(function* () { | ... | @@ -244,6 +244,14 @@ const make = Effect.gen(function* () { |
| 244 | deletions: file.deletions, | 244 | deletions: file.deletions, |
| 245 | })), | 245 | })), |
| 246 | ), | 246 | ), |
| 247 | Effect.tapError((error) => | ||
| 248 | appendCaptureFailureActivity({ | ||
| 249 | sessionId: event.sessionId, | ||
| 250 | turnId, | ||
| 251 | detail: `Checkpoint captured, but turn diff summary is unavailable: ${error.message}`, | ||
| 252 | createdAt: event.createdAt, | ||
| 253 | }), | ||
| 254 | ), | ||
| 247 | Effect.catch((error) => | 255 | Effect.catch((error) => |
| 248 | Effect.logWarning("failed to derive checkpoint file summary", { | 256 | Effect.logWarning("failed to derive checkpoint file summary", { |
| 249 | threadId: thread.id, | 257 | threadId: thread.id, |
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts+72| ... | @@ -258,6 +258,78 @@ describe("ProviderRuntimeIngestion", () => { | ... | @@ -258,6 +258,78 @@ describe("ProviderRuntimeIngestion", () => { |
| 258 | expect(message?.streaming).toBe(false); | 258 | expect(message?.streaming).toBe(false); |
| 259 | }); | 259 | }); |
| 260 | 260 | ||
| 261 | it("does not duplicate assistant completion when message.completed is followed by turn.completed", async () => { | ||
| 262 | const harness = await createHarness(); | ||
| 263 | const now = new Date().toISOString(); | ||
| 264 | |||
| 265 | harness.emit({ | ||
| 266 | type: "turn.started", | ||
| 267 | eventId: asEventId("evt-turn-started-for-complete-dedup"), | ||
| 268 | provider: "codex", | ||
| 269 | sessionId: asSessionId("sess-1"), | ||
| 270 | createdAt: now, | ||
| 271 | turnId: asProviderTurnId("turn-complete-dedup"), | ||
| 272 | }); | ||
| 273 | |||
| 274 | await waitForThread( | ||
| 275 | harness.engine, | ||
| 276 | (thread) => | ||
| 277 | thread.session?.status === "running" && thread.session?.activeTurnId === "turn-complete-dedup", | ||
| 278 | ); | ||
| 279 | |||
| 280 | harness.emit({ | ||
| 281 | type: "message.delta", | ||
| 282 | eventId: asEventId("evt-message-delta-for-complete-dedup"), | ||
| 283 | provider: "codex", | ||
| 284 | sessionId: asSessionId("sess-1"), | ||
| 285 | createdAt: now, | ||
| 286 | turnId: asProviderTurnId("turn-complete-dedup"), | ||
| 287 | itemId: asItemId("item-complete-dedup"), | ||
| 288 | delta: "done", | ||
| 289 | }); | ||
| 290 | harness.emit({ | ||
| 291 | type: "message.completed", | ||
| 292 | eventId: asEventId("evt-message-completed-for-complete-dedup"), | ||
| 293 | provider: "codex", | ||
| 294 | sessionId: asSessionId("sess-1"), | ||
| 295 | createdAt: now, | ||
| 296 | turnId: asProviderTurnId("turn-complete-dedup"), | ||
| 297 | itemId: asItemId("item-complete-dedup"), | ||
| 298 | }); | ||
| 299 | harness.emit({ | ||
| 300 | type: "turn.completed", | ||
| 301 | eventId: asEventId("evt-turn-completed-for-complete-dedup"), | ||
| 302 | provider: "codex", | ||
| 303 | sessionId: asSessionId("sess-1"), | ||
| 304 | createdAt: now, | ||
| 305 | turnId: asProviderTurnId("turn-complete-dedup"), | ||
| 306 | status: "completed", | ||
| 307 | }); | ||
| 308 | |||
| 309 | await waitForThread( | ||
| 310 | harness.engine, | ||
| 311 | (thread) => | ||
| 312 | thread.session?.status === "ready" && | ||
| 313 | thread.session?.activeTurnId === null && | ||
| 314 | thread.messages.some( | ||
| 315 | (message) => message.id === "assistant:item-complete-dedup" && !message.streaming, | ||
| 316 | ), | ||
| 317 | ); | ||
| 318 | |||
| 319 | const events = await Effect.runPromise( | ||
| 320 | Stream.runCollect(harness.engine.readEvents(0)).pipe(Effect.map((chunk) => Array.from(chunk))), | ||
| 321 | ); | ||
| 322 | const completionEvents = events.filter((event) => { | ||
| 323 | if (event.type !== "thread.message-sent") { | ||
| 324 | return false; | ||
| 325 | } | ||
| 326 | return ( | ||
| 327 | event.payload.messageId === "assistant:item-complete-dedup" && event.payload.streaming === false | ||
| 328 | ); | ||
| 329 | }); | ||
| 330 | expect(completionEvents).toHaveLength(1); | ||
| 331 | }); | ||
| 332 | |||
| 261 | it("maps runtime.error into errored session state", async () => { | 333 | it("maps runtime.error into errored session state", async () => { |
| 262 | const harness = await createHarness(); | 334 | const harness = await createHarness(); |
| 263 | const now = new Date().toISOString(); | 335 | const now = new Date().toISOString(); |
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts+20| ... | @@ -145,6 +145,22 @@ const make = Effect.gen(function* () { | ... | @@ -145,6 +145,22 @@ const make = Effect.gen(function* () { |
| 145 | } | 145 | } |
| 146 | }; | 146 | }; |
| 147 | 147 | ||
| 148 | const forgetAssistantMessageId = ( | ||
| 149 | sessionId: ProviderSessionId, | ||
| 150 | turnId: TurnId, | ||
| 151 | messageId: MessageId, | ||
| 152 | ) => { | ||
| 153 | const key = providerTurnKey(sessionId, turnId); | ||
| 154 | const existingIds = turnMessageIdsByTurnKey.get(key); | ||
| 155 | if (!existingIds) { | ||
| 156 | return; | ||
| 157 | } | ||
| 158 | existingIds.delete(messageId); | ||
| 159 | if (existingIds.size === 0) { | ||
| 160 | turnMessageIdsByTurnKey.delete(key); | ||
| 161 | } | ||
| 162 | }; | ||
| 163 | |||
| 148 | const getAssistantMessageIdsForTurn = (sessionId: ProviderSessionId, turnId: TurnId) => { | 164 | const getAssistantMessageIdsForTurn = (sessionId: ProviderSessionId, turnId: TurnId) => { |
| 149 | return turnMessageIdsByTurnKey.get(providerTurnKey(sessionId, turnId)) ?? new Set<MessageId>(); | 165 | return turnMessageIdsByTurnKey.get(providerTurnKey(sessionId, turnId)) ?? new Set<MessageId>(); |
| 150 | }; | 166 | }; |
| ... | @@ -257,6 +273,10 @@ const make = Effect.gen(function* () { | ... | @@ -257,6 +273,10 @@ const make = Effect.gen(function* () { |
| 257 | ...(turnId ? { turnId } : {}), | 273 | ...(turnId ? { turnId } : {}), |
| 258 | createdAt: now, | 274 | createdAt: now, |
| 259 | }); | 275 | }); |
| 276 | |||
| 277 | if (turnId) { | ||
| 278 | forgetAssistantMessageId(event.sessionId, turnId, assistantMessageId); | ||
| 279 | } | ||
| 260 | } | 280 | } |
| 261 | 281 | ||
| 262 | if (event.type === "turn.completed") { | 282 | if (event.type === "turn.completed") { |