Production vlaues - #151
Conversation
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
genai/app/routers/rag.py (1)
283-294: 🩺 Stability & Availability | 🔵 TrivialConsider surfacing the quality-review-failed fallback for observability.
When all corrective attempts are exhausted, the endpoint silently returns the last (still-rejected) candidate with HTTP 200. This is the intended behavior per the PR objective, but there's currently no metric/counter or response signal indicating that quality review failed and a best-effort plan was returned — only a log warning. Consider emitting a metric (e.g. counter tagged by topic/exercise type) here so recurring quality failures are visible in dashboards/alerts rather than only discoverable via log search.
🤖 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 `@genai/app/routers/rag.py` around lines 283 - 294, In the exhausted-attempt fallback immediately before returning from the RAG endpoint, add an observability metric or counter indicating that quality review failed and the last rejected candidate was returned. Tag it with the available topic and exercise-type dimensions, while preserving the existing warning log and HTTP 200 fallback behavior.genai/tests/test_rag.py (1)
568-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a boundary test for
rag_learning_plan_max_repair_attempts = 0.Config allows
ge=0, and the router'sfor attempt in range(1, max_attempts + 1)loop should simply skip repair and return the initial rejected candidate at 200 when this is0. There's currently no test asserting this boundary (only 1 and 2 are covered).Suggested test
def test_job_interview_plan_returns_initial_plan_when_repair_attempts_disabled( client, tmp_path ): rejected_plan = _speaking_plan( "Explain what you should wear to an interview.", "Describe suitable professional attire.", ) with ( patch("app.routers.rag._rag_doc_db", return_value=tmp_path), patch("app.routers.rag.query_topic", return_value=[]), patch("app.routers.rag.settings.rag_learning_plan_max_repair_attempts", 0), patch( "app.routers.rag.get_structured_llm", side_effect=_structured_llm_sequence(rejected_plan, _accepted_review()), ) as structured_llm, ): response = client.post( "/api/v1/genai/rag/learning-plan", json=_learning_plan_request(), ) assert response.status_code == 200 assert structured_llm.call_count == 2🤖 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 `@genai/tests/test_rag.py` around lines 568 - 639, Add a boundary test alongside the existing corrective-attempt tests that patches rag_learning_plan_max_repair_attempts to 0, supplies only the initial rejected plan and review through _structured_llm_sequence, and posts the standard learning-plan request. Assert a 200 response, exactly two LLM calls, and that the response returns the initial rejected plan unchanged.
🤖 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 @.secrets/README.md:
- Around line 27-32: Update the verification command in the README to render
Compose configuration as JSON and assert that LLM_API_KEY is non-empty, while
redacting or avoiding the secret value itself. Do not use config --quiet, since
it only validates configuration without proving the variable was loaded.
---
Nitpick comments:
In `@genai/app/routers/rag.py`:
- Around line 283-294: In the exhausted-attempt fallback immediately before
returning from the RAG endpoint, add an observability metric or counter
indicating that quality review failed and the last rejected candidate was
returned. Tag it with the available topic and exercise-type dimensions, while
preserving the existing warning log and HTTP 200 fallback behavior.
In `@genai/tests/test_rag.py`:
- Around line 568-639: Add a boundary test alongside the existing
corrective-attempt tests that patches rag_learning_plan_max_repair_attempts to
0, supplies only the initial rejected plan and review through
_structured_llm_sequence, and posts the standard learning-plan request. Assert a
200 response, exactly two LLM calls, and that the response returns the initial
rejected plan unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b419ed73-aefb-4328-ad6d-7c61b6555e1f
📒 Files selected for processing (11)
.env.example.gitignore.secrets/README.md.secrets/api-key.env.exampledocker-compose.ymlgenai/app/config.pygenai/app/routers/rag.pygenai/tests/test_rag.pyhelm/team-drops/templates/configmap.yamlhelm/team-drops/values-rancher.yamlhelm/team-drops/values.yaml
| The second file takes precedence, so the key remains separate from the normal | ||
| local settings. To confirm that Compose received the variable without printing | ||
| the secret, run: | ||
|
|
||
| ```powershell | ||
| docker compose --env-file .env --env-file .secrets/api-key.env config --quiet |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'File map:\n'
git ls-files .secrets/README.md
printf '\nRelevant lines:\n'
cat -n .secrets/README.md | sed -n '20,40p'Repository: AET-DevOps26/team-drops
Length of output: 747
🌐 Web query:
docker compose config --quiet env-file loaded variable rendered JSON non-empty documentation
💡 Result:
The docker compose config command allows you to parse, resolve, and render your Compose file in its final canonical format [1][2]. While there is no single flag that combines all the requirements you mentioned, you can achieve your goal of rendering a resolved configuration using specific options [3][2]. Key concepts and commands: 1. Rendered Output Format: To output the resolved Compose file in JSON format instead of the default YAML, use the --format flag: docker compose config --format json [3][2] 2. Environment Files: You can specify an alternate environment file using the --env-file global option for docker compose [1]. For example: docker compose --env-file.env.production config --format json 3. Controlling Resolution: - To see how environment variables were interpolated during the configuration process, use the --environment flag [3][4]. - By default, Compose resolves env_file values [5][6]. If you wish to disable this behavior, you can use the --no-env-resolution flag [3][7]. 4. Validation: The --quiet (or -q) flag is used for validation only [3][2]. When used, it suppresses all output to stdout, which contradicts the goal of rendering a configuration [2]. In summary, to get a rendered, non-empty JSON output that includes resolved variables from a specific environment file, you should use the following command structure: docker compose --env-file <PATH_TO_ENV_FILE> config --format json [3][1] This command will output the fully resolved, canonical Compose model in JSON format, incorporating the variables from your specified environment file [3][1][6]. If the resulting output is empty, it may indicate a failure in the configuration parsing or that the provided Compose file does not resolve to any active services or configurations [2].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/compose/
- 2: https://github.com/docker/compose/blob/main/cmd/compose/config.go
- 3: https://docs.docker.com/reference/cli/docker/compose/config/
- 4: https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/
- 5: https://docs.docker.com/reference/compose-file/services/
- 6: https://github.com/docker/compose/blob/master/compose/config/config.py
- 7: https://github.com/docker/compose/blob/main/docs/reference/compose_config.md
Use a command that proves LLM_API_KEY is set. docker compose config --quiet only validates the file and prints nothing, so it doesn’t confirm the secret was loaded. Replace it with a redacted check that renders JSON and asserts the key is non-empty without echoing the value.
🤖 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 @.secrets/README.md around lines 27 - 32, Update the verification command in
the README to render Compose configuration as JSON and assert that LLM_API_KEY
is non-empty, while redacting or avoiding the secret value itself. Do not use
config --quiet, since it only validates configuration without proving the
variable was loaded.
1 participant
Summary
This PR reduces the maximum number of corrective attempts from 3 to 2 across production, Docker Compose, and application defaults.
LLM Call Reduction
The worst-case flow is now:
This results in a maximum of 6 LLM calls instead of 8.
Fallback Behavior
If all corrective attempts fail the quality review, the GenAI service now:
200Verification
Summary by CodeRabbit
New Features
Documentation
Configuration