Make generated Copilot setup workflows Zizmor-compliant - #55641
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| return fmt.Sprintf(copilotSetupActionTemplate, checkoutRef, actionRepo, actionRef, version) | ||
| } | ||
|
|
||
| // Default (dev/script mode): try to resolve the main branch to a pinned SHA so the |
There was a problem hiding this comment.
@copilot use GitHub API to resolve the repository default branch.
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. ✅
|
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check.
|
|
✅ 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 checkout pinning is headed in the right direction, but the upgrader still leaves an insecure configuration behind when an existing workflow already sets persist-credentials: true.
Blocking theme
- The upgrade path rewrites
actions/checkoutto the pinned ref but does not forcepersist-credentials: false, so previously customized workflows can keep persisting credentials aftergh aw upgrade.
I did not find other changed-line issues severe enough to block beyond that.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 5.53 AIC · ⌖ 6.93 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| if i+1 < len(lines) && strings.HasPrefix(lines[i+1], indent+"with:") { | ||
| blockEnd := checkoutWithBlockEnd(lines, i+2, indent) | ||
| if !checkoutWithBlockHasPersistCredentials(lines, i+2, blockEnd) { |
There was a problem hiding this comment.
This upgrader treats any existing persist-credentials: entry as "good enough", so a workflow that explicitly sets persist-credentials: true will keep persisting the GitHub token after upgrade and still violate the hardening this change is supposed to enforce.
💡 Why this needs to be fixed
checkoutWithBlockHasPersistCredentials only checks for the key name, not the value. In the with: branch that means we skip insertion whenever the file already contains persist-credentials: true, and we also rewrite the checkout action to the pinned ref, which makes the upgrade look successful while silently preserving the insecure setting.
Please normalize the checkout block to persist-credentials: false during upgrade instead of only adding the key when it is missing. For example, replace an existing persist-credentials: line in the checkout with: block, and only insert the setting when the key is absent.
if line is persist-credentials:
rewrite it to false
else if key absent:
insert persist-credentials: falseWithout that, previously customized setup workflows remain credential-persisting even after gh aw upgrade.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Pins generated checkout actions and hardens Copilot setup workflows for Zizmor compliance.
Changes:
- Adds permissions, concurrency, job naming, and credential hardening.
- Upgrades existing checkout references.
- Adds generation and upgrade regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/copilot_setup.go |
Generates and upgrades hardened setup workflows. |
pkg/cli/copilot_setup_test.go |
Tests checkout pinning and YAML upgrades. |
Review details
Suppressed comments (2)
pkg/cli/copilot_setup.go:671
- Checking only for the key treats
persist-credentials: true(or a dynamic expression) as already hardened, so upgrades can leave the checkout credentials persisted and retain the Zizmor finding. Require a literalfalse; replace other values or report that the workflow could not be hardened.
}
return len(lines)
}
func checkoutNextNonBlankLineInBlock(lines []string, start int, usesIndent string) bool {
pkg/cli/copilot_setup.go:639
- The helper reports
changed=truefor every matched checkout line, even when the reconstructed result is byte-for-byte identical. Since the caller trusts this flag, every repeated upgrade reports success and rewrites an already-current file instead of taking the no-op path.
i = blockEnd - 1
continue
}
lines = slices.Insert(lines, i+1,
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| if i+1 < len(lines) && strings.HasPrefix(lines[i+1], indent+"with:") { | ||
| blockEnd := checkoutWithBlockEnd(lines, i+2, indent) | ||
| if !checkoutWithBlockHasPersistCredentials(lines, i+2, blockEnd) { | ||
| insert := indent + " persist-credentials: false" | ||
| lines = slices.Insert(lines, i+2, insert) |
| hadTrailingNewline := strings.HasSuffix(contentStr, "\n") | ||
| lines := strings.Split(strings.TrimSuffix(contentStr, "\n"), "\n") |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a few correctness and test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Unguarded empty pin:
pinCheckoutUsesInContentdoesn't guard againstlatestCheckoutActionRef()returning"", which would silently produce malformed YAML (uses:). - Testability:
pinCheckoutUsesInContentbakes in the global pin lookup; acceptingcheckoutRefas a parameter would make unit tests independent of the embedded pins map. - Missing edge-case tests: no coverage for
persist-credentials: truesurviving upgrade unchanged, nor for CRLF-encoded files.
Positive Highlights
- ✅ Excellent extraction of the large
generateCopilotSetupStepsYAMLinto named template constants — much easier to read and audit. - ✅ The refactor into
handleExistingCopilotSetupSteps/upgradeExistingCopilotSetupStepsis a clean decomposition of a previously deeply-nested function. - ✅ Good blank-line-in-with-block tests (
checkoutWithBlockEndedge cases) — these are exactly the fiddly cases that break regex-only approaches. - ✅
copilotSetupStepsYAMLnow reusescopilotSetupScriptTemplateso the static test fixture stays in sync with the runtime template automatically.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 49.1 AIC · ⌖ 10.4 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/copilot_setup.go:390
[/diagnosing-bugs] If latestCheckoutActionRef() returns "" (e.g. the pin is absent from the embedded map), the function silently writes uses: — syntactically valid YAML that breaks at runtime with no warning.
<details>
<summary>💡 Suggested fix</summary>
Add an early return at the top of pinCheckoutUsesInContent:
checkoutRef := latestCheckoutActionRef()
if checkoutRef == "" {
return content, false
}And add a test asserting no-op behavior when the resolved pin is …
pkg/cli/copilot_setup.go:376
[/codebase-design] latestCheckoutActionRef() is called twice per pinCheckoutUsesInContent invocation (once at call site in upgradeSetupCliVersionInContent, once inside the loop). Since actionpins.ResolveLatestActionPin is deterministic, this is harmless today but makes the contract ambiguous. Passing checkoutRef as a parameter would make the function easier to test in isolation (inject any ref string without touching global state).
<details>
<summary>💡 Suggested signature</summa…
pkg/cli/copilot_setup.go:421
[/tdd] checkoutWithBlockEnd treats a blank line as a potential block terminator but delegates the look-ahead to checkoutNextNonBlankLineInBlock. The two functions are tightly coupled and neither has a dedicated unit test for the case where the blank line is the last line of the file (i.e. start >= len(lines) in checkoutNextNonBlankLineInBlock). The existing tests use blank lines mid-block but not at EOF — a blank trailing line currently returns false, which correctly terminates …
pkg/cli/copilot_setup.go:338
[/diagnosing-bugs] checkoutUsesLinePattern does not handle \r in the uses: value itself (only \r? at line end). A Windows CRLF file with uses: actions/checkout@v4\r will match, but if the SHA ref returned by latestCheckoutActionRef() contains no \r, the replacement produces a mixed-CRLF line that subtly corrupts YAML on Windows runners. The hadTrailingNewline guard only checks the final newline, not per-line endings.
@copilot please address this.
pkg/cli/copilot_setup_test.go:567
[/tdd] The TestPinCheckoutUsesInContent suite doesn't cover the case where persist-credentials: true is already present — the function should leave it untouched (not inject a duplicate false). checkoutWithBlockHasPersistCredentials guards against the inject, but there's no regression test validating that persist-credentials: true survives the upgrade unchanged.
<details>
<summary>💡 Suggested test skeleton</summary>
t.Run("does not overwrite existing persist-credentials", …
</details>
There was a problem hiding this comment.
Zizmor Compliance Review
This PR correctly adds:
- Pinned SHA for
actions/checkoutviaactionpins persist-credentials: falsein new and upgraded workflows- Workflow-level
permissions: contents: read concurrencyblock withcancel-in-progress: true
Two blocking issues were found:
1. Empty checkout ref produces invalid YAML silently
latestCheckoutActionRef() does not guard against actionpins.ResolveLatestActionPin returning "". An empty return produces a uses: line that is syntactically invalid YAML. A panic-on-empty guard (or explicit error return) would surface this at build/test time.
2. pinCheckoutUsesInContent does not fix persist-credentials: true
When upgrading a file that already has persist-credentials: true, the function detects the key, skips insertion, and leaves the true value in place — defeating the Zizmor compliance goal for that case. The function needs to replace the value, not just skip if any key is found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 45.9 AIC · ⌖ 9.31 AIC · ⊞ 6.2K
| func latestCheckoutActionRef() string { | ||
| return actionpins.ResolveLatestActionPin("actions/checkout", nil) | ||
| } | ||
|
|
There was a problem hiding this comment.
[harden] Silent empty checkout ref can produce invalid YAML
latestCheckoutActionRef() returns actionpins.ResolveLatestActionPin("actions/checkout", nil) with no guard on an empty result. If pin resolution returns "" (e.g., no embedded data found), the generated template will contain uses: — an invalid YAML value. GitHub Actions does not reject it at generation time.
Consider adding a validation guard before writing:
func latestCheckoutActionRef() string {
ref := actionpins.ResolveLatestActionPin("actions/checkout", nil)
if ref == "" {
panic("no embedded pin for actions/checkout — rebuild required")
}
return ref
}Or have generateCopilotSetupStepsYAML / validateCopilotSetupStepsContent catch an empty ref and return an error before persisting.
@copilot please address this.
| fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr(fmt.Sprintf("Skipping %s (already has gh-aw extension install step)", setupStepsPath))) | ||
| } | ||
| return nil | ||
| upgraded, updatedContent, err := upgradeSetupCliVersionInContent(ctx, content, actionMode, version, resolver) |
There was a problem hiding this comment.
[harden] pinCheckoutUsesInContent is not idempotent when persist-credentials already exists but is true
checkoutWithBlockHasPersistCredentials returns true whenever any persist-credentials: key is found in the block — including persist-credentials: true. This means if an existing workflow already has persist-credentials: true, the upgrade will:
- Replace the
actions/checkoutref with the pinned SHA ✓ - Skip inserting
persist-credentials: falsebecause it sees the existing key ✓ - Leave
persist-credentials: truein place ✗
The Zizmor compliance goal is defeated: the credential is still persisted. The function should either replace the existing persist-credentials value, or at minimum warn when the existing value is true.
@copilot please address this.
There was a problem hiding this comment.
pkg/cli/copilot_setup.go:L412-L468: yagni: extra handleExistingCopilotSetupSteps/upgradeExistingCopilotSetupSteps wrappers for one branch. Keep the existing-file logic inline until a second caller exists.
pkg/cli/copilot_setup.go:L597-L675: shrink: line-by-line YAML mutation with four tiny helpers. Replace with one small regex-based replacement for the checkout block.
net: -24 lines possible.
Generated by ✂️ Ponytail Reviewer for #55641 · codex · mai10 · 9.92 AIC · ⌖ 2.11 AIC · ⊞ 16.7K
Comment /ponytail to run again
| hasLegacyInstall := strings.Contains(contentStr, "install-gh-aw.sh") || | ||
| (strings.Contains(contentStr, "Install gh-aw extension") && strings.Contains(contentStr, "curl -fsSL")) | ||
| hasActionInstall := strings.Contains(contentStr, "actions/setup-cli") | ||
| func handleExistingCopilotSetupSteps(ctx context.Context, verbose bool, actionMode workflow.ActionMode, version string, resolver workflow.SHAResolver, setupStepsPath string, upgradeVersion bool) error { |
There was a problem hiding this comment.
pkg/cli/copilot_setup.go:L412-L468: yagni: extra handleExistingCopilotSetupSteps/upgradeExistingCopilotSetupSteps wrappers for one branch. Keep the existing-file logic inline until a second caller exists.
| return true, updated, nil | ||
| } | ||
|
|
||
| func pinCheckoutUsesInContent(content []byte) ([]byte, bool) { |
There was a problem hiding this comment.
pkg/cli/copilot_setup.go:L597-L675: shrink: line-by-line YAML mutation with four tiny helpers. Replace with one small regex-based replacement for the checkout block.
|
Warning The 🏗️ Architecture Decision Record RequiredThis PR modifies 345+ lines in No ADR was found in the PR body, the PR branch, or a linked issue. Draft ADR committed to this branchA draft ADR has been generated and committed to this PR branch: 📄 Inferred decision: Use the existing Next steps for the PR author
If the draft is inaccurate or the ADR should live elsewhere (e.g., in a linked issue), update it and re-run the gate. Evidence used
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Generated
copilot-setup-steps.ymlworkflows still emittedactions/checkout@v6, leaving consumers with Zizmor findings after upgrading. The setup workflow also needed checkout hardening for clean Zizmor output.actions/checkoutthrough the embedded action pins mapping.actions/checkout@v6with the latest SHA-pinned ref.Harden generated setup workflows
copilot-setup-stepsjob.Upgrade existing setup workflows
persist-credentials: falsewhile preserving surrounding YAML formatting and line endings.Add regression coverage
with:blocks and blank-line edge cases.