diff --git a/.changeset/agent-phase-wall-time.md b/.changeset/agent-phase-wall-time.md new file mode 100644 index 0000000..d46dcd5 --- /dev/null +++ b/.changeset/agent-phase-wall-time.md @@ -0,0 +1,9 @@ +--- +"uptide": patch +--- + +`uptide fix` spends less wall time on test runs and dependency copies: + +- The agent's own `run_tests` check runs only its runner's related tests when the `test` script builds first and then runs a script that is just the runner (`pnpm run build && pnpm run test:unit`). The gate after the agent's edits and the final verification run exactly what they ran before. +- The upgrade's install moves the installed `node_modules` out of its temporary worktree instead of copying it. +- Each phase of the fix report lists the repository's commands it ran (installs, generators, builds, type-checks, tests, lint, format) with their count and summed wall time. An agent-led run's clean install has its own phase. diff --git a/packages/core/src/fix/fix.test.ts b/packages/core/src/fix/fix.test.ts index 85668ae..8a376a7 100644 --- a/packages/core/src/fix/fix.test.ts +++ b/packages/core/src/fix/fix.test.ts @@ -965,6 +965,37 @@ describe('test detection', () => { ]); }); + it("scopes only the agent's own check to the runner a test script chain ends with", () => { + const root = mkdtempSync(join(scratch, 'chain-')); + const write = (file: string, text: string, mode?: number) => { + mkdirSync(join(root, file, '..'), { recursive: true }); + writeFileSync(join(root, file), text, mode ? { mode } : {}); + }; + write('node_modules/.bin/vitest', '#!/bin/sh\n', 0o755); + const test = 'pnpm run build && pnpm run test:unit'; + write( + 'package.json', + JSON.stringify({ name: 'app', scripts: { build: 'tsc', test, 'test:unit': 'vitest run' } }), + ); + write('src/a.ts', ''); + const files = ['src/a.ts']; + expect(planTests(root, ['.'], files, { related: true, agentCheck: true })).toEqual([ + { + workspace: '.', + covers: ['.'], + cwd: root, + command: 'vitest related --run --passWithNoTests src/a.ts', + scope: + 'vitest tests related to 1 changed file (its `test` script ends with `test:unit`, the runner; the steps before it did not run)', + }, + ]); + // The gate and verification run the whole script, build first, as before. + expect(planTests(root, ['.'], files, { related: true }).map((p) => p.command)).toEqual([test]); + expect(planTests(root, ['.'], files, { fullSuite: true }).map((p) => p.command)).toEqual([ + test, + ]); + }); + it('knows jest, prefers a workspace script, then the root script, and says when nothing covers a workspace', () => { const jest = monorepo('jest'); expect( diff --git a/packages/core/src/fix/generic.test.ts b/packages/core/src/fix/generic.test.ts index 9508522..c2e9b67 100644 --- a/packages/core/src/fix/generic.test.ts +++ b/packages/core/src/fix/generic.test.ts @@ -305,6 +305,9 @@ describe('fix for a dependency without a pack', () => { 'feedback', ]); expect(result.phases?.find((p) => p.name === 'agent')?.costUsd).toBe(0.6); + // The repository's commands each phase ran, counted where they ran. + expect(result.phases?.find((p) => p.name === 'baseline')?.runs?.types?.count).toBe(1); + expect(result.phases?.find((p) => p.name === 'baseline')?.runs?.tests?.count).toBe(1); const total = (result.phases ?? []).reduce((ms, p) => ms + p.ms, 0); expect(total).toBeLessThanOrEqual(result.timingMs); expect(prBody(result)).toMatch(/Phases: analyze \d+s · baseline \d+s · .* agent \d+s \$0\.60/); @@ -382,6 +385,99 @@ describe('fix for a dependency without a pack', () => { ); }, 60000); + it.each([ + [ + 'ends by running the runner', + 'pnpm run build && pnpm run test:unit', + ['full', 'check', 'gate', 'full'], + false, + ], + // The check is the gate's own run, whose result the gate reuses on the same tree. + ['is the runner', 'vitest run', ['full', 'gate', 'full'], false], + // Agent-led: the forced feedback round's check and its after-round run are the full + // suite, and the branch must still install from scratch (UPT-102). + [ + 'ends by running the runner, agent-led', + 'pnpm run build && pnpm run test:unit', + ['full', 'check', 'gate', 'full', 'full', 'full'], + true, + ], + ])( + "narrows only the agent's own check, where the test script %s; the gate and the final verification run as before", + async (_, test, expected, agentLed) => { + const { root, services, fixer } = paintFixture(); + const manifest = JSON.parse(readFileSync(join(root, 'package.json'), 'utf8')); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ ...manifest, scripts: { build: 'tsc', test, 'test:unit': 'vitest run' } }), + ); + mkdirSync(join(root, 'node_modules/.bin'), { recursive: true }); + writeFileSync(join(root, 'node_modules/.bin/vitest'), '#!/bin/sh\n', { mode: 0o755 }); + git(root, 'commit', '-qam', 'test scripts'); + const calls: { + files: string[]; + related?: boolean; + agentCheck?: boolean; + fullSuite?: boolean; + }[] = []; + const result = await fix( + { + cwd: root, + only: 'paint', + fixer: fixer(0.01), + agent: 'sdk', + ...(agentLed ? { agentLed: ['a peer blocker the agent continues past'] } : {}), + }, + { + ...services, + tests: async (_root, _workspaces, _timeout, files = [], options = {}) => { + calls.push({ files, ...options }); + return [{ workspace: '.', status: 'passed', output: '' }]; + }, + agentRunner: async ({ root, checks }) => { + writeFileSync( + join(root, 'wall1.ts'), + "import { fill } from 'paint';\nfill({ name: 'red' });\n", + ); + await checks.tests(); + return { + turns: 2, + inputTokens: 1, + outputTokens: 1, + costUsd: 0.01, + durationMs: 1, + toolCalls: {}, + summary: { + changed: [], + newFiles: [], + decisions: [], + assumptions: [], + couldNotDo: [], + }, + }; + }, + }, + ); + expect(result.verification.passed).toBe(true); + expect(result.sites.map((s) => s.outcome)).toEqual(['agent']); + const kind = (c: (typeof calls)[number]) => + c.agentCheck ? 'check' : c.fullSuite ? 'full' : c.related ? 'gate' : 'other'; + // Baseline in full; the agent's check; its gate as before; then the full suite. + expect(calls.map(kind)).toEqual(expected); + expect(calls.at(-1)).toMatchObject({ fullSuite: true }); + expect(calls.at(-1)?.related).toBeUndefined(); + // The final verification still builds, type-checks and tests the head the agent left. + const verify = result.phases?.find((p) => p.name === 'verify'); + expect(verify?.runs?.build?.count).toBe(1); + expect(verify?.runs?.types?.count).toBe(1); + expect(verify?.runs?.tests?.count).toBe(1); + const clean = result.phases?.find((p) => p.name === 'clean-install'); + expect(clean?.runs?.install?.count).toBe(agentLed ? 1 : undefined); + expect(result.cleanInstall?.passed).toBe(agentLed ? true : undefined); + }, + 30000, + ); + it('stops before any agent call when the estimate exceeds the limit', async () => { for (const [costSetting, raise] of [ [undefined, 'Run again with --max-cost 0.18.'], diff --git a/packages/core/src/fix/managers/isolated-install.test.ts b/packages/core/src/fix/managers/isolated-install.test.ts index da8ab37..293fb35 100644 --- a/packages/core/src/fix/managers/isolated-install.test.ts +++ b/packages/core/src/fix/managers/isolated-install.test.ts @@ -1,4 +1,14 @@ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readlinkSync, + rmSync, + statSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, expect, it } from 'vitest'; @@ -69,6 +79,34 @@ it('installs in a detached worktree, promotes lockfile bytes unchanged and clean expect(existsSync(temporary)).toBe(false); expect(git(root, 'worktree', 'list')).not.toContain('uptide-install-'); }); +it('moves the installed tree out of the worktree, links pointing into it relocated', async () => { + const root = fixture(); + let installed = 0; + await isolatedUpgrade(root, upgrade, async (dir) => { + mkdirSync(join(dir, 'node_modules/.bin'), { recursive: true }); + writeFileSync(join(dir, 'node_modules/target'), 'new'); + installed = statSync(join(dir, 'node_modules/target')).ino; + symlinkSync('../target', join(dir, 'node_modules/.bin/relative')); + symlinkSync(join(dir, 'node_modules/target'), join(dir, 'node_modules/.bin/absolute')); + writeFileSync(join(dir, 'package-lock.json'), '{"lockfileVersion":3,"packages":{}}\n'); + return { + file: 'package-lock.json', + manager: 'npm', + added: ['target'], + removed: [], + changed: [], + allowed: ['target'], + }; + }); + // The same file the install wrote, not a copy of it. + expect(statSync(join(root, 'node_modules/target')).ino).toBe(installed); + expect(existsSync(join(root, 'node_modules/baseline'))).toBe(false); + expect(readlinkSync(join(root, 'node_modules/.bin/relative'))).toBe('../target'); + expect(readlinkSync(join(root, 'node_modules/.bin/absolute'))).toBe( + join(root, 'node_modules/target'), + ); + expect(readFileSync(join(root, 'node_modules/.bin/absolute'), 'utf8')).toBe('new'); +}); it('failed/out-of-scope installs never replace the branch lockfile or dependencies', async () => { const root = fixture(), before = readFileSync(join(root, 'package-lock.json'), 'utf8'); diff --git a/packages/core/src/fix/managers/isolated-install.ts b/packages/core/src/fix/managers/isolated-install.ts index a124cae..69eec91 100644 --- a/packages/core/src/fix/managers/isolated-install.ts +++ b/packages/core/src/fix/managers/isolated-install.ts @@ -31,6 +31,19 @@ function relocateLinks(dir: string, from: string, to: string): void { } } } +/** + * The installed tree moves out of the worktree, which is removed anyway, rather than being + * copied: a copy of a large node_modules cost seconds (enki: 7.5 s) and gave the same tree. + * Across filesystems it is copied as before. + */ +function moveTree(source: string, target: string): void { + try { + renameSync(source, target); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EXDEV') throw error; + cpSync(source, target, { recursive: true, verbatimSymlinks: true }); + } +} /** Promote only a validated lockfile and its already-installed dependencies. No install runs on the branch. */ export async function isolatedUpgrade( root: string, @@ -71,7 +84,7 @@ export async function isolatedUpgrade( if (!existsSync(source) && !existsSync(target)) continue; const staged = existsSync(source) ? join(staging, `new-${i}`) : undefined; if (staged) { - cpSync(source, staged, { recursive: true, verbatimSymlinks: true }); + moveTree(source, staged); relocateLinks(staged, checkout, root); } modules.push({ diff --git a/packages/core/src/fix/run.ts b/packages/core/src/fix/run.ts index b9eac7f..522f52b 100644 --- a/packages/core/src/fix/run.ts +++ b/packages/core/src/fix/run.ts @@ -63,7 +63,7 @@ import { describeRootPin, planRootPins, writeRootPins } from './root-pins.js'; import { assistWithSdk, sdkInstalled, type TaskRunner } from './sdk-agent.js'; import { selectedFindings } from './select.js'; import { applyFollowUps, commit, type Followed, safeFile, settle } from './settle.js'; -import { lintFiles } from './style.js'; +import { formatFiles, lintFiles } from './style.js'; import { type ResolvedTarget, resolveTarget } from './target.js'; import type { BuildResult, @@ -71,6 +71,7 @@ import type { Fixer, FixPhase, FixReport, + FixRunKind, FixSite, LintResult, TestResult, @@ -82,6 +83,7 @@ import { gitLineMoves, markPreexisting, newDiagnostics, + planTests, type TestOptions, testArtifacts, testWorkspaces, @@ -266,14 +268,37 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise = {}; const lap = (name: FixPhase['name'], costUsd?: number): void => { const now = Date.now(); - phases.push({ name, ms: now - lapped, ...(costUsd !== undefined ? { costUsd } : {}) }); + phases.push({ + name, + ms: now - lapped, + ...(costUsd !== undefined ? { costUsd } : {}), + ...(Object.keys(runs).length ? { runs } : {}), + }); lapped = now; + runs = {}; }; + // The repository's commands each phase runs, counted where they are called. + const timed = + (kind: FixRunKind, work: (...args: A) => R) => + (...args: A): R => { + const start = Date.now(); + const done = (): void => { + const entry = runs[kind] ?? { count: 0, ms: 0 }; + entry.count++; + entry.ms += Date.now() - start; + runs[kind] = entry; + }; + const result = work(...args); + if (result instanceof Promise) return result.finally(done) as R; + done(); + return result; + }; // Every build the run executes, preparation and verification, with its wall time. const buildRuns = { count: 0, ms: 0 }; - const timedBuild: typeof buildWorkspaces = async (...args) => { + const timedBuild: typeof buildWorkspaces = timed('build', async (...args) => { const start = Date.now(); try { const results = await (services.build ?? buildWorkspaces)(...args); @@ -282,7 +307,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise ({ ...g, phase: 'bump' as const }))); let consumed = rebuilt.builds; let consumedAt = treeState(); - const targetChecker = - sdkAgent && services.diagnostics === diagnostics ? typeChecker(root, workspaces) : undefined; - const checkTarget = targetChecker?.check ?? (() => services.diagnostics(root, workspaces)); + const targetChecker = sdkAgent && defaultDiagnostics ? typeChecker(root, workspaces) : undefined; + const checkTarget = targetChecker + ? timed('types', targetChecker.check) + : () => services.diagnostics(root, workspaces); const targetErrors = await verify(`types (${names(workspaces)})`, checkTarget); lap('install'); const sites: FixSite[] = []; @@ -902,6 +938,21 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise + planTests(root, workspaces, affected, o) + .map((p) => p.command) + .join('\n'); + const narrowCheck = commands({ ...testOptions, agentCheck: true }) !== commands(testOptions); + const agentCheckTests = (files: string[]) => + services.tests(root, workspaces, options.testTimeoutMs, files, { + ...testOptions, + agentCheck: true, + }); const llm = await progress(options.onProgress, { phase: 'assist', package: pack.name }, () => sdkAgent ? assistWithSdk({ @@ -919,6 +970,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise @@ -1160,6 +1212,7 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise { }, ); + it("runs the agent's own run_tests with checkTests, and gates with tests, never sharing results", async () => { + const root = repo({ 'src/a.ts': 'old a\n' }); + const calls: string[] = []; + const llm = await assistWithSdk({ + root, + workspaces: ['.'], + sites: [{ ...site('src/a.ts', 1, diag('src/a.ts', 1, 2339, 'old')), rule: 'a' }], + pack, + context: { from: '1.0.0', to: '2.0.0', includeDeprecated: false }, + model: 'claude-sonnet-5-5', + maxCostUsd: 1, + diagnostics: () => + readFileSync(join(root, 'src/a.ts'), 'utf8').includes('old') + ? [diag('src/a.ts', 1, 2339, 'old')] + : [], + tests: async () => { + calls.push('gate'); + return [{ workspace: '.', status: 'passed', output: '' }]; + }, + checkTests: async () => { + calls.push('check'); + return [{ workspace: '.', status: 'passed', output: 'related only' }]; + }, + lint: async () => [], + format: async () => [], + baselineTests: [{ workspace: '.', status: 'passed', output: '' }], + baselineLint: [], + affected: ['src/a.ts'], + runner: async ({ root, checks }) => { + writeFileSync(join(root, 'src/a.ts'), 'new a\n'); + expect(await checks.tests()).toContain('related only'); + // Repeated on the same tree, the check reuses its own result. + await checks.tests(); + return ran(summary()); + }, + }); + expect(llm.tasks?.[0]?.gate).toBe('accepted'); + // The gate runs its own tests on the same tree: a check's narrower result never stands in. + expect(calls).toEqual(['check', 'gate']); + }); + it('runs the start-of-task tests only to explain a failure, and restores the tree after', async () => { const { root, record } = await task({ agent: doing(migrate), diff --git a/packages/core/src/fix/sdk-agent.ts b/packages/core/src/fix/sdk-agent.ts index ef967b1..dbd8577 100644 --- a/packages/core/src/fix/sdk-agent.ts +++ b/packages/core/src/fix/sdk-agent.ts @@ -75,6 +75,8 @@ export interface SdkAssistInput { diagnostics: () => FixDiagnostic[]; /** Tests on these repository-relative files, as the run's `services.tests`. */ 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; lint?: (files: string[], baseline?: LintResult[]) => Promise; /** The repository's formatter, run on the edited files before lint, as `settle` does. */ format?: (files: string[]) => Promise; @@ -419,10 +421,11 @@ export async function assistWithSdk(input: SdkAssistInput): Promise(); - const runTests = async (start: string, files: string[]) => { + const runTests = async (start: string, files: string[], check = false) => { + const run = (check && input.checkTests) || input.tests; const key = () => { const hash = createHash('sha256') - .update(JSON.stringify([start, [...new Set(files)].sort()])) + .update(JSON.stringify([start, [...new Set(files)].sort(), run === input.tests])) .update(git(root, 'diff', '--binary', start)); for (const file of list('ls-files', '--others', '--exclude-standard').sort()) hash.update(JSON.stringify([file, readFileSync(join(root, file)).toString('base64')])); @@ -431,7 +434,7 @@ export async function assistWithSdk(input: SdkAssistInput): Promise `${r.tool}: ${r.status}\n${tail(r.output, 2000)}`).join('\n'); }, tests: async () => { - const results = await runTests(taskStart, [ - ...new Set([...task.files, ...since(taskStart)]), - ]); + const results = await runTests( + taskStart, + [...new Set([...task.files, ...since(taskStart)])], + true, + ); if (!results.length) return 'no tests relate to these files'; return [ "Tests related to the task's files. A failure may predate the task: the gate compares with the start.", diff --git a/packages/core/src/fix/types.ts b/packages/core/src/fix/types.ts index 2f64bdf..6c01361 100644 --- a/packages/core/src/fix/types.ts +++ b/packages/core/src/fix/types.ts @@ -203,11 +203,22 @@ export interface Fixer { fix(input: FixRequest, remainingUsd?: number): Promise; } export interface FixPhase { - name: 'analyze' | 'baseline' | 'install' | 'rules' | 'agent' | 'verify' | 'feedback'; + name: + | 'analyze' + | 'baseline' + | 'install' + | 'rules' + | 'agent' + | 'verify' + | 'feedback' + | 'clean-install'; ms: number; /** Agent spend in this phase; only the agent and feedback phases call an LLM. */ costUsd?: number; + /** The repository's own commands this phase ran: how many, and their summed wall time. */ + runs?: Partial>; } +export type FixRunKind = 'install' | 'generate' | 'build' | 'types' | 'tests' | 'lint' | 'format'; export interface FixReport { rootPins?: RootPin[]; /** Pack pipeline limitations: agent-led results can be published only as drafts. */ diff --git a/packages/core/src/fix/verify.ts b/packages/core/src/fix/verify.ts index f44d530..4263523 100644 --- a/packages/core/src/fix/verify.ts +++ b/packages/core/src/fix/verify.ts @@ -637,6 +637,12 @@ export interface TestOptions { * Final verification runs the full selection. */ related?: boolean; + /** + * With `related`, for the agent's own `run_tests` only, never a gate or verification: a + * `test` script that ends by running a script that is just the runner + * (`pnpm run build && pnpm run test:unit`) runs that runner's related tests alone. + */ + agentCheck?: boolean; /** Run every test in the selected workspaces, including detected runner configurations. */ fullSuite?: boolean; } @@ -719,6 +725,7 @@ function relatedScript( workspace: string, script: string | undefined, files: string[], + via?: string, ): TestPlan | undefined { const runner = /^\s*(?:(?:npx|pnpm exec|yarn)\s+)?(vitest(?:\s+run)?|jest)\s*$/.exec( script ?? '', @@ -740,7 +747,7 @@ function relatedScript( covers: [workspace], cwd: dir, command: [bin, ...args.map(quote)].join(' '), - scope: `${bin} tests related to ${paths.length} changed file${paths.length === 1 ? '' : 's'} (its \`test\` script is the runner)`, + scope: `${bin} tests related to ${paths.length} changed file${paths.length === 1 ? '' : 's'} (${via ? `its \`test\` script ends with \`${via}\`, the runner; the steps before it did not run` : 'its `test` script is the runner'})`, }; } @@ -783,8 +790,21 @@ export function planTests( const selected = options.related ? 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`. + const last = /(?:^|&&)\s*(?:pnpm|npm|yarn)\s+(?:run\s+)?([\w:.-]+)\s*$/.exec(script ?? '')?.[1]; const scoped = - options.related && files.length ? relatedScript(root, workspace, script, files) : undefined; + options.related && files.length + ? (relatedScript(root, workspace, script, files) ?? + (options.agentCheck && last && last !== 'test' + ? relatedScript( + root, + workspace, + manifest(join(root, workspace))?.scripts?.[last], + files, + last, + ) + : undefined)) + : undefined; if (scoped) { plans.push(scoped); continue;