fix(fix): rerun a session's failing test files once before rejecting its edits (UPT-111)

.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.

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).

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<TaskRunner>(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<TaskRunner>(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<TaskRunner>(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.',
+    );
+  },
+);

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.`,

packages/core/src/fix/run.ts

@@ -956,6 +956,12 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
       ...testOptions,
       agentCheck: true,
     });
+  // A gate's failing test files, run once more alone (UPT-111).
+  const rerunTests = (files: string[]) =>
+    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<F
               related: true,
             }),
           ...(narrowCheck ? { checkTests: agentCheckTests } : {}),
+          rerun: rerunTests,
           ...(services.lint
             ? {
                 lint: (files: string[], b?: LintResult[]) =>
@@ -1165,7 +1172,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
     : fixer
       ? feedbackPatchRunner(fixer, feedbackFinding)
       : undefined;
-  const feedbackLimits = {
+  const feedbackLimits: NonNullable<FixReport['llm']['feedback']> = {
     roundsUsed: 0,
     maxRounds: 3,
     maxCostUsd,
@@ -1184,6 +1191,17 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
     const remaining =
       (nanos(maxCostUsd) - nanos(llm.costUsd) - nanos(llm.unreportedCostUsd ?? 0)) / 1e9;
     if (remaining < 0.02 || llm.unreportedCostUsd) break;
+    // A session that would likely cross the limit is not started: one that does stops at the
+    // limit mid-edit, and its spend buys nothing (UPT-111).
+    // ponytail: the mean of this run's sessions; a per-pack history if sessions vary widely.
+    const sessions = (llm.tasks ?? []).filter((t) => 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<F
           fullSuite: true,
           withServices: options.withServices === true,
         }),
+      rerun: rerunTests,
       baselineTests,
       baselineLint,
       affected,

packages/core/src/fix/sdk-agent.test.ts

@@ -116,7 +116,10 @@ async function task(input: {
   /** The tests when the task starts: after the upgrade, before the agent. */
   startTests?: TestResult[];
   baselineTests?: TestResult[];
+  /** The failing files run again alone; absent, the gate has no rerun. */
+  rerun?: TestResult[];
 }) {
+  const reruns: string[][] = [];
   const root = repo({ 'src/a.ts': ORIGINAL, 'src/other.ts': 'export const x = 1;\n' });
   const siteError = diag('src/a.ts', 2, 2339, "Property 'current_period_end' does not exist");
   const preexisting = diag('src/a.ts', 3, 2304, "Cannot find name 'line3'.");
@@ -161,8 +164,16 @@ async function task(input: {
     baselineLint: [],
     affected: ['src/a.ts'],
     runner: async ({ root }) => 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'] }),

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<TestResult[]>;
   /** The agent's own `run_tests`, when it may run less than the gate; the gate runs `tests`. */
   checkTests?: (files: string[]) => Promise<TestResult[]>;
+  /** These repository-relative test files alone, run again: how the gate tells a flaky file. */
+  rerun?: (files: string[]) => Promise<TestResult[]>;
   lint?: (files: string[], baseline?: LintResult[]) => Promise<LintResult[]>;
   /** The repository's formatter, run on the edited files before lint, as `settle` does. */
   format?: (files: string[]) => Promise<string[]>;
@@ -497,6 +505,7 @@ export async function assistWithSdk(input: SdkAssistInput): Promise<FixReport['l
     after: FixDiagnostic[],
     files: string[],
     sites: FixSite[],
+    record: AgentTaskRecord,
   ) => {
     // 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<FixReport['l
       { anyCommand: true },
     );
     // A failure the batch did not cause was there at its start: run the same tests there, once.
-    if (tests.some((t) => (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<FixReport['l
       current,
       [...new Set([...task.files, ...touched])],
       task.sites,
+      record,
     );
     if (verdict) {
       reset(taskStart);