fix(repos): give create_or_update_file callers a SHA they can actually get - #3131
fix(repos): give create_or_update_file callers a SHA they can actually get#3131dylanpulver wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…y get The create_or_update_file tool description and both of its SHA errors told the caller to run `git rev-parse <branch>:<path>`. The caller is an MCP client talking to the GitHub API, and the same description tells it not to use this tool for local file operations, so it has no working tree to run that command against. Point the description at get_file_contents instead, which returns the blob SHA over the API. In the already-exists error the server has just fetched the file, so return that SHA directly rather than asking for a round trip. The stale-SHA error already interpolates the current SHA, so it only needed the impossible instruction removed.
|
Hi, so the reason this protection exists is so that the model is updating a version of a file that it is expecting to update. Race conditions occur where two users update a file at same time and second one undoes the work. Just returning the head sha of the file is risky. I'd it wasn't we'd just update the file and not error. Get file content returns the SHA so the model can use it. I'm ok with an error that doesn't assume git access, but the model needs to know what version of a file it's actually editing and refetch it if it's changed since. FWIW I see you do cover this, and I will hopefully look fully soon. |
Sorry, something went wrong.
The already-exists path is reached only when the caller sent no sha, so it has not read the file. Returning the current blob SHA there let it overwrite content it never saw on the next call, which is the race the SHA gate exists to prevent. Send the caller to get_file_contents for the path and ref instead, so obtaining the SHA still requires reading the file. The stale-SHA path is unaffected: the caller did supply a sha, and that message already reported the current SHA before this change. Only its recovery step moved off git rev-parse, and it is now imperative rather than conditional, since a stale SHA means the file definitely changed. Assert the already-exists error does not contain the blob SHA so the gate cannot be loosened again without a test failing.
|
There are two error paths here and only one of them loosens anything. Splitting them: Stale SHA, where the caller did supply a sha: main already interpolated the current SHA into that message ("Current file SHA is %s") before this PR, and it still does. The only thing this PR changed on that path was the recovery step, from git rev-parse to get_file_contents. No new information is disclosed. File exists and no sha was supplied: your objection lands. A caller on that path has not read the file, and handing it the SHA lets it overwrite content it never saw in one retry. That is a real weakening of the gate and I have taken it back out. The message now sends the caller to get_file_contents for the path and ref and tells it to retry with the SHA that call reports, so obtaining the SHA still requires reading the file, which is what the gate was buying. Same protection as main, without the local-git instruction that #3130 is about. I also made the stale-SHA recovery imperative instead of conditional. It read "re-read the file with get_file_contents if you need its latest content"; the file definitely changed, so the re-read is not optional. It now says to re-read, rebuild the content against what comes back, and retry with the SHA that call reports. Test side: added an unexpectedErrMsgs field to the Test_CreateOrUpdateFile table and asserted the already-exists error does not contain the blob SHA, so the gate cannot be loosened again without a test going red. The existing assertion that no error path emits git rev-parse stays. go test ./... passes, go vet ./... is clean, and create_or_update_file.snap is unchanged since the tool description did not move again. |
Sorry, something went wrong.
Summary
create_or_update_file told its caller to run git rev-parse <branch>:<path> to get a blob SHA, which an MCP client has no way to do. This replaces that instruction with the SHA itself where the server has it, and with a pointer to get_file_contents where it does not.
Why
Fixes #3130
git rev-parse reads a local object database, and a client using this tool over the API has no clone. The instruction appears in both SHA error paths, so an agent hitting either is handed a recovery step it cannot perform. On the already-exists path the handler has already called client.Repositories.GetContents and is holding existingFile, so the SHA the caller is being sent away to fetch is in scope.
What changed
I grepped the package for other local-git instructions in tool-facing strings; these three were the only hits outside Dockerfile and script/.
#2772 rewrites this function to commit through createCommitOnBranch, keeps the SHA validation block, and touches neither the description nor either error string. Cherry-picking this onto pull/2772/head applies cleanly except for two test-table entries, and only because #2772 branched from a June main predating the expectedErrMsgs field; it reports as conflicting with main today and needs that rebase regardless. Happy to rebase on top of it if it lands first.
MCP impact
Description and the two SHA error messages changed. Inputs, outputs and success behavior are untouched.
Prompts tested (tool changes only)
Read against the mocked handler paths in Test_CreateOrUpdateFile rather than a live token, since I do not have an e2e PAT for this repo.
Security / limits
The already-exists error surfaces a blob SHA the server just read with the caller's own credentials on that same call. A caller who could not read the file would have received the GetContents error instead.
Tool renaming
Lint & tests
./script/lint reports 0 issues., ./script/test passes across all packages, go vet ./... clean, all green on a clean main baseline too. The new assertions fail against unmodified repositories.go. Not run: e2e/, which needs GITHUB_MCP_SERVER_E2E_TOKEN.
Docs
script/generate-docs produces no diff. README lists the tool title and parameters, not the description body.