Fix skipIf inversion, unchecked download status, and dropped preflight scripts - #27
Merged
Merged
Conversation
…t scripts Three defects in the manifest and download path. skipIf was interpreted twice with opposite senses. The stage loops used the InstallApplications sense (skip when the named architecture is the current one) while the download gate in ManifestManager used the inverse, so on the very architecture an item was written for the payload was never fetched and the download reported success. The install or script step then ran against a file that was not there, and the log said "skipped" rather than "failed". Both call sites now use one shared helper, ArchitectureSkip, which keeps the orchestrator sense; the two private copies are gone. The download response status was never inspected. Whatever a server returned — a 404 page, a CDN error document, an auth challenge — was written to the destination as though it were the payload, surfacing later as a hash mismatch or, for an item with no hash, not at all. Any non-2xx now fails the download and logs the status code and the URL, with 401 and 403 called out as the signature of a private origin without a valid Authorization header. DownloadError also conforms to LocalizedError so that detail reaches the log rather than a generic Foundation description. Only the first preflight rootscript ran. The stage picked items.first(where:) with no loop and no warning, so a preflight of four scripts silently ran one and nothing in the log distinguished that from a preflight of one. The stage now iterates in manifest order like the other two, applying the existing short-circuits per item: exit 0 still skips the rest of the bootstrap, a download failure or a negative exit code still fails the stage, and exit 1+ moves on to the next script. Preflight remains rootscript-only, but a non-rootscript item is now named in a warning instead of dropped in silence, and per-item skipIf is honoured the way it is in the other stages. Adds tests pinning the skipIf helper in both directions. Fixes #13 Fixes #21 Fixes #22 Claude-Session: https://claude.ai/code/session_01LQxgdJHaS4UhjJnJueqhcs
rodchristiansen
added a commit
that referenced
this pull request
Sep 5, 2026
Both branches changed IAOrchestrator. #27 replaced the private shouldSkipForArchitecture and getCurrentArchitecture helpers with the shared ArchitectureSkip type; this branch rewrote waitForUserSession to be bounded by the userlandLoginTimeout preference. Resolved by keeping the bounded waitForUserSession and dropping both architecture helpers, which the shared type supersedes. Claude-Session: https://claude.ai/code/session_01LQxgdJHaS4UhjJnJueqhcs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three bugs in the manifest/download path, which is why they are one PR.
skipIfwas evaluated twice with opposite senses (#13)IAOrchestrator.shouldSkipForArchitectureused the InstallApplications sense — skip when the named architecture is the current one — whileManifestManager.shouldSkip, which gated the download, returned the opposite. So on the architecture an item was written for, the stage loop correctly ran it whiledownloadIfNeededdecided it was skipped, loggedSkipping … due to skip_if:and returned success without fetching. The install then ran against a missing file and the log blamed a skip rather than a failure.Both private copies are deleted and replaced by one helper,
ArchitectureSkipinSources/core/Utilities/, keeping the orchestrator's sense. It takes the current architecture as a defaulted parameter so the logic is testable without a real machine.The download gate in
downloadIfNeededis kept rather than removed: it is now consistent with the stage loops, and it is the only skip check protecting call sites that do not do their own.Download response status was never inspected (#21)
downloadFilediscarded theURLResponse, so a 404 page, a CDN error document or an auth challenge body was written to the destination as if it were the payload — surfacing one step later as a hash mismatch, or not at all for an item with nohash. Any non-2xx is now a failed download, logging the status code and the URL; 401 and 403 say explicitly that the origin is private and the request carried no validAuthorizationheader.DownloadErrorgained aLocalizedErrorconformance so that detail actually reaches the log instead of a generic Foundation description.Only the first preflight rootscript ran (#22)
runPreflightStageuseditems.first(where: { $0.type == "rootscript" })with no loop and no warning, so a preflight of four scripts silently ran one.Decision: loop, rather than keep it single-script. The stage now iterates in manifest order like setupassistant and userland, and the existing short-circuits are preserved per item — exit 0 still returns
.skipBootstrapimmediately, a download failure or a negative exit code still returns.failedimmediately, and exit 1+ moves on to the next script. The status plist records the last exit code reached. Preflight is still rootscript-only (that is a spec constraint, not an accident), but a non-rootscript item is now named in a warning instead of dropped in silence, and per-itemskipIfis honoured here the way it is in the other two stages — previously the preflight script went straight todownloadIfNeededwith no skip check of its own.Tests
ArchitectureSkipTestspins the helper in both directions, case-insensitively, plus the unrecognized-value case. Added in the existing swift-testing style of the file.Not compiled or tested locally
This was written on a machine with no Swift toolchain, so nothing here was built or run — it relies entirely on CI. Please treat the build and test results as the first check.
What a reviewer should look at first: the sense of
ArchitectureSkip.shouldSkip(a second inversion here would be worse than the bug), and the preflight loop's short-circuit ordering against the old single-script flow.https://claude.ai/code/session_01LQxgdJHaS4UhjJnJueqhcs