diff --git a/CLAUDE.md b/CLAUDE.md index 1b081324..a65cfc13 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -322,7 +322,7 @@ Learnings are stored per-connection by default (`cross_connection_learnings_enab ### Storage -- App data: SQLite in dev (`backend/data/agent.db`), PostgreSQL in production (`DATABASE_URL`). +- App data: SQLite in dev (`backend/data/agent.db`), **Supabase PostgreSQL 17.6 in production since 2026-08-29** (`DATABASE_URL`). Reached through the **Supavisor pooler on port 5432 (session mode)**, and both halves of that are forced rather than chosen: `db..supabase.co` publishes an AAAA record and no A record while Heroku dynos have no outbound IPv6, so the pooler is the only route; and transaction mode (6543) breaks the named prepared statements SQLAlchemy's asyncpg dialect uses by default, intermittently. Revisit session mode when connection count is a measured constraint, not before. The former Heroku database is still attached as `HEROKU_PG_ROLLBACK_URL` — it was read, never written, and is the only rollback. **`public` is not served by the Data API** and RLS is on all 65 tables with no grants to `anon`/`authenticated`; see `scripts/migrate_to_supabase.sh` for why that is load-bearing rather than tidy. - Vectors: **`VECTOR_STORE_BACKEND` picks the backend** — `pgvector` (Postgres table `doc_embeddings`, one row per chunk, HNSW `vector_cosine_ops`) or `chroma` (the default, `CHROMA_PERSIST_DIR` or `CHROMA_SERVER_URL`; collections named `project_{project_id}`). **ChromaDB's persist dir on Heroku is the container filesystem** — wiped on every dyno restart, and `web`/`worker` are separate process types with separate copies. An empty store makes `pipeline_runner` set `force_full`, a full rebuild costs 12 039 s against the nightly ceiling of 7 200 s, so the store was empty again by morning: **`index_repo` completed 16 times in 94 runs**. Embeddings are identical across backends (bundled ONNX `all-MiniLM-L6-v2`, 384-d) and the metric matches the `{"hnsw:space": "cosine"}` the collections were created with, so the swap does not move retrieval ranking. pgvector is available on both deployments (0.8.1 Heroku, 0.8.2 Supabase). Requires Postgres — the migration is a deliberate no-op on SQLite, and asking for pgvector there fails at start-up saying so. - BM25 snapshots: `backend/data/bm25/{project_id}.json.gz` and `schema_{connection_id}.json.gz` — **gzip JSON, not pickle, since 2026-08-21 (F-KNOW-06)**: `pickle.load` executes its payload, and `BM25_DATA_DIR` is configurable. The tokenized corpus is stored and `BM25Okapi` is rebuilt on load; a leftover `.pkl` is deleted, never read. Both are rebuilt from Postgres at start-up when missing (`app/ops/bm25_local_reconcile.py`). - Redis (`REDIS_URL`): rate limiting, agent concurrency tokens, WS tickets, ARQ task queue. In-memory fallback for dev — keep it working when adding Redis features. diff --git a/docs/qa-audit/issues.md b/docs/qa-audit/issues.md index bb924770..4b19bef7 100644 --- a/docs/qa-audit/issues.md +++ b/docs/qa-audit/issues.md @@ -92,12 +92,12 @@ frontend **A** (563 smells, 7 SOLID). | Severity | Open | |---|---| | 🔴 Critical | 0 | -| 🟠 High | **1** | +| 🟠 High | **0** | | 🟡 Medium | 3 | | 🟢 Low | 26 | | ⚪ Info | 12 | -*Counted 2026-08-28, not estimated: **33 open `F-` rows and 76 struck** by `grep -cE '^\| F-'` / `grep -cE '^\| ~~F-'` over this file, plus **9 open `CB-` rows** those two commands do not see. The severity table above counts all 42 open rows of both kinds, which is why it does not match the `F-` figure — the two measure different sets and each says which. Both are derived from the rows themselves.* +*Counted 2026-08-29, not estimated: **33 open `F-` rows and 76 struck** by `grep -cE '^\| F-'` / `grep -cE '^\| ~~F-'` over this file, plus **8 open `CB-` rows** those two commands do not see. The severity table above counts all 41 open rows of both kinds, which is why it does not match the `F-` figure — the two measure different sets and each says which. Both are derived from the rows themselves.* *(R1+R2 closed 4 High + 8 Medium + 3 Low. R3 (`fbf8112`) closed 2 High (F-SSH-08, F-RULE-01) + 5 Medium (F-RULE-05, F-DG-07/09, F-GRAPH-01, F-LEARN-07) + 1 Low (F-SSH-06). The 2026-07-19 UX @@ -401,7 +401,7 @@ maintainability / reliability risks. | ~~CB-SEN1~~ | ✅ | ~~**Sentry was reachable by two secrets neither scrubbing layer could see**~~ — the built-in `EventScrubber` matches key names and its 33-key default carries neither `dsn` nor `database_url`; `before_send` matched values but walked only `exception.values`, `logentry` and `breadcrumbs`. A key of either name in `extra` or `contexts` was caught by **neither**. Urgent rather than theoretical from the moment `SENTRY_DSN` was set in production. **Fixed 2026-08-26** (#230): layer 1 wired with the denylist extended 33 → 39, layer 2 walks `extra` and `contexts` recursively and depth-bounded, host preserved. Fifteen tests, one of them an assertion about *Sentry* — that layer 1 alone still leaks values — so the redundancy question re-opens from a red test rather than from memory. | | ~~CB-SEN2~~ | ✅ | ~~**The Sentry release would have been blank on the container stack**~~ — `HEROKU_SLUG_COMMIT`, the value every guide names, is populated only for slug (buildpack) deploys. This app is on the **container** stack, where the variable exists and is always **empty** (measured on v271 *after* `runtime-dyno-metadata` was enabled). Issues would attach to a release with no commits and suspect-commit attribution would silently do nothing. Enabling the labs feature was necessary and not sufficient, and nothing would have said so. **Fixed 2026-08-26** (#231): the commit is baked into the image via `--build-arg GIT_SHA` → `ENV RELEASE`; verified in production, `RELEASE == main` HEAD. The empty string is the trap — `os.getenv` returns `""` there, not `None`, so an `is None` check would have accepted it; a test catches that form. | | CB-UX1 | ⚪ | **102 UX scenarios carry a verification older than 30 days.** 110 of 127 were dated 2026-07-19 while 152 commits had landed since; five were re-audited 2026-08-26 and the ceiling now stands at 105, of which 102 still have a changed Coverage file under them. Ordered and computable: `python3 scripts/ux_verification_status.py --backlog 2026-07-19`. The ceiling in `tests/unit/docs/test_ux_scenarios.py` may fall but not rise. | Re-audit in batches, worst first; date each verdict and add an `SCN-NNN` anchor so a machine can check it (21 of 127 have one). | -| CB-KNOW2 | 🟠 | **The vector store lived on a disk that every dyno restart wipes, and that is why the repo index almost never completes.** `CHROMA_PERSIST_DIR` is `/app/data/chroma` — the Heroku container filesystem — and `web`/`worker` are separate process types with separate copies. Measured 2026-08-27, five hours after a deploy: `repair_embeddings` fired ("Vector store empty but 758 docs in DB"), the pipeline set `force_full`, `code_symbol_embed` ran 38 min over all 8 646 files, and the run was reaped at the 7 200 s ceiling on document 380 of 758 — so the store was empty again the next night. **`index_repo` 16 completions in 94 runs; `daily_sync` 13 in 91.** The C3 self-repair was correct; it cost more than the budget allowed. **Fix written, not yet enabled:** `PgVectorStore` on branch `feat/pgvector-store`, verified end to end against the production database (same four documents, same ranking, distances 0.2234 / 0.7867 / 1.0161), with `VECTOR_STORE_BACKEND` still defaulting to `chroma`. | Deploy, flip `VECTOR_STORE_BACKEND=pgvector`, run one full re-index and require `pipeline_end`; then watch a deploy happen and the next nightly run stay incremental. Strike on that, not on the merge. | +| ~~CB-KNOW2~~ | 🟢 | **CLOSED 2026-08-28, on the event that caused it.** The vector store is in Postgres (`doc_embeddings`, HNSW `vector_cosine_ops`), `VECTOR_STORE_BACKEND=pgvector` is live, and a full rebuild reached `pipeline_end` — checkpoint deleted, which only happens on success, and the chained sync ran unprompted (`code_db_sync` 20:01:51→20:02:51). Then the proof: **34 038 vectors before restarting both dynos, 34 038 after**, and visible to a *fresh* one-off dyno — the half ChromaDB could never do, since `web` and `worker` hold separate filesystems. Repeated across a real deploy (v286), not just `ps:restart`. `handle.count()` reads 34 038, so `force_full` no longer fires and the nightly run stays incremental. Embeddings unchanged (same ONNX model, same cosine metric); ranking verified identical to four decimals. Cost, stated because it is real: the rebuild went 12 039 s → 15 051 s before the write batch was split from the embed batch (#243), and the ceiling followed the measurement to 21600. | | CB-OPS1 | 🟡 | **Measured on a rebuild that finished, 2026-08-27.** Peak **1 246 MB (112.7 %)**, **45 × R14**, **0 × R15** across the run that reached `pipeline_end` at 15:51:39 on Standard-2X. Every R14 falls in `generate_docs`, not `graph_build`. This row was struck earlier on "zero R14/R15" — but every run behind that claim died in or before `code_symbol_embed` and never reached the late steps where memory peaks, so it measured a truncated rebuild. What the resize genuinely bought, stated precisely: before it, 170 × R14 **and 2 × R15** with a 1 143 MiB peak against a 512 MiB quota — the process was killed. Now it runs 12 % over quota for hours and survives. | Decide from the number: accept a permanently over-quota worker, take the next size up, or make `generate_docs` hold less state. Not urgent — no `R15` in 3.34 h — but it is over quota by design now, and that should be a choice. | | CB-OPS2 | 🟡 | **The nightly cron can rebuild a repository the "Re-index repository" button never can.** `run_repo_index_task` is called by two ARQ jobs carrying two ceilings: the cron's `run_daily_project_knowledge_sync` at 7200 s, and `run_repo_index` at 1800 s. Measured on the same repository from `indexing_runs`: nightly `completed` in **42.4 min** (08-25 22:00), manual `TimeoutError` at **exactly 1800.02 s** inside `_run_code_symbol_embed` (08-27 09:30). Diagnosed once already — AUD-0819-20 added the knob on 2026-08-19 for this failure and left the default at the value just measured as too small. **Fix written, not yet in production:** `repo_index_job_timeout_seconds` defaults to 3600 on branch `fix/repo-index-ceiling`, with both orderings asserted in `tests/unit/services/test_repo_index_ceiling.py`. | Deploy, then force one manual full re-index and require it to reach `pipeline_end`; strike this row on that evidence, not on the merge. | | CB-OPS3 | 🟡 | **A worker restart during a repo index loses the run, and nothing retries it.** Measured 2026-08-27: release `v279` restarted the worker at 10:59:34 UTC, 37 min into an index. arq logged `shutdown on SIGTERM ◆ 0 jobs complete ◆ 2 failed ◆ 0 retries ◆ 1 ongoing to cancel`, exited 143, and the fresh worker started at 10:59:45 with **no job re-queued** — no `run_repo_index` or `run_daily_project_knowledge_sync` start appears in the next 30 min of worker log. `WorkerSettings` sets neither `retry_jobs` nor `max_tries`, so arq's defaults were in force and still did not retry. The reaper correctly flipped both rows to `failed / stale run reaped`, visible in `error_log` since N3. The restart was a deploy of our own, not an incident — the finding is that a routine deploy costs a whole index. | Decide the semantics before coding: re-enqueue on shutdown (risking a double run against `_indexing_locks` and the advisory locks), or leave it to the cron and make the loss explicit in the UI. Not a silent implementation choice. | diff --git a/scripts/migrate_to_supabase.sh b/scripts/migrate_to_supabase.sh new file mode 100755 index 00000000..a351b748 --- /dev/null +++ b/scripts/migrate_to_supabase.sh @@ -0,0 +1,190 @@ +#!/usr/bin/env bash +# Copy the application database from Heroku Postgres to Supabase. +# +# RUN ON 2026-08-29. Production now runs on Supabase; this script is kept because it +# is the record of how, and because it is the thing to run again for a second +# environment. Every check below still holds and it is safe to re-run: it refuses to +# copy into a target whose hardening has come undone, and it diffs row counts rather +# than trusting the copy. +# +# The schema is applied and compared column-for-column; so is the hardening, and that +# part is not optional — read "Why the hardening comes first" before changing it. +# +# ./scripts/migrate_to_supabase.sh # dry run: check, measure, report +# ./scripts/migrate_to_supabase.sh --apply # copy the data +# +# Needs SUPABASE_DB_PASSWORD in the environment. That is the one thing no automation +# here can obtain: it is set when the project is created and the API will not return it. +# +# --------------------------------------------------------------------------------- +# Why the hardening comes first, and must stay +# +# Supabase's `public` schema is served by the Data API, and `ALTER DEFAULT PRIVILEGES` +# grants `anon` and `authenticated` ALL privileges on everything created there. The +# anon key is designed to be public — it ships inside frontends. Restoring 65 tables +# into the default configuration would have made `users`, `connections` (Fernet- +# encrypted credentials), `ssh_keys`, `mcp_api_keys` and `audit_logs` readable AND +# writable by anyone holding it, the moment the restore finished. +# +# Three defences are in place, and each was verified rather than assumed: +# +# 1. The Data API no longer exposes `public` (db_schema = "graphql_public"). +# Verified from outside with the real anon key against a canary row: 404 with the +# lock on, HTTP 200 and the canary leaking with it deliberately removed. +# 2. Default privileges for `postgres` in `public` revoked from anon, authenticated +# and service_role — including PG17's MAINTAIN. +# 3. RLS enabled on all 65 tables, with every grant to anon/authenticated revoked. +# The application is unaffected: it connects as `postgres`, which has BYPASSRLS. +# `anon` and `authenticated` do not. +# +# --------------------------------------------------------------------------------- +# Why the pooler, and why session mode +# +# `db..supabase.co` resolves to an AAAA record ONLY — there is no A record, and +# Heroku dynos have no outbound IPv6. A direct connection from a dyno cannot be made at +# all, so the pooler is not a performance choice here, it is the only route. +# +# Port 5432 on the pooler is session mode; 6543 is transaction mode. Transaction mode +# breaks named prepared statements, which SQLAlchemy's asyncpg dialect uses by default, +# and the failure is intermittent rather than immediate. At 9 of 20 connections in use +# the throughput argument for transaction mode is theoretical and the ways to be subtly +# wrong are not. Revisit when connection count is the measured constraint. +set -euo pipefail + +APP="${HEROKU_APP:-checkmydata-api}" +REF="${SUPABASE_PROJECT_REF:-gbtnnipdxtefnaietmli}" +POOLER_HOST="${SUPABASE_POOLER_HOST:-aws-1-eu-west-1.pooler.supabase.com}" +PG_BIN="${PG_BIN:-/opt/homebrew/opt/postgresql@17/bin}" +APPLY="${1:-}" + +say() { printf '\n\033[1m%s\033[0m\n' "$*"; } +die() { printf '\033[31m%s\033[0m\n' "$*" >&2; exit 1; } + +[ -n "${SUPABASE_DB_PASSWORD:-}" ] || die \ + "SUPABASE_DB_PASSWORD is not set. It is chosen when the Supabase project is created + and the Management API will not return it — this is the one step no automation here + can take. Set it, or reset it in the dashboard, then re-run." + +# pg_dump refuses to dump from a server newer than itself, and Heroku runs 17.9. +[ -x "$PG_BIN/pg_dump" ] || die "PostgreSQL 17 client not found at $PG_BIN (brew install postgresql@17)" +DUMP_VERSION="$("$PG_BIN/pg_dump" --version | grep -oE '[0-9]+' | head -1)" +[ "$DUMP_VERSION" -ge 17 ] || die "pg_dump is $DUMP_VERSION; Heroku Postgres is 17.x and pg_dump refuses to read a newer server" + +# The source is NOT DATABASE_URL. Since the switch on 2026-08-29 that variable points +# at the target, and reading it here would copy Supabase into Supabase while every +# check in this script passed — schema matches itself, row counts match themselves. +# It reads the old database's own attachment instead, and refuses if the two ends turn +# out to be the same host. +SOURCE_URL="${MIGRATION_SOURCE_URL:-$(heroku config:get HEROKU_PG_ROLLBACK_URL -a "$APP")}" +[ -n "$SOURCE_URL" ] || die \ + "no source database. Set MIGRATION_SOURCE_URL, or attach the source as + HEROKU_PG_ROLLBACK. DATABASE_URL is deliberately not used: it is the TARGET." +TARGET_URL="postgresql://postgres.${REF}:${SUPABASE_DB_PASSWORD}@${POOLER_HOST}:5432/postgres" + +src_host() { printf '%s' "$1" | sed -E 's#.*@([^/?]+).*#\1#'; } +[ "$(src_host "$SOURCE_URL")" != "$(src_host "$TARGET_URL")" ] || die \ + "source and target are the same host — this would copy a database onto itself and + every check below would pass while doing it." + +say "1. Both ends answer" +"$PG_BIN/psql" "$SOURCE_URL" -tAc "select 'source ' || version()" | cut -c1-60 +"$PG_BIN/psql" "$TARGET_URL" -tAc "select 'target ' || version()" | cut -c1-60 + +say "2. The schema matches before any data moves" +SHAPE="select (select count(*) from information_schema.tables where table_schema='public' and table_type='BASE TABLE') + || '/' || (select count(*) from pg_indexes where schemaname='public') + || '/' || (select count(*) from information_schema.table_constraints + where constraint_schema='public' and constraint_type='FOREIGN KEY')" +SRC_SHAPE="$("$PG_BIN/psql" "$SOURCE_URL" -tAc "$SHAPE")" +DST_SHAPE="$("$PG_BIN/psql" "$TARGET_URL" -tAc "$SHAPE")" +printf ' source tables/indexes/fks: %s\n target tables/indexes/fks: %s\n' "$SRC_SHAPE" "$DST_SHAPE" +[ "$SRC_SHAPE" = "$DST_SHAPE" ] || die "schema differs — resolve that before copying data" + +say "3. The hardening is still in place" +LEAKY="$("$PG_BIN/psql" "$TARGET_URL" -tAc \ + "select count(*) from information_schema.role_table_grants + where table_schema='public' and grantee in ('anon','authenticated')")" +NO_RLS="$("$PG_BIN/psql" "$TARGET_URL" -tAc \ + "select count(*) from pg_tables where schemaname='public' and not rowsecurity")" +printf ' tables granting to anon/authenticated: %s (must be 0)\n tables without RLS: %s (must be 0)\n' "$LEAKY" "$NO_RLS" +[ "$LEAKY" = "0" ] && [ "$NO_RLS" = "0" ] || die \ + "the target is not hardened — copying production data into it now would expose it" + +if [ "$APPLY" != "--apply" ]; then + say "Dry run only. Re-run with --apply to copy the data." + "$PG_BIN/psql" "$SOURCE_URL" -tAc \ + "select 'source holds ' || pg_size_pretty(pg_database_size(current_database()))" + exit 0 +fi + +say "4. Copying data (schema already present, so data only)" +# `trace_spans.parent_span_id` references `trace_spans`, so no ordering of the tables +# can satisfy the foreign key while rows are still arriving. pg_dump's answer is +# `--disable-triggers`, and it does not work here: it emits +# `ALTER TABLE ... DISABLE TRIGGER ALL`, which needs superuser, and Supabase's +# `postgres` role is not one (`rolsuper=false`; only `supabase_admin` is). Measured — +# the first attempt failed with +# ERROR: permission denied: "RI_ConstraintTrigger_a_18816" is a system trigger +# and rolled back cleanly, because of --single-transaction below. +# +# `session_replication_role = replica` defers the same checks and IS available to +# `postgres` here (verified). Set inside the same transaction as the copy, so it cannot +# outlive a failure and leave the session quietly not enforcing constraints. +{ + echo "SET session_replication_role = 'replica';" + "$PG_BIN/pg_dump" --data-only --no-owner --no-privileges --schema=public "$SOURCE_URL" + echo "SET session_replication_role = 'origin';" +} | "$PG_BIN/psql" "$TARGET_URL" -v ON_ERROR_STOP=1 --single-transaction -q + +say "5. Row counts, table by table" +COUNTS="select table_name, (xpath('/row/c/text()', + query_to_xml(format('select count(*) as c from public.%I', table_name), + false, true, '')))[1]::text::int as n + from information_schema.tables + where table_schema='public' and table_type='BASE TABLE' order by table_name" +"$PG_BIN/psql" "$SOURCE_URL" -tAF, -c "$COUNTS" > /tmp/src_counts.csv +"$PG_BIN/psql" "$TARGET_URL" -tAF, -c "$COUNTS" > /tmp/dst_counts.csv +if diff -u /tmp/src_counts.csv /tmp/dst_counts.csv > /tmp/counts.diff; then + printf ' every table matches (%s tables)\n' "$(wc -l < /tmp/src_counts.csv | tr -d ' ')" +else + printf '\033[31m ROW COUNTS DIFFER:\033[0m\n'; sed -n '1,40p' /tmp/counts.diff + die "do not switch DATABASE_URL until this is understood" +fi + +say "6. Sequences follow the data" +# A restored table with a sequence left at 1 accepts one insert and then fails on the +# primary key. It is the classic way a migration looks fine for an hour. +"$PG_BIN/psql" "$TARGET_URL" -q -c " +do \$\$ +declare r record; +begin + for r in + select s.relname as seq, t.relname as tbl, a.attname as col + from pg_class s + join pg_depend d on d.objid = s.oid and d.deptype = 'a' + join pg_class t on t.oid = d.refobjid + join pg_attribute a on a.attrelid = t.oid and a.attnum = d.refobjsubid + where s.relkind = 'S' and t.relnamespace = 'public'::regnamespace + loop + execute format('select setval(%L, coalesce((select max(%I) from public.%I), 1))', + r.seq, r.col, r.tbl); + end loop; +end \$\$;" +echo " done" + +cat <<'NEXT' + +Data is in. What is left is one config change and one measurement, in this order: + + heroku config:set DATABASE_URL='postgresql+asyncpg://postgres.:@:5432/postgres' -a checkmydata-api + +Then, before believing it: + + * `alembic_version` on the target must equal the one on the source. + * A chat request must complete — that exercises asyncpg through the session pooler, + which is the part no dry run can prove. + * `select count(*) from doc_embeddings` must be non-zero, or the next repo index + will force a full rebuild and spend hours re-earning what was already copied. + +Keep the Heroku database for a few days. It is the only rollback there is. +NEXT