diff --git a/docs/adr/49799-decompose-cli-largefunc-helpers.md b/docs/adr/49799-decompose-cli-largefunc-helpers.md new file mode 100644 index 00000000000..9aacd299842 --- /dev/null +++ b/docs/adr/49799-decompose-cli-largefunc-helpers.md @@ -0,0 +1,44 @@ +# ADR-49799: Decompose Oversized CLI Functions into Focused Helper Functions + +**Date**: 2026-08-02 +**Status**: Accepted +**Deciders**: pelikhan, app/copilot-swe-agent + +--- + +### Context + +The codebase enforces a custom `largefunc` linter rule capping functions at 60 lines (`make golangci-lint`). Several CLI entry-point and flow-control functions in `pkg/cli/` exceeded this limit: `RunAddInteractive` (126 lines), `createWorkflowPRAndConfigureSecret` (171 lines), `computeFirewallDiff` (144 lines), `parseSquidAccessLog` (80 lines), `buildAction` (83 lines), and `selectAIEngineAndKey` (118 lines). These functions mixed multiple sequential phases — host auto-detection, preflight checks, PR merge orchestration, secret configuration, and domain diff classification — into single units, making them difficult to test in isolation and harder to reason about under review. + +### Decision + +We will decompose each oversized CLI function into focused, single-responsibility helper functions co-located in the same file and package. Each extracted helper covers exactly one logical phase (e.g., host detection, PR merge loop, domain diff entry construction) and is tested independently where behavior is non-trivial. The `mergeAction` type and its constants, previously function-local, are promoted to package scope to allow multiple helpers to reference them without re-declaration. + +### Alternatives Considered + +#### Alternative 1: Raise the `largefunc` lint threshold + +Increase the per-function line limit (or add per-file lint suppressions) to accommodate the current oversized functions without refactoring. This avoids code churn but leaves the underlying mixed-responsibility problem in place, degrades long-term readability, and weakens the lint rule for the rest of the codebase. + +#### Alternative 2: Extract logic into separate packages or interface types + +Move the orchestration phases into new dedicated packages or types rather than inline helper functions in the same file. This would provide module-level separation of concerns but would introduce new packages, potential import complexity, and substantial structural churn for what is fundamentally a sequential orchestration flow with no reuse across packages. + +### Consequences + +#### Positive +- All affected functions pass the 60-line `largefunc` lint check, unblocking CI. +- Extracted helpers are individually testable; focused unit tests were added for `prioritizeEngineOption` and `buildMergeOptions`. +- Orchestrator functions (`RunAddInteractive`, `createWorkflowPRAndConfigureSecret`) now read as a sequence of high-level named steps, improving code review clarity. + +#### Negative +- Some extracted helpers carry high parameter counts (e.g., `processSquidAccessLogLine` takes five parameters) because context is passed down rather than captured in a receiver or struct — trading function-length compliance for increased parameter coupling. +- The call graph is deeper; understanding the full flow now requires tracing through more helper function boundaries. + +#### Neutral +- Command behavior and semantics are fully preserved; existing tests continue to pass unmodified. +- The `mergeAction` type and constants are now package-level rather than closure-local; this widens their scope but does not expose them outside the package. + +--- + +*ADR created by [adr-writer agent]. Status: Accepted.* diff --git a/pkg/cli/access_log.go b/pkg/cli/access_log.go index 2e66f38d9d0..7256a0c5bb4 100644 --- a/pkg/cli/access_log.go +++ b/pkg/cli/access_log.go @@ -103,54 +103,7 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error) scanner := bufio.NewScanner(file) for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - - entry, err := parseSquidLogLine(line) - if err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse log line: %v", err))) - } - continue - } - - analysis.TotalRequests++ - - // Extract domain from URL - domain := stringutil.ExtractDomainFromURL(entry.URL) - if domain == "" { - continue - } - - // Determine if request was allowed or blocked based on status code - // Squid typically returns: - // - 200, 206, 304: Allowed/successful - // - 403: Forbidden (blocked by ACL) - // - 407: Proxy authentication required - // - 502, 503: Connection/upstream errors - statusCode := entry.Status - isAllowed := statusCode == "TCP_HIT/200" || statusCode == "TCP_MISS/200" || - statusCode == "TCP_REFRESH_MODIFIED/200" || statusCode == "TCP_IMS_HIT/304" || - strings.Contains(statusCode, "/200") || strings.Contains(statusCode, "/206") || - strings.Contains(statusCode, "/304") - - if isAllowed { - analysis.AllowedRequests++ - if !setutil.Contains(allowedDomainsSet, domain) { - allowedDomainsSet[domain] = struct { - }{} - analysis.AllowedDomains = append(analysis.AllowedDomains, domain) - } - } else { - analysis.BlockedRequests++ - if !setutil.Contains(blockedDomainsSet, domain) { - blockedDomainsSet[domain] = struct { - }{} - analysis.BlockedDomains = append(analysis.BlockedDomains, domain) - } - } + processSquidAccessLogLine(strings.TrimSpace(scanner.Text()), verbose, analysis, allowedDomainsSet, blockedDomainsSet) } if err := scanner.Err(); err != nil { @@ -167,6 +120,50 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error) return analysis, nil } +func processSquidAccessLogLine(line string, verbose bool, analysis *DomainAnalysis, allowedDomainsSet, blockedDomainsSet map[string]struct{}) { + if line == "" || strings.HasPrefix(line, "#") { + return + } + + entry, err := parseSquidLogLine(line) + if err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse log line: %v", err))) + } + return + } + + analysis.TotalRequests++ + domain := stringutil.ExtractDomainFromURL(entry.URL) + if domain == "" { + return + } + + if isAllowedSquidStatus(entry.Status) { + analysis.AllowedRequests++ + addUniqueDomain(allowedDomainsSet, domain, &analysis.AllowedDomains) + return + } + + analysis.BlockedRequests++ + addUniqueDomain(blockedDomainsSet, domain, &analysis.BlockedDomains) +} + +func isAllowedSquidStatus(statusCode string) bool { + return statusCode == "TCP_HIT/200" || statusCode == "TCP_MISS/200" || + statusCode == "TCP_REFRESH_MODIFIED/200" || statusCode == "TCP_IMS_HIT/304" || + strings.Contains(statusCode, "/200") || strings.Contains(statusCode, "/206") || + strings.Contains(statusCode, "/304") +} + +func addUniqueDomain(domainSet map[string]struct{}, domain string, domains *[]string) { + if setutil.Contains(domainSet, domain) { + return + } + domainSet[domain] = struct{}{} + *domains = append(*domains, domain) +} + // parseSquidLogLine parses a single squid access log line // Squid log format: timestamp duration client status size method url user hierarchy type func parseSquidLogLine(line string) (*AccessLogEntry, error) { diff --git a/pkg/cli/actions_build_command.go b/pkg/cli/actions_build_command.go index 4c8f193eec7..5780473da48 100644 --- a/pkg/cli/actions_build_command.go +++ b/pkg/cli/actions_build_command.go @@ -228,6 +228,14 @@ func buildAction(actionsDir, actionName string) error { return nil } + if err := buildJavaScriptAction(actionPath, actionName); err != nil { + return err + } + + return nil +} + +func buildJavaScriptAction(actionPath, actionName string) error { srcPath := filepath.Join(actionPath, "src", "index.js") outputPath := filepath.Join(actionPath, "index.js") diff --git a/pkg/cli/add_interactive_engine.go b/pkg/cli/add_interactive_engine.go index 194f5873555..63f81f5abbd 100644 --- a/pkg/cli/add_interactive_engine.go +++ b/pkg/cli/add_interactive_engine.go @@ -24,56 +24,8 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { return err } - // Determine default engine based on existing secrets, workflow preference, then environment - // Priority order: flag override > existing secrets > workflow frontmatter > environment > default - defaultEngine := string(constants.DefaultEngine) - workflowSpecifiedEngine := "" - - // Check if workflow specifies a preferred engine in frontmatter - if c.resolvedWorkflows != nil && len(c.resolvedWorkflows.Workflows) > 0 { - for _, wf := range c.resolvedWorkflows.Workflows { - if wf.Engine != "" { - workflowSpecifiedEngine = wf.Engine - addInteractiveLog.Printf("Workflow specifies engine in frontmatter: %s", wf.Engine) - break - } - } - } - - // If engine is explicitly overridden via flag, use that - if c.EngineOverride != "" { - defaultEngine = c.EngineOverride - } else { - // Priority 1: Check existing repository secrets using EngineOptions - // This takes precedence over workflow preference since users should use what's already available - for _, opt := range constants.EngineOptions { - if setutil.Contains(c.existingSecrets, opt.SecretName) { - defaultEngine = opt.Value - addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value) - break - } - } - - // Priority 2: If no existing secret found, use workflow frontmatter preference - if defaultEngine == string(constants.DefaultEngine) && workflowSpecifiedEngine != "" { - defaultEngine = workflowSpecifiedEngine - } - - // Priority 3: Check environment variables if no existing secret or workflow preference found - if defaultEngine == string(constants.DefaultEngine) && workflowSpecifiedEngine == "" { - for _, opt := range constants.EngineOptions { - envVar := opt.SecretName - if opt.EnvVarName != "" { - envVar = opt.EnvVarName - } - if lookupEnv(envVar) != "" { - defaultEngine = opt.Value - addInteractiveLog.Printf("Found env var %s, recommending engine: %s", envVar, opt.Value) - break - } - } - } - } + workflowSpecifiedEngine := c.getWorkflowSpecifiedEngine() + defaultEngine := c.determineDefaultEngine(workflowSpecifiedEngine) // If engine is already overridden, skip selection if c.EngineOverride != "" { @@ -88,35 +40,11 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { // Build engine options with notes about existing secrets and workflow specification. // The list of engines is derived from the catalog to ensure all registered engines appear. - catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry()) - engineOptions := sliceutil.Map(catalog.All(), func(def *workflow.EngineDefinition) huh.Option[string] { - opt := constants.GetEngineOption(def.ID) - label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description) - // Add markers for secret availability and workflow specification. - // opt may be nil for catalog engines not yet represented in EngineOptions; - // in that case we conservatively show '[no secret]'. - if opt != nil && setutil.Contains(c.existingSecrets, opt.SecretName) { - label += " [secret exists]" - } else { - label += " [no secret]" - } - if def.ID == workflowSpecifiedEngine { - label += " [specified in workflow]" - } - return huh.NewOption(label, def.ID) - }) + engineOptions := c.buildEngineOptions(workflowSpecifiedEngine) var selectedEngine string - // Set the default selection by moving it to front - for i, opt := range engineOptions { - if opt.Value == defaultEngine { - if i > 0 { - engineOptions[0], engineOptions[i] = engineOptions[i], engineOptions[0] - } - break - } - } + prioritizeEngineOption(engineOptions, defaultEngine) fmt.Fprintln(os.Stderr, "") form := console.NewSelectForm( @@ -137,6 +65,86 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { return c.configureEngineAPISecret(selectedEngine) } +func (c *AddInteractiveConfig) getWorkflowSpecifiedEngine() string { + if c.resolvedWorkflows == nil || len(c.resolvedWorkflows.Workflows) == 0 { + return "" + } + + for _, wf := range c.resolvedWorkflows.Workflows { + if wf.Engine == "" { + continue + } + addInteractiveLog.Printf("Workflow specifies engine in frontmatter: %s", wf.Engine) + return wf.Engine + } + return "" +} + +func (c *AddInteractiveConfig) determineDefaultEngine(workflowSpecifiedEngine string) string { + defaultEngine := string(constants.DefaultEngine) + if c.EngineOverride != "" { + return c.EngineOverride + } + + for _, opt := range constants.EngineOptions { + if setutil.Contains(c.existingSecrets, opt.SecretName) { + addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value) + if opt.Value != string(constants.DefaultEngine) { + return opt.Value + } + // The secret maps to the default engine; fall through so that a + // workflow-specified engine or env-var credential can still override it. + break + } + } + + if workflowSpecifiedEngine != "" { + return workflowSpecifiedEngine + } + + for _, opt := range constants.EngineOptions { + envVar := opt.SecretName + if opt.EnvVarName != "" { + envVar = opt.EnvVarName + } + if lookupEnv(envVar) != "" { + addInteractiveLog.Printf("Found env var %s, recommending engine: %s", envVar, opt.Value) + return opt.Value + } + } + + return defaultEngine +} + +func (c *AddInteractiveConfig) buildEngineOptions(workflowSpecifiedEngine string) []huh.Option[string] { + catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry()) + return sliceutil.Map(catalog.All(), func(def *workflow.EngineDefinition) huh.Option[string] { + opt := constants.GetEngineOption(def.ID) + label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description) + if opt != nil && setutil.Contains(c.existingSecrets, opt.SecretName) { + label += " [secret exists]" + } else { + label += " [no secret]" + } + if def.ID == workflowSpecifiedEngine { + label += " [specified in workflow]" + } + return huh.NewOption(label, def.ID) + }) +} + +func prioritizeEngineOption(engineOptions []huh.Option[string], defaultEngine string) { + for i, opt := range engineOptions { + if opt.Value != defaultEngine { + continue + } + if i > 0 { + engineOptions[0], engineOptions[i] = engineOptions[i], engineOptions[0] + } + return + } +} + // configureEngineAPISecret collects the API key for the selected engine using the unified engine secrets functions func (c *AddInteractiveConfig) configureEngineAPISecret(engine string) error { addInteractiveLog.Printf("Collecting API key for engine: %s", engine) diff --git a/pkg/cli/add_interactive_engine_test.go b/pkg/cli/add_interactive_engine_test.go index 28a1bbfc498..19370d69c3c 100644 --- a/pkg/cli/add_interactive_engine_test.go +++ b/pkg/cli/add_interactive_engine_test.go @@ -5,6 +5,8 @@ package cli import ( "testing" + "charm.land/huh/v2" + "github.com/github/gh-aw/pkg/constants" "github.com/stretchr/testify/assert" ) @@ -50,3 +52,85 @@ func TestApplyCopilotAuthMethodChoice_ReEntryClearsOldValue(t *testing.T) { cfg.applyCopilotAuthMethodChoice("pat") assert.False(t, cfg.UseCopilotRequests) } + +func TestPrioritizeEngineOption(t *testing.T) { + options := []huh.Option[string]{ + huh.NewOption("B", "b"), + huh.NewOption("A", "a"), + } + + prioritizeEngineOption(options, "a") + assert.Equal(t, "a", options[0].Value) + assert.Equal(t, "b", options[1].Value) + + prioritizeEngineOption(options, "missing") + assert.Equal(t, "a", options[0].Value) + assert.Equal(t, "b", options[1].Value) +} + +func TestDetermineDefaultEngine(t *testing.T) { + makeSecrets := func(keys ...string) map[string]struct{} { + m := make(map[string]struct{}, len(keys)) + for _, k := range keys { + m[k] = struct{}{} + } + return m + } + + tests := []struct { + name string + engineOverride string + existingSecrets map[string]struct{} + workflowSpecifiedEngine string + want string + }{ + { + name: "engine override takes priority over everything", + engineOverride: "codex", + existingSecrets: makeSecrets(constants.AnthropicAPIKey), + workflowSpecifiedEngine: "claude", + want: "codex", + }, + { + name: "non-default secret overrides workflow preference", + existingSecrets: makeSecrets(constants.AnthropicAPIKey), + workflowSpecifiedEngine: "codex", + want: string(constants.ClaudeEngine), + }, + { + name: "default-engine (Copilot) secret falls through to workflow preference", + existingSecrets: makeSecrets(constants.CopilotGitHubToken), + workflowSpecifiedEngine: string(constants.ClaudeEngine), + want: string(constants.ClaudeEngine), + }, + { + name: "default-engine secret with no workflow preference stays default", + existingSecrets: makeSecrets(constants.CopilotGitHubToken), + workflowSpecifiedEngine: "", + want: string(constants.DefaultEngine), + }, + { + name: "no secret defers to workflow preference", + existingSecrets: nil, + workflowSpecifiedEngine: string(constants.ClaudeEngine), + want: string(constants.ClaudeEngine), + }, + { + name: "no secret no workflow returns default engine", + existingSecrets: nil, + workflowSpecifiedEngine: "", + want: string(constants.DefaultEngine), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &AddInteractiveConfig{ + EngineOverride: tc.engineOverride, + existingSecrets: tc.existingSecrets, + } + got := cfg.determineDefaultEngine(tc.workflowSpecifiedEngine) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/pkg/cli/add_interactive_git.go b/pkg/cli/add_interactive_git.go index bb4ae9a995e..9a0d99aa00a 100644 --- a/pkg/cli/add_interactive_git.go +++ b/pkg/cli/add_interactive_git.go @@ -22,6 +22,16 @@ func isAlreadyMergedGHError(err error) bool { return strings.Contains(err.Error(), "already merged") || strings.Contains(err.Error(), "MERGED") } +type mergeAction string + +const ( + mergeActionAttempt mergeAction = "attempt" + mergeActionEditTitle mergeAction = "editTitle" + mergeActionReview mergeAction = "review" + mergeActionConfirmed mergeAction = "confirmed" + mergeActionExit mergeAction = "exit" +) + // createWorkflowPRAndConfigureSecret creates the PR, merges it, and adds the secret func (c *AddInteractiveConfig) createWorkflowPRAndConfigureSecret(ctx context.Context, workflowFiles, initFiles []string, secretName, secretValue string) error { addInteractiveLog.Print("Applying changes") @@ -53,123 +63,144 @@ func (c *AddInteractiveConfig) createWorkflowPRAndConfigureSecret(ctx context.Co } c.addResult = result - // Step 8b: Optionally merge the PR – loop until merged, confirmed-merged, or user exits - if result.PRNumber == 0 { - if result.PRURL == "" { + if err := c.ensurePullRequestMerged(result.PRNumber, result.PRURL); err != nil { + return err + } + + // Step 8c: Add the secret (skip if no secret configured or already exists in repository). + return c.configureRepositorySecret(secretName, secretValue) +} + +func (c *AddInteractiveConfig) ensurePullRequestMerged(prNumber int, prURL string) error { + if prNumber == 0 { + if prURL == "" { fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Requested workflow files already exist locally; no pull request was created.")) return nil } fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Could not determine PR number")) fmt.Fprintln(os.Stderr, "Please merge the PR manually from the GitHub web interface.") - } else { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Pull request created: "+result.PRURL)) - fmt.Fprintln(os.Stderr, "") - - // mergeAction values used in the select loop - type mergeAction string - const ( - mergeActionAttempt mergeAction = "attempt" - mergeActionEditTitle mergeAction = "editTitle" - mergeActionReview mergeAction = "review" - mergeActionConfirmed mergeAction = "confirmed" - mergeActionExit mergeAction = "exit" - ) + return nil + } - mergeDone := false // true when the PR is merged (or confirmed merged) - mergeFailed := false // true after an unsuccessful merge attempt - userReviewing := false // true after the user chose "I'll review myself" + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Pull request created: "+prURL)) + fmt.Fprintln(os.Stderr, "") + return c.runPRMergeLoop(prNumber, prURL) +} - for !mergeDone { - // Build option list based on current state - var options []huh.Option[mergeAction] +func (c *AddInteractiveConfig) runPRMergeLoop(prNumber int, prURL string) error { + mergeDone := false + mergeFailed := false + userReviewing := false - options = append(options, huh.NewOption("Attempt to merge", mergeActionAttempt)) + for !mergeDone { + chosen, err := promptMergeAction(prURL, mergeFailed, userReviewing) + if err != nil { + return err + } - if mergeFailed { - options = append(options, huh.NewOption("Edit PR title and retry", mergeActionEditTitle)) + switch chosen { + case mergeActionAttempt: + done, failed := c.handleMergeAttempt(prNumber, prURL, mergeFailed) + mergeDone = done + mergeFailed = failed + case mergeActionEditTitle: + updated, err := c.promptAndEditPRTitle(prNumber) + if err != nil { + return err } - - if userReviewing { - options = append(options, huh.NewOption("PR has been manually merged", mergeActionConfirmed)) - } else { - options = append(options, huh.NewOption("I'll review/merge myself", mergeActionReview)) + if updated { + mergeFailed = false } + case mergeActionReview: + userReviewing = true + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Please review and merge the pull request: "+prURL)) + fmt.Fprintln(os.Stderr, "") + case mergeActionConfirmed: + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Great – continuing with the merged pull request")) + mergeDone = true + case mergeActionExit: + fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Exiting. You can merge the pull request later: "+prURL)) + return errors.New("user exited before PR was merged") + } + } - if userReviewing { - options = append(options, huh.NewOption("Exit, I'm done here", mergeActionExit)) - } else { - options = append(options, huh.NewOption("Exit", mergeActionExit)) - } + return nil +} - var chosen mergeAction - selectForm := console.NewSelectForm( - huh.NewSelect[mergeAction](). - Title("What would you like to do with pull request " + result.PRURL + "?"). - Options(options...). - Value(&chosen), - ) +func promptMergeAction(prURL string, mergeFailed, userReviewing bool) (mergeAction, error) { + var chosen mergeAction + selectForm := console.NewSelectForm( + huh.NewSelect[mergeAction](). + Title("What would you like to do with pull request " + prURL + "?"). + Options(buildMergeOptions(mergeFailed, userReviewing)...). + Value(&chosen), + ) + if err := selectForm.Run(); err != nil { + return "", fmt.Errorf("failed to get user input: %w", err) + } + return chosen, nil +} - if selectErr := selectForm.Run(); selectErr != nil { - return fmt.Errorf("failed to get user input: %w", selectErr) - } +func buildMergeOptions(mergeFailed, userReviewing bool) []huh.Option[mergeAction] { + options := []huh.Option[mergeAction]{ + huh.NewOption("Attempt to merge", mergeActionAttempt), + } + if mergeFailed { + options = append(options, huh.NewOption("Edit PR title and retry", mergeActionEditTitle)) + } + if userReviewing { + options = append(options, huh.NewOption("PR has been manually merged", mergeActionConfirmed)) + options = append(options, huh.NewOption("Exit, I'm done here", mergeActionExit)) + return options + } + options = append(options, huh.NewOption("I'll review/merge myself", mergeActionReview)) + options = append(options, huh.NewOption("Exit", mergeActionExit)) + return options +} - switch chosen { - case mergeActionAttempt: - if mergeErr := c.mergePullRequest(result.PRNumber); mergeErr != nil { - if isAlreadyMergedGHError(mergeErr) { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Merged pull request "+result.PRURL)) - mergeDone = true - } else { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to merge PR: %v", mergeErr))) - if mergeFailed { - fmt.Fprintln(os.Stderr, "Please merge the PR manually: "+result.PRURL) - } - mergeFailed = true - } - } else { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Merged pull request "+result.PRURL)) - mergeDone = true - } - - case mergeActionEditTitle: - var newTitle string - titleForm := console.NewInputForm( - huh.NewInput(). - Title("Enter new PR title"). - Description("Add a prefix if required, for example: feat: or fix:"). - Value(&newTitle), - ) - if titleErr := titleForm.Run(); titleErr != nil { - return fmt.Errorf("failed to get user input: %w", titleErr) - } - newTitle = strings.TrimSpace(newTitle) - if newTitle == "" { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("PR title cannot be empty, keeping current title")) - } else if editErr := editPRTitle(result.PRNumber, newTitle, c.RepoOverride); editErr != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update PR title: %v", editErr))) - } else { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("PR title updated to: "+newTitle)) - mergeFailed = false - } - - case mergeActionReview: - userReviewing = true - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Please review and merge the pull request: "+result.PRURL)) - fmt.Fprintln(os.Stderr, "") - - case mergeActionConfirmed: - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Great – continuing with the merged pull request")) - mergeDone = true - - case mergeActionExit: - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Exiting. You can merge the pull request later: "+result.PRURL)) - return errors.New("user exited before PR was merged") - } +func (c *AddInteractiveConfig) handleMergeAttempt(prNumber int, prURL string, mergeFailed bool) (mergeDone bool, nowFailed bool) { + if mergeErr := c.mergePullRequest(prNumber); mergeErr != nil { + if isAlreadyMergedGHError(mergeErr) { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Merged pull request "+prURL)) + return true, mergeFailed + } + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to merge PR: %v", mergeErr))) + if mergeFailed { + fmt.Fprintln(os.Stderr, "Please merge the PR manually: "+prURL) } + return false, true } - // Step 8c: Add the secret (skip if no secret configured or already exists in repository) + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Merged pull request "+prURL)) + return true, mergeFailed +} + +func (c *AddInteractiveConfig) promptAndEditPRTitle(prNumber int) (bool, error) { + var newTitle string + titleForm := console.NewInputForm( + huh.NewInput(). + Title("Enter new PR title"). + Description("Add a prefix if required, for example: feat: or fix:"). + Value(&newTitle), + ) + if err := titleForm.Run(); err != nil { + return false, fmt.Errorf("failed to get user input: %w", err) + } + newTitle = strings.TrimSpace(newTitle) + if newTitle == "" { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("PR title cannot be empty, keeping current title")) + return false, nil + } + if err := editPRTitle(prNumber, newTitle, c.RepoOverride); err != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update PR title: %v", err))) + return false, nil + } + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("PR title updated to: "+newTitle)) + return true, nil +} + +func (c *AddInteractiveConfig) configureRepositorySecret(secretName, secretValue string) error { if secretName == "" { // No secret to configure (e.g., user doesn't have write access to the repository) } else if secretValue == "" { diff --git a/pkg/cli/add_interactive_git_test.go b/pkg/cli/add_interactive_git_test.go index cc8ce2f5a4a..427dbbeffb0 100644 --- a/pkg/cli/add_interactive_git_test.go +++ b/pkg/cli/add_interactive_git_test.go @@ -180,3 +180,42 @@ func runGitIn(t *testing.T, dir string, args ...string) { out, err := cmd.CombinedOutput() require.NoError(t, err, "git %s failed: %s", strings.Join(args, " "), string(out)) } + +func TestBuildMergeOptions(t *testing.T) { + tests := []struct { + name string + mergeFailed bool + userReviewing bool + wantValues []mergeAction + }{ + { + name: "default options", + mergeFailed: false, + userReviewing: false, + wantValues: []mergeAction{mergeActionAttempt, mergeActionReview, mergeActionExit}, + }, + { + name: "merge failed adds edit title", + mergeFailed: true, + userReviewing: false, + wantValues: []mergeAction{mergeActionAttempt, mergeActionEditTitle, mergeActionReview, mergeActionExit}, + }, + { + name: "user reviewing shows confirmation path", + mergeFailed: false, + userReviewing: true, + wantValues: []mergeAction{mergeActionAttempt, mergeActionConfirmed, mergeActionExit}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + options := buildMergeOptions(tt.mergeFailed, tt.userReviewing) + values := make([]mergeAction, 0, len(options)) + for _, opt := range options { + values = append(values, opt.Value) + } + assert.Equal(t, tt.wantValues, values) + }) + } +} diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 4617933eec6..26560822aa0 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -82,119 +82,123 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error // Set context on the config config.Ctx = ctx - // Auto-detect GHES host from git remote if not already set - if os.Getenv("GH_HOST") == "" { //nolint:osgetenvlibrary - detectedHost := getHostFromOriginRemote() - if detectedHost != "github.com" { - addInteractiveLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost) - workflow.SetDefaultGHHost(detectedHost) - if config.Verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Auto-detected GitHub Enterprise host: "+detectedHost)) - } - } - } + config.configureDefaultGHHostFromRemote() - // Step 1: Welcome message - console.ShowWelcomeBanner("This tool will walk you through adding an automated workflow to your repository.") - - // Step 1b: Resolve workflows early to get descriptions and validate specs - if err := config.resolveWorkflows(); err != nil { + if err := config.runInitialAddInteractiveChecks(); err != nil { return err } - // Step 1c: Show workflow descriptions if available - config.showWorkflowDescriptions() + remainingBootstrapProfile := config.getRemainingBootstrapProfile() - // Step 2: Check gh auth status - if err := config.checkGHAuthStatus(); err != nil { + filesToAdd, initFiles, secretName, secretValue, err := config.prepareAndConfirmAddInteractive() + if err != nil { return err } - // Step 3: Check git repository and get org/repo - if err := config.checkGitRepository(); err != nil { + if err := config.createWorkflowPRAndConfigureSecret(ctx, filesToAdd, initFiles, secretName, secretValue); err != nil { return err } - // Step 3b: Check working directory is clean (must be clean for PR creation later) - if err := config.checkCleanWorkingDirectory(); err != nil { + if err := config.applyBootstrapConfigIfNeeded(ctx, remainingBootstrapProfile); err != nil { return err } - // Step 4: Check GitHub Actions is enabled - if err := config.checkActionsEnabled(); err != nil { + // Step 10: Check status and offer to run + if err := config.checkStatusAndOfferRun(ctx); err != nil { return err } - // Step 5: Check user permissions - if err := config.checkUserPermissions(); err != nil { - return err + return nil +} + +func (c *AddInteractiveConfig) configureDefaultGHHostFromRemote() { + if os.Getenv("GH_HOST") != "" { //nolint:osgetenvlibrary + return + } + detectedHost := getHostFromOriginRemote() + if detectedHost == "github.com" { + return + } + addInteractiveLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost) + workflow.SetDefaultGHHost(detectedHost) + if c.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Auto-detected GitHub Enterprise host: "+detectedHost)) } +} - var bootstrapProfile *resolvedBootstrapProfile - if config.resolvedWorkflows != nil { - bootstrapProfile = config.resolvedWorkflows.BootstrapProfile +func (c *AddInteractiveConfig) getRemainingBootstrapProfile() *resolvedBootstrapProfile { + if c.resolvedWorkflows == nil { + return nil } // 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 + return c.resolvedWorkflows.BootstrapProfile +} - // Step 6: Select coding agent and collect API key - if err := config.selectAIEngineAndKey(); err != nil { - return err +func (c *AddInteractiveConfig) applyBootstrapConfigIfNeeded(ctx context.Context, profile *resolvedBootstrapProfile) error { + if profile == nil { + return nil + } + if c.hasWriteAccess { + return executeBootstrapConfigForAdd(ctx, c.RepoOverride, c.WorkflowSpecs, profile, c.UseCopilotRequests, c.Verbose) } + printBootstrapConfigTODO(os.Stderr, profile) + return nil +} - initFiles, err := ensureAddRepositoryInitializedWithDetails(config.EngineOverride, config.Verbose, config.NoGitattributes) - if err != nil { +func (c *AddInteractiveConfig) runInitialAddInteractiveChecks() error { + console.ShowWelcomeBanner("This tool will walk you through adding an automated workflow to your repository.") + if err := c.resolveWorkflows(); err != nil { return err } - - // Step 7: Determine files to add - filesToAdd, _, err := config.determineFilesToAdd() - if err != nil { + c.showWorkflowDescriptions() + if err := c.checkGHAuthStatus(); err != nil { return err } - - // Step 7b: Offer schedule frequency selection for scheduled workflows - if err := config.selectScheduleFrequency(); err != nil { + if err := c.checkGitRepository(); err != nil { + return err + } + if err := c.checkCleanWorkingDirectory(); err != nil { return err } + if err := c.checkActionsEnabled(); err != nil { + return err + } + return c.checkUserPermissions() +} - // Step 8: Confirm with user - var secretName, secretValue string - if config.hasWriteAccess && !config.SkipSecret && !config.UseCopilotRequests { - secretName, secretValue, err = config.resolveEngineApiKeyCredential() - if err != nil { - return err - } +func (c *AddInteractiveConfig) prepareAndConfirmAddInteractive() (workflowFiles, initFiles []string, secretName, secretValue string, err error) { + if err := c.selectAIEngineAndKey(); err != nil { + return nil, nil, "", "", err } - if err := config.confirmChanges(filesToAdd, initFiles, secretName, secretValue); err != nil { - return err + initFiles, err = ensureAddRepositoryInitializedWithDetails(c.EngineOverride, c.Verbose, c.NoGitattributes) + if err != nil { + return nil, nil, "", "", err } - // Step 9: Apply changes (create PR, merge, add secret) - if err := config.createWorkflowPRAndConfigureSecret(ctx, filesToAdd, initFiles, secretName, secretValue); err != nil { - return err + workflowFiles, _, err = c.determineFilesToAdd() + if err != nil { + return nil, nil, "", "", err } - // Step 9b: Apply bootstrap config steps interactively (if the package declares any) - if remainingBootstrapProfile != nil { - if config.hasWriteAccess { - if err := executeBootstrapConfigForAdd(ctx, config.RepoOverride, config.WorkflowSpecs, remainingBootstrapProfile, config.UseCopilotRequests, config.Verbose); err != nil { - return err - } - } else { - printBootstrapConfigTODO(os.Stderr, remainingBootstrapProfile) + if err := c.selectScheduleFrequency(); err != nil { + return nil, nil, "", "", err + } + + if c.hasWriteAccess && !c.SkipSecret && !c.UseCopilotRequests { + secretName, secretValue, err = c.resolveEngineApiKeyCredential() + if err != nil { + return nil, nil, "", "", err } } - // Step 10: Check status and offer to run - if err := config.checkStatusAndOfferRun(ctx); err != nil { - return err + if err := c.confirmChanges(workflowFiles, initFiles, secretName, secretValue); err != nil { + return nil, nil, "", "", err } - return nil + return workflowFiles, initFiles, secretName, secretValue, nil } // resolveWorkflows resolves workflow specifications by installing repositories, diff --git a/pkg/cli/audit_diff.go b/pkg/cli/audit_diff.go index f002dee2cda..6a4df055795 100644 --- a/pkg/cli/audit_diff.go +++ b/pkg/cli/audit_diff.go @@ -71,130 +71,22 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi Run2ID: run2ID, } - // Handle nil cases - run1Stats := make(map[string]DomainRequestStats) - run2Stats := make(map[string]DomainRequestStats) - - if run1 != nil { - run1Stats = run1.RequestsByDomain - } - if run2 != nil { - run2Stats = run2.RequestsByDomain - } + run1Stats, run2Stats := firewallDomainStats(run1, run2) // If both are nil/empty, return empty diff if len(run1Stats) == 0 && len(run2Stats) == 0 { return diff } - // Collect all domains - allDomains := make(map[string]struct{}) - for domain := range run1Stats { - allDomains[domain] = struct{}{} - } - for domain := range run2Stats { - allDomains[domain] = struct{}{} - } - // Sorted domain list for deterministic output - sortedDomains := sliceutil.SortedKeys(allDomains) + sortedDomains := sliceutil.SortedKeys(collectAllDomains(run1Stats, run2Stats)) anomalyCount := 0 for _, domain := range sortedDomains { stats1, inRun1 := run1Stats[domain] stats2, inRun2 := run2Stats[domain] - - if !inRun1 && inRun2 { - // New domain in run 2 - entry := DomainDiffEntry{ - Domain: domain, - DiffEntryBase: DiffEntryBase{Status: "new"}, - Run2Allowed: stats2.Allowed, - Run2Blocked: stats2.Blocked, - Run2Status: classifyFirewallDomainStatus(stats2), - } - // Anomaly: new denied domain - if stats2.Blocked > 0 { - entry.IsAnomaly = true - entry.AnomalyNote = "new denied domain" - anomalyCount++ - } - diff.NewDomains = append(diff.NewDomains, entry) - } else if inRun1 && !inRun2 { - // Removed domain - entry := DomainDiffEntry{ - Domain: domain, - DiffEntryBase: DiffEntryBase{Status: "removed"}, - Run1Allowed: stats1.Allowed, - Run1Blocked: stats1.Blocked, - Run1Status: classifyFirewallDomainStatus(stats1), - } - // Anomaly: the removed domain was denied in the base run. This indicates a - // transient firewall block that prevented the agent from reaching an MCP server - // (e.g. awmg-mcpg:8080) — even though the domain is absent from the comparison - // run (and therefore looks "normal"), its prior denial is worth surfacing so - // post-completion relaunch failures are detectable in audit diffs. - if stats1.Blocked > 0 { - entry.IsAnomaly = true - entry.AnomalyNote = "denied in base run — absent from comparison run" - anomalyCount++ - } - diff.RemovedDomains = append(diff.RemovedDomains, entry) - } else { - // Domain exists in both runs - check for changes - status1 := classifyFirewallDomainStatus(stats1) - status2 := classifyFirewallDomainStatus(stats2) - - if status1 != status2 { - // Status changed - entry := DomainDiffEntry{ - Domain: domain, - DiffEntryBase: DiffEntryBase{Status: "status_changed"}, - Run1Allowed: stats1.Allowed, - Run1Blocked: stats1.Blocked, - Run2Allowed: stats2.Allowed, - Run2Blocked: stats2.Blocked, - Run1Status: status1, - Run2Status: status2, - } - // Anomaly: previously denied, now allowed - if status1 == "denied" && status2 == "allowed" { - entry.IsAnomaly = true - entry.AnomalyNote = "previously denied, now allowed" - anomalyCount++ - } - // Anomaly: previously allowed, now denied - if status1 == "allowed" && status2 == "denied" { - entry.IsAnomaly = true - entry.AnomalyNote = "previously allowed, now denied" - anomalyCount++ - } - diff.StatusChanges = append(diff.StatusChanges, entry) - } else { - // Check for significant volume changes (>100% threshold) - total1 := stats1.Allowed + stats1.Blocked - total2 := stats2.Allowed + stats2.Blocked - - if total1 > 0 { - pctChange := (float64(total2-total1) / float64(total1)) * 100 - if math.Abs(pctChange) > volumeChangeThresholdPercent { - entry := DomainDiffEntry{ - Domain: domain, - DiffEntryBase: DiffEntryBase{Status: "volume_changed"}, - Run1Allowed: stats1.Allowed, - Run1Blocked: stats1.Blocked, - Run2Allowed: stats2.Allowed, - Run2Blocked: stats2.Blocked, - Run1Status: status1, - Run2Status: status2, - VolumeChange: formatVolumeChange(total1, total2), - } - diff.VolumeChanges = append(diff.VolumeChanges, entry) - } - } - } - } + anomalyCount += appendFirewallDomainDiff(diff, domain, stats1, stats2, inRun1, inRun2) } diff.Summary = FirewallDiffSummary{ @@ -211,6 +103,131 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi return diff } +func firewallDomainStats(run1, run2 *FirewallAnalysis) (map[string]DomainRequestStats, map[string]DomainRequestStats) { + run1Stats := make(map[string]DomainRequestStats) + run2Stats := make(map[string]DomainRequestStats) + if run1 != nil { + run1Stats = run1.RequestsByDomain + } + if run2 != nil { + run2Stats = run2.RequestsByDomain + } + return run1Stats, run2Stats +} + +func collectAllDomains(run1Stats, run2Stats map[string]DomainRequestStats) map[string]struct{} { + allDomains := make(map[string]struct{}) + for domain := range run1Stats { + allDomains[domain] = struct{}{} + } + for domain := range run2Stats { + allDomains[domain] = struct{}{} + } + return allDomains +} + +func appendFirewallDomainDiff(diff *FirewallDiff, domain string, stats1, stats2 DomainRequestStats, inRun1, inRun2 bool) int { + if !inRun1 && inRun2 { + entry, anomalyCount := buildNewFirewallDomainEntry(domain, stats2) + diff.NewDomains = append(diff.NewDomains, entry) + return anomalyCount + } + if inRun1 && !inRun2 { + entry, anomalyCount := buildRemovedFirewallDomainEntry(domain, stats1) + diff.RemovedDomains = append(diff.RemovedDomains, entry) + return anomalyCount + } + return appendExistingFirewallDomainDiff(diff, domain, stats1, stats2) +} + +func buildNewFirewallDomainEntry(domain string, stats2 DomainRequestStats) (DomainDiffEntry, int) { + entry := DomainDiffEntry{ + Domain: domain, + DiffEntryBase: DiffEntryBase{Status: "new"}, + Run2Allowed: stats2.Allowed, + Run2Blocked: stats2.Blocked, + Run2Status: classifyFirewallDomainStatus(stats2), + } + if stats2.Blocked > 0 { + entry.IsAnomaly = true + entry.AnomalyNote = "new denied domain" + return entry, 1 + } + return entry, 0 +} + +func buildRemovedFirewallDomainEntry(domain string, stats1 DomainRequestStats) (DomainDiffEntry, int) { + entry := DomainDiffEntry{ + Domain: domain, + DiffEntryBase: DiffEntryBase{Status: "removed"}, + Run1Allowed: stats1.Allowed, + Run1Blocked: stats1.Blocked, + Run1Status: classifyFirewallDomainStatus(stats1), + } + if stats1.Blocked > 0 { + entry.IsAnomaly = true + entry.AnomalyNote = "denied in base run — absent from comparison run" + return entry, 1 + } + return entry, 0 +} + +// appendExistingFirewallDomainDiff appends a diff entry for a domain present in both runs. +// Returns 1 if an anomaly was detected (a security-relevant status flip), 0 otherwise. +// Volume changes are recorded in diff.VolumeChanges but are not counted as anomalies. +func appendExistingFirewallDomainDiff(diff *FirewallDiff, domain string, stats1, stats2 DomainRequestStats) int { + status1 := classifyFirewallDomainStatus(stats1) + status2 := classifyFirewallDomainStatus(stats2) + if status1 != status2 { + entry := DomainDiffEntry{ + Domain: domain, + DiffEntryBase: DiffEntryBase{Status: "status_changed"}, + Run1Allowed: stats1.Allowed, + Run1Blocked: stats1.Blocked, + Run2Allowed: stats2.Allowed, + Run2Blocked: stats2.Blocked, + Run1Status: status1, + Run2Status: status2, + } + if status1 == "denied" && status2 == "allowed" { + entry.IsAnomaly = true + entry.AnomalyNote = "previously denied, now allowed" + diff.StatusChanges = append(diff.StatusChanges, entry) + return 1 // anomaly: a previously-blocked domain is now allowed + } + if status1 == "allowed" && status2 == "denied" { + entry.IsAnomaly = true + entry.AnomalyNote = "previously allowed, now denied" + diff.StatusChanges = append(diff.StatusChanges, entry) + return 1 // anomaly: a previously-allowed domain is now blocked + } + diff.StatusChanges = append(diff.StatusChanges, entry) + return 0 // status changed (e.g. mixed ↔ allowed) but not a security-relevant flip + } + + total1 := stats1.Allowed + stats1.Blocked + total2 := stats2.Allowed + stats2.Blocked + if total1 == 0 { + return 0 // no baseline traffic; nothing to compare + } + pctChange := (float64(total2-total1) / float64(total1)) * 100 + if math.Abs(pctChange) <= volumeChangeThresholdPercent { + return 0 // volume within threshold; not noteworthy + } + diff.VolumeChanges = append(diff.VolumeChanges, DomainDiffEntry{ + Domain: domain, + DiffEntryBase: DiffEntryBase{Status: "volume_changed"}, + Run1Allowed: stats1.Allowed, + Run1Blocked: stats1.Blocked, + Run2Allowed: stats2.Allowed, + Run2Blocked: stats2.Blocked, + Run1Status: status1, + Run2Status: status2, + VolumeChange: formatVolumeChange(total1, total2), + }) + return 0 // volume change recorded but not classified as an anomaly +} + // classifyFirewallDomainStatus returns "allowed", "denied", or "mixed" based on request stats func classifyFirewallDomainStatus(stats DomainRequestStats) string { if stats.Allowed > 0 && stats.Blocked == 0 {