refactor(cli): split long functions in bootstrap_profile_runner to fix largefunc lint#46332
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors bootstrap/profile runner functions into focused helpers to satisfy largefunc lint limits without changing CLI behavior.
Changes:
- Extracts bootstrap action dispatch and GitHub App credential flows.
- Extracts GitHub App setup and HTTP mux construction.
- Refreshes generated workflow action-version metadata.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/bootstrap_profile_runner.go |
Splits long bootstrap functions into helpers. |
.github/workflows/skillet.lock.yml |
Refreshes generated checkout metadata. |
.github/workflows/release.lock.yml |
Refreshes generated setup-go metadata. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Refreshes generated checkout metadata. |
.github/workflows/avenger.lock.yml |
Refreshes generated checkout metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Medium
🤖 PR Triage
Rationale: Mechanical largefunc lint fix for bootstrap_profile_runner. Modest diff (209+/156−, 5 files). Review together with #46335 as a batch.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — clean mechanical refactor with no behavioral changes detected.
📋 Key Themes & Highlights
Key Themes
- Extraction correctness: All three extracted helpers faithfully preserve the original logic. The guard condition inversion in
handleBootstrapGitHubAppCreateOrExistingChoice(line 472) correctly applies De Morgan's law. - No test coverage: This is a pure structural refactor, so the absence of new tests is acceptable — the existing test suite covers the behavior.
- One minor nit:
setupBootstrapGitHubAppDetailsuses named return values with barereturnstatements; mixing named returns with a function that also setsappName,homepageURL,descriptionvia assignment is slightly harder to follow at a glance, but it's idiomatic Go for this kind of multi-value extraction.
Positive Highlights
- ✅
bootstrapGitHubAppFlowChannelsstruct is a clean solution to the parameter-count lint constraint - ✅
io.WriteStringin place ofw.Write([]byte(...))is a good opportunistic improvement - ✅ All extracted functions have clear, accurate doc comments
- ✅ The CSRF
state→csrfStaterename inbuildBootstrapGitHubAppMuximproves local clarity
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.6 AIC · ⌖ 6.16 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
The refactoring is clean and correct. All behavioral equivalences hold:
- The
continuetoreturn nilchange inapplyBootstrapActionfor thecopilot-authcase is safe — the extracted function handles one action at a time. handleBootstrapGitHubAppExistingFlowcorrectly inverts the condition guard.handleBootstrapGitHubAppCreateOrExistingChoicecorrectly passes empty strings matching the original code path.bootstrapGitHubAppFlowChannelsis a clean encapsulation with correctly typed send-only channels.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 30.3 AIC · ⌖ 4.29 AIC · ⊞ 5K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (201 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. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Non-blocking observations
The extraction is mechanically correct — no control-flow changes, no dropped errors. Two maintainability concerns worth addressing (see inline comments).
- Guard semantics in
handleBootstrapGitHubAppCreateOrExistingChoice: The guard condition correctly preserves original behavior but the function name implies tighter scope than its actual trigger conditions. - Naked returns in
setupBootstrapGitHubAppDetails: Technically correct today, fragile to future additions.
🔎 Code quality review by PR Code Quality Reviewer · 103.8 AIC · ⌖ 4.69 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/bootstrap_profile_runner.go:340
Guard condition in handleBootstrapGitHubAppCreateOrExistingChoice is subtly wrong: it admits the function body when overrides.Mode == "create" even without action.Mode == "create-or-existing", diverging from the original createNew gate.
<details>
<summary>💡 Details</summary>
Original guard:
createNew := action.Mode == "create-or-existing" || overrides.Mode == "create"
if createNew {New guard (De Morgan negation):
if action.Mode != "create-or-existing" && overrid…
</details>
<details><summary>pkg/cli/bootstrap_profile_runner.go:452</summary>
**Naked returns in `setupBootstrapGitHubAppDetails` leave return values undefined on the error path**: the named return variables are zero-value strings if the early `return` fires, which is safe today but fragile to future callers.
<details>
<summary>💡 Details</summary>
```go
if overrides.Owner != "" {
appOwner = overrides.Owner
appOwnerType, err = bootstrapCheckOwnerType(ctx, appOwner)
if err != nil {
return // naked return — appName, homepageURL, description are all "…
</details>
<details><summary>pkg/cli/bootstrap_profile_runner.go:395</summary>
**Regression: `buildBootstrapGitHubAppMux` receives the original `owner`/`ownerType` instead of the override-resolved `appOwner`/`appOwnerType`**, causing GitHub App creation to use the wrong org/user when `overrides.Owner` is set.
<details>
<summary>💡 Details</summary>
Line 395:
```go
Handler: buildBootstrapGitHubAppMux(ctx, state, owner, ownerType, appName, description, registrationPage, flowCh),In the original code appOwner and appOwnerType were set in the same scope and were us…
|
@copilot run pr-finisher skill |
|
@copilot please run the Unresolved review feedback:
|
Running pr-finisher. Summary:
Actions taken: Triage only — no code changes needed. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…r/appOwnerType to buildBootstrapGitHubAppMux; update ADR Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed in commit Naked returns ( Owner propagation ( Guard semantics ( ADR: Updated to ADR linkage in PR body: The design gate requires adding Local validation: |
pkg/cli/bootstrap_profile_runner.gohad 14largefunclint findings (limit: 60 lines), concentrated in three functions. All three are split into focused helpers without behavior change.Extractions
executeBootstrapProfile(78 → 32 lines): action-type dispatch moved toapplyBootstrapActionrunBootstrapGitHubAppAction(86 → ~59 lines): two credential-resolution paths extracted:handleBootstrapGitHubAppExistingFlow— env-var / explicit-existing modehandleBootstrapGitHubAppCreateOrExistingChoice— interactivecreate-or-existingpromptcreateBootstrapGitHubApp(118 → ~56 lines): two blocks extracted:setupBootstrapGitHubAppDetails— owner override + app name/homepage/description derivationbuildBootstrapGitHubAppMux— HTTP/register+/callbackhandler setupSupporting changes
bootstrapGitHubAppFlowChannelsstruct to keepbuildBootstrapGitHubAppMuxunder the 8-parameter limit, grouping the result/error send channels.io.WriteStringreplacesw.Write([]byte(...))in the mux helper.