From 25a286f0ee027b27c3ab887d4132dd5d3e87a536 Mon Sep 17 00:00:00 2001 From: Greg Allen Date: Tue, 16 Jun 2026 16:38:59 -0400 Subject: [PATCH] refactor(cli): migrate uninstall flows to harness-first agent discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uninstall commands (runUninstall and runGitHubUninstall) now discover agent slugs from harness wrapper files in the config repo before falling back to the config.yaml agents: block. A shared discoverAgentSlugs helper encapsulates the three-tier fallback chain (harness files → agents: block → caller default) and emits a deprecation warning when the legacy path is used. This is Phase 3, PR 5 of ADR-0045 (forge-portable harness schema). Signed-off-by: Greg Allen Signed-off-by: Claude Opus 4.6 Signed-off-by: Greg Allen --- internal/cli/admin.go | 33 ++--- internal/cli/admin_test.go | 63 ++++++++++ internal/cli/discover_slugs.go | 69 +++++++++++ internal/cli/discover_slugs_test.go | 185 ++++++++++++++++++++++++++++ internal/cli/github.go | 15 ++- internal/cli/github_test.go | 57 +++++++++ 6 files changed, 400 insertions(+), 22 deletions(-) create mode 100644 internal/cli/discover_slugs.go create mode 100644 internal/cli/discover_slugs_test.go diff --git a/internal/cli/admin.go b/internal/cli/admin.go index c9c99cc9e6..9756f3e219 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1598,30 +1598,35 @@ func runInstall(ctx context.Context, client forge.Client, printer *ui.Printer, o // runUninstall tears down the fullsend installation. func runUninstall(ctx context.Context, client forge.Client, printer *ui.Printer, org, appSet string, browser appsetup.BrowserOpener, stdin io.Reader) error { - // Try to load agent slugs from existing config. If the .fullsend repo - // is already gone (e.g., previous partial uninstall), fall back to the - // default naming convention so we can still guide the user to delete - // the apps. Without this fallback, a partial uninstall leaves orphaned - // apps that block reinstallation (PEM keys are one-shot). + // Try to discover agent slugs. Prefer harness wrapper files, then + // fall back to config.yaml agents: block, then default naming. + // If the .fullsend repo is already gone (e.g., previous partial + // uninstall), fall back to the default naming convention so we can + // still guide the user to delete the apps. Without this fallback, + // a partial uninstall leaves orphaned apps that block reinstallation + // (PEM keys are one-shot). var agentSlugs []string var configMode string var enrolledRepos []string + var parsedCfg *config.OrgConfig cfgData, err := client.GetFileContent(ctx, org, forge.ConfigRepoName, "config.yaml") if err == nil { - if parsedCfg, parseErr := config.ParseOrgConfig(cfgData); parseErr == nil { - for _, agent := range parsedCfg.Agents { - agentSlugs = append(agentSlugs, agent.Slug) - } - configMode = parsedCfg.Dispatch.Mode - enrolledRepos = parsedCfg.EnabledRepos() + if parsed, parseErr := config.ParseOrgConfig(cfgData); parseErr == nil { + parsedCfg = parsed + configMode = parsed.Dispatch.Mode + enrolledRepos = parsed.EnabledRepos() } else { printer.StepWarn(fmt.Sprintf("Could not parse existing config: %v; using defaults", parseErr)) } } + + agentSlugs = discoverAgentSlugs(ctx, client, org, forge.ConfigRepoName, "main", appSet, parsedCfg, printer) + if len(agentSlugs) == 0 { - // Config unavailable — assume default app naming convention and - // also include any legacy app-set prefixes so that apps created - // under an older version are not silently skipped. + // Neither harness files nor config agents found — assume default + // app naming convention and also include any legacy app-set + // prefixes so that apps created under an older version are not + // silently skipped. for _, role := range config.DefaultAgentRoles() { agentSlugs = append(agentSlugs, appsetup.AppSlug(appSet, role)) } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 14deaa0128..7c88a42484 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -1822,6 +1822,69 @@ func TestRunUninstall_NopBrowserSkipsBrowserOpen(t *testing.T) { assert.NotContains(t, output, "Could not open browser") } +func TestRunUninstall_UsesHarnessDiscovery(t *testing.T) { + client := forge.NewFakeClient() + client.TokenScopes = []string{"admin:org", "repo", "delete_repo"} + + // Provide config.yaml with agents: block (should be skipped in favor of harness). + client.FileContents = map[string][]byte{ + "test-org/.fullsend/config.yaml": []byte("version: v1\ndispatch:\n platform: github-actions\nagents:\n - role: triage\n slug: old-triage\n"), + } + // Provide harness directory with wrapper files. + client.DirContents = map[string][]forge.DirectoryEntry{ + "test-org/.fullsend/harness@main": { + {Path: "harness/triage.yaml", Type: "file"}, + {Path: "harness/coder.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "test-org/.fullsend/harness/triage.yaml@main": []byte("role: triage\nslug: my-triage\n"), + "test-org/.fullsend/harness/coder.yaml@main": []byte("role: coder\nslug: my-coder\n"), + } + + client.Installations = []forge.Installation{ + {ID: 1, AppSlug: "my-triage"}, + {ID: 2, AppSlug: "my-coder"}, + } + + var buf strings.Builder + printer := ui.New(&buf) + + err := runUninstall(context.Background(), client, printer, "test-org", "fullsend-ai", appsetup.NopBrowser{}, strings.NewReader("\n\n")) + require.NoError(t, err) + + output := buf.String() + // Should use harness-discovered slugs. + assert.Contains(t, output, "my-triage") + assert.Contains(t, output, "my-coder") + // Should NOT emit the deprecation warning about agents: block. + assert.NotContains(t, output, "agents: block") +} + +func TestRunUninstall_FallsBackToAgentsBlockWithWarning(t *testing.T) { + client := forge.NewFakeClient() + client.TokenScopes = []string{"admin:org", "repo", "delete_repo"} + + // Provide config.yaml with agents: block but no harness directory. + client.FileContents = map[string][]byte{ + "test-org/.fullsend/config.yaml": []byte("version: v1\ndispatch:\n platform: github-actions\nagents:\n - role: triage\n slug: cfg-triage\n"), + } + + client.Installations = []forge.Installation{ + {ID: 1, AppSlug: "cfg-triage"}, + } + + var buf strings.Builder + printer := ui.New(&buf) + + err := runUninstall(context.Background(), client, printer, "test-org", "fullsend-ai", appsetup.NopBrowser{}, strings.NewReader("\n")) + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "cfg-triage") + assert.Contains(t, output, "agents: block") +} + func TestAwaitRepoMaintenance_Success(t *testing.T) { client := forge.NewFakeClient() dispatchTime := time.Now().UTC().Add(-10 * time.Second) diff --git a/internal/cli/discover_slugs.go b/internal/cli/discover_slugs.go new file mode 100644 index 0000000000..26c0aef7f4 --- /dev/null +++ b/internal/cli/discover_slugs.go @@ -0,0 +1,69 @@ +package cli + +import ( + "context" + "fmt" + + "github.com/fullsend-ai/fullsend/internal/appsetup" + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/harness" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +// discoverAgentSlugs discovers agent slugs using a three-tier 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 { + 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)) + } + if len(agents) > 0 { + seen := make(map[string]bool, len(agents)) + var slugs []string + for _, a := range agents { + slug := a.Slug + if slug == "" && a.Role != "" { + slug = appsetup.AppSlug(appSet, a.Role) + } + if slug == "" { + continue + } + if !seen[slug] { + seen[slug] = true + slugs = append(slugs, slug) + } + } + if len(slugs) > 0 { + return slugs + } + } + + if cfg != nil && len(cfg.Agents) > 0 { + printer.StepWarn("agent identity read from config.yaml agents: block; migrate to harness files with role/slug fields") + var slugs []string + seen := make(map[string]bool, len(cfg.Agents)) + for _, a := range cfg.Agents { + slug := a.Slug + if slug == "" && a.Role != "" { + slug = appsetup.AppSlug(appSet, a.Role) + } + if slug != "" && !seen[slug] { + seen[slug] = true + slugs = append(slugs, slug) + } + } + if len(slugs) > 0 { + return slugs + } + } + + return nil +} diff --git a/internal/cli/discover_slugs_test.go b/internal/cli/discover_slugs_test.go new file mode 100644 index 0000000000..5fd58d4e29 --- /dev/null +++ b/internal/cli/discover_slugs_test.go @@ -0,0 +1,185 @@ +package cli + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func TestDiscoverAgentSlugs_HarnessFirst(t *testing.T) { + client := forge.NewFakeClient() + client.DirContents = map[string][]forge.DirectoryEntry{ + "acme/.fullsend/harness@main": { + {Path: "harness/triage.yaml", Type: "file"}, + {Path: "harness/coder.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "acme/.fullsend/harness/triage.yaml@main": []byte("role: triage\nslug: acme-triage\n"), + "acme/.fullsend/harness/coder.yaml@main": []byte("role: coder\nslug: acme-coder\n"), + } + + cfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Role: "triage", Slug: "old-triage"}, + }, + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", cfg, printer) + + require.Len(t, slugs, 2) + assert.Contains(t, slugs, "acme-triage") + assert.Contains(t, slugs, "acme-coder") + assert.NotContains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_FallsBackToAgentsBlock(t *testing.T) { + client := forge.NewFakeClient() + + cfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Role: "triage", Slug: "acme-triage"}, + {Role: "coder", Slug: "acme-coder"}, + }, + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", cfg, printer) + + require.Len(t, slugs, 2) + assert.Contains(t, slugs, "acme-triage") + assert.Contains(t, slugs, "acme-coder") + assert.Contains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_HarnessWithoutSlug_DerivesFromRole(t *testing.T) { + client := forge.NewFakeClient() + client.DirContents = map[string][]forge.DirectoryEntry{ + "acme/.fullsend/harness@main": { + {Path: "harness/triage.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "acme/.fullsend/harness/triage.yaml@main": []byte("role: triage\n"), + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", nil, printer) + + require.Len(t, slugs, 1) + assert.Equal(t, "fullsend-ai-triage", slugs[0]) + assert.NotContains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_ConfigAgentWithoutSlug_DerivesFromRole(t *testing.T) { + client := forge.NewFakeClient() + + cfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Role: "triage"}, + }, + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", cfg, printer) + + require.Len(t, slugs, 1) + assert.Equal(t, "fullsend-ai-triage", slugs[0]) + assert.Contains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_NeitherSource_ReturnsNil(t *testing.T) { + client := forge.NewFakeClient() + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", nil, printer) + + assert.Nil(t, slugs) + assert.NotContains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_DeduplicatesSlugs(t *testing.T) { + client := forge.NewFakeClient() + client.DirContents = map[string][]forge.DirectoryEntry{ + "acme/.fullsend/harness@main": { + {Path: "harness/coder.yaml", Type: "file"}, + {Path: "harness/fix.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "acme/.fullsend/harness/coder.yaml@main": []byte("role: coder\nslug: acme-coder\n"), + "acme/.fullsend/harness/fix.yaml@main": []byte("role: fix\nslug: acme-coder\n"), + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", nil, printer) + + require.Len(t, slugs, 1) + assert.Equal(t, "acme-coder", slugs[0]) +} + +func TestDiscoverAgentSlugs_EmptyAgentsBlock_ReturnsNil(t *testing.T) { + client := forge.NewFakeClient() + + cfg := &config.OrgConfig{ + Agents: []config.AgentEntry{}, + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", cfg, printer) + + assert.Nil(t, slugs) + assert.NotContains(t, buf.String(), "agents: block") +} + +func TestDiscoverAgentSlugs_PartialError_UsesValidAgents(t *testing.T) { + client := forge.NewFakeClient() + client.DirContents = map[string][]forge.DirectoryEntry{ + "acme/.fullsend/harness@main": { + {Path: "harness/triage.yaml", Type: "file"}, + {Path: "harness/broken.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "acme/.fullsend/harness/triage.yaml@main": []byte("role: triage\nslug: acme-triage\n"), + "acme/.fullsend/harness/broken.yaml@main": []byte("invalid: [yaml"), + } + + cfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Role: "triage", Slug: "old-triage"}, + }, + } + + var buf strings.Builder + printer := ui.New(&buf) + + slugs := discoverAgentSlugs(context.Background(), client, "acme", ".fullsend", "main", "fullsend-ai", cfg, printer) + + require.Len(t, slugs, 1) + assert.Equal(t, "acme-triage", slugs[0]) + assert.Contains(t, buf.String(), "some harness files could not be read") + assert.NotContains(t, buf.String(), "agents: block") +} diff --git a/internal/cli/github.go b/internal/cli/github.go index bfc4751995..a36e8babaf 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -819,20 +819,19 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr printer.Header("Uninstalling fullsend from " + org) printer.Blank() - // Read config before deleting repo to discover actual installed app slugs. + // Discover agent slugs: harness files first, then config.yaml agents: + // block, then default naming convention. var agentSlugs []string + var parsedCfg *config.OrgConfig cfgData, cfgErr := client.GetFileContent(ctx, org, forge.ConfigRepoName, "config.yaml") if cfgErr == nil { if parsed, parseErr := config.ParseOrgConfig(cfgData); parseErr == nil { - for _, agent := range parsed.Agents { - if agent.Slug != "" { - agentSlugs = append(agentSlugs, agent.Slug) - } else { - agentSlugs = append(agentSlugs, appsetup.AppSlug(appSet, agent.Role)) - } - } + parsedCfg = parsed } } + + agentSlugs = discoverAgentSlugs(ctx, client, org, forge.ConfigRepoName, "main", appSet, parsedCfg, printer) + if len(agentSlugs) == 0 { for _, role := range config.DefaultAgentRoles() { agentSlugs = append(agentSlugs, appsetup.AppSlug(appSet, role)) diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 99804e2c9d..86988ebc45 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -453,6 +453,63 @@ func TestRunGitHubUninstall_NoConfigRepo(t *testing.T) { require.NoError(t, err) } +func TestRunGitHubUninstall_UsesHarnessDiscovery(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{ + {Name: ".fullsend", FullName: "acme/.fullsend"}, + } + // Provide config.yaml with agents: block (should be bypassed). + client.FileContents = map[string][]byte{ + "acme/.fullsend/config.yaml": []byte("version: v1\ndispatch:\n platform: github-actions\nagents:\n - role: triage\n slug: old-triage\n"), + } + // Provide harness directory with wrapper files. + client.DirContents = map[string][]forge.DirectoryEntry{ + "acme/.fullsend/harness@main": { + {Path: "harness/triage.yaml", Type: "file"}, + }, + } + client.FileContentsRef = map[string][]byte{ + "acme/.fullsend/harness/triage.yaml@main": []byte("role: triage\nslug: harness-triage\n"), + } + client.Installations = []forge.Installation{ + {ID: 1, AppSlug: "harness-triage"}, + } + + var buf strings.Builder + printer := ui.New(&buf) + + err := runGitHubUninstall(context.Background(), client, printer, "acme", "fullsend-ai") + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "harness-triage") + assert.NotContains(t, output, "old-triage") + assert.NotContains(t, output, "agents: block") +} + +func TestRunGitHubUninstall_FallsBackToAgentsBlock(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{ + {Name: ".fullsend", FullName: "acme/.fullsend"}, + } + client.FileContents = map[string][]byte{ + "acme/.fullsend/config.yaml": []byte("version: v1\ndispatch:\n platform: github-actions\nagents:\n - role: triage\n slug: cfg-triage\n"), + } + client.Installations = []forge.Installation{ + {ID: 1, AppSlug: "cfg-triage"}, + } + + var buf strings.Builder + printer := ui.New(&buf) + + err := runGitHubUninstall(context.Background(), client, printer, "acme", "fullsend-ai") + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "cfg-triage") + assert.Contains(t, output, "agents: block") +} + // --- Sync-scaffold command tests --- func TestGitHubSyncScaffoldCmd_RequiresOrg(t *testing.T) {