feat: merge bootstrap config into add/add-wizard; rename actions→config#45758
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot flatten bootstrap.config to config |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 508fad3. The name: my-package
config:
- type: repo-variable
name: EXAMPLE
prompt: Enter a valueChanges:
|
…implify warning message Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. |
There was a problem hiding this comment.
Pull request overview
Adds package bootstrap configuration support to add and add-wizard, sharing execution logic with bootstrap.
Changes:
- Renames action storage to
Configand adds strict typed schema variants. - Propagates bootstrap profiles through workflow resolution.
- Adds non-interactive checklists and interactive configuration execution.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schemas/aw_manifest_schema.json |
Defines strict configuration variants. |
pkg/cli/bootstrap.go |
Uses renamed configuration field. |
pkg/cli/bootstrap_test.go |
Updates bootstrap tests. |
pkg/cli/bootstrap_profile_runner.go |
Executes renamed configuration entries. |
pkg/cli/bootstrap_profile_manifest.go |
Parses configuration entries. |
pkg/cli/bootstrap_config.go |
Adds shared checklist and execution helpers. |
pkg/cli/add_workflow_resolution.go |
Propagates resolved bootstrap profiles. |
pkg/cli/add_package_manifest.go |
Loads configuration from manifests. |
pkg/cli/add_package_manifest_test.go |
Updates manifest parsing tests. |
pkg/cli/add_interactive_orchestrator.go |
Runs configuration during the wizard. |
pkg/cli/add_command.go |
Prints post-installation steps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Medium
| if configValue, ok := root["config"]; ok { | ||
| warnings = append(warnings, "Using experimental feature: config") | ||
| bootstrap, err := extractManifestConfig(configValue, manifestPath) |
There was a problem hiding this comment.
The public contract is now consistently top-level config:. The schema removes the bootstrap property and defines config as the top-level array — manifests using the old bootstrap: { config: [...] } shape are rejected at schema validation time with a clear error ("Unknown property: bootstrap. Valid fields are: agents, branding, config, ..."). This is verified by the new rejects_old_bootstrap_key_with_schema_error test in add_package_manifest_test.go.
| for _, action := range profile.Profile.Config { | ||
| switch action.Type { | ||
| case "require-owner-type": | ||
| fmt.Fprintf(os.Stderr, " ✓ Repository owner type constraint: %s\n", action.Value) |
There was a problem hiding this comment.
Fixed in e9b7ae2 — require-owner-type now renders as ☐ Verify repository owner type: <value> (unchecked) instead of ✓, since the actual owner type isn't validated at print time.
| if config.resolvedWorkflows != nil && config.resolvedWorkflows.BootstrapProfile != nil { | ||
| if err := executeBootstrapConfigForAdd(ctx, config.RepoOverride, config.WorkflowSpecs, config.resolvedWorkflows.BootstrapProfile, config.Verbose); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in e9b7ae2 — executeBootstrapConfigForAdd is now gated on config.hasWriteAccess. Users without write access fall through to printBootstrapConfigTODO (manual checklist) instead of attempting secret/variable mutations.
|
|
||
| // Step 9b: Apply bootstrap config steps interactively (if the package declares any) | ||
| if config.resolvedWorkflows != nil && config.resolvedWorkflows.BootstrapProfile != nil { | ||
| if err := executeBootstrapConfigForAdd(ctx, config.RepoOverride, config.WorkflowSpecs, config.resolvedWorkflows.BootstrapProfile, config.Verbose); err != nil { |
There was a problem hiding this comment.
Fixed in e9b7ae2 — UseCopilotRequests is now threaded into executeBootstrapConfigForAdd and passed into bootstrapProfileRunConfig, so the runner skips copilot-auth steps when the user has chosen org-billing, consistent with the permissions.copilot-requests: write injection.
| if pkg.Bootstrap != nil { | ||
| bootstrapProfiles = append(bootstrapProfiles, &resolvedBootstrapProfile{ | ||
| PackageID: pkg.ManifestPath, | ||
| Source: workflow, | ||
| Profile: pkg.Bootstrap, |
There was a problem hiding this comment.
Fixed in e9b7ae2 — added TestResolveWorkflows_BootstrapProfile_SinglePackage (asserts BootstrapProfile is populated and correct) and TestResolveWorkflows_BootstrapProfile_MultiplePackagesWarnsAndSuppresses (asserts BootstrapProfile is nil and warning is present).
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: merge bootstrap config into add/add-wizard; rename actions→config
The refactor is well-structured and the rename from bootstrap.actions → bootstrap.config is cleanly executed. Five issues were identified (all already inline-commented):
Blocking
-
Schema / manifest key mismatch (
add_package_manifest.go:299) — The parser reads a top-levelconfig:key, but the schema still describes a nestedbootstrap:section. Manifest authors following the schema will writebootstrap: { config: [...] }and silently get no bootstrap; those writing the flatconfig: [...]the parser now expects will fail schema validation. -
require-owner-typealways shown as ✓ (bootstrap_config.go:25) — The TODO list unconditionally renders a check-mark without checking the actual target repository. Users installing into a non-qualifying repo see a false "satisfied" signal. -
Bootstrap mutations attempted without write access (
add_interactive_orchestrator.go:172) —executeBootstrapConfigForAddis called even when the wizard took the no-write-access path (PR-only flow). Secret/variable mutations will fail post-merge with no user guidance.
Non-blocking
-
WorkflowSpecspassed instead of installed specs (add_interactive_orchestrator.go:169) —executeBootstrapConfigForAddreceives original source specs, socopilot-authpermission injection may not match what was committed. -
Missing test coverage for
BootstrapProfilepropagation (add_workflow_resolution.go:98) — No test asserts single-package →BootstrapProfilepopulated, multi-package → warning+nil, or nil-guard inprintBootstrapConfigTODO.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 21.6 AIC · ⌖ 6.84 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (2 test files, ~60 changed hunks)
|
…add-wizard commands
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (421 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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.
REQUEST_CHANGES — Two correctness/usability issues and one testability gap that should be fixed before merge.
### Blocking findings summary
- Silent breaking change (
add_package_manifest.go:297) — manifests still using the oldbootstrap:key silently skip all bootstrap processing with no warning. This is a regression for any existing consumer that hasn't migrated. - Empty repo not validated (
bootstrap_config.go:302) —executeBootstrapConfigForAddforwards an empty string toexecuteBootstrapProfilewhen--repois omitted, producing confusing downstream API errors instead of a clear early failure. - Hardcoded
os.Stderr(bootstrap_config.go:244) —printBootstrapConfigTODOuses the global stderr, making the new code untestable and inconsistent with the rest of the CLI.
Existing review comments on owner-type always-✓, write-access wizard path, and test coverage gaps also remain open.
🔎 Code quality review by PR Code Quality Reviewer · 56.9 AIC · ⌖ 4.7 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/add_package_manifest.go:297
Silent breaking change: manifests still using the old bootstrap: key are silently ignored with no warning or error.
<details>
<summary>💡 Details and suggested fix</summary>
The diff replaces root["bootstrap"] with root["config"] but adds no fallback, deprecation warning, or migration error for the old key. Any package manifest that has not been migrated will skip bootstrap processing entirely — the user gets no indication that their bootstrap: section was ignored.
Suggested fix:…
pkg/cli/bootstrap_config.go:302
Missing validation: executeBootstrapConfigForAdd passes an empty repo string to executeBootstrapProfile when the user omits --repo.
<details>
<summary>💡 Details and suggested fix</summary>
config.RepoOverride can be empty if the user did not provide --repo. The function only guards against profile == nil, then immediately calls executeBootstrapProfile with an empty repo slug. All downstream API calls (setting secrets, variables, GitHub App) will fail with confusing errors…
pkg/cli/bootstrap_config.go:244
Untestable output: printBootstrapConfigTODO writes directly to os.Stderr instead of accepting an io.Writer, making it impossible to capture output in tests.
<details>
<summary>💡 Details and suggested fix</summary>
All output paths in the function use fmt.Fprintln(os.Stderr, ...) with the global stderr. This means:
- Unit tests cannot assert the output content
- CLI callers cannot redirect or suppress it
- It's inconsistent with the rest of the CLI which threads the cobra `OutOrSt…
There was a problem hiding this comment.
Skills-Based Review
Applied /codebase-design, /tdd, /domain-modeling — requesting changes on correctness and coverage gaps.
Key issues:
- Silent breaking change (add_package_manifest.go:297): the bootstrap→config rename has no deprecation path; existing manifests silently lose bootstrap steps.
- Unchecked owner-type (bootstrap_config.go:25): require-owner-type always shows ✓ without verifying the actual repo owner.
- Missing test coverage (add_workflow_resolution.go:98,147): BootstrapProfile propagation and multi-profile conflict path are untested.
- Permission drift (add_interactive_orchestrator.go:169): wizard passes original sources (not post-mutation installed ones) to executeBootstrapConfigForAdd.
Positive highlights:
- Clean extraction into bootstrap_config.go
- Schema tightened with 6 typed anyOf variants — strong improvement
- Multi-package conflict detection is solid defensive design
- printBootstrapConfigTODO / executeBootstrapConfigForAdd split is a clear seam
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.2 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
| PackageID: packageID, | ||
| Source: workflow, | ||
| Profile: pkg.Bootstrap, | ||
| }) |
There was a problem hiding this comment.
[/tdd] The multi-package conflict path (multiple bootstrap profiles → warn + skip) has no test coverage. A regression here would silently stop applying bootstrap config.
💡 Suggested test
Add a table test in the existing ResolveWorkflows test suite:
// two packages each with bootstrap config
// expect BootstrapProfile == nil and warnings contain "multiple bootstrap profiles"@copilot please address this.
There was a problem hiding this comment.
Covered by TestResolveWorkflows_BootstrapProfile_MultiplePackagesWarnsAndSuppresses added in e9b7ae2 — it resolves two packages each declaring a config section, asserts BootstrapProfile is nil, and verifies the "multiple bootstrap profiles found" warning contains both package IDs.
| if bootstrapValue, ok := root["bootstrap"]; ok { | ||
| warnings = append(warnings, "Using experimental feature: manifest.bootstrap") | ||
| bootstrap, err := extractManifestBootstrap(bootstrapValue, manifestPath) | ||
| if configValue, ok := root["config"]; ok { |
There was a problem hiding this comment.
[/codebase-design] Silent breaking change: existing manifests using bootstrap: { actions: [...] } will silently produce no bootstrap config — no error, no warning, no migration hint.
💡 Suggested fix
Add a deprecation warning when the old key is present:
if _, ok := root["bootstrap"]; ok {
warnings = append(warnings, "manifest.bootstrap is deprecated; rename to config: [...] at the root level")
}
if configValue, ok := root["config"]; ok {Without this, any existing package author who hasn't seen the rename will get a silent no-op — their bootstrap steps simply won't run.
@copilot please address this.
There was a problem hiding this comment.
The old bootstrap: key is not silently ignored — the schema has additionalProperties: false with no bootstrap property, so it's rejected at schema validation time with an explicit error ("Unknown property: bootstrap"). The new rejects_old_bootstrap_key_with_schema_error test in add_package_manifest_test.go documents and verifies this behavior.
|
@copilot please run the
|
- Show `☐` for require-owner-type in printBootstrapConfigTODO (cannot verify owner type without querying the repo at install time) - Gate interactive bootstrap execution in add-wizard on hasWriteAccess; fall back to printBootstrapConfigTODO when write access is absent so users without permissions see the manual checklist instead of hitting a permission error - Thread UseCopilotRequests through bootstrapProfileRunConfig and skip copilot-auth actions when org-billing is selected (matching the behavior of permissions.copilot-requests: write injection) - Add ResolveWorkflows tests covering BootstrapProfile propagation: single package populates the profile, multiple packages warn and suppress execution 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>
Done. Addressed all four unresolved review threads in commit
Local |
|
|
- Add empty repo validation in executeBootstrapConfigForAdd to prevent confusing API errors when --repo flag is omitted - Change printBootstrapConfigTODO to accept io.Writer for testability; add_command passes cmd.ErrOrStderr(), add-wizard passes os.Stderr - Fix lint: replace fmt.Sprintf return with fmt.Appendf (modernize) - Add test: rejects old bootstrap key with clear schema error - Add test: TestPrintBootstrapConfigTODO verifying output goes to writer Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Fixed all outstanding review items in commit
Local |
🤖 PR Triage
Score breakdown: Impact 30/50 · Urgency 15/30 · Quality 15/20 Rationale: Merges bootstrap config into
|
|
@copilot simplify bootstrap terminology - use less word, pick something more obvious that aligns with the frontmatter |
Bootstraps
bootstrap.configsupport intogh aw add(TODO list) andgh aw add-wizard(interactive), unifying the three commands around shared config execution logic.Rename:
bootstrap.actions→bootstrap.configSchema: strict
anyOfper action typeadditionalProperties: truewith 6 typed variants (require-owner-type,repo-variable,repo-secret,github-app,copilot-auth,handoff), each withadditionalProperties: falseadd— non-interactive TODO outputResolveWorkflowsnow populatesBootstrapProfileonResolvedWorkflowsAddResolvedWorkflows, prints a checklist of manual steps frombootstrap.config:add-wizard— interactive bootstrap after PR creationexecuteBootstrapConfigForAddwhich delegates to the existingexecuteBootstrapProfilerunnerShared logic (
bootstrap_config.go)printBootstrapConfigTODOandexecuteBootstrapConfigForAddextracted into a new shared file, both no-ops when profile is nilbootstrap,add,add-wizard) now converge onexecuteBootstrapProfile