ci: PR template and stricter danger.js review checks

.github/pull_request_template.md

@@ -0,0 +1,25 @@
+<!-- Fill this in yourself, or at least have your favorite AI agent draft it from the diff. -->
+
+_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._

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

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
 

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 = [
+            "<!-- Fill this in yourself, or have your favorite AI agent draft it from the diff. -->",
+            "",
+            "## 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:
+                "<!-- This is a long template comment that should not count toward the body length. -->\n" +
+                "<!-- More scaffolding here that adds many characters but conveys nothing useful. -->\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 = {

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(/<!--[\s\S]*?-->/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),
     ];