services/course_context_service.py::update_course_context is the single write chokepoint for the
class-aggregate rollup (offering_concept_stats / offering_summary). PR #587's /code-review
pass (medium effort, 2026-08-26) turned up five pre-existing debts in the function while reviewing
the unrelated effective_explanations dead-read deletion. None of these are regressions from #587
— they predate it — so they're filed here rather than fixed in that PR.
F1 — two early exits skip the purge-on-empty / cache-clear the #72 write chokepoint otherwise guarantees
if not abstract_course_id: return (course_context_service.py:199) and if not node_rows: return
(course_context_service.py:214-215) both bail with a bare return. Contrast the sibling early exits
a few lines up (:174-179 "no enrollment" and :184-192 "all opted out"), which purge the stale
offering_concept_stats/offering_summary rows and call clear_course_context_cache() before
returning. If a course's abstract-course resolution goes empty, or every enrolled student's graph
nodes for the course disappear (e.g. the last node deleted), the previous refresh's published
aggregates are left standing — stale data keeps being served with no equivalent purge.
Separately, the step-7 upsert loop (course_context_service.py:319-338) only iterates
concept_metrics for concepts present in this refresh — it has no delete step for concepts that
dropped out since the last refresh (e.g. a concept's only node got removed). Those rows sit in
offering_concept_stats forever, orphaned.
F2 — two silent-empty catches turn a transient failure into "no quiz context", and the caller then overwrites good data with []
_fetch_quiz_context_rows's except Exception: rows = [] (course_context_service.py:270-271) and
the decrypt fallback except Exception: cj = {} (course_context_service.py:301-302) both swallow
the exception with no logging — a transient PostgREST error or a decrypt failure looks identical to
"this concept genuinely has no quiz context yet." That's the #529/#548 silent-empty class
(services/tool_signals.py::report_empty_result exists for exactly this shape of ambiguity and
isn't used here).
It compounds with the upsert: _parse_quiz_context_to_arrays's output (cm, pg) is written
unconditionally into the per-concept upsert (course_context_service.py:324-338), which runs
on_conflict="offering_id,concept_name" — a merge-duplicates UPDATE on an existing row. If the row
already had real common_misconceptions/prerequisite_gaps from a previous successful refresh, a
transient failure this time silently overwrites them with [] instead of leaving the previous
(good) values in place or skipping the write.
F4 — the parse loop has no legacy-shape guards; its sibling reader does
_parse_quiz_context_to_arrays's two loops (course_context_service.py:304-314) do
for m in cj.get("common_mistakes", []): / for w in cj.get("weak_areas", []): with no
isinstance(..., list) check. agents/tools/quiz_history.py::_coerce_summary reads the same
quiz_context.context_json shape and explicitly guards this
(agents/tools/quiz_history.py:103-104: if not isinstance(raw, list): continue) because
"legacy free-form rows can hold a string (or dict) under a list-shaped key." Here, a string under
either key iterates character-by-character and sprays single-character "misconceptions" into the
class aggregate; a dict under the key raises TypeError inside the for loop. That exception
propagates out of update_course_context, but every call site wraps it in a bare
try: ... except Exception: pass (e.g. services/graph_service.py:447-450, :504-507, :540-541,
:889-890), so the failure is silently dropped rather than logged.
F11/F12 — N+1 reads and N+1 writes in the per-concept loop
_fetch_quiz_context_rows (course_context_service.py:257-273) is called once per concept inside the
step-7 loop (course_context_service.py:321), even though every node id across every concept is
already known before the loop starts (concept_data, built in step 4). It could be fetched once for
the full node-id set and grouped by concept afterward, instead of one quiz_context SELECT (or
several, given the existing chunking) per concept.
Symmetrically, each concept issues its own single-row table("offering_concept_stats").upsert(...)
call (course_context_service.py:324-338). db/connection.py::SupabaseTable.upsert posts data
straight through to PostgREST (Prefer: resolution=merge-duplicates), which accepts a JSON array
for a native batch upsert — so all of this refresh's concept rows could go in one call instead of N.
For a course with many concepts this is 2N extra round-trips per update_course_context call.
Not fixing here
These are all pre-existing in the touched function, not introduced by #587's dead-read deletion —
filing rather than scope-creeping that PR.
Refs #587 (the deletion PR whose review turned these up).
Origin: PR #587/code-review medium pass, 2026-08-26.
services/course_context_service.py::update_course_contextis the single write chokepoint for theclass-aggregate rollup (
offering_concept_stats/offering_summary). PR #587's/code-reviewpass (medium effort, 2026-08-26) turned up five pre-existing debts in the function while reviewing
the unrelated
effective_explanationsdead-read deletion. None of these are regressions from #587— they predate it — so they're filed here rather than fixed in that PR.
F1 — two early exits skip the purge-on-empty / cache-clear the #72 write chokepoint otherwise guarantees
if not abstract_course_id: return(course_context_service.py:199) andif not node_rows: return(course_context_service.py:214-215) both bail with a bare
return. Contrast the sibling early exitsa few lines up (:174-179 "no enrollment" and :184-192 "all opted out"), which purge the stale
offering_concept_stats/offering_summaryrows and callclear_course_context_cache()beforereturning. If a course's abstract-course resolution goes empty, or every enrolled student's graph
nodes for the course disappear (e.g. the last node deleted), the previous refresh's published
aggregates are left standing — stale data keeps being served with no equivalent purge.
Separately, the step-7 upsert loop (course_context_service.py:319-338) only iterates
concept_metricsfor concepts present in this refresh — it has no delete step for concepts thatdropped out since the last refresh (e.g. a concept's only node got removed). Those rows sit in
offering_concept_statsforever, orphaned.F2 — two silent-empty catches turn a transient failure into "no quiz context", and the caller then overwrites good data with
[]_fetch_quiz_context_rows'sexcept Exception: rows = [](course_context_service.py:270-271) andthe decrypt fallback
except Exception: cj = {}(course_context_service.py:301-302) both swallowthe exception with no logging — a transient PostgREST error or a decrypt failure looks identical to
"this concept genuinely has no quiz context yet." That's the #529/#548 silent-empty class
(
services/tool_signals.py::report_empty_resultexists for exactly this shape of ambiguity andisn't used here).
It compounds with the upsert:
_parse_quiz_context_to_arrays's output (cm,pg) is writtenunconditionally into the per-concept upsert (course_context_service.py:324-338), which runs
on_conflict="offering_id,concept_name"— a merge-duplicates UPDATE on an existing row. If the rowalready had real
common_misconceptions/prerequisite_gapsfrom a previous successful refresh, atransient failure this time silently overwrites them with
[]instead of leaving the previous(good) values in place or skipping the write.
F4 — the parse loop has no legacy-shape guards; its sibling reader does
_parse_quiz_context_to_arrays's two loops (course_context_service.py:304-314) dofor m in cj.get("common_mistakes", []):/for w in cj.get("weak_areas", []):with noisinstance(..., list)check.agents/tools/quiz_history.py::_coerce_summaryreads the samequiz_context.context_jsonshape and explicitly guards this(
agents/tools/quiz_history.py:103-104:if not isinstance(raw, list): continue) because"legacy free-form rows can hold a string (or dict) under a list-shaped key." Here, a string under
either key iterates character-by-character and sprays single-character "misconceptions" into the
class aggregate; a dict under the key raises
TypeErrorinside theforloop. That exceptionpropagates out of
update_course_context, but every call site wraps it in a baretry: ... except Exception: pass(e.g.services/graph_service.py:447-450,:504-507,:540-541,:889-890), so the failure is silently dropped rather than logged.F11/F12 — N+1 reads and N+1 writes in the per-concept loop
_fetch_quiz_context_rows(course_context_service.py:257-273) is called once per concept inside thestep-7 loop (course_context_service.py:321), even though every node id across every concept is
already known before the loop starts (
concept_data, built in step 4). It could be fetched once forthe full node-id set and grouped by concept afterward, instead of one
quiz_contextSELECT (orseveral, given the existing chunking) per concept.
Symmetrically, each concept issues its own single-row
table("offering_concept_stats").upsert(...)call (course_context_service.py:324-338).
db/connection.py::SupabaseTable.upsertpostsdatastraight through to PostgREST (
Prefer: resolution=merge-duplicates), which accepts a JSON arrayfor a native batch upsert — so all of this refresh's concept rows could go in one call instead of N.
For a course with many concepts this is 2N extra round-trips per
update_course_contextcall.Not fixing here
These are all pre-existing in the touched function, not introduced by #587's dead-read deletion —
filing rather than scope-creeping that PR.
Refs #587 (the deletion PR whose review turned these up).
Origin: PR #587
/code-reviewmedium pass, 2026-08-26.