-
Notifications
You must be signed in to change notification settings - Fork 61
fix(#2017): unenroll repos during uninstall via repo-maintenance workflow #2020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1638,13 +1638,15 @@ func runUninstall(ctx context.Context, client forge.Client, printer *ui.Printer, | |
| // apps that block reinstallation (PEM keys are one-shot). | ||
| var agentSlugs []string | ||
| var configMode string | ||
| var enrolledRepos []string | ||
| 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) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] architectural-violation The change passes enrolledRepos as the disabledRepos parameter to NewEnrollmentLayer during uninstall, but the approved design specification (docs/superpowers/specs/2026-04-18-enrollment-reconciliation-design.md, line 69) explicitly states that runUninstall should pass nil for disabledRepos. The design spec should be updated before or alongside the implementation change. Suggested fix: Update the design spec to document the rationale for passing enrolled repos during uninstall, then implement the code change. Alternatively, follow the existing spec and pass nil. |
||
| configMode = parsedCfg.Dispatch.Mode | ||
| enrolledRepos = parsedCfg.EnabledRepos() | ||
| } else { | ||
| printer.StepWarn(fmt.Sprintf("Could not parse existing config: %v; using defaults", parseErr)) | ||
| } | ||
|
|
@@ -1694,15 +1696,14 @@ func runUninstall(ctx context.Context, client forge.Client, printer *ui.Printer, | |
| } | ||
|
|
||
| // Build a minimal stack for uninstall. | ||
| // Only ConfigRepoLayer matters for uninstall since other layers are no-ops. | ||
| emptyCfg := config.NewOrgConfig(nil, nil, nil, nil, "") | ||
| stack := layers.NewStack( | ||
| layers.NewConfigRepoLayer(org, client, emptyCfg, printer, false), | ||
| layers.NewWorkflowsLayer(org, client, printer, "", version), | ||
| layers.NewSecretsLayer(org, client, nil, printer), | ||
| layers.NewInferenceLayer(org, client, nil, printer), | ||
| dispatchLayer, | ||
| layers.NewEnrollmentLayer(org, client, nil, nil, printer), | ||
| layers.NewEnrollmentLayer(org, client, nil, enrolledRepos, printer), | ||
| ) | ||
|
|
||
| if err := runPreflight(ctx, stack, layers.OpUninstall, client, printer); err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/fullsend-ai/fullsend/internal/config" | ||
| "github.com/fullsend-ai/fullsend/internal/forge" | ||
| "github.com/fullsend-ai/fullsend/internal/ui" | ||
| ) | ||
|
|
@@ -53,7 +54,10 @@ func (l *EnrollmentLayer) RequiredScopes(op Operation) []string { | |
| // Enrollment dispatches repo-maintenance.yml on .fullsend. | ||
| return []string{"repo"} | ||
| case OpUninstall: | ||
| return nil // no-op | ||
| if len(l.disabledRepos) > 0 { | ||
| return []string{"repo"} | ||
| } | ||
| return nil | ||
| case OpAnalyze: | ||
| return []string{"repo"} | ||
| default: | ||
|
|
@@ -176,9 +180,88 @@ func (l *EnrollmentLayer) reportPRByTitle(ctx context.Context, repo, title strin | |
| } | ||
| } | ||
|
|
||
| // Uninstall is a no-op. Individual repo cleanup is not automated — | ||
| // repos keep their shim workflows. | ||
| func (l *EnrollmentLayer) Uninstall(_ context.Context) error { | ||
| // Uninstall updates config.yaml to mark all repos as disabled and | ||
| // dispatches the repo-maintenance workflow to create unenrollment PRs | ||
| // that remove shim workflows from enrolled repos. This runs before | ||
| // ConfigRepoLayer deletes the .fullsend repo (layers uninstall in | ||
| // reverse order), so the workflow is still available to dispatch. | ||
| // | ||
| // Errors during unenrollment are non-fatal — the user is informed but | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] scope-creep The approved design spec (line 62) states Uninstall stays a no-op and the alternatives-considered section (line 100) explicitly rejected Go-side unenrollment in EnrollmentLayer. This PR implements the rejected approach. The approach still dispatches the workflow rather than doing Go-side unenrollment directly, which partially addresses the rejection reason. Suggested fix: Either revert EnrollmentLayer.Uninstall() to remain a no-op, or update the design spec first to document why the original decision should be reversed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] coherence-drift The Uninstall method always returns nil, swallowing all errors as warnings. This bypasses Stack.UninstallAll() error-collection mechanism. The user is informed via StepWarn but the error is not propagated to the caller. |
||
| // the uninstall continues. Repos that cannot be unenrolled | ||
| // automatically will need manual removal of .github/workflows/fullsend.yaml. | ||
| func (l *EnrollmentLayer) Uninstall(ctx context.Context) error { | ||
| if len(l.disabledRepos) == 0 { | ||
| l.ui.StepInfo("no repositories to unenroll") | ||
| return nil | ||
| } | ||
|
|
||
| // Read current config and mark all repos as disabled so the | ||
| // reconcile script knows to create unenrollment PRs. | ||
| cfgData, err := l.client.GetFileContent(ctx, l.org, forge.ConfigRepoName, "config.yaml") | ||
| if err != nil { | ||
| if forge.IsNotFound(err) { | ||
| l.ui.StepInfo("config repo unavailable, skipping unenrollment") | ||
| return nil | ||
| } | ||
| l.ui.StepWarn(fmt.Sprintf("could not read config for unenrollment: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| cfg, err := config.ParseOrgConfig(cfgData) | ||
| if err != nil { | ||
| l.ui.StepWarn(fmt.Sprintf("could not parse config for unenrollment: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| for name, rc := range cfg.Repos { | ||
| rc.Enabled = false | ||
| cfg.Repos[name] = rc | ||
| } | ||
|
|
||
| data, err := cfg.Marshal() | ||
| if err != nil { | ||
| l.ui.StepWarn(fmt.Sprintf("could not marshal config for unenrollment: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| l.ui.StepStart("Updating config to disable all repos") | ||
| err = l.client.CreateOrUpdateFile(ctx, l.org, forge.ConfigRepoName, "config.yaml", | ||
| "chore: disable all repos for uninstall", data) | ||
| if err != nil { | ||
| l.ui.StepWarn(fmt.Sprintf("could not update config: %v", err)) | ||
| return nil | ||
| } | ||
| l.ui.StepDone("Disabled all repos in config") | ||
|
|
||
| // Dispatch repo-maintenance to create unenrollment PRs. | ||
| dispatchTime := time.Now().UTC().Add(-30 * time.Second) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] race condition / ordering If awaitWorkflowRun times out (~3 minutes), the method logs a warning and returns nil. The workflow may still be running when ConfigRepoLayer.Uninstall subsequently deletes the .fullsend repo, killing the in-progress workflow run and preventing unenrollment PRs from being created. |
||
| l.ui.StepStart("Dispatching repo-maintenance for unenrollment") | ||
| err = l.client.DispatchWorkflow(ctx, l.org, forge.ConfigRepoName, repoMaintenanceWorkflow, "main", nil) | ||
| if err != nil { | ||
| l.ui.StepWarn(fmt.Sprintf("could not dispatch unenrollment workflow: %v", err)) | ||
| l.ui.StepInfo("repos may need manual cleanup of .github/workflows/fullsend.yaml") | ||
| return nil | ||
| } | ||
| l.ui.StepDone("Dispatched repo-maintenance for unenrollment") | ||
|
|
||
| // Wait for the workflow run to complete. | ||
| run, err := l.awaitWorkflowRun(ctx, dispatchTime) | ||
| if err != nil { | ||
| l.ui.StepWarn(fmt.Sprintf("could not confirm unenrollment: %v", err)) | ||
| l.ui.StepInfo("check the repo-maintenance workflow in .fullsend for results") | ||
| return nil | ||
| } | ||
|
|
||
| if run.Conclusion == "success" { | ||
| l.ui.StepDone("Unenrollment completed successfully") | ||
| } else { | ||
| l.ui.StepWarn(fmt.Sprintf("unenrollment workflow completed with conclusion: %s", run.Conclusion)) | ||
| l.showWorkflowLogs(ctx, run.ID) | ||
| } | ||
| l.ui.StepInfo(fmt.Sprintf("workflow run: %s", run.HTMLURL)) | ||
|
|
||
|
ralphbean marked this conversation as resolved.
|
||
| l.reportReconciliationPRs(ctx) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,17 +155,119 @@ func TestEnrollmentLayer_Install_WorkflowWarning(t *testing.T) { | |
| assert.Contains(t, output, "conclusion: failure") | ||
| } | ||
|
|
||
| func TestEnrollmentLayer_Uninstall_Noop(t *testing.T) { | ||
| func TestEnrollmentLayer_Uninstall_NoRepos(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test adequacy No test covers the case where enabledRepos is non-empty but disabledRepos is empty (layer constructed with enabled repos but no disabled repos during uninstall). |
||
| client := &forge.FakeClient{} | ||
| layer, _ := newEnrollmentLayer(t, client, []string{"repo-a"}, nil) | ||
| layer, buf := newEnrollmentLayer(t, client, nil, nil) | ||
|
|
||
| err := layer.Uninstall(context.Background()) | ||
| require.NoError(t, err) | ||
|
|
||
| output := buf.String() | ||
| assert.Contains(t, output, "no repositories to unenroll") | ||
| } | ||
|
|
||
| func TestEnrollmentLayer_Uninstall_DisablesAndDispatches(t *testing.T) { | ||
| now := time.Now().UTC() | ||
|
|
||
| // Seed config.yaml with an enabled repo. | ||
| cfgYAML := `version: "1" | ||
| dispatch: | ||
| platform: github-actions | ||
| defaults: | ||
| roles: [triage] | ||
| max_implementation_retries: 2 | ||
| auto_merge: false | ||
| agents: [] | ||
| repos: | ||
| repo-a: | ||
| enabled: true | ||
| repo-b: | ||
| enabled: true | ||
| ` | ||
| client := &forge.FakeClient{ | ||
| FileContents: map[string][]byte{ | ||
| "test-org/.fullsend/config.yaml": []byte(cfgYAML), | ||
| }, | ||
| WorkflowRuns: map[string]*forge.WorkflowRun{ | ||
| "test-org/.fullsend/repo-maintenance.yml": { | ||
| ID: 42, | ||
| Status: "completed", | ||
| Conclusion: "success", | ||
| CreatedAt: now.Add(time.Minute).Format(time.RFC3339), | ||
| HTMLURL: "https://github.com/test-org/.fullsend/actions/runs/42", | ||
| }, | ||
| }, | ||
| PullRequests: map[string][]forge.ChangeProposal{ | ||
| "test-org/repo-a": { | ||
| {Title: "chore: disconnect from fullsend agent pipeline", URL: "https://github.com/test-org/repo-a/pull/10"}, | ||
| }, | ||
| "test-org/repo-b": { | ||
| {Title: "chore: disconnect from fullsend agent pipeline", URL: "https://github.com/test-org/repo-b/pull/11"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| layer, buf := newEnrollmentLayer(t, client, nil, []string{"repo-a", "repo-b"}) | ||
|
|
||
| err := layer.Uninstall(context.Background()) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Empty(t, client.CreatedBranches) | ||
| assert.Empty(t, client.CreatedFiles) | ||
| assert.Empty(t, client.CreatedProposals) | ||
| assert.Empty(t, client.DeletedRepos) | ||
| output := buf.String() | ||
| assert.Contains(t, output, "Disabled all repos in config") | ||
| assert.Contains(t, output, "Dispatched repo-maintenance for unenrollment") | ||
| assert.Contains(t, output, "Unenrollment completed successfully") | ||
| assert.Contains(t, output, "repo-a/pull/10") | ||
| assert.Contains(t, output, "repo-b/pull/11") | ||
|
|
||
| // Verify config was updated with all repos disabled. | ||
| require.Len(t, client.CreatedFiles, 1) | ||
| assert.Equal(t, "config.yaml", client.CreatedFiles[0].Path) | ||
| assert.Contains(t, string(client.CreatedFiles[0].Content), "enabled: false") | ||
| assert.NotContains(t, string(client.CreatedFiles[0].Content), "enabled: true") | ||
| } | ||
|
|
||
| func TestEnrollmentLayer_Uninstall_ConfigNotFound(t *testing.T) { | ||
| client := &forge.FakeClient{ | ||
| FileContents: map[string][]byte{}, | ||
| } | ||
| layer, buf := newEnrollmentLayer(t, client, nil, []string{"repo-a"}) | ||
|
|
||
| err := layer.Uninstall(context.Background()) | ||
| require.NoError(t, err) | ||
|
|
||
| output := buf.String() | ||
| assert.Contains(t, output, "config repo unavailable") | ||
| } | ||
|
|
||
| func TestEnrollmentLayer_Uninstall_DispatchError(t *testing.T) { | ||
| cfgYAML := `version: "1" | ||
| dispatch: | ||
| platform: github-actions | ||
| defaults: | ||
| roles: [triage] | ||
| max_implementation_retries: 2 | ||
| auto_merge: false | ||
| agents: [] | ||
| repos: | ||
| repo-a: | ||
| enabled: true | ||
| ` | ||
| client := &forge.FakeClient{ | ||
| FileContents: map[string][]byte{ | ||
| "test-org/.fullsend/config.yaml": []byte(cfgYAML), | ||
| }, | ||
| Errors: map[string]error{ | ||
| "DispatchWorkflow": assert.AnError, | ||
| }, | ||
| } | ||
| layer, buf := newEnrollmentLayer(t, client, nil, []string{"repo-a"}) | ||
|
|
||
| err := layer.Uninstall(context.Background()) | ||
| require.NoError(t, err) // non-fatal | ||
|
|
||
| output := buf.String() | ||
| assert.Contains(t, output, "could not dispatch unenrollment workflow") | ||
| assert.Contains(t, output, "manual cleanup") | ||
| } | ||
|
ralphbean marked this conversation as resolved.
|
||
|
|
||
| func TestEnrollmentLayer_Analyze_AllEnrolled(t *testing.T) { | ||
|
|
||
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.
[medium] intent-misalignment
The variable enrolledRepos is extracted via EnabledRepos() (repos with enabled: true) but passed as the disabledRepos parameter. The semantic inversion is not documented.