feat(oabctl): programmatic delete API with explicit target contract by chaodu-agent · Pull Request #1415 · openabdev/openab

chaodu-agent · 2026-07-16T04:16:12Z

#1415 feat(oabctl): programmatic delete API with explicit target contract
State: open | Author: chaodu-agent | Branch: refactor/oabctl-delete-api

Summary

Designs and implements the programmatic delete contract that the #1404 review intentionally deferred ("until their programmatic contracts are designed independently"). This PR is stacked on refactor/oabctl-lib.

let report = oabctl::delete_services(
    &amp;aws,
    &amp;[DeleteTarget::new(&#34;prod&#34;, &#34;nest-my-oab&#34;)],
    &amp;DeleteOptions::new(&#34;oab&#34;),
).await?;

The API is symmetric with apply_manifests:

DeleteOptions requires an explicit cluster; the library path never reads ~/.oabctl/config.toml.
Bucket resolution shares apply's chain: explicit option Ã�Â¢Ã�Â�Ã�Â� OAB_CONTROL_PLANE_BUCKET Ã�Â¢Ã�Â�Ã�Â� oab-control-plane-{account}.
Contract guards run before AWS calls, including rejection of delimiter-ambiguous namespaces.
DeleteError { Validation | Target | Teardown } preserves the DeleteReport for targets completed before a failure.
Exact AWS identities are checkpointed before mutation so teardown failures can be retried without name-only cleanup.
Programmatic calls suppress human-readable progress while the CLI retains its existing rendering.

Review Contract

Contract revision: RC-1

Goal

Provide a public, in-process Rust API that safely deletes one or more explicitly identified OAB ECS deployments without reading CLI home configuration. A successful result must mean that the checkpointed ECS incarnation and its exact ingress dependencies are gone and the target's control-plane manifest/artifacts are removed. Ambiguous ownership or incomplete teardown must fail closed with a structured, retryable error rather than report false success or delete another deployment.

Non-goals

Providing linearizable coordination between concurrent apply_manifests and delete_services calls for the same logical target; callers must serialize same-target mutations as described under Accepted Residual Risks.
Performing name-only orphan cleanup when neither a live ECS service nor a matching durable delete checkpoint can establish ownership.
Removing platform-side webhooks, Telegram disclosure state, Secrets Manager secrets, shared bootstrap infrastructure, logs, or unrelated task-definition revisions.
Redesigning the stacked apply_manifests API, changing existing CLI UX, or introducing a general distributed workflow engine.
Parallelizing multi-target deletion or continuing later targets after the first target failure.

Accepted Residual Risks

Same-target apply/delete concurrency is unsupported in RC-1. The caller must serialize mutations for the same (AWS account, Region, control-plane bucket, cluster, namespace, name) tuple. The intended NEST control-plane caller uses a per-target single-flight/lock. If this precondition is violated, recovery is to stop concurrent writers, inspect the retained checkpoint, re-apply the intended desired state if apply should win, or retry delete if delete should win. A shared generation fence remains a follow-up.
AWS control-plane eventual consistency can require retries. Ambiguous or not-yet-converged ECS responses fail closed, dependent cleanup does not start, and the durable checkpoint remains for retry.
An already-orphaned deployment without a checkpoint is not auto-cleaned. This can leave resources behind, but avoids destructive name-only guessing; recovery is explicit operator inspection/remediation.
Batch deletion is sequential and stops at the first failed target. The structured error retains prior completed targets and identifies the failed target, so the caller can retry deterministically.

Acceptance Criteria

The crate publicly exports delete_services, DeleteTarget, DeleteOptions, DeletedService, DeleteReport, DeleteError, and DeleteErrorKind without changing the existing CLI command surface.
Before any AWS call, the API rejects an empty target set, a blank cluster, blank namespace/name components, and any logical target that cannot map unambiguously to one physical delete identity. Distinct accepted (namespace, name) pairs must not resolve to the same ECS/API/Cloud Map identity.
The library requires an explicit cluster, never reads ~/.oabctl/config.toml, and resolves the control-plane bucket using the same explicit option Ã�Â¢Ã�Â�Ã�Â� environment Ã�Â¢Ã�Â�Ã�Â� caller-account chain as apply.
Before mutating ECS, delete persists a checkpoint bound to the exact target and caller boundary: namespace/name, bucket, partition/account/Region, requested and canonical cluster identity, ECS service ARN and createdAt incarnation, and exact registry/API identifiers when ingress exists.
Initial missing, empty-success, mixed-failure, duplicate, or otherwise ambiguous AWS identity responses fail closed. Retries may continue only from a matching checkpoint, and dependent ingress/S3 cleanup must not begin until the checkpointed ECS incarnation is unambiguously absent.
Delete has no first-name-match or name-only fallback for ECS, Cloud Map, or API Gateway. It must not remove resources belonging to a different cluster, account, Region, logical target, or recreated ECS incarnation.
A successful DeleteReport contains only fully completed targets. DeleteError identifies validation/target/teardown phase, preserves prior completed targets, identifies the failed target for teardown failures, and retains the checkpoint whenever retryable cleanup remains.
Programmatic apply/delete calls emit no human-readable progress to process-global stdout/stderr; existing CLI output and aggregate CLI failure behavior remain unchanged.
Public Rustdocs or user documentation explicitly state the same-target serialization precondition and the no-orphan-cleanup behavior so callers can satisfy the accepted-risk mitigations.
Focused tests cover pre-AWS validation, logical-identity collision prevention, checkpoint matching and fail-closed AWS response classification, incarnation mismatch, drain timeout, partial-report preservation, and retry behavior. cargo test and cargo clippy --all-targets -- -D warnings pass for the operator crate on the reviewed head.

Follow-ups

Add a shared target generation/lease fence plus conditional S3 writes so apply and delete can safely arbitrate concurrent same-target mutations instead of requiring caller serialization.
Introduce a durable opaque deployment ID or reversible canonical resource-name encoding across apply/delete, beyond the minimum collision prevention required by RC-1.
Design an explicit, auditable orphan-recovery API that can discover and remove resources without weakening the default fail-closed path.
Add optional bounded parallelism and a collect-all-results batch mode if control-plane scale requires it.
Decide separately whether secret deletion, webhook removal, disclosure cleanup, and broader tenant deprovisioning belong in this crate or in the control plane.

CLI

No intended behavior change: oabctl delete and oabctl delete -f retain config-file cluster resolution, printed progress, and aggregate failure reporting.

Motivation

NEST's manager needs /delete for tenant deprovisioning. Webhook removal and Telegram-side disclosure remain in the control plane, while ECS, ingress, and S3 teardown reuse this API in-process, matching how the manager already consumes apply_manifests.

Validation Evidence

Follow-up implementation head: 18322e16f2b63e5484b800507fe23451bbac1751.
RC-1 fixes reject delimiter-ambiguous delete namespaces before AWS, preserve partial reports, and fail closed before destructive mutation when an apply ingress-teardown checkpoint is pending.
Prior exact-head Review Contract validation and operator CI passed before this follow-up; the current head is tracked below.
Author-reported real-AWS E2E on the prior exact-identity head covered initial fail-closed drain behavior, checkpointed retry to full teardown, already-gone refusal without a checkpoint, and post-success checkpoint removal.
Focused re-review of 1bd61e8..54bd8f2 found all round-2 findings resolved with no new in-scope blocker.

Follow-up validation

Follow-up head 18322e16f2b63e5484b800507fe23451bbac1751 adds duplicate-target validation, a bounded Telegram webhook client timeout, alias-equivalent physical-identity serialization guidance, bounded ownership probes, CLI cleanup warnings, and S3 path redaction.
B4 follow-up preserves CLI best-effort behavior after drain timeout, continues S3 cleanup after individual failures, and treats an absent exact apply service as detached; the strict API remains fail-closed.
B7-B2-B16 follow-up enforces the documented identity charset (with a legacy CLI namespace-hyphen exception), aborts ownership probes before admitting replacements after the first error, and treats ECS INACTIVE apply responses as absent.
B14 fixes resumable DRAINING ECS deletion by reissuing delete_service(force=true) before polling, extends Cloud Map deletion to 12 attempts with capped 5/10/20/30-second backoff, and documents that manifest-first apply failures are recovered by re-running apply because no apply rollback checkpoint exists.
Local git diff --check, Rustfmt parser checks, and focused B14 static assertions passed. Local cargo check was attempted but is blocked by the environment (linker \cc` not found`); GitHub CI for this exact head is the authoritative Rust validation.
This head also bounds identity ownership probes to 8 concurrent S3 requests and adds a 201-character/100-alias regression test; GitHub CI for this exact head completed with changes, check, validate, and poll-and-review successful, but operator failed at cargo check (job 89065627641).

Designs the delete contract deferred by the lib-split review, symmetric with apply_manifests: - delete_services(&SdkConfig, &[DeleteTarget], &DeleteOptions) -> Result<DeleteReport, DeleteError> - DeleteOptions requires an explicit cluster and shares apply's bucket resolution chain (explicit -> OAB_CONTROL_PLANE_BUCKET -> account) so delete always cleans the bucket apply wrote to; never reads CLI config - Contract guards before any AWS call: non-empty target set, non-empty cluster, non-blank namespace/name (regression-tested without AWS) - Typed DeleteError {Validation, Target, Teardown} preserving the report of targets completed before a failure; teardown remains resumable - Best-effort ingress skips surface in DeletedService.warnings; S3 cleanup failures still fail the target (safe to retry) - delete.rs output now routes through the shared progress gate: CLI behavior unchanged, programmatic calls are silent - CLI paths (delete <name>, delete -f) unchanged

- needless_borrow in apply's registry-detach wait - allow too_many_arguments on validate_checkpoint / prepare_identity / ingress::delete_exact — these deliberately thread the full deployment identity (partition/account/region/cluster/bucket) as separate explicit parameters; bundling them would obscure the F1 ownership contract - box PreparedIdentity::Exact payload (large_enum_variant) No behavior change; 90 tests + doc-test pass, release build clean.

… refactor/oabctl-delete-api # Conflicts: # operator/src/apply.rs # operator/src/delete.rs

- apply: redundant_guards — match empty failures with a slice pattern - ingress: cloud_map_service_id_from_arn became test-only after the exact-identity merge (all production paths use the boundary-pinned parser); mark it #[cfg(test)] with a note instead of carrying dead code Verified with the CI toolchain (rustc 1.97.1): clippy -D warnings clean, 96 tests + doc-test pass.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — The logical delete target is non-unique, and post-ECS cleanup is not fenced against a concurrent apply.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

github-actions · 2026-07-18T08:50:33Z

Caution

This PR has been waiting on the author for more than 2 days (labeled pending-contributor since 2026-07-16).
It will be automatically closed in 24 hours if there is no update.

@chaodu-agent — You must add a new comment on this PR to remove the closing-soon label and keep it open. Pushing commits alone is not sufficient. Feel free to reopen a new PR later if it gets closed and you want to pick it back up.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — RC-1 is frozen; logical identity collision plus cleanup, documentation, and test gaps remain.

Consolidated review: #1415 (comment)

Contract freeze: RC-1 at 1bd61e825e3f2e1308bde3be6294e10b0e3bead6; SHA-256 a2377a772fc9ba7f4856cea6af2cc6f6596dba919f12256295fd65516de3c674.

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — The delete-only namespace guard does not close the shared apply/CLI identity collision.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

Apply and delete previously validated logical identity independently: the hyphen-free namespace guard existed only in programmatic delete, so apply could still create 'prod-team/bot' while 'prod/team-bot' remained a valid delete target — both resolving to the physical name 'oab-prod-team-bot' (round-3 review finding F1). A new shared module, operator/src/identity.rs, now owns the rule: - Domain rule: validate_injective_identity (non-empty components, hyphen-free namespace) is enforced before any AWS call by manifest validation (CLI apply, fleet expansion, apply_manifests) and by delete_services. - Ownership rule: ensure_exclusive_physical_identity probes the exact control-plane keys (manifest, delete checkpoint, ingress-teardown checkpoint) of every colliding alias pair before mutation, in apply_ecs and in the shared delete choke point run_with_bucket (CLI + programmatic). Contested physical identities fail closed naming the colliding pair. - All physical name and control-plane key derivations (ECS/Cloud Map names, manifest/checkpoint keys, scale, ingress) route through the shared module so probe and mutation cannot drift. - Manifest schema namespace pattern tightened to ^[a-z0-9]+$. BREAKING CHANGE / legacy policy: manifests with hyphenated namespaces are rejected at apply with a migration message. Existing hyphenated-namespace deployments stay deletable via 'oabctl delete' (warning + ownership check); migrate by deleting and re-creating under a hyphen-free namespace. Tests: identity module unit tests plus cross-entry-point regressions in manifest, apply, and delete proving colliding logical pairs cannot overwrite, checkpoint, or delete one another. cargo clippy --all-targets -- -D warnings and cargo test (113 + 1 doctest) pass.

chaodu-agent

Note

LGTM ✅ — The exact requested head closes the prior identity-collision and teardown-safety findings; no new in-scope blockers were found.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent · 2026-07-21T02:36:56Z

Note

LGTM ✅ — The round-3 blocker is closed: apply and every delete entry point now share one injective identity rule plus a pre-mutation control-plane ownership check, with an explicit legacy-namespace policy.

What This PR Does

Adds a public in-process delete_services API for explicit OAB namespace/name targets, symmetric with apply_manifests: explicit cluster, shared bucket-resolution chain, no ~/.oabctl/config.toml reads, durable exact-identity checkpoints before mutation, structured partial results, and suppressed progress output for programmatic callers. The CLI keeps its existing flow.

How It Works

Each target resolves to one live ECS service. Before mutation, delete stores a durable S3 checkpoint (caller boundary, bucket, canonical cluster/service ARNs, ECS createdAt incarnation, registry ARN, API ID), then drains that exact incarnation, removes exact ingress dependencies, deletes the manifest/artifacts, and removes the checkpoint last. Ambiguous AWS identity responses fail closed; matching retries resume from the checkpoint.

New in this revision, a shared operator/src/identity.rs module owns logical→physical identity:

Domain rule — validate_injective_identity (non-empty components, hyphen-free namespace) runs before any AWS call in manifest validation (CLI apply, fleet expansion, programmatic apply) and in programmatic delete, so oab-{namespace}-{name} parses back to exactly one accepted pair.
Ownership rule — ensure_exclusive_physical_identity runs pre-mutation in apply_ecs and in the shared delete choke point run_with_bucket (all CLI and programmatic delete paths). It probes the exact control-plane keys (manifest, delete checkpoint, ingress-teardown checkpoint) of every colliding alias split and fails closed naming the contested pair. Non-NotFound probe errors also fail closed.
No drift — all ECS/Cloud Map names and control-plane keys (manifest, delete-checkpoint, ingress-teardown-checkpoint) derive from the shared module; a regression asserts probe keys equal mutation keys.
Legacy policy — hyphenated-namespace manifests are rejected at apply with a migration message; existing legacy deployments remain deletable via oabctl delete (warning + ownership check). Schema metadata.namespace tightened to ^[a-z0-9]+$.

Findings

#	Severity	Finding	Location
1	🟢 Praise	Round-3 F1 is closed at every entry point: `prod/team-bot` vs `prod-team/bot` can no longer overwrite, checkpoint, or delete one another — the hyphenated side is outside the accepted domain, and the accepted side is blocked by the ownership probe whenever the legacy pair is recorded.	`operator/src/identity.rs`
2	🟢 Praise	The legacy-deployment policy is explicit rather than silent: apply fails with a migration hint, CLI delete warns and proceeds only under the ownership check.	`operator/src/delete.rs`, `operator/src/manifest.rs`
3	🟢 Praise	Cross-entry-point regressions cover apply-side rejection/acceptance, programmatic-delete rejection, alias symmetry, and probe/mutation key alignment.	`operator/src/{identity,manifest,apply,delete}.rs` tests

Finding Details

🟢 F1 (round-3) — Shared injective identity/ownership rule, verified per entry point

Independent exact-SHA audit of 54bd8f2..be006ec traced every entry point:

Apply: CLI apply -f and programmatic apply_manifests both reach validate_apply_request → OABServiceManifest::validate → validate_injective_identity; fleet manifests are validated post-expansion. apply_ecs runs the ownership probe before reading generation state or mutating.
Delete: CLI delete <name>, CLI delete -f, and programmatic delete_services all funnel through run_with_bucket, which runs the ownership probe before the pending-ingress-teardown guard, identity preparation, and any checkpoint/ECS mutation. Programmatic targets are additionally domain-rule-rejected pre-AWS in validate_delete_request.
Probe correctness: collision_aliases enumerates every alternative split point of {namespace}-{name}; within the hyphen-free-namespace domain the mapping is injective by construction, so collisions can only involve recorded legacy pairs — exactly what the probe checks. Probed keys are derived from the same helpers the mutation paths use.

ℹ️ Residual notes (within RC-1's accepted risks, documented)

The ownership check treats the control plane as the source of truth. A legacy deployment whose stored manifest was manually removed is invisible to the probe. Apply always records manifests and delete removes them only after full teardown (checkpoint last), so system-created states are covered; manual S3 tampering stays out of scope, consistent with RC-1's no-name-only-guessing stance. The existing "durable opaque deployment ID" follow-up would close this fully.
The probe-then-mutate sequence is not atomic. Same-physical-identity concurrency is governed by the same caller-serialization precondition RC-1 already documents; the generation/lease-fence follow-up remains the structural fix.

Baseline Check

PR opened: 2026-07-16
Reviewed head: be006ec67f5329df90423785e417e0ffb4671d85
Previous reviewed head: 54bd8f28eda19c3bd209789be89c44740d3a55f1
Main checked at: d8d4bf15ca9a6e7380a5683a87527b93f527badb
Main already has: no public delete_services/DeleteTarget/DeleteOptions API and no shared identity module
Net-new value since round 3: operator/src/identity.rs (domain rule, alias enumeration, ownership probe), wiring into manifest validation / apply_ecs / validate_delete_request / run_with_bucket, schema namespace pattern ^[a-z0-9]+$, legacy policy docs, and cross-entry-point regressions
git diff --check clean on the delta

Verified

On the exact reviewed head be006ec:

cargo clippy --all-targets -- -D warnings — clean
cargo test — 113 unit tests + 1 doctest passed, 0 failed
Exact-head CI: operator job (check, clippy, test, build) ✅, Review Contract ✅, all other required checks ✅
Independent exact-SHA audit (entry-point tracing, probe-key/mutation-key alignment, legacy policy, offline-safety of new tests) — passed
Note: the crate is intentionally not blanket-rustfmted in this commit; the pre-existing head was not fmt-clean and the operator CI job has no fmt gate, so the delta stays minimal for review.

Previous Review and External Feedback

Round-3 consolidated review

🔴 F1 — the collision guard exists only in programmatic delete; apply still accepts hyphenated namespaces and every CLI delete path bypasses the guard. Requested: one shared injective identity/ownership rule across all entry points, an explicit legacy-deployment policy, and cross-entry-point regressions.

✅ Addressed in be006ec. Shared rule in operator/src/identity.rs, enforced at every apply and delete entry point (domain rule pre-AWS, ownership probe pre-mutation); legacy hyphenated namespaces are apply-rejected with a migration message but remain CLI-deletable under the ownership check; regressions exercise both apply and delete sides of the colliding pair, including the alias-keyed checkpoint gap called out in round 3 (the probe checks the alias's manifest, delete-checkpoint, and ingress-teardown-checkpoint keys).

Prior real-AWS verification

Exact-checkpoint retry completed ECS, Cloud Map, API Gateway, and S3 teardown and removed the checkpoint last.

✅ Still valid for the canonical path; this revision adds only pre-mutation validation/probing before that path, plus read-only S3 probes that fail closed.

Automated stale-contributor notice

Operational only; superseded by current PR activity.

No other external reviewer findings were present on the reviewed head.

What's Good (🟢)

One identity module now owns every physical-name and control-plane-key derivation — probe and mutation cannot drift.
Fail-closed posture is preserved end-to-end: ambiguous ECS responses, contested identities, and probe errors all stop before destructive mutation.
The breaking change is explicit: ! commit, migration message in the error itself, README/CLI-guide policy sections, and a tightened published schema.
Programmatic progress suppression and structured partial reports are unchanged and still covered by tests.

5️⃣ Three Reasons We Might Not Need This PR

The CLI already performs teardown — if every control-plane caller could shell out safely, the public library contract adds compatibility and testing surface.
Logical names may be the wrong destructive identity — a durable opaque deployment ID would eliminate the alias problem structurally instead of validating around a string encoding; it remains the listed follow-up.
Tenant deprovisioning spans more than ECS/ingress/S3 — webhook, disclosure, secret, and serialization policy still live in the control plane, which could argue for orchestrating deletion there entirely.

chaodu-agent

Note

LGTM ✅ — Round-3 F1 (non-injective apply/delete identity) is closed on head be006ec by a shared identity module enforced at every apply and delete entry point, with an explicit legacy-namespace policy and cross-entry-point regressions.

Consolidated review: #1415 (comment)

chaodu-obk · 2026-07-21T12:02:14Z

CHANGES REQUESTED ⚠️ - The delete API contract is well-structured, but the current head still has actionable concurrency, documentation, timeout, and integration-validation gaps.

Important

This is the final consolidated review for the exact head be006ec67f5329df90423785e417e0ffb4671d85. PR #1415 is a stacked change based on refactor/oabctl-lib; the correct net diff is 10 files with 2,829 additions and 431 deletions. This summary uses that base rather than main so the stacked library split is not double-counted.

What This PR Does

Adds a public delete_services API to oabctl, with explicit target and cluster contracts, shared bucket resolution, injective physical-identity validation, exact-resource checkpoints, structured partial reports, resumable teardown, and documentation for programmatic callers.

How It Works

The library path requires explicit cluster and bucket resolution, validates targets before AWS mutation, derives a canonical physical identity, probes control-plane keys for collision aliases, persists a checkpoint before destructive work, and retries using exact ARN and service-incarnation data. Apply and delete share identity helpers and an ingress-teardown interlock, while the CLI retains its legacy human-readable path and legacy namespace behavior.

Findings

#	Severity	Finding	Location
1	🟡	The ownership check is a non-atomic probe followed by mutation, while the documented serialization key covers only the logical `(namespace, name)` tuple. Distinct aliases such as `prod/team-bot` and legacy `prod-team/bot` can map to `oab-prod-team-bot` and race through the probe before mutating the same physical deployment.	`operator/src/identity.rs:135`; `operator/src/lib.rs:72`
2	🟡	`validate_delete_request` does not reject duplicate `DeleteTarget` entries in one request, allowing the same logical target to be processed more than once in a batch.	`operator/src/delete.rs:407-426`
3	🟡	The top-level multi-target `delete_services`/`run_with_bucket` retry and checkpoint-resume path has no AWS SDK mock integration test; current tests primarily cover pure classification and validation helpers.	`operator/src/delete.rs:481-503,1427-1528`
4	🟡	Telegram webhook registration creates a `reqwest::Client` without an explicit timeout, so a hung upstream request can block teardown indefinitely.	`operator/src/ingress.rs:340`
5	🟡	`operator/README.md` documents the library `apply_manifests` API but does not document `delete_services`, the injective identity rule, or the same-target serialization precondition. The public README should match the Rustdoc and the newly exposed API contract.	`operator/README.md:76-113`
6	🟡	The PR validation section still references an earlier head (`54bd8f2`) rather than the reviewed head `be006ec67f5329df90423785e417e0ffb4671d85`; refresh or explicitly attach exact-head validation evidence.	PR validation section
7	🟢	Fail-closed ECS classification, full ARN/account/region/cluster/incarnation validation, exact-resource checkpoints, partial-report preservation, and ingress teardown interlock substantially reduce accidental deletion and retry leakage.	`operator/src/delete.rs`; `operator/src/identity.rs`; `operator/src/apply.rs`
8	🟢	The public API boundary is clear: explicit cluster/bucket inputs, symmetric Apply/Delete options, shared identity derivation, suppressed library progress, and preserved CLI behavior avoid hidden configuration coupling.	`operator/src/lib.rs`; `operator/src/delete.rs`; `operator/src/apply.rs`
9	🟢	Schema hardening, shell escaping, pinned dependencies, and documentation of no-orphan-cleanup and serialization preconditions provide useful defense-in-depth for callers.	`schema/oabservice-v2.json`; `operator/src/cli.rs`; `docs/oabctl.md`

Finding Details

🟡 F1: Serialize by canonical physical identity

The alias probe must be paired with a conditional lease/fence keyed by the canonical physical identity and held through mutation, or the documented caller precondition must require serialization across every logical alias that maps to the same physical identity. A non-atomic S3 probe alone does not close the TOCTOU window.

🟡 F2: Reject duplicate batch targets

Validate the complete target set before orchestration and return a deterministic contract error for duplicate or conflicting entries. Add unit coverage for duplicate DeleteTarget values and any alias-equivalent combinations.

🟡 F3: Add AWS mock integration coverage

Add a stub/mock S3 and ECS harness for multi-target teardown, interruption, checkpoint persistence, resume, and eventual-consistency outcomes. Pure-function tests do not establish that the complete orchestration path actually resumes safely.

🟡 F4: Bound Telegram webhook requests

Build the webhook client with an explicit timeout (for example, 10 seconds) and return a contextual error if client construction fails, so teardown cannot wait forever on the external API.

🟡 F5: Document the public delete API

Add a delete_services example and describe the injective namespace rule, same-target serialization requirement, and no-orphan-cleanup boundary in operator/README.md, keeping it aligned with operator/src/lib.rs and docs/oabctl.md.

🟡 F6: Refresh exact-head evidence

Re-run or link validation for be006ec67f5329df90423785e417e0ffb4671d85, rather than relying on evidence labeled with 54bd8f2. The current CI result is useful, but the review record should identify the exact reviewed head.

Baseline Check

PR base: refactor/oabctl-lib (stacked on the library split), not main.
main already contains the earlier refactor(oabctl): split operator into library + thin CLI binary #1404 library split; feat(oabctl): programmatic delete API with explicit target contract #1415's net-new scope is the programmatic delete contract and identity/checkpoint hardening.
Exact reviewed head: be006ec67f5329df90423785e417e0ffb4671d85.
Correct net diff against the stacked base: 10 files, +2,829/-431.
Net-new value: a public, resumable, exact-identity deletion API with shared apply/delete ownership rules; the existing CLI behavior remains available.

Addressing External and Prior Review Feedback

Prior review rounds

Earlier rounds identified the need for the programmatic delete contract, exact-resource checkpoints, identity collision protection, and clear caller preconditions. The current head adds the shared identity module, schema restriction, checkpoint/incarnation validation, ingress interlock, and documentation. The alias-concurrency TOCTOU and exact-head validation-label issues remain open as described above.

Repository/CI feedback

The available CI evidence reports the operator, validation, polling/review, change, and check jobs successful. Local Rust execution was unavailable in some review environments because cargo/rustc were not installed; the PR record should therefore retain the CI result and refresh its head reference rather than claim a local rerun.

Validation

Local stacked-base clone/diff review completed with git fetch origin pull/1415/head and git fetch origin refactor/oabctl-lib.
git diff --check origin/main...pr-1415 and the corrected stacked-base comparison were inspected; the stacked-base scope is 10 files and 2,829 additions.
Static inspection confirmed shared ownership guards before apply/delete mutation, exact checkpoint boundaries, schema injectivity, public exports, and documented caller preconditions.
CI evidence for the reviewed change reports the operator, validation, poll/review, changes, and check jobs successful.
Local cargo test/cargo clippy could not be rerun in the available environment because the Rust toolchain is absent; this limitation is explicitly retained.
The missing AWS mock harness and missing ACP-style end-to-end teardown exercise are why the integration finding remains actionable.

What's Good (🟢)

The design is strongly fail-closed and binds retries to exact resource identity and service incarnation.
Apply/delete symmetry and the CLI/library boundary are clear without changing the existing CLI command surface.
Shared identity derivation, schema hardening, shell escaping, pinned dependencies, ingress interlock, and structured partial reports provide a solid foundation for safe teardown.

5. Three Reasons We Might Not Need This PR

The existing CLI may be sufficient - a higher-level manager could invoke the established command rather than expand the public Rust API and its long-term compatibility contract.
Teardown ownership may belong at a higher control-plane layer - secrets, logs, webhooks, VPC links, and task-definition revisions remain outside this API's scope, so a partial delete surface may not deliver the lifecycle semantics callers expect.
The concurrency and integration contract is not fully enforceable yet - same-target and alias serialization relies on callers, while the complete AWS retry path lacks mock integration coverage; exposing the API before those guarantees are stronger could increase operational risk.

chaodu-obk · 2026-07-22T22:21:27Z

Important

CHANGES REQUESTED ⚠️ -- The code-review findings are addressed on the reviewed head, but the required operator cargo validation is still failing.

What This PR Does

This PR adds a public, in-process delete API for explicitly identified OAB ECS services. It provides explicit cluster and bucket options, structured partial results, exact-identity checkpoints, fail-closed ownership checks, and a CLI compatibility path.

How It Works

Delete validates the shared injective logical identity before AWS calls, records the exact ECS incarnation and dependent resource identifiers before mutation, and performs cleanup from those checkpointed identities. Programmatic deletion uses strict cleanup; the legacy CLI retains best-effort cleanup with warnings and checkpoint retention. Apply and delete document the required caller-side serialization boundary for the same physical identity.

Findings

#	Severity	Finding	Location
1	🟡	The exact-head operator validation gate is failing at `cargo check`; AC#10 cannot yet be declared satisfied.	`operator` CI job `89065627641`
2	🟢	The active serialization documentation finding is addressed across public Rustdoc and user documentation.	`operator/src/apply.rs:411`, `operator/src/delete.rs:458`, `operator/src/lib.rs`
3	🟢	The active regression-evidence finding is addressed by collision, partial-report, pending-checkpoint, and drain-state tests.	`operator/src/delete.rs:1647-1720`

Finding Details

🟡 F1: Operator validation remains blocked

The exact-head GitHub checks changes, check, validate, and poll-and-review succeeded, but operator job 89065627641 completed with failure at cargo check. Local validation also cannot replace CI because this environment has no cc linker (linker cc` not found). This remains a validation gate, not a newly identified code defect.

Requested change: repair the operator CI/compiler failure and rerun the required operator cargo validation before treating the review gate as green.

🟢 F2: Serialization contract documented

The public apply API, delete API, crate-level Rustdoc, operator README, and docs/oabctl.md state that callers must serialize mutations for the same account, Region, control-plane bucket, ECS cluster, and physical service identity, including alias-equivalent logical pairs. Recovery requires stopping concurrent writers and re-applying the intended desired state or retrying delete.

🟢 F3: Regression evidence present

The current head includes tests for shared collision aliases across entry points, completed partial-report preservation when a later target fails, pending apply ingress-teardown blocking, and ECS phase transitions including DRAINING resume behavior. The exact cleanup path removes the apply-side ingress-teardown checkpoint after successful cleanup and retains retry state on failure.

Inline Review Disposition

Historical identity-collision threads are addressed by the shared injective identity rule, legacy namespace policy, ownership probes, and cross-entry-point regression coverage; those threads are outdated.
The historical same-target concurrency thread is accepted as the documented RC-1 residual risk. Internal serialization is a non-goal; the public contract now states the complete caller boundary and recovery procedure.
The historical stale apply ingress-teardown checkpoint thread is addressed by the successful-delete cleanup path and retry-safe retention on failure; that thread is outdated.
The active serialization thread is addressed by the documentation added on this head.
The active regression-evidence thread is addressed by the collision, partial-report, pending-checkpoint, and ECS phase tests listed above.

Baseline Check

Head: 18322e16f2b63e5484b800507fe23451bbac1751
Base: refactor/oabctl-lib
Merge-base: 6dd5285af10a237bef54f7ebeb797dc3b7450d1f
Diff: 10 files, 3276 insertions, 396 deletions
Net-new value: a structured programmatic delete contract with exact checkpointed teardown and explicit safety boundaries.

Validation

git diff --check: passed
Rustfmt parser checks: passed
Focused static assertions: passed
Exact-head CI: changes, check, validate, and poll-and-review passed; operator failed at cargo check (job 89065627641)
The PR remains open and must not be merged while the operator validation gate is failing.

5. Three Reasons We Might Not Need This PR

The existing CLI already performs teardown, so a second public API increases maintenance surface.
Caller-side serialization is weaker than a built-in distributed lease or generation fence.
A durable opaque deployment ID could eventually be stronger than namespace plus name, even with the current collision protections.

chaodu-obk

Important

CHANGES REQUESTED ⚠️ -- Code findings are addressed on this exact head, but the operator cargo validation gate is failing.

Consolidated review: #1415 (comment)

The exact-head checks changes, check, validate, and poll-and-review passed. Operator job 89065627641 failed at cargo check; repair and rerun that gate before treating this review as complete.

chaodu-agent requested a review from thepagent as a code owner July 16, 2026 04:16

This comment has been minimized.

Sign in to view

github-actions Bot added the pending-maintainer label Jul 16, 2026

fix(review): preserve delete teardown failures

e0f5333

This comment has been minimized.

Sign in to view

github-actions Bot added pending-contributor and removed pending-maintainer labels Jul 16, 2026

chaodu-agent added 5 commits July 16, 2026 22:24

fix(review): require exact delete resource identity

7b9b432

fix(operator): bind delete cleanup to exact identity

be9b909

Merge remote-tracking branch 'origin/refactor/oabctl-delete-api' into…

d4389cd

… refactor/oabctl-delete-api # Conflicts: # operator/src/apply.rs # operator/src/delete.rs

This comment has been minimized.

Sign in to view

chaodu-agent commented Jul 17, 2026

View reviewed changes

Comment thread operator/src/delete.rs Outdated

Comment thread operator/src/delete.rs Outdated

github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 18, 2026

openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 18, 2026

This comment has been minimized.

Sign in to view

chaodu-agent commented Jul 18, 2026

View reviewed changes

Comment thread operator/src/delete.rs Outdated

Comment thread operator/src/delete.rs Outdated

Comment thread operator/src/delete.rs

Comment thread operator/src/delete.rs

github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 18, 2026

fix(oabctl): satisfy delete review contract

54bd8f2

openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 18, 2026

This comment has been minimized.

Sign in to view

github-actions Bot added pending-maintainer and removed pending-contributor labels Jul 18, 2026

chaodu-agent commented Jul 18, 2026

View reviewed changes

Comment thread operator/src/delete.rs Outdated

github-actions Bot added pending-contributor and removed pending-maintainer labels Jul 18, 2026

github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 20, 2026

openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Jul 21, 2026

This comment has been minimized.

Sign in to view

github-actions Bot added pending-maintainer and removed pending-contributor labels Jul 21, 2026

chaodu-agent commented Jul 21, 2026

View reviewed changes

openabdev deleted a comment from chaodu-obk Bot Jul 21, 2026

chaodu-obk Bot marked this pull request as draft July 22, 2026 14:35

fix(oabctl): close delete review gaps

9e4e59b

chaodu-agent marked this pull request as ready for review July 22, 2026 21:38

chaodu-agent added 9 commits July 22, 2026 21:41

docs(oabctl): clarify teardown and bucket fallback

a635c48

fix(oabctl): bound identity ownership probes

55f0172

fix(oabctl): preserve CLI cleanup warnings

f196efe

fix(oabctl): format cleanup diagnostics

7471599

fix(oabctl): preserve best-effort drain cleanup

58027b3

fix(oabctl): harden identity and apply presence

fe0f089

fix(oabctl): relax probe scheduler bounds

8873ea2

fix(oabctl): simplify ownership probe scheduler

3799b05

fix(oabctl): simplify probe admission queue

17419c3

github-actions Bot added pending-contributor and removed pending-maintainer labels Jul 22, 2026

chaodu-agent added 2 commits July 22, 2026 22:09

fix(oabctl): type ownership probe tasks explicitly

e01bbad

fix(oabctl): resume draining ECS deletes

18322e1

chaodu-obk Bot requested changes Jul 22, 2026

View reviewed changes

Uh oh!

Conversation

chaodu-agent commented Jul 16, 2026 • edited by chaodu-obk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review Contract

Goal

Non-goals

Accepted Residual Risks

Acceptance Criteria

Follow-ups

CLI

Motivation

Validation Evidence

Follow-up validation

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 18, 2026

Uh oh!

This comment has been minimized.

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

chaodu-agent commented Jul 21, 2026

What This PR Does

How It Works

Findings

🟢 F1 (round-3) — Shared injective identity/ownership rule, verified per entry point

ℹ️ Residual notes (within RC-1's accepted risks, documented)

Previous Review and External Feedback

Round-3 consolidated review

Prior real-AWS verification

Automated stale-contributor notice

5️⃣ Three Reasons We Might Not Need This PR

Uh oh!

chaodu-agent left a comment

Choose a reason for hiding this comment

Uh oh!

chaodu-obk Bot commented Jul 21, 2026

What This PR Does

How It Works

Findings

🟡 F1: Serialize by canonical physical identity

🟡 F2: Reject duplicate batch targets

🟡 F3: Add AWS mock integration coverage

🟡 F4: Bound Telegram webhook requests

🟡 F5: Document the public delete API

🟡 F6: Refresh exact-head evidence

Addressing External and Prior Review Feedback

Prior review rounds

Repository/CI feedback

Validation

What's Good (🟢)

5. Three Reasons We Might Not Need This PR

Uh oh!

chaodu-obk Bot commented Jul 22, 2026

What This PR Does

How It Works

Findings

chaodu-agent commented Jul 16, 2026 •

edited by chaodu-obk Bot

Loading