Repository navigation
fix(ci): refuse a partial jobs page list before reporting - #15174
Conversation
`--paginate` prints one document per page and the step merges them with `jq -s`, so a page 2 failure leaves a list holding only page 1. The exit status cannot tell that apart from a complete read, and the partial file reads exactly like a whole run to the failed-job projections and to the infra classifier — which counts only the jobs present, turning a regression parked on the missing page into a rerun instead of an issue. Stage the merge in a temp file and promote it only when the merged job count matches the payload's own total_count; otherwise report without the job list. The receipt keeps the guard independent of the shell's pipefail setting, which the meta-test now exercises by spawning the step without it.
Verification reportScope: the partial-pagination finding only. The destination-lookup finding in #15138 is untouched. Method: step-level harness. The workflow's Before the fix
After the fix
Repository tests
Notes
|
|
|
|
I tested the exact new The new condition is: ([.[].total_count] | max) as $total
| if $total == null or ($doc.jobs | length) == $total then $doc
else error("listed ...") endWhen none of the emitted page objects carries printf '%s\n' \
'{"jobs":[{"id":9001,"conclusion":"failure"}]}' \
'{"message":"API rate limit exceeded"}' |
jq -sc '{jobs: [ .[].jobs[]? ]} as $doc | ([.[].total_count] | max) as $total | if $total == null or ($doc.jobs | length) == $total then $doc else error("incomplete") end'
# ACTUAL: {"jobs":[{"id":9001,"conclusion":"failure"}]}
# Expected under the stated receipt contract: error -> delete stage, publish {"jobs":[]}, emit warningThis is an isolated jq fixture, not a claim that the real GitHub jobs endpoint normally omits Bounded fix: require a finite/integer nonnegative |
|
Thanks for the PR! Template looks good ✓ — every required heading is there, the Tested-on table is filled in honestly (macOS ✅ / Windows N/A / Linux Problem: observed and source-verified, with one honest caveat. #15138 is open and records this as finding #2 out of @doudouOUC's post-merge review of #14522, acceptance test included. Reading the base code, the defect is actually slightly worse than the issue describes: Direction: aligned. This is the repo's own failure-reporting infrastructure, and the scoping is disciplined — it takes one of the issue's two findings and explicitly leaves the other (the destination-lookup budget walk) open for a maintainer decision instead of bundling both. CHANGELOG: no direct reference, which is expected for a workflow-only change. Size: core paths are not touched. Production lines 28 ( Approach: the scope feels right and I would not cut anything. The remedy is the one the issue prescribed — atomic staging plus a Risk: no elevated risk signals — neither file matches the revert-correlated paths. One note on timing, in the interest of transparency: the head moved while this review was running. I had started against Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需小节齐全，测试平台表格如实填写（macOS ✅ / Windows N/A / Linux 问题：已观测且经源码验证，但有一点需要如实说明。#15138 处于 open 状态，将本项记录为 @doudouOUC 对 #14522 合并后评审中的第 2 项发现，并附了验收测试。阅读基线代码后可以确认，缺陷实际比 issue 描述的更严重一点： 方向：对齐。这是仓库自身的失败上报基础设施，范围划分克制 —— 只处理该 issue 两项发现中的一项，并明确把另一项（目标发现的预算遍历）留给维护者决策，没有打包在一起。CHANGELOG：无直接关联，对纯 workflow 改动而言属于预期。 规模：未触及核心路径。生产代码 28 行（ 方案：范围合理，我不会砍任何部分。采用的正是 issue 所指定的做法 —— 原子暂存加 风险：无升级风险信号 —— 两个文件都不匹配与回滚相关的路径。 关于时间点，为透明起见说明一下：审查进行期间 head 发生了变化。我最初是针对 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewThe fix is correct. I checked the load-bearing claims against the source and the live API rather than taking them from the description. Verified
One suggestion, non-blocking. The new comment says a run that yields no receipt "falls back to the empty list the per-commit body already tolerates, and says so" — but the Not verified: whether CI test evidenceQuoted from the current head's own check-runs via the API. No PR code was built or executed. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Nothing is red on this head. That is a change from when I started: on the pre-merge head That failure was not this PR's code. The lane died in I classified that as staleness from the diff and the commit graph, not from anything the log asserts: the branch head was 1 ahead of and 29 behind The consequence is the part that matters for reading this review. Sandboxed verification would settle the remaining gap: 中文说明代码审查 修复是正确的。关键结论我是对照源码和线上 API 验证的，没有直接采信 PR 描述。 已验证：
一条非阻塞建议：新注释说没有凭据时"回退到 per-commit 正文已能容忍的空列表，并作出说明"，但 未验证： CI 测试证据 以上表格取自当前 head 自身的 check-runs（经 API 获取），未构建或执行任何 PR 代码。表格由标记区域包裹，CI 结束后可原地更新。 这个 head 上没有红色项。与我开始审查时相比这是一个变化：在合并前的 head 那次失败不是本 PR 代码导致的。该泳道在 对阅读本审查真正重要的是其后果： 沙箱验证可以补上剩余缺口： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the fix is correct, well-scoped and fails safe; the two reservations are non-blocking, and the only thing outstanding is CI that has not finished running. My independent read before looking at the diff was: stage the merged pages somewhere the consumers cannot see, and promote only against a completeness receipt taken from the response itself, because this workflow runs What I most like is the discipline around it. #15138 carries two findings; this PR takes one, says so plainly, and leaves the other open for a maintainer decision rather than bundling a larger change. The test has a positive control alongside the negative case, and I confirmed it is load-bearing — strip the guard and it goes red. The failure direction is safe everywhere in this change: the worst case is a report that names no job, never a report that does not happen. My reservations, neither blocking. The comment about the no-receipt case overstates what the code does — it says the reporter "says so", and on the The honest caveat on necessity: the bug cannot fire today. No watched workflow has run above 13 jobs, so nothing paginates and the partial-page path is unreachable. This is a correct fix for a dormant defect. I would still take it — 28 production lines, fail-safe, and it closes out a documented finding from a post-merge review — and the day a lane grows past 100 jobs is precisely the day you do not want to discover the reporter had been silently truncating. But if the bar is "fix what is currently firing", parking it is a defensible call, and that is a maintainer's judgement rather than mine. Not approving in this run, for a mechanical reason only. Three lanes are still in progress on this head — including Worth recording for whoever reads the thread later: the head moved mid-review. I had started against @wenshao no action needed from you to unblock CI — the merge already cleared the freshness gate. The one thing I would consider changing is the "and says so" clause in the new comment, since the 中文说明置信度：4/5 —— 修复正确、范围克制、失败方向安全；两条保留意见均不阻塞，唯一未完成的是尚未跑完的 CI。 在看 diff 之前我自己的判断是：把合并后的分页结果暂存到消费方看不到的位置，只有对照响应自身携带的完整性凭据校验通过才采用，因为这个 workflow 以 最欣赏的是它的克制。#15138 包含两项发现；本 PR 只取其一，明确说明，并把另一项留给维护者决策，没有打包成更大的改动。测试在负向用例外还配了正向对照，而且我确认它是真正起作用的 —— 去掉守卫就会变红。整个改动的失败方向都是安全的：最坏情况是上报中不点名任何 job，绝不会是上报没有发生。 我的保留意见，两条都不阻塞。关于"无凭据"分支的注释夸大了代码的实际行为 —— 它说上报器"会作出说明"，而 关于必要性，需要如实说明：这个 bug 今天无法触发。被监听的 workflow 没有一次超过 13 个 job，因此不会分页，部分页路径不可达。这是对一处休眠缺陷的正确修复。我仍然倾向于接受 —— 28 行生产代码、失败方向安全，并且落实了合并后评审中一项有记录的发现 —— 而且某个泳道增长到超过 100 个 job 的那一天，恰恰是最不希望发现上报器一直在静默截断的时候。但如果标准是"只修当前正在发生的问题"，把它搁置也是站得住脚的，那是维护者的判断，不是我的。 本次运行不予批准，原因纯属机制层面。这个 head 上有三个泳道仍在进行中 —— 包括 有一点值得记录，方便后来阅读该 thread 的人：审查期间 head 发生了变化。我最初针对 @wenshao 你不需要为解除 CI 阻塞做任何事 —— 那次合并已经清除了新鲜度门禁。我唯一会考虑修改的是新注释里"并作出说明"那半句，因为 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Nakagawa-master
left a comment
Nakagawa-master
left a comment
There was a problem hiding this comment.
Current 9b9bd5e blocking inverse to the fail-closed contract, independently reproduced with jq 1.7 (not a hypothetical shell-status issue).
The production predicate in .github/workflows/main-ci-failure-issue.yml currently reads:
if $total == null or ($doc.jobs | length) == $total then $doc
else error("listed ...") endBut null is lack of a completeness receipt, and the or arm promotes the partial, unverifiable page body as a complete job list. Example exact reproducer:
printf '%s\n' \
'{"jobs":[{"id":9001,"conclusion":"failure"}]}' \
'{"message":"API rate limit exceeded"}' |
jq -sc '{jobs: [ .[].jobs[]? ]} as $doc | ([.[].total_count] | max) as $total | if $total == null or ($doc.jobs | length) == $total then $doc else error("listed \($doc.jobs | length) of \($total) jobs") end'
# exit 0: {"jobs":[{"id":9001,"conclusion":"failure"}]}Confirmed in a real local jq-1.7 run. This contradicts the PR's own explanatory contract, "a run that yields no receipt at all falls back to the empty list," and its new test only covers a non-null total_count:2 truncation. It is a materially distinct negative test input.
Smallest conservative source fix:
if $total != null and ($doc.jobs | length) == $total
then $doc
else error("missing or incomplete total_count receipt")
endPlease add a real spawnSync negative case where the gh stub emits a populated jobs response without total_count followed by an API error body, asserting run-jobs.json is {"jobs":[]}, the staging file is absent, and the workflow warning is emitted. Then a complete real-paged response remains the positive control.
I saw the automated Stage 2 comment call this fix fail-safe; the above executable inverse means that judgment is too broad at this head, regardless of other passing CI. This review addresses only this exact missing-receipt branch; it does not allege a production incident or claim ownership of the receiving author's implementation.
`$total == null or …` treated a response that carries no `total_count` as complete, so a populated first page followed by an API error published its partial list — the inverse of the contract the step's own comment states, reproduced with jq: the predicate exits 0 and emits page 1 alone. Make the receipt part of the pass condition: a page set is promoted only when it declares a total and the merged jobs match it. No total means no proof of completeness, so it takes the same fallback as a failed fetch — empty list, staging file removed, warning. The new negative case feeds a populated page without `total_count` and asserts that fallback.
|
Thanks — the reproducer holds, and it is fixed in You are right that this was an inverse of the contract rather than a hypothetical: if $total != null and ($doc.jobs | length) == $total then $doc
else error("incomplete job list: merged \($doc.jobs | length) of \($total)") endA response with no receipt therefore takes the same fallback as a failed fetch: staging file removed, Your requested negative case is in the suite: the step is spawned under One correction for the record, because it changes how the original gap reads: On the two red checks on the previous head: |
Nakagawa-master
left a comment
Nakagawa-master
left a comment
There was a problem hiding this comment.
Follow-up to my earlier CHANGES_REQUESTED review on 9b9bd5e. I reviewed the actual new head ef371fd06f34672adcacce42e3ca2032c8d96790 and the two changed files, together with @wenshao's explicit reproducer acknowledgment.
The blocking no-total_count inverse has been corrected. The workflow now requires $total != null and ($doc.jobs | length) == $total before promoting the staged JSON. The new noReceipt negative fixture asserts empty fallback, warning, and staging cleanup; the incomplete second-page negative and full two-page positive are also present. That addresses the specific source-level condition on which my prior request for changes was based. The author owns the correction and tests; my earlier review supplied the counterexample, not the code.
Review scope / CI distinction: On this head, Ubuntu Node Test, Lint & Static and no-AK Integration checks have passed. web-shell E2E Smoke is currently FAILED (the native job log repeatedly shows ECONNREFUSED 127.0.0.1:4170 and multiple browser visibility timeouts); a separate review-pr check remains in progress. This narrow workflow/test approval is not a claim that the PR has all checks green, that the smoke failure is known to be unrelated, or that the implementation has been merged or exercised in production. Maintainers should make the independent CI and merge decisions from exact-head data. The attempt-scoped semantics of total_count on retried runs remain an honest nonblocking unknown, not a reason to reinstate the proven inverse.
I approve the current fix for the specific issue I blocked. Thanks for turning a concrete negative case into receiver-owned tested code.
|
评审提交：
CI 状态单独说明：该 head 的 Ubuntu Test、Lint & Static、no-AK Integration 已通过；Web Shell E2E Smoke 仍失败，日志包含反复的 |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": whether the test_macos leg's npm run test:ci step (ci.yml:1705, 1792) is fatal or warning-downgraded the way test_windows is (ci.yml:1974-1985 captures S…; "agent 1a": none — I stopped with budget remaining, not at the ceiling..
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/main-ci-failure-issue.yml — actionlint embedded-shell source mapping is not yet supported.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：whether the test_macos leg's npm run test:ci step (ci.yml:1705, 1792) is fatal or warning-downgraded the way test_windows is (ci.yml:1974-1985 captures S…；"agent 1a"：none — I stopped with budget remaining, not at the ceiling.。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
未检查（工具限制，非阻断）：.github/workflows/main-ci-failure-issue.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
— qwen3.8-max via Qwen Code /review (v0.26.0)
The reporter degraded to an empty list with only an annotation on its own job, so the PR comment and the issue looked exactly like a run with no identifiable failure — the readers who need the signal never saw it. Carry a jobs_complete output onto both bodies: when it is false they say the report may name fewer failing jobs than the run had, and the issue body guards the sentence so a recurrence does not stack a second copy. Also widen the meta-test pin back over the fetch's producer, so --paginate and per_page=100 stay pinned: the earlier rewrite started the span at the merge and left both unpinned while the comment still claimed they rode it.
The case now rejects both a truncated page set and a response carrying no total_count, so a name that mentions only "stops short of total_count" understates it. The flag's comment said "both reports" where the two readers are the PR comment and the issue body.
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 no bounded retry on the all-or-nothing fetch — still stands, author declined with a documented rationale (PR description, Risk & Scope); already reported (comment 4348575273)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": could not execute the vitest suite — the review worktree has no node_modules ( ls node_modules/.bin/vitest → not found), so the new test's own greenness is u…; "agent reverse-audit (round 1)": did not independently re-verify the helper's plan --existing merge ( splitOccurrenceBlock tail preservation) that the re-plan fixture at test:1096 depends on…; "agent reverse-audit (round 2)": could not execute scripts/tests/main-ci-failure-issue-workflow.test.js — the review worktree has no installed node_modules ( npx vitest fails with Cannot ….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/main-ci-failure-issue.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
scripts/tests/main-ci-failure-issue-workflow.test.js:624 — [probe] the receipt harness spawns without production's -o pipefail and no fixture covers gh-exits-0-short, so the receipt's only witness is the harness's own shell
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：could not execute the vitest suite — the review worktree has no node_modules ( ls node_modules/.bin/vitest → not found), so the new test's own greenness is u…；"agent reverse-audit (round 1)"：did not independently re-verify the helper's plan --existing merge ( splitOccurrenceBlock tail preservation) that the re-plan fixture at test:1096 depends on…；"agent reverse-audit (round 2)"：could not execute scripts/tests/main-ci-failure-issue-workflow.test.js — the review worktree has no installed node_modules ( npx vitest fails with Cannot …。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：.github/workflows/main-ci-failure-issue.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.26.0)
|
Exact-head CI readback for maintainer review (2026-10-23 JST; no new code request). I rechecked OPEN #15174 at The final Playwright summary matters more than the first-attempt failure list: 343 passed, 10 flaky (passed on retry), 3 failed. The final failures were Compare This is a new exact-HEAD browser/CI classification only; my earlier missing- |
The note added by the previous commit was append-only: once one degraded run wrote it into a tracking issue, every later replan preserved it -- including runs whose job list was read in full and receipt-certified -- so a rebuilt failure block sat under a permanent claim that the list might be short, and the substring guard then refused a later degraded run a fresh note of its own. Strip the exact sentence before deciding whether to re-add it, so a certified run clears a stale copy and a degraded run speaks for itself. Pin the producer-to-consumer wiring (step id and both env mirrors), add the plan path's negative control, and cover gh exiting 0 with a short list.
main's #15256 replaced the signature-based issue planner with a workflow-identity aggregate that comments the failure summary onto one issue, so this resolution carries the branch's receipt and flag into the new shape: - the download step keeps the total_count receipt, the atomic promotion and the jobs_complete output; - the summary step keeps the JOBS_COMPLETE note, which now reaches both the PR comment and the issue record, because the issue job publishes comment_body; - the plan-step note and its strip-then-append guard go away with the step itself: the aggregate posts a fresh comment per run, so a stale copy cannot accumulate.
Maintainer local verification — real GitHub API + fault injectionVerified at head What was run
Before / after (page 1 declares 2 jobs; page 2 is the regression)
The no-receipt row is the one the first revision missed; the exit-0-short row is the one the exit status cannot see. Both are rejected by the receipt alone. Scope note on the receipt vs.
|
| Check | Result |
|---|---|
| meta-test | head 76/76, base 75/75 |
actionlint (CI invocation) |
head exit 0, base exit 0 |
| workflow size ratchet | head pass, base pass |
| prettier / eslint | changed files clean / 0 errors |
M1 $total == null or … |
suite 2 failed; cell publishes 1 job and jobs_complete=true |
M2 drop --paginate |
suite 1 failed; cell 0 jobs / false |
M3 jobs_complete=true on fallback |
suite 2 failed; flag true, note absent |
| M4 delete the summary sentence | suite 1 failed; note absent |
M5 delete the jobs_complete output |
suite 1 failed; flag null, note appears on a complete list |
Every mutation was applied to the head worktree, measured, and restored with git checkout HEAD --; the tree was clean and at 2e77947c03 after each. The five mutations are all pinned by the PR's own suite, and four of them are also visible at the published-body level.
Evidence
Artifacts: tmp/pr15174-verify-20261023-145810/ (harness, per-cell JSON, real-run logs, mutation diffs).
中文摘要
维护者本地真实环境验证
head 2e77947c03，base 69df02bb9a。结论：可以合并，无阻塞问题。
- 真实视野：
gh2.103.0 直接打api.github.com，run39908521630（13 个 job）。用per_page=1让真实分页产生 13 个 JSON 文档，每页都带total_count: 13，合并13/13；两臂的抓取步骤都保留了全部 13 个 job，head 报jobs_complete=true。不存在的 attempt 返回真实的 404 错误体（stdout + exit 1），head 落回{"jobs":[]}+ 警告 +false。 - 中途 403：让真实 gh 通过本地 HTTPS 仿 GitHub 走真实
Link分页并在第 2 页收到 403，不是手写 gh 桩。 - 整条 analyze 链：逐字执行
Download failed job logs→Detect infrastructure-only failures→Build the PR failure summary，日志用 main 上真实失败 job 的日志，所以表里的infra_only和正文就是下游实际看到的东西。 - 前后对比：第 2 页 403 时，base 发布 1/2 个 job、
infra_only=true、无说明 → 只会重跑、不建 issue；head 发布 0/2、false、带说明 → 建 issue。gh退出 0 但列表偏短时，base 连警告都没有；head 用total_count凭据拒绝。完整两页、零 job、首页 403 均符合预期。 - no-pipefail 对照：base 仍是 1/2 +
true+ 无说明；head 0/2 +false+ 说明——拒绝来自凭据，不是 shell 选项。 - 门禁：head 元测试 76/76，base 75/75；actionlint 两臂 0；体积棘轮两臂通过；prettier/eslint 干净。
- 变异：M1 去掉「必须声明总数」、M2 删
--paginate、M3 兜底置true、M4 删说明句、M5 删jobs_complete输出——套件分别红 2/1/2/1/1 条，对应 cell 也红；每次都用git checkout HEAD --还原并确认树干净。 - 范围说明：Actions 的
bash -eo pipefail下，page-2 403 本来就会因 gh 非零退出走兜底，所以凭据真正独立承重的是「退出 0 但偏短」「缺 total_count」以及元测试的 no-pipefail 生成方式。这不是缺陷，是说明。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed head 2e77947c0380caf4aaa544ef4e6a7256b17c4f97. Two files: .github/workflows/main-ci-failure-issue.yml (+27/-3) and scripts/tests/main-ci-failure-issue-workflow.test.js (+154/-9). I read the whole production hunk and the workflow's downstream consumers at head.
No Critical findings. Both blocking items from earlier rounds are resolved at this head, and I verified each in the current file rather than from the disposition replies.
The fail-open receipt inverse is fixed, and pinned
The blocking review reproduced that if $total == null or ($doc.jobs | length) == $total then $doc promotes an uncertifiable page body: null is the absence of a completeness receipt, and the or arm treated it as a licence. At head the predicate is the conservative form that review asked for:
if $total != null and ($doc.jobs | length) == $total then $doc
else error("incomplete job list: merged \($doc.jobs | length) of \($total)") end
All three shapes now fail closed. A short merge (total_count: 2, one page's jobs) errors; an all-pages-failed response carries no total_count, so max is null and the != null conjunct errors; only a fully accounted list is promoted. The promotion is staged — jq writes ${jobs_json}.tmp, and mv replaces run-jobs.json only on success, while the failure branch does rm -f "${jobs_tmp}" and writes an explicit {"jobs":[]}. That matters on a reused self-hosted runner, where a previous attempt's run-jobs.json would otherwise survive into this one; overwriting it with a certified-empty list is what keeps the infra classifier from reading stale jobs.
The tests pin the producer and the predicate together, not just the predicate: the assertion span starts at gh api "repos/${REPO}/actions/runs/${WORKFLOW_RUN_ID}/attempts/${RUN_ATTEMPT}/jobs?per_page=100" --paginate and runs through > "${jobs_tmp}"; then mv "${jobs_tmp}" "${jobs_json}", so dropping --paginate or per_page=100 reddens the suite. refuses a job list it cannot certify complete spawns the real receipt expression against an executable gh stub on PATH and asserts the resulting run-jobs.json, that the staging file is absent, and the warning text; the no-total_count case is there as its own input, and a complete run asserts stdout carries no warning as the positive control. That is the negative case the blocking review specified.
The stated reason for the change is the part worth recording: a partial list is indistinguishable from a complete one to every consumer, including the infra classifier, which would rerun a regression parked on the missing page instead of filing it. jobs_complete is exported as a step output and both human-facing bodies read it, so a short report says so.
The append-only degradation note is gone with the mechanism that made it append-only
The fix-induced Critical was that the degradation sentence, once written into a tracking issue's human-owned tail, was re-emitted by every --existing merge and could never be retracted by a later clean run. That mechanism no longer exists at this head:
- The sentence is generated per run inside the
analyzejob fromJOBS_COMPLETE: '${{ steps.download.outputs.jobs_complete }}'(:287,:304-305) and lands insteps.summary.outputs.body, published ascomment_body(:63). - The PR path (
:479-500) writes that body to a fresh file and upserts a comment keyed by a per-run marker,<!-- qwen-managed-agent-ci-failure:${WORKFLOW_RUN_ID} -->, so a run updates its own comment and never appends to another run's text. - The issue path (
:364-377) is "Create or comment on the workflow issue" against the aggregate marker<!-- main-ci-failure-aggregate:${WORKFLOW_ID} -->. There is no--existingand no planner step anywhere in the head file — the strip-then-append merge the finding was about left with the upstream aggregate change.
So a later run that reads the full job list simply omits the sentence; nothing persists it forward. The retraction property the finding demanded now comes from the publication model rather than from a guard.
Non-blocking items, recorded not reopened
R1-2 (no bounded retry on the all-or-nothing fetch) stands as declined with a written rationale: the failures this branch sees are 403/429/5xx, where an immediate retry repeats the same walk inside the same rate-limit window. R1-1, R1-7-annotation-only, R2-1 (producer→consumer wiring pin) and R2-2 (the != 'true' negative control) are all implemented at head. The deferred [probe] about the receipt harness spawning without -o pipefail does not weaken the receipt: the pipeline's exit status comes from jq, the last element, and jq is what errors on an uncertified merge — so pipefail is not what makes the gate fail closed. Worth noting for the record that reviewDecision still reads CHANGES_REQUESTED because the automated review at 9f18c4661b was never dismissed, even though the Critical backing it is resolved here.
CI
gh pr checks at this head shows no failures and nothing pending among the lanes that run: Test (ubuntu-latest, Node 22.x) (48m30s, the lane that executes the changed test file), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, Managed Agent gate, Select the Managed Agent tier, Classify PR, route, label, assign and Remind on force-push all pass. The Java, native, distribution and parity lanes are skipped for this diff, which touches only a workflow file and a script test. An earlier "deferred approval withheld" note belongs to the superseded head 9b9bd5e.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The receipt gate is fail-closed, the four pagination scenarios are covered with distinct fixtures (truncated page, no total_count, gh-exits-0-short, and the positive control), and the summary degradation test covers both sides of the flag. The wiring pin (step id + JOBS_COMPLETE binding) protects against drift.
Previously raised concerns on earlier heads are addressed: the no-total_count inverse was corrected (Nakagawa-master confirmed on ef371fd), the append-only concern is moot after upstream replaced the issue writer with per-run comments, and R2-1/R2-2 from the bot's review on 9f18c46 now have matching assertions and negative controls on this head.
|
Released in v0.26.1. |
What this PR does
The main-branch failure reporter only publishes a job list it can prove it finished reading, and it tells its readers when it could not. The paginated fetch is merged into a staging file and promoted only when the jobs it holds account for the total the response itself declares; a response carrying no total cannot be certified either, so it takes the same fallback as a failed page: the staging file is removed, an empty list is published, and a warning is emitted. The summary step then adds one sentence when the list was short. Because the issue writer now publishes that same summary body, one flag covers both surfaces — the PR comment and the issue record — and since each occurrence posts a fresh body, the sentence is always about the run it sits in and can never go stale.
Why it's needed
When a run has more than one page of jobs and a later page fails, the merge keeps whichever pages arrived. The reporter then holds a list that looks complete: the failing-job count is plausible, the summary names real jobs, and nothing downstream can tell it apart from a full read. The infrastructure classifier consumes that same list and counts only the jobs present, so a genuine regression sitting on a missing page could be classified as a runner problem — the run is rerun, the issue is suppressed, and the rerun's own green result means nothing re-enters the reporter to file it. Two follow-on defects surfaced during review: the first version of the receipt treated a missing total as nothing to check, which published a populated page as the whole run, and the degradation note was append-only into a shared issue body. Upstream has since replaced that issue planner with a workflow-identity aggregate that posts a fresh comment per run, so the append-only mechanism is gone with it; this branch carries only the receipt and the per-run sentence.
Reviewer Test Plan
How to verify
Run the failure-reporter meta-test; it executes the step bodies under a fake
ghthat emulates paginated responses. Four pagination cases matter: a later page fails with a declared total, a later page fails with no declared total,ghexits 0 while carrying fewer jobs than it declares, and both pages arrive intact. The first three must publish{"jobs":[]}, leave no staging file, emit the warning, and reportjobs_complete=false; the last must publish both jobs, reportjobs_complete=true, and stay silent. The summary step is then spawned twice on the same fixtures: withjobs_complete=falseits body must carry the "could not be read in full" sentence, withtrueit must not. A pin keeps the producer'sjobs_completeoutput and the consumer'sJOBS_COMPLETEbinding from drifting apart.To see each guard rather than take it on trust, apply the mutation named in the evidence table and confirm the suite turns red on that mutation alone.
Evidence (Before & After)
Behaviour rows were measured on the step-level harness against the base for the before column; every mutation, lint and size row was measured at the commit named in its row.
gh: page 1 of 150 jobs, page 2 answers 403infra_only=true; after: empty list, warning,infra_only=false, issue filed461be87/ef371fdtotal_countgh: populated page, then an API error body{"jobs":[]}, staging file removed, warning emittedef371fdgh: two pages, 150 jobsef371fdghexits 0 but shortgh: one page declaringtotal_count: 2, carrying one job, exit 02e77947c03npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/main-ci-failure-issue-workflow.test.js2e77947c03--paginatefrom the workflowAssertionError: expected 'log_dir="${RUNNER_TEMP}/failed-logs" …' to contain 'gh api "repos/${REPO}/actions/runs/${…'2e77947c03jobs_complete=trueon the fallback pathAssertionError: expected 'true' to be 'false'2e77947c03AssertionError: expected 'body<<QWEN_CI_COMMENT_…' to contain 'The job list could not be read in full'2e77947c03id: 'download'toid: 'fetch_logs'AssertionError: expected undefined to be truthy2e77947c03prettier --check,eslinton the changed files2e77947c03actionlint -shellcheck= -ignore 'SC2016:' …(the flags CI passes)2e77947c03.github/scripts/check-workflow-size.sh2e77947c03Tested on
macOS ran the suites locally with bash 5 on
PATH, because the step body uses a builtin the system bash 3.2 lacks; Linux is where CI runs them and was not executed locally.Environment (optional)
Local unit tests only; no sandbox, no CLI session. The harness spawns the workflow's step bodies verbatim with a fake
gh.Risk & Scope
main; the upstream restructure of the issue writer means the earlier plan-step note logic is no longer present, and the summary body it replaces is published per run. CI note:web-shell E2E Smokehas been red across recent heads with failures in specs this PR does not touch (at9f18c4661b: 343 passed, 10 flaky, 3 failed inmanaged-context-compression,web-shell.browser-notificationsandweb-shell.excel-preview), whileTest,Lint & Static,Integration TestsandManaged Agent gatepassed on the same head. Each push re-runs the suite.Linked Issues
Refs #15138 — this addresses the partial-pagination finding only. The destination-lookup finding in the same issue is untouched and remains open for discussion.
中文说明
本 PR 做了什么
main 分支的失败上报器只发布它能证明自己读完的 job 列表，并在读不全时明确告诉读者。分页抓取结果先合并进暂存文件，只有当其中 job 数与响应自身声明的总数相符时才替换正式文件；响应若根本没有声明总数，同样不能被认定为完整，因此走与「某一页失败」相同的兜底：删除暂存文件、发布空列表、发出警告。列表偏短时，summary 步骤追加一句说明。由于 issue 写入现在直接发布这份 summary 正文，一个标记就同时覆盖两个出口 —— PR 评论与 issue 记录 —— 且每次发生都以新正文发布，这句话永远描述它所在的那次运行，不会过期。
为什么需要
当一次运行有多页 job 而后面某一页失败时，合并步骤会保留已经到达的那几页。上报器于是持有一份看起来完整的列表：失败 job 数量看似合理、摘要里点名了真实 job，下游没有任何东西能把它与完整读取区分开。基础设施分类器读的是同一份列表，只统计其中存在的 job，因此落在缺失页上的真实回归可能被判成 runner 问题 —— 运行被重跑、issue 被抑制，而重跑自身变绿意味着再没有任何东西会重新进入上报器去建这个 issue。评审过程中还暴露出两个次生缺陷：凭据的第一版把「缺少总数」当成「无需校验」，会把一页数据发布成整次运行；降级说明则是「只增不删」地写进共享的 issue 正文。上游随后把该 issue 规划器换成了按 workflow 身份聚合、每次运行发布一条新评论的写入器，因此「只增不删」的机制随之一并消失；本分支只保留凭据与「按次」的说明句。
评审测试计划
如何验证
运行失败上报器的元测试；它用假
gh模拟分页响应并逐字执行步骤脚本。四个分页用例最关键：后续页失败且声明了总数、后续页失败但无总数、gh退出 0 但携带的 job 少于声明、以及两页完整。前三种必须发布{"jobs":[]}、不留下暂存文件、发出警告并报告jobs_complete=false；最后一种必须发布两个 job、报告jobs_complete=true且保持沉默。随后在同一批夹具上把 summary 步骤跑两次：jobs_complete=false时正文必须带「读不全」的说明，true时不得带。另有断言把生产者的jobs_complete输出与消费者的JOBS_COMPLETE绑定钉在一起，防止两者漂移。要亲自验证每条守卫而不是采信结论，按证据表里写明的变异改一处，确认只有对应的断言变红。
证据（前后对比）
行为类各行在基线版本的步骤级 harness 上测得；变异、lint 与体积各行均在表格标注的 commit 上测得。
gh：第 1 页 150 个 job，第 2 页返回 403infra_only=true；修复后：空列表、警告、infra_only=false，仍建 issue461be87/ef371fdtotal_countgh：一页有数据，随后 API 错误体{"jobs":[]}，暂存文件已删，发出警告ef371fdgh：两页共 150 个 jobef371fdgh退出 0 但偏短gh：单页声明total_count: 2、只带 1 个 job、退出 02e77947c03npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/main-ci-failure-issue-workflow.test.js2e77947c03--paginateAssertionError: expected 'log_dir="${RUNNER_TEMP}/failed-logs" …' to contain 'gh api "repos/${REPO}/actions/runs/${…'2e77947c03jobs_complete设为trueAssertionError: expected 'true' to be 'false'2e77947c03AssertionError: expected 'body<<QWEN_CI_COMMENT_…' to contain 'The job list could not be read in full'2e77947c03id: 'download'改名为id: 'fetch_logs'AssertionError: expected undefined to be truthy2e77947c03prettier --check、eslint2e77947c03actionlint -shellcheck= -ignore 'SC2016:' …（与 CI 相同参数）2e77947c03.github/scripts/check-workflow-size.sh2e77947c03测试平台
macOS 本机用 PATH 上的 bash 5 运行测试，因为步骤脚本使用的某个内建在系统 bash 3.2 中不存在；Linux 是 CI 运行环境，未在本机执行。Windows 不适用。
环境（可选）
仅本地单元测试；未使用沙箱，未启动 CLI 会话。harness 用假
gh逐字执行 workflow 的步骤脚本。风险与范围
web-shell E2E Smoke在最近几个 head 上一直红，失败在本 PR 未触及的 spec 里（9f18c4661b：343 通过、10 个 flaky、3 个失败，分别在managed-context-compression、web-shell.browser-notifications、web-shell.excel-preview），而同一 head 上Test、Lint & Static、Integration Tests、Managed Agent gate均通过。每次推送都会重跑该套件。关联 issue
Refs #15138 —— 本 PR 只处理分页不完整这一项。同一 issue 中的目标发现项未改动，仍待讨论。