feat(agent): bootstrap FastAPI agent service#765
Closed
Zawiszowski wants to merge 34 commits intothe-momentum:mainfrom
Closed
feat(agent): bootstrap FastAPI agent service#765Zawiszowski wants to merge 34 commits intothe-momentum:mainfrom
Zawiszowski wants to merge 34 commits intothe-momentum:mainfrom
Conversation
- FastAPI app with /session and /chat endpoints, JWT auth - Celery worker, beat and Flower setup with Redis broker - SQLAlchemy async models, Alembic migrations scaffold - ChatSession expiration periodic task (every 5 min, 1h TTL) - Docker Compose services: agent, celery-worker, celery-beat - Process message Celery task with WorkflowAgent placeholder
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new standalone Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant FastAPI as FastAPI\nAgent API
participant ConvService as Conversation\nService
participant Celery as Celery\nTask Queue
participant Workflow as LLM\nWorkflow Engine
participant DB as PostgreSQL\nDatabase
participant OW as Open Wearables\nAPI
Client->>FastAPI: POST /api/v1/chat/{conversation_id}\n(message, callback_url)
FastAPI->>FastAPI: Validate JWT
FastAPI->>ConvService: get_active(conversation_id, user_id)
ConvService->>DB: Query conversation & session
DB-->>ConvService: Conversation + Session
ConvService-->>FastAPI: Active session
FastAPI->>Celery: Enqueue process_message(...)\n(return task_id)
FastAPI-->>Client: 200 {task_id}
Celery->>Workflow: Run workflow_engine.run(user_id, message, history)
Workflow->>OW: Fetch wearables data (tools)
OW-->>Workflow: Metrics
Workflow-->>Celery: response_text
Celery->>ConvService: save_messages(user_msg, assistant_msg)
ConvService->>DB: Persist messages & increment counts
Celery->>Client: POST callback_url {task_id, response_text}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
agent/app/schemas/error_code.py-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorFix typo in enum member name (
VALIDATION_ERRROR).Line 6 likely intended
VALIDATION_ERROR. The current typo can break call sites expecting the conventional name.Suggested fix
- VALIDATION_ERRROR = "validation_failed" + VALIDATION_ERROR = "validation_failed"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/schemas/error_code.py` at line 6, Rename the enum member VALIDATION_ERRROR to VALIDATION_ERROR in the error_code.py enum to fix the typo; update all references/usages that import or compare against VALIDATION_ERRROR to use VALIDATION_ERROR (search for VALIDATION_ERRROR, update usages in callers/tests), and run tests/static checks to ensure no remaining references break.agent/Dockerfile-21-38 (1)
21-38:⚠️ Potential issue | 🟠 MajorRun the runtime container as a non-root user.
There is no
USERin the runtime stage (Lines 21-38), so the app runs as root. This increases blast radius if compromised.Suggested hardening
FROM python:3.13-slim @@ WORKDIR /root_project + RUN groupadd -r app && useradd -r -g app app @@ ENV UV_PROJECT_ENVIRONMENT=/opt/venv EXPOSE 8000 + USER app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/Dockerfile` around lines 21 - 38, The Dockerfile runtime stage currently runs as root; add a non-root user and switch to it with USER to reduce blast radius. Create a dedicated user/group (e.g., appuser/appgroup), chown the application directories used at runtime (/root_project and /opt/venv) and any needed runtime PATH locations, set a safe HOME if required, and then add USER appuser at the end of the runtime stage (before EXPOSE 8000). Ensure the created user has permission to read/execute the VIRTUAL_ENV (/opt/venv) and write where the app needs to write; adjust ownership/permissions in the runtime stage after copying files and before switching users.agent/scripts/postgres/init.sh-4-5 (1)
4-5:⚠️ Potential issue | 🟠 MajorDatabase bootstrap is not idempotent and can fail startup.
Line 5 hardcodes
agentand unconditionally creates it. If that DB already exists (common whenPOSTGRES_DB=agent), initialization fails and the container exits.Suggested idempotent pattern
psql -v ON_ERROR_STOP=1 --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL - CREATE DATABASE agent OWNER "$POSTGRES_USER"; + SELECT 'CREATE DATABASE agent OWNER "' || :'POSTGRES_USER' || '"' + WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'agent') \gexec EOSQL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/postgres/init.sh` around lines 4 - 5, The current psql invocation unconditionally runs CREATE DATABASE agent which fails if that DB already exists; change the SQL here (the psql command that contains CREATE DATABASE agent) to perform an existence check against pg_database and only run CREATE DATABASE when the database name (agent or $POSTGRES_DB) is not present (i.e., replace the hardcoded CREATE DATABASE agent with a conditional CREATE via a SELECT/DO that queries pg_database for datname = 'agent' or uses $POSTGRES_DB). Ensure the script uses the POSTGRES_DB env var rather than hardcoding "agent" so initialization is idempotent.agent/scripts/cryptography/encrypt_setting.py-5-7 (1)
5-7:⚠️ Potential issue | 🟠 MajorAdd arg validation and decode ciphertext to UTF-8 string for clean CLI output.
Lines 6-7 crash on missing arguments and print a Python bytes literal (
b'...') rather than a usable token string. Refactoring into a typedmain()function aligns with the project's type annotation requirements.Suggested fix
import sys from cryptography.fernet import Fernet -if __name__ == "__main__": - fernet = Fernet(sys.argv[1].encode("utf-8")) - print(fernet.encrypt(sys.argv[2].encode("utf-8"))) +def main(argv: list[str]) -> int: + if len(argv) != 3: + print("Usage: encrypt_setting.py <fernet_key> <plaintext>") + return 1 + fernet = Fernet(argv[1].encode("utf-8")) + token = fernet.encrypt(argv[2].encode("utf-8")).decode("utf-8") + print(token) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/cryptography/encrypt_setting.py` around lines 5 - 7, Refactor the top-level script into a typed main() function that validates CLI args and returns an exit code: check len(sys.argv) and print a usage error + non-zero exit when missing arguments, construct Fernet from sys.argv[1].encode("utf-8") and encrypt sys.argv[2].encode("utf-8"), then decode the resulting ciphertext to a UTF-8 string with .decode("utf-8") before printing so the CLI shows a clean token (no b'...'); ensure main() has type annotations (-> int) and call it from if __name__ == "__main__" with sys.exit(main()) and handle exceptions to log or return non-zero on failure.agent/scripts/healthchecks/db_up_check.py-7-17 (1)
7-17:⚠️ Potential issue | 🟠 MajorAdd explicit DB connect timeout for reliable healthchecks and fix typo.
Missing
connect_timeoutcan stall startup/healthcheck loops during network issues. Also fix the typo "unavaliable" → "unavailable" and use the conventional exit codesys.exit(1)instead ofsys.exit(-1).Suggested fix
try: - psycopg.connect( + with psycopg.connect( dbname=os.getenv("DB_NAME", ""), user=os.getenv("DB_USER", ""), password=os.getenv("DB_PASSWORD", ""), host=os.getenv("DB_HOST", ""), port=os.getenv("DB_PORT", ""), + connect_timeout=5, - ) + ): + pass except psycopg.OperationalError: - print("- PostgreSQL unavaliable - waiting") - sys.exit(-1) + print("- PostgreSQL unavailable - waiting") + sys.exit(1) sys.exit(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/healthchecks/db_up_check.py` around lines 7 - 17, The healthcheck call to psycopg.connect is missing an explicit connection timeout and uses a misspelled message and non-standard exit code; update the psycopg.connect(...) call to include connect_timeout (e.g., connect_timeout=5) to avoid stalling, change the except handler message from "unavaliable" to "unavailable", and replace sys.exit(-1) with sys.exit(1) in the psycopg.OperationalError except block (refer to psycopg.connect and the except psycopg.OperationalError handling and the print/sys.exit calls).agent/app/api/__init__.py-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorApply API version prefix at the head router include point.
Line 6 currently mounts
v1_routerwithout a version prefix. That breaks the established router layering pattern and can lead to inconsistent API paths as the service evolves.Based on learnings: "Head router should include version routers with version prefix from settings, NO tags" and "Main router should include head_router with NO prefix".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/api/__init__.py` at line 6, The head_router currently mounts v1_router without the API version prefix; change the include on head_router to use the configured version prefix (e.g. settings.API_V1_STR or API_V1_PREFIX) so that head_router.include_router(v1_router, prefix=settings.API_V1_STR) is used, and ensure no tags are added at this include point; the main router should remain unchanged and include head_router with no prefix.agent/scripts/start/app.sh-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorAvoid running Alembic migrations in every API container startup.
Line 6 can cause rollout contention or startup failures when more than one app instance starts simultaneously. Prefer a one-off migration job/init step before app replicas come up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/start/app.sh` around lines 5 - 6, The script currently runs migrations unconditionally by executing the command "uv run alembic upgrade head", which causes rollout contention; modify the startup script to avoid running Alembic on every container start—either remove that line and run migrations as a separate one-off/init job, or guard it behind an explicit check such as an environment flag (e.g., RUN_MIGRATIONS=true) so the "uv run alembic upgrade head" command only runs when intentionally requested during deployment.agent/scripts/start/worker.sh-4-4 (1)
4-4: 🛠️ Refactor suggestion | 🟠 MajorUse
execwhen starting Celery from the entrypoint script.Line 4 should replace the shell process so signals reach Celery directly (clean shutdown in containers).
Suggested patch
-uv run celery -A app.main:celery_app worker --loglevel=info +exec uv run celery -A app.main:celery_app worker --loglevel=info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/start/worker.sh` at line 4, The entrypoint script currently launches Celery with "uv run celery -A app.main:celery_app worker --loglevel=info" which leaves the shell as PID 1 and prevents signals from reaching Celery; update the command in worker.sh to use exec so the shell is replaced by the Celery process (prepend exec to the existing uv run celery invocation) to ensure proper signal propagation and clean shutdown.agent/scripts/start/flower.sh-2-2 (1)
2-2:⚠️ Potential issue | 🟠 MajorDisable command echoing to avoid leaking broker credentials.
set -xcan expose sensitive values (including broker URLs with credentials) in logs.🔒 Suggested hardening
-set -e -x +set -euo pipefail +# Optional for local debugging only: +[[ "${DEBUG:-0}" == "1" ]] && set -x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/start/flower.sh` at line 2, Remove command echoing by dropping the -x flag from the shell options in the start script (replace set -e -x with a non-verbose mode such as set -e) so broker credentials and other sensitive values are not printed to logs; if debugging trace is required, enable -x only under a guarded conditional (e.g., an environment variable like DEBUG) around the commands needing tracing to avoid leaking secrets in normal runs.agent/app/schemas/chat.py-13-13 (1)
13-13:⚠️ Potential issue | 🟠 MajorEnforce HTTPS-only callbacks and add SSRF guardrails.
The
AnyHttpUrltype accepts non-TLShttp://schemes, creating transport security risk for callback dispatches. Additionally, the callback sender inagent/app/integrations/celery/tasks/process_message.py(line 32–36) makes direct HTTP requests to the callback URL without validating against private IP ranges, loopback, or link-local addresses.🔐 Suggested schema tightening
-from pydantic import AnyHttpUrl, BaseModel, Field +from pydantic import BaseModel, Field, HttpsUrl @@ class ChatRequest(BaseModel): message: str = Field(..., min_length=1, max_length=4000) - callback_url: AnyHttpUrl + callback_url: HttpsUrlAdditionally, implement allowlist/denylist logic in the callback dispatch to reject private/loopback/link-local targets (e.g., using
ipaddress.ip_address()to validate the resolved IP before posting).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/schemas/chat.py` at line 13, Change the callback_url schema to enforce TLS and add runtime SSRF guards: replace the AnyHttpUrl usage in the callback_url field (schemas/chat.py) with a URL type that is validated for scheme "https" (e.g., use pydantic HttpUrl/AnyUrl plus a validator that asserts url.scheme == "https" and raises a validation error if not). In the callback sender code in agent/app/integrations/celery/tasks/process_message.py (the block that posts to the callback URL), before making the HTTP request resolve the callback host to IP(s) and validate each ipaddress.ip_address(...) to reject loopback, link-local, private, and other non-routable ranges (and raise/log and abort dispatch); only proceed to post if the scheme is https and resolved IPs pass the allowlist/denylist checks. Ensure errors are logged and surfaced cleanly (ValidationError/ValueError) so invalid callbacks are rejected early.agent/scripts/start/flower.sh-7-16 (1)
7-16:⚠️ Potential issue | 🟠 MajorPass the explicit broker to the readiness probe to match the startup command.
The
worker_ready()function usescelery inspect pingwithout specifying--broker, relying on the app's default configuration, while the Flower startup command explicitly passes--broker="$CELERY_BROKER_URL". If these resolve to different brokers (e.g., due to environment differences or configuration overrides), the readiness probe could pass against the wrong broker, causing false positives.Suggested fix
worker_ready() { - uv run celery -A app.main:celery_app inspect ping + uv run celery --app=app.main:celery_app --broker="$CELERY_BROKER_URL" inspect ping }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/start/flower.sh` around lines 7 - 16, The readiness probe uses the worker_ready() function which calls "uv run celery -A app.main:celery_app inspect ping" without a broker, potentially checking a different broker than the Flower startup line; update worker_ready() to pass the same broker option used by Flower (add --broker="$CELERY_BROKER_URL" to the celery inspect/ping invocation) so both the readiness probe and the startup command target the identical broker.agent/app/middlewares.py-9-17 (1)
9-17:⚠️ Potential issue | 🟠 Major
allow_credentials=Trueis ineffective whenallow_origins=["*"].Per the CORS specification, browsers reject
Access-Control-Allow-Credentials: truewhenAccess-Control-Allow-Origin: *. This combination silently fails—credentials (cookies, auth headers) will not be sent by browsers whencors_allow_allis enabled.Consider either:
- Disabling credentials when using wildcard origins, or
- Echoing back the request's
Originheader (which FastAPI'sCORSMiddlewaredoes automatically when you provide a list of allowed origins, but not with literal"*")🔧 Proposed fix
def add_cors_middleware(app: FastAPI) -> None: cors_origins = [str(origin).rstrip("/") for origin in settings.cors_origins] + allow_credentials = True if settings.cors_allow_all: cors_origins = ["*"] + allow_credentials = False # Credentials not supported with wildcard origin app.add_middleware( CORSMiddleware, # type: ignore[invalid-argument-type] allow_origins=cors_origins, - allow_credentials=True, + allow_credentials=allow_credentials, allow_methods=["*"], allow_headers=["*"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/middlewares.py` around lines 9 - 17, The current setup uses allow_origins=["*"] while also passing allow_credentials=True (in app.add_middleware with CORSMiddleware), which is invalid per CORS rules; update the logic so that when settings.cors_allow_all is true you disable credentials (set allow_credentials=False) or, if you need credentials, do not use the wildcard origin and instead populate cors_origins with a concrete list of allowed origins (so CORSMiddleware will echo the request Origin). Concretely, modify the block that sets cors_origins and the app.add_middleware call (references: settings.cors_allow_all, cors_origins, app.add_middleware, CORSMiddleware, allow_credentials) to choose one of the two behaviors above and ensure allow_credentials matches the origin policy.agent/app/main.py-27-29 (1)
27-29:⚠️ Potential issue | 🟠 MajorFragile exception message extraction may raise runtime errors.
exc.args[0][0]["msg"]assumes a specific nested structure. If the validation error has a different format, this will raiseIndexError,KeyError, orTypeError, masking the original error.🛡️ Suggested defensive fix
`@api.exception_handler`(RequestValidationError) async def request_validation_exception_handler(_: Request, exc: RequestValidationError) -> None: - raise handle_exception(exc, err_msg=exc.args[0][0]["msg"]) + try: + err_msg = exc.errors()[0].get("msg", str(exc)) + except (IndexError, KeyError, TypeError): + err_msg = str(exc) + raise handle_exception(exc, err_msg=err_msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/main.py` around lines 27 - 29, The current request_validation_exception_handler assumes exc.args[0][0]["msg"] exists and can raise IndexError/KeyError/TypeError; modify request_validation_exception_handler to defensively extract the validation message for passing to handle_exception by trying the structured path (exc.args -> first item -> first item -> "msg") inside a try/except and falling back to safe alternatives such as str(exc), getattr(exc, "errors", None), or joining messages from exc.errors() if available; ensure you still call raise handle_exception(exc, err_msg=<safe_message>) so handle_exception gets a reliable string even when the validation error format differs.agent/app/integrations/celery/tasks/process_message.py-27-29 (1)
27-29:⚠️ Potential issue | 🟠 MajorSilent failure when session not found—callback still reports success.
If
sessionisNone, messages are silently not saved, but the callback at line 32-36 still reports"status": "done". This could cause data loss without any visibility to the caller. Consider logging a warning or reporting failure in the callback.🛡️ Suggested fix
async with AsyncSessionLocal() as db: session = await chat_session_repository.get_by_id(db, UUID(session_id)) if session: await ChatSessionService(db).save_messages(UUID(conversation_id), message, response_text, session) + else: + logger.warning(f"Session {session_id} not found, message not persisted") try: async with httpx.AsyncClient(timeout=10.0) as client: await client.post( callback_url, - json={"task_id": task_id, "status": "done", "result": response_text}, + json={ + "task_id": task_id, + "status": "done" if session else "error", + "result": response_text if session else "Session not found", + }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/celery/tasks/process_message.py` around lines 27 - 29, The code silently skips saving when session is None and still sends a success callback; update the logic around chat_session_repository.get_by_id so that if session is None you log a warning and send a failure callback (or set "status":"failed") instead of the current success response, and only call ChatSessionService(db).save_messages(UUID(conversation_id), message, response_text, session) when a valid session exists; reference the session variable, chat_session_repository.get_by_id, and ChatSessionService.save_messages to locate the change and ensure the callback block (currently sending "status":"done") reflects the error path.agent/app/utils/healthcheck.py-23-35 (1)
23-35:⚠️ Potential issue | 🟠 MajorFunction should use
definstead ofasync deffor synchronous database calls, and return non-2xx status for unhealthy state.Two issues:
The function is defined as
async defbut calls synchronousDbSessionmethods (line 25). This blocks the FastAPI event loop. Usedefinstead, which FastAPI automatically offloads to a threadpool for sync I/O. Only switch toasync defonce the repository layer is fully async (tracked in issues#571and#622).Returning HTTP 200 with
"status": "unhealthy"is problematic—Kubernetes probes and load balancers expect non-2xx status codes for unhealthy services.🐛 Suggested fix
+from fastapi.responses import JSONResponse + `@healthcheck_router.get`("/db") -async def database_health(db: DbSession) -> dict[str, str | dict[str, str]]: +def database_health(db: DbSession) -> dict[str, str | dict[str, str]]: """Database health check endpoint.""" try: # Test connection db.execute(text("SELECT 1")) pool_status = get_pool_status() return { "status": "healthy", "pool": pool_status, } except Exception as e: - return { + return JSONResponse( + status_code=503, + content={ "status": "unhealthy", "error": str(e), - } + }, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/healthcheck.py` around lines 23 - 35, Change the healthcheck endpoint from async def to def because it calls synchronous DbSession methods (replace "async def" with "def" in the healthcheck function), and when the DB call fails return a non-2xx HTTP status (e.g. raise fastapi.HTTPException(status_code=503) or return a Response with status_code=503) instead of returning {"status":"unhealthy"} with 200; update the exception branch that currently returns {"status":"unhealthy","error": str(e)} to emit the 503 status and include the error body, and leave the successful branch returning the healthy payload unchanged (references: the healthcheck function, db.execute(text("SELECT 1")), and get_pool_status()).docker-compose.yml-18-18 (1)
18-18:⚠️ Potential issue | 🟠 MajorDon't rely on
/docker-entrypoint-initdb.dfor already-initialized volumes.This script only runs when Postgres creates a fresh data directory. Anyone who already has
postgres_datafrom the existing stack will skip this bootstrap entirely, so the newagent*services can start against a partially initialized database until the volume is recreated or the bootstrap is run manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 18, Bootstrap relying on /docker-entrypoint-initdb.d only runs on fresh Postgres data dirs so existing postgres_data can skip init and leave agent services partially initialized; modify the compose to ensure initialization always runs: add a dedicated one-shot init container/service that mounts ./agent/scripts/postgres/init.sh, waits for the postgres server to be reachable, runs the init script idempotently (or guards with a marker table/check), and make the agent* services depend_on that init service (or have them wait-for-db readiness) so initialization cannot be skipped; reference the existing init script name init.sh, the /docker-entrypoint-initdb.d mount, the postgres_data volume, and the agent* services when making the change.agent/app/services/services.py-82-98 (1)
82-98:⚠️ Potential issue | 🟠 MajorWrap
updateanddeletewith the same service-level exception handling.These two methods still call into repository commits without
@handle_exceptions, soIntegrityErrorand similar failures can leak past the service layer whilecreate/get/get_allare translated consistently.Possible fix
+ `@handle_exceptions` def update( self, db_session: DbSession, object_id: UUID | int, updater: UpdateSchemaType, @@ + `@handle_exceptions` def delete(self, db_session: DbSession, object_id: UUID | int, raise_404: bool = False) -> ModelType | None: if originator := self.get(db_session, object_id, print_log=False, raise_404=raise_404): deleted = self.crud.delete(db_session, originator) self.logger.info(f"Deleted {self.name} with ID: {deleted.id}.") return deletedBased on learnings, "Use handle_exceptions decorator from app.utils.exceptions in service methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/services.py` around lines 82 - 98, The service methods update and delete call into the repository and can raise DB exceptions directly; wrap both Service.update and Service.delete with the same service-level exception handling used elsewhere by applying the handle_exceptions decorator from app.utils.exceptions (or adding the same try/except translation logic) so IntegrityError and similar database errors are caught and translated consistently; specifically, add handle_exceptions to the update and delete method definitions (or implement identical exception handling inside those methods) to mirror how create/get/get_all are protected.agent/app/repositories/chat_session.py-39-42 (1)
39-42:⚠️ Potential issue | 🟠 MajorMake the counter increment atomic.
This read-modify-write loses updates when two requests hit the same session concurrently. Do the increment in SQL so both workers can't commit the same next value.
Possible fix
async def increment_request_count(self, db: AsyncSession, obj: ChatSession) -> None: - obj.request_count += 1 - db.add(obj) - await db.commit() + await db.execute( + update(ChatSession) + .where(ChatSession.id == obj.id) + .values(request_count=ChatSession.request_count + 1) + ) + await db.commit() + await db.refresh(obj)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/repositories/chat_session.py` around lines 39 - 42, The increment_request_count function currently does a Python read-modify-write on obj.request_count which races; change it to perform an atomic SQL UPDATE so the DB increments the counter (e.g. execute an UPDATE on the ChatSession table setting request_count = request_count + 1 WHERE id = :id), commit the session, and then refresh or re-query the ChatSession row if you need the new value back. Target the increment_request_count method, use the AsyncSession to execute the UPDATE statement against ChatSession (filtering on obj.id) rather than incrementing obj.request_count in Python, and ensure you commit after the execute so concurrent workers cannot overwrite each other.agent/app/database.py-56-59 (1)
56-59: 🛠️ Refactor suggestion | 🟠 MajorRemove
str: Textfromtype_annotation_map— it remaps a natively handled type.SQLAlchemy maps
strnatively toVARCHAR, and this repo reservestype_annotation_mapfor types requiring custom SQL mappings (e.g.,UUID,datetimewith timezone). The global remap toTextwill convert all plainMapped[str]fields to unboundedTEXTcolumns, contradicting the project convention of expressing string constraints intentionally viaapp.mappingstypes.Existing code (
agent/app/models/message.py:32) already explicitly specifiesTextfor its string column, confirming the global mapping is redundant and unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/database.py` around lines 56 - 59, Remove the global remapping of Python str to SQLAlchemy Text in the type_annotation_map dict (the entry "str: Text") so that native str types are handled by SQLAlchemy's default VARCHAR semantics; leave custom mappings like UUID: SQL_UUID intact in the type_annotation_map and rely on explicit use of Text in models such as the Text-typed field in agent.app.models.message (so only classes needing custom SQL types remain in type_annotation_map).agent/app/utils/exceptions.py-49-55 (1)
49-55:⚠️ Potential issue | 🟠 MajorUse
exc.errors()instead of unpackingRequestValidationError.args[0][0]directly.The
ctxfield is optional in validation errors and may not always be present, causingKeyErrorwhen accessingerr_args['ctx']['error']. Use the documentederrors()method with guards for missing fields.Possible fix
`@handle_exception.register` def _(exc: RequestValidationError, _: str) -> HTTPException: - err_args = exc.args[0][0] + errors = exc.errors() + first_error = errors[0] if errors else {} + detail = first_error.get("msg", "Validation error") + ctx_error = first_error.get("ctx", {}).get("error") + if ctx_error: + detail = f"{detail} - {ctx_error}" return HTTPException( status_code=400, - detail=f"{err_args['msg']} - {err_args['ctx']['error']}", + detail=detail, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 49 - 55, The RequestValidationError handler (the `@handle_exception.register` function for RequestValidationError) currently unpacks exc.args[0][0] and assumes err_args['ctx']['error'] exists; change it to call exc.errors() and extract the first error dict safely (e.g., errors = exc.errors(); err = errors[0] if errors else {}), then read err.get('msg') and guard access to ctx via err.get('ctx', {}) and ctx.get('error') so you never KeyError — compose the HTTPException detail using the msg and the optional ctx error only when present.agent/app/utils/exceptions.py-58-67 (1)
58-67:⚠️ Potential issue | 🟠 MajorAdd async support to
handle_exceptionsdecorator.The decorator currently only wraps synchronous methods. When applied to
async defmethods, it returns a coroutine immediately without awaiting it, causing exceptions to bypass the exception handler entirely. This will become a bug once async service methods are decorated with@handle_exceptions. The codebase already contains async methods (e.g.,agent/app/services/chat_session.py,backend/app/services/api_key_service.py) and async patterns are required per coding guidelines.Use
inspect.iscoroutinefunction()to detect async functions and provide separate sync and async wrappers—the async wrapper mustawaitthe function call within the try block.Proposed fix
+from inspect import iscoroutinefunction from collections.abc import Callable from functools import singledispatch, wraps from typing import TYPE_CHECKING @@ def handle_exceptions[**P, T, Service: AppService](func: Callable[P, T]) -> Callable[P, T]: - `@wraps`(func) - def async_wrapper(instance: Service, *args: P.args, **kwargs: P.kwargs) -> T: - try: - return func(instance, *args, **kwargs) - except Exception as exc: - entity_name = getattr(instance, "name", "unknown") - raise handle_exception(exc, entity_name) from exc - - return async_wrapper + if iscoroutinefunction(func): + `@wraps`(func) + async def async_wrapper(instance: Service, *args: P.args, **kwargs: P.kwargs) -> T: + try: + return await func(instance, *args, **kwargs) + except Exception as exc: + entity_name = getattr(instance, "name", "unknown") + raise handle_exception(exc, entity_name) from exc + + return async_wrapper + + `@wraps`(func) + def sync_wrapper(instance: Service, *args: P.args, **kwargs: P.kwargs) -> T: + try: + return func(instance, *args, **kwargs) + except Exception as exc: + entity_name = getattr(instance, "name", "unknown") + raise handle_exception(exc, entity_name) from exc + + return sync_wrapper🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 58 - 67, The handle_exceptions decorator currently treats all targets as sync and thus doesn't await coroutine functions; update handle_exceptions to detect async targets using inspect.iscoroutinefunction(func) and return two wrappers: a sync_wrapper (existing behavior) and an async_wrapper that awaits func inside the try/except and raises handle_exception(exc, entity_name) from exc; keep the same instance param typing (Service) and ensure both wrappers preserve wraps(func) so decorated async methods (e.g., in chat_session.py and api_key_service.py) have their exceptions caught and re-raised via handle_exception.agent/app/utils/config_utils.py-71-82 (1)
71-82:⚠️ Potential issue | 🟠 MajorThis decorator is a no-op right now.
wrapper()only callsfuncand returns its result, so nothing is ever written toos.environ. Any caller relying on the decorator name/docstring to bootstrap env-backed settings will still see missing values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/config_utils.py` around lines 71 - 82, The decorator set_env_from_settings currently just returns the wrapped function's result; modify wrapper to call func to get settings, iterate over settings attributes (e.g., using vars(settings) or settings.__dict__), and for each UPPERCASE attribute with a non-None value call os.environ[attribute] = str(value) (ensure os is imported); keep returning settings. Update wrapper in set_env_from_settings to perform this environment population so callers relying on UPPERCASE settings (like EXAMPLE_API_KEY) see them in os.environ.agent/app/utils/config_utils.py-64-68 (1)
64-68:⚠️ Potential issue | 🟠 MajorFail fast when the Fernet key env var is missing.
Returning
FakeFernet()here makes decryption silently optional. An encrypted value can pass through unchanged and only fail much later when it's used as a credential or signing secret. Make the plaintext bypass explicit for local/test only, and raise by default.🛠️ Minimal fail-fast change
master_key = os.environ.get(value) - if not master_key: - return FakeFernet() - return Fernet(os.environ[value]) + if not master_key: + raise ValueError(f"Missing Fernet master key env var: {value}") + return Fernet(master_key.encode("utf-8"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/config_utils.py` around lines 64 - 68, The validate method currently returns FakeFernet() when the named Fernet env var is missing, which silently allows plaintext; change validate (in config_utils.py) to fail-fast by raising a clear exception (e.g., ValueError) when os.environ.get(value) is missing by default, and only return FakeFernet() for explicit local/test contexts (e.g., when an env like APP_ENV in ("local","test") or a dedicated ALLOW_PLAINTEXT_DECRYPTION flag is set); keep the success path returning Fernet(os.environ[value]) and include the env var name in the error message to aid debugging.agent/app/utils/config_utils.py-29-31 (1)
29-31:⚠️ Potential issue | 🟠 MajorFix Pydantic v2 schema-hook signature incompatibility.
Both
__get_pydantic_json_schema__methods use the old Pydantic v1 mutation style and will fail during schema generation with Pydantic v2. The v2 contract requires these to be classmethods accepting(core_schema, handler)parameters and returning the modified JSON schema dict, not mutating a parameter and returning None.Line 29-31:
EncryptedFieldmethod is a classmethod but has the wrong signature and doesn't return the schema.Line 56-57:
FernetDecryptorFieldmethod is an instance method (missing@classmethod) with the wrong signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/config_utils.py` around lines 29 - 31, Both __get_pydantic_json_schema__ implementations use the old v1 mutation style; change them to the Pydantic v2 signature and return the schema dict: make EncryptedField.__get_pydantic_json_schema__ a classmethod with signature (cls, core_schema, handler) and have it return a new dict like {"type": "str", "writeOnly": True} (do not mutate an input), and add `@classmethod` to FernetDecryptorField.__get_pydantic_json_schema__ and change its signature to (cls, core_schema, handler) as well, returning the constructed schema dict instead of returning None. Ensure both methods reference their class (cls) and return the modified JSON schema dict per Pydantic v2 contract.
🟡 Minor comments (11)
agent/scripts/healthchecks/db_up_check.py-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorFix typo in healthcheck output message.
Line 15 has
unavaliable; should beunavailablefor clear logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/healthchecks/db_up_check.py` at line 15, The healthcheck print message contains a typo: change the string in the retry/log statement that currently prints "- PostgreSQL unavaliable - waiting" to use the correct spelling "- PostgreSQL unavailable - waiting" (locate the print call in db_up_check.py that emits the PostgreSQL unavailable/waiting message).agent/scripts/cryptography/generate_master_key.py-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorDecode bytes output to plain text for usability.
Line 4 prints the raw bytes object returned by
Fernet.generate_key(), which outputsb'...'—awkward when copying to environment variables or config files. Decode the bytes to a plain string.Suggested fix
if __name__ == "__main__": - print(Fernet.generate_key()) + print(Fernet.generate_key().decode("utf-8"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/cryptography/generate_master_key.py` around lines 3 - 4, The script currently prints the raw bytes from Fernet.generate_key(), producing a b'...' output; change the print to decode the returned bytes to a UTF-8 string (e.g., call .decode('utf-8') on the value returned by Fernet.generate_key()) so the main block (if __name__ == "__main__":) prints a plain string suitable for copying into env vars/configs.agent/scripts/cryptography/decrypt_setting.py-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorMissing argument validation will cause cryptic errors.
If the script is invoked without the required arguments, it will raise an
IndexErrorinstead of a helpful usage message.🛡️ Proposed fix to add argument validation
import sys from cryptography.fernet import Fernet if __name__ == "__main__": + if len(sys.argv) != 3: + print("Usage: python decrypt_setting.py <fernet_key> <encrypted_value>", file=sys.stderr) + sys.exit(1) fernet = Fernet(sys.argv[1].encode("utf-8")) print(fernet.decrypt(sys.argv[2].encode("utf-8")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/scripts/cryptography/decrypt_setting.py` around lines 5 - 7, The script's main block uses sys.argv[1] and sys.argv[2] directly which causes IndexError when arguments are missing; update the __main__ block in decrypt_setting.py to validate argc (or check len(sys.argv)) before constructing Fernet or calling decrypt, and if arguments are missing print a clear usage message (e.g., required key and ciphertext) and exit with a non-zero status; keep references to Fernet and the decrypt call so the behavior remains the same when valid args are provided.agent/app/utils/conversion.py-8-19 (1)
8-19:⚠️ Potential issue | 🟡 MinorReturn type annotation is inaccurate.
The return type
dict[str, str | None]only accounts for strings, but SQLAlchemy columns can contain integers, floats, booleans, UUIDs, etc. Onlydatetimevalues are converted to strings.🔧 Proposed fix
-def base_to_dict(instance: BaseDbModel) -> dict[str, str | None]: - """Function to convert SQLALchemy Base model into dict.""" +from typing import Any + +def base_to_dict(instance: BaseDbModel) -> dict[str, Any]: + """Function to convert SQLAlchemy Base model into dict.""" b2d = {} for column in inspect(instance).mapper.column_attrs: value = getattr(instance, column.key) - if isinstance(value, (datetime)): + if isinstance(value, datetime): value = value.isoformat() b2d[column.key] = value return b2d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/conversion.py` around lines 8 - 19, The return type for base_to_dict is too narrow — it claims values are only str or None while SQLAlchemy columns can be int, float, bool, UUID, etc.; update the annotation to reflect arbitrary column types (e.g., dict[str, Any] or dict[str, object | None]) and import Any from typing, keeping the function behavior (only converting datetime to isoformat) intact; reference the base_to_dict function, the instance parameter of type BaseDbModel, and the inspect(instance).mapper.column_attrs loop when making the change.agent/README.md-39-40 (1)
39-40:⚠️ Potential issue | 🟡 MinorMissing instructions for starting PostgreSQL locally.
Line 39 mentions "Start PostgreSQL locally" but provides no actual command or guidance. Consider adding instructions (e.g., Docker command or reference to a local Postgres setup).
📝 Suggested addition
# Start PostgreSQL locally +docker run -d --name postgres-local -e POSTGRES_PASSWORD=postgres -p 5432:5432 postgres:16🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/README.md` around lines 39 - 40, The README heading "Start PostgreSQL locally" lacks concrete steps; add a short, copy-pasteable set of instructions under that heading showing how to run Postgres via Docker (e.g., docker run -e POSTGRES_USER=dev -e POSTGRES_PASSWORD=dev -e POSTGRES_DB=app_db -p 5432:5432 -d postgres:15) plus optional psql commands to create a DB/schema and example environment variables (POSTGRES_HOST, POSTGRES_PORT, POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB) and a one-liner to stop/remove the container (docker stop <name> && docker rm <name>) so contributors can start/stop Postgres locally; reference the README heading "Start PostgreSQL locally" when inserting these instructions.agent/app/utils/api_utils.py-20-23 (1)
20-23:⚠️ Potential issue | 🟡 MinorPotential
KeyErrorifpageorlimitnot in kwargs.Direct dictionary access with
kwargs["page"]andkwargs["limit"]will raiseKeyErrorif the decorated route doesn't define these parameters. Consider using.get()with defaults or adding explicit validation.🛡️ Suggested defensive fix
- if type(result) is list: - page = kwargs["page"] - limit = kwargs["limit"] + if isinstance(result, list): + page = kwargs.get("page", 1) + limit = kwargs.get("limit", 10) formatted = get_hateoas_list(result, page, limit, base_url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/api_utils.py` around lines 20 - 23, The code path that formats list results (the if type(result) is list block) directly indexes kwargs["page"] and kwargs["limit"], which can raise KeyError; update this block to safely read pagination by using kwargs.get("page", <default>) and kwargs.get("limit", <default>) or perform explicit validation before calling get_hateoas_list(result, page, limit, base_url) so page/limit are always defined (reference the variables result, kwargs and the call to get_hateoas_list).agent/app/config.py-57-66 (1)
57-66:⚠️ Potential issue | 🟡 MinorInconsistent type annotation in
cors_originsvalidator.The return type
list[str] | strdoesn't align with the field typelist[AnyHttpUrl]. Also, withmode="after", the valuevshould already be alist[AnyHttpUrl](not a raw string), making the string-splitting logic unreachable in practice.🔧 Proposed fix using mode="before"
- `@field_validator`("cors_origins", mode="after") + `@field_validator`("cors_origins", mode="before") `@classmethod` - def assemble_cors_origins(cls, v: str | list[str]) -> list[str] | str: + def assemble_cors_origins(cls, v: str | list[str] | None) -> list[str]: if isinstance(v, str) and not v.startswith("["): return [i.strip() for i in v.split(",")] - if isinstance(v, (list, str)): + if isinstance(v, list): return v - - # This should never be reached given the type annotation, but ensures type safety - raise ValueError(f"Unexpected type for cors_origins: {type(v)}") + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/config.py` around lines 57 - 66, The validator assemble_cors_origins for the cors_origins field has mismatched types and uses mode="after" so string-handling is unreachable; change the field_validator to mode="before" and update the method signature to accept v: str | list[str] and return list[AnyHttpUrl] (or the configured list type) so it parses comma-separated strings into a list and returns the correctly-typed list for Pydantic to validate; ensure you keep the branches for str (split/strip into list) and for list (return as-is), and remove the unreachable error branch or adapt it to validate unexpected types with a ValueError.agent/app/utils/auth.py-59-59 (1)
59-59:⚠️ Potential issue | 🟡 MinorHandle invalid UUID format in
subclaim.
UUID(user_id)raisesValueErrorif thesubclaim contains a malformed UUID string. This would result in an unhandled 500 error instead of a proper 401 response.🛡️ Proposed fix
async def get_user_id(self, token: _TokenDep) -> UUID: payload = self._decode(token) user_id: str | None = payload.get("sub") if user_id is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, ) - return UUID(user_id) + try: + return UUID(user_id) + except ValueError: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid user identifier in token", + headers={"WWW-Authenticate": "Bearer"}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/auth.py` at line 59, The call that converts the JWT "sub" into a UUID (return UUID(user_id)) can raise ValueError for malformed values; update the code to wrap that conversion in a try/except that catches ValueError, and on error return the proper authentication failure (e.g., raise the same 401/Unauthorized exception your auth flow uses or return None per surrounding convention) and log/record the invalid claim. Locate the UUID(user_id) conversion in the function handling the JWT/sub extraction (the line with UUID and user_id) and implement the ValueError handling so malformed "sub" claims produce a 401 rather than an unhandled 500.agent/app/utils/hateoas.py-71-82 (1)
71-82:⚠️ Potential issue | 🟡 MinorHandle empty
itemslist gracefully.When
itemsis empty,namebecomes""andbuilt_urlwill be malformed (e.g.,/api/v1/s/). Consider returning early or providing a sensible default for empty collections.🛡️ Proposed fix
def get_hateoas_list( items: list[BaseDbModel], page: int, limit: int, base_url: str, ) -> dict[str, list[dict[str, str]] | list[dict[str, str | None]]]: + if not items: + return { + "items": [], + "_links": _generate_collection_links(page, limit, base_url), + } name = items[0].__tablename__ if len(items) else "" built_url = _build_query(base_url, name) return { "items": [base_to_dict(item) for item in items], "_links": _generate_collection_links(page, limit, built_url), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/hateoas.py` around lines 71 - 82, get_hateoas_list currently builds a query using items[0].__tablename__, which breaks when items is empty; update get_hateoas_list to handle empty lists by checking if not items, set built_url to base_url.rstrip('/') (or otherwise the base_url without adding a collection segment) and return {"items": [], "_links": _generate_collection_links(page, limit, built_url)} (avoiding calling items[0] or passing an empty name to _build_query); keep the existing behavior for non-empty lists (use name = items[0].__tablename__, built_url = _build_query(base_url, name), and items = [base_to_dict(item) for item in items]).agent/app/repositories/repositories.py-18-19 (1)
18-19:⚠️ Potential issue | 🟡 MinorAdd return annotations to both constructors.
__init__is missing-> Nonein both repository classes, which breaks the repo-wide Python typing rule.Possible fix
- def __init__(self, model: type[ModelType]): + def __init__(self, model: type[ModelType]) -> None: self.model = model @@ - def __init__(self, model: type[ModelType]): + def __init__(self, model: type[ModelType]) -> None: self.model = modelAs per coding guidelines, "All Python functions must have type annotations".
Also applies to: 76-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/repositories/repositories.py` around lines 18 - 19, Both repository classes' constructors (__init__) are missing an explicit return annotation; add "-> None" to each __init__ signature (e.g., def __init__(self, model: type[ModelType]) -> None:) so they comply with the repo-wide Python typing rule. Locate the two __init__ methods in repositories.py (the one that accepts model: type[ModelType] and the other at the later repository class) and update their signatures only to include "-> None" while keeping existing parameter and attribute assignments unchanged; run the type checker after the change.agent/app/utils/config_utils.py-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorAdd the missing function annotations.
EncryptedField.__init__is missing-> None, andwrapperleaves*args/**kwargsuntyped. This file does not satisfy the repo's Python typing rule until both are annotated.As per coding guidelines, "All Python functions must have type annotations".
Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/config_utils.py` at line 43, EncryptedField.__init__ is missing a return type annotation and the inner wrapper function lacks types for its variadic parameters; update EncryptedField.__init__(self, value: str) to include "-> None" and annotate the wrapper's signature to type *args and **kwargs (e.g., *args: Any, **kwargs: Any) and its return type (e.g., -> Any) so both functions satisfy the repo typing rule; ensure you import Any from typing if not already present and apply the same pattern to the other occurrence referenced (line ~79) for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d51f447a-8940-423e-8e9e-3ca39ef2b867
⛔ Files ignored due to path filters (1)
agent/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.pre-commit-config.yamlMakefileagent/.python-versionagent/Dockerfileagent/README.mdagent/alembic.iniagent/app/__init__.pyagent/app/api/__init__.pyagent/app/api/routes/v1/__init__.pyagent/app/api/routes/v1/chat.pyagent/app/api/routes/v1/session.pyagent/app/config.pyagent/app/database.pyagent/app/integrations/celery/__init__.pyagent/app/integrations/celery/core.pyagent/app/integrations/celery/tasks/__init__.pyagent/app/integrations/celery/tasks/dummy_task.pyagent/app/integrations/celery/tasks/expire_sessions.pyagent/app/integrations/celery/tasks/process_message.pyagent/app/integrations/sentry.pyagent/app/main.pyagent/app/mappings.pyagent/app/middlewares.pyagent/app/models/__init__.pyagent/app/models/chat_session.pyagent/app/models/conversation.pyagent/app/models/message.pyagent/app/repositories/__init__.pyagent/app/repositories/chat_session.pyagent/app/repositories/conversation.pyagent/app/repositories/message.pyagent/app/repositories/repositories.pyagent/app/schemas.pyagent/app/schemas/__init__.pyagent/app/schemas/agent.pyagent/app/schemas/chat.pyagent/app/schemas/common.pyagent/app/schemas/error_code.pyagent/app/schemas/message.pyagent/app/schemas/session.pyagent/app/services/__init__.pyagent/app/services/chat_session.pyagent/app/services/services.pyagent/app/utils/api_utils.pyagent/app/utils/auth.pyagent/app/utils/config_utils.pyagent/app/utils/conversion.pyagent/app/utils/exceptions.pyagent/app/utils/hateoas.pyagent/app/utils/healthcheck.pyagent/app/utils/mappings_meta.pyagent/config/.env.exampleagent/migrations/READMEagent/migrations/env.pyagent/migrations/script.py.makoagent/pyproject.tomlagent/scripts/cryptography/decrypt_setting.pyagent/scripts/cryptography/encrypt_setting.pyagent/scripts/cryptography/generate_master_key.pyagent/scripts/healthchecks/db_up_check.pyagent/scripts/postgres/init.shagent/scripts/start/app.shagent/scripts/start/beat.shagent/scripts/start/flower.shagent/scripts/start/worker.shdocker-compose.yml
Comment on lines
+15
to
+17
| def validate_filters(cls, v: dict[str, str]) -> dict[str, str]: | ||
| """Remove empty or whitespace-only filters.""" | ||
| return {k: v for k, v in v.items() if v and v.strip()} |
There was a problem hiding this comment.
Variable shadowing bug: loop variable shadows parameter.
The comprehension {k: v for k, v in v.items() ...} shadows the parameter v with the loop variable. After the first iteration, v refers to the current value, not the original dict, causing unpredictable behavior.
Note: agent/app/schemas/common.py has the correct implementation using value as the loop variable.
🐛 Proposed fix
`@field_validator`("filters")
`@classmethod`
def validate_filters(cls, v: dict[str, str]) -> dict[str, str]:
"""Remove empty or whitespace-only filters."""
- return {k: v for k, v in v.items() if v and v.strip()}
+ return {k: value for k, value in v.items() if value and value.strip()}📝 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
| def validate_filters(cls, v: dict[str, str]) -> dict[str, str]: | |
| """Remove empty or whitespace-only filters.""" | |
| return {k: v for k, v in v.items() if v and v.strip()} | |
| def validate_filters(cls, v: dict[str, str]) -> dict[str, str]: | |
| """Remove empty or whitespace-only filters.""" | |
| return {k: value for k, value in v.items() if value and value.strip()} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/schemas.py` around lines 15 - 17, The validate_filters method
currently shadows its parameter v by using v as the loop variable in the dict
comprehension; change the comprehension to use a different loop variable (e.g.,
value) so the parameter isn’t overwritten: in validate_filters(cls, v: dict[str,
str]) return {k: value for k, value in v.items() if value and value.strip()} and
ensure the parameter name v remains the dict being iterated. This mirrors the
correct pattern used in agent/app/schemas/common.py.
Comment on lines
+105
to
+113
| for my_attr, (my_type, target_name) in local_rels.items(): | ||
| target_rels = cls._registry.get(target_name, {}) | ||
| for tgt_attr, (tgt_type, tgt_target) in target_rels.items(): | ||
| if tgt_target == mapped_cls.__name__ and tgt_type != my_type: | ||
| setattr( | ||
| mapped_cls, | ||
| my_attr, | ||
| relationship(target_name, back_populates=tgt_attr), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=py 'Mapped\[\s*(OneToMany|ManyToOne)\[' agent/appRepository: the-momentum/open-wearables
Length of output: 53
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and get its size
stat agent/app/utils/mappings_meta.py 2>/dev/null || echo "File not found"
wc -l agent/app/utils/mappings_meta.py 2>/dev/null || echo "Unable to count lines"Repository: the-momentum/open-wearables
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review
head -n 120 agent/app/utils/mappings_meta.py | tail -n 30 | cat -nRepository: the-momentum/open-wearables
Length of output: 1478
🏁 Script executed:
#!/bin/bash
# Search for relationship definitions with different patterns
rg -n "relationship\(" agent/app/utils/mappings_meta.py -A 2 -B 2Repository: the-momentum/open-wearables
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Search for any relation-like annotations or type hints in the agent directory
rg -n "Mapped\[" agent/app/ --type=py | head -20Repository: the-momentum/open-wearables
Length of output: 1871
🏁 Script executed:
#!/bin/bash
# Check function signatures in mappings_meta.py for type annotations
rg -n "def " agent/app/utils/mappings_meta.py -A 1Repository: the-momentum/open-wearables
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Read the entire mappings_meta.py file
cat -n agent/app/utils/mappings_meta.pyRepository: the-momentum/open-wearables
Length of output: 4416
Replace the relationship at line 112 to preserve options from line 93.
Recreating the relationship in _handle_back_populates discards the options set at line 93. For OneToMany relations, this loses cascade="all, delete-orphan" and passive_deletes=True from DEFAULT_ONE_TO_MANY. Instead of replacing the entire relationship, update only the back_populates parameter on the existing relationship object:
Example fix
for my_attr, (my_type, target_name) in local_rels.items():
target_rels = cls._registry.get(target_name, {})
for tgt_attr, (tgt_type, tgt_target) in target_rels.items():
if tgt_target == mapped_cls.__name__ and tgt_type != my_type:
existing_rel = getattr(mapped_cls, my_attr)
existing_rel.back_populates = tgt_attr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/utils/mappings_meta.py` around lines 105 - 113, The
_handle_back_populates loop currently recreates relationships (using
relationship(...)) which discards options like DEFAULT_ONE_TO_MANY (e.g.,
cascade="all, delete-orphan", passive_deletes=True); instead, locate the
existing relationship object on mapped_cls (use getattr(mapped_cls, my_attr))
and set its back_populates attribute to the target attribute (tgt_attr) rather
than calling relationship(...) again; update the code in
mappings_meta._handle_back_populates to modify existing_rel.back_populates so
relation options are preserved.
Comment on lines
+11
to
+12
| "passlib>=1.7.4", | ||
| "python-jose[cryptography]>=3.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for security advisories on these packages
gh api graphql -f query='
{
passlib: securityVulnerabilities(first: 5, ecosystem: PIP, package: "passlib") {
nodes { advisory { summary severity } vulnerableVersionRange }
}
jose: securityVulnerabilities(first: 5, ecosystem: PIP, package: "python-jose") {
nodes { advisory { summary severity } vulnerableVersionRange }
}
}'Repository: the-momentum/open-wearables
Length of output: 231
🌐 Web query:
passlib maintenance status last update 2024 2025
💡 Result:
Passlib is unmaintained. The official PyPI package (passlib) last released v1.7.4 on 2020-10-08, with no updates since. The original repository at foss.heptapod.net/python-libs/passlib shows last commits in October 2020. It has known compatibility issues with bcrypt >=4.0/5.0, argon2, and Python 3.13's crypt removal, causing warnings/errors in 2024-2025. A maintained fork exists at https://github.com/notypecheck/passlib (last push 2025-10-27, latest release 1.9.3 on 2025-10-09), published as libpass on PyPI (https://pypi.org/project/libpass/), with active development addressing the issues. Distributions like Slackware, OpenBSD, and others have switched to it. FastAPI docs switched to pwdlib (another alternative). A PEP 541 ownership transfer request for passlib was filed Feb 2026 (pypi/support#9290, open as of Mar 2026), but unresolved as of 2026-04-03. Users should migrate to libpass or alternatives like pwdlib.
Citations:
- 1: https://pypi.org/project/passlib/
- 2: https://passlib.readthedocs.io/en/stable/history/
- 3: https://github.com/notypecheck/passlib
- 4: https://pypi.org/project/libpass/
- 5: PEP 541 Request: passlib pypi/support#9290
- 6: passlib takeover? notypecheck/passlib#11
- 7: passlib seems not being maintenanced anymore. However FastAPI's docs still using. Consider change it. fastapi/fastapi#11773
- 8: Consider the future of
passliband password hashing/upgrading pypi/warehouse#15454 - 9: https://pypi.python.org/pypi/passlib
- 10: https://pypistats.org/packages/passlib
- 11: https://repology.org/project/python%3Apasslib/versions
🌐 Web query:
python-jose security vulnerabilities maintenance status 2024 2025
💡 Result:
python-jose has known security vulnerabilities and recent maintenance activity. Security Vulnerabilities (2024-2025): - CVE-2024-33663: Algorithm confusion with OpenSSH ECDSA keys (affects <=3.3.0, fixed in 3.4.0). CVSS 6.5 Medium. {2,3,6,12,18} - CVE-2024-33664: DoS via high compression ratio JWE ("JWT bomb", affects <=3.3.0, fixed in 3.4.0). CVSS 5.3 Medium. {3,12,15} - CVE-2024-29370: DoS via compressed JWE content in jwe.decrypt (affects 3.3.0). CVSS 5.3 Medium. {1,3,17} Maintenance Status (2024-2025): - Releases: 3.4.0 (2025-02-14, fixes CVE-2024-33663/33664, drops Python 3.6/3.7, adds 3.10/3.11), 3.5.0 (2025-05-28, drops Python 3.8, adds 3.12/3.13). Last push: 2026-03-28. {9,12} - Despite releases addressing CVEs, community concerns persist about maintenance (open issues/PRs like #340, #349; suggestions to migrate to authlib/joserfc/PyJWT). {6,7,8,19} - Recommend upgrading to >=3.5.0 and monitoring for further updates or considering alternatives like PyJWT, joserfc (actively maintained, recent releases up to 1.6.3 in 2026), or Authlib. {9,13,14}
Citations:
- 1: GHSA-h4pw-wxh7-4vjj
- 2: https://nvd.nist.gov/vuln/detail/cve-2024-33663
- 3: https://app.opencve.io/cve/?product=python-jose&vendor=python-jose_project
- 4: Fix CVE-2024-33663 mpdavis/python-jose#349
- 5: python-jose library is unmaintained GNS3/gns3-server#2466
- 6: Dependency python-Jose appears to be unmaintained okta/okta-jwt-verifier-python#54
- 7: https://github.com/mpdavis/python-jose/
- 8: https://github.com/mpdavis/python-jose/blob/master/CHANGELOG.md
- 9: https://jose.authlib.org/en/changelog/
- 10: https://pypi.org/project/joserfc/
- 11: https://pypi.org/project/python-jose/2.0.0/
- 12: https://cvefeed.io/vuln/detail/CVE-2024-29370
- 13: https://github.com/pypa/advisory-database/blob/main/vulns/python-jose/PYSEC-2024-232.yaml
- 14: Is python-jose still supported? mpdavis/python-jose#340
🌐 Web query:
argon2-cffi bcrypt PyJWT joserfc actively maintained alternatives
💡 Result:
All four libraries—argon2-cffi, bcrypt, PyJWT, and joserfc—are actively maintained as of 2026-04-03. argon2-cffi: Latest release 25.1.0 on 2025-06-03, last GitHub push 2026-03-03, supports Python 3.13/3.14. Recommended by OWASP as gold standard for password hashing. bcrypt: Latest release 5.0.0 on 2025-09-25, last GitHub push 2026-04-03 (today), supports Python 3.14 including free-threaded. Notes prefer argon2id or scrypt for new projects. PyJWT: Latest release 2.12.1 on 2026-03-13, healthy maintenance with recent commits and no known vulnerabilities. joserfc: Latest release 1.6.3 on 2026-02-25, multiple 2026 releases, high download counts (~34M/month), comprehensive JOSE implementation from Authlib project. No actively maintained alternatives needed, but for password hashing: OWASP prefers argon2id (argon2-cffi) over bcrypt; scrypt or PBKDF2 as fallbacks. For JWT/JOSE: python-jose appears less active (open issues, no recent releases mentioned), so joserfc or PyJWT preferred.
Citations:
- 1: https://github.com/hynek/argon2-cffi/releases
- 2: https://github.com/hynek/argon2-cffi
- 3: https://pypi.org/project/argon2-cffi/
- 4: https://github.com/pyca/bcrypt/
- 5: https://pypi.org/project/bcrypt/
- 6: https://github.com/jpadilla/pyjwt/releases/tag/2.11.0
- 7: https://github.com/jpadilla/pyjwt/releases
- 8: https://pypi.org/project/PyJWT/
- 9: https://pypi.org/project/joserfc/
- 10: https://togithub.com/authlib/joserfc
Upgrade dependencies to address unmaintained code and security vulnerabilities.
-
passlib(line 11): Unmaintained since 2020 and incompatible with Python 3.13 (which this project targets). Migrate toargon2-cffi(OWASP recommended) orbcrypt. -
python-jose(line 12): Currently allows versions ≤3.3.0, which contain CVE-2024-33663 and CVE-2024-33664 (algorithm confusion, JWE DoS). Upgrade constraint topython-jose[cryptography]>=3.5.0to fix these vulnerabilities and add Python 3.13 support. Alternatively, considerjoserfcorPyJWTfor more recent active maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/pyproject.toml` around lines 11 - 12, Replace the unmaintained
"passlib>=1.7.4" dependency with a supported password hashing library (e.g., add
"argon2-cffi" or "bcrypt" and migrate any usages of passlib APIs in
authentication code to the chosen library's APIs) and update the
"python-jose[cryptography]>=3.3.0" constraint to
"python-jose[cryptography]>=3.5.0" to mitigate CVE-2024-33663/33664 and add
Python 3.13 support; after updating the pyproject.toml entries for "passlib" and
"python-jose[cryptography]" adjust any import/usages referencing passlib, run
tests, and refresh your lockfile (or poetry/pip-compile) to ensure dependency
resolution.
Invert the Conversation/Session ownership hierarchy to match the spec: Conversation is now the top-level entity (with user_id + status enum), Session has a FK to Conversation (many sessions per conversation), and Message gains dual ownership via both conversation_id (primary) and session_id (secondary, SET NULL on session delete). Introduce ConversationStatus enum (active/inactive/closed) and rename ChatSession model/repository to Session. Replace ChatSessionService with ConversationService which owns upsert, deactivate_session, get_active, save_messages, touch, and build_history (stub — wired in later commits). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Create the initial database schema for conversations, sessions, and messages tables. Conversations are top-level (user_id + status enum), sessions belong to conversations, and messages have dual ownership via conversation_id (CASCADE) and session_id (SET NULL). PostgreSQL native ENUMs are created for conversationstatus and messagerole. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add pygentic-ai (git dependency), anthropic, openai, google-generativeai, and pydantic-ai to project dependencies. Extend settings with LLM provider config (LLM_PROVIDER, LLM_MODEL, LLM_MODEL_WORKERS, per-provider API keys), OW backend integration (OW_API_URL, OW_API_KEY), and conversation lifecycle params (SESSION_TIMEOUT_MINUTES, CONVERSATION_CLOSE_HOURS, HISTORY_SUMMARY_THRESHOLD, MAX_RETRIES). Add model_utils.get_llm() factory that returns the (vendor, model, api_key) tuple based on the configured provider, with startup validation via validate_llm_config() called in main.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add DataPointSeriesRepository.get_daily_vitals_aggregates() returning
per-day averages of resting HR, HRV SDNN, and SpO2 grouped by source/device
- Implement SummariesService.get_recovery_summaries() merging sleep
efficiency/duration with vitals into PaginatedResponse[RecoverySummary]
- Wire GET /users/{id}/summaries/recovery route to service (was HTTP 501)
- Extend UserRead schema with birth_date, sex, gender sourced from
personal_record via a model_validator that flattens the relationship
Singleton OWClient using httpx.AsyncClient (one per call) with X-API-Key auth. Covers: user profile, body summary, timeseries, activity/sleep/ recovery summaries, workout and sleep events.
Toolsets (tools/): - ow_tools.py: 8 async tool functions covering user profile, body composition, activity, sleep, recovery, workouts, sleep events, HR timeseries - date_tools.py: get_today_date, get_current_week utilities - tool_registry.py: ToolManager mapping AgentMode to Toolpack lists Prompts (prompts/): - agent_prompts.py: GENERAL_SYSTEM_PROMPT built from composable text blocks (priming + ReAct guidance + health ruleset + data capabilities) - worker_prompts.py: router and guardrails system prompts Engines (engines/): - reasoning.py: builds a pydantic-ai Agent with mode-specific prompt and tools - router.py: lightweight classifier returning RouterDecision(route, reasoning) - guardrails.py: formatting pass using the worker model Workflow (workflows/agent_workflow.py): - WorkflowEngine.run(): ROUTER → REASONING → GUARDRAILS pipeline - WorkflowEngine.summarize(): history compression using worker model - workflow_engine singleton used by Celery task Static (static/): - default_msgs.py: fallback error and refusal message strings
Replaces the stub echo with the full pipeline: 1. Load conversation + session from DB 2. build_history() for LLM context (with summarization if over threshold) 3. workflow_engine.run() → router → reasoning → guardrails 4. save_messages() to persist user + assistant turns and touch conversation 5. POST result to callback_url; log warning on HTTP failure (no retry) Falls back to WORKFLOW_ERROR_MSG on any unhandled workflow exception.
Replaces expire-sessions beat task with manage-conversation-lifecycle running every 5 minutes: 1. Deactivate sessions idle > SESSION_TIMEOUT_MINUTES 2. Mark ACTIVE conversations as INACTIVE when updated_at is stale 3. Close INACTIVE conversations idle > CONVERSATION_CLOSE_HOURS All thresholds read from settings so they are configurable via env vars.
Structure mirrors the backend test suite (testcontainers + factory-boy,
per-test savepoint rollback, autouse mocks for LLM/Celery/OW client):
tests/conftest.py
- PostgreSQL via testcontainers (or TEST_DATABASE_URL env var)
- Async session with savepoint rollback per test
- FastAPI TestClient with async DB dependency override
- JWT auth token fixture
- Autouse mocks: Celery tasks, pydantic-ai Agent, OW backend client
tests/factories.py
- ConversationFactory, SessionFactory, MessageFactory
tests/repositories/
- test_conversation_repository: create, get, update status/summary,
mark_inactive_stale, close_stale
- test_session_repository: create, get, deactivate, increment_request_count,
deactivate_expired
- test_message_repository: create, get_by_conversation_id, limit, isolation
tests/services/
- test_conversation_service: upsert (all paths), get_active (ownership/status
checks), save_messages, build_history (threshold + summary injection)
tests/api/v1/
- test_session: POST /session (create/reuse), PATCH /session/{id} (deactivate)
- test_chat: POST /chat/{id} (queuing, validation, 404/410/401 guards)
tests/agent/
- test_tools: date tools correctness; OW tool calls, error handling, clamping
- test_workflow: full pipeline (answer path, refusal, router failure fallback,
guardrails failure fallback, reasoning propagation, history passing)
- test_tool_registry: mode→toolpack mapping, callability
tests/tasks/
- test_conversation_lifecycle: session expiry, inactive marking, close logic,
no-op on fresh data
pyproject.toml: added factory-boy, testcontainers[postgres] to dev deps;
added [tool.pytest.ini_options] with asyncio_mode=auto.
… and harden provider selection
…o bottom conversation->session->message
…Service - Delete app/services/chat_session.py — dead code left over from the conversation->session->message rename; ConversationService covers all the same surface area - Add explicit None-guard in model_utils.get_llm() so ty narrows effective_model from str|None to str - Cast run_result.output to str in WorkflowEngine.run() to satisfy the declared return type - Suppress library-level false positives (SQLAlchemy Pool methods, CursorResult.rowcount, Celery Proxy, httpx params, declared_attr, generic decorator bounds) with bare # type: ignore - Add # type: ignore on repository create() overrides whose signatures intentionally differ from the generic base - Fix all ruff violations in tests: missing ANN001/ANN201 annotations, SIM117 nested-with, RET504 unnecessary assignments, import ordering - Add [tool.ty.src] exclude = ["tests"] to pyproject.toml — factory-boy metaclass magic is opaque to ty; tests lint cleanly via ruff - Reformat backend files touched by pre-commit ruff format
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (18)
agent/Dockerfile (2)
3-7: Remove unusual indentation from Dockerfile commands.All RUN, COPY, ENV, and other Dockerfile instructions have leading whitespace (4 spaces). Standard Dockerfile syntax does not indent instructions. While Docker tolerates this, it's unconventional and may cause readability or tooling issues.
🧹 Proposed fix to remove indentation
- RUN apt-get update && \ +RUN apt-get update && \ apt-get install -y --no-install-recommends build-essential libpq-dev git && \ apt-get clean && \ rm -rf /var/lib/apt/lists/* - RUN pip install --no-cache-dir --upgrade pyopenssl +RUN pip install --no-cache-dir --upgrade pyopensslApply the same pattern to all instructions throughout the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/Dockerfile` around lines 3 - 7, The Dockerfile contains leading spaces before instructions (e.g., the RUN lines shown) which is unconventional; remove the leading indentation from all Dockerfile instructions (RUN, COPY, ENV, etc.) so each instruction starts at column 0, and apply this consistently across the file (for example update the two RUN lines shown to have no leading spaces and do the same for any other indented Dockerfile directives).
13-14: Optimize layer caching by copying source after dependency installation.Currently, line 13 copies
uv.lockandpyproject.toml, then line 14 immediately copies the entire directory (which includes those files again). This breaks Docker layer caching—any source code change invalidates the dependency installation layer.⚡ Proposed fix to improve caching
Move the full source copy (line 14) to after line 20 (after
uv sync):WORKDIR /root_project COPY uv.lock pyproject.toml ./ - COPY . . ENV VIRTUAL_ENV=/opt/venv ENV UV_PROJECT_ENVIRONMENT=/opt/venv RUN uv venv $VIRTUAL_ENV ENV PATH="$VIRTUAL_ENV/bin:$PATH" RUN uv sync --locked --no-dev + COPY . .This ensures dependency installation is cached unless
uv.lockorpyproject.tomlchange.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/Dockerfile` around lines 13 - 14, The Dockerfile currently copies the whole repo before running dependency sync which defeats layer caching: keep the initial dependency layer by retaining the "COPY uv.lock pyproject.toml ./" step, remove or move the "COPY . ." step so that the full source copy occurs only after the dependency install (after the "uv sync" step); specifically, place the "COPY . ." instruction after the "uv sync" command so changes to application source do not invalidate the cached dependency installation based on uv.lock and pyproject.toml.backend/app/schemas/model_crud/user_management/user.py (1)
63-75: Reduce field-mapping drift risk in_flatten_personal_record.The hardcoded dict duplicates
UserReadfield definitions and can silently diverge as the schema evolves. Consider deriving base fields fromcls.model_fieldsand only overriding personal-record fields explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/model_crud/user_management/user.py` around lines 63 - 75, The _flatten_personal_record function currently builds a hardcoded dict duplicating UserRead fields; change it to derive the base mapping from cls.model_fields (iterate cls.model_fields keys and set each value to getattr(data, key, None)) to avoid drift, then explicitly override the personal-record fields (birth_date, sex, gender) using pr.birth_date/pr.sex/pr.gender if pr exists, leaving other fields untouched; reference the _flatten_personal_record function, cls.model_fields, UserRead, and the pr variable when making this change.agent/app/agent/tools/ow_tools.py (2)
148-148: Movedatetimeandtimezoneimports to module level.The
datetimeandtimezoneimports insideget_heart_rate_timeseriescould be consolidated with the existingdatetimemodule imports at line 10 for consistency.♻️ Proposed fix
At line 10:
-from datetime import date, timedelta +from datetime import date, datetime, timedelta, timezoneAt line 148:
try: - from datetime import datetime, timezone - hours = min(max(1, hours), 168)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/agent/tools/ow_tools.py` at line 148, The function get_heart_rate_timeseries contains a local "from datetime import datetime, timezone" import; move these imports to the module-level imports (consolidate with the existing datetime-related imports at the top of the file) and remove the in-function import so datetime and timezone are imported once for the module and available to get_heart_rate_timeseries and other functions.
165-174: Consider more specific type annotation forOW_TOOLS.The
OW_TOOLS: listannotation is bare. A more specific type would improve IDE support and static analysis.♻️ Proposed fix
+from collections.abc import Awaitable, Callable + -OW_TOOLS: list = [ +OW_TOOLS: list[Callable[..., Awaitable[str]]] = [ get_user_profile, get_body_composition, ... ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/agent/tools/ow_tools.py` around lines 165 - 174, OW_TOOLS is currently annotated with the bare type "list"; update it to a more specific typing such as List[Callable[..., Any]] or Sequence[Callable[..., Any]] (or the concrete Tool type if one exists) to improve IDE/autocomplete and static analysis; update/import the appropriate typing symbols (e.g., from typing import List, Callable, Any or from typing import Sequence) and annotate OW_TOOLS: List[Callable[..., Any]] = [get_user_profile, get_body_composition, get_recent_activity, get_recent_sleep, get_recovery_data, get_workouts, get_sleep_events, get_heart_rate_timeseries] (or replace Callable[..., Any] with the project's Tool type if available).agent/app/repositories/conversation.py (1)
44-51: Consider adding ordering or limit toget_active_by_user_id.If a user could theoretically have multiple active conversations (e.g., due to a race condition),
scalar_one_or_none()would raiseMultipleResultsFound. Consider adding.order_by(Conversation.created_at.desc()).limit(1)or documenting the uniqueness constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/repositories/conversation.py` around lines 44 - 51, get_active_by_user_id can raise MultipleResultsFound if multiple ACTIVE conversations exist; update the select in get_active_by_user_id to pick the most recent one by adding an ordering and limiting to one result (e.g., add .order_by(Conversation.created_at.desc()).limit(1) to the select for Conversation) so the query returns a single row even if duplicates exist; alternatively document or enforce a uniqueness constraint on Conversation.user_id + status if you prefer.agent/app/utils/healthcheck.py (2)
45-71: Blocking SDK calls in async endpoint.The OpenAI, Google, and Anthropic SDK clients used here (
client.models.retrieve(),genai_get_model()) are synchronous and will block the event loop. For a health check endpoint that may be called frequently, consider running these in a thread pool or using async SDK variants if available.♻️ Option: Run in thread pool
+import asyncio +from functools import partial `@healthcheck_router.get`("/llm") async def llm_health() -> dict[str, str]: """LLM provider health check — sends a minimal request to verify the API is reachable.""" try: vendor, model, api_key = get_llm() + loop = asyncio.get_event_loop() match vendor: case "openai": client = openai.OpenAI(api_key=api_key) - client.models.retrieve(model) + await loop.run_in_executor(None, partial(client.models.retrieve, model)) ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/healthcheck.py` around lines 45 - 71, The llm_health async endpoint is calling blocking SDK methods (openai.OpenAI(...).models.retrieve, genai_get_model, anthropic.Anthropic(...).models.retrieve) directly; update llm_health to run these blocking calls off the event loop (e.g., use asyncio.to_thread or loop.run_in_executor) around the calls invoked after get_llm and genai_configure so the health check remains non-blocking, preserve the existing try/except behavior and returned fields, and ensure you call the same unique symbols (openai.OpenAI, client.models.retrieve, genai_configure, genai_get_model, anthropic.Anthropic) inside the threaded task.
38-42: Consider returning 503 for unhealthy status.The endpoint returns HTTP 200 with
{"status": "unhealthy", ...}when checks fail. Load balancers and monitoring tools typically expect 5xx status codes to detect unhealthy instances. Consider raisingHTTPException(status_code=503)or usingResponse(status_code=503).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/healthcheck.py` around lines 38 - 42, The current except block returns a 200 with {"status":"unhealthy",...}; change it to return an actual 503 response so load balancers detect failures: replace the dict return in the except with either raise HTTPException(status_code=503, detail={"status":"unhealthy","error":str(e)}) or return Response(content=json.dumps({"status":"unhealthy","error":str(e)}), media_type="application/json", status_code=503), and add the necessary imports (HTTPException or Response and json) at top of the module.agent/app/integrations/ow_backend/client.py (1)
27-35: Consider adding request timeouts for reliability.The
httpx.AsyncClient()is instantiated without atimeoutparameter. If the OW backend becomes unresponsive, requests could hang indefinitely, potentially exhausting Celery worker resources.♻️ Proposed fix to add timeout
async def get_user_profile(self, user_id: UUID) -> dict: """GET /users/{user_id} — basic profile (name, email, birth_date, sex, gender).""" - async with httpx.AsyncClient() as client: + async with httpx.AsyncClient(timeout=30.0) as client: resp = await client.get( f"{self._base()}/api/v1/users/{user_id}", headers=self._headers(), ) resp.raise_for_status() return resp.json()Consider defining a class-level constant
_TIMEOUT = 30.0and applying it consistently across all methods, or passing it to__init__for configurability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/ow_backend/client.py` around lines 27 - 35, The get_user_profile method opens an httpx.AsyncClient without a timeout, risking hung requests; add a consistent timeout by defining a class-level constant (e.g. _TIMEOUT = 30.0) on the client class and pass it into httpx.AsyncClient(timeout=self._TIMEOUT) (or accept timeout via __init__ and store it) so that get_user_profile (and other HTTP methods) use that timeout to avoid indefinite blocking.agent/app/integrations/celery/tasks/conversation_lifecycle.py (1)
24-25: Add defensive validation for lifecycle TTL settings.If
session_timeout_minutesorconversation_close_hoursis0/negative, this task can aggressively transition large amounts of data unexpectedly.🔧 Proposed guard
async def _run_lifecycle() -> None: + if settings.session_timeout_minutes <= 0 or settings.conversation_close_hours <= 0: + raise ValueError("Lifecycle settings must be positive: session_timeout_minutes, conversation_close_hours") + session_timeout = timedelta(minutes=settings.session_timeout_minutes) close_after = timedelta(hours=settings.conversation_close_hours)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/celery/tasks/conversation_lifecycle.py` around lines 24 - 25, Validate settings.session_timeout_minutes and settings.conversation_close_hours before constructing timedelta: check that both are positive integers (or at least > 0); if a value is zero/negative, either replace it with a safe minimum (e.g., 1 minute / 1 hour) or log an error and raise/abort startup so the task cannot use an aggressive TTL. Update the logic around session_timeout and close_after in conversation_lifecycle.py to perform this guard, referencing session_timeout, close_after, settings.session_timeout_minutes, and settings.conversation_close_hours so invalid configs are handled defensively.agent/tests/repositories/test_message_repository.py (1)
58-63: Unnecessary local import.
uuid4could be imported at the module level alongside other imports for consistency with the rest of the codebase.Proposed fix
+from uuid import uuid4 + from sqlalchemy.ext.asyncio import AsyncSession from app.repositories import message_repository from app.schemas.agent import MessageRole from tests.factories import ConversationFactory, SessionFactoryThen remove the local import:
async def test_returns_empty_list_for_unknown_conversation(self, db: AsyncSession) -> None: - from uuid import uuid4 - messages = await message_repository.get_by_conversation_id(db, uuid4())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tests/repositories/test_message_repository.py` around lines 58 - 63, Move the local import of uuid4 out of the test body and import it at module level for consistency; in the test_returns_empty_list_for_unknown_conversation function remove "from uuid import uuid4" and use the module-level uuid4 when calling message_repository.get_by_conversation_id(db, uuid4()) so the test uses the top-level import like other tests.agent/config/.env.example (1)
21-25: SENTRY_ENABLED=True with empty DSN may cause startup issues.
SENTRY_ENABLED=TruebutSENTRY_DSN=""could cause initialization failures if the Sentry SDK expects a valid DSN when enabled. Consider defaultingSENTRY_ENABLED=Falsein the example file.Proposed fix
`#---` SENTRY ---# -SENTRY_ENABLED=True +SENTRY_ENABLED=False SENTRY_DSN=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/config/.env.example` around lines 21 - 25, The .env example currently sets SENTRY_ENABLED=True while SENTRY_DSN is empty which can break startup; update the example to set SENTRY_ENABLED=False by default (or explicitly document that SENTRY_DSN must be set when SENTRY_ENABLED=True) and add a short comment next to SENTRY_ENABLED/SENTRY_DSN (SENTRY_ENABLED, SENTRY_DSN, SENTRY_ENV, SENTRY_SAMPLES_RATE) clarifying that enabling Sentry requires a valid DSN to avoid initialization errors.agent/tests/api/v1/test_chat.py (2)
37-55: Test doesn't verify all arguments passed toprocess_message.delay().Per the endpoint implementation (
agent/app/api/routes/v1/chat.py:29-35),delay()is also called withuser_idandcallback_urlarguments, but the test only verifiesmessage,session_id, andconversation_id. Consider adding assertions for the remaining arguments for complete coverage.Suggested additions
mock_celery.delay.assert_called_once() call_kwargs = mock_celery.delay.call_args.kwargs assert call_kwargs["message"] == "Steps today?" assert call_kwargs["session_id"] == str(active_session.id) assert call_kwargs["conversation_id"] == str(active_conversation.id) + assert "user_id" in call_kwargs + assert call_kwargs["callback_url"] == CALLBACK_URL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tests/api/v1/test_chat.py` around lines 37 - 55, Update the test_calls_process_message_delay_with_correct_args test to assert that process_message.delay was called with all expected kwargs: add assertions checking call_kwargs["user_id"] equals the active_session.user_id (or str(active_session.user_id) depending on implementation) and call_kwargs["callback_url"] equals CALLBACK_URL; locate the mock invocation via mock_celery.delay.call_args.kwargs in the existing test and append these two assertions alongside the existing checks for "message", "session_id", and "conversation_id".
58-64: Redundant local import ofuuid4.
uuid4is already imported at line 4 (from uuid import UUID, uuid4), so the local import at line 59 is unnecessary.Proposed fix
def test_returns_empty_list_for_unknown_conversation(self, db: AsyncSession) -> None: - from uuid import uuid4 - messages = await message_repository.get_by_conversation_id(db, uuid4())Note: This comment applies to line 59 in
test_message_repository.py, but the same pattern is correctly used in this file at line 59.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tests/api/v1/test_chat.py` around lines 58 - 64, Remove the redundant local import of uuid4 and rely on the existing top-level import (from uuid import UUID, uuid4); specifically delete the duplicate "from uuid import uuid4" entry so uses of uuid4() in the test (e.g., the f"/api/v1/chat/{uuid4()}" call in test_chat.py) use the already-imported symbol and avoid shadowing or duplication.agent/app/agent/workflows/agent_workflow.py (1)
76-101: Consider moving imports to module level.The local imports of
Agentandget_llminsidesummarize()could be moved to the module level for consistency. If this is intentional (e.g., to avoid circular imports at module load time), consider adding a brief comment explaining why.If no circular dependency, move imports to module level
from pydantic_ai.messages import ModelMessage, ModelRequest, UserPromptPart +from pydantic_ai import Agent from pygentic_ai import WorkflowState, user_assistant_graph from pygentic_ai.workflows.nodes import StartNode from app.agent.engines.guardrails import HealthGuardrailsAgent from app.agent.engines.reasoning import HealthReasoningAgent from app.agent.engines.router import HealthRouter from app.agent.tools.tool_registry import tool_manager +from app.agent.utils.model_utils import get_llm from app.schemas.agent import AgentModeThen remove lines 82-84 inside
summarize().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/agent/workflows/agent_workflow.py` around lines 76 - 101, The local imports of Agent and get_llm inside summarize() cause inconsistent import placement; move "from pydantic_ai import Agent" and "from app.agent.utils.model_utils import get_llm" to the module level (top of agent_workflow.py) and remove the local imports in summarize(), or if they were kept to avoid a circular import, leave them in place but add a brief comment above the local imports explaining that they are intentionally local to avoid a circular dependency; reference the summarize function and the Agent and get_llm symbols when making the change.agent/tests/factories.py (1)
45-46: Consider usingfactory.Maybe()to deriveMessageFactory.conversationfromsessionwhen provided.Passing
session=without also overridingconversation=would create a message whoseconversation_iddisagrees withsession.conversation_id, resulting in invalid test data. While no current call sites exhibit this pattern, usingfactory.Maybe()as shown below locks in the invariant and prevents accidental misuse:Suggested refactoring
class MessageFactory(BaseFactory): class Meta: model = Message - conversation = factory.SubFactory(ConversationFactory) session = None + conversation = factory.Maybe( + "session", + yes_declaration=factory.SelfAttribute("session.conversation"), + no_declaration=factory.SubFactory(ConversationFactory), + ) role = MessageRole.USER content = Sequence(lambda n: f"Test message {n}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tests/factories.py` around lines 45 - 46, The MessageFactory currently sets conversation = factory.SubFactory(ConversationFactory) and session = None which can produce a message whose conversation_id disagrees with a provided session; change MessageFactory to derive conversation from session using factory.Maybe (or equivalent) so that when session is provided conversation is set to session.conversation (or session.conversation_id) and when session is None it falls back to factory.SubFactory(ConversationFactory), ensuring the conversation and session IDs always match; update the MessageFactory fields `conversation` and `session` to use factory.Maybe(session, yes=... , no=...) referencing `MessageFactory.conversation` and `MessageFactory.session`.agent/app/services/conversation.py (2)
150-151: Use negative indexing for cleaner slicing.The slicing can be simplified using Python's negative index syntax.
Suggested simplification
- old = messages[: len(messages) - threshold] - recent = messages[len(messages) - threshold :] + old = messages[:-threshold] + recent = messages[-threshold:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 150 - 151, Replace the manual index arithmetic when splitting messages into old and recent by using Python negative indexing: instead of computing old = messages[: len(messages) - threshold] and recent = messages[len(messages) - threshold :], use messages[:-threshold] for old and messages[-threshold:] for recent (handle threshold==0 if needed); update the variables old and recent in conversation.py accordingly to simplify the slicing logic.
107-123: Consider validating session existence before persisting messages.If
session_idpoints to a non-existent session, messages are still created butrequest_countisn't incremented. This silent fallback could mask bugs. Consider either logging a warning or raising an error if the session is not found.Option: Add logging for missing session
session = await session_repository.get_by_id(self._db, session_id) + if session is None: + logger.warning(f"Session {session_id} not found; skipping request_count increment") await message_repository.create(self._db, conversation_id, MessageRole.USER, user_message, session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 107 - 123, save_messages currently creates messages even when session_repository.get_by_id returns None, causing request_count to never be incremented and masking missing sessions; modify save_messages to first call session_repository.get_by_id(self._db, session_id) and validate the result before creating messages: if the session is None either raise a clear exception (e.g., SessionNotFoundError or ValueError) or log a warning and abort the message creation flow, ensuring you do not call message_repository.create or session_repository.increment_request_count when no session exists, and keep the existing call to self._touch_conversation(conversation_id) only if you decide that touch should still occur for missing sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/app/agent/prompts/agent_prompts.py`:
- Around line 37-38: In agent_prompts.py fix the typos and remove the stale
`fitedo_` prefix guidance: change "befor" to "before" and "consultion" to
"consulting", and replace the paragraph that instructs to prefer tools with a
`fitedo_` prefix with guidance that references the real tool naming scheme
(e.g., prefer functions exposed by agent/app/agent/tools/ow_tools.py such as
get_user_profile, get_recent_activity, etc.); ensure the prompt says to query
those tool functions first and only consult general tools if they return no
relevant info, and to favor their outputs when conflicts arise.
In `@agent/app/agent/prompts/worker_prompts.py`:
- Around line 127-130: Validate soft_word_limit before formatting: change the
branch that currently checks only "if soft_word_limit is None" to treat
non-positive values as equivalent to None (or raise a ValueError) so you don't
format _GUARDRAILS_LENGTH_SECTION with 0 or negative numbers; update the logic
around the soft_word_limit check (the variables/functions: soft_word_limit,
length_section, _GUARDRAILS_NO_LENGTH_SECTION, _GUARDRAILS_LENGTH_SECTION) to
use "if soft_word_limit is None or soft_word_limit <= 0" (or raise) and only
call _GUARDRAILS_LENGTH_SECTION.format(...) when soft_word_limit > 0.
In `@agent/app/database.py`:
- Around line 47-49: The id_str property uses inspect(self).identity[0] which
will raise when the object is transient (inspect(self).identity is None); update
the id_str property implementation (the property method named id_str) to guard
against a None identity — e.g., check inspect(self).identity is not None before
indexing and return a safe fallback (empty string or str(getattr(self, "id",
"")) or a classname-based placeholder) when identity is None so accessing id_str
on unsaved instances no longer raises.
In `@agent/app/integrations/celery/tasks/expire_sessions.py`:
- Line 13: SESSION_MAX_AGE is hardcoded to timedelta(hours=1) which can drift
from the configured session timeout; replace the constant with a settings-driven
value by reading settings.session_timeout_minutes (or equivalent) and computing
SESSION_MAX_AGE = timedelta(minutes=settings.session_timeout_minutes) where
SESSION_MAX_AGE is defined/used (refer to the SESSION_MAX_AGE symbol in
expire_sessions.py) so the TTL matches the lifecycle task timeout; ensure
settings is imported and add a sensible fallback if the setting is missing or
invalid.
In `@agent/app/models/chat_session.py`:
- Around line 34-35: The Session.messages relationship currently uses
cascade="all, delete-orphan" which contradicts Message.session_id
(ForeignKey(..., ondelete="SET NULL")) and can delete messages instead of
letting the DB set session_id to NULL; remove the delete-orphan part of the
cascade (or remove cascade entirely) and add passive_deletes=True on the
relationship definition for messages in the Session class so deletion behavior
is delegated to the FK constraint (refer to Session.messages and
Message.session_id to locate the change).
In `@agent/app/services/conversation.py`:
- Around line 133-162: The build_history method currently takes a db:
AsyncSession parameter which is inconsistent with the rest of the service that
uses self._db; change the signature of build_history to remove the db parameter
and use self._db inside (update all internal calls such as
message_repository.get_by_conversation_id(self._db, conversation.id) and
conversation_repository.update_summary(self._db, conversation, summary)), keep
the lazy import of workflow_engine and the summarize call as-is, and update any
callers to stop passing a db argument so the service always uses its own
session.
In `@agent/app/utils/exceptions.py`:
- Around line 15-20: The __init__ of the exception class incorrectly treats
falsy IDs (like 0) as missing and lacks an explicit return annotation; update
the signature of __init__ to include a -> None return type and change the check
from "if entity_id:" to "if entity_id is not None:" so valid falsy IDs are
handled correctly (modify the __init__ in exceptions.py where entity_name and
entity_id are set).
- Around line 29-33: The helper function named "_" currently places raw DB
exception text (exc.args[0]) into the HTTPException detail; change it to return
a generic 400 message like "{entity.capitalize()} already exists." and remove
exc.args[0] from the response, and instead log the full exception server-side
(use the existing logger or add one) before raising the HTTPException; update
the HTTPException construction in "_" and ensure the SQLAIntegrityError |
PsycopgIntegrityError handling no longer exposes internal DB details.
- Around line 50-55: The helper function _(exc: RequestValidationError, _: str)
is unsafely indexing into exc.args and assumes exc.args[0][0]['ctx']['error']
exists; change it to pull the first error from exc.errors() and use safe .get()
access for fields like 'msg' and nested 'ctx'/'error' (e.g., error =
err.get('ctx', {}).get('error') or a sensible fallback such as err.get('type')
or err.get('msg')), then build the HTTPException.detail using those safely
accessed values so missing ctx or error keys no longer raise KeyError in the
function named "_".
- Around line 58-67: The decorator handle_exceptions currently always calls func
synchronously and so misses exceptions raised by coroutine functions; update it
to detect async functions (use inspect.iscoroutinefunction(func)) and return an
async wrapper that awaits func when it's a coroutine, and a sync wrapper that
calls func normally otherwise; both wrappers should use `@wraps`(func), accept
instance: Service, *args, **kwargs, catch Exception as exc, compute entity_name
= getattr(instance, "name", "unknown") and raise handle_exception(exc,
entity_name) from exc, and preserve the existing typing/generic signatures (P,
T, Service) and return types.
In `@agent/app/utils/healthcheck.py`:
- Around line 26-42: The database_health async endpoint currently uses the
synchronous DbSession which blocks the event loop; change the dependency and
type hint to AsyncDbSession, call the async execution (await
db.execute(text("SELECT 1"))) instead of a sync call, and update imports to
import AsyncDbSession; ensure any helper used to inspect the pool
(get_pool_status) is compatible with async (make it async or call it in a
non-blocking way) so database_health remains fully non-blocking.
In `@agent/Dockerfile`:
- Around line 22-39: The Dockerfile currently runs as root; create and switch to
a non-root user before the final EXPOSE to follow least-privilege practices: add
steps after the COPY/ENV blocks (after WORKDIR /root_project and before EXPOSE
8000) to create a group/user (e.g., appuser), chown relevant runtime paths
(/root_project, /opt/venv and the copied binaries /uv /uvx /bin) to that user,
and add a USER appuser (or use a numeric UID/GID for compatibility). Ensure the
USER instruction is applied after all files are copied and permissions are set
so the process runs as the non-root user at container start.
- Around line 24-27: The runtime Dockerfile currently installs git in the RUN
command (the apt-get install -y --no-install-recommends libpq5 git step) which
is unnecessary at runtime; remove "git" from that install line so only libpq5 is
installed in the final stage, keeping git limited to the builder stage where
pygentic-ai is cloned and dependencies are prepared.
- Line 9: The Dockerfile uses an unpinned image reference in the COPY
--from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ instruction which makes builds
non-reproducible; replace the :latest tag with a specific released uv tag (for
example a tested release like vX.Y.Z from the project's releases) so the COPY
--from=... reference is deterministic, then rebuild and verify the image
contents (ensure the chosen tag exists on ghcr.io and matches expected /uv and
/uvx binaries).
In `@agent/tests/api/v1/test_chat.py`:
- Around line 84-100: In test_returns_410_for_inactive_session the
ConversationFactory and SessionFactory instances are created but not flushed to
the test DB before making the HTTP request; after creating conv =
ConversationFactory(...) and SessionFactory(...), call await db.flush() (or
db.flush() if synchronous in this test harness) to persist those fixtures so the
Chat endpoint sees the inactive session prior to calling client.post.
- Around line 66-82: The test test_returns_410_for_closed_conversation creates
ConversationFactory(...) and SessionFactory(... ) but doesn’t persist them; add
an await db.flush() immediately after those factory calls (before the
client.post) so the Conversation and Session are written to the DB and the
endpoint can find closed_conv.id; reference the ConversationFactory,
SessionFactory, test_returns_410_for_closed_conversation and use db.flush() to
persist.
In `@agent/tests/conftest.py`:
- Around line 109-116: The _create_schema fixture currently calls
BaseDbModel.metadata.create_all() but skips creating Postgres ENUM types marked
with create_type=False, causing table creation to fail; update the fixture to
run the project's migrations before creating/dropping tables (e.g., invoke the
agent migrations runner or Alembic programmatically using the provided migration
module/runner) using the async_engine/conn so the ENUMs (conversationstatus,
messagerole) are created prior to BaseDbModel.metadata.create_all and are
dropped/rolled back appropriately after tests.
In `@backend/app/schemas/model_crud/user_management/user.py`:
- Around line 57-75: The validator UserRead._flatten_personal_record currently
uses getattr(data, "personal_record", None), which can trigger SQLAlchemy
lazy-loads during validation; change the validator to avoid descriptor access by
reading the mapped attribute only from the instance state (e.g., use
data.__dict__.get("personal_record") when data has a __dict__ or treat data as
dict) so you never call the attribute descriptor and thus avoid lazy loads in
_flatten_personal_record; additionally ensure queries eagerly load the
relationship by adding joined/eager loading of personal_record in
UserRepository.get_users_with_filters (used by get_users_paginated/list
endpoints) and in CrudRepository.get (used by user_service.get and async
list_users) so the personal_record is present before validation.
In `@backend/app/services/summaries_service.py`:
- Around line 407-417: When paginating backwards the code filters and reverses
all_dates before slicing but never reverses them back, causing descending order
results; update the logic in the function handling pagination (the block using
variables direction, all_dates, cursor_date, limit, has_more) so that after
computing has_more and slicing all_dates = all_dates[:limit], you reverse the
sliced list back to ascending order when direction == "prev" (mirror the
behavior used in get_sleep_summaries) to ensure both forward and backward
pagination return dates in consistent ascending order.
---
Nitpick comments:
In `@agent/app/agent/tools/ow_tools.py`:
- Line 148: The function get_heart_rate_timeseries contains a local "from
datetime import datetime, timezone" import; move these imports to the
module-level imports (consolidate with the existing datetime-related imports at
the top of the file) and remove the in-function import so datetime and timezone
are imported once for the module and available to get_heart_rate_timeseries and
other functions.
- Around line 165-174: OW_TOOLS is currently annotated with the bare type
"list"; update it to a more specific typing such as List[Callable[..., Any]] or
Sequence[Callable[..., Any]] (or the concrete Tool type if one exists) to
improve IDE/autocomplete and static analysis; update/import the appropriate
typing symbols (e.g., from typing import List, Callable, Any or from typing
import Sequence) and annotate OW_TOOLS: List[Callable[..., Any]] =
[get_user_profile, get_body_composition, get_recent_activity, get_recent_sleep,
get_recovery_data, get_workouts, get_sleep_events, get_heart_rate_timeseries]
(or replace Callable[..., Any] with the project's Tool type if available).
In `@agent/app/agent/workflows/agent_workflow.py`:
- Around line 76-101: The local imports of Agent and get_llm inside summarize()
cause inconsistent import placement; move "from pydantic_ai import Agent" and
"from app.agent.utils.model_utils import get_llm" to the module level (top of
agent_workflow.py) and remove the local imports in summarize(), or if they were
kept to avoid a circular import, leave them in place but add a brief comment
above the local imports explaining that they are intentionally local to avoid a
circular dependency; reference the summarize function and the Agent and get_llm
symbols when making the change.
In `@agent/app/integrations/celery/tasks/conversation_lifecycle.py`:
- Around line 24-25: Validate settings.session_timeout_minutes and
settings.conversation_close_hours before constructing timedelta: check that both
are positive integers (or at least > 0); if a value is zero/negative, either
replace it with a safe minimum (e.g., 1 minute / 1 hour) or log an error and
raise/abort startup so the task cannot use an aggressive TTL. Update the logic
around session_timeout and close_after in conversation_lifecycle.py to perform
this guard, referencing session_timeout, close_after,
settings.session_timeout_minutes, and settings.conversation_close_hours so
invalid configs are handled defensively.
In `@agent/app/integrations/ow_backend/client.py`:
- Around line 27-35: The get_user_profile method opens an httpx.AsyncClient
without a timeout, risking hung requests; add a consistent timeout by defining a
class-level constant (e.g. _TIMEOUT = 30.0) on the client class and pass it into
httpx.AsyncClient(timeout=self._TIMEOUT) (or accept timeout via __init__ and
store it) so that get_user_profile (and other HTTP methods) use that timeout to
avoid indefinite blocking.
In `@agent/app/repositories/conversation.py`:
- Around line 44-51: get_active_by_user_id can raise MultipleResultsFound if
multiple ACTIVE conversations exist; update the select in get_active_by_user_id
to pick the most recent one by adding an ordering and limiting to one result
(e.g., add .order_by(Conversation.created_at.desc()).limit(1) to the select for
Conversation) so the query returns a single row even if duplicates exist;
alternatively document or enforce a uniqueness constraint on
Conversation.user_id + status if you prefer.
In `@agent/app/services/conversation.py`:
- Around line 150-151: Replace the manual index arithmetic when splitting
messages into old and recent by using Python negative indexing: instead of
computing old = messages[: len(messages) - threshold] and recent =
messages[len(messages) - threshold :], use messages[:-threshold] for old and
messages[-threshold:] for recent (handle threshold==0 if needed); update the
variables old and recent in conversation.py accordingly to simplify the slicing
logic.
- Around line 107-123: save_messages currently creates messages even when
session_repository.get_by_id returns None, causing request_count to never be
incremented and masking missing sessions; modify save_messages to first call
session_repository.get_by_id(self._db, session_id) and validate the result
before creating messages: if the session is None either raise a clear exception
(e.g., SessionNotFoundError or ValueError) or log a warning and abort the
message creation flow, ensuring you do not call message_repository.create or
session_repository.increment_request_count when no session exists, and keep the
existing call to self._touch_conversation(conversation_id) only if you decide
that touch should still occur for missing sessions.
In `@agent/app/utils/healthcheck.py`:
- Around line 45-71: The llm_health async endpoint is calling blocking SDK
methods (openai.OpenAI(...).models.retrieve, genai_get_model,
anthropic.Anthropic(...).models.retrieve) directly; update llm_health to run
these blocking calls off the event loop (e.g., use asyncio.to_thread or
loop.run_in_executor) around the calls invoked after get_llm and genai_configure
so the health check remains non-blocking, preserve the existing try/except
behavior and returned fields, and ensure you call the same unique symbols
(openai.OpenAI, client.models.retrieve, genai_configure, genai_get_model,
anthropic.Anthropic) inside the threaded task.
- Around line 38-42: The current except block returns a 200 with
{"status":"unhealthy",...}; change it to return an actual 503 response so load
balancers detect failures: replace the dict return in the except with either
raise HTTPException(status_code=503,
detail={"status":"unhealthy","error":str(e)}) or return
Response(content=json.dumps({"status":"unhealthy","error":str(e)}),
media_type="application/json", status_code=503), and add the necessary imports
(HTTPException or Response and json) at top of the module.
In `@agent/config/.env.example`:
- Around line 21-25: The .env example currently sets SENTRY_ENABLED=True while
SENTRY_DSN is empty which can break startup; update the example to set
SENTRY_ENABLED=False by default (or explicitly document that SENTRY_DSN must be
set when SENTRY_ENABLED=True) and add a short comment next to
SENTRY_ENABLED/SENTRY_DSN (SENTRY_ENABLED, SENTRY_DSN, SENTRY_ENV,
SENTRY_SAMPLES_RATE) clarifying that enabling Sentry requires a valid DSN to
avoid initialization errors.
In `@agent/Dockerfile`:
- Around line 3-7: The Dockerfile contains leading spaces before instructions
(e.g., the RUN lines shown) which is unconventional; remove the leading
indentation from all Dockerfile instructions (RUN, COPY, ENV, etc.) so each
instruction starts at column 0, and apply this consistently across the file (for
example update the two RUN lines shown to have no leading spaces and do the same
for any other indented Dockerfile directives).
- Around line 13-14: The Dockerfile currently copies the whole repo before
running dependency sync which defeats layer caching: keep the initial dependency
layer by retaining the "COPY uv.lock pyproject.toml ./" step, remove or move the
"COPY . ." step so that the full source copy occurs only after the dependency
install (after the "uv sync" step); specifically, place the "COPY . ."
instruction after the "uv sync" command so changes to application source do not
invalidate the cached dependency installation based on uv.lock and
pyproject.toml.
In `@agent/tests/api/v1/test_chat.py`:
- Around line 37-55: Update the
test_calls_process_message_delay_with_correct_args test to assert that
process_message.delay was called with all expected kwargs: add assertions
checking call_kwargs["user_id"] equals the active_session.user_id (or
str(active_session.user_id) depending on implementation) and
call_kwargs["callback_url"] equals CALLBACK_URL; locate the mock invocation via
mock_celery.delay.call_args.kwargs in the existing test and append these two
assertions alongside the existing checks for "message", "session_id", and
"conversation_id".
- Around line 58-64: Remove the redundant local import of uuid4 and rely on the
existing top-level import (from uuid import UUID, uuid4); specifically delete
the duplicate "from uuid import uuid4" entry so uses of uuid4() in the test
(e.g., the f"/api/v1/chat/{uuid4()}" call in test_chat.py) use the
already-imported symbol and avoid shadowing or duplication.
In `@agent/tests/factories.py`:
- Around line 45-46: The MessageFactory currently sets conversation =
factory.SubFactory(ConversationFactory) and session = None which can produce a
message whose conversation_id disagrees with a provided session; change
MessageFactory to derive conversation from session using factory.Maybe (or
equivalent) so that when session is provided conversation is set to
session.conversation (or session.conversation_id) and when session is None it
falls back to factory.SubFactory(ConversationFactory), ensuring the conversation
and session IDs always match; update the MessageFactory fields `conversation`
and `session` to use factory.Maybe(session, yes=... , no=...) referencing
`MessageFactory.conversation` and `MessageFactory.session`.
In `@agent/tests/repositories/test_message_repository.py`:
- Around line 58-63: Move the local import of uuid4 out of the test body and
import it at module level for consistency; in the
test_returns_empty_list_for_unknown_conversation function remove "from uuid
import uuid4" and use the module-level uuid4 when calling
message_repository.get_by_conversation_id(db, uuid4()) so the test uses the
top-level import like other tests.
In `@backend/app/schemas/model_crud/user_management/user.py`:
- Around line 63-75: The _flatten_personal_record function currently builds a
hardcoded dict duplicating UserRead fields; change it to derive the base mapping
from cls.model_fields (iterate cls.model_fields keys and set each value to
getattr(data, key, None)) to avoid drift, then explicitly override the
personal-record fields (birth_date, sex, gender) using
pr.birth_date/pr.sex/pr.gender if pr exists, leaving other fields untouched;
reference the _flatten_personal_record function, cls.model_fields, UserRead, and
the pr variable when making this change.
🪄 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: b3b0f584-98c4-4b88-8728-810c95b4a036
⛔ Files ignored due to path filters (1)
agent/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitattributes.vscode/settings.jsonagent/Dockerfileagent/app/agent/__init__.pyagent/app/agent/engines/__init__.pyagent/app/agent/engines/guardrails.pyagent/app/agent/engines/reasoning.pyagent/app/agent/engines/router.pyagent/app/agent/prompts/__init__.pyagent/app/agent/prompts/agent_prompts.pyagent/app/agent/prompts/worker_prompts.pyagent/app/agent/static/__init__.pyagent/app/agent/static/default_msgs.pyagent/app/agent/tools/__init__.pyagent/app/agent/tools/date_tools.pyagent/app/agent/tools/ow_tools.pyagent/app/agent/tools/tool_registry.pyagent/app/agent/utils/__init__.pyagent/app/agent/utils/model_utils.pyagent/app/agent/workflows/__init__.pyagent/app/agent/workflows/agent_workflow.pyagent/app/api/routes/v1/__init__.pyagent/app/api/routes/v1/chat.pyagent/app/api/routes/v1/conversation.pyagent/app/config.pyagent/app/database.pyagent/app/integrations/celery/core.pyagent/app/integrations/celery/tasks/conversation_lifecycle.pyagent/app/integrations/celery/tasks/expire_sessions.pyagent/app/integrations/celery/tasks/process_message.pyagent/app/integrations/ow_backend/__init__.pyagent/app/integrations/ow_backend/client.pyagent/app/main.pyagent/app/models/__init__.pyagent/app/models/chat_session.pyagent/app/models/conversation.pyagent/app/models/message.pyagent/app/repositories/__init__.pyagent/app/repositories/chat_session.pyagent/app/repositories/conversation.pyagent/app/repositories/message.pyagent/app/repositories/repositories.pyagent/app/schemas/agent.pyagent/app/schemas/chat.pyagent/app/schemas/conversation.pyagent/app/schemas/language.pyagent/app/schemas/message.pyagent/app/services/__init__.pyagent/app/services/conversation.pyagent/app/utils/exceptions.pyagent/app/utils/healthcheck.pyagent/config/.env.exampleagent/migrations/versions/001_initial_schema.pyagent/pyproject.tomlagent/scripts/start/app.shagent/scripts/start/beat.shagent/scripts/start/flower.shagent/scripts/start/worker.shagent/tests/__init__.pyagent/tests/agent/__init__.pyagent/tests/agent/test_tool_registry.pyagent/tests/agent/test_tools.pyagent/tests/agent/test_workflow.pyagent/tests/api/__init__.pyagent/tests/api/v1/__init__.pyagent/tests/api/v1/conftest.pyagent/tests/api/v1/test_chat.pyagent/tests/api/v1/test_conversation.pyagent/tests/conftest.pyagent/tests/factories.pyagent/tests/repositories/__init__.pyagent/tests/repositories/test_conversation_repository.pyagent/tests/repositories/test_message_repository.pyagent/tests/repositories/test_session_repository.pyagent/tests/services/__init__.pyagent/tests/services/test_conversation_service.pyagent/tests/tasks/__init__.pyagent/tests/tasks/test_conversation_lifecycle.pybackend/app/api/routes/v1/summaries.pybackend/app/repositories/data_point_series_repository.pybackend/app/schemas/model_crud/user_management/user.pybackend/app/services/summaries_service.py
✅ Files skipped from review due to trivial changes (12)
- .vscode/settings.json
- .gitattributes
- agent/app/integrations/ow_backend/init.py
- agent/scripts/start/worker.sh
- agent/scripts/start/beat.sh
- agent/app/repositories/init.py
- agent/scripts/start/app.sh
- agent/app/agent/tools/date_tools.py
- agent/app/schemas/message.py
- agent/app/schemas/conversation.py
- agent/pyproject.toml
- agent/app/repositories/repositories.py
🚧 Files skipped from review as they are similar to previous changes (14)
- agent/app/schemas/chat.py
- agent/app/models/init.py
- agent/app/services/init.py
- agent/app/api/routes/v1/init.py
- agent/app/api/routes/v1/chat.py
- agent/scripts/start/flower.sh
- agent/app/repositories/message.py
- agent/app/schemas/agent.py
- agent/app/integrations/celery/core.py
- agent/app/main.py
- agent/app/integrations/celery/tasks/process_message.py
- agent/app/repositories/chat_session.py
- agent/app/models/conversation.py
- agent/app/config.py
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| SESSION_MAX_AGE = timedelta(hours=1) |
There was a problem hiding this comment.
Use settings-driven timeout instead of a hardcoded 1-hour TTL.
SESSION_MAX_AGE here can drift from the lifecycle task timeout (settings.session_timeout_minutes), causing inconsistent deactivation behavior between tasks.
🔧 Proposed fix
from __future__ import annotations
import asyncio
import logging
from datetime import timedelta
+from app.config import settings
from app.database import AsyncSessionLocal
from app.repositories import session_repository
from celery import shared_task
logger = logging.getLogger(__name__)
-SESSION_MAX_AGE = timedelta(hours=1)
+SESSION_MAX_AGE = timedelta(minutes=settings.session_timeout_minutes)📝 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
| SESSION_MAX_AGE = timedelta(hours=1) | |
| from __future__ import annotations | |
| import asyncio | |
| import logging | |
| from datetime import timedelta | |
| from app.config import settings | |
| from app.database import AsyncSessionLocal | |
| from app.repositories import session_repository | |
| from celery import shared_task | |
| logger = logging.getLogger(__name__) | |
| SESSION_MAX_AGE = timedelta(minutes=settings.session_timeout_minutes) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/integrations/celery/tasks/expire_sessions.py` at line 13,
SESSION_MAX_AGE is hardcoded to timedelta(hours=1) which can drift from the
configured session timeout; replace the constant with a settings-driven value by
reading settings.session_timeout_minutes (or equivalent) and computing
SESSION_MAX_AGE = timedelta(minutes=settings.session_timeout_minutes) where
SESSION_MAX_AGE is defined/used (refer to the SESSION_MAX_AGE symbol in
expire_sessions.py) so the TTL matches the lifecycle task timeout; ensure
settings is imported and add a sensible fallback if the setting is missing or
invalid.
Comment on lines
+34
to
+35
| messages: Mapped[list["Message"]] = relationship( # type: ignore[name-defined] | ||
| back_populates="session", cascade="all, delete-orphan" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'delete-orphan|back_populates="session"' agent/app/models/chat_session.py
rg -n -C2 'session_id|ondelete="SET NULL"' agent/app/models/message.pyRepository: the-momentum/open-wearables
Length of output: 577
🏁 Script executed:
# Check if there's any code that depends on delete-orphan behavior
# Look for delete patterns on Message or Session
rg -n -A3 -B3 'delete.*Message|Message.*delete' agent/app/models/ --type py
# Look for session deletion or message cleanup code
rg -n -A3 -B3 'session.*delete|delete.*session' agent/app/ --type py | head -50
# Check if there are any comments explaining this design
rg -n -B5 'delete-orphan' agent/app/models/chat_session.pyRepository: the-momentum/open-wearables
Length of output: 3181
🏁 Script executed:
# Check the full chat_session.py model to understand the complete picture
cat -n agent/app/models/chat_session.py
# Check the full message.py model
cat -n agent/app/models/message.py
# Look for any session deletion tests or code that might rely on cascade behavior
rg -n 'Session\(' agent/app/ --type py | grep -i delete | head -20Repository: the-momentum/open-wearables
Length of output: 3402
Remove delete-orphan cascade—it conflicts with the FK constraint.
Message.session_id is nullable and uses ForeignKey("sessions.id", ondelete="SET NULL"), meaning the database schema preserves messages when a session is deleted by setting session_id to NULL. However, cascade="all, delete-orphan" on Session.messages will delete the messages instead, contradicting the FK intent and risking unexpected loss of conversation history.
Suggested fix
messages: Mapped[list["Message"]] = relationship( # type: ignore[name-defined]
- back_populates="session", cascade="all, delete-orphan"
+ back_populates="session",
+ passive_deletes=True,
)Set passive_deletes=True to delegate deletion behavior to the foreign key constraint instead of overriding it at the ORM level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/models/chat_session.py` around lines 34 - 35, The Session.messages
relationship currently uses cascade="all, delete-orphan" which contradicts
Message.session_id (ForeignKey(..., ondelete="SET NULL")) and can delete
messages instead of letting the DB set session_id to NULL; remove the
delete-orphan part of the cascade (or remove cascade entirely) and add
passive_deletes=True on the relationship definition for messages in the Session
class so deletion behavior is delegated to the FK constraint (refer to
Session.messages and Message.session_id to locate the change).
Comment on lines
+109
to
+116
| @pytest_asyncio.fixture(scope="session") | ||
| async def _create_schema(async_engine: Any) -> AsyncGenerator[None, None]: | ||
| """Create all tables once per session.""" | ||
| async with async_engine.begin() as conn: | ||
| await conn.run_sync(BaseDbModel.metadata.create_all) | ||
| yield | ||
| async with async_engine.begin() as conn: | ||
| await conn.run_sync(BaseDbModel.metadata.drop_all) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'metadata\.create_all|metadata\.drop_all' agent/tests/conftest.py
rg -n -C2 'Enum\(.*create_type=False|create_type=False' agent/app/models
rg -n 'alembic|upgrade\(|downgrade\(' agent/tests/conftest.py agent/testsRepository: the-momentum/open-wearables
Length of output: 1029
🏁 Script executed:
# Check full conftest.py for any setup that might handle enum types
head -120 agent/tests/conftest.py | tail -50
# Also check if there are any other test fixtures that might pre-create types
rg -n 'def.*enum|create_type|pg_enum' agent/tests/ -A 3
# Check if pytest plugins or other conftest files in parent directories handle this
find agent/tests -name 'conftest.py' -type fRepository: the-momentum/open-wearables
Length of output: 1914
🏁 Script executed:
# Find Alembic migrations that create enum types
rg -n 'CREATE TYPE|enum|Enum' backend/migrations/versions/ -l | head -5
# Check a few migration files for enum type creation patterns
find backend/migrations/versions -name '*.py' -type f | head -3 | xargs grep -l 'CREATE TYPE\|enum' | head -1 | xargs cat
# Check if there's any migration runner or alembic upgrade in test initialization
rg -n 'alembic|upgrade|Alembic' agent/tests/ agent/app/ --type pyRepository: the-momentum/open-wearables
Length of output: 139
🏁 Script executed:
# Read the priority migration to see how enums are handled
cat backend/migrations/versions/2026_02_03_2007-0e8f0a9be2cc_priority.py
# Find the initial/earliest migration to see base enum setup
ls -1t backend/migrations/versions/*.py | tail -5
# Check for any enum type creation in migrations
rg -B2 -A2 'op\.execute.*CREATE TYPE|op\.inline_literal' backend/migrations/versions/ | head -50Repository: the-momentum/open-wearables
Length of output: 4768
🏁 Script executed:
# Search for imports of Message and Conversation in tests
rg -n 'from.*message|from.*conversation|import.*Message|import.*Conversation' agent/tests/ --type py
# Check if BaseDbModel.metadata includes all models
rg -n 'BaseDbModel|metadata' agent/app/models/__init__.py agent/app/models/base.py
# List all models to see what's in the metadata
ls -la agent/app/models/Repository: the-momentum/open-wearables
Length of output: 4288
🏁 Script executed:
# Find BaseDbModel definition
rg -n 'class BaseDbModel' agent/ backend/ -A 3
# Check conftest imports
head -30 agent/tests/conftest.py
# Find where metadata is defined
rg -n 'metadata.*=' agent/app/models/ -B 2Repository: the-momentum/open-wearables
Length of output: 1728
🏁 Script executed:
# Check conftest imports for BaseDbModel
grep -n 'import.*BaseDbModel\|from.*BaseDbModel' agent/tests/conftest.py
# Full view of conftest.py to find BaseDbModel usage
rg -n 'BaseDbModel' agent/tests/conftest.py -B5 -A5
# Check database.py for metadata and how models are registered
cat agent/app/database.py | head -80Repository: the-momentum/open-wearables
Length of output: 3100
🏁 Script executed:
# Check what models are auto-discovered by checking model __init__.py
cat agent/app/models/__init__.py
# Check if all models are explicitly imported somewhere in database.py or conftest
rg -n 'import.*models' agent/app/database.py agent/tests/conftest.py
# Verify that Message and Conversation models actually extend BaseDbModel
grep -n 'class Message\|class Conversation' agent/app/models/message.py agent/app/models/conversation.py -A 2Repository: the-momentum/open-wearables
Length of output: 586
🏁 Script executed:
# Check what factories imports (which should trigger model imports)
head -20 agent/tests/factories.py
# Verify if conftest imports factories before calling metadata.create_all
grep -n 'import factories\|from tests import' agent/tests/conftest.py
# Check if there's any migration runner or alembic in the full conftest
rg -n 'alembic\|Alembic\|upgrade\|migrate' agent/tests/conftest.py
# Try to understand the actual test execution: do tests actually pass?
# Check if there's a test that would fail if enum types don't exist
fd test_.*\.py agent/tests/repositories/ -exec grep -l 'Message\|Conversation' {} \;Repository: the-momentum/open-wearables
Length of output: 890
🏁 Script executed:
# Check if conftest imports factories before calling metadata.create_all
grep -n 'import factories\|from tests import' agent/tests/conftest.py -A 2
# Check the order of imports and fixture definitions in conftest
head -50 agent/tests/conftest.py | tail -20
# Search for actual test execution with Message/Conversation
rg -n 'ConversationFactory\|MessageFactory' agent/tests/ | head -10
# Most importantly: verify the enum actually exists after metadata.create_all
# by checking if there's any PostgreSQL type creation
rg -n 'CREATE TYPE\|ENUM' agent/app/models/Repository: the-momentum/open-wearables
Length of output: 848
🏁 Script executed:
# Check the earliest migration to see the base schema setup
ls -1 backend/migrations/versions/*.py | sort | head -1 | xargs cat | head -100
# Check if there's any Python code in conftest that might pre-create enum types
rg -n 'op\.execute\|raw_sql\|exec\|CREATE' agent/tests/conftest.py
# Most importantly: check the actual models to see exactly how enums are declared
cat agent/app/models/message.py
# And check conversation model
cat agent/app/models/conversation.pyRepository: the-momentum/open-wearables
Length of output: 7226
🏁 Script executed:
# Search all migrations for ConversationStatus and MessageRole enum creation
rg -n 'ConversationStatus\|MessageRole' backend/migrations/versions/
# Check if there's any migration that creates these types explicitly
rg -n 'CREATE TYPE.*messagerstatus\|CREATE TYPE.*conversationstatus' backend/migrations/versions/ -i
# Check all migrations for the actual enum name that would be created
rg -n "name=" backend/migrations/versions/ | grep -i 'messagerstatus\|conversationstatus\|messageerole\|role\|status'Repository: the-momentum/open-wearables
Length of output: 590
🏁 Script executed:
# Check if there's a migration that creates these agent tables at all
rg -n 'conversations\|messages' backend/migrations/versions/ | head -20
# List all migrations in order
ls -1 backend/migrations/versions/*.py | sort
# Check if agent tests are even running the schema creation
rg -n '@pytest_asyncio.fixture' agent/tests/conftest.py -A 5Repository: the-momentum/open-wearables
Length of output: 1779
🏁 Script executed:
# The agent app has its own database setup, separate from backend
# Check if there's an agent/app/migrations directory
find agent/ -type d -name migrations
# Check if agent/app/models/__init__.py is imported before metadata.create_all in conftest
head -120 agent/tests/conftest.py | grep -n 'import\|metadata'
# Check what happens when conftest imports 'from tests import factories'
# The factories module imports Message and Conversation
# This should trigger model registration but won't create enum types
# Let's verify: does the test suite actually work currently?
# Check if there's any issue tracking or known failures
rg -n 'TODO\|FIXME\|BUG\|HACK' agent/tests/conftest.py
# Most importantly: check if metadata.create_all can even work with create_type=False
# by looking at what SQLAlchemy version is being used
rg -n 'sqlalchemy' agent/requirements.txt setup.py pyproject.toml 2>/dev/null | head -10Repository: the-momentum/open-wearables
Length of output: 1127
🏁 Script executed:
# Check agent/migrations for the schema setup
ls -la agent/migrations/
# Check for enum creation in agent migrations
rg -n 'MessageRole\|ConversationStatus\|CREATE TYPE' agent/migrations/versions/
# Check the env.py to understand how migrations are set up
cat agent/migrations/env.py 2>/dev/null | head -80
# Check the initial agent migration
ls -1 agent/migrations/versions/*.py | head -1 | xargs catRepository: the-momentum/open-wearables
Length of output: 6095
🏁 Script executed:
# Critical question: does the test fixture actually run migrations?
# Check if there's any migration runner in conftest or test setup
rg -n 'upgrade\|downgrade\|Alembic\|alembic\|revision' agent/tests/conftest.py
# Check if agent/migrations is ever invoked during test setup
rg -n 'agent.migrations\|agent/migrations' agent/tests/
# Check the env.py to see if it's configured to auto-run or if tests invoke it
grep -n 'import app.models' agent/migrations/env.py
# Verify: if migrations aren't run, then enum types don't exist, so metadata.create_all() should fail
# Let's confirm by checking if the test actually passes with this setup
# by looking for recent test runs or CI config
find . -name '.github' -o -name 'pytest.ini' -o -name 'tox.ini' | head -5Repository: the-momentum/open-wearables
Length of output: 114
🏁 Script executed:
# Check GitHub workflows to see if tests actually pass
cat .github/workflows/*.yml 2>/dev/null | grep -A 10 'pytest\|test' | head -30
# Check if there's a pytest.ini or setup.cfg that might run migrations
cat agent/pytest.ini pyproject.toml setup.cfg 2>/dev/null | head -80
# Most crucially: check if the test fixture catches and ignores exceptions
rg -n 'try\|except\|ignore' agent/tests/conftest.py -B 2 -A 2Repository: the-momentum/open-wearables
Length of output: 911
Test schema bootstrap bypasses enum type creation.
The fixture uses metadata.create_all() but agent models declare enums with create_type=False. The agent migrations (agent/migrations/versions/001_initial_schema.py) explicitly create these types with CREATE TYPE conversationstatus and CREATE TYPE messagerole, but the test fixture never runs migrations. With create_type=False, metadata.create_all() won't create the types, causing table creation to fail when referencing undefined enum columns. This fixture must run agent migrations before calling metadata.create_all().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/tests/conftest.py` around lines 109 - 116, The _create_schema fixture
currently calls BaseDbModel.metadata.create_all() but skips creating Postgres
ENUM types marked with create_type=False, causing table creation to fail; update
the fixture to run the project's migrations before creating/dropping tables
(e.g., invoke the agent migrations runner or Alembic programmatically using the
provided migration module/runner) using the async_engine/conn so the ENUMs
(conversationstatus, messagerole) are created prior to
BaseDbModel.metadata.create_all and are dropped/rolled back appropriately after
tests.
Comment on lines
+57
to
+75
| @model_validator(mode="before") | ||
| @classmethod | ||
| def _flatten_personal_record(cls, data: Any) -> Any: | ||
| if isinstance(data, dict): | ||
| return data | ||
| pr = getattr(data, "personal_record", None) | ||
| return { | ||
| "id": getattr(data, "id", None), | ||
| "created_at": getattr(data, "created_at", None), | ||
| "first_name": getattr(data, "first_name", None), | ||
| "last_name": getattr(data, "last_name", None), | ||
| "email": getattr(data, "email", None), | ||
| "external_user_id": getattr(data, "external_user_id", None), | ||
| "last_synced_at": getattr(data, "last_synced_at", None), | ||
| "last_synced_provider": getattr(data, "last_synced_provider", None), | ||
| "birth_date": pr.birth_date if pr else None, | ||
| "sex": pr.sex if pr else None, | ||
| "gender": pr.gender if pr else None, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find all UserRead serialization call sites
rg -n --type=py -C4 'UserRead\.model_validate\(|response_model\s*=\s*UserRead' backend
# 2) Check whether personal_record is eagerly loaded in user query paths
rg -n --type=py -C4 'selectinload\([^)]*personal_record|joinedload\([^)]*personal_record|contains_eager\([^)]*personal_record' backend/app/services/user_service.py backend/app/api/routes/v1/users.pyRepository: the-momentum/open-wearables
Length of output: 2737
🏁 Script executed:
#!/bin/bash
# Check the exact implementation of user_service methods
cat backend/app/services/user_service.py | head -130Repository: the-momentum/open-wearables
Length of output: 4520
🏁 Script executed:
#!/bin/bash
# Look at the full user_service file to see query patterns
wc -l backend/app/services/user_service.pyRepository: the-momentum/open-wearables
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Check the get method specifically
sed -n '1,200p' backend/app/services/user_service.py | tail -100Repository: the-momentum/open-wearables
Length of output: 3908
🏁 Script executed:
#!/bin/bash
# Find UserRepository implementation
find backend -name "*user_repository*" -type fRepository: the-momentum/open-wearables
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Check AppService base class to understand inherited get() method
find backend -name "services.py" -o -name "*base*service*" | head -5Repository: the-momentum/open-wearables
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Look at the User model to confirm personal_record is a relationship
find backend -path "*models*" -name "*.py" | xargs grep -l "class User"Repository: the-momentum/open-wearables
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Read UserRepository to see query construction
cat backend/app/repositories/user_repository.pyRepository: the-momentum/open-wearables
Length of output: 4343
🏁 Script executed:
#!/bin/bash
# Read AppService to understand inherited get() method
cat backend/app/services/services.pyRepository: the-momentum/open-wearables
Length of output: 3810
🏁 Script executed:
#!/bin/bash
# Read User model to confirm personal_record relationship
sed -n '1,100p' backend/app/models/user.pyRepository: the-momentum/open-wearables
Length of output: 731
🏁 Script executed:
#!/bin/bash
# Check the UserRead schema to see if personal_record fields are actually required/used
cat backend/app/schemas/model_crud/user_management/user.pyRepository: the-momentum/open-wearables
Length of output: 3636
🏁 Script executed:
#!/bin/bash
# Check if there are any async endpoints in the users.py routes
cat backend/app/api/routes/v1/users.pyRepository: the-momentum/open-wearables
Length of output: 2306
🏁 Script executed:
#!/bin/bash
# Look for PersonalRecord model and schema to understand the relationship
find backend -name "*personal*" -type f | head -10Repository: the-momentum/open-wearables
Length of output: 169
Avoid implicit relationship lazy-loads inside UserRead validator across all endpoints.
getattr(data, "personal_record", None) triggers SQLAlchemy lazy loading during schema validation. In the list endpoint get_users_paginated(), this causes N+1 queries when looping over users. In all endpoints (including async list_users), lazy loading from the descriptor protocol can fail outside the session context.
Suggested adjustment (avoid lazy-load side effect in schema layer)
`@model_validator`(mode="before")
`@classmethod`
def _flatten_personal_record(cls, data: Any) -> Any:
if isinstance(data, dict):
return data
- pr = getattr(data, "personal_record", None)
+ # Avoid triggering ORM lazy-load from schema validation
+ pr = getattr(data, "__dict__", {}).get("personal_record")
return {
"id": getattr(data, "id", None),
"created_at": getattr(data, "created_at", None),
"first_name": getattr(data, "first_name", None),
"last_name": getattr(data, "last_name", None),
"email": getattr(data, "email", None),
"external_user_id": getattr(data, "external_user_id", None),
"last_synced_at": getattr(data, "last_synced_at", None),
"last_synced_provider": getattr(data, "last_synced_provider", None),
"birth_date": pr.birth_date if pr else None,
"sex": pr.sex if pr else None,
"gender": pr.gender if pr else None,
}Additionally, ensure all user query paths eagerly load personal_record:
UserRepository.get_users_with_filters()for list endpointCrudRepository.get()(inherited inuser_service.get()) for single-user endpoints andasync list_users()
📝 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
| @model_validator(mode="before") | |
| @classmethod | |
| def _flatten_personal_record(cls, data: Any) -> Any: | |
| if isinstance(data, dict): | |
| return data | |
| pr = getattr(data, "personal_record", None) | |
| return { | |
| "id": getattr(data, "id", None), | |
| "created_at": getattr(data, "created_at", None), | |
| "first_name": getattr(data, "first_name", None), | |
| "last_name": getattr(data, "last_name", None), | |
| "email": getattr(data, "email", None), | |
| "external_user_id": getattr(data, "external_user_id", None), | |
| "last_synced_at": getattr(data, "last_synced_at", None), | |
| "last_synced_provider": getattr(data, "last_synced_provider", None), | |
| "birth_date": pr.birth_date if pr else None, | |
| "sex": pr.sex if pr else None, | |
| "gender": pr.gender if pr else None, | |
| } | |
| `@model_validator`(mode="before") | |
| `@classmethod` | |
| def _flatten_personal_record(cls, data: Any) -> Any: | |
| if isinstance(data, dict): | |
| return data | |
| # Avoid triggering ORM lazy-load from schema validation | |
| pr = getattr(data, "__dict__", {}).get("personal_record") | |
| return { | |
| "id": getattr(data, "id", None), | |
| "created_at": getattr(data, "created_at", None), | |
| "first_name": getattr(data, "first_name", None), | |
| "last_name": getattr(data, "last_name", None), | |
| "email": getattr(data, "email", None), | |
| "external_user_id": getattr(data, "external_user_id", None), | |
| "last_synced_at": getattr(data, "last_synced_at", None), | |
| "last_synced_provider": getattr(data, "last_synced_provider", None), | |
| "birth_date": pr.birth_date if pr else None, | |
| "sex": pr.sex if pr else None, | |
| "gender": pr.gender if pr else None, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/schemas/model_crud/user_management/user.py` around lines 57 - 75,
The validator UserRead._flatten_personal_record currently uses getattr(data,
"personal_record", None), which can trigger SQLAlchemy lazy-loads during
validation; change the validator to avoid descriptor access by reading the
mapped attribute only from the instance state (e.g., use
data.__dict__.get("personal_record") when data has a __dict__ or treat data as
dict) so you never call the attribute descriptor and thus avoid lazy loads in
_flatten_personal_record; additionally ensure queries eagerly load the
relationship by adding joined/eager loading of personal_record in
UserRepository.get_users_with_filters (used by get_users_paginated/list
endpoints) and in CrudRepository.get (used by user_service.get and async
list_users) so the personal_record is present before validation.
Previously _build_history() only converted user turns to ModelRequest objects and silently dropped assistant messages. This meant the agent had no context of its previous responses each turn. Now assistant messages are also converted to ModelResponse(TextPart) objects, giving the agent the full alternating history via message_history.
…uidance
The old TEXT_REACTAGENT_PATTERN was a LlamaIndex/LangChain-style text-parsing
ReAct prompt (Thought/Action/Action Input/Observation) incompatible with
pydantic-ai's native function-calling tool loop.
Problems fixed:
- {tool_desc} and {tool_names} placeholders were never substituted, appearing
as literal strings in the system prompt
- Stale tool prefix references from a previous project
- Redundant language instructions already handled by BaseAgent's
@agent.instructions decorator
- Manual ReAct format instructions that conflict with pydantic-ai's execution
Replaced with plain guidance that works with pydantic-ai's native tool use:
the model calls tools via function-calling API, pydantic-ai handles the
multi-turn loop internally — no text-format orchestration needed.
Added max_tool_calls setting (default 10) and wired it to UsageLimits on HealthReasoningAgent to prevent runaway tool-call loops. Removed dead max_retries config field that was never used.
Previously user_id was prepended to the user message as [user_id=...] and the model was expected to parse and pass it correctly to every tool call — fragile and leaking an internal ID into the conversation. - Add HealthAgentDeps(BaseAgentDeps) with user_id field (app/agent/deps.py) - Rewrite all OW tools to take RunContext[HealthAgentDeps] as first arg; user_id is read from ctx.deps, removed from model-visible parameters - HealthReasoningAgent stores user_id on the instance, passes deps_type, and overrides generate_response to inject HealthAgentDeps per run - WorkflowEngine passes user_id to the agent constructor, drops augmented_message - Remove dead get_guardrails_refusal_msg and its backing dict - Restore max_retries to config (used by Celery @shared_task decorator)
…ssification ClassifyNode only forwards the current message to router.route(), so vague follow-ups like "What can I do to improve it?" were classified as off-topic without any conversation context. HealthRouter now accepts history at construction time and overrides route() to prepend the last 3 conversation turns as a context block, letting the LLM recognise health-related follow-ups correctly.
The router's raw reasoning was embedded verbatim in the refusal template,
leaking verbose English chain-of-thought even for non-English conversations.
- Pass conversation language to HealthRouter so BaseAgent's dynamic
instruction ("respond in X language") applies to the reasoning field
- Instruct the router prompt to cap the reasoning at 2 sentences and
omit internal classification logic
- Extract _msg_to_line() helper to keep _build_context() under complexity limit
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
agent/app/utils/exceptions.py (3)
44-49:⚠️ Potential issue | 🟠 MajorAvoid returning raw DB exception details to clients.
Line 48 includes
exc.args[0]in the HTTP response, which can expose internal constraint/schema names. Return a generic message and log the raw details server-side.Suggested fix
`@handle_exception.register` def _(exc: SQLAIntegrityError | PsycopgIntegrityError, entity: str) -> HTTPException: + logger.warning("Integrity error for %s: %s", entity, exc.args[0] if exc.args else "unknown") return HTTPException( status_code=400, - detail=f"{entity.capitalize()} entity already exists. Details: {exc.args[0]}", + detail=f"{entity.capitalize()} entity already exists.", )Note: You'll need to add
logger = logging.getLogger(__name__)at the module level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 44 - 49, The SQLA/Psycopg integrity error handler (the handle_exception.register function that takes exc: SQLAIntegrityError | PsycopgIntegrityError and entity: str) must stop returning raw DB details to clients; add logger = logging.getLogger(__name__) at module scope, log the full exc (e.g., logger.exception or logger.error with exc) inside this handler, and change the HTTPException.detail to a generic message like "{entity.capitalize()} entity already exists." while preserving status_code=400—do not include exc.args[0] in the response.
80-87:⚠️ Potential issue | 🟠 MajorUnsafe access to
exc.args[0][0]can raise IndexError/TypeError.Line 82 assumes
exc.args[0][0]exists. Ifexc.argsis empty orexc.args[0]isn't subscriptable, this crashes. Useexc.errors()for safer access.Suggested fix
`@handle_exception.register` def _(exc: RequestValidationError, _: str) -> HTTPException: - err_args = exc.args[0][0] - msg = err_args.get("msg", "Validation error") - ctx = err_args.get("ctx", {}) - error = ctx.get("error", "") if ctx else "" + errors = exc.errors() + first_error = errors[0] if errors else {} + msg = first_error.get("msg", "Validation error") + ctx = first_error.get("ctx") or {} + error = ctx.get("error", "") detail = f"{msg} - {error}" if error else msg return HTTPException(status_code=400, detail=detail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 80 - 87, The handler registered for RequestValidationError (the function decorated with `@handle_exception.register`) unsafely indexes exc.args[0][0]; replace that with a safe extraction using exc.errors() (e.g., get the first error dict from exc.errors() if present), then pull "msg" and "ctx" from that dict with defaults and fallback to a generic message; ensure you handle exc.errors() being empty or non-list by using defaults before returning HTTPException(status_code=400, detail=detail).
15-21:⚠️ Potential issue | 🟡 MinorFix falsy ID handling and add missing
-> Nonereturn annotation.The check
if entity_id:on line 18 treats valid IDs like0as missing. Also,__init__methods should annotate-> Noneper coding guidelines.Suggested fix
class ResourceNotFoundError(Exception): - def __init__(self, entity_name: str, entity_id: int | UUID | None = None): + def __init__(self, entity_name: str, entity_id: int | UUID | None = None) -> None: self.entity_name = entity_name - if entity_id: + if entity_id is not None: self.detail = f"{entity_name.capitalize()} with ID: {entity_id} not found." else: self.detail = f"{entity_name.capitalize()} not found."As per coding guidelines:
**/*.py: All Python functions must have type annotations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 15 - 21, Update ResourceNotFoundError.__init__ to include the missing return annotation and to correctly detect explicit falsy IDs: change the signature to annotate return type as "-> None" and replace the conditional "if entity_id:" with "if entity_id is not None" so that valid IDs like 0 are treated as present when building self.detail; keep the existing parameter types and behavior otherwise.agent/app/services/conversation.py (1)
145-153: 🛠️ Refactor suggestion | 🟠 MajorInconsistent
dbparameter vs.self._dbused elsewhere.This method accepts a separate
db: AsyncSessionparameter while all other methods useself._db. This inconsistency can cause session-mixing bugs.Suggested fix
- async def build_history(self, conversation: Conversation, db: AsyncSession) -> list[dict[str, str]]: + async def build_history(self, conversation: Conversation) -> list[dict[str, str]]: """Return message history for the LLM, summarizing if over threshold. Imported lazily to avoid circular imports with workflow_engine. """ from app.config import settings - messages = await message_repository.get_by_conversation_id(db, conversation.id) + messages = await message_repository.get_by_conversation_id(self._db, conversation.id)Also update line 171:
- await conversation_repository.update_summary(db, conversation, summary) + await conversation_repository.update_summary(self._db, conversation, summary)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 145 - 153, The build_history method currently takes a db: AsyncSession parameter but other methods use the instance attribute self._db, risking session mixing; remove the db parameter from build_history's signature and replace uses of the local db variable with self._db (e.g., call message_repository.get_by_conversation_id(self._db, conversation.id)), and update any callers (referenced near the previous call site) to call build_history(conversation) without passing a session; ensure imports and the `@handle_exceptions` decorator remain unchanged.
🧹 Nitpick comments (3)
agent/app/integrations/ow_backend/client.py (2)
56-68: Remove the# type: ignoreon Line 67 by shapingparamsas a typed mapping.The ignore suppresses static checks in a hot path and can be avoided with a clearer params structure.
Proposed diff
- params: list[tuple[str, str]] = [ - ("start_time", start_time), - ("end_time", end_time), - ("resolution", resolution), - ] - for t in types: - params.append(("types[]", t)) + params: dict[str, str | list[str]] = { + "start_time": start_time, + "end_time": end_time, + "resolution": resolution, + "types[]": types, + } async with httpx.AsyncClient() as client: resp = await client.get( f"{self._base()}/api/v1/users/{user_id}/timeseries", headers=self._headers(), - params=params, # type: ignore + params=params, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/ow_backend/client.py` around lines 56 - 68, The params variable is currently a list[tuple[str, str]] with a trailing "# type: ignore" when passed to httpx.AsyncClient.get; replace it with a properly typed mapping (e.g. params: dict[str, str | list[str]]) so multiple "types" values are passed as a list under a single key (e.g. "types": types) and then call client.get(..., params=params) without the ignore; update the params construction in the method that builds the request (the params variable and the client.get invocation) to remove the type: ignore and satisfy static typing.
27-150: Extract a private request helper to de-duplicate the repeated GET/raise/JSON pattern.Seven methods repeat identical transport logic; centralizing it will reduce bug surface and future edit cost.
Refactor sketch
+from typing import Any, Mapping + class OWClient: @@ + async def _get_json( + self, + path: str, + params: Mapping[str, Any] | None = None, + ) -> dict[str, Any]: + async with httpx.AsyncClient() as client: + resp = await client.get( + f"{self._base()}{path}", + headers=self._headers(), + params=params, + ) + resp.raise_for_status() + return resp.json() @@ async def get_user_profile(self, user_id: UUID) -> dict: - async with httpx.AsyncClient() as client: - resp = await client.get( - f"{self._base()}/api/v1/users/{user_id}", - headers=self._headers(), - ) - resp.raise_for_status() - return resp.json() + return await self._get_json(f"/api/v1/users/{user_id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/ow_backend/client.py` around lines 27 - 150, Extract a private helper like _get(self, path: str, params: dict | list | None = None) that creates httpx.AsyncClient(), calls client.get(f"{self._base()}{path}", headers=self._headers(), params=params), calls resp.raise_for_status(), and returns resp.json(); then replace the repeated client/get/raise/json blocks in get_user_profile, get_body_summary, get_timeseries, get_activity_summaries, get_sleep_summaries, get_recovery_summaries, get_workout_events, and get_sleep_events to call this helper (pass the already-built path strings and the params list used in get_timeseries so the ("types[]", t) entries are preserved). Ensure signatures and return types remain the same and that _get is async.agent/app/services/conversation.py (1)
163-172: Summary may become stale as conversation grows.The summary is generated once when messages first exceed
threshold. As more messages accumulate, the "old" slice grows but the summary isn't updated—leaving recent-but-not-in-recent messages unsummarized. Consider either:
- Re-summarizing periodically (e.g., when old messages double)
- Documenting this as intentional one-time compression
Simplified slicing (optional)
- old = messages[: len(messages) - threshold] - recent = messages[len(messages) - threshold :] + old = messages[:-threshold] + recent = messages[-threshold:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 163 - 172, The current logic only generates conversation.summary once and never refreshes it as "old" grows; add a small mechanism to trigger re-summarization when the amount of old messages grows significantly (e.g., doubles): store an integer like conversation.summary_message_count when you call workflow_engine.summarize, then on subsequent calls compute old = messages[:len(messages)-threshold] and if len(old) >= max(threshold * 2, conversation.summary_message_count * 2) (or simply len(old) >= conversation.summary_message_count * 2) call workflow_engine.summarize(old_history) again and persist the new summary via conversation_repository.update_summary and update conversation.summary_message_count to len(old); keep using the existing symbols (messages, threshold, conversation.summary, workflow_engine.summarize, conversation_repository.update_summary, old, recent) to locate where to insert this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/app/services/conversation.py`:
- Around line 127-135: In save_messages, the code calls
session_repository.get_by_id(self._db, session_id) but proceeds to create
messages even if session is None and only conditionally skips
increment_request_count; either treat a missing session as an error or make the
resilience explicit—modify save_messages to check the session immediately after
get_by_id and if a missing session is considered an error, raise a clear
exception (including session_id) before calling message_repository.create;
otherwise, add an explicit log entry (via your logger) or a comment explaining
why creating messages without a session is allowed, and ensure
increment_request_count is only called when session is present; refer to
save_messages, session_repository.get_by_id, message_repository.create,
session_repository.increment_request_count, and _touch_conversation when making
the change.
In `@agent/app/utils/exceptions.py`:
- Around line 24-36: The three custom exception classes (AccessDeniedError,
GoneError, ConflictError) have __init__ methods missing return type annotations;
update each __init__ signature (in AccessDeniedError.__init__,
GoneError.__init__, ConflictError.__init__) to include the explicit return
annotation "-> None" (e.g., def __init__(..., ...) -> None:) while leaving the
bodies unchanged so they satisfy the project-wide Python type-annotation rule.
- Around line 72-77: The AttributeError handler registered via
handle_exception.register currently returns an HTTPException that includes
exc.args[0], which can leak internal details; change the handler (the registered
function handling AttributeError) to return a generic client-safe message like
"{entity.capitalize()} does not support the requested attribute or method"
without including exc.args[0], and instead log the full exception server-side
(e.g., logger.exception or logger.error with exc) for debugging while ensuring
the HTTP response contains no raw exception data.
---
Duplicate comments:
In `@agent/app/services/conversation.py`:
- Around line 145-153: The build_history method currently takes a db:
AsyncSession parameter but other methods use the instance attribute self._db,
risking session mixing; remove the db parameter from build_history's signature
and replace uses of the local db variable with self._db (e.g., call
message_repository.get_by_conversation_id(self._db, conversation.id)), and
update any callers (referenced near the previous call site) to call
build_history(conversation) without passing a session; ensure imports and the
`@handle_exceptions` decorator remain unchanged.
In `@agent/app/utils/exceptions.py`:
- Around line 44-49: The SQLA/Psycopg integrity error handler (the
handle_exception.register function that takes exc: SQLAIntegrityError |
PsycopgIntegrityError and entity: str) must stop returning raw DB details to
clients; add logger = logging.getLogger(__name__) at module scope, log the full
exc (e.g., logger.exception or logger.error with exc) inside this handler, and
change the HTTPException.detail to a generic message like "{entity.capitalize()}
entity already exists." while preserving status_code=400—do not include
exc.args[0] in the response.
- Around line 80-87: The handler registered for RequestValidationError (the
function decorated with `@handle_exception.register`) unsafely indexes
exc.args[0][0]; replace that with a safe extraction using exc.errors() (e.g.,
get the first error dict from exc.errors() if present), then pull "msg" and
"ctx" from that dict with defaults and fallback to a generic message; ensure you
handle exc.errors() being empty or non-list by using defaults before returning
HTTPException(status_code=400, detail=detail).
- Around line 15-21: Update ResourceNotFoundError.__init__ to include the
missing return annotation and to correctly detect explicit falsy IDs: change the
signature to annotate return type as "-> None" and replace the conditional "if
entity_id:" with "if entity_id is not None" so that valid IDs like 0 are treated
as present when building self.detail; keep the existing parameter types and
behavior otherwise.
---
Nitpick comments:
In `@agent/app/integrations/ow_backend/client.py`:
- Around line 56-68: The params variable is currently a list[tuple[str, str]]
with a trailing "# type: ignore" when passed to httpx.AsyncClient.get; replace
it with a properly typed mapping (e.g. params: dict[str, str | list[str]]) so
multiple "types" values are passed as a list under a single key (e.g. "types":
types) and then call client.get(..., params=params) without the ignore; update
the params construction in the method that builds the request (the params
variable and the client.get invocation) to remove the type: ignore and satisfy
static typing.
- Around line 27-150: Extract a private helper like _get(self, path: str,
params: dict | list | None = None) that creates httpx.AsyncClient(), calls
client.get(f"{self._base()}{path}", headers=self._headers(), params=params),
calls resp.raise_for_status(), and returns resp.json(); then replace the
repeated client/get/raise/json blocks in get_user_profile, get_body_summary,
get_timeseries, get_activity_summaries, get_sleep_summaries,
get_recovery_summaries, get_workout_events, and get_sleep_events to call this
helper (pass the already-built path strings and the params list used in
get_timeseries so the ("types[]", t) entries are preserved). Ensure signatures
and return types remain the same and that _get is async.
In `@agent/app/services/conversation.py`:
- Around line 163-172: The current logic only generates conversation.summary
once and never refreshes it as "old" grows; add a small mechanism to trigger
re-summarization when the amount of old messages grows significantly (e.g.,
doubles): store an integer like conversation.summary_message_count when you call
workflow_engine.summarize, then on subsequent calls compute old =
messages[:len(messages)-threshold] and if len(old) >= max(threshold * 2,
conversation.summary_message_count * 2) (or simply len(old) >=
conversation.summary_message_count * 2) call
workflow_engine.summarize(old_history) again and persist the new summary via
conversation_repository.update_summary and update
conversation.summary_message_count to len(old); keep using the existing symbols
(messages, threshold, conversation.summary, workflow_engine.summarize,
conversation_repository.update_summary, old, recent) to locate where to insert
this logic.
🪄 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: 8e01b2b8-0cdd-4411-a445-babad986f0bd
📒 Files selected for processing (11)
agent/app/integrations/celery/core.pyagent/app/integrations/celery/tasks/__init__.pyagent/app/integrations/ow_backend/client.pyagent/app/services/conversation.pyagent/app/utils/exceptions.pyagent/tests/agent/test_workflow.pyagent/tests/api/v1/conftest.pyagent/tests/api/v1/test_chat.pyagent/tests/api/v1/test_conversation.pyagent/tests/conftest.pyagent/tests/tasks/test_conversation_lifecycle.py
✅ Files skipped from review due to trivial changes (3)
- agent/tests/api/v1/test_chat.py
- agent/tests/agent/test_workflow.py
- agent/tests/api/v1/test_conversation.py
🚧 Files skipped from review as they are similar to previous changes (5)
- agent/app/integrations/celery/tasks/init.py
- agent/app/integrations/celery/core.py
- agent/tests/tasks/test_conversation_lifecycle.py
- agent/tests/conftest.py
- agent/tests/api/v1/conftest.py
Comment on lines
+127
to
+135
| session = await session_repository.get_by_id(self._db, session_id) | ||
|
|
||
| await message_repository.create(self._db, conversation_id, MessageRole.USER, user_message, session_id) | ||
| await message_repository.create(self._db, conversation_id, MessageRole.ASSISTANT, assistant_message, session_id) | ||
|
|
||
| if session is not None: | ||
| await session_repository.increment_request_count(self._db, session) | ||
|
|
||
| await self._touch_conversation(conversation_id) |
There was a problem hiding this comment.
Handle missing session explicitly in save_messages.
Line 127 fetches the session, but if it's None, messages are still created (lines 129-130) while increment_request_count is silently skipped. If a missing session is an error state, raise early; if it's intentional resilience, add a log or comment.
Suggested fix (if missing session is an error)
async def save_messages(
self,
conversation_id: UUID,
session_id: UUID,
user_message: str,
assistant_message: str,
) -> None:
"""Persist user + assistant message pair and update conversation timestamp."""
session = await session_repository.get_by_id(self._db, session_id)
+ if session is None:
+ raise ResourceNotFoundError("session", session_id)
await message_repository.create(self._db, conversation_id, MessageRole.USER, user_message, session_id)
await message_repository.create(self._db, conversation_id, MessageRole.ASSISTANT, assistant_message, session_id)
- if session is not None:
- await session_repository.increment_request_count(self._db, session)
+ await session_repository.increment_request_count(self._db, session)
await self._touch_conversation(conversation_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/services/conversation.py` around lines 127 - 135, In save_messages,
the code calls session_repository.get_by_id(self._db, session_id) but proceeds
to create messages even if session is None and only conditionally skips
increment_request_count; either treat a missing session as an error or make the
resilience explicit—modify save_messages to check the session immediately after
get_by_id and if a missing session is considered an error, raise a clear
exception (including session_id) before calling message_repository.create;
otherwise, add an explicit log entry (via your logger) or a comment explaining
why creating messages without a session is allowed, and ensure
increment_request_count is only called when session is present; refer to
save_messages, session_repository.get_by_id, message_repository.create,
session_repository.increment_request_count, and _touch_conversation when making
the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/app/agent/engines/router.py`:
- Line 40: The route method's variadic kwargs parameter is untyped — update the
async def route(self, message: str, **kwargs) -> RoutingResponse signature to
annotate kwargs (e.g., **kwargs: Any) and add the required import (from typing
import Any) at top of the module so the function is fully typed; ensure you
modify the signature in the Router class's route method and run type checks to
confirm compliance.
🪄 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: 851ed85a-4194-4b7c-92f7-10ab16d18c6a
📒 Files selected for processing (9)
agent/app/agent/deps.pyagent/app/agent/engines/reasoning.pyagent/app/agent/engines/router.pyagent/app/agent/prompts/agent_prompts.pyagent/app/agent/prompts/worker_prompts.pyagent/app/agent/static/default_msgs.pyagent/app/agent/tools/ow_tools.pyagent/app/agent/workflows/agent_workflow.pyagent/app/config.py
✅ Files skipped from review due to trivial changes (1)
- agent/app/agent/deps.py
🚧 Files skipped from review as they are similar to previous changes (7)
- agent/app/agent/static/default_msgs.py
- agent/app/agent/prompts/agent_prompts.py
- agent/app/agent/engines/reasoning.py
- agent/app/agent/workflows/agent_workflow.py
- agent/app/agent/prompts/worker_prompts.py
- agent/app/agent/tools/ow_tools.py
- agent/app/config.py
Security: - Replace unmaintained python-jose + unused passlib with PyJWT - Remove raw DB/exception details from HTTP error responses (log server-side) - Pin uv to 0.11.6 in Dockerfile instead of :latest - Add non-root appuser to Dockerfile runtime stage Correctness: - Remove delete-orphan cascade from Session.messages (messages belong to conversations, not sessions) - Fix build_history to use self._db consistently; drop separate db param and update callers - Guard id_str against unsaved (transient) model instances - Fix if entity_id: → if entity_id is not None: in ResourceNotFoundError - Fix backward pagination order in summaries_service (re-reverse after prev-direction slice) - Create messagrole PG enum type before metadata.create_all in test conftest Quality: - Add -> None return annotations to all exception __init__ methods - Add **kwargs: Any annotation to HealthRouter.route - Add soft_word_limit > 0 validation in build_worker_prompt - Switch healthcheck /db endpoint to AsyncDbSession + await db.execute - Add logger.warning when session is None in save_messages
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
agent/app/utils/exceptions.py (1)
85-92:⚠️ Potential issue | 🟡 MinorUnsafe indexing into
exc.argsmay raise IndexError.Line 87 accesses
exc.args[0][0]without checking ifexc.argsis non-empty or ifexc.args[0]is indexable. While the subsequent.get()calls are safe, the initial access could raiseIndexErrororTypeErrorifRequestValidationErroris raised with unexpected arguments.🛠️ Proposed fix using exc.errors()
`@handle_exception.register` def _(exc: RequestValidationError, _: str) -> HTTPException: - err_args = exc.args[0][0] + errors = exc.errors() + err_args = errors[0] if errors else {} msg = err_args.get("msg", "Validation error") ctx = err_args.get("ctx", {}) error = ctx.get("error", "") if ctx else "" detail = f"{msg} - {error}" if error else msg return HTTPException(status_code=400, detail=detail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/exceptions.py` around lines 85 - 92, The handler registered as handle_exception for RequestValidationError currently unsafely indexes exc.args (err_args = exc.args[0][0]) which can raise IndexError/TypeError; replace that with a safe extraction using exc.errors() (e.g., errs = exc.errors(); err_args = errs[0] if errs else {}), then keep the existing msg/ctx/error extraction and return HTTPException(status_code=400, detail=detail); update the function _ for RequestValidationError accordingly to avoid direct exc.args indexing.
🧹 Nitpick comments (8)
agent/app/services/conversation.py (3)
164-165: Off-by-one edge case when messages equal threshold.When
len(messages) == threshold, line 161 returns all messages (correct). Whenlen(messages) > threshold, lines 164-165 compute:
old = messages[:len(messages) - threshold]recent = messages[len(messages) - threshold:]This means
recentalways has exactlythresholdmessages, andoldcontains the remainder. The logic is correct, but the slice at line 165 creates a copy. Consider using negative indexing for clarity:recent = messages[-threshold:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 164 - 165, Change the slicing that builds `old` and `recent` in the conversation handling to use negative indexing for clarity: keep `old` as messages up to the cutoff and set `recent` using `-threshold` (i.e., `recent = messages[-threshold:]`) so behaviour is identical when len(messages) == threshold but the intent is clearer; update the code around the `old` and `recent` assignments in conversation.py (the variables `old` and `recent`) accordingly.
167-173: Summarization only runs whenconversation.summaryis falsy.If
conversation.summaryis an empty string"", the conditionif not conversation.summaryevaluates toTrue, triggering re-summarization. This is probably fine, but if an empty summary is intentionally stored (e.g., to indicate "no summary needed"), it would be re-summarized on every call.Consider using
if conversation.summary is None:if onlyNoneshould trigger summarization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 167 - 173, The current check uses a falsy test on conversation.summary which causes re-summarization for empty strings; change the condition in the conversation logic to only trigger when the summary is truly missing by using an identity check (i.e., check conversation.summary is None) so workflow_engine.summarize is only called for None summaries; update the conditional around the block that builds old_history, calls workflow_engine.summarize, and invokes conversation_repository.update_summary accordingly.
138-144:func.now()assigns SQL expression to Python attribute.Line 142 assigns
func.now()(a SQL expression object) toconversation.updated_at. While this works for generating the correct SQL UPDATE, the Python object'supdated_atattribute will hold the expression object, not the actual timestamp, until the session is refreshed.If downstream code accesses
conversation.updated_atafter this method returns, it may encounter unexpected behavior.♻️ Alternative using Python datetime
+from datetime import datetime, timezone + async def _touch_conversation(self, conversation_id: UUID) -> None: """Update conversation.updated_at so lifecycle worker can track staleness.""" conversation = await conversation_repository.get_by_id(self._db, conversation_id) if conversation is not None: - conversation.updated_at = func.now() + conversation.updated_at = datetime.now(timezone.utc) self._db.add(conversation) await self._db.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/services/conversation.py` around lines 138 - 144, _touch_conversation currently assigns the SQL expression func.now() to conversation.updated_at which leaves the Python object holding an expression; change it to assign a real Python datetime (e.g. from datetime import datetime; conversation.updated_at = datetime.utcnow() or datetime.now(timezone.utc)) before self._db.add and await self._db.commit(), and add the necessary import; if you need the DB-generated timestamp instead, call await self._db.refresh(conversation) after commit to populate conversation.updated_at with the persisted value.agent/app/integrations/celery/tasks/process_message.py (1)
23-34: Fragile attribute access with multipletype: ignorecomments.The
_resolve_conversation_paramsfunction usestype: ignorecomments to accessconversation.languageandconversation.agent_mode. This suggests theconversationparameter type is too broad (object | None). Consider narrowing the type toConversation | Nonefor better type safety.♻️ Proposed fix
+from app.models.conversation import Conversation + def _resolve_conversation_params( - conversation: object | None, + conversation: Conversation | None, ) -> tuple[Language | None, AgentMode]: if conversation is None: return None, AgentMode.GENERAL - language = Language(conversation.language) if conversation.language else None # type: ignore - agent_mode = ( - AgentMode(conversation.agent_mode) # type: ignore - if conversation.agent_mode # type: ignore - else AgentMode.GENERAL - ) + language = Language(conversation.language) if conversation.language else None + agent_mode = AgentMode(conversation.agent_mode) if conversation.agent_mode else AgentMode.GENERAL return language, agent_mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/integrations/celery/tasks/process_message.py` around lines 23 - 34, The function _resolve_conversation_params uses a too-broad parameter type (object | None) and uses multiple type: ignore to access conversation.language and conversation.agent_mode; change the parameter to a proper Conversation | None type (import or define Conversation) so you can safely read conversation.language and conversation.agent_mode without ignores, then convert them the same way (Language(conversation.language) if conversation.language else None and AgentMode(conversation.agent_mode) if conversation.agent_mode else AgentMode.GENERAL) and remove the type: ignore comments to restore type safety.agent/app/utils/healthcheck.py (2)
15-23: Pool status reports sync engine metrics, not async engine.
get_pool_status()inspectsengine.pool(the synchronous engine), but the/dbendpoint usesAsyncDbSessionbacked byasync_engine. The pool metrics shown won't reflect the async connection pool state.Consider importing and inspecting
async_engine.poolinstead for accurate async pool metrics:-from app.database import AsyncDbSession, engine +from app.database import AsyncDbSession, async_engine def get_pool_status() -> dict[str, str]: """Get connection pool status for monitoring.""" - pool = engine.pool + pool = async_engine.pool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/healthcheck.py` around lines 15 - 23, get_pool_status() currently reads metrics from the synchronous engine.pool (engine), but the app uses AsyncDbSession/async_engine for the /db endpoint so those metrics are inaccurate; update get_pool_status to inspect async_engine.pool instead (import async_engine and use async_engine.pool.size()/checkedin()/checkedout()/overflow()), and keep a safe fallback to engine.pool if async_engine is not available to avoid runtime errors; reference the existing function name get_pool_status and the symbols engine, async_engine, and AsyncDbSession when making the change.
51-59: Blocking SDK calls in async endpoint may cause event loop stalls.The LLM reachability checks use synchronous SDK methods (
client.models.retrieve,genai_get_model) inside an async function. Under load, these blocking calls can stall the FastAPI event loop.Consider either:
- Running blocking calls in a thread pool via
asyncio.to_thread()- Using async SDK methods where available (OpenAI and Anthropic SDKs have async clients)
♻️ Example using asyncio.to_thread for blocking calls
+import asyncio + `@healthcheck_router.get`("/llm") async def llm_health() -> dict[str, str]: """LLM provider health check — sends a minimal request to verify the API is reachable.""" try: vendor, model, api_key = get_llm() match vendor: case "openai": client = openai.OpenAI(api_key=api_key) - client.models.retrieve(model) + await asyncio.to_thread(client.models.retrieve, model) case "google": genai_configure(api_key=api_key) - genai_get_model(f"models/{model}") + await asyncio.to_thread(genai_get_model, f"models/{model}") case _: # anthropic client = anthropic.Anthropic(api_key=api_key) - client.models.retrieve(model) + await asyncio.to_thread(client.models.retrieve, model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/healthcheck.py` around lines 51 - 59, The reachability checks use blocking SDK calls (client.models.retrieve, genai_get_model) inside an async endpoint; update the code to avoid blocking the event loop by either: 1) replacing synchronous clients with async clients for OpenAI and Anthropic and awaiting their async model-retrieve methods (e.g., use openai.AsyncClient/AnthropicAsync or equivalent and await model retrieval), or 2) if async clients aren't available, run the blocking calls in a thread pool using asyncio.to_thread(...) around calls like client.models.retrieve(model) and genai_get_model(f"models/{model}") after genai_configure(api_key=api_key); apply this change to the switch/case branches handling "openai", "google" and the anthropic/default branch so the healthcheck endpoint remains non-blocking.agent/tests/conftest.py (1)
61-83: Consider adding a more specific exception type in the retry loop.The
_connectmethod catches a bareException(line 81), which could mask unexpected errors. Consider catchingpsycopg.OperationalErroror similar connection-specific exceptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tests/conftest.py` around lines 61 - 83, The retry loop in _SyncReadyPostgresContainer._connect currently catches a bare Exception which can hide real bugs; modify the except block to catch the connection-specific exception from psycopg (e.g., psycopg.OperationalError or psycopg.errors.OperationalError) so only transient connection failures are retried, and re-raise any other unexpected exceptions instead of swallowing them; ensure you import psycopg at the top of the method as already done and reference the same _connect method and conn variable when implementing this change.agent/app/utils/auth.py (1)
23-27: Centralize repeated unauthorized response construction.The same
HTTPExceptionshape andWWW-Authenticateheader are duplicated across multiple branches. A small helper + constant would reduce repetition and avoid rebuilding immutable structures repeatedly.Refactor sketch
+_AUTH_HEADERS = {"WWW-Authenticate": "Bearer"} + +def _unauthorized(detail: str = "Could not validate credentials") -> HTTPException: + return HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=detail, + headers=_AUTH_HEADERS, + ) ... - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Could not validate credentials", - headers={"WWW-Authenticate": "Bearer"}, - ) + raise _unauthorized() ... - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Token has expired", - headers={"WWW-Authenticate": "Bearer"}, - ) + raise _unauthorized("Token has expired")Based on learnings: "When reviewing Python code, flag functions that rebuild constant data structures ... recommend moving such structures to module-level or class-level constants."
Also applies to: 31-35, 39-43, 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/app/utils/auth.py` around lines 23 - 27, The code repeats construction of the same HTTPException with identical headers; add a module-level constant like UNAUTHORIZED_HEADERS = {"WWW-Authenticate": "Bearer"} and a small helper function make_unauthorized_exc(detail: str = "Could not validate credentials") -> HTTPException that returns HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=detail, headers=UNAUTHORIZED_HEADERS), then replace all repeated raise HTTPException(...) occurrences (e.g., in the current auth functions that build the exception at lines shown) with: raise make_unauthorized_exc() (or pass a custom detail where needed) so the immutable headers structure is created once and exception creation is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/app/utils/auth.py`:
- Around line 52-59: The code currently converts payload.get("sub") to UUID and
lets UUID(user_id) raise ValueError for malformed IDs; update the logic in the
function that performs this conversion (look for the user_id: str | None =
payload.get("sub") block) to catch ValueError around UUID(user_id) and raise the
same HTTPException used for missing sub
(status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate
credentials", headers={"WWW-Authenticate": "Bearer"}) so malformed UUIDs produce
a 401 instead of a 500.
- Around line 37-43: The expiry check using payload.get("exp") currently allows
tokens exactly at the exp timestamp because it uses `<`; update the condition in
the auth expiry check so that it rejects tokens whose expiration time is
less-than-or-equal-to the current time (use `<=`), i.e. replace the
`datetime.fromtimestamp(exp, tz=timezone.utc) < datetime.now(tz=timezone.utc)`
comparison with a `<=` check; keep the same HTTPException raising logic (status
code, detail, headers) surrounding this check.
In `@agent/tests/conftest.py`:
- Line 117: The test DB setup in conftest.py creates the enum type with the
misspelled name "messagrole"; change the SQL in the await conn.execute(...) call
to create "messagerole" to match the migration's postgresql.ENUM name
("messagerole" as defined in 001_initial_schema.py). Update the CREATE TYPE
statement in conftest.py (and any other test fixtures referencing messagrole) so
the role column can reference the correct enum type.
In `@agent/tests/services/test_conversation_service.py`:
- Around line 204-206: The test patches the entire app.config.settings object
which can break other attribute access in build_history; instead, patch only the
specific attribute so other settings stay intact: use patch.object on the
settings module (targeting the imported settings object used by build_history)
to set history_summary_threshold to 20 for the test, e.g. replace
patch("app.config.settings") with patch.object(settings_module,
"history_summary_threshold", 20) so build_history (the function under test) sees
the patched threshold without losing other attributes.
In `@backend/app/services/summaries_service.py`:
- Around line 404-415: The current broad except in the cursor handling swallows
decode_cursor failures causing malformed/stale cursors to be treated as
first-page requests; remove the blanket "except Exception: pass" and instead
allow decode_cursor errors to propagate (or catch only the specific decode error
type, e.g. ValueError or CursorDecodeError if defined) and re-raise or convert
it to a clear HTTP/BadRequest error so the global handler can return an
appropriate cursor error; update the block around decode_cursor(cursor) (the
code manipulating cursor_dt, cursor_date, direction, and all_dates) to either
not catch exceptions or to catch a narrow exception and raise a descriptive
error.
- Around line 427-432: The next/previous cursor emission should branch on the
requested scan direction instead of only using has_more: update the logic around
next_cursor/previous_cursor (variables next_cursor, previous_cursor, has_more,
cursor, all_dates and the encode_cursor(...) calls) so that when the request
direction is "prev" you still emit a next_cursor (e.g., if all_dates and cursor)
and when the direction is "next" you emit previous_cursor appropriately; in
other words change the next_cursor condition from "if all_dates and has_more" to
a conditional that checks the requested direction (e.g., if direction == "next"
then use has_more, else use cursor) and mirror that for previous_cursor so
encode_cursor(...) is called with the right midnight timestamps and _nil_id in
both scan directions.
- Around line 395-396: The hard-coded 1000 limit on the sleep fetch causes
silent truncation; stop passing the fixed cap and fetch all pages instead:
replace the single call to event_record_repo.get_sleep_summaries(..., None,
1000) with either a call that uses an unlimited/None limit if the repo supports
it, or implement a pagination loop that repeatedly calls
event_record_repo.get_sleep_summaries(db_session, user_id, start_date, end_date,
cursor_or_offset, page_size) aggregating sleep_rows until no more rows are
returned, then pass the full aggregated sleep_rows into
self._filter_by_priority(db_session, user_id, sleep_rows,
date_key="sleep_date").
- Around line 452-460: The RecoverySummary creation is passing avg_resting_hr
(from get_daily_vitals_aggregates) directly into the integer field
resting_heart_rate_bpm, causing fractional values; update the RecoverySummary
construction in the function that builds these summaries to normalize/round/cast
vitals["avg_resting_hr"] to an integer (same approach used in get_body_summary)
before assigning to resting_heart_rate_bpm, while leaving the other vitals
assignments unchanged.
---
Duplicate comments:
In `@agent/app/utils/exceptions.py`:
- Around line 85-92: The handler registered as handle_exception for
RequestValidationError currently unsafely indexes exc.args (err_args =
exc.args[0][0]) which can raise IndexError/TypeError; replace that with a safe
extraction using exc.errors() (e.g., errs = exc.errors(); err_args = errs[0] if
errs else {}), then keep the existing msg/ctx/error extraction and return
HTTPException(status_code=400, detail=detail); update the function _ for
RequestValidationError accordingly to avoid direct exc.args indexing.
---
Nitpick comments:
In `@agent/app/integrations/celery/tasks/process_message.py`:
- Around line 23-34: The function _resolve_conversation_params uses a too-broad
parameter type (object | None) and uses multiple type: ignore to access
conversation.language and conversation.agent_mode; change the parameter to a
proper Conversation | None type (import or define Conversation) so you can
safely read conversation.language and conversation.agent_mode without ignores,
then convert them the same way (Language(conversation.language) if
conversation.language else None and AgentMode(conversation.agent_mode) if
conversation.agent_mode else AgentMode.GENERAL) and remove the type: ignore
comments to restore type safety.
In `@agent/app/services/conversation.py`:
- Around line 164-165: Change the slicing that builds `old` and `recent` in the
conversation handling to use negative indexing for clarity: keep `old` as
messages up to the cutoff and set `recent` using `-threshold` (i.e., `recent =
messages[-threshold:]`) so behaviour is identical when len(messages) ==
threshold but the intent is clearer; update the code around the `old` and
`recent` assignments in conversation.py (the variables `old` and `recent`)
accordingly.
- Around line 167-173: The current check uses a falsy test on
conversation.summary which causes re-summarization for empty strings; change the
condition in the conversation logic to only trigger when the summary is truly
missing by using an identity check (i.e., check conversation.summary is None) so
workflow_engine.summarize is only called for None summaries; update the
conditional around the block that builds old_history, calls
workflow_engine.summarize, and invokes conversation_repository.update_summary
accordingly.
- Around line 138-144: _touch_conversation currently assigns the SQL expression
func.now() to conversation.updated_at which leaves the Python object holding an
expression; change it to assign a real Python datetime (e.g. from datetime
import datetime; conversation.updated_at = datetime.utcnow() or
datetime.now(timezone.utc)) before self._db.add and await self._db.commit(), and
add the necessary import; if you need the DB-generated timestamp instead, call
await self._db.refresh(conversation) after commit to populate
conversation.updated_at with the persisted value.
In `@agent/app/utils/auth.py`:
- Around line 23-27: The code repeats construction of the same HTTPException
with identical headers; add a module-level constant like UNAUTHORIZED_HEADERS =
{"WWW-Authenticate": "Bearer"} and a small helper function
make_unauthorized_exc(detail: str = "Could not validate credentials") ->
HTTPException that returns
HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=detail,
headers=UNAUTHORIZED_HEADERS), then replace all repeated raise
HTTPException(...) occurrences (e.g., in the current auth functions that build
the exception at lines shown) with: raise make_unauthorized_exc() (or pass a
custom detail where needed) so the immutable headers structure is created once
and exception creation is centralized.
In `@agent/app/utils/healthcheck.py`:
- Around line 15-23: get_pool_status() currently reads metrics from the
synchronous engine.pool (engine), but the app uses AsyncDbSession/async_engine
for the /db endpoint so those metrics are inaccurate; update get_pool_status to
inspect async_engine.pool instead (import async_engine and use
async_engine.pool.size()/checkedin()/checkedout()/overflow()), and keep a safe
fallback to engine.pool if async_engine is not available to avoid runtime
errors; reference the existing function name get_pool_status and the symbols
engine, async_engine, and AsyncDbSession when making the change.
- Around line 51-59: The reachability checks use blocking SDK calls
(client.models.retrieve, genai_get_model) inside an async endpoint; update the
code to avoid blocking the event loop by either: 1) replacing synchronous
clients with async clients for OpenAI and Anthropic and awaiting their async
model-retrieve methods (e.g., use openai.AsyncClient/AnthropicAsync or
equivalent and await model retrieval), or 2) if async clients aren't available,
run the blocking calls in a thread pool using asyncio.to_thread(...) around
calls like client.models.retrieve(model) and genai_get_model(f"models/{model}")
after genai_configure(api_key=api_key); apply this change to the switch/case
branches handling "openai", "google" and the anthropic/default branch so the
healthcheck endpoint remains non-blocking.
In `@agent/tests/conftest.py`:
- Around line 61-83: The retry loop in _SyncReadyPostgresContainer._connect
currently catches a bare Exception which can hide real bugs; modify the except
block to catch the connection-specific exception from psycopg (e.g.,
psycopg.OperationalError or psycopg.errors.OperationalError) so only transient
connection failures are retried, and re-raise any other unexpected exceptions
instead of swallowing them; ensure you import psycopg at the top of the method
as already done and reference the same _connect method and conn variable when
implementing this change.
🪄 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: af85face-1613-4dde-9556-effc293ceaca
⛔ Files ignored due to path filters (1)
agent/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
agent/Dockerfileagent/app/agent/engines/router.pyagent/app/agent/prompts/worker_prompts.pyagent/app/database.pyagent/app/integrations/celery/tasks/process_message.pyagent/app/models/chat_session.pyagent/app/services/conversation.pyagent/app/utils/auth.pyagent/app/utils/exceptions.pyagent/app/utils/healthcheck.pyagent/pyproject.tomlagent/tests/conftest.pyagent/tests/services/test_conversation_service.pybackend/app/services/summaries_service.py
✅ Files skipped from review due to trivial changes (2)
- agent/Dockerfile
- agent/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- agent/app/models/chat_session.py
- agent/app/agent/engines/router.py
Comment on lines
+37
to
+43
| exp = payload.get("exp") | ||
| if exp is None or datetime.fromtimestamp(exp, tz=timezone.utc) < datetime.now(tz=timezone.utc): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Token has expired", | ||
| headers={"WWW-Authenticate": "Bearer"}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant section
find . -name "auth.py" -path "*/agent/app/utils/*" | head -5Repository: the-momentum/open-wearables
Length of output: 96
🏁 Script executed:
# Look at the file structure to understand context
cat -n agent/app/utils/auth.py | head -80Repository: the-momentum/open-wearables
Length of output: 2753
🏁 Script executed:
# Get specific lines around 37-43 with more context
sed -n '30,60p' agent/app/utils/auth.pyRepository: the-momentum/open-wearables
Length of output: 1200
🌐 Web query:
PyJWT exp claim handling type format datetime
💡 Result:
PyJWT handles the 'exp' (expiration time) claim as a NumericDate value per RFC 7519, which is a JSON numeric value (integer or float) representing seconds since the Unix epoch (1970-01-01T00:00:00Z UTC), ignoring leap seconds. When encoding with jwt.encode: - Accepts either a UTC UNIX timestamp (int/float) or a datetime object. - If a datetime is provided, converts it to an int timestamp using timegm(datetime.utctimetuple). - Stores as numeric value in the JWT payload. When decoding with jwt.decode: - Expects 'exp' as numeric (int/float); casts to int and validates against current UTC time (datetime.now(tz=timezone.utc).timestamp). - Raises ExpiredSignatureError if exp <= (now - leeway). - Validation only if 'exp' present and verify_exp=True (default True); use require=['exp'] to mandate presence. - Supports leeway (seconds or timedelta) for clock skew. Examples from docs: jwt.encode({"exp": 1371720939}, "secret") jwt.encode({"exp": datetime.now(tz=timezone.utc)}, "secret") Always use UTC datetime/timestamps to match internal UTC comparisons.
Citations:
- 1: https://pyjwt.readthedocs.io/en/latest/usage.html
- 2: https://pyjwt.readthedocs.io/en/latest/api.html
- 3: https://pyjwt.readthedocs.io/en/stable/api.html
- 4: https://github.com/jpadilla/pyjwt/blob/master/jwt/api_jwt.py
🏁 Script executed:
# Test Python datetime.fromtimestamp() behavior with invalid inputs
python3 << 'EOF'
from datetime import datetime, timezone
# Test 1: Valid numeric exp
try:
result = datetime.fromtimestamp(1704067200, tz=timezone.utc)
print(f"Valid int exp: SUCCESS - {result}")
except Exception as e:
print(f"Valid int exp: FAILED - {type(e).__name__}: {e}")
# Test 2: Valid float exp
try:
result = datetime.fromtimestamp(1704067200.5, tz=timezone.utc)
print(f"Valid float exp: SUCCESS - {result}")
except Exception as e:
print(f"Valid float exp: FAILED - {type(e).__name__}: {e}")
# Test 3: String exp (common case from API/JSON)
try:
result = datetime.fromtimestamp("1704067200", tz=timezone.utc)
print(f"String exp: SUCCESS - {result}")
except Exception as e:
print(f"String exp: FAILED - {type(e).__name__}: {e}")
# Test 4: None exp
try:
result = datetime.fromtimestamp(None, tz=timezone.utc)
print(f"None exp: SUCCESS - {result}")
except Exception as e:
print(f"None exp: FAILED - {type(e).__name__}: {e}")
# Test 5: Malformed string
try:
result = datetime.fromtimestamp("abc", tz=timezone.utc)
print(f"Malformed string exp: SUCCESS - {result}")
except Exception as e:
print(f"Malformed string exp: FAILED - {type(e).__name__}: {e}")
# Test 6: Invalid type (dict/list)
try:
result = datetime.fromtimestamp({"ts": 1234}, tz=timezone.utc)
print(f"Dict exp: SUCCESS - {result}")
except Exception as e:
print(f"Dict exp: FAILED - {type(e).__name__}: {e}")
# Test 7: Out of range value
try:
result = datetime.fromtimestamp(999999999999999999, tz=timezone.utc)
print(f"Out of range exp: SUCCESS - {result}")
except Exception as e:
print(f"Out of range exp: FAILED - {type(e).__name__}: {e}")
EOFRepository: the-momentum/open-wearables
Length of output: 602
🏁 Script executed:
# Check PyJWT behavior - what does jwt.decode return for exp?
python3 << 'EOF'
import jwt
import json
from datetime import datetime, timedelta, timezone
# Create a test token with exp as integer (standard JWT format)
secret = "test_secret"
payload = {
"sub": "user123",
"exp": int((datetime.now(tz=timezone.utc) + timedelta(hours=1)).timestamp()),
"iat": int(datetime.now(tz=timezone.utc).timestamp())
}
token = jwt.encode(payload, secret, algorithm="HS256")
print(f"Test token created")
# Decode with verify_exp=False
decoded = jwt.decode(token, secret, algorithms=["HS256"], options={"verify_exp": False})
print(f"Decoded payload: {decoded}")
print(f"Type of exp: {type(decoded['exp'])}")
print(f"Value of exp: {decoded['exp']}")
# Verify that exp is indeed a numeric type (int or float)
print(f"exp is int: {isinstance(decoded['exp'], int)}")
print(f"exp is float: {isinstance(decoded['exp'], float)}")
EOFRepository: the-momentum/open-wearables
Length of output: 756
Use <= for expiry boundary check to comply with RFC 7519.
Line 38 uses < which accepts tokens at exactly the expiry timestamp. Per RFC 7519, tokens must be rejected on or after the exp time, so the comparison should be <=.
Suggested fix
exp = payload.get("exp")
- if exp is None or datetime.fromtimestamp(exp, tz=timezone.utc) < datetime.now(tz=timezone.utc):
+ if exp is None or datetime.fromtimestamp(exp, tz=timezone.utc) <= datetime.now(tz=timezone.utc):
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Token has expired",
headers={"WWW-Authenticate": "Bearer"},
)📝 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
| exp = payload.get("exp") | |
| if exp is None or datetime.fromtimestamp(exp, tz=timezone.utc) < datetime.now(tz=timezone.utc): | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Token has expired", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) | |
| exp = payload.get("exp") | |
| if exp is None or datetime.fromtimestamp(exp, tz=timezone.utc) <= datetime.now(tz=timezone.utc): | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Token has expired", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/utils/auth.py` around lines 37 - 43, The expiry check using
payload.get("exp") currently allows tokens exactly at the exp timestamp because
it uses `<`; update the condition in the auth expiry check so that it rejects
tokens whose expiration time is less-than-or-equal-to the current time (use
`<=`), i.e. replace the `datetime.fromtimestamp(exp, tz=timezone.utc) <
datetime.now(tz=timezone.utc)` comparison with a `<=` check; keep the same
HTTPException raising logic (status code, detail, headers) surrounding this
check.
Comment on lines
+52
to
+59
| user_id: str | None = payload.get("sub") | ||
| if user_id is None: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Could not validate credentials", | ||
| headers={"WWW-Authenticate": "Bearer"}, | ||
| ) | ||
| return UUID(user_id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its structure
find . -type f -name "auth.py" | head -20Repository: the-momentum/open-wearables
Length of output: 190
🏁 Script executed:
# Get line count to understand file size
wc -l agent/app/utils/auth.pyRepository: the-momentum/open-wearables
Length of output: 97
🏁 Script executed:
cat -n agent/app/utils/auth.pyRepository: the-momentum/open-wearables
Length of output: 2753
Handle invalid sub UUID format explicitly.
Line 59 can raise ValueError for malformed sub claims, which currently bubbles as a 500 error instead of a 401 auth failure. This should be caught and mapped to the same 401 path as other validation failures.
Suggested fix
- return UUID(user_id)
+ try:
+ return UUID(user_id)
+ except (ValueError, TypeError):
+ raise HTTPException(
+ status_code=status.HTTP_401_UNAUTHORIZED,
+ detail="Could not validate credentials",
+ headers={"WWW-Authenticate": "Bearer"},
+ )📝 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
| user_id: str | None = payload.get("sub") | |
| if user_id is None: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Could not validate credentials", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) | |
| return UUID(user_id) | |
| user_id: str | None = payload.get("sub") | |
| if user_id is None: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Could not validate credentials", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) | |
| try: | |
| return UUID(user_id) | |
| except (ValueError, TypeError): | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Could not validate credentials", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/app/utils/auth.py` around lines 52 - 59, The code currently converts
payload.get("sub") to UUID and lets UUID(user_id) raise ValueError for malformed
IDs; update the logic in the function that performs this conversion (look for
the user_id: str | None = payload.get("sub") block) to catch ValueError around
UUID(user_id) and raise the same HTTPException used for missing sub
(status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate
credentials", headers={"WWW-Authenticate": "Bearer"}) so malformed UUIDs produce
a 401 instead of a 500.
| async with async_engine.begin() as conn: | ||
| # MessageRole uses create_type=False so metadata.create_all won't create the PG enum; | ||
| # we must create it explicitly before creating the tables. | ||
| await conn.execute(text("CREATE TYPE IF NOT EXISTS messagrole AS ENUM ('user', 'assistant')")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the MessageRole enum is used in the model to determine expected PG type name
rg -n 'Enum.*MessageRole|messagerole|messagrole' agent/app/models/ -iRepository: the-momentum/open-wearables
Length of output: 346
🏁 Script executed:
# Find the MessageRole enum definition to understand its actual usage
rg -n 'class MessageRole|MessageRole.*=' agent/app/schemas/ -A 2Repository: the-momentum/open-wearables
Length of output: 234
🏁 Script executed:
# Search for any explicit type_name in Enum configuration
rg -n 'type_name|Enum' agent/app/models/message.py -B 2 -A 2Repository: the-momentum/open-wearables
Length of output: 499
🏁 Script executed:
# Check if there are any database dialect-specific type definitions
rg -n 'TypeDecorator|impl|TypeEngine' agent/app/models/ -B 1 -A 1Repository: the-momentum/open-wearables
Length of output: 53
🏁 Script executed:
# Check if there are any other CREATE TYPE statements or enum configurations
rg -n 'CREATE TYPE|type_name|Enum\(' agent/ -B 1 -A 1Repository: the-momentum/open-wearables
Length of output: 1544
🏁 Script executed:
# Check if there's any explicit dialect configuration or type mapping
rg -n 'type_name=|dialect|postgresql' agent/app/models/ -B 1 -A 1Repository: the-momentum/open-wearables
Length of output: 888
🏁 Script executed:
# Check what conftest creates for ConversationStatus to see the pattern
rg -n 'conversationstatus|ConversationStatus' agent/tests/conftest.py -B 2 -A 2Repository: the-momentum/open-wearables
Length of output: 53
🏁 Script executed:
# Look at the full migration file to see if there's any explicit type_name configuration
cat -n agent/migrations/versions/001_initial_schema.py | head -50Repository: the-momentum/open-wearables
Length of output: 2160
🏁 Script executed:
# Check conftest.py for any ConversationStatus enum creation
cat -n agent/tests/conftest.py | grep -A 5 -B 5 'conversationstatus\|ConversationStatus\|CREATE TYPE'Repository: the-momentum/open-wearables
Length of output: 731
Fix enum type name mismatch: messagrole should be messagerole.
The PostgreSQL enum type created in conftest.py is messagrole, but the actual migration file explicitly defines it as messagerole (with an 'e'). This mismatch will cause the message table creation to fail during tests when the role column references a non-existent type.
Details
The migration file (agent/migrations/versions/001_initial_schema.py line 21) explicitly defines:
messagerole = postgresql.ENUM("user", "assistant", name="messagerole", create_type=False)This must be matched in conftest.py for test database setup.
Proposed fix
- await conn.execute(text("CREATE TYPE IF NOT EXISTS messagrole AS ENUM ('user', 'assistant')"))
+ await conn.execute(text("CREATE TYPE IF NOT EXISTS messagerole AS ENUM ('user', 'assistant')"))
await conn.run_sync(BaseDbModel.metadata.create_all)
yield
async with async_engine.begin() as conn:
await conn.run_sync(BaseDbModel.metadata.drop_all)
- await conn.execute(text("DROP TYPE IF EXISTS messagrole"))
+ await conn.execute(text("DROP TYPE IF EXISTS messagerole"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/tests/conftest.py` at line 117, The test DB setup in conftest.py
creates the enum type with the misspelled name "messagrole"; change the SQL in
the await conn.execute(...) call to create "messagerole" to match the
migration's postgresql.ENUM name ("messagerole" as defined in
001_initial_schema.py). Update the CREATE TYPE statement in conftest.py (and any
other test fixtures referencing messagrole) so the role column can reference the
correct enum type.
Comment on lines
+204
to
+206
| with patch("app.config.settings") as mock_settings: | ||
| mock_settings.history_summary_threshold = 20 | ||
| history = await service.build_history(conv) |
There was a problem hiding this comment.
Settings patch may not affect build_history due to import timing.
The build_history method imports settings lazily inside the function (from app.config import settings), so patching app.config.settings should work. However, the mock replaces the entire settings object but only sets history_summary_threshold. Other attributes accessed by build_history would raise AttributeError.
Consider using patch.object to patch only the specific attribute:
🛠️ Proposed fix
- with patch("app.config.settings") as mock_settings:
- mock_settings.history_summary_threshold = 20
+ with patch("app.config.settings.history_summary_threshold", 20):
history = await service.build_history(conv)📝 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
| with patch("app.config.settings") as mock_settings: | |
| mock_settings.history_summary_threshold = 20 | |
| history = await service.build_history(conv) | |
| with patch("app.config.settings.history_summary_threshold", 20): | |
| history = await service.build_history(conv) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent/tests/services/test_conversation_service.py` around lines 204 - 206,
The test patches the entire app.config.settings object which can break other
attribute access in build_history; instead, patch only the specific attribute so
other settings stay intact: use patch.object on the settings module (targeting
the imported settings object used by build_history) to set
history_summary_threshold to 20 for the test, e.g. replace
patch("app.config.settings") with patch.object(settings_module,
"history_summary_threshold", 20) so build_history (the function under test) sees
the patched threshold without losing other attributes.
Comment on lines
+395
to
+396
| sleep_rows = self.event_record_repo.get_sleep_summaries(db_session, user_id, start_date, end_date, None, 1000) | ||
| sleep_rows = self._filter_by_priority(db_session, user_id, sleep_rows, date_key="sleep_date") |
There was a problem hiding this comment.
Remove the hard 1000-row cap from sleep aggregation.
Lines 395-396 will silently drop sleep metrics for long ranges because get_sleep_summaries() is still paginated and grouped by (sleep_date, source, device_model). Multi-provider histories can exceed 1000 rows well before 1000 days, so recovery summaries can start returning vitals-only dates with missing sleep fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/summaries_service.py` around lines 395 - 396, The
hard-coded 1000 limit on the sleep fetch causes silent truncation; stop passing
the fixed cap and fetch all pages instead: replace the single call to
event_record_repo.get_sleep_summaries(..., None, 1000) with either a call that
uses an unlimited/None limit if the repo supports it, or implement a pagination
loop that repeatedly calls event_record_repo.get_sleep_summaries(db_session,
user_id, start_date, end_date, cursor_or_offset, page_size) aggregating
sleep_rows until no more rows are returned, then pass the full aggregated
sleep_rows into self._filter_by_priority(db_session, user_id, sleep_rows,
date_key="sleep_date").
Comment on lines
+404
to
+415
| if cursor: | ||
| try: | ||
| cursor_dt, _, direction = decode_cursor(cursor) | ||
| cursor_date = cursor_dt.date() | ||
| if direction == "prev": | ||
| all_dates = [d for d in all_dates if d < cursor_date] | ||
| all_dates = list(reversed(all_dates)) | ||
| prev_page = True | ||
| else: | ||
| all_dates = [d for d in all_dates if d > cursor_date] | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Don’t swallow cursor decode failures.
Lines 404-415 catch every decode error and fall back to the full date set. A malformed or stale cursor then looks like a valid first-page request, which will duplicate data instead of returning a proper cursor error through the global handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/summaries_service.py` around lines 404 - 415, The
current broad except in the cursor handling swallows decode_cursor failures
causing malformed/stale cursors to be treated as first-page requests; remove the
blanket "except Exception: pass" and instead allow decode_cursor errors to
propagate (or catch only the specific decode error type, e.g. ValueError or
CursorDecodeError if defined) and re-raise or convert it to a clear
HTTP/BadRequest error so the global handler can return an appropriate cursor
error; update the block around decode_cursor(cursor) (the code manipulating
cursor_dt, cursor_date, direction, and all_dates) to either not catch exceptions
or to catch a narrow exception and raise a descriptive error.
Comment on lines
+427
to
+432
| if all_dates and has_more: | ||
| last_midnight = datetime.combine(all_dates[-1], datetime.min.time()).replace(tzinfo=timezone.utc) | ||
| next_cursor = encode_cursor(last_midnight, _nil_id, "next") | ||
| if all_dates and cursor: | ||
| first_midnight = datetime.combine(all_dates[0], datetime.min.time()).replace(tzinfo=timezone.utc) | ||
| previous_cursor = encode_cursor(first_midnight, _nil_id, "prev") |
There was a problem hiding this comment.
Backward pages still lose their forward link.
Lines 427-432 only emit next_cursor when has_more is true for the current scan direction. For a "prev" request that lands on the earliest page, next_cursor becomes None even though there is a later page to navigate back to. Cursor emission here needs to branch on the requested direction, not just on has_more.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/summaries_service.py` around lines 427 - 432, The
next/previous cursor emission should branch on the requested scan direction
instead of only using has_more: update the logic around
next_cursor/previous_cursor (variables next_cursor, previous_cursor, has_more,
cursor, all_dates and the encode_cursor(...) calls) so that when the request
direction is "prev" you still emit a next_cursor (e.g., if all_dates and cursor)
and when the direction is "next" you emit previous_cursor appropriately; in
other words change the next_cursor condition from "if all_dates and has_more" to
a conditional that checks the requested direction (e.g., if direction == "next"
then use has_more, else use cursor) and mirror that for previous_cursor so
encode_cursor(...) is called with the right midnight timestamps and _nil_id in
both scan directions.
Comment on lines
+452
to
+460
| data.append( | ||
| RecoverySummary( | ||
| date=d, | ||
| source=source, | ||
| sleep_duration_seconds=sleep_duration, | ||
| sleep_efficiency_percent=efficiency, | ||
| resting_heart_rate_bpm=vitals["avg_resting_hr"] if vitals else None, | ||
| avg_hrv_sdnn_ms=vitals["avg_hrv_sdnn"] if vitals else None, | ||
| avg_spo2_percent=vitals["avg_spo2"] if vitals else None, |
There was a problem hiding this comment.
Normalize averaged resting HR to integer BPM.
Lines 458-460 pass avg_resting_hr straight from get_daily_vitals_aggregates(), which is an SQL average and will commonly be fractional. RecoverySummary.resting_heart_rate_bpm is an integer field, so this should be rounded/cast here the same way get_body_summary() does.
Suggested fix
for d in all_dates:
vitals = vitals_by_date.get(d)
sleep = sleep_by_date.get(d)
+ resting_hr_avg = vitals.get("avg_resting_hr") if vitals else None
# Determine source: prefer vitals source (more signal), fallback to sleep
if vitals:
source = SourceMetadata(provider=vitals["source"] or "unknown", device=vitals.get("device_model"))
@@
RecoverySummary(
date=d,
source=source,
sleep_duration_seconds=sleep_duration,
sleep_efficiency_percent=efficiency,
- resting_heart_rate_bpm=vitals["avg_resting_hr"] if vitals else None,
+ resting_heart_rate_bpm=int(round(resting_hr_avg)) if resting_hr_avg is not None else None,
avg_hrv_sdnn_ms=vitals["avg_hrv_sdnn"] if vitals else None,
avg_spo2_percent=vitals["avg_spo2"] if vitals else None,
recovery_score=None,
)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/summaries_service.py` around lines 452 - 460, The
RecoverySummary creation is passing avg_resting_hr (from
get_daily_vitals_aggregates) directly into the integer field
resting_heart_rate_bpm, causing fractional values; update the RecoverySummary
construction in the function that builds these summaries to normalize/round/cast
vitals["avg_resting_hr"] to an integer (same approach used in get_body_summary)
before assigning to resting_heart_rate_bpm, while leaving the other vitals
assignments unchanged.
…he-momentum#765 Agent: - Fix mappings_meta._handle_back_populates to preserve cascade/passive_deletes options when patching back_populates on auto-generated relationships - Add passive_deletes=True to Session.messages relationship - Fix JWT exp comparison to use <= per RFC 7519 - Catch malformed sub UUID claim and return 401 instead of 500 - Fix enum type name: messagrole → messagerole to match migration - Fix settings patch in build_history test to use patch.object Backend (summaries_service): - Remove hardcoded 1000-row cap from sleep fetch in get_recovery_summaries; update event_record_repository.get_sleep_summaries to accept limit=None - Let cursor decode errors propagate instead of silently falling back to first-page results on malformed cursors - Fix backward-page cursor emission: always emit next_cursor on prev pages, only emit previous_cursor when has_more in the backward direction - Round avg_resting_hr float to int for resting_heart_rate_bpm field Backend (user_repository): - Eagerly load personal_record via selectinload in get_users_with_filters to prevent N+1 queries when serialising UserRead
FBegiello
added a commit
to Zawiszowski/open-wearables
that referenced
this pull request
Apr 15, 2026
…tum#765 - Fix unsafe exc.args[0][0] indexing in RequestValidationError handler; use exc.errors() instead - Replace func.now() with datetime.now(timezone.utc) in _touch_conversation so the Python object holds a real timestamp - Use negative indexing (messages[-threshold:]) for clarity in build_history - Use `is None` instead of falsy check on conversation.summary to avoid re-summarizing on empty strings - Narrow _resolve_conversation_params type from object|None to Conversation|None and remove all type: ignore comments - Switch healthcheck pool metrics from sync engine to async_engine - Wrap blocking SDK calls in asyncio.to_thread() in llm_health endpoint - Catch psycopg.OperationalError instead of bare Exception in container retry loop - Extract _unauthorized() helper and _AUTH_HEADERS constant in auth.py to eliminate repeated HTTPException construction
FBegiello
added a commit
that referenced
this pull request
Apr 17, 2026
* feat(agent): bootstrap FastAPI agent service
- FastAPI app with /session and /chat endpoints, JWT auth
- Celery worker, beat and Flower setup with Redis broker
- SQLAlchemy async models, Alembic migrations scaffold
- ChatSession expiration periodic task (every 5 min, 1h TTL)
- Docker Compose services: agent, celery-worker, celery-beat
- Process message Celery task with WorkflowAgent placeholder
* refactor(agent): align data model with spec
Invert the Conversation/Session ownership hierarchy to match the spec:
Conversation is now the top-level entity (with user_id + status enum),
Session has a FK to Conversation (many sessions per conversation), and
Message gains dual ownership via both conversation_id (primary) and
session_id (secondary, SET NULL on session delete).
Introduce ConversationStatus enum (active/inactive/closed) and rename
ChatSession model/repository to Session. Replace ChatSessionService
with ConversationService which owns upsert, deactivate_session,
get_active, save_messages, touch, and build_history (stub — wired in
later commits).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(agent): add initial alembic migration
Create the initial database schema for conversations, sessions, and
messages tables. Conversations are top-level (user_id + status enum),
sessions belong to conversations, and messages have dual ownership via
conversation_id (CASCADE) and session_id (SET NULL). PostgreSQL native
ENUMs are created for conversationstatus and messagerole.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(agent): add pygentic-ai with multi-provider LLM support
Add pygentic-ai (git dependency), anthropic, openai, google-generativeai,
and pydantic-ai to project dependencies.
Extend settings with LLM provider config (LLM_PROVIDER, LLM_MODEL,
LLM_MODEL_WORKERS, per-provider API keys), OW backend integration
(OW_API_URL, OW_API_KEY), and conversation lifecycle params
(SESSION_TIMEOUT_MINUTES, CONVERSATION_CLOSE_HOURS,
HISTORY_SUMMARY_THRESHOLD, MAX_RETRIES).
Add model_utils.get_llm() factory that returns the (vendor, model,
api_key) tuple based on the configured provider, with startup validation
via validate_llm_config() called in main.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(backend): implement recovery endpoint and extend user profile
- Add DataPointSeriesRepository.get_daily_vitals_aggregates() returning
per-day averages of resting HR, HRV SDNN, and SpO2 grouped by source/device
- Implement SummariesService.get_recovery_summaries() merging sleep
efficiency/duration with vitals into PaginatedResponse[RecoverySummary]
- Wire GET /users/{id}/summaries/recovery route to service (was HTTP 501)
- Extend UserRead schema with birth_date, sex, gender sourced from
personal_record via a model_validator that flattens the relationship
* feat(agent): implement OW backend REST client
Singleton OWClient using httpx.AsyncClient (one per call) with X-API-Key
auth. Covers: user profile, body summary, timeseries, activity/sleep/
recovery summaries, workout and sleep events.
* feat(agent): implement agent toolsets, prompts, and workflow engine
Toolsets (tools/):
- ow_tools.py: 8 async tool functions covering user profile, body composition,
activity, sleep, recovery, workouts, sleep events, HR timeseries
- date_tools.py: get_today_date, get_current_week utilities
- tool_registry.py: ToolManager mapping AgentMode to Toolpack lists
Prompts (prompts/):
- agent_prompts.py: GENERAL_SYSTEM_PROMPT built from composable text blocks
(priming + ReAct guidance + health ruleset + data capabilities)
- worker_prompts.py: router and guardrails system prompts
Engines (engines/):
- reasoning.py: builds a pydantic-ai Agent with mode-specific prompt and tools
- router.py: lightweight classifier returning RouterDecision(route, reasoning)
- guardrails.py: formatting pass using the worker model
Workflow (workflows/agent_workflow.py):
- WorkflowEngine.run(): ROUTER → REASONING → GUARDRAILS pipeline
- WorkflowEngine.summarize(): history compression using worker model
- workflow_engine singleton used by Celery task
Static (static/):
- default_msgs.py: fallback error and refusal message strings
* feat(agent): wire workflow engine into process_message Celery task
Replaces the stub echo with the full pipeline:
1. Load conversation + session from DB
2. build_history() for LLM context (with summarization if over threshold)
3. workflow_engine.run() → router → reasoning → guardrails
4. save_messages() to persist user + assistant turns and touch conversation
5. POST result to callback_url; log warning on HTTP failure (no retry)
Falls back to WORKFLOW_ERROR_MSG on any unhandled workflow exception.
* feat(agent): implement conversation lifecycle Celery worker
Replaces expire-sessions beat task with manage-conversation-lifecycle
running every 5 minutes:
1. Deactivate sessions idle > SESSION_TIMEOUT_MINUTES
2. Mark ACTIVE conversations as INACTIVE when updated_at is stale
3. Close INACTIVE conversations idle > CONVERSATION_CLOSE_HOURS
All thresholds read from settings so they are configurable via env vars.
* chore: add .gitattributes for LF line endings and update scripts
* test(agent): add pytest test suite covering all new features
Structure mirrors the backend test suite (testcontainers + factory-boy,
per-test savepoint rollback, autouse mocks for LLM/Celery/OW client):
tests/conftest.py
- PostgreSQL via testcontainers (or TEST_DATABASE_URL env var)
- Async session with savepoint rollback per test
- FastAPI TestClient with async DB dependency override
- JWT auth token fixture
- Autouse mocks: Celery tasks, pydantic-ai Agent, OW backend client
tests/factories.py
- ConversationFactory, SessionFactory, MessageFactory
tests/repositories/
- test_conversation_repository: create, get, update status/summary,
mark_inactive_stale, close_stale
- test_session_repository: create, get, deactivate, increment_request_count,
deactivate_expired
- test_message_repository: create, get_by_conversation_id, limit, isolation
tests/services/
- test_conversation_service: upsert (all paths), get_active (ownership/status
checks), save_messages, build_history (threshold + summary injection)
tests/api/v1/
- test_session: POST /session (create/reuse), PATCH /session/{id} (deactivate)
- test_chat: POST /chat/{id} (queuing, validation, 404/410/401 guards)
tests/agent/
- test_tools: date tools correctness; OW tool calls, error handling, clamping
- test_workflow: full pipeline (answer path, refusal, router failure fallback,
guardrails failure fallback, reasoning propagation, history passing)
- test_tool_registry: mode→toolpack mapping, callability
tests/tasks/
- test_conversation_lifecycle: session expiry, inactive marking, close logic,
no-op on fresh data
pyproject.toml: added factory-boy, testcontainers[postgres] to dev deps;
added [tool.pytest.ini_options] with asyncio_mode=auto.
* chore(agent): Language coding in line with ISO 629 codes and minor cleanup
* fix(agent): Prompt rework and general housekeeping
* chore(agent): Add language handling for default messages
* chore(agent): Change llm handling to cover defaults for each provider and harden provider selection
* chore(agent): Add healthcheck for LLM providers
* chore(agent): Unified naming convention across the project with top to bottom conversation->session->message
* fix(agent): Fix migrations and dependencies for pygentic-ai
* fix(agent): Fix default llm falbacks
* chore(agent): Fix all ruff and ty violations, remove dead ChatSessionService
- Delete app/services/chat_session.py — dead code left over from the
conversation->session->message rename; ConversationService covers all
the same surface area
- Add explicit None-guard in model_utils.get_llm() so ty narrows
effective_model from str|None to str
- Cast run_result.output to str in WorkflowEngine.run() to satisfy
the declared return type
- Suppress library-level false positives (SQLAlchemy Pool methods,
CursorResult.rowcount, Celery Proxy, httpx params, declared_attr,
generic decorator bounds) with bare # type: ignore
- Add # type: ignore on repository create() overrides whose signatures
intentionally differ from the generic base
- Fix all ruff violations in tests: missing ANN001/ANN201 annotations,
SIM117 nested-with, RET504 unnecessary assignments, import ordering
- Add [tool.ty.src] exclude = ["tests"] to pyproject.toml — factory-boy
metaclass magic is opaque to ty; tests lint cleanly via ruff
- Reformat backend files touched by pre-commit ruff format
* test(agent): refactor agent test suite for pydantic-ai workflow
* fix(agent): convert fixtures and tests to async and fix validation error handling
* chore(agent): remove unused celery tasks: dummy_task and expire_sessions
* fix(agent): rename auth header to X-Open-Wearables-API-Key in OWClient
* chore(agent): delete unused filter and error schemas
* refactor(agent): align exception handling with backend pattern
* fix(agent): include assistant messages in chat history seed
Previously _build_history() only converted user turns to ModelRequest
objects and silently dropped assistant messages. This meant the agent
had no context of its previous responses each turn.
Now assistant messages are also converted to ModelResponse(TextPart)
objects, giving the agent the full alternating history via message_history.
* fix(agent): replace manual ReAct prompt with pydantic-ai-compatible guidance
The old TEXT_REACTAGENT_PATTERN was a LlamaIndex/LangChain-style text-parsing
ReAct prompt (Thought/Action/Action Input/Observation) incompatible with
pydantic-ai's native function-calling tool loop.
Problems fixed:
- {tool_desc} and {tool_names} placeholders were never substituted, appearing
as literal strings in the system prompt
- Stale tool prefix references from a previous project
- Redundant language instructions already handled by BaseAgent's
@agent.instructions decorator
- Manual ReAct format instructions that conflict with pydantic-ai's execution
Replaced with plain guidance that works with pydantic-ai's native tool use:
the model calls tools via function-calling API, pydantic-ai handles the
multi-turn loop internally — no text-format orchestration needed.
* fix(agent): cap tool calls per turn via UsageLimits from config
Added max_tool_calls setting (default 10) and wired it to UsageLimits
on HealthReasoningAgent to prevent runaway tool-call loops.
Removed dead max_retries config field that was never used.
* fix(agent): inject user_id via RunContext deps instead of message text
Previously user_id was prepended to the user message as [user_id=...] and
the model was expected to parse and pass it correctly to every tool call —
fragile and leaking an internal ID into the conversation.
- Add HealthAgentDeps(BaseAgentDeps) with user_id field (app/agent/deps.py)
- Rewrite all OW tools to take RunContext[HealthAgentDeps] as first arg;
user_id is read from ctx.deps, removed from model-visible parameters
- HealthReasoningAgent stores user_id on the instance, passes deps_type,
and overrides generate_response to inject HealthAgentDeps per run
- WorkflowEngine passes user_id to the agent constructor, drops augmented_message
- Remove dead get_guardrails_refusal_msg and its backing dict
- Restore max_retries to config (used by Celery @shared_task decorator)
* fix(router): pass chat history to router for contextual follow-up classification
ClassifyNode only forwards the current message to router.route(), so vague
follow-ups like "What can I do to improve it?" were classified as off-topic
without any conversation context.
HealthRouter now accepts history at construction time and overrides route()
to prepend the last 3 conversation turns as a context block, letting the LLM
recognise health-related follow-ups correctly.
* fix(router): emit short refusals in the configured conversation language
The router's raw reasoning was embedded verbatim in the refusal template,
leaking verbose English chain-of-thought even for non-English conversations.
- Pass conversation language to HealthRouter so BaseAgent's dynamic
instruction ("respond in X language") applies to the reasoning field
- Instruct the router prompt to cap the reasoning at 2 sentences and
omit internal classification logic
- Extract _msg_to_line() helper to keep _build_context() under complexity limit
* fix(agent): address CodeRabbit review issues from PR #765
Security:
- Replace unmaintained python-jose + unused passlib with PyJWT
- Remove raw DB/exception details from HTTP error responses (log server-side)
- Pin uv to 0.11.6 in Dockerfile instead of :latest
- Add non-root appuser to Dockerfile runtime stage
Correctness:
- Remove delete-orphan cascade from Session.messages (messages belong to conversations, not sessions)
- Fix build_history to use self._db consistently; drop separate db param and update callers
- Guard id_str against unsaved (transient) model instances
- Fix if entity_id: → if entity_id is not None: in ResourceNotFoundError
- Fix backward pagination order in summaries_service (re-reverse after prev-direction slice)
- Create messagrole PG enum type before metadata.create_all in test conftest
Quality:
- Add -> None return annotations to all exception __init__ methods
- Add **kwargs: Any annotation to HealthRouter.route
- Add soft_word_limit > 0 validation in build_worker_prompt
- Switch healthcheck /db endpoint to AsyncDbSession + await db.execute
- Add logger.warning when session is None in save_messages
* fix(agent): address second batch of CodeRabbit review issues from PR #765
Agent:
- Fix mappings_meta._handle_back_populates to preserve cascade/passive_deletes
options when patching back_populates on auto-generated relationships
- Add passive_deletes=True to Session.messages relationship
- Fix JWT exp comparison to use <= per RFC 7519
- Catch malformed sub UUID claim and return 401 instead of 500
- Fix enum type name: messagrole → messagerole to match migration
- Fix settings patch in build_history test to use patch.object
Backend (summaries_service):
- Remove hardcoded 1000-row cap from sleep fetch in get_recovery_summaries;
update event_record_repository.get_sleep_summaries to accept limit=None
- Let cursor decode errors propagate instead of silently falling back to
first-page results on malformed cursors
- Fix backward-page cursor emission: always emit next_cursor on prev pages,
only emit previous_cursor when has_more in the backward direction
- Round avg_resting_hr float to int for resting_heart_rate_bpm field
Backend (user_repository):
- Eagerly load personal_record via selectinload in get_users_with_filters
to prevent N+1 queries when serialising UserRead
* fix(agent): final sweep of CodeRabbit review issues from PR #765
- Fix unsafe exc.args[0][0] indexing in RequestValidationError handler;
use exc.errors() instead
- Replace func.now() with datetime.now(timezone.utc) in _touch_conversation
so the Python object holds a real timestamp
- Use negative indexing (messages[-threshold:]) for clarity in build_history
- Use `is None` instead of falsy check on conversation.summary to avoid
re-summarizing on empty strings
- Narrow _resolve_conversation_params type from object|None to Conversation|None
and remove all type: ignore comments
- Switch healthcheck pool metrics from sync engine to async_engine
- Wrap blocking SDK calls in asyncio.to_thread() in llm_health endpoint
- Catch psycopg.OperationalError instead of bare Exception in container retry loop
- Extract _unauthorized() helper and _AUTH_HEADERS constant in auth.py
to eliminate repeated HTTPException construction
* chore(agent): squash migrations into one clean alembic-generated revision
Replace hand-crafted 001_initial_schema with a single autogenerated
migration (8d7d4c1a725d) produced by alembic against a clean database.
Also fix pre-existing ty errors surfaced by the hook run: wrong tuple
annotation in mappings_meta, UUID | None narrowing in ow_tools, and
router.py method signature mismatch with GenericRouter.route.
* fix(agent): create home directory for appuser to allow uv cache initialization
* fix(agent): drop enum types in migration downgrade; remove dead setup.cfg
- Add DROP TYPE for messagerole and conversationstatus in migration downgrade
to match the behaviour of the old 001_initial_schema migration it replaces
- Remove setup.cfg (flake8/mypy/isort config with no consumers — project uses
Ruff and ty)
* feat(agent): add statistical analysis toolpack for the reasoning agent
Adds stats_tools.py with mean, stdev, linear trend, and autocorrelation-based
seasonality detection — all pure stdlib, no new dependencies. Registers the
STATS toolpack in the tool registry under AgentMode.GENERAL so the agent can
chain OW data-retrieval tools with statistical computations in a single turn.
* docs: add agent architecture developer guide
Comprehensive Mintlify MDX guide covering the full agent codebase:
three-engine pipeline, conversation lifecycle, API reference, all 14 tools
across three toolpacks, multi-provider LLM config, data models, test suite
coverage, extension patterns, and troubleshooting.
* fix(agent): address CodeRabbit review comments
- Replace traceback.format_exc() in OW tools with logger.exception() +
generic user-facing messages to prevent internal path/stack leaks to the LLM
- Add SSRF validator on ChatRequest.callback_url rejecting private/loopback
IP addresses
- Fix exception handler in main.py to return JSONResponse instead of raising
- Disable allow_credentials when cors_allow_all is set (wildcard + credentials
is invalid per the CORS spec)
- Make request_count increment atomic via SQL UPDATE instead of ORM read-modify-write
- Wrap save_messages in a single commit to prevent orphaned user messages on
partial failure; fold _touch_conversation into the same transaction
- Add @handle_exceptions decorator to AppService.update and .delete
- Fix double-pluralisation bug in HATEOAS URL builder (table names already plural)
- Annotate _PROVIDER_DEFAULTS as ClassVar; collapse duplicate db_uri_async
- Remove dead set_env_from_settings decorator from config_utils.py
- Migrate healthcheck from deprecated google-generativeai to google-genai
- Pin pydantic-ai<2.0.0 and pygentic-ai to commit SHA in pyproject.toml
- Fix CREATE TYPE IF NOT EXISTS (invalid PostgreSQL syntax) in test schema
fixture; replace with DO block + duplicate_object exception handler
- Update stale test asserting [user_id=] prefix that was never added to deps
* chore: add agent services to production compose
* fix(agent): deployment hardening for agent service
- Fix db_name default to match init.sh ("agent" not "open-wearables")
- Add Docker healthcheck to agent service; worker/beat now wait for service_healthy
- Move validate_llm_config() after init_sentry() so startup errors are captured
- Fix flower.sh to use injected CELERY_BROKER_URL instead of fragile grep/cut parsing
- Add agent/** CI coverage: build, code-quality, and test jobs
- Align agent CI Postgres to version 18 (matches backend and prod compose)
- Fix .env.example: SENTRY_ENABLED=False with empty DSN, add MAX_TOOL_CALLS
* fix(agent): audit hardening — deploy safety, config, and type fixes
Deployment:
- scripts/postgres/ensure_db.py: idempotent DB creation before migrations,
using psycopg.sql to satisfy type checker (no f-string in execute)
- scripts/start/app.sh: run ensure_db.py before alembic upgrade head
VSCode multi-root workspace:
- open-wearables.code-workspace: defines backend/agent/frontend/root folders
- backend/.vscode/settings.json, agent/.vscode/settings.json: per-folder
Python interpreter paths so each sub-project gets correct IntelliSense
- .vscode/settings.json: stripped to shared settings only
- .gitignore: replace bare .vscode rule with **/.vscode/* + exception for
settings.json; add !**/.vscode/settings.json alongside !.gemini/settings.json
Dead config removed:
- config.py: drop max_retries (task never called self.retry())
- process_message.py: drop bind=True/max_retries/default_retry_delay from
decorator; use current_task.request.id; remove unused Any/settings imports
- config/.env.example: drop MAX_RETRIES
Tunable constants moved to Settings:
- config.py: add guardrails_soft_word_limit=150, router_context_turns=3,
ow_api_timeout=30
- guardrails.py: read settings.guardrails_soft_word_limit
- router.py: read settings.router_context_turns
Celery timezone: Europe/Warsaw → UTC
OWClient timeouts: all AsyncClient() calls now pass timeout=settings.ow_api_timeout
* fix(agent): fix CI test failures — lifespan validation and ctx mocks
app/main.py:
- Move validate_llm_config() from module level into FastAPI lifespan so
it never runs on import. Module-level execution blocked all test setup
in CI where ANTHROPIC_API_KEY is set to empty string (setdefault can't
override an explicitly-set-empty env var).
tests/conftest.py:
- Add autouse mock_validate_llm_config fixture that patches validate_llm_config
via app.main so the lifespan check never runs during tests regardless of
what env vars CI provides.
tests/agent/test_tools.py:
- Add _make_ctx() helper that builds a minimal MagicMock satisfying
RunContext[HealthAgentDeps] (ctx.deps.user_id set to a UUID).
- Update all OW tool test calls from get_foo(str(uuid4())) to
get_foo(_make_ctx()) — the tool signatures take RunContext, not a
plain string; passing a string caused AttributeError on ctx.deps.
* fix(agent): fix remaining CI test failures for PR #901
- Use `or` pattern instead of setdefault in conftest.py so CI empty-string
secrets (ANTHROPIC_API_KEY="") are overridden with safe test defaults
- Fix RequestValidationError handler to return 422 instead of 400
- Update TestBuildHistory tests to reflect that _build_history correctly
includes both user and assistant turns (not user-only)
* fix(agent): address pre-merge deployment review findings
- Fix JWT auth: replace verify_exp=False + manual check with PyJWT's
built-in ExpiredSignatureError handling
- Fix init.sh: use AGENT_DB_NAME env var (default: agent) instead of
hardcoded database name
- Fix README: correct Python version (3.13) and wrong exec target
(app → agent) in migration commands
- Fix CI: align agent redis to redis:7 (matches backend)
- Fix typo: unavaliable → unavailable in db_up_check.py
- Remove unused SERVER_HOST boilerplate from .env.example
- Add comments: local vs Docker host differences in .env.example,
asyncio.run() prefork-only constraint in Celery tasks,
SSRF domain-name limitation in callback_url validator
---------
Co-authored-by: Tomasz Zawiszowski <zawyszosky@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 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.
Description
Checklist
General
docs/(or no docs update needed)Backend Changes
You have to be in
backenddirectory to make it work:uv run pre-commit run --all-filespassesFrontend Changes
pnpm run lintpassespnpm run format:checkpassespnpm run buildsucceedsTesting Instructions
Steps to test:
1.
2.
3.
Expected behavior:
Screenshots
Additional Notes
Summary by CodeRabbit