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 repository default branch — still trusted, and
// never the PR head.
assert.match(
checkoutStep,
/ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*github\.event\.repository\.default_branch\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
72 changes: 71 additions & 1 deletion .github/scripts/pr-quality-messages.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ 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 -->";

function inlineCode(value) {
const text = String(value);
Expand Down Expand Up @@ -57,7 +62,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 +90,69 @@ 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(
new RegExp(`${HYGIENE_BLOCK_START}([\\s\\S]*?)${HYGIENE_BLOCK_END}`)
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
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 (base.includes(HYGIENE_BLOCK_START) && base.includes(HYGIENE_BLOCK_END)) {
return base.replace(
new RegExp(`${HYGIENE_BLOCK_START}[\\s\\S]*?${HYGIENE_BLOCK_END}`),
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 +319,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
58 changes: 58 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,56 @@ 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));
});
});
69 changes: 64 additions & 5 deletions .github/workflows/enforce-pr-target.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ on:
- edited
- ready_for_review
- synchronize
# A maintainer issue comment ("not touching gui") waives the GUI-screenshot
# gate. `pull_request_target` types do not include issue comments, so a
# separate `issue_comment` trigger re-runs the gate the moment the waiver is
# posted. The gate is idempotent — it re-reads the live PR and updates the
# single consolidated comment — so a comment cannot race or double-mutate.
issue_comment:
types:
- created
- edited
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
Wibias marked this conversation as resolved.
Comment thread
Wibias marked this conversation as resolved.

# pull-requests:write covers title/comment/label updates.
# contents:write is required for convertPullRequestToDraft /
Expand All @@ -19,10 +28,21 @@ permissions:
pull-requests: write

concurrency:
group: enforce-pr-target-${{ github.event.pull_request.number }}
# `issue_comment` events carry the issue number, not the PR number.
group: enforce-pr-target-${{ github.event.pull_request.number || github.event.issue.number }}

jobs:
enforce-target:
# `issue_comment` fires for comments on ANY issue, PR or not, from ANY
# user. This gate is PR-only and write-capable, so a comment on a plain
# issue — or from a non-maintainer — must not start it. Only a maintainer
# comment on a PR (the GUI-waiver case) may re-run the gate.
if: >-
github.event_name != 'issue_comment' ||
(github.event.issue.pull_request != null &&
(github.event.comment.author_association == 'OWNER' ||
github.event.comment.author_association == 'COLLABORATOR' ||
github.event.comment.author_association == 'MEMBER'))
runs-on: ubuntu-latest

steps:
Expand All @@ -33,8 +53,10 @@ jobs:
# runs this workflow from the base revision, and the scripts must come
# from the same revision or a merged gate would run against the
# pre-promotion scripts on `main`. The immutable SHA pins the checkout
# to the exact base commit the event was built against.
ref: ${{ github.event.pull_request.base.sha }}
# to the exact base commit the event was built against. On an
# `issue_comment` event there is no PR payload, so the checkout falls
# back to the repository default branch — the trusted gate source.
ref: ${{ github.event.pull_request.base.sha || github.event.repository.default_branch }}
Comment thread
Wibias marked this conversation as resolved.
Outdated
persist-credentials: false
sparse-checkout: |
.github/scripts
Expand Down Expand Up @@ -81,8 +103,12 @@ jobs:
const {
GATE_MARKER,
READINESS_MARKER,
HYGIENE_MARKER,
HYGIENE_BLOCK_START,
HYGIENE_BLOCK_END,
inlineCode,
buildGateCommentBody,
extractHygieneSection,
buildFailureSections,
failureSummary,
buildStaleNotice,
Expand Down Expand Up @@ -115,7 +141,31 @@ jobs:
const MAINTAINERS_FILE = "MAINTAINERS.md";

const { owner, repo } = context.repo;
const pull_number = context.payload.pull_request.number;
// `issue_comment` events carry the PR's issue object, not a
// `pull_request` object. The issue number is the PR number either
// way, so resolve it from whichever payload the event delivered.
const pull_number =
context.payload.pull_request?.number ??
context.payload.issue?.number;
Comment thread
Wibias marked this conversation as resolved.

// Defensive re-check of the job-level guard. `issue_comment` events
// carry a `comment` object with the author's association; a comment
// on a plain issue has no `issue.pull_request`, and a comment from
// anyone but a maintainer must not re-run this write-capable gate.
if (context.eventName === "issue_comment") {
const isPrComment =
context.payload.issue?.pull_request != null;
const association = context.payload.comment?.author_association;
const isMaintainer = ["OWNER", "COLLABORATOR", "MEMBER"].includes(
association
);
if (!isPrComment || !isMaintainer) {
core.info(
"issue_comment not from a maintainer on a PR; skipping the gate."
);
return;
}
}

const { data: pr } = await github.rest.pulls.get({
owner,
Expand Down Expand Up @@ -231,7 +281,16 @@ jobs:
* readiness section or a stale intermediate checkpoint body.
*/
async function upsertGateComment(state, opts) {
const body = buildGateCommentBody(state, opts).join("\n");
let body = buildGateCommentBody(state, opts).join("\n");
// The hygiene workflow writes its status into this same comment.
// Preserve whatever it left so a gate rebuild does not drop it.
const existingHygiene = extractHygieneSection(gateComment?.body);
if (existingHygiene && !body.includes("pr-hygiene-block")) {
body = body.replace(
/\s+$/,
`\n\n## Hygiene\n\n${HYGIENE_BLOCK_START}\n${HYGIENE_MARKER}\n\n${existingHygiene}\n\n${HYGIENE_BLOCK_END}`
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (gateCommentId) {
await github.rest.issues.updateComment({
owner,
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/issue-quality-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:
- ".github/scripts/issue-quality.test.cjs"
- ".github/scripts/pr-quality.cjs"
- ".github/scripts/pr-quality.test.cjs"
- ".github/scripts/pr-quality-messages.cjs"
- ".github/scripts/pr-quality-messages.test.cjs"
- ".github/scripts/pr-labeler.cjs"
- ".github/scripts/pr-labeler.test.cjs"
- ".github/scripts/enforce-pr-target.test.cjs"
Expand Down Expand Up @@ -35,6 +37,8 @@ on:
- ".github/scripts/issue-quality.test.cjs"
- ".github/scripts/pr-quality.cjs"
- ".github/scripts/pr-quality.test.cjs"
- ".github/scripts/pr-quality-messages.cjs"
- ".github/scripts/pr-quality-messages.test.cjs"
- ".github/scripts/pr-labeler.cjs"
- ".github/scripts/pr-labeler.test.cjs"
- ".github/scripts/enforce-pr-target.test.cjs"
Expand Down
Loading
Loading