Feat/project delete branch crud#73
Open
iamafridi wants to merge 4 commits into
Open
Conversation
Summary by CodeRabbitNew Features
Documentation
WalkthroughThis pull request introduces four new CLI commands for managing projects and branches in the Prisma CLI. The 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Author
|
Summary 🔒 Fixes
📖 Docs
✨ Features
🧹 Cleanup
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@packages/cli/src/adapters/token-storage.ts`:
- Around line 72-80: The constructor of FileTokenStorage registers a
process.on("exit") listener that closes the lock file but never removes the
listener, causing accumulation; update FileTokenStorage to either (A) register a
single module-level exit handler that cleans any instance locks (preferable) or
(B) store the handler function returned when adding the listener and expose a
dispose/destroy method on FileTokenStorage that calls
process.removeListener("exit", handler) and clears references; ensure you
reference currentRefreshLockId and lockFilePath inside the centralized handler
or removed listener so each instance can be GC'd when disposed.
In `@packages/cli/src/controllers/branch.ts`:
- Around line 26-314: The three functions runBranchCreate, runBranchDelete, and
runBranchRename duplicate the same fixture check, auth/workspace retrieval,
client acquisition, and resolveProjectTarget calls; extract that shared
boilerplate into a helper (e.g., prepareBranchOperation) that accepts (context,
commandName) and returns { client, workspace, target }, update each runBranch*
to perform only input-specific validation and call prepareBranchOperation to get
client/workspace/target, and keep existing error types/behavior
(featureUnavailableError, requireAuthenticatedAuthState, requireComputeAuth,
workspaceRequiredError, resolveProjectTarget, listRealWorkspaceProjects,
createSelectPromptPort) so callers and subsequent provider calls remain
unchanged.
In `@packages/cli/src/controllers/project.ts`:
- Around line 259-262: The code currently calls readLocalResolutionPin to check
localPin before calling removeLocalResolutionPin, causing redundant filesystem
I/O; replace that read+check with a single call to removeLocalResolutionPin
passing the expected pin ({ workspaceId: workspace.id, projectId: project.id })
so removeLocalResolutionPin itself re-reads and validates to prevent TOCTOU;
update the localPinCleared variable to capture the boolean result from
removeLocalResolutionPin and remove the prior readLocalResolutionPin and manual
check (symbols: readLocalResolutionPin, removeLocalResolutionPin, localPin,
localPinCleared, workspace.id, project.id, context.runtime.cwd,
context.runtime.signal).
- Around line 259-264: The code unconditionally sets localPinCleared = true
after calling removeLocalResolutionPin even though removeLocalResolutionPin
returns a boolean indicating success; update the block that reads localPin (from
readLocalResolutionPin) to capture the return value of removeLocalResolutionPin
and set localPinCleared based on that result (e.g., const removed = await
removeLocalResolutionPin(...); localPinCleared = removed), ensuring the
variables localPin, removeLocalResolutionPin, and localPinCleared reflect actual
removal before nextSteps is computed.
In `@packages/cli/src/lib/app/env-file.ts`:
- Around line 26-35: The current prefix check using cwdRoot fails for root
directories (cwdRoot becomes '//' and breaks the startsWith check); replace that
logic in env-file.ts where cwdRoot/resolvedPath are used by computing
path.relative between cwd and resolvedPath and use that to determine
containment: call path.relative(cwd, resolvedPath) and if the result starts with
'..' or is absolute treat it as outside and throw the existing usageError (same
message/params referencing usageError and the same `filePath`/`resolvedPath`
variables), otherwise consider the path contained. Ensure you reference the same
symbols: cwd, resolvedPath, usageError, and replace the cwdRoot startsWith check
with the path.relative-based check.
In `@packages/cli/src/lib/app/preview-provider.ts`:
- Around line 306-333: The renameBranch function uses a non-null assertion on
result.data.data without verifying it exists; update renameBranch to defensively
check that result.data.data is present (e.g., if (!result.data?.data) { throw
new Error(...); }) before accessing properties, mirror the safe handling used in
createBranch, and return or throw a clear error if the nested data is missing so
you avoid runtime exceptions from result.data.data!.
- Around line 266-285: The createBranch function uses an unsafe non-null
assertion at result.data.data! which can throw if result.data exists but lacks
the nested data field; update createBranch to defensively validate the nested
payload (check result.data.data exists and has id/gitName/role) before using it,
and surface a clear error if missing (e.g., throw an Error indicating malformed
Management API response). Locate the createBranch method and replace the direct
use of result.data.data! with guarded checks on result.data and result.data.data
(and their id/gitName/role) and return only when those fields are present.
- Around line 230-242: deleteProject (and sibling methods
deleteBranch/createBranch/renameBranch) manually cast the client result and
duplicate error/status handling instead of using the shared RawApiErrorBody
shape and helpers; update these methods to treat the response as RawApiErrorBody
(or simply pass the client call result into the shared
apiCallError/domainApiCallError helper) rather than doing a local cast and
manual status ?? 0 handling, so error messages are read via error.error?.message
consistently and avoid duplicated logic by reusing
apiCallError/domainApiCallError.
In `@packages/cli/src/lib/project/local-pin.ts`:
- Around line 87-103: The rm call in removeLocalResolutionPin doesn't pass the
AbortSignal, breaking the file's abort-signal discipline; update the rm
invocation inside removeLocalResolutionPin to pass the provided signal (the same
signal passed to readLocalResolutionPin and other fs ops) so that
rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true }) becomes
rm(..., { force: true, signal }) ensuring consistent abort handling.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79c8a661-9546-4cad-9d3f-c955e78ea1b7
📒 Files selected for processing (16)
docs/product/command-spec.mdpackages/cli/src/adapters/token-storage.tspackages/cli/src/commands/branch/index.tspackages/cli/src/commands/project/index.tspackages/cli/src/controllers/branch.tspackages/cli/src/controllers/project.tspackages/cli/src/lib/app/env-file.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/src/lib/project/local-pin.tspackages/cli/src/presenters/branch.tspackages/cli/src/presenters/project.tspackages/cli/src/shell/command-meta.tspackages/cli/src/types/branch.tspackages/cli/src/types/project.tspackages/cli/tests/branch-controller.test.tspackages/cli/tests/project-controller.test.ts
Comment on lines
+72
to
+80
| process.on("exit", () => { | ||
| if (this.currentRefreshLockId) { | ||
| try { | ||
| fs.unlinkSync(this.lockFilePath); | ||
| } catch { | ||
| // Best-effort cleanup | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Exit handler accumulates without cleanup.
Each FileTokenStorage instance registers a new exit listener in its constructor, but this listener is never removed. When multiple instances are created (as shown in tests with firstStorage and secondStorage), listeners accumulate. This can trigger Node's MaxListenersExceededWarning after 10 instances and keeps each instance referenced indefinitely.
Consider storing the handler reference and removing it when the instance is no longer needed, or use a module-level singleton pattern for the exit handler.
♻️ Proposed fix using module-level handler
+const activeStorages = new Set<FileTokenStorage>();
+let exitHandlerInstalled = false;
+
+function installExitHandler(): void {
+ if (exitHandlerInstalled) return;
+ exitHandlerInstalled = true;
+ process.on("exit", () => {
+ for (const storage of activeStorages) {
+ storage.cleanupLockSync();
+ }
+ });
+}
+
export class FileTokenStorage implements TokenStorage {
private readonly credentialsStore: CredentialsStore;
private readonly lockFilePath: string;
private currentRefreshLockId: string | null = null;
constructor(env: NodeJS.ProcessEnv = process.env, private readonly signal?: AbortSignal) {
const authFilePath = getAuthFilePath(env);
this.credentialsStore = new CredentialsStore(authFilePath);
this.lockFilePath = `${authFilePath}.lock`;
- process.on("exit", () => {
- if (this.currentRefreshLockId) {
- try {
- fs.unlinkSync(this.lockFilePath);
- } catch {
- // Best-effort cleanup
- }
- }
- });
+ activeStorages.add(this);
+ installExitHandler();
+ }
+
+ cleanupLockSync(): void {
+ if (this.currentRefreshLockId) {
+ try {
+ fs.unlinkSync(this.lockFilePath);
+ } catch {
+ // Best-effort cleanup
+ }
+ }
}🤖 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/adapters/token-storage.ts` around lines 72 - 80, The
constructor of FileTokenStorage registers a process.on("exit") listener that
closes the lock file but never removes the listener, causing accumulation;
update FileTokenStorage to either (A) register a single module-level exit
handler that cleans any instance locks (preferable) or (B) store the handler
function returned when adding the listener and expose a dispose/destroy method
on FileTokenStorage that calls process.removeListener("exit", handler) and
clears references; ensure you reference currentRefreshLockId and lockFilePath
inside the centralized handler or removed listener so each instance can be GC'd
when disposed.
Comment on lines
+26
to
+314
| export async function runBranchCreate( | ||
| context: CommandContext, | ||
| branchName: string, | ||
| ): Promise<CommandSuccess<BranchCreateResult>> { | ||
| if (!isRealMode(context)) { | ||
| throw featureUnavailableError( | ||
| "Branch create is not available in fixture mode", | ||
| "Creating Branches requires live platform integration.", | ||
| "Rerun without fixture mode enabled to create a Branch.", | ||
| ["prisma-cli auth login"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| if (!branchName.trim()) { | ||
| throw usageError( | ||
| "Branch create requires a name", | ||
| "The branch name must be a non-empty value.", | ||
| "Pass a branch name explicitly.", | ||
| ["prisma-cli branch create feat-login"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| const authState = await requireAuthenticatedAuthState(context); | ||
| const client = await requireComputeAuth(context.runtime.env, context.runtime.signal); | ||
| if (!client) { | ||
| throw authRequiredError(["prisma-cli auth login"]); | ||
| } | ||
|
|
||
| const workspace = authState.workspace; | ||
| if (!workspace) { | ||
| throw workspaceRequiredError(); | ||
| } | ||
|
|
||
| const target = await resolveProjectTarget({ | ||
| context, | ||
| workspace, | ||
| listProjects: () => listRealWorkspaceProjects(client, workspace, context.runtime.signal), | ||
| prompt: createSelectPromptPort(context), | ||
| remember: true, | ||
| }); | ||
|
|
||
| const provider = createPreviewAppProvider(client); | ||
| const branch = await provider.createBranch({ | ||
| projectId: target.project.id, | ||
| name: branchName.trim(), | ||
| signal: context.runtime.signal, | ||
| }).catch((error) => { | ||
| throw new CliError({ | ||
| code: "BRANCH_CREATE_FAILED", | ||
| domain: "branch", | ||
| summary: `Could not create Branch "${branchName}"`, | ||
| why: error instanceof Error ? error.message : String(error), | ||
| fix: "Retry the command, or check that the Branch does not already exist.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| }); | ||
|
|
||
| return { | ||
| command: "branch.create", | ||
| result: { | ||
| projectId: target.project.id, | ||
| projectName: target.project.name, | ||
| verboseContext: { | ||
| workspace, | ||
| project: target.project, | ||
| resolution: target.resolution, | ||
| }, | ||
| branch: { | ||
| id: branch.id, | ||
| name: branch.name, | ||
| role: branch.role, | ||
| envMap: branch.role, | ||
| }, | ||
| }, | ||
| warnings: [], | ||
| nextSteps: ["prisma-cli app deploy"], | ||
| }; | ||
| } | ||
|
|
||
| export async function runBranchDelete( | ||
| context: CommandContext, | ||
| branchName: string, | ||
| ): Promise<CommandSuccess<BranchDeleteResult>> { | ||
| if (!isRealMode(context)) { | ||
| throw featureUnavailableError( | ||
| "Branch delete is not available in fixture mode", | ||
| "Deleting Branches requires live platform integration.", | ||
| "Rerun without fixture mode enabled to delete a Branch.", | ||
| ["prisma-cli auth login"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| if (!branchName.trim()) { | ||
| throw usageError( | ||
| "Branch delete requires a name", | ||
| "The branch name must be a non-empty value.", | ||
| "Pass a branch name explicitly.", | ||
| ["prisma-cli branch delete feat-login"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| const authState = await requireAuthenticatedAuthState(context); | ||
| const client = await requireComputeAuth(context.runtime.env, context.runtime.signal); | ||
| if (!client) { | ||
| throw authRequiredError(["prisma-cli auth login"]); | ||
| } | ||
|
|
||
| const workspace = authState.workspace; | ||
| if (!workspace) { | ||
| throw workspaceRequiredError(); | ||
| } | ||
|
|
||
| const target = await resolveProjectTarget({ | ||
| context, | ||
| workspace, | ||
| listProjects: () => listRealWorkspaceProjects(client, workspace, context.runtime.signal), | ||
| prompt: createSelectPromptPort(context), | ||
| remember: true, | ||
| }); | ||
|
|
||
| const branches = await listBranches(client, target.project.id, context.runtime.signal); | ||
| const matched = branches.find((b) => b.gitName === branchName.trim()); | ||
| if (!matched) { | ||
| throw new CliError({ | ||
| code: "BRANCH_NOT_FOUND", | ||
| domain: "branch", | ||
| summary: `Branch "${branchName}" not found`, | ||
| why: `The resolved project does not have a Branch named "${branchName}".`, | ||
| fix: "List available Branches and try again with an existing Branch name.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| } | ||
|
|
||
| if (matched.role === "production") { | ||
| throw new CliError({ | ||
| code: "BRANCH_DELETE_FAILED", | ||
| domain: "branch", | ||
| summary: `Cannot delete the production Branch`, | ||
| why: `The Branch "${branchName}" has role "production" and cannot be deleted.`, | ||
| fix: "Production Branches are protected. Promote a different Branch to production first, or use the platform dashboard.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| } | ||
|
|
||
| const provider = createPreviewAppProvider(client); | ||
| await provider.deleteBranch({ | ||
| projectId: target.project.id, | ||
| branchId: matched.id, | ||
| signal: context.runtime.signal, | ||
| }).catch((error) => { | ||
| throw new CliError({ | ||
| code: "BRANCH_DELETE_FAILED", | ||
| domain: "branch", | ||
| summary: `Could not delete Branch "${branchName}"`, | ||
| why: error instanceof Error ? error.message : String(error), | ||
| fix: "Retry the command.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| }); | ||
|
|
||
| return { | ||
| command: "branch.delete", | ||
| result: { | ||
| projectId: target.project.id, | ||
| projectName: target.project.name, | ||
| branchName: branchName.trim(), | ||
| }, | ||
| warnings: [], | ||
| nextSteps: [], | ||
| }; | ||
| } | ||
|
|
||
| export async function runBranchRename( | ||
| context: CommandContext, | ||
| oldName: string, | ||
| newName: string, | ||
| ): Promise<CommandSuccess<BranchRenameResult>> { | ||
| if (!isRealMode(context)) { | ||
| throw featureUnavailableError( | ||
| "Branch rename is not available in fixture mode", | ||
| "Renaming Branches requires live platform integration.", | ||
| "Rerun without fixture mode enabled to rename a Branch.", | ||
| ["prisma-cli auth login"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| if (!oldName.trim() || !newName.trim()) { | ||
| throw usageError( | ||
| "Branch rename requires old and new names", | ||
| "Both the old branch name and new branch name must be non-empty.", | ||
| "Pass the old and new branch names.", | ||
| ["prisma-cli branch rename feat-login feat-auth"], | ||
| "branch", | ||
| ); | ||
| } | ||
|
|
||
| const authState = await requireAuthenticatedAuthState(context); | ||
| const client = await requireComputeAuth(context.runtime.env, context.runtime.signal); | ||
| if (!client) { | ||
| throw authRequiredError(["prisma-cli auth login"]); | ||
| } | ||
|
|
||
| const workspace = authState.workspace; | ||
| if (!workspace) { | ||
| throw workspaceRequiredError(); | ||
| } | ||
|
|
||
| const target = await resolveProjectTarget({ | ||
| context, | ||
| workspace, | ||
| listProjects: () => listRealWorkspaceProjects(client, workspace, context.runtime.signal), | ||
| prompt: createSelectPromptPort(context), | ||
| remember: true, | ||
| }); | ||
|
|
||
| const branches = await listBranches(client, target.project.id, context.runtime.signal); | ||
| const matched = branches.find((b) => b.gitName === oldName.trim()); | ||
| if (!matched) { | ||
| throw new CliError({ | ||
| code: "BRANCH_NOT_FOUND", | ||
| domain: "branch", | ||
| summary: `Branch "${oldName}" not found`, | ||
| why: `The resolved project does not have a Branch named "${oldName}".`, | ||
| fix: "List available Branches and try again with an existing Branch name.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| } | ||
|
|
||
| if (matched.role === "production") { | ||
| throw new CliError({ | ||
| code: "BRANCH_RENAME_FAILED", | ||
| domain: "branch", | ||
| summary: `Cannot rename the production Branch`, | ||
| why: `The Branch "${oldName}" has role "production" and cannot be renamed.`, | ||
| fix: "Production Branches are protected and cannot be renamed.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| } | ||
|
|
||
| const provider = createPreviewAppProvider(client); | ||
| const branch = await provider.renameBranch({ | ||
| projectId: target.project.id, | ||
| branchId: matched.id, | ||
| newName: newName.trim(), | ||
| signal: context.runtime.signal, | ||
| }).catch((error) => { | ||
| throw new CliError({ | ||
| code: "BRANCH_RENAME_FAILED", | ||
| domain: "branch", | ||
| summary: `Could not rename Branch "${oldName}" to "${newName}"`, | ||
| why: error instanceof Error ? error.message : String(error), | ||
| fix: "Retry the command, or check that the new Branch name does not already exist.", | ||
| exitCode: 1, | ||
| nextSteps: ["prisma-cli branch list"], | ||
| }); | ||
| }); | ||
|
|
||
| return { | ||
| command: "branch.rename", | ||
| result: { | ||
| projectId: target.project.id, | ||
| projectName: target.project.name, | ||
| verboseContext: { | ||
| workspace, | ||
| project: target.project, | ||
| resolution: target.resolution, | ||
| }, | ||
| branch: { | ||
| id: branch.id, | ||
| name: branch.name, | ||
| role: branch.role, | ||
| envMap: branch.role, | ||
| }, | ||
| }, | ||
| warnings: [], | ||
| nextSteps: [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider extracting common boilerplate into a shared helper.
All three new controller functions (runBranchCreate, runBranchDelete, runBranchRename) repeat nearly identical boilerplate (~30 lines each) for:
- Fixture mode validation
- Input validation
- Auth/workspace checks
- Project target resolution
Extracting this into a shared helper (e.g., prepareBranchOperation) that returns { client, workspace, target } would reduce duplication, make future changes to auth/validation logic easier to apply consistently, and reduce the risk of divergent implementations.
♻️ Example refactor approach
async function prepareBranchOperation(
context: CommandContext,
commandName: string,
): Promise<{ client: ManagementApiClient; workspace: AuthWorkspace; target: ResolvedProjectTarget }> {
if (!isRealMode(context)) {
throw featureUnavailableError(
`${commandName} is not available in fixture mode`,
`${commandName} requires live platform integration.`,
`Rerun without fixture mode enabled to ${commandName.toLowerCase()}.`,
["prisma-cli auth login"],
"branch",
);
}
const authState = await requireAuthenticatedAuthState(context);
const client = await requireComputeAuth(context.runtime.env, context.runtime.signal);
if (!client) {
throw authRequiredError(["prisma-cli auth login"]);
}
const workspace = authState.workspace;
if (!workspace) {
throw workspaceRequiredError();
}
const target = await resolveProjectTarget({
context,
workspace,
listProjects: () => listRealWorkspaceProjects(client, workspace, context.runtime.signal),
prompt: createSelectPromptPort(context),
remember: true,
});
return { client, workspace, target };
}Then each operation validates its specific inputs and calls:
const { client, workspace, target } = await prepareBranchOperation(context, "Branch create");🤖 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/branch.ts` around lines 26 - 314, The three
functions runBranchCreate, runBranchDelete, and runBranchRename duplicate the
same fixture check, auth/workspace retrieval, client acquisition, and
resolveProjectTarget calls; extract that shared boilerplate into a helper (e.g.,
prepareBranchOperation) that accepts (context, commandName) and returns {
client, workspace, target }, update each runBranch* to perform only
input-specific validation and call prepareBranchOperation to get
client/workspace/target, and keep existing error types/behavior
(featureUnavailableError, requireAuthenticatedAuthState, requireComputeAuth,
workspaceRequiredError, resolveProjectTarget, listRealWorkspaceProjects,
createSelectPromptPort) so callers and subsequent provider calls remain
unchanged.
Comment on lines
+259
to
+262
| const localPin = await readLocalResolutionPin(context.runtime.cwd, context.runtime.signal); | ||
| let localPinCleared = false; | ||
| if (localPin.kind === "present" && localPin.pin.projectId === project.id) { | ||
| await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider simplifying local pin removal to avoid redundant read.
Lines 259-262 read the local pin, check if projectId matches, then call removeLocalResolutionPin which re-reads the pin to verify both workspaceId and projectId. You can simplify this to a single call:
const localPinCleared = await removeLocalResolutionPin(
context.runtime.cwd,
{ workspaceId: workspace.id, projectId: project.id },
context.runtime.signal
);This reduces filesystem I/O while maintaining the same safety guarantees (the function re-reads to prevent TOCTOU races).
🤖 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/project.ts` around lines 259 - 262, The code
currently calls readLocalResolutionPin to check localPin before calling
removeLocalResolutionPin, causing redundant filesystem I/O; replace that
read+check with a single call to removeLocalResolutionPin passing the expected
pin ({ workspaceId: workspace.id, projectId: project.id }) so
removeLocalResolutionPin itself re-reads and validates to prevent TOCTOU; update
the localPinCleared variable to capture the boolean result from
removeLocalResolutionPin and remove the prior readLocalResolutionPin and manual
check (symbols: readLocalResolutionPin, removeLocalResolutionPin, localPin,
localPinCleared, workspace.id, project.id, context.runtime.cwd,
context.runtime.signal).
Comment on lines
+259
to
+264
| const localPin = await readLocalResolutionPin(context.runtime.cwd, context.runtime.signal); | ||
| let localPinCleared = false; | ||
| if (localPin.kind === "present" && localPin.pin.projectId === project.id) { | ||
| await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal); | ||
| localPinCleared = true; | ||
| } |
There was a problem hiding this comment.
Check return value of removeLocalResolutionPin to determine if pin was cleared.
Line 262 calls removeLocalResolutionPin but doesn't check the return value (which is true if the pin was removed, false if it wasn't). Line 263 unconditionally sets localPinCleared = true, even if the function returned false (e.g., when the workspaceId in the pin doesn't match). This causes incorrect nextSteps on line 273.
🐛 Proposed fix
const localPin = await readLocalResolutionPin(context.runtime.cwd, context.runtime.signal);
let localPinCleared = false;
if (localPin.kind === "present" && localPin.pin.projectId === project.id) {
- await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal);
- localPinCleared = true;
+ localPinCleared = await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const localPin = await readLocalResolutionPin(context.runtime.cwd, context.runtime.signal); | |
| let localPinCleared = false; | |
| if (localPin.kind === "present" && localPin.pin.projectId === project.id) { | |
| await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal); | |
| localPinCleared = true; | |
| } | |
| const localPin = await readLocalResolutionPin(context.runtime.cwd, context.runtime.signal); | |
| let localPinCleared = false; | |
| if (localPin.kind === "present" && localPin.pin.projectId === project.id) { | |
| localPinCleared = await removeLocalResolutionPin(context.runtime.cwd, localPin.pin, context.runtime.signal); | |
| } |
🤖 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/project.ts` around lines 259 - 264, The code
unconditionally sets localPinCleared = true after calling
removeLocalResolutionPin even though removeLocalResolutionPin returns a boolean
indicating success; update the block that reads localPin (from
readLocalResolutionPin) to capture the return value of removeLocalResolutionPin
and set localPinCleared based on that result (e.g., const removed = await
removeLocalResolutionPin(...); localPinCleared = removed), ensuring the
variables localPin, removeLocalResolutionPin, and localPinCleared reflect actual
removal before nextSteps is computed.
Comment on lines
+26
to
+35
| const cwdRoot = path.resolve(cwd) + path.sep; | ||
| if (!resolvedPath.startsWith(cwdRoot)) { | ||
| throw usageError( | ||
| `Env file path escapes the current directory`, | ||
| `"${filePath}" resolves to "${resolvedPath}" which is outside of "${cwd}".`, | ||
| "Pass a dotenv file path that is contained within the current working directory.", | ||
| [`prisma-cli project env ${command} --file .env --role preview`], | ||
| "app", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Path check fails when cwd is the filesystem root.
When cwd is / (or C:\ on Windows), path.resolve(cwd) returns /, so cwdRoot becomes //. Since no resolved path starts with //, all legitimate files are incorrectly rejected.
Using path.relative() handles this and other edge cases more robustly:
🐛 Proposed fix using path.relative()
const resolvedPath = path.resolve(cwd, filePath);
- const cwdRoot = path.resolve(cwd) + path.sep;
- if (!resolvedPath.startsWith(cwdRoot)) {
+ const normalizedCwd = path.resolve(cwd);
+ const relative = path.relative(normalizedCwd, resolvedPath);
+ if (relative.startsWith("..") || path.isAbsolute(relative)) {
throw usageError(
`Env file path escapes the current directory`,
`"${filePath}" resolves to "${resolvedPath}" which is outside of "${cwd}".`,
"Pass a dotenv file path that is contained within the current working directory.",
[`prisma-cli project env ${command} --file .env --role preview`],
"app",
);
}🤖 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/lib/app/env-file.ts` around lines 26 - 35, The current
prefix check using cwdRoot fails for root directories (cwdRoot becomes '//' and
breaks the startsWith check); replace that logic in env-file.ts where
cwdRoot/resolvedPath are used by computing path.relative between cwd and
resolvedPath and use that to determine containment: call path.relative(cwd,
resolvedPath) and if the result starts with '..' or is absolute treat it as
outside and throw the existing usageError (same message/params referencing
usageError and the same `filePath`/`resolvedPath` variables), otherwise consider
the path contained. Ensure you reference the same symbols: cwd, resolvedPath,
usageError, and replace the cwdRoot startsWith check with the
path.relative-based check.
Comment on lines
+230
to
+242
| async deleteProject(options) { | ||
| const result = await (client as unknown as { DELETE: Function }).DELETE("/v1/projects/{projectId}", { | ||
| params: { path: { projectId: options.projectId } }, | ||
| signal: options.signal, | ||
| }) as { error?: { error?: { message?: string } }; response?: { status?: number } }; | ||
| if (result.error) { | ||
| const status = result.response?.status ?? 0; | ||
| if (status === 404) { | ||
| throw new Error("Project Not Found"); | ||
| } | ||
| throw new Error(result.error.error?.message ?? `Management API returned HTTP ${status}.`); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify usage pattern of similar Management API DELETE calls
# Search for other DELETE endpoint calls to compare error handling patterns
rg -nP 'DELETE.*v1/(projects|branches|domains)' --type=ts -A5 -B2Repository: prisma/prisma-cli
Length of output: 2158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/lib/app/preview-provider.ts"
# Show the relevant implementations around deleteBranch/removeDomain (include error handling)
sed -n '260,360p' "$FILE"
echo "-----"
sed -n '400,520p' "$FILE"
echo "-----"
# Broader search for DELETE calls hitting /v1 and any inline error/response usage patterns
rg -n 'DELETE\("/v1/' --type=ts -S -g'*.ts'
echo "-----"
rg -n 'response\?\.\s*status|error\.error\?\.\s*message|Management API returned HTTP' --type=ts -S -g'*.ts'Repository: prisma/prisma-cli
Length of output: 10468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/lib/app/preview-provider.ts"
sed -n '200,270p' "$FILE"
echo "-----"
# Show helper(s) near the referenced line numbers
sed -n '980,1060p' "$FILE"
echo "-----"
# Show domainApiCallError if it's nearby
rg -n "function domainApiCallError|domainApiCallError" --type=ts "$FILE"Repository: prisma/prisma-cli
Length of output: 5316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/lib/app/preview-provider.ts"
# Find the type definitions for the error/response used by apiCallError/domainApiCallError
rg -n "type RawApiErrorBody|interface RawApiErrorBody|export type RawApiErrorBody|RawApiErrorBody" --type=ts "$FILE"
rg -n "type Response|interface Response|export type Response|Response" --type=ts "$FILE" | head -n 50
# Also locate any imports that define these types
rg -n "^import .*RawApiErrorBody|^import .*Response" "$FILE"Repository: prisma/prisma-cli
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/cli/src/lib/app/preview-provider.ts"
sed -n '700,770p' "$FILE"
echo "-----"
# show top imports / type declarations where Response might be imported or aliased
sed -n '1,120p' "$FILE"
echo "-----"
# show where Response type is declared if it's in-file (after imports)
rg -n "type Response|interface Response|import .*Response" "$FILE"Repository: prisma/prisma-cli
Length of output: 4539
DELETE /v1/projects/{projectId}: assumed error shape aligns with existing types/usage; optional dedupe
deleteProject (and deleteBranch/createBranch/renameBranch) consistently treats the client error as RawApiErrorBody and reads result.error.error?.message, which matches RawApiErrorBody and how shared helpers (apiCallError, domainApiCallError) format error.error?.message. Optional: reuse apiCallError to avoid duplicating the manual result cast and status ?? 0 handling.
🤖 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/lib/app/preview-provider.ts` around lines 230 - 242,
deleteProject (and sibling methods deleteBranch/createBranch/renameBranch)
manually cast the client result and duplicate error/status handling instead of
using the shared RawApiErrorBody shape and helpers; update these methods to
treat the response as RawApiErrorBody (or simply pass the client call result
into the shared apiCallError/domainApiCallError helper) rather than doing a
local cast and manual status ?? 0 handling, so error messages are read via
error.error?.message consistently and avoid duplicated logic by reusing
apiCallError/domainApiCallError.
Comment on lines
+266
to
+285
| async createBranch(options) { | ||
| const result = await (client as unknown as { POST: Function }).POST("/v1/projects/{projectId}/branches", { | ||
| params: { path: { projectId: options.projectId } }, | ||
| body: { gitName: options.name }, | ||
| signal: options.signal, | ||
| }) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } }; | ||
| if (result.error || !result.data) { | ||
| const status = result.response?.status ?? 0; | ||
| if (status === 409) { | ||
| throw new Error(`Branch "${options.name}" already exists.`); | ||
| } | ||
| throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`); | ||
| } | ||
| const branch = result.data.data!; | ||
| return { | ||
| id: branch.id, | ||
| name: branch.gitName, | ||
| role: branch.role, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Unsafe non-null assertion on API response data.
Line 279 uses result.data.data! after only checking !result.data on line 272. If the Management API returns { data: {} } without the nested data field, this will throw at runtime.
🛡️ Proposed fix
}) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } };
if (result.error || !result.data) {
const status = result.response?.status ?? 0;
if (status === 409) {
throw new Error(`Branch "${options.name}" already exists.`);
}
throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`);
}
- const branch = result.data.data!;
+ if (!result.data.data) {
+ throw new Error("Management API returned invalid response structure.");
+ }
+ const branch = result.data.data;
return {🤖 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/lib/app/preview-provider.ts` around lines 266 - 285, The
createBranch function uses an unsafe non-null assertion at result.data.data!
which can throw if result.data exists but lacks the nested data field; update
createBranch to defensively validate the nested payload (check result.data.data
exists and has id/gitName/role) before using it, and surface a clear error if
missing (e.g., throw an Error indicating malformed Management API response).
Locate the createBranch method and replace the direct use of result.data.data!
with guarded checks on result.data and result.data.data (and their
id/gitName/role) and return only when those fields are present.
Comment on lines
+306
to
+333
| async renameBranch(options) { | ||
| const result = await (client as unknown as { PATCH: Function }).PATCH("/v1/projects/{projectId}/branches/{branchId}", { | ||
| params: { | ||
| path: { | ||
| projectId: options.projectId, | ||
| branchId: options.branchId, | ||
| }, | ||
| }, | ||
| body: { gitName: options.newName }, | ||
| signal: options.signal, | ||
| }) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } }; | ||
| if (result.error || !result.data) { | ||
| const status = result.response?.status ?? 0; | ||
| if (status === 404) { | ||
| throw new Error("Branch Not Found"); | ||
| } | ||
| if (status === 409) { | ||
| throw new Error(`Branch "${options.newName}" already exists.`); | ||
| } | ||
| throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`); | ||
| } | ||
| const branch = result.data.data!; | ||
| return { | ||
| id: branch.id, | ||
| name: branch.gitName, | ||
| role: branch.role, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Unsafe non-null assertion on API response data.
Line 327 uses result.data.data! after only checking !result.data on line 317. This has the same runtime safety issue as createBranch.
🛡️ Proposed fix
}) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } };
if (result.error || !result.data) {
const status = result.response?.status ?? 0;
if (status === 404) {
throw new Error("Branch Not Found");
}
if (status === 409) {
throw new Error(`Branch "${options.newName}" already exists.`);
}
throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`);
}
- const branch = result.data.data!;
+ if (!result.data.data) {
+ throw new Error("Management API returned invalid response structure.");
+ }
+ const branch = result.data.data;
return {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async renameBranch(options) { | |
| const result = await (client as unknown as { PATCH: Function }).PATCH("/v1/projects/{projectId}/branches/{branchId}", { | |
| params: { | |
| path: { | |
| projectId: options.projectId, | |
| branchId: options.branchId, | |
| }, | |
| }, | |
| body: { gitName: options.newName }, | |
| signal: options.signal, | |
| }) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } }; | |
| if (result.error || !result.data) { | |
| const status = result.response?.status ?? 0; | |
| if (status === 404) { | |
| throw new Error("Branch Not Found"); | |
| } | |
| if (status === 409) { | |
| throw new Error(`Branch "${options.newName}" already exists.`); | |
| } | |
| throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`); | |
| } | |
| const branch = result.data.data!; | |
| return { | |
| id: branch.id, | |
| name: branch.gitName, | |
| role: branch.role, | |
| }; | |
| }, | |
| async renameBranch(options) { | |
| const result = await (client as unknown as { PATCH: Function }).PATCH("/v1/projects/{projectId}/branches/{branchId}", { | |
| params: { | |
| path: { | |
| projectId: options.projectId, | |
| branchId: options.branchId, | |
| }, | |
| }, | |
| body: { gitName: options.newName }, | |
| signal: options.signal, | |
| }) as { error?: { error?: { message?: string } }; data?: { data?: { id: string; gitName: string; role: BranchKind } }; response?: { status?: number } }; | |
| if (result.error || !result.data) { | |
| const status = result.response?.status ?? 0; | |
| if (status === 404) { | |
| throw new Error("Branch Not Found"); | |
| } | |
| if (status === 409) { | |
| throw new Error(`Branch "${options.newName}" already exists.`); | |
| } | |
| throw new Error(result.error?.error?.message ?? `Management API returned HTTP ${status}.`); | |
| } | |
| if (!result.data.data) { | |
| throw new Error("Management API returned invalid response structure."); | |
| } | |
| const branch = result.data.data; | |
| return { | |
| id: branch.id, | |
| name: branch.gitName, | |
| role: branch.role, | |
| }; | |
| }, |
🤖 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/lib/app/preview-provider.ts` around lines 306 - 333, The
renameBranch function uses a non-null assertion on result.data.data without
verifying it exists; update renameBranch to defensively check that
result.data.data is present (e.g., if (!result.data?.data) { throw new
Error(...); }) before accessing properties, mirror the safe handling used in
createBranch, and return or throw a clear error if the nested data is missing so
you avoid runtime exceptions from result.data.data!.
Comment on lines
+87
to
+103
| export async function removeLocalResolutionPin( | ||
| cwd: string, | ||
| pin: LocalResolutionPin, | ||
| signal?: AbortSignal, | ||
| ): Promise<boolean> { | ||
| const existing = await readLocalResolutionPin(cwd, signal); | ||
| if ( | ||
| existing.kind === "present" && | ||
| existing.pin.workspaceId === pin.workspaceId && | ||
| existing.pin.projectId === pin.projectId | ||
| ) { | ||
| signal?.throwIfAborted(); | ||
| await rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true }); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Pass signal to rm for consistent abort handling.
Line 99 calls rm without passing the signal parameter, but Node.js fs/promises.rm accepts signal (similar to other operations in this file like writeFile on line 51 and readFile on line 19). This is inconsistent with the file's abort-signal discipline.
⚡ Proposed fix
) {
signal?.throwIfAborted();
- await rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true });
+ await rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true, signal });
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function removeLocalResolutionPin( | |
| cwd: string, | |
| pin: LocalResolutionPin, | |
| signal?: AbortSignal, | |
| ): Promise<boolean> { | |
| const existing = await readLocalResolutionPin(cwd, signal); | |
| if ( | |
| existing.kind === "present" && | |
| existing.pin.workspaceId === pin.workspaceId && | |
| existing.pin.projectId === pin.projectId | |
| ) { | |
| signal?.throwIfAborted(); | |
| await rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true }); | |
| return true; | |
| } | |
| return false; | |
| } | |
| export async function removeLocalResolutionPin( | |
| cwd: string, | |
| pin: LocalResolutionPin, | |
| signal?: AbortSignal, | |
| ): Promise<boolean> { | |
| const existing = await readLocalResolutionPin(cwd, signal); | |
| if ( | |
| existing.kind === "present" && | |
| existing.pin.workspaceId === pin.workspaceId && | |
| existing.pin.projectId === pin.projectId | |
| ) { | |
| signal?.throwIfAborted(); | |
| await rm(path.join(cwd, LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true, signal }); | |
| return true; | |
| } | |
| return false; | |
| } |
🤖 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/lib/project/local-pin.ts` around lines 87 - 103, The rm call
in removeLocalResolutionPin doesn't pass the AbortSignal, breaking the file's
abort-signal discipline; update the rm invocation inside
removeLocalResolutionPin to pass the provided signal (the same signal passed to
readLocalResolutionPin and other fs ops) so that rm(path.join(cwd,
LOCAL_RESOLUTION_PIN_RELATIVE_PATH), { force: true }) becomes rm(..., { force:
true, signal }) ensuring consistent abort handling.
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
1 participant
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.
No description provided.