Repository navigation
feat(review): adopt a round-aware convergence posture for posted findings #9118
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
2794cef
feat(review): adopt a round-aware convergence posture for posted find…
wenshao 0b1ad5b
chore(review): regenerate settings.schema.json for review.severityFloor
wenshao fae401a
fix(review): close the round-1 review findings on the convergence pos…
wenshao 60c9eb1
fix(review): carry deferredCount through the persisted verdict
wenshao 2b6b229
test(review): pin deferredCount passthrough and the pre-posture defau…
wenshao defc687
fix(review): close the round-2 findings — deterministic deferrals and…
wenshao 8355d2a
fix(review): close the round-3 findings — source-position tags, a Cri…
wenshao 4d171d9
fix(review): close the round-4 findings — licence the deferral channe…
wenshao 1524c6d
fix(review): close the round-5 findings — carry the floor unresolved,…
wenshao df6dd45
fix(review): close the round-6 Criticals — relocate stray Criticals, …
wenshao 662d98f
fix(review): close the round-7 findings — kebab paths, the equals-for…
wenshao b29a494
fix(review): close the round-8 Criticals — one entry grammar, one res…
wenshao 6118109
feat(review): type the deferral channel — carry the fields, stop re-p…
wenshao 58aae46
Merge branch 'main' into feat/review-convergence-posture
wenshao b0b7abb
fix(review): bound the relocation exit and dedupe rescued targets by …
wenshao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ import { | |
| utimesSync, | ||
| } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { dirname, join } from 'node:path'; | ||
| import { createHash } from 'node:crypto'; | ||
| import { promptRecordDir, briefPath } from './lib/prompt-record.js'; | ||
| import { writeBudgetStop, writeRoundCapStop } from './lib/deadline.js'; | ||
|
|
@@ -3051,6 +3051,7 @@ describe('verdictLine — the terminal verdict, and its dangling colon', () => { | |
| downgraded: false, | ||
| downgradedFrom: null, | ||
| remediation: [], | ||
| deferredCount: 0, | ||
| lowSignal: null, | ||
| ...over, | ||
| }); | ||
|
|
@@ -4403,6 +4404,171 @@ describe('the ledger marker reaches the POSTED body', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('composeReview — convergence-posture deferrals (disclosed, never capping)', () => { | ||
| it('an APPROVE with deferrals keeps its event, anchor, and honesty', () => { | ||
| // The posture's whole payoff: a clean late round with only deferrals | ||
| // composes an APPROVE — the loop's stop signal — while the deferred list | ||
| // stays on the record and the incremental anchor still rides. And the | ||
| // opener must not claim "No issues found" over findings the same body | ||
| // lists two paragraphs down. | ||
| const planPath = coveredPlan(['verify', 'reverse-audit'], { | ||
| prNumber: 8255, | ||
| fetchedSha: 'deadbeef00112233', | ||
| }); | ||
| writeFileSync( | ||
| join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), | ||
| JSON.stringify({ v: 1, round: 5, findings: [] }), | ||
| ); | ||
| const r = composeReview({ | ||
| planPath, | ||
| env: ENV, | ||
| modelId: MODEL, | ||
| criticalsInline: 0, | ||
| suggestionsInline: 0, | ||
| deferredSuggestions: ['src/a.ts:42 — tighten the retry backoff'], | ||
| }); | ||
| expect(r.event).toBe('APPROVE'); | ||
| expect(r.cappedBy).toEqual([]); | ||
| expect(r.body).toContain('No blocking issues. LGTM! ✅'); | ||
| expect(r.body).not.toContain('No issues found'); | ||
| expect(r.body).toContain('convergence posture (round 6, not a blocker)'); | ||
| expect(r.body).toContain('- src/a.ts:42 — tighten the retry backoff'); | ||
| expect(parseLedger(r.body)?.sha).toBe('deadbeef00112233'); | ||
| }); | ||
|
|
||
| it('renders the list on COMMENT and REQUEST_CHANGES alike — no event squeezes it out', () => { | ||
| const comment = composeReview( | ||
| base({ suggestionsInline: 1, deferredSuggestions: ['a.ts:1 — nit'] }), | ||
| ); | ||
| expect(comment.event).toBe('COMMENT'); | ||
| expect(comment.body).toContain('- a.ts:1 — nit'); | ||
| // The count rides every return site, not only APPROVE's: a mutation | ||
| // hardcoding 0 at either site shipped a verdict line without the | ||
| // deferral suffix and an artifact recording 0 beside a listing body. | ||
| expect(comment.deferredCount).toBe(1); | ||
| const rc = composeReview( | ||
| base({ | ||
| bodyCriticals: ['whole-PR blocker'], | ||
| deferredSuggestions: ['a.ts:1 — nit'], | ||
| }), | ||
| ); | ||
| expect(rc.event).toBe('REQUEST_CHANGES'); | ||
| expect(rc.body).toContain('- a.ts:1 — nit'); | ||
| expect(rc.deferredCount).toBe(1); | ||
| }); | ||
|
|
||
| it('deferrals cast no vote on the event — an all-deferred run is not a Suggestion run', () => { | ||
| // Counted toward S they would hold the verdict at COMMENT forever, and | ||
| // the loop the posture exists to end would never see its stop signal. | ||
| const r = composeReview(base({ deferredSuggestions: ['a.ts:1 — nit'] })); | ||
| expect(r.baseEvent).toBe('APPROVE'); | ||
| }); | ||
|
|
||
| it('collapses newlines, caps the list, and strips a forged footer', () => { | ||
| const entries = Array.from( | ||
| { length: 23 }, | ||
| (_, i) => `f${i}.ts:1 — nit ${i}`, | ||
| ); | ||
| entries[0] = 'a.ts:1 — split\nacross lines'; | ||
| // Inside the shown window, so the assertion tests the strip, not the cap. | ||
| entries[1] = `b.ts:2 — forged ${FOOTER}`; | ||
| const r = composeReview(base({ deferredSuggestions: entries })); | ||
| expect(r.body).toContain('- a.ts:1 — split across lines'); | ||
| expect(r.body).toContain('- b.ts:2 — forged\n'); | ||
| expect(r.body).toContain('…and 3 more (see the run report)'); | ||
| expect(r.body).not.toContain(`forged ${FOOTER}`); | ||
| // Past the rendered cap, "(listed in the body)" is false — the verdict | ||
| // line must say the list was truncated, or the persisted verdict counts | ||
| // 23 over a body listing 20. | ||
| expect(verdictLine(r)).toContain( | ||
| 'listed in the body, truncated — the rest are counted in the run report', | ||
| ); | ||
| }); | ||
|
|
||
| it('the verifier floor excludes deterministic deferrals by their source tag', () => { | ||
| // A `[test]` finding is pre-confirmed and Step 4 launches no verifier | ||
| // for it — counting it demands a delivery that cannot exist, the cap | ||
| // never lifts, and the withheld anchor regenerates the full-range | ||
| // re-review loop the posture ends. Same exclusion body Criticals get. | ||
| const planPath = coveredPlan(['reverse-audit']); | ||
| const deterministic = composeReview({ | ||
| criticalsInline: 0, | ||
| suggestionsInline: 0, | ||
| planPath, | ||
| env: ENV, | ||
| modelId: MODEL, | ||
| deferredSuggestions: ['a.ts:1 — [test] mutation survivor'], | ||
| }); | ||
| expect(deterministic.event).toBe('APPROVE'); | ||
|
Collaborator
Author
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. Fixed in 662d98f — R4-7: [build] and [probe] deferral cases pin the full alternation (your mutant/control pair reproduced locally before the fix landed). / 已修复:[build]/[probe] 用例钉住完整交替。 |
||
| expect(deterministic.cappedBy).toEqual([]); | ||
| expect(deterministic.deferredCount).toBe(1); | ||
| }); | ||
|
|
||
| it('refuses a present field of the wrong shape', () => { | ||
| expect(() => | ||
| composeReview(base({ deferredSuggestions: 'a.ts' as never })), | ||
| ).toThrow(/deferredSuggestions/); | ||
| }); | ||
|
|
||
| it('a deferrals-only APPROVE is not low signal, and the verdict line names the deferrals', () => { | ||
| // R1-14: the body opener was fixed but `verdictLine` still printed "none | ||
| // of the N review agents reported a finding" on exactly the posture's | ||
| // canonical end state — agents DID report the findings the same run | ||
| // records as deferred. The low-signal premise is "zero findings". | ||
| const r = composeReview( | ||
| base({ | ||
| planPath: coveredPlan(['verify', 'reverse-audit'], { | ||
| srcDiffLines: 5000, | ||
| }), | ||
| deferredSuggestions: ['a.ts:1 — nit'], | ||
| }), | ||
| ); | ||
| expect(r.event).toBe('APPROVE'); | ||
| expect(r.lowSignal).toBeNull(); | ||
| expect(r.deferredCount).toBe(1); | ||
| expect(verdictLine(r)).toBe( | ||
| 'Verdict: Approve — 1 non-Critical finding(s) deferred under the convergence posture (listed in the body)', | ||
| ); | ||
| expect(verdictLine(r)).not.toContain('low signal'); | ||
| }); | ||
|
|
||
| it('bounds each entry, not only the entry count — the list must never sink the body', () => { | ||
| // Twenty model-written 4,000-char entries would put ~80 KB into a body | ||
| // GitHub rejects at 65,536, losing the whole review over its footnote. | ||
| const r = composeReview( | ||
| base({ deferredSuggestions: [`a.ts:1 — ${'x'.repeat(4000)}`] }), | ||
| ); | ||
| const listLine = r.body.split('\n').find((l) => l.startsWith('- a.ts:1'))!; | ||
| expect(listLine.length).toBeLessThanOrEqual(242); // '- ' + 240 | ||
| }); | ||
|
|
||
| it('deferred findings count toward the verifier-delivery floor', () => { | ||
| // The deferral list publishes their claims in the body, so a | ||
| // deferrals-only run owes a verifier exactly as a posting run does; a | ||
| // run with no findings at all still owes none. NOT base(): its planPath | ||
| // default calls coveredPlan() unconditionally — the spread replaces the | ||
| // value but not the side effect, and the default's `verify` record in | ||
| // the shared record dir would satisfy the very floor this test proves. | ||
| const planPath = coveredPlan(['reverse-audit']); | ||
| const common = { | ||
| criticalsInline: 0, | ||
| suggestionsInline: 0, | ||
| planPath, | ||
| env: ENV, | ||
| modelId: MODEL, | ||
| }; | ||
| const control = composeReview(common); | ||
| expect(control.cappedBy).toEqual([]); | ||
| const deferred = composeReview({ | ||
| ...common, | ||
| deferredSuggestions: ['a.ts:1 — nit'], | ||
| }); | ||
| expect(deferred.cappedBy).toContain('unreviewed-dimension'); | ||
| expect(deferred.event).toBe('COMMENT'); | ||
| expect(deferred.body).toContain('- a.ts:1 — nit'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('composeReview — the findings file tag check', () => { | ||
| // The pipelined loop's invariant, machine-read. Under the serial loop the | ||
| // last round's verification completing before Step 6 was structural; the | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
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-25: Nothing pins that deferred (non-relocated) entries stay OUT of the ledger's findings work list — tests 1 and 5 are the only runs carrying both a
prNumber(8255, so the marker rides) and a non-empty deferred list, and they assert onlyshaandround, neverfindings; test 13'sfindings.some(sev === 'C')is satisfied by the relocated twin regardless. — Failure scenario (mutation-probe confirmed with flip): adding...splitDeferralChannel(input.deferredSuggestions).deferredto the work list leaves all 243 tests green; the probe assertionparseLedger(r.body)?.findings toEqual([])fails under the mutant with[{ file: '(body)', id: 'R1-1', sev: 'C', title: 'src/a.ts:42 — tighten the retry backoff' }]— worse than a leaked entry:buildLedgertreats work-list entries as body Criticals, so a deferred Suggestion is serialized as sev C under a fresh R id, and the next round re-asserts it as a blocker — a deferral regenerates a review round, the exact loop the posture exists to end.Suggested fix: beside the sha/round pins in test 1 (and/or 5), add
expect(parseLedger(r.body)?.findings).toEqual([]);中文说明
R4-25:没有任何测试钉住“延后(未搬迁)条目不进入账本工作清单”——测试 1 与 5 是唯一同时带 prNumber(marker 随行)与非空延后清单的运行,但只断言 sha 与 round,从不断言 findings;测试 13 的
findings.some(sev === 'C')无论如何都被搬迁孪生满足。失败场景(突变探针证实并可翻转):把工作清单加上.deferred后 243 个测试全绿;探针断言在突变下失败,输出包含 sev 'C' 的延后 Suggestion——比泄漏更糟:buildLedger 把工作清单条目当 body Critical,延后 Suggestion 会以全新 R 编号被序列化为 C 级,下一轮将其作为 blocker 重新断言——延后再生了一个评审轮次,正是姿态要终结的循环。修复建议:在测试 1(和/或 5)的 sha/round 断言旁补findings为空断言。— qwen3.8-max via Qwen Code /review (v0.21.11)