Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
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
86 changes: 82 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,8 @@
*/

const { ERR_API } = require("./error_codes.cjs");
const { normalizeTemporaryId, replaceTemporaryIdReferencesInPatch } = require("./temporary_id.cjs");
const TEMPORARY_ID_REFERENCE_PATTERN = /#aw_[A-Za-z0-9_]{3,12}\b/i;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot move to temporaryid file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved in 86b539e: the candidate precheck regex now lives in temporary_id.cjs and push_signed_commits.cjs imports it from there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot move to temporaryid file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 86b539e: regex precheck was moved to temporary_id.cjs and reused from push_signed_commits.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 +126,74 @@ 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.


/**
* Build a normalized temporary ID map from resolved temporary IDs.
*
* @param {Record<string, any> | null | undefined} resolvedTemporaryIds
* @param {string} currentRepo
* @returns {Map<string, {repo: string, number: number}>}
*/
function buildTemporaryIdMap(resolvedTemporaryIds, currentRepo) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot review if this function is a duplicate and refactor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored in ccca10a: removed the local duplicate buildTemporaryIdMap from push_signed_commits.cjs and now reuse loadTemporaryIdMapFromResolved from temporary_id.cjs with signed-replay-specific validation options.

/** @type {Map<string, {repo: string, number: number}>} */
const map = new Map();
if (!resolvedTemporaryIds || typeof resolvedTemporaryIds !== "object" || Array.isArray(resolvedTemporaryIds)) {
return map;
}
for (const [key, value] of Object.entries(resolvedTemporaryIds)) {
const normalizedKey = normalizeTemporaryId(key);
if (typeof value === "number") {
map.set(normalizedKey, { repo: currentRepo, number: value });
continue;
}
if (value && typeof value === "object" && "number" in value) {
map.set(normalizedKey, {
repo: String("repo" in value && value.repo ? value.repo : currentRepo),
number: Number(value.number),
});
}
}
return map;
}

/**
* 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_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 +237,14 @@ 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 = buildTemporaryIdMap(resolvedTemporaryIds, effectiveCurrentRepo);

// 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 +378,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 +398,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 +413,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
32 changes: 32 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,38 @@ 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 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
Loading