Add delete command to bb apps - #281
Conversation
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. The new logical-delete command follows the existing Apps control-plane boundary and is non-graphical CLI work, but its confirmation does not cover the environment that selects the destructive target, and an indeterminate response can report failure after deletion has already committed. All 11 supplied GitHub check runs passed; the supplied combined commit status remains pending with no status contexts, so required checks still independently govern merge readiness.
Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| .help("Repeat the exact app identifier to confirm logical deletion"), | ||
| ) | ||
| .arg( | ||
| Arg::new("environment") |
There was a problem hiding this comment.
🤖 P1 · Confirm the complete delete target (blocking)
The command requires an exact repeat of APP_ID, but the request also accepts an optional environment that selects which app route is retired. Because confirmation validates only APP_ID, a stale, mistyped, or implicitly selected environment can change the destructive target without invalidating consent.
User effect: Someone can successfully confirm one app name yet take that app offline in the wrong environment.
Recommended fix: Bind confirmation to the complete resolved target. Require an explicit environment and include it in the value being confirmed, or remove the environment override so the confirmed app ID uniquely identifies the target.
Test: Add a discriminating command test proving that deletion cannot be sent when the confirmed environment differs from the requested environment, and that an omitted environment cannot leave the destructive target implicit.
There was a problem hiding this comment.
🤖 addressed in d9ed047. --environment is now required, and --confirm-environment must exactly match it before auth or network access. I added CLI and process coverage for omitted and mismatched environments.
| app_id: &str, | ||
| request: &DeleteAppRequest<'_>, | ||
| ) -> Result<Value> { | ||
| let url = self.app_url(app_id, &[])?; |
There was a problem hiding this comment.
🤖 P1 · Handle indeterminate delete outcomes (blocking)
The DELETE uses the ordinary JSON request path. If the control plane commits deletion but the response is lost, times out, is truncated, or cannot be parsed, the CLI returns a generic failure even though the active route may already be retired. The new command supplies no idempotency, reconciliation, or outcome-unknown recovery behavior for this destructive boundary.
User effect: Someone can be told deletion failed while their app is already offline, leading them to retry or continue under the false assumption that it is still serving.
Recommended fix: Use a server-supported idempotency mechanism or reconcile the app through status/include-deleted after an indeterminate response. If reconciliation is unavailable, return a stable outcome-unknown error that tells the caller to verify status before retrying.
Test: Add a test where the server receives and commits the DELETE but disconnects or returns an unreadable response, and assert that the CLI reports an indeterminate outcome with a safe verification path rather than a definitive failure.
There was a problem hiding this comment.
🤖 addressed in d9ed047. DELETE transport, body-read, and successful-response parse failures now return stable delete_outcome_unknown guidance to verify with bb apps get before retrying. I added a test where the server receives DELETE and returns an unreadable 200 response.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE. The current diff requires explicit confirmation of both app ID and environment before credential access or network dispatch, and it gives indeterminate transport and success-response failures a stable verification path. The remaining concern that a server-side 5xx can still represent an indeterminate delete outcome is the same underlying issue as an existing unresolved automation thread with a substantive author reply, so it is suppressed rather than republished. All 10 supplied GitHub check runs passed; the supplied combined commit status remains pending with no status contexts, so required checks still independently govern merge readiness.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
Summary
The Apps CLI can create, deploy, inspect, and roll back apps, but owners still cannot retire an app from
bb. This adds:bb apps delete <APP_ID> --environment <ENVIRONMENT> --confirm-app-id <APP_ID> --confirm-environment <ENVIRONMENT>The command maps to
DELETE /v1/agent/apps/{app_id}through the existing allowlisted control-plane client. Because deletion retires the active route, both parts of the target are explicit and must be confirmed exactly before the CLI reads credentials or sends a request. Compose remains responsible for owner authorization and logical-delete behavior: uploaded versions, artifacts, and stack resources are retained, and the full server response is preserved.A valid non-success response is returned normally. If the request may have reached the server but the CLI cannot read or parse a complete success response, it returns the stable
delete_outcome_unknownerror and tells the caller to verify the same app and environment withbb apps getbefore retrying.Related issue
N/A. No matching open issue or PR found.
Testing
At commit
d9ed0475caa628fc17294839b24c74186d76cfc7:cd bb-cli && source ../bin/activate-hermit && just check(149 unit, 78 bb E2E, 22 CLI E2E, and 6 acceptance tests)Live deletion was not run because the local Builderlab CLI session is intentionally logged out and deleting an app requires an explicit real target. The route, request body, owner-only behavior, retention response, and reconciliation path were verified against the current Compose control-plane contract and source, then covered with mocked process and client tests.