Skip to content

Commit 2259754

Browse files
kevincodex1kevincodex1
committed
ci(triage): scan attribute closers token-aware, bound to adjoined added lines
The two-regex fallback matched a path-only attribute line and a ]/( line anywhere else in the patch, so unrelated records could complete each other and silently clear needs-tests; the mandatory separator regex also dropped legal //-to-newline and nested block comments, relabeling real tests. Replace both with one forward token scan that starts on the attribute's added line, continues only through immediately adjoined added lines under a hard bound, and answers 'no inline test' on anything uncertain. Fence the detector for extraction and add scripts/test-pr-triage-detect.mjs, run by a new pr-checks job, since pull_request_target executes the target branch's workflow and can never exercise the proposed body.
1 parent 1105320 commit 2259754

3 files changed

Lines changed: 271 additions & 14 deletions

File tree

‎.github/workflows/pr-checks.yml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,22 @@ jobs:
6868
- name: Test release tag resolver
6969
run: scripts/test-resolve-release-tag.sh
7070

71+
triage-detector:
72+
name: triage detector matrix
73+
runs-on: ubuntu-latest
74+
timeout-minutes: 5
75+
steps:
76+
- name: Check out repository
77+
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
78+
with:
79+
persist-credentials: false
80+
81+
# pr-triage.yml runs on pull_request_target and never checks out PR
82+
# code, so its inline detector can only be exercised here, where the
83+
# proposed workflow body IS the checked-out one.
84+
- name: Test inline-test detector
85+
run: node scripts/test-pr-triage-detect.mjs
86+
7187
test:
7288
name: test (${{ matrix.toolchain }})
7389
runs-on: ubuntu-latest

‎.github/workflows/pr-triage.yml‎

Lines changed: 83 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -91,22 +91,91 @@ jobs:
9191
// ambiguous and backtracks exponentially: on this workflow's pull_request_target
9292
// trigger f.patch is fork-controlled, so `#[test` followed by a long `_a` run is
9393
// a permissionless stall of the triage job.
94-
// Rust allows whitespace, line breaks, and block comments between the
95-
// attribute path and ] or (. Match those token separators on one added
96-
// line, or a path-only added line followed by a ]/( line.
94+
// Rust allows whitespace, line breaks, and comments between the
95+
// attribute path and ] or (. A flat regex cannot decide that
96+
// boundary: `//` runs to end of line, and block comments NEST, which
97+
// is beyond regular languages — every enumeration of comment
98+
// spellings so far has either dropped a legal separator (regressing
99+
// real tests into needs-tests) or let unrelated patch records
100+
// complete each other (clearing needs-tests from a testless PR, the
101+
// invisible direction). So the closer is found by a token-aware
102+
// scan instead: it consumes horizontal whitespace, `//`-to-newline,
103+
// and depth-counted `/* */` runs, and accepts only when the first
104+
// real token after the attribute path is `]` or `(`.
105+
//
106+
// Association is positional: the scan starts on the attribute's own
107+
// added line and may continue ONLY through the immediately following
108+
// added lines (a context line, hunk header, or removal breaks the
109+
// chain). Unrelated `]`/`(` lines elsewhere in the patch can never
110+
// complete a path they do not adjoin. The continuation is bounded;
111+
// past the bound the scan gives up and reports NO inline test, which
112+
// fails toward the loud label rather than the silent clearance.
113+
//
114+
// The detector body is fenced by the TRIAGE_DETECTOR markers so
115+
// scripts/test-pr-triage-detect.mjs can extract and run it against
116+
// the committed case matrix. Keep the fenced block self-contained.
117+
// TRIAGE_DETECTOR_BEGIN
97118
const TEST_ATTR_PATH =
98119
"(?:cfg[ \\t]*\\([ \\t]*test[ \\t]*\\)|(?:::[ \\t]*)?(?:[\\w-]+[ \\t]*::[ \\t]*)*(?:r#)?(?:[A-Za-z0-9]+_)*test(?:_[A-Za-z0-9]+)*)";
99-
const TEST_ATTR_SEP =
100-
"(?:[ \\t\\n]|/\\*[^*]*\\*+(?:[^/*][^*]*\\*+)*/)*";
101-
const addsInlineTest = files.some(f => {
102-
if (!f.filename.endsWith(".rs") || !f.patch) return false;
103-
const p = f.patch;
104-
return (
105-
new RegExp("^\\+[ \\t]*#\\[[ \\t]*" + TEST_ATTR_PATH + TEST_ATTR_SEP + "[\\]\\(]", "m").test(p) ||
106-
(new RegExp("^\\+[ \\t]*#\\[[ \\t]*" + TEST_ATTR_PATH + TEST_ATTR_SEP + "$", "m").test(p) &&
107-
/^\+[ \t]*[\]\(]/m.test(p))
108-
);
109-
});
120+
// Anchored to the start of one added line; group 1 is whatever
121+
// follows the path on that line, handed to the separator scan.
122+
const TEST_ATTR_LINE = new RegExp(
123+
"^[ \\t]*#\\[[ \\t]*" + TEST_ATTR_PATH + "([\\s\\S]*)$"
124+
);
125+
// An attribute split across more added lines than this is not a
126+
// spelling anyone writes; refusing to scan further keeps the walk
127+
// linear in the patch and fails toward the loud label.
128+
const MAX_ATTR_CONTINUATION_LINES = 16;
129+
// segs[0] is the remainder of the attribute's own line; each later
130+
// entry is the text of one immediately following added line. True
131+
// only when the first non-separator token across them is ] or (.
132+
// Single forward pass, no backtracking: fork-controlled input.
133+
function attrCloserFollows(segs) {
134+
let depth = 0; // open block-comment nesting carried across lines
135+
for (const seg of segs) {
136+
let i = 0;
137+
while (i < seg.length) {
138+
if (depth > 0) {
139+
if (seg.startsWith("/*", i)) { depth += 1; i += 2; }
140+
else if (seg.startsWith("*/", i)) { depth -= 1; i += 2; }
141+
else { i += 1; }
142+
continue;
143+
}
144+
const c = seg[i];
145+
if (c === " " || c === "\t") { i += 1; continue; }
146+
if (seg.startsWith("//", i)) break; // comment to end of line
147+
if (seg.startsWith("/*", i)) { depth = 1; i += 2; continue; }
148+
return c === "]" || c === "(";
149+
}
150+
// Line exhausted inside separators; the newline is itself a
151+
// separator, so continue on the next added line.
152+
}
153+
return false; // out of adjoined lines (or bound hit): stay loud
154+
}
155+
function patchAddsInlineTest(patch) {
156+
const lines = patch.split("\n");
157+
for (let i = 0; i < lines.length; i++) {
158+
if (lines[i][0] !== "+") continue;
159+
const m = TEST_ATTR_LINE.exec(lines[i].slice(1));
160+
if (m === null) continue;
161+
const segs = [m[1]];
162+
for (
163+
let j = i + 1;
164+
j < lines.length &&
165+
lines[j][0] === "+" &&
166+
segs.length <= MAX_ATTR_CONTINUATION_LINES;
167+
j++
168+
) {
169+
segs.push(lines[j].slice(1));
170+
}
171+
if (attrCloserFollows(segs)) return true;
172+
}
173+
return false;
174+
}
175+
// TRIAGE_DETECTOR_END
176+
const addsInlineTest = files.some(
177+
f => f.filename.endsWith(".rs") && !!f.patch && patchAddsInlineTest(f.patch)
178+
);
110179
const touchedTests =
111180
names.some(n => n.includes("/tests/") || n.endsWith("_test.rs")) || addsInlineTest;
112181
if (changedRust && !touchedTests) want.add("needs-tests");

‎scripts/test-pr-triage-detect.mjs‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
#!/usr/bin/env node
2+
// Case matrix for the inline-test detector embedded in
3+
// .github/workflows/pr-triage.yml. The workflow runs on pull_request_target
4+
// and deliberately never checks out PR code, so the detector cannot be tested
5+
// where it runs; this script extracts the fenced TRIAGE_DETECTOR block from
6+
// the committed workflow body and exercises it here, where pr-checks.yml DOES
7+
// check out the proposed workflow. If the fence markers move or the block
8+
// stops being self-contained, this script fails loudly rather than testing a
9+
// stale copy.
10+
//
11+
// The matrix encodes the review contract for the detector
12+
// (Gitlawb/node#277): legal Rust separators between the attribute path and
13+
// its ]/( delimiter must be accepted (line comments, nested block comments,
14+
// splits onto immediately following added lines), while unrelated patch
15+
// records — delimiter-looking lines before the path, later in the hunk, or in
16+
// another hunk — must never complete a path they do not adjoin. False
17+
// negatives here SUPPRESS the needs-tests label silently, so every uncertain
18+
// path in the detector is required to answer "no inline test".
19+
20+
import { readFileSync } from "node:fs";
21+
import { dirname, join } from "node:path";
22+
import { fileURLToPath } from "node:url";
23+
24+
const repoRoot = join(dirname(fileURLToPath(import.meta.url)), "..");
25+
const workflow = readFileSync(
26+
join(repoRoot, ".github/workflows/pr-triage.yml"),
27+
"utf8"
28+
);
29+
30+
const BEGIN = "// TRIAGE_DETECTOR_BEGIN";
31+
const END = "// TRIAGE_DETECTOR_END";
32+
const begin = workflow.indexOf(BEGIN);
33+
const end = workflow.indexOf(END);
34+
if (begin === -1 || end === -1 || end <= begin) {
35+
console.error("FAIL: TRIAGE_DETECTOR fence not found in pr-triage.yml");
36+
process.exit(1);
37+
}
38+
const block = workflow.slice(begin + BEGIN.length, end);
39+
40+
let patchAddsInlineTest;
41+
try {
42+
const factory = new Function(`${block}\nreturn patchAddsInlineTest;`);
43+
patchAddsInlineTest = factory();
44+
} catch (err) {
45+
console.error(
46+
"FAIL: fenced detector block is not self-contained JavaScript:",
47+
err.message
48+
);
49+
process.exit(1);
50+
}
51+
52+
// Each patch is the `patch` field GitHub's listFiles API returns: hunk
53+
// headers plus +/-/space-prefixed lines, no ---/+++ file headers.
54+
const cases = [
55+
// ── Accepted spellings ────────────────────────────────────────────────
56+
["bare same-line", "@@ -1,0 +1,2 @@\n+#[test]\n+fn a() {}", true],
57+
["cfg(test)", "@@ -1,0 +1,1 @@\n+#[cfg(test)]", true],
58+
["indented with inner space", "@@ -1,0 +1,1 @@\n+ #[ test ]", true],
59+
[
60+
"namespaced with args",
61+
'@@ -1,0 +1,1 @@\n+#[tokio::test(flavor = "multi_thread")]',
62+
true,
63+
],
64+
["test_case harness", "@@ -1,0 +1,1 @@\n+#[test_case(1)]", true],
65+
["wasm_bindgen_test harness", "@@ -1,0 +1,1 @@\n+#[wasm_bindgen_test]", true],
66+
["raw identifier", "@@ -1,0 +1,1 @@\n+#[r#test]", true],
67+
[
68+
"line comment then closer on next added line",
69+
"@@ -1,0 +1,2 @@\n+#[test // rationale\n+]",
70+
true,
71+
],
72+
[
73+
"nested block comment, same line",
74+
"@@ -1,0 +1,1 @@\n+#[test /* outer /* inner */ outer */]",
75+
true,
76+
],
77+
[
78+
"block comment spanning added lines",
79+
"@@ -1,0 +1,3 @@\n+#[test /* why\n+ still why */ ]\n+fn a() {}",
80+
true,
81+
],
82+
[
83+
"path-only line, ( on the immediately following added line",
84+
"@@ -1,0 +1,2 @@\n+#[test_case\n+(1)]",
85+
true,
86+
],
87+
[
88+
"whitespace-only continuation before closer",
89+
"@@ -1,0 +1,3 @@\n+#[test\n+\t\n+]",
90+
true,
91+
],
92+
// ── Rejected spellings and adversarial shapes ─────────────────────────
93+
["rstest stays excluded", "@@ -1,0 +1,1 @@\n+#[rstest]", false],
94+
["substring #[testable]", "@@ -1,0 +1,1 @@\n+#[testable]", false],
95+
["substring #[contest]", "@@ -1,0 +1,1 @@\n+#[contest]", false],
96+
[
97+
"delimiter-looking line BEFORE the path",
98+
"@@ -1,0 +1,2 @@\n+(\n+#[test_case",
99+
false,
100+
],
101+
[
102+
"raw-string fixture path + unrelated ( later in the same hunk",
103+
'@@ -1,0 +1,5 @@\n+let s = r#"\n+#[test_case\n+not a separator token\n+"#;\n+let t = (1);',
104+
false,
105+
],
106+
[
107+
"path at end of one hunk, closer in another hunk",
108+
"@@ -1,0 +1,1 @@\n+#[test_case\n@@ -10,0 +11,1 @@\n+(1)]",
109+
false,
110+
],
111+
[
112+
"closer only on a context line",
113+
"@@ -1,1 +1,1 @@\n+#[test_case\n (1)]",
114+
false,
115+
],
116+
[
117+
"closer only on a removed line",
118+
"@@ -1,1 +1,1 @@\n+#[test_case\n-(1)]",
119+
false,
120+
],
121+
[
122+
"unfinished block comment never closes",
123+
"@@ -1,0 +1,2 @@\n+#[test /*\n+ still open",
124+
false,
125+
],
126+
[
127+
"continuation bound exceeded stays loud",
128+
"@@ -1,0 +1,40 @@\n+#[test /*\n" + "+ filler\n".repeat(30) + "+ */ ]",
129+
false,
130+
],
131+
];
132+
133+
let failures = 0;
134+
for (const [name, patch, expected] of cases) {
135+
const got = patchAddsInlineTest(patch);
136+
if (got !== expected) {
137+
failures += 1;
138+
console.error(`FAIL: ${name}: expected ${expected}, got ${got}`);
139+
}
140+
}
141+
142+
// Runtime probe: the detector walks fork-controlled input on
143+
// pull_request_target, so a pathological head must not stall the job. The
144+
// long `_a` run is the historical exponential-backtracking shape for the
145+
// attribute-path regex; the comment run exercises the scanner loop.
146+
const probes = [
147+
["long _a run", "@@ -1,0 +1,1 @@\n+#[test" + "_a".repeat(30000), false],
148+
[
149+
"long unclosed comment line",
150+
"@@ -1,0 +1,1 @@\n+#[test /*" + " *".repeat(30000),
151+
false,
152+
],
153+
];
154+
for (const [name, patch, expected] of probes) {
155+
const t0 = process.hrtime.bigint();
156+
const got = patchAddsInlineTest(patch);
157+
const ms = Number(process.hrtime.bigint() - t0) / 1e6;
158+
if (got !== expected) {
159+
failures += 1;
160+
console.error(`FAIL: probe ${name}: expected ${expected}, got ${got}`);
161+
}
162+
if (ms > 1000) {
163+
failures += 1;
164+
console.error(`FAIL: probe ${name}: took ${ms.toFixed(0)}ms (>1000ms)`);
165+
}
166+
}
167+
168+
if (failures) {
169+
console.error(`${failures} failure(s) across ${cases.length + probes.length} cases`);
170+
process.exit(1);
171+
}
172+
console.log(`ok: ${cases.length + probes.length} detector cases passed`);

0 commit comments

Comments
 (0)