Skip to content

perf(web-shell): paint the composer git chip before git status completes#7680

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:perf/webshell-git-chip-fast-path
Jul 25, 2026
Merged

perf(web-shell): paint the composer git chip before git status completes#7680
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:perf/webshell-git-chip-fast-path

Conversation

@wenshao

@wenshao wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Makes the git branch chip in the Web Shell composer appear almost instantly when opening a new session. The daemon's git-status route now answers from a per-workspace cached summary — recomputed in the background with in-flight dedup and a short throttle, and pushed to clients over SSE only when the summary actually changes — while a new opt-in wait query flag preserves the previous blocking behavior for callers that need it. The composer issues both a fast (cached) and a fresh (blocking) request on each refresh; the two share a single daemon-side git status computation, so the branch name paints in single-digit milliseconds and the dirty/ahead/behind counters land as soon as the working-tree scan finishes. The fresh request also covers the no-session state (before the first prompt), where no per-session event stream exists and an SSE push alone would never arrive. Sidebar workspace chips explicitly opt into the blocking behavior, and worktree sessions keep their direct, uncached computation.

Why it's needed

Previously, creating a new session left the composer without a git chip for hundreds of milliseconds to several seconds: every status request synchronously spawned git status --porcelain behind the HTTP response (no caching, no dedup), and the chip rendered nothing until the full round trip completed. The branch name alone is available in milliseconds from a lightweight HEAD read with a live reflog watcher, but the response held it hostage to the heavy working-tree computation. On top of that, the same route was hit twice concurrently on connect (session metadata prefetch and the composer effect), spawning two identical git subprocesses.

Reviewer Test Plan

How to verify

  • Unit tests: daemon git state (cache hit/miss, in-flight dedup, throttle, publish-only-on-change, wait semantics, failure keeps cache, dispose suppresses publish), route wait flag passthrough, SDK query-string building, webui event mapping (workspace guard), and web-shell composer fast+fresh double fetch plus SSE mirror — all added or updated and green. npm run build and npm run typecheck are clean.
  • Protocol-level E2E against a real daemon (numbers in the E2E comment below): with a cold cache the fast request returns a branch-only body in ~3ms vs ~25ms for the blocking one; a git_status_changed event follows on the session event stream; a second fast request serves the enriched cache immediately; touching an untracked file triggers one new push, and a quiet period triggers none.
  • Manual: open the Web Shell and start a new session — the branch chip should render immediately with the branch name, with counters appearing shortly after (most visible in a large repository). Sidebar folder chips should still show their counters. Switching branches should still update the chip via the existing live branch event.

Evidence (Before & After)

Before: new-session composer shows no git chip until a full git status --porcelain round trip completes (hundreds of ms on real repositories; up to the 5s subprocess timeout in the worst case). After (protocol timings, clean small repo): fast request returns {v: 2, workspaceCwd, branch: "main"} with no enriched fields in 3.3ms, vs 25.5ms for the same request with the blocking flag — and the enriched summary follows via the fresh request / SSE push. In the target scenario (new session in a large working tree) the perceived chip latency drops from the full git status duration to one local HTTP round trip.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested (CI)
🐧 Linux ⚠️ not tested (CI)

Environment (optional)

Local daemon from the built bundle (node packages/cli/dist/index.js serve --workspace <repo> --no-web --safe-mode) with curl timing and a raw SSE subscription; unit tests via per-package vitest runs.

Risk & Scope

  • Main risk or tradeoff: the default (no-flag) route can now return last-known data — branch-only on a cold cache, or a summary that is one background-refresh behind — instead of always blocking for a fresh computation. Every existing caller was audited: the session provider only reads the branch (kept live by the reflog watcher, so it is never stale), the composer is the intended beneficiary, and the sidebar was moved to the blocking flag explicitly. A summary computed while git status fails keeps the previous cache and stays silent.
  • Not validated / out of scope: browser-level visual assertions (covered by protocol E2E timings plus unit tests instead); worktree sessions intentionally keep the uncached direct computation to avoid leaking one fs watcher per worktree; no daemon warm-up at startup.
  • Breaking changes / migration notes: none — the response shape is unchanged (enriched fields were already optional), the wait query flag and the git_status_changed event are purely additive, and older SDK clients silently drop the unknown event.

Linked Issues

N/A — raised from direct usage feedback.

中文说明

让 Web Shell 输入框里的 git 分支 chip 在新建会话时几乎立即出现。daemon 的 git status 路由改为从每个工作区的缓存摘要直接应答——后台异步重算(带 in-flight 去重和短节流),且仅在摘要真正变化时通过 SSE 推送给客户端;同时新增可选的 wait 查询参数,为需要的调用方保留原来的阻塞语义。composer 每次刷新时并发发出 fast(读缓存)和 fresh(阻塞等新鲜结果)两个请求,二者在 daemon 端共享同一次 git status 计算,因此分支名在几毫秒内渲染出来,dirty/ahead/behind 等计数器在工作区扫描完成后立即补齐。fresh 请求还覆盖了无会话状态(首个 prompt 之前)——此时不存在按会话的事件流,仅靠 SSE 推送永远到不了。侧栏工作区 chip 显式选择保留阻塞语义;worktree 会话保持直接计算、不进缓存。

此前,新建会话后 composer 的 git chip 要等几百毫秒到数秒才出现:每个 status 请求都在 HTTP 响应前同步 spawn git status --porcelain(无缓存、无去重),chip 在整个往返完成前完全不渲染。分支名本身通过轻量的 HEAD 读取加 reflog watcher 毫秒级可得,却被重量的工作区计算拖住。此外,连接建立时同一路由会被并发打两次(会话元数据预取和 composer effect),spawn 两个相同的 git 子进程。

验证方式:单测覆盖 daemon git state(缓存命中/未命中、in-flight 去重、节流、仅变化才推送、wait 语义、失败保留缓存、dispose 后不再推送)、路由 wait 参数透传、SDK query 拼接、webui 事件映射(workspace 守卫)、web-shell composer 双发与 SSE 镜像,全部新增或更新并通过;npm run buildnpm run typecheck 干净。协议级 E2E(真实 daemon,数据见下方 E2E 评论):冷缓存 fast 请求约 3ms 返回 branch-only,阻塞请求约 25ms;会话事件流随后收到 git_status_changed;第二次 fast 请求立即返回 enrich 后的缓存;新增未跟踪文件触发一次新推送,静置期不推送。手动验证:打开 Web Shell 新建会话,chip 应立即显示分支名,计数器随后补齐(大仓库更明显);侧栏文件夹 chip 计数器不变;切换分支仍能通过既有的分支事件更新 chip。

主要风险:默认(不带参数)路由现在可能返回 last-known 数据(冷缓存时 branch-only,或落后一次后台刷新的摘要),而非总是阻塞等待新鲜计算。已逐一审计所有现存调用方:session provider 只读分支(由 reflog watcher 保持实时,不会过期),composer 是本次优化的服务对象,侧栏已显式改用阻塞参数。git status 失败时保留旧缓存且不发事件。未验证/范围外:浏览器级视觉断言(由协议级 E2E 计时和单测覆盖);worktree 会话有意保持无缓存直接计算(避免每个 worktree 泄漏一个 fs watcher);无 daemon 启动预热。无破坏性变更:响应形状不变(enriched 字段本就可选),wait 参数与 git_status_changed 事件均为纯新增,旧版 SDK 客户端会静默丢弃未知事件。

New sessions gated the chip on a full `git status --porcelain` subprocess
behind GET /workspaces/:ws/git, so the branch chip appeared hundreds of
milliseconds (worst case seconds) after the composer was ready.

The daemon now keeps a per-workspace last-known summary with in-flight
dedup and a 2s background-refresh throttle: the default GET returns the
cached status (branch-only on a cold start) immediately and recomputes in
the background, publishing git_status_changed over SSE only on a delta,
while ?wait=1 keeps the previous blocking semantics. The composer fetches
both paths concurrently — the fresh GET also covers the no-session state,
which has no per-session SSE stream — so the branch paints in ~3ms and
the counters land when the computation finishes. The sidebar keeps
wait:true since it has no SSE fill-in path.
@wenshao

wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

E2E test report (protocol-level, real daemon)

Environment: macOS, node packages/cli/dist/index.js serve --port 4291 --workspace <clean git repo> --no-web --safe-mode (loopback), curl timing + raw SSE subscription. All 6 protocol assertions PASS.

# Assertion Result Evidence
1 Cold-cache fast GET is branch-only and fast {v:2, workspaceCwd, branch:"main"}, no computedAt, no enriched fields — 3.3ms vs 25.5ms for ?wait=1 (~7.6x)
2 git_status_changed follows on the event stream One event after the fast GET; data carries the full summary incl. computedAt
3 Warm cache serves last-known immediately Second fast GET 2.5ms, enriched, computedAt matches the fresh computation
4 ?wait=1 keeps blocking semantics Full summary; forces a fresh computation inside the throttle window (new untracked file visible immediately)
5 No push when the summary is unchanged After a 3s quiet period (past the 2s throttle), fast GET → event count stays 1; computedAt silently refreshes
6 Push on real change touch a file → fast GET returns the stale cache in 4.7ms (untracked:0) → second git_status_changed arrives (untracked:1) → next GET is enriched

Notes:

  • Typical fast-GET latency measured at 1–5ms (0.9/1.1/2.1/2.5/3.3/4.7ms); one 71ms outlier while the background git status subprocess was being spawned (event-loop scheduling noise), settling back to 1–2ms.
  • Daemon log clean (no errors/drops); route log shows GET /workspace/git at 0–2ms.
  • Browser-level visual assertions were not run (protocol timings + unit coverage instead): the composer's data path is fast GET + ?wait=1 fresh (assertions 1/3/4), the sidebar is wait: true (assertion 4), and the SSE realtime path (assertions 2/6).

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments above for the result.

Qwen Triage 已完成 —— 查看运行。结果见上方各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed and measured — the composer git chip waits for a full git status --porcelain round trip (hundreds of ms, up to the 5s subprocess timeout) before rendering anything, even though the branch name alone is available in milliseconds from a lightweight HEAD read. The PR description includes protocol-level timings (3.3ms fast vs 25.5ms blocking on a clean small repo) and identifies four distinct contributing factors, including duplicate concurrent git status spawns on connect.

Direction: aligned — this is a straightforward UX latency fix for the Web Shell composer, squarely within the project's scope. The stale-while-revalidate pattern with SSE push-on-change is the right architecture for this kind of "show something fast, fill in details later" problem. CHANGELOG: no direct reference, but the area (Web Shell git chip responsiveness) is clearly relevant.

Size: cross-package (cli, sdk-typescript, webui, web-shell) — 299 production logic lines, 551 test lines, 249 docs lines. Under the 500-line threshold; no maintainer escalation needed.

Approach: the scope feels right. Every layer change (daemon cache → SDK event → webui mapper → web-shell consumer) is necessary for the end-to-end behavior. The design doc's rejected alternatives (TTL-only cache, daemon pre-warm, frontend-only dedup) are well-reasoned. The P2 frontend request dedup was correctly scoped out after the daemon-side in-flight dedup made it redundant. No unrelated changes or drive-by refactors.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测且有测量数据——composer git chip 必须等待完整的 git status --porcelain 往返(数百毫秒,最坏情况达 5s 子进程超时)才开始渲染,尽管分支名本身通过轻量 HEAD 读取毫秒级可得。PR 描述包含协议级计时(小仓库冷缓存 fast 3.3ms vs 阻塞 25.5ms),并识别了四个不同的贡献因素,包括连接时重复并发 spawn git status

方向:对齐——这是 Web Shell composer 的直接 UX 延迟修复,完全在项目范围内。stale-while-revalidate + SSE 变化推送是"先快速展示、再补齐细节"这类问题的正确架构。CHANGELOG:无直接引用,但领域(Web Shell git chip 响应速度)明显相关。

规模:跨包(cli、sdk-typescript、webui、web-shell)——299 行生产逻辑、551 行测试、249 行文档。低于 500 行阈值,无需维护者升级。

方案:范围合理。每层改动(daemon 缓存 → SDK 事件 → webui mapper → web-shell 消费)对端到端行为都是必要的。设计文档中被否决的备选方案(纯 TTL 缓存、daemon 预热、前端去重)论证充分。P2 前端请求去重在 daemon 端 in-flight 去重使其多余后被正确裁剪。无无关改动或顺手重构。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 1cd94891882207b7c0e71c00a7d5258d46b423c4 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 1cd9489. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:

  • packages/web-shell/client/App.tsx
  • packages/web-shell/client/components/sidebar/WorkspaceSection.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 1cd9489, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: I would have done exactly what this PR does — add a per-workspace cached GitWorkingTreeStatus to WorkspaceGitEntry, return it immediately on the fast path (branch-only when cold), kick a throttled background refresh with in-flight dedup, publish git_status_changed via SSE only on delta, add a wait flag for callers that need blocking semantics, and have the composer double-fetch (fast + fresh) so the chip paints instantly and counters fill in without depending on a session SSE stream that doesn't exist yet.

Comparison with the diff: the PR's approach matches my independent proposal at every layer. No simpler path was missed.

No critical findings. Specific observations:

  • The sameWorkingTreeStatus exhaustiveness guard (compile-time Record<keyof GitWorkingTreeStatus, true>) is a nice touch — adding a field to the status type forces a compile error until the comparison is updated.
  • The publishWorkspaceEvent call is wrapped in try/catch so an SSE fan-out failure can't reject an awaited wait:true route or drop the cache. Correct.
  • The disposed flag is checked in both the refresh callback and the branch-watcher callback, with test coverage for both paths.
  • All prior review feedback has been addressed: writeStderrLinewriteStderrLineSafe (EPIPE safety), stdio mock in tests, console.warn on the fresh-path catch, disposed-watcher test, and writeStderrLineSafe assertion in the failure-path test.
  • The data as unknown as DaemonWorkspaceGitStatus cast in mappers.ts is a double cast, but it follows the existing pattern for daemon events in this file and the workspace guard provides the safety net.

CI Test Evidence

All substantive checks passed on the reviewed commit. Windows and macOS unit tests were skipped (fork PR restriction). No failures.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
precheck-pr / precheck ✅ success
Real daemon E2E / Java 11 ✅ success
windows-latest / Java 21 ✅ success
macos-latest / Java 21 ✅ success
ubuntu-latest / Java 21 ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
Test (windows-latest, Node 22.x) ⏭️ skipped
Test (macos-latest, Node 22.x) ⏭️ skipped
Integration Tests (CLI, No Sandbox) ⏭️ skipped

Real-Scenario Testing

N/A — the change is in the Web Shell (browser-based UI), not the terminal UI. The daemon-side behavior is covered by the protocol-level E2E evidence in the PR description (cold-cache fast request 3.3ms vs 25.5ms blocking, SSE push on change, in-flight dedup verified). A maintainer can trigger the isolated @qwen-code /tmux job if terminal-surface regression coverage is desired, though this PR does not touch any TUI code path.

中文说明

代码审查

独立方案: 我的方案与本 PR 完全一致——在 WorkspaceGitEntry 中增加每工作区的 GitWorkingTreeStatus 缓存,fast path 立即返回(冷缓存时 branch-only),触发带节流的后台刷新(in-flight 去重),仅在摘要变化时通过 SSE 推送 git_status_changed,为需要阻塞语义的调用方增加 wait 参数,composer 双发(fast + fresh)使 chip 即时渲染、计数器无需依赖尚不存在的会话 SSE 流即可补齐。

与 diff 对比: PR 方案在每一层都与我的独立方案一致,未发现更简路径。

无关键发现。具体观察:

  • sameWorkingTreeStatus 的穷举守卫(编译期 Record<keyof GitWorkingTreeStatus, true>)很好——给 status 类型加字段会强制编译报错直到比较逻辑更新。
  • publishWorkspaceEvent 调用包在 try/catch 中,SSE 扇出失败不会 reject 被 await 的 wait:true 路由,也不会丢弃缓存。正确。
  • disposed 标志在刷新回调和分支 watcher 回调中都有检查,且两条路径都有测试覆盖。
  • 所有先前评审反馈已处理:writeStderrLinewriteStderrLineSafe(EPIPE 安全)、测试中 mock stdio、fresh-path catch 加 console.warn、disposed-watcher 测试、失败路径测试中断言 writeStderrLineSafe
  • mappers.ts 中的 data as unknown as DaemonWorkspaceGitStatus 双重转换,但遵循该文件中 daemon 事件的既有模式,workspace 守卫提供了安全网。

CI 测试证据

审查提交上所有实质性检查通过。Windows 和 macOS 单测被跳过(fork PR 限制)。无失败。

真实场景测试

不适用——改动在 Web Shell(浏览器 UI),非终端 UI。daemon 端行为由 PR 描述中的协议级 E2E 证据覆盖(冷缓存 fast 请求 3.3ms vs 阻塞 25.5ms、SSE 变化推送、in-flight 去重验证)。如需终端界面回归覆盖,维护者可触发隔离的 @qwen-code /tmux 任务,但本 PR 未触及任何 TUI 代码路径。

Qwen Code · qwen3.8-max-preview

Reviewed at 1cd94891882207b7c0e71c00a7d5258d46b423c4 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean across every stage; would merge without hesitation.

Re-run at the current head after five rounds of review feedback were addressed. The PR is in better shape than at the initial triage: the EPIPE-unsafe writeStderrLine is now writeStderrLineSafe, the fresh-path catch logs instead of silently swallowing, the disposed branch-watcher guard has a dedicated test, and the stdio mock/assertion gaps are closed.

The design is the right one — stale-while-revalidate with in-flight dedup and SSE push-on-change is exactly how you decouple a fast branch read from a slow working-tree scan. The implementation mirrors the existing git_branch_changed pattern at every layer (daemon → SDK → webui → web-shell), which means a maintainer reading this in six months will recognize every piece. The sameWorkingTreeStatus exhaustiveness guard is the kind of defensive detail that prevents silent regressions when the status type grows.

Every call site in the codebase was audited and migrated: the session provider reads only the branch (kept live by the watcher), the composer gets the fast+fresh double fetch, the sidebar explicitly opts into blocking semantics, and worktree sessions keep their direct uncached computation. The test suite covers cold cache, dedup, throttle, wait semantics, failure-keeps-cache, dispose-suppresses-publish, workspace guard, double fetch, SSE mirror, and the worktree skip — 17 daemon state tests, 8 route tests, plus SDK, webui, and web-shell tests.

CI is fully green on the reviewed commit (ubuntu unit, web-shell visuals, serve A/B, web-shell E2E smoke, daemon E2E, precheck, Java matrix). The stale CHANGES_REQUESTED review from the autofix bot reflects issues that have since been fixed in subsequent pushes.

中文说明

置信度:5/5 —— 每个阶段都干净,毫不犹豫可以合并。

在处理完五轮评审反馈后于当前 head 重新审查。PR 状态优于初次 triage:EPIPE 不安全的 writeStderrLine 已替换为 writeStderrLineSafe,fresh-path catch 改为日志输出而非静默吞掉,disposed 分支 watcher 守卫有了专门测试,stdio mock/断言缺口已关闭。

设计正确——stale-while-revalidate + in-flight 去重 + SSE 变化推送正是将快速分支读取与慢速工作区扫描解耦的方式。实现在每一层(daemon → SDK → webui → web-shell)都镜像了既有的 git_branch_changed 模式,维护者六个月后阅读时每个部分都会很熟悉。sameWorkingTreeStatus 的穷举守卫是防止 status 类型扩展时静默回归的那种防御性细节。

代码库中每个调用点都已审计并迁移:session provider 只读分支(由 watcher 保持实时),composer 获得 fast+fresh 双发,侧栏显式选择阻塞语义,worktree 会话保持直接无缓存计算。测试套件覆盖冷缓存、去重、节流、wait 语义、失败保留缓存、dispose 抑制推送、workspace 守卫、双发、SSE 镜像和 worktree 跳过——17 个 daemon state 测试、8 个路由测试,加上 SDK、webui 和 web-shell 测试。

CI 在审查提交上全绿(ubuntu 单测、web-shell 视觉、serve A/B、web-shell E2E smoke、daemon E2E、precheck、Java 矩阵)。autofix bot 的过期 CHANGES_REQUESTED 评审反映的问题已在后续推送中修复。

Qwen Code · qwen3.8-max-preview

Reviewed at 1cd94891882207b7c0e71c00a7d5258d46b423c4 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx
@wenshao

wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen- code /takeover

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/workspace-git-state.ts
@wenshao

wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Jul 24, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@wenshao

wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review — #7680 perf(web-shell): paint the composer git chip before git status completes

Reviewed the full diff (16 files, +906/-45) against the base tree. Overall this is a well-designed, well-tested change with an honest caller audit and no blocking issues. Details below.

Overview

Turns the daemon's git-status route into a stale-while-revalidate cache: the default path returns last-known (branch-only on a cold cache) immediately and kicks a throttled, in-flight-deduped background git status that pushes git_status_changed over SSE only on a delta; an opt-in ?wait=1 preserves the old blocking semantics. The composer fires a fast + a fresh (wait:true) request that share one daemon-side computation, so the branch paints in ~ms and counters fill in when the scan lands. The fresh request covers the no-session state (no per-session SSE stream exists before the first prompt). Sidebar chips opt into wait:true; worktree ?cwd= reads stay on the direct, uncached path.

Strengths

  • Cache invariants are tight. sameWorkingTreeStatus compares all 11 fields of GitWorkingTreeStatus (branch, detached, hasUpstream, ahead, behind, staged, unstaged, untracked, conflicted, stashCount, operation) — verified against gitDiff.ts:1172, nothing is missed, so "publish only on change" won't silently swallow a real delta.
  • Concurrency is handled correctly. In-flight dedup via statusPromise, throttle keyed on refreshStartedAt (bypassed by wait), .finally clears the promise only when it's still the current one, and the disposed guard prevents a late refresh from publishing after teardown. Each is covered by a dedicated test.
  • getStatus failure path is safe. A failed/null compute keeps the previous cache and stays silent (no publish, no reject) — and the SSE publish is wrapped so fan-out can't reject an awaited wait:true route into a 500.
  • Caller audit is accurate. I confirmed both DaemonSessionProvider call sites read only gitResult.value.branch (lines 841-842, 1348-1349), which the reflog watcher keeps live — so leaving them on the default fast path is correct, not a stale-data regression. The two callers that need counters (composer, sidebar) are handled explicitly.
  • Backward compatible. Additive git_status_changed event (old SDKs drop it via asKnownDaemonEvent), optional ?wait=1, unchanged response shape. normalizer.ts returns [] mirroring git_branch_changed.
  • Test coverage is thorough across all five layers (state, route, SDK query-building, webui mapper incl. the workspace-mismatch guard, and web-shell fast+fresh + SSE mirror).

Minor notes (non-blocking)

  1. wait:true no longer degrades to branch-only on a transient failure once a cache exists. Previously the sidebar (blocking) would drop to branch-only when git status failed; now it returns the last-known enriched summary (workspace-git-state.test.ts "keeps the cached summary … when a refresh fails"). This is arguably better (no counter flicker), but it is a behavior change for the sidebar worth being explicit about — a genuinely broken git now shows stale counters rather than blanking them.

  2. Fast-path .catch can theoretically clobber a resolved fresh result. In App.tsx, the fast request's .catch(() => setSelectedWorkspaceGitStatus(undefined)) and the fresh request's .then(setSelectedWorkspaceGitStatus) are independent promises. If the fast request were to reject after the fresh one resolves, the chip would blank. In practice fast does a strict subset of fresh's work and resolves first, so the window is essentially nil — but if you want to harden it, either let the fresh request be the sole error-clearer or skip the setUndefined when a fresh request is in flight.

  3. Redundant double setState on active sessions. The wait:true response and the SSE mirror both write the same enriched status → one extra re-render. Harmless (the hidden toolbar-measure copy absorbs width shift), just noting it.

  4. Worktree "fast" isn't fast. The composer's fast request for a worktree session uses ?cwd=, which the route computes directly (no cache), so the chip won't paint faster for worktree sessions. This is correctly acknowledged as out of scope; flagging only so it isn't mistaken for a regression during manual testing.

Conventions / performance / security

  • Conventions: matches surrounding style; comments explain the why (throttle rationale, no-SSE-before-first-prompt, worktree bypass) rather than the what. ?cwd= retains its realpath containment check — unchanged and correct.
  • Performance: net subprocess count for the composer is unchanged (fast+fresh share one compute via dedup), the throttle bounds focus-storm spawns on the reactive path, and git_status_changed only fires on real deltas — so no 30s-poll re-render storm. This is a clear win on perceived latency.
  • Security: no new inputs beyond ?wait=1 (strict === '1'); SSE fan-out stays on the existing per-workspace bus with its workspaceCwd guard on both the daemon publish and the webui mapper. No concerns.

Verdict

LGTM. Correctness, conventions, and test coverage are all solid; the notes above are polish, not blockers. Worth a one-line acknowledgement in the PR body that wait:true now returns last-known (rather than branch-only) on transient git failure, since that's the one user-observable semantic shift not called out in "Breaking changes".

中文说明

已审阅完整 diff(16 文件,+906/-45)。整体是一个设计良好、测试充分的改动,调用方审计诚实、无阻塞性问题。

做得好的地方:

  • sameWorkingTreeStatus 覆盖了 GitWorkingTreeStatus 全部 11 个字段(对照 gitDiff.ts:1172 核实),"仅变化才推送"不会漏掉真实 delta。
  • 并发处理正确:statusPromise in-flight 去重、refreshStartedAt 节流(wait 绕过)、.finally 仅在仍是当前 promise 时清空、disposed 守卫防止 dispose 后推送——每项都有对应单测。
  • 失败路径安全:计算失败保留旧缓存且不推送;SSE 推送被 try/catch 包裹,不会把 wait:true 路由变成 500。
  • 调用方审计准确:两处 DaemonSessionProvider 只读 .branch(第 841-842、1348-1349 行),由 reflog watcher 保持实时,走默认 fast path 无回归。需要计数器的 composer/sidebar 已显式处理。
  • 向后兼容:git_status_changed 纯新增(旧 SDK 静默丢弃)、?wait=1 可选、响应形状不变。
  • 五层测试覆盖完整。

小提示(非阻塞):

  1. wait:true 在已有缓存时,git 瞬时失败不再降级为 branch-only,而是返回旧的 enrich 摘要——对 sidebar 是行为变化(少了闪烁但会显示过期计数器),建议在"破坏性变更"里点一句。
  2. App.tsx 中 fast 请求的 .catchsetUndefined;理论上若 fast 在 fresh 之后 reject 会清空 chip。实际几乎不可能(fast 是 fresh 的子集、先返回),但可让 fresh 作为唯一的错误清理方以加固。
  3. active session 下 wait:true 响应与 SSE 镜像会各 setState 一次——多一次无害的 re-render。
  4. worktree 会话的"fast"请求走 ?cwd= 直接计算,不进缓存,chip 不会更快——已在范围外声明,仅提醒手测时勿误判为回归。

结论:LGTM。 正确性、规范、测试都扎实;上述均为打磨项。建议在 PR body 里补一句"wait:true 在 git 瞬时失败时现在返回 last-known 而非 branch-only",这是唯一未被点出的可观测语义变化。

🤖 Generated with Claude Code

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

1. Worktree sessions skip wait: true — regression test (rc:3646502552)

[Suggestion] No test verified that worktree sessions skip the wait: true fresh path. If the if (!sessionWorktree) guard were removed or inverted, a worktree session would fire workspaceGit({ wait: true }) without a cwd parameter, overwriting the chip with the main workspace's git status.

Decision: Implemented. Added a test in App.test.tsx that sets up a worktree session (via sessionStatus returning worktree metadata), waits for the workspaceGit({ cwd: <worktree-path> }) call, then asserts no { wait: true } call follows it.

Files changed: packages/web-shell/client/App.test.tsx

2. Exhaustiveness guard for sameWorkingTreeStatus (rc:3647143953)

[Suggestion] sameWorkingTreeStatus in packages/cli manually compares all fields of GitWorkingTreeStatus (defined in packages/core). A future field addition to the type would not be flagged by TypeScript, silently making the comparator treat different summaries as equal and stopping git_status_changed events for that delta.

Decision: Implemented. Added a Record<keyof GitWorkingTreeStatus, true> exhaustiveness guard that fails to compile if a field is added to GitWorkingTreeStatus without updating the comparator. Follows the codebase's existing _exhaustive pattern (e.g. coreToolScheduler.ts, prompts.ts).

Files changed: packages/cli/src/serve/workspace-git-state.ts

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • vitest packages/cli src/serve/workspace-git-state.test.ts — 15 passed
  • vitest packages/cli src/serve/routes/workspace-git.test.ts — 8 passed
  • vitest packages/web-shell client/App.test.tsx — 171 passed
中文说明

已处理的审查反馈

1. Worktree 会话跳过 wait: true — 回归测试 (rc:3646502552)

[建议] 此前没有测试验证 worktree 会话会跳过 wait: true 的新鲜数据请求路径。如果 if (!sessionWorktree) 守卫被移除或反转,worktree 会话将不带 cwd 参数触发 workspaceGit({ wait: true }),导致芯片被主工作区的 git 状态覆盖。

决定: 已实现。在 App.test.tsx 中新增测试,通过 sessionStatus 返回 worktree 元数据来设置 worktree 会话,等待 workspaceGit({ cwd: <worktree-path> }) 调用出现,然后断言其后没有 { wait: true } 调用。

修改文件: packages/web-shell/client/App.test.tsx

2. sameWorkingTreeStatus 的穷举守卫 (rc:3647143953)

[建议] packages/cli 中的 sameWorkingTreeStatus 手动比较 GitWorkingTreeStatus(定义在 packages/core)的所有字段。如果将来向该类型添加新字段,TypeScript 不会标记此比较器,导致不同摘要被视为相同,git_status_changed 事件将不再针对该差异触发。

决定: 已实现。添加了 Record<keyof GitWorkingTreeStatus, true> 穷举守卫,如果向 GitWorkingTreeStatus 添加字段而未更新比较器,编译将失败。遵循代码库中现有的 _exhaustive 模式(如 coreToolScheduler.tsprompts.ts)。

修改文件: packages/cli/src/serve/workspace-git-state.ts

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • vitest packages/cli src/serve/workspace-git-state.test.ts — 15 个通过
  • vitest packages/cli src/serve/routes/workspace-git.test.ts — 8 个通过
  • vitest packages/web-shell client/App.test.tsx — 171 个通过

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/workspace-git-state.ts
Comment thread packages/cli/src/serve/workspace-git-state.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

rc:3647718417 — sameWorkingTreeStatus branch comparison (Suggestion)

Decision: Implemented (comment added)

The reviewer noted that materialize() overlays entry.branch (from the reflog watcher) on top of status.branch, so a branch-only delta in sameWorkingTreeStatus can fire a redundant SSE event. Dropping the comparison was considered but rejected: when the watcher lags behind a checkout, status.branch is the first signal that something changed — suppressing it would delay the delta until the next watcher tick. Added a three-line comment explaining the intentionally conservative publish behavior.

Change: packages/cli/src/serve/workspace-git-state.ts — comment before the return in sameWorkingTreeStatus.

rc:3647718420 — Missing test for publishWorkspaceEvent throw path (Suggestion)

Decision: Implemented (test added)

The catch {} around publishWorkspaceEvent in startRefresh is load-bearing — it prevents a throwing SSE fan-out from rejecting an awaited wait:true route call into a 500. No existing test exercised this path. Added a test that makes publishWorkspaceEvent throw on the first call, then asserts: (1) getStatus({ wait: true }) still resolves with the computed summary, and (2) the cache is populated (a follow-up fast call returns the enriched status with computedAt).

Change: packages/cli/src/serve/workspace-git-state.test.ts — new test case "keeps the cache and resolves wait:true when publishWorkspaceEvent throws".

@wenshao issue-level comment — LGTM with minor notes

Decision: No code change needed

Acknowledged. The four minor notes (wait:true returning last-known on transient failure, fast-path .catch race, redundant double setState, worktree fast path) are all correctly characterized as non-blocking polish items. No code changes required.

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • vitest packages/cli src/serve/workspace-git-state.test.ts — 16 passed (16 total)
中文说明

已处理的审阅反馈

rc:3647718417 — sameWorkingTreeStatus 中的 branch 比较(建议)

决定:已实现(添加注释)

审阅者指出 materialize() 会用 entry.branch(来自 reflog watcher)覆盖 status.branch,因此 sameWorkingTreeStatus 中仅 branch 不同时会触发一次冗余的 SSE 事件。考虑过移除该比较但予以否决:当 watcher 落后于 checkout 操作时,status.branch 是第一个表明发生变化的信号——抑制它会延迟到下一次 watcher 触发才推送 delta。添加了三行注释,说明有意采用保守的推送策略。

变更: packages/cli/src/serve/workspace-git-state.ts — 在 sameWorkingTreeStatusreturn 前添加注释。

rc:3647718420 — 缺少 publishWorkspaceEvent 抛异常路径的测试(建议)

决定:已实现(添加测试)

startRefreshpublishWorkspaceEvent 周围的 catch {} 是关键保护——它防止 SSE 广播抛异常时将 wait:true 路由调用变成 500 错误。现有测试未覆盖此路径。新增测试使 publishWorkspaceEvent 在首次调用时抛异常,然后断言:(1) getStatus({ wait: true }) 仍然以计算出的摘要正常 resolve;(2) 缓存已填充(后续 fast 调用返回带 computedAt 的 enriched 状态)。

变更: packages/cli/src/serve/workspace-git-state.test.ts — 新增测试用例 "keeps the cache and resolves wait:true when publishWorkspaceEvent throws"。

@wenshao issue 级评论 — LGTM 及少量提示

决定:无需代码变更

已确认。四条提示(wait:true 在瞬时失败时返回 last-known、fast-path .catch 竞态、冗余双次 setState、worktree fast path)均为非阻塞的打磨项,无需代码变更。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • vitest packages/cli src/serve/workspace-git-state.test.ts — 16 通过(共 16 个)

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

中文说明

已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/workspace-git-state.test.ts
Comment thread packages/cli/src/serve/workspace-git-state.ts
Comment on lines +272 to +282
case 'git_status_changed': {
const data = getRecord(event.data);
const workspaceCwd = getString(data, 'workspaceCwd');
setConnection((current) =>
workspaceCwd && workspaceCwd !== current.workspaceCwd
? current
: {
...current,
gitStatus: data as unknown as DaemonWorkspaceGitStatus,
},
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The git_status_changed handler stores the raw SSE record via as unknown as DaemonWorkspaceGitStatus blind cast, while the adjacent git_branch_changed handler (three lines above) extracts each field with getString. The inconsistency suppresses all structural validation for this event.

Failure scenario: If a future daemon change alters the git_status_changed payload shape (renames a field, drops a required field like branch), the client silently stores the malformed object into connection.gitStatus. The App.tsx SSE mirror effect and any future consumer receive the broken object with no runtime error. The git_branch_changed handler would catch the same class of bug at extraction time.

Suggested change
case 'git_status_changed': {
const data = getRecord(event.data);
const workspaceCwd = getString(data, 'workspaceCwd');
setConnection((current) =>
workspaceCwd && workspaceCwd !== current.workspaceCwd
? current
: {
...current,
gitStatus: data as unknown as DaemonWorkspaceGitStatus,
},
);
case 'git_status_changed': {
const data = getRecord(event.data);
const workspaceCwd = getString(data, 'workspaceCwd');
const branch = getString(data, 'branch');
setConnection((current) => {
if (workspaceCwd && workspaceCwd !== current.workspaceCwd) return current;
const v = typeof data?.v === 'number' ? data.v : 2;
return {
...current,
gitStatus: {
v,
workspaceCwd: workspaceCwd ?? current.workspaceCwd ?? '',
branch: branch ?? null,
...(typeof data?.staged === 'number' ? { staged: data.staged } : {}),
...(typeof data?.unstaged === 'number' ? { unstaged: data.unstaged } : {}),
...(typeof data?.untracked === 'number' ? { untracked: data.untracked } : {}),
...(typeof data?.ahead === 'number' ? { ahead: data.ahead } : {}),
...(typeof data?.behind === 'number' ? { behind: data.behind } : {}),
...(typeof data?.stashCount === 'number' ? { stashCount: data.stashCount } : {}),
...(data?.computedAt != null ? { computedAt: data.computedAt } : {}),
} as DaemonWorkspaceGitStatus,
};
});
break;
}
中文说明

git_status_changed 处理器通过 as unknown as DaemonWorkspaceGitStatus 盲转型存储原始 SSE 记录,而相邻的 git_branch_changed 处理器使用 getString 逐字段提取。不一致导致此事件缺少结构验证。如果未来 daemon 端修改了 payload 结构(重命名或丢弃字段),客户端会静默存储格式错误的对象。建议显式提取字段,与 git_branch_changed 的模式保持一致。

— qwen3.7-max via Qwen Code /review

doudouOUC
doudouOUC previously approved these changes Jul 24, 2026

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 3/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

[rc:3648048233] Test for wait:true joining an in-flight fast-path refresh — Implemented

Added a focused test 'wait:true joins an already in-flight fast-path refresh' in workspace-git-state.test.ts. It fires a fast-path call (kicking a background refresh that sets statusPromise), then fires a wait:true call while the refresh is still in-flight, and asserts that getGitWorkingTreeStatus is called exactly once — confirming the wait:true caller shares the in-flight computation rather than spawning a second subprocess.

[rc:3648048245] Diagnostic logging on git subprocess failure — Implemented

Replaced the silent .catch(() => null) in startRefresh with a writeStderrLine call that logs the workspace path and error message, following the existing qwen serve: prefix convention used in workspace-skills-status.ts. A persistently failing git status (corrupt .git, PATH issue, disk full) is now visible in the daemon's stderr output instead of silently freezing the cache.

[rc:3648048252] Explicit field extraction in git_status_changed mapper — Declined

The suggested ~20-line per-field type-guard extraction adds significant diff growth for an internal daemon→client SSE protocol where both sides live in the same monorepo and the producer (materialize()) is fully typed. The git_branch_changed handler extracts fields because it needs only one field (branch); git_status_changed needs the entire object, making pass-through the natural approach. If the daemon changes the payload shape, TypeScript compilation catches it at the materialize() call site. Not worth the diff growth.

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • vitest run src/serve/workspace-git-state.test.ts (packages/cli, touched) — 17 passed
中文说明

已处理的审查反馈

[rc:3648048233] wait:true 加入正在进行的 fast-path 刷新的测试 — 已实现

workspace-git-state.test.ts 中添加了聚焦测试 'wait:true joins an already in-flight fast-path refresh'。测试先发起 fast-path 调用(触发后台刷新并设置 statusPromise),然后在刷新仍在进行时发起 wait:true 调用,并断言 getGitWorkingTreeStatus 仅被调用一次——确认 wait:true 调用方共享了正在进行的计算,而非启动第二个子进程。

[rc:3648048245] git 子进程失败时的诊断日志 — 已实现

startRefresh 中静默的 .catch(() => null) 替换为 writeStderrLine 调用,记录工作区路径和错误信息,遵循 workspace-skills-status.ts 中已有的 qwen serve: 前缀约定。持续失败的 git status(损坏的 .git、PATH 问题、磁盘满)现在会在 daemon 的 stderr 输出中可见,而非静默冻结缓存。

[rc:3648048252] git_status_changed 映射器中的显式字段提取 — 已拒绝

建议的约 20 行逐字段类型守卫提取为内部 daemon→client SSE 协议增加了显著的 diff 增长,而该协议的双方位于同一 monorepo 中,且生产者(materialize())具有完整类型。git_branch_changed 处理器提取字段是因为它只需要一个字段(branch);git_status_changed 需要整个对象,因此直接传递是自然的方式。如果 daemon 更改了 payload 结构,TypeScript 编译会在 materialize() 调用处捕获。不值得增加 diff。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • vitest run src/serve/workspace-git-state.test.ts(packages/cli,已修改)— 17 个通过

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

Comment thread packages/cli/src/serve/workspace-git-state.ts Outdated
Comment thread packages/cli/src/serve/workspace-git-state.test.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 4/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

[rc:3648536511] Critical — writeStderrLine can throw EPIPE inside .catch() callback

Decision: Implemented.

Analysis: Verified that writeStderrLine writes directly to process.stderr and can throw on EPIPE when stderr is closed or piped away. Inside the .catch() callback of startRefresh, such a throw would escape into the IIFE promise — causing an unhandled rejection on the fast path and a spurious 500 on wait: true routes. The codebase provides writeStderrLineSafe (same module) that wraps the write in a try/catch, and it is already used in analogous serve error paths (workspace-service/index.ts, acpAgent.ts).

Change: Replaced writeStderrLine with writeStderrLineSafe in the import and the single call site in workspace-git-state.ts.

[rc:3648536519] Suggestion — writeStderrLine not mocked in test file

Decision: Implemented.

Analysis: Verified that neighboring serve test files (workspace-skills-status.test.ts, sse-last-event-id.test.ts, create-sub-session.test.ts) all mock ../utils/stdioHelpers.js. Without the mock, the error-path test (degrades to the branch-only shape when the working-tree summary throws) writes real stderr output during CI runs.

Change: Added vi.mock('../utils/stdioHelpers.js', ...) to workspace-git-state.test.ts, mocking both writeStderrLine and writeStderrLineSafe.

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • vitest run src/serve/workspace-git-state.test.ts (packages/cli) — 17 passed
  • vitest run src/serve/routes/workspace-git.test.ts (packages/cli) — 8 passed
中文说明

已处理的审查反馈

[rc:3648536511] Critical — writeStderrLine.catch() 回调中可能抛出 EPIPE

决定: 已实现。

分析: 已验证 writeStderrLine 直接写入 process.stderr,当 stderr 关闭或被管道重定向时可能抛出 EPIPE。在 startRefresh.catch() 回调中,这样的抛出会逃逸到 IIFE promise 中——在快速路径上导致未处理的 rejection,在 wait: true 路由上导致错误的 500 响应。代码库提供了 writeStderrLineSafe(同一模块),它用 try/catch 包裹写入操作,并且已在类似的 serve 错误处理路径中使用(workspace-service/index.tsacpAgent.ts)。

更改:workspace-git-state.ts 的导入和唯一调用点中将 writeStderrLine 替换为 writeStderrLineSafe

[rc:3648536519] Suggestion — 测试文件中未 mock writeStderrLine

决定: 已实现。

分析: 已验证相邻的 serve 测试文件(workspace-skills-status.test.tssse-last-event-id.test.tscreate-sub-session.test.ts)都 mock 了 ../utils/stdioHelpers.js。没有该 mock,错误路径测试(degrades to the branch-only shape when the working-tree summary throws)会在 CI 运行期间写入真实的 stderr 输出。

更改:workspace-git-state.test.ts 中添加了 vi.mock('../utils/stdioHelpers.js', ...),mock 了 writeStderrLinewriteStderrLineSafe

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • vitest run src/serve/workspace-git-state.test.ts(packages/cli)— 17 个通过
  • vitest run src/serve/routes/workspace-git.test.ts(packages/cli)— 8 个通过

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: build-and-test — Agent 7 ran and confirmed all 16 workspaces build and all 5 PR-changed test files pass (505 tests), but the harness transcript could not verify its brief was read; verdict is capped at COMMENT per compose-review. Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 1a: Line-by-line correctness, Agent 2: Security, Agent 3: Code quality, Agent 4: Performance & efficiency, Agent 5: Test coverage, Agent 6a: Undirected audit — attacker mindset, Agent 6b: Undirected audit — 3 AM oncall mindset, Agent 6c: Undirected audit — six-months-later maintainer, Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 4: Performance & efficiency, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/cli/src/serve/workspace-git-state.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 5/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

[rc:3648954297] Fresh-path .catch(() => {}) silently swallows errors — Implemented

Added console.warn('[web-shell] git status fresh path failed:', err) to the fresh-path catch handler in App.tsx. Used console.warn instead of the suggested console.debug because the project's ESLint no-console rule only permits warn and error. This gives oncall engineers a visible trace in DevTools when the fresh path fails (ECONNRESET, network error), without cluttering normal output.

[rc:3648954298] Branch-watcher dispose guard has no test coverage — Implemented

Added test 'does not publish git_branch_changed when the watcher fires after dispose' to workspace-git-state.test.ts. The test creates an entry, captures the watcher callback, disposes the state, invokes the callback with a changed branch, and asserts publishWorkspaceEvent was never called — directly covering the if (entry.disposed) return; guard in the branch-watcher callback.

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed (initially failed on console.debug; fixed to console.warn)
  • vitest run packages/cli/src/serve/workspace-git-state.test.ts — 18 passed (17 existing + 1 new)
  • vitest run packages/web-shell/client/App.test.tsx — 171 passed
中文说明

已处理的审查反馈

[rc:3648954297] 新路径 .catch(() => {}) 静默吞掉错误 — 已实现

App.tsx 的新路径 catch 处理程序中添加了 console.warn('[web-shell] git status fresh path failed:', err)。使用 console.warn 而非建议的 console.debug,因为项目的 ESLint no-console 规则仅允许 warnerror。这为值班工程师在新路径失败(ECONNRESET、网络错误)时提供了 DevTools 中可见的追踪信息,同时不会污染正常输出。

[rc:3648954298] 分支监视器 dispose 守卫缺少测试覆盖 — 已实现

workspace-git-state.test.ts 中添加了测试 'does not publish git_branch_changed when the watcher fires after dispose'。该测试创建一个条目,捕获监视器回调,dispose 状态,使用已更改的分支调用回调,并断言 publishWorkspaceEvent 从未被调用——直接覆盖了分支监视器回调中的 if (entry.disposed) return; 守卫。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过(最初因 console.debug 失败;已修复为 console.warn
  • vitest run packages/cli/src/serve/workspace-git-state.test.ts — 18 个通过(17 个已有 + 1 个新增)
  • vitest run packages/web-shell/client/App.test.tsx — 171 个通过

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/workspace-git-state.test.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 6/100 轮)。改动内容与我反驳保留之处如下:

Review feedback addressed

[rc:3649082877] Assert writeStderrLineSafe in the refresh-failure test — Implemented

Feedback: writeStderrLineSafe is mocked but never asserted in the 'keeps the cached summary and stays silent when a refresh fails' test. A future removal of the stderr log inside startRefresh's catch would go undetected.

Change: Added a static import of writeStderrLineSafe, a vi.mocked() reference (matching the existing pattern for the other three mocks), and an assertion in the failure test that writeStderrLineSafe was called with a message containing 'git status failed'.

Files changed: packages/cli/src/serve/workspace-git-state.test.ts (+5 lines)

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • npx vitest run src/serve/workspace-git-state.test.ts (packages/cli) — 18 passed
中文说明

已处理的审查反馈

[rc:3649082877] 在刷新失败测试中断言 writeStderrLineSafe已实现

反馈: writeStderrLineSafe 已被 mock,但在 'keeps the cached summary and stays silent when a refresh fails' 测试中从未被断言。如果将来移除了 startRefresh 的 catch 中的 stderr 日志,将无法被检测到。

变更: 添加了 writeStderrLineSafe 的静态导入、vi.mocked() 引用(与其他三个 mock 的现有模式一致),以及在失败测试中添加了断言,验证 writeStderrLineSafe 被调用时消息包含 'git status failed'

变更文件: packages/cli/src/serve/workspace-git-state.test.ts(+5 行)

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • npx vitest run src/serve/workspace-git-state.test.ts(packages/cli)— 18 个测试通过

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4 — no agent reported covering these; nobody read them. Not reviewed: every dimension — none of the 12 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下:

No action taken

The two review comments on this round contain no actionable code findings:

  • @doudouOUC (COMMENTED): A meta-comment about the review tooling process itself (review chunks not covered, agents not launched with proper prompts). It raises no code-level findings against the diff.
  • @qwen-code-ci-bot (COMMENTED): "Downgraded from Approve to Comment: CI still running." This is a CI status note, not a code finding.

There are no inline comments, no issue-level comments, no failed checks, and no still-red checks to address. No changes were made.

中文说明

未采取任何操作

本轮的两条评审评论均不包含可操作的代码发现:

  • @doudouOUCCOMMENTED):这是一条关于评审工具流程本身的元评论(评审分块未被覆盖、agent 未使用正确的 prompt 启动)。它没有针对 diff 提出任何代码层面的发现。
  • @qwen-code-ci-botCOMMENTED):"从 Approve 降级为 Comment:CI 仍在运行。"这是一条 CI 状态说明,而非代码发现。

没有行内评论、没有 issue 级评论、没有失败的检查、也没有持续红色的检查需要处理。未做任何更改。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Solid stale-while-revalidate design: in-flight dedup, 2s throttle, publish-only-on-change, disposed guard. Sidebar correctly opts into blocking semantics. No blocking findings after OCR + manual audit.

@wenshao
wenshao added this pull request to the merge queue Jul 25, 2026
Merged via the queue into QwenLM:main with commit d61b0ea Jul 25, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants