refactor(cli): migrate uninstall flows to harness-first agent discovery - #43
refactor(cli): migrate uninstall flows to harness-first agent discovery#43guyoron1 wants to merge 9 commits into
Conversation
|
/fs-qf |
|
🤖 Finished Review · ✅ Success · Started 10:45 AM UTC · Completed 10:56 AM UTC |
ReviewFindingsCritical
Medium
Low
Info
Previous runReviewReason: stale-head The review agent reviewed commit Previous run (2)ReviewReason: stale-head The review agent reviewed commit Previous run (3)ReviewReason: stale-head The review agent reviewed commit |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:59 AM UTC · Completed 11:12 AM UTC |
Fix 8 major and 3 minor findings from automated STP review: - Replace internal function names with user-facing language in Section III - Fix Go version to 1.26+ (matching go.mod) - Replace [Functional] tier labels with [Tier 1] classification - Rebalance priority distribution (P0: 44%, P1: 33%, P2: 22%) - Add GH-43 requirement IDs to all Section III groups - Reformat Section IV to Reviewers/Approvers template format - Clarify Dependencies (upstream only, not internal packages) - Remove empty risk entries and sandbox LSP limitation - Improve Developer Handoff, Enhancement links, and cross-references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:15 AM UTC · Completed 11:30 AM UTC |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:33 AM UTC · Completed 11:46 AM UTC |
| 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.
| if err != nil { | ||
| printer.StepWarn(fmt.Sprintf("some harness files could not be read: %v", err)) | ||
| } | ||
| 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.
| 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.
[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.
| printer.Blank() | ||
|
|
||
| // Read config before deleting repo to discover actual installed app slugs. | ||
| // Discover agent slugs: harness files first, then config.yaml agents: |
There was a problem hiding this comment.
[low] test-regression-risk
No integration test for role-to-slug derivation in runGitHubUninstall after delegation to discoverAgentSlugs.
| } else { | ||
| printer.StepWarn(fmt.Sprintf("Could not parse existing config: %v; using defaults", parseErr)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] edge-case
Behavioral change: old code appended empty slugs from config agents, new code correctly skips them. This improvement should be noted in the PR description.
| 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.
| // (PEM keys are one-shot). | ||
| var agentSlugs []string | ||
| var configMode string | ||
| var enrolledRepos []string |
There was a problem hiding this comment.
[low] test-inadequate
Both runUninstall and runGitHubUninstall pass hardcoded ref 'main' to discoverAgentSlugs. No test verifies behavior with non-main default branches.
Generate Ginkgo v2 test implementations from STD YAML for GH-43 (Migrate Uninstall Flows to Harness-First Agent Discovery). 9 test scenarios covering: - discoverAgentSlugs three-tier fallback (6 scenarios) - Org-level and GitHub-specific uninstall integration (3 scenarios)
QualityFlow Pipeline Summary
Test Output
Issue: GH-43 Generated by QualityFlow |
Mirror of upstream fullsend-ai#2364 for QF pipeline demo.
Migrates uninstall flows from legacy slug-file lookup to harness-first agent discovery, with 6 files changed including new discover_slugs module and updated tests.