From 6001593a6253a8d1f74a947c65886db309e6e519 Mon Sep 17 00:00:00 2001 From: TheJakeBraun <210177983+TheJakeBraun@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:10:32 -0700 Subject: [PATCH 1/3] fix: prove writes landed, stop tenant-wide fan-out, and give each run its own state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two audits and an adversarial review of the resulting diff. Everything here is a defect that reached a release; the review caught four more that this change itself introduced, including one worse than the bug it was fixing. abctl — writes now prove they landed ------------------------------------ The imperative path had both defects the reconcile engine was fixed for in v0.4.19, and it is what abgui's "Save profile" button calls. `create`/`replace`/`edit`/`apply -f` recorded the baseline from the bytes they SENT. Apple accepts an out-of-spec profile with a 2xx and silently declines to store it, so that baseline asserted a state the tenant was not in — and it is self-sustaining: the next --refresh=smart run matches id and timestamp, reuses the poisoned hash, never re-reads the XML, and reports "in sync — no changes" forever while Apple holds the old profile. They now read back and record what Apple STORED (imp.confirmStored / imp.recordWrite), failing loudly when the write did not take. A read-back that ERRORS is reported unverified, not failed: a GET that failed says nothing about the PATCH. `validateProfile` on the write path was three substring checks and a size cap, so the top-level PayloadVersion rule — added precisely because of that incident — guarded `abctl validate` and nothing that writes. It now runs the same inspector. abctl — stop paying for the whole tenant ---------------------------------------- `resolveLiveConfig` called FetchCustomSettings to map ONE name to an id. That requests customSettingsValues for every config in the tenant and falls back to a per-config GET when the list is sparse — the trap that made `adopt` time out, sitting under `replace`, `edit`, `delete` and `status config`, on abgui's 60s budget. Now: metadata list + one detail fetch. `apply -f` resolved names once per DOCUMENT; the index is cached for the command and appended to on create, so a later document still resolves a config an earlier one made. Smart refresh no longer fetches a profile per live-only config on a run that will never write it. abctl — failures that reported success -------------------------------------- `status config` and `apply -f` swallowed relationship errors: a rate-limited tenant printed "attached to 0 blueprint(s)" and exited 0, an affirmatively false answer. `apply -f` warned and exited 0 when the manifest write failed, leaving git and the tenant disagreeing — the defect `attach` was already fixed for. `abctl attach` rewrote a manifest wholesale from live, silently discarding configs declared in git but not yet attached; it now unions, and a detach excludes only the member it removed. `sync --apply --refresh=metadata-only` is refused up front instead of failing item by item and re-planning identical work forever. `gitops.WriteConfig` no longer lets a tenant-supplied name escape lib/. abgui — each run owns its own state ----------------------------------- Flipping git source of truth mid-compute showed the PRE-FLIP plan with Cancel gone and Apply enabled, and applying used the NEW mode, which forces --prune. Pressing it deleted every Apple-only configuration against a plan that never mentioned a deletion. The cancelled run's teardown was clearing state that belonged to its successor. The first fix for that was worse than the bug: it reused `loadGeneration`, which `run()` bumps for every resource list, so opening Configurations during a seed invalidated the SEED's token, `isSeeding` was never cleared, and the Diff screen sat on "Initializing workspace…" forever with its escape buttons disabled. Two clicks, no error condition, relaunch the only exit. Plan and seed now own `workGeneration`, and the plan owns `isPlanning` rather than the shared `isLoading`. Also: `lastWriteError` gained an owner, so a failed config delete no longer paints red text across the Diff plan and the Archive table; ArchiveView's two sibling views got a container (a bare TupleView composited the error over the table); ApplyResult decodes abctl's structured `verification` key instead of grepping stderr for the word FAILED against a hand-maintained list of Go narration strings; ProcessRunner writes stdin off-thread inside the cancellation/watchdog scope and reports a partial write rather than sending Apple half a profile; context-store failures are surfaced; the Devices table lost a permanently-empty OS column and reads managedAppleAccount. Closing the blind spot ---------------------- `internal/cli` could only test pure helpers, because every command builds a client — which is why all of this shipped. It needs no credentials: config reads AB_TOKEN_URL/AB_API_BASE from the environment and a generated EC key mints a real ES256 assertion, so two httptest servers make a fake tenant that COUNTS requests. Eight end-to-end tests now drive real commands, including the two that would have caught this session's bugs as failures rather than bug reports: a 2xx-with- unchanged-bytes write must fail, and resolving one name must not request tenant-wide profile XML. Go: build, vet, gofmt, full suite green. Swift: macOS CI is the gate. Co-Authored-By: Claude Opus 5 --- abgui/Sources/abgui/AppModel.swift | 175 +++++++++--- .../Sources/abgui/Backend/ProcessRunner.swift | 43 ++- abgui/Sources/abgui/Models/ApplyResult.swift | 57 ++++ abgui/Sources/abgui/Models/ReadOnlyKind.swift | 9 +- abgui/Sources/abgui/Views/ApplySheet.swift | 19 +- abgui/Sources/abgui/Views/ArchiveView.swift | 35 ++- .../abgui/Views/ConfigEditorView.swift | 2 +- abgui/Sources/abgui/Views/ConsoleView.swift | 3 - abgui/Sources/abgui/Views/DiffView.swift | 12 +- .../Sources/abgui/Views/MembershipSheet.swift | 2 +- abgui/Tests/abguiTests/ContractTests.swift | 67 +++++ internal/cli/apply.go | 56 ++-- internal/cli/command_behavior_test.go | 226 ++++++++++++++++ internal/cli/deploy.go | 21 +- internal/cli/fakeapi_test.go | 256 ++++++++++++++++++ internal/cli/imperative.go | 240 +++++++++++++--- internal/cli/imperative_test.go | 37 ++- internal/cli/phase1.go | 59 ++-- internal/gitops/gitops.go | 35 ++- internal/reconcile/apply.go | 6 + 20 files changed, 1215 insertions(+), 145 deletions(-) create mode 100644 internal/cli/command_behavior_test.go create mode 100644 internal/cli/fakeapi_test.go 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..bd7cb40 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,35 @@ 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 { + throw AbctlError.cli("failed to send the profile to abctl (it may have received only part of it): " + + stdinError.localizedDescription + "\n" + stderr) + } + // 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 +94,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..56587c5 100644 --- a/abgui/Tests/abguiTests/ContractTests.swift +++ b/abgui/Tests/abguiTests/ContractTests.swift @@ -1506,3 +1506,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..b058f8a 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, fl.gitSourceOfTruth, 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, gitSourceOfTruth bool, refresh string, willWrite bool, progress func(string)) ([]ab.LiveConfig, error) { switch refresh { case refreshFull: return c.FetchCustomSettingsWithProgress(progress) @@ -877,13 +890,13 @@ 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, gitSourceOfTruth, 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) { +func fetchLiveConfigsSmart(c *ab.Client, desired map[string][]byte, base *state.State, gitSourceOfTruth bool, willWrite bool, progress func(string)) ([]ab.LiveConfig, error) { live, err := c.FetchCustomSettingsMetadata(progress) if err != nil { return nil, err @@ -896,15 +909,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 +1005,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 +1025,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, gitSourceOfTruth, 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 { From 740bae4f89f3ada8f5176a7ea646002faede2216 Mon Sep 17 00:00:00 2001 From: TheJakeBraun <210177983+TheJakeBraun@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:15:41 -0700 Subject: [PATCH 2/3] fix: decode stderr before concatenating it, and drop a parameter the switch made dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both caught by CI on the previous commit. ProcessRunner: the stdin-failure message concatenated `stderr` while it is still Data — it is decoded once, lower down, for the result. Swift is right to refuse "String + Data"; the message now decodes it at the point of use. phase1: rewriting the smart-refresh detail decision as a switch made `gitSourceOfTruth` genuinely unused, and revive said so. It is not merely unreferenced — it is redundant: under that mode a live-only config is a DELETE, and a delete needs the bytes only when the run will archive them, which is exactly what `willWrite` already says. Two parameters meaning one thing is how the dry-run path ended up fetching profiles it discarded, so the flag is removed from the fetch signatures rather than silenced with an underscore. Co-Authored-By: Claude Opus 5 --- abgui/Sources/abgui/Backend/ProcessRunner.swift | 4 +++- internal/cli/phase1.go | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/abgui/Sources/abgui/Backend/ProcessRunner.swift b/abgui/Sources/abgui/Backend/ProcessRunner.swift index bd7cb40..bf3a3d7 100644 --- a/abgui/Sources/abgui/Backend/ProcessRunner.swift +++ b/abgui/Sources/abgui/Backend/ProcessRunner.swift @@ -67,8 +67,10 @@ actor ProcessRunner: AbctlRunner { // 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" + stderr) + + stdinError.localizedDescription + "\n" + + String(decoding: stderr, as: UTF8.self)) } // The watchdog ran to completion iff it fired (timeout); if it was still sleeping diff --git a/internal/cli/phase1.go b/internal/cli/phase1.go index b058f8a..7c33280 100644 --- a/internal/cli/phase1.go +++ b/internal/cli/phase1.go @@ -253,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, true, 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 { @@ -878,7 +878,7 @@ func validateVerifyMode(mode string) 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, gitSourceOfTruth bool, refresh string, willWrite bool, progress func(string)) ([]ab.LiveConfig, error) { +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) @@ -890,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, willWrite, 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, willWrite 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 @@ -1025,7 +1030,7 @@ func loadPlan(gitSourceOfTruth bool, refresh string, willWrite bool) (*planCtx, } 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, willWrite, func(line string) { + live, err := fetchLiveConfigsForPlan(c, desired, base, refresh, willWrite, func(line string) { fmt.Fprintln(os.Stderr, "building plan: "+line) }) if err != nil { From 7b6079a6a7f7eead2118e121b1ed43ef26b766d0 Mon Sep 17 00:00:00 2001 From: TheJakeBraun <210177983+TheJakeBraun@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:18:11 -0700 Subject: [PATCH 3/3] test(abgui): the users fixture invented an attribute Apple does not emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this: after the Managed Apple ID column was pointed at the real attribute, testUserRolesDecodeAndColumns failed. The fixture said `managedAppleId`. Apple emits `managedAppleAccount` — every Go read of it agrees (ab/client.go:453,475, ab/detail.go:165, cli/inspect.go:304, cli/deploy.go:189) and UserDetailSheet already had it right. Only the list column and this fixture used the invented key, so the column silently fell through to `email` for every user and the test passed by asserting against the same wrong key the column read. A fixture that invents its input can only ever confirm the bug it was written beside. The fixture now carries BOTH attributes, so it also pins the precedence the column's header promises: the managed account wins over the corporate email. A second case covers a user with no managed account falling back to email rather than rendering an em dash. Co-Authored-By: Claude Opus 5 --- abgui/Tests/abguiTests/ContractTests.swift | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/abgui/Tests/abguiTests/ContractTests.swift b/abgui/Tests/abguiTests/ContractTests.swift index 56587c5..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] = []