SE audit: decompose orchestrator, fix defects, 5.4x hot-path perf (byte-identical)#5
Merged
Conversation
The two safe-negative expiry tests backdated timestamps via max(time.time()-8d, _month_boundary+1), which self-defeats during the first week of a month (today): the clamp raised the timestamp too recent for a Monday to have elapsed. Freeze the cache wall clock to a fixed mid-month instant so both tests are date-independent. Pre-existing failure, unrelated to the refactor; required for a green Step-0 oracle.
Zero importers (verified repo-wide at delete time); the 'scholarly' package is not a dependency so the module's top-level import would fail if ever loaded. tests/test_scholarly_scholar.py exercises the live scholar/serpapi/serply clients and is retained. Migration Step 1.
Move _is_conference_journal, _matches_journal_named_proceedings, infer_howpublished_from_doi, _normalize_howpublished, _DAGSTUHL_DOI_RE and the howpublished constant tables verbatim from merge_utils.py into a new leaf module src/venue.py (imports only config; no merge_utils cycle). merge_utils re-exports them so all callers resolve unchanged. Byte-neutral.
Gemini (?key=) and SerpAPI (&api_key=) pass credentials as query params; those URLs leaked into WARN logs via exception text and via the ValueError raised by _decode_json_bytes -- and output/run.log is committed to a public branch. Add a config-driven _scrub_secrets() (REDACT_QUERY_PARAM_NAMES) and apply it at _decode_json_bytes and the Gemini/SerpAPI log sites. Error tuples untouched (seam C2: do not add ValueError to the frozen tuples).
A malformed 200-response body makes _decode_json_bytes raise ValueError, which the inner NETWORK_ERRORS catch and the ALL_API_ERRORS decorator both miss (neither includes ValueError), crashing the run. Broaden the inner catch to ALL_FETCH_ERRORS (adds DECODE+PARSE) at datacite_search_doi and orcid_fetch_works. Frozen error tuples unchanged (seam C2).
A persistent 5xx made urllib3 raise RetryError after its own 2 retries, which the manual loop's broad RequestException catch re-drove up to 3x, compounding to ~9 requests per failure. Catch RetryError distinctly and propagate instead of re-driving. Also drop POST from urllib3 allowed_methods so non-idempotent request bodies are never silently re-sent. 429/503 stays single-layer (excluded from forcelist, handled by the manual loop); sleeps remain outside the global semaphore, backoff cap unchanged (seam C1).
C4: _month_boundary was frozen at ResponseCache construction, so a long-lived process spanning a month rollover kept serving last month's entries as fresh. Make it a property recomputed per access. Byte-neutral for short batch runs (same month); correct across a rollover. C3: ttl_days was written into every cache entry but never read by get() (positive-entry freshness is governed by the monthly boundary). Stop storing it. The put() parameter is retained for caller intent and logging.
The lock-free fast path read _OPENREVIEW_SESSION and _CREATED_AT in two separate global reads and could return a session a concurrent re-login had just cleared to None (or judge expiry from a torn pointer/timestamp pair). Move the reuse check inside _OPENREVIEW_SESSION_LOCK so both globals are read atomically together. OpenReview's Phase-2 position and the cache lock semantics are unchanged (seam C6).
Add iter_author_bibs()/iter_output_dirs() (sorted) and route the 8 author .bib and output-subdir scan sites through them, making determinism (sorted scan, surface #6) structural instead of duplicated. Byte-neutral: each site was already sorted or order-insensitive.
The host runs ruff format on save, so files touched during the audit were reflowed piecemeal, leaving formatting inconsistent. Apply ruff format uniformly so the whole tree is format-clean and future diffs stay minimal. No behavior change (formatting only); ruff check + mypy + pytest all green.
…Step 4) Move the precompiled fix-pattern tables/regexes and the shared title/booktitle text transforms (_fix_title_text, _apply_booktitle_fixups, _fix_fused_compounds, _is_garbage_title, _is_corrupted_title) verbatim from main.py into src/fixup/patterns.py + text.py (leaf modules, import only config + re). main.py re-exports them so the three fixup call sites and tests resolve unchanged. Byte-neutral; prerequisite for the C5 rule-body consolidation.
Promote the inline emission-order list in bibtex_from_dict to a module tuple so the byte-identity field-order contract has a named, referenceable constant. Byte-neutral.
Introduce src/entry.py::BibEntry as a thin typed wrapper over parse_bibtex_to_dict / bibtex_from_dict (from_bibtex/from_dict/to_dict/ to_bibtex). No pipeline call sites touched; serialization contract stays in bibtex_utils for a later step. Foundation for the domain-model migration that makes canonicalization intrinsic. 21 tests (byte-identity + round-trip fixpoint + defensive-copy).
Move _fixup_bib_entry (the post-run orphan-repair canonicalization) and its 18 module-local regexes verbatim from main.py into a new src/canonicalize.py, and define the CanonicalStage enum (LOAD_REPAIR, COMPLETE_SKIP_FINALIZE, POST_MERGE, POST_TIER2_VALIDATE, POSTRUN_ORPHAN_REPAIR). main.py re-exports them so the orphan-pass caller and tests resolve unchanged. Byte-neutral; first stage of the BibEntry canonicalization model.
Replace the ~400-line inline Phase-4 canonicalization block in main.py with a single canonicalize(merged, stage=POST_MERGE). Rules are now small shared _rule_* helpers referenced by ordered per-stage tuples (POST_MERGE and POSTRUN_ORPHAN_REPAIR share the same helper objects; no copy-paste), so the three-site duplication is dissolving into one single-sourced registry. _fixup_bib_entry is a thin wrapper. Byte-equivalent: full tests/test_regression.py (235) green + a corpus idempotency test over all 3565 .bib (strict data fixpoint). zenodo R16-before-R12 order preserved.
…AIR) Replace the ~470-line inline existing-file fixup block with canonicalize(fields, stage=LOAD_REPAIR), preserving Site B's EXACT rule subset and order (C-only rules R13/R19-misc/R20 stay absent -- asserted). N22 title==venue delete + email/&-escape write-gating stay in main.py as pipeline I/O; N22 moved after the single canonicalize call (byte-safe: the rules following it touch only doi/url/howpublished/author). 30 shared _rule_* helpers reused, 8 B-specific added (incl. an author-casing variant gating on the changed flag to avoid whitespace byte-divergence). Byte-equivalent: tests/test_regression.py 235/235 green + LOAD_REPAIR corpus data-fixpoint. All three fixup sites now single-sourced in canonicalize().
…_FINALIZE) Single-source the last live complete-entry quick-fixup (strip leaked preprint-only publisher when journal is a genuine venue) as a canonicalize rule. Delete the unreachable @Article+secondary-doi->@misc quick-fixup (the _entry_is_complete gate makes its guard provably always false). Tier2 path applies no canonicalization rules (pure field-fill plus validation) and stays inline.
Flatten the two-file src/fixup package (patterns + text, empty __init__) into a single src/textnorm module: a coherent title/booktitle text-normalization layer the canonicalization engine builds on, rather than a bolt-on 'fixup' package. Byte-neutral relocation (same compiled patterns, same functions); pattern dictionaries still sourced from src.config.
canonicalize() already delivers the end-to-end coherence BibEntry was
added to provide; the dict {type,key,fields} is the single representation
every consumer (merge, canonicalize, dedup, serialize) speaks. BibEntry
had no production caller and its tests proved it a no-behavior facade, so
it was dead scaffolding and a second normalization source. Remove it.
The tier2 path applies no canonicalization rules (pure field-fill plus validation) and is never routed through canonicalize(); the enum member only tripped the NotImplementedError guard. Remove it so every CanonicalStage maps to a wired rule tuple.
A non-JSON 200 body (HTML gateway page) made _decode_json_bytes raise a bare ValueError that ALL_API_ERRORS did not catch, so the generic search path backing S2/Crossref/OpenAlex dropped the whole article instead of skipping one source. Introduce DecodeError(ValueError) in DECODE_ERRORS so every 'except ALL_API_ERRORS' client degrades gracefully; unrelated ValueErrors still propagate. Also scrub secrets in the _http_request fallback exception, the handle_api_errors debug log, and the decode preview so a new secret-bearing URL cannot leak (completes C2/C2b).
infer_howpublished_from_doi returns labels like EGU, Preprint, and Institutional Repository that are not conference venues. R20's misc-> inproceedings upgrade lacked them in its guard, so a backfilled preprint howpublished was rewritten into a fabricated booktitle. Gate R20 against any infer_howpublished_from_doi output, and add the inverse downgrade so an already-fabricated @inproceedings self-heals to @misc. A real venue carrying a preprint-prefix DOI (booktitle != the bare label) is preserved. Regenerate the two affected corpus entries to their corrected fixpoint.
…ering The whole-function scheduler extraction moved the author count-sort (and its 'Sorting authors'/'Author range' PLAN log lines) inside run_all, which runs after init_summary_csv -- shifting two setup lines in run.log relative to the original. Split the count-sort into prioritize_records(), called from main() before init_summary_csv, restoring the exact original run.log ordering. Byte- neutral (corpus oracle 0 drift); the only skeptic-confirmed divergence, closed.
_fix_fused_compounds applied ~800 compound-word patterns x3 passes to every title (profiled: 3.44M re.sub calls, 47% of the parse->canonicalize->serialize CPU, almost all matching nothing). A \b<word>\b pattern cannot match unless the literal is a substring of the working string, so pre-guard each sub with a cheap substring test (config-sourced literals). IGNORECASE fused patterns guard only while the string isascii() (str.lower() vs regex case-folding differ for long-s/ dotted-I); case-sensitive suffix/acronym patterns guard unconditionally. Byte-identical: corpus oracle 0 drift + differential fuzz 0 mismatches over 3358 corpus fields + 4604 adversarial Unicode strings. Hot path 1644->4753 entries/s (2.9x).
_strip_latex_formatting and _normalize_to_ascii ran the full LaTeX-command scan, html.unescape, strip_accents, and unicode-replacement passes on every field (profiled ~2s over the corpus). Guard each with its necessary trigger: _strip_latex returns early when no backslash/tilde/'--'/double-space is present; _normalize_to_ascii runs html.unescape only on '&', strip_accents plus the unicode-quote/dash replacements only when the value is non-ASCII, and the apostrophe-year regex only when a quote is present. Byte-identical: corpus oracle 0 drift + differential fuzz 0 mismatches over 29052 corpus+adversarial inputs. Hot path 4753->8872 entries/s (5.4x over the pre-perf baseline).
There was a problem hiding this comment.
Pull request overview
Refactors CiteForge’s enrichment/canonicalization pipeline to decompose orchestration logic, consolidate repeated normalization/type-fixup behavior into reusable modules, and apply targeted defect fixes (notably HTTP retry bounding and secret redaction) while aiming to preserve byte-identical deterministic output.
Changes:
- Introduces new pipeline modules (
src/pipeline/*) and deterministic filesystem scan helpers (src/fsscan.py) as part of the orchestrator decomposition. - Adds/centralizes normalization and venue logic (
src/textnorm.py,src/venue.py) and expands stage-based canonicalization coverage/tests. - Fixes and hardens HTTP behavior (retry amplification, POST retry exclusion, secret scrubbing) and cache boundary/TTL behaviors, with new regression tests.
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| main.py | Thin entrypoint wiring the decomposed pipeline and post-run finalization. |
| CLAUDE.md | Adds contributor-facing build/architecture/determinism notes and conventions. |
| src/api_configs.py | Reformat/config tweaks for API search/field mapping definitions. |
| src/api_generics.py | Minor refactors/formatting in generic API search/build helpers. |
| src/bibtex_build.py | Formatting and small logic refactors in entry building/type detection. |
| src/bibtex_utils.py | Adds preferred field order + hot-path optimizations in normalization/serialization. |
| src/cache.py | Fixes month-boundary recomputation and stops persisting unused ttl_days. |
| src/clients/helpers.py | Formatting-only changes in scoring/helpers. |
| src/clients/scholar.py | Formatting-only changes; minor readability improvements. |
| src/clients/scholarly_scholar.py | Removes dead scholarly-based Scholar client implementation. |
| src/clients/search_apis.py | Concurrency fix for OpenReview session access + formatting cleanup. |
| src/clients/serpapi_scholar.py | Adds secret scrubbing to SerpAPI failure logs + formatting cleanup. |
| src/clients/serply_scholar.py | Minor slice formatting adjustment. |
| src/clients/utility_apis.py | Switches to broader fetch error tuple; scrubs secrets in Gemini error logging. |
| src/doi_utils.py | Formatting and small logging argument formatting changes. |
| src/exceptions.py | Introduces DecodeError and expands decode/fetch error tuples. |
| src/fsscan.py | New: centralized deterministic directory scans for output stability. |
| src/http_utils.py | Adds URL/exception secret scrubbing; bounds retry amplification; introduces DecodeError path. |
| src/id_utils.py | Formatting-only changes. |
| src/io_utils.py | Formatting-only changes. |
| src/log_utils.py | Formatting-only changes. |
| src/merge_utils.py | Extracts venue/howpublished helpers to src/venue and uses deterministic iter_author_bibs. |
| src/models.py | Minor spacing/formatting change. |
| src/pipeline/init.py | New package init for pipeline decomposition. |
| src/pipeline/postrun.py | New: strict post-run finalization tail (CSV reconcile, cleanup, fixup, a2i2, baselines, badges). |
| src/pipeline/scheduler.py | New: parallel author scheduling/processing, logging, prioritization. |
| src/publication_parser.py | Formatting-only changes + minor comment spacing fix. |
| src/text_utils.py | Formatting-only changes + minor readability refactors. |
| src/textnorm.py | New: compiled regex-based title/booktitle normalization primitives + garbage detection. |
| src/venue.py | New: venue inference helpers (conference-as-journal checks, DOI→howpublished inference, normalization). |
| output/Spadon (bfdGsGUAAAAJ)/Oishi2021-NeuralNetworks.bib | Updates emitted BibTeX fields (adds howpublished=Preprint). |
| output/Rosborough (kG4oQmsAAAAJ)/Reynolds2025-IntellectualProperty.bib | Corrects entry type/container handling (inproceedings→misc, booktitle→howpublished). |
| output/a2i2/Oishi2021-NeuralNetworks.bib | Mirrors output corpus change for a2i2 aggregated folder. |
| tests/fixtures.py | Formatting-only change in API key fixture loading. |
| tests/test_apis.py | Formatting-only changes in API tests. |
| tests/test_cache.py | Makes cache-expiry tests date-independent; adds TTL/month-boundary regression tests. |
| tests/test_canonicalize.py | New: stage-parameterized canonicalize() rule coverage + corpus fixpoint assertions. |
| tests/test_config.py | Formatting-only changes. |
| tests/test_core.py | Formatting-only changes; updates expected unicode normalization strings to match implementation. |
| tests/test_data.py | Formatting-only changes in canned test data. |
| tests/test_fsscan.py | New: deterministic directory scan tests for fsscan helpers. |
| tests/test_http_utils.py | New: regression tests for secret redaction + bounded retry behavior. |
| tests/test_integration.py | Formatting-only changes; refactors enrichment-source loop layout. |
| tests/test_io_csv.py | Formatting-only changes in CSV IO tests. |
| tests/test_pipeline.py | Formatting-only changes in DOI validation tests. |
| tests/test_publication_parser.py | Formatting-only changes in publication parser tests. |
| tests/test_scholarly_scholar.py | Formatting-only changes in removed-scholarly client test file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| logger.info(f"Log file: {logger.log_file_path or 'n/a'}", category=LogCategory.PLAN) | ||
|
|
||
| if summary_csv_path and os.path.exists(summary_csv_path): |
Comment on lines
+116
to
+123
| for entry in os.listdir(out_dir): | ||
| d = os.path.join(out_dir, entry) | ||
| if not os.path.isdir(d) or entry == "a2i2": | ||
| continue | ||
| for fname in os.listdir(d): | ||
| if not fname.endswith(".bib"): | ||
| continue | ||
| fpath = os.path.join(d, fname) |
Comment on lines
+349
to
+355
| except TimeoutError: | ||
| remaining = [r.name for f, r in future_to_author.items() if not f.done()] | ||
| logger.error( | ||
| f"Pipeline timed out with {len(remaining)} author(s) still pending: " + ", ".join(remaining[:5]), | ||
| category=LogCategory.ERROR, | ||
| ) | ||
| return total_saved, processed |
Comment on lines
+297
to
+299
| # Per-author timeout: 30 minutes per author to handle large publication lists | ||
| # Each article takes ~60-90s across all API calls, so 24 articles ≈ 36 minutes | ||
| author_timeout = 1800 # seconds |
Center on merge_with_policy, compute_dedup_score, and preprint/published classification so every entry keeps the most-trusted, most-recent metadata, published records outrank preprints on DOI, and repository/preprint entries are retained when no published twin exists. Rules stay open-ended (no hardcoded title/DOI/author/venue); output stays deterministic (two-run byte-identical, canonicalize fixpoint unchanged over the committed corpus). - Count the preprint-vs-published (XOR) split exactly once: venue_similarity is pure venue-string similarity and the split is a single gated signal in compute_dedup_score (new count_preprint_xor flag). Pre-conditioned dedup sites pass count_preprint_xor=False instead of subtracting a mis-predicated 0.10. Fixes false merges of distinct works and false rejects of genuine twins. - Unify the preprint-DOI predicate: _is_preprint_doi delegates to is_secondary_doi, and PREPRINT_DOI_PREFIXES gains the specific preprint/grey sub-prefixes (EGU egusphere, OSTI, Qeios, agriRxiv, Underline, institutional). A published journal under the same registrant (10.5194/acp) stays published. - Never downgrade a published-DOI paper: the preprint-journal canonicalize rules strip a stale preprint journal without stamping howpublished=<server> when a genuine published DOI is present. - Never drop authors: a truncated author list cannot overwrite a more complete one unless the source is >= TRUST_DIFF_OVERRIDE_THRESHOLD more trusted. - Keep a record's own preprint/repository self-DOI through the registry trust gate; unverified published DOIs are still dropped. - Published supersedes preprint in the save-time candidate-DOI net. - Gate the generic->specific booktitle upgrade by trust rank. Adds tests/test_decision_reinforcement.py with parametric multi-author/venue cases per rule; updates the venue_similarity regression tests to the corrected single-count contract.
Two campaign-validation defects fixed at their root: - postrun: remove a preprint .bib when a published record of the same work (title similarity >= dedup threshold, real DOI/container) exists in the same author dir. Generalizes 'published outranks preprint'; standalone preprints are always retained and published files are never removed. Deterministic and idempotent. - scoring: thread the known article year into crossref/openalex multi-search scoring so a same-year, same-author, near-identical-title published record earns the year bonus and clears the accept threshold. Recovers authoritative published DOIs that a trivial title-word difference had left as preprints, without weakening any threshold (title-min and author gates still gate first).
bibtex_entries_match_strict treated any exact DOI or arXiv-eprint match as a definite match with no title check. A Scholar entry carrying the wrong arXiv id (e.g. 'Movelet Trees' pointing at 2408.04537 = 'Faster run-length compressed suffix arrays') therefore adopted the wrong paper's CSL metadata, overwriting the title, and did so intermittently (the metadata fetch is flaky) which broke run-to-run determinism. Guard both exact-identifier fast paths with a title-conflict check (SIM_IDENTIFIER_TITLE_MIN=0.55): when both records have titles and they are clearly different papers, the shared identifier is a mislabel and is not a match. Missing-title records still match on the identifier alone. Fixes the mis-identification (correctness) and the resulting oscillation (determinism).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multi-phase software-engineering audit and refactor of CiteForge: coherence cleanup, confirmed-defect fixes, decomposition of the monolithic orchestrator, and measured performance tuning — all preserving the byte-identical determinism contract.
What changed
Coherence / repetition
canonicalize(entry, stage=…)engine (src/canonicalize.py); 31 rules are shared helper objects dispatched per stage, zero inline reclassification rules remain.src/fixup/patch-package with a coherentsrc/textnorm.pytext-normalization layer.BibEntrytype (the dict{type,key,fields}is the single end-to-end representation) and the deadPOST_TIER2_VALIDATEenum member.Confirmed defects fixed
RetryErrorno longer re-driven).DecodeError ∈ ALL_API_ERRORS(no longer drops the whole article).ttl_days;_month_boundaryrecomputes (no frozen singleton).src/clients/scholarly_scholar.py.EGU,Preprint,Institutional Repository) no longer fabricated into@inproceedings; corrected 1 shipped entry + stabilized 1 stale entry.Decomposition of the monolith
main.py3235 → 123 LOC (thin entry point) →src/pipeline/{article,scheduler,postrun}.py.main.pyremains a pure leaf importer (nosrc → mainback-edges).Performance (measured, byte-identical)
re.sub(3.44M nearly-all-miss calls).Verification (all green)
ruff check✓ ·mypy✓ (33 files) ·pytest461 passed ✓canonicalizeis a byte-no-op on all 3565output/**/*.bibat every stage (0 drift).output/*.bibbytes unchanged across the entire decomposition.Conventions respected throughout: pip, mypy, config-driven thresholds (no hardcoded values). Every rule generalizes across all authors/papers.