Skip to content

fix: preserve schema-twin bytes during Git checkout - #3120

Open
wozhenbang2004 wants to merge 7 commits into
modelcontextprotocol:mainfrom
wozhenbang2004:fix-schema-twin-bytes
Open

wozhenbang2004 wants to merge 7 commits into
modelcontextprotocol:mainfrom
wozhenbang2004:fix-schema-twin-bytes

Conversation

@wozhenbang2004

@wozhenbang2004 wozhenbang2004 commented Oct 29, 2026 •

Copy link
Copy Markdown

Preserve the raw schema-twin JSON bytes when Git checks out the repository with core.autocrlf=true.

Motivation and Context

Addresses the schema-twin CRLF failure in #3117 only. The symlink, signal, and repository-wide Prettier cases remain separate. The provenance manifest intentionally pins the upstream bytes, so these two artifacts should bypass Git text conversion.

How Has This Been Tested?

On Linux with Git 2.51.1 and Node 24.22.1, a fresh core.autocrlf=true sparse checkout failed both manifest byte-count and SHA-256 checks:

  • 2026-07-28: 185437 bytes instead of 181474.
  • 2025-11-25: 178384 bytes instead of 174326.

After adding the attribute and materializing a new checkout with git checkout-index, both byte counts and hashes matched exactly. git diff --cached --check passed. An independent isolated Git reproduction confirmed the result and that unrelated text still converts normally.

A native Windows run passed on Windows Server 2025 with Git 2.56.0.windows.2, PowerShell 7.6.7, and Node 24.22.1. The test clones explicitly used core.autocrlf=true. A stable-stat old checkout showed that checkout-index --force --index can leave the CRLF files untouched. Moving the two files to an external backup directory before checkout-index --index restored both pinned sizes and hashes. The checks also verified backup preservation, unchanged staged contents, unrelated staged/unstaged edits, and a fresh checkout.

The same run passed all 180 tests in the unchanged schemaTwinConformance.test.ts, using the exact corpus and source-locked dependencies in an isolated harness. The source was 0e2bb6d; the subsequent commit only updates the recovery documentation. This was not the full SDK suite. Prettier 3.6.2 passed for the changed files using the project configuration; package builds and the full lint suite were not run locally.

Existing checkouts

Pulling this change may leave previously checked-out CRLF copies unchanged. Move the two schema files listed below to a backup directory outside the repository, then run this from the repository root:

git checkout-index --index -- packages/core-internal/test/corpus/schema-twins/2025-11-25.schema.json packages/core-internal/test/corpus/schema-twins/2026-07-28.schema.json

This recreates the files from the index without changing the staged contents. Keep the backups if they contain local edits. Fresh clones do not need this step.

Breaking Changes

None. Only schema-twins/*.schema.json opts out of text conversion. No schemas, manifest entries, assertion logic, APIs, or dependencies change. The test messages now name the affected file and point to checkout recovery instructions in CONTRIBUTING.md.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally (full suite not run; focused verification above)
  • I have added appropriate error handling (not applicable to Git attributes)
  • I have added or updated documentation as needed

Additional context

Developed with Codex assistance.

@wozhenbang2004
wozhenbang2004 requested a review from a team as a code owner October 29, 2026 04:39
@changeset-bot

changeset-bot Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 1a27d35

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3120

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3120

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3120

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3120

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3120

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3120

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3120

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3120

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3120

commit: 1a27d35

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Oct 29, 2026
@arhancanli

Copy link
Copy Markdown

Checked the attribute rule in a scratch repo: with the same path pattern, git check-attr -a packages/core-internal/test/corpus/schema-twins/x.schema.json reports text: unset. The directory has no other .gitattributes in the way (the repo root has none, and the dir holds the two *.schema.json files plus manifest.json), so the two pinned files are covered and manifest.json still converts normally, which is fine since only the schema bytes are hashed.

One thing the PR text doesn't cover: existing Windows clones with core.autocrlf=true already hold CRLF copies, and the attribute only changes what a fresh checkout writes. After pulling this, those clones need git rm --cached plus checkout of the two files (or a re-clone) before the byte-count test passes. A line in the PR description or CONTRIBUTING would save the next person a confusing failure. Does the provenance test also hash manifest.json itself, or only the two schemas?

@wozhenbang2004

Copy link
Copy Markdown
Author

Thanks for checking this. The test hashes only the two schema files; manifest.json is parsed for the expected sizes and hashes.

I've added an Existing checkouts section with the targeted git checkout-index --force --index command and a note to back up local edits first.

I also verified this on Windows using the exact files from this PR in an isolated repo. The attribute-only pull left the old CRLF copies unchanged, and the refresh restored both pinned hashes and sizes without changing the index contents. The working tree was clean, and the fresh-checkout case passed too.

This ran on Windows Server 2025 with Git 2.56.0.windows.2. It was a focused checkout test, not the full SDK suite.

@arhancanli

Copy link
Copy Markdown

That covers the gap. The Windows run is the right evidence: the attribute-only pull leaving the old CRLF copies in place, then the refresh restoring both hashes and sizes, is the exact sequence an existing clone goes through. The command is scoped to the two paths, so it won't touch anything else in the working tree. One suggestion: if the byte-count test fails, could its assertion message name the two files and point at this "Existing checkouts" section? Someone on an old clone will see the failure before they read the PR text. Does the test's current message say which of the two files differs?

Updated error messages for byte size and SHA256 checks to include additional context regarding CRLF checkouts and re-vendor instructions.
@wozhenbang2004

Copy link
Copy Markdown
Author

The errors now include the full schema filename rather than just the revision, and point to the recovery steps in CONTRIBUTING.md. The byte-length and hash checks are unchanged.

@arhancanli

Copy link
Copy Markdown

The new messages read well: the filename plus a pointer to the schema-twin-checkout-recovery anchor is what someone with a CRLF clone needs at the moment the test fails. One small thing: the byte-size message says "the vendored twin was rewritten", which is also true for a deliberate edit, so a contributor who changed the schema on purpose gets sent to a CRLF recovery section. Would it be worth a short clause such as "if you did not edit it, this is usually line-ending conversion"? Do you know yet whether the Windows job in CI will run this test, or only your fork run?

@wozhenbang2004

Copy link
Copy Markdown
Author

I've clarified that the recovery hint applies when you haven't edited the file and have core.autocrlf=true.

The upstream test jobs currently run on Ubuntu. The Windows run linked above is from my fork and checks checkout recovery, not the Vitest suite.

Would you prefer a targeted Windows CI check in this PR, or a separate follow-up?

@arhancanli

Copy link
Copy Markdown

Thanks, the clarified scope reads right: .gitattributes fixes the checkout, the hint only matters when someone has autocrlf=true and hasn't touched the file. I'd keep this PR to the attribute plus the message change and leave Windows CI as a separate follow-up. Your fork run already shows the attribute holds on Windows, and a new OS job in the matrix is a cost and runtime decision the maintainers should make on its own, not something bundled into a bug fix. If you open the follow-up, a single Windows job running only schemaTwinConformance.test.ts keeps it cheap. Does the fork run check out with the default Windows autocrlf setting, or did you set it explicitly?

@wozhenbang2004

Copy link
Copy Markdown
Author

I set core.autocrlf=true explicitly in the test clones.

Another Windows run caught a recovery issue: checkout-index --force --index can leave CRLF files untouched when Git's cached stat information matches. The instructions now move the two files to a backup directory before restoring them from the index.

The latest run passed the recovery checks and all 180 tests in schemaTwinConformance.test.ts, run separately with the original corpus and locked dependencies.

@arhancanli

Copy link
Copy Markdown

The stat-cache detail is a good catch: checkout-index --force --index trusting matching cached stat data is exactly the kind of recovery step that looks right and does nothing. Moving the two files aside and restoring from the index sidesteps it, and 180 tests passing from the original corpus with locked dependencies is the evidence this needed. One check on the instructions: after the restore, is there a one-line way for the contributor to confirm it worked, for example git ls-files --eol showing w/lf for both files, so they do not have to rerun the whole suite to find out?

@wozhenbang2004

Copy link
Copy Markdown
Author

Yes, from the repository root:

git ls-files --eol -- packages/core-internal/test/corpus/schema-twins/2025-11-25.schema.json packages/core-internal/test/corpus/schema-twins/2026-07-28.schema.json

Both files should show i/lf w/lf attr/-text after recovery. This checks the line endings without rerunning the suite; the manifest checks still verify the exact bytes. I added the command to the recovery instructions.

@arhancanli

Copy link
Copy Markdown

That is the check I wanted: scoped to the two paths, and i/lf w/lf against attr/-text shows the index copy and the working copy agree without touching the suite. One edge worth a line in the instructions: attr/-text only appears once the .gitattributes change is on the branch the contributor has checked out, so someone recovering on an older branch will see a different attr column and might think the fix failed. Does your Windows run print that column after the attribute-only pull, so the expected output can be quoted for both states?

@wozhenbang2004

Copy link
Copy Markdown
Author

The Windows run doesn't print ls-files --eol; it checks the bytes and hashes, then verifies text: unset after recovery.

I checked the same sequence locally on Linux: after pulling the attribute change, the old copies show i/lf w/crlf attr/-text; after recovery they show i/lf w/lf attr/-text. Without the attribute change, attr/-text won't appear. I've made that branch prerequisite explicit and explained the fields in the recovery section.

@arhancanli

Copy link
Copy Markdown

That covers both states, and the Linux sequence (i/lf w/crlf attr/-text before recovery, i/lf w/lf attr/-text after) is the pair a reader can match line for line. One case left: a contributor who already has core.autocrlf=true and a clean working tree after the attribute pull. git status can look clean while the working copies still hold CRLF, because the attribute change does not renormalise files git thinks are unmodified. Does the recovery section say they need to delete and re-checkout those two files (or run git add --renormalize), or does the Windows check catch it before they get that far?

@wozhenbang2004

Copy link
Copy Markdown
Author

Yes, that’s covered in the recovery section. It says to move the two files to an external backup directory before running git checkout-index --index for those paths, avoiding the cached-stat shortcut without changing staged contents. The Windows run reproduces the old command leaving CRLF bytes untouched and verifies recovery against both manifest hashes. I’d avoid git add --renormalize here: it stages working-tree content rather than restoring the pinned bytes.

@arhancanli

Copy link
Copy Markdown

Thanks, the backup-then-checkout-index --index route makes sense, and agreed that --renormalize would stage the working-tree bytes instead of restoring the pinned ones. Question on the recovery section: if someone runs it with core.autocrlf=true on Windows, does checkout-index re-apply the CRLF conversion to the restored twin files, and does the manifest hash check then fail? The Windows run you describe may already cover this, so a one-line note on which autocrlf setting it used would settle it.

@wozhenbang2004

Copy link
Copy Markdown
Author

The Windows test clones explicitly use core.autocrlf=true, including during recovery. The -text attribute disables line-ending conversion for these two files, so checkout-index restores their original bytes; the Windows run verifies both hashes after recovery.

@arhancanli

Copy link
Copy Markdown

That settles it: with -text set the autocrlf conversion cannot touch those two files, and a run that verifies both hashes after recovery is the proof I wanted. One more setting worth a line in the section if the run does not already cover it: core.autocrlf=input, which some Windows contributors use. It converts on add but not on checkout, so I would expect the recovery to behave like the false case, but is that what the run shows, or have you only exercised true?

@wozhenbang2004

Copy link
Copy Markdown
Author

The Windows run covers true, not input. With input, Git doesn't convert line endings on checkout; -text also disables conversion for these two files. I haven't run that variant on Windows.

@DoroteoNavarrete

Copy link
Copy Markdown

Tested this on a Windows 11 desktop against full clones of the repo (Git 2.52.0.windows.1, which is where I hit this for #3117):

  • Fresh clone, core.autocrlf=true: both twins are i/lf w/lf attr/-text, and byte size and sha256 match the manifest. Same clone of main as a control: 178384 / 185437 bytes, both hashes fail.
  • Existing CRLF clone that pulls this branch: the twins stay CRLF and git status looks clean, as you describe. Following the recovery section exactly as written brings both back to LF with matching hashes.
  • core.autocrlf=input (the open question above): a fresh clone passes on this branch and also on main. input never converts on checkout, so the bug can't happen there.

I checked sizes and hashes with a small script that does the same two assertions as the test, not the vitest run itself.

Unrelated to this PR's scope: pnpm lint:all still fails on Windows for the same CRLF reason (see my comment on #3117). Whether a repo-wide eol=lf is wanted is up to the maintainers.

@arhancanli

Copy link
Copy Markdown

Thanks, that is a clear scope statement. Since input was not run, I would say so in the recovery section in one line (tested with core.autocrlf=true; input is expected to behave the same because -text bypasses conversion, but is not covered by the CI run). Then a reader who uses input knows which part is measured and which is reasoning. Is the Windows check meant to stay in the PR as a one-off verification, or will it be wired into a recurring workflow later?

@wozhenbang2004

Copy link
Copy Markdown
Author

Thanks, @DoroteoNavarrete, for checking this on a full Windows clone. Your results also cover fresh clones with input; my fork run still only covers true. That run is one-off verification for this fix, not a recurring upstream CI job.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants