-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(daemon): guard cross-worktree Git mutations #8687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2480163
2b0e8cd
107b5f6
b175cd7
2af7419
b1b7606
83428be
18e997a
ab1e3e2
45753e0
c14ffc4
7f66be6
b11585b
dfeb461
af36eca
8112fae
338e0c7
d9a3911
012d252
2bf64cd
0a58fe7
cbcde28
3a247f7
a1652cc
581d2bb
6b755d1
8baf7a7
0f970d4
8e46dba
089c3e3
a2736b8
cf2ea23
7f4746f
6273464
01587ab
2f0b99f
a2ae8bb
6deb8bd
a43faa8
1203072
e595f7b
70f5162
68eb534
3333331
636602c
5fc219d
6128e69
d95d87c
a5f8099
c7b6aa4
a0427ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -434,6 +434,71 @@ Notes: | |
| > matched case-sensitively by yargs `choices` (`--memory-project-scope | ||
| Workspace` is rejected). Use lowercase values when copying between the two. | ||
|
|
||
| ### Built-in daemon Git relocation guard | ||
|
|
||
| Every managed daemon ACP session applies a built-in pre-execution guard for | ||
| model shell commands, independent of `--external-tool-guard-mode` and without | ||
| any capability advertisement. The daemon owns the bound workspace and the | ||
| session's current effective working directory; both are supplied from trusted | ||
| session state and never accepted from the ACP child. | ||
|
|
||
| The guard inspects the tools that run a shell command line — `run_shell_command` | ||
| and `monitor` — and denies a mutating Git | ||
| command before execution when its repository location resolves outside the | ||
| session's effective working directory. Relocation is recognized for literal | ||
| forms of `git -C <path>`, `git --git-dir[=]<path>`, | ||
| `git --work-tree[=]<path>`, leading | ||
| `GIT_DIR`/`GIT_WORK_TREE`/`GIT_COMMON_DIR`/`GIT_INDEX_FILE` assignments (also | ||
| when made through `export`/`declare`/`readonly`, which keep them in the | ||
| environment of every later command in the chain), | ||
| directory-shifting wrapper flags (`env -C`, `sudo -D`), and `cd`, `pushd`, or | ||
| `popd` builtins earlier in the same command chain. Common wrapper prefixes | ||
| (`sh -c`, `bash -c`, `eval`, `sudo`, `nohup`, `timeout`, `exec`, `command`, | ||
| `builtin`, | ||
| `env`, path-qualified `git` binaries, and `{ …; }` / `! …` shell syntax) are | ||
| unwrapped so the same policy applies to the inner Git invocation, and `$(…)` | ||
| or backtick substitution bodies are analyzed as commands of their own. | ||
|
|
||
| A sub-agent pinned to its own worktree is contained to that worktree rather | ||
| than to the session's directory; a shell call whose execution directory the | ||
| daemon cannot place is denied. | ||
|
|
||
| Relative targets resolve from the command's effective starting directory | ||
| (`arguments.directory` when present, otherwise the session's current effective | ||
| working directory) after canonical path resolution, including `.git` gitfile | ||
| redirects, symlinks, and per-worktree administrative directories. A relocated | ||
| target that cannot be fully resolved before execution — a dynamic target | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This paragraph's two sentences contradict each other and both misdescribe the shipped denial scope for unresolved relocation targets. Sentence 1 scopes denial of unresolvable targets to 'mutating or unclassifiable subcommands' — but Suggested fix: replace the two sentences with the actual rule: dynamic/unresolved targets and command-executing 中文说明本段的两句话自相矛盾,且都错误描述了"无法解析的重定位目标"的实际拒绝范围。第一句把不可解析目标的拒绝限定为"可变更或不可分类的子命令"——但 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| (`$VAR`, backticks, `~`, globs), a path that does not exist yet, or an | ||
| unreadable indirection — is denied for mutating or unclassifiable subcommands. | ||
|
Comment on lines
+470
to
+472
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-36: two adjacent sentences make contradictory claims about unresolvable relocated targets and read-only subcommands, and each is half-wrong against the code. — Probe-verified behavior: a dynamic target ( 中文说明相邻两句对不可解析的重定位目标与只读子命令给出互相矛盾的描述,且各自对代码都只对了一半。探针验证的行为:动态目标( — qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| A relocated target that cannot be resolved is denied whatever the subcommand | ||
| is — including the read-only ones. Relocated commands whose subcommand is one | ||
|
Comment on lines
+472
to
+474
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R9-18: Two adjacent sentences in the new guard doc contradict each other, and each misstates the shipped code on a different axis of 'unresolvable target'. — Failure scenario: Dynamic targets are denied for read-only subcommands too (denyDynamicRelocation runs before the read-only allowance — contradicting the first sentence), while a not-yet-existing path is allowed for read-only subcommands (the allowance returns before the existence loop — contradicting the second; pinned test 'allows relocated read-only Git commands' confirms). An operator cannot determine which relocated read-only commands fail closed. 中文说明[Suggestion] 新 guard 文档中两个相邻句子互相矛盾,且各自在“不可解析目标”的不同轴上误述已交付代码。 — 失败场景:动态目标对只读 subcommand 同样拒绝(denyDynamicRelocation 先于只读放行 —— 与第一句矛盾);不存在的路径对只读 subcommand 被允许(放行先于存在性循环返回 —— 与第二句矛盾;pin 测试 'allows relocated read-only Git commands' 确认)。操作者无法判断哪些重定位只读命令是 fail-closed。 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| of a small verified read-only set (`rev-parse`, `cat-file`) remain allowed | ||
| once the target resolves, unless the command carries command-executing `-c` | ||
| config, or | ||
| it carries a `--output`, `--textconv`, or `--filters` flag: those write a file | ||
| or run the target repository's configured drivers. Commands with no recognized | ||
| relocation keep their existing behavior. | ||
| Denials are final and are reported to the model as | ||
| `Daemon shell guard denied a mutating Git command…` for a resolved, dynamic, | ||
|
Comment on lines
+481
to
+482
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R9-49: The documented taxonomy of guard denial messages omits the third, reachable family 'Daemon shell guard could not establish the execution directory of this call: …' (UNVERIFIABLE_SCOPE_DENIAL_PREFIX, guard.ts:260), even though the section describes its trigger two paragraphs earlier. — Failure scenario: A sub-agent pinned to a directory the daemon cannot place from state it owns produces a denial matching neither documented prefix; the message does not contain the word 'denied', so triage by the two documented prefixes — or grepping logs for 'Daemon shell guard denied' — silently misses these failures. 中文说明[Suggestion] 文档的 guard 拒绝消息分类遗漏了第三个可达家族 'Daemon shell guard could not establish the execution directory of this call: …'(UNVERIFIABLE_SCOPE_DENIAL_PREFIX,guard.ts:260),尽管该节两段之前已描述其触发条件。 — 失败场景:子代理固定到 daemon 无法从自有状态定位的目录时产生的拒绝不匹配任何已文档化前缀;该消息不含 'denied' 一词,按两个已文档化前缀分诊 —— 或在日志中 grep 'Daemon shell guard denied' —— 会静默漏掉这些失败。 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| or unresolvable repository location, and as | ||
|
Comment on lines
+481
to
+483
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-43: the denial-message enumeration added by this diff omits two messages this same diff introduces — Concrete cost: verified at this commit — 'Daemon shell guard could not establish the execution directory of this call: ' (guard.ts:246-247, used at :2398/:2416 — the outcome is documented at :462-464 without its message) and 'Managed external tool guard cannot consult an external provider without an active prompt binding.' (:236-237) match neither documented family. Users/operators matching model-visible denials against the doc (or grepping logs for the documented prefix) cannot recognize or explain these denials. 中文说明本 diff 新增的拒绝文案枚举遗漏了同一 diff 引入的两条文案:执行目录不可确立的拒绝(guard.ts:246-247)与无 prompt 绑定时拒绝咨询外部 provider(:236-237)。代价:按文档比对模型可见拒绝(或在日志里按文档前缀检索)的用户无法识别/解释这两类拒绝。修复:补全枚举。 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| `Daemon shell guard denied a shell command…` when the command could not be | ||
| parsed, its payload could not be resolved, or an unrecognized program may run | ||
| a relocated Git command. | ||
|
|
||
| The guard is reliable against Git relocation written in the literal forms | ||
| above — the mis-targeted command this control exists for — and is | ||
| **best-effort, not a boundary**, against shell text written to defeat it: | ||
| constructions that hide the relocation from a static reader may pass, and new | ||
| ones will keep being found. Do not grant a daemon broader trust on the | ||
| strength of it. It does not interpret script files, | ||
| track environment variable values across commands, or analyze heredoc bodies | ||
| (Git-shaped text inside a heredoc can be denied even though the shell never | ||
| executes it). `/fork` and agent-backed workspace memory remember/dream remain | ||
|
Comment on lines
+494
to
+496
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The new user-facing docs describe pre-stripping heredoc behavior this same PR eliminates: the guard now strips heredoc bodies before analysis (probe: 中文说明新增的用户文档描述了同一 PR 已消除的预剥离前行为:守卫现在在分析前剥离 heredoc 正文(探针: — qwen3.8-max via Qwen Code /review (v0.21.11) |
||
| available under the built-in guard; they are only restricted while the | ||
| external provider mode below is active. An optional external tool guard | ||
| remains an additional policy and receives the same request only after the | ||
| built-in policy allows it. | ||
|
|
||
| ### Required external Tool Guard | ||
|
|
||
| This opt-in is for managed ACP deployments that need an external allow/deny | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -603,6 +603,8 @@ function sliceLineRange( | |
| */ | ||
| export interface BridgeClientSessionEntry { | ||
| sessionId: string; | ||
| workspaceCwd: string; | ||
|
wenshao marked this conversation as resolved.
|
||
| effectiveCwd: string; | ||
|
wenshao marked this conversation as resolved.
|
||
| events: EventBus; | ||
| artifacts: SessionArtifactStore; | ||
| recordingDegraded: boolean; | ||
|
|
@@ -1297,8 +1299,8 @@ export class BridgeClient implements Client { | |
| if ( | ||
| typeof sessionId !== 'string' || | ||
| sessionId.length === 0 || | ||
| typeof promptId !== 'string' || | ||
| promptId.length === 0 || | ||
| (promptId !== undefined && | ||
| (typeof promptId !== 'string' || promptId.length === 0)) || | ||
| typeof toolCallId !== 'string' || | ||
| toolCallId.length === 0 || | ||
| typeof toolName !== 'string' || | ||
|
|
@@ -1310,32 +1312,49 @@ export class BridgeClient implements Client { | |
| 'Invalid external tool guard request', | ||
| ); | ||
| } | ||
| // Context-less shell checks (subagents, cron turns, resumed background | ||
| // agents) carry no prompt binding; they are validated by session | ||
| // ownership alone. The host handler decides whether its policy can run | ||
|
wenshao marked this conversation as resolved.
|
||
| // without a live prompt. | ||
| const promptScoped = promptId !== undefined; | ||
| if (!this.ownsSession(sessionId)) { | ||
|
Comment on lines
+1319
to
1320
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R8-10 still stands: prompt-less guard requests accept any sibling sessionId owned by the same multiplexed BridgeClient channel — ownsSession is channel-level, so context-less work in session A can nominate session B and receive B's trusted effectiveCwd as the containment basis. — Failure scenario: Multiplexed channel owning A (/repo-a) and B (/repo-b): a prompt-less turn in A sends externalToolGuardPrepare with sessionId B, no promptId, command 中文说明[Critical] 无 prompt 的 guard 请求可接受同一多路复用 BridgeClient channel 下的任意兄弟 sessionId —— ownsSession 是 channel 级别的,因此 session A 中的无上下文任务可以指定 session B,并获得 B 的可信 effectiveCwd 作为 containment 基准。 — 失败场景:同一 channel 拥有 A(/repo-a) 与 B(/repo-b):A 的无 prompt 轮次发送 sessionId 为 B、无 promptId 的 externalToolGuardPrepare,命令为 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| throw RequestError.invalidParams( | ||
|
Comment on lines
1320
to
1321
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R8-10 still stands (= open threads R2-16/R8-10 re-checks): prompt-less guard requests are gated only by channel-level ownsSession (bridge.ts wires sessionIds.has over the multiplexed set; constructor default () => true), so any sibling sessionId owned by the same multiplexed channel passes and lends its trusted effectiveCwd as the containment basis; nothing binds the requester to the nominated sessionId. — Failure scenario: A subagent loop / cron turn / resumed background agent (the prompt-less paths this PR creates) supplies sibling session B's id; channel ownership passes and the guard authorizes mutations of B's checkout — defeating the PR's own purpose, since invocationCwd is validated as untrusted while the equally child-supplied sessionId is not. 中文说明R8-10 仍然成立(= 未决线程 R2-16/R8-10 复核):无 prompt 的 guard 请求仅由 channel 级 ownsSession 把关(bridge.ts 接入的是多路复用集合 sessionIds.has;构造函数默认 () => true),同一多路复用 channel 拥有的任意兄弟 sessionId 都能通过,并出借其受信任的 effectiveCwd 作为包含性基准;请求者与所提名 sessionId 之间没有任何绑定。失败场景:子 agent 循环 / cron 轮次 / 恢复的后台 agent(正是本 PR 创建的无 prompt 路径)提交兄弟会话 B 的 id:channel 所有权通过,guard 授权对 B checkout 的变更——违背本 PR 自身目的:invocationCwd 被当作不可信并校验,而同样由 child 提供的 sessionId 却没有。 — qwen3.8-max via Qwen Code /review (v0.21.11) |
||
| undefined, | ||
| 'External tool guard session is not owned by this connection', | ||
| ); | ||
| } | ||
| const entry = this.resolveEntry(sessionId); | ||
| if (!entry || !entry.promptActive || entry.activePromptId !== promptId) { | ||
| if ( | ||
| !entry || | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R2-16: Prompt-less guard requests accept any sibling sessionId owned by the same multiplexed BridgeClient channel. Failure scenario: Context-less work originating in session A supplies session B's id; channel ownership passes, and the guard receives B's trusted effectiveCwd, allowing mutation of B's checkout. Suggested fix: Bind every context-less request to host-owned originating-session state rather than trusting a child-supplied sessionId. 中文说明无 prompt 的守卫请求会接受同一复用 失败场景:来自会话 A 的无上下文工作可提交会话 B 的 ID;通道所有权检查会通过,守卫随后使用 B 受信任的 建议修复:将每个无上下文请求绑定到宿主持有的发起会话状态,不要信任子进程提供的 — gpt-5.6-sol via Qwen Code /review (v0.21.9) |
||
| (promptScoped && | ||
| (!entry.promptActive || entry.activePromptId !== promptId)) | ||
| ) { | ||
|
Comment on lines
+1327
to
+1331
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R8-10: Prompt-less guard requests accept any sibling sessionId owned by the same multiplexed BridgeClient channel — — Failure scenario: a subagent loop / cron turn / resumed background agent (the prompt-less paths this PR creates) supplies sibling session B's id; channel ownership passes and the guard authorizes mutations of B's checkout — defeating the PR's own purpose of preventing the ACP child from forging workspace context, since Suggested fix: bind context-less requests to host-owned originating-session state, or fail them closed whenever the channel multiplexes more than one session. 中文说明无 prompt 的守卫请求会接受同一复用 失败场景:子 agent 循环 / cron 回合 / 恢复的后台 agent(正是本 PR 创建的无 prompt 路径)提交兄弟会话 B 的 ID;通道所有权检查通过,守卫随后授权对 B checkout 的修改——这违背了 PR 自身"防止 ACP child 伪造 workspace context"的目标: 建议修复:将无上下文请求绑定到宿主持有的发起会话状态,或在通道复用多个会话时直接 fail closed。 — qwen3.8-max via Qwen Code /review (v0.21.10)
Comment on lines
1326
to
+1331
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-S3: On the new prompt-less path (promptScoped=false) both rejection messages reference a prompt the request does not carry: a missing entry is rejected with 'External tool guard prompt is not the active prompt' and a replaced entry with '…no longer active' — the actual cause is session-entry lifecycle. — Failure scenario: An operator diagnosing denied subagent/cron/resumed-agent shell calls is pointed at prompt binding when the cause is a missing or changed session entry. Suggested fix: When !promptScoped, use entry-scoped messages ('session has no live entry' / 'session entry changed during evaluation'). 中文说明新的无 prompt 路径(promptScoped=false)上,两条拒绝消息都在引用请求根本未携带的 prompt:条目缺失被拒为 'External tool guard prompt is not the active prompt',条目被替换被拒为 '…no longer active'——真实原因是会话条目生命周期。后果:排查子代理/cron/恢复后台代理 shell 调用被拒的运维会被引向 prompt 绑定而非会话条目状态。修复:!promptScoped 时改用会话条目语义的消息('session has no live entry' / 'session entry changed during evaluation')。 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| throw RequestError.invalidParams( | ||
| undefined, | ||
| 'External tool guard prompt is not the active prompt', | ||
| ); | ||
| } | ||
| const invocationCwd = params['invocationCwd']; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R13-S3: The BridgeClient invocationCwd pass-through — the middle hop of the only chain linking a worktree-pinned sub-agent's execution directory to the daemon guard — has zero test coverage: repo-wide grep finds invocationCwd only in the producer (acpAgent.test.ts:19014) and consumer (daemon-git-worktree-guard.test.ts:1443) tests; bridgeClient.test.ts never mentions it. — Failure scenario: Deleting bridgeClient.ts:1337-1349 (the read, non-empty-string filter, and spread) keeps the entire suite green, and isolated sub-agent shell calls then lose their containment basis and fail closed with UNVERIFIABLE_SCOPE — a silent functional regression in exactly the flow this PR adds the field for. 中文说明(见英文说明) — qwen3.8-max via Qwen Code /review (v0.21.11) |
||
| const decision: unknown = await this.externalToolGuard({ | ||
|
Comment on lines
+1337
to
1338
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The middle hop of the invocationCwd chain (child → BridgeClient → daemon handler) has no test; both endpoints are tested with mocks at the seams. — Concrete cost: the child side is pinned ('forwards the invocation directory to the daemon') and the daemon side is pinned ('reported execution directory' block), but no bridgeClient.test.ts case sends a payload containing invocationCwd through externalToolGuardPrepare. A dropped spread, typo'd key, or filtered value silently stops the daemon receiving the reported directory: every worktree-pinned sub-agent's containment basis falls back to the parent session's directory — over-denying sibling worktree mutations and mis-resolving paths — while both endpoint suites stay green. Suggested fix: add a bridgeClient.test.ts case asserting the handler receives invocationCwd verbatim, and one asserting a non-string/empty value is omitted. 中文说明invocationCwd 链路的中间跳(child → BridgeClient → daemon handler)没有测试;两端都在接缝处用 mock 测了。代价:child 侧已钉('forwards the invocation directory to the daemon'),daemon 侧已钉('reported execution directory' 块),但 bridgeClient.test.ts 没有任何用例经 externalToolGuardPrepare 发送含 invocationCwd 的 payload。spread 被删、键打错或值被过滤都会让 daemon 静默收不到上报目录:每个 worktree 钉住子代理的包含基准退回父会话目录——过度拒绝兄弟 worktree 变更、路径解析错误——而两端测试套件全绿。修复:在 bridgeClient.test.ts 增加断言 handler 原样收到 invocationCwd 的用例,以及非字符串/空值被省略的用例。 — qwen3.8-max via Qwen Code /review (v0.21.11) |
||
| sessionId: entry.sessionId, | ||
|
Comment on lines
+1337
to
1339
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-15: the 中文说明
— qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| promptId: entry.activePromptId, | ||
| ...(promptScoped ? { promptId } : {}), | ||
| toolCallId, | ||
| toolName, | ||
| arguments: args, | ||
| effectiveCwd: entry.effectiveCwd, | ||
|
wenshao marked this conversation as resolved.
|
||
| // Forwarded verbatim and explicitly untrusted: the host policy decides | ||
| // whether it can establish this scope from state it owns. | ||
|
Comment on lines
+1345
to
+1346
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R9-12: The invocationCwd relay hop (child → BridgeClient → daemon handler) is the only hop of the chain with zero test coverage — the mutation 'drop this forwarding' keeps the entire suite green. — Failure scenario: acpAgent.test.ts asserts the field outgoing and daemon-git-worktree-guard.test.ts consumes it, but the daemon tests inject requests directly, bypassing the relay. A future change dropping or sanitizing the field (its own comment calls it 'untrusted', inviting exactly that) silently removes sibling-agent isolation end-to-end: a sub-agent's 中文说明[Suggestion] invocationCwd 中继跳(child → BridgeClient → daemon handler)是整条链中唯一零测试覆盖的跳 —— “删除该转发”的变异让整个测试套件保持绿色。 — 失败场景:acpAgent.test.ts 断言字段出站,daemon-git-worktree-guard.test.ts 消费它,但 daemon 测试直接注入请求、绕过了该中继。未来删除或“消毒”该字段的改动(其注释称其 untrusted,恰恰引诱这种改动)会静默移除端到端的兄弟代理隔离:子代理的 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| ...(typeof invocationCwd === 'string' && invocationCwd.length > 0 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R2-3: A child-reported invocationCwd is accepted by containment alone instead of being bound to the invoking agent's assigned worktree. Failure scenario: An agent assigned to worktree A reports sibling worktree B as invocationCwd; the daemon accepts the session-owned path and authorizes a mutating Git command against B. Suggested fix: Derive the invocation boundary from host-owned agent/session state and reject child-reported directories that do not exactly match that assignment. 中文说明子进程上报的 失败场景:分配到 worktree A 的 agent 可将兄弟 worktree B 上报为 建议修复:从宿主持有的 agent/session 状态推导调用边界,并拒绝与该分配不完全一致的子进程上报目录。 — gpt-5.6-sol via Qwen Code /review (v0.21.9)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R2-14: Bridge tests do not verify invocationCwd reaches the daemon handler together with effectiveCwd. Concrete cost: A transport regression can drop or rename invocationCwd while lower-layer guard tests remain green and worktree-pinned agents are checked against the wrong boundary. Suggested fix: Add a bridge-level regression test asserting both cwd fields and their session binding at the handler. 中文说明Bridge 测试没有验证 具体代价:传输层回归可能丢弃或重命名 建议修复:增加 bridge 层回归测试,在 handler 处断言两个 cwd 字段及其 session 绑定。 — gpt-5.6-sol via Qwen Code /review (v0.21.9) |
||
| ? { invocationCwd } | ||
| : {}), | ||
|
Comment on lines
+1347
to
+1349
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-16: the bridge client's verbatim 中文说明bridge 客户端对 — qwen3.8-max via Qwen Code /review (v0.21.8)
Comment on lines
+1347
to
+1349
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Suggested fix: add a test mirroring 'ignores a forged effective directory in the child payload': send 中文说明
— qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| }); | ||
| const currentEntry = this.resolveEntry(sessionId); | ||
| if ( | ||
| !this.ownsSession(sessionId) || | ||
| currentEntry !== entry || | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R13-11: Prompt-less guard requests are not serialized against changeSessionCwd (probe-verified with a fix-and-flip): effectiveCwd — the containment basis forwarded at line 1344 — is mutated IN PLACE by changeSessionCwd (bridge.ts:7693) during the handler await; the post-call recheck is reference-identity only and prompt-less requests skip the prompt-state arm (changeSessionCwd throws CdWhilePromptActiveError only while promptActive). — Failure scenario: A context-less shell check (subagent/cron/background path — the exact flows this PR's prompt-less support creates) is in flight while a /cd completes on the same session: the decision validated against basis B0 is returned after the basis became B1, allowing e.g. 中文说明(见英文说明) — qwen3.8-max via Qwen Code /review (v0.21.11) |
||
| !currentEntry.promptActive || | ||
| currentEntry.activePromptId !== promptId | ||
| (promptScoped && | ||
| (!currentEntry.promptActive || | ||
| currentEntry.activePromptId !== promptId)) | ||
| ) { | ||
| throw RequestError.invalidParams( | ||
| undefined, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,18 @@ | |
| export const PRIVATE_EXTERNAL_TOOL_GUARD_ENV = | ||
| 'QWEN_CODE_PRIVATE_EXTERNAL_TOOL_GUARD'; | ||
|
|
||
| /** | ||
| * Private, non-secret marker passed from `qwen serve` to its ACP child only | ||
| * when a real external tool guard provider is attached. Without it the child | ||
| * still installs the managed guard plumbing (the daemon's built-in policy | ||
| * needs it), but resolves every non-shell tool locally and keeps `/fork` and | ||
| * agent-backed workspace memory available: those features are only disabled | ||
| * for the external provider's v1 contract, which cannot observe hidden-agent | ||
| * execution. | ||
| */ | ||
| export const PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV = | ||
| 'QWEN_CODE_PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER'; | ||
|
|
||
| /** | ||
| * ACP initialize-response metadata proving that the child consumed the | ||
| * private activation marker and installed the required executor callback. | ||
|
|
@@ -26,6 +38,26 @@ export const EXTERNAL_TOOL_GUARD_READY_META_KEY = | |
| */ | ||
| export const EXTERNAL_TOOL_GUARD_REQUIRED_VALUE = 'required-v1'; | ||
|
|
||
| /** | ||
| * The provider-attached marker value `qwen serve` passes to the child when a | ||
| * real external tool guard provider is configured. | ||
| */ | ||
| export const EXTERNAL_TOOL_GUARD_PROVIDER_ATTACHED_VALUE = 'attached-v1'; | ||
|
|
||
| /** | ||
| * Tools whose arguments carry a shell command line the host runs on the | ||
| * session's behalf. The daemon's built-in policy inspects exactly these, and | ||
| * the ACP child resolves every other tool locally when no external provider | ||
| * is attached. Pinned to `ToolNames.SHELL`/`ToolNames.MONITOR` in | ||
| * `@qwen-code/qwen-code-core`, which this package deliberately does not | ||
| * depend on; `daemon-git-worktree-guard.test.ts` asserts the values still | ||
|
Comment on lines
+51
to
+53
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-26: the added JSDoc claims acp-bridge 'deliberately does not depend on' 中文说明新增 JSDoc 声称 acp-bridge「有意不依赖」 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| * match so a rename cannot silently unhook a tool from the guard. | ||
| */ | ||
| export const SHELL_EXECUTING_TOOL_NAMES: ReadonlySet<string> = new Set([ | ||
| 'monitor', | ||
| 'run_shell_command', | ||
| ]); | ||
|
|
||
| /** Daemon-local bearer token for the loopback external Tool Guard provider. */ | ||
| export const EXTERNAL_TOOL_GUARD_TOKEN_ENV = | ||
| 'QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN'; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R6-42: the doc claims the session's effective working directory is 'never accepted from the ACP child', but this PR makes
entry.effectiveCwdthe guard's containment basis and that field is set verbatim from the child'ssessionCdreply (bridge.ts:7564, shape-checked only) — Concrete cost: verified at this commit —changeSessionCwdrecords the child-reportednewCwdwithout re-validating it against the requested path or allowed roots; the new relay hands exactly that field to the daemon guard as the trusted basis (bridgeClient.ts:1332). A lying/compromised child permanently moves the session's containment boundary on any/cdflow. The child-side handler does validate — but the doc's trust-lineage claim is false as written for the very field it names.中文说明
文档声称会话的有效工作目录「从不来自 ACP 子进程」,但本 PR 把
entry.effectiveCwd作为 guard 的包含基准,而该字段正是逐字取自子进程sessionCd回复(bridge.ts:7564,仅做类型检查)。代价:daemon 端不校验newCwd是否匹配请求路径/允许根;说谎或被攻陷的子进程可在任一/cd流程中永久移动会话边界。修复:daemon 端对newCwd做再校验,或更正文档表述。— qwen3.8-max via Qwen Code /review (v0.21.9)