From 6c5d6d9756fe966c5584d33697ce879de014b35f Mon Sep 17 00:00:00 2001 From: bloosqr Date: Mon, 5 Oct 2026 17:38:35 -0700 Subject: [PATCH 1/2] route check: a picture is not a declaration, and an unbuilt step says so Three reporting faults, all found by running a long route and reading what came back. A model may draw its own route through a capability fence and hand-write the SVG inside it. The species parser read that drawing. One role label inside a picture that was cut off before its closing tag claimed every character to the end of the answer, and two fragments of markup became species of that step; nothing resolves them, and a step with an unresolved species is emptied by design, so the step was reported as impossible to build although the author's own list was complete. It also cost a correction round, because the fragments were handed back as species to repair. maskDrawnRegions blanks any capability fence and any raw with equal-length whitespace, so offsets into the answer still address the original text, and an unclosed picture is blanked to the end of its fence. isNameLikeSpecies drops a parsed name that is markup, not a name. An unbuilt step was reported as a failed check. That overstates the route's problems and hides what the author has to fix, because nothing was checked on that step at all: it is now its own state, counted separately in the verdict, and it names the species whose structure could not be resolved. The application already had those names; only the step line omitted them. A chiral building block of the opposite configuration has the same formula, the same atom counts and the same constitution as the intended one, so balance and continuity both pass and no deterministic check can see the difference. Where the package measures the configuration at a free acid's nitrogen-bearing stereocentre, the route check now reports it beside what the author's own name asserts. Reported, never judged: which letter belongs to a given series flips when a sulfur-bearing branch outranks the carboxyl, so the letter alone proves nothing. The field is optional, so an older package simply produces no such line. Finally, the answer now says where each structure came from -- the built-in dictionary, a reference service, or the answer itself. The per-species source was already carried on every resolved species and never surfaced, so a run could not tell an offline dictionary hit from a network lookup or from a structure the model drew itself, which is the category a wrong structure hides in. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LVYxvTRGs6MrP7LsQaAJKz --- electron/ai/moleculeInspection.ts | 11 +- electron/ai/researchAssistant.ts | 7 +- scripts/test-molecule-inspection.mjs | 89 +++++++++++++++- shared/moleculeInspection.ts | 150 +++++++++++++++++++++++++-- 4 files changed, 244 insertions(+), 13 deletions(-) diff --git a/electron/ai/moleculeInspection.ts b/electron/ai/moleculeInspection.ts index a9fdd6601..9a5f6164c 100644 --- a/electron/ai/moleculeInspection.ts +++ b/electron/ai/moleculeInspection.ts @@ -346,6 +346,9 @@ export interface RouteResolutionOutcome { /** Species the model supplied as structures because no name would resolve, as prose, so the * caller discloses that their structure came from the model, not a reference. */ authorStructures: string[]; + /** Every species' resolution status and source, so a run can record where each structure came + * from. The author-supplied ones are the category silent wrongness hides in. */ + resolutionSources: Array<{ status: string; source?: string }>; /** True when the installed package has no resolve-names tool, so the caller falls back to * the legacy reaction-line path. */ legacy: boolean; @@ -505,7 +508,7 @@ export async function resolveNamedRoute( modelAnswer: string, options: InspectOptions = {}, ): Promise { - const legacy: RouteResolutionOutcome = { answer: finalAnswer, steps: [], labels: [], consistent: true, corrections: [], authorStructures: [], legacy: true }; + const legacy: RouteResolutionOutcome = { answer: finalAnswer, steps: [], labels: [], consistent: true, corrections: [], resolutionSources: [], authorStructures: [], legacy: true }; if (options.enabled === false) return legacy; const provider = resolveProvider(); if (!provider) return legacy; @@ -597,6 +600,7 @@ export async function resolveNamedRoute( const steps = buildRouteSteps(resolvedByStep); const labels: RouteSpeciesLabel[][] = resolvedByStep.map((step) => step.filter((entry) => entry.smiles).map((entry) => ({ role: entry.role, byproduct: entry.byproduct, name: entry.name, smiles: entry.smiles! }))); const authorStructures = resolvedByStep.flat().filter((entry) => entry.status === 'fallback' && entry.smiles).map((entry) => `${entry.name} — \`${entry.smiles}\``); + const resolutionSources = resolvedByStep.flat().map((entry) => ({ status: entry.status, ...(entry.source ? { source: entry.source } : {}) })); const annotated = `${annotateSpeciesSmiles(finalAnswer, resolvedByStep).trimEnd()}\n`; return { answer: annotated, @@ -605,6 +609,7 @@ export async function resolveNamedRoute( consistent: critical.length === 0, corrections, authorStructures, + resolutionSources, ...(critical.length ? { clarification: formatUnresolvedNameClarification(critical, options.target), unresolved: critical } : {}), legacy: false, }; @@ -888,7 +893,7 @@ export async function appendRouteReportAndDrawings( // Paint the deterministic report and drawings before the reviewer returns. The transport // replaces the provisional stream with this returned answer, so the route only waits on // the reviewer when the reviewer is the last thing outstanding. - if (options.onDeterministic) options.onDeterministic(`${finalAnswer.trimEnd()}\n\n${formatRouteAudit(audit, labels, null, true)}\n${drawings}`); + if (options.onDeterministic) options.onDeterministic(`${finalAnswer.trimEnd()}\n\n${formatRouteAudit(audit, labels, null, true, overrides.unresolved ?? [])}\n${drawings}`); // The lookup already carries its drawings; this only formats. Best-effort throughout. The // step support (textbook passage per reaction class, ORD alternatives for a failed or // unprecedented step) needs the lookup's classes, so it follows it, still beside the review. @@ -912,7 +917,7 @@ export async function appendRouteReportAndDrawings( // Which starting materials the user's vendor stock lists hold (no lists: nothing is said). const stockPromise = options.evidenceScope?.external === false ? Promise.resolve('') : startingMaterialStockLine(runner, labels).catch(() => ''); const review = await reviewPromise; - const report = formatRouteAudit(audit, labels, review); + const report = formatRouteAudit(audit, labels, review, false, overrides.unresolved ?? []); const precedentText = await precedentSection; const support = await supportPromise; // A step's high-severity clashes ride along in its fix prompt, as evidence. diff --git a/electron/ai/researchAssistant.ts b/electron/ai/researchAssistant.ts index 4edd82706..589492b94 100644 --- a/electron/ai/researchAssistant.ts +++ b/electron/ai/researchAssistant.ts @@ -20,7 +20,7 @@ import { ResearchCorpusRun } from './researchCorpusRun'; import { RESEARCH_CHAT_AGENT_DECISION_BYTES, RESEARCH_CHAT_AGENT_SETTINGS, RESEARCH_CHAT_LIGHT_AGENT_SETTINGS, researchScopeForPrompt, validateRetrievalSettings, compactResearchTraversal } from '@shared/researchCorpus'; import { planResearchTurn, literalResearchTurnPlan } from './researchTurnPlanner'; import { inspectResearchMolecules, appendStructureAudit, appendRouteReportAndDrawings, resolveNamedRoute, chemistryRunner } from './moleculeInspection'; -import { countRouteSteps, findStepNamedSpecies, formatAuthorStructureNote, formatMissingSpeciesPrompt, formatNameCorrectionNote, formatRouteCheckUnavailable, isRouteFixPrompt, MOLECULE_DOSSIER_SYSTEM_RULE, ROUTE_CONTINUITY_SYSTEM_RULE, requestedTargetFor, routeFixPromptForHistory, routeReportsForHistory } from '@shared/moleculeInspection'; +import { countRouteSteps, findStepNamedSpecies, formatAuthorStructureNote, formatResolutionSourceNote, formatMissingSpeciesPrompt, formatNameCorrectionNote, formatRouteCheckUnavailable, isRouteFixPrompt, MOLECULE_DOSSIER_SYSTEM_RULE, ROUTE_CONTINUITY_SYSTEM_RULE, requestedTargetFor, routeFixPromptForHistory, routeReportsForHistory } from '@shared/moleculeInspection'; import { SYNTHESIS_TEMPLATE_ADDENDUM, looksLikeSynthesisRequest } from '@shared/synthesisPrompt'; import { reviseRouteWithEvidence, revisionUserMessage, routeEvidencePassEnabled } from './routeEvidencePass'; import { SYNTHESIS_EVIDENCE_KEY, SYNTHESIS_EVIDENCE_SYSTEM_RULE, synthesisEvidencePayload, synthesisRetrievalQuery } from '@shared/synthesisEvidence'; @@ -288,7 +288,10 @@ async function auditAnswer(answer: string, execution: ReturnType { const strict = normalizeRouteAudit({ ...audit, steps: [{ ...audit.steps[0], stereoNotRequired: false }] }); assert.match(routeStepFailure(strict.steps[0]) ?? '', /2 unspecified stereocentre/); }); + +test('a route the model draws in a capability fence does not turn its own labels into species', () => { + // Seen on a long route: the model emitted its own picture through the `nodus-view` + // fence and hand-wrote the SVG inside it, repeating the role labels in its `` elements, + // and the drawing ran out before ``. One `Byproducts:` inside the picture claimed the + // rest of the answer, and two fragments of markup became species of that step that no resolver + // could turn into structures — so the step was emptied and reported as unbuilt although the + // author's own list was complete. + const answer = [ + '**Step 1 — Coupling**', + 'Reactants: ethanol; ethanoic acid', + 'Products: ethyl ethanoate', + 'Byproducts: water', + 'Agents: sulfuric acid', + '', + '**Step 2 — Hydrolysis**', + 'Reactants: ethyl ethanoate; water', + 'Products: ethanol', + 'Byproducts: ethanoic acid', + 'Agents: none', + '', + '```nodus-view', + '', + ' Step 1 — Coupling', + ' Byproducts: water, carbon dioxide, etc.', + ' Product: ethyl ethanoate, purified.', + '```', + '', + 'Caveats: the conditions are a proposal.', + ].join('\n'); + assert.equal(countRouteSteps(answer), 2, 'the picture does not add a step'); + const species = findStepNamedSpecies(answer, 2); + assert.deepEqual(species[0].map((entry) => entry.name), ['ethanol', 'ethanoic acid', 'ethyl ethanoate', 'water', 'sulfuric acid']); + assert.deepEqual(species[1].map((entry) => entry.name), ['ethyl ethanoate', 'water', 'ethanol', 'ethanoic acid'], + 'no fragment of the drawing is read as a species'); + // Every species still resolves, so both steps are built rather than reported unbuilt. + const steps = buildRouteSteps([ + species[0].map((entry) => ({ role: entry.role, smiles: 'CCO' })), + species[1].map((entry) => ({ role: entry.role, smiles: 'CCO' })), + ]); + assert.ok(steps.every((step) => step.length > 0), 'a step is not emptied by the drawing'); +}); + +test('a closed inline SVG is masked too, and a species name that is markup is dropped', () => { + const answer = [ + '**Step 1 — Oxidation**', + 'Reactants: cyclohexanol', + 'Products: cyclohexanone', + 'Byproducts: water; Product: something', + 'Agents: none', + 'Reactants: benzene', + ].join('\n'); + const species = findStepNamedSpecies(answer, 1); + assert.deepEqual(species[0].map((entry) => entry.name), ['cyclohexanol', 'cyclohexanone', 'water'], + 'markup is not a species name, and the closed picture contributes nothing'); +}); + +test('a step the application could not build is reported as UNBUILT and names the species', () => { + // Nothing was checked on such a step, so calling it a failed check both overstates the route's + // problems and hides what the author has to fix. The old line read + // "Step 2 FAIL — This step could not be built: a species it names has no resolved structure." + // and named nothing, which made a real diagnosis slow. + const audit = normalizeRouteAudit({ + steps: [ + { index: 0, reaction: 'CCO>>CC=O', ok: true, reactants: [], agents: [], products: [], balanced: true, chargeBalanced: true, differences: [], unspecifiedStereocentres: 0 }, + { index: 1, reaction: '', ok: false, error: 'This step could not be built: a species it names has no resolved structure.', reactants: [], agents: [], products: [], balanced: null, chargeBalanced: null, differences: [], unspecifiedStereocentres: 0 }, + ], + }); + const report = formatRouteAudit(audit, [[], []], null, false, [ + { step: 2, role: 'reactant', byproduct: false, name: 'the supported intermediate' }, + ]); + assert.match(report, /- Step 2 UNBUILT — nothing was checked: no structure resolved for reactant "the supported intermediate"/); + assert.doesNotMatch(report, /Step 2 FAIL/, 'an unbuilt step is not reported as a failed check'); + assert.match(report, /1 step\(s\) could not be built because a species they name has no resolved structure \(step 2\)/); + assert.doesNotMatch(report, /1 of 2 step\(s\) do not pass/, 'it is not counted among the steps that do not pass'); +}); + +test('the resolution source of every structure is reported', () => { + assert.equal(formatResolutionSourceNote([ + { status: 'resolved', source: 'builtin' }, { status: 'resolved', source: 'builtin' }, + { status: 'resolved', source: 'pubchem' }, { status: 'resolved', source: 'opsin' }, + { status: 'fallback', source: 'declared' }, + { status: 'unresolved' }, + ]), 'Structures resolved: 2 from the built-in dictionary · 1 from PubChem · 1 from OPSIN · 1 from the answer itself.'); + assert.equal(formatResolutionSourceNote([]), '', 'nothing resolved is no note'); + assert.equal(formatResolutionSourceNote([{ status: 'unresolved' }]), '', 'an unresolved species is not a source'); +}); diff --git a/shared/moleculeInspection.ts b/shared/moleculeInspection.ts index b67d1d6ff..b22bd0a71 100644 --- a/shared/moleculeInspection.ts +++ b/shared/moleculeInspection.ts @@ -67,6 +67,13 @@ export interface RouteSpeciesSummary { heavyAtoms: number; stereocentres: number; unspecifiedStereocentres: number; + /** For a species written as a free acid with a stereocentre carrying a nitrogen — a chiral + * building block — the CIP descriptor at that centre, as the package measured it. Reported, + * not judged: a block of the opposite configuration parses and balances exactly like the + * intended one, so the atom check can never see it, and the letter that corresponds to a + * given series flips when a sulfur-bearing branch outranks the carboxyl. The report puts the + * measurement beside what the author's own name asserts. */ + alphaConfiguration?: '(R)' | '(S)' | 'unassigned'; /** The systematic name the author wrote for this species, when the answer carries one. */ name?: string; /** Set by the capability when it could resolve the name: true when the name denotes this @@ -710,6 +717,48 @@ function cleanSpeciesName(raw: string): string { return raw.replace(/^[\s>*_`:：-]+/, '').replace(/[\s*_`]+$/, '').replace(/\s+/g, ' ').trim().slice(0, 200); } +/** Whether a parsed species name can be a chemical name at all. A model that draws its route + * as an inline SVG writes the role labels into the picture too ("Byproducts: isobutylene, CO2…" + * inside a `` element), and those were read as species: they cannot resolve, so the step + * they land on is emptied and reported as unbuilt even though the author's own list was + * complete. Markup is not a name. */ +function isNameLikeSpecies(name: string): boolean { + return !/[<>{}\\"\n\r\t|]/.test(name) && !name.includes('→'); +} + +/** The answer with the blocks the interface renders specially blanked, the same length, so + * offsets into it still address the original text. A species list the author wrote in prose, in + * an ordinary code fence or in a table is untouched. + * + * A model may emit its own picture through a capability fence (`nodus-view`, which + * splitChatVisuals classifies as a view, not prose) and hand-write the SVG inside it. Seen on a + * solid-phase route: the drawing repeated the role labels in its `` elements and ran out + * before ``, so one `Byproducts:` inside the picture claimed every character to the end of + * the answer and two fragments of markup became species of that step. They resolve to nothing, + * so the step was emptied and reported as unbuilt although the author's own list was complete. + * A picture of a route is not a declaration of one. */ +function maskDrawnRegions(text: string): string { + const blank = (value: string, from: number, to: number) => + value.slice(0, from) + ' '.repeat(Math.max(0, to - from)) + value.slice(to); + let out = text; + // A capability fence carries a structured payload, not a species list. An unclosed one runs to + // the end of the answer, which is what a truncated drawing leaves behind. + for (const open of [...text.matchAll(/```nodus-[A-Za-z0-9_-]*/g)].reverse()) { + const from = open.index ?? 0; + const close = out.indexOf('```', from + open[0].length); + out = blank(out, from, close >= 0 ? close + 3 : out.length); + } + // Raw markup outside a fence, with the same allowance for a drawing that was cut off. + for (const open of [...out.matchAll(//i.exec(out.slice(from)); + const fence = out.indexOf('```', from); + const to = close ? from + (close.index ?? 0) + close[0].length : fence >= 0 ? fence : out.length; + out = blank(out, from, to); + } + return out; +} + interface RoleSegment { role: RouteLabelRole; byproduct: boolean; start: number; end: number } /** Every labelled segment, in document order, with the span of text that belongs to it. */ @@ -845,11 +894,11 @@ function parseRoleEntries(fragment: string): RoleEntry[] { const pair = /^(.+?)\s*[—–]\s*`([^`]+)`/.exec(entry); if (pair) { const name = cleanSpeciesName(pair[1]); - if (name) out.push({ name, declaredSmiles: pair[2].trim(), start: span.start, end: span.end }); + if (name && isNameLikeSpecies(name)) out.push({ name, declaredSmiles: pair[2].trim(), start: span.start, end: span.end }); continue; } const name = cleanSpeciesName(entry.replace(/[—–]?\s*`[^`]*`/g, '').replace(/[*_`]/g, '').replace(/[.,;:\s]+$/, '')); - if (!name || /^(?:none|no|n\/a|nil)\b/i.test(name) || /^[—–-]+$/.test(name)) continue; + if (!name || !isNameLikeSpecies(name) || /^(?:none|no|n\/a|nil)\b/i.test(name) || /^[—–-]+$/.test(name)) continue; out.push({ name, start: span.start, end: span.end }); } return out; @@ -860,7 +909,8 @@ interface NamedSegment { step: number; role: RouteLabelRole; byproduct: boolean; /** Every named role segment, assigned to its step. In the name-first path only the plural * labels count; a non-step section (an "Alternative…") is skipped; without headings the role * cycle splits the steps. */ -function namedSegments(text: string, count: number): NamedSegment[] { +function namedSegments(answer: string, count: number): NamedSegment[] { + const text = maskDrawnRegions(answer); const segments = roleSegments(text, NAME_ROLE_MARKER); if (!segments.length) return []; const headings = sectionHeadings(text); @@ -892,7 +942,8 @@ function namedSegments(text: string, count: number): NamedSegment[] { * carry species, or the role cycle (a new step begins at a `Reactants:` that follows a * product) when there are no headings. A prose summary heading with no species under it does * not count, so a route written twice is still one route. */ -export function countRouteSteps(text: string): number { +export function countRouteSteps(answer: string): number { + const text = maskDrawnRegions(answer); const segments = roleSegments(text, NAME_ROLE_MARKER); const headings = sectionHeadings(text); const stepHeadings = headings.filter((heading) => heading.step); @@ -1209,6 +1260,8 @@ function normalizeRouteSpecies(entry: unknown): RouteSpeciesSummary | null { heavyAtoms: numberOr(value.heavyAtoms, 0), stereocentres: numberOr(value.stereocentres, 0), unspecifiedStereocentres: numberOr(value.unspecifiedStereocentres, 0), + ...(value.alphaConfiguration === '(R)' || value.alphaConfiguration === '(S)' || value.alphaConfiguration === 'unassigned' + ? { alphaConfiguration: value.alphaConfiguration } : {}), ...(typeof value.name === 'string' && value.name.trim() ? { name: value.name.trim().slice(0, 200) } : {}), ...(typeof value.nameOk === 'boolean' ? { nameOk: value.nameOk } : {}), ...(value.byproduct === true ? { byproduct: true } : {}), @@ -1717,7 +1770,52 @@ const LARGE_COEFFICIENT = 6; /** `reviewPending` is set for the interim repaint shown while the model review still runs: a * route whose checks pass is then reported as passing so far, not as verified. */ -export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][] = [], review: RouteReview | null = null, reviewPending = false): string { +/** What a species name asserts about configuration, when it says anything: an L-/D- prefix, or an + * explicit CIP descriptor. Only what the author wrote — no mapping is applied, because L maps to + * one letter in most of the series and the other when a sulfur-bearing branch outranks the + * carboxyl, so no mapping is applied here. */ +export function statedConfiguration(name: string | undefined): string | null { + if (!name) return null; + if (/\(2?R\)/.test(name)) return '(R)'; + if (/\(2?S\)/.test(name)) return '(S)'; + if (/\bD-|\bD\b(?=[- ])|-D-/.test(name)) return 'D'; + if (/\bL-|\bL\b(?=[- ])|-L-/.test(name)) return 'L'; + return null; +} + +/** The measured configuration of each chiral building block a step consumes, beside what its + * own name asserts. Reported, never a verdict: this is the one error class the deterministic checks + * cannot see, because the wrong enantiomer has the same formula, the same atom counts and the + * same canonical constitution as the right one. Where the name and the structure both state a + * CIP descriptor the two are compared directly, which needs no mapping; an L-/D- prefix is + * printed as-is for the reader to weigh. */ +function alphaConfigurationLine(step: RouteStepAudit, stepLabels: RouteSpeciesLabel[]): string | null { + const blocks = step.reactants.filter((entry) => entry.alphaConfiguration); + if (!blocks.length) return null; + const nameFor = (entry: RouteSpeciesSummary, index: number): string | undefined => + entry.name ?? stepLabels.filter((label) => label.role === 'reactant')[index]?.name; + const parts = blocks.map((entry) => { + const name = nameFor(entry, step.reactants.indexOf(entry)); + const measured = entry.alphaConfiguration!; + const stated = statedConfiguration(name); + const note = !stated ? '' + : (stated === '(R)' || stated === '(S)') + ? stated === measured ? ', name agrees' : `, NAME SAYS ${stated}` + : `, name says ${stated}`; + return `${name ?? entry.formula} ${measured}${note}`; + }); + const counts = new Map(); + for (const entry of blocks) counts.set(entry.alphaConfiguration!, (counts.get(entry.alphaConfiguration!) ?? 0) + 1); + const tally = [...counts].sort().map(([key, count]) => `${count} ${key}`).join(', '); + return ` Building blocks, alpha configuration as measured (${tally}) — a block of the wrong configuration balances exactly like the right one: ${parts.join(' · ')}`; +} + +/** How the package reports a step it could not build, because a species the step names has no + * resolved structure. It is not a verdict on the chemistry: nothing was checked. Matched by + * prefix so the report can say UNBUILT and name the species instead of printing FAIL. */ +export const UNBUILT_STEP_ERROR_PREFIX = 'This step could not be built'; + +export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][] = [], review: RouteReview | null = null, reviewPending = false, unresolved: UnresolvedName[] = []): string { const names = routeLabelNames(labels); const lines: string[] = [ '### Route check (RDKit)', @@ -1725,13 +1823,18 @@ export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][ 'Every step was parsed with RDKit and every equation and intermediate link was checked. This block is generated by the application, not by the model.', '', ]; - const failing = (step: RouteStepAudit): boolean => routeStepFailure(step) !== null; + const unbuilt = (step: RouteStepAudit): boolean => !step.ok && Boolean(step.error?.startsWith(UNBUILT_STEP_ERROR_PREFIX)); + const failing = (step: RouteStepAudit): boolean => routeStepFailure(step) !== null && !unbuilt(step); const failedSteps = audit.steps.filter(failing).map((step) => step.index + 1); + const unbuiltSteps = audit.steps.filter(unbuilt).map((step) => step.index + 1); const assembled = audit.steps.filter((step) => Boolean(step.assemblyProblem)).map((step) => step.index + 1); const isolated = isolatedSteps(audit); const reviewProblems = blockingReviewProblems(review); const reasons: string[] = []; if (failedSteps.length) reasons.push(`${failedSteps.length} of ${audit.steps.length} step(s) do not pass (${failedSteps.map((index) => `step ${index}`).join(', ')})`); + // Said separately: an unbuilt step had nothing checked, so counting it as a failed check makes + // a route look worse than it is and hides what the author actually has to fix. + if (unbuiltSteps.length) reasons.push(`${unbuiltSteps.length} step(s) could not be built because a species they name has no resolved structure (${unbuiltSteps.map((index) => `step ${index}`).join(', ')})`); if (assembled.length) reasons.push(`${assembled.length === 1 ? 'a step' : 'steps'} cannot be assembled from a single substrate molecule (${assembled.map((index) => `step ${index}`).join(', ')})`); if (isolated.length) reasons.push(`${isolated.length} step(s) are disconnected from the rest of the route`); if (audit.target?.reason === 'not-formed') reasons.push('no step forms the requested target'); @@ -1745,7 +1848,16 @@ export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][ : `**Route check failed** — ${reasons.join('; ')}.`); for (const step of audit.steps) { const label = `Step ${step.index + 1}`; - if (!step.ok) { lines.push(`- ${label} FAIL — ${step.error ?? 'could not be parsed'}`); continue; } + if (!step.ok) { + if (unbuilt(step)) { + const named = unresolved.filter((entry) => entry.step === step.index + 1) + .map((entry) => `${entry.byproduct ? 'byproduct' : entry.role} "${entry.name}"`); + lines.push(`- ${label} UNBUILT — nothing was checked: ${named.length ? `no structure resolved for ${named.join(', ')}` : 'a species it names has no resolved structure'}. Give that species a name a reference resolves, or its structure.`); + continue; + } + lines.push(`- ${label} FAIL — ${step.error ?? 'could not be parsed'}`); + continue; + } const racemic = step.racemic === true && step.unspecifiedStereocentres > 0; const moot = !racemic && step.stereoNotRequired === true && step.unspecifiedStereocentres > 0; const nameFailure = (step.nameProblems?.length ?? 0) > 0; @@ -1775,6 +1887,8 @@ export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][ const reactantSide = groupedSideTrace(step.reactants, stepLabels.filter((entry) => entry.role === 'reactant'), names, step.balanced === true, step.products); const productSide = groupedSideTrace(step.products, stepLabels.filter((entry) => entry.role === 'product'), names, step.balanced === true, step.reactants); lines.push(`- ${label} ${verdict} — ${balance}${stereo}.${nameNote}${largeNote}${assemblyNote} ${reactantSide}${agents} → ${productSide}`); + const alpha = alphaConfigurationLine(step, stepLabels); + if (alpha) lines.push(alpha); } if (audit.links.length) { lines.push('', 'Intermediate continuity:', ''); @@ -2167,6 +2281,28 @@ export function formatAuthorStructureNote(entries: string[]): string { return unique.length ? `Author-supplied structures (no reference name was available): ${unique.join('; ')}` : ''; } +/** Where each species' structure came from, as one line. The author-supplied ones are named + * individually by formatAuthorStructureNote, because that is the category a wrong structure + * hides in; the rest are counted, which is what a run needs recorded to compare with the next + * one. Without this a run cannot say whether a protected name was resolved offline, looked up, + * or taken from the model. */ +export function formatResolutionSourceNote(species: Array<{ status: string; source?: string }>): string { + const label: Record = { + builtin: 'the built-in dictionary', pubchem: 'PubChem', opsin: 'OPSIN', declared: 'the answer itself', + }; + const counts = new Map(); + for (const entry of species) { + if (entry.status === 'unresolved') continue; + const key = entry.source ?? 'unknown'; + counts.set(key, (counts.get(key) ?? 0) + 1); + } + if (!counts.size) return ''; + const order = ['builtin', 'pubchem', 'opsin', 'declared', 'unknown']; + const parts = [...counts].sort((a, b) => order.indexOf(a[0]) - order.indexOf(b[0])) + .map(([key, count]) => `${count} from ${label[key] ?? 'an unnamed resolver'}`); + return `Structures resolved: ${parts.join(' · ')}.`; +} + /** The escalation when a name cannot be resolved to a structure even after the feedback * loop: name the species and why, so the user can confirm or correct it. */ export function formatUnresolvedNameClarification(unresolved: UnresolvedName[], target?: string | null): string { From d744d8e570f4fca03b2db7e6d0fdf850de5b693f Mon Sep 17 00:00:00 2001 From: bloosqr Date: Mon, 5 Oct 2026 19:06:51 -0700 Subject: [PATCH 2/2] route check: every fragment reaches the equation, and a placeholder is named as one buildRouteSteps discarded a repeated fragment, so that two salts sharing an ion could not put the same token on one side twice. The set was per role, so calcium chloride written as its ions lost a chloride, and any salt with repeated counterions -- magnesium bromide, sodium sulfate, potassium carbonate -- could then never balance: the author would be told a correct equation was wrong. Writing each distinct ion once also left its count to the solver, and a free ion's coefficient is the solver's to choose, so a wrong equation could be rescued by it. groupedSideTrace already knew this and worked around it in the display -- "its solved count is arbitrary (often 1:1); the salts fix it instead" -- which meant the report could show the author's salts while the verdict had been computed on something else. Every fragment of every species is now written out, in order. The package regroups them from the labels it already receives, so each declared species takes one coefficient, the ratio the author wrote survives, and the verdict is computed on the species the report shows. A package that cannot regroup sees the duplicates and reports several possible equations, which is visible rather than silent. Separately: a species line that reads "see prose", "as above" or "see step 2" is a placeholder, not a name a resolver could ever turn into a structure. The step report and the correction prompt said "give that species a name a reference resolves, or its structure", which invites the author to invent a structure for something that has none. They now say what the author actually did and ask for the species to be listed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LVYxvTRGs6MrP7LsQaAJKz --- scripts/test-molecule-inspection.mjs | 69 ++++++++++++++++++++++++++-- shared/moleculeInspection.ts | 41 ++++++++++++++--- 2 files changed, 98 insertions(+), 12 deletions(-) diff --git a/scripts/test-molecule-inspection.mjs b/scripts/test-molecule-inspection.mjs index 9adf47999..1b753f5f6 100644 --- a/scripts/test-molecule-inspection.mjs +++ b/scripts/test-molecule-inspection.mjs @@ -8,7 +8,7 @@ import { pathToFileURL } from 'node:url'; const dir = await mkdtemp(path.join(os.tmpdir(), 'molecule-inspection-')); await build({ entryPoints: ['shared/moleculeInspection.ts'], outfile: path.join(dir, 'inspection.mjs'), bundle: true, platform: 'node', format: 'esm' }); -const { findSmilesCandidates, findAnswerSpecies, normalizeMoleculeDossier, formatMoleculeDossier, formatStructureAudit, MOLECULE_DOSSIER_SYSTEM_RULE, findStepConditions, declaresRacemic, stepDeclaresRacemic, normalizeRouteAudit, formatRouteAudit, ROUTE_CONTINUITY_SYSTEM_RULE, findRequestedTarget, requestedTargetFor, ROUTE_FIX_PROMPT_LEAD, parseRouteReview, buildRouteReviewRequest, ROUTE_REVIEW_SYSTEM, clampReviewDetail, findStepProse, routeLabelNames, countRouteSteps, findStepNamedSpecies, buildRouteSteps, annotateSpeciesSmiles, formatNameCorrectionNote, formatAuthorStructureNote, formatNamedRouteFixPrompts, formatMissingSpeciesPrompt, isRouteFixPrompt, parseNameFeedback, ROUTE_NAME_FEEDBACK_SYSTEM, formatUnresolvedNameClarification, routeReportsForHistory, classifyCoProducts, smilesHasCarbon, routeStepFailure, routeFixPromptForHistory, formatRouteCheckUnavailable, normalizeReactionPrecedent, formatReactionPrecedents, buildPrecedentQueries, similarityBand, precedentDrawingFor, formatResolutionSourceNote, UNBUILT_STEP_ERROR_PREFIX } = await import(pathToFileURL(path.join(dir, 'inspection.mjs'))); +const { findSmilesCandidates, findAnswerSpecies, normalizeMoleculeDossier, formatMoleculeDossier, formatStructureAudit, MOLECULE_DOSSIER_SYSTEM_RULE, findStepConditions, declaresRacemic, stepDeclaresRacemic, normalizeRouteAudit, formatRouteAudit, ROUTE_CONTINUITY_SYSTEM_RULE, findRequestedTarget, requestedTargetFor, ROUTE_FIX_PROMPT_LEAD, parseRouteReview, buildRouteReviewRequest, ROUTE_REVIEW_SYSTEM, clampReviewDetail, findStepProse, routeLabelNames, countRouteSteps, findStepNamedSpecies, buildRouteSteps, annotateSpeciesSmiles, formatNameCorrectionNote, formatAuthorStructureNote, formatNamedRouteFixPrompts, formatMissingSpeciesPrompt, isRouteFixPrompt, parseNameFeedback, ROUTE_NAME_FEEDBACK_SYSTEM, formatUnresolvedNameClarification, routeReportsForHistory, classifyCoProducts, smilesHasCarbon, routeStepFailure, routeFixPromptForHistory, formatRouteCheckUnavailable, normalizeReactionPrecedent, formatReactionPrecedents, buildPrecedentQueries, similarityBand, precedentDrawingFor, formatResolutionSourceNote, UNBUILT_STEP_ERROR_PREFIX, isPlaceholderSpecies } = await import(pathToFileURL(path.join(dir, 'inspection.mjs'))); await build({ entryPoints: ['shared/chatSkills.ts'], outfile: path.join(dir, 'chatSkills.mjs'), bundle: true, platform: 'node', format: 'esm' }); const { splitChatVisuals } = await import(pathToFileURL(path.join(dir, 'chatSkills.mjs'))); await build({ entryPoints: ['shared/synthesisPrompt.ts'], outfile: path.join(dir, 'synthesisPrompt.mjs'), bundle: true, platform: 'node', format: 'esm' }); @@ -693,16 +693,31 @@ test('precedent queries leave byproducts out, as the Open Reaction Database reco assert.deepEqual(buildPrecedentQueries(gap), [{ step: 1, query: 'CCO>>CC=O' }]); }); -test('an ion shared by two salts is written once per side so the balance is unique', () => { +test('two salts sharing an ion each keep their own stoichiometry', () => { + // This used to write each distinct ion once and leave the counts to the solver, so that a + // shared ion could not appear twice on one side. It read as tidy and it threw away what the + // author had actually declared: chromium(III) sulfate's own 2:3 ratio became one chromium and + // one sulfate, and the solver re-derived whatever numbers balanced. The same freedom let a + // wrong equation balance elsewhere -- a hydrolysis one hydrogen short came back balanced by + // taking two of one reagent -- because a free ion's coefficient is the solver's to choose. + // + // Now every fragment of every species is written out, and the package regroups them from the + // labels so each DECLARED species takes one coefficient. The ratio the author wrote survives. const sulfate = 'S(=O)(=O)([O-])[O-]'; + const chromiumSulfate = `${sulfate}.[Cr+3].${sulfate}.${sulfate}.[Cr+3]`; + const sodiumSulfate = `${sulfate}.[Na+].[Na+]`; const resolved = [[ { role: 'reactant', byproduct: false, name: 'sodium dichromate', status: 'resolved', smiles: '[O-][Cr](=O)(=O)O[Cr](=O)(=O)[O-].[Na+].[Na+]' }, { role: 'reactant', byproduct: false, name: 'cyclohexanol', status: 'resolved', smiles: 'C1CCC(CC1)O' }, - { role: 'product', byproduct: false, name: 'chromium(III) sulfate', status: 'resolved', smiles: `${sulfate}.[Cr+3].${sulfate}.${sulfate}.[Cr+3]` }, - { role: 'product', byproduct: false, name: 'sodium sulfate', status: 'resolved', smiles: `${sulfate}.[Na+].[Na+]` }, + { role: 'product', byproduct: false, name: 'chromium(III) sulfate', status: 'resolved', smiles: chromiumSulfate }, + { role: 'product', byproduct: false, name: 'sodium sulfate', status: 'resolved', smiles: sodiumSulfate }, ]]; const [step] = buildRouteSteps(resolved); - assert.deepEqual(step.split('>')[2].split('.'), [sulfate, '[Cr+3]', '[Na+]'], 'sulfate, chromium and sodium each appear once'); + assert.equal(step.split('>')[2], `${chromiumSulfate}.${sodiumSulfate}`, 'each salt contributes its own fragments, in order'); + const products = step.split('>')[2].split('.'); + assert.equal(products.filter((part) => part === sulfate).length, 4, 'three sulfates from one salt and one from the other'); + assert.equal(products.filter((part) => part === '[Cr+3]').length, 2); + assert.equal(products.filter((part) => part === '[Na+]').length, 2); }); test('the resolved SMILES is attached to the name in place, replacing any declared one', () => { @@ -1614,3 +1629,47 @@ test('the resolution source of every structure is reported', () => { assert.equal(formatResolutionSourceNote([]), '', 'nothing resolved is no note'); assert.equal(formatResolutionSourceNote([{ status: 'unresolved' }]), '', 'an unresolved species is not a source'); }); + +test('a placeholder where a species belongs is called a placeholder, not an unresolvable name', () => { + // Seen live: a step whose Byproducts line read "see prose". Asking for "its structure" invites + // the author to invent one; the fault is that the species were never listed. + for (const name of ['see prose', 'see prose (protected building blocks)', 'as above', 'see step 2', 'as described in the text', 'various', 'etc.']) { + assert.equal(isPlaceholderSpecies(name), true, name); + } + for (const name of ['water', 'sodium bromide', '9H-fluoren-9-ylidenemethanone', 'propan-2-ol', 'the supported intermediate', 'Seebach amide']) { + assert.equal(isPlaceholderSpecies(name), false, name); + } + const audit = normalizeRouteAudit({ + steps: [{ index: 0, reaction: '', ok: false, error: 'This step could not be built: a species it names has no resolved structure.', reactants: [], agents: [], products: [], balanced: null, chargeBalanced: null, differences: [], unspecifiedStereocentres: 0 }], + }); + const placeholder = formatRouteAudit(audit, [[]], null, false, [{ step: 1, role: 'product', byproduct: true, name: 'see prose' }]); + assert.match(placeholder, /no structure resolved for byproduct "see prose"\. That is a placeholder, not a species: list each one by name, or write "none"\./); + const ordinary = formatRouteAudit(audit, [[]], null, false, [{ step: 1, role: 'reactant', byproduct: false, name: 'bornan-2-ol' }]); + assert.match(ordinary, /Give that species a name a reference resolves, or its structure\./); + assert.doesNotMatch(ordinary, /placeholder/); +}); + +test('every fragment of every species reaches the equation, including repeated counterions', () => { + // The components of one species are written out because a reaction SMILES cannot carry the + // boundary; the package regroups them from the labels. Dropping a repeated token to avoid an + // ambiguous balance used to cost atoms, which is the worse failure: calcium chloride lost a + // chloride, so any salt with repeated counterions could never balance. + const salt = buildRouteSteps([[ + { role: 'reactant', smiles: 'CC(=O)O' }, { role: 'reactant', smiles: '[Ca+2].[Cl-].[Cl-]' }, + { role: 'product', smiles: 'CC(=O)[O-]' }, + ]]); + assert.equal(salt[0], 'CC(=O)O.[Ca+2].[Cl-].[Cl-]>>CC(=O)[O-]', 'both chlorides survive'); + + const shared = buildRouteSteps([[ + { role: 'reactant', smiles: 'C[Mg]Br' }, { role: 'reactant', smiles: '[Na+].[Br-]' }, + { role: 'product', smiles: 'C' }, { role: 'product', smiles: '[Mg+2].[Br-]' }, { role: 'product', smiles: '[Na+].[Br-]' }, + ]]); + assert.equal(shared[0], 'C[Mg]Br.[Na+].[Br-]>>C.[Mg+2].[Br-].[Na+].[Br-]', 'two salts sharing an ion keep both'); + + // Unchanged: agents are still dropped from the equation, and a step missing a side is unbuilt. + const agents = buildRouteSteps([[ + { role: 'reactant', smiles: 'CCO' }, { role: 'agent', smiles: 'O=S(=O)(O)O' }, { role: 'product', smiles: 'CC=O' }, + ]]); + assert.equal(agents[0], 'CCO>O=S(=O)(O)O>CC=O'); + assert.equal(buildRouteSteps([[{ role: 'reactant', smiles: 'CCO' }]])[0], '', 'no product is still unbuilt'); +}); diff --git a/shared/moleculeInspection.ts b/shared/moleculeInspection.ts index b22bd0a71..9ecc1e353 100644 --- a/shared/moleculeInspection.ts +++ b/shared/moleculeInspection.ts @@ -983,15 +983,23 @@ export function findStepNamedSpecies(text: string, count: number): NamedSpecies[ * an empty line, so every later step keeps its number and lines up with its labels, prose and * conditions, and the checker reports that step as unbuilt. */ export function buildRouteSteps(speciesByStep: Array>>): string[] { + // Every fragment of every species, in order, with nothing dropped. The components of one + // species are written out because a reaction SMILES has no other way to carry them; the + // package regroups them from the labels, so a salt still counts once and takes one coefficient. + // + // This used to discard a repeated token, to stop two salts sharing an ion from putting the same + // token on one side twice. That cost atoms: the set was per ROLE, so calcium chloride written + // as `[Ca+2].[Cl-].[Cl-]` lost a chloride, and any salt with repeated counterions — magnesium + // bromide, sodium sulfate, potassium carbonate — could then never balance. Losing an atom to + // avoid an ambiguous balance is the wrong trade: a duplicate token is at worst reported as + // several possible equations, which the author can see and fix, while a missing atom is a + // verdict on an equation nobody wrote. const fragments = (step: Array>, role: RouteLabelRole): string[] => { - const seen = new Set(); const out: string[] = []; for (const entry of step.filter((item) => item.role === role)) { for (const part of (entry.smiles ?? '').split('.')) { const token = part.trim(); - if (!token || seen.has(token)) continue; - seen.add(token); - out.push(token); + if (token) out.push(token); } } return out; @@ -1063,6 +1071,16 @@ export function annotateSpeciesSmiles(answer: string, speciesByStep: ResolvedSpe return out; } +/** A placeholder the author wrote where a species belongs: "see prose", "as above", "see step 2". + * It is not a name a resolver could ever turn into a structure, and asking for "its structure" + * invites the author to invent one. Seen live: a step whose Byproducts line read "see prose", + * which made the whole step uncheckable. The rules already say a step that gives its species + * only in prose cannot be checked; this names the specific thing the author did. */ +export function isPlaceholderSpecies(name: string): boolean { + return /^(?:see|as)\b[^.]{0,40}\b(?:prose|above|below|text|step\s*\d*|described|discussion|list)\b/i.test(name.trim()) + || /^(?:unchanged|same as|ditto|various|etc\.?|multiple|several)\b/i.test(name.trim()); +} + /** A species name as the resolver feedback may return it: a short label with letters, and no * markup or escaped syntax. A reply that smuggled an SVG, a JSON fragment or a newline into a * name is not shown to the user as a "correction". */ @@ -1850,9 +1868,15 @@ export function formatRouteAudit(audit: RouteAudit, labels: RouteSpeciesLabel[][ const label = `Step ${step.index + 1}`; if (!step.ok) { if (unbuilt(step)) { - const named = unresolved.filter((entry) => entry.step === step.index + 1) - .map((entry) => `${entry.byproduct ? 'byproduct' : entry.role} "${entry.name}"`); - lines.push(`- ${label} UNBUILT — nothing was checked: ${named.length ? `no structure resolved for ${named.join(', ')}` : 'a species it names has no resolved structure'}. Give that species a name a reference resolves, or its structure.`); + const forStep = unresolved.filter((entry) => entry.step === step.index + 1); + const named = forStep.map((entry) => `${entry.byproduct ? 'byproduct' : entry.role} "${entry.name}"`); + // A placeholder is a different fault from a name that merely would not resolve, and it + // needs different advice: no structure exists to give, the species have to be listed. + const placeholder = forStep.some((entry) => isPlaceholderSpecies(entry.name)); + const advice = placeholder + ? 'That is a placeholder, not a species: list each one by name, or write "none".' + : 'Give that species a name a reference resolves, or its structure.'; + lines.push(`- ${label} UNBUILT — nothing was checked: ${named.length ? `no structure resolved for ${named.join(', ')}` : 'a species it names has no resolved structure'}. ${advice}`); continue; } lines.push(`- ${label} FAIL — ${step.error ?? 'could not be parsed'}`); @@ -2313,6 +2337,9 @@ export function formatUnresolvedNameClarification(unresolved: UnresolvedName[], 'Unresolved species:', ...lines, '', + ...(unresolved.some((entry) => isPlaceholderSpecies(entry.name)) + ? ['A placeholder such as "see prose" or "as above" is not a species and has no structure: list every species of that step by name, or write "none" when a side has none.', ''] + : []), 'Re-output the complete route, in order, with each unresolved species corrected. What may change: only those names — every step keeps its prose and every other name exactly. Each step ends with the four labelled lines of systematic IUPAC names, names only:', ...NAMES_ONLY_FORMAT, ...correctionRules(target),