| author |  Julius Marminge <julius0216@outlook.com> 2026-02-25 19:44:48-08:00 |
| committer |  Julius Marminge <jmarminge@gmail.com> 2026-02-26 13:20:36-08:00 |
| log | 6954d1b91c34a437df4f8cfd5141d1a585524424 |
| tree | b73bf4929e6c40fb919f174581fddbf1e339fb4c |
| parent | 7e46da9f22828aad1707e4a3ecc83c2979d83177 |
phase 1
11 files changed, 1258 insertions(+), 148 deletions(-)
.plans/16-pr89-review-remediation-phases.md+1
| ... | @@ -10,6 +10,7 @@ | ... | @@ -10,6 +10,7 @@ |
| 10 | - Invalid/false-positive: 3 (excluded) | 10 | - Invalid/false-positive: 3 (excluded) |
| 11 | - Duplicate reposts: collapsed | 11 | - Duplicate reposts: collapsed |
| 12 | - Unique actionable findings after filtering: 58 | 12 | - Unique actionable findings after filtering: 58 |
| 13 | - Post-rewrite validity audit: 5 additional stale items marked invalid, leaving 53 actionable (`34 valid` + `19 partially-valid`) | ||
| 13 | 14 | ||
| 14 | ## Phase 0: Canonical Triage Lock | 15 | ## Phase 0: Canonical Triage Lock |
| 15 | 16 |
.plans/16a-pr89-phase0-canonical-triage.json+12-107
| ... | @@ -10,11 +10,11 @@ | ... | @@ -10,11 +10,11 @@ |
| 10 | "phase0_generated_at": "2026-02-26T02:49:21.091145Z", | 10 | "phase0_generated_at": "2026-02-26T02:49:21.091145Z", |
| 11 | "summary": { | 11 | "summary": { |
| 12 | "total_threads": 185, | 12 | "total_threads": 185, |
| 13 | "actionable_canonical": 58, | 13 | "actionable_canonical": 53, |
| 14 | "actionable_duplicates": 24, | 14 | "actionable_duplicates": 24, |
| 15 | "filtered_outdated": 94, | 15 | "filtered_outdated": 94, |
| 16 | "filtered_resolved": 6, | 16 | "filtered_resolved": 6, |
| 17 | "filtered_invalid": 3 | 17 | "filtered_invalid": 8 |
| 18 | }, | 18 | }, |
| 19 | "canonical_items": [ | 19 | "canonical_items": [ |
| 20 | { | 20 | { |
| ... | @@ -379,24 +379,6 @@ | ... | @@ -379,24 +379,6 @@ |
| 379 | "macroscopeapp" | 379 | "macroscopeapp" |
| 380 | ] | 380 | ] |
| 381 | }, | 381 | }, |
| 382 | { | ||
| 383 | "canonical_id": "C021", | ||
| 384 | "area": "Other", | ||
| 385 | "title": "Shared mutable default metadata object causes stale eventId", | ||
| 386 | "severity": "Medium", | ||
| 387 | "path": "apps/server/src/orchestration/decider.ts", | ||
| 388 | "line": 27, | ||
| 389 | "thread_ids": [ | ||
| 390 | "PRRT_kwDORLtfbc5wkPaA" | ||
| 391 | ], | ||
| 392 | "thread_indexes": [ | ||
| 393 | 128 | ||
| 394 | ], | ||
| 395 | "duplicate_count": 0, | ||
| 396 | "authors": [ | ||
| 397 | "cursor" | ||
| 398 | ] | ||
| 399 | }, | ||
| 400 | { | 382 | { |
| 401 | "canonical_id": "C022", | 383 | "canonical_id": "C022", |
| 402 | "area": "Edge-case parsing/platform behavior", | 384 | "area": "Edge-case parsing/platform behavior", |
| ... | @@ -451,24 +433,6 @@ | ... | @@ -451,24 +433,6 @@ |
| 451 | "macroscopeapp" | 433 | "macroscopeapp" |
| 452 | ] | 434 | ] |
| 453 | }, | 435 | }, |
| 454 | { | ||
| 455 | "canonical_id": "C025", | ||
| 456 | "area": "Checkpointing correctness", | ||
| 457 | "title": "Duplicated checkpoint ref computation across two files", | ||
| 458 | "severity": "Medium", | ||
| 459 | "path": "apps/server/src/wsServer.ts", | ||
| 460 | "line": 128, | ||
| 461 | "thread_ids": [ | ||
| 462 | "PRRT_kwDORLtfbc5wvwag" | ||
| 463 | ], | ||
| 464 | "thread_indexes": [ | ||
| 465 | 148 | ||
| 466 | ], | ||
| 467 | "duplicate_count": 0, | ||
| 468 | "authors": [ | ||
| 469 | "cursor" | ||
| 470 | ] | ||
| 471 | }, | ||
| 472 | { | 436 | { |
| 473 | "canonical_id": "C026", | 437 | "canonical_id": "C026", |
| 474 | "area": "WebSocket robustness", | 438 | "area": "WebSocket robustness", |
| ... | @@ -561,24 +525,6 @@ | ... | @@ -561,24 +525,6 @@ |
| 561 | "macroscopeapp" | 525 | "macroscopeapp" |
| 562 | ] | 526 | ] |
| 563 | }, | 527 | }, |
| 564 | { | ||
| 565 | "canonical_id": "C031", | ||
| 566 | "area": "Event ordering and state consistency", | ||
| 567 | "title": "Revert uses wrong turn count from positional inference", | ||
| 568 | "severity": "Medium", | ||
| 569 | "path": "apps/web/src/session-logic.ts", | ||
| 570 | "line": 127, | ||
| 571 | "thread_ids": [ | ||
| 572 | "PRRT_kwDORLtfbc5v9SCp" | ||
| 573 | ], | ||
| 574 | "thread_indexes": [ | ||
| 575 | 41 | ||
| 576 | ], | ||
| 577 | "duplicate_count": 0, | ||
| 578 | "authors": [ | ||
| 579 | "cursor" | ||
| 580 | ] | ||
| 581 | }, | ||
| 582 | { | 528 | { |
| 583 | "canonical_id": "C032", | 529 | "canonical_id": "C032", |
| 584 | "area": "Other", | 530 | "area": "Other", |
| ... | @@ -657,24 +603,6 @@ | ... | @@ -657,24 +603,6 @@ |
| 657 | "cursor" | 603 | "cursor" |
| 658 | ] | 604 | ] |
| 659 | }, | 605 | }, |
| 660 | { | ||
| 661 | "canonical_id": "C036", | ||
| 662 | "area": "Checkpointing correctness", | ||
| 663 | "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", | ||
| 664 | "severity": "Low", | ||
| 665 | "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", | ||
| 666 | "line": 284, | ||
| 667 | "thread_ids": [ | ||
| 668 | "PRRT_kwDORLtfbc5wiqFX" | ||
| 669 | ], | ||
| 670 | "thread_indexes": [ | ||
| 671 | 115 | ||
| 672 | ], | ||
| 673 | "duplicate_count": 0, | ||
| 674 | "authors": [ | ||
| 675 | "cursor" | ||
| 676 | ] | ||
| 677 | }, | ||
| 678 | { | 606 | { |
| 679 | "canonical_id": "C037", | 607 | "canonical_id": "C037", |
| 680 | "area": "Other", | 608 | "area": "Other", |
| ... | @@ -1003,24 +931,6 @@ | ... | @@ -1003,24 +931,6 @@ |
| 1003 | "macroscopeapp" | 931 | "macroscopeapp" |
| 1004 | ] | 932 | ] |
| 1005 | }, | 933 | }, |
| 1006 | { | ||
| 1007 | "canonical_id": "C055", | ||
| 1008 | "area": "Checkpointing correctness", | ||
| 1009 | "title": "Duplicate `checkpointRefForThreadTurn` function across files", | ||
| 1010 | "severity": "Low", | ||
| 1011 | "path": "apps/server/src/wsServer.ts", | ||
| 1012 | "line": 128, | ||
| 1013 | "thread_ids": [ | ||
| 1014 | "PRRT_kwDORLtfbc5wkPaG" | ||
| 1015 | ], | ||
| 1016 | "thread_indexes": [ | ||
| 1017 | 129 | ||
| 1018 | ], | ||
| 1019 | "duplicate_count": 0, | ||
| 1020 | "authors": [ | ||
| 1021 | "cursor" | ||
| 1022 | ] | ||
| 1023 | }, | ||
| 1024 | { | 934 | { |
| 1025 | "canonical_id": "C056", | 935 | "canonical_id": "C056", |
| 1026 | "area": "Other", | 936 | "area": "Other", |
| ... | @@ -1684,9 +1594,8 @@ | ... | @@ -1684,9 +1594,8 @@ |
| 1684 | "title": "Revert uses wrong turn count from positional inference", | 1594 | "title": "Revert uses wrong turn count from positional inference", |
| 1685 | "severity": "Medium", | 1595 | "severity": "Medium", |
| 1686 | "has_false_positive_signal": false, | 1596 | "has_false_positive_signal": false, |
| 1687 | "canonical_id": "C031", | 1597 | "phase0_status": "filtered_invalid", |
| 1688 | "phase0_status": "actionable_canonical", | 1598 | "phase0_reason": "Invalid after post-rewrite audit: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary." |
| 1689 | "phase0_reason": "Canonical actionable item." | ||
| 1690 | }, | 1599 | }, |
| 1691 | { | 1600 | { |
| 1692 | "index": 42, | 1601 | "index": 42, |
| ... | @@ -2740,9 +2649,8 @@ | ... | @@ -2740,9 +2649,8 @@ |
| 2740 | "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", | 2649 | "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", |
| 2741 | "severity": "Low", | 2650 | "severity": "Low", |
| 2742 | "has_false_positive_signal": false, | 2651 | "has_false_positive_signal": false, |
| 2743 | "canonical_id": "C036", | 2652 | "phase0_status": "filtered_invalid", |
| 2744 | "phase0_status": "actionable_canonical", | 2653 | "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; single production source via Refs.ts." |
| 2745 | "phase0_reason": "Canonical actionable item." | ||
| 2746 | }, | 2654 | }, |
| 2747 | { | 2655 | { |
| 2748 | "index": 116, | 2656 | "index": 116, |
| ... | @@ -2929,9 +2837,8 @@ | ... | @@ -2929,9 +2837,8 @@ |
| 2929 | "title": "Shared mutable default metadata object causes stale eventId", | 2837 | "title": "Shared mutable default metadata object causes stale eventId", |
| 2930 | "severity": "Medium", | 2838 | "severity": "Medium", |
| 2931 | "has_false_positive_signal": false, | 2839 | "has_false_positive_signal": false, |
| 2932 | "canonical_id": "C021", | 2840 | "phase0_status": "filtered_invalid", |
| 2933 | "phase0_status": "actionable_canonical", | 2841 | "phase0_reason": "Invalid after post-rewrite audit: Stale-eventId claim no longer applies; eventId is regenerated per event." |
| 2934 | "phase0_reason": "Canonical actionable item." | ||
| 2935 | }, | 2842 | }, |
| 2936 | { | 2843 | { |
| 2937 | "index": 129, | 2844 | "index": 129, |
| ... | @@ -2944,9 +2851,8 @@ | ... | @@ -2944,9 +2851,8 @@ |
| 2944 | "title": "Duplicate `checkpointRefForThreadTurn` function across files", | 2851 | "title": "Duplicate `checkpointRefForThreadTurn` function across files", |
| 2945 | "severity": "Low", | 2852 | "severity": "Low", |
| 2946 | "has_false_positive_signal": false, | 2853 | "has_false_positive_signal": false, |
| 2947 | "canonical_id": "C055", | 2854 | "phase0_status": "filtered_invalid", |
| 2948 | "phase0_status": "actionable_canonical", | 2855 | "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; helper is centralized." |
| 2949 | "phase0_reason": "Canonical actionable item." | ||
| 2950 | }, | 2856 | }, |
| 2951 | { | 2857 | { |
| 2952 | "index": 130, | 2858 | "index": 130, |
| ... | @@ -3226,9 +3132,8 @@ | ... | @@ -3226,9 +3132,8 @@ |
| 3226 | "title": "Duplicated checkpoint ref computation across two files", | 3132 | "title": "Duplicated checkpoint ref computation across two files", |
| 3227 | "severity": "Medium", | 3133 | "severity": "Medium", |
| 3228 | "has_false_positive_signal": false, | 3134 | "has_false_positive_signal": false, |
| 3229 | "canonical_id": "C025", | 3135 | "phase0_status": "filtered_invalid", |
| 3230 | "phase0_status": "actionable_canonical", | 3136 | "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; checkpoint ref helper now centralized." |
| 3231 | "phase0_reason": "Canonical actionable item." | ||
| 3232 | }, | 3137 | }, |
| 3233 | { | 3138 | { |
| 3234 | "index": 149, | 3139 | "index": 149, |
.plans/16a-pr89-phase0-canonical-triage.md+31-31
| ... | @@ -7,20 +7,18 @@ | ... | @@ -7,20 +7,18 @@ |
| 7 | ## Summary | 7 | ## Summary |
| 8 | 8 | ||
| 9 | - Total threads: 185 | 9 | - Total threads: 185 |
| 10 | - Actionable canonical: 58 | 10 | - Actionable canonical: 53 |
| 11 | - Actionable duplicates: 24 | 11 | - Actionable duplicates: 24 |
| 12 | - Filtered outdated: 94 | 12 | - Filtered outdated: 94 |
| 13 | - Filtered resolved: 6 | 13 | - Filtered resolved: 6 |
| 14 | - Filtered invalid: 3 | 14 | - Filtered invalid: 8 |
| 15 | 15 | ||
| 16 | ## Canonical Actionable Checklist | 16 | ## Canonical Actionable Checklist |
| 17 | 17 | ||
| 18 | ### Other (17) | 18 | ### Other (16) |
| 19 | 19 | ||
| 20 | - [ ] C011 [Medium] Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead. (apps/server/src/orchestration/Errors.ts:117) | 20 | - [ ] C011 [Medium] Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead. (apps/server/src/orchestration/Errors.ts:117) |
| 21 | - Threads: PRRT_kwDORLtfbc5wBFv4 | 21 | - Threads: PRRT_kwDORLtfbc5wBFv4 |
| 22 | - [ ] C021 [Medium] Shared mutable default metadata object causes stale eventId (apps/server/src/orchestration/decider.ts:27) | ||
| 23 | - Threads: PRRT_kwDORLtfbc5wkPaA | ||
| 24 | - [ ] C024 [Medium] In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release. (apps/server/src/persistence/NodeSqliteClient.ts:145) | 22 | - [ ] C024 [Medium] In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release. (apps/server/src/persistence/NodeSqliteClient.ts:145) |
| 25 | - Threads: PRRT_kwDORLtfbc5whxX9 | 23 | - Threads: PRRT_kwDORLtfbc5whxX9 |
| 26 | - [ ] C028 [Medium] Branch sync dispatches both server and stale local update (apps/web/src/components/BranchToolbar.tsx:102) | 24 | - [ ] C028 [Medium] Branch sync dispatches both server and stale local update (apps/web/src/components/BranchToolbar.tsx:102) |
| ... | @@ -54,15 +52,15 @@ | ... | @@ -54,15 +52,15 @@ |
| 54 | 52 | ||
| 55 | ### Runtime resilience and failure handling (10) | 53 | ### Runtime resilience and failure handling (10) |
| 56 | 54 | ||
| 57 | - [ ] C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) (+5 dup) | 55 | - [x] C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) (+5 dup) |
| 58 | - Threads: PRRT_kwDORLtfbc5wj4cH, PRRT_kwDORLtfbc5wnWwF, PRRT_kwDORLtfbc5wyTaP, PRRT_kwDORLtfbc5wzliw, PRRT_kwDORLtfbc5w0_g3, PRRT_kwDORLtfbc5w1HGT | 56 | - Threads: PRRT_kwDORLtfbc5wj4cH, PRRT_kwDORLtfbc5wnWwF, PRRT_kwDORLtfbc5wyTaP, PRRT_kwDORLtfbc5wzliw, PRRT_kwDORLtfbc5w0_g3, PRRT_kwDORLtfbc5w1HGT |
| 59 | - [ ] C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) (+4 dup) | 57 | - [x] C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) (+4 dup) |
| 60 | - Threads: PRRT_kwDORLtfbc5whszW, PRRT_kwDORLtfbc5wyTaS, PRRT_kwDORLtfbc5wzli0, PRRT_kwDORLtfbc5w0_g4, PRRT_kwDORLtfbc5w1HGX | 58 | - Threads: PRRT_kwDORLtfbc5whszW, PRRT_kwDORLtfbc5wyTaS, PRRT_kwDORLtfbc5wzli0, PRRT_kwDORLtfbc5w0_g4, PRRT_kwDORLtfbc5w1HGX |
| 61 | - [ ] C019 [Medium] ProviderRuntimeIngestion processes events for wrong thread on race (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:178) | 59 | - [ ] C019 [Medium] ProviderRuntimeIngestion processes events for wrong thread on race (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:178) |
| 62 | - Threads: PRRT_kwDORLtfbc5wkPaL | 60 | - Threads: PRRT_kwDORLtfbc5wkPaL |
| 63 | - [ ] C020 [Medium] 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. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:266) | 61 | - [ ] C020 [Medium] 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. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:266) |
| 64 | - Threads: PRRT_kwDORLtfbc5w1GPr | 62 | - Threads: PRRT_kwDORLtfbc5w1GPr |
| 65 | - [ ] C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) (+1 dup) | 63 | - [x] C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) (+1 dup) |
| 66 | - Threads: PRRT_kwDORLtfbc5wyTaW, PRRT_kwDORLtfbc5wzli3 | 64 | - Threads: PRRT_kwDORLtfbc5wyTaW, PRRT_kwDORLtfbc5wzli3 |
| 67 | - [ ] C042 [Low] Duplicated `resolveThreadWorkspaceCwd` across three files (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) | 65 | - [ ] C042 [Low] Duplicated `resolveThreadWorkspaceCwd` across three files (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) |
| 68 | - Threads: PRRT_kwDORLtfbc5wzli2 | 66 | - Threads: PRRT_kwDORLtfbc5wzli2 |
| ... | @@ -96,26 +94,7 @@ | ... | @@ -96,26 +94,7 @@ |
| 96 | - [ ] C040 [Low] `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/keybindings.ts:488) | 94 | - [ ] C040 [Low] `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/keybindings.ts:488) |
| 97 | - Threads: PRRT_kwDORLtfbc5wxvIA | 95 | - Threads: PRRT_kwDORLtfbc5wxvIA |
| 98 | 96 | ||
| 99 | ### Checkpointing correctness (8) | 97 | ### Event ordering and state consistency (5) |
| 100 | |||
| 101 | - [ ] C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) (+3 dup) | ||
| 102 | - Threads: PRRT_kwDORLtfbc5widJw, PRRT_kwDORLtfbc5wnWv_, PRRT_kwDORLtfbc5w0_g7, PRRT_kwDORLtfbc5w1C36 | ||
| 103 | - [ ] C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) | ||
| 104 | - Threads: PRRT_kwDORLtfbc5wiLhQ | ||
| 105 | - [ ] C025 [Medium] Duplicated checkpoint ref computation across two files (apps/server/src/wsServer.ts:128) | ||
| 106 | - Threads: PRRT_kwDORLtfbc5wvwag | ||
| 107 | - [ ] C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) | ||
| 108 | - Threads: PRRT_kwDORLtfbc5wlYgo | ||
| 109 | - [ ] C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) | ||
| 110 | - Threads: PRRT_kwDORLtfbc5wj5fn | ||
| 111 | - [ ] C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) (+2 dup) | ||
| 112 | - Threads: PRRT_kwDORLtfbc5wlYgs, PRRT_kwDORLtfbc5wxsO6, PRRT_kwDORLtfbc5w1C4B | ||
| 113 | - [ ] C036 [Low] Duplicate `checkpointRefForThreadTurn` function in two production files (apps/server/src/checkpointing/Layers/CheckpointStore.ts:284) | ||
| 114 | - Threads: PRRT_kwDORLtfbc5wiqFX | ||
| 115 | - [ ] C055 [Low] Duplicate `checkpointRefForThreadTurn` function across files (apps/server/src/wsServer.ts:128) | ||
| 116 | - Threads: PRRT_kwDORLtfbc5wkPaG | ||
| 117 | |||
| 118 | ### Event ordering and state consistency (6) | ||
| 119 | 98 | ||
| 120 | - [ ] C001 [High] Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:96) | 99 | - [ ] C001 [High] Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:96) |
| 121 | - Threads: PRRT_kwDORLtfbc5wzp4T | 100 | - Threads: PRRT_kwDORLtfbc5wzp4T |
| ... | @@ -127,14 +106,12 @@ | ... | @@ -127,14 +106,12 @@ |
| 127 | - Threads: PRRT_kwDORLtfbc5whxJO | 106 | - Threads: PRRT_kwDORLtfbc5whxJO |
| 128 | - [ ] C016 [Medium] The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:490) | 107 | - [ ] C016 [Medium] The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:490) |
| 129 | - Threads: PRRT_kwDORLtfbc5wxvH8 | 108 | - Threads: PRRT_kwDORLtfbc5wxvH8 |
| 130 | - [ ] C031 [Medium] Revert uses wrong turn count from positional inference (apps/web/src/session-logic.ts:127) | ||
| 131 | - Threads: PRRT_kwDORLtfbc5v9SCp | ||
| 132 | 109 | ||
| 133 | ### WebSocket robustness (5) | 110 | ### WebSocket robustness (5) |
| 134 | 111 | ||
| 135 | - [ ] C003 [High] Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake. (apps/server/src/wsServer.ts:75) | 112 | - [ ] C003 [High] Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake. (apps/server/src/wsServer.ts:75) |
| 136 | - Threads: PRRT_kwDORLtfbc5v-cf4 | 113 | - Threads: PRRT_kwDORLtfbc5v-cf4 |
| 137 | - [ ] C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) | 114 | - [x] C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) |
| 138 | - Threads: PRRT_kwDORLtfbc5wj4cE | 115 | - Threads: PRRT_kwDORLtfbc5wj4cE |
| 139 | - [ ] C053 [Low] Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection. (apps/server/src/wsServer.ts:83) | 116 | - [ ] C053 [Low] Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection. (apps/server/src/wsServer.ts:83) |
| 140 | - Threads: PRRT_kwDORLtfbc5v-WPD | 117 | - Threads: PRRT_kwDORLtfbc5v-WPD |
| ... | @@ -143,6 +120,19 @@ | ... | @@ -143,6 +120,19 @@ |
| 143 | - [ ] C059 [Low] Suggestion: don’t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object. (apps/web/src/wsTransport.ts:59) | 120 | - [ ] C059 [Low] Suggestion: don’t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object. (apps/web/src/wsTransport.ts:59) |
| 144 | - Threads: PRRT_kwDORLtfbc5whtrN | 121 | - Threads: PRRT_kwDORLtfbc5whtrN |
| 145 | 122 | ||
| 123 | ### Checkpointing correctness (5) | ||
| 124 | |||
| 125 | - [ ] C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) (+3 dup) | ||
| 126 | - Threads: PRRT_kwDORLtfbc5widJw, PRRT_kwDORLtfbc5wnWv_, PRRT_kwDORLtfbc5w0_g7, PRRT_kwDORLtfbc5w1C36 | ||
| 127 | - [ ] C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) | ||
| 128 | - Threads: PRRT_kwDORLtfbc5wiLhQ | ||
| 129 | - [ ] C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) | ||
| 130 | - Threads: PRRT_kwDORLtfbc5wlYgo | ||
| 131 | - [ ] C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) | ||
| 132 | - Threads: PRRT_kwDORLtfbc5wj5fn | ||
| 133 | - [ ] C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) (+2 dup) | ||
| 134 | - Threads: PRRT_kwDORLtfbc5wlYgs, PRRT_kwDORLtfbc5wxsO6, PRRT_kwDORLtfbc5w1C4B | ||
| 135 | |||
| 146 | ### Build/runtime portability (2) | 136 | ### Build/runtime portability (2) |
| 147 | 137 | ||
| 148 | - [ ] C006 [Medium] Build script runs TypeScript file with bare node (apps/server/package.json:14) | 138 | - [ ] C006 [Medium] Build script runs TypeScript file with bare node (apps/server/package.json:14) |
| ... | @@ -161,8 +151,18 @@ | ... | @@ -161,8 +151,18 @@ |
| 161 | - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. | 151 | - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. |
| 162 | - PRRT_kwDORLtfbc5v9An- | apps/web/src/wsNativeApi.ts:165 | push stream, and the callbacks rely on unchecked casts. Please reintroduce a `providers.onEvent` (or equivalent) that subscribes to the `providers.event` channel and decode read-model/domain-event payloads using the shared contracts schemas before invoking callbacks. As per coding guidelines: "Web app must consume provider event streams via WebSocket push on channel `providers.event`" and "Use Zod schemas from `packages/contracts` for shared type contracts covering provider events, WebSocket protocol, and model/session types". <details> <summary>🤖 Prompt for AI Agents</summary> | 152 | - PRRT_kwDORLtfbc5v9An- | apps/web/src/wsNativeApi.ts:165 | push stream, and the callbacks rely on unchecked casts. Please reintroduce a `providers.onEvent` (or equivalent) that subscribes to the `providers.event` channel and decode read-model/domain-event payloads using the shared contracts schemas before invoking callbacks. As per coding guidelines: "Web app must consume provider event streams via WebSocket push on channel `providers.event`" and "Use Zod schemas from `packages/contracts` for shared type contracts covering provider events, WebSocket protocol, and model/session types". <details> <summary>🤖 Prompt for AI Agents</summary> |
| 163 | - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. | 153 | - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. |
| 154 | - PRRT_kwDORLtfbc5v9SCp | apps/web/src/session-logic.ts:127 | Revert uses wrong turn count from positional inference | ||
| 155 | - Rationale: Invalid after post-rewrite audit: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary. | ||
| 164 | - PRRT_kwDORLtfbc5widJx | apps/server/package.json:14 | Build script requires implicit Node.js TypeScript support | 156 | - PRRT_kwDORLtfbc5widJx | apps/server/package.json:14 | Build script requires implicit Node.js TypeScript support |
| 165 | - Rationale: Thread contains explicit false-positive determination in replies. | 157 | - Rationale: Thread contains explicit false-positive determination in replies. |
| 158 | - PRRT_kwDORLtfbc5wiqFX | apps/server/src/checkpointing/Layers/CheckpointStore.ts:284 | Duplicate `checkpointRefForThreadTurn` function in two production files | ||
| 159 | - Rationale: Invalid after post-rewrite audit: No longer duplicated; single production source via Refs.ts. | ||
| 160 | - PRRT_kwDORLtfbc5wkPaA | apps/server/src/orchestration/decider.ts:27 | Shared mutable default metadata object causes stale eventId | ||
| 161 | - Rationale: Invalid after post-rewrite audit: Stale-eventId claim no longer applies; eventId is regenerated per event. | ||
| 162 | - PRRT_kwDORLtfbc5wkPaG | apps/server/src/wsServer.ts:128 | Duplicate `checkpointRefForThreadTurn` function across files | ||
| 163 | - Rationale: Invalid after post-rewrite audit: No longer duplicated; helper is centralized. | ||
| 164 | - PRRT_kwDORLtfbc5wvwag | apps/server/src/wsServer.ts:128 | Duplicated checkpoint ref computation across two files | ||
| 165 | - Rationale: Invalid after post-rewrite audit: No longer duplicated; checkpoint ref helper now centralized. | ||
| 166 | 166 | ||
| 167 | ## Duplicate Mapping | 167 | ## Duplicate Mapping |
| 168 | 168 |
.plans/16b-pr89-architecture-validity-audit.json created+790
| ... | @@ -0,0 +1,790 @@ | ||
| 1 | { | ||
| 2 | "source": ".plans/16a-pr89-phase0-canonical-triage.json", | ||
| 3 | "architecture_assumption": "Provider runtime activity is projected and consumed via orchestration.domainEvent; no separate providers.event push channel is expected.", | ||
| 4 | "summary": { | ||
| 5 | "total_canonical_reviewed": 58, | ||
| 6 | "valid": 34, | ||
| 7 | "partially_valid": 19, | ||
| 8 | "invalid": 5 | ||
| 9 | }, | ||
| 10 | "records": [ | ||
| 11 | { | ||
| 12 | "canonical_id": "C001", | ||
| 13 | "verdict": "valid", | ||
| 14 | "rationale": "Append/project/receipt are non-atomic; retry can duplicate events.", | ||
| 15 | "severity": "High", | ||
| 16 | "area": "Event ordering and state consistency", | ||
| 17 | "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", | ||
| 18 | "line": 96, | ||
| 19 | "title": "Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 20 | "thread_ids": [ | ||
| 21 | "PRRT_kwDORLtfbc5wzp4T" | ||
| 22 | ] | ||
| 23 | }, | ||
| 24 | { | ||
| 25 | "canonical_id": "C002", | ||
| 26 | "verdict": "valid", | ||
| 27 | "rationale": "Ingestion worker loop can terminate on unhandled processEvent failure.", | ||
| 28 | "severity": "High", | ||
| 29 | "area": "Runtime resilience and failure handling", | ||
| 30 | "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", | ||
| 31 | "line": 333, | ||
| 32 | "title": "A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber.", | ||
| 33 | "thread_ids": [ | ||
| 34 | "PRRT_kwDORLtfbc5wj4cH", | ||
| 35 | "PRRT_kwDORLtfbc5wnWwF", | ||
| 36 | "PRRT_kwDORLtfbc5wyTaP", | ||
| 37 | "PRRT_kwDORLtfbc5wzliw", | ||
| 38 | "PRRT_kwDORLtfbc5w0_g3", | ||
| 39 | "PRRT_kwDORLtfbc5w1HGT" | ||
| 40 | ] | ||
| 41 | }, | ||
| 42 | { | ||
| 43 | "canonical_id": "C003", | ||
| 44 | "verdict": "valid", | ||
| 45 | "rationale": "Upgrade reject writes then destroys socket without defensive error listener.", | ||
| 46 | "severity": "High", | ||
| 47 | "area": "WebSocket robustness", | ||
| 48 | "path": "apps/server/src/wsServer.ts", | ||
| 49 | "line": 75, | ||
| 50 | "title": "Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake.", | ||
| 51 | "thread_ids": [ | ||
| 52 | "PRRT_kwDORLtfbc5v-cf4" | ||
| 53 | ] | ||
| 54 | }, | ||
| 55 | { | ||
| 56 | "canonical_id": "C006", | ||
| 57 | "verdict": "partially-valid", | ||
| 58 | "rationale": "Node 24 makes this work, but script still relies on runtime-specific TS execution.", | ||
| 59 | "severity": "Medium", | ||
| 60 | "area": "Build/runtime portability", | ||
| 61 | "path": "apps/server/package.json", | ||
| 62 | "line": 14, | ||
| 63 | "title": "Build script runs TypeScript file with bare node", | ||
| 64 | "thread_ids": [ | ||
| 65 | "PRRT_kwDORLtfbc5wxsO-" | ||
| 66 | ] | ||
| 67 | }, | ||
| 68 | { | ||
| 69 | "canonical_id": "C007", | ||
| 70 | "verdict": "partially-valid", | ||
| 71 | "rationale": "Same as C006 (duplicate concern).", | ||
| 72 | "severity": "Medium", | ||
| 73 | "area": "Build/runtime portability", | ||
| 74 | "path": "apps/server/package.json", | ||
| 75 | "line": 14, | ||
| 76 | "title": "Build script uses `node` to run `.ts` file directly", | ||
| 77 | "thread_ids": [ | ||
| 78 | "PRRT_kwDORLtfbc5wiLNS", | ||
| 79 | "PRRT_kwDORLtfbc5wj5fl" | ||
| 80 | ] | ||
| 81 | }, | ||
| 82 | { | ||
| 83 | "canonical_id": "C008", | ||
| 84 | "verdict": "partially-valid", | ||
| 85 | "rationale": "Normalization inconsistency remains, but upstream callsites now mostly canonicalize inputs.", | ||
| 86 | "severity": "Medium", | ||
| 87 | "area": "Checkpointing correctness", | ||
| 88 | "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", | ||
| 89 | "line": 94, | ||
| 90 | "title": "Inconsistent input normalization across CheckpointStore methods", | ||
| 91 | "thread_ids": [ | ||
| 92 | "PRRT_kwDORLtfbc5widJw", | ||
| 93 | "PRRT_kwDORLtfbc5wnWv_", | ||
| 94 | "PRRT_kwDORLtfbc5w0_g7", | ||
| 95 | "PRRT_kwDORLtfbc5w1C36" | ||
| 96 | ] | ||
| 97 | }, | ||
| 98 | { | ||
| 99 | "canonical_id": "C009", | ||
| 100 | "verdict": "valid", | ||
| 101 | "rationale": "Braced rename parsing still breaks paths like src/{old => new}/file.ts.", | ||
| 102 | "severity": "Medium", | ||
| 103 | "area": "Edge-case parsing/platform behavior", | ||
| 104 | "path": "apps/server/src/git/Layers/GitCore.ts", | ||
| 105 | "line": 41, | ||
| 106 | "title": "Git's braced rename syntax (e.g., `src/{old => new}/file.ts`) isn't handled correctly. The current slice after ` => ` produces invalid paths like `new}/file.ts`. Consider expanding the braces to construct the full destination path. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 107 | "thread_ids": [ | ||
| 108 | "PRRT_kwDORLtfbc5w1CxT" | ||
| 109 | ] | ||
| 110 | }, | ||
| 111 | { | ||
| 112 | "canonical_id": "C010", | ||
| 113 | "verdict": "valid", | ||
| 114 | "rationale": "ENOENT for missing keybindings config still not handled as empty/default.", | ||
| 115 | "severity": "Medium", | ||
| 116 | "area": "Edge-case parsing/platform behavior", | ||
| 117 | "path": "apps/server/src/keybindings.ts", | ||
| 118 | "line": 418, | ||
| 119 | "title": "`loadCustomKeybindingsConfig` fails when the config file doesn't exist, which is expected for new users. Consider catching `ENOENT` and returning an empty array instead. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 120 | "thread_ids": [ | ||
| 121 | "PRRT_kwDORLtfbc5wxvIJ" | ||
| 122 | ] | ||
| 123 | }, | ||
| 124 | { | ||
| 125 | "canonical_id": "C011", | ||
| 126 | "verdict": "valid", | ||
| 127 | "rationale": "Template-literal interpolation of Symbol still throws.", | ||
| 128 | "severity": "Medium", | ||
| 129 | "area": "Other", | ||
| 130 | "path": "apps/server/src/orchestration/Errors.ts", | ||
| 131 | "line": 117, | ||
| 132 | "title": "Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead.", | ||
| 133 | "thread_ids": [ | ||
| 134 | "PRRT_kwDORLtfbc5wBFv4" | ||
| 135 | ] | ||
| 136 | }, | ||
| 137 | { | ||
| 138 | "canonical_id": "C012", | ||
| 139 | "verdict": "valid", | ||
| 140 | "rationale": "Revert completion dispatch remains forked; state consistency window remains.", | ||
| 141 | "severity": "Medium", | ||
| 142 | "area": "Runtime resilience and failure handling", | ||
| 143 | "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", | ||
| 144 | "line": 542, | ||
| 145 | "title": "Forked revert dispatch risks read model inconsistency", | ||
| 146 | "thread_ids": [ | ||
| 147 | "PRRT_kwDORLtfbc5whszW", | ||
| 148 | "PRRT_kwDORLtfbc5wyTaS", | ||
| 149 | "PRRT_kwDORLtfbc5wzli0", | ||
| 150 | "PRRT_kwDORLtfbc5w0_g4", | ||
| 151 | "PRRT_kwDORLtfbc5w1HGX" | ||
| 152 | ] | ||
| 153 | }, | ||
| 154 | { | ||
| 155 | "canonical_id": "C013", | ||
| 156 | "verdict": "valid", | ||
| 157 | "rationale": "Persisted event can outpace in-memory projection on mid-flight failure.", | ||
| 158 | "severity": "Medium", | ||
| 159 | "area": "Event ordering and state consistency", | ||
| 160 | "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", | ||
| 161 | "line": 99, | ||
| 162 | "title": "If `projectionPipeline.projectEvent` fails after `eventStore.append` succeeds, the event is persisted but `readModel` isn't updated, causing desync. Consider updating the in-memory `readModel` immediately after append (before the external projection), so local state stays consistent regardless of downstream failures.", | ||
| 163 | "thread_ids": [ | ||
| 164 | "PRRT_kwDORLtfbc5whtrM" | ||
| 165 | ] | ||
| 166 | }, | ||
| 167 | { | ||
| 168 | "canonical_id": "C014", | ||
| 169 | "verdict": "partially-valid", | ||
| 170 | "rationale": "Broad catch may be intentional for liveness; correctness concern is contextual.", | ||
| 171 | "severity": "Medium", | ||
| 172 | "area": "Event ordering and state consistency", | ||
| 173 | "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", | ||
| 174 | "line": 144, | ||
| 175 | "title": "Engine error handler catches all errors including non-invariant ones", | ||
| 176 | "thread_ids": [ | ||
| 177 | "PRRT_kwDORLtfbc5wkPaJ" | ||
| 178 | ] | ||
| 179 | }, | ||
| 180 | { | ||
| 181 | "canonical_id": "C015", | ||
| 182 | "verdict": "partially-valid", | ||
| 183 | "rationale": "Message fallback retention issue is real, but prior FK-violation claim is overstated.", | ||
| 184 | "severity": "Medium", | ||
| 185 | "area": "Event ordering and state consistency", | ||
| 186 | "path": "apps/server/src/orchestration/Layers/ProjectionPipeline.ts", | ||
| 187 | "line": 99, | ||
| 188 | "title": "The gap-filling fallback logic can retain messages from turns that are about to be deleted, causing foreign key violations. Consider removing the fallback logic entirely, or filtering `fallbackUserMessages` and `fallbackAssistantMessages` to only include messages whose `turnId` is in `retainedTurnIds`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 189 | "thread_ids": [ | ||
| 190 | "PRRT_kwDORLtfbc5whxJO" | ||
| 191 | ] | ||
| 192 | }, | ||
| 193 | { | ||
| 194 | "canonical_id": "C016", | ||
| 195 | "verdict": "valid", | ||
| 196 | "rationale": "Pending turn-start map is in-memory only and not rebuilt on bootstrap.", | ||
| 197 | "severity": "Medium", | ||
| 198 | "area": "Event ordering and state consistency", | ||
| 199 | "path": "apps/server/src/orchestration/Layers/ProjectionPipeline.ts", | ||
| 200 | "line": 490, | ||
| 201 | "title": "The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 202 | "thread_ids": [ | ||
| 203 | "PRRT_kwDORLtfbc5wxvH8" | ||
| 204 | ] | ||
| 205 | }, | ||
| 206 | { | ||
| 207 | "canonical_id": "C017", | ||
| 208 | "verdict": "partially-valid", | ||
| 209 | "rationale": "Snapshot sequence can under-report due to extra projectors, but replay impact is lower now.", | ||
| 210 | "severity": "Medium", | ||
| 211 | "area": "Checkpointing correctness", | ||
| 212 | "path": "apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts", | ||
| 213 | "line": 71, | ||
| 214 | "title": "`REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 215 | "thread_ids": [ | ||
| 216 | "PRRT_kwDORLtfbc5wiLhQ" | ||
| 217 | ] | ||
| 218 | }, | ||
| 219 | { | ||
| 220 | "canonical_id": "C018", | ||
| 221 | "verdict": "valid", | ||
| 222 | "rationale": "handledTurnStartKeys still grows without pruning.", | ||
| 223 | "severity": "Medium", | ||
| 224 | "area": "Memory/resource growth", | ||
| 225 | "path": "apps/server/src/orchestration/Layers/ProviderCommandReactor.ts", | ||
| 226 | "line": 84, | ||
| 227 | "title": "Unbounded memory growth in turn start deduplication set", | ||
| 228 | "thread_ids": [ | ||
| 229 | "PRRT_kwDORLtfbc5whszQ", | ||
| 230 | "PRRT_kwDORLtfbc5wl2A8", | ||
| 231 | "PRRT_kwDORLtfbc5wyTaT", | ||
| 232 | "PRRT_kwDORLtfbc5wzliz", | ||
| 233 | "PRRT_kwDORLtfbc5w0_g-", | ||
| 234 | "PRRT_kwDORLtfbc5w1HGW" | ||
| 235 | ] | ||
| 236 | }, | ||
| 237 | { | ||
| 238 | "canonical_id": "C019", | ||
| 239 | "verdict": "partially-valid", | ||
| 240 | "rationale": "SessionId-only routing can misassociate events under races/rebinds.", | ||
| 241 | "severity": "Medium", | ||
| 242 | "area": "Runtime resilience and failure handling", | ||
| 243 | "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", | ||
| 244 | "line": 178, | ||
| 245 | "title": "ProviderRuntimeIngestion processes events for wrong thread on race", | ||
| 246 | "thread_ids": [ | ||
| 247 | "PRRT_kwDORLtfbc5wkPaL" | ||
| 248 | ] | ||
| 249 | }, | ||
| 250 | { | ||
| 251 | "canonical_id": "C020", | ||
| 252 | "verdict": "partially-valid", | ||
| 253 | "rationale": "Duplicate complete dispatch exists; downstream impact often idempotent.", | ||
| 254 | "severity": "Medium", | ||
| 255 | "area": "Runtime resilience and failure handling", | ||
| 256 | "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", | ||
| 257 | "line": 266, | ||
| 258 | "title": "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\u2014including 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.", | ||
| 259 | "thread_ids": [ | ||
| 260 | "PRRT_kwDORLtfbc5w1GPr" | ||
| 261 | ] | ||
| 262 | }, | ||
| 263 | { | ||
| 264 | "canonical_id": "C021", | ||
| 265 | "verdict": "invalid", | ||
| 266 | "rationale": "Stale-eventId claim no longer applies; eventId is regenerated per event.", | ||
| 267 | "severity": "Medium", | ||
| 268 | "area": "Other", | ||
| 269 | "path": "apps/server/src/orchestration/decider.ts", | ||
| 270 | "line": 27, | ||
| 271 | "title": "Shared mutable default metadata object causes stale eventId", | ||
| 272 | "thread_ids": [ | ||
| 273 | "PRRT_kwDORLtfbc5wkPaA" | ||
| 274 | ] | ||
| 275 | }, | ||
| 276 | { | ||
| 277 | "canonical_id": "C022", | ||
| 278 | "verdict": "valid", | ||
| 279 | "rationale": "fish PATH formatting risk still exists in os-jank path recovery.", | ||
| 280 | "severity": "Medium", | ||
| 281 | "area": "Edge-case parsing/platform behavior", | ||
| 282 | "path": "apps/server/src/os-jank.ts", | ||
| 283 | "line": 10, | ||
| 284 | "title": "Fish shell outputs `$PATH` as space-separated, not colon-separated. Consider checking if the shell is fish and using `string join : $PATH` instead, or validating the result contains colons before assigning.", | ||
| 285 | "thread_ids": [ | ||
| 286 | "PRRT_kwDORLtfbc5wkRZM" | ||
| 287 | ] | ||
| 288 | }, | ||
| 289 | { | ||
| 290 | "canonical_id": "C023", | ||
| 291 | "verdict": "valid", | ||
| 292 | "rationale": "-ilc shell invocation can pollute captured PATH output.", | ||
| 293 | "severity": "Medium", | ||
| 294 | "area": "Edge-case parsing/platform behavior", | ||
| 295 | "path": "apps/server/src/os-jank.ts", | ||
| 296 | "line": 10, | ||
| 297 | "title": "Using `-il` flags causes the shell to source profile scripts that may print banners or other text, polluting the captured `PATH`. Consider using `-lc` (login only, non-interactive) to reduce unwanted output.", | ||
| 298 | "thread_ids": [ | ||
| 299 | "PRRT_kwDORLtfbc5wj4cM" | ||
| 300 | ] | ||
| 301 | }, | ||
| 302 | { | ||
| 303 | "canonical_id": "C024", | ||
| 304 | "verdict": "partially-valid", | ||
| 305 | "rationale": "prepare() still outside managed effect boundary; failure handling risk remains.", | ||
| 306 | "severity": "Medium", | ||
| 307 | "area": "Other", | ||
| 308 | "path": "apps/server/src/persistence/NodeSqliteClient.ts", | ||
| 309 | "line": 145, | ||
| 310 | "title": "In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release.", | ||
| 311 | "thread_ids": [ | ||
| 312 | "PRRT_kwDORLtfbc5whxX9" | ||
| 313 | ] | ||
| 314 | }, | ||
| 315 | { | ||
| 316 | "canonical_id": "C025", | ||
| 317 | "verdict": "invalid", | ||
| 318 | "rationale": "No longer duplicated; checkpoint ref helper now centralized.", | ||
| 319 | "severity": "Medium", | ||
| 320 | "area": "Checkpointing correctness", | ||
| 321 | "path": "apps/server/src/wsServer.ts", | ||
| 322 | "line": 128, | ||
| 323 | "title": "Duplicated checkpoint ref computation across two files", | ||
| 324 | "thread_ids": [ | ||
| 325 | "PRRT_kwDORLtfbc5wvwag" | ||
| 326 | ] | ||
| 327 | }, | ||
| 328 | { | ||
| 329 | "canonical_id": "C026", | ||
| 330 | "verdict": "valid", | ||
| 331 | "rationale": "runPromise result still not caught; rejection can surface unhandled.", | ||
| 332 | "severity": "Medium", | ||
| 333 | "area": "WebSocket robustness", | ||
| 334 | "path": "apps/server/src/wsServer.ts", | ||
| 335 | "line": 545, | ||
| 336 | "title": "Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails.", | ||
| 337 | "thread_ids": [ | ||
| 338 | "PRRT_kwDORLtfbc5wj4cE" | ||
| 339 | ] | ||
| 340 | }, | ||
| 341 | { | ||
| 342 | "canonical_id": "C027", | ||
| 343 | "verdict": "valid", | ||
| 344 | "rationale": "Same unhandled rejection path remains in WS message handler.", | ||
| 345 | "severity": "Medium", | ||
| 346 | "area": "Runtime resilience and failure handling", | ||
| 347 | "path": "apps/server/src/wsServer.ts", | ||
| 348 | "line": 545, | ||
| 349 | "title": "WS message handler can cause unhandled promise rejection", | ||
| 350 | "thread_ids": [ | ||
| 351 | "PRRT_kwDORLtfbc5wyTaW", | ||
| 352 | "PRRT_kwDORLtfbc5wzli3" | ||
| 353 | ] | ||
| 354 | }, | ||
| 355 | { | ||
| 356 | "canonical_id": "C028", | ||
| 357 | "verdict": "partially-valid", | ||
| 358 | "rationale": "Optimistic local+server dual update is intentional but can temporarily diverge.", | ||
| 359 | "severity": "Medium", | ||
| 360 | "area": "Other", | ||
| 361 | "path": "apps/web/src/components/BranchToolbar.tsx", | ||
| 362 | "line": 102, | ||
| 363 | "title": "Branch sync dispatches both server and stale local update", | ||
| 364 | "thread_ids": [ | ||
| 365 | "PRRT_kwDORLtfbc5v-XCu" | ||
| 366 | ] | ||
| 367 | }, | ||
| 368 | { | ||
| 369 | "canonical_id": "C029", | ||
| 370 | "verdict": "valid", | ||
| 371 | "rationale": "file URL decoding still double-decodes in one path.", | ||
| 372 | "severity": "Medium", | ||
| 373 | "area": "Edge-case parsing/platform behavior", | ||
| 374 | "path": "apps/web/src/markdown-links.ts", | ||
| 375 | "line": 105, | ||
| 376 | "title": "`parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null.", | ||
| 377 | "thread_ids": [ | ||
| 378 | "PRRT_kwDORLtfbc5wnVsU" | ||
| 379 | ] | ||
| 380 | }, | ||
| 381 | { | ||
| 382 | "canonical_id": "C030", | ||
| 383 | "verdict": "valid", | ||
| 384 | "rationale": "Scheme regex still misclassifies script.ts:10 as external scheme.", | ||
| 385 | "severity": "Medium", | ||
| 386 | "area": "Edge-case parsing/platform behavior", | ||
| 387 | "path": "apps/web/src/markdown-links.ts", | ||
| 388 | "line": 111, | ||
| 389 | "title": "`EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 390 | "thread_ids": [ | ||
| 391 | "PRRT_kwDORLtfbc5wnVsK" | ||
| 392 | ] | ||
| 393 | }, | ||
| 394 | { | ||
| 395 | "canonical_id": "C031", | ||
| 396 | "verdict": "invalid", | ||
| 397 | "rationale": "Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary.", | ||
| 398 | "severity": "Medium", | ||
| 399 | "area": "Event ordering and state consistency", | ||
| 400 | "path": "apps/web/src/session-logic.ts", | ||
| 401 | "line": 127, | ||
| 402 | "title": "Revert uses wrong turn count from positional inference", | ||
| 403 | "thread_ids": [ | ||
| 404 | "PRRT_kwDORLtfbc5v9SCp" | ||
| 405 | ] | ||
| 406 | }, | ||
| 407 | { | ||
| 408 | "canonical_id": "C032", | ||
| 409 | "verdict": "valid", | ||
| 410 | "rationale": "Dummy workflow marker comment is still present in docs.", | ||
| 411 | "severity": "Low", | ||
| 412 | "area": "Other", | ||
| 413 | "path": "AGENTS.md", | ||
| 414 | "line": 45, | ||
| 415 | "title": "Dummy workflow marker accidentally committed to docs", | ||
| 416 | "thread_ids": [ | ||
| 417 | "PRRT_kwDORLtfbc5wlYgj", | ||
| 418 | "PRRT_kwDORLtfbc5w1C4H" | ||
| 419 | ] | ||
| 420 | }, | ||
| 421 | { | ||
| 422 | "canonical_id": "C033", | ||
| 423 | "verdict": "partially-valid", | ||
| 424 | "rationale": "Original claim overstated; some errors used, others appear unused.", | ||
| 425 | "severity": "Low", | ||
| 426 | "area": "Checkpointing correctness", | ||
| 427 | "path": "apps/server/src/checkpointing/Errors.ts", | ||
| 428 | "line": 51, | ||
| 429 | "title": "Three error classes defined but never instantiated anywhere", | ||
| 430 | "thread_ids": [ | ||
| 431 | "PRRT_kwDORLtfbc5wlYgo" | ||
| 432 | ] | ||
| 433 | }, | ||
| 434 | { | ||
| 435 | "canonical_id": "C034", | ||
| 436 | "verdict": "valid", | ||
| 437 | "rationale": "CheckpointInvariantError remains redundantly included in service union.", | ||
| 438 | "severity": "Low", | ||
| 439 | "area": "Checkpointing correctness", | ||
| 440 | "path": "apps/server/src/checkpointing/Errors.ts", | ||
| 441 | "line": 79, | ||
| 442 | "title": "Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type", | ||
| 443 | "thread_ids": [ | ||
| 444 | "PRRT_kwDORLtfbc5wj5fn" | ||
| 445 | ] | ||
| 446 | }, | ||
| 447 | { | ||
| 448 | "canonical_id": "C035", | ||
| 449 | "verdict": "valid", | ||
| 450 | "rationale": "Same as C034.", | ||
| 451 | "severity": "Low", | ||
| 452 | "area": "Checkpointing correctness", | ||
| 453 | "path": "apps/server/src/checkpointing/Errors.ts", | ||
| 454 | "line": 79, | ||
| 455 | "title": "Redundant error type in CheckpointServiceError union definition", | ||
| 456 | "thread_ids": [ | ||
| 457 | "PRRT_kwDORLtfbc5wlYgs", | ||
| 458 | "PRRT_kwDORLtfbc5wxsO6", | ||
| 459 | "PRRT_kwDORLtfbc5w1C4B" | ||
| 460 | ] | ||
| 461 | }, | ||
| 462 | { | ||
| 463 | "canonical_id": "C036", | ||
| 464 | "verdict": "invalid", | ||
| 465 | "rationale": "No longer duplicated; single production source via Refs.ts.", | ||
| 466 | "severity": "Low", | ||
| 467 | "area": "Checkpointing correctness", | ||
| 468 | "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", | ||
| 469 | "line": 284, | ||
| 470 | "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", | ||
| 471 | "thread_ids": [ | ||
| 472 | "PRRT_kwDORLtfbc5wiqFX" | ||
| 473 | ] | ||
| 474 | }, | ||
| 475 | { | ||
| 476 | "canonical_id": "C037", | ||
| 477 | "verdict": "partially-valid", | ||
| 478 | "rationale": "Callback cleanup missing, but practical exposure is low in one-shot startup path.", | ||
| 479 | "severity": "Low", | ||
| 480 | "area": "Other", | ||
| 481 | "path": "apps/server/src/config.ts", | ||
| 482 | "line": 41, | ||
| 483 | "title": "`Effect.callback` should return a cleanup function to close the server(s) on fiber interruption. Without it, the `Net.Server` handles keep the process alive and leak the port if the effect is cancelled. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 484 | "thread_ids": [ | ||
| 485 | "PRRT_kwDORLtfbc5wj4cO" | ||
| 486 | ] | ||
| 487 | }, | ||
| 488 | { | ||
| 489 | "canonical_id": "C038", | ||
| 490 | "verdict": "valid", | ||
| 491 | "rationale": "Chunk-by-chunk UTF-8 decode can still corrupt split multibyte characters.", | ||
| 492 | "severity": "Low", | ||
| 493 | "area": "Edge-case parsing/platform behavior", | ||
| 494 | "path": "apps/server/src/git/Layers/CodexTextGeneration.ts", | ||
| 495 | "line": 136, | ||
| 496 | "title": "Multi-byte UTF-8 characters split across chunks will be corrupted when decoding each chunk separately. Consider accumulating all chunks first, then decoding once, or use `TextDecoder` with `stream: true`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 497 | "thread_ids": [ | ||
| 498 | "PRRT_kwDORLtfbc5w1GPo" | ||
| 499 | ] | ||
| 500 | }, | ||
| 501 | { | ||
| 502 | "canonical_id": "C039", | ||
| 503 | "verdict": "partially-valid", | ||
| 504 | "rationale": "Parser/encoder mismatch remains, but encoder path currently low-use.", | ||
| 505 | "severity": "Low", | ||
| 506 | "area": "Edge-case parsing/platform behavior", | ||
| 507 | "path": "apps/server/src/keybindings.ts", | ||
| 508 | "line": 352, | ||
| 509 | "title": "The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes(\"+\")` returns true for the literal `+` key. Consider checking `shortcut.key === \"+\"` separately and encoding it as `\"space\"` style (e.g., a special token), or adjusting the condition to allow the single `+` character. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 510 | "thread_ids": [ | ||
| 511 | "PRRT_kwDORLtfbc5wxvIB" | ||
| 512 | ] | ||
| 513 | }, | ||
| 514 | { | ||
| 515 | "canonical_id": "C040", | ||
| 516 | "verdict": "valid", | ||
| 517 | "rationale": "upsertKeybindingRule read-modify-write remains race-prone.", | ||
| 518 | "severity": "Low", | ||
| 519 | "area": "Edge-case parsing/platform behavior", | ||
| 520 | "path": "apps/server/src/keybindings.ts", | ||
| 521 | "line": 488, | ||
| 522 | "title": "`upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 523 | "thread_ids": [ | ||
| 524 | "PRRT_kwDORLtfbc5wxvIA" | ||
| 525 | ] | ||
| 526 | }, | ||
| 527 | { | ||
| 528 | "canonical_id": "C041", | ||
| 529 | "verdict": "valid", | ||
| 530 | "rationale": "Same Symbol interpolation hazard as C011 remains.", | ||
| 531 | "severity": "Low", | ||
| 532 | "area": "Other", | ||
| 533 | "path": "apps/server/src/orchestration/Errors.ts", | ||
| 534 | "line": 126, | ||
| 535 | "title": "Template literal interpolation throws `TypeError` if `cause` is a `Symbol`. Consider using `String(cause)` for safe coercion.", | ||
| 536 | "thread_ids": [ | ||
| 537 | "PRRT_kwDORLtfbc5wBFWs" | ||
| 538 | ] | ||
| 539 | }, | ||
| 540 | { | ||
| 541 | "canonical_id": "C042", | ||
| 542 | "verdict": "partially-valid", | ||
| 543 | "rationale": "Duplication exists but one instance is variant logic, so impact is moderate.", | ||
| 544 | "severity": "Low", | ||
| 545 | "area": "Runtime resilience and failure handling", | ||
| 546 | "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", | ||
| 547 | "line": 62, | ||
| 548 | "title": "Duplicated `resolveThreadWorkspaceCwd` across three files", | ||
| 549 | "thread_ids": [ | ||
| 550 | "PRRT_kwDORLtfbc5wzli2" | ||
| 551 | ] | ||
| 552 | }, | ||
| 553 | { | ||
| 554 | "canonical_id": "C043", | ||
| 555 | "verdict": "valid", | ||
| 556 | "rationale": "Workspace CWD resolution duplication still present across modules.", | ||
| 557 | "severity": "Low", | ||
| 558 | "area": "Runtime resilience and failure handling", | ||
| 559 | "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", | ||
| 560 | "line": 62, | ||
| 561 | "title": "Duplicated workspace CWD resolution logic across reactor modules", | ||
| 562 | "thread_ids": [ | ||
| 563 | "PRRT_kwDORLtfbc5wnWwM", | ||
| 564 | "PRRT_kwDORLtfbc5w1C3-", | ||
| 565 | "PRRT_kwDORLtfbc5w1HGZ" | ||
| 566 | ] | ||
| 567 | }, | ||
| 568 | { | ||
| 569 | "canonical_id": "C044", | ||
| 570 | "verdict": "partially-valid", | ||
| 571 | "rationale": "Errors are swallowed to empty diff with warning; not fully silent but still lossy.", | ||
| 572 | "severity": "Low", | ||
| 573 | "area": "Runtime resilience and failure handling", | ||
| 574 | "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", | ||
| 575 | "line": 274, | ||
| 576 | "title": "Checkpoint reactor swallows diff errors silently for `turn.completed`", | ||
| 577 | "thread_ids": [ | ||
| 578 | "PRRT_kwDORLtfbc5wkPaO" | ||
| 579 | ] | ||
| 580 | }, | ||
| 581 | { | ||
| 582 | "canonical_id": "C045", | ||
| 583 | "verdict": "valid", | ||
| 584 | "rationale": "truncateDetail still overshoots limit.", | ||
| 585 | "severity": "Low", | ||
| 586 | "area": "Runtime resilience and failure handling", | ||
| 587 | "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", | ||
| 588 | "line": 29, | ||
| 589 | "title": "`truncateDetail` slices to `limit - 1` then appends `\"...\"` (3 chars), producing strings of length `limit + 2`. Consider slicing to `limit - 3` instead. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 590 | "thread_ids": [ | ||
| 591 | "PRRT_kwDORLtfbc5wzp4R" | ||
| 592 | ] | ||
| 593 | }, | ||
| 594 | { | ||
| 595 | "canonical_id": "C046", | ||
| 596 | "verdict": "valid", | ||
| 597 | "rationale": "latestMessageIdByTurnKey still unused/unpruned in per-turn clear path.", | ||
| 598 | "severity": "Low", | ||
| 599 | "area": "Runtime resilience and failure handling", | ||
| 600 | "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", | ||
| 601 | "line": 133, | ||
| 602 | "title": "`latestMessageIdByTurnKey` is written to but never read, and `clearAssistantMessageIdsForTurn` doesn't clear its entries\u2014only `clearTurnStateForSession` does. Consider removing this map entirely if unused, or clearing it alongside `turnMessageIdsByTurnKey` in `clearAssistantMessageIdsForTurn`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 603 | "thread_ids": [ | ||
| 604 | "PRRT_kwDORLtfbc5wxvIQ" | ||
| 605 | ] | ||
| 606 | }, | ||
| 607 | { | ||
| 608 | "canonical_id": "C047", | ||
| 609 | "verdict": "valid", | ||
| 610 | "rationale": "Decode and SQL errors still collapsed into one persistence error kind.", | ||
| 611 | "severity": "Low", | ||
| 612 | "area": "Other", | ||
| 613 | "path": "apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts", | ||
| 614 | "line": 75, | ||
| 615 | "title": "`SqlSchema.findOneOption` can produce both SQL errors and decode errors, but `mapError` wraps all as `PersistenceSqlError`. Consider distinguishing `ParseError` from SQL errors and mapping decode failures to `PersistenceDecodeError` instead. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 616 | "thread_ids": [ | ||
| 617 | "PRRT_kwDORLtfbc5wiaR-" | ||
| 618 | ] | ||
| 619 | }, | ||
| 620 | { | ||
| 621 | "canonical_id": "C048", | ||
| 622 | "verdict": "partially-valid", | ||
| 623 | "rationale": "Unsafe run() result cast remains; runtime risk depends on raw-result consumers.", | ||
| 624 | "severity": "Low", | ||
| 625 | "area": "Other", | ||
| 626 | "path": "apps/server/src/persistence/NodeSqliteClient.ts", | ||
| 627 | "line": 99, | ||
| 628 | "title": "`statement.run()` returns a `RunResult` object, not an array. Casting it to `ReadonlyArray<any>` will cause runtime errors when consumers call array methods. Consider wrapping the result (e.g., `[result]`) or returning a properly typed object.", | ||
| 629 | "thread_ids": [ | ||
| 630 | "PRRT_kwDORLtfbc5wio-r" | ||
| 631 | ] | ||
| 632 | }, | ||
| 633 | { | ||
| 634 | "canonical_id": "C049", | ||
| 635 | "verdict": "valid", | ||
| 636 | "rationale": "JSON.stringify(cause) may return undefined despite string expectations.", | ||
| 637 | "severity": "Low", | ||
| 638 | "area": "Other", | ||
| 639 | "path": "apps/server/src/provider/Layers/ProviderService.ts", | ||
| 640 | "line": 59, | ||
| 641 | "title": "`JSON.stringify(cause)` returns `undefined` for `undefined`, functions, or symbols, violating the `string` return type. Consider coercing the result to a string (e.g., `String(JSON.stringify(cause))`) or adding a fallback.", | ||
| 642 | "thread_ids": [ | ||
| 643 | "PRRT_kwDORLtfbc5wnVsI" | ||
| 644 | ] | ||
| 645 | }, | ||
| 646 | { | ||
| 647 | "canonical_id": "C050", | ||
| 648 | "verdict": "valid", | ||
| 649 | "rationale": "ProviderSessionDirectory upsert remains read-merge-write without concurrency control.", | ||
| 650 | "severity": "Low", | ||
| 651 | "area": "Other", | ||
| 652 | "path": "apps/server/src/provider/Layers/ProviderSessionDirectory.ts", | ||
| 653 | "line": 94, | ||
| 654 | "title": "The read-modify-write pattern (`getBySessionId` \u2192 merge \u2192 `upsert`) is susceptible to lost updates under concurrent writes. Consider wrapping in a transaction or adding optimistic concurrency control (e.g., version field) if concurrent session updates are expected. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 655 | "thread_ids": [ | ||
| 656 | "PRRT_kwDORLtfbc5wiLhY" | ||
| 657 | ] | ||
| 658 | }, | ||
| 659 | { | ||
| 660 | "canonical_id": "C051", | ||
| 661 | "verdict": "partially-valid", | ||
| 662 | "rationale": "Null-clearing issue is real for providerThreadId; adapterKey part overstated.", | ||
| 663 | "severity": "Low", | ||
| 664 | "area": "Other", | ||
| 665 | "path": "apps/server/src/provider/Layers/ProviderSessionDirectory.ts", | ||
| 666 | "line": 119, | ||
| 667 | "title": "Using `??` for `providerThreadId` and `adapterKey` makes it impossible to clear these fields by passing `null`, since `null ?? existing` evaluates to `existing`. Consider using explicit `undefined` checks (like `resumeCursor` does) if clearing should be supported. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 668 | "thread_ids": [ | ||
| 669 | "PRRT_kwDORLtfbc5wxvH9" | ||
| 670 | ] | ||
| 671 | }, | ||
| 672 | { | ||
| 673 | "canonical_id": "C052", | ||
| 674 | "verdict": "valid", | ||
| 675 | "rationale": "Data callback may race before processHandle assignment.", | ||
| 676 | "severity": "Low", | ||
| 677 | "area": "Other", | ||
| 678 | "path": "apps/server/src/terminal/Layers/BunPTY.ts", | ||
| 679 | "line": 97, | ||
| 680 | "title": "Race condition: `processHandle` may be `null` when `data` callback fires, since it's assigned after `Bun.spawn` returns. Consider initializing `BunPtyProcess` first, then passing it to the callback to avoid losing initial output. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 681 | "thread_ids": [ | ||
| 682 | "PRRT_kwDORLtfbc5w1CxE" | ||
| 683 | ] | ||
| 684 | }, | ||
| 685 | { | ||
| 686 | "canonical_id": "C053", | ||
| 687 | "verdict": "valid", | ||
| 688 | "rationale": "Still uses write+destroy rather than end() for rejection response.", | ||
| 689 | "severity": "Low", | ||
| 690 | "area": "WebSocket robustness", | ||
| 691 | "path": "apps/server/src/wsServer.ts", | ||
| 692 | "line": 83, | ||
| 693 | "title": "Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection.", | ||
| 694 | "thread_ids": [ | ||
| 695 | "PRRT_kwDORLtfbc5v-WPD" | ||
| 696 | ] | ||
| 697 | }, | ||
| 698 | { | ||
| 699 | "canonical_id": "C054", | ||
| 700 | "verdict": "valid", | ||
| 701 | "rationale": "Array chunk UTF-8 decode remains vulnerable to split multibyte corruption.", | ||
| 702 | "severity": "Low", | ||
| 703 | "area": "WebSocket robustness", | ||
| 704 | "path": "apps/server/src/wsServer.ts", | ||
| 705 | "line": 104, | ||
| 706 | "title": "When array chunks contain a multi-byte UTF-8 character split across boundaries, decoding each chunk separately produces replacement characters. Consider using `Buffer.concat()` on all chunks before calling `.toString(\"utf8\")`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 707 | "thread_ids": [ | ||
| 708 | "PRRT_kwDORLtfbc5whtrR" | ||
| 709 | ] | ||
| 710 | }, | ||
| 711 | { | ||
| 712 | "canonical_id": "C055", | ||
| 713 | "verdict": "invalid", | ||
| 714 | "rationale": "No longer duplicated; helper is centralized.", | ||
| 715 | "severity": "Low", | ||
| 716 | "area": "Checkpointing correctness", | ||
| 717 | "path": "apps/server/src/wsServer.ts", | ||
| 718 | "line": 128, | ||
| 719 | "title": "Duplicate `checkpointRefForThreadTurn` function across files", | ||
| 720 | "thread_ids": [ | ||
| 721 | "PRRT_kwDORLtfbc5wkPaG" | ||
| 722 | ] | ||
| 723 | }, | ||
| 724 | { | ||
| 725 | "canonical_id": "C056", | ||
| 726 | "verdict": "partially-valid", | ||
| 727 | "rationale": "Bug pattern exists, but current callsites mostly avoid triggering it.", | ||
| 728 | "severity": "Low", | ||
| 729 | "area": "Other", | ||
| 730 | "path": "apps/web/src/components/ui/sidebar.tsx", | ||
| 731 | "line": 114, | ||
| 732 | "title": "When `onOpenChange` is provided without `open`, the internal `_open` state never updates because `setOpenProp` takes precedence. Consider calling `_setOpen` when `openProp === undefined`, regardless of whether `setOpenProp` exists.", | ||
| 733 | "thread_ids": [ | ||
| 734 | "PRRT_kwDORLtfbc5wxvIq" | ||
| 735 | ] | ||
| 736 | }, | ||
| 737 | { | ||
| 738 | "canonical_id": "C057", | ||
| 739 | "verdict": "valid", | ||
| 740 | "rationale": "Resizable object recreation still retriggers effect/storage reads.", | ||
| 741 | "severity": "Low", | ||
| 742 | "area": "Other", | ||
| 743 | "path": "apps/web/src/routes/_chat.$threadId.tsx", | ||
| 744 | "line": 105, | ||
| 745 | "title": "The `resizable` object is recreated on every render, causing `SidebarRail`'s `useEffect` to repeatedly read localStorage and update the DOM. Consider memoizing the object with `useMemo`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 746 | "thread_ids": [ | ||
| 747 | "PRRT_kwDORLtfbc5wyWz4" | ||
| 748 | ] | ||
| 749 | }, | ||
| 750 | { | ||
| 751 | "canonical_id": "C058", | ||
| 752 | "verdict": "valid", | ||
| 753 | "rationale": "Number(null) -> 0 path still forces min width on initial load.", | ||
| 754 | "severity": "Low", | ||
| 755 | "area": "Other", | ||
| 756 | "path": "apps/web/src/routes/_chat.$threadId.tsx", | ||
| 757 | "line": 122, | ||
| 758 | "title": "When `localStorage.getItem()` returns `null`, `Number(null)` evaluates to `0`, which passes `Number.isFinite(0)`. This causes the sidebar to clamp to `minWidth` on first load, overriding the `DIFF_INLINE_DEFAULT_WIDTH` CSS clamp. Consider checking for `null` or empty string before parsing, e.g. guard with `storedWidth === null || storedWidth === ''`. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 759 | "thread_ids": [ | ||
| 760 | "PRRT_kwDORLtfbc5wnVsX" | ||
| 761 | ] | ||
| 762 | }, | ||
| 763 | { | ||
| 764 | "canonical_id": "C059", | ||
| 765 | "verdict": "partially-valid", | ||
| 766 | "rationale": "Transport _tag override risk exists but current callsites are constrained.", | ||
| 767 | "severity": "Low", | ||
| 768 | "area": "WebSocket robustness", | ||
| 769 | "path": "apps/web/src/wsTransport.ts", | ||
| 770 | "line": 59, | ||
| 771 | "title": "Suggestion: don\u2019t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object.", | ||
| 772 | "thread_ids": [ | ||
| 773 | "PRRT_kwDORLtfbc5whtrN" | ||
| 774 | ] | ||
| 775 | }, | ||
| 776 | { | ||
| 777 | "canonical_id": "C060", | ||
| 778 | "verdict": "valid", | ||
| 779 | "rationale": "Schema still cannot express null clear for defaultModel patch.", | ||
| 780 | "severity": "Low", | ||
| 781 | "area": "Other", | ||
| 782 | "path": "packages/contracts/src/orchestration.ts", | ||
| 783 | "line": 253, | ||
| 784 | "title": "`defaultModel` should be `Schema.optional(Schema.NullOr(Schema.String))` to allow clearing the value. Currently there's no way to reset it to `null` since omitting means \"no change\" in patch semantics. <details> <summary>\ud83d\ude80 Reply \"<strong>fix it for me</strong>\" or copy this <strong>AI Prompt</strong> for your agent:</summary>", | ||
| 785 | "thread_ids": [ | ||
| 786 | "PRRT_kwDORLtfbc5whxJC" | ||
| 787 | ] | ||
| 788 | } | ||
| 789 | ] | ||
| 790 | } | ||
.plans/16b-pr89-architecture-validity-audit.md created+125
| ... | @@ -0,0 +1,125 @@ | ||
| 1 | # PR #89 Post-Rewrite Validity Audit | ||
| 2 | |||
| 3 | Architecture baseline used for this audit: | ||
| 4 | - Provider runtime activity is projected into orchestration domain events. | ||
| 5 | - Web client consumes `orchestration.domainEvent`; no separate `providers.event` channel is expected. | ||
| 6 | |||
| 7 | ## Summary | ||
| 8 | |||
| 9 | - Canonical items reviewed: 58 | ||
| 10 | - Valid: 34 | ||
| 11 | - Partially valid: 19 | ||
| 12 | - Invalid (stale/no longer applicable): 5 | ||
| 13 | |||
| 14 | ## Invalid Items To Close | ||
| 15 | |||
| 16 | - C021 [Medium] Shared mutable default metadata object causes stale eventId (apps/server/src/orchestration/decider.ts:27) | ||
| 17 | - Reason: Stale-eventId claim no longer applies; eventId is regenerated per event. | ||
| 18 | - Threads: PRRT_kwDORLtfbc5wkPaA | ||
| 19 | - C025 [Medium] Duplicated checkpoint ref computation across two files (apps/server/src/wsServer.ts:128) | ||
| 20 | - Reason: No longer duplicated; checkpoint ref helper now centralized. | ||
| 21 | - Threads: PRRT_kwDORLtfbc5wvwag | ||
| 22 | - C031 [Medium] Revert uses wrong turn count from positional inference (apps/web/src/session-logic.ts:127) | ||
| 23 | - Reason: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary. | ||
| 24 | - Threads: PRRT_kwDORLtfbc5v9SCp | ||
| 25 | - C036 [Low] Duplicate `checkpointRefForThreadTurn` function in two production files (apps/server/src/checkpointing/Layers/CheckpointStore.ts:284) | ||
| 26 | - Reason: No longer duplicated; single production source via Refs.ts. | ||
| 27 | - Threads: PRRT_kwDORLtfbc5wiqFX | ||
| 28 | - C055 [Low] Duplicate `checkpointRefForThreadTurn` function across files (apps/server/src/wsServer.ts:128) | ||
| 29 | - Reason: No longer duplicated; helper is centralized. | ||
| 30 | - Threads: PRRT_kwDORLtfbc5wkPaG | ||
| 31 | |||
| 32 | ## Partially Valid Items (de-scope or lower priority) | ||
| 33 | |||
| 34 | - C006 [Medium] Build script runs TypeScript file with bare node (apps/server/package.json:14) | ||
| 35 | - Note: Node 24 makes this work, but script still relies on runtime-specific TS execution. | ||
| 36 | - C007 [Medium] Build script uses `node` to run `.ts` file directly (apps/server/package.json:14) | ||
| 37 | - Note: Same as C006 (duplicate concern). | ||
| 38 | - C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) | ||
| 39 | - Note: Normalization inconsistency remains, but upstream callsites now mostly canonicalize inputs. | ||
| 40 | - C014 [Medium] Engine error handler catches all errors including non-invariant ones (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:144) | ||
| 41 | - Note: Broad catch may be intentional for liveness; correctness concern is contextual. | ||
| 42 | - C015 [Medium] The gap-filling fallback logic can retain messages from turns that are about to be deleted, causing foreign key violations. Consider removing the fallback logic entirely, or filtering `fallbackUserMessages` and `fallbackAssistantMessages` to only include messages whose `turnId` is in `retainedTurnIds`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:99) | ||
| 43 | - Note: Message fallback retention issue is real, but prior FK-violation claim is overstated. | ||
| 44 | - C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) | ||
| 45 | - Note: Snapshot sequence can under-report due to extra projectors, but replay impact is lower now. | ||
| 46 | - C019 [Medium] ProviderRuntimeIngestion processes events for wrong thread on race (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:178) | ||
| 47 | - Note: SessionId-only routing can misassociate events under races/rebinds. | ||
| 48 | - C020 [Medium] 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. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:266) | ||
| 49 | - Note: Duplicate complete dispatch exists; downstream impact often idempotent. | ||
| 50 | - C024 [Medium] In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release. (apps/server/src/persistence/NodeSqliteClient.ts:145) | ||
| 51 | - Note: prepare() still outside managed effect boundary; failure handling risk remains. | ||
| 52 | - C028 [Medium] Branch sync dispatches both server and stale local update (apps/web/src/components/BranchToolbar.tsx:102) | ||
| 53 | - Note: Optimistic local+server dual update is intentional but can temporarily diverge. | ||
| 54 | - C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) | ||
| 55 | - Note: Original claim overstated; some errors used, others appear unused. | ||
| 56 | - C037 [Low] `Effect.callback` should return a cleanup function to close the server(s) on fiber interruption. Without it, the `Net.Server` handles keep the process alive and leak the port if the effect is cancelled. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/config.ts:41) | ||
| 57 | - Note: Callback cleanup missing, but practical exposure is low in one-shot startup path. | ||
| 58 | - C039 [Low] The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes("+")` returns true for the literal `+` key. Consider checking `shortcut.key === "+"` separately and encoding it as `"space"` style (e.g., a special token), or adjusting the condition to allow the single `+` character. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/keybindings.ts:352) | ||
| 59 | - Note: Parser/encoder mismatch remains, but encoder path currently low-use. | ||
| 60 | - C042 [Low] Duplicated `resolveThreadWorkspaceCwd` across three files (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) | ||
| 61 | - Note: Duplication exists but one instance is variant logic, so impact is moderate. | ||
| 62 | - C044 [Low] Checkpoint reactor swallows diff errors silently for `turn.completed` (apps/server/src/orchestration/Layers/CheckpointReactor.ts:274) | ||
| 63 | - Note: Errors are swallowed to empty diff with warning; not fully silent but still lossy. | ||
| 64 | - C048 [Low] `statement.run()` returns a `RunResult` object, not an array. Casting it to `ReadonlyArray<any>` will cause runtime errors when consumers call array methods. Consider wrapping the result (e.g., `[result]`) or returning a properly typed object. (apps/server/src/persistence/NodeSqliteClient.ts:99) | ||
| 65 | - Note: Unsafe run() result cast remains; runtime risk depends on raw-result consumers. | ||
| 66 | - C051 [Low] Using `??` for `providerThreadId` and `adapterKey` makes it impossible to clear these fields by passing `null`, since `null ?? existing` evaluates to `existing`. Consider using explicit `undefined` checks (like `resumeCursor` does) if clearing should be supported. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/provider/Layers/ProviderSessionDirectory.ts:119) | ||
| 67 | - Note: Null-clearing issue is real for providerThreadId; adapterKey part overstated. | ||
| 68 | - C056 [Low] When `onOpenChange` is provided without `open`, the internal `_open` state never updates because `setOpenProp` takes precedence. Consider calling `_setOpen` when `openProp === undefined`, regardless of whether `setOpenProp` exists. (apps/web/src/components/ui/sidebar.tsx:114) | ||
| 69 | - Note: Bug pattern exists, but current callsites mostly avoid triggering it. | ||
| 70 | - C059 [Low] Suggestion: don’t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object. (apps/web/src/wsTransport.ts:59) | ||
| 71 | - Note: Transport _tag override risk exists but current callsites are constrained. | ||
| 72 | |||
| 73 | ## Valid Items (actionable unless marked completed) | ||
| 74 | |||
| 75 | - C001 [High] Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:96) | ||
| 76 | - C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) | ||
| 77 | - Status: Completed in Phase 1. | ||
| 78 | - C003 [High] Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake. (apps/server/src/wsServer.ts:75) | ||
| 79 | - C009 [Medium] Git's braced rename syntax (e.g., `src/{old => new}/file.ts`) isn't handled correctly. The current slice after ` => ` produces invalid paths like `new}/file.ts`. Consider expanding the braces to construct the full destination path. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/git/Layers/GitCore.ts:41) | ||
| 80 | - C010 [Medium] `loadCustomKeybindingsConfig` fails when the config file doesn't exist, which is expected for new users. Consider catching `ENOENT` and returning an empty array instead. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/keybindings.ts:418) | ||
| 81 | - C011 [Medium] Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead. (apps/server/src/orchestration/Errors.ts:117) | ||
| 82 | - C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) | ||
| 83 | - Status: Completed in Phase 1. | ||
| 84 | - C013 [Medium] If `projectionPipeline.projectEvent` fails after `eventStore.append` succeeds, the event is persisted but `readModel` isn't updated, causing desync. Consider updating the in-memory `readModel` immediately after append (before the external projection), so local state stays consistent regardless of downstream failures. (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:99) | ||
| 85 | - C016 [Medium] The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:490) | ||
| 86 | - C018 [Medium] Unbounded memory growth in turn start deduplication set (apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:84) | ||
| 87 | - C022 [Medium] Fish shell outputs `$PATH` as space-separated, not colon-separated. Consider checking if the shell is fish and using `string join : $PATH` instead, or validating the result contains colons before assigning. (apps/server/src/os-jank.ts:10) | ||
| 88 | - C023 [Medium] Using `-il` flags causes the shell to source profile scripts that may print banners or other text, polluting the captured `PATH`. Consider using `-lc` (login only, non-interactive) to reduce unwanted output. (apps/server/src/os-jank.ts:10) | ||
| 89 | - C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) | ||
| 90 | - Status: Completed in Phase 1. | ||
| 91 | - C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) | ||
| 92 | - Status: Completed in Phase 1. | ||
| 93 | - C029 [Medium] `parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null. (apps/web/src/markdown-links.ts:105) | ||
| 94 | - C030 [Medium] `EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/web/src/markdown-links.ts:111) | ||
| 95 | - C032 [Low] Dummy workflow marker accidentally committed to docs (AGENTS.md:45) | ||
| 96 | - C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) | ||
| 97 | - C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) | ||
| 98 | - C038 [Low] Multi-byte UTF-8 characters split across chunks will be corrupted when decoding each chunk separately. Consider accumulating all chunks first, then decoding once, or use `TextDecoder` with `stream: true`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/git/Layers/CodexTextGeneration.ts:136) | ||
| 99 | - C040 [Low] `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/keybindings.ts:488) | ||
| 100 | - C041 [Low] Template literal interpolation throws `TypeError` if `cause` is a `Symbol`. Consider using `String(cause)` for safe coercion. (apps/server/src/orchestration/Errors.ts:126) | ||
| 101 | - C043 [Low] Duplicated workspace CWD resolution logic across reactor modules (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) | ||
| 102 | - C045 [Low] `truncateDetail` slices to `limit - 1` then appends `"..."` (3 chars), producing strings of length `limit + 2`. Consider slicing to `limit - 3` instead. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:29) | ||
| 103 | - C046 [Low] `latestMessageIdByTurnKey` is written to but never read, and `clearAssistantMessageIdsForTurn` doesn't clear its entries—only `clearTurnStateForSession` does. Consider removing this map entirely if unused, or clearing it alongside `turnMessageIdsByTurnKey` in `clearAssistantMessageIdsForTurn`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:133) | ||
| 104 | - C047 [Low] `SqlSchema.findOneOption` can produce both SQL errors and decode errors, but `mapError` wraps all as `PersistenceSqlError`. Consider distinguishing `ParseError` from SQL errors and mapping decode failures to `PersistenceDecodeError` instead. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts:75) | ||
| 105 | - C049 [Low] `JSON.stringify(cause)` returns `undefined` for `undefined`, functions, or symbols, violating the `string` return type. Consider coercing the result to a string (e.g., `String(JSON.stringify(cause))`) or adding a fallback. (apps/server/src/provider/Layers/ProviderService.ts:59) | ||
| 106 | - C050 [Low] The read-modify-write pattern (`getBySessionId` → merge → `upsert`) is susceptible to lost updates under concurrent writes. Consider wrapping in a transaction or adding optimistic concurrency control (e.g., version field) if concurrent session updates are expected. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/provider/Layers/ProviderSessionDirectory.ts:94) | ||
| 107 | - C052 [Low] Race condition: `processHandle` may be `null` when `data` callback fires, since it's assigned after `Bun.spawn` returns. Consider initializing `BunPtyProcess` first, then passing it to the callback to avoid losing initial output. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/terminal/Layers/BunPTY.ts:97) | ||
| 108 | - C053 [Low] Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection. (apps/server/src/wsServer.ts:83) | ||
| 109 | - C054 [Low] When array chunks contain a multi-byte UTF-8 character split across boundaries, decoding each chunk separately produces replacement characters. Consider using `Buffer.concat()` on all chunks before calling `.toString("utf8")`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/server/src/wsServer.ts:104) | ||
| 110 | - C057 [Low] The `resizable` object is recreated on every render, causing `SidebarRail`'s `useEffect` to repeatedly read localStorage and update the DOM. Consider memoizing the object with `useMemo`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/web/src/routes/_chat.$threadId.tsx:105) | ||
| 111 | - C058 [Low] When `localStorage.getItem()` returns `null`, `Number(null)` evaluates to `0`, which passes `Number.isFinite(0)`. This causes the sidebar to clamp to `minWidth` on first load, overriding the `DIFF_INLINE_DEFAULT_WIDTH` CSS clamp. Consider checking for `null` or empty string before parsing, e.g. guard with `storedWidth === null || storedWidth === ''`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (apps/web/src/routes/_chat.$threadId.tsx:122) | ||
| 112 | - C060 [Low] `defaultModel` should be `Schema.optional(Schema.NullOr(Schema.String))` to allow clearing the value. Currently there's no way to reset it to `null` since omitting means "no change" in patch semantics. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> (packages/contracts/src/orchestration.ts:253) | ||
| 113 | |||
| 114 | ## By Area | ||
| 115 | |||
| 116 | - Build/runtime portability: valid=0, partially-valid=2, invalid=0 | ||
| 117 | - Checkpointing correctness: valid=2, partially-valid=3, invalid=3 | ||
| 118 | - Edge-case parsing/platform behavior: valid=8, partially-valid=1, invalid=0 | ||
| 119 | - Event ordering and state consistency: valid=3, partially-valid=2, invalid=1 | ||
| 120 | - Memory/resource growth: valid=1, partially-valid=0, invalid=0 | ||
| 121 | - Other: valid=10, partially-valid=6, invalid=1 | ||
| 122 | - Runtime resilience and failure handling: valid=6, partially-valid=4, invalid=0 | ||
| 123 | - WebSocket robustness: valid=4, partially-valid=1, invalid=0 | ||
| 124 | |||
| 125 | Machine-readable mapping: `.plans/16b-pr89-architecture-validity-audit.json`. | ||
apps/server/src/orchestration/Layers/CheckpointReactor.test.ts+165-4
| ... | @@ -42,10 +42,12 @@ const asSessionId = (value: string): ProviderSessionId => ProviderSessionId.make | ... | @@ -42,10 +42,12 @@ const asSessionId = (value: string): ProviderSessionId => ProviderSessionId.make |
| 42 | const asProviderThreadId = (value: string): ProviderThreadId => ProviderThreadId.makeUnsafe(value); | 42 | const asProviderThreadId = (value: string): ProviderThreadId => ProviderThreadId.makeUnsafe(value); |
| 43 | const asTurnId = (value: string): TurnId => TurnId.makeUnsafe(value); | 43 | const asTurnId = (value: string): TurnId => TurnId.makeUnsafe(value); |
| 44 | 44 | ||
| 45 | function createProviderServiceHarness(cwd: string, hasSession = true) { | 45 | function createProviderServiceHarness(cwd: string, hasSession = true, sessionCwd = cwd) { |
| 46 | const now = new Date().toISOString(); | 46 | const now = new Date().toISOString(); |
| 47 | const runtimeEventPubSub = Effect.runSync(PubSub.unbounded<ProviderRuntimeEvent>()); | 47 | const runtimeEventPubSub = Effect.runSync(PubSub.unbounded<ProviderRuntimeEvent>()); |
| 48 | const rollbackConversation = vi.fn(() => Effect.void); | 48 | const rollbackConversation = vi.fn( |
| 49 | (_input: { readonly sessionId: ProviderSessionId; readonly numTurns: number }) => Effect.void, | ||
| 50 | ); | ||
| 49 | 51 | ||
| 50 | const unsupported = <A>() => | 52 | const unsupported = <A>() => |
| 51 | Effect.die(new Error("Unsupported provider call in test")) as Effect.Effect<A, never>; | 53 | Effect.die(new Error("Unsupported provider call in test")) as Effect.Effect<A, never>; |
| ... | @@ -57,7 +59,7 @@ function createProviderServiceHarness(cwd: string, hasSession = true) { | ... | @@ -57,7 +59,7 @@ function createProviderServiceHarness(cwd: string, hasSession = true) { |
| 57 | provider: "codex", | 59 | provider: "codex", |
| 58 | status: "ready", | 60 | status: "ready", |
| 59 | threadId: asProviderThreadId("provider-thread-1"), | 61 | threadId: asProviderThreadId("provider-thread-1"), |
| 60 | cwd, | 62 | cwd: sessionCwd, |
| 61 | createdAt: now, | 63 | createdAt: now, |
| 62 | updatedAt: now, | 64 | updatedAt: now, |
| 63 | }, | 65 | }, |
| ... | @@ -214,10 +216,15 @@ describe("CheckpointReactor", () => { | ... | @@ -214,10 +216,15 @@ describe("CheckpointReactor", () => { |
| 214 | readonly seedFilesystemCheckpoints?: boolean; | 216 | readonly seedFilesystemCheckpoints?: boolean; |
| 215 | readonly projectWorkspaceRoot?: string; | 217 | readonly projectWorkspaceRoot?: string; |
| 216 | readonly threadWorktreePath?: string | null; | 218 | readonly threadWorktreePath?: string | null; |
| 219 | readonly providerSessionCwd?: string; | ||
| 217 | }) { | 220 | }) { |
| 218 | const cwd = createGitRepository(); | 221 | const cwd = createGitRepository(); |
| 219 | tempDirs.push(cwd); | 222 | tempDirs.push(cwd); |
| 220 | const provider = createProviderServiceHarness(cwd, options?.hasSession ?? true); | 223 | const provider = createProviderServiceHarness( |
| 224 | cwd, | ||
| 225 | options?.hasSession ?? true, | ||
| 226 | options?.providerSessionCwd ?? cwd, | ||
| 227 | ); | ||
| 221 | const orchestrationLayer = OrchestrationEngineLive.pipe( | 228 | const orchestrationLayer = OrchestrationEngineLive.pipe( |
| 222 | Layer.provide(OrchestrationProjectionPipelineLive), | 229 | Layer.provide(OrchestrationProjectionPipelineLive), |
| 223 | Layer.provide(OrchestrationEventStoreLive), | 230 | Layer.provide(OrchestrationEventStoreLive), |
| ... | @@ -496,6 +503,65 @@ describe("CheckpointReactor", () => { | ... | @@ -496,6 +503,65 @@ describe("CheckpointReactor", () => { |
| 496 | ); | 503 | ); |
| 497 | }); | 504 | }); |
| 498 | 505 | ||
| 506 | it("continues processing runtime events after a single checkpoint runtime failure", async () => { | ||
| 507 | const nonRepositorySessionCwd = fs.mkdtempSync( | ||
| 508 | path.join(os.tmpdir(), "t3-checkpoint-runtime-non-repo-"), | ||
| 509 | ); | ||
| 510 | tempDirs.push(nonRepositorySessionCwd); | ||
| 511 | |||
| 512 | const harness = await createHarness({ | ||
| 513 | seedFilesystemCheckpoints: false, | ||
| 514 | providerSessionCwd: nonRepositorySessionCwd, | ||
| 515 | }); | ||
| 516 | const createdAt = new Date().toISOString(); | ||
| 517 | |||
| 518 | await Effect.runPromise( | ||
| 519 | harness.engine.dispatch({ | ||
| 520 | type: "thread.session.set", | ||
| 521 | commandId: CommandId.makeUnsafe("cmd-session-set-non-repo-runtime"), | ||
| 522 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 523 | session: { | ||
| 524 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 525 | status: "ready", | ||
| 526 | providerName: "codex", | ||
| 527 | providerSessionId: asSessionId("sess-1"), | ||
| 528 | providerThreadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 529 | activeTurnId: null, | ||
| 530 | lastError: null, | ||
| 531 | updatedAt: createdAt, | ||
| 532 | }, | ||
| 533 | createdAt, | ||
| 534 | }), | ||
| 535 | ); | ||
| 536 | |||
| 537 | harness.provider.emit({ | ||
| 538 | type: "checkpoint.captured", | ||
| 539 | eventId: EventId.makeUnsafe("evt-runtime-capture-failure"), | ||
| 540 | provider: "codex", | ||
| 541 | sessionId: asSessionId("sess-1"), | ||
| 542 | createdAt: new Date().toISOString(), | ||
| 543 | threadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 544 | turnId: asTurnId("turn-runtime-failure"), | ||
| 545 | turnCount: 1, | ||
| 546 | status: "completed", | ||
| 547 | }); | ||
| 548 | |||
| 549 | harness.provider.emit({ | ||
| 550 | type: "turn.started", | ||
| 551 | eventId: EventId.makeUnsafe("evt-turn-started-after-runtime-failure"), | ||
| 552 | provider: "codex", | ||
| 553 | sessionId: asSessionId("sess-1"), | ||
| 554 | createdAt: new Date().toISOString(), | ||
| 555 | threadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 556 | turnId: asTurnId("turn-after-runtime-failure"), | ||
| 557 | }); | ||
| 558 | |||
| 559 | await waitForGitRefExists(harness.cwd, checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 0)); | ||
| 560 | expect( | ||
| 561 | gitRefExists(harness.cwd, checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 0)), | ||
| 562 | ).toBe(true); | ||
| 563 | }); | ||
| 564 | |||
| 499 | it("executes provider revert and emits thread.reverted for checkpoint revert requests", async () => { | 565 | it("executes provider revert and emits thread.reverted for checkpoint revert requests", async () => { |
| 500 | const harness = await createHarness(); | 566 | const harness = await createHarness(); |
| 501 | const createdAt = new Date().toISOString(); | 567 | const createdAt = new Date().toISOString(); |
| ... | @@ -575,6 +641,101 @@ describe("CheckpointReactor", () => { | ... | @@ -575,6 +641,101 @@ describe("CheckpointReactor", () => { |
| 575 | ); | 641 | ); |
| 576 | }); | 642 | }); |
| 577 | 643 | ||
| 644 | it("processes consecutive revert requests with deterministic rollback sequencing", async () => { | ||
| 645 | const harness = await createHarness(); | ||
| 646 | const createdAt = new Date().toISOString(); | ||
| 647 | |||
| 648 | await Effect.runPromise( | ||
| 649 | harness.engine.dispatch({ | ||
| 650 | type: "thread.session.set", | ||
| 651 | commandId: CommandId.makeUnsafe("cmd-session-set-inline-revert"), | ||
| 652 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 653 | session: { | ||
| 654 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 655 | status: "ready", | ||
| 656 | providerName: "codex", | ||
| 657 | providerSessionId: asSessionId("sess-1"), | ||
| 658 | providerThreadId: ProviderThreadId.makeUnsafe("provider-thread-1"), | ||
| 659 | activeTurnId: null, | ||
| 660 | lastError: null, | ||
| 661 | updatedAt: createdAt, | ||
| 662 | }, | ||
| 663 | createdAt, | ||
| 664 | }), | ||
| 665 | ); | ||
| 666 | |||
| 667 | await Effect.runPromise( | ||
| 668 | harness.engine.dispatch({ | ||
| 669 | type: "thread.turn.diff.complete", | ||
| 670 | commandId: CommandId.makeUnsafe("cmd-inline-revert-diff-1"), | ||
| 671 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 672 | turnId: asTurnId("turn-1"), | ||
| 673 | completedAt: createdAt, | ||
| 674 | checkpointRef: checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 1), | ||
| 675 | status: "ready", | ||
| 676 | files: [], | ||
| 677 | checkpointTurnCount: 1, | ||
| 678 | createdAt, | ||
| 679 | }), | ||
| 680 | ); | ||
| 681 | await Effect.runPromise( | ||
| 682 | harness.engine.dispatch({ | ||
| 683 | type: "thread.turn.diff.complete", | ||
| 684 | commandId: CommandId.makeUnsafe("cmd-inline-revert-diff-2"), | ||
| 685 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 686 | turnId: asTurnId("turn-2"), | ||
| 687 | completedAt: createdAt, | ||
| 688 | checkpointRef: checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 2), | ||
| 689 | status: "ready", | ||
| 690 | files: [], | ||
| 691 | checkpointTurnCount: 2, | ||
| 692 | createdAt, | ||
| 693 | }), | ||
| 694 | ); | ||
| 695 | |||
| 696 | await Effect.runPromise( | ||
| 697 | harness.engine.dispatch({ | ||
| 698 | type: "thread.checkpoint.revert", | ||
| 699 | commandId: CommandId.makeUnsafe("cmd-sequenced-revert-request-1"), | ||
| 700 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 701 | turnCount: 1, | ||
| 702 | createdAt, | ||
| 703 | }), | ||
| 704 | ); | ||
| 705 | await Effect.runPromise( | ||
| 706 | harness.engine.dispatch({ | ||
| 707 | type: "thread.checkpoint.revert", | ||
| 708 | commandId: CommandId.makeUnsafe("cmd-sequenced-revert-request-0"), | ||
| 709 | threadId: ThreadId.makeUnsafe("thread-1"), | ||
| 710 | turnCount: 0, | ||
| 711 | createdAt, | ||
| 712 | }), | ||
| 713 | ); | ||
| 714 | |||
| 715 | const deadline = Date.now() + 2000; | ||
| 716 | const waitForRollbackCalls = async (): Promise<void> => { | ||
| 717 | if (harness.provider.rollbackConversation.mock.calls.length >= 2) { | ||
| 718 | return; | ||
| 719 | } | ||
| 720 | if (Date.now() >= deadline) { | ||
| 721 | throw new Error("Timed out waiting for rollbackConversation calls."); | ||
| 722 | } | ||
| 723 | await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| 724 | return waitForRollbackCalls(); | ||
| 725 | }; | ||
| 726 | await waitForRollbackCalls(); | ||
| 727 | |||
| 728 | expect(harness.provider.rollbackConversation).toHaveBeenCalledTimes(2); | ||
| 729 | expect(harness.provider.rollbackConversation.mock.calls[0]?.[0]).toEqual({ | ||
| 730 | sessionId: asSessionId("sess-1"), | ||
| 731 | numTurns: 1, | ||
| 732 | }); | ||
| 733 | expect(harness.provider.rollbackConversation.mock.calls[1]?.[0]).toEqual({ | ||
| 734 | sessionId: asSessionId("sess-1"), | ||
| 735 | numTurns: 1, | ||
| 736 | }); | ||
| 737 | }); | ||
| 738 | |||
| 578 | it("appends an error activity when revert is requested without an active session", async () => { | 739 | it("appends an error activity when revert is requested without an active session", async () => { |
| 579 | const harness = await createHarness({ hasSession: false }); | 740 | const harness = await createHarness({ hasSession: false }); |
| 580 | const createdAt = new Date().toISOString(); | 741 | const createdAt = new Date().toISOString(); |
apps/server/src/orchestration/Layers/CheckpointReactor.ts+18-3
| ... | @@ -9,7 +9,7 @@ import { | ... | @@ -9,7 +9,7 @@ import { |
| 9 | type OrchestrationEvent, | 9 | type OrchestrationEvent, |
| 10 | type ProviderRuntimeEvent, | 10 | type ProviderRuntimeEvent, |
| 11 | } from "@t3tools/contracts"; | 11 | } from "@t3tools/contracts"; |
| 12 | import { Effect, Layer, Option, Queue, Stream } from "effect"; | 12 | import { Cause, Effect, Layer, Option, Queue, Stream } from "effect"; |
| 13 | 13 | ||
| 14 | import { parseTurnDiffFilesFromUnifiedDiff } from "../../checkpointing/Diffs.ts"; | 14 | import { parseTurnDiffFilesFromUnifiedDiff } from "../../checkpointing/Diffs.ts"; |
| 15 | import { checkpointRefForThreadTurn } from "../../checkpointing/Refs.ts"; | 15 | import { checkpointRefForThreadTurn } from "../../checkpointing/Refs.ts"; |
| ... | @@ -537,7 +537,6 @@ const make = Effect.gen(function* () { | ... | @@ -537,7 +537,6 @@ const make = Effect.gen(function* () { |
| 537 | createdAt: now, | 537 | createdAt: now, |
| 538 | }), | 538 | }), |
| 539 | ), | 539 | ), |
| 540 | Effect.forkScoped, | ||
| 541 | Effect.asVoid, | 540 | Effect.asVoid, |
| 542 | ); | 541 | ); |
| 543 | }); | 542 | }); |
| ... | @@ -672,11 +671,27 @@ const make = Effect.gen(function* () { | ... | @@ -672,11 +671,27 @@ const make = Effect.gen(function* () { |
| 672 | ): Effect.Effect<void, CheckpointStoreError | OrchestrationDispatchError, never> => | 671 | ): Effect.Effect<void, CheckpointStoreError | OrchestrationDispatchError, never> => |
| 673 | input.source === "domain" ? processDomainEvent(input.event) : processRuntimeEvent(input.event); | 672 | input.source === "domain" ? processDomainEvent(input.event) : processRuntimeEvent(input.event); |
| 674 | 673 | ||
| 674 | const processInputSafely = (input: ReactorInput) => | ||
| 675 | processInput(input).pipe( | ||
| 676 | Effect.catchCause((cause) => { | ||
| 677 | if (Cause.hasInterruptsOnly(cause)) { | ||
| 678 | return Effect.failCause(cause); | ||
| 679 | } | ||
| 680 | return Effect.logWarning("checkpoint reactor failed to process input", { | ||
| 681 | source: input.source, | ||
| 682 | eventType: input.event.type, | ||
| 683 | cause: Cause.pretty(cause), | ||
| 684 | }); | ||
| 685 | }), | ||
| 686 | ); | ||
| 687 | |||
| 675 | const start: CheckpointReactorShape["start"] = Effect.gen(function* () { | 688 | const start: CheckpointReactorShape["start"] = Effect.gen(function* () { |
| 676 | const queue = yield* Queue.unbounded<ReactorInput>(); | 689 | const queue = yield* Queue.unbounded<ReactorInput>(); |
| 677 | yield* Effect.addFinalizer(() => Queue.shutdown(queue).pipe(Effect.asVoid)); | 690 | yield* Effect.addFinalizer(() => Queue.shutdown(queue).pipe(Effect.asVoid)); |
| 678 | 691 | ||
| 679 | yield* Effect.forkScoped(Effect.forever(Queue.take(queue).pipe(Effect.flatMap(processInput)))); | 692 | yield* Effect.forkScoped( |
| 693 | Effect.forever(Queue.take(queue).pipe(Effect.flatMap(processInputSafely))), | ||
| 694 | ); | ||
| 680 | 695 | ||
| 681 | yield* Effect.forkScoped( | 696 | yield* Effect.forkScoped( |
| 682 | Stream.runForEach(orchestrationEngine.streamDomainEvents, (event) => { | 697 | Stream.runForEach(orchestrationEngine.streamDomainEvents, (event) => { |
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts+36
| ... | @@ -327,4 +327,40 @@ describe("ProviderRuntimeIngestion", () => { | ... | @@ -327,4 +327,40 @@ describe("ProviderRuntimeIngestion", () => { |
| 327 | expect(thread.session?.status).toBe("ready"); | 327 | expect(thread.session?.status).toBe("ready"); |
| 328 | expect(thread.activities.some((activity) => activity.kind === "tool.started")).toBe(true); | 328 | expect(thread.activities.some((activity) => activity.kind === "tool.started")).toBe(true); |
| 329 | }); | 329 | }); |
| 330 | |||
| 331 | it("continues processing runtime events after a single event handler failure", async () => { | ||
| 332 | const harness = await createHarness(); | ||
| 333 | const now = new Date().toISOString(); | ||
| 334 | |||
| 335 | harness.emit({ | ||
| 336 | type: "message.delta", | ||
| 337 | eventId: asEventId("evt-invalid-delta"), | ||
| 338 | provider: "codex", | ||
| 339 | sessionId: asSessionId("sess-1"), | ||
| 340 | createdAt: now, | ||
| 341 | turnId: asProviderTurnId("turn-invalid"), | ||
| 342 | itemId: asItemId("item-invalid"), | ||
| 343 | delta: undefined, | ||
| 344 | } as unknown as ProviderRuntimeEvent); | ||
| 345 | |||
| 346 | harness.emit({ | ||
| 347 | type: "runtime.error", | ||
| 348 | eventId: asEventId("evt-runtime-error-after-failure"), | ||
| 349 | provider: "codex", | ||
| 350 | sessionId: asSessionId("sess-1"), | ||
| 351 | createdAt: new Date().toISOString(), | ||
| 352 | turnId: asProviderTurnId("turn-after-failure"), | ||
| 353 | message: "runtime still processed", | ||
| 354 | }); | ||
| 355 | |||
| 356 | const thread = await waitForThread( | ||
| 357 | harness.engine, | ||
| 358 | (entry) => | ||
| 359 | entry.session?.status === "error" && | ||
| 360 | entry.session?.activeTurnId === "turn-after-failure" && | ||
| 361 | entry.session?.lastError === "runtime still processed", | ||
| 362 | ); | ||
| 363 | expect(thread.session?.status).toBe("error"); | ||
| 364 | expect(thread.session?.lastError).toBe("runtime still processed"); | ||
| 365 | }); | ||
| 330 | }); | 366 | }); |
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts+16-2
| ... | @@ -7,7 +7,7 @@ import { | ... | @@ -7,7 +7,7 @@ import { |
| 7 | type ProviderRuntimeEvent, | 7 | type ProviderRuntimeEvent, |
| 8 | type ProviderSessionId, | 8 | type ProviderSessionId, |
| 9 | } from "@t3tools/contracts"; | 9 | } from "@t3tools/contracts"; |
| 10 | import { Effect, Layer, Queue, Stream } from "effect"; | 10 | import { Cause, Effect, Layer, Queue, Stream } from "effect"; |
| 11 | 11 | ||
| 12 | import { ProviderService } from "../../provider/Services/ProviderService.ts"; | 12 | import { ProviderService } from "../../provider/Services/ProviderService.ts"; |
| 13 | import { OrchestrationEngineService } from "../Services/OrchestrationEngine.ts"; | 13 | import { OrchestrationEngineService } from "../Services/OrchestrationEngine.ts"; |
| ... | @@ -324,13 +324,27 @@ const make = Effect.gen(function* () { | ... | @@ -324,13 +324,27 @@ const make = Effect.gen(function* () { |
| 324 | ).pipe(Effect.asVoid); | 324 | ).pipe(Effect.asVoid); |
| 325 | }); | 325 | }); |
| 326 | 326 | ||
| 327 | const processEventSafely = (event: ProviderRuntimeEvent) => | ||
| 328 | processEvent(event).pipe( | ||
| 329 | Effect.catchCause((cause) => { | ||
| 330 | if (Cause.hasInterruptsOnly(cause)) { | ||
| 331 | return Effect.failCause(cause); | ||
| 332 | } | ||
| 333 | return Effect.logWarning("provider runtime ingestion failed to process event", { | ||
| 334 | eventId: event.eventId, | ||
| 335 | eventType: event.type, | ||
| 336 | cause: Cause.pretty(cause), | ||
| 337 | }); | ||
| 338 | }), | ||
| 339 | ); | ||
| 340 | |||
| 327 | const start: ProviderRuntimeIngestionShape["start"] = Effect.gen(function* () { | 341 | const start: ProviderRuntimeIngestionShape["start"] = Effect.gen(function* () { |
| 328 | const providerEventQueue = yield* Queue.unbounded<ProviderRuntimeEvent>(); | 342 | const providerEventQueue = yield* Queue.unbounded<ProviderRuntimeEvent>(); |
| 329 | 343 | ||
| 330 | yield* Effect.addFinalizer(() => Queue.shutdown(providerEventQueue).pipe(Effect.asVoid)); | 344 | yield* Effect.addFinalizer(() => Queue.shutdown(providerEventQueue).pipe(Effect.asVoid)); |
| 331 | 345 | ||
| 332 | yield* Effect.forkScoped( | 346 | yield* Effect.forkScoped( |
| 333 | Effect.forever(Queue.take(providerEventQueue).pipe(Effect.flatMap(processEvent))), | 347 | Effect.forever(Queue.take(providerEventQueue).pipe(Effect.flatMap(processEventSafely))), |
| 334 | ); | 348 | ); |
| 335 | yield* Effect.forkScoped( | 349 | yield* Effect.forkScoped( |
| 336 | Stream.runForEach(providerService.streamEvents, (event) => | 350 | Stream.runForEach(providerService.streamEvents, (event) => |
apps/server/src/wsServer.test.ts+59
| ... | @@ -1000,6 +1000,65 @@ describe("WebSocket Server", () => { | ... | @@ -1000,6 +1000,65 @@ describe("WebSocket Server", () => { |
| 1000 | expect(response!.error!.message).toContain("Invalid request format"); | 1000 | expect(response!.error!.message).toContain("Invalid request format"); |
| 1001 | }); | 1001 | }); |
| 1002 | 1002 | ||
| 1003 | it("catches websocket message handler rejections and keeps the socket usable", async () => { | ||
| 1004 | const unhandledRejections: unknown[] = []; | ||
| 1005 | const onUnhandledRejection = (reason: unknown) => { | ||
| 1006 | unhandledRejections.push(reason); | ||
| 1007 | }; | ||
| 1008 | process.on("unhandledRejection", onUnhandledRejection); | ||
| 1009 | |||
| 1010 | const brokenOpenService: OpenShape = { | ||
| 1011 | openBrowser: () => Effect.void, | ||
| 1012 | openInEditor: () => | ||
| 1013 | Effect.sync(() => BigInt(1)).pipe(Effect.map((result) => result as unknown as void)), | ||
| 1014 | }; | ||
| 1015 | |||
| 1016 | try { | ||
| 1017 | server = await createTestServer({ cwd: "/test", open: brokenOpenService }); | ||
| 1018 | const addr = server.address(); | ||
| 1019 | const port = typeof addr === "object" && addr !== null ? addr.port : 0; | ||
| 1020 | |||
| 1021 | const ws = await connectWs(port); | ||
| 1022 | connections.push(ws); | ||
| 1023 | await waitForMessage(ws); | ||
| 1024 | |||
| 1025 | ws.send( | ||
| 1026 | JSON.stringify({ | ||
| 1027 | id: "req-broken-open", | ||
| 1028 | body: { | ||
| 1029 | _tag: WS_METHODS.shellOpenInEditor, | ||
| 1030 | cwd: "/tmp", | ||
| 1031 | editor: "cursor", | ||
| 1032 | }, | ||
| 1033 | }), | ||
| 1034 | ); | ||
| 1035 | |||
| 1036 | await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| 1037 | expect(unhandledRejections).toHaveLength(0); | ||
| 1038 | |||
| 1039 | const workspace = makeTempDir("t3code-ws-handler-still-usable-"); | ||
| 1040 | fs.writeFileSync(path.join(workspace, "file.txt"), "ok\n", "utf8"); | ||
| 1041 | const response = await sendRequest(ws, WS_METHODS.projectsSearchEntries, { | ||
| 1042 | cwd: workspace, | ||
| 1043 | query: "file", | ||
| 1044 | limit: 5, | ||
| 1045 | }); | ||
| 1046 | expect(response.error).toBeUndefined(); | ||
| 1047 | expect(response.result).toEqual( | ||
| 1048 | expect.objectContaining({ | ||
| 1049 | entries: expect.arrayContaining([ | ||
| 1050 | expect.objectContaining({ | ||
| 1051 | path: "file.txt", | ||
| 1052 | kind: "file", | ||
| 1053 | }), | ||
| 1054 | ]), | ||
| 1055 | }), | ||
| 1056 | ); | ||
| 1057 | } finally { | ||
| 1058 | process.off("unhandledRejection", onUnhandledRejection); | ||
| 1059 | } | ||
| 1060 | }); | ||
| 1061 | |||
| 1003 | it("returns errors for removed projects CRUD methods", async () => { | 1062 | it("returns errors for removed projects CRUD methods", async () => { |
| 1004 | server = await createTestServer({ cwd: "/test" }); | 1063 | server = await createTestServer({ cwd: "/test" }); |
| 1005 | const addr = server.address(); | 1064 | const addr = server.address(); |
apps/server/src/wsServer.ts+5-1
| ... | @@ -542,7 +542,11 @@ export const createServer = Effect.fn(function* (options: ServerOptions = {}): E | ... | @@ -542,7 +542,11 @@ export const createServer = Effect.fn(function* (options: ServerOptions = {}): E |
| 542 | ws.send(JSON.stringify(welcome)); | 542 | ws.send(JSON.stringify(welcome)); |
| 543 | 543 | ||
| 544 | ws.on("message", (raw) => { | 544 | ws.on("message", (raw) => { |
| 545 | void Effect.runPromise(handleMessage(ws, raw)); | 545 | void Effect.runPromise( |
| 546 | handleMessage(ws, raw).pipe( | ||
| 547 | Effect.catch((error) => Effect.logError("Error handling message", error)), | ||
| 548 | ), | ||
| 549 | ); | ||
| 546 | }); | 550 | }); |
| 547 | 551 | ||
| 548 | ws.on("close", () => { | 552 | ws.on("close", () => { |