| author | |
| committer | |
| log | 320f249a3139b7aaa531c04a4b2bbbdad522c74c |
| tree | ae4f95c951b88569fb5bbce0767936f51b1b9fc3 |
| parent | 734ab399ebf030dd799a5744b3e780ac5e7c6a77 |
7 files changed, 896 insertions(+), 100 deletions(-)
.oxfmtrc.json+2-1| ... | @@ -6,7 +6,8 @@ | ... | @@ -6,7 +6,8 @@ |
| 6 | "dist-electron", | 6 | "dist-electron", |
| 7 | "node_modules", | 7 | "node_modules", |
| 8 | "bun.lock", | 8 | "bun.lock", |
| 9 | "*.tsbuildinfo" | 9 | "*.tsbuildinfo", |
| 10 | "**/routeTree.gen.ts" | ||
| 10 | ], | 11 | ], |
| 11 | "experimentalSortPackageJson": {} | 12 | "experimentalSortPackageJson": {} |
| 12 | } | 13 | } |
.plans/16-frontend-contract-schema-validation-alignment.md created+182| ... | @@ -0,0 +1,182 @@ | ||
| 1 | # Plan: Frontend Contract Schema Validation Alignment | ||
| 2 | |||
| 3 | ## Summary | ||
| 4 | Align frontend input/output validation with `@t3tools/contracts` schemas so the web client stops relying on ad-hoc checks and unsafe casts at critical boundaries. | ||
| 5 | |||
| 6 | ## Goals | ||
| 7 | 1. Decode all WebSocket envelopes with shared contract schemas. | ||
| 8 | 2. Decode push payloads before handing them to app logic. | ||
| 9 | 3. Replace duplicated frontend range/input checks with contract schema decoding. | ||
| 10 | 4. Validate project search params with shared constraints before dispatch. | ||
| 11 | 5. Validate script keybinding updates with `KeybindingRule` schema before RPC. | ||
| 12 | 6. Propagate branded IDs (`ThreadId`, `ProjectId`, etc.) through typed app flow after single ingress conversion. | ||
| 13 | |||
| 14 | ## Non-Goals | ||
| 15 | - No protocol shape changes. | ||
| 16 | - No server behavior changes unless uncovered by failing tests. | ||
| 17 | - No UI redesign; only validation/transport boundary hardening. | ||
| 18 | |||
| 19 | ## Phase 1: WebSocket Envelope Decoding | ||
| 20 | |||
| 21 | ### Issue | ||
| 22 | `apps/web/src/wsTransport.ts` parses JSON and performs manual object/cast checks (`as WsPush`) instead of decoding with contracts. | ||
| 23 | |||
| 24 | ### Changes | ||
| 25 | 1. Decode inbound messages with contract schema union (`WsResponse`). | ||
| 26 | 2. Remove manual push/response shape checks that duplicate schema logic. | ||
| 27 | 3. Convert decode failures into safe no-op handling with structured debug logging. | ||
| 28 | |||
| 29 | ### Target Files | ||
| 30 | - `apps/web/src/wsTransport.ts` | ||
| 31 | - `apps/web/src/wsTransport.test.ts` | ||
| 32 | |||
| 33 | ### Tests | ||
| 34 | 1. Accept valid push envelope and route to channel listener. | ||
| 35 | 2. Accept valid response envelope and resolve pending request. | ||
| 36 | 3. Reject malformed envelope without crashing transport. | ||
| 37 | |||
| 38 | ## Phase 2: Push Payload Decoding at API Boundary | ||
| 39 | |||
| 40 | ### Issue | ||
| 41 | `apps/web/src/wsNativeApi.ts` forwards unknown push payloads and uses direct casts (`data as WsWelcomePayload`). | ||
| 42 | |||
| 43 | ### Changes | ||
| 44 | 1. Decode `server.welcome` payload with `WsWelcomePayload`. | ||
| 45 | 2. Decode `terminal.event` payload with `TerminalEvent`. | ||
| 46 | 3. Decode `orchestration.domainEvent` payload with `OrchestrationEvent`. | ||
| 47 | 4. Skip invalid payloads safely and keep subscriptions alive. | ||
| 48 | |||
| 49 | ### Target Files | ||
| 50 | - `apps/web/src/wsNativeApi.ts` | ||
| 51 | - `apps/web/src/wsNativeApi.test.ts` | ||
| 52 | |||
| 53 | ### Tests | ||
| 54 | 1. Valid welcome payload is delivered and cached. | ||
| 55 | 2. Invalid welcome payload is ignored. | ||
| 56 | 3. Valid terminal/orchestration events are forwarded. | ||
| 57 | 4. Invalid push data does not invoke consumers. | ||
| 58 | |||
| 59 | ## Phase 3: Replace Duplicated Checkpoint Range Validation | ||
| 60 | |||
| 61 | ### Issue | ||
| 62 | `apps/web/src/lib/providerReactQuery.ts` duplicates `fromTurnCount`/`toTurnCount` validation rules already represented in contracts. | ||
| 63 | |||
| 64 | ### Changes | ||
| 65 | 1. Use `OrchestrationGetTurnDiffInput` / `OrchestrationGetFullThreadDiffInput` decoding before RPC calls. | ||
| 66 | 2. Remove bespoke integer/range checks where schema decoding can be authoritative. | ||
| 67 | 3. Preserve current UX behavior (`enabled` gating + retry policy). | ||
| 68 | |||
| 69 | ### Target Files | ||
| 70 | - `apps/web/src/lib/providerReactQuery.ts` | ||
| 71 | - `apps/web/src/lib/providerReactQuery.test.ts` | ||
| 72 | |||
| 73 | ### Tests | ||
| 74 | 1. Valid range decodes and calls expected RPC. | ||
| 75 | 2. Invalid range fails fast and avoids RPC call. | ||
| 76 | 3. `fromTurnCount === 0` path still routes to `getFullThreadDiff`. | ||
| 77 | |||
| 78 | ## Phase 4: Project Search Input Validation from Contracts | ||
| 79 | |||
| 80 | ### Issue | ||
| 81 | `apps/web/src/lib/projectReactQuery.ts` forwards search inputs without enforcing `ProjectSearchEntriesInput` constraints, and `ChatView` can pass empty query strings. | ||
| 82 | |||
| 83 | ### Changes | ||
| 84 | 1. Decode project search input with `ProjectSearchEntriesInput` before request dispatch. | ||
| 85 | 2. Gate query execution when query is empty after trim. | ||
| 86 | 3. Replace magic search limit usage with shared constant (`PROJECT_SEARCH_ENTRIES_MAX_LIMIT`) or schema-safe value. | ||
| 87 | |||
| 88 | ### Target Files | ||
| 89 | - `apps/web/src/lib/projectReactQuery.ts` | ||
| 90 | - `apps/web/src/components/ChatView.tsx` | ||
| 91 | - `apps/web/src/lib/projectReactQuery.test.ts` | ||
| 92 | |||
| 93 | ### Tests | ||
| 94 | 1. Empty query does not dispatch request. | ||
| 95 | 2. Over-limit values are rejected or clamped via schema-safe path. | ||
| 96 | 3. Valid queries still return prior placeholder behavior. | ||
| 97 | |||
| 98 | ## Phase 5: Script/Keybinding Validation Consistency | ||
| 99 | |||
| 100 | ### Issue | ||
| 101 | `ProjectScriptsControl` and `ChatView` rely mostly on local non-empty checks while sending keybinding data into a contract-validated endpoint (`server.upsertKeybinding`). | ||
| 102 | |||
| 103 | ### Changes | ||
| 104 | 1. Add pre-submit decode for keybinding payload using `KeybindingRule`. | ||
| 105 | 2. Normalize and validate keybinding values via schema before calling `upsertKeybinding`. | ||
| 106 | 3. Keep existing script ID command generation logic, but ensure command + key are schema-valid before mutation. | ||
| 107 | |||
| 108 | ### Target Files | ||
| 109 | - `apps/web/src/components/ProjectScriptsControl.tsx` | ||
| 110 | - `apps/web/src/components/ChatView.tsx` | ||
| 111 | - (optional helper) `apps/web/src/projectScripts.ts` | ||
| 112 | - `apps/web/src/components/ProjectScriptsControl.test.tsx` (or logic test file) | ||
| 113 | |||
| 114 | ### Tests | ||
| 115 | 1. Invalid keybinding string surfaces UI validation error and blocks submission. | ||
| 116 | 2. Valid keybinding updates still persist and refresh cached server config. | ||
| 117 | 3. Editing existing script preserves valid keybinding mapping behavior. | ||
| 118 | |||
| 119 | ## Phase 6: Branded ID Flow (Single Ingress Branding) | ||
| 120 | |||
| 121 | ### Issue | ||
| 122 | Frontend currently converts plain strings to branded IDs repeatedly (`asThreadId`, `asProjectId`, etc.) at many callsites, which spreads unsafe conversions across app logic. | ||
| 123 | |||
| 124 | ### Desired Model | ||
| 125 | 1. Convert/decode to branded IDs once at ingress boundaries. | ||
| 126 | 2. Keep IDs branded through store state, selectors, and command/query builders. | ||
| 127 | 3. Avoid repeated `*.makeUnsafe` wrappers in feature logic. | ||
| 128 | |||
| 129 | ### Boundary Rules | ||
| 130 | 1. Router/search params (string) -> decode to branded IDs at route boundary. | ||
| 131 | 2. Storage/network payloads (untyped JSON) -> decode with contract schemas at transport/query boundary. | ||
| 132 | 3. ID generation (`newThreadId`, `newProjectId`, etc.) remains branded at source. | ||
| 133 | 4. UI components should consume typed IDs from store/hooks, not raw strings. | ||
| 134 | |||
| 135 | ### Changes | ||
| 136 | 1. Introduce a thin typed ID layer in web app (or reuse contracts decoders directly) for safe `unknown -> branded` conversion at boundaries. | ||
| 137 | 2. Refactor web store and thread/project domain types to carry branded IDs where feasible. | ||
| 138 | 3. Replace repeated `asThreadId`/`asProjectId` callsites in: | ||
| 139 | - `apps/web/src/components/ChatView.tsx` | ||
| 140 | - `apps/web/src/components/Sidebar.tsx` | ||
| 141 | - `apps/web/src/components/BranchToolbar.tsx` | ||
| 142 | - `apps/web/src/lib/providerReactQuery.ts` | ||
| 143 | 4. Keep unavoidable casts isolated in one boundary module (not feature components). | ||
| 144 | 5. Minimize/remove broad `asXId` helpers from `apps/web/src/lib/orchestrationIds.ts` once flow is typed. | ||
| 145 | |||
| 146 | ### Target Files | ||
| 147 | - `apps/web/src/lib/orchestrationIds.ts` | ||
| 148 | - `apps/web/src/types.ts` | ||
| 149 | - `apps/web/src/store.ts` | ||
| 150 | - `apps/web/src/components/ChatView.tsx` | ||
| 151 | - `apps/web/src/components/Sidebar.tsx` | ||
| 152 | - `apps/web/src/components/BranchToolbar.tsx` | ||
| 153 | - `apps/web/src/lib/providerReactQuery.ts` | ||
| 154 | - route boundary files (`apps/web/src/routes/*`) as needed | ||
| 155 | |||
| 156 | ### Tests | ||
| 157 | 1. Type-level regression checks (compile-time) proving command builders accept branded IDs directly. | ||
| 158 | 2. Route/load path tests verifying invalid ID ingress is rejected/fails safely. | ||
| 159 | 3. Existing behavior tests still pass for thread/project operations after refactor. | ||
| 160 | |||
| 161 | ## Cross-Cutting Execution Notes | ||
| 162 | 1. Prefer a small shared frontend utility for Effect schema decoding to avoid repeated `decodeUnknownSync` boilerplate. | ||
| 163 | 2. Use `Schema.decodeUnknownEither` or equivalent non-throwing path where event stream robustness matters. | ||
| 164 | 3. Keep failure behavior fail-soft at stream boundaries (drop invalid events, keep transport running). | ||
| 165 | 4. Brands are TypeScript-level constraints; JSON boundaries still require decode/re-brand at ingress. | ||
| 166 | |||
| 167 | ## Validation Checklist | ||
| 168 | 1. `bun run lint` | ||
| 169 | 2. `bun run test --filter=@t3tools/web` (or equivalent web test command in repo) | ||
| 170 | 3. Manual smoke checks: | ||
| 171 | - App boot + welcome sync | ||
| 172 | - Terminal stream still renders | ||
| 173 | - Diff panel loads checkpoint ranges | ||
| 174 | - Path autocomplete search remains responsive | ||
| 175 | - Script add/edit with keybinding works in Electron flow | ||
| 176 | |||
| 177 | ## Done Criteria | ||
| 178 | 1. No unsafe push/event casts remain at core WebSocket boundaries. | ||
| 179 | 2. Frontend no longer duplicates contract input validation for checkpoint range and project search. | ||
| 180 | 3. Keybinding submission path is schema-validated before RPC. | ||
| 181 | 4. Branded IDs flow through frontend feature code without repeated `asThreadId`/`asProjectId` wrappers. | ||
| 182 | 5. Updated tests cover malformed payload and invalid input regressions. | ||
.plans/pr89-review-feedback-resolution-plan.md created+421| ... | @@ -0,0 +1,421 @@ | ||
| 1 | # PR #89 Review Feedback Resolution Plan | ||
| 2 | |||
| 3 | ## Context | ||
| 4 | - Source: GitHub review threads/comments on PR `#89` (`Add server-side orchestration engine with event sourcing`). | ||
| 5 | - Triage baseline: | ||
| 6 | - 147 threads, 149 comments total. | ||
| 7 | - 54 active unresolved threads. | ||
| 8 | - Deduped to 47 unique active issues. | ||
| 9 | - Filtered invalid/wrong claims: 2 issue clusters. | ||
| 10 | - Net actionable backlog: 45 unique issues. | ||
| 11 | |||
| 12 | ## Objectives | ||
| 13 | 1. Remove high-risk runtime failure modes first (worker/fiber death, unrecoverable ingestion stalls). | ||
| 14 | 2. Preserve event-sourcing correctness under failure/retry (projection, receipts, checkpoint ordering). | ||
| 15 | 3. Resolve client-visible correctness regressions (branch/session races, markdown link parsing). | ||
| 16 | 4. Close low-signal or duplicate review noise with explicit disposition. | ||
| 17 | |||
| 18 | ## Non-Goals | ||
| 19 | - No protocol reintroduction for removed `providers.event` channel (invalid comment cluster). | ||
| 20 | - No architecture rewrite beyond targeted hardening and correctness fixes needed to land PR safely. | ||
| 21 | |||
| 22 | ## Execution Order | ||
| 23 | 1. **Phase A (P0): Runtime fault tolerance** | ||
| 24 | 2. **Phase B (P0/P1): Checkpoint consistency + sequencing** | ||
| 25 | 3. **Phase C (P1): Persistence/projection correctness** | ||
| 26 | 4. **Phase D (P1/P2): Runtime/platform hardening** | ||
| 27 | 5. **Phase E (P1/P2): Web/client correctness** | ||
| 28 | 6. **Phase F (P2): Cleanup/typing hygiene + review thread closure** | ||
| 29 | |||
| 30 | --- | ||
| 31 | |||
| 32 | ## Phase A (P0) - Runtime Fault Tolerance | ||
| 33 | |||
| 34 | ### A1) Provider runtime ingestion loop can die permanently on one dispatch error | ||
| 35 | - Issue: | ||
| 36 | - `ProviderRuntimeIngestion` uses `Effect.forever(Queue.take(...).flatMap(processEvent))` without local recovery. A single failure can terminate the worker fiber. | ||
| 37 | - Planned resolution: | ||
| 38 | - Wrap per-item processing with `Effect.catch` (don't use `catchAllCause` as that is a signal for detects which should not be recovered from) and structured logging (`event.type`, `sessionId`, `threadId if known`). | ||
| 39 | - Keep queue consumer alive after failures. | ||
| 40 | - Add a bounded error counter metric/log cadence to avoid log spam. | ||
| 41 | - Target files: | ||
| 42 | - `apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` | ||
| 43 | - `apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts` | ||
| 44 | - Tests: | ||
| 45 | - Add test where first dispatch fails, second event still processes successfully. | ||
| 46 | - Assert worker remains alive and queue continues draining. | ||
| 47 | |||
| 48 | ### A2) Provider command reactor loop can die permanently on provider-side failure | ||
| 49 | - Issue: | ||
| 50 | - `ProviderCommandReactor` queue consumer also runs forever with no protection around `processDomainEvent`. | ||
| 51 | - Planned resolution: | ||
| 52 | - Apply same per-item fault boundary as A1. | ||
| 53 | - Differentiate retryable provider errors vs invariant errors for log level (`warn` vs `error`). | ||
| 54 | - Target files: | ||
| 55 | - `apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` | ||
| 56 | - `apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts` | ||
| 57 | - Tests: | ||
| 58 | - Inject provider service failure for one event and verify subsequent events still execute. | ||
| 59 | |||
| 60 | ### A3) Checkpoint reactor worker can die and halt checkpointing for process lifetime | ||
| 61 | - Issue: | ||
| 62 | - `CheckpointReactor` queue worker has no internal error boundary; checkpoint/git failures can stop all future checkpoint handling. | ||
| 63 | - Planned resolution: | ||
| 64 | - Add per-item recovery with contextual logging. | ||
| 65 | - Keep worker alive for transient checkpoint/git errors. | ||
| 66 | - Optionally emit `thread.activity.append` error activity for visibility when checkpoint operations fail. | ||
| 67 | - Target files: | ||
| 68 | - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` | ||
| 69 | - `apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` | ||
| 70 | - Tests: | ||
| 71 | - Simulate failing `CheckpointStore` call on one event; verify next event still processes. | ||
| 72 | |||
| 73 | ### A4) WS message handler can surface unhandled rejection at runtime | ||
| 74 | - Issue: | ||
| 75 | - `Effect.runPromise(handleMessage(...))` path flagged for potential unhandled rejection. | ||
| 76 | - Planned resolution: | ||
| 77 | - Add explicit `.catch` with structured logging on message processing invocation path. | ||
| 78 | - Ensure one bad message cannot destabilize the WebSocket server. | ||
| 79 | - Target files: | ||
| 80 | - `apps/server/src/wsServer.ts` | ||
| 81 | - `apps/server/src/wsServer.test.ts` | ||
| 82 | - Tests: | ||
| 83 | - Add regression test forcing `handleMessage` error and assert server still accepts/processes subsequent messages. | ||
| 84 | |||
| 85 | --- | ||
| 86 | |||
| 87 | ## Phase B (P0/P1) - Checkpoint Consistency + Sequencing | ||
| 88 | |||
| 89 | ### B1) Revert flow uses forked completion dispatch and can expose stale read model window | ||
| 90 | - Issue: | ||
| 91 | - `thread.revert.complete` dispatch is forked in reactor path, allowing downstream logic to observe stale state during revert completion window. | ||
| 92 | - Planned resolution: | ||
| 93 | - Make revert completion dispatch part of the same serialized effect flow (await it rather than fire-and-forget). | ||
| 94 | - Preserve ordering guarantees: fs restore/provider rollback/delete ref -> completion command -> next dependent work. | ||
| 95 | - Target files: | ||
| 96 | - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` | ||
| 97 | - `apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` | ||
| 98 | - Tests: | ||
| 99 | - Add ordering test ensuring revert completion is visible before subsequent checkpoint-dependent processing. | ||
| 100 | |||
| 101 | ### B2) Turn-count inference can be wrong when positional index diverges from authoritative count | ||
| 102 | - Issue: | ||
| 103 | - `inferCheckpointTurnCountByTurnId` fallback behavior may infer count from position, leading to incorrect revert semantics. | ||
| 104 | - Planned resolution: | ||
| 105 | - Use authoritative checkpoint metadata when available. | ||
| 106 | - Restrict positional fallback to explicit best-effort mode and prevent it from driving destructive operations. | ||
| 107 | - Target files: | ||
| 108 | - `apps/web/src/session-logic.ts` | ||
| 109 | - `apps/web/src/session-logic.test.ts` | ||
| 110 | - Tests: | ||
| 111 | - Add cases where positional order differs from persisted turn count and assert correct count resolution. | ||
| 112 | |||
| 113 | ### B3) Inconsistent input normalization across checkpoint store methods | ||
| 114 | - Issue: | ||
| 115 | - Some `CheckpointStore` methods trim/normalize input while others do not, creating inconsistent ref/cwd behavior. | ||
| 116 | - Planned resolution: | ||
| 117 | - Introduce shared normalization utility and apply uniformly across all public `CheckpointStore` methods. | ||
| 118 | - Keep normalization semantics explicit and covered by tests. | ||
| 119 | - Target files: | ||
| 120 | - `apps/server/src/checkpointing/Layers/CheckpointStore.ts` | ||
| 121 | - existing checkpointing tests (and add targeted method-level tests) | ||
| 122 | - Tests: | ||
| 123 | - Parameterized tests that call all methods with leading/trailing whitespace and assert consistent behavior. | ||
| 124 | |||
| 125 | ### B4) Duplicate checkpoint-ref generation logic across runtime and tests | ||
| 126 | - Issue: | ||
| 127 | - `checkpointRefForThreadTurn` logic is duplicated across multiple files, creating drift risk. | ||
| 128 | - Planned resolution: | ||
| 129 | - Extract a single shared helper module for checkpoint ref generation. | ||
| 130 | - Migrate runtime and test callsites to helper. | ||
| 131 | - Target files: | ||
| 132 | - `apps/server/src/checkpointing/*` or `apps/server/src/orchestration/*` shared helper (new) | ||
| 133 | - `apps/server/src/wsServer.ts` | ||
| 134 | - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` | ||
| 135 | - impacted tests/integration harnesses | ||
| 136 | - Tests: | ||
| 137 | - Add dedicated helper tests for deterministic encoding format and stability. | ||
| 138 | |||
| 139 | --- | ||
| 140 | |||
| 141 | ## Phase C (P1) - Persistence / Projection Correctness | ||
| 142 | |||
| 143 | ### C1) Event append and projection update can diverge on mid-pipeline failure | ||
| 144 | - Issue: | ||
| 145 | - If event append succeeds and projection/update fails, state can become desynchronized. | ||
| 146 | - Planned resolution: | ||
| 147 | - Rework processing boundary so in-memory and persisted projection updates happen in a single coherent success path. | ||
| 148 | - If projection fails, fail command deterministically and record consistent receipt/error path. | ||
| 149 | - Target files: | ||
| 150 | - `apps/server/src/orchestration/Layers/OrchestrationEngine.ts` | ||
| 151 | - `apps/server/src/orchestration/Layers/ProjectionPipeline.ts` | ||
| 152 | - related persistence layer files as needed | ||
| 153 | - Tests: | ||
| 154 | - Simulate projection failure post-append and verify deterministic observable state + receipt behavior. | ||
| 155 | |||
| 156 | ### C2) Engine error handling/receipt semantics are incomplete for non-invariant failures | ||
| 157 | - Issue: | ||
| 158 | - Non-invariant failures may not persist failure receipt consistently, weakening idempotency/retry behavior. | ||
| 159 | - Planned resolution: | ||
| 160 | - Define explicit receipt policy for invariant vs infrastructure failures. | ||
| 161 | - Persist rejected receipt shape with error classification and retryability signal. | ||
| 162 | - Target files: | ||
| 163 | - `apps/server/src/orchestration/Layers/OrchestrationEngine.ts` | ||
| 164 | - `apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts` | ||
| 165 | - `apps/server/src/orchestration/*test.ts` | ||
| 166 | - Tests: | ||
| 167 | - Retry same `commandId` after failure and assert deterministic replay/idempotency behavior. | ||
| 168 | |||
| 169 | ### C3) Projection pipeline fallback can preserve orphaned messages (FK risk) | ||
| 170 | - Issue: | ||
| 171 | - Gap-filling fallback logic can retain messages tied to removed turns. | ||
| 172 | - Planned resolution: | ||
| 173 | - Remove fallback or strictly filter fallback messages by retained turn ids. | ||
| 174 | - Validate FK-safe mutation path. | ||
| 175 | - Target files: | ||
| 176 | - `apps/server/src/orchestration/Layers/ProjectionPipeline.ts` | ||
| 177 | - `apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts` | ||
| 178 | - Tests: | ||
| 179 | - Revert/delete turn scenarios proving no orphaned message rows remain. | ||
| 180 | |||
| 181 | ### C4) Snapshot projector barrier includes projectors not read by snapshot query | ||
| 182 | - Issue: | ||
| 183 | - Barrier can wait for projector state not represented in returned payload, causing sequence mismatch/replay confusion. | ||
| 184 | - Planned resolution: | ||
| 185 | - Align `REQUIRED_SNAPSHOT_PROJECTORS` with actual snapshot query surface. | ||
| 186 | - Add explicit comment/contract for inclusion criteria. | ||
| 187 | - Target files: | ||
| 188 | - `apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts` | ||
| 189 | - `apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts` | ||
| 190 | - Tests: | ||
| 191 | - Validate `snapshotSequence` monotonic correctness against included tables/data only. | ||
| 192 | |||
| 193 | ### C5) NodeSqlite client resource/typing hazards | ||
| 194 | - Issue: | ||
| 195 | - `executeUnprepared` statement lifecycle and return-shape comments indicate leak/type hazards. | ||
| 196 | - Planned resolution: | ||
| 197 | - Wrap prepare/finalize in `Effect.acquireUseRelease`. | ||
| 198 | - Correct return typing contract and caller expectations. | ||
| 199 | - Target files: | ||
| 200 | - `apps/server/src/persistence/NodeSqliteClient.ts` | ||
| 201 | - `apps/server/src/persistence/NodeSqliteClient.test.ts` | ||
| 202 | - Tests: | ||
| 203 | - Resource cleanup test (finalize always invoked). | ||
| 204 | - Return-shape test for `run` paths. | ||
| 205 | |||
| 206 | ### C6) Event metadata defaults created at module load are fragile | ||
| 207 | - Issue: | ||
| 208 | - Shared `defaultMetadata` object pattern risks stale defaults if future event construction forgets overrides. | ||
| 209 | - Planned resolution: | ||
| 210 | - Replace static default object with per-event metadata factory function. | ||
| 211 | - Avoid mutable shared default structures. | ||
| 212 | - Target files: | ||
| 213 | - `apps/server/src/orchestration/decider.ts` | ||
| 214 | - decider/orchestration tests | ||
| 215 | - Tests: | ||
| 216 | - Ensure unique event metadata (`eventId`, timestamps) per decision invocation. | ||
| 217 | |||
| 218 | ### C7) Provider session directory read-modify-write lost update risk | ||
| 219 | - Issue: | ||
| 220 | - Concurrent upserts can clobber fields without transactional/version guard. | ||
| 221 | - Planned resolution: | ||
| 222 | - Introduce transaction or optimistic version check around merge-upsert path. | ||
| 223 | - Target files: | ||
| 224 | - `apps/server/src/provider/Layers/ProviderSessionDirectory.ts` | ||
| 225 | - corresponding test file(s) | ||
| 226 | - Tests: | ||
| 227 | - Concurrency test with competing updates preserving expected merged state. | ||
| 228 | |||
| 229 | --- | ||
| 230 | |||
| 231 | ## Phase D (P1/P2) - Runtime / Platform Hardening | ||
| 232 | |||
| 233 | ### D1) WS upgrade reject path socket safety | ||
| 234 | - Issue: | ||
| 235 | - Potential `EPIPE/ECONNRESET` during reject write path and response flush concerns. | ||
| 236 | - Planned resolution: | ||
| 237 | - Attach socket error listener in reject path and use `socket.end` where appropriate. | ||
| 238 | - Keep behavior deterministic under disconnect-mid-handshake. | ||
| 239 | - Target files: | ||
| 240 | - `apps/server/src/wsServer.ts` | ||
| 241 | - `apps/server/src/wsServer.test.ts` | ||
| 242 | - Tests: | ||
| 243 | - Add handshake-disconnect scenario confirming no crash. | ||
| 244 | |||
| 245 | ### D2) PTY adapter platform selection and lifecycle races | ||
| 246 | - Issue: | ||
| 247 | - Bun+Windows selection can hit `Effect.die`. | ||
| 248 | - `didExit` guard can drop buffered output. | ||
| 249 | - executable-fix flag can be set before successful chmod. | ||
| 250 | - Planned resolution: | ||
| 251 | - Gate Bun PTY path by platform capability. | ||
| 252 | - Adjust output/exit ordering to avoid dropped data. | ||
| 253 | - Move helper-executable flag set after successful chmod. | ||
| 254 | - Target files: | ||
| 255 | - `apps/server/src/ptyAdapter.ts` | ||
| 256 | - `apps/server/src/ptyAdapter.test.ts` | ||
| 257 | - Tests: | ||
| 258 | - Platform selection unit test. | ||
| 259 | - Output-after-exit race regression test. | ||
| 260 | - chmod failure retry behavior test. | ||
| 261 | |||
| 262 | ### D3) Shell PATH probing robustness | ||
| 263 | - Issue: | ||
| 264 | - `-il` can pollute output; fish shell PATH formatting may not be colon-separated. | ||
| 265 | - Planned resolution: | ||
| 266 | - Switch probing flags/commands to deterministic non-interactive mode. | ||
| 267 | - Add fish-specific join handling or fallback validation. | ||
| 268 | - Target files: | ||
| 269 | - `apps/server/src/os-jank.ts` | ||
| 270 | - add/extend tests | ||
| 271 | - Tests: | ||
| 272 | - Parsing tests for bash/zsh/fish simulated outputs. | ||
| 273 | |||
| 274 | ### D4) Effect callback cleanup omissions | ||
| 275 | - Issue: | ||
| 276 | - Missing cleanup function can leak server handles on interruption. | ||
| 277 | - Planned resolution: | ||
| 278 | - Ensure `Effect.callback` paths return cleanup that closes listeners/servers. | ||
| 279 | - Target files: | ||
| 280 | - `apps/server/src/config.ts` | ||
| 281 | - relevant tests | ||
| 282 | - Tests: | ||
| 283 | - Start/interrupt lifecycle test verifies no leaked handles. | ||
| 284 | |||
| 285 | --- | ||
| 286 | |||
| 287 | ## Phase E (P1/P2) - Web / Client Correctness | ||
| 288 | |||
| 289 | ### E1) Pending approvals indicator is non-functional | ||
| 290 | - Issue: | ||
| 291 | - `pendingApprovalByThreadId` map is currently static/empty, so badge never reflects real state. | ||
| 292 | - Planned resolution: | ||
| 293 | - Derive map from authoritative read model activity/pending approval state. | ||
| 294 | - Remove dead map initialization pattern. | ||
| 295 | - Target files: | ||
| 296 | - `apps/web/src/components/Sidebar.tsx` | ||
| 297 | - add sidebar logic tests | ||
| 298 | - Tests: | ||
| 299 | - Given approval activity in thread, badge appears and clears appropriately. | ||
| 300 | |||
| 301 | ### E2) Branch toolbar dual-write can cause flicker/inconsistent session state | ||
| 302 | - Issue: | ||
| 303 | - Local `SET_THREAD_BRANCH` + server command dispatch creates temporary contradictory state. | ||
| 304 | - Planned resolution: | ||
| 305 | - Move to single-source update path: server command + read-model sync. | ||
| 306 | - Keep optimistic UI only if it is reconciled safely. | ||
| 307 | - Target files: | ||
| 308 | - `apps/web/src/components/BranchToolbar.tsx` | ||
| 309 | - `apps/web/src/components/BranchToolbar.logic.test.ts` | ||
| 310 | - Tests: | ||
| 311 | - Ensure no local stale session overwrite when server model update arrives. | ||
| 312 | |||
| 313 | ### E3) Markdown link parser false-positive scheme + double decode | ||
| 314 | - Issue: | ||
| 315 | - `script.ts:10` misclassified as external scheme. | ||
| 316 | - file URL path can be decoded twice. | ||
| 317 | - Planned resolution: | ||
| 318 | - Tighten external scheme detection (`://` or equivalent guard). | ||
| 319 | - Avoid second decode when source was already parsed from file URL. | ||
| 320 | - Target files: | ||
| 321 | - `apps/web/src/markdown-links.ts` | ||
| 322 | - `apps/web/src/markdown-links.test.ts` | ||
| 323 | - Tests: | ||
| 324 | - Add tests for `script.ts:10`, percent-encoded file names, and valid external schemes. | ||
| 325 | |||
| 326 | ### E4) WS transport request/error edge cases | ||
| 327 | - Issue: | ||
| 328 | - Spreading params into body can override reserved keys. | ||
| 329 | - malformed error payload can resolve instead of reject. | ||
| 330 | - Planned resolution: | ||
| 331 | - Normalize request envelope shape to prevent key collisions. | ||
| 332 | - Harden error decoding fallback so promise rejects on any error object. | ||
| 333 | - Target files: | ||
| 334 | - `apps/web/src/wsTransport.ts` | ||
| 335 | - add/extend wsTransport tests | ||
| 336 | - Tests: | ||
| 337 | - Reserved key collision case. | ||
| 338 | - Error object without message still rejects with fallback. | ||
| 339 | |||
| 340 | ### E5) Sidebar width bootstrap from localStorage can default incorrectly | ||
| 341 | - Issue: | ||
| 342 | - `Number(null) === 0` can force unintended min width on first load. | ||
| 343 | - Planned resolution: | ||
| 344 | - Explicit null/empty guard before number parse. | ||
| 345 | - Target files: | ||
| 346 | - `apps/web/src/routes/_chat.$threadId.tsx` | ||
| 347 | - add route/component unit test | ||
| 348 | - Tests: | ||
| 349 | - First-load with missing key preserves default width behavior. | ||
| 350 | |||
| 351 | --- | ||
| 352 | |||
| 353 | ## Phase F (P2) - Cleanup / Typing Hygiene | ||
| 354 | |||
| 355 | ### F1) Symbol-safe string interpolation for error messages | ||
| 356 | - Issue: | ||
| 357 | - Interpolating raw symbol causes TypeError in some error formatting paths. | ||
| 358 | - Planned resolution: | ||
| 359 | - Use `String(cause)`/safe coercion helper consistently. | ||
| 360 | - Target files: | ||
| 361 | - `apps/server/src/orchestration/Errors.ts` | ||
| 362 | - `apps/server/src/provider/Layers/ProviderService.ts` | ||
| 363 | - Tests: | ||
| 364 | - Add symbol/undefined/function cause formatting tests. | ||
| 365 | |||
| 366 | ### F2) Remove redundant union members and dead error classes if truly unused | ||
| 367 | - Issue: | ||
| 368 | - Redundant type entries and potentially dead error classes increase noise. | ||
| 369 | - Planned resolution: | ||
| 370 | - Remove redundant union members. | ||
| 371 | - Either remove dead classes or document retained intent with usage boundary. | ||
| 372 | - Target files: | ||
| 373 | - `apps/server/src/checkpointing/Errors.ts` | ||
| 374 | - related type/tests | ||
| 375 | - Tests: | ||
| 376 | - Type-level and runtime regression checks around error mapping. | ||
| 377 | |||
| 378 | ### F3) Documentation cleanup comments | ||
| 379 | - Issue: | ||
| 380 | - Review flagged temporary marker in docs. | ||
| 381 | - Planned resolution: | ||
| 382 | - Remove transient marker if no longer needed. | ||
| 383 | - Target files: | ||
| 384 | - `AGENTS.md` | ||
| 385 | |||
| 386 | --- | ||
| 387 | |||
| 388 | ## Review Comment Disposition Plan | ||
| 389 | |||
| 390 | ### Duplicate clusters to close with one canonical fix comment | ||
| 391 | 1. `providers.event` restoration requests (same underlying claim). | ||
| 392 | 2. Build script `node scripts/bundle-client.ts` (same claim repeated). | ||
| 393 | 3. CheckpointStore normalization duplicates. | ||
| 394 | 4. `handledTurnStartKeys` growth duplicates. | ||
| 395 | 5. checkpoint-ref helper duplication variants. | ||
| 396 | |||
| 397 | ### Invalid/wrong clusters to close as “no action” | ||
| 398 | 1. Reintroduce `providers.event` channel: | ||
| 399 | - Current contracts/channel surface does not define it. | ||
| 400 | - PR is intentionally orchestration-domain-event based. | ||
| 401 | 2. Build script cannot run `.ts` under Node: | ||
| 402 | - Repository pins modern Node and script executes successfully in current environment. | ||
| 403 | |||
| 404 | --- | ||
| 405 | |||
| 406 | ## Verification and Quality Gates | ||
| 407 | |||
| 408 | ### Required per phase | ||
| 409 | 1. Implement targeted tests for backend behavior changes (no mocking out core business logic). | ||
| 410 | 2. Run affected package test suites (at minimum): | ||
| 411 | - `bun run --cwd apps/server test` | ||
| 412 | - `bun run --cwd apps/web test` | ||
| 413 | 3. Run root lint before marking phase complete: | ||
| 414 | - `bun run lint` | ||
| 415 | |||
| 416 | ### Final pre-merge gate | ||
| 417 | 1. Full lint pass with no new warnings introduced by this work. | ||
| 418 | 2. Server tests pass for orchestration/reactor/checkpoint/persistence packages. | ||
| 419 | 3. Web tests pass for markdown/branch/sidebar/ws transport paths. | ||
| 420 | 4. Resolve or explicitly disposition all active review threads with links to fixes. | ||
| 421 | |||
SPEC.md+12| ... | @@ -527,6 +527,7 @@ Projection consistency rules: | ... | @@ -527,6 +527,7 @@ Projection consistency rules: |
| 527 | 1. `orchestration.getSnapshot` is fully served from projection tables and returns `snapshotSequence: number`. | 527 | 1. `orchestration.getSnapshot` is fully served from projection tables and returns `snapshotSequence: number`. |
| 528 | 2. Snapshot must include `projects[]` from `projection_projects`. | 528 | 2. Snapshot must include `projects[]` from `projection_projects`. |
| 529 | 3. Thread snapshot must include `checkpoints[]` from `projection_checkpoints`: | 529 | 3. Thread snapshot must include `checkpoints[]` from `projection_checkpoints`: |
| 530 | |||
| 530 | - `turnId: TurnId` | 531 | - `turnId: TurnId` |
| 531 | - `completedAt: IsoDateTime` | 532 | - `completedAt: IsoDateTime` |
| 532 | - `status` | 533 | - `status` |
| ... | @@ -534,15 +535,23 @@ Projection consistency rules: | ... | @@ -534,15 +535,23 @@ Projection consistency rules: |
| 534 | - `checkpointRef: CheckpointRef` | 535 | - `checkpointRef: CheckpointRef` |
| 535 | - `assistantMessageId?: MessageId` | 536 | - `assistantMessageId?: MessageId` |
| 536 | - `checkpointTurnCount: number` | 537 | - `checkpointTurnCount: number` |
| 538 | |||
| 537 | 4. Client does not require `listCheckpoints` RPC: | 539 | 4. Client does not require `listCheckpoints` RPC: |
| 540 | |||
| 538 | - checkpoint list comes from snapshot projections | 541 | - checkpoint list comes from snapshot projections |
| 539 | - full diff text comes from `orchestration.getTurnDiff` backed by `checkpoint_diff_blobs` | 542 | - full diff text comes from `orchestration.getTurnDiff` backed by `checkpoint_diff_blobs` |
| 543 | |||
| 540 | 5. Provider session identity is not a client routing key: | 544 | 5. Provider session identity is not a client routing key: |
| 545 | |||
| 541 | - client acts on `ThreadId` | 546 | - client acts on `ThreadId` |
| 542 | - server resolves provider session internally via `projection_thread_sessions` | 547 | - server resolves provider session internally via `projection_thread_sessions` |
| 548 | |||
| 543 | 6. `snapshotSequence` is derived from `projection_state`: | 549 | 6. `snapshotSequence` is derived from `projection_state`: |
| 550 | |||
| 544 | - if snapshot depends on multiple projectors, use the minimum `lastAppliedSequence` across those projectors. | 551 | - if snapshot depends on multiple projectors, use the minimum `lastAppliedSequence` across those projectors. |
| 552 | |||
| 545 | 7. Event subscription handoff contract: | 553 | 7. Event subscription handoff contract: |
| 554 | |||
| 546 | - client performs `getSnapshot` and reads `snapshotSequence` | 555 | - client performs `getSnapshot` and reads `snapshotSequence` |
| 547 | - client subscribes/replays with `fromSequenceExclusive = snapshotSequence` | 556 | - client subscribes/replays with `fromSequenceExclusive = snapshotSequence` |
| 548 | - server guarantees no gap between snapshot visibility and subsequent event stream from that sequence. | 557 | - server guarantees no gap between snapshot visibility and subsequent event stream from that sequence. |
| ... | @@ -557,14 +566,17 @@ Projection consistency rules: | ... | @@ -557,14 +566,17 @@ Projection consistency rules: |
| 557 | ### 7.6 Existing Repository Placement | 566 | ### 7.6 Existing Repository Placement |
| 558 | 567 | ||
| 559 | 1. `ProjectsRepository` | 568 | 1. `ProjectsRepository` |
| 569 | |||
| 560 | - Fits as projection/query access on top of `projection_projects` plus orchestration command dispatch for writes. | 570 | - Fits as projection/query access on top of `projection_projects` plus orchestration command dispatch for writes. |
| 561 | - Must not bypass command->event append path for mutations. | 571 | - Must not bypass command->event append path for mutations. |
| 562 | 572 | ||
| 563 | 2. `CheckpointsRepository` | 573 | 2. `CheckpointsRepository` |
| 574 | |||
| 564 | - Fits as projection/query access on top of `projection_checkpoints` plus `checkpoint_diff_blobs` (or git-service on-demand diff implementation). | 575 | - Fits as projection/query access on top of `projection_checkpoints` plus `checkpoint_diff_blobs` (or git-service on-demand diff implementation). |
| 565 | - Represents the same concept as prior `turn_diff_summary`; canonical naming is `checkpoint`. | 576 | - Represents the same concept as prior `turn_diff_summary`; canonical naming is `checkpoint`. |
| 566 | - Must not be an independent source of truth outside orchestration events. | 577 | - Must not be an independent source of truth outside orchestration events. |
| 567 | 578 | ||
| 568 | 3. `ProviderSessionsRepository` | 579 | 3. `ProviderSessionsRepository` |
| 580 | |||
| 569 | - Fits as server-internal runtime persistence on top of `provider_session_runtime`, with domain-visible state projected into `projection_thread_sessions`. | 581 | - Fits as server-internal runtime persistence on top of `provider_session_runtime`, with domain-visible state projected into `projection_thread_sessions`. |
| 570 | - Not part of client RPC/domain aggregate boundary. | 582 | - Not part of client RPC/domain aggregate boundary. |
apps/web/src/routeTree.gen.ts+70-70| ... | @@ -8,111 +8,111 @@ | ... | @@ -8,111 +8,111 @@ |
| 8 | // You should NOT make any changes in this file as it will be overwritten. | 8 | // You should NOT make any changes in this file as it will be overwritten. |
| 9 | // Additionally, you should also exclude this file from your linter and/or formatter to prevent it from being checked or modified. | 9 | // Additionally, you should also exclude this file from your linter and/or formatter to prevent it from being checked or modified. |
| 10 | 10 | ||
| 11 | import { Route as rootRouteImport } from "./routes/__root"; | 11 | import { Route as rootRouteImport } from './routes/__root' |
| 12 | import { Route as ChatRouteImport } from "./routes/_chat"; | 12 | import { Route as ChatRouteImport } from './routes/_chat' |
| 13 | import { Route as ChatIndexRouteImport } from "./routes/_chat.index"; | 13 | import { Route as ChatIndexRouteImport } from './routes/_chat.index' |
| 14 | import { Route as ChatSettingsRouteImport } from "./routes/_chat.settings"; | 14 | import { Route as ChatSettingsRouteImport } from './routes/_chat.settings' |
| 15 | import { Route as ChatThreadIdRouteImport } from "./routes/_chat.$threadId"; | 15 | import { Route as ChatThreadIdRouteImport } from './routes/_chat.$threadId' |
| 16 | 16 | ||
| 17 | const ChatRoute = ChatRouteImport.update({ | 17 | const ChatRoute = ChatRouteImport.update({ |
| 18 | id: "/_chat", | 18 | id: '/_chat', |
| 19 | getParentRoute: () => rootRouteImport, | 19 | getParentRoute: () => rootRouteImport, |
| 20 | } as any); | 20 | } as any) |
| 21 | const ChatIndexRoute = ChatIndexRouteImport.update({ | 21 | const ChatIndexRoute = ChatIndexRouteImport.update({ |
| 22 | id: "/", | 22 | id: '/', |
| 23 | path: "/", | 23 | path: '/', |
| 24 | getParentRoute: () => ChatRoute, | 24 | getParentRoute: () => ChatRoute, |
| 25 | } as any); | 25 | } as any) |
| 26 | const ChatSettingsRoute = ChatSettingsRouteImport.update({ | 26 | const ChatSettingsRoute = ChatSettingsRouteImport.update({ |
| 27 | id: "/settings", | 27 | id: '/settings', |
| 28 | path: "/settings", | 28 | path: '/settings', |
| 29 | getParentRoute: () => ChatRoute, | 29 | getParentRoute: () => ChatRoute, |
| 30 | } as any); | 30 | } as any) |
| 31 | const ChatThreadIdRoute = ChatThreadIdRouteImport.update({ | 31 | const ChatThreadIdRoute = ChatThreadIdRouteImport.update({ |
| 32 | id: "/$threadId", | 32 | id: '/$threadId', |
| 33 | path: "/$threadId", | 33 | path: '/$threadId', |
| 34 | getParentRoute: () => ChatRoute, | 34 | getParentRoute: () => ChatRoute, |
| 35 | } as any); | 35 | } as any) |
| 36 | 36 | ||
| 37 | export interface FileRoutesByFullPath { | 37 | export interface FileRoutesByFullPath { |
| 38 | "/": typeof ChatIndexRoute; | 38 | '/': typeof ChatIndexRoute |
| 39 | "/$threadId": typeof ChatThreadIdRoute; | 39 | '/$threadId': typeof ChatThreadIdRoute |
| 40 | "/settings": typeof ChatSettingsRoute; | 40 | '/settings': typeof ChatSettingsRoute |
| 41 | } | 41 | } |
| 42 | export interface FileRoutesByTo { | 42 | export interface FileRoutesByTo { |
| 43 | "/$threadId": typeof ChatThreadIdRoute; | 43 | '/$threadId': typeof ChatThreadIdRoute |
| 44 | "/settings": typeof ChatSettingsRoute; | 44 | '/settings': typeof ChatSettingsRoute |
| 45 | "/": typeof ChatIndexRoute; | 45 | '/': typeof ChatIndexRoute |
| 46 | } | 46 | } |
| 47 | export interface FileRoutesById { | 47 | export interface FileRoutesById { |
| 48 | __root__: typeof rootRouteImport; | 48 | __root__: typeof rootRouteImport |
| 49 | "/_chat": typeof ChatRouteWithChildren; | 49 | '/_chat': typeof ChatRouteWithChildren |
| 50 | "/_chat/$threadId": typeof ChatThreadIdRoute; | 50 | '/_chat/$threadId': typeof ChatThreadIdRoute |
| 51 | "/_chat/settings": typeof ChatSettingsRoute; | 51 | '/_chat/settings': typeof ChatSettingsRoute |
| 52 | "/_chat/": typeof ChatIndexRoute; | 52 | '/_chat/': typeof ChatIndexRoute |
| 53 | } | 53 | } |
| 54 | export interface FileRouteTypes { | 54 | export interface FileRouteTypes { |
| 55 | fileRoutesByFullPath: FileRoutesByFullPath; | 55 | fileRoutesByFullPath: FileRoutesByFullPath |
| 56 | fullPaths: "/" | "/$threadId" | "/settings"; | 56 | fullPaths: '/' | '/$threadId' | '/settings' |
| 57 | fileRoutesByTo: FileRoutesByTo; | 57 | fileRoutesByTo: FileRoutesByTo |
| 58 | to: "/$threadId" | "/settings" | "/"; | 58 | to: '/$threadId' | '/settings' | '/' |
| 59 | id: "__root__" | "/_chat" | "/_chat/$threadId" | "/_chat/settings" | "/_chat/"; | 59 | id: '__root__' | '/_chat' | '/_chat/$threadId' | '/_chat/settings' | '/_chat/' |
| 60 | fileRoutesById: FileRoutesById; | 60 | fileRoutesById: FileRoutesById |
| 61 | } | 61 | } |
| 62 | export interface RootRouteChildren { | 62 | export interface RootRouteChildren { |
| 63 | ChatRoute: typeof ChatRouteWithChildren; | 63 | ChatRoute: typeof ChatRouteWithChildren |
| 64 | } | 64 | } |
| 65 | 65 | ||
| 66 | declare module "@tanstack/react-router" { | 66 | declare module '@tanstack/react-router' { |
| 67 | interface FileRoutesByPath { | 67 | interface FileRoutesByPath { |
| 68 | "/_chat": { | 68 | '/_chat': { |
| 69 | id: "/_chat"; | 69 | id: '/_chat' |
| 70 | path: ""; | 70 | path: '' |
| 71 | fullPath: "/"; | 71 | fullPath: '/' |
| 72 | preLoaderRoute: typeof ChatRouteImport; | 72 | preLoaderRoute: typeof ChatRouteImport |
| 73 | parentRoute: typeof rootRouteImport; | 73 | parentRoute: typeof rootRouteImport |
| 74 | }; | 74 | } |
| 75 | "/_chat/": { | 75 | '/_chat/': { |
| 76 | id: "/_chat/"; | 76 | id: '/_chat/' |
| 77 | path: "/"; | 77 | path: '/' |
| 78 | fullPath: "/"; | 78 | fullPath: '/' |
| 79 | preLoaderRoute: typeof ChatIndexRouteImport; | 79 | preLoaderRoute: typeof ChatIndexRouteImport |
| 80 | parentRoute: typeof ChatRoute; | 80 | parentRoute: typeof ChatRoute |
| 81 | }; | 81 | } |
| 82 | "/_chat/settings": { | 82 | '/_chat/settings': { |
| 83 | id: "/_chat/settings"; | 83 | id: '/_chat/settings' |
| 84 | path: "/settings"; | 84 | path: '/settings' |
| 85 | fullPath: "/settings"; | 85 | fullPath: '/settings' |
| 86 | preLoaderRoute: typeof ChatSettingsRouteImport; | 86 | preLoaderRoute: typeof ChatSettingsRouteImport |
| 87 | parentRoute: typeof ChatRoute; | 87 | parentRoute: typeof ChatRoute |
| 88 | }; | 88 | } |
| 89 | "/_chat/$threadId": { | 89 | '/_chat/$threadId': { |
| 90 | id: "/_chat/$threadId"; | 90 | id: '/_chat/$threadId' |
| 91 | path: "/$threadId"; | 91 | path: '/$threadId' |
| 92 | fullPath: "/$threadId"; | 92 | fullPath: '/$threadId' |
| 93 | preLoaderRoute: typeof ChatThreadIdRouteImport; | 93 | preLoaderRoute: typeof ChatThreadIdRouteImport |
| 94 | parentRoute: typeof ChatRoute; | 94 | parentRoute: typeof ChatRoute |
| 95 | }; | 95 | } |
| 96 | } | 96 | } |
| 97 | } | 97 | } |
| 98 | 98 | ||
| 99 | interface ChatRouteChildren { | 99 | interface ChatRouteChildren { |
| 100 | ChatThreadIdRoute: typeof ChatThreadIdRoute; | 100 | ChatThreadIdRoute: typeof ChatThreadIdRoute |
| 101 | ChatSettingsRoute: typeof ChatSettingsRoute; | 101 | ChatSettingsRoute: typeof ChatSettingsRoute |
| 102 | ChatIndexRoute: typeof ChatIndexRoute; | 102 | ChatIndexRoute: typeof ChatIndexRoute |
| 103 | } | 103 | } |
| 104 | 104 | ||
| 105 | const ChatRouteChildren: ChatRouteChildren = { | 105 | const ChatRouteChildren: ChatRouteChildren = { |
| 106 | ChatThreadIdRoute: ChatThreadIdRoute, | 106 | ChatThreadIdRoute: ChatThreadIdRoute, |
| 107 | ChatSettingsRoute: ChatSettingsRoute, | 107 | ChatSettingsRoute: ChatSettingsRoute, |
| 108 | ChatIndexRoute: ChatIndexRoute, | 108 | ChatIndexRoute: ChatIndexRoute, |
| 109 | }; | 109 | } |
| 110 | 110 | ||
| 111 | const ChatRouteWithChildren = ChatRoute._addFileChildren(ChatRouteChildren); | 111 | const ChatRouteWithChildren = ChatRoute._addFileChildren(ChatRouteChildren) |
| 112 | 112 | ||
| 113 | const rootRouteChildren: RootRouteChildren = { | 113 | const rootRouteChildren: RootRouteChildren = { |
| 114 | ChatRoute: ChatRouteWithChildren, | 114 | ChatRoute: ChatRouteWithChildren, |
| 115 | }; | 115 | } |
| 116 | export const routeTree = rootRouteImport | 116 | export const routeTree = rootRouteImport |
| 117 | ._addFileChildren(rootRouteChildren) | 117 | ._addFileChildren(rootRouteChildren) |
| 118 | ._addFileTypes<FileRouteTypes>(); | 118 | ._addFileTypes<FileRouteTypes>() |
apps/web/src/wsTransport.test.ts created+175| ... | @@ -0,0 +1,175 @@ | ||
| 1 | import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| 2 | |||
| 3 | import { WsTransport } from "./wsTransport"; | ||
| 4 | |||
| 5 | type WsEventType = "open" | "message" | "close" | "error"; | ||
| 6 | type WsListener = (event?: { data?: unknown }) => void; | ||
| 7 | |||
| 8 | const sockets: MockWebSocket[] = []; | ||
| 9 | |||
| 10 | class MockWebSocket { | ||
| 11 | static readonly CONNECTING = 0; | ||
| 12 | static readonly OPEN = 1; | ||
| 13 | static readonly CLOSING = 2; | ||
| 14 | static readonly CLOSED = 3; | ||
| 15 | |||
| 16 | readyState = MockWebSocket.CONNECTING; | ||
| 17 | readonly sent: string[] = []; | ||
| 18 | private readonly listeners = new Map<WsEventType, Set<WsListener>>(); | ||
| 19 | |||
| 20 | constructor(_url: string) { | ||
| 21 | sockets.push(this); | ||
| 22 | } | ||
| 23 | |||
| 24 | addEventListener(type: WsEventType, listener: WsListener) { | ||
| 25 | const listeners = this.listeners.get(type) ?? new Set<WsListener>(); | ||
| 26 | listeners.add(listener); | ||
| 27 | this.listeners.set(type, listeners); | ||
| 28 | } | ||
| 29 | |||
| 30 | send(data: string) { | ||
| 31 | this.sent.push(data); | ||
| 32 | } | ||
| 33 | |||
| 34 | close() { | ||
| 35 | this.readyState = MockWebSocket.CLOSED; | ||
| 36 | this.emit("close"); | ||
| 37 | } | ||
| 38 | |||
| 39 | open() { | ||
| 40 | this.readyState = MockWebSocket.OPEN; | ||
| 41 | this.emit("open"); | ||
| 42 | } | ||
| 43 | |||
| 44 | serverMessage(data: unknown) { | ||
| 45 | this.emit("message", { data }); | ||
| 46 | } | ||
| 47 | |||
| 48 | private emit(type: WsEventType, event?: { data?: unknown }) { | ||
| 49 | const listeners = this.listeners.get(type); | ||
| 50 | if (!listeners) return; | ||
| 51 | for (const listener of listeners) { | ||
| 52 | listener(event); | ||
| 53 | } | ||
| 54 | } | ||
| 55 | } | ||
| 56 | |||
| 57 | const originalWebSocket = globalThis.WebSocket; | ||
| 58 | |||
| 59 | function getSocket(): MockWebSocket { | ||
| 60 | const socket = sockets.at(-1); | ||
| 61 | if (!socket) { | ||
| 62 | throw new Error("Expected a websocket instance"); | ||
| 63 | } | ||
| 64 | return socket; | ||
| 65 | } | ||
| 66 | |||
| 67 | beforeEach(() => { | ||
| 68 | sockets.length = 0; | ||
| 69 | |||
| 70 | Object.defineProperty(globalThis, "window", { | ||
| 71 | configurable: true, | ||
| 72 | value: { | ||
| 73 | location: { hostname: "localhost", port: "3020" }, | ||
| 74 | desktopBridge: undefined, | ||
| 75 | }, | ||
| 76 | }); | ||
| 77 | |||
| 78 | globalThis.WebSocket = MockWebSocket as unknown as typeof WebSocket; | ||
| 79 | }); | ||
| 80 | |||
| 81 | afterEach(() => { | ||
| 82 | globalThis.WebSocket = originalWebSocket; | ||
| 83 | vi.restoreAllMocks(); | ||
| 84 | }); | ||
| 85 | |||
| 86 | describe("WsTransport", () => { | ||
| 87 | it("routes valid push envelopes to channel listeners", () => { | ||
| 88 | const transport = new WsTransport("ws://localhost:3020"); | ||
| 89 | const socket = getSocket(); | ||
| 90 | socket.open(); | ||
| 91 | |||
| 92 | const listener = vi.fn(); | ||
| 93 | transport.subscribe("providers.event", listener); | ||
| 94 | |||
| 95 | socket.serverMessage( | ||
| 96 | JSON.stringify({ | ||
| 97 | type: "push", | ||
| 98 | channel: "providers.event", | ||
| 99 | data: { status: "ok" }, | ||
| 100 | }), | ||
| 101 | ); | ||
| 102 | |||
| 103 | expect(listener).toHaveBeenCalledTimes(1); | ||
| 104 | expect(listener).toHaveBeenCalledWith({ status: "ok" }); | ||
| 105 | |||
| 106 | transport.dispose(); | ||
| 107 | }); | ||
| 108 | |||
| 109 | it("resolves pending requests for valid response envelopes", async () => { | ||
| 110 | const transport = new WsTransport("ws://localhost:3020"); | ||
| 111 | const socket = getSocket(); | ||
| 112 | socket.open(); | ||
| 113 | |||
| 114 | const requestPromise = transport.request("projects.list"); | ||
| 115 | const sent = socket.sent.at(-1); | ||
| 116 | if (!sent) { | ||
| 117 | throw new Error("Expected request envelope to be sent"); | ||
| 118 | } | ||
| 119 | |||
| 120 | const requestEnvelope = JSON.parse(sent) as { id: string }; | ||
| 121 | socket.serverMessage( | ||
| 122 | JSON.stringify({ | ||
| 123 | id: requestEnvelope.id, | ||
| 124 | result: { projects: [] }, | ||
| 125 | }), | ||
| 126 | ); | ||
| 127 | |||
| 128 | await expect(requestPromise).resolves.toEqual({ projects: [] }); | ||
| 129 | |||
| 130 | transport.dispose(); | ||
| 131 | }); | ||
| 132 | |||
| 133 | it("drops malformed envelopes without crashing transport", () => { | ||
| 134 | const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||
| 135 | const transport = new WsTransport("ws://localhost:3020"); | ||
| 136 | const socket = getSocket(); | ||
| 137 | socket.open(); | ||
| 138 | |||
| 139 | const listener = vi.fn(); | ||
| 140 | transport.subscribe("providers.event", listener); | ||
| 141 | |||
| 142 | socket.serverMessage("{ invalid-json"); | ||
| 143 | socket.serverMessage( | ||
| 144 | JSON.stringify({ | ||
| 145 | type: "push", | ||
| 146 | channel: 42, | ||
| 147 | data: { bad: true }, | ||
| 148 | }), | ||
| 149 | ); | ||
| 150 | socket.serverMessage( | ||
| 151 | JSON.stringify({ | ||
| 152 | type: "push", | ||
| 153 | channel: "providers.event", | ||
| 154 | data: { ok: true }, | ||
| 155 | }), | ||
| 156 | ); | ||
| 157 | |||
| 158 | expect(listener).toHaveBeenCalledTimes(1); | ||
| 159 | expect(listener).toHaveBeenCalledWith({ ok: true }); | ||
| 160 | expect(warnSpy).toHaveBeenCalledTimes(2); | ||
| 161 | expect(warnSpy).toHaveBeenNthCalledWith(1, "Dropped inbound WebSocket envelope", { | ||
| 162 | reason: "decode-failed", | ||
| 163 | issue: | ||
| 164 | "SchemaError: SyntaxError: Expected property name or '}' in JSON at position 2 (line 1 column 3)", | ||
| 165 | raw: "{ invalid-json", | ||
| 166 | }); | ||
| 167 | expect(warnSpy).toHaveBeenNthCalledWith(2, "Dropped inbound WebSocket envelope", { | ||
| 168 | reason: "decode-failed", | ||
| 169 | issue: expect.stringContaining("SchemaError: Expected string, got 42"), | ||
| 170 | raw: '{"type":"push","channel":42,"data":{"bad":true}}', | ||
| 171 | }); | ||
| 172 | |||
| 173 | transport.dispose(); | ||
| 174 | }); | ||
| 175 | }); | ||
apps/web/src/wsTransport.ts+34-29| ... | @@ -1,4 +1,5 @@ | ... | @@ -1,4 +1,5 @@ |
| 1 | import type { WsPush } from "@t3tools/contracts"; | 1 | import { WebSocketResponse, WsPush, WsResponse, WebSocketRequest } from "@t3tools/contracts"; |
| 2 | import { Cause, Schema, SchemaIssue } from "effect"; | ||
| 2 | 3 | ||
| 3 | type PushListener = (data: unknown) => void; | 4 | type PushListener = (data: unknown) => void; |
| 4 | 5 | ||
| ... | @@ -10,6 +11,14 @@ interface PendingRequest { | ... | @@ -10,6 +11,14 @@ interface PendingRequest { |
| 10 | 11 | ||
| 11 | const REQUEST_TIMEOUT_MS = 60_000; | 12 | const REQUEST_TIMEOUT_MS = 60_000; |
| 12 | const RECONNECT_DELAYS_MS = [500, 1_000, 2_000, 4_000, 8_000]; | 13 | const RECONNECT_DELAYS_MS = [500, 1_000, 2_000, 4_000, 8_000]; |
| 14 | const decodeWsResponseFromJson = Schema.decodeUnknownExit(Schema.fromJsonString(WsResponse)); | ||
| 15 | const isWsPushEnvelope = Schema.is(WsPush); | ||
| 16 | |||
| 17 | function isWebSocketResponseEnvelope( | ||
| 18 | message: typeof WsResponse.Type, | ||
| 19 | ): message is WebSocketResponse { | ||
| 20 | return message instanceof WebSocketResponse; | ||
| 21 | } | ||
| 13 | 22 | ||
| 14 | interface WsRequestEnvelope { | 23 | interface WsRequestEnvelope { |
| 15 | id: string; | 24 | id: string; |
| ... | @@ -127,25 +136,24 @@ export class WsTransport { | ... | @@ -127,25 +136,24 @@ export class WsTransport { |
| 127 | } | 136 | } |
| 128 | 137 | ||
| 129 | private handleMessage(raw: unknown) { | 138 | private handleMessage(raw: unknown) { |
| 130 | let parsed: unknown; | 139 | const exit = decodeWsResponseFromJson(raw); |
| 131 | try { | 140 | if (exit._tag === "Failure") { |
| 132 | parsed = JSON.parse(String(raw)); | 141 | console.warn("Dropped inbound WebSocket envelope", { |
| 133 | } catch { | 142 | reason: "decode-failed", |
| 143 | raw, | ||
| 144 | issue: Cause.pretty(exit.cause), | ||
| 145 | }); | ||
| 134 | return; | 146 | return; |
| 135 | } | 147 | } |
| 136 | 148 | const message = exit.value; | |
| 137 | if (!parsed || typeof parsed !== "object") return; | ||
| 138 | |||
| 139 | const message = parsed as Record<string, unknown>; | ||
| 140 | 149 | ||
| 141 | // Push event | 150 | // Push event |
| 142 | if (message.type === "push") { | 151 | if (isWsPushEnvelope(message)) { |
| 143 | const push = message as unknown as WsPush; | 152 | const channelListeners = this.listeners.get(message.channel); |
| 144 | const channelListeners = this.listeners.get(push.channel); | ||
| 145 | if (channelListeners) { | 153 | if (channelListeners) { |
| 146 | for (const listener of channelListeners) { | 154 | for (const listener of channelListeners) { |
| 147 | try { | 155 | try { |
| 148 | listener(push.data); | 156 | listener(message.data); |
| 149 | } catch { | 157 | } catch { |
| 150 | // Swallow listener errors | 158 | // Swallow listener errors |
| 151 | } | 159 | } |
| ... | @@ -155,23 +163,20 @@ export class WsTransport { | ... | @@ -155,23 +163,20 @@ export class WsTransport { |
| 155 | } | 163 | } |
| 156 | 164 | ||
| 157 | // Response to a request | 165 | // Response to a request |
| 158 | if (typeof message.id === "string") { | 166 | if (!isWebSocketResponseEnvelope(message)) { |
| 159 | const responseId = message.id; | 167 | return; |
| 160 | const pending = this.pending.get(responseId); | 168 | } |
| 161 | if (!pending) return; | ||
| 162 | 169 | ||
| 163 | clearTimeout(pending.timeout); | 170 | const pending = this.pending.get(message.id); |
| 164 | this.pending.delete(responseId); | 171 | if (!pending) return; |
| 165 | 172 | ||
| 166 | if ( | 173 | clearTimeout(pending.timeout); |
| 167 | message.error && | 174 | this.pending.delete(message.id); |
| 168 | typeof message.error === "object" && | 175 | |
| 169 | typeof (message.error as { message?: unknown }).message === "string" | 176 | if (message.error) { |
| 170 | ) { | 177 | pending.reject(new Error(message.error.message)); |
| 171 | pending.reject(new Error((message.error as { message: string }).message)); | 178 | } else { |
| 172 | } else { | 179 | pending.resolve(message.result); |
| 173 | pending.resolve(message.result); | ||
| 174 | } | ||
| 175 | } | 180 | } |
| 176 | } | 181 | } |
| 177 | 182 |