Skip to content

fix(browser): preserve explicit cdpPort when cdpUrl omits port#83707

Merged
clawsweeper[bot] merged 4 commits into
mainfrom
clawsweeper/automerge-openclaw-openclaw-82166
May 18, 2026
Merged

fix(browser): preserve explicit cdpPort when cdpUrl omits port#83707
clawsweeper[bot] merged 4 commits into
mainfrom
clawsweeper/automerge-openclaw-openclaw-82166

Conversation

@clawsweeper
Copy link
Copy Markdown
Contributor

@clawsweeper clawsweeper Bot commented May 18, 2026

Makes #82166 merge-ready for the ClawSweeper automerge loop.
The edit pass should inspect the live PR diff, review comments, and failing checks; rebase if needed; keep the contributor branch credited; and stop only when validation is green or an external blocker is proven.

ClawSweeper 🐠 replacement reef notes:

  • Repair fallback: GitHub rejected the repair branch push because it updates workflow files and the ClawSweeper app token does not have workflows permission

Co-author credit kept:

fish notes: model gpt-5.5, reasoning high; reviewed against 92dca8f.

@clawsweeper clawsweeper Bot added size: M clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge proof: supplied External PR includes structured after-fix real behavior proof. proof: sufficient ClawSweeper judged the real behavior proof convincing. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. clawsweeper Tracked by ClawSweeper automation labels May 18, 2026
@openclaw-barnacle openclaw-barnacle Bot removed proof: supplied External PR includes structured after-fix real behavior proof. proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 18, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor Author

clawsweeper Bot commented May 18, 2026

Codex review: passed.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR adds raw explicit-port detection for browser CDP URLs, updates profile resolution precedence, adds regression tests, and records the browser fix in the changelog.

Reproducibility: yes. Source inspection shows current main resolves a portless profile cdpUrl through parseBrowserHttpUrl, receives 80 or 443, and overwrites the configured cdpPort; the source PR also provides live before/after Chrome output.

PR rating
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Summary: Good focused bug-fix PR with strong real behavior proof, targeted regression tests, and one explicit compatibility choice for maintainers to accept.

Rank-up moves:

  • none
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

PR egg
✨ Hatched: 🥚 common Frosted Review Wisp

       /\  .---.  /\         
      /  \/     \/  \        
     /   ( -   - )   \       
    |       ._.       |      
    |   /|  ===  |\   |      
     \  \|______/|/  /       
      '._  `--'  _.'         
         '-.__.-'            
       _/|_|  |_|\_          
      /__|      |__\         
       .-----------.         
      '-------------'        

Rarity: 🥚 common.
Trait: stacks clean commits.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Frosted Review Wisp in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • How to hatch it: reach status: 👀 ready for maintainer look or status: 🚀 automerge armed; that usually means sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

Real behavior proof
Sufficient (live_output): The linked source PR body includes live terminal output from a real macOS Chrome run showing the before-fix port-80 failure and after-fix CDP response on port 18800.

Risk before merge
Why this matters: - Existing configs that intentionally relied on a portless cdpUrl plus a different cdpPort resolving to protocol default 80 or 443 will now use the configured cdpPort; those users must write :80 or :443 explicitly.

Maintainer options:

  1. Accept explicit-port precedence (recommended)
    Merge as-is if maintainers agree that portless cdpUrl should defer to configured cdpPort and users who want default 80 or 443 should write the port explicitly.
  2. Document the precedence first
    Add a short docs or release-note sentence explaining explicit URL port > configured cdpPort > protocol default before merge if maintainers want the upgrade behavior more discoverable.

Next step before merge
No repair lane is needed: this automerge-opted replacement PR has no blocking findings, and CI/mergeability plus the compatibility-risk decision should gate merge.

Security
Cleared: The diff changes browser CDP URL normalization, profile resolution, tests, and changelog only; it adds no dependency, workflow, permission, secret-handling, or new code-execution surface.

Review details

Best possible solution:

Land the browser parser/profile-resolution fix with the regression tests once maintainers accept the explicit-port precedence rule; add a short docs or release-note sentence only if maintainers want that upgrade behavior spelled out.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main resolves a portless profile cdpUrl through parseBrowserHttpUrl, receives 80 or 443, and overwrites the configured cdpPort; the source PR also provides live before/after Chrome output.

Is this the best way to solve the issue?

Yes. The fix belongs in browser profile resolution and URL parsing because that is where URL-derived defaults currently replace configured profile ports; the helper handles WHATWG default-port normalization and the tests cover the edge cases.

Label justifications:

  • P2: This is a normal-priority browser integration bug fix with a clear source path and limited blast radius.
  • merge-risk: 🚨 compatibility: Merging changes how existing configs with both a portless cdpUrl and a configured cdpPort resolve during upgrade.

What I checked:

  • Current main source reproduces the bug path: On current main, resolveProfile always assigns cdpPort = parsed.port whenever rawProfileUrl is set, while parseBrowserHttpUrl returns protocol defaults 80 or 443 when the URL omits a port. (extensions/browser/src/browser/config.ts:503, 06a39015f21c)
  • PR diff fixes the precedence in the implicated path: The PR makes parseBrowserHttpUrl report whether a port was explicitly written, preserves default ports that WHATWG URL normalization would otherwise drop, and changes resolveProfile to use explicit URL port > configured cdpPort > protocol default. (extensions/browser/src/browser/config.ts:502, 070c31cdcfad)
  • Regression tests cover the behavior matrix: The PR adds tests for explicit non-default and default URL ports, portless URLs deferring to configured cdpPort, no-cdpPort protocol defaults, userinfo, IPv6, and the stale WebSocket path. (extensions/browser/src/browser/config.test.ts:556, 070c31cdcfad)
  • Documented config surface matches the affected behavior: Browser docs show local profiles configured with cdpPort and remote profiles configured with cdpUrl, which places the fix at the profile-resolution boundary rather than in launcher code. Public docs: docs/tools/browser.md. (docs/tools/browser.md:195, 06a39015f21c)
  • Real behavior proof is supplied through the source PR: The linked source PR body includes copied live terminal output from macOS Chrome showing current-main resolution to port 80 with bind() failed: Permission denied, and after-fix resolution to port 18800 with a responding CDP endpoint. (070c31cdcfad)
  • Validation recorded by automation: The ClawSweeper automerge timeline reports pnpm check:changed, the focused browser config Vitest wrapper, pnpm lint, and pnpm check:test-types after the branch repair at the current head. (070c31cdcfad)

Likely related people:

  • steipete: git shortlog --all shows Peter Steinberger with the largest share of recent commits in extensions/browser/src/browser/config.ts, cdp.helpers.ts, and config.test.ts, including the bundled browser plugin ownership refactor. (role: feature owner / heavy area contributor; confidence: high; commits: 8eeb7f082975, 471d056e2f0d, 1fd049e3074c; files: extensions/browser/src/browser/config.ts, extensions/browser/src/browser/cdp.helpers.ts, extensions/browser/src/browser/config.test.ts)
  • Han Yang: Commit 68ee311 added the nearby stale-WebSocket branch where configured cdpPort already wins over a stale cdpUrl, which is the closest prior precedence behavior. (role: adjacent behavior contributor; confidence: medium; commits: 68ee3113a9fa; files: extensions/browser/src/browser/config.ts, extensions/browser/src/browser/config.test.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 06a39015f21c.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 18, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor Author

clawsweeper Bot commented May 18, 2026

🦞✅
ClawSweeper merged this PR after the passing review.

Source: clawsweeper[bot]
Feedback: structured ClawSweeper verdict: pass (sha=070c31cdcfad0b718a55d760a0632f86c0e99a98)
Merge status: merged by ClawSweeper automerge
Merged at: 2026-05-18T18:20:56Z
Merge commit: 3e6f7494af75

What merged:

  • The PR adds raw explicit-port detection for browser CDP URLs, updates profile resolution precedence, adds regression tests, and records the browser fix in the changelog.
  • Reproducibility: yes. Source inspection shows current main resolves a portless profile cdpUrl through par ... 443, and overwrites the configured cdpPort`; the source PR also provides live before/after Chrome output.

Automerge notes:

  • PR branch already contained follow-up commit before automerge: fix(browser): encapsulate explicit-port detection in parseBrowserHttpUrl
  • PR branch already contained follow-up commit before automerge: fix(browser): preserve explicit cdpPort when cdpUrl omits port

The automerge loop is complete.

Automerge progress:

  • 2026-05-18 17:59:26 UTC review queued 92dca8fde03a (queued)
  • 2026-05-18 18:14:24 UTC review queued 070c31cdcfad (after repair)
  • 2026-05-18 18:20:43 UTC review passed 070c31cdcfad (structured ClawSweeper verdict: pass (sha=070c31cdcfad0b718a55d760a0632f86c0e99...)
  • 2026-05-18 18:20:59 UTC merged 070c31cdcfad (merged by ClawSweeper automerge)

Marvae and others added 4 commits May 18, 2026 18:07
When a browser profile sets both cdpPort (e.g. 18800) and a cdpUrl
without an explicit port (e.g. "http://127.0.0.1"), resolveProfile
unconditionally overwrote cdpPort with the URL's protocol default (80).
This caused Chrome to attempt binding port 80, which fails without root
privileges.

Now cdpPort from the URL only overrides an existing cdpPort when the URL
contains an explicit port. When the URL omits the port and cdpPort is
already set, the existing value is preserved.
The previous implementation checked parsed.parsed.port in resolveProfile,
which missed WHATWG-normalized default ports (:80/:443) and was vulnerable
to false positives on IPv6 hosts and userinfo colons.

Move explicit-port detection into parseBrowserHttpUrl as hasExplicitPort
using a robust raw-string parser that handles:
- IPv6 bracket notation ([::1] vs [::1]:9222)
- userinfo (user:pass@host — colon is not a port)
- WHATWG default-port normalization (:80/:443 → empty .port)

Return normalizedWithPort alongside normalized so callers never need
raw-string URL hacks. resolveProfile now reads:
  explicit URL port → use it
  no URL port + cdpPort → inject cdpPort
  no URL port + no cdpPort → protocol default

Consolidate scattered port-precedence tests into a focused describe block
covering all 9 cases including IPv6, default ports, stale WS, and error.
@clawsweeper clawsweeper Bot force-pushed the clawsweeper/automerge-openclaw-openclaw-82166 branch from 92dca8f to 070c31c Compare May 18, 2026 18:14
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 18, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 18, 2026
@clawsweeper clawsweeper Bot merged commit 3e6f749 into main May 18, 2026
104 checks passed
@clawsweeper clawsweeper Bot deleted the clawsweeper/automerge-openclaw-openclaw-82166 branch May 18, 2026 18:20
eleqtrizit pushed a commit to eleqtrizit/openclaw that referenced this pull request May 18, 2026
…law#83707)

Summary:
- The PR adds raw explicit-port detection for browser CDP URLs, updates profile resolution precedence, adds regression tests, and records the browser fix in the changelog.
- Reproducibility: yes. Source inspection shows current main resolves a portless profile `cdpUrl` through `par ...  443, and overwrites the configured `cdpPort`; the source PR also provides live before/after Chrome output.

Automerge notes:
- PR branch already contained follow-up commit before automerge: fix(browser): encapsulate explicit-port detection in parseBrowserHttpUrl
- PR branch already contained follow-up commit before automerge: fix(browser): preserve explicit cdpPort when cdpUrl omits port

Validation:
- ClawSweeper review passed for head 070c31c.
- Required merge gates passed before the squash merge.

Prepared head SHA: 070c31c
Review: openclaw#83707 (comment)

Co-authored-by: Hongwei Ma <marvae24@gmail.com>
Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com>
Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
Approved-by: takhoffman
Co-authored-by: takhoffman <781889+takhoffman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge clawsweeper Tracked by ClawSweeper automation merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant