From 4ef17f41d7e723ae802808cfdae1e6b296a35ec8 Mon Sep 17 00:00:00 2001 From: Lucas Braum Date: Sun, 18 Oct 2026 10:53:23 -0700 Subject: [PATCH] fix: the PR's build line states the build's result, never "not required" in its place (UPT-110) A pack without requiresBuild never ran the repository's build, and the Verified by row read "build not required (pack-verified migration)" even where `npm run build` fails on the final head (enki, uptide-dev/uptide#169). fix now runs the repository's build once on the final head when the pack does not gate on it, in its own `build` phase, and verify rebuilds on its head instead of keeping a stale stored result. The build line names one of: passed, failed with its first error, or not run and why. "Not required by the pack" sits beside that result. The pack's gate is unchanged: a build it does not require is reported, not a blocker. Closes UPT-110. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Lucas Braum --- .changeset/build-line-states-result.md | 5 +++ .../__snapshots__/report-budget.test.ts.snap | 8 ++--- .../src/fix/__snapshots__/report.test.ts.snap | 8 ++--- packages/core/src/fix/fix.test.ts | 28 ++++++++++++---- packages/core/src/fix/report.test.ts | 6 ++-- packages/core/src/fix/report.ts | 32 ++++++++++++------- packages/core/src/fix/required-build.test.ts | 22 ++++++++++--- packages/core/src/fix/reverify.ts | 20 ++++++------ packages/core/src/fix/run.ts | 12 ++++--- packages/core/src/fix/types.ts | 1 + 10 files changed, 96 insertions(+), 46 deletions(-) create mode 100644 .changeset/build-line-states-result.md diff --git a/.changeset/build-line-states-result.md b/.changeset/build-line-states-result.md new file mode 100644 index 0000000..603e8a8 --- /dev/null +++ b/.changeset/build-line-states-result.md @@ -0,0 +1,5 @@ +--- +"uptide": patch +--- + +The PR description's build check now gives the build's actual result: passed, failed with its first error, or not run and why. When a migration pack doesn't require a build, `fix` still runs the repository's build once on the final head, and `uptide verify` runs it again, and reports what happened. "Not required by the pack" now appears next to that result instead of replacing it. Before, the line read "build not required" even when the repository's build failed. A build the pack doesn't require still doesn't block verification or publishing. diff --git a/packages/core/src/fix/__snapshots__/report-budget.test.ts.snap b/packages/core/src/fix/__snapshots__/report-budget.test.ts.snap index 21287c5..70d52da 100644 --- a/packages/core/src/fix/__snapshots__/report-budget.test.ts.snap +++ b/packages/core/src/fix/__snapshots__/report-budget.test.ts.snap @@ -7,7 +7,7 @@ exports[`groups and caps a large Stripe changelog, linking the omitted range 1`] | | | |---|---| -| **Verified by** | ✅ type-check · build not required (pack-verified migration) · ✅ tests (1 run) | +| **Verified by** | ✅ type-check · ⚠️ build not run · ✅ tests (1 run) | | **Risk** | High: behavior changes | | **Changes** | 4 sites in 1 file · 0 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 4 · Uptide left 0 | @@ -319,7 +319,7 @@ exports[`keeps every evidenced Stripe entry above details and uncapped under bud | | | |---|---| -| **Verified by** | ✅ type-check · build not required (pack-verified migration) · ✅ tests (1 run) | +| **Verified by** | ✅ type-check · ⚠️ build not run · ✅ tests (1 run) | | **Risk** | High: behavior changes | | **Changes** | 4 sites in 1 file · 0 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 4 · Uptide left 0 | @@ -518,7 +518,7 @@ exports[`keeps the under-budget output byte-for-byte unchanged 1`] = ` | | | |---|---| -| **Verified by** | ✅ the zod migration pack · ✅ type-check · build not required (pack-verified migration) · ✅ tests (2 runs) | +| **Verified by** | ✅ the zod migration pack · ✅ type-check · ⚠️ build not run (not required by the pack) · ✅ tests (2 runs) | | **Risk** | Medium: request validation in webhooks | | **Changes** | 29 sites in 8 files · 25 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 28 · Uptide left 0 (1 pre-existing, not counted) | @@ -770,7 +770,7 @@ exports[`trims low-priority material before verification and retains diffs with | | | |---|---| -| **Verified by** | ✅ type-check · build not required (pack-verified migration) · ✅ tests (1 run) | +| **Verified by** | ✅ type-check · ⚠️ build not run · ✅ tests (1 run) | | **Risk** | Medium: agent edits; sensitive paths; no tests | | **Changes** | 500 sites in 500 files · 0 auto-fixed · 500 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 4 · Uptide left 0 | diff --git a/packages/core/src/fix/__snapshots__/report.test.ts.snap b/packages/core/src/fix/__snapshots__/report.test.ts.snap index 5407ced..12e0104 100644 --- a/packages/core/src/fix/__snapshots__/report.test.ts.snap +++ b/packages/core/src/fix/__snapshots__/report.test.ts.snap @@ -9,7 +9,7 @@ exports[`storefront-zod > snapshots the full PR description and compact terminal | | | |---|---| -| **Verified by** | ✅ the zod migration pack · ✅ type-check · build not required (pack-verified migration) · ✅ tests (2 runs) | +| **Verified by** | ✅ the zod migration pack · ✅ type-check · ⚠️ build not run (not required by the pack) · ✅ tests (2 runs) | | **Risk** | Medium: request validation in webhooks | | **Changes** | 29 sites in 8 files · 25 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 28 · Uptide left 0 (1 pre-existing, not counted) | @@ -259,7 +259,7 @@ exports[`storefront-zod > snapshots the full PR description and compact terminal | | | |---|---| -| **Verified by** | ✅ the zod migration pack · ✅ type-check · build not required (pack-verified migration) · ✅ tests (2 runs) | +| **Verified by** | ✅ the zod migration pack · ✅ type-check · ⚠️ build not run (not required by the pack) · ✅ tests (2 runs) | | **Risk** | Medium: request validation in webhooks | | **Changes** | 29 sites in 8 files · 25 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 28 · Uptide left 0 (1 pre-existing, not counted) | @@ -282,7 +282,7 @@ exports[`stripe > snapshots the full PR description and compact terminal/comment | | | |---|---| -| **Verified by** | ✅ type-check · build not required (pack-verified migration) · ⚠️ tests not run: none ran | +| **Verified by** | ✅ type-check · ⚠️ build not run · ⚠️ tests not run: none ran | | **Risk** | High: behavior changes | | **Changes** | 4 sites in 1 file · 0 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 4 · Uptide left 0 | @@ -499,7 +499,7 @@ exports[`stripe > snapshots the full PR description and compact terminal/comment | | | |---|---| -| **Verified by** | ✅ type-check · build not required (pack-verified migration) · ⚠️ tests not run: none ran | +| **Verified by** | ✅ type-check · ⚠️ build not run · ⚠️ tests not run: none ran | | **Risk** | High: behavior changes | | **Changes** | 4 sites in 1 file · 0 auto-fixed · 4 fixed by the agent (LLM) | | **Types** | ✅ the upgrade broke 4 · Uptide left 0 | diff --git a/packages/core/src/fix/fix.test.ts b/packages/core/src/fix/fix.test.ts index 8a376a7..63d1cf1 100644 --- a/packages/core/src/fix/fix.test.ts +++ b/packages/core/src/fix/fix.test.ts @@ -1526,8 +1526,9 @@ it('records baseline builds and rebuilds affected output after the upgrade throu }, ); expect(upgraded).toBe(true); - // Baseline, right after the bump (the agent's declarations), and again on the final head. - expect(build).toHaveBeenCalledTimes(3); + // Baseline, right after the bump (the agent's declarations), again on the final head, and + // the repository's own build there, which the pack does not require (UPT-110). + expect(build).toHaveBeenCalledTimes(4); expect(build).toHaveBeenNthCalledWith( 1, realpathSync(root), @@ -1542,9 +1543,18 @@ it('records baseline builds and rebuilds affected output after the upgrade throu undefined, 'build:ci', ); + expect(build).toHaveBeenNthCalledWith( + 4, + realpathSync(root), + ['.', 'packages/internal'], + undefined, + 'build:ci', + ); expect(result.verification.build).toMatchObject([ { phase: 'baseline', workspace: 'packages/internal', script: 'build:ci', status: 'passed' }, { workspace: 'packages/internal', script: 'build:ci', status: 'passed' }, + { workspace: '.', status: 'missing' }, + { workspace: 'packages/internal', script: 'build:ci', status: 'passed' }, ]); expect(prBody(result)).toContain( 'Build before the baseline in `packages/internal` (`build:ci` script)', @@ -1674,9 +1684,12 @@ describe('consumed workspace builds on the final head (UPT-104)', () => { expect(result.verification.build).toMatchObject([ { phase: 'baseline', workspace: 'packages/internal', status: 'passed' }, { workspace: 'packages/internal', status: 'passed' }, + { workspace: '.', status: 'passed' }, + { workspace: 'packages/internal', status: 'passed' }, ]); - // Baseline, after the bump, and once on the edited head. - expect(result.buildRuns?.count).toBe(3); + // Baseline, after the bump, once on the edited head, then the repository's build there + // (root and `packages/internal`). + expect(result.buildRuns?.count).toBe(5); const body = prBody(result); expect(body).toContain( 'Build after the bump in `packages/internal` (`build:ci` script): `node build.cjs` — passed', @@ -1695,11 +1708,14 @@ describe('consumed workspace builds on the final head (UPT-104)', () => { { ...services, build }, ); expect(result.changedFiles).not.toContain('index.ts'); - expect(build).toHaveBeenCalledTimes(2); - expect(result.buildRuns?.count).toBe(2); + // Preparation and after the bump; then only the repository's own build on the head. + expect(build).toHaveBeenCalledTimes(3); + expect(result.buildRuns?.count).toBe(4); expect(result.verification.build).toMatchObject([ { phase: 'baseline', workspace: 'packages/internal', status: 'passed' }, { workspace: 'packages/internal', status: 'passed' }, + { workspace: '.', status: 'passed' }, + { workspace: 'packages/internal', status: 'passed' }, ]); }); diff --git a/packages/core/src/fix/report.test.ts b/packages/core/src/fix/report.test.ts index 5434e45..8629d81 100644 --- a/packages/core/src/fix/report.test.ts +++ b/packages/core/src/fix/report.test.ts @@ -48,9 +48,9 @@ for (const name of ['storefront-zod', 'stripe']) body = prBody(report), top = visible(body); expect(top.match(/^\| \*\*/gm)).toHaveLength(6); - // A pack analyzed the upgrade: the build is not owed, so it is not a warning. + // A build that did not run says so; the pack's gate sits beside it, never in its place. expect(top).toMatch( - /^\| \*\*Verified by\*\* \|.* · build not required \(pack-verified migration\) · /m, + /^\| \*\*Verified by\*\* \|.* · ⚠️ build not run(?: \(not required by the pack\))? · /m, ); // Decisions and concrete changelog evidence name the code they concern. expect( @@ -1007,7 +1007,7 @@ it('says why a required build failed when it stopped before the framework build' }, ]; expect(visible(prBody(report))).toContain( - '❌ build (build needs DATABASE_URL; set verify.build)', + '❌ build: error: Environment variable not found: DATABASE_URL. (build needs DATABASE_URL; set verify.build)', ); expect(prBody(report)).toContain( 'the build needs DATABASE_URL before the framework build started. Set `verify.build` in uptide.yml', diff --git a/packages/core/src/fix/report.ts b/packages/core/src/fix/report.ts index 36831d7..1471917 100644 --- a/packages/core/src/fix/report.ts +++ b/packages/core/src/fix/report.ts @@ -561,18 +561,28 @@ export function verifiedBy(report: FixReport): string { ? `⚠️ type-check is not proof: ${low}` : '✅ type-check'; const failing = (s: string) => s === 'failed' || s === 'timeout'; - // Name the result of each verification step, including pack-required builds. - const build = !v.build - ? report.tier === 'generic' || v.buildRequired - ? '⚠️ build not run' + // The build's result: passed, failed with its first error, or not run and why. Whether the + // pack gates on it sits beside the result, never in its place (UPT-110). + const gate = + report.tier === 'generic' || v.buildRequired || !pack.length + ? '' : report.packWarning - ? 'build not required by this candidate pack' - : 'build not required (pack-verified migration)' - : !v.build.length || v.build.some((b) => b.status === 'missing') - ? '⚠️ build not run: no build script' - : v.build.some((b) => failing(b.status)) - ? `❌ build${buildCauseText(v.build)}` - : '✅ build'; + ? ' (not required by this candidate pack)' + : ' (not required by the pack)'; + const firstError = v.build + ?.filter((b) => failing(b.status)) + .flatMap((b) => b.output?.split('\n') ?? []) + .find((line) => /\berror\b/i.test(line)) + ?.trim(); + const build = `${ + !v.build + ? '⚠️ build not run' + : !v.build.length || v.build.some((b) => b.status === 'missing') + ? '⚠️ build not run: no build script' + : v.build.some((b) => failing(b.status)) + ? `❌ build${firstError ? `: ${cell(firstError.slice(0, 160))}` : ''}${buildCauseText(v.build)}` + : '✅ build' + }${gate}`; const ran = v.tests.filter((t) => t.status !== 'missing'); const tests = !ran.length ? '⚠️ tests not run: none ran' diff --git a/packages/core/src/fix/required-build.test.ts b/packages/core/src/fix/required-build.test.ts index ad0db37..0355859 100644 --- a/packages/core/src/fix/required-build.test.ts +++ b/packages/core/src/fix/required-build.test.ts @@ -89,13 +89,27 @@ it('reruns the required build when re-verifying and cannot publish a stale pass' expect(publicationBlockers(again).join('; ')).toContain('build required'); }); -it('leaves packs without requiresBuild unchanged', async () => { +it('reports the repo build a pack does not require: a failure reads failed, not "not required" (UPT-110)', async () => { const { root, services } = zodFixture(scratch); - const build = vi.fn(async () => []); + const build = vi.fn(async () => [ + { + workspace: '.', + status: 'failed' as const, + command: 'tsc -b', + output: "> tsc -b\nsrc/db.ts(3,8): error TS2307: Cannot find module 'pg'.\nFound 1 error.", + }, + ]); const result = await fix( { cwd: root, only: 'zod', pack: zodPack, fixer: null }, { ...services, build }, ); - expect(build).not.toHaveBeenCalled(); - expect(result.verification.build).toBeUndefined(); + expect(build).toHaveBeenCalledOnce(); + const body = prBody(result); + expect(body).toContain( + "❌ build: src/db.ts(3,8): error TS2307: Cannot find module 'pg'. (not required by the pack)", + ); + expect(body).not.toContain('build not required'); + // The pack's gate is unchanged: a build it does not require is reported, not a blocker. + expect(result.verification.passed).toBe(true); + expect(publicationBlockers(result).filter((b) => b.startsWith('build'))).toEqual([]); }); diff --git a/packages/core/src/fix/reverify.ts b/packages/core/src/fix/reverify.ts index 025e145..dc6a8e0 100644 --- a/packages/core/src/fix/reverify.ts +++ b/packages/core/src/fix/reverify.ts @@ -135,14 +135,13 @@ export async function reverify( ...(options.testTimeoutMs ? { testTimeoutMs: options.testTimeoutMs } : {}), }); const buildRequired = report.verification.buildRequired || pack.requiresBuild === true; - const build = buildRequired - ? await (services.build ?? buildWorkspaces)( - root, - workspaces, - undefined, - options.buildScript ?? configuredBuildScript(root), - ) - : report.verification.build; + // Rebuilt on this head even when the pack does not require it: a stored result is stale. + const build = await (services.build ?? buildWorkspaces)( + root, + workspaces, + undefined, + options.buildScript ?? configuredBuildScript(root), + ); assertInstalledVersions(root, moves); const left = git(root, 'status', '--porcelain', '--untracked-files=all') .split('\n') @@ -191,7 +190,8 @@ export async function reverify( after, newErrors, tests, - ...(buildRequired ? { buildRequired: true, build } : {}), + ...(buildRequired ? { buildRequired: true } : {}), + build, ...(lint.length ? { lint } : {}), ...(formatted.length ? { formatted } : {}), ...(unverified ? { typesUnverified: unverified } : {}), @@ -199,7 +199,7 @@ export async function reverify( !unverified && newErrors.length === 0 && left.length === 0 && - (!buildRequired || (!!build?.length && build.every((b) => b.status === 'passed'))) && + (!buildRequired || (build.length > 0 && build.every((b) => b.status === 'passed'))) && tests.every((t) => t.status === 'passed' || t.status === 'missing') && testsOrBuildVouch(report.typeCoverage, tests, build) && lint.every((l) => l.status !== 'failed'), diff --git a/packages/core/src/fix/run.ts b/packages/core/src/fix/run.ts index 522f52b..c0b610b 100644 --- a/packages/core/src/fix/run.ts +++ b/packages/core/src/fix/run.ts @@ -639,8 +639,8 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise => { - if (!needsBuild) return []; + const runBuild = async (always = false): Promise => { + if (!needsBuild && !always) return []; return keepClean(async () => { try { return await verify(`build (${names(workspaces)})`, () => @@ -1208,6 +1208,10 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise