fix: bake product identity buildflags for browseros/browserclaw release#1526
Conversation
Return browseros_allow_runtime_product_override to false and make browseros_package_all_server_resources follow it, resolving the pre-claw-release TODO from c840d95. Managed builds are unaffected: bos_build configure writes all three product args explicitly, and the debug preset keeps the runtime override for cross-product testing.
get_product_gn_args hardcoded browseros_package_all_server_resources to false, so a debug build with the runtime product override enabled was missing the other product's server resources and the override was half-broken. Debug now sets override and package-all together; release still bakes identity with both off.
…taged Review follow-ups: early configure-time warning instead of a late ninja missing-input error when a product's server resources aren't staged for a debug (package-all) build; derive the debug configure test's expected args from get_product_gn_args instead of a third literal copy; one-line docstring.
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 1m 51s —— View job Code Review
This PR cleanly resolves the pre-claw-release TODO by restoring safe
|
Greptile SummaryThis PR resolves a pre-claw-release TODO by flipping the
Confidence Score: 5/5Safe to merge — changes are narrow, well-tested, and resolve a known pre-release TODO without introducing new runtime risk. The three-file logic change (GNI patch, GN args generation, configure warning) is tightly scoped and fully covered by new tests. Debug and release golden args are verified explicitly. No pre-existing release-path logic is altered in ways that could affect shipped artifacts. No files require special attention. The configure_test.py warning test relies on all_server_bundles() returning at least one bundle from the real product registry, which is reasonable but worth keeping in mind if the registry changes shape. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bos_build configure step] --> B{build_type?}
B -- debug --> C["get_product_gn_args()\noverride = true\npackage_all = true"]
B -- release --> D["get_product_gn_args()\noverride = false\npackage_all = false"]
C --> E[Write args.gn]
D --> E
E --> F{debug build?}
F -- yes --> G[_warn_missing_server_resources]
G --> H{bundle resources exist?}
H -- missing --> I[log_warning: actionable message]
H -- present --> J[no warning]
I --> K[gn gen]
J --> K
F -- no --> K
K --> L[Build with ninja]
%%{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[bos_build configure step] --> B{build_type?}
B -- debug --> C["get_product_gn_args()\noverride = true\npackage_all = true"]
B -- release --> D["get_product_gn_args()\noverride = false\npackage_all = false"]
C --> E[Write args.gn]
D --> E
E --> F{debug build?}
F -- yes --> G[_warn_missing_server_resources]
G --> H{bundle resources exist?}
H -- missing --> I[log_warning: actionable message]
H -- present --> J[no warning]
I --> K[gn gen]
J --> K
F -- no --> K
K --> L[Build with ninja]
Reviews (1): Last reviewed commit: "fix(browseros): warn at configure when d..." | Re-trigger Greptile |
Greptile SummaryThis PR resolves a pre-claw-release TODO by restoring
Confidence Score: 4/5The logic is sound and the release path is unchanged; the only gaps are in test robustness, not in production code. The production code changes are clean: the .gni default flip is byte-identical to the verified pre-flip blob, get_product_gn_args() correctly gates both flags on build type, and the warning helper reads a directory check that cannot affect the build outcome. The two test gaps could mask future regressions but do not affect current correctness. configure_test.py — the new debug arg test constructs its expected value from the same function under test, and the warning test relies on the server bundle registry being non-empty without asserting it. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[configure.execute] --> B{build_type == debug?}
B -- Yes --> C["get_product_gn_args()\nallow_runtime_override = true\npackage_all_server_resources = true"]
B -- No --> D["get_product_gn_args()\nallow_runtime_override = false\npackage_all_server_resources = false\nbrowseros_product baked in"]
C --> E[Write args.gn]
D --> E
E --> F{build_type == debug?}
F -- Yes --> G[_warn_missing_server_resources]
G --> H{for each bundle\nresources_dir.exists?}
H -- No --> I[log_warning: run resources step first]
H -- Yes --> J[OK]
I --> K[gn gen out_dir]
J --> K
F -- No --> L[gn gen out_dir --fail-on-unused-args]
%%{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[configure.execute] --> B{build_type == debug?}
B -- Yes --> C["get_product_gn_args()\nallow_runtime_override = true\npackage_all_server_resources = true"]
B -- No --> D["get_product_gn_args()\nallow_runtime_override = false\npackage_all_server_resources = false\nbrowseros_product baked in"]
C --> E[Write args.gn]
D --> E
E --> F{build_type == debug?}
F -- Yes --> G[_warn_missing_server_resources]
G --> H{for each bundle\nresources_dir.exists?}
H -- No --> I[log_warning: run resources step first]
H -- Yes --> J[OK]
I --> K[gn gen out_dir]
J --> K
F -- No --> L[gn gen out_dir --fail-on-unused-args]
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/steps/setup/configure_test.py:98-109
**Circular expected-value construction in debug args test**
`ctx.get_product_gn_args()` is called to build the expected string, so the test validates that "the file contains what `get_product_gn_args()` returns" rather than a concrete known value. If `get_product_gn_args()` had a bug — e.g., emitting `"false"` for debug builds — both sides of the `assertEqual` would reflect that bug and the test would still pass. The release counterpart (`test_writes_args_gn_with_target_cpu`) hard-codes its expected lines; this debug test should do the same.
### Issue 2 of 2
packages/browseros/bos_build/steps/setup/configure_test.py:157-162
**Warning test has an implicit dependency on a non-empty server bundle registry**
`test_debug_warns_when_server_resources_not_staged` does not mock `all_server_bundles()`, so the assertion (`assertTrue(any(...))`) silently passes vacuously if the registry ever returns an empty tuple — no warning would be fired, but the test would also never fail. A guard like `self.assertTrue(len(all_server_bundles()) > 0, "registry is empty")` at the top of the test, or mocking `all_server_bundles` with a known stub, would make the dependency explicit.
Reviews (2): Last reviewed commit: "fix(browseros): warn at configure when d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_debug_args_append_product_args_verbatim(self): | ||
| ctx, chromium, _ = self._execute( | ||
| "debug", architecture="arm64", flags="is_debug = true\n" | ||
| ) | ||
|
|
||
| args_gn = (chromium.src / ctx.out_dir / "args.gn").read_text() | ||
| self.assertEqual( | ||
| args_gn, | ||
| 'is_debug = true\n\ntarget_cpu = "arm64"\n' | ||
| + "\n".join(ctx.get_product_gn_args()) | ||
| + "\n", | ||
| ) |
There was a problem hiding this comment.
ctx.get_product_gn_args() is called to build the expected string, so the test validates that "the file contains what get_product_gn_args() returns" rather than a concrete known value. If get_product_gn_args() had a bug — e.g., emitting "false" for debug builds — both sides of the assertEqual would reflect that bug and the test would still pass. The release counterpart (test_writes_args_gn_with_target_cpu) hard-codes its expected lines; this debug test should do the same.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/steps/setup/configure_test.py
Line: 98-109
Comment:
**Circular expected-value construction in debug args test**
`ctx.get_product_gn_args()` is called to build the expected string, so the test validates that "the file contains what `get_product_gn_args()` returns" rather than a concrete known value. If `get_product_gn_args()` had a bug — e.g., emitting `"false"` for debug builds — both sides of the `assertEqual` would reflect that bug and the test would still pass. The release counterpart (`test_writes_args_gn_with_target_cpu`) hard-codes its expected lines; this debug test should do the same.
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!
| def test_debug_warns_when_server_resources_not_staged(self): | ||
| with mock.patch.object(configure, "log_warning") as log_warning: | ||
| self._execute("debug") | ||
|
|
||
| messages = [call.args[0] for call in log_warning.call_args_list] | ||
| self.assertTrue(any("server resources" in m for m in messages), messages) |
There was a problem hiding this comment.
test_debug_warns_when_server_resources_not_staged does not mock all_server_bundles(), so the assertion (assertTrue(any(...))) silently passes vacuously if the registry ever returns an empty tuple — no warning would be fired, but the test would also never fail. A guard like self.assertTrue(len(all_server_bundles()) > 0, "registry is empty") at the top of the test, or mocking all_server_bundles with a known stub, would make the dependency explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/steps/setup/configure_test.py
Line: 157-162
Comment:
**Warning test has an implicit dependency on a non-empty server bundle registry**
`test_debug_warns_when_server_resources_not_staged` does not mock `all_server_bundles()`, so the assertion (`assertTrue(any(...))`) silently passes vacuously if the registry ever returns an empty tuple — no warning would be fired, but the test would also never fail. A guard like `self.assertTrue(len(all_server_bundles()) > 0, "registry is empty")` at the top of the test, or mocking `all_server_bundles` with a known stub, would make the dependency explicit.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
buildflags.gnidefaults:browseros_allow_runtime_product_override = false,browseros_package_all_server_resourcesfollows the override — resolving the pre-claw-release TODO from c840d95. Content is byte-identical to the pre-flip version (blob0a4b34ea72336), verified to apply cleanly.get_product_gn_args()now couplesbrowseros_package_all_server_resourcesto the runtime override: debug builds gettrue/true(one debug binary can actually run as either product — previously the override was on but the other product's server resources were missing), release builds bakebrowseros_productwith both flagsfalse. Release args.gn output is unchanged from current main.Design
Release safety lives in two layers on purpose: bos_build states managed-build policy explicitly in args.gn (auditable, belt-and-suspenders for shipped artifacts), while the .gni default protects anything built outside bos_build. The Jun 30 testing need (run either product from one build) is preserved through the managed debug path instead of unsafe global defaults.
Notes for dev workflows
browseros buildconfigure) must addbrowseros_allow_runtime_product_override = trueto keep the--browseros-productswitch; the flipped default compiles it out otherwise. This is the intended pre-claw-release behavior.override && !package_all(legitimate for browser-only cross-product testing without server resources) and declined dropping the explicit package_all arg from args.gn (see Design).Test plan
uv run python -m unittest discover -s bos_build -t . -p "*_test.py"— 596 tests OK (new: release golden args for both products, debug wiring test, configure warning tests)uv run ruff check bos_build— cleangit hash-objectof the result matches the embedded index hashBrowserOSServerandBrowserClawServerresources land in the output and--browseros-product=browserclawworks end to end