From 6d47b9e936666bc8496722f8a509bc1d3b076e62 Mon Sep 17 00:00:00 2001 From: Alan de Freitas Date: Wed, 13 May 2026 15:46:26 -0500 Subject: [PATCH 1/3] chore: gitignore entries for Python coverage artifacts --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index f8d6b546ce..032bec24b3 100644 --- a/.gitignore +++ b/.gitignore @@ -31,4 +31,8 @@ # Ignore hidden OS files under golden fixtures /test-files/golden-tests/**/.* /.code +# Python (bootstrap.py at repo root, util/bootstrap/ utilities) __pycache__/ +.coverage +.coverage.* +htmlcov/ From 4d8ea72c5bce44ebd17a08f1c9084eddcc85ae6a Mon Sep 17 00:00:00 2001 From: Alan de Freitas Date: Wed, 13 May 2026 16:33:30 -0500 Subject: [PATCH 2/3] ci: PR-aggregate size warning and log-scaled description floor Adds an aggregate source-churn warning (5000 lines) so large PRs are flagged even when individual commits stay under the per-commit limit, and replaces the fixed 40-character description floor with a log-scaled threshold (~80 * log2(churn)) so descriptions are expected to grow with the size of the change. Template scaffolding (HTML comments, Markdown headers, standalone italic placeholders, and `- **Label**: _placeholder_` bullets) is stripped from the body before measurement so an unfilled template cannot inflate the length count or mask the testing-mention check via the `## Testing` header. --- util/danger/README.md | 3 + util/danger/logic.test.ts | 170 ++++++++++++++++++++++++++++++++++++++ util/danger/logic.ts | 71 +++++++++++++++- 3 files changed, 242 insertions(+), 2 deletions(-) diff --git a/util/danger/README.md b/util/danger/README.md index af27537cb1..17f1d9a2e4 100644 --- a/util/danger/README.md +++ b/util/danger/README.md @@ -33,7 +33,10 @@ npm --prefix util/danger run danger:ci # run Danger in CI mode (requires Git - Scopes reflect the MrDocs tree: `source`, `tests`, `golden-tests`, `docs`, `ci`, `build`, `tooling`, `third-party`, `other`. - Conventional commit types allowed: `feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert`. - Non-test commit size notice triggers at 2000 lines of churn (tests and golden fixtures ignored) and is informational. +- Aggregate PR source churn triggers a warning above 5000 lines (much more generous than the per-commit limit, since well-sliced commits can still amount to a large overall change). +- PR description length is checked against a log-scaled floor (`80 * log2(churn)` characters). Tiny diffs need ~80 chars; ~1k-line changes need ~800; ~30k-line changes need ~1200. HTML comments in the body (e.g. PR template scaffolding) are stripped before measurement. - Feature PRs (`feat:` commits, `feat` PR title, or `feature` label) without any `docs/` change get a light warning. Opt out with the `no-docs-needed` label or a `[skip danger docs]` marker in the PR body. +- The PR template (`.github/pull_request_template.md`) mirrors these rules; following it tends to satisfy all of them automatically. ## Updating rules diff --git a/util/danger/logic.test.ts b/util/danger/logic.test.ts index fedebde6d1..0bb09d2247 100644 --- a/util/danger/logic.test.ts +++ b/util/danger/logic.test.ts @@ -9,7 +9,10 @@ // import { describe, expect, it } from "vitest"; import { + aggregateSizeWarnings, commitSizeInfos, + evaluateDanger, + expectedBodyLength, parseCommitSummary, basicChecks, summarizeScopes, @@ -72,6 +75,52 @@ describe("summarizeScopes", () => { }); }); +describe("aggregateSizeWarnings", () => { + // Stays quiet when source churn is under the aggregate threshold. + it("does not warn under the aggregate threshold", () => { + const summary = summarizeScopes([ + { filename: "src/lib/file.cpp", additions: 1000, deletions: 500 }, + ]); + expect(aggregateSizeWarnings(summary)).toEqual([]); + }); + + // Fires once aggregate source churn crosses the limit, even with well-sliced commits. + it("warns when aggregate source churn exceeds the threshold", () => { + const summary = summarizeScopes([ + { filename: "src/lib/a.cpp", additions: 3000, deletions: 0 }, + { filename: "src/lib/b.cpp", additions: 2500, deletions: 0 }, + ]); + const warnings = aggregateSizeWarnings(summary); + expect(warnings.length).toBe(1); + expect(warnings[0]).toContain("5500"); + }); + + // Ignores test, golden-test, and docs churn — only source counts. + it("ignores non-source churn", () => { + const summary = summarizeScopes([ + { filename: "test-files/golden-tests/big.xml", additions: 20000, deletions: 0 }, + { filename: "docs/big.adoc", additions: 5000, deletions: 0 }, + { filename: "src/lib/small.cpp", additions: 10, deletions: 0 }, + ]); + expect(aggregateSizeWarnings(summary)).toEqual([]); + }); +}); + +describe("expectedBodyLength", () => { + // Bottoms out at 80 chars (log2(2) * 80) for tiny or zero-churn diffs. + it("bottoms out at 80 chars for tiny changes", () => { + expect(expectedBodyLength(0)).toBe(80); + expect(expectedBodyLength(1)).toBe(80); + expect(expectedBodyLength(2)).toBe(80); + }); + + // Grows roughly logarithmically with churn — 30k lines should still be only a few hundred chars. + it("grows logarithmically with churn", () => { + expect(expectedBodyLength(30000)).toBeGreaterThan(expectedBodyLength(1000)); + expect(expectedBodyLength(30000)).toBeLessThan(expectedBodyLength(1000) * 3); + }); +}); + describe("commitSizeInfos", () => { // Confirms that large non-test churn emits an informational note while ignoring test fixtures. it("flags large non-test commits", () => { @@ -184,6 +233,127 @@ describe("starterChecks", () => { expect(warnings.some((message) => message.includes("does not update any documentation"))).toBe(false); }); + // Warns when the description is too short relative to the size of the change. + it("warns when description is short relative to churn", () => { + const inputs: DangerInputs = { + files: [], + commits: [], + prBody: + "Big refactor across the codebase. Tested locally with the existing suite; no behavior change expected.", + prTitle: "refactor: massive", + labels: [], + }; + const summary = summarizeScopes([ + { filename: "src/lib/big.cpp", additions: 5000, deletions: 1000 }, + ]); + const parsed = validateCommits([{ sha: "sx", message: "refactor: massive" }]).parsed; + const warnings = basicChecks(inputs, summary, parsed); + expect(warnings.some((m) => m.includes("relative to the size of this change"))).toBe(true); + expect(warnings.some((m) => m.includes("PR description looks empty"))).toBe(false); + }); + + // An unfilled PR template (italic placeholders + headers, no real content) must still trip + // the empty-description warning and must not satisfy the testing-mention check via the + // "## Testing" header. + it("treats an unfilled PR template as empty after cleaning", () => { + const unfilledTemplate = [ + "", + "", + "## Summary", + "", + "_What this PR does and why. For large or non-trivial changes, link a design doc or issue, or explain the design inline._", + "", + "## Changes", + "", + "_Replace the lines that apply, delete the rest._", + "", + "- **Source**: _Implementation changes._", + "- **Tests**: _New or updated unit tests and fixtures._", + "- **Breaking changes**: _Anything downstream users need to know._", + "", + "## Testing", + "", + "_How this change stays tested going forward — the tests in this PR that cover the new behavior, plus any CI workflow changes needed so that coverage runs on every future build._", + "", + "## Documentation", + "", + "_What was updated in the documentation or why documentation is not needed._", + ].join("\n"); + const inputs: DangerInputs = { + files: [], + commits: [], + prBody: unfilledTemplate, + prTitle: "feat: something", + labels: [], + }; + const summary = summarizeScopes([ + { filename: "src/lib/file.cpp", additions: 100, deletions: 0 }, + ]); + const parsed = validateCommits([{ sha: "swt", message: "feat: something" }]).parsed; + const warnings = basicChecks(inputs, summary, parsed); + expect(warnings.some((m) => m.includes("PR description looks empty"))).toBe(true); + // The "## Testing" header must not satisfy the test-mention check on its own — + // the source-without-tests warning should still fire. + expect(warnings.some((m) => m.includes("Source changed"))).toBe(true); + }); + + // Strips HTML comments before measuring length so PR-template scaffolding cannot game the check. + it("ignores HTML comments when measuring description length", () => { + const inputs: DangerInputs = { + files: [], + commits: [], + prBody: + "\n" + + "\n" + + "Fix.", + prTitle: "fix: tiny", + labels: [], + }; + const summary = summarizeScopes([ + { filename: "src/lib/tiny.cpp", additions: 1, deletions: 1 }, + ]); + const parsed = validateCommits([{ sha: "sy", message: "fix: tiny" }]).parsed; + const warnings = basicChecks(inputs, summary, parsed); + expect(warnings.some((m) => m.includes("PR description looks empty"))).toBe(true); + }); + + // Stays quiet when description length is well-matched to a small change. + it("does not warn on short body for tiny diffs", () => { + const inputs: DangerInputs = { + files: [], + commits: [], + prBody: "Fix a typo in a code comment. Verified locally by running the existing unit test suite.", + prTitle: "fix: typo", + labels: [], + }; + const summary = summarizeScopes([ + { filename: "src/lib/typo.cpp", additions: 1, deletions: 1 }, + ]); + const parsed = validateCommits([{ sha: "sz", message: "fix: typo" }]).parsed; + const warnings = basicChecks(inputs, summary, parsed); + expect(warnings.some((m) => m.includes("PR description looks empty"))).toBe(false); + expect(warnings.some((m) => m.includes("relative to the size of this change"))).toBe(false); + }); + + // End-to-end: PR #1178-shaped change surfaces both the aggregate-size and short-description warnings. + it("flags a large PR with a terse description through evaluateDanger", () => { + const result = evaluateDanger({ + files: [ + { filename: "src/lib/a.cpp", additions: 3000, deletions: 0 }, + { filename: "src/lib/b.cpp", additions: 3000, deletions: 0 }, + { filename: "test-files/golden-tests/x.xml", additions: 10000, deletions: 0 }, + { filename: "docs/option.adoc", additions: 5, deletions: 0 }, + ], + commits: [{ sha: "ab", message: "feat: large change" }], + prBody: + "Adds a new schema-generation option to mrdocs. Tested locally against the golden test suite and verified the generated schemas render correctly.", + prTitle: "feat: large change", + labels: [], + }); + expect(result.warnings.some((m) => m.includes("source lines"))).toBe(true); + expect(result.warnings.some((m) => m.includes("relative to the size of this change"))).toBe(true); + }); + // Stays quiet for non-feature commits even without docs. it("does not warn for fix commits without docs", () => { const inputs: DangerInputs = { diff --git a/util/danger/logic.ts b/util/danger/logic.ts index 4b79445768..aecde8d88e 100644 --- a/util/danger/logic.ts +++ b/util/danger/logic.ts @@ -145,6 +145,9 @@ const skipTestMarkers = ["[skip danger tests]", "[danger skip tests]"]; const skipDocsLabels = new Set(["no-docs-needed", "skip-docs", "docs-not-required"]); const skipDocsMarkers = ["[skip danger docs]", "[danger skip docs]"]; const nonTestCommitLimit = 2000; +const aggregateSourceLimit = 5000; +const minBodyChars = 40; +const bodyCharsPerLog2Churn = 80; /** * Format churn as a + / - pair with explicit signs. @@ -502,6 +505,61 @@ export function commitSizeInfos(commits: CommitInfo[]): string[] { return messages; } +/** + * Warn when the aggregate source-scope churn across the whole PR is large. + * + * Complementary to {@link commitSizeInfos}: that rule fires on a single oversized commit, + * this one fires when the total source delta — however well-sliced — crosses a generous + * threshold and warrants explicit reviewer guidance. + * + * @param scopes summarized scope totals for the PR. + * @returns warnings, or empty if the PR stays under the aggregate limit. + */ +export function aggregateSizeWarnings(scopes: ScopeReport): string[] { + const sourceChurn = scopes.totals.source.additions + scopes.totals.source.deletions; + if (sourceChurn <= aggregateSourceLimit) { + return []; + } + return [ + `This PR touches **${sourceChurn}** source lines across ${scopes.totals.source.files} files (threshold ${aggregateSourceLimit}). ` + + "Large changes are harder to review even when well-sliced — consider splitting, or expand the description with rationale, design notes, and reviewer guidance.", + ]; +} + +/** + * Strip PR-template scaffolding (HTML comments, Markdown headers, and italic placeholders) + * so an unfilled template does not inflate body-length checks or satisfy mention checks. + * + * The PR template uses visible italic placeholder text (e.g. `_What this PR does and why._`) + * that contributors are expected to overwrite. Without stripping, an unfilled template + * contributes ~1 KB of "content" and contains the word "Testing" via the section header, + * which would mask both the length floor and the missing-testing-note check. + */ +function cleanBody(body: string): string { + return body + // HTML comments. + .replace(//g, "") + // Markdown ATX headers — scaffolding, not content. + .replace(/^[ \t]*#+\s+.*$/gm, "") + // Bullet placeholders of the form: `- **Label**: _placeholder text._` + .replace(/^[ \t]*-\s+\*\*[^*\n]+\*\*:\s+_[^_\n]+_[ \t]*$/gm, "") + // Standalone italic placeholder lines. + .replace(/^[ \t]*_[^_\n]+_[ \t]*$/gm, "") + .trim(); +} + +/** + * Suggested body length given the size of the change. + * + * Roughly logarithmic: tiny diffs → ~80 chars; 1k lines → ~800; 30k lines → ~1200. + * The {@link minBodyChars} floor is enforced separately by callers so an empty body + * is reported with a clearer message than a short-but-non-empty one. + */ +export function expectedBodyLength(totalChurn: number): number { + const churn = Math.max(totalChurn, 2); + return Math.floor(bodyCharsPerLog2Churn * Math.log2(churn)); +} + /** * Check for explicit signals to skip source-vs-test warnings. * @@ -549,10 +607,18 @@ export function basicChecks(input: DangerInputs, scopes: ScopeReport, parsedComm /refactor/i.test(input.prTitle || "") || input.labels.some((label) => /refactor/i.test(label)); - const cleanedBody = (input.prBody || "").trim(); - if (cleanedBody.length < 40) { + const cleanedBody = cleanBody(input.prBody || ""); + const totalChurn = scopes.overall.additions + scopes.overall.deletions; + const expectedBodyChars = expectedBodyLength(totalChurn); + if (cleanedBody.length < minBodyChars) { // === PR description completeness warnings === warnings.push("PR description looks empty. Please add a short rationale and testing notes."); + } else if (cleanedBody.length < expectedBodyChars) { + // === PR description too short for the size of the change === + warnings.push( + `PR description is short (${cleanedBody.length} chars) relative to the size of this change (~${totalChurn} lines of churn). ` + + `Aim for around **${expectedBodyChars}** characters — expand on rationale, testing, and reviewer guidance.`, + ); } else if ( scopes.totals.source.files > 0 && scopes.totals["golden-tests"].files === 0 && @@ -625,6 +691,7 @@ export function evaluateDanger(input: DangerInputs): DangerResult { const warnings = [ ...commitValidation.warnings, + ...aggregateSizeWarnings(summary), ...basicChecks(input, summary, commitValidation.parsed), ]; From e77f9089a156669fd15dd424a273935f0d531550 Mon Sep 17 00:00:00 2001 From: Alan de Freitas Date: Wed, 13 May 2026 16:33:34 -0500 Subject: [PATCH 3/3] ci: pull request template Prompts contributors to describe the change, enumerate per-area impacts, document testing (including any workflow updates), and note documentation changes. Placeholders are visible italic text so any section a contributor forgets to replace stands out in the rendered PR. --- .github/pull_request_template.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000000..70d767a74d --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,25 @@ + + +_What this PR does and why. For large or non-trivial changes, link a design doc or issue, or explain the design inline — the larger the change, the more context belongs here._ + +## Changes + +_Replace the lines that apply, delete the rest._ + +- **Source**: _Implementation changes._ +- **Tests**: _New or updated unit tests and fixtures._ +- **Golden tests**: _Intentional output changes or regenerations._ +- **Build**: _Build-system, packaging, or schema changes._ +- **CI**: _Workflows or automation._ +- **Toolchain**: _Bootstrap or environment tooling._ +- **Tooling**: _Developer tools and helper scripts._ +- **Third-party**: _Vendored dependencies or patches._ +- **Breaking changes**: _Anything downstream users need to know._ + +## Testing + +_How this change stays tested going forward — the tests in this PR that cover the new behavior, plus any CI workflow changes needed so that coverage runs on every future build._ + +## Documentation + +_What was updated in the documentation or why documentation is not needed._