diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 0ad7b1b0..a4d691e9 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -68,6 +68,22 @@ jobs: - name: Test release tag resolver run: scripts/test-resolve-release-tag.sh + triage-detector: + name: triage detector matrix + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Check out repository + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + + # pr-triage.yml runs on pull_request_target and never checks out PR + # code, so its inline detector can only be exercised here, where the + # proposed workflow body IS the checked-out one. + - name: Test inline-test detector + run: node scripts/test-pr-triage-detect.mjs + test: name: test (${{ matrix.toolchain }}) runs-on: ubuntu-latest diff --git a/.github/workflows/pr-triage.yml b/.github/workflows/pr-triage.yml index 1883746f..17d427e0 100644 --- a/.github/workflows/pr-triage.yml +++ b/.github/workflows/pr-triage.yml @@ -76,9 +76,106 @@ jobs: // line after horizontal whitespace, covering bare, cfg(test), namespaced, // optional leading ::, and raw-identifier (r#test) forms. No head-file fetch // or lexer: needs-tests is advisory, not a merge gate. - const addsInlineTest = files.some(f => - f.filename.endsWith(".rs") && f.patch && - /^\+[ \t]*#\[[ \t]*(?:cfg[ \t]*\([ \t]*test[ \t]*\)|(?:::[ \t]*)?(?:[\w-]+[ \t]*::[ \t]*)*(?:r#)?test\b)/m.test(f.patch)); + // + // The final path component may also carry `test` as an underscore-delimited + // segment, so third-party harnesses land without enumerating them one by one + // (#[test_case(1)], #[wasm_bindgen_test]). It is deliberately a segment and + // not a substring: a substring would also match #[contest] / #[latest], and a + // false positive here SUPPRESSES needs-tests rather than adding it. This + // heuristic has to fail loud, because a wrongly labeled PR gets corrected by + // the author while a wrongly cleared one is invisible. That trade costs the + // no-underscore harness names (#[rstest]), which stay unmatched on purpose. + // + // The segment classes are [A-Za-z0-9]+ rather than \w+ on purpose, so that `_` + // is only ever the literal delimiter. \w+ contains `_`, which makes the repeat + // ambiguous and backtracks exponentially: on this workflow's pull_request_target + // trigger f.patch is fork-controlled, so `#[test` followed by a long `_a` run is + // a permissionless stall of the triage job. + // Rust allows whitespace, line breaks, and comments between the + // attribute path and ] or (. A flat regex cannot decide that + // boundary: `//` runs to end of line, and block comments NEST, which + // is beyond regular languages — every enumeration of comment + // spellings so far has either dropped a legal separator (regressing + // real tests into needs-tests) or let unrelated patch records + // complete each other (clearing needs-tests from a testless PR, the + // invisible direction). So the closer is found by a token-aware + // scan instead: it consumes horizontal whitespace, `//`-to-newline, + // and depth-counted `/* */` runs, and accepts only when the first + // real token after the attribute path is `]` or `(`. + // + // Association is positional: the scan starts on the attribute's own + // added line and may continue ONLY through the immediately following + // added lines (a context line, hunk header, or removal breaks the + // chain). Unrelated `]`/`(` lines elsewhere in the patch can never + // complete a path they do not adjoin. The continuation is bounded; + // past the bound the scan gives up and reports NO inline test, which + // fails toward the loud label rather than the silent clearance. + // + // The detector body is fenced by the TRIAGE_DETECTOR markers so + // scripts/test-pr-triage-detect.mjs can extract and run it against + // the committed case matrix. Keep the fenced block self-contained. + // TRIAGE_DETECTOR_BEGIN + const TEST_ATTR_PATH = + "(?:cfg[ \\t]*\\([ \\t]*test[ \\t]*\\)|(?:::[ \\t]*)?(?:[\\w-]+[ \\t]*::[ \\t]*)*(?:r#)?(?:[A-Za-z0-9]+_)*test(?:_[A-Za-z0-9]+)*)"; + // Anchored to the start of one added line; group 1 is whatever + // follows the path on that line, handed to the separator scan. + const TEST_ATTR_LINE = new RegExp( + "^[ \\t]*#\\[[ \\t]*" + TEST_ATTR_PATH + "([\\s\\S]*)$" + ); + // An attribute split across more added lines than this is not a + // spelling anyone writes; refusing to scan further keeps the walk + // linear in the patch and fails toward the loud label. + const MAX_ATTR_CONTINUATION_LINES = 16; + // segs[0] is the remainder of the attribute's own line; each later + // entry is the text of one immediately following added line. True + // only when the first non-separator token across them is ] or (. + // Single forward pass, no backtracking: fork-controlled input. + function attrCloserFollows(segs) { + let depth = 0; // open block-comment nesting carried across lines + for (const seg of segs) { + let i = 0; + while (i < seg.length) { + if (depth > 0) { + if (seg.startsWith("/*", i)) { depth += 1; i += 2; } + else if (seg.startsWith("*/", i)) { depth -= 1; i += 2; } + else { i += 1; } + continue; + } + const c = seg[i]; + if (c === " " || c === "\t") { i += 1; continue; } + if (seg.startsWith("//", i)) break; // comment to end of line + if (seg.startsWith("/*", i)) { depth = 1; i += 2; continue; } + return c === "]" || c === "("; + } + // Line exhausted inside separators; the newline is itself a + // separator, so continue on the next added line. + } + return false; // out of adjoined lines (or bound hit): stay loud + } + function patchAddsInlineTest(patch) { + const lines = patch.split("\n"); + for (let i = 0; i < lines.length; i++) { + if (lines[i][0] !== "+") continue; + const m = TEST_ATTR_LINE.exec(lines[i].slice(1)); + if (m === null) continue; + const segs = [m[1]]; + for ( + let j = i + 1; + j < lines.length && + lines[j][0] === "+" && + segs.length <= MAX_ATTR_CONTINUATION_LINES; + j++ + ) { + segs.push(lines[j].slice(1)); + } + if (attrCloserFollows(segs)) return true; + } + return false; + } + // TRIAGE_DETECTOR_END + const addsInlineTest = files.some( + f => f.filename.endsWith(".rs") && !!f.patch && patchAddsInlineTest(f.patch) + ); const touchedTests = names.some(n => n.includes("/tests/") || n.endsWith("_test.rs")) || addsInlineTest; if (changedRust && !touchedTests) want.add("needs-tests"); diff --git a/scripts/test-pr-triage-detect.mjs b/scripts/test-pr-triage-detect.mjs new file mode 100755 index 00000000..793abe57 --- /dev/null +++ b/scripts/test-pr-triage-detect.mjs @@ -0,0 +1,172 @@ +#!/usr/bin/env node +// Case matrix for the inline-test detector embedded in +// .github/workflows/pr-triage.yml. The workflow runs on pull_request_target +// and deliberately never checks out PR code, so the detector cannot be tested +// where it runs; this script extracts the fenced TRIAGE_DETECTOR block from +// the committed workflow body and exercises it here, where pr-checks.yml DOES +// check out the proposed workflow. If the fence markers move or the block +// stops being self-contained, this script fails loudly rather than testing a +// stale copy. +// +// The matrix encodes the review contract for the detector +// (Gitlawb/node#277): legal Rust separators between the attribute path and +// its ]/( delimiter must be accepted (line comments, nested block comments, +// splits onto immediately following added lines), while unrelated patch +// records — delimiter-looking lines before the path, later in the hunk, or in +// another hunk — must never complete a path they do not adjoin. False +// negatives here SUPPRESS the needs-tests label silently, so every uncertain +// path in the detector is required to answer "no inline test". + +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); +const workflow = readFileSync( + join(repoRoot, ".github/workflows/pr-triage.yml"), + "utf8" +); + +const BEGIN = "// TRIAGE_DETECTOR_BEGIN"; +const END = "// TRIAGE_DETECTOR_END"; +const begin = workflow.indexOf(BEGIN); +const end = workflow.indexOf(END); +if (begin === -1 || end === -1 || end <= begin) { + console.error("FAIL: TRIAGE_DETECTOR fence not found in pr-triage.yml"); + process.exit(1); +} +const block = workflow.slice(begin + BEGIN.length, end); + +let patchAddsInlineTest; +try { + const factory = new Function(`${block}\nreturn patchAddsInlineTest;`); + patchAddsInlineTest = factory(); +} catch (err) { + console.error( + "FAIL: fenced detector block is not self-contained JavaScript:", + err.message + ); + process.exit(1); +} + +// Each patch is the `patch` field GitHub's listFiles API returns: hunk +// headers plus +/-/space-prefixed lines, no ---/+++ file headers. +const cases = [ + // ── Accepted spellings ──────────────────────────────────────────────── + ["bare same-line", "@@ -1,0 +1,2 @@\n+#[test]\n+fn a() {}", true], + ["cfg(test)", "@@ -1,0 +1,1 @@\n+#[cfg(test)]", true], + ["indented with inner space", "@@ -1,0 +1,1 @@\n+ #[ test ]", true], + [ + "namespaced with args", + '@@ -1,0 +1,1 @@\n+#[tokio::test(flavor = "multi_thread")]', + true, + ], + ["test_case harness", "@@ -1,0 +1,1 @@\n+#[test_case(1)]", true], + ["wasm_bindgen_test harness", "@@ -1,0 +1,1 @@\n+#[wasm_bindgen_test]", true], + ["raw identifier", "@@ -1,0 +1,1 @@\n+#[r#test]", true], + [ + "line comment then closer on next added line", + "@@ -1,0 +1,2 @@\n+#[test // rationale\n+]", + true, + ], + [ + "nested block comment, same line", + "@@ -1,0 +1,1 @@\n+#[test /* outer /* inner */ outer */]", + true, + ], + [ + "block comment spanning added lines", + "@@ -1,0 +1,3 @@\n+#[test /* why\n+ still why */ ]\n+fn a() {}", + true, + ], + [ + "path-only line, ( on the immediately following added line", + "@@ -1,0 +1,2 @@\n+#[test_case\n+(1)]", + true, + ], + [ + "whitespace-only continuation before closer", + "@@ -1,0 +1,3 @@\n+#[test\n+\t\n+]", + true, + ], + // ── Rejected spellings and adversarial shapes ───────────────────────── + ["rstest stays excluded", "@@ -1,0 +1,1 @@\n+#[rstest]", false], + ["substring #[testable]", "@@ -1,0 +1,1 @@\n+#[testable]", false], + ["substring #[contest]", "@@ -1,0 +1,1 @@\n+#[contest]", false], + [ + "delimiter-looking line BEFORE the path", + "@@ -1,0 +1,2 @@\n+(\n+#[test_case", + false, + ], + [ + "raw-string fixture path + unrelated ( later in the same hunk", + '@@ -1,0 +1,5 @@\n+let s = r#"\n+#[test_case\n+not a separator token\n+"#;\n+let t = (1);', + false, + ], + [ + "path at end of one hunk, closer in another hunk", + "@@ -1,0 +1,1 @@\n+#[test_case\n@@ -10,0 +11,1 @@\n+(1)]", + false, + ], + [ + "closer only on a context line", + "@@ -1,1 +1,1 @@\n+#[test_case\n (1)]", + false, + ], + [ + "closer only on a removed line", + "@@ -1,1 +1,1 @@\n+#[test_case\n-(1)]", + false, + ], + [ + "unfinished block comment never closes", + "@@ -1,0 +1,2 @@\n+#[test /*\n+ still open", + false, + ], + [ + "continuation bound exceeded stays loud", + "@@ -1,0 +1,40 @@\n+#[test /*\n" + "+ filler\n".repeat(30) + "+ */ ]", + false, + ], +]; + +let failures = 0; +for (const [name, patch, expected] of cases) { + const got = patchAddsInlineTest(patch); + if (got !== expected) { + failures += 1; + console.error(`FAIL: ${name}: expected ${expected}, got ${got}`); + } +} + +// Runtime probe: the detector walks fork-controlled input on +// pull_request_target, so a pathological head must not stall the job. The +// long `_a` run is the historical exponential-backtracking shape for the +// attribute-path regex; the comment run exercises the scanner loop. +const probes = [ + ["long _a run", "@@ -1,0 +1,1 @@\n+#[test" + "_a".repeat(30000), false], + [ + "long unclosed comment line", + "@@ -1,0 +1,1 @@\n+#[test /*" + " *".repeat(30000), + false, + ], +]; +for (const [name, patch, expected] of probes) { + const t0 = process.hrtime.bigint(); + const got = patchAddsInlineTest(patch); + const ms = Number(process.hrtime.bigint() - t0) / 1e6; + if (got !== expected) { + failures += 1; + console.error(`FAIL: probe ${name}: expected ${expected}, got ${got}`); + } + if (ms > 1000) { + failures += 1; + console.error(`FAIL: probe ${name}: took ${ms.toFixed(0)}ms (>1000ms)`); + } +} + +if (failures) { + console.error(`${failures} failure(s) across ${cases.length + probes.length} cases`); + process.exit(1); +} +console.log(`ok: ${cases.length + probes.length} detector cases passed`);