Fix migration workflow: upload as artifact instead of scp to practice EC2
This commit is contained in:
@@ -0,0 +1,279 @@
|
||||
# 27 — Technical debt
|
||||
|
||||
Only items with a clear technical justification are listed. Architectural
|
||||
choices that are deliberate and documented in-code (fail-open trace writes, one
|
||||
entailment pass, character-exact number comparison, quarantine over
|
||||
reconstruction) are **not** debt and are not listed here.
|
||||
|
||||
Priority: **P0** production risk now · **P1** likely to cause an incident or
|
||||
block work · **P2** real but tolerable · **P3** cleanliness.
|
||||
|
||||
---
|
||||
|
||||
## P0
|
||||
|
||||
### D-01 — No test gate before production deploy
|
||||
|
||||
**Evidence** `.github/workflows/deploy.yml` is the only workflow; it triggers on
|
||||
`push: master` and goes straight to SSH + `docker compose up --build`. No
|
||||
`ruff`, no `pytest`, no `tsc`, no `pull_request` trigger. `ruff` is configured in
|
||||
`apps/ai-service/pyproject.toml` and never invoked.
|
||||
**Impact** A commit that breaks all 555 passing tests deploys to a live medical
|
||||
reference tool. The only automated check is one smoke query after the fact.
|
||||
**Risk** High — the safety mechanisms (grounding, entailment, quarantine, scope
|
||||
gates) are exactly what the tests cover.
|
||||
**Remediation** Add a workflow running both suites (`EMBEDDING_PROVIDER=disabled`
|
||||
for ai-service) plus `ruff check` and `turbo run lint build`, on `push` and
|
||||
`pull_request`; make `deploy` `needs:` it.
|
||||
|
||||
### D-02 — Committed default PostgreSQL credential
|
||||
|
||||
**Evidence** `infra/docker/docker-compose.prod.yml` sets
|
||||
`POSTGRES_USER: duoc_thu` / `POSTGRES_PASSWORD: duoc_thu`;
|
||||
`infra/helm/medical-chatbot/values.yaml` sets `secret.postgresPassword:
|
||||
duoc_thu` and `grafanaAdminPassword: change-me`.
|
||||
**Impact** A default credential in version control. Bounded today because
|
||||
PostgreSQL publishes no host port, but the database holds raw user queries that
|
||||
can contain patient context, and the Helm path would carry it into a cluster.
|
||||
**Remediation** Generate a password, inject via `.env.prod` / a Kubernetes
|
||||
Secret, and remove the literals from both files.
|
||||
|
||||
### D-03 — No backup for either datastore
|
||||
|
||||
**Evidence** No dump job, cron, snapshot script or restore procedure anywhere in
|
||||
the repository. Docker named volumes on a single EC2 host.
|
||||
**Impact** Losing the host loses every trace, conversation and feedback row, and
|
||||
requires a full Qdrant re-load — whose embed step **costs real Bedrock spend**.
|
||||
**Remediation** A scheduled `pg_dump` and a Qdrant snapshot to S3, plus a
|
||||
written restore drill.
|
||||
|
||||
---
|
||||
|
||||
## P1
|
||||
|
||||
### D-04 — In-process agent state blocks horizontal scaling, silently
|
||||
|
||||
**Evidence** `rag/agent.py` keeps `_last_frame` and `_clarify_streak` as plain
|
||||
dicts. `PostgresConversationStore` replaces `_history` only.
|
||||
**Impact** With more than one replica, the structured prior-frame merge (which
|
||||
stops the model re-asking an answered clarify) and the clarify circuit breaker
|
||||
both become per-replica. Multi-turn quality degrades and nothing detects it.
|
||||
**Remediation** Persist both alongside the conversation turns, or document a
|
||||
hard single-replica constraint in the chart and enforce it.
|
||||
|
||||
### D-05 — Test suite cannot be collected without an undocumented env var
|
||||
|
||||
**Evidence** `main.py` calls `build_runtime()` at module scope;
|
||||
`tests/test_api.py` imports `main`; with the repo's `.env` present, collection
|
||||
raises `ResponseHandlingException` against Qdrant.
|
||||
**Impact** A new contributor's first `pytest` run fails in a way that looks like
|
||||
a broken suite. Reproduced this session.
|
||||
**Remediation** Either move runtime construction behind a factory the tests can
|
||||
avoid (an app factory already exists — `create_app`), or add a `conftest.py` that
|
||||
sets `EMBEDDING_PROVIDER=disabled`. The latter is a two-line change.
|
||||
|
||||
### D-06 — No `.env.example`
|
||||
|
||||
**Evidence** `git ls-files` shows no env template; the only env file is the
|
||||
gitignored `apps/ai-service/.env`. `config.py` is the sole record of ~22
|
||||
settings.
|
||||
**Impact** Nobody can configure the service without reading the source, and
|
||||
production's `.env.prod` cannot be reviewed or reconstructed.
|
||||
**Remediation** Commit `apps/ai-service/.env.example` with every key, safe
|
||||
defaults and a comment per secret.
|
||||
|
||||
### D-07 — No Python lockfile; the Dockerfile duplicates and diverges from `pyproject.toml`
|
||||
|
||||
**Evidence** `apps/ai-service/Dockerfile` pip-installs a hand-written list
|
||||
including `boto3` **with no version bound**, and comments that this mirrors
|
||||
`pyproject.toml` plus extras. `pyproject.toml` itself does not declare `boto3`
|
||||
at all, though `adapters/` imports it.
|
||||
**Impact** Two builds of the same commit can install different versions; the
|
||||
declared dependency set is incomplete; a boto3 breaking change reaches
|
||||
production unannounced.
|
||||
**Remediation** Declare `boto3` in `pyproject.toml`, generate a lockfile
|
||||
(`uv`/`pip-compile`), and have the Dockerfile install from it.
|
||||
|
||||
### D-08 — `search_lexical` uses `MatchText` on an unindexed payload field
|
||||
|
||||
**Evidence** `INDEXED_PAYLOAD_FIELDS` in `ingestion/load/models.py` contains no
|
||||
entry for `text`; `adapters/qdrant.py::search_lexical` builds
|
||||
`FieldCondition(key="text", match=MatchText(...))` conditions.
|
||||
**Impact** Qdrant requires an explicit full-text index for `MatchText`. Without
|
||||
one those conditions may not filter as intended, which would make the lexical
|
||||
route depend entirely on the Python re-scoring of whatever the scroll returned.
|
||||
The neighbour-pooling and patient-safety facet routes both use it.
|
||||
**Remediation** Verify the deployed collection's index list; if absent, add a
|
||||
`text` field index to `INDEXED_PAYLOAD_FIELDS` and create it on the existing
|
||||
collection.
|
||||
|
||||
### D-09 — Rate limiting is in-memory, in the wrong tier, and does not cover every route
|
||||
|
||||
**Evidence** `apps/web/middleware.ts` — per-process counters, IP-keyed, rules
|
||||
only for `/api/chat` and `/api/suggest`. `/api/pdf` (37 MB per request) and
|
||||
`/api/feedback` fall through unlimited. The file documents the first two
|
||||
limitations itself.
|
||||
**Impact** The only guard on unauthenticated Bedrock spend does not survive a
|
||||
second replica, and a 37 MB endpoint is unthrottled.
|
||||
**Remediation** Add rules for the remaining routes now; move counters to Redis
|
||||
(already reserved for this) or to the gateway when it exists.
|
||||
|
||||
### D-10 — No evaluation runner despite substantial evaluation material
|
||||
|
||||
**Evidence** 209 hand-labelled rows in `Golden Dataset/*.csv` read by no code;
|
||||
`rag/condition_evaluation.py` and `rag/evaluation.py` implement full metric
|
||||
summaries with no production caller; `rag/run_eval.py` measures the in-memory
|
||||
retriever, not Qdrant.
|
||||
**Impact** No retrieval or answer-quality number can be reproduced, so no
|
||||
regression is detectable. Assertions about RAG quality cannot be supported.
|
||||
**Remediation** Wire `evals/condition_to_drug_v1.jsonl` through the live service
|
||||
into `summarize_condition_outcomes`, store a baseline, and fail on regression.
|
||||
|
||||
### D-11 — Zero frontend tests for code that encodes fixed production bugs
|
||||
|
||||
**Evidence** No test script, no test files, no runner in `apps/web` or
|
||||
`packages/*`. The untested code includes the 65 s timeout derivation, abort
|
||||
handling, the Strict-Mode duplicate-request guard, the ~25-entry `REFUSALS` map
|
||||
and the citation-grouping logic — each added in response to a real incident,
|
||||
each documented in a comment.
|
||||
**Impact** Silent regression of user-visible safety wording and behaviour.
|
||||
**Remediation** Vitest + Testing Library for `route.ts` mapping and
|
||||
`middleware.ts` rules first; those are pure functions and cheap to cover.
|
||||
|
||||
---
|
||||
|
||||
## P2
|
||||
|
||||
### D-12 — Dead code: three tested modules with no runtime caller
|
||||
|
||||
**Evidence** Verified by import-graph grep: `rag/fusion.py`
|
||||
(`reciprocal_rank_fusion`), `rag/expansion.py` (`expand_siblings`) and
|
||||
`rag/calculators.py` (`body_surface_area_m2`) are referenced only by their own
|
||||
tests.
|
||||
**Impact** ~140 lines plus 8 tests suggesting capabilities the system does not
|
||||
have. `calculators.py` is the notable one — it was written specifically so a
|
||||
BSA-based dose would be computed rather than read off a quarantined table, and
|
||||
that never happened.
|
||||
**Remediation** Wire `calculators.py` into the dosing path or record why not;
|
||||
delete or clearly mark `fusion.py`/`expansion.py` as unused experiments.
|
||||
|
||||
### D-13 — Four Prometheus metrics registered but never incremented
|
||||
|
||||
**Evidence** `duocthu_loop_retrieval_rounds_total`, `duocthu_loop_refined_total`,
|
||||
`duocthu_loop_repaired_total`, `duocthu_followup_inherited_total` appear only in
|
||||
`rag/metrics.py` and in the registration/help tables of
|
||||
`adapters/prometheus.py`. Leftovers of the retired ADR 0007 loop.
|
||||
**Impact** Permanently-zero series that read as "this never happens" rather than
|
||||
"this is not measured". A dashboard panel on them would be misleading.
|
||||
**Remediation** Delete them, or re-point them at the paths that replaced the loop.
|
||||
|
||||
### D-14 — Optional retriever capabilities discovered by `getattr`, not by protocol
|
||||
|
||||
**Evidence** `rag/service.py` probes `find_by_section`, `find_by_indication`,
|
||||
`search_indication`, `search_lexical` and `find_by_drug` with
|
||||
`getattr(self._retriever, name, None)`. `rag/ports.py` declares only `search`,
|
||||
`find_by_section` (on a separate `SectionRetriever`) and `ParentStore.get`.
|
||||
**Impact** A retriever missing a method silently disables a whole route instead
|
||||
of failing loudly, and the real interface is undocumented.
|
||||
**Remediation** Declare the full capability set in `ports.py` as explicit
|
||||
optional protocols.
|
||||
|
||||
### D-15 — Hand-maintained contract between Pydantic models and TypeScript DTOs
|
||||
|
||||
**Evidence** `routers/rag.py` response models, `packages/shared-types/src/dto/
|
||||
chat.ts`, and the snake_case→camelCase mapping in
|
||||
`apps/web/app/api/chat/route.ts` are three independent hand-written copies of
|
||||
one contract.
|
||||
**Impact** A new backend field is silently dropped until three files are edited;
|
||||
a renamed field fails at runtime, not at build time.
|
||||
**Remediation** Generate the TS types from the OpenAPI schema FastAPI already
|
||||
serves.
|
||||
|
||||
### D-16 — Container images run as root with build tooling included
|
||||
|
||||
**Evidence** Neither Dockerfile has a `USER`; `apps/ai-service/Dockerfile`
|
||||
installs `gcc` into the runtime image; `apps/web`'s runtime stage copies the
|
||||
whole `/repo` rather than Next's standalone output.
|
||||
**Impact** Larger attack surface and image size than necessary.
|
||||
**Remediation** Multi-stage build for ai-service, non-root user in both,
|
||||
`output: "standalone"` for Next.
|
||||
|
||||
### D-17 — No Kubernetes hardening in the Helm chart
|
||||
|
||||
**Evidence** No `securityContext`, `runAsNonRoot`, `readOnlyRootFilesystem`,
|
||||
`NetworkPolicy`, `PodDisruptionBudget` or HPA in
|
||||
`infra/helm/medical-chatbot/templates/`. A `ServiceAccount` is created with no
|
||||
RBAC bound.
|
||||
**Impact** Not a current production risk (the chart is unapplied) but it is the
|
||||
target state.
|
||||
**Remediation** Add them before the chart is ever applied.
|
||||
|
||||
### D-18 — Helm chart cannot produce a working deployment as written
|
||||
|
||||
**Evidence** `values.yaml` defaults `embeddingProvider`/`answerProvider` to
|
||||
`disabled`, and the bundled Qdrant starts empty — against which `ai-service`'s
|
||||
manifest check refuses to start. There is no corpus-load Job in the chart, and
|
||||
no image registry produces `duocthu-ai-service:<tag>`.
|
||||
**Impact** The documented target deployment path is not runnable.
|
||||
**Remediation** Add a corpus-restore Job (or document a snapshot prerequisite)
|
||||
and produce tagged images in CI.
|
||||
|
||||
### D-19 — Migrations are forward-only with no version tracking
|
||||
|
||||
**Evidence** `migrate.py` replays every `migrations/*.sql` on each deploy; all
|
||||
are `IF NOT EXISTS`. No version table, no down scripts, no ordering guard beyond
|
||||
filenames.
|
||||
**Impact** Works today because the migrations are trivially idempotent; the
|
||||
first non-idempotent migration breaks it, and rollback across one is uncovered.
|
||||
**Remediation** Adopt a real migration tool, or add a `schema_migrations` table.
|
||||
|
||||
### D-20 — Per-call PostgreSQL connections with no pool
|
||||
|
||||
**Evidence** `adapters/postgres.py` — both classes open a fresh
|
||||
`psycopg.connect()` per call. Both docstrings acknowledge it (F-09).
|
||||
**Impact** Connection setup on every trace write, history read and history
|
||||
append: three round trips of connection overhead per turn. Bounded by
|
||||
`connect_timeout=5`, which is what keeps it from being worse.
|
||||
**Remediation** `psycopg_pool` with a startup lifecycle.
|
||||
|
||||
---
|
||||
|
||||
## P3
|
||||
|
||||
### D-21 — Unpinned `qdrant/qdrant:latest`
|
||||
|
||||
Every other image is pinned. A rebuild can move the Qdrant version underneath a
|
||||
loaded collection.
|
||||
|
||||
### D-22 — Routine timings logged at WARNING
|
||||
|
||||
`rag/agent.py` logs per-turn timing unconditionally at `warning` level, with a
|
||||
comment explaining that uvicorn does not wire handlers onto the root logger so
|
||||
`info` would go nowhere. The instrumentation is temporary (added 2026-08-07 to
|
||||
chase a specific bug) and now duplicates
|
||||
`duocthu_stage_duration_seconds`. It also pollutes the WARNING level, which
|
||||
makes real warnings hard to spot.
|
||||
|
||||
### D-23 — Repository root and working tree noise
|
||||
|
||||
~25 untracked `.codex-*.log` / `.codex-*.png` files, `tmp/`, `output/`,
|
||||
`.venv_docling_test/`, `.next/` and `.turbo/` at the root; `apps/ai-service/`
|
||||
holds a dozen `.codex-live-80xx.*.log` files. None is gitignored.
|
||||
|
||||
### D-24 — `@duoc-thu/api-client` is a declared but unused dependency
|
||||
|
||||
`apps/web` depends on it and the live chat path calls `fetch("/api/chat")`
|
||||
directly. Either adopt it or drop the dependency.
|
||||
|
||||
### D-25 — `SECTION_ORDER` omits `ten_thuong_mai`
|
||||
|
||||
`rag/sections.py::SECTION_ORDER` has 18 entries while `SECTION_KEYS` (and the
|
||||
corpus) has 19. In `find_by_drug`, a `ten_thuong_mai` chunk sorts to the end via
|
||||
the `order.get(..., len(order))` default rather than into its book position.
|
||||
|
||||
### D-26 — Duplicated section vocabulary across two projects
|
||||
|
||||
The 19 keys are defined independently in `ingestion/segment/vocab.py`,
|
||||
`apps/ai-service/rag/sections.py` and `apps/ai-service/rag/understanding.py`.
|
||||
The separation between the two deployables is deliberate, but the two copies
|
||||
inside `ai-service` are not.
|
||||
Reference in New Issue
Block a user