Skip to content

fix: resolve 6 critical bugs from deep code review (round 2)#337

Merged
jackwener merged 1 commit intomainfrom
fix/critical-bugs-round2
Mar 24, 2026
Merged

fix: resolve 6 critical bugs from deep code review (round 2)#337
jackwener merged 1 commit intomainfrom
fix/critical-bugs-round2

Conversation

@jackwener
Copy link
Owner

Summary

Fixes the remaining 6 Critical issues from the deep code review (round 1 fixed security issues in PR #335).

  • execution.ts: Guard lazy-loaded func commands against null page — throw a clear CommandExecutionError instead of cryptic TypeError when a lazy module incorrectly requires browser context
  • daemon.ts: Fix readBody race condition — add aborted flag to prevent req.destroy() from triggering both reject and resolve on the same Promise (truncated data processing)
  • browser/cdp.ts: Prevent CDPBridge.connect() reentry — throw if already connected instead of silently leaking the previous WebSocket
  • interceptor.ts: Store intercept pattern in a separate global so installInterceptor with a new pattern updates the match condition instead of being blocked by patchGuard
  • record.ts: Always call cleanupEnter() after Promise.race — previously only called in timeout path, leaving readline open on Enter exit. Removed dead enterRace variable
  • generate.ts: Fix undefined entering String.includes()c.name?.toLowerCase() returns undefined which gets coerced to string "undefined", causing false positive candidate matches

Test plan

  • All 330 unit tests pass
  • TypeScript typecheck passes
  • Manual: verify interceptor pattern update works across multiple installInterceptor calls
  • Manual: verify record mode exits cleanly on Enter press

🤖 Generated with Claude Code

1. execution.ts: Guard lazy-loaded func commands against null page — if a
   lazy module incorrectly requires browser context, throw a clear error
   instead of a cryptic TypeError on page.goto().

2. daemon.ts: Fix readBody race condition — add aborted flag to prevent
   req.destroy() from triggering both reject (via error) and resolve
   (via end event) on the same Promise, which could process truncated data.

3. browser/cdp.ts: Prevent CDPBridge.connect() reentry — throw if already
   connected instead of silently leaking the previous WebSocket and its
   message handlers.

4. interceptor.ts: Store intercept pattern in a separate global variable
   so subsequent installInterceptor calls with different patterns update
   the match condition without being blocked by the patchGuard.

5. record.ts: Always call cleanupEnter() after Promise.race — previously
   only called in the timeout path, leaving readline open when user pressed
   Enter, potentially blocking process exit. Also removed unused enterRace.

6. generate.ts: Fix undefined entering String.includes() — when c.name is
   undefined, toLowerCase() returns undefined which gets coerced to the
   string "undefined" by includes(), causing false positive matches.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jackwener jackwener merged commit bdcffd1 into main Mar 24, 2026
11 of 12 checks passed
@jackwener
Copy link
Owner Author

Reviewed and merged. The 6 critical fixes are coherent, regression risk is low, and local validation passed (npm run typecheck, npm test). Thanks for the fast follow-up.

jackwener added a commit that referenced this pull request Mar 24, 2026
The C2 fix in PR #337 added a null-page guard after lazy-loading TS
modules, but it threw unconditionally — breaking all browser:false
commands (bloomberg, apple-podcasts, google, yollomi, etc.) that
use func() with a null page. Guard now checks updated.browser !== false.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jackwener added a commit that referenced this pull request Mar 24, 2026
The C2 fix in PR #337 added a null-page guard after lazy-loading TS
modules, but it threw unconditionally — breaking all browser:false
commands (bloomberg, apple-podcasts, google, yollomi, etc.) that
use func() with a null page. Guard now checks updated.browser !== false.

Also fixes apple-podcasts top E2E flake: when the command times out on
CI, stderr is empty and the guard didn't catch it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jackwener added a commit that referenced this pull request Mar 24, 2026
…#347)

The C2 fix in PR #337 added a null-page guard after lazy-loading TS
modules, but it threw unconditionally — breaking all browser:false
commands (bloomberg, apple-podcasts, google, yollomi, etc.) that
use func() with a null page. Guard now checks updated.browser !== false.

Also fixes apple-podcasts top E2E flake: when the command times out on
CI, stderr is empty and the guard didn't catch it.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant