| ... | ... | @@ -1,421 +0,0 @@ |
| 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 | | |
Julius Marminge <julius0216@outlook.com> 2026-02-25 18:36:03-08:00
Julius Marminge <jmarminge@gmail.com> 2026-02-26 13:20:36-08:00