Encode component search predicates as JSON instead of ";"-delimited strings - #206
Merged
Conversation
The component query parameter used a ";"-delimited string, which collided with SMARTS patterns that contain ";" (a low-precedence AND) -- a model- or user-supplied SMARTS like "[#6;R]" mis-parsed and 500'd. Each component value is now a JSON object {pattern, target, mode}, parsed via a ComponentSpec model. - search.py: add ComponentSpec; parse component values with model_validate_json instead of str.split(";"). - Frontend: encode with JSON.stringify (MainSearch) and decode with JSON.parse (SearchOptions); drops the dead "%3D" un-escaping hack (URLSearchParams already decodes). - Tests: JSON-format query params via a _spec() helper, plus a regression test for a SMARTS containing ";". The natural-language endpoint (separate PR) builds the same spec string and will switch to JSON in a one-line follow-up once both land. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Previously shared search URLs use the old "pattern;target;mode" encoding. Parse both formats on read so those URLs keep working: ComponentSpec.parse() takes JSON or the legacy form (right-split, since a SMARTS pattern may contain ";"), and the frontend's parseComponent() falls back the same way instead of throwing on JSON.parse. A spec that is neither JSON nor a 3-field legacy string now returns a controlled 400 rather than a 500. Addresses Greptile's two P1s on #206 (legacy URL crash/500). Adds tests for the legacy format and the invalid-spec 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI runs `ruff format --check`; reformat the new test params. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses two follow-up P1s from Greptile on #206: - Backend: a valid JSON spec with an unknown target/mode (enum KeyError) or an unparseable pattern (ValueError from ReactionComponentQuery) bypassed the parse guard and 500'd. Widen the try/except to cover predicate construction so these return a 400. Adds a bogus-mode regression test. - Frontend: JSON.parse succeeds for "null"/"123"/etc., which then crash on parsed.target. parseComponent now shape-checks the parsed value and falls back to the legacy parse otherwise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
skearnes
added a commit
that referenced
this pull request
Jun 28, 2026
- nl_query: build_query_params now emits JSON ComponentSpec strings (matching the new /query encoding from #206) instead of "smiles;target;mode". - Dev mode: GET /api/nl_query?dry_run=true translates and resolves but skips the search, returning the would-be-executed query_components for inspection. The /ask page gets a URL-persisted "Dry run" toggle that shows the structured query instead of results. - Add an "in development" banner to the /ask page. - Remove the "Ask" link from the nav bar; the /ask route still resolves directly. - Apply ruff format to the eval test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
skearnes
added a commit
that referenced
this pull request
Jun 28, 2026
* Add natural-language query interface (/ask)
Translate a chemist's free-text question (e.g. "reactions using benzene as an
input with yield greater than 70%") into the existing structured QueryParams via
a single forced Claude tool call, resolve compound names to SMILES, and dispatch
through the existing index-accelerated search path. The model emits a structured
query, never SQL or invented SMILES; name resolution is grounded in PubChem/OPSIN.
Backend (ord_interface/api/nl_query.py):
- translate(): forced build_query tool call -> NLQuery; RateLimitError -> 429,
other anthropic.APIError -> 503, no tool call -> 502.
- Async, cached name resolution (SMARTS passthrough, verbatim SMILES, else
resolve_name); blocking lookups run in a thread, failures are not cached.
- Redis caches (best-effort): identical questions (1h) and name->SMILES (30d),
so repeated questions skip the model call and shared compounds skip PubChem.
- System prompt lives in nl_query_prompt.md, shipped via package-data.
- GET /api/nl_query returns the interpretation and resolved structures alongside
results for transparency.
Eval: nl_query_eval.py + nl_query_eval_cases.json (10 cases); --search executes
against the DB to flag zero-result translations. Translation accuracy 10/10.
Frontend: a separate /ask page (MainNLSearch) with an "Interpreted as:" panel,
useNLQuery hook, types, route, and nav link.
Tests: nl_query_test.py and nl_query_eval_test.py (18 tests, fully stubbed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Address review: translation-only cache, fast-fail Redis, query guards
Cache only the model's translation (NLQuery), not the search results: repeated
identical questions still skip the model call, but the DB query is always re-run
so results stay fresh and Redis entries stay small (Greptile P2).
Other review fixes and hardening:
- Guard the empty-interpretation case: run_query's "no parameters" ValueError now
maps to 422 instead of an unhandled 500 (Greptile P1).
- Bound the q parameter with Query(min_length=1, max_length=2000) (Greptile P2).
- Resolve components concurrently with asyncio.gather (Greptile P2).
- Best-effort Redis ops fast-fail via asyncio.timeout (REDIS_OP_TIMEOUT_SECONDS):
a missing/slow Redis degrades to a miss in ~1s instead of stalling each request
~10-20s. Measured against the live DB without Redis: per-compound resolve
dropped from ~21s to ~2s.
- Bump ord-schema to >=0.7.1 for the new NCI/CADD CIR resolver; resolve_name now
cascades PubChem -> CIR -> OPSIN, so a PubChem 503 falls through to CIR (which
resolves benzene/aspirin/ibuprofen/palladium acetate that OPSIN cannot).
Eval: add a per-phase time breakdown (translate/resolve/search) and a per-search
asyncio timeout so a pathologically slow query is reported, not hung.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Move /ask frontend to a follow-up branch
The natural-language web UI (the /ask page, useNLQuery hook, types, route, and
nav link) is split out to the nl-query-frontend branch so this PR is backend-only
(the /api/nl_query endpoint and its translation/resolution/caching/eval). The
frontend follow-up depends on this endpoint and lands separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Eval: exact identifier match, YAML cases, SMILES coverage
- Match component identifiers exactly (case- and whitespace-insensitive) instead
of by substring, so the model must reproduce the specific compound -- e.g.
"4-aminophenol", not "aminophenol". This surfaced a real prompt bug: the model
emitted "pyridine ring" for "a pyridine ring"; the prompt now instructs it to
strip descriptive words ("ring"/"group"/"moiety"/"scaffold") and to pass a
user-supplied SMILES/SMARTS through verbatim.
- Move eval cases from JSON to YAML (nl_query_eval_cases.yaml); add pyyaml as a
dependency and ship *.yaml via package-data.
- Add SMILES-identifier cases (CCO, an aspirin SMILES, a C(=O)O substructure).
- Reformat the system prompt into sections (Components / Match mode / Filters /
Output).
Translation accuracy: 13/13 with Haiku.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Eval: infer SUBSTRUCTURE from "contain" without the literal word
Drop "substructure" from the C(=O)O case so it reads "products contain C(=O)O";
verifies the model maps "contain" to SUBSTRUCTURE mode on its own. Still 13/13.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Restore /ask frontend into this PR
Recombine the natural-language web UI (the /ask page, useNLQuery hook, types,
route, and nav link) with the backend so the feature can be reviewed and iterated
on as a single full-stack change. Reverses the earlier split to the
nl-query-frontend branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Eval: complete the YAML/exact-match switch (load yaml, prompt, pyyaml)
These changes belong with the earlier eval commit but were dropped when its
git add aborted on the already-removed JSON pathspec, leaving the branch in a
broken state (JSON deleted while nl_query_eval still loaded it):
- nl_query_eval.py: load cases from YAML; exact (case/whitespace-insensitive)
identifier matching via the renamed `identifier` field.
- nl_query_eval_test.py: assert exact matching ("aminophenol" != "4-aminophenol").
- nl_query_prompt.md: sectioned format; strip descriptive words from identifiers
and pass user SMILES/SMARTS through verbatim.
- pyproject.toml / uv.lock: add pyyaml; ship *.yaml via package-data.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix component-spec parsing for SMARTS containing ";"
The component spec is "pattern;target;mode", but SMARTS uses ";" as a
low-precedence AND, so a model- or user-supplied SMARTS like "[#6;R]" broke the
str.split(";") unpacking and surfaced as an unhandled 500. Split from the right
(rsplit(";", 2)) since target and mode are the trailing fields and never contain
";". Adds a regression test.
(Flagged by Greptile on #203; their suggested split(";", 2) would mis-parse a
leading-";" SMARTS, so rsplit is the correct form.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Emit JSON component specs; add /ask dev mode, in-dev banner, unlist nav
- nl_query: build_query_params now emits JSON ComponentSpec strings (matching the
new /query encoding from #206) instead of "smiles;target;mode".
- Dev mode: GET /api/nl_query?dry_run=true translates and resolves but skips the
search, returning the would-be-executed query_components for inspection. The
/ask page gets a URL-persisted "Dry run" toggle that shows the structured query
instead of results.
- Add an "in development" banner to the /ask page.
- Remove the "Ask" link from the nav bar; the /ask route still resolves directly.
- Apply ruff format to the eval test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Route compound classes to SMARTS/SUBSTRUCTURE; clarify interpretation UI
Functional-group and element classes ("brominated products", "an aryl
boronic acid") were translated as exact name lookups and failed in the
resolver, which only handles specific named compounds. Teach the prompt to
express such classes as a model-authored SMARTS or SUBSTRUCTURE pattern
instead, and never send them for resolution. SUBSTRUCTURE fragments are now
SMILES the model writes directly (e.g. a pyridine ring -> c1ccncc1).
Validate model-authored SMARTS with RDKit in _resolve_component so a bad
pattern is a clean 422 with the pattern echoed, rather than a 400 deep in
query execution that a dry run would skip entirely.
Eval matching is now structure-aware: SMARTS and SMILES identifiers are
canonicalized with RDKit so equivalent patterns compare equal, while names
RDKit cannot parse keep the case- and whitespace-insensitive string compare.
The /ask interpretation box now separates provenance: a "From the model"
section (the build_query tool call, with the raw JSON available) and a
"Resolved to structures" section that lists only identifiers a network
resolver actually handled, headed by the resolvers used. The redundant
dry-run query dump is dropped since the box already shows it.
Bump the translation cache version to v3 for the prompt change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Map malformed model/cached payloads to 502/miss, not 500
NLQuery.model_validate in translate() and model_validate_json in
_translation_cache_get can raise pydantic.ValidationError. Catch it
explicitly: a tool payload that fails schema validation becomes a 502
(consistent with the no-tool-call case), and a schema-mismatched cache entry
degrades to a miss. ValidationError subclasses ValueError in our Pydantic, so
the cache path was already covered, but the translate() path was unwrapped
entirely and the explicit catch is robust to Pydantic's inheritance quirk.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Validate reaction SMARTS up front; resync /ask input on navigation
Validate reaction_smarts with RDKit in build_query_params so a bad pattern
is a clear 422 in both normal and dry-run mode, instead of a misleading "no
constraints" 422 from run_query (normal) or an unvalidated pass-through (dry
run, which skips run_query). ReactionFromSmarts both returns None and raises
ValueError for different malformed inputs, so handle both.
On the /ask page, sync the text input to the ?q= URL parameter via an effect
so browser back/forward navigation updates the visible query rather than
leaving a stale value.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* eval: fix stale cases filename and cover all numeric filters
The harness docstring referenced nl_query_eval_cases.json; the cases are YAML.
Add similarity_threshold and limit to the over-extraction check (and to
CaseExpectation) so the model is flagged for extracting either without the
question asking.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Tighten verbose comments in nl_query
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
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
1 participant
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.
The
componentquery parameter encoded each predicate as a"pattern;target;mode"string. That delimiter collides with SMARTS, which uses;as a low-precedence AND — a pattern like[#6;R]mis-parsed and surfaced as an unhandled 500. (A right-split was a stop-gap; JSON removes the ambiguity entirely.)Changes
search.py: add aComponentSpecPydantic model (pattern,target,mode); parse eachcomponentvalue viaComponentSpec.parse(...). New values are JSON objects; the legacy;-delimited form is still accepted so previously shared search URLs keep working (right-split, since a SMARTS pattern may contain;). A spec that is neither JSON nor a 3-field legacy string returns a controlled 400 instead of a 500.JSON.stringify({pattern, target, mode})(MainSearch.tsx); decode with aparseComponent()helper (SearchOptions.tsx) that tries JSON and falls back to the legacy parse instead of throwing onJSON.parse. Also drops the dead%3Dun-escaping hack —URLSearchParamsalready decodes=.QUERY_PARAMSbuild specs via a_spec()helper (JSON), plus regression tests for a;-containing SMARTS, a legacy-format URL (backward compat), and an invalid spec → 400.Search stays a shareable GET request; only the per-component encoding changed (now a JSON object string), and old URLs continue to resolve.
Verification
search_test.py39 pass;ruff check,ruff format --check, andtyclean.tsc -bandeslintclean. (URL round-trip not exercised end-to-end against a running app — worth a quick manual smoke test of the /search page, including an old;-format URL.)Coordination
mainhas nonl_query.py, so this PR leaves the natural-language path (#203) alone. #203 builds the same predicate string; once this merges and #203 rebases,build_query_paramsgets a one-line change to emit a JSONComponentSpec. Flagging so the merge order is intentional.🤖 Generated with Claude Code
Greptile Summary
This PR changes component search predicates to use JSON query values. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Map all malformed component specs to 400..." | Re-trigger Greptile