From 174bc2fd7b57fa0e463d1d99ff0d9f335f790a8a Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 04:23:22 +0000 Subject: [PATCH] fix: pre-landing review fixes (pull_request runs stay broad, deleted hubs, READY and summary previews) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .github/workflows/evals.yml | 6 ++++-- docs/ship-policy.md | 5 +++-- lib/ship-queue-train-ready.ts | 6 +++--- scripts/test-pr-profile.ts | 5 +++-- test/pr-dependencies.test.ts | 23 ++++++++++++++++++++--- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index ed6a68049..451f3a150 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -475,6 +475,7 @@ jobs: - name: Print the screening selection and its estimated cost (no paid case runs) env: PR_NUMBER: ${{ github.event.pull_request.number }} + EVALS_SCREEN_POLICY: narrow run: | EVALS_TIER=gate bun --no-install run scripts/test-paid-shards.ts --tier gate --emit-plan /tmp/paid-plan/manifest.json --slices 1 bun --no-env-file --config=/dev/null --no-install run scripts/train-gate.ts screen-summary /tmp/paid-plan/manifest.json --pr "$PR_NUMBER" >> "$GITHUB_STEP_SUMMARY" @@ -569,8 +570,9 @@ jobs: EVALS_CACHE_DIR: ${{ steps.reuse.outputs.reuse == 'on' && '/tmp/gstack-eval-input-cache' || '' }} # Dispatch diff base (CEO-17): a screen's merge-commit first parent, else the exact merge-base, else the named base branch, else detection. EVALS_BASE: ${{ needs.route.outputs.screen_base || inputs.base_sha || (inputs.base_ref != '' && format('origin/{0}', inputs.base_ref)) || '' }} - # A broad screen (`ready --screen --broad`) keeps every derived dependent; screens and eval:bg:pr default to narrow. - EVALS_SCREEN_POLICY: ${{ inputs.screen_policy || 'narrow' }} + # Dispatched screens and eval:bg:pr default to narrow; a broad screen (`ready --screen --broad`) and a + # pull_request run (the merge gate when no train block exists) keep every derived dependent. + EVALS_SCREEN_POLICY: ${{ inputs.screen_policy || (github.event_name == 'workflow_dispatch' && 'narrow') || 'broad' }} run: EVALS_TIER=gate bun --no-install run scripts/test-paid-shards.ts --tier gate --emit-plan /tmp/paid-plan/manifest.json --slice-budget 420 --jobs 2 --max-parallel 20 # R2a/R2: reuse holds only when this head's own plan selects exactly the diff --git a/docs/ship-policy.md b/docs/ship-policy.md index c8216e26a..b2987dffa 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -416,8 +416,9 @@ dispatches `gate_workflow` with `screen_pr`, `screen_merge_sha` (the `refs/pull//merge` SHA) and `screen_policy` (`narrow`, or `broad` with `--broad`, the full PR selection). A narrow screen, and `eval:bg:pr`, skip a changed file whose derived references reach every gate case and nothing -narrower selects: the plan's reason names it, and the train gate's full -census runs those cases. Full-gate files, shared runtime and paid-runner +narrower selects, along with its derived judges: the plan's reason names it, +and the train gate's full census runs those cases. A `pull_request` run, the +merge gate when the base has no `train` block, keeps the broad selection. Full-gate files, shared runtime and paid-runner inputs, and files the derivation cannot place still run the full gate on a narrow screen. The nonce key carries the purpose and the policy, so a repeat follows the same run and a broad screen never adopts diff --git a/lib/ship-queue-train-ready.ts b/lib/ship-queue-train-ready.ts index bbb4cbda0..5a04ec381 100644 --- a/lib/ship-queue-train-ready.ts +++ b/lib/ship-queue-train-ready.ts @@ -49,14 +49,14 @@ 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 (today's PR selection; T2 narrows 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. */ 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 = "today's PR selection; T2's narrowed screen has not landed"; + 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"; try { - const s = computePaidCaseSelection({ profile: 'pr', rootDir: repoRoot, changedFiles: changed, env: { ...process.env, EVALS_BASE: baseSha, EVALS_ALL: '' } }).selection; + 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; } catch (error) { diff --git a/scripts/test-pr-profile.ts b/scripts/test-pr-profile.ts index 13051a7a0..2cf7d6d8d 100644 --- a/scripts/test-pr-profile.ts +++ b/scripts/test-pr-profile.ts @@ -446,7 +446,7 @@ export function selectPrProfile(options: { } const gateIds = Object.keys(maps.e2eTouchfiles).filter(id => maps.tiers[id] === 'gate'); const hubFiles = options.screenPolicy === 'narrow' && gateIds.length > 0 - ? [...consumers.keys()].filter(file => gateIds.every(id => consumers.get(file)!.e2e.has(id))) : []; + ? [...consumers.keys()].filter(file => derived?.tracked.has(file) && gateIds.every(id => consumers.get(file)!.e2e.has(id))) : []; const derivedFiles = [...consumers.keys()].filter(file => !hubFiles.includes(file)); const unknownFiles = unmapped.filter(file => !derivedGlobal.includes(file) && !consumers.has(file) && !noConsumerFiles.includes(file)); const sharedInputs = files.filter(file => depends(file, PR_FULL_GATE_FILES)); @@ -563,7 +563,7 @@ function shadowSelection(options: Parameters[0] & { maps export interface CoverageSummaryInput { profile?: string; selection?: { e2e: string[] | null; judges: string[] | null }; - prCoverage?: Pick & Partial>; + prCoverage?: Pick & Partial>; } /** @@ -584,6 +584,7 @@ export function formatPrCoverageSummary(manifest: CoverageSummaryInput, totals?: lines.push(`- Derived dependents (scripts/pr-dependencies.ts) of ${coverage.derivedFiles.length} unmapped file(s):`); for (const item of coverage.derivedFiles.slice(0, 40)) lines.push(` - \`${item.file}\`: ${item.e2e} case(s), ${item.judges} judge(s)`); } + if (coverage?.hubFiles?.length) lines.push(`- Left to the train gate's full census (narrow screen): ${coverage.hubFiles.slice(0, 40).map(file => `\`${file}\``).join(', ')}`); const shadow = coverage?.shadow; if (shadow) lines.push(shadow.error ? `- Shadow exercised-path map: unavailable (${shadow.error}); selection unaffected` : `- Shadow exercised-path map (not active): would select ${shadow.e2e.length} gate case(s), dropping ${shadow.wouldDrop.length} and adding ${shadow.wouldAdd.length}; predicted saving $${shadow.predictedSavingUsd.toFixed(2)}${shadow.unpricedCases ? ` (${shadow.unpricedCases} unpriced)` : ''}, a lower bound: known cost excludes the ${Math.round(100 * (shadow.unknownCostShare ?? 0))}% of observed case runs with unknown cost (PTY) (${EXERCISED_MAP_ANCHOR})`); diff --git a/test/pr-dependencies.test.ts b/test/pr-dependencies.test.ts index 767459774..95abcfb08 100644 --- a/test/pr-dependencies.test.ts +++ b/test/pr-dependencies.test.ts @@ -11,7 +11,7 @@ import * as path from 'node:path'; import { spawnSync } from 'node:child_process'; import { deriveDependencies, derivedDependencies, referenceText } from '../scripts/pr-dependencies'; import { - DERIVABLE_PREFIXES, PERIODIC_ONLY_PR_FILES, PR_FULL_GATE_FILES, PR_PROFILE_MAPS, selectPrProfile, unknownFileLabel, + DERIVABLE_PREFIXES, PERIODIC_ONLY_PR_FILES, PR_FULL_GATE_FILES, PR_PROFILE_MAPS, formatPrCoverageSummary, selectPrProfile, unknownFileLabel, } from '../scripts/test-pr-profile'; import { buildRunManifest, computePaidCaseSelection } from '../scripts/test-paid-shards'; import { E2E_TIERS } from './helpers/touchfiles-data'; @@ -336,11 +336,14 @@ describe('narrow screens leave hub files to the train gate', () => { expect(gate.every(id => real.e2e.get(hub)?.has(id))).toBe(true); }); - test('a hub-only diff selects no case on a narrow screen and names the hub file', () => { + test('a hub-only diff selects no case or judge on a narrow screen and names the hub file', () => { const result = plan([hub]); expect(result.coverage?.hubFiles).toEqual([hub]); expect(result.coverage?.mode).toBe('pr'); - expect(result.selection.e2e).toEqual([]); + expect(result.selection).toEqual({ e2e: [], judges: [] }); + expect(plan([hub], 'broad').selection.judges!.length).toBeGreaterThan(0); + expect(formatPrCoverageSummary({ profile: 'pr', selection: result.selection, prCoverage: result.coverage }).join('\n')) + .toContain(`Left to the train gate's full census (narrow screen): \`${hub}\``); expect(result.reason).toContain(`train gate's full census runs them: ${hub}`); }); @@ -365,6 +368,20 @@ describe('narrow screens leave hub files to the train gate', () => { expect(plan([hub, 'browse/src/retired-module.ts']).selection.e2e).toEqual(direct); }); + test('a deleted file that still reaches every gate case is not a hub: a narrow screen keeps its dependents', () => { + const without = { ...real, tracked: new Set([...real.tracked].filter(file => file !== hub)) }; + const result = selectPrProfile({ selectedE2E: [], selectedJudges: [], changedFiles: [hub], derived: without, screenPolicy: 'narrow' }); + expect(result.hubFiles).toEqual([]); + expect(result.mode).toBe('dependents'); + expect(result.e2e).toEqual(gate.sort()); + }); + + test('only dispatched screens default to narrow in the PR workflow; a pull_request run plans broad', () => { + const workflow = fs.readFileSync(path.join(ROOT, '.github', 'workflows', 'evals.yml'), 'utf8'); + expect(workflow).toContain("EVALS_SCREEN_POLICY: ${{ inputs.screen_policy || (github.event_name == 'workflow_dispatch' && 'narrow') || 'broad' }}"); + expect(workflow).not.toContain("EVALS_SCREEN_POLICY: ${{ inputs.screen_policy || 'narrow' }}"); + }); + test('size-ledger baselines select only the cases that read them, none in the gate', () => { const result = plan(['test/size-ledger/ship/base.json']); expect(result.coverage?.unknownFiles).toEqual([]);