-
Notifications
You must be signed in to change notification settings - Fork 2.9k
perf(review): extend the convergence pair to 3B (chunked) reviews #8903
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
434e9c7
e3c274d
1da9264
04f1179
876d315
3e231e5
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 |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ import { | |
| DEADLINE_ENV, | ||
| RESERVE_ENV, | ||
| COMPOSE_FLOOR_ENV, | ||
| TOOL_CONCURRENCY_ENV, | ||
| readBudgetStop, | ||
| readRoundStamps, | ||
| } from './lib/deadline.js'; | ||
|
|
@@ -2658,6 +2659,7 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () | |
| afterEach(() => { | ||
| delete process.env[DEADLINE_ENV]; | ||
| delete process.env[RESERVE_ENV]; | ||
| delete process.env[TOOL_CONCURRENCY_ENV]; | ||
| process.exitCode = undefined; | ||
| for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); | ||
| }); | ||
|
|
@@ -3103,6 +3105,59 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () | |
| // A refusal is not an admission. | ||
| expect(readRoundStamps(plan)).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('prices the 3B pair as one admission — round 2 bears the pair wall', () => { | ||
| // Round 2's build lands seconds after round 1's stamp, so nothing has | ||
| // measured a round yet; the price is both members' wall in waves of the | ||
| // tool-concurrency pool. PLAN has three chunks; at a 2-slot pool each | ||
| // round runs two waves and the pair three, so round 2 pays 3/2 of the | ||
| // round estimate — and the gate refuses it when the reserve plus that | ||
| // does not fit, even though round 1 (one estimate) just admitted. | ||
| process.env[TOOL_CONCURRENCY_ENV] = '2'; | ||
| process.env[RESERVE_ENV] = '600'; | ||
| process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3000); | ||
| const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); | ||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(readRoundStamps(plan).some((st) => st.round === 1)).toBe(true); | ||
|
|
||
| (writeStdoutLine as unknown as Mock).mockClear(); | ||
| call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); | ||
| // Reserve 600 + pair price 2700 = 3300 > the 3000 remaining. | ||
| expect(process.exitCode).toBe(4); | ||
| expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); | ||
| expect(readBudgetStop(plan)?.entry).toBe( | ||
| 'reverse audit — stopped before round 2 by the review time budget', | ||
| ); | ||
| expect(readRoundStamps(plan)).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('admits the 3B pair when the reserve plus the pair wall fits', () => { | ||
| process.env[TOOL_CONCURRENCY_ENV] = '2'; | ||
| process.env[RESERVE_ENV] = '600'; | ||
| process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3400); | ||
| const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); | ||
| expect(process.exitCode).toBeUndefined(); | ||
| (writeStdoutLine as unknown as Mock).mockClear(); | ||
| call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); | ||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(readRoundStamps(plan).map((st) => st.round)).toEqual([1, 2]); | ||
| expect(readBudgetStop(plan)).toBeNull(); | ||
| }); | ||
|
|
||
| it('prices the pair at one round when the pool holds both fan-outs at once', () => { | ||
| // The default 10-slot pool holds all six auditors of PLAN's 3-chunk | ||
| // pair in one wave, so round 2 pays one round estimate — a flat 2x | ||
|
Comment on lines
+3147
to
+3149
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] This is the only one of the three new pair tests that never pins Suggested fix (verified by probe to flip the failing arm green): it('prices the pair at one round when the pool holds both fan-outs at once', () => {
// …
delete process.env[TOOL_CONCURRENCY_ENV]; // shield the ambient before the builds
process.env[RESERVE_ENV] = '600';中文说明这是三个新配对测试中唯一没有钉住 建议修复(探针已验证可使失败分支转绿,见上方英文代码块)。 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| // price would refuse this admission (reserve 600 + 3600 > 3000) and | ||
| // gut the pair's admission win near the deadline. | ||
| process.env[RESERVE_ENV] = '600'; | ||
| process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3000); | ||
| const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); | ||
| expect(process.exitCode).toBeUndefined(); | ||
| (writeStdoutLine as unknown as Mock).mockClear(); | ||
| call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); | ||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(readRoundStamps(plan).map((st) => st.round)).toEqual([1, 2]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('per-chunk retirement — cold territories stop costing a round', () => { | ||
|
|
@@ -3310,6 +3365,30 @@ describe('per-chunk retirement — cold territories stop costing a round', () => | |
| expect(keysOf(2)).toHaveLength(3); | ||
| }); | ||
|
|
||
| it('the 3B pair: round 2 builds every chunk with round 1 still in flight (no round-1 transcripts)', () => { | ||
|
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. [Suggestion] This test proves that both prompt builders can run without round-1 transcripts, but it does not pin the effect-bearing SKILL.md instruction that both 3B rounds must launch in the same response. — Concrete cost: a later edit can serialize the skill while this test stays green because it calls 中文说明这个测试证明了两个 prompt builder 可以在没有 round-1 transcript 时运行,但没有固定真正影响行为的 SKILL.md 约束:两个 3B rounds 必须在同一个 response 中启动。具体代价:后续可以把 skill 改回串行,而该测试仍然通过,因为它自己顺序调用 — Qwen Code via Qwen Code /review (v0.21.9) |
||
| // The convergence pair on 3B — the latency lever: rounds 1 and 2 are | ||
| // launched together, so round 2's builder runs BEFORE round 1's auditors | ||
| // have returned any transcript. Round 2 must still fan out to every chunk | ||
| // (the retirement schedule only reads history at k >= 3, so nothing here | ||
| // depends on round 1's records existing) and stamp its own admission, so | ||
| // the two rounds' auditors run concurrently instead of one round-wall | ||
| // apart. Pins the mechanism the SKILL 3B-pair orchestration relies on. | ||
| const r1 = runRound(1); // built, but no transcripts written for it | ||
| expect(r1).toContain('3 auditors required this round — one per chunk.'); | ||
| const r2 = runRound(2); // round 1's transcripts don't exist yet at this point | ||
| expect(r2).toContain('3 auditors required this round — one per chunk.'); | ||
| expect(r2).not.toContain('retirement:'); | ||
| expect(keysOf(1)).toHaveLength(3); | ||
| expect(keysOf(2)).toHaveLength(3); | ||
| // Both admissions are stamped, so the deadline gate prices each and the | ||
| // clock advances a round per stamp. | ||
| const rounds = readRoundStamps(plan) | ||
| .map((s) => s.round) | ||
| .sort(); | ||
| expect(rounds).toContain(1); | ||
| expect(rounds).toContain(2); | ||
| }); | ||
|
|
||
| it('round 3 skips a chunk dry in rounds 1 and 2, and the note names it', () => { | ||
| answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); | ||
| answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; | |
| import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; | ||
| import { | ||
| clearBudgetStop, | ||
| expectedRoundSeconds, | ||
| expectedAdmissionSeconds, | ||
| readRoundStamps, | ||
| reverseAuditBudgetExhausted, | ||
| reverseAuditBudgetMessage, | ||
|
|
@@ -1862,12 +1862,17 @@ function requireAuditableChunks(report: PlanReport): DiffChunk[] { | |
| * round was refused: the caller builds nothing. The admission STAMP is not | ||
| * written here — it lands after the build succeeds, in each build path: the | ||
| * stamp is what the next round's gate measures cost from, and a build that | ||
| * throws must not leave one behind. | ||
| * throws must not leave one behind. `fanOutWidth` is the auditors this | ||
| * round fans out (1 for a whole-diff round): when the previous round is | ||
| * still in flight — the convergence pair's second member — the price | ||
| * covers both members' wall in waves of the tool-concurrency pool, not | ||
| * just this round's (deadline.ts `expectedAdmissionSeconds`). | ||
| */ | ||
| function admitReverseAuditRound( | ||
| planPath: string, | ||
| round: number | undefined, | ||
| cap: number, | ||
| fanOutWidth: number, | ||
| ): boolean { | ||
| // The plan's round cap first: deterministic, and cheaper than the | ||
| // deadline arithmetic. The full cap normally; a reduced cap for a huge | ||
|
|
@@ -1900,7 +1905,7 @@ function admitReverseAuditRound( | |
| } | ||
| const spent = reverseAuditBudgetExhausted( | ||
| process.env, | ||
| expectedRoundSeconds(planPath, round), | ||
| expectedAdmissionSeconds(planPath, round, fanOutWidth, process.env), | ||
| ); | ||
| if (spent !== null) { | ||
| writeBudgetStop(planPath, spent, round); | ||
|
|
@@ -2009,6 +2014,7 @@ function runAllChunks( | |
| planPath, | ||
| round, | ||
| reverseAuditRoundCap(report.budget), | ||
| chunks.length, | ||
| ) | ||
| ) { | ||
| return; | ||
|
|
@@ -2403,7 +2409,9 @@ function runAgentPrompt(args: AgentPromptArgs): void { | |
| // admits on the reserve alone hands the terminal round a start right at | ||
| // the boundary, which is the killed-mid-verification failure one round | ||
| // wide. The round's cost is the previous round's, measured admission to | ||
| // admission. The admission is stamped AFTER the build succeeds (below), | ||
| // admission — except when this round launches with the previous one still | ||
| // in flight (the convergence pair), where it covers both. The admission is | ||
| // stamped AFTER the build succeeds (below), | ||
| // never here: the stamp is what the next round's gate measures cost from, | ||
| // and a build that throws must not leave one behind — priced from a | ||
| // failed build, the next round would be floored to the 600s minimum, | ||
|
|
@@ -2421,6 +2429,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { | |
| args.plan, | ||
| args.round, | ||
| reverseAuditRoundCap(report.budget), | ||
| 1, | ||
| ) | ||
| ) { | ||
| return; | ||
|
|
@@ -2471,14 +2480,17 @@ function runAgentPrompt(args: AgentPromptArgs): void { | |
| hasChunk && | ||
| !readRoundStamps(args.plan).some((s) => s.round === (args.round ?? null)) | ||
| ) { | ||
| const planChunkIds = ( | ||
| Array.isArray(report.chunks) ? (report.chunks as DiffChunk[]) : [] | ||
| ) | ||
| .map((c) => c?.id) | ||
| .filter((id): id is number => typeof id === 'number'); | ||
| if (args.round !== undefined) { | ||
| let schedule: RoundSchedule | null = null; | ||
| try { | ||
| schedule = scheduleReverseAuditRound( | ||
| args.plan, | ||
| (Array.isArray(report.chunks) ? (report.chunks as DiffChunk[]) : []) | ||
| .map((c) => c?.id) | ||
| .filter((id): id is number => typeof id === 'number'), | ||
| planChunkIds, | ||
| args.round, | ||
| process.env, | ||
| typeof report.diffPathAbsolute === 'string' | ||
|
|
@@ -2500,6 +2512,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { | |
| args.plan, | ||
| args.round, | ||
| reverseAuditRoundCap(report.budget), | ||
| planChunkIds.length, | ||
| ) | ||
|
Comment on lines
2512
to
2516
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 Suggested fix: add one handler-level test admitting round 1 via 中文说明[Suggestion] 建议修复:新增一个 handler 级测试——通过 — qwen3.8-max via Qwen Code /review (v0.21.9)
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 this round (not dropped): the wiring is correct as written — the finding itself classifies this as a missing guard, not a live defect — and this round landed the cheaper fixes under a budget warning. Test recipe preserved for the follow-up: one handler-level test admitting round 1 via 中文说明本轮推迟(并非丢弃):接线按现状是正确的——发现本身将其归类为缺失的防护,而非现存缺陷——且本轮在预算警告下落地了成本较低的修复。为后续保留的测试配方:一个 handler 级测试,先通过 |
||
| ) | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,9 +23,12 @@ import { | |
| DEFAULT_RESERVE_SECONDS, | ||
| DEFAULT_ROUND_SECONDS, | ||
| DEFAULT_COMPOSE_FLOOR_SECONDS, | ||
| DEFAULT_TOOL_CONCURRENCY, | ||
| TOOL_CONCURRENCY_ENV, | ||
| budgetStopEntry, | ||
| budgetStopEntryZh, | ||
| clearBudgetStop, | ||
| expectedAdmissionSeconds, | ||
| expectedRoundSeconds, | ||
| readBudgetStop, | ||
| readRoundStamps, | ||
|
|
@@ -282,6 +285,117 @@ describe('the round-cost estimate — measured when it can be', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('the pair admission price — a round launched beside an in-flight round pays for both', () => { | ||
| // The convergence pair's second member is built seconds after the first's | ||
| // stamp, so nothing has measured a round yet. Pricing it off that | ||
| // seconds-old span committed the pair at one round's price for up to two | ||
| // rounds' wall — these pin the wave-priced pair instead. | ||
| const dirs: string[] = []; | ||
| afterEach(() => { | ||
| for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); | ||
| }); | ||
| function plan(): string { | ||
| const dir = mkdtempSync(join(tmpdir(), 'deadline-pair-')); | ||
| dirs.push(dir); | ||
| const p = join(dir, 'plan.json'); | ||
| writeFileSync(p, '{}'); | ||
| backdatePlan(p); | ||
| return p; | ||
| } | ||
|
|
||
| it('prices a round with no in-flight predecessor like expectedRoundSeconds', () => { | ||
| const p = plan(); | ||
| expect(expectedAdmissionSeconds(p, 1, 6, {}, NOW_MS)).toBe( | ||
| DEFAULT_ROUND_SECONDS, | ||
| ); | ||
| stampRound(p, 1, NOW_MS - 2_400_000); // round 1 returned 40 min ago | ||
| expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe( | ||
| expectedRoundSeconds(p, 2, NOW_MS), | ||
| ); | ||
| expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe(2400); | ||
| }); | ||
|
|
||
| it('prices the pair at both members when the pool serializes them', () => { | ||
| // Six chunks on the default 10-slot pool: one wave per round, two | ||
| // waves for the pair — the seconds-old round-1 stamp has measured | ||
| // nothing, so the price is 2x the round estimate, not the floor. | ||
| const p = plan(); | ||
| stampRound(p, 1, NOW_MS - 30_000); | ||
| expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe( | ||
| 2 * DEFAULT_ROUND_SECONDS, | ||
| ); | ||
| }); | ||
|
|
||
| it('prices the pair at one round when the pool holds both members at once', () => { | ||
| // Three chunks on ten slots: both members fit in a single wave, and | ||
| // the pair's wall is one round's — the 3A shape reads the same (width | ||
| // 1 on any pool of two or more). | ||
| const p = plan(); | ||
| stampRound(p, 1, NOW_MS - 30_000); | ||
| expect(expectedAdmissionSeconds(p, 2, 3, {}, NOW_MS)).toBe( | ||
| DEFAULT_ROUND_SECONDS, | ||
| ); | ||
| expect(expectedAdmissionSeconds(p, 2, 1, {}, NOW_MS)).toBe( | ||
| DEFAULT_ROUND_SECONDS, | ||
| ); | ||
| }); | ||
|
|
||
| it('reads the pool from the tool-concurrency env, like the scheduler', () => { | ||
| const p = plan(); | ||
| stampRound(p, 1, NOW_MS - 30_000); | ||
| // A 12-slot pool holds all twelve auditors of a 6-chunk pair in one | ||
| // wave. | ||
| expect( | ||
| expectedAdmissionSeconds( | ||
| p, | ||
| 2, | ||
| 6, | ||
| { [TOOL_CONCURRENCY_ENV]: '12' }, | ||
| NOW_MS, | ||
| ), | ||
| ).toBe(DEFAULT_ROUND_SECONDS); | ||
| // A 3-slot pool runs a 6-chunk round in two waves and the pair in | ||
| // four — two rounds' price again. | ||
| expect( | ||
| expectedAdmissionSeconds( | ||
| p, | ||
| 2, | ||
| 6, | ||
| { [TOOL_CONCURRENCY_ENV]: '3' }, | ||
| NOW_MS, | ||
| ), | ||
| ).toBe(2 * DEFAULT_ROUND_SECONDS); | ||
| // Malformed falls back to the default pool, never to a wedge. | ||
| expect( | ||
| expectedAdmissionSeconds( | ||
| p, | ||
| 2, | ||
| 6, | ||
| { [TOOL_CONCURRENCY_ENV]: 'soon' }, | ||
| NOW_MS, | ||
| ), | ||
| ).toBe( | ||
| Math.ceil( | ||
| (DEFAULT_ROUND_SECONDS * Math.ceil(12 / DEFAULT_TOOL_CONCURRENCY)) / | ||
| Math.ceil(6 / DEFAULT_TOOL_CONCURRENCY), | ||
|
Comment on lines
+378
to
+380
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 default-pool oracle in this new suite recomputes its expected value from Suggested fix: pin one oracle to a literal at a discriminating width (width 16 diverges between pools 10 and 11), or assert expect(
expectedAdmissionSeconds(p, 2, 16, {}, NOW_MS),
).toBe(3600); // literal, not DEFAULT_TOOL_CONCURRENCY-derived: pool 10 ⇒ 2 waves中文说明这个新套件中所有默认池的期望值都用 建议修复:用一个可区分的宽度把某个期望值钉成字面量(宽度 16 在池 10 与池 11 之间结果不同),或断言 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| it('keeps the reserve on top of the pair price at the refusal boundary', () => { | ||
| const p = plan(); | ||
| stampRound(p, 1, NOW_MS - 30_000); | ||
| const price = expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS); | ||
| expect(price).toBe(2 * DEFAULT_ROUND_SECONDS); | ||
| const env = { | ||
| [DEADLINE_ENV]: String(NOW_S + DEFAULT_RESERVE_SECONDS + price), | ||
| }; | ||
| expect(reverseAuditBudgetExhausted(env, price, NOW_MS)).toBeNull(); | ||
| env[DEADLINE_ENV] = String(NOW_S + DEFAULT_RESERVE_SECONDS + price - 1); | ||
| expect(reverseAuditBudgetExhausted(env, price, NOW_MS)).not.toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('the budget-stop marker — the deterministic half of the disclosure', () => { | ||
| const dirs: string[] = []; | ||
| afterEach(() => { | ||
|
|
||
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] This changed test file sits outside every npm workspace, so the workspace-scoped unit-test pass (
npm testper package, as run by the unit CI jobs) never collects it — only the dedicated integration job does. — Concrete cost: if theIntegration Tests (CLI, No Sandbox)job is skipped on this PR, this changed file ships without ever having executed in the pipeline. It was skipped in this PR's CI. Mitigated here: the hunk is formatting-only (destructuring re-wrap) and an explicit run against the PR bundle passed 11/11 — the residual risk is CI-job coverage of the run, not the code.Suggested fix: N/A for the code; confirm the integration-tests CI job ran (not skipped) for this PR before merge.
中文说明
[Suggestion] 这个被修改的测试文件位于所有 npm workspace 之外,因此按 workspace 划分的单测通行流程(各 package 的
npm test,即单测 CI 作业所运行的)永远不会收集它——只有专门的集成作业会。— 具体代价:如果Integration Tests (CLI, No Sandbox)作业在本 PR 上被跳过,这个被修改的文件就会从未在流水线中执行过就合入。本 PR 的 CI 中该作业确实被跳过了。此处有缓解:该 hunk 仅是格式化(解构重排),且针对 PR bundle 的显式运行 11/11 通过——残余风险在于该运行的 CI 作业覆盖,而非代码本身。建议修复:代码层面无需改动;合入前确认集成测试 CI 作业确实运行了(而非被跳过)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
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.
Declined — no code change exists: the suggested fix is "N/A for the code" by its own text. The hunk in this file is formatting-only (a destructuring re-wrap), and the review's own explicit run against the PR bundle passed 11/11. The residual risk named — the
Integration Tests (CLI, No Sandbox)job being skipped on this PR — is a merge-time check owned by the workflow/maintainer, not something a branch change can fix. Leaving this thread open so the check stays visible at merge time.中文说明
拒绝——不存在可做的代码改动:建议修复按原文即为 "N/A for the code"。该文件中被改动的 hunk 仅是格式化(解构重排),且评审自己针对 PR bundle 的显式运行 11/11 通过。所指的残余风险——
Integration Tests (CLI, No Sandbox)作业在本 PR 上被跳过——是 workflow/维护者在合入时的检查项,不是分支改动能解决的。线程保持打开,使该检查在合入时保持可见。