setup and uninstall: rely on deck's sweep, reject app names - #444
Conversation
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
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. 📝 WalkthroughWalkthroughThe 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. ChangesDeck app setup and uninstall
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🟡 Moderate · up to 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
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
…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>
ecdb46a to
d995658
Compare
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 setupstops registering them one by one, andrt uninstallcan 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
servecatalog 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)gitqHasReposand theirmkdirpmrstoboardadoptiondeck adoptanswerschanged: true, so a board row deck's sweep already made is left alonex-local-caller: rt, the registrar deck's structural gate expectsrt setup applyUninstall
rt uninstallrejects any argument outside its five flags withunexpected-args(exit 2), before a dry run or promptdeck.managed-removemakes onePOST /api/v1/apps/managed/removeinstead of a per-name loop that depended on deck ignoring the nameservices.unregister, which stops deck itselfstayed, while the other flavor's app is installed, since both decks share one registryAlso
DEFAULT_EXPOSED, the gitq deps.lock row and thestandalone:gitqpreflight row are untouchedVerification
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 e2esetup.test.ts, run in CI.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
rt uninstallnow rejects app names and other unexpected arguments before prompting or performing checks, with a clear error message.Documentation
rt uninstallremoves all mattstack apps and does not accept an app name.