Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55388 does not have the 'implementation' label and has only 43 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
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
This change makes safe-output PR checkouts root-only by default, but the compiler never proves the resulting patch only touches root files. That means normal PR patches against nested paths can fail during apply/push instead of creating the PR reliably.
Blocking themes
- The new
minimalDefaultCheckoutgate is driven by workflow shape, not by the patch's actual file set. create_pull_requestis explicitly a path that can modify arbitrary repository files, so shrinking the checkout to.is unsafe unless the handler expands the sparse set before patch application.- The added tests only assert YAML emission and opt-out cases; they do not cover a nested-file patch under the new default.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 30.1 AIC · ⌖ 6.85 AIC · ⊞ 7K
Comment /review to run again
| len(data.SafeOutputs.Steps) == 0 && | ||
| len(data.SafeOutputs.Actions) == 0 && | ||
| len(data.SafeOutputs.Scripts) == 0 { | ||
| checkoutMgr.SetMinimalDefaultCheckout(true) |
There was a problem hiding this comment.
This optimization silently stops checking out most tracked files, so any generated patch that touches pkg/, .github/workflows/, or other non-root paths can fail during apply/push with missing-path errors. PR creation is exactly the path that often edits nested files, so this turns a performance tweak into a correctness regression.
💡 Why this blocks merge
buildSharedPRCheckoutSteps enables sparse-checkout: . whenever there is no explicit root checkout override and no custom safe-output extensions. That condition says nothing about what files the generated patch will modify. create_pull_request commonly edits nested files, and those paths will not exist in a root-only sparse checkout.
Please either keep the full checkout for PR-producing handlers, or prove the handler materializes every patched path before apply. At minimum this needs a regression test covering a patch that modifies something like pkg/workflow/x.go or .github/workflows/foo.md under the new minimal checkout path.
There was a problem hiding this comment.
Addressed in 1464b2f with regression coverage. git_patch_integration.test.cjs adds two real-git tests over a root-only sparse clone (sparse-checkout set --cone .):
- nested-path patch application, where
pkg/deep/nested.txtis tracked but not materialized beforegit am --3way, asserting both the modified and newly created nested files land correctly; - nested add/add conflict recovery, asserting plain
git add -- <file>fails outside the sparse cone whilegit add --sparseresolves it and yields the patch version.
The handlers were updated to stage with git add --sparse, and the full-checkout guard is covered per case in TestBuildSharedPRCheckoutSteps (custom safe-output steps, actions, scripts). go test ./pkg/workflow and the three vitest suites (299 tests) pass on the current head.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — leaving two suggestions on test coverage gaps; no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Test granularity: the new
"custom safe-output extension keeps full default checkout"case validates the combined condition but not each individual arm (Stepsalone,Actionsalone,Scriptsalone). A regression in any single arm would pass undetected. - Explicit override path: no test directly validates that a configured root-checkout override (
GetDefaultCheckoutOverride() != nil) suppresses sparse checkout.
Positive Highlights
- ✅ Logic placement is correct: the guard in
buildSharedPRCheckoutStepsis the single right place to set this, keeping the decision out ofGenerateDefaultCheckoutStep. - ✅ The
override == nilcheck inGenerateDefaultCheckoutStepcorrectly prevents sparse checkout from being injected when an explicit override already controls the checkout parameters. - ✅ Backward compatibility is well-handled: the three extension types (Steps / Actions / Scripts) are checked before enabling minimisation, and the existing
fetch-depth: 0test now assertscheckNotContains: ["sparse-checkout: ."]. - ✅ Lock file regeneration across 63 workflows is consistent — all add exactly one line.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 53.8 AIC · ⌖ 9.98 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_safe_outputs_steps_test.go:789
[/tdd] The new test bundles all three bypass conditions (Steps + Actions + Scripts) into one case, so it doesn't verify that each condition alone is sufficient to suppress sparse checkout. A single combined config silently masks any regression where, say, only Scripts is non-empty.
<details>
<summary>💡 Suggested approach</summary>
Split into at least three cases:
{
name: "custom step alone keeps full checkout",
safeOutputs: &SafeOutputsConfig{
CreatePullRequest…
</details>
<details><summary>pkg/workflow/compiler_safe_outputs_steps.go:50</summary>
**[/tdd]** Missing test: the guard checks `GetDefaultCheckoutOverride() == nil` (explicit root checkout keeps full checkout), but there is no test case for this branch — only the `fetch-depth: 0` explicit-checkout case exercises it indirectly. A direct test where an explicit root checkout override is configured would make the invariant explicit and catch regressions if the override detection logic changes.
<details>
<summary>💡 Suggested test case</summary>
Add a case like:
```go
{
name:…
</details>There was a problem hiding this comment.
Pull request overview
Optimizes implicit safe-output PR checkouts by materializing only repository-root files while retaining compatibility opt-outs.
Changes:
- Adds a root-only sparse-checkout mode for implicit checkouts.
- Preserves full checkouts for explicit configuration and custom extensions.
- Updates tests and regenerated workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_safe_outputs_steps.go |
Enables minimal safe-output checkouts. |
pkg/workflow/compiler_safe_outputs_steps_test.go |
Tests sparse checkout and opt-outs. |
pkg/workflow/checkout_step_generator.go |
Emits the sparse-checkout input. |
pkg/workflow/checkout_manager.go |
Stores minimal-checkout state. |
.github/workflows/weekly-safe-outputs-spec-review.lock.yml |
Regenerates checkout configuration. |
.github/workflows/weekly-editors-health-check.lock.yml |
Regenerates checkout configuration. |
.github/workflows/weekly-blog-post-writer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/update-astro.lock.yml |
Regenerates checkout configuration. |
.github/workflows/unbloat-docs.lock.yml |
Regenerates checkout configuration. |
.github/workflows/ubuntu-image-analyzer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/tidy.lock.yml |
Regenerates checkout configuration. |
.github/workflows/technical-doc-writer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/squad.lock.yml |
Regenerates checkout configuration. |
.github/workflows/squad-implement-worker.lock.yml |
Regenerates checkout configuration. |
.github/workflows/spec-extractor.lock.yml |
Regenerates checkout configuration. |
.github/workflows/spec-enforcer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/smoke-update-cross-repo-pr.lock.yml |
Regenerates checkout configuration. |
.github/workflows/smoke-project.lock.yml |
Regenerates checkout configuration. |
.github/workflows/smoke-multi-pr.lock.yml |
Regenerates checkout configuration. |
.github/workflows/smoke-create-cross-repo-pr.lock.yml |
Regenerates checkout configuration. |
.github/workflows/ruflo-backed-task.lock.yml |
Regenerates checkout configuration. |
.github/workflows/refiner.lock.yml |
Regenerates checkout configuration. |
.github/workflows/q.lock.yml |
Regenerates checkout configuration. |
.github/workflows/purelock.lock.yml |
Regenerates checkout configuration. |
.github/workflows/necromancer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/mergefest.lock.yml |
Regenerates checkout configuration. |
.github/workflows/linter-miner.lock.yml |
Regenerates checkout configuration. |
.github/workflows/layout-spec-maintainer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/jsweep.lock.yml |
Regenerates checkout configuration. |
.github/workflows/instructions-janitor.lock.yml |
Regenerates checkout configuration. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Regenerates checkout configuration. |
.github/workflows/go-logger.lock.yml |
Regenerates checkout configuration. |
.github/workflows/github-mcp-tools-report.lock.yml |
Regenerates checkout configuration. |
.github/workflows/functional-pragmatist.lock.yml |
Regenerates checkout configuration. |
.github/workflows/evoskill-evolver.lock.yml |
Regenerates checkout configuration. |
.github/workflows/eslint-miner.lock.yml |
Regenerates checkout configuration. |
.github/workflows/dictation-prompt.lock.yml |
Regenerates checkout configuration. |
.github/workflows/developer-docs-consolidator.lock.yml |
Regenerates checkout configuration. |
.github/workflows/design-decision-gate.lock.yml |
Regenerates checkout configuration. |
.github/workflows/dependabot-burner.lock.yml |
Regenerates checkout configuration. |
.github/workflows/dead-code-remover.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-yamllint-fixer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-workflow-updater.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-trajectory-grader-implementer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-safe-output-integrator.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-rendering-scripts-verifier.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-go-test-stubs-aider.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-go-test-parallelizer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-elixir-credo-snippet-audit.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-documentation-diagram.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-doc-updater.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-doc-healer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-compiler-threat-spec-optimizer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-community-attribution.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-code-debt-aider.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-caveman-optimizer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-architecture-diagram.lock.yml |
Regenerates checkout configuration. |
.github/workflows/daily-agent-of-the-day-blog-writer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/craft.lock.yml |
Regenerates checkout configuration. |
.github/workflows/code-simplifier.lock.yml |
Regenerates checkout configuration. |
.github/workflows/code-scanning-fixer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/cloclo.lock.yml |
Regenerates checkout configuration. |
.github/workflows/ci-coach.lock.yml |
Regenerates checkout configuration. |
.github/workflows/chaos-pr-bundle-fuzzer.lock.yml |
Regenerates checkout configuration. |
.github/workflows/changeset.lock.yml |
Regenerates checkout configuration. |
.github/workflows/avenger.lock.yml |
Regenerates checkout configuration. |
Review details
- Files reviewed: 67/67 changed files
- Comments generated: 2
- Review effort level: Balanced
| if checkoutMgr.GetDefaultCheckoutOverride() == nil && | ||
| len(data.SafeOutputs.Steps) == 0 && | ||
| len(data.SafeOutputs.Actions) == 0 && | ||
| len(data.SafeOutputs.Scripts) == 0 { | ||
| checkoutMgr.SetMinimalDefaultCheckout(true) |
There was a problem hiding this comment.
Fixed in 1464b2f (and refined in 689656b). The buildSharedPRCheckoutSteps doc comment now lists the minimal workspace-root checkout as a third safe_outputs-specific difference, describing the root-only sparse default (sparse-checkout: .), the dropped agent-oriented history extras, and the opt-out (agent-identical checkout kept when custom safe-output steps/actions/scripts are declared). The inline comment at the default-checkout call site now says "same generator as the agent job; root-only sparse by default" instead of claiming it is identical.
| len(data.SafeOutputs.Steps) == 0 && | ||
| len(data.SafeOutputs.Actions) == 0 && | ||
| len(data.SafeOutputs.Scripts) == 0 { | ||
| checkoutMgr.SetMinimalDefaultCheckout(true) |
There was a problem hiding this comment.
Fixed in 1464b2f. Both handlers now stage resolutions with git add --sparse -- <file> (create_pull_request.cjs:174, push_to_pull_request_branch.cjs:1215).
Reproduced with real git first: git am --3way itself is fine for nested paths (git materializes the touched paths on demand), but add/add recovery broke exactly as described — plain git add -- <file> refuses index updates outside the sparse cone, leaving the file unmerged and failing git am --continue. git_patch_integration.test.cjs covers nested-file conflicts over a root-only sparse clone, asserting plain git add fails while --sparse resolves and produces the patch version; handler unit expectations were updated too.
|
Please use the
Also refresh the branch as needed, run
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 1464b2f. Patch-application safety under the root-only sparse checkout:
Tests:
|
|
Please use the
Run: https://github.com/github/gh-aw/actions/runs/32750412251
|
No further changes needed — the blocking review is answered by 1464b2f.
Re-verified on the current head: |
|
@dsyme we are doing a full checkout here. something better could be done (there's a script checkout_pr_branch.sh that we could use too) |
|
@copilot look for the checkout_pr_branch . sh script and reuse |
|
@copilot this PR still needs forward progress before maintainer investigation is likely to be productive. Please do one focused pass on the latest maintainer request:
Run: https://github.com/github/gh-aw/actions/runs/32755962919
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Found it — the script is What it does: it never relies on a heavy checkout — it fetches just the one ref it needs ( The full checkout you spotted came from the safe_outputs job replaying the workflow's agent-oriented - name: Checkout repository
uses: actions/checkout@... # v7.0.1
with:
persist-credentials: true
- fetch-depth: 0
token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}
- - name: Fetch additional refs
- run: git ... fetch origin '+refs/pull/*/head:refs/remotes/origin/pull/*/head'The agent job keeps its configured depth and refs. Combined with the root-only sparse default for implicit checkouts, the safe_outputs job now starts from depth-1/root-only state and fetches only the ref it operates on. The agent-identical checkout is still kept when custom safe-output steps/actions/scripts are declared. Covered by new cases in |
Already done in 689656b — no further changes needed on this pass.
Re-verified on the current head: |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two design observations and two test-coverage notes.
📋 Key Themes & Highlights
Key Themes
- Opt-out predicate locality (
compiler_safe_outputs_steps.go): the three-field inline check will silently miss futureSafeOutputsConfigextensions; moving it to a method on the config type would be safer and self-documenting. - Override polarity asymmetry (
checkout_step_generator.go):sparse-checkoutappended only whenoverride == nil, while fetch-depth/fetchRefs suppressed for both; the asymmetry is valid but undocumented and easy to misread. - Sparse clone mode in integration tests (
git_patch_integration.test.cjs):--conemode in the test helper may not exactly mirror the non-conesparse-checkout: .thatactions/checkoutuses — worth a confirming comment. - Handler-level integration coverage: the add/add recovery integration test proves raw git semantics but not the handler wiring; unit mock coverage is present but a shallow handler integration test would add confidence.
Positive Highlights
- ✅ Excellent root cause fix:
git add --sparseprecisely addresses the failure mode at the narrowest possible scope. - ✅ Compatibility safeguards for custom steps/actions/scripts are well-structured and tested with clear test names.
- ✅ The
daily-safeoutputs-git-simulatorlock file cleanup (removing redundantfetch-depth: 0+ fetch step) is a nice simplification. - ✅ Consistent application of the change across all 60+ lock files via recompile — no manual drift.
- ✅ Good inline comments in the
.cjshandlers explaining why--sparseis needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 59.2 AIC · ⌖ 10.4 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_safe_outputs_steps.go:66
[/codebase-design] The opt-out predicate checks Steps, Actions, and Scripts inline. If SafeOutputsConfig gains a new extension field that also requires full history, the minimal checkout will silently activate for those workflows.
<details>
<summary>💡 Suggestion: move the check onto the config type</summary>
// HasCustomSafeOutputExtensions reports whether any custom steps/actions/scripts
// are present that may require the full working tree or agent history.
func (s *SafeO…
</details>
<details><summary>pkg/workflow/checkout_manager.go:201</summary>
**[/codebase-design]** `SetMinimalDefaultCheckout` is gated on `override == nil` in `checkout_step_generator.go`, but `fetchRefs` suppression checks `override != nil && !cm.minimalDefaultCheckout`. The asymmetry means the `sparse-checkout: .` line and the fetch-ref suppression follow different `override` polarity, which makes the combined behaviour hard to reason about.
<details>
<summary>💡 Suggestion: document the polarity contract</summary>
Add a doc comment to `SetMinimalDefaultCheckout` …
</details>
<details><summary>actions/setup/js/git_patch_integration.test.cjs:52</summary>
**[/tdd]** The `createRootOnlySparseClone` helper sets `--cone` mode but the safe-outputs `actions/checkout` step uses `sparse-checkout: .` without cone mode. The two behaviours differ: cone mode restricts to a directory prefix pattern, while non-cone mode uses `.gitignore`-style patterns. The test may pass while the production path silently uses different sparse semantics.
<details>
<summary>💡 Details</summary>
`actions/checkout` with `sparse-checkout: .` uses non-cone (pattern-based) mode …
</details>
<details><summary>actions/setup/js/git_patch_integration.test.cjs:95</summary>
**[/tdd]** The integration test for the sparse + add/add recovery path (line 95–135) directly calls the raw git commands, but does not invoke the actual `tryRecoverGitAmAddAddConflict` JavaScript function. This means the test proves the git semantics are correct but not that the handler wiring (`create_pull_request.cjs` / `push_to_pull_request_branch.cjs`) calls `git add --sparse` in exactly the right place under real execution.
The unit-test assertions in `create_pull_request.test.cjs` and `p…
</details>
|
@copilot PR Sous Chef triage: this PR still has unresolved review feedback. Please address the unresolved review threads, refresh the branch if needed, and then run the Unresolved threads (newest first):
I also attempted a branch refresh for this PR in this run.
|
|
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>
Done in 72143d5 — the remaining gap was the maintainer-facing docs follow-up, so that is now the only change in this pass.
The three earlier review threads were already answered in-thread (docs/comments in 1464b2f + 689656b, Verification: |
Safe-output PR creation materialized the full repository despite needing only enough Git state to apply and push the generated patch. This adds a minimal default checkout while preserving opt-out paths.
Minimal default
Compatibility safeguards
Generated workflows
Run: https://github.com/github/gh-aw/actions/runs/32755962919
Branch refresh requested by PR Sous Chef for run https://github.com/github/gh-aw/actions/runs/32760251859.