feat(cleanup): select and record nightly test-wipe deletions - #29
feat(cleanup): select and record nightly test-wipe deletions#29thelostorbital wants to merge 1 commit into
Conversation
Add internal/isolation/cleanup: an I/O-free selector, planner, canonical record, and executor for WF-TEST-01 step T7. Selection admits only the closed three-kind capability set with D-157 ownership proof, refuses any ambiguous, unsupported, permanent, or cross-project target by failing the whole inventory, orders instances before disks before run firewall rules with retained-disk semantics, and seals every deletion so an adapter cannot be called with a plain name. The first run after bootstrap and any run whose harness state is not open and usable plan and record but delete nothing. Every deletion is recorded before it is issued and re-runs converge from observed state. Claude-Session: https://claude.ai/code/session_01MQYGGYsYAzMU3MiM2SKhEZ
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52fcfacf5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case state.BootstrapPhase() != isolation.BootstrapPhaseOpen || state.TestUsability() != isolation.TestUsabilityUsable: | ||
| plan.Disposition = DispositionHarnessNotUsable |
There was a problem hiding this comment.
When a persisted open state still says TestUsabilityUsable but input.Now has reached its T8ValidUntil, this branch nevertheless grants DispositionDelete; harness states do not automatically change usability when their evidence expires. AdmitHarnessAction explicitly enforces that deadline, but Plan never reads it, so the nightly wipe can mutate resources after its isolation proof has gone stale. Check the state's T8 window at planning time and fail closed or produce the non-mutating disposition.
AGENTS.md reference: AGENTS.md:L24-L27
Useful? React with 👍 / 👎.
| outcome.Failure = redact.Sanitize("deletion intent was not persisted") | ||
| return outcome, fmt.Errorf("%w: deletion intent was not persisted", ErrWipeExecution) | ||
| } | ||
| deleteErr := deleter.Delete(ctx, deletion) |
There was a problem hiding this comment.
If Execute is called after the inventory window used by Plan has expired—or after a same-named resource was deleted and recreated—it sends the old sealed deletion directly to the provider without fresh discovery or comparison against current creation time, labels, attachments, or firewall lifetime evidence. A seal only proves that the in-memory plan was unchanged; it does not prove that provider state is unchanged, so this can delete a replacement resource selected by stale identity data. Require fresh provider revalidation at this boundary before calling Delete.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
| } else { | ||
| final.Status = DeletionDeleted | ||
| outcome.Status = DeletionDeleted | ||
| } |
There was a problem hiding this comment.
When Deleter.Delete returns nil, the code immediately records DeletionDeleted and increments the successful count without any observed post-change state. A provider adapter that returns after accepting an asynchronous operation, or whose operation later fails, therefore leaves the resource present while the durable journal claims it was deleted; execution can then continue to dependent disk and firewall steps using a false result. Extend the execution boundary to obtain and validate fresh absence evidence before recording success.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
| if record.SchemaVersion != WipeRecordSchemaV1 || !sha256Pattern.MatchString(record.IntegritySHA256) { | ||
| return nil, ErrInvalidWipeRecord |
There was a problem hiding this comment.
For an imported or corrupted journal document whose SHA-256 is recomputed, CanonicalJSON validates only the schema string and checksum; it accepts empty required identities, unknown dispositions/status-like values, zero timestamps and run numbers, duplicate deletion sequences, and contradictory resource lists. ParseWipeRecordV1 consequently returns malformed durable history as valid canonical state, which can make later run counting or resume logic derive authorization and attempts from unrecognized data. Add complete field, enum, uniqueness, and cross-field validation before accepting the checksum.
AGENTS.md reference: AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
| selection.Ignored++ | ||
| continue | ||
| } | ||
| attachments[item.Identity.CanonicalKey] = sortedNames(item.AttachedInstances) |
There was a problem hiding this comment.
When an exhaustive snapshot is internally contradictory—for example, a fresh or foreign instance lists an expired disk in AttachedDisks while that disk's AttachedInstances is empty—selection trusts only the disk-side list and schedules the disk for deletion. InstanceObservation.AttachedDisks is never consulted anywhere, so malformed or partial provider data causes a destructive attempt instead of refusing the inventory; the provider will commonly reject the attached-disk deletion and stop the rest of the wipe. Cross-check both directions using full canonical instance identities before selecting a disk.
AGENTS.md reference: AGENTS.md:L24-L27
Useful? React with 👍 / 👎.
| for _, deletion := range selection.Deletions { | ||
| record.Deletions = append(record.Deletions, plannedDeletion(deletion)) | ||
| } | ||
| for _, retained := range selection.Retained { |
There was a problem hiding this comment.
When the provider returns the same exhaustive resource set in a different order, retained instances/disks/firewalls and protected firewalls preserve that input order, and these loops serialize it directly into the hashed record. Thus semantically identical selections with the same inventory revision can produce different IntegritySHA256 values, undermining deterministic comparison and audit integrity; only deletions are currently sorted. Sort every set-like recorded collection by canonical identity before computing the plan seal.
AGENTS.md reference: AGENTS.md:L67-L69
Useful? React with 👍 / 👎.
| HarnessIntegritySHA256: plan.HarnessIntegritySHA256, BootstrapPhase: string(plan.BootstrapPhase), | ||
| TestUsability: string(plan.TestUsability), Disposition: plan.Disposition, | ||
| InventoryRevision: selection.InventoryRevision, PlannedAt: selection.Now, | ||
| MaxLifetimeSeconds: int64(selection.MaxLifetime / time.Second), IgnoredForeign: selection.Ignored, |
There was a problem hiding this comment.
When WipePolicy.MaxLifetime contains a fractional second, this conversion truncates it even though policy validation accepts arbitrary time.Duration values. For a firewall-only or otherwise unchanged selection, policies such as 6h+100ms and 6h+900ms can therefore produce the same recorded value and plan integrity despite representing different expiry limits, while the audit document also reports a threshold different from the one actually enforced. Either reject non-whole-second lifetimes or serialize the exact duration so every semantic policy change alters the seal.
AGENTS.md reference: AGENTS.md:L67-L69
Useful? React with 👍 / 👎.
|
Coordinator (Thread F) review. All ten M1-08 PR1 required-proof items map to code and a test (closed capability set via frozen F-1 — This is the in-scope half of Codex thread "Revalidate each deletion immediately before issuing it"; the per-target describe-before-delete half belongs to the PR2 provider adapter (§4.4) and can be adjudicated DEFERRED there. Coordinator verdicts on the other Codex threads, for alignment:
Full local gate is queued behind PR #28 here; Quality is authoritative meanwhile. |
1 participant
Summary
M1-08 PR1 (Thread D). Adds
internal/isolation/cleanup: the I/O-free nightly test-wipe selector, planner, canonicaltest/wipe/<date>.jsonrecord, and executor for WF-TEST-01 step T7. No process execution, provider I/O, durable I/O, CLI, dependency, or CI change.Selectconsumes an exhaustive typed inventory (instances, disks, classic firewalls, plus any test-namespace resource of an unsupported kind) and the durable run lifetime records, and returns sealed, ordered deletions: instances first (always retaining disks), then disks, then run firewall rules.Planbinds the selection toHarnessStateV1and the durable wipe history and decides the disposition:first-run-observe-only,harness-not-usable,nothing-expired, ordelete. Every disposition still records the complete selection.Executerecords the plan, then records each deletion before issuing it and again with its outcome, stopping at the first failure. A later run converges from freshly observed state with visible attempt numbers.cleanup.Journalis a minimal two-method create-only interface until the M1-05 store adapter lands (assumption A7 inm1-records/D-m1-08.md).Safety properties
ctrldb-test-<runId>-prefix, all three reserved labels, and arun-idlabel that binds the name; prefix without labels, labels without prefix, or a non-binding run-id refuse the whole inventory. Run firewalls need the exact name, immutable description fingerprint, matching durable record, and reached expiry; the two permanent harness rules are protected and never candidates.KeepDisks; an expired disk still attached to an unselected instance is deferred, not deleted.Verification
From
.worktrees/m1-test-wipe-runtime, all withGOWORK=off:go mod tidy -diff— passgo run ./internal/archcheck— passgo test ./...— passgo test -race ./internal/isolation/cleanup/...— pass (aggregate race run skipped locally per board; GitHub Quality is authoritative)QLTY_TELEMETRY=off qlty check --all --no-fix --level=low --fail-level=low— pass (No issues)bash scripts/test-architecture-rules.sh— passbash scripts/check-go-licenses.sh— skipped locally:go-licensesnot on PATH; no dependency changegit diff --check— passRead-only probes used (all
--format=json, values not copied into the repository):gcloud version,gcloud auth list,gcloud config list,gcloud run jobs list --account=… --project=…,gcloud scheduler jobs list --account=… --project=…(refused without--location; not retried). No GCP command or mutation beyond those read-only probes was performed.https://claude.ai/code/session_01MQYGGYsYAzMU3MiM2SKhEZ