Stop shipping a mock API to users, and stop fixtures being tidier than the real one - #159
Merged
Merged
Conversation
Four pieces of the same problem: the CLI carried a second implementation for tests to run, and the tests that ran it could not tell you anything about the code users get. **Fixture mode is gone.** `isRealMode` forked behaviour in thirty places across six controllers, choosing between the real path and a mock API selected at runtime by `PRISMA_CLI_MOCK_FIXTURE_PATH`. The mock shipped: `dist/adapters/mock-api.js`, twelve kilobytes, in the published package. The whole `use-cases` layer existed only to serve those branches — nothing else called it — so it goes too, along with the fixture data file and the four test files that covered the deleted layer. That is 696 lines of mock, five use-case modules, and about 2,900 lines net. The hidden `--provider`, `--user` and `--workspace` flags on `auth login` go with it. They drove the fixture-only selection flow and had done nothing else since. Three unit tests genuinely needed an API to talk to. They now talk to one: `tests/helpers/fake-management-api.ts` starts a real HTTP server. The distinction matters — the CLI runs its ordinary client, request pipeline and response parsing, and only the far end of the socket is ours. A fake server tests the shipped code; a fixture mode tested a branch users never reach. **Fixtures now carry the id shapes the API really uses.** Workspace ids appeared as `ws_1` throughout, a form that exists nowhere: the API returns `wksp_`-prefixed ids and a credential's claim carries the bare one. Fixtures that wrote one string for both could not see the mismatch that made `project list` report "No projects found." for a workspace holding fifteen. API-shaped rows are prefixed now and credential-shaped ones are not, which is how they arrive in production. Every test still passes, which is the point: it demonstrates the comparison fix works, against fixtures that would have caught the original defect. **The three `agent` commands have real happy paths**, so AWAITING_COVERAGE drops from sixteen to thirteen. They install Prisma's skills and write files, so the test asserts the state transition — nothing installed, then installed — rather than trusting the command's own answer. **And the CodeQL stack-trace exposure is fixed.** The OAuth callback server wrote an internal error's message straight into the HTTP response, where a page this process does not control could read it. The browser gets a fixed sentence; the operator still gets the real error, which is what the promise rejects with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Summary by CodeRabbit
WalkthroughThe CLI removes the mock API, fixture runtime fields, and fixture-mode use cases. Authentication and command controllers now use direct Management API operations. Tests use a local fake Management API and production-shaped workspace identifiers. Initialization tests cover API-backed linking. New end-to-end tests cover agent status, installation, lock-file creation, and updates. Login callback responses now hide underlying error details. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/tests/init.test.ts (1)
217-243: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose each fake API in
finally.If
executeClior an assertion throws, the laterapi.close()call does not run. The listening HTTP server can keep the test worker alive.
packages/cli/tests/init.test.ts#L217-L243: wrap the API-backed test body intry/finally.packages/cli/tests/init.test.ts#L247-L268: wrap the API-backed test body intry/finally.packages/cli/tests/init.test.ts#L899-L945: wrap the API-backed test body intry/finally.packages/cli/tests/init.test.ts#L949-L1017: wrap the API-backed test body intry/finally.Proposed cleanup pattern
const api = await startFakeManagementApi(); try { // CLI execution and assertions } finally { await api.close(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/tests/init.test.ts` around lines 217 - 243, Ensure every API-backed test closes its fake management API in a finally block: wrap the test bodies at packages/cli/tests/init.test.ts lines 217-243, 247-268, 899-945, and 949-1017 in try/finally after startFakeManagementApi(), with each finally awaiting api.close().packages/cli/src/controllers/database.ts (1)
712-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the bare block left by the deleted fixture branch.
Lines 712 and 734 form a block statement with no controlling condition. It is the remnant of the removed
if (isRealMode)branch. The block adds a scope that serves no purpose and suggests a missing alternative branch. Unindent the body.♻️ Proposed refactor
- { - const targetResult = await resolveProjectTarget({ - context, - workspace, - explicitProject: flags.projectRef, - listProjects: () => - listRealWorkspaceProjects(client, context.runtime.signal), - commandName, - }); - if (targetResult.isErr()) { - throw projectResolutionErrorToCliError(targetResult.error); - } - - return { - provider: createManagementDatabaseProvider(client, { - formatCommand: resolvePrismaCliPackageCommandFormatterSync( - context.runtime.cwd, - ), - workspaceId: workspace.id, - }), - target: targetResult.value, - }; - } + const targetResult = await resolveProjectTarget({ + context, + workspace, + explicitProject: flags.projectRef, + listProjects: () => + listRealWorkspaceProjects(client, context.runtime.signal), + commandName, + }); + if (targetResult.isErr()) { + throw projectResolutionErrorToCliError(targetResult.error); + } + + return { + provider: createManagementDatabaseProvider(client, { + formatCommand: resolvePrismaCliPackageCommandFormatterSync( + context.runtime.cwd, + ), + workspaceId: workspace.id, + }), + target: targetResult.value, + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/controllers/database.ts` around lines 712 - 734, Remove the standalone brace block surrounding the resolveProjectTarget and return logic, and unindent its contents while preserving the existing behavior and control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 30: Correct the fake API helper reference in AGENTS.md to use the
repository-relative path packages/cli/tests/helpers/fake-management-api.ts
instead of tests/helpers/fake-management-api.ts; leave the surrounding testing
guidance unchanged.
In `@packages/cli/e2e/agent.e2e.ts`:
- Around line 69-78: Update the “updates the skills already installed” test in
the agent update describe block to initialize its own workdir and install skills
before running agent update. Ensure the setup uses the existing CLI session and
install flow, then preserve the current update assertions and skills-lock.json
check.
In `@packages/cli/src/controllers/database.ts`:
- Line 907: Remove the unused underscore-prefixed helpers: delete
_backupNotFoundError and _connectionNotFoundError in
packages/cli/src/controllers/database.ts (907-907 and 929), _keyNotFoundError in
packages/cli/src/controllers/bucket.ts (320-330), and
_createPendingRepositoryConnection in packages/cli/src/controllers/project.ts
(1848-1848). If any corresponding errors still need to reach users, wire them
into the relevant Management API path instead of retaining the dead helpers.
In `@packages/cli/src/controllers/project.ts`:
- Around line 300-310: In the project setup flow, declare provider and projects
as const, make provider non-null, and update resolveInteractiveProjectLinkSetup
to accept the non-null provider type. Remove the unreachable provider-null guard
and its fixture-mode error message, while preserving the remaining setup logic.
- Line 993: Update the Git flow client setup around the ManagementApiClient
usage to use the generated SDK client directly, removing the unknown casts and
any locally defined method contract. Reuse ManagementApiClient for the Git
endpoints so request paths, bodies, and response types are compile-time checked.
In `@packages/cli/tests/auth-real-mode.test.ts`:
- Line 6: Restore direct unit coverage for renderAuthSuccess alongside the
existing auth presenter tests. Add assertions for empty rows, users without an
email, and service-token identity rendering, while retaining the existing JSON
and signed-out coverage.
---
Outside diff comments:
In `@packages/cli/src/controllers/database.ts`:
- Around line 712-734: Remove the standalone brace block surrounding the
resolveProjectTarget and return logic, and unindent its contents while
preserving the existing behavior and control flow.
In `@packages/cli/tests/init.test.ts`:
- Around line 217-243: Ensure every API-backed test closes its fake management
API in a finally block: wrap the test bodies at packages/cli/tests/init.test.ts
lines 217-243, 247-268, 899-945, and 949-1017 in try/finally after
startFakeManagementApi(), with each finally awaiting api.close().
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aca85968-8afa-487c-b0f3-aba13d40d202
📒 Files selected for processing (49)
AGENTS.mdpackages/cli/e2e/agent.e2e.tspackages/cli/fixtures/mock-api.jsonpackages/cli/src/adapters/mock-api.tspackages/cli/src/auth/login.tspackages/cli/src/cli.tspackages/cli/src/commands/auth/index.tspackages/cli/src/controllers/app.tspackages/cli/src/controllers/auth.tspackages/cli/src/controllers/branch.tspackages/cli/src/controllers/bucket.tspackages/cli/src/controllers/database.tspackages/cli/src/controllers/project.tspackages/cli/src/controllers/select-prompt-port.tspackages/cli/src/shell/runtime.tspackages/cli/src/use-cases/auth.tspackages/cli/src/use-cases/branch.tspackages/cli/src/use-cases/contracts.tspackages/cli/src/use-cases/create-cli-gateways.tspackages/cli/src/use-cases/project.tspackages/cli/tests/app-branch-database.test.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-env-vars.test.tspackages/cli/tests/app.test.tspackages/cli/tests/auth-controller.test.tspackages/cli/tests/auth-real-mode.test.tspackages/cli/tests/auth-usecases.test.tspackages/cli/tests/auth.test.tspackages/cli/tests/branch-controller.test.tspackages/cli/tests/branch-usecases.test.tspackages/cli/tests/database.test.tspackages/cli/tests/e2e-coverage.test.tspackages/cli/tests/helpers.tspackages/cli/tests/helpers/fake-management-api.tspackages/cli/tests/init-agent-setup.test.tspackages/cli/tests/init.test.tspackages/cli/tests/project-real-mode.test.tspackages/cli/tests/project-usecases.test.tspackages/cli/tests/project.test.tspackages/cli/tests/shell.test.tspackages/cli/tests/update-check.test.tspackages/cli/tests/use-case-helpers.tspackages/cli/tests/v8-branch.test.tspackages/cli/tests/v8-bucket.test.tspackages/cli/tests/v8-git.test.tspackages/cli/tests/v8-legacy-context.test.tspackages/cli/tests/v8-postgres.test.tspackages/cli/tests/v8-project.test.tspackages/cli/tests/version.test.ts
💤 Files with no reviewable changes (27)
- packages/cli/tests/version.test.ts
- packages/cli/tests/auth-controller.test.ts
- packages/cli/src/cli.ts
- packages/cli/tests/project-usecases.test.ts
- packages/cli/tests/branch-usecases.test.ts
- packages/cli/tests/use-case-helpers.ts
- packages/cli/tests/auth-usecases.test.ts
- packages/cli/fixtures/mock-api.json
- packages/cli/src/use-cases/auth.ts
- packages/cli/src/use-cases/create-cli-gateways.ts
- packages/cli/src/use-cases/contracts.ts
- packages/cli/tests/project.test.ts
- packages/cli/tests/init-agent-setup.test.ts
- packages/cli/tests/app.test.ts
- packages/cli/src/use-cases/project.ts
- packages/cli/tests/app-env-vars.test.ts
- packages/cli/src/adapters/mock-api.ts
- packages/cli/src/shell/runtime.ts
- packages/cli/tests/database.test.ts
- packages/cli/tests/update-check.test.ts
- packages/cli/src/use-cases/branch.ts
- packages/cli/tests/app-controller.test.ts
- packages/cli/tests/shell.test.ts
- packages/cli/tests/app-branch-database.test.ts
- packages/cli/src/controllers/app.ts
- packages/cli/tests/helpers.ts
- packages/cli/tests/auth.test.ts
…that outlived its mode Review found that four helpers left without callers by the fixture-mode removal had been renamed with a leading underscore rather than deleted. That was `biome check --write --unsafe` silencing the unused-symbol rule, and I let it stand: `_backupNotFoundError`, `_connectionNotFoundError`, `_keyNotFoundError` and `_createPendingRepositoryConnection` all served the deleted fixture providers. An underscore hides the missing caller instead of answering it, which is the same habit this branch exists to break. They are gone. `resolveInteractiveProjectLinkSetup` still took a nullable provider, though the only caller now assigns one unconditionally. The nullable type kept alive a guard whose message told the reader to "rerun without fixture mode enabled" — advice about a mode this branch removes, in a branch no user could reach. The binding is `const` and non-null now, and the guard is gone with it. `agent update` shared a working directory with `agent install` and so depended on the order they ran in; a focused run would have found an empty directory. It installs into its own directory first. Also corrects the fake-server path in AGENTS.md: the file sits under `packages/cli`, and AGENTS.md is at the repository root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…k its only ones Removing the fixture-mode tests removed the only coverage of `renderAuthSuccess` with it: no test file referenced it afterwards. The rendering is real behaviour that users see, so losing its tests along with the mode that happened to reach it was a regression, not a cleanup. It is tested directly now rather than through whichever command calls it: the login rows, the rows omitted when there is no provider, user or workspace, the service-token labels for a credential with and without a name, signed-out, and the logout line. Not addressed, with a reason: review also asked for the Git flow in `runGitConnect` to use the generated SDK client instead of `client as unknown as SourceRepositoryApiClient` and a hand-written method contract. That cast arrived in #8 and this branch only reindented it while unwrapping a fixture branch. Replacing it means checking whether the published SDK covers the source-repository paths at all, which is its own change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
approved these changes
Aug 12, 2026
…hand-written contract
`runGitConnect` and `runGitDisconnect` reached the management API through `client as unknown as SourceRepositoryApiClient`, a 110-line interface in the controller that restated the request paths, bodies and response shapes by hand. A cast through `unknown` disables every check that would tell you the restatement had drifted from the API.
The restatement was unnecessary: `@prisma/management-api-sdk@1.55.0` declares all five paths these commands use — `/v1/source-repositories`, `/v1/source-repositories/{id}`, `/v1/scm-installations`, `/v1/scm-installations/install-intents`, and `/v1/scm-installations/{installationId}/repositories`. The helpers take `ManagementApiClient` now, both casts are gone, and the interface with them. The generated types accepted every call site unchanged, which is the evidence the hand-written copy was accurate — and equally the evidence that nothing would have told us when it stopped being.
The same cast in the v8 git context goes too, so `ctx.api` reaches those helpers as itself.
Checked against the real API rather than the compiler alone: `git disconnect` queries `/v1/source-repositories` through the newly typed client and correctly reports no connected repository for a fresh project.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
wmadden-electric
added a commit
that referenced
this pull request
Aug 12, 2026
… onto the new engine Main moved substantially while this branch was in flight: S2c's service and agent families, S3's composer commands and ctx.spawn, S8's service resource surface, engine-owned CI detection, engine-owned package manager operations, an on-demand config loader, and the fixture machinery already deleted by #159. What this merge decided, beyond taking both sides: - init's link step keeps calling the shared linkDirectoryToProject, updated to main's listWorkspaceProjects signature. Main had inlined the handler body this branch extracted; the extraction survives because init depends on it. - The bin-side detectPackageManager this branch carried is deleted in favour of main's engine-owned detection — Runtime.packageManager is now an optional override, exactly the shape the operator asked for. - ctx.host joins main's grown context beside ctx.isCI; every Runtime literal in the engine tests gains the fixed test host. - init and its tests adopt main's summary-block field (status, not tone) and the mount-coverage test's FAMILYLESS set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until this change, every
prisma-clirelease shipped a mock of the management API to users:It was not dead code. Thirty branches across six controllers chose between the real path and the mock at runtime:
What we're changing
Fixture mode is deleted. Along with the
use-caseslayer that existed only to serve it, the fixture data file, and the tests that covered the deleted layer.Three more changes ride along, all the same subject: fixtures now use the API's real id shapes, the three
agentcommands get real happy paths, and a CodeQL stack-trace exposure is fixed.Why fixture mode had to go
It is the structural cause of the bug this series started with.
prisma-v8 project listreported "No projects found." for a workspace holding fifteen projects, and the unit suite passed throughout.When production code carries a second implementation for tests, "the tests pass" can mean "the fixture path works." The clearest evidence was already in the repo: a test file named
project-real-mode.test.tsexists because someone noticed the fixture path was being tested instead of the real one.Nothing outside those branches used the use-cases layer, so it went with them. So did the hidden
--provider,--userand--workspaceflags onauth login— they drove the fixture-only selection flow and had done nothing else since.What replaces it
Three unit tests genuinely needed an API to talk to. They now talk to one:
The distinction is the whole point. The CLI runs its ordinary client, its request pipeline, its response parsing — only the far end of the socket is ours. A fake server tests the shipped code. A fixture mode tested a branch users never reach.
Fixtures that can no longer be tidier than the API
Workspace ids appeared throughout as
ws_1— a form that exists nowhere. The API returnswksp_-prefixed ids; a credential'sworkspace_idclaim carries the bare one. Fixtures writing one string for both cannot see the mismatch that emptiedproject list.wksp_ws_1/v1/projectsreturnsws_1Every test still passes, and that is the result worth having: the suite now reproduces production's disagreement, and the comparison fix from the earlier PR handles it. These fixtures would have caught the original defect.
Doing this surfaced the distinction sharply. My first pass prefixed expectations too, and four tests failed — correctly, because the workspace a command reports comes from the credential and is therefore bare. The failures were the fixtures teaching me the difference.
Coverage and the security fix
The three
agentcommands now have real happy paths, soAWAITING_COVERAGEdrops from 16 to 13. They install skills and write files, so the test asserts the transition — nothing installed, then installed — rather than trusting the command's own answer.The OAuth callback server wrote an internal error's message straight into the HTTP response, readable by a page this process does not control (CodeQL alert 6,
js/stack-trace-exposure). The browser now gets a fixed sentence; the operator still gets the real error, which is what the promise rejects with.Verification
dist/adapters/no longer containsmock-api.js; noisRealMode,MockApiorfixturePathreference survives insrc.One note on running tests locally:
pnpm teststraight after wipingdistfails with missingcli-enginechunks. That reproduces on unmodifiedmain, so it predates this change; CI builds first and then filters, which is why CI is green.pnpm build && pnpm --filter @prisma/cli testis the reliable local sequence.Alternatives considered
Keep fixture mode, stop shipping it. Tree-shaking or a build flag would have removed the 12KB without touching the branches. Rejected: the size was never the problem. Thirty runtime forks between what tests run and what users run is the problem, and a build flag preserves every one of them.
Rewrite the fixture-mode tests onto mocked API clients instead of deleting them. Rejected for the four use-case test files: they covered a layer that no longer exists, so there was no behaviour left to cover. The three tests that covered real behaviour were kept and moved onto the fake server.
Blanket-rename
ws_1towksp_ws_1everywhere. Rejected, and this is the trap worth naming: it would make both sides of every comparison prefixed and still identical, preserving the exact blindness while looking like a fix. The value is in the two forms disagreeing.Split this into four PRs. Reasonable, and I would have defaulted to it. Kept as one at the author's request; the four parts share a subject, and the fixture-id change only makes sense once fixture mode is gone.
🤖 Generated with Claude Code