-
Notifications
You must be signed in to change notification settings - Fork 470
fix(ci): carry PR-target/issue-quality harden onto dev2-go (#631) #677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| "use strict"; | ||
|
|
||
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
|
|
||
| describe("enforce-pr-target workflow", () => { | ||
| const workflowPath = path.join(__dirname, "../workflows/enforce-pr-target.yml"); | ||
| const workflow = fs.readFileSync(workflowPath, "utf8"); | ||
|
|
||
| it("uses pull_request_target without checking out PR head code", () => { | ||
| assert.match(workflow, /pull_request_target:/); | ||
| assert.doesNotMatch( | ||
| workflow, | ||
| /actions\/checkout@/, | ||
| "wrong-branch enforcer must not check out untrusted PR code", | ||
| ); | ||
| }); | ||
|
|
||
| it("grants contents:write so draft GraphQL mutations work with GITHUB_TOKEN", () => { | ||
| // convertPullRequestToDraft / markPullRequestReadyForReview fail with | ||
| // "Resource not accessible by integration" when contents stays unset/read | ||
| // (seen on #626). Assert the real permissions block, not comment text | ||
| // that also mentions these scopes. | ||
| const permissionsBlock = workflow.match(/^permissions:\n((?:[ \t]+.+\n)+)/m); | ||
| assert.ok(permissionsBlock, "workflow must declare a top-level permissions block"); | ||
| const lines = permissionsBlock[1] | ||
| .split("\n") | ||
| .map((line) => line.trim()) | ||
| .filter(Boolean) | ||
| .sort(); | ||
| assert.deepEqual(lines, ["contents: write", "pull-requests: write"]); | ||
| }); | ||
|
|
||
| it("fails the required check on a wrong base even if draft conversion fails", () => { | ||
| assert.match(workflow, /core\.setFailed\(/); | ||
| assert.match(workflow, /draftConversionFailed/); | ||
| assert.match(workflow, /Could not convert pull request to draft/); | ||
| }); | ||
|
|
||
| it("soft-fails ready-for-review restoration the same way", () => { | ||
| assert.match(workflow, /readyConversionFailed/); | ||
| assert.match(workflow, /Could not mark pull request ready for review/); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,19 +25,29 @@ function unwrapSingleEnclosingFence(text) { | |
| return match[2]; | ||
| } | ||
|
|
||
| function isPlaceholderOnlyValue(raw) { | ||
| if (typeof raw !== "string") return false; | ||
| /** | ||
| * Shared strip/trim/unwrap used by placeholder and unusable-stand-in matchers. | ||
| * Returns null when the value is absent after normalisation. | ||
| */ | ||
| function normalizeRawSectionValue(raw) { | ||
| if (typeof raw !== "string") return null; | ||
| let value = raw.replace(/<!--[\s\S]*?-->/g, "").trim(); | ||
| if (!value) return false; | ||
| if (!value) return null; | ||
|
|
||
| // A lone fenced block whose entire body is a placeholder is still placeholder | ||
| // text (e.g. ```text\nN/A\n```), not a real example. | ||
| // A lone fenced block whose entire body is a stand-in is still a stand-in | ||
| // (e.g. ```text\nN/A\n```), not a real example. | ||
| const unwrapped = unwrapSingleEnclosingFence(value); | ||
| if (unwrapped !== null) { | ||
| value = unwrapped.trim(); | ||
| if (!value) return false; | ||
| if (!value) return null; | ||
| } | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| function isPlaceholderOnlyValue(raw) { | ||
| const value = normalizeRawSectionValue(raw); | ||
| if (value === null) return false; | ||
| return PLACEHOLDER_ONLY_RE.test(value); | ||
| } | ||
|
|
||
|
|
@@ -398,6 +408,20 @@ function isPlaceholder(text) { | |
| return isPlaceholderOnlyValue(text); | ||
| } | ||
|
|
||
| /** | ||
| * True when Version is an "I don't know" stand-in rather than an install id. | ||
| * Kept separate from PLACEHOLDER_ONLY_RE so legacy N/A / No response soft-pass | ||
| * behaviour is unchanged. | ||
| */ | ||
| const UNUSABLE_VERSION_RE = | ||
| /^[\s_*~`]*(?:unknown|unkown|uknown|don'?t\s+know|do\s+not\s+know|idk|dunno|not\s+sure|unsure|\?+|모름|잘\s*모름|모르겠(?:습니다|음)?|不明|わからない|分からない|不知道|不清楚|keine\s+ahnung|wei[sß]{1,2}\s+nicht)[\s_*~`]*[.!?]*$/i; | ||
|
|
||
| function isUnusableVersion(raw) { | ||
| const value = normalizeRawSectionValue(raw); | ||
| if (value === null) return false; | ||
| return UNUSABLE_VERSION_RE.test(value); | ||
| } | ||
|
|
||
| const CJK_RE = | ||
| /[\p{Script=Han}\p{Script=Hiragana}\p{Script=Katakana}\p{Script=Hangul}]/gu; | ||
|
|
||
|
|
@@ -433,6 +457,16 @@ function isTooTerseFeatureSection(text) { | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Bug Reproduction needs steps or concrete signals. A title-like phrase with | ||
| * no commands, paths, digits, or product keywords is not actionable. | ||
| */ | ||
| function isTooTerseBugReproduction(text) { | ||
| if (isEmpty(text) || isPlaceholder(text)) return false; | ||
| if (hasConcreteDetail(text)) return false; | ||
| return countWords(text) < 12; | ||
| } | ||
|
|
||
| /** | ||
| * Check if raw section text is a placeholder-only variant without relying on | ||
| * clean() first. Used to distinguish intentionally blank optional fields | ||
|
|
@@ -628,6 +662,8 @@ function validateIssue(issue) { | |
| const repro = extractSection(body, "Reproduction"); | ||
| const version = extractSection(body, "Version"); | ||
| const os = extractSection(body, "Operating system") ?? extractSection(body, "OS"); | ||
| // New Bug report template always includes Client or integration. | ||
| const isNewBugForm = extractSection(body, "Client or integration") !== null; | ||
|
|
||
| if (isEmpty(summary) && isEmpty(repro)) { | ||
| // Soft-pass substantial non-English / freeform structured reports once | ||
|
|
@@ -655,17 +691,56 @@ function validateIssue(issue) { | |
| if (isEmpty(repro)) { | ||
| reasons.push("Reproduction is empty."); | ||
| guidance.push("List the exact steps to reproduce the problem."); | ||
| } else if (!softPass && isTooTerseBugReproduction(repro)) { | ||
| reasons.push("Reproduction is too vague to act on."); | ||
| guidance.push("List exact steps, commands, and the observed failure — not only a short phrase."); | ||
| } | ||
| } | ||
|
|
||
| // Required environment fields removed after submission. | ||
| // Only fire when the headings exist in the body (new form). Legacy bug | ||
| // reports never had Version or OS fields, so null means absent, not removed. | ||
| // Skip when the raw value is a "No response" placeholder -- the old form had | ||
| // both fields as optional, so legacy issues legitimately contain those headings | ||
| // with the GitHub placeholder. Only close when the field was actively cleared. | ||
| if (!softPass && version !== null && os !== null && isEmpty(version) && isEmpty(os) && | ||
| !isRawPlaceholder(version) && !isRawPlaceholder(os)) { | ||
| // Version "Unknown" / "모름" / "idk" is never actionable, on any form. | ||
| if (!softPass && version !== null && isUnusableVersion(version)) { | ||
| reasons.push("Version is missing or unknown."); | ||
| guidance.push("Report the installed `@bitkyc08/opencodex` version (for example `2.7.42`) or a commit SHA from `ocx --version`."); | ||
| } else if ( | ||
| !softPass && | ||
| isNewBugForm && | ||
| version !== null && | ||
| (isEmpty(version) || isRawPlaceholder(version)) | ||
|
Comment on lines
+704
to
+708
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a contributor edits an otherwise valid new-form bug and deletes the entire Useful? React with 👍 / 👎. |
||
| ) { | ||
| // New form requires Version. Legacy N/A / No response soft-pass stays | ||
| // only for bodies without Client or integration. | ||
| reasons.push("Version is missing."); | ||
| guidance.push("Add your OpenCodex version so we can reproduce the environment."); | ||
| } | ||
|
|
||
| if (!softPass && isNewBugForm && os !== null && isUnusableVersion(os)) { | ||
| reasons.push("Operating system is missing or unknown."); | ||
| guidance.push("Add your OS name and version (for example Windows 11 24H2)."); | ||
| } else if ( | ||
| !softPass && | ||
| isNewBugForm && | ||
| os !== null && | ||
| (isEmpty(os) || isRawPlaceholder(os)) | ||
| ) { | ||
| reasons.push("Operating system is missing."); | ||
| guidance.push("Add your OS name and version (for example Windows 11 24H2)."); | ||
| } | ||
|
|
||
| // Required environment fields removed after submission on bodies that are | ||
| // not the new form (no Client or integration). Legacy reports never had | ||
| // Version or OS fields, so null means absent, not removed. Skip when the | ||
| // raw value is a "No response" placeholder — the old form had both fields | ||
| // as optional. Only close when the field was actively cleared. | ||
| if ( | ||
| !softPass && | ||
| !isNewBugForm && | ||
| version !== null && | ||
| os !== null && | ||
| isEmpty(version) && | ||
| isEmpty(os) && | ||
| !isRawPlaceholder(version) && | ||
| !isRawPlaceholder(os) | ||
| ) { | ||
| reasons.push("Version and Operating system are both missing."); | ||
| guidance.push("Add your OpenCodex version and OS so we can reproduce the environment."); | ||
| } | ||
|
|
@@ -873,6 +948,7 @@ module.exports = { | |
| isPlaceholderOnlyValue, | ||
| isPlaceholder, | ||
| isRawPlaceholder, | ||
| isUnusableVersion, | ||
| countWords, | ||
| hasConcreteDetail, | ||
| labelForKind, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reproduction consisting only of
1,502,ocx, orconfigpasses because any digit or product keyword makeshasConcreteDetail()true and returns before the word-count check. These inputs remain non-actionable despite the new vague-reproduction gate, so require a minimum amount of surrounding content even when a concrete-detail token is present.Useful? React with 👍 / 👎.