feat(gcp): implement native nodes for firestore, gcs, and vertex ai (…#1608
feat(gcp): implement native nodes for firestore, gcs, and vertex ai (…#1608kiet08hogit wants to merge 1 commit into
Conversation
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
📝 WalkthroughWalkthroughIntroduces shared GCP authentication supporting service-account JSON and ADC, then adds Firestore, Google Cloud Storage, and Vertex AI Vector Search nodes with lifecycle management, tools, service schemas, dependencies, and documentation. ChangesGCP data infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 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 `@nodes/src/nodes/core/gcp_auth.py`:
- Around line 25-29: Use single-quoted string literals throughout the affected
Python code unless a literal contains a single quote: in
nodes/src/nodes/core/gcp_auth.py ranges 25-29, 35-36, 42-43, 49-53, 57-57, and
64-68, update the config keys, auth values, comma, and f-strings; in
nodes/src/nodes/tool_gcs/IGlobal.py ranges 23-24, 34-36, 50-52, and 60-61,
update the ImportError message and f-strings; in
nodes/src/nodes/vectordb_vertex/IGlobal.py ranges 24-25, 33-34, 39-41, 56-58,
and 68-69, update the ImportError message, warning string, and f-strings.
Preserve interpolation and runtime behavior.
In `@nodes/src/nodes/tool_gcs/IInstance.py`:
- Around line 10-70: Update all string literals in the download_file and
list_files methods of nodes/src/nodes/tool_gcs/IInstance.py (lines 10-70) to use
single quotes, and apply the same conversion to all string literals in
nodes/src/nodes/vectordb_vertex/IInstance.py (lines 8-40), preserving the
existing behavior and enforcing the project’s ruff single-quote formatting.
- Around line 33-40: Update the temporary-file handling around
blob.download_to_filename in the download method: if the download fails after
tempfile.mkstemp creates temp_path, remove that file before re-raising so the
outer exception handler still returns the existing error response. Preserve the
successful return path and avoid deleting the file after a successful download.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 407a3f04-d9cd-458c-b761-b1f55b47cdad
📒 Files selected for processing (20)
nodes/src/nodes/core/gcp_auth.pynodes/src/nodes/core/services.common.gcp.jsonnodes/src/nodes/db_firestore/IGlobal.pynodes/src/nodes/db_firestore/IInstance.pynodes/src/nodes/db_firestore/README.mdnodes/src/nodes/db_firestore/__init__.pynodes/src/nodes/db_firestore/requirements.txtnodes/src/nodes/db_firestore/services.jsonnodes/src/nodes/tool_gcs/IGlobal.pynodes/src/nodes/tool_gcs/IInstance.pynodes/src/nodes/tool_gcs/README.mdnodes/src/nodes/tool_gcs/__init__.pynodes/src/nodes/tool_gcs/requirements.txtnodes/src/nodes/tool_gcs/services.jsonnodes/src/nodes/vectordb_vertex/IGlobal.pynodes/src/nodes/vectordb_vertex/IInstance.pynodes/src/nodes/vectordb_vertex/README.mdnodes/src/nodes/vectordb_vertex/__init__.pynodes/src/nodes/vectordb_vertex/requirements.txtnodes/src/nodes/vectordb_vertex/services.json
| auth_type = config.get("gcp.authType", "adc") | ||
| project_id = config.get("gcp.projectId") | ||
|
|
||
| if auth_type == "service_account": | ||
| key_data = config.get("gcp.serviceAccountKey") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use single quotes for string literals.
Double quotes are used for several string literals across the Python files. As per path instructions, use single quotes for string literals in nodes/**/*.py unless the string itself contains a single quote.
nodes/src/nodes/core/gcp_auth.py#L25-L29: replace double quotes with single quotes forgcp.authType,adc,gcp.projectId,service_account,gcp.serviceAccountKey.nodes/src/nodes/core/gcp_auth.py#L35-L36: replace double quotes with single quotes for the,character.nodes/src/nodes/core/gcp_auth.py#L42-L43: replace double quotes with single quotes for the f-string.nodes/src/nodes/core/gcp_auth.py#L49-L53: replace double quotes with single quotes for the f-string andproject_id.nodes/src/nodes/core/gcp_auth.py#L57-L57: replace double quotes with single quotes foradc.nodes/src/nodes/core/gcp_auth.py#L64-L68: replace double quotes with single quotes for the f-strings.nodes/src/nodes/tool_gcs/IGlobal.py#L23-L24: replace double quotes with single quotes for theImportErrormessage.nodes/src/nodes/tool_gcs/IGlobal.py#L34-L36: replace double quotes with single quotes for the f-string.nodes/src/nodes/tool_gcs/IGlobal.py#L50-L52: replace double quotes with single quotes for the f-string.nodes/src/nodes/tool_gcs/IGlobal.py#L60-L61: replace double quotes with single quotes for the f-string.nodes/src/nodes/vectordb_vertex/IGlobal.py#L24-L25: replace double quotes with single quotes for theImportErrormessage.nodes/src/nodes/vectordb_vertex/IGlobal.py#L33-L34: replace double quotes with single quotes for the warning string.nodes/src/nodes/vectordb_vertex/IGlobal.py#L39-L41: replace double quotes with single quotes for the f-string.nodes/src/nodes/vectordb_vertex/IGlobal.py#L56-L58: replace double quotes with single quotes for the f-string.nodes/src/nodes/vectordb_vertex/IGlobal.py#L68-L69: replace double quotes with single quotes for the f-string.
📍 Affects 3 files
nodes/src/nodes/core/gcp_auth.py#L25-L29(this comment)nodes/src/nodes/core/gcp_auth.py#L35-L36nodes/src/nodes/core/gcp_auth.py#L42-L43nodes/src/nodes/core/gcp_auth.py#L49-L53nodes/src/nodes/core/gcp_auth.py#L57-L57nodes/src/nodes/core/gcp_auth.py#L64-L68nodes/src/nodes/tool_gcs/IGlobal.py#L23-L24nodes/src/nodes/tool_gcs/IGlobal.py#L34-L36nodes/src/nodes/tool_gcs/IGlobal.py#L50-L52nodes/src/nodes/tool_gcs/IGlobal.py#L60-L61nodes/src/nodes/vectordb_vertex/IGlobal.py#L24-L25nodes/src/nodes/vectordb_vertex/IGlobal.py#L33-L34nodes/src/nodes/vectordb_vertex/IGlobal.py#L39-L41nodes/src/nodes/vectordb_vertex/IGlobal.py#L56-L58nodes/src/nodes/vectordb_vertex/IGlobal.py#L68-L69
🤖 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 `@nodes/src/nodes/core/gcp_auth.py` around lines 25 - 29, Use single-quoted
string literals throughout the affected Python code unless a literal contains a
single quote: in nodes/src/nodes/core/gcp_auth.py ranges 25-29, 35-36, 42-43,
49-53, 57-57, and 64-68, update the config keys, auth values, comma, and
f-strings; in nodes/src/nodes/tool_gcs/IGlobal.py ranges 23-24, 34-36, 50-52,
and 60-61, update the ImportError message and f-strings; in
nodes/src/nodes/vectordb_vertex/IGlobal.py ranges 24-25, 33-34, 39-41, 56-58,
and 68-69, update the ImportError message, warning string, and f-strings.
Preserve interpolation and runtime behavior.
Source: Path instructions
| @tool_function( | ||
| description="Download a file from Google Cloud Storage. Returns a dictionary containing the local temporary path of the downloaded file.", | ||
| args={ | ||
| "file_name": "The name/path of the file in the bucket to download." | ||
| } | ||
| ) | ||
| def download_file(self, file_name: str) -> Dict[str, Any]: | ||
| client = self.glb.client | ||
| if not client: | ||
| return {"error": "GCS client is not connected."} | ||
|
|
||
| bucket_name = self.glb.bucket_name | ||
| if not bucket_name: | ||
| return {"error": "Bucket name not configured."} | ||
|
|
||
| # Optional prefix prefixing | ||
| if self.glb.prefix: | ||
| file_name = f"{self.glb.prefix.rstrip('/')}/{file_name.lstrip('/')}" | ||
|
|
||
| try: | ||
| bucket = client.bucket(bucket_name) | ||
| blob = bucket.blob(file_name) | ||
|
|
||
| # Download to a temporary file | ||
| fd, temp_path = tempfile.mkstemp(prefix="gcs_") | ||
| os.close(fd) | ||
| blob.download_to_filename(temp_path) | ||
|
|
||
| return {"success": True, "local_path": temp_path} | ||
| except Exception as e: | ||
| return {"error": f"Failed to download file: {e}"} | ||
|
|
||
| @tool_function( | ||
| description="List files in the configured Google Cloud Storage bucket.", | ||
| args={ | ||
| "prefix": "Optional prefix to filter files.", | ||
| "max_results": "Maximum number of files to return (default 10)." | ||
| } | ||
| ) | ||
| def list_files(self, prefix: str = "", max_results: int = 10) -> List[str]: | ||
| client = self.glb.client | ||
| if not client: | ||
| return ["Error: GCS client is not connected."] | ||
|
|
||
| bucket_name = self.glb.bucket_name | ||
| if not bucket_name: | ||
| return ["Error: Bucket name not configured."] | ||
|
|
||
| # Combine node-level prefix and runtime prefix | ||
| full_prefix = "" | ||
| if self.glb.prefix: | ||
| full_prefix = self.glb.prefix.rstrip('/') + '/' | ||
| if prefix: | ||
| full_prefix += prefix.lstrip('/') | ||
|
|
||
| try: | ||
| bucket = client.bucket(bucket_name) | ||
| blobs = bucket.list_blobs(prefix=full_prefix, max_results=max_results) | ||
| return [blob.name for blob in blobs] | ||
| except Exception as e: | ||
| return [f"Failed to list files: {e}"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use single quotes for string literals.
Both files use double quotes for string literals, which violates the path instruction requiring single quotes for Python pipeline nodes. Please enforce ruff formatting with single quotes.
nodes/src/nodes/tool_gcs/IInstance.py#L10-L70: convert all double-quoted string literals to single quotes.nodes/src/nodes/vectordb_vertex/IInstance.py#L8-L40: convert all double-quoted string literals to single quotes.
📍 Affects 2 files
nodes/src/nodes/tool_gcs/IInstance.py#L10-L70(this comment)nodes/src/nodes/vectordb_vertex/IInstance.py#L8-L40
🤖 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 `@nodes/src/nodes/tool_gcs/IInstance.py` around lines 10 - 70, Update all
string literals in the download_file and list_files methods of
nodes/src/nodes/tool_gcs/IInstance.py (lines 10-70) to use single quotes, and
apply the same conversion to all string literals in
nodes/src/nodes/vectordb_vertex/IInstance.py (lines 8-40), preserving the
existing behavior and enforcing the project’s ruff single-quote formatting.
Source: Path instructions
| # Download to a temporary file | ||
| fd, temp_path = tempfile.mkstemp(prefix="gcs_") | ||
| os.close(fd) | ||
| blob.download_to_filename(temp_path) | ||
|
|
||
| return {"success": True, "local_path": temp_path} | ||
| except Exception as e: | ||
| return {"error": f"Failed to download file: {e}"} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent temporary file leak on download failure.
If blob.download_to_filename(temp_path) raises an exception, the 0-byte temporary file created by tempfile.mkstemp remains on disk and is never cleaned up. Wrap the download call in a try...except block to remove the file on failure before re-raising the exception to the outer error handler.
🛠️ Proposed fix
- # Download to a temporary file
- fd, temp_path = tempfile.mkstemp(prefix="gcs_")
- os.close(fd)
- blob.download_to_filename(temp_path)
-
- return {"success": True, "local_path": temp_path}
- except Exception as e:
- return {"error": f"Failed to download file: {e}"}
+ # Download to a temporary file
+ fd, temp_path = tempfile.mkstemp(prefix='gcs_')
+ os.close(fd)
+ try:
+ blob.download_to_filename(temp_path)
+ return {'success': True, 'local_path': temp_path}
+ except Exception:
+ if os.path.exists(temp_path):
+ os.remove(temp_path)
+ raise
+ except Exception as e:
+ return {'error': f'Failed to download file: {e}'}📝 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.
| # Download to a temporary file | |
| fd, temp_path = tempfile.mkstemp(prefix="gcs_") | |
| os.close(fd) | |
| blob.download_to_filename(temp_path) | |
| return {"success": True, "local_path": temp_path} | |
| except Exception as e: | |
| return {"error": f"Failed to download file: {e}"} | |
| # Download to a temporary file | |
| fd, temp_path = tempfile.mkstemp(prefix='gcs_') | |
| os.close(fd) | |
| try: | |
| blob.download_to_filename(temp_path) | |
| return {'success': True, 'local_path': temp_path} | |
| except Exception: | |
| if os.path.exists(temp_path): | |
| os.remove(temp_path) | |
| raise | |
| except Exception as e: | |
| return {'error': f'Failed to download file: {e}'} |
🤖 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 `@nodes/src/nodes/tool_gcs/IInstance.py` around lines 33 - 40, Update the
temporary-file handling around blob.download_to_filename in the download method:
if the download fails after tempfile.mkstemp creates temp_path, remove that file
before re-raising so the outer exception handler still returns the existing
error response. Preserve the successful return path and avoid deleting the file
after a successful download.
joshuadarron
left a comment
joshuadarron
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the node structure follows the repo conventions well (shared services.common.* auth file, per-node requirements.txt + depends(), README with generated-params markers). But there are several blocking issues, two of which mean the nodes cannot work at all as shipped.
Blocking (see inline comments for details):
gcp_auth.pyreads config keys with thegcp.prefix, but the engine delivers keys with the dotted namespace stripped —authType/serviceAccountKey/projectId. As written, auth always silently falls back to ADC and the service-account path is dead code.db_firestorecallsClient.collections(page_size=1)— that parameter does not exist (verified against google-cloud-firestore 2.28.0:collections(retry, timeout, *, read_time)), sobeginGlobalraisesTypeErrorevery time and the node can never start.- All three
services.jsonfiles declare lane wiring (lanes.questions) that no code implements,db_firestoredeclares aninvoke.llmit never uses, andvectordb_vertexuses aclassTypevalue ("vector") that doesn't exist in the catalog (vector stores use"store"). - The three icon SVGs referenced by
services.json(db_firestore.svg,tool_gcs.svg,vertex.svg) are not in the PR. - Ruff fails on the new Python files: 83× Q000 (single quotes are enforced repo-wide) plus 1× D204. All auto-fixable with
python -m ruff check --fix. - The PR checklist claims tests were added, but there are none. Other nodes ship suites under
nodes/test/(seenodes/test/tool_gmail/for a good template using the stub framework — no live GCP needed). At minimum,gcp_auth.get_gcp_credentialskey parsing (data-url + raw JSON + error paths) is very testable.
Also: please document the new services.common.gcp.json in nodes/src/nodes/core/README.md alongside the existing AWS/Google Workspace sections (co-located documentation rule).
Happy to re-review once these are addressed — the fixes are mostly mechanical.
| except ImportError: | ||
| raise GCPAuthError("google-auth library is not installed. Ensure node requirements include 'google-auth'.") | ||
|
|
||
| auth_type = config.get("gcp.authType", "adc") |
There was a problem hiding this comment.
🔴 bug: engine delivers node config with the dotted namespace stripped — keys arrive as authType, projectId, serviceAccountKey (see tool_gmail/IGlobal.py:64 reading cfg.get('authType') for field google.authType, and the comment in db_postgres/IGlobal.py:41). config.get("gcp.authType") never matches → always falls back to adc; the service_account branch is dead code and gcp.projectId is ignored. Fix: read authType / serviceAccountKey / projectId.
| # Fail fast connection check | ||
| try: | ||
| # simple check to verify connectivity/auth | ||
| next(self.client.collections(page_size=1), None) |
There was a problem hiding this comment.
🔴 bug: Client.collections() has no page_size parameter (google-cloud-firestore 2.28.0 signature: collections(retry, timeout, *, read_time)) → TypeError on every beginGlobal; the node can never start. Use next(self.client.collections(), None). Also note: listing collections needs broader IAM than document get/set — a narrowly-scoped SA will fail this check even though the actual tools would work. Consider a cheaper probe (e.g. a no-op doc get).
| "icon": "db_firestore.svg", | ||
| "documentation": "https://docs.rocketride.org", | ||
| "invoke": { | ||
| "llm": { |
There was a problem hiding this comment.
🔴 bug: invoke.llm ("craft NoSQL queries") declared but IInstance never uses an LLM. The db_* nodes get question→query machinery from DatabaseGlobalBase/DatabaseInstanceBase (see db_postgres); this node extends raw IGlobalBase. Same for lanes.questions: ["table", "text", "answers"] below — no writeQuestions handler exists. Either derive from the database base classes or make this a pure tool node: "invoke": {}, "lanes": {} (cf. tool_tavily, tool_slack).
| "path": "nodes.db_firestore", | ||
| "prefix": "firestore", | ||
| "description": ["A processing component that connects to Google Cloud Firestore (Datastore Mode or Native)."], | ||
| "icon": "db_firestore.svg", |
There was a problem hiding this comment.
🔴 bug: db_firestore.svg referenced but not added (same for tool_gcs.svg, vertex.svg). Icon SVG goes next to services.json — docs/README-nodes.md "Adding a New Node" step 4.
| "icon": "tool_gcs.svg", | ||
| "documentation": "https://docs.rocketride.org", | ||
| "invoke": {}, | ||
| "lanes": { |
There was a problem hiding this comment.
🔴 bug: lanes.questions declared but no lane handler (writeQuestions) implemented. Pure tool nodes declare "lanes": {} (cf. tool_tavily/services.json, tool_slack/services.json). Same issue in vectordb_vertex/services.json.
| { | ||
| "title": "Vertex AI Vector Search", | ||
| "protocol": "vectordb_vertex://", | ||
| "classType": ["vector", "tool"], |
There was a problem hiding this comment.
🔴 bug: classType "vector" doesn't exist anywhere in the catalog — vector stores use "store" (milvus, pinecone, qdrant, vectordb_postgres). Use ["store", "tool"] or just ["tool"] given only tool functions are implemented.
| @@ -0,0 +1,16 @@ | |||
| # Firestore Node | |||
|
|
|||
| A standard node for interacting with Google Cloud Firestore (Datastore mode or Native mode). | |||
There was a problem hiding this comment.
🟡 risk: google-cloud-firestore only works against Native-mode databases; Datastore mode requires google-cloud-datastore. Drop the "Datastore mode" claim here and in the services.json description.
| if response and len(response) > 0: | ||
| for neighbor in response[0]: | ||
| if score_threshold > 0.0 and neighbor.distance < score_threshold: | ||
| continue |
There was a problem hiding this comment.
🟡 risk: filtering out neighbor.distance < score_threshold keeps far neighbors and drops the closest ones for distance metrics (L2, cosine distance) — only correct for similarity-style metrics (dot product). Either document the metric assumption in the arg description or invert the comparison based on the index's metric.
|
|
||
| # Download to a temporary file | ||
| fd, temp_path = tempfile.mkstemp(prefix="gcs_") | ||
| os.close(fd) |
There was a problem hiding this comment.
🟡 risk: mkstemp temp files are never cleaned up — they accumulate for the life of the server, and the empty temp file also leaks when download_to_filename fails. Track created paths and remove them in endGlobal; delete the temp file in the except path.
| self.deployed_index_id = str((cfg.get('deployedIndexId') or '')).strip() | ||
|
|
||
| if not index_endpoint_id or not self.deployed_index_id: | ||
| warning("indexEndpointId and deployedIndexId are required for Vertex AI Vector Search.") |
There was a problem hiding this comment.
🔵 nit: message says "required" but this only warns and continues — node starts with index_endpoint = None and every search() call then errors. Raise here instead, matching the fail-fast behavior of the firestore/gcs beginGlobal.
2 participants
…#1515)
Summary
gcp_auth.pyandservices.common.gcp.jsonto handle both Service Account JSON keys and Application Default Credentials (ADC) fallback.db_firestore):** Created a native NoSQL database node for setting and getting documents from Google Cloud Firestore.tool_gcs):** Created a native file storage node for listing and securely downloading files from Google Cloud Storage buckets.vectordb_vertex):** Created a native vector database node to perform semantic nearest-neighbor searches against Vertex AI Index Endpoints.Type
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1515, #1511, #1512, #1513
Summary by CodeRabbit
New Features
Documentation