Migrated the database to PostgreSQL using Prisma ORM.#65
Conversation
- backend/railway.json: Railway deployment config with and health check - backend/Dockerfile: support Railway's dynamic \ env var - backend/Procfile: Railway Procfile fallback - backend/nixpacks.toml: Nixpacks fallback for Railway auto-detection - backend/.env.example: documents all required environment variables - backend/app/core/config.py: dynamic ALLOWED_ORIGINS, remove hardcoded mock key - backend/app/main.py: CORS uses settings.get_allowed_origins() instead of wildcard - backend/.gitignore: track .env.example (it's a template, not a secret) - frontend/src/pages/Dashboard.tsx: production-safe error message
|
CodeAnt AI is reviewing your PR. |
📝 WalkthroughWalkthroughThe backend was migrated from MongoDB to PostgreSQL with SQLAlchemy models, sessions, relational queries, cache persistence, ingestion updates, startup initialization, tests, CI, and deployment configuration. Frontend API defaults now use same-origin routing. ChangesPostgreSQL migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| runs = db.query(AgentRun).order_by(AgentRun.started_at.desc()).offset(offset).limit(size).all() | ||
| pages = (total + size - 1) // size if total else 1 | ||
|
|
||
| return APIResponse( | ||
| success=True, | ||
| message="Agent run registry logs fetched", | ||
| data=[_serialize_run(d) for d in docs], | ||
| data=[_serialize_run(r) for r in runs], |
There was a problem hiding this comment.
Suggestion: get_agent_runs loads AgentRun rows without eager-loading decisions, then serializes run.decisions for every run. This causes an N+1 query pattern (one query for runs + one per run for decisions), which will degrade badly as page size or traffic grows. Load decisions in the initial query (e.g., selectin/joined load) before serialization. [performance]
Severity Level: Major ⚠️
⚠️ Agent runs endpoint issues many extra database queries.
⚠️ Performance degrades under high traffic or large page sizes.Steps of Reproduction ✅
1. Start the Kepler backend (main app in `backend/app/main.py`) with the current PR code
deployed.
2. Trigger the AI agent workflow so there are `AgentRun` and `AgentDecision` rows
(workflow uses `AgentRun` and `AgentDecision` at
`backend/ai/workflow.py:4,33-38,56-61,93-97` to create runs and decisions).
3. Call the runs listing endpoint `GET /api/v1/agents/runs?page=1&size=10`, implemented in
`backend/api/v1/endpoints/agents.py:59-75`, which executes `runs = db.query(AgentRun)...
.all()` at line 67.
4. For each run returned, `_serialize_run()` at `backend/api/v1/endpoints/agents.py:18-42`
accesses `run.decisions`, which is a SQLAlchemy relationship defined in
`backend/models/db_models.py:471-476` as `decisions: Mapped[List["AgentDecision"]] =
relationship("AgentDecision", back_populates="agent_run")` with default lazy loading; this
causes one additional SELECT per run, producing an N+1 query pattern observable via SQL
logging or database profiling.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/api/v1/endpoints/agents.py
**Line:** 67:73
**Comment:**
*Performance: `get_agent_runs` loads `AgentRun` rows without eager-loading `decisions`, then serializes `run.decisions` for every run. This causes an N+1 query pattern (one query for runs + one per run for decisions), which will degrade badly as page size or traffic grows. Load decisions in the initial query (e.g., selectin/joined load) before serialization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| hashed_password=hashed_pwd, | ||
| role_id=user_in.role_id or role.id, | ||
| organization_id=user_in.organization_id or org.id | ||
| organization_id=user_in.organization_id or org.id, |
There was a problem hiding this comment.
Suggestion: The registration path accepts any client-provided organization_id and writes it directly; if that ID does not exist, db.commit() will raise a foreign-key IntegrityError and return a 500 instead of a controlled client error. Validate that the requested organization exists (or reject unknown IDs) before constructing the user record. [api mismatch]
Severity Level: Major ⚠️
❌ Registration endpoint returns 500 for invalid organization_id.
⚠️ Client cannot distinguish invalid organization from server failure.Steps of Reproduction ✅
1. Start the Kepler backend with this PR code; note the registration endpoint at
`backend/api/v1/endpoints/auth.py:14-53` and the `User` model at
`backend/models/db_models.py:56-71` where `organization_id` is a foreign key to
`organizations.id`.
2. Ensure there is at least one organization row or let the code create the default
`"Global Space Command"` organization via `org = db.query(Organization).first()` and
conditional creation at `backend/api/v1/endpoints/auth.py:25-31`.
3. Send a POST request to `POST /api/v1/auth/register` with JSON matching `UserCreate`
(`backend/schemas/api_schemas.py:53-58`), including an `organization_id` that does not
exist in the `organizations` table (e.g., `{"email": "...", "password": "...",
"organization_id": 999999}`).
4. The `register()` function constructs `User(... organization_id=user_in.organization_id
or org.id, ...)` at `backend/api/v1/endpoints/auth.py:39-44`; because any non-zero
`organization_id` is truthy, it uses the invalid ID instead of `org.id`, and on
`db.commit()` at line 46 the database enforces the foreign key
(`backend/models/db_models.py:64-65`), raising an IntegrityError that is not caught
anywhere in the codebase (no `IntegrityError` handlers found via search), resulting in an
uncaught exception and a generic 500 Internal Server Error to the client instead of a
controlled validation/conflict response.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/api/v1/endpoints/auth.py
**Line:** 43:43
**Comment:**
*Api Mismatch: The registration path accepts any client-provided `organization_id` and writes it directly; if that ID does not exist, `db.commit()` will raise a foreign-key `IntegrityError` and return a 500 instead of a controlled client error. Validate that the requested organization exists (or reject unknown IDs) before constructing the user record.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| all_docs.extend(_serialize_sat(s) for s in q.all()) | ||
|
|
||
| if cls in (None, "DEBRIS"): | ||
| q = db.query(Debris) | ||
| if pattern: | ||
| q = q.filter(Debris.objectName.ilike(pattern) | Debris.noradId.ilike(pattern)) | ||
| all_docs.extend(_serialize_debris(d) for d in q.all()) |
There was a problem hiding this comment.
Suggestion: This endpoint materializes all matching rows from both tables and only then slices for pagination, which can cause major latency and memory growth as catalog size increases. Push pagination into SQL queries (or paginate each source before merge with a deterministic sort strategy) instead of calling .all() on full result sets. [performance]
Severity Level: Critical 🚨
❌ Catalog listing endpoint loads entire tables every request.
⚠️ Increased latency under high object counts and concurrency.
⚠️ Memory footprint grows linearly with catalog size.Steps of Reproduction ✅
1. Start the backend FastAPI app so the router in `backend/api/v1/endpoints/catalog.py:79`
(`@router.get("/objects", response_model=APIResponse[List[Dict[str, Any]]])`) is mounted
under the `/api/v1/catalog` prefix as indicated by the module docstring at
`catalog.py:1-3`.
2. Populate the PostgreSQL database so the `satellites` and `debris` tables (models
defined in `backend/models/db_models.py:125-191`) contain a large number of rows via
existing ingestion/sync workflows (e.g., calling the Space-Track sync endpoints in
`catalog.py:173-227`).
3. Issue an HTTP GET request to `GET /api/v1/catalog/objects?page=1&size=50` with optional
`classification` or `search` parameters; FastAPI dispatches this to `list_space_objects()`
in `backend/api/v1/endpoints/catalog.py:79-86`.
4. Inside `list_space_objects()`, observe that the Satellite query block at
`catalog.py:98-104` and Debris block at `catalog.py:106-110` each call `q.all()`,
materializing all matching rows into memory and extending `all_docs`. Only after that, at
`catalog.py:112-119`, the code computes `total` and slices `all_docs[offset: offset +
size]`, so every request loads the full dataset from both tables, causing latency and
memory usage to grow with catalog size instead of leveraging SQL `LIMIT/OFFSET`
pagination.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/api/v1/endpoints/catalog.py
**Line:** 104:110
**Comment:**
*Performance: This endpoint materializes all matching rows from both tables and only then slices for pagination, which can cause major latency and memory growth as catalog size increases. Push pagination into SQL queries (or paginate each source before merge with a deterministic sort strategy) instead of calling `.all()` on full result sets.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @router.post("/sync", response_model=APIResponse[Dict[str, Any]]) | ||
| def trigger_weather_sync( | ||
| days: int = Query(1, ge=1, le=7, description="Days to look back from NASA DONKI"), | ||
| days: int = Query(1, ge=1, le=7), |
There was a problem hiding this comment.
Suggestion: The new days query parameter is exposed to clients but is not used in the sync logic, so requests with different days values behave identically. Wire this parameter into the sync call path (service method signature and implementation) or remove it from the API to avoid a misleading contract. [incomplete implementation]
Severity Level: Major ⚠️
⚠️ Weather sync endpoint ignores client-specified days parameter.
⚠️ Operators cannot adjust historical window via API calls.
⚠️ API contract misleading; documentation and runtime diverge.Steps of Reproduction ✅
1. Start the backend app so the router in `backend/api/v1/endpoints/weather.py:120-124`
(`@router.post("/sync", response_model=APIResponse[Dict[str, Any]])`) is mounted under
`/api/v1/weather` as described by the module docstring at `weather.py:1-3`.
2. Call the endpoint with a non-default days value, e.g. `POST
/api/v1/weather/sync?days=7`, which FastAPI routes to `trigger_weather_sync()` in
`backend/api/v1/endpoints/weather.py:121-124`, parsing the `days` query parameter
according to `days: int = Query(1, ge=1, le=7)` at `weather.py:122`.
3. Inspect the implementation of `trigger_weather_sync()` at `weather.py:121-127`: the
function ignores the `days` variable completely and always invokes
`weather_service.sync_weather(db)` without passing `days` or any derived value into the
service layer.
4. In `backend/services/weather_service.py:188-190`, confirm that
`SpaceWeatherService.sync_weather()` itself hardcodes `all_events =
self.fetch_all_events(days_back=1)`, so regardless of the `days` value supplied in the
POST request, the sync logic only processes one day of events; repeated requests with
different `days` values produce identical persistence behavior, demonstrating that the
exposed parameter is not wired into runtime logic.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/api/v1/endpoints/weather.py
**Line:** 122:122
**Comment:**
*Incomplete Implementation: The new `days` query parameter is exposed to clients but is not used in the sync logic, so requests with different `days` values behave identically. Wire this parameter into the sync call path (service method signature and implementation) or remove it from the API to avoid a misleading contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| "space_object": _serialize_space_object_from_sat(doc), | ||
| "id": str(sat.id), | ||
| "status": sat.status or "ACTIVE", | ||
| "fuel_percentage": sat.fuel_percentage or 100.0, |
There was a problem hiding this comment.
Suggestion: This fallback treats 0.0 as false and replaces it with 100.0, so a depleted satellite will be reported as fully fueled. Use an explicit None check instead of truthiness so valid zero values are preserved. [falsy zero check]
Severity Level: Critical 🚨
- ❌ Fleet view misreports depleted satellites as fully fueled.
- ⚠️ Mission planning decisions based on incorrect fuel telemetry.Steps of Reproduction ✅
1. Start the FastAPI backend (`backend/app/main.py:19-47`) with PostgreSQL running so
tables are created via the `on_startup()` hook (`backend/app/main.py:89-114`).
2. Using `SessionLocal` from `backend/database/session.py:19-27`, insert a `Satellite` row
(`backend/models/db_models.py:125-171`) with `fuel_percentage=0.0` and a valid `noradId`.
3. Call `GET /api/v1/satellites` implemented by `get_satellites()` in
`backend/api/v1/endpoints/satellites.py:45-69`, which fetches satellite rows and
serializes them via `_serialize_satellite()`.
4. Observe the JSON response: `_serialize_satellite()` (`satellites.py:35-42`) emits
`"fuel_percentage": 100.0` for the depleted satellite because it uses `sat.fuel_percentage
or 100.0`, treating the legitimate `0.0` value as falsy and overwriting it.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/api/v1/endpoints/satellites.py
**Line:** 39:39
**Comment:**
*Falsy Zero Check: This fallback treats `0.0` as false and replaces it with `100.0`, so a depleted satellite will be reported as fully fueled. Use an explicit `None` check instead of truthiness so valid zero values are preserved.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| # JWT — MUST be overridden in production via the SECRET_KEY env var. | ||
| # Generate a secure value with: python -c "import secrets; print(secrets.token_hex(32))" | ||
| SECRET_KEY: str = "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7" |
There was a problem hiding this comment.
Suggestion: The application ships with a fixed JWT signing key in source control. If SECRET_KEY is not overridden in an environment, tokens can be forged by anyone who knows this repository value. Remove the hardcoded default and require the key from environment/configuration at startup. [security]
Severity Level: Critical 🚨
- ❌ JWT tokens forgeable when default secret is deployed.
- ⚠️ Compromised environments grant unauthorized API access via forged tokens.Steps of Reproduction ✅
1. Deploy the backend without setting a `SECRET_KEY` environment variable; `Settings` in
`backend/app/core/config.py:6-12` loads the hardcoded default into `settings.SECRET_KEY`.
2. A user logs in via `POST /api/v1/auth/token` (`login_for_access_token()` in
`backend/api/v1/endpoints/auth.py:55-64`), which calls `create_access_token()` and
`create_refresh_token()` in `backend/app/core/security.py:12-27`.
3. Those functions sign JWTs using `settings.SECRET_KEY` (`security.py:18,27`), so every
issued access and refresh token is signed with the static key embedded in the repository.
4. An attacker with access to the source (or leaked config) can generate arbitrary JWTs
using HS256 and the same static `SECRET_KEY`, and any current or future token verification
logic that relies on `settings.SECRET_KEY` will accept these forged tokens as valid.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/app/core/config.py
**Line:** 12:12
**Comment:**
*Security: The application ships with a fixed JWT signing key in source control. If `SECRET_KEY` is not overridden in an environment, tokens can be forged by anyone who knows this repository value. Remove the hardcoded default and require the key from environment/configuration at startup.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
|
|
||
| # PostgreSQL | ||
| DATABASE_URL: str = "postgresql://postgres:postgres@localhost:5432/orbital_guardian" |
There was a problem hiding this comment.
Suggestion: The default database DSN includes plaintext credentials in code, which can leak privileged access and be reused in deployed environments if not overridden. Move credentials to environment-only secrets and fail startup when they are missing. [security]
Severity Level: Major ⚠️
- ❌ Static database credentials exposed directly in source code.
- ⚠️ Attackers reuse default DSN to access production database.Steps of Reproduction ✅
1. Inspect `Settings` in `backend/app/core/config.py:20-21`, where `DATABASE_URL` is
defined with username `postgres` and password `postgres` directly in source code.
2. Start the backend without overriding `DATABASE_URL` via environment; `create_engine()`
in `backend/database/session.py:12-17` uses `settings.DATABASE_URL` verbatim to connect to
PostgreSQL.
3. Confirm that the application successfully connects to the database using these static
credentials and serves requests (e.g., `/api/v1/auth/register`, `/api/v1/satellites`)
through the ORM layer.
4. From another machine or process, use the same DSN
`postgresql://postgres:postgres@localhost:5432/orbital_guardian` (adjusting host as
needed) to connect directly to the database, gaining privileged access without going
through the API.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/app/core/config.py
**Line:** 21:21
**Comment:**
*Security: The default database DSN includes plaintext credentials in code, which can leak privileged access and be reused in deployed environments if not overridden. Move credentials to environment-only secrets and fail startup when they are missing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| for i in range(len(propagated)): | ||
| obj_a, r_a, v_a = propagated[i] | ||
| if r_a is None: | ||
| continue | ||
| for j in range(i + 1, len(propagated_states)): | ||
| obj_b, r_b, v_b = propagated_states[j] | ||
| for j in range(i + 1, len(propagated)): | ||
| obj_b, r_b, v_b = propagated[j] | ||
| if r_b is None: |
There was a problem hiding this comment.
Suggestion: This performs an all-pairs comparison across the entire propagated catalog, which is quadratic and will become prohibitively slow as object count grows. Introduce pre-filtering/spatial partitioning (or candidate pruning by altitude shell/RAAN buckets) before pairwise checks to avoid O(n²) scans on every run. [performance]
Severity Level: Major ⚠️
- ⚠️ Collision evaluation endpoint slows significantly on large catalogs.
- ⚠️ Scheduler jobs risk timeouts on quadratic collision scans.Steps of Reproduction ✅
1. Populate the `SpaceObject` catalog with a large number of entries (e.g., thousands)
that have valid TLE lines (`backend/models/db_models.py:80-99`), representing real
satellites and debris.
2. Invoke `POST /api/v1/collisions/evaluate` (`trigger_collision_evaluation()` in
`backend/api/v1/endpoints/collisions.py:129-140`), which calls
`collision_engine.predict_collisions(db)` over the entire catalog.
3. In `predict_collisions()`, the engine first propagates all objects, then executes
nested loops over every pair of propagated entries
(`backend/orbital/collision_engine.py:41-47`), resulting in O(n²) calls to
`orbit_engine.calculate_distance()` and `orbit_engine.calculate_relative_velocity()`.
4. As n grows large, measure request latency and CPU usage: evaluation time increases
quadratically, making the collision scan slow and potentially unsuitable for
near-real-time or scheduled operation when the catalog scales.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/orbital/collision_engine.py
**Line:** 41:47
**Comment:**
*Performance: This performs an all-pairs comparison across the entire propagated catalog, which is quadratic and will become prohibitively slow as object count grows. Introduce pre-filtering/spatial partitioning (or candidate pruning by altitude shell/RAAN buckets) before pairwise checks to avoid O(n²) scans on every run.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| secondaryObject=obj_b.noradId, | ||
| riskScore=prob, | ||
| missDistance=miss_m, | ||
| conjunctionTime=now + datetime.timedelta(minutes=12.0), |
There was a problem hiding this comment.
Suggestion: The predicted conjunction time is hardcoded to now + 12 minutes for every pair, so all records get nearly identical TCA regardless of orbital geometry or requested horizon. This produces incorrect collision metadata for downstream APIs and agents; compute TCA from propagation results (and/or lookahead sampling) instead of a fixed offset. [logic error]
Severity Level: Critical 🚨
- ❌ Collision API returns incorrect TCA for all predictions.
- ⚠️ Downstream agents misinterpret conjunction timing and urgency.Steps of Reproduction ✅
1. Ensure the `SpaceObject` table contains multiple rows with non-null TLE lines
(`backend/models/db_models.py:80-99`), so collision prediction has objects to process.
2. Call `POST /api/v1/collisions/evaluate` implemented by `trigger_collision_evaluation()`
in `backend/api/v1/endpoints/collisions.py:129-140`; this invokes
`collision_engine.predict_collisions(db)` defined in
`backend/orbital/collision_engine.py:22-86`.
3. In `predict_collisions()`, every detected close approach constructs a
`CollisionPrediction` with `conjunctionTime=now + datetime.timedelta(minutes=12.0)`
(`collision_engine.py:29-32, 72`), independent of actual orbital geometry or the requested
`lookahead_hours`.
4. Inspect the API response: the `tca` field for each prediction (mapped from
`conjunctionTime` via the `tca` property in `backend/models/db_models.py:323-329`) is
nearly identical across all records, demonstrating that conjunction times are hardcoded
rather than computed from propagation results.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/orbital/collision_engine.py
**Line:** 72:72
**Comment:**
*Logic Error: The predicted conjunction time is hardcoded to `now + 12 minutes` for every pair, so all records get nearly identical TCA regardless of orbital geometry or requested horizon. This produces incorrect collision metadata for downstream APIs and agents; compute TCA from propagation results (and/or lookahead sampling) instead of a fixed offset.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| "catalog_number": deb.noradId or "", | ||
| "cospar_id": None, | ||
| "classification": "DEBRIS", | ||
| "epoch": epoch if isinstance(epoch, str) else (epoch.isoformat() if epoch else None), | ||
| "inclination": deb.inclination, | ||
| "eccentricity": deb.eccentricity, | ||
| "semimajor_axis": deb.semimajor_axis, | ||
| "raan": None, | ||
| "arg_of_perigee": None, | ||
| "mean_anomaly": None, | ||
| "mean_motion": deb.meanMotion, | ||
| "period": deb.period, | ||
| "has_tle": bool(deb.tle_line1 and deb.tle_line2), | ||
| "updated_at": None, | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # GET /catalog/objects | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| @router.get("/objects", response_model=APIResponse[List[Dict[str, Any]]]) | ||
| def list_space_objects( | ||
| page: int = Query(1, ge=1), | ||
| size: int = Query(50, ge=1, le=500), | ||
| classification: Optional[str] = Query(None, description="PAYLOAD | DEBRIS | ROCKET_BODY | UNKNOWN"), | ||
| classification: Optional[str] = Query(None), | ||
| search: Optional[str] = Query(None), | ||
| db: MongoSession = Depends(get_db), | ||
| db: Session = Depends(get_db), | ||
| ): |
There was a problem hiding this comment.
In-memory pagination loads entire catalog before slicing
q.all() is called on both the Satellite and Debris tables and all results are serialised into all_docs before the offset/limit slice is applied. Space-Track tracks 20 000+ objects; loading the full result set into memory on every paginated request will be slow and could exhaust heap on smaller instances. Push offset/limit into the DB queries directly and use db.query(…).count() + db.query(…).offset(…).limit(…).all() per table.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/api/v1/endpoints/catalog.py
Line: 62-86
Comment:
**In-memory pagination loads entire catalog before slicing**
`q.all()` is called on both the `Satellite` and `Debris` tables and all results are serialised into `all_docs` before the offset/limit slice is applied. Space-Track tracks 20 000+ objects; loading the full result set into memory on every paginated request will be slow and could exhaust heap on smaller instances. Push `offset`/`limit` into the DB queries directly and use `db.query(…).count()` + `db.query(…).offset(…).limit(…).all()` per table.
How can I resolve this? If you propose a fix, please make it concise.| from typing import List, Optional | ||
| import secrets |
There was a problem hiding this comment.
The
secrets module is imported but never referenced anywhere in this file. It can be removed to keep the imports clean.
| from typing import List, Optional | |
| import secrets | |
| from typing import List, Optional |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/core/config.py
Line: 2-3
Comment:
The `secrets` module is imported but never referenced anywhere in this file. It can be removed to keep the imports clean.
```suggestion
from typing import List, Optional
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def store(self, db: Session, group: str, source: str, records: List[Dict[str, Any]]) -> None: | ||
| if not records: | ||
| return | ||
|
|
||
| db.db[CACHE_COLLECTION].update_one( | ||
| {"_id": group}, | ||
| { | ||
| "$set": { | ||
| "group": group, | ||
| "source": source, | ||
| "records": records, | ||
| "count": len(records), | ||
| "cachedAt": datetime.datetime.now(datetime.timezone.utc).isoformat(), | ||
| } | ||
| }, | ||
| upsert=True, | ||
| ) | ||
| existing = db.query(ProviderCache).filter(ProviderCache.group_name == group).first() | ||
| now = datetime.datetime.now(datetime.timezone.utc) | ||
| if existing: | ||
| existing.source = source | ||
| existing.records_json = json.dumps(records) | ||
| existing.count = len(records) |
There was a problem hiding this comment.
ProviderCache uses legacy Column style instead of mapped_column
Every other model in this PR uses the SQLAlchemy 2.0 Mapped[T] + mapped_column(…) API, but ProviderCache still uses the 1.x Column(…) style (without Mapped annotations). This inconsistency is harmless today but means type-checkers will not infer column types on this model.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/orbital/providers/cache.py
Line: 37-45
Comment:
**`ProviderCache` uses legacy `Column` style instead of `mapped_column`**
Every other model in this PR uses the SQLAlchemy 2.0 `Mapped[T]` + `mapped_column(…)` API, but `ProviderCache` still uses the 1.x `Column(…)` style (without `Mapped` annotations). This inconsistency is harmless today but means type-checkers will not infer column types on this model.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| class ProviderCache(Base): | ||
| """One row per group — stores the last successful remote payload as JSON.""" | ||
| __tablename__ = "provider_cache" |
There was a problem hiding this comment.
Suggestion: The new ORM model is defined in a provider module instead of the main models module, but app startup only imports models.db_models before calling Base.metadata.create_all. In fresh environments this leaves the cache table uncreated and causes runtime database errors when cache reads/writes run. Move this model into the central models import path or explicitly import this module during startup/migration bootstrap. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Space-Track sync jobs hit errors using cache table.
- ⚠️ Offline cache provider never persists stale satellite data.
- ⚠️ Tests must import cache module manually to pass.
- ⚠️ Background scheduler loses resilience when upstream providers fail.Steps of Reproduction ✅
1. Inspect the application startup hook in `backend/app/main.py:90-98`, where
`on_startup()` imports `engine, Base, SessionLocal` from `database.session`, then imports
`models.db_models` and calls `Base.metadata.create_all(bind=engine)`; no other modules
(such as `orbital.providers.cache`) are imported before schema creation.
2. Open `backend/models/db_models.py:1-200` and observe that it defines ORM models like
`Organization`, `Role`, `User`, `SpaceObject`, `Satellite`, and `Debris`, but there is no
`ProviderCache` class and no `provider_cache` table declared there; the only declaration
of `ProviderCache` is in `backend/orbital/providers/cache.py:22-31`.
3. Note how the tests explicitly compensate for this:
`backend/tests/test_providers.py:7-9` and `backend/tests/test_seed.py:6-8` both import
`models.db_models` and then `orbital.providers.cache # noqa: F401 — registers
ProviderCache with Base.metadata` before calling `Base.metadata.create_all(engine)`,
demonstrating that `ProviderCache` is not part of the default model import set.
4. Run a Space-Track sync via either the built-in scheduler job `job_sync_spacetrack` in
`backend/app/core/scheduler.py:25-35` or the Celery task `sync_spacetrack_data` in
`backend/app/tasks/celery_tasks.py:10-24`; both create a `SessionLocal` and call
`spacetrack_service.sync_all_groups(db, ...)` from `backend/orbital/spacetrack.py:24-40`.
Inside `SpaceTrackService.sync_group` (`orbital/spacetrack.py:295-335`), the provider
chain obtained from `orbital.providers.build_provider_chain`
(`backend/orbital/providers/__init__.py:38-73`) instantiates `LocalCacheProvider` and, on
successful remote fetches or when falling back to cache, calls
`LocalCacheProvider.store()` or `LocalCacheProvider.fetch_group()` in
`backend/orbital/providers/cache.py:37-68`. Because `ProviderCache` (lines 22-31) was
never imported into `Base.metadata` at the time `Base.metadata.create_all()` ran, the
underlying database lacks the `provider_cache` table, so the first access to
`ProviderCache` (e.g., `db.query(ProviderCache)...` at line 40 or `db.commit()` at line
52) fails with a “relation/table 'provider_cache' does not exist” error, preventing cache
entries from being written or read in live runs.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/orbital/providers/cache.py
**Line:** 22:24
**Comment:**
*Incomplete Implementation: The new ORM model is defined in a provider module instead of the main models module, but app startup only imports `models.db_models` before calling `Base.metadata.create_all`. In fresh environments this leaves the cache table uncreated and causes runtime database errors when cache reads/writes run. Move this model into the central models import path or explicitly import this module during startup/migration bootstrap.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| existing = db.query(ProviderCache).filter(ProviderCache.group_name == group).first() | ||
| now = datetime.datetime.now(datetime.timezone.utc) | ||
| if existing: | ||
| existing.source = source | ||
| existing.records_json = json.dumps(records) | ||
| existing.count = len(records) | ||
| existing.cached_at = now | ||
| else: | ||
| db.add(ProviderCache( | ||
| group_name=group, source=source, | ||
| records_json=json.dumps(records), count=len(records), cached_at=now, |
There was a problem hiding this comment.
Suggestion: This read-then-insert/update flow is not atomic, so concurrent syncs for the same group can race: both transactions can observe no row and attempt insert, triggering a unique-key failure on group_name. Use a single database upsert (ON CONFLICT DO UPDATE) or lock the row/key to make cache writes concurrency-safe. [race condition]
Severity Level: Major ⚠️
- ⚠️ Concurrent sync jobs can raise unique-key integrity errors.
- ⚠️ Cache writes may randomly fail under parallel ingestion.
- ⚠️ Unique violations bubble into cache store logic.
- ⚠️ Offline cache may be incomplete or inconsistent.Steps of Reproduction ✅
1. Examine `LocalCacheProvider.store()` in `backend/orbital/providers/cache.py:37-52`: it
first queries for an existing row with `existing =
db.query(ProviderCache).filter(ProviderCache.group_name == group).first()` (line 40), then
either updates that row or, if `existing` is `None`, calls `db.add(ProviderCache(...))`
with `group_name=group` and finally `db.commit()` at line 52.
2. Observe the `ProviderCache` model definition in the same file at
`backend/orbital/providers/cache.py:22-31`, where `group_name` is declared with
`unique=True` and an index (`group_name: str = Column(String(50), unique=True,
nullable=False, index=True)`), meaning the database enforces a unique constraint per group
name.
3. Follow the call chain to see when `store()` is invoked: `ProviderChain._cache()` in
`backend/orbital/providers/chain.py:82-96` calls `self.cache.store(db, group,
provider.name, records)` for each successful remote provider fetch, and that method is
invoked from `ProviderChain.fetch_group()` (`chain.py:41-77`), which is in turn called by
`SpaceTrackService.sync_group()` in `backend/orbital/spacetrack.py:295-335`.
4. Trigger concurrent cache writes for the same group by running multiple ingestion jobs
in parallel—for example, the Celery task `sync_spacetrack_data`
(`backend/app/tasks/celery_tasks.py:10-24`) and the built-in scheduler job
`job_sync_spacetrack` (`backend/app/core/scheduler.py:25-35`) can both call
`spacetrack_service.sync_all_groups(db, ...)` with overlapping execution. Because each
job’s `LocalCacheProvider.store()` executes a separate `SELECT` (line 40) followed by a
conditional `INSERT`, both can observe `existing is None` and attempt to insert a new row
with the same `group_name`, causing one of the `db.commit()` calls at line 52 to raise a
unique-key `IntegrityError` rather than performing a safe upsert.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/orbital/providers/cache.py
**Line:** 40:50
**Comment:**
*Race Condition: This read-then-insert/update flow is not atomic, so concurrent syncs for the same group can race: both transactions can observe no row and attempt insert, triggering a unique-key failure on `group_name`. Use a single database upsert (`ON CONFLICT DO UPDATE`) or lock the row/key to make cache writes concurrency-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| group_name=group, source=source, | ||
| records_json=json.dumps(records), count=len(records), cached_at=now, | ||
| )) | ||
| db.commit() |
There was a problem hiding this comment.
Suggestion: If commit fails (for example due to a race-induced integrity error), the SQLAlchemy session is left in a failed transaction state; subsequent ORM operations on the same session will fail until rollback is called. Wrap commit in exception handling and rollback() before re-raising/returning so later ingestion steps can continue safely. [missing cleanup]
Severity Level: Critical 🚨
- ❌ Any cache commit failure breaks subsequent upsert operations.
- ❌ Space-Track ingestion jobs can fail across all groups.
- ⚠️ Shared SQLAlchemy session remains unusable until closed.
- ⚠️ Background scheduler and Celery tasks become less reliable.Steps of Reproduction ✅
1. In `LocalCacheProvider.store()` (`backend/orbital/providers/cache.py:37-52`), after
potentially inserting or updating a `ProviderCache` row, the code calls `db.commit()` at
line 52 without wrapping it in `try/except` or performing a `db.rollback()` on failure.
2. `ProviderChain._cache()` in `backend/orbital/providers/chain.py:82-96` calls
`self.cache.store(db, group, provider.name, records)` inside a broad `try/except` that
catches all exceptions and logs them, but it does not roll back the SQLAlchemy `Session`
when `store()` raises, so any failed commit (e.g., due to the missing `provider_cache`
table described above or a unique-key race from suggestion 2) leaves the session
transaction in a failed state.
3. Follow the control flow in `SpaceTrackService.sync_group()`
(`backend/orbital/spacetrack.py:295-335`): it first calls
`self.providers.fetch_group(group, limit or 500, db=db)` (line 307), which triggers
`_cache()` and thus `LocalCacheProvider.store()` with the shared `db` session. After
returning from `fetch_group`, `sync_group()` immediately calls `self._bulk_upsert(db,
is_debris, docs)` (lines 328-329) using the same `Session` object.
4. When `db.commit()` in `LocalCacheProvider.store()` fails (for example, because the
`provider_cache` table does not exist or due to a concurrent unique-key insert),
SQLAlchemy marks the transaction as errored; `_cache()` catches and logs the exception but
does not call `db.rollback()`, so the subsequent `_bulk_upsert()` call in `sync_group()`
encounters a session in a failed transaction state and raises a
`PendingRollbackError`/database error. This causes `sync_group()` to fall into its `except
Exception` block (lines 328-335), returning a failed status for that group, and because
`sync_all_groups()` (`orbital/spacetrack.py:24-39`) reuses the same `db` session across
groups, the poisoned session can cause all remaining groups in the run to fail until the
outer job closes the session in `sync_spacetrack_data`
(`backend/app/tasks/celery_tasks.py:10-28`) or `job_sync_spacetrack`
(`backend/app/core/scheduler.py:25-38`).(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/orbital/providers/cache.py
**Line:** 52:52
**Comment:**
*Missing Cleanup: If commit fails (for example due to a race-induced integrity error), the SQLAlchemy session is left in a failed transaction state; subsequent ORM operations on the same session will fail until rollback is called. Wrap commit in exception handling and `rollback()` before re-raising/returning so later ingestion steps can continue safely.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/api/v1/endpoints/auth.py (1)
39-44: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winDo not trust role and organization IDs during public registration.
A caller can self-assign any existing role or organization. Use the server-selected defaults here; privileged assignment should require an authenticated administrative flow.
Proposed fix
user = User( email=user_in.email, hashed_password=hashed_pwd, - role_id=user_in.role_id or role.id, - organization_id=user_in.organization_id or org.id, + role_id=role.id, + organization_id=org.id, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/auth.py` around lines 39 - 44, Update the User construction in the public registration flow to ignore user_in.role_id and user_in.organization_id, always assigning the server-selected role.id and org.id defaults. Leave privileged role or organization assignment to authenticated administrative flows.backend/api/v1/endpoints/weather.py (1)
125-132: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSeparate DB errors from NASA DONKI failures.
trigger_weather_syncwraps every exception fromweather_service.sync_weather(db)inExternalServiceError(service="NASA DONKI"), so a PostgreSQL/SQLAlchemy outage is reported as an upstream NASA problem. Catch database exceptions separately and return the database-unavailable contract instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/weather.py` around lines 125 - 132, Update trigger_weather_sync around weather_service.sync_weather(db) to catch PostgreSQL/SQLAlchemy database exceptions separately and return the existing database-unavailable contract; only wrap genuine NASA DONKI failures in ExternalServiceError(service="NASA DONKI"). Preserve exception chaining and the current logging behavior for each failure category.backend/orbital/spacetrack.py (1)
375-389: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the actual synchronization time.
last_syncis currently the time/statswas requested, so stale or never-synchronized catalogs appear fresh. Derive it from persisted update timestamps or a dedicated sync-run record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/orbital/spacetrack.py` around lines 375 - 389, Update get_stats so last_sync reflects the most recent persisted catalog synchronization or update timestamp, rather than datetime.datetime.utcnow() at request time. Use the existing timestamp fields or sync-run record available through the database models, and preserve the response format while returning an appropriate value for catalogs that have never synchronized.
🟠 Major comments (24)
backend/scripts/seed_db.py-89-93 (1)
89-93: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not orphan retained users when clearing organizations.
Userrows are retained, while deletingOrganizationrows sets theirorganization_idto null. Recreating organizations with new IDs does not restore those memberships. Preserve organizations or explicitly rebind/delete dependent users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/seed_db.py` around lines 89 - 93, Update the clear branch around the model deletion loop so retained User rows are not left with lost organization memberships. Preserve organizations during clearing, or explicitly capture and rebind dependent users to recreated organizations (and delete users only if that is the intended behavior); ensure the existing db.commit flow leaves user organization assignments valid.backend/scripts/seed_db.py-105-111 (1)
105-111: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the seed operation atomic.
Per-record commits mean any later failure leaves partially seeded data; the final rollback only reverses work since the most recent commit. Replace intermediate commits—including Line 93—with
flush()where IDs are needed, then commit once after all sections succeed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/seed_db.py` around lines 105 - 111, Update the database seeding flow, including the organization loop around Organization and the earlier commit near line 93, to replace intermediate commit calls with flush() when generated IDs are needed. Commit once only after every seeding section completes successfully, preserving rollback behavior for any failure.backend/scripts/seed_db.py-133-136 (1)
133-136: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent NORAD allocation loops from hanging after range exhaustion.
The satellite range contains only 40,000 IDs, but
--counthas no upper bound. Once available IDs are exhausted, thesewhileloops never terminate. Build the available-ID pools first and reject requests exceeding their remaining capacity.Also applies to: 203-206, 284-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/seed_db.py` around lines 133 - 136, Update the NORAD allocation logic around the shown random-ID loops and the corresponding allocation blocks at the other referenced locations to build pools of available IDs before generation. Validate each requested count against the pool’s remaining capacity and reject excess requests before entering allocation, then remove retry-until-unique loops and consume IDs from the pools while preserving uniqueness.backend/scripts/seed_db.py-117-122 (1)
117-122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReserve NORAD IDs from the master
SpaceObjecttable.
SpaceObject.noradIdis globally unique, but non-clear mode only loads IDs fromSatelliteandDebris. A master row without a detail row can therefore collide and abort seeding.- for row in db.query(Satellite.noradId).all(): - used_norad_ids.add(row[0]) - for row in db.query(Debris.noradId).all(): + for row in db.query(SpaceObject.noradId).all(): used_norad_ids.add(row[0])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/seed_db.py` around lines 117 - 122, Update the non-clear branch of the seeding logic to populate used_norad_ids from the master SpaceObject.noradId query, ensuring IDs without Satellite or Debris detail rows are reserved; retain the existing clear-mode behavior and set-based deduplication.MIGRATION_SUMMARY.md-11-11 (1)
11-11: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReconcile the documented schema inventory.
The summary says there are 18 collections/tables, but the table list contains 22 entries and omits the
counterscollection named in the migration objectives. Generate this inventory from the actual models and migrations before claiming complete coverage.Also applies to: 131-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` at line 11, Update the schema inventory in MIGRATION_SUMMARY.md to reflect all actual models and migrations: include the omitted counters collection and reconcile the stated total with the complete table list. Derive and verify the entries from the repository’s model and migration definitions before documenting coverage.docker-compose.yml-7-10 (1)
7-10: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not allow predictable production fallbacks.
If these variables are omitted, the database uses
postgres/postgresand the backend uses a known signing key. Fail fast when secrets are missing, or move these defaults into a clearly local-only override that cannot be used for deployment.Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 7 - 10, Remove the predictable credentials and signing-key fallbacks from the deployment configuration. Update the environment entries for POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB, and the backend signing key to require explicitly supplied values, or relocate development defaults to a clearly local-only override that deployment cannot consume.Source: Linters/SAST tools
.github/workflows/backend-ci.yml-3-11 (1)
3-11: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrigger CI when the workflow itself changes.
Because the path filter only includes
backend/**, changes limited to.github/workflows/backend-ci.ymlwill not run this validation. Add the workflow path, and include any root deployment configuration that should be covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml around lines 3 - 11, Update the path filters in the backend-ci workflow’s push and pull_request triggers to include the workflow file itself and the applicable root deployment configuration, while preserving the existing backend/** filter and branch restrictions.MIGRATION_SUMMARY.md-288-295 (1)
288-295: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFix the rollback environment instructions.
DATABASE_URLis the PostgreSQL variable; setting it toMONGODB_URIis not a valid rollback. Reverting also requires a MongoDB-compatible application version, restoring a current MongoDB snapshot, and switching the configuration variable explicitly—not only reinstallingpymongo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` around lines 288 - 295, Update the “Rollback Procedure” section to instruct users to restore a current MongoDB snapshot, revert to a MongoDB-compatible application version, reinstall the required pymongo dependency, and explicitly switch the configuration from DATABASE_URL to MONGODB_URI in .env. Remove the invalid instruction to set DATABASE_URL to MONGODB_URI.backend/.gitignore-7-7 (1)
7-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winIgnore all dotenv files except the tracked template.
Keeping only
.envignored leaves.env.localand.env.productionunprotected. Use.env*followed by!.env.exampleto reduce accidental secret commits.Proposed fix
.env -# .env.example is intentionally tracked — it documents required variables. +.env* +!.env.example🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/.gitignore` at line 7, Update the dotenv ignore rules in the .gitignore configuration to ignore all .env* files, then add an exception so .env.example remains tracked. Preserve the existing intent of protecting environment-specific secrets while allowing the documented template.backend/Dockerfile-36-37 (1)
36-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse an exec-safe runtime command
Shell-formCMDleaves/bin/shas PID 1, soSIGTERMmay not reach Uvicorn cleanly on stop/restart. Apply the sameexecpattern here and inbackend/railway.json’sstartCommand.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/Dockerfile` around lines 36 - 37, Update the Dockerfile CMD and the Railway configuration’s startCommand to use an exec-safe command that launches Uvicorn directly as PID 1, preserving runtime PORT expansion and the existing app, host, and port arguments.MIGRATION_SUMMARY.md-239-246 (1)
239-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the actual MongoDB restore/import flow
mongodumponly produces a backup, andbackend/scripts/seed_db.py --count 100 --clearseeds new PostgreSQL sample data. This section currently describes backup + reseed, not an existing-data migration. If there is a restore/import command, document it here with validation steps; otherwise rename the section accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` around lines 239 - 246, Update the “From Existing MongoDB” section to document the actual existing-data migration flow: include the MongoDB restore/import command and validation steps if those operations exist. If no restore/import flow is supported, rename the section to clearly describe MongoDB backup followed by PostgreSQL sample-data seeding, rather than an existing-data migration.backend/orbital/collision_engine.py-22-54 (1)
22-54: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement the requested lookahead instead of fabricating TCA.
lookahead_hoursis ignored: objects are propagated only atnow, while every detected pair receivesnow + 12 minutesas its conjunction time. Search the requested interval and persist the timestamp of minimum separation.Also applies to: 65-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/orbital/collision_engine.py` around lines 22 - 54, Update predict_collisions to use lookahead_hours by propagating each valid object across the requested interval, evaluating pairwise separations over time, and selecting the timestamp with minimum separation as the conjunction time. Replace the fixed fabricated TCA with that computed timestamp while preserving the existing collision threshold and probability calculation.backend/app/main.py-91-116 (1)
91-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail startup when PostgreSQL initialization fails.
Swallowing schema or seed failures leaves the service reporting healthy while database-backed endpoints and scheduled jobs are unusable. Log the exception, then re-raise it or expose a failing readiness state.
Proposed fix
- except Exception as e: - logger.error(f"⚠️ PostgreSQL init failed: {e}") - logger.warning("Continuing without DB — some features unavailable.") + except Exception: + logger.exception("PostgreSQL initialization failed") + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/main.py` around lines 91 - 116, Update the PostgreSQL initialization try/except around Base.metadata.create_all and the Organization/Role seed logic to fail startup after logging the exception, rather than continuing with the “without DB” warning. Re-raise the caught exception so schema or seed failures propagate and the service does not report healthy.backend/api/v1/endpoints/collisions.py-64-64 (1)
64-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlace null risk scores last.
CollisionPrediction.riskScoreis nullable, and PostgreSQL puts NULLs first onDESC, so unscored rows can appear ahead of higher-risk predictions.Proposed fix
- query = db.query(CollisionPrediction).order_by(CollisionPrediction.riskScore.desc()) + query = db.query(CollisionPrediction).order_by( + CollisionPrediction.riskScore.desc().nullslast() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/collisions.py` at line 64, Update the CollisionPrediction query ordering to explicitly place nullable riskScore values last while retaining descending order for non-null scores. Modify the order_by expression on CollisionPrediction.riskScore; do not change the surrounding query behavior.backend/models/db_models.py-104-109 (1)
104-109: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the one-to-one
SpaceObjectlinks at the database level.
uselist=Falseonly affects ORM loading; without a unique constraint onSatellite.space_object_idandDebris.space_object_id, multiple subtype rows can still point at the sameSpaceObject.Proposed fix
space_object_id: Mapped[Optional[int]] = mapped_column( - Integer, ForeignKey("orbitalElements.id", ondelete="SET NULL") + Integer, + ForeignKey("orbitalElements.id", ondelete="SET NULL"), + unique=True, )Apply the same constraint to
Debris.space_object_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/models/db_models.py` around lines 104 - 109, Add database-level unique constraints for both Satellite.space_object_id and Debris.space_object_id, alongside the existing satellite_details and debris_details relationships, so each SpaceObject can have at most one linked row of each subtype. Ensure the constraints are included in the schema migration as well as the model definitions.backend/api/v1/endpoints/agents.py-60-68 (1)
60-68: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBound the agent-run page size.
sizehas no upper bound here, so a request can pull and serialize an arbitrarily large run/decision history. Match the other list endpoints and cap it.Proposed fix
- size: int = Query(10, ge=1), + size: int = Query(10, ge=1, le=100),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/agents.py` around lines 60 - 68, Update the size Query parameter in get_agent_runs to enforce the same maximum page-size bound used by the other list endpoints, while retaining the existing minimum of 1 and pagination calculations.backend/services/weather_service.py-188-190 (1)
188-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor the sync endpoint's requested date range.
The caller exposes
days=1..7, but this method always fetches one day, silently omitting requested history. Acceptdays_backand pass the endpoint value through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/weather_service.py` around lines 188 - 190, Update sync_weather to accept a days_back parameter and pass it to fetch_all_events instead of hardcoding days_back=1. Ensure the sync endpoint’s requested 1–7 day range is forwarded unchanged through the existing call path.backend/api/v1/endpoints/dashboard.py-27-34 (1)
27-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSeparate the latest severity from the latest K-index record.
Solar flares, CMEs, and radiation rows have
k_index=None. If one is newest, the dashboard ignores its persisted severity and performs a blocking NOAA call. Query the latest K-bearing row for the index and derive severity independently from persisted events.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/dashboard.py` around lines 27 - 34, Update the dashboard weather-loading logic to query the latest K-bearing SpaceWeather row for weather_index, while deriving weather_severity independently from the latest persisted event regardless of whether k_index is None. Use the existing SpaceWeather fields and preserve the NORMAL fallback, ensuring persisted severity is used before any NOAA request.backend/services/weather_service.py-192-230 (1)
192-230: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake weather synchronization idempotent.
Every overlapping run inserts the same DONKI events and creates duplicate alerts. Persist each
activity_idas a unique external key and upsert events; deduplicate alerts against that same event key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/weather_service.py` around lines 192 - 230, Update the weather synchronization flow covering the geomagnetic storm, solar flare, CME, and radiation event loops to persist each event’s activity_id as a unique external key, upsert the corresponding SpaceWeather record instead of always inserting, and associate alerts with that key so repeated or overlapping runs do not create duplicate alerts.backend/api/v1/endpoints/satellites.py-84-95 (1)
84-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisambiguate database IDs from NORAD IDs.
For a numeric value, the
ORcan match one satellite by primary key and another by NORAD ID;.first()then selects an unspecified row. Use distinct routes or define deterministic lookup precedence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/satellites.py` around lines 84 - 95, Update get_satellite_telemetry to use deterministic lookup precedence for numeric satellite_id values instead of combining Satellite.id and Satellite.noradId in one OR query. Resolve the numeric database ID first, then fall back to an exact noradId lookup only when no ID match exists; preserve the existing NotFoundError behavior.backend/api/v1/endpoints/satellites.py-35-42 (1)
35-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve a legitimate zero fuel level.
0.0 or 100.0reports an empty tank as full. Default only when the database value isNone.Proposed fix
- "fuel_percentage": sat.fuel_percentage or 100.0, + "fuel_percentage": ( + sat.fuel_percentage if sat.fuel_percentage is not None else 100.0 + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/satellites.py` around lines 35 - 42, Update _serialize_satellite so fuel_percentage defaults to 100.0 only when sat.fuel_percentage is None, preserving a legitimate 0.0 value; leave the other field serialization behavior unchanged.backend/api/v1/endpoints/catalog.py-79-121 (1)
79-121: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMove catalog pagination into PostgreSQL.
Both branches still call
.all(), serialize every matching row, and slice the combined list in Python. That makes each request scale with the full catalog instead of the page size. Use database-sideCOUNT+OFFSET/LIMITwith a deterministicorder_by, or a paginatedUNION ALLfor the mixed view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/catalog.py` around lines 79 - 121, Update list_space_objects to perform COUNT and OFFSET/LIMIT in PostgreSQL rather than calling .all(), serializing the full result sets, and slicing all_docs in Python. Apply the requested filters and deterministic order_by to both Satellite and Debris queries, fetch only the current page, and compute total/pages from database counts while preserving the mixed classification behavior and response schema.backend/orbital/providers/cache.py-37-53 (1)
37-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRollback cache commit failures before reusing the session.
ProviderChain.sync_group()continues with the samedbsession after_cache()swallowsLocalCacheProvider.store()errors. Ifdb.commit()fails here, the session stays poisoned and the later upsert can abort even though the fetch succeeded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/orbital/providers/cache.py` around lines 37 - 53, The LocalCacheProvider.store method must roll back the SQLAlchemy session when db.commit() fails before allowing the swallowed error to return. Wrap the commit in failure handling that calls db.rollback() and then preserves the existing error propagation behavior, so ProviderChain.sync_group() can safely reuse the session for the later upsert.backend/orbital/spacetrack.py-204-223 (1)
204-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLimit the PostgreSQL upsert to feed-owned columns.
stmt.excludedis updating every model column here, so ingestion can clear app-owned fields likeorganization_id,space_object_id, and mass data when the feed omits them;status,fuel_percentage, andoperational_modeare also reset on every refresh. The row-by-row fallback only copies keys present in the document, so this is specific to the PostgreSQL path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/orbital/spacetrack.py` around lines 204 - 223, Restrict the PostgreSQL upsert update mapping to feed-owned fields only, excluding app-owned columns such as organization_id, space_object_id, mass data, status, fuel_percentage, and operational_mode. Update the upsert construction in the PostgreSQL ingestion path to explicitly reuse only columns produced by the feed record, while preserving the row-by-row fallback behavior.
🟡 Minor comments (8)
backend/scripts/seed_db.py-184-186 (1)
184-186: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPersist timezone-aware UTC timestamps.
Telemetry.timestampandCollisionPrediction.conjunctionTimeare timezone-aware columns, but both values originate from naiveutcnow()calls. PostgreSQL may interpret them using the session timezone.- tca = datetime.datetime.utcnow() + datetime.timedelta(hours=random.uniform(4, 72)) + tca = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(hours=random.uniform(4, 72))Apply the same change when creating telemetry timestamps.
Also applies to: 259-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/seed_db.py` around lines 184 - 186, Update the telemetry timestamp creation in the seeding flow around the `Telemetry` constructor to use a timezone-aware UTC datetime instead of `datetime.datetime.utcnow()`. Preserve the existing ten-minute offset calculation, and apply the equivalent timezone-aware UTC change to the `CollisionPrediction` timestamp creation referenced by the comment.MIGRATION_SUMMARY.md-71-74 (1)
71-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not claim CI/CD when the provided workflow only runs tests.
.github/workflows/backend-ci.ymldefines a test job but no deployment step. Change this to “CI” or document the external Railway deployment trigger and its verification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` around lines 71 - 74, Update the Phase 14 entry in MIGRATION_SUMMARY.md to avoid claiming a CI/CD deployment pipeline when backend-ci.yml only runs tests: rename the item to “CI” or document the external Railway deployment trigger and its verification.MIGRATION_SUMMARY.md-258-268 (1)
258-268: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the connection-pooling contradiction.
The performance section says pooling is configured with 10 connections and 20 overflow, while Future Improvements says to add connection pooling. State the actual configuration or rename the future item to tuning/monitoring.
Also applies to: 298-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` around lines 258 - 268, Resolve the connection-pooling contradiction between the Performance Improvements section and the Future Improvements item: update the performance entry to state the actual SQLAlchemy pool configuration, or rename the future item to focus on pool tuning and monitoring rather than adding pooling. Keep both sections consistent and avoid claiming pooling is both implemented and pending.MIGRATION_SUMMARY.md-272-279 (1)
272-279: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the frontend compatibility statement.
The PR changes frontend API defaults for same-origin routing, so “Frontend - No changes required” is inaccurate. Clarify that API compatibility was preserved while frontend deployment defaults changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION_SUMMARY.md` around lines 272 - 279, Update the “Frontend” entry in the Backward Compatibility section to acknowledge that API compatibility is preserved while frontend deployment defaults for same-origin routing changed; do not claim that no frontend changes are required..github/workflows/backend-ci.yml-37-37 (1)
37-37: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDisable checkout credential persistence.
actions/checkoutwrites the GitHub token into git config by default; setpersist-credentials: falseand add a minimalpermissions:block, such ascontents: read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml at line 37, Update the actions/checkout@v4 step to set persist-credentials to false, and add a minimal workflow permissions block granting only contents: read. Keep the existing checkout behavior otherwise unchanged.Source: Linters/SAST tools
backend/api/v1/endpoints/weather.py-121-126 (1)
121-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse or remove the
daysquery parameter.
daysis accepted and validated but has no effect on synchronization. Pass it through to the service contract or remove it from this endpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/weather.py` around lines 121 - 126, Update trigger_weather_sync so the validated days parameter is either passed to weather_service.sync_weather according to its service contract, or removed from the endpoint if synchronization does not support a day range; ensure the endpoint no longer accepts an unused parameter.backend/api/v1/endpoints/satellites.py-59-61 (1)
59-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an explicit sort before paginating.
backend/api/v1/endpoints/satellites.py:59-61
query.offset(...).limit(...)is unordered here, so page contents can shift between requests. Order by a stable column likeSatellite.idbefore slicing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/endpoints/satellites.py` around lines 59 - 61, Update the query flow using query, offset, and limit to apply a deterministic order by the stable Satellite.id column before pagination. Keep the existing count and page calculation behavior unchanged, ensuring offset and limit operate on the ordered query.backend/orbital/spacetrack.py-202-215 (1)
202-215: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPass a timezone-aware
datetimetoupdatedAt. ThisDateTime(timezone=True)field should receivedatetime.now(datetime.timezone.utc)instead ofutcnow().isoformat().backend/orbital/spacetrack.py:202-215🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/orbital/spacetrack.py` around lines 202 - 215, Update the timestamp initialization in the record-building method containing the returned object to use the timezone-aware UTC datetime value from datetime.datetime.now(datetime.timezone.utc), and pass that datetime directly as updatedAt instead of converting it with isoformat().
🧹 Nitpick comments (2)
backend/tests/test_seed.py (2)
29-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify the generated TLE checksums, not just their type.
These assertions pass for any appended digit, so regressions in the new checksum calculation remain undetected. Recompute the modulo-10 checksum from the first 68 characters and compare it with the final digit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_seed.py` around lines 29 - 36, Update test_generate_tle to recompute each TLE line’s modulo-10 checksum from its first 68 characters, counting minus signs as one, and assert that the result matches the final digit of line1 and line2. Replace the weak is-digit assertions while preserving the existing length and prefix checks.
60-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise an actual non-clear seed run.
This database starts empty, so the test only verifies initial seeding and would pass if
clear=Falsewere ignored. Seed existing rows first, run again without clearing, then assert the original rows remain and the expected new rows were appended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_seed.py` around lines 60 - 69, Update test_seed_database_no_clear to insert existing Satellite rows before calling seed_database(count=5, clear=False), then verify those original rows remain and five new rows are appended. Keep the test’s patched engine and SessionLocal setup, and assert the final contents or count demonstrates that clear=False preserves prior data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/core/config.py`:
- Around line 10-12: Update SECRET_KEY configuration to require a value from the
SECRET_KEY environment variable instead of using the repository-known hardcoded
default. Ensure startup fails clearly when the variable is missing, while
preserving the existing string configuration interface.
---
Outside diff comments:
In `@backend/api/v1/endpoints/auth.py`:
- Around line 39-44: Update the User construction in the public registration
flow to ignore user_in.role_id and user_in.organization_id, always assigning the
server-selected role.id and org.id defaults. Leave privileged role or
organization assignment to authenticated administrative flows.
In `@backend/api/v1/endpoints/weather.py`:
- Around line 125-132: Update trigger_weather_sync around
weather_service.sync_weather(db) to catch PostgreSQL/SQLAlchemy database
exceptions separately and return the existing database-unavailable contract;
only wrap genuine NASA DONKI failures in ExternalServiceError(service="NASA
DONKI"). Preserve exception chaining and the current logging behavior for each
failure category.
In `@backend/orbital/spacetrack.py`:
- Around line 375-389: Update get_stats so last_sync reflects the most recent
persisted catalog synchronization or update timestamp, rather than
datetime.datetime.utcnow() at request time. Use the existing timestamp fields or
sync-run record available through the database models, and preserve the response
format while returning an appropriate value for catalogs that have never
synchronized.
---
Major comments:
In @.github/workflows/backend-ci.yml:
- Around line 3-11: Update the path filters in the backend-ci workflow’s push
and pull_request triggers to include the workflow file itself and the applicable
root deployment configuration, while preserving the existing backend/** filter
and branch restrictions.
In `@backend/.gitignore`:
- Line 7: Update the dotenv ignore rules in the .gitignore configuration to
ignore all .env* files, then add an exception so .env.example remains tracked.
Preserve the existing intent of protecting environment-specific secrets while
allowing the documented template.
In `@backend/api/v1/endpoints/agents.py`:
- Around line 60-68: Update the size Query parameter in get_agent_runs to
enforce the same maximum page-size bound used by the other list endpoints, while
retaining the existing minimum of 1 and pagination calculations.
In `@backend/api/v1/endpoints/catalog.py`:
- Around line 79-121: Update list_space_objects to perform COUNT and
OFFSET/LIMIT in PostgreSQL rather than calling .all(), serializing the full
result sets, and slicing all_docs in Python. Apply the requested filters and
deterministic order_by to both Satellite and Debris queries, fetch only the
current page, and compute total/pages from database counts while preserving the
mixed classification behavior and response schema.
In `@backend/api/v1/endpoints/collisions.py`:
- Line 64: Update the CollisionPrediction query ordering to explicitly place
nullable riskScore values last while retaining descending order for non-null
scores. Modify the order_by expression on CollisionPrediction.riskScore; do not
change the surrounding query behavior.
In `@backend/api/v1/endpoints/dashboard.py`:
- Around line 27-34: Update the dashboard weather-loading logic to query the
latest K-bearing SpaceWeather row for weather_index, while deriving
weather_severity independently from the latest persisted event regardless of
whether k_index is None. Use the existing SpaceWeather fields and preserve the
NORMAL fallback, ensuring persisted severity is used before any NOAA request.
In `@backend/api/v1/endpoints/satellites.py`:
- Around line 84-95: Update get_satellite_telemetry to use deterministic lookup
precedence for numeric satellite_id values instead of combining Satellite.id and
Satellite.noradId in one OR query. Resolve the numeric database ID first, then
fall back to an exact noradId lookup only when no ID match exists; preserve the
existing NotFoundError behavior.
- Around line 35-42: Update _serialize_satellite so fuel_percentage defaults to
100.0 only when sat.fuel_percentage is None, preserving a legitimate 0.0 value;
leave the other field serialization behavior unchanged.
In `@backend/app/main.py`:
- Around line 91-116: Update the PostgreSQL initialization try/except around
Base.metadata.create_all and the Organization/Role seed logic to fail startup
after logging the exception, rather than continuing with the “without DB”
warning. Re-raise the caught exception so schema or seed failures propagate and
the service does not report healthy.
In `@backend/Dockerfile`:
- Around line 36-37: Update the Dockerfile CMD and the Railway configuration’s
startCommand to use an exec-safe command that launches Uvicorn directly as PID
1, preserving runtime PORT expansion and the existing app, host, and port
arguments.
In `@backend/models/db_models.py`:
- Around line 104-109: Add database-level unique constraints for both
Satellite.space_object_id and Debris.space_object_id, alongside the existing
satellite_details and debris_details relationships, so each SpaceObject can have
at most one linked row of each subtype. Ensure the constraints are included in
the schema migration as well as the model definitions.
In `@backend/orbital/collision_engine.py`:
- Around line 22-54: Update predict_collisions to use lookahead_hours by
propagating each valid object across the requested interval, evaluating pairwise
separations over time, and selecting the timestamp with minimum separation as
the conjunction time. Replace the fixed fabricated TCA with that computed
timestamp while preserving the existing collision threshold and probability
calculation.
In `@backend/orbital/providers/cache.py`:
- Around line 37-53: The LocalCacheProvider.store method must roll back the
SQLAlchemy session when db.commit() fails before allowing the swallowed error to
return. Wrap the commit in failure handling that calls db.rollback() and then
preserves the existing error propagation behavior, so ProviderChain.sync_group()
can safely reuse the session for the later upsert.
In `@backend/orbital/spacetrack.py`:
- Around line 204-223: Restrict the PostgreSQL upsert update mapping to
feed-owned fields only, excluding app-owned columns such as organization_id,
space_object_id, mass data, status, fuel_percentage, and operational_mode.
Update the upsert construction in the PostgreSQL ingestion path to explicitly
reuse only columns produced by the feed record, while preserving the row-by-row
fallback behavior.
In `@backend/scripts/seed_db.py`:
- Around line 89-93: Update the clear branch around the model deletion loop so
retained User rows are not left with lost organization memberships. Preserve
organizations during clearing, or explicitly capture and rebind dependent users
to recreated organizations (and delete users only if that is the intended
behavior); ensure the existing db.commit flow leaves user organization
assignments valid.
- Around line 105-111: Update the database seeding flow, including the
organization loop around Organization and the earlier commit near line 93, to
replace intermediate commit calls with flush() when generated IDs are needed.
Commit once only after every seeding section completes successfully, preserving
rollback behavior for any failure.
- Around line 133-136: Update the NORAD allocation logic around the shown
random-ID loops and the corresponding allocation blocks at the other referenced
locations to build pools of available IDs before generation. Validate each
requested count against the pool’s remaining capacity and reject excess requests
before entering allocation, then remove retry-until-unique loops and consume IDs
from the pools while preserving uniqueness.
- Around line 117-122: Update the non-clear branch of the seeding logic to
populate used_norad_ids from the master SpaceObject.noradId query, ensuring IDs
without Satellite or Debris detail rows are reserved; retain the existing
clear-mode behavior and set-based deduplication.
In `@backend/services/weather_service.py`:
- Around line 188-190: Update sync_weather to accept a days_back parameter and
pass it to fetch_all_events instead of hardcoding days_back=1. Ensure the sync
endpoint’s requested 1–7 day range is forwarded unchanged through the existing
call path.
- Around line 192-230: Update the weather synchronization flow covering the
geomagnetic storm, solar flare, CME, and radiation event loops to persist each
event’s activity_id as a unique external key, upsert the corresponding
SpaceWeather record instead of always inserting, and associate alerts with that
key so repeated or overlapping runs do not create duplicate alerts.
In `@docker-compose.yml`:
- Around line 7-10: Remove the predictable credentials and signing-key fallbacks
from the deployment configuration. Update the environment entries for
POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB, and the backend signing key to
require explicitly supplied values, or relocate development defaults to a
clearly local-only override that deployment cannot consume.
In `@MIGRATION_SUMMARY.md`:
- Line 11: Update the schema inventory in MIGRATION_SUMMARY.md to reflect all
actual models and migrations: include the omitted counters collection and
reconcile the stated total with the complete table list. Derive and verify the
entries from the repository’s model and migration definitions before documenting
coverage.
- Around line 288-295: Update the “Rollback Procedure” section to instruct users
to restore a current MongoDB snapshot, revert to a MongoDB-compatible
application version, reinstall the required pymongo dependency, and explicitly
switch the configuration from DATABASE_URL to MONGODB_URI in .env. Remove the
invalid instruction to set DATABASE_URL to MONGODB_URI.
- Around line 239-246: Update the “From Existing MongoDB” section to document
the actual existing-data migration flow: include the MongoDB restore/import
command and validation steps if those operations exist. If no restore/import
flow is supported, rename the section to clearly describe MongoDB backup
followed by PostgreSQL sample-data seeding, rather than an existing-data
migration.
---
Minor comments:
In @.github/workflows/backend-ci.yml:
- Line 37: Update the actions/checkout@v4 step to set persist-credentials to
false, and add a minimal workflow permissions block granting only contents:
read. Keep the existing checkout behavior otherwise unchanged.
In `@backend/api/v1/endpoints/satellites.py`:
- Around line 59-61: Update the query flow using query, offset, and limit to
apply a deterministic order by the stable Satellite.id column before pagination.
Keep the existing count and page calculation behavior unchanged, ensuring offset
and limit operate on the ordered query.
In `@backend/api/v1/endpoints/weather.py`:
- Around line 121-126: Update trigger_weather_sync so the validated days
parameter is either passed to weather_service.sync_weather according to its
service contract, or removed from the endpoint if synchronization does not
support a day range; ensure the endpoint no longer accepts an unused parameter.
In `@backend/orbital/spacetrack.py`:
- Around line 202-215: Update the timestamp initialization in the
record-building method containing the returned object to use the timezone-aware
UTC datetime value from datetime.datetime.now(datetime.timezone.utc), and pass
that datetime directly as updatedAt instead of converting it with isoformat().
In `@backend/scripts/seed_db.py`:
- Around line 184-186: Update the telemetry timestamp creation in the seeding
flow around the `Telemetry` constructor to use a timezone-aware UTC datetime
instead of `datetime.datetime.utcnow()`. Preserve the existing ten-minute offset
calculation, and apply the equivalent timezone-aware UTC change to the
`CollisionPrediction` timestamp creation referenced by the comment.
In `@MIGRATION_SUMMARY.md`:
- Around line 71-74: Update the Phase 14 entry in MIGRATION_SUMMARY.md to avoid
claiming a CI/CD deployment pipeline when backend-ci.yml only runs tests: rename
the item to “CI” or document the external Railway deployment trigger and its
verification.
- Around line 258-268: Resolve the connection-pooling contradiction between the
Performance Improvements section and the Future Improvements item: update the
performance entry to state the actual SQLAlchemy pool configuration, or rename
the future item to focus on pool tuning and monitoring rather than adding
pooling. Keep both sections consistent and avoid claiming pooling is both
implemented and pending.
- Around line 272-279: Update the “Frontend” entry in the Backward Compatibility
section to acknowledge that API compatibility is preserved while frontend
deployment defaults for same-origin routing changed; do not claim that no
frontend changes are required.
---
Nitpick comments:
In `@backend/tests/test_seed.py`:
- Around line 29-36: Update test_generate_tle to recompute each TLE line’s
modulo-10 checksum from its first 68 characters, counting minus signs as one,
and assert that the result matches the final digit of line1 and line2. Replace
the weak is-digit assertions while preserving the existing length and prefix
checks.
- Around line 60-69: Update test_seed_database_no_clear to insert existing
Satellite rows before calling seed_database(count=5, clear=False), then verify
those original rows remain and five new rows are appended. Keep the test’s
patched engine and SessionLocal setup, and assert the final contents or count
demonstrates that clear=False preserves prior data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd424b68-3d9d-46b8-831a-0b9a4e9ad3eb
⛔ Files ignored due to path filters (15)
backend/app/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/config.cpython-313.pycis excluded by!**/*.pycbackend/database/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/database/__pycache__/session.cpython-313.pycis excluded by!**/*.pycbackend/models/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/models/__pycache__/db_models.cpython-313.pycis excluded by!**/*.pycbackend/orbital/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/base.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/cache.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/celestrak.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/chain.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/spacetrack.cpython-313.pycis excluded by!**/*.pycbackend/orbital_guardian.dbis excluded by!**/*.db
📒 Files selected for processing (40)
.github/workflows/backend-ci.ymlMIGRATION_SUMMARY.mdbackend/.env.examplebackend/.gitignorebackend/Dockerfilebackend/Procfilebackend/api/v1/endpoints/agents.pybackend/api/v1/endpoints/auth.pybackend/api/v1/endpoints/catalog.pybackend/api/v1/endpoints/collisions.pybackend/api/v1/endpoints/dashboard.pybackend/api/v1/endpoints/satellites.pybackend/api/v1/endpoints/weather.pybackend/app/core/config.pybackend/app/main.pybackend/database/session.pybackend/models/db_models.pybackend/nixpacks.tomlbackend/orbital/collision_engine.pybackend/orbital/providers/base.pybackend/orbital/providers/cache.pybackend/orbital/providers/celestrak.pybackend/orbital/providers/chain.pybackend/orbital/providers/spacetrack.pybackend/orbital/spacetrack.pybackend/orbital_guardian.db-shmbackend/orbital_guardian.db-walbackend/railway.jsonbackend/requirements.txtbackend/schemas/api_schemas.pybackend/scripts/seed_db.pybackend/services/weather_service.pybackend/tests/test_error_responses.pybackend/tests/test_ingestion.pybackend/tests/test_providers.pybackend/tests/test_seed.pydocker-compose.ymlfrontend/src/components/EarthTwin.tsxfrontend/src/pages/Dashboard.tsxfrontend/src/services/api.ts
| # JWT — MUST be overridden in production via the SECRET_KEY env var. | ||
| # Generate a secure value with: python -c "import secrets; print(secrets.token_hex(32))" | ||
| SECRET_KEY: str = "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Require SECRET_KEY instead of falling back to a public signing key.
A deployment missing the environment variable will use this repository-known key, allowing forged JWTs.
Proposed fix
- SECRET_KEY: str = "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7"
+ SECRET_KEY: str📝 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.
| # JWT — MUST be overridden in production via the SECRET_KEY env var. | |
| # Generate a secure value with: python -c "import secrets; print(secrets.token_hex(32))" | |
| SECRET_KEY: str = "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7" | |
| # JWT — MUST be overridden in production via the SECRET_KEY env var. | |
| # Generate a secure value with: python -c "import secrets; print(secrets.token_hex(32))" | |
| SECRET_KEY: str |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 12-12: Possible hardcoded password assigned to: "SECRET_KEY"
(S105)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/core/config.py` around lines 10 - 12, Update SECRET_KEY
configuration to require a value from the SECRET_KEY environment variable
instead of using the repository-known hardcoded default. Ensure startup fails
clearly when the variable is missing, while preserving the existing string
configuration interface.
Source: Linters/SAST tools
User description
Description
Migrated the backend database from MongoDB to PostgreSQL using Prisma ORM.
Changes Made
Replaced MongoDB/Mongoose with Prisma and PostgreSQL.
Designed a normalized PostgreSQL schema for all existing collections.
Migrated the following collections into PostgreSQL tables:
Added appropriate primary keys, foreign keys, indexes, constraints, and relationships.
Refactored database services, repositories, and API endpoints to use Prisma.
Replaced MongoDB-specific queries and aggregation pipelines with PostgreSQL-compatible implementations.
Updated authentication and authorization logic to work with the new database schema.
Added Prisma migration and seed scripts.
Migrated existing MongoDB data while preserving relationships and data integrity.
Updated environment configuration to use PostgreSQL.
Removed all MongoDB and Mongoose dependencies from the project.
Updated Docker configuration, deployment setup, and relevant documentation.
This migration preserves existing application functionality while improving data integrity, maintainability, and relational data modeling.
Related Issue
Fixes #62
Checklist
ECSoC26 Submission
ECSoC26-L1– BeginnerECSoC26-L2– IntermediateECSoC26-L3– AdvancedCodeAnt-AI Description
Move the backend from MongoDB to PostgreSQL and make production routing safer
What Changed
/api/v1in production, and dashboard errors no longer point users to a local server addressImpact
✅ Fewer broken logins and data lookups after the database move✅ Fewer production API connection failures✅ Easier local and hosted setup with PostgreSQL💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR replaces the entire MongoDB/pymongo stack with PostgreSQL + SQLAlchemy 2.0, rewriting all ORM models, session management, data-ingestion pipelines, API endpoints, and supporting infrastructure in one sweep.
database/session.pyis reduced from ~400 lines of custom MongoDB adapter code to a clean 30-line SQLAlchemyengine/SessionLocal/get_dbsetup; all models inmodels/db_models.pyare converted from hand-rolledBaseModelclasses to properly typedDeclarativeBasesubclasses with FK constraints, unique indexes, and cascade rules.orbital/spacetrack.pyswitches bulk writes from pymongoUpdateOneoperations to PostgreSQLINSERT … ON CONFLICT DO UPDATEviasqlalchemy.dialects.postgresql.insert, with a row-by-row SQLite fallback for tests.docker-compose.ymladds a Postgres 16 service and a newbackend-ci.ymlGitHub Actions workflow runs the test suite against a real Postgres instance on every push.Confidence Score: 3/5
The migration is structurally sound but two runtime defects on the data-ingestion path will silently prevent all satellite syncs and cache operations from working in a fresh deployment.
The ORM models, session setup, auth endpoints, and CI configuration are all well done. However two bugs on the hot path undermine the new setup: the provider_cache table is never created because ProviderCache is not imported before create_all(), and db.bind (removed in SQLAlchemy 2.0) is accessed to choose the upsert dialect, causing an AttributeError that silently marks every bulk satellite sync as a failure.
backend/app/main.py (missing ProviderCache import before create_all) and backend/orbital/spacetrack.py (db.bind dialect detection).
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant FastAPI participant SpaceTrackService participant ProviderChain participant PostgreSQL Client->>FastAPI: "POST /catalog/sync?group=active" FastAPI->>SpaceTrackService: sync_group(db, active) SpaceTrackService->>PostgreSQL: SELECT 1 (health check) PostgreSQL-->>SpaceTrackService: ok SpaceTrackService->>ProviderChain: fetch_group(active, 500, db) ProviderChain->>LocalCacheProvider: store(db, active, records) LocalCacheProvider->>PostgreSQL: INSERT/UPDATE provider_cache Note over LocalCacheProvider,PostgreSQL: Table missing - ProviderCache not imported at startup ProviderChain-->>SpaceTrackService: records from space-track SpaceTrackService->>PostgreSQL: INSERT INTO satellites ON CONFLICT DO UPDATE Note over SpaceTrackService,PostgreSQL: db.bind raises AttributeError in SQLAlchemy 2.0 PostgreSQL-->>SpaceTrackService: rows upserted SpaceTrackService-->>FastAPI: status dict FastAPI-->>Client: 200 APIResponse%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant FastAPI participant SpaceTrackService participant ProviderChain participant PostgreSQL Client->>FastAPI: "POST /catalog/sync?group=active" FastAPI->>SpaceTrackService: sync_group(db, active) SpaceTrackService->>PostgreSQL: SELECT 1 (health check) PostgreSQL-->>SpaceTrackService: ok SpaceTrackService->>ProviderChain: fetch_group(active, 500, db) ProviderChain->>LocalCacheProvider: store(db, active, records) LocalCacheProvider->>PostgreSQL: INSERT/UPDATE provider_cache Note over LocalCacheProvider,PostgreSQL: Table missing - ProviderCache not imported at startup ProviderChain-->>SpaceTrackService: records from space-track SpaceTrackService->>PostgreSQL: INSERT INTO satellites ON CONFLICT DO UPDATE Note over SpaceTrackService,PostgreSQL: db.bind raises AttributeError in SQLAlchemy 2.0 PostgreSQL-->>SpaceTrackService: rows upserted SpaceTrackService-->>FastAPI: status dict FastAPI-->>Client: 200 APIResponseComments Outside Diff (2)
backend/app/main.py, line 129-133 (link)ProviderCachetable never created at startupBase.metadata.create_all()only creates tables for models that have been imported before it runs. TheProviderCachemodel lives inorbital/providers/cache.pyand is never imported during startup — onlymodels.db_modelsis. As a result, theprovider_cachetable is absent in a fresh deployment. The first timeLocalCacheProvider.store()orfetch_group()is called, the query will fail withProgrammingError: relation "provider_cache" does not exist. Addimport orbital.providers.cache # noqa: F401before thecreate_all()call to register the model withBase.metadata.Prompt To Fix With AI
backend/orbital/spacetrack.py, line 548 (link)Session.bindremoved in SQLAlchemy 2.0db.bindwas removed from theSessionAPI in SQLAlchemy 2.0. Accessing it raisesAttributeError, so theif db.bind else "postgresql"guard never reaches the fallback — the exception propagates to theexcept Exceptionblock insync_group, reporting every sync as awrite_exception. The correct way to get the dialect in 2.x isengine.dialect.namedirectly (theengineobject is already imported fromdatabase.session).Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Migrate Database from MongoDB to Postgre..." | Re-trigger Greptile