lint: warn on long comment blocks; trim three that landed this week

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 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
 
 ### 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)

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

eslint.config.mjs

@@ -4,20 +4,120 @@ 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*<reference)/.test(
+        c.value
+      );
+    const startsLine = (c) =>
+      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,
   {
     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
 );

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<void> {
   throw new Error(

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

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.
  *

src/mock-server/stateless.ts

@@ -77,6 +77,7 @@ export type StatelessValidation =
       params: Record<string, unknown>;
     };
 
+// 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`

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).
  *

src/scenarios/client/auth/issuer-parameter.ts

@@ -15,26 +15,13 @@ const metadataSpecRefs = [
 ];
 
 /**
- * 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;

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
  *

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<string, object> = new Map([
   ['tools/list', { tools: [] }],

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)
  *

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)
  *

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

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.
  *