From 148bba31617a3a94851e1e79e4203c4816cc3e55 Mon Sep 17 00:00:00 2001 From: Rod Christiansen Date: Fri, 4 Sep 2026 22:55:38 -0700 Subject: [PATCH] Fix skipIf inversion, unchecked download status, and dropped preflight scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Sources/core/Managers/IAOrchestrator.swift | 134 +++++++++--------- Sources/core/Managers/ManifestManager.swift | 30 +--- Sources/core/Managers/NetworkManager.swift | 30 +++- Sources/core/Utilities/ArchitectureSkip.swift | 52 +++++++ .../BootstrapMateCoreTests.swift | 41 ++++++ 5 files changed, 190 insertions(+), 97 deletions(-) create mode 100644 Sources/core/Utilities/ArchitectureSkip.swift diff --git a/Sources/core/Managers/IAOrchestrator.swift b/Sources/core/Managers/IAOrchestrator.swift index df272c2..bce415f 100644 --- a/Sources/core/Managers/IAOrchestrator.swift +++ b/Sources/core/Managers/IAOrchestrator.swift @@ -153,49 +153,77 @@ public final class IAOrchestrator { StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .running) DialogManager.shared.notifyPhaseStarted(phase: "Preflight") - // Preflight only supports a single rootscript per InstallApplications spec - guard let firstScript = items.first(where: { $0.type == "rootscript" }) else { + // Preflight runs rootscripts only, per the InstallApplications spec. Any + // other item type in this stage is ignored — name it rather than dropping + // it silently, so a mistyped manifest is visible in the log. + for item in items where item.type != "rootscript" { + Logger.warning("Ignoring preflight item \(item.name ?? item.file): preflight supports rootscript only (type: \(item.type))") + } + + let scripts = items.filter { $0.type == "rootscript" } + guard !scripts.isEmpty else { Logger.info("No preflight rootscript found, continuing with bootstrap") StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .skipped) return .continueBootstrap } - - let displayName = firstScript.name ?? firstScript.file - DialogManager.shared.addListItem(name: displayName, status: .pending) - - Logger.writeProgress("Running preflight script", displayName) - DialogManager.shared.updateListItem(name: displayName, status: .wait, statusText: "Running...") - - // Download if needed - if !ManifestManager.shared.downloadIfNeeded(firstScript) { - Logger.error("Failed to download preflight script: \(displayName)") - DialogManager.shared.notifyPackageFailure(packageName: displayName, error: "Download failed") - StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .failed, errorMessage: "Download failed") - return .failed + + // Add all scripts to dialog + for script in scripts { + DialogManager.shared.addListItem(name: script.name ?? script.file, status: .pending) } - - // Run the script and capture exit code - let exitCode = ScriptManager.shared.runScriptWithExitCode(firstScript) - - if exitCode == 0 { - // Exit 0 = Skip bootstrap, machine is already configured - Logger.success("Preflight script exited 0 - skipping bootstrap") - DialogManager.shared.updateListItem(name: displayName, status: .success, statusText: "Already configured") - StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .completed, exitCode: 0) - return .skipBootstrap - } else if exitCode > 0 { - // Exit 1+ = Continue with bootstrap - Logger.info("Preflight script exited \(exitCode) - continuing with bootstrap") - DialogManager.shared.updateListItem(name: displayName, status: .success, statusText: "Continue setup") - StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .completed, exitCode: Int(exitCode)) - return .continueBootstrap - } else { - // Negative exit code = error - Logger.error("Preflight script failed with exit code \(exitCode)") - DialogManager.shared.notifyPackageFailure(packageName: displayName, error: "Exit code: \(exitCode)") - StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .failed, errorMessage: "Exit code: \(exitCode)", exitCode: Int(exitCode)) - return .failed + + // Run every script in manifest order. The short-circuits are unchanged, + // now applied per item: exit 0 skips the rest of the bootstrap, a download + // error or a negative exit code fails the stage, and exit 1+ moves on to + // the next script. + var lastExitCode: Int32 = 1 + + for script in scripts { + let displayName = script.name ?? script.file + + // Check architecture skip condition + if let skipIf = script.skipIf, ArchitectureSkip.shouldSkip(skipIf) { + Logger.writeSkipped("\(displayName) (architecture: \(skipIf))") + DialogManager.shared.notifyPackageSkipped(packageName: displayName, reason: "Not for this architecture") + continue + } + + Logger.writeProgress("Running preflight script", displayName) + DialogManager.shared.updateListItem(name: displayName, status: .wait, statusText: "Running...") + + // Download if needed + if !ManifestManager.shared.downloadIfNeeded(script) { + Logger.error("Failed to download preflight script: \(displayName)") + DialogManager.shared.notifyPackageFailure(packageName: displayName, error: "Download failed") + StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .failed, errorMessage: "Download failed") + return .failed + } + + // Run the script and capture exit code + let exitCode = ScriptManager.shared.runScriptWithExitCode(script) + lastExitCode = exitCode + + if exitCode == 0 { + // Exit 0 = Skip bootstrap, machine is already configured + Logger.success("Preflight script exited 0 - skipping bootstrap") + DialogManager.shared.updateListItem(name: displayName, status: .success, statusText: "Already configured") + StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .completed, exitCode: 0) + return .skipBootstrap + } else if exitCode > 0 { + // Exit 1+ = Continue with bootstrap + Logger.info("Preflight script exited \(exitCode) - continuing with bootstrap") + DialogManager.shared.updateListItem(name: displayName, status: .success, statusText: "Continue setup") + } else { + // Negative exit code = error + Logger.error("Preflight script failed with exit code \(exitCode)") + DialogManager.shared.notifyPackageFailure(packageName: displayName, error: "Exit code: \(exitCode)") + StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .failed, errorMessage: "Exit code: \(exitCode)", exitCode: Int(exitCode)) + return .failed + } } + + StatusManager.shared.setPhaseStatus(phase: .preflight, stage: .completed, exitCode: Int(lastExitCode)) + return .continueBootstrap } // MARK: - Setup Assistant Stage @@ -217,7 +245,7 @@ public final class IAOrchestrator { let displayName = item.name ?? item.file // Check architecture skip condition - if let skipIf = item.skipIf, shouldSkipForArchitecture(skipIf) { + if let skipIf = item.skipIf, ArchitectureSkip.shouldSkip(skipIf) { Logger.writeSkipped("\(displayName) (architecture: \(skipIf))") DialogManager.shared.notifyPackageSkipped(packageName: displayName, reason: "Not for this architecture") continue @@ -260,7 +288,7 @@ public final class IAOrchestrator { let displayName = item.name ?? item.file // Check architecture skip condition - if let skipIf = item.skipIf, shouldSkipForArchitecture(skipIf) { + if let skipIf = item.skipIf, ArchitectureSkip.shouldSkip(skipIf) { Logger.writeSkipped("\(displayName) (architecture: \(skipIf))") DialogManager.shared.notifyPackageSkipped(packageName: displayName, reason: "Not for this architecture") continue @@ -401,34 +429,6 @@ public final class IAOrchestrator { return count } - private func shouldSkipForArchitecture(_ skipIf: String) -> Bool { - let currentArch = getCurrentArchitecture() - let skipLower = skipIf.lowercased() - - // ARM-based skip conditions - if (skipLower.contains("arm") || skipLower.contains("apple_silicon")) && currentArch == "arm64" { - return true - } - - // Intel-based skip conditions - if (skipLower.contains("x86_64") || skipLower.contains("intel")) && currentArch == "x86_64" { - return true - } - - return false - } - - private func getCurrentArchitecture() -> String { - var systemInfo = utsname() - uname(&systemInfo) - let machineMirror = Mirror(reflecting: systemInfo.machine) - let chars = machineMirror.children.compactMap { $0.value as? Int8 } - .filter { $0 != 0 } - .map { Character(UnicodeScalar(UInt8($0))) } - let identifier = String(chars) - return identifier.contains("arm64") ? "arm64" : "x86_64" - } - private func waitForUserSession() { Logger.info("Waiting for user session...") DialogManager.shared.updateProgressText(text: "Waiting for user to log in...") diff --git a/Sources/core/Managers/ManifestManager.swift b/Sources/core/Managers/ManifestManager.swift index 6d5e152..128b60a 100644 --- a/Sources/core/Managers/ManifestManager.swift +++ b/Sources/core/Managers/ManifestManager.swift @@ -102,7 +102,7 @@ public final class ManifestManager { let path = item.file let expectedHash = item.hash - if let arch = item.skipIf, shouldSkip(arch: arch) { + if let arch = item.skipIf, ArchitectureSkip.shouldSkip(arch) { Logger.log("Skipping \(item.name ?? path) due to skip_if: \(arch)") return true } @@ -180,34 +180,6 @@ public final class ManifestManager { let digest = hasher.finalize() return digest.map { String(format: "%02x", $0) }.joined() } - - private func shouldSkip(arch: String) -> Bool { - let isArm = arch.contains("arm") || arch.contains("apple_silicon") - let isIntel = arch.contains("x86_64") || arch.contains("intel") - let currentArch = localArch() - - if isArm && currentArch == "arm64" { - return false - } else if isArm && currentArch == "x86_64" { - return true - } else if isIntel && currentArch == "arm64" { - return true - } else if isIntel && currentArch == "x86_64" { - return false - } - return false - } - - private func localArch() -> String { - var systemInfo = utsname() - uname(&systemInfo) - let machineMirror = Mirror(reflecting: systemInfo.machine) - let chars = machineMirror.children.compactMap { $0.value as? Int8 } - .filter { $0 != 0 } - .map { Character(UnicodeScalar(UInt8($0))) } - let identifier = String(chars) - return identifier.contains("arm64") ? "arm64" : "x86_64" - } } public struct BootstrapManifest: Codable { diff --git a/Sources/core/Managers/NetworkManager.swift b/Sources/core/Managers/NetworkManager.swift index de361f4..1324071 100644 --- a/Sources/core/Managers/NetworkManager.swift +++ b/Sources/core/Managers/NetworkManager.swift @@ -5,6 +5,17 @@ enum DownloadError: Error { case requestFailed(String) } +extension DownloadError: LocalizedError { + var errorDescription: String? { + switch self { + case .invalidURL: + return "Invalid URL." + case .requestFailed(let message): + return message + } + } +} + public final class NetworkManager { nonisolated(unsafe) public static let shared = NetworkManager() @@ -60,11 +71,28 @@ public final class NetworkManager { // which is read-only during Setup Assistant. Route through the shared // no-cache session so manifest and artifact bytes always reflect origin // truth (a stale cached payload would defeat the per-item hash check). - let task = Self.noCacheSession.dataTask(with: request) { data, _, error in + let task = Self.noCacheSession.dataTask(with: request) { data, response, error in if let error = error { completion(.failure(error)) return } + // A non-2xx response still carries a body — a 404 page, a CDN error + // document, an auth challenge. Writing that to the destination makes a + // broken URL look like a hash mismatch (or, for an item with no hash, + // hands the installer an HTML error page), so fail the download here + // and name the status and the URL. + if let http = response as? HTTPURLResponse, !(200...299).contains(http.statusCode) { + let detail: String + switch http.statusCode { + case 401, 403: + detail = "HTTP \(http.statusCode) (unauthorized — the origin is private and the request carried no valid Authorization header)" + default: + detail = "HTTP \(http.statusCode)" + } + Logger.error("Download failed: \(detail) for \(urlString)") + completion(.failure(DownloadError.requestFailed("\(detail) for \(urlString)"))) + return + } guard let data = data else { completion(.failure(DownloadError.requestFailed("No data received."))) return diff --git a/Sources/core/Utilities/ArchitectureSkip.swift b/Sources/core/Utilities/ArchitectureSkip.swift new file mode 100644 index 0000000..b5a3e7a --- /dev/null +++ b/Sources/core/Utilities/ArchitectureSkip.swift @@ -0,0 +1,52 @@ +// +// ArchitectureSkip.swift +// BootstrapMate +// +// Single interpretation of a manifest item's `skipIf` value. +// +// The InstallApplications sense is authoritative: `skipIf` names the +// architecture the item must NOT run on, so the item is skipped when the +// named architecture is the one we are running on. Two copies of this rule +// used to exist — one in the stage loops and one gating the download — and +// they disagreed, so an item carrying `skipIf` was never downloaded on the +// architecture it was written for. +// + +import Foundation + +public enum ArchitectureSkip { + + /// The architecture of the machine this run is on: "arm64" or "x86_64". + public static func currentArchitecture() -> String { + var systemInfo = utsname() + uname(&systemInfo) + let machineMirror = Mirror(reflecting: systemInfo.machine) + let chars = machineMirror.children.compactMap { $0.value as? Int8 } + .filter { $0 != 0 } + .map { Character(UnicodeScalar(UInt8($0))) } + let identifier = String(chars) + return identifier.contains("arm64") ? "arm64" : "x86_64" + } + + /// Returns true when `skipIf` names the architecture we are running on, + /// i.e. when the item should be skipped on this machine. + /// + /// - Parameters: + /// - skipIf: the manifest item's raw `skipIf` value. + /// - currentArch: the running architecture; defaults to this machine's. + public static func shouldSkip(_ skipIf: String, currentArch: String = currentArchitecture()) -> Bool { + let skipLower = skipIf.lowercased() + + // ARM-based skip conditions + if (skipLower.contains("arm") || skipLower.contains("apple_silicon")) && currentArch == "arm64" { + return true + } + + // Intel-based skip conditions + if (skipLower.contains("x86_64") || skipLower.contains("intel")) && currentArch == "x86_64" { + return true + } + + return false + } +} diff --git a/Tests/BootstrapMateCoreTests/BootstrapMateCoreTests.swift b/Tests/BootstrapMateCoreTests/BootstrapMateCoreTests.swift index 4f3f865..98780a8 100644 --- a/Tests/BootstrapMateCoreTests/BootstrapMateCoreTests.swift +++ b/Tests/BootstrapMateCoreTests/BootstrapMateCoreTests.swift @@ -580,3 +580,44 @@ struct SessionLogTests { #expect(Set(try fm.contentsOfDirectory(atPath: logs)) == ["2026-08-30", "2026-09-03"]) } } + +// MARK: - ArchitectureSkip Tests + +@Suite("ArchitectureSkip Tests") +struct ArchitectureSkipTests { + + @Test("An item is skipped on the architecture its skipIf names") + func skipsNamedArchitecture() { + #expect(ArchitectureSkip.shouldSkip("arm64", currentArch: "arm64") == true) + #expect(ArchitectureSkip.shouldSkip("apple_silicon", currentArch: "arm64") == true) + #expect(ArchitectureSkip.shouldSkip("x86_64", currentArch: "x86_64") == true) + #expect(ArchitectureSkip.shouldSkip("intel", currentArch: "x86_64") == true) + } + + @Test("An item runs on the other architecture") + func runsOnOtherArchitecture() { + #expect(ArchitectureSkip.shouldSkip("arm64", currentArch: "x86_64") == false) + #expect(ArchitectureSkip.shouldSkip("apple_silicon", currentArch: "x86_64") == false) + #expect(ArchitectureSkip.shouldSkip("x86_64", currentArch: "arm64") == false) + #expect(ArchitectureSkip.shouldSkip("intel", currentArch: "arm64") == false) + } + + @Test("Matching is case-insensitive") + func caseInsensitive() { + #expect(ArchitectureSkip.shouldSkip("ARM64", currentArch: "arm64") == true) + #expect(ArchitectureSkip.shouldSkip("Intel", currentArch: "x86_64") == true) + #expect(ArchitectureSkip.shouldSkip("Apple_Silicon", currentArch: "x86_64") == false) + } + + @Test("An unrecognized value never skips") + func unknownValueRuns() { + #expect(ArchitectureSkip.shouldSkip("", currentArch: "arm64") == false) + #expect(ArchitectureSkip.shouldSkip("ppc", currentArch: "arm64") == false) + #expect(ArchitectureSkip.shouldSkip("ppc", currentArch: "x86_64") == false) + } + + @Test("The current architecture is one of the two we support") + func currentArchitectureIsKnown() { + #expect(["arm64", "x86_64"].contains(ArchitectureSkip.currentArchitecture())) + } +}