feat: the data that should have been in Postgres, and the tables to hold it - #26
Merged
Merged
Conversation
Every worked-example slide in eight decks calls `fetchLectureExample`, which calls `.single()` — so a missing row is not a blank space, it is an error, and all thirty-eight of them have been rendering "Example content unavailable" over an empty panel. The table was live and empty: the decks were written against a Supabase project that had the content, the content never reached this repository, and no build, type or test ever mentioned the table. The content comes from two places, and they are not equally trustworthy. Lectures 9 and 10 still carry theirs as render-time fallbacks — `FALLBACK_ROWS`, `defaultCode`, `defaultBullets`, the `?? t(en, he)` default behind every field, and lecture 10's verdict badges — so those twenty are recovered from the decks themselves rather than retyped; the strings are bilingual and a mangled Hebrew transcription is not something review catches. The eighteen for lectures 1 to 6 are new, written against each slide's manifest description, because that content exists nowhere in this repository or its history. `lectureExamples.spec.ts` is the part that stops this recurring. Two numbers decide whether a slide finds its row — `LECTURE_ITEM_ID` in the deck's client and the position each slide passes — and both are literals sitting far from the seed that has to match them. It reads the call sites and checks them against the content and the seed, in both languages, with no database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL
The DDL was a Python string holding two tables, and neither existed in the Supabase project this API points at — which is why `/api/entitlements/course` answers 503 and the admin screen lists nobody. Moving it to `schema.sql` next to the module is not tidying: it is read far more often than it is run, and the comments in it are the only documentation of what each table is for. Six tables join them, each answering a question nothing could answer before: who signed in, what they have finished, what they have asked the AI proxy for, and what a completed test run found. All of them are new; none changes an existing code path. The security block at the bottom is the reason this file is worth reading. These land in the same project as the content tables, `public` is the schema PostgREST exposes, and Supabase grants the anon role broad access to new tables there by default — so without it, `course_purchases` would be customer email addresses and payment intent ids readable over HTTPS with a key that ships in the browser. Row level security with no policy is a closed door to every role that is not the owner, and the owner is the role this API connects as. `api_rate_limits` also gets a purge. Rows are reused in place, so the table is bounded by distinct callers rather than by traffic, which for IP-keyed buckets is not bounded at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL
…ng away There was no user record anywhere. Identity existed only inside a signed cookie, which is why entitlement is decided by an OR across email and subject on every request, and why a purchase made at checkout by someone not signed in could never be attached to the account that signed in afterwards. Signing in is the moment those two can be connected, so that is where it now happens — and never at the cost of a sign-in Google has already verified. The Prometheus counters answer "how many, right now" and are gone at the next restart. Both wanted the same four facts about a request, so `activity.py` records the counter and the row from one call rather than from two that drift apart. The row carries the same HMAC of the email the metric carries as a label, so a spike on a dashboard and a row in a table can be matched up without either holding an address; prompts, responses and keys are absent for the reason they are absent from the labels. Neither write can fail the request it describes. A request that succeeded and then could not be written down still succeeded, and turning that into a 500 would make the record more important than the thing it records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL
`ProgressContext` has tracked this since there was a client, in `localStorage` — so clearing site data lost it, a phone and a laptop each held a different half of it, and nothing on the server could see any of it. This is the same record with an owner. The merge is the whole design, not the write. Booleans are OR-ed, the answer counter takes the larger value, the two id lists are unioned, and the response is the result. Last-write-wins would mean that opening the site on a second device silently discards what the first one recorded, which is precisely the situation an account is supposed to fix. `lastTool` is the exception, because it is a cursor rather than an achievement. On the client the guard in `adopt` is what keeps that from looping: taking the server's answer sets state, setting state re-runs the effect, and the next push returns the same union — comparing before adopting makes the second round a no-op instead of the next lap. A tab regaining focus re-reads rather than merges, which is the other half of "follows you between devices": progress made on a phone is already stored, and that is the moment the laptop is worth telling. `useOptionalAuth` exists because progress is tracked for everyone and synced only for someone with an account, so a tree with no `AuthProvider` — which is how the component tests mount it — is an ordinary state rather than the programming error `useAuth` reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL
The Pushgateway holds one push per grouping key, so what the module called "history" was one run deep — enough to say whether the latest run on a branch was green, and unable to say when a suite started getting slower or which commit first broke it. That is the question `test_runs` exists to answer, and it is the one lecture 9 spends a slide teaching. Both destinations now run, independently: a missing `PUSHGATEWAY_URL` or `DATABASE_URL` disables that half and says which, and a failure in either is reported rather than raised. This step runs after the suites have already produced their verdict, so losing the record of a run must never be the reason a build goes red. Suite totals are zero-filled across every status on purpose. Without it, "this suite had no failures" and "this suite did not report" are the same answer to a query over the stored history — which is exactly the question the table is for. A re-run claims its existing row rather than inserting a second one, because otherwise re-running a job doubles every count against the same commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Summary by QodoPersist academy content, progress, activity, and test history
AI Description
Diagram
High-Level Assessment
Files changed (45)
|
Code Review by Qodo
1. Existing database boot fails
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
-64
to
-67
| connection.execute( | ||
| """ALTER TABLE course_purchases | ||
| ADD COLUMN IF NOT EXISTS retention_until timestamptz""" | ||
| ) |
There was a problem hiding this comment.
1. Existing database boot fails 🐞 Bug ☼ Reliability
Removing the compatibility ALTER TABLE means an existing course_purchases table without retention_until is left unchanged by CREATE TABLE IF NOT EXISTS, after which schema initialization tries to index the missing column and fails. Deployments using the prior schema therefore cannot start against their existing database.
Agent Prompt
## Issue description
Existing databases may have `course_purchases` without `retention_until`, and `CREATE TABLE IF NOT EXISTS` does not add that column. Restore an idempotent migration before any index, update, or delete references it.
## Issue Context
The previous initialization explicitly added the column, while the new schema assumes it is already present.
## Fix Focus Areas
- server/app/database.py[57-67]
- server/app/schema.sql[18-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+107
to
+112
| useEffect(() => { | ||
| if (!signedIn) return; | ||
| const controller = new AbortController(); | ||
| const timer = window.setTimeout(() => { | ||
| void pushProgress(progress, controller.signal).then(adopt); | ||
| }, SYNC_DELAY_MS); |
There was a problem hiding this comment.
2. Progress crosses user accounts 🐞 Bug ≡ Correctness
ProgressProvider keeps one global localStorage/state copy and pushes it whenever any user becomes signed in, so signing out user A and signing in user B merges A's progress into B's server record. Neither the auth transition nor the sync effect resets or namespaces progress by identity.
Agent Prompt
## Issue description
Progress retained from one account is pushed under the next account's authenticated session. Partition or reset progress when the authenticated identity changes while preserving a deliberate anonymous-to-first-account merge.
## Issue Context
The provider remains mounted through logout/login and currently tracks only `user !== null`, not which user is active.
## Fix Focus Areas
- artifacts/ai-testing-academy/src/context/ProgressContext.tsx[24-42]
- artifacts/ai-testing-academy/src/context/ProgressContext.tsx[72-117]
- artifacts/ai-testing-academy/src/context/AuthContext.tsx[96-140]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+98
to
+101
| if (sameProgress(current, remote)) return current; | ||
| writeValidated(localStorage, STORAGE_KEY, remote); | ||
| return remote; | ||
| }); |
There was a problem hiding this comment.
3. Stale refresh erases progress 🐞 Bug ≡ Correctness
adopt replaces current state and localStorage with every differing response, so a visibility GET started before a local update can return the older server copy and erase that unsynced update. The same replacement is also unsafe for delayed PUT responses because no request generation or state-version check is applied.
Agent Prompt
## Issue description
Asynchronous GET/PUT responses can be older than the current local state, but `adopt` replaces that state outright. Preserve monotonic local achievements when adopting remote data and reject responses from stale request/state generations.
## Issue Context
The visibility GET is independent of progress changes, so its controller is not aborted when the user records new progress.
## Fix Focus Areas
- artifacts/ai-testing-academy/src/context/ProgressContext.tsx[95-136]
- artifacts/ai-testing-academy/src/lib/progressApi.ts[63-101]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| insert into lecture_items (id, track_id, position, num, ready, title, description, url) values (38, 4, 7, 8, false, 'אבטחת מערכות ה-AI עצמן', 'הצד השני של המטבע — הגנה על מערכות ה-AI שלכם מפני prompt injection, גניבת מודלים, הרעלת נתונים וסיכוני שרשרת אספקה.', null); | ||
| insert into lecture_items (id, track_id, position, num, ready, title, description, url) values (39, 4, 8, 9, false, 'תגובה לאירועי אבטחה בסיוע AI', 'שימוש בעוזרי AI כדי להאיץ טריאז'', ניתוח שורש הבעיה ודיווח במהלך אירוע אבטחה חי.', null); | ||
| insert into lecture_items (id, track_id, position, num, ready, title, description, url) values (40, 4, 9, 10, false, 'בניית אסטרטגיית AI לאבטחת מידע', 'הכל ביחד — מפת דרכים מעשית לאימוץ AI על פני זיהוי, תגובה ומניעה בתוכנית האבטחה שלכם.', null); | ||
| select setval('question_bank_stages_id_seq', 10); |
There was a problem hiding this comment.
4. Stage sequence remains reset 🐞 Bug ≡ Correctness
The chunked seed removes the question_bank_stages_id_seq advancement and never restores it in the new final chunk, although stages are inserted with explicit IDs. After applying the documented chunk sequence, the next default-ID stage insert therefore starts at 1 and collides with an existing row.
Agent Prompt
## Issue description
The chunked seed no longer advances `question_bank_stages_id_seq` after inserting explicit stage IDs. Add the missing `setval` to the generated final chunk and ensure chunk generation retains every statement from the full seed.
## Issue Context
The full seed contains the correct stage sequence statement, but the committed chunk set does not.
## Fix Focus Areas
- scripts/src/seed-chunk-36.sql[1-8]
- scripts/src/seed-chunk-46.sql[1-6]
- scripts/src/academy-seed.sql[1027-1034]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+399
to
+406
| practice_completed = COALESCE(( | ||
| SELECT array_agg(DISTINCT id) FROM unnest( | ||
| learner_progress.practice_completed || EXCLUDED.practice_completed) AS id | ||
| ), '{{}}')::text[], | ||
| lectures_viewed = COALESCE(( | ||
| SELECT array_agg(DISTINCT id) FROM unnest( | ||
| learner_progress.lectures_viewed || EXCLUDED.lectures_viewed) AS id | ||
| ), '{{}}')::text[], |
There was a problem hiding this comment.
5. Unbounded growth of merged progress arrays 🐞 Bug ≡ Correctness
The SQL merge in _merge_progress unions the full stored practice_completed and lectures_viewed arrays with incoming arrays capped at 500, but never truncates the merged result, allowing authenticated clients syncing disjoint sets across repeated requests or devices to grow the persisted arrays indefinitely and defeat the intended row-size bound. Client-side response validation then truncates the arrays to 500, causing entries beyond the cutoff to disappear from device state while the unrestricted database row continues growing.
Agent Prompt
## Issue description
The intended 500-ID bound currently applies only to each incoming request, not to the accumulated SQL union. Enforce `MAX_PROGRESS_IDS` on the merged `practice_completed` and `lectures_viewed` results so persisted arrays cannot grow indefinitely, and keep client and server truncation behavior consistent.
## Issue Context
`MAX_PROGRESS_IDS` is intended to bound row size and is applied to incoming payloads through Python slicing before the upsert. However, the `ON CONFLICT DO UPDATE` expressions union the complete stored arrays with the incoming arrays and write every distinct value back without truncation; authenticated clients can therefore submit repeated disjoint batches, the array columns have no database constraint, and the client subsequently truncates server responses to 500.
## Fix Focus Areas
- server/app/database.py[295-299]
- server/app/database.py[369-406]
- server/app/schema.sql[70-80]
- artifacts/ai-testing-academy/src/lib/progressApi.ts[41-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+59
to
+76
| def store(directory: Path) -> bool: | ||
| """Keep the run in Postgres. False when no database is configured.""" | ||
| if not database_url(): | ||
| print("DATABASE_URL is not configured; test-history storage is disabled.") | ||
| return False | ||
| counts, durations = read_allure_results(directory) | ||
| if not counts: | ||
| raise RuntimeError(f"No Allure test results found in {directory}") | ||
| identity = run_identity() | ||
| record_test_run( | ||
| repository=identity["repository"], | ||
| branch=identity["branch"], | ||
| commit_sha=identity["commit"], | ||
| run_id=identity["run_id"], | ||
| run_attempt=positive_int("GITHUB_RUN_ATTEMPT", 1), | ||
| suites=suite_totals(counts, durations), | ||
| ) | ||
| return True |
There was a problem hiding this comment.
6. First ci test-history publish may fail 🐞 Bug ☼ Reliability
test_history.store() inserts directly into test_runs/test_suite_results via record_test_run() without ever calling initialize_database() (which runs schema.sql), so against a fresh Supabase database where the API server has never started, the insert fails with a missing-relation error; the failure is silently swallowed by main()'s try/except, so the run's history is simply never recorded with no hard failure signal beyond a printed message.
Agent Prompt
## Issue description
`test_history.store()` writes to `test_runs`/`test_suite_results` without ensuring the schema has been applied, so the very first CI run against a fresh database (before the API has started at least once) will fail to persist history, and the failure is silently swallowed.
## Issue Context
`initialize_database()` in server/app/database.py applies `schema.sql` (including `test_runs`/`test_suite_results`) but is only called from the FastAPI lifespan startup in server/app/main.py. The standalone CI script `python -m app.test_history` never triggers it.
## Fix Focus Areas
- server/app/test_history.py[59-76]
- server/app/database.py[472-520]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Stacked on #25 — based on
fix/ci-format-generated, so retarget tomainonce that merges.Six things that either belong in the Supabase project and are not there, or are there and empty.
The one that is currently broken
lecture_examplesis live and has zero rows, and eight decks fetch from it with.single()— so a missing row is an error, not a blank space, and all 38 worked-example slides render "Example content unavailable" over an empty panel. Nothing in the build, the types or the tests ever mentioned the table.Content comes from two places, and they are not equally trustworthy:
tests/unit/lectureExamples.spec.tsreads everyfetchLectureExample(n)call site and checks it against the content and the seed, in both languages, with no database.The rest
/api/entitlements/courseanswers 503 and the admin screen lists nobody. Nowserver/app/schema.sql, plus a purge for the otherwise-unbounded rate-limit table.localStorage, so per-browser. Now an account's, merged rather than replaced — see below.google_subjectrow is what finally lets a purchase made at checkout attach to the account that signs in later.test_runsaccumulates.Two decisions worth arguing with
The progress merge is a union, not a replacement. Booleans OR, the answer counter takes the max, the id lists combine, and the response is the result. Last-write-wins would mean opening the site on a second device silently discards what the first recorded — precisely the thing an account is meant to fix.
Every new table gets RLS on and anon revoked. These land in the same project as the content tables,
publicis the schema PostgREST exposes, and Supabase grants anon broad access to new tables there by default. Without the revoke,course_purchasesis customer emails and payment intent ids readable over HTTPS with a key that ships in the browser. Verified on the live project after applying:Already applied to Supabase
Additively — no
TRUNCATEwas run.lecture_examplesnow holds 76 rows (38 × en/he) and all eight operational tables exist. The committed seed still contains its truncate, sopnpm --filter @workspace/scripts run seed:academyremains the reproducible path.Needed outside this PR
DATABASE_URLsecret on the repo.ci.ymlpasses it to the publish step; without it the test-history storage prints that it is disabled and moves on — intended, not a failure.DATABASE_URLset. The absence ofcourse_purchasesin this project suggests it has never booted against it.Also worth a look
Lectures 3 and 4 disagree with themselves: the manifest puts their worked examples at positions 8/13/20 and 7/11/15, but the slides fetch 5/7/10 and 5/7/9. Rows are keyed on the fetch argument, since that is what queries the database, but one of the two numbers is wrong.
I left the Prometheus
userlabel alone despite its cardinality, because removing it changes what the Grafana dashboards see.Verification
195 Python · 312 unit · 216 component · 58 API · 88 contract · lint · format · workspace typecheck — all green.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NrZUXGffUKSQBUKQtiSbeL