Skip to content

Commit 4504bf2

Browse files
committed
docs(review): log second code review status
Ran another comparison of feature/interrupt-support versus main. Documented that the prior findings (Gemini close handler and Codex cancellation labeling) have been fixed and noted that no new issues were discovered during this pass.
1 parent eaa5966 commit 4504bf2

File tree

1 file changed

+9
-2
lines changed

1 file changed

+9
-2
lines changed

CODE_REVIEW.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,16 @@
22

33
## Findings
44

5+
## Review #1 Findings (addressed)
6+
57
1. **Gemini stream close handler swallows CLI exit failures (packages/gemini-adapter/src/index.ts:250-285)**
6-
The new `handleClose` marks the stream as `finished` and enqueues `DONE` as soon as `readline` closes. Because the `child.once('exit', …)` handler bails out when `finished` is set, any subsequent non-zero exit status (or signal) is never surfaced to the iterator. As a result, Gemini CLI crashes now look like successful completions: consumers get `DONE` with no `error`, and the underlying process may continue running until the cleanup logic eventually sends SIGTERM. Please defer setting `finished` until the `exit` event runs (or forward the exit status through the close handler) so that we still emit the `Error` event when the CLI fails.
8+
The original `handleClose` marked the stream as `finished` immediately, preventing the `exit` handler from reporting non-zero exit codes or signals. Consumers therefore saw `DONE` with no error even when the CLI crashed. The fix should defer `finished` until the `exit` event runs (or only short-circuit when an abort is already in progress) so worker failures still surface errors.
79

810
2. **Codex adapter mislabels worker crashes as user cancellations (packages/codex-adapter/src/index.ts:373-399)**
9-
The worker exit handler now emits `createCancelledEvent(...)` for every unexpected exit path (signal, exit code 0 without `streamDone`, and non-zero exit codes) before pushing the error event. This means front-ends will always see a `cancelled` event even when the worker crashed or timed out, making it impossible to distinguish real user-initiated interrupts from infrastructure failures. Only emit the `cancelled` event when `active.aborted` is true; for other exit paths, emit the error event (and possibly `done`) without asserting `cancelled`.
11+
The worker exit handler emitted `createCancelledEvent(...)` for every unexpected exit path, making crashes indistinguishable from user cancellations. The adapter should only emit `cancelled` when `active.aborted` is true; other exits must emit the worker-exit error event without cancellation metadata.
12+
13+
Both findings have since been resolved in subsequent commits.
14+
15+
## Review #2 Findings (current)
1016

17+
No additional blocking issues were identified when re-reviewing `feature/interrupt-support` against `main`. The earlier Gemini and Codex regressions were fixed, and no new regressions or design gaps were observed in the remaining diffs (README/docs updates, new interrupt tests, and adapter refactors). Keep an eye on end-to-end CI once Codex worker changes roll out, but no corrective action is required from this review.

0 commit comments

Comments
 (0)