diff --git a/.github/workflows/mutation-testing.yml b/.github/workflows/mutation-testing.yml index 2da0c5c5b4..83e52182bc 100644 --- a/.github/workflows/mutation-testing.yml +++ b/.github/workflows/mutation-testing.yml @@ -46,7 +46,7 @@ jobs: if: github.event_name == 'pull_request' run: pnpm test:mutation-ci - - name: Enforce executable-line scope and run advisory mutation testing + - name: Enforce executable-line scope and directive policy, run advisory mutation testing if: github.event_name == 'pull_request' env: HEAD_SHA: ${{ github.sha }} diff --git a/AGENTS.md b/AGENTS.md index ce1d760892..8c8b2b8298 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,14 @@ New or substantially changed test files must not add `@typescript-eslint/no-expl - Validate with `pnpm --dir src run check-types`, the narrowest affected Vitest suite, and `pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 `; the run must not add a new entry or raise an existing count. - An exception requires a comment next to the cast explaining why no typed alternative exists and maintainer approval in review. +## Stryker Suppression Comments + +Do not add Stryker suppression comments (`// Stryker disable ...`, `// Stryker enable ...`) to source files. The changed-code mutation gate (`scripts/stryker-diff.mjs`, run by the `mutation-diff` CI job) fails any pull request that introduces them in mutation-tested files. + +- Kill surviving or uncovered mutants by adding or strengthening a focused test that fails under the mutation. +- If a mutant is genuinely equivalent, explain the equivalence in the PR description; maintainers decide on narrow gate-config exclusions. Do not add a suppression locally to get green. +- Suppressions left by earlier revisions are removed when their file is touched; do not reintroduce them. + ## Persisted Setting Checklist When adding or changing a user setting, trace the complete round trip. A setting is not complete merely because its control renders or its value reaches storage. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 61bb417bf2..16f09df1fe 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -119,6 +119,7 @@ pnpm install - Follow the ESLint and TypeScript best practices. - Write clear, descriptive commits that reference issues (for example, `Fixes #123`). Follow the [Writing Style](#writing-style). - Test your change fully (`npm test`). +- Do not add Stryker suppression comments (`// Stryker disable ...` or `// Stryker enable ...`). The mutation-testing CI check rejects them in any touched mutation-tested file, including directives left by earlier revisions. Remove existing directives when you touch the file. Kill surviving mutants with focused tests; if a mutant is genuinely equivalent, explain the equivalence in the pull request body instead. - Run both dead-code checks: `pnpm knip` and `pnpm knip:production`. See [Dead-code checks before review](docs/development-quality-checks.md) for findings, exceptions, and verification. - Rebase onto the latest `main` branch before you submit. diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index 9742879106..5a8c5f8b22 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -74,8 +74,7 @@ const VALID_MUTANT_STATUSES = new Set(["Killed", "Timeout", "Survived", "NoCover const BLOCKING_MUTANT_STATUSES = new Set(["Survived", "NoCoverage"]) const SOURCE_EXTENSIONS = new Set([".js", ".jsx", ".mjs", ".cjs", ".ts", ".tsx"]) const EXCLUDED_PATH_SEGMENTS = ["/__mocks__/", "/__tests__/", "/fixtures/", "/test-utils/"] -const DISABLE_DIRECTIVE = - /^\s*\/\/\s*Stryker disable next-line ([A-Za-z][A-Za-z0-9]*(?:,[A-Za-z][A-Za-z0-9]*)*)\s*:\s*(\S.*)$/ +const SUPPRESSION_DIRECTIVE = /\/\/\s*Stryker\s+(?:disable|enable)\b/ export function parseNameStatus(output) { const tokens = output.split("\0") @@ -159,20 +158,42 @@ export function executableChangedLines(source, changedLines, filePath = "source. return executable } -export function validateDisableDirectives(source, changedLines, filePath) { - const lines = source.split(/\r?\n/) - for (const lineNumber of changedLines) { - const line = lines[lineNumber - 1] ?? "" - if (!line.includes("Stryker disable")) continue - - const match = DISABLE_DIRECTIVE.exec(line) - if (!match || match[1].split(",").some((mutator) => mutator.toLowerCase() === "all")) { - throw new Error( - `${filePath}:${lineNumber}: mutation exclusions must use ` + - '"// Stryker disable next-line MutatorName: concrete reason"; broad or unreasoned exclusions are not allowed.', - ) +export function assertNoDisableDirectives(source, lineNumbers, filePath) { + // Comment ranges come from the parsed source: string literals and template text that mention + // the directive syntax must not fail the gate, and a raw scanner misclassifies text after a + // template interpolation because it never resumes the template lexical goal. + const scriptKind = filePath.endsWith("x") ? ts.ScriptKind.TSX : ts.ScriptKind.TS + const sourceFile = ts.createSourceFile(filePath, source, ts.ScriptTarget.Latest, true, scriptKind) + const seen = new Set() + function checkRanges(ranges) { + for (const range of ranges ?? []) { + if (seen.has(range.pos)) continue + seen.add(range.pos) + // getLeadingCommentRanges is text-based: inside JSX, directive-looking rendered text + // (e.g.
// Stryker disable all) is JsxText, not a comment, and must not fail. + if (ts.getTokenAtPosition(sourceFile, range.pos).kind === ts.SyntaxKind.JsxText) continue + if (!SUPPRESSION_DIRECTIVE.test(source.slice(range.pos, range.end))) continue + const lineNumber = (source.slice(0, range.pos).match(/\n/g)?.length ?? 0) + 1 + if (lineNumbers.has(lineNumber)) { + throw new Error( + `${filePath}:${lineNumber}: Stryker suppression comments are not allowed in this repository. ` + + "Kill the mutant with a focused test instead; if the mutant is genuinely equivalent, explain the " + + "equivalence in the pull request and a maintainer will decide on a narrow gate-config exclusion.", + ) + } } } + function visitTokens(node) { + for (const child of node.getChildren(sourceFile)) { + if (ts.isToken(child)) { + checkRanges(ts.getLeadingCommentRanges(source, child.pos)) + checkRanges(ts.getTrailingCommentRanges(source, child.end)) + } else { + visitTokens(child) + } + } + } + visitTokens(sourceFile) } export function toRanges(lines) { @@ -225,11 +246,7 @@ export function buildManifest(entries, readSource, diffForPath) { ? new Set(Array.from({ length: sourceLineCount }, (_, index) => index + 1)) : parseChangedLines(diffForPath(entry.path)) - try { - validateDisableDirectives(source, new Set(source.split(/\r?\n/).map((_, index) => index + 1)), entry.path) - } catch (error) { - advisories.push(error.message) - } + assertNoDisableDirectives(source, new Set(source.split(/\r?\n/).map((_, index) => index + 1)), entry.path) const executableLines = executableChangedLines(source, changedLines, entry.path) if (executableLines.size === 0) continue @@ -608,14 +625,7 @@ export function formatSummary(rows, advisories, manifest = {}) { lines.push( "### Resolve a mutation gap", "", - "Add or strengthen a focused test that fails under the mutation. If the mutant is equivalent, request maintainer approval for the narrowest mutator-specific exclusion and explain why it cannot change behavior:", - "", - "```ts", - "// Stryker disable next-line ConditionalExpression: normalized input cannot reach the alternate branch", - "const result = condition ? value : fallback", - "```", - "", - "Broad `all` exclusions and exclusions without a concrete reason are reported as advisories.", + "Add or strengthen a focused test that fails under the mutation. Stryker suppression comments (`// Stryker disable ...`) are not allowed in this repository: the gate fails any pull request that introduces them. If you believe a mutant is genuinely equivalent, explain why in the pull request and a maintainer will decide on a narrow gate-config exclusion.", ) } diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index 22284e598d..900b0dd39a 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -11,6 +11,7 @@ import { MAX_MUTANTS, PACKAGE_CONFIGS, appendSummary, + assertNoDisableDirectives, buildManifest, discoverRelatedTestFiles, evaluateReport, @@ -32,7 +33,6 @@ import { runManifest, selectFromGit, testsFromMutationReport, - validateDisableDirectives, } from "./stryker-diff.mjs" const repositoryRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..") @@ -61,7 +61,9 @@ describe("mutation testing workflow", () => { assert.ok(!workflow.includes("github.event.pull_request.base.sha")) assert.ok(workflow.includes("steps.mutation_report.outputs.artifact-url")) assert.ok(workflow.includes("open the package's mutation.html file")) - assert.ok(workflow.includes("Enforce executable-line scope and run advisory mutation testing")) + assert.ok( + workflow.includes("Enforce executable-line scope and directive policy, run advisory mutation testing"), + ) assert.equal(workflow.match(/continue-on-error: true/g)?.length, 1) assert.equal(workflow.match(/Could not write the job summary/g)?.length, 2) const script = fs.readFileSync(path.join(repositoryRoot, "scripts/stryker-diff.mjs"), "utf8") @@ -297,15 +299,16 @@ describe("buildManifest", () => { ) }) - it("reports invalid mutation exclusions without bypassing executable-line accounting", () => { - const manifest = buildManifest( - [{ status: "A", path: "packages/core/src/value.ts" }], - () => "// Stryker disable next-line all: noisy\nexport const value = true\n", - () => "", + it("fails the manifest when a mutation-tested file contains a Stryker suppression comment", () => { + assert.throws( + () => + buildManifest( + [{ status: "A", path: "packages/core/src/value.ts" }], + () => "// Stryker disable next-line all: noisy\nexport const value = true\n", + () => "", + ), + /Stryker suppression comments are not allowed/, ) - - assert.equal(manifest.packages[0].changedExecutableLines, 1) - assert.match(manifest.advisories.join("\n"), /broad or unreasoned exclusions are not allowed/) }) }) @@ -583,35 +586,97 @@ describe("selectFromGit", () => { }) }) -describe("mutation exclusions", () => { - it("allows a targeted mutator exclusion with a reason", () => { +describe("Stryker suppression directive prohibition", () => { + it("rejects every Stryker suppression comment, however well-formed", () => { + for (const source of [ + "// Stryker disable next-line EqualityOperator: equivalent for normalized input\nreturn value <= limit\n", + "// Stryker disable next-line all: noisy\nreturn value\n", + "// Stryker disable next-line EqualityOperator\nreturn value\n", + "// Stryker disable start\nreturn value\n// Stryker disable end\n", + "// Stryker enable next-line EqualityOperator: undo\nreturn value\n", + "\t\t// Stryker disable string\nreturn value\n", + ]) { + assert.throws( + () => assertNoDisableDirectives(source, new Set([1]), "source.ts"), + /Stryker suppression comments are not allowed/, + ) + } + }) + + it("matches comment tokens only, never string literals or template text", () => { assert.doesNotThrow(() => - validateDisableDirectives( - "// Stryker disable next-line EqualityOperator: equivalent for normalized input\nreturn value <= limit\n", - new Set([1]), + assertNoDisableDirectives( + 'export const guidance = "// Stryker disable all"\nreturn value\n', + new Set([1, 2]), + "source.ts", + ), + ) + assert.doesNotThrow(() => + assertNoDisableDirectives( + "const doc = `// Stryker disable next-line all`\nreturn value\n", + new Set([1, 2]), + "source.ts", + ), + ) + assert.doesNotThrow(() => + assertNoDisableDirectives( + "const text = `${value} // Stryker disable all`\nreturn value\n", + new Set([1, 2]), "source.ts", ), ) - }) - - it("rejects broad or unreasoned exclusions", () => { assert.throws( () => - validateDisableDirectives( - "// Stryker disable next-line all: noisy\nreturn value\n", - new Set([1]), + assertNoDisableDirectives( + "const text = `${value}`\n// Stryker disable next-line all: reason\nreturn value\n", + new Set([1, 2, 3]), "source.ts", ), - /broad or unreasoned exclusions are not allowed/, + /Stryker suppression comments are not allowed/, ) assert.throws( () => - validateDisableDirectives( - "// Stryker disable next-line EqualityOperator\nreturn value\n", + assertNoDisableDirectives( + "const a = `x${`y${z}`} tail` // Stryker disable all: reason\nreturn value\n", + new Set([1, 2]), + "source.ts", + ), + /Stryker suppression comments are not allowed/, + ) + assert.throws( + () => + assertNoDisableDirectives( + "return value // Stryker disable next-line all: reason\n", new Set([1]), "source.ts", ), - /broad or unreasoned exclusions are not allowed/, + /Stryker suppression comments are not allowed/, + ) + }) + + it("ignores directive-looking JSX text in tsx sources", () => { + assert.doesNotThrow(() => + assertNoDisableDirectives( + "const el =
// Stryker disable all\n
\n", + new Set([1, 2]), + "source.tsx", + ), + ) + assert.throws( + () => + assertNoDisableDirectives( + "const el =
{ // Stryker disable next-line all: reason\nx}
\n", + new Set([1, 2]), + "source.tsx", + ), + /Stryker suppression comments are not allowed/, + ) + }) + + it("accepts sources without suppression comments", () => { + assert.doesNotThrow(() => assertNoDisableDirectives("return value <= limit\n", new Set([1]), "source.ts")) + assert.doesNotThrow(() => + assertNoDisableDirectives("// not a stryker comment\nreturn value\n", new Set([1]), "source.ts"), ) }) }) @@ -641,7 +706,7 @@ describe("failure output", () => { }, ] - it("lists every advisory mutant with tests, reproduction, exclusion, and report guidance", () => { + it("lists every advisory mutant with tests, reproduction, directive policy, and report guidance", () => { const baseSha = "a".repeat(40) const headSha = "b".repeat(40) const summary = formatSummary( @@ -672,7 +737,8 @@ describe("failure output", () => { for (const mutant of blocking) assert.ok(summary.includes(mutant.mutatorName)) assert.ok(summary.includes('"left \\| right"')) assert.ok(summary.includes(`node scripts/stryker-diff.mjs ci --base ${baseSha} --head ${headSha}`)) - assert.ok(summary.includes("Stryker disable next-line ConditionalExpression:")) + assert.ok(summary.includes("Stryker suppression comments")) + assert.ok(summary.includes("are not allowed in this repository")) assert.ok(summary.includes("`reports/mutation/extension/mutation.html`")) assert.ok(summary.includes("`changed-code-mutation-report` artifact")) assert.ok(summary.includes("### Advisory findings"))