fix(bos_build): isolate BrowserClaw user-data dir from BrowserOS#1531
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 39s —— View job Code Review
Overall: This is a clean, well-reasoned fix. The structural-anchor approach (substituting on key/variable names rather than current values) makes stamping genuinely idempotent, and the exactly-one-match guard turns patch drift into an immediate build failure rather than a silent profile collision. Test coverage is thorough. A few observations below.
|
| Category | Finding |
|---|---|
| Minor | _PATHS_LINUX_ANCHOR: use [ \t]* instead of \s* before \n for clarity |
| Minor | Missing anchor-miss tests for PATHS_LINUX and INSTALL_MODES in the test file |
| Nit | _kebab_case behavior on consecutive-uppercase not documented |
| ✓ | Idempotency via structural anchors — solid design |
| ✓ | Exactly-one-match guard — build fails on drift, not silently |
| ✓ | mtime preservation for BrowserOS no-op path |
| ✓ | UserDataIsolationTest regression guard |
| ✓ | Golden tests for both descriptors |
The core fix and design are sound. The two minor findings above are quality improvements rather than blockers.
|
Note on the red |
Greptile SummaryThis PR fixes a SingletonLock collision between BrowserOS and BrowserClaw caused by both products sharing the same user-data directory names across all three platforms. It introduces a
Confidence Score: 4/5Safe to merge; the fix is well-tested and the idempotent, fail-loud design prevents silent regressions from patch drift. The implementation is solid and the test suite is thorough (stamp, no-op, idempotence, missing-file, missing-anchor, duplicate-anchor cases all covered). The two observations are both forward-looking: No files require special attention for this merge. The Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[patches step\napply BrowserOS-default values] --> B[product_identity step]
B --> C{product == browseros?}
C -- yes --> D[byte-level no-op\nall three files unchanged]
C -- no, e.g. browserclaw --> E[rewrite mac: CrProductDirName\napp-Info.plist]
C -- no --> F[rewrite linux: data_dir_basename\nchrome_paths_linux.cc]
C -- no --> G[rewrite windows: kProductPathName\nchromium_install_modes.h]
E --> H[exactly 1 anchor match\nor build dies]
F --> H
G --> H
H --> I[configure step]
D --> I
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[patches step\napply BrowserOS-default values] --> B[product_identity step]
B --> C{product == browseros?}
C -- yes --> D[byte-level no-op\nall three files unchanged]
C -- no, e.g. browserclaw --> E[rewrite mac: CrProductDirName\napp-Info.plist]
C -- no --> F[rewrite linux: data_dir_basename\nchrome_paths_linux.cc]
C -- no --> G[rewrite windows: kProductPathName\nchromium_install_modes.h]
E --> H[exactly 1 anchor match\nor build dies]
F --> H
G --> H
H --> I[configure step]
D --> I
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros/bos_build/core/products.py:221-223
`_kebab_case` doesn't insert dashes at upper→upper→lower boundaries, so product names with acronym-prefixed words (e.g., "BrowserOSPro") produce "browser-ospro" instead of "browser-os-pro". "BrowserOS" and "BrowserClaw" work correctly today, but a new product named anything like "MyHTTPSProxy" or "BrowserOSPro" would silently get a surprising kebab value that then gets stamped into the user-data path.
### Issue 2 of 2
packages/browseros/bos_build/steps/patches/product_identity.py:31-33
The Linux anchor matches `#else` followed by the `data_dir_basename` assignment, which is structurally less unique than the other two anchors (which use distinct key/variable names like `CrProductDirName` and `kProductPathName`). If the upstream chromium `chrome_paths_linux.cc` ever gains a second `#else` block that assigns `data_dir_basename` for a new branding tier, this would fire the "found 2 anchors" error and block the build. The "exactly one match" invariant will catch this loudly rather than silently, which is the right failure mode — just worth keeping in mind when upmerging chromium versions.
Reviews (1): Last reviewed commit: "chore: merge origin/main into fix/browse..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _kebab_case(name: str) -> str: | ||
| """CamelCase → kebab-case ("BrowserOS" → "browser-os").""" | ||
| return re.sub(r"(?<=[a-z0-9])(?=[A-Z])", "-", name).lower() |
There was a problem hiding this comment.
_kebab_case doesn't insert dashes at upper→upper→lower boundaries, so product names with acronym-prefixed words (e.g., "BrowserOSPro") produce "browser-ospro" instead of "browser-os-pro". "BrowserOS" and "BrowserClaw" work correctly today, but a new product named anything like "MyHTTPSProxy" or "BrowserOSPro" would silently get a surprising kebab value that then gets stamped into the user-data path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/core/products.py
Line: 221-223
Comment:
`_kebab_case` doesn't insert dashes at upper→upper→lower boundaries, so product names with acronym-prefixed words (e.g., "BrowserOSPro") produce "browser-ospro" instead of "browser-os-pro". "BrowserOS" and "BrowserClaw" work correctly today, but a new product named anything like "MyHTTPSProxy" or "BrowserOSPro" would silently get a surprising kebab value that then gets stamped into the user-data path.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| _PATHS_LINUX_ANCHOR = re.compile( | ||
| r'(#else\s*\n\s*std::string data_dir_basename = ")[^"]*(";)' | ||
| ) |
There was a problem hiding this comment.
#else followed by the data_dir_basename assignment, which is structurally less unique than the other two anchors (which use distinct key/variable names like CrProductDirName and kProductPathName). If the upstream chromium chrome_paths_linux.cc ever gains a second #else block that assigns data_dir_basename for a new branding tier, this would fire the "found 2 anchors" error and block the build. The "exactly one match" invariant will catch this loudly rather than silently, which is the right failure mode — just worth keeping in mind when upmerging chromium versions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/steps/patches/product_identity.py
Line: 31-33
Comment:
The Linux anchor matches `#else` followed by the `data_dir_basename` assignment, which is structurally less unique than the other two anchors (which use distinct key/variable names like `CrProductDirName` and `kProductPathName`). If the upstream chromium `chrome_paths_linux.cc` ever gains a second `#else` block that assigns `data_dir_basename` for a new branding tier, this would fire the "found 2 anchors" error and block the build. The "exactly one match" invariant will catch this loudly rather than silently, which is the right failure mode — just worth keeping in mind when upmerging chromium versions.
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR fixes the
Confidence Score: 4/5Safe to merge; the stamping logic is correct, idempotent, and fails loudly on patch drift. The implementation is well-designed and thoroughly tested — stamp, no-op, idempotency, missing file, and duplicate-anchor cases are all covered. The one item worth a second look is the _PATHS_LINUX_ANCHOR regex, where \s* before the explicit packages/browseros/bos_build/steps/patches/product_identity.py — specifically the _PATHS_LINUX_ANCHOR regex. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[git_setup / clean / provision] --> B[download_resources]
B --> C[bundled_extensions]
C --> D[chromium_replace]
D --> E[string_replaces]
E --> F[series_patches]
F --> G[patches\napply BrowserOS-default values]
G --> H[product_identity NEW\nstamp per-product user-data identity]
H --> I[configure]
I --> J[compile]
subgraph stamp [product_identity stamps 3 files]
P1[chrome/app/app-Info.plist\nCrProductDirName = BrowserOS to BrowserClaw]
P2[chrome/common/chrome_paths_linux.cc\ndata_dir_basename = browser-os to browser-claw]
P3[chrome/install_static/chromium_install_modes.h\nkProductPathName = BrowserOS to BrowserClaw]
end
H -.-> P1
H -.-> P2
H -.-> P3
style H fill:#d4edda,stroke:#28a745
style stamp fill:#f8f9fa,stroke:#6c757d
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[git_setup / clean / provision] --> B[download_resources]
B --> C[bundled_extensions]
C --> D[chromium_replace]
D --> E[string_replaces]
E --> F[series_patches]
F --> G[patches\napply BrowserOS-default values]
G --> H[product_identity NEW\nstamp per-product user-data identity]
H --> I[configure]
I --> J[compile]
subgraph stamp [product_identity stamps 3 files]
P1[chrome/app/app-Info.plist\nCrProductDirName = BrowserOS to BrowserClaw]
P2[chrome/common/chrome_paths_linux.cc\ndata_dir_basename = browser-os to browser-claw]
P3[chrome/install_static/chromium_install_modes.h\nkProductPathName = BrowserOS to BrowserClaw]
end
H -.-> P1
H -.-> P2
H -.-> P3
style H fill:#d4edda,stroke:#28a745
style stamp fill:#f8f9fa,stroke:#6c757d
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browseros/bos_build/steps/patches/product_identity.py:31-33
The `\s*` before the explicit `
` in this pattern includes `
` itself (Python's `\s` matches `[
\f\v]`), so the greedy quantifier will first consume the newline before backtracking to let the explicit `
` match. This works correctly in practice, but using `[^\S
]*` ("any whitespace that is not a newline") makes the intent explicit and avoids the unnecessary backtracking step.
Reviews (2): Last reviewed commit: "chore: merge origin/main into fix/browse..." | Re-trigger Greptile |
* feat(patches): per-product user-data dir via browseros_product buildflag BrowserOS and BrowserClaw shared one profile root (CrProductDirName, linux data_dir_basename, win kProductPathName all said BrowserOS), so the second product to launch hit the first one's SingletonLock instead of starting its own instance. The identity now derives from the browseros_product GN arg inside the patch stack: macOS substitutes BROWSEROS_PRODUCT_DIR_NAME into app-Info.plist, Linux and Windows branch on BUILDFLAG(BROWSEROS_PRODUCT_BROWSERCLAW). * revert(bos_build): drop product_identity source stamping (#1531) The stamping step regex-rewrote patched chromium files after apply — app-Info.plist is itself a patch file, so the stamp fought the patch stack and re-extraction would silently undo it. The user-data identity now lives in the patch stack keyed off the browseros_product GN arg (which bos_build already passes per product), so the step, the model fields, and their tests go away. This reverts commit 730e36b.
## Summary - removed stale product identity residue left after #1536 superseded #1531's Python stamping step - deleted the unreferenced Info.plist additions file that hardcoded BrowserOS as the product directory - added patch-stack regression coverage for macOS, Linux, and Windows product user-data roots - stabilized colored CLI help assertions in bos_build tests ## Tests - uv run python -m unittest discover -s bos_build -t . -p '*_test.py' - uv run ruff check bos_build
1 participant
Summary
CrProductDirName = BrowserOS, sharing~/Library/Application Support/BrowserOSand its SingletonLock — launching one blocked the other. Same hardcoding existed for Linux (browser-os) and Windows (kProductPathName = L"BrowserOS").ProductDescriptornow models the user-data directory identity per platform: macproduct_dir_name(BrowserOS / BrowserClaw), linuxuser_data_dir_name(browser-os / browser-claw), windowsproduct_path_name(BrowserOS / BrowserClaw).product_identityprep step runs right afterpatchesand stamps those values into the three patched Chromium files; wired into release, debug, and universal presets plus phase-flag mode.Design
The chromium patches stay product-neutral (they carry the default BrowserOS values); per-product deviation happens in bos_build, same as the
chromium_files/products/<id>/overlays. The step rewrites via structural anchors (theCrProductDirNamekey, the#else-guardeddata_dir_basenameassignment, thekProductPathNamevariable) — never the current value — so it is idempotent, and each anchor must match exactly once or the build dies (a stale patch can never silently ship a shared profile root). For--product browserosthe stamp is a byte-level no-op, so default trees stay identical to the patch stack and BrowserOS artifacts are unchanged. Buildflag conditionals inside the patches were rejected becausechrome/common/chrome/install_staticcan't depend onchrome/browser/browseros:buildflags(GN layering) and the plist is static text.Test plan
uv run python -m unittest discover -s bos_build -t . -p "*_test.py"— 601 tests green (goldens for both descriptors, stamp/no-op/idempotence/anchor-failure step tests, planner + phase ordering)uv run ruff check bos_build— cleanuv run browseros build --preset release --product browserclaw --show-planlistsproduct_identityright afterpatchesBrowserClaw.app/Contents/Info.plisthasCrProductDirName = BrowserClawand both browsers run side by sideFollow-ups (out of scope)
chromium_install_modes.h) is still shared — needed for side-by-side Windows installs, not for running two profiles.resources/entitlements/Info.plist.additionsis dead (referenced nowhere) and still says BrowserOS — candidate for deletion.🤖 Generated with Claude Code