fix(browser): detect Windows browsers when install roots are blank#111256
Conversation
d7d2b3c to
108a59a
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 3:23 AM ET / 07:23 UTC. Summary PR surface: Source +2, Tests +23. Total +25 across 2 files. Reproducibility: yes. from source: empty Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Merge the focused boundary normalization after the ordinary current-head CI and maintainer checks complete, keeping blank roots equivalent to absent roots and retaining nonblank custom-root precedence. Do we have a high-confidence way to reproduce the issue? Yes, from source: empty Is this the best way to solve the issue? Yes. Normalizing optional install-root inputs at the browser-plugin boundary is the narrowest fix: it restores existing defaults for blank values without changing meaningful nonblank overrides. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7c4292ee966d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +2, Tests +23. Total +25 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
108a59a to
a64c13a
Compare
clawsweeper
Bot
added
proof: sufficient
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
The set-but-blank distinction this is built on is the right one — ?? only catches undefined, and a blank ProgramFiles sails through it into a path join, which is precisely the failure mode that's annoying to diagnose from a bug report.
Two things I checked that hold up: localAppData is guarded by if (localAppData) at :677 and :747 rather than being joined blank, so the ?? "" there is deliberate and safe; and the new test is load-bearing — it stubs " ", which does not trigger ?? pre-fix, so the test genuinely fails without the change rather than passing either way.
One more site in the same file has the identical hole, inline.
| function findChromeExecutableWindows(): BrowserExecutable | null { | ||
| const localAppData = process.env.LOCALAPPDATA ?? ""; | ||
| const programFiles = process.env.ProgramFiles ?? "C:\\Program Files"; | ||
| const localAppData = normalizeOptionalString(process.env.LOCALAPPDATA) ?? ""; |
There was a problem hiding this comment.
There's a third instance of this exact flaw in the same file, ~190 lines above — and it already imports the helper you're adopting here:
// chrome.executables.ts:474-478 (not in this diff)
function expandWindowsEnvVars(value: string): string {
return value.replace(/%([^%]+)%/g, (_match, name) => {
const key = normalizeOptionalString(name) ?? "";
return key ? (process.env[key] ?? `%${key}%`) : _match;
});
}Line 476 calls normalizeOptionalString on the variable name; line 477 then reads process.env[key] with a bare ??, which is the same set-but-blank hole this PR closes at :669 and :740.
That matters under this PR's own scenario, because it's a different code path to the same goal. expandWindowsEnvVars feeds detectDefaultChromiumExecutableWindows (:316), which expands the registry shell\open\command value. With ProgramFiles blank, a registry value of
%ProgramFiles%\Google\Chrome\Application\chrome.exe
expands to \Google\Chrome\Application\chrome.exe — an absolute path on the current drive that won't exist. So after this PR the fallback scan path recovers on a blank-env machine while default-browser detection still fails, which is an awkward split to debug later.
The fix at :477 is one line, in the same shape as what you did here:
return key ? (normalizeOptionalString(process.env[key]) ?? `%${key}%`) : _match;(Deliberately not a suggestion block — the target line is outside this diff, so it would apply to the wrong place.)
a64c13a to
0c0d342
Compare
| return { | ||
| localAppData: | ||
| normalizeOptionalString(process.env.LOCALAPPDATA) ?? | ||
| path.win32.join(os.homedir(), "AppData", "Local"), |
There was a problem hiding this comment.
resolveWindowsBrowserInstallRoots() adds an unguarded os.homedir() to the Windows detection path. On Windows os.homedir() is itself derived from USERPROFILE, and it does not treat a blank value the way this PR treats blank install roots — it throws.
Isolating USERPROFILE only (real Windows, Node v22.20.0, child process per row):
USERPROFILE |
os.homedir() |
|---|---|
| normal (control) | C:\Users\Asus |
| absent (deleted) | C:\Users\Asus — libuv falls back to the profile API |
blank "" |
throws ERR_SYSTEM_ERROR: uv_os_homedir returned ENOENT |
whitespace " " |
returns " " → root becomes \AppData\Local, not absolute |
For this one input, blank is not like absent — the inverse of the invariant this helper establishes two lines above it.
End-to-end through the real resolver (resolveBrowserExecutableForPlatform({} , "win32"), no mocks, same machine, same command, only the tree differs). Head 0c0d3424484, base ab2ad22533b (this PR's merge-base):
| env | base | PR head |
|---|---|---|
| normal env (CONTROL) | {"kind":"chromium","path":"…\Opera GX\opera.exe"} |
same |
LOCALAPPDATA + ProgramFiles + ProgramFiles(x86) + USERPROFILE all blank |
{"kind":"chromium","path":"…\Opera GX\opera.exe"} |
ERR_SYSTEM_ERROR: A system error occurred: uv_os_homedir returned ENOENT |
The control row is only there to show the harness resolves on both trees; just the blank-env row diverges.
It fires early and unconditionally: expandWindowsEnvVars calls the helper before it inspects the string, so the registry-derived default-browser step throws before a %VAR% even has to be present.
Neither caller catches it:
chrome.ts:1025expectsnulland converts it intoNo supported browser found (…). That becomes an uncaughtERR_SYSTEM_ERRORinstead.chrome.ts:707calls the resolver inside acatch (err)block, so the new throw replaceserrand masks the original port-in-use failure it was recovering from.
The added tests can't see this: they vi.mock("node:os") and stub os.homedir, so the real uv_os_homedir path is never exercised.
The repo already has a house rule for this exact input — packages/terminal-core/src/display-string.ts:13:
/** Run a home resolver defensively because some runtimes throw for missing passwd data. */
function normalizeSafe(fn: () => string | undefined): string | undefined {used as normalize(env.USERPROFILE) ?? … ?? normalizeSafe(homedir), and its test stubs USERPROFILE to "" — blank USERPROFILE is an already-exercised case in this codebase. src/daemon/schtasks.ts:77 has the same shape: env.USERPROFILE?.trim(), then a domain error, never a system error.
A shape that keeps this PR's thesis and the pre-PR fail-soft behaviour (both call sites still guard with if (localAppData), which is exactly what empty LOCALAPPDATA used to hit):
function resolveWindowsUserAppDataRoot(): string | null {
const configured = normalizeOptionalString(process.env.LOCALAPPDATA);
if (configured) {
return configured;
}
let home: string | null = null;
try {
home = normalizeOptionalString(os.homedir()) ?? null;
} catch {
home = null;
}
return home ? path.win32.join(home, "AppData", "Local") : null;
}localAppData then widens to string | null and installRootByEnvName to Record<string, string | null>; the existing ?? \%${key}%`` fallback already covers a null entry.
The whitespace row above is the same hole in its quieter form: os.homedir() is the only root joined without normalizeOptionalString, so " " yields a drive-relative candidate root instead of falling back to a usable default.
|
Maintainer repair is ready on exact head What changed:
Proof on this head:
Known proof gap: a fresh native Windows run was attempted, but the broker returned HTTP 401. I did not bypass that boundary or represent the older-head Windows run as exact-head proof. The focused resolver tests cover blank defaults, explicit custom-root precedence, Chrome-only discovery, and registry expansion. ClawSweeper: the latest completed review has no rank-up moves. The fresh exact-head worker left only a started placeholder and its lease expired without a verdict; there are therefore no new moves to apply. I checked that state before merge rather than treating the placeholder as proof. |
|
Merged via squash.
|
…penclaw#111256) * fix(browser): ignore blank Windows install roots * fix(browser): complete Windows install root fallbacks Co-authored-by: LZY3538 <[email protected]> --------- Co-authored-by: Peter Steinberger <[email protected]>
3 participants
What Problem This Solves
Fixes an issue where Windows browser auto-detection could miss standard Chrome, Edge, and Brave installations when
LOCALAPPDATA,ProgramFiles, orProgramFiles(x86)were present but blank.Why This Change Was Made
Windows install-root inputs are normalized once at the browser plugin boundary before candidate paths are built. Blank values now behave like absent values, while nonblank custom roots retain their existing precedence. Registry environment expansion consumes the same resolved roots so default-browser detection and fallback scanning cannot diverge.
User Impact
Windows users can keep using browser auto-detection even when a launcher or service supplies blank install-root environment variables.
Evidence
Exact repaired head:
0c0d34244847bfc59278a6feb1ee1f26cb691a95node scripts/run-vitest.mjs extensions/browser/src/browser/chrome.default-browser.test.ts— 11/11 passed.git diff --check origin/main...HEAD— passed.29957874365— passed: https://github.com/openclaw/openclaw/actions/runs/29957874365OPENCLAW_TESTBOX=1 scripts/pr prepare-run 111256— passed inhosted_exact_or_recent_parentmode; no standalone remote lease was dispatched.Fresh native Windows proof was attempted, but the broker returned HTTP 401. That boundary was not bypassed. Earlier PR history contains a real Windows run on the pre-repair head; it is not represented here as exact-head proof.