-
Notifications
You must be signed in to change notification settings - Fork 2k
chore: update spec.types.ts from upstream #2027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| * | ||
| * Source: https://github.com/modelcontextprotocol/modelcontextprotocol | ||
| * Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts | ||
| * Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63 | ||
| * Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871 | ||
|
Check warning on line 6 in packages/core-internal/src/types/spec.types.2026-07-28.ts
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The nightly workflow that maintains this PR rebuilds its head branch from scratch on every drift detection ( Extended reasoning...What the bug isThe nightly cron in Standard git semantics: the branch is rebuilt as Why the trigger is guaranteed for the life of this PRThe drift gate (line 53) runs Step-by-step failure scenario
Why existing comments don't cover this#3485357818 asks for the manifest refresh to land in this PR and separately suggests the cron should regenerate the twins alongside the anchor; #3487174588 covers the header-only drift churn and the fetch scripts' single-SHA/path-filter defects; the 2026-07-05 comment corrects the target SHA to How to fixOptions, roughly in order of robustness: (a) have the workflow fetch the existing remote branch, regenerate the anchor on top of it, and push only if the regenerated anchor differs from the branch's copy (rather than recreating from main) — preserving extra commits; (b) have the workflow itself run SeverityFiled as a nit: the defective mechanism lives in the pre-existing workflow file, which this diff does not touch, and merging this one-line sync breaks nothing by itself. But it materially corrects the remediation plan two standing review comments prescribe, so it belongs on record as companion/process scoping alongside the other accepted workflow findings on this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Two pre-existing gaps in the fetch-spec-types.ts pipeline that stamps this provenance line (neither introduced by this PR): (1) fetchLatestSHA() resolves the draft anchor's commit via an unauthenticated api.github.com call (60 req/hr shared per runner-IP) and the nightly workflow passes no GITHUB_TOKEN to that step, so intermittent 403 rate-limits fail the job before the drift check and silently skip that night's refresh; (2) the sibling frozen anchor spec.types.2025-11-25.ts:6 records commit 357adac4 while RELEASED_REVISION_PINS pins 0168c57f, so the documented no-arg 'pnpm run fetch:spec-types' produces an unexpected header-only dirty diff on the frozen anchor, contrary to README lifecycle rule 1. Fix by sending 'Authorization: Bearer $GITHUB_TOKEN' when set (and exposing github.token to the fetch step), and by realigning the 2025-11-25 header/pin (content at both commits is verified byte-identical, so a header-only commit suffices). Extended reasoning...Gap 1 — unauthenticated SHA lookup silently skips nightly refreshes
Why it fails: unauthenticated GitHub REST calls are limited to 60 requests/hour per origin IP, and GitHub-hosted runners share a small egress IP pool across all tenants, so the quota is routinely exhausted by other workloads on the same IP. When that happens Impact: fail-safe but silent staleness — no wrong content is ever written, but the nightly refresh is skipped, and cron-failure notifications go only to whoever last touched the workflow file, so repeated skips can go unnoticed while the draft anchor drifts behind upstream (contrary to lifecycle rule 2 in packages/core-internal/src/types/README.md, which says the anchor floats via refresh PRs). Contributors running Step-by-step proof:
Fix: in Gap 2 — frozen 2025-11-25 anchor header disagrees with its governing pinpackages/core-internal/src/types/spec.types.2025-11-25.ts:6 records Upstream Why it's silent: no test cross-checks any anchor header commit against Fix: a one-line alignment — regenerate the 2025-11-25 anchor from the pin so its header records 0168c57f (content unchanged, so this is a header-only commit), or move the pin to 357adac4 if that is the intended freeze point. Optionally add a test asserting each released anchor header's recorded commit equals its pin (and the draft anchor's equals the twins-manifest commit), so provenance-only divergences fail loudly. Why pre-existingThis PR's diff touches only the 2026-07-28 anchor's provenance header. Neither There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing pipeline gap (in .github/workflows/update-spec-types.yml, not this diff): the nightly refresh branch is pushed and the PR created/updated with the default GITHUB_TOKEN, and GitHub suppresses workflow runs for events created by that token — so the pull_request-triggered gate workflows (main.yml, conformance.yml, examples.yml) never run on this or any refresh PR, and the drift-guard comparison suites structurally cannot produce the red-CI signal that README lifecycle rule 3 relies on. Fix by pushing/creating the PR with a GitHub App installation token or PAT (or explicitly dispatching the gate workflows after pr create/edit), or at minimum document that refresh PRs carry no CI verdict and must be validated locally before merge. Extended reasoning...What the bug isThe nightly cron that produces and maintains this PR authenticates every write with the repository's default The code path
Why this defeats the repo's own design
Concrete failure scenario
Why nothing prevents itNo other trigger covers the How to fixIn order of robustness: (a) push the branch and create the PR with a GitHub App installation token or a PAT — pass it as This is pre-existing: the entire mechanism lives in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The nightly workflow refreshes this PR with Extended reasoning...What the bug isThe nightly refresh workflow maintains this PR asymmetrically:
Step-by-step proof
Why nothing prevents it
ImpactCosmetic but user-facing and un-self-healing: the PR list, notifications, and any changelog/merge tooling that consumes PR titles record a filename that does not exist in the tree — misleading for anyone searching history for changes to How to fixFactor the title into a variable shared by both branches and pass it on the edit path, e.g. in gh pr edit "$EXISTING_PR" --title "$PR_TITLE" --body "$PR_BODY"One-line change, no behavior risk. Note: the defective mechanism lives in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing pipeline gap, time-bound (the 2026-07-28 publication date is 18 days out): the 2026-07-28 anchor is draft-tracked ( Extended reasoning...The mechanismThe 2026-07-28 anchor is draft-tracked: The revision's namesake publication date is 2026-07-28 — 18 days from this re-pull. When upstream publishes, Why the existing guards don't close this
Concrete step-by-step scenario
How to fix (folds into the already-planned pipeline hardening)
This is a pre-existing gap — the entire mechanism lives in |
||
| * | ||
| * DO NOT EDIT THIS FILE MANUALLY. Changes will be overwritten by automated updates. | ||
| * To update this file, run: pnpm run fetch:spec-types 2026-07-28 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The two prior comments covering this exact situation (#3485357818's rule-4 remediation and #3487174588's content-noop verification) were written against the previous re-pull (2ffc3fa2), but this PR now records upstream commit 93671a3f in the anchor header — so following the earlier comment's command verbatim (
pnpm fetch:schema-twins 2ffc3fa2...) would stamp the manifests with a third commit and recreate the rule-4 anchor↔manifest provenance skew. Any twin/fixtures manifest refresh landed with this PR must pin 93671a3f (or whatever the anchor records at merge time) instead; re-verified against the new head, the upstream delta between f68d864a and 93671a3f is formatting-only (schema.json hash unchanged, no examples touched), so the close-as-no-op / land-with-manifest-refresh options from the prior comments remain valid.Extended reasoning...
What this corrects
This re-pull moves the anchor's provenance header (packages/core-internal/src/types/spec.types.2026-07-28.ts:6) from
f68d864a813754e188c6df52dcc5772a12f96c63to93671a3f2bac3bc11b0eb6327c2d029e272b2871. The two most recent review comments covering exactly this situation were written against the previous re-pull (2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184) and are now stale in ways that matter for whoever acts on them:pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184… so both manifests record the same commit as the anchor." Following that command verbatim against the current head would stamppackages/core-internal/test/corpus/schema-twins/manifest.json(and, viafetch:spec-examples,test/corpus/fixtures/2026-07-28/manifest.json) with 2ffc3fa2 while the anchor header says 93671a3f — recreating the very anchor↔derived-artifact provenance disagreement that lifecycle rule 4 (packages/core-internal/src/types/README.md, marked OPERATIVE) forbids, just at a different pair of commits.schema/draft/schema.tsandschema.jsonbeing byte-identical between f68d864a and 2ffc3fa2. That specific verification does not automatically carry over to the new head.Re-verification against the new head (93671a3f)
schema/draft/schema.tsdoes change (96,322 → 96,256 bytes, 63 diff lines) — but every changed hunk is pure union-type re-wrapping (multi-line| A | B | C;collapsed to a single line) with zero semantic or JSDoc change. Prettier inscripts/fetch-spec-types.tsnormalizes this away, which is why the regenerated anchor body is byte-identical and this PR's diff is still header-only.schema/draft/schema.jsonat 93671a3f is unchanged: 178,613 bytes, sha25614398c3dd2c66b9c3f6661fc7a7eaa24174952ed1598d0b7f011b686ba5c4c83, exactly matching the hash pinned for the 2026-07-28 twin inschema-twins/manifest.json— so the vendored twin oracle remains content-current.schema/draft/examples, so the fixtures corpus attest/corpus/fixtures/2026-07-28/is also content-current.Concrete step-by-step consequence of following the stale command
pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184.source.commit = 2ffc3fa2.93671a3f.mainafter merge is again in the state rule 4 forbids — anchor and derived twins disagreeing on their upstream commit — only now across three different SHAs (f68d864a in history, 2ffc3fa2 in the manifests, 93671a3f in the anchor). Nothing in CI catches it:schemaTwinConformance.test.tshashes only twin bytes and never cross-checkssource.commitagainst the anchor header.What remains valid, and the recurring-churn point
The prior comments' bottom-line options remain valid for the current head — the content is verified unchanged, so the PR can either be closed as a no-op or landed together with a 2026-07-28-only twin/fixtures manifest refresh, pinned to 93671a3f (or whatever commit the anchor records at merge time), not the SHA quoted in the earlier comment. This is now the second consecutive header-only refresh PR (f68d864a → 2ffc3fa2 → 93671a3f), which reinforces the already-suggested workflow fixes: make the drift check ignore the provenance line (
git diff -I 'Last updated from commit:') or havefetch-spec-types.tsskip the write when the regenerated body is unchanged, so content-neutral upstream touches ofschema/draft/schema.tsstop generating these PRs.Why nit
The PR is already held and both prior findings are already on record; nothing newly breaks. This comment corrects the SHA in the porting/remediation instructions and re-establishes the content-noop verification against the new revision so the maintainer acting on the earlier comments doesn't recreate the provenance skew — the same stale-checklist-correction tier already established on this PR.