diff --git a/abgui/Sources/abgui/AppModel.swift b/abgui/Sources/abgui/AppModel.swift index f61ab9a..7380097 100644 --- a/abgui/Sources/abgui/AppModel.swift +++ b/abgui/Sources/abgui/AppModel.swift @@ -50,7 +50,7 @@ final class AppModel { /// 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 { - await run { self.osReleases = try await $0.osReleases() } + await run(.osReleases) { self.osReleases = try await $0.osReleases() } } // Apps & Books (VPP) — a separate service; the content token is held in-memory only. @@ -121,7 +121,11 @@ final class AppModel { } // Per-screen UI state - var isLoading = false + // + // `loadError` belongs to the PLAN (DiffView renders it); every list screen owns its own via + // `failure(_:)`. The old shared `isLoading` is gone — the plan has `isPlanning`, the seed has + // `isSeeding`, and each list pane answers `loading(_:)` for itself, so no screen can spin or + // stop spinning because of work it did not start. var loadError: String? var progressLog: [String] = [] // live stderr narration from abctl during diff/seed var applyProgressLog: [String] = [] // live sync/apply progress, separate from final outcomes @@ -757,8 +761,8 @@ final class AppModel { // MARK: loads (each spawns a fresh abctl; errors surface in loadError) - func loadConfigurations() async { await run { self.configurations = try await $0.configurations() } } - func loadBlueprints() async { await run { self.blueprints = try await $0.blueprints() } } + func loadConfigurations() async { await run(.configurations) { self.configurations = try await $0.configurations() } } + func loadBlueprints() async { await run(.blueprints) { self.blueprints = try await $0.blueprints() } } /// Compute the 3-way plan. `resetLog: false` keeps whatever is already in `progressLog` and /// appends this run's transcript underneath it — the seed → diff hand-off, where wiping the @@ -970,7 +974,7 @@ final class AppModel { /// Load a read-only resource (a live GET; never writes). func loadReadOnly(_ kind: ReadOnlyKind) async { - await run { client in + await run(.readOnly(kind)) { client in switch kind { case .devices: self.devices = try await client.devices() case .mdmDevices: self.mdmDevices = try await client.mdmDevices() @@ -1267,31 +1271,58 @@ final class AppModel { /// 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 - /// shared isLoading/loadError, so an overlapping older run can't clear a live - /// load's spinner or overwrite its error with a stale one. - private func run(_ body: (AbctlClient) async throws -> Void) async { + // MARK: per-pane load state + // + // One `isLoading`/`loadError` shared by every screen meant one screen's fetch was another + // screen's spinner and another screen's error. Concretely: a slow Devices fetch spun + // Configurations; an OS Releases failure — which can be raised from inside a device detail + // SHEET — rendered on whatever list was behind it; and a generation counter existed purely + // to stop two overlapping reads clobbering each other's message. A pane owns its own state + // instead, so "is this screen busy" is a question about that screen. + + /// Which screen a load belongs to. + enum LoadPane: Hashable { + case configurations, blueprints, osReleases, contexts + case readOnly(ReadOnlyKind) + } + + private var loadingPanes: Set = [] + private var paneErrors: [LoadPane: String] = [:] + + /// Named `loading(_:)`/`failure(_:)` rather than overloading the existing `isLoading` and + /// `loadError` properties: those still belong to the Diff screen's plan, and two things with + /// one name is how this state got confusing in the first place. + func loading(_ pane: LoadPane) -> Bool { loadingPanes.contains(pane) } + func failure(_ pane: LoadPane) -> String? { paneErrors[pane] } + + /// Per-pane load wrapper. Cancellation (navigating away terminates the abctl child) is + /// swallowed — never shown as an error — and a pane's state is only ever written by a load + /// of that pane, so two screens loading at once cannot interfere at all. + private func run(_ pane: LoadPane, _ body: (AbctlClient) async throws -> Void) async { guard let client = makeClient() else { - loadError = "abctl was not found in the app bundle." + paneErrors[pane] = "abctl was not found in the app bundle." return } + // Still a generation, but scoped to THIS pane: re-entering a screen while its own + // previous fetch is in flight should not have the older one clear the newer's spinner. loadGeneration += 1 let generation = loadGeneration - isLoading = true - loadError = nil + paneGeneration[pane] = generation + loadingPanes.insert(pane) + paneErrors[pane] = nil do { try await body(client) } catch is CancellationError { - // user navigated away / cancelled — clear the in-flight state, no error shown + // user navigated away / cancelled — no error shown } catch { - if !Task.isCancelled, generation == loadGeneration { - loadError = error.localizedDescription + if !Task.isCancelled, paneGeneration[pane] == generation { + paneErrors[pane] = error.localizedDescription } } - if generation == loadGeneration { isLoading = false } + if paneGeneration[pane] == generation { loadingPanes.remove(pane) } } + + private var paneGeneration: [LoadPane: Int] = [:] } /// The model owns the notice type; the banner that renders it spells it plainly. diff --git a/abgui/Sources/abgui/Views/ArchiveView.swift b/abgui/Sources/abgui/Views/ArchiveView.swift index ae95926..4389e0c 100644 --- a/abgui/Sources/abgui/Views/ArchiveView.swift +++ b/abgui/Sources/abgui/Views/ArchiveView.swift @@ -86,6 +86,15 @@ struct ArchiveFileView: View { @Environment(\.dismiss) private var dismiss let entry: ArchiveEntry + /// Read ONCE into state, not inside `body`. + /// + /// The profile was previously loaded by a synchronous `String(contentsOf:)` in the view + /// body — up to Apple's 1 MiB cap, on the main actor, re-executed on every body evaluation + /// rather than once per file. The same class of mistake as rebuilding the whole transcript + /// per log line: work that looks free because it is written as an expression. + @State private var contents: String? + @State private var failed = false + var body: some View { VStack(spacing: 0) { HStack { @@ -95,14 +104,30 @@ struct ArchiveFileView: View { } .padding() Divider() - ScrollView([.horizontal, .vertical]) { - Text((try? String(contentsOf: entry.fileURL, encoding: .utf8)) ?? "(couldn't read this archived file)") - .font(.system(.footnote, design: .monospaced)) - .textSelection(.enabled) - .padding() - .frame(maxWidth: .infinity, alignment: .leading) + if let contents { + ScrollView([.horizontal, .vertical]) { + Text(contents) + .font(.system(.footnote, design: .monospaced)) + .textSelection(.enabled) + .padding() + .frame(maxWidth: .infinity, alignment: .leading) + } + } else if failed { + ContentUnavailableView("Couldn't read this archived file", systemImage: "exclamationmark.triangle", + description: Text(entry.fileURL.lastPathComponent)) + .frame(maxWidth: .infinity, maxHeight: .infinity) + } else { + ProgressView().frame(maxWidth: .infinity, maxHeight: .infinity) } } .frame(minWidth: 640, minHeight: 480) + .task(id: entry.fileURL) { + // Off the main actor: a large archived profile on a slow volume should not + // stall the window while the sheet is opening. + let url = entry.fileURL + let loaded = await Task.detached { try? String(contentsOf: url, encoding: .utf8) }.value + contents = loaded + failed = loaded == nil + } } } diff --git a/abgui/Sources/abgui/Views/AssignSheet.swift b/abgui/Sources/abgui/Views/AssignSheet.swift index adaca27..5829af8 100644 --- a/abgui/Sources/abgui/Views/AssignSheet.swift +++ b/abgui/Sources/abgui/Views/AssignSheet.swift @@ -22,9 +22,10 @@ struct AssignSheet: View { @State private var activity: Resource? // the last polled activity status @State private var statusBusy = false @State private var statusError: String? - // The server-list fetch has its OWN busy/error state (like assign/poll above) — - // the shared model.isLoading/loadError belong to the list screens, and a swallowed - // failure here would masquerade as "No MDM servers found." on a write-gating picker. + // The server-list fetch has its OWN busy/error state (like assign/poll above): this is a + // sheet, not a pane, so it has no entry in the model's per-pane load state — and a + // swallowed failure here would masquerade as "No MDM servers found." on a write-gating + // picker. @State private var serversBusy = false @State private var serversError: String? diff --git a/abgui/Sources/abgui/Views/BlueprintsView.swift b/abgui/Sources/abgui/Views/BlueprintsView.swift index f51536a..f0deb25 100644 --- a/abgui/Sources/abgui/Views/BlueprintsView.swift +++ b/abgui/Sources/abgui/Views/BlueprintsView.swift @@ -20,7 +20,7 @@ struct BlueprintsView: View { openDetail(ids.first) } .overlay { - ListStateOverlay(isLoading: model.isLoading, error: model.loadError, + ListStateOverlay(isLoading: model.loading(.blueprints), error: model.failure(.blueprints), isEmpty: model.blueprints.isEmpty, emptyTitle: "No blueprints", emptySymbol: "square.stack.3d.up") } diff --git a/abgui/Sources/abgui/Views/Components.swift b/abgui/Sources/abgui/Views/Components.swift index ce14c35..13f8e18 100644 --- a/abgui/Sources/abgui/Views/Components.swift +++ b/abgui/Sources/abgui/Views/Components.swift @@ -2,20 +2,47 @@ // SPDX-License-Identifier: AGPL-3.0-or-later import SwiftUI +/// How much room a toolbar control earns. +/// +/// Showing the title on EVERY control was the first attempt, and it traded one problem for +/// another: an unlabelled toolbar is a row of anonymous glyphs with nothing to hover for, but a +/// fully labelled one outgrew the window — at ~1090px the last item was clipped off the right +/// edge with no overflow affordance. Width is finite, so it has to be spent where a word +/// actually resolves an ambiguity. +enum ToolbarWeight { + /// Title + icon, always. For the control whose consequence you must not misread: `Apply…` + /// writes the live tenant, and `Verify Configs` sits beside it wearing a near-identical + /// checkmark glyph. These two are exactly the pair a bare icon cannot distinguish. + case titled + /// Icon + tooltip. For controls whose glyph is universally understood — a refresh arrow, a + /// folder, a trash can — where the title costs width and adds nothing a hover does not. + case compact +} + extension View { - /// Make a toolbar control READ as what it does: show its title next to the icon, and give it - /// a tooltip saying what pressing it will actually do. + /// Make a toolbar control READ as what it does. /// - /// macOS renders a bare `Label` inside a `.toolbar` as its icon alone, and SwiftUI supplies - /// no tooltip of its own — so an unannotated toolbar is a row of anonymous glyphs with - /// nothing to hover for. That is not a cosmetic problem in this app: `checkmark.shield` - /// (validate local files) and `checkmark.circle` (write the live tenant) are near-identical - /// at toolbar size, and a `trash` means "delete from Apple Business" on one screen and - /// "clear this session's log" on another. The word is the affordance; `help` carries the - /// consequence, and should say plainly when a button reaches the tenant. - func toolbarLabel(_ help: String) -> some View { - labelStyle(.titleAndIcon).help(help) + /// `help` is supplied for BOTH weights and is never optional: macOS renders a bare `Label` + /// in a `.toolbar` as its icon alone and SwiftUI adds no tooltip of its own, so a control + /// without one is unidentifiable by any means at all. The tooltip should name the + /// consequence, and say plainly when the button reaches the tenant — a `trash` means + /// "delete from Apple Business" on one screen and "clear this session's log" on another. + func toolbarLabel(_ help: String, weight: ToolbarWeight = .compact) -> some View { + labelStyle(weight == .titled ? AnyLabelStyle(TitleAndIconLabelStyle()) + : AnyLabelStyle(IconOnlyLabelStyle())) + .help(help) + } +} + +/// Type-erasing wrapper so one modifier can choose between label styles at runtime. +/// `labelStyle` takes a concrete generic type, so a ternary over two different styles needs +/// this rather than being expressible inline. +struct AnyLabelStyle: LabelStyle { + private let make: (Configuration) -> AnyView + init(_ style: S) { + make = { AnyView(style.makeBody(configuration: $0)) } } + func makeBody(configuration: Configuration) -> some View { make(configuration) } } /// A toolbar refresh button that runs an async action. `help` names WHAT is being re-fetched — diff --git a/abgui/Sources/abgui/Views/ConfigurationsView.swift b/abgui/Sources/abgui/Views/ConfigurationsView.swift index 7948825..b01fdf6 100644 --- a/abgui/Sources/abgui/Views/ConfigurationsView.swift +++ b/abgui/Sources/abgui/Views/ConfigurationsView.swift @@ -52,14 +52,14 @@ struct ConfigurationsView: View { } } .overlay { - ListStateOverlay(isLoading: model.isLoading, error: model.loadError, + ListStateOverlay(isLoading: model.loading(.configurations), error: model.failure(.configurations), isEmpty: model.configurations.isEmpty, emptyTitle: "No configurations", emptySymbol: "doc.text") } .navigationTitle("Configurations") .toolbar { Button { editorTarget = .create } label: { Label("New", systemImage: "plus") } - .toolbarLabel("Write a new .mobileconfig and create it in Apple Business.") + .toolbarLabel("Write a new .mobileconfig and create it in Apple Business.", weight: .titled) Button { if let config = selectedResource { editorTarget = .edit(config) } } label: { Label("Edit", systemImage: "pencil") } @@ -71,7 +71,7 @@ struct ConfigurationsView: View { .toolbarLabel("Attach or detach the selected configuration to a blueprint. Writes Apple Business and updates gitops/blueprints/.") .disabled(selection == nil) Button(role: .destructive) { confirmDelete = true } label: { Label("Delete", systemImage: "trash") } - .toolbarLabel("Delete the selected configuration from Apple Business (archived first).") + .toolbarLabel("Delete the selected configuration from Apple Business (archived first).", weight: .titled) .disabled(selection == nil) RefreshButton(help: "Re-fetch the configuration list from Apple Business.") { await model.loadConfigurations() diff --git a/abgui/Sources/abgui/Views/DashboardView.swift b/abgui/Sources/abgui/Views/DashboardView.swift index 5098512..129876f 100644 --- a/abgui/Sources/abgui/Views/DashboardView.swift +++ b/abgui/Sources/abgui/Views/DashboardView.swift @@ -120,17 +120,24 @@ struct DashboardView: View { loaded.insert(item) // a cache its own screen already filled counts as loaded continue } + // Each load reports into its OWN pane, so the pass reads back the failure of the + // thing it just ran rather than whatever last touched a shared slot. That + // misattribution used to need a comment explaining why the error might not be ours. + var pane: AppModel.LoadPane switch item { - case .configurations: await model.loadConfigurations() - case .blueprints: await model.loadBlueprints() + case .configurations: + pane = .configurations + await model.loadConfigurations() + case .blueprints: + pane = .blueprints + await model.loadBlueprints() default: guard let kind = item.readOnly else { continue } + pane = .readOnly(kind) await model.loadReadOnly(kind) } - // Cancelled mid-call (tile click / navigation): the shared loadError now - // belongs to the destination screen's load — don't read or misattribute it. - if Task.isCancelled { return } - if let error = model.loadError { + if Task.isCancelled { return } // navigated away mid-call + if let error = model.failure(pane) { loadFailed = error // stop the pass on the first failure (see above) return } diff --git a/abgui/Sources/abgui/Views/DiffView.swift b/abgui/Sources/abgui/Views/DiffView.swift index 960efbf..58c0eca 100644 --- a/abgui/Sources/abgui/Views/DiffView.swift +++ b/abgui/Sources/abgui/Views/DiffView.swift @@ -48,18 +48,19 @@ struct DiffView: View { GitSourceOfTruthControl(layout: .toolbar, isOn: model.gitSourceOfTruth, pending: $pendingGitSourceOfTruth) - // These four SHOW THEIR TITLES. A macOS toolbar renders a bare `Label` as - // its icon alone, and with no `.help` there is not even a tooltip to fall - // back on — the row read as four anonymous glyphs, two of which - // (checkmark.shield / checkmark.circle) are near-identical at toolbar size - // while doing very different things: one only reads local files, the other - // writes the live tenant. The word is the affordance, exactly as it is on - // the Git-source-of-truth control sitting beside them. + // Titles go to the two controls a glyph cannot tell apart: `checkmark.shield` + // (reads local files) and `checkmark.circle` (writes the live tenant) are + // near-identical at toolbar size while doing very different things. Refresh + // and Workspace keep their universally-legible glyph plus a tooltip — giving + // every control a title is what made this toolbar outgrow the window and clip + // its last item at ~1090px. Button { sheet = .validate } label: { Label("Verify Configs", systemImage: "checkmark.shield") } - .toolbarLabel("Check the profiles in gitops/lib against Apple's schema. Reads local files only — no tenant change.") + .toolbarLabel("Check the profiles in gitops/lib against Apple's schema. Reads local files only — no tenant change.", + weight: .titled) .disabled(model.isSeeding) Button { sheet = .apply } label: { Label("Apply…", systemImage: "checkmark.circle") } - .toolbarLabel("Reconcile Apple Business with this plan. Opens a sheet that previews the exact abctl command and asks you to confirm before anything is written.") + .toolbarLabel("Reconcile Apple Business with this plan. Opens a sheet that previews the exact abctl command and asks you to confirm before anything is written.", + weight: .titled) .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.") diff --git a/abgui/Sources/abgui/Views/OSReleasesView.swift b/abgui/Sources/abgui/Views/OSReleasesView.swift index d15eeda..73b2526 100644 --- a/abgui/Sources/abgui/Views/OSReleasesView.swift +++ b/abgui/Sources/abgui/Views/OSReleasesView.swift @@ -32,8 +32,8 @@ struct OSReleasesView: View { TableColumn("Devices") { Text(String($0.supportedDevices?.count ?? 0)) } } .overlay { - if model.isLoading { ProgressView() } - else if let error = model.loadError { ContentUnavailableView("Couldn't load releases", systemImage: "exclamationmark.triangle", description: Text(error)) } + if model.loading(.osReleases) { ProgressView() } + else if let error = model.failure(.osReleases) { ContentUnavailableView("Couldn't load releases", systemImage: "exclamationmark.triangle", description: Text(error)) } else if rows.isEmpty { ContentUnavailableView.search(text: search) } } } diff --git a/abgui/Sources/abgui/Views/ReadOnlyListView.swift b/abgui/Sources/abgui/Views/ReadOnlyListView.swift index d3ebb65..07779bf 100644 --- a/abgui/Sources/abgui/Views/ReadOnlyListView.swift +++ b/abgui/Sources/abgui/Views/ReadOnlyListView.swift @@ -57,7 +57,7 @@ struct ReadOnlyListView: View { openDetail(ids.first) } .overlay { - ListStateOverlay(isLoading: model.isLoading, error: model.loadError, + ListStateOverlay(isLoading: model.loading(.readOnly(kind)), error: model.failure(.readOnly(kind)), isEmpty: displayedRows.isEmpty, emptyTitle: searchText.isEmpty ? "No \(kind.title.lowercased())" : "No matches", emptySymbol: kind.symbol) diff --git a/abgui/Sources/abgui/Views/RunLogsView.swift b/abgui/Sources/abgui/Views/RunLogsView.swift index 4f0f708..995b92c 100644 --- a/abgui/Sources/abgui/Views/RunLogsView.swift +++ b/abgui/Sources/abgui/Views/RunLogsView.swift @@ -42,14 +42,14 @@ struct RunLogsView: View { .toolbarLabel("Show this log file in Finder.") .disabled(selected == nil) - RefreshButton(help: "Re-scan ~/Library/Logs/abgui for run logs.") { reload() } + RefreshButton(help: "Re-scan ~/Library/Logs/abgui for run logs.") { await reload() } } .fileExporter(isPresented: $showExporter, document: TextDocument(text: contents ?? ""), contentType: .plainText, defaultFilename: selected?.name ?? "abgui.log") { _ in } - .task { reload() } - .onChange(of: selection) { _, _ in loadSelected() } + .task { await reload() } + .onChange(of: selection) { _, _ in Task { await loadSelected() } } } @ViewBuilder private var content: some View { @@ -61,7 +61,7 @@ struct RunLogsView: View { + "abctl printed, how long each step took and how the run ended. " + "Run one from Diff / Drift and it will appear here.") } actions: { - Button("Refresh") { reload() } + Button("Refresh") { Task { await reload() } } } } else { HStack(spacing: 0) { @@ -173,20 +173,32 @@ struct RunLogsView: View { return log.isFailure ? .red : .green } - private func reload() { - logs = RunLogIndex.scan() - // Keep the current selection if it survived the re-scan; otherwise open the newest, which - // is the run someone almost always came here to read. + /// Scan and read OFF the main actor. + /// + /// The directory scan probes the tail of up to fifty files, and opening a run reads a + /// transcript that `RunLog` caps at 5 MiB. Doing that synchronously from `.task` put both + /// on the main actor — a hitch on a slow volume, and the same "work written as an + /// expression looks free" mistake as reading a profile inside a view body. + private func reload() async { + let scanned = await Task.detached { RunLogIndex.scan() }.value + logs = scanned + // Keep the current selection if it survived the re-scan; otherwise open the newest, + // which is the run someone almost always came here to read. if selection == nil || !logs.contains(where: { $0.id == selection }) { selection = logs.first?.id } - loadSelected() + await loadSelected() } - private func loadSelected() { + private func loadSelected() async { guard let selected else { contents = nil; loadFailed = false; return } - contents = RunLogIndex.contents(of: selected.url) - loadFailed = contents == nil + let url = selected.url + let loaded = await Task.detached { RunLogIndex.contents(of: url) }.value + // The selection can change while a large log is being read; publishing a stale one + // would show the wrong transcript under the right filename. + guard selected.id == selection else { return } + contents = loaded + loadFailed = loaded == nil } private func copy(_ text: String, field: String) { diff --git a/internal/cli/get.go b/internal/cli/get.go index a7179b0..8fdd2e4 100644 --- a/internal/cli/get.go +++ b/internal/cli/get.go @@ -299,9 +299,20 @@ func getBlueprint(nameOrID string, asJSON bool) error { if err != nil { return err } + // Propagate relationship errors rather than rendering a failure as an empty collection. + // A swallowed 429 or 403 shows up as "0 members" in the table AND in the --json document + // abgui's blueprint inspector consumes — an affirmatively false answer, from the one screen + // an operator opens to check what a blueprint actually carries. `ListPackages` is + // permission-gated and legitimately 403s, so this fires on a real, expected condition + // rather than a hypothetical one. (`status config`, `status device` and `apply -f` already + // learned this; this was the last caller still discarding the error.) rels := make(map[string][]ab.Resource, len(blueprintRels)) for _, rel := range blueprintRels { - rels[rel], _ = c.BlueprintRelationship(r.ID, rel) + members, relErr := c.BlueprintRelationship(r.ID, rel) + if relErr != nil { + return fmt.Errorf("reading %s membership of blueprint %q: %w", rel, r.AttrStr("name"), relErr) + } + rels[rel] = members } configs, apps, devices := rels["configurations"], rels["apps"], rels["orgDevices"] deficient, _ := r.Attr()["appLicenseDeficient"].(bool) // built-in-MDM Apps & Books signal diff --git a/internal/cli/phase1_test.go b/internal/cli/phase1_test.go index b58cefb..9a9bed3 100644 --- a/internal/cli/phase1_test.go +++ b/internal/cli/phase1_test.go @@ -636,6 +636,13 @@ type fakeTenant struct { gets []string } +// BlueprintRelationship satisfies reconcile.Applier. These tests drive the CONFIG phase, +// which makes no membership writes, so the post-apply membership check has nothing to +// verify and never calls this with a real expectation. +func (f *fakeTenant) BlueprintRelationship(_, _ string) ([]ab.Resource, error) { + return nil, nil +} + func (f *fakeTenant) CreateConfiguration(name, xml string, _ []string) (string, string, error) { id := "cfg-" + name f.stored[id] = ab.LiveConfig{Name: name, ID: id, XML: xml, Updated: "2026-07-25T12:00:00Z"} diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index de656d7..7f2671a 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -49,6 +49,12 @@ type Applier interface { CreateBlueprint(name, description string, members map[string][]string) (*ab.Resource, error) AddBlueprintMembers(bpID, rel, memberType string, ids []string) error RemoveBlueprintMembers(bpID, rel, memberType string, ids []string) error + // BlueprintRelationship re-reads one relationship so a membership write can be + // confirmed against what Apple actually holds. The project's rule — a 2xx is not + // proof — was enforced for configuration writes and nowhere for membership, which + // left `attached` reported on the strength of the response code alone. Signature is + // *ab.Client's own, so the real client keeps satisfying Applier with no adapter. + BlueprintRelationship(bpID, rel string) ([]ab.Resource, error) } // Archiver files a pre-overwrite live profile + sidecar (see internal/archive). diff --git a/internal/reconcile/apply_test.go b/internal/reconcile/apply_test.go index 668ef20..1d96599 100644 --- a/internal/reconcile/apply_test.go +++ b/internal/reconcile/apply_test.go @@ -46,6 +46,12 @@ type fakes struct { bpSpecs map[string]gitops.BlueprintSpec bpSpecErr bool // LoadBlueprints fails (unreadable/malformed manifest) bpSpecWrErr bool // WriteBlueprintSpec fails (read-only tree) + // members is the tenant's actual blueprint membership, keyed "/". + members map[string]map[string]bool + // dropMembership: Apple answers 2xx to an attach and does not apply it — the + // membership analogue of dropWrites, and what the verification pass must catch. + dropMembership bool + bpRelErr bool // the verifying re-read itself fails } func newFakes() *fakes { @@ -53,6 +59,7 @@ func newFakes() *fakes { files: map[string]string{}, stored: map[string]string{}, bpSpecs: map[string]gitops.BlueprintSpec{}, + members: map[string]map[string]bool{}, updatedTS: "ts-server", readBackTS: "ts-server", } @@ -140,7 +147,21 @@ func (f *fakes) CreateBlueprint(name, description string, members map[string][]s inline += ";" + rel + "=" + strings.Join(members[rel], ",") } f.events = append(f.events, "createbp:"+name+":"+description+inline) - return &ab.Resource{Type: "blueprints", ID: "bp-" + name}, nil + id := "bp-" + name + // Inlined members really are attached by the create, so the fake tenant must hold + // them — otherwise the post-apply membership check reads an empty relationship and + // correctly reports every inlined member as not applied. + if !f.dropMembership { + for _, rel := range rels { + if f.members[id+"/"+rel] == nil { + f.members[id+"/"+rel] = map[string]bool{} + } + for _, mid := range members[rel] { + f.members[id+"/"+rel][mid] = true + } + } + } + return &ab.Resource{Type: "blueprints", ID: id}, nil } func (f *fakes) AddBlueprintMembers(bpID, rel, memberType string, ids []string) error { @@ -149,6 +170,14 @@ func (f *fakes) AddBlueprintMembers(bpID, rel, memberType string, ids []string) } f.events = append(f.events, "attach:"+bpID+":"+strings.Join(ids, ",")) f.relOps = append(f.relOps, "POST:"+rel+":"+memberType) + if !f.dropMembership { + if f.members[bpID+"/"+rel] == nil { + f.members[bpID+"/"+rel] = map[string]bool{} + } + for _, id := range ids { + f.members[bpID+"/"+rel][id] = true + } + } return nil } @@ -158,9 +187,27 @@ func (f *fakes) RemoveBlueprintMembers(bpID, rel, memberType string, ids []strin } f.events = append(f.events, "detach:"+bpID+":"+strings.Join(ids, ",")) f.relOps = append(f.relOps, "DELETE:"+rel+":"+memberType) + for _, id := range ids { + delete(f.members[bpID+"/"+rel], id) + } return nil } +// BlueprintRelationship is what the post-apply membership check reads. It reports the +// fake tenant's ACTUAL membership, so `dropMembership` can model Apple accepting a write +// and not applying it — the membership analogue of dropWrites. +func (f *fakes) BlueprintRelationship(bpID, rel string) ([]ab.Resource, error) { + if f.bpRelErr { + return nil, errors.New("relationship read boom") + } + f.events = append(f.events, "relread:"+bpID+"/"+rel) + out := []ab.Resource{} + for id := range f.members[bpID+"/"+rel] { + out = append(out, ab.Resource{ID: id}) + } + return out, nil +} + func (f *fakes) LoadBlueprints() (map[string]gitops.BlueprintSpec, error) { if f.bpSpecErr { return nil, errors.New("load blueprints boom") diff --git a/internal/reconcile/blueprint.go b/internal/reconcile/blueprint.go index af1e6eb..c1007bb 100644 --- a/internal/reconcile/blueprint.go +++ b/internal/reconcile/blueprint.go @@ -364,6 +364,8 @@ func (e *Engine) ApplyBlueprints(p *BlueprintPlan, opts Opts, priorWrites int) * res := &BlueprintResult{Outcomes: []BlueprintOutcome{}} createdIDs := map[string]string{} // blueprint name → id created this run + // What each membership write claimed, checked in one pass at the end. + var claims []membershipClaim // Members inlined into each blueprint-new create. Apple rejects a member-less // create (409 MISSING_MEMBERS / MISSING_RESOURCES — live-verified 2026-07-05, @@ -445,6 +447,9 @@ func (e *Engine) ApplyBlueprints(p *BlueprintPlan, opts Opts, priorWrites int) * // separate write (relationships POST merges, so re-attaching would be a // harmless but rate-limited no-op). e.bpDone(res, it, "attached "+it.Config+" (inlined in the blueprint create)") + // Reported done on the strength of the create's 2xx, so it is exactly the + // kind of claim the verification pass below exists to check. + claims = append(claims, membershipClaim{outcome: len(res.Outcomes), bpID: bpID, rel: rel, memberID: it.ConfigID, present: true}) continue } if !e.budget(opts, priorWrites+res.Writes) { @@ -458,6 +463,7 @@ func (e *Engine) ApplyBlueprints(p *BlueprintPlan, opts Opts, priorWrites int) * } res.Writes++ e.bpDone(res, it, "attached "+it.Config) + claims = append(claims, membershipClaim{outcome: len(res.Outcomes), bpID: bpID, rel: rel, memberID: it.ConfigID, present: true}) case it.Action.IsAdopt(): // A LOCAL write: it changes gitops/blueprints/.yml, never the tenant. // So it spends no rate limit, is not counted in res.Writes, and is not @@ -494,13 +500,97 @@ func (e *Engine) ApplyBlueprints(p *BlueprintPlan, opts Opts, priorWrites int) * } res.Writes++ e.bpDone(res, it, "detached "+it.Config) + claims = append(claims, membershipClaim{outcome: len(res.Outcomes), bpID: it.BPID, rel: rel, memberID: it.ConfigID, present: false}) default: // BlueprintGone — reported, not applied e.bpSkip(res, it, it.Detail) } } + e.verifyMembership(res, claims, opts) return res } +// membershipClaim is one membership write and what it asserted about the tenant. +// `outcome` is the 1-based position of the row in res.Outcomes, so a failed check can +// downgrade the exact row that made the claim. +type membershipClaim struct { + // outcome is the 1-based index of the row this claim belongs to — captured AFTER the + // row is appended, so `res.Outcomes[outcome-1]` is that row and not the one before it. + outcome int + bpID string + rel string + memberID string + present bool // true for an attach, false for a detach +} + +// verifyMembership re-reads the relationships that were written and confirms the tenant +// actually holds what each write claimed. +// +// The project's rule is that a 2xx is not proof — Apple acknowledges a configuration +// write and then silently declines to store it, which is why every config write reads +// back. Membership writes had no such check: `attached` was reported on the response +// code alone, and a member inlined into a blueprint create was reported attached purely +// because the create returned 2xx. +// +// The cost is deliberately per RELATIONSHIP, not per member: attaching twenty configs to +// one blueprint is one extra GET, not twenty. Apple rate-limits hard, and a verification +// that scales with the number of writes would discourage its own use. +// +// A read that FAILS downgrades the row to "not verified" rather than failing it — a GET +// that errored says nothing about the POST, the same posture the config read-back takes. +func (e *Engine) verifyMembership(res *BlueprintResult, claims []membershipClaim, opts Opts) { + if len(claims) == 0 { + return + } + type relKey struct{ bpID, rel string } + live := map[relKey]map[string]bool{} + unreadable := map[relKey]string{} + + for _, c := range claims { + key := relKey{c.bpID, c.rel} + if _, done := live[key]; done { + continue + } + if _, failed := unreadable[key]; failed { + continue + } + progress(opts, "verifying blueprint membership: "+c.rel) + members, err := e.Client.BlueprintRelationship(c.bpID, c.rel) + if err != nil { + unreadable[key] = err.Error() + continue + } + set := make(map[string]bool, len(members)) + for _, m := range members { + set[m.ID] = true + } + live[key] = set + } + + for _, c := range claims { + if c.outcome <= 0 || c.outcome > len(res.Outcomes) { + continue + } + row := &res.Outcomes[c.outcome-1] + key := relKey{c.bpID, c.rel} + if reason, failed := unreadable[key]; failed { + row.Detail += " — NOT VERIFIED (re-read failed: " + reason + ")" + continue + } + if live[key][c.memberID] == c.present { + continue // the tenant holds what the write claimed + } + verb := "attach" + if !c.present { + verb = "detach" + } + row.Status = "error" + row.Detail += " — but Apple does not reflect it: the " + verb + + " was accepted (2xx) and the membership did not change. Re-run sync; if it repeats, " + + "the member or the blueprint may have been changed in the console mid-run." + res.Errors++ + } +} + // adoptMember records one live member in its blueprint manifest. The existing // manifest is loaded and only this collection's list GAINS the name — see // gitops.BlueprintSpec.WithMember for why the write is additive rather than a diff --git a/internal/reconcile/blueprint_test.go b/internal/reconcile/blueprint_test.go index 609d037..4c9bac9 100644 --- a/internal/reconcile/blueprint_test.go +++ b/internal/reconcile/blueprint_test.go @@ -433,8 +433,11 @@ func TestApplyBlueprintsCreateThenAttach(t *testing.T) { t.Fatalf("create-with-inline = errors %d writes %d skipped %d, want 0/1/0: %+v", res.Errors, res.Writes, res.Skipped, res.Outcomes) } wantCreate := "createbp:NewBP:fresh from git;configurations=c-wifi;orgDevices=d-aaa" - if len(f.events) != 1 || f.events[0] != wantCreate { - t.Errorf("events = %v, want exactly [%s]", f.events, wantCreate) + // WRITES only: the post-apply membership check also reads each touched relationship + // back (`relread:…`), which is what confirms the inlined members really landed rather + // than trusting the create's 2xx. Reads spend no --limit-writes budget. + if got := writeEvents(f.events); len(got) != 1 || got[0] != wantCreate { + t.Errorf("write events = %v, want exactly [%s]", got, wantCreate) } if len(f.relOps) != 0 { t.Errorf("inlined members must not also POST relationships: %v", f.relOps) @@ -452,8 +455,8 @@ func TestApplyBlueprintsCreateThenAttach(t *testing.T) { if res.Writes != 1 || res.Skipped != 0 || res.Errors != 0 { t.Errorf("budget=1 → writes=%d skipped=%d errors=%d, want 1/0/0", res.Writes, res.Skipped, res.Errors) } - if indexOf(f.events, wantCreate) != 0 || len(f.events) != 1 { - t.Errorf("budget=1 must spend the only write on the inlining create: %v", f.events) + if got := writeEvents(f.events); len(got) != 1 || got[0] != wantCreate { + t.Errorf("budget=1 must spend the only write on the inlining create: %v", got) } // Budget already exhausted by phase 1 → the create skips, and the attaches @@ -661,3 +664,93 @@ func TestApplyBlueprintsAdoptUnmanagedCollection(t *testing.T) { t.Errorf("an unmanaged collection must stay unmanaged, got %v", *f.bpSpecs["Sales"].Apps) } } + +// A membership write Apple ACKNOWLEDGES but does not apply must not be reported as done. +// +// The project's rule is that a 2xx is not proof — every configuration write reads back +// because Apple accepts an out-of-spec profile and silently drops it. Membership writes had +// no such check: "attached" was reported on the response code alone. +func TestApplyBlueprintsCatchesAMembershipWriteAppleDidNotApply(t *testing.T) { + f := newFakes() + f.dropMembership = true // 2xx, membership unchanged + plan := &BlueprintPlan{Items: []BlueprintItem{ + {Blueprint: "Sales", BPID: "bp-sales", Action: Attach, Collection: ab.CollectionConfigurations, + Config: "wifi.mobileconfig", ConfigID: "c-wifi"}, + }} + res := engineWith(f).ApplyBlueprints(plan, Opts{}, 0) + if res.Errors != 1 { + t.Fatalf("errors = %d, want 1 — an unapplied attach must not read as done: %+v", res.Errors, res.Outcomes) + } + if res.Outcomes[0].Status != "error" { + t.Errorf("status = %q, want error", res.Outcomes[0].Status) + } + if !strings.Contains(res.Outcomes[0].Detail, "does not reflect it") { + t.Errorf("detail should say the tenant disagrees, got: %q", res.Outcomes[0].Detail) + } +} + +// The check is per RELATIONSHIP, not per member: attaching many configs to one blueprint +// costs ONE extra read. A verification whose cost scales with the number of writes would +// discourage its own use on a rate-limited API. +func TestMembershipVerificationCostsOneReadPerRelationship(t *testing.T) { + f := newFakes() + items := []BlueprintItem{} + for _, name := range []string{"a", "b", "c", "d", "e"} { + items = append(items, BlueprintItem{ + Blueprint: "Sales", BPID: "bp-sales", Action: Attach, + Collection: ab.CollectionConfigurations, Config: name, ConfigID: "c-" + name, + }) + } + res := engineWith(f).ApplyBlueprints(&BlueprintPlan{Items: items}, Opts{}, 0) + if res.Errors != 0 { + t.Fatalf("errors = %d, want 0: %+v", res.Errors, res.Outcomes) + } + if res.Writes != 5 { + t.Errorf("writes = %d, want 5", res.Writes) + } + // One relationship touched → exactly one verifying read, regardless of member count. + reads := 0 + for _, e := range f.events { + if strings.HasPrefix(e, "relread:") { + reads++ + } + } + if reads > 1 { + t.Errorf("verification made %d reads for one relationship; it must batch", reads) + } +} + +// A verifying read that FAILS is "not verified", never "failed": a GET that errored says +// nothing about the POST. Same posture the configuration read-back takes. +func TestMembershipVerificationReadFailureIsNotAWriteFailure(t *testing.T) { + f := newFakes() + f.bpRelErr = true + plan := &BlueprintPlan{Items: []BlueprintItem{ + {Blueprint: "Sales", BPID: "bp-sales", Action: Attach, Collection: ab.CollectionConfigurations, + Config: "wifi.mobileconfig", ConfigID: "c-wifi"}, + }} + res := engineWith(f).ApplyBlueprints(plan, Opts{}, 0) + if res.Errors != 0 { + t.Errorf("a failed re-read must not fail the write: errors = %d", res.Errors) + } + if res.Outcomes[0].Status != "done" { + t.Errorf("status = %q, want done", res.Outcomes[0].Status) + } + if !strings.Contains(res.Outcomes[0].Detail, "NOT VERIFIED") { + t.Errorf("detail should say it could not be verified, got: %q", res.Outcomes[0].Detail) + } +} + +// writeEvents filters the fake's event log down to tenant WRITES, dropping the +// post-apply verifying reads. Tests that assert "this run made exactly one write" are +// about rate limit and --limit-writes, and a read must not disturb them. +func writeEvents(events []string) []string { + out := []string{} + for _, e := range events { + if strings.HasPrefix(e, "relread:") { + continue + } + out = append(out, e) + } + return out +}