fix(cli): make contract emission atomic and serialized under rapid saves#365
fix(cli): make contract emission atomic and serialized under rapid saves#365
Conversation
The SQL provider was updated to use context.configDir but the mongo provider was missed, causing watcher/loader path divergence when cwd differs from the config directory.
…onfigDir - Switch config-loader from node:path to pathe for consistency with the rest of the tooling layer. - Resolve .ts contract paths inside load() via context.configDir instead of capturing process.cwd() at config-definition time, so the watcher and loader always reference the same file.
The paths branch omitted the config file from the watch set, so edits to prisma-next.config.ts (e.g. changing paths or swapping provider) would not trigger re-emit. Also surface a partial-coverage warning when loadConfig fails instead of silently falling back.
Close D1 from the PR #356 review by driving a real Vite dev server through both scenarios that were previously only unit-mocked: modifying `contract.prisma` re-emits the contract, and editing the config to swap the declared PSL input re-emits against the new file.
Align with the Mongo PSL provider and with the parser step's sourceId. The absolute form stays accessible via meta.absoluteSchemaPath for downstream consumers that need it.
Collapse the ~20-line inline provider that postgres and mongo facades each build around a dynamic import of the user's `contract.ts`. The logic (path resolution, pathToFileURL, default-export extraction) is family-agnostic, so each contract-ts package exposes it as a sibling to typescriptContract.
- handleTrackedFileChange now runs ctx.file through pathe.resolve before comparing against watchedFiles / ignoredOutputFiles, so symlinked or case-folded paths line up with the resolved entries. - resolveContractOutputFiles no longer falls back to the hardcoded 'src/prisma/contract.json' when contract.output is missing. The filter simply skips; defineConfig() guarantees output is present in the happy path, and raw-object configs get a clean no-op instead of a path the CLI never actually writes.
…ceContext Left over from the split that moved configDir out of ContractSourceContext into ContractSourceEnvironment — the context builder no longer needs it.
Introduce publishPairWithRollback so writePlainContractArtifacts no longer needs a nested try/catch or the replacedJson/replacedDts tracking flags. The outer try/finally now owns only temp cleanup, which uses the idempotent rm(force: true) directly, so the cleanupJsonTemp/cleanupDtsTemp booleans drop out too.
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Rename "managed"/"plain" to publication strategies in contract-emit.ts
Intent
The two write paths in packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts are not two kinds of contract artifact; they are two publication strategies for the same pair of artifacts. The current names ("plain" vs "managed") read as a quality axis — one sounds like the baseline, the other like the upgrade. That is misleading in two ways:
- Nothing about the "managed" path produces a different artifact. It produces the same
contract.json+contract.d.ts, it just commits the pair to the public paths via a different mechanism. - The vocabulary leaks:
managedPublication: boolean,ManagedArtifactPaths,bootstrapManagedArtifactsFromExistingOutputs,ensurePublicManagedLinksall imply a property of the artifacts rather than a choice of commit mechanism.
Rename both strategies symmetrically so the code names the axis it actually varies, and so neither strategy wears the mask of being the default or the premium option.
The axis
Both paths:
- receive the same inputs,
- run through the same per-target serial queue (
emitWriteTargets), - honor the same generation check,
- emit the same bytes.
They differ only in how the pair becomes visible to readers:
| current "plain" | current "managed" | |
|---|---|---|
| Commit mechanism | Two renames over the public paths |
One rename of a staged current symlink |
| Public paths | Regular files | Symlinks into generations/g-<N>/ |
| Prior versions | Overwritten | Accumulate under generations/ |
| Rollback on failure | Restore captured bytes | Don't flip current |
The naming should name that.
Naming decision
Two strategies, named after the commit mechanism:
pair-rename— stage two temp files alongside the outputs, then rename them into the public paths as a pair (DTS first, JSON last).pointer-swap— materializegenerations/g-<N>/inside a hidden sibling root, then atomically re-point acurrentsymlink to it; the public paths are symlinks intocurrent/.
Default: 'pair-rename'.
Rename map
Types / options
| Before | After |
|---|---|
(local) const managedPublication = options.signal !== undefined |
(removed; see "API change" below) |
interface ManagedArtifactPaths |
interface PointerPublicationLayout |
API (exported) change
Add an explicit publication strategy option to ContractEmitOptions and stop deriving the strategy from signal:
export interface ContractEmitOptions {
readonly configPath: string;
readonly signal?: AbortSignal;
/**
* How the emitted pair is committed to the public paths.
* Defaults to 'pair-rename'.
*/
readonly publication?: 'pair-rename' | 'pointer-swap';
}Callers:
vite-plugin-contract-emit/src/plugin.tspassespublication: 'pointer-swap'explicitly (along withsignal).cli/src/commands/init/init.tsand all other callers stay on the default.
Functions
| Before | After |
|---|---|
writePlainContractArtifacts |
publishByPairRename |
writeManagedContractArtifacts |
publishByPointerSwap |
writeContractArtifacts (dispatcher) |
unchanged in name; dispatches on the new publication value |
getManagedArtifactPaths |
getPointerPublicationLayout |
createManagedGenerationDirName |
createGenerationDirName |
bootstrapManagedArtifactsFromExistingOutputs |
bootstrapPointerPublicationFromExistingOutputs |
ensurePublicManagedLinks |
ensurePointerPublicationSymlinks |
ensureSymlinkIfMissing |
unchanged |
replacePathWithSymlink |
unchanged |
publishPairWithRollback |
unchanged (already named after its mechanism) |
restoreArtifact |
unchanged |
tryLstat, pathExists, readExistingArtifact, createTempArtifactPath |
unchanged |
issueEmitGeneration, queueEmitWrite, getEmitWriteTargetState |
unchanged (these are target-level coordination, not strategy-specific) |
Parameters / local variables
| Before | After |
|---|---|
Parameter managedPublication: boolean on writeContractArtifacts |
publication: 'pair-rename' | 'pointer-swap' |
Local managedPaths (in publishByPointerSwap, bootstrap) |
layout |
publicJsonLinkTarget / publicDtsLinkTarget on ManagedArtifactPaths |
unchanged |
Scope
Files in scope:
packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts— the renames above.packages/1-framework/3-tooling/cli/src/control-api/types.ts— add thepublicationfield toContractEmitOptions.packages/1-framework/3-tooling/vite-plugin-contract-emit/src/plugin.ts— passpublication: 'pointer-swap'at theexecuteContractEmitcall site.packages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts— rename test descriptions to say(pair-rename publication)/(pointer-swap publication)where relevant; update any tests that passsignalpurely to opt into the old managed path so they now also passpublication: 'pointer-swap'.packages/1-framework/3-tooling/vite-plugin-contract-emit/README.md— update the "Atomic publication" bullet and the "How It Works" / mermaid flow to use the strategy name (pointer-swap) rather than "managed".
Non-goals
- No change to on-disk layout, bytes, or semantics. This is a naming / API-shape change only.
- No change to the generation counter, the serial queue, or the rollback logic.
- No change to callers that don't pass a signal — they continue to use
pair-renameimplicitly via the default. - Does not address the other findings from the PR code review (
.gitignore, restart-timeg-<N>collision, documentation of disk side effects). Those are tracked separately.
Acceptance criteria
rg "managed"inpackages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.tsreturns no matches.ContractEmitOptionsexposes apublication?: 'pair-rename' | 'pointer-swap'field, andexecuteContractEmitdoes not infer strategy fromsignal.- The vite plugin opts into pointer-swap explicitly.
pnpm --filter @prisma-next/cli typecheckand the fullcontract-emit.test.tssuite pass unchanged in behavior.- The vite plugin's README no longer uses "managed" to describe the publication strategy.
Code Review — PR #365Scope:
SummaryThe core hardening (per-output serial queue, monotonic generation counter issued synchronously, temp-file staging, pair-rename with rollback) is sound and the test intent is right. The managed-publication path built on top of it is not landable on its own terms (see banner and F01). Once that path is removed, a short list of smaller findings on the remaining code stands. What looks solid (and survives after F01)
FindingsF01 — Remove the managed-publication path (blocker)packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts — lines 26–32 ( Issue. Managed publication rewrites the public Suggestion. Delete the managed-publication path. Concretely:
The surviving architecture gives you AC2 (generation + queue) and AC3 ( F02 — Silently skipping older superseded emits is a behavioral change worth surfacingpackages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts — lines 285–303 (applies equally after F01's collapse) Issue. Before this PR, every call to Suggestion. Two options:
F03 — Docblock on
|
| AC | Verdict | Detail |
|---|---|---|
AC1: contract.json and contract.d.ts are replaced as one logical update rather than a split-brain sequence. |
WEAK | With F01 applied, the commit is two sequential renames (DTS then JSON, by design). A reader that samples the pair between the two renames can observe {new_dts, old_json}. For the dev-inner-loop use case this is well below any observable window. Strict atomicity of both files at the exact same instant is not achievable without changing the user-visible output shape (directory renames, symlink flips, or similar), and the only implementation of strict atomicity in the PR as submitted (pointer-swap) is being rejected on other grounds (F01). Recommendation: update the spec (and Linear) to reword AC1 from "replaced as one logical update" to something like "ordered commit where DTS is in place before JSON, such that any consumer that gates on contract.json existence sees matching types", and keep the current test as evidence. |
| AC2: A superseded emit cannot overwrite a newer successful emit. | PASS | "keeps the newer generation on disk when an older emit finishes later" (contract-emit.test.ts lines 308–335) drives two overlapping emits, resolves them in reverse order, and asserts the on-disk bytes match the newer createEmitResult('newer') — byte-level, for both files. The implementation advances nextGeneration synchronously in issueEmitGeneration (contract-emit.ts line 493) and guards at publish time (lines 288–290, 301–303). Unchanged by F01. |
| AC3: A failed emit leaves the previous good artifacts intact. | PASS | "preserves the previous artifacts when a new emit write fails" (lines 337–366) seeds a previous pair, forces the .d.ts write to throw, and byte-compares the on-disk content against the seeded previousJson/previousDts. "keeps the last good artifacts when a newer request fails after superseding an older emit" (lines 368–408) covers the composite case where the newer emit fails after advancing the generation, locking out the still-in-flight older emit — again with byte-level assertions. Both tests drive the pair-rename path and therefore survive F01 unchanged. |
AC4: The dev plugin can keep using the same executeContractEmit() control path after the hardening. |
NOT VERIFIED | The plugin source at plugin.ts lines 115–124 still calls executeContractEmit({ configPath, signal }) — no plugin-side changes required. The ContractEmitOptions / ContractEmitResult shape in types.ts lines 518–543 is unchanged. Type-level compatibility is real. There is no integration or E2E test exercising the plugin's full flow against the new executeContractEmit, so "it compiles and re-exports" is the only evidence. Given TML-2279 calls this out as an acceptance criterion, an E2E or plugin-level test that runs a dev-style scenario (two rapid emits, assert on the output pair) would close this gap. |
Summary
| Result | Count | ACs |
|---|---|---|
| PASS | 2 | AC2, AC3 |
| FAIL | 0 | — |
| NOT VERIFIED | 1 | AC4 |
| WEAK | 1 | AC1 |
Note on missing integration/E2E coverage. AC1 and AC4 are end-to-end claims. The current coverage is entirely mock-based at the control-api boundary. A small integration test at packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts that drives emitContract() twice in quick succession against a real temp directory and inspects the final pair would materially strengthen both ACs and is in scope for a PR that claims to deliver them. Safe to land as fast-follow once F01 is resolved.
Note on AC1's wording. Strict "replaced as one logical update" requires atomic multi-file commit, which the filesystem doesn't offer without changing the user-visible output shape. The PR's current answer to that (pointer-swap) is rejected by F01. A narrower, achievable AC is proposed in the table above.
System / Solution Design Review — PR #365Scope:
What the PR changes at the design level
The PR introduces two publication strategies inside this single operation, selected by whether a caller provided an
Both strategies share per-output coordination state: a This review is scoped around the banner above. The one-strategy simplification that follows from D1 is treated as given; remaining concerns are evaluated against that simplified shape. Subsystem fit (after D1)
Boundary correctness
Design-level concernsD1 — Reject the managed / pointer-swap publication strategy (blocker)packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts — lines 26–32 ( Issue. Managed publication's commit mechanism is "flip a symlink called
All of this is in exchange for closing a sub-millisecond window between two Recommendation. Remove the managed-publication path. Concretely:
A theoretical alternative that preserves strict multi-file atomicity without symlinking public files is to emit into a directory at the output path and rename a staged directory over it. That would require reshaping the public output API (the user's D2 —
|
| }; | ||
| } | ||
|
|
||
| function createDeferred<T>() { |
There was a problem hiding this comment.
I think we can use Promise.withResolvers() instead
| () => action(state), | ||
| () => action(state), | ||
| ); | ||
| state.queue = run.then( |
There was a problem hiding this comment.
What's the point of this? Why not just state.queue = run?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts (1)
181-195:⚠️ Potential issue | 🟡 MinorSkip dependency warnings for superseded bytes.
When
publication === 'superseded', these declarations were not written, so warning about their imports can confuse Vite/control-api callers during rapid saves.🔧 Proposed fix
const publication = await publishContractArtifactPairSerialized({ outputJsonPath, outputDtsPath, generation, signal, contractJson: emitResult.contractJson, contractDts: emitResult.contractDts, }); - const { validateContractDeps } = await import('../../utils/validate-contract-deps'); - const depsValidation = validateContractDeps(emitResult.contractDts, dirname(outputDtsPath)); - if (depsValidation.warning) { - process.stderr.write(`\n⚠ ${depsValidation.warning}\n`); + if (publication === 'written') { + const { validateContractDeps } = await import('../../utils/validate-contract-deps'); + const depsValidation = validateContractDeps(emitResult.contractDts, dirname(outputDtsPath)); + if (depsValidation.warning) { + process.stderr.write(`\n⚠ ${depsValidation.warning}\n`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts` around lines 181 - 195, When publishContractArtifactPairSerialized returns publication === 'superseded', skip running validateContractDeps and avoid emitting depsValidation.warning; update the control flow around the publishContractArtifactPairSerialized call (variable publication) so that validateContractDeps(emitResult.contractDts, dirname(outputDtsPath)) is only executed and its warning written to stderr when publication is not 'superseded', ensuring callers aren't shown import warnings for declarations that were not written.packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts (1)
457-463:⚠️ Potential issue | 🟡 MinorUpdate this stale emit mock to include
publication.This test still returns the old result shape, while the rest of the file now uses
createContractEmitResult()withpublication. Reuse the helper here so the fallback path exercises the current control-api contract.🧪 Proposed fix
- mockedExecuteContractEmit.mockResolvedValue({ - storageHash: 'abc123', - profileHash: 'def456', - files: { json: '/out/contract.json', dts: '/out/contract.d.ts' }, - }); + mockedExecuteContractEmit.mockResolvedValue(createContractEmitResult());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts` around lines 457 - 463, The test "falls back to watching the config file and warns when loadConfig fails" uses a stale mockedExecuteContractEmit result shape; update the mock to return the current shape by calling or reusing createContractEmitResult(...) and include a valid publication object (e.g., with needed fields) so the fallback path exercises the control-api contract; replace the hardcoded storageHash/profileHash/files object with createContractEmitResult({ publication: { ... } }) or equivalent to match other tests.
🧹 Nitpick comments (2)
packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts (1)
110-110: Remove the hardcoded test timeouts.These unit tests should rely on the default timeout or a shared timeout helper instead of raw
1000values. As per coding guidelines, “Use sharedtimeoutshelper functions instead of raw timeout numbers in tests”.🧹 Proposed cleanup
- }, 1000); + });Apply the same removal to the other two
it(..., 1000)calls.Also applies to: 143-143, 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts` at line 110, Three tests in publish-contract-artifact-pair.test.ts are using a hardcoded timeout value (the trailing , 1000) on their it(...) calls; remove the raw numeric timeout and use the shared test timeout helper (or rely on the default Jest timeout). Specifically, edit the three it(...) invocations that end with ", 1000" so they no longer pass the literal 1000; instead import and apply the project's shared timeouts helper (or simply omit the timeout parameter) for the tests that wrap the async assertions.packages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts (1)
431-443: Assert the older emit reportssupersededhere too.This test is the closest coverage for the “newer request fails after superseding an older emit” precondition; asserting
firstResult.publicationwould lock in the return semantics, not just the final disk state.🧪 Proposed test tightening
await expect(second).rejects.toThrow('simulated newer generation write failure'); firstEmit.resolve(createEmitResult('older')); - await first; + const firstResult = await first; + + expect(firstResult.publication).toBe('superseded'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts` around lines 431 - 443, The test must also assert that the older emit returns a publication status of "superseded": change the final await of the first emit to capture its resolved value (e.g. const firstResult = await first) and add an assertion like expect(firstResult.publication).toBe('superseded') after firstEmit.resolve(createEmitResult('older')) and before exiting the withMockedConfig block; this involves updating the executeContractEmit usage and the final await so the returned result from executeContractEmit can be inspected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts`:
- Around line 48-50: Update the docblock in contract-emit.ts to avoid
guaranteeing rollback; change the sentence that currently reads "and restores
the previous pair if publication fails after either path has been replaced" to a
best-effort phrasing such as "and attempts to restore the previous pair on
failure" (or similar), so the documentation reflects that rollback is
best-effort and may itself fail; locate the comment around the
publication/rename behavior in the contract-emit module to make this single-line
wording change.
In
`@packages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair.ts`:
- Around line 114-127: Re-check the beforePublish callback after taking rollback
snapshots to avoid TOCTOU races: after calling readExistingArtifact
(previousJson/previousDts) but before invoking publishPairWithRollback, call
beforePublish() again and if it returns false, abort (return false) so we don't
overwrite the newer emit with an older one; update the logic around the existing
beforePublish check and the publishPairWithRollback call in
publish-contract-artifact-pair.ts to perform this secondary check.
In
`@packages/1-framework/3-tooling/cli/test/commands/contract-emit.command.test.ts`:
- Around line 72-74: The test "returns an error envelope when publication is
superseded" currently uses a stable tmpdir-derived path via join(tmpdir(), ...),
leaving dirname(outputPath) behind; change it to create a unique per-test temp
directory using fs.mkdtemp (or mkdtempSync) and set outputPath inside that
directory, then ensure the test removes that temp directory in a finally block
or via afterEach cleanup so dirname(outputPath) is removed; locate the
outputPath and any logic that calls dirname(outputPath) in this test to replace
the tmpdir() usage and add cleanup.
In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/README.md`:
- Line 15: The README's "Ordered pair publication" line promises guaranteed
restoration; change the wording to indicate best-effort rollback — e.g., replace
"and restore the last good pair if publication fails" with language like "and
attempts to restore the last good pair if publication fails (rollback is
best-effort and may fail if rollback I/O errors occur)"; update the same
phrasing wherever it appears (the "Ordered pair publication" line referencing
renaming contract.d.ts before contract.json).
---
Outside diff comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts`:
- Around line 181-195: When publishContractArtifactPairSerialized returns
publication === 'superseded', skip running validateContractDeps and avoid
emitting depsValidation.warning; update the control flow around the
publishContractArtifactPairSerialized call (variable publication) so that
validateContractDeps(emitResult.contractDts, dirname(outputDtsPath)) is only
executed and its warning written to stderr when publication is not 'superseded',
ensuring callers aren't shown import warnings for declarations that were not
written.
In
`@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts`:
- Around line 457-463: The test "falls back to watching the config file and
warns when loadConfig fails" uses a stale mockedExecuteContractEmit result
shape; update the mock to return the current shape by calling or reusing
createContractEmitResult(...) and include a valid publication object (e.g., with
needed fields) so the fallback path exercises the control-api contract; replace
the hardcoded storageHash/profileHash/files object with
createContractEmitResult({ publication: { ... } }) or equivalent to match other
tests.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts`:
- Around line 431-443: The test must also assert that the older emit returns a
publication status of "superseded": change the final await of the first emit to
capture its resolved value (e.g. const firstResult = await first) and add an
assertion like expect(firstResult.publication).toBe('superseded') after
firstEmit.resolve(createEmitResult('older')) and before exiting the
withMockedConfig block; this involves updating the executeContractEmit usage and
the final await so the returned result from executeContractEmit can be
inspected.
In
`@packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts`:
- Line 110: Three tests in publish-contract-artifact-pair.test.ts are using a
hardcoded timeout value (the trailing , 1000) on their it(...) calls; remove the
raw numeric timeout and use the shared test timeout helper (or rely on the
default Jest timeout). Specifically, edit the three it(...) invocations that end
with ", 1000" so they no longer pass the literal 1000; instead import and apply
the project's shared timeouts helper (or simply omit the timeout parameter) for
the tests that wrap the async assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: afc2ff97-a8b7-4da6-92e0-e256a59295bc
📒 Files selected for processing (12)
packages/1-framework/3-tooling/cli/src/commands/contract-emit.tspackages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair-serialized.tspackages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair.tspackages/1-framework/3-tooling/cli/test/commands/contract-emit.command.test.tspackages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.tspackages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.tspackages/1-framework/3-tooling/vite-plugin-contract-emit/README.mdpackages/1-framework/3-tooling/vite-plugin-contract-emit/src/plugin.tspackages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.tstest/integration/test/cli.emit-command.e2e.test.ts
Address SevInf review feedback: drop the single-use toQueueTail wrapper and assign the in-flight promise directly. Subsequent waiters already handle both fulfillment and rejection branches, so the noop tail was redundant. Type the queue field as Promise<unknown> so the assignment type-checks.
Trim ~230 LOC of duplication and boilerplate without losing coverage: - Drop three integration tests that re-proved publishContractArtifactPair atomic semantics already covered by its unit tests: preserves-on-write-failure, restores-on-rename-failure, and publishes-dts-before-json. The "supersedes/superseded" tests in the integration suite still exercise that the helper is wired in. - Table-drive the four near-identical source-provider validation tests via describe.each, parameterized by source payload, expected error code, and expected substring. - Extract a seedPreviousArtifacts helper in the publish-contract- artifact-pair test to remove the repeated mkdir + writeFile preamble, and lift the previous/next fixture strings to module-level constants.
Both concurrent-emit tests assumed that the synchronously-first executeContractEmit call would also win the issueContractArtifactGeneration race. Under turbo's parallel workers that ordering is not guaranteed, and a swap leaves the "newer" call holding generation 1 — bypassing the .2.next.tmp write-failure mock and flipping the test's expectations. Wait for the first call to reach emit() before invoking the second so the queue assigns generations 1 and 2 in the expected order, and re-import actualFs locally inside the failing-write test (matching the prior pattern that survived parallel load).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts (1)
105-106: Eager generation claim widens the D2 supersession window (documented tradeoff).
issueContractArtifactGenerationis called here beforesource.loadandemitrun, so a later caller that races through a faster pipeline can easily claimgeneration + 1while this call is still insource.load. The docblock at lines 56-58 documents this correctly; flagging only because moving this line to just beforepublishContractArtifactPairSerialized(...)would narrow the window substantially without changing public semantics (the serialized publisher still enforces per-path ordering on enqueue). Leave as-is if the current ordering is an intentional serialization point for queueing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts` around lines 105 - 106, The call to issueContractArtifactGeneration(outputJsonPath) happens before source.load and emit, which unnecessarily widens the D2 supersession window; move the invocation of issueContractArtifactGeneration (currently assigned to generation) from just after destructuring outputPaths to immediately before publishContractArtifactPairSerialized(...) so the generation claim happens right before enqueueing the serialized artifact, narrowing the race window while preserving public semantics; keep references to outputJsonPath/outputDtsPath and ensure source.load and emit run first, then call issueContractArtifactGeneration, then call publishContractArtifactPairSerialized.packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts (4)
1-921: File size exceeds the 500-line test-file guideline; consider splitting.
plugin.test.tsis ~920 lines and covers distinct concerns (configResolved,configureServersetup,handleHotUpdate, error handling, concurrency/supersession). A split along the top-leveldescribeblocks — e.g.,plugin.configure-server.test.ts,plugin.hot-update.test.ts,plugin.errors.test.ts,plugin.concurrency.test.ts— would match the coding guideline and keep the new overlapping-emit coverage easier to evolve. This PR only adds to the file; a preparatory split can be done separately.As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts` around lines 1 - 921, The test file is too large; split plugin.test.ts into multiple focused test files by moving each top-level describe block into its own file (for example: move "configResolved" tests to plugin.config-resolved.test.ts, "configureServer" to plugin.configure-server.test.ts, "handleHotUpdate" and debounce/concurrency tests to plugin.hot-update.test.ts, and "error handling"/overlay tests to plugin.errors.test.ts), and factor shared helpers (createMockServer, createLoadedConfig, createContractEmitResult, applyModuleGraph, eventually, mocks/vi.setup) into a new test-utils module that each new test file imports; update each new file to import prismaVitePlugin and the shared utilities and preserve existing vi.mock calls so behavior is unchanged.
769-826: Good coverage for overlapping emits; verify the abort assertion timing.This test nicely closes the plugin-level gap the reviewer flagged (AC4). One thing worth confirming: the assertion
expect(firstSignal?.aborted).toBe(true)at line 817 relies on the plugin cancelling the in-flight emit synchronously when the secondhandleHotUpdatefires and its debounce elapses. If the plugin were ever changed to defer cancellation (e.g., schedule a microtask), this could become a flake — consider guarding witheventually()as a hardening step.- expect(mockedExecuteContractEmit).toHaveBeenCalledTimes(2); - expect(firstSignal?.aborted).toBe(true); + expect(mockedExecuteContractEmit).toHaveBeenCalledTimes(2); + await eventually(() => { + expect(firstSignal?.aborted).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts` around lines 769 - 826, The assertion that the first emit's AbortSignal is aborted is timing-sensitive; replace the direct synchronous check of firstSignal.aborted (the expect at the line asserting firstSignal?.aborted toBe(true)) with a guarded assertion using eventually() to poll until firstSignal?.aborted becomes true to avoid flakiness when handleHotUpdate or cancellation becomes asynchronous; update the test around the handleHotUpdate calls / mockedExecuteContractEmit checks to use eventually(() => expect(firstSignal?.aborted).toBe(true)) while keeping the rest of the flow (handleHotUpdate, advanceTimersByTimeAsync, firstEmit/secondEmit resolution, and ws.send assertions) intact.
91-103:eventuallyhelper is duplicated withcontract-emit.test.ts.The same polling helper (body identical, 20 attempts, awaits
Promise.resolve()between checks) is defined inpackages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts(lines 72-84). Extract into a shared test util (e.g., underpackages/1-framework/3-tooling/cli/test/utils/eventually.tsor a shared test-utils package) and import from both call sites.Also consider using
vi.waitFor(assertion, { timeout, interval })from Vitest, which already provides this semantics with configurable timing and better failure diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts` around lines 91 - 103, The helper function eventually is duplicated; extract the function eventually into a shared test util (e.g., create an exported helper eventually in a new test utils module) and replace the local definitions in both plugin.test.ts and contract-emit.test.ts with imports from that module, or alternatively replace calls to eventually with Vitest's vi.waitFor(assertion, { timeout, interval }) for better diagnostics; ensure the exported symbol is named eventually so existing call sites need only update their imports.
34-40: Mockingpatheto re-exportnode:pathis unnecessary.
patheis a drop-in replacement fornode:pathwith an identical API, so the production code under test (plugin.ts,executeContractEmit) can run against the realpathein a Node test environment without any divergence fromnode:pathbehaviour for the paths used here (all POSIX). Dropping this mock removes a source-of-truth divergence between tests and runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts` around lines 34 - 40, Remove the unnecessary vi.mock that re-exports node:path for 'pathe' in the test: delete the vi.mock(...) block so the tests use the real 'pathe' implementation. Ensure the test still imports/run the code under test (executeContractEmit and any helpers in plugin.ts) normally and that no other mocks for 'pathe' remain; run the tests to verify behavior unchanged.packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts (1)
160-178: Optional: capture-and-assert reads more naturally thanrejects.toSatisfy+ innerexpect.The inner
expect()calls insidetoSatisfywork (they throw on mismatch, so Vitest surfaces the right failure), but capturing the rejection and asserting on it is a bit more direct and avoids thereturn truedance. Not blocking.✏️ Suggested alternative
- await expect( - publishContractArtifactPair({ - outputJsonPath, - outputDtsPath, - contractJson: NEXT_JSON, - contractDts: NEXT_DTS, - publicationToken: 'publish', - }), - ).rejects.toSatisfy((error: unknown) => { - expect(error).toBe(publishError); - expect(error).toBeInstanceOf(Error); - expect((error as Error & { cause?: unknown }).cause).toBeInstanceOf(AggregateError); - expect((error as Error & { cause?: AggregateError }).cause?.errors).toEqual([rollbackError]); - return true; - }); + const caught = await publishContractArtifactPair({ + outputJsonPath, + outputDtsPath, + contractJson: NEXT_JSON, + contractDts: NEXT_DTS, + publicationToken: 'publish', + }).then( + () => undefined, + (error: unknown) => error, + ); + expect(caught).toBe(publishError); + const cause = (caught as Error & { cause?: unknown }).cause; + expect(cause).toBeInstanceOf(AggregateError); + expect((cause as AggregateError).errors).toEqual([rollbackError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts` around lines 160 - 178, Replace the rejects.toSatisfy pattern in the publishContractArtifactPair test with an explicit capture of the thrown error (try/catch or await expect(...).rejects to capture via a variable) and perform direct assertions on that captured error: assert equality to publishError, assert instance of Error, assert that (error as Error & { cause?: unknown }).cause is an instance of AggregateError, and assert that (error as Error & { cause?: AggregateError }).cause?.errors equals [rollbackError]; keep the subsequent file-read assertions the same. Ensure you reference the same call to publishContractArtifactPair and the same local variables (outputJsonPath, outputDtsPath, NEXT_JSON, NEXT_DTS, publishError, rollbackError) when replacing the rejects.toSatisfy block.packages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.ts (1)
310-317: Brittle match on.2.next.tmp; also redundantlocalFs.Two small concerns:
- The substring
'.2.next.tmp'bakes in both the tmp-file naming convention and the assumption that the second emit gets generation2. The latter holds because each test uses a freshmkdtemp(soissueContractArtifactGenerationstarts at 1 peroutputJsonPath) and the publicationToken is stringified as-is. A refactor of the generation counter (start at 0, UUID token, different phase suffix, etc.) would silently break this test without changing observable behaviour. Consider deriving the expected token from the known second-call generation, or at minimum leaving a comment explaining the2andnext.tmpderivation.localFsat line 310 is the same module already captured asactualFsinbeforeEach(line 145). ReuseactualFshere.- const localFs = await vi.importActual<FsModule>('node:fs/promises'); mockedWriteFile.mockImplementation(async (...args: Parameters<FsWriteFile>) => { const [path] = args; - if (String(path).includes('.2.next.tmp')) { + // Token comes from `publishContractArtifactPairSerialized` which passes + // `String(generation)`; per-path generation starts at 1 in a fresh tmpdir, + // so the second emit's tmp file contains '.2.next.tmp'. + if (String(path).includes('.2.next.tmp')) { throw new Error('simulated newer generation write failure'); } - return localFs.writeFile(...args); + return actualFs.writeFile(...args); });
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts`:
- Around line 191-197: The dynamic import of validateContractDeps (used inside
the publication === 'written' branch and called with emitResult.contractDts and
dirname(outputDtsPath)) is unnecessary—either make the import a top-level import
(import { validateContractDeps } from '../../utils/validate-contract-deps') for
clarity, or if you intentionally deferred loading to avoid work when publication
!== 'written' add a one-line comment above the dynamic import explaining the
laziness intent; apply the same change to the identical pattern in the other
contract-emit usage so both places consistently use a top-level import or
clearly-documented lazy import.
In
`@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts`:
- Around line 7-17: Extract the duplicated ambient declaration into a single
shared test-declaration file named promise-with-resolvers.d.ts that exports the
PromiseResolvers<T> interface and the global declaration for
Promise.withResolvers<T>(); remove the duplicate blocks from both test files
(the PromiseResolvers<T> interface and the declare global { interface
PromiseConstructor { withResolvers<T>(): PromiseResolvers<T>; } }); then add a
reference to that shared declaration from each test tsconfig (or include it in
the test-utils type entries) so both tests pick up the declaration during
compilation.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.ts`:
- Around line 105-106: The call to
issueContractArtifactGeneration(outputJsonPath) happens before source.load and
emit, which unnecessarily widens the D2 supersession window; move the invocation
of issueContractArtifactGeneration (currently assigned to generation) from just
after destructuring outputPaths to immediately before
publishContractArtifactPairSerialized(...) so the generation claim happens right
before enqueueing the serialized artifact, narrowing the race window while
preserving public semantics; keep references to outputJsonPath/outputDtsPath and
ensure source.load and emit run first, then call
issueContractArtifactGeneration, then call
publishContractArtifactPairSerialized.
In
`@packages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.ts`:
- Around line 160-178: Replace the rejects.toSatisfy pattern in the
publishContractArtifactPair test with an explicit capture of the thrown error
(try/catch or await expect(...).rejects to capture via a variable) and perform
direct assertions on that captured error: assert equality to publishError,
assert instance of Error, assert that (error as Error & { cause?: unknown
}).cause is an instance of AggregateError, and assert that (error as Error & {
cause?: AggregateError }).cause?.errors equals [rollbackError]; keep the
subsequent file-read assertions the same. Ensure you reference the same call to
publishContractArtifactPair and the same local variables (outputJsonPath,
outputDtsPath, NEXT_JSON, NEXT_DTS, publishError, rollbackError) when replacing
the rejects.toSatisfy block.
In
`@packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts`:
- Around line 1-921: The test file is too large; split plugin.test.ts into
multiple focused test files by moving each top-level describe block into its own
file (for example: move "configResolved" tests to
plugin.config-resolved.test.ts, "configureServer" to
plugin.configure-server.test.ts, "handleHotUpdate" and debounce/concurrency
tests to plugin.hot-update.test.ts, and "error handling"/overlay tests to
plugin.errors.test.ts), and factor shared helpers (createMockServer,
createLoadedConfig, createContractEmitResult, applyModuleGraph, eventually,
mocks/vi.setup) into a new test-utils module that each new test file imports;
update each new file to import prismaVitePlugin and the shared utilities and
preserve existing vi.mock calls so behavior is unchanged.
- Around line 769-826: The assertion that the first emit's AbortSignal is
aborted is timing-sensitive; replace the direct synchronous check of
firstSignal.aborted (the expect at the line asserting firstSignal?.aborted
toBe(true)) with a guarded assertion using eventually() to poll until
firstSignal?.aborted becomes true to avoid flakiness when handleHotUpdate or
cancellation becomes asynchronous; update the test around the handleHotUpdate
calls / mockedExecuteContractEmit checks to use eventually(() =>
expect(firstSignal?.aborted).toBe(true)) while keeping the rest of the flow
(handleHotUpdate, advanceTimersByTimeAsync, firstEmit/secondEmit resolution, and
ws.send assertions) intact.
- Around line 91-103: The helper function eventually is duplicated; extract the
function eventually into a shared test util (e.g., create an exported helper
eventually in a new test utils module) and replace the local definitions in both
plugin.test.ts and contract-emit.test.ts with imports from that module, or
alternatively replace calls to eventually with Vitest's vi.waitFor(assertion, {
timeout, interval }) for better diagnostics; ensure the exported symbol is named
eventually so existing call sites need only update their imports.
- Around line 34-40: Remove the unnecessary vi.mock that re-exports node:path
for 'pathe' in the test: delete the vi.mock(...) block so the tests use the real
'pathe' implementation. Ensure the test still imports/run the code under test
(executeContractEmit and any helpers in plugin.ts) normally and that no other
mocks for 'pathe' remain; run the tests to verify behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0db79898-9259-419d-b330-6500776d8068
📒 Files selected for processing (8)
packages/1-framework/3-tooling/cli/src/control-api/operations/contract-emit.tspackages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair-serialized.tspackages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair.tspackages/1-framework/3-tooling/cli/test/commands/contract-emit.command.test.tspackages/1-framework/3-tooling/cli/test/control-api/contract-emit.test.tspackages/1-framework/3-tooling/cli/test/utils/publish-contract-artifact-pair.test.tspackages/1-framework/3-tooling/vite-plugin-contract-emit/README.mdpackages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/1-framework/3-tooling/vite-plugin-contract-emit/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/1-framework/3-tooling/cli/test/commands/contract-emit.command.test.ts
- packages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair.ts
- packages/1-framework/3-tooling/cli/src/utils/publish-contract-artifact-pair-serialized.ts
Address coderabbit review: the dynamic await import had no cycle or load-time cost justification. validate-contract-deps.ts only depends on node:module and has no back-edges into contract-emit.ts. Use a regular static import in both call sites for consistency and clarity.
Address coderabbit review: the duplicated PromiseConstructor.withResolvers ambient declaration in two test files only existed because the base tsconfig pinned lib to ES2022. Engines.node is >=24 (which has Promise.withResolvers natively), so opt the cli and vite-plugin-contract-emit packages into the ES2024 lib and drop the local ambient declarations from contract-emit.test.ts and plugin.test.ts.
jkomyno
changed the title
3 participants
Summary
publishContractArtifactPairhelper: stagescontract.jsonandcontract.d.tsto per-PID/per-token temp paths, then renamescontract.d.tsbeforecontract.jsonso type-only consumers never observe a mismatched pair. On failure mid-rename it best-effort restores the previous pair and attaches rollback failures to the publish error viacause(AggregateError).publishContractArtifactPairSerialized: a per-output-path FIFO queue keyed off a monotonic generation counter. An older emit that arrives at the queue after a newer one returnspublication: 'superseded'instead of overwriting the newer artifacts. BothexecuteContractEmitand theprisma-next emitcommand go through this path.ContractEmitResult.publication: 'written' | 'superseded'field; the vite plugin consumes it for HMR ordering, and the CLI command translates'superseded'into anerrorRuntimeenvelope so callers can react.@prisma-next/cliand@prisma-next/vite-plugin-contract-emittsconfigs tolib: ["ES2024"]soPromise.withResolversis available natively (projectengines.node >= 24); drop the duplicated ambient declarations from the test files.validateContractDepsfrom a dynamicawait import(...)to a top-level static import in both call sites — the deferred load had no cycle/cost justification.publish-contract-artifact-pairunit suite (rename ordering, write-failure preservation,beforePublishrecheck, rollback-failurecause); integration tests for queued supersession (keeps the newer generation on disk,keeps the last good artifacts when a newer request fails); table-driven source-provider validation; and a vite-plugin overlapping-emit test.Validation
pnpm --filter @prisma-next/cli typecheckpnpm --filter @prisma-next/cli testpnpm --filter @prisma-next/vite-plugin-contract-emit typecheckpnpm --filter @prisma-next/vite-plugin-contract-emit testpnpm linton the touched filesRefs TML-2279
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests