chore(python): apply ruff format and enforce in CI#744
Closed
TrishaReddygari wants to merge 3 commits intorocketride-org:developfrom
Closed
chore(python): apply ruff format and enforce in CI#744TrishaReddygari wants to merge 3 commits intorocketride-org:developfrom
TrishaReddygari wants to merge 3 commits intorocketride-org:developfrom
Conversation
Mechanical formatting pass using ruff 0.15.12 against the existing [tool.ruff] config in pyproject.toml. 93 of 720 Python files reformatted; changes are limited to whitespace and quote-style normalization, no semantic edits. This is the one-time baseline pass called for in the issue. Subsequent commits in this PR add CI enforcement so formatting drift can no longer re-enter the codebase. Refs rocketride-org#432
Adds a new lint-python job to the CI workflow that installs ruff at the same pinned version used for the baseline format pass (0.15.12) and runs ruff format --check against the repo. Wired into the ci-ok gatekeeper so formatting drift blocks merges. The job does not depend on init or build, runs on ubuntu-latest with a 5-minute timeout, and skips on scheduled runs (matching the existing build job's exclusion). Pinning ruff is intentional: an unpinned version would let ruff releases silently change "passing" formatting and surprise PR authors. Lint enforcement (ruff check, separate from format) is deferred to a follow-up because there are 480 pre-existing lint violations in the codebase that warrant their own review and fix. Refs rocketride-org#432
New 'Formatting' section documents the ruff version pin, install command, format/check commands, and what CI enforces. The existing single-quotes note in the Python style section now links to it. Refs rocketride-org#432
TrishaReddygari
requested review from
Rod-Christensen,
jmaionchi,
kwit75 and
stepmikhaylov
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR applies the ChangesCode Formatting & Linting Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes The PR applies a uniform formatter (ruff) across a large codebase (~80+ files), making individual changes highly repetitive and low-risk. However, the scope is substantial, requiring a reviewer to sample-check that no semantic logic was inadvertently altered, verify CI/configuration correctness, and confirm the formatter settings are project-appropriate. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/test/mocks/psycopg2/__init__.py (1)
179-189:⚠️ Potential issue | 🟠 MajorBoolean values misclassified as LIMIT candidates in two locations.
In Python,
boolis a subclass ofint, soisinstance(False, int)returnsTrue. At line 185 in_handle_semantic_search, this causes boolean parameters likeFalse(fromisDeleted = %s) to be assigned tolimitinstead of added tofilter_params, breaking WHERE filtering. The same bug exists at line 307 in_extract_limit.Proposed fixes
- elif isinstance(p, int) and p < 1000: + elif isinstance(p, int) and not isinstance(p, bool) and p < 1000: limit = p- if isinstance(p, int) and p < 10000: + if isinstance(p, int) and not isinstance(p, bool) and p < 10000: return p🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/test/mocks/psycopg2/__init__.py` around lines 179 - 189, The bug: boolean values (since bool subclasses int) are being misclassified as LIMIT candidates and assigned to limit in _handle_semantic_search and _extract_limit; update the parameter type checks to exclude bools (e.g., check isinstance(p, bool) first or use type(p) is int / isinstance(p, int) and not isinstance(p, bool)) so booleans are appended to filter_params instead of overwriting limit; apply the same change at both occurrences (functions _handle_semantic_search and _extract_limit) and ensure tests that pass booleans (e.g., isDeleted) now route to filter_params.
🤖 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/test/mocks/chromadb/__init__.py`:
- Around line 169-170: In get() the check "if limit and len(results['ids']) >=
limit" treats limit=0 as falsy (no limit) and returns all rows; update get() to
explicitly handle limit being provided: if limit == 0 return an empty result
immediately, otherwise use "if limit is not None and len(results['ids']) >=
limit: break" (or equivalent) so zero is respected and only None means
unlimited. Reference the get() function and the results['ids'] accumulation when
making the change.
In `@nodes/test/mocks/weaviate/__init__.py`:
- Around line 267-270: Sorting uses a truthy check that collapses 0.0 into the
fallback 1.0; update the sort key used on results (the lambda passed to
results.sort in the QueryResult construction) to explicitly check for None
(e.g., use x.metadata.distance if x.metadata.distance is not None else 1.0) so
that valid 0.0 distances are preserved and only missing distances get the high
default.
---
Outside diff comments:
In `@nodes/test/mocks/psycopg2/__init__.py`:
- Around line 179-189: The bug: boolean values (since bool subclasses int) are
being misclassified as LIMIT candidates and assigned to limit in
_handle_semantic_search and _extract_limit; update the parameter type checks to
exclude bools (e.g., check isinstance(p, bool) first or use type(p) is int /
isinstance(p, int) and not isinstance(p, bool)) so booleans are appended to
filter_params instead of overwriting limit; apply the same change at both
occurrences (functions _handle_semantic_search and _extract_limit) and ensure
tests that pass booleans (e.g., isDeleted) now route to filter_params.
🪄 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: 655ef2e3-8431-420f-a635-95e8abba7cdb
📒 Files selected for processing (95)
.github/workflows/ci.ymlCONTRIBUTING.mdnodes/src/nodes/accessibility_describe/accessibility_vision.pynodes/src/nodes/agent_rocketride/IGlobal.pynodes/src/nodes/anonymize/IInstance.pynodes/src/nodes/anonymize/anonymize.pynodes/src/nodes/anonymize/glinerRecognizer.pynodes/src/nodes/audio_transcribe/IGlobal.pynodes/src/nodes/audio_tts/IGlobal.pynodes/src/nodes/db_mysql/IGlobal.pynodes/src/nodes/db_postgres/IGlobal.pynodes/src/nodes/index_search/IEndpoint.pynodes/src/nodes/index_search/IGlobal.pynodes/src/nodes/index_search/IInstance.pynodes/src/nodes/index_search/__init__.pynodes/src/nodes/index_search/constants.pynodes/src/nodes/index_search/elasticsearch_store.pynodes/src/nodes/index_search/opensearch_client.pynodes/src/nodes/library/msgraph/application_permissions.pynodes/src/nodes/library/permissions/utils.pynodes/src/nodes/llamaparse/IInstance.pynodes/src/nodes/llm_openai_api/openai_client.pynodes/src/nodes/memory_internal/memory.pynodes/src/nodes/ner/IGlobal.pynodes/src/nodes/ner/IInstance.pynodes/src/nodes/ocr/IGlobal.pynodes/src/nodes/ocr/IInstance.pynodes/src/nodes/tool_pipe/IInstance.pynodes/src/nodes/vectordb_postgres/vectordb_postgres.pynodes/test/__init__.pynodes/test/framework/__init__.pynodes/test/framework/expectations.pynodes/test/framework/pipeline.pynodes/test/framework/runner.pynodes/test/milvus/test_search_semantic.pynodes/test/mocks/__init__.pynodes/test/mocks/astrapy/__init__.pynodes/test/mocks/astrapy/data_types.pynodes/test/mocks/astrapy/info.pynodes/test/mocks/chromadb/__init__.pynodes/test/mocks/chromadb/config.pynodes/test/mocks/elasticsearch/__init__.pynodes/test/mocks/langchain_anthropic/__init__.pynodes/test/mocks/langchain_aws/__init__.pynodes/test/mocks/langchain_google_vertexai/__init__.pynodes/test/mocks/langchain_openai/__init__.pynodes/test/mocks/langchain_xai/__init__.pynodes/test/mocks/opensearchpy/__init__.pynodes/test/mocks/pgvector/__init__.pynodes/test/mocks/pgvector/psycopg2.pynodes/test/mocks/pinecone/grpc.pynodes/test/mocks/psycopg2/__init__.pynodes/test/mocks/pymilvus/__init__.pynodes/test/mocks/qdrant_client/__init__.pynodes/test/mocks/qdrant_client/conversions/common_types.pynodes/test/mocks/qdrant_client/models.pynodes/test/mocks/weaviate/__init__.pynodes/test/mocks/weaviate/classes/__init__.pynodes/test/mocks/weaviate/classes/config.pynodes/test/mocks/weaviate/classes/init.pynodes/test/mocks/weaviate/classes/query.pynodes/test/mocks/weaviate/client.pynodes/test/mocks/weaviate/collections/__init__.pynodes/test/mocks/weaviate/collections/collection.pynodes/test/mocks/weaviate/util.pynodes/test/test_contracts.pynodes/test/test_dynamic.pynodes/test/test_embedding_image.pypackages/ai/src/ai/__init__.pypackages/ai/src/ai/common/agent/_internal/utils.pypackages/ai/src/ai/common/dap/transport_tcpip.pypackages/ai/src/ai/common/database/db_global_base.pypackages/ai/src/ai/common/models/audio/kokoro_loader.pypackages/ai/src/ai/common/models/gliner/__init__.pypackages/ai/src/ai/common/models/vision/vision.pypackages/ai/src/ai/common/tools.pypackages/ai/src/ai/node.pypackages/ai/tests/ai/modules/profiler/test_auth.pypackages/ai/tests/ai/modules/task/test_env_var_exfil.pypackages/ai/tests/ai/modules/task/test_temp_file_security.pypackages/ai/tests/ai/web/test_server.pypackages/client-mcp/tests/conftest.pypackages/client-mcp/tests/test_server.pypackages/client-mcp/tests/test_tools.pypackages/client-python/src/rocketride/cli/__init__.pypackages/client-python/src/rocketride/cli/monitors/status.pypackages/client-python/src/rocketride/schema/__init__.pypackages/client-python/src/rocketride/schema/question.pypackages/client-python/src/rocketride/types/data.pypackages/client-python/tests/echo_pipeline.pypackages/client-python/tests/embedding_image_pipeline.pypackages/server/engine-lib/rocketlib-python/lib/depends.pyscripts/check_gpu.pytest/testdata/tests/tasktest.pytestdata/images/generate_test_images.py
💤 Files with no reviewable changes (11)
- nodes/test/mocks/weaviate/classes/query.py
- nodes/test/mocks/chromadb/config.py
- packages/ai/src/ai/common/models/gliner/init.py
- nodes/test/mocks/weaviate/collections/collection.py
- nodes/test/mocks/weaviate/collections/init.py
- nodes/test/mocks/pinecone/grpc.py
- nodes/test/mocks/weaviate/util.py
- nodes/test/init.py
- nodes/test/mocks/weaviate/client.py
- nodes/test/mocks/weaviate/classes/init.py
- nodes/test/framework/init.py
Comment on lines
+169
to
170
| if limit and len(results['ids']) >= limit: | ||
| break |
Contributor
There was a problem hiding this comment.
limit=0 is handled incorrectly in get().
Using if limit and ... makes limit=0 behave like “no limit”, which can return all rows instead of none.
Proposed fix
- if limit and len(results['ids']) >= limit:
+ if limit is not None and len(results['ids']) >= limit:
break📝 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.
Suggested change
| if limit and len(results['ids']) >= limit: | |
| break | |
| if limit is not None and len(results['ids']) >= limit: | |
| break |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/test/mocks/chromadb/__init__.py` around lines 169 - 170, In get() the
check "if limit and len(results['ids']) >= limit" treats limit=0 as falsy (no
limit) and returns all rows; update get() to explicitly handle limit being
provided: if limit == 0 return an empty result immediately, otherwise use "if
limit is not None and len(results['ids']) >= limit: break" (or equivalent) so
zero is respected and only None means unlimited. Reference the get() function
and the results['ids'] accumulation when making the change.
Comment on lines
267
to
270
| # Sort by distance (lower is better for cosine distance) | ||
| results.sort(key=lambda x: x.metadata.distance if x.metadata.distance else 1.0) | ||
|
|
||
| return QueryResult(objects=results[:limit]) |
Contributor
There was a problem hiding this comment.
Sort by distance without collapsing 0.0 to 1.0.
0.0 is a valid cosine distance for an exact match, but this sort key treats it as falsy and ranks it like a miss. That can mis-order nearest-neighbor results in tests.
Proposed fix
- results.sort(key=lambda x: x.metadata.distance if x.metadata.distance else 1.0)
+ results.sort(
+ key=lambda x: x.metadata.distance if x.metadata.distance is not None else 1.0
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/test/mocks/weaviate/__init__.py` around lines 267 - 270, Sorting uses a
truthy check that collapses 0.0 into the fallback 1.0; update the sort key used
on results (the lambda passed to results.sort in the QueryResult construction)
to explicitly check for None (e.g., use x.metadata.distance if
x.metadata.distance is not None else 1.0) so that valid 0.0 distances are
preserved and only missing distances get the high default.
This was referenced May 3, 2026
Author
|
Thanks @coderabbitai — all three findings look real, but they're pre-existing in the test mocks rather than introduced by this format-only PR. To keep this PR's scope mechanical, I've filed them as #747 and opened a follow-up PR #748 that fixes all three with one commit per file. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
ʕ •ᴥ•ʔ ～ makes a mental note ✏️ Learnings added
|
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings the repo into compliance with the formatting acceptance criteria in #432:
ruff formatpass across all Python files (93 of 720 reformatted; whitespace + quote normalization only, no semantic changes).lint-pythonjob in.github/workflows/ci.ymlthat runsruff format --checkon every PR and push, pinned to ruff0.15.12, wired into theci-okgatekeeper so format drift blocks merges.CONTRIBUTING.mdcovering install, local commands, and CI behavior.The
[tool.ruff]config inpyproject.tomlis unchanged — it was already set up correctly; only the format pass and CI enforcement were missing.Scope notes
Full lint enforcement (
ruff check, distinct fromruff format) is deferred to a follow-up PR. There are 480 pre-existing lint violations (347 single-quote, 49 incorrect-blank-line-after-class, 36 undocumented__init__, 26 unused imports, etc.). 400 are auto-fixable, 80 are not. Fixing them in this PR would mix mechanical formatting with code-touching changes that warrant their own review. Happy to follow up with a separate PR if maintainers would like.Also worth a heads-up: ruff reports 3
F821(undefined name) warnings inpackages/ai/src/ai/common/models/transformers/sentence_transformers.pyfornp.ndarrayannotations. They're false positives — the file usesfrom __future__ import annotations, so the names aren't evaluated at runtime. No action needed here, just flagging it.Testing
ruff format --check .→ 720 files clean (was: 93 would reformat).github/workflows/ci.yml→ valid;lint-pythonis inci-ok'sneeds:arrayBreaking changes
None.
Fixes #432
Summary by CodeRabbit
New Features
Documentation
Tests
Style