Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
## Verification

- List the commands or checks you ran.
- If this PR changes the GUI, include a screenshot of the UI change in the description.

## Checklist

Expand Down
82 changes: 82 additions & 0 deletions .github/scripts/pr-quality.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,27 @@ const UNSTRUCTURED_MIN_BLOCKS = 2;
const PR_TEMPLATE_BOILERPLATE_LINES = new Set([
"explain the user-visible or maintainer-facing change.",
"list the commands or checks you ran.",
"if this pr changes the gui, include a screenshot of the ui change in the description.",
"scope stays focused and avoids unrelated cleanup.",
"docs or release notes were updated when needed.",
"security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.",
]);

/** Case-insensitive whole-word match for the GUI surface (repo convention: `gui/`). */
const GUI_CUE_RE = /\bgui\b/i;
/** HTML comments, which GitHub never renders. */
const HTML_COMMENT_RE = /<!--[\s\S]*?-->/g;
/** Fenced code blocks (``` or ~~~) whose content GitHub does not render. */
const FENCED_CODE_RE = /(?:^|\n)[ \t]*(`{3,}|~{3,})[^\n]*\n[\s\S]*?^[ \t]*\1[ \t]*(?=\n|$)/gm;
/** Embedded markdown image (`![alt](url)`), as GitHub renders for dropped images. */
const MARKDOWN_IMAGE_RE = /!\[[^\]]*\]\([^)]+\)/;
Comment thread
Wibias marked this conversation as resolved.
/** Reference-style markdown image (`![alt][id]`, collapsed `![alt][]`). */
const MARKDOWN_REFERENCE_IMAGE_RE = /!\[([^\]]*)\]\[([^\]]*)\]/g;
/** Link definitions (`[id]: url`) that reference-style images depend on. */
const MARKDOWN_REFERENCE_DEF_RE = /^\s*\[([^\]]+)\]:\s*\S+/gm;
/** Embedded HTML image with a renderable `src` (`<img ... src="...">`). */
const HTML_IMAGE_RE = /<img\b[^>]*\bsrc\s*=\s*(?:"[^"]+"|'[^']+'|[^\s>"']+)[^>]*>/i;

function isWrongAncestry({
behindMain,
behindBase,
Expand Down Expand Up @@ -124,9 +140,61 @@ function assessPrDescription(body) {
return { ok: false, reason: "thin" };
}

/**
* True when the PR title or description names the GUI surface as a whole word.
* The description is template-stripped first so the template's own screenshot
* instruction cannot arm the gate on its own.
*/
function hasGuiCue(title, body) {
return (
(typeof title === "string" && GUI_CUE_RE.test(title)) ||
(typeof body === "string" && GUI_CUE_RE.test(body))
);
}

/**
* Drop the regions GitHub does not render as Markdown: HTML comments and
* fenced code blocks. Image syntax there is literal text, not evidence.
*/
function stripNonRenderedRegions(body) {
return body.replace(HTML_COMMENT_RE, "").replace(FENCED_CODE_RE, "");
}

/**
* True when the description contains a reference-style image (`![alt][id]` or
* collapsed `![alt][]`) backed by a matching `[id]: url` definition — GitHub
* renders only those reference images, so a bare token is not evidence.
*/
function hasRenderableReferenceImage(visible) {
const definitions = new Set();
for (const match of visible.matchAll(MARKDOWN_REFERENCE_DEF_RE)) {
definitions.add(match[1].trim().toLowerCase());
}
if (definitions.size === 0) return false;
for (const match of visible.matchAll(MARKDOWN_REFERENCE_IMAGE_RE)) {
const id = (match[2] || match[1]).trim().toLowerCase();
if (id && definitions.has(id)) return true;
}
return false;
}

/**
* True when the rendered description embeds a screenshot image: an inline
* markdown image, a reference-style image with a definition, or an `<img>` tag
* with a non-empty `src`. A plain link to an image is not visual evidence.
*/
function hasScreenshotEvidence(body) {
if (typeof body !== "string") return false;
const visible = stripNonRenderedRegions(body);
if (MARKDOWN_IMAGE_RE.test(visible)) return true;
if (HTML_IMAGE_RE.test(visible)) return true;
return hasRenderableReferenceImage(visible);
}

function collectPrQualityFailures({
baseRef,
allowedBases,
title = "",
body,
behindMain,
behindBase,
Expand Down Expand Up @@ -162,6 +230,18 @@ function collectPrQualityFailures({
if (!desc.ok) {
failures.push({ code: "bad_description", reason: desc.reason });
}

// GUI-cued PRs must prove the UI change visually. The template's own
// screenshot instruction is boilerplate, so it cannot trigger this gate.
if (
hasGuiCue(
title,
typeof body === "string" ? stripPrTemplateBoilerplate(body) : "",
) &&
!hasScreenshotEvidence(body)
) {
failures.push({ code: "missing_ui_screenshot" });
}
return failures;
}

Expand All @@ -171,6 +251,8 @@ module.exports = {
isWrongAncestry,
authorHasPushPermission,
assessPrDescription,
hasGuiCue,
hasScreenshotEvidence,
collectPrQualityFailures,
hasEscapedNewlines,
stripPrTemplateBoilerplate,
Expand Down
233 changes: 233 additions & 0 deletions .github/scripts/pr-quality.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const {
isWrongAncestry,
authorHasPushPermission,
assessPrDescription,
hasGuiCue,
hasScreenshotEvidence,
collectPrQualityFailures,
} = require("./pr-quality.cjs");

Expand Down Expand Up @@ -81,6 +83,7 @@ describe("assessPrDescription", () => {
"## Verification",
"",
"- List the commands or checks you ran.",
"- If this PR changes the GUI, include a screenshot of the UI change in the description.",
"",
"## Checklist",
"",
Expand Down Expand Up @@ -114,9 +117,115 @@ describe("assessPrDescription", () => {
});
});

describe("hasGuiCue", () => {
it("matches gui as a whole word, case-insensitively, in title or body", () => {
assert.equal(hasGuiCue("Fix GUI spacing", ""), true);
assert.equal(hasGuiCue("fix gui spacing", ""), true);
assert.equal(hasGuiCue("", "This changes gui/src/App.tsx"), true);
assert.equal(
hasGuiCue("", "## Summary\nGUI button is misaligned in the dashboard"),
true,
);
});

it("does not match gui inside other words", () => {
assert.equal(hasGuiCue("Add contributor guidance", ""), false);
assert.equal(hasGuiCue("", "Fix the guild invitation bug"), false);
assert.equal(hasGuiCue("", "Remove guillotine dead code"), false);
});

it("does not match missing or non-string inputs", () => {
assert.equal(hasGuiCue(undefined, undefined), false);
assert.equal(hasGuiCue(null, null), false);
});
});

describe("hasScreenshotEvidence", () => {
it("accepts embedded markdown images", () => {
assert.equal(
hasScreenshotEvidence("![after](https://example.com/after.png)"),
true,
);
assert.equal(
hasScreenshotEvidence(
"Before:\n\n![Screenshot 2026-08-03 at 14.22](https://user-images.githubusercontent.com/1/2.png)",
),
true,
);
});

it("accepts HTML img tags", () => {
assert.equal(
hasScreenshotEvidence('<img src="https://example.com/ui.png" width="600">'),
true,
);
});

it("accepts reference-style images with a matching definition", () => {
assert.equal(
hasScreenshotEvidence(
"After:\n\n![after][shot]\n\n[shot]: https://example.com/after.png",
),
true,
);
assert.equal(
hasScreenshotEvidence(
"![after][]\n\n[after]: https://example.com/after.png",
),
true,
);
});

it("rejects image syntax inside fenced code and HTML comments", () => {
assert.equal(
hasScreenshotEvidence('```\n![after](https://example.com/after.png)\n```'),
false,
);
assert.equal(
hasScreenshotEvidence('```\n<img src="https://example.com/ui.png">\n```'),
false,
);
assert.equal(
hasScreenshotEvidence("<!-- ![after](https://example.com/after.png) -->"),
false,
);
assert.equal(
hasScreenshotEvidence('<!-- <img src="https://example.com/ui.png"> -->'),
false,
);
});

it("rejects img tags without a renderable src and references without a definition", () => {
assert.equal(hasScreenshotEvidence("<img>"), false);
assert.equal(hasScreenshotEvidence('<img src="">'), false);
assert.equal(
hasScreenshotEvidence("![after][shot]"),
false,
);
});

it("rejects plain links, bare image URLs, and text-only bodies", () => {
assert.equal(
hasScreenshotEvidence("[Screenshot](https://example.com/ui.png)"),
false,
);
assert.equal(hasScreenshotEvidence("https://example.com/ui.png"), false);
assert.equal(hasScreenshotEvidence("No screenshot here."), false);
assert.equal(hasScreenshotEvidence(undefined), false);
});
});

describe("collectPrQualityFailures", () => {
const allowed = ["dev"];

const richBody = [
"## Summary",
"This change fixes the provider list spacing in the dashboard.",
"",
"## Test plan",
"- Ran bun test tests/ci-workflows.test.ts",
].join("\n");

it("reports wrong_base without requiring ancestry inputs", () => {
const failures = collectPrQualityFailures({
baseRef: "main",
Expand Down Expand Up @@ -277,4 +386,128 @@ describe("collectPrQualityFailures", () => {
});
assert.ok(failures.some((f) => f.code === "wrong_base"));
});

it("flags a gui title without a screenshot", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "GUI: fix provider list spacing",
body: richBody,
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(failures.some((f) => f.code === "missing_ui_screenshot"));
});

it("flags a gui mention in the body without a screenshot", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "Fix dashboard spacing",
body: [
"## Summary",
"This change adjusts gui/ spacing tokens used by the dashboard.",
"",
"## Test plan",
"- Ran bun test tests/ci-workflows.test.ts",
].join("\n"),
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(failures.some((f) => f.code === "missing_ui_screenshot"));
});

it("accepts a gui title when a screenshot image is embedded", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "GUI: fix provider list spacing",
body: [
"## Summary",
"This change fixes the provider list spacing in the dashboard.",
"",
"![after](https://example.com/after.png)",
"",
"## Test plan",
"- Ran bun test tests/ci-workflows.test.ts",
].join("\n"),
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(!failures.some((f) => f.code === "missing_ui_screenshot"));
});

it("accepts a gui title when the screenshot uses reference-style markdown", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "GUI: fix provider list spacing",
body: [
"## Summary",
"This change fixes the provider list spacing in the dashboard.",
"",
"![after][shot]",
"",
"[shot]: https://example.com/after.png",
"",
"## Test plan",
"- Ran bun test tests/ci-workflows.test.ts",
].join("\n"),
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(!failures.some((f) => f.code === "missing_ui_screenshot"));
});

it("still flags a gui title when image syntax is only inside a code fence", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "GUI: fix provider list spacing",
body: [
"## Summary",
"This change fixes the provider list spacing in the dashboard.",
"",
"```",
"![after](https://example.com/after.png)",
"```",
"",
"## Test plan",
"- Ran bun test tests/ci-workflows.test.ts",
].join("\n"),
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(failures.some((f) => f.code === "missing_ui_screenshot"));
});

it("ignores the template's own gui/screenshot instruction", () => {
const failures = collectPrQualityFailures({
baseRef: "dev",
allowedBases: allowed,
title: "Add a thing",
body: [
"## Summary",
"This change touches the proxy only; no UI surface changed.",
"",
"## Verification",
"- List the commands or checks you ran.",
"- If this PR changes the GUI, include a screenshot of the UI change in the description.",
"",
"## Checklist",
"- [ ] Scope stays focused and avoids unrelated cleanup.",
"- [ ] Docs or release notes were updated when needed.",
"- [ ] Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.",
].join("\n"),
behindMain: 0,
behindBase: 0,
authorPermission: "read",
});
assert.ok(!failures.some((f) => f.code === "missing_ui_screenshot"));
});
});
Loading
Loading