Skip to content

Refactor deploy command orchestration to satisfy largefunc limits in pkg/cli #36144

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.

Sign up for GitHub

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

Merged
pelikhan merged 7 commits into from
May 31, 2026
+219 −74
Merged

Refactor deploy command orchestration to satisfy largefunc limits in pkg/cli #36144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
33d8c67
Initial plan
Copilot May 31, 2026
8bd4cff
refactor deploy command into focused helpers
Copilot May 31, 2026
435a516
test deploy option parsing engine validation
Copilot May 31, 2026
b73d5cc
harden deploy option parsing tests
Copilot May 31, 2026
ba02652
wrap deploy engine validation errors with context
Copilot May 31, 2026
abfa592
Add draft ADR for deploy command orchestration refactor
github-actions[bot] May 31, 2026
63ca5d8
return engine validation error unwrapped in deploy command, consisten…
Copilot May 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions docs/adr/36144-decompose-deploy-command-orchestration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.*
200 changes: 126 additions & 74 deletions pkg/cli/deploy_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <workflow>...\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 {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] prepareDeployCheckout returns the checkout dir but leaves Chdir to the caller — the 3-step sequence (defer restore → chdir → resolveWorkflowSpecs) is an implicit contract that future callers can easily break.

💡 Suggestion

Either have prepareDeployCheckout also Chdir into the checkout and return an undo func, or add a doc comment on the function explicitly documenting the caller's obligations:

// prepareDeployCheckout clones targetRepo and returns (checkoutDir, originalDir).
// The caller is responsible for:
//   1. deferring os.Chdir(originalDir) before any other work
//   2. calling os.Chdir(checkoutDir) before invoking stage helpers
func prepareDeployCheckout(...) (string, string, error)

Without this, the contract is invisible and fragile.

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 <workflow>...\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)
Expand All @@ -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
}
Comment on lines +137 to +139

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 {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] runDeployUpdatePass accepts full AddOptions (10+ fields) but only reads Verbose and WorkflowDir from it. The same is true for runDeployCompilePass and runDeployAddPass.

💡 Why this matters

Passing the whole struct makes the function's actual dependencies invisible — a reader can't tell what runDeployUpdatePass actually needs without reading its body. It also tightly couples these stage helpers to AddOptions shape changes.

Since the goal here is cleaner decomposition, consider threading only the fields each stage truly needs, or at minimum adding a comment on each helper listing which fields it reads.

if err := PreflightCheckForCreatePR(addOpts.Verbose); err != nil {
return err
}
Expand All @@ -153,23 +198,29 @@ 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)
}
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,
Expand All @@ -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
}

Expand Down
54 changes: 54 additions & 0 deletions pkg/cli/deploy_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package cli

import (
"errors"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -189,3 +190,56 @@

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)

Check failure on line 241 in pkg/cli/deploy_command_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

require-error: for error assertions use require (testifylint)
assert.Equal(t, "custom-engine", validatedEngine)
assert.Equal(t, AddOptions{}, opts)
assert.Zero(t, coolDown)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Tests cover parseDeployCommandOptions error paths well, but the five new stage helpers (prepareDeployCheckout, runDeployUpdatePass, runDeployAddPass, runDeployCompilePass, createDeployPR) have no unit tests. The refactoring makes them independently testable for the first time.

💡 Suggestion

This PR's primary goal is lint compliance, so skipping stage-level tests is a reasonable trade-off. But since these functions now have clear signatures, they're much easier to test in isolation than the original monolith. Consider a follow-up to add at least:

  • TestRunDeployAddPass_SkipsWhenNoNewWorkflows — verifies the early-return path
  • TestRunDeployAddPass_ReturnsErrorOnAddFailure — error propagation

These are the highest-value additions given the existing happy-path is tested end-to-end.

}
Loading