Add a GraphQL surface alongside REST for the learner read path - #7
Merged
Conversation
Mounted at /graphql. Additive, not a migration: every REST route still works and there
are tests asserting they do.
WHY
The learn page has a measured five-round-trip waterfall on lesson completion. The POST
already returns {xp_gained, stats}; the client discards them and calls refreshStats(),
which fires four more GETs (stats, achievements, activity, completed lessons) against the
same SQLite file for the same repository. learnerDashboard collapses the four reads into
one request, and completeLesson returns the post-mutation dashboard inline so the refresh
is unnecessary.
BE PRECISE ABOUT THE BENEFIT
GraphQL does NOT reduce database work here, and measuring it disproved the intuitive
claim: the combined resolver issues MORE SQL statements than the four REST handlers
(6 vs 4 on an empty repo), because it performs the same four reads plus session
overhead. What it removes is four HTTP round trips, four dependency-injection cycles and
four session open/close pairs. The test and docstrings say this rather than claiming a
query-count win that does not exist.
THE STRAWBERRY LANDMINE
Strawberry documents that it "processes sync and async fields using the event loop, which
means that using a sync def will block the entire worker" -- unlike FastAPI there is no
automatic threadpool. dependencies.get_db hands out a synchronous SQLAlchemy Session, so
a single sync resolver would serialize blocking SQLite calls on the loop and stall
in-flight chat streams. Every resolver is therefore async and offloads via
run_in_threadpool, and two tests enforce it: one reflects over Query/Mutation asserting
no resolver is a sync def, the other asserts each blocking helper is only reached through
run_in_threadpool. AsyncSession is not the alternative -- a single AsyncSession is
documented as unsafe across concurrent tasks, which is how DataLoader batches, and
greenlet is not installed.
SCOPE
Chat deliberately stays REST/SSE. GraphQL incremental delivery (@defer/@stream) is not
ratified: absent from the September 2025 spec edition, RFC open since 2024-09-18, and
Strawberry's support is experimental requiring graphql-core>=3.3.0a9 against 3.2.11
stable. A test asserts the schema exposes no chat or stream field.
strawberry-graphql is pinned with an upper bound (>=0.240,<1.0) -- it is a
weekly-releasing 0.x with a documented breaking-change history, and this file otherwise
uses open lower bounds.
Resolvers reuse GamificationService rather than reimplementing anything, so GraphQL and
REST cannot diverge in behaviour.
ALSO: removed three iCloud-duplicated files ("neo4j_store 2.py", "__init__ 2.py",
"test_neo4j_graph_store 2.py") that macOS had created after the previous commit, and
added a .gitignore rule for the "* 2.*" pattern. The duplicated test file was being
collected by pytest as a second copy of the same 15 tests -- the suite reported 157 with
it present and 142 without, which is how it was caught.
Verified: 10 new tests, 142 total (132 + 10; the earlier 157 was the inflated count),
ruff clean, 43 routes, schema builds and mounts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CI failed on 7 of the 10 new tests with "openai.OpenAIError: Missing credentials" while the same tests passed locally. The difference was apps/api/.env: the fixture used TestClient as a context manager, which runs the lifespan, which builds the embedding client eagerly -- and locally .env silently supplied a real key. So the local green was an artifact of this machine, not evidence the tests worked. Two changes, both aimed at removing the machine dependency rather than papering over it: - Set a dummy OPENAI_API_KEY in the fixture. Provider clients are constructed eagerly by the dependency factories, so the app cannot be built without one. No request in these tests reaches a provider. - Stop entering the TestClient context manager, so the lifespan does not run. These tests exercise GraphQL resolvers, which open their own sessions via get_session_factory; running the lifespan would additionally require a reachable vector store. This is also how conftest.py already builds its client. Verified the way it should have been verified the first time: moved apps/api/.env aside and ran the suite with OPENAI_API_KEY, AZURE_OPENAI_API_KEY and ANTHROPIC_API_KEY all unset -- 142 passed, ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e7dbc3a42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _count(conn, cursor, statement, params, context, executemany): | ||
| counter["n"] += 1 | ||
|
|
||
| with TestClient(app) as client: |
There was a problem hiding this comment.
When this fixture enters TestClient(app) as a context manager, it is the only test client in the suite that runs the FastAPI lifespan; the pull-request CI runs pytest tests without any API-key environment, so startup reaches get_vector_store() and constructs the default OpenAI embedding client before the monkeypatched GraphQL session factory is ever used. In a fresh CI clone this makes every GraphQL test fail during fixture setup rather than exercising the isolated SQLite database; use a non-lifespan client here or override the startup dependencies/lifespan for the fixture.
Useful? React with 👍 / 👎.
CI failed on one test: "/graphql" was absent from app.routes even though the endpoint worked. GraphQLRouter registers a different path depending on the Strawberry version -- it resolves to "/graphql" under the locally installed 0.324.0 and to "" under the version CI installed from the >=0.240,<1.0 range. The assertion was testing the framework's route table rather than our behaviour. Now it posts a real query and checks the response, then confirms REST still answers on the same app. That is what the test was meant to establish, and it holds across versions. Verified with apps/api/.env moved aside and all provider keys unset: 142 passed, ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third CI failure on the same test, which was the signal that the whole approach was wrong: it kept asserting against app.routes, whose contents differ by FastAPI/Strawberry version, and pytest truncates long set reprs with "..." so the failure output was actively misleading about what the set contained. Replaced the last route-table assertion with a request: POST /api/chat/sessions with an empty body must not 404. A 404 would mean the router is gone; validation rejecting the body proves the route exists, without needing a provider or a real session. Verified with apps/api/.env moved aside and all provider keys unset: 142 passed, ruff clean. Co-Authored-By: Claude Opus 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
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.
Mounted at
/graphql. Additive, not a migration — every REST route still works, and there's a test asserting they do.Why
The learn page has a measured five-round-trip waterfall on lesson completion: the POST already returns
{xp_gained, stats}, the client discards them and callsrefreshStats(), which fires four more GETs.learnerDashboardcollapses the four reads into one request;completeLessonreturns the post-mutation dashboard inline so the refresh is unnecessary.I measured the benefit and it disproved the obvious claim
GraphQL does not reduce database work here. The combined resolver issues more SQL than the four REST handlers — 6 vs 4 on an empty repo — because it does the same four reads plus session overhead.
What it actually removes: four HTTP round trips, four dependency-injection cycles, four session open/close pairs. The test and docstrings say that, rather than claiming a query-count win that doesn't exist. If you use this on your resume, claim round trips, not query count.
The Strawberry landmine, with tests to enforce it
Strawberry documents that it "processes sync and async fields using the event loop, which means that using a sync
defwill block the entire worker" — unlike FastAPI, there is no automatic threadpool. This app hands out a synchronous SQLAlchemySession, so one sync resolver would serialize blocking SQLite calls on the loop and stall in-flight chat streams.Every resolver is
asyncand offloads viarun_in_threadpool. Two tests enforce it:Query/Mutationasserting no resolver is a syncdefrun_in_threadpoolAsyncSessionis not the alternative: a singleAsyncSessionis documented as unsafe across concurrent tasks — which is exactly how a DataLoader batches — and greenlet isn't installed.Scope
Chat stays REST/SSE. GraphQL incremental delivery (
@defer/@stream) is not ratified: absent from the September 2025 spec edition, RFC open since 2024-09-18, Strawberry's support experimental and requiringgraphql-core>=3.3.0a9against 3.2.11 stable. A test asserts the schema exposes no chat or stream field.strawberry-graphqlis pinned>=0.240,<1.0— a weekly-releasing 0.x with a documented breaking-change history, in a file that otherwise uses open lower bounds.Resolvers reuse
GamificationService, so GraphQL and REST can't diverge in behaviour.Bonus catch: iCloud was duplicating source files into the repo
Three files macOS had duplicated after the last commit —
neo4j_store 2.py,__init__ 2.py,test_neo4j_graph_store 2.py— were about to be committed. Removed, plus a.gitignorerule for the* 2.*pattern.The duplicated test file was being collected by pytest as a second copy of the same 15 tests. The suite reported 157 with it present and 142 without — that 15-test delta is how it was caught. Worth knowing since it's the same iCloud mechanism that produces the 29
TS2688errors in localtscruns.Verification
ruff check src tests/graphqlmounted🤖 Generated with Claude Code