feat(serve): add a required external tool guard provider - #8125
Conversation
Managed ACP external Tool Guard E2E reportCommit: Environment: macOS, Node.js 24, built Command:
All five scenarios passed the following assertions:
Additional final gates:
The local E2E plan and harness are intentionally kept under the repository's ignored |
🩺 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 |
|
CI follow-up for commit 4628195 The Ubuntu Test failure was isolated to the serve pre-listen bundle-closure gate, not to Guard behavior. The new serve entry imported Guard constants from acp-bridge/status; that module has runtime Core dependencies, so the static import expanded the startup closure. This follow-up moves the three protocol/env constants into a zero-runtime-dependency acp-bridge/externalToolGuard subpath and updates imports only. It does not change the protocol, opt-in default, provider behavior, or user-facing surface. Local evidence at this commit:
The pushed commit now triggers the upstream matrix again. |
|
Follow-up for the Ubuntu Test failure at The full suite completed with 15,879 passing tests and two failures. Both failures were exhaustive test declarations introduced by the guard startup surface, not runtime behavior regressions:
Fixed in Local verification:
A new full GitHub Actions run is now pending for the exact head. |
e70e8b4 to
ac9e93b
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）为单个提交。 |
|
Rebased after #8032 merged.
Rebase conflict audit:
Exact-head local gates:
The PR remains Draft until the new exact-head GitHub checks complete. |
|
Thanks for the PR — this is a carefully scoped piece of work, and it shows.
Moving on to code review. 🔍 中文说明感谢贡献——这个 PR 的范围划分非常严谨。
进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewMy independent baseline for this problem — opt-in
I walked every path that can return One non-blocking observation: the control-character/bounded-reason validation predicate is written out inline three times (provider client, The Stage 1e high-risk area ( Runtime flowsequenceDiagram
participant P1 as Host operator
participant P2 as qwen serve daemon
participant P3 as Loopback guard provider
participant P4 as ACP bridge BridgeClient
participant P5 as Private ACP child
participant P6 as Tool executor
P1->>P2: start with mode required
P2->>P3: POST v1 handshake with Bearer token
P3-->>P2: version, nonce, prepare capability
P2->>P5: spawn with private required marker
P5-->>P4: initialize ack required-v1
P1->>P2: prompt
P5->>P5: permission and PreToolUse gates
P5->>P4: ext prepare - sessionId, promptId, toolCallId, name, args
P4->>P4: verify owned session and active prompt
P4->>P3: POST v1 prepare, exactly once, no retry
P3-->>P4: allow or deny
P4->>P4: re-verify prompt still active
P4-->>P5: decision
alt allow
P5->>P6: execute final invocation once
P5-->>P2: existing terminal tool_call_update event
else deny, timeout, cancel, or malformed
P5-->>P2: failed or cancelled terminal event, executor never ran
end
Files changed (29 of 43 shown)
TestingUnattended CI run — I did not build or execute any PR code (per the gate rules); the evidence below is the PR's own CI on the reviewed commit, fetched via the API. All three What CI does not settle: the built-daemon required-mode E2E described in the PR body (allow executes exactly once; deny/timeout/cancel execute zero times; no retry; credential isolation across child, relaunch, channel-worker, hook, and executor environments). Those numbers are the author's self-reported results on macOS, not independently re-run here. Sandboxed verification would settle this: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Real-scenario (tmux) testing: N/A on this path — this is an unattended CI run, and live-behaviour signal comes from the sandboxed lane above instead. 中文说明代码审查：我独立构想的基线方案（默认关闭的 serve 开关、仅回环端点的认证握手、在 8032 执行器边界做一次有界无重试的 prepare 决策、凭据留在 daemon、子进程确认、条件能力通告）与本 PR 基本一致；PR 超出基线的部分也都是有依据的——BridgeClient 在调用 Provider 前后双重校验会话归属与活跃 Prompt（允许决策返回时若 Prompt 已停止则作废）、私有激活标记可穿越 relaunch 但在任何工具/hook/MCP 初始化前被消费删除、loadCliConfig 的 hostPolicy 参数刻意不可从 argv/settings/env 驱动、Provider 客户端用 agent: false 的直连请求避免被全局 dispatcher/代理劫持、serve fast path 遇到新 flag 直接回退完整解析器（已核对基线代码，不存在 fail-open 路径）、受 Guard 的 MCP 调用在传输歧义错误后不再重连重试。我逐条走查了所有能返回 allow 的路径，未发现关键正确性或安全问题。 唯一非阻塞观察：控制字符/有界拒绝理由的校验谓词在三处内联重复（Provider 客户端、BridgeClient、ACP 子进程 Guard），allow/deny 形状检查同理；建议后续抽到 acp-bridge externalToolGuard 模块中共享，避免三处漂移。不构成合并阻塞。 Stage 1e 高风险区（acp-integration）已重点查看：子进程 Guard 在无调用上下文时 fail-closed、嵌套/委托入口在联系 Provider 前即被拒绝、Session.ts 只是两行上下文转发且复用已有导入。无顾虑。 测试：无人值守 CI 运行——未构建或执行任何 PR 代码。本 commit 上三个 pull_request 事件的工作流全部绿灯（Qwen Code CI、Serve A/B、SDK Java）。Serve A/B 是默认关闭路径回归的有效信号（base 与本 PR 对照构建并 diff 固定端点集）。CI 无法覆盖的是 PR 正文所述的真实 daemon required 模式 E2E（allow 恰好执行一次、deny/timeout/cancel 零执行、无重试、凭据隔离）——那是作者的自报结果（macOS），未在此独立复跑。沙箱验证可补齐：@qwen-code /verify（fork PR 为赞助运行，由 maintainer 评论触发，带预执行风险筛查与完整工作区清除；报告仍应以审视 fork CI 日志的怀疑态度阅读）。仍在进行的 triage/review-pr 检查属于 pull_request_target 机器人编排，不是 PR CI。实时场景（tmux）测试：CI 路径不适用。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review across every stage, but the core-module size escalation is pure policy: a fork Stepping back honestly: the PR's approach matches the independent proposal I wrote before reading the diff, and where it diverges it diverges upward — the post-decision re-validation in the bridge, the relaunch-surviving private marker, the hostPolicy seam that config and env cannot reach, the MCP no-replay rule. I looked for the materially simpler path and did not find one; the milestone split already shipped the smaller half as #8032, and what remains here is the coherent rest. Every change in the diff serves the stated goal. Six months from now, the design doc plus 1300 lines of targeted tests would make this a thank-the-author file set, with the one drift risk being the thrice-inlined validation predicate noted in Stage 2. What keeps this at 3/5 rather than higher, in order of weight:
⏸️ Deferring to maintainers — @wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC (owners of the touched 中文说明置信度：3/5——各阶段审查都干净，但核心模块规模升级是纯策略性的：一个约 900 行生产代码、跨三个包的 fork 诚实复盘：PR 方案与我读 diff 前独立写出的提案一致，且所有偏离都是向上的——bridge 的决策后二次校验、可穿越 relaunch 的私有标记、config/env 不可触达的 hostPolicy 接缝、MCP 不重放规则。我找过更简路径，没找到：里程碑拆分已把较小的一半作为 8032 合入，这里是连贯的剩余部分。diff 中每处改动都服务于既定目标，无夹带。六个月后，设计文档加 1300 行针对性测试会让人感谢作者；唯一的漂移风险是 Stage 2 提到的三处内联校验谓词。 停在 3/5 的原因（按权重）：其一，策略上限——Stage 0 已因规模升级给 maintainer 知悉，批准权不在我；其二，方向已跟踪但未收尾——umbrella 提案 8102 仍 open 且带 need-discussion，Milestone 1 合入说明有真实兴趣，但项目是否现在要 Milestone 2（一个自带 wire protocol 的可选企业强制面）应由 maintainer 确认；其三，核心行为声明目前依赖作者的 E2E——CI 全绿且 Serve A/B 钉住了默认关闭的等价性，但 required 模式端到端 fail-closed 行为正是已点名的 @qwen-code /verify 赞助运行可以补齐的。 已转交 maintainer（所触及 /packages/core/ 路径的 owner）：需要人类判断 8102 方向是否足以落地 Milestone 2，以及 daemon/ACP 安全面本身的评审。diff 中未发现阻塞问题——这是升级，不是拒绝。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@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，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Autofix review round — no actionable feedbackHead reviewed: Feedback triageThis round surfaced no reviews, no inline comments, and no failed checks. The five issue-level comments since the last evaluation are all informational and require no code change:
Check stateAll completed checks are green (22 success, 23 skipped). The single in-progress item is the Pending items for maintainers (not autofix work)
No commit was made and no verification commands were run, since nothing was changed. 中文说明🤖 Autofix 审查轮次 — 无可处理反馈审查的 head： 反馈分诊本轮没有浮现任何 review、inline comment 或失败检查。自上次评估以来的 5 条 issue 级评论均为信息性内容，无需代码改动：
检查状态所有已完成的检查均为绿色（22 成功、23 跳过）。唯一进行中的条目是 待 maintainer 处理的事项（不属于 autofix 工作范围）
由于未做任何改动，本轮未产生提交，也未运行验证命令。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs. |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
…wenLM#8125) The reconnect-on-timeout test still built its mock tools without server trust or tool annotations, which the safe replay change now requires before automatically replaying a connection-loss failure. Update the fixtures the same way the surrounding reconnect tests were updated, keeping the test's original assertion that a timeout on a known disconnected server goes through the reconnect path. Mirrors the same alignment already landed on main.
…m/chiga0/qwen-code into feat/external-tool-guard-provider
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8125This round had no new inline review comments or reviews to triage. The actionable item was the persisting red Root cause of the failing Test checkReproduced the CI
The branch already contains Change madePorted the #8478 fixture alignment onto this branch (commit Dispositions of other feedback
VerificationCommands actually run and their results (clean CI-like env =
Unavailable here: the exact GitHub runner's 中文说明Autofix 本轮总结 — PR #8125本轮没有新的行内评论或 review 需要处理。可操作的问题是持续失败的 Test 检查失败的根因在类 CI 的干净环境（
分支通过上次合并 main 已包含 所做的修改将 #8478 的夹具对齐移植到本分支（提交 其他反馈的处理
验证实际执行的命令及结果（干净类 CI 环境 =
本环境不可用：GitHub runner 上 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/模型 |
QwenLM#8125) This PR added `@qwen-code/acp-bridge/externalToolGuard` imports to cli serve/acp modules but not the vitest source alias every other acp-bridge subpath carries. Without it, any vitest run whose acp-bridge dist is stale or absent fails to resolve the import and the five serve test files die at transform time. Add the alias following the documented convention in the config so tests read the live source.
…m/chiga0/qwen-code into feat/external-tool-guard-provider
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8125Feedback handledThe only feedback item this round was a failed check: Test (ubuntu-latest, Node 22.x). There were no review bodies, inline comments, or issue-level comments to triage, and no base-conflict to resolve ( Root-cause defect found and fixedThis PR introduced Other failure candidates investigated and attributed
With logs unavailable, the exact failing CI step/test of the red run cannot be named from here; every locally reproducible step of the job is green on this branch after the fix, and the workflow's independent CI remains the final gate. Changes
VerificationCommands actually run this round and their results:
中文说明Autofix 审查轮次 — PR #8125处理的反馈本轮唯一的反馈项是一个失败的检查：Test (ubuntu-latest, Node 22.x)。没有需要分类的审查正文、行内评论或 issue 级评论，也没有需要解决的基线冲突（ 发现并修复的根因缺陷本 PR 在六个 cli 模块中引入了 其他被调查并已归因的失败候选项
由于无法获得日志，无法从此处指名红色运行中确切失败的 CI 步骤/测试；修复后该作业所有可本地复现的步骤在本分支上均为绿色，工作流的独立 CI 仍是最终验证门禁。 变更
验证本轮实际执行的命令及结果：
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 review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — security contract holds: fail-closed is complete across all paths (timeout/abort/transport/malformed/stale identity/unowned session/child-without-ack), loopback strictly enforced (no DNS/proxy/redirect), token hygiene solid (env-only, 8-layer scrub, never forwarded), identity runtime-owned (compromised child cannot forge), nested-agent bypass surface closed, MCP no-replay verified.
Non-blocking suggestions:
- (P2) 1 MiB payload cliff: large write_file/edit calls are permanently denied with a generic error. Consider documenting the operational cliff or providing a typed size-limit error.
- (P2) Missing e2e test for the full daemon → ACP child → ack → provider chain (design doc verification plan item 13).
- (P3) Add length cap on toolCallId/toolName; add non-200/HTTPS path tests; consider named options object instead of 17th positional param.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Reviewed the latest feedback — no code changes needed this round. Feedback triagedThe only new feedback since the last evaluation is the automated comment "Qwen Code review timed out after 21600 seconds (of the 360-minute budget)" and the corresponding failed Why this is not a code defectEvery substantive CI check on this head is green. Unit tests ( No test in this PR hangs or runs slow. I ran every touched test file locally under hard timeouts:
One local-only failure appeared on the first run — The timeout pattern points at review workload vs. budget, not at this branch. This is the second consecutive budget exhaustion (240 min on 2026-08-03, 360 min now), and the run that timed out also sat ~4 hours in the runner queue before starting (push 23:53 UTC → job start 03:57 UTC). Meanwhile the round-1 review of this same ~2800-line diff completed and posted its findings in roughly 70 minutes on 2026-08-03 — so the review can finish; the variance matches the load-dependent behavior documented in the review workflow itself. The budget repository variables are already at their maximum (the bot comment confirms "already used the maximum 360 minute timeout"), so there is no further headroom to grant from code. I also re-read the full diff for anything that could stall a reviewer's probes (spawned servers that never exit, unbounded timers, watch-mode traps) and found nothing: the new HTTP guard client bounds request/response sizes, its timeout timer is Recommended next step (maintainer's call)The remaining remedies are outside code scope, so I have not acted on them:
中文说明🤖 已审阅最新反馈——本轮无需修改代码。 反馈分类自上次评估以来，唯一的新反馈是自动评论 "Qwen Code review timed out after 21600 seconds (of the 360-minute budget)"（Qwen Code 评审在 360 分钟预算中超时，耗时 21600 秒）以及对应的 为什么这不是代码缺陷本次提交头上所有实质性 CI 检查均为绿色。 单元测试（ 本 PR 中没有任何测试挂起或运行缓慢。 我在硬性超时限制下本地运行了所有被改动的测试文件：
首次运行时出现了一个仅本地存在的失败—— 超时模式指向评审工作量与预算的矛盾，而非本分支的问题。 这已是连续第二次预算耗尽（2026-08-03 为 240 分钟，本次为 360 分钟），且本次超时的运行在开始前还在运行器队列中排队约 4 小时（推送时间 23:53 UTC → 任务启动 03:57 UTC）。与此同时，针对同一份约 2800 行 diff 的第 1 轮评审在 2026-08-03 约 70 分钟内完成并发布了所有发现——说明评审能够完成；这种波动与评审工作流自身已记录的负载相关行为一致。预算相关的仓库变量已处于最大值（机器人评论确认 "already used the maximum 360 minute timeout"，即已使用 360 分钟的最大超时上限），因此代码层面已无法再提供任何额外余量。 我还重新通读了完整 diff，排查任何可能卡住评审探针的内容（永不退出的被启动服务器、无界定时器、watch 模式陷阱等），未发现任何问题：新的 HTTP 守卫客户端限制了请求/响应大小，其超时定时器已 建议的下一步（由维护者决定）其余补救手段超出代码范围，因此我未采取行动：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Maintainer verification — real local stack ✅I rebuilt this branch locally and ran the reviewer test plan against a real built daemon, a real private ACP child, a loopback v1 provider, a mock OpenAI model, and a shell tool with an observable one-shot side effect (append a line to a file). All 16 scenarios behave as the PR claims. Verified head:
Runtime behaviour
No scenario ever issued a second Fail-closed startup / session admission — all refused with exit 1
Correlation and credential isolationThe Credential isolation, checked from two independent angles:
Cancel is genuinely non-blocking: the provider was configured to hold the decision for 8000 ms, and the prompt reached its Quality gates on this head
Notes for reviewers (non-blocking)
Nothing here blocks merge from my side. 中文版本维护者验证 —— 本地真实环境 ✅我在本地重新构建了这个分支，并用真实构建的 daemon、真实的私有 ACP 子进程、回环 v1 Provider、mock OpenAI 模型，以及一个带可观察单次副作用的 shell 工具（向文件追加一行）跑完了 Reviewer 测试计划。16 个场景全部符合 PR 的描述。 验证的 head： 运行时行为
没有任何场景发出第二次 失败关闭的启动 / Session 准入 —— 全部拒绝并 exit 1
关联字段与凭据隔离allow 路径上 Provider 实际收到的 凭据隔离用两个互相独立的来源验证：
取消确实不阻塞：Provider 被配置为把决策挂起 8000 ms，而 Prompt 在被接纳后 519 ms 就到达 该 head 上的质量门禁
给 Reviewer 的说明（不阻塞合并）
从我这边看，没有阻塞合并的问题。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the exact head 9ab84a0a82e5140d075364278235026792f404c2. I found one P2 compatibility issue in the external provider bearer-token validation. The fail-closed architecture otherwise looks sound, and I found no authentication bypass or additional blocking correctness issue. I am leaving this as a comment review rather than approving until the inline issue is addressed and the required CI is green.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review Overview
This is a COMMENT review because the PR author is chiga0 (myself). I performed a blind independent review before reading existing reviews.
Verdict: The fail-closed security contract is structurally sound — loopback-only endpoint, authenticated handshake, single prepare request, no retries, token scrubbing, runtime-owned identity, and nested-agent rejection are all in place. I found no authentication bypass or blocking correctness issue. However, I identified one P2 compatibility issue in the provider HTTP client and a few maintainability/robustness gaps.
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
Non-ASCII bearer token causes ERR_INVALID_CHAR at startup |
doudouOUC (P2) | Confirmed — constructor accepts 密钥, initialize() throws before handshake. Validate ASCII/b64token grammar. |
17th positional param in loadCliConfig |
yiliang114 (P3) | Confirmed — at packages/cli/src/config/config.ts, adding another positional argument increases fragility. |
Missing timer.unref() regression test |
doudouOUC | Confirmed observation, no additional comment. |
| Method-routing guard lacks negative-path test | doudouOUC | Confirmed observation, no additional comment. |
| Denial-with-reason path untested | doudouOUC | Confirmed observation, no additional comment. |
| Duplicated denial-reason validation across packages | doudouOUC | Confirmed — suggest centralizing in externalToolGuard.ts. |
Raw 'required-v1' literal in 12+ locations |
doudouOUC | Confirmed — centralize constant. |
| Ambiguous ack-rejection error message | doudouOUC | Confirmed — include received value for diagnostics. |
| Stale-prompt guard happy path untested | doudouOUC / bot rebuttal | Acknowledge the rebuttal: existing tests do exercise this path; observation may be probe-specific. |
| 1 MiB payload cliff | yiliang114 (P2) | Confirmed — large writes are silently denied; consider typed size-limit error. |
| Missing full daemon→ACP→provider e2e | yiliang114 (P2) | Confirmed. |
toolCallId/toolName length caps |
yiliang114 (P3) | Confirmed as defensive hardening. |
Unique Findings
| ID | Severity | File:Line | Summary |
|---|---|---|---|
| U1 | P2 | packages/cli/src/serve/external-tool-guard-provider.ts:~258 |
Provider HTTP request does not send Accept-Encoding: identity. If the guard endpoint returns gzip/deflate, the client concatenates raw compressed bytes and JSON.parse fails, causing every tool invocation to be denied even though the provider allowed it. Add accept-encoding: 'identity' to the request headers or decompress the response. |
| U2 | P3 | packages/cli/src/serve/run-qwen-serve.ts:~1197 |
Capability external_tool_guard is advertised from config (mode === 'required') rather than from an explicit handshake succeeded flag. If initialize() is skipped or fails silently, the capability could still be advertised. Bind advertisement to the provider's handshake success. |
| U3 | P3 | packages/cli/src/serve/run-qwen-serve.ts:~2757 |
childEnvOverrides does not explicitly set [EXTERNAL_TOOL_GUARD_TOKEN_ENV]: undefined. While other layers scrub the token, making the override explicit here documents the invariant and hardens against future refactor drift. |
| U4 | P3 | packages/cli/src/acp-integration/acpAgent.ts:~2747 |
Nested-agent rejection uses a hardcoded list (AGENT, WORKFLOW, CREATE_SUB_SESSION, SEND_MESSAGE). Future tools that spawn an independent AgentCore/Session could bypass the list. Consider a capability/trait-based mechanism or a central registry for "spawns independent session" tools. |
| U5 | P3 | packages/cli/src/acp-integration/acpAgent.ts:~9569 |
/fork rejection happens after sessionId/directive parameter validation. Move the managed-guard rejection to the top of the sessionForkAgent case so the guard decision runs before parsing unrelated parameters. |
Additional Audit Coverage
Areas I independently checked that go beyond existing findings:
- HTTP transport compatibility: verified request headers and response parsing path; identified the missing
Accept-Encodinghandling. - Capability advertisement semantics: checked that the advertised feature derives from config rather than handshake state.
- Child environment overrides: traced token-scrubbing layers and found one missing explicit override in the serve bootstrap.
- Nested-agent bypass surface: reviewed the denylist approach and noted fragility against future tool additions.
- Control-method ordering: verified
/forkrejects managed guard, but only after parameter validation.
A token outside the ASCII range passed construction but made the handshake throw ERR_INVALID_CHAR when interpolated into the Authorization header, blocking qwen serve startup in required mode with an unexplained error. Enforce printable ASCII (0x21-0x7E) at validation time so the configuration fails fast with a clear message.
|
Heads-up for maintainers: the squash merge appears to have raced with a fix push and landed on the pre-fix head. Timeline (UTC):
So two reviewed-and-confirmed fixes did not reach
Both commits are intact on |
|
Released in v0.21.6. |
6 participants
What this PR does
Adds an opt-in external pre-execution policy provider for managed
qwen serveACP deployments. The new process-start mode isoffby default. When explicitly set torequired, the daemon completes an authenticated, versioned handshake with an origin-only loopback HTTP(S) provider and requires one bounded, no-retry allow/deny decision immediately before every supported top-level tool executor.Required mode keeps provider credentials in the daemon, validates channel-owned Session and active Prompt identity before forwarding a decision request, requires each ACP child to attest that the executor callback is installed before Session creation, and fails closed on cancellation, timeout, transport failure, malformed responses, stale identity, or explicit denial. Unsupported nested/delegated agent entry points are rejected, and guarded MCP invocations are not reconnected or replayed after an ambiguous transport failure.
The provider request carries runtime-owned
sessionIdandpromptId, a runtime-acceptedtoolCallIdcorrelation label, the canonical tool name, and final arguments. Existing tool lifecycle events remain the result-observation channel; this PR does not add a result callback, audit store, or product-specific Task API.This is Milestone 2 of #8102 and now builds on the executor-boundary host Guard merged in #8032. The branch was rebased after #8032 merged, so this PR contains only its three provider/managed-ACP commits.
Why it's needed
Managed and unattended Agent-to-Agent deployments need a host-owned decision at the last safe point before a tool side effect. Existing permission rules and hooks remain useful, but they do not provide a mandatory external, machine-verifiable, fail-closed decision over the final invocation identity and arguments. This PR supplies that narrow enforcement path without changing standalone CLI, ordinary ACP, or default daemon behavior.
Reviewer Test Plan
How to verify
qwen servewithout the new mode and confirm that no provider request occurs and/capabilitiesomitsexternal_tool_guard.qwen servewith required mode and its token. Confirm startup fails for invalid configuration, a failed/incompatible handshake, or an ACP child that does not acknowledge enforcement.Exact rebased head
ac9e93bb7local evidence on upstreammain@2d2bdab2b:npm run check:serve-fast-path-bundle: passed.off,allow,deny,timeout, andcancelpassed. Off made zero provider requests; allow executed exactly once; deny/timeout/cancel executed zero times; correlation, final arguments, terminal outcomes, no-retry behavior, and credential isolation all passed.Evidence (Before & After)
N/A — non-UI runtime and protocol change.
Tested on
Environment (optional)
macOS with Node.js 24. The required-mode E2E used a real built daemon, private ACP child, loopback fake v1 provider, fake model, and an observable executor effect counter.
Risk & Scope
--external-tool-guard-mode=requiredis explicitly selected. Managed deployments adopting required mode must provide the loopback v1 provider and should disable or independently govern hooks and slash commands when they require an all-effects boundary.Linked Issues
References #8102 and builds on merged #8032.
中文说明
本 PR 做了什么
为受管
qwen serveACP 部署增加一个显式开启的外部执行前策略 Provider。新的进程启动模式默认是off。只有显式设置为required时，daemon 才会与仅允许回环地址的 HTTP(S) Provider 完成带认证、带版本的握手，并要求每个受支持的顶层工具在真正进入执行器之前进行一次有界、无重试的允许/拒绝决策。required模式把 Provider 凭据保留在 daemon 内；转发决策请求前校验通道拥有的 Session 和当前活跃 Prompt；在创建 Session 前要求每个 ACP 子进程证明执行器回调已经安装；遇到取消、超时、传输失败、非法响应、过期身份或明确拒绝时全部失败关闭。不支持的嵌套/委派 Agent 入口会被拒绝，受 Guard 保护的 MCP 调用遇到不确定传输失败后不会自动重连或重放。Provider 请求携带运行时拥有的
sessionId和promptId、由运行时接受的toolCallId关联标签、规范化工具名和最终参数。结果观察继续使用已有工具生命周期事件；本 PR 不增加结果回调、审计存储或产品特定的 Task API。这是 #8102 的里程碑 2，现在基于已由 #8032 合入的执行器边界 Host Guard。#8032 合入后已重新 rebase，因此本 PR 只包含自身的 3 个 Provider/受管 ACP 提交。
为什么需要
受管和无人值守的 Agent-to-Agent 部署需要在工具副作用发生前的最后安全点，由宿主做出决策。现有权限规则和 hooks 仍然有价值，但无法针对最终调用身份和参数提供强制、外部、机器可验证且失败关闭的决策。本 PR 提供这一窄范围执行路径，同时不改变独立 CLI、普通 ACP 和默认 daemon 行为。
Reviewer 测试计划
如何验证
qwen serve，确认没有 Provider 请求，且/capabilities不包含external_tool_guard。qwen serve。确认非法配置、失败/不兼容握手、或 ACP 子进程未确认 Guard 时启动/Session 创建失败。在上游
main@2d2bdab2b上对 rebase 后精确 headac9e93bb7的本地证据：npm run check:serve-fast-path-bundle：通过。off、allow、deny、timeout、cancel全部通过。Off 发起 0 次 Provider 请求；allow 只执行 1 次；deny/timeout/cancel 执行 0 次；关联字段、最终参数、终态、无重试和凭据隔离断言全部通过。前后证据
N/A —— 非 UI 的运行时与协议变更。
测试平台
环境
macOS、Node.js 24。required 模式 E2E 使用真实构建后的 daemon、私有 ACP 子进程、回环 fake v1 Provider、fake model 和可观察的执行器副作用计数器。
风险与范围
--external-tool-guard-mode=required才会启用。采用 required 模式的受管部署必须提供回环 v1 Provider；若要求覆盖所有副作用，还应禁用或独立治理 hooks 和 slash commands。关联 Issue
关联 #8102，并基于已合入的 #8032。