feat(web-shell): auto-post visual previews (screenshots + flow GIFs) on PRs - #6880
Merged
Conversation
…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`.
Collaborator
|
Thanks for the PR! (Re-run on commit Template: The body uses custom headings ("What", "How it's wired (security)", "Capture infra", "Verification") instead of the standard PR template, but all substantive sections are present — what it does, why it's needed, how to verify, evidence, and a bilingual description. The content is thorough. ✓ Problem: Real and practical. Reviewing web-shell UI changes currently requires checking out the branch, building, and rendering in a browser. Auto-generated screenshots and flow GIFs directly address this friction. Not theoretical — this is CI tooling infrastructure, not a bug fix. Direction: Well-aligned with the project's CI/CD infrastructure. The two-workflow security split (capture in untrusted Size: No core module paths touched — all changes are in Approach: The scope feels right. Two workflows, a dedicated Playwright config, a shared test harness, and a unit-tested publish script — each piece has a clear role. The extraction of publish logic into One known limitation: the publish workflow can only fire on the default branch, so its first real exercise is on PRs after this merges. The PR body acknowledges this upfront. Moving on to code review. 🔍 中文说明感谢贡献！（在 commit 模板： PR 使用了自定义标题，而非标准模板，但所有实质性部分均齐全——功能说明、动机、验证方法、证据、双语描述。内容详实。✓ 问题： 实际且实用。审查 web-shell UI 改动需要 checkout + 构建 + 浏览器渲染，这是真正的摩擦点。自动生成截图和 GIF 直接解决了这个问题。这是 CI 基础设施，非 bug 修复。 方向： 与项目 CI/CD 基础设施完全对齐。双 workflow 安全拆分是 GitHub 推荐的模式。PR 视觉预览是标准实践。 规模： 未触及核心模块路径——所有改动在 方案： 范围合理。两个 workflow、专用 Playwright 配置、共享测试工具、有单测的发布脚本——每部分职责清晰。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code reviewIndependent proposal (before reading the diff): To auto-post visual previews on PRs touching web-shell UI, I'd use the standard two-workflow split — capture on Comparison with the PR: The PR matches or exceeds this proposal in every dimension. The two-workflow split is correct, and the security model is implemented with genuine attention to failure modes:
No critical blockers found. No AGENTS.md violations. Unit test resultsAll 10 tests pass on commit Real-scenario testingN/A — this is CI infrastructure (GitHub Actions workflows + Playwright test suite), not user-visible CLI behavior. The full Playwright visuals suite runs in CI where Chromium and ffmpeg are available. Locally, the unit tests above validate the security-critical publish pipeline. The maintainer has also verified end-to-end on a real machine (13/13 Playwright tests pass, 10 PNGs + 2 webm produced, valid animated GIFs confirmed). 中文说明代码审查独立方案（阅读 diff 前）： 要自动发布视觉预览，我会使用标准的双 workflow 拆分—— 与 PR 的对比： PR 的方案在每个维度上都匹配或超越了独立方案。双 workflow 拆分正确，安全模型对失败模式有真实关注：TOCTOU 保护（三次验证）、PR 绑定验证（SHA + repo + branch）、瞬态 API 失败处理（空响应守卫）、魔数验证、分类上限、原型链安全、HTML 转义、孤儿分支托管、错误传播、artifact 边界控制。 无阻塞问题。无 AGENTS.md 违规。 单元测试结果10/10 通过，覆盖了关键纯函数——清理、图片选择（带边界）、评论构建（带转义）。 真实场景测试不适用——这是 CI 基础设施（GitHub Actions workflows + Playwright 测试套件），非用户可见 CLI 行为。完整的 Playwright 视觉套件在 CI 中运行（有 Chromium 和 ffmpeg）。本地单测验证了发布管线的安全关键部分。维护者已在真机上端到端验证（13/13 Playwright 测试通过，10 张 PNG + 2 个 webm，有效动图 GIF 已确认）。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean implementation, thorough security model, well-tested, ready to ship. This is solid CI infrastructure that's clearly been through multiple review rounds. The two-workflow security split is the right architectural choice, and the implementation shows genuine attention to the failure modes that matter for running untrusted PR code: triple-point TOCTOU validation, head-SHA + repo + branch binding, magic-byte image classification, per-kind caps to prevent resource starvation, prototype-safe label lookups, and an explicit empty-response guard for transient API failures. The unit tests cover the critical pure functions — sanitization (with a regression test for the shell-basename bug that broke previews before), image selection bounds, and comment HTML escaping. The publish script's extraction from inline workflow bash into a tested module is a clear improvement. The code reuses existing infrastructure well — the visuals tests use the same The one inherent limitation — publish workflows only fire on the default branch, so the first real exercise is on PRs after merge — is acknowledged upfront and is a fundamental constraint of Since the last triage pass, the author addressed the publish-workflow robustness nit by adding an explicit empty-response guard before the No blocking issues. Approving. ✅ 中文说明信心：5/5 — 实现干净，安全模型完善，测试充分，可以合入。 这是扎实的 CI 基础设施，明显经过多轮审查。双 workflow 安全拆分是正确的架构选择，实现对运行不可信 PR 代码时的关键失败模式有真实关注：三重 TOCTOU 验证、head SHA + repo + branch 绑定、魔数图片分类、分类上限防止资源耗尽、原型链安全的标签查找、以及针对瞬态 API 失败的显式空响应守卫。 单测覆盖关键纯函数——清理（含 shell-basename 回归测试）、图片选择边界、评论 HTML 转义。发布脚本从 workflow 内联 bash 提取到有测试的模块，是明确的改进。 代码很好地复用了现有基础设施。一个固有限制——发布 workflow 只能在默认分支上触发，首次真正运行在合入后的后续 PR 上——已在 PR 正文中说明，这是 自上次 triage 以来，作者通过添加显式空响应守卫解决了发布 workflow 健壮性问题。维护者已在真机上端到端验证完整套件。 无阻塞问题。批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
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.
Collaborator
Author
|
Thanks for the review! Addressed the publish-workflow robustness nit in |
- 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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — npm ci timed out; no local build/test verification was performed.
— qwen3.7-max via Qwen Code /review
- 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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
- 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.
wenshao
commented
Jul 14, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 14, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Re-reviewed at head 4d72cd6 (the branch advanced from 38d29a0 mid-review). My review found the capture/publish security split sound; I re-checked the concurrent review's blockers against the code and independently confirm the two below as genuine blockers (its other findings I assessed as real but several severity-inflated — deferring to those inline threads).
[Critical] Publish-workflow staging sanitizer mangles every filename. safe="$(basename "${f}" | tr -c 'A-Za-z0-9._-' '_')" appends a trailing _, because basename emits a newline that tr converts to _. So session-transcript-light.png becomes session-transcript-light.png_, and the Node pairing regex /^(.*)-(light|dark)\.png$/i plus the /\.gif$/i filter then match nothing — the preview comment is built empty even when valid images exist. Independently confirmed via shell repro: basename x.png | tr -c 'A-Za-z0-9._-' '_' yields x.png_. Also flagged by the concurrent review at line 144; the fix is to capture base="$(basename "${f}")" first (command substitution strips the newline), then sanitize.
[Critical] recordFlow re-throws on if (driveError) — a truthiness check. A drive that does throw null, throw undefined, or Promise.reject() stores a falsey reason, so the flow is reported as passed and its incomplete recording can be published. Track a separate driveFailed boolean and rethrow whenever it is set. (harness.ts, also flagged by the concurrent review.)
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
…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.
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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
✅ Maintainer local verification — PR #6880Built and ran this PR end-to-end on a real machine (macOS, Node 22.23, isolated worktree at PR head
What I checked
All 10 screenshots were opened and eyeballed: light/dark are correct, dialogs render, the transcript code block is Shiki-highlighted, and the permission panel shows its Allow/Reject actions. The two flow recordings are genuine multi-frame VP8 (8.2s / 4.2s @ 25fps, 1280×800); the exact CI The actual output this PR would postScreenshots · light / dark
FlowsOpen the slash menu and switch model
Submit a prompt and watch the reply stream in
Notes for the merge decision
Verdict: LGTM from a local-build standpoint. The suite is deterministic and self-contained (mock daemon, no model/secrets), the output renders correctly in both themes, and the flows animate as intended. Assets hosted on 中文版本（点击展开）✅ 维护者本地验证 —— PR #6880在真机上（macOS，Node 22.23，隔离 worktree，checkout 到 PR head 检查项
10 张截图均逐一肉眼确认：亮/暗正确、弹窗渲染正常、会话记录里的代码块经 Shiki 高亮、权限面板显示 Allow/Reject 按钮。两个流程录像是真实的多帧 VP8（8.2s / 4.2s @ 25fps，1280×800）；用 CI 完全相同的 合并决策备注
结论：从本地构建角度 LGTM。 套件确定性强、自包含（mock daemon、无模型/密钥），产物在双主题下渲染正确，流程动画符合预期。 资源托管于 |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
commented
Jul 15, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed — no blockers. The two-workflow security split holds (the privileged publisher never runs PR code, the untrusted PR number is sanitized and bound to the authenticated run head before any write, and all untrusted bytes are quoted/escaped/capped), and all nine Critical blockers from the prior round are fixed in the current code. Suggestions and nice-to-haves below.
Critical (0): none.
Suggestions
- [Suggestion]
esc()HTML-escaping is never exercised by its test (.github/scripts/web-shell-visuals-publish.test.mjs:96). ThebuildComment … escapes …case feeds only HTML-safe inputs (abc1234,https://run.example/1, sanitized filenames), so none of& " < >ever passes throughesc()and a regression there would pass silently. No live injection vector today (inputs are sanitized upstream), so this is a coverage gap, not a defect — but add a case feeding e.g.weird&view-light.pngand assertweird&view. - [Suggestion]
recordFlow's falsy-throw guard is untested for the regression it prevents (packages/web-shell/client/e2e/visuals/flows.spec.ts:95). ThedriveFailedboolean exists precisely sothrow undefined/throw null/Promise.reject()still mark a flow failed, but the only error test throws a truthyError, so reverting the harness to a truthiness check would stay green. Add athrow undefinedcase. - [Suggestion]
MAX_CANDIDATESis applied aftergatherCandidatesopens every file (.github/scripts/web-shell-visuals-publish.mjs:210).gatherCandidatesrunsstatSync+readMagicHexon every extension-matching file beforeselectImages's examined cap, so the cap bounds the selection loop rather than the upfront I/O the comment claims. Not exploitable (the artifact is already fully downloaded upstream bydownload-artifact, and opening N tiny files is cheap on an ephemeral runner), but slicing the candidate list toMAX_CANDIDATESinsidegatherCandidateswould make the code match the comment.
Nice-to-have
buildCommentsorts views with the defaultArray.sort()comparator (.github/scripts/web-shell-visuals-publish.mjs:153); correct today only because the[object Object]suffix is constant and,sorts below every sanitized-view char — an explicit(a, b) => a[0].localeCompare(b[0])states the intent.- The jq dedup can error on a null bot-comment body under
set -euo pipefail(.github/workflows/web-shell-visuals-publish.yml:202); essentially unreachable (the API returns string bodies and the bot always posts one) and fail-safe, but(.body // "") | contains($m)is a one-token hardening. sanitizeNamecollisions can overcount staged files (.github/scripts/web-shell-visuals-publish.mjs:236): two names reducing to the samesafeNameoverwrite on copy, so the printed count exceeds the staged count. Cosmetic only —buildCommentre-reads the real stage dir, so the preview is correct, and names are workflow-controlled.
Open-Critical re-check — the 9 prior blockers, all FIXED (none still stand)
- Sanitizer appended
_to every extension → fixed (sanitizeNameregex, no pipe/newline; regression-tested). - Dedup trusted the public marker without ownership check → fixed (filters on bot login and marker).
- Unbounded base-repo storage growth → fixed (single force-pushed orphan snapshot + 14×3 MB caps).
- Failed comment-listing → duplicate POST → fixed (3× retry, then
exit 1). - Cap counted accepted, not examined → fixed (
MAX_CANDIDATESon examined; see Suggestion 3). - TOCTOU before the comment write → fixed (
validate_prre-run immediately before the write). - Raw videos in the privileged download → fixed (separate
web-shell-visuals-videoartifact the publisher never downloads). - Stray recording in the prod output dir → fixed (
recordFlowdiscards the recording on drive failure). - Error propagation via caught-value truthiness → fixed (explicit
driveFailedboolean).
Verification: npm run build passes; the new unit suite node --test .github/scripts/web-shell-visuals-publish.test.mjs passes 8/8 (wired into CI via ci.yml). Not reviewed: the full npm test suite — it timed out at 120 s, and its 8 failures are all in packages/cli (untouched by this PR) and pre-existing/environmental. Lint/typecheck not run separately (covered by the build).
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
doudouOUC
reviewed
Jul 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Independent review — verified at c9eddf68
Went through the full diff and the security model independently. Net: no unresolved review threads remain, every previously-raised Critical maps to a fix in the current tree, and the one load-bearing security assumption checks out empirically. One non-blocking suggestion below.
Verified
- The SHA binding works and is safe. The capture run for this head reports
head_sha == c9eddf68 == PR head, soworkflow_run.head_shabinds to the real PR head (not a merge commit): a forgedpr.txtnumber is rejected because a victim PR's head won't match, and the check won't spuriously always-fail either. - Publish never checks out PR code; untrusted input is confined to opaque image bytes plus a PR number that is sanitized and SHA-bound. Magic-byte validation, examined/accepted/size caps, HTML escaping +
encodeURIComponent, the pre-write TOCTOU re-validation, thelisted=1gate (a failed comment list never reads as "no comment"), and bot-identity dedup are all present. .github/scripts/web-shell-visuals-publish.test.mjs: 8/8 green locally.
Suggestion (non-blocking) — flow GIFs can be starved by the image cap
In selectImages (.github/scripts/web-shell-visuals-publish.mjs), candidates are concatenated PNG-first and share a single MAX_IMAGES = 14 cap. Today that's fine (10 PNG + 2 GIF = 12). But once the screenshot set grows to ≥14 PNGs — e.g. adding ~2 views (7 views × 2 themes) — the cap fills on PNGs alone and all flow GIFs are silently dropped from the preview. Repro: feeding 16 PNG + 1 GIF yields { png: 14 }, gif accepted? false. Consider capping PNG/GIF separately (or accepting GIFs first) so adding screenshots can't quietly remove the Flows section.
One caveat to merge with eyes open
The publish orchestration (validate_pr → git push to pr-assets → comment post/dedup) only fires once this is on the default branch, so it has not executed end-to-end yet — only bash -n/actionlint plus the empirical head_sha check above. The riskiest part (the SHA binding) is verified; the git-push + comment path first runs on the next web-shell PR after merge and may need a quick fast-follow if anything is off. This is inherent to workflow_run and is already disclosed in the PR body.
Overall this clears the bar from a code standpoint.
wenshao
commented
Jul 15, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
| // The mock returns promptId 'prompt-e2e' for a prompt with no | ||
| // _meta.promptId, so complete the live turn with that id (clears the | ||
| // streaming spinner before the recording ends). | ||
| await daemon.sendEvent(turnCompleteEvent('prompt-e2e', { id: 11 })); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] The prompt recording sends turn_complete but never verifies that streaming returned to idle. — Failure scenario: if the completion event is ignored or routed to the wrong prompt, the already-rendered text assertion still passes and the published GIF ends with the streaming spinner active. Assert a stable streaming-status locator is absent before the final pause and video finalization.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
The prompt-stream flow sends turn_complete and asserts the streamed text renders after it, and the model-switch flow now asserts the daemon request fired (sibling comment). A dedicated streaming-idle locator assertion would tighten it further, but there isn't a stable idle-state locator wired for the composer today — leaving this open as a reasonable follow-up rather than resolving it.
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.
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
previously approved these changes
Jul 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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).
doudouOUC
approved these changes
Jul 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Approving — re-reviewed at 7ac41f20 (rounds 6–7).
Went through the incremental diff since c9eddf68 and re-checked the security model end to end. No unresolved Criticals remain — the single open thread is a non-blocking Suggestion, reasonably deferred. Verified locally: publish unit tests 10/10 green; daemon.modelRequests() backs the new flow assertion; CI Capture / Test / E2E Smoke all pass.
Both of my earlier callouts are resolved
- Flow GIFs could be starved by the shared image cap → now per-kind caps (
MAX_SCREENSHOTS=20/MAX_GIFS=6) tracked viakindCount, plus two regression tests (a screenshot flood no longer drops the GIF). pr-assets/*branch accumulation → newweb-shell-visuals-cleanup.ymldeletes the per-PR asset branch on close.
Additional hardening this round looks solid
buildCommentusesObject.hasOwnfor flow labels, so atoString.gifcan't render a prototype member as the label (with a test).validate_prnow also binds head repo + branch (not just head SHA), retries transient API failures, and distinguishes invalid → skip from transient → fail & re-trigger viagate().- A third TOCTOU gate before the force-push, so a close/new-head mid-run can't orphan the commit the existing comment points at.
- Pre-upload artifact bounds (6 MB/file, 40 files/dir incl.
video/) as defense-in-depth over the publish-side caps. - Stricter
recordFlow: acontext.close()failure after a successful drive is promoted to a failure, and a missing/failed video now fails the flow instead of silently shipping a preview without it. Thedrive-errortest still passes and CI capture is green.
pull_request_target cleanup workflow — checked the privileged surface
Safe: it never checks out or runs PR code, reads only the GitHub-provided integer pull_request.number, and deletes a fixed-pattern branch — so a PR can only ever delete its own asset branch.
One caveat to merge with eyes open (unchanged, inherent to workflow_run)
The publish and cleanup orchestration only fires once it is on the default branch, so it can't be exercised end-to-end pre-merge — the first real run is the next web-shell PR after this merges, and may need a quick fast-follow. The riskiest piece — the authenticated head-SHA binding — is empirically verified: this PR's capture run reports head_sha == PR head.
LGTM. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Comment on lines
+142
to
+143
| const m = f.match(/^(.*)-(light|dark)\.png$/i); | ||
| if (!m) continue; |
Collaborator
There was a problem hiding this comment.
[Suggestion] PNGs without a -light or -dark suffix are silently dropped from the comment output. The file passes selectImages (valid magic, within caps), gets staged and uploaded, but buildComment's regex /^(.*)-(light|dark)\.png$/i skips it with continue. — Failure scenario: a contributor renames a screenshot spec so its output filename loses the theme suffix. The image is staged and pushed to the asset branch, but the PR comment silently omits it. The publisher reports success with no signal that an image was invisible.
Suggested change
| const m = f.match(/^(.*)-(light|dark)\.png$/i); | |
| if (!m) continue; | |
| const m = f.match(/^(.*)-(light|dark)\.png$/i); | |
| if (!m) { | |
| warnings.push(`skipped unmatched PNG: ${f} (expected <view>-(light|dark).png)`); | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
Comment on lines
+109
to
+113
| accepted.push({ | ||
| name: c.name, | ||
| safeName: sanitizeName(basename(c.name)), | ||
| kind, | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] selectImages does not de-duplicate by safeName. Two candidates whose names differ only in characters that sanitizeName collapses to _ (e.g., foo bar.png and foo_bar.png) both pass all caps and magic checks and enter accepted. In stageCli, the second copyFileSync silently overwrites the first. The accepted count is inflated by one, while the comment lists only the surviving file. — Concrete cost: the workflow's "should I post?" decision is based on the inflated count, and the overwrite is invisible.
Suggested change
| accepted.push({ | |
| name: c.name, | |
| safeName: sanitizeName(basename(c.name)), | |
| kind, | |
| }); | |
| const safeName = sanitizeName(basename(c.name)); | |
| if (seenSafeNames.has(safeName)) { | |
| warnings.push(`skipped duplicate safeName: ${c.name} → ${safeName}`); | |
| continue; | |
| } | |
| seenSafeNames.add(safeName); | |
| accepted.push({ | |
| name: c.name, | |
| safeName, | |
| kind, | |
| }); |
— qwen3.7-max via Qwen Code /review
Comment on lines
+56
to
+58
| export function sanitizeName(name) { | ||
| return String(name).replace(/[^A-Za-z0-9._-]/g, '_'); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The safe-character set [A-Za-z0-9._-] includes ., so a filename like .hidden-light.png passes through unchanged. The publish workflow copies staged files with cp "${STAGE}"/* (bash glob, dotglob off by default), which does not match leading-dot entries. The dotfile is silently dropped — never committed to the asset branch — but the comment body (built from the stage directory) still references it, producing a broken image. — Fix: strip or replace a leading . in sanitizeName, or add shopt -s dotglob before the cp glob.
Suggested change
| export function sanitizeName(name) { | |
| return String(name).replace(/[^A-Za-z0-9._-]/g, '_'); | |
| } | |
| export function sanitizeName(name) { | |
| const safe = String(name).replace(/[^A-Za-z0-9._-]/g, '_'); | |
| return safe.startsWith('.') ? '_' + safe : safe; | |
| } |
— qwen3.7-max via Qwen Code /review
Comment on lines
+17
to
+19
| sanitizeName, | ||
| selectImages, | ||
| } from './web-shell-visuals-publish.mjs'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] pretty and esc are exported but neither is imported or directly tested. esc() is the sole HTML-escaping defense for values interpolated into <img src>, alt, and <a href> contexts. The buildComment test named "escapes" uses only benign inputs — none of & " < > ever passes through esc(). A regression in any of its four .replace() calls would pass silently. Similarly, pretty()'s dash/underscore-to-space and title-case logic has no direct coverage. — Add: import { esc, pretty } and test cases like esc('a"b&c<d>e') → 'a"b&c<d>e' and pretty('session-transcript') → 'Session Transcript'.
— qwen3.7-max via Qwen Code /review
| const rawBase = ctx.rawBase ?? ''; | ||
| const shortSha = ctx.shortSha ?? ''; | ||
| const runUrl = ctx.runUrl ?? ''; | ||
| const url = (name) => `${rawBase}/${encodeURIComponent(name)}`; |
Collaborator
There was a problem hiding this comment.
[Suggestion] encodeURIComponent is applied to filenames in the url() helper, but no test verifies that a filename with URL-unsafe characters is correctly encoded in the generated <img src> attributes. All test filenames are already URL-safe after sanitization. — Concrete cost: if sanitizeName were changed to allow a character that needs URL encoding (e.g., +), the generated image URLs would break silently.
— qwen3.7-max via Qwen Code /review
Comment on lines
+73
to
+75
| export function selectImages(candidates, opts = {}) { | ||
| const maxCandidates = opts.maxCandidates ?? MAX_CANDIDATES; | ||
| const maxBytes = opts.maxBytes ?? MAX_BYTES; |
Collaborator
There was a problem hiding this comment.
[Suggestion] All selectImages tests use default cap values. No test exercises the custom-opts override path (e.g., selectImages(candidates, { maxScreenshots: 2, maxGifs: 1 })). The override logic (opts.maxCandidates ?? MAX_CANDIDATES, etc.) is untested exported API. — A single test passing reduced caps and asserting the smaller limits are honored would close this gap.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 17, 2026
…emoved
The "Capture web-shell visuals" job fails on this PR at
`screenshots.spec.ts:395`, asserting the sidebar's "Primary" badge is visible:
Error: expect(locator).toBeVisible() failed
Error: element(s) not found
Not from this branch. The chain is on main:
- 2026-07-15 QwenLM#6880 adds the visuals spec, asserting the "Primary" badge —
correct at the time.
- 2026-07-17 QwenLM#7035 drops that badge as redundant (the workspace selector's
checkmark already conveys the default target), removing the `primaryLabel`
prop and its `<span className={styles.badge}>` render, and updates the *unit*
test to assert its absence — but leaves this spec asserting it is visible.
The capture job only runs on pull requests (it needs a PR head and a
merge-base), so main never went red for it and the breakage surfaces on the
next PR to merge main — this one.
Assert the badge's absence instead of deleting the check, mirroring the unit
test QwenLM#7035 added, so a regression re-adding it still fails here.
pull Bot
pushed a commit
to AmirulAndalib/qwen-code
that referenced
this pull request
Jul 18, 2026
… daemon resume (QwenLM#6561) * fix(goals): persist goal cards and restore the hook on daemon resume In daemon mode a `/goal` was silently lost whenever its session was reloaded or `qwen serve` restarted: the goal card vanished from the transcript and the Stop hook was never re-registered, so the loop simply stopped advancing. The TUI does neither of these things wrong; the ACP path was missing both halves. Goal cards were only ever emitted as live SSE `_meta` (MessageEmitter's emitGoalStatus / emitGoalTerminal) and never written to the transcript, so the one durable store — the ChatRecord JSONL — had nothing to restore from. Record them from Session.emitGoalStatus, the single choke point for `set` and `cleared` (the sessionGoalClear ext method routes through it too), and from the goal terminal observer for `achieved` / `failed` / `aborted`. Persisting `cleared` matters on its own: without it the last stored card stays `set`, and a later resume would revive a goal the user explicitly dropped. HistoryReplayer dropped those records on the way back out — it reads only `item['text']`, and a goal card has no `text` field — so re-emit them as `_meta.goalStatus`. Per-iteration `checking` cards are skipped: a TUI transcript stores one per stop-hook turn and clients suppress them as noise. That costs no fidelity, because restore reads the records directly rather than the replay output. With the transcript carrying the goal again, add #restoreGoalOnResume to loadSession and unstable_resumeSession, alongside #restoreWorktreeOnResume. It rebuilds the goal cards from the resumed ChatRecords (they live inside system/slash_command records' outputHistoryItems) and reuses the existing findGoalToRestore / findLastTerminalGoal / registerGoalHook logic, trust and hook-policy gates included. * feat(web-shell): add a workspace Goals page `/goal` had no visual surface in the web shell. You could set and clear one from the composer, but the only feedback was a status-bar pill and a transcript card, and there was no way to see every goal running in the workspace at once. Add a full-pane Goals page alongside Scheduled Tasks. Each row shows the condition, the session driving it, whether the loop is mid-turn, the judge's turn count and last verdict, and how long the goal has been running. A row opens its session — the transcript IS the goal's history — or clears the goal. A form starts a new goal in a fresh session, so the loop doesn't take over a conversation already in progress. Reading the goals needs a round trip. They live in the owning `qwen --acp` child's in-memory store, and serve runs in a separate process holding only a bridge, so there is nothing local to read. Add a `sessionGoalGet` ext method that reports one session's goal state, wrap it in bridge.getSessionGoal (mirroring clearSessionGoal), and have `GET /goals` fan out over the workspace's live sessions concurrently — one timeout for a wedged child rather than one per session. A session whose probe rejects is dropped rather than failing the whole list. Clearing reuses `POST /session/:id/goal/clear`, so the page and a `/goal clear` typed in chat take the same path through the daemon. Only loaded sessions appear, which is the honest answer rather than a limitation: a goal advances only while its session is resident. Three entry points: a sidebar button, the status-bar goal pill (now a button), and a bare `/goal`, which opens the page instead of asking the daemon to print its status as text — matching how `/schedule` behaves. It sends no prompt and touches no session, so it works mid-turn too. `/goal <condition>` and `/goal clear` are unchanged. The integration test exercises the whole chain against a real daemon: `GET /goals` -> bridge -> ext method in a spawned `qwen --acp` child. * fix(web-shell): stop the Goals poll from overlapping itself `GET /goals` fans out one ext-method probe per live session, and a wedged child holds it for the bridge's 10s `initTimeoutMs` — the same order as the 10s poll interval. `withActionTimeout` rejects the wait at 30s but never aborts the underlying fetch, so a fixed `setInterval` could stack several fan-outs against an already-struggling daemon. `reloadSeqRef` only keeps a stale response from overwriting state; it does nothing about the pile-up. Replace the interval with a single self-chaining loop that owns both the initial load and the polling, scheduling each fetch only once the previous one has settled. Folding the mount load into the chain matters: left in its own effect, the first timer would still fire while it was in flight. Reported by Copilot on QwenLM#6561. * fix(goals): address review — clear-keyword condition, silent failures, theme vars From the /review suggestions on QwenLM#6561. Applied the ones that held up under verification; the rest are answered in the PR thread with evidence. - The New goal form accepted a clear keyword as a condition. It travels as `/goal <condition>`, so "clear" (or stop/off/reset/none/cancel) reached the daemon as a clear command: the fresh session dropped its own goal the instant it was set, with nothing to show for it. Reject it in the form. The keyword list and `/goal` arg parsing move to `utils/goalCondition.ts` so the page and App share one definition instead of the page reaching into App. - Starting a goal failed silently. `onCreateGoal` switches to the chat view first, which unmounts the Goals page, so the inline form error that `sendPrompt` rejection produced was dropped by the page's own unmount guard. Surface it as a toast instead. - `GoalsDialog.module.css` used `var(--destructive, #dc2626)`, but nothing defines `--destructive`; the hardcoded fallback stayed the same red in both themes. Use `--error-color` and match ScheduledTasksDialog's focus outline. - `recordGoalStatusItem` swallowed recording failures with a bare `catch {}`. Silently losing that write is precisely the failure this recording exists to prevent, so log it. - `GET /goals` dropped failed probes silently — an empty page and a page whose probes all failed look identical to the client. Log the dropped sessions and their reasons. Tests: clear-keyword and MAX_GOAL_LENGTH form validation, goalCondition unit tests, `sessionGoalGet` argument validation, session load surviving a throwing goal restore, `/goals` drop logging, and a regression test showing `/goal clear` sent as a prompt does persist its cleared card (a reviewer flagged this as missing; it is not). * fix(goals): cap restored conditions, keep goal-creation errors on screen Second round of review on QwenLM#6561. - `restoreGoalFromHistory` re-registered whatever condition the transcript held, skipping the 4000-char cap `/goal` enforces at set time. A transcript is a file: a corrupted or hand-edited `condition` would ride along in every judge call and continuation prompt for the rest of the session. Gate it alongside the existing trust and hook-policy gates. `MAX_GOAL_LENGTH` moves to `restoreGoal.ts` and `goalCommand.ts` imports it — the reverse direction would be a cycle, since goalCommand already depends on this module. - Starting a goal switched to the chat view before awaiting `sendPrompt`, which unmounted the Goals page. The previous commit routed the rejection to a toast, but the better fix is not to leave: switch views only once the prompt is admitted, so the error lands in the form the user is looking at. `GoalsDialog` keeps a toast fallback for the case where the page is closed while the prompt is still in flight. - Move the `debugLogger` declaration below the imports in `restoreGoal.ts`. Imports are hoisted so this compiled, but a statement wedged between two import blocks is not something to leave behind. * fix(goals): surface restore/record failures, report unprobed sessions Third round of review on QwenLM#6561. - `debugLogger.warn` no-ops unless a debug session is active (`debugLogger.ts:216`), so a failed goal restore and a failed goal-card write were both invisible in production — the two failure modes this PR exists to fix. Promote them to `writeStderrLine`, which both `ui/App.tsx` and `session/Session.ts` already use. - `GET /goals` now returns `droppedCount`. A brownout in which every probe fails returned `{ goals: [] }`, indistinguishable from a workspace with no goals — so the user re-creates goals that are already running. The Goals page shows a notice when the list is incomplete. - `running` on the wire is really "the owning session is mid-turn", which a manual prompt in that session also sets. Renamed to `hasActivePrompt` so the field reports what the daemon actually knows. The UI still maps it to Working/Waiting. - Fix the stale "keep in sync" pointer in `goalCommand.ts`: the clear keywords moved from `App.tsx` to `utils/goalCondition.ts` in the previous commit. Tests for the four coverage gaps the review named: the `systemMessage` fallback in `goalTerminalEventToHistoryItem` (including the known lossy collapse when both fields are set), `#restoreGoalOnResume` on an empty transcript, `listGoals`/`clearGoal` in `actions.ts`, and the `sendPrompt`-after- `createNewSession` failure path (added last commit). Plus `droppedCount` projection and the degradation notice. * test(goals): update the /goals integration test for droppedCount Adding `droppedCount` to the `GET /goals` payload broke the end-to-end assertions, which still expected `{ v: 1, goals: [] }`. Caught in review, not by CI: the Integration Tests job is gated off for this PR, so nothing ran these against a real daemon after the shape changed. `droppedCount: 0` is the load-bearing half of the live-session assertion. A dropped probe also yields an empty `goals`, so the old assertion could not tell a successful ext-method round trip from a silently failed one. Re-ran against a spawned `qwen serve` + `qwen --acp` child: green with the fix, red without it. * fix(goals): refuse to replay an oversized goal card `restoreGoalFromHistory` gates the condition at MAX_GOAL_LENGTH, but `HistoryReplayer` did not: a corrupted or hand-edited transcript could still ship an unbounded `condition` to every client inside `_meta.goalStatus`. Apply the same gate at the replay emit site, so neither the card nor the hook survives an oversized condition. The gate deliberately does NOT move into `parseGoalStatusItem`, which would be the tidier-looking place. `findGoalToRestore` and `findLastTerminalGoal` scan backwards and stop at the FIRST goal card they meet, so dropping a card at parse time silently promotes the card before it. A transcript ending in an oversized `cleared` would then restore the `set` that preceded it — resurrecting a goal the user explicitly cleared, the exact failure persisting `cleared` was added to prevent. Parsing therefore stays lossless and the length check lives at each consumer. Tests pin both halves: replay refuses at 4001 and emits at exactly 4000, and three scanner tests show an oversized card still wins the scan so restore can fail closed on it. * fix(goals): keep the terminal observer alive across ACP resume Addresses the latest review round on QwenLM#6561. `registerGoalHook` calls `unregisterGoalHook`, which clears the session's goal-terminal observer. The ACP restore path passes no `addItem`, so nothing reinstalled it: a restored goal reached achieved/failed/aborted with no wire update and no persisted terminal card, and the next reload revived a goal that had already finished. The no-goal branch unregisters too, so every ACP resume lost the observer, not just ones with a goal. `#restoreGoalOnResume` now reinstalls it unconditionally. A restore blocked by trust or hook policy left the client showing an active goal that nothing drives. Restore now reports `blockedBy`, and history replay emits a trailing `cleared` card naming the reason. The card is emitted, not recorded, so a later resume in a trusted folder still restores the goal. It is emitted from inside replay because `loadSession` batches replay updates into its response, and a notification sent afterwards would reach the client first. Gated behind a `HistoryReplayer` option: export and `restoreSessionHistory` render a transcript rather than resume it, and the export config is a stub that throws on any method it does not implement. Transcript payloads are now treated as untrusted. `outputHistoryItems` is checked with `Array.isArray` before iteration and each entry for being a plain object before any field is read; a hand-edited record could otherwise throw and take the whole restore down, skipping the hook while replay still showed the goal as active. Also: - Carry `setAt` across resume instead of restarting the clock, scanning back to the run's `set` card when the newest card is a `checking` card (which had no `setAt`; they now persist one). - Refuse to restore an empty condition, as `/goal` does. - Warn instead of silently no-opping when no chat recording service is present. - Cap `GET /goals` session probes at 10 in flight. - Drop `lastTerminal` from the `sessionGoalGet` response and `BridgeSessionGoal` — no consumer reads it, and it was returned unprojected. - `GoalsDialog` keeps the form and the typed condition when creation fails, and clears a stale dropped-session count when a reload fails outright. - Cross-package test pinning `GOAL_CLEAR_KEYWORDS` and `MAX_GOAL_LENGTH` against the CLI sources they mirror. * fix(goals): drop the condition length cap on restore and in the web shell QwenLM#6665 removed the 4,000-character cap `/goal` applied when setting a goal, but the restore path and the Web Shell form still enforced it. After merging main that split the surfaces: a long condition `/goal` now accepts was persisted as a `set` card, then refused by `restoreGoalFromHistory` on the next resume and dropped from the replay entirely — the goal died on reload and the user never saw a card explaining why. Remove the cap everywhere rather than reinstate it at set time. A corrupted or hand-edited transcript can now restore an arbitrarily long condition, but that is exactly what `/goal` itself permits, so it is no longer a distinct risk. The empty-condition gate stays: it is the one case that is meaningless rather than merely large. - `goalConditionBlockedBy` rejects only an empty condition. - `HistoryReplayer` no longer skips long goal cards. - `GoalsDialog` drops the form check and the `maxLength` attribute, which had been silently truncating a long condition before the user could submit it. - `MAX_GOAL_LENGTH` and the now-orphaned `goals.error.tooLong` i18n strings are deleted, along with the drift test's length half; the clear-keyword half of that test still guards the constant that is genuinely duplicated. Also drops the `MAX_GOAL_LENGTH` import QwenLM#6665 left unused in `goalCommand.ts`, which failed `eslint --max-warnings 0`. * fix(web-shell): reuse the empty session a failed goal attempt leaves behind Setting a goal starts a fresh session and then sends `/goal <condition>` into it. The daemon session is not created by the "new session" step, though — `clearSession` only detaches and clears local state. `ensureSessionForPrompt` creates the session lazily inside `sendPrompt`, so a prompt that fails after the session exists leaves a created-but-empty one behind. The Goals form keeps the condition and invites a retry, and the retry called `createNewSession()` again: the empty session from the previous attempt was abandoned and another created in its place. A user retrying a few times against a busy daemon ended up with a column of blank chats in the sidebar. Remember the stranded session and reuse it when it is still the current one, rather than creating another. Nothing is deleted — a session is only reused when the failed attempt left it empty and it has not been switched away from. Once a goal actually lands, the session belongs to it, so the next goal starts a fresh one as before. * fix(goals): forget the stranded goal session on leaving the Goals page Addresses the latest review round on QwenLM#6561. The stranded-session reuse added in bee3295 was only safe while the Goals page stayed up. Leaving it (Back button) and then talking to that session from the composer turned it into a real conversation, but the ref still pointed at it: returning to Goals and setting a goal would reuse it and drop the goal loop on top of the user's conversation — the exact thing starting a fresh session exists to prevent. The ref is now cleared whenever the view leaves 'goals', so reuse can only ever hit a session the failed attempt itself created. Also: - `registerGoalHook` rejects a `setAt` in the future, not just a non-finite or non-positive one. Every duration downstream is `Date.now() - setAt`, so a transcript claiming the goal starts tomorrow rendered negative elapsed times. - `makeRestoreInnerConfig` gains `isTrustedFolder`. Without it, `goalRestoreBlockedBy` threw `config.isTrustedFolder is not a function` on every resume in these tests, and `#restoreGoalOnResume` swallowed it — so the goal-gate assertions passed through the catch rather than the branch each one names. The hooks-disabled test now pins the branch it took, and fails if the config regresses. - The status-bar goal pill names the goal in its accessible label. The visible pill is only "◎ /goal active (2m)" and the condition lived solely in `title`, a hover tooltip screen readers do not reliably announce. - `.iconAction` gains a `:focus-visible` rule, matching `.iconButton` in DialogShell.module.css; keyboard users had no focus indicator on the clear-goal button. - `GoalsDialog.test.tsx` restores real timers in `afterEach` rather than inline per test, so a failing assertion can no longer leak fake timers into the rest of the file. - Tests for the Goals form's Cancel button and for the status-bar pill, neither of which had any coverage. * fix(goals): identify a goal run by its condition, not just its card kinds Addresses the latest review round on QwenLM#6561. `findSetAtOfRun` walked back from the active card for the `setAt` on the `set` card that opened the run, stopping at any card that was not `set`/`checking`. That assumed a terminal card always separates two goals, and a transcript is a file: hand-edited, truncated, or written by a version that did not persist terminal cards, it can hold two goals back to back. The scan then walked past the second goal's cards into the first and returned ITS start time, so the active goal's elapsed time was measured from a goal that had already ended. The condition is what identifies a run, so the scan now stops when it changes. Also: - A malformed condition is reported once on resume, not twice. `restoreGoalFromHistory` is the only caller that knows the condition is bad, and three of its four callers (the TUI ones) discard the result entirely, so it stays the reporter; `#restoreGoalOnResume` no longer adds a second line for `condition-invalid`. The env gates were already reporting exactly once. - Goal-restore stderr can no longer take down a session load. `writeStderrLine` reaches `process.stderr.write`, which throws on EPIPE or a closed fd; a throw from the catch block would have escaped into `loadSession`, so a best-effort restore would fail the very load it promises not to block. - `isGoalClearCommand` checks the `/goal` prefix instead of assuming it. `goalArgOf` returns unrecognised text unchanged, so a bare `"clear"` — an ordinary thing to type into a chat box — answered true. Latent today because every caller pre-validates the prefix, but the contract was a trap. - Tests for the throw path reinstalling the terminal observer, and for the Goals page opening a goal's session (success and failure), neither of which had any coverage. * fix(web-shell): announce Goals dialog errors and give its buttons a focus ring Addresses the latest review round on QwenLM#6561. The form-validation error and the goal-list load error were painted but never announced: `role="alert"` puts them in a live region, so a screen-reader user learns the submit was rejected instead of believing the goal was created, and learns the list went stale on a poll that failed after the page was already up. Matches the existing pattern in RewindDialog. `.primaryButton` / `.secondaryButton` had no `:focus-visible` rule, so keyboard users tabbing to Set goal / Cancel saw no focus indicator — an inconsistency with `.iconAction` and `.sessionLink` in the same file. They now take the ring the form controls already use (`outline: 2px solid var(--primary)`), offset outwards rather than inset: `.primaryButton` is filled with `--primary`, so an inset ring in that colour would be invisible on it. * fix(cli): stop a broken stderr from abandoning a transcript replay Addresses the latest review round on QwenLM#6561. `process.stderr.write` throws on EPIPE or a closed fd — reachable whenever the reader goes away (`qwen … | head`) or a daemon redirects its stderr. The goal path writes diagnostics from inside work that must not be destroyed by a failed diagnostic, and `bee3295aa` only guarded one of the five sites. The worst of the rest was in `HistoryReplayer`: the "skipping a goal card whose condition is empty" line sits inside the loop over a record's cards. A throw there abandoned that record's remaining cards, propagated to the record loop, and aborted the whole replay — the user lost their transcript because we failed to complain about one bad card. Add `writeStderrLineSafe` to stdioHelpers and route the goal path's five sites through it, replacing the one-off `#warnGoalRestore` wrapper in acpAgent so there is a single implementation. It is deliberately not the default: `writeStderrLine` still throws, because most of the CLI wants a broken stderr to be loud. This variant is for writes that are incidental to real work. Also adds the first tests for `stdioHelpers`, and covers two untested Goals dialog behaviours: the Refresh button, and the clear button disabling itself while its clear is in flight (a double-click otherwise fired two concurrent clears at the same session). * fix(web-shell): keep the Goals page mounted across createNewSession main's `createNewSession` gained a `setMainView('chat')` of its own, fired synchronously before any await. That silently defeated the Goals handler's deferred switch: by the time `sendPrompt` rejected, the page — and the form that renders the error — was already gone, dropping the user into an empty chat with no explanation. This is the exact failure the deferred switch was written to prevent; the two changes only had to meet for it to come back. `createNewSession` takes a `keepView` opt-out, and the Goals handler uses it, so the page survives until the prompt is admitted. Saving and restoring `mainView` around the call would also work but flips the view to chat and back, which the user would see. A test pins the page staying mounted across a failed submit; it fails if `keepView` stops being honoured. Also from the same round: - `registerGoalHook`'s `initialSetAt` guards are now tested — a future timestamp, NaN, Infinity, 0 and a negative all fall back to now, and a usable value survives. The future case is the one with teeth: `Date.now() - setAt` renders a negative elapsed time rather than failing loudly, and nothing covered it. - The goals list carries `role="list"` / `role="listitem"`. They are divs, and even a real `<ul>` loses its implicit role under `display: flex` in Safari. - The open-session button names the action *and* the session. Its visible text is only the session name, which says nothing about what activating it does; the name stays in the accessible name so it still contains the visible label. - `.fieldLabel` matches ScheduledTasksDialog's `--muted-foreground`. The two dialogs sit side by side and had drifted. Not taken: deferring `setMainView` in `onOpenSession` until the load resolves. The sibling `handleOpenSessionFromOverview` switches first by the same pattern, and `loadSidebarSession` clears the transcript and shows a loading skeleton — which is the feedback for the common success path. Deferring would leave a click looking dead until the load lands, and would make Goals diverge from the Session Overview panel. If we want that behaviour it should change both. * fix(web-shell): stop the visuals spec asserting a badge QwenLM#7035 removed The "Capture web-shell visuals" job fails on this PR at `screenshots.spec.ts:395`, asserting the sidebar's "Primary" badge is visible: Error: expect(locator).toBeVisible() failed Error: element(s) not found Not from this branch. The chain is on main: - 2026-07-15 QwenLM#6880 adds the visuals spec, asserting the "Primary" badge — correct at the time. - 2026-07-17 QwenLM#7035 drops that badge as redundant (the workspace selector's checkmark already conveys the default target), removing the `primaryLabel` prop and its `<span className={styles.badge}>` render, and updates the *unit* test to assert its absence — but leaves this spec asserting it is visible. The capture job only runs on pull requests (it needs a PR head and a merge-base), so main never went red for it and the breakage surfaces on the next PR to merge main — this one. Assert the badge's absence instead of deleting the check, mirroring the unit test QwenLM#7035 added, so a regression re-adding it still fails here. --------- Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
This was referenced Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PRs that touch the web-shell UI now get an auto-updated comment with light/dark screenshots of key views and short GIF recordings of common flows, so reviewers can see the rendered UI without checking out the branch. Everything renders against the existing mock daemon — no real backend, no model, no secrets.
Captured today:
.webmrecordings are attached to the workflow runHow it's wired (security)
Capture runs untrusted PR code, so it's split into two workflows and the write token is never in the same job as PR code:
web-shell-visuals.ymlpull_request(web-shell client paths)contents: read, no secretsweb-shell-visuals-publish.ymlworkflow_run(base context)actions: read+CI_BOT_PATpr-assets/web-shell-visuals-<n>branch (immutable commit SHA) → post/update one inline comment. Never checks out PR code.Fork PRs run capture with a read-only token and no secrets (standard
pull_requestisolation). The privileged publish step only ever handles opaque image bytes plus a PR number it validates and binds to the run's authenticated head SHA (so a forged PR number can't redirect the comment).Capture infra
Self-contained in
packages/web-shell, reusing the existing mock-daemon e2e harness:playwright.visuals.config.ts— dedicated config, isolated from the smoke suiteclient/e2e/visuals/{harness,screenshots.spec,flows.spec}.tsnpm run test:e2e:visuals --workspace=packages/web-shellVerification
run:scripts passbash -n; ESLint + Prettier clean on new TS. actionlint/shellcheck run in CI.workflow_runhandoff,pr-assetspush, comment) first exercises on a real PR once this is on the default branch.中文说明
做了什么
改动 web-shell UI 的 PR 会自动收到一条(自动更新的)评论,内含关键视图的 light/dark 截图和常见操作的 GIF 录屏,评审无需 checkout 分支即可看到渲染效果。全部基于现成的 mock daemon 渲染——无需真后端、真模型、真密钥。
当前覆盖:
.webm高清录像挂在 workflow run 的 artifact 里安全架构
截图 job 会执行不可信 PR 代码,因此拆成两个 workflow,写 token 绝不与跑 PR 代码同 job:
web-shell-visuals.yml(pull_request触发,web-shell client 路径):contents: read、无密钥;checkout PR head → 构建 → Playwright 截图/录像 → ffmpeg 转 GIF → 上传 artifact。fork PR 以只读 token、无密钥运行。web-shell-visuals-publish.yml(workflow_run,base 上下文):actions: read+CI_BOT_PAT;下载 artifact → 用 run 的已认证 head SHA 绑定到真实 PR(伪造 PR 号无法把评论导向他人)→ 把图片托管到每 PR 独立的pr-assets/web-shell-visuals-<n>分支(按 commit SHA 引用,URL 不失效)→ 发/更新一条内联评论。从不 checkout PR 代码。本地验证
run:脚本bash -n通过;新增 TS 的 ESLint/Prettier 干净。actionlint/shellcheck 由 CI 跑。workflow_run交接、pr-assets推送、发评论)会在合入默认分支后,于后续 web-shell PR 上首次实跑。