feat(llama_stack): centralize vector/RAG config and shared helpers#1266
Merged
jgarciao merged 6 commits intoopendatahub-io:mainfrom Mar 20, 2026
Merged
Conversation
- Fix automation for product bug https://redhat.atlassian.net/browse/RHAIENG-3816 - Move Postgres, vLLM, embedding, and AWS-related defaults into constants (env overrides) - Add IBM 2025 earnings PDFs (encrypted/unencrypted) and finance query sets per search mode - Add vector_store_create_and_poll, file-from-URL/path helpers, and upload assertions in utils - Replace vector_store_with_example_docs with a doc_sources parameter on the vector_store fixture - Refactor conftest and vector store + upgrade RAG tests to use the new constants and helpers Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com> Made-with: Cursor Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/wip', '/cherry-pick', '/lgtm', '/build-push-pr-image'} |
Contributor
📝 WalkthroughWalkthroughEnvironment/config constants were moved to a new constants module; the vector store fixture was rewritten to accept parametrized document sources and to ingest local or URL documents with polling and error handling; new file-upload utilities and updated tests were added; dataset README introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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}
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)
tests/llama_stack/utils.py (1)
209-236:⚠️ Potential issue | 🟠 MajorCWE-918: SSRF risk in arbitrary remote URL fetching without validation.
requests.get(url, timeout=60)will fetch anyhttp(s)URL passed as a test parameter. Even thoughdoc_sourcesis currently parametrized with hardcoded URLs only, pytest fixtures can be driven from external configuration, plugins, or environment sources. Validate scheme/host/IP before the request, revalidate any redirect target, and stream with a size cap instead of bufferingresponse.contentto avoid unbounded memory consumption.Also remove invalid
# noqa: FCN001at line 225.Suggested fix
- response = requests.get(url, timeout=60) + _validate_remote_doc_source(url) + response = requests.get(url, timeout=60, stream=True) response.raise_for_status() @@ - temp_file.write(response.content) - temp_file_path = Path(temp_file.name) # noqa: FCN001 + for chunk in response.iter_content(chunk_size=1024 * 1024): + if chunk: + temp_file.write(chunk) + temp_file_path = Path(temp_file.name)def _validate_remote_doc_source(url: str, allowed_hosts: set[str]) -> None: parsed = urllib.parse.urlsplit(url) if parsed.scheme not in {"http", "https"} or not parsed.hostname: raise ValueError(f"Unsupported doc source URL: {url!r}") if parsed.hostname not in allowed_hosts: raise ValueError(f"Untrusted doc source host: {parsed.hostname!r}") for result in socket.getaddrinfo(parsed.hostname, parsed.port or 443, type=socket.SOCK_STREAM): ip = ipaddress.ip_address(result[4][0]) if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved: raise ValueError(f"Blocked doc source host {parsed.hostname!r} resolving to {ip}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llama_stack/utils.py` around lines 209 - 236, The download block using requests.get(url, timeout=60) in this file poses SSRF and memory risks; add a pre-validation function (e.g., _validate_remote_doc_source) and call it before requests.get to enforce allowed schemes (http/https), a whitelist of hostnames, and reject hostnames resolving to private/loopback/reserved IPs, then perform requests.get with allow_redirects=True but re-validate each redirect target against the same rules and use stream=True to iterate response.iter_content with a hard size cap to avoid buffering response.content; also remove the incorrect "# noqa: FCN001" on temp_file_path and ensure temp_file_path is safely initialized/checked before os.unlink in the finally block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llama_stack/conftest.py`:
- Around line 810-850: Wrap the entire doc_sources ingestion block (the code
that iterates doc_sources and calls vector_store_create_file_from_url and
vector_store_create_file_from_path) in a try/except that catches Exception; in
the except handler call the vector store deletion API to remove the
partially-created store (e.g., vector_store.delete() or use
unprivileged_llama_stack_client to delete the store by id) and then re-raise the
exception so the fixture teardown/yield behavior is preserved; ensure you
reference the same variables (doc_sources, vector_store,
unprivileged_llama_stack_client, vector_store_create_file_from_url,
vector_store_create_file_from_path) when implementing the try/except and
deletion.
- Around line 819-849: The code handling doc_sources allows absolute paths and
"../" traversal; fix by resolving each non-URL source against
request.config.rootdir using Path(...).resolve() and compare it to the repo root
Path(request.config.rootdir).resolve(); if the resolved path is not within the
repo root (use path.relative_to(root) or a startswith check), raise
FileNotFoundError/ValueError and do not call vector_store_create_file_from_path;
keep existing behavior for URLs and the calls to
vector_store_create_file_from_path and vector_store_create_file_from_url
otherwise.
---
Outside diff comments:
In `@tests/llama_stack/utils.py`:
- Around line 209-236: The download block using requests.get(url, timeout=60) in
this file poses SSRF and memory risks; add a pre-validation function (e.g.,
_validate_remote_doc_source) and call it before requests.get to enforce allowed
schemes (http/https), a whitelist of hostnames, and reject hostnames resolving
to private/loopback/reserved IPs, then perform requests.get with
allow_redirects=True but re-validate each redirect target against the same rules
and use stream=True to iterate response.iter_content with a hard size cap to
avoid buffering response.content; also remove the incorrect "# noqa: FCN001" on
temp_file_path and ensure temp_file_path is safely initialized/checked before
os.unlink in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 626426c8-00d9-413b-b1b8-1543e6445ca6
⛔ Files ignored due to path filters (5)
tests/llama_stack/dataset/corpus/finance/ibm-1q25-earnings-press-release-unencrypted.pdfis excluded by!**/*.pdftests/llama_stack/dataset/corpus/finance/ibm-2q25-earnings-press-release-unencrypted.pdfis excluded by!**/*.pdftests/llama_stack/dataset/corpus/finance/ibm-3q25-earnings-press-release-unencrypted.pdfis excluded by!**/*.pdftests/llama_stack/dataset/corpus/finance/ibm-4q25-earnings-press-release-unencrypted.pdfis excluded by!**/*.pdftests/llama_stack/dataset/corpus/pdf-testing/ibm-4q25-press-release-encrypted.pdfis excluded by!**/*.pdf
📒 Files selected for processing (5)
tests/llama_stack/conftest.pytests/llama_stack/constants.pytests/llama_stack/utils.pytests/llama_stack/vector_io/test_vector_stores.pytests/llama_stack/vector_io/upgrade/test_upgrade_vector_store_rag.py
Bobbins228
reviewed
Mar 20, 2026
- Improved error handling for doc_sources input, ensuring it is a list and paths are validated against the repository root. - Added logging for successful and failed ingestion of document sources. - Streamlined the process for uploading files from URLs and local paths, including directory handling. Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com>
Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com>
Bobbins228
previously approved these changes
Mar 20, 2026
Ygnas
approved these changes
Mar 20, 2026
Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com>
Contributor
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 `@tests/llama_stack/conftest.py`:
- Around line 785-788: The fixture currently uses
params_raw/params/vector_io_provider/doc_sources but hard-codes the persisted
store name ("test_vector_store"), causing collisions; add a helper function
named _vector_store_name that takes vector_io_provider and doc_sources,
normalizes/sorts doc_sources, serializes the payload (vector_io_provider +
normalized sources), hashes it (e.g., sha256) and returns a deterministic store
name (like "test-vector-store-<hash-prefix>"), then replace the hard-coded
"test_vector_store" usages in the fixture (and the other occurrences referenced
around the same block) with a call to
_vector_store_name(vector_io_provider=vector_io_provider,
doc_sources=doc_sources) so each param combination gets a unique persisted store
name.
- Around line 839-849: Re-resolve each directory entry before uploading to
prevent symlink/traversal attacks: inside the loop over files, call resolve() on
the child (file_path_resolved = file_path.resolve(strict=True)) and compare it
against the already-resolved repo_root (repo_root.resolve()) to ensure
file_path_resolved is a descendant of repo_root; skip or raise if not. Use
file_path_resolved.is_file() and pass that resolved path into
vector_store_create_file_from_path (referencing file_path, source_path,
repo_root, and vector_store_create_file_from_path) so you never follow a symlink
that points outside the repo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6115d4e1-8a3b-425d-81fe-c70de57cb749
📒 Files selected for processing (3)
tests/llama_stack/conftest.pytests/llama_stack/constants.pytests/llama_stack/dataset/README.md
✅ Files skipped from review due to trivial changes (1)
- tests/llama_stack/dataset/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llama_stack/constants.py
Signed-off-by: Jorge Garcia Oncins <jgarciao@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/llama_stack/utils.py (2)
225-225: Invalid# noqa: FCN001rule codes.Ruff doesn't recognize
FCN001. Either remove these comments or addFCN001tolint.externalin pyproject.toml/ruff.toml if it's from another linter in your toolchain.Also applies to: 322-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llama_stack/utils.py` at line 225, The inline comment "# noqa: FCN001" on the temp_file_path assignment (temp_file_path = Path(temp_file.name) in utils.py) uses an unknown Ruff rule code; remove the invalid noqa marker or replace it with a recognized code, or alternatively add "FCN001" to lint.external in pyproject.toml/ruff.toml if that code is required by another linter; apply the same fix to the other occurrence at the similar assignment around line 322 so no invalid noqa codes remain.
231-233: Redundant exception tuple.
Exceptionis a superclass ofrequests.exceptions.RequestException, so the tuple is redundant.Proposed fix
- except (requests.exceptions.RequestException, Exception) as e: + except Exception as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llama_stack/utils.py` around lines 231 - 233, The except clause currently uses a redundant tuple "(requests.exceptions.RequestException, Exception)"; change it to catch only requests.exceptions.RequestException (i.e., "except requests.exceptions.RequestException as e:") so you don't redundantly include Exception or unintentionally swallow unrelated exceptions; update the except block referencing LOGGER and url and keep the existing LOGGER.warning and raise behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llama_stack/utils.py`:
- Line 268: Fix the typos in the log statements: in the LOGGER.info call that
references uploaded_file.filename and vector_store.id (the f-string currently
has "filename{uploaded_file.filename") change it to use
"filename={uploaded_file.filename}" so the variable interpolates correctly; also
correct the misspelling "Addded" to "Added" in the other LOGGER.info message
that mentions the upload/vector store operation. Ensure both messages remain
f-strings and still reference uploaded_file.filename and vector_store.id (or the
equivalent variables) exactly.
---
Nitpick comments:
In `@tests/llama_stack/utils.py`:
- Line 225: The inline comment "# noqa: FCN001" on the temp_file_path assignment
(temp_file_path = Path(temp_file.name) in utils.py) uses an unknown Ruff rule
code; remove the invalid noqa marker or replace it with a recognized code, or
alternatively add "FCN001" to lint.external in pyproject.toml/ruff.toml if that
code is required by another linter; apply the same fix to the other occurrence
at the similar assignment around line 322 so no invalid noqa codes remain.
- Around line 231-233: The except clause currently uses a redundant tuple
"(requests.exceptions.RequestException, Exception)"; change it to catch only
requests.exceptions.RequestException (i.e., "except
requests.exceptions.RequestException as e:") so you don't redundantly include
Exception or unintentionally swallow unrelated exceptions; update the except
block referencing LOGGER and url and keep the existing LOGGER.warning and raise
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b2ff3dae-cffe-44c1-99ec-f16d3ac917bf
📒 Files selected for processing (2)
tests/llama_stack/conftest.pytests/llama_stack/utils.py
dbasunag
approved these changes
Mar 20, 2026
|
Status of building tag latest: success. |
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
5 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.
Note, in a follow-up PR we'll add a jsonl file in the dataset to store questions and expected answers
Summary by CodeRabbit