fix: preserve regular-file modes in push_files - #3410
Open
nateberkopec wants to merge 2 commits into
Open
nateberkopec wants to merge 2 commits into
nateberkopec wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Deep existing paths need a traversal limit to bound sequential GitHub API requests.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates push_files to preserve executable permissions and support explicit regular-file modes, addressing #2578.
Changes:
- Adds optional
100644and100755modes. - Validates paths against the pinned base tree before creating missing branches.
- Adds regression tests and updates documentation and symlink recovery advice.
| File | Description |
|---|---|
| README.md | Documents optional modes and preservation behavior. |
| pkg/github/repositories.go | Adds mode validation and deferred branch creation. |
| pkg/github/repositories_test.go | Updates mocks and recovery-message expectations. |
| pkg/github/repositories_helper.go | Resolves modes and rejects unsupported entries. |
| pkg/github/push_files_test.go | Adds handler-level regression coverage. |
| pkg/github/__toolsnaps__/push_files.snap | Records the updated input schema. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+83
to
+85
| parts := strings.Split(entry.GetPath(), "/") | ||
| mode := "100644" | ||
| for i, part := range parts { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This is AI-assisted. Model: gpt-6.1-sol
Summary
Preserve existing regular-file modes when
push_fileschanges content, and accept an optional per-filemodeof100644or100755. New files without a mode remain100644.Why
Fixes #2578. Updating an executable shell script through
push_filescurrently resets it to100644, which breaks workflows that execute the script directly. This happened while publishing a dotfiles change.Thanks to @why-pengo for the earlier mode-support work in #2579. This proposal adds automatic preservation as well as explicit regular-file modes. It deliberately excludes tree, symlink, and gitlink creation: these cannot safely be represented by the existing blob-with-content input.
What changed
files[].mode, limited to the strings100644and100755, with runtime validation before client acquisition.MCP impact
files[].modeis optional. Existing regular-file updates retain their executable bit by default; explicit mode can add or remove it. New files still default to ordinary mode. Writes that would replace special entries now return an error. No new tool is added.Prompts tested (tool changes only)
No end-to-end model prompts or live GitHub writes were used to validate this patch. Local tests invoke the actual handler with MCP arguments and inspect go-github HTTP requests. Covered use cases correspond to:
Security / limits
Uses the existing authenticated client and Git APIs; no new credentials or authorization mechanism. Mode lookup adds tree reads, cached by SHA within the request.
Traversal does not follow symlinks and rejects incomplete lookup results. Only regular-file modes are accepted. Untouched special entries remain inherited from the base tree.
This is not a transactional rollback change: existing empty-repository initialization still writes an initial README before tree resolution, and failures in later Git API mutations can leave created objects or a newly created branch.
Tool renaming
Lint & tests
./script/lintImplementer ran the script; independent reviewer also ran golangci-lint 2.14.0 (zero issues) and checked formatting.
./script/testFull race suite passed for the implementer and independent reviewer.
Also passed focused PushFiles/CreateOrUpdateFile tests,
UPDATE_TOOLSNAPS=true go test ./...,go test -v ./..., andscript/generate-docs. The independent reviewer found no P0/P1/P2 defects. A preserved baseline regression demonstrated 100644 instead of expected 100755 before the fix.npm auditwas not run; no UI or dependency files changed. Tests establish local handler/SDK behavior, not the revision or behavior of the hosted MCP service.Docs
Regenerated README parameter documentation and the push_files tool snapshot.