From 2c677041c70a80fcab817730ee09e01520bb6863 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 01:00:24 -0400 Subject: [PATCH 1/7] ci: run the DCO check on every v2 PR now, before push, and after merge (#2616) dco.yml ran on pull_request_target, which GitHub reads from the default branch, so the check reported on no v2 PR until a milestone merge carried the file to main. Unsigned commits would have surfaced only then. - PR job on pull_request, so it works as soon as it is on v2/main. Any base except main and v1/main, so stacked PRs are checked too. Still runs the script from the base and only fetches the PR head. PR-keyed concurrency with cancel-in-progress. - New push job over before..after on v2/main, as a post-merge backstop. - local:dco runs the script over origin/v2/main..HEAD as the gate's second stage, kept out of validate (CI) and pinned by workflow-gate.test.mjs. - dco-check: a GitHub-committed (squash-merge) commit passes on an author email match, since GitHub writes its author name from the profile. Four squash merges on v2/main would otherwise fail the push job. - Docs: workflow header, dco-check header, pr-flow step 3, pre-push-gate, docs/quality-gate.md. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 13 ++- .claude/skills/pre-push-gate/SKILL.md | 9 ++ .github/workflows/dco.yml | 121 +++++++++++++++++++------- docs/quality-gate.md | 3 +- package.json | 3 +- scripts/dco-check.mjs | 32 +++++-- scripts/dco-check.test.mjs | 41 +++++++++ scripts/lib/workflow-gate.test.mjs | 19 ++++ 8 files changed, 199 insertions(+), 42 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 9827b44dd..71654177d 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -104,14 +104,19 @@ repair below. ⚠️ **It is a merge gate only because it is a _required_ status check** — a ruleset setting, not something the workflow file can declare. The job runs on -`pull_request_target`, so its workflow is read from `main`: it reports on PRs -only once a milestone merge has carried it there (#2566). The probot DCO app +`pull_request`, from the PR's own ref, so it reports on every v2 PR — stacked +ones included, any base but `main` and `v1/main` — with no wait for a +milestone merge (#2616). A second job, `DCO (v2/main push)`, +re-runs the check over every push that lands on `v2/main` — a backstop for +anything that merged without a passing PR check — so a red there means an +unsigned commit is already on the branch. The probot DCO app it replaced was never required, so when the app was suspended its check simply stopped appearing (after #1981) and nothing went red for two months. If the `DCO` check is ever missing from a PR, treat that as the outage it is. -**Check before you push** — the same script runs locally against the range the -PR will show: +**Check before you push.** `npm run local:gate` already does it: its +`local:dco` stage runs the same script over `origin/v2/main..HEAD`, right after +`local:validate`. To check on its own, against the range the PR will show: ```sh npm run dco:check -- --base origin/v2/main diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 33d7f1ab5..990474e42 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -98,6 +98,15 @@ stale install otherwise passes every check and fails later as a behavioral test reporting the *old* dependency's behavior as a product bug (#2494). Don't "fix" that test. +### `local:dco` + +A commit in `origin/v2/main..HEAD` has no `Signed-off-by:` trailer matching its +author or committer (#2616). The output names each commit and prints the +repair. Repair and prevention (`git commit -s`, the `--signoff` rebase) are +step 3 of `/pr-flow`. If it lists commits you never made, your `origin/v2/main` +is probably behind what the branch was cut from: `git fetch origin v2/main` and +re-run. + ### `verify:action-pins` A job that holds a credential (`id-token`/`packages: write`, a non-default diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index 6b03cc9ea..58f968eb7 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -3,48 +3,64 @@ # The app was suspended and its check silently stopped appearing after #1981 # (2026-08-12). Nothing went red, because it was never a required check — so # for two months "the DCO check is a hard merge gate" was true only by habit. -# This job is the repo-owned replacement: `scripts/dco-check.mjs` fails the PR -# unless every commit in it carries a `Signed-off-by:` matching its author or -# committer (merge and bot-authored commits exempt — the app's rule). +# This workflow is the repo-owned replacement: `scripts/dco-check.mjs` fails +# unless every commit in a range carries a `Signed-off-by:` matching its +# author or committer (merge and bot-authored commits exempt — the app's rule). # -# ⚠️ A job only GATES when its check is required. Making `DCO` a required -# status check is a ruleset change in repo settings (one covering `v2/main` -# does not exist yet), not something a workflow file can do — without it, this -# job going missing would pass as silently as the app did. +# Two jobs, one script: # -# Its own workflow because it needs the PR's base, which `main.yml` (push -# only) never sees. +# - `DCO` — on every v2 PR, over the PR's commits. This is the +# one to make a REQUIRED status check (a ruleset change in repo settings, +# not something this file can do — without it, the job going missing +# passes as silently as the app did). +# - `DCO (v2/main push)` — a backstop over every push that lands on +# `v2/main`, for whatever reached the branch without a passing PR check: +# an admin merge, a direct push, or a PR that edited its own check (below). +# It reports when the commit lands, not one milestone later — it cannot +# un-merge anything, but an unsigned commit found the same day is still +# cheap to deal with. # -# ⚠️ `pull_request_target`, deliberately — a check that gates a PR must not run -# code the PR can change. Under `pull_request` both this file and the script -# come from the PR's merge ref, so a PR could rewrite either to pass itself. -# `pull_request_target` takes the workflow from the repo's DEFAULT branch -# (`main`), and the job checks out the PR's base (`v2/main`) for the script — -# both already-merged code. The PR's commits are only FETCHED: `git log` reads -# their metadata and nothing from the PR is ever executed, which is the -# condition that makes `pull_request_target` safe. Keep it that way: never -# check out, install, build or run the PR head in this job. +# ⚠️ `pull_request`, not `pull_request_target` (#2616). #2603 shipped on +# `pull_request_target` so a PR could not rewrite its own check to pass — but +# GitHub reads `pull_request_target` workflows from the DEFAULT branch +# (`main`), so the check reported on no v2 PR at all until a milestone merge +# carried this file there, and unsigned commits would have surfaced only when +# the milestone PR into `main` was opened. Under `pull_request` the workflow +# runs from the PR's own ref and works the moment it is on `v2/main`, and an +# edit to this file takes effect on the PR that makes it. # -# ⚠️ Rollout follows from the workflow living on `main`: this file does -# nothing until a milestone merge carries it from `v2/main` to `main`, and a -# later edit to it takes effect only on the next milestone merge. An edit to -# `scripts/dco-check.mjs` takes effect as soon as it merges to `v2/main`. Make -# `DCO` a required check only once this file is on `main` — required before -# then, every v2 PR would wait on a check that never reports. +# The trade-off, accepted deliberately: a PR can now edit this file to pass +# itself. That is tolerable here because PRs are opened by maintainers only +# (AGENTS.md, Contributing) and the check exists to catch a FORGOTTEN signoff, +# not a forged one — the trailer is self-asserted text either way (see the +# header of `scripts/dco-check.mjs`). The push job is what still sees such a +# PR's commits once they land. The PR job keeps the shape that made the old +# trigger safe anyway: it checks out the PR's BASE for the script and only +# FETCHES the PR's commits, reading their metadata with `git log` — nothing +# from the PR is checked out, installed, built or run. # -# Scoped to `v2/main`. Once on `main` an unscoped trigger would also fire for -# `v1/main` PRs (whose checkout has no script) and for milestone PRs into -# `main` (whose range holds GitHub's squash-merge commits, signed under a -# different name). `edited` re-runs it when a PR is retargeted, since the -# checked range changes with the base even when the head does not. +# The PR job runs on every base EXCEPT `main` and `v1/main`, so a stacked PR — +# one based on another feature branch — is checked on its own range rather +# than only once its parent merges (the servers repo's #5055 scopes it the +# same way). `v1/main` has no copy of the script, and milestone PRs into +# `main` carry GitHub's squash-merge commits and reach back before the check +# existed. A stacked PR's base must be a branch cut after #2603, since the +# script is read from it. `edited` re-runs the PR job when a PR is retargeted, +# since the checked range changes with the base even when the head does not. +# +# Before either job, `npm run local:gate` runs the same script over +# `origin/v2/main..HEAD` (`local:dco`), so an unsigned commit is normally +# caught before it is pushed at all. # # Only `contents: read`, no credential persisted, no secret — so it stays on # moving action tags (#2235; see verify:action-pins, #2484). name: DCO on: - pull_request_target: + pull_request: types: [opened, synchronize, reopened, edited] + branches-ignore: [main, v1/main] + push: branches: [v2/main] permissions: @@ -53,14 +69,21 @@ permissions: jobs: dco: name: DCO + if: github.event_name == 'pull_request' runs-on: ubuntu-latest + # A newer push to the same PR supersedes the range being checked. Job-level + # and PR-keyed on purpose: the push job below must NOT share it, since a + # cancelled push run is a range that is never checked. + concurrency: + group: dco-pr-${{ github.event.pull_request.number }} + cancel-in-progress: true # A git fetch, a git log and a node script; expected to finish well inside # a minute. Not yet observed on a runner — revisit with the measured range # per the rule in main.yml (#2333) once it has a history. timeout-minutes: 5 steps: - - name: Checkout the base branch (trusted) with full history + - name: Checkout the base branch with full history uses: actions/checkout@v7 with: ref: ${{ github.base_ref }} @@ -87,3 +110,39 @@ jobs: BASE_REF: ${{ github.base_ref }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: node scripts/dco-check.mjs --base "origin/$BASE_REF" --head "$HEAD_SHA" + + dco-push: + name: DCO (v2/main push) + if: github.event_name == 'push' + runs-on: ubuntu-latest + # Same work as the PR job; same unmeasured budget. + timeout-minutes: 5 + + steps: + - name: Checkout the pushed commit with full history + uses: actions/checkout@v7 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + + - name: Check every pushed commit is signed off + # `before..after` is exactly what the push added: for a PR merge, the + # merge commit (exempt) plus every commit it brought in. A zero + # `before` means the branch was just created, which leaves no range to + # check. A `before` missing from the history (a force-push that + # rewrote it away) makes `git log` fail and the script exit 2 — loud + # on purpose, since `v2/main` is never meant to be rewritten. + env: + BEFORE: ${{ github.event.before }} + AFTER: ${{ github.event.after }} + run: | + if [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then + echo "Branch creation push: no prior commit, nothing to check." + exit 0 + fi + node scripts/dco-check.mjs --base "$BEFORE" --head "$AFTER" diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 032d6c8dd..cedbdefd8 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -48,8 +48,9 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. | `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #1942 and #1742 were each an instance, found one site at a time. Deliberately **not** cited: #2278 (a missing condition wait around a geometry read) and #2250 (a real race in a test's own timing) were fixed by making the test wait for the right thing, and presenting a race fix as evidence for a larger ceiling would argue against #1596. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. A **seventh project** with no row, or a config file it does not discover (it knows all twelve filenames Vitest accepts, not just the two this repo uses), is an error rather than a silent skip. It also checks that every project loads `vitest.setup.shared.mts`. ⚠️ **Everything it reads comes from a resolved project, never from source text** — the two rules no config can report are asserted at runtime instead: `retry` by `vitest.setup.shared.mts` (which reads the value Vitest resolved, so a per-test option, a `describe` option, a project setting and a `--retry` flag are one check) and Testing Library's `asyncUtilTimeout` by `clients/web/src/test/asyncUtilTimeout.test.ts` (which reads what the project's own `waitFor`s use). Both began as source scanning in #2334 and the review found a new valid spelling missed in five consecutive rounds, so the boundary is deliberate: if a rule cannot be answered by asking the tool, assert it at runtime rather than reading the source for it. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. | `npm run verify:action-pins` | Guards the credentialed-job pinning rule ([#2484](https://github.com/modelcontextprotocol/inspector/issues/2484)). Actions stay on moving major tags by default (#2235), but a tag is mutable, so whoever can move one replaces the code running next to a credential. For every job under `.github/workflows/` that holds `id-token: write` / `packages: write` / `write-all` (its own `permissions:` or the workflow's), is handed any secret other than `GITHUB_TOKEN`, or uploads an artifact such a job downloads and `needs`, it fails on any `uses:` that is not a 40-hex commit SHA followed by an exact `# vX.Y.Z` comment. The comment is what keeps the pin in the monthly `dependency-refresh` sweep, which ranks a SHA pin by it to the patch. Offline, so it cannot check that the SHA matches the comment — resolve both from one tag lookup. | -| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus one local-only one — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it, and queued gates start in the order they arrived ([#2473](https://github.com/modelcontextprotocol/inspector/issues/2473)). Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection) — unless its lock directory cannot be removed, in which case the wait runs to its 45-minute cap and names the path; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. Why it is a queue rather than a load wait or a worker cap, and what it does and does not change about capacity, is [Multi-agent testing](#multi-agent-testing-the-gate-lease). | +| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `local:dco` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus one local-only one — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it, and queued gates start in the order they arrived ([#2473](https://github.com/modelcontextprotocol/inspector/issues/2473)). Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection) — unless its lock directory cannot be removed, in which case the wait runs to its 45-minute cap and names the path; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. Why it is a queue rather than a load wait or a worker cap, and what it does and does not change about capacity, is [Multi-agent testing](#multi-agent-testing-the-gate-lease). | | `npm run local:validate` | The gate's first stage: `validate` with each client's `test` leg removed (#2341) — the same `validate:guards` and `validate:core`, then every client's `check` — `format:check` + `lint` + `typecheck`, plus `build` for web, tui and launcher, which name one explicitly (cli's only validate-time build was `test`'s `pretest` hook, so it goes with the `test` leg and `coverage:cli` builds the binary once, later) — each client's `validate` being `check && test`. The suites run once, under `coverage`, instead of bare here and instrumented there. In the `local:` namespace so the workflow guard keeps it out of CI, where the bare pass is free. | +| `npm run local:dco` | The gate's second stage ([#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)): `scripts/dco-check.mjs` over `origin/v2/main..HEAD`, so a commit missing its `Signed-off-by:` trailer fails before it is pushed rather than on the PR's `DCO` check. The same script `.github/workflows/dco.yml` runs on every v2 PR and on every push to `v2/main`. Local-only: it reads `origin/v2/main`, which CI's shallow push checkout does not have, and `workflow-gate.test.mjs` pins it out of `validate`. A stale `origin/v2/main` only widens the range to commits that are already merged. | | `npm run local:storybook` | The gate's last stage: from `clients/web`, `npx playwright install chromium` and then `test:storybook` — the Storybook play functions, run headless by Vitest's browser project (CI runs `test:storybook` directly after its own Playwright install step, which is why this wrapper is in the `local:` namespace). Since [#2340](https://github.com/modelcontextprotocol/inspector/issues/2340) (PR [#2342](https://github.com/modelcontextprotocol/inspector/pull/2342)) the project's `optimizeDeps` carries `force: true` (`getStorybookOptimizeDeps` in `clients/web/server/vite-base-config.ts`), so Vite discards its dep pre-bundle cache and re-scans every story entry before the browser opens — the cold path CI takes on every PR. Without it, Vite keys that cache on the lockfile and the config, never on what the story graph imports, so after a story gains an import of an already-installed package the "valid" cache re-runs the optimizer mid-run and sends a `full-reload` the Vitest tester iframes do not act on; every story an already-loaded iframe renders from then on fails with `Failed to fetch dynamically imported module`, and the next run is green because the cache has caught up. Measured: a stale cache went 3/3 red → 3/3 green with `force` → 3/3 red reverted, load refuted as a cause (0 of 6 under load 8–12), cost ~0.8s per run against a ~25s stage. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/package.json b/package.json index a9ee203fb..df2e4a1dc 100644 --- a/package.json +++ b/package.json @@ -44,8 +44,9 @@ "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", "local:gate": "node scripts/gate-lease.mjs npm run local:gate:stages", - "local:gate:stages": "npm run local:validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", + "local:gate:stages": "npm run local:validate && npm run local:dco && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", "local:validate": "npm run validate:guards && npm run validate:core && cd clients/web && npm run check && cd ../cli && npm run check && cd ../mcpdo && npm run check && cd ../tui && npm run check && cd ../launcher && npm run check", + "local:dco": "node scripts/dco-check.mjs --base origin/v2/main", "local:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", diff --git a/scripts/dco-check.mjs b/scripts/dco-check.mjs index d54a20f9e..2242b512e 100644 --- a/scripts/dco-check.mjs +++ b/scripts/dco-check.mjs @@ -4,15 +4,18 @@ // Replaces the probot DCO app, which was suspended and whose check simply // stopped appearing after #1981 (2026-08-12). Nothing failed when it vanished, // because it was never a required check — so this is a check the repo owns, -// run by `.github/workflows/dco.yml` on every PR targeting `v2/main` (v1 and -// milestone PRs into `main` are out of its scope), and meant to be -// made REQUIRED so a future outage blocks merges instead of passing silently. +// run by `.github/workflows/dco.yml` on every PR targeting `v2/main` and, +// as a backstop, on every push that lands there (#2616; v1 and milestone PRs +// into `main` are out of its scope). The PR job is meant to be made REQUIRED +// so a future outage blocks merges instead of passing silently. // // The rule is the app's: every commit in `base..head` must carry a // `Signed-off-by: Name ` line whose name AND email match the commit's // author or its committer (one identity — a name from one and an email from // the other is not a match). Names and emails compare case-insensitively, -// after trimming. The app's two exemptions are kept: +// after trimming. One relaxation: for a commit GitHub itself committed (a +// squash merge), an author EMAIL match is enough — see `webFlowAuthorMatch`. +// The app's two exemptions are kept: // // - merge commits (more than one parent), which certify nothing new; and // - bot-authored commits — an author email of the GitHub noreply shape @@ -83,6 +86,23 @@ const norm = (value) => value.trim().toLowerCase(); const sameIdentity = (a, b) => norm(a.name) === norm(b.name) && norm(a.email) === norm(b.email); +// The identity GitHub commits as when it creates a commit itself — a squash +// merge, a rebase merge, a web edit. +const GITHUB_WEB_FLOW_EMAIL = "noreply@github.com"; + +/** + * GitHub writes a squash-merge commit's author NAME from the merger's GitHub + * profile ("Cliff Hall"), while the squashed trailers carry their git + * `user.name` ("cliffhall") — same person, same email, different name. Four + * such squash merges sit on `v2/main` (#2213, #2322, #2379, #2383), and the + * push job (#2616) sees every one of them. So for a commit GitHub itself + * committed, an author EMAIL match is accepted. A signoff is still required, + * and every other commit still needs name and email from one identity. + */ +const webFlowAuthorMatch = (sig, commit) => + norm(commit.committer.email) === GITHUB_WEB_FLOW_EMAIL && + norm(sig.email) === norm(commit.author.email); + /** * Why this commit fails the check, or `null` when it passes or is exempt. * Exempt commits return `null` as well — they are reported separately by @@ -93,7 +113,9 @@ export function failureReason(commit) { if (found.length === 0) return "no Signed-off-by trailer"; const matches = found.some( (sig) => - sameIdentity(sig, commit.author) || sameIdentity(sig, commit.committer), + sameIdentity(sig, commit.author) || + sameIdentity(sig, commit.committer) || + webFlowAuthorMatch(sig, commit), ); if (matches) return null; const listed = found.map((sig) => `${sig.name} <${sig.email}>`).join(", "); diff --git a/scripts/dco-check.test.mjs b/scripts/dco-check.test.mjs index 46973edf1..5c2361525 100644 --- a/scripts/dco-check.test.mjs +++ b/scripts/dco-check.test.mjs @@ -106,6 +106,47 @@ test("name from one identity and email from the other is not a match", () => { assert.notEqual(reason, null); }); +// A GitHub squash merge: author name from the GitHub profile, committer +// GitHub's web-flow identity, trailers carrying the git `user.name` (#2616). +const GITHUB = { name: "GitHub", email: "noreply@github.com" }; +const ADA_PROFILE = { name: "Ada L.", email: ADA.email }; +const ADA_GIT = { name: "ada", email: ADA.email }; + +test("a GitHub-committed squash passes on an author email match", () => { + assert.equal( + failureReason( + commit({ + author: ADA_PROFILE, + committer: GITHUB, + message: signed(ADA_GIT), + }), + ), + null, + ); +}); + +test("the email-only match applies to GitHub-committed commits alone", () => { + assert.notEqual( + failureReason( + commit({ author: ADA_PROFILE, committer: BOB, message: signed(ADA_GIT) }), + ), + null, + ); +}); + +test("a GitHub-committed commit still needs the AUTHOR's email", () => { + assert.notEqual( + failureReason( + commit({ author: ADA_PROFILE, committer: GITHUB, message: signed(BOB) }), + ), + null, + ); + assert.equal( + failureReason(commit({ author: ADA_PROFILE, committer: GITHUB })), + "no Signed-off-by trailer", + ); +}); + test("exemption: merge commits and noreply bot authors only", () => { assert.equal(exemption(commit({ parents: ["p1", "p2"] })), "merge"); assert.equal( diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index 7fd7c2f5a..332048047 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -614,6 +614,25 @@ describe("the gate's name", () => { assert.match(scripts["local:gate:stages"], /local:storybook/); }); + it("checks DCO signoffs before pushing, and only locally (#2616)", () => { + // The pre-push half of the DCO check: `dco.yml` reports on the PR, this + // catches the unsigned commit before it is pushed. It reads + // `origin/v2/main`, which CI's shallow push checkout does not have — so it + // must stay unreachable from `validate`, which CI runs. + assert.equal( + scripts["local:dco"], + "node scripts/dco-check.mjs --base origin/v2/main", + ); + assert.ok( + scriptChainRuns(scripts, "local:gate", "local:dco"), + "local:gate must run local:dco", + ); + assert.ok( + !reachableScripts(scripts, "validate").has("local:dco"), + "validate (CI) must not reach local:dco", + ); + }); + it("runs its stages under the lease wrapper, and nothing else (#2339)", () => { // The wrapper serializes gates across worktrees. It must be the ONLY thing // `local:gate` does — a stage placed beside it would run outside the From 38862e65f2077112050c5b82632d9b517a065f63 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 01:14:42 -0400 Subject: [PATCH 2/7] docs(dco): describe the PR check's scope as every v2 PR, stacked ones included (#2616) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 2 +- scripts/dco-check.mjs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 71654177d..d3e41df19 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -95,7 +95,7 @@ previous one, not all cut from `v2/main`. **The `DCO` check fails the PR on any unsigned commit.** It is this repo's own job (`.github/workflows/dco.yml` → `scripts/dco-check.mjs`, #2566), run on every -PR targeting `v2/main`, and it requires each commit to carry a `Signed-off-by: Name ` +v2 PR, stacked PRs included, and it requires each commit to carry a `Signed-off-by: Name ` trailer whose name **and** email match either the commit's author or its committer (case-insensitively). Its only exemptions are merge commits and bot-authored commits; there is no partial credit — one unsigned commit out of six diff --git a/scripts/dco-check.mjs b/scripts/dco-check.mjs index 2242b512e..aae2e2bcd 100644 --- a/scripts/dco-check.mjs +++ b/scripts/dco-check.mjs @@ -4,9 +4,9 @@ // Replaces the probot DCO app, which was suspended and whose check simply // stopped appearing after #1981 (2026-08-12). Nothing failed when it vanished, // because it was never a required check — so this is a check the repo owns, -// run by `.github/workflows/dco.yml` on every PR targeting `v2/main` and, -// as a backstop, on every push that lands there (#2616; v1 and milestone PRs -// into `main` are out of its scope). The PR job is meant to be made REQUIRED +// run by `.github/workflows/dco.yml` on every v2 PR — any base but `main` and +// `v1/main`, so stacked PRs too — and, as a backstop, on every push that lands +// on `v2/main` (#2616; v1 and milestone PRs into `main` are out of its scope). The PR job is meant to be made REQUIRED // so a future outage blocks merges instead of passing silently. // // The rule is the app's: every commit in `base..head` must carry a From 702001ea62c1d3b92b950b326e6788a9f564b40d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 01:26:30 -0400 Subject: [PATCH 3/7] ci(dco): scope the PR job to v2/** bases rather than excluding main and v1/main (#2616) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 2 +- .github/workflows/dco.yml | 18 ++++++++++-------- scripts/dco-check.mjs | 9 +++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index d3e41df19..70ab7a7fd 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -105,7 +105,7 @@ repair below. ⚠️ **It is a merge gate only because it is a _required_ status check** — a ruleset setting, not something the workflow file can declare. The job runs on `pull_request`, from the PR's own ref, so it reports on every v2 PR — stacked -ones included, any base but `main` and `v1/main` — with no wait for a +ones included, any `v2/**` base — with no wait for a milestone merge (#2616). A second job, `DCO (v2/main push)`, re-runs the check over every push that lands on `v2/main` — a backstop for anything that merged without a passing PR check — so a red there means an diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index 58f968eb7..517ae264d 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -39,13 +39,15 @@ # FETCHES the PR's commits, reading their metadata with `git log` — nothing # from the PR is checked out, installed, built or run. # -# The PR job runs on every base EXCEPT `main` and `v1/main`, so a stacked PR — -# one based on another feature branch — is checked on its own range rather -# than only once its parent merges (the servers repo's #5055 scopes it the -# same way). `v1/main` has no copy of the script, and milestone PRs into -# `main` carry GitHub's squash-merge commits and reach back before the check -# existed. A stacked PR's base must be a branch cut after #2603, since the -# script is read from it. `edited` re-runs the PR job when a PR is retargeted, +# The PR job runs on every `v2/**` base — `v2/main` and, since branch names +# carry their version segment, every v2 feature branch — so a stacked PR is +# checked on its own range rather than only once its parent merges (the +# servers repo's #5055 checks stacks too). A positive filter rather than a +# list of exclusions, so the scope is the v2 line by construction: `main` +# (milestone PRs carry GitHub's squash-merge commits and reach back before the +# check existed) and the whole v1 line, `v1/main` and its stacks alike (no copy +# of the script), are out of it without being named. A stacked PR's base must +# be a branch cut after #2603, since the script is read from it. `edited` re-runs the PR job when a PR is retargeted, # since the checked range changes with the base even when the head does not. # # Before either job, `npm run local:gate` runs the same script over @@ -59,7 +61,7 @@ name: DCO on: pull_request: types: [opened, synchronize, reopened, edited] - branches-ignore: [main, v1/main] + branches: ['v2/**'] push: branches: [v2/main] diff --git a/scripts/dco-check.mjs b/scripts/dco-check.mjs index aae2e2bcd..456c1dcc0 100644 --- a/scripts/dco-check.mjs +++ b/scripts/dco-check.mjs @@ -4,10 +4,11 @@ // Replaces the probot DCO app, which was suspended and whose check simply // stopped appearing after #1981 (2026-08-12). Nothing failed when it vanished, // because it was never a required check — so this is a check the repo owns, -// run by `.github/workflows/dco.yml` on every v2 PR — any base but `main` and -// `v1/main`, so stacked PRs too — and, as a backstop, on every push that lands -// on `v2/main` (#2616; v1 and milestone PRs into `main` are out of its scope). The PR job is meant to be made REQUIRED -// so a future outage blocks merges instead of passing silently. +// run by `.github/workflows/dco.yml` on every v2 PR — any `v2/**` base, so +// stacked PRs too — and, as a backstop, on every push that lands on `v2/main` +// (#2616; v1 and milestone PRs into `main` are out of its scope). The PR job +// is meant to be made REQUIRED so a future outage blocks merges instead of +// passing silently. // // The rule is the app's: every commit in `base..head` must carry a // `Signed-off-by: Name ` line whose name AND email match the commit's From fa6538c70839e00f27a4e0ad87a30c959429a1bc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 01:41:37 -0400 Subject: [PATCH 4/7] docs(dco): stacked-branch check and repair guidance, and refresh the quality-gate DCO paragraph (#2616) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 13 +++++++++++-- .claude/skills/pre-push-gate/SKILL.md | 13 ++++++++++--- docs/quality-gate.md | 2 +- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 70ab7a7fd..17ede3cfb 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -116,10 +116,13 @@ stopped appearing (after #1981) and nothing went red for two months. If the **Check before you push.** `npm run local:gate` already does it: its `local:dco` stage runs the same script over `origin/v2/main..HEAD`, right after -`local:validate`. To check on its own, against the range the PR will show: +`local:validate`. On a **stacked** branch that range covers the whole stack, +parents included, which is stricter than the child PR's own check (that one +runs against the parent's branch). To check on its own, against the range the +PR will show, pass the PR's actual base: ```sh -npm run dco:check -- --base origin/v2/main +npm run dco:check -- --base origin/v2/main # or origin/ when stacked ``` **Prevent it with `git commit -s`.** Two things that look like automation and are @@ -141,6 +144,12 @@ git rebase --rebase-merges --signoff origin/v2/main # the base the PR targets git push --force-with-lease ``` +⚠️ **On a stacked branch, rebase against the parent branch, never +`origin/v2/main`.** A `--signoff` rebase onto `origin/v2/main` rewrites every +parent commit too, which forks the child from its parent and breaks the stack. +If the unsigned commit is in the parent, repair the parent's own branch first, +then rebase the child onto the repaired parent. + `--rebase-merges` keeps any merge commit on the branch — without it the rebase flattens them, silently dropping a conflict resolution that lives only in the merge. Use `--force-with-lease` rather than `--force`, and only rewrite when you are the diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 990474e42..3bb4ab92a 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -103,9 +103,16 @@ reporting the *old* dependency's behavior as a product bug (#2494). Don't A commit in `origin/v2/main..HEAD` has no `Signed-off-by:` trailer matching its author or committer (#2616). The output names each commit and prints the repair. Repair and prevention (`git commit -s`, the `--signoff` rebase) are -step 3 of `/pr-flow`. If it lists commits you never made, your `origin/v2/main` -is probably behind what the branch was cut from: `git fetch origin v2/main` and -re-run. +step 3 of `/pr-flow`. + +If it lists commits you never made, check which case you are in: + +- **A stacked branch** (cut from another feature branch): `origin/v2/main..HEAD` + covers the whole stack, parent commits included. Fetching changes nothing. + An unsigned parent commit is repaired on the parent's own branch first, and + the child is then rebased onto the repaired parent (`/pr-flow` step 3). +- **Not stacked**: your `origin/v2/main` is probably behind what the branch was + cut from. `git fetch origin v2/main` and re-run. ### `verify:action-pins` diff --git a/docs/quality-gate.md b/docs/quality-gate.md index cedbdefd8..5bf006733 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -14,7 +14,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | **GitHub CI** (`.github/workflows/main.yml`) | Automatically, on every push | `npm install`, then `validate`, `verify:skills:cli`, `verify:build-gate`, `verify:bundle-externals`, `smoke` (which includes `smoke:web:chromium`), `test:storybook` — plus `coverage` in a parallel job ([#2159](https://github.com/modelcontextprotocol/inspector/issues/2159)) | | **The local gate** (`npm run local:gate`) | By hand, before you push | Every check above (the install is yours to run; `local:validate` stands in for `validate`, see below), **plus** the Firefox engine pass (`smoke:web:firefox`) | -One more CI check runs outside that table: **`.github/workflows/dco.yml`**, on every *pull request targeting `v2/main`* rather than every push (v1 PRs and milestone PRs into `main` are out of its scope), fails a PR whose commits are not all signed off ([#2566](https://github.com/modelcontextprotocol/inspector/issues/2566)). It needs the PR's base branch, which a push run and the local gate never see, so the gate does not run it; `npm run dco:check -- --base origin/v2/main` is the same check by hand. It runs on `pull_request_target`, so neither the workflow (read from `main`) nor the script (checked out from the base) can be changed by the PR it gates — and so it is active only once a milestone merge has carried the workflow to `main`. It replaced the probot DCO app, whose check vanished unnoticed when the app was suspended because it was never required — the replacement gates merges only as a **required** status check, a ruleset setting the workflow cannot declare. +One more CI check runs outside that table: **`.github/workflows/dco.yml`** fails on any commit that is not signed off ([#2566](https://github.com/modelcontextprotocol/inspector/issues/2566), [#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)). It has two jobs. `DCO` runs on every *pull request with a `v2/**` base*, so `v2/main` and stacked v2 PRs, over the PR's own commits; v1 PRs and milestone PRs into `main` are out of its scope. `DCO (v2/main push)` re-checks every push that lands on `v2/main`, as a backstop for anything that merged without a passing PR check. The PR job runs on `pull_request`, so it is live as soon as the workflow is on `v2/main`. The trade-off is that a PR could edit its own check, which is accepted because PRs are maintainer-only and the check catches a *forgotten* signoff, not a forged one. The local gate runs the same script before a push as its `local:dco` stage (below), over `origin/v2/main..HEAD`. It replaced the probot DCO app, whose check vanished unnoticed when the app was suspended because it was never required. The replacement gates merges only as a **required** status check, a ruleset setting the workflow cannot declare. The local gate runs **every check** `main.yml` runs, and is not a mirror. One of its steps has no GitHub CI counterpart: From 15762ac5fa0bbc12c988c2a11fdcd923603b19df Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 01:57:17 -0400 Subject: [PATCH 5/7] fix(dco): match GitHub's web-flow identity whole, and document the squash relaxation (#2616) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 5 ++++- scripts/dco-check.mjs | 8 +++++--- scripts/dco-check.test.mjs | 13 +++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 17ede3cfb..27d9b3942 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -97,7 +97,10 @@ previous one, not all cut from `v2/main`. job (`.github/workflows/dco.yml` → `scripts/dco-check.mjs`, #2566), run on every v2 PR, stacked PRs included, and it requires each commit to carry a `Signed-off-by: Name ` trailer whose name **and** email match either the commit's author or its -committer (case-insensitively). Its only exemptions are merge commits and +committer (case-insensitively). One relaxation: a commit GitHub itself +committed (`GitHub `, as on a squash merge, whose author +name GitHub takes from the profile) passes on an author **email** match alone. +Its only exemptions are merge commits and bot-authored commits; there is no partial credit — one unsigned commit out of six fails the whole check, and the job's output names each offending commit and the repair below. diff --git a/scripts/dco-check.mjs b/scripts/dco-check.mjs index 456c1dcc0..59ffbc69f 100644 --- a/scripts/dco-check.mjs +++ b/scripts/dco-check.mjs @@ -88,8 +88,10 @@ const sameIdentity = (a, b) => norm(a.name) === norm(b.name) && norm(a.email) === norm(b.email); // The identity GitHub commits as when it creates a commit itself — a squash -// merge, a rebase merge, a web edit. -const GITHUB_WEB_FLOW_EMAIL = "noreply@github.com"; +// merge, a rebase merge, a web edit. Matched as a whole identity, name and +// email, like every other comparison here: git metadata is user-set either +// way, but a commit that merely borrows the email is not a web-flow commit. +const GITHUB_WEB_FLOW = { name: "GitHub", email: "noreply@github.com" }; /** * GitHub writes a squash-merge commit's author NAME from the merger's GitHub @@ -101,7 +103,7 @@ const GITHUB_WEB_FLOW_EMAIL = "noreply@github.com"; * and every other commit still needs name and email from one identity. */ const webFlowAuthorMatch = (sig, commit) => - norm(commit.committer.email) === GITHUB_WEB_FLOW_EMAIL && + sameIdentity(commit.committer, GITHUB_WEB_FLOW) && norm(sig.email) === norm(commit.author.email); /** diff --git a/scripts/dco-check.test.mjs b/scripts/dco-check.test.mjs index 5c2361525..914078633 100644 --- a/scripts/dco-check.test.mjs +++ b/scripts/dco-check.test.mjs @@ -134,6 +134,19 @@ test("the email-only match applies to GitHub-committed commits alone", () => { ); }); +test("the web-flow identity is matched whole, not by its email alone", () => { + assert.notEqual( + failureReason( + commit({ + author: ADA_PROFILE, + committer: { name: "Mallory", email: GITHUB.email }, + message: signed(ADA_GIT), + }), + ), + null, + ); +}); + test("a GitHub-committed commit still needs the AUTHOR's email", () => { assert.notEqual( failureReason( From 8ee5754b844b6419739ba8be0da3eabde237b5e8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 02:11:31 -0400 Subject: [PATCH 6/7] docs(dco): the push backstop does not cover a PR that edits the check itself (#2616) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .github/workflows/dco.yml | 15 +++++++++++---- docs/quality-gate.md | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index 517ae264d..0d6e7afc9 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -14,8 +14,10 @@ # not something this file can do — without it, the job going missing # passes as silently as the app did). # - `DCO (v2/main push)` — a backstop over every push that lands on -# `v2/main`, for whatever reached the branch without a passing PR check: -# an admin merge, a direct push, or a PR that edited its own check (below). +# `v2/main`, for whatever reached the branch without a passing PR check +# while this file and the script stayed intact: an admin merge, a direct +# push, a merge made while the PR check was red or missing. NOT a PR that +# edited the check itself (below). # It reports when the commit lands, not one milestone later — it cannot # un-merge anything, but an unsigned commit found the same day is still # cheap to deal with. @@ -33,8 +35,13 @@ # itself. That is tolerable here because PRs are opened by maintainers only # (AGENTS.md, Contributing) and the check exists to catch a FORGOTTEN signoff, # not a forged one — the trailer is self-asserted text either way (see the -# header of `scripts/dco-check.mjs`). The push job is what still sees such a -# PR's commits once they land. The PR job keeps the shape that made the old +# header of `scripts/dco-check.mjs`). ⚠️ The push job does NOT cover that +# case: a `push` run also reads this file, and runs the script, from the +# pushed revision — so a PR that edits the check edits the backstop with it. +# Nothing in a workflow the repo's own branches carry can close that; only a +# check read from elsewhere could, which is what `pull_request_target` was and +# what this change gives up. The control for it is review: an edit to this +# file or to `scripts/dco-check.mjs` is in the PR's diff. The PR job keeps the # trigger safe anyway: it checks out the PR's BASE for the script and only # FETCHES the PR's commits, reading their metadata with `git log` — nothing # from the PR is checked out, installed, built or run. diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 5bf006733..76bb2ae74 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -14,7 +14,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | **GitHub CI** (`.github/workflows/main.yml`) | Automatically, on every push | `npm install`, then `validate`, `verify:skills:cli`, `verify:build-gate`, `verify:bundle-externals`, `smoke` (which includes `smoke:web:chromium`), `test:storybook` — plus `coverage` in a parallel job ([#2159](https://github.com/modelcontextprotocol/inspector/issues/2159)) | | **The local gate** (`npm run local:gate`) | By hand, before you push | Every check above (the install is yours to run; `local:validate` stands in for `validate`, see below), **plus** the Firefox engine pass (`smoke:web:firefox`) | -One more CI check runs outside that table: **`.github/workflows/dco.yml`** fails on any commit that is not signed off ([#2566](https://github.com/modelcontextprotocol/inspector/issues/2566), [#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)). It has two jobs. `DCO` runs on every *pull request with a `v2/**` base*, so `v2/main` and stacked v2 PRs, over the PR's own commits; v1 PRs and milestone PRs into `main` are out of its scope. `DCO (v2/main push)` re-checks every push that lands on `v2/main`, as a backstop for anything that merged without a passing PR check. The PR job runs on `pull_request`, so it is live as soon as the workflow is on `v2/main`. The trade-off is that a PR could edit its own check, which is accepted because PRs are maintainer-only and the check catches a *forgotten* signoff, not a forged one. The local gate runs the same script before a push as its `local:dco` stage (below), over `origin/v2/main..HEAD`. It replaced the probot DCO app, whose check vanished unnoticed when the app was suspended because it was never required. The replacement gates merges only as a **required** status check, a ruleset setting the workflow cannot declare. +One more CI check runs outside that table: **`.github/workflows/dco.yml`** fails on any commit that is not signed off ([#2566](https://github.com/modelcontextprotocol/inspector/issues/2566), [#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)). It has two jobs. `DCO` runs on every *pull request with a `v2/**` base*, so `v2/main` and stacked v2 PRs, over the PR's own commits; v1 PRs and milestone PRs into `main` are out of its scope. `DCO (v2/main push)` re-checks every push that lands on `v2/main`, as a backstop for anything that merged without a passing PR check. The PR job runs on `pull_request`, so it is live as soon as the workflow is on `v2/main`. The trade-off is that a PR could edit its own check, and the push job could not catch that either, since a push run reads the workflow and the script from the pushed revision. It is accepted because PRs are maintainer-only, such an edit shows in the PR's diff, and the check exists to catch a *forgotten* signoff, not a forged one. The local gate runs the same script before a push as its `local:dco` stage (below), over `origin/v2/main..HEAD`. It replaced the probot DCO app, whose check vanished unnoticed when the app was suspended because it was never required. The replacement gates merges only as a **required** status check, a ruleset setting the workflow cannot declare. The local gate runs **every check** `main.yml` runs, and is not a mirror. One of its steps has no GitHub CI counterpart: From 84480392d44239a1124a140ffec39c3a92b0145e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 7 Oct 2026 02:51:53 -0400 Subject: [PATCH 7/7] fix(dco): local:dco excludes origin/main, so the gate passes on a milestone-merge branch (#2616) A milestone-merge branch is cut from main, so origin/v2/main..HEAD there is main's own released history (117 commits, 116 unsigned) and the gate the release runs on that branch would fail at stage 2. dco-check gains a repeatable --exclude; local:dco passes --exclude origin/main. A no-op on a feature branch. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 3 +- .claude/skills/pre-push-gate/SKILL.md | 3 +- .github/workflows/dco.yml | 3 +- docs/quality-gate.md | 2 +- package.json | 2 +- scripts/dco-check.mjs | 51 +++++++++++++++++++++------ scripts/dco-check.test.mjs | 26 ++++++++++++-- scripts/lib/workflow-gate.test.mjs | 2 +- 8 files changed, 74 insertions(+), 18 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 27d9b3942..cbbb8994d 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -118,7 +118,8 @@ stopped appearing (after #1981) and nothing went red for two months. If the `DCO` check is ever missing from a PR, treat that as the outage it is. **Check before you push.** `npm run local:gate` already does it: its -`local:dco` stage runs the same script over `origin/v2/main..HEAD`, right after +`local:dco` stage runs the same script over `origin/v2/main..HEAD` (minus +anything already on `origin/main`), right after `local:validate`. On a **stacked** branch that range covers the whole stack, parents included, which is stricter than the child PR's own check (that one runs against the parent's branch). To check on its own, against the range the diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 3bb4ab92a..cb31fc1cd 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -100,7 +100,8 @@ reporting the *old* dependency's behavior as a product bug (#2494). Don't ### `local:dco` -A commit in `origin/v2/main..HEAD` has no `Signed-off-by:` trailer matching its +A commit in `origin/v2/main..HEAD` (minus anything already on `origin/main`) +has no `Signed-off-by:` trailer matching its author or committer (#2616). The output names each commit and prints the repair. Repair and prevention (`git commit -s`, the `--signoff` rebase) are step 3 of `/pr-flow`. diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index 0d6e7afc9..0ecc89e73 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -58,7 +58,8 @@ # since the checked range changes with the base even when the head does not. # # Before either job, `npm run local:gate` runs the same script over -# `origin/v2/main..HEAD` (`local:dco`), so an unsigned commit is normally +# `origin/v2/main..HEAD`, minus anything already on `origin/main` so it holds +# on a milestone-merge branch too (`local:dco`), so an unsigned commit is normally # caught before it is pushed at all. # # Only `contents: read`, no credential persisted, no secret — so it stays on diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 76bb2ae74..3d6bec1d4 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -50,7 +50,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:action-pins` | Guards the credentialed-job pinning rule ([#2484](https://github.com/modelcontextprotocol/inspector/issues/2484)). Actions stay on moving major tags by default (#2235), but a tag is mutable, so whoever can move one replaces the code running next to a credential. For every job under `.github/workflows/` that holds `id-token: write` / `packages: write` / `write-all` (its own `permissions:` or the workflow's), is handed any secret other than `GITHUB_TOKEN`, or uploads an artifact such a job downloads and `needs`, it fails on any `uses:` that is not a 40-hex commit SHA followed by an exact `# vX.Y.Z` comment. The comment is what keeps the pin in the monthly `dependency-refresh` sweep, which ranks a SHA pin by it to the patch. Offline, so it cannot check that the SHA matches the comment — resolve both from one tag lookup. | | `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `local:dco` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus one local-only one — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it, and queued gates start in the order they arrived ([#2473](https://github.com/modelcontextprotocol/inspector/issues/2473)). Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection) — unless its lock directory cannot be removed, in which case the wait runs to its 45-minute cap and names the path; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. Why it is a queue rather than a load wait or a worker cap, and what it does and does not change about capacity, is [Multi-agent testing](#multi-agent-testing-the-gate-lease). | | `npm run local:validate` | The gate's first stage: `validate` with each client's `test` leg removed (#2341) — the same `validate:guards` and `validate:core`, then every client's `check` — `format:check` + `lint` + `typecheck`, plus `build` for web, tui and launcher, which name one explicitly (cli's only validate-time build was `test`'s `pretest` hook, so it goes with the `test` leg and `coverage:cli` builds the binary once, later) — each client's `validate` being `check && test`. The suites run once, under `coverage`, instead of bare here and instrumented there. In the `local:` namespace so the workflow guard keeps it out of CI, where the bare pass is free. | -| `npm run local:dco` | The gate's second stage ([#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)): `scripts/dco-check.mjs` over `origin/v2/main..HEAD`, so a commit missing its `Signed-off-by:` trailer fails before it is pushed rather than on the PR's `DCO` check. The same script `.github/workflows/dco.yml` runs on every v2 PR and on every push to `v2/main`. Local-only: it reads `origin/v2/main`, which CI's shallow push checkout does not have, and `workflow-gate.test.mjs` pins it out of `validate`. A stale `origin/v2/main` only widens the range to commits that are already merged. | +| `npm run local:dco` | The gate's second stage ([#2616](https://github.com/modelcontextprotocol/inspector/issues/2616)): `scripts/dco-check.mjs` over `origin/v2/main..HEAD`, excluding anything already on `origin/main` (`--exclude`; on a milestone-merge branch, cut from `main`, the bare range would be `main`'s own released and largely unsigned history), so a commit missing its `Signed-off-by:` trailer fails before it is pushed rather than on the PR's `DCO` check. The same script `.github/workflows/dco.yml` runs on every v2 PR and on every push to `v2/main`. Local-only: it reads `origin/v2/main`, which CI's shallow push checkout does not have, and `workflow-gate.test.mjs` pins it out of `validate`. A stale `origin/v2/main` only widens the range to commits that are already merged. | | `npm run local:storybook` | The gate's last stage: from `clients/web`, `npx playwright install chromium` and then `test:storybook` — the Storybook play functions, run headless by Vitest's browser project (CI runs `test:storybook` directly after its own Playwright install step, which is why this wrapper is in the `local:` namespace). Since [#2340](https://github.com/modelcontextprotocol/inspector/issues/2340) (PR [#2342](https://github.com/modelcontextprotocol/inspector/pull/2342)) the project's `optimizeDeps` carries `force: true` (`getStorybookOptimizeDeps` in `clients/web/server/vite-base-config.ts`), so Vite discards its dep pre-bundle cache and re-scans every story entry before the browser opens — the cold path CI takes on every PR. Without it, Vite keys that cache on the lockfile and the config, never on what the story graph imports, so after a story gains an import of an already-installed package the "valid" cache re-runs the optimizer mid-run and sends a `full-reload` the Vitest tester iframes do not act on; every story an already-loaded iframe renders from then on fails with `Failed to fetch dynamically imported module`, and the next run is green because the cache has caught up. Measured: a stale cache went 3/3 red → 3/3 green with `force` → 3/3 red reverted, load refuted as a cause (0 of 6 under load 8–12), cost ~0.8s per run against a ~25s stage. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/package.json b/package.json index df2e4a1dc..6dabb9f3a 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "local:gate": "node scripts/gate-lease.mjs npm run local:gate:stages", "local:gate:stages": "npm run local:validate && npm run local:dco && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", "local:validate": "npm run validate:guards && npm run validate:core && cd clients/web && npm run check && cd ../cli && npm run check && cd ../mcpdo && npm run check && cd ../tui && npm run check && cd ../launcher && npm run check", - "local:dco": "node scripts/dco-check.mjs --base origin/v2/main", + "local:dco": "node scripts/dco-check.mjs --base origin/v2/main --exclude origin/main", "local:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", diff --git a/scripts/dco-check.mjs b/scripts/dco-check.mjs index 59ffbc69f..f9135dc56 100644 --- a/scripts/dco-check.mjs +++ b/scripts/dco-check.mjs @@ -1,5 +1,6 @@ #!/usr/bin/env node -// DCO signoff check (#2566) — `npm run dco:check -- --base [--head ]`. +// DCO signoff check (#2566) — `npm run dco:check -- --base [--head ] +// [--exclude …]`. // // Replaces the probot DCO app, which was suspended and whose check simply // stopped appearing after #1981 (2026-08-12). Nothing failed when it vanished, @@ -50,12 +51,20 @@ const BOT_EMAIL = /^\d+\+[^@\s]+\[bot\]@users\.noreply\.github\.com$/i; export function parseDcoArgs(argv) { const { values } = parseArgs({ args: argv, - options: { base: { type: "string" }, head: { type: "string" } }, + options: { + base: { type: "string" }, + head: { type: "string" }, + exclude: { type: "string", multiple: true }, + }, }); if (!values.base) { throw new Error("--base is required (e.g. origin/v2/main)"); } - return { base: values.base, head: values.head ?? "HEAD" }; + return { + base: values.base, + head: values.head ?? "HEAD", + exclude: values.exclude ?? [], + }; } /** Split `git log -z --format=` output into commit records. */ @@ -151,16 +160,37 @@ export function classify(commits) { return { checked: commits.length - exempt, exempt, failures }; } -function readRange(spawn, base, head) { +/** + * `--exclude ` drops everything reachable from `rev` as well, on top of + * `base` (#2616). `local:dco` excludes `origin/main`: a milestone-merge branch + * is cut from `main`, so `origin/v2/main..HEAD` there is `main`'s own history + * — 117 already-released commits, the v1 era and the 2.0.0 candidates among + * them, almost none signed — and the gate the release runs on that branch + * would fail on all of it. A commit already on `main` has shipped, so it says + * nothing about the change being checked. On a feature branch the exclusion + * is a no-op, since it holds nothing `main` has that `v2/main` lacks. + */ +function describeRange(base, head, exclude) { + const range = `${base}..${head}`; + return exclude.length ? `${range} (excluding ${exclude.join(", ")})` : range; +} + +function readRange(spawn, base, head, exclude) { const result = spawn( "git", - ["log", "-z", `--format=${LOG_FORMAT}`, `${base}..${head}`], + [ + "log", + "-z", + `--format=${LOG_FORMAT}`, + `${base}..${head}`, + ...exclude.map((rev) => `^${rev}`), + ], { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }, ); if (result.error) throw result.error; if (result.status !== 0) { throw new Error( - `git log ${base}..${head} failed: ${(result.stderr ?? "").trim()}`, + `git log ${describeRange(base, head, exclude)} failed: ${(result.stderr ?? "").trim()}`, ); } return parseLog(result.stdout); @@ -168,20 +198,21 @@ function readRange(spawn, base, head) { /** Returns the process exit code: 0 when every commit passes, 1 otherwise. */ export function main(argv = process.argv.slice(2), spawn = spawnSync) { - const { base, head } = parseDcoArgs(argv); - const commits = readRange(spawn, base, head); + const { base, head, exclude } = parseDcoArgs(argv); + const commits = readRange(spawn, base, head, exclude); + const range = describeRange(base, head, exclude); const { checked, exempt, failures } = classify(commits); if (failures.length === 0) { console.log( - `dco: OK — ${checked} commit(s) signed off in ${base}..${head}` + + `dco: OK — ${checked} commit(s) signed off in ${range}` + (exempt ? ` (${exempt} merge/bot commit(s) exempt)` : ""), ); return 0; } console.error( - `dco: FAIL — ${failures.length} of ${checked} commit(s) in ${base}..${head} lack a matching signoff:\n`, + `dco: FAIL — ${failures.length} of ${checked} commit(s) in ${range} lack a matching signoff:\n`, ); for (const { commit, reason } of failures) { const subject = commit.message.split("\n", 1)[0]; diff --git a/scripts/dco-check.test.mjs b/scripts/dco-check.test.mjs index 914078633..510b5e885 100644 --- a/scripts/dco-check.test.mjs +++ b/scripts/dco-check.test.mjs @@ -38,11 +38,17 @@ test("parseDcoArgs requires --base and defaults --head to HEAD", () => { assert.deepEqual(parseDcoArgs(["--base", "origin/v2/main"]), { base: "origin/v2/main", head: "HEAD", + exclude: [], }); assert.deepEqual(parseDcoArgs(["--base", "a", "--head", "b"]), { base: "a", head: "b", + exclude: [], }); + assert.deepEqual( + parseDcoArgs(["--base", "a", "--exclude", "x", "--exclude", "y"]).exclude, + ["x", "y"], + ); assert.throws(() => parseDcoArgs([]), /--base/); }); @@ -231,7 +237,7 @@ function makeRepo() { const spawnIn = (cwd) => (cmd, args, opts) => spawnSync(cmd, args, { ...opts, cwd }); -function runMain(dir) { +function runMain(dir, extra = []) { const out = []; const err = []; const log = console.log; @@ -239,7 +245,7 @@ function runMain(dir) { console.log = (line) => out.push(line); console.error = (line) => err.push(line); try { - const code = main(["--base", "base"], spawnIn(dir)); + const code = main(["--base", "base", ...extra], spawnIn(dir)); return { code, out: out.join("\n"), err: err.join("\n") }; } finally { console.log = log; @@ -256,6 +262,22 @@ test("main passes a range of signed commits and ignores the base's history", () assert.match(out, /dco: OK — 2 commit\(s\) signed off in base\.\.HEAD/); }); +test("main --exclude drops a released branch's history from the range", () => { + // The milestone-merge shape (#2616): a branch cut from `main`, whose own + // unsigned history must not fail a gate run against `v2/main`. + const dir = makeRepo(); + git(dir, ["commit", "-q", "--allow-empty", "-m", "released, unsigned"]); + git(dir, ["branch", "released"]); + git(dir, ["commit", "-q", "-s", "--allow-empty", "-m", "new, signed"]); + assert.equal(runMain(dir).code, 1, "without --exclude the old commit fails"); + const { code, out } = runMain(dir, ["--exclude", "released"]); + assert.equal(code, 0); + assert.match( + out, + /1 commit\(s\) signed off in base\.\.HEAD \(excluding released\)/, + ); +}); + test("main fails on one unsigned commit and prints the repair", () => { const dir = makeRepo(); git(dir, ["commit", "-q", "-s", "--allow-empty", "-m", "signed"]); diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index 332048047..dbc04bd81 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -621,7 +621,7 @@ describe("the gate's name", () => { // must stay unreachable from `validate`, which CI runs. assert.equal( scripts["local:dco"], - "node scripts/dco-check.mjs --base origin/v2/main", + "node scripts/dco-check.mjs --base origin/v2/main --exclude origin/main", ); assert.ok( scriptChainRuns(scripts, "local:gate", "local:dco"),