Evaluation: Gemini batch integration (text)#867
Merged
Conversation
- Introduced `run_evaluation_batch_submission` task to handle evaluation batch submissions. - Created `start_evaluation_batch_submission` utility to enqueue evaluation tasks. - Updated `start_evaluation` to utilize the new Celery task for batch submissions. - Enhanced error handling in evaluation run creation and processing. - Added support for Google Gemini in evaluation batch processing. - Refactored model configuration to support multiple providers and completion types. - Improved logging for better traceability during evaluation processes. - Updated tests to reflect changes in evaluation batch submission logic.
📝 WalkthroughWalkthroughAdds provider-aware evaluation batch submission through Celery, with Gemini/OpenAI JSONL generation and result parsing. It also updates organization, project, and user-project lifecycle behavior to support active/inactive states, soft and hard deletes, and related access checks. ChangesMulti-provider Evaluation Batch (Gemini + Async Celery)
Access, delete, and lifecycle rules
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
vprashrex
changed the title
OpenAPI changes 🟢 1 non-breaking changeTip Safe to merge from an API-contract perspective. Full changelog ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new optional request property tts/anyOf[subschema #1: TTSBlockSpec]/params/anyOf[subschema #1: TTSLLMParams]/instructions |
main ↔ 8d9815e7 · generated by oasdiff
…xtLLMParams class
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… logic in evaluation batch processing
vprashrex
requested review from
AkhileshNegi and
Prajna1999
and removed request for
AkhileshNegi
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/crud/evaluations/langfuse.py (1)
158-170: 💤 Low valueConsider using WARNING level for rate-limit logs.
Rate-limit responses (429) are expected transient conditions, not errors in the application. Using
logger.errormay cause alert fatigue.logger.warningwould be more appropriate since the code handles this gracefully by continuing to the next item.💡 Suggested change
if getattr(e, "status_code", None) == 429: - logger.error( + logger.warning( f"[create_langfuse_dataset_run] Langfuse rate limit (429) | " f"item_id={item_id}" )🤖 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 `@backend/app/crud/evaluations/langfuse.py` around lines 158 - 170, In the exception handler of the create_langfuse_dataset_run function, the rate-limit (429) response is being logged at the ERROR level, but this is an expected transient condition that should not trigger alerts. Change the logger.error call in the if block that checks for status_code == 429 to logger.warning instead, since the code handles this gracefully by continuing to the next item.backend/app/tests/api/routes/test_evaluation.py (1)
611-818: 💤 Low valueConsider adding type hints to mock parameters for full guideline compliance.
The coding guideline requires type hints on all function parameters. While the existing test suite doesn't type-hint mock parameters, adding them would align with the guideline. For example:
from unittest.mock import MagicMock def test_example(self, mock_fetch: MagicMock, mock_map: MagicMock) -> None:As per coding guidelines:
**/*.py: Always add type hints to all function parameters and return values in Python🤖 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 `@backend/app/tests/api/routes/test_evaluation.py` around lines 611 - 818, All test methods in the TestBatchEvaluationJSONLBuilding class should comply with the coding guideline requiring type hints on all function parameters. Add type hints to the self parameter for all test methods (test_build_batch_jsonl_basic, test_build_batch_jsonl_with_tools, test_build_batch_jsonl_minimal_config, test_build_batch_jsonl_skips_empty_questions, test_build_batch_jsonl_multiple_items, test_build_batch_jsonl_temperature_included_when_explicitly_set, test_build_batch_jsonl_temperature_excluded_when_not_set, and test_build_batch_jsonl_temperature_zero_included_when_explicitly_set). If any of these test methods use mock parameters (injected via pytest fixtures or as function arguments), add appropriate type hints such as MagicMock from unittest.mock to those parameters as well.Source: Coding guidelines
backend/app/services/evaluations/batch_job.py (1)
21-30: ⚡ Quick winAdd type hints to function parameters and return value.
Per coding guidelines, all function parameters and return values need type hints. The
task_instanceparameter and**kwargslack type annotations. Consider usingAnyfortask_instance(matching the Celery task pattern) and aTypedDictor more specific return type.+from typing import Any + def execute_evaluation_batch_submission( project_id: int, job_id: str, task_id: str, - task_instance, + task_instance: Any, organization_id: int, config_id: str, config_version: int, **kwargs, -) -> dict: +) -> dict[str, Any]:🤖 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 `@backend/app/services/evaluations/batch_job.py` around lines 21 - 30, Add missing type hints to the execute_evaluation_batch_submission function. Annotate the task_instance parameter with Any type to match Celery task patterns, add type hints to the **kwargs parameter (also using Any), and consider defining the return type more specifically than just dict by using a TypedDict or a more precise type annotation that reflects what the function actually returns.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@backend/app/crud/evaluations/langfuse.py`:
- Around line 158-170: In the exception handler of the
create_langfuse_dataset_run function, the rate-limit (429) response is being
logged at the ERROR level, but this is an expected transient condition that
should not trigger alerts. Change the logger.error call in the if block that
checks for status_code == 429 to logger.warning instead, since the code handles
this gracefully by continuing to the next item.
In `@backend/app/services/evaluations/batch_job.py`:
- Around line 21-30: Add missing type hints to the
execute_evaluation_batch_submission function. Annotate the task_instance
parameter with Any type to match Celery task patterns, add type hints to the
**kwargs parameter (also using Any), and consider defining the return type more
specifically than just dict by using a TypedDict or a more precise type
annotation that reflects what the function actually returns.
In `@backend/app/tests/api/routes/test_evaluation.py`:
- Around line 611-818: All test methods in the TestBatchEvaluationJSONLBuilding
class should comply with the coding guideline requiring type hints on all
function parameters. Add type hints to the self parameter for all test methods
(test_build_batch_jsonl_basic, test_build_batch_jsonl_with_tools,
test_build_batch_jsonl_minimal_config,
test_build_batch_jsonl_skips_empty_questions,
test_build_batch_jsonl_multiple_items,
test_build_batch_jsonl_temperature_included_when_explicitly_set,
test_build_batch_jsonl_temperature_excluded_when_not_set, and
test_build_batch_jsonl_temperature_zero_included_when_explicitly_set). If any of
these test methods use mock parameters (injected via pytest fixtures or as
function arguments), add appropriate type hints such as MagicMock from
unittest.mock to those parameters as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d78ee05c-da68-4a57-8a2c-a6e4fe2ac00b
📒 Files selected for processing (13)
backend/app/celery/tasks/job_execution.pybackend/app/celery/utils.pybackend/app/core/batch/gemini.pybackend/app/core/batch/operations.pybackend/app/crud/evaluations/batch.pybackend/app/crud/evaluations/langfuse.pybackend/app/crud/evaluations/processing.pybackend/app/services/evaluations/batch_job.pybackend/app/services/evaluations/evaluation.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/services/evaluations/test_evaluation_service_s3.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/celery/tasks/job_execution.py (2)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace raw queue and priority literals with named constants.
The repeated
"default"/9/6/2/1values make this routing policy easy to drift from the module docstring and harder to update consistently.Suggested direction
+DEFAULT_TASK_QUEUE = "default" +LLM_TASK_PRIORITY = 9 +FAST_EVAL_TASK_PRIORITY = 6 +STANDARD_TASK_PRIORITY = 2 +NOTIFICATION_TASK_PRIORITY = 1 + -@celery_app.task(bind=True, queue="default", priority=9) +@celery_app.task( + bind=True, + queue=DEFAULT_TASK_QUEUE, + priority=LLM_TASK_PRIORITY, +)As per coding guidelines,
**/*.py: Do not use magic values in code; extract repeated literals into constants,Enummembers, or settings.Also applies to: 97-97, 115-115, 133-135, 151-151, 169-169, 189-189, 209-209, 229-229, 249-249, 269-269, 289-289, 309-309, 332-332, 354-355, 384-386
🤖 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 `@backend/app/celery/tasks/job_execution.py` at line 79, The task routing decorators in job_execution.py use repeated raw queue and priority literals, which should be centralized so the policy stays consistent with the module docstring. Define named constants (or Enum/settings values) for the queue name and each priority level, then update the celery_app.task declarations on the affected task functions to use those symbols instead of hardcoded values. Keep the existing routing behavior unchanged while making the constants the single source of truth for all task decorators in this module.Source: Coding guidelines
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the bound task signatures fully.
run_guardrails_jobstill leavesself,**kwargs, and the return type untyped, andsend_eval_completion_notificationleavesselfuntyped with a very broaddictreturn. These wrappers already have concrete contracts, so please annotate them the same wayrun_evaluation_fastdoes.Suggested direction
+from typing import Any + -def run_guardrails_job(self, project_id: int, job_id: str, trace_id: str, **kwargs): +def run_guardrails_job( + self: Task, + project_id: int, + job_id: str, + trace_id: str, + **kwargs: object, +) -> dict[str, Any]: ... -def send_eval_completion_notification(self, evaluation_id: int) -> dict: +def send_eval_completion_notification( + self: Task, evaluation_id: int +) -> dict[str, Any]: ...As per coding guidelines,
**/*.py: Use type hints on every function parameter and return value;-> Anyis not acceptable unless the type can be narrowed.Also applies to: 388-388
🤖 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 `@backend/app/celery/tasks/job_execution.py` at line 135, The bound Celery wrapper signatures are still partially untyped: `run_guardrails_job` needs a typed `self`, a typed `**kwargs`, and an explicit return annotation, and `send_eval_completion_notification` should also type `self` and narrow its `dict` return to the concrete shape it returns. Update these in `job_execution.py` to match the typing style used by `run_evaluation_fast`, using the existing task/function names to locate the wrappers and fully annotating every parameter and return value.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@backend/app/celery/tasks/job_execution.py`:
- Line 79: The task routing decorators in job_execution.py use repeated raw
queue and priority literals, which should be centralized so the policy stays
consistent with the module docstring. Define named constants (or Enum/settings
values) for the queue name and each priority level, then update the
celery_app.task declarations on the affected task functions to use those symbols
instead of hardcoded values. Keep the existing routing behavior unchanged while
making the constants the single source of truth for all task decorators in this
module.
- Line 135: The bound Celery wrapper signatures are still partially untyped:
`run_guardrails_job` needs a typed `self`, a typed `**kwargs`, and an explicit
return annotation, and `send_eval_completion_notification` should also type
`self` and narrow its `dict` return to the concrete shape it returns. Update
these in `job_execution.py` to match the typing style used by
`run_evaluation_fast`, using the existing task/function names to locate the
wrappers and fully annotating every parameter and return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a46f97b-e4da-449f-9da1-dc2d10234d68
📒 Files selected for processing (7)
backend/app/celery/tasks/job_execution.pybackend/app/celery/utils.pybackend/app/crud/evaluations/langfuse.pybackend/app/crud/evaluations/processing.pybackend/app/services/evaluations/evaluation.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/crud/evaluations/test_processing.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/celery/utils.py
- backend/app/crud/evaluations/langfuse.py
- backend/app/services/evaluations/evaluation.py
- backend/app/tests/crud/evaluations/test_processing.py
- backend/app/crud/evaluations/processing.py
…alidation for unsupported instructions; Update evaluation process to enforce text completion type
Co-authored-by: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com>
Prajna1999
approved these changes
Jul 1, 2026
| voice: str = DEFAULT_TTS_VOICE | ||
| language: str | None = None | ||
| response_format: Literal["mp3", "wav", "ogg"] | None = "wav" | ||
| instructions: str | None = Field(default=None, exclude=True) |
Collaborator
There was a problem hiding this comment.
not sure why instructions are inside TTSLLMParams. May be a typo?
Collaborator
Author
There was a problem hiding this comment.
one bug I found .. when I am trying to create config for text to speech from kaapi console ui .. the frontend used to send the instructions field .. and the backend tend to reject it .. which becomes very difficult to create tts configuration from ui .. so added this line to handle it.. empty instruction from frontend will be handled properly
| f"item_id={item_id} | {e}", | ||
| exc_info=True, | ||
| ) | ||
| if getattr(e, "status_code", None) == 429: |
Collaborator
There was a problem hiding this comment.
nitpick: If we can follow the same error handling standards as used inside providers
Collaborator
Author
There was a problem hiding this comment.
thanks will check
| output = json.loads(output) | ||
| except (json.JSONDecodeError, ValueError): | ||
| try: | ||
| output = ast.literal_eval(output) |
Collaborator
There was a problem hiding this comment.
not sure if these errors need to be handled specifically
| # Providers whose configs can run through the async batch evaluation path. | ||
| # Mirrors app.services.assessment.service._SUPPORTED_BATCH_PROVIDERS — the | ||
| # native variants forward raw params but still submit via the same batch API. | ||
| _SUPPORTED_BATCH_PROVIDERS = { |
Collaborator
There was a problem hiding this comment.
THis is already inside registry.py and constants.py
Collaborator
Author
There was a problem hiding this comment.
it is specifically for those providers which support batch
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/routes/auth.py (1)
99-125: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove the no-project guard before user activation.
Both flows commit
user.is_active = Truebefore the new 403. An inactive user with no active projects is rejected but still reactivated, undoing the lifecycle deactivation. Computeavailable_projectsand raise first; only activate after the guard passes.Suggested ordering
+ available_projects = get_user_accessible_projects(session=session, user_id=user.id) + + if not user.is_superuser and not available_projects: + logger.info( + f"[google_auth] User has no accessible projects | user_id: {user.id}" + ) + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You are not assigned to any active project. Please contact your administrator.", + ) + # Activate user on first Google login if not user.is_active: user.is_active = True session.add(user) session.commit() session.refresh(user) logger.info(f"[google_auth] User activated on first login | user_id: {user.id}")Apply the same ordering in
verify_magic_link.Also applies to: 388-409
🤖 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 `@backend/app/api/routes/auth.py` around lines 99 - 125, The no-project guard is happening after user activation, so an inactive user with zero accessible projects gets reactivated before the 403 is raised. In the Google login flow around get_user_accessible_projects and the user.is_active commit, move the available_projects check and HTTPException before any activation/commit logic, then only set user.is_active=True after the guard passes. Apply the same ordering in verify_magic_link so both auth paths behave consistently.
🧹 Nitpick comments (3)
backend/app/api/routes/login.py (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated no-active-project error detail.
This detail string is repeated across login, Google auth, and magic-link auth, so future copy changes can drift. Put it behind a shared constant and reuse it. As per coding guidelines, do not use magic values in code; extract repeated literals into constants, Enum members, or settings.
🤖 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 `@backend/app/api/routes/login.py` around lines 45 - 51, The no-active-project HTTPException detail is duplicated across the auth flows, so extract that literal into a shared constant and reuse it from the login path. Update the login logic around the user/project check to reference the shared symbol instead of embedding the string, and apply the same constant in the Google auth and magic-link auth handlers to keep the message consistent.Source: Coding guidelines
backend/app/crud/project.py (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the plural fetch helper to
list_projects_by_organization.
get_projects_by_organizationreturns a list, so the helper should use thelist_*prefix and callers should be updated with the new name. As per coding guidelines, “uselist_*for plural fetch functions,get_*for single-item lookups.”🤖 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 `@backend/app/crud/project.py` around lines 58 - 64, Rename the plural fetch helper currently named get_projects_by_organization to list_projects_by_organization in the project CRUD module. Update any callers to use the new list_* name, since this function returns a list of Project objects; keep the same parameters and behavior, and use the new symbol name consistently wherever it is referenced.Source: Coding guidelines
backend/app/crud/user_project.py (1)
146-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the plural ID fetch helper to
list_user_ids_for_project.This returns multiple user IDs, so the helper should use the
list_*prefix and its route callers should be updated. As per coding guidelines, “uselist_*for plural fetch functions,get_*for single-item lookups.”🤖 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 `@backend/app/crud/user_project.py` around lines 146 - 152, The helper currently named get_user_ids_for_project returns multiple IDs, so rename it to list_user_ids_for_project to match the list_* naming convention for plural fetches. Update every caller in the route layer and elsewhere to use the new function name, keeping the behavior in the UserProject/session query unchanged.Source: Coding guidelines
🤖 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 `@backend/app/api/routes/organization.py`:
- Line 57: Add a stable ordering to the organizations query before applying
pagination. In the organizations route where the `statement =
select(Organization).where(*filters).offset(skip).limit(limit)` query is built,
add an `order_by` on a deterministic key such as `Organization.id` so
`skip`/`limit` returns consistent pages across requests.
In `@backend/app/api/routes/project.py`:
- Around line 43-55: The changed route handlers are missing explicit return type
annotations even though they return APIResponse[...] values. Update
read_projects and the other affected route handler in the same module to include
precise return annotations matching the APIResponse type they produce, keeping
all parameters fully typed. Use the existing handler names to locate the
functions and ensure the signatures satisfy the project’s type-hinting
guideline.
In `@backend/app/crud/user_project.py`:
- Around line 117-118: The logging in deactivate_users_without_projects and the
related reactivation helper is exposing raw user ID lists, which should be
avoided. Update the logger.info calls in user_project.py to report counts or
summary totals instead of the full deactivated/reactivated identifier arrays,
and keep the existing helper names ([deactivate_users_without_projects] and the
corresponding reactivation function) so the log context remains clear without
leaking identifiers.
---
Outside diff comments:
In `@backend/app/api/routes/auth.py`:
- Around line 99-125: The no-project guard is happening after user activation,
so an inactive user with zero accessible projects gets reactivated before the
403 is raised. In the Google login flow around get_user_accessible_projects and
the user.is_active commit, move the available_projects check and HTTPException
before any activation/commit logic, then only set user.is_active=True after the
guard passes. Apply the same ordering in verify_magic_link so both auth paths
behave consistently.
---
Nitpick comments:
In `@backend/app/api/routes/login.py`:
- Around line 45-51: The no-active-project HTTPException detail is duplicated
across the auth flows, so extract that literal into a shared constant and reuse
it from the login path. Update the login logic around the user/project check to
reference the shared symbol instead of embedding the string, and apply the same
constant in the Google auth and magic-link auth handlers to keep the message
consistent.
In `@backend/app/crud/project.py`:
- Around line 58-64: Rename the plural fetch helper currently named
get_projects_by_organization to list_projects_by_organization in the project
CRUD module. Update any callers to use the new list_* name, since this function
returns a list of Project objects; keep the same parameters and behavior, and
use the new symbol name consistently wherever it is referenced.
In `@backend/app/crud/user_project.py`:
- Around line 146-152: The helper currently named get_user_ids_for_project
returns multiple IDs, so rename it to list_user_ids_for_project to match the
list_* naming convention for plural fetches. Update every caller in the route
layer and elsewhere to use the new function name, keeping the behavior in the
UserProject/session query 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ae00a84-c289-485f-9d4c-fa137271dd20
📒 Files selected for processing (25)
backend/app/api/docs/organization/delete.mdbackend/app/api/docs/organization/list.mdbackend/app/api/docs/projects/delete.mdbackend/app/api/docs/projects/list.mdbackend/app/api/docs/projects/list_by_org.mdbackend/app/api/docs/user_project/delete.mdbackend/app/api/routes/auth.pybackend/app/api/routes/login.pybackend/app/api/routes/organization.pybackend/app/api/routes/project.pybackend/app/crud/organization.pybackend/app/crud/project.pybackend/app/crud/user_project.pybackend/app/models/__init__.pybackend/app/models/message.pybackend/app/tests/api/routes/test_login.pybackend/app/tests/api/routes/test_org.pybackend/app/tests/api/routes/test_project.pybackend/app/tests/api/routes/test_users.pybackend/app/tests/api/test_auth.pybackend/app/tests/api/test_deps.pybackend/app/tests/crud/test_org.pybackend/app/tests/crud/test_project.pybackend/app/tests/crud/test_user_project.pybackend/app/tests/utils/test_data.py
✅ Files skipped from review due to trivial changes (4)
- backend/app/api/docs/user_project/delete.md
- backend/app/api/docs/organization/list.md
- backend/app/api/docs/projects/list_by_org.md
- backend/app/api/docs/projects/list.md
| count = session.exec(count_statement).one() | ||
|
|
||
| statement = select(Organization).offset(skip).limit(limit) | ||
| statement = select(Organization).where(*filters).offset(skip).limit(limit) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'read_organizations|/organizations|has_more|skip|limit|order_by' backend/app/api/routes/organization.py backend/app/testsRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== organization route =="
sed -n '1,90p' backend/app/api/routes/organization.py | nl -ba | sed -n '1,90p'
echo
echo "== list routes using offset/limit with order_by =="
rg -n -C2 'select\(.*\)\.where\(\*?filters?\).*offset\(skip\)\.limit\(limit\)|\.offset\(skip\)\.limit\(limit\)|\.order_by\(' backend/app/api/routes -g '*.py'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend/app/api/routes/organization.py (lines 37-62) =="
sed -n '37,62p' backend/app/api/routes/organization.py
echo
echo "== pagination tests for organizations =="
sed -n '120,150p' backend/app/tests/api/routes/test_org.py
echo
echo "== nearby list route pattern with stable ordering =="
rg -n -C2 '\.order_by\(' backend/app/api/routes -g '*.py'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2631
Add a stable sort before paginating /organizations. skip/limit without order_by can return inconsistent pages across requests; sort by a stable key like Organization.id first.
Suggested fix
- statement = select(Organization).where(*filters).offset(skip).limit(limit)
+ statement = (
+ select(Organization)
+ .where(*filters)
+ .order_by(Organization.id)
+ .offset(skip)
+ .limit(limit)
+ )📝 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
| statement = select(Organization).where(*filters).offset(skip).limit(limit) | |
| statement = ( | |
| select(Organization) | |
| .where(*filters) | |
| .order_by(Organization.id) | |
| .offset(skip) | |
| .limit(limit) | |
| ) |
🤖 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 `@backend/app/api/routes/organization.py` at line 57, Add a stable ordering to
the organizations query before applying pagination. In the organizations route
where the `statement =
select(Organization).where(*filters).offset(skip).limit(limit)` query is built,
add an `order_by` on a deterministic key such as `Organization.id` so
`skip`/`limit` returns consistent pages across requests.
Comment on lines
43
to
55
| def read_projects( | ||
| session: SessionDep, | ||
| skip: int = Query(0, ge=0), | ||
| limit: int = Query(100, ge=1, le=100), | ||
| search: str | ||
| | None = Query( | ||
| None, description="Case-insensitive substring match on the project name" | ||
| ), | ||
| is_active: bool = Query( | ||
| True, | ||
| description="Filter by active status. Pass false to list soft-deleted projects.", | ||
| ), | ||
| ): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add return annotations to the changed route handlers.
Both handlers return APIResponse[...]; annotate them explicitly. As per coding guidelines, “Use type hints on every function parameter and return value; -> Any is not acceptable unless the type can be narrowed.”
Proposed fix
def read_projects(
session: SessionDep,
@@
is_active: bool = Query(
True,
description="Filter by active status. Pass false to list soft-deleted projects.",
),
-):
+) -> APIResponse[list[ProjectPublic]]:
@@
def delete_project_endpoint(
session: SessionDep,
project_id: int,
body: DeleteRequest | None = None,
-):
+) -> APIResponse[None]:Also applies to: 166-170
🤖 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 `@backend/app/api/routes/project.py` around lines 43 - 55, The changed route
handlers are missing explicit return type annotations even though they return
APIResponse[...] values. Update read_projects and the other affected route
handler in the same module to include precise return annotations matching the
APIResponse type they produce, keeping all parameters fully typed. Use the
existing handler names to locate the functions and ensure the signatures satisfy
the project’s type-hinting guideline.
Source: Coding guidelines
Comment on lines
+117
to
+118
| logger.info( | ||
| f"[deactivate_users_without_projects] Users deactivated | user_ids: {deactivated}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging raw user identifiers.
These helpers can log full user ID lists during bulk deactivation/reactivation. Prefer counts to reduce privacy exposure and log volume.
Proposed fix
session.flush()
logger.info(
- f"[deactivate_users_without_projects] Users deactivated | user_ids: {deactivated}"
+ f"[deactivate_users_without_projects] Users deactivated | deactivated_users: {len(deactivated)}"
)
@@
session.flush()
logger.info(
- f"[reactivate_users_with_access] Users reactivated | user_ids: {reactivated}"
+ f"[reactivate_users_with_access] Users reactivated | reactivated_users: {len(reactivated)}"
)Also applies to: 140-141
🤖 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 `@backend/app/crud/user_project.py` around lines 117 - 118, The logging in
deactivate_users_without_projects and the related reactivation helper is
exposing raw user ID lists, which should be avoided. Update the logger.info
calls in user_project.py to report counts or summary totals instead of the full
deactivated/reactivated identifier arrays, and keep the existing helper names
([deactivate_users_without_projects] and the corresponding reactivation
function) so the log context remains clear without leaking identifiers.
AkhileshNegi
approved these changes
Jul 1, 2026
|
🎉 This PR is included in version 1.3.0-main.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 tasks
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
4 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.
Target issue is #879
Summary
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.