refactor(harness): migrate loadKnownSlugs to harness-first discovery

[ggallen]

Summary

• ADR-0045 Phase 3, PR 4: Migrates loadKnownSlugs() in internal/cli/admin.go to prefer harness wrapper files over the config.yaml agents: block for agent identity discovery.
• Renames the existing function to loadKnownSlugsLegacy and introduces a new loadKnownSlugs that calls harness.DiscoverRemoteAgents first, falling back to the legacy path with a deprecation warning.
• Duplicate roles in harness files are handled deterministically (first in sort order wins).

Test plan

• TestLoadKnownSlugs_HarnessFilesPreferred — harness files present → uses harness slugs, no deprecation warning
• TestLoadKnownSlugs_FallbackToAgentsBlock — no harness dir → falls back to config.yaml, emits deprecation warning
• TestLoadKnownSlugs_HarnessFilesWithoutRoleSlug_FallsBack — harness files without role/slug → falls back to config.yaml
• TestLoadKnownSlugs_NeitherSource_ReturnsNil — no harness files, no config.yaml → returns nil
• TestLoadKnownSlugs_DuplicateRoles_FirstWins — duplicate roles in harness files → first entry (sorted order) wins
• make lint passes
• go vet ./internal/cli/ clean
• go test ./internal/cli/ all tests pass

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://534e1cb5-site.fullsend-ai.workers.dev

Commit: f902ef876bc9ffcc0c63fb3b4566ba7f361dcabe

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:05 PM UTC · Completed 8:17 PM UTC
Commit: 21feb10 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

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

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Low

• [edge-case] internal/cli/admin.go:2017 — An agent entry with Role set but Slug empty (or vice versa) is returned by DiscoverRemoteAgents (which skips entries where both are empty) but silently dropped by loadKnownSlugs (which skips entries where either is empty). If all discovered agents have this partial-field pattern, the function falls through to legacy without any warning explaining why harness files were insufficient. Consider logging a warning when an agent has exactly one of role/slug set, as this likely indicates a misconfigured harness file.
• [function-comment-style] internal/cli/admin.go:2013 — The loadKnownSlugs function comment includes implementation details (function name in parentheses) that are unusual for this codebase's godoc style. Consider simplifying to match sibling functions.
• [scope-alignment] internal/cli/admin.go:2014 — Implementation hard-codes forge.ConfigRepoName and "HEAD" instead of accepting them as parameters as specified in the phase plan. Pragmatic for a single call site, but reduces reusability if a second call site emerges.
• [scope-alignment] internal/cli/admin.go:2024 — The phase plan specifies debug-level logging for duplicate roles; implementation uses printer.StepWarn, which may be noisy in well-configured environments with intentional overrides.
• [warning-message-consistency] internal/cli/admin.go:2039 — The deprecation warning uses imperative style ("migrate to") which differs from the descriptive tone of other StepWarn messages in this file.

Info

• [edge-case] internal/cli/admin.go:2020 — When DiscoverRemoteAgents returns agents where every entry has an empty Role or Slug, the code silently falls through to loadKnownSlugsLegacy. This is intentional fallback behavior.
• [parameter-ordering] internal/cli/admin.go:2013 — The signature places printer as the last parameter (ctx, client, org, printer), differing from loadRepoConfig which uses (ctx, client, printer, org). Acceptable given mixed ordering elsewhere in the file.
• [architectural-coherence] internal/cli/admin.go:2014 — The implementation correctly follows ADR-0045 Phase 3 migration strategy: harness-first discovery, config.yaml fallback, deprecation warning.
• [naming-convention] internal/cli/admin.go:2045 — loadKnownSlugsLegacy follows the established Legacy suffix pattern in the codebase.

Previous run (2)

Review

Findings

Medium

• [error-handling-gap] internal/cli/admin.go:2015 — DiscoverRemoteAgents returns ([]AgentInfo, error) where a non-nil error can accompany valid agents (partial success — e.g., 2 of 3 harness files parse successfully, 1 fails). The new loadKnownSlugs discards this error entirely with agents, _ := .... In the partial-success case, the function returns an incomplete slug map (missing the failed file's role) and never falls back to the legacy path, silently losing agent identity. Even in the total-failure case (non-nil error, nil agents), the error is lost with no logging.
  Remediation: At minimum, log the error via printer.StepWarn when err != nil. For partial success (len(agents) > 0 && err != nil), decide whether partial data is acceptable or whether it should fall through to the legacy path.

Low

• [deviation-from-design] internal/cli/admin.go:2020 — The phase 3 plan (line 210 of adr-0045-forge-portable-harness-phase3.md) specifies: "Log at debug level when a duplicate role is encountered." The implementation silently skips duplicate roles without any logging, making it harder to diagnose misconfigured harness files where two files accidentally claim the same role.
• [missing-authorization] internal/cli/admin.go — Non-trivial change has no linked issue. The PR references ADR-0045 Phase 3 PR 4, which provides architectural authorization via the accepted ADR and its phase plan. Consider linking a tracking issue for traceability.

Info

• [edge-case] internal/cli/admin.go:2027 — When DiscoverRemoteAgents returns agents but all have empty Role or Slug, the code falls through to loadKnownSlugsLegacy. The return value behavior differs slightly from the old code path (empty map vs nil), but this is benign — both are handled identically at the call site.
• [scope-alignment] internal/cli/admin.go:2008 — The implementation correctly follows the Phase 3 PR 4 specification. Scope is well-controlled.
• [architectural-coherence] internal/cli/admin.go:2008 — The change aligns with ADR-0045's Phase 3 (Deprecate) migration path.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:42 PM UTC · Completed 8:53 PM UTC
Commit: 5f459cd · View workflow run →

[ralphbean]

A couple things inline.

[ralphbean]

LGTM. My earlier concern about the silent skip of role-without-slug entries is moot — PR #2364 replaces this with discoverAgentSlugs, which derives the slug from appSet+role instead of dropping the entry.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:18 AM UTC · Completed 12:27 AM UTC
Commit: f902ef8 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:20 AM UTC · Completed 1:29 AM UTC
Commit: f902ef8 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2361 — refactor(harness): migrate loadKnownSlugs to harness-first discovery

Verdict: workflow went well, no new proposals.

This human-authored PR went through 3 review iterations, all productive:

1. First review caught a real medium bug — DiscoverRemoteAgents error was silently discarded. Author fixed it.
2. Second review downgraded all findings to low (style, scope alignment). Human reviewer (ralphbean) added two inline comments reinforcing an edge-case concern about role-without-slug entries.
3. Third review approved cleanly after the author addressed all bot + human findings with new tests and parameterized the function signature.

The review bot and human reviewer complemented each other well — the bot caught the error-handling gap first, the human added the UX perspective on silent failures.

Efficiency notes:

• 12 pull_request_review shim runs fired (7 from author response comments), most resolving to "no stage matched." This noise is already tracked by #1125.
• Re-reviewing after human approval is already tracked by #963.
• Review comment history stacking is tracked by #2358.

No new proposals — existing issues cover the minor inefficiencies observed.