From 5c37544386382c2453c03e4b9617534d31902798 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Sun, 6 Sep 2026 19:55:52 +0000 Subject: [PATCH 1/3] lint: warn on long comment blocks (local/comment-length) and state the norm in AGENTS.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment blocks over 15 lines (25 for a file header) get a warning whose text tells the author — human or agent — to keep the rule and the non-obvious why in the comment and move history/process notes to the PR description. Implemented as an inline local rule in eslint.config.mjs (no new dependency); vendored src/spec-types are exempt; the ten pre-existing long comments that are genuine API/scenario contracts carry a targeted disable with a reason. Warnings do not fail npm run lint / CI; setup-node's eslint-stylish problem matcher turns them into PR annotations. Co-Authored-By: Claude --- AGENTS.md | 3 + CONTRIBUTING.md | 1 + eslint.config.mjs | 102 +++++++++++++++++- .../typescript/helpers/dpopClientFlow.ts | 1 + src/expected-failures.ts | 1 + src/mock-server/stateless.ts | 1 + src/scenarios/client/auth/dpop.ts | 1 + src/scenarios/client/auth/issuer-parameter.ts | 1 + .../client/auth/resource-mismatch.ts | 1 + .../json-schema-2020-12-preservation.ts | 1 + src/scenarios/client/json-schema-ref-deref.ts | 1 + src/scenarios/server/tasks/helpers.ts | 1 + .../server/tasks/required-task-error.ts | 1 + 13 files changed, 115 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index ca88ff87..f1d34323 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,6 +63,8 @@ Be precise about what's **required** vs **optional**. A scenario description tha When in doubt about spec details (OAuth parameters, audiences, grant types), check the actual spec in `modelcontextprotocol` rather than guessing. +**Code comments are short.** A comment states the rule being enforced and the non-obvious "why" in a few lines; spec history, process notes, review back-and-forth and PR context belong in the PR description (or an issue), not in the source. `npm run lint` warns (`local/comment-length`) on comment blocks over 15 lines (25 for a file header) — treat that warning as "move this to the PR description", not as an invitation to add an eslint-disable. + ## Reviewing PRs ### SEP scenarios @@ -121,6 +123,7 @@ Use the existing CLI runner (`npx @modelcontextprotocol/conformance client|serve - `npm run build` passes - `npm test` passes +- `npm run lint` is clean, including warnings (a `local/comment-length` warning means a comment should be trimmed, see "Descriptions and wording") - For non-trivial scenario changes, run against at least one real SDK (typescript-sdk or python-sdk) to see actual output. For changes to shared infrastructure (runner, tier-check), test against go-sdk or csharp-sdk too. - Scenario is registered in the right suite in `src/scenarios/index.ts` - If you changed a `sep-*.yaml` or scenario check IDs, `src/seps/traceability.json` will drift; the traceability workflow refreshes it via PR (or regenerate locally with `--results` from a suite run) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 920b1750..382db97f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,3 +69,4 @@ See the [README](./README.md) for full CLI options and the [SDK Integration Guid - Register your scenario in the right suite in `src/scenarios/index.ts` - Run against at least one real SDK (see above) before opening the PR — we'll ask what the output looked like - Keep PRs focused; one feature or scenario group at a time +- Keep code comments short (the rule and the non-obvious why); put history and process notes in the PR description — `npm run lint` warns on long comment blocks diff --git a/eslint.config.mjs b/eslint.config.mjs index 8d98c92d..7ad81403 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -4,6 +4,97 @@ import eslint from '@eslint/js'; import tseslint from 'typescript-eslint'; import eslintConfigPrettier from 'eslint-config-prettier/flat'; +/** + * local/comment-length: warn on comment blocks longer than `max` lines. + * Long comments in this repo are almost always process narration or history + * that belongs in the PR description; the warning text says so, so that an + * agent running `npm run lint` can act on it without a human round-trip. + * @type {import('eslint').Rule.RuleModule} + */ +const commentLength = { + meta: { + type: 'suggestion', + schema: [ + { + type: 'object', + properties: { + max: { type: 'integer', minimum: 1 }, + maxHeader: { type: 'integer', minimum: 1 } + }, + additionalProperties: false + } + ], + messages: { + tooLong: + 'Comment block is {{lines}} lines (max {{max}}). Keep comments to the rule being enforced and the non-obvious "why"; move history, process notes and PR context to the PR description.', + tooLongHeader: + 'File header comment is {{lines}} lines (max {{max}}). Say what the file is for in a few lines; move design history and process notes to the PR description or an issue.' + } + }, + create(context) { + const max = context.options[0]?.max ?? 15; + // A file's first comment (module header) gets a larger allowance. + const maxHeader = context.options[0]?.maxHeader ?? 25; + const sourceCode = context.sourceCode; + const isDirective = (c) => + /^\s*(eslint-disable|eslint-enable|eslint\s|global\s|@ts-|\/\s* + sourceCode.lines[c.loc.start.line - 1] + .slice(0, c.loc.start.column) + .trim() === ''; + return { + Program() { + /** @type {import('estree').Comment[]} */ + let run = []; + const check = (loc, lines) => { + const header = loc.start.line <= 3; + const limit = header ? maxHeader : max; + if (lines > limit) { + context.report({ + loc, + messageId: header ? 'tooLongHeader' : 'tooLong', + data: { lines: String(lines), max: String(limit) } + }); + } + }; + const flush = () => { + if (run.length > 0) { + check( + { start: run[0].loc.start, end: run[run.length - 1].loc.end }, + run.length + ); + } + run = []; + }; + for (const c of sourceCode.getAllComments()) { + if (c.type === 'Shebang' || isDirective(c) || !c.loc) { + flush(); + continue; + } + if (c.type === 'Line') { + const prev = run[run.length - 1]; + if ( + startsLine(c) && + (!prev || prev.loc.end.line + 1 === c.loc.start.line) + ) { + run.push(c); + } else { + flush(); + if (startsLine(c)) run.push(c); + } + continue; + } + flush(); + check(c.loc, c.loc.end.line - c.loc.start.line + 1); + } + flush(); + } + }; + } +}; + export default tseslint.config( eslint.configs.recommended, ...tseslint.configs.recommended, @@ -11,13 +102,22 @@ export default tseslint.config( linterOptions: { reportUnusedDisableDirectives: false }, + plugins: { + local: { rules: { 'comment-length': commentLength } } + }, rules: { '@typescript-eslint/no-unused-vars': [ 'error', { argsIgnorePattern: '^_' } ], - '@typescript-eslint/no-explicit-any': 'off' + '@typescript-eslint/no-explicit-any': 'off', + 'local/comment-length': ['warn', { max: 15, maxHeader: 25 }] } }, + { + // Vendored spec schema typings carry the spec's own long docblocks. + files: ['src/spec-types/**'], + rules: { 'local/comment-length': 'off' } + }, eslintConfigPrettier ); diff --git a/examples/clients/typescript/helpers/dpopClientFlow.ts b/examples/clients/typescript/helpers/dpopClientFlow.ts index f4cb81ff..f0a22ee0 100644 --- a/examples/clients/typescript/helpers/dpopClientFlow.ts +++ b/examples/clients/typescript/helpers/dpopClientFlow.ts @@ -8,6 +8,7 @@ import { } from '../../../../src/scenarios/client/auth/helpers/dpopProof'; import { logger } from './logger'; +// eslint-disable-next-line local/comment-length -- pre-existing step-by-step protocol walkthrough for the example client /** * Shared DPoP client flow (SEP-1932 / RFC 9449). Acquires a DPoP-bound access * token via the authorization_code + PKCE grant (with a DPoP proof at the token diff --git a/src/expected-failures.ts b/src/expected-failures.ts index 0f372f49..3c00ac32 100644 --- a/src/expected-failures.ts +++ b/src/expected-failures.ts @@ -150,6 +150,7 @@ export async function loadExpectedFailures( return result; } +// eslint-disable-next-line local/comment-length -- pre-existing baseline evaluation contract /** * Evaluate scenario results against an expected-failures baseline. * diff --git a/src/mock-server/stateless.ts b/src/mock-server/stateless.ts index 3d06c86a..0c982198 100644 --- a/src/mock-server/stateless.ts +++ b/src/mock-server/stateless.ts @@ -77,6 +77,7 @@ export type StatelessValidation = params: Record; }; +// eslint-disable-next-line local/comment-length -- pre-existing shared validation contract used by other mock servers /** * Shared SEP-2575 request validation: header presence, `_meta` 3-key check, * header/`_meta` version match, version-supported check, and `server/discover` diff --git a/src/scenarios/client/auth/dpop.ts b/src/scenarios/client/auth/dpop.ts index 8b16ba5a..40b40397 100644 --- a/src/scenarios/client/auth/dpop.ts +++ b/src/scenarios/client/auth/dpop.ts @@ -79,6 +79,7 @@ const CHECK_DEFS: Record< } }; +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * Scenario: DPoP sender-constrained tokens — MCP client (SEP-1932 / RFC 9449). * diff --git a/src/scenarios/client/auth/issuer-parameter.ts b/src/scenarios/client/auth/issuer-parameter.ts index 85773e23..80788579 100644 --- a/src/scenarios/client/auth/issuer-parameter.ts +++ b/src/scenarios/client/auth/issuer-parameter.ts @@ -14,6 +14,7 @@ const metadataSpecRefs = [ SpecReferences.MCP_AUTH_DISCOVERY ]; +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * Reason-bound verdict for the RFC 9207 `iss` rejection checks (issue #467). * diff --git a/src/scenarios/client/auth/resource-mismatch.ts b/src/scenarios/client/auth/resource-mismatch.ts index a5cc157a..de4e88fd 100644 --- a/src/scenarios/client/auth/resource-mismatch.ts +++ b/src/scenarios/client/auth/resource-mismatch.ts @@ -8,6 +8,7 @@ import { SpecReferences } from './spec-references.js'; import { MockTokenVerifier } from './helpers/mockTokenVerifier.js'; import { untestableCheck } from '../../untestable.js'; +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * Scenario: Resource Mismatch Detection * diff --git a/src/scenarios/client/json-schema-2020-12-preservation.ts b/src/scenarios/client/json-schema-2020-12-preservation.ts index 15185958..0779cb40 100644 --- a/src/scenarios/client/json-schema-2020-12-preservation.ts +++ b/src/scenarios/client/json-schema-2020-12-preservation.ts @@ -1,3 +1,4 @@ +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * Client-side JSON Schema 2020-12 keyword preservation (SEP-1613, SEP-2106) * diff --git a/src/scenarios/client/json-schema-ref-deref.ts b/src/scenarios/client/json-schema-ref-deref.ts index aff791d3..dda64464 100644 --- a/src/scenarios/client/json-schema-ref-deref.ts +++ b/src/scenarios/client/json-schema-ref-deref.ts @@ -9,6 +9,7 @@ import type { Scenario, ConformanceCheck } from '../../types'; import express, { Request, Response } from 'express'; import { ScenarioUrls, DRAFT_PROTOCOL_VERSION } from '../../types'; +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * Scenario: JSON Schema network $ref dereferencing (SEP-2106) * diff --git a/src/scenarios/server/tasks/helpers.ts b/src/scenarios/server/tasks/helpers.ts index b786d9ca..9a427827 100644 --- a/src/scenarios/server/tasks/helpers.ts +++ b/src/scenarios/server/tasks/helpers.ts @@ -12,6 +12,7 @@ import type { Connection } from '../../../connection'; export const TASKS_EXTENSION_ID = 'io.modelcontextprotocol/tasks'; +// eslint-disable-next-line local/comment-length -- pre-existing fixture contract shared by the tasks scenarios /** * Baseline "otherwise well-formed" params for a tasks-namespace * method, used by negative-path checks that need to isolate a single diff --git a/src/scenarios/server/tasks/required-task-error.ts b/src/scenarios/server/tasks/required-task-error.ts index 1da07c8f..b7c07da7 100644 --- a/src/scenarios/server/tasks/required-task-error.ts +++ b/src/scenarios/server/tasks/required-task-error.ts @@ -1,3 +1,4 @@ +// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** * SEP-2663 Tasks Extension — required-task error conformance. * From 6e717b00bdcc3cef8021d48b91d7004df448468b Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Sun, 6 Sep 2026 19:55:52 +0000 Subject: [PATCH 2/3] docs(AGENTS): comments are standalone; no review/PR-time context --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index f1d34323..953898a1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,7 +63,7 @@ Be precise about what's **required** vs **optional**. A scenario description tha When in doubt about spec details (OAuth parameters, audiences, grant types), check the actual spec in `modelcontextprotocol` rather than guessing. -**Code comments are short.** A comment states the rule being enforced and the non-obvious "why" in a few lines; spec history, process notes, review back-and-forth and PR context belong in the PR description (or an issue), not in the source. `npm run lint` warns (`local/comment-length`) on comment blocks over 15 lines (25 for a file header) — treat that warning as "move this to the PR description", not as an invitation to add an eslint-disable. +**Code comments are short and standalone.** A comment states the rule being enforced and the non-obvious "why" in a few lines: invariants, ordering constraints, why-not. Delete comments that restate what the name or code already says, and never reference review-time or PR-time context (a reviewer ask, an option that was discussed, the history of a SEP): that belongs in the PR description or an issue, and a comment must make sense to a reader who never saw the PR. `npm run lint` warns (`local/comment-length`) on comment blocks over 15 lines (25 for a file header); treat that warning as "move this to the PR description", not as an invitation to add an eslint-disable. ## Reviewing PRs From 4cd0c9394ac11deb567f4a446abbc460d1f4dae6 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Sun, 6 Sep 2026 19:55:52 +0000 Subject: [PATCH 3/3] chore: trim explanatory comments from #476 and #483 to the standalone why Comment-only change. Drops review-time narration (which sites had drifted, what the pre-fix behaviour was) and keeps the rule plus the non-obvious reason, per AGENTS.md. Co-Authored-By: Claude --- .../clients/typescript/auth-test-inert.ts | 11 +++----- src/scenarios/client/auth/issuer-parameter.ts | 28 +++++-------------- src/scenarios/client/http-base.ts | 17 ++++------- 3 files changed, 16 insertions(+), 40 deletions(-) diff --git a/examples/clients/typescript/auth-test-inert.ts b/examples/clients/typescript/auth-test-inert.ts index 74e9be3f..43cc9e4c 100644 --- a/examples/clients/typescript/auth-test-inert.ts +++ b/examples/clients/typescript/auth-test-inert.ts @@ -5,13 +5,10 @@ import { runAsCli } from './helpers/cliRunner'; /** * Broken client that gives up before performing any discovery request. * - * BUG: it never fetches Protected Resource Metadata, so it never reads — let - * alone validates — the `resource` value the scenario mismatches on purpose. - * - * It exists to pin issue #467. `auth/resource-mismatch` decides its verdict - * from `!authorizationRequestMade` alone, and a client that does nothing at - * all satisfies that verdict, so the check scores SUCCESS for a client that - * cannot possibly have performed the validation under test. + * BUG: it never fetches Protected Resource Metadata, so it cannot have + * validated the `resource` value `auth/resource-mismatch` mismatches on + * purpose. Pins #467: the negative checks must report this client as + * untestable, not as having correctly rejected anything. */ export async function runClient(_serverUrl: string): Promise { throw new Error( diff --git a/src/scenarios/client/auth/issuer-parameter.ts b/src/scenarios/client/auth/issuer-parameter.ts index 80788579..bd174d11 100644 --- a/src/scenarios/client/auth/issuer-parameter.ts +++ b/src/scenarios/client/auth/issuer-parameter.ts @@ -14,28 +14,14 @@ const metadataSpecRefs = [ SpecReferences.MCP_AUTH_DISCOVERY ]; -// eslint-disable-next-line local/comment-length -- pre-existing scenario contract /** - * Reason-bound verdict for the RFC 9207 `iss` rejection checks (issue #467). - * - * `authReached && !tokenRequestMade` is a verdict, not a reason. SEP-2468 - * conditions every one of these requirements on the issuer the client recorded - * "from the selected authorization server validated metadata document", so a - * client that never retrieved that document cannot have performed the - * comparison under test — yet it satisfies the verdict, because not reaching - * the token endpoint is exactly what a client that fell over earlier also - * does. Absent the retrieval the requirement was never exercised, which is the - * untestable case (#248) rather than a pass or a violation. - * - * `auth/metadata-issuer-mismatch` in this same file already gates on the - * metadata fetch; the other checks did not. Keeping the policy in one function - * is deliberate: the duplication is what let five of six sites drift apart. - * - * Residual gap, deliberately not papered over: a client that receives the - * redirect and then aborts before the token request for an unrelated reason is - * still indistinguishable from one that rejected on `iss`. Closing that needs - * a signal from inside the client, which a black-box harness does not have. - * What this closes is the "never reached the requirement at all" class. + * Verdict for the RFC 9207 `iss` rejection checks. "Reached the authorization + * endpoint and made no token request" only proves rejection if the client also + * fetched the AS metadata that carries the issuer it must compare against; + * without that fetch the requirement was never exercised, so the check is + * reported as untestable (#248) rather than passed or failed. A client that + * aborts after the redirect for an unrelated reason is still indistinguishable + * from one that rejected on `iss`; a black-box harness cannot close that gap. */ function issRejectionCheck(opts: { id: string; diff --git a/src/scenarios/client/http-base.ts b/src/scenarios/client/http-base.ts index 6751dedd..21b49fb5 100644 --- a/src/scenarios/client/http-base.ts +++ b/src/scenarios/client/http-base.ts @@ -21,18 +21,11 @@ import { } from '../../types.js'; /** - * Schema-valid empty results for the standard list-shaped methods, keyed by - * method. A Map, not an object literal, so a method name that collides with - * Object.prototype ("constructor", "toString", ...) misses instead of - * returning a function. Merged into the generic fallback so a list method a - * scenario does not route still carries its required list member — a bare - * `{}` fails schema validation and strict clients drop the connection before - * the scenario's real checks run (#474). `tasks/list` exists only at - * 2025-11-25 (the draft schema has no ListTasksResult); the empty member is - * harmless on the draft wire. Non-list results (tools/call, resources/read, - * prompts/get, ...) have no meaningful empty default and keep the bare - * stamped fallback, so a route a scenario forgot surfaces instead of being - * masked. + * Schema-valid empty results for the standard list methods, merged into the + * generic fallback so an unrouted list method still satisfies its result + * schema (a bare `{}` makes strict clients drop the connection, #474). A Map so + * names colliding with Object.prototype miss. Non-list methods keep the bare + * fallback so a route a scenario forgot still surfaces. */ const EMPTY_LIST_RESULTS: ReadonlyMap = new Map([ ['tools/list', { tools: [] }],