diff --git a/.plans/16-pr89-review-remediation-phases.md b/.plans/16-pr89-review-remediation-phases.md index 513727edabd5c9cebbfc46954a7359f6d4554fe0..4adafa656cbd98d61a6796e1c5c9c32906ed9e14 100644 --- a/.plans/16-pr89-review-remediation-phases.md +++ b/.plans/16-pr89-review-remediation-phases.md @@ -10,6 +10,7 @@ - Invalid/false-positive: 3 (excluded) - Duplicate reposts: collapsed - Unique actionable findings after filtering: 58 + - Post-rewrite validity audit: 5 additional stale items marked invalid, leaving 53 actionable (`34 valid` + `19 partially-valid`) ## Phase 0: Canonical Triage Lock diff --git a/.plans/16a-pr89-phase0-canonical-triage.json b/.plans/16a-pr89-phase0-canonical-triage.json index 761146cf852d4a3530a1cb6c351c880168962c26..8c6a93cd603cc496cd6e78cd69bd656e81997c48 100644 --- a/.plans/16a-pr89-phase0-canonical-triage.json +++ b/.plans/16a-pr89-phase0-canonical-triage.json @@ -10,11 +10,11 @@ "phase0_generated_at": "2026-02-26T02:49:21.091145Z", "summary": { "total_threads": 185, - "actionable_canonical": 58, + "actionable_canonical": 53, "actionable_duplicates": 24, "filtered_outdated": 94, "filtered_resolved": 6, - "filtered_invalid": 3 + "filtered_invalid": 8 }, "canonical_items": [ { @@ -379,24 +379,6 @@ "macroscopeapp" ] }, - { - "canonical_id": "C021", - "area": "Other", - "title": "Shared mutable default metadata object causes stale eventId", - "severity": "Medium", - "path": "apps/server/src/orchestration/decider.ts", - "line": 27, - "thread_ids": [ - "PRRT_kwDORLtfbc5wkPaA" - ], - "thread_indexes": [ - 128 - ], - "duplicate_count": 0, - "authors": [ - "cursor" - ] - }, { "canonical_id": "C022", "area": "Edge-case parsing/platform behavior", @@ -451,24 +433,6 @@ "macroscopeapp" ] }, - { - "canonical_id": "C025", - "area": "Checkpointing correctness", - "title": "Duplicated checkpoint ref computation across two files", - "severity": "Medium", - "path": "apps/server/src/wsServer.ts", - "line": 128, - "thread_ids": [ - "PRRT_kwDORLtfbc5wvwag" - ], - "thread_indexes": [ - 148 - ], - "duplicate_count": 0, - "authors": [ - "cursor" - ] - }, { "canonical_id": "C026", "area": "WebSocket robustness", @@ -561,24 +525,6 @@ "macroscopeapp" ] }, - { - "canonical_id": "C031", - "area": "Event ordering and state consistency", - "title": "Revert uses wrong turn count from positional inference", - "severity": "Medium", - "path": "apps/web/src/session-logic.ts", - "line": 127, - "thread_ids": [ - "PRRT_kwDORLtfbc5v9SCp" - ], - "thread_indexes": [ - 41 - ], - "duplicate_count": 0, - "authors": [ - "cursor" - ] - }, { "canonical_id": "C032", "area": "Other", @@ -657,24 +603,6 @@ "cursor" ] }, - { - "canonical_id": "C036", - "area": "Checkpointing correctness", - "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", - "severity": "Low", - "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", - "line": 284, - "thread_ids": [ - "PRRT_kwDORLtfbc5wiqFX" - ], - "thread_indexes": [ - 115 - ], - "duplicate_count": 0, - "authors": [ - "cursor" - ] - }, { "canonical_id": "C037", "area": "Other", @@ -1003,24 +931,6 @@ "macroscopeapp" ] }, - { - "canonical_id": "C055", - "area": "Checkpointing correctness", - "title": "Duplicate `checkpointRefForThreadTurn` function across files", - "severity": "Low", - "path": "apps/server/src/wsServer.ts", - "line": 128, - "thread_ids": [ - "PRRT_kwDORLtfbc5wkPaG" - ], - "thread_indexes": [ - 129 - ], - "duplicate_count": 0, - "authors": [ - "cursor" - ] - }, { "canonical_id": "C056", "area": "Other", @@ -1684,9 +1594,8 @@ "title": "Revert uses wrong turn count from positional inference", "severity": "Medium", "has_false_positive_signal": false, - "canonical_id": "C031", - "phase0_status": "actionable_canonical", - "phase0_reason": "Canonical actionable item." + "phase0_status": "filtered_invalid", + "phase0_reason": "Invalid after post-rewrite audit: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary." }, { "index": 42, @@ -2740,9 +2649,8 @@ "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", "severity": "Low", "has_false_positive_signal": false, - "canonical_id": "C036", - "phase0_status": "actionable_canonical", - "phase0_reason": "Canonical actionable item." + "phase0_status": "filtered_invalid", + "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; single production source via Refs.ts." }, { "index": 116, @@ -2929,9 +2837,8 @@ "title": "Shared mutable default metadata object causes stale eventId", "severity": "Medium", "has_false_positive_signal": false, - "canonical_id": "C021", - "phase0_status": "actionable_canonical", - "phase0_reason": "Canonical actionable item." + "phase0_status": "filtered_invalid", + "phase0_reason": "Invalid after post-rewrite audit: Stale-eventId claim no longer applies; eventId is regenerated per event." }, { "index": 129, @@ -2944,9 +2851,8 @@ "title": "Duplicate `checkpointRefForThreadTurn` function across files", "severity": "Low", "has_false_positive_signal": false, - "canonical_id": "C055", - "phase0_status": "actionable_canonical", - "phase0_reason": "Canonical actionable item." + "phase0_status": "filtered_invalid", + "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; helper is centralized." }, { "index": 130, @@ -3226,9 +3132,8 @@ "title": "Duplicated checkpoint ref computation across two files", "severity": "Medium", "has_false_positive_signal": false, - "canonical_id": "C025", - "phase0_status": "actionable_canonical", - "phase0_reason": "Canonical actionable item." + "phase0_status": "filtered_invalid", + "phase0_reason": "Invalid after post-rewrite audit: No longer duplicated; checkpoint ref helper now centralized." }, { "index": 149, diff --git a/.plans/16a-pr89-phase0-canonical-triage.md b/.plans/16a-pr89-phase0-canonical-triage.md index e59ef61dcc65b2f02113fa45ed89a7eb4213aa3e..6bbe9d3855b0e957e2c99e4de06881863b5f7ea6 100644 --- a/.plans/16a-pr89-phase0-canonical-triage.md +++ b/.plans/16a-pr89-phase0-canonical-triage.md @@ -7,20 +7,18 @@ ## Summary - Total threads: 185 -- Actionable canonical: 58 +- Actionable canonical: 53 - Actionable duplicates: 24 - Filtered outdated: 94 - Filtered resolved: 6 -- Filtered invalid: 3 +- Filtered invalid: 8 ## Canonical Actionable Checklist -### Other (17) +### Other (16) - [ ] C011 [Medium] Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead. (apps/server/src/orchestration/Errors.ts:117) - Threads: PRRT_kwDORLtfbc5wBFv4 -- [ ] C021 [Medium] Shared mutable default metadata object causes stale eventId (apps/server/src/orchestration/decider.ts:27) - - Threads: PRRT_kwDORLtfbc5wkPaA - [ ] C024 [Medium] In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release. (apps/server/src/persistence/NodeSqliteClient.ts:145) - Threads: PRRT_kwDORLtfbc5whxX9 - [ ] C028 [Medium] Branch sync dispatches both server and stale local update (apps/web/src/components/BranchToolbar.tsx:102) @@ -54,15 +52,15 @@ ### Runtime resilience and failure handling (10) -- [ ] C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) (+5 dup) +- [x] C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) (+5 dup) - Threads: PRRT_kwDORLtfbc5wj4cH, PRRT_kwDORLtfbc5wnWwF, PRRT_kwDORLtfbc5wyTaP, PRRT_kwDORLtfbc5wzliw, PRRT_kwDORLtfbc5w0_g3, PRRT_kwDORLtfbc5w1HGT -- [ ] C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) (+4 dup) +- [x] C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) (+4 dup) - Threads: PRRT_kwDORLtfbc5whszW, PRRT_kwDORLtfbc5wyTaS, PRRT_kwDORLtfbc5wzli0, PRRT_kwDORLtfbc5w0_g4, PRRT_kwDORLtfbc5w1HGX - [ ] C019 [Medium] ProviderRuntimeIngestion processes events for wrong thread on race (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:178) - Threads: PRRT_kwDORLtfbc5wkPaL - [ ] C020 [Medium] On `message.completed`, the message ID is added to the set and `thread.message.assistant.complete` is dispatched. On `turn.completed`, the same set is iterated and `thread.message.assistant.complete` is dispatched again for each IDβ€”including already-completed ones. Consider removing message IDs from the set after dispatching on `message.completed`, or filtering out already-completed IDs before the `turn.completed` loop. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:266) - Threads: PRRT_kwDORLtfbc5w1GPr -- [ ] C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) (+1 dup) +- [x] C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) (+1 dup) - Threads: PRRT_kwDORLtfbc5wyTaW, PRRT_kwDORLtfbc5wzli3 - [ ] C042 [Low] Duplicated `resolveThreadWorkspaceCwd` across three files (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) - Threads: PRRT_kwDORLtfbc5wzli2 @@ -96,26 +94,7 @@ - [ ] C040 [Low] `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/keybindings.ts:488) - Threads: PRRT_kwDORLtfbc5wxvIA -### Checkpointing correctness (8) - -- [ ] C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) (+3 dup) - - Threads: PRRT_kwDORLtfbc5widJw, PRRT_kwDORLtfbc5wnWv_, PRRT_kwDORLtfbc5w0_g7, PRRT_kwDORLtfbc5w1C36 -- [ ] C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) - - Threads: PRRT_kwDORLtfbc5wiLhQ -- [ ] C025 [Medium] Duplicated checkpoint ref computation across two files (apps/server/src/wsServer.ts:128) - - Threads: PRRT_kwDORLtfbc5wvwag -- [ ] C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) - - Threads: PRRT_kwDORLtfbc5wlYgo -- [ ] C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) - - Threads: PRRT_kwDORLtfbc5wj5fn -- [ ] C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) (+2 dup) - - Threads: PRRT_kwDORLtfbc5wlYgs, PRRT_kwDORLtfbc5wxsO6, PRRT_kwDORLtfbc5w1C4B -- [ ] C036 [Low] Duplicate `checkpointRefForThreadTurn` function in two production files (apps/server/src/checkpointing/Layers/CheckpointStore.ts:284) - - Threads: PRRT_kwDORLtfbc5wiqFX -- [ ] C055 [Low] Duplicate `checkpointRefForThreadTurn` function across files (apps/server/src/wsServer.ts:128) - - Threads: PRRT_kwDORLtfbc5wkPaG - -### Event ordering and state consistency (6) +### Event ordering and state consistency (5) - [ ] C001 [High] Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:96) - Threads: PRRT_kwDORLtfbc5wzp4T @@ -127,14 +106,12 @@ - Threads: PRRT_kwDORLtfbc5whxJO - [ ] C016 [Medium] The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:490) - Threads: PRRT_kwDORLtfbc5wxvH8 -- [ ] C031 [Medium] Revert uses wrong turn count from positional inference (apps/web/src/session-logic.ts:127) - - Threads: PRRT_kwDORLtfbc5v9SCp ### WebSocket robustness (5) - [ ] C003 [High] Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake. (apps/server/src/wsServer.ts:75) - Threads: PRRT_kwDORLtfbc5v-cf4 -- [ ] C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) +- [x] C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) - Threads: PRRT_kwDORLtfbc5wj4cE - [ ] C053 [Low] Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection. (apps/server/src/wsServer.ts:83) - Threads: PRRT_kwDORLtfbc5v-WPD @@ -143,6 +120,19 @@ - [ ] C059 [Low] Suggestion: don’t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object. (apps/web/src/wsTransport.ts:59) - Threads: PRRT_kwDORLtfbc5whtrN +### Checkpointing correctness (5) + +- [ ] C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) (+3 dup) + - Threads: PRRT_kwDORLtfbc5widJw, PRRT_kwDORLtfbc5wnWv_, PRRT_kwDORLtfbc5w0_g7, PRRT_kwDORLtfbc5w1C36 +- [ ] C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) + - Threads: PRRT_kwDORLtfbc5wiLhQ +- [ ] C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) + - Threads: PRRT_kwDORLtfbc5wlYgo +- [ ] C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) + - Threads: PRRT_kwDORLtfbc5wj5fn +- [ ] C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) (+2 dup) + - Threads: PRRT_kwDORLtfbc5wlYgs, PRRT_kwDORLtfbc5wxsO6, PRRT_kwDORLtfbc5w1C4B + ### Build/runtime portability (2) - [ ] C006 [Medium] Build script runs TypeScript file with bare node (apps/server/package.json:14) @@ -161,8 +151,18 @@ - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. - PRRT_kwDORLtfbc5v9An- | apps/web/src/wsNativeApi.ts:165 | push stream, and the callbacks rely on unchecked casts. Please reintroduce a `providers.onEvent` (or equivalent) that subscribes to the `providers.event` channel and decode read-model/domain-event payloads using the shared contracts schemas before invoking callbacks. As per coding guidelines: "Web app must consume provider event streams via WebSocket push on channel `providers.event`" and "Use Zod schemas from `packages/contracts` for shared type contracts covering provider events, WebSocket protocol, and model/session types".
πŸ€– Prompt for AI Agents - Rationale: Invalid by design: provider runtime events are intentionally surfaced through orchestration domain events (`orchestration.domainEvent`), not a separate `providers.event` WebSocket channel in the current architecture. +- PRRT_kwDORLtfbc5v9SCp | apps/web/src/session-logic.ts:127 | Revert uses wrong turn count from positional inference + - Rationale: Invalid after post-rewrite audit: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary. - PRRT_kwDORLtfbc5widJx | apps/server/package.json:14 | Build script requires implicit Node.js TypeScript support - Rationale: Thread contains explicit false-positive determination in replies. +- PRRT_kwDORLtfbc5wiqFX | apps/server/src/checkpointing/Layers/CheckpointStore.ts:284 | Duplicate `checkpointRefForThreadTurn` function in two production files + - Rationale: Invalid after post-rewrite audit: No longer duplicated; single production source via Refs.ts. +- PRRT_kwDORLtfbc5wkPaA | apps/server/src/orchestration/decider.ts:27 | Shared mutable default metadata object causes stale eventId + - Rationale: Invalid after post-rewrite audit: Stale-eventId claim no longer applies; eventId is regenerated per event. +- PRRT_kwDORLtfbc5wkPaG | apps/server/src/wsServer.ts:128 | Duplicate `checkpointRefForThreadTurn` function across files + - Rationale: Invalid after post-rewrite audit: No longer duplicated; helper is centralized. +- PRRT_kwDORLtfbc5wvwag | apps/server/src/wsServer.ts:128 | Duplicated checkpoint ref computation across two files + - Rationale: Invalid after post-rewrite audit: No longer duplicated; checkpoint ref helper now centralized. ## Duplicate Mapping diff --git a/.plans/16b-pr89-architecture-validity-audit.json b/.plans/16b-pr89-architecture-validity-audit.json new file mode 100644 index 0000000000000000000000000000000000000000..ab9407fd70eccae8aae88a5573c444ba975e4bcc --- /dev/null +++ b/.plans/16b-pr89-architecture-validity-audit.json @@ -0,0 +1,790 @@ +{ + "source": ".plans/16a-pr89-phase0-canonical-triage.json", + "architecture_assumption": "Provider runtime activity is projected and consumed via orchestration.domainEvent; no separate providers.event push channel is expected.", + "summary": { + "total_canonical_reviewed": 58, + "valid": 34, + "partially_valid": 19, + "invalid": 5 + }, + "records": [ + { + "canonical_id": "C001", + "verdict": "valid", + "rationale": "Append/project/receipt are non-atomic; retry can duplicate events.", + "severity": "High", + "area": "Event ordering and state consistency", + "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", + "line": 96, + "title": "Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wzp4T" + ] + }, + { + "canonical_id": "C002", + "verdict": "valid", + "rationale": "Ingestion worker loop can terminate on unhandled processEvent failure.", + "severity": "High", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", + "line": 333, + "title": "A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wj4cH", + "PRRT_kwDORLtfbc5wnWwF", + "PRRT_kwDORLtfbc5wyTaP", + "PRRT_kwDORLtfbc5wzliw", + "PRRT_kwDORLtfbc5w0_g3", + "PRRT_kwDORLtfbc5w1HGT" + ] + }, + { + "canonical_id": "C003", + "verdict": "valid", + "rationale": "Upgrade reject writes then destroys socket without defensive error listener.", + "severity": "High", + "area": "WebSocket robustness", + "path": "apps/server/src/wsServer.ts", + "line": 75, + "title": "Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake.", + "thread_ids": [ + "PRRT_kwDORLtfbc5v-cf4" + ] + }, + { + "canonical_id": "C006", + "verdict": "partially-valid", + "rationale": "Node 24 makes this work, but script still relies on runtime-specific TS execution.", + "severity": "Medium", + "area": "Build/runtime portability", + "path": "apps/server/package.json", + "line": 14, + "title": "Build script runs TypeScript file with bare node", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxsO-" + ] + }, + { + "canonical_id": "C007", + "verdict": "partially-valid", + "rationale": "Same as C006 (duplicate concern).", + "severity": "Medium", + "area": "Build/runtime portability", + "path": "apps/server/package.json", + "line": 14, + "title": "Build script uses `node` to run `.ts` file directly", + "thread_ids": [ + "PRRT_kwDORLtfbc5wiLNS", + "PRRT_kwDORLtfbc5wj5fl" + ] + }, + { + "canonical_id": "C008", + "verdict": "partially-valid", + "rationale": "Normalization inconsistency remains, but upstream callsites now mostly canonicalize inputs.", + "severity": "Medium", + "area": "Checkpointing correctness", + "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", + "line": 94, + "title": "Inconsistent input normalization across CheckpointStore methods", + "thread_ids": [ + "PRRT_kwDORLtfbc5widJw", + "PRRT_kwDORLtfbc5wnWv_", + "PRRT_kwDORLtfbc5w0_g7", + "PRRT_kwDORLtfbc5w1C36" + ] + }, + { + "canonical_id": "C009", + "verdict": "valid", + "rationale": "Braced rename parsing still breaks paths like src/{old => new}/file.ts.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/git/Layers/GitCore.ts", + "line": 41, + "title": "Git's braced rename syntax (e.g., `src/{old => new}/file.ts`) isn't handled correctly. The current slice after ` => ` produces invalid paths like `new}/file.ts`. Consider expanding the braces to construct the full destination path.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5w1CxT" + ] + }, + { + "canonical_id": "C010", + "verdict": "valid", + "rationale": "ENOENT for missing keybindings config still not handled as empty/default.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/keybindings.ts", + "line": 418, + "title": "`loadCustomKeybindingsConfig` fails when the config file doesn't exist, which is expected for new users. Consider catching `ENOENT` and returning an empty array instead.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvIJ" + ] + }, + { + "canonical_id": "C011", + "verdict": "valid", + "rationale": "Template-literal interpolation of Symbol still throws.", + "severity": "Medium", + "area": "Other", + "path": "apps/server/src/orchestration/Errors.ts", + "line": 117, + "title": "Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wBFv4" + ] + }, + { + "canonical_id": "C012", + "verdict": "valid", + "rationale": "Revert completion dispatch remains forked; state consistency window remains.", + "severity": "Medium", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", + "line": 542, + "title": "Forked revert dispatch risks read model inconsistency", + "thread_ids": [ + "PRRT_kwDORLtfbc5whszW", + "PRRT_kwDORLtfbc5wyTaS", + "PRRT_kwDORLtfbc5wzli0", + "PRRT_kwDORLtfbc5w0_g4", + "PRRT_kwDORLtfbc5w1HGX" + ] + }, + { + "canonical_id": "C013", + "verdict": "valid", + "rationale": "Persisted event can outpace in-memory projection on mid-flight failure.", + "severity": "Medium", + "area": "Event ordering and state consistency", + "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", + "line": 99, + "title": "If `projectionPipeline.projectEvent` fails after `eventStore.append` succeeds, the event is persisted but `readModel` isn't updated, causing desync. Consider updating the in-memory `readModel` immediately after append (before the external projection), so local state stays consistent regardless of downstream failures.", + "thread_ids": [ + "PRRT_kwDORLtfbc5whtrM" + ] + }, + { + "canonical_id": "C014", + "verdict": "partially-valid", + "rationale": "Broad catch may be intentional for liveness; correctness concern is contextual.", + "severity": "Medium", + "area": "Event ordering and state consistency", + "path": "apps/server/src/orchestration/Layers/OrchestrationEngine.ts", + "line": 144, + "title": "Engine error handler catches all errors including non-invariant ones", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkPaJ" + ] + }, + { + "canonical_id": "C015", + "verdict": "partially-valid", + "rationale": "Message fallback retention issue is real, but prior FK-violation claim is overstated.", + "severity": "Medium", + "area": "Event ordering and state consistency", + "path": "apps/server/src/orchestration/Layers/ProjectionPipeline.ts", + "line": 99, + "title": "The gap-filling fallback logic can retain messages from turns that are about to be deleted, causing foreign key violations. Consider removing the fallback logic entirely, or filtering `fallbackUserMessages` and `fallbackAssistantMessages` to only include messages whose `turnId` is in `retainedTurnIds`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5whxJO" + ] + }, + { + "canonical_id": "C016", + "verdict": "valid", + "rationale": "Pending turn-start map is in-memory only and not rebuilt on bootstrap.", + "severity": "Medium", + "area": "Event ordering and state consistency", + "path": "apps/server/src/orchestration/Layers/ProjectionPipeline.ts", + "line": 490, + "title": "The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvH8" + ] + }, + { + "canonical_id": "C017", + "verdict": "partially-valid", + "rationale": "Snapshot sequence can under-report due to extra projectors, but replay impact is lower now.", + "severity": "Medium", + "area": "Checkpointing correctness", + "path": "apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts", + "line": 71, + "title": "`REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wiLhQ" + ] + }, + { + "canonical_id": "C018", + "verdict": "valid", + "rationale": "handledTurnStartKeys still grows without pruning.", + "severity": "Medium", + "area": "Memory/resource growth", + "path": "apps/server/src/orchestration/Layers/ProviderCommandReactor.ts", + "line": 84, + "title": "Unbounded memory growth in turn start deduplication set", + "thread_ids": [ + "PRRT_kwDORLtfbc5whszQ", + "PRRT_kwDORLtfbc5wl2A8", + "PRRT_kwDORLtfbc5wyTaT", + "PRRT_kwDORLtfbc5wzliz", + "PRRT_kwDORLtfbc5w0_g-", + "PRRT_kwDORLtfbc5w1HGW" + ] + }, + { + "canonical_id": "C019", + "verdict": "partially-valid", + "rationale": "SessionId-only routing can misassociate events under races/rebinds.", + "severity": "Medium", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", + "line": 178, + "title": "ProviderRuntimeIngestion processes events for wrong thread on race", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkPaL" + ] + }, + { + "canonical_id": "C020", + "verdict": "partially-valid", + "rationale": "Duplicate complete dispatch exists; downstream impact often idempotent.", + "severity": "Medium", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", + "line": 266, + "title": "On `message.completed`, the message ID is added to the set and `thread.message.assistant.complete` is dispatched. On `turn.completed`, the same set is iterated and `thread.message.assistant.complete` is dispatched again for each ID\u2014including already-completed ones. Consider removing message IDs from the set after dispatching on `message.completed`, or filtering out already-completed IDs before the `turn.completed` loop.", + "thread_ids": [ + "PRRT_kwDORLtfbc5w1GPr" + ] + }, + { + "canonical_id": "C021", + "verdict": "invalid", + "rationale": "Stale-eventId claim no longer applies; eventId is regenerated per event.", + "severity": "Medium", + "area": "Other", + "path": "apps/server/src/orchestration/decider.ts", + "line": 27, + "title": "Shared mutable default metadata object causes stale eventId", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkPaA" + ] + }, + { + "canonical_id": "C022", + "verdict": "valid", + "rationale": "fish PATH formatting risk still exists in os-jank path recovery.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/os-jank.ts", + "line": 10, + "title": "Fish shell outputs `$PATH` as space-separated, not colon-separated. Consider checking if the shell is fish and using `string join : $PATH` instead, or validating the result contains colons before assigning.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkRZM" + ] + }, + { + "canonical_id": "C023", + "verdict": "valid", + "rationale": "-ilc shell invocation can pollute captured PATH output.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/os-jank.ts", + "line": 10, + "title": "Using `-il` flags causes the shell to source profile scripts that may print banners or other text, polluting the captured `PATH`. Consider using `-lc` (login only, non-interactive) to reduce unwanted output.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wj4cM" + ] + }, + { + "canonical_id": "C024", + "verdict": "partially-valid", + "rationale": "prepare() still outside managed effect boundary; failure handling risk remains.", + "severity": "Medium", + "area": "Other", + "path": "apps/server/src/persistence/NodeSqliteClient.ts", + "line": 145, + "title": "In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release.", + "thread_ids": [ + "PRRT_kwDORLtfbc5whxX9" + ] + }, + { + "canonical_id": "C025", + "verdict": "invalid", + "rationale": "No longer duplicated; checkpoint ref helper now centralized.", + "severity": "Medium", + "area": "Checkpointing correctness", + "path": "apps/server/src/wsServer.ts", + "line": 128, + "title": "Duplicated checkpoint ref computation across two files", + "thread_ids": [ + "PRRT_kwDORLtfbc5wvwag" + ] + }, + { + "canonical_id": "C026", + "verdict": "valid", + "rationale": "runPromise result still not caught; rejection can surface unhandled.", + "severity": "Medium", + "area": "WebSocket robustness", + "path": "apps/server/src/wsServer.ts", + "line": 545, + "title": "Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wj4cE" + ] + }, + { + "canonical_id": "C027", + "verdict": "valid", + "rationale": "Same unhandled rejection path remains in WS message handler.", + "severity": "Medium", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/wsServer.ts", + "line": 545, + "title": "WS message handler can cause unhandled promise rejection", + "thread_ids": [ + "PRRT_kwDORLtfbc5wyTaW", + "PRRT_kwDORLtfbc5wzli3" + ] + }, + { + "canonical_id": "C028", + "verdict": "partially-valid", + "rationale": "Optimistic local+server dual update is intentional but can temporarily diverge.", + "severity": "Medium", + "area": "Other", + "path": "apps/web/src/components/BranchToolbar.tsx", + "line": 102, + "title": "Branch sync dispatches both server and stale local update", + "thread_ids": [ + "PRRT_kwDORLtfbc5v-XCu" + ] + }, + { + "canonical_id": "C029", + "verdict": "valid", + "rationale": "file URL decoding still double-decodes in one path.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/web/src/markdown-links.ts", + "line": 105, + "title": "`parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wnVsU" + ] + }, + { + "canonical_id": "C030", + "verdict": "valid", + "rationale": "Scheme regex still misclassifies script.ts:10 as external scheme.", + "severity": "Medium", + "area": "Edge-case parsing/platform behavior", + "path": "apps/web/src/markdown-links.ts", + "line": 111, + "title": "`EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wnVsK" + ] + }, + { + "canonical_id": "C031", + "verdict": "invalid", + "rationale": "Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary.", + "severity": "Medium", + "area": "Event ordering and state consistency", + "path": "apps/web/src/session-logic.ts", + "line": 127, + "title": "Revert uses wrong turn count from positional inference", + "thread_ids": [ + "PRRT_kwDORLtfbc5v9SCp" + ] + }, + { + "canonical_id": "C032", + "verdict": "valid", + "rationale": "Dummy workflow marker comment is still present in docs.", + "severity": "Low", + "area": "Other", + "path": "AGENTS.md", + "line": 45, + "title": "Dummy workflow marker accidentally committed to docs", + "thread_ids": [ + "PRRT_kwDORLtfbc5wlYgj", + "PRRT_kwDORLtfbc5w1C4H" + ] + }, + { + "canonical_id": "C033", + "verdict": "partially-valid", + "rationale": "Original claim overstated; some errors used, others appear unused.", + "severity": "Low", + "area": "Checkpointing correctness", + "path": "apps/server/src/checkpointing/Errors.ts", + "line": 51, + "title": "Three error classes defined but never instantiated anywhere", + "thread_ids": [ + "PRRT_kwDORLtfbc5wlYgo" + ] + }, + { + "canonical_id": "C034", + "verdict": "valid", + "rationale": "CheckpointInvariantError remains redundantly included in service union.", + "severity": "Low", + "area": "Checkpointing correctness", + "path": "apps/server/src/checkpointing/Errors.ts", + "line": 79, + "title": "Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type", + "thread_ids": [ + "PRRT_kwDORLtfbc5wj5fn" + ] + }, + { + "canonical_id": "C035", + "verdict": "valid", + "rationale": "Same as C034.", + "severity": "Low", + "area": "Checkpointing correctness", + "path": "apps/server/src/checkpointing/Errors.ts", + "line": 79, + "title": "Redundant error type in CheckpointServiceError union definition", + "thread_ids": [ + "PRRT_kwDORLtfbc5wlYgs", + "PRRT_kwDORLtfbc5wxsO6", + "PRRT_kwDORLtfbc5w1C4B" + ] + }, + { + "canonical_id": "C036", + "verdict": "invalid", + "rationale": "No longer duplicated; single production source via Refs.ts.", + "severity": "Low", + "area": "Checkpointing correctness", + "path": "apps/server/src/checkpointing/Layers/CheckpointStore.ts", + "line": 284, + "title": "Duplicate `checkpointRefForThreadTurn` function in two production files", + "thread_ids": [ + "PRRT_kwDORLtfbc5wiqFX" + ] + }, + { + "canonical_id": "C037", + "verdict": "partially-valid", + "rationale": "Callback cleanup missing, but practical exposure is low in one-shot startup path.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/config.ts", + "line": 41, + "title": "`Effect.callback` should return a cleanup function to close the server(s) on fiber interruption. Without it, the `Net.Server` handles keep the process alive and leak the port if the effect is cancelled.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wj4cO" + ] + }, + { + "canonical_id": "C038", + "verdict": "valid", + "rationale": "Chunk-by-chunk UTF-8 decode can still corrupt split multibyte characters.", + "severity": "Low", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/git/Layers/CodexTextGeneration.ts", + "line": 136, + "title": "Multi-byte UTF-8 characters split across chunks will be corrupted when decoding each chunk separately. Consider accumulating all chunks first, then decoding once, or use `TextDecoder` with `stream: true`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5w1GPo" + ] + }, + { + "canonical_id": "C039", + "verdict": "partially-valid", + "rationale": "Parser/encoder mismatch remains, but encoder path currently low-use.", + "severity": "Low", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/keybindings.ts", + "line": 352, + "title": "The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes(\"+\")` returns true for the literal `+` key. Consider checking `shortcut.key === \"+\"` separately and encoding it as `\"space\"` style (e.g., a special token), or adjusting the condition to allow the single `+` character.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvIB" + ] + }, + { + "canonical_id": "C040", + "verdict": "valid", + "rationale": "upsertKeybindingRule read-modify-write remains race-prone.", + "severity": "Low", + "area": "Edge-case parsing/platform behavior", + "path": "apps/server/src/keybindings.ts", + "line": 488, + "title": "`upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvIA" + ] + }, + { + "canonical_id": "C041", + "verdict": "valid", + "rationale": "Same Symbol interpolation hazard as C011 remains.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/orchestration/Errors.ts", + "line": 126, + "title": "Template literal interpolation throws `TypeError` if `cause` is a `Symbol`. Consider using `String(cause)` for safe coercion.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wBFWs" + ] + }, + { + "canonical_id": "C042", + "verdict": "partially-valid", + "rationale": "Duplication exists but one instance is variant logic, so impact is moderate.", + "severity": "Low", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", + "line": 62, + "title": "Duplicated `resolveThreadWorkspaceCwd` across three files", + "thread_ids": [ + "PRRT_kwDORLtfbc5wzli2" + ] + }, + { + "canonical_id": "C043", + "verdict": "valid", + "rationale": "Workspace CWD resolution duplication still present across modules.", + "severity": "Low", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", + "line": 62, + "title": "Duplicated workspace CWD resolution logic across reactor modules", + "thread_ids": [ + "PRRT_kwDORLtfbc5wnWwM", + "PRRT_kwDORLtfbc5w1C3-", + "PRRT_kwDORLtfbc5w1HGZ" + ] + }, + { + "canonical_id": "C044", + "verdict": "partially-valid", + "rationale": "Errors are swallowed to empty diff with warning; not fully silent but still lossy.", + "severity": "Low", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/CheckpointReactor.ts", + "line": 274, + "title": "Checkpoint reactor swallows diff errors silently for `turn.completed`", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkPaO" + ] + }, + { + "canonical_id": "C045", + "verdict": "valid", + "rationale": "truncateDetail still overshoots limit.", + "severity": "Low", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", + "line": 29, + "title": "`truncateDetail` slices to `limit - 1` then appends `\"...\"` (3 chars), producing strings of length `limit + 2`. Consider slicing to `limit - 3` instead.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wzp4R" + ] + }, + { + "canonical_id": "C046", + "verdict": "valid", + "rationale": "latestMessageIdByTurnKey still unused/unpruned in per-turn clear path.", + "severity": "Low", + "area": "Runtime resilience and failure handling", + "path": "apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts", + "line": 133, + "title": "`latestMessageIdByTurnKey` is written to but never read, and `clearAssistantMessageIdsForTurn` doesn't clear its entries\u2014only `clearTurnStateForSession` does. Consider removing this map entirely if unused, or clearing it alongside `turnMessageIdsByTurnKey` in `clearAssistantMessageIdsForTurn`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvIQ" + ] + }, + { + "canonical_id": "C047", + "verdict": "valid", + "rationale": "Decode and SQL errors still collapsed into one persistence error kind.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts", + "line": 75, + "title": "`SqlSchema.findOneOption` can produce both SQL errors and decode errors, but `mapError` wraps all as `PersistenceSqlError`. Consider distinguishing `ParseError` from SQL errors and mapping decode failures to `PersistenceDecodeError` instead.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wiaR-" + ] + }, + { + "canonical_id": "C048", + "verdict": "partially-valid", + "rationale": "Unsafe run() result cast remains; runtime risk depends on raw-result consumers.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/persistence/NodeSqliteClient.ts", + "line": 99, + "title": "`statement.run()` returns a `RunResult` object, not an array. Casting it to `ReadonlyArray` will cause runtime errors when consumers call array methods. Consider wrapping the result (e.g., `[result]`) or returning a properly typed object.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wio-r" + ] + }, + { + "canonical_id": "C049", + "verdict": "valid", + "rationale": "JSON.stringify(cause) may return undefined despite string expectations.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/provider/Layers/ProviderService.ts", + "line": 59, + "title": "`JSON.stringify(cause)` returns `undefined` for `undefined`, functions, or symbols, violating the `string` return type. Consider coercing the result to a string (e.g., `String(JSON.stringify(cause))`) or adding a fallback.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wnVsI" + ] + }, + { + "canonical_id": "C050", + "verdict": "valid", + "rationale": "ProviderSessionDirectory upsert remains read-merge-write without concurrency control.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/provider/Layers/ProviderSessionDirectory.ts", + "line": 94, + "title": "The read-modify-write pattern (`getBySessionId` \u2192 merge \u2192 `upsert`) is susceptible to lost updates under concurrent writes. Consider wrapping in a transaction or adding optimistic concurrency control (e.g., version field) if concurrent session updates are expected.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wiLhY" + ] + }, + { + "canonical_id": "C051", + "verdict": "partially-valid", + "rationale": "Null-clearing issue is real for providerThreadId; adapterKey part overstated.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/provider/Layers/ProviderSessionDirectory.ts", + "line": 119, + "title": "Using `??` for `providerThreadId` and `adapterKey` makes it impossible to clear these fields by passing `null`, since `null ?? existing` evaluates to `existing`. Consider using explicit `undefined` checks (like `resumeCursor` does) if clearing should be supported.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvH9" + ] + }, + { + "canonical_id": "C052", + "verdict": "valid", + "rationale": "Data callback may race before processHandle assignment.", + "severity": "Low", + "area": "Other", + "path": "apps/server/src/terminal/Layers/BunPTY.ts", + "line": 97, + "title": "Race condition: `processHandle` may be `null` when `data` callback fires, since it's assigned after `Bun.spawn` returns. Consider initializing `BunPtyProcess` first, then passing it to the callback to avoid losing initial output.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5w1CxE" + ] + }, + { + "canonical_id": "C053", + "verdict": "valid", + "rationale": "Still uses write+destroy rather than end() for rejection response.", + "severity": "Low", + "area": "WebSocket robustness", + "path": "apps/server/src/wsServer.ts", + "line": 83, + "title": "Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection.", + "thread_ids": [ + "PRRT_kwDORLtfbc5v-WPD" + ] + }, + { + "canonical_id": "C054", + "verdict": "valid", + "rationale": "Array chunk UTF-8 decode remains vulnerable to split multibyte corruption.", + "severity": "Low", + "area": "WebSocket robustness", + "path": "apps/server/src/wsServer.ts", + "line": 104, + "title": "When array chunks contain a multi-byte UTF-8 character split across boundaries, decoding each chunk separately produces replacement characters. Consider using `Buffer.concat()` on all chunks before calling `.toString(\"utf8\")`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5whtrR" + ] + }, + { + "canonical_id": "C055", + "verdict": "invalid", + "rationale": "No longer duplicated; helper is centralized.", + "severity": "Low", + "area": "Checkpointing correctness", + "path": "apps/server/src/wsServer.ts", + "line": 128, + "title": "Duplicate `checkpointRefForThreadTurn` function across files", + "thread_ids": [ + "PRRT_kwDORLtfbc5wkPaG" + ] + }, + { + "canonical_id": "C056", + "verdict": "partially-valid", + "rationale": "Bug pattern exists, but current callsites mostly avoid triggering it.", + "severity": "Low", + "area": "Other", + "path": "apps/web/src/components/ui/sidebar.tsx", + "line": 114, + "title": "When `onOpenChange` is provided without `open`, the internal `_open` state never updates because `setOpenProp` takes precedence. Consider calling `_setOpen` when `openProp === undefined`, regardless of whether `setOpenProp` exists.", + "thread_ids": [ + "PRRT_kwDORLtfbc5wxvIq" + ] + }, + { + "canonical_id": "C057", + "verdict": "valid", + "rationale": "Resizable object recreation still retriggers effect/storage reads.", + "severity": "Low", + "area": "Other", + "path": "apps/web/src/routes/_chat.$threadId.tsx", + "line": 105, + "title": "The `resizable` object is recreated on every render, causing `SidebarRail`'s `useEffect` to repeatedly read localStorage and update the DOM. Consider memoizing the object with `useMemo`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wyWz4" + ] + }, + { + "canonical_id": "C058", + "verdict": "valid", + "rationale": "Number(null) -> 0 path still forces min width on initial load.", + "severity": "Low", + "area": "Other", + "path": "apps/web/src/routes/_chat.$threadId.tsx", + "line": 122, + "title": "When `localStorage.getItem()` returns `null`, `Number(null)` evaluates to `0`, which passes `Number.isFinite(0)`. This causes the sidebar to clamp to `minWidth` on first load, overriding the `DIFF_INLINE_DEFAULT_WIDTH` CSS clamp. Consider checking for `null` or empty string before parsing, e.g. guard with `storedWidth === null || storedWidth === ''`.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5wnVsX" + ] + }, + { + "canonical_id": "C059", + "verdict": "partially-valid", + "rationale": "Transport _tag override risk exists but current callsites are constrained.", + "severity": "Low", + "area": "WebSocket robustness", + "path": "apps/web/src/wsTransport.ts", + "line": 59, + "title": "Suggestion: don\u2019t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object.", + "thread_ids": [ + "PRRT_kwDORLtfbc5whtrN" + ] + }, + { + "canonical_id": "C060", + "verdict": "valid", + "rationale": "Schema still cannot express null clear for defaultModel patch.", + "severity": "Low", + "area": "Other", + "path": "packages/contracts/src/orchestration.ts", + "line": 253, + "title": "`defaultModel` should be `Schema.optional(Schema.NullOr(Schema.String))` to allow clearing the value. Currently there's no way to reset it to `null` since omitting means \"no change\" in patch semantics.
\ud83d\ude80 Reply \"fix it for me\" or copy this AI Prompt for your agent:", + "thread_ids": [ + "PRRT_kwDORLtfbc5whxJC" + ] + } + ] +} diff --git a/.plans/16b-pr89-architecture-validity-audit.md b/.plans/16b-pr89-architecture-validity-audit.md new file mode 100644 index 0000000000000000000000000000000000000000..946bc2270254a5885f4ff2983cc82619d185d212 --- /dev/null +++ b/.plans/16b-pr89-architecture-validity-audit.md @@ -0,0 +1,125 @@ +# PR #89 Post-Rewrite Validity Audit + +Architecture baseline used for this audit: +- Provider runtime activity is projected into orchestration domain events. +- Web client consumes `orchestration.domainEvent`; no separate `providers.event` channel is expected. + +## Summary + +- Canonical items reviewed: 58 +- Valid: 34 +- Partially valid: 19 +- Invalid (stale/no longer applicable): 5 + +## Invalid Items To Close + +- C021 [Medium] Shared mutable default metadata object causes stale eventId (apps/server/src/orchestration/decider.ts:27) + - Reason: Stale-eventId claim no longer applies; eventId is regenerated per event. + - Threads: PRRT_kwDORLtfbc5wkPaA +- C025 [Medium] Duplicated checkpoint ref computation across two files (apps/server/src/wsServer.ts:128) + - Reason: No longer duplicated; checkpoint ref helper now centralized. + - Threads: PRRT_kwDORLtfbc5wvwag +- C031 [Medium] Revert uses wrong turn count from positional inference (apps/web/src/session-logic.ts:127) + - Reason: Revert now uses explicit checkpointTurnCount first; positional fallback is non-primary. + - Threads: PRRT_kwDORLtfbc5v9SCp +- C036 [Low] Duplicate `checkpointRefForThreadTurn` function in two production files (apps/server/src/checkpointing/Layers/CheckpointStore.ts:284) + - Reason: No longer duplicated; single production source via Refs.ts. + - Threads: PRRT_kwDORLtfbc5wiqFX +- C055 [Low] Duplicate `checkpointRefForThreadTurn` function across files (apps/server/src/wsServer.ts:128) + - Reason: No longer duplicated; helper is centralized. + - Threads: PRRT_kwDORLtfbc5wkPaG + +## Partially Valid Items (de-scope or lower priority) + +- C006 [Medium] Build script runs TypeScript file with bare node (apps/server/package.json:14) + - Note: Node 24 makes this work, but script still relies on runtime-specific TS execution. +- C007 [Medium] Build script uses `node` to run `.ts` file directly (apps/server/package.json:14) + - Note: Same as C006 (duplicate concern). +- C008 [Medium] Inconsistent input normalization across CheckpointStore methods (apps/server/src/checkpointing/Layers/CheckpointStore.ts:94) + - Note: Normalization inconsistency remains, but upstream callsites now mostly canonicalize inputs. +- C014 [Medium] Engine error handler catches all errors including non-invariant ones (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:144) + - Note: Broad catch may be intentional for liveness; correctness concern is contextual. +- C015 [Medium] The gap-filling fallback logic can retain messages from turns that are about to be deleted, causing foreign key violations. Consider removing the fallback logic entirely, or filtering `fallbackUserMessages` and `fallbackAssistantMessages` to only include messages whose `turnId` is in `retainedTurnIds`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:99) + - Note: Message fallback retention issue is real, but prior FK-violation claim is overstated. +- C017 [Medium] `REQUIRED_SNAPSHOT_PROJECTORS` includes `pending-approvals` and `thread-turns`, but `getSnapshot` doesn't query their data. If these projectors lag behind, the returned `snapshotSequence` will be lower than what the included data actually reflects, causing clients to replay already-applied events. Consider filtering `REQUIRED_SNAPSHOT_PROJECTORS` to only include projectors whose data is actually fetched in the snapshot.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts:71) + - Note: Snapshot sequence can under-report due to extra projectors, but replay impact is lower now. +- C019 [Medium] ProviderRuntimeIngestion processes events for wrong thread on race (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:178) + - Note: SessionId-only routing can misassociate events under races/rebinds. +- C020 [Medium] On `message.completed`, the message ID is added to the set and `thread.message.assistant.complete` is dispatched. On `turn.completed`, the same set is iterated and `thread.message.assistant.complete` is dispatched again for each IDβ€”including already-completed ones. Consider removing message IDs from the set after dispatching on `message.completed`, or filtering out already-completed IDs before the `turn.completed` loop. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:266) + - Note: Duplicate complete dispatch exists; downstream impact often idempotent. +- C024 [Medium] In `executeUnprepared`, preparing outside an effect and not closing the statement can cause uncaught errors and leaks. Suggest wrapping with `Effect.acquireUseRelease`: prepare in acquire (error-safe) and `finalize` in release. (apps/server/src/persistence/NodeSqliteClient.ts:145) + - Note: prepare() still outside managed effect boundary; failure handling risk remains. +- C028 [Medium] Branch sync dispatches both server and stale local update (apps/web/src/components/BranchToolbar.tsx:102) + - Note: Optimistic local+server dual update is intentional but can temporarily diverge. +- C033 [Low] Three error classes defined but never instantiated anywhere (apps/server/src/checkpointing/Errors.ts:51) + - Note: Original claim overstated; some errors used, others appear unused. +- C037 [Low] `Effect.callback` should return a cleanup function to close the server(s) on fiber interruption. Without it, the `Net.Server` handles keep the process alive and leak the port if the effect is cancelled.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/config.ts:41) + - Note: Callback cleanup missing, but practical exposure is low in one-shot startup path. +- C039 [Low] The `+` key can be parsed (via trailing `+` handling) but cannot be encoded because `shortcut.key.includes("+")` returns true for the literal `+` key. Consider checking `shortcut.key === "+"` separately and encoding it as `"space"` style (e.g., a special token), or adjusting the condition to allow the single `+` character.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/keybindings.ts:352) + - Note: Parser/encoder mismatch remains, but encoder path currently low-use. +- C042 [Low] Duplicated `resolveThreadWorkspaceCwd` across three files (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) + - Note: Duplication exists but one instance is variant logic, so impact is moderate. +- C044 [Low] Checkpoint reactor swallows diff errors silently for `turn.completed` (apps/server/src/orchestration/Layers/CheckpointReactor.ts:274) + - Note: Errors are swallowed to empty diff with warning; not fully silent but still lossy. +- C048 [Low] `statement.run()` returns a `RunResult` object, not an array. Casting it to `ReadonlyArray` will cause runtime errors when consumers call array methods. Consider wrapping the result (e.g., `[result]`) or returning a properly typed object. (apps/server/src/persistence/NodeSqliteClient.ts:99) + - Note: Unsafe run() result cast remains; runtime risk depends on raw-result consumers. +- C051 [Low] Using `??` for `providerThreadId` and `adapterKey` makes it impossible to clear these fields by passing `null`, since `null ?? existing` evaluates to `existing`. Consider using explicit `undefined` checks (like `resumeCursor` does) if clearing should be supported.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/provider/Layers/ProviderSessionDirectory.ts:119) + - Note: Null-clearing issue is real for providerThreadId; adapterKey part overstated. +- C056 [Low] When `onOpenChange` is provided without `open`, the internal `_open` state never updates because `setOpenProp` takes precedence. Consider calling `_setOpen` when `openProp === undefined`, regardless of whether `setOpenProp` exists. (apps/web/src/components/ui/sidebar.tsx:114) + - Note: Bug pattern exists, but current callsites mostly avoid triggering it. +- C059 [Low] Suggestion: don’t spread `params` into `body`; it can override `_tag` and mishandle non-object values. Keep `_tag` separate and nest `params` under a single key (e.g., `data`), or validate `params` is a plain object. (apps/web/src/wsTransport.ts:59) + - Note: Transport _tag override risk exists but current callsites are constrained. + +## Valid Items (actionable unless marked completed) + +- C001 [High] Non-atomic event appending can corrupt state on retry. If an error occurs mid-loop (lines 96-102) after some events are persisted but before the receipt is written, the command appears to fail. A retry generates new UUIDs via `crypto.randomUUID()` in the decider, appending duplicate events. Consider wrapping the loop in a transaction or using deterministic event IDs derived from `commandId`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:96) +- C002 [High] A dispatch error in `processEvent` will terminate the `Effect.forever` loop, permanently halting event ingestion. Consider adding error recovery (e.g., `Effect.catchAll` with logging) around `processEvent` so failures don't kill the fiber. (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:333) + - Status: Completed in Phase 1. +- C003 [High] Consider attaching a no-op error listener before `socket.write` (e.g., `socket.on('error', () => {})`) to prevent an unhandled `EPIPE`/`ECONNRESET` from crashing the process if the client disconnects mid-handshake. (apps/server/src/wsServer.ts:75) +- C009 [Medium] Git's braced rename syntax (e.g., `src/{old => new}/file.ts`) isn't handled correctly. The current slice after ` => ` produces invalid paths like `new}/file.ts`. Consider expanding the braces to construct the full destination path.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/git/Layers/GitCore.ts:41) +- C010 [Medium] `loadCustomKeybindingsConfig` fails when the config file doesn't exist, which is expected for new users. Consider catching `ENOENT` and returning an empty array instead.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/keybindings.ts:418) +- C011 [Medium] Template literal `${cause}` throws if `cause` is a `Symbol`. Consider using `String(cause)` instead. (apps/server/src/orchestration/Errors.ts:117) +- C012 [Medium] Forked revert dispatch risks read model inconsistency (apps/server/src/orchestration/Layers/CheckpointReactor.ts:542) + - Status: Completed in Phase 1. +- C013 [Medium] If `projectionPipeline.projectEvent` fails after `eventStore.append` succeeds, the event is persisted but `readModel` isn't updated, causing desync. Consider updating the in-memory `readModel` immediately after append (before the external projection), so local state stays consistent regardless of downstream failures. (apps/server/src/orchestration/Layers/OrchestrationEngine.ts:99) +- C016 [Medium] The in-memory `pendingTurnStartByThreadId` map isn't restored during bootstrap. If the service restarts after processing `thread.turn-start-requested` but before `thread.session-set`, the `userMessageId` and `startedAt` will be lost since bootstrap resumes *after* the committed sequence. Consider persisting this pending state or processing these two events atomically.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProjectionPipeline.ts:490) +- C018 [Medium] Unbounded memory growth in turn start deduplication set (apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:84) +- C022 [Medium] Fish shell outputs `$PATH` as space-separated, not colon-separated. Consider checking if the shell is fish and using `string join : $PATH` instead, or validating the result contains colons before assigning. (apps/server/src/os-jank.ts:10) +- C023 [Medium] Using `-il` flags causes the shell to source profile scripts that may print banners or other text, polluting the captured `PATH`. Consider using `-lc` (login only, non-interactive) to reduce unwanted output. (apps/server/src/os-jank.ts:10) +- C026 [Medium] Consider adding `.catch(() => {})` after `Effect.runPromise(handleMessage(ws, raw))` to prevent unhandled rejections from crashing the server if `encodeResponse` or setup logic fails. (apps/server/src/wsServer.ts:545) + - Status: Completed in Phase 1. +- C027 [Medium] WS message handler can cause unhandled promise rejection (apps/server/src/wsServer.ts:545) + - Status: Completed in Phase 1. +- C029 [Medium] `parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null. (apps/web/src/markdown-links.ts:105) +- C030 [Medium] `EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/web/src/markdown-links.ts:111) +- C032 [Low] Dummy workflow marker accidentally committed to docs (AGENTS.md:45) +- C034 [Low] Redundant `CheckpointInvariantError` in `CheckpointServiceError` union type (apps/server/src/checkpointing/Errors.ts:79) +- C035 [Low] Redundant error type in CheckpointServiceError union definition (apps/server/src/checkpointing/Errors.ts:79) +- C038 [Low] Multi-byte UTF-8 characters split across chunks will be corrupted when decoding each chunk separately. Consider accumulating all chunks first, then decoding once, or use `TextDecoder` with `stream: true`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/git/Layers/CodexTextGeneration.ts:136) +- C040 [Low] `upsertKeybindingRule` has a race condition: concurrent calls read the same file state, then the last write overwrites earlier changes. Consider wrapping the read-modify-write sequence with `Effect.Semaphore` to serialize access.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/keybindings.ts:488) +- C041 [Low] Template literal interpolation throws `TypeError` if `cause` is a `Symbol`. Consider using `String(cause)` for safe coercion. (apps/server/src/orchestration/Errors.ts:126) +- C043 [Low] Duplicated workspace CWD resolution logic across reactor modules (apps/server/src/orchestration/Layers/CheckpointReactor.ts:62) +- C045 [Low] `truncateDetail` slices to `limit - 1` then appends `"..."` (3 chars), producing strings of length `limit + 2`. Consider slicing to `limit - 3` instead.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:29) +- C046 [Low] `latestMessageIdByTurnKey` is written to but never read, and `clearAssistantMessageIdsForTurn` doesn't clear its entriesβ€”only `clearTurnStateForSession` does. Consider removing this map entirely if unused, or clearing it alongside `turnMessageIdsByTurnKey` in `clearAssistantMessageIdsForTurn`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:133) +- C047 [Low] `SqlSchema.findOneOption` can produce both SQL errors and decode errors, but `mapError` wraps all as `PersistenceSqlError`. Consider distinguishing `ParseError` from SQL errors and mapping decode failures to `PersistenceDecodeError` instead.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/persistence/Layers/OrchestrationCommandReceipts.ts:75) +- C049 [Low] `JSON.stringify(cause)` returns `undefined` for `undefined`, functions, or symbols, violating the `string` return type. Consider coercing the result to a string (e.g., `String(JSON.stringify(cause))`) or adding a fallback. (apps/server/src/provider/Layers/ProviderService.ts:59) +- C050 [Low] The read-modify-write pattern (`getBySessionId` β†’ merge β†’ `upsert`) is susceptible to lost updates under concurrent writes. Consider wrapping in a transaction or adding optimistic concurrency control (e.g., version field) if concurrent session updates are expected.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/provider/Layers/ProviderSessionDirectory.ts:94) +- C052 [Low] Race condition: `processHandle` may be `null` when `data` callback fires, since it's assigned after `Bun.spawn` returns. Consider initializing `BunPtyProcess` first, then passing it to the callback to avoid losing initial output.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/terminal/Layers/BunPTY.ts:97) +- C053 [Low] Consider using `socket.end(response)` instead of `socket.write(response)` + `socket.destroy()` to ensure the HTTP error response is fully flushed before closing the connection. (apps/server/src/wsServer.ts:83) +- C054 [Low] When array chunks contain a multi-byte UTF-8 character split across boundaries, decoding each chunk separately produces replacement characters. Consider using `Buffer.concat()` on all chunks before calling `.toString("utf8")`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/server/src/wsServer.ts:104) +- C057 [Low] The `resizable` object is recreated on every render, causing `SidebarRail`'s `useEffect` to repeatedly read localStorage and update the DOM. Consider memoizing the object with `useMemo`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/web/src/routes/_chat.$threadId.tsx:105) +- C058 [Low] When `localStorage.getItem()` returns `null`, `Number(null)` evaluates to `0`, which passes `Number.isFinite(0)`. This causes the sidebar to clamp to `minWidth` on first load, overriding the `DIFF_INLINE_DEFAULT_WIDTH` CSS clamp. Consider checking for `null` or empty string before parsing, e.g. guard with `storedWidth === null || storedWidth === ''`.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (apps/web/src/routes/_chat.$threadId.tsx:122) +- C060 [Low] `defaultModel` should be `Schema.optional(Schema.NullOr(Schema.String))` to allow clearing the value. Currently there's no way to reset it to `null` since omitting means "no change" in patch semantics.
πŸš€ Reply "fix it for me" or copy this AI Prompt for your agent: (packages/contracts/src/orchestration.ts:253) + +## By Area + +- Build/runtime portability: valid=0, partially-valid=2, invalid=0 +- Checkpointing correctness: valid=2, partially-valid=3, invalid=3 +- Edge-case parsing/platform behavior: valid=8, partially-valid=1, invalid=0 +- Event ordering and state consistency: valid=3, partially-valid=2, invalid=1 +- Memory/resource growth: valid=1, partially-valid=0, invalid=0 +- Other: valid=10, partially-valid=6, invalid=1 +- Runtime resilience and failure handling: valid=6, partially-valid=4, invalid=0 +- WebSocket robustness: valid=4, partially-valid=1, invalid=0 + +Machine-readable mapping: `.plans/16b-pr89-architecture-validity-audit.json`. diff --git a/apps/server/src/orchestration/Layers/CheckpointReactor.test.ts b/apps/server/src/orchestration/Layers/CheckpointReactor.test.ts index 99b531db04b11df219046ac0061eefc42b4eec19..2f5fa86711089486dc6964b9e80cf24312db9c85 100644 --- a/apps/server/src/orchestration/Layers/CheckpointReactor.test.ts +++ b/apps/server/src/orchestration/Layers/CheckpointReactor.test.ts @@ -42,10 +42,12 @@ const asSessionId = (value: string): ProviderSessionId => ProviderSessionId.make const asProviderThreadId = (value: string): ProviderThreadId => ProviderThreadId.makeUnsafe(value); const asTurnId = (value: string): TurnId => TurnId.makeUnsafe(value); -function createProviderServiceHarness(cwd: string, hasSession = true) { +function createProviderServiceHarness(cwd: string, hasSession = true, sessionCwd = cwd) { const now = new Date().toISOString(); const runtimeEventPubSub = Effect.runSync(PubSub.unbounded()); - const rollbackConversation = vi.fn(() => Effect.void); + const rollbackConversation = vi.fn( + (_input: { readonly sessionId: ProviderSessionId; readonly numTurns: number }) => Effect.void, + ); const unsupported = () => Effect.die(new Error("Unsupported provider call in test")) as Effect.Effect; @@ -57,7 +59,7 @@ function createProviderServiceHarness(cwd: string, hasSession = true) { provider: "codex", status: "ready", threadId: asProviderThreadId("provider-thread-1"), - cwd, + cwd: sessionCwd, createdAt: now, updatedAt: now, }, @@ -214,10 +216,15 @@ describe("CheckpointReactor", () => { readonly seedFilesystemCheckpoints?: boolean; readonly projectWorkspaceRoot?: string; readonly threadWorktreePath?: string | null; + readonly providerSessionCwd?: string; }) { const cwd = createGitRepository(); tempDirs.push(cwd); - const provider = createProviderServiceHarness(cwd, options?.hasSession ?? true); + const provider = createProviderServiceHarness( + cwd, + options?.hasSession ?? true, + options?.providerSessionCwd ?? cwd, + ); const orchestrationLayer = OrchestrationEngineLive.pipe( Layer.provide(OrchestrationProjectionPipelineLive), Layer.provide(OrchestrationEventStoreLive), @@ -496,6 +503,65 @@ describe("CheckpointReactor", () => { ); }); + it("continues processing runtime events after a single checkpoint runtime failure", async () => { + const nonRepositorySessionCwd = fs.mkdtempSync( + path.join(os.tmpdir(), "t3-checkpoint-runtime-non-repo-"), + ); + tempDirs.push(nonRepositorySessionCwd); + + const harness = await createHarness({ + seedFilesystemCheckpoints: false, + providerSessionCwd: nonRepositorySessionCwd, + }); + const createdAt = new Date().toISOString(); + + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.session.set", + commandId: CommandId.makeUnsafe("cmd-session-set-non-repo-runtime"), + threadId: ThreadId.makeUnsafe("thread-1"), + session: { + threadId: ThreadId.makeUnsafe("thread-1"), + status: "ready", + providerName: "codex", + providerSessionId: asSessionId("sess-1"), + providerThreadId: ProviderThreadId.makeUnsafe("provider-thread-1"), + activeTurnId: null, + lastError: null, + updatedAt: createdAt, + }, + createdAt, + }), + ); + + harness.provider.emit({ + type: "checkpoint.captured", + eventId: EventId.makeUnsafe("evt-runtime-capture-failure"), + provider: "codex", + sessionId: asSessionId("sess-1"), + createdAt: new Date().toISOString(), + threadId: ProviderThreadId.makeUnsafe("provider-thread-1"), + turnId: asTurnId("turn-runtime-failure"), + turnCount: 1, + status: "completed", + }); + + harness.provider.emit({ + type: "turn.started", + eventId: EventId.makeUnsafe("evt-turn-started-after-runtime-failure"), + provider: "codex", + sessionId: asSessionId("sess-1"), + createdAt: new Date().toISOString(), + threadId: ProviderThreadId.makeUnsafe("provider-thread-1"), + turnId: asTurnId("turn-after-runtime-failure"), + }); + + await waitForGitRefExists(harness.cwd, checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 0)); + expect( + gitRefExists(harness.cwd, checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 0)), + ).toBe(true); + }); + it("executes provider revert and emits thread.reverted for checkpoint revert requests", async () => { const harness = await createHarness(); const createdAt = new Date().toISOString(); @@ -575,6 +641,101 @@ describe("CheckpointReactor", () => { ); }); + it("processes consecutive revert requests with deterministic rollback sequencing", async () => { + const harness = await createHarness(); + const createdAt = new Date().toISOString(); + + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.session.set", + commandId: CommandId.makeUnsafe("cmd-session-set-inline-revert"), + threadId: ThreadId.makeUnsafe("thread-1"), + session: { + threadId: ThreadId.makeUnsafe("thread-1"), + status: "ready", + providerName: "codex", + providerSessionId: asSessionId("sess-1"), + providerThreadId: ProviderThreadId.makeUnsafe("provider-thread-1"), + activeTurnId: null, + lastError: null, + updatedAt: createdAt, + }, + createdAt, + }), + ); + + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.turn.diff.complete", + commandId: CommandId.makeUnsafe("cmd-inline-revert-diff-1"), + threadId: ThreadId.makeUnsafe("thread-1"), + turnId: asTurnId("turn-1"), + completedAt: createdAt, + checkpointRef: checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 1), + status: "ready", + files: [], + checkpointTurnCount: 1, + createdAt, + }), + ); + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.turn.diff.complete", + commandId: CommandId.makeUnsafe("cmd-inline-revert-diff-2"), + threadId: ThreadId.makeUnsafe("thread-1"), + turnId: asTurnId("turn-2"), + completedAt: createdAt, + checkpointRef: checkpointRefForThreadTurn(ThreadId.makeUnsafe("thread-1"), 2), + status: "ready", + files: [], + checkpointTurnCount: 2, + createdAt, + }), + ); + + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.checkpoint.revert", + commandId: CommandId.makeUnsafe("cmd-sequenced-revert-request-1"), + threadId: ThreadId.makeUnsafe("thread-1"), + turnCount: 1, + createdAt, + }), + ); + await Effect.runPromise( + harness.engine.dispatch({ + type: "thread.checkpoint.revert", + commandId: CommandId.makeUnsafe("cmd-sequenced-revert-request-0"), + threadId: ThreadId.makeUnsafe("thread-1"), + turnCount: 0, + createdAt, + }), + ); + + const deadline = Date.now() + 2000; + const waitForRollbackCalls = async (): Promise => { + if (harness.provider.rollbackConversation.mock.calls.length >= 2) { + return; + } + if (Date.now() >= deadline) { + throw new Error("Timed out waiting for rollbackConversation calls."); + } + await new Promise((resolve) => setTimeout(resolve, 10)); + return waitForRollbackCalls(); + }; + await waitForRollbackCalls(); + + expect(harness.provider.rollbackConversation).toHaveBeenCalledTimes(2); + expect(harness.provider.rollbackConversation.mock.calls[0]?.[0]).toEqual({ + sessionId: asSessionId("sess-1"), + numTurns: 1, + }); + expect(harness.provider.rollbackConversation.mock.calls[1]?.[0]).toEqual({ + sessionId: asSessionId("sess-1"), + numTurns: 1, + }); + }); + it("appends an error activity when revert is requested without an active session", async () => { const harness = await createHarness({ hasSession: false }); const createdAt = new Date().toISOString(); diff --git a/apps/server/src/orchestration/Layers/CheckpointReactor.ts b/apps/server/src/orchestration/Layers/CheckpointReactor.ts index 234bb6f95cbf5a8da67346a79869e8c1d459eb6d..a0fa7dacc42961196dee50e594c71af2a630c255 100644 --- a/apps/server/src/orchestration/Layers/CheckpointReactor.ts +++ b/apps/server/src/orchestration/Layers/CheckpointReactor.ts @@ -9,7 +9,7 @@ import { type OrchestrationEvent, type ProviderRuntimeEvent, } from "@t3tools/contracts"; -import { Effect, Layer, Option, Queue, Stream } from "effect"; +import { Cause, Effect, Layer, Option, Queue, Stream } from "effect"; import { parseTurnDiffFilesFromUnifiedDiff } from "../../checkpointing/Diffs.ts"; import { checkpointRefForThreadTurn } from "../../checkpointing/Refs.ts"; @@ -537,7 +537,6 @@ const make = Effect.gen(function* () { createdAt: now, }), ), - Effect.forkScoped, Effect.asVoid, ); }); @@ -672,11 +671,27 @@ const make = Effect.gen(function* () { ): Effect.Effect => input.source === "domain" ? processDomainEvent(input.event) : processRuntimeEvent(input.event); + const processInputSafely = (input: ReactorInput) => + processInput(input).pipe( + Effect.catchCause((cause) => { + if (Cause.hasInterruptsOnly(cause)) { + return Effect.failCause(cause); + } + return Effect.logWarning("checkpoint reactor failed to process input", { + source: input.source, + eventType: input.event.type, + cause: Cause.pretty(cause), + }); + }), + ); + const start: CheckpointReactorShape["start"] = Effect.gen(function* () { const queue = yield* Queue.unbounded(); yield* Effect.addFinalizer(() => Queue.shutdown(queue).pipe(Effect.asVoid)); - yield* Effect.forkScoped(Effect.forever(Queue.take(queue).pipe(Effect.flatMap(processInput)))); + yield* Effect.forkScoped( + Effect.forever(Queue.take(queue).pipe(Effect.flatMap(processInputSafely))), + ); yield* Effect.forkScoped( Stream.runForEach(orchestrationEngine.streamDomainEvents, (event) => { diff --git a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts index d39fe290be189e7b484950ee658db567f8ca0841..df97787a949d504a843455e945628140e038808e 100644 --- a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts +++ b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts @@ -327,4 +327,40 @@ describe("ProviderRuntimeIngestion", () => { expect(thread.session?.status).toBe("ready"); expect(thread.activities.some((activity) => activity.kind === "tool.started")).toBe(true); }); + + it("continues processing runtime events after a single event handler failure", async () => { + const harness = await createHarness(); + const now = new Date().toISOString(); + + harness.emit({ + type: "message.delta", + eventId: asEventId("evt-invalid-delta"), + provider: "codex", + sessionId: asSessionId("sess-1"), + createdAt: now, + turnId: asProviderTurnId("turn-invalid"), + itemId: asItemId("item-invalid"), + delta: undefined, + } as unknown as ProviderRuntimeEvent); + + harness.emit({ + type: "runtime.error", + eventId: asEventId("evt-runtime-error-after-failure"), + provider: "codex", + sessionId: asSessionId("sess-1"), + createdAt: new Date().toISOString(), + turnId: asProviderTurnId("turn-after-failure"), + message: "runtime still processed", + }); + + const thread = await waitForThread( + harness.engine, + (entry) => + entry.session?.status === "error" && + entry.session?.activeTurnId === "turn-after-failure" && + entry.session?.lastError === "runtime still processed", + ); + expect(thread.session?.status).toBe("error"); + expect(thread.session?.lastError).toBe("runtime still processed"); + }); }); diff --git a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts index a3ab7e7c786874a3e6cbd93b7dbee189af3f21d2..7ac968384f970d8ada99fd8706e843c531bd8db8 100644 --- a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts +++ b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts @@ -7,7 +7,7 @@ import { type ProviderRuntimeEvent, type ProviderSessionId, } from "@t3tools/contracts"; -import { Effect, Layer, Queue, Stream } from "effect"; +import { Cause, Effect, Layer, Queue, Stream } from "effect"; import { ProviderService } from "../../provider/Services/ProviderService.ts"; import { OrchestrationEngineService } from "../Services/OrchestrationEngine.ts"; @@ -324,13 +324,27 @@ const make = Effect.gen(function* () { ).pipe(Effect.asVoid); }); + const processEventSafely = (event: ProviderRuntimeEvent) => + processEvent(event).pipe( + Effect.catchCause((cause) => { + if (Cause.hasInterruptsOnly(cause)) { + return Effect.failCause(cause); + } + return Effect.logWarning("provider runtime ingestion failed to process event", { + eventId: event.eventId, + eventType: event.type, + cause: Cause.pretty(cause), + }); + }), + ); + const start: ProviderRuntimeIngestionShape["start"] = Effect.gen(function* () { const providerEventQueue = yield* Queue.unbounded(); yield* Effect.addFinalizer(() => Queue.shutdown(providerEventQueue).pipe(Effect.asVoid)); yield* Effect.forkScoped( - Effect.forever(Queue.take(providerEventQueue).pipe(Effect.flatMap(processEvent))), + Effect.forever(Queue.take(providerEventQueue).pipe(Effect.flatMap(processEventSafely))), ); yield* Effect.forkScoped( Stream.runForEach(providerService.streamEvents, (event) => diff --git a/apps/server/src/wsServer.test.ts b/apps/server/src/wsServer.test.ts index 121b75d07d98a333d11c93d6e3f2ceccfa47b4fc..b0057acd69b00436fd921dca23d98f79489c2264 100644 --- a/apps/server/src/wsServer.test.ts +++ b/apps/server/src/wsServer.test.ts @@ -1000,6 +1000,65 @@ describe("WebSocket Server", () => { expect(response!.error!.message).toContain("Invalid request format"); }); + it("catches websocket message handler rejections and keeps the socket usable", async () => { + const unhandledRejections: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => { + unhandledRejections.push(reason); + }; + process.on("unhandledRejection", onUnhandledRejection); + + const brokenOpenService: OpenShape = { + openBrowser: () => Effect.void, + openInEditor: () => + Effect.sync(() => BigInt(1)).pipe(Effect.map((result) => result as unknown as void)), + }; + + try { + server = await createTestServer({ cwd: "/test", open: brokenOpenService }); + const addr = server.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + + const ws = await connectWs(port); + connections.push(ws); + await waitForMessage(ws); + + ws.send( + JSON.stringify({ + id: "req-broken-open", + body: { + _tag: WS_METHODS.shellOpenInEditor, + cwd: "/tmp", + editor: "cursor", + }, + }), + ); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toHaveLength(0); + + const workspace = makeTempDir("t3code-ws-handler-still-usable-"); + fs.writeFileSync(path.join(workspace, "file.txt"), "ok\n", "utf8"); + const response = await sendRequest(ws, WS_METHODS.projectsSearchEntries, { + cwd: workspace, + query: "file", + limit: 5, + }); + expect(response.error).toBeUndefined(); + expect(response.result).toEqual( + expect.objectContaining({ + entries: expect.arrayContaining([ + expect.objectContaining({ + path: "file.txt", + kind: "file", + }), + ]), + }), + ); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + } + }); + it("returns errors for removed projects CRUD methods", async () => { server = await createTestServer({ cwd: "/test" }); const addr = server.address(); diff --git a/apps/server/src/wsServer.ts b/apps/server/src/wsServer.ts index 2afceede57daaa793dd0e99ad5b722df85e128f2..1b0a8ec1e16ac51553df34a8596b57b74f81f5c8 100644 --- a/apps/server/src/wsServer.ts +++ b/apps/server/src/wsServer.ts @@ -542,7 +542,11 @@ export const createServer = Effect.fn(function* (options: ServerOptions = {}): E ws.send(JSON.stringify(welcome)); ws.on("message", (raw) => { - void Effect.runPromise(handleMessage(ws, raw)); + void Effect.runPromise( + handleMessage(ws, raw).pipe( + Effect.catch((error) => Effect.logError("Error handling message", error)), + ), + ); }); ws.on("close", () => {