Prevent workspace wipe during local-actions checkout in generated workflows#42611
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
Updates the workflow compiler’s generated actions/checkout steps for local-action bootstrapping to avoid wiping the workspace state (notably .git) during “Checkout actions folder” operations, addressing failures in safe_outputs where post-step git commands run.
Changes:
- Emit
with: clean: falsefor “Checkout actions folder” in dev mode and script mode generation. - Emit
with: clean: falsefor the “Restore actions folder” generation. - Update compiler assertions and wasm golden fixtures to reflect the new emitted YAML.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_step_generation.go | Adds clean: false to generated checkout/restore steps for local actions. |
| pkg/workflow/compiler_custom_actions_test.go | Extends dev-mode contract test to require clean: false. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Golden update: dev-mode checkout actions folder includes clean: false. |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Workflow lock metadata updated, but checkout steps in the lock content still appear to omit clean: false. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Low
| @@ -1,4 +1,4 @@ | |||
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"0a7a1cc9a9d3e316b42d163c8f7452df59e96a2dd1056a07170aca6d955bd9ca","body_hash":"4b304b423080e4bab9d72281c704a8436fe09fa92cf666fe179d4681a922d405","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65","copilot-sdk":"1.0.4"}} | |||
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"0a7a1cc9a9d3e316b42d163c8f7452df59e96a2dd1056a07170aca6d955bd9ca","body_hash":"3741a05e0a954d005c6a5d50b7567b198260ad61d940646d75c20b3b923c1606","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65","copilot-sdk":"1.0.4"}} | |||
| " actions\n", | ||
| " path: /tmp/gh-aw/actions-source\n", | ||
| " fetch-depth: 1\n", | ||
| " clean: false\n", |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot The following review comments need attention:
Once you have addressed these comments, please run the
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
What was changed: A single assertion was added to the existing test verifying that
Go: 1 ( Notes: The other two Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![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 /diagnosing-bugs and /tdd — one observation on test coverage completeness, otherwise a solid targeted fix.
📋 Key Themes & Highlights
Key Themes
- Test coverage asymmetry:
clean: falseis correctly added to all three code paths (dev, script, restore), but only dev mode gets a new unit-test assertion. Script mode and the restore step rely solely on golden fixture diffing for regression protection.
Positive Highlights
- ✅ Root cause correctly identified and addressed in all three code paths
- ✅ Dev-mode regression test extended with the
clean: falseassertion - ✅ All nine golden fixture files regenerated consistently
- ✅ PR description clearly traces the failure (
Run 28486539819) to the root cause
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45.3 AIC · ⌖ 9.81 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| if !strings.Contains(combined, "clean: false") { | ||
| t.Error("Dev mode Checkout actions folder should include 'clean: false' to avoid workspace cleanup removing .git") | ||
| } |
There was a problem hiding this comment.
[/tdd] The new clean: false assertion only covers dev mode — script mode and the restore step have no equivalent unit test.
💡 Suggested additions
TestScriptActionModeCompilation checks many checkout properties (repository, path, fetch-depth, ref) but not clean: false. Add it near line 279:
if !strings.Contains(lockStr, "clean: false") {
t.Error("Script mode Checkout actions folder should include 'clean: false' to avoid workspace cleanup removing .git")
}generateRestoreActionsSetupStep was also changed but has no unit test. A small dedicated test closes the gap:
func TestRestoreActionsSetupStepHasCleanFalse(t *testing.T) {
compiler := NewCompiler(WithVersion("dev"))
compiler.SetActionMode(ActionModeDev)
step := compiler.generateRestoreActionsSetupStep()
if !strings.Contains(step, "clean: false") {
t.Error("Restore actions folder step should include 'clean: false' to avoid workspace wipe")
}
}Without these, regressions in script-mode or restore-step branches would only be caught by golden fixture diffing.
@copilot please address this.
There was a problem hiding this comment.
Review: Prevent workspace wipe during local-actions checkout
The root cause is correctly identified and the fix is sound: adding clean: false to all three checkout/restore paths prevents actions/checkout from wiping the workspace .git state before local-action and post-step git commands run.
What looks good:
clean: falseis applied consistently across all three generation paths (script mode, dev mode, and the restore step)- Dev-mode test updated with a direct assertion
- All golden fixtures regenerated consistently
pr-code-quality-reviewer.lock.ymlhash update is expected and correct
Non-blocking suggestions (2 inline comments):
TestScriptActionModeCompilationis missing aclean: falseassertion for the script-mode checkout pathgenerateRestoreActionsSetupStephas no dedicated unit test; the golden fixtures provide indirect coverage but a direct assertion would make the invariant explicit
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 49.9 AIC · ⌖ 6.78 AIC · ⊞ 4.8K
| } | ||
| if !strings.Contains(combined, "clean: false") { | ||
| t.Error("Dev mode Checkout actions folder should include 'clean: false' to avoid workspace cleanup removing .git") | ||
| } |
There was a problem hiding this comment.
The new assertion covers the dev-mode path, but TestScriptActionModeCompilation (which already asserts repository:, path:, fetch-depth: 1, ref:, etc.) does not assert clean: false for the script-mode checkout. The script-mode code path also received the fix, so a parallel assertion is needed:
if !strings.Contains(lockStr, "clean: false") {
t.Error("Script mode Checkout actions folder should include clean: false to avoid workspace cleanup removing .git")
}@copilot please address this.
| step.WriteString(" sparse-checkout: |\n") | ||
| step.WriteString(" actions/setup\n") | ||
| step.WriteString(" sparse-checkout-cone-mode: true\n") | ||
| step.WriteString(" clean: false\n") |
There was a problem hiding this comment.
generateRestoreActionsSetupStep has no dedicated unit tests, so the new clean: false line is only covered indirectly via golden fixtures. A direct test analogous to TestCheckoutActionsFolderDevModeHasRepository would guard this invariant more explicitly. For example:
func TestRestoreActionsSetupStepHasCleanFalse(t *testing.T) {
compiler := NewCompiler(WithVersion("v1.0.0"))
step := compiler.generateRestoreActionsSetupStep()
if !strings.Contains(step, "clean: false") {
t.Error("Restore actions folder step should include clean: false to avoid workspace cleanup removing .git")
}
}@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations on test coverage
The core fix is correct — adding clean: false to all three checkout code paths addresses the root cause (actions/checkout's default git clean -fdx wiping .git state). The golden-file updates are consistent across all 8 affected fixtures.
📋 What was reviewed
compiler_yaml_step_generation.go: 3clean: falseadditions (script, dev, restore paths) — all correctly placed beforepersist-credentials: falsecompiler_custom_actions_test.go: dev-mode assertion added — correct- 9 golden files: all consistently updated — correct
.github/workflows/pr-code-quality-reviewer.lock.yml: metadata hash updated but workflow YAML steps inside still lackclean: false(flagged separately in existing review thread)
Two test coverage gaps noted in inline comments: (1) script-mode assertion is missing from TestScriptActionModeCompilation, and (2) generateRestoreActionsSetupStep has no test at all despite being called from 5 production sites. These don't block correctness today but leave regression holes.
🔎 Code quality review by PR Code Quality Reviewer · 151.1 AIC · ⌖ 7.38 AIC · ⊞ 1.6K
Comment /review to run again
| } | ||
| if !strings.Contains(combined, "clean: false") { | ||
| t.Error("Dev mode Checkout actions folder should include 'clean: false' to avoid workspace cleanup removing .git") | ||
| } |
There was a problem hiding this comment.
Script-mode checkout has no clean: false assertion, leaving a regression gap.
💡 Details
The PR adds clean: false to both dev-mode and script-mode checkout generation (generateCheckoutActionsFolder), and correctly adds a test assertion for dev mode (TestCheckoutActionsFolderDevModeHasRepository). However, TestScriptActionModeCompilation (line 189) tests 10 other properties of the script-mode checkout step but has no check for clean: false. If the script-mode branch of generateCheckoutActionsFolder silently regresses (e.g., the clean: false line is accidentally removed), no test will catch it.
Suggested fix — add to TestScriptActionModeCompilation after the existing assertions:
// 11. Checkout should include clean: false to avoid removing .git state
if !strings.Contains(lockStr, "clean: false") {
t.Error("Expected 'clean: false' in checkout step for script mode to prevent workspace wipe")
}This mirrors the dev-mode assertion added in this same PR at line 360.
| step.WriteString(" sparse-checkout: |\n") | ||
| step.WriteString(" actions/setup\n") | ||
| step.WriteString(" sparse-checkout-cone-mode: true\n") | ||
| step.WriteString(" clean: false\n") |
There was a problem hiding this comment.
generateRestoreActionsSetupStep has no test asserting clean: false, and the function is exercised in 5 production call sites.
💡 Details
The fix adds clean: false to this function at line 106, but there is no unit test for generateRestoreActionsSetupStep anywhere in the test suite, and the golden files under testdata/ do not contain a "Restore actions folder" step — so the golden tests don't cover it either.
The function is called from:
publish_assets.gocompiler_safe_outputs_job.gorepo_memory.gocompiler_experiments.gocompiler_yaml_main_job.go
If clean: false is removed from this function, no automated check will fail. The bug that prompted this PR could silently resurface in any of those 5 code paths.
Suggested fix — add a dedicated test (or extend an existing integration test that exercises one of the callers) that asserts the restore step output contains clean: false:
func TestRestoreActionsSetupStepHasCleanFalse(t *testing.T) {
compiler := NewCompiler(WithVersion("1.0.0"))
step := compiler.generateRestoreActionsSetupStep()
if !strings.Contains(step, "clean: false") {
t.Error("Restore actions setup step should include 'clean: false' to avoid workspace wipe")
}
}| " repository: github/gh-aw\n", | ||
| " sparse-checkout: |\n", | ||
| " actions\n", | ||
| " clean: false\n", |
There was a problem hiding this comment.
Fix is incomplete — at least 10 hardcoded checkout steps in other files bypass generateCheckoutActionsFolder() and still lack clean: false.
💡 Details
maintenance_workflow_yaml.go contains 8 direct yaml.WriteString(" - name: Checkout actions folder\n") calls (lines ~162, 228, 322, 372, 679, 778, 827, 937) that go straight from sparse-checkout: to persist-credentials: false — no clean: false. These all bypass generateCheckoutActionsFolder() entirely.
side_repo_maintenance.go has 2 more identical hardcoded blocks (lines ~308, 376) in dev/script mode paths, also without clean: false.
Confirmed via grep — none of them emit clean: false:
$ grep -A15 Checkout
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Both issues have been addressed:
Committed in the latest push. |
… unit test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Run
28486539819failed insafe_outputsbecause the generatedCheckout actions folderstep used default checkout cleaning, which removed the workspace.gitstate and broke subsequent local-action/post-step git commands (fatal: --local can only be used inside a git repository).Root cause alignment
Compiler changes
clean: falseto dev-modeCheckout actions foldergeneration.clean: falseto script-modeCheckout actions foldergeneration.clean: falsetoRestore actions foldergeneration.Contract updates
clean: falsein the generated dev-mode checkout step.