From ef67af933ce11bdb262118b81d51c0473b920d87 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Jul 2026 06:36:41 +0000 Subject: [PATCH 1/3] Initial plan From 509fb3cc758ee3e648e3e5c15b7cbb2ac67d5116 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Jul 2026 06:55:12 +0000 Subject: [PATCH 2/3] Fix pre-install bootstrap config ordering in add-wizard Run repo-variable and repo-secret bootstrap steps before engine selection so required configuration is collected up front, matching what TestTuistoryAddWizardManifestBootstrapRunsBeforeEngineSelection expects. - Add isPreInstallBootstrapType helper (repo-variable, repo-secret) - Add splitBootstrapProfile to partition config into pre/post phases - Execute pre-install steps (Step 5b) before selectAIEngineAndKey - Add TestSplitBootstrapProfile unit tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 20 ++++- pkg/cli/bootstrap_config.go | 48 ++++++++++ pkg/cli/bootstrap_profile_runner_test.go | 107 ++++++++++++++++++++++- 3 files changed, 169 insertions(+), 6 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 4617933eec6..60adf52d7c2 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -134,10 +134,22 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error if config.resolvedWorkflows != nil { bootstrapProfile = config.resolvedWorkflows.BootstrapProfile } - // All config steps run post-install in the exact order they are declared in the - // manifest. We no longer split them into a pre-install and post-install phase so - // that the declared ordering is preserved. - remainingBootstrapProfile := bootstrapProfile + // Split bootstrap config into pre-install (repo-variable, repo-secret) and + // post-install phases. Pre-install steps collect configuration values before + // engine selection; post-install steps run after the workflow PR is created. + preInstallBootstrapProfile, remainingBootstrapProfile := splitBootstrapProfile(bootstrapProfile) + + // Step 5b: Run pre-install bootstrap config steps (repo-variable, repo-secret) + // before engine selection so required configuration is collected up front. + if preInstallBootstrapProfile != nil { + if config.hasWriteAccess { + if err := executeBootstrapConfigForAdd(ctx, config.RepoOverride, config.WorkflowSpecs, preInstallBootstrapProfile, config.UseCopilotRequests, config.Verbose); err != nil { + return err + } + } else { + printBootstrapConfigTODO(os.Stderr, preInstallBootstrapProfile) + } + } // Step 6: Select coding agent and collect API key if err := config.selectAIEngineAndKey(); err != nil { diff --git a/pkg/cli/bootstrap_config.go b/pkg/cli/bootstrap_config.go index 2ade8076f51..eb62949886b 100644 --- a/pkg/cli/bootstrap_config.go +++ b/pkg/cli/bootstrap_config.go @@ -11,6 +11,54 @@ import ( "github.com/github/gh-aw/pkg/gitutil" ) +// isPreInstallBootstrapType reports whether the config action type should run +// before engine selection and workflow installation in the add-wizard flow. +func isPreInstallBootstrapType(actionType string) bool { + switch actionType { + case "repo-variable", "repo-secret": + return true + default: + return false + } +} + +// splitBootstrapProfile splits a resolved bootstrap profile into pre-install and +// post-install parts for the add-wizard flow. Pre-install actions (repo-variable +// and repo-secret) run before engine selection; all remaining actions run after +// the workflow PR has been created. +// +// Either returned value may be nil if there are no actions in that phase. +func splitBootstrapProfile(profile *resolvedBootstrapProfile) (preInstall, postInstall *resolvedBootstrapProfile) { + if profile == nil || profile.Profile == nil || len(profile.Profile.Config) == 0 { + return nil, nil + } + + var preActions, postActions []repositoryPackageBootstrapAction + for _, action := range profile.Profile.Config { + if isPreInstallBootstrapType(action.Type) { + preActions = append(preActions, action) + } else { + postActions = append(postActions, action) + } + } + + if len(preActions) > 0 { + preInstall = &resolvedBootstrapProfile{ + PackageID: profile.PackageID, + Source: profile.Source, + Profile: &repositoryPackageBootstrap{Config: preActions}, + } + } + if len(postActions) > 0 { + postInstall = &resolvedBootstrapProfile{ + PackageID: profile.PackageID, + Source: profile.Source, + Profile: &repositoryPackageBootstrap{Config: postActions}, + } + } + return preInstall, postInstall +} + // printBootstrapConfigTODO prints a TODO checklist of manual steps required by the // config entries in the package manifest. Called by the non-interactive // "add" command after workflows have been installed. diff --git a/pkg/cli/bootstrap_profile_runner_test.go b/pkg/cli/bootstrap_profile_runner_test.go index 209cbd3a07b..73212e6fc95 100644 --- a/pkg/cli/bootstrap_profile_runner_test.go +++ b/pkg/cli/bootstrap_profile_runner_test.go @@ -74,8 +74,8 @@ func TestBootstrapProfileState(t *testing.T) { // TestPrintBootstrapConfigTODO_PreservesManifestOrder verifies that all declared config // actions are included in the TODO output in their exact manifest order. This guards -// against regressions that split actions into pre-install and post-install phases and -// thereby reorder or omit steps. +// against regressions to printBootstrapConfigTODO that would reorder or omit steps; +// note that the add-wizard flow intentionally splits pre-install and post-install actions. func TestPrintBootstrapConfigTODO_PreservesManifestOrder(t *testing.T) { // Declare a profile with an interleaved mix of action types to ensure ordering // cannot be accidentally satisfied by any implicit categorisation. @@ -132,3 +132,106 @@ func TestPrintBootstrapConfigTODO_PreservesManifestOrder(t *testing.T) { } } } + +func TestSplitBootstrapProfile(t *testing.T) { + tests := []struct { + name string + profile *resolvedBootstrapProfile + wantPreTypes []string + wantPostTypes []string + }{ + { + name: "nil profile", + profile: nil, + wantPreTypes: nil, + wantPostTypes: nil, + }, + { + name: "empty config", + profile: &resolvedBootstrapProfile{Profile: &repositoryPackageBootstrap{}}, + wantPreTypes: nil, + wantPostTypes: nil, + }, + { + name: "only pre-install types", + profile: &resolvedBootstrapProfile{ + PackageID: "owner/repo", + Profile: &repositoryPackageBootstrap{ + Config: []repositoryPackageBootstrapAction{ + {Type: "repo-variable", Name: "MY_VAR"}, + {Type: "repo-secret", Name: "MY_SECRET"}, + }, + }, + }, + wantPreTypes: []string{"repo-variable", "repo-secret"}, + wantPostTypes: nil, + }, + { + name: "only post-install types", + profile: &resolvedBootstrapProfile{ + PackageID: "owner/repo", + Profile: &repositoryPackageBootstrap{ + Config: []repositoryPackageBootstrapAction{ + {Type: "handoff", Message: "done"}, + {Type: "copilot-auth", Secret: "TOKEN"}, + }, + }, + }, + wantPreTypes: nil, + wantPostTypes: []string{"handoff", "copilot-auth"}, + }, + { + name: "mixed pre and post install types", + profile: &resolvedBootstrapProfile{ + PackageID: "owner/repo", + Profile: &repositoryPackageBootstrap{ + Config: []repositoryPackageBootstrapAction{ + {Type: "repo-variable", Name: "MY_VAR"}, + {Type: "handoff", Message: "done"}, + {Type: "repo-secret", Name: "MY_SECRET"}, + {Type: "require-owner-type", Value: "org"}, + }, + }, + }, + wantPreTypes: []string{"repo-variable", "repo-secret"}, + wantPostTypes: []string{"handoff", "require-owner-type"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pre, post := splitBootstrapProfile(tt.profile) + + checkPart := func(part *resolvedBootstrapProfile, want []string, label string) { + if want == nil { + if part != nil { + t.Errorf("%s: expected nil, got profile with %d actions", label, len(part.Profile.Config)) + } + return + } + if part == nil { + t.Fatalf("%s: expected non-nil profile with types %v, got nil", label, want) + } + got := make([]string, len(part.Profile.Config)) + for i, a := range part.Profile.Config { + got[i] = a.Type + } + if len(got) != len(want) { + t.Errorf("%s: got types %v, want %v", label, got, want) + return + } + for i := range want { + if got[i] != want[i] { + t.Errorf("%s: action[%d] type=%q, want %q", label, i, got[i], want[i]) + } + } + if tt.profile != nil && part.PackageID != tt.profile.PackageID { + t.Errorf("%s: PackageID=%q, want %q", label, part.PackageID, tt.profile.PackageID) + } + } + + checkPart(pre, tt.wantPreTypes, "pre-install") + checkPart(post, tt.wantPostTypes, "post-install") + }) + } +} From 109ab5fd99faf1441020645e162e0931bfe33f4d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Jul 2026 07:37:55 +0000 Subject: [PATCH 3/3] Fix test: bootstrap config runs after engine selection, not before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current behavior (all bootstrap steps post-install) is correct. Rewrite the test to verify that the engine selection prompt appears first, and the bootstrap variable prompt has not yet appeared at that point. - Rename TestTuistoryAddWizardManifestBootstrapRunsBeforeEngineSelection → TestTuistoryAddWizardManifestBootstrapRunsAfterEngineSelection - Wait for engine selection prompt first; assert bootstrap prompt has not appeared yet - Revert orchestrator and bootstrap_config.go to single-phase post-install behavior - Revert TestSplitBootstrapProfile and related test changes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 20 +--- .../add_wizard_tuistory_integration_test.go | 29 +---- pkg/cli/bootstrap_config.go | 48 -------- pkg/cli/bootstrap_profile_runner_test.go | 107 +----------------- 4 files changed, 12 insertions(+), 192 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 60adf52d7c2..4617933eec6 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -134,22 +134,10 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error if config.resolvedWorkflows != nil { bootstrapProfile = config.resolvedWorkflows.BootstrapProfile } - // Split bootstrap config into pre-install (repo-variable, repo-secret) and - // post-install phases. Pre-install steps collect configuration values before - // engine selection; post-install steps run after the workflow PR is created. - preInstallBootstrapProfile, remainingBootstrapProfile := splitBootstrapProfile(bootstrapProfile) - - // Step 5b: Run pre-install bootstrap config steps (repo-variable, repo-secret) - // before engine selection so required configuration is collected up front. - if preInstallBootstrapProfile != nil { - if config.hasWriteAccess { - if err := executeBootstrapConfigForAdd(ctx, config.RepoOverride, config.WorkflowSpecs, preInstallBootstrapProfile, config.UseCopilotRequests, config.Verbose); err != nil { - return err - } - } else { - printBootstrapConfigTODO(os.Stderr, preInstallBootstrapProfile) - } - } + // All config steps run post-install in the exact order they are declared in the + // manifest. We no longer split them into a pre-install and post-install phase so + // that the declared ordering is preserved. + remainingBootstrapProfile := bootstrapProfile // Step 6: Select coding agent and collect API key if err := config.selectAIEngineAndKey(); err != nil { diff --git a/pkg/cli/add_wizard_tuistory_integration_test.go b/pkg/cli/add_wizard_tuistory_integration_test.go index d60fa6613da..1265ad6dfc1 100644 --- a/pkg/cli/add_wizard_tuistory_integration_test.go +++ b/pkg/cli/add_wizard_tuistory_integration_test.go @@ -124,7 +124,7 @@ config: description: Leave blank to skip this optional setup value. optional: true - type: handoff - message: Post-install handoff should not appear before engine selection. + message: Post-install handoff message. ` require.NoError(t, os.WriteFile(filepath.Join(packagePath, "aw.yml"), []byte(manifestContent), 0644), "Failed to write local package manifest") require.NoError(t, os.WriteFile(filepath.Join(packagePath, "README.md"), []byte("# Local Add Wizard Package\n"), 0644), "Failed to write local package README") @@ -391,16 +391,15 @@ func TestTuistoryAddWizardIntegration(t *testing.T) { assert.ErrorIs(t, statErr, os.ErrNotExist, "Workflow file should not be created when add-wizard is cancelled") } -func TestTuistoryAddWizardManifestBootstrapRunsBeforeEngineSelection(t *testing.T) { +func TestTuistoryAddWizardManifestBootstrapRunsAfterEngineSelection(t *testing.T) { setup := setupAddWizardManifestTuistoryTest(t) defer func() { _ = os.RemoveAll(setup.tempDir) _ = os.RemoveAll(setup.fakeGHDir) }() - const earlyPrompt = "Enter optional bootstrap variable" + const bootstrapPrompt = "Enter optional bootstrap variable" const enginePrompt = "Which coding agent would you like to use?" - const lateMessage = "Post-install handoff should not appear before engine selection." cmd := exec.Command(setup.binaryPath, "add-wizard", "./local-package", "--no-secret") cmd.Dir = setup.tempDir @@ -418,27 +417,11 @@ func TestTuistoryAddWizardManifestBootstrapRunsBeforeEngineSelection(t *testing. session := startInteractivePTYSession(t, cmd) defer session.close(t) - require.NoError(t, session.waitForText(earlyPrompt, 30*time.Second), "Expected pre-install bootstrap prompt") - - beforeEngineOutput := session.readAll() - assert.Contains(t, beforeEngineOutput, earlyPrompt, "Expected pre-install bootstrap prompt to be shown") - assert.NotContains(t, beforeEngineOutput, enginePrompt, "Engine selection should not start before pre-install bootstrap setup") - assert.NotContains(t, beforeEngineOutput, lateMessage, "Post-install bootstrap steps should not run before engine selection") - - session.writeString(t, "\r") - require.NoError(t, session.waitForText(enginePrompt, 30*time.Second), "Expected engine selection prompt") - afterEngineOutput := session.readAll() - assert.Contains(t, afterEngineOutput, earlyPrompt, "Expected pre-install bootstrap prompt in final session output") - assert.Contains(t, afterEngineOutput, enginePrompt, "Expected engine selection prompt after pre-install bootstrap setup") - assert.NotContains(t, afterEngineOutput, lateMessage, "Post-install bootstrap steps should not run before installation") - - earlyPromptIndex := strings.Index(afterEngineOutput, earlyPrompt) - enginePromptIndex := strings.Index(afterEngineOutput, enginePrompt) - require.NotEqual(t, -1, earlyPromptIndex, "Expected to find pre-install bootstrap prompt in session output") - require.NotEqual(t, -1, enginePromptIndex, "Expected to find engine selection prompt in session output") - assert.Less(t, earlyPromptIndex, enginePromptIndex, "Pre-install bootstrap prompt should appear before engine selection") + beforeInterruptOutput := session.readAll() + assert.Contains(t, beforeInterruptOutput, enginePrompt, "Expected engine selection prompt to be shown") + assert.NotContains(t, beforeInterruptOutput, bootstrapPrompt, "Bootstrap variable prompt should not appear before engine selection") session.interrupt(t) } diff --git a/pkg/cli/bootstrap_config.go b/pkg/cli/bootstrap_config.go index eb62949886b..2ade8076f51 100644 --- a/pkg/cli/bootstrap_config.go +++ b/pkg/cli/bootstrap_config.go @@ -11,54 +11,6 @@ import ( "github.com/github/gh-aw/pkg/gitutil" ) -// isPreInstallBootstrapType reports whether the config action type should run -// before engine selection and workflow installation in the add-wizard flow. -func isPreInstallBootstrapType(actionType string) bool { - switch actionType { - case "repo-variable", "repo-secret": - return true - default: - return false - } -} - -// splitBootstrapProfile splits a resolved bootstrap profile into pre-install and -// post-install parts for the add-wizard flow. Pre-install actions (repo-variable -// and repo-secret) run before engine selection; all remaining actions run after -// the workflow PR has been created. -// -// Either returned value may be nil if there are no actions in that phase. -func splitBootstrapProfile(profile *resolvedBootstrapProfile) (preInstall, postInstall *resolvedBootstrapProfile) { - if profile == nil || profile.Profile == nil || len(profile.Profile.Config) == 0 { - return nil, nil - } - - var preActions, postActions []repositoryPackageBootstrapAction - for _, action := range profile.Profile.Config { - if isPreInstallBootstrapType(action.Type) { - preActions = append(preActions, action) - } else { - postActions = append(postActions, action) - } - } - - if len(preActions) > 0 { - preInstall = &resolvedBootstrapProfile{ - PackageID: profile.PackageID, - Source: profile.Source, - Profile: &repositoryPackageBootstrap{Config: preActions}, - } - } - if len(postActions) > 0 { - postInstall = &resolvedBootstrapProfile{ - PackageID: profile.PackageID, - Source: profile.Source, - Profile: &repositoryPackageBootstrap{Config: postActions}, - } - } - return preInstall, postInstall -} - // printBootstrapConfigTODO prints a TODO checklist of manual steps required by the // config entries in the package manifest. Called by the non-interactive // "add" command after workflows have been installed. diff --git a/pkg/cli/bootstrap_profile_runner_test.go b/pkg/cli/bootstrap_profile_runner_test.go index 73212e6fc95..209cbd3a07b 100644 --- a/pkg/cli/bootstrap_profile_runner_test.go +++ b/pkg/cli/bootstrap_profile_runner_test.go @@ -74,8 +74,8 @@ func TestBootstrapProfileState(t *testing.T) { // TestPrintBootstrapConfigTODO_PreservesManifestOrder verifies that all declared config // actions are included in the TODO output in their exact manifest order. This guards -// against regressions to printBootstrapConfigTODO that would reorder or omit steps; -// note that the add-wizard flow intentionally splits pre-install and post-install actions. +// against regressions that split actions into pre-install and post-install phases and +// thereby reorder or omit steps. func TestPrintBootstrapConfigTODO_PreservesManifestOrder(t *testing.T) { // Declare a profile with an interleaved mix of action types to ensure ordering // cannot be accidentally satisfied by any implicit categorisation. @@ -132,106 +132,3 @@ func TestPrintBootstrapConfigTODO_PreservesManifestOrder(t *testing.T) { } } } - -func TestSplitBootstrapProfile(t *testing.T) { - tests := []struct { - name string - profile *resolvedBootstrapProfile - wantPreTypes []string - wantPostTypes []string - }{ - { - name: "nil profile", - profile: nil, - wantPreTypes: nil, - wantPostTypes: nil, - }, - { - name: "empty config", - profile: &resolvedBootstrapProfile{Profile: &repositoryPackageBootstrap{}}, - wantPreTypes: nil, - wantPostTypes: nil, - }, - { - name: "only pre-install types", - profile: &resolvedBootstrapProfile{ - PackageID: "owner/repo", - Profile: &repositoryPackageBootstrap{ - Config: []repositoryPackageBootstrapAction{ - {Type: "repo-variable", Name: "MY_VAR"}, - {Type: "repo-secret", Name: "MY_SECRET"}, - }, - }, - }, - wantPreTypes: []string{"repo-variable", "repo-secret"}, - wantPostTypes: nil, - }, - { - name: "only post-install types", - profile: &resolvedBootstrapProfile{ - PackageID: "owner/repo", - Profile: &repositoryPackageBootstrap{ - Config: []repositoryPackageBootstrapAction{ - {Type: "handoff", Message: "done"}, - {Type: "copilot-auth", Secret: "TOKEN"}, - }, - }, - }, - wantPreTypes: nil, - wantPostTypes: []string{"handoff", "copilot-auth"}, - }, - { - name: "mixed pre and post install types", - profile: &resolvedBootstrapProfile{ - PackageID: "owner/repo", - Profile: &repositoryPackageBootstrap{ - Config: []repositoryPackageBootstrapAction{ - {Type: "repo-variable", Name: "MY_VAR"}, - {Type: "handoff", Message: "done"}, - {Type: "repo-secret", Name: "MY_SECRET"}, - {Type: "require-owner-type", Value: "org"}, - }, - }, - }, - wantPreTypes: []string{"repo-variable", "repo-secret"}, - wantPostTypes: []string{"handoff", "require-owner-type"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pre, post := splitBootstrapProfile(tt.profile) - - checkPart := func(part *resolvedBootstrapProfile, want []string, label string) { - if want == nil { - if part != nil { - t.Errorf("%s: expected nil, got profile with %d actions", label, len(part.Profile.Config)) - } - return - } - if part == nil { - t.Fatalf("%s: expected non-nil profile with types %v, got nil", label, want) - } - got := make([]string, len(part.Profile.Config)) - for i, a := range part.Profile.Config { - got[i] = a.Type - } - if len(got) != len(want) { - t.Errorf("%s: got types %v, want %v", label, got, want) - return - } - for i := range want { - if got[i] != want[i] { - t.Errorf("%s: action[%d] type=%q, want %q", label, i, got[i], want[i]) - } - } - if tt.profile != nil && part.PackageID != tt.profile.PackageID { - t.Errorf("%s: PackageID=%q, want %q", label, part.PackageID, tt.profile.PackageID) - } - } - - checkPart(pre, tt.wantPreTypes, "pre-install") - checkPart(post, tt.wantPostTypes, "post-install") - }) - } -}