From 478c2f87ab61c12f0548041af1e65e054bea549d Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 6 Nov 2026 23:10:16 +0800 Subject: [PATCH] feat(ci): make flaky-patrol rerun and no_action decisions visible (#16459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(ci): make flaky-patrol rerun and no_action decisions visible The CI Failure Patrol posted only a hidden HTML marker when it re-ran a flaky failure or could not classify one, so maintainers saw a red PR with no explanation and clicked Update Branch — advancing the head, dismissing approvals, and re-running the whole CI for a failure that was already being handled on the same commit. Post a short bilingual notice alongside the marker for both decisions: the rerun notice states the transient cause and that no Update Branch is needed; the no_action notice asks for manual review before rerunning or updating the branch. The deterministic-failure comment is byte-identical, and the post-success reset marker stays hidden. Pin the visible notices, the marker round-trip through dedup and the per-head action count, and reason escaping for all three actions. * fix(ci): strip bidi overrides from flaky-patrol notice reasons An adversarial audit of the visible patrol notices found that bidi control characters (U+200E/U+200F, U+202A-U+202E, U+2066-U+2069) passed through the reason escaping untouched. The reason is classifier output derived from untrusted CI logs, and a right-to-left override can visually rewrite the very sentence maintainers trust when deciding whether to update a branch. Drop those characters in the shared escaping helper so all three visible decisions (and the deflake issue signature) are covered, and pin the stripping for each action. Removing the strip line fails the new test. * fix(ci): cover the Arabic letter mark in the patrol bidi strip An escalated audit round compared the strip class against the full Unicode Bidi_Control set and found U+061C (Arabic letter mark) missing; it can reorder adjacent Arabic-script text in a visible notice. Add it to the class and extend the per-action pin payload, so the class now matches the complete Bidi_Control set. Removing just U+061C fails the pin test. * fix(ci): collapse newlines in patrol reasons and pin the audit gaps The verification round found that a newline plus a line-start fence in a classifier reason opens a block-level code fence in the visible notice: the folded Chinese block and the tracking marker render as visible code. Collapse newlines and their surrounding whitespace to a single space in the shared escaping helper, keeping every reason single-line. Close the coverage gaps the same round measured: feed all twelve Bidi_Control code points individually, pin the English-before-fold and Chinese-inside-fold placement of the deterministic comment, and assert the fold's closing tag in all three notices. Witnesses: removing the collapse, narrowing the bidi class, dropping U+200F alone, swapping EN/ZH, or dropping the closing tag each fails the suite; a marked rendering probe shows the fence swallowing the fold before the fix and a clean render after; benign comment bodies remain byte-identical to base. --- .github/scripts/ci-flaky-rerun.mjs | 54 ++++++-- .qwen/skills/ci-flaky-patrol/SKILL.md | 2 +- scripts/tests/ci-flaky-rerun.test.js | 177 ++++++++++++++++++++++---- 3 files changed, 198 insertions(+), 35 deletions(-) diff --git a/.github/scripts/ci-flaky-rerun.mjs b/.github/scripts/ci-flaky-rerun.mjs index a2fbc64b727..df719cd5f03 100644 --- a/.github/scripts/ci-flaky-rerun.mjs +++ b/.github/scripts/ci-flaky-rerun.mjs @@ -428,12 +428,27 @@ export function eligibleAttemptJob(job, target, runAttempt, options = {}) { } function safeReason(reason) { - return reason - .trim() - .replaceAll('&', '&') - .replaceAll('<', '<') - .replaceAll('>', '>') - .replaceAll('@', '@\u200b'); + return ( + reason + .trim() + // A newline lets a reason open a block-level Markdown construct: a + // line-start fence swallows the folded Chinese block and renders the + // tracking marker as visible code. Keep the reason a single line. + .replace(/\s*\n\s*/g, ' ') + // Bidi overrides can visually rewrite a notice maintainers trust when + // deciding whether to update a branch; the reason is classifier output + // derived from untrusted logs, so drop them like the markup below. + // The class is the full Unicode Bidi_Control set. + .replace(/[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/g, '') + .replaceAll('&', '&') + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('@', '@\u200b') + ); +} + +function bilingualComment(en, zh, marker) { + return `${en}\n\n
\n中文说明\n\n${zh}\n\n
\n\n${marker}`; } export async function actOnDecision(client, target, decision) { @@ -474,7 +489,17 @@ export async function actOnDecision(client, target, decision) { const marker = markerFor(target, decision.action, count); if (decision.action === 'rerun') { await client.rerunFailedJobs(target.runId); - await client.comment(target.prNumber, marker); + // The notice must be visible: a marker-only comment left maintainers + // unable to tell a patrolled flake from a deterministic failure, so they + // clicked Update Branch — advancing the head and dismissing approvals. + await client.comment( + target.prNumber, + bilingualComment( + `CI Failure Patrol: this failure looks transient, not a PR regression: ${safeReason(decision.reason_en)}\n\nFailed jobs were re-run on the same commit. No Update Branch is needed — if the rerun passes, the failure was a flake.`, + `CI 失败巡逻：此失败疑似瞬时问题，而非 PR 引入的回归：${safeReason(decision.reason_zh)}\n\n已在同一 commit 上重跑失败的 job，无需 Update Branch；重跑通过即为 flake。`, + marker, + ), + ); // A flaky TEST (not infra) also gets a deflake issue so the autofix loop // can stabilize it. Best-effort: the rerun + marker already succeeded, so a // transient issue-creation failure must not propagate (it would surface as @@ -490,10 +515,21 @@ export async function actOnDecision(client, target, decision) { } else if (decision.action === 'comment') { await client.comment( target.prNumber, - `CI is failing because: ${safeReason(decision.reason_en)}\n\n
\n中文说明\n\n${safeReason(decision.reason_zh)}\n\n
\n\n${marker}`, + bilingualComment( + `CI is failing because: ${safeReason(decision.reason_en)}`, + safeReason(decision.reason_zh), + marker, + ), ); } else { - await client.comment(target.prNumber, marker); + await client.comment( + target.prNumber, + bilingualComment( + `CI Failure Patrol: this failure could not be confidently classified: ${safeReason(decision.reason_en)}\n\nNo automatic action was taken. Inspect the failed job before rerunning or updating the branch.`, + `CI 失败巡逻：无法确信归类此失败：${safeReason(decision.reason_zh)}\n\n未采取自动动作。请先人工查看失败 job，再决定重跑或更新分支。`, + marker, + ), + ); } } diff --git a/.qwen/skills/ci-flaky-patrol/SKILL.md b/.qwen/skills/ci-flaky-patrol/SKILL.md index 07d3f18fefc..6c6f6879eae 100644 --- a/.qwen/skills/ci-flaky-patrol/SKILL.md +++ b/.qwen/skills/ci-flaky-patrol/SKILL.md @@ -11,7 +11,7 @@ For every candidate, choose exactly one action: - `rerun`: concrete transient evidence such as a runner/network timeout, interrupted infrastructure, transient install/download failure, or explicit flaky-test evidence. - `comment`: the failure is clearly caused by the PR. Compare the failure with `changedFiles`; the reason must state the causal evidence, not merely that the failure is deterministic. -- `no_action`: evidence is ambiguous, unsafe, incomplete, or does not justify another action. This still records an internal tracking marker on the PR. +- `no_action`: evidence is ambiguous, unsafe, incomplete, or does not justify another action. This still records a tracking marker and a short manual-review notice on the PR. When (and ONLY when) the `rerun` cause is a nondeterministic **TEST** — a specific named test that timed out, is order-dependent, or depends on wall-clock/randomness — also identify it so the loop can open a deflake fix. Add a `flakyTest` object with the exact failing `file` (repo-relative path) and `name` (the full test title, e.g. `describe › it`) taken verbatim from the log. Emit `flakyTest` ONLY for genuine test nondeterminism, NEVER for infra flakiness (ENOSPC, network, runner death, dependency download) — those get a plain `rerun` with no `flakyTest`. If the log does not name a specific test, omit `flakyTest`. Keep `file` and `name` each at most 200 characters (take the test title verbatim; if a nested `describe › it` chain is longer, keep the most specific tail). A malformed or over-length `flakyTest` is simply ignored — the rerun still happens — so never drop a valid rerun over it. diff --git a/scripts/tests/ci-flaky-rerun.test.js b/scripts/tests/ci-flaky-rerun.test.js index e6697b733ea..27765f326f1 100644 --- a/scripts/tests/ci-flaky-rerun.test.js +++ b/scripts/tests/ci-flaky-rerun.test.js @@ -545,13 +545,35 @@ describe('ci flaky rerun patrol', () => { expect(kinds).toContain('comment'); }); - it('reruns before recording the hidden action marker', async () => { + it('reruns before posting the visible rerun notice', async () => { const c = client(); await actOnDecision(c, target(), decision()); - expect(c.calls).toEqual([ - ['rerunFailedJobs', 123], - ['comment', 42, expect.stringMatching(/^$/)], + expect(c.calls.map((call) => call[0])).toEqual([ + 'rerunFailedJobs', + 'comment', ]); + const body = c.calls[1][2]; + expect(body).toContain('runner timeout'); + expect(body).toContain('re-run on the same commit'); + expect(body).toContain('No Update Branch'); + expect(body).toContain('中文说明'); + expect(body).toContain('runner 超时'); + expect(body.split('').length - 1).toBe(1); + expect(body).toMatch(/$/); + }); + + it('keeps markers parseable inside the visible notices', async () => { + // The visible text must not break dedup or the per-head action count: + // both read the marker back out of the posted comment body. + const c = client(); + await actOnDecision(c, target(), decision()); + const posted = { + body: c.calls.find((call) => call[0] === 'comment')[2], + createdAt: '2026-07-12T07:25:00.000Z', + author: { login: 'patrol-bot' }, + }; + expect(alreadyHandled([posted], target(), 'patrol-bot')).toBe(true); + expect(currentActionCount(pr(), [posted], 'patrol-bot')).toBe(1); }); it('posts deterministic failures in English with folded Chinese', async () => { @@ -566,22 +588,36 @@ describe('ci flaky rerun patrol', () => { }), ); expect(c.calls).toHaveLength(1); - expect(c.calls[0][2]).toContain('TypeScript cannot resolve'); - expect(c.calls[0][2]).toContain('
'); - expect(c.calls[0][2]).toContain('TypeScript 无法解析'); - expect(c.calls[0][2]).toContain('action=comment'); + const body = c.calls[0][2]; + expect(body).toContain('TypeScript cannot resolve'); + expect(body).toContain('
'); + expect(body).toContain('TypeScript 无法解析'); + expect(body).toContain('action=comment'); + // Pin the bilingual placement, not just the presence of both strings: + // English before the fold, Chinese inside it. + const enIndex = body.indexOf('TypeScript cannot resolve'); + const foldIndex = body.indexOf('中文说明'); + const zhIndex = body.indexOf('TypeScript 无法解析'); + expect(enIndex).toBeLessThan(foldIndex); + expect(foldIndex).toBeLessThan(zhIndex); + expect(body.split('
').length - 1).toBe(1); }); - it('records no_action without a visible comment', async () => { + it('records no_action with a visible manual-review notice', async () => { const c = client(); await actOnDecision( c, target(), decision({ action: 'no_action', confidence: 'low' }), ); - expect(c.calls).toEqual([ - ['comment', 42, expect.stringMatching(/^$/)], - ]); + expect(c.calls).toHaveLength(1); + const body = c.calls[0][2]; + expect(body).toContain('could not be confidently classified'); + expect(body).toContain('runner timeout'); + expect(body).toContain('中文说明'); + expect(body).toContain('未采取自动动作'); + expect(body.split('
').length - 1).toBe(1); + expect(body).toMatch(/$/); }); it('rejects malformed, unsafe, or mismatched decisions', async () => { @@ -604,19 +640,110 @@ describe('ci flaky rerun patrol', () => { }); it('neutralizes mentions and markup copied into visible reasons', async () => { - const c = client(); - await actOnDecision( - c, - target(), - decision({ - action: 'comment', - reason_en: '@qwen-maintainers ', - reason_zh: '@全体成员 ', - }), - ); - expect(c.calls[0][2]).toContain('@\u200bqwen-maintainers'); - expect(c.calls[0][2]).toContain('<script>'); - expect(c.calls[0][2]).toContain('@\u200b全体成员 </details>'); + // All three actions now surface the reason in a visible comment, so the + // escaping must hold for every action, not just `comment`. + for (const action of ['rerun', 'comment', 'no_action']) { + const c = client(); + await actOnDecision( + c, + target(), + decision({ + action, + confidence: action === 'no_action' ? 'low' : 'high', + reason_en: '@qwen-maintainers ', + reason_zh: '@全体成员 ', + }), + ); + const body = c.calls.find((call) => call[0] === 'comment')[2]; + expect(body).toContain('@\u200bqwen-maintainers'); + expect(body).toContain('<script>'); + expect(body).toContain('@\u200b全体成员 </details>'); + } + }); + + it('strips bidi overrides that could visually rewrite a notice', async () => { + // The reason is classifier output derived from untrusted logs; a bidi + // override could make "no Update Branch needed" read as its opposite. + // Every member of the Unicode Bidi_Control set is also fed individually + // so narrowing the strip class cannot survive this pin. + const bidiControls = [ + '\u061C', + '\u200E', + '\u200F', + '\u202A', + '\u202B', + '\u202C', + '\u202D', + '\u202E', + '\u2066', + '\u2067', + '\u2068', + '\u2069', + ]; + for (const action of ['rerun', 'comment', 'no_action']) { + const confidence = action === 'no_action' ? 'low' : 'high'; + const c = client(); + await actOnDecision( + c, + target(), + decision({ + action, + confidence, + reason_en: 'safe \u202Ereversed\u202C text', + reason_zh: '安全 \u2066隔离\u2069 \u061C文本', + }), + ); + const body = c.calls.find((call) => call[0] === 'comment')[2]; + expect(body).not.toMatch( + /[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/, + ); + expect(body).toContain('safe reversed text'); + expect(body).toContain('安全 隔离 文本'); + for (const ch of bidiControls) { + const cc = client(); + await actOnDecision( + cc, + target(), + decision({ + action, + confidence, + reason_en: `a${ch}b`, + reason_zh: `a${ch}b`, + }), + ); + const single = cc.calls.find((call) => call[0] === 'comment')[2]; + expect(single).not.toContain(ch); + expect(single).toContain('ab'); + } + } + }); + + it('collapses newlines so a reason cannot open a block-level fence', async () => { + // A line-start fence in a reason swallows the folded Chinese block and + // renders the tracking marker as visible code; keep reasons single-line. + for (const action of ['rerun', 'comment', 'no_action']) { + const c = client(); + await actOnDecision( + c, + target(), + decision({ + action, + confidence: action === 'no_action' ? 'low' : 'high', + reason_en: 'npm error:\n```\nnpm error code ETIMEDOUT', + reason_zh: 'npm 错误：\n```\nnpm error code ETIMEDOUT', + }), + ); + const body = c.calls.find((call) => call[0] === 'comment')[2]; + expect(body).not.toMatch(/^```/m); + expect(body).toContain('npm error: ``` npm error code ETIMEDOUT'); + expect(body.split('').length - 1).toBe(1); + const posted = { + body, + createdAt: '2026-07-12T07:25:00.000Z', + author: { login: 'patrol-bot' }, + }; + expect(alreadyHandled([posted], target(), 'patrol-bot')).toBe(true); + } }); it('stops after three actions on the same head', async () => {