-
Notifications
You must be signed in to change notification settings - Fork 3.1k
refactor(review): build the incremental scope from the PR's diff, not a check #9267
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
c50f173
a4339fe
0806cbc
bba8deb
1a55008
1e592dc
59207d8
356aecf
ceccd55
62572c8
2dde9e6
cef5952
b66bded
8c5d94b
c25efbe
8ae07e3
45bf8fc
9d53584
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1541,11 +1541,13 @@ export function buildRoleBrief( | |
| `\`${wt}\`. Do not \`cd\` elsewhere and do not build the user's main checkout.`, | ||
| ); | ||
| } | ||
| // On a delta-scoped incremental round the probe's range must match the | ||
| // round's scope: test-efficacy recomputes its own diff as base..HEAD, and | ||
| // handed the merge base it would reverse hunks and delete mutants from | ||
| // commits an earlier round already reviewed — spending the probe budget | ||
| // out of scope and reporting survivors this round's diff never contains. | ||
| // On a narrowed incremental round the probe's range must cover the | ||
| // published scope: test-efficacy recomputes its own diff as base..HEAD. | ||
| // The published hunks are hunks of `diffBase..head` — the merge-base | ||
| // range the producer assembled them from — so that range covers every | ||
| // one of them and never a byte the PR's diff does not display; the | ||
|
Comment on lines
+1546
to
+1548
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] R4-4: Welding Agent 7's probe Failure scenario: long-lived PR, round N; file F edited in earlier rounds (which added safety-verb statements at earlier line positions) and again this round. The published scope is this round's hunks only, but the probe recomputes Witness (executed — verifier probe driving the real Consistent with Agent 7's own efficacy run on this small PR (5 mutants / 6 hunks Suggested fix: constrain the probe to the published scope where the data already exists — the plan report test-efficacy reads carries per-file 中文说明[Suggestion] R4-4:把 Agent 7 探针的 失败场景:长命 PR 的第 N 轮;文件 F 在更早的轮次中被编辑过(在较早行位置加入过安全动词语句),本轮又被编辑。发布范围只含本轮 hunk,但探针对 F 重算 见证(已执行——验证者探针用长命 PR 形状驱动真实 建议修复:在数据已经存在的地方把探针约束到发布范围——test-efficacy 读取的 plan 报告携带逐文件的 — qwen3.8-max via Qwen Code /review (v0.21.12)
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. Deferred to the next round. The soundness half of this finding is already documented in code (the comment at the weld site states the superset argument), and the budget cost is real — but the two available fixes differ sharply in size. Restoring the trade-off warning is a comment; constraining the probe to the published scope needs a new seam (per-file scope ranges from the fetch report into test-efficacy's mutant/hunk selection) with its own tests. This round's batch budget went to the Critical (R4-1) and the battery-power scenarios first; the probe-scope fix is queued for the next round, where the seam can be tested on its own. Until then the spend is not silent: 中文说明推迟至下一轮。本发现中「可靠性」的一半已在代码中记录在案(焊接处的注释已陈述超集论证),预算代价也确实存在——但两种可行修复的体量相差悬殊。把权衡警告写回原处只是一条注释;而把探针约束到发布范围需要一条新的数据接缝(从 fetch 报告把逐文件范围传给 test-efficacy 的 mutant/hunk 选择),并需要配套的测试。本轮的批处理预算优先用于 Critical(R4-1)与各测试组威力场景;探针范围的修复排入下一轮,届时该接缝可以单独获得测试。在此之前,这部分消耗并非无声无息:每一份 efficacy 报告都会通过 |
||
| // anchor range, by contrast, can carry hunks an undo round netted out | ||
| // of the PR's diff, which no comment can anchor on. | ||
| const inc = report.incremental as | ||
| | { effective?: unknown; upToDate?: unknown; diffBase?: unknown } | ||
| | undefined; | ||
|
|
||
This file was deleted.
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] R4-4 (re-post, still stands): welding Agent 7's probe
--baseto the merge base re-expands test-efficacy's mutant/hunk selection to the PR's entire range on every effective incremental round — the out-of-scope budget spend this thread named. Deferred in round 4 ("the two available fixes differ sharply in size"); unfixed at this commit: on narrowed roundsdiffBaseis the merge base (fetch-pr.ts publish arm) and this weld feedsmergeBase..HEADto the probe. — Failure scenario: on a large long-lived PR, every effective incremental round spends the probe/mutant-selection budget over the PR's entire range rather than the round's scope.Witness: not run — code inspection at the reviewed commit:
anchor.incremental.diffBase = mergeBaseShaon the publish arm, andshaOrNull(inc.diffBase) ?? shaOrNull(report.mergeBaseSha)here.As discussed in the thread: feed test-efficacy the published diff/plan instead of recomputing a second range, or weld the probe base to the anchor range on narrowed rounds with hunk-level filtering against the published plan.
中文说明
[建议] R4-4(重新发布,仍然存在):把 Agent 7 探针的
--base焊到 merge base,会使 test-efficacy 的突变体/hunk 选择在每个有效增量轮重新扩张到 PR 的整个区间——即本线程指出的范围外预算开销。第 4 轮已推迟处理(「两种可行修复的规模差异很大」);本提交仍未修复:收窄轮次中diffBase即 merge base(fetch-pr.ts 的发布分支),此焊点把mergeBase..HEAD交给探针。— 失败场景:在大型长命 PR 上,每个有效增量轮都把探针/突变体选择的预算花在整个 PR 区间上,而不是本轮范围上。见证:未运行——在受审提交上直接读码:发布分支写入
anchor.incremental.diffBase = mergeBaseSha,此处读取shaOrNull(inc.diffBase) ?? shaOrNull(report.mergeBaseSha)。按线程中的讨论:把已发布的 diff/plan 交给 test-efficacy,而不是重算第二个区间;或在收窄轮次把探针 base 焊到锚点区间,并按已发布 plan 做 hunk 级过滤。
— qwen3.8-max via Qwen Code /review (v0.21.13)
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.
Deferred to the follow-up queue (verified real, fix out of this PR's footprint). The claim is confirmed at this commit by code inspection:
fetch-pr.ts's publish arm setsanchor.incremental.diffBase = mergeBaseSha, so the incremental arm inagent-prompt.ts(shaOrNull(inc.diffBase) ?? shaOrNull(report.mergeBaseSha)) always resolves to the merge base, and on every effective incremental round the test-efficacy probe recomputesmergeBase..HEAD— spending mutant/hunk selection over the PR's entire range rather than the round's scope. Both fixes discussed in this thread (feeding test-efficacy the published diff/plan, or welding the probe base to the anchor range with hunk-level filtering against the published plan) require changingtest-efficacy.ts's own range recomputation, a file this PR never touches, and they differ sharply in size — so the choice belongs to a follow-up rather than this PR's remaining rounds. The thread stays open and the finding is recorded in the deferred-findings queue so it survives merge.中文说明
推迟到后续跟进队列(已核实属实,修复超出本 PR 的 footprint)。通过在本提交上读码确认该论断:
fetch-pr.ts的发布分支把anchor.incremental.diffBase设为mergeBaseSha,因此agent-prompt.ts的增量分支(shaOrNull(inc.diffBase) ?? shaOrNull(report.mergeBaseSha))总是落到 merge base,每个有效增量轮中 test-efficacy 探针都重算mergeBase..HEAD——把突变体/hunk 选择预算花在整个 PR 区间上,而不是本轮范围。线程中讨论的两种修复(把已发布的 diff/plan 交给 test-efficacy,或把探针 base 焊到锚点区间并按已发布 plan 做 hunk 级过滤)都需要修改test-efficacy.ts自身的区间重算——该文件不在本 PR 改动范围内——且两者规模差异很大,因此取舍应由后续 PR 决定,不属于本 PR 的剩余轮次。线程保持打开,发现已记入 deferred-findings 队列,合并后不会丢失。