fix(safe-outputs): bridge git safe.directory trust via GIT_CONFIG_* env vars#46057
Conversation
…ship Extract ensureSafeDirectoryTrust from safe_outputs_handlers.cjs to git_helpers.cjs so it is available to the bridge (Process Safe Outputs step) as well as the MCP server container. The bridge runs outside the Docker container as a potentially different user/HOME, so the in-container `git config --global safe.directory` never reaches it. Using GIT_CONFIG_* env vars in the bridge process directly ensures git trusts the checkout regardless of HOME or uid. - Extract ensureSafeDirectoryTrust to git_helpers.cjs (uses core.debug, no server param), export it - Update safe_outputs_handlers.cjs to import from git_helpers.cjs and remove local definition; update 3 call sites to drop server param - Call ensureSafeDirectoryTrust in push_to_pull_request_branch.cjs: - In main() factory with GITHUB_WORKSPACE (covers all messages) - Per-message for cross-repo repoCwd subdirectory checkouts - Fix bundle apply error message to include actual error text - Add 8 new unit tests for ensureSafeDirectoryTrust Closes #46028 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Bridges Git safe-directory trust into safe-output processes that may use a different user or home directory.
Changes:
- Extracts and exports the shared trust helper.
- Applies trust to workspace and cross-repository checkouts.
- Adds unit coverage and improves bundle errors.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_helpers.cjs |
Adds the shared safe-directory helper. |
actions/setup/js/git_helpers.test.cjs |
Tests environment configuration behavior. |
actions/setup/js/safe_outputs_handlers.cjs |
Uses the shared helper. |
actions/setup/js/push_to_pull_request_branch.cjs |
Trusts bridge checkouts and reports bundle errors. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher and pushed This follow-up hardens malformed Focused JS tests passed, |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![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 — two minor issues worth addressing, no blockers.
Positive highlights
- Root cause properly addressed: bridge now gets
safe.directorytrust viaGIT_CONFIG_*env vars, bypassing~/.gitconfigfor cross-uid/HOME scenarios. - Deduplication and malformed-count guards (
Number.isSafeInteger) prevent subtle env-var chain corruption. - Factory-level + per-message call pattern in
push_to_pull_request_branch.cjscovers bothGITHUB_WORKSPACEand cross-repo subdirectory checkouts. - Comprehensive test coverage: falsy input, dedup, multi-directory, auth coexistence, malformed counts, missing
coreglobal. - Improved bundle-apply error message surfaces the actual git error.
Two issues raised (see inline comments)
- Debug logging regression —
server.debug()in the MCP container path is silently dropped after removing theserverarg. - Test fragility — auth-coexistence test hardcodes index 0 instead of deriving it from
getGitAuthEnv's actual count.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 37 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot review #46057 (review) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in
Addressed in |
|
@copilot please run the
|
Ran the PR finisher pass. No new maintainer feedback required additional code changes beyond Re-ran checks on the current head:
CI on this head is still unverified until a maintainer re-triggers it, since agent pushes do not start workflows. |
🤖 PR Triage
Rationale: Safe-outputs git safe.directory trust bridge — prevents 'dubious ownership' rejections on push_to_pull_request_branch for cross-uid runners. CI running. Expedite.
|
|
@copilot please run the Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in I traced the failed Re-ran checks on the updated branch:
There are no unresolved in-scope review threads left from the PR feedback I could access. CI on this head still needs a maintainer re-trigger to verify the new commit. |
|
@copilot please run the
|
Checked the current PR state on
|
|
@copilot please run the
|
Checked the current head again on
|
The
push_to_pull_request_branchbridge (Process Safe Outputsstep) runs outside the safeoutputs Docker container, potentially as a different uid/HOME. The in-containergit config --global safe.directorynever reaches this process, causing git to reject the workspace with "detected dubious ownership" — silently leaving the Changeset Generator read-only.Changes
git_helpers.cjs— ExtractedensureSafeDirectoryTrust(gitCwd)fromsafe_outputs_handlers.cjsinto the shared helper module and exported it. SetsGIT_CONFIG_COUNT/KEY/VALUEenv vars in-process (git ≥2.31), bypassing~/.gitconfigentirely:safe_outputs_handlers.cjs— Replaced local definition with import fromgit_helpers.cjs; removedserverarg from three call sites.push_to_pull_request_branch.cjs— Added two call sites in the bridge path:main()factory: trustsGITHUB_WORKSPACEat handler init (covers all messages)repoCwdfor cross-repo subdirectory checkoutsgit_helpers.test.cjs— 8 new unit tests: no-op on falsy input, deduplication, multi-directory accumulation, coexistence with existingGIT_CONFIG_*entries (e.g. fromgetGitAuthEnv), malformed-count guard.