Repository navigation
fix(deploy): fix broken container image, ingress routing, and CI coverage gaps - #299
Merged
Merged
Conversation
…prompt-injection guards (round 9) The previous batch commit (1a7c9de) introduced several unnoticed regressions alongside its 82 issue fixes, found via ruff/pytest plus a targeted 6-agent review of that same diff: - router/graph.py: guard disabled the documented mid-confidence fallback for the default threshold (0.7 == HIGH_CONFIDENCE) - router/schema.py: extra="ignore" weakened the OWASP-LLM01 strict-JSON guard back to extra="forbid" - router/prompts.py: query sanitization targeted <<< / >>> instead of the actual << / >> fence tokens, making it a no-op against fence-forgery - backend/app/health.py: /ready leaked the dependency breakdown to any request carrying an unregistered X-API-Key (presence check, not validation) - backend/app/api/schemas.py: response-side Document.domain over-tightened to the write-side pattern, 500ing on any existing non-conforming value - db/qdrant_bootstrap.py: vector-size verification ran even on freshly created collections instead of only pre-existing ones - db/models.py: uq_documents_tenant_hash index predicate drifted from migration 004's tombstoned_at fix (issue #206) - ingestion/pipeline.py + backend/app/adapters/ingestor.py: non-GLOBAL residency_region silently dropped to the DB default on the fallback-only ingest path (DPDP FR-028) - backend/app/productization/roi_calculator.py: docstring stale vs. the actual positive-only validation contract Also: normalized 15 Python files flagged by the project's ASCII-only standard (em-dash/section-sign/arrow -> ASCII, stripped a stray UTF-8 BOM), and cleared all pre-existing ruff findings (silent except:pass, unsorted imports, import-not-at-top, line length). All 331 backend tests + 64 frontend tests pass; ruff and eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rage gaps Continuation of the code-review pass (round 10 - deploy/CI + migrations scope, reviewed by a devops-engineer-persona agent since the earlier rounds only covered Python backend/db/ingestion/router + frontend). - Dockerfile: builder and runtime stages only copied backend/ and ingestion/, but the app imports db.models and router directly as top-level packages (declared in pyproject.toml's [tool.setuptools] packages list). The image would fail to build or crash at runtime with ModuleNotFoundError. Added COPY router ./router and COPY db ./db to both stages. - deploy/k8s/ingress.yaml: the /api path used plain Prefix matching with no rewrite, but the backend mounts routes without an /api prefix (/health, /ready, /v1/*, per backend/app/main.py) -- matching what Caddyfile.example already strips via handle_path. Every request routed through the Kubernetes Ingress would have 404'd. Fixed with a regex capture path + rewrite-target annotation mirroring the Caddy behavior. - .github/workflows/ci.yml: added a frontend-test job (eslint + tsc + vitest were never run in CI, only npm audit) and extended the ruff lint job to cover router/ and db/ (already covered by typecheck/test/security jobs and by ruff's own [tool.ruff] src config, just missing from the lint command). Migrations 001-004, docker-compose (dev + prod), both Dockerfiles' security posture, k8s api-deployment.yaml, Caddyfile.example, and the .env.example files were all reviewed and found correct -- no changes needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…test bug Discovered by actually inspecting CI results for #298 rather than trusting local ruff+pytest alone -- mypy, bandit, and pip-audit were never run locally in this session, and CI turned out to already be red on main from the batch commit onward (over a month), for reasons unrelated to the round-9 fixes. - backend/app/api/interfaces.py: GenerationLLM.stream_answer's Protocol stub body was `...` with no `yield`, so mypy inferred calling it returns Coroutine[Any, Any, AsyncIterator[str]] instead of AsyncIterator[str] directly -- breaking `async for` at every call site through the Protocol type (backend/app/api/answer.py) and forcing two now-dead `type: ignore` workarounds in dependencies.py's singleton. Fixed the stub to be a real (unreachable) async generator so mypy treats it correctly; removed the dead ignores. - db/qdrant_bootstrap.py: `_verify_vector_size` accessed `.size` on a `VectorParams | dict[str, VectorParams] | None` union without narrowing. Added an isinstance guard that raises a clear RuntimeError for named-vector collections instead of leaving an unchecked AttributeError risk. - router/tests/test_router.py: removed a stale `type: ignore[arg-type]` that mypy now reports as unused. - pyproject.toml: pinned setuptools>=83.0.0 (build-system and dev extras) -- pip-audit flagged the installed 79.0.1 for PYSEC-2026-3447. This vulnerability was previously masked because bandit failed first in the same CI job (on the S110 issue this round's earlier commit already fixed), and pip-audit never got to run. - db/tests/test_schema.py: `test_forward_then_rollback_restores_baseline` used sync `create_engine` against `DATABASE_URL`, which CI sets to a `postgresql+asyncpg://` DSN (asyncpg is the project's only Postgres driver) -- raising `sqlalchemy.exc.MissingGreenlet`. Converted to `create_async_engine` + `conn.run_sync(inspect(...))`; added the matching `anyio_backend` fixture to db/tests/conftest.py. This test is skipped locally (no DATABASE_URL) so it was never caught outside CI. ruff, mypy, and pytest (330 passed, 1 skipped -- the live-DB test, correctly skipped without DATABASE_URL) are all clean locally after this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
….py edit The auth.py E402 fix earlier in this round moved a module-level constant below the imports, shifting PrincipalKind.API_KEY (the acknowledged Secret-Keyword false positive) from line 36 to line 38. detect-secrets' baseline compares findings by exact line number, so the shift made CI treat the same already-acknowledged false positive as a brand-new secret and fail the security job. Updated the recorded line_number to match; left every other baseline entry untouched (no new scan performed, to avoid the Windows-vs-Linux path-separator mismatch a native rescan would introduce). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DATABASE_URL was set to a postgresql+asyncpg://localhost:5432 DSN with no `services:` block provisioning that server, so test_forward_then_rollback_restores_baseline (which runs for real rather than skipping, since CI does set DATABASE_URL) always failed on connection refused -- previously masked by the sync/async engine bug fixed earlier in this pass, which failed before ever reaching the network. Added a postgres:16 service container with a pg_isready health check, matching the test/test/test credentials already baked into DATABASE_URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DATABASE_URL was set to a postgresql+asyncpg://localhost:5432 DSN with no `services:` block provisioning that server, so test_forward_then_rollback_restores_baseline (which runs for real rather than skipping, since CI does set DATABASE_URL) always failed on connection refused -- previously masked by the sync/async engine bug fixed earlier in this pass, which failed before ever reaching the network. Added a postgres:16 service container with a pg_isready health check, matching the test/test/test credentials already baked into DATABASE_URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test_forward_then_rollback_restores_baseline reached a live Postgres for the first time after the MissingGreenlet fix, surfacing a second real bug: SQLAlchemy's asyncpg dialect always executes through prepared statements, and Postgres's extended query protocol rejects multiple SQL commands in a single prepared statement -- every migration file here is multi-statement DDL. conn.execute(text(sql)) failed with "PostgresSyntaxError: cannot insert multiple commands into a prepared statement" for every migration, then cascaded into unrelated test failures (event-loop-closed noise) later in the same CI run. Added _exec_script(), which runs the script through conn.get_raw_connection().driver_connection.execute(sql) -- the raw asyncpg connection's simple query protocol, which has no such restriction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ation test Adding a real Postgres service to CI (previous commit) surfaced a real bug: test_unconfigured_dependencies_report_degraded constructed Settings(database_url=None, qdrant_url=None, ...) using the Python attribute names, but Settings has no populate_by_name=True and its fields are aliased (alias="DATABASE_URL" etc.) with extra="ignore" -- so those two kwargs were silently dropped as unrecognized extras, and the settings fell through to the real DATABASE_URL env var instead of the intended None override. This was invisible before because CI had no Postgres server either way, so the env-var fallback also produced "down" by coincidence. Every other Settings(...) construction in the test suite already uses the alias spelling (DATABASE_URL=, JWT_SECRET=, ...); fixed this one call site to match. Also fixed a new mypy union-attr error from the prior commit's _exec_script helper: AsyncConnection.get_raw_connection()'s driver_connection is typed Any | None. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round 10 of the code-review pass -- deploy/CI + migrations scope (devops-engineer persona), following up on the earlier backend/db/ingestion/router/frontend rounds.
db/orrouter/, even though the app importsdb.modelsandrouterdirectly andpyproject.tomldeclares both as packages. Image build/runtime would fail withModuleNotFoundError./apiused plainPrefixmatching with no rewrite, so every request through the Ingress would 404 against the backend's un-prefixed routes (/health,/v1/*). Fixed to mirror theCaddyfile.exampleprefix-stripping behavior.npm audit); the Python ruff job omittedrouter/anddb/even though typecheck/test/security jobs already covered them.Migrations 001-004, both docker-compose files, both Dockerfiles' security posture,
k8s/api-deployment.yaml,Caddyfile.example, and.env.examplefiles were reviewed and found correct.Test plan
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com