Skip to content

setup and uninstall: rely on deck's sweep, reject app names - #444

Merged
m4ttheweric merged 8 commits into
mainfrom
rt-2-13-e-setup-cli-cleanup
Sep 25, 2026
Merged

m4ttheweric merged 8 commits into
mainfrom
rt-2-13-e-setup-cli-cleanup

Conversation

@m4ttheweric

@m4ttheweric m4ttheweric commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Setup and uninstall stop guessing which apps exist (RT-281, RT-284)

Deck's boot sweep now owns which bundled apps it serves, so rt setup stops registering them one by one, and rt uninstall can no longer be aimed at one app while it removes everything.

Merge gate met: the deps.lock PR pinning deck 1.1.0 with the serve catalog merged as #448 (bd91b21).

Dev flavor (ruled 2026-09-25 by axel, standing in for Matt): deck's sweep creates missing catalog rows in both flavors, so a dev-only machine still gets board, chat, console and boxscore once setup stops registering them. Conditions, implemented in deck 1.1.0 (m4ttstack/apps#154): a row is missing only when no row of that name exists under any owner (no dev adopt, no duplicate); dev creates a row only when its bundle ships Helpers/<name>, otherwise deck raises a board issue; a created row stores no bundle-absolute command.

What changed

Setup (lib/setup/steps/deck.ts)

  • Removes the gitq, console and chat legs, gitqHasRepos and their mkdirp
  • Keeps the legacy mrs to board adoption
  • Repoints board only when deck adopt answers changed: true, so a board row deck's sweep already made is left alone
  • The repoint PATCH sends x-local-caller: rt, the registrar deck's structural gate expects
  • A refused repoint lands in the step's detail instead of halting rt setup apply

Uninstall

  • rt uninstall rejects any argument outside its five flags with unexpected-args (exit 2), before a dry run or prompt
  • deck.managed-remove makes one POST /api/v1/apps/managed/remove instead of a per-name loop that depended on deck ignoring the name
  • The action now runs before services.unregister, which stops deck itself
  • It skips, and lists deck's apps in stayed, while the other flavor's app is installed, since both decks share one registry
  • A partial teardown, a non-200, no answer or a malformed reply fails the action; a partial teardown's remedy names the kept apps
  • Title is now "Remove mattstack's apps from deck"

Also

  • The command reference, the DEBUG tray stub and the setup contract doc follow
  • DEFAULT_EXPOSED, the gitq deps.lock row and the standalone:gitq preflight row are untouched

Verification

Targeted, all green: steps-b.test.ts (59), lib/setup/__tests__/uninstall.test.ts (53), commands/__tests__/uninstall.test.ts (24), stub-rt/stub.test.ts (18), contract.test.ts (7), tsc --noEmit, picker:check, docs:check. The managed remove route and its { ok, removed, failed } reply already exist on the pinned deck 1.0.7, so the uninstall half is safe before and after the pin bump. Full suites, including e2e setup.test.ts, run in CI.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • rt uninstall now rejects app names and other unexpected arguments before prompting or performing checks, with a clear error message.
    • Uninstallation now removes managed apps from Deck before unregistering services, while preserving shared app records when another app flavor is installed.
    • Setup now limits Deck adoption to the board app and handles repointing only when adoption changes the existing record.
  • Documentation

    • Clarified that rt uninstall removes all mattstack apps and does not accept an app name.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 91d93821-440d-4d2f-bc67-3fe27c88bdad

📥 Commits

Reviewing files that changed from the base of the PR and between bd91b21 and d995658.

📒 Files selected for processing (12)
  • commands/__tests__/uninstall.test.ts
  • commands/uninstall.ts
  • docs/superpowers/specs/2026-08-21-rt-setup-contract.md
  • e2e/tests/setup.test.ts
  • lib/command-tree-def.ts
  • lib/setup/__tests__/steps-b.test.ts
  • lib/setup/__tests__/uninstall.test.ts
  • lib/setup/steps/deck.ts
  • lib/setup/uninstall.ts
  • rt-tray/Tests/stub-rt/stub.test.ts
  • rt-tray/Tests/stub-rt/stub.ts
  • website/docs/reference/uninstall.mdx

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

The uninstall command now rejects positional arguments. Deck setup adopts and repoints the legacy board record. Uninstall removes Deck-managed apps before service unregistration, using one API request and validating the response.

Changes

Deck app setup and uninstall

Layer / File(s) Summary
Uninstall command contract
commands/uninstall.ts, commands/__tests__/uninstall.test.ts, lib/command-tree-def.ts, e2e/tests/setup.test.ts, website/docs/reference/uninstall.mdx
The command accepts only its declared flags and rejects positional app names. Tests and descriptions reflect that uninstall covers all mattstack apps.
Board adoption and repointing
lib/setup/steps/deck.ts, lib/setup/__tests__/steps-b.test.ts
The Deck setup step adopts legacy mrs as board and repoints it only after a changed adoption. Tests cover adoption, repoint, skip, and failure outcomes.
Managed-app uninstall ordering and request
lib/setup/uninstall.ts, lib/setup/__tests__/uninstall.test.ts, docs/superpowers/specs/2026-08-21-rt-setup-contract.md, e2e/tests/setup.test.ts, rt-tray/Tests/stub-rt/*
Uninstall places managed-app removal before service unregistration. The handler skips when the other flavor’s app is installed or Deck is unhealthy; otherwise, it posts to Deck and validates the response. Tests, the contract, and the tray stub reflect the action order and outcomes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d9956

No PR-introduced issue is established that should block merge. Existing uninstall limitations remain for an unhealthy Deck or an installation without a resolvable Deck CLI.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d9956

The changes narrow accidental uninstall commands, but they also make app removal depend on Deck’s bulk-removal behavior and make a refused board update non-retryable through the normal setup path. No security exploit is established; the ownership and recovery guarantees need confirmation.

Retained concerns

  • Medium · security · inferred: The new bulk removal sends no app, owner, or flavor identity. Its caller-side check protects an installed other flavor, but protection of shared registry rows when that check does not match depends on Deck’s unverified endpoint-side ownership rules.
  • Medium · reliability · inferred: After a successful legacy rename, a refused board PATCH is reported within a completed setup step. A rerun that receives unchanged adoption skips the PATCH, so this path cannot repair the stored command unless Deck’s separate sweep does so.
Security review details

Security Blast Radius

  • inferred — The independently reachable scope shown here is a local uninstall invocation and its loopback Deck requests. The material destructive boundary is Deck’s shared app registry, not a demonstrated remote entrypoint.

Security Findings and Attack Paths

  • inferred — A mistaken broad selection by Deck’s bulk endpoint could remove records belonging to the other flavor when its app-presence guard does not match. The available evidence does not show that the endpoint makes such a selection or establish an exploit.

Trust Boundaries and Controls

  • observed — The command rejects stray arguments before uninstall begins, and the removal client checks for an installed other flavor before sending its bodyless POST.
  • inferred — The registrar header identifies the setup caller to Deck, but caller code and a test fake cannot establish how production Deck authenticates that identity or limits modification of the board record.

Resilience and Maintainability Implications

  • observed — Malformed, non-200, and reported partial-removal results fail before service unregistration. An unhealthy Deck instead produces a skip, allowing uninstall to continue; that unhealthy-Deck skip predates this PR.

Hardening Proposals

  • proposed — Confirm Deck’s managed-remove owner selection and idempotency after partial commit or timeout, including behavior for stale rows and simultaneous flavor changes.
  • proposed — Confirm that Deck’s sweep repairs a board record after a refused repoint, or preserve a retry path for that update.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 10 files. (2 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two main changes: setup relies on Deck’s managed-app sweep, and uninstall rejects app names.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 10 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@m4ttheweric

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@m4ttheweric
m4ttheweric marked this pull request as ready for review September 25, 2026 09:56
m4ttheweric and others added 8 commits September 25, 2026 04:56
…undle's catalog

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… everything

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… removes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… registrar

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…; name kept apps in the remedy

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ollow the new order

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@m4ttheweric
m4ttheweric force-pushed the rt-2-13-e-setup-cli-cleanup branch from ecdb46a to d995658 Compare September 25, 2026 09:56
@m4ttheweric
m4ttheweric merged commit 55a50f7 into main Sep 25, 2026
7 checks passed
@m4ttheweric
m4ttheweric deleted the rt-2-13-e-setup-cli-cleanup branch September 25, 2026 10:09
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.

1 participant