Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add support for targeting named Firestore databases via: - FIRESTORE_DATABASE_ID env var as a server-wide default - Per-tool databaseId parameter on all Firestore tools Resolution priority: explicit databaseId arg → env var → (default). Adds getFirestoreInstance() helper that resolves the database ID and returns the appropriate Firestore instance while reusing the single Firebase app. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lists all Firestore databases in the project via the Firestore Admin REST API using the existing service account credentials. Useful for discovering available database IDs to pass to the databaseId parameter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds multi-database Firestore support: new Changes
Sequence DiagramsequenceDiagram
participant Client as "Client"
participant MCP as "MCP Server"
participant Handler as "Tool Handler"
participant Resolver as "getFirestoreInstance"
participant Admin as "Firebase Admin SDK"
participant DB as "Firestore DB"
Client->>MCP: Invoke Firestore tool (optional databaseId)
MCP->>Handler: Route request
Handler->>Resolver: resolve(databaseId?)
Resolver->>Resolver: choose param -> config.firestoreDatabaseId -> "(default)"
Resolver->>Admin: getFirestore(resolvedId)
Admin->>DB: return Firestore instance
Resolver-->>Handler: Firestore instance
Handler->>DB: perform operation (doc/collection/query)
DB-->>Handler: operation result
Handler-->>MCP: formatted tool response
MCP-->>Client: return result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 72-75: The new .claude ignore entry has an extra blank line before
it; remove one of the blank lines so the `.claude/settings.local.json` entry is
separated from the previous section by a single blank line (matching the
existing section spacing used elsewhere), ensuring consistency in the .gitignore
formatting.
In `@CLAUDE.md`:
- Around line 27-33: The README line in CLAUDE.md incorrectly states "Registers
12 MCP tools"; update the documentation to reflect the current code in
src/index.ts where FirebaseMcpServer (and its setupToolHandlers) now registers
13 tools (including the newly added firestore_list_databases). Edit the bullet
under "Server Core (`src/index.ts`)" to say "Registers 13 MCP tools" so the doc
matches the FirebaseMcpServer/setupToolHandlers implementation.
In `@src/__tests__/multi-db.test.ts`:
- Around line 109-110: The fake app in setupModule does not provide the fields
the new firestore_list_databases handler expects (app.options.projectId and
app.options.credential.getAccessToken), so update the fakeApp created in
setupModule to include an options object with a projectId string and a
credential object that implements getAccessToken (returning a Promise or object
with access_token and expiry_date as the real credential does). Locate the
fakeApp inside the setupModule helper in src/__tests__/multi-db.test.ts and add
options: { projectId: 'test-project', credential: { getAccessToken: async () =>
({ access_token: 'fake-token', expiry_date: Date.now() + 3600 * 1000 }) } } so
the happy-path tests exercise the new code path.
In `@src/index.ts`:
- Around line 1601-1604: The Firestore Admin API fetch (building `url` and
calling `fetch` with `Authorization: Bearer ${tokenResult.access_token}`) has no
timeout/abort handling; wrap this call with an AbortController and a setTimeout
that calls controller.abort() after a reasonable ms (e.g. 5s), pass
controller.signal into fetch, and clear the timeout on success; update the
surrounding tool handler to catch the abort error (or a DOMException with name
"AbortError") and return/throw a bounded timeout error instead of letting the
request hang.
- Around line 1585-1589: The projectId resolution in firestore_list_databases is
using credential.getAccessToken() and app.options.projectId which can be empty
for service-account initialization; update it to reuse the same service-account
parsing helper used in initializeFirebase (or call the existing getProjectId
helper) so the resolved projectId comes from the same source used during
initialization (e.g., replace the current projectId lookup with
getProjectId(app) or the service-account parser used in initializeFirebase), and
add the corresponding getProjectId import to the top of the file so
firestore_list_databases and any related code use a consistent project ID.
In `@vitest.setup.ts`:
- Around line 64-67: The current catch in the Firebase initialization (in
vitest.setup.ts) swallows all errors; change this so failures fail fast unless
an explicit opt-in for mocked suites is present: detect an env flag like
USE_FIREBASE_MOCKS (or a provided config) and only suppress the error when that
flag is true; otherwise rethrow the caught error (or call process.exit(1)) so
the test run fails immediately and downstream beforeAll/afterAll auth failures
are not hidden.
🪄 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: c7d11c0f-8dae-448a-9282-e30ba8c656e6
📒 Files selected for processing (9)
.gitignoreCLAUDE.mdREADME.mdsrc/__tests__/config.test.tssrc/__tests__/multi-db.test.tssrc/config.tssrc/index.tssrc/lib/firebase/firestoreClient.tsvitest.setup.ts
| ### Server Core (`src/index.ts`) | ||
| Single `FirebaseMcpServer` class that: | ||
| 1. Initializes Firebase Admin SDK from `SERVICE_ACCOUNT_KEY_PATH` | ||
| 2. Registers 12 MCP tools via `setupToolHandlers()` (ListTools + CallTool handlers) | ||
| 3. Dispatches tool calls through a switch statement | ||
| 4. Connects to a transport (stdio or HTTP) | ||
|
|
There was a problem hiding this comment.
Update the documented tool count.
src/index.ts now registers 13 tools after adding firestore_list_databases, so "Registers 12 MCP tools" is already stale.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 27 - 33, The README line in CLAUDE.md incorrectly
states "Registers 12 MCP tools"; update the documentation to reflect the current
code in src/index.ts where FirebaseMcpServer (and its setupToolHandlers) now
registers 13 tools (including the newly added firestore_list_databases). Edit
the bullet under "Server Core (`src/index.ts`)" to say "Registers 13 MCP tools"
so the doc matches the FirebaseMcpServer/setupToolHandlers implementation.
| const url = `https://firestore.googleapis.com/v1/projects/${projectId}/databases`; | ||
| const response = await fetch(url, { | ||
| headers: { Authorization: `Bearer ${tokenResult.access_token}` }, | ||
| }); |
There was a problem hiding this comment.
Add a timeout to the Firestore Admin API call.
This fetch runs directly inside the tool handler with no abort signal. A slow or blackholed network request will hang the MCP call indefinitely instead of returning a bounded error.
Suggested timeout wrapper
const url = `https://firestore.googleapis.com/v1/projects/${projectId}/databases`;
- const response = await fetch(url, {
- headers: { Authorization: `Bearer ${tokenResult.access_token}` },
- });
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 5000);
+
+ let response: Response;
+ try {
+ response = await fetch(url, {
+ headers: { Authorization: `Bearer ${tokenResult.access_token}` },
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeoutId);
+ }📝 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.
| const url = `https://firestore.googleapis.com/v1/projects/${projectId}/databases`; | |
| const response = await fetch(url, { | |
| headers: { Authorization: `Bearer ${tokenResult.access_token}` }, | |
| }); | |
| const url = `https://firestore.googleapis.com/v1/projects/${projectId}/databases`; | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 5000); | |
| let response: Response; | |
| try { | |
| response = await fetch(url, { | |
| headers: { Authorization: `Bearer ${tokenResult.access_token}` }, | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 1601 - 1604, The Firestore Admin API fetch
(building `url` and calling `fetch` with `Authorization: Bearer
${tokenResult.access_token}`) has no timeout/abort handling; wrap this call with
an AbortController and a setTimeout that calls controller.abort() after a
reasonable ms (e.g. 5s), pass controller.signal into fetch, and clear the
timeout on success; update the surrounding tool handler to catch the abort error
(or a DOMException with name "AbortError") and return/throw a bounded timeout
error instead of letting the request hang.
| } catch (error) { | ||
| console.error('Firebase initialization failed:', error); | ||
| throw error; | ||
| // Don't throw — unit tests mock Firebase and don't need real credentials | ||
| } |
There was a problem hiding this comment.
Don't suppress Firebase bootstrap failures for the whole test run.
This file runs before every suite. Swallowing the initialization error here means a broken emulator/service-account setup only gets logged, while the later beforeAll/afterAll auth failures are swallowed too, so integration tests can continue in an invalid environment. Gate this behavior behind an explicit opt-in for mocked suites, or keep failing fast here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.setup.ts` around lines 64 - 67, The current catch in the Firebase
initialization (in vitest.setup.ts) swallows all errors; change this so failures
fail fast unless an explicit opt-in for mocked suites is present: detect an env
flag like USE_FIREBASE_MOCKS (or a provided config) and only suppress the error
when that flag is true; otherwise rethrow the caught error (or call
process.exit(1)) so the test run fails immediately and downstream
beforeAll/afterAll auth failures are not hidden.
- Fix stale tool count in CLAUDE.md - Remove extra blank line in .gitignore - Add projectId and credential to fakeApp mock so list_databases happy path is properly asserted - Resolve projectId via getProjectId() fallback instead of casting credential object - Add 10s timeout to Firestore Admin API fetch call - Fix broken error-handling test by adding missing module mocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 27-44: Add a blank line immediately after each "###" subsection
heading in CLAUDE.md to satisfy markdownlint MD022; specifically update the
headings "Server Core (`src/index.ts`)", "Firebase Service Clients
(`src/lib/firebase/`)", "Transport Layer (`src/transports/`)", and
"Configuration (`src/config.ts`)" so each is followed by one empty line before
the paragraph or list that follows.
In `@src/__tests__/index.test.ts`:
- Around line 2597-2605: The test currently mocks getFirestore to throw (via
vi.doMock('firebase-admin/firestore')) but still allows the generic fallback
assertion "Firebase initialization failed", so tighten the assertion to require
the mocked failure: update the test's expect/should-reject to assert that the
thrown error originates from the mocked getFirestore (e.g., error.message
includes genericError.message or equals genericError.message) rather than the
generic fallback string; ensure the test references the
vi.doMock('firebase-admin/firestore') mock and the genericError symbol so the
test only passes when getFirestore actually throws.
In `@src/__tests__/multi-db.test.ts`:
- Around line 100-103: The afterEach currently calls vi.resetModules() and
vi.clearAllMocks() but does not restore any overwritten global.fetch, which lets
stubs leak between tests; capture the original fetch (e.g. const ORIGINAL_FETCH
= global.fetch at module scope or save it before each stub) and restore it in
afterEach by assigning global.fetch = ORIGINAL_FETCH (or using a saved
reference), ensuring this restore runs alongside
vi.resetModules/vi.clearAllMocks so that tests which stub global.fetch are
cleaned up; update the tests that stub fetch (around the blocks referenced and
any other places) to either save/restore their original fetch or rely on the
shared ORIGINAL_FETCH restoration in afterEach.
In `@src/index.ts`:
- Around line 1586-1610: The firestore_list_databases path currently always
fetches a real OAuth token and calls the live Firestore Admin API (see
tokenResult, projectId, url, fetch); update it to first check for emulator mode
by looking for process.env.FIRESTORE_EMULATOR_HOST (or equivalent config flag)
and if present short-circuit: do not call credential.getAccessToken() or fetch
the Admin API, instead return the appropriate emulator-local response (or query
the emulator host) so the code stays local to the emulator; ensure tokenResult,
fetch call and AbortController logic are skipped when FIRESTORE_EMULATOR_HOST is
set.
- Around line 90-95: getFirestoreInstance currently treats empty or
whitespace-only databaseId as "not provided" and falls back to
config.firestoreDatabaseId; change it to first normalize and validate the
explicit input: if databaseId is provided (not null/undefined) but is an empty
or whitespace-only string (e.g., databaseId.trim() === ''), throw a clear Error
indicating an invalid databaseId; otherwise use the explicit databaseId, then
fall back to config.firestoreDatabaseId using the nullish coalescing operator
(config.firestoreDatabaseId ?? '(default)') before calling getFirestore(app,
resolvedId). Ensure validation logic sits at the start of getFirestoreInstance
and references the databaseId parameter and config.firestoreDatabaseId.
🪄 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: 2a56d982-ac67-4842-b010-e1c6877dae54
📒 Files selected for processing (5)
.gitignoreCLAUDE.mdsrc/__tests__/index.test.tssrc/__tests__/multi-db.test.tssrc/index.ts
| ### Server Core (`src/index.ts`) | ||
| Single `FirebaseMcpServer` class that: | ||
| 1. Initializes Firebase Admin SDK from `SERVICE_ACCOUNT_KEY_PATH` | ||
| 2. Registers MCP tools via `setupToolHandlers()` (ListTools + CallTool handlers) | ||
| 3. Dispatches tool calls through a switch statement | ||
| 4. Connects to a transport (stdio or HTTP) | ||
|
|
||
| ### Firebase Service Clients (`src/lib/firebase/`) | ||
| - `firestoreClient.ts` - Firestore CRUD, queries, collection group queries | ||
| - `storageClient.ts` - File list/info/upload (local, base64, text, URL) | ||
| - `authClient.ts` - User lookup by email or UID | ||
|
|
||
| ### Transport Layer (`src/transports/`) | ||
| - `index.ts` - Factory that selects stdio vs HTTP based on config | ||
| - `http.ts` - Express.js server with session-based SSE for MCP over HTTP | ||
|
|
||
| ### Configuration (`src/config.ts`) | ||
| Environment variables loaded via dotenv. `SERVICE_ACCOUNT_KEY_PATH` is required; transport defaults to stdio. `FIRESTORE_DATABASE_ID` optionally targets a named Firestore database. Auto-detects stdio context from TTY state. |
There was a problem hiding this comment.
Add blank lines after these subsection headings.
markdownlint is already flagging MD022 here, so the docs/lint step will keep failing until each ### heading is followed by a blank line.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 39-39: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 43-43: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 27 - 44, Add a blank line immediately after each
"###" subsection heading in CLAUDE.md to satisfy markdownlint MD022;
specifically update the headings "Server Core (`src/index.ts`)", "Firebase
Service Clients (`src/lib/firebase/`)", "Transport Layer (`src/transports/`)",
and "Configuration (`src/config.ts`)" so each is followed by one empty line
before the paragraph or list that follows.
| vi.doMock('firebase-admin/firestore', () => ({ | ||
| Timestamp: { fromDate: vi.fn() }, | ||
| getFirestore: vi.fn().mockImplementation(() => { | ||
| throw genericError; | ||
| }), | ||
| })); | ||
| vi.doMock('../lib/firebase/firebaseConfig.js', () => ({ | ||
| getProjectId: vi.fn().mockReturnValue('test-project'), | ||
| })); |
There was a problem hiding this comment.
Make this test require the mocked getFirestore() failure.
With Lines 2597-2605 now forcing getFirestore() to throw, the fallback branch below that still accepts "Firebase initialization failed" keeps this test green on the same regression it was meant to catch.
Tighten the assertion in this test
- if (content.error === 'Firebase initialization failed') {
- expect(content).toHaveProperty('error');
- } else {
- expect(result.content[0].text).toContain('Generic Firestore error');
- }
+ expect(result.content[0].text).toContain('Generic Firestore error');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/index.test.ts` around lines 2597 - 2605, The test currently
mocks getFirestore to throw (via vi.doMock('firebase-admin/firestore')) but
still allows the generic fallback assertion "Firebase initialization failed", so
tighten the assertion to require the mocked failure: update the test's
expect/should-reject to assert that the thrown error originates from the mocked
getFirestore (e.g., error.message includes genericError.message or equals
genericError.message) rather than the generic fallback string; ensure the test
references the vi.doMock('firebase-admin/firestore') mock and the genericError
symbol so the test only passes when getFirestore actually throws.
| afterEach(() => { | ||
| vi.resetModules(); | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Restore fetch after stubbing it in these cases.
These tests overwrite global.fetch, but the current afterEach never puts the original global back. That can leak this stub into later cases in the same worker.
Simple cleanup pattern
let loggerMock: Record<string, ReturnType<typeof vi.fn>>;
let getFirestoreMock: ReturnType<typeof vi.fn>;
let firestoreInstanceMock: ReturnType<typeof createFirestoreMock>;
+let originalFetch: typeof global.fetch;
describe('Multi-database support', () => {
beforeEach(async () => {
vi.resetModules();
vi.clearAllMocks();
+ originalFetch = global.fetch;
@@
afterEach(() => {
+ global.fetch = originalFetch;
vi.resetModules();
vi.clearAllMocks();
});
@@
- global.fetch = mockFetch;
+ global.fetch = mockFetch as typeof global.fetch;
@@
- global.fetch = mockFetch;
+ global.fetch = mockFetch as typeof global.fetch;Also applies to: 372-390, 412-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/multi-db.test.ts` around lines 100 - 103, The afterEach
currently calls vi.resetModules() and vi.clearAllMocks() but does not restore
any overwritten global.fetch, which lets stubs leak between tests; capture the
original fetch (e.g. const ORIGINAL_FETCH = global.fetch at module scope or save
it before each stub) and restore it in afterEach by assigning global.fetch =
ORIGINAL_FETCH (or using a saved reference), ensuring this restore runs
alongside vi.resetModules/vi.clearAllMocks so that tests which stub global.fetch
are cleaned up; update the tests that stub fetch (around the blocks referenced
and any other places) to either save/restore their original fetch or rely on the
shared ORIGINAL_FETCH restoration in afterEach.
| function getFirestoreInstance(databaseId?: string): FirebaseFirestore.Firestore { | ||
| const resolvedId = databaseId || config.firestoreDatabaseId || '(default)'; | ||
| if (!app) { | ||
| throw new Error('Firebase app is not initialized'); | ||
| } | ||
| return getFirestore(app, resolvedId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find the src/index.ts file
git ls-files | head -20Repository: gannonh/firebase-mcp
Length of output: 462
🏁 Script executed:
# Let's examine the exact lines in question
if [ -f "src/index.ts" ]; then
wc -l src/index.ts
fiRepository: gannonh/firebase-mcp
Length of output: 81
🏁 Script executed:
# Read lines 85-100 to see the context
sed -n '85,100p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 642
🏁 Script executed:
# Also search for the firestore_list_databases function mentioned
rg "firestore_list_databases" -A 5Repository: gannonh/firebase-mcp
Length of output: 3269
🏁 Script executed:
# Find all calls to getFirestoreInstance
rg "getFirestoreInstance" -B 2 -A 2Repository: gannonh/firebase-mcp
Length of output: 4110
🏁 Script executed:
# Search for where databaseId comes from in the handlers
rg "databaseId" src/index.ts | head -30Repository: gannonh/firebase-mcp
Length of output: 1308
🏁 Script executed:
# Look at the firestore_list_databases implementation in detail
sed -n '1558,1667p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 3803
🏁 Script executed:
# Check the test file to see how databaseId is tested
sed -n '1,50p' src/__tests__/multi-db.test.ts | grep -A 30 "getFirestoreInstance resolution"Repository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Look for test cases involving blank or empty databaseId
rg "databaseId.*['\"']" src/__tests__/multi-db.test.tsRepository: gannonh/firebase-mcp
Length of output: 1088
🏁 Script executed:
# Check if there are any handlers that might pass empty strings
rg "args.databaseId" -B 5 src/index.ts | head -50Repository: gannonh/firebase-mcp
Length of output: 2081
🏁 Script executed:
# Look for the structure of databaseId schema definition
rg "databaseId" src/index.ts -B 3 -A 1 | grep -A 5 "databaseIdProperty"Repository: gannonh/firebase-mcp
Length of output: 1459
🏁 Script executed:
# Get the complete databaseIdProperty definition
rg "const databaseIdProperty" -A 5 src/index.tsRepository: gannonh/firebase-mcp
Length of output: 275
🏁 Script executed:
# Check if there's any input validation before args is used
rg "args\s*=.*as|args\s*=.*\?" src/index.ts | head -20Repository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Look for the CallToolRequest handling and argument validation
rg "CallToolRequest|call_tool" -A 10 src/index.ts | head -50Repository: gannonh/firebase-mcp
Length of output: 851
🏁 Script executed:
# Check how the tool arguments are processed and if there's validation
sed -n '200,250p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 1784
🏁 Script executed:
# Look for where the tools are registered to understand the flow
sed -n '100,150p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 1441
🏁 Script executed:
# Search for emulator-related code
rg "emulator|FIRESTORE_EMULATOR" src/Repository: gannonh/firebase-mcp
Length of output: 4924
🏁 Script executed:
# Check how the config is structured
cat src/config.tsRepository: gannonh/firebase-mcp
Length of output: 4256
🏁 Script executed:
# Look at tests to see if blank databaseId is tested
rg "databaseId.*['\"']" src/__tests__/Repository: gannonh/firebase-mcp
Length of output: 1491
🏁 Script executed:
# Examine the firestore_list_databases handler more closely to understand the REST API call
sed -n '1558,1620p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 2250
🏁 Script executed:
# Check if there's any client-side validation of databaseId before it reaches the handler
rg "databaseId" src/index.ts -B 5 | grep -A 5 "validate\|trim\|check\|error"Repository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Look at how the MCP framework handles arguments - is there schema validation?
rg "CallToolRequest" -A 20 src/index.ts | head -40Repository: gannonh/firebase-mcp
Length of output: 1251
🏁 Script executed:
# Check if there are any tests that attempt to pass empty strings as databaseId
rg "databaseId.*['\"][ ]*['\"]" src/__tests__/Repository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Verify the actual runtime behavior - look at the full handler to see if args validation happens
sed -n '560,600p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 1485
🏁 Script executed:
# Check if there's any schema-level validation or if MCP framework validates against the schema
rg "properties\|required" src/index.ts | grep -A 2 "databaseId"Repository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Verify the exact flow - when arguments come in, are they validated against schema?
sed -n '240,280p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 1321
🏁 Script executed:
# Check if firebase-admin's getFirestore function handles empty strings gracefully
rg "getFirestore" src/ -A 2 | head -30Repository: gannonh/firebase-mcp
Length of output: 1541
🏁 Script executed:
# Look for any trim or validation logic on databaseId anywhere in the codebase
rg "trim|isEmpty|isBlank" src/index.tsRepository: gannonh/firebase-mcp
Length of output: 46
🏁 Script executed:
# Double-check the falsy behavior - verify empty string behavior in JavaScript context
python3 << 'EOF'
# Simulate JavaScript || behavior for empty string
empty_string = ""
config_value = "configured-db"
default_value = "(default)"
# This is how JavaScript || works with empty strings
result = empty_string or config_value or default_value
print(f"Empty string: '{empty_string}'")
print(f"Result of empty || configured || default: '{result}'")
print(f"Empty string is falsy: {not bool(empty_string)}")
EOFRepository: gannonh/firebase-mcp
Length of output: 166
🏁 Script executed:
# Check if there are any real-world examples or tests that pass empty strings
rg "databaseId" src/__tests__/ -B 3 -A 3 | grep -E "databaseId.*['\"]|test.*empty|blank"Repository: gannonh/firebase-mcp
Length of output: 1491
🏁 Script executed:
# Search for any error handling or defensive checks elsewhere in the codebase
rg "databaseId" src/index.ts | wc -lRepository: gannonh/firebase-mcp
Length of output: 66
🏁 Script executed:
# Let's verify what firebase-admin's getFirestore actually does with empty strings
# Check documentation or behavior by looking at how it's used
rg "getFirestore" -B 3 -A 3 src/ | grep -v testRepository: gannonh/firebase-mcp
Length of output: 4424
🏁 Script executed:
# Check if there are any guards or try-catch blocks around getFirestore calls
sed -n '640,680p' src/index.tsRepository: gannonh/firebase-mcp
Length of output: 1744
🏁 Script executed:
# Verify: can the MCP schema validation prevent empty strings from being passed?
rg "minLength|pattern|enum" src/index.ts | head -20Repository: gannonh/firebase-mcp
Length of output: 153
Reject blank databaseId values instead of silently falling through.
On line 91, databaseId || config.firestoreDatabaseId || '(default)' treats empty strings as "not provided". If a client explicitly passes an empty databaseId and a server-wide FIRESTORE_DATABASE_ID is configured, the application silently operates on the configured database instead of rejecting the invalid input. This can lead to unintended data writes to the wrong database.
Normalize explicit input first, fail on blank values, then apply the fallback chain. The suggested approach using trim() to detect whitespace-only strings, explicit validation, and the nullish coalescing operator (??) prevents this silent failure:
Normalization approach
function getFirestoreInstance(databaseId?: string): FirebaseFirestore.Firestore {
- const resolvedId = databaseId || config.firestoreDatabaseId || '(default)';
+ const explicitDatabaseId = databaseId?.trim();
+ const configuredDatabaseId = config.firestoreDatabaseId?.trim() || undefined;
+
+ if (databaseId !== undefined && !explicitDatabaseId) {
+ throw new Error('databaseId must not be empty');
+ }
+
+ const resolvedId = explicitDatabaseId ?? configuredDatabaseId ?? '(default)';
if (!app) {
throw new Error('Firebase app is not initialized');
}
return getFirestore(app, resolvedId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 90 - 95, getFirestoreInstance currently treats
empty or whitespace-only databaseId as "not provided" and falls back to
config.firestoreDatabaseId; change it to first normalize and validate the
explicit input: if databaseId is provided (not null/undefined) but is an empty
or whitespace-only string (e.g., databaseId.trim() === ''), throw a clear Error
indicating an invalid databaseId; otherwise use the explicit databaseId, then
fall back to config.firestoreDatabaseId using the nullish coalescing operator
(config.firestoreDatabaseId ?? '(default)') before calling getFirestore(app,
resolvedId). Ensure validation logic sits at the start of getFirestoreInstance
and references the databaseId parameter and config.firestoreDatabaseId.
There was a problem hiding this comment.
Intentionally leaving this as-is. The MCP schema defines databaseId as type: "string" — AI clients won't send empty strings here, and if they somehow do, getFirestore(app, "") will throw from the Firebase SDK itself, which is already caught and returned as an error response. Adding trim/validation/nullish-coalescing for this scenario adds complexity for a case that can't happen in practice.
There was a problem hiding this comment.
@mtio, understood — the combination of the MCP schema type constraint, the Firebase SDK's own validation, and the existing catch-all error handler already covers this path. Happy to leave it as-is.
(˵ •̀ ᴗ - ˵ ) ✧
✏️ Learnings added
Learnt from: mtio
Repo: gannonh/firebase-mcp PR: 171
File: src/index.ts:90-95
Timestamp: 2026-04-04T15:43:30.375Z
Learning: In `src/index.ts` of the `firebase-mcp` project, the `getFirestoreInstance(databaseId?: string)` function intentionally uses `||` (not `??`) for the fallback chain `databaseId || config.firestoreDatabaseId || '(default)'`. Empty-string `databaseId` values are considered invalid input that the MCP schema (`type: "string"`) prevents in practice; if they do slip through, `getFirestore(app, "")` throws from the Firebase SDK itself, which is already caught by the surrounding try/catch handlers. Adding explicit trim/validation/nullish-coalescing was deliberately rejected as unnecessary complexity.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Return an explicit error when FIRESTORE_EMULATOR_HOST is set, since the tool calls the live Firestore Admin REST API which would bypass the emulator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 641-647: When applying pagination in the
firestore_query_collection and firestore_query_collection_group handlers, do not
silently ignore stale/mismatched page tokens; after resolving pageToken
(args.pageToken) and fetching lastDoc via firestore.doc(pageToken).get(), if
(!lastDoc.exists) throw or return an explicit invalid-token error instead of
falling through to page 1. Update both places that currently only set query =
query.startAfter(lastDoc) to validate lastDoc.exists and return a clear error
(e.g., InvalidPageToken or similar) so callers see an invalid-token response
rather than duplicated first-page results.
- Around line 426-430: The schema for the firestore_list_collections operation
currently only exposes databaseId (inputSchema) but the implementation still
reads args.documentPath and forwards it (firestore_list_collections); update the
inputSchema to include a documentPath property (matching how args.documentPath
is consumed) so discovery-driven MCP clients can see that argument, or remove
the hidden branch that relies on args.documentPath and stop forwarding it in the
firestore_list_collections call — ensure the symbol names to change are
inputSchema and any places referencing
args.documentPath/firestore_list_collections so they remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| inputSchema: { | ||
| type: 'object', | ||
| properties: {}, | ||
| properties: { | ||
| databaseId: databaseIdProperty, | ||
| }, |
There was a problem hiding this comment.
Publish documentPath in the schema or remove the hidden branch.
Line 1269 still reads args.documentPath and Line 1291 forwards it, but the advertised firestore_list_collections schema now exposes only databaseId. Discovery-driven MCP clients cannot see that argument, so subcollection requests can silently degrade into root-collection listings when documentPath is dropped.
Suggested schema fix
{
name: 'firestore_list_collections',
description: 'List root collections in Firestore',
inputSchema: {
type: 'object',
properties: {
+ documentPath: {
+ type: 'string',
+ description:
+ 'Optional document path to list subcollections under. Omit to list root collections.',
+ },
databaseId: databaseIdProperty,
},
required: [],
},
},Also applies to: 1268-1295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 426 - 430, The schema for the
firestore_list_collections operation currently only exposes databaseId
(inputSchema) but the implementation still reads args.documentPath and forwards
it (firestore_list_collections); update the inputSchema to include a
documentPath property (matching how args.documentPath is consumed) so
discovery-driven MCP clients can see that argument, or remove the hidden branch
that relies on args.documentPath and stop forwarding it in the
firestore_list_collections call — ensure the symbol names to change are
inputSchema and any places referencing
args.documentPath/firestore_list_collections so they remain consistent.
| // Apply pagination if pageToken is provided | ||
| const pageToken = args.pageToken as string | undefined; | ||
| if (pageToken) { | ||
| const lastDoc = await admin.firestore().doc(pageToken).get(); | ||
| const lastDoc = await firestore.doc(pageToken).get(); | ||
| if (lastDoc.exists) { | ||
| query = query.startAfter(lastDoc); | ||
| } |
There was a problem hiding this comment.
Reject missing page tokens instead of silently restarting pagination.
Line 644 and Line 1401 only paginate when the token document exists. A stale token — or one copied from another databaseId — currently falls through and returns page 1 again, which makes pagination look successful while duplicating results. Return an explicit invalid-token error when !lastDoc.exists in both handlers.
Suggested guard
if (pageToken) {
const lastDoc = await firestore.doc(pageToken).get();
- if (lastDoc.exists) {
- query = query.startAfter(lastDoc);
- }
+ if (!lastDoc.exists) {
+ return {
+ content: [
+ {
+ type: 'text',
+ text: JSON.stringify({ error: 'Invalid pagination token' }),
+ },
+ ],
+ };
+ }
+ query = query.startAfter(lastDoc);
}Apply the same !lastDoc.exists check in firestore_query_collection_group.
Also applies to: 1398-1404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 641 - 647, When applying pagination in the
firestore_query_collection and firestore_query_collection_group handlers, do not
silently ignore stale/mismatched page tokens; after resolving pageToken
(args.pageToken) and fetching lastDoc via firestore.doc(pageToken).get(), if
(!lastDoc.exists) throw or return an explicit invalid-token error instead of
falling through to page 1. Update both places that currently only set query =
query.startAfter(lastDoc) to validate lastDoc.exists and return a clear error
(e.g., InvalidPageToken or similar) so callers see an invalid-token response
rather than duplicated first-page results.
1 participant
Summary
FIRESTORE_DATABASE_IDenvironment variable as a server-wide defaultdatabaseIdparameter on all Firestore toolsdatabaseIdarg → env var →(default)firestore_list_databasestool that lists all databases in the project via the Firestore Admin REST APIgetFirestoreInstance()helper that resolves the database ID and returns the correct Firestore instance while reusing the single Firebase appTest plan
multi-db.test.tscovering schema registration, resolution priority chain, per-tool forwarding for all 7 Firestore tools, and list databases toolconfig.test.tsfor theFIRESTORE_DATABASE_IDenv varpublish-preflight)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores