fix(browser): default Chrome profile selection without unsafe data-root discovery - #4539
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7b2fa6985
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lidation Profile mode rejected any call that omitted path/user_data_dir/profile_directory, so using a real Chrome profile required three hand-typed paths and usually failed argument validation first. The information was already discoverable: launch.ts resolves the installed Chrome/Chromium and profile-discovery.ts knows the per-OS user data roots. Reconstructed onto current dev (3d9c8f2) from PR #4539 head d7b2fa6. Lore-id: 7c41ad92 Constraint: keep the ownership guards -- never kill/relaunch a live profile Rejected: settings-based defaults | duplicates OS discovery already in-tree Confidence: high Scope-risk: narrow Reversibility: easy Tested: default resolution, explicit profile_directory, both failure paths Not-tested: real Chrome launch on win32/linux
Default executable discovery walks the shared Chromium candidate list, which ends in Microsoft Edge. With Chrome uninstalled but its user data directory still on disk, profile mode would have launched Edge against Chrome's profile root. Profile-mode discovery now skips Edge and only falls back to Chrome or Chromium. Reconstructed onto current dev (3d9c8f2) from PR #4539 head d7b2fa6. Lore-id: 4b8ef013 Constraint: headless launch keeps the unfiltered candidate order (Edge is a valid engine when no profile is involved) Confidence: high Scope-risk: narrow Reversibility: easy Tested: Edge/Chrome/Chromium path classification on posix and win32 forms Not-tested: real Edge-only machine end to end
d7b2fa6 to
4fc893d
Compare
Yeachan-Heo
changed the title
|
@codex review |
Recovery owner verdict: NEEDS-HUMANExact GitHub evidence at refreshed signing time:
Verdict remains NEEDS-HUMAN until a non-author collaborator approves this exact head and required product CI is green. Requested reviewers: Signed-off-by: GJC recovery owner recovery-owner@local |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc893db7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 869f5897fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 406d41f537
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b39be6fd31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8868c8d9be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6301e33e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae40acccc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daff3f17f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b56381d6b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2022e99690
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The supported Flatpak and Snap executables relocate their default user-data roots outside the host XDG config directory. Those roots must remain in the Chrome 136 fail-closed set or profile mode waits for a CDP endpoint Chrome will never expose. Lore-id: 9ae4c821 Constraint: custom sandbox data roots outside known defaults remain valid Confidence: high Scope-risk: narrow Reversibility: easy Tested: 33 focused contracts; 144 applicable browser tests; coding-agent check and build Not-tested: live Flatpak and Snap Chrome launches
The saved-profile example still pointed at macOS Chrome's live default data root, which the runtime now correctly rejects and Chrome 136 cannot debug. Show a dedicated persistent automation root and state the initialization and attachment boundaries explicitly. Lore-id: c6eb4db8 Constraint: documentation must not promise direct CDP launch of a daily default Chrome root Confidence: high Scope-risk: narrow Reversibility: easy Tested: focused browser contracts; coding-agent check and build
Final frozen review found that Linux Canary and injected Windows profile discovery were not fully governed by the platform contract. Add the Canary executable/root candidates and join discovered profile paths with the requested platform semantics rather than the host runner's separators. Lore-id: 2d436fa9 Constraint: cross-platform discovery remains pure and injectable Confidence: high Scope-risk: narrow Reversibility: easy Tested: 34 focused contracts; 145 applicable browser tests; coding-agent check and build Not-tested: live Linux Canary installation
Bun merges a repository's .env into process.env before browser modules load. Reading profile-root variables directly let project content redirect automatic profile discovery at repository-controlled state. Resolve LOCALAPPDATA and Chrome/XDG data-root overrides through the existing trusted environment boundary instead. Lore-id: 45c1af0e Constraint: repository .env never controls browser executable, proxy, certificates, or profile discovery roots Confidence: high Scope-risk: narrow Reversibility: easy Tested: 42 focused contracts; 153 applicable browser tests; coding-agent check and build Not-tested: interactive login-shell environment refresh
Exact-head review found two remaining contract mismatches: project dotenv could redirect Windows executable discovery, and the Edge-only guard still admitted other Chromium brands. Resolve Windows roots through the trusted environment boundary, positively allow only Chrome/Chromium executable names, and render the effective Default profile name. Lore-id: e59aa802 Constraint: non-Chrome Chromium browsers must use spawn mode with separate profiles Confidence: high Scope-risk: narrow Reversibility: easy Tested: 47 focused contracts; 158 applicable browser tests; coding-agent check and build Not-tested: live Brave/Vivaldi/Opera binaries on every platform
Keep the public docs, runtime prompt, and generated catalog aligned with the fail-closed executable-brand contract: Chrome profile mode admits only Chrome/Chromium and routes every other browser brand to separate spawn mode. Lore-id: 983c10de Constraint: generated tool catalog must match the static browser prompt Confidence: high Scope-risk: narrow Reversibility: easy Tested: 47 focused contracts; 158 applicable browser tests; coding-agent check and build
Snap's supported /snap/bin/chromium entrypoint resolves to the generic /usr/bin/snap dispatcher. Preserve that one canonical launcher identity while continuing to validate every other symlink by its resolved target, so renamed cross-brand browsers remain rejected. Lore-id: 107be534 Constraint: only /snap/bin/chromium bypasses resolved-target brand naming Confidence: high Scope-risk: narrow Reversibility: easy Tested: 48 focused contracts; 159 applicable browser tests; coding-agent check and build Not-tested: live Snap Chromium launch
The /snap/bin/chromium identity is safe only when canonicalization reaches the expected /usr/bin/snap dispatcher. Bind the exception to both paths so a misconfigured or hostile link to another browser remains rejected. Lore-id: 18ec655a Constraint: Snap launcher exception requires the canonical dispatcher target Confidence: high Scope-risk: narrow Reversibility: easy Tested: 48 focused contracts; 159 applicable browser tests; coding-agent check and build
Linux Chrome and Snap launchers do not remain the executable image recorded by the kernel, so exact-path process lookup misses an already-authorized CDP profile. When exact lookup is empty, scan stable Linux process references and admit only Chrome/Chromium argv matching the exact profile plus loopback CDP guards. Lore-id: 5c60d8c3 Constraint: fallback process scans require Chrome-branded argv and exact guarded profile arguments Confidence: high Scope-risk: medium Reversibility: easy Tested: 50 focused contracts; 161 applicable browser tests; coding-agent check and build Not-tested: live Google Chrome and Snap Chromium wrapper reuse
The Linux wrapper fallback must not trust caller-controlled argv zero or block Bun's main thread while enumerating procfs. Read /proc asynchronously and require the kernel-derived executable image to be Chrome/Chromium before applying the exact profile and loopback-CDP checks. Lore-id: 73cf82bb Constraint: fallback reuse never trusts argv identity and never performs synchronous procfs directory I/O Confidence: high Scope-risk: medium Reversibility: easy Tested: 50 focused contracts; 161 applicable browser tests; coding-agent check and build Not-tested: high-process-count procfs latency under production load
Cancellation must stop the detached acquire path before it can inspect more processes or launch an authenticated profile. Check the browser signal before and after asynchronous procfs reads and throughout candidate evaluation. Lore-id: f3d4dc1a Constraint: an aborted browser open never continues to profile launch or cache insertion Confidence: high Scope-risk: narrow Reversibility: easy Tested: 51 focused contracts; 162 applicable browser tests; coding-agent check and build
Profile safety canonicalized the executable, requested data root, and every protected channel root with synchronous filesystem calls. Move that work to abort-aware promises so slow NFS or autofs metadata cannot block Bun's main/TUI thread. Lore-id: 6fd02ae6 Constraint: symlink alias rejection remains authoritative without synchronous filesystem I/O Confidence: high Scope-risk: medium Reversibility: easy Tested: 51 focused contracts; 162 applicable browser tests; coding-agent check and build Not-tested: intentionally stalled NFS/autofs mount
A browser abort during the asynchronous local CDP probe was converted into a normal probe miss. Re-check the caller signal in the probe catch and after awaited probes so cancellation cannot degrade into profile refusal or continue acquisition. Lore-id: c01c2c77 Constraint: caller abort always outranks CDP probe fallbacks Confidence: high Scope-risk: narrow Reversibility: easy Tested: 52 focused contracts; 163 applicable browser tests; coding-agent check and build
Cancellation can arrive after the local CDP fetch resolves while its response body is being closed. Re-check the caller signal before accepting a successful probe so detached acquisition cannot connect and cache an authenticated handle after cancellation. Lore-id: e631fa71 Constraint: a successful CDP response never outranks a concurrent caller abort Confidence: high Scope-risk: narrow Reversibility: easy Tested: 53 focused contracts; 164 applicable browser tests; coding-agent check and build
Promises alone keep a browser call stuck when realpath waits on NFS or autofs. Race executable and protected-root canonicalization against the caller signal so cancellation returns immediately while preserving asynchronous symlink alias validation. Lore-id: 3a5163b0 Constraint: stalled filesystem metadata never blocks browser cancellation Confidence: high Scope-risk: narrow Reversibility: easy Tested: 54 focused contracts; 165 applicable browser tests; coding-agent check and build Not-tested: live stalled NFS/autofs mount
The safety test hardcoded the Linux default root (~/.config/google-chrome) via os.homedir(), so it failed on macOS where the guard correctly does not list that root. Drive the refusal through the exported isDefaultChromeUserDataDirForTest seam with explicit darwin/win32/linux platforms and homes so all three branches assert deterministically on any host; keep one end-to-end refusal through the live resolution path using the host platform's actual default root. Lore-id: pr4539-review-repair Constraint: darwin chromeUserDataRoots branch never lists ~/.config/google-chrome Tested: focused browser-chrome-profile suite (31 pass) on Linux host Confidence: high Scope-risk: narrow Reversibility: trivial gaebal-gajae
7643674 to
3230e54
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact head advanced to — gaebal-gajae |
|
@probepark @HaD0Yun — requesting re-review at exact head State since the CHANGES_REQUESTED at
— gaebal-gajae |
OWNER_CONFIRMATION_REQUIRED — exact-head hold for PR #4539Head: Latest authoritative CI inventory (Dev CI run
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 3230e5411 - the platform binding is fixed the right way
test(browser): prove default-root refusal on every platform. My block was that the refusal test
hardcoded a Linux path via os.homedir() and therefore failed on every macOS machine while passing
on Linux CI.
$ bun test <the PR touched suites>
50 pass 0 fail 165 expect() calls
$ bun --cwd=packages/coding-agent run check -> exit 0
You took the option I said was preferable - inject the platform into the exported seam rather than
depend on the host:
expect(await isDefaultChromeUserDataDirForTest(entry.defaultRoot, [entry.defaultRoot], entry.platform)).toBe(true)
// entries: darwin / win32 / linux
...
expect(await isDefaultChromeUserDataDirForTest(customRoot, roots, entry.platform)).toBe(false)All three platforms' default roots are now proven refused, and a non-default root proven accepted,
from any host. The Windows case is covered from a Mac, which ambient os.homedir() could never do.
The host-platform case is handled honestly too:
// host's actual platform default root instead of a Linux-only spelling
const hostRoot = chromeUserDataRoots({ platform: process.platform, home: os.homedir(), exists: () => false })[0]!;That keeps a real assertion against the machine actually running the suite without reintroducing the
platform dependency.
merge-approved. Nothing outstanding from my earlier review.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head, read of the new platform-parameterised assertions to confirm all three branches are exercised deterministically.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head contract reconciliation
- Head:
3230e5411c9caa362ec7290c611cd17154942e4a - Immutable base:
dev@96e718a2b0f2a46cad4876bf922886966c6fd0e3 - Canonical full-index binary diff:
sha256:6efb3787e372c5167df9f917a83a4445fc1b4b2c906ad8f591bcc4e45a758fe3(recomputed before PR-body mutation) - Independent authenticated approval: @probepark review
4942905881, stateAPPROVED, exact commit3230e5411c9caa362ec7290c611cd17154942e4a, repository permissionwrite - PR body now contains exactly one
merge-approvedverdict line backed by that review.
This exact-head COMMENT review intentionally triggers a fresh base-owned PR-contract evaluation while the edited-body Dev CI run reconciles all required product checks.
— gaebal-gajae
MERGE_READYExact-head terminal gate is satisfied for
MERGE_READY — gaebal-gajae |
Terminal merge receiptPR #4539 is merged and closed.
— gaebal-gajae |
2 participants
Problem
Chrome profile mode required callers to spell the browser executable and profile name even though GJC already has platform-aware Chrome/Chromium discovery. The dirty prior PR head also tried to default the live Chrome user-data root, but Chrome 136+ ignores remote-debugging switches for default data directories and Edge must never be paired with Chrome profile data.
Change
app.pathis optional inapp.browser: "chrome"mode and defaults to installed Chrome/Chromium across supported Stable/Beta/Dev/Canary channels.app.profile_directoryis optional and defaults to"Default".app.user_data_dirremains required and must be a non-default Chrome data root. Missing values, every supported channel's platform default root, trusted Linux environment, Flatpak, and Snap defaults, symlink aliases, and Windows case aliases fail before launch with Chrome 136 remediation.msedge, macOS app names, Linuxmicrosoft-edge*, Flatpak launcher names, and resolvable executable aliases.Recovery provenance
Reconstructed the intended PR change onto
devbase96e718a2b0f2a46cad4876bf922886966c6fd0e3(current dev; previously rebased onto64c15281691280be7854dac04baeb05188328ef4, then re-replayed when dev advanced via #4540), then added review-hardening commits. Prior remote headd7b2fa69859d977e49dbc53a70144d911b702b6dand evidence-only candidatebb5d96ebf5de1350a74ed624006658a011127e9ewere not merged wholesale. PRs #4563 and #4564 remain independent and are not duplicated.Verification
bun --cwd=packages/coding-agent run check— pass.bun --cwd=packages/coding-agent run build— pass.git diff --check origin/dev..HEAD— pass.The offline stealth benchmark remains independently red on exact base and this branch for the same WebGL/improvement-gate condition; this PR does not alter that subsystem.
gajae.pr-review-verdict.v1 merge-approved sha256:6efb3787e372c5167df9f917a83a4445fc1b4b2c906ad8f591bcc4e45a758fe3 reviewer:human reviewer-id:probepark evidence:#4539 (review)