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..3a4f50fe 100644 --- a/.github/workflows/pr-triage.yml +++ b/.github/workflows/pr-triage.yml @@ -76,9 +76,221 @@ 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 + } + // Before the candidate scan, a pre-pass walks added patch lines + // tracking Rust lexical state: raw strings (r#*", br#*", cr#*"), + // block comments (/* */ with nesting), regular strings ("...", + // which can span newlines), and line comments (//). An added line + // that starts inside any of these is skipped as a candidate, + // because a #[test_case] appearing there is lexical non-code, not + // a real test attribute. State is only tracked across consecutive + // added lines and reset at any break (context line, removal, + // hunk header): context lines carry a partial view of the file, + // so a " or r#" or /* there might be inside a string that opened + // before the hunk's visible window, and treating it as an opener + // would mark every later added line as non-code (a false negative + // on a real #[test]). The tradeoff is that a non-code region + // opened by a context line is invisible: an added #[test] inside + // an existing raw string or block comment is scanned as code and + // may match (a false positive, silent direction). This is accepted + // because (1) the old detector had the same behavior, (2) the + // pattern is rare, and (3) needs-tests is advisory, not a merge + // gate. Scanning context lines to close this gap would re-introduce + // the false negative, which affects the far more common case of a + // context line carrying a stray " from a string that opened before + // the visible window. Single forward pass, O(n) in patch length. + function patchAddsInlineTest(patch) { + const lines = patch.split("\n"); + const nonCodeStart = new Set(); + let rawHashes = -1; // -1 = not in raw string; >=0 = # count + let blockDepth = 0; + let inString = false; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (!line) continue; + const prefix = line[0]; + // Only scan added lines. Context and removal lines carry + // a partial view of the file: a " or r#" or /* on a context + // line might be inside a string that opened before the + // hunk's visible window, and treating it as an opener would + // mark every later added line as non-code (a false negative + // on a real #[test]). So lexical state is only tracked across + // consecutive added lines and reset at any break. A non-code + // region opened by a context line is invisible; the candidate + // is still scanned, which fails toward the loud label. + if (prefix !== "+") { + rawHashes = -1; blockDepth = 0; inString = false; + continue; + } + const text = line.slice(1); + if (rawHashes >= 0 || blockDepth > 0 || inString) + nonCodeStart.add(i); + let j = 0; + while (j < text.length) { + if (rawHashes >= 0) { + if (text[j] === '"') { + let h = 1; + while (h <= rawHashes && text[j + h] === "#") h++; + if (h > rawHashes) { rawHashes = -1; j += h; continue; } + } + j += 1; continue; + } + if (blockDepth > 0) { + if (text.startsWith("/*", j)) { blockDepth += 1; j += 2; } + else if (text.startsWith("*/", j)) { blockDepth -= 1; j += 2; } + else { j += 1; } + continue; + } + if (inString) { + if (text[j] === "\\") { j += 2; continue; } + if (text[j] === '"') { inString = false; j += 1; continue; } + j += 1; continue; + } + // Recognize Rust char and byte-char literals ('X', '\X', + // '\u{...}', b'X') so their contents never open string or + // comment state. A char literal has a closing ' within a + // bounded distance; a lifetime or label ('a, 'static, + // 'label:) does not, so it falls through as a regular char. + // This is not a skip-to-next-' rule: the lookahead is 2-4 + // chars, or bounded to 20 for \u escapes (at most 6 hex + // digits), never unbounded. + if (text[j] === "'") { + let k = j + 1; + if (text[k] === "\\") { + k += 1; + if (text[k] === "u" && text[k + 1] === "{") { + const uBound = k + 20; + while (k < uBound && text[k] && text[k] !== "}") k++; + if (text[k] === "}") k++; + } else { + k += 1; + } + } else { + k += 1; + } + if (text[k] === "'") { j = k + 1; continue; } + // lifetime or label: just skip the apostrophe + } + if (text[j] === '"') { inString = true; j += 1; continue; } + if (text.startsWith("//", j)) break; + if (text.startsWith("/*", j)) { blockDepth = 1; j += 2; continue; } + const prev = j > 0 ? text[j - 1] : ""; + const isIdent = /[A-Za-z0-9_]/.test(prev); + if (!isIdent && text[j] === "r") { + let k = j + 1, h = 0; + while (text[k + h] === "#") h++; + if (text[k + h] === '"') { + rawHashes = h; j = k + h + 1; continue; + } + } + if (!isIdent && (text[j] === "b" || text[j] === "c") && text[j + 1] === "r") { + let k = j + 2, h = 0; + while (text[k + h] === "#") h++; + if (text[k + h] === '"') { + rawHashes = h; j = k + h + 1; continue; + } + } + j += 1; + } + } + for (let i = 0; i < lines.length; i++) { + if (lines[i][0] !== "+") continue; + if (nonCodeStart.has(i)) 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..26d137a2 --- /dev/null +++ b/scripts/test-pr-triage-detect.mjs @@ -0,0 +1,297 @@ +#!/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 +// positives here SUPPRESS the needs-tests label silently (they set +// touchedTests, which clears the label), while false negatives apply it +// (the loud, corrigible direction). 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, + ], + [ + "raw-string fixture with adjoining closer", + '@@ -1,0 +1,4 @@\n+const FIXTURE: &str = r#"\n+#[test_case\n+(1)]\n+"#;', + false, + ], + [ + "block comment with adjoining closer", + '@@ -1,0 +1,4 @@\n+/* this is a comment\n+#[test_case\n+(1)]\n+end of comment */', + false, + ], + [ + "raw string with no hash delimiters", + '@@ -1,0 +1,4 @@\n+let s = r"\n+#[test_case\n+(1)]\n+";', + false, + ], + [ + "raw string closes then real test attribute on next line", + '@@ -1,0 +1,2 @@\n+let s = r#""#;\n+#[test_case(1)]', + true, + ], + [ + "block comment closes then real test attribute on next line", + '@@ -1,0 +1,2 @@\n+/* c */\n+#[test]', + true, + ], + [ + "multiline regular string with test attribute inside", + '@@ -1,0 +1,3 @@\n+const S: &str = "\n+#[test]\n+";', + false, + ], + [ + "regular string closes then real test on next line", + '@@ -1,0 +1,2 @@\n+let s = "text";\n+#[test]', + true, + ], + [ + "char literal with double quote then real test", + "@@ -1,0 +1,4 @@\n+const QUOTE: char = '\"';\n+#[test]\n+fn real_test() {\n+ assert_eq!(QUOTE as u32, 34);\n+}", + true, + ], + [ + "byte char literal with double quote then real test", + "@@ -1,0 +1,4 @@\n+const QUOTE: u8 = b'\"';\n+#[test]\n+fn real_test() {\n+ assert_eq!(QUOTE, 34);\n+}", + true, + ], + [ + "char literal with escaped quote then real test", + "@@ -1,0 +1,4 @@\n+const Q: char = '\\'';\n+#[test]\n+fn real_test() {\n+ assert_eq!(Q as u32, 39);\n+}", + true, + ], + [ + "char literal with escaped backslash then real test", + "@@ -1,0 +1,4 @@\n+const Q: char = '\\\\';\n+#[test]\n+fn real_test() {\n+ assert_eq!(Q as u32, 92);\n+}", + true, + ], + [ + "char literal with unicode escape then real test", + "@@ -1,0 +1,4 @@\n+const Q: char = '\\u{2764}';\n+#[test]\n+fn real_test() {\n+ assert_eq!(Q as u32, 10084);\n+}", + true, + ], + [ + "lifetime in type then real test", + "@@ -1,0 +1,3 @@\n+fn foo<'a>(x: &'a str) {}\n+#[test]\n+fn real_test() {}", + true, + ], + [ + "label then real test", + "@@ -1,0 +1,3 @@\n+'label: loop {}\n+#[test]\n+fn real_test() {}", + true, + ], + [ + "static lifetime then real test", + "@@ -1,0 +1,3 @@\n+const S: &'static str = \"hi\";\n+#[test]\n+fn real_test() {}", + true, + ], + [ + "context line with stray closing quote then real test", + '@@ -1,1 +1,2 @@\n );"\n+#[test]', + true, + ], + [ + "context line with stray opening quote then real test", + '@@ -1,1 +1,2 @@\n let s = "\n+#[test]', + true, + ], + [ + "r# inside string on context line then real test", + '@@ -1,1 +1,2 @@\n let s = "r#";\n+#[test]', + true, + ], + [ + "/* inside string on context line then real test", + '@@ -1,1 +1,2 @@\n let s = "/*";\n+#[test]', + true, + ], + // Known limitation: a non-code region opened by a context line is invisible + // to the pre-pass (context lines are not scanned). An added #[test] inside + // an existing raw string or block comment is scanned as code and may match. + // This is a false positive (silent direction), accepted because the old + // detector had the same behavior, the pattern is rare, and needs-tests is + // advisory. Scanning context lines to close this gap would re-introduce a + // false negative on the far more common case of a context line with a stray + // " from a string that opened before the visible window. + [ + "known limitation: #[test] inside context-opened raw string (FP)", + '@@ -1,1 +1,2 @@\n const FIX: &str = r#"\n+#[test]\n "#;', + true, + ], + [ + "known limitation: #[test_case] inside context-opened raw string (FP)", + '@@ -1,1 +1,3 @@\n const FIX: &str = r#"\n+#[test_case\n+(1)]\n "#;', + true, + ], + [ + "known limitation: #[test] inside context-opened block comment (FP)", + '@@ -1,1 +1,2 @@\n /*\n+#[test]\n */', + true, + ], +]; + +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, + ], + [ + "long malformed \\u{ escape run (quadratic guard)", + "@@ -1,0 +1,1 @@\n+" + "'\\u{".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`);