refactor(cli): migrate uninstall flows to harness-first agent discovery #43
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)) | ||
|
There was a problem hiding this comment. [critical] api-contract discoverAgentSlugs calls harness.DiscoverRemoteAgents(ctx, client, owner, configRepo, ref) but this function does not exist in the harness package. The only discovery function is harness.DiscoverAgents(dir string), which operates on a local filesystem directory. This PR will not compile. Suggested fix: Add harness.DiscoverRemoteAgents with the appropriate signature to the harness package, or change the call site to use an existing API. |
||
| } | ||
|
There was a problem hiding this comment. [low] error-handling Warning message 'some harness files could not be read' is misleading for wholesale failures (network error, repo not found) vs per-file parse errors. |
||
| if len(agents) > 0 { | ||
|
There was a problem hiding this comment. [medium] test-inadequate No test covers the case where DiscoverRemoteAgents returns a non-nil error with zero valid agents. This is a distinct code path (err != nil but len(agents) == 0) that differs from the no-harness-directory case because the warning is still emitted. Suggested fix: Add a test where DiscoverRemoteAgents returns an error and no valid agents, verifying fallthrough to config.yaml with warning. |
||
| seen := make(map[string]bool, len(agents)) | ||
| var slugs []string | ||
| for _, a := range agents { | ||
| slug := a.Slug | ||
| if slug == "" && a.Role != "" { | ||
|
There was a problem hiding this comment. [low] test-inadequate No test covers the case where a harness file produces an agent with both role and slug empty. |
||
| 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] test-inadequate
Both runUninstall and runGitHubUninstall pass hardcoded ref 'main' to discoverAgentSlugs. No test verifies behavior with non-main default branches.