refactor(browseros): universal build as planner runs

[Nikhil (shadowfax92)]

Summary

• Replaces the universal_build mega-step (a 350-line pipeline-within-a-step that re-entered six steps per arch and bypassed the runner) with three planner-emitted runs: arm64 build → x64 build → merge_universal + sign/package/upload, executed by the existing cli/build.py runs loop.
• New planner API plan_runs(switches) -> [(arch, steps)] consumed for all preset builds; non-universal plans stay byte-identical (review verified 576 switch combinations against the old planner).
• New ~70-line merge_universal step (macos-only, optional): merges the two deterministic per-arch out dirs via the vendored universalizer_patched.py into ctx(universal).get_app_path(), registers built_app for sign_macos.
• Deletes Context._fixed_app_path (redundant now that get_app_path resolves strictly from the ctx's own out dir), the planner universal tail special-case, and dead universal helpers: sign_universal (macos/linux/windows) and merge.py's create_minimal_context/merge_sign_package/handle_merge_command — all verified caller-less. Net −350 lines.

Behavior changes (intentional)

• Upload failures now fail the run loudly; the old per-arch try/except swallow is gone.
• Every universal phase now emits runner lifecycle events (per-step Slack/terminal notifications, per-run banners, upfront preflight for all three runs).
• Universal explicitly rejects --no-sign (the old flow always signed), mixing universal with other arches, and debug/off-macOS as before.
• clean (run 1) resets the tree and removes arm64's out dir; x64 rebuilds incrementally against the freshly reset+repatched tree instead of being force-rmtree'd; merge_architectures still removes a stale universal output app.

Design

Universal is sequential runs with deterministic paths — the CLI already had the runs loop (multi-arch Linux). Run 1 carries the prep prefix exactly as the old universal YAML did (prep once, no outer resources), runs 1–2 share the per-arch tail with resources staged per arch, run 3 merges. Rejections are raised at planning time, before chromium_src resolution, so bad invocations die instantly. Universal has no CI lane; the safety net is the runs-shape golden asserting the three step lists verbatim plus per-run required_env parity with the deleted release YAMLs.

Note: .llm/0701-build-refactor/design.md (gitignored design log) says universal is a mega-step — superseded by this PR.

Test plan

• uv run python -m unittest discover -s bos_build -t . -p "*_test.py" — 300 tests green (287 → 300)
• uv run ruff check bos_build clean; git diff --check clean
• Smokes: browseros build --list (shows merge_universal, no universal_build), browseros product doctor
• --preset release --arch universal --no-sign and --preset debug --arch universal fail fast with clear planning errors
• Adversarial review round: 0 critical / 0 important; 3 minor findings fixed (EOF whitespace, per-run pipeline summary)
• Next official universal release exercises the three-run flow end to end (no CI lane exists for universal)

🤖 Generated with Claude Code

[Nikhil (shadowfax92)]

Greptile (@greptileai) review

[Nikhil (shadowfax92)]

Claude (@claude) review

[claude]

Claude finished Nikhil (@shadowfax92)'s task in 4m 21s —— View job

---

Code Review

Tasks

• Gather context and read CLAUDE.md
• Review diff and key changed files
• Analyze planner changes (planner.py)
• Analyze universal step changes (universal.py)
• Analyze build CLI changes (build.py)
• Analyze context/merge/sign changes
• Post comprehensive review findings

---

Summary

This is a clean, well-motivated refactor. Replacing the 350-line universal_build mega-step (a pipeline-within-a-step that bypassed the runner entirely) with three planner-emitted runs is the correct architectural direction — it reuses the existing multi-arch runs loop, gets proper Slack/terminal lifecycle events, and makes universal a first-class citizen rather than a hidden side-channel.

No critical bugs found. A few observations below, mostly minor.

---

planner.py

plan() now raises for universal — intentional API break.
The new error message "universal is planned as multiple runs; use plan_runs()" is developer-facing and clear. Just worth noting that any external callers of plan(switches, "universal") will see a hard ValueError rather than a step list. The test test_flat_plan_rejects_universal covers this.

Double-resolution of switches.resolved() in plan_runs() → plan().
plan_runs() calls switches.resolved() then delegates to plan() for non-universal arches, which calls switches.resolved() again. resolved() appears to be idempotent (fills None fields with preset defaults), so this is harmless, but it's a minor inefficiency. Could pass a sentinel or restructure plan() to accept pre-resolved switches. Low priority.

Error precedence in _plan_universal_runs. The PR says "preset, then platform, then sign" — the code matches this. The new test test_debug_rejection_wins_over_platform covers debug winning over platform. One small gap: there's no test asserting platform wins over sign (e.g., plan_runs(Switches(preset="release", architectures=("universal",), sign=False), "linux") — should raise the platform error, not the sign error). Cosmetic, but completing the precedence matrix in tests would be consistent.

("x64", list(arch_tail)) — correct.
The x64 run intentionally has no clean step. A stale x64 out_dir from a previous failed universal run won't be cleaned before run 2 starts (unlike the old mega-step which force-cleaned both dirs upfront). This is a behavioral change the PR acknowledges and is reasonable for incremental rebuild semantics. Worth flagging to anyone debugging a "why did my x64 build pick up stale artifacts" issue in the future — the clean step for x64 can always be forced by a separate invocation.

---

steps/compile/universal.py (MergeUniversalModule)

validate() checks both input apps exist just-in-time — correct placement.
Checking the per-arch apps in validate() rather than preflight() is the right call: at preflight time (before any run starts) the arm64/x64 outputs don't exist yet. The comment explaining this is helpful.

merge_architectures in merge.py re-validates the input paths (lines 31–37 of merge.py), which validate() already checked with a ValidationError. This means the merge function logs log_error(...) and returns False for paths that validate() already rejected as ValidationError. For this call path the inner check is dead, but merge_architectures is also a public helper, so the self-contained validation in merge.py is defensible. Minor duplication.

execute() raises RuntimeError on merge failure, doesn't clean up.
If merge_architectures returns False after partially creating output_path, the partial app directory is left behind (the parent directory is created on line 54 of merge.py, the stale output is removed on line 57–59, but if the universalizer command fails mid-run, a partial .app bundle could remain). This matches the old mega-step's behavior and is probably acceptable since a retry would clean anyway, but it's worth noting.

produces = ["built_app"] / requires = [].
requires = [] is correct — the arm64/x64 apps come from separate prior runs, not from the same pipeline's produces chain. The validate() check substitutes for the missing inter-run dependency declaration. This is the right design given the framework.

---

cli/build.py

Early-failure of planning before _resolve_chromium_src.

planned = plan_runs(switches)
src = _resolve_chromium_src(...)

This is a good ordering: an invalid invocation (e.g. --arch universal --no-sign) raises a planning error immediately, before the CLI tries to resolve CHROMIUM_SRC from the environment. Clear UX win.

Pipeline summary banner for multi-run (line 285–292).

if len(runs) > 1:
    for run_ctx, run_steps in runs:
        log_info(f"📍 Pipeline[{run_ctx.architecture}]: {' → '.join(run_steps)}")
else:
    log_info(f"📍 Pipeline: {' → '.join(runs[0][1])}")

For a --preset release --arch arm64 build (single run), this still hits the else branch and shows one line. Good. For multi-arch Linux (two runs), it shows two lines. For universal (three runs), it shows all three run shapes. Correct.

Whole-pipeline preflight fires for all runs before any run starts.

for run_ctx, run_steps in runs:
    preflight(run_steps, ctx=run_ctx)

This is important: a misconfigured third run (e.g., wrong env) aborts before any work is done. The design note in the comment confirms the intent. Good.

---

core/context.py

Removing _fixed_app_path.
The old pin-the-path hack in _create_arch_context() / _create_universal_context() was essentially working around get_app_path()'s resolution logic by overriding it per-instance. Now get_app_path() resolves purely from out_dir (which encodes architecture), so the hack is correctly removed. The _arch_app_path() helper in universal.py derives sibling paths by constructing sibling Context objects — same source of truth, no override needed.

---

steps/sign/*.py and steps/package/merge.py

Dead code deletion is clean. The three sign_universal(contexts) functions (macos/linux/windows) were all caller-less and correctly removed. The create_minimal_context / merge_sign_package / handle_merge_command helpers in merge.py are gone, leaving only the merge_architectures primitive that MergeUniversalModule uses.

---

Test Coverage

• Golden test test_macos_universal now asserts the three-run shape verbatim — this is the key regression guard for the planner.
• test_non_universal_runs_match_flat_plan_per_arch verifies parity between plan_runs and plan for non-universal arches — good for catching future drift.
• UniversalRunsTest adds six new cases: flat plan rejection, sign rejection, multi-arch rejection, noupload propagation, non-universal parity.
• UniversalEnvTest verifies all three runs have signing env vars (via sign_macos).
• universal_test.py covers preflight, validate, execute, and step registration. The mock of merge_architectures is appropriate — no real filesystem ops needed for the logic tests.

One gap worth noting: the noupload case in test_noupload_drops_upload_from_every_run verifies upload not in steps, but the universal run tail ends with package_macos. A test confirming the universal run still contains merge_universal first (order check) would be slightly more complete, but this is cosmetic.

---

Overall Verdict

Looks good to merge. The design is sound, the deletion is aggressive in the right places, and the test coverage is solid for the planner-shape invariant that's most likely to regress. The main behavioral changes (upload failures now loud, universal now requires --sign, x64 incremental rebuild) are intentional and documented.

[greptile-apps]

Greptile Summary

This PR replaces the 350-line UniversalBuildModule mega-step (which re-entered six steps per arch internally, bypassed the runner lifecycle, and swallowed upload errors) with three planner-emitted runs executed by the existing CLI runs loop: arm64 build+sign+package+upload → x64 build+sign+package+upload → merge_universal+sign+package+upload. A new plan_runs() API returns (arch, steps) pairs; non-universal paths are byte-identical to before, verified against 576 switch combinations.

• New merge_universal step (~70 lines): validates both per-arch apps exist just-in-time, calls merge_architectures, registers built_app in the artifact registry; sign_macos/package_macos/upload in run 3 then treat the merged app like any other build.
• Dead code removed: _fixed_app_path, sign_universal (all three platforms), create_minimal_context/merge_sign_package/handle_merge_command in merge.py, and the old universal_build planner tail — all verified caller-less.
• Behavior changes: upload failures now fail loudly (no per-arch try/except swallow); universal now explicitly rejects --no-sign, debug preset, and non-macOS at planning time before any environment resolution.

Confidence Score: 4/5

Safe to merge; the refactoring is logically sound and well-tested, with the only gap being the absence of a CI lane for end-to-end universal validation (acknowledged in the PR).

The 3-run planner design is clean and the step contracts hold: merge_architectures creates the output directory, the arm64/x64 .app bundles survive sign_macos/package_macos for run 3's validate check, and all rejection conditions (debug, non-macOS, --no-sign, mixed arches) are caught at planning time. Dead code removal is thorough and verified. The main residual risk is the universal path being exercised only on the next official release — no CI lane exists to catch a regression before then.

The _plan_universal_runs function in core/planner.py and MergeUniversalModule in steps/compile/universal.py are the new load-bearing code paths that can only be fully validated by an actual universal release build.

Important Files Changed

Filename	Overview
packages/browseros/bos_build/cli/build.py	Switches from plan to plan_runs, adds per-run pipeline logging for multi-run builds, plans before resolving chromium_src for fast failure on invalid invocations.
packages/browseros/bos_build/core/planner.py	New plan_runs() public API emits (arch, steps) pairs; plan() now raises ValueError for universal; _plan_universal_runs() encodes 3-run sequential shape with prep-once semantics.
packages/browseros/bos_build/steps/compile/universal.py	Complete rewrite: 349-line UniversalBuildModule mega-step replaced by 71-line MergeUniversalModule that only merges per-arch apps; validate() does just-in-time app existence checks, preflight() statically checks universalizer script.
packages/browseros/bos_build/core/context.py	Removes _fixed_app_path field and its short-circuit in get_app_path(); get_app_path() now always resolves strictly from out_dir, which is already deterministic per arch.
packages/browseros/bos_build/steps/package/merge.py	Removes three dead helpers (create_minimal_context, merge_sign_package, handle_merge_command) verified as caller-less; merge_architectures retained unchanged.
packages/browseros/bos_build/core/planner_test.py	Replaces old universal golden with new 3-run golden; adds UniversalRunsTest with 5 new tests covering rejection conditions, noupload variant, and plan_runs/plan parity for non-universal arches.
packages/browseros/bos_build/steps/compile/universal_test.py	New test file with 7 tests covering validate (non-universal ctx rejection, missing arch app, both present), execute (merge called correctly, failure raises, registry), and preflight (vendored script present/absent).
packages/browseros/bos_build/steps/sign/macos.py	Removes dead sign_universal function (caller-less after UniversalBuildModule deletion) and unused sys import.
packages/browseros/bos_build/steps/sign/linux.py	Removes dead sign_universal stub and unused imports (List, log_warning).
packages/browseros/bos_build/steps/sign/windows.py	Removes dead sign_universal stub (Windows doesn't support universal binaries).

_{Reviews (1): Last reviewed commit: "fix(browseros): address review findings ..." | Re-trigger Greptile}

[greptile-apps]

Greptile Summary

This PR replaces the universal_build mega-step — a 350-line pipeline-within-a-step that internally re-ran six pipeline stages per arch — with three proper sequential runs emitted by a new plan_runs() planner API, then executed by the existing runner loop. Dead helpers (sign_universal ×3, create_minimal_context, merge_sign_package, handle_merge_command, Context._fixed_app_path) are removed, and the new MergeUniversalModule is a lean ~70-line step that merges the two deterministic per-arch out-dirs and registers built_app for sign_macos.

• New plan_runs(switches) → [(arch, steps)] replaces the per-arch plan() call loop; for non-universal switches the output is byte-identical to the old planner (golden-tested against 576 combinations).
• Upload failures in the universal flow now propagate loudly — the old per-arch try/except swallow in UniversalBuildModule is gone, which is a correct behavioral improvement.
• New universal_test.py adds 13 focused unit tests covering validate, execute, preflight, and step registration for MergeUniversalModule.

Confidence Score: 4/5

The three-run universal flow is well-structured and the new planner API is cleanly tested; the main untested surface is the live universal build end-to-end, which has no CI lane by design.

The refactor correctly moves upload failures from silently-swallowed exceptions to loud pipeline failures, removes _fixed_app_path in favour of deterministic path resolution, and deletes a substantial block of dead code. The golden tests cover 576 switch combinations and the new step has dedicated unit tests. Two style observations (banner label, heavyweight Context init in _arch_app_path) are non-blocking.

No files have logic issues; steps/compile/universal.py is the most novel code and would benefit from a live universal build smoke before the next release.

Important Files Changed

Filename	Overview
packages/browseros/bos_build/cli/build.py	Switches from plan to plan_runs and adds multi-run pipeline summary logging; logic is clean and the change is well-scoped.
packages/browseros/bos_build/core/planner.py	Adds plan_runs and _plan_universal_runs; plan() now correctly rejects universal before _plan_release/_plan_debug are called, keeping both helpers arch-agnostic and the path positions for resources preserved.
packages/browseros/bos_build/core/planner_test.py	Replaces the old UniversalEnvTest with UniversalRunsTest; adds rejection tests for multi-arch combinations, no-sign, debug precedence, and upload-less variant; new golden asserts the three-run shape verbatim.
packages/browseros/bos_build/steps/compile/universal.py	Shrinks from 349 to 71 lines; MergeUniversalModule correctly defers per-arch app existence checks to validate() (just-in-time) since inputs are produced by prior runs, and uses _arch_app_path to single-source the out-dir scheme.
packages/browseros/bos_build/steps/compile/universal_test.py	New test file with solid coverage of validate (non-universal context, missing arch apps, both present), execute (merge called with correct args, artifact registered, merge failure raises), preflight (missing universalizer), and step registration metadata.
packages/browseros/bos_build/steps/package/merge.py	Removes create_minimal_context, merge_sign_package, and handle_merge_command (confirmed caller-less); merge_architectures itself is unchanged.
packages/browseros/bos_build/core/context.py	Removes _fixed_app_path field and its short-circuit in get_app_path(); the docstring is updated to reflect that universal now resolves via its own out_dir naturally.
packages/browseros/bos_build/steps/sign/macos.py	Removes the dead sign_universal function (was caller-less); the remaining signing code is untouched.
packages/browseros/bos_build/steps/sign/linux.py	Removes dead sign_universal stub and an unused List import; no functional change.
packages/browseros/bos_build/steps/sign/windows.py	Removes dead sign_universal stub (Windows-specific no-op); no functional change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    CLI["--preset release --arch universal"] --> PR["plan_runs(switches)"]
    PR --> R1["Run 1 arm64\nclean, git_setup, sparkle_setup, download_resources\nbundled_extensions, chromium_replace, string_replaces\nseries_patches, patches, resources, configure\ncompile, sign_macos, package_macos, upload"]
    PR --> R2["Run 2 x64\nresources, configure, compile\nsign_macos, package_macos, upload"]
    PR --> R3["Run 3 universal\nmerge_universal, sign_macos, package_macos, upload"]
    R1 -->|sequential| R2
    R2 -->|sequential| R3
    R1 -->|arm64 app at deterministic out_dir| MU
    R2 -->|x64 app at deterministic out_dir| MU
    MU["merge_universal\n_arch_app_path x2 via universalizer_patched.py"]
    MU -->|built_app registered| SA["sign_macos, package_macos, upload"]
    style R1 fill:#d4edda,stroke:#28a745
    style R2 fill:#d4edda,stroke:#28a745
    style R3 fill:#cce5ff,stroke:#0d6efd
    style MU fill:#fff3cd,stroke:#ffc107

Loading

%%{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
    CLI["--preset release --arch universal"] --> PR["plan_runs(switches)"]
    PR --> R1["Run 1 arm64\nclean, git_setup, sparkle_setup, download_resources\nbundled_extensions, chromium_replace, string_replaces\nseries_patches, patches, resources, configure\ncompile, sign_macos, package_macos, upload"]
    PR --> R2["Run 2 x64\nresources, configure, compile\nsign_macos, package_macos, upload"]
    PR --> R3["Run 3 universal\nmerge_universal, sign_macos, package_macos, upload"]
    R1 -->|sequential| R2
    R2 -->|sequential| R3
    R1 -->|arm64 app at deterministic out_dir| MU
    R2 -->|x64 app at deterministic out_dir| MU
    MU["merge_universal\n_arch_app_path x2 via universalizer_patched.py"]
    MU -->|built_app registered| SA["sign_macos, package_macos, upload"]
    style R1 fill:#d4edda,stroke:#28a745
    style R2 fill:#d4edda,stroke:#28a745
    style R3 fill:#cce5ff,stroke:#0d6efd
    style MU fill:#fff3cd,stroke:#ffc107

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/browseros/bos_build/cli/build.py:274-277
The "multi-arch loop" label is slightly misleading for a universal build, where the three runs are sequential pipeline phases (arm64 build → x64 build → merge), not independent builds of the same product. An operator reading logs could reasonably expect all three entries to be arch-specific compilation runs. Consider "sequential runs" to match the PR's own terminology.

```suggestion
    if len(runs) > 1:
        log_info(
            f"📍 Architectures: {[c.architecture for c, _ in runs]} (sequential runs)"
        )
```

### Issue 2 of 2
packages/browseros/bos_build/steps/compile/universal.py:25-33
**Instantiating a full `Context` just to derive a sibling path**

`Context.__post_init__` reads version files from `chromium_src` (semantic version, Chromium version, build offset) and initialises `ArtifactRegistry` and `EnvConfig`. `_arch_app_path` is called twice per `validate()` invocation and twice in `execute()`, so each `merge_universal` run triggers four full `Context` initialisations at job-execution time.

For a build that typically takes hours this is negligible, but it's worth knowing the helper is not just a path computation — if version-file reads fail on a partially initialised tree (e.g., after `clean` wiped them), the error surface at merge time would be a confusing `Context` init failure rather than a clear merge-step error. The existing contract ("deterministic out dirs") means a thin standalone helper would be equally correct and cheaper. No change required unless the `Context` init ever becomes fallible.

_{Reviews (2): Last reviewed commit: "fix(browseros): address review findings ..." | Re-trigger Greptile}