Conversation
The design for moving the one thing that still needs a database off Postgres, and for the 429 that discovery turned up on the way. `routes/progress.py` has been able to keep a signed-in reader's progress since it was written and has never run in production. The table was only ever created by `initialize_database()` at boot, boot-time DDL went away when the API became a Vercel function, and the Supabase project it would have lived in no longer answers. The spec puts progress in the `LearnPracticeWorkData` spreadsheet, behind an Apps Script web app — chosen over a service account because `LockService` gives the read-modify-write merge a real mutex, and because it keeps a Google private key out of a function bundle already tuned against a 225 MB ceiling. Only the Google `sub` is stored; email is dropped, since a spreadsheet has no row level security. It also records what the investigation found: with `DATABASE_URL` set and the database unreachable, the AI buckets are `when_unavailable="refuse"`, so every AI request comes back 429 — a refusal that reads to a visitor exactly like an exhausted quota. Verified against production. The quota moves to Redis, which is what `api_rate_limits` was emulating in SQL anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight tasks in two phases, each ending in something independently testable. Phase A moves the AI quota to Redis and can ship on its own — it fixes a live outage, since production answers 429 to every AI request today. Phase B puts learner progress in the spreadsheet behind an Apps Script web app. The plan's Review Focus names the five things the spec implies but does not say, and each one has a test attached to the task that owns the code: an id beginning with `=` is a formula to Sheets and the ids come from localStorage; two rows for one sub must be an error rather than a coin toss; a lock that times out must fail rather than write unlocked; the shared token is the only guard on a web app deployed to anyone with the link; and Redis going away must still refuse the AI buckets while sign-in degrades. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
SharedRateLimiter now asks quota_store.redis_url() instead of config.database_url() whether it can share a quota, and calls quota_store.hit_rate_limit/release_rate_limit instead of the Postgres functions of the same name (Task 1, 82f5db4). The four buckets, their limits/windows, and every when_unavailable policy are unchanged. Retires the Postgres counters this replaces: hit_rate_limit, _hit_rate_limit, release_rate_limit, _release_rate_limit and the now-unused STALE_RATE_LIMIT_DAYS cleanup in database.py, and the api_rate_limits table (plus its index and its entry in the RLS table list) from schema.sql. Updates the stale schema ownership table in deploy/README.md to match. test_rate_limit_config.py gets three new tests for the Redis-missing path, and its `production` fixture and one existing test move from faking DATABASE_URL to faking REDIS_URL, since the question the limiter asks changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…QL rules Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The 500-cap on id arrays uses stored-first ordering to prevent an incoming payload from evicting recorded progress. Real ceilings (40 challenges, 12 lectures) are far below the cap, so it only engages under tampering. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tasks 1 and 2 moved the AI quota off Postgres, and four documents plus one class docstring were still describing the table that no longer exists. - `deploy/README.md` — the serverless-quota paragraph now describes Redis keys and `EXPIRE … NX` rather than "atomic Postgres rows"; a `REDIS_URL` row joins the environment table, naming the 7.0 floor that the `NX` flag imposes and what a missing URL does to the AI buckets; the `DATABASE_URL` row says plainly that it no longer has anything to do with quotas. - `replit.md`, `architecture.html`, `artifacts/ai-testing-academy/README.md` — same correction, and each now distinguishes the AI buckets failing closed from sign-in and admin degrading, which the old text collapsed into "fails closed". - `server/app/rate_limit.py` — the `SharedRateLimiter` docstring opened with "Postgres-backed in production". Flagged as a deferred minor in the Task 2 review; it is one line and it is the first thing anyone reads about the class. Also records the second occurrence of a foreign Vercel project building this repo: `learn-practice-work-ai-testing-academy`, connected 25 August, disconnected 2026-09-27. Twice is a pattern, so the section now leads with the diagnostic — read the project name on the failing deployment first, because if it is not `learn-practice-work` then nothing here caused it and nothing here will fix it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Finding 1: the lock guarding doPost's read-modify-write had no test proving tryLock(false) refuses the write instead of falling through unlocked, or that releaseLock still runs when the locked work throws. Added a doPost harness that passes live LockService/SpreadsheetApp/ContentService mocks (not JSON splices) so call tracking is possible, plus tests for: a denied lock writes nothing, a throw inside the lock still releases it, and an unknown-sub load returns emptyProgress and writes nothing. Finding 2: Apps Script's doPost(e) cannot read custom headers, so the X-Academy-Token header from the brief was never implementable; a token in the query string was the wrong fix too, since it lands in execution and proxy logs. authorized() now reads body.token, and doPost parses postData before authorizing, treating a malformed body as unauthorized rather than throwing outside the lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The shared token travels in the JSON body, not the query string, matching Task 5's authorized(body) contract; a query-string token would land in Google's execution logs and any proxy log along the way. Test pins both that the token is in the body and that the URL carries no query. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Narrow MergeProgress to (subject, incoming) — the email argument is gone, so ProgressService can no longer hand a client's email to a store that has no row-level security. Point dependencies.py at sheets_store instead of database, delete the Postgres progress path (EMPTY_PROGRESS, MAX_PROGRESS_IDS, _progress_view, _PROGRESS_COLUMNS, load_progress, _load_progress, merge_progress, _merge_progress) and the learner_progress table from schema.sql, keeping every other table's RLS entry intact. Update the seed script's --check query and deploy/README.md, which both still named the removed table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three defects that existed because the stubs modelled Sheets' happy path
rather than its rules, so the stubs now enforce the rules.
A Google `sub` is a ~21-digit decimal string. `text()` only disarms a
formula, so the sub went into the cell bare and Sheets parsed it as a
number: past IEEE-754 precision it reads back with its tail zeroed, so
`rowIndexFor` never matches, every sync appends another row, and every
load answers `emptyProgress()` that the client adopts over the learner's
real `localStorage`. `forcedText()` prefixes an apostrophe for that one
column unconditionally, and tab creation sets the column's number format
to `@` so a hand-typed sub behaves the same.
`rowIndexFor` asked for `Math.max(getLastRow() - 1, 0)` rows. Apps Script
requires `numRows >= 1`, and `sheet()` creates a tab with only a header
row — so the first request after every deployment threw. It returns -1
without touching `getRange` when there are no data rows.
Also: the lock waits `LOCK_WAIT_MS` (12s) rather than the full 20s HTTP
timeout, so `{"error":"busy"}` can reach a client that is still
listening; an `op` that is neither `load` nor `merge` is refused instead
of falling through to a write; and `union()`'s seen-set is
`Object.create(null)`, so an id called `constructor` or `toString` is no
longer read as already seen and dropped.
The doPost harness now models one grid with a header row, refuses a
range of fewer than one row with Apps Script's own message, and coerces
every written cell the way a cell does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…with An `/exec` URL answers 302 with a Location on `script.googleusercontent.com/macros/echo`, and only that second URL serves the body. `httpx.AsyncClient` does not follow redirects unless told to and `raise_for_status()` raises on a 3xx, so every load and every merge was a 500 — for every signed-in learner, permanently. `follow_redirects=True` is passed explicitly, the way `monitor.py` passes True and `relay.py` passes False: which one it is should be readable at the call site. The test serves a 302 from a MockTransport and the JSON from the URL it points at. `TIMEOUT` now says why it is larger than the script's lock wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Readiness answered 503 `not_ready` whenever `database_ready()` was false, and that probe cannot tell an unconfigured `DATABASE_URL` from an unreachable one. The end state this branch builds towards has no Supabase project at all — progress in a spreadsheet, quotas in Redis — so readiness would have answered 503 permanently, on a path Fly health-checks and the deploy workflow reads. An absent URL now reports `database: not_configured` and stays ready. A configured database that cannot be reached is still a 503, unchanged. `rateLimiting` is reported whatever the database is doing. It sat behind the database check, so the one field that names the cause of a quota outage was unreachable in exactly the deployment it was written for — and the workflow step that greps for it could never have fired. Its warning now names `REDIS_URL` rather than `DATABASE_URL`, which stopped having anything to do with quotas one commit ago. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_client()` called `redis.from_url` on every `hit_rate_limit` and `release_rate_limit`, and nothing ever awaited `aclose()`. Every AI, sign-in and admin request paid a fresh TCP+TLS handshake for a client that only `__del__` would reclaim. Under burst traffic that reaches a hosted Redis connection cap, `hit_rate_limit` raises, and the AI buckets refuse with a 429 indistinguishable from an exhausted quota — the failure this branch exists to remove. A `redis.asyncio` client is itself a connection pool and is safe to share, so it is built on first use and kept, keyed on the resolved URL. Not at import time: `redis_url()` reads the environment, which tests change per test. `reset_client()` is the seam, and an autouse fixture drops it around every test so nothing leaks between them under xdist. Also two test gaps this file had: - both "Redis is broken" tests deleted `REDIS_URL`, which exercises `shared_quota_problem()` and returns long before the `except` arm a real outage takes. One test now stubs a client that refuses on `incr` and holds the trade on that path: billed buckets shut, sign-in degrades to a per-worker bound. - `test_the_window_is_set_once_when_the_key_is_created` asserted two EXPIRE calls, the opposite of its name. Renamed to what it checks, and `FakeRedis.expire` now insists on `nx=True` — which is what makes the second call a no-op inside Redis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… is false `deploy/README.md` never mentioned `SHEETS_WEBAPP_URL` or `SHEETS_WEBAPP_TOKEN` — not in the environment table, not in the verification section. Unconfigured, `/api/progress` answers 503 and the academy carries on from `localStorage`, which is exactly what it does correctly for a signed-out reader: a broken deployment is indistinguishable from a working one to the visitor *and* to the operator. Both now have rows saying what they do and what happens without them, the token's 32-character minimum and its pairing with the Apps Script `ACADEMY_TOKEN` property are stated, and the verification section says plainly that neither health check can see this store and that the spreadsheet is the only place to look. Recorded there too: `vercel.json` sets no `maxDuration`, so the platform default of 10s sits under `sheets_store.TIMEOUT` of 20 and a genuinely cold Apps Script start returns a platform 504 rather than the clean 500 the design expects. `vercel.json` is left alone. Statements this branch made false: - `seed-academy-content.ts` credited `schema.sql` with "learner progress" and "quota rows"; both tables were deleted. - the README's ownership table listed four of the six tables `schema.sql` creates — `test_runs` and `test_suite_results` were missing, and a table nobody lists is a table nobody applies. - `progressApi.ts` pointed at `database.merge_progress`, deleted here. - `replit.md`'s module table mentioned none of `progress.py`, `sheets_store.py` or `quota_store.py`. - `schemas.py` mirrored `database.MAX_PROGRESS_IDS`, which no longer exists; the cap it mirrors is `MAX_IDS` in `Progress.gs`. - the spec claimed 500 ids are "a few kilobytes, far under the 50,000-character cell limit". `ProgressBody` accepts 500 ids of 200 characters, which serialise to ~101,500 — twice the limit. The real bound is stated, along with the fact that it is the count cap and not any headroom in the cell that does the work. `tests/unit/deployDocs.spec.ts` holds the first three of those, so the ownership table and the two variables cannot drift out again silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amielnoy
changed the title
This branch was successfully deployed
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.
Two stores move off Postgres, so this deployment needs no Supabase project.
AI rate-limit counters → Redis.
api_rate_limitswas emulatingINCRwith anupsert and a window comparison. It is now a Redis key with a TTL, set
NXso asteady caller cannot push their own window out.
Learner progress → the
LearnPracticeWorkDataspreadsheet, behind a GoogleApps Script web app that holds
LockService.getScriptLock()aroundread-modify-write. Apps Script was chosen over the Sheets REST API for exactly
that lock: the merge is a union, and without a mutex two devices syncing at once
lose one device's work.
ProgressServicealready took its two operations as injected callables, sonothing above the seam changed — not the client, not
routes/progress.py, notProgressBody. Only the Googlesubreaches the sheet; a spreadsheet has norow-level security, so the email argument was removed from the interface
entirely.
Spec:
docs/superpowers/specs/2026-09-27-sheets-progress-store-design.mdPlan:
docs/superpowers/plans/2026-09-27-sheets-progress-store.mdIt also fixes a live outage
/api/ai/generateanswers 429 on the first request of the day, verifiedagainst production.
DATABASE_URLis set while the database is unreachable, sothe burst limiter cannot count, and the AI buckets are
when_unavailable="refuse"by design — they guard a key that bills per call. The refusal reads to a visitor
exactly like an exhausted quota. Unsetting
DATABASE_URLdoes not help; thequota needed a store that is not Postgres.
Readiness was hiding the diagnostic too: it returned 503 whenever
DATABASE_URLwas unset — the intended end state of this branch — and only reported the
rateLimitingfield after that check passed, so the CI grep for it could neverfire. An absent database is now
not_configuredand still ready; aconfigured-but-unreachable one still 503s.
The review found three Criticals no task-level test could
A whole-branch review caught three defects that would have shipped, all on the
one path the feature depends on, all invisible because the stubs modelled a
platform's happy path rather than its rules:
/execanswers 302 andserves the body at a second URL;
httpxdefaults tofollow_redirects=False.Every call would have raised, forever, and the client swallows the 500.
subcorrupted, which erases progress. Asubis a ~21-digitnumeric string; Sheets parsed it as a number past IEEE-754 precision. The row
lookup never matched, so every sync appended a row and every load returned
empty progress with a 200 — which the client writes over the learner's real
localStorage.getRange(2, 1, 0, 1)isinvalid; a freshly created tab has only headers.
Each fix came with a stub that enforces the real rule. The
numRows < 1stubturned a pre-existing test red — the bug reproducing against code already on the
branch, which is the difference between a test that documents a fix and one that
would have caught the defect.
Before this does anything
The branch lands complete but inert. Three steps provision resources, write
secrets and deploy, so they are deliberately not automated:
vercel integration add redis --yes --no-claim, then confirm the variablename it sets is one of
REDIS_URL/KV_URL/REDIS_TLS_URL, and thatthe server is Redis 7.0 or newer — the window uses
EXPIRE … NX, whicholder servers reject.
server/sheets/Progress.gsas a web app bound to the spreadsheet,set its
ACADEMY_TOKENscript property (≥32 random characters), and setSHEETS_WEBAPP_URLandSHEETS_WEBAPP_TOKENon the Vercel project.If you ever created the
learner_progresstab by hand, set column A'sformat to plain text first — the
@format is only applied on thecreation path.
different challenge in each, and confirm both survive a reload.
Until then, progress answers 503 and the client keeps using
localStorage—which is what it does today. Note also that
SESSION_SECRETis still unset onthe project, so sign-in itself returns 503 and none of the above is reachable
until it exists.
Known and deferred
Nine minor findings were triaged and deferred rather than fixed, listed with
reasons in the review trail: the
maxDurationnote omits the 12s lock wait; athird "quota rows" falsehood survives in
database.py; four tests are weakerthan the properties they name;
reset_client()does notaclose(); the spec'srequest sketch still shows a header that was replaced by a body field; and
whether
test_runs/test_suite_resultsstill belong in the API's schema is anopen question.
Verification
218 pytest · 377 Node unit tests · ruff · typecheck — all green.
Seventeen commits: six implementation tasks each gated by its own review, one
whole-branch review, one fix wave, one scoped re-review.
🤖 Generated with Claude Code