Files
qwen-code/.github/workflows/web-shell-visuals.yml
T
Shaojin WenandQwen-Coder 61aef512e7 feat(web-shell): offer a details side task from pending AskUser questions (#16458)
* feat(web-shell): offer a details side task from pending AskUser questions

While an AskUserQuestion is pending, the Web Shell hides the composer and
the btw shortcut ignores the pending-approval state, so a user who does not
understand the options can only answer blindly or ignore the question.

Add an "Ask for details" action to the pending panel. It opens a side task
whose prefilled prompt snapshots the question and options and asks for a
verified explanation — background, per-option meaning, differences, impact
and risks, and a recommendation — delivered as a self-contained single-file
HTML in controlled ASD-STE100-style sentences under .qwen/tmp/, previewable
in the right panel. The side task inherits the parent context but never
answers the question or touches the main session; the question stays
pending until the user answers or ignores it.

The action is main-view-only and gated on the session_side_task capability,
matching /btw side. No daemon, protocol, or settings changes.

* fix(web-shell): one details side task per question, reachable on narrow panels

Post-publication audit of the AskUser details action found three issues:

- Repeat clicks provisioned duplicate side tasks for the same pending
  question, each burning a model turn. Give the details tab a stable id
  derived from the parent session and the permission request id and upsert
  it, reusing the artifact panel's open-or-focus pattern; /btw side keeps
  its unique per-invocation draft id.
- The question snapshot dropped multiSelect, so the explanation prompt did
  not say options can combine. Append a [multiSelect] marker (strict true).
- On a 320px two-question panel the actions row overflowed to the left and
  pushed the details button out of the panel (left: -103px in a real-browser
  probe; scrollWidth misses left overflow on a flex-end row). Let the row
  wrap and pin a narrow-viewport regression test asserting every action
  button stays inside the panel and stays clickable.

Each fix carries a red/green witness: the repeat-click App test, the
multiSelect unit test, and a flex-wrap mutation of the narrow visuals test.

* fix(web-shell): keep the details tab id within the daemon clientId budget

The dedup fix derived the details tab id from the parent session id AND the
request id. SideTaskPanel derives the pane's daemon clientId as
side-task:<parentSessionId>:<tabId>, which with two uuids reached 138 chars —
over the daemon's MAX_CLIENT_ID_LENGTH (128). The child session load then
400s, the pane never connects, and the prefilled prompt is never sent.
Caught by re-running the real-daemon scenario on the post-fix head; mocked
unit tests could not see it.

Derive the stable id from the permission request id alone (a uuid; tabs are
already bucketed per parent session), bringing the clientId to 101 chars.
Add a SideTaskPanel regression test asserting the derived clientId stays
within 128 for uuid-length inputs; the old id shape fails it with 138.

* fix(web-shell): retain the details question-to-child association across tab close

Review found that deduplicating only against open tabs loses the existing
child when the details tab closes: clicking "Ask for details" again for the
same pending request provisioned a second child, and reopening the child
from the side-task catalog first did not deduplicate either (its tab id is
side-task:<sessionId>, not the details-request id).

Persist the pending-request-to-child association per parent session in
localStorage, written when the details child is created. On click, focus
whichever open tab shows the known child (details tab or catalog tab);
with no tab open, reopen one attached to the existing child session — no
draft, no initial prompt, so nothing is provisioned or sent again. One
entry per parent supersedes the resolved question's association, keeping
the store bounded; the association also survives reloads.

Regression tests cover both reported paths (close + re-click keeps exactly
one createSideTaskSession call and reopens with the child's sessionId and
no initialPrompt; catalog reopen + click focuses the single existing tab).
Red witness: dropping the association lookup fails the close + re-click
test with two creation calls.

* fix(web-shell): close the details association lifecycle gaps

Re-review of the association persistence found three lifecycle gaps, all
fixed here with regression tests and mutation witnesses:

- A child exists before its explanation prompt is admitted. Reopening a
  closed details tab now retains the prompt on the tab; the pane's existing
  empty-session guard delivers it exactly once (only while the child has no
  user prompt) and never re-sends it.
- Archiving a details child left its persisted association pointing at an
  unloadable session (the daemon's load route 409s session_archived).
  archiveSideTasks now invalidates associations pointing at archived
  children, so the next click provisions a fresh child.
- Closing the draft tab before creation resolved skipped the association
  write, orphaning the child and provisioning a duplicate on the next click.
  The parent identity is captured when creation starts, and the write no
  longer depends on the tab still existing.

Also pin two invariants the deep-verification report found unasserted:
the derived clientId budget with uuid-length inputs through the real tab-id
derivation (App test), and the formatter's array guard against non-array
objects (formatter test).

* fix(web-shell): supply the unsent details prompt to a catalog-reopened tab

Co-authored-by: Qwen-Coder <[email protected]>

* fix(web-shell): treat media-only user prompts as interaction in the side-task guard

Co-authored-by: Qwen-Coder <[email protected]>

* fix(web-shell): validate details child archive state at the read site and address review suggestions

Co-authored-by: Qwen-Coder <[email protected]>

* fix(web-shell): replace stale details tab on fresh draft and re-check pending after the liveness query

Co-authored-by: Qwen-Coder <[email protected]>

* fix(web-shell): replace a details tab bound to a different child than the validated binding

Co-authored-by: Qwen-Coder <[email protected]>

---------

Co-authored-by: Qwen-Coder <[email protected]>
2026-11-07 19:16:53 +08:00

327 lines
17 KiB
YAML

name: 'Web-shell Visuals'
# For PRs that touch the web-shell UI, render its screenshots (light + dark)
# against BOTH the PR base (`main`) and the PR head, diff them, and stitch a
# "main | this PR" composite for the views that CHANGED (plus short flow
# recordings). The images are handed to the companion
# `web-shell-visuals-publish.yml` (workflow_run), which posts them inline on the
# PR. A PR with no visual impact composites nothing → "no visual change".
#
# Security model: this workflow BUILDS AND RENDERS untrusted PR code, so it runs
# on the `pull_request` trigger (fork PRs get a read-only token and NO secrets),
# on an ephemeral hosted runner, with `contents: read` and no secrets of its
# own. It produces only image/video bytes as an artifact. The privileged step
# that needs a write token — pushing the images and commenting on the PR — lives
# in the separate workflow_run workflow that never checks out PR code.
on:
pull_request:
branches:
- 'main'
- 'release/**'
# Matches the /tmux flow's web-shell surface: only the client UI, so
# doc/config-only PRs don't trigger a full build + render.
paths:
- 'packages/web-shell/client/**'
- 'packages/web-shell/package.json'
- 'packages/web-shell/vite.config.ts'
- 'packages/web-shell/playwright.visuals.config.ts'
# NOTE: packages/sdk-typescript/src/** is deliberately NOT a trigger.
# It is aliased in too, but the visuals render against a *mock* daemon, so
# the SDK's transport/client layer is stubbed at the network boundary and
# its changes don't alter the canned scenarios — e.g. #6911 only added a
# DaemonClient data-layer method yet still re-posted identical screenshots
# on a pure-backend PR. The web-shell client imports no runtime code from
# the SDK root and only type-imports DaemonClient, so leaving the SDK out
# avoids spamming backend PRs. If an SDK-only, render-shaping change ever
# needs a preview, add the specific file (e.g. daemon/events.ts) here
# rather than the whole tree.
# The capture pipeline itself, so a workflow-only change is exercised.
- '.github/workflows/web-shell-visuals.yml'
# The before/after compositor, so a compose-only change is exercised.
- '.github/scripts/web-shell-visuals-compose.mjs'
permissions:
contents: 'read'
concurrency:
group: '${{ github.workflow }}-${{ github.event.pull_request.number }}'
cancel-in-progress: true
defaults:
run:
shell: 'bash'
jobs:
capture:
name: 'Capture web-shell visuals (ubuntu-latest, Node 22.x)'
if: "${{ github.repository == 'QwenLM/qwen-code' }}"
runs-on: 'ubuntu-latest'
# Leave room for cold dependency setup before both render passes.
timeout-minutes: 45
steps:
- name: 'Checkout PR head'
uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3
with:
ref: '${{ github.event.pull_request.head.sha }}'
fetch-depth: 1
persist-credentials: false
- name: 'Set up Node.js 22.x'
uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0
with:
node-version: '22.x'
package-manager-cache: false
registry-url: 'https://registry.npmjs.org/'
- name: 'Cache pnpm store (hosted)'
if: "${{ runner.environment == 'github-hosted' }}"
uses: './.github/actions/pnpm-store-cache'
- name: 'Configure npm for rate limiting'
run: |-
npm config set fetch-retry-mintimeout 20000
npm config set fetch-retry-maxtimeout 120000
npm config set fetch-retries 5
npm config set fetch-timeout 300000
- name: 'Install dependencies'
run: 'corepack pnpm install --frozen-lockfile --prefer-offline --reporter=append-only'
- name: 'Install Playwright Chromium'
run: 'npx playwright install --with-deps chromium'
- name: 'Choose web-shell Playwright port'
run: |-
port="$(node -e "const net=require('node:net');const server=net.createServer();server.listen(0,'127.0.0.1',()=>{console.log(server.address().port);server.close();});")"
echo "PLAYWRIGHT_PORT=${port}" >> "${GITHUB_ENV}"
echo "Using web-shell Playwright port ${port}"
# continue-on-error: a single failing/timing-out scenario must NOT discard
# the whole preview. Playwright writes each PNG/webm as its test passes, so
# the captures from the scenarios that DID pass are already on disk; the
# steps below (compose/upload) then publish those instead of the job dying
# here and the passing shots never being uploaded. The real result is read
# from `steps.after_capture.outcome` (which continue-on-error does NOT mask,
# unlike `.conclusion`) and shipped to the publisher as render-status.txt,
# so the comment says "render incomplete" rather than a misleading
# "no changes" when a scenario broke. The job stays green because the
# publish workflow only runs on a `success` conclusion; the broken scenario
# is surfaced in the comment, not by sinking every other PR author's preview.
- name: 'Capture screenshots and flow recordings (after / PR head)'
id: 'after_capture'
continue-on-error: true
env:
WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-visuals'
run: 'npm run test:e2e:visuals --workspace=packages/web-shell'
# --- Before/after: render the PR's MERGE-BASE too, then keep only CHANGED
# views. The merge-base — NOT the base-branch tip — is the correct "before"
# for a `head.sha` render: a PR branch behind `main` would otherwise show
# other people's already-landed web-shell changes, reversed, as this PR's
# diff. The merge-base is trusted history, so this adds no secret exposure
# to the untrusted-PR job — it only produces more image bytes. Every base
# step is best-effort (continue-on-error): a flaky base build/render
# degrades to after-only (each view tagged NEW), never sinking the preview.
- name: 'Resolve the merge-base'
id: 'mergebase'
env:
GH_TOKEN: '${{ secrets.GITHUB_TOKEN }}'
BASE_SHA: '${{ github.event.pull_request.base.sha }}'
HEAD_SHA: '${{ github.event.pull_request.head.sha }}'
run: |-
set -euo pipefail
# Read token suffices and works for fork PRs. Retry a few times; if it
# STILL fails, emit an EMPTY sha so the before render is skipped
# entirely (after-only). Falling back to the base-branch tip would
# reintroduce the exact reversed-diff bug the merge-base prevents.
MB=''
for attempt in 1 2 3; do
MB="$(gh api "repos/${GITHUB_REPOSITORY}/compare/${BASE_SHA}...${HEAD_SHA}" --jq '.merge_base_commit.sha' 2>/dev/null || true)"
[ -n "${MB}" ] && break
sleep 2
done
echo "sha=${MB}" >> "${GITHUB_OUTPUT}"
if [ -n "${MB}" ]; then
echo "before (merge-base) = ${MB}"
else
echo "::warning::could not resolve the merge-base; skipping the before render (after-only)."
fi
- name: 'Check out the merge-base (before render)'
id: 'base_checkout'
if: "${{ steps.mergebase.outputs.sha != '' }}"
continue-on-error: true
uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3
with:
ref: '${{ steps.mergebase.outputs.sha }}'
path: 'base'
fetch-depth: 1
persist-credentials: false
- name: 'Install base dependencies'
id: 'base_install'
if: "${{ steps.base_checkout.outcome == 'success' }}"
continue-on-error: true
working-directory: 'base'
# The base checkout can predate pnpm-lock.yaml; install it the way
# that commit was built.
run: |-
if [ -f pnpm-lock.yaml ]; then
corepack pnpm install --frozen-lockfile --prefer-offline --reporter=append-only
else
npm ci --prefer-offline --no-audit --progress=false
fi
# Run ONLY when the checkout AND install both succeeded. `base/` is nested
# under the head checkout, so if the base's own node_modules is absent (a
# failed npm ci) the render would resolve Playwright/Vite UP to the head's
# node_modules and produce a HYBRID before. A skip here → absent baseline →
# after-only. Screenshots only (base flow videos are discarded); `--`
# passes the spec through to `playwright test`; browsers are shared.
#
# The positional filters are an explicit allowlist of every
# screenshot-producing spec: a view rendered only by a spec missing here
# never gets a merge-base baseline, so the composer keeps stamping it
# "new scenario" on every PR instead of diffing it. When adding a new
# screenshot spec, append it here too. The video-flow specs
# (flows.spec.ts) and the animation-harness test (harness.spec.ts) stay
# out — the base arm discards recordings, so rendering them is waste.
- name: 'Capture before (base) screenshots'
if: "${{ steps.base_checkout.outcome == 'success' && steps.base_install.outcome == 'success' }}"
working-directory: 'base'
continue-on-error: true
env:
WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-before'
run: 'npm run test:e2e:visuals --workspace=packages/web-shell -- screenshots.spec.ts workflow-page.spec.ts git-branch-picker.spec.ts session-workflow.spec.ts collaboration.spec.ts ask-user-details.spec.ts'
- name: 'Compose before/after and keep only changed views'
env:
BEFORE_DIR: '${{ runner.temp }}/web-shell-before/screenshots'
AFTER_DIR: '${{ runner.temp }}/web-shell-visuals/screenshots'
OUT_DIR: '${{ runner.temp }}/web-shell-visuals'
run: |-
set -euo pipefail
# Diff each <view>-<theme>.png against the base render and stitch a
# labelled "main | this PR" composite for the views that changed. The
# composites REPLACE the raw after-shots as the published screenshots,
# reusing the <view>-<theme>.png name the publish step already expects.
if node .github/scripts/web-shell-visuals-compose.mjs \
"${BEFORE_DIR}" "${AFTER_DIR}" "${OUT_DIR}/composite"; then
rm -f "${OUT_DIR}/composite/manifest.json"
rm -rf "${OUT_DIR}/screenshots"
mv "${OUT_DIR}/composite" "${OUT_DIR}/screenshots"
echo "Changed views composited: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')"
else
# Degrade rather than sink: leave the raw after-shots in
# ${OUT_DIR}/screenshots so the PR still gets a (head-only) preview.
echo "::warning::compose failed; publishing raw after-shots without a before/after."
fi
- name: 'Convert flow recordings to inline GIFs'
env:
OUT_DIR: '${{ runner.temp }}/web-shell-visuals'
run: |-
set -euo pipefail
if ! command -v ffmpeg >/dev/null 2>&1; then
echo "::warning::ffmpeg not found on the runner; skipping GIF conversion (raw .webm is still uploaded)."
exit 0
fi
mkdir -p "${OUT_DIR}/gifs"
shopt -s nullglob
converted=0
for webm in "${OUT_DIR}"/video/*.webm; do
name="$(basename "${webm%.webm}")"
gif="${OUT_DIR}/gifs/${name}.gif"
# -ss 1 trims the ~1s blank while the page loads. Two-pass palette
# (generate + apply) keeps the GIF sharp at a fraction of naive size.
if err="$(ffmpeg -y -ss 1 -i "${webm}" \
-vf "fps=12,scale=960:-1:flags=lanczos,split[s0][s1];[s0]palettegen=stats_mode=diff[p];[s1][p]paletteuse=dither=bayer:bayer_scale=3" \
"${gif}" 2>&1)"; then
echo "converted ${name}.webm -> gifs/${name}.gif ($(du -h "${gif}" | cut -f1))"
converted=$((converted + 1))
else
# Surface ffmpeg's own diagnostic (codec/container/filter error)
# instead of a bare "failed", so a future breakage is actionable.
detail="$(printf '%s' "${err}" | tr '\n' ' ' | tail -c 400)"
echo "::warning::ffmpeg failed to convert ${name}.webm; skipping its GIF. ${detail}"
rm -f "${gif}"
fi
done
echo "GIFs produced: ${converted}"
- name: 'Record PR metadata for the publish workflow'
env:
OUT_DIR: '${{ runner.temp }}/web-shell-visuals'
PR_NUMBER: '${{ github.event.pull_request.number }}'
# The TRUE after-render result. `.outcome` is the pre-continue-on-error
# value ('failure' when >=1 scenario failed), unlike `.conclusion` which
# the continue-on-error masks to 'success'. The publisher reads this to
# decide whether an empty/partial preview means "no visual change" or
# "a scenario failed to render" — the two must not look alike.
AFTER_OUTCOME: '${{ steps.after_capture.outcome }}'
run: |-
set -euo pipefail
# Ensure both dirs exist so the counts below are robust even when an
# upstream step produced none (e.g. no ffmpeg -> no gifs/). (The finds
# sit inside `echo "$(...)"`, so a missing dir wouldn't actually trip
# set -e — echo masks it — but create them for clarity all the same.)
mkdir -p "${OUT_DIR}/screenshots" "${OUT_DIR}/gifs"
# Bound artifact contents BEFORE upload: this job ran untrusted PR
# code, so drop oversized files and cap the count per directory —
# otherwise a hostile spec could bloat the published artifact (which
# the privileged publisher downloads) or the retained video artifact.
MAX_FILE_BYTES=$((6 * 1024 * 1024))
MAX_FILES=40
for d in screenshots gifs video; do
dir="${OUT_DIR}/${d}"
[ -d "${dir}" ] || continue
find "${dir}" -maxdepth 1 -type f -size "+${MAX_FILE_BYTES}c" \
-printf '::warning::dropping oversized artifact file %p\n' -delete || true
find "${dir}" -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort \
| tail -n "+$((MAX_FILES + 1))" \
| while IFS= read -r extra; do
echo "::warning::dropping excess artifact file ${d}/${extra}"
rm -f "${dir}/${extra}"
done
done
# PR number for the workflow_run publish job (which validates it).
# The head SHA is intentionally NOT shipped in the artifact: the
# publish job binds to the authenticated github.event.workflow_run
# .head_sha, and an artifact-sourced SHA would be untrusted.
printf '%s\n' "${PR_NUMBER}" > "${OUT_DIR}/pr.txt"
# Render status for the publisher: 'failure' when >=1 scenario failed
# (default to 'failure' if the step somehow reported nothing, so an
# unknown state fails safe toward "incomplete" rather than a false
# all-clear). Anything other than the literal 'success' is treated as
# incomplete on the publish side.
printf '%s\n' "${AFTER_OUTCOME:-failure}" > "${OUT_DIR}/render-status.txt"
echo "Screenshots: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')"
echo "GIFs: $(find "${OUT_DIR}/gifs" -name '*.gif' | wc -l | tr -d ' ')"
echo "After-render outcome: ${AFTER_OUTCOME:-unknown}"
# The privileged publish workflow downloads THIS artifact, so keep the raw
# videos out of it: an untrusted PR could drop a multi-GB file under
# video/ and exhaust the publisher's bandwidth/disk/timeout. Screenshots
# and GIFs (which the publisher hosts) are size-capped again on that side.
- name: 'Upload web-shell visuals artifact (published)'
uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1
with:
name: 'web-shell-visuals'
path: |-
${{ runner.temp }}/web-shell-visuals/screenshots
${{ runner.temp }}/web-shell-visuals/gifs
${{ runner.temp }}/web-shell-visuals/pr.txt
${{ runner.temp }}/web-shell-visuals/render-status.txt
if-no-files-found: 'warn'
retention-days: 7
# Raw recordings live in a SEPARATE artifact the publish workflow never
# downloads — they're only the "full-resolution recordings" link target.
- name: 'Upload raw flow recordings'
uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1
with:
name: 'web-shell-visuals-video'
path: '${{ runner.temp }}/web-shell-visuals/video'
if-no-files-found: 'ignore'
retention-days: 7