Conversation
|
Hey A couple of things to address before this is ready to merge:
If you'd like a hand, here's a prompt to assign to your coding agent:
|
Replace fs.readFileSync (working tree) with git show <sha>:<path> for all three file addition cases (rename, copy, add/modify). Remove unused fs and path imports. Fixes: multi-commit pushes produced wrong file contents for intermediate commits because working tree always reflected HEAD. Resolves #26156 Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c0841d22-80e2-4bd8-995b-019888de359e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes pushSignedCommits replay logic so file contents are read from each commit object being replayed (A/B/C), rather than incorrectly reading from the working tree at the tip commit.
Changes:
- Replace working-tree
fs.readFileSync(...)file reads with per-commitgit show <sha>:<path>reads for rename/copy/add-modify cases. - Remove now-unused
fsandpathimports.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Reads file contents scoped to the commit SHA being replayed, preventing wrong content in intermediate signed commits. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/push_signed_commits.cjs:207
git showfailures are currently silent here:getExecOutput()typically does not throw on non-zero exit codes (it returnsexitCode), so a missing path/blob could result in committing empty or partial content. Please checkexitCode/stderrfor thegit showcall and throw (to trigger the existing fallback) when it’s non-zero, and ensure the base64 is generated from raw stdout bytes (not the UTF-8 string) to avoid corrupting binary files.
const { stdout: fileContent } = await exec.getExecOutput("git", ["show", `${sha}:${filePath}`], { cwd });
additions.push({ path: filePath, contents: Buffer.from(fileContent).toString("base64") });
actions/setup/js/push_signed_commits.cjs:196
- This
git showread has the same raw-bytes issue as the rename path:getExecOutput()yields a UTF-8 string, soBuffer.from(copiedContent)is not byte-faithful for binary files. Please base64-encode the actual blob bytes (collectBufferoutput) rather than re-encoding a JS string.
const { stdout: copiedContent } = await exec.getExecOutput("git", ["show", `${sha}:${copiedPath}`], { cwd });
additions.push({ path: copiedPath, contents: Buffer.from(copiedContent).toString("base64") });
- Files reviewed: 1/1 changed files
- Comments generated: 2
| const { stdout: fileContent } = await exec.getExecOutput("git", ["show", `${sha}:${filePath}`], { cwd }); | ||
| additions.push({ path: filePath, contents: Buffer.from(fileContent).toString("base64") }); |
There was a problem hiding this comment.
This change fixes an important correctness issue (intermediate commits were using working-tree content), but push_signed_commits.test.cjs doesn’t appear to cover the regression scenario (same path modified across multiple commits A→B→C, ensuring A/B payloads use their own blob contents). Adding a test for that case would prevent this from reoccurring.
| const { stdout: renamedContent } = await exec.getExecOutput("git", ["show", `${sha}:${renamedPath}`], { cwd }); | ||
| additions.push({ path: renamedPath, contents: Buffer.from(renamedContent).toString("base64") }); |
There was a problem hiding this comment.
exec.getExecOutput() returns stdout as a UTF-8 string; wrapping that with Buffer.from(str) will corrupt non-text/binary files (and potentially any content with invalid UTF-8). Previously fs.readFileSync() preserved raw bytes. Consider capturing raw stdout bytes (e.g., by running git via a spawn API with encoding: null, or using exec.exec with stdout listeners that accumulate Buffer chunks) and base64-encoding the resulting Buffer so signed commits preserve exact file content.
This issue also appears in the following locations of the same file:
- line 195
- line 206
|
@copilot review comments, add integration tests |
…ed_commits Extract readBlobAsBase64() helper that uses exec.exec with stdout listeners accumulating raw Buffer chunks instead of getExecOutput (which UTF-8 decodes stdout via StringDecoder and corrupts binary files). Update makeRealExec test mock to support listeners.stdout with encoding:null so binary content is preserved in tests. Add regression test verifying that A→B→C multi-commit push sends each commit's own file content, not the working-tree tip. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cc9df24f-9c1d-48e3-bff9-22fb9ab97b4c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in ff19dce:
|
When replaying a series of commits (A → B → C) via
createCommitOnBranch, all commits were getting file contents from the working tree (at C), so commits A and B contained wrong file contents.Changes
actions/setup/js/push_signed_commits.cjs: Replace all threefs.readFileSync(path.join(cwd, filePath))calls (rename, copy, add/modify paths) with a newreadBlobAsBase64(sha, filePath, cwd)helper that usesexec.execwith rawBufferlisteners, scoping each file read to the specific commit being replayed and preserving exact byte content for binary filesfsandpathimportsactions/setup/js/push_signed_commits.test.cjs: Add regression test verifying that a series of commits (A→B→C) each carry their own file content in the GraphQL payload, not the working-tree tip; updatemakeRealExecmock to support rawBufferaccumulation vialisteners.stdout