Repository audit: fix all High/Medium findings and 13 of 14 Low findings#19
Merged
Merged
Conversation
Full manual review of the quantui package: 5 high-severity functional bugs (isosurface charge/spin omission, Bohr/Angstrom unit mismatch in frequency results, wB97X-D export-script breakage, dead RDKit XYZ->SVG path, XYZ parser dropping the first atom on blank title lines), 14 medium findings, and a set of low-severity inefficiencies and doc drift, with file:line references and suggested fixes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
- H1: generate_cube_from_arrays/generate_cube_file built the PySCF Mole without charge, so the isosurface panel failed to build for every charged or open-shell (odd-electron) molecule. Add infer_charge_and_spin() (derives both from the atom list + saved MO occupations) and thread charge/spin through the render path. Also dedupes the atomic-number table into molecule.ATOMIC_NUMBERS. - H2: freq_calc built pyscf_mol_atom from PySCF's internal mol._atom, which is always Bohr, while every consumer (Molden export, cube generation, orbital replay) assumes Angstrom like session_calc/ optimizer use. Build it from molecule.atoms/coordinates instead, and convert Angstrom->Bohr explicitly when writing Molden's [FR-COORD] block (which the Molden spec requires in Bohr regardless of [Atoms]'s unit tag). - H3: PySCFCalculation uppercased the method name before checking it against the mixed-case SUPPORTED_METHODS list, so "wB97X-D" became "WB97X-D" and matched nothing -> Export Script and the educational notes panel silently broke for every mixed-case method. Compare case-insensitively while preserving the canonical spelling. - H4: generate_2d_structure_svg's xyz_string path called AddAtom on an immutable Chem.Mol (only RWMol supports it) and had an atom/conformer index desync when malformed lines were skipped, so it always returned None. Build on RWMol with a properly sized Conformer, convert with GetMol() before DetermineBonds. - H5: parse_xyz_input stripped blank/comment lines before detecting the count+title header, so a standard XYZ file with a blank or "#"/"!" title line had its first atom silently absorbed into the header skip. Detect the header positionally on raw lines first, then filter. Added regression tests for all five (charge/spin cube generation, Bohr/Angstrom pyscf_mol_atom + FR-COORD conversion, wB97X-D method canonicalization, XYZ-to-SVG round trip, and XYZ header/title edge cases). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
M1: config.VALID_ATOMS stopped at Kr (Z=36), rejecting real structures resolved via PubChem/CACTUS/SMILES for any heavier element (iodine, tin, gold, platinum, ...) even though molecule.py's own error text listed iodine as a valid example. Extend to the full periodic table (Z=1..118) via a new config.ATOMIC_NUMBERS table, with VALID_ATOMS derived from it so the two can never drift apart again. molecule.py's ATOMIC_NUMBERS now re-exports the config copy instead of duplicating it. M2: investigating the audited "silent RHF/ROHF reference" concern for MP2/CCSD/CCSD(T) in session_calc.py showed PySCF's own factory functions (scf.RHF, mp.MP2, cc.CCSD) already auto-dispatch correctly for open-shell input (ROHF reference, ROHF-based UMP2/UCCSD) - verified empirically, no fix needed there beyond clarifying the misleading "RHF reference" comments. That investigation surfaced the real bug: optimizer.py, pes_scan.py, freq_calc.py, tddft_calc.py, and nmr_calc.py have no special-casing for these post-HF methods at all, so selecting MP2/CCSD/CCSD(T) for any calc type other than Single Point silently falls into the DFT branch (sets e.g. mf.xc = "CCSD") and fails deep inside PySCF with a cryptic "LibXCFunctional: name 'CCSD' not found" - reachable from the UI since SUPPORTED_METHODS isn't restricted per calc type. Added an early guard (new config.POST_HF_METHODS) to all five entry points that raises a clear, actionable ValueError before any PySCF machinery runs. Also fixed a pre-existing test-isolation bug in test_optimizer.py: TestOptimizeGeometryImportGuards monkeypatched ase_bridge.ASE_AVAILABLE then used importlib.reload() to pick it up in quantui.optimizer, but monkeypatch's auto-revert doesn't undo the reload - quantui.optimizer. ASE_AVAILABLE stayed stuck at False for the rest of the test session. Patch optimizer's own ASE_AVAILABLE name directly instead; no reload needed, no leak. Added regression tests for all of the above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
M3: session_calc.py skipped Mulliken charge and dipole moment extraction entirely for method_upper == "UHF", even though both mf.mulliken_pop() and mf.dip_moment() are well-defined and work correctly for a real UHF object (verified empirically) - UKS (open-shell DFT) went through the identical code successfully the whole time. Removed the unnecessary restriction. M4: nmr_calc.py silently substituted the B3LYP/6-31G* TMS reference constants for any method/basis not in config.NMR_REFERENCE_SHIELDINGS, with no record anywhere that a substitution happened - chemical shifts could be off by a few ppm with no indication to the student, and the lookup was also case-sensitive. Added resolve_nmr_reference() (case- insensitive lookup), new NMRResult.reference_key / is_fallback_reference fields, and a result-card warning in app_formatters.py surfacing the substitution when it happens. Added regression tests for both, including a fix to a pre-existing test in test_session_calc.py that asserted the old (buggy) None behavior for UHF Mulliken/dipole. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
Both the serial (freq_calc._displaced_scf_dipole) and parallel
(freq_ir_workers.run_displaced_scf) inner loops for the numerical
IR-intensity finite-difference dipole calculation picked RHF/UHF (or
RKS/UKS) based on mol.spin == 0 alone. That only agrees with the
parent SCF's actual type when the user's method choice matches the
molecule's natural spin state. They diverge when a user explicitly
selects UHF for a closed-shell molecule (mol.spin == 0, but the parent
mf - and its dm0 initial guess - is still UHF-shaped, a legitimate
technique for probing symmetry-broken solutions): the inner loop built
RHF from mol.spin == 0, then fed it the UHF-shaped dm0, which raised a
shape-mismatch ValueError deep inside PySCF - silently dropping IR
intensities for the entire run (caught by the broad except around the
whole IR-intensity block). Verified this exact failure mode
empirically before fixing it.
Fixed by dispatching on dm0's actual shape - (2, nao, nao) for
UHF/UKS/ROHF, (nao, nao) for RHF/RKS - instead of mol.spin, in both
the serial and parallel (ProcessPoolExecutor worker) code paths.
Also implemented the fallback that freq_ir_workers.run_displaced_scf's
own docstring already claimed existed ("the freq_calc driver catches
such failures and falls back to the serial loop") but didn't: any
single worker failure previously propagated straight out to the outer
except, dropping IR intensities entirely even when most of the other
6N-1 displacements would have succeeded serially. A parallel-path
failure now falls back to the serial loop instead of giving up.
Added regression tests for both the dispatch fix and the fallback
behavior (the latter via a mocked ProcessPoolExecutor failure).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
…al scans)
M6: PESScanResult.energy_hartree, energies_relative_kcal, and summary()
all called min()/max() directly on energies_hartree, which contains
float("nan") for failed scan points. Python's min/max are order-
dependent with NaN present - a NaN as the first element poisons the
result, one later in the list is silently ignored - so whether a scan
was usable depended on *which* point happened to fail. Added
_finite_energies() to filter NaN out before every min/max call.
Also fixed a failed scan point's trajectory frame: it previously
recorded the raw *input* molecule regardless of how far the scan had
progressed, producing a bogus discontinuity in the trajectory
animation. Now falls back to the last successfully-computed geometry.
M7: verified empirically that FixInternals(angles=[...]) /
FixInternals(dihedrals=[...]) - the deprecated radian-based kwargs
pes_scan.py used - don't just emit a FutureWarning with the ASE
version QuantUI targets (verified against 3.29.0): they unconditionally
raise "setting an array element with a sequence" from an internal
np.asarray reshape. Every angle and dihedral PES scan was failing at
100% of its points, silently (caught by the per-point try/except) -
this was a currently-broken feature, not just a deprecation warning
waiting to become an error. Switched to angles_deg/dihedrals_deg
(plain degrees, no radian conversion, correct list-of-lists format per
ASE's own docstring), which fixes both scan types.
Added regression tests for all of the above, including one verified to
fail against the pre-fix code (a monkeypatched BFGS failure mid-scan)
and integration tests that actually run angle/dihedral scans end to
end with real PySCF - which the existing test suite never did, which
is how the M7 bug went undetected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
M8: log_event() called prune_events() after every single append, and prune_events() itself read the file (acquiring and releasing the module lock), computed the filtered list outside any lock, and only then reacquired the lock to rewrite it. A concurrent append landing in that gap was silently discarded when the rewrite replaced the whole file with the stale filtered list. Verified empirically: an 8-thread, 240-event stress test lost 114 events (~47%) on the old code, zero on the fix. Fixed by making prune_events' read + filter + rewrite a single lock-held critical section, and by only running the full prune every 20 events instead of on every one (removing the O(N) cost per event this was otherwise paying). M9: save_thumbnail's docstring promises to "silently skip ... if matplotlib is unavailable or any error occurs", but only the `import matplotlib` block was actually guarded - figure construction, text rendering, and fig.savefig() (a real filesystem write that can hit disk-full/permission errors) could all raise straight past the function. Split the figure-building code into a separate helper and wrapped the whole call + save in try/except/finally so the documented behavior actually holds, with plt.close(fig) still running via finally regardless of where a failure happens. Added regression tests for both, including a concurrency stress test for M8 and a mocked savefig-failure test for M9, both verified to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
…tion)
M10: _http_get() returned None (instead of a requests.Response) if
config.PUBCHEM_MAX_RETRIES were ever 0 or negative, since
`range(0)` never enters the loop body that assigns `response` -
every caller would then hit AttributeError on response.status_code.
Guard with max(1, ...) so at least one attempt always happens.
check_pubchem_availability() called requests.get() directly, bypassing
the shared client-side rate limiter every other PubChem call goes
through (a burst of concurrent availability checks could exceed
PubChem's server-side throttle) and hardcoding timeout=5 instead of
the config.PUBCHEM_AVAILABILITY_TIMEOUT_S constant defined specifically
for this probe. Routed through _throttle() + the configured timeout
(kept as a single no-retry request, matching its "quick reachability
probe" intent).
M11: four export filename builders in app_exports.py (Export Script,
XYZ, MOL, PDB) embedded the basis set verbatim, e.g. "6-31G*.py" -
invalid on Windows ("*" is a reserved filename character there, and
the non-PySCF UI is supported on Windows) and glob-hostile on POSIX.
Reused results_storage._safe_name(), already used for the identical
purpose when naming result directories, instead of introducing a new
sanitizer.
Added regression tests for all of the above, each verified to fail
against the pre-fix code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NtrUGxtEAakaEqL1PNjTSa
.replace("**", ..., 1) only converts the first **bold** pair in the
whole string, so every paragraph after the first in
get_educational_notes()'s output kept its literal ** markers instead
of rendering bold. Replace with a regex that converts every pair.
Importing any submodule of quantui always runs quantui/__init__.py first, and that module eagerly imported QuantUIApp, StepProgress, and help_content — pulling in ipywidgets and the rest of the GUI stack even for callers that only want pure-Python helpers like quantui.cli. Resolve these lazily via module __getattr__ (PEP 562), matching the existing MOLECULE_LIBRARY pattern in config.py.
utils.py configured the root logger process-wide the moment quantui was imported, hijacking/duplicating any logging setup a host application or notebook already had. quantui/__init__.py already attaches a NullHandler to the package logger, so drop the basicConfig() call and let host apps opt into console logging themselves.
- calc_log.py: 6-31G**'s He entry was 2 (bare 6-31G* value); 6-31G** adds a p-polarization shell on H/He like it does on heavy atoms in 6-31G*, so He should match H's 5, not stay at 2 (verified against pyscf.gto.M(atom="He", basis="6-31g**").nao == 5). - Add config.BOHR_TO_ANGSTROM (matching pyscf.data.nist.BOHR) as the single source of truth for Bohr<->Angstrom conversions; optimizer.py previously hand-typed a different, slightly stale literal (0.529177249) than freq_calc.py's (0.52917721092), and results_storage.py had its own independently-typed inverse. - benchmarks.py: _count_electrons carried its own truncated (Z=1-18) atomic-number table, silently falling back to carbon's Z=6 for any heavier element; now uses the shared, full-periodic-table config.ATOMIC_NUMBERS. - Fix docstring/comment drift: session_calc.run_in_session's verbose default text (3 -> 4), pubchem.search_molecule_by_name's "None otherwise" (the function always raises, never returns None), and pyproject.toml's incorrect claims about pyscf.prop.infrared / pyscf.nmr availability (verified neither exists in pyscf 2.13). - cli.py: silence an unused _open_in_browser return value.
pubchem.py, cactus.py, molecule.py, and app_visualization.py each re-raised a different exception type inside an except block without `from`, discarding the original traceback context (a real HTTP failure vs. a bug in the handler look identical in the log). Chain via `raise ... from e` at all 12 sites B904 identifies.
_render() interpolated step labels and status messages directly into the widget's HTML string. A message containing "<"/">" (e.g. echoing a parse error back from user input) broke the widget's markup instead of rendering as literal text. Escape both via html.escape().
energy_hartree, homo_lumo_gap_ev, converged, n_iterations, and the post-HF correlation fields were written straight from getattr() without going through _opt_float/_opt_int. numpy.float64 happens to subclass float so it round-tripped through json.dumps by accident, but numpy.float32, numpy.int64, and numpy.bool_ do not subclass float/int/bool and raise TypeError unconverted — a real risk for any duck-typed result object (e.g. one carrying GPU-offloaded float32 values). Add _opt_int alongside the existing _opt_float and apply both uniformly.
All 5 High and 14 Medium findings, plus 8 of 14 Low findings, are fixed and merged. The remaining 6 Low items (perf caching, CI/Python 3.9 verification, NMR monkey-patch idempotency, list_results sort order, prune_events retention, single-atom XYZ rejection) are design/infra-level changes rather than bugs and are left as deliberately deferred, noted individually.
Same-microsecond collision-suffixed directories (…_1, …_10, …_2) sorted lexicographically, so "…_10" (created later, after 10+ collisions) sorted before "…_2" instead of after it. Sort on the numeric suffix instead of the raw string.
estimate_time() re-parses the entire (indefinitely-kept) perf log on every UI refresh, even when nothing new has been written since the last call. Cache the parsed records keyed by the file's (mtime, size), so repeat calls between writes skip the read and per-line json.loads entirely; a write always bumps both, so the cache can't go stale.
_run_nmr_calc_body redefined and reassigned the same two closures (gen_vind, get_vxc_giao) on every NMR call, even though nothing about the installed pyscf/pyscf-properties changes between calls. Extract both into a versioned, idempotent _ensure_nmr_compat_patches_applied() that checks a sentinel attribute before re-patching.
Atomic calculations (e.g. a lone Ar atom) are legitimate PySCF targets — the >=2-atom rejection was a design restriction, not a correctness requirement. Remove the check and confirm single-atom input runs end-to-end through run_in_session (RHF/STO-3G on Ar converges cleanly).
pyproject.toml claims Python 3.9 support (requires-python, classifier) but CI only ran the test suite on 3.10/3.11, so the claim was never actually verified. Add 3.9 to the Linux test matrix.
Perf-log caching, Python 3.9 CI coverage, NMR patch idempotency, list_results() sort order, and single-atom XYZ support are now all fixed. Only #12 (prune_events malformed-entry retention) remains deferred, by explicit choice to keep its existing deliberate behavior.
- orbital_visualization.py: unquote infer_charge_and_spin's type annotation (redundant under `from __future__ import annotations`, and the quoted form was both unformatted by black and flagged by ruff's UP037) and reformat the resulting line. - app.py: add a no-op _LogCapture.close(). ase.utils.IOContext.openfile() (used by BFGS(..., logfile=...) in optimizer.py / pes_scan.py) checks hasattr(file, "close") to decide whether *file* is an already-open stream vs. a path to open() itself. ase==3.26.0 — the newest version pip resolves for Python 3.9 — enforces this strictly and raised TypeError for a _LogCapture instance; ase==3.29.0 (resolved for 3.10/3.11) tolerated it via a later refactor, masking the gap until the L6 audit fix's CI matrix expansion added a 3.9 job. Verified end-to-end against a real ase==3.26.0 install: the previously-failing geometry-opt/frequency history tests all pass now. - test_results_storage.py: switch a type()-is comparison to isinstance() — flagged by the pre-commit-pinned ruff (0.4.7) even though a newer local ruff didn't flag it.
NCCU-Schultz-Lab
added a commit
that referenced
this pull request
Jul 16, 2026
Bump version to 0.4.1 and add changelog 0.4.1 is a bug-fix and hardening release capturing the repository audit (PR #19): 5 High, 14 Medium, and 13/14 Low findings fixed, plus Python 3.9 added to CI. No new features and no breaking changes. - Bump version 0.4.0 -> 0.4.1 in pyproject.toml, quantui/__init__.py, and the apptainer/quantui.def Version label. - Add the CHANGELOG [0.4.1] section (user-facing summary of the audit fixes). - Refresh the CHANGELOG compare-links, which were stale (Unreleased pointed at v0.2.0; the 0.3.0 and 0.4.0 links were missing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
Full audit of the repo's core calculation, persistence, resolver, and infrastructure modules, followed by fixes for every finding except one (left as-is by choice). See
AUDIT_FINDINGS.mdfor the full writeup with per-item status.wB97X-Dexport/notes, a dead XYZ→SVG code path, and an XYZ parser silently dropping the first atom on files with a blank/#title line.FixInternalskwargs, an event-log append/prune race, an overly-narrow exception guard insave_thumbnail, several PubChem client edge cases, unsanitized export filenames, markdown-to-HTML mangling in method notes, the CLI eagerly importing the full GUI stack, andlogging.basicConfig()mutating the root logger at import time.raise ... fromexception chaining (12 sites), unescaped HTML inStepProgress, non-JSON-safe fields insave_result, perf-log parsing now cached on(mtime, size), an idempotent NMR monkey-patch, a numeric (not lexicographic)list_results()sort, single-atom molecules no longer rejected, and Python 3.9 added to the CI matrix.prune_eventskeeping malformed log lines forever) is left as-is — that behavior was already documented as deliberate, and it stays that way by explicit choice.Test plan
git stash) and pass post-fix.pytest -q -m "not network") green after every change — no regressions.black --checkandruff checkclean on every touched file (pre-existing lint findings unrelated to these changes were left alone and confirmed pre-existing viagit stashdiffing).run_in_session(RHF/STO-3G) and converges.main, so this PR is the first real signal on whether 3.9 actually passes.Generated by Claude Code