Refactor deploy command orchestration to satisfy largefunc limits in pkg/cli#36144
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pkg/cli
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| if err := validateEngine(engineOverride); err != nil { | ||
| return AddOptions{}, 0, fmt.Errorf("failed to validate engine: %w", err) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactoring — no blocking issues
All execution-ordering invariants from the original runDeploy are preserved: the defer os.Chdir(originalDir) is correctly re-anchored in runDeploy; resolveDeployWorkflowSpecs is still called with originalDir before the second os.Chdir(checkoutDir); all sub-passes (Update -> Add -> Compile -> PR) run in the right CWD context.
The one observable behaviour change — engine-validation errors are now wrapped with failed to validate engine — is intentional, tested with errors.Is, and an improvement for user clarity.
Test coverage for parseDeployCommandOptions is solid: cardinality short-circuit, invalid cool-down, and engine-error propagation are all covered.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.4M
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (180 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Even mechanical decompositions encode a real decision — here, honor the 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in 🔒 Blocking: link the completed ADR in the PR body to clear this gate.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, and /tdd — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Implicit caller contract on
prepareDeployCheckout: returns dirs but leaves chdir + defer to the caller as an undocumented 3-step sequence (line 59) - Over-wide
AddOptionsthreading: stage helpers accept the full 10-field struct but use 2–3 fields each — dependencies are invisible - Engine error wrapping: subtle observable behaviour change; existing tests use
assert.ErrorIscorrectly but worth a grep for any message-matching tests elsewhere - Test coverage gap on stage helpers: the 5 new stage functions are now independently testable but have no tests yet
Positive Highlights
- ✅ Clean linear stage sequence in
runDeploy— easy to read and follow - ✅ New tests are well-structured with
requirefor setup,assertfor assertions, and parallel execution - ✅
createDeployPRcorrectly scopes to only what it needs (verbose bool), unlike the stage helpers - ✅ Zero change to public signatures, flags, or user-visible error messages (except the intentional engine validation wrapping)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.7M
|
|
||
| resolvedWorkflows := resolveDeployWorkflowSpecs(workflows, originalDir) | ||
|
|
||
| if err := os.Chdir(checkoutDir); err != nil { |
There was a problem hiding this comment.
[/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 checkoutDir, originalDir, nil | ||
| } | ||
|
|
||
| func runDeployUpdatePass(ctx context.Context, addOpts AddOptions, coolDown time.Duration) error { |
There was a problem hiding this comment.
[/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.
| 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, fmt.Errorf("failed to validate engine: %w", err) |
There was a problem hiding this comment.
[/zoom-out] The engine validation error is now wrapped with "failed to validate engine: %w" — this wasn't present before and is a silent observable behaviour change for any caller or test that matches on the raw error message.
💡 Detail
The PR description calls this out as an intentional consistency improvement, which is fine. But the new test TestParseDeployCommandOptions_EngineValidationError uses assert.ErrorIs (correct), while the existing broader test suite may have strings.Contains(err.Error(), ...) checks that now need updating. Worth a quick grep before merging:
grep -r 'validateEngine\|engine.*invalid\|invalid.*engine' pkg/cli/ --include='*_test.go'| assert.ErrorIs(t, err, expectedErr) | ||
| assert.Equal(t, "custom-engine", validatedEngine) | ||
| assert.Equal(t, AddOptions{}, opts) | ||
| assert.Zero(t, coolDown) |
There was a problem hiding this comment.
[/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 pathTestRunDeployAddPass_ReturnsErrorOnAddFailure— error propagation
These are the highest-value additions given the existing happy-path is tested end-to-end.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics & Test Classification (3 new tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 3 new tests cover behavioral error-path contracts for the extracted helper functions. Please add descriptive message arguments to bare assert.* calls before merging.
|
@copilot run copilot-review skill |
…t with add/update commands Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the |
|
|
|
Please post a short completion plan for the deploy refactor blockers.
|
|
|
pelikhan
deleted the
copilot/lint-monster-fix-function-length-violations-again
branch
pkg/clihas widespread function-length violations under the 60-line custom lint rule. This PR addresses the deploy command slice by decomposing long command wiring/orchestration paths into focused internal helpers while preserving CLI behavior and public surfaces.Command construction split (
NewDeployCommand)validateDeployArgsregisterDeployFlagsrunDeployCommandparseDeployCommandOptionsExecution flow decomposition (
runDeploy)prepareDeployCheckoutrunDeployUpdatePassrunDeployAddPassrunDeployCompilePasscreateDeployPRupdate -> add -> compile --purge -> create PR) and existing error paths.Error-handling consistency
failed to validate engine: %wFocused test coverage for extracted parsing paths
--namewith multiple workflows (short-circuit validation behavior)--cool-down