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 9 commits into
Conversation
Co-authored-by: pelikhan <[email protected]>
|
🧠 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.
| .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.
| // 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.
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.
| } | ||
| 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.
| 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.
| 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;
}
});
|
@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 <[email protected]>
🤖 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.
|
… of GIT_CONFIG_COUNT chain Co-authored-by: Copilot <[email protected]> Co-authored-by: pelikhan <[email protected]>
Security, GitHub Actions best-practices, and supply-chain review of the Secret leaks — none found Weaknesses identified and addressed One actionable weakness found and fixed in
Remaining observations (no code changes needed)
|
|
@copilot please run the
|
🤖 PR Triage — Run §28307424127
Update from prior run (~23h old): CI clean (1/1 pass). Bot
|
|
@copilot please run the
|
Local validation is green: |
|
@copilot please run the
|
PR Triage — Run §28315307719
Production fix for git
|
|
@copilot please run the
|
|
``
|
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.rev-parsebranch pinning in both bundle-capable flows: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.debug-only error path.Targeted regression coverage
safe_outputs_handlers.test.cjsto verify each handler injectsGITHUB_WORKSPACEintoGIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_Nprocess env vars before branch pinning.finally-block cleanup to remove allGIT_CONFIG_KEY/VALUE_Nentries added during the test before restoring originals.pr-sous-chef run 28311797805
pr-sous-chef run https://github.com/github/gh-aw/actions/runs/28324508728