fix(ci): classify the CI profile from the trusted base SHA - #10799
Merged
Conversation
Move CI profile classification into the classify_pr job and run the classifier from a sparse checkout of the PR's base commit instead of the PR head, so a fork cannot replace the code that decides which tests to skip. The test and no-AK jobs consume the job output and fall back to full when it is missing. Sweep the trusted checkout path with the other shared-pool residue names. Extracted from #10548. Claude-Session: https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h
Collaborator
|
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Fork PRs are unconditionally forced to Direction: aligned. This is the trust-model half of #10548, deliberately extracted so it can land on its own (the ECS updater allowlist and dependency-free helper lane stay in the parent PR). That scoping call makes sense — the mechanism is independent of the allowlist additions. Size: not applicable — no core module paths touched (all changes are Approach: the shape is right — classify once in Risk: no elevated risk signals — no high-risk paths matched, and the author is a maintainer. One property worth keeping in mind through review: the fail-closed chain is the whole safety story here, so each link (checkout → classify → output → consumer fallback) needs to stay explicit. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的真实问题，不是理论假设。fork PR 目前被无条件强制跑 方向：对齐。这是 #10548 中信任模型的那一半，刻意拆出来单独合入（ECS updater 白名单和免依赖 helper 车道留在原 PR）。这个拆分合理——机制本身独立于白名单扩展。 规模：不适用——未触及核心模块路径（全部改动在 方案：形态正确——在 风险：无升级风险信号——未命中高风险路径，作者是维护者。审查中值得持续盯住的一点：fail-closed 链条是整个安全故事，每一环（checkout → 分类 → 输出 → 消费端兜底）都需要保持显式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewThe implementation matches what I would have proposed, and I walked each link of the fail-closed chain it depends on:
No blockers, no convention violations. Tests are colocated and pin the load-bearing details rather than the trivia. Test evidence — the PR's own CI (unattended run; no PR code executed here)Check-runs fetched once for the reviewed commit: 10 success, 13 skipped, 4 in progress, none failing at fetch time.
Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
One honest limit: the fork-side behaviour change (a docs-only fork PR selecting Real-scenario testing: N/A — CI configuration with no user-visible product surface; the PR's own CI above is the evidence. 中文说明代码审查实现与我的独立方案一致，并逐环检查了它依赖的 fail-closed 链条：
无阻塞问题，无规范违反。测试与源码同目录放置，钉住的是关键细节而非琐碎内容。 测试证据——来自 PR 自己的 CI（无人值守运行；此处未执行任何 PR 代码）对审查提交抓取 check-runs：10 项成功、13 项跳过、4 项进行中，抓取时无失败。
一个诚实的边界：fork 侧的行为变化（只改文档的 fork PR 选出 真实场景测试：N/A——CI 配置改动，无用户可见的产品界面变化；上方 PR 自身 CI 即证据。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; the fail-closed chain is explicit, tested, and already live-verified by this PR's own CI. This is the rare CI PR where the safety story and the efficiency win point the same way. The approach is exactly the minimal one: classify once from a credential-less base-SHA checkout, publish a validated literal, let consumers read it, and make every degraded path land on The diff carries nothing beyond that goal: the What I verified against the PR's own CI on the reviewed commit: the new 中文说明置信度：5/5 —— 每个阶段都干净；fail-closed 链条显式、有测试覆盖，并且已由本 PR 自己的 CI 实跑验证。 这是一个安全收益和效率收益方向一致的少见的 CI PR。方案正是最小形态：从无凭据的 base SHA checkout 分类一次，发布校验过的字面量，消费端直接读取，所有降级路径都落在 diff 没有超出目标的内容： 对照本 PR 自己在审查提交上的 CI 验证：新的 — Qwen Code · qwen3.8-max Reviewed at |
Refuse to let actions/checkout reuse a trusted classifier directory that survived the shared-runner cleanup chain. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
commented
Sep 2, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
Author
There was a problem hiding this comment.
Found one fail-closed gap in the shared-runner cleanup path and fixed it in b67ae89. The trusted classifier checkout now aborts if its old directory survives cleanup, so actions/checkout cannot reuse a repository that may still contain hooks; the existing empty-output fallback then selects full CI.
Verified with the focused cleanup, no-AK wiring, and workflow-size suites, plus Prettier and git diff --check. No remaining findings in the reviewed scope.
Collaborator
Author
Follow-up test report (
|
yiliang114
enabled auto-merge
September 2, 2026 10:47
qwen-code-dev-bot
approved these changes
Sep 2, 2026
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head 530b118c.
- This closes a real trust-boundary hole: the profile classifier used to run from the PR-head checkout inside the
testjob, so a same-repo PR could rewrite the very script that decides whether CI executes, and the fork carve-out papered over only half of it. Moving classification intoclassify_pragainst an explicitgithub.event.pull_request.base.shasparse checkout (persist-credentials off, fetch-depth 1) means the classifier implementation always comes from the trusted base. - The surrounding hygiene is deliberate and correct: a hard stop if a
trusted-ci-classifierresidue exists before checkout (actions/checkout reuses origin-matching leftovers and runs git hooks inside them), the third sweep name added to all four copies of the cleanup, ownership restored first, and the downstream jobs consume a whitelist-validated output where empty/unknown values fall back tofull— double-checked both inclassify_prand at each consumer. - The same-repo gate's removal is safe with the new design: the classifier's inputs are the event-provided changed-file listing, and
docs_only/github_ci_onlyonly skip jobs — no fork code executes on the classifier path anymore. Pin tests lockbase.sha(and asserthead.shanever appears), the execution path, and the fallback whitelist. - No prior reviews or open threads; CI on this head has no failures (Test/review lanes running); per the channel convention the call is on the review itself.
qqqys
approved these changes
Sep 2, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head 530b118c. No prior blocking issues exist on this PR (the only earlier review artifacts are the triage pass at the prior head, the author's fail-closed fix note for b67ae8954f6, and an approval at this head), and my independent Critical-only pass over the full diff and the head's ci.yml finds none.
Trust boundary — verified at this head. Classification moves into classify_pr and runs exclusively from an explicit github.event.pull_request.base.sha sparse checkout of .github/scripts/ci (pinned actions/checkout, persist-credentials: false, fetch-depth: 1, job permissions contents: read + pull-requests: read), so the classifier implementation a PR is judged by always comes from the trusted base, never from a PR-head checkout; the base ref is event-provided and not author-controllable. The old per-job classifier (which is why fork PRs were forced to full) is gone from both consumers, and the consumers carry no GH_TOKEN at all.
Fail-closed chain — walked link by link. Producer normalizes to the three known literals before writing GITHUB_OUTPUT (unexpected output, exit 2, and any other non-zero all land on full); both consumers (test, integration_no_ak) re-validate against the same allowlist, default via ${TRUSTED_CI_PROFILE:-full}, and emit the distinct ::warning:: breadcrumb when the producer output is empty. integration_no_ak declares needs: 'classify_pr' (verified at head line 1465), so the output reference resolves; for non-pull_request events the checkout is event-guarded off and the profile stays full; a failed checkout or a failing guard leaves the output empty rather than partial. The residue guard (Verify trusted classifier checkout is clean) exits 1 between the sweep and the trusted checkout, closing the shared-pool reuse hole where actions/checkout would run git — hooks included — inside a leftover directory, and trusted-ci-classifier is added to every sweep copy (five in ci.yml plus the review workflow), kept honest by the byte-identity pin. The pin tests lock base.sha (asserting head.sha never appears), the permission block, the fallback breadcrumb, step position (cleanup < guard < checkout), and exercise both the guard's exit-1 and the sweep's removal of a hook-bearing residue.
No correctness, security, or regression issues found; the .size-baseline bump is mechanical.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x), review-pr and route are still pending, which does not gate this review per policy.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Post-merge review — merged 2026-09-02 (head Tier: Deep (trust-boundary change — CI profile classification moved to base-SHA sparse checkout to prevent fork-PR code injection) Scope: No blocking findings. What was checked and how it settled: Trust boundary — verified at head. Classification moves into Sparse checkout sufficiency — Fail-closed chain — all six paths from the PR description traced:
Step ordering — cleanup < guard < checkout. Verified in code and locked by the test in Sweep consistency — Ownership restore step — present and ordered before the sweep in all paths. The existence assertion added in this diff ( Cross-check against prior reviews: Two independent approvals at this head (qwen-code-dev-bot, qqqys) cover the same dimensions. Author identified and fixed one fail-closed gap during review (guard step added in Reviewed with AI assistance. |
wenshao
added a commit
that referenced
this pull request
Sep 2, 2026
Nine review findings, one root cause: the split copied test's behavior without copying its guards — every contract test that pins test's checkout, recovery, telemetry, budget and token surface enumerated jobs by name and never reached the copy. Contract extensions: the checkout-ref whole-literal pin now loops both jobs; no-ak-integration-ci's guardCalls gains a lint_and_static entry pinned to test's event-aware expected_sha shape; qwen-triage's unconditional-ownership-restore suite gains the new job; ci-disk-pressure pins the lane's sampler AND its collector — the collector was simply missing (the lane produced the #10035 telemetry and destroyed it with the runner temp dir on the exact ENOSPC death the sampler explains), now added with a per-job artifact name since upload-artifact v4+ rejects duplicates when both jobs fail in one run. New pins beside the gate-equality test: timeout stays the measured 45 (30 was the beheading value), and permissions stays exactly {contents: read} — the workflow grants checks/statuses write, and this lane runs contributor lifecycle scripts. Fidelity fix: the size gate moved back above the dependency install, the fail-fast position it deliberately held in the pre-split job — after npm ci it delivered its verdict ~6 minutes late warm, ~15 cold, and hid size violations behind unrelated install failures. An ordering assertion in workflow-size.test.js pins the position. Drift guard: the merge that landed mid-review proved the copies rot — #10799's trusted-classifier rework updated test's prelude and left the lint copy stale, resolved by hand. A shared-prelude equality test now holds every same-named step field-for-field identical (sole deliberate divergence: the collector's artifact name), so the next drift is a red test, not review archaeology. Prose: the three stale two-job narratives (test's header, HELPER_TESTS, the lanes test file) now describe the three-job reality. All nine mutations red: push-arm drop, verify-head drop, gate reordering, collector drop, ownership-restore drop, timeout lowered to 30, permissions deleted, permissions widened, one-character prelude drift.
wenshao
added a commit
to zhourrr/qwen-code
that referenced
this pull request
Sep 3, 2026
* ci: split lint and static checks out of the Test job The `test` job bundled two halves with opposite reliability profiles: twenty deterministic lint/static steps (ESLint, actionlint, shellcheck, yamllint, Prettier, schema/notices freshness, lockfile, workspace isolation, TUI dep direction, bundle closure, helper tests, workflow size) and the unit-test run, which was measured failing 41% of post-merge runs on flake alone this week. main still has zero required status checks — a duplicate-declaration compile error shipped to main on 09-01 and sat red for half an hour precisely because nothing gates merges — but the combined job could never become required while the flaky half holds the deterministic half hostage. Move the twenty static steps into a new `lint_and_static` job with the same event surface, the same runner routing, the same release-sync convention (run on skip_ci PRs, no-op per step, so a required context still reports), and its own copy of the checkout/node/install prelude plus the CI-profile classifier (the classifier is one gh api call; duplicating it costs seconds and keeps `test`'s `ci_profile` output — which web_shell_e2e_smoke consumes — untouched). The surgery is a pure move, proven at YAML level: the old job's 40 steps equal the new jobs' union exactly, every step is field-for-field identical to its original, every other job is byte-identical, and `lint_and_static.if` equals `test.if` — pinned by a new equality assertion that rides on test's whole-literal pin, so the two gates cannot diverge silently. Event-matrix A/B against main shows exactly one new row: lint_and_static running on push, pull_request, merge_group and workflow_dispatch, and not on schedule. Mutation-checked: a lint gate that silently drops push, or silently joins the nightly, each reddens two assertions. Scope disclosure: this lane could NOT have caught the 09-01 incident — that was an esbuild transform error inside a test file, ESLint accepts the duplicate binding (verified against the actual broken file), and web-shell's tsconfig excludes *.test.tsx from typecheck. Catching that class needs the unit-test lane, which can only become required once its flake rate is fixed. This split makes the half that CAN be required independent of that work. The size-baseline bump records the duplicated prelude comments; the growth is the split itself. * ci: give lint_and_static a cold-cache-sized timeout The first real run of the new lane was beheaded at exactly 30 minutes: a cold-cache pool runner paid 892s for npm install where the sibling Test job on the same run paid 347s, ESLint and Prettier both ran at ~2x their historical timings, and the job died mid bundle-closure. The 13-15 minute prediction in the split commit came from warm-cache step timings and did not price the first run on any given runner. 45 matches desktop_shell and still surfaces a wedged runner within the hour. * ci: give lint_and_static the contracts its template job already had Nine review findings, one root cause: the split copied test's behavior without copying its guards — every contract test that pins test's checkout, recovery, telemetry, budget and token surface enumerated jobs by name and never reached the copy. Contract extensions: the checkout-ref whole-literal pin now loops both jobs; no-ak-integration-ci's guardCalls gains a lint_and_static entry pinned to test's event-aware expected_sha shape; qwen-triage's unconditional-ownership-restore suite gains the new job; ci-disk-pressure pins the lane's sampler AND its collector — the collector was simply missing (the lane produced the QwenLM#10035 telemetry and destroyed it with the runner temp dir on the exact ENOSPC death the sampler explains), now added with a per-job artifact name since upload-artifact v4+ rejects duplicates when both jobs fail in one run. New pins beside the gate-equality test: timeout stays the measured 45 (30 was the beheading value), and permissions stays exactly {contents: read} — the workflow grants checks/statuses write, and this lane runs contributor lifecycle scripts. Fidelity fix: the size gate moved back above the dependency install, the fail-fast position it deliberately held in the pre-split job — after npm ci it delivered its verdict ~6 minutes late warm, ~15 cold, and hid size violations behind unrelated install failures. An ordering assertion in workflow-size.test.js pins the position. Drift guard: the merge that landed mid-review proved the copies rot — QwenLM#10799's trusted-classifier rework updated test's prelude and left the lint copy stale, resolved by hand. A shared-prelude equality test now holds every same-named step field-for-field identical (sole deliberate divergence: the collector's artifact name), so the next drift is a red test, not review archaeology. Prose: the three stale two-job narratives (test's header, HELPER_TESTS, the lanes test file) now describe the three-job reality. All nine mutations red: push-arm drop, verify-head drop, gate reordering, collector drop, ownership-restore drop, timeout lowered to 30, permissions deleted, permissions widened, one-character prelude drift. * ci: route lint_and_static's timeout like its siblings The flat 45 beheaded a second honest run: on cold-cache hk3 pool runners every step lands 2-4x its warm timing (install 940s vs 347s, bundle closure 681s vs 171s) and the serial sum cleared 45 minutes with no step wedged. The pool contention that sized test at 120/60 and no_ak at 60/30 applies here identically; a bare constant priced only the warm half of that reality. ECS 90 / hosted 45, same expression shape as test's, pins updated with both measurements. --------- Co-authored-by: 易良 <1204183885@qq.com>
yiliang114
added a commit
that referenced
this pull request
Sep 3, 2026
Resolve conflicts after #10756/#10799 moved the helper checks and the trusted classifier into lint_and_static on main: keep main's structure, keep this PR's HELPER_TESTS_DEP_FREE split so the github_ci_only lane runs only node-builtin-only suites before any dependency install, and record the merged ci.yml size in .size-baseline. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtlc9sozfg
Collaborator
|
Released in v0.23.0. |
yiliang114
added a commit
that referenced
this pull request
Sep 4, 2026
Resolve conflicts after #10799 landed the trusted-classifier half and the lint_and_static split moved the github_ci_only helper lane: - ci.yml env: keep HELPER_TESTS_DEP_FREE and add the e2e-build suite from main to both lists (node-builtin-only import closure). - ci.yml: adopt main's 'Verify trusted classifier checkout is clean' step. - ci.yml: drop the helper lane from the test job (main retired it there) and point lint_and_static's 'GitHub CI helper checks' at HELPER_TESTS_DEP_FREE, since setup-node/npm ci stay gated on the full profile and the lane runs without node_modules. - no-ak-integration-ci.test.js: keep the ECS suite pin. - .size-baseline: record the merged ci.yml size (131224 bytes). Verified locally: ci-platform-lanes, no-ak-integration-ci, workflow-size, and review-worktree-cleanup-workflow suites pass (261 passed, 10 platform skips); classify-profile and update-ecs-runner-qwen-workflow node:test suites pass; check-workflow-size.sh passes. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtmla619hx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
CI profile classification (docs_only / github_ci_only / full) moves out of the product jobs and into the classify_pr job. That job checks out only the classifier directory from the pull request's base commit into a separate path, with no persisted credentials, runs the classifier once, validates the result against the three known profiles, and publishes it as a job output. The Ubuntu test job and the no-AK integration gate read that output instead of running a classifier from their own checkout, and treat a missing or unknown value as full. The classifier checkout path is added to the pre-checkout sweep in every shared-pool job so a leftover copy from an earlier run is never reused by actions/checkout.
This is the trust-model half of #10548, extracted so it can land on its own. The ECS updater allowlist and the dependency-free helper lane stay in that PR.
Why it's needed
Today the classifier runs from the PR checkout, so it is only trusted for same-repo pull requests and every fork PR is forced to full. Running the base-SHA copy removes the trust problem instead of working around it: the code that decides which tests to skip can no longer be changed by the PR it is evaluating, fork PRs can be classified with the ordinary read-only event token, and no collaborator-permission lookup is needed. Every failure path (checkout skipped, listing failed, classifier crashed, unexpected output, job output empty) still selects full.
Reviewer Test Plan
How to verify
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/no-ak-integration-ci.test.js scripts/tests/review-worktree-cleanup-workflow.test.js scripts/tests/qwen-pr-review-workflow.test.js scripts/tests/ci-platform-lanes.test.jspasses. Two cases in the review-workflow suite fail when run as root on any branch, including main, because they rely on an unwritable directory; they are unrelated.node --testover the classifier helper suites passes (47 tests).github.event.pull_request.base.sha, never from the head; both consumer steps only readneeds.classify_pr.outputs.ci_profile; each of the five sweep copies in ci.yml and the one in the review workflow lists the classifier path.Evidence (Before & After)
N/A (CI configuration). Before: fork PRs always log "Fork PR detected; running full CI." After: the classify_pr log shows "Selected trusted CI profile: " and the consumers log the same value.
Tested on
Risk & Scope
Linked Issues
Extracted from #10548.
中文说明
这个 PR 做了什么
CI profile 的分类（docs_only / github_ci_only / full）从产品 job 移到 classify_pr job。该 job 只把分类器目录从 PR 的 base 提交 sparse checkout 到一个独立路径，不持久化凭据，运行一次分类器，按三个已知 profile 校验结果，并作为 job 输出发布。Ubuntu 测试 job 和 no-AK 集成门禁改为读取该输出，不再从自己的 checkout 执行分类器，输出缺失或未知时按 full 处理。分类器的 checkout 路径加入所有共享池 job 的 checkout 前清理列表，避免 actions/checkout 复用上一次运行的残留副本。
这是 #10548 中信任模型的那一半，单独拆出来以便独立合入。ECS updater 白名单和免依赖 helper 车道仍留在原 PR。
为什么需要
目前分类器从 PR checkout 执行，因此只对同仓库 PR 可信，所有 fork PR 都被强制 full。改为执行 base SHA 的副本后，问题本身被消除而不是绕开：决定跳过哪些测试的代码不再可能被它所评估的 PR 修改，fork PR 可以用普通只读 event token 分类，也不再需要协作者权限查询。所有失败路径（checkout 跳过、列文件失败、分类器崩溃、输出异常、job 输出为空）仍然选择 full。
评审验证方案
如何验证
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/no-ak-integration-ci.test.js scripts/tests/review-worktree-cleanup-workflow.test.js scripts/tests/qwen-pr-review-workflow.test.js scripts/tests/ci-platform-lanes.test.js通过。review-workflow 套件里有两个用例以 root 身份运行时在任何分支（包括 main）都会失败，因为它们依赖一个不可写目录，与本改动无关。node --test通过（47 个用例）。github.event.pull_request.base.sha而非 head checkout 分类器；两个消费步骤只读取needs.classify_pr.outputs.ci_profile；ci.yml 的五处清理副本和 review workflow 的一处都列出了分类器路径。前后证据
N/A（CI 配置）。之前：fork PR 总是打印 "Fork PR detected; running full CI."。之后：classify_pr 日志显示 "Selected trusted CI profile: "，消费端打印相同的值。
测试平台
风险与范围
关联 Issue
拆自 #10548。
https://claude.ai/code/session_01AWWgJEqafyAT1Mc75T8N7h