Skip to content

Add delete command to bb apps - #281

Merged
nathan-thillairajah merged 2 commits into
mainfrom
codex/apps-delete
Sep 3, 2026
Merged

Add delete command to bb apps#281
nathan-thillairajah merged 2 commits into
mainfrom
codex/apps-delete

Conversation

@nathan-thillairajah

@nathan-thillairajah nathan-thillairajah commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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_unknown error and tells the caller to verify the same app and environment with bb apps get before 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)
  • repository pre-commit and pre-push hooks (frontend checks, Tauri checks, formatting, and Clippy)

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.

@nathan-thillairajah
nathan-thillairajah requested a review from a team September 2, 2026 21:14

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread bb-cli/src/bb/apps.rs
.help("Repeat the exact app identifier to confirm logical deletion"),
)
.arg(
Arg::new("environment")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread bb-cli/src/bb/apps.rs
app_id: &str,
request: &DeleteAppRequest<'_>,
) -> Result<Value> {
let url = self.app_url(app_id, &[])?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

@nathan-thillairajah
nathan-thillairajah merged commit c5058ae into main Sep 3, 2026
10 checks passed
@nathan-thillairajah
nathan-thillairajah deleted the codex/apps-delete branch September 3, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants