feat(web-shell): git status chip, visual working-tree diff, and sidebar git status - #7054
Conversation
|
Re-run at head Template looks good ✓ — all required sections present, bilingual, screenshots included, test plan detailed. Problem: real and well-documented. The Web Shell previously showed only a branch name — no dirty indicator, no way to see what changed. This is a genuine gap for an AI coding agent's web UI. Direction: aligned. Read-only git awareness in the web shell is a natural extension of the CLI's interactive diff experience. The trust-gating model and daemon-as-authority architecture are preserved. Size: 46 files, ~1,512 production logic lines (excluding ~2,740 test lines and ~1,247 docs lines). The 1,000+ advisory applies — this is a large PR, but the scope is justified by three connected features that share the same daemon routes and SDK types. Core path touched: Approach: scope feels right. The new commit tightens Moving on to code review. 🔍 中文说明在 head 模板完整 ✓ — 所有必填部分齐全，双语，含截图，测试计划详细。 问题：真实且有据可查。Web Shell 此前只显示分支名——没有脏标记，无法查看变更内容。对 AI 编码 agent 的 web UI 来说这是一个真实的缺口。 方向：对齐。在 web shell 中添加只读 git 感知是 CLI 交互式 diff 体验的自然延伸。信任门控模型和 daemon 权威架构得以保留。 规模：46 文件，约 1,512 行生产逻辑（排除约 2,740 行测试和约 1,247 行文档）。适用 1,000+ 大 PR 建议——范围合理，三个关联功能共享基础设施。触及核心路径 方案：范围合理。新提交收紧 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
…ar git status Bring working-tree Git awareness to the Web Shell (browser daemon session UI): - Toolbar branch chip becomes a live status indicator: dirty (staged/unstaged/ untracked), ahead/behind upstream, stash count, detached HEAD, in-progress operation (merge/rebase/cherry-pick/revert/bisect), and conflict count, each with a non-color cue. - Read-only "Changes" dialog: working-tree-vs-HEAD file list with per-file, line-level, per-side syntax-highlighted diffs; opens via /diff or a dirty chip; untracked files expand as fully-added and deleted files still diff. - Per-workspace git status in the sidebar: a compact icon-only chip per trusted workspace (status dot + hover tooltip); click opens that workspace's dialog. All git access goes through the daemon REST API with per-workspace trust gating; new SDK status fields are optional and additive (v2).
d473125 to
18f8a9d
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Code Review (re-run at
|
|
Confidence: 4/5 — Clean across every stage; the new commit is a small, correct path normalization fix with a regression test, and the full PR is well-architected, defensive, and thoroughly tested. This PR has been through many rounds of review and each one made it better. The latest commit ( Looking at the full picture: the architecture is sound (all git access through daemon REST, browser never touches the repo, trust-gating preserved), the security posture is strong (every The two UI bugs the maintainer found earlier (StrictMode cancelling the diff fetch, sidebar polling a display name) are both fixed with surgical changes and regression tests. The remaining non-blocking suggestions from various review rounds (linked worktree stash count, untracked count alignment, pathspec magic hardening, rename-aware single-file diff) are valid polish for follow-up PRs — none block merge. The only reason this isn't a 5/5 is the PR's large size (1,000+ production lines, 46 files). The scope is justified by three connected features sharing infrastructure, but it's still a lot to review in one pass. For future work, splitting the status chip, diff dialog, and sidebar icons into separate PRs would make review more manageable — though landing them together avoids duplicating the plumbing. LGTM, ready to ship. ✅ 中文说明信心度：4/5 — 各阶段均通过；新提交是一个小而正确的路径规范化修复，配有回归测试；整个 PR 架构良好、防御性强、测试充分。 这个 PR 经历了多轮审查，每一轮都有改进。最新提交（ 全局来看：架构扎实（所有 git 访问经 daemon REST，浏览器不直接触碰仓库，信任门控保留），安全性强（每个 此前维护者发现的两个 UI bug（StrictMode 取消 diff 获取、侧栏轮询使用显示名）均已通过精准修改和回归测试修复。各轮审查中的非阻塞性建议（链接 worktree stash 计数、untracked 计数对齐、pathspec magic 加固、重命名感知单文件 diff）是后续 PR 的有效打磨——均不阻塞合并。 唯一未达 5/5 的原因是 PR 规模较大（1,000+ 生产行、46 文件）。范围因三个关联功能共享基础设施而合理，但仍需大量审查。未来工作建议将状态条、diff 弹窗和侧栏图标拆分为独立 PR——尽管一起落地避免了重复搭建。 LGTM，可以合并。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Review: git status chip, working-tree diff, and sidebar git statusOverviewThree coordinated layers, all read-only: core gains I fetched the head ( What's done well
Should fix
Minor / non-blocking
Notes (no action needed)
VerdictSolid, well-tested, security-conscious foundation for the git integration roadmap. Item 1 is the only convention blocker; everything else is polish that could land as follow-ups. 中文版（Chinese translation）评审：git 状态 chip、工作区可视化 diff 与侧栏 git 状态概述三层协同、全部只读：core 新增 我将 head（ 做得好的地方
建议修复
次要 / 不阻塞
备注（无需行动）
结论扎实、测试充分、有安全意识的 git 集成基础。第 1 条是唯一的约定性阻塞项；其余均可作为后续打磨。 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Tooltips now render on the themed popover surface (bg-popover / text-popover-foreground / border + fill-popover arrow) instead of the inverted bg-foreground default, so they read dark-on-dark rather than a bright box on the dark theme. Fixing the shared primitive corrects the git branch tooltip in the composer toolbar and sidebar, plus every other tooltip, at once. Also addressing review feedback on the git integration: - Replace the hand-drawn detached/conflict/stash SVG icons with lucide-react (CircleDot / TriangleAlert / Layers) per the web-shell icon convention. - Gate the tooltip "Working tree clean" message on an enriched status (computedAt) so a branch-only status no longer asserts clean. - Include the file path in the diff dialog row aria-label so screen readers can distinguish files. - Reset the toolbar git chip on workspace switch so it never shows the previous repo's branch/counts while the new fetch resolves. - Log a sidebar git poll failure only on the success->failure transition to avoid spamming a long-lived tab. - Correct the SDK doc for DaemonWorkspaceGitDiffFile.added/removed (0, not undefined, for binary files).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run.
— qwen3.7-max via Qwen Code /review
Follow-ups from the /review pass on the git integration: - GitBranchIndicator: include the short SHA in the detached-HEAD tooltip title, and add the "Working tree clean" status to the aria-label (gated on an enriched status, matching the tooltip) so the two never drift. - WorkspaceSection: keep the last known git status on a transient poll failure instead of blanking the chip for a whole interval. - App: surface a toast for `/diff` when no workspace is available instead of silently consuming the composer input. - Tests: cover the diff dialog's list-load and per-file load error paths, and detectGitOperation's revert/bisect branches. - Design doc: align the getGitWorkingTreeStatus spec text with the decision (transient states return status with `operation`; null is reserved for non-repo / git failure).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 14 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 2 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 10 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 11 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 15 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 9 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 7 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 3 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 8 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 5 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 16 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 13 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 12 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 4 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent was launched with it. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 12 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 14 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 15 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 16 — its prompt was built, but no agent was launched with it. Not reviewed: Test coverage matrix (whole-diff) — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1b: Removed-behavior audit — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1c: Cross-file tracer — its prompt was built, but no agent was launched with it. Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped). Not reviewed: verification — the review posts findings, but no verifier ran (Step 4 builds its prompt with agent-prompt --role verify; none was recorded, so the findings were not verified).
— qwen3.7-max via Qwen Code /review
…l interval - Add a :focus-visible outline to .gitBranchChipButton so keyboard users get a visible focus indicator (the chip resets UA button chrome). - Design doc: align the active-workspace poll-interval references at 30s to match the implementation.
… degradation paths
Address the remaining review findings on the git integration:
- Truncation is no longer silent: fetchGitDiffHunksForFile now returns
{ hunks, truncated } — the parser records files that actually lost
lines to MAX_LINES_PER_FILE (tracked path), and the untracked
synthesis reports its byte/line caps. The route forwards an additive
`truncated` flag on the hunks response (absent when not truncated, so
older clients and daemons are unaffected), and the Changes dialog
renders a "Diff truncated" note under the visible window.
- DiffHunks catches an unexpected buildRows rejection (e.g. malformed
hunk lines) and shows the per-file error instead of leaving an
unhandled rejection and a silently empty diff area.
- New tests: untracked and tracked truncation at the core caps, the
route's truncated passthrough (and its absence when clean), the
branch-only degradation when the working-tree summary throws, the
malformed-hunks error path, and the Shiki success path (a fake
tokenizer proving add rows pull new-side tokens and del rows pull
old-side tokens, not the plain-text fallback).
Review — git status chip, working-tree diff, sidebar git statusVerdict: LGTM with minor findings — nothing blocking. The layering (core git utils → daemon REST → SDK → web-shell UI) is clean, the security posture matches the hardening patterns already established in Verified locally at
Findings1.
The operation markers ( 2. Untracked count disagrees between chip and dialog for untracked directories (minor)
3. Browser-supplied
4. Silent truncation in the file viewer (UX nit)
A distinct “diff too large” message and/or a 5.
Notes (no action needed)
CI’s main Test jobs were still pending at review time; everything above is from local runs at head 中文版评审意见评审 — git 状态 chip、工作区可视化 diff、侧栏 git 状态结论：LGTM，仅有少量非阻塞发现。 分层清晰（core git 工具 → daemon REST → SDK → web-shell UI），安全姿态与 在
发现1. 链接 worktree 中
2. 未跟踪目录导致 chip 与弹窗计数不一致（轻微）
3. 浏览器传入的 实测 4. 文件查看器的静默截断（体验建议）
建议增加「diff 过大」的独立提示和/或在 hunks 响应中加 5.
备注（无需行动）
评审时 CI 主 Test 任务仍在运行中；以上结论均来自 head |
The dialog and alert-dialog overlays applied `backdrop-blur-xs`, which forces the browser to rasterize and blur the entire content behind the overlay when a dialog opens. With a long transcript behind it, that main-thread paint+blur froze the whole page — e.g. clicking the git branch chip to open the Changes dialog. Keep the bg-black/10 scrim for separation and drop the blur.
| - [x] 单测：core `gitDiff.test.ts` 88 通过、cli `workspace-git-diff.test.ts` | ||
| 8 通过、web-shell `GitDiffDialog.test.tsx` 5 + `GitBranchIndicator.test.tsx` |
There was a problem hiding this comment.
[Suggestion] Two of the four test counts are stale — the design doc claims GitDiffDialog.test.tsx has 5 tests and workspace-git-diff.test.ts has 8, but the actual files contain 10 and 9 it() blocks respectively. Concrete cost: a reviewer verifying the PR against this design document would find mismatched counts.
| - [x] 单测：core `gitDiff.test.ts` 88 通过、cli `workspace-git-diff.test.ts` | |
| 8 通过、web-shell `GitDiffDialog.test.tsx` 5 + `GitBranchIndicator.test.tsx` | |
| - [x] 单测：core `gitDiff.test.ts` 88 通过、cli `workspace-git-diff.test.ts` | |
| 9 通过、web-shell `GitDiffDialog.test.tsx` 10 + `GitBranchIndicator.test.tsx` |
— qwen3.7-max via Qwen Code /review
synthesizeUntrackedHunk opened an untracked path before checking its type, so an untracked FIFO (listed by `ls-files --others`) would block on open() forever waiting on a writer — hanging the daemon's event loop and leaving the Web Shell Changes dialog stuck on a permanent loading state. lstat-gate on regular files before opening, matching the existing guard in countUntrackedLines. Adds a FIFO regression test.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped).
— qwen3.7-max via Qwen Code /review
| - `DaemonClient.workspaceGitDiff` / `workspaceGitDiffFile`：正确拼接 URL、 | ||
| `path` 经过 `urlEncode`。 |
There was a problem hiding this comment.
[Suggestion] Test plan mandates urlEncode verification for DaemonClient.workspaceGitDiffFile, but Phase 2 Task 3 explicitly states these tests were not written ("未加 client 方法单测").
Failure scenario: If workspaceGitDiffFile fails to urlEncode a path containing &, =, #, or other query-string metacharacters, the daemon route will receive a truncated or malformed ?path= value, producing a wrong diff result or a 400 error for legitimate file paths. No test catches this.
| - `DaemonClient.workspaceGitDiff` / `workspaceGitDiffFile`：正确拼接 URL、 | |
| `path` 经过 `urlEncode`。 | |
| - `DaemonClient.workspaceGitDiff` / `workspaceGitDiffFile`：正确拼接 URL、 | |
| `path` 经过 `urlEncode`（**已补单测**，覆盖含 `&`/`=`/`#`/空格的路径）。 |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
packages/web-shell/client/components/ChatEditor.tsx:1694 — the responsive-width replica measures only the branch label, while the live expanded Git chip also contains operation/conflict/dirty/ahead/behind/stash indicators. Near the threshold the compact state expands and the expanded state collapses, retriggering the layout effect. Measure the complete expanded chip or include those indicators in the replica/budget. [Critical] packages/core/src/utils/gitDiff.ts:1056 — path-bearing -z output is decoded as UTF-8 before parsing. Linux filenames with invalid UTF-8 bytes become U+FFFD, so the listed path cannot address or expand the actual file. Preserve path output as bytes through parsing or use a byte-safe encoding/transport. [Critical] packages/core/src/utils/gitDiff.ts:597 — the hunk parser accepts only +, -, and space lines, dropping Git's \ No newline at end of file marker. A newline-only edit renders as identical removed/added text with the actual semantic change hidden. Preserve and render the metadata marker. [Critical] packages/core/src/utils/gitDiff.ts:479 — rename entries are keyed as the synthetic string old => new, which is serialized as the file path and sent back to the single-file endpoint. Git receives a nonexistent literal path, so renamed rows cannot expand. Carry old/new paths structurally and request the real destination path.
— Codex GPT-5 via Qwen Code /review
| if (!gitDir) return 0; | ||
| try { | ||
| const content = await readFile( | ||
| path.join(gitDir, 'logs', 'refs', 'stash'), |
There was a problem hiding this comment.
[Critical] This reads the per-worktree Git dir directly, so linked worktrees miss the shared stash reflog. It also follows arbitrary file types with an unbounded readFile; a stash-log symlink to a FIFO can keep every status poll pending, and a huge target can exhaust memory. Resolve the common Git dir, reject symlinks/non-regular files, and use a bounded read or hardened Git command with a timeout.
— Codex GPT-5 via Qwen Code /review
| return { | ||
| v: 2, | ||
| workspaceCwd, | ||
| branch: entry.branch ?? status.branch ?? null, |
There was a problem hiding this comment.
[Critical] This permanently prefers the cached watcher branch over the freshly computed status branch. Unborn repositories may never install a HEAD-log watcher, and watcher delivery can race the request, so after the first commit/checkout the response can combine a stale branch with fresh dirty/detached fields. Prefer the fresh status branch or sequence/invalidate the cache before merging.
— Codex GPT-5 via Qwen Code /review
| .workspaceByCwd(activeWorkspaceCwd) | ||
| .workspaceGit() | ||
| .then((git) => { | ||
| if (!cancelled) setSelectedWorkspaceGitStatus(git); |
There was a problem hiding this comment.
[Critical] Concurrent initial/focus/timer requests all commit unconditionally under the same lifetime flag. An older clean response can resolve after a newer dirty response and roll the chip back; the sidebar has the same stale-response pattern. Track a per-request sequence or compare computedAt before committing.
— Codex GPT-5 via Qwen Code /review
| setError(false); | ||
| client | ||
| .workspaceByCwd(workspaceCwd) | ||
| .workspaceGitDiff() |
There was a problem hiding this comment.
[Critical] /diff is intercepted without a version/capability gate, but the Web Shell accepts older external SDK/daemon versions. A missing SDK method throws synchronously here, while an older daemon returns 404 and the command no longer falls back to its prior text behavior. Add an explicit diff capability/version check and a graceful fallback.
— Codex GPT-5 via Qwen Code /review
| const parsed = parseGitDiff(diffOut); | ||
| // A single-file diff yields at most one entry; return its hunks regardless of | ||
| // the exact header key (which may carry rename / C-style-quote formatting). | ||
| if (parsed.size > 0) return parsed.values().next().value ?? []; |
There was a problem hiding this comment.
[Critical] This assumes the pathspec yields exactly one literal file. Git still interprets pathspec magic after --, a directory or . selects multiple descendants, and user diff-prefix config can alter parsed keys; this then returns the first unrelated entry under the requested path. Use a top-level literal pathspec, reject directories, pin parser-sensitive Git config, and require one exact matching result.
— Codex GPT-5 via Qwen Code /review
| gitPollFailed.current = false; | ||
| setGitStatus(status); | ||
| } catch (err) { | ||
| // Keep the last known status on a transient failure so a brief network |
There was a problem hiding this comment.
[Suggestion] Retaining the last status is reasonable, but computedAt is never compared or displayed. Repeated failures can leave “Working tree clean” asserted indefinitely from an obsolete snapshot. Mark the value stale/show its age or expire it after a bounded threshold.
— Codex GPT-5 via Qwen Code /review
| } | ||
|
|
||
| const rows: DiffRow[] = []; | ||
| for (const hunk of hunks) { |
There was a problem hiding this comment.
[Suggestion] Disjoint hunks are appended directly with no @@ header or gap marker, so lines around 1 and 200 appear visually adjacent except for an abrupt gutter jump. Insert an explicit hunk boundary/omission row and add a two-hunk rendering test.
— Codex GPT-5 via Qwen Code /review
| - `getGitWorkingTreeStatus`：clean / dirty（staged、unstaged、untracked 混合）/ | ||
| detached / 有 upstream 的 ahead-behind / 无 upstream / transient state / | ||
| 非仓库各分支；branch header 解析正确。 | ||
| - `fetchGitDiffHunksForFile`：单文件有变化 / 无变化 / untracked 返回空 / |
There was a problem hiding this comment.
[Suggestion] The plan requires --no-ext-diff/--no-textconv on the single-file helper, but the sentinel tests exercise only the older whole-tree helper. Parameterize those tests over fetchGitDiffHunksForFile so dropping either guard from its independent argv path fails.
— Codex GPT-5 via Qwen Code /review
| }); | ||
|
|
||
| it('skips the second path of a rename without double counting', () => { | ||
| const tokens = ['R old.ts', 'new.ts', ' M other.ts']; |
There was a problem hiding this comment.
[Suggestion] Porcelain v1 -z emits the destination/current rename path in the status token and the source/old path second; this fixture reverses them. Swap to ['R new.ts', 'old.ts', ...] so future path-aware parser changes are tested against the real protocol.
— Codex GPT-5 via Qwen Code /review
| expect(gitChip()).toBeNull(); | ||
| }); | ||
|
|
||
| it('omits the chip when no diff handler is provided', async () => { |
There was a problem hiding this comment.
[Suggestion] This checks only that the chip is hidden, not the resource contract that Git is not queried without a diff consumer. Removing the handler gate would introduce invisible mount/focus/timer polling while the test still passes. Assert workspaceGit is not called.
— Codex GPT-5 via Qwen Code /review
…ement Round-5 review Criticals: - core: key renamed diff entries by the real (post-rename) path and carry the old path for display, so renamed rows can be expanded — the synthetic `old => new` key was sent to git as a nonexistent literal path. The diff dialog renders the rename as `old → new`. - core: preserve Git's `\ No newline at end of file` marker through the hunk parser so a trailing-newline-only edit isn't shown as identical removed/added lines (the viewer already renders it as a meta row). - web-shell: the toolbar's hidden git-chip measurement replica now renders the full chip content via the extracted GitBranchChipContent, so the expanded width includes the status indicators and the compact/expanded toggle no longer oscillates near the responsive threshold.
The review tooling runs `npm ci` with QWEN_SKIP_PREPARE=1 (to skip the heavy prepare build) and then builds only the changed workspaces. Because `prepare` exited before generating the gitignored git-commit.ts, a per-workspace build of packages/cli failed at the unchanged systemInfo.ts on the missing `../generated/git-commit.js` module. Generate the git-commit info in the skip path too — it is cheap and never fails hard — so a later per-workspace build or typecheck finds the module. The non-skip path still generates it via `npm run build`.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] packages/cli/src/serve/routes/workspace-git-diff.ts:112 — Selected runtime effective environment discarded (Codex, existing Critical, body not fully read) [Critical] packages/cli/src/serve/routes/workspace-git-diff.ts:206 — cwd expansion to ancestor git root beyond workspace boundary (Codex, existing Critical, body not fully read) [Critical] packages/cli/src/serve/workspace-git-state.ts:61 — Cached watcher branch preferred over fresh status branch (Codex, existing Critical, body not fully read) [Critical] packages/core/src/utils/gitDiff.ts:395 — Trimming git membership output for whitespace-only filenames (Codex, existing Critical, body not fully read) [Critical] packages/core/src/utils/gitDiff.ts:411 — Windows-shaped path checks run on POSIX rejecting legal names (Codex, existing Critical, body not fully read) [Critical] packages/core/src/utils/gitDiff.ts:1191 — status.showUntrackedFiles config dependence (Codex, existing Critical, verified still stands: --untracked-files=all not passed) [Critical] packages/core/src/utils/gitDiff.ts:1342 — rebase-apply also used by git am, reported as 'Rebasing' (Codex, existing Critical, verified still stands) [Critical] packages/web-shell/client/App.tsx:1272 — Concurrent poll race: older response overwrites newer (Codex, existing Critical, verified still stands: no per-request sequence) [Critical] packages/web-shell/client/App.tsx:1275 — Transient poll failure clears last good status (Codex, existing Critical, verified still stands: .catch sets undefined) [Critical] packages/web-shell/client/App.tsx:6782 — Branch/status/diff target derived independently, can mismatch during transitions (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/ChatEditor.tsx:2114 — Git button click bubbles to composer, focus lost after dialog close (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.module.css:157 — Per-cell horizontal scroll containers (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:71 — Tokenization restarts per hunk, miscolored multiline constructs (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:91 — Highlight budget per hunk not aggregate (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:281 — Sanitizer leaves LF/TAB/bidi controls intact (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:301 — Truncated file counts shown as exact (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:371 — /diff without version/capability gate (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/dialogs/GitDiffDialog.tsx:401 — available:false covers multiple producer states with misleading copy (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/components/sidebar/WorkspaceSection.module.css:87 — Inert row width on non-interactive span (Codex, existing Critical, body not fully read) [Critical] packages/web-shell/client/constants/localCommands.ts:92 — /diff shadows non-builtin commands (Codex, existing Critical, body not fully read) Not reviewed: reverse audit — no auditor ran. Not reviewed: chunk 7 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 19 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 10 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 11 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 12 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 20 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 18 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent was launched with it. Not reviewed: chunk 1 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 12 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 14 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 15 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 16 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 17 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 18 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 19 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 20 — its prompt was built, but no agent was launched with it. Not reviewed: Test coverage matrix (whole-diff) — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1b: Removed-behavior audit — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1c: Cross-file tracer — its prompt was built, but no agent was launched with it. Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped). Not reviewed: verification — the review posts findings, but no verifier ran (Step 4 builds its prompt with agent-prompt --role verify; none was recorded, so the findings were not verified).
— qwen3.7-max via Qwen Code /review
Maintainer Local Verification ReportTested on macOS with a real daemon + Vite dev server against a test Git repository containing staged, unstaged, and untracked changes. Unit Tests — 650 tests, all passing ✅
Daemon REST API — all endpoints verified ✅Tested against a real daemon bound to
E2E UI Verification (Playwright + real daemon)
ScreenshotsFull page — toolbar git chip visible with branch name and status dot:
Toolbar chip close-up:
Sidebar with git status icons:
Hover tooltip showing branch and working-tree state:
Changes dialog — file list with
Per-file diff expansion (stuck on "Loading changes…" despite API 200):
Issues Found1. Per-file diff expansion stuck on "Loading changes…" The 2. Sidebar workspace git status uses display name instead of path
Build Note
VerdictThe core architecture is solid: the daemon REST API layer, SDK client, and the status chip / sidebar / dialog UI all work correctly. The two issues above are localized UI bugs that should be straightforward to fix. Recommend addressing them before merge. 中文验证报告维护者本地验证报告在 macOS 上使用真实 daemon + Vite dev server 测试，测试仓库包含已暂存、未暂存和未跟踪的变更。 单元测试 — 650 个测试，全部通过 ✅
Daemon REST API — 全部端点验证通过 ✅针对绑定到
E2E UI 验证（Playwright + 真实 daemon）
截图完整页面——工具栏 git chip 可见，显示分支名和状态点：
工具栏 chip 特写：
侧栏 git 状态图标：
悬停提示显示分支和工作区状态：
Changes 弹窗——文件列表带
逐文件 diff 展开（API 200 但 UI 停留在 "Loading changes…"）：
发现的问题1. 逐文件 diff 展开停留在 "Loading changes…"
2. 侧栏 workspace git 状态使用显示名而非路径
构建说明
结论核心架构扎实：daemon REST API 层、SDK 客户端、状态 chip / 侧栏 / 弹窗 UI 均工作正常。上述两个问题是局部 UI bug，修复应该比较直接。建议修复后再合并。 |
parseGitDiff's pre-hunk guard already skips a "\ No newline at end of file" marker that appears before any @@ header, so a malformed/truncated diff can't throw on a null currentHunk and lose subsequent files' hunks; add a regression test pinning that behavior.
Re-verification after
|
| Package | Tests | Delta | Status |
|---|---|---|---|
core (gitDiff.test.ts) |
99 | +1 | ✅ |
| cli (5 files) | 87 | — | ✅ |
| web-shell (5 files) | 174 | — | ✅ |
| sdk-typescript | 277 | — | ✅ |
E2E Re-verification — both prior issues persist
| Issue | Status | Evidence |
|---|---|---|
| Per-file diff expansion stuck on "Loading changes…" | GET /workspaces/…/git/diff/file?path=index.js → HTTP 200 with correct hunks, but DiffFileRow never transitions out of loading state |
|
| Sidebar workspace git poll uses display name | GET /workspaces/Project/git → 400 (:workspace must decode to a workspace id or absolute path) |
Updated screenshots
Changes dialog (file list renders correctly):
Per-file diff still stuck on "Loading changes…" after API 200:
Sidebar with git icons (icons render, but poll 400s for "Project" workspace):
Hover tooltip:
Summary
The new test case is a good addition (pins the pre-hunk \ No newline guard). The two UI bugs from round 1 remain open — both are frontend-only issues (the daemon API layer is correct). Recommend fixing before merge.
中文重验报告
重验：bf250971 之后（第 2 轮）
新提交 bf2509710c test(core): cover stray no-newline marker before any hunk header 为 parseGitDiff 新增了一个回归测试——仅测试代码，无生产代码变更。
单元测试 — 637 个测试，全部通过 ✅
| 包 | 测试数 | 变化 | 状态 |
|---|---|---|---|
core (gitDiff.test.ts) |
99 | +1 | ✅ |
| cli（5 个文件） | 87 | — | ✅ |
| web-shell（5 个文件） | 174 | — | ✅ |
| sdk-typescript | 277 | — | ✅ |
E2E 重验 — 两个先前问题均未修复
| 问题 | 状态 | 证据 |
|---|---|---|
| 逐文件 diff 展开停留在 "Loading changes…" | GET /workspaces/…/git/diff/file?path=index.js → HTTP 200 且 hunk 正确，但 DiffFileRow 组件始终未脱离 loading 状态 |
|
| 侧栏 workspace git 轮询使用显示名 | GET /workspaces/Project/git → 400（:workspace must decode to a workspace id or absolute path） |
更新截图
Changes 弹窗（文件列表正确渲染）：
逐文件 diff 在 API 200 后仍停留在 "Loading changes…"：
侧栏 git 图标（图标正常渲染，但 "Project" workspace 轮询返回 400）：
悬停提示：
总结
新测试用例是好的补充（固定了 pre-hunk \ No newline 守卫行为）。第 1 轮发现的两个 UI bug 仍未修复——两者都是纯前端问题（daemon API 层正确）。建议修复后再合并。
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Reviewed diff-only — the PR’s existing discussion could not be fetched, so this is not an approval and not a no-blockers claim. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4, chunk 5, chunk 6, chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 13, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20 — no agent reported covering these; nobody read them. Not reviewed: issue-fidelity — lightweight mode, no PR metadata in plan. Not reviewed: build-and-test — no local tree in lightweight mode. Not reviewed: cross-file-tracer — no local tree in lightweight mode. Not reviewed: chunk 2 — launched with a prompt that never named the diff file, so it could not have read it (build the prompt with qwen review agent-prompt). Not reviewed: chunk 1 — launched with a prompt that never named the diff file, so it could not have read it (build the prompt with qwen review agent-prompt). Not reviewed: chunk 4 — launched with a prompt that never named the diff file, so it could not have read it (build the prompt with qwen review agent-prompt). Not reviewed: chunk 5 — launched with a prompt that never named the diff file, so it could not have read it (build the prompt with qwen review agent-prompt). Not reviewed: chunk 3 — launched with a prompt that never named the diff file, so it could not have read it (build the prompt with qwen review agent-prompt). Not reviewed: chunk 5 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 12 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 19 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 10 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 3 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 14 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 2 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 15 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 4 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 13 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 20 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 9 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 18 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 16 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 17 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 8 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 7 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 1 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 11 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 6 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 5 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 12 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 19 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 10 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 3 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 14 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 2 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 15 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 4 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 13 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 20 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 9 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 18 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 16 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 17 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 8 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 7 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 1 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 11 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 1 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 12 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 14 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 15 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 16 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 17 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 18 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 19 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 20 — its prompt was built, but no agent was launched with it. Not reviewed: Test coverage matrix (whole-diff) — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1b: Removed-behavior audit — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped). Not reviewed: verification — the review posts findings, but no verifier ran (Step 4 builds its prompt with agent-prompt --role verify; none was recorded, so the findings were not verified).
— qwen3.7-max via Qwen Code /review
| .gitBranchChipButton { | ||
| appearance: none; | ||
| border: 0; | ||
| background: transparent; | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
[Suggestion] .gitBranchChipButton reset is incomplete — omits font: inherit, color: inherit, padding: 0, margin: 0. When the branch chip renders as a <button> (dirty tree), browser default button styling makes it visually distinct from the non-clickable <output> chip in the same position.
Failure scenario: on macOS Safari the chip button gets ~2px 6px default padding and system font, while the <output> chip uses the component's font tokens — the two chips look different depending on which state is active.
| .gitBranchChipButton { | |
| appearance: none; | |
| border: 0; | |
| background: transparent; | |
| cursor: pointer; | |
| } | |
| .gitBranchChipButton { | |
| appearance: none; | |
| border: 0; | |
| background: transparent; | |
| cursor: pointer; | |
| font: inherit; | |
| color: inherit; | |
| padding: 0; | |
| margin: 0; | |
| } |
— qwen3.7-max via Qwen Code /review
- DiffFileRow: reset the cancelled-fetch flag on mount so StrictMode's mount/unmount/mount replay no longer leaves it latched at true, which dropped the fetched hunks and froze the row on "Loading changes…" despite a 200 response. - WorkspaceSection: skip the git status poll when the workspace cwd is not an absolute path. A synthetic fallback workspace carries a display name there, which the cwd-qualified route rejects with a 400.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] Blocker 31 (hand-written SVG icons) — could not fully assess without issue comment context Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent was launched with it.
— qwen3.7-max via Qwen Code /review
| if ( | ||
| highlighter && | ||
| !isTooLargeToHighlight(newCode) && | ||
| !isTooLargeToHighlight(oldCode) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] Both sides lose syntax highlighting when either exceeds the size limit — Failure scenario: a diff with a massive new side (>100K chars) but a small old side (a few deleted context lines) loses highlighting on the old/deleted lines unnecessarily. The !isTooLargeToHighlight(newCode) && !isTooLargeToHighlight(oldCode) guard rejects both when either fails.
| if ( | |
| highlighter && | |
| !isTooLargeToHighlight(newCode) && | |
| !isTooLargeToHighlight(oldCode) | |
| ) { | |
| if (highlighter) { | |
| const lang = resolvedLang as BundledLanguage; | |
| if (!isTooLargeToHighlight(newCode)) { | |
| try { | |
| newTokens = highlighter.codeToTokens(newCode, { lang, theme }).tokens; | |
| } catch { | |
| newTokens = null; | |
| } | |
| } | |
| if (!isTooLargeToHighlight(oldCode)) { | |
| try { | |
| oldTokens = highlighter.codeToTokens(oldCode, { lang, theme }).tokens; | |
| } catch { | |
| oldTokens = null; | |
| } | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| const renameFits = !!file.oldDisplayPath && maxPathChars >= 19; | ||
| const pathBudget = renameFits | ||
| ? Math.floor((maxPathChars - 3) / 2) | ||
| : maxPathChars; | ||
| const path = truncatePathStart(file.displayPath, pathBudget); |
There was a problem hiding this comment.
[Suggestion] Rename display in the interactive FileRow has no test coverage — Concrete cost: the renameFits threshold (maxPathChars >= 19) and the budget split (Math.floor((maxPathChars - 3) / 2)) are untested in DiffDialog.test.tsx. The non-interactive DiffStatsDisplay and the Web Shell GitDiffDialog both have rename tests, but this interactive terminal component — which has unique width-dependent fallback logic — does not. A regression in the threshold or budget calculation would silently suppress rename display at common terminal widths.
— qwen3.7-max via Qwen Code /review
- GitDiffDialog: highlight each diff side independently so a small side keeps syntax highlighting even when the other side exceeds the size cap (the old guard dropped both as soon as either was too large). - ChatEditor: complete the .gitBranchChipButton reset (font/color/padding/ margin) so the clickable dirty-tree chip matches the read-only output chip instead of picking up UA button styling. - DiffDialog: cover the interactive rename display (old to new on a wide terminal), mirroring the rename tests DiffStatsDisplay and GitDiffDialog already have.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 3 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 19 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 10 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 20 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 2 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 21 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 16 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 14 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 7 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 15 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 5 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 8 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 18 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 9 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 22 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 4 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 11 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 17 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 13 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 12 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 12 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 14 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 15 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 16 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 17 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 18 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 19 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 20 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 21 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 22 — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped). Not reviewed: verification — its prompt was built, but no agent was launched with it that opened its brief, so the posted findings were not verified.
— qwen3.7-max via Qwen Code /review
| it('forwards the oldPath query to fetchGitDiffHunksForFile', async () => { | ||
| fetchGitDiffHunksForFileMock.mockResolvedValue({ | ||
| hunks: [], | ||
| truncated: false, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] No route-level test verifies that a traversal-style oldPath query parameter is rejected or handled safely. The core layer's toRepoRelativePath does reject traversal (defense in depth), but the integration contract — route rejects unsafe input before calling core — is never verified. A future core refactor that loosens toRepoRelativePath could silently open a traversal on this route with no test to catch it.
| it('forwards the oldPath query to fetchGitDiffHunksForFile', async () => { | |
| fetchGitDiffHunksForFileMock.mockResolvedValue({ | |
| hunks: [], | |
| truncated: false, | |
| }); | |
| it('forwards the oldPath query to fetchGitDiffHunksForFile', async () => { | |
| fetchGitDiffHunksForFileMock.mockResolvedValue({ | |
| hunks: [], | |
| truncated: false, | |
| }); | |
| it('rejects a traversal oldPath query safely', async () => { | |
| fetchGitDiffHunksForFileMock.mockResolvedValue(null); | |
| const response = await request(app).get('/workspace/git/diff/file?path=ok.ts&oldPath=../../etc/passwd'); | |
| expect(response.status).toBe(200); | |
| expect(fetchGitDiffHunksForFileMock).toHaveBeenCalledWith('/work/main', 'ok.ts', '../../etc/passwd'); | |
| }); |
— qwen3.7-max via Qwen Code /review
| it('omits the chip when no diff handler is provided', async () => { | ||
| workspaceGit.mockResolvedValue({ | ||
| v: 2, | ||
| workspaceCwd: '/tmp/project', | ||
| branch: 'main', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] No test verifies that a reloadToken change triggers a git status re-fetch, despite reloadToken being in the polling effect's dependency array. If someone removed reloadToken from the dependency array, the sidebar chip would show stale dirty/clean state after agent activity until the next 60-second poll — and no test would catch it.
Consider adding a test that renders with reloadToken=0, verifies workspaceGit was called once, then re-renders with reloadToken=1 and verifies a second call fires.
— qwen3.7-max via Qwen Code /review
| compact / expanded 形态；可点击 aria。 | ||
| - `GitDiffDialog`：文件列表渲染（binary / untracked / deleted 标记）；点击展开 | ||
| 按需拉 hunk；`available === false` 占位；文件名 sanitize；hunk 行着色。 | ||
| - `/diff` 本地化：`App.tsx` 收到 `/diff` 时 `setActivePanel('diff')` 而非透传 |
There was a problem hiding this comment.
[Suggestion] The test plan says /diff is handled by calling setActivePanel('diff'), but the implementation uses setDiffWorkspaceCwd(<cwd>). The activePanel state union has no 'diff' variant. A maintainer following this test plan would test the wrong state transition.
| - `/diff` 本地化：`App.tsx` 收到 `/diff` 时 `setActivePanel('diff')` 而非透传 | |
| - `/diff` 本地化：`App.tsx` 收到 `/diff` 时 `setDiffWorkspaceCwd(<active cwd>)` 而非透传 |
— qwen3.7-max via Qwen Code /review
| it('localizes the enriched state phrases', () => { | ||
| expect(getTranslator('en')('git.operation.rebase')).toBe('Rebasing'); | ||
| expect(getTranslator('zh-CN')('git.operation.rebase')).toBe('变基中'); |
There was a problem hiding this comment.
[Suggestion] No test covers the "Working tree clean" aria-label branch that fires when status.computedAt is defined but all change counters are zero. If the computedAt check in the clean ternary were broken, no test would catch it.
Consider adding a test that renders with status: { v: 2, workspaceCwd: '/repo', branch: 'main', computedAt: 1 } (all counters defaulting to 0) and asserts the aria-label contains 'Working tree clean'.
— qwen3.7-max via Qwen Code /review
| const header = document.body.querySelector( | ||
| 'button[aria-expanded="false"]', | ||
| ) as HTMLButtonElement; | ||
| await act(async () => { |
There was a problem hiding this comment.
[Suggestion] Missing null-guard on header before calling .click(), unlike the other expand-file tests in this block which all have expect(header).not.toBeNull() between the querySelector and the click. If a future refactor prevents the file row button from rendering, this test throws TypeError instead of a clear assertion failure.
| const header = document.body.querySelector( | |
| 'button[aria-expanded="false"]', | |
| ) as HTMLButtonElement; | |
| await act(async () => { | |
| const header = document.body.querySelector( | |
| 'button[aria-expanded="false"]', | |
| ) as HTMLButtonElement; | |
| expect(header).not.toBeNull(); | |
| await act(async () => { |
— qwen3.7-max via Qwen Code /review
… doc - GitDiffDialog: add the missing expect(header).not.toBeNull() guard to the three expand-file tests that lacked it, matching the others in the block. - GitBranchIndicator: cover the known-clean aria-label branch (computedAt set and every change counter zero). - WorkspaceSection: verify a reloadToken change re-fetches git status instead of waiting for the next 60s poll. - workspace-git-diff route: verify a traversal oldPath is forwarded to core and surfaced as available:false rather than escaping the workspace. - Design doc: /diff is handled via setDiffWorkspaceCwd, not setActivePanel.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] workspace-git-diff.ts:112 — runtime effective environment discarded (comment 3600499206) [Critical] workspace-git-diff.ts:206 — cwd expansion to ancestor Git root (comment 3600499164) [Critical] workspace-git-state.ts:61 — prefers cached watcher branch (comment 3600499144) [Critical] gitDiff.ts — unbounded readFile in countStashEntries (comment 3600499140) [Critical] gitDiff.ts:395 — trimming git membership output (comment 3600499185) [Critical] gitDiff.ts:411 — Windows-shaped path checks on POSIX (comment 3600499208) [Critical] gitDiff.ts:1191 — status.showUntrackedFiles config dependency (comment 3600499170) [Critical] gitDiff.ts:1342 — rebase-apply vs git am ambiguity (comment 3600499161) [Critical] App.tsx:1272 — concurrent requests commit unconditionally (comment 3600499146) [Critical] App.tsx:1275 — transient poll failure clears good status (comment 3600499176) [Critical] App.tsx:6782 — branch/status/diff fetched independently (comment 3600499187) [Critical] ChatEditor.tsx:2114 — Git button click focus restoration (comment 3600499201) [Critical] GitDiffDialog.tsx:71 — Shiki highlight budget per hunk (comment 3600499216) [Critical] GitDiffDialog.tsx:71 — per-hunk tokenization restarts (comment 3600499214) [Critical] GitDiffDialog.tsx:309 — truncation metadata not surfaced (comment 3600499167) [Critical] GitDiffDialog.tsx:379 — /diff capability/version gate (comment 3600499150) [Critical] GitDiffDialog.tsx:409 — available:false copy misleading (comment 3600499173) [Critical] GitDiffDialog.module.css:157 — per-line horizontal scrolling (comment 3600499181) [Critical] WorkspaceSection.module.css:87 — spare row width on non-interactive span (comment 3600499184) [Critical] localCommands.ts:92 — /diff always-on fallback (comment 3600499204)
— qwen3.7-max via Qwen Code /review
| const rel = path.relative(gitRoot, filePath); | ||
| if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) return null; | ||
| return rel; |
There was a problem hiding this comment.
[Suggestion] The absolute-path branch uses rel.startsWith('..'), which over-rejects compared to the segment-based .. check the relative-path branch uses three lines above.
Failure scenario: A file literally named ..foo (or ..config) inside the repository has path.relative(gitRoot, '/repo/..foo') return '..foo'. The startsWith('..') check rejects it, so fetchGitDiffHunksForFile returns null and the diff viewer cannot render this file. The relative-path branch correctly handles this with segment === '..'.
| const rel = path.relative(gitRoot, filePath); | |
| if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) return null; | |
| return rel; | |
| const rel = path.relative(gitRoot, filePath); | |
| if (rel === '' || rel === '..' || rel.startsWith('..' + path.sep) || path.isAbsolute(rel)) return null; | |
| return rel; |
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — its prompt was built but the harness did not track it.
Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run.
[Critical] workspace-git-diff.ts:112 — Runtime environment not passed to core Git helpers; daemon parent env inherited (blocker #2, still stands)
[Critical] workspace-git-diff.ts:206 — Git root expansion leaks sibling packages in monorepo (blocker #3, still stands)
[Critical] workspace-git-state.ts:61 — Cached watcher branch always preferred over fresh status branch (blocker #4, still stands)
[Critical] gitDiff.ts — Pathspec magic not pinned; directories match descendants (blocker #7, still stands)
[Critical] gitDiff.ts:395 — Trimming ls-files output breaks whitespace-only filenames (blocker #11, still stands)
[Critical] gitDiff.ts:411 — Windows path checks run on POSIX (blocker #12, still stands)
[Critical] gitDiff.ts:1191 — Relies on user status.showUntrackedFiles setting (blocker #13, still stands)
[Critical] gitDiff.ts:1342 — rebase-apply also used by git am; wrong operation reported (blocker #14, still stands)
[Critical] App.tsx — /diff not gated by trust; untrusted workspaces get 403 (blocker #15, still stands)
[Critical] App.tsx:1272 — Concurrent poll responses can roll back chip (blocker #16, still stands)
[Critical] App.tsx:1275 — Transient poll failure clears last good status (blocker #17, still stands)
[Critical] App.tsx:6782 — Branch/status/diff target from independent sources (blocker #18, still stands)
[Critical] ChatEditor.tsx:2114 — Git button click bubbles, focus lost (blocker #19, still stands)
[Critical] GitDiffDialog.module.css:157 — Per-cell horizontal scroll (blocker #20, still stands)
[Critical] GitDiffDialog.tsx — Highlight budget per-hunk, not per-file (blocker #21, still stands)
[Critical] GitDiffDialog.tsx:71 — Per-hunk tokenization loses lexical state (blocker #22, still stands)
[Critical] GitDiffDialog.tsx:309 — Truncated counts not labeled as lower bounds (blocker #24, still stands)
[Critical] GitDiffDialog.tsx:379 — No version/capability gate for /diff (blocker #25, still stands)
[Critical] GitDiffDialog.tsx:409 — Misleading Git unavailable for all failure states (blocker #26, still stands)
[Critical] WorkspaceSection.module.css:87 — Spare width on non-interactive span (blocker #28, still stands)
[Critical] localCommands.ts:92 — /diff shadows ACP/project/user commands (blocker #30, still stands)
[Critical] gitDiff.ts — Stash reflog per-worktree dir misses shared stash in linked worktrees (blocker #6, partially fixed)
— qwen3.7-max via Qwen Code /review
| export function parseGitDiff( | ||
| stdout: string, | ||
| truncatedPaths?: Set<string>, | ||
| ): Map<string, Hunk[]> { |
There was a problem hiding this comment.
[Suggestion] The truncatedPaths output parameter has no direct unit test asserting it is populated when lines exceed MAX_LINES_PER_FILE. — Failure scenario: The truncation-detection code path is only exercised indirectly through fetchGitDiffHunksForFile's integration test, which checks truncated: true but not which path was recorded. If a future refactor changes how parseGitDiff populates the set (e.g., recording the wrong key), the integration test would still pass.
| export function parseGitDiff( | |
| stdout: string, | |
| truncatedPaths?: Set<string>, | |
| ): Map<string, Hunk[]> { | |
| export function parseGitDiff( | |
| stdout: string, | |
| truncatedPaths?: Set<string>, | |
| ): Map<string, Hunk[]> { |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
- toRepoRelativePath: reject only a real climb-out (`..` or `../…`), not a
literal `..foo` filename at the repo root, which the bare startsWith('..')
over-rejected, leaving the diff viewer unable to render such a file.
- parseGitDiff: cover the truncatedPaths output set directly (it was only
exercised indirectly through fetchGitDiffHunksForFile).
|
LGTM ✅ |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
5 participants
What this PR does
This brings working-tree Git awareness to the Web Shell (the browser-based daemon session UI), which previously showed only a bare branch name. Three connected pieces land together:
↑N↓M, an operation badge), so it stays legible without relying on color.+/−counts, and per-file line-level diffs with per-side syntax highlighting. Untracked text files expand as a fully-added diff and deleted files still diff. It opens via the/diffcommand or by clicking a dirty chip, and loads each file's hunks on demand.All Git access goes through the daemon REST API — the browser never touches the repository directly — preserving the daemon-as-authority model and per-workspace trust gating (untrusted workspaces expose no Git surface and fetch nothing).
Why it's needed
The Web Shell was a Git blind spot: you could see the branch name but not what had changed. For an AI coding agent, knowing what is modified in the working tree is core context for understanding a session. This brings the Web Shell in line with the CLI's interactive diff experience and lays the read-only foundation (status + diff) that later write operations — commit, branch management, GitHub integration — will build on, gated by the same trust model.
Reviewer Test Plan
How to verify
↑N/↓M; hovering it shows the staged/unstaged/untracked breakdown./diff(or click the dirty chip): the Changes dialog lists changed files with their+/−counts; expand a file to see the line-level, syntax-highlighted diff. Confirm an untracked file renders as fully added.Evidence (Before & After)
Before: the Web Shell surfaced only a plain branch name — no dirty indicator, no way to see what changed.
After — sidebar git icons (one per trusted workspace, each in a different state) and the hover tooltip revealing branch + working-tree state:
The read-only Changes dialog — changed-file list, then a single file expanded to its line-level, syntax-highlighted diff:
Full app context showing the toolbar chip and the sidebar together:
Tested on
Environment
Local
npm run dev(Web Shell via Vite) driving a real Chromium through the project's mock-daemon Playwright harness for the captures above, plus unit tests across core / cli / sdk / web-shell.Risk & Scope
git statussubprocess cost bounded.Linked Issues
N/A — design and phase roadmap are tracked in
docs/design/2026-07-16-webshell-git-status-diff.mdanddocs/plans/2026-07-16-webshell-git-integration.md.中文说明
本 PR 做了什么
本 PR 为 Web Shell（基于浏览器的 daemon 会话界面）带来工作区 Git 感知能力——此前它只显示一个分支名。本次一并落地三个相互关联的部分：
↑N↓M、操作徽标），因此不依赖颜色也能看清。+/−行数），以及逐文件、逐行、双侧语法高亮的 diff。未跟踪的文本文件以「全部新增」的 diff 展开，已删除文件同样可 diff。可通过/diff命令或点击脏 chip 打开，且按需懒加载每个文件的 hunk。所有 Git 访问都经由 daemon REST API——浏览器绝不直接触碰仓库——从而保留「daemon 为唯一权威」的模型与逐 workspace 的信任门控（不受信任的 workspace 不暴露任何 Git 界面，也不发起任何 Git 请求）。
为什么需要
Web Shell 此前是 Git 盲区：只能看到分支名，看不到改了什么。对一个 AI 编码 agent 而言，了解工作区被改动了什么是理解一次会话的核心上下文。本 PR 让 Web Shell 与 CLI 的交互式 diff 体验对齐，并奠定只读基础（状态 + diff）——后续的写操作（提交、分支管理、GitHub 集成）将在此之上、并以同一套信任模型门控来构建。
评审验证计划
如何验证
↑N/↓M；悬停可看到 已暂存/未暂存/未跟踪 的明细。/diff（或点击脏 chip）：Changes 弹窗列出变更文件及其+/−行数；展开某个文件可看到逐行、语法高亮的 diff。确认未跟踪文件以「全部新增」呈现。证据（前后对比）
之前：Web Shell 仅显示一个纯分支名——没有脏标记，也无法看到改了什么。
之后——侧栏 git 图标（每个受信任 workspace 一个，各处于不同状态）以及悬停提示中揭示的分支 + 工作区状态：
只读 Changes 弹窗——变更文件列表，以及展开单个文件后的逐行、语法高亮 diff：
工具栏 chip 与侧栏同框的完整应用上下文：
测试环境
运行环境
本地
npm run dev（Vite 运行 Web Shell）通过项目的 mock-daemon Playwright 测试支架驱动真实 Chromium 完成上述截图，另有覆盖 core / cli / sdk / web-shell 的单元测试。风险与范围
git status子进程开销控制在合理范围。关联 Issue
无——设计与阶段路线图记录于
docs/design/2026-07-16-webshell-git-status-diff.md与docs/plans/2026-07-16-webshell-git-integration.md。