Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/mcp-inspector.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-otel-backends.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1507,6 +1507,8 @@ async function main(config = {}) {
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
});
core.info("Changes pushed to branch (from bundle)");

Expand Down Expand Up @@ -1537,6 +1539,8 @@ async function main(config = {}) {
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
});
core.info("Changes pushed to branch after bundle rewrite retry");

Expand Down Expand Up @@ -1765,6 +1769,8 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
});
core.info("Changes pushed to branch");

Expand Down Expand Up @@ -1911,6 +1917,8 @@ ${patchPreview}`;
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
});
core.info("Empty branch pushed successfully");

Expand Down
62 changes: 58 additions & 4 deletions actions/setup/js/push_signed_commits.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

const { ERR_API } = require("./error_codes.cjs");
const { loadTemporaryIdMapFromResolved, replaceTemporaryIdReferencesInPatch, TEMPORARY_ID_CANDIDATE_REFERENCE_PATTERN } = require("./temporary_id.cjs");

/** Sentinel error class used to signal that the commit range contains a shape
* that the GitHub GraphQL `createCommitOnBranch` mutation cannot represent
Expand Down Expand Up @@ -124,6 +125,45 @@ async function readBlobAsBase64(blobHash, cwd) {
return Buffer.concat(chunks).toString("base64");
}
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] buildTemporaryIdMap is almost identical to the already-exported loadTemporaryIdMapFromResolved in temporary_id.cjs. The only meaningful difference is that loadTemporaryIdMapFromResolved falls back to the GitHub Actions context.repo while this function accepts an explicit currentRepo string.

Rather than maintaining two copies of the same normalisation loop, consider exporting a variant (or extending the existing one with an optional currentRepo parameter) so this file can delegate:

const { loadTemporaryIdMapFromResolved } = require("./temporary_id.cjs");
// ... pass currentRepo explicitly rather than relying on context

Duplicated logic here will silently diverge if loadTemporaryIdMapFromResolved is ever updated.


/**
* Replace temporary ID references in base64-encoded UTF-8 text content.
* Returns original content unchanged for:
* - binary / non-UTF8 blobs
* - UTF-8 text with no temporary ID matches
* Returns rewritten base64 content when UTF-8 text contains resolvable temporary IDs.
*
* @param {string} base64Content
* @param {Map<string, {repo: string, number: number}>} temporaryIdMap
* @param {string} currentRepo
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.

[/diagnose] The function replaceTemporaryIdReferencesInPatch is named for patch/diff text, but here it's applied to raw file contents. The function happens to work correctly (it does two-pass URL + text replacement), but the name creates a misleading mental model for future readers who may wonder whether patch-specific heuristics (like +/- diff markers) affect the behaviour.

A renaming like replaceTemporaryIdReferencesInText — or a thin, clearly-named wrapper in this file — would make the intent unambiguous.

* @param {string} filePath
* @returns {string}
*/
function maybeReplaceTemporaryIdsInBase64Content(base64Content, temporaryIdMap, currentRepo, filePath) {
if (!(temporaryIdMap instanceof Map) || temporaryIdMap.size === 0) {
return base64Content;
}

const rawBytes = Buffer.from(base64Content, "base64");
const utf8Text = rawBytes.toString("utf8");

// Treat only clean UTF-8 round-trippable content as text.
if (!Buffer.from(utf8Text, "utf8").equals(rawBytes)) {
return base64Content;
}

if (!TEMPORARY_ID_CANDIDATE_REFERENCE_PATTERN.test(utf8Text)) {
return base64Content;
}

const replaced = replaceTemporaryIdReferencesInPatch(utf8Text, temporaryIdMap, currentRepo);
if (replaced === utf8Text) {
return base64Content;
}

core.info(`pushSignedCommits: resolved temporary ID references in file content: ${filePath}`);
return Buffer.from(replaced, "utf8").toString("base64");
}

/**
* Push the local branch to origin using git directly and return the local HEAD
* SHA after the push succeeds.
Expand Down Expand Up @@ -167,9 +207,20 @@ async function resolveLocalHeadSha(cwd) {
* @param {string} opts.cwd - Working directory of the local git checkout
* @param {object} [opts.gitAuthEnv] - Environment variables for git push fallback auth
* @param {boolean} [opts.signedCommits=true] - When false, skip GraphQL signed commits and use git push directly
* @param {Record<string, any>} [opts.resolvedTemporaryIds] - Resolved temporary IDs map
* @param {string} [opts.currentRepo] - Repository slug used for same-repo temporary ID resolution
* @returns {Promise<string | undefined>} SHA of the commit that landed on the target branch
*/
async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd, gitAuthEnv, signedCommits = true }) {
async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd, gitAuthEnv, signedCommits = true, resolvedTemporaryIds, currentRepo }) {
const effectiveCurrentRepo = currentRepo || `${owner}/${repo}`;
const temporaryIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds, {
defaultRepo: effectiveCurrentRepo,
validatePositiveIntegers: true,
onInvalidNumber: (normalizedKey, rawValue) => {
core.warning(`pushSignedCommits: ignoring invalid resolved temporary ID number for '${normalizedKey}': ${String(rawValue)}`);
},
});

// The default parameter value converts undefined to true; this check tests only the explicit false value.
if (signedCommits === false) {
core.info(`pushSignedCommits: signed-commits disabled (using direct git push) for branch ${branch}`);
Expand Down Expand Up @@ -303,7 +354,8 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c
if (dstMode === "100755") {
core.warning(`pushSignedCommits: executable bit on ${renamedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`);
}
additions.push({ path: renamedPath, contents: await readBlobAsBase64(dstHash, cwd) });
const blobContents = await readBlobAsBase64(dstHash, cwd);
additions.push({ path: renamedPath, contents: maybeReplaceTemporaryIdsInBase64Content(blobContents, temporaryIdMap, effectiveCurrentRepo, renamedPath) });
} else if (status && status.startsWith("C")) {
// Copy: source path is kept (no deletion), only the destination path is added
const copiedPath = unquoteCPath(paths[1]);
Expand All @@ -322,7 +374,8 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c
if (dstMode === "100755") {
core.warning(`pushSignedCommits: executable bit on ${copiedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`);
}
additions.push({ path: copiedPath, contents: await readBlobAsBase64(dstHash, cwd) });
const blobContents = await readBlobAsBase64(dstHash, cwd);
additions.push({ path: copiedPath, contents: maybeReplaceTemporaryIdsInBase64Content(blobContents, temporaryIdMap, effectiveCurrentRepo, copiedPath) });
} else {
// Added or Modified
if (dstMode === "160000") {
Expand All @@ -336,7 +389,8 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c
if (dstMode === "100755") {
core.warning(`pushSignedCommits: executable bit on ${filePath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`);
}
additions.push({ path: filePath, contents: await readBlobAsBase64(dstHash, cwd) });
const blobContents = await readBlobAsBase64(dstHash, cwd);
additions.push({ path: filePath, contents: maybeReplaceTemporaryIdsInBase64Content(blobContents, temporaryIdMap, effectiveCurrentRepo, filePath) });
}
}

Expand Down
93 changes: 93 additions & 0 deletions actions/setup/js/push_signed_commits.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,99 @@ describe("push_signed_commits integration tests", () => {
expect(Buffer.from(variables.input.fileChanges.additions[0].contents, "base64").toString()).toBe("Hello World\n");
});

it("should resolve temporary ID references in text file contents before GraphQL replay", async () => {
execGit(["checkout", "-b", "temp-id-branch"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "quarantine.cs"), '[QuarantinedTest("https://github.com/test-owner/test-repo/issues/#aw_test1")]\n// linked: #aw_test1\n');
execGit(["add", "quarantine.cs"], { cwd: workDir });
execGit(["commit", "-m", "Add quarantine reference"], { cwd: workDir });
execGit(["push", "-u", "origin", "temp-id-branch"], { cwd: workDir });

global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

await pushSignedCommits({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "temp-id-branch",
baseRef: "origin/main",
cwd: workDir,
resolvedTemporaryIds: {
aw_test1: { repo: "test-owner/test-repo", number: 66708 },
},
currentRepo: "test-owner/test-repo",
});

expect(githubClient.graphql).toHaveBeenCalledTimes(1);
const additions = githubClient.graphql.mock.calls[0][1].input.fileChanges.additions;
expect(additions).toHaveLength(1);
const resolvedContent = Buffer.from(additions[0].contents, "base64").toString();
expect(resolvedContent).toContain("https://github.com/test-owner/test-repo/issues/66708");
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.

[/tdd] The assertion toContain("#66708") is too broad — it passes as long as the number appears anywhere in the resolved content, including inside the URL replacement (issues/66708). This means the text-context replacement (// linked: #aw_test1// linked: #66708) is never actually verified independently.

Consider a more specific assertion:

expect(resolvedContent).toContain("// linked: #66708");

This confirms both the URL path (/issues/66708) and the plain #66708 text-context replacements are actually working.

expect(resolvedContent).toContain("#66708");
expect(resolvedContent).not.toContain("#aw_test1");
});
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.

[/tdd] No test covers the backward-compatibility path where resolvedTemporaryIds is undefined (callers that haven't been updated yet). Without it, we can't confirm that omitting the new parameter leaves existing behaviour untouched.

A simple regression guard:

it("should leave file content unchanged when resolvedTemporaryIds is undefined", async () => {
  // ... set up a commit with a regular file ...
  await pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd });
  const additions = githubClient.graphql.mock.calls[0][1].input.fileChanges.additions;
  expect(Buffer.from(additions[0].contents, "base64").toString()).toBe("expected original content");
});


it("should still run replacement logic for malformed temporary ID candidates and emit warning", async () => {
execGit(["checkout", "-b", "temp-id-malformed-candidate-branch"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "quarantine.cs"), "// malformed link: #aw_test-id\n");
execGit(["add", "quarantine.cs"], { cwd: workDir });
execGit(["commit", "-m", "Add malformed temporary ID reference"], { cwd: workDir });
execGit(["push", "-u", "origin", "temp-id-malformed-candidate-branch"], { cwd: workDir });

global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

await pushSignedCommits({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "temp-id-malformed-candidate-branch",
baseRef: "origin/main",
cwd: workDir,
resolvedTemporaryIds: {
aw_test: { repo: "test-owner/test-repo", number: 66708 },
},
currentRepo: "test-owner/test-repo",
});

expect(githubClient.graphql).toHaveBeenCalledTimes(1);
const additions = githubClient.graphql.mock.calls[0][1].input.fileChanges.additions;
const replayedContent = Buffer.from(additions[0].contents, "base64").toString();
expect(replayedContent).toContain("#66708-id");
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Malformed temporary ID reference '#aw_test-id'"));
});

it("should ignore invalid resolved temporary ID numbers instead of replacing with NaN", async () => {
execGit(["checkout", "-b", "temp-id-invalid-number-branch"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "quarantine.cs"), "// linked: #aw_test2\n");
execGit(["add", "quarantine.cs"], { cwd: workDir });
execGit(["commit", "-m", "Add temporary ID with invalid map entry"], { cwd: workDir });
execGit(["push", "-u", "origin", "temp-id-invalid-number-branch"], { cwd: workDir });

global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

await pushSignedCommits({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "temp-id-invalid-number-branch",
baseRef: "origin/main",
cwd: workDir,
resolvedTemporaryIds: {
aw_test2: { repo: "test-owner/test-repo", number: "not-a-number" },
},
currentRepo: "test-owner/test-repo",
});

expect(githubClient.graphql).toHaveBeenCalledTimes(1);
const additions = githubClient.graphql.mock.calls[0][1].input.fileChanges.additions;
const replayedContent = Buffer.from(additions[0].contents, "base64").toString();
expect(replayedContent).toContain("#aw_test2");
expect(replayedContent).not.toContain("#NaN");
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("ignoring invalid resolved temporary ID number for 'aw_test2'"));
});

it("should call GraphQL once per commit for multiple new commits", async () => {
execGit(["checkout", "-b", "multi-commit-branch"], { cwd: workDir });

Expand Down
2 changes: 2 additions & 0 deletions actions/setup/js/push_to_pull_request_branch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,8 @@ async function main(config = {}) {
cwd: repoCwd || process.cwd(),
gitAuthEnv,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
});
if (pushedSha) {
pushedCommitSha = pushedSha;
Expand Down
58 changes: 53 additions & 5 deletions actions/setup/js/temporary_id.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ const TEMPORARY_ID_PATTERN = /#(aw_[A-Za-z0-9_]{3,12})\b/gi;
*/
const TEMPORARY_ID_CANDIDATE_PATTERN = /#aw_([A-Za-z0-9_-]+)/gi;

/**
* Regex pattern for quick candidate detection of temporary ID references.
* Non-global on purpose so repeated `.test()` calls are state-free.
*/
const TEMPORARY_ID_CANDIDATE_REFERENCE_PATTERN = /#aw_/i;

/**
* @typedef {Object} RepoIssuePair
* @property {string} repo - Repository slug in "owner/repo" format
Expand Down Expand Up @@ -256,32 +262,73 @@ function loadTemporaryIdMap() {
* - { repo, number }
*
* @param {any} resolvedTemporaryIds - Object or Map of temporary IDs to resolved values
* @param {object} [options]
* @param {string} [options.defaultRepo] - Fallback repo for legacy number-only values; when null/undefined, uses GitHub Action context repo when available, else ""
* @param {boolean} [options.validatePositiveIntegers] - When true, ignore non-positive-integer numbers
* @param {(normalizedKey: string, rawValue: unknown) => void} [options.onInvalidNumber] - Callback invoked when a value is skipped for non-finite parsing, or for non-positive/non-integer values when `validatePositiveIntegers` is true
* @returns {Map<string, RepoIssuePair>} Map of normalized temporary_id to {repo, number}
*/
function loadTemporaryIdMapFromResolved(resolvedTemporaryIds) {
function loadTemporaryIdMapFromResolved(resolvedTemporaryIds, options = {}) {
/** @type {Map<string, RepoIssuePair>} */
const result = new Map();

if (!resolvedTemporaryIds) {
return result;
}

const contextRepo = typeof context !== "undefined" ? `${context.repo.owner}/${context.repo.repo}` : "";
const contextRepo = options.defaultRepo ?? (typeof context !== "undefined" ? `${context.repo.owner}/${context.repo.repo}` : "");

/**
* @param {string} normalizedKey
* @param {unknown} rawValue
* @returns {number | null}
*/
const toNumber = (normalizedKey, rawValue) => {
const number = Number(rawValue);
if (!Number.isFinite(number)) {
if (typeof options.onInvalidNumber === "function") {
options.onInvalidNumber(normalizedKey, rawValue);
}
return null;
}
if (!options.validatePositiveIntegers) {
return number;
}
if (!Number.isInteger(number) || number < 1) {
if (typeof options.onInvalidNumber === "function") {
options.onInvalidNumber(normalizedKey, rawValue);
}
return null;
}
return number;
};

const entries = resolvedTemporaryIds instanceof Map ? Array.from(resolvedTemporaryIds.entries()) : Object.entries(resolvedTemporaryIds);
for (const [key, value] of entries) {
const normalizedKey = normalizeTemporaryId(key);
if (typeof value === "number") {
result.set(normalizedKey, { repo: contextRepo, number: value });
const number = toNumber(normalizedKey, value);
if (number === null) {
continue;
}
result.set(normalizedKey, { repo: contextRepo, number });
continue;
}
if (typeof value === "object" && value !== null) {
if ("repo" in value && "number" in value) {
result.set(normalizedKey, { repo: String(value.repo), number: Number(value.number) });
const number = toNumber(normalizedKey, value.number);
if (number === null) {
continue;
}
result.set(normalizedKey, { repo: String(value.repo), number });
continue;
}
if ("number" in value) {
result.set(normalizedKey, { repo: contextRepo, number: Number(value.number) });
const number = toNumber(normalizedKey, value.number);
if (number === null) {
continue;
}
result.set(normalizedKey, { repo: contextRepo, number });
continue;
}
}
Expand Down Expand Up @@ -673,6 +720,7 @@ function resolveNumberFromTemporaryId(value, resolvedTemporaryIds) {
module.exports = {
TEMPORARY_ID_PATTERN,
TEMPORARY_ID_CANDIDATE_PATTERN,
TEMPORARY_ID_CANDIDATE_REFERENCE_PATTERN,
generateTemporaryId,
isTemporaryId,
normalizeTemporaryId,
Expand Down