diff --git a/docs/adr/36144-decompose-deploy-command-orchestration.md b/docs/adr/36144-decompose-deploy-command-orchestration.md new file mode 100644 index 00000000000..62028dd39da --- /dev/null +++ b/docs/adr/36144-decompose-deploy-command-orchestration.md @@ -0,0 +1,39 @@ +# ADR-36144: Decompose Deploy Command Orchestration into Focused Helpers + +**Date**: 2026-05-31 +**Status**: Draft + +## Context + +The repository enforces a custom `largefunc` linter that caps functions at 60 lines, and `pkg/cli` has widespread violations of this rule. The deploy command slice concentrated two oversized functions: `NewDeployCommand`, whose `Args` and `RunE` cobra hooks were large inline closures performing flag parsing, cardinality validation, engine validation, and option assembly; and `runDeploy`, which executed the entire deploy lifecycle (checkout prep, update, add, compile, PR creation) in one long body. Both exceeded the 60-line limit while mixing command-wiring concerns with execution orchestration, making the parsing paths impossible to unit-test without invoking the full command. + +## Decision + +We will decompose the deploy command along two axes while preserving all CLI behavior, flags, usage text, and option semantics. For command construction, we will extract the inline cobra closures into named helpers — `validateDeployArgs`, `registerDeployFlags`, `runDeployCommand`, and `parseDeployCommandOptions` — so flag wiring and option parsing become independently testable. For execution flow, we will split `runDeploy` into explicit lifecycle stages — `prepareDeployCheckout`, `runDeployUpdatePass`, `runDeployAddPass`, `runDeployCompilePass`, and `createDeployPR` — preserving the existing `update -> add -> compile --purge -> create PR` sequencing and error paths. We will also wrap engine validation failures with contextual error text (`failed to validate engine: %w`). The primary driver is conformance with the `largefunc` linter and isolation of the parsing logic for focused test coverage. + +## Alternatives Considered + +### Alternative 1: Exempt the deploy command from the `largefunc` limit +We could add `nolint` directives or raise the threshold for `deploy_command.go`, since cobra command wiring naturally accretes flag-handling boilerplate. Rejected because it weakens a repo-wide quality gate and the same precedent (ADR-36064, ADR-36012) established that this code can be expressed just as clearly with named helpers that become independently testable. + +### Alternative 2: Shorten the closures in place without extracting named functions +We could trim the `Args`/`RunE` closures and `runDeploy` body below 60 lines while keeping them anonymous and monolithic. Rejected because anonymous closures cannot be unit-tested in isolation, and the line count is dominated by genuine parsing and orchestration logic that cannot be meaningfully compressed without extraction — leaving the validation contract untested. + +## Consequences + +### Positive +- `parseDeployCommandOptions` is now directly unit-testable, and the PR adds focused coverage for `--name` cardinality, invalid `--cool-down`, the zero-value return contract on parse errors, and engine validation propagation. +- The deploy lifecycle reads as a short sequence of named stages, making the `update -> add -> compile -> create PR` ordering explicit at a glance. +- Engine validation errors now carry contextual prefix text, improving diagnostics. + +### Negative +- More top-level functions in `pkg/cli` increase the package symbol surface and add a layer of indirection when tracing a single deploy invocation end to end. +- The stage-helper decomposition must be kept in sync by future authors; adding a new lifecycle step means threading it into both `runDeploy` and the helper set rather than editing one function. + +### Neutral +- No change to CLI behavior, flags, usage text, or option semantics; this is a structural refactor within the existing cobra command framework. +- `runDeploy` retains the same `defer os.Chdir(originalDir)` restoration and error sequencing as before the split. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26720290531) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/cli/deploy_command.go b/pkg/cli/deploy_command.go index f548cad921a..aae31a88684 100644 --- a/pkg/cli/deploy_command.go +++ b/pkg/cli/deploy_command.go @@ -34,61 +34,60 @@ Examples: ` + string(constants.CLIExtensionPrefix) + ` deploy githubnext/agentics/ci-doctor --repo owner/repo ` + string(constants.CLIExtensionPrefix) + ` deploy githubnext/agentics/repo-assist githubnext/agentics/ci-doctor --repo owner/repo --force ` + string(constants.CLIExtensionPrefix) + ` deploy ./my-workflow.md --repo owner/repo`, - Args: func(cmd *cobra.Command, args []string) error { - if len(args) < 1 { - return fmt.Errorf("missing workflow specification\n\nUsage:\n %s ...\n\nExamples:\n %[1]s githubnext/agentics/ci-doctor --repo owner/repo\n\nRun '%[1]s --help' for more information", cmd.CommandPath()) - } - return nil - }, + Args: validateDeployArgs, RunE: func(cmd *cobra.Command, args []string) error { - workflows := args - targetRepo, _ := cmd.Flags().GetString("repo") - if strings.TrimSpace(targetRepo) == "" { - return errors.New("--repo flag is required (target repository in owner/repo format)") - } - - engineOverride, _ := cmd.Flags().GetString("engine") - nameFlag, _ := cmd.Flags().GetString("name") - forceFlag, _ := cmd.Flags().GetBool("force") - appendText, _ := cmd.Flags().GetString("append") - verbose, _ := cmd.Flags().GetBool("verbose") - noGitattributes, _ := cmd.Flags().GetBool("no-gitattributes") - workflowDir, _ := cmd.Flags().GetString("dir") - noStopAfter, _ := cmd.Flags().GetBool("no-stop-after") - stopAfter, _ := cmd.Flags().GetString("stop-after") - disableSecurityScanner, _ := cmd.Flags().GetBool("disable-security-scanner") - coolDownStr, _ := cmd.Flags().GetString("cool-down") - - if nameFlag != "" && len(workflows) > 1 { - return errors.New("--name flag cannot be used when adding multiple workflows at once") - } - - if err := validateEngine(engineOverride); err != nil { - return err - } - - coolDown, err := parseCoolDownFlag(coolDownStr) - if err != nil { - return fmt.Errorf("invalid --cool-down value: %w", err) - } - - opts := AddOptions{ - Verbose: verbose, - EngineOverride: engineOverride, - Name: nameFlag, - Force: forceFlag, - AppendText: appendText, - NoGitattributes: noGitattributes, - WorkflowDir: workflowDir, - NoStopAfter: noStopAfter, - StopAfter: stopAfter, - DisableSecurityScanner: disableSecurityScanner, - } - - return runDeploy(cmd.Context(), targetRepo, workflows, opts, coolDown) + return runDeployCommand(cmd, args, validateEngine) }, } + registerDeployFlags(cmd) + + return cmd +} + +func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOpts AddOptions, coolDown time.Duration) error { + checkoutDir, originalDir, err := prepareDeployCheckout(ctx, targetRepo) + if err != nil { + return err + } + defer func() { + _ = os.Chdir(originalDir) + }() + + resolvedWorkflows := resolveDeployWorkflowSpecs(workflows, originalDir) + + if err := os.Chdir(checkoutDir); err != nil { + return fmt.Errorf("failed to change directory to checkout %s: %w", checkoutDir, err) + } + + if err := runDeployUpdatePass(ctx, addOpts, coolDown); err != nil { + return err + } + + if err := runDeployAddPass(ctx, resolvedWorkflows, addOpts); err != nil { + return err + } + + if err := runDeployCompilePass(ctx, addOpts); err != nil { + return err + } + + if err := createDeployPR(resolvedWorkflows, targetRepo, addOpts.Verbose); err != nil { + return err + } + + deployLog.Printf("Successfully deployed workflows to %s", targetRepo) + return nil +} + +func validateDeployArgs(cmd *cobra.Command, args []string) error { + if len(args) < 1 { + return fmt.Errorf("missing workflow specification\n\nUsage:\n %s ...\n\nExamples:\n %[1]s githubnext/agentics/ci-doctor --repo owner/repo\n\nRun '%[1]s --help' for more information", cmd.CommandPath()) + } + return nil +} + +func registerDeployFlags(cmd *cobra.Command) { addRepoFlag(cmd) cmd.Flags().StringP("name", "n", "", "Specify name for the added workflow (without .md extension)") addEngineFlag(cmd) @@ -103,40 +102,86 @@ Examples: RegisterEngineFlagCompletion(cmd) RegisterDirFlagCompletion(cmd, "dir") +} - return cmd +func runDeployCommand(cmd *cobra.Command, workflows []string, validateEngine func(string) error) error { + targetRepo, _ := cmd.Flags().GetString("repo") + if strings.TrimSpace(targetRepo) == "" { + return errors.New("--repo flag is required (target repository in owner/repo format)") + } + + opts, coolDown, err := parseDeployCommandOptions(cmd, workflows, validateEngine) + if err != nil { + return err + } + + return runDeploy(cmd.Context(), targetRepo, workflows, opts, coolDown) } -func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOpts AddOptions, coolDown time.Duration) error { +func parseDeployCommandOptions(cmd *cobra.Command, workflows []string, validateEngine func(string) error) (AddOptions, time.Duration, error) { + engineOverride, _ := cmd.Flags().GetString("engine") + nameFlag, _ := cmd.Flags().GetString("name") + forceFlag, _ := cmd.Flags().GetBool("force") + appendText, _ := cmd.Flags().GetString("append") + verbose, _ := cmd.Flags().GetBool("verbose") + noGitattributes, _ := cmd.Flags().GetBool("no-gitattributes") + workflowDir, _ := cmd.Flags().GetString("dir") + noStopAfter, _ := cmd.Flags().GetBool("no-stop-after") + stopAfter, _ := cmd.Flags().GetString("stop-after") + disableSecurityScanner, _ := cmd.Flags().GetBool("disable-security-scanner") + coolDownStr, _ := cmd.Flags().GetString("cool-down") + + if nameFlag != "" && len(workflows) > 1 { + return AddOptions{}, 0, errors.New("--name flag cannot be used when adding multiple workflows at once") + } + if err := validateEngine(engineOverride); err != nil { + return AddOptions{}, 0, err + } + + coolDown, err := parseCoolDownFlag(coolDownStr) + if err != nil { + return AddOptions{}, 0, fmt.Errorf("invalid --cool-down value: %w", err) + } + + return AddOptions{ + Verbose: verbose, + EngineOverride: engineOverride, + Name: nameFlag, + Force: forceFlag, + AppendText: appendText, + NoGitattributes: noGitattributes, + WorkflowDir: workflowDir, + NoStopAfter: noStopAfter, + StopAfter: stopAfter, + DisableSecurityScanner: disableSecurityScanner, + }, coolDown, nil +} + +func prepareDeployCheckout(ctx context.Context, targetRepo string) (string, string, error) { gitRoot, err := gitutil.FindGitRoot() if err != nil { - return fmt.Errorf("deploy command requires running inside a git repository: %w", err) + return "", "", fmt.Errorf("deploy command requires running inside a git repository: %w", err) } updatesDir, err := ensureUpdateTargetRepoGitignore(gitRoot) if err != nil { - return err + return "", "", err } checkoutDir := filepath.Join(updatesDir, sanitizeRepoPath(targetRepo)) if err := shallowCloneTargetRepo(ctx, targetRepo, checkoutDir); err != nil { - return err + return "", "", err } originalDir, err := os.Getwd() if err != nil { - return fmt.Errorf("failed to read current directory: %w", err) + return "", "", fmt.Errorf("failed to read current directory: %w", err) } - defer func() { - _ = os.Chdir(originalDir) - }() - - resolvedWorkflows := resolveDeployWorkflowSpecs(workflows, originalDir) - if err := os.Chdir(checkoutDir); err != nil { - return fmt.Errorf("failed to change directory to checkout %s: %w", checkoutDir, err) - } + return checkoutDir, originalDir, nil +} +func runDeployUpdatePass(ctx context.Context, addOpts AddOptions, coolDown time.Duration) error { if err := PreflightCheckForCreatePR(addOpts.Verbose); err != nil { return err } @@ -153,7 +198,10 @@ func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOp if err := RunUpdateWorkflows(ctx, updateOpts); err != nil { return fmt.Errorf("failed to update existing workflows: %w", err) } + return nil +} +func runDeployAddPass(ctx context.Context, resolvedWorkflows []string, addOpts AddOptions) error { workflowsToAdd, skippedWorkflows, err := excludeExistingSourcedWorkflows(resolvedWorkflows, addOpts) if err != nil { return fmt.Errorf("failed to inspect existing workflows: %w", err) @@ -161,15 +209,18 @@ func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOp if len(skippedWorkflows) > 0 { deployLog.Printf("Skipping add for existing sourced workflows (already handled by update): %s", strings.Join(skippedWorkflows, ", ")) } - - if len(workflowsToAdd) > 0 { - if _, err := AddWorkflows(ctx, workflowsToAdd, addOpts); err != nil { - return fmt.Errorf("failed to add workflows: %w", err) - } - } else { + if len(workflowsToAdd) == 0 { deployLog.Print("No new workflows to add after update pass") + return nil + } + + if _, err := AddWorkflows(ctx, workflowsToAdd, addOpts); err != nil { + return fmt.Errorf("failed to add workflows: %w", err) } + return nil +} +func runDeployCompilePass(ctx context.Context, addOpts AddOptions) error { compileConfig := CompileConfig{ Verbose: addOpts.Verbose, EngineOverride: addOpts.EngineOverride, @@ -179,14 +230,15 @@ func runDeploy(ctx context.Context, targetRepo string, workflows []string, addOp if _, err := CompileWorkflows(ctx, compileConfig); err != nil { return fmt.Errorf("failed to compile workflows with purge: %w", err) } + return nil +} +func createDeployPR(resolvedWorkflows []string, targetRepo string, verbose bool) error { prTitle, prBody := buildDeployPRMetadata(resolvedWorkflows, targetRepo) - _, err = CreatePRWithChanges("deploy-workflows", deployCommitMessage, prTitle, prBody, addOpts.Verbose) + _, err := CreatePRWithChanges("deploy-workflows", deployCommitMessage, prTitle, prBody, verbose) if err != nil { return fmt.Errorf("failed to create deploy pull request: %w", err) } - - deployLog.Printf("Successfully deployed workflows to %s", targetRepo) return nil } diff --git a/pkg/cli/deploy_command_test.go b/pkg/cli/deploy_command_test.go index 697fc882937..74eb2157567 100644 --- a/pkg/cli/deploy_command_test.go +++ b/pkg/cli/deploy_command_test.go @@ -3,6 +3,7 @@ package cli import ( + "errors" "os" "path/filepath" "testing" @@ -189,3 +190,56 @@ func TestResolveDeployWorkflowSpecs_ResolvesRelativeWildcardLocalPaths(t *testin assert.Equal(t, filepath.Join(baseDir, "*.md"), workflows[0]) } + +func TestParseDeployCommandOptions_NameFlagWithMultipleWorkflows(t *testing.T) { + t.Parallel() + + cmd := NewDeployCommand(func(string) error { return nil }) + require.NotNil(t, cmd) + require.NoError(t, cmd.Flags().Set("name", "custom-workflow")) + + validateEngineCalled := false + opts, coolDown, err := parseDeployCommandOptions(cmd, []string{"a", "b"}, func(string) error { + validateEngineCalled = true + return nil + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "--name flag cannot be used when adding multiple workflows at once") + assert.Equal(t, AddOptions{}, opts) + assert.Zero(t, coolDown) + assert.False(t, validateEngineCalled) +} + +func TestParseDeployCommandOptions_InvalidCoolDown(t *testing.T) { + t.Parallel() + + cmd := NewDeployCommand(func(string) error { return nil }) + require.NotNil(t, cmd) + require.NoError(t, cmd.Flags().Set("cool-down", "not-a-duration")) + + opts, coolDown, err := parseDeployCommandOptions(cmd, []string{"a"}, func(string) error { return nil }) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid --cool-down value") + assert.Equal(t, AddOptions{}, opts) + assert.Zero(t, coolDown) +} + +func TestParseDeployCommandOptions_EngineValidationError(t *testing.T) { + t.Parallel() + + cmd := NewDeployCommand(func(string) error { return nil }) + require.NotNil(t, cmd) + require.NoError(t, cmd.Flags().Set("engine", "custom-engine")) + + var validatedEngine string + expectedErr := errors.New("engine invalid") + opts, coolDown, err := parseDeployCommandOptions(cmd, []string{"a"}, func(engine string) error { + validatedEngine = engine + return expectedErr + }) + require.Error(t, err) + assert.ErrorIs(t, err, expectedErr) + assert.Equal(t, "custom-engine", validatedEngine) + assert.Equal(t, AddOptions{}, opts) + assert.Zero(t, coolDown) +}