Rebuild up as a Deploy Execution adapter - #3193
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
veryfront up predates full Deploy Execution adoption: it ran pushCommand and
then deployCommand, a two-command dance around what DeployProject.execute
already owns end to end (bootstrap push, release, deployment, verification,
readiness). up now makes one execute call with source { kind: "ensure-pushed" },
branch main, environment preview, and the mode its --dry-run implies, so the
module decides whether a bootstrap push is needed instead of up pushing first
and telling deploy to skip it.
The URL up printed was not the URL it deployed. deployCommand handed back a
DeployResult whose url had been probed until it answered, up discarded it, and
printed buildPushUrls(projectSlug, "main") instead: a hostname rebuilt locally
as {slug}.preview.veryfront.com. The two agree only when the preview
environment has no configured domain and the app has no static page route.
When the environment carries a domain the control plane returns that domain,
and when the app has a non-root page the verified URL carries the route that
was probed (for example .../dashboard), so the old output could send a user to
a hostname nobody had checked. Preview is now the verified outcome URL in both
human and JSON output. The Studio link is a constructed canonical link, never
probed, so it is still built with buildStudioUrl - but from the deployed slug
and branch rather than the locally resolved ones. JSON field names
(projectSlug, studioUrl, previewUrl, nextCommand) are unchanged.
Dry-run planned actions are now derived from the module's plan instead of
being asserted by up: create-project and push-source are reported when the plan
contains them, and the module's create-release plus deploy collapse into the
single deploy-preview step up has always reported. For the dry run that had no
receipt this is the same JSON as before. Two edge cases change on purpose: with
a valid push receipt already on disk the plan omits push-source, because the
deploy would not push; and with a receipt for another branch the dry run now
fails on the receipt mismatch instead of silently planning a push, since up
dry-run previously returned before deploy ever looked at the receipt.
Progress rendering moves to cli/shared/deployment/progress.ts so up and the
deploy command speak the same words for the same step; the deploy command's
progressForEvent is that module's deployProgressText, moved verbatim. up feeds
a spinner in human mode and stays silent in JSON mode, which keeps its "one
JSON result" contract. Routing-convergence warnings remain unreported by up, as
they were when it called deployCommand with quiet: true.
The up unit tests no longer monkey-patch globalThis.fetch six times to fake an
entire deploy. They inject a fake DeployProject and assert what up asked deploy
to do - environment preview, branch main, source ensure-pushed, mode from
--dry-run - which the old tests could not observe at all, plus URL provenance:
the printed preview URL is the outcome URL and no output line contains the
rebuilt preview.veryfront.com pattern. Project creation, the pulled local link,
the auth guard, the empty-folder guard, and the single-JSON-result contract
keep their tests; the remaining fetch stubs cover only the auth check and
project creation, through the shared withMockFetch helper.
Because that fake stops at the module boundary, up.integration.test.ts now
drives the whole command against the real module, with
createDeployProject({ polling, controlPlaneFactory }) over
InMemoryDeployControlPlane: project creation, the bootstrap push, and the
readiness probe go over a fetch stub, releases and deployments through the
in-memory control plane, and the release mirrors exactly what the push
uploaded so release-source verification sees the digest the receipt recorded.
It pins the provenance a fake cannot: the preview URL up prints and emits is
the environment domain the control plane returned and the deploy probed
(preview.example.test), where the old code would have printed
my-project.preview.veryfront.com. The dry run asserts the other direction -
no project created, nothing uploaded, no release, no deployment, not even a
project lookup - plus the planned-actions shape. A fourth case pins the
behaviour change above: seeded with a receipt for feature-x, a dry run
targeting main rejects with the branch-mismatch message from
validatePushReceipt and creates no release or deployment.
Constraint: vf deploy CLI behavior unchanged; up's human output is unchanged
except that Preview is now the verified deployment URL, and its spinner reports
deploy progress where it previously showed nothing.
Tested: deno test --no-check --allow-all cli/commands/up/ cli/commands/deploy/ cli/commands/push/ cli/shared/deployment/ cli/mcp/tools/deploy-tool.test.ts (52 tests, 250 steps)
Tested: deno task typecheck, deno lint cli/, deno task lint:cli-boundary, deno task lint:module-boundaries, deno task fmt:check (all pass)
Tested: deno check --no-lock on all four touched test files (clean)
Not-tested: a real deploy against the control plane.
Two adapters call the same Deploy Execution module in two different styles.
The MCP deploy tool injects a DeployProject through its options and production
falls back to createDeployProject(). The deploy command instead exported a
second entry point, deployCommandWithProjectForTesting, plus a
DeployCommandTestingSeams interface and a createDeployProject factory hook -
production surface that existed only so command.test.ts could substitute a
fake. The command now takes the same optional deployProject its MCP sibling
does, and the two exports are gone.
The polling knobs go with them. assetManifestPollIntervalMs,
assetManifestTimeoutMs, environmentPollIntervalMs, and environmentTimeoutMs sat
on DeployOptions, and parseDeployArgs maps no flag to any of them, so no user
could ever set one: they were a way for tests to reach the module's polling
policy through the command. Tests now build that policy where it belongs, by
injecting createDeployProject({ polling }) - the integration suites still drive
the real module over their fetch stubs, they only stop routing its
configuration through the command. suppressJsonOutput goes too; up was its only
caller and up no longer composes deploy.
skipSourcePush stays, though it is equally unparseable and equally without a
production caller now that up is a Deploy Execution adapter. It is the only
expression of source { kind: "already-pushed" } at the command boundary, and
eight integration assertions depend on it for real behaviour - that a deploy
with an already-pushed source issues no PUT uploads, and that its dry run does
not claim it would push. Removing it would delete those guarantees rather than
relocate them, so it keeps its place until the command grows a user-facing way
to say the same thing.
Constraint: vf deploy CLI behavior unchanged; no deploy or push assertion
weakened, only rewired.
Tested: deno test --no-check --allow-all cli/commands/up/ cli/commands/deploy/ cli/commands/push/ cli/shared/deployment/ cli/mcp/tools/deploy-tool.test.ts (51 tests, 246 steps)
Tested: deno task typecheck, deno lint cli/, deno task lint:cli-boundary, deno task lint:module-boundaries, deno task fmt:check (all pass)
66c70d4 to
804021a
Compare
The up end-to-end suite was declared with { sanitizeOps: false,
sanitizeResources: false }, which pushed the repo ratchet from 408 to 410 and
red-lit lint:sanitizer-baseline. Nothing was leaking. The options were copied
from the "Up Command Integration" describe directly above it, on the assumption
that a suite spawning git processes, temp directories, and a readiness-probe
fetch would need them. It does not: with both sanitizers restored the four
cases pass, and five consecutive runs report no leaked ops and no leaked
resources. Deploy Execution's polling and the readiness probe complete inside
execute(), withFetchStub restores globalThis.fetch in a finally, and runUp
removes its temp directory on every path including the rejection case.
The neighbouring describe turned out not to need them either, so both pairs
are gone and the baseline drops to 406, which is what the check asks for when
opt-outs disappear. That suite only reads and writes temp files.
An opt-out that suppresses nothing is worse than none: it silently disarms the
detector for every future test added to the block, which is how a suite that
never leaked ends up hiding one.
Constraint: no test behaviour changed; only the sanitizer options and the
baseline constant.
Tested: deno run --allow-read scripts/lint/check-sanitizer-baseline.ts (406/406)
Tested: deno test --no-check --allow-all cli/commands/up/up.integration.test.ts x5 (clean, no sanitizer findings)
Tested: deno test --no-check --allow-all cli/commands/up/ cli/commands/deploy/ cli/commands/push/ cli/shared/deployment/ cli/mcp/tools/deploy-tool.test.ts (52 tests, 250 steps)
Tested: check-test-typecheck-baseline.ts (92 grandfathered, 0 new), deno task typecheck, deno task lint, deno task fmt:check (all pass)
There was a problem hiding this comment.
Pull request overview
This PR refactors veryfront up to delegate end-to-end work to the Deploy Execution module (DeployProject.execute) so up prints/returns the verified preview URL produced by Deploy Execution, and it simplifies the deploy command adapter by removing bespoke testing seams in favor of dependency injection.
Changes:
- Rebuild
upto call Deploy Execution directly (apply/dry-run), stream consistent progress text, and output the verifiedpreviewUrl(human + JSON). - Extract shared deploy progress messaging into
cli/shared/deployment/progress.tsand reuse it from bothdeployandup. - Update integration/unit tests to inject a fake
DeployProject(unit) or use an in-memory control plane (E2E-style integration), and re-enable sanitizers (baseline lowered accordingly).
Verification
- Not run in this review environment. Safest next step: run the repo’s targeted CLI tests, then broaden if needed:
deno test --no-check --allow-all --unstable-worker-options --unstable-net cli/commands/up/command.test.ts cli/commands/up/up.integration.test.ts cli/commands/deploy/command.test.ts cli/commands/deploy/command.integration.test.ts- Then:
VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text deno test --no-check --allow-all --unstable-worker-options --unstable-net
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/lint/check-sanitizer-baseline.ts | Lowers sanitizer opt-out baseline after re-enabling sanitizers in the up integration suite. |
| cli/shared/deployment/progress.ts | Introduces shared human progress text mapping for Deploy Execution step events. |
| cli/commands/up/up.integration.test.ts | Adds end-to-end coverage that asserts up prints/returns the control-plane-verified preview URL and avoids the rebuilt hostname pattern. |
| cli/commands/up/command.ts | Replaces push+deploy composition with a single Deploy Execution call; prints/streams verified URLs; introduces dependency injection for tests. |
| cli/commands/up/command.test.ts | Refactors unit tests to inject a fake DeployProject and assert requested deploy behavior + verified URL handling. |
| cli/commands/deploy/command.ts | Removes bespoke testing seams/options; injects DeployProject directly; reuses shared progress text. |
| cli/commands/deploy/command.test.ts | Updates adapter tests to use deployCommand with injected deployProject. |
| cli/commands/deploy/command.integration.test.ts | Uses a bounded-polling Deploy Execution instance for integration stability without adding adapter-only polling knobs. |
Suppressed comments (4)
cli/commands/up/command.test.ts:290
- This
withMockFetchhandler returns an identity response for any request. Tighten it to only allow/me(and throw otherwise) so the test reliably catches unexpected fetch orchestration outside Deploy Execution.
const { output } = await captureLog(() =>
withMockFetch(
() => Promise.resolve(identityResponse()),
() => upCommand({ projectDir }, authenticatedEnv(projectDir), { deployProject }),
)
);
cli/commands/up/command.test.ts:329
- This fetch stub responds with
identityResponse()for any request, which can hide unexpected additional fetches during JSON mode. Restrict it to/meand throw on other paths to keep the test fail-closed.
const { output } = await captureLog(() =>
withMockFetch(
() => Promise.resolve(identityResponse()),
() => upCommand({ projectDir }, authenticatedEnv(projectDir), { deployProject }),
)
);
cli/commands/up/command.test.ts:366
- This fetch stub is permissive (returns an identity response for any request). Consider throwing on non-
/mepaths so the dry-run adapter test fails ifupCommandstarts making extra network requests.
const { output } = await captureLog(() =>
withMockFetch(
() => Promise.resolve(identityResponse()),
() =>
upCommand({ projectDir, dryRun: true }, authenticatedEnv(projectDir), {
deployProject,
}),
)
);
cli/commands/up/command.test.ts:409
- This fetch stub accepts any request and always returns an identity response. Restricting it to
/mehelps ensure the test fails ifupCommandunexpectedly performs other fetches during planning.
const { output } = await captureLog(() =>
withMockFetch(
() => Promise.resolve(identityResponse()),
() =>
upCommand({ projectDir, dryRun: true }, authenticatedEnv(projectDir), {
deployProject,
}),
)
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Six cases in the up unit tests stubbed fetch with a function that answered every request with the same identity payload, whatever the method or path. With Deploy Execution injected, up should reach the network for nothing but the auth check, so that stub asserted nothing: had up started calling the control plane again, the tests would have handed it a user object and stayed green. They now share authCheckOnlyFetch, which answers GET /me and throws "Unexpected request: <method> <path>" for anything else. Verified it has teeth by pointing the allowed path at a route nobody requests: the suite fails. The integration suite's stub answered unmatched requests with a 404, which is worse than it looks. waitForEnvironmentReady classifies 404 as transient and retries, so an unexpected call would burn the readiness deadline and surface as a timeout rather than naming the request that was not expected. It throws now too. Nothing in the suite was relying on the 404 - all four cases pass unchanged. The PreviewControlPlane.listReleaseFiles override took no parameters while the base method takes (reference, releaseId), so TypeScript had nothing to check the override against and a change to the base signature would have gone unnoticed. It now matches the base, including the AsyncIterable<DeployReleaseFile> return type. Constraint: no behaviour under test changed; the stubs only stop tolerating calls that were never expected. Tested: deno test --no-check --allow-all cli/commands/up/ cli/commands/deploy/ cli/commands/push/ cli/shared/deployment/ cli/mcp/tools/deploy-tool.test.ts (52 tests, 250 steps) Tested: negative check - allowed path deliberately broken, suite fails (exit 1) and passes again when restored Tested: check-sanitizer-baseline.ts (406/406, no opt-outs), check-test-typecheck-baseline.ts (92 grandfathered, 0 new) Tested: deno task typecheck, deno task lint, deno task fmt:check (all pass)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/shared/deployment/progress.ts:38
- deployProgressText() is declared to return
string | null, but the switch can fall through and implicitly returnundefinedif a new DeployStepName is added and not handled here. Add an explicitreturn nullafter the switch to keep the runtime behavior aligned with the type contract.
case "wait-environment-url":
return verbose ? `Waiting for ${environment} URL...` : `Verifying ${environment} URL...`;
}
}
Summary
Stacked on #3190 (consumes the Project Resolution module). Two commits:
1. Rebuild
upas a Deploy Execution adapter —veryfront uppreviously composedpushCommand+deployCommand({skipSourcePush})around whatDeployProject.executealready owns end-to-end, then discarded the verified deployment URL and printed one rebuilt viabuildPushUrlson the{slug}.preview.veryfront.compattern. Those coincide only for a domain-less preview environment on an app with no static page route — with a configured domain or a probed route path, the old output handed users a hostname nobody had checked. Now: oneDeployProject.executecall (sourceensure-pushed, branchmain, envpreview), and the printed/JSONpreviewUrlis the outcome's verified URL. Injection unified on the MCP style (dependencies: { deployProject? }). Progress text shared viacli/shared/deployment/progress.ts(moved verbatim). JSON field names and dry-runplannedActionsshape unchanged.Deliberate behavior changes (commit body): dry-run with a valid main receipt omits
push-sourcefrom the plan (deploy wouldn't push); dry-run with a receipt for another branch now surfaces the mismatch error instead of silently planning — pinned by an exact-message integration test that also asserts zero control-plane mutations.2. Retire the deploy command's bespoke testing seams —
deployCommandWithProjectForTesting+DeployCommandTestingSeamsdeleted;suppressJsonOutputand the four never-parseable polling knobs dropped fromDeployOptions(polling now reaches the module viacreateDeployProject({ polling }));skipSourcePushkept as the only command-boundary expression of sourcealready-pushed(eight integration assertions ride it), flagged for removal once a user-facing flag exists.Tests: the
upsuite no longer monkey-patchesglobalThis.fetch(was 6×) — it injects a fake DeployProject and finally asserts what deploy was asked to do, plus a new end-to-end suite over the real module withInMemoryDeployControlPlane(verified-URL provenance: output must carry the control plane's domain and must NOT contain the rebuilt pattern).Review
Independent code-review pass: approve, no blocking issues. Its one Low item (no pinning test for the stale-receipt dry-run change) is closed by the exact-message test above — which also corrected the review's own guess at the error (it's the
validatePushReceiptbranch-check error, notSOURCE_DIGEST_MISMATCH).Tested