diff --git a/CHANGELOG.md b/CHANGELOG.md index 656a27f42..9eaeb3845 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ ## [Unreleased] +**Most PRs no longer pay for a solo paid screen before the merge train runs the same cases.** `gstack-ship-queue ready` used to recommend `--screen` whenever a diff touched a skill's own cases, which is almost every PR, and the screens (16 to 19 a day at about $32 each) cost more than the train gates that rerun all 137 cases a few hours later. Of 41 full screens between Oct 20 and 29, the 3 that caught a real bug all came from diffs the selector can't narrow (an unknown dependency or a shared runtime input). The recommendation is now `--screen` only in that case; every other PR rides unscreened and the train gate's full census covers it. Because most PRs now ride unscreened, a train also stopped holding them back: unscreened PRs fill the seats a screened PR leaves. + +### Changed +- **Screen recommendation** (`lib/ship-queue-train-ready.ts`): `--screen` when the paid selector's mode is `full-fallback` or the selector cannot select; otherwise `--no-screen`. The `SCREEN:` lines name the selector's mode and reason. +- **Slot order** (`lib/ship-queue-train.ts` `orderSlots`): screened PRs first, then unscreened PRs fill the remaining seats in READY order. The rule that built a screened-only train and held every unscreened PR once `max_size` of them waited is gone, along with its `HELD: … (unscreened PRs form their own train)` line. `--order` still overrides. +- **Train replay model** (`lib/gate-cost-history.ts` `pickTrain`) seats trains the same way. +- **Not in this change:** a focused paid run of a no-screen PR's own cases, bound to its READY receipt, needs a new `screen_policy: own` route in evals.yml (GSTA-165 1b, deferred). + +### Tests +- `test/ship-queue-train-logic.test.ts`: one screened plus five unscreened at size 5 seats five; a full unscreened pool at size 3 seats three; a skill-template diff recommends `--no-screen`; a `bun.lock` diff recommends `--screen` with its full-fallback reason. `test/ship-queue-train.test.ts` and `test/gate-cost.test.ts` change their ordering expectations on purpose. These tests failed before the change. + **A PR screen no longer runs the whole paid gate just because a changed file sits on every case's import path.** Editing a file like `lib/restamp.ts` reached all 113 gate cases through the derived-reference closure, so 19 of the 41 full screens between Oct 20 and 29 ran the full $33 gate for that reason alone. A narrow screen (`gstack-ship-queue ready --screen`, and `bun run eval:bg:pr`) now leaves such a file's cases to the train gate's full census, which runs every case before anything merges. Replayed on those 41 screens with today's maps, this saves about $457 a week, and the screens that caught real PR-caused reds (#3434, #3453) still run the full gate. ### Changed diff --git a/docs/ship-policy.md b/docs/ship-policy.md index b2987dffa..1475e1d7b 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -406,9 +406,12 @@ gstack-ship-queue ready --pr 42 --screen [--broad] # dispatches one paid screen ``` Without a choice it prints the `SCREEN:` selection (with -`screen_selection: gstack-paid`, the skill's own cases and `recommend ---screen` or `--no-screen`: `--screen` when the screen selects a skill's own -cases) and refuses with `READY_SCREEN_CHOICE_REQUIRED`. The price line says +`screen_selection: gstack-paid`, the skill's own cases, the selector's mode and +`recommend --screen` or `--no-screen`: `--screen` only when the selector's +mode is `full-fallback`, an unknown dependency or a shared runtime input that +restores every case, or when it cannot select at all; every other PR rides +unscreened, and the train gate's full census runs its cases on the tree that +merges) and refuses with `READY_SCREEN_CHOICE_REQUIRED`. The price line says the dollar figure is pending the cost replay; until it lands, `SCREEN:` shows case counts and the recommendation only. A lane that already ran focused local cases says so in the READY block and may pass `--no-screen`. `--screen` @@ -458,8 +461,8 @@ gstack-ship-queue train build --prs 41@,42@,43@ ``` It prints `ORDER:` (the slot order to announce: screened PRs first in READY -order, then unscreened; when `max_size` or more unscreened PRs wait they form -their own train and `HELD:` lists them; `--order ,,...` naming every +order, then unscreened PRs fill the remaining seats, and `HELD:` lists the PRs +over `max_size`; `--order ,,...` naming every pinned PR once replaces that rule, for a PR that must merge before another, and the train's later `NEXT:` rebuilds keep it), one `PREFIX: T tree=` per prefix and one `SLOT:` per PR. `--dry-run` adds the diff --git a/lib/gate-cost-history.ts b/lib/gate-cost-history.ts index f6f4b668e..b379de9f3 100644 --- a/lib/gate-cost-history.ts +++ b/lib/gate-cost-history.ts @@ -317,22 +317,16 @@ export interface ArrivalTrainResult { interface Waiting { pr: number; readyMs: number; enqueuedMs: number; screened: boolean; defect: boolean } /** - * The next train, or null to keep waiting: five or more waiting screened (or - * unscreened) PRs dispatch a full train of their own; otherwise a full mixed - * train, screened first; otherwise everything waiting, screened first, once - * the oldest has waited the fill limit. The train with the oldest PR goes first. + * The next train, or null to keep waiting, as orderSlots and fillDue seat it: + * screened PRs first, then unscreened PRs fill the remaining seats, each by + * age; a full train dispatches at once, a smaller one once its oldest PR has + * waited the fill limit. */ export function pickTrain(pool: T[], nowMs: number, size: number, fillMs: number): T[] | null { const byAge = (a: T, b: T) => a.enqueuedMs - b.enqueuedMs; - const screened = pool.filter(x => x.screened).sort(byAge), unscreened = pool.filter(x => !x.screened).sort(byAge); - const options = [screened, unscreened].filter(list => list.length >= size).map(list => list.slice(0, size)); - if (!options.length) { - const mixed = [...screened, ...unscreened]; - if (mixed.length >= size) options.push(mixed.slice(0, size)); - else if (mixed.length && nowMs - Math.min(...mixed.map(x => x.enqueuedMs)) >= fillMs) options.push(mixed); - } - const oldest = (train: T[]) => Math.min(...train.map(x => x.enqueuedMs)); - return options.sort((a, b) => oldest(a) - oldest(b))[0] ?? null; + const mixed = [...pool.filter(x => x.screened).sort(byAge), ...pool.filter(x => !x.screened).sort(byAge)]; + if (mixed.length >= size) return mixed.slice(0, size); + return mixed.length && nowMs - Math.min(...mixed.map(x => x.enqueuedMs)) >= fillMs ? mixed : null; } /** diff --git a/lib/ship-queue-train-ready.ts b/lib/ship-queue-train-ready.ts index 5a04ec381..32a318742 100644 --- a/lib/ship-queue-train-ready.ts +++ b/lib/ship-queue-train-ready.ts @@ -2,8 +2,8 @@ * `ready --pr --screen | --no-screen` under a ship policy with a `train` * block (docs/ship-policy.md#run-a-merge-train; plan GSTA-49 T1). The * READY block is today's (lib/ship-queue.ts ready()); this adds the screen - * selection and the default recommendation (screen when the screen - * selects a skill's own cases), requires an explicit choice + * selection and the default recommendation (screen only when the selector + * restores every case or cannot select), requires an explicit choice * (READY_SCREEN_CHOICE_REQUIRED), dispatches a chosen screen through * eval-bg's nonce with its purpose and policy in the key, and records the * choice as a READY receipt (local state and refs/notes/gstack-trains) that @@ -49,28 +49,41 @@ export function skillOwnCases(selected: readonly string[], touchfiles: Record (touchfiles[id] ?? []).some(glob => skills.some(s => glob.startsWith(`${s}/`)))).sort(); } -/** gstack's paid selector over a diff, as a narrow screen (`--screen` without `--broad`) plans it, loaded only when the policy asks for it. */ +/** + * gstack's paid selector over a diff, as a narrow screen (`--screen` without `--broad`) plans it, loaded only when the + * policy asks for it. The recommendation is --screen only when the selector restores every case (its `full-fallback` + * mode: an unknown dependency or a shared runtime input) or cannot select at all; every other PR rides the train + * unscreened, where the full gate census runs every case on the tree that merges (GSTA-165). + */ export async function gstackPaidSelection(repoRoot: string, baseSha: string, changed: string[], hasTemplate: (dir: string) => boolean): Promise { const { computePaidCaseSelection } = await import('../scripts/lib/paid-select'); const { E2E_TOUCHFILES, LLM_JUDGE_TOUCHFILES } = await import('../test/helpers/touchfiles'); const skills = touchedSkills(changed, hasTemplate); let e2e: string[]; let judges: number; let note = "the narrow screen's selection; --broad adds the dependents of files whose references reach every gate case"; + let why: string; + let everyCase = false; try { - const s = computePaidCaseSelection({ profile: 'pr', rootDir: repoRoot, changedFiles: changed, env: { ...process.env, EVALS_BASE: baseSha, EVALS_ALL: '', EVALS_SCREEN_POLICY: 'narrow' } }).selection; - e2e = s.e2e ?? Object.keys(E2E_TOUCHFILES); - judges = s.judges ? s.judges.length : Object.keys(LLM_JUDGE_TOUCHFILES).length; + const r = computePaidCaseSelection({ profile: 'pr', rootDir: repoRoot, changedFiles: changed, env: { ...process.env, EVALS_BASE: baseSha, EVALS_ALL: '', EVALS_SCREEN_POLICY: 'narrow' } }); + e2e = r.selection.e2e ?? Object.keys(E2E_TOUCHFILES); + judges = r.selection.judges ? r.selection.judges.length : Object.keys(LLM_JUDGE_TOUCHFILES).length; + const mode = r.coverage?.mode ?? 'unknown'; + everyCase = mode === 'full-fallback'; + why = everyCase ? `selector mode full-fallback (${r.reason.slice(0, 200)})` : `selector mode ${mode}: the train gate's full census covers these cases`; } catch (error) { e2e = Object.keys(E2E_TOUCHFILES); judges = Object.keys(LLM_JUDGE_TOUCHFILES).length; note = `the PR profile needs full validation (${error instanceof Error ? error.message.slice(0, 160) : String(error)}): the screen is the full gate selection`; + everyCase = true; + why = 'the selector could not select'; } const own = skillOwnCases(e2e, E2E_TOUCHFILES, skills); - const recommend = own.length ? 'screen' : 'no-screen'; + const recommend = everyCase ? 'screen' : 'no-screen'; return { recommend, lines: [ `SCREEN: selection ${e2e.length} e2e case(s) + ${judges} judge(s) (${note})`, - `SCREEN: skill's own cases ${own.length ? own.slice(0, 12).join(',') + (own.length > 12 ? ` (+${own.length - 12})` : '') : 'none'} → recommend --${recommend}`, + `SCREEN: skill's own cases ${own.length ? own.slice(0, 12).join(',') + (own.length > 12 ? ` (+${own.length - 12})` : '') : 'none'}`, + `SCREEN: ${why} → recommend --${recommend}`, ], }; } diff --git a/lib/ship-queue-train.ts b/lib/ship-queue-train.ts index e44c46a5b..ed86d522e 100644 --- a/lib/ship-queue-train.ts +++ b/lib/ship-queue-train.ts @@ -100,22 +100,22 @@ export const pinList = (slots: Array<{ pr: string; head: string }>) => slots.map export const prsArgs = (slots: Array<{ pr: string; head: string }>, explicit?: boolean) => `--prs ${pinList(slots)}${explicit ? ` --order ${slots.map(s => s.pr).join(',')}` : ''}`; /** - * Slot order: screened PRs first, then unscreened, each in READY order; - * enough unscreened PRs form their own train. An explicit order (`--order`, + * Slot order: screened PRs first, then unscreened PRs fill the remaining + * seats, each in READY order (GSTA-165: most PRs ride unscreened, so a + * screened-only train would hold the pool). An explicit order (`--order`, * every admitted PR once) replaces that rule: slots follow it, the first * `max` ride and the rest are held. */ -export function orderSlots(slots: TrainSlot[], max: number, explicit?: readonly string[]): { train: TrainSlot[]; held: TrainSlot[]; split: boolean } { +export function orderSlots(slots: TrainSlot[], max: number, explicit?: readonly string[]): { train: TrainSlot[]; held: TrainSlot[] } { if (explicit) { const all = explicit.map(pr => slots.find(s => s.pr === pr)!); - return { train: all.slice(0, max), held: all.slice(max), split: false }; + return { train: all.slice(0, max), held: all.slice(max) }; } const byReady = (a: TrainSlot, b: TrainSlot) => a.ready_at.localeCompare(b.ready_at) || Number(a.pr) - Number(b.pr); const screened = slots.filter(s => s.screen.choice === 'screen').sort(byReady); const unscreened = slots.filter(s => s.screen.choice !== 'screen').sort(byReady); - if (screened.length && unscreened.length >= max) return { train: screened.slice(0, max), held: [...screened.slice(max), ...unscreened], split: true }; const all = [...screened, ...unscreened]; - return { train: all.slice(0, max), held: all.slice(max), split: false }; + return { train: all.slice(0, max), held: all.slice(max) }; } /** When a train under its size may dispatch: its oldest READY plus the fill time; null when it may dispatch now. */ @@ -389,7 +389,7 @@ async function newTrain(env: TrainEnv, o: BuildOptions): Promise<{ t: TrainRecor const lines = [ ...admitted.lines, `ORDER: ${order.train.map((s, i) => `${i + 1}:#${s.pr}(${s.screen.choice === 'screen' ? 'screened' : 'unscreened'})`).join(' ')}${explicit ? ' (explicit --order)' : ''}`, - ...(order.held.length ? [`HELD: ${order.held.map(s => '#' + s.pr).join(',')} (${order.split ? 'unscreened PRs form their own train' : `over the train size ${env.train.max_size}`})`] : []), + ...(order.held.length ? [`HELD: ${order.held.map(s => '#' + s.pr).join(',')} (over the train size ${env.train.max_size})`] : []), ]; const baseSha = git(env.ctx.repoRoot, ['rev-parse', `${env.remote}/${env.ctx.base}`]).out; const key = sha256(JSON.stringify({ base: baseSha, slots: order.train.map(s => `${s.pr}@${s.head}`) })); diff --git a/test/gate-cost.test.ts b/test/gate-cost.test.ts index 155a94bfd..b73a1cd53 100644 --- a/test/gate-cost.test.ts +++ b/test/gate-cost.test.ts @@ -344,10 +344,10 @@ describe('READY arrivals and trains', () => { expect([percentile([5, 1, 3, 2, 4], 0.5), percentile([5, 1, 3, 2, 4], 0.9), percentile([], 0.5)]).toEqual([3, 5, 0]); }); - test('train composition: unscreened PRs form their own train at five, else screened first, else the fill limit', () => { + test('train composition: screened first, unscreened fill the remaining seats, else the fill limit', () => { const item = (enqueuedMs: number, screened: boolean) => ({ enqueuedMs, screened }); const pool = [item(0, true), item(1, true), ...[2, 3, 4, 5, 6].map(t => item(t, false))]; - expect(pickTrain(pool, 10, 5, 100)).toEqual(pool.slice(2)); + expect(pickTrain(pool, 10, 5, 100)).toEqual(pool.slice(0, 5)); const mixed = [item(5, false), item(0, true), item(1, false), item(2, true), item(3, true)]; expect(pickTrain(mixed, 10, 5, 100)?.map(x => x.screened)).toEqual([true, true, true, false, false]); expect(pickTrain([item(0, false), item(1, true)], 99, 5, 100)).toBeNull(); diff --git a/test/ship-queue-train-logic.test.ts b/test/ship-queue-train-logic.test.ts index 15bae88b2..d5cedc971 100644 --- a/test/ship-queue-train-logic.test.ts +++ b/test/ship-queue-train-logic.test.ts @@ -28,17 +28,17 @@ describe('ordering and fill', () => { const o = orderSlots([slot('4', false, '2026-10-18T01:00:00Z'), slot('3', true, '2026-10-18T03:00:00Z'), slot('2', false, '2026-10-18T00:00:00Z'), slot('1', true, '2026-10-18T02:00:00Z')], 5); expect(o.train.map(s => s.pr)).toEqual(['1', '3', '2', '4']); expect(o.held).toEqual([]); - expect(o.split).toBe(false); }); - test('when as many unscreened PRs wait as fill a train, they form their own train; over-size PRs are held in order', () => { - const unscreened = ['5', '6', '7'].map((pr, i) => slot(pr, false, `2026-10-18T0${i}:00:00Z`)); - const split = orderSlots([slot('9', true, '2026-10-18T05:00:00Z'), ...unscreened], 3); - expect(split.split).toBe(true); - expect(split.train.map(s => s.pr)).toEqual(['9']); - expect(split.held.map(s => s.pr)).toEqual(['5', '6', '7']); + test('unscreened PRs fill the seats a screened PR leaves, however many wait; over-size PRs are held in order', () => { + const unscreened = ['5', '6', '7', '10', '11'].map((pr, i) => slot(pr, false, `2026-10-18T0${i}:00:00Z`)); + const mixed = orderSlots([slot('9', true, '2026-10-18T05:00:00Z'), ...unscreened], 5); + expect(mixed.train.map(s => s.pr)).toEqual(['9', '5', '6', '7', '10']); + expect(mixed.held.map(s => s.pr)).toEqual(['11']); + const atThree = orderSlots([slot('9', true, '2026-10-18T05:00:00Z'), ...unscreened.slice(0, 3)], 3); + expect([atThree.train.map(s => s.pr), atThree.held.map(s => s.pr)]).toEqual([['9', '5', '6'], ['7']]); + expect(orderSlots(unscreened, 3).train.map(s => s.pr)).toEqual(['5', '6', '7']); const over = orderSlots([slot('8', true, '2026-10-18T06:00:00Z'), slot('9', true, '2026-10-18T05:00:00Z'), unscreened[0]!], 2); - expect(over.split).toBe(false); expect(over.train.map(s => s.pr)).toEqual(['9', '8']); expect(over.held.map(s => s.pr)).toEqual(['5']); }); @@ -68,7 +68,7 @@ describe('ordering and fill', () => { expect(orderSlots([docs, catalog], 5, order as string[]).train.map(s => s.pr)).toEqual(['3343', '3337']); const screened = slot('9', true, '2026-10-22T20:00:00Z'); const over = orderSlots([docs, catalog, screened], 2, ['3343', '3337', '9']); - expect([over.train.map(s => s.pr), over.held.map(s => s.pr), over.split]).toEqual([['3343', '3337'], ['9'], false]); + expect([over.train.map(s => s.pr), over.held.map(s => s.pr)]).toEqual([['3343', '3337'], ['9']]); expect(parseOrder(undefined, pins)).toBeUndefined(); for (const bad of ['3343', '3343,3337,3337', '3343,3337,1', '3343,x']) expect(parseOrder(bad, pins)).toEqual({ error: `--order must list every PR in --prs exactly once (--prs has #3337,#3343; got ${bad})` }); expect(prsArgs([{ pr: '3343', head: SHA }, { pr: '3337', head: SHA }], true)).toBe(`--prs 3343@${SHA},3337@${SHA} --order 3343,3337`); @@ -194,12 +194,16 @@ describe('screen recommendation', () => { expect(skillOwnCases(['a', 'b', 'c'], { a: ['review/**'], b: ['lib/**'], c: ['review/SKILL.md'] }, ['review'])).toEqual(['a', 'c']); }); - test("gstack's selector on gstack's own tree: a skill template change recommends --screen, a docs change --no-screen", async () => { + test("gstack's selector on gstack's own tree: only a change that restores every case recommends --screen", async () => { const has = (dir: string) => fs.existsSync(path.join(ROOT, dir, 'SKILL.md.tmpl')); const skill = await gstackPaidSelection(ROOT, 'HEAD', ['review/SKILL.md.tmpl'], has); - expect(skill.recommend).toBe('screen'); + expect(skill.recommend).toBe('no-screen'); expect(skill.lines[0]).toMatch(/^SCREEN: selection \d+ e2e case\(s\) \+ \d+ judge\(s\)/); - expect(skill.lines[1]).toMatch(/^SCREEN: skill's own cases \S+ → recommend --screen$/); + expect(skill.lines[1]).toMatch(/^SCREEN: skill's own cases \S+$/); + expect(skill.lines[2]).toMatch(/^SCREEN: selector mode (pr|dependents): the train gate's full census covers these cases → recommend --no-screen$/); + const shared = await gstackPaidSelection(ROOT, 'HEAD', ['bun.lock'], has); + expect(shared.recommend).toBe('screen'); + expect(shared.lines[2]).toMatch(/^SCREEN: selector mode full-fallback \(.*bun\.lock.*\) → recommend --screen$/); const docs = await gstackPaidSelection(ROOT, 'HEAD', ['docs/ship-policy.md'], has); expect(docs.recommend).toBe('no-screen'); }, 60_000); diff --git a/test/ship-queue-train.test.ts b/test/ship-queue-train.test.ts index 15e24c277..ff77594b0 100644 --- a/test/ship-queue-train.test.ts +++ b/test/ship-queue-train.test.ts @@ -564,7 +564,7 @@ describe('train build', () => { expect(dispatches(fx, 'free-tests.yml').length).toBe(4); }, 180_000); - test('ordering: screened PRs first in READY order; enough unscreened PRs wait for their own train', () => { + test('ordering: screened PRs first in READY order; unscreened PRs fill the remaining seats', () => { const fx = make({ ...TRAIN_POLICY, train: { ...TRAIN_BLOCK, max_size: 2 } }); const h = ['1', '2', '3'].map(pr => openPr(fx, pr)); readyReceipt(fx, '1', h[0]!, { at: '2026-10-18T08:00:00.000Z' }); @@ -572,8 +572,8 @@ describe('train build', () => { readyReceipt(fx, '3', h[2]!, { at: '2026-10-18T09:00:00.000Z', screen: true }); const r = queue(fx, ['train', 'build', '--prs', `1@${h[0]},2@${h[1]},3@${h[2]}`, '--dry-run']); expect(r.status).toBe(EXIT.ok); - expect(line(r.out, 'ORDER:')).toBe('ORDER: 1:#3(screened)'); - expect(line(r.out, 'HELD:')).toBe('HELD: #2,#1 (unscreened PRs form their own train)'); + expect(line(r.out, 'ORDER:')).toBe('ORDER: 1:#3(screened) 2:#2(unscreened)'); + expect(line(r.out, 'HELD:')).toBe('HELD: #1 (over the train size 2)'); const wide = make(); const w = ['1', '2', '3'].map(pr => openPr(wide, pr)); readyReceipt(wide, '1', w[0]!, { at: '2026-10-18T08:00:00.000Z' });