Retire unsound NQE removal; consolidate mutations behind one guarded gateway - #7
Merged
Merged
Conversation
End-to-end architecture review of the reconcile/apply paths, prompted by the recent run of reactive patches around deletions and edge cases. Core finding: there is no single desired-state computation and no single guarded mutation chokepoint. Six independent writers reach the setup account list, and safeguards are enforced per call site rather than at the mutation boundary. Two CRITICAL findings: - A truncated but nonempty NQE result is indistinguishable from a complete one. Zero rows are rejected, but a single surviving row under --prune-missing means "remove everything else". There is no completeness token, expected count, or explicit deprovisioning event in the input model. Network 253234 currently holds 978 accounts against a PageLimit of 1000. - The account list is replaced by a full-list PATCH with no client-visible version token. Forward's server-side kvStore.getAndUpdate retry loop re-applies the client's absolute intent onto freshly read state, so a concurrent edit is clobbered deterministically, not racily. Findings were validated against the live Forward API (read-only) and the Forward server source. Two hedged findings are resolved: assumeRoleInfos is confirmed full-list replacement, and the suspected loss of unmodeled fields is disproven — PATCH is a top-level tri-state merge, so omitted keys are preserved. Phase 0 adds 21 characterization tests pinning the unsafe behaviors: the final GET/PATCH race, incomplete inventory, partial multi-setup apply, the apply-plan disable bypass, ambiguous PATCH retry, and webhook loss/ordering/scope. Each is fully written but guarded behind a runP0*FailureTests constant so the suite stays green; flipping a guard reproduces the finding it cites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 1 of the refactor plan in docs/ARCHITECTURE_REVIEW.md. Planning consumed raw map[string]any rows directly, so identity rules were re-derived per call site and disagreed with each other. Adds AccountID, SetupID, Partition, RoleARN, AccountLifecycle, and InventorySnapshot, plus adapters converting NQE rows, manifests, the External ID CSV, and API responses into them. Raw maps no longer cross the adapter boundary. InventorySnapshot carries a completeness marker that the NQE adapter sets to unknown, because NQE cannot currently prove its result is complete. That unknown state is the point: phase 3 consumes it to block absence-based removal rather than inferring deletion from a possibly truncated inventory. Behavior changes: - Account IDs are now exactly 12 digits everywhere. NQE previously accepted 1-12 while manifests and External ID required 12. - Malformed rows now fail the plan instead of being skipped. Silently dropping rows is how a partial inventory turns into a deletion, so this fails closed on purpose — but one bad NQE row now blocks the whole sync, which on a large setup is a full outage. Recorded in the review document. - Duplicate accounts across rows, duplicate setup IDs, and accountId / roleArn disagreement are rejected instead of resolved first-wins. Forward rejects duplicate IDs server-side too, but the old client dropped the conflict before it ever got there, so the operator saw the wrong account survive rather than an error. Existing test changes are account-ID literal widening only; no assertions were weakened. Phase 0 guards remain disabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 2a. The reconciler treated any nonempty NQE result as a complete picture of the organization, so a truncated result meant "remove everything missing from it". Empty results were already rejected; a partial one was not. Truncation is invisible at exactly the wrong moment. The paginator stops on the first short page, so a result that is an exact multiple of PageLimit is indistinguishable from a complete one. Production network 253234 currently holds 978 accounts against a PageLimit of 1000. InventorySnapshot completeness is now derived from real pagination metadata and marked unproven when the result count is an exact multiple of PageLimit, when a page repeats, or when the cursor fails to advance. buildPlanFromSnapshot refuses to emit removals when completeness is unproven, regardless of policy flags or removal ceilings. The guard sits at the single planning entry, so no caller can route around it. Adds and re-enables are unaffected. Also adds --allow-malformed-rows, an opt-in escape hatch for the fail-closed parsing introduced in the previous commit, so one bad row cannot block a 978-account sync. Skipped rows are reported in both the JSON summary and human output. Using the hatch marks completeness unproven, which blocks removal by the rule above: the hatch lets an operator keep adding despite a bad row, never prune against an inventory known to be partial. This covers detectable truncation only. A query whose result is short relative to AWS reality is indistinguishable from a small organization, and no client-side check can close that gap. Unattended pruning remains unsafe and is a policy question, not a code one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 2b. Planning intent was encoded in interacting booleans (PruneMissing, AuthoritativeInput, AllowNoOrgEvidence) evaluated inline against raw setup state, so "what should this setup look like" and "is this change allowed" were computed in the same pass and each caller re-derived both. Adds ComputeDesired(CurrentSetup, InventorySnapshot, ReconcilePolicy), which is pure: reconcile.go imports only fmt, reflect, sort, and strings, and has no clock or I/O fallback. The planning instant is a required input, so a zero TestInstant no longer becomes time.Now() and preview and apply digests are stable for identical inputs. The clock is injected at the CLI boundary instead. ReconcilePolicy replaces the booleans with a tagged kind: Additive, CompleteInventory, or ExplicitOperations. Existing flags map onto it at the boundary and keep working. An unset organization-evidence tag defaults fail-closed. The completeness invariant from the previous commit survives the translation and is enforced inside the engine, so CompleteInventory still cannot emit a removal against an inventory that is not provably complete. ChangeSet classifies Add, Enable, Disable, Remove, Rename, RotateExternalID, ChangeRole, and setup-metadata changes, each carrying typed before/after state. Disable is now a distinct destructive change rather than a silent field write, which is what lets phase 3 charge it to the same authorization budget as Remove. Root NQE, safe-sync, webhook, and manifest planning all route through the engine. apply-plan and the External ID writer still have their own writers; rerouting those is phase 3. Behavior changes: - Cross-setup moves now fail closed. An account moving between two selected setups is refused rather than emitting sequential PATCHes. This also corrects a review invariant that was too strong: unique pre- and post-ownership cannot guarantee an account ends in exactly one setup, because sequential PATCHes necessarily pass through a state where it is in both or neither. Permitting moves safely needs the atomicity work in phases 3 and 5. - Accounts in generated payloads are sorted by account ID, so audit file ordering and its digest change once. - An empty ChangeSet produces no payload and no PATCH; the summary reports the setup as unpatched. Also fixes a defective phase 0 fixture. Every row in the incomplete inventory test used the same account ID, so a full page tripped duplicate validation before reaching the completeness guard, and its "truncated later page" case was really a normal complete two-page fetch. Rewritten with distinct IDs against the conditions that are actually detectable; it now passes with its guard flipped. The undetectable case is recorded as an explicitly skipped test rather than faked green: a short first page cannot be distinguished from a genuinely small organization without an independent expected count, so that one rests on policy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 3a. The safeguards were real but enforced at each call site, so whether a mutation was checked depended on which path reached it. That is the shape the review identified as the root cause: not missing guards, but guards that live beside the callers instead of at the boundary. Adds ApplyIntent, ApplyAuthorization, and GuardAndApply. Root NQE, safe-sync, webhook, and manifest sync now mutate only through it. Eight checks move into the gateway: - Empty ChangeSets never reach the network, for every caller. - Remove and Disable are both destructive and share one authorization and one budget. Disable being a silent field write is what made the apply-plan bypass possible; classifying it centrally is what closes it when phase 3b reroutes that writer. - Absence-based removal still requires proven inventory completeness. - Removal ceilings and destructive authorization are enforced once. - GovCloud and organization-evidence rules are policy, not CLI conditionals. The GovCloud partition is read from the baseline so an empty target cannot hide it. - The approval digest binds baseline state, source snapshot and its completeness, policy, and target payload — not just payload bytes. - Rollback, audit, and a per-setup result journal (planned / pending / applied / conflicted / failed) are gateway-owned, so a partial multi-setup apply reports disposition instead of a bare error. - The pre-PATCH re-read is retained and documented as a weak mitigation. It is not CAS. Forward exposes no ETag or version field, and its internal kvStore.getAndUpdate loop re-applies absolute client intent onto freshly read state, so a concurrent edit is clobbered deterministically rather than racily. patch_chokepoint_test.go asserts PatchCloudAccount has no production callers outside the gateway and the two phase 3b holdouts, and that the gateway calls it exactly once. Verified by injecting a violation: the test names the offending file and fails. Because compare-and-swap is impossible, destructive changes are now refused in unattended contexts unless --allow-unattended-destructive is given. This newly refuses: awssync --apply --yes --prune-missing ... (when removals planned) awssync sync-accounts --apply --yes ... (when omissions remove) serve-webhook --apply --yes ... (destructive jobs) --yes counts as unattended even in a terminal. safe-sync is unaffected, being additive-only. The override does not bypass --allow-removals, evidence rules, or either ceiling. Also corrects the review: the executor already suppressed zero-diff applies as of phase 2b. What was actually missing was that suppression inside a durable guarded path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 3b, completing phase 3. The last two writers with their own apply paths now go through GuardAndApply, and api.PatchCloudAccount has exactly one production caller. patch_chokepoint_test.go drops both holdout exemptions and enforces that. apply-plan classified danger only by missing account IDs, so a reviewed payload could keep every account and set every one to enabled:false while tripping neither --allow-removals nor either ceiling. Routing it through the gateway closes that without a dedicated fix, because phase 3a already made Disable destructive and budget-consuming. Its JSON format is unchanged — payloads are adapted to typed current/desired state and classified by the engine — so existing files keep working. Invalid account IDs and role ARNs are now rejected by typed validation. External ID rotation was an independent full-list read/modify/PATCH with no rollback artifact and no pre-PATCH re-read, confirming before the target payload was computed. It now gets rollback, audit, result journal, the weak re-read, and digest-bound approval, and confirms after the target and digest exist, so a no-change rotation no longer prompts. Its PATCH now carries the full preserved setup payload rather than type and assumeRoleInfos alone; the operator-facing payload file is unchanged. Behavior changes: - apply-plan removals or disables require --allow-unattended-destructive, since apply-plan always implies --yes. Disables additionally require --allow-removals and consume both ceilings. Non-destructive plans are unaffected. - External ID rotations classify only as RotateExternalID and never require the unattended flag. - Zero-diff apply-plan files patch nothing and report zero setups. Phase 0: TestP0ApplyPlanDisableRequiresDestructiveAuthorization now passes on all three subtests. The GET/PATCH race still fails on all three writers and should — Forward exposes no compare-and-swap token, so the re-read remains a weak mitigation. The external_ID race fixture moves its injection past the newly added re-read, so it still targets the real window instead of passing on the strength of a check that does not close the gap. Review §5 is now stale on both bypasses, on apply-plan zero-diff behavior, on External ID rollback and re-read, and on its "not a true chokepoint" conclusion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
The webhook server could apply changes unauthenticated, could be directed at networks it was never configured for, silently dropped events it had already marked seen, and had no ordering guarantee. Each is independently sufficient to lose or misdirect a destructive reconciliation. - Authentication is mandatory when apply is enabled. The server refuses to start without both Basic Auth values, and authorized() rejects unauthenticated requests rather than returning true on empty config. Non-apply servers may still run open. - Event scope is intersected with configured scope instead of replacing it. Apply mode requires an explicit --network-id, event networks must match it, and event setup IDs must be a subset of configured ones. Expansion attempts get 403. - Dedupe is recorded only after a successful run, not before queue admission. A full queue returns 503 with no dedupe record, and a failed run stays redeliverable, so neither is silently swallowed as a duplicate. - Dedupe keys cover event type, network, snapshot, sorted setup IDs, and event ID, so two different scopes sharing an ID no longer collide. Successful keys persist for 24 hours across restart. - Snapshot chronology is enforced per network/setup by watermark, checked both at admission and immediately before execution. An older snapshot arriving late returns 409 instead of reconciling over a newer one. - Explicit snapshot IDs are resolved and checked against MaxSnapshotAge rather than skipping freshness entirely. Durable state lives in $UserConfigDir/awssync/webhook-state.json, written atomically at 0600 with a versioned schema, overridable with --webhook-state-file. It follows the file-based approach already used for the phase 3a result journal; no new dependency. BREAKING: serve-webhook --apply now fails at startup without --webhook-basic-username, --webhook-basic-password, and --network-id. Operators must set those, configure Forward to send matching credentials, and ensure the service user can write the state directory. Known gap, now documented rather than implied: the accepted-job queue is still in memory, so a crash between admission and completion loses queued work. That is the remaining phase 5 item. Also refreshes docs/ARCHITECTURE_REVIEW.md for phases 1-3. Fixed findings now say so and cite where, rather than disappearing: a review that silently drops what got solved reads later as though it was never found. Phases 0-3 are marked done, phase 4 closed by the no-CAS finding with the --allow-unattended-destructive mitigation, phase 5 partial. The unsolved items are explicitly retained: no CAS, future-dated snapshots, ambiguous PATCH retries, no crash-recoverable queue, monitor consistency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Both safety changes from this branch fail closed, so a deployment that misses them stops rather than degrades. The release template had no breaking-changes section, so they would have been discovered by a failed upgrade. Adds that section above Highlights, covering webhook authentication and network scoping, and --allow-unattended-destructive for removals and disables without a human present, each with the command form an operator needs. Also records the limitation that motivates the second one: Forward exposes no compare-and-swap token, so a concurrent UI edit is overwritten deterministically, not racily. Operators cannot weigh unattended pruning without knowing that, and it is not inferable from the flag name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
--prune-missing deleted accounts that were absent from an NQE result, on the assumption that the result enumerated the organization. It does not. The query returns the snapshot's observed cloud inventory: the union of accounts that successfully collected and accounts merely visible through organizations:ListAccounts metadata. It is not sourced from configured accounts and carries no completeness contract. Collector authorization failures are ignored and service exceptions return the partial accumulated list, so partial results are routine rather than exceptional. So absence conflated five different things — genuinely deleted from the org, collection failed this snapshot, auth failure, belongs to another org, transient error — and treated all of them as delete. Measured against production before making this change: network 253234: 978 configured, 10 NQE rows -> 968 deletions network 253236: 565 configured, 540 NQE rows -> 27 deletions All 27 on the second network were live and enabled, so this was not one misconfigured network. A 96%-complete inventory still destroys real accounts. The flag stays recognized and refuses before credentials, NQE, planning, or PATCH, so existing automation gets an actionable error rather than silently doing nothing. Refusal is enforced at three layers: the CLI boundary, ComputeDesired rejecting CompleteInventory for an NQE-sourced snapshot, and the gateway rejecting any removal from an NQE snapshot regardless of policy. Reviewed-manifest removal via sync-accounts is unaffected and remains the supported path. CompleteInventory is kept because that path is a legitimate caller — a human-approved manifest genuinely is complete — but the boolean mapping that let NQE select it is gone. The pagination completeness work from 8cf4ef9 stays. It correctly proves a result was not truncated in transport, which is worth having for diagnostics. It never could have made this safe: it constrains the transport, not the meaning of the data. Characterization tests whose premise no longer exists are skipped with a reason and their bodies intact rather than deleted, so the record of what was once true survives. This is the root cause behind the deletion problems that prompted the review. The guards added in earlier phases narrowed the blast radius but could not fix an input that never meant what the design assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Three defects found by running this branch against live Forward networks. The approval digest hashed ReconcilePolicy, which carries the planning instant, plus region testInstant values. Two identical plans therefore produced different digests in separate processes. Preview and apply share an instant within one process, so tests and safe-sync passed and the bug was invisible to the suite. That broke the workflow the digest exists for: review a plan, then apply it. Worse than a missing check, since a digest that changes when nothing an approver would recognize has changed teaches people to click through mismatches. Digest v2 covers network, snapshot, reconciliation semantics, baseline and target configuration, and classified change counts. It excludes the planning instant and zeroes region timestamps, which are execution metadata rather than anything an approver is agreeing to. The exact generated bytes are still bound separately by payload_sha256. The policy portion is now an explicit allow-list struct, so a volatile field added to ReconcilePolicy later cannot silently re-enter approval identity. Existing v1 digests change once. Also: - A destructive dry-run reported the removals and exited 0 with no sign that apply would refuse, because the unattended-destructive gate sits past the dry-run return. Preview now reports removal_blocked and names the flag, in both output formats, without failing the preview. - Completeness blocks caused by --allow-malformed-rows cited "a terminating short page", because the skipped-row reason was only recorded when no pagination reason was already set. It now names the actual cause. - The blocked-apply hint still advertised flags from the retired NQE prune path and omitted --allow-unattended-destructive. The digest regression test spawns two independent processes with different injected instants and asserts the digests match while the payload hashes differ, so it cannot pass by accident. An in-process test would have passed with the bug present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Phase 5, the last substantive item from the architecture review. The accepted-job queue was an in-memory channel, so a crash between admission and completion lost queued work silently — the sender had already been given a 202 and would not retry. Events are now persisted before the handler acknowledges them, and a persistence failure returns 503 rather than a false 202. Pending work is recovered and requeued on startup. Failures retry with exponential backoff, bounded at five attempts, after which an event becomes a visible dead letter instead of spinning forever. /healthz exposes pending and dead-letter depth. State schema is v2, extending the existing atomically written 0600 file with pending_events and dead_letter_events. A v1 file is upgraded in place with its dedupe records and watermarks preserved, so upgrading does not hit a corrupt-state startup failure. Unknown versions still fail closed. Replay safety was checked rather than assumed. Since NQE-based removal was retired, webhook reconciliation is additive: a replayed job can add or re-enable but cannot delete or disable. It is not free of effects — it can re-enable a deliberately disabled account and remains exposed to the no-CAS race — so this is at-least-once, not exactly-once, and that is now stated rather than implied. Fixes a regression this work introduced. A cleanly completed event could be reprocessed after an orderly restart, because there was no durable completion boundary: the test callback signalled execution rather than completion, cancellation did not join the worker, and a replacement server could reload a still-in-flight job and rewrite it as queued while the original worker was entering recordSuccess. Two server instances with separate mutexes then raced. The worker now signals completion only after its success, retry, or dead-letter transition is durable, and Run joins the worker before returning. The at-least-once tradeoff is meant to cover a crash between PATCH and state persistence, which is unavoidable; it must not cover a clean completion followed by an orderly restart, which operators will reasonably assume is safe. The failure was intermittent — 5 of 10 repeated runs — and invisible to a single pass, so concurrency changes here are now verified with -count=10 under -race. Remaining by design: a crash after an external PATCH but before success state is persisted may replay, loss of the state file loses recovery metadata, and two daemons must not share one state file as there is no interprocess locking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
The chokepoint test and the phase 0 guards only protect anything if CI runs them. A contributor adding a second PatchCloudAccount caller, or flipping a guard to "fix" the skipped tests, would otherwise turn a documented limitation into a green build. Adds explicit vet and gofmt gates, a guard-state check that fails if any runP0*FailureTests constant is not literally false, and race-detector runs. The webhook package runs with -count=10 under -race on every PR, not once. Two concurrency bugs on this branch were invisible to single-pass runs: one needed -count=3 to surface, and the other failed 5 of 10 repeated runs while passing 5/5 in isolation. A single green run is not evidence about this code. It costs about 12 seconds locally, which is cheap next to shipping a lost webhook event. The guard step was verified by flipping a guard and confirming it exits non-zero with a file annotation naming the offending constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Last open items from the architecture review, all read-only paths. - Wait recognized only FAILED and ARCHIVED as terminal, case-sensitively, so an unknown or differently-cased state polled forever. States are now normalized, and an unrecognized state is reported as such rather than silently waiting or being assumed terminal. - Wait polled indefinitely for a snapshot that does not exist, making a typo'd ID indistinguishable from one still processing. It now fails fast after the first complete listing. - Snapshot listing was unpaginated, so a snapshot past the first page was invisible. It now follows pages and fails closed on repeats or oversized responses instead of looping or silently truncating. - MaxSnapshotAge only checked whether age exceeded the maximum, so a future-dated snapshot passed a check that exists to prevent stale reconciliation. Future timestamps are now rejected as bad data rather than clamped, since a timestamp ahead of now means clock skew or corruption, and accepting it defeats the check. Rejection allows 5 minutes of tolerance. Forward and the client are different machines and ordinary NTP drift must not fail a run; zero tolerance would have failed every sync against a server whose clock ran a second fast. - Status fetched "latest" and the snapshot list separately and presented them as one observation. The API cannot provide an atomic read, so rather than papering over it, Status now reports observation_atomic, latest/list consistency, and a warning. This is the third place the review concluded an honest limitation beats a fabricated guarantee, after the absent CAS token and the absent organization provenance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Coverage of internal/app was 71.7% overall, but the newly added mutation path's error handling was the least covered code in it. failPendingApply — the code that records per-setup disposition when an apply goes wrong — was at 0%. Untested error handling in the failure path of a destructive operation is the worst place for a blind spot, because it only runs once something has already gone wrong. Removes the ExplicitOperations policy kind and both applyExplicit* functions. It had no production caller: the only references used the kind as digest metadata and never invoked the reconciler. A policy kind nobody uses is one a future caller adopts without understanding its guarantees, and this one could express removals directly — the same dead-policy risk that CompleteInventory raised during the NQE retirement. It can come back with tests when there is a real caller. External ID rotation now declares Additive, which is honest since it cannot change membership. Reviewed apply-plan payloads use CompleteInventory with completeness explicitly recorded. failPendingApply 0% -> 100%, validateDestructiveEvidence 60% -> 100%, Digest 66.7% -> 100%, package 71.7% -> 73.4%. The new digest test covers sensitivity rather than determinism: plans targeting different accounts must produce different digests. Determinism was already proven by the cross-process test, but an over-normalized digest that never changed would have passed that and every other existing test. The partial-journal test now asserts full histories: an applied setup stays applied, the failing setup becomes failed, and an untouched setup stays pending. Operator-visible: apply-plan and external-id approval digests change once because policy metadata changed, so a digest approved with a prior binary will not match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
…uide The procedure doc was written when the tool had --prune-missing and had been patched incrementally without being re-read as a whole. Steps implied NQE could authorize deletion, required flags were missing from examples, and the closing summary still claimed Organizations/NQE was authoritative for all changes. Reworked as a runbook: an index for finding the right section under pressure, copyable commands carrying every required safety flag, and recovery guidance covering rollback artifacts, the result journal, partial and ambiguous applies, and dead-lettered webhook events. About 155 lines shorter — repeated command variants, the exhaustive field catalog, and the inline External ID tutorial were cut rather than annotated. A 41KB document nobody can navigate at 3am is its own failure mode. Adds docs/upgrading.md, since three things break on upgrade and there was no migration path. It covers replacing --prune-missing automation, reconfiguring webhook receivers for mandatory auth and explicit network scope, and reviewing unattended destructive automation. The manifest-building section is the important part. It tells operators to start from the configured baseline and reconcile against sources that actually own account lifecycle, to run ListAccounts against every relevant Organization, and to keep any account whose lifecycle cannot be confirmed. It states plainly that NQE output, a failed collection, a missing IAM role, and Collected? false are not evidence an account should be removed. That is the finding that motivated retiring prune, and the manifest is where an operator could otherwise reintroduce it by hand. The unattended-destructive section explains the absent compare-and-swap token rather than just naming the flag. An operator who does not know why it exists will add it to silence the error, which defeats it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Format compatibility across this branch was asserted but never demonstrated. Operators upgrading will feed the new binary files written by the old one, and a break would surface during an incident. Builds the merge-base binary (0c0dbd5) and uses it to emit genuine artifacts against a local fake, rather than hand-writing approximations of the old formats — approximating the old format tests our belief about it, which is how this class of bug survives. Verified against the current binary: an old apply-plan payload is accepted with its SHA-256 preserved and patches the expected setup; an old .rollback.json is accepted as an apply-plan payload and correctly restores prior disabled state; the External ID CSV the old binary accepted produces an identical payload hash. No incompatibility found. Fixtures are checked in under internal/app/testdata with their provenance and original hashes recorded, so a future change that breaks the old format fails against the real thing. No pre-branch webhook state file could be produced, because the old binary had no durable webhook state at all — that feature was added on this branch. Rather than fabricate a fixture, this is recorded as untested: the v1 schema was never released, so the v1-to-v2 migration only matters for someone running an intermediate commit of this branch. Also prints recovery paths in apply-plan and external-id human output. Both carried rollback and result-journal information in their structured summaries but did not print it the way normal sync does, so recovery depended on knowing the artifact naming convention — which is not something to rely on mid-incident. JSON shape is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
Live write validation against two workspace networks surfaced two documentation inaccuracies. No code defect — both are about a reader forming a wrong belief. The rollback artifact was described in six places as a complete pre-change copy of the setup. It is not. It carries the account list and PATCHable fields, and omits collect, connectionTimeoutSeconds, requestTimeoutSeconds, numVirtualizedDevices, and useForwardAccountToAssumeRole. That omission is safe for its actual purpose, because Forward's PATCH is a top-level tri-state merge and absent fields are left unchanged — the artifact was applied live and restored exact endpoint hashes on both networks, including across two setups. But an operator mid-incident could reasonably read "complete copy" as "backup" and try to reconstruct a setup from it, and those settings are not there. The docs now say what it contains, what it does not, and why restoration still works. Also records what live validation actually covered, so a future reader does not read "validated live" as covering everything. Verified against real Forward: sync-accounts removing and restoring an account; the ceiling, allow-removals, and unattended-destructive refusals each leaving a failed journal entry and no PATCH; apply-plan mutation and rollback restoration across one and two setups; digest stability across separate processes; webhook auth, 403 scope rejection, and dedupe of a replay; status reporting observation_atomic=false; and zero-diff suppression sending no PATCH. Explicitly not verifiable on these networks: older-snapshot 409 rejection, because each workspace holds a single snapshot, so it remains unit-tested only; and GovCloud paths, since neither network has a GovCloud partition. Documents one surprising behavior: restoring via a reviewed manifest returns the same account set sorted, so the raw endpoint hash differs while the configuration is semantically identical. Restoring via the emitted rollback reproduces the original byte for byte. Expected, but an operator comparing hashes during recovery would otherwise think something had gone wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The recent run of patches around deletions and edge cases was treating symptoms. The root cause is one level below the code: the tool's central input never meant what the design assumed.
The NQE query returns the snapshot's observed cloud inventory — accounts that collected successfully, plus accounts merely visible through
organizations:ListAccountsmetadata. It is not sourced from configured accounts and carries no completeness contract. Collector authorization failures are ignored and service exceptions return partial lists, so partial results are routine.--prune-missingtreated absence from that result as proof an account should be deleted, conflating five different situations: genuinely removed from the org, collection failed this snapshot, auth failure, belongs to another org, transient error.Measured against production before changing anything:
The second network returned 96% of its inventory and would still have destroyed 27 live accounts. This was not one misconfigured network.
What changed
--prune-missingrefuses at three layers (CLI, planner, gateway).sync-accountswith a reviewed manifest is the only supported removal path.PatchCloudAccounthas exactly one production caller, enforced by a test that fails if another appears. Closes theapply-plandisable bypass, where a payload could set every account toenabled: falsewithout tripping any removal guard.map[string]anycrosses the adapter boundary;ComputeDesiredhas no clock or I/O, so preview and apply digests are stable.Limitations documented rather than papered over
--allow-unattended-destructive, not solved.ListAccountscannot distinguish "left the org" from "belongs to another org in this setup". A reviewed manifest supplies exactly the provenance the API cannot.observation_atomic=false.Breaking changes
See
docs/upgrading.md.--prune-missinghard-fails with an actionable error.serve-webhook --applyrequires Basic Auth credentials and an explicit--network-id.--allow-unattended-destructive.Validation
202 tests. 21 characterization tests pin known-unfixable behavior behind guards that CI keeps disabled. Verified live against two workspace networks, both restored to byte-identical baseline hashes:
sync-accountsremoving and restoring an account; all three destructive refusals;apply-planmutation and rollback restoration across one and two setups; digest stability across processes; webhook auth,403scope rejection, and dedupe.Not verifiable on those networks and unit-tested only: older-snapshot
409rejection (one snapshot per workspace) and GovCloud paths (no such partition).🤖 Generated with Claude Code
https://claude.ai/code/session_01Arw4zhKVDNry9zV1Ej6jRt