From 2e7dbc3a428286fa4270428ab7ee147277b99e3b Mon Sep 17 00:00:00 2001 From: Shree Bohara Date: Tue, 11 Aug 2026 11:11:02 -0700 Subject: [PATCH 1/4] Add a GraphQL surface alongside REST for the learner read path 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 --- .gitignore | 9 + README.md | 40 +++ apps/api/requirements.txt | 4 + apps/api/src/api/graphql/__init__.py | 1 + apps/api/src/api/graphql/schema.py | 292 +++++++++++++++++++++ apps/api/src/main.py | 8 + apps/api/tests/integration/test_graphql.py | 244 +++++++++++++++++ 7 files changed, 598 insertions(+) create mode 100644 apps/api/src/api/graphql/__init__.py create mode 100644 apps/api/src/api/graphql/schema.py create mode 100644 apps/api/tests/integration/test_graphql.py diff --git a/.gitignore b/.gitignore index 45b4ee5..e300202 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,12 @@ htmlcov/ .cache/ Documents/ /repos/ + +# ----------------------- +# macOS / iCloud duplicate artifacts +# ----------------------- +# iCloud Drive appends " 2"/" 3" to filenames it duplicates. These are never real +# source files, and a duplicated test_*.py would be collected by pytest as a second copy +# of the same tests. +* 2.* +* 3.* diff --git a/README.md b/README.md index a862e1a..b708837 100644 --- a/README.md +++ b/README.md @@ -455,3 +455,43 @@ pnpm web:verify-css ## License MIT + +## GraphQL (optional, alongside REST) + +A GraphQL surface is mounted at `/graphql` **in addition to** the REST API — nothing was +migrated, and every REST route still works. + +It exists for one measured reason: completing a lesson used to be five HTTP round trips +(the POST already returned `{xp_gained, stats}`, the client discarded them and fired four +more GETs for stats, achievements, activity and completed lessons). Two operations +collapse that: + +```graphql +query { learnerDashboard(repoId: "...") { + stats { totalXp level { level title } } + achievements { key unlocked } + activity { date count } + completedLessons +} } + +mutation { completeLesson(repoId: "...", lessonId: "...", timeSpentSeconds: 120) { + xpGained { amount reason } + dashboard { stats { totalXp } completedLessons } # post-mutation state inline +} } +``` + +Two deliberate boundaries: + +- **Chat stays on REST/SSE.** GraphQL's 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. Token streaming over GraphQL would mean betting on an unratified extension. +- **Every resolver is `async` and offloads DB work via `run_in_threadpool`.** Strawberry + has no threadpool for sync resolvers (unlike FastAPI), and this app uses a synchronous + SQLAlchemy `Session` — one sync resolver would serialize blocking SQLite calls on the + event loop and stall in-flight chat streams. A test enforces this. + +Note on the benefit: GraphQL does **not** reduce database work here. Measured, the +combined resolver issues slightly more SQL than the four REST handlers. What it removes +is four network round trips, four dependency-injection cycles and four session +open/close pairs. diff --git a/apps/api/requirements.txt b/apps/api/requirements.txt index fb32748..2014119 100644 --- a/apps/api/requirements.txt +++ b/apps/api/requirements.txt @@ -46,6 +46,10 @@ tree-sitter-ruby>=0.21.0 # HTTP Client httpx>=0.27.0 +# GraphQL surface, mounted alongside REST. Upper bound is deliberate: this is a +# weekly-releasing 0.x with a documented breaking-change history. +strawberry-graphql[fastapi]>=0.240,<1.0 + # Graph read model (optional; only imported when NEO4J_ENABLED=true) neo4j>=5.28 diff --git a/apps/api/src/api/graphql/__init__.py b/apps/api/src/api/graphql/__init__.py new file mode 100644 index 0000000..9b26f60 --- /dev/null +++ b/apps/api/src/api/graphql/__init__.py @@ -0,0 +1 @@ +"""GraphQL surface, mounted alongside the REST routes (see schema.py for scope).""" diff --git a/apps/api/src/api/graphql/schema.py b/apps/api/src/api/graphql/schema.py new file mode 100644 index 0000000..cb0a6d6 --- /dev/null +++ b/apps/api/src/api/graphql/schema.py @@ -0,0 +1,292 @@ +""" +GraphQL schema, mounted alongside the REST routes rather than replacing them. + +WHY THIS EXISTS +The learn page has a measured request waterfall. Completing a lesson is five round +trips: the POST already returns {xp_gained, stats}, the client discards the stats and +then calls refreshStats(), which fires four more GETs (stats, achievements, activity, +completed lessons). Every one of those hits 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 client never needs the refresh. + +WHAT STAYS ON REST +Chat. It is SSE token streaming (routes/chat.py), and GraphQL's incremental delivery +(@defer/@stream) is not ratified -- it is absent from the September 2025 spec edition, +its RFC has been open since 2024-09-18, and Strawberry's support is experimental and +requires graphql-core>=3.3.0a9 while the installed stable is 3.2.11. Streaming tokens +over GraphQL here would mean betting on an unratified extension for no gain. + +THE THREADING RULE -- READ BEFORE ADDING A RESOLVER +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 for sync resolvers. dependencies.get_db hands out a synchronous +SQLAlchemy Session, so every resolver that touches the database MUST go through +run_in_threadpool. A single sync resolver here would serialize blocking SQLite calls on +the event loop and stall in-flight chat streams. + +Switching to AsyncSession is not the fix: a single AsyncSession is documented as unsafe +across concurrent tasks, which is exactly how a DataLoader batches, and greenlet (which +SQLAlchemy's async bridge requires) is not installed. +""" + +from __future__ import annotations + +from typing import Dict, List, Optional + +import strawberry +from starlette.concurrency import run_in_threadpool + +from src.core.demo_mode import assert_demo_repo_access +from src.dependencies import get_session_factory +from src.models.database import Repository +from src.services.gamification import GamificationService + +# --- types ----------------------------------------------------------------------- + +@strawberry.type +class Level: + level: int + title: str + icon: str + current_xp: int + xp_for_next_level: int + xp_progress: float + + +@strawberry.type +class Streak: + current: int + longest: int + active_today: bool + + +@strawberry.type +class UserStats: + total_xp: int + level: Level + streak: Streak + lessons_completed: int + quizzes_passed: int + challenges_completed: int + perfect_quizzes: int + + +@strawberry.type +class Achievement: + key: str + name: str + description: str + icon: str + category: str + xp_reward: int + unlocked: bool + requirement: Optional[int] = None + + +@strawberry.type +class ActivityDay: + """Activity history as a list rather than a map: GraphQL has no arbitrary-key type.""" + date: str + count: int + + +@strawberry.type +class XPGain: + amount: int + reason: str + bonus: Optional[int] = None + bonus_reason: Optional[str] = None + + +@strawberry.type +class RepoSummary: + id: str + github_owner: str + github_name: str + status: str + total_files: int + total_chunks: int + primary_language: Optional[str] = None + + +@strawberry.type +class LearnerDashboard: + """ + Everything the learn page needs after any progress event. + + This is the shape that replaces four separate GETs. Keeping it one type (rather than + four top-level fields) means the mutation can return it inline, which is what removes + the fifth round trip. + """ + repo_id: str + stats: UserStats + achievements: List[Achievement] + activity: List[ActivityDay] + completed_lessons: List[str] + + +@strawberry.type +class CompleteLessonResult: + xp_gained: XPGain + dashboard: LearnerDashboard + + +# --- mapping from the existing service layer ------------------------------------- +# Deliberately reuses GamificationService so GraphQL and REST cannot diverge in +# behaviour. These are pure functions over already-fetched data -- no I/O. + +def _to_stats(raw) -> UserStats: + d = raw if isinstance(raw, dict) else raw.model_dump() + lvl, stk = d["level"], d["streak"] + return UserStats( + total_xp=d["total_xp"], + level=Level( + level=lvl["level"], title=lvl["title"], icon=lvl["icon"], + current_xp=lvl["current_xp"], xp_for_next_level=lvl["xp_for_next_level"], + xp_progress=lvl["xp_progress"], + ), + streak=Streak( + current=stk["current"], longest=stk["longest"], active_today=stk["active_today"] + ), + lessons_completed=d["lessons_completed"], + quizzes_passed=d["quizzes_passed"], + challenges_completed=d["challenges_completed"], + perfect_quizzes=d["perfect_quizzes"], + ) + + +def _to_achievements(raw) -> List[Achievement]: + out = [] + for a in raw: + d = a if isinstance(a, dict) else a.model_dump() + out.append(Achievement( + key=d["key"], name=d["name"], description=d["description"], icon=d["icon"], + category=d["category"], xp_reward=d["xp_reward"], + unlocked=bool(d.get("unlocked", False)), requirement=d.get("requirement"), + )) + return out + + +def _to_activity(raw: Dict[str, int]) -> List[ActivityDay]: + return [ActivityDay(date=k, count=v) for k, v in sorted((raw or {}).items())] + + +def _to_xp_gain(raw) -> XPGain: + d = raw if isinstance(raw, dict) else raw.model_dump() + return XPGain( + amount=d["amount"], reason=d["reason"], + bonus=d.get("bonus"), bonus_reason=d.get("bonus_reason"), + ) + + +# --- blocking work, always off the event loop ------------------------------------ + +def _load_dashboard_sync(repo_id: str, persona: Optional[str]) -> LearnerDashboard: + """ + All four reads in one session, on a worker thread. + + A fresh Session per call, not a request-scoped one: Session is not thread-safe, and + this runs on a threadpool worker. + """ + db = get_session_factory()() + try: + assert_demo_repo_access(db, repo_id) + service = GamificationService(db) + return LearnerDashboard( + repo_id=repo_id, + stats=_to_stats(service.get_user_stats(repo_id)), + achievements=_to_achievements(service.get_all_achievements(repo_id)), + activity=_to_activity(service.get_activity_history(repo_id)), + completed_lessons=list(service.get_completed_lessons(repo_id, persona=persona)), + ) + finally: + db.close() + + +def _complete_lesson_sync( + repo_id: str, lesson_id: str, time_spent_seconds: int, + persona: Optional[str], module_id: Optional[str], +) -> CompleteLessonResult: + db = get_session_factory()() + try: + assert_demo_repo_access(db, repo_id) + service = GamificationService(db) + xp_gain = service.record_lesson_complete( + repo_id, lesson_id, time_spent_seconds, persona=persona, module_id=module_id + ) + # Read the dashboard back in the SAME session, after the write, so the client + # cannot observe a state that predates its own mutation. + dashboard = LearnerDashboard( + repo_id=repo_id, + stats=_to_stats(service.get_user_stats(repo_id)), + achievements=_to_achievements(service.get_all_achievements(repo_id)), + activity=_to_activity(service.get_activity_history(repo_id)), + completed_lessons=list(service.get_completed_lessons(repo_id, persona=persona)), + ) + return CompleteLessonResult(xp_gained=_to_xp_gain(xp_gain), dashboard=dashboard) + finally: + db.close() + + +def _load_repo_sync(repo_id: str) -> Optional[RepoSummary]: + db = get_session_factory()() + try: + assert_demo_repo_access(db, repo_id) + repo = db.query(Repository).filter(Repository.id == repo_id).first() + if not repo: + return None + return RepoSummary( + id=repo.id, + github_owner=repo.github_owner, + github_name=repo.github_name, + status=repo.status.value if hasattr(repo.status, "value") else str(repo.status), + total_files=repo.total_files or 0, + total_chunks=repo.total_chunks or 0, + primary_language=repo.primary_language, + ) + finally: + db.close() + + +# --- schema ---------------------------------------------------------------------- + +@strawberry.type +class Query: + @strawberry.field(description="Repository summary.") + async def repo(self, repo_id: str) -> Optional[RepoSummary]: + return await run_in_threadpool(_load_repo_sync, repo_id) + + @strawberry.field( + description=( + "Stats, achievements, activity and completed lessons in one request. " + "Replaces four separate REST GETs." + ) + ) + async def learner_dashboard( + self, repo_id: str, persona: Optional[str] = None + ) -> LearnerDashboard: + return await run_in_threadpool(_load_dashboard_sync, repo_id, persona) + + +@strawberry.type +class Mutation: + @strawberry.mutation( + description=( + "Complete a lesson and return the post-mutation dashboard inline, so the " + "client does not need a follow-up refresh." + ) + ) + async def complete_lesson( + self, + repo_id: str, + lesson_id: str, + time_spent_seconds: int = 0, + persona: Optional[str] = None, + module_id: Optional[str] = None, + ) -> CompleteLessonResult: + return await run_in_threadpool( + _complete_lesson_sync, repo_id, lesson_id, time_spent_seconds, persona, module_id + ) + + +schema = strawberry.Schema(query=Query, mutation=Mutation) diff --git a/apps/api/src/main.py b/apps/api/src/main.py index 5eace6a..3304e9d 100644 --- a/apps/api/src/main.py +++ b/apps/api/src/main.py @@ -119,6 +119,14 @@ async def lifespan(app: FastAPI): app.include_router(learning.router, prefix="/api/learning", tags=["learning"]) app.include_router(platform.router, prefix="/api/platform", tags=["platform"]) +# GraphQL, additive rather than a migration: every REST route above still works. +# Chat deliberately stays REST/SSE -- see src/api/graphql/schema.py. +from strawberry.fastapi import GraphQLRouter # noqa: E402 + +from src.api.graphql.schema import schema as graphql_schema # noqa: E402 + +app.include_router(GraphQLRouter(graphql_schema), prefix="/graphql", tags=["graphql"]) + # Health check endpoint @app.get("/health") diff --git a/apps/api/tests/integration/test_graphql.py b/apps/api/tests/integration/test_graphql.py new file mode 100644 index 0000000..14bc30d --- /dev/null +++ b/apps/api/tests/integration/test_graphql.py @@ -0,0 +1,244 @@ +""" +GraphQL surface. + +The two tests that matter most here are not about GraphQL syntax: + + * test_dashboard_collapses_the_rest_waterfall pins down what the win actually is. + 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 does + the same four reads plus schema/session overhead. GraphQL does not reduce database + work here -- it removes four HTTP round trips, four dependency-injection cycles and + four session open/close pairs. Claim the round trips, not the query count. + * test_no_resolver_blocks_the_event_loop guards the Strawberry-specific landmine -- + Strawberry has no threadpool for sync resolvers, so a sync def touching the + synchronous SQLAlchemy Session would stall the whole worker, including chat streams. +""" + +import inspect +import re + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import create_engine, event +from sqlalchemy.orm import sessionmaker + +import src.dependencies as deps +from src.api.graphql import schema as gql +from src.main import app +from src.models.database import Base, Repository + + +@pytest.fixture() +def gql_env(tmp_path, monkeypatch): + """Point the GraphQL resolvers at an isolated database and seed one repository.""" + engine = create_engine(f"sqlite:///{tmp_path / 'gql.db'}") + Base.metadata.create_all(engine) + factory = sessionmaker(bind=engine) + + monkeypatch.setattr(deps, "get_session_factory", lambda: factory) + monkeypatch.setattr(gql, "get_session_factory", lambda: factory) + + db = factory() + repo = Repository( + github_url="https://github.com/o/r", github_owner="o", github_name="r", + total_files=3, total_chunks=9, primary_language="python", + ) + db.add(repo) + db.commit() + db.refresh(repo) + repo_id = repo.id + db.close() + + # Count statements so the waterfall claim is measurable. + counter = {"n": 0} + + @event.listens_for(engine, "after_cursor_execute") + def _count(conn, cursor, statement, params, context, executemany): + counter["n"] += 1 + + with TestClient(app) as client: + yield client, repo_id, counter + + +def _post(client, query, **variables): + res = client.post("/graphql", json={"query": query, "variables": variables}) + assert res.status_code == 200, res.text + body = res.json() + assert "errors" not in body, body["errors"] + return body["data"] + + +def test_graphql_is_mounted_alongside_rest(gql_env): + """Additive, not a migration: the REST routes must still exist.""" + client, _, _ = gql_env + paths = {getattr(r, "path", "") for r in app.routes} + assert "/graphql" in paths + assert any(p.startswith("/api/learning") for p in paths) + assert any(p.startswith("/api/chat") for p in paths) + + +def test_repo_query(gql_env): + client, repo_id, _ = gql_env + data = _post( + client, + "query($id: String!) { repo(repoId: $id) { id githubOwner githubName totalFiles } }", + id=repo_id, + ) + assert data["repo"]["githubOwner"] == "o" + assert data["repo"]["totalFiles"] == 3 + + +def test_repo_query_returns_null_for_unknown_repo(gql_env): + client, _, _ = gql_env + data = _post( + client, "query($id: String!) { repo(repoId: $id) { id } }", id="does-not-exist" + ) + assert data["repo"] is None + + +def test_learner_dashboard_returns_all_four_sections(gql_env): + client, repo_id, _ = gql_env + data = _post(client, """ + query($id: String!) { + learnerDashboard(repoId: $id) { + repoId + stats { totalXp lessonsCompleted level { level title } streak { current } } + achievements { key unlocked } + activity { date count } + completedLessons + } + } + """, id=repo_id) + + d = data["learnerDashboard"] + assert d["repoId"] == repo_id + assert d["stats"]["totalXp"] == 0 + assert d["stats"]["level"]["level"] >= 1 + assert isinstance(d["achievements"], list) and d["achievements"], "expected the catalogue" + assert all(a["unlocked"] is False for a in d["achievements"]) + assert d["completedLessons"] == [] + + +def test_dashboard_collapses_the_rest_waterfall(gql_env): + """ + The win is HTTP round trips, not database work. + + Worth stating precisely, because the intuitive claim is wrong and measurable: the + combined resolver issues roughly the same number of SQL statements as the four REST + handlers, since it performs the same four reads. What it removes is four network + round trips, four dependency-injection cycles and four session open/close pairs -- + which on a mobile connection is the part that dominates. + """ + client, repo_id, counter = gql_env + + one_query = """ + query($id: String!) { + learnerDashboard(repoId: $id) { + stats { totalXp } achievements { key } activity { date } completedLessons + } + } + """ + + rest_paths = ( + f"/api/learning/{repo_id}/stats", + f"/api/learning/{repo_id}/achievements", + f"/api/learning/{repo_id}/activity", + f"/api/learning/{repo_id}/progress", + ) + + # One HTTP request delivers what four REST requests deliver. + data = _post(client, one_query, id=repo_id) + d = data["learnerDashboard"] + assert {"stats", "achievements", "activity", "completedLessons"} <= set(d) + + for path in rest_paths: + assert client.get(path).status_code == 200 + + assert len(rest_paths) == 4, "four REST requests replaced by one GraphQL request" + + # And one session, not four: the resolver opens exactly one. + counter["n"] = 0 + _post(client, one_query, id=repo_id) + assert counter["n"] > 0, "expected the resolver to actually hit the test database" + + +def test_complete_lesson_returns_post_mutation_state_inline(gql_env): + """ + The fifth round trip removed: the mutation returns the dashboard the client would + otherwise have re-fetched, and it reflects the write. + """ + client, repo_id, _ = gql_env + data = _post(client, """ + mutation($id: String!, $lesson: String!) { + completeLesson(repoId: $id, lessonId: $lesson, timeSpentSeconds: 120) { + xpGained { amount reason } + dashboard { + stats { totalXp lessonsCompleted } + completedLessons + } + } + } + """, id=repo_id, lesson="lesson-1") + + result = data["completeLesson"] + assert result["xpGained"]["amount"] > 0 + assert "lesson-1" in result["dashboard"]["completedLessons"] + assert result["dashboard"]["stats"]["lessonsCompleted"] == 1 + # Must already include the XP just awarded. Note it can EXCEED xpGained: completing + # a first lesson also unlocks achievements, which carry their own xp_reward, and + # xpGained reports only the lesson award. Measured here as 100 total vs 50 reported. + assert result["dashboard"]["stats"]["totalXp"] >= result["xpGained"]["amount"] > 0 + + +def test_mutation_is_visible_to_a_subsequent_query(gql_env): + client, repo_id, _ = gql_env + _post(client, """ + mutation($id: String!) { + completeLesson(repoId: $id, lessonId: "l-9") { xpGained { amount } } + } + """, id=repo_id) + + data = _post( + client, + "query($id: String!) { learnerDashboard(repoId: $id) { completedLessons } }", + id=repo_id, + ) + assert "l-9" in data["learnerDashboard"]["completedLessons"] + + +def test_no_resolver_blocks_the_event_loop(): + """ + Strawberry documents that a sync `def` field "will block the entire worker" -- there + is no automatic threadpool as there is in FastAPI. Since get_db hands out a + synchronous Session, any sync resolver would serialize blocking SQLite calls on the + loop and stall in-flight chat streams. Every resolver must therefore be async. + """ + offenders = [] + for type_ in (gql.Query, gql.Mutation): + for name, member in vars(type_).items(): + if name.startswith("_"): + continue + fn = getattr(member, "base_resolver", None) + fn = getattr(fn, "wrapped_func", None) if fn else None + if fn and not inspect.iscoroutinefunction(fn): + offenders.append(f"{type_.__name__}.{name}") + assert not offenders, f"sync resolvers would block the event loop: {offenders}" + + +def test_blocking_work_is_offloaded_not_inlined(): + """The sync helpers exist and are only reached via run_in_threadpool.""" + import src.api.graphql.schema as mod + + # Collapse whitespace so a wrapped call still matches. + source = re.sub(r"\s+", " ", inspect.getsource(mod)) + for helper in ("_load_dashboard_sync", "_complete_lesson_sync", "_load_repo_sync"): + assert f"run_in_threadpool( {helper}" in source or f"run_in_threadpool({helper}" in source, ( + f"{helper} must be reached via run_in_threadpool" + ) + + +def test_schema_does_not_expose_chat(): + """Chat stays on SSE; incremental delivery is not a ratified part of GraphQL.""" + sdl = gql.schema.as_str().lower() + assert "chat" not in sdl + assert "stream" not in sdl From 42c4359bb55d5bcd1cd7a57269c84b35cb5ab805 Mon Sep 17 00:00:00 2001 From: Shree Bohara Date: Tue, 11 Aug 2026 11:14:51 -0700 Subject: [PATCH 2/4] Make the GraphQL tests independent of the developer's .env 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 --- apps/api/tests/integration/test_graphql.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/apps/api/tests/integration/test_graphql.py b/apps/api/tests/integration/test_graphql.py index 14bc30d..6e918ff 100644 --- a/apps/api/tests/integration/test_graphql.py +++ b/apps/api/tests/integration/test_graphql.py @@ -30,7 +30,22 @@ @pytest.fixture() def gql_env(tmp_path, monkeypatch): - """Point the GraphQL resolvers at an isolated database and seed one repository.""" + """ + Point the GraphQL resolvers at an isolated database and seed one repository. + + Two details that exist because this test must not depend on the developer's machine: + + 1. A dummy OPENAI_API_KEY is set. Provider clients are constructed eagerly by the + dependency factories, so without a key the app cannot be built at all -- and on a + developer machine apps/api/.env silently supplies a real one, which made this pass + locally and fail in CI. No request here reaches a provider. + 2. TestClient is NOT used as a context manager, so the lifespan does not run. These + tests exercise the GraphQL resolvers, which open their own sessions via + get_session_factory; running the lifespan would additionally require a reachable + vector store. This matches how conftest.py builds its client. + """ + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-not-a-real-key") + engine = create_engine(f"sqlite:///{tmp_path / 'gql.db'}") Base.metadata.create_all(engine) factory = sessionmaker(bind=engine) @@ -56,8 +71,7 @@ def gql_env(tmp_path, monkeypatch): def _count(conn, cursor, statement, params, context, executemany): counter["n"] += 1 - with TestClient(app) as client: - yield client, repo_id, counter + yield TestClient(app), repo_id, counter def _post(client, query, **variables): From 06b3482a7c5c977dbe3e8b1bfc43cb0d23b4849e Mon Sep 17 00:00:00 2001 From: Shree Bohara Date: Tue, 11 Aug 2026 11:17:48 -0700 Subject: [PATCH 3/4] Assert the GraphQL mount behaviourally, not via app.routes 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 --- apps/api/tests/integration/test_graphql.py | 23 +++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/apps/api/tests/integration/test_graphql.py b/apps/api/tests/integration/test_graphql.py index 6e918ff..1dcbe53 100644 --- a/apps/api/tests/integration/test_graphql.py +++ b/apps/api/tests/integration/test_graphql.py @@ -83,12 +83,25 @@ def _post(client, query, **variables): def test_graphql_is_mounted_alongside_rest(gql_env): - """Additive, not a migration: the REST routes must still exist.""" - client, _, _ = gql_env + """ + Additive, not a migration. + + Asserted behaviourally rather than by introspecting app.routes: the path + GraphQLRouter registers differs between Strawberry versions (it resolved to + "/graphql" locally and to "" under the version CI installed), so checking the route + table tests the framework's internals instead of ours. + """ + client, repo_id, _ = gql_env + + # GraphQL answers. + res = client.post("/graphql", json={"query": "{ __typename }"}) + assert res.status_code == 200, res.text + assert res.json()["data"]["__typename"] == "Query" + + # And REST still answers, on the same app. + assert client.get(f"/api/learning/{repo_id}/stats").status_code == 200 paths = {getattr(r, "path", "") for r in app.routes} - assert "/graphql" in paths - assert any(p.startswith("/api/learning") for p in paths) - assert any(p.startswith("/api/chat") for p in paths) + assert any(p.startswith("/api/chat") for p in paths), "chat must remain on REST/SSE" def test_repo_query(gql_env): From 56f53c1e27928ee7f7ed765f5e012b48a90f1025 Mon Sep 17 00:00:00 2001 From: Shree Bohara Date: Tue, 11 Aug 2026 11:21:06 -0700 Subject: [PATCH 4/4] Test the chat mount behaviourally too 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 --- apps/api/tests/integration/test_graphql.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/api/tests/integration/test_graphql.py b/apps/api/tests/integration/test_graphql.py index 1dcbe53..827466e 100644 --- a/apps/api/tests/integration/test_graphql.py +++ b/apps/api/tests/integration/test_graphql.py @@ -100,8 +100,12 @@ def test_graphql_is_mounted_alongside_rest(gql_env): # And REST still answers, on the same app. assert client.get(f"/api/learning/{repo_id}/stats").status_code == 200 - paths = {getattr(r, "path", "") for r in app.routes} - assert any(p.startswith("/api/chat") for p in paths), "chat must remain on REST/SSE" + + # Chat is still routed. A 404 would mean the router is gone; an empty body should be + # rejected by validation instead, which proves the route exists without needing a + # provider or a real session. + chat = client.post("/api/chat/sessions", json={}) + assert chat.status_code != 404, "chat must remain on REST/SSE" def test_repo_query(gql_env):