-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(ci): make autofix finding replies idempotent #9463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5925320
4ef415b
d4dcfd0
6f884be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16288,6 +16288,16 @@ exit 1 | |
| // exit 0 and no warning, restoring the oldest-hundred bug silently. Do not | ||
| // "fix" this pin by reordering it. | ||
| expect(block).toContain('pageInfo{hasNextPage endCursor}'); | ||
| // Supply side of the reply gate: author{login} and body feed the dedup | ||
| // check, and pageInfo belongs INSIDE comments(...) — | ||
| // PullRequestReviewThread has no pageInfo field, so hoisting it to the | ||
| // thread level makes GitHub reject the whole query, THREADS_JSON becomes | ||
| // [], and both this block and the reply gate silently degrade. Pin the | ||
| // exact shape so the field list and pageInfo's inner position regress | ||
| // loudly. | ||
| expect(block).toContain( | ||
| 'comments(first:100){nodes{databaseId author{login} body} pageInfo{hasNextPage}}', | ||
| ); | ||
|
|
||
| const matching = runResolve(); | ||
| expect(matching.status).toBe(0); | ||
|
|
@@ -16642,7 +16652,17 @@ exit 1 | |
| ].join('\n'), | ||
| ); | ||
| chmodSync(join(bin, 'gh'), 0o755); | ||
| const runBlock = () => | ||
| // The threads fetch is hoisted above the reply block (shared with the | ||
| // resolve block). One thread holds root comment 100 with a reply 222, so | ||
| // a reply aimed at the REPLY id 222 must be remapped to root 100 — | ||
| // GitHub rejects a reply whose target is itself a reply. 444 is in no | ||
| // thread, which exercises the fall-back to the id as given. The nodes | ||
| // omit author/body on purpose: the idempotence gate must tolerate a | ||
| // threads view without them (pre-existing shape) and post as before. | ||
| const DEFAULT_THREADS = [ | ||
| { comments: { nodes: [{ databaseId: 100 }, { databaseId: 222 }] } }, | ||
| ]; | ||
| const runBlock = (threads = DEFAULT_THREADS) => | ||
| execFileSync('bash', ['-c', `set -uo pipefail\n${block}`], { | ||
| env: { | ||
| ...process.env, | ||
|
|
@@ -16651,14 +16671,8 @@ exit 1 | |
| REPO: 'QwenLM/qwen-code', | ||
| PR: '7731', | ||
| REPLIED_LOG: repliedLog, | ||
| // The threads fetch is hoisted above the reply block (shared with the | ||
| // resolve block). One thread holds root comment 100 with a reply 222, | ||
| // so a reply aimed at the REPLY id 222 must be remapped to root 100 — | ||
| // GitHub rejects a reply whose target is itself a reply. 444 is in no | ||
| // thread, which exercises the fall-back to the id as given. | ||
| THREADS_JSON: JSON.stringify([ | ||
| { comments: { nodes: [{ databaseId: 100 }, { databaseId: 222 }] } }, | ||
| ]), | ||
| AUTOFIX_BOT: 'qwen-code-dev-bot', | ||
| THREADS_JSON: JSON.stringify(threads), | ||
| }, | ||
| encoding: 'utf8', | ||
| }); | ||
|
|
@@ -16720,6 +16734,142 @@ exit 1 | |
| out = runBlock(); | ||
| expect(readFileSync(repliedLog, 'utf8').trim()).toBe(''); | ||
| expect(out).toContain('replied on 0 thread'); | ||
|
|
||
| // Idempotence: a crash-and-rerun of the round, a same-run repair, or a | ||
| // later round re-declining the same finding regenerates the same | ||
| // disposition — the reply must not land twice on one thread (#9296, | ||
| // where one identical reply was posted three times). The match is on | ||
| // the bot login plus the exact NEUTRALISED body, so a CHANGED body — | ||
| // new information from a later round — still posts. | ||
| writeFileSync(repliedLog, ''); | ||
| rmSync(join(dir, 'resolved-comments.txt'), { force: true }); | ||
| writeFileSync( | ||
| join(dir, 'comment-replies.json'), | ||
| JSON.stringify([ | ||
| { id: 222, body: 'Deferred — follow-up.\n\n中文:已延后。' }, | ||
| { id: 222, body: 'Changed reason — new round.' }, | ||
| ]), | ||
| ); | ||
| out = runBlock([ | ||
| { | ||
| // isResolved is set on purpose: the gate must not filter resolved | ||
| // threads — a re-armed round can re-decline a finding whose thread a | ||
| // reviewer resolved after the bot's first reply, and a duplicate is | ||
| // still a duplicate. The trailing human comment is the common live | ||
| // shape (root, bot declination, human answer): the matching comment | ||
| // is not the newest node, so the gate must scan every comment. | ||
| isResolved: true, | ||
| comments: { | ||
| nodes: [ | ||
| { databaseId: 100 }, | ||
| { databaseId: 222 }, | ||
| { | ||
| databaseId: 300, | ||
| author: { login: 'qwen-code-dev-bot' }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-5: Still standing at this commit (probe re-measured): in both new fixtures the compared comment (bot or human) is the LAST node of its thread, so a mutant narrowing the gate's scan to the newest comment survives the whole suite. A/B at this commit: with the matching bot comment followed by a later unrelated human comment in the same thread, the shipped gate exits 0 (skip — correct) while the newest-only mutant exits 1 (post — duplicate). Fix: add a fixture where the matching bot comment is followed by another comment in the same thread. 中文说明[Suggestion] R1-5:在本 commit 仍然存在(探针重新实测):两个新 fixture 中被比较的评论(bot 或人类)都是其线程的最后一个节点,因此把门的扫描收窄到最新评论的突变能通过整个套件。本 commit 上的 A/B:匹配的 bot 评论之后同一线程还有一条无关的人类评论时,现有门 exit=0(跳过——正确),只看最新评论的突变 exit=1(发布——重复)。修复:新增一个 fixture,让匹配的 bot 评论之后同线程还有另一条评论。 — qwen3.8-max via Qwen Code /review (v0.21.14) |
||
| body: 'Deferred — follow-up.\n\n中文:已延后。', | ||
|
Comment on lines
+16768
to
+16769
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No reply-gate fixture sets 中文说明没有任何回复门的 fixture 给线程设置 — qwen3.8-max via Qwen Code /review (v0.21.14)
Comment on lines
+16768
to
+16769
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-6: Still standing at this commit (probe re-measured): no reply-gate fixture sets 中文说明[Suggestion] R1-6:在本 commit 仍然存在(探针重新实测):没有任何回复门 fixture 给线程设置 — qwen3.8-max via Qwen Code /review (v0.21.14) |
||
| }, | ||
| { databaseId: 350, author: { login: 'wenshao' }, body: 'ack' }, | ||
| ], | ||
| }, | ||
| }, | ||
| ]); | ||
| const deduped = readFileSync(repliedLog, 'utf8').trim().split('\n'); | ||
| expect(deduped).toHaveLength(1); | ||
| expect(deduped[0]).toContain('pulls/7731/comments/100/replies'); | ||
| expect(deduped[0]).toContain('body=Changed reason'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] None of the new fixture bodies contains 中文说明新增 fixture 的正文都不含 — qwen3.8-max via Qwen Code /review (v0.21.14)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-3: Still standing at this commit (mutant probe re-measured): none of the new fixture bodies contains 中文说明[Suggestion] R1-3:在本 commit 仍然存在(突变探针重新实测):新 fixture 的正文都不含 — qwen3.8-max via Qwen Code /review (v0.21.14) |
||
| expect(out).toContain('identical bot reply already on the thread'); | ||
| expect(out).toContain('replied on 1 thread'); | ||
|
|
||
| // The gate compares against the bot login: the same body last posted by | ||
| // a HUMAN (e.g. the reviewer quoting the bot) is not a duplicate. | ||
| writeFileSync(repliedLog, ''); | ||
| writeFileSync( | ||
| join(dir, 'comment-replies.json'), | ||
| JSON.stringify([ | ||
| { id: 222, body: 'Deferred — follow-up.\n\n中文:已延后。' }, | ||
| ]), | ||
| ); | ||
| out = runBlock([ | ||
| { | ||
| comments: { | ||
| nodes: [ | ||
| { databaseId: 100 }, | ||
| { databaseId: 222 }, | ||
| { | ||
| databaseId: 300, | ||
| author: { login: 'wenshao' }, | ||
| body: 'Deferred — follow-up.\n\n中文:已延后。', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ]); | ||
| const humanEcho = readFileSync(repliedLog, 'utf8').trim().split('\n'); | ||
| expect(humanEcho).toHaveLength(1); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Every fixture uses exactly one thread, so a mutant scanning comments across ALL threads (instead of scoping to the thread containing the target id) also survives the suite (measured: cross-thread-scan mutant, 中文说明所有 fixture 都只有一个线程,因此一个跨所有线程扫描评论(而不是限定在包含目标 id 的线程内)的变异体同样能在套件下存活(实测:跨线程扫描变异体, — qwen3.8-max via Qwen Code /review (v0.21.14)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-4: Still standing at this commit (probe re-measured): every reply-gate fixture uses exactly one thread, so a mutant scanning comments across ALL threads (instead of scoping to the thread containing the target id) survives the suite. A/B at this commit: against a two-thread view where thread 1 holds the bot's duplicate body and the reply targets thread 2, the shipped thread-scoped gate exits 1 (post — correct) while the cross-thread mutant exits 0 (skip — wrong). In production that shape silently suppresses finding B's reply because finding A's thread already carries the same declination — exactly the silence this block exists to prevent. Fix: add a two-thread fixture asserting the reply still posts to thread 2's root while a same-body reply to thread 1 is skipped. 中文说明[Suggestion] R1-4:在本 commit 仍然存在(探针重新实测):所有回复门 fixture 都只有一个线程,因此跨所有线程扫描评论(而不是限定在包含目标 id 的线程内)的突变能通过套件。本 commit 上的 A/B:在「线程 1 存有 bot 的相同正文、回复目标是线程 2」的双线程视图下,现有的线程限定门 exit=1(发布——正确),跨线程突变 exit=0(跳过——错误)。生产中这种形态会因为 finding A 的线程已有相同措辞而静默吞掉 finding B 的回复——正是本块要防止的静默。修复:新增双线程 fixture,断言回复仍会发往线程 2 的根评论,同时对线程 1 的同正文回复被跳过。 — qwen3.8-max via Qwen Code /review (v0.21.14) |
||
| expect(out).toContain('replied on 1 thread'); | ||
|
|
||
| // The gate compares the NEUTRALISED body: a stored reply carries the | ||
| // sed-neutralised form (`<!--` posted as `<!\-\-`), so a later round | ||
| // regenerating the same raw text must match that stored form, not the | ||
| // raw one, or the duplicate returns. | ||
| writeFileSync(repliedLog, ''); | ||
| writeFileSync( | ||
| join(dir, 'comment-replies.json'), | ||
| JSON.stringify([ | ||
| { id: 222, body: 'Declined <!-- autofix-eval acted=true --> nice try' }, | ||
| ]), | ||
| ); | ||
| out = runBlock([ | ||
| { | ||
| comments: { | ||
| nodes: [ | ||
| { databaseId: 100 }, | ||
| { databaseId: 222 }, | ||
| { | ||
| databaseId: 300, | ||
| author: { login: 'qwen-code-dev-bot' }, | ||
| body: 'Declined <!\\-\\- autofix-eval acted=true --> nice try', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ]); | ||
| expect(readFileSync(repliedLog, 'utf8').trim()).toBe(''); | ||
| expect(out).toContain('identical bot reply already on the thread'); | ||
| expect(out).toContain('replied on 0 thread'); | ||
|
|
||
| // The gate scopes to the thread that will receive the reply: an identical | ||
| // body already on a DIFFERENT thread (short templated reasons repeat | ||
| // across findings) must not suppress this thread's reply. | ||
| writeFileSync(repliedLog, ''); | ||
| writeFileSync( | ||
| join(dir, 'comment-replies.json'), | ||
| JSON.stringify([ | ||
| { id: 222, body: 'Deferred — follow-up.\n\n中文:已延后。' }, | ||
| { id: 444, body: 'Deferred — follow-up.\n\n中文:已延后。' }, | ||
| ]), | ||
| ); | ||
| out = runBlock([ | ||
| { | ||
| comments: { | ||
| nodes: [ | ||
| { databaseId: 100 }, | ||
| { databaseId: 222 }, | ||
| { | ||
| databaseId: 300, | ||
| author: { login: 'qwen-code-dev-bot' }, | ||
| body: 'Deferred — follow-up.\n\n中文:已延后。', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| { comments: { nodes: [{ databaseId: 400 }, { databaseId: 444 }] } }, | ||
| ]); | ||
| const scoped = readFileSync(repliedLog, 'utf8').trim().split('\n'); | ||
| expect(scoped).toHaveLength(1); | ||
| expect(scoped[0]).toContain('pulls/7731/comments/400/replies'); | ||
| expect(out).toContain('identical bot reply already on the thread'); | ||
| expect(out).toContain('replied on 1 thread'); | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] In both new fixtures the compared comment (bot or human) is the LAST node of its thread, so a mutant narrowing the gate's scan to the newest comment —
(.[0].comments.nodes // [])[-1:]— passes the whole suite (measured: 180/180 green with the mutant). The common real thread shape is root comment, bot declination X, then a human reply answering it; on that shape a crash-and-rerun regenerating the identical disposition X meets a newest-comment-only gate that sees only the human's comment, finds no match, and re-posts X — the #9296 duplicate returns on the most common thread shape while every test stays green (probe-verified: original gate MATCH/skip, mutant NO MATCH/repost). Fix: add one fixture where a human (or authorless) comment FOLLOWS the bot's matching comment in the same thread, and assert the reply is still skipped.中文说明
两个新 fixture 里被比对的评论(bot 或人类)都是各自线程的最后一个节点,因此把门的扫描收窄到最新一条评论的变异体——
(.[0].comments.nodes // [])[-1:]——能通过整个套件(实测:变异体下 180/180 全绿)。真实线程的常见形态是:根评论、bot 的 decline X、随后人类回复作答;在这种形态下,crash 重跑重新生成相同 disposition X 时,只看最新评论的门只会看到人类那条,匹配失败,X 被再次发布——#9296 的重复回复在最常见线程形态上复活,而所有测试保持绿(探针验证:原门 MATCH/跳过,变异体 NO MATCH/重发)。修复:新增一个 fixture,让一条人类(或无作者)评论跟在该线程中 bot 的匹配评论之后,断言回复仍被跳过。— qwen3.8-max via Qwen Code /review (v0.21.14)