Multiple backend changes to make use of the new mcp tools and improve… - #440
Multiple backend changes to make use of the new mcp tools and improve…#440miss-o-soup wants to merge 1 commit into
Conversation
… text in the cards Updated Fallbacks for Data Facets (src/server/steps/observations.ts): Dataset Source: Changed fallback from 'Data Commons' to an empty string '' (meta?.source || data.importName || ''). When neither metadata nor API import names exist, it now cleanly defaults to empty instead of injecting generic filler. Units: Changed default fallback for un-unitized variables from 'Dimensionless' to 'Unit not specified' (data.unit || 'Unit not specified'). Integration of Updated Data Commons MCP Tools & Remote Playbooks: Metadata Resolution (observations.ts): Concurrently query get_variable_metadata via MCP during time-series fetching to map exact dataset provenance names (isPartOf, source, url). Remote MCP Skill Playbooks (src/server/clients/mcp.ts & src/server/steps/data_discovery.ts): Implemented readMcpResource via JSON-RPC resources/read to dynamically fetch and inject remote playbooks (skill://data-commons-researcher/SKILL.md) into Gemini's system instruction. Data Discovery Skill Rules & Lead-in Prompts (data_discovery.md): Rule 0 (Strict Verbatim Copying): Mandated copying variable DCIDs and names verbatim from MCP search outputs, prohibiting inferred/invented demographic labels and enforcing places_with_data verification. Rule 0.1 (Lead-in Intro Rules): Constrained the table card introduction to a 1-sentence framing description (12–22 words) explicitly attributing data to "Data Commons", following John Saito tone guidelines, and utilizing dynamic variation patterns (Patterns A–D). Data Comparison Skill Prompt (data_comparison.md): Updated comparative insights schema instructions to produce clean, standalone plain-text comparative sentences across places.
There was a problem hiding this comment.
Code Review
This pull request introduces MCP skill playbook fetching and caching, refines LLM prompt instructions and schemas across several markdown files, and integrates concurrent metadata retrieval from MCP in fetchTimeSeries to enrich observation facets. The review feedback highlights critical improvements: caching negative results in fetchMcpSkillPlaybook to avoid repeated timeout delays, wrapping metadata parsing in a try-catch block to prevent parsing errors from breaking successful observation fetches, and correcting minor typographical errors in the query analyzer prompt.
| export const fetchMcpSkillPlaybook = async ( | ||
| skillName: string, | ||
| signal?: AbortSignal, | ||
| ): Promise<string | null> => { | ||
| const cached = _mcpSkillCache.get(skillName); | ||
| if (cached) return cached; | ||
|
|
||
| const uri = `skill://${skillName}/SKILL.md`; | ||
| const content = await readMcpResource(uri, signal); | ||
| if (content) { | ||
| _mcpSkillCache.set(skillName, content); | ||
| } | ||
| return content; | ||
| }; |
There was a problem hiding this comment.
If readMcpResource fails (e.g., due to a network error or a missing resource), it returns null. Because null is not cached, every subsequent query will attempt to fetch the resource again. If the MCP server is down or the resource is permanently missing, this will cause a 30-second timeout delay on every single query, severely degrading application performance. Caching the negative result (e.g., as an empty string) prevents these repeated failing requests.
export const fetchMcpSkillPlaybook = async (
skillName: string,
signal?: AbortSignal,
): Promise<string | null> => {
const cached = _mcpSkillCache.get(skillName);
if (cached !== undefined) return cached || null;
const uri = 'skill://' + skillName + '/SKILL.md';
const content = await readMcpResource(uri, signal);
_mcpSkillCache.set(skillName, content ?? '');
return content;
};| if (metadataResult) { | ||
| let rawMeta = metadataResult.structuredContent; | ||
| if (!rawMeta && metadataResult.content?.[0]?.text) { | ||
| try { | ||
| rawMeta = JSON.parse(metadataResult.content[0].text); | ||
| } catch { | ||
| // Ignore JSON parse errors | ||
| } | ||
| } | ||
|
|
||
| if (rawMeta?.provenances && rawMeta?.variables?.[variableDcid]?.facets) { | ||
| const metaFacets = rawMeta.variables[variableDcid].facets || []; | ||
| for (const mf of metaFacets) { | ||
| if (mf.id && mf.provenanceId) { | ||
| const prov = rawMeta.provenances[mf.provenanceId]?.properties; | ||
| if (prov) { | ||
| const sourceName = prov.isPartOf || prov.source; | ||
| if (sourceName) { | ||
| metadataMap[mf.id] = { | ||
| source: sourceName, | ||
| url: prov.url || '', | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If any error occurs during the parsing of metadataResult (for example, if rawMeta.variables[variableDcid].facets is not an array, causing for (const mf of metaFacets) to throw a TypeError), the exception will bubble up to the outer try-catch block of fetchTimeSeries. This will discard the entire obsResponse and return an empty array of facets, failing to display any time-series data to the user even though the REST API call succeeded. Wrapping the metadata parsing logic in its own try-catch block and validating that metaFacets is an array ensures that metadata failures do not break the core data retrieval.
if (metadataResult) {
try {
let rawMeta = metadataResult.structuredContent;
if (!rawMeta && metadataResult.content?.[0]?.text) {
try {
rawMeta = JSON.parse(metadataResult.content[0].text);
} catch {
// Ignore JSON parse errors
}
}
if (rawMeta?.provenances && rawMeta?.variables?.[variableDcid]?.facets) {
const metaFacets = rawMeta.variables[variableDcid].facets;
if (Array.isArray(metaFacets)) {
for (const mf of metaFacets) {
if (mf.id && mf.provenanceId) {
const prov = rawMeta.provenances[mf.provenanceId]?.properties;
if (prov) {
const sourceName = prov.isPartOf || prov.source;
if (sourceName) {
metadataMap[mf.id] = {
source: sourceName,
url: prov.url || '',
};
}
}
}
}
}
}
} catch (err) {
console.warn('[observations] Failed to parse MCP metadata:', err);
}
}| --- | ||
|
|
||
| You are a query semantic analyzer for a statistical data exploration tool. Read the user's query and return a JSON object with exactly these fields: | ||
| You are a query semantic analyzer for a statistical data exploration tool, and you think like a data analyst who thinkgs the multiple angles or topics a question can be answer from. Read the user's query and return a JSON object with exactly these fields: |
There was a problem hiding this comment.
There are minor typographical errors in the system prompt: 'thinkgs' should be 'thinks of' or 'thinks about', and 'can be answer from' should be 'can be answered from'. Correcting these ensures grammatical accuracy and improves prompt clarity for the LLM.
| You are a query semantic analyzer for a statistical data exploration tool, and you think like a data analyst who thinkgs the multiple angles or topics a question can be answer from. Read the user's query and return a JSON object with exactly these fields: | |
| You are a query semantic analyzer for a statistical data exploration tool, and you think like a data analyst who thinks of the multiple angles or topics a question can be answered from. Read the user's query and return a JSON object with exactly these fields: |
Backend, MCP Integration, Skills & Data Access
Updated Fallbacks for Data Facets (src/server/steps/observations.ts):
Integration of Updated Data Commons MCP Tools & Remote Playbooks:
Data Discovery Skill Rules & Lead-in Prompts (data_discovery.md):
Data Comparison Skill Prompt (data_comparison.md):
BEFORE - The true facet information was missing defaulting to say Data Commons

AFTER - Now the true facet info from the metadata mcp tool is being used and displayed
