Skip to content

fix(autofix): paginate review threads instead of reaching the oldest 100 - #9390

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
qqqys:fix/autofix-paginate-review-threads
Aug 19, 2026
Merged

fix(autofix): paginate review threads instead of reaching the oldest 100#9390
wenshao merged 5 commits into
QwenLM:mainfrom
qqqys:fix/autofix-paginate-review-threads

Conversation

@qqqys

@qqqys qqqys commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Paginates the review-thread fetch in resolve_and_reply_threads, so a round can resolve and reply to the threads it actually addressed rather than only the oldest hundred on the PR.

Why it's needed

The fetch was reviewThreads(first:100) with no pagination. GitHub returns review threads in ascending creation order, so a single page is the oldest hundred — on a long-running PR, precisely not the threads the current round is answering.

Both blocks downstream map an inline-comment id to its thread. A thread past that page is absent from THREADS_JSON, so:

  • an implemented Critical is never resolved and reads as still open, and
  • a declined finding's reply is answered by silence.

Those are the two outcomes the function's own comment says it exists to prevent.

Measured on the live pool: 8 of the 22 open takeover PRs exceed the cap. #8403 carries 1256 threads, so one page reached 8% of them — and all 1256 are currently unresolved.

The code already detected the condition. It requested pageInfo{hasNextPage} and emitted a ::warning:: when it was true; it simply never fetched the next page. This closes that gap rather than adding a new signal.

How

gh api graphql --paginate, which exists for this shape: the query gains an $endCursor variable and pageInfo{hasNextPage endCursor}, and gh walks the pages itself. Its --jq '…nodes[]' stream is slurped into the same flat array both blocks already consume, so nothing downstream changes.

On #8403 that is 13 requests in about 10 seconds.

Two deliberate choices worth reviewing:

A partial fetch is used, not discarded. If pagination dies partway — a rate limit on a later page — the threads already in hand still map, and the incompleteness is announced. Discarding them would mean losing twelve good pages to a failure on the thirteenth and resolving nothing at all, which is strictly worse than today's behaviour. The step runs under bash -e -o pipefail, so the fetch and the slurp each carry an explicit || fallback and the assignment cannot abort the step.

One residual stays open, and is now announced rather than implied. A thread carrying more than 100 comments still truncates its comments page, so a comment past it is unmapped and each block falls back to the id as given — the same fallback as before. A new warning names it. No thread in the live pool comes close: across #8403's 1256 threads, zero have more than 100 comments.

Reviewer Test Plan

How to verify

npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-workflow.test.js

179 passed. The suite extracts the real bash block from the workflow and executes it against a stub gh.

What the new coverage pins, and what it honestly cannot:

  • Structural — the block asks for --paginate, the reviewThreads(first:100, after:$endCursor) shape, and pageInfo{hasNextPage endCursor}. The stub gh exits 3 if --paginate is absent, so the request shape cannot silently regress. Verified to bite: removing --paginate from the workflow turns the test red.
  • Behavioural, deep stream — a 401-node stream resolves a target sitting at index 400, so any surviving hundred-item ceiling in the consumer drops it.
  • Behavioural, partial fetch — a pagination that exits non-zero still resolves the threads it got and prints review-thread pagination did not complete.
  • Behavioural, residual — a thread reporting comments.pageInfo.hasNextPage produces the new warning.
  • Cannot be proven here — that GitHub actually returns every page. The stub replaces gh wholesale, so page-walking is gh's contract, not this suite's. That is why the request shape is pinned structurally and why the live check below is worth running.

Live end-to-end against the worst PR in the repo:

gh api graphql --paginate -f owner=QwenLM -f name=qwen-code -F pr=8403 \
  -f query='query($owner:String!,$name:String!,$pr:Int!,$endCursor:String){
    repository(owner:$owner,name:$name){pullRequest(number:$pr){
      reviewThreads(first:100, after:$endCursor){
        nodes{id isResolved comments(first:100){nodes{databaseId} pageInfo{hasNextPage}}}
        pageInfo{hasNextPage endCursor}}}}}' \
  --jq '.data.repository.pullRequest.reviewThreads.nodes[]' | jq -s 'length'

Returns 1256 in ~10s, against 100 before this change.

Two contract pins in the suite move with this diff and both are deliberate: the --paginate site count goes 18 → 19, and the pin's comment gains a sentence placing this site in the existing "consumed inline into a shell variable, never lands in a WORKDIR json file" class — so it does not join the jq -s 'add // []' normalizer count, which stays at 10.

Evidence (Before & After)

N/A — CI machinery, no user-visible surface. The observable change is on the PR page: threads the round addressed get resolved and replied to instead of staying open.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

Environment (optional)

Contract suite on Node 22.23.0, plus the live gh query above against PR #8403.

Risk & Scope

  • Main risk or tradeoff: a PR with very many threads now costs several GraphQL requests where it cost one. feat(audit): add legacy code audit workflow #8403 — the largest in the repo — is 13 requests in ~10s, inside a round that already budgets ~2 agent-hours. The cost scales with thread count, which is bounded by real review activity.
  • Second risk: the partial-fetch path is new behaviour rather than a strict improvement, since today's code cannot fail partway. It is pinned by test and announced at runtime, and the alternative (discard everything) is worse.
  • Not validated / out of scope: the >100-comments-per-thread residual is announced, not closed — nested pagination inside the thread loop is a materially larger change for a case with no live instance. The warning makes it visible if that ever changes.
  • Breaking changes / migration notes: none. THREADS_JSON keeps its shape and both consumers are untouched.

Linked Issues

None.

中文说明

这个 PR 做了什么

resolve_and_reply_threads 里的评审 thread 拉取加上分页,让一轮能够解决并回复它真正处理过的那些 thread,而不是只够得着 PR 上最老的一百条。

为什么需要

原来的拉取是 reviewThreads(first:100),没有分页。GitHub 返回评审 thread 是按创建时间升序的,所以单页拿到的是最老的一百条——在一个长期运行的 PR 上,恰恰不是当前这一轮在回应的那些。

下游两个代码块都要把 inline 评论 id 映射到它所属的 thread。落在这一页之外的 thread 不在 THREADS_JSON 里,于是:

  • 已经实现的 Critical 永远不会被标记解决,看起来像还没人管;
  • 被驳回的发现,它的回复变成了沉默。

而这两点正是该函数自己的注释里写明"存在就是为了防止"的后果。

线上实测:22 个托管中的 PR 里有 8 个超过该上限。 #84031256 条 thread,一页只够得着其中 8%——而且这 1256 条目前全部处于未解决状态。

代码其实早就检测到了这个情况:它请求了 pageInfo{hasNextPage},为 true 时还会打一条 ::warning::;只是从来没有去取下一页。本 PR 补的就是这个缺口,而不是新增一个信号。

怎么改的

gh api graphql --paginate——它就是为这种形态设计的:查询增加一个 $endCursor 变量和 pageInfo{hasNextPage endCursor},由 gh 自己走完所有页。它 --jq '…nodes[]' 的输出流被 slurp 成两个代码块本来就在消费的那个扁平数组,因此下游没有任何改动。

#8403 上,这是 13 次请求、约 10 秒。

有两处刻意的取舍值得评审关注:

部分拉取会被"使用",而不是丢弃。 如果分页中途失败(比如在靠后的某页遇到限流),已经拿到手的 thread 仍然参与映射,同时把"不完整"这件事播报出来。丢弃它们意味着为了第 13 页的失败而扔掉前 12 页的成果、最终一条都解决不了,那比现状严格更差。该步骤运行在 bash -e -o pipefail 下,因此拉取与 slurp 各自都带了显式的 || 兜底,赋值不会中断整个步骤。

有一处残留没有关闭,现在改为明确播报而非默认。 一条 thread 若携带超过 100 条评论,其 comments 页仍会被截断,因此超出的评论无法映射,两个代码块都会回落到"按原样使用该 id"——与改动前的回落路径一致。新增了一条 warning 点名这一点。线上没有任何 thread 接近该上限:#8403 的 1256 条里,超过 100 条评论的有 0 条。

审阅者验证方案

如何验证

npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-workflow.test.js

179 项通过。该套件会从工作流里抽出真实的 bash 块,并对着一个 stub 版 gh 执行。

新增覆盖钉住了什么、以及诚实地说它钉不住什么:

  • 结构层 —— 该块必须请求 --paginatereviewThreads(first:100, after:$endCursor) 这一形态,以及 pageInfo{hasNextPage endCursor}。stub 版 gh 在缺少 --paginate 时以 3 退出,因此请求形态不可能悄悄回退。已验证它真的会咬:把 --paginate 从工作流里去掉,测试即转红。
  • 行为层,深流 —— 一个 401 个节点的流中,目标位于第 400 个索引位;消费端若还残留任何"一百条上限",它就会被丢掉。
  • 行为层,部分拉取 —— 分页以非零码退出时,仍然解决了已取到的 thread,并打印 review-thread pagination did not complete
  • 行为层,残留 —— 一条报告 comments.pageInfo.hasNextPage 的 thread 会触发新增的那条 warning。
  • 这里证明不了的 —— GitHub 是否真的返回了每一页。stub 完全替换了 gh,因此"走完分页"属于 gh 的契约而非本套件的。这正是要在结构层钉住请求形态、并建议跑一次下面这条线上验证的原因。

对着仓库里最糟的那个 PR 做线上端到端验证:

gh api graphql --paginate -f owner=QwenLM -f name=qwen-code -F pr=8403 \
  -f query='query($owner:String!,$name:String!,$pr:Int!,$endCursor:String){
    repository(owner:$owner,name:$name){pullRequest(number:$pr){
      reviewThreads(first:100, after:$endCursor){
        nodes{id isResolved comments(first:100){nodes{databaseId} pageInfo{hasNextPage}}}
        pageInfo{hasNextPage endCursor}}}}}' \
  --jq '.data.repository.pullRequest.reviewThreads.nodes[]' | jq -s 'length'

返回 1256,耗时约 10 秒;改动前是 100

套件里有两条契约钉子随本 diff 变动,均为刻意:--paginate 站点计数由 18 变为 19;该钉子的注释新增一句,把本站点归入既有的"就地消费进 shell 变量、从不落入 WORKDIR json 文件"那一类——因此它加入 jq -s 'add // []' 归一化计数,后者保持为 10。

证据(前后对比)

N/A —— CI 机制,无用户可见界面。可观察到的变化在 PR 页面上:本轮处理过的 thread 会被解决和回复,而不是一直挂着。

测试环境

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

环境(可选)

Node 22.23.0 上的契约套件,外加对 PR #8403 执行上述线上 gh 查询。

风险与范围

  • 主要风险或取舍:thread 极多的 PR 现在要发若干次 GraphQL 请求,而以前只发一次。feat(audit): add legacy code audit workflow #8403——仓库里最大的那个——是 13 次请求、约 10 秒,而它所在的这一轮本身就预算了约 2 个 agent 小时。开销随 thread 数量增长,而后者受真实评审活动约束。
  • 第二个风险:部分拉取路径是新增行为而非严格改进,因为今天的代码根本不会中途失败。它已被测试钉住并在运行时播报,而备选方案(全部丢弃)更差。
  • 未验证 / 不在范围内:单条 thread 超过 100 条评论的残留只做播报、未做关闭——在 thread 循环内部再做一层嵌套分页是明显更大的改动,而线上并无实例。那条 warning 会在情况改变时让它显形。
  • 破坏性变更 / 迁移说明:无。THREADS_JSON 形状不变,两个消费方均未改动。

关联 Issue

无。

`resolve_and_reply_threads` fetched `reviewThreads(first:100)` with no
pagination. GitHub returns review threads in ASCENDING creation order, so a
single page is the OLDEST hundred — on a long-running PR, precisely not the
threads the current round is answering.

Both blocks downstream map an inline-comment id to its thread. A thread past
the page is absent from `THREADS_JSON`, so an implemented Critical is never
resolved and reads as still open, and a declined finding's reply is answered
by silence. Those are the two outcomes the function exists to prevent.

Live: 8 of the 22 open takeover PRs exceed the cap. QwenLM#8403 carries 1256
threads, so one page reached 8% of them — and all 1256 are unresolved.

The code already detected this: it requested `pageInfo{hasNextPage}` and
emitted a `::warning::` when true. It just never fetched the next page.

Use `gh api graphql --paginate`, which is built for exactly this shape, and
slurp its node stream into the flat array both blocks already expect. On
QwenLM#8403 that is 13 requests in ~10s.

A partial fetch is USED rather than discarded: losing twelve good pages to a
rate limit on the thirteenth would resolve nothing at all, so the failure is
announced and the threads in hand still map.

One residual stays open and is now announced rather than implied: a thread
carrying more than 100 comments still truncates, so a comment past that page
is unmapped and each block falls back to the id as given. No thread in the
live pool comes close.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 0629b1b and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 0629b1b 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, and the codebase itself corroborates it. The fetch is a bare reviewThreads(first:100) that requests pageInfo{hasNextPage} and emits a ::warning:: when it's true — but never fetches the next page. So on any PR past 100 threads, both consumers (resolve block, reply block) silently fall back for every thread past the page. The live-pool numbers in the description (8 of 22 takeover PRs over the cap, #8403 at 1256 threads) make this a current, not hypothetical, failure.

Direction: aligned. This closes a gap in the repo's own review automation — an implemented Critical reading as unaddressed and a declined finding answered by silence are exactly the two outcomes resolve_and_reply_threads says it exists to prevent. No CHANGELOG reference (internal CI machinery), and none needed.

Size: not applicable — no core paths touched. 47 production lines in the workflow plus 139 lines of contract-test updates.

Approach: the scope feels right. Reusing gh api graphql --paginate (with the $endCursor/pageInfo shape gh requires) instead of a hand-rolled cursor loop is the minimal fix, and leaving both consumers untouched by keeping THREADS_JSON a flat array is the right call. The two judgment calls are reasonable: using a partial fetch (announced) rather than discarding it, since losing twelve good pages to a failure on the thirteenth resolves nothing; and announcing rather than closing the >100-comments-per-thread residual, which has no live instance. Nothing unrelated in the diff.

Risk: no elevated risk signals — no high-risk paths matched. One thing for the reviewer's awareness: this is the workflow's first GraphQL --paginate site (the existing 18 are REST), and the contract pin's comment now documents that class distinction.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到,且代码本身可以佐证。原来的拉取是裸的 reviewThreads(first:100),它请求了 pageInfo{hasNextPage}、并在其为 true 时打 ::warning::——却从不去取下一页。因此任何超过 100 条 thread 的 PR 上,两个消费方(resolve 块、reply 块)都会对超出该页的 thread 静默回落。描述里的线上数据(22 个托管 PR 中 8 个超上限,#8403 有 1256 条 thread)说明这是当下正在发生的失败,而非假设。

方向:对齐。这补上的是仓库自身评审自动化的缺口——已实现的 Critical 看起来无人处理、被驳回的发现得到沉默回应,正是 resolve_and_reply_threads 写明自己要防止的两种后果。CHANGELOG 无相关条目(内部 CI 机制),也不需要。

规模:不适用——未触及核心路径。工作流 47 行生产代码,外加 139 行契约测试更新。

方案:范围合理。复用 gh api graphql --paginate(带上 gh 要求的 $endCursor/pageInfo 形态)而不是手写游标循环,是最小的修法;保持 THREADS_JSON 为扁平数组、两个消费方都不动,也是对的。两处取舍都站得住:部分拉取"使用"而非丢弃(并播报),因为为第 13 页的失败扔掉前 12 页会一条都解决不了;对"单条 thread 超过 100 条评论"的残留只做播报不做关闭——线上没有实例。diff 无无关改动。

风险:无升级风险信号——未命中高风险路径。供评审者留意的一点:这是该工作流第一个 GraphQL --paginate 站点(现有 18 处都是 REST),契约钉子的注释已把这一类别区分写明。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 0629b1b5c3e6875b82ff735b97bbfd925b52deea · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Code review

The implementation lands on the same fix I'd have chosen independently: hand gh api graphql --paginate the $endCursor variable and pageInfo{hasNextPage endCursor} it expects, keep the per-page --jq '…nodes[]' stream slurped into the same flat THREADS_JSON array, and leave both consumers untouched. I checked the parts of this shape that are easy to get wrong:

  • The THREADS_RAW="$(gh …)" || THREADS_FETCH_OK='false' form is correct under the step's bash -eo pipefail (the test harness runs the extracted block under set -euo pipefail, matching it): the assignment carries the command substitution's exit status, and gh emits the pages it already fetched before a mid-pagination failure, so the "use a partial fetch" policy actually holds. Total failure still lands on [], the same semantics as the old || echo '{"nodes":[]}' path.
  • The residual warning is data-backed: the query now requests comments(first:100){… pageInfo{hasNextPage}}, and map(select(.comments.pageInfo.hasNextPage)) is null-safe on the node shape.
  • The two consumer filters (resolve: map(select(.isResolved | not) …), reply: .[0].comments.nodes[0].databaseId // $id) are unchanged and consume the identical array shape — the stale "first-100 page cap" comment in the reply block was updated to match the new reality rather than left misleading.
  • The contract pins move consistently: --paginate site count 18 → 19, and the new site is deliberately documented as the "consumed inline into a shell variable" class so the jq -s 'add // []' normalizer count stays at 10. This is the workflow's first GraphQL paginate site (the existing 18 are REST), and the pin's comment now says so.

The tests are the stronger part: they execute the real extracted bash block (not a reimplementation) against a stub gh that exits 3 if --paginate is missing, so the request shape cannot silently regress. The new cases pin the deep stream (401 nodes, target at index 400 — any surviving hundred-item ceiling drops it), the partial fetch (still resolves, announces itself), and the residual warning. runResolve resets its state files per invocation, so the inserted cases don't leak into the tests that follow. No blockers found.

What static review cannot settle here: that gh --paginate actually walks every page against the live API, and that a real 100+-thread PR sees its later threads resolved end-to-end.

Testing

This is an unattended CI run — no PR code was built or executed here; evidence is the PR's own CI on the reviewed commit, fetched via the API. The Test (ubuntu-latest, Node 22.x) job is the load-bearing one: CI's test:ci ends with npm run test:scripts, which runs exactly scripts/tests/qwen-autofix-workflow.test.js. It is still running — no polling, so the table below reflects a single fetch, and the finalize job will update it when CI settles. The Windows/macOS/integration jobs were skipped by PR classification (expected for a workflow + test-script change); nothing is red so far.

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

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

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

Not verified: the live pagination result — the PR body reports 1256 threads fetched from #8403 in ~10s via the exact gh api graphql --paginate command it publishes — is the author's claim, not independently re-run here. The command is read-only, so a maintainer can reproduce it directly.

Sandboxed verification would settle part of this: @qwen-code /verify — an A/B run would prove the new deep-stream and structural pins actually flip against the base build (i.e. the suite is load-bearing for this diff, not green either way). The live-API page-walk itself is gh's contract and only the author's published command observes it; the end-to-end effect on a real 100+-thread PR is observable post-merge, since this workflow dogfoods on the repo's own takeover PRs.

中文说明

代码审查:实现与我独立想到的修法一致——给 gh api graphql --paginate 提供它要求的 $endCursor 变量和 pageInfo{hasNextPage endCursor},把每页 --jq '…nodes[]' 输出 slurp 成同样的扁平 THREADS_JSON 数组,两个消费方保持不动。几个容易出错的点都核过:THREADS_RAW="$(gh …)" || THREADS_FETCH_OK='false' 在该步骤的 bash -eo pipefail 下语义正确(测试 harness 用 set -euo pipefail 执行抽取出的真实代码块,与之吻合),gh 在中途失败前已输出取到的页,所以"部分拉取继续使用"真的成立;全失败仍落到 [],与旧路径语义相同。残留 warning 有数据支撑(查询现在一并请求 commentspageInfo{hasNextPage})。reply 块里过时的"first-100 page cap"注释已同步更新。契约钉子变动自洽:--paginate 站点 18 → 19,新站点被明确归入"就地消费进 shell 变量"一类,normalizer 计数保持 10。测试更强:执行的是从工作流里抽取的真实 bash 块,stub gh 在缺少 --paginate 时以 3 退出,请求形态不可能悄悄回退;新增用例钉住深流(401 节点、目标在第 400 位)、部分拉取、残留告警。未发现阻塞项。

测试:这是无人值守 CI 运行,未构建或执行任何 PR 代码;证据来自 PR 自身 CI(API 拉取)。关键的 Test (ubuntu-latest, Node 22.x) 仍在运行(CI 的 test:ci 最后一步就是跑该契约套件),不做轮询,finalize 任务会在 CI 落定后更新表格;目前无红。Windows/macOS/集成任务被分类器跳过(对 workflow + 测试脚本改动属预期)。未验证:对 #8403 的线上分页结果(1256 条、约 10 秒)是作者声明,未在此独立复跑——该命令只读,维护者可直接复现。沙箱验证可补上一部分:@qwen-code /verify 可做 A/B,证明新增钉子相对 base 构建真的会翻红;线上分页本身属于 gh 契约,端到端效果要合并后在该仓库自己的托管 PR 上观察。

Qwen Code · qwen3.8-max

Reviewed at 0629b1b5c3e6875b82ff735b97bbfd925b52deea · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean gate and a clean review; the one point withheld is inherent, not a doubt: the live page-walk can't be proven before merge, only bounded.

Stepping back: the problem is real and code-acknowledged — the base block already requested pageInfo{hasNextPage} and warned about it while never fetching the next page, which is about as clear an admission of a gap as code gets. The fix is the minimal one I'd have chosen myself: gh api graphql --paginate with the $endCursor/pageInfo shape, the stream slurped into the exact array both consumers already read, so nothing downstream moves. The two judgment calls (use a partial fetch and announce it; announce rather than close the >100-comments-per-thread residual) are defensible and honestly disclosed — discarding twelve good pages for a failure on the thirteenth would be strictly worse than today.

The test work is what pushes this past "looks plausible": the suite executes the real extracted bash block under set -euo pipefail against a stub gh that hard-fails if --paginate disappears, and the new cases pin the deep stream (target at index 400), the partial fetch, and the residual warning. Every edit in the diff earns its place — even the moved contract pins carry an explanation of why the new site joins one count and not the other. If I'm maintaining this in six months, the comment block above the fetch tells me exactly why pagination exists and what the residual is.

The residual risk is the one every CI-machinery fix carries: the end-to-end claim (GitHub returns every page; later threads actually get resolved on a 1000+-thread PR) is only observable live, and the pre-merge evidence for it is the author's read-only, reproducible command plus gh's documented pagination contract. That's an acceptable basis here — the failure mode if the claim were wrong is "same as today", not corruption, and the workflow dogfoods on this repo's own PRs the moment it merges.

Approval is deferred until CI lands green on 0629b1b5c3e6875b82ff735b97bbfd925b52deea — the unit suite carrying the contract tests is still running.

中文说明

回顾整体:问题是真实存在且代码自己承认的——base 代码块早已请求 pageInfo{hasNextPage} 并在其为 true 时告警,却从不取下一页。修法也是我自己会选的最小方案:gh api graphql --paginate$endCursor/pageInfo 形态,流被 slurp 成两个消费方本来就在读的数组,下游零改动。两处取舍(部分拉取继续使用并播报;单条 thread 超 100 条评论的残留只播报不关闭)都站得住,且如实披露。

测试是它超越"看起来合理"的地方:套件在 set -euo pipefail 下执行从工作流抽取的真实 bash 块,stub gh 在缺少 --paginate 时直接失败;新用例钉住深流(目标在第 400 位)、部分拉取与残留告警。diff 里每一处改动都有必要,连契约钉子的移动都附带了类别说明。

残留风险是每份 CI 机制修复都带着的:端到端效果只能线上观察,合并前的证据是作者提供的只读、可复现命令加上 gh 分页契约。在本 PR 上这是可接受的——即便声明不成立,失败模式也是"同今天一样"而非损坏,且合并后立即在本仓库自己的 PR 上生效。

置信度 4/5,扣掉的一分来自"合并前无法证明线上分页"这一固有局限,而非疑虑。CI(含契约测试的单元套件)仍在运行,待其在上述提交上全绿后完成批准。

Qwen Code · qwen3.8-max

Reviewed at 0629b1b5c3e6875b82ff735b97bbfd925b52deea · 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.

LGTM, looks ready to ship — CI landed green after the review. ✅

@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.

Reviewed — no blockers. Suggestions are inline.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

中文说明

已审查——无阻断问题。 建议见行内评论。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
Comment thread .github/workflows/qwen-autofix.yml
Comment thread scripts/tests/qwen-autofix-workflow.test.js
…ings' absence

Round 1's three Suggestions, all on the partial-fetch path this PR adds.

R1-1: `2> /dev/null` on the paginated fetch discarded gh's stderr — the only
text saying WHY pagination stopped. The warning announced THAT it stopped, so
the oncall could not separate a transient rate limit (back off) from an
expired PAT (rotate) or a network failure without re-running the ~13-request
query by hand. Captured to `${WORKDIR}/threads-fetch.err` with the pattern
already used elsewhere in this workflow, and its tail folded into the warning.

R1-2: the outer thread pagination silently depends on the inner `comments`
pageInfo NOT asking for `endCursor` — gh's paginator adopts the first pageInfo
carrying both fields. The `Residual:` note actively invited a maintainer to
close that residual by adding it, which would hijack the thread-page cursor
and stop after page one at exit 0 with no warning, silently restoring the
oldest-hundred bug. Documented as load-bearing, in the comment block above the
fetch rather than inside the query literal — a `#` line there is transmitted.

R1-3: both new warnings were asserted only in the positive, so a mutation
making either unconditional shipped green. Added the clean-run absence
assertions this file's own convention calls for (321 `not.toContain` uses),
and the gh stub now writes a reason to stderr on failure so the folded-in text
is assertable.

Verified: qwen-autofix-workflow 178 passed. Mutation-checked — restoring
`2> /dev/null` and making the pagination warning unconditional each fail a
test. The one remaining failure (`behaviorally replays the stale-duplicate
revalidation`, 5s timeout) is identical with these changes stashed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qqqys

qqqys commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /review

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Qwen Code review request accepted. Review is queued in workflow run.

@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.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • scripts/tests/qwen-autofix-workflow.test.js:15771 (+2 locations) — [probe] Three load-bearing query pieces ($endCursor declaration, inner pageInfo shape, --jq stream filter) are pinned by no test; each mutation ships green and kills the fea…
中文说明

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
Comment thread .github/workflows/qwen-autofix.yml Outdated

@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.

Reviewed — no blockers. Suggestions are inline.

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

  • R2-2 multi-line gh stderr splits the ::warning:: annotation at the first newline — already reported (comment 3804433267)

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • .github/workflows/qwen-autofix.yml:6278 — [probe] unreachable dead guard: after the slurp line THREADS_JSON is always non-empty, so the [[ -n ]] branch never executes
  • scripts/tests/qwen-autofix-workflow.test.js:15772 — [probe] structural pin block under-covers the fetch contract: inner pageInfo shape, $endCursor signature, and --jq filter all ship green under mutation
中文说明

已审查——无阻断问题。 建议见行内评论。

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

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
Round 2 of the review on QwenLM#9390 found the paginated review-thread fetch
poisons its own output on a partial page, and asked for two clarifications
around it.

R2-C (Critical) — on a failing page gh skips `--jq` and appends that page's
raw response body (a rate-limit message, or a GraphQL error envelope) to
stdout after the good nodes. The unfiltered `jq -s '.'` slurped it as an
extra element, and both consumers below iterate `.comments.nodes[]` over
every element, so the first one exited 5. This step runs under errexit, so
that aborted 'Push and report' AFTER a good push had landed — the report and
the markers were skipped and the job failed. That contradicts the two
invariants the block documents: a resolve failure must never fail a good
push, and a partial fetch is used rather than discarded. The slurp now keeps
only thread-shaped documents.

R2-1 — the comment block warned against adding `endCursor` to the inner
`comments` pageInfo, but the outer pageInfo's field ORDER is load-bearing for
the same reason: gh's cursor scanner carries its flags across pageInfo
objects and breaks at the first one yielding both fields, so alphabetizing to
`pageInfo{endCursor hasNextPage}` stops after page one just as silently. Said
so at the query, and at the test pin that goes red on a reorder, so the pin
is understood rather than bumped.

R2-2 — the stderr fold dropped the `tr '\r\n' '  '` that its ten sibling
sites apply. Actions parses workflow commands line by line and gh's
secondary-rate-limit stderr spans two lines, so the annotation kept only the
first — cutting off the words that separate a back-off from a credential
rotation.

Verification: `scripts/tests/qwen-autofix-workflow.test.js` 179/179; yaml
parses; eslint and prettier clean. Mutation-checked all three: reverting the
slurp filter fails the resolve arm with exit 5 (expected 5 to be 0),
reordering the outer pageInfo fails the field-order pin, and dropping the
`tr` fails the folded-reason arm on the second stderr line.

@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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": empirical gh runs for the zero-threads and exact-multiple-of-100 edge cases (reasoned from the observed hasNextPage=false stop rule instead)..

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • .github/workflows/qwen-autofix.yml:6266 — [review] no runtime completeness signal for an exit-0 under-fetch of review threads
  • scripts/tests/qwen-autofix-workflow.test.js:15642 — [review] the load-bearing --jq stream filter is pinned by no test
  • scripts/tests/qwen-autofix-workflow.test.js:15783 — [review] $endCursor declaration and inner comments pageInfo shape are pinned by no test
中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)"empirical gh runs for the zero-threads and exact-multiple-of-100 edge cases (reasoned from the observed hasNextPage=false stop rule instead).

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
… R3-1)

The comment explaining why `pageInfo{hasNextPage endCursor}` order is
load-bearing described the silent stop as happening with the carried
`hasNextPage` "already true" from the last inner page. That cannot
produce the symptom: gh's `findEndCursor` returns a cursor only `if
hasNextPage`, so a carried true would keep the walk going.

The real mechanism is the opposite one. The scanner carries its flags
across `pageInfo` objects and breaks at the first point both have been
seen; under `pageInfo{endCursor hasNextPage}` that break lands on the
outer `endCursor` while `hasNextPage` still holds the last INNER page's
value — almost always false, since thread comment pages rarely truncate
— and the outer page's own `hasNextPage` is never read. gh returns no
cursor and the walk stops after page one, exit 0 and silent.

Reworded in both places the clause was copied to: the workflow comment
and the field-order pin's comment in the test. No assertion, no shell,
and no query text changes; `pageInfo{hasNextPage endCursor}` and the
test that pins it are untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qqqys

qqqys commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /review

@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.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • scripts/tests/qwen-autofix-workflow.test.js:15785 — [probe] inner comments pageInfo shape (absence of endCursor) is pinned by no test
  • scripts/tests/qwen-autofix-workflow.test.js:15641 — [review] stub gh does not pin the --jq nodes[] projection
  • scripts/tests/qwen-autofix-workflow.test.js:15953 — [probe] total-fetch-failure (empty stream) arm never exercised end to end
  • scripts/tests/qwen-autofix-workflow.test.js:15776 — [probe] $endCursor:String variable declaration is pinned by no test
中文说明

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
# pages rarely truncate, and the outer page's own hasNextPage is
# read only after the break) — gh then returns no cursor and the
# walk silently stops after page one.
if [[ -s "${WORKDIR}/resolved-comments.txt" || -s "${WORKDIR}/comment-replies.json" ]]; then

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.

[Suggestion] R4-2: The reply-only side of this hoisted fetch guard is exercised by no test — every runResolve() cycle in the resolve harness writes a non-empty resolved-comments.txt (so the || short-circuits), and the reply-block harness injects THREADS_JSON via env with a block extraction that starts AFTER the fetch — even though this diff's own comment says the hoist exists "so a round that only replies (no resolved-comments.txt) still has them". — Concrete cost: dropping or breaking -s "${WORKDIR}/comment-replies.json" from this guard ships green (mutation-verified: both tests still pass); in production an all-decline round — agent implements nothing, only comment-replies.json exists — then skips the fetch entirely, THREADS_JSON stays unset, jq over empty input executes the filter zero times and prints nothing (so even the // $id fallback never fires), root_id is empty → POST repos/.../comments//replies → 404 → every in-thread reply is lost, restoring exactly the silence-in-open-threads gap this block exists to close.

Witness (probe — a reply-only round driven through the real function, both sides):

MUTATED:  api repos/QwenLM/qwen-code/pulls/7731/comments//replies
          → ::warning::could not reply to review comment 222 → replied on 0 thread(s), exit 0
PRISTINE: api repos/QwenLM/qwen-code/pulls/7731/comments/100/replies → replied on 1 thread(s)

Suggested fix (in scripts/tests/qwen-autofix-workflow.test.js): add a behavioral run where resolved-comments.txt is absent/empty and comment-replies.json is present, asserting the fetch ran and a reply aimed at a reply id was remapped through THREADS_JSON; or at minimum pin this guard verbatim — expect(block).toContain('-s "${WORKDIR}/comment-replies.json" ]]; then') (verified to fail under the mutation and pass on pristine code).

中文说明

[建议] R4-2:这个被提升出来的拉取守卫,其“仅回复”一侧没有任何测试覆盖——resolve 测试夹具里 runResolve() 的每一轮都写了非空的 resolved-comments.txt(因此 || 短路),而 reply 代码块的测试夹具又直接经 env 注入 THREADS_JSON、其代码块抽取范围从拉取之后开始——尽管本 diff 自己的注释写明提升守卫正是为了“让只回复(没有 resolved-comments.txt)的一轮也能拿到 threads”。—— 具体代价:把这行守卫里的 -s "${WORKDIR}/comment-replies.json" 删掉或弄坏,整套测试仍为绿(已用变异验证:两个测试照常通过);生产中一次“全部驳回”的轮次——agent 没有实现任何修改、只有 comment-replies.json 存在——会因此完全跳过拉取,THREADS_JSON 保持未设置,jq 对空输入零次执行过滤器、什么都不输出(连 // $id 兜底都不会触发),root_id 为空 → POST repos/.../comments//replies → 404 → 所有 thread 内回复丢失,恰好恢复了本代码块存在就是为了消除的“开放 thread 里的沉默”。

证据(探针——对真实函数驱动一次仅回复轮次,两侧对比):见上方英文输出块。

修复建议(在 scripts/tests/qwen-autofix-workflow.test.js 中):新增一个行为式运行——resolved-comments.txt 不存在/为空而 comment-replies.json 非空,断言拉取确实发生、且指向回复 id 的回复被 THREADS_JSON 重映射;至少也应逐字钉住本守卫——expect(block).toContain('-s "${WORKDIR}/comment-replies.json" ]]; then')(已验证:变异下失败、原样代码下通过)。

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

@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.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

中文说明

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

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

Comment thread .github/workflows/qwen-autofix.yml Outdated
… path

R4-1 (Critical, QwenLM#9390): the review-thread pagination wrote gh's stderr to
`${WORKDIR}/threads-fetch.err` and read it back with `tail -c 300`, both
without a file-type guard. WORKDIR (`/tmp/autofix-review-<pr>`) is
bind-mounted read-write into the agent docker sandbox, and the round that
just finished ran branch code inside that sandbox, so the name is
attacker-chosen by the time this step runs.

A planted FIFO makes bash block on the O_WRONLY open before gh even execs,
and the only reader is the `tail` that runs strictly after gh returns — so
the step hangs to the job timeout with the push already landed, losing the
report comment and the round markers. That breaks the invariant this block
states for itself: a resolve failure must never fail a good push. A planted
symlink instead turns the redirect into a truncate/write against the link
target and the tail into a 300-byte arbitrary-file read folded into a public
`::warning::`.

Route the stderr through a fresh `mktemp` regular file instead, matching the
`gh api user` checks elsewhere in this workflow, and remove it afterwards.
The diagnostic is unchanged: the warning still carries gh's own reason, which
is the only text separating a transient rate limit from an expired PAT.

Test: plant a symlink at the old path, run the block through a failing fetch,
and assert the target's bytes are neither overwritten nor folded into the
annotation; plus assert the named path is not created at all. Mutation-
verified — restoring the `${WORKDIR}` redirect turns the canary assertion red
(`expected 'threads-fetch stub failure' to be 'CANARY-MUST-SURVIVE'`). The
FIFO half cannot be written as a plain assertion because the pre-fix code
hangs rather than fails; the same "named path is never opened" property
defuses it.

@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.

No blocking issues. LGTM! ✅

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:

  • .github/workflows/qwen-autofix.yml:6287 — [probe] the inner comments pageInfo shape (hasNextPage alone, no endCursor) is load-bearing but pinned by no test; the forbidden mutation ships green and silently stops pagination after page one
  • .github/workflows/qwen-autofix.yml:6307 — [probe] the total-fetch-failure path (empty/unparseable stream) is never exercised; the new slurp filter has no empty-input pin, unlike the ten add // [] normalizer sites
  • scripts/tests/qwen-autofix-workflow.test.js:15642 — [probe] the --jq reviewThreads.nodes[] stream filter is load-bearing but pinned by nothing; a dropped filter ships green and silently zeroes THREADS_JSON
  • scripts/tests/qwen-autofix-workflow.test.js:15777 — [probe] the $endCursor:String declaration's nullability is load-bearing but pinned by no assertion; a String! mutation breaks every round while tests stay green
中文说明

无阻断问题。LGTM!✅

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。

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

@wenshao
wenshao added this pull request to the merge queue Aug 19, 2026
Merged via the queue into QwenLM:main with commit b6e93d2 Aug 19, 2026
120 of 122 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.14.

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.

3 participants