-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(review): correct the round-cap marker lifecycle and stale cap docs #8850
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
bf999e8
153598f
2f99876
0b23361
93ff7ca
52590f5
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3445,6 +3445,10 @@ describe('per-chunk retirement — cold territories stop costing a round', () => | |||||||||||||
| expect(out).toContain('2 auditors required this round'); | ||||||||||||||
| expect(out).toContain('chunk 13 — retired: dry in rounds 1 and 2'); | ||||||||||||||
| expect(out).toContain('certificate final'); | ||||||||||||||
| // Pin the spelled cap number, not just the branch: a hardcoded `5-round | ||||||||||||||
| // cap leaves` in the note wording would otherwise ship silently and tell | ||||||||||||||
| // the orchestrator a false cap on exactly the huge-diff runs this targets. | ||||||||||||||
| expect(out).toContain('3-round cap leaves'); | ||||||||||||||
| expect(out).not.toContain('next cold check round 4'); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -3472,6 +3476,19 @@ describe('per-chunk retirement — cold territories stop costing a round', () => | |||||||||||||
| .join('\n'); | ||||||||||||||
| expect(msg).toContain('ROUND CAP'); | ||||||||||||||
| expect(msg).toContain('round cap is 3'); | ||||||||||||||
| // The load-bearing tail rules — the same verify-only / compose-floor | ||||||||||||||
| // contract the budget message's test pins and SKILL.md's round-cap | ||||||||||||||
| // bullet mirrors; a reword that drops any of these silently loosens | ||||||||||||||
| // the termination contract, so pin each. | ||||||||||||||
| expect(msg).toContain('agent-prompt --role verify'); | ||||||||||||||
| expect(msg).toContain('never a hand-rolled agent'); | ||||||||||||||
| expect(msg).toContain('compose floor'); | ||||||||||||||
| expect(msg).toContain('Do NOT re-verify findings already'); | ||||||||||||||
|
Comment on lines
+3483
to
+3486
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] Round-cap refusal message's wait-bound clauses not pinned — Failure scenario: A reword that drops either clause from the round-cap refusal message would pass the test suite, silently loosening the termination contract for the cap gate.
Suggested change
中文说明round-cap 拒绝消息的 wait-bound 从句未被测试固定——失败场景:删除 round-cap 拒绝消息中任何一个从句的改写将不会导致测试失败,从而静默地放宽了 cap 门的终止契约。建议在 — deepseek-v4-flash via Qwen Code /review (v0.21.8) |
||||||||||||||
| // The wait-bound and no-fresh-pass clauses too — the budget message's | ||||||||||||||
| // test pins the same two for the sibling refusal; one bounded-tail | ||||||||||||||
| // protocol, both pin both. | ||||||||||||||
| expect(msg).toContain('stop waiting on any verifier batch still out'); | ||||||||||||||
| expect(msg).toContain('invent a fresh re-verification pass'); | ||||||||||||||
| // The marker is on disk so compose-review caps without the relay. | ||||||||||||||
| expect(readBudgetStop(plan)?.cause).toBe('round-cap'); | ||||||||||||||
| expect(readBudgetStop(plan)?.cap).toBe(3); | ||||||||||||||
|
|
@@ -3592,6 +3609,165 @@ describe('per-chunk retirement — cold territories stop costing a round', () => | |||||||||||||
| expect(readRoundStamps(plan)).toHaveLength(stampsBefore); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it('huge cap: a converged past-cap round exits 5, not the cap — convergence outranks it', () => { | ||||||||||||||
| // The ordering the PR documents four times (the convergence check runs | ||||||||||||||
| // BEFORE the round-cap gate) with no test pin: hoisting the cap check | ||||||||||||||
| // above it survives the whole suite. Round 5 is past the cap of 3, but | ||||||||||||||
| // its schedule has converged (every chunk twice-dry, odd round → all | ||||||||||||||
| // skipped), so it must exit 5 CONVERGED with NO marker — not exit 4 at | ||||||||||||||
| // the cap. History that lands convergence on an odd past-cap round: 13/14 | ||||||||||||||
| // dry in rounds 1-2 (retire at 3), 15 whiffs round 1 then goes dry in | ||||||||||||||
| // 2-3, so round 3 (odd) builds only 15 and nothing converges before 5. | ||||||||||||||
| writeFileSync( | ||||||||||||||
| plan, | ||||||||||||||
| JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), | ||||||||||||||
| ); | ||||||||||||||
| const old = new Date(2020, 0, 1); | ||||||||||||||
| utimesSync(plan, old, old); | ||||||||||||||
| runRound(1); | ||||||||||||||
| auditorTranscript(recordOf(1, 13), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 14), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); | ||||||||||||||
| answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
| answerRound(3, { 15: DRY }); // 13,14 retired (odd → skipped); only 15 built | ||||||||||||||
| expect(keysOf(3)).toHaveLength(1); | ||||||||||||||
|
|
||||||||||||||
| const out = runRound(5); // 5 > cap 3, but the schedule has converged | ||||||||||||||
| expect(process.exitCode).toBe(5); | ||||||||||||||
| expect(out).toBe(''); | ||||||||||||||
| const msg = (writeStderrLine as unknown as Mock).mock.calls | ||||||||||||||
| .map((c) => c[0]) | ||||||||||||||
| .join('\n'); | ||||||||||||||
| expect(msg).toContain('CONVERGED'); | ||||||||||||||
| // Convergence outranks the cap: no round-cap refusal, no marker written. | ||||||||||||||
| expect(readBudgetStop(plan)).toBeNull(); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it('huge cap: a CONVERGED exit clears a stale same-run round-cap marker', () => { | ||||||||||||||
| // Retry-after-refusal: round 4 (even) is refused at the cap — every | ||||||||||||||
| // retired chunk is DUE a cold check, so the schedule is not converged and | ||||||||||||||
| // 4 > 3 refuses, writing the marker. The orchestrator then asks for round | ||||||||||||||
| // 5, which converges. Nothing else unlinks budget-stop.json, so without | ||||||||||||||
| // the converged-branch clear the stale marker caps a verdict that | ||||||||||||||
| // legitimately converged. | ||||||||||||||
| writeFileSync( | ||||||||||||||
| plan, | ||||||||||||||
| JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), | ||||||||||||||
| ); | ||||||||||||||
| const old = new Date(2020, 0, 1); | ||||||||||||||
| utimesSync(plan, old, old); | ||||||||||||||
| runRound(1); | ||||||||||||||
| auditorTranscript(recordOf(1, 13), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 14), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); | ||||||||||||||
| answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
| answerRound(3, { 15: DRY }); | ||||||||||||||
|
|
||||||||||||||
| runRound(4); // even → retired chunks due cold checks → not converged → cap refuses | ||||||||||||||
| expect(process.exitCode).toBe(4); | ||||||||||||||
| expect(readBudgetStop(plan)?.cause).toBe('round-cap'); | ||||||||||||||
|
|
||||||||||||||
| process.exitCode = undefined; | ||||||||||||||
| const out = runRound(5); // odd → all skipped → converged | ||||||||||||||
| expect(process.exitCode).toBe(5); | ||||||||||||||
| expect(out).toBe(''); | ||||||||||||||
| const msg = (writeStderrLine as unknown as Mock).mock.calls | ||||||||||||||
| .map((c) => c[0]) | ||||||||||||||
| .join('\n'); | ||||||||||||||
| expect(msg).toContain('CONVERGED'); | ||||||||||||||
| // The marker channel is closed AND the relay channel is recalled: the | ||||||||||||||
| // refusal instructed the orchestrator to add the stop entry to | ||||||||||||||
| // unreviewedDimensions, and nothing but this sentence removes it once | ||||||||||||||
| // the marker (and with it compose-review's dedup splice) is gone. | ||||||||||||||
| expect(msg).toContain('remove it now — this convergence supersedes'); | ||||||||||||||
| expect(readBudgetStop(plan)).toBeNull(); // the stale marker is cleared | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it('huge cap: a CONVERGED exit clears a stale same-run time-budget marker too', () => { | ||||||||||||||
| // The clear is cause-blind, but both sibling clear tests produce their | ||||||||||||||
| // marker via the round-cap gate — a cause-conditional clear | ||||||||||||||
| // (`if (readBudgetStop(p)?.cause === 'round-cap') clearBudgetStop(p)`) | ||||||||||||||
| // passes them both and leaves a time-budget marker capping a verdict | ||||||||||||||
| // the audit legitimately converged. Cap 5 so even round 4 reaches the | ||||||||||||||
| // TIME gate instead of the cap gate: cold checks due → not converged → | ||||||||||||||
| // admitted at the cap, refused at the near deadline. Round 5 then | ||||||||||||||
| // converges and must clear the time-budget marker the same way. | ||||||||||||||
| writeFileSync( | ||||||||||||||
| plan, | ||||||||||||||
| JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 5 } }), | ||||||||||||||
| ); | ||||||||||||||
| const old = new Date(2020, 0, 1); | ||||||||||||||
| utimesSync(plan, old, old); | ||||||||||||||
| runRound(1); | ||||||||||||||
| auditorTranscript(recordOf(1, 13), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 14), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); | ||||||||||||||
| answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
| answerRound(3, { 15: DRY }); | ||||||||||||||
|
|
||||||||||||||
| process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); | ||||||||||||||
| runRound(4); // even → not converged → 4 <= cap 5 → refused at the time gate | ||||||||||||||
| expect(process.exitCode).toBe(4); | ||||||||||||||
| expect(readBudgetStop(plan)?.entry).toBe( | ||||||||||||||
| 'reverse audit — stopped before round 4 by the review time budget', | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| process.exitCode = undefined; | ||||||||||||||
| const out = runRound(5); // odd → all skipped → converged, before any gate | ||||||||||||||
| expect(process.exitCode).toBe(5); | ||||||||||||||
| expect(out).toBe(''); | ||||||||||||||
| const msg = (writeStderrLine as unknown as Mock).mock.calls | ||||||||||||||
| .map((c) => c[0]) | ||||||||||||||
| .join('\n'); | ||||||||||||||
| expect(msg).toContain('CONVERGED'); | ||||||||||||||
| expect(readBudgetStop(plan)).toBeNull(); // the stale time-budget marker is cleared | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it('huge cap: a converged --chunk retry clears the stale cap marker too', () => { | ||||||||||||||
| // The --chunk gate threads the same convergence-first path with its own | ||||||||||||||
| // `args.plan`, but only the --all-chunks site's marker clear is pinned | ||||||||||||||
| // above: a converged per-chunk retry after a cap refusal must exit 5 | ||||||||||||||
| // CONVERGED and clear the stale marker exactly like it, not exit 4 at | ||||||||||||||
| // the cap (the ordering) and not leave the marker capping a verdict the | ||||||||||||||
| // audit legitimately converged (the clear). Same retry-after-refusal | ||||||||||||||
| // history as the --all-chunks test. | ||||||||||||||
| writeFileSync( | ||||||||||||||
| plan, | ||||||||||||||
| JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), | ||||||||||||||
| ); | ||||||||||||||
| const old = new Date(2020, 0, 1); | ||||||||||||||
| utimesSync(plan, old, old); | ||||||||||||||
| runRound(1); | ||||||||||||||
| auditorTranscript(recordOf(1, 13), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 14), DRY); | ||||||||||||||
| auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); | ||||||||||||||
| answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
| answerRound(3, { 15: DRY }); | ||||||||||||||
|
|
||||||||||||||
| runRound(4); // even → retired chunks due cold checks → cap refuses | ||||||||||||||
| expect(process.exitCode).toBe(4); | ||||||||||||||
| expect(readBudgetStop(plan)?.cause).toBe('round-cap'); | ||||||||||||||
|
|
||||||||||||||
| process.exitCode = undefined; | ||||||||||||||
| (writeStdoutLine as unknown as Mock).mockClear(); | ||||||||||||||
| (writeStderrLine as unknown as Mock).mockClear(); | ||||||||||||||
| (agentPromptCommand.handler as (a: unknown) => void)({ | ||||||||||||||
| plan, | ||||||||||||||
| role: 'reverse-audit', | ||||||||||||||
| findings, | ||||||||||||||
| chunk: 13, | ||||||||||||||
| round: 5, | ||||||||||||||
| }); | ||||||||||||||
| expect(process.exitCode).toBe(5); | ||||||||||||||
| expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); | ||||||||||||||
| expect(keysOf(5)).toHaveLength(0); | ||||||||||||||
| const msg = (writeStderrLine as unknown as Mock).mock.calls | ||||||||||||||
| .map((c) => c[0]) | ||||||||||||||
| .join('\n'); | ||||||||||||||
| expect(msg).toContain('CONVERGED'); | ||||||||||||||
| expect(readBudgetStop(plan)).toBeNull(); // the stale marker is cleared | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it('a cold-check-only round is still built, admitted and stamped', () => { | ||||||||||||||
| answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
| answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ import { dirname, join, resolve } from 'node:path'; | |
| import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; | ||
| import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; | ||
| import { | ||
| clearBudgetStop, | ||
| expectedRoundSeconds, | ||
| readRoundStamps, | ||
| reverseAuditBudgetExhausted, | ||
|
|
@@ -1885,8 +1886,14 @@ function admitReverseAuditRound( | |
| `not rebuild or retry. A marker has been recorded and compose-review ` + | ||
| `will cap the verdict; still add \`${roundCapStopEntry(cap)}\` to ` + | ||
| `unreviewedDimensions so the terminal report agrees. If the cap ` + | ||
| `round reported findings whose verdicts have not landed, launch ` + | ||
| `their verifiers alone and wait, then proceed to Step 6.`, | ||
| `round reported findings whose verdicts have not landed, verify them ` + | ||
| `ONLY through \`agent-prompt --role verify\` (never a hand-rolled ` + | ||
| `agent) — it is gated on the compose floor and will refuse once too ` + | ||
| `little time remains; when the deadline is within that floor, stop ` + | ||
| `waiting on any verifier batch still out and compose with the tags ` + | ||
|
Comment on lines
+1892
to
+1893
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] The bounded-tail protocol this PR mirrors into the ROUND CAP message is duplicated in 中文说明[建议] 本 PR 镜像进 ROUND CAP 消息的有界收尾协议,在 — qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| `in hand. Do NOT re-verify findings already confirmed in earlier ` + | ||
| `rounds, and do NOT invent a fresh re-verification pass. Then ` + | ||
| `proceed to Step 6.`, | ||
| ); | ||
| process.exitCode = 4; | ||
| return false; | ||
|
|
@@ -1909,13 +1916,27 @@ function admitReverseAuditRound( | |
| * twice over, so another round would audit nothing the history has not | ||
| * already answered. Not an error and not a gap — no record, no stamp, no | ||
| * disclosure owed; a round that builds nothing was never admitted. | ||
| * | ||
| * Clears any same-run stop marker first: a converged exit can follow an | ||
| * over-cap round the gate already refused (round 4 refused under cap 3, | ||
|
Comment on lines
+1920
to
+1921
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] The CONVERGED-exit clear closes the marker channel but leaves the relay channel open: nothing tells the orchestrator to drop the round-cap 中文说明[建议] CONVERGED 退出时的清标记只关闭了标记通道, relay 通道仍然敞着:没有任何提示让编排器丢弃此前拒绝消息明确要求它添加的 round-cap — qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| * then round 5's schedule is converged — the convergence check runs before | ||
| * the cap gate), and that stale round-cap marker would otherwise cap a | ||
| * verdict the audit legitimately converged. The message also recalls the | ||
| * relay channel: the earlier refusal told the orchestrator to add its stop | ||
| * entry to unreviewedDimensions, and once the marker is gone the | ||
| * compose-review splice that dedups it no longer runs — only this | ||
| * instruction removes it. | ||
| */ | ||
| function refuseConverged(): void { | ||
| function refuseConverged(planPath: string): void { | ||
| clearBudgetStop(planPath); | ||
|
Comment on lines
+1930
to
+1931
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] The cause-blind clear on a CONVERGED exit is only tested against round-cap markers — both new clear tests produce their marker via a cap-3 round-4 refusal, so a future cause-conditional clear ( Fix: add a sibling test beside the two existing clear tests — same retirement history under cap 5 with a near deadline so even round 4 is refused at the time gate (assert the time-budget marker is written), then run odd round 5 and assert exit 5, 中文说明[建议] CONVERGED 退出时的无差别清标记目前只有 round-cap 标记的测试覆盖——两个新的清标记测试都通过 cap-3 第 4 轮拒绝来产生标记,因此未来若改成按 cause 条件清标记( — qwen3.8-max via Qwen Code /review (v0.21.8) |
||
| writeStderrLine( | ||
| 'CONVERGED: every chunk holds two consecutive substantive dry audits; ' + | ||
| 'the reverse audit has converged — stop the loop and proceed to ' + | ||
| 'Step 6. This is a clean convergence, not a gap: no ' + | ||
| 'unreviewedDimensions entry is owed.', | ||
| 'unreviewedDimensions entry is owed. If an earlier round-cap or ' + | ||
| 'budget refusal told you to add its stop entry to ' + | ||
| 'unreviewedDimensions, remove it now — this convergence supersedes ' + | ||
| 'it.', | ||
| ); | ||
| process.exitCode = 5; | ||
| } | ||
|
|
@@ -1968,7 +1989,7 @@ function runAllChunks( | |
| } | ||
|
|
||
| if (schedule !== null && schedule.converged) { | ||
| refuseConverged(); | ||
| refuseConverged(planPath); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -2470,7 +2491,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { | |
| schedule = null; | ||
| } | ||
| if (schedule !== null && schedule.converged) { | ||
| refuseConverged(); | ||
| refuseConverged(args.plan); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
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] The round-cap refusal's two new bounded-tail clauses are pinned only against
reverseAuditBudgetMessage(deadline.test.ts:481-482), not at this round-cap refusal site — the deadline.test.ts comment this diff adds claims "both pin both", but the round-cap side of that symmetry does not exist. — Failure scenario: probe-verified mutation — deleting bothstop waiting on any verifier batch still outandinvent a fresh re-verification passfrom the round-cap refusal in agent-prompt.ts alone ships green (agent-prompt.test.ts 13 passed, deadline.test.ts 43 passed, SKILL.test.ts 7 passed), silently loosening the bounded-tail termination contract on exactly the huge-diff cap-stopped runs.中文说明
[建议] round-cap 拒绝消息新增的两条有界收尾子句目前只钉在了
reverseAuditBudgetMessage(deadline.test.ts:481-482)上,没有钉在这个 round-cap 拒绝消息的位置——本 diff 在 deadline.test.ts 新增的注释声称 "both pin both",但那个对称的 round-cap 一侧并不存在。— 失败场景:经探针验证的变异——仅从 agent-prompt.ts 的 round-cap 拒绝消息中删除stop waiting on any verifier batch still out与invent a fresh re-verification pass两条子句,整个套件仍为绿色(agent-prompt.test.ts 13 通过、deadline.test.ts 43 通过、SKILL.test.ts 7 通过),恰好在大 diff 触发上限停止的运行时悄悄放松了有界收尾的终止契约。— qwen3.8-max via Qwen Code /review (v0.21.9)