From 320f249a3139b7aaa531c04a4b2bbbdad522c74c Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 25 Feb 2026 13:00:16 -0800 Subject: [PATCH] harden transport --- .oxfmtrc.json | 3 +- ...nd-contract-schema-validation-alignment.md | 182 ++++++++ .../pr89-review-feedback-resolution-plan.md | 421 ++++++++++++++++++ SPEC.md | 12 + apps/web/src/routeTree.gen.ts | 140 +++--- apps/web/src/wsTransport.test.ts | 175 ++++++++ apps/web/src/wsTransport.ts | 61 +-- 7 files changed, 895 insertions(+), 99 deletions(-) create mode 100644 .plans/16-frontend-contract-schema-validation-alignment.md create mode 100644 .plans/pr89-review-feedback-resolution-plan.md create mode 100644 apps/web/src/wsTransport.test.ts diff --git a/.oxfmtrc.json b/.oxfmtrc.json index 8bb9df724136400cb609b09185715d36a8e98432..f0fbfde6d757042c8da2cce0b01315e9e415a498 100644 --- a/.oxfmtrc.json +++ b/.oxfmtrc.json @@ -6,7 +6,8 @@ "dist-electron", "node_modules", "bun.lock", - "*.tsbuildinfo" + "*.tsbuildinfo", + "**/routeTree.gen.ts" ], "experimentalSortPackageJson": {} } diff --git a/.plans/16-frontend-contract-schema-validation-alignment.md b/.plans/16-frontend-contract-schema-validation-alignment.md new file mode 100644 index 0000000000000000000000000000000000000000..5ead8949c4f120e8d399bd3cab5f8c7dde834326 --- /dev/null +++ b/.plans/16-frontend-contract-schema-validation-alignment.md @@ -0,0 +1,182 @@ +# Plan: Frontend Contract Schema Validation Alignment + +## Summary +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. + +## Goals +1. Decode all WebSocket envelopes with shared contract schemas. +2. Decode push payloads before handing them to app logic. +3. Replace duplicated frontend range/input checks with contract schema decoding. +4. Validate project search params with shared constraints before dispatch. +5. Validate script keybinding updates with `KeybindingRule` schema before RPC. +6. Propagate branded IDs (`ThreadId`, `ProjectId`, etc.) through typed app flow after single ingress conversion. + +## Non-Goals +- No protocol shape changes. +- No server behavior changes unless uncovered by failing tests. +- No UI redesign; only validation/transport boundary hardening. + +## Phase 1: WebSocket Envelope Decoding + +### Issue +`apps/web/src/wsTransport.ts` parses JSON and performs manual object/cast checks (`as WsPush`) instead of decoding with contracts. + +### Changes +1. Decode inbound messages with contract schema union (`WsResponse`). +2. Remove manual push/response shape checks that duplicate schema logic. +3. Convert decode failures into safe no-op handling with structured debug logging. + +### Target Files +- `apps/web/src/wsTransport.ts` +- `apps/web/src/wsTransport.test.ts` + +### Tests +1. Accept valid push envelope and route to channel listener. +2. Accept valid response envelope and resolve pending request. +3. Reject malformed envelope without crashing transport. + +## Phase 2: Push Payload Decoding at API Boundary + +### Issue +`apps/web/src/wsNativeApi.ts` forwards unknown push payloads and uses direct casts (`data as WsWelcomePayload`). + +### Changes +1. Decode `server.welcome` payload with `WsWelcomePayload`. +2. Decode `terminal.event` payload with `TerminalEvent`. +3. Decode `orchestration.domainEvent` payload with `OrchestrationEvent`. +4. Skip invalid payloads safely and keep subscriptions alive. + +### Target Files +- `apps/web/src/wsNativeApi.ts` +- `apps/web/src/wsNativeApi.test.ts` + +### Tests +1. Valid welcome payload is delivered and cached. +2. Invalid welcome payload is ignored. +3. Valid terminal/orchestration events are forwarded. +4. Invalid push data does not invoke consumers. + +## Phase 3: Replace Duplicated Checkpoint Range Validation + +### Issue +`apps/web/src/lib/providerReactQuery.ts` duplicates `fromTurnCount`/`toTurnCount` validation rules already represented in contracts. + +### Changes +1. Use `OrchestrationGetTurnDiffInput` / `OrchestrationGetFullThreadDiffInput` decoding before RPC calls. +2. Remove bespoke integer/range checks where schema decoding can be authoritative. +3. Preserve current UX behavior (`enabled` gating + retry policy). + +### Target Files +- `apps/web/src/lib/providerReactQuery.ts` +- `apps/web/src/lib/providerReactQuery.test.ts` + +### Tests +1. Valid range decodes and calls expected RPC. +2. Invalid range fails fast and avoids RPC call. +3. `fromTurnCount === 0` path still routes to `getFullThreadDiff`. + +## Phase 4: Project Search Input Validation from Contracts + +### Issue +`apps/web/src/lib/projectReactQuery.ts` forwards search inputs without enforcing `ProjectSearchEntriesInput` constraints, and `ChatView` can pass empty query strings. + +### Changes +1. Decode project search input with `ProjectSearchEntriesInput` before request dispatch. +2. Gate query execution when query is empty after trim. +3. Replace magic search limit usage with shared constant (`PROJECT_SEARCH_ENTRIES_MAX_LIMIT`) or schema-safe value. + +### Target Files +- `apps/web/src/lib/projectReactQuery.ts` +- `apps/web/src/components/ChatView.tsx` +- `apps/web/src/lib/projectReactQuery.test.ts` + +### Tests +1. Empty query does not dispatch request. +2. Over-limit values are rejected or clamped via schema-safe path. +3. Valid queries still return prior placeholder behavior. + +## Phase 5: Script/Keybinding Validation Consistency + +### Issue +`ProjectScriptsControl` and `ChatView` rely mostly on local non-empty checks while sending keybinding data into a contract-validated endpoint (`server.upsertKeybinding`). + +### Changes +1. Add pre-submit decode for keybinding payload using `KeybindingRule`. +2. Normalize and validate keybinding values via schema before calling `upsertKeybinding`. +3. Keep existing script ID command generation logic, but ensure command + key are schema-valid before mutation. + +### Target Files +- `apps/web/src/components/ProjectScriptsControl.tsx` +- `apps/web/src/components/ChatView.tsx` +- (optional helper) `apps/web/src/projectScripts.ts` +- `apps/web/src/components/ProjectScriptsControl.test.tsx` (or logic test file) + +### Tests +1. Invalid keybinding string surfaces UI validation error and blocks submission. +2. Valid keybinding updates still persist and refresh cached server config. +3. Editing existing script preserves valid keybinding mapping behavior. + +## Phase 6: Branded ID Flow (Single Ingress Branding) + +### Issue +Frontend currently converts plain strings to branded IDs repeatedly (`asThreadId`, `asProjectId`, etc.) at many callsites, which spreads unsafe conversions across app logic. + +### Desired Model +1. Convert/decode to branded IDs once at ingress boundaries. +2. Keep IDs branded through store state, selectors, and command/query builders. +3. Avoid repeated `*.makeUnsafe` wrappers in feature logic. + +### Boundary Rules +1. Router/search params (string) -> decode to branded IDs at route boundary. +2. Storage/network payloads (untyped JSON) -> decode with contract schemas at transport/query boundary. +3. ID generation (`newThreadId`, `newProjectId`, etc.) remains branded at source. +4. UI components should consume typed IDs from store/hooks, not raw strings. + +### Changes +1. Introduce a thin typed ID layer in web app (or reuse contracts decoders directly) for safe `unknown -> branded` conversion at boundaries. +2. Refactor web store and thread/project domain types to carry branded IDs where feasible. +3. Replace repeated `asThreadId`/`asProjectId` callsites in: + - `apps/web/src/components/ChatView.tsx` + - `apps/web/src/components/Sidebar.tsx` + - `apps/web/src/components/BranchToolbar.tsx` + - `apps/web/src/lib/providerReactQuery.ts` +4. Keep unavoidable casts isolated in one boundary module (not feature components). +5. Minimize/remove broad `asXId` helpers from `apps/web/src/lib/orchestrationIds.ts` once flow is typed. + +### Target Files +- `apps/web/src/lib/orchestrationIds.ts` +- `apps/web/src/types.ts` +- `apps/web/src/store.ts` +- `apps/web/src/components/ChatView.tsx` +- `apps/web/src/components/Sidebar.tsx` +- `apps/web/src/components/BranchToolbar.tsx` +- `apps/web/src/lib/providerReactQuery.ts` +- route boundary files (`apps/web/src/routes/*`) as needed + +### Tests +1. Type-level regression checks (compile-time) proving command builders accept branded IDs directly. +2. Route/load path tests verifying invalid ID ingress is rejected/fails safely. +3. Existing behavior tests still pass for thread/project operations after refactor. + +## Cross-Cutting Execution Notes +1. Prefer a small shared frontend utility for Effect schema decoding to avoid repeated `decodeUnknownSync` boilerplate. +2. Use `Schema.decodeUnknownEither` or equivalent non-throwing path where event stream robustness matters. +3. Keep failure behavior fail-soft at stream boundaries (drop invalid events, keep transport running). +4. Brands are TypeScript-level constraints; JSON boundaries still require decode/re-brand at ingress. + +## Validation Checklist +1. `bun run lint` +2. `bun run test --filter=@t3tools/web` (or equivalent web test command in repo) +3. Manual smoke checks: + - App boot + welcome sync + - Terminal stream still renders + - Diff panel loads checkpoint ranges + - Path autocomplete search remains responsive + - Script add/edit with keybinding works in Electron flow + +## Done Criteria +1. No unsafe push/event casts remain at core WebSocket boundaries. +2. Frontend no longer duplicates contract input validation for checkpoint range and project search. +3. Keybinding submission path is schema-validated before RPC. +4. Branded IDs flow through frontend feature code without repeated `asThreadId`/`asProjectId` wrappers. +5. Updated tests cover malformed payload and invalid input regressions. diff --git a/.plans/pr89-review-feedback-resolution-plan.md b/.plans/pr89-review-feedback-resolution-plan.md new file mode 100644 index 0000000000000000000000000000000000000000..5757ee7b14dc3f6a39b2e8bd00354bb90b568447 --- /dev/null +++ b/.plans/pr89-review-feedback-resolution-plan.md @@ -0,0 +1,421 @@ +# PR #89 Review Feedback Resolution Plan + +## Context +- Source: GitHub review threads/comments on PR `#89` (`Add server-side orchestration engine with event sourcing`). +- Triage baseline: + - 147 threads, 149 comments total. + - 54 active unresolved threads. + - Deduped to 47 unique active issues. + - Filtered invalid/wrong claims: 2 issue clusters. + - Net actionable backlog: 45 unique issues. + +## Objectives +1. Remove high-risk runtime failure modes first (worker/fiber death, unrecoverable ingestion stalls). +2. Preserve event-sourcing correctness under failure/retry (projection, receipts, checkpoint ordering). +3. Resolve client-visible correctness regressions (branch/session races, markdown link parsing). +4. Close low-signal or duplicate review noise with explicit disposition. + +## Non-Goals +- No protocol reintroduction for removed `providers.event` channel (invalid comment cluster). +- No architecture rewrite beyond targeted hardening and correctness fixes needed to land PR safely. + +## Execution Order +1. **Phase A (P0): Runtime fault tolerance** +2. **Phase B (P0/P1): Checkpoint consistency + sequencing** +3. **Phase C (P1): Persistence/projection correctness** +4. **Phase D (P1/P2): Runtime/platform hardening** +5. **Phase E (P1/P2): Web/client correctness** +6. **Phase F (P2): Cleanup/typing hygiene + review thread closure** + +--- + +## Phase A (P0) - Runtime Fault Tolerance + +### A1) Provider runtime ingestion loop can die permanently on one dispatch error +- Issue: + - `ProviderRuntimeIngestion` uses `Effect.forever(Queue.take(...).flatMap(processEvent))` without local recovery. A single failure can terminate the worker fiber. +- Planned resolution: + - 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`). + - Keep queue consumer alive after failures. + - Add a bounded error counter metric/log cadence to avoid log spam. +- Target files: + - `apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` + - `apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts` +- Tests: + - Add test where first dispatch fails, second event still processes successfully. + - Assert worker remains alive and queue continues draining. + +### A2) Provider command reactor loop can die permanently on provider-side failure +- Issue: + - `ProviderCommandReactor` queue consumer also runs forever with no protection around `processDomainEvent`. +- Planned resolution: + - Apply same per-item fault boundary as A1. + - Differentiate retryable provider errors vs invariant errors for log level (`warn` vs `error`). +- Target files: + - `apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` + - `apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts` +- Tests: + - Inject provider service failure for one event and verify subsequent events still execute. + +### A3) Checkpoint reactor worker can die and halt checkpointing for process lifetime +- Issue: + - `CheckpointReactor` queue worker has no internal error boundary; checkpoint/git failures can stop all future checkpoint handling. +- Planned resolution: + - Add per-item recovery with contextual logging. + - Keep worker alive for transient checkpoint/git errors. + - Optionally emit `thread.activity.append` error activity for visibility when checkpoint operations fail. +- Target files: + - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` + - `apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` +- Tests: + - Simulate failing `CheckpointStore` call on one event; verify next event still processes. + +### A4) WS message handler can surface unhandled rejection at runtime +- Issue: + - `Effect.runPromise(handleMessage(...))` path flagged for potential unhandled rejection. +- Planned resolution: + - Add explicit `.catch` with structured logging on message processing invocation path. + - Ensure one bad message cannot destabilize the WebSocket server. +- Target files: + - `apps/server/src/wsServer.ts` + - `apps/server/src/wsServer.test.ts` +- Tests: + - Add regression test forcing `handleMessage` error and assert server still accepts/processes subsequent messages. + +--- + +## Phase B (P0/P1) - Checkpoint Consistency + Sequencing + +### B1) Revert flow uses forked completion dispatch and can expose stale read model window +- Issue: + - `thread.revert.complete` dispatch is forked in reactor path, allowing downstream logic to observe stale state during revert completion window. +- Planned resolution: + - Make revert completion dispatch part of the same serialized effect flow (await it rather than fire-and-forget). + - Preserve ordering guarantees: fs restore/provider rollback/delete ref -> completion command -> next dependent work. +- Target files: + - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` + - `apps/server/src/orchestration/Layers/CheckpointReactor.test.ts` +- Tests: + - Add ordering test ensuring revert completion is visible before subsequent checkpoint-dependent processing. + +### B2) Turn-count inference can be wrong when positional index diverges from authoritative count +- Issue: + - `inferCheckpointTurnCountByTurnId` fallback behavior may infer count from position, leading to incorrect revert semantics. +- Planned resolution: + - Use authoritative checkpoint metadata when available. + - Restrict positional fallback to explicit best-effort mode and prevent it from driving destructive operations. +- Target files: + - `apps/web/src/session-logic.ts` + - `apps/web/src/session-logic.test.ts` +- Tests: + - Add cases where positional order differs from persisted turn count and assert correct count resolution. + +### B3) Inconsistent input normalization across checkpoint store methods +- Issue: + - Some `CheckpointStore` methods trim/normalize input while others do not, creating inconsistent ref/cwd behavior. +- Planned resolution: + - Introduce shared normalization utility and apply uniformly across all public `CheckpointStore` methods. + - Keep normalization semantics explicit and covered by tests. +- Target files: + - `apps/server/src/checkpointing/Layers/CheckpointStore.ts` + - existing checkpointing tests (and add targeted method-level tests) +- Tests: + - Parameterized tests that call all methods with leading/trailing whitespace and assert consistent behavior. + +### B4) Duplicate checkpoint-ref generation logic across runtime and tests +- Issue: + - `checkpointRefForThreadTurn` logic is duplicated across multiple files, creating drift risk. +- Planned resolution: + - Extract a single shared helper module for checkpoint ref generation. + - Migrate runtime and test callsites to helper. +- Target files: + - `apps/server/src/checkpointing/*` or `apps/server/src/orchestration/*` shared helper (new) + - `apps/server/src/wsServer.ts` + - `apps/server/src/orchestration/Layers/CheckpointReactor.ts` + - impacted tests/integration harnesses +- Tests: + - Add dedicated helper tests for deterministic encoding format and stability. + +--- + +## Phase C (P1) - Persistence / Projection Correctness + +### C1) Event append and projection update can diverge on mid-pipeline failure +- Issue: + - If event append succeeds and projection/update fails, state can become desynchronized. +- Planned resolution: + - Rework processing boundary so in-memory and persisted projection updates happen in a single coherent success path. + - If projection fails, fail command deterministically and record consistent receipt/error path. +- Target files: + - `apps/server/src/orchestration/Layers/OrchestrationEngine.ts` + - `apps/server/src/orchestration/Layers/ProjectionPipeline.ts` + - related persistence layer files as needed +- Tests: + - Simulate projection failure post-append and verify deterministic observable state + receipt behavior. + +### C2) Engine error handling/receipt semantics are incomplete for non-invariant failures +- Issue: + - Non-invariant failures may not persist failure receipt consistently, weakening idempotency/retry behavior. +- Planned resolution: + - Define explicit receipt policy for invariant vs infrastructure failures. + - Persist rejected receipt shape with error classification and retryability signal. +- Target files: + - `apps/server/src/orchestration/Layers/OrchestrationEngine.ts` + - `apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts` + - `apps/server/src/orchestration/*test.ts` +- Tests: + - Retry same `commandId` after failure and assert deterministic replay/idempotency behavior. + +### C3) Projection pipeline fallback can preserve orphaned messages (FK risk) +- Issue: + - Gap-filling fallback logic can retain messages tied to removed turns. +- Planned resolution: + - Remove fallback or strictly filter fallback messages by retained turn ids. + - Validate FK-safe mutation path. +- Target files: + - `apps/server/src/orchestration/Layers/ProjectionPipeline.ts` + - `apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts` +- Tests: + - Revert/delete turn scenarios proving no orphaned message rows remain. + +### C4) Snapshot projector barrier includes projectors not read by snapshot query +- Issue: + - Barrier can wait for projector state not represented in returned payload, causing sequence mismatch/replay confusion. +- Planned resolution: + - Align `REQUIRED_SNAPSHOT_PROJECTORS` with actual snapshot query surface. + - Add explicit comment/contract for inclusion criteria. +- Target files: + - `apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts` + - `apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts` +- Tests: + - Validate `snapshotSequence` monotonic correctness against included tables/data only. + +### C5) NodeSqlite client resource/typing hazards +- Issue: + - `executeUnprepared` statement lifecycle and return-shape comments indicate leak/type hazards. +- Planned resolution: + - Wrap prepare/finalize in `Effect.acquireUseRelease`. + - Correct return typing contract and caller expectations. +- Target files: + - `apps/server/src/persistence/NodeSqliteClient.ts` + - `apps/server/src/persistence/NodeSqliteClient.test.ts` +- Tests: + - Resource cleanup test (finalize always invoked). + - Return-shape test for `run` paths. + +### C6) Event metadata defaults created at module load are fragile +- Issue: + - Shared `defaultMetadata` object pattern risks stale defaults if future event construction forgets overrides. +- Planned resolution: + - Replace static default object with per-event metadata factory function. + - Avoid mutable shared default structures. +- Target files: + - `apps/server/src/orchestration/decider.ts` + - decider/orchestration tests +- Tests: + - Ensure unique event metadata (`eventId`, timestamps) per decision invocation. + +### C7) Provider session directory read-modify-write lost update risk +- Issue: + - Concurrent upserts can clobber fields without transactional/version guard. +- Planned resolution: + - Introduce transaction or optimistic version check around merge-upsert path. +- Target files: + - `apps/server/src/provider/Layers/ProviderSessionDirectory.ts` + - corresponding test file(s) +- Tests: + - Concurrency test with competing updates preserving expected merged state. + +--- + +## Phase D (P1/P2) - Runtime / Platform Hardening + +### D1) WS upgrade reject path socket safety +- Issue: + - Potential `EPIPE/ECONNRESET` during reject write path and response flush concerns. +- Planned resolution: + - Attach socket error listener in reject path and use `socket.end` where appropriate. + - Keep behavior deterministic under disconnect-mid-handshake. +- Target files: + - `apps/server/src/wsServer.ts` + - `apps/server/src/wsServer.test.ts` +- Tests: + - Add handshake-disconnect scenario confirming no crash. + +### D2) PTY adapter platform selection and lifecycle races +- Issue: + - Bun+Windows selection can hit `Effect.die`. + - `didExit` guard can drop buffered output. + - executable-fix flag can be set before successful chmod. +- Planned resolution: + - Gate Bun PTY path by platform capability. + - Adjust output/exit ordering to avoid dropped data. + - Move helper-executable flag set after successful chmod. +- Target files: + - `apps/server/src/ptyAdapter.ts` + - `apps/server/src/ptyAdapter.test.ts` +- Tests: + - Platform selection unit test. + - Output-after-exit race regression test. + - chmod failure retry behavior test. + +### D3) Shell PATH probing robustness +- Issue: + - `-il` can pollute output; fish shell PATH formatting may not be colon-separated. +- Planned resolution: + - Switch probing flags/commands to deterministic non-interactive mode. + - Add fish-specific join handling or fallback validation. +- Target files: + - `apps/server/src/os-jank.ts` + - add/extend tests +- Tests: + - Parsing tests for bash/zsh/fish simulated outputs. + +### D4) Effect callback cleanup omissions +- Issue: + - Missing cleanup function can leak server handles on interruption. +- Planned resolution: + - Ensure `Effect.callback` paths return cleanup that closes listeners/servers. +- Target files: + - `apps/server/src/config.ts` + - relevant tests +- Tests: + - Start/interrupt lifecycle test verifies no leaked handles. + +--- + +## Phase E (P1/P2) - Web / Client Correctness + +### E1) Pending approvals indicator is non-functional +- Issue: + - `pendingApprovalByThreadId` map is currently static/empty, so badge never reflects real state. +- Planned resolution: + - Derive map from authoritative read model activity/pending approval state. + - Remove dead map initialization pattern. +- Target files: + - `apps/web/src/components/Sidebar.tsx` + - add sidebar logic tests +- Tests: + - Given approval activity in thread, badge appears and clears appropriately. + +### E2) Branch toolbar dual-write can cause flicker/inconsistent session state +- Issue: + - Local `SET_THREAD_BRANCH` + server command dispatch creates temporary contradictory state. +- Planned resolution: + - Move to single-source update path: server command + read-model sync. + - Keep optimistic UI only if it is reconciled safely. +- Target files: + - `apps/web/src/components/BranchToolbar.tsx` + - `apps/web/src/components/BranchToolbar.logic.test.ts` +- Tests: + - Ensure no local stale session overwrite when server model update arrives. + +### E3) Markdown link parser false-positive scheme + double decode +- Issue: + - `script.ts:10` misclassified as external scheme. + - file URL path can be decoded twice. +- Planned resolution: + - Tighten external scheme detection (`://` or equivalent guard). + - Avoid second decode when source was already parsed from file URL. +- Target files: + - `apps/web/src/markdown-links.ts` + - `apps/web/src/markdown-links.test.ts` +- Tests: + - Add tests for `script.ts:10`, percent-encoded file names, and valid external schemes. + +### E4) WS transport request/error edge cases +- Issue: + - Spreading params into body can override reserved keys. + - malformed error payload can resolve instead of reject. +- Planned resolution: + - Normalize request envelope shape to prevent key collisions. + - Harden error decoding fallback so promise rejects on any error object. +- Target files: + - `apps/web/src/wsTransport.ts` + - add/extend wsTransport tests +- Tests: + - Reserved key collision case. + - Error object without message still rejects with fallback. + +### E5) Sidebar width bootstrap from localStorage can default incorrectly +- Issue: + - `Number(null) === 0` can force unintended min width on first load. +- Planned resolution: + - Explicit null/empty guard before number parse. +- Target files: + - `apps/web/src/routes/_chat.$threadId.tsx` + - add route/component unit test +- Tests: + - First-load with missing key preserves default width behavior. + +--- + +## Phase F (P2) - Cleanup / Typing Hygiene + +### F1) Symbol-safe string interpolation for error messages +- Issue: + - Interpolating raw symbol causes TypeError in some error formatting paths. +- Planned resolution: + - Use `String(cause)`/safe coercion helper consistently. +- Target files: + - `apps/server/src/orchestration/Errors.ts` + - `apps/server/src/provider/Layers/ProviderService.ts` +- Tests: + - Add symbol/undefined/function cause formatting tests. + +### F2) Remove redundant union members and dead error classes if truly unused +- Issue: + - Redundant type entries and potentially dead error classes increase noise. +- Planned resolution: + - Remove redundant union members. + - Either remove dead classes or document retained intent with usage boundary. +- Target files: + - `apps/server/src/checkpointing/Errors.ts` + - related type/tests +- Tests: + - Type-level and runtime regression checks around error mapping. + +### F3) Documentation cleanup comments +- Issue: + - Review flagged temporary marker in docs. +- Planned resolution: + - Remove transient marker if no longer needed. +- Target files: + - `AGENTS.md` + +--- + +## Review Comment Disposition Plan + +### Duplicate clusters to close with one canonical fix comment +1. `providers.event` restoration requests (same underlying claim). +2. Build script `node scripts/bundle-client.ts` (same claim repeated). +3. CheckpointStore normalization duplicates. +4. `handledTurnStartKeys` growth duplicates. +5. checkpoint-ref helper duplication variants. + +### Invalid/wrong clusters to close as “no action” +1. Reintroduce `providers.event` channel: + - Current contracts/channel surface does not define it. + - PR is intentionally orchestration-domain-event based. +2. Build script cannot run `.ts` under Node: + - Repository pins modern Node and script executes successfully in current environment. + +--- + +## Verification and Quality Gates + +### Required per phase +1. Implement targeted tests for backend behavior changes (no mocking out core business logic). +2. Run affected package test suites (at minimum): + - `bun run --cwd apps/server test` + - `bun run --cwd apps/web test` +3. Run root lint before marking phase complete: + - `bun run lint` + +### Final pre-merge gate +1. Full lint pass with no new warnings introduced by this work. +2. Server tests pass for orchestration/reactor/checkpoint/persistence packages. +3. Web tests pass for markdown/branch/sidebar/ws transport paths. +4. Resolve or explicitly disposition all active review threads with links to fixes. + diff --git a/SPEC.md b/SPEC.md index 42881e29800e49c45c6e39e078733e2efea3fccd..175a3f552807c8f17aa9229aef38d2f636611783 100644 --- a/SPEC.md +++ b/SPEC.md @@ -527,6 +527,7 @@ Projection consistency rules: 1. `orchestration.getSnapshot` is fully served from projection tables and returns `snapshotSequence: number`. 2. Snapshot must include `projects[]` from `projection_projects`. 3. Thread snapshot must include `checkpoints[]` from `projection_checkpoints`: + - `turnId: TurnId` - `completedAt: IsoDateTime` - `status` @@ -534,15 +535,23 @@ Projection consistency rules: - `checkpointRef: CheckpointRef` - `assistantMessageId?: MessageId` - `checkpointTurnCount: number` + 4. Client does not require `listCheckpoints` RPC: + - checkpoint list comes from snapshot projections - full diff text comes from `orchestration.getTurnDiff` backed by `checkpoint_diff_blobs` + 5. Provider session identity is not a client routing key: + - client acts on `ThreadId` - server resolves provider session internally via `projection_thread_sessions` + 6. `snapshotSequence` is derived from `projection_state`: + - if snapshot depends on multiple projectors, use the minimum `lastAppliedSequence` across those projectors. + 7. Event subscription handoff contract: + - client performs `getSnapshot` and reads `snapshotSequence` - client subscribes/replays with `fromSequenceExclusive = snapshotSequence` - server guarantees no gap between snapshot visibility and subsequent event stream from that sequence. @@ -557,14 +566,17 @@ Projection consistency rules: ### 7.6 Existing Repository Placement 1. `ProjectsRepository` + - Fits as projection/query access on top of `projection_projects` plus orchestration command dispatch for writes. - Must not bypass command->event append path for mutations. 2. `CheckpointsRepository` + - Fits as projection/query access on top of `projection_checkpoints` plus `checkpoint_diff_blobs` (or git-service on-demand diff implementation). - Represents the same concept as prior `turn_diff_summary`; canonical naming is `checkpoint`. - Must not be an independent source of truth outside orchestration events. 3. `ProviderSessionsRepository` + - Fits as server-internal runtime persistence on top of `provider_session_runtime`, with domain-visible state projected into `projection_thread_sessions`. - Not part of client RPC/domain aggregate boundary. diff --git a/apps/web/src/routeTree.gen.ts b/apps/web/src/routeTree.gen.ts index 20f8541ae635ba6d9c1d20ace937eced7933c69c..880d5ef64ba441142945fdfe91d85a9e897d7dbe 100644 --- a/apps/web/src/routeTree.gen.ts +++ b/apps/web/src/routeTree.gen.ts @@ -8,111 +8,111 @@ // You should NOT make any changes in this file as it will be overwritten. // Additionally, you should also exclude this file from your linter and/or formatter to prevent it from being checked or modified. -import { Route as rootRouteImport } from "./routes/__root"; -import { Route as ChatRouteImport } from "./routes/_chat"; -import { Route as ChatIndexRouteImport } from "./routes/_chat.index"; -import { Route as ChatSettingsRouteImport } from "./routes/_chat.settings"; -import { Route as ChatThreadIdRouteImport } from "./routes/_chat.$threadId"; +import { Route as rootRouteImport } from './routes/__root' +import { Route as ChatRouteImport } from './routes/_chat' +import { Route as ChatIndexRouteImport } from './routes/_chat.index' +import { Route as ChatSettingsRouteImport } from './routes/_chat.settings' +import { Route as ChatThreadIdRouteImport } from './routes/_chat.$threadId' const ChatRoute = ChatRouteImport.update({ - id: "/_chat", + id: '/_chat', getParentRoute: () => rootRouteImport, -} as any); +} as any) const ChatIndexRoute = ChatIndexRouteImport.update({ - id: "/", - path: "/", + id: '/', + path: '/', getParentRoute: () => ChatRoute, -} as any); +} as any) const ChatSettingsRoute = ChatSettingsRouteImport.update({ - id: "/settings", - path: "/settings", + id: '/settings', + path: '/settings', getParentRoute: () => ChatRoute, -} as any); +} as any) const ChatThreadIdRoute = ChatThreadIdRouteImport.update({ - id: "/$threadId", - path: "/$threadId", + id: '/$threadId', + path: '/$threadId', getParentRoute: () => ChatRoute, -} as any); +} as any) export interface FileRoutesByFullPath { - "/": typeof ChatIndexRoute; - "/$threadId": typeof ChatThreadIdRoute; - "/settings": typeof ChatSettingsRoute; + '/': typeof ChatIndexRoute + '/$threadId': typeof ChatThreadIdRoute + '/settings': typeof ChatSettingsRoute } export interface FileRoutesByTo { - "/$threadId": typeof ChatThreadIdRoute; - "/settings": typeof ChatSettingsRoute; - "/": typeof ChatIndexRoute; + '/$threadId': typeof ChatThreadIdRoute + '/settings': typeof ChatSettingsRoute + '/': typeof ChatIndexRoute } export interface FileRoutesById { - __root__: typeof rootRouteImport; - "/_chat": typeof ChatRouteWithChildren; - "/_chat/$threadId": typeof ChatThreadIdRoute; - "/_chat/settings": typeof ChatSettingsRoute; - "/_chat/": typeof ChatIndexRoute; + __root__: typeof rootRouteImport + '/_chat': typeof ChatRouteWithChildren + '/_chat/$threadId': typeof ChatThreadIdRoute + '/_chat/settings': typeof ChatSettingsRoute + '/_chat/': typeof ChatIndexRoute } export interface FileRouteTypes { - fileRoutesByFullPath: FileRoutesByFullPath; - fullPaths: "/" | "/$threadId" | "/settings"; - fileRoutesByTo: FileRoutesByTo; - to: "/$threadId" | "/settings" | "/"; - id: "__root__" | "/_chat" | "/_chat/$threadId" | "/_chat/settings" | "/_chat/"; - fileRoutesById: FileRoutesById; + fileRoutesByFullPath: FileRoutesByFullPath + fullPaths: '/' | '/$threadId' | '/settings' + fileRoutesByTo: FileRoutesByTo + to: '/$threadId' | '/settings' | '/' + id: '__root__' | '/_chat' | '/_chat/$threadId' | '/_chat/settings' | '/_chat/' + fileRoutesById: FileRoutesById } export interface RootRouteChildren { - ChatRoute: typeof ChatRouteWithChildren; + ChatRoute: typeof ChatRouteWithChildren } -declare module "@tanstack/react-router" { +declare module '@tanstack/react-router' { interface FileRoutesByPath { - "/_chat": { - id: "/_chat"; - path: ""; - fullPath: "/"; - preLoaderRoute: typeof ChatRouteImport; - parentRoute: typeof rootRouteImport; - }; - "/_chat/": { - id: "/_chat/"; - path: "/"; - fullPath: "/"; - preLoaderRoute: typeof ChatIndexRouteImport; - parentRoute: typeof ChatRoute; - }; - "/_chat/settings": { - id: "/_chat/settings"; - path: "/settings"; - fullPath: "/settings"; - preLoaderRoute: typeof ChatSettingsRouteImport; - parentRoute: typeof ChatRoute; - }; - "/_chat/$threadId": { - id: "/_chat/$threadId"; - path: "/$threadId"; - fullPath: "/$threadId"; - preLoaderRoute: typeof ChatThreadIdRouteImport; - parentRoute: typeof ChatRoute; - }; + '/_chat': { + id: '/_chat' + path: '' + fullPath: '/' + preLoaderRoute: typeof ChatRouteImport + parentRoute: typeof rootRouteImport + } + '/_chat/': { + id: '/_chat/' + path: '/' + fullPath: '/' + preLoaderRoute: typeof ChatIndexRouteImport + parentRoute: typeof ChatRoute + } + '/_chat/settings': { + id: '/_chat/settings' + path: '/settings' + fullPath: '/settings' + preLoaderRoute: typeof ChatSettingsRouteImport + parentRoute: typeof ChatRoute + } + '/_chat/$threadId': { + id: '/_chat/$threadId' + path: '/$threadId' + fullPath: '/$threadId' + preLoaderRoute: typeof ChatThreadIdRouteImport + parentRoute: typeof ChatRoute + } } } interface ChatRouteChildren { - ChatThreadIdRoute: typeof ChatThreadIdRoute; - ChatSettingsRoute: typeof ChatSettingsRoute; - ChatIndexRoute: typeof ChatIndexRoute; + ChatThreadIdRoute: typeof ChatThreadIdRoute + ChatSettingsRoute: typeof ChatSettingsRoute + ChatIndexRoute: typeof ChatIndexRoute } const ChatRouteChildren: ChatRouteChildren = { ChatThreadIdRoute: ChatThreadIdRoute, ChatSettingsRoute: ChatSettingsRoute, ChatIndexRoute: ChatIndexRoute, -}; +} -const ChatRouteWithChildren = ChatRoute._addFileChildren(ChatRouteChildren); +const ChatRouteWithChildren = ChatRoute._addFileChildren(ChatRouteChildren) const rootRouteChildren: RootRouteChildren = { ChatRoute: ChatRouteWithChildren, -}; +} export const routeTree = rootRouteImport ._addFileChildren(rootRouteChildren) - ._addFileTypes(); + ._addFileTypes() diff --git a/apps/web/src/wsTransport.test.ts b/apps/web/src/wsTransport.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..e47073fc21c09cbe104dca3a42a85ffcf78576a1 --- /dev/null +++ b/apps/web/src/wsTransport.test.ts @@ -0,0 +1,175 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { WsTransport } from "./wsTransport"; + +type WsEventType = "open" | "message" | "close" | "error"; +type WsListener = (event?: { data?: unknown }) => void; + +const sockets: MockWebSocket[] = []; + +class MockWebSocket { + static readonly CONNECTING = 0; + static readonly OPEN = 1; + static readonly CLOSING = 2; + static readonly CLOSED = 3; + + readyState = MockWebSocket.CONNECTING; + readonly sent: string[] = []; + private readonly listeners = new Map>(); + + constructor(_url: string) { + sockets.push(this); + } + + addEventListener(type: WsEventType, listener: WsListener) { + const listeners = this.listeners.get(type) ?? new Set(); + listeners.add(listener); + this.listeners.set(type, listeners); + } + + send(data: string) { + this.sent.push(data); + } + + close() { + this.readyState = MockWebSocket.CLOSED; + this.emit("close"); + } + + open() { + this.readyState = MockWebSocket.OPEN; + this.emit("open"); + } + + serverMessage(data: unknown) { + this.emit("message", { data }); + } + + private emit(type: WsEventType, event?: { data?: unknown }) { + const listeners = this.listeners.get(type); + if (!listeners) return; + for (const listener of listeners) { + listener(event); + } + } +} + +const originalWebSocket = globalThis.WebSocket; + +function getSocket(): MockWebSocket { + const socket = sockets.at(-1); + if (!socket) { + throw new Error("Expected a websocket instance"); + } + return socket; +} + +beforeEach(() => { + sockets.length = 0; + + Object.defineProperty(globalThis, "window", { + configurable: true, + value: { + location: { hostname: "localhost", port: "3020" }, + desktopBridge: undefined, + }, + }); + + globalThis.WebSocket = MockWebSocket as unknown as typeof WebSocket; +}); + +afterEach(() => { + globalThis.WebSocket = originalWebSocket; + vi.restoreAllMocks(); +}); + +describe("WsTransport", () => { + it("routes valid push envelopes to channel listeners", () => { + const transport = new WsTransport("ws://localhost:3020"); + const socket = getSocket(); + socket.open(); + + const listener = vi.fn(); + transport.subscribe("providers.event", listener); + + socket.serverMessage( + JSON.stringify({ + type: "push", + channel: "providers.event", + data: { status: "ok" }, + }), + ); + + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({ status: "ok" }); + + transport.dispose(); + }); + + it("resolves pending requests for valid response envelopes", async () => { + const transport = new WsTransport("ws://localhost:3020"); + const socket = getSocket(); + socket.open(); + + const requestPromise = transport.request("projects.list"); + const sent = socket.sent.at(-1); + if (!sent) { + throw new Error("Expected request envelope to be sent"); + } + + const requestEnvelope = JSON.parse(sent) as { id: string }; + socket.serverMessage( + JSON.stringify({ + id: requestEnvelope.id, + result: { projects: [] }, + }), + ); + + await expect(requestPromise).resolves.toEqual({ projects: [] }); + + transport.dispose(); + }); + + it("drops malformed envelopes without crashing transport", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const transport = new WsTransport("ws://localhost:3020"); + const socket = getSocket(); + socket.open(); + + const listener = vi.fn(); + transport.subscribe("providers.event", listener); + + socket.serverMessage("{ invalid-json"); + socket.serverMessage( + JSON.stringify({ + type: "push", + channel: 42, + data: { bad: true }, + }), + ); + socket.serverMessage( + JSON.stringify({ + type: "push", + channel: "providers.event", + data: { ok: true }, + }), + ); + + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({ ok: true }); + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy).toHaveBeenNthCalledWith(1, "Dropped inbound WebSocket envelope", { + reason: "decode-failed", + issue: + "SchemaError: SyntaxError: Expected property name or '}' in JSON at position 2 (line 1 column 3)", + raw: "{ invalid-json", + }); + expect(warnSpy).toHaveBeenNthCalledWith(2, "Dropped inbound WebSocket envelope", { + reason: "decode-failed", + issue: expect.stringContaining("SchemaError: Expected string, got 42"), + raw: '{"type":"push","channel":42,"data":{"bad":true}}', + }); + + transport.dispose(); + }); +}); diff --git a/apps/web/src/wsTransport.ts b/apps/web/src/wsTransport.ts index de05dca98fceb72b7949cedd6a7a1ad4d7ecae10..8911d3598fbbec80556764e818545675061a90f6 100644 --- a/apps/web/src/wsTransport.ts +++ b/apps/web/src/wsTransport.ts @@ -1,4 +1,5 @@ -import type { WsPush } from "@t3tools/contracts"; +import { WebSocketResponse, WsPush, WsResponse, WebSocketRequest } from "@t3tools/contracts"; +import { Cause, Schema, SchemaIssue } from "effect"; type PushListener = (data: unknown) => void; @@ -10,6 +11,14 @@ interface PendingRequest { const REQUEST_TIMEOUT_MS = 60_000; const RECONNECT_DELAYS_MS = [500, 1_000, 2_000, 4_000, 8_000]; +const decodeWsResponseFromJson = Schema.decodeUnknownExit(Schema.fromJsonString(WsResponse)); +const isWsPushEnvelope = Schema.is(WsPush); + +function isWebSocketResponseEnvelope( + message: typeof WsResponse.Type, +): message is WebSocketResponse { + return message instanceof WebSocketResponse; +} interface WsRequestEnvelope { id: string; @@ -127,25 +136,24 @@ export class WsTransport { } private handleMessage(raw: unknown) { - let parsed: unknown; - try { - parsed = JSON.parse(String(raw)); - } catch { + const exit = decodeWsResponseFromJson(raw); + if (exit._tag === "Failure") { + console.warn("Dropped inbound WebSocket envelope", { + reason: "decode-failed", + raw, + issue: Cause.pretty(exit.cause), + }); return; } - - if (!parsed || typeof parsed !== "object") return; - - const message = parsed as Record; + const message = exit.value; // Push event - if (message.type === "push") { - const push = message as unknown as WsPush; - const channelListeners = this.listeners.get(push.channel); + if (isWsPushEnvelope(message)) { + const channelListeners = this.listeners.get(message.channel); if (channelListeners) { for (const listener of channelListeners) { try { - listener(push.data); + listener(message.data); } catch { // Swallow listener errors } @@ -155,23 +163,20 @@ export class WsTransport { } // Response to a request - if (typeof message.id === "string") { - const responseId = message.id; - const pending = this.pending.get(responseId); - if (!pending) return; + if (!isWebSocketResponseEnvelope(message)) { + return; + } - clearTimeout(pending.timeout); - this.pending.delete(responseId); + const pending = this.pending.get(message.id); + if (!pending) return; - if ( - message.error && - typeof message.error === "object" && - typeof (message.error as { message?: unknown }).message === "string" - ) { - pending.reject(new Error((message.error as { message: string }).message)); - } else { - pending.resolve(message.result); - } + clearTimeout(pending.timeout); + this.pending.delete(message.id); + + if (message.error) { + pending.reject(new Error(message.error.message)); + } else { + pending.resolve(message.result); } } -- 2.54.0