From 254b8e8e1ac7f1bb54b2f0fe3fd745881bff5886 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 8 Aug 2026 14:44:02 +0800 Subject: [PATCH 1/4] fix(ci): render the queued-acknowledgement comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ack comment posted on every PR that requests a review was built as _Qwen Code review request accepted. …[workflow run](URL)._ with the prose glued straight onto the marker. A line opening with `_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 affc71a6c0b..c3342b08167 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2207,3 +2207,32 @@ describe('workflow expression length', () => { expect(runReviewStep()).not.toContain('${{'); }); }); + +describe('bot comment markers', () => { + // A line that opens with `/g; + let m; + while ((m = re.exec(workflow)) !== null) { + // Prose in a YAML or shell comment never reaches a comment body — and + // this rule is itself explained with an inline example of the bad form. + const lineStart = workflow.lastIndexOf('\n', m.index) + 1; + if (/^\s*#/.test(workflow.slice(lineStart, m.index))) continue; + const rest = workflow.slice(m.index + m[0].length); + // Fine: an escaped newline in the printf/format string that builds the + // body, or the marker ending the shell/jq string it was written into. + if (rest.startsWith('\\n') || /^["')\n]/.test(rest)) continue; + offenders.push(workflow.slice(m.index, m.index + 72).split('\n')[0]); + } + expect(offenders).toEqual([]); + }); +}); From aff47ff8d61487f4e641eba8b547009579bd085b Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 8 Aug 2026 19:58:34 +0000 Subject: [PATCH 2/4] fix(ci): harden the marker guard per review round 2 Pin the workflow-run URL weaving into the ack printf, bound the marker scan to the marker's physical line, anchor the newline exemption to the literal the marker opens, widen it to double-quoted printf formats, and flag unquoted command-substitution concatenation. Declare the remaining coverage gaps in the test instead of papering over them. Co-authored-by: Qwen-Coder --- scripts/tests/qwen-pr-review-workflow.test.js | 66 +++++++++++++++---- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index a313551b5be..da846006f8f 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2288,12 +2288,23 @@ describe('bot comment markers', () => { // what remains inside that literal after it. That is what distinguishes // BUILDING a comment body from merely REFERENCING a marker — `jq // contains("")` and `printf '' "$VAR"` leave nothing - // after the marker and are fine, while `="prose"` does not. + // 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 gap, stated rather than papered over: a body split across printf - // arguments (`printf '%s%s' '' 'prose'`) is NOT caught. Detecting - // it means modelling which literal is the format string, and every cheap - // approximation flagged the legitimate `printf '' "$VAR"` form. + // 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; and a line-wrapped printf whose + // format opens with the marker (the `\n` exemption reads one physical + // line). All are latent — no workflow uses these shapes today. const dir = '.github/workflows'; const files = readdirSync(dir).filter((f) => /\.ya?ml$/.test(f)); @@ -2317,25 +2328,37 @@ describe('bot comment markers', () => { // 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 end = rest.indexOf(quote); + 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 = rest.slice(0, end); + 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. - const after = rest.slice(end + 1); + // 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; - // A real newline separates them — but ONLY where the shell expands - // `\n`: printf's format string, or ANSI-C `$'…'`. In a plain - // double-quoted assignment `\n` is a literal backslash-n, so the - // prose stays on the marker's physical line and still breaks. - if (glued.startsWith('\\n') && /printf\s+'|\$'/.test(prefix)) 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+['"]$|\$'$/.test(prefix)) { + continue; + } offenders.push( `${file}: ${text.slice(m.index, m.index + 56).split('\n')[0]}`, ); @@ -2343,4 +2366,19 @@ describe('bot comment markers', () => { } 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. + const text = readFileSync(join(dir, 'qwen-code-pr-review.yml'), 'utf8'); + const ackLine = text + .split('\n') + .find( + (l) => l.includes('printf') && l.includes(''), + ); + expect(ackLine).toBeDefined(); + expect(ackLine).toContain('%s'); + expect(ackLine).toContain('"$RUN_URL"'); + }); }); From 798cb71e63cffa54c3956e773cef46815c122f4e Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 8 Aug 2026 23:10:11 +0000 Subject: [PATCH 3/4] fix(ci): widen marker-guard regex and declare known gaps per review --- scripts/tests/qwen-pr-review-workflow.test.js | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index da846006f8f..18ee18c1a70 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2302,9 +2302,24 @@ describe('bot comment markers', () => { // `--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; and a line-wrapped printf whose + // 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). All are latent — no workflow uses these shapes today. + // 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. const dir = '.github/workflows'; const files = readdirSync(dir).filter((f) => /\.ya?ml$/.test(f)); @@ -2313,11 +2328,12 @@ describe('bot comment markers', () => { const offenders = []; for (const file of files) { const text = readFileSync(join(dir, file), 'utf8'); - // `[^>\n]` and not `[^>]`: a `/g; + // 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; @@ -2356,7 +2372,10 @@ describe('bot comment markers', () => { // 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+['"]$|\$'$/.test(prefix)) { + if ( + glued.startsWith('\\n') && + /printf\s+(?:-\S+\s+(?:\S+\s+)?|--\s+)?['"]$|\$'$/.test(prefix) + ) { continue; } offenders.push( From 0259892a0071f33bc1dc83e855d6d7d558bccc59 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 9 Aug 2026 00:20:08 +0000 Subject: [PATCH 4/4] fix(ci): pin ack link shape and dedupe workflow scan per review --- scripts/tests/qwen-pr-review-workflow.test.js | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 18ee18c1a70..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; @@ -2320,14 +2322,12 @@ describe('bot comment markers', () => { // `.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. - const dir = '.github/workflows'; - const files = readdirSync(dir).filter((f) => /\.ya?ml$/.test(f)); it('never glues prose onto a comment marker, in any workflow', () => { - expect(files.length).toBeGreaterThan(0); + expect(workflowFiles.length).toBeGreaterThan(0); const offenders = []; - for (const file of files) { - const text = readFileSync(join(dir, file), 'utf8'); + for (const file of workflowFiles) { + const text = readFileSync(join(workflowsDir, file), 'utf8'); // The class excludes `\n` so a `'), ); expect(ackLine).toBeDefined(); - expect(ackLine).toContain('%s'); + expect(ackLine).toContain('[workflow run](%s)'); expect(ackLine).toContain('"$RUN_URL"'); }); });