Uh oh!
There was an error while loading. Please reload this page.
feat(academics): ingest real per-section Fall 2026 BU offerings (#280) - #512
feat(academics): ingest real per-section Fall 2026 BU offerings (#280)#512Darkest-Teddy wants to merge 7 commits into
Conversation
`course_offerings` has been hollow since the 0020 academics split: staging held 12,318 rows, of which 4 (the demo seed) carried a section and none carried a meeting time or location. Courses could not behave like a registrar. This ingests the operational layer for fall-2026 from bu.edu. Scraper (tasks 1). `_extract_schedule` flattened each course's schedule tables down to a semester list, discarding exactly the fields #280 needs. Replaced with `_extract_sections` + `_derive_schedule`: sections are read by table header name, not column position, so a column BU inserts doesn't silently shift the data. Staff/TBA instructors and "NO ROOM" locations become NULL instead of being stored literally. Added a `--rescan` mode that refetches only the URLs already in the JSON — sections were never stored, so recovering them needed a re-fetch, and skipping the listing walk preserves the index-orphan courses that only `probe_unscraped_courses.py` finds. Importer (task 2). `db/import_offerings.py` writes one offering per published section. It *adopts* rather than re-creates: staging already had 4,122 hollow fall-2026 offerings with enrollments, sessions, documents and notes pointing at them, and `course_offerings` is referenced by 8 tables with mixed CASCADE/SET NULL semantics, so delete-and-reinsert would have destroyed or orphaned real rows. The first published section takes over the existing section-less row in place, id preserved. Offerings the scrape no longer lists are reported, never deleted — a partial scrape must not be able to wipe a term. BU publishes multiple meeting patterns per section (CAS MA 123 A1 is an MWF lecture *and* a Thursday exam block). A keep-first dedup would have dropped a real meeting from 308 sections, so `_merge_meetings` joins them. School linkage (task 3). `--link-school` creates the Boston University `schools` row and links courses to it, after a duplicate-course_code precheck that refuses outright rather than half-linking. All 8,510 courses were already unique, so no merge was needed. Determinism fix. With one offering per section, `resolve_offering`'s `order="created_at.asc", limit=1` was non-deterministic — sections land in a single batch insert and share a created_at, so the winner was the planner's choice, and two calls could split one user's documents and notes across sections. Now ordered by `section.asc` first; '' sorts before 'A1', preserving pre-#280 behaviour wherever a hollow offering remains. No migration here. The `section NOT NULL DEFAULT ''` change this depends on is already in the chain as 0033 (recovered by #510), and 0036's now-unreachable NULL-section index was dropped by 20260801062439. Also corrects `.env.example` / `.env.staging.example`, which still handed out the direct `db.<ref>.supabase.co` host — IPv6-only, and the reason applying migrations failed here. `migrate.py` and CLAUDE.md were already fixed; these two templates were missed. Task 4 of #280 (API + frontend read path, letting a student pick their section) is deliberately out of scope. Verified on staging: 11,079 fall-2026 offerings, 10,958 sectioned and 121 left hollow for courses BU dropped from the schedule; meeting_times 100%, instructor 92%, location 59%; 0 duplicate (course, term, section); 0 orphaned enrollments/sessions/documents/notes; a re-run reports "Nothing to do". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:8 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR adds section-aware BU catalog scraping, rescan and verification tools, a dry-run offering importer, missing-course and school-linking workflows, deterministic offering selection, connection guidance, tests, and decision documentation. ChangesBU offering ingestion
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BUCatalog
participant scrape_bu_catalog
participant import_offerings
participant PostgREST
BUCatalog->>scrape_bu_catalog: course pages and schedule tables
scrape_bu_catalog-->>import_offerings: section-level catalog JSON
import_offerings->>PostgREST: resolve term and course rows
import_offerings->>PostgREST: insert or update course_offerings
PostgREST-->>import_offerings: synchronization results
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | a623d29 | Commit Preview URL Branch Preview URL | Aug 19 2026, 09:11 PM |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
backend/scripts/probe_unscraped_courses.py (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe probe defaults contradict the scraper's stated crawl policy.
scrape_bu_catalog.pyLines 53-56 state thatbu.edu/robots.txtasks forCrawl-delay: 15, and it defaults to one worker with a 15-second delay. This script targets the same host and reuses the same env var names, but defaults to 4 workers and 1.5 seconds. An operator who setsBU_CONCURRENCYandBU_PAGE_DELAYfor the scraper gets consistent behavior; an operator who sets neither gets a 40x faster crawl here.Either align the defaults with the scraper, or document why the probe's much smaller URL set justifies the difference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/probe_unscraped_courses.py` around lines 60 - 61, Align the probe defaults in CONCURRENCY and DELAY with the crawl policy used by scrape_bu_catalog.py: default to one worker and a 15-second delay while preserving the existing BU_CONCURRENCY and BU_PAGE_DELAY environment-variable overrides.backend/tests/test_scrape_bu_catalog.py (1)
146-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the listing-truncation branch.
The tests cover the parser well. They do not cover the
FETCH_FAILEDchange, which is the fix for the truncated crawl described inscrape_bu_catalog.pyLines 74-78. That branch is the highest-cost regression in this PR, and a silent return to the old behavior would again cut a school short without an error.
scrape_schoolaccepts a client, so the branch is testable without network access by passing a stub client that raiseshttpx.TimeoutException. Assert that_errorsgains aTRUNCATEDentry and that the walk stops.I can draft the test if you want.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_scrape_bu_catalog.py` around lines 146 - 150, Add a test for the FETCH_FAILED handling in scrape_school using a stub client that raises httpx.TimeoutException. Assert the result records a TRUNCATED entry in _errors and that traversal stops without processing subsequent listings, covering the truncated-crawl regression without network access.backend/scripts/verify_catalog_scrape.py (2)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
codes.count(c)branch.Line 53 uses
codes.count(c)inside a comprehension, which is O(n²). Lines 55-57 already implement the same result in O(n). Thelen(codes) < 3000guard bounds the cost at about 9 million comparisons, so this is not a real slowdown, but the branch adds no value.♻️ Proposed simplification
codes = [c.get("course_code", "") for c in catalog] - dupes = {c for c in codes if codes.count(c) > 1} if len(codes) < 3000 else None- if dupes is None:- seen, dupes = set(), set()- for c in codes:- (dupes if c in seen else seen).add(c)+ seen: set[str] = set()+ dupes: set[str] = set()+ for c in codes:+ (dupes if c in seen else seen).add(c)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/verify_catalog_scrape.py` around lines 53 - 57, Remove the conditional codes.count(c) comprehension and initialize dupes and seen through the existing linear-time iteration in the verification flow. Preserve the current duplicate-detection result and behavior for all code-list sizes, using the loop around seen and dupes as the sole implementation.
189-196: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe two early
continuepaths skipSPOT_DELAY.
time.sleep(SPOT_DELAY)runs at Line 227, after the comparison. Line 193 and Line 196 return to the top of the loop before that point. If bu.edu rate-limits or returns errors, the loop issues requests with no delay, which makes the rate limiting worse.Move the delay into a
finallyblock, or call it before eachcontinue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/verify_catalog_scrape.py` around lines 189 - 196, Ensure every iteration of the scrape loop applies SPOT_DELAY, including the exception and non-200 response paths around client.get and status_code handling. Move the existing delay from its current post-comparison location into a finally block covering the request and response processing, or invoke it before each early continue, while preserving the existing success-path behavior.backend/scripts/scrape_bu_catalog.py (2)
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
fetchreturn annotation for the new sentinel.
fetchis annotated-> Optional[str]on Line 82, but Line 100 now returnsFETCH_FAILED, anobject(). Runtime behavior is correct because callers useis FETCH_FAILED. The annotation no longer describes the contract, and a type checker cannot flag a caller that forgets the sentinel branch.♻️ Proposed typed sentinel
-FETCH_FAILED = object()+class _FetchFailed:+ """Sentinel type so `fetch` can be annotated precisely."""++FETCH_FAILED = _FetchFailed()Then update the signature:
asyncdeffetch(client: httpx.AsyncClient, url: str, retries: int=2) ->Union[str, None, _FetchFailed]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/scrape_bu_catalog.py` around lines 74 - 79, Update the fetch function’s return annotation to include the FETCH_FAILED sentinel type, defining or reusing a typed _FetchFailed symbol for the sentinel as needed. Preserve the existing str/None contract and FETCH_FAILED identity checks so callers can type-check all return branches.
222-226: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBuild the column map from one header row, not from every
<th>in the table.Line 223 collects every
<th>in the table. Line 226 then maps each header name to its index in that flat list. The mapping is only valid if the table contains exactly one header row.If a table repeats its header row, or nests a table,
headersgrows past the width of a data row.colthen keeps the last index for each name, andcell()returns""for every field because of thei < len(_cells)guard. The section code becomes"", and Line 243 drops every row of that table. The loss is silent, which is the failure mode this parser is meant to prevent.Scope the header lookup to the first row that contains
<th>.♻️ Proposed fix
for tbl in soup.find_all("table"): - headers = [th.get_text(" ", strip=True).lower() for th in tbl.find_all("th")]+ header_row = tbl.find("tr", recursive=True)+ while header_row is not None and not header_row.find_all("th", recursive=False):+ header_row = header_row.find_next_sibling("tr")+ if header_row is None:+ continue+ headers = [th.get_text(" ", strip=True).lower() for th in header_row.find_all("th")] if "section" not in headers: continue col = {name: i for i, name in enumerate(headers)}Add a regression test in
backend/tests/test_scrape_bu_catalog.pyfor a table that repeats its header row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/scrape_bu_catalog.py` around lines 222 - 226, Update the table parsing around the headers/col construction to select only the first table row containing <th> elements, then build the header list and column map from that row so repeated or nested headers cannot shift data indices. Add a regression test in test_scrape_bu_catalog.py using a table with a repeated header row and verify its data rows are still parsed.docs/decisions/0026-per-section-offering-ingest.md (1)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix two markdownlint findings.
Static analysis flags two formatting issues in this file:
- Line 19: the fenced code block has no language tag (MD040).
- Line 47: the line starts with
#280, which markdownlint reads as a heading missing a space after#(MD018).📝 Proposed formatting fixes
-```+```text Section | Instructor | Location | Schedule | Notes A1 | Erdos | LSE B01 | TR 2:00 pm-3:15 pm | ...```diff **Status: already applied.** This shipped as `0033_offering_section_not_null.sql`. It reached staging out-of-band during the -#280 work and was recovered into the repo by `#510` (one of the three migrations +\`#280` work and was recovered into the repo by `#510` (one of the three migrationsAlso applies to: 45-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0026-per-section-offering-ingest.md` around lines 19 - 22, Update the fenced code block containing the section offering table to include the text language tag. Adjust the line beginning with “#280” in the same document so markdownlint no longer interprets it as a malformed heading, while preserving the displayed content.Source: Linters/SAST tools
backend/db/import_offerings.py (2)
146-149: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider guarding the "one instructor per section" assumption.
_merge_meetingssilently takes the first non-nullinstructor_nameacross a section's rows, backed by the comment that this was "verified across the whole scrape that no multi-meeting section lists two different instructors." If a future scrape (or--rescan) violates that assumption, a second instructor is silently dropped with no diagnostic. Consider logging a warning when more than one distinct non-null instructor name is seen for a section, so a scraper regression surfaces instead of silently losing data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/db/import_offerings.py` around lines 146 - 149, Update _merge_meetings to collect distinct non-null instructor_name values from all rows and emit a warning when more than one unique instructor is present, while retaining the existing first-instructor selection behavior for the merged result.
89-100: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch
course_offeringsscoped to the target term.The existing-offerings loader pages through every
course_offeringsrow, then filters byterm_idin Python.select_with_countacceptsfilters, so pass{"term_id": f"eq.{term_id}"}from_all_rowsand remove the client-side discard to avoid transferring unrelated term rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/db/import_offerings.py` around lines 89 - 100, Update _all_rows to accept a term_id parameter and pass {"term_id": f"eq.{term_id}"} via select_with_count’s filters argument when loading course_offerings; remove the subsequent client-side term_id filtering so only target-term rows are fetched and retained.backend/tests/test_import_offerings.py (1)
23-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the fake Supabase table fixture into
backend/tests/conftest.py.
backend/tests/conftest.pyonly installs global hermetic guard fixtures; it does not provide a reusable fake table/store. This module andbackend/tests/test_seed_staging.pyboth define independent Supabase fakes with different PostgREST semantics. Centralize the shared fake fixture there and have both callers reuse/extend it so the mock DB contract cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_import_offerings.py` around lines 23 - 101, Move the shared _Store, _FakeTable, and store fixture definitions from test_import_offerings.py into backend/tests/conft.py, preserving their current behavior and patching of imp.table. Update test_import_offerings.py and test_seed_staging.py to reuse the centralized store fixture, extending or adapting it only where their Supabase/PostgREST behavior differs, and remove the duplicate fake implementations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/probe_unscraped_courses.py`:
- Around line 99-121: Track candidate URL transport failures separately from
genuinely retired courses in the probing flow around candidate_urls and the
httpx.HTTPError handler. Add and populate an unreachable bucket when all
attempts fail due to network errors, return it alongside the existing buckets,
and print it with the other result summaries; reserve retired for candidates
that complete probing without finding a valid course.
In `@backend/scripts/scrape_bu_catalog.py`:
- Around line 391-411: Update _refetch to reject redirected responses before
returning fresh: compare fresh["source_url"] with the original url, and when
they differ, append an appropriate rescan error and return None. Preserve the
existing empty-response and parse-failure handling so the in-place
rec.clear()/rec.update(fresh) path only processes records for the original URL.
In `@backend/scripts/verify_catalog_scrape.py`:
- Around line 212-217: Update the mismatch reporting in the section comparison
branch to sort section values safely when missing keys produce None. Preserve
the existing set comparison and diff output, but use a consistent comparable
ordering for values from stored and fresh sections so missing section codes are
reported instead of raising TypeError.
---
Nitpick comments:
In `@backend/db/import_offerings.py`:
- Around line 146-149: Update _merge_meetings to collect distinct non-null
instructor_name values from all rows and emit a warning when more than one
unique instructor is present, while retaining the existing first-instructor
selection behavior for the merged result.
- Around line 89-100: Update _all_rows to accept a term_id parameter and pass
{"term_id": f"eq.{term_id}"} via select_with_count’s filters argument when
loading course_offerings; remove the subsequent client-side term_id filtering so
only target-term rows are fetched and retained.
In `@backend/scripts/probe_unscraped_courses.py`:
- Around line 60-61: Align the probe defaults in CONCURRENCY and DELAY with the
crawl policy used by scrape_bu_catalog.py: default to one worker and a 15-second
delay while preserving the existing BU_CONCURRENCY and BU_PAGE_DELAY
environment-variable overrides.
In `@backend/scripts/scrape_bu_catalog.py`:
- Around line 74-79: Update the fetch function’s return annotation to include
the FETCH_FAILED sentinel type, defining or reusing a typed _FetchFailed symbol
for the sentinel as needed. Preserve the existing str/None contract and
FETCH_FAILED identity checks so callers can type-check all return branches.
- Around line 222-226: Update the table parsing around the headers/col
construction to select only the first table row containing <th> elements, then
build the header list and column map from that row so repeated or nested headers
cannot shift data indices. Add a regression test in test_scrape_bu_catalog.py
using a table with a repeated header row and verify its data rows are still
parsed.
In `@backend/scripts/verify_catalog_scrape.py`:
- Around line 53-57: Remove the conditional codes.count(c) comprehension and
initialize dupes and seen through the existing linear-time iteration in the
verification flow. Preserve the current duplicate-detection result and behavior
for all code-list sizes, using the loop around seen and dupes as the sole
implementation.
- Around line 189-196: Ensure every iteration of the scrape loop applies
SPOT_DELAY, including the exception and non-200 response paths around client.get
and status_code handling. Move the existing delay from its current
post-comparison location into a finally block covering the request and response
processing, or invoke it before each early continue, while preserving the
existing success-path behavior.
In `@backend/tests/test_import_offerings.py`:
- Around line 23-101: Move the shared _Store, _FakeTable, and store fixture
definitions from test_import_offerings.py into backend/tests/conft.py,
preserving their current behavior and patching of imp.table. Update
test_import_offerings.py and test_seed_staging.py to reuse the centralized store
fixture, extending or adapting it only where their Supabase/PostgREST behavior
differs, and remove the duplicate fake implementations.
In `@backend/tests/test_scrape_bu_catalog.py`:
- Around line 146-150: Add a test for the FETCH_FAILED handling in scrape_school
using a stub client that raises httpx.TimeoutException. Assert the result
records a TRUNCATED entry in _errors and that traversal stops without processing
subsequent listings, covering the truncated-crawl regression without network
access.
In `@docs/decisions/0026-per-section-offering-ingest.md`:
- Around line 19-22: Update the fenced code block containing the section
offering table to include the text language tag. Adjust the line beginning with
“#280” in the same document so markdownlint no longer interprets it as a
malformed heading, while preserving the displayed content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbfa0fe8-0136-47e2-bfe0-10a690649915
📒 Files selected for processing (13)
.gitignoreCLAUDE.mdbackend/.env.examplebackend/.env.staging.examplebackend/db/import_offerings.pybackend/scripts/probe_unscraped_courses.pybackend/scripts/scrape_bu_catalog.pybackend/scripts/verify_catalog_scrape.pybackend/services/academics.pybackend/tests/test_academics.pybackend/tests/test_import_offerings.pybackend/tests/test_scrape_bu_catalog.pydocs/decisions/0026-per-section-offering-ingest.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Correctness:
- probe_unscraped_courses: a code whose every candidate URL died in transport
was filed as "retired (404)". Nothing was learned about those courses, and the
retired list exists to drive a cleanup decision — so a flaky network could
have argued for deleting live courses. Transport failures now land in a
separate `unreachable` bucket; `retired` means "answered, and no valid page".
- scrape_bu_catalog: `_extract_sections` read headers from every <th> in the
table. On a page that repeats its header row mid-table the dict comprehension
kept the LAST index per name, shifting instructor into location for every row
below the repeat. Headers now come from the first <th> row only. Regression
test added.
- scrape_bu_catalog: `fetch` follows redirects, so a retired slug can serve an
unrelated course's page, and `rescan` would then overwrite one course's record
with another's. `_refetch` now rejects a response whose course_code isn't the
one being refreshed. (Reviewer suggested comparing source_url, but
parse_course stamps that from the requested URL, so it can never differ — the
code on the page is the only tell.)
- verify_catalog_scrape: the section-set mismatch branch sorted values that can
be None — exactly the missing-section-code case the check exists to report —
raising TypeError instead. Now sorted through a None-tolerant key.
Politeness / cost:
- verify_catalog_scrape: SPOT_DELAY was skipped on all three error paths, so a
struggling bu.edu got hammered hardest. Moved into a finally.
- probe_unscraped_courses: defaults were 4 workers / 1.5s against the same host
and same robots.txt the crawler honors at 1 worker / 15s (Crawl-delay: 15).
Aligned; BU_CONCURRENCY / BU_PAGE_DELAY still override.
- import_offerings: `_all_rows("course_offerings", ...)` pulled every term's
rows and filtered client-side. Filtered server-side instead.
Clarity:
- import_offerings: `_merge_meetings` silently kept the first instructor. That
no multi-meeting section disagrees was a point-in-time check, not a BU
guarantee, so it now warns when it does.
- scrape_bu_catalog: FETCH_FAILED was a bare object() while `fetch` was
annotated `Optional[str]`. Gave the sentinel a type and a FetchResult alias.
- verify_catalog_scrape: dropped the O(n^2) `codes.count(c)` branch; the linear
path it fell back to above 3,000 records handles every size.
- ADR 0026: fenced block tagged `text`; rewrapped a line starting with "#280"
that markdownlint read as a malformed heading.
Test added for the truncated-crawl regression the FETCH_FAILED sentinel exists
to prevent: a listing timeout must record TRUNCATED, not read as the end of
pagination (the bug that cut the previous crawl to 416 of 2,253 CAS courses).
Not done: moving the _Store/_FakeTable fakes into conftest.py and sharing them
with test_seed_staging.py. That pulls an unrelated test file into this PR, and
the two fakes model different PostgREST surfaces; keeping them local keeps each
suite's failure modes independent.
Verified: 1,592 passed, 38 skipped. Ruff clean. Verifier re-run offline over the
real 8,857-record scrape reports identical numbers to before the dedup rewrite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
Darkest-Teddy
commented
Aug 2, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
Darkest-Teddy
commented
Aug 2, 2026
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 minutes. |
- import_offerings: `sys.exit(main())` called sys.exit on a procedure that returns None, so it was always sys.exit(None). Failures already raise SystemExit from run()/_resolve_term and carry their own status, so the wrapper only obscured that. Call main() directly; `sys` is no longer needed. - test_scrape_bu_catalog: the module was pulled in with both `import scrape_bu_catalog as scrape` and `from scrape_bu_catalog import ...`. Dropped the module import; the truncated-crawl test patches through monkeypatch's string targets and asserts on a list it owns. Behaviour unchanged. 1,592 passed, 38 skipped; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Code review — per-section Fall 2026 BU offeringsThis lands the operational layer for
Findings below are things I could not talk myself out of. Nothing rises to a data-loss or security issue. Findings[P1] ifnotto_insertandnotto_update:
print("\nNothing to do — already in sync.")
returnThis [P2] Registrar placeholder schedules are stored verbatim in _NO_LOCATION= {"no room", "tba", "tbd", "arr", ""}
...
t= (s.get("meeting_times") or"").strip()
iftandtnotintimes:
times.append(t)
loc= (s.get("location") or"").strip()
iflocandloc.lower() notin_NO_LOCATIONandlocnotinlocations:
locations.append(loc)
[P2] Updates and adoptions are one HTTP PATCH per row while inserts are batched — foriinrange(0, len(to_insert), BATCH):
table("course_offerings").insert(to_insert[i : i+BATCH])
print(f" inserted {min(i+BATCH, len(to_insert)):,}/{len(to_insert):,}", flush=True)
forn, (offering_id, patch) inenumerate(to_update, 1):
table("course_offerings").update(patch, filters={"id": f"eq.{offering_id}"})
[P2] # No offering in the target term and not creating — fall back to any offering# of this course so reads still resolve to something sensible.any_off=table("course_offerings").select(
"id", filters={"course_id": f"eq.{course_id}"}, limit=1
)
returnany_off[0]["id"] ifany_offelseNoneADR 0026 Decision 3 and the fix at [P3] The 409 race re-select still orders by rows=table("course_offerings").select(
"id",
filters={"course_id": f"eq.{course_id}", "term_id": f"eq.{term_id}"},
order="created_at.asc",
limit=1,
)Sibling of the query fixed 20 lines above, same function, same tuple, left on the ordering the comment at [P3] whileTrue:
rows, total=table(name).select_with_count(
columns, filters=filters, order="id.asc", limit=PAGE, offset=offset
)
out.extend(rows)
offset+=len(rows)
ifnotrowsoroffset>=total:
returnout
What's good
Verdict: request changes — the Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy |
…query The cross-term fallback and the post-409 re-select were still on created_at.asc (or no order at all). Since #280 a course has one offering per published section, all written by one batch insert, so created_at ties and the winner is the planner's — two calls could hand the same user different sections and split their documents/notes. The fallback is the worse of the two: it is unfiltered by term, so it picks among every section in every term, latent only while current_term() is the newest seeded term. Extends ADR 0026 Decision 3 to name all three reads.
test_import_offerings.py and test_seed_staging.py each carried their own Supabase fake with quietly different insert/upsert semantics: a script could satisfy one module's model of PostgREST and break against the other's, and a fix to one fake never reached the second. One shared store/table pair now serves both — rows keyed by primary key like the real database, eq./in.() filters, paged select_with_count, insert, update, upsert(on_conflict) — with the per-table primary key (user_profiles keyed on user_id) passed in rather than forked. It also records writes, so a test can assert HOW a script wrote. No semantic divergence turned out to be genuine; the only real difference was the primary-key column, now a constructor argument.
…nest paging --link-school was dropped whenever the offering sync had nothing to do: the "already in sync" early return fired before it, so the documented runbook command linked nothing on any re-run — and the re-run is the path the flag exists for, since link_school [FAIL]s on duplicate course_codes and asks the operator to merge them and run again. The branch now still runs the linking step (respecting --apply) and says so. Placeholder schedules were the one student-facing column with no placeholder handling: instructor is nulled at scrape time and location at import time, but an arranged section's "ARR 12:00 am-12:00 am" was stored verbatim and shown as a real class time. Nulled on both sides now (the importer also runs against older catalog files), with an ARR-aware check rather than an exact-match set, and the verifier fails a scrape that stored one instead of counting it as meeting_times coverage. Updates and adoptions went out one PATCH per row while inserts were batched — O(sections) sequential round trips, 4,122 of them for staging's hollow fall-2026 offerings alone, each its own transaction and each firing trg_course_offerings_updated_at. They now use upsert(on_conflict=id) in the same 500-row batches; rows are sent whole because ON CONFLICT checks NOT NULL before it detects the conflict. _all_rows returned a truncated table without a word whenever Content-Range was missing (select_with_count reports total=0, so offset >= total read 1000 >= 0). It now stops on a short page and treats the count as advisory — truncating 'existing' is the direction that queues inserts for sections that already exist and takes a duplicate-key 409 mid-batch. Same copy fixed in probe_unscraped_courses.py.
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Review fixes appliedEvery outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed. Major
Minor
Nits409-race re-select ordered consistently · AlsoThe PR body said Verification — Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate. |
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Verify against the real database before mergingI could not reach a live database while working on this — there are no credentials on this machine (only These are the checks that need a real connection. This is the PR where the database check matters mostIt writes ~11k rows and adopts 4,122 existing hollow offerings in place, keeping their ids so enrollments, sessions, documents and notes stay attached. Nothing here can be judged from the repo alone. 1. Always dry-run first — it is the defaultCompare against the numbers in the PR body: 11,079 fall-2026 offerings, 10,958 sectioned, 121 left hollow. A materially different count means the scrape or the term changed underneath it. 2. The placeholder change alters rows that are ALREADY storedThe fix nulls registrar filler ( SELECTcount(*) FROM course_offerings
WHERE term_id ='fall-2026'AND meeting_times ~*'^(ARR|TBA|TBD)';Confirm a re-run actually queues these as updates rather than reporting "already in sync". If the count is non-zero before the run and unchanged after, the reconciliation is comparing post-normalisation values on both sides and silently skipping them — please check before trusting the coverage figure, since 3. |
Refs #280 (tasks 1–3). Does not close#280 — task 4 (API + frontend read path, letting a student pick the section they actually registered for) is deliberately deferred, so the issue must stay open after this merges.
Why
course_offeringshas been hollow since the 0020 academics split. Staging held 12,318 rows, of which 4 (the demo seed) carried a section and none carried a meeting time or location. Courses could not behave like a registrar.This ingests the operational layer for
fall-2026from bu.edu: one offering per published section, carryingsection/instructor_name/meeting_times/location.What changed
Scraper (task 1) —
_extract_scheduleflattened each course's schedule tables down to a semester list, discarding exactly the fields #280 needs. Replaced with_extract_sections+_derive_schedule:Staff/TBAinstructors andNO ROOMlocations becomeNULLrather than being stored literally and shown to students.--rescanmode refetches only the URLs already in the JSON. Sections were never stored, so recovering them required a re-fetch; skipping the listing walk preserves the index-orphan courses that onlyprobe_unscraped_courses.pyfinds.Importer (task 2) —
backend/db/import_offerings.py, dry-run by default.It adopts rather than re-creates. Staging already had 4,122 hollow fall-2026 offerings with enrollments, sessions, documents and notes pointing at them, and
course_offeringsis referenced by 8 tables with mixedCASCADE/SET NULLsemantics — a delete-and-reinsert import would have destroyed or silently orphaned real rows. So for each course the first published section takes over the existing section-less row in place, id preserved, and only the remaining sections are inserted. Re-running matches every section by code and writes nothing.Offerings the scrape no longer lists are reported, never deleted. A partial or failed scrape must not be able to wipe a term.
BU publishes multiple meeting patterns per section — CAS MA 123 A1 is an MWF 9:05 lecture and a Thursday 6:30 exam block. A keep-first dedup would have dropped a real meeting from 308 sections, so
_merge_meetingsjoins them ("R 6:30 pm-8:30 pm; MWF 9:05 am-9:55 am"). Verified across the whole scrape that no multi-meeting section has two different instructors.School linkage (task 3) —
--link-schoolcreates the Boston Universityschoolsrow and links courses to it, after a duplicate-course_codeprecheck that refuses outright rather than half-linking. All 8,510 courses turned out to be unique, so the merge work #280 anticipated wasn't needed.Determinism fix — with one offering per section,
resolve_offering'sorder="created_at.asc", limit=1was non-deterministic: sections land in a single batch insert and share acreated_at, so the winner was the planner's choice. Two calls could split one user's documents and notes across sections. Now ordered bysection.ascfirst;''sorts beforeA1, preserving pre-#280 behaviour wherever a hollow offering remains.No migration in this PR
The
section NOT NULL DEFAULT ''change this depends on is already on main as0033_offering_section_not_null.sql(recovered by #510 — it reached staging out-of-band during this work), and 0036's now-unreachable NULL-section partial index was dropped by20260801062439_drop_dead_null_section_index.sql. Reasoning for choosing the non-null default overNULLS NOT DISTINCTis recorded in the ADR.Separable
backend/.env.exampleandbackend/.env.staging.examplestill handed out the directdb.<ref>.supabase.cohost, which is IPv6-only and is the reason applying migrations failed locally.migrate.pyandCLAUDE.mdwere already fixed upstream; these two templates were missed. Happy to split this into its own PR if preferred.Out of scope
Task 4 (API + frontend read path, letting a student pick the section they actually registered for) is not here. Today a student enrolling in CAS CS 330 lands in A1 regardless — this PR makes that choice stable, not correct.
Verification
Staging, after applying:
meeting_times100%,instructor_name92%,location59%(course, term, section); 0 orphaned enrollments / sessions / documents / notesSTO B50Tests: 53 targeted tests pass (
test_scrape_bu_catalog.py12,test_import_offerings.py20 against a hermetic PostgREST fake, plus academics). Full backend suite: 1,585 passed, 41 skipped. Ruff clean on every touched file.Unaffected by design
courses.id, and the import resolves eachcourse_codeto the existing catalog row rather than creating a parallel course — sections cannot fork a course's graph identity.course_chunksoncourse_code, notoffering_id, so the shared-course-corpus property survives sections.enrollment_idand study analytics onoffering_id; both keep working because adoption preserves offering ids.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation