functions:kits:uninstall but not terrible by Berlioz · Pull Request #10981 · firebase/firebase-tools

Berlioz · 2026-08-24T22:49:47Z

Files changed:
src/config.ts: Extends some existing helper functions for filesystem operations with project root relative paths to also have options for directory-level operations
src/commands/functions-kits-uninstall.ts‎: Implements the actual command
src/commands/index.ts: Registers the command with the CLI, gated by the kits experiment

Scenarios tested:
Instance deletion (not last instance), instance deletion (last instance triggering kit deletion), direct kit deletion.
Conservative filesystem case tested by manually modifying nonstandardKitLayout() since directory kits don't exist yet

UX example:

vsfan-mac:kits-test vsfan$ firebaselocal functions:kits:uninstall --kit example-firebase-kit-hello-world
✔ Uninstall will delete all active instances of this kit. You have the following active instances that will be deleted:
asdf,jkl
Do you want to continue with uninstall (y/N)? Yes
✔ You are about to delete the following Cloud Functions:
    asdf:kit-asdf-helloWorld(us-central1)
    asdf:kit-asdf-helloWorldCallable(us-central1)
  Are you sure? Yes
i  functions: deleting Node.js 24 (2nd Gen) function asdf:kit-asdf-helloWorldCallable(us-central1)...
i  functions: deleting Node.js 24 (2nd Gen) function asdf:kit-asdf-helloWorld(us-central1)...
✔  functions[asdf:kit-asdf-helloWorldCallable(us-central1)] Successful delete operation.
✔  functions[asdf:kit-asdf-helloWorld(us-central1)] Successful delete operation.
⚠  functions: Kits instance config directory function-kits/example-firebase-kit-hello-world/config-asdf still contains files (bar,bar.json,baz,foo,foo.txt, ...4 more); not deleting automatically
✔  Wrote configuration info to firebase.json
✔ You are about to delete the following Cloud Functions:
    jkl:kit-jkl-helloWorld(us-east1)
    jkl:kit-jkl-helloWorldCallable(us-east1)
  Are you sure? Yes
i  functions: deleting Node.js 24 (2nd Gen) function jkl:kit-jkl-helloWorldCallable(us-east1)...
i  functions: deleting Node.js 24 (2nd Gen) function jkl:kit-jkl-helloWorld(us-east1)...
✔  functions[jkl:kit-jkl-helloWorld(us-east1)] Successful delete operation.
✔  functions[jkl:kit-jkl-helloWorldCallable(us-east1)] Successful delete operation.
✔  Wrote configuration info to firebase.json
✔  Wrote configuration info to firebase.json

Stuff currently slated for fast-follow unless someone makes me do it in this PR:

What about .env.alias
Moving shared code between this and functions:delete into a shared helper
What about kits/instances that were never deployed, or whose cloud resources are in a bad state

wiz-9635d3485b · 2026-08-24T22:50:21Z

Wiz Scan Summary

Scanner	Findings
Vulnerabilities	-
Sensitive Data	-
Secrets	-
IaC Misconfigurations	-
SAST Findings	2 5
Software Management Findings	-

Total	2 5

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio.

gemini-code-assist

Code Review

This pull request introduces the functions:kits:uninstall command to allow users to uninstall a function kit or kit instance from their Firebase project, along with adding directory-handling helper methods to the Config class. The review feedback is highly constructive and identifies several critical areas for improvement: preventing potential ENOENT crashes by checking directory existence before file operations, handling non-interactive environments when prompting for multiple projects, making the --kit and --instance options mutually exclusive, and ensuring local configurations are preserved if cloud deletion fails. Additionally, the feedback highlights style guide violations regarding strict null checks, unsafe non-null assertions, and the use of the any type.

ajperel

You've brought up a bunch of interesting edge cases around what to delete on disk. This probably maybe should have started as a more detailed doc or discussion but we can hash it out. It was more complicated that I realized.

ajperel · 2026-08-25T04:18:47Z

+
+/*
+ * For each .env.<projectId> file present in a Kit instance config folder, destroy the
+ * Function (project = input, region = env.FIREBASE_FUNCTION_KIT_REGION, id = kitInstanceId)


Do we need region? Is it possible to deploy a kit instance to two different regions in the same project? I'm honestly not sure.

Not anymore. That's a relic from when I thought I had to implement all the deletion logic myself instead of taking advantage of the codebase.

ajperel · 2026-08-25T04:26:48Z

+    const projectId = fileName.replace(new RegExp("^.env."), "");
+    await uninstallProjectInstance(options, config, projectId, instanceId, kitInstancePath);
+  }
+  config.deleteProjectDir(kitInstancePath);


You're careful to filter out files that aren't .env files ...honestly hadn't thought about anyone creating those. If they have do you think we should still delete this directory? warn? error?

ajperel · 2026-08-25T04:29:24Z

+  instanceId: string,
+  kitInstancePath: string,
+): Promise<void> {
+  const envFilePath = join(kitInstancePath, `.env.${projectId}`);


What if folks are using an alias?

I am OK if we say handling aliases is a follow up PR FWIW

ajperel · 2026-08-25T04:31:31Z

+    projectId: projectId,
+    filters: [{ codebase: instanceId } as EndpointFilter],
+  };
+  const haveBackend = await backend.existingBackend(context);


You are duplicating what functions:delete does here. Is there a reason not to refactor that command so you can re-use it here?

In the interests of time I could accept this, but I'd like us to unify and reduce tech debt in the near future.

Not really needing the codebase disambiguation logic, I guess, and slightly different semantics around what messages we want to print to the user and what happens if some but not all of the deletion operations fail. Could definitely work around it, if time is available.

ajperel

More comments from tools repo review skill

ajperel · 2026-08-25T04:37:44Z

    fs.removeSync(this.path(p));
  }

+  deleteProjectDir(p: string) {


🔴 [Critical Safety] Insufficient Directory Deletion Protection

Rationale: deleteProjectDir checks for /.. but does not prevent deleting the project directory itself (if passed . or resolving to project root) or arbitrary directories if passed absolute paths.

Suggested Fix:

deleteProjectDir(p: string) { if (p.includes("/..")) { throw new FirebaseError("sanity: refusing to delete project-relative dir containing '/..'"); } const resolvedPath = path.resolve(this.path(p)); const resolvedProjectDir = path.resolve(this.projectDir); if (!resolvedPath.startsWith(resolvedProjectDir + path.sep)) { throw new FirebaseError("sanity: refusing to delete directory outside of project directory"); } // ... rest of the method

I don't think this correct because of the guards in the config.path() helper itself, but the method should definitely reject any absolute paths.

...Also, is it necessarily wrong that this might be called on project root?

ajperel · 2026-08-25T04:37:44Z

+  if (instanceConfigDirPath === "") {
+    throw new FirebaseError(`Instance ID ${instanceId} not found in firebase.json`);
+  }
+  kitForInstance = kitForInstance!;


🟡 Nit: Avoid Non-Null Assertion

Rationale: kitForInstance = kitForInstance! can be avoided by checking if it is undefined after the loop.

Suggested Fix:

let kitForInstance: ValidatedKitSingle | undefined; for (const kitConfig of kits) { if (kitConfig.instances[instanceId]) { kitForInstance = kitConfig; instanceConfigDirPath = kitConfig.instances[instanceId]; break; } } if (!kitForInstance) { throw new FirebaseError(`Instance ID ${instanceId} not found in firebase.json`); }

The .find() idea was, frankly, a lot better than this.

ajperel · 2026-08-25T04:37:44Z

+import { Config } from "../config";
+import { listKitConfigs } from "../functions/kits/config";
+import { Options } from "../options";
+import { join } from "path";


🟡 Nit: Inconsistent Path Imports

Rationale: Mixing path and path/posix imports can be confusing.

Suggested Fix: Use namespace import import * as path from "path" and access path.join and path.posix.dirname explicitly.

ajperel

Also good to follow the contribution guidelines on PR descriptions, etc.

wandamora · 2026-08-26T00:33:17Z

+// If this is the case, blind deletions of directories associate with this kit could delete things the user did not intend to.
+function nonstandardKitLayout(kitConfig: ValidatedKitSingle): boolean {
+  if (kitConfig.sourcePackage) {
+    if (kitConfig.source !== `function-kits/${kitConfig.kit}/source`) {


nit: could leverage FUNCTION_KITS_DIR https://github.com/firebase/firebase-tools/blob/main/src/functions/kits/install.ts#L40 to keep the kits directory consistent if it changes. maybe we could move that constant to a common.ts file. Makes me wonder if we should put these common file pattern paths in a constant as well. Ditto for configDir.

wandamora · 2026-08-26T00:45:05Z

+    if (!fileName.startsWith(".env.")) {
+      continue;
+    }
+    projectsWithConfigs.push(fileName.slice(5));


took me a sec to realize "5" is the char count for .env., can you write a short comment about why that value?

Berlioz added 7 commits August 21, 2026 15:47

functions:kits:uninstall

1ad9ada

lint

fb3999c

slop

5550f7b

add to cli

b16dd8f

logging

d84ac6a

Merge branch 'main' into vsfan_kits_uninstall

4cfa3e8

oh no

4528ea5

Berlioz requested review from ajperel and wandamora August 24, 2026 22:49

Berlioz assigned ajperel and wandamora Aug 24, 2026

gemini-code-assist Bot reviewed Aug 24, 2026

View reviewed changes

style

9f124ee

ajperel reviewed Aug 25, 2026

View reviewed changes

Berlioz added 2 commits August 24, 2026 22:22

low-hanging fruit

1396e92

changes for single-project delete, conservative filesystem changes

ffe567d

wandamora approved these changes Aug 26, 2026

View reviewed changes

Uh oh!

Conversation

Berlioz commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wiz-9635d3485b Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Wiz Scan Summary

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajperel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajperel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajperel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Berlioz commented Aug 24, 2026 •

edited

Loading

wiz-9635d3485b Bot commented Aug 24, 2026 •

edited

Loading