Skip to content

safe_outputs: set git safe.directory via process-scoped env vars before bundle branch pinning #41830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 7 commits into
base: main
Choose a base branch
from
+99 −0
Open

safe_outputs: set git safe.directory via process-scoped env vars before bundle branch pinning #41830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28e9b40
Initial plan
Copilot Jun 27, 2026
6ab65a8
Apply remaining changes
Copilot Jun 27, 2026
9c75a57
safe_outputs: use GIT_CONFIG_COUNT env vars for safe.directory trust
Copilot Jun 27, 2026
40ab4f7
Merge remote-tracking branch 'origin/main' into copilot/deep-report-f…
Copilot Jun 27, 2026
d9f4fa2
Merge remote-tracking branch 'origin/main' into copilot/deep-report-f…
Copilot Jun 27, 2026
ea090f1
Merge remote-tracking branch 'origin/main' into copilot/deep-report-f…
Copilot Jun 27, 2026
49d5020
fix(safe_outputs): guard parseInt with || 0 to prevent NaN corruption…
Copilot Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions actions/setup/js/safe_outputs_handlers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,39 @@ function resolvePatchWorkspacePath(workspacePath) {
return { success: true, absolutePath: resolved };
}

/**
* Ensure the current git checkout path is trusted in this process context.
* Injects safe.directory via GIT_CONFIG_COUNT/KEY/VALUE env vars so that all
* subsequent git commands in this process inherit the trust without writing to
* ~/.gitconfig (no persistent global git config side effects).
*
* GIT_CONFIG_COUNT/KEY/VALUE is supported since git 2.31.
*
* @param {string} gitCwd
* @param {{ debug: (message: string) => void }} server
* @returns {void}
*/
function ensureSafeDirectoryTrust(gitCwd, server) {
if (!gitCwd) return;

// Check if gitCwd is already present in the injected env-var config to avoid
// duplicate entries when the handler is called more than once in the same process.
// The `|| 0` guard converts NaN (from a malformed pre-existing GIT_CONFIG_COUNT) to 0,
// preventing GIT_CONFIG_KEY_NaN/VALUE_NaN entries that would corrupt the env-var config chain.
const existingCount = parseInt(process.env.GIT_CONFIG_COUNT || "0", 10) || 0;
for (let i = 0; i < existingCount; i++) {
if (process.env[`GIT_CONFIG_KEY_${i}`] === "safe.directory" && process.env[`GIT_CONFIG_VALUE_${i}`] === gitCwd) {
return;
}
}

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const idx = existingCount;
process.env.GIT_CONFIG_COUNT = String(existingCount + 1);
process.env[`GIT_CONFIG_KEY_${idx}`] = "safe.directory";
process.env[`GIT_CONFIG_VALUE_${idx}`] = gitCwd;
server.debug(`Configured git safe.directory for bridge context: ${gitCwd}`);
}

/**
* Create handlers for safe output tools
* @param {Object} server - The MCP server instance for logging
Expand Down Expand Up @@ -752,6 +785,7 @@ function createHandlers(server, appendSafeOutput, config = {}) {
// 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);

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

let pinnedSha;
try {
pinnedSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: gitCwd })
Expand Down Expand Up @@ -1196,6 +1230,7 @@ function createHandlers(server, appendSafeOutput, config = {}) {
// This prevents TOCTOU races where the agent flips the ref between patch and bundle
// generation, causing the two to represent different commit sets.
const pushGitCwd = repoCwd || process.env.GITHUB_WORKSPACE || process.cwd();
ensureSafeDirectoryTrust(pushGitCwd, server);
let pushPinnedSha;
try {
pushPinnedSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: pushGitCwd })
Expand Down
64 changes: 64 additions & 0 deletions actions/setup/js/safe_outputs_handlers.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,39 @@ describe("safe_outputs_handlers", () => {
expect(mockAppendSafeOutput).not.toHaveBeenCalled();
});

it("injects GITHUB_WORKSPACE into GIT_CONFIG env vars for safe.directory before branch pinning", async () => {
const prevCount = process.env.GIT_CONFIG_COUNT;
const prevKeys = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^GIT_CONFIG_(KEY|VALUE)_\d+$/.test(k)));
// Clear any pre-existing GIT_CONFIG_COUNT so the injection starts at index 0.
delete process.env.GIT_CONFIG_COUNT;
for (const key of Object.keys(prevKeys)) delete process.env[key];
try {
await handlers.createPullRequestHandler({
branch: "feature-branch",
title: "Test PR",
body: "Test description",
});

const count = parseInt(process.env.GIT_CONFIG_COUNT || "0", 10);
const injected = [];
for (let i = 0; i < count; i++) {
if (process.env[`GIT_CONFIG_KEY_${i}`] === "safe.directory") {
injected.push(process.env[`GIT_CONFIG_VALUE_${i}`]);
}
}
expect(injected).toContain(testWorkspaceDir);
} finally {
// Restore original env state.
if (prevCount === undefined) delete process.env.GIT_CONFIG_COUNT;
else process.env.GIT_CONFIG_COUNT = prevCount;
// Remove any GIT_CONFIG_KEY/VALUE vars added during the test, then restore originals.
for (const key of Object.keys(process.env).filter(k => /^GIT_CONFIG_(KEY|VALUE)_\d+$/.test(k))) {
delete process.env[key];
}
for (const [key, value] of Object.entries(prevKeys)) process.env[key] = value;
}
});

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
});


it("should include helpful details in error response", async () => {
const args = {
branch: "test-branch",
Expand Down Expand Up @@ -1208,6 +1241,37 @@ describe("safe_outputs_handlers", () => {
expect(mockAppendSafeOutput).not.toHaveBeenCalled();
});

it("injects GITHUB_WORKSPACE into GIT_CONFIG env vars for safe.directory before push branch pinning", async () => {
const prevCount = process.env.GIT_CONFIG_COUNT;
const prevKeys = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^GIT_CONFIG_(KEY|VALUE)_\d+$/.test(k)));
// Clear any pre-existing GIT_CONFIG_COUNT so the injection starts at index 0.
delete process.env.GIT_CONFIG_COUNT;
for (const key of Object.keys(prevKeys)) delete process.env[key];
try {
await handlers.pushToPullRequestBranchHandler({
branch: "feature-branch",
});

const count = parseInt(process.env.GIT_CONFIG_COUNT || "0", 10);
const injected = [];
for (let i = 0; i < count; i++) {
if (process.env[`GIT_CONFIG_KEY_${i}`] === "safe.directory") {
injected.push(process.env[`GIT_CONFIG_VALUE_${i}`]);
}
}
expect(injected).toContain(testWorkspaceDir);
} finally {
// Restore original env state.
if (prevCount === undefined) delete process.env.GIT_CONFIG_COUNT;
else process.env.GIT_CONFIG_COUNT = prevCount;
// Remove any GIT_CONFIG_KEY/VALUE vars added during the test, then restore originals.
for (const key of Object.keys(process.env).filter(k => /^GIT_CONFIG_(KEY|VALUE)_\d+$/.test(k))) {
delete process.env[key];
}
for (const [key, value] of Object.entries(prevKeys)) process.env[key] = value;
}
});

it("should require explicit pull_request_number when push_to_pull_request_branch target is '*'", async () => {
const wildcardHandlers = createHandlers(mockServer, mockAppendSafeOutput, {
push_to_pull_request_branch: {
Expand Down
Loading