refactor(config): remove legacy agent discovery fallbacks (ADR-0045 Phase 4 PR 3)

[ggallen]

Summary

• Delete loadKnownSlugsLegacy and the config.yaml agents: block fallback tier from loadKnownSlugs — harness wrapper files are now the sole discovery path
• Remove the cfg *config.OrgConfig parameter and tier-2 fallback from discoverAgentSlugs
• Simplify runUninstall and runGitHubUninstall callers to stop parsing config.yaml for discoverAgentSlugs
• Remove 3 legacy fallback test cases; update remaining tests to match new signatures

ADR: docs/ADRs/0045-forge-portable-harness-schema.md
Plan: docs/plans/adr-0045-forge-portable-harness-phase4.md — PR 3

Test plan

• make go-test — all tests pass
• make lint — clean
• go vet ./... — clean
• Verify loadKnownSlugs returns slugs from harness files
• Verify loadKnownSlugs returns nil when no harness files exist (no legacy fallback)
• Verify discoverAgentSlugs returns nil when no harness files exist
• Verify runUninstall falls back to DefaultAgentRoles() when harness discovery is empty
• Verify runGitHubUninstall falls back to DefaultAgentRoles() when harness discovery is empty
• grep -rn 'loadKnownSlugsLegacy' --include='*.go' returns no results

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Remove legacy config.yaml agent-discovery fallbacks; rely on harness wrappers only
✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Description

• Remove config.yaml agents: fallback tier from agent slug discovery.
• Simplify uninstall flows to use harness discovery, then default role naming.
• Update and delete tests to match the new discovery behavior and signatures.

Diagram

graph TD
  A["CLI uninstall"] --> B["runUninstall"] --> C["discoverAgentSlugs"] --> D["harness.DiscoverRemoteAgents"] --> E["harness/*.yaml"]
  A --> F["runGitHubUninstall"] --> C
  B --> G["DefaultAgentRoles()"]
  F --> G

  subgraph Legend
    direction LR
    _cli(["CLI command"]) ~~~ _fn["Go function"] ~~~ _fs["Repo files"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep legacy fallback behind a temporary feature flag

• ➕ Preserves backward compatibility for orgs that haven’t migrated harness wrappers yet
• ➕ Enables staged rollout and easier rollback if customers are surprised by behavior change
• ➖ Adds branching complexity and prolongs support for deprecated config schema
• ➖ Risk of the flag becoming permanent unless aggressively removed

2. Add an explicit migration error/warning when no harness files exist

• ➕ Makes the break in behavior obvious and actionable (points users to migration steps)
• ➕ Reduces silent reliance on DefaultAgentRoles() when harness discovery is empty
• ➖ More user-visible friction for uninstall flows that previously “just worked” via config.yaml
• ➖ Requires deciding when to warn vs. error in partially-uninstalled states

Recommendation: The PR’s approach is consistent with the ADR direction (single discovery source via harness wrappers) and meaningfully reduces ambiguity by removing config.yaml agents: fallback logic. Consider adding a targeted, user-facing warning when no harness wrappers are found (and DefaultAgentRoles() is used), to make migration issues discoverable without reintroducing the legacy fallback.

Files changed (6) +50 / -253

Refactor (3) +28 / -79

admin.goRemove legacy agents-block fallback from uninstall and slug loading +22/-42

Remove legacy agents-block fallback from uninstall and slug loading

• Updates runUninstall to stop passing parsed org config into slug discovery and clarifies that discovery is harness-only. Refactors loadKnownSlugs to return nil when no harness agents exist and deletes the legacy config.yaml agents-block reader and deprecation warnings.

internal/cli/admin.go

discover_slugs.goSimplify discoverAgentSlugs signature and remove config.yaml tier +4/-27

Simplify discoverAgentSlugs signature and remove config.yaml tier

• Removes the OrgConfig parameter and deletes the entire config.yaml agents-block fallback path. The helper now returns nil when harness discovery yields no slugs, leaving default-role fallback to the caller.

internal/cli/discover_slugs.go

github.goStop parsing config.yaml for GitHub uninstall slug discovery +2/-10

Stop parsing config.yaml for GitHub uninstall slug discovery

• Removes config.yaml parsing in runGitHubUninstall and calls discoverAgentSlugs using only harness discovery. Retains DefaultAgentRoles() fallback when discovery returns no slugs.

internal/cli/github.go

Tests (3) +22 / -174

admin_test.goUpdate admin uninstall and slug-loading tests for harness-only discovery +13/-91

Update admin uninstall and slug-loading tests for harness-only discovery

• Replaces the agents-block fallback uninstall test with a default-naming fallback test when no harness files exist. Removes or rewrites loadKnownSlugs tests that relied on config.yaml agents-block fallback; adjusts expectations to nil when harness discovery is empty/invalid.

internal/cli/admin_test.go

discover_slugs_test.goRemove config-based discovery tests and update calls for new signature +5/-78

Remove config-based discovery tests and update calls for new signature

• Deletes tests that asserted fallback to config.yaml agents-block and updates remaining tests to call discoverAgentSlugs without OrgConfig. Keeps coverage for harness-first behavior, derived slugs from role, deduplication, and partial harness read errors.

internal/cli/discover_slugs_test.go

github_test.goUpdate GitHub uninstall test to default naming when no harness files +4/-5

Update GitHub uninstall test to default naming when no harness files

• Replaces the agents-block fallback test case with a scenario where config.yaml exists but no harness files exist, asserting fallback to the default app slug naming convention.

internal/cli/github_test.go

[github-actions]

Site preview

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

Commit: 2c3be06518494296edd4c72d4e9dd1a0b0eecbe7

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:47 PM UTC · Completed 10:00 PM UTC
Commit: 2c3be06 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Shared Feature Toggle Strategy for HCC

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Uninstall proceeds on discovery error 🐞 Bug ☼ Reliability

Description

discoverAgentSlugs logs a warning on harness discovery errors but returns nil, and uninstall then
falls back to default slugs and proceeds to delete the .fullsend repo. If discovery failed
(transient API error/partial read) and the org’s apps don’t match the default naming, this can
orphan installed apps while removing the source-of-truth harness files needed to discover them
later.

Code

internal/cli/discover_slugs.go[R13-22]

+// discoverAgentSlugs discovers agent slugs from harness wrapper files in the
+// config repo. Returns nil when no slugs are found — the caller is responsible
+// for its own default-role fallback.
//
-//  1. Harness wrapper files in the config repo (via DiscoverRemoteAgents)
-//  2. config.yaml agents: block (legacy, emits deprecation warning)
-//  3. Empty — caller is responsible for its own default-role fallback
-//
-// The ref parameter specifies the git ref for harness directory discovery.
// When an agent has a role but no slug, the slug is derived from appSet and
// the role using the standard naming convention.
-func discoverAgentSlugs(ctx context.Context, client forge.Client, owner, configRepo, ref, appSet string, cfg *config.OrgConfig, printer *ui.Printer) []string {
+func discoverAgentSlugs(ctx context.Context, client forge.Client, owner, configRepo, ref, appSet string, printer *ui.Printer) []string {
	agents, err := harness.DiscoverRemoteAgents(ctx, client, owner, configRepo, ref)
	if err != nil {
		printer.StepWarn(fmt.Sprintf("some harness files could not be read: %v", err))

Evidence

discoverAgentSlugs cannot propagate discovery failure (warn-only), and runUninstall proceeds with
default fallback before uninstalling layers; the config repo layer’s uninstall deletes the repo,
removing harness wrappers that would otherwise be used to discover true slugs.

internal/cli/discover_slugs.go[19-46]
internal/cli/admin.go[1586-1628]
internal/cli/admin.go[1671-1675]
internal/layers/configrepo.go[120-143]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`discoverAgentSlugs` downgrades harness discovery errors to a warning and returns `nil`, causing uninstall flows to fall back to default naming and continue. Because uninstall deletes the config repo, a discovery failure can permanently remove the discovery source while still missing non-default app slugs.

### Issue Context
After this PR, harness wrapper files are the only supported discovery source. Treating discovery failures as non-fatal is riskier than before because there is no config.yaml agents-block fallback.

### Fix Focus Areas
- internal/cli/discover_slugs.go[13-46]
- internal/cli/admin.go[1586-1642]
- internal/cli/github.go[816-833]

### Suggested fix
Change `discoverAgentSlugs` to return `([]string, error)`.
- If `harness.DiscoverRemoteAgents` returns a non-nil error and produces zero usable slugs, return a non-nil error so uninstall can stop before deleting `.fullsend`.
- Update `runUninstall` and `runGitHubUninstall` to:
 - abort (or require explicit user confirmation) when discovery errored and no slugs were found,
 - only proceed with default fallback when discovery succeeded but legitimately found no harness directory/files.
This preserves the ability to retry uninstall without losing the harness-based identity source.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Legacy harness silently skipped 🐞 Bug ≡ Correctness

Description

Harness YAML files that omit both role and slug are silently ignored by remote discovery, so
loadKnownSlugs returns nil with no indication that a legacy harness schema exists and was skipped.
With the config.yaml agents: fallback removed, this can cause installs/uninstalls to incorrectly
fall back to defaults while giving the user no actionable migration signal.

Code

internal/cli/admin.go[R2025-2032]

func loadKnownSlugs(ctx context.Context, client forge.Client, org, configRepo, ref string, printer *ui.Printer) map[string]string {
	agents, err := harness.DiscoverRemoteAgents(ctx, client, org, configRepo, ref)
	if err != nil {
		printer.StepWarn(fmt.Sprintf("harness discovery: %v", err))
	}
-	if len(agents) > 0 {
-		slugs := make(map[string]string, len(agents))
-		seen := make(map[string]bool, len(agents))
-		for _, a := range agents {
-			if a.Role == "" && a.Slug == "" {
-				continue
-			}
-			if a.Role == "" || a.Slug == "" {
-				printer.StepWarn(fmt.Sprintf("harness %s has role=%q slug=%q; both must be set", a.Filename, a.Role, a.Slug))
-				continue
-			}
-			if seen[a.Role] {
-				printer.StepInfo(fmt.Sprintf("duplicate role %q in harness file %s, using first occurrence", a.Role, a.Filename))
-				continue
-			}
-			seen[a.Role] = true
-			slugs[a.Role] = a.Slug
+	if len(agents) == 0 {
+		return nil
+	}

Evidence

The CLI now returns nil immediately when discovery yields zero agents (no legacy fallback), and the
harness discovery implementation explicitly skips parsed YAMLs where both role and slug are empty
without producing an error, making legacy harness presence indistinguishable from “no harness
files”.

internal/cli/admin.go[2023-2054]
internal/harness/discover_remote.go[14-31]
internal/harness/discover_remote.go[51-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Remote harness discovery skips YAML files where both `role` and `slug` are empty, producing `(agents=[], err=nil)` even when harness files exist but are in the legacy schema. After this PR, `loadKnownSlugs` returns `nil` without any warning (no legacy config fallback), so callers fall back to defaults silently.

### Issue Context
`harness.DiscoverRemoteAgents` currently treats `role=="" && slug==""` as a normal “skip” case, which removes any chance for CLI flows to inform users that their harness wrappers are present but not yet migrated.

### Fix Focus Areas
- internal/harness/discover_remote.go[14-66]
- internal/cli/admin.go[2023-2054]

### Suggested fix
In `harness.DiscoverRemoteAgents`, when a YAML file parses successfully but has `Role=="" && Slug==""` **and** appears to contain legacy identity (e.g., `Agent` field is non-empty, or other non-zero fields), append an error like `"<file>: missing role/slug (legacy harness schema); migrate harness wrapper"` instead of silently skipping. This will make `err` non-nil so existing callers (`loadKnownSlugs`, `discoverAgentSlugs`) emit a warning without changing their signatures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov]

Codecov Report

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

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

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] internal/cli/admin.go — After this PR, loadKnownSlugs no longer falls back to config.yaml when harness files exist but none contain valid role/slug pairs. Orgs that defined agents only in the config.yaml agents: block (without migrating to harness files) will now see nil returned, causing callers to fall back to default naming convention. This could produce incorrect app slugs for orgs using custom slugs exclusively in config.yaml. This is the intended Phase 4 deprecation trade-off per ADR-0045 — not a bug.

---

Labels: PR removes legacy config.yaml agent discovery fallbacks from CLI install/uninstall commands as part of harness migration

[ggallen]

Bug 1: Uninstall proceeds on discovery error

Pre-existing behavior, unchanged by this PR. The old code also warned on discovery errors and fell through to defaults. The config.yaml fallback used the same forge client (client.GetFileContent), so a transient API error that prevents DiscoverRemoteAgents would also prevent reading config.yaml — both paths fail together. This PR removes a redundant fallback, not a safety net.

Bug 2: Legacy harness silently skipped

Also pre-existing and already addressed by the migration plan. The role=="" && slug=="" skip in loadKnownSlugs is unchanged. Phase 3 Lint() already warns on missing role, and Phase 4 PR 1 makes role a hard Validate() error. The migration signal exists; this PR does not remove it.

[ralphbean]

LGTM.

[waynesun09]

ADR-0045 Review Squad — 7 findings (medium+ severity). Reviewed for correctness, security, and ADR alignment.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:43 PM UTC · Completed 3:52 PM UTC
Commit: 2c3be06 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2448 — Remove legacy agent discovery fallbacks

Timeline

1. 2026-06-18 21:43 UTC — ggallen opens PR removing legacy config.yaml agent discovery fallbacks (ADR-0045 Phase 4 PR 3). Single commit: +50/−253 across 6 files.
2. 2026-06-18 21:44–21:50 — Automated reviewers run. qodo-code-review[bot] flags 2 bugs (discovery error silently swallowed during uninstall). fullsend-ai-review[bot] approves with 1 low edge-case finding.
3. 2026-06-18 22:17 — Author responds to Qodo findings, explaining the error-swallowing behavior is pre-existing and the removed config.yaml fallback used the same forge client (both paths would fail together on transient errors).
4. 2026-06-22 14:28 — ralphbean approves ("LGTM").
5. 2026-06-22 15:09–15:10 — waynesun09 posts 7 medium findings covering bugs (error silently swallowed in 3 places), design issues (dead code, misleading naming), and testing gaps.
6. 2026-06-22 15:38 — PR merged. Findings tracked in #2512.

Review quality gap

The fullsend review agent approved with 1 low finding while human and bot reviewers later identified 7+ medium-severity issues. From the JSONL trace, the correctness sub-agent traced the discoverAgentSlugs → nil-return → DefaultAgentRoles() fallback path and concluded the code was "safe" because nil is handled gracefully. It missed the semantic risk: during uninstall, proceeding with default slugs means deleting the .fullsend repo — permanently destroying the harness files that are now the sole discovery source.

The challenger pass then further filtered sub-agent findings, correctly catching false positives from stale base-branch reads but also discarding directionally-correct concerns about error handling.

Existing issues cover the gaps

The specific improvements needed are already tracked:

• #1393 — "Review agent should detect silent failure paths in Go functions" — directly covers the missed finding pattern. The sub-agent detected the path but didn't classify it as a silent failure because nil was handled without panic.
• #2485 — "Review orchestrator should accept challenger findings when backed by verifiable config evidence" — covers the challenger pass over-filtering behavior.
• #2320 — "Review agent should surface pre-existing inconsistencies relevant to the PR's change mechanism" — covers the pattern where removing a fallback elevates a pre-existing error-handling weakness.
• #1972 — "Review agent: require evidence before dismissing flagged findings as false positives" — related to the challenger pass dismissing valid concerns.

Assessment

The workflow outcome was acceptable — human reviewers caught the gaps and findings were tracked. The author's pushback was well-reasoned (the behavior is pre-existing, not introduced by this PR). However, the review agent's APPROVE verdict with only 1 low finding understated the risk surface of removing a fallback tier. No new proposals are needed since the existing issues, if implemented, would address the root causes.