safe_outputs: set git safe.directory via process-scoped env vars before bundle branch pinning#41830
safe_outputs: set git safe.directory via process-scoped env vars before bundle branch pinning#41830Copilot wants to merge 16 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. TQS analysis complete for PR #41830. Comment and APPROVE review already submitted this run (limits reached). Score: 100/100 — 2 design tests, 0 implementation tests, 0 guideline violations. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41830 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This pull request hardens the safe-outputs MCP handlers against Git’s “detected dubious ownership in repository” failures that can occur when bundle-based branch pinning runs under a different HOME/user context (bridge vs container), preventing real-world drops where changesets were not pushed.
Changes:
- Added
ensureSafeDirectoryTrust(gitCwd, server)to configuregit config --global safe.directoryfor the current process HOME context. - Invoked
ensureSafeDirectoryTrust(...)immediately beforerev-parsebranch pinning in both bundle-capable flows:createPullRequestHandlerandpushToPullRequestBranchHandler. - Added targeted tests to verify
GITHUB_WORKSPACEis written into globalsafe.directoryunder an isolated HOME for both handlers.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_outputs_handlers.cjs | Adds a helper to trust the active checkout path via global safe.directory, and calls it before branch pinning in PR-creation and PR-branch-push flows. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Adds regression tests ensuring the handlers configure safe.directory in an isolated HOME context before pinning. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 0 ( Verdict
|
![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 /diagnose, /tdd, and /zoom-out — commenting with non-blocking observations on this production bug fix.
📋 Key Themes & Highlights
Key Themes
- Silent error swallowing: The first
catchinensureSafeDirectoryTrustsilently drops all--get-allerrors, not just "no values configured" — unexpected errors become invisible in production logs. - Missing idempotency test: The PR description promises "no duplicate growth", but no test exercises the check-then-add deduplication. A regression here would be silent.
- Per-call-site wiring: The guard is applied at two call sites rather than once at handler init, creating a maintenance trap if new bundle-capable handlers are added.
Positive Highlights
- ✅ Surgical fix — targets exactly the two code paths that trigger branch pinning under a different HOME
- ✅ Defensive by design — failure to configure
safe.directorydegrades gracefully (debug log only, no error propagation) - ✅ Good test isolation — using a dedicated
testHomedirectory avoids polluting the real global git config - ✅ Idempotency logic is present (check before add); the gap is test coverage, not the implementation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 75.3 AIC · ⌖ 12.7 AIC · ⊞ 6.6K
| if (existingEntries.includes(gitCwd)) { | ||
| return; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
[/diagnose] The silent catch here swallows all errors from --get-all, not just the expected "no values configured" exit code. If git is missing from PATH, HOME has wrong permissions, or a corrupted config causes a parsing error, the code silently falls through to --add — which may also fail, leaving the root cause invisible in production.
💡 Suggested improvement
Capture and log unexpected errors at the first catch site:
} catch (err) {
// git config --get-all exits non-zero when no values are configured yet.
// Log anything else to aid future diagnosis.
server.debug(`ensureSafeDirectoryTrust: --get-all failed: ${getErrorMessage(err)}`);
}This keeps the no-values path visible while surfacing genuinely unexpected failures.
@copilot please address this.
There was a problem hiding this comment.
Obsolete: the git config --global implementation with its catch block was replaced in commit 9c75a57 with process-scoped env-var injection (GIT_CONFIG_COUNT/KEY_N/VALUE_N). There is no catch block in the current implementation — process.env assignment never throws, eliminating this concern entirely.
| .toString() | ||
| .split("\n") | ||
| .map(line => line.trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
[/tdd] The PR description explicitly claims "no duplicate growth" when the same path is registered more than once, but there's no test for this idempotency property. If the check-then-add logic had a bug (e.g., a path normalisation mismatch or a TOCTOU race under concurrent requests), safe.directory could grow unboundedly without detection.
💡 Suggested idempotency test
it("does not add duplicate safe.directory entries on repeated handler calls", async () => {
const testHome = path.join(testWorkspaceDir, "home-safe-dir-idempotent");
fs.mkdirSync(testHome, { recursive: true });
const previousHome = process.env.HOME;
process.env.HOME = testHome;
try {
// Two calls simulate a retry or concurrent invocation.
await handlers.createPullRequestHandler({ branch: "b", title: "t", body: "b" });
await handlers.createPullRequestHandler({ branch: "b", title: "t", body: "b" });
const entries = execSync("git config --global --get-all safe.directory", {
env: { ...process.env, HOME: testHome },
stdio: "pipe",
}).toString().split("\n").map(l => l.trim()).filter(Boolean);
expect(entries.filter(e => e === testWorkspaceDir)).toHaveLength(1);
} finally {
if (previousHome === undefined) delete process.env.HOME;
else process.env.HOME = previousHome;
}
});@copilot please address this.
There was a problem hiding this comment.
Obsolete: the old git config --global approach and its test were replaced. The current env-var implementation includes a dedup test "does not add duplicate safe.directory entries on repeated handler calls with the same path" (line 775, commit 43ea018) that verifies idempotency via GIT_CONFIG_COUNT/KEY_N/VALUE_N env vars.
| // This prevents TOCTOU races where the agent flips the ref between patch and bundle | ||
| // generation, causing the two to represent different commit sets. | ||
| const gitCwd = repoCwd || process.env.GITHUB_WORKSPACE || process.cwd(); | ||
| ensureSafeDirectoryTrust(gitCwd, server); |
There was a problem hiding this comment.
[/zoom-out] ensureSafeDirectoryTrust is wired at two call sites independently. If a third bundle-capable handler is added later, contributors must remember to insert the guard — and a missed call site would silently re-introduce the same production failure.
💡 Consider a shared initialization guard
Calling the guard once at the top of createHandlers (before the handler map is built) would make protection automatic for all current and future handlers:
function createHandlers(server, appendSafeOutput, config = {}) {
// Guard once for the lifetime of this server instance.
ensureSafeDirectoryTrust(
config.repoCwd || process.env.GITHUB_WORKSPACE || process.cwd(),
server,
);
// ... rest of handler setup
}This eliminates per-call-site duplication and makes the protection opt-out rather than opt-in.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 43ea018: ensureSafeDirectoryTrust(process.env.GITHUB_WORKSPACE || process.cwd(), server) is now called once at the top of createHandlers (line 241), covering all current and future handlers automatically. Per-handler calls remain to cover per-request checkout paths that may differ from GITHUB_WORKSPACE.
There was a problem hiding this comment.
REQUEST_CHANGES — The fix is correctly targeted at the right root cause, but there are medium-severity issues in both the implementation and the new tests that need addressing before merge.
🔍 Blocking themes
Silent failure gives no diagnostic trail
When git config --global --add safe.directory fails (read-only HOME, broken git install, etc.), the function swallows the error at debug level and the caller proceeds to rev-parse. That rev-parse will then fail with the same "dubious ownership" message as before, and there's no log entry connecting it to the failed fixup. Debug logs are gated behind extra flags, so this failure mode is invisible in normal CI output. See inline comment on line 224.
git config --global is a global side effect that outlives the operation
Writing to ~/.gitconfig is persistent, accumulates across retries, and is not rolled back if the PR/push operation subsequently fails. A per-command -c safe.directory=PATH flag would scope the trust exactly to the git calls that need it without touching persistent state. See inline comment on line 221.
Tests are structurally fragile
execSync("git config --global --get-all safe.directory", ...)exits with code 1 when no entries exist, whichexecSynctranslates to a thrown error. If the handler fails beforeensureSafeDirectoryTrustruns, the test crashes with a genericexecSyncerror rather than a cleanexpect(...).toContain(...)failure. See inline comment on line 754 of the test file.- The "no duplicate growth" invariant is explicitly claimed in the PR description but has no test covering it. A deduplication regression would be invisible. See inline comment on line 767 of the test file.
🔎 Code quality review by PR Code Quality Reviewer · 88.4 AIC · ⌖ 7.25 AIC · ⊞ 5.2K
| server.debug(`Configured git safe.directory for bridge context: ${gitCwd}`); | ||
| } catch (error) { | ||
| server.debug(`Failed to configure git safe.directory '${gitCwd}': ${getErrorMessage(error)}`); | ||
| } |
There was a problem hiding this comment.
Silent failure degrades to debug only, leaving no breadcrumb when the config write itself is the root cause.
💡 Details and suggested fix
When git config --global --add safe.directory fails (e.g., the bridge HOME has a read-only gitconfig or an unusable path), the catch block calls server.debug(...) and returns silently. The immediately following rev-parse then fails with the original "detected dubious ownership" message — which says nothing about the safe.directory write failure. An operator debugging a production drop will see the pinning error but have no trail connecting it to the failed fixup.
Minimum fix: use a higher-severity log level so the failure surfaces without debug flags:
} catch (error) {
server.error(
`Failed to configure git safe.directory '${gitCwd}'; ` +
`subsequent git operations may still fail with ownership errors: ${getErrorMessage(error)}`
);
}If this helper is deliberately best-effort, document that intent and consider whether the caller should at least know it didn't succeed.
There was a problem hiding this comment.
The git config --global approach with its catch block has been replaced entirely with process-scoped GIT_CONFIG_COUNT/GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N env-var injection. process.env assignment never throws, so there is no failure path and no catch block needed. This eliminates the silent-swallow risk entirely.
| } | ||
| try { | ||
| execGitSync(["config", "--global", "--add", "safe.directory", gitCwd], { suppressLogs: true }); | ||
| server.debug(`Configured git safe.directory for bridge context: ${gitCwd}`); |
There was a problem hiding this comment.
git config --global permanently mutates the process-level gitconfig; git -c safe.directory=PATH per-command would be more surgical.
💡 Details and alternative approach
Every invocation of ensureSafeDirectoryTrust writes a persistent entry into the bridge HOME's ~/.gitconfig. If the bridge process is long-lived and handles multiple repos (or the same repo is retried), entries accumulate. The wildcard safe.directory = * entry (which globally trusts all dirs) is also not detected by the current string comparison, so the function may redundantly write specific entries even when universal trust is already configured.
Git supports a per-command alternative that requires no gitconfig mutation:
git -c safe.directory=PATH rev-parse ...In code terms, pass ['-c', safe.directory=${gitCwd}] as extra git args to the commands that need it, rather than patching the global config. This approach:
- Scopes the trust to exactly the commands that need it
- Leaves the global gitconfig untouched
- Works even if the global gitconfig is read-only
The tradeoff is a slightly more invasive change to execGitSync call sites, but it eliminates the hidden global-state side effect entirely.
There was a problem hiding this comment.
Obsolete: git config --global was replaced in commit 9c75a57 with process-scoped GIT_CONFIG_COUNT/GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N env-var injection. No writes to ~/.gitconfig, no persistent global side effects, and the approach works even with a read-only gitconfig.
| body: "Test description", | ||
| }); | ||
|
|
||
| const safeDirectories = execSync("git config --global --get-all safe.directory", { |
There was a problem hiding this comment.
execSync throws on exit code 1 when no safe.directory entries exist, producing a cryptic error instead of a clean Jest assertion failure.
💡 Details and fix
git config --global --get-all safe.directory exits with code 1 when the key has no configured values — not just when the command is invalid. execSync translates non-zero exit to a thrown Error. If ensureSafeDirectoryTrust fails (e.g., silently swallowed exception in the add step), or the handler returns before reaching it, the execSync call here throws and the test fails with:
Error: Command failed: git config --global --get-all safe.directory
...instead of the much more diagnostic:
Expected: ["...", "/workspace"]
Received: []
Fix by catching the throw and defaulting to an empty array:
let safeDirectories = [];
try {
safeDirectories = execSync("git config --global --get-all safe.directory", {
env: { ...process.env, HOME: testHome },
stdio: "pipe",
})
.toString()
.split("\n")
.map(line => line.trim())
.filter(Boolean);
} catch { /* no entries configured */ }
expect(safeDirectories).toContain(testWorkspaceDir);This same issue applies to the identical execSync call in the pushToPullRequestBranchHandler test.
There was a problem hiding this comment.
Obsolete: the test no longer uses git config --global --get-all / execSync. It now reads directly from process.env (GIT_CONFIG_COUNT, GIT_CONFIG_KEY_N, GIT_CONFIG_VALUE_N), so there is no execSync throw to guard against.
| if (previousHome === undefined) delete process.env.HOME; | ||
| else process.env.HOME = previousHome; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The PR claims "no duplicate growth" but no test exercises the deduplication path — a future breakage would go undetected.
💡 Details and suggested test
The idempotency guarantee — that calling ensureSafeDirectoryTrust twice with the same path doesn't accumulate duplicate safe.directory entries — is the main stated invariant of this feature. The current test only verifies the forward path ("entry is written") but not that a second call does not write a duplicate.
Add a test like:
it("does not add duplicate safe.directory entries on repeated calls", async () => {
const testHome = path.join(testWorkspaceDir, "home-safe-directory-dedup");
fs.mkdirSync(testHome, { recursive: true });
const previousHome = process.env.HOME;
process.env.HOME = testHome;
try {
// First call
await handlers.createPullRequestHandler({ branch: "feature", title: "T", body: "B" });
// Second call
await handlers.createPullRequestHandler({ branch: "feature", title: "T", body: "B" });
let safeDirectories = [];
try {
safeDirectories = execSync("git config --global --get-all safe.directory", {
env: { ...process.env, HOME: testHome }, stdio: "pipe",
}).toString().split("\n").map(l => l.trim()).filter(Boolean);
} catch { /* no entries */ }
const occurrences = safeDirectories.filter(d => d === testWorkspaceDir);
expect(occurrences).toHaveLength(1);
} finally {
if (previousHome === undefined) delete process.env.HOME;
else process.env.HOME = previousHome;
}
});There was a problem hiding this comment.
Added in commit 43ea018: test "does not add duplicate safe.directory entries on repeated handler calls with the same path" (line 775 in safe_outputs_handlers.test.cjs) verifies that two successive createPullRequestHandler calls with the same workspace path grow the env-var chain by exactly 1 entry. 177 tests pass.
|
@copilot please run the
|
Replace the git config --global approach with process-scoped GIT_CONFIG_COUNT/KEY/VALUE env var injection so that all subsequent git commands in the same process inherit the safe.directory trust without writing to ~/.gitconfig. This eliminates the persistent global git config side effect (no file writes) and removes the failure mode that was previously only surfaced at debug level. The env var approach is supported by git 2.31+ and carries over to all git calls made by generateGitPatch and generateGitBundle in the same process. Update tests to verify env var injection instead of checking the global git config file, and fix the finally-block cleanup to delete all GIT_CONFIG_KEY/VALUE entries added during the test before restoring originals. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage — §28282332784
Rationale: Production fix for git ownership errors in safe_outputs bundle-push path. CI mostly passing; one automated — clear it then expedite merge.
|
…ix-bundle-push-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch was 53 commits behind main — merged and pushed now. |
🔍 PR Triage — §28395315609
Score breakdown: impact 35 + urgency 28 + quality 14
|
|
@copilot please run the Context:
|
…ix-bundle-push-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch refreshed from main (was 2 commits behind). The |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Review SummaryApplied 📋 Issues found (4 comments)
All issues are non-blocking for correctness; the implementation itself is sound. @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — no blocking correctness issues, but 4 test-coverage gaps worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Test coverage asymmetry: Deduplication test exists only for
createPullRequestHandler;pushToPullRequestBranchHandlerlacks a matching guard. - Untested contract: The NaN guard for malformed
GIT_CONFIG_COUNTis well-documented in comments but has no test — the very scenario the guard exists for. - Missing motivating test case: Per-handler calls exist for
repoCwd ≠ GITHUB_WORKSPACE, but tests only exercise theGITHUB_WORKSPACEfallback path. - Test boilerplate duplication: The save-restore setup pattern is copy-pasted across all three new tests, making future maintenance error-prone.
Positive Highlights
- ✅ Correct root-cause fix: process-scoped env vars instead of persistent
~/.gitconfigmutations — clean and side-effect-free. - ✅ Idempotent design with deduplication is sound and
execGitSynccorrectly spreadsprocess.envinto the child env viasafeEnv. - ✅ Defense-in-depth layering: injection at both
createHandlerslevel and per-handler level provides proper coverage for per-request checkout paths. - ✅ Good JSDoc on
ensureSafeDirectoryTrust— the function contract is clearly stated.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 60.8 AIC · ⌖ 9.29 AIC · ⊞ 6.6K
Comment /matt to run again
|
@copilot this branch has merge conflicts. Please run
|
Branch is already current with main — the merge at |
|
@copilot please run the Generated by PR Sous Chef
|
…ix-bundle-push-issue # Conflicts: # actions/setup/js/safe_outputs_handlers.test.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ix-bundle-push-issue (resolve test conflict) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch merged and updated. Resolved a conflict in |
push_to_pull_request_branch/create_pull_requestwithpatch-format: bundlecould fail during branch pinning with git "detected dubious ownership in repository" when safe-outputs ran in a different user/HOME context than the container. This caused real production drops where generated changesets were not pushed.Root-cause containment in branch-pin path
ensureSafeDirectoryTrust(gitCwd, server)inactions/setup/js/safe_outputs_handlers.cjs.createHandlers(covering all current and future handlers automatically) and also at each bundle-capable handler call site to trust per-request checkout paths that may differ fromGITHUB_WORKSPACE:createPullRequestHandlerpushToPullRequestBranchHandlerProcess-scoped trust via GIT_CONFIG env vars (no persistent side effects)
~/.gitconfig, the helper injectssafe.directoryusingGIT_CONFIG_COUNT/GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_Nenvironment variables (supported since git 2.31).generateGitPatchandgenerateGitBundle) inherit the trust automatically viaprocess.env.~/.gitconfig— no persistent global git config side effects.gitCwdis already present in the env-var config;parseInt(...) || 0guard prevents NaN corruption of the config chain ifGIT_CONFIG_COUNTis pre-set to a non-numeric value.Targeted regression coverage
GITHUB_WORKSPACEintoGIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_Nprocess env vars before branch pinning.safe.directoryentries in the env-var chain.finally-block cleanup removes allGIT_CONFIG_KEY/VALUE_Nentries added during the test before restoring originals.https://github.com/github/gh-aw/actions/runs/28339613080