diff --git a/.github/scripts/issue-quality.cjs b/.github/scripts/issue-quality.cjs index 2caa8a240..2f7264cba 100644 --- a/.github/scripts/issue-quality.cjs +++ b/.github/scripts/issue-quality.cjs @@ -6,13 +6,56 @@ // --------------------------------------------------------------------------- /** - * Strip HTML comments, "No response" placeholders, and trim whitespace. + * True when the entire meaningful value is a placeholder-only token. + * Supports harmless Markdown emphasis/code markers and trailing punctuation. + * Sentences that merely contain a placeholder phrase are not matches. + */ +const PLACEHOLDER_ONLY_RE = + /^[\s_*~`]*(?:no\s+response|n\/?a|not\s+applicable|not\s+available|none|todo|tbd)[\s_*~`]*[.!?]*$/i; + +/** + * If `text` is exactly one enclosing fenced code block (``` or ~~~), return the + * inner body; otherwise null. Real multi-statement fences are left alone by + * the placeholder matcher after unwrap. + */ +function unwrapSingleEnclosingFence(text) { + const trimmed = text.trim(); + const match = trimmed.match(/^(```|~~~)[^\n]*\r?\n([\s\S]*?)\r?\n\1[ \t]*$/); + if (!match) return null; + return match[2]; +} + +function isPlaceholderOnlyValue(raw) { + if (typeof raw !== "string") return false; + let value = raw.replace(//g, "").trim(); + if (!value) return false; + + // A lone fenced block whose entire body is a placeholder is still placeholder + // text (e.g. ```text\nN/A\n```), not a real example. + const unwrapped = unwrapSingleEnclosingFence(value); + if (unwrapped !== null) { + value = unwrapped.trim(); + if (!value) return false; + } + + return PLACEHOLDER_ONLY_RE.test(value); +} + +/** + * Strip HTML comments, placeholder-only values, and trim whitespace. */ function clean(raw) { if (typeof raw !== "string") return ""; let s = raw.replace(//g, ""); - // Treat GitHub's "No response" placeholder as empty. - s = s.replace(/^[\s_*]*No response[\s_*]*$/gim, ""); + // Whole-value placeholders first (including a single enclosing fence), so + // line-by-line stripping cannot leave bare fence markers behind. + if (isPlaceholderOnlyValue(s)) return ""; + // Treat placeholder-only lines (GitHub "No response", N/A, etc.) as empty. + s = s + .split("\n") + .map((line) => (isPlaceholderOnlyValue(line) ? "" : line)) + .join("\n"); + if (isPlaceholderOnlyValue(s)) return ""; return s.trim(); } @@ -329,29 +372,52 @@ function allRepeatTitle(sections, title) { } function isPlaceholder(text) { + return isPlaceholderOnlyValue(text); +} + +const CJK_RE = + /[\p{Script=Han}\p{Script=Hiragana}\p{Script=Katakana}\p{Script=Hangul}]/gu; + +function countWords(text) { + const c = clean(text); + if (!c) return 0; + + // Count each CJK character as one unit, and non-CJK scripts as Unicode + // word tokens. Mixing one CJK glyph into a Latin/Cyrillic word must not + // inflate the count to letter-length. + const cjkChars = c.match(CJK_RE) || []; + const nonCjkText = c.replace(CJK_RE, " "); + const nonCjkTokens = nonCjkText.match(/[\p{L}\p{N}']+/gu) || []; + + return cjkChars.length + nonCjkTokens.length; +} + +function hasConcreteDetail(text) { const c = clean(text); - if (!c) return true; - const lower = c.toLowerCase(); + if (!c) return false; return ( - lower === "no response" || - lower === "n/a" || - lower === "none" || - lower === "todo" || - lower === "tbd" || - /^_no response_$/i.test(c) + /\d/.test(c) || + /[`{}\[\]<>/\\]/.test(c) || + /\b(ocx|config|api|cli|dashboard|provider|proxy|route|endpoint|workflow|command)\b/i.test(c) ); } +function isTooTerseFeatureSection(text) { + if (isEmpty(text) || isPlaceholder(text)) return false; + const words = countWords(text); + if (words >= 8) return false; + if (words >= 6 && hasConcreteDetail(text)) return false; + return true; +} + /** - * Check if raw section text is a GitHub "No response" placeholder variant - * without stripping it first. Used to distinguish intentionally blank optional - * fields from actively cleared required fields. + * Check if raw section text is a placeholder-only variant without relying on + * clean() first. Used to distinguish intentionally blank optional fields + * (legacy "No response" / N/A) from actively cleared required fields. */ function isRawPlaceholder(raw) { if (raw === null) return false; - const trimmed = raw.trim(); - if (!trimmed) return false; - return /^[\s_*]*no response[\s_*]*$/i.test(trimmed); + return isPlaceholderOnlyValue(raw); } /** @@ -386,7 +452,12 @@ function validateIssue(issue) { // On the legacy / translated forms these sections may be absent (null). if (blocker !== null && isEmpty(blocker)) emptyCore.push("current limitation"); if (isEmpty(behaviour)) emptyCore.push("expected behaviour"); - if (example !== null && isEmpty(example)) emptyCore.push("example usage"); + if (example !== null && isPlaceholder(example)) { + reasons.push("Example usage or interface contains placeholder text instead of a concrete example."); + guidance.push("Add a real CLI command, config snippet, API exchange, or before/after workflow example."); + } else if (example !== null && isEmpty(example)) { + emptyCore.push("example usage"); + } const mappedHeadingPresent = goal !== null || blocker !== null || behaviour !== null || example !== null; @@ -421,6 +492,15 @@ function validateIssue(issue) { guidance.push("Replace placeholder text with your actual proposal."); } } + + const terseSections = []; + if (goal !== null && isTooTerseFeatureSection(goal)) terseSections.push("goal / problem"); + if (blocker !== null && isTooTerseFeatureSection(blocker)) terseSections.push("current limitation"); + if (behaviour !== null && isTooTerseFeatureSection(behaviour)) terseSections.push("expected behaviour"); + if (terseSections.length > 0) { + reasons.push(`Required sections are too vague to act on: ${terseSections.join(", ")}.`); + guidance.push("Describe the workflow, limitation, and expected behaviour with enough detail for someone to implement or evaluate the request."); + } } if (kind === "bug") { @@ -587,6 +667,47 @@ function shouldReopen(botState, issue, maintainerOverride) { return true; } +/** + * workflow_dispatch accepts a bare issue number, but GitHub reuses the same + * number namespace for issues and pull requests. Reject PR targets before any + * validation or mutation runs. + * + * @param {{ pull_request?: unknown }} issue + * @param {number|string} issueNumber + * @param {string} eventName + * @returns {string|null} + */ +function rejectsWorkflowDispatchPullRequest(issue, issueNumber, eventName) { + if (eventName !== "workflow_dispatch") return null; + if (!issue?.pull_request) return null; + return `#${issueNumber} is a pull request. This workflow only accepts issue numbers.`; +} + +/** + * workflow_dispatch can be started from a selected branch. Reject runs whose + * selected ref is not the repository default branch so untrusted branch code + * cannot drive issue mutations with issues:write. + * + * @param {string} eventName + * @param {string|null|undefined} ref + * @param {string|null|undefined} defaultBranch + * @returns {string|null} + */ +function rejectsWorkflowDispatchNonDefaultBranch(eventName, ref, defaultBranch) { + if (eventName !== "workflow_dispatch") return null; + if (!defaultBranch || typeof defaultBranch !== "string") { + return "workflow_dispatch requires repository.default_branch to be available."; + } + const expected = `refs/heads/${defaultBranch}`; + if (ref !== expected) { + return ( + `workflow_dispatch must run from the default branch (${defaultBranch}); ` + + `selected ref was ${ref || "(empty)"}.` + ); + } + return null; +} + // --------------------------------------------------------------------------- // Exports // --------------------------------------------------------------------------- @@ -601,8 +722,14 @@ module.exports = { validateIssue, shouldReopen, shouldEnforceClosure, + isPlaceholderOnlyValue, + isPlaceholder, isRawPlaceholder, + countWords, + hasConcreteDetail, labelForKind, KIND_TO_LABEL, hasSubstantialStructuredContent, + rejectsWorkflowDispatchPullRequest, + rejectsWorkflowDispatchNonDefaultBranch, }; diff --git a/.github/scripts/issue-quality.test.cjs b/.github/scripts/issue-quality.test.cjs index 80d1c2f9a..ee27b34f1 100644 --- a/.github/scripts/issue-quality.test.cjs +++ b/.github/scripts/issue-quality.test.cjs @@ -12,8 +12,43 @@ const { shouldReopen, shouldEnforceClosure, labelForKind, + isPlaceholderOnlyValue, + isPlaceholder, + isRawPlaceholder, + countWords, + hasConcreteDetail, + rejectsWorkflowDispatchPullRequest, + rejectsWorkflowDispatchNonDefaultBranch, } = require("./issue-quality.cjs"); +function featureBodyWithGoal(goal) { + return [ + "### Area", + "CLI", + "### What are you trying to accomplish?", + goal, + "### What prevents this today?", + "Port resets to 10100 after every ocx stop command.", + "### What should OpenCodex do?", + "Persist the last used port in config across restarts.", + "### Example usage or interface", + "ocx start --port 8080 && ocx stop && ocx start", + ].join("\n"); +} + +function featureBodyWithExample(example) { + return [ + "### What are you trying to accomplish?", + "Route voice requests to a configured fallback provider when the primary quota is exhausted.", + "### What prevents this today?", + "Voice mode is hard-wired to the primary Codex quota and cannot switch providers.", + "### What should OpenCodex do?", + "Expose a setting to choose the fallback voice model and provider.", + "### Example usage or interface", + example, + ].join("\n"); +} + // --------------------------------------------------------------------------- // Detection // --------------------------------------------------------------------------- @@ -163,6 +198,79 @@ describe("validateIssue - feature", () => { assert.equal(result.valid, true); }); + it("rejects issue #401-style low-effort feature with placeholder example", () => { + const body = [ + "### Area", + "Proxy and routing", + "### What are you trying to accomplish?", + "Quota for Chatgpt running out that can no longer use voice mode. Would like to change other model for that", + "### What prevents this today?", + "No usage without codex quota", + "### What should OpenCodex do?", + "Change another voice model", + "### Example usage or interface", + "NA", + ].join("\n"); + const result = validateIssue({ + title: "Change voice chat to different model", + body, + labels: ["enhancement"], + }); + assert.equal(result.kind, "feature"); + assert.equal(result.valid, false); + assert.ok(result.reasons.some((r) => r.includes("placeholder"))); + assert.ok(result.reasons.some((r) => r.includes("expected behaviour"))); + }); + + it("rejects feature reports with placeholder example variants", () => { + const placeholders = [ + "NA", + "N/A", + "_N/A_", + "NA.", + "N/A.", + "Not applicable", + "Not applicable.", + "Not available!", + ]; + for (const example of placeholders) { + const body = [ + "### What are you trying to accomplish?", + "Route voice requests to a configured fallback provider when the primary quota is exhausted.", + "### What prevents this today?", + "Voice mode is hard-wired to the primary Codex quota and cannot switch providers.", + "### What should OpenCodex do?", + "Expose a setting to choose the fallback voice model and provider.", + "### Example usage or interface", + example, + ].join("\n"); + const result = validateIssue({ title: "Voice fallback routing", body, labels: ["enhancement"] }); + assert.equal(result.valid, false, `Expected placeholder example "${example}" to be invalid`); + assert.ok( + result.reasons.some((r) => r.includes("placeholder")), + `Expected placeholder reason for "${example}", got: ${result.reasons.join("; ")}`, + ); + assert.ok(!result.reasons.some((r) => r.includes("example usage"))); + } + }); + + it("reports blank example usage as missing, not placeholder", () => { + const body = [ + "### What are you trying to accomplish?", + "Route voice requests to a configured fallback provider when the primary quota is exhausted.", + "### What prevents this today?", + "Voice mode is hard-wired to the primary Codex quota and cannot switch providers.", + "### What should OpenCodex do?", + "Expose a setting to choose the fallback voice model and provider.", + "### Example usage or interface", + "", + ].join("\n"); + const result = validateIssue({ title: "Voice fallback routing", body, labels: ["enhancement"] }); + assert.equal(result.valid, false); + assert.ok(result.reasons.some((r) => r.includes("example usage"))); + assert.ok(!result.reasons.some((r) => r.includes("placeholder"))); + }); + it("accepts a valid legacy feature request without blocker/example headings", () => { const body = [ "### Problem to solve", @@ -192,8 +300,167 @@ describe("validateIssue - feature", () => { assert.equal(result.kind, "feature"); assert.equal(result.valid, true); }); -}); + it("rejects terse goal sections that only contain a keyword, digit, or punctuation", () => { + const terseGoals = ["API", "provider", "1", "/", "use CLI", "route 1"]; + for (const goal of terseGoals) { + const result = validateIssue({ + title: "Improve feature request quality", + body: featureBodyWithGoal(goal), + labels: ["enhancement"], + }); + assert.equal(result.kind, "feature"); + assert.equal(result.valid, false, `Expected terse goal "${goal}" to be invalid`); + assert.ok( + result.reasons.some((r) => r.includes("too vague")), + `Expected too vague reason for "${goal}", got: ${result.reasons.join(", ")}`, + ); + } + }); + + it("rejects a single long non-CJK word as overly terse", () => { + const terseGoals = [ + "провайдер", + "маршрут", + "πάροχος", + "واجهة", + ]; + for (const goal of terseGoals) { + assert.equal(countWords(goal), 1, `Expected "${goal}" to count as one word`); + const result = validateIssue({ + title: "Improve feature request quality", + body: featureBodyWithGoal(goal), + labels: ["enhancement"], + }); + assert.equal(result.kind, "feature"); + assert.equal(result.valid, false, `Expected terse goal "${goal}" to be invalid`); + assert.ok( + result.reasons.some((r) => r.includes("too vague")), + `Expected too vague reason for "${goal}", got: ${result.reasons.join(", ")}`, + ); + } + }); + + it("counts mixed-script CJK text without inflating non-CJK letter length", () => { + assert.equal(countWords("provider中"), 2); + assert.equal(countWords("провайдер中"), 2); + assert.equal(countWords("configuration中"), 2); + assert.equal(countWords("中provider文"), 3); + }); + + it("rejects mixed-script CJK stubs that only inflate letter counts", () => { + const terseGoals = ["provider中", "провайдер中", "configuration中"]; + for (const goal of terseGoals) { + assert.ok(countWords(goal) < 8, `Expected "${goal}" to stay under 8 units`); + const result = validateIssue({ + title: "Improve feature request quality", + body: featureBodyWithGoal(goal), + labels: ["enhancement"], + }); + assert.equal(result.kind, "feature"); + assert.equal(result.valid, false, `Expected terse goal "${goal}" to be invalid`); + assert.ok( + result.reasons.some((r) => r.includes("too vague")), + `Expected too vague reason for "${goal}", got: ${result.reasons.join(", ")}`, + ); + } + }); + + it("accepts sufficiently detailed goal sections", () => { + const detailedGoal = + "Expose a dashboard setting to choose the fallback voice model and provider."; + const detailedResult = validateIssue({ + title: "Voice fallback routing", + body: featureBodyWithGoal(detailedGoal), + labels: ["enhancement"], + }); + assert.equal(detailedResult.kind, "feature"); + assert.equal(detailedResult.valid, true); + assert.ok(countWords(detailedGoal) >= 8); + }); + + it("accepts a 6-7 word goal when it includes concrete technical detail", () => { + const concreteGoal = "Route requests through the configured API provider."; + assert.equal(countWords(concreteGoal), 7); + assert.ok(countWords(concreteGoal) < 8); + assert.ok(countWords(concreteGoal) >= 6); + assert.equal(hasConcreteDetail(concreteGoal), true); + + const concreteResult = validateIssue({ + title: "Voice fallback routing", + body: featureBodyWithGoal(concreteGoal), + labels: ["enhancement"], + }); + assert.equal(concreteResult.kind, "feature"); + assert.equal(concreteResult.valid, true); + }); + + it("rejects a 6-7 word goal that lacks concrete technical detail", () => { + // Avoid keywords that count as concrete detail (api/provider/workflow/…). + const vagueGoal = "Make this process easier for all users."; + assert.equal(countWords(vagueGoal), 7); + assert.equal(hasConcreteDetail(vagueGoal), false); + + const vagueResult = validateIssue({ + title: "Voice fallback routing", + body: featureBodyWithGoal(vagueGoal), + labels: ["enhancement"], + }); + assert.equal(vagueResult.kind, "feature"); + assert.equal(vagueResult.valid, false); + assert.ok(vagueResult.reasons.some((r) => r.includes("too vague"))); + }); + + it("rejects fenced placeholder-only examples", () => { + const fencedPlaceholders = [ + "```\nN/A\n```", + "```text\nN/A\n```", + "```json\nNot applicable.\n```", + "~~~text\nN/A\n~~~", + ]; + for (const example of fencedPlaceholders) { + assert.equal( + isPlaceholderOnlyValue(example), + true, + `Expected fenced placeholder to match: ${JSON.stringify(example)}`, + ); + const result = validateIssue({ + title: "Voice fallback routing", + body: featureBodyWithExample(example), + labels: ["enhancement"], + }); + assert.equal(result.valid, false, `Expected fenced placeholder to be invalid: ${JSON.stringify(example)}`); + assert.ok( + result.reasons.some((r) => r.includes("placeholder")), + `Expected placeholder reason for ${JSON.stringify(example)}, got: ${result.reasons.join("; ")}`, + ); + } + }); + + it("accepts real fenced examples that merely mention N/A", () => { + const realExamples = [ + "```text\nThe API returns N/A when no provider is configured.\n```", + '```json\n{"provider":"N/A","fallback":"anthropic"}\n```', + ]; + for (const example of realExamples) { + assert.equal( + isPlaceholderOnlyValue(example), + false, + `Expected real example not to be placeholder-only: ${JSON.stringify(example)}`, + ); + const result = validateIssue({ + title: "Voice fallback routing", + body: featureBodyWithExample(example), + labels: ["enhancement"], + }); + assert.equal( + result.valid, + true, + `Expected real fenced example to remain valid, got: ${result.reasons.join("; ")}`, + ); + } + }); +}); // --------------------------------------------------------------------------- // Validation: bug // --------------------------------------------------------------------------- @@ -271,6 +538,29 @@ describe("validateIssue - bug", () => { assert.equal(result.valid, true, `Expected valid but got: ${result.reasons.join(", ")}`); }); + it("accepts a legacy bug with N/A-style placeholders in Version and Operating system", () => { + for (const placeholder of ["N/A", "NA", "Not applicable.", "Not available!"]) { + const body = [ + "### Summary", + "Proxy crashes on startup when streaming is enabled.", + "### Reproduction", + "Run ocx start and send any streaming request.", + "### Version", + placeholder, + "### Operating system", + placeholder, + ].join("\n"); + const result = validateIssue({ title: "[Bug]: crash", body, labels: ["bug"] }); + assert.equal(result.kind, "bug"); + assert.equal( + result.valid, + true, + `Expected legacy env placeholder "${placeholder}" to remain valid, got: ${result.reasons.join(", ")}`, + ); + assert.ok(!result.reasons.some((r) => r.includes("Version"))); + } + }); + it("rejects a new-form bug where env fields were actively cleared", () => { const body = [ "### Client or integration", @@ -424,6 +714,55 @@ describe("normalisation", () => { assert.equal(clean("_No response_"), ""); }); + it("treats NA and not applicable as placeholders", () => { + assert.equal(clean("NA"), ""); + assert.equal(clean("N/A"), ""); + assert.equal(clean("_N/A_"), ""); + assert.equal(clean("NA."), ""); + assert.equal(clean("N/A."), ""); + assert.equal(clean("not applicable"), ""); + assert.equal(clean("Not applicable."), ""); + assert.equal(clean("Not available!"), ""); + }); + + it("does not treat sentences containing placeholder phrases as empty", () => { + assert.equal(clean("This is N/A for voice mode today."), "This is N/A for voice mode today."); + assert.equal(clean("Not applicable to Claude Code."), "Not applicable to Claude Code."); + }); + + it("shares one placeholder matcher across clean, isPlaceholder, and isRawPlaceholder", () => { + const placeholders = [ + "No response", + "NA", + "N/A", + "_N/A_", + "NA.", + "N/A.", + "None", + "Todo", + "TBD", + "Not applicable", + "Not applicable.", + "Not available!", + "```\nN/A\n```", + "```text\nN/A\n```", + "```json\nNot applicable.\n```", + "~~~text\nN/A\n~~~", + ]; + for (const value of placeholders) { + assert.equal(isPlaceholderOnlyValue(value), true, value); + assert.equal(isPlaceholder(value), true, value); + assert.equal(isRawPlaceholder(value), true, value); + assert.equal(clean(value), "", value); + } + assert.equal(isPlaceholderOnlyValue("Route voice traffic to provider N/A fallback"), false); + assert.equal(isPlaceholderOnlyValue("```text\nThe API returns N/A when no provider is configured.\n```"), false); + assert.equal(isPlaceholderOnlyValue('```json\n{"provider":"N/A","fallback":"anthropic"}\n```'), false); + assert.equal(isPlaceholder("use CLI"), false); + assert.equal(isRawPlaceholder(""), false); + assert.equal(isRawPlaceholder(null), false); + }); + it("strips HTML comments", () => { assert.equal(clean("Hello world"), "Hello world"); }); @@ -750,3 +1089,49 @@ describe("labelForKind", () => { assert.equal(labelForKind("unknown"), null); }); }); + +// --------------------------------------------------------------------------- +// workflow_dispatch guards +// --------------------------------------------------------------------------- + +describe("rejectsWorkflowDispatchPullRequest", () => { + it("rejects pull request numbers on workflow_dispatch", () => { + assert.equal( + rejectsWorkflowDispatchPullRequest({ pull_request: {} }, 423, "workflow_dispatch"), + "#423 is a pull request. This workflow only accepts issue numbers.", + ); + }); + + it("allows issues and non-dispatch events", () => { + assert.equal(rejectsWorkflowDispatchPullRequest({ pull_request: {} }, 423, "issues"), null); + assert.equal(rejectsWorkflowDispatchPullRequest({}, 42, "workflow_dispatch"), null); + }); +}); + +describe("rejectsWorkflowDispatchNonDefaultBranch", () => { + it("rejects workflow_dispatch runs that are not on the default branch", () => { + assert.equal( + rejectsWorkflowDispatchNonDefaultBranch( + "workflow_dispatch", + "refs/heads/fix/issue-quality-low-effort-reports", + "main", + ), + "workflow_dispatch must run from the default branch (main); selected ref was refs/heads/fix/issue-quality-low-effort-reports.", + ); + }); + + it("allows default-branch dispatches and normal issue events", () => { + assert.equal( + rejectsWorkflowDispatchNonDefaultBranch("workflow_dispatch", "refs/heads/main", "main"), + null, + ); + assert.equal( + rejectsWorkflowDispatchNonDefaultBranch( + "issues", + "refs/heads/fix/issue-quality-low-effort-reports", + "main", + ), + null, + ); + }); +}); \ No newline at end of file diff --git a/.github/workflows/enforce-issue-quality.yml b/.github/workflows/enforce-issue-quality.yml index 73a1e4b6b..3cec9fa5b 100644 --- a/.github/workflows/enforce-issue-quality.yml +++ b/.github/workflows/enforce-issue-quality.yml @@ -10,23 +10,33 @@ on: - opened - edited - reopened - -permissions: - contents: read - issues: write + workflow_dispatch: + inputs: + issue_number: + description: Issue number to validate and enforce + required: true + type: number concurrency: - group: issue-quality-${{ github.event.issue.number }} + group: issue-quality-${{ github.event.issue.number || inputs.issue_number }} cancel-in-progress: true jobs: validate: runs-on: ubuntu-latest + permissions: + contents: read + # Required for issue closure, reopen, and bot comments. + issues: write steps: - name: Checkout trusted workflow code uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + # Always load validator scripts from the repository default branch so + # a branch-selected workflow_dispatch cannot execute untrusted code + # with issues:write. + ref: ${{ github.event.repository.default_branch }} persist-credentials: false sparse-checkout: .github/scripts @@ -42,15 +52,46 @@ jobs: shouldReopen, shouldEnforceClosure, labelForKind, + rejectsWorkflowDispatchPullRequest, + rejectsWorkflowDispatchNonDefaultBranch, } = require(path.join(process.cwd(), ".github", "scripts", "issue-quality.cjs")); const { owner, repo } = context.repo; - const issue_number = context.payload.issue.number; + let issue_number; + if (context.eventName === "workflow_dispatch") { + const rawIssueNumber = context.payload.inputs.issue_number; + const parsedIssueNumber = Number(rawIssueNumber); + if ( + !Number.isSafeInteger(parsedIssueNumber) || + parsedIssueNumber <= 0 + ) { + core.setFailed( + `Invalid workflow_dispatch issue_number: ${rawIssueNumber}. Expected a positive integer.`, + ); + return; + } + issue_number = parsedIssueNumber; + } else { + issue_number = context.payload.issue.number; + } const actor = context.actor; const eventType = context.eventName === "issues" ? context.payload.action : ""; + // ----------------------------------------------------------------- + // Reject branch-selected manual dispatches before any issue API use + // ----------------------------------------------------------------- + const nonDefaultBranchFailure = rejectsWorkflowDispatchNonDefaultBranch( + context.eventName, + context.ref, + context.payload.repository?.default_branch, + ); + if (nonDefaultBranchFailure) { + core.setFailed(nonDefaultBranchFailure); + return; + } + // ----------------------------------------------------------------- // Fetch live issue state // ----------------------------------------------------------------- @@ -58,6 +99,16 @@ jobs: owner, repo, issue_number, }); + const pullRequestFailure = rejectsWorkflowDispatchPullRequest( + issue, + issue_number, + context.eventName, + ); + if (pullRequestFailure) { + core.setFailed(pullRequestFailure); + return; + } + // ----------------------------------------------------------------- // Trusted-user exemption // ----------------------------------------------------------------- diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index cb06538c7..c5dfee63e 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -164,6 +164,62 @@ describe("GitHub Actions hardening", () => { expect(workflow).not.toMatch(/uses:\s+\S+@(?:v\d+|main|master)\b/); }); + test("issue-quality workflow rejects workflow_dispatch pull request numbers before mutation", async () => { + const workflow = await readText(".github/workflows/enforce-issue-quality.yml"); + + // Manual dispatch is supported, but only with a positive issue number. + expect(workflow).toContain("workflow_dispatch:"); + expect(workflow).toContain("issue_number:"); + expect(workflow).toContain("Number.isSafeInteger(parsedIssueNumber)"); + expect(workflow).toContain("parsedIssueNumber <= 0"); + + // Job-scoped permissions only (no top-level issues:write). + expect(workflow).toMatch( + /jobs:\s*\n\s*validate:[\s\S]*?permissions:\s*\n\s*contents: read\s*\n\s*#.*\n\s*issues: write/, + ); + const beforeJobs = workflow.split(/jobs:\s*\n/)[0]!; + expect(beforeJobs).not.toMatch(/^\s*permissions:/m); + + // Trusted scripts always come from the repository default branch. + const checkoutStep = workflow + .split("- name: Checkout trusted workflow code")[1]! + .split(/\n {6}- name:/)[0]!; + expect(checkoutStep).toContain("ref: ${{ github.event.repository.default_branch }}"); + expect(checkoutStep).toContain("persist-credentials: false"); + expect(checkoutStep).toContain("sparse-checkout: .github/scripts"); + + const script = workflow + .split("script: |")[1]! + .split(/\n {6}- name:/)[0]!; + + // Invalid issue numbers fail before any issues API call. + const invalidNumberIdx = script.indexOf("Invalid workflow_dispatch issue_number:"); + const firstIssuesGetIdx = script.indexOf("github.rest.issues.get({"); + expect(invalidNumberIdx).toBeGreaterThan(-1); + expect(firstIssuesGetIdx).toBeGreaterThan(-1); + expect(invalidNumberIdx).toBeLessThan(firstIssuesGetIdx); + + // Non-default-branch dispatches fail before any issues API mutation. + const branchGuardIdx = script.indexOf("const nonDefaultBranchFailure = rejectsWorkflowDispatchNonDefaultBranch("); + const firstMutationIdx = script.indexOf("github.rest.issues.update({"); + expect(branchGuardIdx).toBeGreaterThan(-1); + expect(firstMutationIdx).toBeGreaterThan(-1); + expect(branchGuardIdx).toBeLessThan(firstMutationIdx); + expect(branchGuardIdx).toBeLessThan(firstIssuesGetIdx); + + // Pull-request numbers are rejected after issues.get and before mutations. + const prGuardIdx = script.indexOf("const pullRequestFailure = rejectsWorkflowDispatchPullRequest("); + const listCommentsIdx = script.indexOf("github.rest.issues.listComments"); + const addLabelsIdx = script.indexOf("github.rest.issues.addLabels"); + expect(prGuardIdx).toBeGreaterThan(-1); + expect(prGuardIdx).toBeGreaterThan(firstIssuesGetIdx); + expect(prGuardIdx).toBeLessThan(listCommentsIdx); + expect(prGuardIdx).toBeLessThan(addLabelsIdx); + expect(prGuardIdx).toBeLessThan(firstMutationIdx); + expect(script).toContain("if (pullRequestFailure) {"); + expect(script).toContain("core.setFailed(pullRequestFailure);"); + }); + test("React Doctor workflow is SHA-pinned, engine-pinned, advisory, and read-only", async () => { const workflow = await readText(".github/workflows/react-doctor.yml");