feat(core): show what a workflow will do, and scope the grant that approves it - #9837
Conversation
…proves it A user approving a workflow was shown `Confirm WorkflowTool` over `Run a workflow script (4127 chars)` — a character count standing in for arbitrary model-authored JavaScript that can fan out to the per-run agent cap, provision git worktrees and spend an uncapped token budget. The asymmetry was visible inside one run: the subagent approvals that same workflow bubbles up each got a full dialog. Override getConfirmationDetails to show what the script says it will do, read from `export const meta`: the workflow's name and description, its declared phases, the resolved args, and a bounded excerpt of the source. Meta is obtained through `extractAndStripMeta`, which parses rather than evaluates, so nothing model-authored runs before the user has approved anything. It throws on a malformed literal, so the call is wrapped: a script with a broken meta block stays approvable-or-rejectable rather than taking the dialog down with it. Everything displayed goes through `stripAnsiAndControl` first. The screen ships with the preview rather than after it, because until now nothing was displayed and so nothing could be spoofed — a preview without it is what would open the hole. Single-line fields are flattened, which is what we want for a `meta.name` spanning three lines. The script excerpt is sanitized per line instead: `\n` is a C0 control character, so the naive call would collapse the script into one unreadable line. Scope the grant on the same object. An inline `script` is fresh source every time, so it can never be pre-approved: `hideAlwaysAllow` removes the option, and an empty `permissionRules` stops `injectPermissionRulesIfMissing` from supplying the bare tool name, which `buildPermissionRules` documents as matching every invocation of the tool. A `scriptPath` names a file the user chose, so it stays pre-approvable but scoped to that path via a `key:value` param matcher. The rule is built with the same helpers the matcher uses, and the test asserts it behaviourally -- a rule that reads plausibly but never matches would make "always allow" silently do nothing. Finally, move the token-cost warning ahead of the spend it warns about. It previously appeared only on the success path, i.e. after the run, and never at all when the run failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MDuRWMhKbqEYZXbbLhn3en
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: real, and verifiable in code rather than reported. Without this change the base Direction: aligned. Approval disclosure is core to qwen-code's safety model, and the upstream CHANGELOG shows sustained investment in exactly this area — including "Fixed permission prompts so commands padded with tabs or invisible Unicode can no longer hide part of the command from the approval dialog", the same threat family (attacker-shaped text displayed on the approval path) that this PR's sanitization addresses. Size: touches core paths ( Approach: scope is right. Disclosure and grant scoping are two halves of one consent decision, and shipping them together is correct — a rich preview without the grant fix would still leave the blanket "always allow" hole open. The PR reuses existing load-bearing primitives ( Risk: no high-risk path matches. No elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:真实存在,且可直接在代码中验证。改动之前,基类对话框渲染的就是 方向:对齐。审批披露是 qwen-code 安全模型的核心,上游 CHANGELOG 也显示了对这一领域的持续投入——包括"修复权限提示,使带制表符或不可见 Unicode 的命令无法再在审批对话框中隐藏部分命令",与本 PR 净化逻辑应对的是同一威胁家族(审批路径上的攻击者可控展示文本)。 规模:触及核心路径( 方案:范围合理。披露与授权收窄是同一个同意决定的两半,一起交付是正确的——只加预览不修授权,"always allow" 的口子仍然开着。PR 复用了既有的承重原语(#9325 的 风险:未命中高风险路径。无升级风险信号。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this problem — parse
No critical findings. One non-blocking nit: the persisted rule inherits glob semantics from the path — a Not verified here: the author's claim that reverting Test evidence (PR's own CI, via API)Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The unit suite (ubuntu) is still running on the reviewed commit; the macOS/Windows matrix and integration jobs show skipped at this point. The Sandboxed verification would settle what static review cannot: 中文说明代码审查我对该问题的独立方案——解析(而非求值)
无阻塞性发现。一个非阻塞的小问题:持久化规则会继承路径的 glob 语义——若 此处未验证:作者关于"回滚 测试证据(PR 自身 CI,经 API 获取)单元测试套件(ubuntu)在被审提交上仍在运行;macOS/Windows 矩阵与集成测试此时显示为 skipped。CI 落定后, 沙盒验证可以解决静态审查看不到的部分: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, focused change with every load-bearing claim verified against the base code; the one open item is the unit suite still running on the reviewed commit (plus the non-blocking wildcard-in-path nit from Stage 2). Stepping back: this fixes a consent asymmetry that is visible in the code itself — the user was asked to authorize arbitrary model-authored JavaScript with a character count as the only disclosure, and one "always allow" click extended that consent to every future script. The implementation matches my independent proposal, reuses the repo's existing primitives instead of adding parallel machinery, and the diff is the minimal set the goal needs — one tool file, one test file, nothing else. The judgment calls are sound and stated out loud: inline scripts lose "always allow" entirely (there is no stable identity to scope fresh model-authored source against), malformed meta degrades instead of taking the dialog down, and the cost warning moves ahead of the spend. If I were maintaining this in six months, the degrade-on-malformed test and the behavioural rule-match test are exactly the anchors I would want. CI is still running on the reviewed commit ( 中文说明置信度:4/5 —— 干净、聚焦的改动,所有承重声明均已在基础代码中核实;唯一未落定的是被审提交上仍在运行的单元测试套件(外加第 2 阶段提到的"路径含通配符"非阻塞小问题)。 退一步看:这个改动修复的是代码中直接可见的同意不对称——用户被要求授权任意的模型编写 JavaScript,而全部披露只是一个字符数;一次"always allow"点击就把这份同意扩展到此后的每一个脚本。实现与我的独立方案一致,复用了仓库既有原语而非新造平行机制,diff 也是达成目标所需的最小集合——一个工具文件、一个测试文件,别无其他。几个判断都合理且明说:内联脚本完全失去"always allow"(全新的模型编写源码没有可供收窄的稳定身份)、格式错误的 meta 降级而非拖垮对话框、成本警告移到开销发生之前。若六个月后由我维护,malformed-meta 降级测试与按行为断言的规则匹配测试正是我会想要的锚点。 被审提交上的 CI 仍在运行( — Qwen Code · qwen3.8-max Reviewed at |
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. ✅
|
Released in v0.22.2. |
What this PR does
Gives the Workflow tool a real approval dialog. Instead of
Confirm WorkflowTooloverRun a workflow script (4127 chars), the user now sees what the script says it will do — the workflow's name and description, its declared phases with their details, the resolvedargs, and a bounded excerpt of the source — under the titleRun a dynamic workflow?.All of that is read from
export const metaviaextractAndStripMeta, which parses the literal rather than evaluating it. That property is what makes reading it safe on the approval path: nothing model-authored executes before the user has approved anything. It does throw on a malformed literal, so the call is wrapped and degrades to a reduced dialog — a script with a broken meta block has to stay approvable-or-rejectable rather than take the dialog down with it.Everything displayed is passed through
stripAnsiAndControlfirst, and the screen ships with the preview rather than as a follow-up, because until now nothing was displayed and so nothing could be spoofed — a preview without the screen is what would open the hole. Single-line fields are flattened, which is the desired behaviour for ameta.namespanning three lines. The script excerpt is sanitized line by line instead, because\nis a C0 control character and the naive call would collapse the script into one unreadable line.The same object also scopes the grant. An inline
scriptis fresh model-authored source on every call, so it can never be pre-approved:hideAlwaysAllowremoves the option and an emptypermissionRulesstopsinjectPermissionRulesIfMissingfrom supplying the bare tool name, whichbuildPermissionRulesdocuments as matching all invocations of the tool. AscriptPathnames a file the user chose, so it stays pre-approvable, scoped to that path through akey:valueparam matcher.Finally, the token-cost warning now arrives before the spend it warns about, rather than only on the success path after the run.
Why it's needed
The user is asked to authorize arbitrary model-authored JavaScript that can dispatch up to the per-run agent cap, provision git worktrees, and spend an uncapped token budget — and the entire disclosure was a character count. The asymmetry is visible within a single run: the subagent approvals that same workflow bubbles up each get a full rich dialog, while the run that spawns them gets a number.
The grant was the second half of the problem.
WorkflowToolInvocationsupplied nopermissionRules, soinjectPermissionRulesIfMissingfilled in the bare tool name — and one click on "always allow" turned consent for the one script the user had read into standing consent for every inline script the model writes afterwards.Reviewer Test Plan
How to verify
Observed:
Test Files 4 passed (4),Tests 68 passed (68). The workflow tool file alone is 52 passing, 10 of them new.The ten new tests sit under a single
approval dialogdescribe block and each names the regression it exists to catch. The two worth reading first:Malformed meta must degrade, never throw. This is the load-bearing case. Reading meta now happens on the approval path, and
extractAndStripMetathrows on a broken literal — if the dialog threw, the user could not even say no. The test feedsexport const meta = { name: someIdentifier }(an identifier, which the parser refuses) and asserts the dialog still renders and still shows the script.The scriptPath grant is asserted behaviourally, not textually. A rule that reads plausibly but never matches would make "always allow" appear to work while silently doing nothing, which is a worse affordance than not offering the button. So the test parses the rule the tool actually emitted and runs it back through
matchesRule: it must resolve an invocation carrying the samescriptPath, must not resolve a different path, and must not resolve an inlinescript.To confirm the tests are anchored to the implementation rather than passing vacuously, revert only the production file and re-run:
git stash push -- packages/core/src/tools/workflow/workflow.ts && cd packages/core && npx vitest run src/tools/workflow/workflow.test.ts. Observed: all 10 new tests fail, the other 42 stay green.Also run:
npx tsc --noEmit -p tsconfig.json(exit 0),npx prettier --check,npx eslinton both files — all clean.Evidence (Before & After)
Before, for any workflow at all:
After, for a script whose meta declares two phases:
These are the strings the new tests assert on rather than a screenshot — the change is in
packages/coreand the TUI renders thepromptfield it is given, unmodified, via the existingrenderPromptAsPlainTextpath.Tested on
Environment
Unit tests via
npx vitest runfrompackages/core, plustsc --noEmit, prettier and eslint.Risk & Scope
vm.Scriptevaluation. The second tradeoff is deliberate: users who relied on "always allow" for inline scripts lose it. That is the point of the change, but it is a real workflow change for anyone who had granted it, and their existing broad rule is not migrated or revoked by this PR.meta.titleis not implemented —WorkflowMetahas notitlefield and adding one means touching the literal parser's contract and the run registry, which is a separate concern from disclosure; the dialog usesmeta.name. AscriptPathworkflow's own meta is not read — the dialog names the resolved path but does not open the file, becausereadWorkflowFileSecurelyis private toworkflow-saved.tsand its realpath/symlink fence is load-bearing; adding file I/O and its failure modes to the dialog path was not worth widening this diff, so the full preview covers inline scripts, which is where arbitrary model-authored code actually arrives. There is no "refuse if too large to display" branch —getConfirmationDetailshas no refusal channel, so the excerpt is bounded and labelled instead; refusing an oversized script pre-launch belongs with the pre-launch compile gate, not here. No changes to enablement, the keyword trigger, the runtime, or thescriptauthoring contract.Workflowrule keeps working until the user removes it — this PR stops new ones being created, it does not revoke old ones.Linked Issues
None.
中文说明
这个 PR 做了什么
给 Workflow 工具一个真正的审批对话框。用户看到的不再是
Confirm WorkflowTool加上Run a workflow script (4127 chars),而是脚本自己声明将要做的事——workflow 的名称与描述、声明的各个阶段及其细节、解析后的args,以及一段有长度上限的源码摘录,标题为Run a dynamic workflow?。这些内容全部通过
extractAndStripMeta从export const meta读取,该函数是解析字面量而非求值。正是这个性质使得在审批路径上读取它是安全的:在用户批准任何东西之前,不会有任何模型编写的代码被执行。它在字面量格式错误时会抛异常,因此调用被包裹起来并降级为简化版对话框——meta 块损坏的脚本必须仍然可批准或可拒绝,而不是把对话框一起拖垮。所有展示出来的内容都先经过
stripAnsiAndControl,并且这道防护是与预览同时加入的,而不是事后补上:在此之前什么都不显示,所以什么都无法被伪造——只加预览不加防护恰恰会打开这个口子。单行字段会被压平,这对于跨三行的meta.name正是我们想要的行为。脚本摘录则改为逐行净化,因为\n属于 C0 控制字符,直接调用会把整个脚本压成一行不可读的文本。同一个对象还收窄了授权范围。内联
script每次调用都是全新的模型编写源码,因此永远不能被预先批准:hideAlwaysAllow移除了该选项,而空的permissionRules阻止injectPermissionRulesIfMissing填入裸工具名——buildPermissionRules明确记载该规则会匹配该工具的所有调用。scriptPath指向的是用户自己选择的文件,因此仍可预先批准,但通过key:value参数匹配器限定到该路径。最后,token 成本警告现在出现在它所警告的那笔开销之前,而不是仅在运行成功之后才出现。
为什么需要
用户被要求授权的是任意的、由模型编写的 JavaScript,它可以派发到每次运行的 agent 上限、创建 git worktree、并消耗不设上限的 token 预算——而全部披露信息只是一个字符数。这种不对称在同一次运行内就能看到:该 workflow 冒泡上来的子 agent 审批,每一个都能拿到完整的富对话框,而派发它们的那次运行只拿到一个数字。
授权是问题的另一半。
WorkflowToolInvocation没有提供任何permissionRules,于是injectPermissionRulesIfMissing填入了裸工具名——点一次"always allow",就把用户对自己读过的那一个脚本的同意,变成了对模型此后编写的每一个内联脚本的长期同意。审阅者测试计划
如何验证
实测结果:
Test Files 4 passed (4)、Tests 68 passed (68)。仅 workflow 工具文件本身为 52 个通过,其中 10 个是新增的。十个新测试集中在一个
approval dialogdescribe 块里,每个都写明了它存在是为了捕捉哪一类回归。其中最值得先读的两个:meta 格式错误时必须降级,绝不能抛异常。 这是承重的用例。读取 meta 现在发生在审批路径上,而
extractAndStripMeta在字面量损坏时会抛异常——如果对话框抛了,用户连"拒绝"都点不了。该测试传入export const meta = { name: someIdentifier }(标识符,解析器会拒绝),并断言对话框仍然渲染、仍然显示脚本。scriptPath 授权是按行为断言的,不是按文本。 一条读起来合理却永远匹配不上的规则,会让"always allow"看似生效实则静默失效,这比不提供这个按钮更糟。因此测试把工具实际产出的规则解析出来,再送回
matchesRule:它必须匹配携带相同scriptPath的调用,必须不匹配另一个路径,也必须不匹配内联script。要确认这些测试是真的锚定在实现上、而非空转通过,可只回滚生产文件后重跑:
git stash push -- packages/core/src/tools/workflow/workflow.ts && cd packages/core && npx vitest run src/tools/workflow/workflow.test.ts。实测结果:10 个新测试全部失败,其余 42 个保持通过。另外执行了:
npx tsc --noEmit -p tsconfig.json(退出码 0)、npx prettier --check、npx eslint,两个文件均无问题。证据(前后对比)
之前,对任何 workflow 都是:
之后,对一个 meta 声明了两个阶段的脚本:
这里给出的是新测试所断言的字符串而非截图——改动位于
packages/core,TUI 会通过既有的renderPromptAsPlainText路径原样渲染它收到的prompt字段。测试环境
在
packages/core下通过npx vitest run运行单元测试,另加tsc --noEmit、prettier 与 eslint。风险与范围
vm.Script求值替换掉了。第二个取舍是刻意的:此前依赖对内联脚本使用"always allow"的用户会失去该选项。这正是本改动的目的,但对于已经授予过该权限的人来说是真实的流程变化,而本 PR 并不迁移或撤销他们已有的宽泛规则。meta.title——WorkflowMeta没有title字段,新增它意味着改动字面量解析器的契约和运行注册表,这与"披露"是两件事;对话框使用meta.name。不读取scriptPathworkflow 自身的 meta——对话框会给出解析后的路径,但不会打开该文件,因为readWorkflowFileSecurely是workflow-saved.ts的私有函数,且它的 realpath/符号链接围栏是承重的;把文件 I/O 及其失败模式引入对话框路径不值得为此扩大本次改动,因此完整预览覆盖的是内联脚本,而那正是任意模型编写代码真正到达的地方。没有"内容过大则直接拒绝"的分支——getConfirmationDetails没有拒绝通道,因此改为对摘录设上限并明确标注;对超大脚本在启动前予以拒绝属于启动前编译门的职责,不属于这里。未改动启用方式、关键词触发器、运行时,以及script的编写契约。Workflow规则在用户手动移除前仍然有效——本 PR 阻止新规则的产生,但不撤销旧规则。关联 Issue
无。