fix(cli): open correct uninstall URL and wait for user confirmation

[rh-hemartin]

Summary

• Open /organizations/{org}/settings/installations/{id} instead of /organizations/{org}/settings/apps/{slug}/advanced — the /advanced page requires app owner access; org admins installing a third-party app need the installation settings page (Uninstall doesn't clean everything up (part 2) #1490 item 2)
• After opening each app's installation page, prompt the user to press Enter to confirm the app was uninstalled before continuing, so the command no longer exits immediately with a misleading success message (admin uninstall exits immediately after opening browser for app deletion #323 / Uninstall doesn't clean everything up (part 2) #1490 item 5)
• Rename "App cleanup" section header to "App uninstall"

Relates to #1490
Closes #323

Test plan

• Run fullsend admin uninstall <org> against an org with fullsend installed as a third-party app
• Verify browser opens to /settings/installations/{id} (not /settings/apps/.../advanced)
• Verify command pauses and waits for Enter after each app page is opened
• Verify fallback to /settings/installations list when installation ID is unavailable

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://a30e3bb0-site.fullsend-ai.workers.dev

Commit: 07f902fab2a6086aca8c72a9055c573e4bcf56f1

[fullsend-ai-review]

Review

Findings

Low

• [stale-reference] internal/cli/github.go:936 — The parallel runGitHubUninstall function (reachable via fullsend github uninstall) still uses the old /settings/apps/{slug}/advanced URL pattern and "App cleanup" header. This is the same bug this PR fixes in runUninstall, but in a separate code path. Consider updating github.go in a follow-up PR.

• [missing-test] internal/cli/admin_test.go — No test covers the new post-uninstall verification logic (the second ListOrgInstallations call after user confirmation). Since FakeClient returns the same Installations slice on every call, all existing tests silently hit the "Some fullsend apps are still installed" warning path. A test that mutates client.Installations between calls (or injects an error) would verify both the success and warning paths.

Info

• [test-integrity] internal/cli/admin_test.go:1680 — In TestRunUninstall_LegacySlugsIncludedWhenConfigUnavailable, the assertion assert.NotContains(t, output, "App fullsend-coder not found") is now vacuously true because the new code no longer emits that message. The test still validates its core intent via other assertions.

Previous run

Review

Findings

No findings.

Previous run (2)

Review

Findings

No findings.

Previous run (3)

Review

Findings

Low

• [correctness] internal/cli/admin.go:1700 — Diagnostic messages removed when installation lookup fails. The old code warned "Could not verify which apps exist; opening all" when ListOrgInstallations returned an error, and printed "App {slug} not found, skipping" for individual misses. Both messages were removed, making the fallback behavior silent. Consider restoring a warning when listErr != nil so users understand why they are being directed to the generic installations list page instead of individual installation pages.

• [correctness] internal/cli/admin_test.go:1561 — Post-uninstall verification logic is untested. The new code calls ListOrgInstallations a second time after user confirmation to check whether apps were actually removed, but no test exercises this path. With FakeClient, the Installations slice is immutable between calls, so the verification always reports "Some fullsend apps are still installed." Consider adding a test that either (a) asserts the warning message appears (documenting the FakeClient limitation), or (b) uses a FakeClient callback to simulate mutation so the success path ("Apps were uninstalled") is also covered.

Info

• [docs-currency] docs/guides/admin/installation.md:286 — The uninstall section does not describe the new interactive confirmation behavior (press Enter after each app) or the post-uninstall verification step. The current wording is not misleading but could be expanded in a follow-up to set user expectations for the interactive flow.

Previous run (4)

Review

Findings

Low

• [correctness] internal/cli/admin.go:1678 — "App uninstall" header is printed unconditionally before the if len(agentSlugs) > 0 check. Previously, "App cleanup" was only shown when existingSlugs > 0. When all apps are already removed, the header now appears with no corresponding action beneath it. Consider moving it back inside the if len(existingSlugs) > 0 block.

• [correctness] internal/cli/admin.go:1717 — os.Stdin is hardcoded, making the new confirmation behavior untestable. Existing tests pass because os.Stdin returns EOF immediately in test environments (and EOF is correctly handled), but no test can verify the prompt-and-wait behavior. Consider accepting an io.Reader parameter to enable testing.

[fullsend-ai-review]

[low] correctness

Diagnostic messages removed when installation lookup fails. The old code warned "Could not verify which apps exist; opening all" when ListOrgInstallations returned an error, and printed "App {slug} not found, skipping" for individual misses. Both were removed, making the fallback behavior silent.

Suggested fix: Consider restoring a warning when listErr != nil so users understand why they are directed to the generic installations list page.

[fullsend-ai-review]

[low] correctness

Post-uninstall verification logic is untested. The new code calls ListOrgInstallations a second time to check whether apps were removed, but no test covers this path. With FakeClient, installations are immutable, so verification always reports still-installed.

Suggested fix: Add a test that asserts the warning message appears, or use a FakeClient callback to simulate mutation so the success path is also covered.

[ralphbean]

LGTM. One non-blocking note inline about the rebase.

[ralphbean]

[non-blocking] Heads up — this branch is behind main and GitHub shows it as conflicting. During the rebase you'll run into the BrowserOpener injection that landed on main recently (plus slug dedup logic and two new tests). I think the cleanest path is to keep both browser and stdin as parameters so tests can still use NopBrowser and CI doesn't try to open a real browser.

[fullsend-ai-retro]

Retro: PR #1573 — fix(cli): open correct uninstall URL and wait for user confirmation

Workflow outcome: Good. Human-authored PR with useful review agent findings that drove iterative improvements before merge.

Timeline

1. May 27 08:18 — Human (rh-hemartin) opens PR fixing uninstall URL and adding user confirmation (3 files, +102/−75). Closes admin uninstall exits immediately after opening browser for app deletion #323, relates to Uninstall doesn't clean everything up (part 2) #1490.
2. May 27 08:24 — Review run 1 (single-agent, ~2M tokens, 6 min) → APPROVE with 2 low findings: header printed unconditionally, os.Stdin hardcoded.
3. May 27 08:46–08:58 — Human pushes 4 fix commits addressing review findings (stdin injection, post-uninstall verification, diagnostic warnings).
4. May 27 08:49–09:04 — Review runs 2–3 triggered by rapid pushes → both APPROVE. Run 2 found 2 more low findings (removed diagnostics, untested verification).
5. May 28 07:54 — Review run 4 after another push → APPROVE.
6. Jun 6 19:13 — Human reviewer (ralphbean) approves with non-blocking rebase note.
7. Jun 8 06:26 — Review run 5 (multi-agent architecture, ~7.8M tokens, 11 min) → APPROVE. Found stale github.go reference using old URL pattern.
8. Jun 8 09:28 — PR merged.

What went well

• Review agent caught real correctness issues (testability, unconditional header, removed diagnostics, stale cross-file reference) that improved code quality.
• Human responded to review findings promptly, addressing 4 of them within 12 minutes.
• The multi-agent review run (run 5) found a cross-file stale reference in github.go that the simpler single-agent runs missed — demonstrating the value of broader coverage.

Cost concerns (already tracked)

• 5 review runs totaling ~12M+ tokens for a small PR. Rapid-fire pushes (commits 4–7 within 12 min) triggered overlapping reviews. Already tracked by Debounce review dispatch on rapid synchronize events #1014 (debounce), Add concurrency group to dispatch-review job in fullsend.yaml #981 (concurrency groups), Deduplicate review runs on rapid successive pushes #1418/Deduplicate review runs when PR is rebased multiple times in quick succession #1422 (dedup on rapid pushes).
• Run 5 used 4x more tokens than run 1 due to the multi-agent architecture upgrade. Combined with 5 runs per PR, this amplifies the importance of debouncing and incremental review optimizations tracked in Pass prior review findings to follow-up reviews for targeted verification #1552 and Investigate empty PRIOR_REVIEW_SHA causing redundant full re-reviews #1447.
• E2E tests failed 3 of 5 runs — likely flaky, tracked by Track and reduce E2E test flakiness in CI #1879.

No new proposals

All identified improvement opportunities are already covered by existing open issues. No new proposals warranted.