feat(web-shell): manage pairing approvals - #8081
Conversation
E2E evidenceVerified locally on macOS with Playwright Chromium against the Web Shell mock daemon:
Light-theme result: 
|
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
🖼️ 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 |
|
Thanks for the PR! Template looks good ✓ Problem: this is a real feature gap, not theoretical hardening. The Web Shell could approve a pending pairing code but gave operators no way to see or revoke the resulting access — they had to leave the UI and inspect local state. It's a phased piece of the labeled roadmap issue #7209 (P2, scope/web-shell, daemon), and it builds on the workspace-scoped pairing-approval API that already landed in the previous phase. Direction: aligned. This completes the management surface #7209 asks for, and it consumes an already-merged daemon contract ( Size: not a core-module change — everything sits in Approach: the scope feels right and the implementation leans on existing patterns throughout — the new Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个真实的功能缺口，而非理论性加固。Web Shell 之前可以批准待处理的配对码，但管理员无法查看或撤销批准后产生的访问权限，只能离开页面手动检查本地状态。这是带标签的路线图 issue #7209（P2、scope/web-shell、daemon）的一个阶段性部分，并且建立在上一阶段已合入的工作区范围配对授权 API 之上。 方向：对齐。它补全了 #7209 要求的管理界面，并且复用了已合入的 daemon 契约（ 规模：非核心模块改动——全部位于 方案：范围合理，实现充分复用既有模式——新增的 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewI wrote my independent proposal first (thin No blockers. A few things I checked specifically:
One non-blocking product nit for the author to consider: when a revoke races and the approval is already gone, the UI refreshes the list (good) but also shows a destructive-styled alert titled "Pairing approvals were not updated". For a benign concurrent-revoke case a neutral tone might read better — but it's tested and intentional, so not a merge concern. Files changed (17 of 17 shown)
Testing evidence (this PR's own CI, read via the API — I did not run any PR code)The main ubuntu unit suite and the Web-shell Visuals job (which carries the Playwright channels flow) are still running on the reviewed commit; nothing has failed so far and Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The behavioral claim (approve → sender appears in the list → revoke → 中文说明代码审查我先独立写出自己的方案（在 webui 中做与 无阻塞项。我重点核对了：
一个非阻塞的产品小建议：当撤销发生竞态、授权已不存在时，UI 会刷新列表（好），但同时弹出标题为「Pairing approvals were not updated」的 destructive 提示。对于这种良性并发场景，中性语气可能更合适——但这是有测试覆盖的有意设计，不构成合入问题。 测试证据（来自本 PR 自身的 CI，通过 API 读取——我未运行任何 PR 代码）主 ubuntu 单测套件与 Web-shell Visuals 任务（包含 Playwright 频道流程）仍在所审 commit 上运行；目前没有失败， 行为性结论（批准 → 用户出现在列表 → 撤销 → 发出 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, convention-following review with no blockers; the only thing keeping this from a 5 is that CI hasn't landed green yet on the reviewed commit. Stepping back: this is exactly the kind of PR that's easy to maintain in six months. It doesn't try to do more than the goal needs — every file earns its place — and it leans on patterns already in the codebase instead of adding parallel structure. My independent proposal and the actual diff converged, which means the approach isn't fighting the repo. The phase boundary is clean too: it consumes an already-merged daemon contract and adds only the UI + thin action wrappers on top, so the blast radius is the Web Shell channels editor. The tests are about as good as this gets for a UI flow: they assert the approve → list → revoke sequence, that revoke removes only the selected sender, and that the 404 race reconciles the list rather than leaving stale state. The one reservation I'd leave for the author (non-blocking) is the destructive-styled alert on the benign "already gone" path — worth a thought, not a rework. I'd merge this. Since the ubuntu unit suite and the Web-shell Visuals job are still running on the reviewed commit, I'm deferring the approval rather than attesting to a result that doesn't exist yet — approval will land automatically once CI is green on this commit, and will be withheld if anything lands red or the head moves. 中文说明置信度：4/5 —— 审查干净、遵循约定、无阻塞项；唯一让它未到 5 分的原因，是所审 commit 上的 CI 尚未落绿。 退一步看：这正是六个月后仍易于维护的那种 PR。它没有做超出目标的事——每个文件都物有所值——并且依赖代码库中既有的模式，而非另起一套结构。我的独立方案与实际 diff 收敛，说明方案没有与仓库对着干。阶段边界也很清晰：它复用一个已合入的 daemon 契约，只在其上增加 UI 与薄 action 封装，因此影响范围限定在 Web Shell 频道编辑器。 对一个 UI 流程而言，这里的测试已属上乘：它断言了批准 → 列表 → 撤销的完整链路、撤销只移除选中的用户、以及 404 竞态会重新对齐列表而非留下过期状态。我给作者留的唯一保留意见（非阻塞）是良性「已不存在」路径上那个 destructive 样式的提示——值得斟酌，但不必返工。 我会合入它。由于主 ubuntu 单测套件与 Web-shell Visuals 任务仍在所审 commit 上运行，我选择延迟批准，而不是为一个尚不存在的结果背书——CI 在该 commit 上转绿后会自动批准；若有任何检查转红或 head 移动，则会保留不批。 — Qwen Code · qwen3.8-max-preview 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 — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview 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.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
Review —
|
| Check | Result |
|---|---|
web-shell channels suite (5 files) |
42/42 pass |
webui actions.test.ts + useDaemonChannels.test.tsx |
25/25 pass |
prettier --check on all 17 changed files |
clean |
eslint on the 4 changed source files |
clean |
| Mutation probes on the new code | 1 killed / 4 survived (below) |
Full web-shell suite shows 23 failing files, all environmental in my tree (Cannot find package '@datafe-open/markdown-chart', missing dist/index.js, a stale useUsageDashboard mock) — none touch this PR's files.
1. Approving a request while a revoke is in flight wipes the approvals list · ChannelPairingRequests.tsx:252
revoke() writes setApprovedSenderIds(result.senderIds) with no generation guard — unlike the load effect, which has its active flag (:145-171). Meanwhile the two actions don't disable each other: Approve is disabled={Boolean(approvingCode)} (:385) and Revoke is disabled={Boolean(revokingSenderId)} (:485), so Approve stays clickable during a revoke round-trip.
Sequence:
- Approvals
['old-user'], one pending request fromuser-42. - Revoke
old-user→ confirm → DELETE in flight. - Click Approve (still enabled) → resolves → bumps
approvalsReloadToken→listApprovalsreturns the fresh['old-user', 'user-42']. - The slow DELETE resolves with its pre-approve snapshot
senderIds: []→ clobbers the newer read.
I reproduced this with a throwaway test using a deferred revokeApproval. Final rendered state:
Ada can now use this Channel. … No pending requests
Pairing approvals … 0
Pairing approval for old-user was revoked.
No pairing approvals — Approved sender IDs will appear here.
The operator has just granted user-42 access and the panel reports "No pairing approvals". Server state is correct; only the display is stale, and it only clears on a manual refresh. Under-reporting rather than over-reporting access, so not a security hole — but this panel is exactly what an operator reads to decide who has access.
Either fix works:
- Cross-disable the two actions (
disabled={Boolean(approvingCode) || Boolean(revokingSenderId)}on both), matching the intent of the existing in-section guards; or - capture
approvalsReloadTokenat the top ofrevoke()and skip the write if it has moved — the same shape as the effect'sactiveflag.
2. The allowedUsers read has no coverage at all · ChannelEditorDialog.tsx:109
configuredAllowedUsers() reads instance.config['allowedUsers'], and staticAllowedUsers is only ever supplied by that call in production. But allowedUsers appears exactly once in the entire web-shell client — in that line. No unit test, no E2E scenario, no visual scenario sets it.
Mutation: renaming the key to 'allowedUsersMUTANT' leaves 42/42 green. The "Configured allowlist" surface — a headline item in the PR description — is only exercised via a hand-passed prop in ChannelPairingRequests.test.tsx; the wiring that actually produces it is unpinned.
The key itself is correct (channel-settings-store.ts:162 validates allowedUsers as a shared string-array field, and buildChannelUpsertRequest spreads instance.config, so saving from the editor doesn't drop it). It just has nothing keeping it correct. A single ChannelEditorDialog test with instance.config = { senderPolicy: 'pairing', allowedUsers: ['configured-user'] } closes this and #3 at once — note the shared INSTANCE fixture is senderPolicy: 'open', so ChannelPairingRequests is never mounted in any editor-dialog test today.
3. Nothing asserts the allowlist alert is absent when there are no configured users · :499
Relaxing staticAllowedUsers.length > 0 to >= 0 also leaves 42/42 green. Combined with #2, the alert can neither appear when it should nor stop appearing when it shouldn't without a test noticing.
4. No channel-switch isolation test for approvals · :149
The pending-requests side has does not show requests from the previous Channel while loading. The approvals side has no equivalent: deleting setApprovedSenderIds([]) from the reset block leaves 42/42 green. The code is correct today, but approved sender IDs are the more sensitive of the two lists to leak across a channel switch — worth the sibling test, given the precedent already exists two effects up.
5. Minor
- Concurrent-revoke guard untested (
:240): deletingif (revokingSenderId) return;leaves 42/42 green. Low severity (the buttons are disabled), but it's the last line of defence and it's free to pin. - Visual baseline reframed (
screenshots.spec.ts):captureScreenshotcallspage.screenshot()withoutfullPage, so it is viewport-clipped. Replacingawait editHeading.click()withscrollIntoViewIfNeeded()on the Pairing approvals heading meanschannel-editor-existing-{light,dark}now frames the bottom of the dialog instead of the top — the previously-covered region silently drops out of the baseline. Also worth confirming what the removed click was for: it was added deliberately in feat(web-shell): add Channel configuration flows #7893 and, on a dialog that autofocuses its first field, reads like a focus-ring determinism guard. Consider keeping the original capture and adding a separatechannel-editor-approvals-{theme}shot. - A11y (
:296,:405): the new block lives inside the same<section aria-labelledby={headingId}>whose accessible name is "Pending requests", and its<h4>carries noid. A screen reader announces the approvals list and its Revoke buttons as part of the pending-requests region. A second<section>with its ownuseId-backedaria-labelledbymatches what the first one already does. - i18n: EN/ZH keys are both present and consistent — but
type Messages = Record<string, MessageValue>is an open record, so nothing type-checks parity, and there's no parity test in this package. The existinguses consistent approval wording in Chinesetest has no counterpart for the new approvals strings.
Verdict
No blockers on the daemon contract or the wiring. Finding #1 is a real user-visible defect and cheap to fix; #2–#4 are three surviving mutants concentrated on the parts of this change that the PR description advertises most, and one editor-dialog test with a pairing instance closes most of them.
中文说明
结论
整体方向正确：编辑器接入了 #8045 的工作区级配对授权 API，批准后会刷新，撤销有明确的确认对话框；错误处理与既有的待处理请求路径保持一致；webui 侧的读操作带 withActionTimeout、写操作不带，符合现有约定。
已运行的验证（PR head 3b66b09，隔离 worktree）：web-shell channels 套件 42/42 通过；webui 相关 2 个测试文件 25/25 通过；prettier 与 eslint 干净。web-shell 全量套件的 23 个失败文件均为环境问题（缺 @datafe-open/markdown-chart、未构建 dist/index.js），与本 PR 无关。
1. 撤销进行中再批准请求会清空授权列表（ChannelPairingRequests.tsx:252）— 唯一的功能缺陷。revoke() 写入 setApprovedSenderIds 时没有代际保护（加载 effect 有 active 标志），而"批准"按钮在撤销期间并未禁用。若批准先返回并触发刷新、随后较慢的 DELETE 才返回其过期快照，就会覆盖较新的读取结果。我用延迟 revokeApproval 的临时测试复现了：操作员刚刚授予 user-42 访问权限，面板却显示"No pairing approvals"，只有手动刷新才恢复。服务端状态正确，仅显示错误。修法二选一：两个操作互相禁用；或在 revoke() 开头记录 approvalsReloadToken，变化则跳过写入。
2. allowedUsers 读取完全没有测试覆盖（ChannelEditorDialog.tsx:109）。allowedUsers 在整个 web-shell client 中只出现这一处；把键名改成 'allowedUsersMUTANT' 后 42/42 仍然全绿。键名本身是对的（channel-settings-store.ts:162 有校验，buildChannelUpsertRequest 会展开 instance.config，保存不会丢失白名单），但没有任何测试守住它。注意共享的 INSTANCE fixture 是 senderPolicy: 'open'，因此编辑器对话框的测试从未挂载 ChannelPairingRequests。
3. 没有断言"无配置白名单时提示不出现"（:499）。把 length > 0 放宽为 >= 0 后依然 42/42 全绿。
4. 缺少切换频道时的授权列表隔离测试（:149）。待处理请求侧已有同类测试，授权侧没有；删除 setApprovedSenderIds([]) 后仍 42/42 全绿。已授权的用户 ID 跨频道泄漏比待处理请求更敏感，建议补齐。
5. 次要问题：撤销并发保护（:240）删掉后测试仍全绿；视觉基线被改变——captureScreenshot 未使用 fullPage，把 editHeading.click() 换成 scrollIntoViewIfNeeded() 后，channel-editor-existing-{light,dark} 截取的是对话框底部而非顶部，原有覆盖区域悄然丢失（那个 click 是 #7893 有意加入的，看起来是为了避免焦点环带来的不确定性），建议保留原截图并新增一张 channel-editor-approvals-{theme}；无障碍方面，新区块位于名为 "Pending requests" 的 <section> 内且 <h4> 没有 id，建议单独开一个带 aria-labelledby 的 <section>；i18n 的 EN/ZH 键已齐全，但 Messages 是开放 Record，类型层面并不校验对齐，本包也没有对齐测试。
守护契约与接线没有阻断性问题。#1 是真实且易修的用户可见缺陷；#2–#4 是三个存活变异体，恰好集中在本 PR 最主打的功能上，补一个带 pairing instance 的编辑器对话框测试即可覆盖大部分。
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.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /verify |
|
Sandboxed verification: Skipped because GitHub had not computed the PR merge ref after several retries — try again shortly. 中文 — 判定：
|
|
已修复于 4a1d2d1：批准与撤销操作现在互相禁用，避免慢撤销的旧快照覆盖批准后的授权刷新；新增两个 deferred-promise 行为测试覆盖双向并发。验证：focused Vitest 22/22、全仓 npm run build、全仓 npm run typecheck、变更文件 ESLint/Prettier 与 git diff --check 均通过。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max-preview via Qwen Code /review
Review:
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 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. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Same-run verification repairThe previous commit was rejected because Root causeThe prior round addressed @wenshao's suggestion that the revoke-confirmation Fix (3 lines, behavior-preserving)
Net behavior is exactly what the prior round intended: with a target set the Review feedbackNo new inline findings this round — this commit only repairs the rejected Verification
中文说明同轮验证修复上一个提交被拒绝，原因是 根因上一轮为处理 @wenshao 的建议（撤销确认对话框标题在退场动画期间会闪现 "Revoke pairing approval for ?"），将插值改为 修复（3 行，行为保持一致）
最终行为与上一轮的预期完全一致：设置了目标时标题显示 "Revoke pairing approval for ?"；在目标被清空后对话框淡出期间显示 "Revoke pairing approval for this sender?"，而非异常的 "…for ?"。 审查反馈本轮没有新的行内发现——本提交仅修复被拒绝的构建。此前所有行内建议已在之前的分支提交（ 验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max-preview via Qwen Code /review
Local verification report — real build + real browserVerified Results
Non-vacuity: replaying this PR's Backend claim is accurate. Approve → list → revoke, driven in a real browser
Wire-level assertions from that run: exactly one
Failure and edge states
Notes (all non-blocking)N1 — the approvals list blanks out on every refresh, not just on Channel change. N2 — the badge reads N3 — a wedged N4 — the N5 — revoking an ID that is also allowlisted reads as "access removed". Screenshot: green N6 — copy. (a) "Pending requests" already warns "Approvals take effect immediately; Save and N7 — nit. During a revoke, "Refresh pairing approvals" is disabled but "Refresh pairing requests" FYI (not this PR): switching the policy to Open unmounts the panel while the daemon keeps The repo's own visual captures, reproduced locally
中文版本地验证报告 —— 真实构建 + 真实浏览器在独立 worktree 中基于 结果
非空转验证： 把本 PR 的 后端语义确认无误。 真实浏览器中的「批准 → 进列表 → 撤销」见上方 flow 图。该次运行的协议层断言：恰好 1 次 备注（均不阻塞）N1 —— 每次刷新都会清空已配对列表，而不只是切换频道时。 N2 —— 加载失败时 badge 仍显示 N3 —— N4 —— N5 —— 撤销一个同时在白名单里的 ID，看起来像「权限已移除」。 截图：绿色 ✓「Pairing approval for N6 —— 文案。（a）「待处理的配对请求」已有提示「批准立即生效；保存和取消都不会撤销」。已配对分区 N7 —— 细节。 撤销进行中，「刷新已配对用户」被禁用而「刷新配对请求」仍可点，同时批准是禁用的。 顺带一提（非本 PR）： 把策略切到「开放模式」会卸载该面板，但 daemon 仍保留 |
|
Released in v0.21.2. |
5 participants
What this PR does
Adds workspace-scoped pairing approval management to the Channel editor. Saved Channels in pairing mode now show approved sender IDs below pending requests, refresh after approving a request, and let operators revoke an approval through an explicit confirmation dialog. The editor also distinguishes persisted pairing approvals from users configured in the Channel allowlist.
Why it's needed
The previous Web Shell flow could approve pending pairing codes but offered no way to see or remove the resulting access. Operators had to leave the UI and inspect local state manually, and revoking a pairing approval could be confused with changing the configured allowlist.
Reviewer Test Plan
How to verify
Open a saved Channel in pairing mode and confirm that pending requests and pairing approvals appear directly below the access policy. Approve a pending request and expect its sender ID to appear in the approvals list. Choose Revoke, cancel once to confirm no change, then confirm revocation and expect the sender ID to disappear. When the Channel has configured allowlist users, confirm the editor explains that those users remain allowed after a pairing approval is revoked.
Focused component and workspace-action tests pass (56 tests). The Web Shell Channel Playwright flow passes, including the approve-to-list-to-revoke sequence. Channel manager visual tests pass in light and dark themes. Repository build, typecheck, lint, and the final Web Shell scoped build/typecheck/lint all pass.
Evidence (Before & After)
Before: the pairing-mode editor only showed pending requests and could not list or revoke existing pairing approvals.
After: the editor shows pairing approvals below pending requests, provides explicit revocation, and calls out configured allowlist access. A rendered light-theme screenshot is attached in a follow-up E2E evidence comment.
Tested on
Environment (optional)
Local workspace build and Playwright Chromium against the Web Shell mock daemon.
Risk & Scope
Linked Issues
Part of #7209
中文说明
本 PR 做了什么
为频道编辑器增加工作区范围内的配对授权管理。以配对模式保存的频道现在会在待处理请求下方显示已授权的用户 ID，批准请求后自动刷新，并允许管理员通过明确的确认对话框撤销授权。编辑器同时区分持久化配对授权与频道配置白名单中的用户。
为什么需要
此前 Web Shell 可以批准待处理的配对码，但无法查看或移除批准后产生的访问权限。管理员只能离开页面手动检查本地状态，而且撤销配对授权容易与修改配置白名单混淆。
Reviewer Test Plan
如何验证
打开一个已保存且处于配对模式的频道，确认待处理请求和已配对用户直接显示在准入策略下方。批准一条待处理请求，预期其用户 ID 出现在授权列表中。点击撤销，先取消并确认列表不变，再确认撤销并预期该用户 ID 消失。若频道配置了白名单用户，确认编辑器明确说明撤销配对授权后这些用户仍然可以访问。
聚焦组件与工作区 action 测试通过，共 56 项。Web Shell 频道 Playwright 流程通过，覆盖批准、进入列表和撤销的完整链路。频道管理器明暗主题视觉测试通过。仓库构建、类型检查、lint，以及最终 Web Shell 范围内的构建、类型检查和 lint 全部通过。
证据（前后对比）
改动前：配对模式编辑器只显示待处理请求，无法列出或撤销已有配对授权。
改动后：编辑器在待处理请求下方显示已配对用户，提供明确的撤销操作，并提示配置白名单仍然生效。渲染后的明亮主题截图会附在后续 E2E 证据评论中。
测试平台
环境（可选）
本地工作区构建，以及针对 Web Shell mock daemon 的 Playwright Chromium。
风险与范围
关联 Issue
属于 #7209 的一部分。