Refactor bootstrap profile runner into focused modules#46855
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 execution into focused modules while preserving the CLI flow.
Changes:
- Separates repository, GitHub App, git, and helper logic.
- Splits and expands corresponding unit tests.
- Adds injection support for testing GitHub App code exchange.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/bootstrap_profile_runner.go |
Retains orchestration and shared types. |
pkg/cli/bootstrap_profile_runner_test.go |
Focuses orchestration tests. |
pkg/cli/bootstrap_profile_actions_repo.go |
Handles repository variables, secrets, and Copilot auth. |
pkg/cli/bootstrap_profile_actions_repo_test.go |
Tests repository actions. |
pkg/cli/bootstrap_profile_github_app.go |
Contains GitHub App setup and installation flow. |
pkg/cli/bootstrap_profile_github_app_test.go |
Tests GitHub App flows; exceeds the 500-line acceptance criterion. |
pkg/cli/bootstrap_profile_git.go |
Contains commit-and-push operations. |
pkg/cli/bootstrap_profile_git_test.go |
Tests git operations. |
pkg/cli/bootstrap_profile_helpers.go |
Contains shared parsing, UI, and manifest helpers. |
pkg/cli/bootstrap_profile_helpers_test.go |
Tests shared helpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Medium
| }) | ||
| } | ||
|
|
||
| func TestBuildBootstrapGitHubAppMux(t *testing.T) { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 89/100 — Excellent
📊 Metrics (36 tests)
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (2,177 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 Matter
ADRs 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor bootstrap profile runner into focused modules
The refactor is well-structured — concerns are cleanly separated, the new modules are appropriately sized, and the injectable function variables preserve testability. Two non-blocking issues worth addressing:
-
Unused
ctxin concrete implementations —upsertBootstrapRepoVariableandsetBootstrapRepoSecretacceptctx context.Contextto match the injectable var signature but never use it; name it_to make the intent explicit. -
Partial credential write (pre-existing, now more visible) — If writing
AppIDVariablesucceeds butPrivateKeySecretfails, the repo is left in an inconsistent state that silently prevents recovery on re-run. Worth reversing the variable on secret failure, or at minimum documenting the operator recovery step.
The 539-line test file size issue is already covered by an existing review comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 50.8 AIC · ⌖ 4.52 AIC · ⊞ 5K
Comments that could not be inline-anchored
pkg/cli/bootstrap_profile_actions_repo.go:97
Both upsertBootstrapRepoVariable and setBootstrapRepoSecret accept a ctx context.Context parameter that is never used in the function body. Since these are the concrete implementations assigned to the injectable bootstrapUpsertVariable/bootstrapSetSecret function variables, the parameter must match the type signature — but the unused parameter should use _ naming (e.g. _ context.Context) to make the intent explicit and avoid confusing future readers.
@copilot please address this.
pkg/cli/bootstrap_profile_github_app.go:546
Partial credential write on error: if bootstrapUpsertVariable for AppIDVariable succeeds but the subsequent bootstrapSetSecret for PrivateKeySecret fails, the repo ends up with the variable set but no private-key secret. The next run will skip the action entirely (hasVar && hasSecret check at the top is false because the secret is still missing, but hasVar is now true), so re-running won't re-attempt the variable write and won't prompt for the private key via the normal path…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on interface honesty, stdlib usage, and a potential hang.
📋 Key Themes & Highlights
Issues Found
-
Unused
ctxparameters —upsertBootstrapRepoVariableandsetBootstrapRepoSecretacceptctxbut neither underlying function (upsertDefaultsVariable,setRepoSecret) takes a context. Cancellation and deadlines are silently lost; callers deserve an honest interface. -
Unbounded server shutdown —
server.Shutdown(context.Background())in thedefercan hang indefinitely if an in-flight callback request is slow. A 5-second deadline is sufficient here. -
htmlEscapereinventshtml.EscapeString— the standard library covers all HTML entities including'; a hand-rolled replacer will miss them over time. -
firstNonEmptytrims for emptiness but returns untrimmed — callers compensate with repeatedstrings.TrimSpacecalls. The function should trim on return or not trim at all.
Positive Highlights
- ✅ Clean split into focused modules — the separation of git, GitHub App, repo actions, and helpers is well-judged and makes the code significantly more navigable.
- ✅ Good test coverage across all new modules, including non-interactive paths and edge cases.
- ✅ The polling loop in
waitForBootstrapGitHubAppInstallationcorrectly handles retryable errors vs. hard failures. - ✅ Idempotency guards (
state.variables,state.secrets) are preserved in every action function.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.4 AIC · ⌖ 4.91 AIC · ⊞ 6.7K
Comment /matt to run again
| if err != nil { | ||
| return err | ||
| } | ||
| target.scope = defaultsScopeRepo |
There was a problem hiding this comment.
[/codebase-design] upsertBootstrapRepoVariable accepts ctx but silently discards it — upsertDefaultsVariable does not take a context, so cancellation and deadlines are lost. The same applies to setBootstrapRepoSecret (line 109).
💡 Options
Either propagate context into the underlying functions (preferred for long-term correctness), or drop the unused ctx parameter from both wrappers to keep the interface honest:
func upsertBootstrapRepoVariable(repo, name, value string) error { ... }
func setBootstrapRepoSecret(repo, name, value string) error { ... }Callers already have ctx in scope, so this is a mechanical change.
@copilot please address this.
| return "https://github.com/settings/apps/new?state=" + state | ||
| } | ||
|
|
||
| func renderBootstrapGitHubAppRegistrationPage(registrationURL string, manifest map[string]any) (string, error) { |
There was a problem hiding this comment.
[/codebase-design] htmlEscape reimplements html.EscapeString from the standard library — using a hand-rolled replacer risks missing edge cases and adds maintenance surface.
💡 Suggested fix
import "html"
func htmlEscape(value string) string {
return html.EscapeString(value)
}Or inline html.EscapeString directly at the two call sites in renderBootstrapGitHubAppRegistrationPage.
@copilot please address this.
| return hex.EncodeToString(buf), nil | ||
| } | ||
|
|
||
| func htmlEscape(value string) string { |
There was a problem hiding this comment.
[/codebase-design] htmlEscape reimplements html.EscapeString from the standard library — a hand-rolled replacer risks missing future escape cases (e.g., single-quote) and adds maintenance surface.
💡 Suggested fix
import "html"
func htmlEscape(value string) string {
return html.EscapeString(value)
}Or inline html.EscapeString at the two call sites in renderBootstrapGitHubAppRegistrationPage and remove the helper entirely.
@copilot please address this.
| return net.Listen("tcp", "127.0.0.1:0") | ||
| } | ||
|
|
||
| func firstNonEmpty(values ...string) string { |
There was a problem hiding this comment.
[/codebase-design] firstNonEmpty checks strings.TrimSpace(value) != "" but returns the original untrimmed value — callers then call strings.TrimSpace again on the result (e.g., lines 732–737 in bootstrap_profile_github_app.go). Either trim before returning or document that callers are expected to trim.
💡 Option A – trim on return (consistent)
func firstNonEmpty(values ...string) string {
for _, value := range values {
if trimmed := strings.TrimSpace(value); trimmed != "" {
return trimmed
}
}
return ""
}This eliminates the redundant strings.TrimSpace at every call site.
@copilot please address this.
| _ = server.Serve(listener) | ||
| }() | ||
| defer func() { | ||
| _ = server.Shutdown(context.Background()) |
There was a problem hiding this comment.
[/codebase-design] server.Shutdown(context.Background()) uses an unbounded context — if the server is slow to shut down (e.g., an in-flight callback request), this defer will block indefinitely with no timeout.
💡 Suggested fix
Use a short deadline so the deferred cleanup is bounded:
defer func() {
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
_ = server.Shutdown(shutdownCtx)
}()@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 5 issues must be fixed before merge
This refactor is structurally sound but the extraction surfaced several correctness and security-adjacent bugs.
Security — bootstrapExchangeGitHubAppCodeImpl: the OAuth code from the browser callback is interpolated directly into a gh api URL path without sanitisation. See inline comment.
Correctness — resolveBootstrapSecretValue: interactive empty input returns ("", true, nil), causing callers to silently store a blank secret.
Correctness — completeExistingGitHubAppCredentials: PEM private key trimmed with TrimSpace instead of TrimRight(..., "\r\n"), inconsistent with every other callsite and capable of corrupting PEM content.
Context propagation — upsertBootstrapRepoVariable and setBootstrapRepoSecret accept ctx but never forward it; cancellation is silently dropped for network operations.
Resource leak — openBootstrapBrowser calls cmd.Start() without cmd.Wait(), leaving zombie processes on Unix.
🔎 Code quality review by PR Code Quality Reviewer · 121.8 AIC · ⌖ 4.95 AIC · ⊞ 5.6K
Comment /review to run again
| func bootstrapExchangeGitHubAppCodeImpl(ctx context.Context, code, owner, ownerType, appName, description string) (*bootstrapCreatedGitHubApp, error) { | ||
| output, err := workflow.RunGHContext(ctx, "Exchanging GitHub App manifest code...", "api", "-X", "POST", "-H", "Accept: application/vnd.github+json", "/app-manifests/"+code+"/conversions") | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Unvalidated OAuth code interpolated into API URL path: code from the browser callback is embedded directly into /app-manifests/"+code+"/conversions with no sanitisation.
💡 Suggested fix
The code parameter comes from r.URL.Query().Get("code") and is never validated before use. A path segment containing ../../ or percent-encoded characters could trigger unintended API endpoints. Validate the code before passing it downstream:
// in the /callback handler, before calling bootstrapExchangeGitHubAppCode:
if !isSimpleToken(code) {
http.Error(w, "Invalid manifest code format.", http.StatusBadRequest)
select { case flowCh.errCh <- errors.New("invalid manifest code format"): default: }
return
}
func isSimpleToken(s string) bool {
for _, c := range s {
if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '_') {
return false
}
}
return len(s) > 0
}GitHub manifest codes are short alphanumeric strings; anything else should be rejected at the callback boundary.
| if trimmed == "" { | ||
| return nil | ||
| } | ||
| lines := strings.Split(trimmed, "\n") |
There was a problem hiding this comment.
Empty secret silently stored as ok=true: in the non-optional interactive path, if the user submits an empty string, resolveBootstrapSecretValue returns ("", true, nil) — a blank secret with no error.
💡 Suggested fix
After the console.PromptSecretInput call:
value = strings.TrimRight(value, "\r\n")
if value == "" {
if optional {
return "", false, nil
}
return "", false, errors.New("secret value cannot be empty")
}
return value, true, nilWithout this guard, runBootstrapRepoSecretAction and runBootstrapCopilotAuthAction will call bootstrapSetSecret with an empty string, silently overwriting a valid existing secret with a blank value in the repository.
| } | ||
| if handled { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
PEM key trimmed inconsistently: runBootstrapGitHubAppAction uses strings.TrimRight(key, "\r\n") to preserve leading whitespace in PEM blocks, but completeExistingGitHubAppCredentials uses strings.TrimSpace on the same value — stripping leading whitespace and potentially corrupting the key.
💡 Suggested fix
In completeExistingGitHubAppCredentials:
// was: privateKey := strings.TrimSpace(existingPrivateKey)
privateKey := strings.TrimRight(existingPrivateKey, "\r\n")This aligns with how the env-var path handles the same field in runBootstrapGitHubAppAction (line ~522). PEM keys begin with -----BEGIN and must not have leading whitespace stripped.
| if err != nil { | ||
| return err | ||
| } | ||
| target.scope = defaultsScopeRepo |
There was a problem hiding this comment.
Context parameter ignored in upsertBootstrapRepoVariable: ctx is accepted but never forwarded, so cancellation and deadlines are silently dropped for the underlying network call.
💡 Details
upsertDefaultsVariable currently takes a defaultsTarget and name/value, with no context parameter. If it makes network calls (REST or git), the caller's context timeout won't propagate and the operation can hang after cancellation.
Same issue exists in setBootstrapRepoSecret — api.NewRESTClient and setRepoSecret don't receive ctx.
If these functions cannot accept a context today, the ctx parameter should either be removed from the exported signature to avoid the false safety signal, or the downstream functions should be updated to accept one.
| return true, nil | ||
| case "0", "false", "no", "off": | ||
| return false, nil | ||
| default: |
There was a problem hiding this comment.
openBootstrapBrowser leaks zombie processes: cmd.Start() is called without a corresponding cmd.Wait(), leaving zombie entries in the process table for every successful browser launch.
💡 Suggested fix
if err := cmd.Start(); err == nil {
go func() { _ = cmd.Wait() }()
return true
}On Unix, a child process that has exited but whose parent hasn't called wait stays in the zombie state. For a short-lived CLI invocation this is usually harmless, but it's a correctness issue and a bad pattern to establish.
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
Thanks for the refactor! 🎯 This PR looks great — it tackles the oversized What stands out:
The separation into This PR is ready for review and merge. 🚀
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining bootstrap-profile review feedback in 4f04aaf. I reran |
|
🎉 This pull request is included in a new release. Release: |
pkg/cli/bootstrap_profile_runner.gohad grown past the healthy size threshold and mixed orchestration, repo mutation, GitHub App setup, git operations, and shared parsing/UI helpers in one file. This change separates those concerns into focused modules while preserving the existing bootstrap flow and CLI surface.Top-level runner
bootstrap_profile_runner.gofocused on orchestration only:executeBootstrapProfileapplyBootstrapActionbootstrapProfileStatebootstrapActionNeedsMutationRepository actions
bootstrap_profile_actions_repo.gofor repository variable/secret and Copilot auth paths:runBootstrapRepoVariableActionrunBootstrapRepoSecretActionrunBootstrapCopilotAuthActionGitHub App flow
bootstrap_profile_github_app.gofor the manifest registration flow, installation polling, and credential handling.Git operations
bootstrap_profile_git.gofor the commit/push bootstrap action and git command helpers.Shared helpers
bootstrap_profile_helpers.gofor parsing, prompt/env resolution, naming, HTML/browser/network helpers, and Copilot permission detection.Tests
Example of the new separation:
pkg/cli/bootstrap_profile_runner.goexceeds healthy size threshold (1259 lines) #46836pr-sous-chef run https://github.com/github/gh-aw/actions/runs/29761732160
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.