Ship CBP enrichment as grammar in the standard enrichment engine

[JSv4]

What

Reworks the CBP CROSS enrichment features from this branch so they 1) run automatically inside the existing enrichment workflows and 2) ship as additional grammar for the existing enrichment system. The standalone CustomsRulingCitationService, the enrich_customs_rulings management command, and their bespoke analyzer/label are deleted — no custom commands or parallel machinery remain. (Note for reviewers: those files were added and removed on this same branch, so the net diff vs main never contains them — you won't see file deletions in the diff, and a repo-wide grep confirms zero remaining references.)

How it integrates

Both features are now ordinary Tier-2a grammar candidates inside GenericCitationExtractor (opencontractserver/enrichment/grammars.py), so they execute wherever EnrichmentService.apply already executes — the corpus_reference_enrichment analyzer task, the ADD_DOCUMENT corpus action installed by setupCorpusIntelligence / ingest_corpus --enrich (i.e. every new document automatically re-enriches the corpus), the runCorpusEnrichment mutation, and the agent tools. Detection tier, confidence, dedup, PDF token projection, CorpusReference persistence, DocumentRelationship rollup, and the provisional/finalize lifecycle are all inherited from the existing engine.

• HTS tariff codes (grammars.py::_hts) → REF_LAW citations keyed htsus:<code>, with the htsus prefix declared in authority_mappings.yaml (classification + AuthorityNamespace seeding, discover() inventory, governance-graph ghost nodes, and future cross-corpus linking for free). Gated on a document-level HTSUS cue — the bare acronym "HTS" deliberately does not open the gate — with anchored (0.9) vs contextual (0.7) per-mention confidence so ordinary corpora's dotted decimals are never mined.
• Title-identifier document citations (CBP ruling numbers, grammars.py::_document_identifier_citations) → REF_DOCUMENT citations resolved against sibling document titles by ReferenceResolver. The grammar activates only on corpora whose titles are predominantly identifier-shaped (constants.DOC_IDENTIFIER_TITLE_GATE_* — ≥2 identifier titles and ≥50% of titles), so serial/order/patent numbers in unrelated corpora are never mined. Titles are canonicalized via constants.document_identifier_from_title (extension-stripped, path-separator-guarded, preserving the earlier A83482.doc resolution fix); a ruling's self-identifying header mention is dropped at resolution (ReferenceResolver.resolve may now return None), keyed on the source document's own title identifier so duplicate-titled siblings are handled; citations to rulings not yet ingested persist UNRESOLVED and are healed to RESOLVED by the writer's forward-only heal on the automatic re-apply when the sibling lands.

Kept from the branch as-is: the EnrichmentWriter UNRESOLVED→RESOLVED heal (+ references_resolved counter), the doc_parse Celery queue routing (ops docs updated to match), compose memory caps (now covering every local.yml service), and the ingest_corpus convertible-extension gate (now with regression tests) — these integrate with the existing ingestion system rather than bypassing it.

Testing

• New opencontractserver/tests/test_customs_trade_grammars.py (40 tests): HTS normalization/gates/confidence tiers (incl. the odd-digit-grouping rejection branch), ruling-citation shapes + crossfeed's documented false-positive guards (bare 6-digit numbers, NY 10022), corpus-shape gate open/closed at thresholds, resolver semantics (extension-carrying titles, path-like titles, self-mention drop incl. duplicate-titled siblings, unknown → UNRESOLVED), and end-to-end apply() including sibling-ingested-later healing, DocumentRelationship rollup, idempotency, and the non-rulings-corpus negative case. Plus ingest_corpus extension-gate regression tests.
• Affected suites all green locally (Postgres 16 + pgvector): test_generic_grammars, test_enrichment_writer, test_enrichment_resolver, test_reconcile, test_enrichment_extractor, test_enrichment_discovery, test_authority_mappings_file, test_enrichment_classification_constants, test_authority_pack_taxonomy, test_corpus_reference_model, test_enrichment_linking, test_enrichment_backfill, test_enrichment_in_flight, test_governance_graph, test_enrichment_analyzer_integration, test_enrichment_tools, test_graph_navigation_tools, test_intelligence_setup, test_enrichment_llm_integration, test_enrichment_run_mutation, test_authority_frontier, test_authority_pack, test_authority_pack_config, test_ingest_corpus_command.
• black / flake8 / isort / mypy clean on all changed files; manage.py check (incl. opencontracts.E001) passes; changelog fragments validate.

Docs / changelog

• docs/architecture/reference-web-enrichment.md: new "Customs / trade grammar family" subsection under the detection tiers.
• docs/upload_methods/worker_celery_setup.md (+ worker_uploads.md, remote_ingest_worker.md, upload_methods/index.md): updated for the doc_parse queue — the previous text told self-hosted operators -Q celery,worker_uploads was sufficient, which under the new routing would leave documents silently stuck mid-chain.
• changelog.d/customs-ruling-citation-enrichment.added.md rewritten for the grammar integration; the two fragments describing the removed command features deleted; the writer-heal fragment trimmed of references to the deleted service.

---

Generated by Claude Code

[claude]

Review

Went through the diff in detail (grammars.py, resolver.py, writer.py, enrichment_service.py, ingest_corpus.py, config/settings/base.py, local.yml, and the new test file). Overall this is a clean, well-documented integration — the design (gate-then-grammar, forward-only heal, Resolution | None for self-mention suppression) is sound and the new tests exercise the interesting edge cases (extension-carrying titles, self-mention drop, corpus-shape gate thresholds, healing on later ingest). A few things worth a look:

1. local.yml mem-limit changelog overclaims coverage (minor, but real gap)

The new fragment local-compose-mem-limits-remaining-services.fixed.md says:

All Docker-managed services now have an individual ceiling.

but privacy_filter and celerybeat in local.yml still have no mem_limit. privacy_filter in particular "bakes the model weights and runs CPU-only" per its own comment block — of all the uncapped services it's the one most plausibly capable of reproducing the exact host-OOM scenario this PR is trying to close off. Worth either capping it too or softening the changelog claim so it doesn't overstate what's covered. (frontend is profile-gated dev tooling so it's lower stakes, but also uncapped.)

Also a minor narrative nit: local-compose-mem-limits.fixed.md describes capping docling-parser/warp-ingest/vector-embedder/celeryworker as "a prior pass," but the diff shows those mem_limit lines being added in this same PR alongside the "remaining services" fragment — there was no actual prior pass in this branch's history. Not functionally important, just slightly confusing changelog framing for anyone tracing it back later.

2. Everything else checked out

• _normalize_hts digit-regrouping logic is correct for the 4/6/8/10-digit cases (traced through by hand against the test cases).
• The _doc_identifier_gate computation in GenericCitationExtractor.__init__ short-circuits safely on an empty idents list (no ZeroDivisionError — matching >= MIN_DOCS fails first via and).
• documents is materialized via list(...) before being handed to both ReferenceResolver and GenericCitationExtractor(documents=documents) in _build_detection — no shared-generator consumption bug.
• resolve_document's new Resolution | None return is handled at its one production call site (_resolve_doc in enrichment_service.py), which filters None before returning; no other caller exists that could crash on the new None.
• The writer's forward-only heal only touches the created=False branch (guarded by an early return on created=True), so newly-created already-resolved rows can't double-count into references_resolved alongside references_created.
• DocumentRelationship rollup (reconcile_document_graph) re-queries CorpusReference from the DB rather than relying on WriteResult.reference_ids, so healed (not newly-created) references still correctly participate in the doc-graph rollup even though _ensure_corpus_reference doesn't append their id to reference_ids.
• ingest_corpus.py's extension gate correctly unions with get_convertible_extensions() (which returns bare, lowercased extensions — matches the f".{ext}" construction and p.suffix.lower() comparison).
• Regexes (DOC_IDENTIFIER_RE, _HTS_TEXT_RE, _HTS_DOC_CUE_RE, _HTS_ANCHOR_RE) are all bounded, no catastrophic-backtracking risk.

Test coverage

The new test_customs_trade_grammars.py is thorough — corpus-shape gate open/closed at both thresholds, false-positive guards ported from crossfeed (bare 6-digit numbers, "NY 10022"), extension-stripping title canonicalization, self-mention suppression, and full apply() round-trips including the heal-on-later-ingest path. Nothing obviously missing.

Nice work — just the local.yml coverage/changelog-accuracy point above is worth a follow-up.

[claude]

Review Summary

This is a clean, well-scoped PR that integrates the CBP CROSS enrichment work as two ordinary Tier-2a grammar families instead of bespoke standalone machinery. The architecture choice (grammar candidates that flow through the existing EnrichmentService.apply pipeline) is the right call — it gets dedup, provisional/finalize lifecycle, DocumentRelationship rollup, and every existing trigger point "for free," and the diff is honest about the fact that it's additive to an existing well-tested system rather than a parallel path.

I traced the Resolution | None change through all callers (resolver.py, enrichment_service.py::_resolve_doc, both the sync _iter_doc_resolutions and the async _aresolve_documents orchestrator, plus scan()/discover() via _resolutions) — every call site funnels through _resolve_doc, which is the single place filtering None, so the self-mention-drop feature can't leak an orphaned None into writer.write(). Good containment of a signature change that could easily have caused a NoneType has no attribute crash somewhere.

Code quality / correctness

• GenericCitationExtractor.__init__ gate math (grammars.py:421-431): matching / len(idents) divides only when matching >= DOC_IDENTIFIER_TITLE_GATE_MIN_DOCS is already True (short-circuited and), so len(idents) == 0 can never reach the division — no latent ZeroDivisionError for an empty/titleless document set. Worth a one-line comment noting the short-circuit is load-bearing, since a future refactor that reorders the conditions would reintroduce a crash.
• _normalize_hts correctly regroups digit runs into 4-2-2-2 form regardless of how the source text was originally dotted (verified against the 10-digit test case) — avoids canonicalization drift between differently-formatted mentions of the same code.
• ext_ok union in ingest_corpus.py (f".{ext}" for ext in get_convertible_extensions()): checked get_enabled_extensions()'s docstring ("no leading dots") and normalize_extension() (lowercases + strips dots) — the prepended-dot + lowercase p.suffix.lower() comparison lines up correctly. Good catch matching the REST upload path's eligibility logic (resolve_convertible_upload).
• Minor/edge case, not a blocker: ReferenceResolver._identifier_index uses setdefault, so if two documents in a corpus happen to canonicalize to the same identifier (duplicate/near-duplicate titles), the second document silently loses its "self" status — a citation to that identifier from the second document would resolve to the first document instead of being dropped as a self-mention. Likely rare in practice for real CROSS corpora (rulings are presumably unique per identifier), but might be worth an explicit test or a code comment acknowledging the limitation, given the self-mention-drop logic is otherwise very careful about not persisting header/footer noise.
• Confidence tiering (_CONF_HTS_ANCHORED / _CONF_HTS_CONTEXTUAL) and the document-level gates (_HTS_DOC_CUE_RE excluding bare "HTS", the title-shape gate with MIN_DOCS/FRACTION thresholds) are thoughtfully justified in comments with concrete false-positive scenarios (dollar amounts, ZIP codes, "high-throughput screening"). This is the kind of documented precision/recall tradeoff that's easy to get wrong silently — good to see it spelled out and tested (test_bare_six_digit_legacy_number_not_mined, test_state_plus_zip_not_mined, test_bare_hts_acronym_does_not_open_the_document_gate).

Writer heal logic (writer.py::_ensure_corpus_reference)

The new UNRESOLVED→RESOLVED heal block is forward-only (never downgrades an already-RESOLVED row) and mirrors the existing jurisdiction/authority_type heal pattern in the same method, which keeps the method internally consistent. Field selection in the heal (target_annotation_id, target_document_id, resolution_status, confidence, normalized_data) looks complete for the resolved-vs-unresolved transition. One thing worth being aware of (may already be covered by the "affected suites green" note in the PR description): the heal runs before the "claim rule" block for provisional rows further down in the same method — since both mutate disjoint field sets and both append to update_fields, this ordering looks safe today, but is worth double-checking if either block grows to touch overlapping fields later.

Test coverage

36 new tests in test_customs_trade_grammars.py are thorough — unit coverage for normalization, both confidence tiers, all three gate thresholds (open/closed at min-docs, open/closed at fraction), the documented false-positive guards ported from crossfeed, and full apply() integration tests including the sibling-ingested-later healing scenario and idempotency. The test_enrichment_resolver.py/test_enrichment_writer.py updates correctly retrofit assert r is not None guards for the new Optional return type. Didn't find a gap here beyond the duplicate-identifier-title edge case noted above.

Infra changes (queue routing, compose mem limits)

Both are well-justified in the changelog fragments with concrete observed numbers (190:1 completion ratio, 5.7GB unbounded growth). The doc_parse queue routing change requires the production Celery worker to actually be subscribed to the new queue — confirmed compose/production/django/celery/worker/start was updated in this same diff (-Q celery,worker_uploads,doc_parse), so there's no risk of tasks silently queuing to a consumer-less queue in prod. Local compose mem limits are all additive and scoped to dev tooling — low risk.

Security

No concerns — this is pure regex/text processing and in-memory dict lookups against already permission-scoped document lists (the resolver/extractor never touch the DB directly, consistent with the "no DB access" contract stated in the grammars.py module docstring). No user-controlled input reaches string formatting in a way that could inject anything downstream.

Nit

_document_identifier_citations constructs Candidate directly instead of via the _cand() helper used by every other grammar function in the file — reasonable since _cand() is REF_LAW-specific (sets canonical_key, jurisdiction, authority_type unconditionally), but it does mean this one candidate's normalized_data carries a "tier" key redundant with the detection_tier field already set on Candidate. Harmless (matches the existing _cand() convention of duplicating tier into normalized_data), just flagging for consistency awareness.

Overall: solid, well-tested, appropriately scoped. No blocking issues found.

[claude]

Review: Ship CBP enrichment as grammar in the standard enrichment engine

Overview

This PR retires the standalone CBP CROSS customs enrichment approach in favor of expressing it as two ordinary Tier-2a grammar candidates (_hts, _document_identifier_citations) inside GenericCitationExtractor, so the feature inherits all the existing detection/resolution/writer/heal machinery instead of running through bespoke parallel code. It's bundled with a few unrelated-but-explained infra fixes (celery doc_parse queue routing, local.yml memory caps, ingest_corpus extension gate). Test coverage is excellent (36 new tests, plus updates to existing resolver/writer tests for the new Resolution | None contract).

Strengths

• Single construction point: GenericCitationExtractor(documents=...) and ReferenceResolver(documents) are both built once in _build_detection, which is shared by discover/scan (sync) and apply's async LLM path (_aresolve_documents via sync_to_async). The corpus-shape gate is therefore evaluated consistently everywhere; no divergent code paths.
• resolve_document -> Resolution | None contract is threaded correctly: the only production call site (enrichment_service.py:224, inside _resolve_doc) already filters None out of the returned list, and existing tests were updated with assert r is not None guards. No other caller was missed.
• Forward-only heal logic in EnrichmentWriter._ensure_corpus_reference is scoped correctly — it only heals UNRESOLVED -> RESOLVED, never downgrades a RESOLVED row, and is a generic mechanism (not special-cased to the new grammar) that will also benefit future REF_SECTION/REF_DOCUMENT resolvers.
• Gate short-circuit ordering is correct and well commented: matching >= MIN_DOCS and matching / len(idents) >= FRACTION — since MIN_DOCS > 0, the left side guarantees idents is non-empty before the division runs, deliberately avoiding a ZeroDivisionError on a titleless document set. The comment calling out that the order is load-bearing is appreciated.
• Self-mention drop compares against the source document's own canonical identifier (not target == source), which correctly handles the duplicate-titled-sibling case (same ruling ingested twice) — verified by test_duplicate_title_self_mentions_both_dropped.
• Confidence tiering (_CONF_HTS_ANCHORED vs _CONF_HTS_CONTEXTUAL) and the document-level HTSUS cue gate are sensible, well-tested precautions against mining ordinary dotted decimals/dollar amounts as tariff codes.
• No N+1 concerns — the identifier/exhibit indexes are built once per resolver construction (O(documents)), not per-candidate.

Issues / things worth double-checking

1. PR description vs. diff mismatch: The description states the standalone CustomsRulingCitationService, enrich_customs_rulings management command, and their bespoke analyzer/label "are deleted." The actual diff (gh pr diff 2154) contains no deleted files — deletions: 14 total, all of which are the four ext_ok lines rewritten in ingest_corpus.py. Neither CustomsRulingCitationService nor enrich_customs_rulings appears anywhere in the current tree. This is likely just an artifact of the description narrating cleanup that happened on a different, never-merged branch — but it's worth confirming explicitly in the PR thread that there's no dead code left behind elsewhere (e.g. an orphaned migration, GraphQL type, or URL route referencing the old service) rather than relying on the description's claim.

2. Scope bundling: The doc_parse Celery queue routing, the full local.yml memory-limit sweep across ~10 services, and the ingest_corpus extension-gate fix are all independent, well-justified fixes but have nothing to do with shipping the customs grammar. They're individually low-risk and each has its own changelog fragment, but bundling them makes this diff harder to review/revert atomically (e.g. reverting the HTS grammar would also revert memory caps and queue routing). Understandable if this branch is just consolidating prior work, but worth flagging.

3. document_identifier_from_title only strips one extension level: Path(title).stem on "A83482.v2.doc" yields "A83482.V2", which won't fullmatch DOC_IDENTIFIER_RE and will silently fail to canonicalize/index. Probably a non-issue for real CBP filenames, but since the whole point of this helper is defending against materialized-filename drift, a titled document with a version suffix would quietly fall through the gate/index rather than erroring — might be worth a comment noting the single-strip limitation is an accepted tradeoff (or confirming it never happens in practice).

4. DOC_IDENTIFIER_RE is case-sensitive (requires uppercase): reasonable for canonical CBP ruling formatting, and document_identifier_from_title uppercases titles before indexing, but the text-mining regex (DOC_IDENTIFIER_RE.finditer in _document_identifier_citations) does not uppercase the source text first — a lowercase or mixed-case in-body mention (e.g. OCR'd or re-typed text) would never match. Likely fine given the grammar is ported from crossfeed's golden tests against real CROSS text, just flagging as a known blind spot rather than a defect.

5. Self-mention drop is document-wide, not header-scoped: every occurrence of a document's own identifier anywhere in its text is dropped, not just the header/footer restatement the docstring motivates. This is presumably deliberate (documented and tested via test_self_mentions_never_persisted), but worth confirming it's the intended behavior rather than an approximation — a ruling that substantively re-cites itself deep in the body (rare, but possible in amended/superseding rulings) would silently lose that citation too.

Test coverage

Very thorough — unit tests for _normalize_hts, the HTS document/mention-level gates, the ruling-citation grammar's false-positive guards (bare 6-digit numbers, "NY 10022"), the corpus-shape gate at/around its thresholds, resolver semantics (extension-carrying titles, self-mention drop including the duplicate-title edge case), and end-to-end apply() coverage including the sibling-ingested-later healing path and idempotency. The references_resolved counter is tested both for the heal case and for a subsequent no-op run staying at zero. Nothing obviously missing.

Security

No user-controlled input reaches SQL or shell; regexes are bounded (no ReDoS risk); no new permission-relevant code paths — resolution and writing continue to go through the existing EnrichmentService/EnrichmentWriter machinery. Looks fine from a security standpoint.

[JSv4]

Re: the description-vs-diff mismatch (review point 1) — the diff showing zero deleted files is expected, not a sign of leftover code. CustomsRulingCitationService, the enrich_customs_rulings command, and their test file were added and then deleted on this same (never-merged) branch, so the net diff against main simply never contains them — main never had those files to delete.

Verified there's nothing orphaned:

• Repo-wide grep for customs_ruling / CustomsRuling / enrich_customs / HTS_CODE / the old analyzer id across opencontractserver/, config/, and frontend/src/ returns zero hits outside the new test_customs_trade_grammars.py.
• The branch touches zero migrations, and the old service's Analyzer row was only ever created at runtime by the service itself (never seeded), so no deployment can hold an orphaned row from it.
• No GraphQL type, URL route, or tool registration ever referenced the old service.

Points 3–5 (single-extension strip, uppercase-only matching, document-wide self-mention drop) are all deliberate tradeoffs — now documented as such in code comments as of 3a05fbe. Point 2 (scope bundling): correct read — the queue routing, mem caps, and extension-gate fixes are prior work consolidated on this branch; each carries its own changelog fragment so they stay individually traceable.

---

Generated by Claude Code

[claude]

Review: Ship CBP enrichment as grammar in the standard enrichment engine

Overview

Solid integration work: the two customs/trade grammars (HTS tariff codes, CBP ruling-number document citations) are wired into the existing Tier-2a GenericCitationExtractor rather than living as a bespoke service, and the corpus-shape gating (DOC_IDENTIFIER_TITLE_GATE_*), self-mention drop, and UNRESOLVED→RESOLVED heal are all well-reasoned with good inline rationale. The resolve()/resolve_document() signature change to Resolution | None was propagated correctly — every production call site (enrichment_service.py:224) already filters None, and the two existing resolver tests were updated with the new assert r is not None guard. Test coverage for the new grammars themselves (36 new tests) is thorough, including the documented crossfeed false-positive guards (bare 6-digit numbers, "NY 10022") and the gate threshold edges.

Bug: docs/upload_methods/worker_celery_setup.md is now materially wrong (not updated in this PR)

config/settings/base.py routes extract_thumbnail, ingest_doc, and set_doc_lock_state to a new doc_parse queue. compose/local and compose/production worker start scripts were updated to consume it — but docs/upload_methods/worker_celery_setup.md was not touched, and it now actively contradicts the new routing:

• Its section header literally says "The two queues (there are only two)" and states CELERY_TASK_ROUTES routes only worker_uploads.tasks.* to a dedicated queue, "everything else uses the default celery queue."
• Its table explicitly names extract_thumbnail as an example task carried by the celery queue.
• Its "TL;DR — minimum viable config" tells operators of custom/self-hosted deployments to run -Q celery,worker_uploads as sufficient.

Per this PR's own change, extract_thumbnail/ingest_doc/set_doc_lock_state (confirmed via opencontractserver/documents/signals.py — these are the general per-document ingest chain, not worker-upload-specific) no longer route to celery. A self-hosted deployment following this doc's exact recommended command would now have documents get stuck mid-chain (thumbnail/ingest/lock-release never run) with no worker consuming doc_parse — silently, since nothing in this doc mentions the new queue. This is exactly the "silent failure" class the doc warns about elsewhere. CLAUDE.md's own doc-maintenance rule ("update the relevant doc … as part of the same change") applies directly here. Please update this page's queue table, TL;DR command, and the "two queues" framing to include doc_parse.

Minor / worth a look

• opencontractserver/corpuses/management/commands/ingest_corpus.py: the new get_convertible_extensions() union looks correct (verified the extensions are lowercase, bare (no dot), matching p.suffix.lower()), but I didn't see a dedicated test exercising "a .doc file is now picked up when a converter is configured" / "still dropped when none is configured" for this specific command — the PR's own test list doesn't call one out either. Given this is described as a real production bug fix (silently dropped .doc files), a regression test here would guard against re-breaking it.
• local.yml: flower inherits mem_limit: 8g via the <<: *django anchor merge. Consistent with the changelog fragment's claim, but 8g for a lightweight Flower monitoring UI is generous — not a bug, just worth confirming it's intentional rather than an artifact of reusing the anchor.
• HTS grammar heuristic (grammars.py::_hts): once a document trips the document-level HTSUS/Harmonized Tariff Schedule cue, every \d{4}\.\d{2}(...)-shaped token in that document becomes a CONTEXTUAL-confidence candidate — including dollar amounts formatted 1234.56 that happen to share the shape. Known, tested, documented tradeoff (not a bug) — just flagging as something to watch on real CROSS corpora with financial figures.

Nice-to-haves already done well

• The _doc_identifier_gate short-circuit ordering (matching >= MIN_DOCS and matching/len(idents) >= FRACTION) correctly avoids a ZeroDivisionError, and the comment calling out why the order is load-bearing is exactly the kind of non-obvious invariant worth documenting.
• The self-mention drop in _resolve_document_identifier keys off the source document's own identifier (not target == source), correctly handling duplicate-titled siblings — covered by test_duplicate_title_self_mentions_both_dropped.
• The references_resolved heal counter is forward-only (never downgrades a RESOLVED row), mirroring the existing jurisdiction/authority_type heal pattern, and has both a writer-level and a full apply()-integration test for the heal-on-later-sibling-ingest scenario.

Security / permissions

No concerns — GenericCitationExtractor and ReferenceResolver both consume the same permission-scoped documents list the caller (EnrichmentService) already resolved via the service layer; no new DB queries or Tier-0 permission logic introduced.

Overall: well-tested, well-documented change to the enrichment engine. The one item worth addressing before merge is the stale worker_celery_setup.md, since it now gives operators an incomplete/actively misleading queue configuration for a scenario (self-hosted/custom deployment) this repo explicitly documents support for.

[codecov]

Codecov Report

❌ Patch coverage is 97.54098% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
opencontractserver/pipeline/base/file_converter.py	70.00%	3 Missing ⚠️
.../src/components/widgets/modals/BulkImportModal.tsx	94.73%	2 Missing ⚠️
...ponents/widgets/modals/UploadModal/UploadModal.tsx	97.61%	1 Missing ⚠️
opencontractserver/corpuses/models.py	97.56%	1 Missing ⚠️
opencontractserver/enrichment/grammars.py	97.61%	1 Missing ⚠️
opencontractserver/enrichment/resolver.py	97.91%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Review: Ship CBP enrichment as grammar in the standard enrichment engine

Reviewed the diff (grammar/resolver/writer changes, ingest_corpus extension gate, Celery queue split, and local.yml mem caps). Overall this is a well-architected integration -- folding the bespoke CROSS service into the existing Tier-2a grammar tier is the right call, and the self-mention / heal / gating logic is unusually well commented for the tradeoffs involved. A few notes:

Correctness -- looks solid

• ReferenceResolver.resolve() / resolve_document() now returning Resolution | None is threaded through correctly: the only production call site (EnrichmentService._resolve_doc, enrichment_service.py:216-224) filters None immediately, and that method is the sole entry point used by both the sync (_iter_doc_resolutions) and async (_aresolve_documents) apply paths, plus scan/discover via _resolutions. No caller drops the filter.
• The self-mention drop keys off the source document's own canonical identifier (self._doc_identifiers.get(source_doc_id) == ident) rather than "target == source", which correctly handles the duplicate-title case (same ruling ingested as .doc and .pdf) -- confirmed by test_duplicate_title_self_mentions_both_dropped.
• The DOC_IDENTIFIER_TITLE_GATE divide-by-zero guard is correct: matching >= MIN_DOCS (MIN_DOCS=2) always short-circuits before matching / len(idents) can run on an empty list, since matching <= len(idents).
• The writer's forward-only heal (writer.py:559-593) mirrors the existing jurisdiction/authority_type heal pattern, never downgrades an already-RESOLVED row, and the references_resolved counter is properly plumbed through WriteResult -> _accumulate -> apply()'s result dict.
• _normalize_hts digit-count validation (4/6/8/10) and the corresponding grouping logic check out against all the unit test cases (6-digit, 8-digit, 10-digit HTS/HTSUS codes).

Minor nits (non-blocking)

1. ingest_corpus.py:456 -- from opencontractserver.pipeline.utils import get_convertible_extensions is a local import inside handle(). Nothing else in the file needs a deferred/circular-import workaround, so this could likely be a top-level import for consistency, unless there's a reason (e.g. avoiding pulling in pipeline machinery for --help/argument parsing) that isn't stated in a comment.
2. authority_mappings.yaml -- every other aliases: entry in the file uses single-line flow style (aliases: ["a", "b"]), but the new htsus entry wraps into a multi-line flow list with a trailing comma. Valid YAML, but it's the only entry formatted this way in the file -- worth a quick check that this wasn't just prettier auto-wrapping on line length (in which case it's fine and not worth changing).
3. _HTS_ANCHOR_RE includes a bare [Hh]eading\b as an anchor token -- this is fairly generic and could anchor confidence for unrelated "Heading 2" style document-structure mentions within the 48-char window in cue-bearing documents. Given the document-level gate already requires an explicit HTSUS cue, this is a minor precision tradeoff rather than a bug, and it's already tested at the contextual/anchored boundary.

Test coverage

Strong -- 36 new tests cover the regex shapes, both gates (including exact-threshold and below-threshold cases), resolver self-mention/duplicate-title semantics, and full apply() integration (idempotency, unresolved->resolved healing on sibling arrival, DocumentRelationship rollup, discover() inventory surfacing). The resolver test file update (assert r is not None narrowing) is a clean, minimal adaptation to the new Optional return type.

Infra changes (queue split, local.yml mem caps)

Out of the core review scope but scanned for correctness: the doc_parse queue routing is applied consistently across config/settings/base.py, both compose worker start scripts, and all four affected ops docs (queue tables, TL;DR commands, verification/symptom sections). The local.yml mem_limit additions are correctly scoped per-service (confirmed the django/flower shared-anchor placement is correct) and each carries a rationale comment tied to the observed OOM incident.

No blocking issues found -- nice work on the self-mention/heal edge cases in particular, those are easy to get subtly wrong.

[claude]

Review Summary

This PR reworks CBP CROSS enrichment (HTS tariff codes + ruling-number document citations) into the standard Tier-2a grammar tier, plus a handful of unrelated-but-bundled ops fixes (Celery doc_parse queue split, local.yml memory caps, ingest_corpus convertible-extension gate, and a forward-only UNRESOLVED->RESOLVED heal in EnrichmentWriter). I traced the core logic end-to-end (grammar -> resolver -> writer -> service) and it holds up well -- this is a solid, carefully-documented PR. Notes below are minor/non-blocking.

Correctness -- looks solid

• GenericCitationExtractor.__init__ gate math (grammars.py): the matching >= MIN_DOCS and matching / len(idents) >= FRACTION short-circuit is correct -- MIN_DOCS=2 > 0 guarantees idents non-empty whenever the left side is true, so the documented "no ZeroDivisionError" claim checks out.
• _normalize_hts: verified the 4/6/8/10-digit regrouping against all four test cases by hand; the "reject 9-digit tokens the text regex can still match" case (test_odd_digit_grouping_rejected_by_normalization) is correctly handled.
• resolve_document -> Optional[Resolution]: checked every production call site (ReferenceResolver.resolve internally, and the single external caller in enrichment_service.py::_resolve_doc) -- the None (self-mention drop) is filtered correctly and there's no other caller in the codebase that would choke on the new None return.
• Single instantiation point for GenericCitationExtractor(documents=...) (enrichment_service.py:149) -- confirmed it's the only call site in production code, so the doc-identifier grammar is never silently left off in some other path.
• Writer heal logic (writer.py::_ensure_corpus_reference): forward-only (never downgrades RESOLVED->UNRESOLVED), correctly scoped to res.resolution_status == STATUS_RESOLVED (so it doesn't fire for EXTERNAL/law rows), and update_fields is properly threaded into ref.save(...).
• Celery routing: extract_thumbnail/ingest_doc/set_doc_lock_state task names in the new CELERY_TASK_ROUTES entries match the actual function names in doc_tasks.py exactly; convert_document_to_pdf is correctly left off the new queue per the stated design.
• ingest_corpus.py extension gate: get_convertible_extensions() returns dot-less, normalized extensions per its docstring, so the f".{ext}" construction is correct and matches the existing .suffix.lower() comparison.

Minor observations (non-blocking)

1. document_identifier_from_title uses Path(...).stem, which treats / as a path separator (constants.py). If a document title ever contains a / (e.g., a user-entered title like "Reports/N301234" rather than a filename-derived one), Path("Reports/N301234").stem returns "N301234" -- silently discarding everything before the slash instead of treating the title as a non-identifier string. In practice titles come from materialized filenames so this is unlikely, but since titles can also be user-edited (or come from non-file ingest paths), a title.rsplit("/", 1)[-1] guard or explicit "no path separators" check might be worth considering if you want to fully close this off rather than rely on filenames never containing /.
2. DOC_IDENTIFIER_RE / _HTS_TEXT_RE use \d without re.ASCII, so Python's default Unicode-aware \d would technically match non-ASCII decimal digits (e.g., Arabic-Indic digits). Extremely unlikely to matter for CBP ruling text, but if these grammars are ever pointed at less English-only corpora it's a one-line hardening (\d -> [0-9] or add re.ASCII).
3. Bundling scope: the PR bundles four fairly unrelated concerns (grammar feature, Celery queue split, local.yml memory limits, ingest_corpus extension gate) into one diff. Each is well-tested and well-changelogged individually, so this isn't a correctness issue, but it does make the PR harder to git revert atomically if one part needs to be pulled back later (e.g., if the doc_parse queue split needs a quick rollback, it's entangled with the grammar changes in the same commit).
4. PR description mismatch: the description says the standalone CustomsRulingCitationService / enrich_customs_rulings management command / bespoke analyzer are "deleted," but the diff contains no deletions of such files -- presumably because they only ever existed on the unmerged source branch being reworked, not on main. Worth double-checking the description doesn't cause confusion for reviewers expecting to see a deletion in this diff.

Test coverage

Coverage is thorough: test_customs_trade_grammars.py covers HTS normalization, both confidence tiers, the document-cue gate (including the deliberate "bare HTS" non-cue and false-positive guards for 6-digit numbers / ZIP-like tokens), the title-identifier gate at and around its thresholds, extension-stripping title canonicalization, self-mention dropping (including the duplicate-title edge case), and full EnrichmentService.apply() round-trips (idempotency, discover() surfacing, DocumentRelationship rollup, and the sibling-ingested-later healing path with an assertion on the new references_resolved counter). The writer.py heal also gets a dedicated regression test asserting the counter doesn't double-count on a no-op re-apply. No gaps found.

Security

No user-controlled input reaches raw SQL, template rendering, or shell execution in this diff. Document titles and text flow only into regex matching and ORM lookups (get_or_create, .get()), which are parameterized. Permission scoping is preserved -- documents fed to both ReferenceResolver and GenericCitationExtractor come from CorpusDocumentService.get_corpus_documents_visible_to_user, consistent with the CLAUDE.md-documented MIN(corpus, document) requirement for this service (issue #1682); the grammar change doesn't introduce a new document-loading path.

Overall: nice, well-documented change with strong test coverage. Nothing here blocks merge.

[claude]

Reviewed the diff (grammar/resolver/writer core changes, local.yml mem caps, doc_parse queue routing, ingest_corpus extension gate, and the new test_customs_trade_grammars.py). Overall this is a clean, well-scoped integration — genuinely no leftover references to the deleted CustomsRulingCitationService/enrich_customs_rulings command (verified with a repo-wide grep), and the new grammar family plugs into the existing Tier-2a pipeline without any parallel machinery.

Strengths

• _normalize_hts and the HTS regex/gate logic check out against the test cases (traced the 4/6/8/10-digit grouping math and the 9-digit rejection case by hand — both correct).
• The document_identifier_from_title extension-stripping + path-separator guard is a nice, well-justified fix for the "materialized filename as title" problem, and is unit-tested directly (DocumentIdentifierTitleTests).
• resolve_document/resolve() changing to Optional[Resolution] for the self-mention drop is threaded through carefully — the only other caller is resolve() itself, and the two pre-existing call sites in test_enrichment_resolver.py got the assert r is not None follow-up. Duplicate-titled sibling self-mentions are handled and tested (test_duplicate_title_self_mentions_both_dropped).
• The _ensure_corpus_reference forward-only heal (UNRESOLVED → RESOLVED) mirrors the existing jurisdiction/authority_type heal pattern, is documented, and has a good regression test (test_apply_heals_unresolved_document_reference_on_rerun) that also checks the counter doesn't over-report on a no-op second run.
• DOC_IDENTIFIER_TITLE_GATE_MIN_DOCS/FRACTION gate ordering comment about avoiding ZeroDivisionError is correct — verified matching >= MIN_DOCS (MIN_DOCS=2) does guarantee idents non-empty before the division.
• Test suite is extensive and covers the tricky edges (bare 6-digit false positives, NY 10022 ZIP collision, corpus-shape gate thresholds, idempotency, discover() inventory surfacing, sibling-ingested-later healing end-to-end).

Minor observations / nits (none blocking)

1. opencontractserver/enrichment/grammars.py::_document_identifier_citations — the gate check (self._doc_identifier_gate) lives in extract(), not in this generator itself, and the docstring says "Callers must apply the corpus-shape gate." That's fine since the only caller is extract(), but if this ever gets called directly/reused (e.g. from a test or another extractor) it would silently skip the gate. Not a real risk today given it's private (_-prefixed), just flagging the implicit contract.
2. local.yml's new mem_limits are sized from empirical observations described in the changelog fragments/comments (5.7GB peak → 6g cap, etc.) — reasonable, but since this is local dev tooling only, worth double-checking these caps don't cause spurious OOM-kills on CI machines with less headroom if any of these services run in CI. (If CI doesn't use local.yml, disregard.)
3. _HTS_ANCHOR_WINDOW = 48 and the anchor regex re-scan a fixed-size text slice per HTS match — negligible cost, but for pathological documents with very many dotted-decimal-shaped tokens after the doc-level cue matches, this is O(matches) regex searches over small windows; not a real concern at expected document sizes, just noting for the record since there's no cap on match count.
4. The self-mention drop in _resolve_document_identifier is intentionally document-wide rather than header-scoped (well documented in the docstring) — this trades away legitimate body self-citations (rare) for simplicity, which seems like the right call, just confirming I read the tradeoff as intended rather than as a bug.

Security/permissions: No Tier-0 permission-layer concerns — the resolver/extractor only consume an already permission-scoped documents list passed in by EnrichmentService, consistent with the codebase's service-layer pattern. No new user input parsing paths that weren't already going through the existing candidate → resolution → writer pipeline.

Test coverage: Strong — 40 new tests plus updates to 3 existing suites, covering unit-level grammar/regex correctness, resolver semantics, and full apply() integration (including the heal-on-reapply path and the negative "ordinary corpus" case). Didn't spot any gaps worth calling out.

No blocking issues found.

[JSv4]

Ported the PR 2153 infra fixes into this branch (commit b8d43af) and re-ran the 10K official-export smoke test against the same benchmark corpus that PR 2153's runs enriched. Output equivalence, measured against 2153's persisted results:

Metric	PR 2153 (standalone service)	This branch (grammar engine)
Documents scanned / skipped	10,000 / 0	10,000 / 0
Unique cited (doc, identifier) pairs	9,377	9,377 (identical set — 0 novel, 0 missing)
Resolved (source, target) pair set	5,007	5,007 (delta 0 added / 0 removed)
Graph edges	11,644	11,644 (0 created / 0 pruned)
Mentions	9,377 (one per doc+identifier)	16,381 (one per occurrence — this engine's convention; every surplus mention is a repeat occurrence of an already-detected pair)

The port: durable path/external_id-derived document identity shared by the resolver index, self-mention drop, and grammar gate (official-export titles are subjects, so the previous title-only gate never opened on the real export); series-token legacy citation grammar ("HRL 087392" — 9,377 of the 9,677 candidates on this corpus); meta.csv external_id contract + upversion lineage inheritance; relationships.csv import idempotency; writer span-fallback page=0/text canonical shape with in-place healing. One pre-existing expectation updated: duplicate document identities now resolve to UNRESOLVED + a warning instead of first-writer-wins (documented in IdentifierResolverTests).

536 tests green across the affected suites; manage.py check and pre-commit pass. Wall time for the full-corpus run: 96 minutes (sequential engine).

[claude]

Review: Ship CBP enrichment as grammar in the standard enrichment engine

Reviewed the full diff (62 files, +4083/-256) across four areas in parallel: the new grammar/enrichment core, ingestion/infra carry-overs, frontend upload/intelligence UI, and docs/changelog. Verified the notable findings directly against the checked-out merge commit rather than relying solely on the diff. Overall this is a well-structured integration, with solid targeted test coverage on the core grammar logic, resolver, and writer changes, and the celery queue/compose/idempotency carry-overs check out. Two real (verified) bugs and a few doc/test-gap issues are noted below.

Bugs (verified against source)

1. Closing the Bulk Import modal mid-job silently drops the completion refresh. frontend/src/components/widgets/modals/BulkImportModal.tsx:497-498 (if (!visible) return null) unmounts the whole component on close, which tears down the GET_BULK_DOCUMENT_UPLOAD_STATUS poll and the jobStatus?.completed effect (lines 109-135) that calls evictCorpusDocumentCaches plus the success toast. The "Close" button is shown once !loading (line 556), which happens well before the background job actually completes. The UI copy at line 456-457 explicitly says "Documents are being processed in the background; you can close this dialog safely" - but closing early means the corpus view never gets the cache eviction/refresh, contradicting that promise. The user has to manually refresh to see the imported documents.

2. Same pattern in UploadModal. frontend/src/components/widgets/modals/UploadModal/UploadModal.tsx:275-280: the close effect does setZipJobId(null) on !open, which trips skip: !zipJobId on the status query and permanently short-circuits the onUploadComplete/refetch effect (lines 160-181) for any zip-import job still running when the user closes the dialog.

   Both spots reset job-tracking state as part of "clean up on close," but the intent seems to have been to reset stale state on the next open, not to abandon a live background jobs completion callback. Worth deciding: keep polling in a location that survives modal close (e.g. lift job tracking to a longer-lived provider/atom), or gate the "safe to close" messaging/eviction differently.

Docs

3. Fabricated code pointer. docs/upload_methods/bulk_zip_import.md:108 cites opencontractserver/enrichment/services/customs_ruling_citation_service.py::_build_ruling_identity_index - this file does not exist in the repo (confirmed: opencontractserver/enrichment/services/ has no such module, and it is not created anywhere in this PRs diff, consistent with the PR description noting that service was added-then-deleted on this branch). The real logic now lives in opencontractserver/enrichment/resolver.py::document_identity_candidates. CLAUDE.md calls out exactly this risk ("a pointer stays live... a wrong pointer is worse") - please fix the pointer.

4. reference-web-enrichment.mds new "Customs / trade grammar family" section undersells the resolution sources. It describes document-identifier resolution as matching only against "sibling document titles," but resolver.py::document_identity_candidates (per this same PR) also layers in DocumentPath.external_id (cross: namespace) and corpus-path basename stems as higher-priority identity sources, plus a legacy series-token grammar (LEGACY_DOC_IDENTIFIER_CITE_RE) - none of which are mentioned in the doc.

5. Minor undercounting: changelog.d/doc-parse-queue-starvation.fixed.md and docs/upload_methods/worker_celery_setup.md list only 3 tasks routed to doc_parse (extract_thumbnail, ingest_doc, set_doc_lock_state), but config/settings/base.pys CELERY_TASK_ROUTES (added in this PR) also routes remap_pending_annotations there. Operator guidance (-Q celery,worker_uploads,doc_parse) is unaffected, but worth correcting the description for accuracy.

Test coverage gaps

6. The document-identifier corpus-shape gates boundary tests do not exercise the production code path. opencontractserver/enrichment/grammars.py:438-460: GenericCitationExtractor.__init__ computes the "predominantly identifier-shaped" gate differently depending on whether identity_candidates is passed. Production (EnrichmentService._build_detection) always passes it, so considered = len(documents) (every document, titled or not). But test_customs_trade_grammars.pys gate boundary tests (test_gate_open_at_thresholds, test_gate_closed_below_fraction, test_gate_closed_below_min_docs) construct the extractor without identity_candidates, exercising only the fallback branch where considered = len(idents) (title-only documents). For corpora containing untitled documents, the two denominators diverge, and none of the existing tests cover the branch that production actually uses. Worth adding a boundary test that passes identity_candidates explicitly.

7. No exact-50% boundary test with considered > 4. All threshold tests hold matching == MIN_DOCS == 2, so the >= vs > comparison in the fraction check is never exercised away from the minimum-docs edge.

8. test_doc_tasks.pys DocumentPipelineRoutingTestCase (around line 47) only asserts CELERY_TASK_ROUTES for remap_pending_annotations, not for extract_thumbnail, ingest_doc, or set_doc_lock_state - all routed to doc_parse in this same PR. A future accidental removal of the ingest_doc route (the actual parsing step) would not be caught by this test.

What checked out well

• Grammar core (grammars.py, constants.py, resolver.py, writer.py): HTS confidence values (0.9 anchored / 0.7 contextual), the HTSUS document-cue gate (bare "HTS" correctly excluded), the odd-digit-grouping rejection, and the DOC_IDENTIFIER_TITLE_GATE_MIN_DOCS=2/FRACTION=0.5 constants all match the PR description and are backed by solid targeted tests. ReferenceResolver.resolve returning None is handled safely everywhere it is called, with no crashes or silent-swallow risk found. Regexes are all linear/bounded (no ReDoS risk). The writers UNRESOLVED-to-RESOLVED heal is correctly forward-only (never downgrades a RESOLVED row).
• Celery/infra carry-overs: doc_parse queue routing is consistent between both worker start scripts and settings; every call site of the re-routed tasks uses .si() with no queue override that could bypass routing. Compose memory limits cover every local.yml service (including flower via YAML anchor) with no gaps or duplication. ingest_corpuss extension gate, relationship-import idempotency (get_or_create on natural edge identity), and upversion lineage inheritance are correctly implemented with targeted regression tests.
• config/graphql/document_queries.py change is a clean DRY refactor with no services-layer or permission-logic changes, so no opencontracts.E001 risk.
• Changelog fragments: all 9 follow the <slug>.<type>.md convention correctly with valid categories and substantive content.
• Frontend test hygiene: the new .ct.tsx/.test.tsx files correctly follow the test-wrapper pattern and the Playwright CT split-import rule (JSX-component imports kept separate from helper imports).

Scope note

The BulkImportModal/UploadModal job-polling rework and the IntelligencePanel navigation fix appear unrelated to this PRs stated purpose (customs/trade grammar integration) and are not referenced by any of the 9 changelog fragments. Given they contain the two verified bugs above, it may be worth splitting them into a separate PR so they get focused review and their own changelog entry, rather than riding in on an otherwise-unrelated backend feature.

[JSv4]

Disposition of the latest review, as of 6343fc8:

Fixed (items 3–8): the dead customs_ruling_citation_service pointer in bulk_zip_import.md now points at resolver.py::document_identity_candidates; the reference-web doc's customs section now documents the layered identity derivation (cross: external_id → path basename stem → title stem), the legacy series-token grammar, and duplicate-identity UNRESOLVED semantics; the queue fragment + worker_celery_setup.md now list all four doc_parse tasks including remap_pending_annotations; gate boundary tests now exercise the production identity_candidates branch (denominator = all documents, including identity-less ones) plus exact-50% boundaries on both branches; and the routing test pins all four doc_parse routes plus convert_document_to_pdf staying on the default queue.

Left for @JSv4 (items 1–2 + scope note): the BulkImportModal/UploadModal close-mid-job findings are real per the review's trace, but they're in the code ported from PR 2153 and any fix is an architecture call (lifting job polling out of the modals into a longer-lived provider/atom, vs. gating the "safe to close" messaging/eviction) — deferring to you rather than redesigning it on your branch. Same for whether to split the modal/IntelligencePanel work into its own PR with a changelog fragment.

---

Generated by Claude Code