Skip to content

fix: concurrency bugs, license, docs, and CI#1

Merged
desperatee merged 2 commits into
mainfrom
fix/review-bugs-and-docs
Jun 30, 2026
Merged

fix: concurrency bugs, license, docs, and CI#1
desperatee merged 2 commits into
mainfrom
fix/review-bugs-and-docs

Conversation

@desperatee

Copy link
Copy Markdown
Contributor

Summary

  • Fix 2 HIGH-severity concurrency bugs found in code review (routeInjecting race, pipeTransport.Close race)
  • Fix global logger mutation that caused log output corruption with multiple concurrent drivers
  • Fix transformRunOptions mutating caller's shared *RunOptions in-place
  • Fix Verbose default being silently lost when caller passes any RunOptions
  • Add isolatedContext: true to all remaining evaluate methods
  • Change LICENSE from MIT to Apache-2.0 (matching Patchright upstream)
  • Add LLM-friendly usage guide with anti-detection best practices
  • Add GitHub Actions CI workflow

Test plan

  • All unit tests pass
  • Integration test (driver install + browser launch) passes
  • E2E test navigating walmart.ca with anti-detection config works
  • go build ./... and go vet ./... clean
  • navigator.webdriver returns false

Review findings addressed

# Severity Issue Fix
1 HIGH routeInjecting race on concurrent AddInitScript/ExposeBinding atomic.Bool.CompareAndSwap
2 HIGH Dead code in route handler (both branches identical) Simplified to single Fallback call
3 HIGH Global logger mutation across concurrent drivers Driver reads d.options.Logger; package-level logger never mutated
4 HIGH transformRunOptions mutates caller's *RunOptions Clone before defaulting
5 MEDIUM Verbose: true default lost when caller passes options Fixed by cloning defaults struct
6 MEDIUM pipeTransport.Close() race on channel close sync.Once (consistent with rest of codebase)
7 MEDIUM downloadPatchrightCore duplicates safeJoin traversal guard Reuse safeJoin
8 LOW EvalOnSelector missing isolatedContext Added to all eval methods

Code review fixes:
- Fix routeInjecting race condition: use atomic.Bool.CompareAndSwap
  instead of bare bool read-check-write
- Fix pipeTransport.Close() race: replace manual channel-close check
  with sync.Once (consistent with rest of codebase)
- Fix global logger mutation: driver.log() now reads d.options.Logger
  instead of mutating package-level var. Default logger writes to
  options.Stderr via slog.TextHandler
- Fix transformRunOptions mutating caller's *RunOptions: clone before
  applying defaults so concurrent callers sharing one RunOptions don't
  race
- Fix Verbose default being lost when caller passes any RunOptions
- Add isolatedContext: true to EvalOnSelector and element_handle eval
  methods for consistency with all other evaluate methods
- Reuse safeJoin in downloadPatchrightCore instead of duplicating
  traversal guard
- Remove dead code in installInjectRoute (both if/else branches were
  identical)

Other changes:
- LICENSE: change from MIT to Apache-2.0 (matching Patchright upstream)
- Add docs/llm-guide.md: concise LLM-friendly reference with
  anti-detection best practices
- Add .github/workflows/ci.yml: build, vet, unit + integration tests
  on Go 1.22 and 1.23
@desperatee desperatee merged commit 0151563 into main Jun 30, 2026
4 checks passed
@desperatee desperatee deleted the fix/review-bugs-and-docs branch June 30, 2026 23:37
desperatee added a commit that referenced this pull request Jul 1, 2026
Anti-bot solutions can still detect headless browsers via deep
fingerprinting (WebGL, navigator.plugins, screen dimensions) even
with Patchright patches. Verified: headful passes Walmart PerimeterX
on product pages while headless gets blocked.

- README: add headless vs headful section, update quick start to headful
- LLM guide: headful as rule #1, add troubleshooting section
- Example: default to headful
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