vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-08-04 13:51:13 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 19
7996 commits 2935 branches 595 tags 1.1 GiB
Commit graph

4 commits

Author SHA1 Message Date
Shaojin Wen
14f1f2bb36
fix(ci): don't let one failing scenario sink the whole visual preview (#7511) 
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>
 2026-07-23 02:34:07 +00:00
Shaojin Wen
ddaf3aa548
ci(web-shell): before/after visual previews, showing only changed views (#6963) 
* 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>
 2026-07-16 01:54:20 +00:00
Shaojin Wen
96225b03c4
ci(web-shell): stop visual previews firing on SDK-only PRs (#6959) 
The visual-preview capture workflow triggered on
packages/sdk-typescript/src/**, but the previews render against a mock
daemon — the SDK's transport/client layer is stubbed at the network
boundary and its changes can't move a pixel in the rendered scenarios.
So #6911 (a pure-backend PR that only added a DaemonClient data-layer
method) still got the five canned screenshots re-posted as noise.

Drop the SDK trigger. The web-shell client imports no runtime code from
the SDK root and only type-imports DaemonClient, so no real UI coverage
is lost: genuine web-shell UI PRs (#6881, #6951) still trigger via
packages/web-shell/client/**. Confirmed by simulating the path predicate
against each PR's changed-file list.

Co-authored-by: wenshao <wenshao@example.com>
 2026-07-15 12:13:43 +00:00
Shaojin Wen
02c79beb62
feat(web-shell): auto-post visual previews (screenshots + flow GIFs) on PRs (#6880) 
* 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>
 2026-07-15 06:48:52 +00:00