Fix add-wizard existing Copilot token flow - #55438
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an explicit choice to reuse or replace an existing Copilot token.
Changes:
- Defaults to reusing the detected token.
- Adds replacement-path tests and a patch changeset.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/engine_secrets.go |
Adds existing-token confirmation flow. |
pkg/cli/engine_secrets_test.go |
Tests reuse and replacement choices. |
.changeset/patch-use-existing-copilot-token-add-wizard.md |
Documents the patch. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
|
@copilot this PR has unresolved review feedback that needs follow-up before merge. Please address these threads, push the fixes, and then run the
Run: https://github.com/github/gh-aw/actions/runs/32748984422
|
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>
Addressed both review threads in 37c58e4: organization secrets are reused only when visible to the target repository, and the release-note wording is corrected. |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The wizard flow still has a correctness hole around local writes, and the generated PR description is overstating secret readiness.
Blocking themes
- The refactored add-wizard path now applies repository initialization side effects even when the user chooses local writes, which breaks the stated separation between PR and local delivery.
- The new PR body collapses repo and org secret availability into one vague “existing secret” state, so reviewers cannot tell what authentication setup actually exists.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 39.2 AIC · ⌖ 8.18 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
pkg/cli/add_command.go:81-102: yagni: AddOptions now carries wizard-specific state (workingTreePrevalidated, showInteractiveProgress, createdByAddWizard, etc.) for one path. Keep that state in a small wizard-specific struct or local values.
pkg/cli/add_workflow_pr.go:199-270: yagni: PR body formatting now has a mini formatter library (workflowSourceMarkdown, workflowTriggerSummary, enabledText, markdownBlock, joinCodeValues) for one output. Inline the few formatting calls and keep the builder as a straight string writer.
net: -22 lines possible.
Generated by ✂️ Ponytail Reviewer for #55438 · codex · mai10 · 14.8 AIC · ⌖ 1.31 AIC · ⊞ 16.7K
Comment /ponytail to run again
There was a problem hiding this comment.
Review: Fix add-wizard existing Copilot token flow
Overall: The PR's core intent — defaulting to the existing COPILOT_GITHUB_TOKEN instead of always re-prompting, filtering org secrets by visibility, and splitting working-tree validation from the AddResolvedWorkflows path — is well-structured and well-tested. Two issues need attention before merge.
Blocking
1. copilotAuthMethodDescription always says "existing" secret (line 317, add_interactive_engine.go)
The PAT option description is hardcoded to "Use the existing COPILOT_GITHUB_TOKEN repository secret" regardless of whether the secret actually exists. When COPILOT_GITHUB_TOKEN is absent, a first-time user sees misleading copy that implies they already have the secret configured. The function should accept a copilotSecretExists bool and vary the text accordingly (see inline comment).
2. addRepositoryInitializationPlan.files is stored but never consumed (line 444, add_command.go)
confirmAddRepositoryInitialization populates plan.files with the set of missing markers discovered during the confirmation step, but applyAddRepositoryInitialization ignores plan.files entirely and re-queries via ensureAddRepositoryInitializedWithDetails. The files field on the exported struct signals a contract that isn't honoured, and the silent re-discover creates a minor TOCTOU window. The field should either be removed or actually used to avoid the second scan (see inline comment).
Non-blocking observations
- The new
organizationSecretAvailablelogic correctly handlesall,private, andselectedvisibility — good hardening of the previous flat-list approach. - The
workingTreePrevalidatedbypass is set tocreatePR, which is safe: local writes don't need the new interactive blocker resolution loop. - The spinner deferral (
defer stopProgress()) inaddWorkflowWithTrackingwill callStopeven on error paths, which is correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 129.7 AIC · ⌖ 9.49 AIC · ⊞ 6.2K
Comments that could not be inline-anchored
pkg/cli/add_interactive_engine.go:317
Hardening — misleading PAT description when secret does not exist
copilotAuthMethodDescription unconditionally produces "Use the existing COPILOT_GITHUB_TOKEN repository secret" regardless of whether existingSecrets actually contains COPILOT_GITHUB_TOKEN. When the secret does not yet exist, the user reads a description that implies an existing secret is available, while they will actually be asked to create and store a new one.
selectCopilotAuthMethod is a receiver method on `*A…
pkg/cli/add_command.go:444
Harden — plan.files is populated but silently ignored in applyAddRepositoryInitialization
confirmAddRepositoryInitialization stores the list of missing support files in addRepositoryInitializationPlan.files, but applyAddRepositoryInitialization discards it and calls ensureAddRepositoryInitializedWithDetails, which re-discovers missing markers from scratch:
func applyAddRepositoryInitialization(plan ...) ([]string, error) {
if !plan.enabled {
return nil, nil
…
</details>
PR Review SummaryApplied 📋 Issues raised (click to expand)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on several correctness and maintainability issues.
📋 Key Themes & Highlights
Key Themes
- Implicit ordering dependency (
isPublicReposet in auth flow, consumed in secrets check) with no guard. - Unbounded retry loop in
checkCleanWorkingDirectoryForPRwith no context cancellation. - Double
addFindGitRoot()call in the same logical operation, risking path-mismatch bugs. - Duplicated three-way auth decision between
buildAddWorkflowPRBodyand engine-selection helpers. - Hardcoded, mismatched URL pair in PR body builder.
- Confirm/apply TOCTOU —
plan.filescomputed during confirmation is discarded and re-derived during apply.
Positive Highlights
- ✅ Excellent improvement replacing the "assume all org secrets are visible" bug with proper visibility-aware filtering.
- ✅ Clean separation of
confirm/applyinit phases — the intent is right, just the apply implementation re-derives instead of reusing. - ✅ The working-tree blocker model (
stagedvsoverlapping) is precise and well-tested. - ✅ Strong test coverage upgrade on
checkExistingSecrets— the new mock-based approach is much more reliable than the previous "don't panic" check. - ✅
addInteractiveRunGHvar injection pattern gives tests clean control overghAPI calls.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 140.7 AIC · ⌖ 10.5 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/add_interactive_secrets.go:76
[/diagnosing-bugs] organizationSecretAvailable reads c.isPublicRepo, but isPublicRepo is set in checkGHAuthStatus (auth flow) which runs before selectAIEngineAndKey → checkExistingSecrets. The ordering is currently correct, but there is no guard — if any future refactor calls checkExistingSecrets before checkGHAuthStatus, private-visibility org secrets will be incorrectly offered on public repos.
<details>
<summary>💡 Suggested guard</summary>
Make the dependency explicit …
pkg/cli/add_interactive_git.go:716
[/diagnosing-bugs] The for {} retry loop in checkCleanWorkingDirectoryForPR has no iteration limit or context-cancellation check on the workingTreeCleaned path. If inspectAddWorkingTree consistently returns blockers (e.g. a background editor auto-saving to a planned path), the user cannot escape without killing the process.
<details>
<summary>💡 Suggested fix</summary>
Check the context on each iteration so Ctrl-C terminates cleanly:
for {
select {
case <-ctx.Done()…
</details>
<details><summary>pkg/cli/add_interactive_git.go:771</summary>
**[/diagnosing-bugs]** `inspectAddWorkingTree` calls `addFindGitRoot()` independently from `plannedAddPaths`, which also calls `addFindGitRoot()`. Two separate calls means the git root could theoretically differ between them (test stubs, CWD changes), leading to path-normalisation mismatches and incorrect blocker classification.
<details>
<summary>💡 Suggested fix</summary>
Compute the git root once in `checkCleanWorkingDirectoryForPR` and pass it to both helpers:
```go
func inspectAddWorkin…
</details>
<details><summary>pkg/cli/add_workflow_pr.go:1782</summary>
**[/codebase-design]** The three-way auth decision (`AddCopilotRequestsPermission` → `addWizardSecretExists` → `addWizardSkipSecret`) is duplicated here and in the engine-selection display helpers. Both sites must be kept in sync when precedence rules change.
<details>
<summary>💡 Suggested refactor</summary>
Extract a `copilotAuthSummary(opts AddOptions) string` helper and call it from both `buildAddWorkflowPRBody` and the engine-selection path, so there is a single source of truth.
</detai…
</details>
<details><summary>pkg/cli/add_workflow_pr.go:1759</summary>
**[/codebase-design]** Two different documentation URLs appear next to each other: `https://github.github.com/gh-aw/` and `https://github.com/github/gh-aw`. If either URL changes both sites must be updated. Consider defining them as package-level constants so they are maintained in one place.
@copilot please address this.
</details>
<details><summary>pkg/cli/add_init.go:440</summary>
**[/diagnosing-bugs]** `applyAddRepositoryInitialization` calls `ensureAddRepositoryInitializedWithDetails` which re-derives the missing markers internally, ignoring the pre-computed `plan.files`. If the working tree changes between `confirmAddRepositoryInitialization` and `applyAddRepositoryInitialization` (e.g. the user creates a file while answering the delivery-method prompt), additional files may be written that were never shown to the user during confirmation.
<details>
<summary>💡 Sugge…
</details>
|
Warning The 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,385 new lines in business logic directories) but did not have a linked Architecture Decision Record (ADR). 📄 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
|
|
Please do one focused follow-up pass:
I also attempted a branch refresh for this PR in this run. Run: https://github.com/github/gh-aw/actions/runs/32768957910
|
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>
Addressed the focused follow-up in a5ccc64 and f84f90a: local writes no longer initialize support files; secret source/visibility and auth descriptions are precise; pagination, initialization tracking, PR rendering, cancellation, and release metadata feedback are covered. |
…b.com/github/gh-aw into fix/add-wizard-existing-copilot-token # Conflicts: # pkg/cli/add_command.go # pkg/cli/add_init.go # pkg/cli/add_interactive_auth.go # pkg/cli/add_interactive_engine.go # pkg/cli/add_interactive_engine_test.go # pkg/cli/add_interactive_git.go # pkg/cli/add_interactive_orchestrator.go # pkg/cli/add_interactive_secrets.go # pkg/cli/add_interactive_secrets_test.go # pkg/cli/add_workflow_pr.go # pkg/cli/add_workflow_pr_test.go # pkg/cli/preconditions.go
Summary
Improve
gh aw add-wizardfrom credential selection through pull request handoff. The wizard now reuses existing Copilot credentials safely, records the choices it makes, keeps interactive output compact and consistently spaced, handles local repository state precisely, and creates a pull request that gives reviewers enough context to move the workflow forward.Authentication and secrets
COPILOT_GITHUB_TOKEN, while retaining an explicit option to replace it.permissions.copilot-requests: write.Wizard flow and output
workflow_dispatchinputs unset and prompt only for required inputs.PromptFormthat centrally owns one leading blank line, reserves inline terminal space to remain clearable after scrolling, and removes completed questions.Repository and pull request safety
Workflow compilation
--gh-aw-refto bothgh aw addandgh aw add-wizard.github/gh-awbranches and tags to immutable commit SHAs before compiling action references.Generated pull request descriptions
Pull requests created by
addoradd-wizardnow include:gh-awversion, with documentation and repository links;YAML block-scalar descriptions are treated as Markdown, preserving paragraphs and
-lists instead of collapsing them into one line.Documentation and tests
--gh-aw-ref, dirty-tree handling, prompt rendering, wizard orchestration, pull request routing, and generated pull request Markdown.Validation
make fmtgo build ./cmd/gh-awpkg/console,pkg/cli, andpkg/workflowtests for the changed behaviorgit diff --checkAt the time of this update, hosted checks report 7 successful, 12 pending, 2 skipped, and no failures. Local
make agent-report-progresscompletes formatting, build, and synchronization checks but has repeatedly stalled after starting the parallel lint, schema, and impacted-test phase in this environment.