feat(bos_build): --show-plan, --skip, --from and modules profiles#1502
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 4m 4s —— View job Code ReviewTasks
SummaryStrong PR. The architecture is clean — Issues1.
2. f"Step '{start}' is not in the composed plan "
f"({' → '.join(steps)}); nothing to resume from"For a full release plan (~15 steps), this is fine. For a larger future plan or a universal plan, this string could become unwieldy in error output. Consider capping or omitting the inline plan dump here, since 3. self.assertEqual(plan_lines(result.output)[0], "configure")
self.assertEqual(plan_lines(result.output),
plan(Switches(preset="release"), "x64", get_platform())[plan(...).index("configure"):])4. No test for bare profile name resolution (
5. Deferred
6. Multi-arch If Minor Observations (non-blocking)
VerdictThe design decisions are sound and well-justified. Items 1–2 are clean-up candidates; items 3–5 are low-risk test coverage gaps. Nothing blocks merging. |
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge. The new flags are additive and the refactoring of The implementation is well-structured: No files require special attention. The largest change is Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI invocation] --> B{has_preset?}
B -- No --> C[resolve_pipeline\nquiet=show_plan]
C --> D{show_plan?}
D -- Yes --> E[validate_pipeline\n_print_plan\nexit 0]
D -- No --> F[resolve_config\nbuild runs]
B -- Yes --> G[_resolve_preset]
G --> H{profile has\nmodules:?}
H -- Yes --> I[_resolve_modules_profile\nreject planner flags\nvalidate_pipeline]
H -- No --> J[merge overrides\nunion skip\nswitches.resolved]
J --> K[plan per arch\nslice_from if --from]
I --> L[_PlanProjection\nbuild_runs closure]
K --> L
L --> M{show_plan?}
M -- Yes --> N[_print_plan\nenv markers\nexit 0\nno checkout needed]
M -- No --> O[projection.build_runs\n_resolve_chromium_src\nContext construction\nexecute pipeline]
%%{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[CLI invocation] --> B{has_preset?}
B -- No --> C[resolve_pipeline\nquiet=show_plan]
C --> D{show_plan?}
D -- Yes --> E[validate_pipeline\n_print_plan\nexit 0]
D -- No --> F[resolve_config\nbuild runs]
B -- Yes --> G[_resolve_preset]
G --> H{profile has\nmodules:?}
H -- Yes --> I[_resolve_modules_profile\nreject planner flags\nvalidate_pipeline]
H -- No --> J[merge overrides\nunion skip\nswitches.resolved]
J --> K[plan per arch\nslice_from if --from]
I --> L[_PlanProjection\nbuild_runs closure]
K --> L
L --> M{show_plan?}
M -- Yes --> N[_print_plan\nenv markers\nexit 0\nno checkout needed]
M -- No --> O[projection.build_runs\n_resolve_chromium_src\nContext construction\nexecute pipeline]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/browseros/bos_build/core/planner.py:874-879
**Verbose error message for absent `--from` step**
The error message inline-joins all plan steps with `→`, which for a full release plan can produce a 20+ step single-line string that is hard to read in a terminal. If the plan grows, this becomes nearly unreadable. Consider either truncating to the first/last few steps or directing the user to `--show-plan` instead of printing the full plan inline.
### Issue 2 of 3
packages/browseros/bos_build/core/planner_test.py:1117-1121
**Hardcoded plan tail makes the test fragile to plan changes**
`test_slices_composed_plan_from_step` asserts an exact slice `["sign_macos", "package_macos", "upload"]`. If new steps are inserted into the release arm64/macos plan between `sign_macos` and `upload`, this test breaks with a confusing diff rather than a signal about `slice_from` itself. Consider deriving the expected value from `plan()` directly: `full = plan(RELEASE, "arm64", "macos"); expected = full[full.index("sign_macos"):]`.
### Issue 3 of 3
packages/browseros/bos_build/cli/build.py:253-263
**`frozen=True` with mutable `List` fields gives a misleading immutability signal**
`_PlanProjection` is `frozen=True`, which prevents attribute rebinding but does not protect the contents of `header: List[str]` and `arch_plans: List[Tuple[str, List[str]]]`. A reader seeing `frozen=True` may assume the object is deeply immutable. Since the class is private and single-use, changing the field types to `Tuple` (e.g., `Tuple[str, ...]` for `header`) would make the invariant accurate, or alternatively dropping `frozen=True` would remove the false signal.
Reviews (1): Last reviewed commit: "fix(bos_build): tighten plan projection ..." | Re-trigger Greptile |
Re-applies skip/slice/Profile on top of the plan_runs universal refactor (#1501): skip subtraction now covers the universal runs, --from resumes the sequential run timeline (slice_runs_from) so a failed universal merge restarts at merge_universal without recompiling, and the README documents the timeline semantics.
Greptile SummaryThis PR introduces
Confidence Score: 4/5The change is safe to merge; all new flags are additive and normal builds follow the identical path as before — plan() output for skip-free switch combinations is byte-identical to the base, verified across a 324-combination matrix. The redesign is well-structured and heavily tested (333 tests, 46 new). The main risk surface is the deferred build_runs closure inside _resolve_preset, which captures pre-computed arch plans across the show-plan boundary — but the logic is correct and the closure's empty-plan guard fires before the checkout is touched. The three findings are all style/quality items that do not affect correctness. The restructured build.py — specifically _resolve_preset, _resolve_modules_profile, and the _PlanProjection closure pattern — benefits from a close read to confirm the deferred execution contract holds as the file evolves. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[browseros build CLI] --> B{has_preset?}
B -->|yes| C[_resolve_preset]
B -->|no| D[resolve_pipeline]
C --> E{profile has modules:?}
E -->|yes| F[_resolve_modules_profile\nvalidate_pipeline\neff_arch/build_type]
E -->|no| G[load switch-based profile\nmerge CLI overrides\nunion skip sets\nswitch.resolved]
G --> H[plan per arch\nslice_from if --from]
F --> I[_PlanProjection\nheader + arch_plans + build_runs closure]
H --> I
I --> J{show_plan?}
J -->|yes| K[_print_plan: steps + env markers\nexit 0 no checkout needed]
J -->|no| L[projection.build_runs\nresolve chromium_src\nbuild Context per arch]
D --> M{show_plan?}
M -->|yes| N[validate_pipeline\nprint direct plan\nexit 0]
M -->|no| O[resolve_config\nContext per arch]
L --> P[validate_pipeline\npreflight\nrun_pipeline]
O --> P
%%{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[browseros build CLI] --> B{has_preset?}
B -->|yes| C[_resolve_preset]
B -->|no| D[resolve_pipeline]
C --> E{profile has modules:?}
E -->|yes| F[_resolve_modules_profile\nvalidate_pipeline\neff_arch/build_type]
E -->|no| G[load switch-based profile\nmerge CLI overrides\nunion skip sets\nswitch.resolved]
G --> H[plan per arch\nslice_from if --from]
F --> I[_PlanProjection\nheader + arch_plans + build_runs closure]
H --> I
I --> J{show_plan?}
J -->|yes| K[_print_plan: steps + env markers\nexit 0 no checkout needed]
J -->|no| L[projection.build_runs\nresolve chromium_src\nbuild Context per arch]
D --> M{show_plan?}
M -->|yes| N[validate_pipeline\nprint direct plan\nexit 0]
M -->|no| O[resolve_config\nContext per arch]
L --> P[validate_pipeline\npreflight\nrun_pipeline]
O --> P
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/browseros/bos_build/cli/build_test.py:30-33
The `except (ValueError, AttributeError)` in `combined()` catches `ValueError` alongside `AttributeError`. `AttributeError` is the right guard for older Click versions where `.stderr` may not exist, but `ValueError` is unrelated to attribute access and could silently swallow a genuine infrastructure error. Narrowing to `AttributeError` only keeps the intent precise.
```suggestion
try:
out += result.stderr
except AttributeError:
pass
```
### Issue 2 of 3
packages/browseros/bos_build/cli/build.py:25
`EnvConfig` is imported lazily inside both the `--show-plan` direct-mode branch and `_resolve_modules_profile`. Since `build.py` already has a rich set of top-level imports and `EnvConfig` is not heavyweight, a top-level import is cleaner and makes the dependency visible at a glance.
```suggestion
from ..core.env import EnvConfig
from ..core.resolver import resolve_config, resolve_pipeline
```
### Issue 3 of 3
packages/browseros/bos_build/cli/build.py:249-263
**`_display_only_runs` sentinel vs. `Optional` default**
`_display_only_runs` raises `RuntimeError` as a guard against callers that create a `_PlanProjection` without a real `build_runs`. A caller who accidentally omits the argument in a future code path gets a `RuntimeError` with a message pointing to display semantics rather than the actual mistake. An `Optional[Callable]` field defaulting to `None` with an explicit `None`-check at the call site would produce a more actionable error and make the intent clear in the type signature.
Reviews (2): Last reviewed commit: "chore: merge main into feat/preset-plan-..." | Re-trigger Greptile |
1 participant
Summary
--show-planprints the composed step list + required env vars (marked set/missing, never values) for any preset/profile/product/arch — or a--modules/phase-flag pipeline — and exits without needing a chromium checkout. The projection is generated fromplan(), so it can never drift.--skip upload,series_patches(CLI) andskip:(profile key) subtract steps after composition — commenting out a step as an operation. CLI and profile skips union; unknown names fail loudly listing valid steps; a valid-but-absent step is a no-op, so saved profiles inherit future preset changes automatically.--from sign_macosresumes the tail of the composed (post-skip) plan — re-run after a failure without recompiling. CLI-only, by design.modules: [...]as an explicit enumerated-pipeline opt-in ("you own this list now") routed through the DIRECT-mode machinery; planner-owned keys and planner CLI flags are rejected beside it. Shipped profiles stay switch-based (drift-tested).Design
plan()stays the single source of truth; the file/CLI layer gets subtraction and visibility instead of enumeration.skipjoinsSwitches(validated inresolved(), subtracted asplan()'s last act so composition rules never re-trigger — skippingsign_windowscannot resurrectmini_installer),slice_fromis a planner helper the CLI applies, andload_profilereturns aProfilecarrier so modules-profiles can't masquerade as planner input.--show-planexits before the banner, before chromium_src resolution, and before env preflight. An adversarial review verifiedplan()output for skip-free switches is byte-identical to base across a 324-combination switch/arch/platform matrix.Test plan
uv run python -m unittest discover -s bos_build -t . -p "*_test.py"— 333 tests green (46 new: skip/from goldens, Profile parsing + rejection rules, CliRunner projection tests that scrubCHROMIUM_SRCto prove the no-checkout property)uv run ruff check bos_build— cleanuv run browseros build --preset release --skip upload --from configure --show-planwithCHROMIUM_SRCunset — prints plan + env, exits 0--show-planon the checkout-less runner🤖 Generated with Claude Code