Repository navigation
Security implementations - #87
Merged
Merged
Conversation
…vel security was not yet fully implemented. this commit makes it un-bypassable
… manually typed ones
w-richter
reviewed
Jul 9, 2026
w-richter
reviewed
Jul 9, 2026
w-richter
reviewed
Jul 9, 2026
alexander-wudy
suggested changes
Jul 9, 2026
Collaborator
|
Btw, maybe you also take a look at #86 and integrate the comments of Werner I marked with 👍 as well here |
alexander-wudy
suggested changes
Jul 9, 2026
alexander-wudy
suggested changes
Jul 10, 2026
Collaborator
|
lgtm |
Collaborator
|
lgtm now too |
Contributor
📝 WalkthroughWalkthroughThe PR adds JWT authentication to checklist and GenAI APIs, introduces generated OpenAPI response models and routing, enforces checklist ownership, updates GenAI deployment paths, and adds PostgreSQL-backed endpoint and contract tests. ChangesChecklist authentication and response contracts
GenAI generated API and runtime authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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: 8
🧹 Nitpick comments (4)
services/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.java (3)
115-117: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
Objects.equalsfor safe comparison.Using
Objects.equalssafely compares the IDs and prevents a potentialNullPointerExceptionin the unlikely event thatentity.getUserId()is null.💡 Proposed refactor
- if (!entity.getUserId().equals(userId)) { + if (!java.util.Objects.equals(entity.getUserId(), userId)) { throw new IllegalChecklistAccessException(userId, entity.getUserId(), 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 `@services/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.java` around lines 115 - 117, Update the user ID comparison in the access check around entity.getUserId() to use Objects.equals rather than invoking equals on the entity value. Preserve the existing IllegalChecklistAccessException behavior when the IDs differ, including null-versus-non-null values.
88-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract checklist item retrieval and validation into a helper.
This logic to fetch a checklist item and validate its association with the parent checklist is duplicated here and in
deleteChecklistItem. Consider extracting it into a private helper method (e.g.,getChecklistItemEntity(checklistId, itemId)) to keep the code DRY.🤖 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 `@services/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.java` around lines 88 - 92, Extract the duplicated checklist-item lookup and checklist-association validation from the current method and deleteChecklistItem into a private helper such as getChecklistItemEntity(checklistId, itemId). Preserve the existing ChecklistItemNotFoundException and ChecklistItemNotInChecklistException behavior, then replace both call sites with the helper.
121-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider delegating entity mapping to MapStruct.
Since MapStruct is already configured via
ChecklistMapper, you can define mapping methods forChecklistEntity -> IdentifiedChecklistandChecklistItemEntity -> IdentifiedChecklistItemdirectly in the mapper interface. This will automatically generate the conversion code, standardize the mapping approach across the service, and allow you to remove these manualtoDtomethods entirely.🤖 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 `@services/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.java` around lines 121 - 140, Move the ChecklistEntity and ChecklistItemEntity conversion logic from the private toDto methods in ChecklistServiceImpl into mapping methods on the existing ChecklistMapper interface. Configure any necessary field or timestamp mappings there, update service callers to use ChecklistMapper, and remove the manual toDto methods while preserving all currently mapped fields and UTC timestamp behavior.services/checklist-service/src/test/java/de/tum/devopss26/checklistservice/controller/ChecklistControllerTest.java (1)
121-129: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for cross-user access returning
403.The negative tests cover
401and404, but never exerciseIllegalChecklistAccessException. Add checklist and nested-item cases proving that another user's resource returns403and satisfies the OpenAPI contract.Also applies to: 198-233, 274-354
🤖 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 `@services/checklist-service/src/test/java/de/tum/devopss26/checklistservice/controller/ChecklistControllerTest.java` around lines 121 - 129, Add tests alongside ChecklistControllerTest cases such as getChecklistById_NOT_FOUND and the additional checklist/nested-item negative-test sections to cover IllegalChecklistAccessException for resources owned by another user. Mock the service methods with that exception, perform the corresponding authenticated checklist and nested-item requests, and assert HTTP 403 plus openApi().isValid("checklist-service.yaml").
🤖 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 @.github/workflows/ci.yml:
- Around line 46-71: Harden the python-test job by setting its job-level
GITHUB_TOKEN permissions to read-only and configuring the actions/checkout@v4
step with credential persistence disabled. Apply these changes specifically to
the python-test job without altering unrelated workflow jobs.
In `@api/checklist-service.yaml`:
- Around line 266-272: Mark the inherited server-managed createdAt and items
properties as readOnly in the shared checklist schema used by
CreateChecklistRequest and UpdateChecklistRequest, including the corresponding
schema section around the additional referenced lines. Ensure generated clients
cannot treat these fields as writable while preserving them in response models.
In `@infra/docker-compose.yml`:
- Line 182: Update the health check for the GenAI service in the docker-compose
test command to use Python’s urllib instead of curl, unless curl is explicitly
installed in the runtime image; preserve the existing health endpoint and
failure behavior.
In `@services/genai-service/Dockerfile`:
- Around line 13-23: Update the runtime stage of the Dockerfile to create a
dedicated non-root appuser, grant it ownership or access to /app as needed, and
switch to USER appuser before CMD. Keep the existing uvicorn startup command
unchanged.
- Around line 8-10: Pin the OpenAPI generator used by the Docker build: update
the generator invocation in the RUN command to use an explicit package version,
or copy and apply the repository’s openapitools.json before generation. Ensure
clean builds resolve the same fixed generator version instead of selecting it
dynamically.
In `@services/genai-service/main.py`:
- Around line 453-489: Refactor the conversation/message flow in the async
session block so conversation creation or lookup and the initial USER message
are committed before calling _fetch_user_data, _rag_retrieve, and _build_chain.
Perform those external calls after the session is released, then open a new
short-lived _sessions transaction to add the AGENT message and commit it using
the persisted conversation.id.
- Around line 120-140: Update _fetch_public_key_locked and
_get_or_fetch_public_key to record the time of failed fetch attempts and enforce
a cooldown before retrying. When a fetch fails, preserve _public_key as None,
update _last_fetch_time, and have subsequent callers return the existing
unavailable result without making another request until the cooldown expires, so
the authentication flow can respond with 503.
In `@services/genai-service/tests/conftest.py`:
- Around line 46-60: Update the database setup fixture around db_url and
schema_engine to require an explicitly configured, test-only database URL or
name before calling main.Base.metadata.drop_all. Validate it against an explicit
allowlist and fail fast when the configured value is absent or not allowlisted;
do not preserve or reuse injected production database values through defaults.
---
Nitpick comments:
In
`@services/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.java`:
- Around line 115-117: Update the user ID comparison in the access check around
entity.getUserId() to use Objects.equals rather than invoking equals on the
entity value. Preserve the existing IllegalChecklistAccessException behavior
when the IDs differ, including null-versus-non-null values.
- Around line 88-92: Extract the duplicated checklist-item lookup and
checklist-association validation from the current method and deleteChecklistItem
into a private helper such as getChecklistItemEntity(checklistId, itemId).
Preserve the existing ChecklistItemNotFoundException and
ChecklistItemNotInChecklistException behavior, then replace both call sites with
the helper.
- Around line 121-140: Move the ChecklistEntity and ChecklistItemEntity
conversion logic from the private toDto methods in ChecklistServiceImpl into
mapping methods on the existing ChecklistMapper interface. Configure any
necessary field or timestamp mappings there, update service callers to use
ChecklistMapper, and remove the manual toDto methods while preserving all
currently mapped fields and UTC timestamp behavior.
In
`@services/checklist-service/src/test/java/de/tum/devopss26/checklistservice/controller/ChecklistControllerTest.java`:
- Around line 121-129: Add tests alongside ChecklistControllerTest cases such as
getChecklistById_NOT_FOUND and the additional checklist/nested-item
negative-test sections to cover IllegalChecklistAccessException for resources
owned by another user. Mock the service methods with that exception, perform the
corresponding authenticated checklist and nested-item requests, and assert HTTP
403 plus openApi().isValid("checklist-service.yaml").
🪄 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: 20833d23-c30d-45af-9d2d-1f1325a21fa8
⛔ Files ignored due to path filters (4)
services/genai-service/__pycache__/main.cpython-311.pycis excluded by!**/*.pycservices/genai-service/tests/__pycache__/conftest.cpython-311-pytest-8.3.3.pycis excluded by!**/*.pycservices/genai-service/tests/__pycache__/test_endpoints.cpython-311-pytest-8.3.3.pycis excluded by!**/*.pycservices/genai-service/tests/__pycache__/test_openapi_contract.cpython-311-pytest-8.3.3.pycis excluded by!**/*.pyc
📒 Files selected for processing (20)
.github/workflows/ci.yml.gitignoreapi/checklist-service.yamlapi/genai-service.yamlinfra/docker-compose.ymlinfra/iac/aet/templates/genai-service/deployment.yamlservices/checklist-service/pom.xmlservices/checklist-service/src/main/java/de/tum/devopss26/checklistservice/controller/ChecklistController.javaservices/checklist-service/src/main/java/de/tum/devopss26/checklistservice/exception/IllegalChecklistAccessException.javaservices/checklist-service/src/main/java/de/tum/devopss26/checklistservice/mapper/ChecklistMapper.javaservices/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistService.javaservices/checklist-service/src/main/java/de/tum/devopss26/checklistservice/service/ChecklistServiceImpl.javaservices/checklist-service/src/test/java/de/tum/devopss26/checklistservice/controller/ChecklistControllerTest.javaservices/genai-service/Dockerfileservices/genai-service/main.pyservices/genai-service/pytest.iniservices/genai-service/requirements-dev.txtservices/genai-service/tests/conftest.pyservices/genai-service/tests/test_endpoints.pyservices/genai-service/tests/test_openapi_contract.py
Comment on lines
+46
to
+71
| python-test: | ||
| name: Test Python (GenAI Service) | ||
| needs: openapi-lint | ||
| runs-on: ubuntu-latest | ||
| services: | ||
| postgres: | ||
| image: postgres:16-alpine | ||
| env: | ||
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: genai_service_db | ||
| ports: | ||
| - 5432:5432 | ||
| options: >- | ||
| --health-cmd pg_isready | ||
| --health-interval 5s | ||
| --health-timeout 5s | ||
| --health-retries 10 | ||
| env: | ||
| SERVICES_POSTGRES_USER: postgres | ||
| SERVICES_POSTGRES_PASSWORD: postgres | ||
| SERVICES_POSTGRES_URL: localhost | ||
| SERVICES_POSTGRES_PORT_INT: 5432 | ||
| steps: | ||
| - name: Checkout Code | ||
| uses: actions/checkout@v4 |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict the test job's GitHub token.
This job executes third-party installers and repository tests while inheriting default token permissions and retaining checkout credentials. Limit the token to read-only access and disable credential persistence.
Proposed hardening
python-test:
name: Test Python (GenAI Service)
needs: openapi-lint
runs-on: ubuntu-latest
+ permissions:
+ contents: read
...
- name: Checkout Code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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
| python-test: | |
| name: Test Python (GenAI Service) | |
| needs: openapi-lint | |
| runs-on: ubuntu-latest | |
| services: | |
| postgres: | |
| image: postgres:16-alpine | |
| env: | |
| POSTGRES_USER: postgres | |
| POSTGRES_PASSWORD: postgres | |
| POSTGRES_DB: genai_service_db | |
| ports: | |
| - 5432:5432 | |
| options: >- | |
| --health-cmd pg_isready | |
| --health-interval 5s | |
| --health-timeout 5s | |
| --health-retries 10 | |
| env: | |
| SERVICES_POSTGRES_USER: postgres | |
| SERVICES_POSTGRES_PASSWORD: postgres | |
| SERVICES_POSTGRES_URL: localhost | |
| SERVICES_POSTGRES_PORT_INT: 5432 | |
| steps: | |
| - name: Checkout Code | |
| uses: actions/checkout@v4 | |
| python-test: | |
| name: Test Python (GenAI Service) | |
| needs: openapi-lint | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| services: | |
| postgres: | |
| image: postgres:16-alpine | |
| env: | |
| POSTGRES_USER: postgres | |
| POSTGRES_PASSWORD: postgres | |
| POSTGRES_DB: genai_service_db | |
| ports: | |
| - 5432:5432 | |
| options: >- | |
| --health-cmd pg_isready | |
| --health-interval 5s | |
| --health-timeout 5s | |
| --health-retries 10 | |
| env: | |
| SERVICES_POSTGRES_USER: postgres | |
| SERVICES_POSTGRES_PASSWORD: postgres | |
| SERVICES_POSTGRES_URL: localhost | |
| SERVICES_POSTGRES_PORT_INT: 5432 | |
| steps: | |
| - name: Checkout Code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 70-71: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 46-100: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml around lines 46 - 71, Harden the python-test job by
setting its job-level GITHUB_TOKEN permissions to read-only and configuring the
actions/checkout@v4 step with credential persistence disabled. Apply these
changes specifically to the python-test job without altering unrelated workflow
jobs.
Source: Linters/SAST tools
Comment on lines
266
to
+272
| createdAt: | ||
| type: string | ||
| format: date-time | ||
| items: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/ChecklistItem' | ||
| $ref: '#/components/schemas/IdentifiedChecklistItem' |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark server-managed checklist fields as read-only.
CreateChecklistRequest and UpdateChecklistRequest inherit createdAt and items, but the controller discards both. Generated clients can therefore submit data that is silently ignored. Use a write-only base schema or mark these properties readOnly.
Proposed contract fix
createdAt:
type: string
format: date-time
+ readOnly: true
items:
type: array
+ readOnly: true
items:
$ref: '`#/components/schemas/IdentifiedChecklistItem`'Also applies to: 311-337
🤖 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 `@api/checklist-service.yaml` around lines 266 - 272, Mark the inherited
server-managed createdAt and items properties as readOnly in the shared
checklist schema used by CreateChecklistRequest and UpdateChecklistRequest,
including the corresponding schema section around the additional referenced
lines. Ensure generated clients cannot treat these fields as writable while
preserving them in response models.
| condition: service_started | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "curl -f http://localhost:${GENAI_SERVICE_APP_PORT}/health || exit 1"] | ||
| test: ["CMD-SHELL", "curl -f http://localhost:${GENAI_SERVICE_APP_PORT}/api/genai/api/v1/health || exit 1"] |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a health-check executable that exists in the runtime image.
This health check depends on curl, but the runtime image doesn’t install it, so a healthy Uvicorn process can still be marked unhealthy. Use Python’s urllib instead, or add curl to the image.
🤖 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 `@infra/docker-compose.yml` at line 182, Update the health check for the GenAI
service in the docker-compose test command to use Python’s urllib instead of
curl, unless curl is explicitly installed in the runtime image; preserve the
existing health endpoint and failure behavior.
Comment on lines
+8
to
+10
| RUN npx --yes @openapitools/openapi-generator-cli generate \ | ||
| -i api/genai-service.yaml -g python-fastapi \ | ||
| -o services/genai-service/generated --skip-validate-spec |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'Dockerfile:\n'
sed -n '1,40p' services/genai-service/Dockerfile
printf '\nRelevant package/lock files:\n'
git ls-files | rg '(^|/)(package\.json|package-lock\.json|npm-shrinkwrap\.json|pnpm-lock\.yaml|yarn.lock|bun.lockb)$'
printf '\nSearch for openapi-generator-cli references:\n'
rg -n "`@openapitools/openapi-generator-cli`|openapi-generator-cli" -S .Repository: AET-DevOps26/team-devopss26
Length of output: 1669
🏁 Script executed:
set -euo pipefail
printf 'openapitools.json:\n'
if [ -f openapitools.json ]; then
cat -n openapitools.json
else
echo '(missing)'
fi
printf '\npackage.json:\n'
if [ -f package.json ]; then
cat -n package.json
fi
printf '\napi/scripts/gen-all.sh:\n'
if [ -f api/scripts/gen-all.sh ]; then
cat -n api/scripts/gen-all.sh
fi
printf '\nSearch for generatorVersion/version pins:\n'
rg -n '"generatorVersion"|"version"' openapitools.json package.json api/scripts/gen-all.sh services/genai-service/Dockerfile -SRepository: AET-DevOps26/team-devopss26
Length of output: 2883
Pin the generator in this Docker stage. services/genai-service/Dockerfile doesn’t copy the repo-level openapitools.json, so npx @openapitools/openapi-generator-cli generate still resolves tooling dynamically on clean builds. Copy the config into the image or pin the package version here.
🤖 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 `@services/genai-service/Dockerfile` around lines 8 - 10, Pin the OpenAPI
generator used by the Docker build: update the generator invocation in the RUN
command to use an explicit package version, or copy and apply the repository’s
openapitools.json before generation. Ensure clean builds resolve the same fixed
generator version instead of selecting it dynamically.
Comment on lines
13
to
+23
| FROM python:3.12-slim | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY requirements.txt . | ||
| COPY services/genai-service/requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
|
||
| COPY main.py . | ||
| COPY services/genai-service/main.py . | ||
| COPY --from=codegen /app/services/genai-service/generated/src ./generated/src | ||
|
|
||
| CMD ["sh", "-c", "uvicorn main:app --host 0.0.0.0 --port ${GENAI_SERVICE_APP_PORT:-8006} --root-path /api/genai"] | ||
| CMD ["sh", "-c", "uvicorn main:app --host 0.0.0.0 --port ${GENAI_SERVICE_APP_PORT:-8006}"] |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Drop root privileges in the runtime stage.
Create a dedicated application user and add USER appuser before CMD; this service does not need root privileges.
🤖 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 `@services/genai-service/Dockerfile` around lines 13 - 23, Update the runtime
stage of the Dockerfile to create a dedicated non-root appuser, grant it
ownership or access to /app as needed, and switch to USER appuser before CMD.
Keep the existing uvicorn startup command unchanged.
Source: Linters/SAST tools
Comment on lines
+120
to
+140
| async def _fetch_public_key_locked(): | ||
| global _public_key, _last_fetch_time | ||
| try: | ||
| async with httpx.AsyncClient(timeout=5.0) as client: | ||
| resp = await client.get(f"{_USER_SERVICE_URL}{_PUBLIC_KEY_PATH}") | ||
| resp.raise_for_status() | ||
| base64_key = resp.json()["publicKey"] | ||
| _public_key = load_der_public_key(base64.b64decode(base64_key)) | ||
| _last_fetch_time = time.monotonic() | ||
| except Exception: | ||
| _public_key = None | ||
| return _public_key | ||
|
|
||
|
|
||
| async def _get_or_fetch_public_key(): | ||
| if _public_key is not None: | ||
| return _public_key | ||
| async with _public_key_lock: | ||
| if _public_key is not None: | ||
| return _public_key | ||
| return await _fetch_public_key_locked() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Throttle failed public-key fetches.
On failure, _last_fetch_time remains unchanged, so every authenticated request serially retries the five-second user-service call. Record failed attempts and apply a cooldown/backoff, returning 503 until another fetch is allowed.
🤖 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 `@services/genai-service/main.py` around lines 120 - 140, Update
_fetch_public_key_locked and _get_or_fetch_public_key to record the time of
failed fetch attempts and enforce a cooldown before retrying. When a fetch
fails, preserve _public_key as None, update _last_fetch_time, and have
subsequent callers return the existing unavailable result without making another
request until the cooldown expires, so the authentication flow can respond with
503.
Comment on lines
+453
to
+489
| async with _sessions() as db: | ||
| if chat_request.conversation_id: | ||
| result = await db.execute( | ||
| select(ChatConversation).where(ChatConversation.id == chat_request.conversation_id) | ||
| ) | ||
| conversation = result.scalar_one_or_none() | ||
| if not conversation: | ||
| raise HTTPException(status_code=404, detail="Conversation not found") | ||
| if conversation.user_id != user_id: | ||
| raise HTTPException(status_code=403, detail="Access denied") | ||
| else: | ||
| conversation = ChatConversation(user_id=user_id, title=chat_request.message[:100]) | ||
| db.add(conversation) | ||
| await db.flush() | ||
|
|
||
| db.add(ChatMessage( | ||
| conversation_id=conversation.id, role="USER", | ||
| content=chat_request.message, timestamp=_now_ms(), | ||
| )) | ||
|
|
||
| context = "" | ||
| try: | ||
| chunks = await _fetch_user_data(user_id) | ||
| context = await _rag_retrieve(chat_request.message, chunks) | ||
| except Exception: | ||
| pass | ||
|
|
||
| response_text = await _build_chain(chat_request.model).ainvoke({ | ||
| "message": chat_request.message, | ||
| "context": context or "No relevant data found in the user's notes, calendar, or checklists.", | ||
| }) | ||
|
|
||
| db.add(ChatMessage( | ||
| conversation_id=conversation.id, role="AGENT", | ||
| content=response_text, timestamp=_now_ms(), | ||
| )) | ||
| await db.commit() |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Release the database transaction before external calls.
The select/flush starts a transaction that remains open across downstream HTTP, RAG, and LLM calls. Their latency can exhaust the connection pool under load. Persist the initial state, release the session, perform external work, then use a short transaction to store the agent response.
🤖 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 `@services/genai-service/main.py` around lines 453 - 489, Refactor the
conversation/message flow in the async session block so conversation creation or
lookup and the initial USER message are committed before calling
_fetch_user_data, _rag_retrieve, and _build_chain. Perform those external calls
after the session is released, then open a new short-lived _sessions transaction
to add the AGENT message and commit it using the persisted conversation.id.
Comment on lines
+46
to
+60
| db_url = ( | ||
| f"postgresql+asyncpg://{os.environ['SERVICES_POSTGRES_USER']}:" | ||
| f"{os.environ['SERVICES_POSTGRES_PASSWORD']}@" | ||
| f"{os.environ['SERVICES_POSTGRES_URL']}:" | ||
| f"{os.environ['SERVICES_POSTGRES_PORT_INT']}/genai_service_db" | ||
| ) | ||
| # Schema setup happens on its own throwaway engine/connection so that | ||
| # main._engine's connection pool isn't bound to *this* event loop: some | ||
| # callers (schemathesis's call_asgi) run requests on a different loop than | ||
| # this fixture's, and asyncpg connections can't cross loops. | ||
| schema_engine = create_async_engine(db_url) | ||
| async with schema_engine.begin() as conn: | ||
| await conn.run_sync(main.Base.metadata.drop_all) | ||
| await conn.run_sync(main.Base.metadata.create_all) | ||
| await schema_engine.dispose() |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Require a dedicated test database before dropping tables.
This fixture targets genai_service_db using generic service credentials and immediately calls drop_all. Because setdefault preserves injected production values, an accidental test run in a deployed environment can erase production data. Require a test-only URL/database name and fail unless it is explicitly allowlisted.
🤖 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 `@services/genai-service/tests/conftest.py` around lines 46 - 60, Update the
database setup fixture around db_url and schema_engine to require an explicitly
configured, test-only database URL or name before calling
main.Base.metadata.drop_all. Validate it against an explicit allowlist and fail
fast when the configured value is absent or not allowlisted; do not preserve or
reuse injected production database values through defaults.
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
3 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.
Here is the summary of changes:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation