Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/build-line-states-result.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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) |
Expand Down Expand Up @@ -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 |
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/fix/__snapshots__/report.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down Expand Up @@ -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) |
Expand All @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down
28 changes: 22 additions & 6 deletions packages/core/src/fix/fix.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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)',
Expand Down Expand Up @@ -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',
Expand All @@ -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' },
]);
});

Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/fix/report.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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',
Expand Down
32 changes: 21 additions & 11 deletions packages/core/src/fix/report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
22 changes: 18 additions & 4 deletions packages/core/src/fix/required-build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
});
20 changes: 10 additions & 10 deletions packages/core/src/fix/reverify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -191,15 +190,16 @@ export async function reverify(
after,
newErrors,
tests,
...(buildRequired ? { buildRequired: true, build } : {}),
...(buildRequired ? { buildRequired: true } : {}),
build,
...(lint.length ? { lint } : {}),
...(formatted.length ? { formatted } : {}),
...(unverified ? { typesUnverified: unverified } : {}),
passed:
!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'),
Expand Down
12 changes: 8 additions & 4 deletions packages/core/src/fix/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,8 +639,8 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
}
}
};
const runBuild = async (): Promise<BuildResult[]> => {
if (!needsBuild) return [];
const runBuild = async (always = false): Promise<BuildResult[]> => {
if (!needsBuild && !always) return [];
return keepClean(async () => {
try {
return await verify(`build (${names(workspaces)})`, () =>
Expand Down Expand Up @@ -1208,6 +1208,10 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
);
}
if (feedbackLimits.roundsUsed) lap('feedback', (nanos(llm.costUsd) - nanos(agentSpend)) / 1e9);
// The repository's own build on the final head, reported where the pack does not gate on it:
// the PR names its result, never "not required" in its place (UPT-110).
const headBuild = needsBuild ? [] : await runBuild(true);
if (!needsBuild) lap('build');
assertInstalledVersions(root, [{ name: pack.name, to: target }, ...companions]);
const tests = markPreexisting(ran, baselineTests);
// An agent-led draft must still install from scratch, as the repository's CI would (UPT-102).
Expand Down Expand Up @@ -1265,9 +1269,9 @@ async function fixPackage(options: FixOptions, services: FixServices): Promise<F
newErrors,
baselineTests,
tests,
...(needsBuild || workspaceBuilds.length || consumed.length
...(needsBuild || workspaceBuilds.length || consumed.length || headBuild.length
? {
build: [...workspaceBuilds, ...consumed, ...build],
build: [...workspaceBuilds, ...consumed, ...build, ...headBuild],
...(needsBuild ? { baselineBuild } : {}),
}
: {}),
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/fix/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ export interface FixPhase {
| 'agent'
| 'verify'
| 'feedback'
| 'build'
| 'clean-install';
ms: number;
/** Agent spend in this phase; only the agent and feedback phases call an LLM. */
Expand Down
Loading