Harden installer failure paths and add battle-test regressions

[malik-na]

Interrupted installations could resume against a changed APFS partition, resize changed geometry before rejecting it, or partially overwrite a repair before discovering an invalid later image. This change rejects those conditions before the next disk operation and adds regression coverage for nine failure paths.

Changes

• Validate saved partition identity before resuming stage one, and installed content before resuming Recovery. Fresh installs avoid redundant readback.
• Reject changed resize geometry before mutation; preflight every repair image entry and enforce partition capacity and raw-device alignment.
• Require actual download completion before publishing verified state. Bind prefetch reuse to the artifact and destination, and honor cancellation during hashing, assembly and promotion.
• Distinguish proven app-side pre-submission failures from uncertain helper outcomes. Initial installation can prepare a fresh plan; Recovery retry retains its eligibility; post-submission connection loss remains locked.
• Guard quit and window dismissal during execution, keep a single installer window, show the planned release/target, expose accessible progress, respect Reduce Motion and provide copyable Recovery steps.
• Verify temporary engine archives before replacing the previous good build; clean up rejected output.
• Document the scenario matrix, remaining practical improvements and physical qualification requirements in docs/battle-testing.md.

Validation

Tested commit: 068f809786f2aa814386ae8fdd845f58a6456981, based on 7a61e0a604ae812ddf9c1ff0f322ad301f6da7fb.

Host: arm64 macOS 27.0 (26A428), Xcode 27.0 (27A266a), Swift 6.4, XcodeBuildMCP 2.7.0, Bash 5.3.15 and Python 3.14.7.

• Portable suite: 173 Python tests passed (38 tooling, 111 engine overlay, 24 catalog/release), plus shell and packaging fixtures.
• Swift via XcodeBuildMCP: 497 debug and 490 release tests passed, with zero failures. One private M3 catalog test was skipped in each configuration because OMARCHY_PRIVATE_CATALOG was not supplied.
• Strict Swift formatting and Git whitespace checks passed.
• Independent image checks: 20 payload tests, 11 lab-access tests, and image input/build shell fixtures passed.
• New download, resume, resize, repair, cancellation and rejected-build regressions reproduced failures before their fixes. Engine-build tests use disposable mocked build tools and exercise mode, size, digest and success cases.

Limits and release boundary

Native visual, keyboard and VoiceOver behavior remains unverified: simulation processes launched, but UI automation could not attach. The complete image-builder suite stopped because GnuPG was unavailable. Three pre-existing weak-capture warnings remain in release compilation.

No physical installation, privileged helper registration, production signing or authenticated engine rebuild was performed. Source locks, trust roots and hardware restrictions—including the apple,j614s block—are unchanged. The overlay edits require a separately reviewed engine rebuild/repack and updated authenticated artifact identity before distribution; this PR is not an installation release.

[maralcbr]

PR #29 review: Harden installer failure paths and add battle-test regressions

Reviewed at head 48ffd49 (main 6f1b40d merged in). Since my first pass at cbc7581, the only change is that merge, which brings in main's image-builder work from #33 and doesn't touch any file in this PR, so the notes below still apply line for line. CI green. On this head, the engine and overlay suites pass (38 + 111). With the three overlay sources and Engine/build-locked-engine.sh reverted to main, 15 overlay tests and 3 build-script subtests fail, so the new tests really cover the new guards. Swift passes on this head too: 564 XCTest (1 skipped) + 4 Swift Testing locally in debug, and 566 debug / 556 release (2 skipped each) in CI.

Verdict

Merge after small changes. Thanks, Naeem, this is careful work: the resize check is evaluated on raw geometry before anything is written, the repair preflight validates every member before a device is opened, and the unlock after a pre-submission failure is scoped to the steps that really run before the single execute.

Done since the first pass: the Swift suite now runs on the current head (above), so item 3 is covered. Updating the validation section of the PR body to match would still be nice.

Requested before merge

1. Repair manifest compatibility (Engine/overlay/src/omarchy_repair.py:465-474). Rewritten roles now need before/after sizes within partition capacity and a multiple of 4096. That matches the rule for image members, but the manifest producer isn't in this repo, so a manifest we already ship with an unaligned existing_content size would silently yield no repair candidate. Please check the producer (or a real shipped manifest) and add a test with one, and write the producer contract down in docs/battle-testing.md.
2. A clear error when a prepared resume doesn't match (Engine/overlay/src/omarchy_asahi.py:540-549). Exact identity is right (the ±5 % discovery tolerance still applies when the partition is first found), but a mismatch currently falls to the unclassified failure in Sources/OmarchyAppleInstaller/EngineFailureDiagnostics.swift:32-46. Please give it its own recoverable diagnostic that keeps the journal and says what was already changed, so a Mac isn't left in the locked state with no explanation.

Suggestions

• InstallerSession.submit() sets isExecuting (Sources/OmarchyInstallerUXCore/InstallerSession.swift:524) before waitUntilPayloadVerified() (:529), and a failure there isn't wrapped as pre-submission. Install can't get there today, because canStartInstallation requires a verified payload, so this is defensive only: either wrap it in InstallerPreSubmissionFailure or add a test that pins the invariant.
• InstallerPreSubmissionFailure (Sources/OmarchyAppleInstaller/InstallerExecutionCoordinator.swift:6) is a public type that unlocks retry. Keep the wrapped sites as narrow as they are now; a short test that execute is never wrapped would guard that.
• The window, Reduce Motion, accessibility, plan identity and copy-steps changes are welcome but unrelated to failure paths. Next time, a separate PR would make each easier to review.

Hardware

The body is right that physical qualification is still owed. Before a release that includes this, run interrupted stage one and Recovery resume on a lab Mac.

[scottjones]

Thanks, this is a solid hardening pass. It stops the engine build from replacing a good engine when a rebuild fails, checks the whole repair payload before writing any partition, confirms the resize source's geometry, re-checks a resumed install's prepared and installed state before the next step, and closes several prefetch and stager cancellation races. ./test/all passes with Python 3.12 (all seven new engine tests included), and swift test passes in debug and release.

Two things to fix before merge, plus a question about the engine lock:

1. Quitting and closing the window are blocked for the whole payload download once credentials are entered, before anything reaches the helper (inline below).
2. Wrapping the connection and ping errors hides the specific "installation service isn't responding" advice (inline below).
3. Engine/source-lock.json no longer matches the overlay. The SHA-256 of omarchy_asahi.py, omarchy_repair.py, omarchy_stage1.py and their three test files differs from the lock. That's 8 of 20 overlay files, against 2 on main (the planner and its test). Is the lock refresh and locked-engine rebuild planned for this PR or a follow-up? None of the engine fixes here reach users until that happens.

[scottjones]

isExecutionInProgress just reads isExecuting, and submit() sets that at InstallerSession.swift:524, before waitUntilPayloadVerified() at line 529. So if the owner enters their password while the multi-GB payload is still downloading, or while it's paused on a metered network, the app can't be quit (this line beeps and returns .terminateCancel) and the close button is disabled (line 149). dismissCredentials() also refuses while isExecuting is true (InstallerSession.swift:467). Nothing has been sent to the helper yet, so the only way out is Force Quit.

Before this change, quitting at that point was allowed, and cancelPrefetchOnQuit (line 146) handled cleanup. Could the block depend on the request actually having been sent, for example a flag set just before environment.execute(...) at line 532? Or split isExecuting into "waiting for payload" and "submitted". A test for "quit allowed while waiting for the payload" would pin it down.

[scottjones]

Same condition as line 12: the close button stays disabled for the whole payload wait. Whatever fixes line 12 should apply here too.

[scottjones]

Wrapping every error from AuthenticatedEngineXPCSubmitter.init and ping() in InstallerPreSubmissionFailure makes the .helperUnresponsive handling at PlainLanguage.swift:554 unreachable. That error is only thrown by ping() (AuthenticatedEngineXPCSubmitter.swift:113 and :131), and PlainLanguage.swift:524 matches the wrapper first, returning the generic message without looking inside.

So when the helper isn't registered or isn't answering, the owner no longer sees "Run the downloaded package again, then reopen this app". They see "choose Check again to prepare and review a fresh plan", which can't fix a missing helper, so they can go round in circles.

Suggestion: let EngineXPCSubmissionError.helperUnresponsive (and any other specific connection error) through unwrapped, or have the InstallerPreSubmissionFailure case at PlainLanguage.swift:524 check preflight.underlying for errors that have their own message first. A test sending a .helperUnresponsive ping through the coordinator and checking the resulting FailureDisplay would cover it.

[scottjones]

Optional nit: the generation check under the lock is the right fix. It does mean the file move happens while holding the lock. That's a quick rename within the same folder, so it's fine unless the destination can ever be on another volume, where moveItem becomes a copy.

[malik-na]

Pushed the review-response fixes in ebe76e6 and the validation/evidence notes through 36c41c4.

Fixed

• Quit/close protection now begins immediately before helper execution, after payload verification. Cancellation during the wait prevents a late submission. A failed wait is treated as a pre-submission failure. The normal initial-install UI already requires a verified payload, so the delayed-wait regression is defensive coverage.
• Wrapped helper connection/ping failures retain the specific package-reinstallation advice, including the saved checkpoint during Recovery retry. A regression also verifies that execution failures remain unwrapped.
• Prepared-resume target mismatches now have a named diagnostic explaining that disk preparation already occurred. The journal is preserved, with no blind fresh-install or retry action over uncertain disk state.
• Added an unchanged, checksum-pinned repair manifest from the recorded physical M1 canary and a candidate-discovery test. Documented the required size/alignment contract.

Local validation

At source commit ebe76e6: portable checks passed (174 Python tests plus shell/packaging fixtures); Swift debug passed 577 XCTest + 4 Swift Testing tests, and release passed 567 XCTest + 4 Swift Testing tests. Each Swift configuration skipped the private M3 catalog test because its input was unavailable. Strict formatting, syntax/compilation, and whitespace checks passed. Native simulation verified execution-time quit/close protection and the prepared-resume failure screen. Subsequent commits change documentation only. Full validation record.

Remaining P2 gap: released repair-manifest compatibility

Marcelo's request remains open. The real fixture comes from an explicitly non-release canary, so it cannot establish compatibility with production output. I verified public Stable/RC catalog signatures and metadata, inspected image-builder source, and checked the published engine archive against its release checksum. These establish fresh-install metadata and image alignment, but do not establish how repair manifests choose existing_content[role].size_bytes. The public repair preparation/release tools accept a manifest as an external input; I could not locate its generator or a manifest tied to a published release in the inspected sources. Evidence and search limits.

@maralcbr, could you provide the repair-manifest producer at a specific revision with representative input/output, or an unchanged manifest and the release that distributed it? That will let us test the actual existing/replacement content sizes against partition capacity and 4096 alignment rather than assume compatibility.

For Scott's engine-lock question: the lock refresh and authenticated engine rebuild remain an explicit, separately reviewed release follow-up. The pinned archive does not yet contain these engine fixes. Interrupted stage-one and Recovery-resume qualification on authorized supported hardware also remains required before release; local tests and simulation do not replace it.