Skip to content

fix(ci): grant the packaging check the permissions its callee declares - #11532

Merged
yiliang114 merged 2 commits into
mainfrom
fix/packaging-check-permissions
Sep 10, 2026
Merged

yiliang114 merged 2 commits into
mainfrom
fix/packaging-check-permissions

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Grants the scheduled desktop packaging check the two permissions the release workflow's own jobs declare, so it can start.

Why it's needed

The first dispatch of the check died in two seconds with no job, no step and no annotation — the shape of a startup failure. A workflow that calls another must grant every permission the called workflow's jobs declare, and the release workflow's publish job declares write access to contents while the OSS mirror job asks to read actions. The check granted read access to contents and nothing else.

The trap is that this validation happens when the file is loaded, before any condition is evaluated. Both of those jobs are unreachable from this check — it hardcodes a dry run, and each of them is additionally gated on a real publish — but being unreachable does not exempt their declared permissions from the comparison. So a check that can never write still has to say it may.

Nothing here writes. The dry run leaves the publish and mirror jobs skipped on their own conditions, exactly as it did in the release runs this reuses.

Reviewer Test Plan

How to verify

Dispatch the check against main and confirm it now starts and fans out into the release workflow's four build jobs, and that the publish and mirror jobs report skipped rather than running. Before this change the same dispatch produced a run with zero jobs.

Evidence (Before & After)

Before: run 34444598159, startup_failure, 2 seconds, no jobs. After: a run that reaches the build matrix.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ⚠️ not tested

Workflow permission resolution only happens on GitHub's side, so this can only be confirmed by dispatching after merge.

Environment (optional)

N/A

Risk & Scope

  • Main risk or tradeoff: a scheduled workflow now declares write access to contents that it never exercises. The narrower alternative would be relaxing the permissions the release workflow's publish job declares, which trades a real safeguard for a cosmetic one on the caller.
  • Not validated / out of scope: whether the check passes once it starts. Its first real run is still ahead of it.
  • Breaking changes / migration notes: none.

Linked Issues

Follows #11519.

中文说明

这个 PR 做了什么

为定时的桌面打包检查授予被调用 workflow 各 job 所声明的两项权限,使其能够正常启动。

为什么需要

该检查的第一次触发在两秒内结束,没有 job、没有 step、也没有任何注解——这是启动失败的典型形态。调用方 workflow 必须授予被调用 workflow 中各 job 所声明的全部权限,而发布 workflow 的 publish job 声明了对 contents 的写权限,OSS 镜像 job 则请求读取 actions。该检查只授予了 contents 的读权限。

陷阱在于:这项校验发生在文件被加载时,早于任何条件求值。这两个 job 从本检查出发都是不可达的——它把 dry run 写死为 true,而两者各自还有以真实发布为前提的门槛——但"不可达"并不能让它们声明的权限免于比对。因此一个永远不会写入的检查,仍然必须声明它可能写入。

这里不会有任何写入。dry run 会让 publish 与镜像 job 按各自条件保持 skipped,与它所复用的那些发布运行中的表现完全一致。

评审验证计划

如何验证

针对 main 触发该检查,确认它现在能够启动并展开为发布 workflow 的四个构建 job,且 publish 与镜像 job 报告 skipped 而非实际运行。在此改动之前,同样的触发产生的是一个零 job 的运行。

证据(Before & After)

之前:运行 34444598159startup_failure,2 秒,无任何 job。之后:能够走到构建矩阵的运行。

测试平台

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ⚠️ 未测试

workflow 的权限解析只发生在 GitHub 侧,因此只能在合并后通过触发来确认。

环境(可选)

N/A

风险与范围

  • 主要风险/取舍:一个定时 workflow 现在声明了它从不使用的 contents 写权限。更收敛的替代方案是放宽发布 workflow 中 publish job 所声明的权限,但那是用一项真实的防护换取调用方形式上的收敛。
  • 未验证 / 不在范围内:该检查启动之后能否通过。它的第一次真实运行仍在前方。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

承接 #11519

The scheduled packaging check failed to start. A caller must grant every
permission the called workflow's jobs declare, and the release workflow's
publish job declares `contents: write` while the OSS mirror declares
`actions: read` — the check granted only `contents: read`.

That validation runs when the workflow file is loaded, before any `if` is
evaluated, so the two jobs being unreachable in a dry run does not exempt
their permissions. The run died in two seconds with no job and no
annotation, which is what a startup failure looks like.

Grant both. Nothing here writes: the check hardcodes `dry_run` to true,
which skips the publish and the mirror job on their own conditions.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval not posted — the PR head moved (or the PR closed) after the review of 1701a22; approving now would attest to unreviewed code. Re-run @qwen-code /triage on the new head. finalize run

⚠️ 延迟审批未提交 —— 审查 1701a22 之后 PR head 已变更(或 PR 已关闭),此时审批会为未审查的代码背书。请在新 head 上重新运行 @qwen-code /triage查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks — and thanks for chasing this the same day it broke.

Template looks good ✓

Problem: observed, not theoretical. I checked run 34444598159 through the API rather than taking the description's word for it: conclusion: startup_failure, workflow_dispatch, started 2026-09-10T06:18:05Z, finished 06:18:07Z, and total_count: 0 jobs against .github/workflows/desktop-packaging-check.yml. Two seconds with no job, no step and no annotation is exactly the shape of a load-time rejection. It also lines up with the origin: #11519 merged at 05:30 that same morning, so this repairs a schedule that broke 48 minutes after it landed — and that failed dispatch is the only run this workflow has ever produced.

Direction: squarely in scope. This is CI infrastructure reviving a currently-dead schedule, not new surface. No CHANGELOG reference applies, and none should.

Size: not applicable — no core paths, 8 changed lines in one workflow file.

Approach: the scope is right and I could not find a smaller correct fix. I wrote down what I would do before reading the diff, and it is the same two lines: the caller has to cover the union of what the callee's jobs declare, and that union is exactly contents: write (publish, desktop-release.yml:601) plus actions: read (sync-oss, desktop-release.yml:746) — prepare and build declare nothing and fall back to the callee's own top-level contents: read. I followed the chain one level further: sync-oss calls sync-desktop-to-oss.yml, whose sync job asks for actions: read + contents: read, already covered by that grant. So the permission set is complete, not merely plausible, and workflow_call declares inputs only — no required secrets the caller fails to pass. Your rejection of the narrower alternative (relaxing what publish declares) is the right call too: that trades a real safeguard on the release path for cosmetics on the caller.

One thing worth thinking about, not a blocker: this couples the two files silently. If desktop-release.yml later grows a job declaring, say, id-token: write, this check dies the same way — two seconds, zero jobs, no annotation — and nothing in CI catches it, because actionlint does not validate permissions across a reusable-workflow boundary. Your comment explains the constraint from the caller's side; a reciprocal note on the callee ("a new job-level permission here must also be granted in desktop-packaging-check.yml") would sit where the next person is actually editing.

Risk: no elevated risk signals — no high-risk paths matched. The residual risk is the one you already named: a scheduled workflow now declares contents: write. Worth saying the exposure is narrower than the declaration reads, since the callee's own top-level permissions: contents: read should cap the jobs that actually run — prepare and build stay read-only and write is reachable only from publish, which the hardcoded dry_run: true skips. I executed nothing to confirm that, so treat it as my reading of the permission model rather than verified behavior; the first real run is the oracle.

Moving on to code review. 🔍

中文说明

感谢提交,也感谢你当天就把这个问题追了下去。

模板完整 ✓

问题:是已观测到的故障,不是理论性加固。我通过 API 核实了运行 34444598159,而不是只采信 PR 描述:conclusion: startup_failureworkflow_dispatch,开始于 2026-09-10T06:18:05Z,结束于 06:18:07Z,针对 .github/workflows/desktop-packaging-check.ymltotal_count: 0(无任何 job)。两秒结束、无 job、无 step、无注解,正是加载期被拒绝的典型形态。这也与源头吻合:#11519 在当天 05:30 合并,因此本 PR 修复的是一个落地 48 分钟后就坏掉的定时任务——而那次失败的触发是该 workflow 迄今唯一的一次运行。

方向:完全在范围内。这是 CI 基础设施修复一个当前已失效的定时检查,不是新增能力。CHANGELOG 无对应条目,也不应有。

规模:不适用——未触及核心路径,单个 workflow 文件共 8 行改动。

方案:范围合适,我找不到更小且正确的修法。在读 diff 之前我先写下了自己的做法,结论是同样的两行:调用方必须覆盖被调用 workflow 各 job 声明的权限并集,而该并集恰好是 contents: writepublish,desktop-release.yml:601)加 actions: readsync-oss,desktop-release.yml:746)——preparebuild 未声明权限,回落到被调用方自身的顶层 contents: read。我又往下追了一层:sync-oss 调用 sync-desktop-to-oss.yml,其 sync job 请求 actions: read + contents: read,已被该授权覆盖。所以这个权限集合是完备的,而不只是看起来合理;workflow_call 也只声明了 inputs,没有调用方未传递的必需 secrets。你对更收敛替代方案(放宽 publish 自身的声明)的否决也是对的:那是用发布路径上一项真实的防护,换取调用方形式上的收敛。

有一点值得考虑,但不是阻塞项:这让两个文件产生了无声的耦合。如果 desktop-release.yml 之后新增一个声明例如 id-token: write 的 job,这个检查会以同样方式死掉——两秒、零 job、无注解——而 CI 里没有任何东西能发现,因为 actionlint 不会跨 reusable workflow 边界校验权限。你的注释从调用方一侧解释了这个约束;在被调用方加一条对应的提示("此处新增 job 级权限时,也必须同步授予 desktop-packaging-check.yml")会落在下一个人真正编辑的位置上。

风险:无升级风险信号——未匹配到高风险路径。剩余风险正是你已经点出的那个:一个定时 workflow 现在声明了 contents: write。值得说明的是,实际暴露面比这行声明看起来要窄,因为被调用方自身的顶层 permissions: contents: read 应当会把真正运行的 job 限制住——preparebuild 仍是只读,写权限只有 publish 能取到,而写死的 dry_run: true 会让它跳过。我没有执行任何东西来确认这一点,所以请把它当作我对权限模型的理解,而非已验证的行为;第一次真实运行才是判据。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 1701a220d588c4efda5d320c6c500eb0c26c3955 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Code review

No blockers. The fix is correct and I could not find a smaller one.

What I checked, beyond the diff:

  • The permission set is exactly the required union, not a guess. desktop-release.yml declares job-level permissions in only two places — publish wants contents: write (line 601) and sync-oss wants actions: read + contents: read (lines 746–748). prepare and build declare none and fall back to the callee's top-level contents: read (line 70). The two lines added here cover that whole set, with write subsuming read on contents.
  • The chain is closed one level deeper. sync-oss is itself a reusable-workflow call into sync-desktop-to-oss.yml, whose sync job declares actions: read + contents: read — already inside what sync-oss grants, which is now inside what this caller grants. Nothing is missing further down.
  • Nothing else about the call would stop it starting. workflow_call declares seven required inputs and no required secrets; the caller passes all seven. secrets: inherit is absent, which is right for a run that must never hold a signing key.
  • This is the only caller. grep across .github/workflows/ finds no other file calling desktop-release.yml, so there is no second place needing the same treatment.
  • Both write-capable jobs really are unreachable. publish gates on dry_run == false, and the caller hardcodes dry_run: true. sync-oss gates on workflow_dispatch && dry_run == false && draft == false && prerelease == false, and the caller passes draft: true and prerelease: true — so it stays skipped even on the dispatch path where github.event_name matches. The code comment's claim holds on both triggers.
  • The comment is the right kind. Five lines explaining a load-time constraint that reads like an over-grant and invites someone to "tidy" it back to contents: read. That is a genuine why, and it is what stops this regressing.

Two observations, neither blocking:

  • Neither granted scope is exercised by any job that actually runs — actions: read is only declared by the skipped sync-oss, contents: write only by the skipped publish. That is precisely the awkward consequence of load-time validation the comment describes, not a defect, but it is worth being explicit that the grant is declarative rather than used.
  • This repo's workflow size ratchet is satisfied without a baseline bump: the file goes from 1634 bytes to 2033 (I measured the head copy through the raw API), and check-workflow-size.sh allows 4096 bytes of growth per file. The Classify PR log confirms the gate does run here (skip_ci=false, WORKFLOW_SIZE_BASE_SHA=cb24374d…), so it will say so itself.

I skipped the sequence diagram and the changed-files table — one workflow file, no runtime flow reshaped. Neither would have earned its place.

Test evidence

This is an unattended CI run, so I executed nothing from the PR and read its own checks through the API instead. Names and conclusions below are GitHub-set metadata for commit 1701a220d588c4efda5d320c6c500eb0c26c3955, fetched once — no polling.

Read this part carefully: a green CI here is not evidence the fix works. Lint & Static runs actionlint (node scripts/lint.js --actionlint, ci.yml:1140/1171) and the workflow-size gate, and actionlint does not model permission requirements across a reusable-workflow boundary — which is exactly why #11519 merged green and then died in two seconds. No job in this PR's CI dispatches a workflow, so nothing in the suite can observe the thing being fixed. The checks that matter most for this diff, Lint & Static and Test, were still in_progress when I fetched.

Final CI results for 1701a22 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Test (ubuntu-latest, Node 22.x) 🚫 cancelled
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The remaining in_progress entries (triage, review-pr) are bot orchestration on pull_request_target, not this PR's CI; the one pull_request run, Qwen Code CI, is still going. Skipped entries are the classify-profile lane doing its job on a .github/-only diff, not failures. No red checks, so no failing-job log to excerpt.

Not verified: that the workflow now starts. The claim this PR makes is behavioural and neither sandboxed lane can settle it — @qwen-code /verify A/Bs the built CLI and @qwen-code /tmux drives the TUI, while GitHub resolves reusable-workflow permissions at dispatch time against the default branch, outside both. The oracle is a real dispatch: after merge, trigger Desktop Packaging Check on main and confirm the run fans out into the four build jobs with publish and sync-oss reported as skipped, instead of zero jobs in two seconds. The daily 0 16 * * * cron will do this unprompted if nobody dispatches first. Also not verified: my Stage 1 reading that the callee's top-level contents: read caps prepare/build, which is my model of the permission semantics rather than something I observed.

中文说明

代码审查

无阻塞项。这个修复是正确的,我也找不到更小的修法。

除 diff 之外我核实的内容:

  • 权限集合恰好等于所需的并集,不是猜测。 desktop-release.yml 只在两处声明 job 级权限——publish 需要 contents: write(第 601 行),sync-oss 需要 actions: read + contents: read(第 746–748 行)。preparebuild 未声明,回落到被调用方的顶层 contents: read(第 70 行)。此处新增的两行覆盖了整个集合,且 contents 上的 write 已包含 read
  • 调用链再往下追一层也是闭合的。 sync-oss 自身是对 sync-desktop-to-oss.yml 的 reusable workflow 调用,其 sync job 声明 actions: read + contents: read——已被 sync-oss 的授权包含,而后者现在又被本调用方的授权包含。更下层没有遗漏。
  • 调用本身没有其他会导致无法启动的因素。 workflow_call 声明了七个必需 inputs,且没有必需 secrets;调用方七个全部传递。未使用 secrets: inherit 是对的——这个运行永远不应持有签名密钥。
  • 这是唯一的调用方。.github/workflows/ 下 grep 未发现其他文件调用 desktop-release.yml,因此没有第二处需要同样处理。
  • 两个具备写能力的 job 确实不可达。 publishdry_run == false 为门槛,而调用方把 dry_run: true 写死。sync-oss 的门槛是 workflow_dispatch && dry_run == false && draft == false && prerelease == false,而调用方传入 draft: trueprerelease: true——因此即使在 github.event_name 匹配的 dispatch 路径上它依然跳过。代码注释的说法在两种触发方式下都成立。
  • 这段注释是恰当的那一类。 五行解释了一个加载期约束:它在表面上看起来像过度授权,很容易被后人"顺手清理"回 contents: read。这是真正的 why,也正是防止此处回退的东西。

两点观察,均非阻塞:

  • 实际运行的 job 一个都没有用到被授予的这两项权限——actions: read 只被跳过的 sync-oss 声明,contents: write 只被跳过的 publish 声明。这正是注释所描述的、加载期校验带来的别扭后果,不是缺陷;但值得说清楚:这个授权是声明性的,而非被使用的。
  • 本仓库的 workflow 体积棘轮无需 bump baseline 即已满足:文件从 1634 字节变为 2033 字节(我通过 raw API 实测了 head 版本),而 check-workflow-size.sh 对单文件允许 4096 字节增长。Classify PR 日志确认该 gate 在本 PR 会运行(skip_ci=falseWORKFLOW_SIZE_BASE_SHA=cb24374d…),所以它自己会给出结论。

我跳过了时序图与变更文件表——只有一个 workflow 文件,没有重塑任何运行时流程,两者都不值得占用篇幅。

测试证据

这是无人值守的 CI 运行,因此我没有执行 PR 中的任何代码,而是通过 API 读取它自身的检查结果。下表中的名称与结论是 GitHub 设定的元数据,对应提交 1701a220d588c4efda5d320c6c500eb0c26c3955,只抓取一次,不做轮询。

这部分请仔细看:此处 CI 全绿并不构成修复有效的证据。 Lint & Static 运行 actionlint(node scripts/lint.js --actionlint,ci.yml:1140/1171)与 workflow 体积 gate,而 actionlint 不会跨 reusable workflow 边界建模权限需求——这恰恰是 #11519 绿着合并、随后两秒内死掉的原因。本 PR 的 CI 中没有任何 job 会触发一次 workflow,因此整个测试套件里没有任何东西能观察到被修复的那个行为。对这个 diff 最关键的两个检查 Lint & StaticTest,在我抓取时仍是 in_progress

(CI 结果表见上方英文部分,此处不重复。)

其余 in_progress 项(triagereview-pr)是 pull_request_target 上的机器人编排任务,不属于本 PR 的 CI;唯一那个 pull_request 运行 Qwen Code CI 仍在进行。skipped 项是分类 profile 在纯 .github/ diff 上的正常行为,不是失败。没有红色检查,因此没有失败 job 的日志可摘录。

未验证:该 workflow 现在能否启动。 本 PR 的主张是行为性的,而两个沙箱通道都无法判定——@qwen-code /verify 对构建产物做 A/B,@qwen-code /tmux 驱动 TUI,而 GitHub 是在触发时针对默认分支解析 reusable workflow 权限的,两者都覆盖不到。判据是一次真实触发:合并后在 main 上手动触发 Desktop Packaging Check,确认运行展开为四个构建 job,且 publishsync-oss 报告为 skipped,而不是两秒内零 job。如果没人先触发,每天 0 16 * * * 的 cron 也会自行完成这件事。同样未验证的还有我在 Stage 1 的判断——被调用方顶层的 contents: read 会限制住 prepare/build;那是我对权限语义的理解,不是我实际观察到的行为。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 1701a220d588c4efda5d320c6c500eb0c26c3955 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — correct, minimal, and backed by a failure I could verify independently; my one reservation is a maintenance coupling that outlives this diff.

Going back to what I wrote down before reading the code: the same two lines. That is the strongest signal I have here, because I could not construct a smaller correct fix and the alternatives are worse in ways worth stating. Splitting the release workflow into a build-only half plus a publish half would remove the awkward declarative grant entirely and is arguably the right long-term shape, but that is a substantial refactor of the release path to repair a broken cron — not this PR's job. Relaxing what publish declares, which the author already weighed, trades a real safeguard on the release path for cosmetics on the caller.

What convinced me is that every load-bearing claim here was checkable, so I checked it rather than reading it. The cited run really is a startup_failure — zero jobs, two seconds, no annotation. The callee's declared permissions really do union to exactly these two scopes, including one level deeper into the OSS mirror workflow. The failure really was introduced by a PR that merged 48 minutes earlier, and this is the only caller, so nothing else is left broken. Nothing in my assessment rests on the description's framing.

The honest limit, and the reason this is 4 rather than 5: no amount of review or CI proves the run will now start, because GitHub resolves this at dispatch time against the default branch. I am approving a static argument I believe is complete, not an observed green run. I think that is the right trade — leaving a currently-dead daily packaging check broken until someone can prove the repair works would just keep it dark, and the downside if I am wrong is another two-second failure with no side effects, not a bad release.

If I am maintaining this in six months, the comment block is what saves me: it is the thing that stops a cleanup pass from reverting contents: write back to read. What could still bite is the silent coupling — a future job-level permission in the release workflow breaks this check the same quiet way, with nothing in CI positioned to notice, since actionlint does not cross the reusable-workflow boundary. A reciprocal note on the callee would fix that. I would not widen this diff to add it; worth doing the next time someone is in that file.

Approval is deferred until CI lands green on 1701a220d588c4efda5d320c6c500eb0c26c3955Qwen Code CI was still running when I wrote this (Lint & Static and Test in progress), and approving now would attest to a result that does not exist yet. The finalize job will post the commit-pinned approval once every check on that commit completes green, and withhold it if anything lands red or the head moves.

中文说明

Confidence: 4/5 — 正确、最小化,且背后有一个我能独立核实的真实故障;我唯一的保留意见是一个会超出本 diff 生命周期的维护耦合。

回到我在读代码之前写下的方案:同样是这两行。这是我在此处能给出的最强信号——我构造不出更小且正确的修法,而替代方案的问题值得说清楚。把发布 workflow 拆成"只构建"与"发布"两半,可以彻底消除这种别扭的声明式授权,长期看也许才是正确形态;但那是为了修一个坏掉的 cron 而对发布路径做大规模重构,不该由本 PR 承担。而放宽 publish 自身声明的做法(作者已经权衡过),是用发布路径上一项真实的防护去换调用方形式上的收敛。

说服我的是:这里每一条关键主张都是可核查的,所以我核查了,而不是读一遍就接受。所引用的运行确实是 startup_failure——零 job、两秒、无注解。被调用方声明的权限确实恰好并成这两个 scope,包括再往下一层的 OSS 镜像 workflow。故障确实由 48 分钟前合并的 PR 引入,而本 PR 是唯一的调用方,因此没有其他地方还处于损坏状态。我的判断没有任何一点建立在 PR 描述的表述之上。

诚实的局限,也是这里是 4 分而非 5 分的原因:无论审查还是 CI,都无法证明这个运行现在能够启动,因为 GitHub 是在触发时针对默认分支做这项解析的。我批准的是一个我认为完备的静态论证,而不是一次已观察到的绿色运行。我认为这个取舍是对的——让一个当前已失效的每日打包检查继续坏着,直到有人能证明修复有效,只会让它一直暗着;而如果我判断错了,代价是再一次两秒的失败、没有任何副作用,而不是一次错误的发布。

如果六个月后由我维护这段代码,那段注释正是救我的东西:它是阻止某次清理把 contents: write 改回 read 的关键。仍可能出问题的是那个无声的耦合——发布 workflow 将来新增一个 job 级权限,会以同样安静的方式弄坏这个检查,而 CI 里没有任何环节处在能发现它的位置上,因为 actionlint 不跨 reusable workflow 边界。在被调用方加一条对应的提示能解决这一点。我不会为此扩大本 diff 的范围;等下次有人改到那个文件时再做即可。

批准已推迟到 CI 在 1701a220d588c4efda5d320c6c500eb0c26c3955 上全绿之后——我写下这段时 Qwen Code CI 仍在运行(Lint & StaticTest 进行中),此刻批准等于为一个尚不存在的结果背书。finalize 流程会在该提交上所有检查全部完成后发出绑定该提交的批准;若有检查变红或 head 发生移动,则会保留不发。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 1701a220d588c4efda5d320c6c500eb0c26c3955 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • X1-1 caller/callee permission-superset contract unpinned — already reported (comments 5614193538, 5614243900)

Not reviewed: reverse audit — stopped before round 1 by the review time budget.

Not linted (tool limitation, not a blocker): .github/workflows/desktop-packaging-check.yml — actionlint embedded-shell source mapping is not yet supported.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:反向审计——评审时间预算不足,未能开始第 1 轮。

未检查(工具限制,非阻断):.github/workflows/desktop-packaging-check.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment thread .github/workflows/desktop-packaging-check.yml
The Test lane on this branch was failing on WebShellSidebar.brand and the
daemon route-surface guard, neither of which this branch touches — both
were red on main, at and before the commit this branched from. #11530
realigned those suites with shipped behavior; merge it in so the lane
reports on this change rather than on inherited breakage.

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.
Approval blockers: none.

Triage: Scan — single CI workflow permissions fix, no production code changes.

What I checked:

  1. Root cause: desktop-packaging-check.yml calls desktop-release.yml via uses:. GitHub Actions checks the caller's permissions against all permissions declared by the callee's jobs when the file loads, before any if is evaluated. So even though dry_run: true skips both publish (which declares contents: write) and sync-oss (which declares actions: read), the caller still needs to grant them.

  2. The fix: contents: readcontents: write + actions: read, matching exactly what desktop-release.yml's jobs declare. Verified: prepare job declares contents: write, sync-oss job declares actions: read.

  3. Safety: The comment correctly explains that nothing here actually writes — dry_run is hardcoded true and the publish/sync-oss jobs are unreachable. The elevated permissions are necessary for the workflow to start, not for any write operation.

  4. Comment quality: The added 7-line comment block clearly explains the "why" (the file-load-time permission check vs. if-evaluation-time) so the next reader doesn't wonder why a dry-run workflow needs contents: write.

Reviewed with AI assistance.

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical-only scan at 8f6f0519 — no blocking defect. Approving.

Single file, +7/-1: five comment lines plus a permission change in .github/workflows/desktop-packaging-check.yml. Because this grants contents: write on a workflow that runs on a nightly cron, I treated the escalation as the whole review and verified its necessity and its blast radius rather than reading the comment's justification.

Prior review history — nothing blocking

No REQUEST_CHANGES review has ever been filed here. The one review at 1701a220 reports a single finding graded sev: "S" with floor: "o" and no Critical, and the triage code-review stage records "No blockers. The fix is correct and I could not find a smaller one." That Suggestion — the caller's dry_run: true becoming load-bearing without a test pinning the reachability invariant — was acknowledged by the author and deferred to a named follow-up. Suggestions do not gate this review, so there is no historical blocking issue to confirm as fixed. The head has since moved to 8f6f0519, a merge of main; the reviewed delta is unchanged by it.

The grant is exactly the required superset, not an over-grant

The callee desktop-release.yml declares permissions: contents: 'read' at the top level (line 70), which alone would not justify this change. The need comes from its two job-level declarations:

  • publish: (line 593) → permissions: contents: 'write' (lines 601-602), and it genuinely writes — gh release create, gh release upload --clobber, gh release edit at lines 656-722.
  • sync-oss: (line 739) → permissions: actions: 'read' (lines 746-747).

Those are the only two permissions: blocks below the top level in the callee, and the two keys this PR adds are precisely those two values. Nothing is granted beyond what the callee declares, so the escalation is the minimum the caller/callee contract requires and the PR title's claim is accurate.

Every job that could exercise the write is unreachable from this caller

  • publish is gated if: inputs.dry_run == false && github.repository == 'QwenLM/qwen-code' (line 595). The caller hardcodes dry_run: true at line 50.
  • sync-oss is gated if: github.event_name == 'workflow_dispatch' && inputs.dry_run == false && inputs.draft == false && inputs.prerelease == false && github.repository == 'QwenLM/qwen-code' (line 741). The caller passes dry_run: true, draft: true and prerelease: true, so this job is unreachable on every trigger of this workflow, including the nightly 0 16 * * * cron and a manual dispatch.
  • dry_run is not exposed as a workflow_dispatch input of the caller — the only input is qwen_code_ref (lines 19-24). So an operator cannot flip it to false through this workflow, which is what keeps the hardcoded literal from being an operator-reachable switch.

Two further limits bound the blast radius independently of the if gates: the caller passes no secrets: inherit (and says so at lines 43-44), so the callee never holds a signing or OSS credential on this path; and the caller's only job is gated on github.repository == 'QwenLM/qwen-code' (line 42), so it does not run on forks. clobber: false is also passed.

No consumer or gate breaks

Nothing in scripts/tests/, packages/ or any other workflow references desktop-packaging-check.yml, so no test pins the previous contents: 'read' and the change cannot turn a suite red; .github/workflows/.size-baseline carries an entry for desktop-release.yml but none for this file, so no size gate applies either. desktop-release.yml itself is untouched, and desktop-packaging-check.yml is the only workflow that calls it, so no second caller inherits a changed contract. The YAML is well-formed — two keys under a top-level permissions: mapping, both valid GitHub permission names and values.

CI

Classify PR, Desktop Shell (ubuntu-22.04), Desktop Shell (windows-2022), Integration Tests (no-AK, No Sandbox), assign, authorize, label and Remind on force-push are green. Lint & Static, Test (ubuntu-latest, Node 22.x) and review-pr were still in progress at publish time; per this gate pending checks are not a blocker and none of the completed checks is attributable to this diff. The one lane that would actually exercise the changed file — the packaging check itself — runs on cron and dispatch, not on a pull request, so its next scheduled run is the real confirmation.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review at head 8f6f0519 (GitHub does not let me approve my own PR). No blocking finding. The grant is correct, minimal, and provably inert on every reachable path. Two notes below, one of which corrects a claim in the description.

Verified against the callee rather than against the description

I enumerated every job-level permissions: block in desktop-release.yml instead of trusting the summary:

callee job declared permissions reachable from this check?
prepare (:82) none — inherits the callee's top-level contents: 'read' yes
build (:153) none — same yes
publish (:593) contents: 'write' (:601-602) noif: inputs.dry_run == false && …, and the caller hardcodes dry_run: true
sync-oss (:739) actions: 'read', contents: 'read' (:746-748) no — four independent exclusions, below

So the union the caller must cover is contents: write + actions: read + contents: read, and the new block grants exactly actions: 'read' + contents: 'write' — complete, with write subsuming the read. Nothing is missing, so the load-time comparison that killed the run should now pass.

sync-oss is worth spelling out because it is the source of the actions: 'read' half and it is excluded four separate ways: its gate is github.event_name == 'workflow_dispatch' && inputs.dry_run == false && inputs.draft == false && inputs.prerelease == false && github.repository == 'QwenLM/qwen-code', and the caller passes dry_run: true, draft: true, prerelease: true — while a schedule run is not workflow_dispatch in the first place. It is also itself a nested reusable call (uses: './.github/workflows/sync-desktop-to-oss.yml'), which is why its declared permissions propagate up to this caller at all.

One defence-in-depth property the new comment does not mention, and should. Because the callee's top-level permissions is contents: 'read', the prepare and build jobs — the only ones this check actually runs — inherit read-only and cannot consume the write grant even if their own gating changed later. The write is consumable solely by publish, which is unreachable here. So the grant is inert twice over: unreachable by condition, and unheld by any job that runs. A future editor who flips dry_run or adds a caller job should be able to see that from the file alone; right now they can only see the dry_run half.

Also confirmed the diagnosis rather than assuming it: run 34444598159 is conclusion: startup_failure, created_at 06:18:05Zupdated_at 06:18:07Z, run_attempt: 1, and its jobs list is empty — the 2-second, zero-job, zero-annotation shape the description claims.

The description overstates what cannot be verified pre-merge

Workflow permission resolution only happens on GitHub's side, so this can only be confirmed by dispatching after merge.

That is too strong. This branch is in-repo (QwenLM:fix/packaging-check-permissions, cross_repo: false), and the file at that ref is blob cf2c37a47b — matching the diff's post-image — so workflow_dispatch accepts it:

gh workflow run desktop-packaging-check.yml --repo QwenLM/qwen-code --ref fix/packaging-check-permissions

A workflow_dispatch run uses the workflow file from the selected ref, so the load-time permission comparison would be exercised against exactly this change and a startup_failure vs. a real job list would settle it before merge.

I did not run it, and I would not. It fans out into the release workflow's whole build matrix on shared runners to save at most a day of latency, and cron: '0 16 * * *' confirms it for free within a day of merge. Worth correcting the sentence so the option is not recorded as impossible — the honest scope note is "not verified because the matrix is not worth spending on a permission fix the daily schedule confirms anyway", which is a different claim from "cannot be verified".

On the "narrower alternative" paragraph

The description rejects narrowing the callee's publish declaration, and I agree — that trades a real safeguard for a cosmetic one. There is a third option it does not name: extract prepare + build into a workflow_call-only workflow that both desktop-release.yml and this check call, so the check would grant contents: 'read' and never reference the publish path at all.

I would still not do it, and the reason is this workflow's own motivation: it exists because a CLI change broke AppImage bundling and only a real release run caught it, two weeks late. Calling a build-only sub-workflow would stop exercising desktop-release.yml itself — the wiring, the gates, the artifact hand-off — which is a large part of what makes the check worth running daily. Calling the real workflow and granting what it declares is the right trade. Naming it here only so that paragraph is not read as if one alternative had been considered and no others existed.

Merge mechanics

desktop-packaging-check.yml has no .github/CODEOWNERS entry (the only workflow paths owned are release.yml, finalize-release.yml and security-checks.yml), so no code-owner approval is required — two approvals from anyone with write access satisfy main's ruleset. CI at 8f6f0519: everything green except Test (ubuntu-latest, Node 22.x) and review-pr, both in flight. Head is a merge of main at 2488d12d, so the diff against base is the one file and nothing else.

中文说明

在 head 8f6f0519 上的自审(GitHub 不允许我批准自己的 PR)。没有阻塞项。 这次授权是正确、最小、且在所有可达路径上可证明为惰性的。下面两条说明,其中一条更正了正文里的一个说法。

对照 callee 核实,而不是对照描述

我把 desktop-release.yml 里每一个 job 级 permissions: 块都列了出来,而不是采信摘要:prepare(:82) 与 build(:153) 都没有声明、继承 callee 顶层的 contents: 'read',二者可达;publish(:593) 声明 contents: 'write'(:601-602),不可达——门禁是 if: inputs.dry_run == false && …,而调用方硬编码 dry_run: truesync-oss(:739) 声明 actions: 'read' + contents: 'read'(:746-748),不可达,见下。

所以调用方必须覆盖的并集是 contents: write + actions: read + contents: read,而新增的块恰好授予 actions: 'read' + contents: 'write'——完整,且 write 已包含 read。没有任何遗漏,所以那次让运行起不来的加载期比较现在应当通过。

sync-oss 值得单独说明,因为它正是 actions: 'read' 那一半的来源,而它被四重独立排除:门禁是 github.event_name == 'workflow_dispatch' && inputs.dry_run == false && inputs.draft == false && inputs.prerelease == false && github.repository == 'QwenLM/qwen-code',而调用方传的是 dry_run: truedraft: trueprerelease: true——并且 schedule 触发时 event_name 本来就不是 workflow_dispatch。它自身还是一个嵌套的 reusable 调用(uses: './.github/workflows/sync-desktop-to-oss.yml'),这正是它声明的权限会向上传导到本调用方的原因。

新注释没提到、但应该提到的一层纵深防御。 因为 callee 的顶层 permissionscontents: 'read',所以本检查真正会跑的 preparebuild 继承的是只读,即使它们自己的门禁将来变了也无法消费这个 write 授权。write 只能被 publish 消费,而它在此不可达。所以该授权是双重惰性的:条件上不可达,且没有任何会运行的 job 持有它。将来某个改 dry_run 或给调用方加 job 的人,应该能只从这个文件里看出这一点;现在他只能看出 dry_run 那一半。

诊断本身我也确认了而不是假定:run 34444598159conclusionstartup_failurecreated_at 06:18:05Zupdated_at 06:18:07Zrun_attempt: 1,jobs 列表为空——正是正文所说的 2 秒、零 job、零 annotation 形态。

正文高估了「合并前无法验证」

Workflow permission resolution only happens on GitHub's side, so this can only be confirmed by dispatching after merge.

这句说过头了。本分支在仓库内(QwenLM:fix/packaging-check-permissionscross_repo: false),且该 ref 上的文件 blob 是 cf2c37a47b——与 diff 的后像一致——所以 workflow_dispatch 接受它:gh workflow run desktop-packaging-check.yml --repo QwenLM/qwen-code --ref fix/packaging-check-permissionsworkflow_dispatch 用所选 ref 上的 workflow 文件,因此加载期权限比较会正好作用在这次改动上,startup_failure 与真实 job 列表之别在合并前就能定论。

我没有跑它,也不建议跑。 它会在共享 runner 上扇出整个 release build matrix,只为了省下至多一天的延迟,而 cron: '0 16 * * *' 会在合并后一天内免费确认。值得更正这句话,免得这个选项被记录成不可能——诚实的范围说明是「未验证,因为为一个由每日定时任务即可确认的权限修复花掉整个 matrix 不值得」,这与「无法验证」是不同的主张。

关于「更窄的替代方案」那一段

正文否决了收窄 callee 的 publish 声明,我同意——那是拿一个真实的保护去换一个调用方表面上的好看。还有一个它没点名的选项:把 prepare + build 抽成一个只带 workflow_call 的 workflow,让 desktop-release.yml 与本检查都去调它,这样检查只需授予 contents: 'read',完全不引用 publish 路径。

我仍然不会这么做,理由就是本 workflow 自己的动机:它存在是因为一次 CLI 改动搞坏了 AppImage 打包,而只有真实的 release 运行才发现,晚了两周。改调一个只含构建的子 workflow,就不再锻炼 desktop-release.yml 本身——它的接线、门禁、artifact 交接——而那正是这个检查值得每天跑的重要部分。调用真实的 workflow 并授予它声明的权限,是正确的取舍。之所以在这里点名,只是为了让那一段不被读成「只考虑过一个替代方案、且不存在其它选项」。

合并机制

desktop-packaging-check.yml.github/CODEOWNERS 里没有条目(唯一被 own 的 workflow 路径是 release.ymlfinalize-release.ymlsecurity-checks.yml),所以不需要 code-owner 批准——两个有 write 权限的人批准即满足 main 的 ruleset。8f6f0519 上的 CI:除 Test (ubuntu-latest, Node 22.x)review-pr 在跑之外全绿。head 是对 main2488d12d)的合并,所以对 base 的 diff 只有这一个文件。

@qwen-code-dev-bot qwen-code-dev-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at head 8f6f0519.

Required CI is green — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell lanes all completed successfully; only review-pr is still running.

The premise is real and not inferred: run 34444598159 is a workflow_dispatch of the packaging check that GitHub reports as startup_failure with no jobs, which is exactly the shape a caller/callee permission mismatch produces.

I checked that the grant is the required union rather than a guess, including the hop the description does not mention. The called workflow declares contents: 'read' at top level (desktop-release.yml:69), the publish job declares contents: 'write' (:601), and the mirror job declares actions: 'read' plus contents: 'read' (:746) — the caller's new actions: read + contents: write is precisely the superset of those three. It is also enough transitively: sync-oss is itself a uses: call into sync-desktop-to-oss.yml, whose job declares exactly actions: read, contents: read, so nothing deeper in the tree can re-trigger the same load-time rejection.

"Nothing here writes" is structural rather than a promise: publish is gated on inputs.dry_run == false && github.repository == 'QwenLM/qwen-code', the mirror job additionally on github.event_name == 'workflow_dispatch' — which is never true for a called workflow — and the caller hardcodes dry_run: true. The two jobs that actually run, prepare and build, declare no permissions of their own and therefore inherit the called workflow's top-level contents: read. So the widened caller entry is a ceiling that GitHub compares at load time, not an effective write grant to any job that executes in this check — which is why the narrower alternative the description declines (stripping the publish job's own declaration) really would trade a real safeguard for a cosmetic one.

The second commit is the right hygiene too: it merges main to pick up the web-shell suite realignment, which is what the currently-red Test lane on other branches needs, and it is why this PR's own suite is green rather than carrying an unrelated failure.

No new Critical found. Two non-blocking notes, the first already recorded on the PR:

  • The contract that bit this check is still unpinned anywhere in the repo (X1-1). The next job-level permission added to desktop-release.yml will fail the same way — silently, at load time, before any if evaluates and with no annotation on the run. The .github/scripts battery that Lint & Static already runs parses these workflows, so a small assertion that each caller's permissions covers the union declared by its callee's jobs would turn a dead schedule into a red test.
  • The check's first real run is still ahead of this change: after merge, dispatch it once against main and confirm it fans out into the four build jobs with publish and the OSS mirror reporting skipped rather than running. That is also the acceptance criterion the description names.

@yiliang114
yiliang114 added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit fe7cf68 Sep 10, 2026
96 of 97 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants