diff --git a/.changeset/pr-body-never-too-large.md b/.changeset/pr-body-never-too-large.md new file mode 100644 index 0000000..e5c407c --- /dev/null +++ b/.changeset/pr-body-never-too-large.md @@ -0,0 +1,7 @@ +--- +"uptide": patch +--- + +A migration whose evidence is larger than GitHub's PR description limit no longer stops `fix` with "Required migration evidence exceeds the PR body budget" and loses the run. Long required lists get a compact form when they do not fit. Lockfile churn shows counts per kind of change and the first packages. Manual sites are grouped by rule, with counts and the first locations. Remaining type errors show the first entries. Every full list stays in report.json and, in the Uptide Action, in the job summary, which the description links to. If even the compact form does not fit, the description is a short summary that says so at the top: the verdict, the checks, the counts and where the full report is. The branch and report.json are always kept. + +An agent's decisions under "Decisions for you" name the files the agent changed, five at most, and count the rest, instead of every file in the task's scope. A verification-feedback task scoped to the whole repository made each of its decisions a list of hundreds of files. diff --git a/packages/core/src/fix/budget.test.ts b/packages/core/src/fix/budget.test.ts index 9c240d2..1577881 100644 --- a/packages/core/src/fix/budget.test.ts +++ b/packages/core/src/fix/budget.test.ts @@ -27,7 +27,7 @@ it('keeps rank 0, then lower ranks first, and never cuts inside a block', () => const out = fitPieces( [...pieces, { kind: 'text', text: 'low priority '.repeat(100), rank: 9 }], 172, - ); + ) as string; expect(out.length).toBeLessThanOrEqual(172); expect(out).toContain('# Title'); expect(out).toContain('long diff'); @@ -52,12 +52,10 @@ it('falls back to one line per piece when almost nothing fits', () => { }); it('keeps a complete document unchanged when it fits, even if omission notices are longer', () => { - const body = fitPieces(pieces); + const body = fitPieces(pieces) as string; expect(fitPieces(pieces, body.length)).toBe(body); }); -it('refuses oversized required evidence instead of silently dropping it', () => { - expect(() => fitPieces([must('required'.repeat(20))], 100)).toThrow( - 'Required migration evidence', - ); +it('returns nothing for oversized required evidence instead of silently dropping it', () => { + expect(fitPieces([must('required'.repeat(20))], 100)).toBeUndefined(); }); diff --git a/packages/core/src/fix/budget.ts b/packages/core/src/fix/budget.ts index d90dc3c..3dd3e3b 100644 --- a/packages/core/src/fix/budget.ts +++ b/packages/core/src/fix/budget.ts @@ -3,7 +3,8 @@ * GitHub refuses a PR body over 65,536 characters. A piece is dropped whole (replaced by its * `alt`, usually one line saying where the content is), a list loses items from its end and * says how many; nothing is ever cut inside a block, so no `
` or code fence is left - * open. Lower ranks are kept first; rank 0 is never dropped. + * open. Lower ranks are kept first; rank 0 is never dropped. When rank 0 alone does not fit, + * there is no document: the caller renders a shorter one (`undefined`). */ export type Piece = | { kind: 'text'; text: string; rank: number; alt?: string } @@ -20,7 +21,7 @@ export type Piece = export const must = (text: string): Piece => ({ kind: 'text', text, rank: 0 }); -export function fitPieces(pieces: Piece[], budget?: number): string { +export function fitPieces(pieces: Piece[], budget?: number): string | undefined { const complete = pieces.map((p) => full(p)).join('\n'); if (budget === undefined || complete.length <= budget) return complete; // What each piece contributes at minimum: a text's alt, a list's "all hidden" line. @@ -34,12 +35,8 @@ export function fitPieces(pieces: Piece[], budget?: number): string { : ''; const chosen = pieces.map(floor); let size = chosen.reduce((n, text) => n + text.length, 0) + Math.max(0, chosen.length - 1); - // Arbitrarily large required evidence cannot fit a finite field. Refuse instead of - // silently deleting decisions/diffs or handing GitHub an oversized description. - if (size > budget) - throw new Error( - 'Required migration evidence exceeds the PR body budget; review the full report.json and narrow the migration before publishing.', - ); + // Arbitrarily large required evidence cannot fit a finite field: nothing is silently cut. + if (size > budget) return undefined; const order = pieces .map((p, i) => ({ p, i })) .filter(({ p }) => p.rank > 0) diff --git a/packages/core/src/fix/isolate.ts b/packages/core/src/fix/isolate.ts index c1d8a92..2424862 100644 --- a/packages/core/src/fix/isolate.ts +++ b/packages/core/src/fix/isolate.ts @@ -336,8 +336,8 @@ function finish( const dir = storedRunDir(source, result.branch); mkdirSync(dir, { recursive: true }); result.prBody = join(dir, 'pr-body.md'); - writeFileSync(result.prBody, prBody(result)); writeFileSync(join(dir, 'report.json'), JSON.stringify(result, null, 2)); + writeFileSync(result.prBody, prBody(result)); return result; } diff --git a/packages/core/src/fix/pr-body.test.ts b/packages/core/src/fix/pr-body.test.ts index f8666a3..03f7f41 100644 --- a/packages/core/src/fix/pr-body.test.ts +++ b/packages/core/src/fix/pr-body.test.ts @@ -87,15 +87,14 @@ it('applies the same budget to a stored many-site preview and the submitted body expect(readFileSync(m.run, 'utf8')).toBe(JSON.stringify(m.report)); }); -it('refuses oversized required evidence before editing a PR and leaves the stored run intact', async () => { +it('sends a shortened description for oversized required evidence and leaves the stored run intact', async () => { const m = mock(); m.report.decisions = ['- Owner decision. '.repeat(PR_BODY_BUDGET)]; writeFileSync(m.run, JSON.stringify(m.report)); - await expect(updatePrBody({ cwd: temp, pr: '82', run: m.run }, m.io)).rejects.toThrow( - 'Required migration evidence', - ); - expect(m.sent()).toBe(''); - expect(m.calls.flat()).not.toContain('edit'); + const updated = await updatePrBody({ cwd: temp, pr: '82', run: m.run }, m.io); + expect(updated.body.length).toBeLessThanOrEqual(PR_BODY_BUDGET); + expect(updated.body).toContain('**Shortened description.**'); + expect(m.sent()).toBe(updated.body); expect(readFileSync(m.run, 'utf8')).toBe(JSON.stringify(m.report)); }); it('falls back to the REST endpoint when gh pr edit trips over retired classic projects', async () => { diff --git a/packages/core/src/fix/report-budget.test.ts b/packages/core/src/fix/report-budget.test.ts index 69e737b..01f96e1 100644 --- a/packages/core/src/fix/report-budget.test.ts +++ b/packages/core/src/fix/report-budget.test.ts @@ -196,3 +196,114 @@ it('keeps the Action report and PR body portable without an npm invocation', asy vi.unstubAllEnvs(); } }); + +/** ~300 manual sites and ~100 churned lockfile entries: required lists far past the limit (UPT-106). */ +function overflowingReport(): FixReport { + const report = manySitesReport(); + report.sites = report.sites.slice(0, 300).map((s, i) => ({ + ...s, + outcome: 'manual' as const, + rule: `rule-${i % 3}`, + reason: `Needs a person: ${'the call site passes options the rule cannot rewrite safely; '.repeat(3)}${i}`, + })); + const churn = (prefix: string) => + Array.from({ length: 100 }, (_, i) => ({ + name: `${prefix}-${i}`, + ...(i % 4 === 0 ? {} : { from: '1.0.0' }), + ...(i % 4 === 1 ? {} : { to: i % 4 === 2 ? '1.0.0' : '1.1.0' }), + })); + report.lockfile = { + manager: 'npm', + file: 'package-lock.json', + added: [], + removed: [], + changed: [], + allowed: [], + outOfScope: churn('outside'), + parentChurn: churn('parent'), + }; + return report; +} + +it('renders 300 manual sites and 100 churned lockfile entries under the limit, every section represented', () => { + const report = overflowingReport(); + report.runUrl = 'https://github.com/acme/app/actions/runs/42'; + expect(migrationBody(report).length).toBeGreaterThan(PR_BODY_BUDGET); + const body = prBody(report); + expect(body.length).toBeLessThanOrEqual(PR_BODY_BUDGET); + for (const section of [ + '### Lockfile changes outside the upgrade: review', + '### Lockfile churn the parent commit already has', + '| **Risk** |', + '### What changed', + '### Worth a look', + '### Decisions for you', + 'Verification details', + 'Run details', + ]) + expect(body).toContain(section); + // Manual sites: by rule, with counts and the first locations. + expect(body).toContain('- Manual: 300 sites to change by hand'); + expect(body).toContain('- `rule-0` (100): `src/billing-0.ts:1`, `src/billing-3.ts:4`'); + expect(body).toContain('and 95 more'); + expect(body).toContain('[job summary](https://github.com/acme/app/actions/runs/42)'); + expect(body).toContain('- parent-99 1.0.0 → 1.1.0'); + balanced(body); + // Tighter still, lockfile churn keeps its counts per kind of change and the first packages. + const tight = renderMigration(report, 'full', 8_000); + expect(tight.length).toBeLessThanOrEqual(8_000); + expect(tight).toContain('### Lockfile churn the parent commit already has'); + expect(tight).toContain( + '100 entries: 25 added, 25 removed, 25 re-resolved at the same version, 25 version changed', + ); + expect(tight).toContain('- parent-9 '); + expect(tight).not.toContain('- parent-10 '); + expect(tight).toContain('- Manual: 300 sites to change by hand'); + balanced(tight); +}); + +it('falls back to a minimal body when even the compact sections do not fit', () => { + const report = overflowingReport(); + report.decisions = ['- Owner decision. '.repeat(PR_BODY_BUDGET / 4)]; + const body = prBody(report); + expect(body.length).toBeLessThanOrEqual(PR_BODY_BUDGET); + expect(body).toMatch(/^## Upgrade example-sdk [^\n]+\n\n> ⚠️ \*\*Shortened description\.\*\*/); + expect(body).toContain('Verification: PASS.'); + expect(body).toContain('300 manual'); + expect(body).toContain('report.json'); + expect(body).not.toContain('Owner decision'); +}); + +it('names the files an agent decision changed, not the task scope (UPT-106)', () => { + const report = manySitesReport(); + const scope = Array.from({ length: 317 }, (_, i) => `src/module-${i}.ts`); + report.llm.tasks = [ + { + id: 'verification-1', + rule: 'verification', + files: scope, + sites: 0, + fixed: 0, + turns: 3, + inputTokens: 0, + outputTokens: 0, + costUsd: 0, + durationMs: 0, + toolCalls: {}, + gate: 'accepted', + diffs: { 'src/module-7.ts': '+x' }, + summary: { + changed: [], + decisions: ['Kept the ref nullable'], + assumptions: [], + couldNotDo: [], + newFiles: [], + }, + }, + ]; + expect(prBody(report)).toContain( + '- `src/module-7.ts`: Kept the ref nullable. Confirm or change.', + ); + delete (report.llm.tasks[0] as { diffs?: unknown }).diffs; + expect(prBody(report)).toContain('`src/module-4.ts` and 312 more files: Kept the ref nullable.'); +}); diff --git a/packages/core/src/fix/report.ts b/packages/core/src/fix/report.ts index c5233c2..36831d7 100644 --- a/packages/core/src/fix/report.ts +++ b/packages/core/src/fix/report.ts @@ -8,7 +8,11 @@ import { UPTIDE_COMMAND } from '../version.js'; import { baselineHeadline, baselineHealthOf } from './baseline.js'; import type { BehaviorResult } from './behavior.js'; import { fitPieces, must, type Piece } from './budget.js'; -import { describeOutOfScope, describeTransitive } from './managers/lock-guard.js'; +import { + describeOutOfScope, + describeTransitive, + type OutOfScopeChange, +} from './managers/lock-guard.js'; import { describeEresolveRetry } from './managers/upgrade.js'; import { noPackBlockers } from './no-pack.js'; import { uniqueSites } from './paired.js'; @@ -695,6 +699,123 @@ const droppable = (rank: number, block: string[], what: string): Piece => ({ rank, alt: `_${what} omitted to fit the PR body budget. Full text is retained in report.json; see the job summary for verification results when run in Actions._`, }); +/** How many entries a list cut down to its compact form still names. */ +const FIRST = 10; +/** Where every list is in full: report.json, and the job summary when the run was in Actions. */ +const fullLists = (report: FixReport): string => + `report.json${report.runUrl ? ` and the [job summary](${report.runUrl})` : ''}`; +/** + * A list no budget may drop: whole when it fits, else its compact form, which still counts + * every entry and names the first ones. It gives way before anything droppable. + */ +const shortened = (text: string[], compact: string[]): Piece => ({ + kind: 'text', + rank: 1, + text: text.join('\n'), + alt: compact.join('\n'), +}); +/** Lockfile entries: whole, or counts per kind of change and the first packages. */ +function lockfileChanges(heading: string[], changes: OutOfScopeChange[], report: FixReport): Piece { + const kinds = new Map(); + for (const c of changes) { + const kind = !c.from + ? 'added' + : !c.to + ? 'removed' + : c.from === c.to + ? 're-resolved at the same version' + : 'version changed'; + kinds.set(kind, (kinds.get(kind) ?? 0) + 1); + } + const line = (c: OutOfScopeChange) => `- ${describeOutOfScope(c)}`; + return shortened( + [...heading, ...changes.map(line)], + [ + ...heading, + `${changes.length} ${changes.length === 1 ? 'entry' : 'entries'}: ${[...kinds].map(([kind, n]) => `${n} ${kind}`).join(', ')}. The first ${FIRST}; all of them are in ${fullLists(report)}.`, + '', + ...changes.slice(0, FIRST).map(line), + ], + ); +} +/** Sites to change by hand: one line each, or per rule a count and the first locations. */ +function manualSites(sites: FixSite[], report: FixReport): Piece { + const byRule = new Map(); + for (const s of sites) + byRule.set(s.rule ?? 'other', [...(byRule.get(s.rule ?? 'other') ?? []), s]); + const shown = 5; + return shortened( + sites.map( + (s) => + `- Manual: \`${basename(s.finding.usage.file)}:${s.finding.usage.line}\` — ${sentence(s.reason)}`, + ), + [ + `- Manual: ${count(sites.length, 'site')} to change by hand, by rule; each one with its reason is in ${fullLists(report)}.`, + ...[...byRule].map( + ([rule, list]) => + ` - \`${rule}\` (${list.length}): ${list + .slice(0, shown) + .map((s) => `\`${s.finding.usage.file}:${s.finding.usage.line}\``) + .join(', ')}${list.length > shown ? `, and ${list.length - shown} more` : ''}`, + ), + ], + ); +} +/** + * The description when even the compact sections do not fit: what was checked and how many of + * everything, and where the rest is. Its size does not grow with the run. + */ +function minimalBody(report: FixReport, title: string): string { + const v = report.verification; + const sites = (outcome: FixSite['outcome']) => + report.sites.filter((s) => s.outcome === outcome).length; + const tests = (status: string) => v.tests.filter((t) => t.status === status).length; + const lock = report.lockfile; + return [ + title, + '', + `> ⚠️ **Shortened description.** This run's evidence does not fit GitHub's size limit even in compact form, so this is its summary. Every section is in ${fullLists(report)}.`, + '', + ...(report.agentFallback?.length + ? [ + '> ⚠️ **Agent-led draft.** The mechanical path stopped; the reasons are in report.json.', + '', + ] + : []), + `**Review required before merging.** Verification: ${v.passed ? 'PASS' : 'FAIL'}.`, + '', + '### Checks', + '', + `- Type errors: baseline ${v.baseline.length}; after ${v.after.length}; ${v.newErrors.length} new.`, + `- Tests: ${v.tests.length ? `${tests('passed')} passed, ${tests('failed') + tests('timeout')} failed or timed out, ${tests('missing')} missing` : 'none ran'}.`, + ...(v.build?.length + ? [ + `- Builds: ${v.build.filter((b) => b.status === 'passed').length} of ${v.build.length} passed.`, + ] + : []), + ...(v.lint?.length + ? [ + `- Lint: ${v.lint.filter((l) => l.status === 'failed').length} of ${v.lint.length} failed.`, + ] + : []), + ...(report.cleanInstall + ? [`- Clean install: ${report.cleanInstall.passed ? 'passed' : 'FAILED'}.`] + : []), + '', + '### Counts', + '', + `- Sites: ${sites('mechanical')} by rule, ${sites('agent')} by the agent, ${sites('manual')} manual.`, + ...(lock + ? [ + `- Lockfile: ${lock.outOfScope?.length ?? 0} outside the upgrade, ${lock.parentChurn?.length ?? 0} the parent commit already has, ${lock.transitive?.reduce((n, m) => n + m.changes.length, 0) ?? 0} transitive.`, + ] + : []), + `- Changed files: ${report.changedFiles?.length ?? 0}. Decisions for you: ${(report.decisions?.length ?? 0) + (report.headline?.decision?.length ?? 0)}. Run notes: ${report.notes.length}.`, + '', + `Branch: \`${report.branch}\`${report.head ? ` · Verified commit: \`${report.head}\`` : ''}`, + '', + ].join('\n'); +} /** Readers of a shared report may not have Uptide installed. */ const withCommand = (text: string): string => text.replace(/\b(?:npx )?uptide(?:@[\w.-]+)?(?= [a-z-])/g, UPTIDE_COMMAND); @@ -708,10 +829,11 @@ export function renderMigration( const from = report.from ?? ([...new Set(report.sites.map((s) => s.finding.change.from))].join(', ') || 'unknown'); - const lines: (string | Piece)[] = + const title = report.mode === 'pin' - ? [`## Pin the Stripe API version to ${report.apiVersion}`, ''] - : [`## Upgrade ${report.package} ${from} → ${report.target}`, '']; + ? `## Pin the Stripe API version to ${report.apiVersion}` + : `## Upgrade ${report.package} ${from} → ${report.target}`; + const lines: (string | Piece)[] = [title, '']; // Why the mechanical path stopped and the agent led: the first thing a reviewer reads. if (report.agentFallback?.length) lines.push( @@ -725,24 +847,25 @@ export function renderMigration( ); if (report.lockfile?.outOfScope?.length) lines.push( - must( - [ - '### Lockfile changes outside the upgrade: review', - '', - ...report.lockfile.outOfScope.map((c) => `- ${describeOutOfScope(c)}`), - ].join('\n'), + lockfileChanges( + ['### Lockfile changes outside the upgrade: review', ''], + report.lockfile.outOfScope, + report, ), '', ); if (report.lockfile?.parentChurn?.length) lines.push( - [ - '### Lockfile churn the parent commit already has', - '', - 'The same install changes these at the parent commit too, so they are not from the upgrade:', - '', - ...report.lockfile.parentChurn.map((c) => `- ${describeOutOfScope(c)}`), - ].join('\n'), + lockfileChanges( + [ + '### Lockfile churn the parent commit already has', + '', + 'The same install changes these at the parent commit too, so they are not from the upgrade:', + '', + ], + report.lockfile.parentChurn, + report, + ), '', ); if (report.rootPins?.length) @@ -1079,7 +1202,7 @@ export function renderMigration( .filter((l): l is string => typeof l === 'string') .join('\n') .trimEnd()}\n`; - const review: string[] = []; + const review: (string | Piece)[] = []; const notChecked = schemas(report).filter(unchecked); if (notChecked.length) review.push( @@ -1135,10 +1258,8 @@ export function renderMigration( review.push( `- The agent ${costStopMessage(report.llm.costLimit, report.llm.estimate).replace(/--max-cost [\d.]+|maxCost(?= in)/, '`$&`')}`, ); - for (const s of report.sites.filter((s) => s.outcome === 'manual')) - review.push( - `- Manual: \`${basename(s.finding.usage.file)}:${s.finding.usage.line}\` — ${sentence(s.reason)}`, - ); + const manual = report.sites.filter((s) => s.outcome === 'manual'); + if (manual.length) review.push(manualSites(manual, report)); if (report.verification.newErrors.length) review.push( `- Resolve ${report.verification.newErrors.length} new type errors before merging (see verification details).`, @@ -1206,7 +1327,7 @@ export function renderMigration( '', ); // Normalize retained commands before measuring the body. - return fitPieces( + const body = fitPieces( lines.map((part): Piece => { const piece = typeof part === 'string' ? must(part) : part; return piece.kind === 'text' @@ -1225,6 +1346,7 @@ export function renderMigration( }), budget, ); + return body ?? minimalBody(report, title); } /** ` (needs DATABASE_URL; set verify.build)` when every failed build stopped before the framework's build. */ function buildCauseText(build: NonNullable): string { @@ -1324,8 +1446,18 @@ function verificationDetails(report: FixReport, bounded: boolean): (string | Pie ), ); } - // What the migration left broken is never dropped. - if (v.newErrors.length) lines.push('', '**Remaining errors**', ...v.newErrors.map(listing)); + // What the migration left broken is never dropped: at most cut to its first entries. + if (v.newErrors.length) + lines.push( + '', + '**Remaining errors**', + shortened(v.newErrors.map(listing), [ + ...v.newErrors.slice(0, FIRST).map(listing), + ...(v.newErrors.length > FIRST + ? [`- … and ${v.newErrors.length - FIRST} more, all in ${fullLists(report)}.`] + : []), + ]), + ); if (v.workspaceTypes?.length) lines.push( '', diff --git a/packages/core/src/fix/review-evidence.ts b/packages/core/src/fix/review-evidence.ts index 314c219..150c332 100644 --- a/packages/core/src/fix/review-evidence.ts +++ b/packages/core/src/fix/review-evidence.ts @@ -1,5 +1,7 @@ import type { FixReport } from './types.js'; +/** How many files a decision line names before it counts the rest. */ +const NAMED = 5; /** * Accepted agent choices belong beside the pack's owner decisions, not buried in logs. One * bullet per rule and resource (UPT-70): a choice the pack already puts to the owner for that @@ -57,23 +59,30 @@ export function agentDecisionLines(report: FixReport): string[] { ...new Set(sites.map((site) => site.finding.change.path.split('#')[0])), ].sort(); const key = rules.length > 0 ? `${rules.join(',')}|${resources.join(',')}` : `#${index}`; + // The files the task changed: its scope can be the whole repository (verification feedback). + const changed = Object.keys(task.diffs ?? {}); + const files = changed.length ? changed : task.files; const first = said.get(key); if (first) { - first.files.push(...task.files.filter((file) => !first.files.includes(file))); + first.files.push(...files.filter((file) => !first.files.includes(file))); return []; } - const entry = { files: [...task.files], decisions }; + const entry = { files: [...files], decisions }; said.set(key, entry); return [entry]; }); - return lines.flatMap((line) => - typeof line === 'string' - ? [line] - : line.decisions.map( - (decision) => - `- ${line.files.map((file) => `\`${file}\``).join(', ')}: ${decision.trim()}${/[.!?]$/.test(decision.trim()) ? '' : '.'} Confirm or change.`, - ), - ); + return lines.flatMap((line) => { + if (typeof line === 'string') return [line]; + const more = line.files.length - NAMED; + const files = `${line.files + .slice(0, NAMED) + .map((file) => `\`${file}\``) + .join(', ')}${more > 0 ? ` and ${more} more files` : ''}`; + return line.decisions.map( + (decision) => + `- ${files}: ${decision.trim()}${/[.!?]$/.test(decision.trim()) ? '' : '.'} Confirm or change.`, + ); + }); } /** First failing suite's error, bounded, plus counts; repeated noise is printed once. */ diff --git a/packages/core/src/fix/run.ts b/packages/core/src/fix/run.ts index 0fd12a2..78d5407 100644 --- a/packages/core/src/fix/run.ts +++ b/packages/core/src/fix/run.ts @@ -6,7 +6,7 @@ import { compareVersions } from '../check/version.js'; import type { PackageFetcher } from '../domain/io.js'; import { type ProgressListener, progress } from '../domain/progress.js'; import type { CheckReport, Finding } from '../domain/report.js'; -import { ReportedError, SafetyRefusal, UptideError } from '../errors.js'; +import { SafetyRefusal, UptideError } from '../errors.js'; import { createNpmFetcher } from '../fetch/npm-fetcher.js'; import { loadRegistryConfig } from '../fetch/npmrc.js'; import { ACCEPTED_KEYS, selectLlm } from '../llm/config.js'; @@ -1214,19 +1214,12 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise `${s.file}:${s.line}`))]; lines.push('', `**\`${u.package}\`** (${places.length})`, ''); - for (const place of places.slice(0, 50)) lines.push(`- \`${cell(place)}\``); - if (places.length > 50) lines.push(`- and ${places.length - 50} more`); + // Every one: the PR description may name only the first few and point here. + for (const place of places) lines.push(`- \`${cell(place)}\``); } } }