Skip to content

chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/)#1788

Merged
cliffhall merged 3 commits into
v2/mainfrom
chore/1778-format-gate-ungated-ts
Jul 26, 2026
Merged

chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/)#1788
cliffhall merged 3 commits into
v2/mainfrom
chore/1778-format-gate-ungated-ts

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1778

Problem

Prettier coverage had a gap: clients/web's format:check was scoped to src/ only, and the root format/validate:core covered core/ plus each client's own scope. Nothing prettier-checked:

  • clients/web/server/** — the Node dev/prod backend + Vite config helpers
  • clients/web/vite.config.ts and the other web top-level configs (tsup.runner.config.ts, eslint.config.js, vitest.shims.d.ts)
  • scripts/**/*.mjs — the root build/verify tooling

So formatting in those files was not gated by validate / CI. eslint already reached them (eslint ., plus root lint:core), so this is specifically a prettier gap. Confirmed live: vite.config.ts, scripts/pack-and-verify.mjs, and scripts/verify-build-gate.mjs all had un-normalized formatting before this change.

Changes

  • Webformat/format:check now cover src, server, and top-level *.{ts,js} (a glob, so future top-level configs are auto-included), not just src.
  • Root — new format:scripts / format:check:scripts (prettier … "scripts/**/*.mjs"), folded into the root format and into validate:core (the root-owned gate). Prettier-only — the root has no eslint config for .mjs.
  • Ran prettier --write once to normalize the newly-covered files. Pure formatting — no runtime behavior change.
  • Updated AGENTS.md / README.md to describe the widened scope.

Verification

npm run ci passes green locally (validate → coverage → verify:build-gate → smoke → Storybook).

🤖 Generated with Claude Code

Prettier only covered core/ and each client's src/, leaving the web Node
backend (clients/web/server/**), web top-level configs (vite.config.ts,
tsup.runner.config.ts, eslint.config.js, ...), and the root build/verify
tooling (scripts/**/*.mjs) ungated — so their formatting was never checked
by validate/CI. eslint already reached these via `eslint .`, so this is a
prettier-only gap.

- clients/web format/format:check now cover src, server, and top-level
  *.{ts,js}, not just src.
- New root format:scripts / format:check:scripts for scripts/**/*.mjs,
  folded into the root format and into validate:core.
- Normalized the newly-covered files with prettier --write (pure
  formatting; no runtime change).

Closes #1778

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 26, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 52s —— View job


Review: PR #1788 — format-gate the ungated first-party TS/JS

  • Gather context (diff vs origin/v2/main)
  • Verify the claim "pure formatting — no runtime behavior change"
  • Check the new/changed npm scripts + glob coverage
  • Check docs updates (AGENTS.md / README.md)
  • Post review

Verdict: LGTM — approve. The wiring is correct, the reformat is provably semantics-free, and the docs match the behavior. Two follow-up observations below (neither blocking).


✅ "Pure formatting" — mechanically verified

I didn't take this on faith. For each reformatted file I compared origin/v2/main vs HEAD after stripping all whitespace, normalizing '", and dropping trailing commas:

File Result
clients/web/vite.config.ts identical
clients/web/tsup.runner.config.ts identical
scripts/pack-and-verify.mjs identical
scripts/verify-build-gate.mjs identical
clients/web/eslint.config.js identical modulo added statement semicolons (all ASI-safe: import … from "x";, …]);)
clients/web/vitest.shims.d.ts trailing newline only

node --check also parses all three touched/adjacent scripts/*.mjs cleanly. The 511-line churn in vite.config.ts is entirely quote style + re-indentation of two defineConfig blocks that were mis-indented at base — nothing moved between the coverage.include / exclude arrays or the project definitions.

✅ The gate itself is correctly wired

I ran the repo's pinned prettier (3.8.1) against the newly-gated scopes on this branch:

clients/web:  prettier --check src server "*.{ts,js}"        → All matched files use Prettier code style!
root:         prettier --check "scripts/**/*.mjs" "core/**"  → All matched files use Prettier code style!

So validate will be green, and the gate is real (I confirmed the pre-change state was dirty). Other things that check out:

  • Both globs are quoted in format and format:check, so prettier expands them, not the shell — cross-platform safe and consistent between the write/check pair.
  • There is no prettier config file anywhere in the repo (root or any client) — every scope runs on identical defaults, so widening a scope can't introduce style divergence between the root-owned and client-owned gates. Good.
  • *.{ts,js} does pick up vitest.shims.d.ts (* matches vitest.shims.d), which the diff confirms.
  • format:scripts runs before the cd clients/web in the root format chain, so it executes at the repo root. Correct.
  • AGENTS.md / README.md accurately describe what shipped — no overclaiming in the docs themselves.

🟡 1. The gap isn't fully closed — 11 first-party TS/JS files remain unformatted and ungated

The PR title is "format-gate the ungated first-party TS/JS", but what ships covers web + scripts/ only. Running pinned prettier 3.8.1 over the rest of the first-party surface:

[warn] clients/cli/eslint.config.js
[warn] clients/cli/tsup.config.ts
[warn] clients/cli/vitest.config.ts
[warn] clients/launcher/eslint.config.js
[warn] clients/launcher/vitest.config.ts
[warn] clients/tui/eslint.config.js
[warn] clients/tui/tsup.config.ts
[warn] test-servers/src/modern-tasks.ts
[warn] test-servers/src/test-server-fixtures.ts
[warn] test-servers/src/test-server-http.ts
[warn] vitest.shared.mts
Code style issues found in 11 files.

These are the exact same class of file this PR just fixed for web:

  • cli/tui/launcher top-level configs — their format scopes are src __tests__ (cli, launcher) and src __tests__ index.ts tui.tsx dev.ts (tui), so tsup.config.ts / vitest.config.ts / eslint.config.js fall out, identically to how web's vite.config.ts did.
  • vitest.shared.mts — this one is the closest sibling of all: it's at the repo root and is imported by the clients/web/vite.config.ts this PR just gated (vitestSharedPaths(dirname), vite.config.ts:14). format:check:scripts is scripts/**/*.mjs, so it doesn't reach it.
  • test-servers/src/** — first-party TS with no package.json, so no client's scope reaches it and the root's doesn't either.

This is out of the letter of #1778's scope (the issue enumerates exactly the three things this PR fixed), so it's not a change-request. But the recurring lesson from #1769/#1777 that motivated the issue — "the ungated set silently grows, and style has to be matched by hand" — applies verbatim to these 11. Worth a follow-up issue on board #28 rather than leaving the title implying completeness.

Fix this →

🟡 2. scripts/**/*.mjs is extension-narrow, while the web glob is deliberately future-proof

The PR body calls out that web's *.{ts,js} is "a glob, so future top-level configs are auto-included" — but format:scripts pins the single extension .mjs, so a future scripts/foo.js or a scripts/lib/foo.ts would silently re-open the gap that this PR exists to close. scripts/ today is 100% .mjs (8 files + lib/prod-web-server.mjs), so this is latent, not live.

I checked the obvious objection — that prettier hard-errors on a pattern matching zero files ([error] No files matching the pattern were found) — and a brace glob is a single pattern, so it's safe as long as some branch matches:

$ npx prettier@3.8.1 --check "scripts/**/*.{mjs,js,cjs,ts}"
All matched files use Prettier code style!   # exit 0

So "scripts/**/*.{mjs,js,cjs,ts}" is a drop-in for both format:scripts and format:check:scripts (package.json:47-48) with no downside.

Fix this →

Nit (no action needed)

validate:core now runs format:check:scripts, so its name is a little off — it's no longer only about core/. The AGENTS.md update handles this honestly by re-describing it as "the root-owned format + lint gate (#1689, widened in #1778)", which I think is the right call over churning the script name. Mentioning only so it's a conscious decision.


Commands used to verify
# semantics-free check, per file
git show origin/v2/main:$f | tr -d " \t\n\r" | tr "'" '"' | sed 's/,}/}/g; s/,]/]/g; s/,)/)/g'
git show HEAD:$f          | tr -d " \t\n\r" | tr "'" '"' | sed 's/,}/}/g; s/,]/]/g; s/,)/)/g'

node --check scripts/{pack-and-verify,verify-build-gate}.mjs scripts/lib/prod-web-server.mjs

# gate is green on the pinned version
(cd clients/web && npx prettier@3.8.1 --check src server "*.{ts,js}")
npx prettier@3.8.1 --check "scripts/**/*.mjs" "core/**/*.{ts,tsx}"

# remaining gap
npx prettier@3.8.1 --check "clients/{cli,tui,launcher}/*.{ts,js}" \
  "test-servers/src/**/*.ts" "eslint.config.js" "vitest.shared.mts"

Note: an unpinned npx prettier@3 reports 4 additional dirty files under the already-gated clients/web/src — that's newer-3.x default drift, not a problem with this PR. Everything above uses the pinned ^3.8.1.

· Branch: [`chore/1778-format-gate-ungated-ts`](https://github.com/modelcontextprotocol/inspector/tree/chore/1778-format-gate-ungated-ts)

Widen format:scripts / format:check:scripts from "scripts/**/*.mjs" to
"scripts/**/*.{mjs,js,cjs,ts}" so a future non-.mjs file under scripts/
is auto-gated, matching the future-proof glob style clients/web now uses.
A brace glob is a single pattern, so prettier does not error on the
unmatched branches. Addresses observation #2 from the PR #1788 review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the thorough review — especially the mechanical semantics-free verification per file. Responses to the three points:

🟡 2 — extension-narrow scripts/**/*.mjs glob: Fixed in d86d8f7. Widened both format:scripts and format:check:scripts to "scripts/**/*.{mjs,js,cjs,ts}", matching the future-proof brace-glob style clients/web now uses, and updated the AGENTS.md mention. Confirmed the widened glob still passes (matches today's 9 .mjs files, no error on the empty branches).

🟡 1 — the gap isn't fully closed (11 remaining files): Agreed, and as you note this is outside #1778's enumerated scope, so rather than expand this PR I filed #1789 (on board #28, Todo) to cover the rest: cli/tui/launcher top-level configs, test-servers/src/**, and vitest.shared.mts. It mirrors this PR's approach (widen the client globs, add a root format:shared for the no-package.json surface) and captures your exact file list + pinned-prettier note.

Nit — validate:core naming: Conscious decision, as you read it — I kept the script name and re-described it in AGENTS.md as "the root-owned format + lint gate" rather than churn the name (and every reference to it).

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 13s —— View job


Review: PR #1788 — format-gate the ungated first-party TS/JS (round 2)

Verdict: LGTM — approve. d86d8f7 correctly addresses observation #2, and this round I upgraded the "pure formatting" check from a heuristic to a proof (below). One small doc straggler from the follow-up commit, and one small addition to suggest for #1789. Neither is blocking.


✅ "Pure formatting" — now proven, not just heuristically checked

Last round I compared whitespace-stripped/quote-normalized text. That's a good heuristic but it can't rule out a hand edit hidden inside a 511-line reformat. This round I ran the stronger check: for each reformatted file, is HEAD byte-for-byte identical to prettier@3.8.1(origin/v2/main)?

clients/web/eslint.config.js       IDENTICAL to prettier(base)  ✅
clients/web/tsup.runner.config.ts  IDENTICAL to prettier(base)  ✅
clients/web/vite.config.ts         IDENTICAL to prettier(base)  ✅
clients/web/vitest.shims.d.ts      IDENTICAL to prettier(base)  ✅
scripts/pack-and-verify.mjs        IDENTICAL to prettier(base)  ✅
scripts/verify-build-gate.mjs      IDENTICAL to prettier(base)  ✅

All six. So the diff is exactly the output of prettier --write on the base tree — nothing hand-authored slipped in alongside it, and semantics are preserved by prettier's own guarantee rather than by my inspection. That also settles the two files whose churn looked large for a reformat (vite.config.ts at 511 lines, pack-and-verify.mjs at +79/-18). node --check also parses all 9 scripts/*.mjs cleanly.

d86d8f7 — the widened glob is correct and safe

format:scripts / format:check:scripts now use "scripts/**/*.{mjs,js,cjs,ts}" (package.json:48-49). Re-verified empirically with the pinned prettier:

prettier --check "scripts/**/*.{mjs,js,cjs,ts}"   → All matched files use Prettier code style!   exit 0
prettier --check "scripts/**/*.{cjs,ts}"          → [error] No files matching the pattern...     exit 2

That second line is the useful one: a brace glob errors only when the whole pattern matches zero files, not per-branch. So the widened glob is safe today (9 .mjs files match) and stays safe as long as scripts/ isn't emptied. Exactly the drop-in the review asked for.

✅ The gate is real and green

Ran the three scopes verbatim as the scripts define them, on the repo's pinned prettier@3.8.1:

root  format:check:core     "core/**/*.{ts,tsx}"            → clean
root  format:check:scripts  "scripts/**/*.{mjs,js,cjs,ts}"  → clean
web   format:check          src server "*.{ts,js}"          → clean

Also confirmed: no .prettierrc* / prettier.config.* / .prettierignore anywhere in the repo, and no prettier key in any package.json — so every scope runs on identical defaults and widening one can't introduce style divergence between the root-owned and client-owned gates. .github/workflows/main.yml:40 runs npm run validate, which reaches validate:core → the new format:check:scripts, so this is CI-enforced with no workflow change needed. (CI for d86d8f7 was still in progress at review time — run 30185384020.)

Worth noting for the record: clients/web/server/** — one of the three scopes this PR newly gates — was already prettier-clean at base, so it contributes no churn. The whole diff comes from the top-level configs and scripts/.


🟡 1. AGENTS.md:217 still says scripts/**/*.mjs — stale after d86d8f7

The follow-up commit updated the glob in the validate:core bullet (AGENTS.md:220) but not the one two lines above it:

AGENTS.md:217 — "the root format auto-fixes core/ (format:core), the root `scripts/**/*.mjs` (format:scripts), and every client's scope…"

package.json now says {mjs,js,cjs,ts}, so the two AGENTS.md mentions of the same glob disagree with each other. Trivial, but this repo treats AGENTS.md as the contract and explicitly requires docs to track tooling changes, and a stale glob in the pre-push gate section is the kind of thing someone copy-pastes.

Fix this →

🟡 2. #1789 is well-scoped, but misses one file — the repo-root eslint.config.js

I read #1789 and re-ran the gap check; the 11-file list still reproduces exactly, and the proposal (widen cli/tui/launcher to their top-level configs + a root format:shared for test-servers/src/** and vitest.shared.mts) is the right shape. Good follow-up.

One omission: the repo-root eslint.config.js is also ungated — format:check:core is core/**, format:check:scripts is scripts/**, and every client's prettier is scoped to its own dir, so nothing reaches it. It doesn't appear in #1789's file list precisely because it's currently clean, which makes it invisible to a --list-different sweep — and that's exactly how a file becomes the next straggler. Worth folding into the proposed format:shared scope while that issue is open.

Fix this →

Nits (no action needed)

  • JSON/Markdown remain ungated everywhere (tsconfig.base.json, the .md files, every package.json). That's a pre-existing, consistent project choice rather than a gap this PR opens — flagging only so it's not mistaken for an oversight if someone reads "the ungated set" broadly.
  • validate:core naming — agreed with your call to re-describe rather than rename; the AGENTS.md wording ("the root-owned format + lint gate") is honest about it.

Commands used to verify
npm i --no-save prettier@3.8.1     # match the repo's pinned ^3.8.1

# The strong check: is HEAD exactly prettier(base)?
for f in clients/web/{eslint.config.js,tsup.runner.config.ts,vite.config.ts,vitest.shims.d.ts} \
         scripts/{pack-and-verify,verify-build-gate}.mjs; do
  git show "origin/v2/main:$f" > base.$ext
  diff <(prettier base.$ext) <(git show "HEAD:$f")
done

node --check $(find scripts -name '*.mjs')

# The gate, exactly as the scripts define it
prettier --check "core/**/*.{ts,tsx}"
prettier --check "scripts/**/*.{mjs,js,cjs,ts}"
(cd clients/web && prettier --check src server "*.{ts,js}")

# Brace-glob semantics: errors only if the WHOLE pattern matches nothing
prettier --check "scripts/**/*.{cjs,ts}"    # → exit 2, "No files matching the pattern"

# Remaining gap (#1789) still reproduces at 11 files
prettier --list-different "clients/{cli,tui,launcher}/*.{ts,js,mts,mjs}" \
  "test-servers/src/**/*.ts" "eslint.config.js" "vitest.shared.mts"

# No prettier config/ignore anywhere → all scopes share defaults
find . -name ".prettierignore" -o -name ".prettierrc*" -not -path "*/node_modules/*"
· Branch: [`chore/1778-format-gate-ungated-ts`](https://github.com/modelcontextprotocol/inspector/tree/chore/1778-format-gate-ungated-ts)

d86d8f7 widened the scripts glob to {mjs,js,cjs,ts} and updated the
validate:core bullet, but the format bullet at AGENTS.md:217 still said
scripts/**/*.mjs. Bring it in line so the two mentions agree.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — and appreciate the upgrade from heuristic to byte-for-byte prettier(base) == HEAD proof. Both round-2 items addressed:

🟡 1 — stale scripts/**/*.mjs at AGENTS.md:217: Fixed in e7fe2b1. I'd updated the validate:core bullet (line 220) but missed the format bullet two lines up; both now read {mjs,js,cjs,ts}, matching package.json. Confirmed no other .mjs-only mentions remain in AGENTS.md / README.md.

🟡 2 — #1789 misses repo-root eslint.config.js: Good catch on the invisible-because-clean straggler. Added it to #1789's scope (folded into the proposed format:shared alongside test-servers/src/** and vitest.shared.mts), with a note on exactly why it didn't show up in the --list-different sweep.

Nits (JSON/MD ungated as a consistent project choice; validate:core naming) — noted, no action, agreed.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 4m 1s —— View job


Review: PR #1788 — format-gate the ungated first-party TS/JS (round 3)

  • Gather context (full 3-commit diff vs origin/v2/main, incl. e7fe2b1)
  • Re-verify "pure formatting" byte-for-byte across all 6 reformatted files
  • Verify the r2 doc fix — AGENTS.md glob consistency
  • Run the gate scopes on the locked prettier, not just the caret floor
  • Check glob safety, scope overlap, and idempotency
  • Post review

Verdict: LGTM — approve. Both r2 items are correctly addressed, and this round I re-ran the verification against the prettier version CI actually resolves (3.8.4) rather than the caret floor (3.8.1) the earlier rounds used. Still green. One new latent observation below, which is pre-existing and not introduced by this PR — flagging it precisely because this round's version check is what surfaced it.


e7fe2b1 — the doc straggler is fixed, and there are no others

AGENTS.md:217 now reads scripts/**/*.{mjs,js,cjs,ts}, matching AGENTS.md:220 and package.json:48-49. I swept both docs for any remaining .mjs-only mention of the glob:

$ grep -n '\*\.mjs' AGENTS.md README.md
(no matches)

Clean. README's format:scripts sentence describes the scope generically ("the root scripts/ tooling"), so it can't go stale on an extension change.

✅ "Pure formatting" — proof re-run on the final tree

Re-ran the strong check (HEAD byte-identical to prettier(origin/v2/main)) against the three-commit tip, not just the first commit:

clients/web/eslint.config.js       IDENTICAL to prettier(base)  ✅
clients/web/tsup.runner.config.ts  IDENTICAL to prettier(base)  ✅
clients/web/vite.config.ts         IDENTICAL to prettier(base)  ✅
clients/web/vitest.shims.d.ts      IDENTICAL to prettier(base)  ✅
scripts/pack-and-verify.mjs        IDENTICAL to prettier(base)  ✅
scripts/verify-build-gate.mjs      IDENTICAL to prettier(base)  ✅

node --check clean on all 9 scripts/*.mjs. The only non-formatting changes in the whole diff are the four package.json script lines and the two doc files — I read each; they match what the PR body claims.

✅ New this round: verified on the locked prettier (3.8.4), not just 3.8.1

Rounds 1–2 pinned prettier@3.8.1 — the floor of the ^3.8.1 range. But all five package.jsons carry a caret and both lockfiles resolve 3.8.4, and .github/workflows/main.yml:29 runs npm install (lockfile-honoring), so 3.8.4 is what CI actually executes. Re-ran all three gate scopes on it:

prettier@3.8.4 --check "core/**/*.{ts,tsx}"            → clean
prettier@3.8.4 --check "scripts/**/*.{mjs,js,cjs,ts}"  → clean
(cd clients/web && prettier@3.8.4 --check src server "*.{ts,js}")  → clean

Green on the version that matters. Also confirmed idempotency: running the root npm run format:core && npm run format:scripts on HEAD with 3.8.4 leaves git status --porcelain empty.

✅ Glob safety and scope disjointness

  • clients/web's top level has no .mts/.cts/.jsx files, and every build artifact is a directory (build/, dist/, coverage/, storybook-static/) — so *.{ts,js} matches exactly the committed configs (vite.config.ts, tsup.runner.config.ts, eslint.config.js, vitest.shims.d.ts) and can't pick up generated output.
  • scripts/ is 9 .mjs files including lib/prod-web-server.mjs; the ** reaches the nested one, and prettier skips node_modules by default.
  • The three scopes are disjoint (core/** · scripts/** · clients/web/{src,server,*.{ts,js}}), so no file is formatted by two different prettier binaries — which matters here because the root and web each resolve their own copy.

🟡 Pre-existing (not this PR): the ^3.8.1 caret is a latent gate landmine — and the newly-gated scopes are the safe ones

Round 1 noted in passing that "an unpinned npx prettier@3 reports 4 additional dirty files" and set it aside. Having now checked which scopes those live in, it's worth recording properly, because the answer is favorable to this PR:

Current prettier@latest is 3.9.6, which satisfies ^3.8.1 in all five package.jsons. Under it:

newly-gated by this PR:
  scripts/**/*.{mjs,js,cjs,ts}     → 0 dirty
  clients/web/server + *.{ts,js}   → 0 dirty

pre-existing scopes:
  core/**            → 6 dirty (challenge.ts, connection-state.ts, oauthUx.ts,
                                __tests__/fakeInspectorClient.ts, types.ts, urlElicitation.ts)
  clients/web/src    → 4 dirty (fetchRequestLogState.test.ts, mcp-app-demo.test.ts,
                                navigation.ts, oauthNetworkPhase.ts)

So this PR's additions are drift-free; the exposure is entirely in scopes that were already gated before it. It doesn't bite today (npm install honors the 3.8.4 lockfile), but a npm update, a dependabot bump, or a regenerated lock would fail format:check:core and web's format:check on files nobody touched. Not a change-request on #1788 — but this PR is about making formatting gates trustworthy, and a gate whose verdict depends on which in-range patch happens to be installed isn't quite that. Pinning prettier exactly (or bumping to 3.9.x and reformatting once) would close it; a natural companion to #1789.

Fix this →

Nits (no action)

  • vitest.shims.d.ts gained a trailing newline via the *.{ts,js} glob (* matches vitest.shims.d). Intentional per the PR body and confirmed harmless — it's a single /// <reference …> line.
  • clients/web/server/** contributed zero churn — it was already prettier-clean at base. The whole 511-line vite.config.ts diff and the scripts/ churn are the entire cost of this PR; server/ is a free gate widening.
  • validate:core naming and the ungated JSON/Markdown surface — both settled in earlier rounds, no change.

CI

The workflow run for e7fe2b1 (30185537642) was still in_progress at review time. My local runs of the three format:check scopes on the locked 3.8.4 all pass, so the format step should be green; the rest of the chain is untouched by this PR.


Commands used to verify
# byte-for-byte: is HEAD exactly prettier(base)?
npm i --no-save prettier@3.8.1
for f in clients/web/{eslint.config.js,tsup.runner.config.ts,vite.config.ts,vitest.shims.d.ts} \
         scripts/{pack-and-verify,verify-build-gate}.mjs; do
  git show "origin/v2/main:$f" > base.tmp
  diff <(prettier base.tmp) <(git show "HEAD:$f") && echo "IDENTICAL $f"
done

# which prettier does CI actually run?
node -e "const l=require('./package-lock.json');console.log(l.packages['node_modules/prettier'].version)"  # 3.8.4
grep -n 'npm install' .github/workflows/main.yml                                                            # :29, lockfile-honoring

# the gate on the LOCKED version
npm i --no-save prettier@3.8.4
prettier --check "core/**/*.{ts,tsx}"
prettier --check "scripts/**/*.{mjs,js,cjs,ts}"
(cd clients/web && prettier --check src server "*.{ts,js}")

# idempotency on HEAD
npm run format:core && npm run format:scripts && git status --porcelain   # empty

# version-drift blast radius
npm i --no-save prettier@3.9.6
prettier --list-different "scripts/**/*.{mjs,js,cjs,ts}"                  # 0
(cd clients/web && prettier --list-different server "*.{ts,js}")          # 0
prettier --list-different "core/**/*.{ts,tsx}"                            # 6
(cd clients/web && prettier --list-different src)                         # 4

# doc consistency + syntax
grep -n '\*\.mjs' AGENTS.md README.md                                     # no matches
node --check $(find scripts -name '*.mjs')
· Branch: [`chore/1778-format-gate-ungated-ts`](https://github.com/modelcontextprotocol/inspector/tree/chore/1778-format-gate-ungated-ts)

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — and appreciate the round-3 rigor: re-running the byte-for-byte proof on the three-commit tip, verifying on the locked 3.8.4 (not just the caret floor), and the idempotency check.

🟡 prettier ^3.8.1 version drift: Agreed this is worth closing, and as you note it's pre-existing and doesn't touch anything this PR adds (the newly-gated scopes are drift-free). Since it's out of #1778's scope, I filed #1790 (board #28, Todo) — captures the 10-file blast radius under 3.9.6, that CI is green today via the 3.8.4 lockfile, and both remediation options (pin exact, or bump-and-reformat). Companion to #1789.

Nits (trailing newline on vitest.shims.d.ts; server/** zero-churn; validate:core naming; ungated JSON/MD) — all noted, no action.

That's three consecutive clean approvals with the only remaining observations pre-existing and now tracked (#1789, #1790), so I'll consider the review converged. Thanks for the thorough passes.

@cliffhall
cliffhall merged commit 166ed38 into v2/main Jul 26, 2026
3 checks passed
@cliffhall
cliffhall deleted the chore/1778-format-gate-ungated-ts branch July 26, 2026 03:15
cliffhall added a commit that referenced this pull request Jul 26, 2026
…1790 #1767) (#1792)

* chore: close a batch of quality-gate & tidy gaps (#1785 #1787 #1789 #1790 #1767)

Bundles five small, related tidy/gate items into one PR:

- #1785: delete the dead src/types/navigation.ts InspectorTab (0 importers;
  duplicated/drifted from utils/inspectorTabs InspectorTabId). src/types/
  now holds only ambient .d.ts stubs.
- #1787: add src/hooks/** to the web coverage `include` (it was silently
  outside the ≥90 gate) and add tests for the two untested hooks,
  useServerJsonImport and useImportClientConfig, bringing every hook to
  ≥90 on lines/statements/functions/branches.
- #1789: widen clients/{cli,tui,launcher} format/format:check to their
  top-level configs; add root format:shared / format:check:shared covering
  test-servers/src/**, vitest.shared.mts, and the repo-root eslint.config.js.
- #1790: pin prettier to an exact version (3.8.4, dropping the ^3.8.1
  caret) in all five package.json files so the format gate's verdict can't
  shift with an in-range patch bump; lockfiles synced.
- #1767: its root-file format+lint gap is closed here (format:shared +
  a new lint:shared via a second files block in the root eslint.config.js;
  web/server/ was already gated in #1788). Its larger cli/tui __tests__
  typecheck piece (76 type errors) is split out to #1791.

Prettier-normalized the newly-covered files (pure formatting). No runtime
behavior change. `npm run ci` green.

Closes #1785
Closes #1787
Closes #1789
Closes #1790
Closes #1767

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* chore: address PR #1792 review findings

- #1789 gap: widen clients/web format to include .storybook and clients/
  launcher format to include scripts/; reformat the 3 .storybook files
  (were non-conforming under the pinned prettier 3.8.4). Audited all 885
  tracked source files — now fully prettier-clean.
- Export VALIDATE_DEBOUNCE_MS / COLLAPSE_DELAY_MS / HIGHLIGHT_DURATION_MS
  from useServerJsonImport and use them in the test instead of hardcoded
  300/1000/250 (so a tuned constant can't silently desync the test).
- Assert the symmetric renameErrors["alpha"] alongside ["beta"] so a
  one-directional collision-check regression fails.
- Comment the blank-rename test as exercising a UI-unreachable defensive
  branch (canImport gates submit).
- Un-stale the two specification/v2_ux_interfaces_plan.md references to the
  now-removed navigation.ts InspectorTab.

npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* docs: sync AGENTS.md format-surface description with the widened globs (#1792 review r2)

1ce3b64 added .storybook to clients/web's format glob and scripts/ to
clients/launcher's, but AGENTS.md:222 still described the pre-fix scope.
Bring it in line and note the non-recursive top-level glob is why nested
dirs are named explicitly. Docs only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* chore: handle PR #1792 round-3 follow-ups in-place (format-coverage guard, uniform globs, theme gate)

Rather than defer these to tickets, fold them into this PR:

- Obs 2: widen every client + root format glob to a uniform
  *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} so a future .mts/.cjs/.tsx top-level
  file can't silently slip the gate.
- Obs 1: add scripts/verify-format-coverage.mjs (wired into `validate`) —
  it parses the format:check globs out of every package.json, enumerates
  all tracked source files, and fails listing any not covered by a glob.
  The durable, standing version of #1789's one-shot audit; negative-tested.
- Obs 3: bring src/theme/** under the coverage gate (21/23 files were
  already ≥90 transitively; add Paper.test.tsx for the one laggard,
  Paper.ts). src/App.tsx (~4.5k lines, ~42% branches) is converted from a
  silent omission into an explicit, documented exclusion (comment on the
  coverage `include` + AGENTS.md) — gating it is a dedicated effort, not a
  whitelist tweak.

npm run ci green (the new guard reports 885/885 tracked source files gated).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* chore: address PR #1792 round-4 findings (guard hardening, docs, exclusion list)

- Harden verify-format-coverage: only harvest format:check globs from
  scripts transitively reachable from `validate` (via npm run refs), so an
  unwired/renamed format:check no longer counts as coverage. Negative-tested
  — renaming clients/web format:check to an unreferenced name now fails the
  guard (627 files flagged) instead of passing.
- Name src/main.tsx and src/index.ts (browser + bin bootstraps) alongside
  src/App.tsx in the coverage-include comment and AGENTS.md, so the
  documented exclusion list is complete rather than claiming App.tsx is the
  only one.
- Doc sync: document verify:format-coverage (AGENTS.md + README script table
  + scripts/ layout line), fix the `validate` ordering (verify:format-
  coverage now runs before validate:core) in AGENTS.md and README, and
  update the quoted globs to the uniform *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} /
  test-servers *.{ts,tsx,mts,cts}.

npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* chore: address PR #1792 round-5 findings (airtight guard + doc/config sync)

- verify-format-coverage: assert the root `validate` chain invokes each
  client's `validate` (cd <dir> && npm run validate), closing the last
  reachability edge — a client dropped from the root chain now fails the
  guard even though its own globs still exist. Negative-tested.
- eslint.config.js: widen the shared `files` block to
  test-servers/src/**/*.{ts,tsx,mts,cts} to match the widened lint:shared
  glob, so a future .mts/.cts there is lint-gated, not just prettier-gated.
- AGENTS.md: scope the "uniform extension set" claim to client globs
  (core stays {ts,tsx}, shared {ts,tsx,mts,cts}), with verify:format-coverage
  as the backstop — removing the self-contradiction with the line below.

npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/)

1 participant