Skip to content

fix: prove writes landed, stop tenant-wide fan-out, and give each run its own state - #11

Merged
TheJakeBraun merged 3 commits into
mainfrom
fix/audit-findings
Aug 16, 2026
Merged

TheJakeBraun merged 3 commits into
mainfrom
fix/audit-findings

Conversation

@TheJakeBraun

Copy link
Copy Markdown
Contributor

Two audits (Go + Swift), then an adversarial workflow review of the resulting diff. Everything here is a defect that reached a release — and 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 carried both defects the reconcile engine was fixed for in v0.4.19. 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, 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.

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, with a per-config GET fallback 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.

apply -f resolved names once per document; the index is now cached per command and appended to on create. 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 / apply -f swallowed relationship errors: a rate-limited tenant printed "attached to 0 blueprint(s)" and exited 0.
  • apply -f warned and exited 0 when the manifest write failed — the defect attach was already fixed for.
  • abctl attach rewrote a manifest wholesale from live, discarding configs declared in git but not yet attached. Now unions; a detach excludes only the member it removed.
  • sync --apply --refresh=metadata-only refused up front; gitops.WriteConfig won't let a tenant 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 first fix 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; the plan owns isPlanning rather than the shared isLoading.

Also: scoped lastWriteError (a failed config delete no longer paints red across the Diff plan and Archive table); ArchiveView's bare TupleView wrapped; ApplyResult decodes abctl's structured verification key instead of grepping stderr for FAILED; ProcessRunner writes stdin inside the cancellation/watchdog scope and reports a partial write; context-store failures surfaced; Devices 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 drive real commands, including the two that would have caught this session's bugs as test failures rather than bug reports:

TestReplaceFailsWhenAppleDropsTheWrite            2xx + unchanged bytes must fail
TestReplaceDoesNotDownloadEveryProfileInTheTenant zero tenant-wide XML requests

Verification

Go: build, vet, gofmt, full suite green. Swift: macOS CI is the gate.

🤖 Generated with Claude Code

TheJakeBraun and others added 3 commits August 15, 2026 18:10
… its own state

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 <noreply@anthropic.com>
…switch made dead

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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@TheJakeBraun
TheJakeBraun merged commit 715385f into main Aug 16, 2026
6 checks passed
@TheJakeBraun
TheJakeBraun deleted the fix/audit-findings branch August 16, 2026 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant