DocumentSource-shaped index, SEO surface, staged R2 publish & Postgres document index - #88
Conversation
…sh, Postgres index
Make every NGM document a first-class, publicly discoverable record and keep an
up-to-date queryable index of all of them.
Index format (DocumentSource-compatible):
- Manuscript carries a roled link list (links: [{link, role}]), a stable
document_id, and a source_type, mirroring the Jawafdehi API DocumentSource
shape field-for-field; url/file_name kept as the RAW back-compat alias.
- Builders consolidate to ONE manuscript per logical document: CIAA press
releases group by press_id (PDF=RAW, others=ALTERNATE, ciaa page=SOURCE_PAGE),
court-order leaves collapse per case_number.
SEO surface (crawlable, streamed):
- build_index emits a static HTML landing page per document under /d/ (canonical,
Open Graph, JSON-LD CreativeWork, roled links, transcript placeholder), a
sitemap index + per-dataset child sitemaps (split at 50k URLs), and robots.txt.
- Streaming writer bounds memory regardless of document count (1M+ safe).
Publish (local staging -> R2 mirror):
- IndexBuilder splits read-root (uploads/) from write-root (output_root); main()
builds the derived tree to a local /tmp staging dir, then publish.py bulk-
uploads via boto3 with explicit Content-Type and syncs deletions so the store
is a clean mirror. Delete scope is guarded to the SEO surface + current
indices/<date>/; uploads/ and older snapshots are never touched.
Postgres index:
- New document_sources table (created by init_db/create_all) + db_index.py
upsert/mirror keyed on last_seen_build, so the whole archive is SQL-queryable
alongside the court tables.
Tests: 157 passing (format consolidation, link validity, SEO output + pagination,
read/write split, publish sync safety incl. fake-S3 end-to-end, row collection).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 45 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds manuscript/source roles and stable document IDs, mirrors them into a new ChangesDocument source indexing pipeline
Sequence Diagram(s)sequenceDiagram
participant main
participant IndexBuilder
participant sync_document_sources
participant publish
participant S3CompatibleStore
main->>IndexBuilder: build tree and write index + SEO files
main->>sync_document_sources: mirror document_sources rows when enabled
main->>publish: publish staged files for remote stores
publish->>S3CompatibleStore: upload staged keys and delete managed orphans
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Postgres-backed document_sources index that mirrors the hierarchical index tree of the NGM archive, alongside an SEO-friendly surface consisting of crawlable HTML landing pages, sitemaps, and robots.txt. It updates the Manuscript model to support roled links and stable document IDs, implements a streaming publisher to sync staged files to Cloudflare R2, and adds database synchronization logic. The review feedback highlights a potential Cross-Site Scripting (XSS) vulnerability in the JSON-LD script generation, a potential KeyError when accessing the link dictionary directly, and a risk of primary key conflicts if press_id is missing from press release metadata.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| raw_link = next( | ||
| ( | ||
| link["link"] | ||
| for link in ms.links | ||
| if link.get("role") == SourceLinkRole.RAW.value | ||
| ), | ||
| "", | ||
| ) |
There was a problem hiding this comment.
Using direct dictionary lookup link["link"] can raise a KeyError if the "link" key is missing from a legacy or malformed manuscript's links. Since link.get("link", "") is already used safely elsewhere in this method (e.g., on line 1279), we should use it here as well for consistency and safety.
raw_link = next(\n (\n link.get("link", "")\n for link in ms.links\n if link.get("role") == SourceLinkRole.RAW.value\n ),\n "",\n )| return None | ||
|
|
||
| return manuscripts | ||
| press_id = metadata.get("press_id") |
There was a problem hiding this comment.
If press_id is missing or None in the metadata dictionary, document_id will be generated as ngm:ciaa-press-release:None. If multiple press releases have a missing press_id, they will all share the same document_id, causing primary key conflicts and overwrites in the database.\n\nFalling back to metadata_path.stem (which is the filename, e.g., "1234") ensures a unique and stable identifier even if press_id is missing from the JSON metadata.
| press_id = metadata.get("press_id") | |
| press_id = metadata.get("press_id") or metadata_path.stem |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@ngm/index/build_index.py`:
- Around line 508-516: The Manuscript construction in the CIAA press-release
path should reject missing or blank press_id before building document_id. In the
relevant builder that reads metadata and returns Manuscript, validate
metadata.get("press_id") is present and non-empty, and fail fast if it is not.
Keep the document_id format in place only after this check so the dedupe key and
document_sources.document_id stay unique and do not collapse bad metadata into
the same ngm:ciaa-press-release:* id.
- Around line 509-513: The Manuscript construction is mismatching the promoted
primary attachment because file_name still uses file_names[0] while url comes
from _primary_url(links). Update the logic in the Manuscript creation path so
file_name is derived from the same promoted RAW/PDF attachment selected by
_primary_url, keeping the two fields aligned; use the existing helpers around
primary_name, _primary_url, and the Manuscript initializer to make the change.
- Around line 1419-1466: Use a per-run build id for the DB mirror sweep instead
of the shared date string. In main(), generate a unique build identifier for
each execution and pass that into sync_document_sources() so last_seen_build
changes every run; keep date_str only for date-based paths like IndexBuilder.
Update the call site and any nearby logging/variables in build_index.py to
clearly distinguish the run id from the calendar date.
- Around line 1294-1342: The JSON-LD block in the HTML rendering is not escaped
before being embedded in the `<script>` tag, which can allow scraped metadata to
terminate the script early. Update the JSON serialization in the
`build_index.py` HTML template generation to use a script-safe JSON-LD string
(for example, by escaping `</` before insertion) in the same place where
`json.dumps(jsonld, ensure_ascii=False)` is currently used.
In `@ngm/index/db_index.py`:
- Around line 87-119: `sync_document_sources()` is using `build_id` as the
stale-row mirror marker, but the caller in `build_index` is passing `date_str`,
which is shared across same-day runs. Update the call site that invokes
`sync_document_sources` to pass a unique per-run marker (for example a UTC
timestamp or UUID) instead of `date_str`, and keep `date_str` only for
naming/SEO uses. Make sure the stale delete condition in `sync_document_sources`
continues to compare against that per-run marker so later reruns can remove rows
that disappeared in the new build.
In `@ngm/index/publish.py`:
- Around line 158-161: The stale-object cleanup in `publish.py` is assuming
every `client.delete_objects` call succeeds, but S3 can return per-key `Errors`
even when the HTTP response is 200. Update the delete flow in the batch loop to
capture the `delete_objects` response, inspect `Errors`, and surface or handle
any failures before continuing. Then make the final `deleted` count in the
publishing logic reflect only successfully deleted keys, using the
`delete_objects` result rather than blindly counting all items in `stale`.
- Around line 132-134: The upload scheduling in publish should not enqueue one
Future for every staged file up front. Update the ThreadPoolExecutor flow in the
publish logic around the put submission and as_completed handling so uploads are
submitted in a bounded batch/streamed queue, using the existing _UPLOAD_WORKERS
limit to cap pending futures. Keep the same failure handling and result draining
behavior, but avoid building a large futures map before any completions are
consumed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84395501-a58f-41da-90bc-4ff4f15a1973
📒 Files selected for processing (10)
ngm/database/models.pyngm/index/build_index.pyngm/index/db_index.pyngm/index/models.pyngm/index/publish.pytests/test_db_index.pytests/test_document_format.pytests/test_output_split.pytests/test_publish.pytests/test_seo_output.py
…efresh updated_at - SECURITY: escape <, >, & in the JSON-LD <script> block so scraped text containing "</script>" can't break out of / inject into the landing page. - SAFETY: abort the build when it produces 0 documents (a source-read failure), so an empty staging tree can't sync-delete every live page/sitemap or, next day, drop every document_sources row. - CORRECTNESS: use a per-run timestamp (not just the date) as the DB build_id so a same-day rebuild still prunes documents that vanished since the earlier run. - Refresh updated_at=now() on conflict in the document_sources upsert (and fix the stale comment). Adds regression tests for the JSON-LD escaping and the empty-build abort. 159 tests passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…DRY, bounded txns)
- press_id falls back to the metadata filename stem (files are {press_id}.json)
so a document_id is never "ngm:ciaa-press-release:None".
- filename-derived ids (kanun-patrika, annual reports, PPMO) get a short
deterministic hash suffix (_slug_with_hash) so distinct filenames that slugify
identically never collide on a document_id / HTML page / DB PK.
- single source of truth for the HTML landing-page path: models.document_html_relpath,
used by both the builder and the DB indexer (removes the duplicated helper).
- document_sources upsert commits per 1000-row batch (and the stale sweep runs in
its own transaction) so a 1M-row sync is not one giant transaction.
Adds tests for slug-hash uniqueness and the press_id fallback. 161 tests passing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Still-valid findings from the automated reviewers on PR #88 (the JSON-LD escape and per-run build_id were already fixed): - build_index: raw_link extraction uses link.get("link","") (no KeyError on a malformed link dict). - build_index: press_id falls back to the metadata stem when absent OR blank; url/file_name now both follow the promoted RAW attachment (PDF), so they no longer disagree for multi-attachment press releases. - publish._upload: bounded in-flight window (~4x workers) instead of scheduling one Future per staged file up front (1M files -> 1M Futures). - publish._delete: inspect delete_objects per-object "Errors" and raise, instead of silently leaving orphaned objects behind. Adds tests for the delete-error path and the url/file_name RAW alignment. 162 tests passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g, no writes NGM_DRY_RUN=1 builds the full tree from the (cloud) read store to local staging but makes NO writes back — no R2 publish, no DB sync — so a real rebuild can be measured/inspected with read-only access to the store. The staging dir is kept for inspection on a dry run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5min -> ~15s) The court-orders read phase dominated the nightly rebuild (~35 min) because the builder enumerated ~23k files via cloudpathlib iterdir()+is_file(), and is_file() on a cloud path costs a network round-trip each. Replace it with a single recursive list_objects_v2 per court type (S3), or rglob (local). The returned cloud paths are used only for .name / _build_url (pure path math, no further I/O), so the rest of the tree-building is unchanged. Measured on the real ngm bucket: court types 0.4s; list special (2,509) 1.8s; list supreme (20,555) 10.6s; full supreme build (20,555 cases) 11.2s. The whole court-orders read drops from ~35 min to ~15s (~150x). Together with local staging (write phase), the rebuild no longer pays either the 35-min read or the 40-min per-file write. Adds S3-branch tests with a fake boto3 client (CI has no R2). 165 tests passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What & why
Makes every NGM document a first-class, publicly discoverable record and keeps an up-to-date queryable index of all of them. NGM only adopts the Jawafdehi API DocumentSource format — there is no API coupling here (a future import is a clean 1:1).
Changes
Index format (DocumentSource-compatible)
Manuscriptcarrieslinks: [{link, role}], a stabledocument_id, andsource_type, mirroringcases.models.SourceLinkRole/SourceTypefield-for-field.url/file_namestay as the RAW back-compat alias.press_id(PDF=RAW, other attachments=ALTERNATE, CIAA page=SOURCE_PAGE); court-order leaves collapse percase_number.SEO surface (crawlable, streamed)
build_indexemits a static HTML landing page per document under/d/(canonical, Open Graph, JSON-LDCreativeWork, roled links, transcript placeholder), a sitemap index + per-dataset child sitemaps (split at the 50k-URL limit), androbots.txt.Publish: local staging → R2 mirror
IndexBuildersplits read-root (uploads/) from write-root (output_root).main()builds the derived tree to a local/tmpstaging dir, thenpublish.pybulk-uploads via boto3 with explicit Content-Type (.html→text/html) and syncs deletions so the store is a clean mirror.is_safe_to_delete) to the SEO surface + the currentindices/<date>/.uploads/(source) and older snapshots are never listed or deleted.FILES_STOREbuilds in place (no staging/publish) — tests &serve_test_indexunchanged.Postgres index of all document sources
document_sourcestable (DocumentSourceIndex, created by the existinginit_db/create_all— no Alembic) +db_index.pythat upserts one row per document and mirrors (deletes rows whoselast_seen_buildis stale). The whole archive is now SQL-queryable alongside the court tables. Skipped viaNGM_SKIP_DB_INDEX.Tests
157 passing — format consolidation & one-per-logical-doc, link validity (matches the API's
validate_url_list), SEO output + sitemap pagination, read/write split, publish sync safety incl. a fake-S3 end-to-end (uploads + Content-Type + orphan pruning whileuploads//old snapshots survive), and DB row collection.ruff+blackclean.Follow-ups (out of scope here)
ngm-frontendrepo consumes the index and must adapt to the consolidated manuscript shape (it grouped press releases bypress_idclient-side; court-order leaves no longer have one manuscript per file).docs/ngm/transcripts.md(meta repo) but not run.DocumentSourcerows (needs a smallexternal_reffield) — deferred./tmp(no infra change, by request); anemptyDiris the escape hatch if node ephemeral-disk pressure bites at full scale.docs/ngm/index-structure.md,docs/ngm/transcripts.md) live in the meta repo (GitLab) and are committed separately.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes