feat(nodes): add tool interface for vector DB operations#524
feat(nodes): add tool interface for vector DB operations#524charliegillet wants to merge 11 commits intorocketride-org:developfrom
Conversation
charliegillet
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Vector DB tool: global state for backend connections, instance-level tool functions (search, upsert, delete) with input normalization and fallback search logic, package exports, and service registration/configuration for Pinecone, ChromaDB, and Qdrant. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent/Caller
participant IInstance as IInstance Tool
participant IGlobal as IGlobal (global state)
participant Store as Backend Store
Agent->>IInstance: invoke(search|upsert|delete, args)
IInstance->>IInstance: _normalize_input(args)
IInstance->>IGlobal: require store initialized
IGlobal-->>IInstance: return Store or error
alt search flow
IInstance->>Store: searchSemantic(question, doc_filter)
alt semantic success
Store-->>IInstance: hits
else semantic fails
IInstance->>Store: searchKeyword(question, doc_filter)
Store-->>IInstance: hits
end
IInstance->>IInstance: apply score_threshold, truncate to top_k
IInstance-->>Agent: {results, total}
else upsert flow
IInstance->>IInstance: build Doc objects with metadata
IInstance->>Store: addChunks(docs)
Store-->>IInstance: ack
IInstance-->>Agent: {success, count, skipped}
else delete flow
IInstance->>Store: remove(object_ids)
Store-->>IInstance: deleted_count
IInstance-->>Agent: {success, deleted_count}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/tool_vectordb/IGlobal.py`:
- Around line 103-113: The validateConfig method contains unnecessary extra
parentheses around expressions like cfg.get('backend') and
cfg.get('serverName'); in validateConfig (and mirroring beginGlobal) remove the
redundant parentheses so backend = str((cfg.get('backend') or
'')).strip().lower() becomes backend = str(cfg.get('backend') or
'').strip().lower() and similarly simplify server_name =
str((cfg.get('serverName') or '')).strip() to server_name =
str(cfg.get('serverName') or '').strip(), keeping the same use of
Config.getNodeConfig, _BACKEND_MODULES checks, and warning(...) calls.
- Around line 58-65: The assignments to variables like server_name, backend,
collection_description, enable_search, enable_upsert, enable_delete,
default_top_k, and score_threshold contain unnecessary extra parentheses around
the cfg.get(...) expressions; remove the redundant outer parentheses so each
line uses cfg.get(...) directly (e.g., server_name = str(cfg.get('serverName')
or 'vectordb').strip(), backend = str(cfg.get('backend') or
'pinecone').strip().lower(), etc.) while keeping the existing casts, defaults,
and method calls intact.
- Around line 93-101: The _create_store static method lacks a return type;
update its signature to return DocumentStoreBase and add the import for
DocumentStoreBase (from ai.common.store import DocumentStoreBase) at the top of
the file so the method signature becomes def _create_store(... ) ->
DocumentStoreBase: and remove the noqa ANN205 suppression; keep the dynamic
import/instantiation logic (module_path, importlib.import_module, mod.Store)
unchanged.
- Around line 73-75: The call to getConnConfig() inside IGlobal will raise an
AttributeError because getConnConfig is implemented only on IGlobalTransform;
fix by adding getConnConfig to IGlobal (implement the method) or make IGlobal
inherit from IGlobalTransform, or move getConnConfig up into IGlobalBase so both
IGlobal and IGlobalTransform share it; update references in IGlobal (where bag,
conn_config = self.getConnConfig(), and store = self._create_store(...)) to use
the chosen shared method name and signature so the call resolves at runtime.
In `@nodes/src/nodes/tool_vectordb/services.json`:
- Around line 11-15: Update the service description to match actual supported
backends by IGlobal.py: change the JSON "description" text in services.json to
list only pinecone, chroma, and qdrant (remove Weaviate and pgvector) so it
aligns with the _BACKEND_MODULES definition in IGlobal.py; ensure the wording
still conveys a generic vector DB tool but references the exact supported
backends (pinecone, chroma, qdrant) and/or say "and other supported backends
configured in _BACKEND_MODULES" for future-proofing.
In `@nodes/src/nodes/tool_vectordb/vectordb_driver.py`:
- Around line 309-315: The current loop over raw_docs silently skips invalid
entries (variables: raw, content, object_id) and only counts successful
documents; modify the surrounding function that processes raw_docs to collect
and return metadata about skipped documents (e.g., a skipped count and/or list
of skipped object_ids with reasons like "missing content" or "missing
object_id"), augment the return value or result object to include this skipped
info alongside the successful count, and ensure callers of this function (the
code using the function in vectordb_driver.py) are updated to handle the new
skipped field for observability and logging.
- Around line 265-274: The fallback from semantic to keyword search in
vectordb_driver (within the try/except around self._store.searchSemantic and the
subsequent self._store.searchKeyword call) is an expected resilience behavior,
so change the current warning(f'tool_vectordb: semantic search failed ({e}),
trying keyword search') to a debug-level log (e.g., logger.debug or debug(...))
including the exception details and context; update the log call used in this
module (replace warning(...) with the module's debug logger) so expected
fallbacks don’t emit warnings while still capturing the exception for
troubleshooting.
- Around line 367-378: The JSON parse currently swallows all exceptions leading
to a vague later warning; in the JSON parse block in vectordb_driver.py (the
code handling "if isinstance(input_obj, str):" before the type-check), catch the
exception as e and emit a warning that includes the exception message and a
truncated version of the original input string so you can debug parse errors
(e.g., "tool_vectordb: failed to parse JSON: {e}; input: {input_snippet}"), then
continue the existing flow so the function still returns {} for non-dict inputs.
🪄 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: f29473b3-4297-40a7-ac99-bf41eff9fb20
📒 Files selected for processing (5)
nodes/src/nodes/tool_vectordb/IGlobal.pynodes/src/nodes/tool_vectordb/IInstance.pynodes/src/nodes/tool_vectordb/__init__.pynodes/src/nodes/tool_vectordb/services.jsonnodes/src/nodes/tool_vectordb/vectordb_driver.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
nodes/src/nodes/tool_vectordb/IGlobal.py (1)
93-101: 🧹 Nitpick | 🔵 TrivialConsider adding the return type annotation.
The
# noqa: ANN205suppression is still present. Adding the return type would improve type safety without requiring significant changes.📝 Proposed fix
Add the import at the top of the file:
from ai.common.store import DocumentStoreBaseThen update the method signature:
`@staticmethod` - def _create_store(backend: str, conn_config: dict, bag: dict): # noqa: ANN205 + def _create_store(backend: str, conn_config: dict, bag: dict) -> DocumentStoreBase: """Dynamically import and instantiate the backend Store class."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/tool_vectordb/IGlobal.py` around lines 93 - 101, The _create_store static method lacks a return type annotation; import DocumentStoreBase from ai.common.store and update the method signature of _create_store to return DocumentStoreBase (e.g., def _create_store(... ) -> DocumentStoreBase:), so callers and type checkers know the concrete store interface provided by Store class instantiation; keep the body as-is but you can safely remove the "# noqa: ANN205" suppression after adding the annotation.nodes/src/nodes/tool_vectordb/vectordb_driver.py (2)
303-335: 🧹 Nitpick | 🔵 TrivialUpsert silently skips invalid documents without feedback.
Invalid documents (non-dict, missing content/object_id) are silently skipped. The agent receives
countof successful documents but no indication of which documents failed validation or why. This could make debugging difficult for agents.♻️ Proposed improvement for observability
def _invoke_upsert(self, args: Dict[str, Any]) -> Dict[str, Any]: raw_docs = args.get('documents', []) if not isinstance(raw_docs, list) or not raw_docs: raise ValueError('upsert requires a non-empty "documents" array') docs: List[Doc] = [] + skipped: List[Dict[str, Any]] = [] for raw in raw_docs: if not isinstance(raw, dict): + skipped.append({'reason': 'not a dict'}) continue content = str(raw.get('content', '')).strip() object_id = str(raw.get('object_id', '')).strip() if not content or not object_id: + skipped.append({'object_id': object_id or None, 'reason': 'missing content or object_id'}) continue # ... rest of processing ... self._store.addChunks(docs) - return {'success': True, 'count': len(docs)} + return {'success': True, 'count': len(docs), 'skipped': len(skipped)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/tool_vectordb/vectordb_driver.py` around lines 303 - 335, In _invoke_upsert, don't silently skip invalid entries; instead validate each raw_docs entry (using the existing checks for dict, non-empty content and object_id), collect failure records (e.g., index and reason like "not a dict", "missing content", "missing object_id") while building Doc/DocMetadata objects, call self._store.addChunks on the valid docs as now, and include the failure details in the returned dict (e.g., {'success': True, 'count': n_valid, 'failed': [ {index: i, reason: "..."} ]}); optionally also emit structured logs via any existing logger on self (e.g., self._logger) with the same failure list to aid debugging.
265-274: 🧹 Nitpick | 🔵 TrivialConsider using debug-level logging for expected fallback path.
The semantic-to-keyword search fallback is an expected resilience pattern. Logging at
warninglevel may create noise in production when embeddings aren't configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/tool_vectordb/vectordb_driver.py` around lines 265 - 274, The fallback from semantic to keyword search in vectordb_driver.py is an expected behavior, so replace the current warning-level log call inside the except for self._store.searchSemantic with a debug-level log; update the call that currently uses warning(f'tool_vectordb: semantic search failed ({e}), trying keyword search') to use your module logger.debug (or equivalent) and include the exception details and context, leaving the subsequent try/except for self._store.searchKeyword intact so failures still raise RuntimeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/tool_vectordb/vectordb_driver.py`:
- Around line 276-301: The code currently sets 'total' to len(results) after
applying score filtering but returns results[:top_k], causing 'total' to
potentially not match the number of returned items; in vectordb_driver.py update
the return to either set 'total' = len(results[:top_k]) when you want total to
represent the number of items returned, or (preferred for clarity) expose both
counts by returning {'results': results[:top_k], 'total_filtered': len(results),
'total_returned': len(results[:top_k])} so callers can see the pre-slice count
(total_filtered) and the actual returned count (total_returned); adjust any
callers of this function if you change field names.
---
Duplicate comments:
In `@nodes/src/nodes/tool_vectordb/IGlobal.py`:
- Around line 93-101: The _create_store static method lacks a return type
annotation; import DocumentStoreBase from ai.common.store and update the method
signature of _create_store to return DocumentStoreBase (e.g., def
_create_store(... ) -> DocumentStoreBase:), so callers and type checkers know
the concrete store interface provided by Store class instantiation; keep the
body as-is but you can safely remove the "# noqa: ANN205" suppression after
adding the annotation.
In `@nodes/src/nodes/tool_vectordb/vectordb_driver.py`:
- Around line 303-335: In _invoke_upsert, don't silently skip invalid entries;
instead validate each raw_docs entry (using the existing checks for dict,
non-empty content and object_id), collect failure records (e.g., index and
reason like "not a dict", "missing content", "missing object_id") while building
Doc/DocMetadata objects, call self._store.addChunks on the valid docs as now,
and include the failure details in the returned dict (e.g., {'success': True,
'count': n_valid, 'failed': [ {index: i, reason: "..."} ]}); optionally also
emit structured logs via any existing logger on self (e.g., self._logger) with
the same failure list to aid debugging.
- Around line 265-274: The fallback from semantic to keyword search in
vectordb_driver.py is an expected behavior, so replace the current warning-level
log call inside the except for self._store.searchSemantic with a debug-level
log; update the call that currently uses warning(f'tool_vectordb: semantic
search failed ({e}), trying keyword search') to use your module logger.debug (or
equivalent) and include the exception details and context, leaving the
subsequent try/except for self._store.searchKeyword intact so failures still
raise RuntimeError.
🪄 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: 336c2f3b-8dec-4af3-85ba-d1940c7cd0f8
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/vectordb.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
nodes/src/nodes/tool_vectordb/IGlobal.pynodes/src/nodes/tool_vectordb/services.jsonnodes/src/nodes/tool_vectordb/vectordb_driver.py
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thank you for this contribution — the ToolsBase pattern is correctly implemented,
the tool descriptors are well-formed, _normalize_input handles all the edge cases
cleanly, and defaulting upsert/delete to disabled is the right security posture.
A few issues need to be resolved before merge
…ctordb - Remove invalid topK param from QuestionText constructor; use doc_filter.limit to control result count instead - Add embedding warning in upsert and document the limitation in the tool description - Read backend connection config from bag first, fall back to own connConfig - Distinguish disabled-tool vs unknown-tool error messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all review feedback:
Thanks for the thorough review! |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks for the contributionю
Needs to be rebased on #599's pattern before this can merge.
PR #599 (da19f6b) retired the DriverClass(ToolsBase) pattern for all tool nodes and replaced it with @tool_function decorators directly on IInstance. VectorDBDriver, vectordb_driver.py, and the IInstance.invoke() → driver.handle_invoke() chain follow the deprecated approach. See tool_firecrawl/IInstance.py or tool_http_request/IInstance.py for the current reference.
…ctordb - Remove invalid topK param from QuestionText constructor; use doc_filter.limit to control result count instead - Add embedding warning in upsert and document the limitation in the tool description - Read backend connection config from bag first, fall back to own connConfig - Distinguish disabled-tool vs unknown-tool error messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charliegillet
force-pushed
the
feature/vectordb-tool-interface
branch
from
f496765 to
074fa70
Compare
|
Rebased on develop and migrated to the @tool_function decorator pattern from PR #599. Removed the VectorDBDriver(ToolsBase) approach and moved all tool logic to @tool_function methods on IInstance, following tool_firecrawl as reference. Ready for re-review. Thanks for the guidance! |
|
Addressing @asclearuc's inline feedback:
|
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 `@nodes/src/nodes/tool_vectordb/IInstance.py`:
- Around line 162-182: The upsert path in IInstance.py currently only accepts
content, object_id, and metadata and never attaches embeddings to the Doc it
constructs (affecting the upsert handler and Doc creation logic around the
upsert method and its Doc builder), so either add an "embeddings" (or
"embedding") array field to the input schema (mirroring
'documents[].items.properties') and set that vector on the created Doc instance,
or detect backends that require caller-supplied vectors and raise a clear
validation error during request handling; update the schema
(documents.items.properties) to include embeddings:
{'type':'array','items':{'type':'number'}} and ensure the code that builds the
Doc (the upsert function / Doc constructor call) assigns doc.embedding =
provided_embedding, otherwise throw a fast-fail error when
backend_requires_external_embeddings is true.
- Around line 62-66: The schema advertises arbitrary metadata via 'filter' and
'documents[*].metadata' but the implementation only persists/filters keys
objectId, nodeId, parent, chunkId; update the code so metadata is
round-trippable and filterable end-to-end: modify the document
serialization/deserialization paths that currently strip metadata (the code
handling documents' metadata / read/write payloads at the sections corresponding
to 62-66, 105-117, 176-180, 216-223) to preserve the full metadata dict
(copy/save every key/value), and update the filter-matching logic to iterate all
provided filter keys and match against the stored metadata rather than only the
four hardcoded keys; alternatively, if you prefer the narrower approach, change
the JSON schemas for 'filter' and 'documents[*].metadata' to list only the
supported keys (objectId, nodeId, parent, chunkId) so the contract matches the
implementation.
🪄 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: e1019b0c-fb7b-4776-ae90-56f673ea31ff
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/vectordb.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
nodes/src/nodes/tool_vectordb/IGlobal.pynodes/src/nodes/tool_vectordb/IInstance.pynodes/src/nodes/tool_vectordb/__init__.pynodes/src/nodes/tool_vectordb/services.json
| 'filter': { | ||
| 'type': 'object', | ||
| 'description': 'Optional metadata filter. Keys are metadata field names, values are the required values. Example: {"nodeId": "my-node", "parent": "/docs"}', | ||
| 'additionalProperties': True, | ||
| }, |
There was a problem hiding this comment.
The metadata contract is broader than the implementation.
filter is documented as arbitrary metadata keys, and documents[*].metadata is documented as arbitrary key/value data, but the code only reads/writes objectId, nodeId, parent, and chunkId. Every other metadata field is silently discarded, so agents cannot round-trip or filter on the metadata this tool advertises. Either narrow the schemas to the supported keys or persist/filter arbitrary metadata end to end.
Also applies to: 105-117, 176-180, 216-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/src/nodes/tool_vectordb/IInstance.py` around lines 62 - 66, The schema
advertises arbitrary metadata via 'filter' and 'documents[*].metadata' but the
implementation only persists/filters keys objectId, nodeId, parent, chunkId;
update the code so metadata is round-trippable and filterable end-to-end: modify
the document serialization/deserialization paths that currently strip metadata
(the code handling documents' metadata / read/write payloads at the sections
corresponding to 62-66, 105-117, 176-180, 216-223) to preserve the full metadata
dict (copy/save every key/value), and update the filter-matching logic to
iterate all provided filter keys and match against the stored metadata rather
than only the four hardcoded keys; alternatively, if you prefer the narrower
approach, change the JSON schemas for 'filter' and 'documents[*].metadata' to
list only the supported keys (objectId, nodeId, parent, chunkId) so the contract
matches the implementation.
There was a problem hiding this comment.
Fixed — narrowed the metadata schema from additionalProperties: true to explicitly listing only the recognized fields (nodeId, parent, chunkId) with additionalProperties: false. The description also now states that unrecognized keys are not accepted.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 17 seconds before sending another message.
| 'documents': { | ||
| 'type': 'array', | ||
| 'description': 'Array of documents to upsert.', | ||
| 'items': { | ||
| 'type': 'object', | ||
| 'properties': { | ||
| 'content': { | ||
| 'type': 'string', | ||
| 'description': 'The text content of the document.', | ||
| }, | ||
| 'object_id': { | ||
| 'type': 'string', | ||
| 'description': 'Unique identifier for the document. Used for deduplication.', | ||
| }, | ||
| 'metadata': { | ||
| 'type': 'object', | ||
| 'description': 'Optional metadata key-value pairs to store with the document.', | ||
| 'additionalProperties': True, | ||
| }, | ||
| }, | ||
| 'required': ['content', 'object_id'], |
There was a problem hiding this comment.
upsert cannot carry embeddings today.
The schema only accepts content, object_id, and metadata, and this method never sets an embedding on the Doc it builds. Line 233 is therefore warning about a field the tool cannot actually accept, and backends that require caller-supplied vectors can never make these inserts searchable. Either accept embeddings in the tool input and attach them to Doc, or fail fast when the backend cannot embed on ingest.
Also applies to: 194-195, 224-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/src/nodes/tool_vectordb/IInstance.py` around lines 162 - 182, The
upsert path in IInstance.py currently only accepts content, object_id, and
metadata and never attaches embeddings to the Doc it constructs (affecting the
upsert handler and Doc creation logic around the upsert method and its Doc
builder), so either add an "embeddings" (or "embedding") array field to the
input schema (mirroring 'documents[].items.properties') and set that vector on
the created Doc instance, or detect backends that require caller-supplied
vectors and raise a clear validation error during request handling; update the
schema (documents.items.properties) to include embeddings:
{'type':'array','items':{'type':'number'}} and ensure the code that builds the
Doc (the upsert function / Doc constructor call) assigns doc.embedding =
provided_embedding, otherwise throw a fast-fail error when
backend_requires_external_embeddings is true.
There was a problem hiding this comment.
Acknowledged — the schema intentionally does not accept an embedding field in the upsert input. Embeddings should be computed by the backend on ingest or by an upstream embedding node in the pipeline. The tool description and a runtime warning both make this explicit.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 15 seconds before sending another message.
Senior Review — PR #524: feat(nodes): add tool interface for vector DB@charliegillet, solid approach here with the multi-backend architecture. Supporting multiple vector DB backends through a common interface is the right design. There are 8 CodeRabbit items that asclearuc has delegated for you to address. Here are the key ones to prioritize: Priority items
Address the full set of CodeRabbit items and this will be in good shape for a follow-up review. |
🚀 Merge RequestGood multi-backend approach (Pinecone, ChromaDB, Qdrant). Before merge (CodeRabbit items per asclearuc):
@asclearuc delegated to CodeRabbit — addressing all 8 items would clear this for merge. |
Introduces a generic tool_vectordb node that exposes vector database operations (search, upsert, delete) as ToolsBase-compatible tools that LLM agents can invoke. The node works with multiple vector DB backends (Pinecone, ChromaDB, Qdrant) through the shared DocumentStoreBase interface, providing a unified tool abstraction for semantic search. Closes rocketride-org#442 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ig() IGlobalBase does not have getConnConfig() — that method is on IGlobalTransform. Use self.glb.connConfig which is the standard pattern for tool nodes (matches tool_http_request, tool_firecrawl). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove Weaviate and pgvector from services.json description since _BACKEND_MODULES only supports pinecone, chroma, and qdrant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Database cylinder icon matching the 24px viewBox, fill="currentColor" pattern used by other node icons (e.g. http.svg). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_normalize_input previously swallowed JSON parse errors silently, making it hard to diagnose bad tool inputs. Now logs a warning with the parse error before returning an empty dict. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clean up double-wrapped parens like str((cfg.get('x') or 'y')) to
the simpler str(cfg.get('x') or 'y').
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ctordb - Remove invalid topK param from QuestionText constructor; use doc_filter.limit to control result count instead - Add embedding warning in upsert and document the limitation in the tool description - Read backend connection config from bag first, fall back to own connConfig - Distinguish disabled-tool vs unknown-tool error messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rocketride-org#599 Remove VectorDBDriver(ToolsBase) and move search/upsert/delete tool logic to @tool_function decorated methods on IInstance. Store config and backend connection directly on IGlobal instead of through a driver object. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add return type annotation to IGlobal._create_store() - Downgrade semantic-to-keyword fallback log from warning to debug - Fix total count to match actual returned results after top_k slice - Return skipped document count from upsert operation - Narrow metadata schema to document only recognized fields (nodeId, parent, chunkId) instead of additionalProperties: true Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charliegillet
force-pushed
the
feature/vectordb-tool-interface
branch
from
074fa70 to
255cc85
Compare
|
No description provided. |
|
@asclearuc Thanks for the thorough reviews! All feedback has been addressed: Re: second review (rebase on #599): Summary of changes in this push:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nodes/src/nodes/tool_vectordb/IInstance.py (2)
62-66:⚠️ Potential issue | 🟠 MajorNarrow
filterto the keys you actually honor.The schema accepts arbitrary metadata keys, but Lines 108-117 only translate
objectId,nodeId, andparentintoDocFilter. Any other key, includingchunkId, is silently ignored, so a valid-looking filter can still return unfiltered results. Either enumerate the supported keys in the schema or map every accepted key end to end.Also applies to: 105-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/tool_vectordb/IInstance.py` around lines 62 - 66, The JSON schema for the 'filter' field currently allows arbitrary keys but the translation code only handles specific keys when building a DocFilter (the keys mapped are objectId, nodeId, parent); either restrict the schema to enumerate only the keys you actually support (set 'additionalProperties' to false and add a 'properties' entry listing supported keys like objectId, nodeId, parent) or extend the translation logic that constructs the DocFilter to handle every key the schema permits (e.g., add mapping for chunkId and any other accepted metadata keys) so the schema and the DocFilter construction in IInstance.py stay consistent.
163-202:⚠️ Potential issue | 🟠 Major
upsertstill has no way to accept embeddings.Every
Docbuilt at Lines 232-235 only getspage_contentandmetadata, so the warning at Line 245 fires on every successful call. Backends that require caller-supplied vectors will ingest documents that this tool can never make searchable. Add anembeddingfield todocuments[*]and assign it toDoc, or fail fast when the selected backend cannot embed on ingest.Also applies to: 224-246
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/tool_vectordb/IInstance.py`:
- Around line 296-317: The current _normalize_input logic only unwraps
dict-valued 'input' but skips cases like {'input': '{"query":"x"}'} or {'input':
SomeModel(...)}, so re-run the same normalization on input_obj['input'] before
merging extras: detect when 'input' key exists, take inner = input_obj['input'],
call the same normalization sequence used earlier (model_dump()/dict conversion
and json.loads parsing) on inner to yield a dict, then merge extras = {k:v for
k,v in input_obj.items() if k != 'input'} and set input_obj =
{**inner_normalized, **extras}; ensure you still validate resulting input_obj is
a dict and preserve existing warning/return behavior in _normalize_input.
---
Duplicate comments:
In `@nodes/src/nodes/tool_vectordb/IInstance.py`:
- Around line 62-66: The JSON schema for the 'filter' field currently allows
arbitrary keys but the translation code only handles specific keys when building
a DocFilter (the keys mapped are objectId, nodeId, parent); either restrict the
schema to enumerate only the keys you actually support (set
'additionalProperties' to false and add a 'properties' entry listing supported
keys like objectId, nodeId, parent) or extend the translation logic that
constructs the DocFilter to handle every key the schema permits (e.g., add
mapping for chunkId and any other accepted metadata keys) so the schema and the
DocFilter construction in IInstance.py stay 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42923ac3-1a30-49de-bd9d-d9db31c6eae3
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/vectordb.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
nodes/src/nodes/tool_vectordb/IGlobal.pynodes/src/nodes/tool_vectordb/IInstance.pynodes/src/nodes/tool_vectordb/__init__.pynodes/src/nodes/tool_vectordb/services.json
| if hasattr(input_obj, 'model_dump') and callable(getattr(input_obj, 'model_dump')): | ||
| input_obj = input_obj.model_dump() | ||
| elif hasattr(input_obj, 'dict') and callable(getattr(input_obj, 'dict')): | ||
| input_obj = input_obj.dict() | ||
|
|
||
| if isinstance(input_obj, str): | ||
| try: | ||
| parsed = json.loads(input_obj) | ||
| if isinstance(parsed, dict): | ||
| input_obj = parsed | ||
| except (json.JSONDecodeError, ValueError) as exc: | ||
| warning(f'tool_vectordb: malformed JSON input, returning empty dict: {exc}') | ||
| return {} | ||
|
|
||
| if not isinstance(input_obj, dict): | ||
| warning(f'tool_vectordb: unexpected input type {type(input_obj).__name__}') | ||
| return {} | ||
|
|
||
| if 'input' in input_obj and isinstance(input_obj['input'], dict): | ||
| inner = input_obj['input'] | ||
| extras = {k: v for k, v in input_obj.items() if k != 'input'} | ||
| input_obj = {**inner, **extras} |
There was a problem hiding this comment.
Normalize wrapped payloads before unwrapping them.
Only a dict-valued input is unwrapped here. {'input': '{"query": "x"}'} and {'input': SomeModel(...)} skip the top-level json.loads() / model_dump() path, so the tool never sees query, documents, or object_ids. Re-run _normalize_input() on input_obj['input'] before merging extras.
🛠️ Possible fix
- if 'input' in input_obj and isinstance(input_obj['input'], dict):
- inner = input_obj['input']
- extras = {k: v for k, v in input_obj.items() if k != 'input'}
- input_obj = {**inner, **extras}
+ if 'input' in input_obj:
+ inner = _normalize_input(input_obj['input'])
+ extras = {k: v for k, v in input_obj.items() if k != 'input'}
+ input_obj = {**inner, **extras}🧰 Tools
🪛 Ruff (0.15.9)
[warning] 296-296: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 298-298: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/src/nodes/tool_vectordb/IInstance.py` around lines 296 - 317, The
current _normalize_input logic only unwraps dict-valued 'input' but skips cases
like {'input': '{"query":"x"}'} or {'input': SomeModel(...)}, so re-run the same
normalization on input_obj['input'] before merging extras: detect when 'input'
key exists, take inner = input_obj['input'], call the same normalization
sequence used earlier (model_dump()/dict conversion and json.loads parsing) on
inner to yield a dict, then merge extras = {k:v for k,v in input_obj.items() if
k != 'input'} and set input_obj = {**inner_normalized, **extras}; ensure you
still validate resulting input_obj is a dict and preserve existing
warning/return behavior in _normalize_input.
There was a problem hiding this comment.
Fixed in commit a1dd7d0. The normalization helper now recurses through _normalize_vectordb_tool_input on the nested input value (after the outer model_dump/json.loads/dict path), so {'input': '{"query": "x"}'} and {'input': SomeModel(...)} are handled the same way as {'input': {'query': 'x'}}. The outer extras still take precedence when merged on top of the normalized inner dict.
Note: tool_vectordb/IInstance.py has been deleted entirely — the normalization helper now lives in packages/ai/src/ai/common/store.py as _normalize_vectordb_tool_input, shared by the VectorStoreToolMixin methods that were inlined into the pinecone/chroma/qdrant nodes per @Rod-Christensen's architectural feedback. Verified against the original regression cases (JSON-string-wrapped, model-wrapped, and dict+extras wrappers).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@CodeRabbit - recheck. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22266
✅ Resolved (4/6)
❌ Still Open (2/6)Item 2 — Every other node that has external dependencies ships a If backend deps are optional (not always needed), they should be listed as commented extras or in a separate Item 4 — Test config block missing from The "preconfig": {
"default": "default",
"profiles": {
"default": { "serverName": "vectordb", "backend": "pinecone" }
}
}There is no "test": {
"title": "Test (mock)",
"serverName": "vectordb-test",
"backend": "chroma",
"topK": 5,
"scoreThreshold": 0.0
}(The exact backend and values should match whatever mock/local fixture is used by the project's test harness.) Summary: 4 of 6 items are clean. The two blockers before merge are the missing |
|
Hey Charlie, This is a great idea! Have you consider just adding the tool definitions to the underlying store.py? This would enable any vectordb to morph into a store directly without changing any of them or having a single generic node as all vectordb drivers inherit from store. |
…ew feedback
Rod-Christensen suggested co-locating the tool definitions with the
underlying store so every vectordb node can "morph into a tool" without
a separate generic wrapper node.
The engine dispatches ``invoke`` against the node's ``IInstance`` (not
the ``DocumentStoreBase`` subclass, which is composed via
``self.IGlobal.store``), so a mixin that lives next to
``DocumentStoreBase`` in ``store.py`` and is inherited by the vectordb
``IInstance`` classes achieves the spirit of the suggestion while being
technically correct.
Changes:
- Add ``VectorStoreToolMixin`` to ``packages/ai/src/ai/common/store.py``
with ``@tool_function`` decorated ``search``, ``upsert`` and
``delete`` methods that delegate to ``self.IGlobal.store``. The mixin
is picked up automatically by ``IInstanceBase._collect_tool_methods``
via MRO walk.
- Have ``pinecone``, ``chroma`` and ``qdrant`` ``IInstance`` classes
inherit from ``VectorStoreToolMixin`` so each vectordb node exposes
the search/upsert/delete tools natively.
- Add ``"tool"`` to ``classType`` and ``"invoke"`` to ``capabilities``
in each node's ``services.json`` so ``getControllerNodeIds('tool')``
discovers them as tool nodes.
- Delete the ``tool_vectordb`` node entirely — obsolete now that every
vectordb node has native tool capability.
- Delete the unused ``vectordb.svg`` icon that existed only for the
now-deleted ``tool_vectordb`` node.
- Fix the wrapped-payload normalization bug flagged in CodeRabbit
comment 3049766102: ``{'input': '{"query": "x"}'}`` and
``{'input': SomeModel(...)}`` now recurse through
``_normalize_vectordb_tool_input`` on the inner value, so string-,
model- and dict-wrapped payloads all receive the same
JSON-parse/``model_dump`` treatment before merging outer extras.
Other vectordb connectors (astra_db, milvus, weaviate, atlas,
vectordb_postgres, index_search) can opt in to the same tool interface
with a one-line change to their ``IInstance.py`` plus the services.json
update — out of scope for this PR.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
module:ai
|
@Rod-Christensen Great suggestion — I've refactored along those lines in commit a1dd7d0. What changed Added The Why a mixin instead of putting the tool methods directly on I investigated doing exactly what you described and ran into a wiring subtlety: the engine dispatches The mixin approach keeps the tool interface co-located with the store in class IInstance(VectorStoreToolMixin, IInstanceTransform):plus the Out of scope / follow-up The other vectordb connectors ( |
Address 3 blocking issues on PR rocketride-org#524: 1. Tool name collisions: adopt the established <serverName>.<toolName> convention used by mcp_client. VectorStoreToolMixin now overrides _collect_tool_methods to prefix each tool with a per-node serverName sourced from IGlobal.serverName (user-configurable via services.json). Two pinecone/chroma/qdrant instances in the same pipeline no longer collide on bare names like "search"/"upsert"/"delete". 2. Broken pinecone semantic search: the mixin was calling searchSemantic with an unset embedding, which raises in every backend (pinecone/ chroma/qdrant) and silently fell through to keyword search. Add an IGlobal.embed_query hook so nodes can plug in an embedder for the control-plane tool path; when no embedder is bound, go straight to keyword search with a one-shot warning and update the tool description so agents know the mode. Unexpected semantic errors still fall back to keyword search with a clearer warning message. 3. No tests for VectorStoreToolMixin: add nodes/test/test_vectordb_tool_mixin.py (28 tests) covering input normalization (dict/JSON/pydantic v1+v2/ nested input wrapper/security_context stripping/malformed payloads), namespaced tool method collection including the collision-avoidance scenario, and search/upsert/delete dispatch through a fake store plus keyword-fallback, semantic-fallback, and missing-store error paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Devil's advocate blockers — all addressed in 4ef3312Thanks for the thorough review. All three blockers are fixed on Blocker 1 — Tool name collisions (namespacing)Adopted the established
Two pinecone instances (or any mix of vectordb nodes) in the same pipeline can now be named e.g. Blocker 2 — Broken semantic searchYou were right: Chose a hybrid of Option A + Option C:
This avoids forcing every existing pipeline to hard-fail on the very first tool call while giving operators a clear, actionable path to enable true semantic ranking. Blocker 3 — Zero testsAdded Coverage:
Commit: 4ef3312 |
|
Latest fixes (commit 4ef3312, pushed to
Files touched:
|
4 participants
Summary
tool_vectordbnode that provides a generic, ToolsBase-compatible tool interface for vector database operations (search, upsert, delete)DocumentStoreBaseinterfaceType
Feature
Why this feature fits this codebase
The RocketRide engine already has individual vector DB nodes for Pinecone, ChromaDB, and Qdrant, but none of them expose a unified tool interface that LLM agents can invoke through the standard
ToolsBaseprotocol. Existing tool nodes liketool_http_requestandtool_firecrawlfollow a pattern of declaringclassType: ["tool"]andcapabilities: ["invoke"]inservices.json, implementingIGlobal.beginGlobal()for driver setup, and delegatingIInstance.invoke()to a driver that extendsToolsBase. This PR replicates that exact pattern for vector DB operations. TheVectorDBDriverclass invectordb_driver.pyextendsToolsBaseand implements the three required hooks --_tool_query(),_tool_validate(), and_tool_invoke()-- so the agent framework can discover tools like<server>.searchvia the same MCP tool descriptor protocol used by every other tool node. TheIGlobal._create_store()method dynamically imports the backend'sStoreclass fromnodes.pinecone.pinecone,nodes.chroma.chroma, ornodes.qdrant.qdrant, reusing the existingDocumentStoreBaseimplementations rather than duplicating backend-specific code.What changed
nodes/src/nodes/tool_vectordb/services.json-- Node registration withclassType: ["tool"],capabilities: ["invoke"], configurable fields for backend, server name, enabled operations, topK, and score thresholdnodes/src/nodes/tool_vectordb/IGlobal.py-- Reads node config, resolves the backend Store class via dynamic import, and instantiatesVectorDBDrivernodes/src/nodes/tool_vectordb/IInstance.py-- Delegatesinvoke()calls toVectorDBDriver.handle_invoke()nodes/src/nodes/tool_vectordb/vectordb_driver.py-- ImplementsToolsBasewith_tool_query(returns tool descriptors),_tool_validate(schema validation), and_tool_invoke(routes to_invoke_search,_invoke_upsert,_invoke_delete). Includes_normalize_input()helper for Pydantic models, JSON strings, and nested wrappersnodes/src/nodes/tool_vectordb/__init__.py-- Package init exporting IGlobal and IInstancepackages/shared-ui/src/assets/nodes/vectordb.svg-- Node iconValidation
classType: ["tool"]andcapabilities: ["invoke"]tool.queryreturns tool descriptors with correct input schemastool.invokefor search returns ranked results with content, score, and metadatatool.queryresultsruff format --checkandruff checkon changed filesHow this could be extended
The same
VectorDBDriverpattern could be extended to support additional backends (e.g., Weaviate, Milvus) by adding entries to the_BACKEND_MODULESmap and ensuring the new backend implementsDocumentStoreBase. The tool descriptors could also be enriched with collection-specific metadata for better agent discovery in multi-collection deployments.Closes #442
#Hack-with-bay-2
Summary by CodeRabbit