* fix(ci): stop cancelling in-progress E2E runs on main
Merges land on main roughly every 18 minutes (median) while a full E2E run takes about 40, so cancelling the in-flight run on every push starved the suite: across the last 100 push runs, 67 were cancelled and only 25 ever reported a result. The nightly regression was the only reliable signal, and each cancelled run still burned roughly 10 minutes on three runners before dying.
Turning cancellation off for main hands the coalescing to GitHub's concurrency queue, which keeps at most one pending run per group and cancels the previously pending one. The queue therefore collapses to the newest tree on its own while the in-flight run always finishes, so every result covers the batch of commits merged since the last one — bisect that range when it goes red. Dev branches keep cancelling superseded runs, where only the latest push matters.
* fix(ci): dedupe main CI failure issues by failing test
The autofix issue for a red main was deduped on the commit SHA, so a standing failure opened a brand new issue on every merge: one broken E2E test on 2026-07-26 produced six duplicate issues in twelve hours, each pointing the autofix agent at an unrelated commit.
The failing tests are now identified from the logs of the failed jobs and used as the dedupe key, with one marker per test so a failure set that grows still matches the issue that already tracks part of it. Later commits hitting the same failure are appended to that issue as recurrences (bounded, newest first) instead of opening another one, and notes written by a human or the agent are preserved when the machine-owned trailer is refreshed. Runs with no identifiable test — an install or build break — keep the previous per-commit behaviour.
* fix(ci): keep the failure analysis out of the bot-PAT job
Identifying the failing tests means running a helper from the repository, and the workflow deliberately checked out nothing so that the job holding the bot PAT could never execute repository code — an invariant its own test enforces.
Rather than weaken it, the work is split: a read-only job checks out the tree, reads the failed run's logs, finds any issue that already tracks the failure and renders the title and body, handing both over as outputs. The job with the PAT keeps checking out nothing and only writes what it was given. The invariant test now asserts that separation — the PAT job runs no repository code and holds only issue write — instead of banning checkout everywhere in the file.
* fix(ci): harden main CI failure dedupe per review (#7795)
- Match the `[run <id>]` link text instead of the run URL when deduping
recurrences: `/301` is a substring of `/3010`, so the URL match silently
deleted an unrelated run's line.
- Rebuild the machine-owned "## Also failing" section from the live failure
set on every merge so a test that has since been fixed drops out instead of
being listed forever.
- Assert the analyze checkout is SHA-pinned with persist-credentials disabled
and pin the failed-job log-download paths in the workflow test.
- Add an e2e workflow test guarding the cancel-in-progress expression that
keeps in-progress runs on main from being cancelled.
* fix(ci): bound the issue body and randomize the heredoc delimiter (#7795)
* fix(ci): test the runCli --existing merge path (#7795)
* fix(ci): address review feedback on e2e signal PR (#7795)
- Assert the full cancel-in-progress expression including && so a
mutation to || is caught by the e2e-workflow test
- Filter the capped-summary line from missingTests so it is not
rendered as a fake bullet under Also failing
---------
Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com>
* feat(triage): add revert-pattern high-risk path detection
Replace the behavior-neutral PR filter (PR #7414 v1, ~2% hit rate) with a
data-backed triage gate based on revert-history analysis of 111 revert
commits and 46 unique reverted PRs in this repo.
Stage 1e checks three signals identified by the analysis:
- touches_high_risk (66.7% precision, 32.3% recall)
- contested-merge pattern (50.0% precision, 19.4% recall)
- non-maintainer + high-risk (58.3% precision, 22.6% recall)
The gate escalates review depth and recommends maintainer sign-off; it
never blocks or closes PRs. Design doc and analysis scripts included.
* fix(triage): avoid stale-exempt hold label
* fix(triage): address review risk detection feedback
* fix(triage): tighten high-risk path patterns
* fix(triage): address review feedback on Stage 1e revert-pattern gate (#7414)
* fix(triage): address round-2 review feedback on Stage 1e gate (#7414)
- Fix APPROVE → APPROVED state name (GitHub API enum)
- Use gh api --paginate for file listing (fixes 100-file truncation)
- Anchor shell/relaunch/sandbox patterns with (^|/) to avoid false positives
- Append || true to grep (exit 1 on no match is the 92% case)
- Scope E2E recommendation to write-access authors per Stage 2c
- Add bot author filter to contested-merge query
- Define core paths explicitly in contested-merge condition
- Wire Stage 1e do-not-auto-approve into Stage 3 guardrail
- Replace precision percentages with p-values/raw counts in skill text
- Add sampling caveat and statistical significance notes to design doc
- Fix design doc errors: 71%→61.5%, 10→8 PRs, Rule 3 attribution,
~20% baseline→10% prevalence, Area field, no_e2e inconsistency
- Make test assertions specific to Stage 1e (not vacuous)
- Add Risk: template field assertion
- Revert drive-by prettier reflow
- Note need-discussion label removal by maintainer
* fix(triage): address round-3 review feedback on Stage 1e gate (#7414)
- Separate gh api call from grep so API failures are visible instead of
being masked by || true (rc:3660753982)
- Include author identity in contested-merge jq output and require
different reviewers for the disagreement check, avoiding false
positives from same-reviewer iteration (rc:3660753990)
- Add Stage 1e to the approval summary checklist so it is not omitted
from the pre-approval conditions (rc:3660753994)
* fix(triage): address round-4 review feedback on Stage 1e gate (#7414)
* fix(triage): use portable ERE grep for test-file exclusion (#7414)
* fix(triage): guard deferred approval on discussion label
* fix(triage): keep only supported revert signal
---------
Co-authored-by: qwen-code-bot <qwen-code-bot@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
The 'Clean stale agent state' step strips non-allowlisted keys from the
persistent workspace's local git config through a
`git config --list | grep -ivE <allowlist> | while ...` pipeline. When
the config holds only allowlisted keys — the steady state on a reused
runner this step already sanitized, since actions/checkout's post step
removes its auth extraheader at job end — grep matches nothing and
exits 1. Under the default `bash -e` shell combined with the script's
`set -o pipefail`, that kills the step exactly when there is nothing
to clean, before any output, and every downstream triage step is
skipped (seen on run 30095456731, runner ecs-qwen-runner-sg-4).
Guard the grep with `|| true` so an empty match feeds an empty loop
instead of failing the job. Sanitization behavior is unchanged:
non-allowlisted keys (core.pager, include.path, `!`-aliases) are
still stripped.
The workflow test harness missed this because its allowlist test
re-assembles the pipeline without the step's shell flags and always
plants non-allowlisted keys first. Add a steady-state regression test
that runs the step's actual script under `bash -e` against a config
holding only allowlisted keys, plus a negative control that strips the
guard and demands the step die — red on the old workflow, green now.
Co-authored-by: verify <verify@local>
Guards the security-critical invariants of qwen-triage.yml that broke silently
once already — the `settings_json:` input name was wrong, so the action dropped
it and the review agent ran with the full default toolset and no deny list.
A new `node:test` suite (wired into the shared HELPER_TESTS list both CI paths
run) asserts: the `settings:` input name (never `settings_json:`), the
tools.core registration whitelist and the deny list, the fork-PR runner routing
invariants, and the git exec-vector cleanup.
It also flips that cleanup from a best-effort denylist — which kept missing new
families (pager, filter.*, includeIf subsections, url.*, credential…) — to a
keep-known-safe allowlist: unset every local config key that isn't plumbing
actions/checkout needs (repo format, remote, branch, fetch/gc/pack/index,
safe.directory, extensions, submodule url/active/branch — not submodule.*.update,
which can be `!cmd`). This closes the whole exec-vector class, including knobs
not yet enumerated. The harness runs the workflow's actual allowlist pattern
against a scratch repo to prove it unsets every exec family and preserves the
checkout plumbing.
Co-authored-by: verify <verify@local>
The web-shell visuals render runs every screenshot and flow in a single
`test:e2e:visuals`, and that step had no `continue-on-error`, while the compose
and upload steps had no `if: always()`. So one failing or timing-out scenario
failed the job, the artifact was never uploaded, and the publish workflow had
nothing to post — the entire preview vanished even when every other scenario
passed and its PNG was already on disk. A flow (a long multi-click sequence) is
the most fragile scenario kind, so the fragile one silently takes down the
deterministic screenshots. PR #7498 hit exactly this: 29 scenarios passed, one
new channel-management flow timed out, and the PR got no preview and no comment
at all.
Make the after-capture step `continue-on-error` so the passing captures survive
and the later steps still compose and upload them. The publish job only runs on
a `success` conclusion, so the job must stay green — but a masked failure must
not read as a clean preview. Ship the step's real `.outcome` (which
continue-on-error does NOT mask, unlike `.conclusion`) to the publisher as
`render-status.txt`, and have the comment builder use it: an empty preview whose
render failed says "one or more scenarios failed to render" and is explicitly
NOT the reassuring green check or the coverage-gap prompt (both imply the render
ran); a partial preview is labelled partial above the shots that did render. A
missing status file (older run) defaults to complete, so this only ever adds a
warning, never suppresses a real preview.
The failing scenario still needs fixing — it's now surfaced in the comment
rather than by silently deleting everyone else's preview.
Co-authored-by: wenshao <wenshao@example.com>
An empty visual preview means one of two opposite things: the change genuinely
moves no pixel, or no scenario renders the UI it touches. The bot printed the
same green check for both, so the second — a coverage gap, where the preview
literally cannot see the feature — read as a clean bill of health.
That has now happened three times (#7035 primary label, #7221 worktree badge,
#7365 empty-state toggle), each caught only because a maintainer noticed the
missing image and asked. The signal to tell them apart was already there and
unused: the render workflow only runs when the web-shell client or webui source
changed, so an empty preview is by construction "UI code changed, nothing
rendered differently".
When no view changed, look at which files the PR touched. If any are
render-shaping (.tsx / .css / .svg under the rendered surface, excluding test
and scenario code), list them and say the result is ambiguous, with a pointer
to where a scenario goes. Otherwise keep the green check — a logic-only PR with
no visual delta is expected, and prompting there would train everyone to ignore
the prompt when it matters. The path list comes from the PR files API in the
privileged publish job, which never checks out PR code; if that call fails the
comment falls back to the current wording.
Co-authored-by: wenshao <wenshao@example.com>
* fix(autofix): resolve owning package for nested paths; report verify-failed handoffs as not pushed
The verify gate mapped each changed file to a flat `packages/<dir>` and
read `<dir>/package.json`, which ENOENT-crashed on nested packages such
as packages/channels/base — the container packages/channels has no
package.json. Walk each changed file up to its nearest package.json in
both the issue-fix and review-address verify steps, and skip any
candidate that still has none.
When such a verify failure follows an agent commit, the review-address
handoff rendered the agent's optimistic address-summary.md (which can
cite a commit SHA) under a neutral "what I found" heading, so a
maintainer chased a commit that was discarded with the runner workspace.
An EXIT trap now records any post-commit non-zero exit as outcome=failed,
and the handoff states plainly that the change did NOT pass the gate and
was NOT pushed.
Tests: walk-up detection over a nested package tree, the outcome=failed
trap, and the not-pushed handoff wording — each mutation-verified.
* refactor(autofix): extract owning-package resolver to a shared staged script
Addresses review on #7330.
Extract the changed-file → owning-package walk into
.github/scripts/resolve-owning-packages.sh, staged to RUNNER_TEMP from the
trusted base alongside check-settings-schema.sh and invoked from both verify
gates, so the two gates cannot drift into resolving packages differently (the
8-line walk was otherwise duplicated verbatim in each). Updates the
package-scripts test that pinned the old inline grep.
Narrow the verify-failed handoff lead-in to "This change was NOT pushed": four
paths set outcome=failed BEFORE the deterministic gate runs (agent abort via
failure.md, dirty tree, unchanged branch, missing address-summary.md), so the
previous "did NOT pass the verification gate" claim was factually wrong for
them. The specific reason stays in the headline and the quoted summary.
* style(autofix): brace variable references in resolve-owning-packages.sh
The repo's shellcheck gate runs --enable=all --severity=style, under which
bare $f/$d references trip SC2250 (prefer ${var}). Brace them to match the
convention already used in check-settings-schema.sh, and update the script
content assertions accordingly. Verified with shellcheck 0.11.0 using the
exact CI flags: clean.
* fix(autofix): resolve owning workspace via npm query; key unpushed-handoff on commit existence
Addresses the deeper review on #7330.
Blocking issue: the "nearest package.json" resolver mapped a change under a
workspace's fixture/example package (e.g.
packages/cli/src/commands/extensions/examples/starter) to that fixture, whose
test script is not Vitest — silently SKIPPING packages/cli's own tests, a
coverage regression invisible in the log. Resolve against the authoritative
`npm query .workspace` set instead and take each file's longest-prefix
workspace: nested workspaces (packages/channels/base) match exactly, fixtures
and non-workspace paths (packages/sdk-python, packages/README.md, the excluded
packages/desktop) drop. Also harden the resolver against a final line with no
trailing newline and against an unmatched last line, which under
`set -o pipefail` would otherwise abort the script.
Handoff wording: keying "was NOT pushed / commit discarded" on outcome=failed
was wrong for the abort paths (failure.md, dirty tree, unchanged branch,
missing address-summary.md), which set outcome=failed before ever making a
commit. Record committed=true right after checkout — before any gate can fail
— and key the wording on that; the abort/no-op paths keep the neutral framing.
This removes the EXIT trap entirely (its only observable effect was that
wording), so it no longer mislabels pre-commit failures either.
* fix(autofix): expand workspaces on-disk so branch-added packages are tested; harden resolver
Addresses the re-review on #7330.
The resolver sourced its workspace set from `npm query .workspace`, which reads
node_modules — installed from the BASE checkout. A workspace the PR branch ADDS
(a new channel adapter, a new sdk — the issue-fix job's whole purpose) was
invisible, so its tests were silently skipped, and for a nested new package the
ENOENT crash this PR fixes turned into a silent skip. Expand the set from the
on-disk root package.json `workspaces` globs instead (shallow `dir/*` + literals,
honouring `!` negations, keeping dirs with a package.json): it reflects the
branch, matches what `npm run --workspace` accepts downstream, and needs no
install. Verified to reproduce `npm query`'s set exactly on the current tree.
Also from the review:
- Fail the gate loudly on an empty/unreadable workspace set instead of the
silent "no package changes" skip, and drop the now-unneeded `|| true` at both
resolver call sites (the resolver already exits 0 on legitimate no-match).
- Record committed=true at the TOP of the step (ref-only diff), covering an
agent that commits then aborts, and count only `git diff --quiet` exit 1 as a
commit (128 is a git error, not a discarded commit).
- Correct the two call-site comments that still described the superseded
nearest-package.json approach.
Also hardens the resolver against a final changed-path with no trailing newline
and an unmatched last line under `set -o pipefail`.
---------
Co-authored-by: wenshao <wenshao@example.com>
* feat(ci): auto-open a deflake fix issue for confirmed flaky tests
The CI Failure Patrol reruns flaky failures but never fixes them, so
the same tests flake forever on a rerun treadmill. This closes the
loop: when the patrol classifies a rerun as a nondeterministic TEST
(not infra), it now also opens ONE deflake issue that the existing
autofix issue pipeline develops into a reviewable stabilization PR.
- ci-flaky-patrol SKILL: a rerun decision whose cause is a specific
named flaky test carries an optional flakyTest {file, name}; infra
reruns (ENOSPC, network, runner death) never do.
- ci-flaky-rerun.mjs: validates flakyTest (malformed → the whole
decision is rejected, so a bad classification can't open a bogus
issue); after a rerun, ensureDeflakeIssue upserts a deflake issue
deduped by a stable (file, name) marker — one open issue per flaky
test across all PRs — labeled status/ready-for-agent + autofix/
approved so the scheduled autofix scan picks it up.
- .qwen/skills/deflake/SKILL.md: constrains the fix to four
assertion-preserving patterns (raise timeout/poll budget, stabilize
timing/waiting, make randomness/time deterministic, isolate
interference) and forbids skipping/deleting/loosening the check;
write failure.md if none applies or the failure looks like a real
bug. The produced PR is reviewable, never auto-merged.
Tests: deflakeKey stability/collision-freedom, the bilingual issue
body, one-issue-per-test dedup, no issue for infra reruns, and
malformed-flakyTest rejection. 34/34 across both patrol suites.
* fix(ci): deflake review hardening — rerun survives bad metadata, no markup injection
Addresses the two Criticals + suggestions on #7231:
- **Critical: a malformed/over-length flakyTest no longer kills the
rerun.** flakyTest validation is removed from validDecision (which
gated the PRIMARY action on secondary metadata — a >200-char nested
test name or a null silently dropped a valid rerun). Well-formedness
is now checked in ensureDeflakeIssue, which simply skips the deflake
issue when the metadata is bad; the rerun always stands.
- **Critical: markup/mention injection via the test path/name.** file
and name are code-span-stripped of backticks (which cannot be escaped
inside a span and would break out into live Markdown, turning
into a mention in a bot-created issue) and both now sit in
code spans. safeReason alone did not close this (it does not touch
backticks).
- Best-effort deflake: ensureDeflakeIssue is wrapped in try/catch so a
transient createIssue failure — after the marker is already posted —
no longer surfaces as a misleading "skipping PR" and permanently
suppresses the deflake; it retries on the next flaky occurrence.
- Run link uses the patrol's own repo (client.repo) instead of the dead
target.repo, so deflake issues on a fork don't 404.
- Body reworded: it no longer claims the rerun already passed (it runs
right after the rerun is triggered) — it says a real deterministic
failure is NOT flakiness and must not be stabilized.
- SKILL: bound file/name to 200 chars, and note a malformed one is
ignored (never drops the rerun).
Tests: malformed flakyTest keeps rerun (no createIssue); long title
truncates ≤240; backtick path/name cannot inject; run link honors the
repo; a throwing createIssue leaves the rerun intact. 38/38.
---------
Co-authored-by: wenshao <wenshao@example.com>
The before/after preview flagged text-heavy views (e.g. workspace-sidebar)
as ~0.1-0.3% "changed" on PRs that do not touch them — #7204 is a live
example (the two panels are pixel-for-pixel indistinguishable). Root cause:
base and head render in SEPARATE CI jobs, so Linux font anti-aliasing is
not bit-identical between them, and the naive per-pixel diff counts the
scatter of isolated / 1px-wide glyph-edge pixels that leaves. At the tight
0.02% threshold that scatter crosses the line.
Measure the changed fraction AFTER a cluster denoise: a differing pixel
counts only when at least 4 of its 8 neighbours also differ. AA scatter
(isolated = 0 neighbours, a 1px line = 2) erodes to ~zero, while a real
change — a badge, chip, icon, panel — is a solid block whose interior keeps
5-8 and easily clears the threshold, so the threshold stays tight without
raising it (which would miss small real changes like a workspace badge).
The browser now returns a compact bit-mask; the denoise + count run in node
against the unit-tested countDenoisedChanges, so there is one tested
implementation of the metric.
Co-authored-by: wenshao <wenshao@example.com>
* ci(autofix): teach the review loop about generated-artifact gates and stop silent stalls
The autofix bot stalled on PRs that edit settingsSchema.ts without
regenerating settings.schema.json (e.g. #6984): a CI-only freshness gate
it could neither run, see, nor recover from.
- Give the agent the tool and the instruction to regenerate: add
`npm run generate:settings-schema` to the develop-issue and address-review
coreTools allowlists, and a SKILL rule to regenerate + commit a source's
generated artifact.
- Mirror CI's "Check settings schema is up-to-date" step in both verify
gates so a stale artifact fails locally instead of red-on-CI after push.
- Inject the actual failing STEP name + a log excerpt into feedback.md so
the agent diagnoses from the real failure instead of guessing from local
test runs. SKILL now forbids "pre-existing"/environment excuses without
evidence.
- Decouple the feedback watermark from base-sync pushes: use the last eval
marker (what the agent evaluated), not the head commit date, so an
"Update branch" merge can no longer bury unaddressed maintainer feedback.
Use PR createdAt as the pre-first-eval floor.
- Bound the pending-check skip so a check wedged pending can't strand a PR
forever; always post a handoff comment + eval marker on failure so the
loop never goes silent; add an issue_comment trigger so an @-mention from
a trusted maintainer re-triggers the review pass promptly.
* ci(autofix): drop comment-trigger and raw-log injection; keep them within existing safety guards
Respects two deliberate, tested design decisions that the first cut collided
with (caught by scripts/tests/qwen-autofix-workflow.test.js):
- Drop the issue_comment @-mention trigger and its route branch: the workflow
intentionally does not expose comment-triggered autofix (only
pull_request_review:submitted) to avoid redundant runs and comment-command
surface. The scheduled scan plus the watermark fix already re-target a PR
after maintainer feedback.
- Drop the raw CI-log injection into feedback.md: feedback fed to the model is
deliberately sanitized and must not pull in URLs / raw context (a
prompt-injection surface). Keep only the sanitized check-name rendering
(.name // .workflowName, still gsub+truncated).
Update the assertions that the retained improvements (watermark decoupling,
pending-check staleness bound, always-post-handoff-on-failure) legitimately
changed. Workflow test: 48/48 green.
* ci(autofix): address review — handle cancellation, fold a PR fetch, cover schema commands
Addresses the three inline /review suggestions on the PR:
- Handoff on any non-success end, not just "failure". A 120-minute job-timeout
cancellation sets job.status = "cancelled", which the "== failure" check
missed — leaving no marker and no comment, so the next scan re-targeted the
same feedback with the same round (an invisible loop). Use "!= success";
the step only runs on failure()/cancelled()/dry-run and dry-run is excluded.
- Fold the createdAt fetch into the existing statusCheckRollup gh pr view call
(statusCheckRollup,createdAt), removing one GitHub API round-trip per PR
scanned.
- Add test coverage the earlier diff lacked: assert generate:settings-schema is
in both agent allowlists and that both verify gates run the schema-freshness
check, so a future edit can't silently drop the guard this PR adds.
* ci(autofix): fix handoff/staleness design flaws from review (4 critical + 2 suggestions)
Addresses the CHANGES_REQUESTED review of the handoff (E-4) and pending-check
staleness (E-3) logic:
- Suppress the handoff once a run published a result (OUTCOME fixed/noop), so a
later always() step failing the job (e.g. artifact upload) can no longer post
a contradictory acted=false handoff over a reported success.
- Bound the agent step at 80m, well under the 120m job timeout, so a runaway
agent fails the STEP (not the job) and the always() report step still runs and
hands off — a job-level timeout would cancel that step too and go silent.
- On a pre-prepare crash (empty NEWEST) the watermark can't advance, so write a
terminal marker (round = MAX_ROUNDS) and skip on the highest marker round
(not last-by-ts), so the scan stops re-handing-off instead of repeating until
MAX_ROUNDS.
- Raise the pending-staleness bound from 30m to 240m so an active check
(review-pr ~50m, review-address up to 120m) is never aged out mid-flight and
the same feedback double-processed; only truly-dead checks are ignored.
- Prefer the agent's detailed failure.md over the generic handoff.md wrapper.
Adds a bash-replay test that extracts the actual POST_HANDOFF decision and
MARK_ROUND logic from the workflow and exercises the state transitions
(published+late-failure, dry-run, verify failure, pre-verify crash,
cancellation; terminal vs incremental round). Workflow test: 49/49.
* ci(autofix): address review nits — symmetric schema-gate outcome, softer SKILL wording
Two non-blocking review suggestions:
- The issue-phase verify gate's schema-freshness check now writes
outcome=failed before exit 1, matching the address-review gate, so the
issue-phase step summary shows outcome=failed instead of outcome=unknown.
- Reword the SKILL rule from "do not invent environment excuses" to "do not
skip a failing check by attributing it to the environment without evidence,"
which keeps the intent (no hand-waving a real failure as an env issue)
without discouraging the agent from reporting a genuine infra failure.
* ci(autofix): close review edge cases — structural schema gate, immutable floor, robust handoff
Review round 3 (2 critical + 3 suggestions):
- Run the review gate's settings-schema freshness check BEFORE the no-op/
unchanged return, so a stale-schema PR the agent wrongly no-ops fails
(outcome=failed) instead of being reported as evaluated while CI stays red —
the exact motivating bug. Single check now covers every path; ordering is
asserted in the test.
- Never fall back to the mutable head commit date for the pre-first-eval
watermark floor: if the PR metadata query fails, use an empty (over-inclusive,
never-buries) floor. A base-sync HEAD as the floor would recreate the burial
bug. Removes the now-unused HEAD_SHA lookup.
- Guard the terminal handoff marker's timestamp (MARK_TS=${NEWEST:-${WATERMARK:-unknown}})
so a cascading API failure that blanks WATERMARK can't emit an unparseable
`ts=` that the scan regex skips, defeating the terminal-round guard.
- Truncate failure.md through `iconv -f utf-8 -t utf-8 -c` so a byte-level
head -c can't split a multi-byte sequence and corrupt the comment body.
- A pre-prepare crash (empty NEWEST) now says "could not start evaluation"
instead of "round 5/5", which would imply MAX_ROUNDS attempts were made.
Workflow test: 49/49.
* test(autofix): assert regression-catching invariants flagged in review
Four review suggestions, test-only: assert the else-branch floor
(EFF_WM=${CREATED_WM}, not the old PUSH_WM), the staleness jq filter
(.startedAt // ... // $cut), the JOB_STATUS env declaration (else it is always
empty → over-eager handoffs), and the .name // .workflowName feedback format —
so a regression on any of these is caught rather than passing silently.
* ci(autofix): fix iconv silent-abort, fold branch fetch, tighten staleness filter
Review round with a real regression in my own round-3 UTF-8 fix:
- CRITICAL: `iconv -c` exits 1 whenever it discards a byte split by `head -c`,
and under the step's `set -eo pipefail` that aborts before the eval marker +
gh pr comment run — a silent stall, the exact failure this block prevents.
Add `|| true`; the cleaned text is already emitted, so the handoff continues.
- Fold headRefName into the PR_META fetch (headRefName,statusCheckRollup,
createdAt) and derive BRANCH from it — one fewer API call per scanned PR.
- Simplify the pending-staleness clock to `.startedAt // $cut`: statusCheckRollup
has no updatedAt and pending checks have no completedAt, so those fallbacks
were dead and contradicted the comment. Now a check blocks only if it actually
started within the bound; comment matches the code.
- Tests: assert `.startedAt // $cut) > $cut` (the comparison, not just the
constant) and the `|| true` guard, so a flipped comparison or a dropped guard
is caught. 49/49.
* ci(autofix): review round — skip empty branch, hoist staleness vars, robust sentinel
Six review suggestions (no criticals):
- Skip a candidate PR when the metadata fetch fails (empty branch) instead of
falling through to an address job that fails on `git checkout -B "" origin/`
and posts a misleading handoff. This also means CREATED_WM is only reached
with populated metadata (subsumes the empty-floor warning suggestion).
- Hoist the invariant PENDING_STALE_MIN / PENDING_CUTOFF out of the per-PR loop
(one `date` fork instead of one per candidate).
- Replace the MARK_TS "unknown" sentinel with a far-future ISO-8601 date, so it
is non-empty AND sorts above real timestamps without relying on an
undocumented lexicographic quirk of a bare word.
- Cross-reference comment on the positional eval-marker regex noting it must
stay in lockstep with every write site (ts= acted= round=).
- Tests: document OUTCOME="" + JOB_STATUS=success → no handoff, and assert the
empty-branch skip guard.
* ci(autofix): DRY schema check via --check, widen handoff detail, honest terminal recovery
Three review suggestions:
- Replace both duplicated schema-freshness blocks with the generator's in-process
`--check` mode (verified: exit 1 when stale, 0 when fresh; no disk write, so
the review gate's later no-op git-diff is unaffected). Single source of truth
with CI; a future change to the check lives in one place.
- Widen the handoff DETAIL_FILE search to address-summary.md/no-action.md: when
the agent succeeds but a post-agent verify gate fails (e.g. the schema gate),
OUTCOME=failed with only the success outputs present, and "Push and report" is
skipped — so this was posting a false "crashed or timed out" and dropping the
agent's real summary.
- Correct the terminal-crash headline: the marker makes the scan skip forever
(even forced dispatch), so "re-trigger if transient" was misleading; the
headline now states the real recovery — delete the terminal autofix-eval
marker comment, then re-trigger. (Keeps the terminal design a prior review
asked for; only the advertised recovery is fixed.)
* ci(autofix): revert schema gate off --check (removed from main by #7031); jq replay
- CRITICAL: `--check` was reverted from main's generator by #7031 (3d4601489),
after this branch's base. Since this PR doesn't touch the generator, the
merged code would run main's argument-ignoring generator and both freshness
gates would go fail-open — the exact stale-artifact stall this PR prevents.
Revert both gates to regenerate + `git status --porcelain` (restoring the file
on failure), which mirrors CI's actual "Check settings schema is up-to-date"
step and works with any generator version. Verified no --check on current
origin/main.
- Add a behavioral jq replay of the pending-staleness filter (started-before vs
started-after cutoff, and no-startedAt) so a flipped comparison is caught, not
just string-matched.
* ci(autofix): set outcome=failed explicitly if the schema generator crashes
Both verify gates ran the generator unguarded: if it crashes (e.g. a type error
the agent introduced in the schema source), set -eo pipefail aborts the step
before outcome=failed is written, leaving OUTCOME unset (the handoff still fires
via job.status, but the outcome is inferred rather than explicit). Wrap the
generator in `if ! ...; then outcome=failed; exit 1; fi` so the failure is
explicit and does not depend on the job.status fallback. Test asserts the guard.
* ci(autofix): extract the settings-schema gate into a shared script
Address review: the 16-line schema-freshness gate (generator crash
guard, porcelain check, restore, outcome=failed) was duplicated verbatim
between the issue-fix verify step and the triage-and-address verify
step, so an edit to either copy could silently diverge from the other.
Move it to .github/scripts/check-settings-schema.sh and call it from
both sites; the site-specific rationale comments stay at the call sites,
the shared mechanics and the crash-guard rationale live in the script.
The script preserves the exact step contract, verified with a PATH-shim
harness over a temp git repo: generator ok + fresh schema exits 0 with
no output written; a generator crash writes outcome=failed to
GITHUB_OUTPUT and exits 1; a stale schema prints the diff, restores the
file, writes outcome=failed, and exits 1.
* test(autofix): update verify-gate assertions for the extracted schema-check script
The main-merge extracted the inline schema-freshness block into the shared
.github/scripts/check-settings-schema.sh (both verify gates now invoke it), which
broke the test that asserted the inline generate/porcelain strings in the step.
Assert the step invokes the script and that the SCRIPT holds the logic
(regenerate, crash guard, git status --porcelain, no --check, outcome=failed),
and that the review gate's script call precedes the no-op return.
---------
Co-authored-by: wenshao <wenshao@example.com>
* ci(web-shell): before/after visual previews, showing only changed views
The visual-preview bot posted the same fixed set of canned screenshots
on every web-shell PR, so it could not show what a PR actually changed
(a mermaid/split feature was invisible) and added noise on PRs that
touch the UI only trivially.
Render each scenario against BOTH the PR base (`main`) and the PR head,
pixel-diff them, and post a stitched "main | this PR" composite for only
the views that CHANGED. A PR with no visual impact composites nothing →
"no visual change". This makes the preview feature-aware with no per-PR
understanding: the diff finds exactly the surface the PR moved (and it
subsumes the backend-PR noise #6959 pre-filtered, at the content level).
- web-shell-visuals-compose.mjs: pixel-diff (canvas) + stitch a labelled
composite; pure helpers (parseShot/isChanged/planWork) unit-tested.
- web-shell-visuals.yml: also render the base — trusted `main` via
pull_request.base.sha, so no secret exposure in the untrusted-PR job —
then compose; composites replace the raw after-shots (same
`<view>-<theme>.png` name the publisher already expects).
- publish buildComment: list composites; "no visual change" when none.
Verified locally by overlaying #6881's real changes onto main + a new
mermaid scenario: the compositor flagged the mermaid view 6.5% changed
(its new zoom controls) and correctly skipped the unchanged transcript.
* ci(web-shell): address before/after review — lazy import, merge-base, robustness
Addresses the /review findings on the before/after preview:
- Lazy the @playwright/test import in the compositor so the pure exports
(parseShot/isChanged/planWork) load dependency-free, and wire
web-shell-visuals-compose.test.mjs into the github_ci_only test step —
it was never actually running in CI. (finding 1)
- Diff against the MERGE-BASE, not the base-branch tip, so a PR branch
behind main doesn't render others' already-landed changes reversed as
this PR's diff. (finding 2)
- continue-on-error on the base checkout + install so a flaky base
degrades to after-only instead of sinking the job; compose likewise
degrades to the raw after-shots on failure. (finding 3)
- timeout 20->30 (the job ~doubled) and scope the base render to
screenshots.spec.ts, skipping the discarded flow videos. (finding 4)
- diffPct: add img.onerror so a corrupt/truncated baseline PNG can't hang
page.evaluate to the job timeout. (finding 5)
- Lower CHANGED_PCT_THRESHOLD 0.1 -> 0.02 (~205px at 1280x800) so an icon
swap or one-word label change isn't classified "no change". (finding 6)
- Nits: correct the stdout comment, esc() the burned-in labels, and scope
the comment wording to "screenshots" (flows are always head-only).
* ci(web-shell): address before/after review round 2 (yiliang114)
- diffPct: a dimension change IS a visual change — comparing only the
overlapping rectangle hid it (a taller viewport with unchanged top
pixels read 0%). Short-circuit any size mismatch to changed. (Critical)
- Composite/comment label: "PR base (before)" not "main" — the workflow
also runs for release/**, whose base is not main. (Critical)
- Merge-base resolve: retry the compare API, then emit an EMPTY sha and
SKIP the base render (after-only) rather than falling back to the
base-branch tip, which reintroduces the reversed-diff bug. (Critical)
- Base steps get ids; the before render runs only when the base checkout
AND install both succeeded — else base/ (nested under head) resolves
node_modules up to head's and produces a hybrid before. (Critical)
- Publisher: a zero-change run now UPDATES the marker comment (image-less
"no screenshot changes") instead of exiting, so a prior preview's stale
images + SHA do not linger. (Critical)
Finding 6 (helper tests skip full CI) is a pre-existing repo-wide gap for
every .github/scripts test; left for a focused follow-up.
* ci(web-shell): close the compositor browser in a finally (leak on error)
A mid-loop rejection in diffPct (evaluate timeout / CDP disconnect on a
corrupt or oversized PNG) exited composeCli via the exception and skipped
browser.close(), leaking a ~200 MB Chromium child for the rest of the CI
job. Wrap the page + loop in try/finally so the browser always closes.
Also drop the stale "main (before)" labels from the docstring (the
composite/comment say "PR base" now, since the workflow also runs for
release/**).
* ci(web-shell): catch the compositor CLI promise for a clean exit
An unhandled composeCli() rejection (e.g. a missing @playwright/test)
printed an UnhandledPromiseRejectionWarning and exited without a
meaningful code; add a .catch that writes the error and exits 1.
* ci: run .github/scripts helper tests in full CI + test planWork nullish guards
- Finding 6: the compositor/publisher helper tests ran only in the
github_ci_only profile, which a `full` PR skips (and vitest test:ci
doesn't collect node:test files) — so a compositor change could pass CI
without its regression tests. Run them in the full ubuntu Test job too.
- Cover planWork's `?? []` guards with null/undefined inputs.
* ci: extract HELPER_TESTS list so both CI profiles share one source of truth
Round-3 F6 fix duplicated the .github/scripts node:test list across the
github_ci_only and full-profile steps; a missed edit would silently drop
coverage in one path. Hoist it to a workflow-level env var both reference.
---------
Co-authored-by: wenshao <wenshao@example.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
* ci(serve): daemon A/B before/after preview on response-surface PRs
The daemon analog of the web-shell visual before/after bot. For PRs that
touch the qwen serve response surface, build the CLI from BOTH the PR
base (main) and head, drive a fixed set of endpoints against each daemon,
and diff the JSON responses into a before/after field table posted on the
PR. A PR with no response change -> "no change".
- serve-ab-diff.mjs: structural JSON diff (set-aware arrays, volatile-
field masking) -> before/after table; buildComment assembles all
scenarios. Pure helpers unit-tested (12 tests).
- serve-ab-drive.mjs: boots the built daemon, drives GET /health,
/health?deep=1, /capabilities (no model -- dummy creds), captures JSON.
- serve-ab.yml / serve-ab-publish.yml: capture (untrusted, no secrets) +
privileged workflow_run publisher (validate+bind PR, dedup, TOCTOU),
mirroring the web-shell split.
Verified locally: real daemon boot + capture, the engine diffing real
capabilities -> the before/after table, and no-change detection. The
workflow wiring (2x build + drive in CI) is exercised on the next serve PR.
* ci(serve): address before/after review — merge-base, base fail-safe, CI-wire tests
Applies the transferable web-shell before/after review findings to the
daemon A/B:
- Diff against the MERGE-BASE, not the base-branch tip, so a PR branch
behind main doesn't show others' already-landed daemon changes reversed
as this PR's diff.
- continue-on-error on the base checkout so a flaky base degrades to
after-only instead of sinking the job (the base build/drive already was).
- Wire serve-ab-diff.test.mjs into the github_ci_only test step — it uses
only node builtins, so it runs there dependency-free (no npm ci needed).
* feat(serve): add a create-session deep-health A/B scenario
Add a `health-deep-with-session` scenario: the drive harness gains
setup-request support, creates one session (POST /session), then probes
GET /health?deep=1 — exercising the session lifecycle + cross-workspace
session aggregation (#6961's exact case). Broaden the volatile mask to
cover lastActivityAt/idleSinceMs/sessionId/clientId/workspaceCwd so the
capture is deterministic, while the meaningful counts (sessions,
activePrompts, pendingPermissions, connectedClients, channelAlive) stay
diffable.
Verified against the real daemon: two runs of the same build diff to "no
change" (deterministic), and a sessions 1->2 change surfaces as a
before/after table row.
* ci(serve): address review — narrow trigger, degraded-base marker, test the entrypoint
- Narrow the trigger from packages/core/src/** (~1100 mostly-unrelated
files) to packages/cli/src/serve/** — the daemon's actual response
surface (core has no serve/health subtree). (Suggestion)
- diffCaptureDirs signals baselineMissing when head captures exist but the
base build/drive produced none, so the comment says "diff skipped"
rather than misreporting every field as added or "no change". (Suggestion)
- Export + test diffCaptureDirs (the function the CI `comment` subcommand
actually invokes) with temp-fixture dirs, covering the diff + degraded
path. (Suggestion)
- Wording: "PR base" not "main" (the workflow also runs for release/**).
* ci(serve): address review round 2 — escape table values, guard setup/baseline/daemon
- fmt escapes `|` and backtick so an arbitrary daemon value can't split a
GFM table cell or close the code span.
- diffCaptureDirs surfaces base-only (removed/failed-to-capture) scenarios
instead of silently dropping them — otherwise a broken scenario lowers the
"across N" count and masks the regression.
- The drive checks each setup request's status and throws on non-2xx, so a
failed POST /session can't let health-deep-with-session capture wrong
state (0 sessions) and fake/mask a diff.
- The drive awaits daemon exit after SIGTERM (SIGKILL after 5s) so a hung
daemon can't linger.
* ci(serve): clear-error JSON parse for both capture sides
The after-side JSON.parse was unguarded while the before-side wasn't; a
readJson helper now gives a 'invalid JSON capture at <path>' error for
either side instead of a raw SyntaxError.
* ci(serve): review round 3 — merge-base skip, surface malformed base, escape path, kill dead stdout
- serve-ab.yml: retry the merge-base compare then SKIP the before build
(after-only) instead of falling back to the base-branch tip; gate the
base checkout/build on a resolved sha + successful checkout.
- diffCaptureDirs: an existing-but-malformed base capture now surfaces
(existsSync + readJson) instead of a bare catch treating it as {} (which
reported every field as "added"); covered by a new test.
- renderTable: escape backticks in the path field too (fmt already did for
values).
- Drop the dead stdout + its misleading "workflow reads it" comment (the
publisher posts whenever body.md exists); log the count to stderr.
* ci(serve): renderTable label 'PR base' not 'main' (release/** provenance)
buildComment already said 'PR base', but the per-scenario table header +
no-change line still said 'main'; align them (the workflow also runs for
release/**, whose base is not main).
---------
Co-authored-by: wenshao <wenshao@example.com>
* feat(web-shell): auto-post visual previews (screenshots + flow GIFs) on PRs
PRs that touch the web-shell UI now get an auto-updated comment with
light/dark screenshots of key views (transcript, slash menu, model/theme
dialogs, permission panel) and short GIF recordings of common flows,
rendered against the existing mock daemon — no real backend, no secrets.
Split into two workflows for security, since capture runs untrusted PR code:
- web-shell-visuals.yml (pull_request): checks out the PR head, builds and
renders it with Playwright, captures PNGs + webm, converts webm->GIF with
ffmpeg, and uploads an artifact. `contents: read` only, references no
secrets — fork PRs run with a read-only token and no secrets.
- web-shell-visuals-publish.yml (workflow_run): downloads the artifact,
binds it to its real PR by requiring the PR head SHA to equal the run's
authenticated head SHA, hosts the images on a per-PR `pr-assets/*` branch
(referenced by immutable commit SHA), and posts/updates one inline
comment. Never checks out or runs PR code; the write token lives only here.
Capture infra is self-contained in packages/web-shell
(playwright.visuals.config.ts + client/e2e/visuals/*), reusing the mock
daemon harness. Run locally with:
`npm run test:e2e:visuals --workspace=packages/web-shell`.
* fix(web-shell): guard empty gh api response in visuals publish
Addresses review feedback on #6880: if `gh api` returns empty (network
error / rate limit), jq on empty stdin errors and `set -e` kills the
publish job. Skip gracefully instead.
* fix(web-shell): address review nits on visuals capture
- harness recordFlow: wrap video saveAs/delete in try/catch so a video
I/O error (e.g. drive failed before navigation) can't mask the real
driveError.
- capture workflow: drop the unused head_sha.txt artifact field; the
publish job binds to the authenticated workflow_run.head_sha, and an
artifact-sourced SHA would be untrusted.
* fix(web-shell): address second review round on visuals capture
- context.close() in recordFlow's finally is now best-effort (try/catch)
so a close/crash error can't mask the real driveError.
- add a flows spec that asserts a throwing drive propagates its own error.
- trigger the capture workflow on playwright.visuals.config.ts changes too.
* fix(web-shell): address third review round on visuals capture
- harness: log (don't silently swallow) a video save/null when drive
succeeded; keep masking-suppression only when driveError is set.
- publish: HTML-escape interpolated values in the comment builder (defense
in depth, independent of the upstream filename sanitization); fix the
stale 'single pr-assets branch' comment and key concurrency on source
repo+branch so different PRs (incl. same-named fork branches) parallelize.
- capture: bump checkout to v6.0.3 (repo standard); surface ffmpeg's stderr
on GIF-conversion failure instead of discarding it.
* fix(web-shell): harden visuals publish/capture (review round 4)
Publish (privileged workflow_run):
- CRITICAL: capture basename before `tr` so its trailing newline isn't
turned into `_` (which broke the .png/.gif filter -> empty preview).
- dedup only against the bot's OWN comment (author + marker), not any
marker-bearing comment a participant can post.
- bound the pr-assets branch: force-push a single orphan snapshot per run
(previous snapshot GC'd) instead of appending unbounded untrusted content;
this also removes the rebase/retry path.
- cap EXAMINED candidates (not just accepted) before validation; tighten
per-file (3MiB) and accepted-image (14) caps.
- re-validate PR open + head-SHA immediately before the comment write
(TOCTOU); retry the comment listing and abort rather than POST a duplicate
when listing fails.
- esc() the runUrl for consistency with the self-defending HTML.
Capture (pull_request):
- upload raw recordings as a SEPARATE artifact the publisher never downloads,
so an untrusted multi-GB video can't exhaust the privileged job.
- also trigger on packages/webui/src and packages/sdk-typescript/src (the
visuals dev server aliases them).
- create screenshots/gifs dirs before the metadata counts (defensive).
Harness recordFlow:
- track drive failure with an explicit boolean (handles `throw undefined`);
discard the recording on failure so a failed flow leaves no bogus webm.
* refactor(web-shell): extract + unit-test the visuals publish staging/comment
Addresses the review's testability gap (the class of bug that let the
filename sanitizer break the whole preview slip through green CI). The image
validation (magic bytes, filename sanitization, examined/accepted/size caps)
and the comment builder (light/dark pairing, flow labels, HTML escaping) move
from inline workflow bash/node into .github/scripts/web-shell-visuals-publish
.mjs, covered by web-shell-visuals-publish.test.mjs (run in ci.yml's
node --test line). The publish workflow sparse-checks-out and calls the
script instead. Behaviour is unchanged; it just gained a test surface.
* fix(web-shell): retry the visuals asset force-push; drop stale comment
Round-4 switched hosting to a force-push but left a comment referencing a
'push-retry loop' that no longer existed, and the force-push was a single
call that set -e would abort on a transient failure. Add a bounded retry and
correct the comment.
* fix(web-shell): harden visuals publish/capture (review round 6)
Script (unit-tested):
- flow labels: own-property lookup so `toString.gif`/`constructor.gif` can't
leak Object.prototype members into the comment.
- per-kind image caps (screenshots vs gifs) so a large screenshot set can't
silently starve the flow GIFs from the preview.
- tests for both, plus the per-kind cap.
Publish:
- bind the artifact PR number to the run's authenticated head repo+branch
(not just head SHA), rejecting a sibling PR that shares the same commit.
- re-validate before the force-push and again right before the comment write
(close the download/stage/lookup TOCTOU windows).
Capture:
- bound artifact contents before upload (drop oversized / excess files) so an
untrusted spec can't bloat the published or video artifact.
- trigger on the capture workflow file itself.
- new close-trigger cleanup workflow deletes a PR's asset branch on close, so
pr-assets/* refs don't accumulate without bound.
- single-source the capture viewport (constants.ts) shared by config + harness.
- model-switch flow asserts the daemon model request actually fired.
* fix(web-shell): stricter visuals error handling (review round 7)
Harness recordFlow:
- when the drive SUCCEEDS, a failed context.close() or video.saveAs() (or a
missing recording) now FAILS the flow instead of a swallowed console.warn —
a silent pass with no .webm makes the downstream GIF step fail confusingly.
A drive FAILURE still discards the partial video and rethrows the original
error (unchanged).
Publish:
- validate_pr distinguishes a transient API failure (empty after retries ->
exit 1, re-triggerable) from a genuine invalid state (closed / head mismatch
-> skip), via a `gate` wrapper used at all three checkpoints.
- add a 2s backoff between comment-listing retries (matching the push retry).
---------
Co-authored-by: wenshao <wenshao@example.com>
