Generate PR patches from current: true checkout path in multi-checkout workflows#34875
Generate PR patches from current: true checkout path in multi-checkout workflows#34875Copilot wants to merge 4 commits into
current: true checkout path in multi-checkout workflows#34875Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
current: true checkout path in multi-checkout workflows
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect PR patch diffs in multi-checkout workflows by generating patches from the current: true checkout directory (when the PR tools target that same repository), rather than always using GITHUB_WORKSPACE.
Changes:
- Compiler/runtime config now propagates a
patch_workspace_path(and acurrent_checkout_repoidentifier) for PR patch generation whencurrent: truepoints to a subdirectory. - Runtime JS patch generation supports a
workspacePathoption with validation to ensure the patch is generated from the intended checkout directory. - Added/updated Go + JS tests and documentation to cover/describe the behavior in multi-checkout scenarios.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_patch_workspace.go | New helper to inject patch_workspace_path/current_checkout_repo into handler configs based on current: true checkout. |
| pkg/workflow/safe_outputs_config_generation.go | Calls injection during safe-outputs config JSON generation. |
| pkg/workflow/safe_outputs_config_generation_test.go | Tests injection into generated safe-outputs config JSON. |
| pkg/workflow/compiler_safe_outputs_config.go | Calls injection when building handler manager env var config. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Tests injected fields appear in compiled handler config env var JSON. |
| pkg/workflow/checkout_manager.go | Adds GetCurrentCheckoutPath() / GetCurrentRepository() helpers. |
| pkg/workflow/checkout_manager_test.go | Unit tests for GetCurrentCheckoutPath(). |
| docs/src/content/docs/reference/safe-outputs.md | Documents patch generation behavior for current: true multi-checkout workflows. |
| docs/src/content/docs/reference/cross-repository.md | Documents patch generation behavior for current: true subdirectory checkouts. |
| actions/setup/js/safe_outputs_handlers.cjs | Uses patch_workspace_path (when repo matches) and validates the directory before patch generation. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Tests that patch_workspace_path is used when the target repo resolves via GH_AW_TARGET_REPO_SLUG. |
| actions/setup/js/generate_git_patch.cjs | Adds options.workspacePath validation and uses it as the git working directory. |
| actions/setup/js/generate_git_patch.test.cjs | Tests successful patch generation from workspacePath + rejection of traversal paths. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 2
| // Skip for wildcard and explicitly different repositories. | ||
| if targetRepo == "*" { | ||
| return | ||
| } | ||
| if targetRepo != "" && currentRepo != "" && targetRepo != currentRepo { | ||
| return |
| // GetCurrentCheckoutPath returns the normalized (workspace-relative) path for | ||
| // the checkout marked current:true. Returns an empty string when no current | ||
| // checkout is configured or when the current checkout is at workspace root. | ||
| func (cm *CheckoutManager) GetCurrentCheckoutPath() string { | ||
| for _, entry := range cm.ordered { | ||
| if !entry.current { | ||
| continue | ||
| } | ||
| path := strings.TrimSpace(strings.TrimPrefix(entry.key.path, "./")) | ||
| if path == "." || path == "" { | ||
| return "" | ||
| } | ||
| return path | ||
| } |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (196 new lines under 📄 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
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.9M
| let repoSlug = null; | ||
| const patchWorkspacePath = typeof prConfig.patch_workspace_path === "string" ? prConfig.patch_workspace_path.trim() : ""; | ||
| const currentCheckoutRepo = typeof prConfig.current_checkout_repo === "string" ? prConfig.current_checkout_repo.trim() : ""; | ||
| const patchWorkspaceMatchesTargetRepo = patchWorkspacePath && (!currentCheckoutRepo || currentCheckoutRepo === repoResult.repo); |
There was a problem hiding this comment.
Runtime analog of compiler-side bug: patchWorkspaceMatchesTargetRepo is true when currentCheckoutRepo is empty, matching any target repo.
The JS handler has no independent defense against this: it trusts that the compiler either sets both patch_workspace_path and current_checkout_repo together, or neither. That contract is not enforced here.
💡 Suggested fix
Add an explicit guard that treats an empty currentCheckoutRepo with a configured target-repo as a non-match:
const configuredTargetRepo = (prConfig["target-repo"] || "").trim();
const patchWorkspaceMatchesTargetRepo =
patchWorkspacePath &&
(
(currentCheckoutRepo && currentCheckoutRepo === repoResult.repo) // explicit target-repo → must match currentCheckoutRepo
);The same fix applies to line 725 (pushPatchWorkspaceMatchesTargetRepo). This makes the JS handler resilient even if the compiler emits an incomplete config.
| return | ||
| } | ||
|
|
||
| checkoutManager := NewCheckoutManager(data.CheckoutConfigs) |
There was a problem hiding this comment.
NewCheckoutManager constructed inside a per-handler call, allocating a full CheckoutManager for every handler in the loop.
injectCurrentCheckoutPatchWorkspacePath is called inside the handler loop in both addHandlerManagerConfigEnvVar and generateSafeOutputsConfig. Each call re-parses and re-builds the ordered checkout list, even though the result is identical across all invocations for the same data.
💡 Suggested fix
Resolve the checkout path and repo once before the loop and pass them in:
func injectCurrentCheckoutPatchWorkspacePath(handlerName string, handlerCfg map[string]any, currentPath, currentRepo string) {
// no more NewCheckoutManager call here
}
// At call site, before the handler loop:
cm := NewCheckoutManager(data.CheckoutConfigs)
currentPath := normalizeCurrentCheckoutPatchPath(cm.GetCurrentCheckoutPath())
currentRepo := strings.TrimSpace(cm.GetCurrentRepository())
for handlerName, handlerCfg := range handlers {
injectCurrentCheckoutPatchWorkspacePath(handlerName, handlerCfg, currentPath, currentRepo)
}This also makes the function easier to test in isolation without needing to construct WorkflowData with CheckoutConfigs.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out. The fix is well-scoped and the root cause is correctly addressed — commenting with suggestions rather than blocking.
📋 Key Themes & Highlights
Key Themes
- Redundant path-traversal validation:
resolvePatchWorkspacePathin the handler already validates and resolves the absolute path, thenpatchOptions.workspacePathpasses the relative string togenerateGitPatchwhich validates a second time. These two validation layers could diverge if maintained independently. - Test coverage gaps on guard conditions: The
target-repo: "*"wildcard skip and the repo-mismatch early-return ininjectCurrentCheckoutPatchWorkspacePathhave no dedicated tests — these are exactly the cases that would silently break multi-repo workflows. - Weak integration-test assertions: Both new
safe_outputs_handlerstests only verify a debug log was emitted, not that the patch content actually came from the subdirectory. entry.repo_cwdasymmetry: Set forpush_to_pull_request_branchbut notcreate_pull_request— warrants a comment on intent.
Positive Highlights
- ✅ Clean separation between compile-time injection (
safe_outputs_patch_workspace.go) and runtime resolution (safe_outputs_handlers.cjs) - ✅ Path-traversal guard is thorough:
path.relativesentinel +existsSync+isDirectorycheck - ✅ Conditional injection (
target-repomismatch, wildcard) correctly avoids corrupting cross-repo workflows - ✅ Good test coverage for the Go layer (
checkout_manager_test.go,compiler_safe_outputs_config_test.go,safe_outputs_config_generation_test.go) - ✅ Docs updated in both
cross-repository.mdandsafe-outputs.md
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.2M
Comments that could not be inline-anchored
actions/setup/js/safe_outputs_handlers.cjs:561
[/diagnose] The path is already fully validated and resolved by resolvePatchWorkspacePath (which returns the absolute path as repoCwd), yet patchOptions.workspacePath is set to the relative patchWorkspacePath string — causing generateGitPatch to re-validate and re-resolve the same path a second time.
<details>
<summary>💡 Why this matters</summary>
generateGitPatch checks workspacePath and silently returns {success: false} if the re-validation fails for any reason (e.g.,…
actions/setup/js/safe_outputs_handlers.cjs:752
[/diagnose] push_to_pull_request_branch sets entry.repo_cwd = repoCwd (line ~248 in the diff) when pushPatchWorkspaceMatchesTargetRepo, but the equivalent create_pull_request handler does not set entry.repo_cwd. If entry.repo_cwd is consumed later in the push flow (e.g., for git checkout/commit steps), this asymmetry is intentional — but it's not documented. If it's not used downstream, it's dead code that should be removed.
<details>
<summary>💡 Suggestion</summary>
Add a com…
actions/setup/js/safe_outputs_handlers.test.cjs:1007
[/tdd] This test only verifies that mockServer.debug was called with a message containing "Using configured patch_workspace_path". It doesn't assert that the patch was actually generated from the subdirectory. A spurious debug log would cause this test to pass even if the patch was still generated from the workspace root.
<details>
<summary>💡 Stronger assertion</summary>
Read the patch file and assert it contains changes from the subdirectory repo, not workspace-root gitlinks:
</details>
<details><summary>pkg/workflow/safe_outputs_patch_workspace.go:17</summary>
**[/zoom-out]** `injectCurrentCheckoutPatchWorkspacePath` constructs a brand-new `CheckoutManager` from `data.CheckoutConfigs` on every call (once per handler, called from two call sites). The compiler already builds a `CheckoutManager` during compilation — if `WorkflowData` exposed it (or if `injectCurrentCheckoutPatchWorkspacePath` accepted a `*CheckoutManager` parameter), this allocation could be avoided and the function would be easier to unit-test without needing to populate `WorkflowData`…
</details>
<details><summary>pkg/workflow/checkout_manager_test.go:1119</summary>
**[/tdd]** The test suite for `GetCurrentCheckoutPath` covers three cases (no current, root `.`, and a non-root subdirectory), but misses the case where `Current: true` is set and `Path` is an empty string `""`. `GetCurrentCheckoutPath` does `strings.TrimPrefix(entry.key.path, "./")` — an empty path would return `""` (the root guard catches it), but it's worth an explicit test to lock the contract, especially since `Path: ""` is a plausible zero-value config.
<details>
<summary>💡 Suggested te…
</details>
<details><summary>pkg/workflow/safe_outputs_patch_workspace.go:25</summary>
**[/tdd]** There is no test covering the case where `target-repo: "*"` (wildcard) skips injection, or where `target-repo` is set to a *different* repo than `current_checkout_repo` (the early-return at line 34). These guard conditions protect against accidentally injecting the wrong checkout path for cross-repo scenarios.
<details>
<summary>💡 Suggested additions to `TestHandlerConfigInjectsCurrentCheckoutPatchWorkspacePath`</summary>
```go
t.Run("skips injection when target-repo is wildcard",…
</details>
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 47/100 — Needs Improvement
📊 Metrics & Test Classification (7 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
✅ Test Quality Sentinel: 47/100. Test quality is acceptable — 28.6% of new tests are implementation tests (threshold: 30%). Advisory: improve assertion messages in Go tests and replace debug-log assertions in JS handlers tests with behavioral output checks.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in commit
Targeted tests pass for the updated workflow config paths. |
|
``
|
create_pull_requestandpush_to_pull_request_branchwere always generating patches fromGITHUB_WORKSPACE, which produced incorrect diffs (e.g., gitlink/subproject changes) when the logical target repo was checked out in a subdirectory viacurrent: true. This change threads the current checkout path from compilation into runtime patch generation and uses it when the operation targets that repo.Compiler: expose and propagate current checkout path
CheckoutManager.GetCurrentCheckoutPath()andGetCurrentRepository().create_pull_request,push_to_pull_request_branch):patch_workspace_pathcurrent_checkout_repocurrent: truecheckout paths and non-conflicting target repo configurations.Runtime: generate patch from configured checkout directory
generateGitPatch()to acceptoptions.workspacePath.workspacePathrelative toGITHUB_WORKSPACEand validates:safe_outputs_handlersnow usespatch_workspace_pathforcreate_pull_requestandpush_to_pull_request_branchwhen repo targeting matches the current checkout repo.Docs: clarify cross-repo/current checkout behavior
current: truecheckout directory when targeting that repository.