feat(autofix): resolve the review threads whose findings it implemented - #7364
Merged
Conversation
A human re-reviewing a managed PR currently has to re-read every thread to work out what the bot already handled. #7308 shows the cost: 17 review threads, 13 still open, with no way to tell which of those were fixed and which were declined. The agent already decides per finding — it records each one in address-summary.md as implemented or declined with a reason. What was missing was a way to act on that: the agent's sandbox carries no GitHub token, so it cannot resolve anything itself, and feedback.md gave it no stable handle to point at even if it could. Three small pieces close that: - feedback.md now renders each inline comment with its id (`- [rc:<id>] …`), giving the agent a handle it can echo back. - The SKILL asks the agent to write resolved-comments.txt: one id per line, for findings it IMPLEMENTED only. A declined or deferred finding must stay unresolved so its recorded reason actually gets read. - After a successful push, the step that already holds the PAT maps each id to its review thread and resolves it. Deliberately narrow: only threads the agent claims it implemented, only ones not already resolved, and entirely best-effort — a resolve failure warns and never fails a good push. Tests: the real extracted block is driven with a stubbed gh over fixture threads — an implemented finding's open thread is resolved, a DECLINED finding's thread is left open, an already-resolved thread is skipped, and an unknown id matches nothing. Mutation-verified: dropping the isResolved guard turns it red.
wenshao
added
the
autofix/takeover
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed and concrete — #7308 shows 17 review threads with 13 still open after the bot addressed feedback, and there's no way to tell which were handled vs. declined. This is a real workflow friction point, not theoretical. Direction: aligned. Autofix thread resolution is a natural extension of the existing autofix reliability line (#7330, #7350, #7351, #7354, #7355, #7358). CHANGELOG has no direct reference to thread resolution, but the autofix area is well-established. Size: not applicable — no core paths touched. Changes are confined to CI workflow, skill docs, and tests. Approach: the scope feels right. Three small pieces close the loop — a handle in 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且具体——#7308 显示 bot 处理反馈后仍有 17 个 review thread、13 个打开，无法分辨哪些已处理、哪些被婉拒。这是真实的工作流摩擦，不是理论问题。 方向：对齐。Autofix thread resolution 是现有 autofix 可靠性主线（#7330、#7350、#7351、#7354、#7355、#7358）的自然延伸。CHANGELOG 无直接引用，但 autofix 领域已成熟。 规模：不适用——未触及核心路径。改动限于 CI workflow、skill 文档和测试。 方案：范围合理。三个小环节闭合回路——feedback.md 中的句柄、agent 的声明文件、推送步骤的 resolve——每个都必要，无无关改动。Best-effort 设计（resolve 失败只告警，不让推送失败）对 CI 自动化是正确的权衡。进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal: given the problem (autofix implements findings but threads stay open, reviewers can't tell what was handled), I'd add a stable comment identifier to Comparison: the PR's approach matches this exactly. Three small pieces — No critical blockers found. Specific observations:
No AGENTS.md violations. The change is minimal — every edit serves the stated goal, no drive-by refactors or scope creep. TestingThis is CI infrastructure (not user-facing CLI), so before/after is the test suite and static checks: Static checks: 71/71 tests pass (including the new resolve-threads test). YAML parses cleanly with both js-yaml and Python yaml. 中文说明代码审查独立方案： 针对问题（autofix 实现了评审意见但 thread 保持打开，复查者无法分辨已处理内容），我会给 feedback.md 中的评论加稳定标识符，让 agent 在文件中声明实现了哪些 ID，推送后通过 GitHub GraphQL API resolve 对应 thread——best-effort，跳过已解决的。 对比： PR 方案与此完全一致。三个小环节——feedback.md 中的 未发现关键阻塞问题：
无 AGENTS.md 违规。改动最小化——每处编辑都服务于目标，无顺手重构或范围蔓延。 测试这是 CI 基础设施（非用户可见 CLI），before/after 即测试套件和静态检查：71/71 测试通过（含新的 resolve-threads 测试），YAML 用 js-yaml 和 Python yaml 均解析正常。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. The problem is real and concrete (#7308: 17 threads, 13 open, no way to tell what the bot handled). The solution is exactly what I'd propose independently — a stable handle in feedback.md, a declaration file from the agent, and a best-effort resolve step in the push. Three small pieces, each necessary, nothing extra. The test is the standout: it extracts the actual bash block from the workflow YAML and drives it with fixture threads and a stubbed The best-effort design is the right call for CI automation: a resolve failure warns and never fails a good push. The If I had to maintain this in six months, I'd thank the author — the comments explain the why, the scope is tight, and the test covers the behavior. 中文说明置信度：5/5 —— 每个阶段都干净；毫不犹豫地合并。 问题真实且具体（#7308：17 个 thread，13 个打开，无法分辨 bot 处理了什么）。方案与我独立提出的完全一致——feedback.md 中的稳定句柄、agent 的声明文件、推送步骤的 best-effort resolve。三个小环节，每个都必要，没有多余。 测试是亮点：提取工作流 YAML 中的实际 bash 块，用夹具 thread 和 stub Best-effort 设计对 CI 自动化是正确选择：resolve 失败只告警，绝不让成功的推送失败。输入文件的 六个月后维护这段代码，我会感谢作者——注释解释了 why，范围紧凑，测试覆盖了行为。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 20, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…reads # Conflicts: # scripts/tests/qwen-autofix-workflow.test.js
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Comment on lines
+2716
to
+2723
| THREADS_JSON="$(gh api graphql -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F pr="${PR}" -f query=' | ||
| query($owner:String!,$name:String!,$pr:Int!){ | ||
| repository(owner:$owner,name:$name){ | ||
| pullRequest(number:$pr){ | ||
| reviewThreads(first:100){nodes{id isResolved comments(first:100){nodes{databaseId}}}} | ||
| } | ||
| } | ||
| }' --jq '.data.repository.pullRequest.reviewThreads.nodes' 2> /dev/null || echo '[]')" |
Collaborator
There was a problem hiding this comment.
[Suggestion] The --jq filter .data.repository.pullRequest.reviewThreads.nodes can output the literal null when .data.repository.pullRequest is null (transient API inconsistency, race condition). Since gh exits 0 in that case, the || echo '[]' fallback does not fire. Downstream, jq 'map(…)' <<< "null" crashes with Cannot iterate over null, and under the step's set -eo pipefail the assignment thread_id="$(jq …)" kills the step — after the push already succeeded, the report-generation block and gh pr comment never execute, and the always() && failure() step posts a misleading failure message.
— Failure scenario: GraphQL returns null for pullRequest → THREADS_JSON is "null" → jq crashes → step dies post-push → misleading CI failure at 3 AM.
Suggested change
| THREADS_JSON="$(gh api graphql -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F pr="${PR}" -f query=' | |
| query($owner:String!,$name:String!,$pr:Int!){ | |
| repository(owner:$owner,name:$name){ | |
| pullRequest(number:$pr){ | |
| reviewThreads(first:100){nodes{id isResolved comments(first:100){nodes{databaseId}}}} | |
| } | |
| } | |
| }' --jq '.data.repository.pullRequest.reviewThreads.nodes' 2> /dev/null || echo '[]')" | |
| }' --jq '(.data.repository.pullRequest.reviewThreads.nodes // [])' 2> /dev/null || echo '[]')" |
— qwen3.7-max via Qwen Code /review
| query($owner:String!,$name:String!,$pr:Int!){ | ||
| repository(owner:$owner,name:$name){ | ||
| pullRequest(number:$pr){ | ||
| reviewThreads(first:100){nodes{id isResolved comments(first:100){nodes{databaseId}}}} |
Collaborator
There was a problem hiding this comment.
[Suggestion] reviewThreads(first:100) fetches only the first 100 threads with no pagination. When a thread falls outside the first 100 (plausible in takeover mode where TAKEOVER_MAX_ROUNDS is 100 and each round can produce multiple threads), the guard [[ -n "${thread_id}" ]] || continue silently skips it — no ::warning:: is emitted on that path (the existing warning only fires on mutation failure), so the silent drop is invisible in CI logs.
— Failure scenario: PR accumulates >100 review threads → agent implements finding #101 → thread not in response → silently skipped → thread stays open with no log entry.
Consider adding pageInfo{hasNextPage} to the query and emitting a ::warning:: when hasNextPage is true, so a silent drop at least surfaces in the logs.
— qwen3.7-max via Qwen Code /review
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
Author
Review: resolve the review threads whose findings it implementedReviewed the diff plus the surrounding workflow context ( What it does well
Suggestions (all non-blocking)
Scope / trustThe over-claim risk is already called out in the PR body, and I agree the framing is right: it fails in the visible direction and the summary comment remains the audit trail. One thing to keep in mind — a resolved thread is a stronger "done" signal to a human skimming the PR than a prose line in a summary, so an over-claim is a bit more likely to be trusted-and-forgotten than the prose equivalent. The auditability argument holds, so this is a note, not an objection. Overall: correct, defensively written, and genuinely useful for re-review ergonomics. Addressing (1) and (2) would round off the last rough edges. |
- Guard --jq against null pullRequest (// {nodes:[]}) so a transient
API inconsistency cannot crash the step after a successful push
- Add pageInfo{hasNextPage} and emit ::warning:: when threads exceed
the first-100 page cap
- Tolerate rc: prefix and trailing CR in resolved-comments.txt
- Emit ::warning:: when a valid numeric id matches no open thread
- Match production set -euo pipefail flags in the extracted-block test
Collaborator
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Review feedback addressedAll feedback points were Suggestions (no blocking bugs). Each one has been implemented: 1.
|
qwen-code-ci-bot
approved these changes
Jul 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
approved these changes
Jul 21, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — the logic is correct and the tests replay the real extracted blocks convincingly.
Non-blocking, for a follow-up: the amount of branching logic living inside run: | blocks (GraphQL queries, jq pipelines, while-read loops, multi-arm conditionals) is getting hard to maintain — tests have to regex-extract YAML substrings and replay them under bash, which couples test correctness to YAML indentation. Worth extracting these into .github/scripts/autofix-*.mjs standalone scripts (same pattern as web-shell-visuals-publish.mjs) so they get proper linting, direct unit-test imports, and reuse across workflows. Not blocking this PR — the current approach works and is well-tested; just flagging the direction for when the next chunk of logic lands here.
wenshao
pushed a commit
that referenced
this pull request
Jul 21, 2026
This PR was stacked on ci/autofix-gate-crash-retry (#7351), which has since merged, so it now targets main directly. That turned the stale in-branch copy of #7351 into conflicts against its reviewed final form. Resolved by keeping this PR's own additions and taking main's version of everything that belongs to #7351 or #7364: - Gate helpers: kept this branch's GATE_LOG capture and run_check, which are what this PR adds; main's plain reject_fix was their predecessor. - Artifact listing: union of gate-rejection.md and resolved-comments.txt. - Handoff comments: took main's wording — it covers both no-verdict causes and is accurate that the agent's commit dies with the runner. - Tests: both sides added an it() at the same anchor. Kept both and supplied the seam terminator; the trailing rmSync in the shared context belongs to main's test, which has no cleanup of its own.
This was referenced Jul 21, 2026
Collaborator
|
Released in v0.20.1. |
Collaborator
|
This is just the CI bot confirming PR #7364 ( ✅ completed |
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
4 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
Makes the autofix loop resolve the review threads whose findings it actually implemented, so a human re-reviewing a managed PR sees only what is still open.
Why it's needed
Today a reviewer has to re-read every thread to work out what the bot already handled. #7308 shows the cost: 17 review threads, 13 still open, with no way to tell which of those were fixed and which were declined.
The agent already decides per finding —
address-summary.mdrecords each one as implemented or declined with a reason. What was missing was any way to act on that decision:feedback.mdgave it no stable handle to point at even if it could.How
Three small pieces close the loop:
feedback.mdcarries the handle. Each inline comment now renders as- [rc:<id>] path:line @author: body.resolved-comments.txt, one id per line, for findings it IMPLEMENTED only — a declined or deferred finding must stay unresolved so its recorded reason actually gets read.Deliberately narrow: only threads the agent claims it implemented, only ones not already resolved, and entirely best-effort — a resolve failure warns and never fails a good push.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 68/68. The real extracted resolve block is driven with a stubbedghover fixture threads:111222333999Mutation-verified: dropping the
isResolvedguard turns it red.Static (run locally with the exact CI toolchain):
js-yamlparses; all 36run:blocks passbash -n; actionlint 1.7.12 clean; prettier clean.Post-merge smoke: on the next round of a managed PR, the threads the bot fixed show as resolved and the ones it declined stay open with their reason.
Evidence (Before & After)
Tested on
Risk & Scope
address-summary.md(which claims the same thing in prose), and it fails in the visible direction — the summary comment still lists every finding and its decision, so an over-claim is auditable in the same place it is made.Linked Issues
Raised from #7308's unresolved-thread pile. Part of the autofix reliability line: #7330, #7350, #7351, #7354, #7355, #7358.
中文说明
本 PR 做了什么
让 autofix 在确实实现了某条评审意见后 resolve 对应的 review thread,这样人来复查时只会看到还没处理完的部分。
为什么需要
现在复查者必须把每个 thread 重读一遍,才能弄清 bot 已经处理了什么。#7308 就是代价:17 个 review thread,13 个仍然打开,而且无法分辨其中哪些已修、哪些是被婉拒的。
agent 本来就是逐条决策的 ——
address-summary.md会记录每条是"已实现"还是"婉拒并附理由"。缺的是把这个决策落到实处的通路:feedback.md也没给它任何稳定句柄可指。怎么做
三个小环节闭合:
feedback.md带上句柄:每条 inline 评论渲染为- [rc:<id>] path:line @author: body。resolved-comments.txt,每行一个 id,且仅限"确实实现了"的——被婉拒或延后的必须保持未解决,好让它记录的理由真的被读到。刻意收窄:只处理 agent 声称已实现的、且尚未解决的 thread;并且全程 best-effort —— resolve 失败只告警,绝不让一次成功的推送失败。
评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 68/68。真实提取的 resolve 块用 stub 的gh在夹具 thread 上驱动:已实现的开放 thread 被 resolve;被婉拒的保持打开;已解决的跳过;未知 id 不匹配任何东西。变异验证:去掉isResolved守卫即变红。run:块过bash -n;actionlint 1.7.12 clean;prettier clean。风险与范围
address-summary.md的信任同级(它本就以散文形式作同样声明),且失败方向是可见的——摘要评论仍会列出每条意见及其决策,过度声称可在同一处被审计。关联 Issue
由 #7308 的未解决 thread 堆积引出。属于 autofix 可靠性主线:#7330、#7350、#7351、#7354、#7355、#7358。