Repository navigation
feat(nodes): add Cohere Rerank pipeline node - #898
stepmikhaylov merged 7 commits into
Conversation
github-actions
Bot
added
module:nodes
|
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:
📝 WalkthroughWalkthroughThis pull request introduces a complete Cohere document reranking node for RocketRide. The implementation includes a rerank client wrapper, global lifecycle management, instance-level question processing, service definition with profiles, comprehensive mocking for tests, a full test suite, and UI icon registration. ChangesCohere Rerank Node
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
|
No description provided. |
7081de9 to
475c9b5
Compare
![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/rerank_cohere/IGlobal.py`:
- Around line 58-66: The validateConfig logic (in IGlobal.py's validateConfig
handling of apikey and model) currently allows non-string or whitespace-only
values; update it to first ensure config.get('apikey') and config.get('model')
are instances of str and that apikey.strip() and model.strip() are non-empty; if
apikey is missing, not a str, or only whitespace, call warning('Cohere API key
is required') and return; if model is missing, not a str, or only whitespace,
call warning('Cohere model name must not be empty') and return; use the same
symbol names (apikey, model) so the checks replace the current loose truthiness
checks.
In `@nodes/src/nodes/rerank_cohere/rerank_client.py`:
- Around line 96-99: Boolean values currently pass the numeric isinstance checks
for _top_n and _min_score, so update those validations to explicitly exclude
bools: for _top_n in the constructor/validator change the condition to reject if
not (isinstance(self._top_n, int) and not isinstance(self._top_n, bool)) or
self._top_n < 1 (or use type(self._top_n) is int), and for _min_score change to
reject if not (isinstance(self._min_score, (int, float)) and not
isinstance(self._min_score, bool)) or not (0.0 <= self._min_score <= 1.0); apply
the same pattern to the other identical checks referenced (around the blocks at
the later occurrences) so True/False are no longer accepted as numeric values.
In `@test/nodes/test_rerank_cohere.py`:
- Around line 194-201: The test currently sets global imports by assigning
sys.modules['rocketlib']=_mock_rocketlib, sys.modules['cohere']=_mock_cohere,
sys.modules['cohere.errors']=_mock_cohere_errors, etc., which leaks mocks across
the pytest process; change these to scoped patches using
unittest.mock.patch.dict or pytest's monkeypatch (e.g., use
patch.dict(sys.modules, {...}) as a context manager or in a fixture) so that the
mappings for keys like 'rocketlib', 'ai', 'ai.common', 'ai.common.schema',
'ai.common.config', 'depends', 'cohere', and 'cohere.errors' are installed only
for the duration of the test(s) and automatically restored afterward; update
existing top-level assignments to be applied inside the test functions or a
module-scoped fixture that yields and then restores.
🪄 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: f52779e9-4d50-48f6-8a38-3d82e078c569
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/cohere.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
nodes/src/nodes/rerank_cohere/IGlobal.pynodes/src/nodes/rerank_cohere/IInstance.pynodes/src/nodes/rerank_cohere/__init__.pynodes/src/nodes/rerank_cohere/requirements.txtnodes/src/nodes/rerank_cohere/rerank_client.pynodes/src/nodes/rerank_cohere/services.jsonnodes/test/framework/pipeline.pynodes/test/mocks/cohere/__init__.pynodes/test/mocks/cohere/errors.pypackages/shared-ui/src/components/canvas/util/get-icon-path.tstest/nodes/test_rerank_cohere.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/rerank_cohere/IInstance.py`:
- Around line 54-63: The code accepts whitespace-only strings as valid
query/docs; update the extraction and validation to trim whitespace and treat
empty-after-trim as missing. Specifically, when building query_text from
question.questions (variables: query_text, first_question, first_question.text),
use .strip() on the extracted string and set query_text = '' if stripped is
empty, then raise ValueError if not query_text; apply the same change to the
analogous documents extraction block (the doc_text/document.text handling around
the other block mentioned).
In `@nodes/src/nodes/rerank_cohere/services.json`:
- Around line 2-235: The file contains JavaScript-style line comments (//) which
break strict JSON parsing; remove all leading and inline // comment lines and
convert any explanatory comment blocks into valid JSON values or external docs
so the document becomes strict JSON (e.g., keep keys like "title": "Cohere
Rerank", "protocol": "rerank_cohere://", "preconfig", "fields", "shape", "test"
intact and only remove/comment text outside JSON syntax). Ensure no trailing
commas remain and the final payload parses as JSON by validating the top-level
object that includes "title", "protocol", "classType", "preconfig.profiles",
"fields" and "shape".
🪄 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: fffff165-0c24-43ec-a3cb-44efee4bb863
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/cohere.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
nodes/src/nodes/rerank_cohere/IGlobal.pynodes/src/nodes/rerank_cohere/IInstance.pynodes/src/nodes/rerank_cohere/__init__.pynodes/src/nodes/rerank_cohere/requirements.txtnodes/src/nodes/rerank_cohere/rerank_client.pynodes/src/nodes/rerank_cohere/services.jsonnodes/test/framework/pipeline.pynodes/test/mocks/cohere/__init__.pynodes/test/mocks/cohere/errors.pypackages/shared-ui/src/components/canvas/util/get-icon-path.tstest/nodes/test_rerank_cohere.py
Adds a `rerank_cohere` pipeline node that slots between vector search and the LLM in RAG pipelines, improving answer quality by reranking retrieved documents by relevance to the query. Components: - RerankClient — wraps Cohere ClientV2 with top_n/min_score validation and an exception hierarchy (RerankAuthenticationError, RerankRateLimitError, RerankBadRequestError, RerankServerError) named for circuit-breaker substring heuristics. - IGlobal — standard validateConfig (no live API call) and beginGlobal lifecycle, with depends() for SDK auto-install. - IInstance — deep-copies question (fan-out safe), maps rerank result indices back through original document positions so metadata stays aligned when min_score filters blank docs, always emits an answer. - services.json — rerank-v3.5 (default), rerank-english-v3.0, custom profiles; top_n and min_score exposed in all preset shapes. Resubmission of the rerank portion of closed PR rocketride-org#499 per maintainer feedback: one PR per node, clean branch from develop, no unrelated file changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
475c9b5 to
e5a1e90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
nodes/src/nodes/rerank_cohere/IInstance.py (1)
53-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim query and document text before emptiness checks.
Whitespace-only strings still pass these guards, so blank queries/documents can reach the reranker instead of being rejected or filtered out locally.
Proposed fix
query_text = '' if question.questions: first_question = question.questions[0] if hasattr(first_question, 'text'): query_text = first_question.text elif isinstance(first_question, dict): query_text = first_question.get('text', '') + if isinstance(query_text, str): + query_text = query_text.strip() if not query_text: raise ValueError('No query text found in question') @@ doc_texts = [] original_indices = [] for idx, doc in enumerate(question.documents): - if hasattr(doc, 'page_content') and doc.page_content: + if ( + hasattr(doc, 'page_content') + and isinstance(doc.page_content, str) + and doc.page_content.strip() + ): doc_texts.append(doc.page_content) original_indices.append(idx) - elif isinstance(doc, dict) and doc.get('page_content'): - doc_texts.append(doc['page_content']) - original_indices.append(idx) + elif isinstance(doc, dict): + page_content = doc.get('page_content') + if isinstance(page_content, str) and page_content.strip(): + doc_texts.append(page_content) + original_indices.append(idx)🤖 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/rerank_cohere/IInstance.py` around lines 53 - 80, The guards currently treat whitespace-only strings as valid; trim query and document strings before checking emptiness. After extracting first_question.text (or dict.get('text')) set query_text = query_text.strip() and then validate (raise ValueError if empty) instead of checking raw string; likewise, when iterating question.documents in the loop that builds doc_texts and original_indices, trim each candidate (from doc.page_content or doc['page_content']) and only append if the trimmed string is non-empty (store the trimmed value), ensuring the final doc_texts check uses trimmed content. Use the existing variables query_text, doc_texts, original_indices, and the question.questions/question.documents accessors to locate the fixes.nodes/src/nodes/rerank_cohere/rerank_client.py (1)
96-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
boolvalues fortop_nandmin_score.
boolstill passes these numericisinstance(...)checks, sotop_n=Trueis accepted as1andmin_score=Falseis accepted as0.0instead of being rejected as invalid input.Proposed fix
- if not isinstance(self._top_n, int) or self._top_n < 1: + if isinstance(self._top_n, bool) or not isinstance(self._top_n, int) or self._top_n < 1: raise ValueError(f'top_n must be an integer >= 1, got {self._top_n}') - if not isinstance(self._min_score, (int, float)) or not (0.0 <= self._min_score <= 1.0): + if ( + isinstance(self._min_score, bool) + or not isinstance(self._min_score, (int, float)) + or not (0.0 <= self._min_score <= 1.0) + ): raise ValueError(f'min_score must be a number between 0.0 and 1.0, got {self._min_score}') @@ - if not isinstance(effective_top_n, int) or effective_top_n < 1: + if isinstance(effective_top_n, bool) or not isinstance(effective_top_n, int) or effective_top_n < 1: raise ValueError(f'top_n must be an integer >= 1, got {effective_top_n}') @@ - if not isinstance(effective_min_score, (int, float)) or not (0.0 <= effective_min_score <= 1.0): + if ( + isinstance(effective_min_score, bool) + or not isinstance(effective_min_score, (int, float)) + or not (0.0 <= effective_min_score <= 1.0) + ): raise ValueError(f'min_score must be a number between 0.0 and 1.0, got {effective_min_score}')Also applies to: 135-136, 188-189
🤖 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/rerank_cohere/rerank_client.py` around lines 96 - 99, The current validation for self._top_n and self._min_score incorrectly accepts bools because isinstance(True, int) and isinstance(False, int) return True; update the checks in the rerank_cohere client where self._top_n and self._min_score are validated (including the other occurrences around the blocks referencing self._top_n and self._min_score) to explicitly reject bool by adding a condition like "and not isinstance(value, bool)" (i.e., require int for top_n and int/float for min_score while ensuring the value is not a bool) and keep the existing range checks and error messages.nodes/src/nodes/rerank_cohere/IGlobal.py (1)
57-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
validateConfigbefore calling.strip().This still accepts whitespace-only API keys, and a non-string
modelfalls through to the broadexceptwith a generic attribute error instead of the intended user-facing warning. That makes config validation less useful and pushes the real failure tobeginGlobal().Proposed fix
- apikey = config.get('apikey') + apikey = config.get('apikey', '') model = config.get('model', 'rerank-v3.5') - if not apikey: + if not isinstance(apikey, str) or not apikey.strip(): warning('Cohere API key is required') return - if not model or not model.strip(): + if not isinstance(model, str) or not model.strip(): warning('Cohere model name must not be empty') return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/rerank_cohere/IGlobal.py` around lines 57 - 66, The config validation currently calls .strip() without ensuring types and accepts whitespace-only keys; update the validateConfig logic (the block using Config.getNodeConfig(self.glb.logicalType, self.glb.connConfig)) to explicitly check that apikey is a non-empty string with non-whitespace characters (e.g., ensure isinstance(apikey, str) and apikey.strip()) and that model is a string before calling model.strip(); if either check fails, call warning('Cohere API key is required') or warning('Cohere model name must not be empty') respectively and return so errors don't bubble to beginGlobal().
🤖 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/rerank_cohere/services.json`:
- Around line 97-99: The JSON defaults and profile names still reference
"rerank-v3.5" but the PR objective requires "rerank-english-v3.0"; update the
"default" value and the corresponding profile keys/objects to use
"rerank-english-v3.0" (replace occurrences of "rerank-v3.5" with
"rerank-english-v3.0" in the "default" property and the "profiles" entries in
nodes/src/nodes/rerank_cohere/services.json, including the other repeated
occurrences mentioned).
---
Duplicate comments:
In `@nodes/src/nodes/rerank_cohere/IGlobal.py`:
- Around line 57-66: The config validation currently calls .strip() without
ensuring types and accepts whitespace-only keys; update the validateConfig logic
(the block using Config.getNodeConfig(self.glb.logicalType,
self.glb.connConfig)) to explicitly check that apikey is a non-empty string with
non-whitespace characters (e.g., ensure isinstance(apikey, str) and
apikey.strip()) and that model is a string before calling model.strip(); if
either check fails, call warning('Cohere API key is required') or
warning('Cohere model name must not be empty') respectively and return so errors
don't bubble to beginGlobal().
In `@nodes/src/nodes/rerank_cohere/IInstance.py`:
- Around line 53-80: The guards currently treat whitespace-only strings as
valid; trim query and document strings before checking emptiness. After
extracting first_question.text (or dict.get('text')) set query_text =
query_text.strip() and then validate (raise ValueError if empty) instead of
checking raw string; likewise, when iterating question.documents in the loop
that builds doc_texts and original_indices, trim each candidate (from
doc.page_content or doc['page_content']) and only append if the trimmed string
is non-empty (store the trimmed value), ensuring the final doc_texts check uses
trimmed content. Use the existing variables query_text, doc_texts,
original_indices, and the question.questions/question.documents accessors to
locate the fixes.
In `@nodes/src/nodes/rerank_cohere/rerank_client.py`:
- Around line 96-99: The current validation for self._top_n and self._min_score
incorrectly accepts bools because isinstance(True, int) and isinstance(False,
int) return True; update the checks in the rerank_cohere client where
self._top_n and self._min_score are validated (including the other occurrences
around the blocks referencing self._top_n and self._min_score) to explicitly
reject bool by adding a condition like "and not isinstance(value, bool)" (i.e.,
require int for top_n and int/float for min_score while ensuring the value is
not a bool) and keep the existing range checks and error messages.
🪄 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: 8ce0e732-1aa9-4077-9a26-8771865b323b
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/cohere.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
nodes/src/nodes/rerank_cohere/IGlobal.pynodes/src/nodes/rerank_cohere/IInstance.pynodes/src/nodes/rerank_cohere/__init__.pynodes/src/nodes/rerank_cohere/requirements.txtnodes/src/nodes/rerank_cohere/rerank_client.pynodes/src/nodes/rerank_cohere/services.jsonnodes/test/framework/pipeline.pynodes/test/mocks/cohere/__init__.pynodes/test/mocks/cohere/errors.pypackages/shared-ui/src/components/canvas/util/get-icon-path.tstest/nodes/test_rerank_cohere.py
|
Heads-up on CI: the Ruff check is currently red on this PR because three pre-existing files on develop ( I've opened #900 ( This PR's code-level CI is otherwise fully green: Build / Ubuntu, Build / Windows, Build / macOS, and gitleaks all pass. |
Mirrors rocketride-org#900. These three pre-existing files on develop drifted out of `ruff format --check` after PR rocketride-org#723's cleanup; including the fix here so this PR's CI `ruff-check` (and the `CI OK` gatekeeper that depends on it) goes green without waiting for rocketride-org#900 to merge first. Pure mechanical `ruff format` output — line-break reformatting only, no semantic changes. If rocketride-org#900 merges first, this commit becomes a no-op on rebase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prior run failed on `from rocketlib import normalize_tool_input` — the symbol IS exported from rocketlib/__init__.py line 63/141 on this branch (identical to develop). Same import passes on companion PRs rocketride-org#897/rocketride-org#898/rocketride-org#899/rocketride-org#900 with the same merge of develop, so this is a stale build-cache flake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Could you please review the new commit? |
stepmikhaylov
left a comment
stepmikhaylov
left a comment
There was a problem hiding this comment.
Change Requests
-
CR-1 · Declare
experimentalcapability —services.json:22
"capabilities": []must be"capabilities": ["experimental"]. All new nodes require this flag before production exposure. Seeguardrails,tool_exa_search,embedding_videoas reference. -
CR-2 · Test file is in the wrong directory —
test/nodes/test_rerank_cohere.py
The project convention isnodes/test/<node-name>/test_all.py(seenodes/test/guardrails/,nodes/test/tool_git/). The current path is outside thebuilder nodes:testdiscovery tree, so the tests never run in CI. Move tonodes/test/rerank_cohere/test_all.py. -
CR-3 ·
sys.pathmanipulation in tests —test_rerank_cohere.py:44–46
sys.path.insert(0, str(NODES_SRC))is prohibited — all import paths are configured by the build interpreter. Remove the manual path injection entirely. -
CR-4 · Built-in modules replaced in
sys.modules—test_rerank_cohere.py:194–201
rocketlib,ai,ai.common,ai.common.schema,ai.common.config, anddependsare all injected asMagicMock()objects. These are provided by the built-in interpreter and must not be mocked. Import the real modules; restrict mocking to external SDK boundaries only (i.e., thecohereSDK client). -
CR-5 · Inline Cohere mock duplicates
nodes/test/mocks/cohere/—test_rerank_cohere.py:168–201
The test file defines its own_MockUnauthorizedError,_MockBadRequestError, etc. and injects them directly intosys.modules, bypassing the project's canonical mock already atnodes/test/mocks/cohere/. Remove the inline mock; usenodes/test/mocks/cohere/as the single source of truth. -
CR-6 · Missing
requiresinservices.jsontest block —services.json:194
Without"requires": ["ROCKETRIDE_COHERE_KEY"], runningbuilder nodes:test-fullwithout mocks will silently attempt a live API call instead of emitting a clear skip. Compare withllm_openaiwhich declares"requires": ["ROCKETRIDE_OPENAI_KEY"]. -
CR-7 ·
top_ninteger type check rejects valid JSON values —rerank_client.py:96,rerank_client.py:135
isinstance(top_n, int)fails for5.0(float), which is a valid representation of an integer in JSON and common in pipeline config round-trips. Replace with a numeric check followed byint()coercion:isinstance(top_n, (int, float)) and top_n >= 1.
|
Thank you for your efforts on the Cohere Rerank node — solid implementation with a nice regression test for the metadata alignment edge case. Could you please address the change requests above before merging? |
…v3.0 - services.json: mark capabilities=["experimental"] (stepmikhaylov CR-1) - services.json: default preset rerank-english-v3.0 (CodeRabbit nit) - services.json: add requires=["ROCKETRIDE_RERANK_COHERE_KEY"] on test block so missing keys skip dynamic tests cleanly (stepmikhaylov CR-6) - rerank_client.py: accept int|float for top_n (excluding bool) and coerce JSON 5.0 -> 5; same bool-exclusion for min_score; centralized in _coerce_top_n / _validate_min_score helpers (stepmikhaylov CR-7 + CodeRabbit non-bool int check) - IGlobal.py: validateConfig rejects non-str/whitespace-only apikey and model before .strip() (CodeRabbit) - IInstance.py: trim query/document strings before emptiness checks so whitespace-only payloads are rejected (CodeRabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per repo convention for node-specific unit tests. Stops mocking engine-level modules (rocketlib, ai.*, depends) and drops the sys.path.insert hack — IGlobal/IInstance are imported through the build interpreter that already provides them, matching guardrails and astra_db. The external cohere SDK is the only dependency mocked, sourced from the canonical nodes/test/mocks/cohere/ package. The mock is loaded via a scoped sys.path so other tests are not contaminated. Additional coverage: - top_n: accepts JSON float 5.0 (coerced to int), rejects 2.5 and bool - min_score: rejects bool (subclass of int) at init and override - validateConfig: rejects non-string apikey/model before .strip() - IInstance: whitespace-only query and whitespace-only page_content raise ValueError Addresses stepmikhaylov CR-2..CR-5 and CodeRabbit nit re patch.dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update — addressed @stepmikhaylov + CodeRabbit feedback (pushed e3b912f)stepmikhaylov CRs:
CodeRabbit nits:
Local pytest: 30 pure tests pass; 21 IGlobal/IInstance tests need the build interpreter ( Re-requesting @stepmikhaylov, @jmaionchi, @Rod-Christensen. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nodes/src/nodes/rerank_cohere/services.json (1)
2-236:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove
//comments to comply with strict JSON parsing.The file contains JavaScript-style line comments (
//) throughout, causing Biome parse failures. This issue was previously flagged and remains unresolved. Either remove all comment lines or move documentation to external files if the parser requires strict JSON.🤖 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/rerank_cohere/services.json` around lines 2 - 236, The file contains JavaScript-style line comments (//) throughout which breaks strict JSON parsing; remove all comment lines or convert them into valid JSON fields (e.g., add "_comment" properties) or move explanatory text out to docs so the JSON objects like "title", "protocol", "preconfig", "fields", and "shape" contain only valid JSON members—ensure every occurrence of leading // before description blocks and inline comments is deleted so nodes/src/nodes/rerank_cohere/services.json becomes strictly valid JSON.
🤖 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/rerank_cohere/rerank_client.py`:
- Around line 154-168: The rerank wrapper currently only checks truthiness and
can pass invalid runtime inputs to self._client.rerank; update rerank() to
validate inputs more strictly by: ensure query is a non-empty string (e.g.,
isinstance(query, str) and query.strip()), ensure model if provided is a
non-empty string (model is None or isinstance(model, str) and model.strip()),
ensure documents is a non-empty list/iterable and that every item is a non-empty
string after stripping (reject numbers, empty/whitespace-only strings, or
non-string items), keep using _coerce_top_n/self._top_n for top_n, and raise
ValueError with clear messages before calling self._client.rerank if any check
fails.
In `@nodes/src/nodes/rerank_cohere/services.json`:
- Around line 166-179: The conditional "value" strings in the conditional array
do not match the enum/profile titles used by the dropdown, so update the three
conditional entries under conditional (the ones referencing properties like
"rerank_cohere.rerank-v3.5", "rerank_cohere.rerank-v3.0",
"rerank_cohere.custom") to use the profile titles instead of keys — change
"rerank-v3.5" → "Rerank v3.5", "rerank-english-v3.0" → "Rerank v3.0", and
"custom" → "Custom" so the condition values match the enum on line 165 and the
UI will correctly show the related properties.
---
Duplicate comments:
In `@nodes/src/nodes/rerank_cohere/services.json`:
- Around line 2-236: The file contains JavaScript-style line comments (//)
throughout which breaks strict JSON parsing; remove all comment lines or convert
them into valid JSON fields (e.g., add "_comment" properties) or move
explanatory text out to docs so the JSON objects like "title", "protocol",
"preconfig", "fields", and "shape" contain only valid JSON members—ensure every
occurrence of leading // before description blocks and inline comments is
deleted so nodes/src/nodes/rerank_cohere/services.json becomes strictly valid
JSON.
🪄 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: 3d02304b-2c68-4195-8fea-05e421b7388a
📒 Files selected for processing (6)
nodes/src/nodes/rerank_cohere/IGlobal.pynodes/src/nodes/rerank_cohere/IInstance.pynodes/src/nodes/rerank_cohere/rerank_client.pynodes/src/nodes/rerank_cohere/services.jsonnodes/test/rerank_cohere/__init__.pynodes/test/rerank_cohere/test_all.py
- Set capabilities to ["experimental"] consistent with sibling new nodes (PRs rocketride-org#897/rocketride-org#898). - Add a "test" block per docs/README-node-testing.md so the dynamic test runner exercises the node through the real server. The "balanced" profile re-ranks three pre-scored documents and asserts the top result mentions "machine learning". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tests Three IInstance tests (test_write_questions_preserves_metadata, test_write_questions_metadata_alignment_after_filter, test_write_questions_does_not_mutate_original) constructed Doc objects with metadata dicts missing the required chunkId (and in one case objectId) fields. The build interpreter's real pydantic DocMetadata schema requires both, so the tests failed with ValidationError under builder nodes:test even though they passed locally with the cohere mock. Also tighten the dict-equality assertion to attribute-level since DocMetadata is a pydantic model, not a dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…erank Reject non-string and whitespace-only query/documents/model values before calling the Cohere SDK, so callers get a deterministic local ValueError instead of RerankBadRequestError/RerankServerError from the network. This keeps the wrapper's stable validation contract aligned with the existing validateConfig hardening. Addresses CodeRabbit review on PR rocketride-org#898. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
All 7 structural review comments from @stepmikhaylov and 6 of the 7 CodeRabbit nits have been addressed across 846a572 and e729f6c. This covers: test relocation under One CodeRabbit nit was respectfully pushed back on: the @stepmikhaylov @Rod-Christensen @jmaionchi — would appreciate another look when you have a moment. |
2 participants
#Hack-with-bay-2
Contribution Type
Feature — Add Cohere Rerank pipeline node for RAG quality improvement
Resubmission of closed PR #499 (rerank portion only)
PR #499 was closed by @Rod-Christensen on 2026-05-15 for scope reasons (it had grown to include 3 nodes plus unrelated file churn). This PR is the rerank_cohere portion only, on a clean branch from current
develop, with no unrelated changes. Companion PRs are being opened for the chunker and search_hybrid nodes separately.Summary
Adds a
rerank_coherepipeline node for theEmbedding → Vector Store → Rerank → LLMRAG flow.RerankClientwraps CohereClientV2.rerank; validatestop_n >= 1and0.0 <= min_score <= 1.0at construction; maps Cohere SDK exceptions onto a stable hierarchy (RerankAuthenticationError,RerankRateLimitError,RerankBadRequestError,RerankServerError) with names chosen for circuit-breaker substring heuristics.IGlobal.validateConfigonly checks config presence/format — no live API call, so a Cohere outage doesn't break the config UI.IInstance.writeQuestionsdeep-copies the question (fan-out safe), builds a paralleloriginal_indiceslist so rerank results index back to the correct originalDoceven when blank documents are filtered out, and always callswriteAnswers(answer)so downstream nodes get an output even whenmin_scorefilters everything.services.jsonexposestop_nandmin_scorefor all preset profiles (not just custom), usesrerank-english-v3.0(real model id), and constrainstop_n.minimum: 1andmin_scoreto[0.0, 1.0].Review items addressed (from @asclearuc 2026-03-31 review on #499)
original_indiceskeeps rerank results pinned to their source docs.writeAnswers(Answer)instead ofwriteAnswers(question).customprofile hastitle;top_n.minimum: 1;min_scorerange[0.0, 1.0];rerank-v3.0→rerank-english-v3.0; presets exposetop_n/min_score.IGlobal.validateConfigno longer callsclient.rerank(...).RerankClientvalidatestop_nandmin_scorein constructor; unknown Cohere errors wrap asRerankServerError.test_rerank_cohere.pyresets mocks between tests.Validation
nodes/src/nodes/rerank_cohere/, the matching test, the Cohere mock package for dynamic tests, the cohere icon, and the one-line icon-map registration.Companion PRs
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests