Resolve #aw_* temporary IDs during bundle-based signed commit replay#33181
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
#aw_* temporary IDs during bundle-based signed commit replay
There was a problem hiding this comment.
Pull request overview
This pull request fixes bundle-based signed commit replay to resolve #aw_* temporary ID placeholders in file contents (matching the behavior already present in patch-based flows).
Changes:
- Update
pushSignedCommitsto optionally rewrite temporary ID references inside UTF-8 text blob contents before submittingcreateCommitOnBranch. - Pass temporary ID substitution context (
resolvedTemporaryIds,currentRepo) from bundle-related callers. - Add an integration test ensuring both URL-context and text-context temporary ID replacements occur during GraphQL replay.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Adds temporary ID substitution for UTF-8 text additions during GraphQL signed commit replay. |
| actions/setup/js/create_pull_request.cjs | Passes resolvedTemporaryIds and currentRepo into pushSignedCommits for bundle and non-bundle push paths. |
| actions/setup/js/push_to_pull_request_branch.cjs | Passes resolvedTemporaryIds and currentRepo into pushSignedCommits when pushing to an existing PR branch. |
| actions/setup/js/push_signed_commits.test.cjs | Adds regression coverage for resolving #aw_* placeholders in replayed file contents. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| 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), | ||
| }); |
|
|
||
| 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; |
🧪 Test Quality Sentinel ReportTest Quality Score: 82/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires ReviewNo tests failed behavioral classification. One minor observation: i️ Missing error/edge casesTest: Test Analysis SummaryThe single new test verifies the core behavioral contract of the PR: that
This is a genuine behavioral contract test — it exercises a real git repository (no git mocking), a mock GitHub API client, and asserts on the observable mutation payload. Deleting this test would allow a regression in temporary-ID resolution to go undetected during signed-commit bundle replay. Test inflation ratio: The test file gained 32 lines vs. 86 lines added across the production files ( Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §26069993142
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnose, and /zoom-out — this is a targeted bug fix with regression coverage, so those three lenses are most useful.
Key Themes
- Test assertion precision — the new test verifies the main scenario but the
toContain("#66708")assertion doesn't distinguish URL from text-context replacement; a more specific check would make the test a true specification. - Missing edge-case coverage — no test for the backward-compatibility path (
resolvedTemporaryIdsomitted) or binary/non-UTF-8 files, which are both branches exercised by the new code. - Duplicated normalisation logic —
buildTemporaryIdMapreimplements the same loop already present inloadTemporaryIdMapFromResolved(exported fromtemporary_id.cjs); extending the existing function to accept an optionalcurrentRepowould remove the duplication. - Naming mismatch —
replaceTemporaryIdReferencesInPatchis applied to raw file content, which may confuse future readers about whether patch-specific handling applies.
Positive Highlights
- ✅ Clean, well-documented helper functions with JSDoc throughout.
- ✅ The binary-detection heuristic (UTF-8 round-trip check) is a pragmatic approach that avoids corrupting non-text blobs.
- ✅ All four call sites in
create_pull_request.cjswere updated consistently — no partial fix. - ✅ The
effectiveCurrentRepofallback (currentRepo || owner/repo) is a nice safety net.
Verdict
The fix is correct and the approach is sound. All comments are non-blocking suggestions — commenting rather than requesting changes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.7M
| 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"); |
There was a problem hiding this comment.
[/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("https://github.com/test-owner/test-repo/issues/66708"); | ||
| expect(resolvedContent).toContain("#66708"); | ||
| expect(resolvedContent).not.toContain("#aw_test1"); | ||
| }); |
There was a problem hiding this comment.
[/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");
});| @@ -124,6 +126,74 @@ async function readBlobAsBase64(blobHash, cwd) { | |||
| return Buffer.concat(chunks).toString("base64"); | |||
| } | |||
There was a problem hiding this comment.
[/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 contextDuplicated logic here will silently diverge if loadTemporaryIdMapFromResolved is ever updated.
| * | ||
| * @param {string} base64Content | ||
| * @param {Map<string, {repo: string, number: number}>} temporaryIdMap | ||
| * @param {string} currentRepo |
There was a problem hiding this comment.
[/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.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed actionable review feedback in commit
|
|
|
||
| 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; |
There was a problem hiding this comment.
Moved in 86b539e: the candidate precheck regex now lives in temporary_id.cjs and push_signed_commits.cjs imports it from there.
|
|
||
| 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; |
There was a problem hiding this comment.
Addressed in 86b539e: regex precheck was moved to temporary_id.cjs and reused from push_signed_commits.cjs.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
| * @param {string} currentRepo | ||
| * @returns {Map<string, {repo: string, number: number}>} | ||
| */ | ||
| function buildTemporaryIdMap(resolvedTemporaryIds, currentRepo) { |
There was a problem hiding this comment.
@copilot review if this function is a duplicate and refactor
There was a problem hiding this comment.
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.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and addressed the open review feedback in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Bundle transport was replaying commit file contents as-is, so
#aw_*placeholders remained in committed code while patch transport correctly substituted them. This change brings parity by applying temporary ID substitution in the signed-commit replay path used by bundle-based pushes.Signed replay now performs file-content substitution
pushSignedCommitsto rewrite temporary ID references in GraphQLcreateCommitOnBranchadditions before mutation submission.Callers now provide substitution context
create_pull_request.cjsandpush_to_pull_request_branch.cjsnow pass:resolvedTemporaryIdscurrentRepoRegression coverage for the failing scenario
push_signed_commits.test.cjsvalidating that replayed file content resolves both:.../issues/#aw_xxx→.../issues/<number>#aw_xxx→#<number>