Skip to content
Merged
22 changes: 21 additions & 1 deletion .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ describe("enforce-pr-target workflow", () => {
assert.match(workflow, /synchronize/);
});

it("re-runs on issue_comment so a maintainer GUI waiver takes effect", () => {
// The GUI-screenshot gate is waived by a maintainer issue comment
// ("not touching gui"). `pull_request_target` types do not include issue
// comments, so without this trigger the waiver sits unread until a PR
// edit or push re-runs the gate.
assert.match(workflow, /^ issue_comment:/m);
assert.match(workflow, /- created/);
assert.match(workflow, /- edited/);
// The script resolves the PR number from the issue payload, which is what
// an issue_comment event delivers instead of a pull_request object.
assert.match(workflow, /context\.payload\.issue\?\.number/);
});

it("does not add review events that would break the trusted-base model", () => {
// `pull_request_review` / `pull_request_review_comment` load the workflow
// from the PR head branch (like `pull_request`), while this workflow's
Expand Down Expand Up @@ -127,7 +140,14 @@ describe("enforce-pr-target workflow", () => {
.split("- name: Checkout trusted PR-quality scripts")[1]
.split(/\n {6}- name:/)[0];
assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/);
assert.match(checkoutStep, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/);
// `pull_request_target` pins the PR's base SHA so the scripts match the
// event's base revision. An `issue_comment` event has no PR payload, so
// the ref falls back to the integration branch `dev` (the gate's only
// allowed base) — still trusted, and never the PR head.
assert.match(
checkoutStep,
/ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*'dev'\s*\}\}/,
);
// The readiness ping reads MAINTAINERS.md from the same trusted checkout.
assert.match(checkoutStep, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/);
assert.match(checkoutStep, /persist-credentials:\s*false/);
Expand Down
76 changes: 75 additions & 1 deletion .github/scripts/pr-quality-messages.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ const {
const READINESS_MARKER = "<!-- pr-quality-readiness -->";
/** Marks the bot's consolidated PR gate message. */
const GATE_MARKER = "<!-- opencodex-pr-gate -->";
/** Marks the hygiene status block inside the consolidated gate comment. */
const HYGIENE_MARKER = "<!-- pr-hygiene -->";
/** HTML comment wrapping the hygiene block so it survives gate rebuilds. */
const HYGIENE_BLOCK_START = "<!-- pr-hygiene-block:start -->";
const HYGIENE_BLOCK_END = "<!-- pr-hygiene-block:end -->";
/**
* Both delimiters must occupy a complete line. A contributor-controlled
* hygiene line (for example a changed filename) can otherwise embed delimiter
* text mid-line and corrupt the block boundary on the next rewrite.
*/
const HYGIENE_BLOCK_RE = new RegExp(
`^[ \\t]*${HYGIENE_BLOCK_START}[ \\t]*\\n([\\s\\S]*?)\\n[ \\t]*${HYGIENE_BLOCK_END}[ \\t]*$`,
"m"
);

function inlineCode(value) {
const text = String(value);
Expand Down Expand Up @@ -57,7 +71,8 @@ function buildGateCommentBody(state, opts) {
actions = [],
readiness,
checklistRequired = true,
notices = []
notices = [],
hygiene
} = opts;
const complete = readiness?.present && readiness?.complete;
const statusEmoji = status === "READY" ? "✅" : "⏳";
Expand All @@ -84,10 +99,64 @@ function buildGateCommentBody(state, opts) {
""
]
: []),
...(hygiene && hygiene.length > 0
? [
"## Hygiene",
"",
HYGIENE_BLOCK_START,
HYGIENE_MARKER,
"",
...hygiene,
"",
HYGIENE_BLOCK_END,
""
]
: []),
...notices
].filter(line => line !== null && line !== undefined);
}

/**
* The hygiene status block as stored inside the consolidated gate comment, or
* `null` when the comment has none. The gate rebuilds its body from scratch
* every run, so without this round-trip a hygiene update from the separate
* hygiene workflow would be silently dropped on the next gate write.
*/
function extractHygieneSection(body) {
if (typeof body !== "string") return null;
const match = body.match(HYGIENE_BLOCK_RE);
if (!match) return null;
return match[1]
.split("\n")
.map(line => line.trim())
.filter(line => line !== "" && line !== HYGIENE_MARKER)
.join("\n");
}

/**
* Insert (or replace) a hygiene block in a gate-comment body. Used by the
* hygiene workflow to write its status into the single consolidated comment
* instead of posting a second bot message.
*/
function withHygieneSection(body, hygieneLines) {
const base = typeof body === "string" ? body : "";
const block = [
HYGIENE_BLOCK_START,
HYGIENE_MARKER,
"",
...hygieneLines,
"",
HYGIENE_BLOCK_END
].join("\n");

if (HYGIENE_BLOCK_RE.test(base)) {
return base.replace(HYGIENE_BLOCK_RE, block);
}

// No existing block: append one at the end.
return `${base.replace(/\s+$/, "")}\n\n## Hygiene\n\n${block}\n`;
}

function descriptionFailureLines(reason) {
switch (reason) {
case "empty":
Expand Down Expand Up @@ -254,9 +323,14 @@ function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) {
module.exports = {
READINESS_MARKER,
GATE_MARKER,
HYGIENE_MARKER,
HYGIENE_BLOCK_START,
HYGIENE_BLOCK_END,
inlineCode,
readinessChecklistLines,
buildGateCommentBody,
extractHygieneSection,
withHygieneSection,
descriptionFailureLines,
buildFailureSections,
failureSummary,
Expand Down
164 changes: 164 additions & 0 deletions .github/scripts/pr-quality-messages.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@ const {
} = require("./pr-quality.cjs");
const {
GATE_MARKER,
HYGIENE_MARKER,
HYGIENE_BLOCK_START,
HYGIENE_BLOCK_END,
inlineCode,
readinessChecklistLines,
buildGateCommentBody,
extractHygieneSection,
withHygieneSection,
descriptionFailureLines,
buildFailureSections,
failureSummary,
Expand Down Expand Up @@ -254,3 +259,162 @@ describe("buildFindingsClaimNotice", () => {
assert.match(notice[1], /Resolve every open review conversation/);
});
});

describe("hygiene section round-trip", () => {
const GATE = [
GATE_MARKER,
'<!-- opencodex-pr-gate-state:{"version":1,"active":false} -->',
"",
"## ✅ READY",
"- all PR quality gates passed.",
].join("\n");

it("renders a hygiene block in the gate comment when requested", () => {
const body = buildGateCommentBody(
{ version: 1, active: false },
{
status: "READY",
statusReason: "all PR quality gates passed.",
checklistRequired: false,
hygiene: ["✅ **Deterministic PR hygiene checks passed.**"],
},
).join("\n");
assert.ok(body.includes(HYGIENE_BLOCK_START));
assert.ok(body.includes(HYGIENE_BLOCK_END));
assert.ok(body.includes(HYGIENE_MARKER));
assert.ok(body.includes("✅ **Deterministic PR hygiene checks passed.**"));
});

it("extracts the hygiene content from a gate comment", () => {
const withBlock = `${GATE}\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n✅ **Deterministic PR hygiene checks passed.**\n\n${HYGIENE_BLOCK_END}\n`;
const extracted = extractHygieneSection(withBlock);
assert.equal(extracted, "✅ **Deterministic PR hygiene checks passed.**");
assert.equal(extractHygieneSection(GATE), null);
});

it("replaces an existing hygiene block without duplicating it", () => {
const withBlock = `${GATE}\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n✅ **Deterministic PR hygiene checks passed.**\n\n${HYGIENE_BLOCK_END}\n`;
const updated = withHygieneSection(withBlock, [
"⚠️ **Deterministic hygiene checks failed.**",
"- `missing_regression_test` — Behavior changed under `src/` without a test change.",
]);
assert.ok(updated.includes("⚠️ **Deterministic hygiene checks failed.**"));
assert.ok(!updated.includes("✅ **Deterministic PR hygiene checks passed.**"));
assert.equal(updated.split(HYGIENE_BLOCK_START).length - 1, 1);
});

it("appends a hygiene block when the gate comment has none", () => {
const updated = withHygieneSection(GATE, [
"✅ **Deterministic PR hygiene checks passed.**",
]);
assert.ok(updated.includes(HYGIENE_BLOCK_START));
assert.ok(updated.includes("✅ **Deterministic PR hygiene checks passed.**"));
assert.ok(updated.includes(GATE_MARKER));
});

it("ignores delimiter text embedded inside a hygiene content line", () => {
// A contributor-controlled changed filename can contain delimiter text
// mid-line (e.g. `src/<!-- pr-hygiene-block:end -->/x.ts`). The block
// regex must anchor delimiters to complete lines so such a line neither
// ends the block early nor corrupts the next rewrite.
const malicious = [
GATE_MARKER,
'<!-- opencodex-pr-gate-state:{"version":1,"active":false} -->',
"",
"## ✅ READY",
"- all PR quality gates passed.",
"",
"## Hygiene",
"",
HYGIENE_BLOCK_START,
"<!-- pr-hygiene -->",
"",
"✅ **Deterministic PR hygiene checks passed.**",
`- Paths: \`src/${HYGIENE_BLOCK_END}/x.ts\`.`,
"",
HYGIENE_BLOCK_END,
].join("\n");

const extracted = extractHygieneSection(malicious);
assert.ok(extracted);
assert.ok(extracted.includes("✅ **Deterministic PR hygiene checks passed.**"));
assert.ok(extracted.includes("Paths"));

// Replacing must preserve the malicious line inside the block, not split
// the block at the embedded delimiter.
const updated = withHygieneSection(malicious, [
"⚠️ **Deterministic hygiene checks failed.**",
]);
assert.ok(updated.includes(HYGIENE_BLOCK_START));
assert.ok(updated.includes(HYGIENE_BLOCK_END));
assert.ok(updated.includes("⚠️ **Deterministic hygiene checks failed.**"));
assert.equal(updated.split(HYGIENE_BLOCK_START).length - 1, 1);
assert.equal(updated.split(HYGIENE_BLOCK_END).length - 1, 1);
});

it("preserves both sections across an interleaved gate rebuild and hygiene update", () => {
// The gate and hygiene workflows share one concurrency group, but the
// merge helpers must also be order-independent: whichever write lands
// second must preserve the other's section. Start with a gate comment
// carrying a hygiene block, apply a gate rebuild, then a hygiene update,
// and assert both the gate status and the hygiene status survive.
const withBlock = [
GATE_MARKER,
'<!-- opencodex-pr-gate-state:{"version":1,"active":false} -->',
"",
"## ✅ READY",
"- all PR quality gates passed.",
"",
"## Hygiene",
"",
HYGIENE_BLOCK_START,
"<!-- pr-hygiene -->",
"",
"✅ **Deterministic PR hygiene checks passed.**",
"",
HYGIENE_BLOCK_END,
].join("\n");

// Gate rebuild (the gate rewrites its own section, preserving hygiene).
const afterGate = buildGateCommentBody(
{ version: 1, active: false },
{
status: "READY",
statusReason: "all PR quality gates passed.",
checklistRequired: false,
hygiene: ["✅ **Deterministic PR hygiene checks passed.**"],
},
).join("\n");

// Hygiene update (the hygiene workflow rewrites its block, preserving gate).
const afterHygiene = withHygieneSection(afterGate, [
"✅ **Deterministic PR hygiene checks passed.**",
]);

assert.ok(afterHygiene.includes(GATE_MARKER));
assert.ok(afterHygiene.includes("## ✅ READY"));
assert.ok(afterHygiene.includes("✅ **Deterministic PR hygiene checks passed.**"));
assert.equal(afterHygiene.split(HYGIENE_BLOCK_START).length - 1, 1);
assert.equal(afterHygiene.split(HYGIENE_BLOCK_END).length - 1, 1);

// Reverse order: hygiene first, then gate rebuild — same invariant.
const afterHygieneFirst = withHygieneSection(withBlock, [
"⚠️ **Deterministic hygiene checks failed.**",
"- `missing_regression_test` — Behavior changed under `src/` without a test change.",
]);
const afterGateSecond = buildGateCommentBody(
{ version: 1, active: false },
{
status: "READY",
statusReason: "all PR quality gates passed.",
checklistRequired: false,
hygiene: ["⚠️ **Deterministic hygiene checks failed.**", "- `missing_regression_test` — Behavior changed under `src/` without a test change."],
},
).join("\n");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
assert.ok(afterGateSecond.includes(GATE_MARKER));
assert.ok(afterGateSecond.includes("## ✅ READY"));
assert.ok(afterGateSecond.includes("⚠️ **Deterministic hygiene checks failed.**"));
assert.equal(afterGateSecond.split(HYGIENE_BLOCK_START).length - 1, 1);
assert.equal(afterGateSecond.split(HYGIENE_BLOCK_END).length - 1, 1);
});
});
17 changes: 17 additions & 0 deletions .github/scripts/pr-quality-state.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,23 @@ describe("completionIsStale", () => {
);
});

it("is stale when the event delivered no head SHA at all (issue_comment rerun)", () => {
// `issue_comment` events carry no `pull_request.head.sha`. The gate passes
// an empty eventHeadSha so a completed checklist with no recorded head
// cannot be accepted as attesting the live head on a comment-triggered
// rerun — the contributor could have pushed since ticking the boxes.
assert.equal(
completionIsStale({
...base,
checklistComplete: true,
completionHeadSha: null,
eventHeadSha: "",
eventAction: "created"
}),
true,
);
});


it("is not stale for maintainers or absent checklists", () => {
assert.equal(
Expand Down
Loading
Loading