Skip to content

fix(identifierLookup): stop rejecting valid short SMILES strings - #42

Merged
djleamen merged 1 commit into
mainfrom
claude/awesome-planck-nwk1xb
Aug 9, 2026
Merged

djleamen merged 1 commit into
mainfrom
claude/awesome-planck-nwk1xb

Conversation

@djleamen

@djleamen djleamen commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Summary

In isReasonableInput, the generic short-token gibberish guard

if (/^[a-z]{1,3}$/.test(trimmed) && ['o', 'c', 'n', 'h'].includes(trimmed)) return false;

ran before the per-type switch, so any 2–3 character lowercase SMILES not in the atom allowlist was rejected with "Invalid {sourceType}" before lookupChemicalIdentifiers ever consulted the local DB or PubChem. That silently breaks valid molecules such as CC (ethane), CCO (ethanol), CCN (ethylamine), and CCC (propane) — even ones present in the built-in commonCompounds table.

Change

Skip that heuristic for smiles and inchi, which already have dedicated format checks inside the switch. Other identifier types (IUPAC names, etc.) still get the short-gibberish guard unchanged.

Verification

Simulated isReasonableInput over representative inputs:

type value before after
smiles CC / CCO / CCN / CCC rejected ❌ accepted ✅
smiles c1ccccc1 accepted accepted ✅
iupacName xyz (gibberish) rejected rejected ✅
iupacName water accepted accepted ✅
casNumber / pubchemCID valid accepted accepted ✅

Single-atom SMILES (O, C, N) remain gated by the pre-existing length < 2 check — a separate limitation left out of scope here.

Relates to #4 (limited SMILES parser). The repo has no test framework configured, so the change is validated by logic simulation rather than a unit test.


Generated by Claude Code

The generic short-token gibberish guard in isReasonableInput ran before the
per-type switch, so any 2-3 character lowercase SMILES not in the atom
allowlist (e.g. CC ethane, CCO ethanol, CCN ethylamine) was rejected with
'Invalid smiles' before any lookup. Skip that heuristic for smiles/inchi,
which already have dedicated format checks. Relates to #4.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_016JeXARZo3JvULrjBzujdzV
Copilot AI lite review requested due to automatic review settings August 8, 2026 11:23
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@sonarqubecloud

sonarqubecloud Bot commented Aug 8, 2026

Copy link
Copy Markdown

djleamen commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

The build matrix is failing at npm ci with an ERESOLVE peer-dependency conflict ([email protected] in the root vs [email protected], which only supports eslint <=8). This happens during dependency install — before any of this PR's code is compiled — so it is not caused by this change; the same failure reproduces on main.

It's the exact conflict already tracked/fixed by #41. Once #41 lands, I'll merge main into this branch and CI should go green. This PR's diff is a single-line guard in isReasonableInput and is unrelated to the dependency graph.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes pre-validation in src/utils/identifierLookup.ts so valid short SMILES strings (e.g., CC, CCO, CCN, CCC) are no longer rejected as “very short gibberish” before the per-identifier-type validation and lookup logic runs.

Changes:

  • Skips the short lowercase-token “gibberish” heuristic for smiles and inchi.
  • Adds inline comments explaining why short SMILES must bypass that heuristic.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

djleamen commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Heads-up on the red check here: the build (18.x) job fails at npm ci with an ERESOLVE between [email protected] and [email protected] (the 20.x/22.x jobs are fail-fast cancellations of the same run). This is a pre-existing dependency conflict on main, not something this change introduces — this PR only touches src/utils/identifierLookup.ts.

The actual fix is #41, which migrates to the ESLint 10 flat config, bumps eslint-plugin-react-hooks to v7 (its peer range covers eslint 10), and drops Node 18 from the CI matrix. Once #41 merges into main, updating this branch will clear the red check with no changes needed here.


Generated by Claude Code

@djleamen
djleamen merged commit 475a3cb into main Aug 9, 2026
7 of 10 checks passed
@djleamen
djleamen deleted the claude/awesome-planck-nwk1xb branch August 9, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants