From a5640f491c7c54c84373f1355cc4d840b28df01d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:13:28 +0000 Subject: [PATCH 1/7] Initial plan From 95b181b029416bc1af16052946a8e77bd429b9ff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:40:17 +0000 Subject: [PATCH 2/7] fix: clear all git config scopes before overriding extraheader to prevent duplicate Authorization headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the duplicate Authorization header issue in fork-backed create_pull_request: Root cause: getExtraheaderValues reads from ALL git config scopes (--get-all), but overridePersistedExtraheader wrote only to the local scope via --replace-all without explicit scope. actions/checkout writes to the global scope, so after override both local (fork CI token) and global (checkout token) were active simultaneously, causing duplicate Authorization headers and HTTP 400 errors. After restore, both global (original) and local (restored original) had the same value, causing getExtraheaderValues to return 2 values on the next read, then 3, 4 — the accumulation bug observed in the issue log. Fix: - Add unsetExtraheaderAllScopes() helper that silently clears from --global and --local scopes before any write operation - overridePersistedExtraheader calls unsetExtraheaderAllScopes before writing fork token to --local scope, ensuring exactly one Authorization source is active - restorePersistedExtraheader calls unsetExtraheaderAllScopes before restoring to --local scope, preventing accumulation across retries - Add regression test that verifies cardinality stays at 1 across override/restore cycles Closes #48952 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/git_auth_helpers.cjs | 87 +++++++--- actions/setup/js/git_auth_helpers.test.cjs | 188 ++++++++++++++++----- 2 files changed, 206 insertions(+), 69 deletions(-) diff --git a/actions/setup/js/git_auth_helpers.cjs b/actions/setup/js/git_auth_helpers.cjs index 0208fd81e1d..5b0d77b3aa4 100644 --- a/actions/setup/js/git_auth_helpers.cjs +++ b/actions/setup/js/git_auth_helpers.cjs @@ -16,6 +16,35 @@ function normalizeServerUrl(serverUrl) { return serverUrl.replace(/\/+$/, ""); } +/** + * Unset the given git config key from all writable scopes (global and local). + * Errors are silently ignored because the key may be absent in some scopes + * or a scope may not be accessible (e.g. read-only system scope on CI runners). + * + * This is necessary because `actions/checkout` typically writes credentials to + * the global scope, and a plain `--replace-all` without an explicit scope flag + * only removes values from the local scope, leaving the global value in place + * and causing duplicate Authorization header HTTP 400 errors. + * + * @param {string} key - Full git config key (e.g. "http.https://github.com/.extraheader") + * @param {string} [cwd] - Optional working directory for local-scope operations + * @returns {Promise} + */ +async function unsetExtraheaderAllScopes(key, cwd) { + // Clear both the global scope (typically written by actions/checkout) and the + // local scope (written by this helper). The system scope is intentionally + // skipped because it is usually read-only in CI environments. + const scopes = ["--global", "--local"]; + for (const scope of scopes) { + try { + const opts = { silent: true, ignoreReturnCode: true, ...(cwd ? { cwd } : {}) }; + await exec.getExecOutput("git", ["config", scope, "--unset-all", key], opts); + } catch { + // Ignore – scope may not be accessible or key may be absent. + } + } +} + /** * Get all configured values for http./.extraheader. * Throws if `exec.getExecOutput` itself throws (e.g. git not available). @@ -75,10 +104,18 @@ async function overridePersistedExtraheader(serverUrl, token, cwd) { } core.info(`git_auth_helpers: overriding http.${normalizedUrl}/.extraheader with CI trigger token`); const tokenBase64 = Buffer.from(`x-access-token:${token.trim()}`).toString("base64"); + const authHeader = `Authorization: basic ${tokenBase64}`; + + // Clear from ALL writable scopes before writing our token to prevent duplicate + // Authorization headers. actions/checkout writes to the global scope; without + // this step a plain --replace-all only writes to the local scope, leaving the + // global value in place and causing duplicate-header HTTP 400 errors. + await unsetExtraheaderAllScopes(`http.${normalizedUrl}/.extraheader`, cwd); + if (cwd) { - await exec.exec("git", ["config", "--replace-all", `http.${normalizedUrl}/.extraheader`, `Authorization: basic ${tokenBase64}`], { cwd }); + await exec.exec("git", ["config", "--local", "--replace-all", `http.${normalizedUrl}/.extraheader`, authHeader], { cwd }); } else { - await exec.exec("git", ["config", "--replace-all", `http.${normalizedUrl}/.extraheader`, `Authorization: basic ${tokenBase64}`]); + await exec.exec("git", ["config", "--local", "--replace-all", `http.${normalizedUrl}/.extraheader`, authHeader]); } core.info(`git_auth_helpers: extraheader override applied`); return previousValues; @@ -94,48 +131,43 @@ async function overridePersistedExtraheader(serverUrl, token, cwd) { */ async function restorePersistedExtraheader(serverUrl, previousValues, cwd) { const key = `http.${normalizeServerUrl(serverUrl)}/.extraheader`; + + // Clear from ALL writable scopes first. This removes the override written by + // overridePersistedExtraheader and prevents values from accumulating across + // multiple override/restore cycles (which happens when global scope values + // are not cleared and get re-read by getExtraheaderValues on each attempt). + await unsetExtraheaderAllScopes(key, cwd); + if (!previousValues || previousValues.length === 0) { - core.info(`git_auth_helpers: no previous extraheader values — unsetting ${key}`); - try { - if (cwd) { - await exec.exec("git", ["config", "--unset-all", key], { cwd }); - } else { - await exec.exec("git", ["config", "--unset-all", key]); - } - } catch { - // Nothing to restore/unset. - } + core.info(`git_auth_helpers: no previous extraheader values — ${key} cleared`); return; } core.info(`git_auth_helpers: restoring ${previousValues.length} previous extraheader value(s) for ${key}`); - // --replace-all removes any existing values for the key (including the CI-token - // entry) and writes previousValues[0]. Subsequent --add calls stack the remaining - // previous values onto the same key without removing any already written. - // If any --add call fails after --replace-all has already run, git config is left + // Restore to the local scope. Even when the original values were written to + // the global scope by actions/checkout, restoring to local is functionally + // equivalent: with the global scope cleared the restored local value is the + // only source, so no duplicate headers can arise on subsequent retries. + // If any call fails after --replace-all has already run, git config is left // in a partially-restored state. In that case we log a warning and attempt a - // best-effort cleanup by unsetting the key entirely, then re-throw so the caller - // is aware that restoration failed. + // best-effort cleanup, then re-throw so the caller is aware that restoration + // failed. try { if (cwd) { - await exec.exec("git", ["config", "--replace-all", key, previousValues[0]], { cwd }); + await exec.exec("git", ["config", "--local", "--replace-all", key, previousValues[0]], { cwd }); for (const value of previousValues.slice(1)) { - await exec.exec("git", ["config", "--add", key, value], { cwd }); + await exec.exec("git", ["config", "--local", "--add", key, value], { cwd }); } } else { - await exec.exec("git", ["config", "--replace-all", key, previousValues[0]]); + await exec.exec("git", ["config", "--local", "--replace-all", key, previousValues[0]]); for (const value of previousValues.slice(1)) { - await exec.exec("git", ["config", "--add", key, value]); + await exec.exec("git", ["config", "--local", "--add", key, value]); } } } catch (err) { core.warning(`git_auth_helpers: partial extraheader restore for ${key} — attempting cleanup: ${getErrorMessage(err)}`); try { - if (cwd) { - await exec.exec("git", ["config", "--unset-all", key], { cwd }); - } else { - await exec.exec("git", ["config", "--unset-all", key]); - } + await unsetExtraheaderAllScopes(key, cwd); } catch { // Best-effort only; ignore secondary failure. } @@ -180,5 +212,6 @@ module.exports = { checkoutHasPersistedExtraheader, overridePersistedExtraheader, restorePersistedExtraheader, + unsetExtraheaderAllScopes, withGitHubHostToken, }; diff --git a/actions/setup/js/git_auth_helpers.test.cjs b/actions/setup/js/git_auth_helpers.test.cjs index 9faafcd4d8c..96c9db023c5 100644 --- a/actions/setup/js/git_auth_helpers.test.cjs +++ b/actions/setup/js/git_auth_helpers.test.cjs @@ -6,6 +6,7 @@ describe("git_auth_helpers.cjs", () => { let checkoutHasPersistedExtraheader; let overridePersistedExtraheader; let restorePersistedExtraheader; + let unsetExtraheaderAllScopes; let withGitHubHostToken; const SERVER_URL = "https://github.com"; @@ -26,7 +27,7 @@ describe("git_auth_helpers.cjs", () => { global.exec = mockExec; delete require.cache[require.resolve("./git_auth_helpers.cjs")]; - ({ checkoutHasPersistedExtraheader, overridePersistedExtraheader, restorePersistedExtraheader, withGitHubHostToken } = require("./git_auth_helpers.cjs")); + ({ checkoutHasPersistedExtraheader, overridePersistedExtraheader, restorePersistedExtraheader, unsetExtraheaderAllScopes, withGitHubHostToken } = require("./git_auth_helpers.cjs")); }); afterEach(() => { @@ -66,21 +67,63 @@ describe("git_auth_helpers.cjs", () => { }); }); + // ────────────────────────────────────────────────────── + // unsetExtraheaderAllScopes + // ────────────────────────────────────────────────────── + + describe("unsetExtraheaderAllScopes", () => { + it("should unset from global and local scopes", async () => { + await unsetExtraheaderAllScopes(EXTRAHEADER_KEY); + + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--global", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--local", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + }); + + it("should pass cwd to both scope unset calls", async () => { + const cwd = "/some/repo"; + await unsetExtraheaderAllScopes(EXTRAHEADER_KEY, cwd); + + for (const call of mockExec.getExecOutput.mock.calls) { + expect(call[2]).toMatchObject({ cwd }); + } + }); + + it("should not throw when any scope unset fails", async () => { + mockExec.getExecOutput.mockRejectedValue(new Error("scope not writable")); + + await expect(unsetExtraheaderAllScopes(EXTRAHEADER_KEY)).resolves.toBeUndefined(); + }); + }); + // ────────────────────────────────────────────────────── // overridePersistedExtraheader // ────────────────────────────────────────────────────── describe("overridePersistedExtraheader", () => { - it("should replace the extraheader with the CI token", async () => { + it("should clear all scopes before writing the new token", async () => { + const token = "ghp_test_token"; + + await overridePersistedExtraheader(SERVER_URL, token); + + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--global", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--local", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + }); + + it("should replace the extraheader with the CI token using --local scope", async () => { const token = "ghp_test_token"; + const expectedHeader = `Authorization: basic ${Buffer.from(`x-access-token:${token}`).toString("base64")}`; await overridePersistedExtraheader(SERVER_URL, token); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--replace-all", EXTRAHEADER_KEY, `Authorization: basic ${Buffer.from(`x-access-token:${token}`).toString("base64")}`]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--local", "--replace-all", EXTRAHEADER_KEY, expectedHeader]); }); it("should return empty array when no previous extraheader exists", async () => { - mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + // Only the --get-all read returns empty; unset-all calls use ignoreReturnCode + if (args[1] === "--get-all") return { exitCode: 1, stdout: "", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; // key not found + }); const previous = await overridePersistedExtraheader(SERVER_URL, "ghp_test_token"); @@ -89,7 +132,10 @@ describe("git_auth_helpers.cjs", () => { it("should return previous extraheader values when one exists", async () => { const prevHeader = `Authorization: basic ${Buffer.from("x-access-token:old_token").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: prevHeader + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: prevHeader + "\n", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); const previous = await overridePersistedExtraheader(SERVER_URL, "ghp_new_token"); @@ -99,7 +145,10 @@ describe("git_auth_helpers.cjs", () => { it("should return multiple previous values when multi-value extraheader exists", async () => { const header1 = `Authorization: basic ${Buffer.from("x-access-token:tok1").toString("base64")}`; const header2 = `Authorization: basic ${Buffer.from("x-access-token:tok2").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: `${header1}\n${header2}\n`, stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: `${header1}\n${header2}\n`, stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); const previous = await overridePersistedExtraheader(SERVER_URL, "ghp_new_token"); @@ -107,14 +156,17 @@ describe("git_auth_helpers.cjs", () => { }); it("should warn and fall back to empty array when reading previous values fails", async () => { - mockExec.getExecOutput.mockRejectedValue(new Error("git read error")); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") throw new Error("git read error"); + return { exitCode: 5, stdout: "", stderr: "" }; + }); const previous = await overridePersistedExtraheader(SERVER_URL, "ghp_test_token"); expect(previous).toEqual([]); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("could not read existing extraheader")); // Override should still proceed despite read failure - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--replace-all", EXTRAHEADER_KEY, expect.any(String)]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--local", "--replace-all", EXTRAHEADER_KEY, expect.any(String)]); }); it("should trim the token before base64-encoding", async () => { @@ -123,12 +175,15 @@ describe("git_auth_helpers.cjs", () => { await overridePersistedExtraheader(SERVER_URL, token); const expected = `Authorization: basic ${Buffer.from("x-access-token:ghp_padded_token").toString("base64")}`; - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--replace-all", EXTRAHEADER_KEY, expected]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--local", "--replace-all", EXTRAHEADER_KEY, expected]); }); it("should log the number of existing values before overriding", async () => { const header = `Authorization: basic ${Buffer.from("x-access-token:tok").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: header + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: header + "\n", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); await overridePersistedExtraheader(SERVER_URL, "new_token"); @@ -145,7 +200,7 @@ describe("git_auth_helpers.cjs", () => { // Using the literal normalized key makes the assertion explicit. const normalizedKey = "http.https://github.com/.extraheader"; expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--get-all", normalizedKey], expect.anything()); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--replace-all", normalizedKey, expect.any(String)]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--local", "--replace-all", normalizedKey, expect.any(String)]); }); }); @@ -154,68 +209,71 @@ describe("git_auth_helpers.cjs", () => { // ────────────────────────────────────────────────────── describe("restorePersistedExtraheader", () => { - it("should unset the extraheader key when previousValues is empty", async () => { + it("should clear all scopes when previousValues is empty", async () => { await restorePersistedExtraheader(SERVER_URL, []); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--unset-all", EXTRAHEADER_KEY]); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--global", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--local", "--unset-all", EXTRAHEADER_KEY], expect.objectContaining({ ignoreReturnCode: true })); + // Should not call exec.exec (no values to write back) + expect(mockExec.exec).not.toHaveBeenCalled(); }); - it("should not throw when unset-all fails (key already absent)", async () => { - mockExec.exec.mockRejectedValue(new Error("key not found")); + it("should not throw when clearing scopes fails (key already absent)", async () => { + mockExec.getExecOutput.mockRejectedValue(new Error("key not found")); await expect(restorePersistedExtraheader(SERVER_URL, [])).resolves.toBeUndefined(); }); - it("should use --replace-all to restore a single previous value", async () => { + it("should use --local --replace-all to restore a single previous value", async () => { const prevHeader = `Authorization: basic ${Buffer.from("x-access-token:old").toString("base64")}`; await restorePersistedExtraheader(SERVER_URL, [prevHeader]); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--replace-all", EXTRAHEADER_KEY, prevHeader]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--local", "--replace-all", EXTRAHEADER_KEY, prevHeader]); expect(mockExec.exec).not.toHaveBeenCalledWith("git", expect.arrayContaining(["--add"])); }); - it("should restore multiple values using --replace-all then --add", async () => { + it("should restore multiple values using --local --replace-all then --local --add", async () => { const header1 = `Authorization: basic ${Buffer.from("x-access-token:tok1").toString("base64")}`; const header2 = `Authorization: basic ${Buffer.from("x-access-token:tok2").toString("base64")}`; await restorePersistedExtraheader(SERVER_URL, [header1, header2]); const calls = mockExec.exec.mock.calls; - const replaceCall = calls.find(c => c[1][1] === "--replace-all"); - const addCall = calls.find(c => c[1][1] === "--add"); + const replaceCall = calls.find(c => c[1][1] === "--local" && c[1][2] === "--replace-all"); + const addCall = calls.find(c => c[1][1] === "--local" && c[1][2] === "--add"); expect(replaceCall).toBeDefined(); - expect(replaceCall[1][3]).toBe(header1); + expect(replaceCall[1][4]).toBe(header1); expect(addCall).toBeDefined(); - expect(addCall[1][3]).toBe(header2); + expect(addCall[1][4]).toBe(header2); // --replace-all must come before --add expect(calls.indexOf(replaceCall)).toBeLessThan(calls.indexOf(addCall)); }); - it("should attempt cleanup and re-throw when --add fails mid-restore", async () => { + it("should attempt cleanup via unsetExtraheaderAllScopes and re-throw when --add fails mid-restore", async () => { const header1 = `Authorization: basic ${Buffer.from("x-access-token:tok1").toString("base64")}`; const header2 = `Authorization: basic ${Buffer.from("x-access-token:tok2").toString("base64")}`; const addError = new Error("git config --add failed"); mockExec.exec.mockImplementation(async (_cmd, args) => { - if (args[1] === "--add") throw addError; + if (args[1] === "--local" && args[2] === "--add") throw addError; return 0; }); await expect(restorePersistedExtraheader(SERVER_URL, [header1, header2])).rejects.toThrow(addError); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("partial extraheader restore")); - // Cleanup --unset-all should have been attempted - const calls = mockExec.exec.mock.calls; - const unsetCall = calls.find(c => c[1][1] === "--unset-all"); - expect(unsetCall).toBeDefined(); + // Cleanup should use unsetExtraheaderAllScopes (getExecOutput calls for --unset-all) + const unsetCalls = mockExec.getExecOutput.mock.calls.filter(c => c[1][2] === "--unset-all"); + expect(unsetCalls.length).toBeGreaterThan(0); }); it("should strip trailing slash from server URL for the config key", async () => { await restorePersistedExtraheader("https://github.com/", []); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["config", "--unset-all", EXTRAHEADER_KEY]); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--global", "--unset-all", EXTRAHEADER_KEY], expect.anything()); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["config", "--local", "--unset-all", EXTRAHEADER_KEY], expect.anything()); }); it("should not throw when previousValues is null/undefined (treated as empty)", async () => { @@ -261,28 +319,34 @@ describe("git_auth_helpers.cjs", () => { await withGitHubHostToken(token, async () => { // Inside callback the override should already be applied - const overrideCall = execCalls.find(a => a[1] === "--replace-all"); + const overrideCall = execCalls.find(a => a[2] === "--replace-all"); expect(overrideCall).toBeDefined(); - expect(overrideCall[3]).toBe(expectedHeader); + expect(overrideCall[4]).toBe(expectedHeader); }); }); it("should restore the previous extraheader after the callback completes", async () => { const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); await withGitHubHostToken("fork-token", async () => {}); // After callback, the last --replace-all should restore the upstream header - const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][1] === "--replace-all"); + const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][2] === "--replace-all"); expect(replaceAllCalls.length).toBeGreaterThanOrEqual(2); const restoreCall = replaceAllCalls[replaceAllCalls.length - 1]; - expect(restoreCall[1][3]).toBe(upstreamHeader); + expect(restoreCall[1][4]).toBe(upstreamHeader); }); it("should restore extraheader even when the callback throws", async () => { const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); const callbackError = new Error("push failed"); await expect( @@ -292,20 +356,27 @@ describe("git_auth_helpers.cjs", () => { ).rejects.toThrow(callbackError); // Restore must still run after the callback throws — the last --replace-all is the restore - const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][1] === "--replace-all"); + const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][2] === "--replace-all"); expect(replaceAllCalls.length).toBeGreaterThanOrEqual(2); const restoreCall = replaceAllCalls[replaceAllCalls.length - 1]; - expect(restoreCall[1][3]).toBe(upstreamHeader); + expect(restoreCall[1][4]).toBe(upstreamHeader); }); - it("should unset extraheader after callback when no previous value existed", async () => { + it("should clear all scopes after callback when no previous value existed", async () => { mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }); await withGitHubHostToken("fork-token", async () => {}); - const unsetCall = mockExec.exec.mock.calls.find(c => c[1][1] === "--unset-all"); - expect(unsetCall).toBeDefined(); - expect(unsetCall[1][2]).toBe(EXTRAHEADER_KEY); + // Restore should call unsetExtraheaderAllScopes (global + local unset-all) + const unsetCalls = mockExec.getExecOutput.mock.calls.filter(c => c[1][2] === "--unset-all"); + expect(unsetCalls.length).toBeGreaterThanOrEqual(2); + const keys = unsetCalls.map(c => c[1][3]); + expect(keys).toContain(EXTRAHEADER_KEY); + // Should NOT call exec.exec for restore (no values to write back) + const restoreExecCalls = mockExec.exec.mock.calls.filter(c => c[1][2] === "--replace-all"); + // First --replace-all is the override; no second one since previousValues is [] + // Check only one --replace-all call (the override itself) + expect(restoreExecCalls.length).toBe(1); }); it("should return the callback's return value", async () => { @@ -316,7 +387,10 @@ describe("git_auth_helpers.cjs", () => { it("should pass cwd to overridePersistedExtraheader and restorePersistedExtraheader", async () => { const cwd = "/some/repo"; const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }; + return { exitCode: 5, stdout: "", stderr: "" }; + }); await withGitHubHostToken("fork-token", async () => {}, cwd); @@ -328,5 +402,35 @@ describe("git_auth_helpers.cjs", () => { expect(call[2]).toMatchObject({ cwd }); } }); + + it("should not accumulate extraheader values across multiple override/restore cycles", async () => { + // Regression test for: restore loop reporting 'read 1', 'read 2', 'read 3', ... + // Root cause: getExtraheaderValues reads from ALL scopes, but --replace-all without + // explicit scope only writes to local scope, leaving the global scope value intact. + // After restore, both global and local have the value → 2 values on next read → etc. + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + let capturedPreviousValueCounts = []; + + // Simulate the initial state: git config --get-all returns 1 value (global scope) + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[1] === "--get-all") return { exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }; + // unset-all calls return success (exit 0) + return { exitCode: 0, stdout: "", stderr: "" }; + }); + + // Intercept the info log to capture the "read N existing" count + mockCore.info.mockImplementation(msg => { + const m = msg.match(/read (\d+) existing extraheader value/); + if (m) capturedPreviousValueCounts.push(Number(m[1])); + }); + + // Run two override/restore cycles (simulating a retry scenario) + for (let i = 0; i < 2; i++) { + await withGitHubHostToken("fork-token", async () => {}); + } + + // Both cycles should read exactly 1 value — no accumulation + expect(capturedPreviousValueCounts).toEqual([1, 1]); + }); }); }); From b1e059ed0f70f833db900df443df6b19c0e937c4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:05:58 +0000 Subject: [PATCH 3/7] chore: plan - address review feedback on error handling and regression test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 6f24708a24e..6fb19019416 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -35,6 +35,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/evals.md` - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` +- `.github/aw/github-mcp-server-pagination.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` - `.github/aw/linter-workflows.md` @@ -64,10 +65,12 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/subagents.md` - `.github/aw/syntax-agentic.md` - `.github/aw/syntax-core.md` +- `.github/aw/syntax-engine.md` - `.github/aw/syntax-tools-imports.md` - `.github/aw/syntax.md` - `.github/aw/test-coverage.md` - `.github/aw/test-expression.md` +- `.github/aw/token-optimization-caching-budgets.md` - `.github/aw/token-optimization.md` - `.github/aw/triggers.md` - `.github/aw/update-agentic-workflow.md` From 2f86016c993aa8e05673400fa9378037315749f0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:12:46 +0000 Subject: [PATCH 4/7] fix: distinguish git-absent exit code 5 from real failures; rewrite regression test with stateful mock Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/git_auth_helpers.cjs | 12 +-- actions/setup/js/git_auth_helpers.test.cjs | 86 +++++++++++++++++----- 2 files changed, 75 insertions(+), 23 deletions(-) diff --git a/actions/setup/js/git_auth_helpers.cjs b/actions/setup/js/git_auth_helpers.cjs index 5b0d77b3aa4..81c5585eaf3 100644 --- a/actions/setup/js/git_auth_helpers.cjs +++ b/actions/setup/js/git_auth_helpers.cjs @@ -36,11 +36,13 @@ async function unsetExtraheaderAllScopes(key, cwd) { // skipped because it is usually read-only in CI environments. const scopes = ["--global", "--local"]; for (const scope of scopes) { - try { - const opts = { silent: true, ignoreReturnCode: true, ...(cwd ? { cwd } : {}) }; - await exec.getExecOutput("git", ["config", scope, "--unset-all", key], opts); - } catch { - // Ignore – scope may not be accessible or key may be absent. + const opts = { silent: true, ignoreReturnCode: true, ...(cwd ? { cwd } : {}) }; + const result = await exec.getExecOutput("git", ["config", scope, "--unset-all", key], opts); + // Exit code 5 means the key was absent in this scope — expected and safe to ignore. + // Any other non-zero exit code indicates a real failure (e.g., permission denied, + // write lock). Throw so the caller knows the credential may still be effective. + if (result.exitCode !== 0 && result.exitCode !== 5) { + throw new Error(`git config ${scope} --unset-all ${key} failed (exit ${result.exitCode}): ${result.stderr.trim()}`); } } } diff --git a/actions/setup/js/git_auth_helpers.test.cjs b/actions/setup/js/git_auth_helpers.test.cjs index 96c9db023c5..8303f146bfe 100644 --- a/actions/setup/js/git_auth_helpers.test.cjs +++ b/actions/setup/js/git_auth_helpers.test.cjs @@ -20,7 +20,9 @@ describe("git_auth_helpers.cjs", () => { mockExec = { exec: vi.fn().mockResolvedValue(0), - getExecOutput: vi.fn().mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }), + // Exit code 5 = key absent (the most common default state). + // This is safe for both --get-all (returns []) and --unset-all (key not found, ignored). + getExecOutput: vi.fn().mockResolvedValue({ exitCode: 5, stdout: "", stderr: "" }), }; global.core = mockCore; @@ -88,11 +90,18 @@ describe("git_auth_helpers.cjs", () => { } }); - it("should not throw when any scope unset fails", async () => { - mockExec.getExecOutput.mockRejectedValue(new Error("scope not writable")); + it("should not throw when key is absent (exit code 5)", async () => { + mockExec.getExecOutput.mockResolvedValue({ exitCode: 5, stdout: "", stderr: "" }); await expect(unsetExtraheaderAllScopes(EXTRAHEADER_KEY)).resolves.toBeUndefined(); }); + + it("should throw when a scope unset fails with an unexpected non-zero exit code", async () => { + // Exit code 4 = file write error (permission denied, config lock, etc.) + mockExec.getExecOutput.mockResolvedValue({ exitCode: 4, stdout: "", stderr: "error: could not lock config file" }); + + await expect(unsetExtraheaderAllScopes(EXTRAHEADER_KEY)).rejects.toThrow(/--unset-all.*failed \(exit 4\)/); + }); }); // ────────────────────────────────────────────────────── @@ -191,7 +200,9 @@ describe("git_auth_helpers.cjs", () => { }); it("should strip trailing slash from server URL when reading previous values", async () => { - mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }); + // Use exit code 5 (key absent) for all calls so --unset-all is treated as "not found", + // which is the expected state when testing URL normalization in isolation. + mockExec.getExecOutput.mockResolvedValue({ exitCode: 5, stdout: "", stderr: "" }); await overridePersistedExtraheader("https://github.com/", "ghp_test_token"); @@ -218,8 +229,8 @@ describe("git_auth_helpers.cjs", () => { expect(mockExec.exec).not.toHaveBeenCalled(); }); - it("should not throw when clearing scopes fails (key already absent)", async () => { - mockExec.getExecOutput.mockRejectedValue(new Error("key not found")); + it("should not throw when key is absent (exit code 5) when clearing scopes", async () => { + mockExec.getExecOutput.mockResolvedValue({ exitCode: 5, stdout: "", stderr: "" }); await expect(restorePersistedExtraheader(SERVER_URL, [])).resolves.toBeUndefined(); }); @@ -363,7 +374,9 @@ describe("git_auth_helpers.cjs", () => { }); it("should clear all scopes after callback when no previous value existed", async () => { - mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }); + // Exit code 5 = key absent (the expected state when no extraheader is configured). + // Using 1 here would cause unsetExtraheaderAllScopes to throw as of the fixed implementation. + mockExec.getExecOutput.mockResolvedValue({ exitCode: 5, stdout: "", stderr: "" }); await withGitHubHostToken("fork-token", async () => {}); @@ -404,21 +417,56 @@ describe("git_auth_helpers.cjs", () => { }); it("should not accumulate extraheader values across multiple override/restore cycles", async () => { - // Regression test for: restore loop reporting 'read 1', 'read 2', 'read 3', ... - // Root cause: getExtraheaderValues reads from ALL scopes, but --replace-all without - // explicit scope only writes to local scope, leaving the global scope value intact. - // After restore, both global and local have the value → 2 values on next read → etc. + // Regression test: verifies header cardinality stays at 1 across retries. + // + // Simulates real git config state with independent global and local scopes so that + // --unset-all, --replace-all, and --get-all operations interact with the same in-memory + // state. Without this, --get-all is hard-coded to one value regardless of what the other + // commands do, meaning the test would pass even with the old accumulating implementation. + // + // Bug: getExtraheaderValues reads ALL scopes (--get-all), but the old --replace-all + // without an explicit scope only wrote to local, leaving the global value intact. + // After each restore both scopes held a copy, so the next --get-all returned N+1 values. const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; - let capturedPreviousValueCounts = []; - // Simulate the initial state: git config --get-all returns 1 value (global scope) + // Simulate git config state: actions/checkout writes the upstream token to global scope. + let globalValues = [upstreamHeader]; + let localValues = []; + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { - if (args[1] === "--get-all") return { exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" }; - // unset-all calls return success (exit 0) - return { exitCode: 0, stdout: "", stderr: "" }; + if (args[1] === "--get-all") { + // git config --get-all reads from all scopes combined + const all = [...globalValues, ...localValues]; + return all.length > 0 ? { exitCode: 0, stdout: all.join("\n") + "\n", stderr: "" } : { exitCode: 5, stdout: "", stderr: "" }; + } + if (args[2] === "--unset-all") { + if (args[1] === "--global") { + const hadValues = globalValues.length > 0; + globalValues = []; + return { exitCode: hadValues ? 0 : 5, stdout: "", stderr: "" }; + } + if (args[1] === "--local") { + const hadValues = localValues.length > 0; + localValues = []; + return { exitCode: hadValues ? 0 : 5, stdout: "", stderr: "" }; + } + } + return { exitCode: 5, stdout: "", stderr: "" }; }); - // Intercept the info log to capture the "read N existing" count + mockExec.exec.mockImplementation(async (_cmd, args) => { + if (args[1] === "--local" && args[2] === "--replace-all") { + localValues = [args[4]]; + return 0; + } + if (args[1] === "--local" && args[2] === "--add") { + localValues.push(args[4]); + return 0; + } + return 0; + }); + + let capturedPreviousValueCounts = []; mockCore.info.mockImplementation(msg => { const m = msg.match(/read (\d+) existing extraheader value/); if (m) capturedPreviousValueCounts.push(Number(m[1])); @@ -429,7 +477,9 @@ describe("git_auth_helpers.cjs", () => { await withGitHubHostToken("fork-token", async () => {}); } - // Both cycles should read exactly 1 value — no accumulation + // Both cycles should read exactly 1 value — no accumulation. + // Without the fix (no --global unset before writing), the second cycle would + // see both global and local copies of the token and return 2. expect(capturedPreviousValueCounts).toEqual([1, 1]); }); }); From cba4cda5aa2f2a4c46ad6bd3d051b9b1b0456fb4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:25:24 +0000 Subject: [PATCH 5/7] test: add git integration tests for auth helpers using real repos with GIT_CONFIG_GLOBAL isolation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../js/git_auth_helpers.integration.test.cjs | 350 ++++++++++++++++++ 1 file changed, 350 insertions(+) create mode 100644 actions/setup/js/git_auth_helpers.integration.test.cjs diff --git a/actions/setup/js/git_auth_helpers.integration.test.cjs b/actions/setup/js/git_auth_helpers.integration.test.cjs new file mode 100644 index 00000000000..8393f1324e4 --- /dev/null +++ b/actions/setup/js/git_auth_helpers.integration.test.cjs @@ -0,0 +1,350 @@ +// Integration tests for git_auth_helpers.cjs. +// +// These tests use real git repositories so that --get-all, --unset-all, +// --replace-all, and --add operations interact with the same on-disk state. +// Global git config is isolated via GIT_CONFIG_GLOBAL so no test can pollute +// the developer's real ~/.gitconfig. +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { spawnSync } from "child_process"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url); + +const SERVER_URL = "https://github.com"; +const EXTRAHEADER_KEY = `http.${SERVER_URL}/.extraheader`; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/** + * Run a real git command with GIT_CONFIG_GLOBAL pointing at the isolated + * config file. Returns the spawnSync result; never throws on non-zero exit. + */ +function runGit(args, repoDir, globalConfigPath) { + const env = { ...process.env, GIT_CONFIG_GLOBAL: globalConfigPath }; + const result = spawnSync("git", args, { encoding: "utf8", cwd: repoDir, env }); + if (result.error) throw result.error; + return result; +} + +/** + * Create a temporary directory containing: + * - a bare global gitconfig file (.gitconfig-global) + * - an initialised git repository (repo/) + * + * Returns { repoDir, globalConfigPath }. + */ +function createIsolatedRepo(prefix) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const globalConfigPath = path.join(root, ".gitconfig-global"); + // Minimal global config so git commits work + fs.writeFileSync(globalConfigPath, "[user]\n\tname = Test User\n\temail = test@example.com\n"); + const repoDir = path.join(root, "repo"); + fs.mkdirSync(repoDir); + runGit(["init", "-q"], repoDir, globalConfigPath); + fs.writeFileSync(path.join(repoDir, "README.md"), "init\n"); + runGit(["add", "."], repoDir, globalConfigPath); + runGit(["commit", "-q", "-m", "init"], repoDir, globalConfigPath); + return { root, repoDir, globalConfigPath }; +} + +/** + * Build the exec API passed to git_auth_helpers.cjs. + * Every git subprocess it spawns carries GIT_CONFIG_GLOBAL so that --global + * operations hit the isolated config file rather than the real user config. + */ +function createExecApi(repoDir, globalConfigPath) { + function spawnGit(args, apiOptions = {}) { + const cwd = apiOptions.cwd || repoDir; + const env = { ...process.env, GIT_CONFIG_GLOBAL: globalConfigPath }; + return spawnSync("git", args, { encoding: "utf8", cwd, env }); + } + return { + async exec(cmd, args = [], options = {}) { + if (cmd !== "git") throw new Error(`unexpected command: ${cmd}`); + const r = spawnGit(args, options); + if (r.error) throw r.error; + if (r.status !== 0) { + throw new Error(`git ${args.join(" ")} failed (${r.status}): ${r.stderr}`); + } + return r.status; + }, + async getExecOutput(cmd, args = [], options = {}) { + if (cmd !== "git") throw new Error(`unexpected command: ${cmd}`); + const r = spawnGit(args, options); + if (r.error) throw r.error; + if (r.status !== 0 && !options.ignoreReturnCode) { + throw new Error(`git ${args.join(" ")} failed (${r.status}): ${r.stderr}`); + } + return { exitCode: r.status, stdout: r.stdout, stderr: r.stderr }; + }, + }; +} + +/** + * Read all values for key from a specific scope (or all scopes if scopeFlag + * is omitted) using a real git process. + */ +function readConfigValues(key, scopeFlag, repoDir, globalConfigPath) { + const args = scopeFlag ? ["config", scopeFlag, "--get-all", key] : ["config", "--get-all", key]; + const r = runGit(args, repoDir, globalConfigPath); + // exit 5 = key absent; any other non-zero = error + if (r.status !== 0) return []; + return r.stdout.trim().split("\n").filter(Boolean); +} + +/** + * Write a single value to the given scope. + */ +function writeConfigValue(key, value, scopeFlag, repoDir, globalConfigPath) { + const r = runGit(["config", scopeFlag, "--add", key, value], repoDir, globalConfigPath); + if (r.status !== 0) throw new Error(`git config write failed: ${r.stderr}`); +} + +// --------------------------------------------------------------------------- +// Suite +// --------------------------------------------------------------------------- + +describe("git_auth_helpers.cjs git integration", () => { + let repoDir; + let globalConfigPath; + let root; + let mockCore; + let overridePersistedExtraheader; + let restorePersistedExtraheader; + let unsetExtraheaderAllScopes; + let withGitHubHostToken; + + let origGithubServerUrl; + + beforeEach(() => { + ({ root, repoDir, globalConfigPath } = createIsolatedRepo("git-auth-helpers-it-")); + + origGithubServerUrl = process.env.GITHUB_SERVER_URL; + process.env.GITHUB_SERVER_URL = SERVER_URL; + + mockCore = { + info: vi.fn(), + warning: vi.fn(), + }; + + global.core = mockCore; + global.exec = createExecApi(repoDir, globalConfigPath); + + delete require.cache[require.resolve("./git_auth_helpers.cjs")]; + ({ overridePersistedExtraheader, restorePersistedExtraheader, unsetExtraheaderAllScopes, withGitHubHostToken } = require("./git_auth_helpers.cjs")); + }); + + afterEach(() => { + if (root && fs.existsSync(root)) { + fs.rmSync(root, { recursive: true, force: true }); + } + if (origGithubServerUrl !== undefined) { + process.env.GITHUB_SERVER_URL = origGithubServerUrl; + } else { + delete process.env.GITHUB_SERVER_URL; + } + delete global.core; + delete global.exec; + vi.clearAllMocks(); + }); + + // ────────────────────────────────────────────────────── + // unsetExtraheaderAllScopes + // ────────────────────────────────────────────────────── + + describe("unsetExtraheaderAllScopes", () => { + it("clears a value from the global scope", async () => { + writeConfigValue(EXTRAHEADER_KEY, "Authorization: basic abc", "--global", repoDir, globalConfigPath); + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(1); + + await unsetExtraheaderAllScopes(EXTRAHEADER_KEY, repoDir); + + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(0); + }); + + it("clears a value from the local scope", async () => { + writeConfigValue(EXTRAHEADER_KEY, "Authorization: basic abc", "--local", repoDir, globalConfigPath); + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toHaveLength(1); + + await unsetExtraheaderAllScopes(EXTRAHEADER_KEY, repoDir); + + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toHaveLength(0); + }); + + it("clears values from both global and local scopes simultaneously", async () => { + writeConfigValue(EXTRAHEADER_KEY, "Authorization: basic global", "--global", repoDir, globalConfigPath); + writeConfigValue(EXTRAHEADER_KEY, "Authorization: basic local", "--local", repoDir, globalConfigPath); + // Both scopes have a value; --get-all should return 2 + expect(readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath)).toHaveLength(2); + + await unsetExtraheaderAllScopes(EXTRAHEADER_KEY, repoDir); + + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(0); + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toHaveLength(0); + expect(readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath)).toHaveLength(0); + }); + + it("does not throw when the key is absent in both scopes (exit code 5)", async () => { + // No value written — key is absent in all scopes. + await expect(unsetExtraheaderAllScopes(EXTRAHEADER_KEY, repoDir)).resolves.toBeUndefined(); + }); + }); + + // ────────────────────────────────────────────────────── + // overridePersistedExtraheader + // ────────────────────────────────────────────────────── + + describe("overridePersistedExtraheader", () => { + it("removes the global token and writes only the fork token to local scope", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + const forkToken = "fork-token-123"; + const expectedForkHeader = `Authorization: basic ${Buffer.from(`x-access-token:${forkToken}`).toString("base64")}`; + + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + await overridePersistedExtraheader(SERVER_URL, forkToken, repoDir); + + // Global must be empty + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(0); + // Local must have exactly the fork token + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toEqual([expectedForkHeader]); + // get-all must see exactly one header + expect(readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath)).toEqual([expectedForkHeader]); + }); + + it("returns the upstream header that was in the global scope before the override", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + const previous = await overridePersistedExtraheader(SERVER_URL, "fork-token", repoDir); + + expect(previous).toEqual([upstreamHeader]); + }); + + it("returns an empty array and still writes fork token when no extraheader exists", async () => { + const forkToken = "fork-only"; + const expectedForkHeader = `Authorization: basic ${Buffer.from(`x-access-token:${forkToken}`).toString("base64")}`; + + const previous = await overridePersistedExtraheader(SERVER_URL, forkToken, repoDir); + + expect(previous).toEqual([]); + expect(readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath)).toEqual([expectedForkHeader]); + }); + }); + + // ────────────────────────────────────────────────────── + // restorePersistedExtraheader + // ────────────────────────────────────────────────────── + + describe("restorePersistedExtraheader", () => { + it("removes the fork token from local and restores the upstream token to local", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + const forkHeader = `Authorization: basic ${Buffer.from("x-access-token:fork").toString("base64")}`; + + // Simulate the state after an override: global is empty, local has the fork token. + writeConfigValue(EXTRAHEADER_KEY, forkHeader, "--local", repoDir, globalConfigPath); + + await restorePersistedExtraheader(SERVER_URL, [upstreamHeader], repoDir); + + // Global must remain empty + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(0); + // Local must have the restored upstream token + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toEqual([upstreamHeader]); + // get-all must see exactly one header + expect(readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath)).toEqual([upstreamHeader]); + }); + + it("clears all scopes and writes nothing when previousValues is empty", async () => { + const forkHeader = `Authorization: basic ${Buffer.from("x-access-token:fork").toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, forkHeader, "--local", repoDir, globalConfigPath); + + await restorePersistedExtraheader(SERVER_URL, [], repoDir); + + expect(readConfigValues(EXTRAHEADER_KEY, "--global", repoDir, globalConfigPath)).toHaveLength(0); + expect(readConfigValues(EXTRAHEADER_KEY, "--local", repoDir, globalConfigPath)).toHaveLength(0); + }); + }); + + // ────────────────────────────────────────────────────── + // withGitHubHostToken — multi-cycle regression + // ────────────────────────────────────────────────────── + + describe("withGitHubHostToken", () => { + it("does not accumulate extraheader values across multiple override/restore cycles", async () => { + // Simulates the exact production failure: actions/checkout writes the upstream + // token to the global scope. Without the scope-clearing fix, each restore would + // leave the global value intact and add a new local copy, so --get-all returns + // N+1 values on each subsequent cycle. + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + const capturedCounts = []; + mockCore.info.mockImplementation(msg => { + const m = msg.match(/read (\d+) existing extraheader value/); + if (m) capturedCounts.push(Number(m[1])); + }); + + // Two full override/restore cycles (simulates two retries) + for (let i = 0; i < 2; i++) { + await withGitHubHostToken("fork-token", async () => {}, repoDir); + } + + // Without the fix: [1, 2] — second cycle sees global + local copies. + // With the fix: [1, 1] — global is always cleared before each write. + expect(capturedCounts).toEqual([1, 1]); + }); + + it("leaves exactly one extraheader value active inside the callback", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + const forkToken = "fork-callback-check"; + const expectedForkHeader = `Authorization: basic ${Buffer.from(`x-access-token:${forkToken}`).toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + let valuesInsideCallback; + await withGitHubHostToken( + forkToken, + async () => { + valuesInsideCallback = readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath); + }, + repoDir + ); + + // Exactly one Authorization header must be active during the callback + expect(valuesInsideCallback).toEqual([expectedForkHeader]); + }); + + it("restores exactly one extraheader value after the callback completes", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + await withGitHubHostToken("fork-token", async () => {}, repoDir); + + // After restore, exactly the upstream token should be in local scope (global was cleared) + const allValues = readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath); + expect(allValues).toEqual([upstreamHeader]); + }); + + it("restores the config even when the callback throws", async () => { + const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`; + writeConfigValue(EXTRAHEADER_KEY, upstreamHeader, "--global", repoDir, globalConfigPath); + + await expect( + withGitHubHostToken( + "fork-token", + async () => { + throw new Error("simulated callback failure"); + }, + repoDir + ) + ).rejects.toThrow("simulated callback failure"); + + const allValues = readConfigValues(EXTRAHEADER_KEY, null, repoDir, globalConfigPath); + expect(allValues).toEqual([upstreamHeader]); + }); + }); +}); From 2b7c36492eba2893128b909b9e0ec529ecf6720b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:41:45 +0000 Subject: [PATCH 6/7] test: update extra_empty_commit.integration.test mock to handle new unset-all calls Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- actions/setup/js/extra_empty_commit.integration.test.cjs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/actions/setup/js/extra_empty_commit.integration.test.cjs b/actions/setup/js/extra_empty_commit.integration.test.cjs index b8687196755..3c9ce1f69d0 100644 --- a/actions/setup/js/extra_empty_commit.integration.test.cjs +++ b/actions/setup/js/extra_empty_commit.integration.test.cjs @@ -105,6 +105,11 @@ describe("extra_empty_commit git integration", () => { if (args[0] === "config" && args[1] === "--get-all") { return { exitCode: 1, stdout: "", stderr: "" }; } + // Handle git config --global/--local --unset-all calls from unsetExtraheaderAllScopes + // Return exit code 5 (key absent) since this test simulates no pre-existing extraheaders + if (args[0] === "config" && (args[1] === "--global" || args[1] === "--local") && args[2] === "--unset-all") { + return { exitCode: 5, stdout: "", stderr: "" }; + } // Return the actual HEAD SHA for rev-parse (needed for GraphQL createCommitOnBranch) if (args[0] === "rev-parse" && args[1] === "HEAD") { const result = execGit(["rev-parse", "HEAD"], repoDir); From 5af7b555ed41634c88bd4c7da5d3cb15aab564da Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:19:39 +0000 Subject: [PATCH 7/7] test: update extra_empty_commit.test mocks to handle new git config scope operations Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- actions/setup/js/extra_empty_commit.test.cjs | 59 ++++++++++++++------ 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/actions/setup/js/extra_empty_commit.test.cjs b/actions/setup/js/extra_empty_commit.test.cjs index 984396480b3..0669ef6993f 100644 --- a/actions/setup/js/extra_empty_commit.test.cjs +++ b/actions/setup/js/extra_empty_commit.test.cjs @@ -26,7 +26,15 @@ describe("extra_empty_commit.cjs", () => { // Default exec mock: resolves successfully, no stdout output mockExec = { exec: vi.fn().mockResolvedValue(0), - getExecOutput: vi.fn().mockResolvedValue({ stdout: "", stderr: "", exitCode: 1 }), + getExecOutput: vi.fn().mockImplementation(async (_cmd, args) => { + // Handle git config --global/--local --unset-all calls from unsetExtraheaderAllScopes + // Return exit code 5 (key absent) since most tests simulate no pre-existing extraheaders + if (args && args[0] === "config" && (args[1] === "--global" || args[1] === "--local") && args[2] === "--unset-all") { + return { exitCode: 5, stdout: "", stderr: "" }; + } + // Default: return exit code 1 (generic failure) for other getExecOutput calls + return { stdout: "", stderr: "", exitCode: 1 }; + }), }; global.core = mockCore; @@ -177,25 +185,34 @@ describe("extra_empty_commit.cjs", () => { const execCalls = mockExec.exec.mock.calls; // Use findIndex so the assertion stays correct even if restorePersistedExtraheader // also emits a --replace-all call (e.g. when restoring previous values). - const replaceIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--replace-all"); + const replaceIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--local" && c[1][2] === "--replace-all"); expect(replaceIdx).toBeGreaterThanOrEqual(0); const replaceCall = execCalls[replaceIdx]; - expect(replaceCall[1][2]).toBe("http.https://github.com/.extraheader"); - expect(replaceCall[1][3]).toBe(`Authorization: basic ${Buffer.from("x-access-token:ghp_test_token_123").toString("base64")}`); + expect(replaceCall[1][3]).toBe("http.https://github.com/.extraheader"); + expect(replaceCall[1][4]).toBe(`Authorization: basic ${Buffer.from("x-access-token:ghp_test_token_123").toString("base64")}`); const pushIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "push"); expect(pushIdx).toBeGreaterThan(-1); expect(replaceIdx).toBeLessThan(pushIdx); - const unsetCall = execCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--unset-all"); + // After push, restoration clears credentials via getExecOutput (unsetExtraheaderAllScopes) + // When there are no previous values to restore, only the unset happens. + const getExecOutputCalls = mockExec.getExecOutput.mock.calls; + const unsetCall = getExecOutputCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][2] === "--unset-all"); expect(unsetCall).toBeDefined(); - expect(execCalls.indexOf(unsetCall)).toBeGreaterThan(pushIdx); }); it("should restore a previously-set extraheader after push", async () => { const prevHeader = `Authorization: basic ${Buffer.from("x-access-token:old_token").toString("base64")}`; // Override getExecOutput so it returns the previous header when reading the extraheader key. - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: prevHeader + "\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (cmd, args) => { + // Handle git config --global/--local --unset-all calls + if (args && args[0] === "config" && (args[1] === "--global" || args[1] === "--local") && args[2] === "--unset-all") { + return { exitCode: 5, stdout: "", stderr: "" }; + } + // Return previous header for --get-all + return { exitCode: 0, stdout: prevHeader + "\n", stderr: "" }; + }); await pushExtraEmptyCommit({ branchName: "feature-branch", @@ -207,10 +224,10 @@ describe("extra_empty_commit.cjs", () => { const pushIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "push"); expect(pushIdx).toBeGreaterThan(-1); - // After push, there should be a --replace-all that restores the old header. - const restoreCall = execCalls.find((c, i) => i > pushIdx && c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--replace-all"); + // After push, there should be a --local --replace-all that restores the old header. + const restoreCall = execCalls.find((c, i) => i > pushIdx && c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--local" && c[1][2] === "--replace-all"); expect(restoreCall).toBeDefined(); - expect(restoreCall[1][3]).toBe(prevHeader); + expect(restoreCall[1][4]).toBe(prevHeader); }); it("should restore extraheader even when push throws", async () => { @@ -232,13 +249,14 @@ describe("extra_empty_commit.cjs", () => { const execCalls = mockExec.exec.mock.calls; - // The override (--replace-all with CI token) must have happened before the failed push. - const overrideIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--replace-all"); + // The override (--local --replace-all with CI token) must have happened before the failed push. + const overrideIdx = execCalls.findIndex(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--local" && c[1][2] === "--replace-all"); expect(overrideIdx).toBeGreaterThanOrEqual(0); // After the failed push, the finally block must restore by unsetting the key - // (default mock returns no previous extraheader). - const unsetCall = execCalls.find((c, i) => i > overrideIdx && c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--unset-all"); + // (default mock returns no previous extraheader). Check getExecOutput calls. + const getExecOutputCalls = mockExec.getExecOutput.mock.calls; + const unsetCall = getExecOutputCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][2] === "--unset-all"); expect(unsetCall).toBeDefined(); }); @@ -396,6 +414,10 @@ describe("extra_empty_commit.cjs", () => { if (cmd === "git" && args && args[0] === "rev-parse" && args[1] === "HEAD") { return { exitCode: 0, stdout: TEST_HEAD_OID + "\n", stderr: "" }; } + // Handle git config --global/--local --unset-all calls + if (args && args[0] === "config" && (args[1] === "--global" || args[1] === "--local") && args[2] === "--unset-all") { + return { exitCode: 5, stdout: "", stderr: "" }; + } // No pre-existing extraheader return { exitCode: 1, stdout: "", stderr: "" }; }); @@ -529,11 +551,12 @@ describe("extra_empty_commit.cjs", () => { }); const execCalls = mockExec.exec.mock.calls; - // Override was applied before the API commit - const overrideCall = execCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--replace-all"); + // Override was applied before the API commit (--local --replace-all) + const overrideCall = execCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--local" && c[1][2] === "--replace-all"); expect(overrideCall).toBeDefined(); - // After the API commit, --unset-all restores to empty (no previous header) - const unsetCall = execCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--unset-all"); + // After the API commit, unsetExtraheaderAllScopes is called (via getExecOutput) + const getExecOutputCalls = mockExec.getExecOutput.mock.calls; + const unsetCall = getExecOutputCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][2] === "--unset-all"); expect(unsetCall).toBeDefined(); }); });