diff --git a/abgui/Sources/abgui/AppModel.swift b/abgui/Sources/abgui/AppModel.swift index 589da24..f61ab9a 100644 --- a/abgui/Sources/abgui/AppModel.swift +++ b/abgui/Sources/abgui/AppModel.swift @@ -43,16 +43,14 @@ final class AppModel { var auditSince = "7d" var osReleases: [OSRelease] = [] + /// Routed through `run` like every other resource load, rather than driving the shared + /// spinner itself. Owning `isLoading`/`loadError` without the generation guard let this one + /// clear another screen's error and another screen's spinner: opening OS Releases mid-fetch + /// wiped the Devices error, and its own `defer` then dropped the Devices spinner while that + /// fetch was still running. It is also called from inside a device detail sheet, so its + /// failures were landing on whatever list happened to be behind that sheet. func loadOSReleases() async { - guard let client = makeClient() else { - loadError = "abctl was not found in the app bundle." - return - } - isLoading = true - loadError = nil - defer { isLoading = false } - do { osReleases = try await client.osReleases() } - catch { loadError = error.localizedDescription } + await run { self.osReleases = try await $0.osReleases() } } // Apps & Books (VPP) — a separate service; the content token is held in-memory only. @@ -74,6 +72,10 @@ final class AppModel { var verifyMode = "targeted" var needsSeed = false // workspace chosen but has no gitops/ tree yet → offer to initialize var isSeeding = false // `abctl seed` in flight + /// `abctl diff` in flight. Distinct from `isLoading`, which every resource list shares: a + /// Devices fetch must not put the Diff screen into its computing state, and finishing one + /// must not take the Diff screen out of it. + var isPlanning = false // Pre-flight verification of the local gitops/ profiles (`abctl validate --json`). Reads // files only — no tenant call, no credentials — so it can run before anything is synced. @@ -428,7 +430,12 @@ final class AppModel { /// Point at a GitOps workspace (the dir containing `gitops/`) and recompute drift. The path /// is remembered so the next launch reopens it (see `restoreWorkspace`). + /// Also clears the in-flight work flags: picking a workspace is the user starting over, and + /// it must never be possible to land in a state whose only exit is relaunching the app. func setWorkspace(_ url: URL) { + workGeneration += 1 // orphan whatever was running against the previous workspace + isSeeding = false + isPlanning = false repoRoot = url plan = nil applyResult = nil @@ -475,9 +482,14 @@ final class AppModel { guard let data = try? Data(contentsOf: entry.fileURL), let xml = String(data: data, encoding: .utf8) else { lastWriteError = "couldn't read the archived profile at \(entry.fileURL.lastPathComponent)." + lastWriteScope = .archive return false } - return await replaceConfiguration(id: entry.configName, xml: xml) + let ok = await runWrite(.archive) { + _ = try await $0.replaceConfiguration(id: entry.configName, xml: Data(xml.utf8)) + } + if ok { await loadConfigurations() } + return ok } /// Reconcile the tenant to the workspace git state. Returns true on a clean apply. @@ -492,6 +504,7 @@ final class AppModel { guard let client = makeClient(narrating: .apply) else { let message = "abctl was not found in the app bundle." lastWriteError = message + lastWriteScope = .apply syncFailure = SyncFailure(kind: .aborted, headline: message, details: "abgui runs the embedded CLI at abgui.app/Contents/Resources/abctl.") return false @@ -549,6 +562,7 @@ final class AppModel { stderr: run.stderr, transcript: applyProgressLog) syncFailure = failure lastWriteError = failure?.headline // the short form; the blob is never the message + lastWriteScope = failure == nil ? nil : .apply isWriting = false let outcome = failure.map { "failed — \($0.headline)" } ?? "succeeded: \(result.totalWrites) write(s), \(result.totalSkipped) skipped" @@ -562,6 +576,7 @@ final class AppModel { let failure = SyncFailure.from(error: error, transcript: applyProgressLog) syncFailure = failure lastWriteError = failure.headline + lastWriteScope = .apply // Only the HEADLINE goes into the transcript: abctl's stderr was already streamed // into it line by line by ProcessRunner, so appending the whole blob again was // duplicating the log the user was complaining about. @@ -622,9 +637,15 @@ final class AppModel { /// Load the saved connection contexts + the current one (for the settings picker/footer). func loadContexts() async { guard let client = makeClient() else { return } - if let list = try? await client.contextList() { + // A failed list used to leave `contexts` at its old value with no message — and on the + // first load that renders as "No saved connections yet" over a store that has them, + // inviting the user to re-enter credentials they already have. + do { + let list = try await client.contextList() contexts = list.contexts currentContext = list.current + } catch { + settingsError = "Couldn't read saved connections: \(error.localizedDescription)" } } @@ -699,10 +720,20 @@ final class AppModel { } /// Switch the current context (the credential store's active tenant) and reconnect. + /// + /// The failure is reported, and `context` only moves if the switch actually took. Swallowing + /// it left the app addressing a tenant abctl had never selected: every later command carried + /// `--context ` for a context the store had refused to make current, with no message + /// anywhere on screen to explain the results that followed. func useConnection(_ name: String) async { guard let client = makeClient() else { return } - try? await client.useContext(name) - context = name + settingsError = nil + do { + try await client.useContext(name) + context = name + } catch { + settingsError = "Couldn't switch to \(name): \(error.localizedDescription)" + } await loadContexts() await check() } @@ -711,8 +742,15 @@ final class AppModel { /// keys; a stale keys/.pem is harmless and overwritten on the next save.) func deleteConnection(_ name: String) async { guard let client = makeClient() else { return } - try? await client.deleteContext(name) - if context == name { context = "" } + settingsError = nil + do { + try await client.deleteContext(name) + if context == name { context = "" } + } catch { + // Previously silent: the row stayed in the list, nothing was said, and the only + // reading available was "the button does nothing". + settingsError = "Couldn't delete \(name): \(error.localizedDescription)" + } await loadContexts() await check() } @@ -746,7 +784,21 @@ final class AppModel { loadError = "abctl was not found in the app bundle." return } - isLoading = true + // Claim ownership of the shared plan state. `refreshPlan` cancels the previous run and + // starts this one, and the cancelled run STILL unwinds — its cleanup must not touch + // state that now belongs to a newer run. + // + // This is not a hypothetical race; the ordering is deterministic. `refreshPlan` returns + // on the main actor while the old run is suspended inside ProcessRunner, so the new run + // always sets `isLoading = true` first, and the old one's teardown then clears it while + // the new abctl child is still running. DiffView checks `isLoading` BEFORE the plan + // branch, so it fell through to the PREVIOUS plan: stale rows, stale "Checked" time, no + // Cancel button — and Apply enabled. Pressing it applied the new mode (which forces + // --prune) against a plan that never mentioned a single deletion. Flipping the + // source-of-truth switch mid-compute reached that in two clicks. + workGeneration += 1 + let generation = workGeneration + isPlanning = true loadError = nil // A new plan compute is a new context, so a write error from before it goes with it. // DiffView renders `lastWriteError` (a refused adopt is otherwise a dead click), and @@ -760,14 +812,22 @@ final class AppModel { resetProgressBuffers() // buffered tail belongs to the run being replaced, not this one progressLog = [] } - defer { isLoading = false } + // Only the CURRENT owner may clear the spinner. A superseded run leaves it alone — + // the run that replaced it is still working, and saying otherwise re-enables Apply + // over a plan nobody is computing. + defer { if generation == workGeneration { isPlanning = false } } if newRunLog { await beginRunLog(.diff, argv: previewArgv(AbctlClient.planArgs(gitSourceOfTruth: gitSourceOfTruth, refresh: refreshMode))) } var outcome = "plan computed" do { - plan = try await client.plan(gitSourceOfTruth: gitSourceOfTruth, refresh: refreshMode) + let computed = try await client.plan(gitSourceOfTruth: gitSourceOfTruth, refresh: refreshMode) + // A superseded run must not publish its result either: its plan was computed for + // the OLD mode, and showing it next to the new mode's switch is the same lie from + // the other direction. + guard generation == workGeneration else { return } + plan = computed lastCheckedAt = Date() // stamp every successful check, so a refresh confirms even when in sync } catch is CancellationError { // user cancelled — clear the in-flight state, no error shown @@ -775,19 +835,25 @@ final class AppModel { } catch { if Task.isCancelled { outcome = "cancelled" // a race: cancelled just after the process returned - } else { + } else if generation == workGeneration { // Publish the tail BEFORE diagnosing: the last lines abctl printed are exactly // what `SyncFailure` reads, and leaving them in the buffer would diagnose the // failure from a transcript missing its most relevant part. flushProgress() loadError = error.localizedDescription outcome = "failed — \(SyncFailure.from(error: error, transcript: progressLog).headline)" + } else { + outcome = "superseded by a newer plan" } } - flushProgress() // idempotent — covers the success and cancelled paths - // Only the call that OPENED a log closes it: the seed hand-off's diff writes into the - // seed's file and lets `seedWorkspace` stamp the single outcome both phases share. - if newRunLog { await finishRunLog(outcome) } + if generation == workGeneration { + flushProgress() // idempotent — covers the success and cancelled paths + } + // Only the call that OPENED a log closes it — and only while it still owns the run. + // A superseded run that closed the log would close its SUCCESSOR's (runLog is a single + // property), stamping the file the user is watching as "cancelled" and silently + // dropping every line the live run still had to write. + if newRunLog && generation == workGeneration { await finishRunLog(outcome) } } /// Seed the workspace as a cancellable task (so the Cancel button can stop it). @@ -804,6 +870,11 @@ final class AppModel { loadError = "Choose a workspace folder first." return false } + // Same ownership rule as loadPlan: `startSeed` cancels whatever ran before, and the + // cancelled run must not tear down the state — or close the run log — of the one that + // replaced it. + workGeneration += 1 + let generation = workGeneration isSeeding = true loadError = nil resetProgressBuffers() @@ -812,17 +883,20 @@ final class AppModel { do { _ = try await client.seed() } catch is CancellationError { + guard generation == workGeneration else { return false } isSeeding = false flushProgress() await finishRunLog("cancelled") return false } catch { + guard generation == workGeneration else { return false } isSeeding = false flushProgress() // the tail is what SyncFailure diagnoses from — publish it first loadError = "Couldn't initialize the workspace from the tenant: \(error.localizedDescription)" await finishRunLog("failed — \(SyncFailure.from(error: error, transcript: progressLog).headline)") return false } + guard generation == workGeneration else { return false } isSeeding = false flushProgress() needsSeed = false @@ -1001,29 +1075,29 @@ final class AppModel { // --yes, so the caller MUST show its own confirm first. Config writes refresh the list. func createConfiguration(name: String, xml: String) async -> Bool { - let ok = await runWrite { _ = try await $0.createConfiguration(name: name, xml: Data(xml.utf8)) } + let ok = await runWrite(.configuration) { _ = try await $0.createConfiguration(name: name, xml: Data(xml.utf8)) } if ok { await loadConfigurations() } return ok } func replaceConfiguration(id: String, xml: String) async -> Bool { - let ok = await runWrite { _ = try await $0.replaceConfiguration(id: id, xml: Data(xml.utf8)) } + let ok = await runWrite(.configuration) { _ = try await $0.replaceConfiguration(id: id, xml: Data(xml.utf8)) } if ok { await loadConfigurations() } return ok } func deleteConfiguration(id: String) async -> Bool { - let ok = await runWrite { _ = try await $0.deleteConfiguration(id: id) } + let ok = await runWrite(.configuration) { _ = try await $0.deleteConfiguration(id: id) } if ok { await loadConfigurations() } return ok } func attach(configID: String, blueprint: String) async -> Bool { - await runWrite { _ = self.announceTreeGap(try await $0.attach(configID: configID, blueprint: blueprint)) } + await runWrite(.membership) { _ = self.announceTreeGap(try await $0.attach(configID: configID, blueprint: blueprint)) } } func detach(configID: String, blueprint: String) async -> Bool { - await runWrite { _ = self.announceTreeGap(try await $0.detach(configID: configID, blueprint: blueprint)) } + await runWrite(.membership) { _ = self.announceTreeGap(try await $0.detach(configID: configID, blueprint: blueprint)) } } /// Surface a write that reached Apple Business but not gitops/. abctl exits 0 for this (the @@ -1046,9 +1120,10 @@ final class AppModel { func adoptMember(_ change: BlueprintChange) async -> Bool { guard let kind = change.memberKind, let name = change.config, !name.isEmpty else { lastWriteError = "This row doesn't name a blueprint member to adopt." + lastWriteScope = .adopt return false } - let ok = await runWrite { + let ok = await runWrite(.adopt) { _ = try await $0.adoptMember(kind: kind, name: name, blueprint: change.blueprint) } if ok { @@ -1101,7 +1176,8 @@ final class AppModel { let argv = CommandLineParser.tokenize(line) guard !argv.isEmpty else { return false } guard let client = makeClient() else { - lastWriteError = "abctl was not found in the app bundle." + consoleEntries.append(ConsoleEntry(argv: CommandFormatter.redact(argv), + failedToStart: "abctl was not found in the app bundle.")) return false } let trimmed = line.trimmingCharacters(in: .whitespaces) @@ -1132,20 +1208,47 @@ final class AppModel { func clearConsole() { consoleEntries = [] } + /// Which screen's write produced `lastWriteError`. + /// + /// The error itself is one property shared by every gated write in the app, and five + /// surfaces render it. Without an owner, a failed config delete on the Configurations + /// screen showed up as red text under the Diff plan, across the Archive table, and + /// pre-filled in the next sheet opened — each one implying a failure that screen had + /// nothing to do with. Tagging the writer lets each surface ask only for its own. + enum WriteScope: Equatable { + case configuration // create / replace / delete a profile + case membership // attach / detach + case adopt // record a live member in the manifest + case archive // restore an archived version + case assignment // device → MDM server + case apply // sync --apply + } + + private(set) var lastWriteScope: WriteScope? + + /// The write error IF it belongs to `scope`. Views call this instead of reading + /// `lastWriteError` directly, so an unrelated failure cannot surface on their screen. + func writeError(_ scope: WriteScope) -> String? { + lastWriteScope == scope ? lastWriteError : nil + } + /// Shared write wrapper: toggles isWriting, clears/sets lastWriteError, returns success. - private func runWrite(_ body: (AbctlClient) async throws -> Void) async -> Bool { + private func runWrite(_ scope: WriteScope, _ body: (AbctlClient) async throws -> Void) async -> Bool { guard let client = makeClient() else { lastWriteError = "abctl was not found in the app bundle." + lastWriteScope = scope return false } isWriting = true lastWriteError = nil + lastWriteScope = nil defer { isWriting = false } do { try await body(client) return true } catch { lastWriteError = error.localizedDescription + lastWriteScope = scope return false } } @@ -1154,6 +1257,16 @@ final class AppModel { /// (the dashboard's sequential pass can overlap a destination screen's own .task). private var loadGeneration = 0 + /// Ownership token for the two workTask-backed operations — computing the plan, and seeding. + /// SEPARATE from `loadGeneration` on purpose. + /// + /// Sharing one counter looked tidy and was a trap: `run()` bumps it for every resource list, + /// so navigating to Configurations while a seed was in flight invalidated the SEED's token. + /// Its completion guard then failed, `isSeeding` was never cleared, and the Diff screen sat + /// on "Initializing workspace from the tenant…" forever with Refresh and Verify disabled — + /// unrecoverable without relaunching. Two independent concerns need two independent tokens. + private var workGeneration = 0 + /// Shared load wrapper: toggles isLoading, clears/sets loadError, runs `body`. /// Cancellation (navigating away terminates the abctl child) is swallowed like /// loadPlan's — never shown as an error — and only the LATEST run may write the diff --git a/abgui/Sources/abgui/Backend/ProcessRunner.swift b/abgui/Sources/abgui/Backend/ProcessRunner.swift index 7c9ac33..bf3a3d7 100644 --- a/abgui/Sources/abgui/Backend/ProcessRunner.swift +++ b/abgui/Sources/abgui/Backend/ProcessRunner.swift @@ -31,12 +31,6 @@ actor ProcessRunner: AbctlRunner { try process.run() - // Write stdin (e.g. profile XML for `create -f -`) then close so abctl sees EOF. - if let stdin { - try? inPipe.fileHandleForWriting.write(contentsOf: stdin) - } - try? inPipe.fileHandleForWriting.close() - // If the caller's Task is cancelled (e.g. a Cancel button), terminate the child so it // doesn't linger; its pipes then close, the drains reach EOF, and we unwind below. return try await withTaskCancellationHandler { @@ -48,14 +42,37 @@ actor ProcessRunner: AbctlRunner { process.terminate() } + // Feed stdin (profile XML for `create -f -`) off the calling thread and INSIDE the + // watchdog's reach, then close so abctl sees EOF. + // + // It used to run before the cancellation handler and before the drains were + // started, so for the duration of the write there was no Cancel, no timeout, and + // nobody reading the child's output. A profile larger than the pipe buffer, sent to + // an abctl that narrates before consuming stdin, deadlocks both ends with no way + // out. The write error is captured rather than discarded, because a partially + // written profile is a truncated one, and sending that to Apple as a `create` is + // worse than failing. + async let written: Error? = Self.writeStdin(inPipe.fileHandleForWriting, data: stdin) + // Drain both pipes concurrently, THEN wait — reading first avoids the classic // 64 KB-pipe-buffer deadlock. stderr streams line-by-line to `onStderrLine`. async let out = Self.readToEnd(outPipe.fileHandleForReading) async let err = Self.streamToEnd(errPipe.fileHandleForReading, onLine: onStderrLine) let stdout = await out let stderr = await err + let stdinError = await written process.waitUntilExit() + // A failed stdin write means abctl received a TRUNCATED profile. It may well have + // sent that to Apple, so this is reported rather than swallowed — the exit code + // alone cannot distinguish "created what you meant" from "created half of it". + if let stdinError { + // `stderr` is still Data here — it is decoded once, below, for the result. + throw AbctlError.cli("failed to send the profile to abctl (it may have received only part of it): " + + stdinError.localizedDescription + "\n" + + String(decoding: stderr, as: UTF8.self)) + } + // The watchdog ran to completion iff it fired (timeout); if it was still sleeping // we cancel it and its sleep throws — telling success from timeout precisely. watchdog.cancel() @@ -79,6 +96,22 @@ actor ProcessRunner: AbctlRunner { } } + /// Write `data` to the child's stdin off the main thread and close the handle, returning the + /// error if the write failed. Closing happens either way: without EOF the child waits for + /// input that is never coming, which is a hang rather than an error. + private static func writeStdin(_ handle: FileHandle, data: Data?) async -> Error? { + await withCheckedContinuation { continuation in + DispatchQueue.global().async { + var failure: Error? + if let data, !data.isEmpty { + do { try handle.write(contentsOf: data) } catch { failure = error } + } + try? handle.close() + continuation.resume(returning: failure) + } + } + } + /// Read a pipe to EOF off the actor (and off the main thread). private static func readToEnd(_ handle: FileHandle) async -> Data { await withCheckedContinuation { continuation in diff --git a/abgui/Sources/abgui/Models/ApplyResult.swift b/abgui/Sources/abgui/Models/ApplyResult.swift index 8ba4876..d6db323 100644 --- a/abgui/Sources/abgui/Models/ApplyResult.swift +++ b/abgui/Sources/abgui/Models/ApplyResult.swift @@ -7,12 +7,69 @@ import Foundation struct ApplyResult: Decodable, Equatable { let configs: Phase let blueprints: Phase + /// abctl's own post-apply verdict, from the `verification` key of the receipt. + /// + /// This is the STRUCTURED answer to "did the tenant end up matching git", and it was being + /// thrown away: abgui reached the same conclusion by grepping stderr for the word FAILED, + /// against a hand-maintained list of abctl's literal narration strings. Rewording one Go + /// sentence silently downgraded the GUI's verdict to a generic "may not match git" — the + /// same contract-drift that misclassified every non-config plan row. Optional because an + /// older abctl, or a run that died before verification, does not emit it. + let verification: Verification? var totalWrites: Int { configs.writes + blueprints.writes } var totalErrors: Int { configs.errors + blueprints.errors } var totalSkipped: Int { configs.skipped + blueprints.skipped } var rows: [OutcomeRow] { configs.rows + blueprints.rows } + /// The post-apply read-back verdict (internal/cli/phase1.go `verificationReport`). + struct Verification: Decodable, Equatable { + let mode: String // targeted | full | none + let written: Int // configs this run pushed to Apple + let verified: Int // of those, shown to match git (always 0 for mode "none") + let mismatches: [Mismatch] + + /// True when abctl positively established that something did NOT land. Distinct from + /// "not everything was checked": `--verify=none` verifies nothing and is not a failure. + var hasMismatches: Bool { !mismatches.isEmpty } + + /// Writes abctl could not reach a verdict on — verified and mismatched are the two + /// things it DID decide, so anything left over was never established either way. + var unchecked: Int { max(0, written - verified - mismatches.count) } + + struct Mismatch: Decodable, Equatable { + let name: String + let detail: String + /// true → abctl READ the config and the bytes differ (the write did not land). + /// false → abctl could not compare it, which says nothing about the write. + /// abctl keeps these apart deliberately; collapsing them would report a network + /// failure as a lost write. + let observed: Bool + } + + enum CodingKeys: String, CodingKey { case mode, written, verified, mismatches } + init(from decoder: Decoder) throws { + let c = try decoder.container(keyedBy: CodingKeys.self) + mode = try c.decodeIfPresent(String.self, forKey: .mode) ?? "unknown" + written = try c.decodeIfPresent(Int.self, forKey: .written) ?? 0 + verified = try c.decodeIfPresent(Int.self, forKey: .verified) ?? 0 + mismatches = try c.decodeIfPresent([Mismatch].self, forKey: .mismatches) ?? [] + } + + /// The one-line verdict, in abgui's own words rather than scraped from abctl's. + var headline: String { + if hasMismatches { + let lost = mismatches.filter(\.observed).count + if lost > 0 { + return "\(lost) of \(written) written configuration(s) did not land on Apple Business." + } + return "\(mismatches.count) of \(written) written configuration(s) could not be checked." + } + if mode == "none" { return "Writes were not verified (--verify=none)." } + return "\(verified) written configuration(s) confirmed on Apple Business." + } + } + /// One reconcile phase (reconcile.Result / reconcile.BlueprintResult share this shape). struct Phase: Decodable, Equatable { let outcomes: [OutcomeRow] diff --git a/abgui/Sources/abgui/Models/ReadOnlyKind.swift b/abgui/Sources/abgui/Models/ReadOnlyKind.swift index ec12ff6..e5780e2 100644 --- a/abgui/Sources/abgui/Models/ReadOnlyKind.swift +++ b/abgui/Sources/abgui/Models/ReadOnlyKind.swift @@ -73,7 +73,10 @@ enum ReadOnlyKind: String, CaseIterable, Identifiable, Hashable { return [ ColumnSpec(header: "Serial") { $0.attr("serialNumber") ?? $0.id }, ColumnSpec(header: "Model") { $0.attr("deviceModel") ?? "—" }, - ColumnSpec(header: "OS") { $0.attr("osVersion") ?? "—" }, + // No "OS" column: `osVersion` is BUILT-IN-MDM posture, not an orgDevices + // attribute — every Go read of it comes off mdmDetails, and abctl's own + // orgDevices table prints serial/family/model only. It was permanently "—", + // in the table and in the CSV export. Enrolled Devices is where OS lives. ColumnSpec(header: "Family") { $0.attr("productFamily") ?? "—" }, ] case .mdmDevices: @@ -87,9 +90,9 @@ enum ReadOnlyKind: String, CaseIterable, Identifiable, Hashable { return [ ColumnSpec(header: "Name") { let n = [$0.attr("firstName"), $0.attr("lastName")].compactMap { $0 }.joined(separator: " ") - return n.isEmpty ? ($0.attr("managedAppleId") ?? $0.id) : n + return n.isEmpty ? ($0.attr("managedAppleAccount") ?? $0.id) : n }, - ColumnSpec(header: "Managed Apple ID") { $0.attr("managedAppleId") ?? $0.attr("email") ?? "—" }, + ColumnSpec(header: "Managed Apple ID") { $0.attr("managedAppleAccount") ?? $0.attr("email") ?? "—" }, ColumnSpec(header: "Roles") { let r = $0.roleNames(); return r.isEmpty ? "—" : r }, ColumnSpec(header: "Status") { $0.attr("status") ?? "—" }, ] diff --git a/abgui/Sources/abgui/Views/ApplySheet.swift b/abgui/Sources/abgui/Views/ApplySheet.swift index 9948c1f..6cd6b18 100644 --- a/abgui/Sources/abgui/Views/ApplySheet.swift +++ b/abgui/Sources/abgui/Views/ApplySheet.swift @@ -146,7 +146,13 @@ struct ApplySheet: View { // Either signal is enough to withhold the green verdict: the phase counters and the // per-item rows are two different numbers out of abctl, and the honest reading of a // disagreement between them is "something failed", never "all clear". - return result.totalErrors > 0 || model.syncFailure != nil ? .partial(result) : .succeeded(result) + // abctl's own STRUCTURED verdict is consulted first: a run can report every item + // `done` and still have failed, because Apple answers 2xx to a write it then + // declines to store. `verification.mismatches` is where abctl says so, and it is + // the reason the green verdict is not simply "no errors". + let notPersisted = result.verification?.mismatches.contains { $0.observed } ?? false + return result.totalErrors > 0 || model.syncFailure != nil || notPersisted + ? .partial(result) : .succeeded(result) } if model.syncFailure != nil { return .failed } // `lastWriteError` is SHARED with every other gated write in the app (a failed config @@ -154,7 +160,7 @@ struct ApplySheet: View { // run something — `apply()` clears the progress log and writes its first line before any // of this can be true. Without that guard, opening Apply after an unrelated write failure // would greet the user with "Sync FAILED" for a sync that never happened. - if !model.applyProgressLog.isEmpty, model.lastWriteError != nil { return .failed } + if !model.applyProgressLog.isEmpty, model.writeError(.apply) != nil { return .failed } return nil } @@ -528,6 +534,15 @@ struct ApplySheet: View { VStack(alignment: .leading, spacing: 6) { Text("\(result.totalWrites) write(s) - \(result.totalErrors) error(s) - \(result.totalSkipped) skipped") .foregroundStyle(result.totalErrors > 0 ? .red : .green) + if let verification = result.verification { + // Read back from abctl's structured verdict rather than its prose. A write + // Apple acknowledged and dropped shows up HERE and nowhere in the counters + // above, which is exactly why it gets its own line. + Text(verification.headline) + .font(.caption) + .foregroundStyle(verification.mismatches.contains { $0.observed } ? .red : .secondary) + .fixedSize(horizontal: false, vertical: true) + } if !result.rows.isEmpty { Divider() ScrollView { diff --git a/abgui/Sources/abgui/Views/ArchiveView.swift b/abgui/Sources/abgui/Views/ArchiveView.swift index 2135bf3..ae95926 100644 --- a/abgui/Sources/abgui/Views/ArchiveView.swift +++ b/abgui/Sources/abgui/Views/ArchiveView.swift @@ -52,19 +52,30 @@ struct ArchiveView: View { Text("Choose a workspace in Diff / Drift to browse its archive.") } } else { - Table(model.archiveEntries, selection: $selection) { - TableColumn("Configuration") { Text($0.configName) } - TableColumn("Archived") { Text($0.archivedAt) } - TableColumn("Reason") { Text($0.reason) } - } - .overlay { - if model.archiveEntries.isEmpty { - ContentUnavailableView("No archived versions", systemImage: "clock.arrow.circlepath", - description: Text("abctl archives a live profile before each overwrite or delete.")) + // A VStack, not two bare siblings. This branch emitted the Table and the error Text + // as an unwrapped pair into a slot that takes ONE view, so the error composited over + // the middle of the table instead of sitting under it — the same class of layout + // fault as the safe-area insets, and it only ever showed up when a write had failed. + VStack(spacing: 0) { + Table(model.archiveEntries, selection: $selection) { + TableColumn("Configuration") { Text($0.configName) } + TableColumn("Archived") { Text($0.archivedAt) } + TableColumn("Reason") { Text($0.reason) } + } + .overlay { + if model.archiveEntries.isEmpty { + ContentUnavailableView("No archived versions", systemImage: "clock.arrow.circlepath", + description: Text("abctl archives a live profile before each overwrite or delete.")) + } + } + if let error = model.writeError(.archive) { + Text(error) + .foregroundStyle(.red) + .font(.caption) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + .padding([.horizontal, .vertical], 8) } - } - if let error = model.lastWriteError { - Text(error).foregroundStyle(.red).font(.caption).padding(.horizontal) } } } diff --git a/abgui/Sources/abgui/Views/ConfigEditorView.swift b/abgui/Sources/abgui/Views/ConfigEditorView.swift index 049c1b3..63e99e0 100644 --- a/abgui/Sources/abgui/Views/ConfigEditorView.swift +++ b/abgui/Sources/abgui/Views/ConfigEditorView.swift @@ -41,7 +41,7 @@ struct ConfigEditorView: View { editor - if let error = model.lastWriteError { + if let error = model.writeError(.configuration) { Text(error).foregroundStyle(.red).font(.caption) .frame(maxWidth: .infinity, alignment: .leading) .padding([.horizontal, .bottom]) diff --git a/abgui/Sources/abgui/Views/ConsoleView.swift b/abgui/Sources/abgui/Views/ConsoleView.swift index 0588e96..774aa5f 100644 --- a/abgui/Sources/abgui/Views/ConsoleView.swift +++ b/abgui/Sources/abgui/Views/ConsoleView.swift @@ -132,9 +132,6 @@ struct ConsoleView: View { .foregroundStyle(.secondary) .fixedSize(horizontal: false, vertical: true) } - if let error = model.lastWriteError, model.consoleEntries.isEmpty { - Text(error).font(.caption).foregroundStyle(.red) - } } .padding() .background(.quaternary.opacity(0.35)) diff --git a/abgui/Sources/abgui/Views/DiffView.swift b/abgui/Sources/abgui/Views/DiffView.swift index e3a0979..960efbf 100644 --- a/abgui/Sources/abgui/Views/DiffView.swift +++ b/abgui/Sources/abgui/Views/DiffView.swift @@ -63,7 +63,7 @@ struct DiffView: View { .disabled((model.plan?.actionableChangeCount ?? 0) == 0) Button { model.refreshPlan() } label: { Label("Refresh", systemImage: "arrow.clockwise") } .toolbarLabel("Recompute the plan: re-read gitops/ and re-fetch the live tenant.") - .disabled(model.isLoading || model.isSeeding) + .disabled(model.isPlanning || model.isSeeding) } Button { showWorkspacePicker = true } label: { Label("Workspace", systemImage: "folder") } .toolbarLabel("Choose the folder that contains your gitops/ tree. Every command on this screen runs there.") @@ -114,8 +114,10 @@ struct DiffView: View { } } else if model.isSeeding { workingView("Initializing workspace from the tenant...") - } else if model.isLoading { - // Check isLoading BEFORE the plan branch, so a refresh from an already-computed + } else if model.isPlanning { + // `isPlanning`, not the shared `isLoading`: a Devices or Configurations fetch must + // not put THIS screen into its computing state, and finishing one must not take it + // out of it. Checked BEFORE the plan branch, so a refresh from an already-computed // state visibly shows progress instead of silently redisplaying the old result. workingView("Computing plan...") } else if model.needsSeed { @@ -229,7 +231,7 @@ struct DiffView: View { // nothing rendering it, a row-button click that abctl rejected (unmanaged // collection, no manifest for the blueprint, member not actually attached) // looked like a click that did nothing at all. - if let error = model.lastWriteError { + if let error = model.writeError(.adopt) { Label(error, systemImage: "exclamationmark.triangle") .font(.caption) .foregroundStyle(.red) @@ -350,7 +352,7 @@ private struct PlanDetailRow: View { .buttonStyle(.bordered) .controlSize(.small) .help(adopt.help) - .disabled(model.isWriting || model.isLoading) + .disabled(model.isWriting || model.isPlanning) } } .padding(.vertical, 8) diff --git a/abgui/Sources/abgui/Views/MembershipSheet.swift b/abgui/Sources/abgui/Views/MembershipSheet.swift index 58e6ee1..85f4ae8 100644 --- a/abgui/Sources/abgui/Views/MembershipSheet.swift +++ b/abgui/Sources/abgui/Views/MembershipSheet.swift @@ -23,7 +23,7 @@ struct MembershipSheet: View { } } - if let error = model.lastWriteError { + if let error = model.writeError(.membership) { Text(error).foregroundStyle(.red).font(.caption) } diff --git a/abgui/Tests/abguiTests/ContractTests.swift b/abgui/Tests/abguiTests/ContractTests.swift index f33293a..7ed02a4 100644 --- a/abgui/Tests/abguiTests/ContractTests.swift +++ b/abgui/Tests/abguiTests/ContractTests.swift @@ -237,16 +237,32 @@ final class ContractTests: XCTestCase { XCTAssertEqual(recordedStdin, Data("".utf8)) } + /// The attribute is `managedAppleAccount`. This fixture used to say `managedAppleId`, which + /// Apple does not emit — every Go read of it is `managedAppleAccount` (ab/client.go:453,475, + /// ab/detail.go:165, cli/inspect.go:304, cli/deploy.go:189) and `UserDetailSheet` had it + /// right. So the column silently fell through to `email` for every user, and this test + /// passed by asserting against the same invented key the column read. A fixture that + /// invents its input can only ever confirm the bug it was written beside. func testUserRolesDecodeAndColumns() throws { - let json = #"{"type":"users","id":"u1","attributes":{"firstName":"Ada","lastName":"Lovelace","managedAppleId":"ada@x.appleid.com","status":"ACTIVE","roles":[{"role":"Administrator","organizationalUnit":"HQ"},{"role":"Manager"}]}}"# + let json = #"{"type":"users","id":"u1","attributes":{"firstName":"Ada","lastName":"Lovelace","managedAppleAccount":"ada@x.appleid.com","email":"ada@corp.example","status":"ACTIVE","roles":[{"role":"Administrator","organizationalUnit":"HQ"},{"role":"Manager"}]}}"# let user = try JSONDecoder().decode(Resource.self, from: Data(json.utf8)) XCTAssertEqual(user.roleNames(), "Administrator, Manager") let columns = ReadOnlyKind.users.columns XCTAssertEqual(columns.first { $0.header == "Name" }?.value(user), "Ada Lovelace") XCTAssertEqual(columns.first { $0.header == "Roles" }?.value(user), "Administrator, Manager") + // The managed account WINS over the corporate email — that is the distinction the + // column's header promises, and reading `email` first would quietly break it again. XCTAssertEqual(columns.first { $0.header == "Managed Apple ID" }?.value(user), "ada@x.appleid.com") } + /// A user with no managed account still shows something useful rather than an em dash. + func testManagedAppleIDColumnFallsBackToEmail() throws { + let json = #"{"type":"users","id":"u2","attributes":{"firstName":"Grace","lastName":"Hopper","email":"grace@corp.example"}}"# + let user = try JSONDecoder().decode(Resource.self, from: Data(json.utf8)) + XCTAssertEqual(ReadOnlyKind.users.columns.first { $0.header == "Managed Apple ID" }?.value(user), + "grace@corp.example") + } + func testPackagesUsesGetPackages() async throws { actor Recorder { var args: [String] = [] @@ -1506,3 +1522,70 @@ final class CommandLineParserTests: XCTestCase { XCTAssertFalse(CommandLineParser.isUnapprovedWrite([])) } } + +// MARK: - the structured post-apply verdict (replacing the stderr grep) + +final class ApplyVerificationTests: XCTestCase { + private func decode(_ json: String) throws -> ApplyResult { + try JSONDecoder().decode(ApplyResult.self, from: Data(json.utf8)) + } + + private let emptyPhases = #""configs":{"outcomes":[],"writes":1,"errors":0,"skipped":0},"blueprints":{"outcomes":[],"writes":0,"errors":0,"skipped":0}"# + + /// abctl publishes its verdict as DATA (`internal/cli/phase1.go` verificationReport). abgui + /// used to re-derive it by grepping stderr for the word FAILED against a hand-maintained + /// list of abctl's narration strings — so rewording one Go sentence silently downgraded the + /// GUI's verdict. Decoding the key is what makes that impossible. + func testDecodesTheVerificationVerdict() throws { + let result = try decode(""" + {\(emptyPhases), + "verification":{"mode":"targeted","written":3,"verified":2, + "mismatches":[{"name":"WiFi.mobileconfig","detail":"still differs","observed":true}]}} + """) + let verification = try XCTUnwrap(result.verification) + XCTAssertEqual(verification.mode, "targeted") + XCTAssertEqual(verification.written, 3) + XCTAssertEqual(verification.verified, 2) + XCTAssertTrue(verification.hasMismatches) + XCTAssertTrue(verification.headline.contains("did not land")) + } + + /// A write Apple ACKNOWLEDGED and dropped is the whole reason this exists: every counter + /// reads clean and the run still failed. `observed` is what separates that from a write + /// abctl merely could not check, and collapsing the two would report a network blip as data + /// loss — abctl keeps them apart deliberately. + func testObservedSeparatesADroppedWriteFromAnUncheckedOne() throws { + let dropped = try decode(""" + {\(emptyPhases), + "verification":{"mode":"targeted","written":1,"verified":0, + "mismatches":[{"name":"A","detail":"stored profile still differs","observed":true}]}} + """) + XCTAssertTrue(dropped.verification?.mismatches.first?.observed == true) + XCTAssertTrue(dropped.verification?.headline.contains("did not land") == true) + + let unchecked = try decode(""" + {\(emptyPhases), + "verification":{"mode":"targeted","written":1,"verified":0, + "mismatches":[{"name":"A","detail":"read-back failed","observed":false}]}} + """) + XCTAssertFalse(unchecked.verification?.mismatches.first?.observed == true) + XCTAssertTrue(unchecked.verification?.headline.contains("could not be checked") == true) + } + + /// `--verify=none` checked nothing; that is not a failure and must not read as one. + func testVerifyNoneIsNotAFailure() throws { + let result = try decode(""" + {\(emptyPhases),"verification":{"mode":"none","written":2,"verified":0,"mismatches":[]}} + """) + XCTAssertFalse(result.verification?.hasMismatches == true) + XCTAssertTrue(result.verification?.headline.contains("not verified") == true) + } + + /// An older abctl — or a run that died before verification — emits no key at all. That must + /// decode, not throw, or the GUI loses the per-item receipt as well as the verdict. + func testAbsentVerificationDecodesToNil() throws { + let result = try decode("{\(emptyPhases)}") + XCTAssertNil(result.verification) + XCTAssertEqual(result.totalWrites, 1) + } +} diff --git a/internal/cli/apply.go b/internal/cli/apply.go index c8c0d4b..95bb15b 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -12,7 +12,6 @@ import ( "gopkg.in/yaml.v3" "github.com/GigaionLLC/abcli/internal/hash" - "github.com/GigaionLLC/abcli/internal/state" ) // applyYes is the shared --yes for `apply -f` / `delete -f`. @@ -158,25 +157,36 @@ func applyOneSpec(i *imp, s Spec, opts applyOpts) error { } // upsertConfig creates the config if absent in ABM, else archives + replaces it. +// +// The lookup is metadata-only, and profile XML is fetched for the ONE config being +// replaced (it is what gets archived). Asking Apple for every profile in the tenant to +// decide whether one name exists is the fan-out that made `adopt` time out — and here it +// was paid once per document in a bulk file. func upsertConfig(i *imp, name string, content []byte, platforms []string) error { - live, err := i.c.FetchCustomSettings() + index, err := i.configIndex() if err != nil { return err } var existing *struct { id, xml, updated string } - for _, l := range live { - if l.Name == name { - existing = &struct{ id, xml, updated string }{l.ID, l.XML, l.Updated} - break + for _, l := range index { + if l.Name != name { + continue + } + detail, derr := i.c.FetchCustomSettingDetail(l.ID) + if derr != nil { + return fmt.Errorf("reading configuration %q before replacing it: %w", name, derr) } + existing = &struct{ id, xml, updated string }{l.ID, detail.XML, detail.Updated} + break } if existing == nil { id, updated, err := i.c.CreateConfiguration(name, string(content), platforms) if err != nil { return err } + i.noteCreatedConfig(id, name) // a later doc in this run can now resolve it return i.recordConfig(name, id, content, updated) } if _, err := i.archiver().Archive(name, "replaced", []byte(existing.xml), map[string]string{ @@ -191,12 +201,12 @@ func upsertConfig(i *imp, name string, content []byte, platforms []string) error return i.recordConfig(name, existing.id, content, updated) } -func (i *imp) recordConfig(name, id string, content []byte, updated string) error { - if err := i.tree.WriteConfig(name, content); err != nil { - return err - } - i.base.Configs[name] = state.Entry{ABMID: id, Hash: hash.Raw(content), UpdatedDateTime: updated} - return i.base.Save(i.tree.StateFile) +// recordConfig confirms the write actually landed, then records the baseline from what +// Apple STORED. See imp.confirmStored: a 2xx is not proof, and a baseline written from the +// bytes we sent makes a dropped write invisible to every later sync. +func (i *imp) recordConfig(name, id string, content []byte, _ string) error { + stored, verdict := i.confirmStored(id, content) + return i.recordWrite(name, id, content, stored, verdict, false) } // applyBlueprintSpec attaches every config listed in the spec that isn't already @@ -207,8 +217,9 @@ func applyBlueprintSpec(i *imp, s Spec) error { return err } // Metadata only — this path resolves names to ids and rewrites the manifest from - // live ids; it never reads a profile. See liveConfigIndex. - live, err := liveConfigIndex(i.c) + // live ids; it never reads a profile. Cached for the whole command: a bulk file used to + // pay one full tenant list per document. + live, err := i.configIndex() if err != nil { return err } @@ -216,7 +227,13 @@ func applyBlueprintSpec(i *imp, s Spec) error { for _, l := range live { idByName[l.Name] = l.ID } - attached, _ := i.c.BlueprintRelationship(bp.ID, "configurations") + // NOT swallowed: an unreadable membership list is indistinguishable from an EMPTY one, + // which would re-POST every config in the spec — spending rate limit to reach a state + // we could not confirm we were not already in. + attached, err := i.c.BlueprintRelationship(bp.ID, "configurations") + if err != nil { + return fmt.Errorf("reading configuration membership of blueprint %q: %w", s.Metadata.Name, err) + } have := map[string]bool{} for _, r := range attached { have[r.ID] = true @@ -240,8 +257,13 @@ func applyBlueprintSpec(i *imp, s Spec) error { } // Mirror the ABM membership into the git manifest (full live set) so the tree // stays == live and a later `sync --prune` never reverts this attach. - if err := i.syncBlueprintManifest(bp.AttrStr("name"), bp.ID, live); err != nil { - fmt.Fprintf(os.Stderr, " warning: attached on ABM but local manifest update failed: %v\n", err) + if err := i.syncBlueprintManifest(bp.AttrStr("name"), bp.ID, live, ""); err != nil { + // A warning here left the command exiting 0 with the tenant attached and git + // untouched — drift manufactured by a "successful" run, and precisely the defect + // `attach`/`detach` were fixed for. Fail instead; the attach has already landed + // and is reported above, so re-running after fixing the tree is safe. + return fmt.Errorf("attached on ABM but the local manifest update failed (gitops/ and the tenant now "+ + "disagree; fix the tree and re-run, or `abctl seed` to adopt): %w", err) } fmt.Fprintf(os.Stderr, " ✓ Blueprint/%s: %d config(s) attached\n", s.Metadata.Name, attachedN) return nil diff --git a/internal/cli/command_behavior_test.go b/internal/cli/command_behavior_test.go new file mode 100644 index 0000000..dabc616 --- /dev/null +++ b/internal/cli/command_behavior_test.go @@ -0,0 +1,226 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/GigaionLLC/abcli/internal/gitops" + "github.com/GigaionLLC/abcli/internal/hash" + "github.com/GigaionLLC/abcli/internal/state" +) + +// End-to-end tests for whole commands against the fake tenant (faketenant_test.go). +// +// Each of these locks a defect that reached a release. They need no credentials and no network, +// which is the point: every one of these bugs was reachable by running the command once. + +const testProfileV1 = ` + + PayloadTypeConfiguration + PayloadVersion1 + PayloadIdentifiercom.example.wifi + PayloadUUID1A2B3C4D-0000-0000-0000-000000000001 + PayloadContent +` + +const testProfileV1Edited = ` + + PayloadTypeConfiguration + PayloadVersion1 + PayloadIdentifiercom.example.wifi + PayloadUUID1A2B3C4D-0000-0000-0000-000000000002 + PayloadContent +` + +func writeTempProfile(t *testing.T, dir, name, body string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +func loadBaseline(t *testing.T, root string) *state.State { + t.Helper() + tree := gitops.NewTree(root) + base, err := state.Load(tree.StateFile) + if err != nil { + t.Fatalf("reading baseline: %v", err) + } + return base +} + +// A `replace` whose bytes Apple silently drops must FAIL and must not record a baseline that +// claims agreement. +// +// This is the incident the whole read-back exists for. The old code wrote +// `Hash: hash.Raw(content)` — the bytes it SENT — so the baseline asserted a state Apple was +// not in. The next `--refresh=smart` run then matched id and timestamp, reused that hash, +// never fetched the real XML, and reported "in sync" forever while Apple held the old profile. +// A poisoned entry is self-sustaining: nothing re-reads it. +func TestReplaceFailsWhenAppleDropsTheWrite(t *testing.T) { + tenant := newFakeAPI(t) + tenant.addConfig("cfg-1", "wifi.mobileconfig", testProfileV1) + root := tenant.use(t) + tenant.dropWrites = true // 2xx, stored bytes unchanged + + file := writeTempProfile(t, t.TempDir(), "new.mobileconfig", testProfileV1Edited) + err := runReplaceConfig("wifi.mobileconfig", writeFlags{file: file, yes: true}) + if err == nil { + t.Fatal("a write Apple did not persist must fail, not report success") + } + if !strings.Contains(err.Error(), "did not persist") { + t.Errorf("error should name the accept-and-drop case, got: %v", err) + } + + base := loadBaseline(t, root) + entry, ok := base.Configs["wifi.mobileconfig"] + if !ok { + return // no baseline at all is also acceptable — nothing false was recorded + } + // If a baseline WAS written it must describe Apple's reality (the old bytes), never the + // bytes we sent — that difference is what makes the next diff show the drift. + if entry.Hash == hashOf(testProfileV1Edited) { + t.Error("baseline recorded the bytes SENT; the next smart refresh would report 'in sync' forever") + } +} + +// The happy path still records a baseline, from the read-back. +func TestReplaceRecordsBaselineFromTheReadBack(t *testing.T) { + tenant := newFakeAPI(t) + tenant.addConfig("cfg-1", "wifi.mobileconfig", testProfileV1) + root := tenant.use(t) + + file := writeTempProfile(t, t.TempDir(), "new.mobileconfig", testProfileV1Edited) + if err := runReplaceConfig("wifi.mobileconfig", writeFlags{file: file, yes: true}); err != nil { + t.Fatalf("replace: %v", err) + } + entry, ok := loadBaseline(t, root).Configs["wifi.mobileconfig"] + if !ok { + t.Fatal("a confirmed write must record a baseline") + } + if entry.Hash != hashOf(testProfileV1Edited) { + t.Errorf("baseline hash = %q, want the stored (== sent) profile's hash", entry.Hash) + } + if entry.UpdatedDateTime != "2026-02-02T00:00:00Z" { + t.Errorf("baseline timestamp = %q, want Apple's stored one", entry.UpdatedDateTime) + } +} + +// Resolving ONE config by name must not download every profile in the tenant. +// +// This is the `adopt` trap generalised: `FetchCustomSettings` asks for `customSettingsValues` +// on the whole tenant. On a real tenant that outran abgui's command budget and the command +// silently did nothing. Counting requests is the only way this class of bug is a test failure +// rather than a bug report. +func TestReplaceDoesNotDownloadEveryProfileInTheTenant(t *testing.T) { + tenant := newFakeAPI(t) + for _, n := range []string{"a", "b", "c", "d", "e", "f", "g", "h"} { + tenant.addConfig("cfg-"+n, n+".mobileconfig", testProfileV1) + } + tenant.use(t) + + file := writeTempProfile(t, t.TempDir(), "new.mobileconfig", testProfileV1Edited) + if err := runReplaceConfig("a.mobileconfig", writeFlags{file: file, yes: true}); err != nil { + t.Fatalf("replace: %v", err) + } + + if n := tenant.countMatching("GET-with-xml"); n != 0 { + t.Errorf("replace requested tenant-wide profile XML %d time(s); it needs one config's bytes, not everyone's", n) + } + // One detail GET to archive the old bytes, one to confirm the write. Never one per config. + if n := tenant.countMatching("GET /v1/configurations/"); n > 3 { + t.Errorf("replace made %d per-config GETs; expected at most 3 (archive + read-back)", n) + } +} + +// `status config` reads nothing but names and ids, so it must not pay for profile bytes either. +func TestStatusConfigDoesNotDownloadProfiles(t *testing.T) { + tenant := newFakeAPI(t) + for _, n := range []string{"a", "b", "c"} { + tenant.addConfig("cfg-"+n, n+".mobileconfig", testProfileV1) + } + tenant.use(t) + + if err := runStatusConfig("a.mobileconfig", true); err != nil { + t.Fatalf("status config: %v", err) + } + if n := tenant.countMatching("GET-with-xml"); n != 0 { + t.Errorf("status config requested profile XML %d time(s); it only reports coverage", n) + } +} + +// A profile Apple is known to drop must be refused BEFORE the write, by the same inspector +// `abctl validate` uses. The write path used to check three substrings and a size cap. +func TestCreateRefusesAProfileAppleWouldSilentlyDrop(t *testing.T) { + tenant := newFakeAPI(t) + tenant.use(t) + + bad := strings.Replace(testProfileV1, "PayloadVersion1", + "PayloadVersion2", 1) + file := writeTempProfile(t, t.TempDir(), "bad.mobileconfig", bad) + + err := runCreateConfig("bad", writeFlags{file: file, yes: true}) + if err == nil { + t.Fatal("a top-level PayloadVersion of 2 must be refused before it reaches Apple") + } + if !strings.Contains(err.Error(), "payload-version") { + t.Errorf("error should name the rule, got: %v", err) + } + if n := tenant.count("POST /v1/configurations"); n != 0 { + t.Errorf("the profile reached Apple %d time(s) despite failing validation", n) + } +} + +// `--refresh=metadata-only` can never populate profile XML, which every write needs. Refusing it +// up front beats failing item by item and re-planning the identical work on the next run. +func TestApplyRefusesMetadataOnlyRefresh(t *testing.T) { + err := runSync(syncFlags{apply: true, refresh: refreshMetadata, verify: verifyTargeted, yes: true}) + if err == nil { + t.Fatal("--apply with --refresh=metadata-only must be refused") + } + if !strings.Contains(err.Error(), "metadata-only") { + t.Errorf("error should name the mode, got: %v", err) + } +} + +// hashOf mirrors the raw-content hash the reconciler and the baseline both use. +func hashOf(s string) string { return hash.Raw([]byte(s)) } + +// A dry run must not fetch a profile per live-only config. +// +// This is the regression the willWrite gate exists to prevent, and it was inert on the first +// attempt: an unconditional `needDetail := l.Hash == ""` ran first, and on an unseeded workspace +// nothing has a cached hash, so every live config was fetched anyway. That is the exact case the +// optimization was for — hence a test rather than a comment. +func TestDiffOnAnUnseededWorkspaceDoesNotFetchEveryProfile(t *testing.T) { + tenant := newFakeAPI(t) + for _, n := range []string{"a", "b", "c", "d", "e"} { + tenant.addConfig("cfg-"+n, n+".mobileconfig", testProfileV1) + } + tenant.use(t) // empty workspace: no lib/, no baseline + + if _, err := loadPlan(false, refreshSmart, false); err != nil { + t.Fatalf("loadPlan: %v", err) + } + if n := tenant.countMatching("GET /v1/configurations/"); n != 0 { + t.Errorf("a dry-run plan fetched %d per-config profile(s); it compares nothing it keeps", n) + } +} + +// The same run WITH writes still gets the bytes it needs to pull or archive. +func TestApplyPlanStillFetchesProfilesItWillWrite(t *testing.T) { + tenant := newFakeAPI(t) + tenant.addConfig("cfg-a", "a.mobileconfig", testProfileV1) + tenant.use(t) + + if _, err := loadPlan(false, refreshSmart, true); err != nil { + t.Fatalf("loadPlan: %v", err) + } + if n := tenant.countMatching("GET /v1/configurations/"); n == 0 { + t.Error("a run that will pull must fetch the bytes it is going to write into git") + } +} diff --git a/internal/cli/deploy.go b/internal/cli/deploy.go index ffbb583..9c4999c 100644 --- a/internal/cli/deploy.go +++ b/internal/cli/deploy.go @@ -144,7 +144,11 @@ func runMembership(verb, configArg, blueprintArg string, yes, noWriteTree, jsonO if !noWriteTree { // Rewrite the manifest to the FULL post-write live membership (not a delta), // so the tree can never omit members that `sync --prune` would then detach. - if err := i.syncBlueprintManifest(bpName, bp.ID, live); err != nil { + detached := "" + if verb == "detach" { + detached = lc.Name + } + if err := i.syncBlueprintManifest(bpName, bp.ID, live, detached); err != nil { treeUpdated, treeErr = false, err.Error() fmt.Fprintf(os.Stderr, "warning: membership changed on ABM but the local manifest update failed: %v\n", err) fmt.Fprintf(os.Stderr, "warning: gitops/blueprints/ still disagrees with ABM — run `abctl adopt config %q --blueprint %q` from the workspace to record it.\n", lc.Name, bpName) @@ -468,7 +472,15 @@ func runStatusConfig(nameOrID string, asJSON bool) error { var carriers []cov totalDevices := 0 for _, bp := range bps { - links, _ := c.BlueprintRelationship(bp.ID, "configurations") + // Propagate relationship errors. A swallowed 429 or 403 here renders as + // "attached to 0 blueprint(s), targeting 0 device(s)" and exits 0 — an + // affirmatively FALSE coverage statement, and this loop is one call per + // blueprint so a rate-limited tenant is the likely case, not an edge one. + // (`status device` already learned this; see inspect.go.) + links, err := c.BlueprintRelationship(bp.ID, "configurations") + if err != nil { + return fmt.Errorf("reading configuration membership of blueprint %q: %w", bp.AttrStr("name"), err) + } on := false for _, l := range links { if l.ID == lc.ID { @@ -479,7 +491,10 @@ func runStatusConfig(nameOrID string, asJSON bool) error { if !on { continue } - devs, _ := c.BlueprintRelationship(bp.ID, "orgDevices") + devs, err := c.BlueprintRelationship(bp.ID, "orgDevices") + if err != nil { + return fmt.Errorf("reading device membership of blueprint %q: %w", bp.AttrStr("name"), err) + } carriers = append(carriers, cov{Blueprint: bp.AttrStr("name"), Devices: len(devs)}) totalDevices += len(devs) } diff --git a/internal/cli/fakeapi_test.go b/internal/cli/fakeapi_test.go new file mode 100644 index 0000000..7d3187b --- /dev/null +++ b/internal/cli/fakeapi_test.go @@ -0,0 +1,256 @@ +package cli + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "encoding/json" + "encoding/pem" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// A FAKE TENANT for driving whole commands. +// +// Until now `internal/cli` could only test pure helpers, because every command builds a client +// and a client needs credentials. That gap is why the two worst defects of this project's life +// shipped: a write path that recorded the baseline from bytes it SENT rather than bytes Apple +// stored, and a name→id lookup that downloaded every profile in the tenant and blew its command +// budget. Neither is exotic; both are obvious the moment anything actually runs the command. +// +// No live credentials are needed. `config.build` reads AB_TOKEN_URL and AB_API_BASE from the +// process environment, and a locally generated EC key mints a real ES256 assertion against a +// local token endpoint — exactly what internal/ab/auth_test.go already does. So: two httptest +// servers, an env pointed at them, and cobra commands run end to end. +// +// The handler COUNTS requests by path, which is the point. "Does this command work" and "does +// this command make 1 request or 200" are different questions, and only the second one catches +// the class of bug that made `adopt` time out. + +// fakeAPI is a stand-in Apple Business API plus its token endpoint. +type fakeAPI struct { + t *testing.T + + mu sync.Mutex + // requests counts GET/POST/PATCH/DELETE by "METHOD /path" (query string stripped), so a + // test can assert on fan-out rather than only on the final answer. + requests map[string]int + // configs is the tenant's CUSTOM_SETTING store, keyed by id. + configs map[string]*fakeAPIConfig + // dropWrites reproduces the defect the read-back exists for: Apple answers 2xx and keeps + // the old bytes. + dropWrites bool + + api *httptest.Server + token *httptest.Server +} + +type fakeAPIConfig struct { + ID string + Name string + XML string + Updated string +} + +func newFakeAPI(t *testing.T) *fakeAPI { + t.Helper() + f := &fakeAPI{t: t, requests: map[string]int{}, configs: map[string]*fakeAPIConfig{}} + + f.token = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"access_token":"fake-bearer","expires_in":3600}`)) + })) + f.api = httptest.NewServer(http.HandlerFunc(f.serve)) + t.Cleanup(func() { + f.api.Close() + f.token.Close() + }) + return f +} + +// addConfig seeds a live CUSTOM_SETTING config. +func (f *fakeAPI) addConfig(id, name, xml string) *fakeAPIConfig { + f.mu.Lock() + defer f.mu.Unlock() + c := &fakeAPIConfig{ID: id, Name: name, XML: xml, Updated: "2026-01-01T00:00:00Z"} + f.configs[id] = c + return c +} + +func (f *fakeAPI) count(key string) int { + f.mu.Lock() + defer f.mu.Unlock() + return f.requests[key] +} + +// countMatching sums every recorded request whose key contains sub. +func (f *fakeAPI) countMatching(sub string) int { + f.mu.Lock() + defer f.mu.Unlock() + n := 0 + for k, v := range f.requests { + if strings.Contains(k, sub) { + n += v + } + } + return n +} + +func (f *fakeAPI) serve(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + f.requests[r.Method+" "+r.URL.Path]++ + // The list endpoint is asked for profile XML via fields[]; recording that separately is + // what lets a test assert "this command must not download the whole tenant". + if r.Method == http.MethodGet && strings.Contains(r.URL.RawQuery, "customSettingsValues") { + f.requests["GET-with-xml "+r.URL.Path]++ + } + f.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + path := strings.TrimPrefix(r.URL.Path, "/v1") + + switch { + case r.Method == http.MethodGet && path == "/configurations": + f.listConfigs(w, strings.Contains(r.URL.RawQuery, "customSettingsValues")) + case r.Method == http.MethodGet && strings.HasPrefix(path, "/configurations/"): + f.getConfig(w, strings.TrimPrefix(path, "/configurations/")) + case r.Method == http.MethodPatch && strings.HasPrefix(path, "/configurations/"): + f.patchConfig(w, r, strings.TrimPrefix(path, "/configurations/")) + case r.Method == http.MethodPost && path == "/configurations": + f.postConfig(w, r) + case r.Method == http.MethodGet && path == "/blueprints": + _, _ = w.Write([]byte(`{"data":[]}`)) + default: + // Anything unmodelled is a hard failure rather than an empty 200: a silent {} would + // let a command "pass" a test by taking a path the fake never described. + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprintf(w, `{"errors":[{"detail":"fake tenant has no route for %s %s"}]}`, r.Method, path) + } +} + +func (f *fakeAPI) listConfigs(w http.ResponseWriter, withXML bool) { + f.mu.Lock() + defer f.mu.Unlock() + items := make([]map[string]any, 0, len(f.configs)) + for _, c := range f.configs { + attrs := map[string]any{"name": c.Name, "type": "CUSTOM_SETTING", "updatedDateTime": c.Updated} + if withXML { + attrs["customSettingsValues"] = map[string]any{"configurationProfile": c.XML} + } + items = append(items, map[string]any{"id": c.ID, "type": "configurations", "attributes": attrs}) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": items}) +} + +func (f *fakeAPI) getConfig(w http.ResponseWriter, id string) { + f.mu.Lock() + defer f.mu.Unlock() + c, ok := f.configs[id] + if !ok { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"detail":"no such configuration"}]}`)) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "id": c.ID, "type": "configurations", + "attributes": map[string]any{ + "name": c.Name, "type": "CUSTOM_SETTING", "updatedDateTime": c.Updated, + "customSettingsValues": map[string]any{"configurationProfile": c.XML}, + }, + }}) +} + +// patchConfig models Apple's accept-and-maybe-drop behaviour. With dropWrites the response is a +// clean 2xx and the stored bytes do not move — the exact incident the read-back guards. +func (f *fakeAPI) patchConfig(w http.ResponseWriter, r *http.Request, id string) { + body, _ := readBody(r) + f.mu.Lock() + defer f.mu.Unlock() + c, ok := f.configs[id] + if !ok { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"detail":"no such configuration"}]}`)) + return + } + if !f.dropWrites { + c.XML = profileFromBody(body) + c.Updated = "2026-02-02T00:00:00Z" + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "id": c.ID, "type": "configurations", + "attributes": map[string]any{"name": c.Name, "updatedDateTime": c.Updated}, + }}) +} + +func (f *fakeAPI) postConfig(w http.ResponseWriter, r *http.Request) { + body, _ := readBody(r) + f.mu.Lock() + defer f.mu.Unlock() + id := fmt.Sprintf("cfg-%d", len(f.configs)+1) + name, _ := body["data"].(map[string]any)["attributes"].(map[string]any)["name"].(string) + c := &fakeAPIConfig{ID: id, Name: name, Updated: "2026-02-02T00:00:00Z"} + if !f.dropWrites { + c.XML = profileFromBody(body) + } + f.configs[id] = c + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "id": id, "type": "configurations", + "attributes": map[string]any{"name": name, "updatedDateTime": c.Updated}, + }}) +} + +func readBody(r *http.Request) (map[string]any, error) { + var m map[string]any + err := json.NewDecoder(r.Body).Decode(&m) + return m, err +} + +func profileFromBody(body map[string]any) string { + data, _ := body["data"].(map[string]any) + attrs, _ := data["attributes"].(map[string]any) + csv, _ := attrs["customSettingsValues"].(map[string]any) + xml, _ := csv["configurationProfile"].(string) + return xml +} + +// use points abctl's config resolution at this fake tenant and gives it a temporary workspace, +// returning the workspace root. Everything is per-test via t.Setenv/t.TempDir. +func (f *fakeAPI) use(t *testing.T) string { + t.Helper() + dir := t.TempDir() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + sec1, err := x509.MarshalECPrivateKey(key) + if err != nil { + t.Fatal(err) + } + keyPath := filepath.Join(dir, "key.pem") + if err := os.WriteFile(keyPath, pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: sec1}), 0o600); err != nil { + t.Fatal(err) + } + + // An explicit context selector would send abctl to ~/.abctl; the env path keeps the test + // hermetic. ABCTL_ENV pins the .env (and therefore the gitops tree) inside the temp dir. + envPath := filepath.Join(dir, ".env") + env := fmt.Sprintf("AB_CLIENT_ID=BUSINESSAPI.test\nAB_PRIVATE_KEY=%s\nAB_TOKEN_URL=%s\nAB_API_BASE=%s/v1/\n", + keyPath, f.token.URL, f.api.URL) + if err := os.WriteFile(envPath, []byte(env), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ABCTL_ENV", envPath) + t.Setenv("ABCTL_CONTEXT", "") // never fall through to the developer's real contexts file + t.Setenv("ABCTL_APPROVE", "1") // the gate is tested separately; these tests exercise behaviour + t.Setenv("XDG_CACHE_HOME", dir) // keep the bearer cache out of the real one + flagContext = "" + return dir +} diff --git a/internal/cli/imperative.go b/internal/cli/imperative.go index beb81c3..710dace 100644 --- a/internal/cli/imperative.go +++ b/internal/cli/imperative.go @@ -15,6 +15,7 @@ import ( "github.com/GigaionLLC/abcli/internal/config" "github.com/GigaionLLC/abcli/internal/gitops" "github.com/GigaionLLC/abcli/internal/hash" + "github.com/GigaionLLC/abcli/internal/reconcile" "github.com/GigaionLLC/abcli/internal/state" ) @@ -36,6 +37,38 @@ type imp struct { cfg *config.Config tree *gitops.Tree base *state.State + + // configIndexCache is the tenant's name↔id list, fetched at most once per command. + // `apply -f` is the reason it exists: it resolves names once per DOCUMENT, so a 20-doc + // bulk file listed the whole tenant twenty times. nil means "not fetched yet"; an empty + // tenant caches as a non-nil empty slice, so an empty result is not refetched forever. + configIndexCache []ab.LiveConfig +} + +// configIndex returns the cached metadata list, fetching it on first use. +func (i *imp) configIndex() ([]ab.LiveConfig, error) { + if i.configIndexCache != nil { + return i.configIndexCache, nil + } + index, err := liveConfigIndex(i.c) + if err != nil { + return nil, err + } + if index == nil { + index = []ab.LiveConfig{} // distinguish "empty tenant" from "never fetched" + } + i.configIndexCache = index + return index, nil +} + +// noteCreatedConfig keeps the cache truthful after a create, so a later document in the same +// `apply -f` run can resolve a config an earlier document just made. Appending beats +// invalidating: a bulk file that creates ten configs would otherwise re-list ten times. +func (i *imp) noteCreatedConfig(id, name string) { + if i.configIndexCache == nil { + return // nothing cached yet; the next configIndex() will fetch a list that includes it + } + i.configIndexCache = append(i.configIndexCache, ab.LiveConfig{ID: id, Name: name}) } func newImp() (*imp, error) { @@ -82,14 +115,94 @@ func readFileArg(path string) ([]byte, error) { return os.ReadFile(path) } -// validateProfile is the built-in structural check (same as `abctl validate`). +// validateProfile is the pre-write structural check. It runs the SAME inspector +// `abctl validate` runs (checkProfile → inspectProfile), not a lesser one. +// +// It used to be three substring tests and a size cap, which meant the one rule the +// project added *because* of a live incident — a top-level PayloadVersion that is +// not exactly 1, which Apple accepts with a 2xx and then silently declines to store +// — was enforced by `abctl validate` and by nothing on the path that actually +// writes. `create`, `replace`, `edit` and `apply -f` would all cheerfully push the +// exact profile shape known to be dropped. +// +// Warnings are deliberately not fatal here: they are advice, and `--force` remains +// the way past a hard error. func validateProfile(b []byte) error { - s := string(b) - if len(b) >= 1<<20 { - return fmt.Errorf("profile is >= 1 MB (Apple Business cap)") + pr := checkProfile("", libFile{name: "profile", data: b}) + if len(pr.Errors) == 0 { + return nil } - if !strings.Contains(s, "PayloadType") || !strings.Contains(s, "Configuration") || !strings.Contains(s, "PayloadContent") { - return fmt.Errorf("profile is missing the Configuration/PayloadContent structure (use --force to skip)") + msgs := make([]string, 0, len(pr.Errors)) + for _, e := range pr.Errors { + msgs = append(msgs, e.Code+": "+e.Message) + } + return fmt.Errorf("profile failed validation (use --force to skip):\n - %s", strings.Join(msgs, "\n - ")) +} + +// confirmStored re-reads a config abctl just created or replaced and reports whether +// Apple actually kept the bytes that were sent. +// +// A 2xx is NOT proof. Apple accepts the write and then silently declines to persist a +// profile that violates its schema, leaving both the stored bytes and updatedDateTime +// untouched. Recording the bytes we SENT as the baseline turns that into a permanent +// lie: the next `--refresh=smart` run finds a baseline whose id and timestamp match +// live, reuses the cached hash, never fetches the real XML, and reports "in sync" +// forever while Apple holds the old profile. The reconcile engine learned this from a +// live incident (see reconcile.push); the imperative commands issue the same writes +// against the same API and need the same proof. +// +// The verdicts are reconcile's, deliberately — one incident, one vocabulary: +// - VerifyConfirmed the stored bytes match; the returned config is authoritative +// - VerifyNotPersisted Apple kept something else; the write did not take +// - VerifyUnconfirmed the read-back gave no answer. This is NOT evidence of +// failure — a GET that failed says nothing about the PATCH — so the caller must +// report it as unverified rather than failed. +func (i *imp) confirmStored(id string, sent []byte) (ab.LiveConfig, reconcile.Verification) { + stored, err := i.c.FetchCustomSettingDetail(id) + switch { + case err != nil: + return ab.LiveConfig{}, reconcile.VerifyUnconfirmed + case stored.ContentHash() == "": + // A detail response with no profile XML cannot be compared, and must not be + // used to accuse Apple of dropping a write it may well have stored. + return ab.LiveConfig{}, reconcile.VerifyUnconfirmed + case stored.ContentHash() != hash.Raw(sent): + return stored, reconcile.VerifyNotPersisted + default: + return stored, reconcile.VerifyConfirmed + } +} + +// recordWrite writes the profile into the git tree and records the baseline from what +// APPLE STORED, never from what was sent. It returns the error the command should exit +// with — non-nil only when Apple did not persist the write. +// +// On VerifyNotPersisted the tree file and the baseline are still written: the tree holds +// the operator's intent, the baseline holds Apple's reality, and the difference between +// them is exactly what makes the next `diff` show the drift and offer to retry. Writing +// neither would leave the operator with a failed command and no record of what they +// meant; writing the sent bytes into the baseline is the bug this exists to prevent. +func (i *imp) recordWrite(name, id string, sent []byte, stored ab.LiveConfig, v reconcile.Verification, noWriteTree bool) error { + if !noWriteTree { + if err := i.tree.WriteConfig(name, sent); err != nil { + return err + } + // No baseline at all when the read-back gave no answer: a baseline is a claim + // that git and Apple agreed at a known instant, and an unconfirmed write cannot + // support that claim. Its absence makes the next run re-read, which is right. + if v != reconcile.VerifyUnconfirmed { + i.base.Configs[name] = state.Entry{ABMID: id, Hash: stored.ContentHash(), UpdatedDateTime: stored.Updated} + if err := i.base.Save(i.tree.StateFile); err != nil { + return err + } + } + } + switch v { + case reconcile.VerifyNotPersisted: + return fmt.Errorf("%s (stored updatedDateTime %s)", reconcile.NotPersistedMessage, stored.Updated) + case reconcile.VerifyUnconfirmed: + fmt.Fprintf(os.Stderr, "warning: %q was written but could not be read back to confirm it landed; "+ + "the baseline was left alone so the next `abctl diff` re-checks it.\n", name) } return nil } @@ -120,6 +233,11 @@ type writeOutcome struct { // caller that reports the write as fully done would be lying, and the drift it // leaves behind resurfaces on every later diff. TreeError string `json:"treeError,omitempty"` + // Verified is the read-back verdict for a create/replace (reconcile.Verification): + // "confirmed", "not-persisted", or "unconfirmed". A 2xx from Apple is not proof the + // profile was stored, so a caller that reports success without reading this is + // reporting the tenant's acknowledgement rather than its state. + Verified string `json:"verified,omitempty"` } // wantsMachine reports whether a machine format (the --json shorthand or a global @@ -180,17 +298,16 @@ func runCreateConfig(name string, fl writeFlags) error { if err != nil { return err } - if !fl.noWriteTree { - if err := i.tree.WriteConfig(name, content); err != nil { - return err - } - i.base.Configs[name] = state.Entry{ABMID: id, Hash: hash.Raw(content), UpdatedDateTime: updated} - if err := i.base.Save(i.tree.StateFile); err != nil { - return err - } + stored, verdict := i.confirmStored(id, content) + if stored.Updated != "" { + updated = stored.Updated // Apple's stored timestamp beats the one the write echoed + } + if err := i.recordWrite(name, id, content, stored, verdict, fl.noWriteTree); err != nil { + return err } if wantsMachine(fl.json) { - return emitWrite(writeOutcome{Action: "create", Name: name, ID: id, UpdatedDateTime: updated, TreeUpdated: !fl.noWriteTree}, fl.json) + return emitWrite(writeOutcome{Action: "create", Name: name, ID: id, UpdatedDateTime: updated, + TreeUpdated: !fl.noWriteTree, Verified: string(verdict)}, fl.json) } fmt.Fprintf(os.Stderr, "created %q (id %s)%s\n", name, id, treeNote(fl.noWriteTree)) return nil @@ -253,17 +370,16 @@ func runReplaceConfig(nameOrID string, fl writeFlags) error { if err != nil { return err } - if !fl.noWriteTree { - if err := i.tree.WriteConfig(lc.Name, content); err != nil { - return err - } - i.base.Configs[lc.Name] = state.Entry{ABMID: lc.ID, Hash: hash.Raw(content), UpdatedDateTime: updated} - if err := i.base.Save(i.tree.StateFile); err != nil { - return err - } + stored, verdict := i.confirmStored(lc.ID, content) + if stored.Updated != "" { + updated = stored.Updated // Apple's stored timestamp beats the one the write echoed + } + if err := i.recordWrite(lc.Name, lc.ID, content, stored, verdict, fl.noWriteTree); err != nil { + return err } if wantsMachine(fl.json) { - return emitWrite(writeOutcome{Action: "replace", Name: lc.Name, ID: lc.ID, UpdatedDateTime: updated, Archive: arch, TreeUpdated: !fl.noWriteTree}, fl.json) + return emitWrite(writeOutcome{Action: "replace", Name: lc.Name, ID: lc.ID, UpdatedDateTime: updated, + Archive: arch, TreeUpdated: !fl.noWriteTree, Verified: string(verdict)}, fl.json) } fmt.Fprintf(os.Stderr, "replaced %q%s\n", lc.Name, treeNote(fl.noWriteTree)) return nil @@ -332,18 +448,12 @@ func applyReplaceResolved(i *imp, lc ab.LiveConfig, content []byte, fl writeFlag }); err != nil { return fmt.Errorf("archive failed (PATCH skipped): %w", err) } - updated, err := i.c.UpdateConfiguration(lc.ID, lc.Name, string(content)) - if err != nil { + if _, err := i.c.UpdateConfiguration(lc.ID, lc.Name, string(content)); err != nil { return err } - if !fl.noWriteTree { - if err := i.tree.WriteConfig(lc.Name, content); err != nil { - return err - } - i.base.Configs[lc.Name] = state.Entry{ABMID: lc.ID, Hash: hash.Raw(content), UpdatedDateTime: updated} - if err := i.base.Save(i.tree.StateFile); err != nil { - return err - } + stored, verdict := i.confirmStored(lc.ID, content) + if err := i.recordWrite(lc.Name, lc.ID, content, stored, verdict, fl.noWriteTree); err != nil { + return err } fmt.Fprintf(os.Stderr, "updated %q%s\n", lc.Name, treeNote(fl.noWriteTree)) return nil @@ -425,16 +535,36 @@ func runDeleteConfig(nameOrID string, fl writeFlags) error { return nil } -// resolveLiveConfig finds a live CUSTOM_SETTING config (with its XML) by name or id. +// resolveLiveConfig finds ONE live CUSTOM_SETTING config by name or id, with its XML. +// +// Two requests, not 1+N: a metadata-only list to turn the name into an id, then a single +// detail fetch for that one config. It used to call FetchCustomSettings, which asks Apple +// for `customSettingsValues` — the profile XML — for EVERY config in the tenant, and falls +// back to a per-config GET whenever the list comes back sparse. That is the trap `adopt` +// hit: the caller wants one profile and pays for all of them, and on a real tenant the +// command outran abgui's budget and appeared to do nothing at all. +// +// Callers here genuinely need the XML (it is what gets archived before an overwrite), so +// unlike liveConfigIndex this still fetches a profile — exactly one. func resolveLiveConfig(c *ab.Client, nameOrID string) (ab.LiveConfig, error) { - live, err := c.FetchCustomSettings() + index, err := liveConfigIndex(c) if err != nil { return ab.LiveConfig{}, err } - if lc, ok := findLiveConfig(live, nameOrID); ok { - return lc, nil + found, ok := findLiveConfig(index, nameOrID) + if !ok { + return ab.LiveConfig{}, fmt.Errorf("CUSTOM_SETTING config %q not found (by name or id)", nameOrID) + } + detail, err := c.FetchCustomSettingDetail(found.ID) + if err != nil { + return ab.LiveConfig{}, fmt.Errorf("reading configuration %q: %w", found.Name, err) + } + // The list is authoritative for the name (the detail response has been seen to omit + // fields); the detail is authoritative for the bytes. + if detail.Name == "" { + detail.Name = found.Name } - return ab.LiveConfig{}, fmt.Errorf("CUSTOM_SETTING config %q not found (by name or id)", nameOrID) + return detail, nil } func findLiveConfig(live []ab.LiveConfig, nameOrID string) (ab.LiveConfig, bool) { @@ -451,7 +581,10 @@ func findLiveConfig(live []ab.LiveConfig, nameOrID string) (ab.LiveConfig, bool) // post-write live config membership, so the git manifest always equals live (never // a partial set that a later `sync --prune` would treat as configs to detach). // live is the current CUSTOM_SETTING list (for id→name resolution). -func (i *imp) syncBlueprintManifest(bpName, bpID string, live []ab.LiveConfig) error { +// removed names the config a DETACH just took off the blueprint, or "" for an attach. +// It is excluded from the union below: without it, detaching a config that the manifest +// still declares would re-add it from the manifest and the detach would never stick. +func (i *imp) syncBlueprintManifest(bpName, bpID string, live []ab.LiveConfig, removed string) error { links, err := i.c.BlueprintRelationship(bpID, "configurations") if err != nil { return err @@ -479,10 +612,37 @@ func (i *imp) syncBlueprintManifest(bpName, bpID string, live []ab.LiveConfig) e return fmt.Errorf("reading existing blueprint manifests: %w", err) } spec := all[bpName] + // UNION with what the manifest already declares, rather than replacing it. + // + // A plain rewrite from live discards any config the operator has declared in git but + // not yet attached — a pending `attach-config` row in the plan — so attaching A would + // silently delete B's intent, and no later sync would ever propose it again. The + // rewrite-from-live rule is still right for the member that was just written (that + // one IS live now); it was only ever wrong about the entries it did not touch. + // + // A DETACH still has to remove its member, so the removed one is excluded explicitly + // rather than trusting the union to drop it. + attachedNames := toStringSet(names) + for _, declared := range spec.Configurations { + if _, isAttached := attachedNames[declared]; !isAttached && declared != removed { + names = append(names, declared) + } + } + sort.Strings(names) spec.Name, spec.ID, spec.Configurations = bpName, bpID, names return i.tree.WriteBlueprintSpec(spec) } +// toStringSet is the membership test used above; a map keeps the union linear rather +// than quadratic on a blueprint with many configurations. +func toStringSet(ss []string) map[string]struct{} { + m := make(map[string]struct{}, len(ss)) + for _, s := range ss { + m[s] = struct{}{} + } + return m +} + func addWriteFlags(cmd *cobra.Command, fl *writeFlags, needFile bool) { cmd.Flags().StringVarP(&fl.file, "file", "f", "", "path to the .mobileconfig ('-' for stdin)") cmd.Flags().StringVar(&fl.platforms, "platforms", "", "configuredForPlatforms (default PLATFORM_MACOS)") diff --git a/internal/cli/imperative_test.go b/internal/cli/imperative_test.go index 78f1358..60cc841 100644 --- a/internal/cli/imperative_test.go +++ b/internal/cli/imperative_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "github.com/GigaionLLC/abcli/internal/ab" @@ -18,9 +19,21 @@ func TestConfigName(t *testing.T) { } } +// wellFormedProfile is the minimum Apple actually accepts: a top-level dict with the +// four required keys and a PayloadVersion of exactly 1. +func wellFormedProfile(payloadVersion string) []byte { + return []byte(` + + PayloadTypeConfiguration + PayloadVersion` + payloadVersion + ` + PayloadIdentifiercom.example.test + PayloadUUID2B9E1A54-9E1D-4E7E-9B0E-9A1B2C3D4E5F + PayloadContent +`) +} + func TestValidateProfile(t *testing.T) { - good := []byte(`PayloadTypeConfigurationPayloadContent`) - if err := validateProfile(good); err != nil { + if err := validateProfile(wellFormedProfile("1")); err != nil { t.Errorf("valid profile rejected: %v", err) } if err := validateProfile([]byte("nope")); err == nil { @@ -31,6 +44,26 @@ func TestValidateProfile(t *testing.T) { } } +// TestValidateProfileRejectsBadPayloadVersion is the regression lock for the incident +// that motivated all of the read-back machinery: Apple accepts a profile whose TOP-LEVEL +// PayloadVersion is not 1 with a 2xx and then silently declines to store it, so the sync +// loops forever on a change that never lands. `abctl validate` has caught this since +// v0.4.19; the WRITE path did not, which meant create/replace/edit/apply -f would happily +// push the exact shape known to be dropped. Catching it here costs no API call at all. +func TestValidateProfileRejectsBadPayloadVersion(t *testing.T) { + err := validateProfile(wellFormedProfile("2")) + if err == nil { + t.Fatal("a top-level PayloadVersion of 2 must be rejected before the write") + } + if !strings.Contains(err.Error(), "payload-version") { + t.Errorf("error should name the payload-version rule, got: %v", err) + } + // The remedy has to be reachable: --force is how an operator overrides a hard error. + if !strings.Contains(err.Error(), "--force") { + t.Errorf("error should point at --force, got: %v", err) + } +} + func TestApiBody(t *testing.T) { // nil when no fields/input if b, err := apiBody(nil, ""); err != nil || b != nil { diff --git a/internal/cli/phase1.go b/internal/cli/phase1.go index 6d501cd..7c33280 100644 --- a/internal/cli/phase1.go +++ b/internal/cli/phase1.go @@ -61,7 +61,7 @@ func newDiffCmd() *cobra.Command { if err := validateRefreshMode(refresh); err != nil { return err } - pc, err := loadPlan(gitSourceOfTruth, refresh) + pc, err := loadPlan(gitSourceOfTruth, refresh, false) // diff never writes if err != nil { return err } @@ -150,7 +150,17 @@ func runSync(fl syncFlags) error { if err := validateVerifyMode(fl.verify); err != nil { return err } - pc, err := loadPlan(fl.gitSourceOfTruth, fl.refresh) + // metadata-only never populates LiveConfig.XML, and every actionable item needs it: + // an update archives the live profile first, a pull writes those bytes into git, a + // prune archives before deleting. Applying in this mode fails item by item and leaves + // the baseline untouched, so the identical plan comes back next run — loud, but a + // dead end. Refuse it here, where the remedy is one flag away. + if fl.apply && fl.refresh == refreshMetadata { + return fmt.Errorf("--refresh=metadata-only cannot be applied: it never fetches profile XML, "+ + "which every write needs to archive, pull or prune. Use --refresh=%s (the default) or --refresh=%s", + refreshSmart, refreshFull) + } + pc, err := loadPlan(fl.gitSourceOfTruth, fl.refresh, fl.apply) if err != nil { return err } @@ -243,7 +253,7 @@ func runSync(fl syncFlags) error { switch fl.verify { case verifyFull: fmt.Fprintln(os.Stderr, "post-apply verification: full live configuration and blueprint refresh...") - fetched, err := fetchLiveConfigsForPlan(pc.c, pc.desired, pc.base, fl.gitSourceOfTruth, refreshFull, func(line string) { + fetched, err := fetchLiveConfigsForPlan(pc.c, pc.desired, pc.base, refreshFull, true, func(line string) { fmt.Fprintln(os.Stderr, "post-apply verification: "+line) }) if err != nil { @@ -865,7 +875,10 @@ func validateVerifyMode(mode string) error { } } -func fetchLiveConfigsForPlan(c *ab.Client, desired map[string][]byte, base *state.State, gitSourceOfTruth bool, refresh string, progress func(string)) ([]ab.LiveConfig, error) { +// willWrite says whether THIS run may actually pull, archive or prune. It gates the +// per-config profile fetch in smart mode: a dry run compares nothing it keeps, so paying +// a request per live-only config buys a hash that is thrown away. +func fetchLiveConfigsForPlan(c *ab.Client, desired map[string][]byte, base *state.State, refresh string, willWrite bool, progress func(string)) ([]ab.LiveConfig, error) { switch refresh { case refreshFull: return c.FetchCustomSettingsWithProgress(progress) @@ -877,13 +890,18 @@ func fetchLiveConfigsForPlan(c *ab.Client, desired map[string][]byte, base *stat reuseCachedLiveHashes(live, base, progress) return live, nil case refreshSmart: - return fetchLiveConfigsSmart(c, desired, base, gitSourceOfTruth, progress) + return fetchLiveConfigsSmart(c, desired, base, willWrite, progress) default: return nil, fmt.Errorf("invalid refresh mode %q", refresh) } } -func fetchLiveConfigsSmart(c *ab.Client, desired map[string][]byte, base *state.State, gitSourceOfTruth bool, progress func(string)) ([]ab.LiveConfig, error) { +// The git-source-of-truth flag is deliberately NOT a parameter: it used to force a detail +// fetch for every live-only config, but under that mode such a config is a DELETE, and a +// delete needs the bytes only when the run will actually archive them — which is precisely +// what willWrite already says. Two flags meaning one thing is how the dry-run path ended up +// fetching profiles it discarded. +func fetchLiveConfigsSmart(c *ab.Client, desired map[string][]byte, base *state.State, willWrite bool, progress func(string)) ([]ab.LiveConfig, error) { live, err := c.FetchCustomSettingsMetadata(progress) if err != nil { return nil, err @@ -896,15 +914,27 @@ func fetchLiveConfigsSmart(c *ab.Client, desired map[string][]byte, base *state. if hasBase && baseEntry.ABMID == l.ID && sameAppleTime(baseEntry.UpdatedDateTime, l.Updated) { l.Hash = baseEntry.Hash } - needDetail := l.Hash == "" - if hasDesired && l.Hash != "" && hash.Raw(desiredXML) != l.Hash { - needDetail = true - } - if !hasDesired { - // Pulls and prunes need the actual live XML for writing or archiving. - needDetail = true - } - if gitSourceOfTruth && !hasDesired { + // Fetch a profile only where its BYTES are actually used. + // + // The order matters: an unconditional `needDetail := l.Hash == ""` first would make + // every other condition unreachable on an unseeded workspace, because with no baseline + // nothing has a cached hash. That is precisely the case worth optimizing, so the + // live-only branch has to be the one that decides. + needDetail := false + switch { + case hasDesired: + // Present on both sides, so the plan is decided by comparing hashes: the live + // hash must be real. Refetch when it was never cached, and when the cached hash + // disagrees with git — that disagreement is what an apply would act on, so it is + // confirmed against the actual bytes rather than trusted. + needDetail = l.Hash == "" || hash.Raw(desiredXML) != l.Hash + case willWrite: + // Live-only. The PLAN never needs these bytes — reconcile.Compute picks pull-new + // vs delete from the BASELINE, not from a hash — but an apply does, to write them + // into git or to archive before deleting. A dry run (`diff`, the CI plan job, + // `sync` without --apply) writes and archives nothing, and paying one request per + // live-only config to compute a hash it then discards is what turned the FAST + // mode into 1+N requests where `--refresh=full` is a single list call. needDetail = true } if !needDetail { @@ -980,7 +1010,7 @@ type planCtx struct { // loadPlan reads git desired + baseline + live for both configs and blueprints and // computes the two 3-way plans. Shared by diff and sync so both see an identical plan. -func loadPlan(gitSourceOfTruth bool, refresh string) (*planCtx, error) { +func loadPlan(gitSourceOfTruth bool, refresh string, willWrite bool) (*planCtx, error) { fmt.Fprintln(os.Stderr, "building plan: loading connection and workspace settings...") c, cfg, err := mustClient() if err != nil { @@ -1000,7 +1030,7 @@ func loadPlan(gitSourceOfTruth bool, refresh string) (*planCtx, error) { } fmt.Fprintf(os.Stderr, "building plan: loaded %d baseline configuration record(s).\n", len(base.Configs)) fmt.Fprintf(os.Stderr, "building plan: fetching live configurations from Apple (%s refresh)...\n", refresh) - live, err := fetchLiveConfigsForPlan(c, desired, base, gitSourceOfTruth, refresh, func(line string) { + live, err := fetchLiveConfigsForPlan(c, desired, base, refresh, willWrite, func(line string) { fmt.Fprintln(os.Stderr, "building plan: "+line) }) if err != nil { diff --git a/internal/gitops/gitops.go b/internal/gitops/gitops.go index 80eee40..8e166dc 100644 --- a/internal/gitops/gitops.go +++ b/internal/gitops/gitops.go @@ -64,17 +64,48 @@ func (t *Tree) LoadDesired() (map[string][]byte, error) { } // WriteConfig writes a profile into lib/ under the given name. +// +// The name comes from the TENANT (it is the configuration's `name` attribute, echoed +// back by Apple), so it is untrusted input on a filesystem path. A console-created +// config called `../../id_rsa` would otherwise have `seed`, `pull` and a reconcile +// Pull write outside the workspace entirely. internal/archive has guarded this since +// the beginning; lib/ never did. func (t *Tree) WriteConfig(name string, content []byte) error { + path, err := t.libPath(name) + if err != nil { + return err + } if err := os.MkdirAll(t.LibDir, 0o755); err != nil { return err } - return os.WriteFile(filepath.Join(t.LibDir, name), content, 0o644) + return os.WriteFile(path, content, 0o644) +} + +// libPath resolves a config name to its file inside LibDir, refusing anything that is +// not a plain filename in that one directory. Rejecting is deliberate over sanitizing: +// a silently rewritten name would no longer match the ABM config it came from, and the +// resulting file would drift forever against a config it can never be compared to. +func (t *Tree) libPath(name string) (string, error) { + // Both separators are checked, not just the host's: a tenant name carrying the + // foreign one still has to be refused, or the guard passes on one OS and not the + // other for the same configuration. + if name == "" || name == "." || name == ".." || + strings.ContainsAny(name, `/\`) || strings.ContainsRune(name, 0) || + filepath.IsAbs(name) || name != filepath.Base(name) { + return "", fmt.Errorf("configuration name %q is not a usable filename — abctl will not write it into %s "+ + "(rename it in Apple Business)", name, t.LibDir) + } + return filepath.Join(t.LibDir, name), nil } // RemoveConfig deletes a profile from lib/ (used when a config was removed from // ABM → the git file is pruned). A missing file is not an error (idempotent). func (t *Tree) RemoveConfig(name string) error { - err := os.Remove(filepath.Join(t.LibDir, name)) + path, perr := t.libPath(name) + if perr != nil { + return perr + } + err := os.Remove(path) if os.IsNotExist(err) { return nil } diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index ffdcd95..de656d7 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -29,6 +29,12 @@ const notPersisted = "write accepted (2xx) but Apple did not persist it — the "Apple silently drops profiles that violate its schema; check that the TOP-LEVEL Configuration PayloadVersion " + "is exactly 1 (Apple requires 1; a value of 2 reproduces this exactly)." +// NotPersistedMessage is the operator-facing diagnosis of a 2xx write Apple did not +// store. Exported so the imperative commands (which issue the same writes against the +// same API) report it in the same words as the reconcile engine — one incident, one +// explanation, one place to improve it. +const NotPersistedMessage = notPersisted + // Applier is the subset of the Apple Business write API the executor needs. It is // an interface so Apply can be unit-tested with a fake — no production writes. type Applier interface {