feat(daemon): guard cross-worktree Git mutations - #8684
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /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,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
🩺 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 |
|
Thanks for the PR! Template looks good ✓ Problem: this one checks out as a real boundary hole rather than a theoretical concern. The shell tool validates the explicit Direction: aligned. The daemon/ACP path is a first-class surface in this repo, and enforcing the boundary host-side — with the boundary values taken from daemon-owned session state ( Size: ~322 production lines (281 for the guard itself, ~41 wiring across Approach: scope feels right. Allowing relocated read-only commands while failing closed on mutating/unclassifiable ones is the correct tradeoff for worktree workflows, and the chained- Risk: Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:这是一个真实存在的边界漏洞,而不是理论性的担忧。shell tool 会校验显式的 方向:对齐。daemon/ACP 是本仓库的一等公民,把边界放在 host 侧强制执行(边界值取自 daemon 持有的 session 状态 规模:约 322 行生产代码(guard 本体 281 行, 方案:范围合理。对重定位后的只读命令放行、对 mutating/无法分类的命令 fail-closed,是 worktree 工作流下正确的取舍;chained 风险: 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Superseded by #8687, which uses the in-repository feature branch as intended. Closing this duplicate fork-based PR. |
Code reviewReviewed independently before reading the diff: given the existing 1. CI is red on the serve fast-path bundle check, and unit tests never ran (blocker). The ubuntu Test job fails at 2. Qualified Git paths bypass the guard entirely (blocker, in the PR's own scope). 3. Observations, not blockers — these are documented non-goals, and the codebase's existing Git handling (commit attribution in the core shell tool) takes the same posture: sequenceDiagram
participant P1 as ACP child managed guard
participant P2 as BridgeClient daemon host
participant P3 as Daemon git-worktree guard
participant P4 as External guard provider
P1->>P2: tool call externalToolGuardPrepare
P2->>P2: validate session and active prompt
P2->>P3: request plus daemon-owned workspaceCwd and effectiveCwd
P3->>P3: parse run shell command for Git relocation
alt relocation escapes effectiveCwd and is mutating or unknown
P3-->>P1: denied, provider not consulted
else allowed and provider configured
P3->>P4: forward request
P4-->>P1: provider decision
else allowed and no provider
P3-->>P1: allowed
end
Files changed (10 of 10 shown)
Test evidence — the PR's own CI, read via the APIPer triage rules no PR code was built or executed here; the table quotes the PR's own check-runs on the reviewed commit, fetched once (no polling —
Not verified: the full unit suite (blocked by the bundle failure on this commit), and Windows/Linux runtime behavior (author tested macOS only — their report, not re-run here). Sandboxed verification would settle the wiring claim: 中文说明代码审查:方案与我独立推演的做法一致——host 侧对
观察项(非 blocker,均属已声明的非目标,且与 core shell tool 里 commit 归属逻辑的姿态一致): 测试证据:按 triage 规则,这里没有构建或执行任何 PR 代码;表格直接引用该 commit 的 CI check-run(一次性拉取,不轮询——review 时 沙箱验证可以敲定接线声明: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design is the right one and the trust wiring is genuinely careful, but the PR is red on the repo's own bundle gate with zero CI unit-test evidence, and the parser has a confirmed trivial bypass inside its own stated scope. Stepping back: this is not a solution looking for a problem — I verified the escape exists in the current code, the host-side placement is correct, and the implementation matches what I'd have proposed independently, down to fail-closed dynamics and daemon-owned boundary values. The test suite pins real Git/shell behavior rather than toy cases, and the design doc states its non-goals honestly. If that were the whole picture this would be close to approve. What stops it is the state it ships in. The fast-path bundle check fails on this commit — mechanically fixable via the dynamic-import pattern the same file already uses for exactly this hazard — and because it fails before vitest, there is currently no CI evidence that even the green-looking parts hold. On top of that, a guard whose stated job is recognizing @wenshao — requesting changes for the three items above (bundle closure, qualified-git bypass, 中文说明Confidence: 2/5 —— 设计方向正确,信任链接线相当仔细,但 PR 在仓库自己的 bundle gate 上是红的、没有任何 CI 单测证据,且 parser 存在一个在其自身声明范围内的已确认的简单绕过。 退一步看:这不是"为方案找问题"——逃逸路径我在现有代码里核实过,host 侧的放置是对的,实现与我独立给出的方案一致,包括对动态目标 fail-closed 和 daemon 持有边界值。测试固定的是真实的 Git/shell 行为而不是玩具场景,设计文档也诚实地写明了非目标。如果只有这些,它接近 approve。 挡住它的是当前的提交状态:fast-path bundle 检查在本 commit 上失败——用同一文件里针对同类风险已有的动态 import 模式即可机械修复——而且它在 vitest 之前就失败了,所以目前没有任何 CI 证据表明那些看起来没问题的部分真的成立。另外,一个以识别 @wenshao —— 就上述三点(bundle 闭包、带路径 git 的绕过、 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes — CI is red on the serve fast-path bundle check (unit tests never ran on this commit), the guard has a confirmed bypass via path-qualified git invocations, and the command-substitution relocation case needs verification against the stated fail-closed policy. Details in the review notes above. 🙏
What this PR does
Adds a built-in host-side guard for model-issued
run_shell_commandcalls inqwen serve. The guard recognizes Git repository relocation through-C,--work-tree, and--git-dir, then blocks mutating or unclassified commands when the resolved target escapes the session's current effective working directory. It uses daemon-owned session state, composes before an optional external guard, handles compound shell forms, dynamic targets, symlink ancestors, chained-C, and Git global options with values.Worktree sessions use their live
effectiveCwdas the boundary, so mutations inside the session's own worktree remain allowed while mutations targeting another checkout or worktree are denied. Read-only relocated Git commands remain allowed.The managed guard is installed only for daemon ACP children. CLI/TUI shell validation, permission classification, confirmation, YOLO behavior, and shared core tool execution are unchanged.
Why it's needed
A daemon model can start a shell inside its assigned workspace but relocate Git to another checkout with
git -C,--work-tree, or--git-dir. That bypasses the existing shell working-directory boundary and can mutate a sibling worktree or unrelated checkout. Enforcing the boundary in the daemon host prevents the ACP child from forging workspace context and avoids changing normal CLI behavior.Reviewer Test Plan
How to verify
git -C <other-worktree> reset --hardis denied before execution.--work-tree,--git-dir, a dynamic relocation target, a symlinked path, a compound command, and a value-taking Git global option such as--namespace; each mutating or unclassified external relocation should be denied.git -C <other-worktree> status --shortremains allowed.effectiveCwdremains subject only to the existing permission flow.Evidence (Before & After)
N/A — daemon policy change with unit and integration coverage; no UI changes.
Tested on
Environment (optional)
macOS, Node.js 22-compatible workspace. Passed: 25 daemon guard tests, 5 managed ACP guard tests, 97 ACP bridge tests, ACP bridge typecheck, targeted ESLint, Prettier, pre-commit hooks, normal CLI Git regression tests (36/36 in independent verification),
npm run dev -- --version, and daemon startup. The targetedrun-qwen-serve.test.tscould not be collected in this checkout because the workspace is missing@qwen-code/channel-github; the root build/typecheck is blocked by the same missing channel packages and existing unrelated type errors.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
为
qwen serve中模型发出的run_shell_command调用增加 Daemon host 侧内置 guard。该 guard 识别通过-C、--work-tree、--git-dir进行的 Git 仓库重定位;当解析后的目标逃出 session 当前有效工作目录时,阻止可变更或无法分类的命令。策略使用 Daemon 持有的可信 session 状态,在可选 external guard 之前执行,并覆盖 compound shell、动态目标、symlink ancestor、连续-C以及带值的 Git 全局选项。Worktree session 使用实时
effectiveCwd作为边界,因此 session 自己 worktree 内的 mutation 仍然允许,而指向其他 checkout 或 worktree 的 mutation 会被拒绝。重定位后的只读 Git 命令仍然允许。Managed guard 只安装到 Daemon ACP child。CLI/TUI 的 shell 校验、权限分类、确认、YOLO 行为以及共享 core tool 执行均保持不变。
为什么需要
Daemon 模型可以让 shell 从分配的 workspace 内启动,但通过
git -C、--work-tree或--git-dir将 Git 重定位到另一个 checkout。这会绕过现有 shell 工作目录边界,并可能修改 sibling worktree 或无关 checkout。把边界放在 Daemon host 强制执行,可以防止 ACP child 伪造 workspace context,同时避免改变普通 CLI 行为。Reviewer Test Plan
如何验证
git -C <other-worktree> reset --hard等 mutation 命令在执行前被拒绝。--work-tree、--git-dir、动态 relocation target、symlink path、compound command,以及--namespace这类带值的 Git 全局选项;所有指向外部的 mutation 或无法分类命令都应被拒绝。git -C <other-worktree> status --short仍然允许。effectiveCwd内目录的 Git mutation 仍只受现有 permission flow 约束。Evidence(Before & After)
N/A——这是带单元和集成覆盖的 Daemon 策略改动,没有 UI 变化。
Tested on
Environment(可选)
macOS,Node.js 22 兼容 workspace。已通过:25 个 Daemon guard 测试、5 个 managed ACP guard 测试、97 个 ACP bridge 测试、ACP bridge typecheck、定向 ESLint、Prettier、pre-commit hooks、普通 CLI Git 回归测试(独立验证 36/36)、
npm run dev -- --version和 Daemon 启动。当前 checkout 缺少@qwen-code/channel-github,因此定向run-qwen-serve.test.ts无法完成测试收集;root build/typecheck 也被相同的 channel package 缺失和已有无关类型错误阻塞。风险与范围
Linked Issues
N/A