refactor(cli): migrate uninstall flows to harness-first agent discovery

[ggallen]

Summary

• Add discoverAgentSlugs helper (internal/cli/discover_slugs.go) that encapsulates three-tier agent slug discovery: harness wrapper files → config.yaml agents: block → caller default (e.g., DefaultAgentRoles() convention)
• Migrate runUninstall (admin.go) and runGitHubUninstall (github.go) to call discoverAgentSlugs before falling back to legacy app-set naming
• Emit deprecation warning when slugs are read from the agents: block instead of harness files

This is Phase 3, PR 5 of ADR-0045 (forge-portable harness schema). It depends on PR 2 (remote harness discovery via DiscoverRemoteAgents).

Dependency graph

PR 2 (remote harness discovery) ──> PR 5 (migrate uninstall) ──> PR 6

Test plan

• TestDiscoverAgentSlugs_HarnessFirst — harness files preferred over agents: block
• TestDiscoverAgentSlugs_FallsBackToAgentsBlock — falls back with deprecation warning
• TestDiscoverAgentSlugs_HarnessWithoutSlug_FallsBack — harness with role but no slug triggers fallback
• TestDiscoverAgentSlugs_NeitherSource_ReturnsNil — both empty returns nil
• TestDiscoverAgentSlugs_DeduplicatesSlugs — duplicate slugs across harnesses are deduplicated
• TestDiscoverAgentSlugs_EmptyAgentsBlock_ReturnsNil — empty agents block doesn't trigger warning
• TestRunUninstall_UsesHarnessDiscovery — uninstall prefers harness slugs
• TestRunUninstall_FallsBackToAgentsBlockWithWarning — uninstall fallback with warning
• TestRunGitHubUninstall_UsesHarnessDiscovery — GitHub uninstall prefers harness slugs
• TestRunGitHubUninstall_FallsBackToAgentsBlock — GitHub uninstall fallback with warning
• All existing uninstall tests pass (legacy slugs, dedup, no-apps-found, etc.)
• make go-test — full suite passes
• make lint — clean

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://7e543980-site.fullsend-ai.workers.dev

Commit: 25a286f0ee027b27c3ab887d4132dd5d3e87a536

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:23 PM UTC
Commit: f06928a · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/discover_slugs.go	93.75%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

High

• [error-handling-gap] internal/cli/discover_slugs.go:21 — discoverAgentSlugs checks err == nil && len(agents) > 0 but DiscoverRemoteAgents intentionally returns valid agents alongside a non-nil multi-error when individual harness files fail to parse or fetch. When any single harness file is malformed, err is non-nil, so the condition fails and ALL valid harness-discovered agents are silently dropped, falling through to the config.yaml agents: block fallback. This contradicts the design intent of DiscoverRemoteAgents which explicitly states "valid files are still returned alongside the error".
  Remediation: Change the condition to if len(agents) > 0 (ignore the error for branching purposes). Optionally log the partial error via printer.StepWarn so the user knows some harness files had issues, but still use the successfully-parsed agents.

Medium

• [behavior-regression] internal/cli/github.go:840 — The old runGitHubUninstall code synthesized a slug for agents that had a role but no slug field (appsetup.AppSlug(appSet, agent.Role)). The new discoverAgentSlugs helper skips agents with empty slugs entirely. Agents installed under the derived-slug naming convention but having no explicit slug in their config will be missed during uninstall, leaving orphaned GitHub Apps. Note: this regression is specific to github.go; the old admin.go code also did not derive slugs from roles.
  Remediation: In discoverAgentSlugs, when an agent has a Role but no Slug, derive the slug using the same convention the installer would have used (e.g., appsetup.AppSlug). This requires passing the appSet parameter into discoverAgentSlugs. Alternatively, handle empty-slug agents at each call site after discoverAgentSlugs returns.

Low

• [test-integrity] internal/cli/discover_slugs_test.go:71 — No test covers the partial-error scenario where DiscoverRemoteAgents returns valid agents AND an error. This would have caught the high-severity error-handling-gap finding above.
• [error-handling] internal/cli/discover_slugs.go:19 — Function silently discards harness discovery errors. Once the branching logic is fixed (see high finding), consider logging partial errors via printer.StepWarn so users know some harness files had issues.
• [function-comment-accuracy] internal/cli/discover_slugs.go:18 — Comment says "ref parameter specifies the git ref to read from the config repo" but the ref parameter is only used for harness discovery (passed to DiscoverRemoteAgents), not for config.yaml reading (which comes from the already-parsed cfg parameter).
• [scope-completeness] internal/cli/discover_slugs_test.go:18 — Verify tests match Phase 3 plan spec, particularly the partial-error scenario.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:43 PM UTC · Completed 8:55 PM UTC
Commit: 25a286f · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:25 AM UTC · Completed 12:35 AM UTC
Commit: 25a286f · View workflow run →

[fullsend-ai-retro]

Retro: PR #2364 — migrate uninstall flows to harness-first agent discovery

Verdict: Clean workflow, no new proposals needed.

Timeline

1. 20:07 UTC — Human (ggallen) opened PR refactor(cli): migrate uninstall flows to harness-first agent discovery #2364, a refactoring PR for Phase 3 PR 5 of ADR-0045
2. 20:08–20:23 UTC — Review run 1 completed with CHANGES_REQUESTED:
   • 1 HIGH (error-handling-gap): err == nil && len(agents) > 0 condition silently drops valid agents when DiscoverRemoteAgents returns partial errors — a genuine bug catch
   • 1 MEDIUM (behavior-regression): agents with role but no slug missed during uninstall — posted only in summary comment, not inline (out-of-diff location)
   • 4 LOW: test-integrity, error-handling, function-comment-accuracy, scope-completeness
3. 20:30 UTC — Human responded to all inline findings with fixes
4. 20:39 UTC — Human pushed updated commit
5. 20:43–20:55 UTC — Review run 2 approved with "Looks good to me"
6. 21:01 UTC — Human reviewer (ralphbean) approved
7. 00:20 UTC — PR merged

What went well

• Review agent caught a real bug. The HIGH error-handling-gap finding was a legitimate correctness issue that could cause silent data loss in production. This is high-value review work.
• Fast rework cycle. One round of review → fix → re-review, completed in ~30 minutes. Minimal friction.
• 95% patch coverage. Only 2 lines missing coverage, and the human added a partial-error test that the review agent specifically requested.

Minor gaps observed (already tracked)

• The MEDIUM behavior-regression finding was only in the summary comment, not posted inline. This is a known GitHub API limitation for out-of-diff findings, tracked by #1352 and #1446.
• The re-review did not explicitly confirm whether the MEDIUM finding was addressed (it collapsed prior findings into a <details> section). This is covered by #1552 (pass prior findings to follow-up reviews for targeted verification).

No new proposals warranted — existing issues cover the observed gaps.