diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 2c178a26da3..4d9f0e94f30 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -131,7 +131,12 @@ jobs: echo "PR #${PR_NUMBER} is ${PR_STATE}; skipping acknowledgement." >> "$GITHUB_STEP_SUMMARY" exit 0 fi - ACK_BODY="_Qwen Code review request accepted. Review is queued in [workflow run](${RUN_URL})._" + # Blank line after the marker, or none of the prose below renders. A + # line opening with `\n\n_Qwen Code review request accepted. Review is queued in [workflow run](%s)._' "$RUN_URL")" EXISTING_ACK_ID="$( # -F would otherwise make gh api default to POST. gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 52c8c7cb17c..6deb3b64303 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -26,6 +26,10 @@ const workflow = readFileSync( '.github/workflows/qwen-code-pr-review.yml', 'utf8', ); +const workflowsDir = '.github/workflows'; +const workflowFiles = readdirSync(workflowsDir).filter((f) => + /\.ya?ml$/.test(f), +); function runReviewStep() { const doc = parse(workflow); @@ -2175,14 +2179,12 @@ describe('workflow expression length', () => { // length 21000` (e.g. run 31239579253). CI stayed green the whole time — no // test covered this, which is why it is covered here. const LIMIT = 21000; - const dir = '.github/workflows'; - const files = readdirSync(dir).filter((f) => /\.ya?ml$/.test(f)); it('keeps every templated run block under the limit', () => { - expect(files.length).toBeGreaterThan(0); + expect(workflowFiles.length).toBeGreaterThan(0); const over = []; - for (const file of files) { - const doc = parse(readFileSync(join(dir, file), 'utf8')); + for (const file of workflowFiles) { + const doc = parse(readFileSync(join(workflowsDir, file), 'utf8')); for (const [jobId, job] of Object.entries(doc?.jobs ?? {})) { for (const step of job?.steps ?? []) { const body = step?.run; @@ -2274,3 +2276,129 @@ describe('command shape matching', () => { expect(stripCr).toBeGreaterThan(firstLine); }); }); + +describe('bot comment markers', () => { + // A line that opens with `")` and `printf '' "$VAR"` leave nothing + // after the marker and are fine, while `="prose"` does not. The + // scan is bounded to the marker's physical line: a glued body is by + // definition on that line, and searching past it would couple the guard to + // unrelated quotes elsewhere in the file — a doc example quoting an unclosed + // `--body "` would break the moment any later line gained a `"`. + // + // Known gaps, stated rather than papered over: bodies split across printf + // arguments (`printf '%s%s' '' 'prose'`) — detecting them means + // modelling which literal is the format string, and every cheap + // approximation flagged the legitimate `printf '' "$VAR"` form; + // bodies assembled across statements or files (one `echo` per line into a + // `--body-file`); markers at physical line start (heredocs, YAML block + // scalars); markers mid-literal that only land at a rendered line start + // after `\n` expansion (`printf 'x\nprose'`); multi-line literals + // whose closing quote sits on a later line; a line-wrapped printf whose + // format opens with the marker (the `\n` exemption reads one physical + // line); continuations after a marker-ending literal other than an + // adjacent quoted literal or bare `$(…)` — `$VAR` expansion, unquoted + // words, `$'…'` literals, backtick substitution, backslash-newline, and + // text after the closing `)` of a wrapping subshell assignment + // (`BODY="$(printf '')prose"`); closing that shape needs a + // subshell discriminator that false-positives on legitimate jq + // `contains(""))` references inside `$(…)` assignments; + // trailing end-of-line comments — the `#` skip only fires when the + // comment OPENS the physical line, so a glued marker quoted in a + // trailing comment is still flagged; YAML double-quoted scalars + // (`body: "\nprose"`) — YAML expands `\n`, but the scanner + // cannot cheaply tell a YAML scalar from a shell literal where `\n` + // stays literal; and marker-headed bodies built outside + // `.github/workflows` — the `.github/scripts/*.mjs` comment builders + // (template literals and pushed marker lines) are not scanned. All are + // latent — nothing glues a marker today. + + it('never glues prose onto a comment marker, in any workflow', () => { + expect(workflowFiles.length).toBeGreaterThan(0); + const offenders = []; + for (const file of workflowFiles) { + const text = readFileSync(join(workflowsDir, file), 'utf8'); + // The class excludes `\n` so a `` on the line) so arrow-style markers are covered too. + const re = //g; + let m; + while ((m = re.exec(text)) !== null) { + const lineStart = text.lastIndexOf('\n', m.index) + 1; + const prefix = text.slice(lineStart, m.index); + // Prose in a YAML or shell comment never reaches a comment body. + if (/^\s*#/.test(prefix)) continue; + const quote = m.index === lineStart ? null : text[m.index - 1]; + // Only a marker that OPENS a string literal can be building a body. + if (quote !== "'" && quote !== '"') continue; + const rest = text.slice(m.index + m[0].length); + const lineEnd = rest.indexOf('\n'); + const line = lineEnd === -1 ? rest : rest.slice(0, lineEnd); + const end = line.indexOf(quote); + // No closing quote on this line means either the marker ends the line + // inside a multi-line literal (a real newline separates the body, + // which renders) or the quote is prose in a doc example — neither + // glues anything ON the marker's line. + if (end === -1) continue; + let glued = line.slice(0, end); + if (glued === '') { + // The literal ended at the marker — but an adjacent literal on the + // same line concatenates onto it at runtime, and so does an + // unquoted `$(…)` (its output is invisible to this scan). + const after = line.slice(end + 1); + if (after[0] === "'" || after[0] === '"') { + const q2 = after[0]; + const e2 = after.slice(1).indexOf(q2); + glued = e2 === -1 ? '' : after.slice(1, 1 + e2); + } else if (after[0] === '$' && after[1] === '(') { + glued = after; + } + } + if (glued === '') continue; + // The two-character `\n` escape separates only where the shell + // expands it: a printf format string or ANSI-C `$'…'` opened at the + // END of the prefix — an unrelated printf earlier on the same line + // must not bless a plain double-quoted assignment, where `\n` stays + // a literal backslash-n and the prose stays on the marker's line. + if ( + glued.startsWith('\\n') && + /printf\s+(?:-\S+\s+(?:\S+\s+)?|--\s+)?['"]$|\$'$/.test(prefix) + ) { + continue; + } + offenders.push( + `${file}: ${text.slice(m.index, m.index + 56).split('\n')[0]}`, + ); + } + } + expect(offenders).toEqual([]); + }); + + it('pins the workflow-run URL into the ack printf', () => { + // bash printf with a leftover argument and no conversion spec exits 0 + // under `set -euo pipefail` and emits `[workflow run]()`, so nothing + // else catches a dropped `%s` or `"$RUN_URL"` on the ack line. Assert + // the link shape, not bare co-existence: a `%s` displaced out of the + // parens keeps both pieces on the line and re-ships the dead link. + const ackLine = workflow + .split('\n') + .find( + (l) => l.includes('printf') && l.includes(''), + ); + expect(ackLine).toBeDefined(); + expect(ackLine).toContain('[workflow run](%s)'); + expect(ackLine).toContain('"$RUN_URL"'); + }); +});