diff --git a/.changeset/flaky-test-rerun.md b/.changeset/flaky-test-rerun.md new file mode 100644 index 0000000..292fc8b --- /dev/null +++ b/.changeset/flaky-test-rerun.md @@ -0,0 +1,5 @@ +--- +"uptide": patch +--- + +An SDK session's edits are no longer thrown away because of one flaky test. Before the gate rejects a session on test failures, it runs the failing test files once more, alone, with their runner and the same isolation as the full run. A file that passes then is recorded as flaky (`llm.tasks[].flaky` in `report.json`, and a line in the PR body's review list) and the edits stay. A file that fails twice still rejects them, and a failure that names no test file (a build, a timeout) is never rerun. Final verification keeps its full gate: it runs the flaky files again, and if one fails there, verification fails and the PR body says so. `fix` also stops starting verification feedback sessions when what is left of `--max-cost` is below the mean cost of the run's sessions so far, so it no longer starts a session it can't afford; the PR's feedback line says when this happened. diff --git a/docs/commands/fix.md b/docs/commands/fix.md index bc9cb37..f96c7d9 100644 --- a/docs/commands/fix.md +++ b/docs/commands/fix.md @@ -235,6 +235,12 @@ repository, and there: Anthropic defaults to one SDK session for the package's manual sites, with repository read/edit tools and Uptide's checks. Its edits are accepted together only if they add no type errors, lint failures or related-test failures compared with the session's start. + Before a test failure rejects them, the failing test files run once more, alone; a file + that passes then is named as flaky in `report.json` (`llm.tasks[].flaky`) and the PR body + and does not reject the edits, and one that fails twice does. Final verification still + runs the flaky files in its full run, where a failure fails it. A verification feedback + session is not started when what is left of `--max-cost` is below the mean cost of the + run's sessions so far; the PR's feedback line says so. OpenAI, Gemini and `--agent patch` use snippet patches judged per attempt, including the pack's validator. Rejected edits are reverted and left manual. `--no-llm` turns assistance off. Which provider and what it costs: [models](../models.md). diff --git a/packages/core/src/fix/feedback.test.ts b/packages/core/src/fix/feedback.test.ts index 9f043ff..3dd6492 100644 --- a/packages/core/src/fix/feedback.test.ts +++ b/packages/core/src/fix/feedback.test.ts @@ -86,14 +86,19 @@ it('feeds a test-only UUID regression to the agent and verifies the guid repair' expect(prBody(result)).not.toContain('No code changes were needed'); }); -it('bounds feedback rounds by the remaining run budget and reports unresolved tests', async () => { +it('starts no session the remaining budget is below a typical session for (UPT-111)', async () => { const { root, services } = await fixture(); const runner = vi.fn(async () => runResult); const result = await fix(options(root), { ...services, agentRunner: runner }); - expect(runner).toHaveBeenCalledTimes(3); - expect(runner.mock.calls.map(([input]) => input.maxBudgetUsd)).toEqual([0.25, 0.15, 0.05]); + // $0.10 a session against $0.25: a third would start with $0.05 and cross the limit. + expect(runner.mock.calls.map(([input]) => input.maxBudgetUsd)).toEqual([0.25, 0.15]); + expect(result.llm.costUsd).toBeLessThanOrEqual(0.25); + expect(result.llm.feedback?.stopped).toEqual({ remainingUsd: 0.05, typicalUsd: 0.1 }); expect(result.verification.passed).toBe(false); expect(prBody(result)).toContain('Invalid UUID'); + expect(prBody(result)).toContain( + "No further round: $0.05 left, below a session's typical $0.10.", + ); }); it('does not send failures already present at the parent to the agent', async () => { @@ -321,9 +326,9 @@ it('reports rounds used/max and total spend/cap, including zero feedback rounds' if (passes) services.tests = async () => [{ workspace: '.', status: 'passed', output: '' }]; const runner = vi.fn(async () => runResult); const result = await fix(options(root), { ...services, agentRunner: runner }); - expect(prBody(result)).toContain(`Feedback rounds: ${passes ? 0 : 3}/3`); - expect(formatFix(result)).toContain(`Feedback rounds: ${passes ? 0 : 3}/3`); - expect(prBody(result)).toContain(`Spend: $${passes ? '0.00' : '0.30'} / $0.25 cap`); + expect(prBody(result)).toContain(`Feedback rounds: ${passes ? 0 : 2}/3`); + expect(formatFix(result)).toContain(`Feedback rounds: ${passes ? 0 : 2}/3`); + expect(prBody(result)).toContain(`Spend: $${passes ? '0.00' : '0.20'} / $0.25 cap`); } }); @@ -370,3 +375,49 @@ it.each(['failed', 'timeout'] as const)( expect(prBody(result)).toContain('old type sentinel'); }, ); + +it.each([ + ['passes in verification', false], + ['fails verification again', true], +] as const)( + 'keeps a session whose new failure passes when run again alone, and names it: %s (UPT-111)', + async (_, again) => { + const { root, services } = await fixture(); + const tests = services.tests; + const reruns: string[][] = []; + let flakes = 0; + services.tests = async (cwd, workspaces, timeout, files, o) => { + if (o?.only) { + reruns.push(files ?? []); + return [{ workspace: '.', status: 'passed', output: '', command: 'vitest run' }]; + } + const results = await tests(cwd, workspaces, timeout, files, o); + const repaired = readFileSync(join(root, 'index.ts'), 'utf8').includes('.guid()'); + if (!repaired || (flakes++ > 0 && !again)) return results; + return results.map((t) => ({ + ...t, + status: 'failed' as const, + output: `${t.output}\n FAIL flaky.test.ts > admission > times out`, + })); + }; + const runner = vi.fn(async () => { + writeFileSync( + join(root, 'index.ts'), + readFileSync(join(root, 'index.ts'), 'utf8').replace('.uuid()', '.guid()'), + ); + return runResult; + }); + const result = await fix(options(root), { ...services, agentRunner: runner }); + expect(result.llm.tasks?.[0]).toMatchObject({ gate: 'accepted', flaky: ['flaky.test.ts'] }); + expect(reruns[0]).toEqual(['flaky.test.ts']); + expect(readFileSync(join(root, 'index.ts'), 'utf8')).toContain('.guid()'); + expect(prBody(result)).toContain('Flaky: `flaky.test.ts` failed in an agent session'); + // The final verification keeps its full gate: a flaky failure there is a failure. + expect(result.verification.passed).toBe(!again); + expect(prBody(result)).toContain( + again + ? "Verification's full run failed `flaky.test.ts` again: rerun before merging." + : 'Verification ran it in its full run.', + ); + }, +); diff --git a/packages/core/src/fix/report.ts b/packages/core/src/fix/report.ts index b9f844b..8a540b1 100644 --- a/packages/core/src/fix/report.ts +++ b/packages/core/src/fix/report.ts @@ -22,7 +22,7 @@ import { sdkFallbackNote } from './sdk-agent.js'; import { stripeChangelog } from './stripe-changelog.js'; import { isTestFile } from './test-edits.js'; import type { FixDiagnostic, FixPhase, FixReport, FixSite, ReviewSection } from './types.js'; -import { missingPackages, newDiagnostics } from './verify.js'; +import { failedTestFiles, missingPackages, newDiagnostics } from './verify.js'; export function fixCounts(report: FixReport) { report = { ...report, sites: uniqueSites(report.sites) }; @@ -1192,10 +1192,10 @@ export function renderMigration( '', ); if (report.llm.feedback) { - const { roundsUsed, maxRounds, maxCostUsd } = report.llm.feedback; + const { roundsUsed, maxRounds, maxCostUsd, stopped } = report.llm.feedback; lines.push( '', - `Feedback rounds: ${roundsUsed}/${maxRounds}. Spend: $${report.llm.costUsd.toFixed(2)} / $${maxCostUsd.toFixed(2)} cap${report.llm.unreportedCostUsd ? `; $${report.llm.unreportedCostUsd.toFixed(2)} additionally reserved (usage unknown)` : ''}.`, + `Feedback rounds: ${roundsUsed}/${maxRounds}. Spend: $${report.llm.costUsd.toFixed(2)} / $${maxCostUsd.toFixed(2)} cap${report.llm.unreportedCostUsd ? `; $${report.llm.unreportedCostUsd.toFixed(2)} additionally reserved (usage unknown)` : ''}.${stopped ? ` No further round: $${stopped.remainingUsd.toFixed(2)} left, below a session's typical $${stopped.typicalUsd.toFixed(2)}.` : ''}`, ); } const changedTests = (report.changedFiles ?? []).filter(isTestFile); @@ -1265,6 +1265,17 @@ export function renderMigration( ? `- Tests in \`${basename(t.workspace)}\` were failing before this change (${t.preexisting[0] === '*' ? 'the whole run' : t.preexisting.map((f) => `\`${basename(f)}\``).join(', ')}); not caused here, but a green suite would verify more.` : `- Tests in \`${basename(t.workspace)}\` ${t.status}; fix or rerun before merging.`, ); + // Kept edits rest on these files' second run; verification judged them in its full gate. + const flaky = [...new Set((report.llm.tasks ?? []).flatMap((t) => t.flaky ?? []))].sort(); + if (flaky.length) { + const failing = report.verification.tests + .filter((t) => t.status === 'failed') + .flatMap((t) => failedTestFiles(t.output)); + const still = flaky.filter((f) => failing.some((g) => f.endsWith(g))); + review.push( + `- Flaky: ${flaky.map((f) => `\`${f}\``).join(', ')} failed in an agent session's tests and passed when run again alone, so the session's edits were kept. ${still.length ? `Verification's full run failed ${still.map((f) => `\`${basename(f)}\``).join(', ')} again: rerun before merging.` : `Verification ran ${flaky.length === 1 ? 'it' : 'them'} in its full run.`}`, + ); + } for (const t of report.verification.tests.filter((t) => t.notRun && t.notRun.files > 0)) review.push( `- ${count(t.notRun?.files ?? 0, 'integration test file')} not run (need ${t.notRun?.needs.join('/')}). Run with \`--with-services\` to include them.`, diff --git a/packages/core/src/fix/run.ts b/packages/core/src/fix/run.ts index 16b02f4..9aecaad 100644 --- a/packages/core/src/fix/run.ts +++ b/packages/core/src/fix/run.ts @@ -956,6 +956,12 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise + services.tests(root, workspaces, options.testTimeoutMs, files, { + withServices: options.withServices === true, + only: true, + }); const llm = await progress(options.onProgress, { phase: 'assist', package: pack.name }, () => sdkAgent ? assistWithSdk({ @@ -974,6 +980,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise @@ -1165,7 +1172,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise = { roundsUsed: 0, maxRounds: 3, maxCostUsd, @@ -1184,6 +1191,17 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise t.costUsd > 0); + const typicalUsd = sessions.length + ? sessions.reduce((n, t) => n + t.costUsd, 0) / sessions.length + : 0; + if (remaining < typicalUsd) { + feedbackLimits.stopped = { remainingUsd: remaining, typicalUsd }; + break; + } // Never let a rejected agent task reset files written by a check. if (git(root, 'status', '--porcelain', '--untracked-files=all')) break; const files = feedbackFiles(root); @@ -1209,6 +1227,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise ran(input.agent(root)), + ...(input.rerun + ? { + rerun: async (files: string[]) => { + reruns.push(files); + return input.rerun ?? []; + }, + } + : {}), }); - return { root, sites, record: llm.tasks?.[0] }; + return { root, sites, record: llm.tasks?.[0], reruns }; } describe('guard: what an SDK session may touch', () => { @@ -337,6 +348,49 @@ describe('end-of-task gate', () => { }); expect(unnamed.record?.rejection).toMatch(/tests fail/); }); + it('keeps edits when only a test file that fails once and passes alone failed (UPT-111)', async () => { + const flaky = 'FAIL test/private-admission.test.ts > admission > creates an owner'; + const kept = await task({ + agent: doing(migrate), + after: [], + baselineTests: [{ workspace: '.', status: 'failed', output: 'FAIL src/old.test.ts' }], + tests: [{ workspace: '.', status: 'failed', output: `FAIL src/old.test.ts\n${flaky}` }], + rerun: [{ workspace: '.', status: 'passed', output: '' }], + }); + expect(kept.record?.gate).toBe('accepted'); + expect(kept.record?.flaky).toEqual(['test/private-admission.test.ts']); + // Only the new failure runs again: the parent's own failure would fail any rerun. + expect(kept.reruns).toEqual([['test/private-admission.test.ts']]); + expect(kept.sites[0]?.outcome).toBe('agent'); + expect(git(kept.root, 'log', '-1', '--format=%s')).toBe( + 'fix(stripe): subscription-period (agent)', + ); + + const twice = await task({ + agent: doing(migrate), + after: [], + tests: [{ workspace: '.', status: 'failed', output: flaky }], + rerun: [{ workspace: '.', status: 'failed', output: flaky }], + }); + expect(twice.record?.gate).toBe('rejected'); + expect(twice.record?.rejection).toMatch(/tests fail/); + expect(twice.record?.flaky).toBeUndefined(); + expect(readFileSync(join(twice.root, 'src/a.ts'), 'utf8')).toBe(ORIGINAL); + + // No run is no pass: a rerun that ran nothing, or a failure that names no file, stands. + for (const [tests, rerun] of [ + [flaky, { workspace: '.', status: 'missing', output: 'no runner runs it alone' }], + ['build failed', { workspace: '.', status: 'passed', output: '' }], + ] as const) { + const stands = await task({ + agent: doing(migrate), + after: [], + tests: [{ workspace: '.', status: 'failed', output: tests }], + rerun: [rerun], + }); + expect(stands.record?.gate).toBe('rejected'); + } + }); it("keeps the site manual, in the agent's own words, when it changes nothing", async () => { const { sites, record } = await task({ agent: () => summary({ changed: [], couldNotDo: ['the period needs a product decision'] }), diff --git a/packages/core/src/fix/sdk-agent.ts b/packages/core/src/fix/sdk-agent.ts index 3974ea7..71fc1f6 100644 --- a/packages/core/src/fix/sdk-agent.ts +++ b/packages/core/src/fix/sdk-agent.ts @@ -34,7 +34,13 @@ import type { LintResult, TestResult, } from './types.js'; -import { gitLineMoves, markPreexisting, newDiagnostics, unplacedKey } from './verify.js'; +import { + failedTestFiles, + gitLineMoves, + markPreexisting, + newDiagnostics, + unplacedKey, +} from './verify.js'; /** * The Anthropic default: assisted fixes run as Claude Agent SDK sessions in the @@ -91,6 +97,8 @@ export interface SdkAssistInput { tests: (files: string[]) => Promise; /** The agent's own `run_tests`, when it may run less than the gate; the gate runs `tests`. */ checkTests?: (files: string[]) => Promise; + /** These repository-relative test files alone, run again: how the gate tells a flaky file. */ + rerun?: (files: string[]) => Promise; lint?: (files: string[], baseline?: LintResult[]) => Promise; /** The repository's formatter, run on the edited files before lint, as `settle` does. */ format?: (files: string[]) => Promise; @@ -497,6 +505,7 @@ export async function assistWithSdk(input: SdkAssistInput): Promise { // An error a line move explains is the old one (`gitLineMoves`): lines the batch edited stay put. const fresh = newDiagnostics(before, after, gitLineMoves(root, start)); @@ -517,11 +526,36 @@ export async function assistWithSdk(input: SdkAssistInput): Promise (t.status === 'failed' || t.status === 'timeout') && !t.preexisting)) - tests = markPreexisting(tests, await testsAt(start, related)); - const failing = tests.filter( + let atStart: TestResult[] = []; + if (tests.some((t) => (t.status === 'failed' || t.status === 'timeout') && !t.preexisting)) { + atStart = await testsAt(start, related); + tests = markPreexisting(tests, atStart); + } + let failing = tests.filter( (t) => t.status !== 'passed' && t.status !== 'missing' && !t.preexisting, ); + // A test file that failed here and passes when it alone runs again is flaky, not the + // batch's: the edits stay and the file is named. One that fails twice still rejects, and a + // run that names no failing file (a build, a timeout) has nothing to run again (UPT-111). + if (failing.length && input.rerun && failing.every((t) => t.status === 'failed')) { + const named = (t: TestResult) => + failedTestFiles(t.output).map((f) => + existsSync(join(root, t.workspace, f)) ? join(t.workspace, f) : f, + ); + const known = new Set( + [...input.baselineTests, ...atStart].filter((t) => t.status !== 'passed').flatMap(named), + ); + const ours = failing.map((t) => named(t).filter((f) => !known.has(f))); + const flaky = [...new Set(ours.flat())].sort(); + const rerun = input.rerun; + const again = ours.every((files) => files.length) + ? await withFollowUps(sites, () => rerun(flaky)) + : []; + if (again.length && again.every((t) => t.status === 'passed')) { + record.flaky = flaky; + failing = []; + } + } if (failing.length) return `tests fail: ${failing.map((t) => `${t.workspace} (${t.status})`).join(', ')}`; return undefined; @@ -687,6 +721,7 @@ export async function assistWithSdk(input: SdkAssistInput): Promise { ).rejects.toThrow(/no supported lockfile found/); }); }); + +describe('running failing test files again alone (UPT-111)', () => { + it('runs exactly the files with their runner, isolated as the full run, never a script', async () => { + const root = repo(); + // A test script that builds first is skipped: the first run built this tree. + writeFileSync( + join(root, 'packages/api/package.json'), + JSON.stringify({ name: 'api', scripts: { test: 'pnpm run build && vitest run' } }), + ); + const failed = ['packages/api/src/billing.test.ts', 'packages/api/e2e/flow.test.ts']; + const plans = planTests(root, ['packages/api'], failed, { only: true }); + expect(plans.map((p) => p.command)).toEqual([ + 'vitest run --config vitest.uptide-unit.config.mjs packages/api/src/billing.test.ts', + // A file the unit-only run leaves out runs nowhere: no run, no pass. + '', + ]); + expect(plans[1]?.scope).toBe('no runner runs packages/api/e2e/flow.test.ts alone'); + const results = await testWorkspaces(root, ['packages/api'], 10_000, failed, { only: true }); + expect(results.map((r) => r.status)).toEqual(['passed', 'missing']); + expect(results[0]?.output).toContain('vitest.uptide-unit.config.mjs'); + expect( + planTests(root, ['packages/api'], [failed[0] as string], { + only: true, + withServices: true, + }).map((p) => p.command), + ).toEqual(['vitest run packages/api/src/billing.test.ts']); + }); + + it('gives jest the exact paths', () => { + const root = repo('jest'); + writeFileSync(join(root, 'jest.config.ts'), 'export default {};'); + const [plan] = planTests(root, ['packages/api'], ['packages/api/src/billing.test.ts'], { + only: true, + }); + expect(plan?.command).toBe( + "jest --runTestsByPath packages/api/src/billing.test.ts --testPathIgnorePatterns '\\.(integration|int|e2e)\\.' '/(integration|e2e)/'", + ); + }); +}); diff --git a/packages/core/src/fix/types.ts b/packages/core/src/fix/types.ts index 7cd0a3b..b0ca9b4 100644 --- a/packages/core/src/fix/types.ts +++ b/packages/core/src/fix/types.ts @@ -162,6 +162,11 @@ export interface AgentTaskRecord { rejection?: string; /** Undeclared new files removed before validating and checking the remaining edits. */ discardedFiles?: string[]; + /** + * Test files that failed in the gate and passed when run again alone: kept as flaky, not + * counted against the edits. Final verification still runs them in its full gate. + */ + flaky?: string[]; /** The SDK's stop reason when it did not finish cleanly (`error_max_budget_usd`). */ stop?: string; /** How many single-site tasks shared this task's gate. */ @@ -347,7 +352,13 @@ export interface FixReport { /** Why the SDK, the Anthropic default, could not run, so the patch executor did. */ sdkMissing?: string; tasks?: AgentTaskRecord[]; - feedback?: { roundsUsed: number; maxRounds: number; maxCostUsd: number }; + feedback?: { + roundsUsed: number; + maxRounds: number; + maxCostUsd: number; + /** No further round started: what was left of the limit, below a typical session's cost. */ + stopped?: { remainingUsd: number; typicalUsd: number }; + }; }; timingMs: number; /** Wall time and agent spend per phase, in run order; together about `timingMs`. */ diff --git a/packages/core/src/fix/verify.ts b/packages/core/src/fix/verify.ts index 4263523..ddeeed2 100644 --- a/packages/core/src/fix/verify.ts +++ b/packages/core/src/fix/verify.ts @@ -645,6 +645,12 @@ export interface TestOptions { agentCheck?: boolean; /** Run every test in the selected workspaces, including detected runner configurations. */ fullSuite?: boolean; + /** + * `files` are test files a run named as failing: run exactly those with their runner, isolated + * as the full run is, and nothing else: no `test` script, no fallback, no retry. A file no + * runner can take alone is not run, and no run is no pass (UPT-111). + */ + only?: boolean; } const VITEST_CONFIGS = ['vitest.config', 'vitest.workspace'].flatMap((base) => @@ -787,7 +793,10 @@ export function planTests( { runner: 'vitest' | 'jest'; dir: string; config: string; covers: string[] } >(); const rootScript: string[] = []; - const selected = options.related ? relatedWorkspaces(root, workspaces, files) : workspaces; + /** With `only`: the files a plan runs, absolute. */ + const taken = new Set(); + const selected = + options.related || options.only ? relatedWorkspaces(root, workspaces, files) : workspaces; for (const workspace of selected) { const script = manifest(join(root, workspace))?.scripts?.test; // The script a `test` chain ends by running: `… && pnpm run test:unit`. @@ -811,7 +820,7 @@ export function planTests( } // A script that is just the runner is planned as the runner: scoped, and classified. const bare = /^\s*(?:(?:npx|pnpm exec|yarn)\s+)?(vitest|jest)\b/.exec(script ?? '')?.[1]; - if (script && !(bare && runnerFor(root, workspace)?.runner === bare)) { + if (script && !options.only && !(bare && runnerFor(root, workspace)?.runner === bare)) { plans.push({ workspace, covers: [workspace], @@ -829,6 +838,7 @@ export function planTests( shared.set(key, group); continue; } + if (options.only) continue; if (workspace !== '.' && manifest(root)?.scripts?.test) rootScript.push(workspace); else plans.push({ @@ -879,6 +889,33 @@ export function planTests( : group.runner === 'vitest' ? ['--config', wrapper] : ['--testPathIgnorePatterns', '\\.(integration|int|e2e)\\.', '/(integration|e2e)/']; + if (options.only) { + if (!targets.length) continue; + for (const file of targets) taken.add(join(group.dir, file)); + // The first run already ran `pretest` on this tree; the files alone run again. + plans.push({ + workspace, + covers: group.covers, + cwd: group.dir, + command: [ + group.runner, + ...(group.runner === 'vitest' + ? ['run', ...isolate, ...targets] + : ['--runTestsByPath', ...targets, ...isolate] + ).map(quote), + ].join(' '), + scope: `${group.runner} on ${targets.length} test file${targets.length === 1 ? '' : 's'} that failed, run again alone`, + ...(unitOnly && group.runner === 'vitest' + ? { + write: { + path: join(group.dir, wrapper), + text: unitConfig(group.config, needs.setups.length > 0), + }, + } + : {}), + }); + continue; + } const args = group.runner === 'vitest' ? targets.length @@ -943,6 +980,15 @@ export function planTests( : {}), }); } + const untaken = options.only ? files.filter((file) => !taken.has(join(root, file))) : []; + if (untaken.length) + plans.push({ + workspace: '.', + covers: [], + cwd: root, + command: '', + scope: `no runner runs ${untaken.join(', ')} alone`, + }); if (rootScript.length) plans.push({ workspace: '.', @@ -1099,7 +1145,13 @@ export async function testWorkspaces( let fellBack: TestResult['fallback']; try { result = await attempt(); - if (result.code && !result.timeout && !options.related && !options.fullSuite) { + if ( + result.code && + !result.timeout && + !options.related && + !options.fullSuite && + !options.only + ) { // A failure in a test that is not about the affected files gets one more run: a socket // reset in an unrelated e2e test must not fail a migration. A test of an affected file // that fails is ours, and is never retried into a pass.