Require strict OpenRAG retrieval by default; add verifier, health endpoint, and recommender support#911
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 1 minor |
| Security | 4 high |
🟢 Metrics 0 complexity
Metric Results Complexity 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d67401389d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| payload = recommend_models_for_query( | ||
| query=query, | ||
| top_k=top_k, | ||
| strict_openrag=True, |
There was a problem hiding this comment.
Passing strict_openrag=True unconditionally makes the RAG_STRICT_OPENRAG environment toggle ineffective for the main HTTP recommendation path, so deployments that set RAG_STRICT_OPENRAG=0 for fallback behavior will still return 503 whenever OpenRAG is unavailable. This turns the new strictness setting into dead configuration for API consumers and can break non-strict runtime scenarios.
Useful? React with 👍 / 👎.
| @patch("website.views.recommend_models_for_query") | ||
| def test_recommendation_api_returns_503_when_openrag_unavailable(self, mock_recommend) -> None: | ||
| """Ensure runtime retrieval errors are surfaced as HTTP 503.""" | ||
| mock_recommend.side_effect = OpenRAGRuntimeUnavailableError("OpenRAG retrieval is required") |
There was a problem hiding this comment.
This test references OpenRAGRuntimeUnavailableError without importing it, so executing the test raises NameError before the view behavior is asserted. As written, the new 503 regression coverage is broken and will fail when this test case runs.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a strict OpenRAG retrieval mode that prevents silent fallbacks when the retrieval runtime is unavailable, returning a 503 error instead. It adds a verification script, a staff-only health API, and updates the FARM module to use the flwr package directly. Feedback suggests removing the hardcoded strict mode in the recommendation view to honor environment configurations and consolidating dependency validation logic to reduce maintenance overhead.
| payload = recommend_models_for_query( | ||
| query=query, | ||
| top_k=top_k, | ||
| strict_openrag=True, |
There was a problem hiding this comment.
The strict_openrag=True argument is hardcoded here, which overrides the RAG_STRICT_OPENRAG environment variable. This prevents the "Optional dev fallback" (RAG_STRICT_OPENRAG=0) mentioned in the documentation (OPENRAG_INTEGRATION.md) from working for the web API. It is better to pass None (or omit the argument) to allow the recommender to resolve the strictness policy from the environment, while still defaulting to strict mode as intended.
| strict_openrag=True, | |
| strict_openrag=None, |
| REQUIRED_KEYS = ( | ||
| "openrag_installed", | ||
| "langchain_ollama_installed", | ||
| "langchain_core_installed", | ||
| "dotenv_installed", | ||
| "openrag_endpoint_set", | ||
| ) |
There was a problem hiding this comment.
The REQUIRED_KEYS tuple and the manual check in runtime_status (line 30) duplicate the validation logic already defined in RAG.healthcheck.is_rag_runtime_ready. This duplication increases maintenance overhead if the set of required dependencies or configuration keys changes. Consider reusing is_rag_runtime_ready() to determine the ready flag.
WalkthroughThis PR introduces strict-mode OpenRAG integration with configurable runtime verification and separate Flower dependency migration in FARM. The main cohort adds error handling when OpenRAG retrieval runtime is unavailable, supplies a health endpoint and CLI verification tool, and updates the recommendation API to fail-fast in strict mode. The secondary cohort completes FARM's migration from Syft to Flower 1.29.0. ChangesOpenRAG Strict Mode Integration
FARM Flower Dependency Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AIMER-ROOT/RAG/recommender.py (1)
626-628:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrict mode still emits ungrounded fallback recommendations.
Line 626–628 always returns
_fallback_recommendations(...)whenscoredis empty, even withresolved_strict=True. That reintroduces non-grounded recommendations in strict mode.🐛 Proposed fix
if not scored: - recommendations = _fallback_recommendations(catalog, profile, top_k) + if resolved_strict: + raise OpenRAGRuntimeUnavailableError( + "Strict OpenRAG mode enabled, but no grounded evidence matched the query.", + ) + recommendations = _fallback_recommendations(catalog, profile, top_k)🤖 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 `@AIMER-ROOT/RAG/recommender.py` around lines 626 - 628, The code currently calls _fallback_recommendations(catalog, profile, top_k) whenever scored is empty, which reintroduces ungrounded recommendations even when resolved_strict is True; update the branch around scored to first check resolved_strict and, if resolved_strict is True and scored is empty, set recommendations to an empty list (or otherwise return no recommendations) instead of calling _fallback_recommendations; modify the logic in the function containing the scored check (referencing variables scored, resolved_strict, recommendations, and the _fallback_recommendations call) so fallback is only used when not in strict mode.
♻️ Duplicate comments (1)
AIMER-ROOT/website/views.py (1)
233-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize exception message to prevent information disclosure.
Directly exposing
str(exc)in the API response can leak implementation details, stack traces, or internal paths that aid reconnaissance. Return a generic message instead.🔒 Proposed fix to use a generic error message
except OpenRAGRuntimeUnavailableError as exc: - return JsonResponse({"error": str(exc)}, status=503) + return JsonResponse( + {"error": "RAG retrieval service is currently unavailable"}, + status=503, + )As per coding guidelines, the CodeQL static analysis tool flagged this as "Information exposure through an exception" at line 234.
🤖 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 `@AIMER-ROOT/website/views.py` around lines 233 - 234, The except block that returns JsonResponse({"error": str(exc)}, status=503) exposes internal exception details (OpenRAGRuntimeUnavailableError); change this to return a generic message like JsonResponse({"error":"Service temporarily unavailable"}, status=503) and log the real exception server-side (e.g., logger.exception or logger.error with the exc) inside the same except branch so internal details are not sent to clients; update the handler in website/views.py where OpenRAGRuntimeUnavailableError is caught and ensure JsonResponse is used only with the generic message.
🧹 Nitpick comments (4)
FARM/tests/test_info.py (1)
20-21: ⚡ Quick winConsider patching at the importlib.metadata level for robustness.
The current
monkeypatch.setattr("info.version", fake_version)patches the importedversionreference in theinfomodule after it has been imported. While this works, patching at the source (importlib.metadata.version) is more explicit and robust.♻️ Alternative patch location
def fake_version(_package_name: str) -> str: raise PackageNotFoundError - monkeypatch.setattr("info.version", fake_version) + monkeypatch.setattr("importlib.metadata.version", fake_version) assert _safe_version("flwr") == "NOT_INSTALLED"This patches the function at its source rather than at the import site, making the test more resilient to refactoring.
Note: The Codacy warning about
assertis a false positive—pytest usesassertstatements natively for test assertions, and they are not removed in test contexts.🤖 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 `@FARM/tests/test_info.py` around lines 20 - 21, Replace the patch location to target the origin of the version lookup: instead of monkeypatch.setattr("info.version", fake_version) patch importlib.metadata.version (e.g., monkeypatch.setattr("importlib.metadata", "version", fake_version)) so the test fakes the source used by info.version; update the test around the _safe_version("flwr") assertion to use this new monkeypatch target, ensuring the mocked function name is exactly importlib.metadata.version and restores correctly after the test.AIMER-ROOT/RAG/verify_openrag.py (1)
53-57: ⚡ Quick winUnify report and exit-code readiness source.
On Line 55 and Line 57, readiness is evaluated twice through different paths. This can make the printed
runtime_readydisagree with the process exit code in transient conditions. Use one computed payload for both report rendering and return code.♻️ Proposed refactor
-def format_report() -> str: +def format_report(payload: RuntimeStatusPayload | None = None) -> str: """Build a human-readable OpenRAG readiness report.""" - payload = runtime_status() + payload = runtime_status() if payload is None else payload status = payload["status"] @@ def main() -> int: """Print readiness report and return shell status code.""" - report = format_report() + payload = runtime_status() + report = format_report(payload) print(report) - return 0 if is_rag_runtime_ready() else 1 + return 0 if payload["ready"] else 1🤖 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 `@AIMER-ROOT/RAG/verify_openrag.py` around lines 53 - 57, The main() function currently calls format_report() which computes readiness and separately calls is_rag_runtime_ready() for the exit code, risking a mismatch; modify main() to compute the readiness once (e.g., call is_rag_runtime_ready() and store the result in a local variable like runtime_ready), pass that same value into format_report() or into whatever payload format_report() needs, print the resulting report, and use the stored runtime_ready to determine the return value so both printed report and exit code come from the same computed readiness.AIMER-ROOT/RAG/tests/test_recommender_strict.py (1)
12-18: ⚡ Quick winIsolate strict-mode test from catalog/index side effects.
This test targets strict retrieval fallback, but
recommend_models_for_query(...)still executes catalog/profile prep first. Mock those dependencies too, so failures stay scoped to strict-mode behavior.♻️ Proposed test hardening
from RAG.recommender import OpenRAGRuntimeUnavailableError, recommend_models_for_query @@ monkeypatch.setattr("RAG.recommender._safe_retrieve", fake_retrieve) + monkeypatch.setattr("RAG.recommender._build_model_catalog", lambda **_: {}) try: recommend_models_for_query(🤖 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 `@AIMER-ROOT/RAG/tests/test_recommender_strict.py` around lines 12 - 18, The test currently only stubs RAG.recommender._safe_retrieve but still runs catalog/profile preparation inside recommend_models_for_query, so side-effects from catalog/index setup can mask strict-mode behavior; extend the monkeypatch to stub the catalog and profile prep functions used by recommend_models_for_query (e.g., the catalog loader/index builder/profile preparer functions called within recommend_models_for_query) to return minimal no-op values or mocks before calling recommend_models_for_query, so the test isolates strict_openrag behavior and any failure is scoped to _safe_retrieve fallback.AIMER-ROOT/website/views.py (1)
242-247: 💤 Low valueAdd method docstring.
The
getmethod lacks a docstring describing its purpose, parameters, and return value.📝 Proposed docstring
`@override` def get( self, request: HttpRequest, *args: object, **kwargs: object, ) -> JsonResponse: + """ + Return OpenRAG runtime health status for staff users. + + Returns: + JsonResponse: Health payload with ready flag and status details, + or error response for unauthorized access. + + """ del args, kwargs🤖 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 `@AIMER-ROOT/website/views.py` around lines 242 - 247, Add a concise docstring to the get method explaining its purpose, the parameters (request: HttpRequest, *args, **kwargs) and the return type (JsonResponse); update the docstring inside the get function definition in AIMER-ROOT/website/views.py (the get method) to state what the endpoint does, what inputs it expects, and what the JsonResponse contains so future readers and tools can understand its behavior.
🤖 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 `@AIMER-ROOT/website/tests.py`:
- Around line 594-606: The test
test_recommendation_api_returns_503_when_openrag_unavailable references
OpenRAGRuntimeUnavailableError but doesn't import it; add an import for
OpenRAGRuntimeUnavailableError at the top of the tests file (or alongside
existing related imports) so the symbol is defined when
test_recommendation_api_returns_503_when_openrag_unavailable is executed; ensure
the import comes from the module that defines OpenRAGRuntimeUnavailableError to
avoid import errors.
In `@FARM/tests/test_info.py`:
- Around line 9-11: Remove the sys.path manipulation and replace the top-of-file
import with a proper package import (e.g., import _safe_version from the package
module) so tests rely on normal package discovery; specifically, delete the
sys.path.append(...) line in FARM/tests/test_info.py and change the import "from
info import _safe_version" to a package-aware import such as "from FARM.info
import _safe_version" or a relative import "from ..info import _safe_version"
depending on how tests are executed and the test runner configuration.
---
Outside diff comments:
In `@AIMER-ROOT/RAG/recommender.py`:
- Around line 626-628: The code currently calls
_fallback_recommendations(catalog, profile, top_k) whenever scored is empty,
which reintroduces ungrounded recommendations even when resolved_strict is True;
update the branch around scored to first check resolved_strict and, if
resolved_strict is True and scored is empty, set recommendations to an empty
list (or otherwise return no recommendations) instead of calling
_fallback_recommendations; modify the logic in the function containing the
scored check (referencing variables scored, resolved_strict, recommendations,
and the _fallback_recommendations call) so fallback is only used when not in
strict mode.
---
Duplicate comments:
In `@AIMER-ROOT/website/views.py`:
- Around line 233-234: The except block that returns JsonResponse({"error":
str(exc)}, status=503) exposes internal exception details
(OpenRAGRuntimeUnavailableError); change this to return a generic message like
JsonResponse({"error":"Service temporarily unavailable"}, status=503) and log
the real exception server-side (e.g., logger.exception or logger.error with the
exc) inside the same except branch so internal details are not sent to clients;
update the handler in website/views.py where OpenRAGRuntimeUnavailableError is
caught and ensure JsonResponse is used only with the generic message.
---
Nitpick comments:
In `@AIMER-ROOT/RAG/tests/test_recommender_strict.py`:
- Around line 12-18: The test currently only stubs
RAG.recommender._safe_retrieve but still runs catalog/profile preparation inside
recommend_models_for_query, so side-effects from catalog/index setup can mask
strict-mode behavior; extend the monkeypatch to stub the catalog and profile
prep functions used by recommend_models_for_query (e.g., the catalog
loader/index builder/profile preparer functions called within
recommend_models_for_query) to return minimal no-op values or mocks before
calling recommend_models_for_query, so the test isolates strict_openrag behavior
and any failure is scoped to _safe_retrieve fallback.
In `@AIMER-ROOT/RAG/verify_openrag.py`:
- Around line 53-57: The main() function currently calls format_report() which
computes readiness and separately calls is_rag_runtime_ready() for the exit
code, risking a mismatch; modify main() to compute the readiness once (e.g.,
call is_rag_runtime_ready() and store the result in a local variable like
runtime_ready), pass that same value into format_report() or into whatever
payload format_report() needs, print the resulting report, and use the stored
runtime_ready to determine the return value so both printed report and exit code
come from the same computed readiness.
In `@AIMER-ROOT/website/views.py`:
- Around line 242-247: Add a concise docstring to the get method explaining its
purpose, the parameters (request: HttpRequest, *args, **kwargs) and the return
type (JsonResponse); update the docstring inside the get function definition in
AIMER-ROOT/website/views.py (the get method) to state what the endpoint does,
what inputs it expects, and what the JsonResponse contains so future readers and
tools can understand its behavior.
In `@FARM/tests/test_info.py`:
- Around line 20-21: Replace the patch location to target the origin of the
version lookup: instead of monkeypatch.setattr("info.version", fake_version)
patch importlib.metadata.version (e.g.,
monkeypatch.setattr("importlib.metadata", "version", fake_version)) so the test
fakes the source used by info.version; update the test around the
_safe_version("flwr") assertion to use this new monkeypatch target, ensuring the
mocked function name is exactly importlib.metadata.version and restores
correctly after the test.
🪄 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: CHILL
Plan: Pro
Run ID: 8263e29a-902a-48ab-a96f-850a98f7ec8d
📒 Files selected for processing (14)
AIMER-ROOT/RAG/OPENRAG_INTEGRATION.mdAIMER-ROOT/RAG/recommender.pyAIMER-ROOT/RAG/tests/test_healthcheck.pyAIMER-ROOT/RAG/tests/test_recommender_strict.pyAIMER-ROOT/RAG/verify_openrag.pyAIMER-ROOT/entrypoint.shAIMER-ROOT/pyproject.tomlAIMER-ROOT/website/tests.pyAIMER-ROOT/website/urls.pyAIMER-ROOT/website/views.pyFARM/README.mdFARM/info.pyFARM/pyproject.tomlFARM/tests/test_info.py
| @patch("website.views.recommend_models_for_query") | ||
| def test_recommendation_api_returns_503_when_openrag_unavailable(self, mock_recommend) -> None: | ||
| """Ensure runtime retrieval errors are surfaced as HTTP 503.""" | ||
| mock_recommend.side_effect = OpenRAGRuntimeUnavailableError("OpenRAG retrieval is required") | ||
|
|
||
| response = self.client.get( | ||
| "/api/rag/recommend/", | ||
| {"q": "classification mri"}, | ||
| ) | ||
|
|
||
| self._check_equal(response.status_code, 503) | ||
| self._check("error" in response.json()) | ||
|
|
There was a problem hiding this comment.
Missing import for OpenRAGRuntimeUnavailableError.
The test references OpenRAGRuntimeUnavailableError at line 597 but it is not imported. This will cause a NameError at runtime.
🔧 Proposed fix to add the missing import
from RAG.omop import build_omop_metadata
-from RAG.recommender import recommend_models_for_query
+from RAG.recommender import OpenRAGRuntimeUnavailableError, recommend_models_for_query
from RAG.timm_articles import (🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 597-597: AIMER-ROOT/website/tests.py#L597
undefined name 'OpenRAGRuntimeUnavailableError' (F821)
🤖 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 `@AIMER-ROOT/website/tests.py` around lines 594 - 606, The test
test_recommendation_api_returns_503_when_openrag_unavailable references
OpenRAGRuntimeUnavailableError but doesn't import it; add an import for
OpenRAGRuntimeUnavailableError at the top of the tests file (or alongside
existing related imports) so the symbol is defined when
test_recommendation_api_returns_503_when_openrag_unavailable is executed; ensure
the import comes from the module that defines OpenRAGRuntimeUnavailableError to
avoid import errors.
| sys.path.append(str(Path(__file__).resolve().parents[1])) | ||
|
|
||
| from info import _safe_version |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Refactor to use proper package imports.
Manipulating sys.path in tests is an anti-pattern that makes tests brittle and environment-dependent. Since this is part of the FARM package, use proper relative imports or configure your test environment to make the package discoverable.
♻️ Proposed refactor using proper imports
-from pathlib import Path
import sys
-sys.path.append(str(Path(__file__).resolve().parents[1]))
-
-from info import _safe_version
+from FARM.info import _safe_versionAlternatively, if running tests from the FARM directory, ensure your test runner (pytest) is configured to discover the package properly rather than modifying sys.path at runtime.
🤖 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 `@FARM/tests/test_info.py` around lines 9 - 11, Remove the sys.path
manipulation and replace the top-of-file import with a proper package import
(e.g., import _safe_version from the package module) so tests rely on normal
package discovery; specifically, delete the sys.path.append(...) line in
FARM/tests/test_info.py and change the import "from info import _safe_version"
to a package-aware import such as "from FARM.info import _safe_version" or a
relative import "from ..info import _safe_version" depending on how tests are
executed and the test runner configuration.
| from RAG.verify_openrag import runtime_status | ||
|
|
||
| payload = runtime_status() | ||
| assert set(payload.keys()) == {"ready", "status"} |
|
|
||
| payload = runtime_status() | ||
| assert set(payload.keys()) == {"ready", "status"} | ||
| assert isinstance(payload["ready"], bool) |
| payload = runtime_status() | ||
| assert set(payload.keys()) == {"ready", "status"} | ||
| assert isinstance(payload["ready"], bool) | ||
| assert isinstance(payload["status"], dict) |
| raise PackageNotFoundError | ||
|
|
||
| monkeypatch.setattr("info.version", fake_version) | ||
| assert _safe_version("flwr") == "NOT_INSTALLED" |
2 participants
Motivation
Description
RAG.verify_openragCLI and module to perform machine-readable and human-readable OpenRAG readiness checks and return POSIX exit codes (newverify-openragscript inpyproject.toml).OpenRAGRuntimeUnavailableError,_safe_retrieve_strict,_resolve_strict_openrag, and astrict_openragargument torecommend_models_for_querythat defaults fromRAG_STRICT_OPENRAGenv var.recommend_models_for_query(..., strict_openrag=True), return503on strict retrieval failures, and expose a newGET /api/rag/health/staff-only endpoint that returnsreadyandstatusflags.RAG_VERIFY_ON_START=1to run the OpenRAG readiness check before migrations inentrypoint.sh.RAG/OPENRAG_INTEGRATION.mddescribing environment variables, strictness policy, verification commands, and health endpoint behavior.RAG/tests/test_recommender_strict.py, extendedRAG/tests/test_healthcheck.py, updated website tests to assert strict behavior and 503 handling, and newFARM/tests/test_info.pyfor_safe_versionbehavior.flwr) and updateFARM/info.pyto reportflwrversion safely and updateFARM/pyproject.tomldependency.Testing
pyteston the changed modules, includingRAG.tests.test_healthcheck,RAG.tests.test_recommender_strict,websiterecommendation/health tests, andFARM.tests.test_info, and the test cases completed successfully.verify-openragandpython -m RAG.verify_openragto confirm output format and exit codes during local validation.recommendreturns200on success and503when strict OpenRAG retrieval errors are raised, and therag/healthendpoint enforces authentication and staff access.Codex Task
Summary by CodeRabbit
Release Notes
New Features
/api/rag/health/endpoint to monitor OpenRAG runtime status (staff-only access).verify-openragcommand-line tool for OpenRAG integration verification.Documentation
Chores