compiler: auto-derive and warn on missing path: for cross-repo checkout entries#42257
Conversation
…issing path Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
path: for cross-repo checkout entries
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🔍 PR Triage — §28395315609
Score breakdown: impact 25 + urgency 10 + quality 12
|
|
👋 Nice fix for the missing The diff is clean and well-scoped across three files, the PR body is thorough (bug context, change summary, before/after YAML), and the 225-line test file in
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR updates the gh-aw workflow compiler to prevent empty checkout paths for cross-repository checkout: entries by auto-deriving a default path: value (and emitting a warning) when repository: is static and path: is omitted—avoiding downstream runtime failures in safe-outputs handlers that rely on workspace path lookup.
Changes:
- Wire a new compiler validation pass to auto-derive missing
path:values for static cross-repo checkout entries and emit a deprecation warning. - Add the new
validateCrossRepoCheckoutPathsimplementation (skipping${{ ... }}repository expressions). - Add unit + light end-to-end validation tests covering derivation, warning counts, and manifest/checkout-step emission.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_validators.go | Wires the new cross-repo checkout path validation into the core tool validation flow. |
| pkg/workflow/checkout_path_validation.go | Implements auto-derivation + warning emission for missing path: on static cross-repo checkouts. |
| pkg/workflow/checkout_path_validation_test.go | Adds tests for derivation/warnings and verifies derived paths flow into manifest + checkout steps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Low
| checkoutPathValidationLog.Printf("auto-deriving path %q for cross-repo checkout %q", repoName, cfg.Repository) | ||
| cfg.Path = repoName | ||
|
|
| for _, cfg := range checkoutConfigs { | ||
| if cfg == nil || cfg.Repository == "" || cfg.Path != "" { | ||
| continue | ||
| } | ||
|
|
||
| // Dynamic expression: skip — cannot determine at compile time whether it | ||
| // targets a different repository (e.g. ${{ github.repository }} is the | ||
| // same repo checked out at a specific ref, which is a valid pattern for | ||
| // pull_request_target and should not trigger a warning). | ||
| if strings.Contains(cfg.Repository, "${{") { | ||
| continue | ||
| } | ||
|
|
||
| checkoutPathValidationLog.Printf("cross-repo checkout %q has no explicit path", cfg.Repository) | ||
|
|
||
| // Static repository slug: auto-derive path from the repository-name portion. | ||
| repoName := cfg.Repository | ||
| if slash := strings.LastIndex(cfg.Repository, "/"); slash >= 0 { | ||
| repoName = cfg.Repository[slash+1:] | ||
| } |
| t.Run("warning message for static repo contains suggested path", func(t *testing.T) { | ||
| c := NewCompiler() | ||
| // Capture stderr by checking warning count and path derivation; | ||
| // the actual message text is validated by the path value and warning count. | ||
| configs := []*CheckoutConfig{ |
…paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (292 new lines in 📄 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
|
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (18 tests analyzed)
Go: 1 (
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: Coding-guideline violation detected. pkg/workflow/checkout_path_validation_test.go is missing the required //go:build !integration build tag on line 1. All peer test files in pkg/workflow/ include this tag. Please review the flagged files in the comment above.
There was a problem hiding this comment.
Review: compiler: auto-derive and warn on missing path: for cross-repo checkout entries
Good fix for the GH_AW_CHECKOUT_PATH_N: "" runtime failure — the approach (auto-derive + deprecation warning + mutation before code generation) is sound, and the test coverage is thorough for the happy paths.
One blocking correctness issue plus two non-blocking suggestions:
Blocking
checkout_path_validation.go:46-51— A trailing-slash slug like"owner/"producesrepoName = ""afterstrings.LastIndex, which then setscfg.Path = ""— the exact empty-path bug being fixed. A guardif repoName == "" { continue }is needed after the split.
Non-blocking
checkout_path_validation.go:28— Thevalidate*naming convention in this codebase implies read-only checks; the mutation is intentional but surprising. A brief doc note (or rename tofixupCrossRepoCheckoutPaths) would make the side-effect explicit.checkout_path_validation_test.go:249— TheTestWarnCrossRepoPath_RepoNameExtractiontable has no entry for"owner/"(trailing slash). Adding that case would lock in the guard and prevent regression.
What's correct ✅
- Dynamic
${{expressions skipped correctly — covers${{ github.repository }}(pull_request_target trusted checkout). - Mutation happens before
validatePushToPullRequestBranchWarnings, so derived path flows into downstream validators and generated YAML correctly. nilentry guard prevents panics.- Integration test via
validateWorkflowDataverifies warning count + path derivation end-to-end. GH_AW_CHECKOUT_PATH_Ntest confirms non-empty env var after auto-derivation.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 76.7 AIC · ⌖ 7.31 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/workflow/checkout_path_validation.go:47
Empty derived path not guarded. When cfg.Repository has a trailing slash (e.g., "owner/") — malformed but user-provided — strings.LastIndex returns the slash position and cfg.Repository[slash+1:] produces an empty string. The code then sets cfg.Path = "", perpetuating the exact runtime failure this PR is fixing, and emits the confusing message The compiler has auto-derived path: "" from the repository name.
Add a guard after the slash split:
if slash := strings.LastIndex…
</details>
<details><summary>pkg/workflow/checkout_path_validation.go:28</summary>
**`validate*` naming convention violated by mutation.** This function is named `validateCrossRepoCheckoutPaths`, following the `validate*` convention for read-only checks, but it mutates `cfg.Path` as a side effect. All other functions in the `validations` table in `validateCoreToolConfiguration` are pure validators with no observable side effects on the input data.
Consider either:
1. Renaming to `fixupCrossRepoCheckoutPaths` (or `deriveCrossRepoCheckoutPaths`) to make the mutation explicit, …
</details>
<details><summary>pkg/workflow/checkout_path_validation_test.go:249</summary>
**Missing test for empty-derived-path edge case.** The table doesn't cover a trailing-slash repository slug like `"owner/"`. After `strings.LastIndex`, `repoName` would be `""` and `cfg.Path` would be set to `""` — the same empty-path bug being fixed. A regression test would ensure any future guard is not accidentally removed:
```go
// In TestWarnCrossRepoPath_RepoNameExtraction:
{"owner/", ""}, // trailing slash: expect no mutation / skip(This test would currently fail, confirming the g…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnose, and /zoom-out — requesting changes on test-coverage gaps and wording issues.
📋 Key Themes & Highlights
Key Issues
- Misleading test (
/tdd):"warning message for static repo contains suggested path"does not actually assert any message text — the comment even says "the actual message text is validated by the path value and warning count", which is incorrect. A future change that removes thepath:hint from the warning text would pass this test. - Wrong semantics for
deprecated(/diagnose): Calling brand-new auto-derivation behaviour "deprecated" confuses workflow authors. "Deprecated" implies a prior stable API being removed. - No strict-mode escalation (
/zoom-out):validateSafeOutputsStepsShellExpansiondemonstrates thec.strictModeguard pattern for turning warnings into errors. Without it here, there is no enforcement path and no test coverage for strict-mode behaviour. - Misleading helper comment (
/tdd):warnCrossRepoPathis described as capturing warning message text, but its signature returns only(path, warnCount). - Missing table entry (
/tdd): zero-slash repository ("just-repo-no-owner") is an untested fallback path.
Positive Highlights
- ✅ Correct fix for the root cause — empty
GH_AW_CHECKOUT_PATH_Nwas the real runtime failure - ✅ Dynamic
${{...}}expressions correctly skipped to avoid false positives on${{ github.repository }} - ✅ Excellent integration tests:
TestCrossRepoCheckoutPathAppearsInManifestStepandTestCrossRepoCheckoutPathAppearsInCheckoutStepconfirm the downstream YAML effects - ✅ Validator placed correctly before
validatePushToPullRequestBranchWarningsin the pipeline - ✅ Nil-safety guard in the loop prevents panics on malformed configs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80 AIC · ⌖ 8.14 AIC · ⊞ 6.6K
Comment /matt to run again
| assert.Empty(t, configs[0].Path) | ||
| }) | ||
|
|
||
| t.Run("warning message for static repo contains suggested path", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Test name promises "contains suggested path" but the body only checks path derivation and warnCount — the actual warning message text is never asserted.
If someone changes the warning to omit the suggested path: YAML snippet, this test would still pass. Consider either renaming the subtest to match what it actually checks, or add a real assertion on the message text.
💡 How to capture and assert stderr in Go
Capture os.Stderr with os.Pipe() before calling the validator:
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
c.validateCrossRepoCheckoutPaths(configs, markdownPath)
w.Close()
os.Stderr = oldStderr
var buf bytes.Buffer
buf.ReadFrom(r)
msg := buf.String()
assert.Contains(t, msg, `path: widget-service`, "warning must include the suggested path line")Alternatively, refactor the validator to return the warning string so the caller controls output — that also makes testing trivial.
@copilot please address this.
| msg := strings.Join([]string{ | ||
| fmt.Sprintf("checkout: repository %q has no explicit path: field.", cfg.Repository), | ||
| fmt.Sprintf("The compiler has auto-derived path: %q from the repository name.", repoName), | ||
| "This auto-derivation is deprecated. Add an explicit path: to silence this warning:", |
There was a problem hiding this comment.
[/diagnose] "This auto-derivation is deprecated" is misleading — this behavior is being introduced here, not removed. "Deprecated" implies a prior stable contract is being withdrawn, which will confuse workflow authors who encounter this for the first time.
Consider rewording to set accurate expectations:
💡 Suggested rewording
"Explicit path: is strongly recommended. Auto-derivation may be enforced as an error in a future version.",or if strict-mode enforcement is planned:
"Add an explicit path: field to silence this warning. Auto-derivation will become an error in a future release.",This aligns with how other gh-aw warnings frame future enforcement (e.g. validateSafeOutputsStepsShellExpansion).
@copilot please address this.
| " path: " + repoName, | ||
| }, "\n") | ||
| fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", msg)) | ||
| c.IncrementWarningCount() |
There was a problem hiding this comment.
[/zoom-out] This validator always produces a warning but never an error. The existing validateSafeOutputsStepsShellExpansion validator shows the pattern for escalating to an error in strict mode:
if c.strictMode {
return formatCompilerError(markdownPath, "error", err.Error(), err)
}If the roadmap is to eventually require explicit path:, strict mode is the natural enforcement lever — and the warning message already says the behavior "is deprecated" (see comment above on that wording). Without strict-mode gating, there is no enforcement path and no test for it.
💡 Suggested structure
if c.strictMode {
// In strict mode, missing path: is an error — do not auto-derive.
errMsg := fmt.Sprintf(
"checkout: repository %q requires an explicit path: field (strict mode)",
cfg.Repository,
)
return // or accumulate errors
}
// Warning path (non-strict): auto-derive and warn.
cfg.Path = repoName
...
c.IncrementWarningCount()Even if strict mode enforcement is not planned right now, a comment documenting the intentional omission would help future contributors.
@copilot please address this.
| {"owner/repo", "repo"}, | ||
| {"github/copilot", "copilot"}, | ||
| {"githubnext/gh-aw-side-repo", "gh-aw-side-repo"}, | ||
| {"a/b/c", "c"}, // only the last segment |
There was a problem hiding this comment.
[/tdd] The table tests cover owner/repo, multi-slash a/b/c, and similar — but not the zero-slash case: Repository: "just-repo-no-owner".
The code falls back to using the full string when strings.LastIndex returns -1, which means path = "just-repo-no-owner". Is that the intended behaviour? It is untested and undocumented. Even if owner/repo is the only valid GitHub slug format, a defensive test documents the fallback contract:
💡 Suggested table entry
{"just-repo-no-owner", "just-repo-no-owner"}, // no slash: full string used as path@copilot please address this.
| } | ||
| } | ||
|
|
||
| // warnCrossRepoPath is a helper used by tests that need to capture the warning message text. |
There was a problem hiding this comment.
[/tdd] The helper comment says it is "for tests that need to capture the warning message text", but its return signature (path string, warnCount int) captures no message text at all. The comment sets a false expectation.
Either update the comment to match the actual return values (path and warnCount), or extend the helper to also return the captured stderr string — which would unblock the message-text assertions noted in the "warning message for static repo..." test above.
💡 Extended helper that captures stderr
func warnCrossRepoPath(t *testing.T, repository string) (path, stderrMsg string, warnCount int) {
t.Helper()
r, w, _ := os.Pipe()
orig := os.Stderr
os.Stderr = w
defer func() { os.Stderr = orig }()
c := NewCompiler()
cfg := &CheckoutConfig{Repository: repository}
c.validateCrossRepoCheckoutPaths([]*CheckoutConfig{cfg}, "test.md")
w.Close()
var buf bytes.Buffer
buf.ReadFrom(r)
return cfg.Path, buf.String(), c.GetWarningCount()
}@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness issue must be addressed before merge
The direction is right: auto-deriving path: before YAML generation is the correct fix for empty GH_AW_CHECKOUT_PATH_N. The overall structure is clean and the test coverage is solid for the happy path.
Blocking issue and two non-blocking concerns
Blocking
- Empty derived path for malformed slugs (
checkout_path_validation.go:51): anyRepositoryslug whose name-segment is empty (e.g."owner/"or"/") causesrepoName = "", socfg.Pathis set to""— the exact runtime failure this PR is fixing. Add a guard that skips or errors whenrepoName == ""after derivation.
Non-blocking (but worth addressing)
validate*naming convention broken (checkout_path_validation.go:28): every othervalidate*method is read-only; this one mutates its input. Rename to reflect the dual responsibility.- Misleading test name (
checkout_path_validation_test.go:111):"warning message for static repo contains suggested path"implies the message text is asserted, but the body only checks count and derived path. Rename or add a stderr capture assertion.
🔎 Code quality review by PR Code Quality Reviewer · 200.8 AIC · ⌖ 7.67 AIC · ⊞ 5.2K
Comment /review to run again
| } | ||
|
|
||
| checkoutPathValidationLog.Printf("auto-deriving path %q for cross-repo checkout %q", repoName, cfg.Repository) | ||
| cfg.Path = repoName |
There was a problem hiding this comment.
Empty cfg.Path still possible for malformed trailing-slash slugs: strings.LastIndex("owner/", "/") returns 5, making repoName = "", so cfg.Path is set to "" — reproducing the very runtime failure this PR is trying to prevent.
💡 Suggested fix
After computing repoName, guard against the empty result before mutating:
repoName := cfg.Repository
if slash := strings.LastIndex(cfg.Repository, "/"); slash >= 0 {
repoName = cfg.Repository[slash+1:]
}
if repoName == "" {
// malformed slug (e.g. "owner/" or "/") — cannot derive a safe path; skip auto-derivation
// and leave it to the user or a future schema-validation pass.
checkoutPathValidationLog.Printf("skipping auto-derivation: empty repo-name for %q", cfg.Repository)
continue
}Without this guard the downstream GH_AW_CHECKOUT_PATH_N: "" emission and the associated safe-outputs runtime failure (Repository not found in workspace) remain reachable for any slug whose repository-name segment is empty — whether that comes from "owner/", "/", or similar degenerate inputs.
| // cannot determine at compile time whether they target a different repository, and | ||
| // common patterns such as `repository: ${{ github.repository }}` are valid | ||
| // same-repository checkouts that should not receive a warning. | ||
| func (c *Compiler) validateCrossRepoCheckoutPaths(checkoutConfigs []*CheckoutConfig, markdownPath string) { |
There was a problem hiding this comment.
validate* naming convention violated — this function mutates its input: every other validate* method in the codebase is read-only and returns an error. This function silently mutates cfg.Path on caller-owned objects. Future maintainers adding a compile pass before this one, or a validator that checks cfg.Path == "" to mean "not yet set", will be surprised by the mutation.
💡 Suggested approach
Rename to deriveAndWarnCrossRepoCheckoutPaths (or similar) to be honest about the dual responsibility, and add a top-of-function comment making the mutation explicit:
// deriveAndWarnCrossRepoCheckoutPaths auto-derives cfg.Path for cross-repo checkout
// entries that have no explicit path: field, and emits a deprecation warning for each.
//
// NOTE: This function mutates the CheckoutConfig entries in the slice; it must be
// called before any step that reads cfg.Path (e.g. GenerateCheckoutManifestStep).
func (c *Compiler) deriveAndWarnCrossRepoCheckoutPaths(...) {Alternatively, return the list of modified configs or only mutate inside a clearly named deriveCheckoutPath helper that is invoked explicitly by the compilation step, keeping validation logic purely diagnostic.
| c.validateCrossRepoCheckoutPaths(configs, markdownPath) | ||
| assert.Equal(t, "widget-service", configs[0].Path) | ||
| assert.Equal(t, 1, c.GetWarningCount()) | ||
| }) |
There was a problem hiding this comment.
Test name promises message content verification but the test body does not deliver it: the comment inside the test even acknowledges this gap: // the actual message text is validated by the path value and warning count. — but the test name is "warning message for static repo contains suggested path", which implies the message text is checked.
💡 Suggested fix
Capture stderr during the call and assert on the message text, e.g.:
var buf bytes.Buffer
origStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
c.validateCrossRepoCheckoutPaths(configs, markdownPath)
w.Close()
os.Stderr = origStderr
io.Copy(&buf, r)
out := buf.String()
assert.Contains(t, out, "widget-service", "warning must include the derived path")
assert.Contains(t, out, `path: widget-service`, "warning must include the explicit-path suggestion")Or rename the test to "cross-repo checkout warning fires once and derives correct path" so the name matches what it actually validates.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage — Run §28413530597
Notes: Carried over (11.1h, was draft). CI all passing (19/19 ✅). Ready for review. Blocked by 4× CHANGES_REQUESTED from
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in aa2521b. I ran the PR-finisher pass, tightened the checkout-path fix around explicit root and malformed repository values, and re-ran local validation ( |
When a
checkout:block has a cross-repo entry without an explicitpath:, the compiler was emittingGH_AW_CHECKOUT_PATH_N: "", causing safe-outputs handlers (e.g.push_to_pull_request_branch) to fail at runtime with "Repository not found in workspace."Changes
pkg/workflow/checkout_path_validation.go— newvalidateCrossRepoCheckoutPathsvalidator that:path:from the repo-name segment ofowner/repo(e.g.githubnext/gh-aw-side-repo→gh-aw-side-repo) and mutates theCheckoutConfigbefore YAML generation, ensuring both the emittedactions/checkoutstep andGH_AW_CHECKOUT_PATH_Nin the manifest are non-emptypath:to silence it${{…}}expressions — cannot be resolved at compile time; common patterns likerepository: ${{ github.repository }}(trustedpull_request_targetcheckout) must not warnpkg/workflow/compiler_validators.go— wiresvalidateCrossRepoCheckoutPathsintovalidateCoreToolConfiguration, before checkout step generation