Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions .github/scripts/ci-flaky-rerun.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const DEFAULT_ACTIVE_DAYS = 7;
const DEFAULT_MAX_CANDIDATES = 5;
const MAX_ACTIONS = 3;
const ACTIONS = new Set(['rerun', 'comment', 'no_action']);
const DEFLAKE_MARKER = 'qwen-deflake';
const DEFLAKE_LABELS = ['status/ready-for-agent', 'autofix/approved'];
const DEFLAKE_MAX_CHARS = 200;

function timeMs(value) {
const ms = Date.parse(value ?? '');
Expand Down Expand Up @@ -306,6 +309,88 @@ function validDecision(target, decision) {
);
}

function wellFormedFlakyTest(ft) {
return (
!!ft &&
typeof ft === 'object' &&
typeof ft.file === 'string' &&
ft.file.trim().length > 0 &&
ft.file.length <= DEFLAKE_MAX_CHARS &&
typeof ft.name === 'string' &&
ft.name.trim().length > 0 &&
ft.name.length <= DEFLAKE_MAX_CHARS
);
}

// A backtick cannot be escaped INSIDE an inline code span — it closes the span
// and lets the remainder render as live Markdown (mention/markup injection via
// a test path or name). Drop backticks so the value stays inert in its span.
function codeSpanSafe(value) {
return String(value).replaceAll('`', "'");
}

export function deflakeKey(flakyTest) {
return createHash('sha256')
.update(`${flakyTest.file}\u0000${flakyTest.name}`)
.digest('hex')
.slice(0, 16);
}

export function deflakeMarker(flakyTest) {
return `<!-- ${DEFLAKE_MARKER} key=${deflakeKey(flakyTest)} -->`;
}

export function deflakeIssueBody(flakyTest, decision, target, repo) {
const marker = deflakeMarker(flakyTest);
const runUrl = `https://github.com/${
repo ?? target?.repo ?? 'QwenLM/qwen-code'
}/actions/runs/${target?.runId ?? ''}`;
Comment thread
wenshao marked this conversation as resolved.
const file = codeSpanSafe(flakyTest.file);
const name = codeSpanSafe(flakyTest.name);
return [
`CI Failure Patrol flagged this test as flaky and triggered a rerun on the same commit. If it passes on rerun it is nondeterministic; if it fails again deterministically it is a real bug, not flakiness — in that case do NOT stabilize it.`,
``,
`- **Test file:** \`${file}\``,
`- **Test name:** \`${name}\``,
`- **Observed on:** #${target?.prNumber ?? '?'} (${runUrl})`,
`- **Signature:** ${safeReason(decision?.reason_en ?? 'flaky test')}`,
``,
`Fix it with \`.qwen/skills/deflake/SKILL.md\` — a minimal, assertion-preserving stabilization. Never weaken, skip, or delete the assertion.`,
``,
`<details>`,
`<summary>中文说明</summary>`,
``,
`CI Failure Patrol 判定此测试疑似 flaky 并在同一 commit 上触发了重跑。重跑通过则为非确定性;若确定性再次失败则是真 bug 而非 flaky —— 那种情况不要稳化它。`,
``,
`- **测试文件:** \`${file}\``,
`- **用例名:** \`${name}\``,
`- **出现于:** #${target?.prNumber ?? '?'}(${runUrl})`,
`- **签名:** ${safeReason(decision?.reason_zh ?? 'flaky 测试')}`,
``,
`请依据 \`.qwen/skills/deflake/SKILL.md\` 做最小、保留断言的稳化修复,绝不弱化、跳过或删除断言。`,
``,
`</details>`,
``,
marker,
].join('\n');
}

export async function ensureDeflakeIssue(client, target, decision) {
if (!wellFormedFlakyTest(decision?.flakyTest)) return;
const marker = deflakeMarker(decision.flakyTest);
if (await client.hasOpenIssueWithMarker(marker)) return;
const title =
`deflake: ${decision.flakyTest.file} \u203a ${decision.flakyTest.name}`.slice(
0,
240,
);
Comment on lines +382 to +386

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The title is truncated with UTF-16 code-unit .slice(0, 240), which can split an astral-plane character (e.g. an emoji in a verbatim test name — the patrol SKILL says to take the name "verbatim from the log"). When such a character straddles code-unit index 240 of the composed string, the slice keeps the high surrogate and drops the low one; Node then encodes the lone surrogate as U+FFFD, so the created issue gets a garbled (replacement-character) title. — Concrete cost: a flaky test whose file is near the 200-char cap and whose name has an emoji straddling index 240 yields a deflake: … title ending in a replacement character instead of the real test name. Rare trigger, cosmetic harm — the dedup marker lives in the body (built from the un-sliced values), so dedup is unaffected. The same UTF-16 root cause also makes wellFormedFlakyTest's .length <= 200 reject a name that is ≤200 code points but >200 code units.

Suggested change
const title =
`deflake: ${decision.flakyTest.file} \u203a ${decision.flakyTest.name}`.slice(
0,
240,
);
const title = Array.from(
`deflake: ${decision.flakyTest.file} \u203a ${decision.flakyTest.name}`,
)
.slice(0, 240)
.join('');

— qwen3.8-max-preview via Qwen Code /review

await client.createIssue({
title,
body: deflakeIssueBody(decision.flakyTest, decision, target, client?.repo),
labels: DEFLAKE_LABELS,
});
}

function currentFailure(run, target) {
return (
run.status === 'completed' &&
Expand Down Expand Up @@ -387,6 +472,18 @@ export async function actOnDecision(client, target, decision) {
if (decision.action === 'rerun') {
await client.rerunFailedJobs(target.runId);
await client.comment(target.prNumber, 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
// a misleading "skipping PR" and, with the marker already posted,
// permanently suppress the deflake). Retried on the next flaky occurrence.
try {
await ensureDeflakeIssue(client, target, decision);
} catch (error) {
console.error(
`::warning::deflake issue creation failed for #${target.prNumber}; the rerun stands and it retries on the next flaky occurrence: ${error?.message ?? error}`,
);
}
} else if (decision.action === 'comment') {
await client.comment(
target.prNumber,
Expand Down Expand Up @@ -574,6 +671,39 @@ export class GhClient {
async jobLog(jobId) {
return this.gh(['api', `repos/${this.repo}/actions/jobs/${jobId}/logs`]);
}

async hasOpenIssueWithMarker(marker) {
const output = await this.gh([
'issue',
'list',
'--repo',
this.repo,
'--state',
'open',
'--search',
`${marker} in:body`,
Comment on lines +683 to +684

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] hasOpenIssueWithMarker interpolates the entire HTML-comment marker (<!-- qwen-deflake key=<hex> -->) into the in:body search, diverging from this file's proven dedup pattern — prsWithMarkers() searches the bare distinctive token (in:comments qwen-ci-flaky-rerun), not a full punctuation-laden HTML comment. — Concrete cost: GitHub's search tokenizer strips/splits punctuation tokens (<!--, -->, key=), so the implicit AND-of-terms can resolve to zero hits even when a matching issue exists; dedup then silently always returns false and every rerun of a still-flaky test opens a fresh status/ready-for-agent / autofix/approved issue (each dispatching the autofix pipeline → duplicate competing PRs). The current unit tests mock this method, so they cannot catch a query that never matches. Search bare distinctive tokens instead — mirroring prsWithMarkers — while keeping the per-test hex key for specificity, e.g. qwen-deflake <hexKey> in:body (both qwen-deflake and the hex key are substrings of the body's <!-- qwen-deflake key=<hexKey> --> marker).

— qwen3.8-max-preview via Qwen Code /review

'--json',
'number',
'--limit',
'1',
]);
return JSON.parse(output).length > 0;
}

async createIssue({ title, body, labels }) {
await this.gh([
'issue',
'create',
'--repo',
this.repo,
'--title',
title,
'--body',
body,
'--label',
labels.join(','),
Comment on lines +703 to +704

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The two new GhClient wrappers — hasOpenIssueWithMarker (the gh issue list --search … --limit 1 call + JSON.parse(output).length > 0 reduction) and createIssue (this --title/--body/--label labels.join(',') arg construction) — have no unit test. Every deflake test replaces them with the client() mock, so the real arg-building and output-parsing is never exercised. — Concrete cost: a wrong search qualifier (dedup silently always-false → a fresh deflake issue on every flaky rerun) or a labels.join(',') regression (deflake issues the autofix loop never picks up) would ship with all tests green. This file already unit-tests other GhClient methods by stubbing api.gh (comments, currentPr) — mirror that, e.g.:

const api = new GhClient('QwenLM/qwen-code');
let args;
api.gh = async (a) => { args = a; return '[]'; };
await api.hasOpenIssueWithMarker('<!-- qwen-deflake key=x -->');
expect(args[args.indexOf('--search') + 1]).toBe('<!-- qwen-deflake key=x --> in:body');
// plus a createIssue case asserting args has '--label' then 'status/ready-for-agent,autofix/approved'

— qwen3.8-max-preview via Qwen Code /review

]);
}
}

export function argsMap(argv) {
Expand Down
12 changes: 9 additions & 3 deletions .qwen/skills/ci-flaky-patrol/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ For every candidate, choose exactly one action:
- `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.

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.

Do not handle main-branch failures; they are outside this skill. The driver enforces a maximum of 3 actions per PR head and supplies the current `actionCount` only as context.

Write only `ci-flaky-decisions.json` with this exact top-level shape:
Expand All @@ -28,13 +30,17 @@ Write only `ci-flaky-decisions.json` with this exact top-level shape:
"failureKey": "check-0123456789abcdef",
"action": "rerun",
"confidence": "high",
"reason_en": "The runner timed out while downloading dependencies.",
"reason_zh": "运行器在下载依赖时超时。"
"reason_en": "shellAstParser test timed out at 5000ms under runner load.",
"reason_zh": "shellAstParser 测试在运行器负载下 5000ms 超时。",
"flakyTest": {
"file": "packages/core/src/utils/shell-ast-parser-lazy.test.ts",
"name": "shellAstParser lazy runtime › loads web-tree-sitter on first use"
}
}
]
}
```

Copy identity fields exactly from each candidate and return one decision per candidate. `action` must be `rerun`, `comment`, or `no_action`. Use `confidence: "high"` only when the evidence directly supports the action; use `confidence: "low"` with `no_action`. Keep each reason at most 200 characters.
Copy identity fields exactly from each candidate and return one decision per candidate. `action` must be `rerun`, `comment`, or `no_action`. Use `confidence: "high"` only when the evidence directly supports the action; use `confidence: "low"` with `no_action`. Keep each reason at most 200 characters. `flakyTest` is optional and only valid alongside `action: "rerun"` (see above); omit it entirely for infra reruns and for `comment`/`no_action`.

Do not call tools except `read_file` and `write_file`. Do not write any other file.
63 changes: 63 additions & 0 deletions .qwen/skills/deflake/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
name: deflake
description: Stabilize a flaky test with a minimal, assertion-preserving fix — never by weakening or deleting the check.
---

# Deflake a flaky test

A `deflake:` issue names ONE test that has been observed failing and then
passing on a rerun of the same commit — the definitive flaky signature. Your
job is to make that test deterministic **without changing what it verifies**.

The issue body carries the test identity (file + name) and the observed failure
signature (e.g. `Test timed out in 5000ms`, `Timed out waiting for …`, an
order-dependent assertion, a wall-clock/random-dependent value). Read the test,
reproduce the mechanism in your head, and apply the SMALLEST fix from the
allowed set below that removes the nondeterminism.

## The only allowed fixes

1. **Raise a timeout / poll budget.** A test that blows vitest's default under
CI contention (fully-mocked or I/O-bound, not a perf test) gets a generous
per-test `testTimeout` (3rd arg to `it`), or its internal poll loop is given
a real wall-clock budget instead of a fixed iteration count (a fixed count of
`setImmediate` turns elapses in milliseconds and races real I/O).
2. **Stabilize timing / waiting.** Replace a bare `setTimeout`/fixed `sleep`
with an explicit `await` of the real condition (`vi.waitFor`, a resolved
promise, an event). Pre-warm a lazy load (e.g. a WASM runtime) in
`beforeAll` so per-test time doesn't include first-load cost.
3. **Make randomness / time deterministic.** Seed the RNG, `vi.useFakeTimers()`
/ mock `Date.now`, or pin the input so a value that depends on the real clock
or `Math.random` can't drift.
4. **Isolate / serialize interference.** Give tests that collide on a shared
resource (a same-named tempdir, a fixed port, a global singleton) unique
per-test resources, or serialize them.

## Hard rules

- **Never** delete the test, `skip`/`todo` it, loosen an assertion, widen an
expected range, add a blanket `try/catch`, or add a retry wrapper around the
assertion. Those hide the flake instead of fixing it — and could hide a real
bug. If none of the four fixes applies, or the failure looks like a REAL
intermittent product bug (not test nondeterminism), write
`<workdir>/failure.md` explaining what you found and stop. A human deflakes it.
- Keep the diff minimal and local to the named test (and its file's helpers).
Do not refactor unrelated code.
- Preserve every assertion and every input exactly. A timeout bump changes only
the ceiling; a determinism fix changes only the source of nondeterminism.
- Prefer a per-test or per-file change over a global config change unless the
same class demonstrably spans the whole package (then a `testTimeout` in that
package's `vitest.config.ts` is acceptable, as it only raises the ceiling and
weakens no assertion).

## Verify

Run the named test's file several times (`npx vitest run <file>` in the right
package, repeated) — it must pass every time. Then run the standard verify gate
(build / typecheck / lint / the changed test). If you cannot make it pass
deterministically, write `<workdir>/failure.md` and stop.

Then follow `.qwen/skills/prepare-pr/SKILL.md` for the PR body and write the
bilingual `<workdir>/e2e-report.md` (per the Shared Rules) stating: the flaky
mechanism, which of the four fixes you applied and why, and the repeated-run
evidence that it is now deterministic.
Loading
Loading