From 3cc056f1a07f15a34095ffa312edffdf7e73614a Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Tue, 28 Jul 2026 19:06:36 +0200 Subject: [PATCH 1/6] fix(tools): correct the regional dimension names and explain metric vs dimension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against the dbt Semantic Layer API directly: searching list_metrics for "country" or "region" returns zero metrics, because those are dimensions. That is what stranded a real client session — it searched metrics for a dimension concept, got an empty array with no explanation, and abandoned the tool. Also corrects a name this guidance had wrong: the organization HQ dimension is entity-prefixed (activity_project_id__organization_lf_region), not a bare organization_lf_region, so the previous wording would have produced an invalid dimension. Spell out what metrics and dimensions each are, and that list_metrics searches metric names and descriptions only. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- internal/tools/lens.go | 6 +++--- internal/tools/lens_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/tools/lens.go b/internal/tools/lens.go index 150ee83..0134672 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -178,9 +178,9 @@ Use query_lfx_lens INSTEAD for: Actions: -- list_metrics: First step. Returns metrics with descriptions. When <=15 match, dimensions are included — often enough to go straight to query. +- list_metrics: First step. Returns metrics — the values you count or aggregate — with descriptions. Its search matches metric names and descriptions ONLY, so search by topic; searching for a dimension concept returns nothing. When <=15 match, dimensions are included — often enough to go straight to query. -- get_dimensions: Get group_by/filter fields for specific metrics. Use when list_metrics returned too many results to include dimensions. +- get_dimensions: Get group_by/filter fields — the attributes you slice by, e.g. country, region, tier — for specific metrics. Use when list_metrics returned too many results to include dimensions, or to find a dimension you could not locate by searching metrics. - query: Execute a metric query. CRITICAL rules: 1. ` @@ -198,7 +198,7 @@ Tips: - Contributors and code-related data (commits, PRs, insertions, deletions) are in the activities model — search for "activities" in list_metrics. IMPORTANT: Questions about contributors and code-related topics that do not involve maintainers should prefer this tool. - Events metrics use project_name rather than project_slug for filtering. -- Country/region breakdowns belong here for contributors, organizations, memberships, event registrations and enrollments — even when the topic would otherwise route to query_lfx_lens. A person's country uses country__* (e.g. country__lf_region); an organization's HQ uses organization_lf_region. Membership metrics don't inline dimensions, so call get_dimensions with search "country" or "region". +- Country/region breakdowns belong here for contributors, organizations, memberships, event registrations and enrollments — even when the topic would otherwise route to query_lfx_lens. Search list_metrics by topic ("contributors", "membership"): country and region are dimensions, not metrics, so searching list_metrics for them returns nothing. Get them from the metric's dimensions instead — country__lf_region for a person's country (contributor, attendee, learner), and the entity-prefixed organization_lf_region (e.g. activity_project_id__organization_lf_region) for an organization's HQ. - ` // semanticLayerSlotSearchProjects: search_projects guidance. diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index 673a28a..c6cf6a3 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -218,8 +218,8 @@ func TestSemanticLayerDescription(t *testing.T) { "Country/region breakdowns belong here for contributors, organizations, memberships, event registrations and enrollments", "even when the topic would otherwise route to query_lfx_lens", "country__lf_region", - "organization_lf_region", - "call get_dimensions with search \"country\" or \"region\"", + "activity_project_id__organization_lf_region", + "country and region are dimensions, not metrics", "Membership questions, EXCEPT country/region breakdowns", } { if !strings.Contains(semanticLayerDescription, want) { From e3ccb79bdf9450cf12b5813335a1ea597b41aa90 Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Wed, 29 Jul 2026 11:02:20 +0200 Subject: [PATCH 2/6] fix(tools): rewrite the semantic layer description around what it can do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The description was 3,662 characters. Descriptions are cut at 2,048 before the model sees them, so 44% of it was invisible — including the entity-prefix rule, the country/region routing tip and the tlf membership caveat. The visible half told the model memberships were an exception without ever saying what to do instead, which is why regional questions kept landing on query_lfx_lens. Rewriting it meant first establishing what the tool actually offers. Probing the live Semantic Layer showed the old framing understated it badly: it is not "pre-aggregated metrics" (MetricFlow compiles SQL per request), and it does not only return numbers — grouping by a name dimension returns a ranked list of named organizations or people, which is exactly the shape of the stakeholder question this work exists to answer. So the description now names the six domains it covers, states that country/region questions always belong here, and describes ranking, trending, multi-dimension breakdown and cross-domain combination. Syntax moved onto the parameter it governs — where carries the MetricFlow forms, group_by the entity__field rule and grain suffixes — because each jsonschema description is a separate field with its own budget, and the model reads it at the moment it fills that field. That redistribution is what made room; the description had been rationing characters against a limit the parameters do not share. describe is renamed help, since it is now a fallback for a failed query rather than a prerequisite. The old name still dispatches so a cached schema does not break. Verified end to end against the live stack rather than by compiling SQL alone: both motivating questions return real data, and the ranked-list example was wrong — it filtered lf_region on 'Asia', which compiles but matches nothing. The value is 'Asia Pacific'. compileSql validates dimension names, never values. The example is fixed and the region values are enumerated. Tests now guard the budget in bytes rather than characters: em-dashes cost three bytes each and the description ran ~30 bytes over its character count. Coverage extends to every parameter and to query_lfx_lens, which has far less headroom and is the likelier of the two to drift past the cut unnoticed. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- internal/tools/lens.go | 312 +++++++++++++++++++----------------- internal/tools/lens_test.go | 170 +++++++++++++++++--- 2 files changed, 316 insertions(+), 166 deletions(-) diff --git a/internal/tools/lens.go b/internal/tools/lens.go index 0134672..a96fba0 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -153,68 +153,38 @@ func handleQueryLFXLens(ctx context.Context, req *mcp.CallToolRequest, args Quer // query_lfx_semantic_layer — structured metric queries // --------------------------------------------------------------------------- -// Description fragments for query_lfx_semantic_layer, assembled below to keep -// the long prose readable. -const ( - // semanticLayerDescHead is everything before the "Use search_projects ..." guidance line. - semanticLayerDescHead = `LFX Insights Semantic Layer — pre-aggregated metrics for code activities & contributions, maintainer counts, project health scores, projects, events & event registrations, and education & certifications. Returns deterministic results in seconds. - -Best for direct, well-scoped questions: totals, counts, averages, breakdowns by a single dimension, and time series (e.g. "total activities for CNCF", "active maintainers by organization", "health score trend by month", "total enrollments by course"). This is also the right tool for contributor/activity questions — it has full contributor data including names, organizations, and activity breakdowns. - -Use query_lfx_lens INSTEAD for: -- Membership questions, EXCEPT country/region breakdowns (see the country/region tip) -- Maintainer names, maintainer+contribution (activities data) joins, or maintainer trends -- Open-ended or exploratory analysis (e.g. "which projects need attention?") -- Questions involving subprojects (e.g. "health scores by project") -- Cross-domain joins (maintainers and contributors are separate models) -- Any question where this tool is struggling or returning errors -- Event sponsorships. All other event and event registration data is fine here - -` - - // semanticLayerDescMid sits between the search_projects guidance and the - // query CRITICAL rule 1 text. - semanticLayerDescMid = ` - -Actions: - -- list_metrics: First step. Returns metrics — the values you count or aggregate — with descriptions. Its search matches metric names and descriptions ONLY, so search by topic; searching for a dimension concept returns nothing. When <=15 match, dimensions are included — often enough to go straight to query. - -- get_dimensions: Get group_by/filter fields — the attributes you slice by, e.g. country, region, tier — for specific metrics. Use when list_metrics returned too many results to include dimensions, or to find a dimension you could not locate by searching metrics. - -- query: Execute a metric query. CRITICAL rules: - 1. ` - - // semanticLayerDescTail sits between the query CRITICAL rule 1 text and - // the final tip line. - semanticLayerDescTail = ` - 2. Different metrics use different entity prefixes — always check the dimensions list from list_metrics to find the correct qualified_names. Do not guess prefixes. - 3. Set a reasonable limit (10-50) to avoid huge results. - 4. If you have loaded in metrics and dimensions, and you still can't get the data you are looking for in 5 query turns or less, use query_lfx_lens. - -- describe: Get detailed syntax reference and examples for any action. - -Tips: -- Contributors and code-related data (commits, PRs, insertions, deletions) are in the activities model — search for "activities" in list_metrics. - IMPORTANT: Questions about contributors and code-related topics that do not involve maintainers should prefer this tool. -- Events metrics use project_name rather than project_slug for filtering. -- Country/region breakdowns belong here for contributors, organizations, memberships, event registrations and enrollments — even when the topic would otherwise route to query_lfx_lens. Search list_metrics by topic ("contributors", "membership"): country and region are dimensions, not metrics, so searching list_metrics for them returns nothing. Get them from the metric's dimensions instead — country__lf_region for a person's country (contributor, attendee, learner), and the entity-prefixed organization_lf_region (e.g. activity_project_id__organization_lf_region) for an organization's HQ. -- ` - - // semanticLayerSlotSearchProjects: search_projects guidance. - semanticLayerSlotSearchProjects = `Use search_projects to find a project slug when scoping to a foundation. Then call list_metrics to discover available metrics.` - - // semanticLayerSlotScopeRule: query CRITICAL rule 1. - semanticLayerSlotScopeRule = `project_slug is optional. When provided, where-clause project filters are validated against that foundation's subtree. It may be omitted for global or cross-foundation questions. To scope to a project, add a where filter — check the dimensions list for the correct one (e.g. registration_id__project_slug). Some models don't have project_slug — they use project_name instead. In that case, use the full project name from search_projects (e.g. "Cloud Native Computing Foundation (CNCF)").` - - // semanticLayerSlotFinalTip: final tip. - semanticLayerSlotFinalTip = `For membership metrics, a Linux Foundation ('tlf') filter only captures direct LF memberships — for global membership aggregates, omit the project filter. Activity metrics are fanned out to foundations, so either a 'tlf' foundation filter or no filter works for global questions.` -) - -// semanticLayerDescription is the assembled query_lfx_semantic_layer description. -const semanticLayerDescription = semanticLayerDescHead + semanticLayerSlotSearchProjects + - semanticLayerDescMid + semanticLayerSlotScopeRule + - semanticLayerDescTail + semanticLayerSlotFinalTip +// semanticLayerDescription is the query_lfx_semantic_layer description. +// +// It is truncated at 2048 characters before the model ever sees it, so it must +// stay under that: anything past the cut is silently invisible, which is how +// earlier guidance (the tlf membership caveat, the project_name tip) went +// unread for as long as it did. TestSemanticLayerDescription_FitsSchemaBudget +// guards the limit. +// +// Detail that does not fit belongs in one of two places, neither of which +// shares this budget: the per-parameter jsonschema descriptions on +// SemanticLayerLFXLensArgs (read at the moment the model fills that field), or +// the help action, whose output is a tool result. Syntax for a parameter goes +// on that parameter; help is a fallback for when a query has already failed, +// not a prerequisite. +const semanticLayerDescription = `LFX Insights Semantic Layer — the query and data-exploration tool for the Linux Foundation data below. + +COVERS (search list_metrics with these words): +- contributions — activity, contributor and org counts, commits, PRs, code lines +- memberships — revenue, counts, churn, discounts, invoices +- events — event, registration, speaker, sponsorship counts and revenue +- education — enrollment and certification counts +- maintainers — total and active maintainer counts +- project health — health scores, software value, cost +- any of the above sliced by country or region — always here, never query_lfx_lens + +Pick metrics, then slice them by any dimension those metrics expose: filter, rank, trend over time, or break down by several dimensions at once. Grouping by a name dimension turns a metric into a ranked list of the things behind it — organizations, people, projects — so "who are the top N" questions belong here. List several metrics in one query, even from different domains — they are joined for you on the dimensions they share, such as country, project, event and organization. Queries run globally, across foundations, or scoped to one. + +Dimension names are entity__field and differ per metric, so copy qualified_names from list_metrics or get_dimensions rather than guessing — e.g. country__lf_region is a person's country, while activity_project_id__organization_lf_region is an organization's HQ. Add metric_time__year (or __quarter, __month, __week, __day) to group_by for a trend. Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. + +USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names, and memberships not sliced by country or region. + +Start with action=list_metrics — it returns dimensions inline when <=15 metrics match, often enough to query straight away. Each parameter's description carries its own syntax.` // RegisterSemanticLayer registers the query_lfx_semantic_layer tool. The // registration gate in cmd/lfx-mcp-server limits this tool to staff callers, @@ -232,94 +202,146 @@ func RegisterSemanticLayer(server *mcp.Server) { } // SemanticLayerLFXLensArgs defines the input for the unified semantic layer tool. +// +// Each jsonschema description is a separate field from the tool description, so +// syntax lives on the parameter it governs — the model reads it at the moment +// it fills that field, and it costs nothing from semanticLayerDescription's +// budget. TestSemanticLayerArgs_FieldsFitSchemaBudget keeps each one bounded. type SemanticLayerLFXLensArgs struct { - ProjectSlug string `json:"project_slug,omitempty" jsonschema:"Optional project slug from search_projects (e.g. 'cncf'). When provided, where-clause project filters are validated against that foundation's subtree. May be omitted for global or cross-foundation queries."` - Action string `json:"action" jsonschema:"Required. Start with list_metrics — often enough to go straight to query. Best for activities, maintainer counts, health scores, projects, events, education. For memberships (except country/region breakdowns), maintainer names/trends, open-ended, subproject, or exploratory questions use query_lfx_lens instead. Values: list_metrics, get_dimensions, query, describe"` - Target string `json:"target,omitempty" jsonschema:"For action=describe only: which action to get help for (e.g. 'query')"` - Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names from list_metrics (for get_dimensions and query)"` - Search string `json:"search,omitempty" jsonschema:"Search term to filter results (for list_metrics and get_dimensions)"` - GroupBy string `json:"group_by,omitempty" jsonschema:"Comma-separated dimension qualified_names to group by (for query)"` - Where string `json:"where,omitempty" jsonschema:"Optional for query action. MetricFlow filter using {{ Dimension('qualified_name') }} = 'value' syntax. Include a project scope filter to scope results (find the correct project_slug or project_name dimension from list_metrics); may be omitted for global or cross-foundation queries. Example: {{ Dimension('registration_id__project_slug') }} = 'cncf'"` - OrderBy string `json:"order_by,omitempty" jsonschema:"Comma-separated sort fields, prefix with - for descending (for query)"` - Limit int `json:"limit,omitempty" jsonschema:"Max rows to return, max 500 (for query)"` + ProjectSlug string `json:"project_slug,omitempty" jsonschema:"Optional project slug from search_projects (e.g. 'cncf'). Omit it for global or cross-foundation questions — the normal case for country and region questions. When provided, the where clause must also carry a project filter and every project reference is validated against that foundation's subtree."` + Action string `json:"action" jsonschema:"Required. One of: list_metrics, get_dimensions, query, help. list_metrics(search) — start here. Matches metric names and descriptions only, so search a COVERS topic word; a dimension word like 'country' matches no metrics. When 15 or fewer metrics match, each comes back with its dimension qualified_names, usually enough to query straight away. If nothing returns, broaden the term; an unknown metric name is rejected with ranked suggestions, so use those rather than guessing again. get_dimensions(metrics, search) — needs at least one metric; passing several returns only the dimensions they share, which is exactly the set a cross-domain query can group by. query(metrics, group_by, where, order_by, limit) — run the query; syntax is on each parameter. help(target) — worked examples; call it when a query fails or you want a template."` + Target string `json:"target,omitempty" jsonschema:"For action=help only: which action to get examples for (e.g. 'query'). Omit for an overview."` + Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names from list_metrics (required for get_dimensions and query). List several to combine them in one result: metrics from different domains are outer-joined on the dimensions they share, so a group present in only one domain still appears, with NULL for the other metric. You can only group such a query by dimensions the metrics have in common — get_dimensions with several metrics returns exactly that set. Many metrics are already filtered — current_* means active-only, total_contributors excludes bots — so do not repeat those conditions in where."` + Search string `json:"search,omitempty" jsonschema:"Filters results by name and description. For list_metrics use a topic word from COVERS ('contributor', 'membership', 'event', 'enrollment', 'maintainer', 'health'). For get_dimensions use the slice you are after, e.g. 'region', 'country', 'tier', 'name'."` + GroupBy string `json:"group_by,omitempty" jsonschema:"Comma-separated dimension qualified_names, copied verbatim from list_metrics or get_dimensions — they are entity__field and the entity prefix differs per metric, so never assemble one by hand. Group by a name dimension to turn a metric into a ranked list of organizations, people or projects. For a trend add metric_time__year, or __quarter, __month, __week, __day. The entities listed alongside a metric are join keys, not group-by values — grouping by one returns raw IDs, so use the matching name dimension instead."` + Where string `json:"where,omitempty" jsonschema:"MetricFlow filter expression; this clause does the actual data filtering. Categorical: {{ Dimension('country__lf_region') }} = 'Europe'. Time: {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01'. Dates are yyyy-mm-dd. Use the qualified_name exactly as returned by list_metrics or get_dimensions. If you passed project_slug, include a project filter here too — find the matching project_slug or project_name dimension in the dimensions list."` + OrderBy string `json:"order_by,omitempty" jsonschema:"Comma-separated sort fields. Each must also appear in group_by or metrics. Prefix with - for descending, e.g. -current_membership_revenue. Pair with limit for top-N questions."` + Limit int `json:"limit,omitempty" jsonschema:"Maximum rows to return, ceiling 500. Use 10-20 for top-N questions and 50-100 for full breakdowns."` } -var lensDescribeTexts = map[string]string{ - "list_metrics": `list_metrics — Discover available LFX Insights metrics. +// lensHelpTexts back the help action. These are tool results, so they carry no +// character budget — but they are a fallback, not a prerequisite: everything +// needed to compose a first query lives in semanticLayerDescription and the +// per-parameter descriptions. +var lensHelpTexts = map[string]string{ + "list_metrics": `list_metrics — discover metrics. Always the first call. -Returns metric names, descriptions, types, and labels. When <=15 metrics match, each metric also includes its available dimension qualified_names — so you can go straight to a query without calling get_dimensions. + search (optional): matches metric NAMES and DESCRIPTIONS only. -Parameters: - search (optional): filter term matched against name and description +Search by topic, not by the slice you want: "contributor", "membership", +"event", "enrollment", "maintainer", "health". Words that name a dimension — +"country", "region", "tier" — match no metrics at all. -Example: - action: "list_metrics", search: "maintainer" - → returns active_maintainers, total_maintainers, etc. with their dimensions`, +When 15 or fewer metrics match, each comes back with its dimension +qualified_names, which is usually enough to go straight to query. - "get_dimensions": `get_dimensions — Get dimensions available for specified metrics. +Each metric also lists its entities. Those are the keys that link domains, not +things to group by: they are why two metrics can be combined (both +total_contributors and current_membership_revenue carry country). To find what +you can actually group a multi-metric query by, call get_dimensions with both +metrics. -Dimensions are attributes you can group by or filter on. The qualified_name in the response is the exact string to use in group_by and where clauses. +Nothing returned? Broaden the topic or drop to a single word. An unknown metric +name is rejected with ranked suggestions — use them rather than guessing again.`, -Use this when list_metrics returned too many results to include dimensions inline, or when you need full dimension detail (descriptions, types, time granularities). + "get_dimensions": `get_dimensions — list the dimensions available to a set of metrics. -Parameters: - metrics (required): comma-separated metric names to get dimensions for - search (optional): filter dimensions by name + metrics (required): comma-separated metric names. Dimensions cannot be + searched without a metric, so choose a metric first. + search (optional): filters by name and description, e.g. "region". -Examples: - action: "get_dimensions", metrics: "active_maintainers" - → finds: maintainer_key__account_name, maintainer_key__project_slug, maintainer_key__platform, ... +Use each returned qualified_name verbatim in group_by and where. - action: "get_dimensions", metrics: "current_membership_revenue" - → finds: asset_id__membership_tier, asset_id__project_slug, asset_id__account_name, ...`, +Passing several metrics returns only the dimensions they SHARE, and that set is +much smaller than either metric's own. Those shared dimensions are what a +cross-domain query can group by.`, - "query": lensQueryDescribeShared + lensQueryDescribeImportant, + "query": lensQueryHelp, } -// lensQueryDescribeShared is the bulk of the "query" describe text; the final -// Important paragraph is kept separate for readability. -const lensQueryDescribeShared = `query — Execute a metric query against the Semantic Layer. - -Parameters: - metrics (required): comma-separated metric names to query. - group_by (optional): comma-separated dimension qualified_names from list_metrics or get_dimensions. - where (optional): MetricFlow filter expression. Use the qualified_name from dimensions: - - Categorical: {{ Dimension('qualified_name') }} = 'value' - - Time: {{ TimeDimension('qualified_name', 'GRAIN') }} >= '2024-01-01' - - Dates must be yyyy-mm-dd format. - order_by (optional): comma-separated sort fields. Must also appear in group_by or metrics. Prefix with - for descending. - limit (optional): max rows to return (max 500). Use 10-20 for "top N" queries, 50-100 for breakdowns. - -For lookback queries (e.g. "last 6 months"), prefer order_by descending on a time dimension + limit, rather than complex where filters. - -Examples: - -"How many active maintainers does CNCF have?" - project_slug: "cncf" - action: "query" - metrics: "active_maintainers" - where: "{{ Dimension('maintainer_key__project_slug') }} = 'cncf'" - -"Membership revenue by tier for CNCF" - project_slug: "cncf" - action: "query" - metrics: "current_membership_revenue" - group_by: "asset_id__membership_tier" - where: "{{ Dimension('asset_id__project_slug') }} = 'cncf'" - order_by: "-current_membership_revenue" - -"Top 10 projects by health score" - project_slug: "cncf" - action: "query" - metrics: "avg_project_health_score" - group_by: "health_metric_key__project_slug, health_metric_key__project_name" - where: "{{ Dimension('health_metric_key__foundation_slug') }} = 'cncf'" - order_by: "-avg_project_health_score" - limit: 10 - -` - -const lensQueryDescribeImportant = `Important: project_slug is optional. When provided, where-clause project filters are validated against that foundation's subtree — the where clause does the actual data filtering. Omit project_slug and the project filter for global or cross-foundation queries.` +// lensHelpOverview is returned by help with no target. +const lensHelpOverview = `LFX Insights Semantic Layer — how to use it + +Workflow: list_metrics(search) → get_dimensions (only if you need more) → query. + + metric the number being measured + dimension an attribute you group, filter or list by + entity the key that links domains — country, project, event, organization + +Because domains share entities, one query can span them: contribution metrics +and membership metrics both reach the country dimensions, so they can be +compared side by side in a single result. You never write a join — list several +metrics and group by a dimension they share, and the join path is derived from +the shared entity. + +Dimension qualified_names are entity__field. The prefix is the primary key of +the metric's own table, so it differs from metric to metric. Always copy the +name from list_metrics or get_dimensions. + +help targets: query, list_metrics, get_dimensions` + +const lensQueryHelp = `query — run a metric query. + + metrics (required) comma-separated metric names. + group_by (optional) dimension qualified_names, comma-separated. + where (optional) MetricFlow filter: + categorical {{ Dimension('country__lf_region') }} = 'Europe' + time {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01' + dates yyyy-mm-dd. + order_by (optional) must also appear in group_by or metrics; - for descending. + limit (optional) ceiling 500. 10-20 for top-N, 50-100 for breakdowns. + +Trends: add metric_time__year (or __quarter, __month, __week, __day) to +group_by rather than writing date ranges by hand. + +Ranked lists: group by a name dimension, order by the metric descending, and +set a limit. + +Combining metrics from different domains outer-joins them, so a group with data +in only one domain still appears, with NULL for the other metric. + +Pre-filtered metrics: current_* is already active-only and total_contributors +already excludes bots. Do not add those conditions again. + +project_slug is optional. Supply it and the where clause must carry a project +filter, validated against that foundation's subtree. Omit both for global or +cross-foundation questions. + +Examples + + Active maintainers in CNCF + project_slug cncf + metrics active_maintainers + where {{ Dimension('maintainer_key__project_slug') }} = 'cncf' + + Membership revenue by tier, CNCF + project_slug cncf + metrics current_membership_revenue + group_by asset_id__membership_tier + where {{ Dimension('asset_id__project_slug') }} = 'cncf' + order_by -current_membership_revenue + + Top 10 organizations by contribution in a region + metrics total_contributors + group_by activity_project_id__organization_name + where {{ Dimension('activity_project_id__organization_lf_region') }} = 'Asia Pacific' + order_by -total_contributors + limit 10 + + Region values are exact strings — group by the dimension with no filter first + to see them. lf_region is one of: North America, Europe, China, India, Japan, + Asia Pacific, Middle East & Africa, Latin America, Other. + + Contribution against financial involvement, by region, globally + metrics total_contributors, total_contributing_organizations, current_membership_revenue + group_by country__lf_region + order_by -current_membership_revenue + + European membership revenue trend by year + metrics current_membership_revenue + group_by country__lf_region, metric_time__year + where {{ Dimension('country__lf_region') }} = 'Europe' + limit 100` func handleSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args SemanticLayerLFXLensArgs) (*mcp.CallToolResult, any, error) { if lensConfig == nil { @@ -327,8 +349,10 @@ func handleSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args Seman } switch args.Action { - case "describe": - return handleLensDescribe(args.Target) + // "describe" is the pre-rename name for this action, kept so a caller + // working from a cached schema does not get an Unknown action error. + case "help", "describe": + return handleLensHelp(args.Target) case "list_metrics": return handleLensListMetrics(ctx, args) case "get_dimensions": @@ -337,26 +361,20 @@ func handleSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args Seman return handleLensQueryMetrics(ctx, args) default: return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: describe, list_metrics, get_dimensions, query", args.Action)}}, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: list_metrics, get_dimensions, query, help", args.Action)}}, IsError: true, }, nil, nil } } -func handleLensDescribe(target string) (*mcp.CallToolResult, any, error) { +func handleLensHelp(target string) (*mcp.CallToolResult, any, error) { if target == "" { - var sb strings.Builder - sb.WriteString("Available actions (use target to get details):\n\n") - for _, action := range []string{"list_metrics", "get_dimensions", "query"} { - lines := strings.SplitN(lensDescribeTexts[action], "\n", 2) - sb.WriteString(" " + lines[0] + "\n") - } return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: sb.String()}}, + Content: []mcp.Content{&mcp.TextContent{Text: lensHelpOverview}}, }, nil, nil } - text, ok := lensDescribeTexts[target] + text, ok := lensHelpTexts[target] if !ok { return &mcp.CallToolResult{ Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid targets: list_metrics, get_dimensions, query", target)}}, diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index c6cf6a3..3c2bfb4 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -207,27 +207,105 @@ func TestSemanticLayer_DescribeQuery(t *testing.T) { // Description content // --------------------------------------------------------------------------- +// schemaDescriptionBudget is the hard limit that makes the rest of this +// guidance meaningful: descriptions shipped in tools/list are truncated at 2048 +// before the model sees them. Anything past the cut is silently invisible — +// which is what happened to the tlf membership caveat and the project_name tip +// for as long as they sat at the end of the description. +// +// These checks measure bytes, because len() on a Go string is bytes and the +// prose is full of em-dashes at 3 bytes each — the description runs ~30 bytes +// over its character count. Whether the truncation upstream counts bytes, +// characters or tokens is not something we control or have measured, so the +// budget is enforced against the larger number. +const schemaDescriptionBudget = 2048 + +// TestSemanticLayerDescription_FitsSchemaBudget guards that limit for the tool +// description itself. +func TestSemanticLayerDescription_FitsSchemaBudget(t *testing.T) { + if got := len(semanticLayerDescription); got > schemaDescriptionBudget { + t.Errorf("description is %d chars; everything past %d is invisible to the model — move detail onto a parameter or into help", + got, schemaDescriptionBudget) + } +} + func TestSemanticLayerDescription(t *testing.T) { for _, want := range []string{ - "project_slug is optional", - "may be omitted for global or cross-foundation questions", - "For membership metrics, a Linux Foundation ('tlf') filter only captures direct LF memberships", - "Activity metrics are fanned out to foundations", - // Regional guidance: country/region questions must route here for every - // topic, including memberships, whose dimensions are not inlined. - "Country/region breakdowns belong here for contributors, organizations, memberships, event registrations and enrollments", - "even when the topic would otherwise route to query_lfx_lens", + // The domains this tool owns are named explicitly. Without them the + // routing is one-sided — query_lfx_lens lists concrete triggers while + // this tool describes itself abstractly, so every specific question + // looks like a better match for the other tool. + "contributions —", + "memberships —", + "events —", + "education —", + "maintainers —", + "project health —", + // Regional questions route here for every topic, memberships included. + "any of the above sliced by country or region — always here, never query_lfx_lens", + "memberships not sliced by country or region", + // Capabilities that the earlier "pre-aggregated metrics" framing hid. + "ranked list", + "List several metrics in one query", + "metric_time__year", + // Regional dimensions: person's country vs organization HQ. "country__lf_region", "activity_project_id__organization_lf_region", - "country and region are dimensions, not metrics", - "Membership questions, EXCEPT country/region breakdowns", + // help is a fallback, not a prerequisite. + "Start with action=list_metrics", } { if !strings.Contains(semanticLayerDescription, want) { t.Errorf("description missing %q", want) } } - if strings.Contains(semanticLayerDescription, "MUST include a project scope filter") { - t.Error("description must not contain the mandatory-scope wording") + for _, unwanted := range []string{ + "MUST include a project scope filter", + // Framings that understate the tool and misroute the questions it + // exists to answer: it compiles SQL per request rather than serving + // stored rollups, and grouping by a name dimension returns lists of + // named organizations and people, not only figures. + "pre-aggregated", + "returns numbers, not records", + } { + if strings.Contains(semanticLayerDescription, unwanted) { + t.Errorf("description must not contain %q", unwanted) + } + } +} + +// TestSemanticLayerArgs_FieldsFitSchemaBudget holds the other half of the +// budget contract. Syntax was deliberately moved out of the tool description +// and onto the parameters it governs; each property description is a separate +// field, so each must independently stay under the limit. +func TestSemanticLayerArgs_FieldsFitSchemaBudget(t *testing.T) { + tool := listSemanticLayerTool(t) + for _, property := range []string{ + "project_slug", "action", "target", "metrics", + "search", "group_by", "where", "order_by", "limit", + } { + if got := len(schemaPropertyDescription(t, tool, property)); got > schemaDescriptionBudget { + t.Errorf("%s description is %d chars; everything past %d is invisible to the model", + property, got, schemaDescriptionBudget) + } + } +} + +// TestBothLensToolDescriptionsFitBudget guards every description that ships in +// tools/list, not just the semantic layer's. query_lfx_lens has far less +// headroom and is the likelier of the two to drift past the cut unnoticed. +func TestBothLensToolDescriptionsFitBudget(t *testing.T) { + for _, tc := range []struct { + name string + register func(*mcp.Server) + }{ + {"query_lfx_semantic_layer", RegisterSemanticLayer}, + {"query_lfx_lens", RegisterQueryLFXLens}, + } { + tool := listRegisteredTool(t, tc.name, tc.register) + if got := len(tool.Description); got > schemaDescriptionBudget { + t.Errorf("%s description is %d chars; everything past %d is invisible to the model", + tc.name, got, schemaDescriptionBudget) + } } } @@ -329,16 +407,70 @@ func TestRegisterSemanticLayer_Schema(t *testing.T) { if !contains(required, "action") { t.Errorf("schema required = %v; expected to contain action", required) } - if !strings.Contains(tool.Description, "project_slug is optional") { - t.Error("description missing optional project_slug wording") + // The optional-scope rule moved onto the parameter it governs, where the + // model reads it while filling the field. + slug := schemaPropertyDescription(t, tool, "project_slug") + if !strings.Contains(slug, "Omit it for global or cross-foundation questions") { + t.Errorf("project_slug schema description missing the optional-scope rule: %q", slug) } - // The action property's own guidance ships with tools/list, so its - // membership routing must carry the same regional exception as the tool - // description — otherwise clients get contradictory instructions. + // The action property's own guidance ships with tools/list, so it must name + // the same four actions the dispatcher accepts — a stale list here sends + // the model to an action that errors. action := schemaPropertyDescription(t, tool, "action") - if !strings.Contains(action, "For memberships (except country/region breakdowns)") { - t.Errorf("action schema description missing the regional exception: %q", action) + for _, want := range []string{"list_metrics", "get_dimensions", "query", "help"} { + if !strings.Contains(action, want) { + t.Errorf("action schema description missing %q: %q", want, action) + } + } + if strings.Contains(action, "describe") { + t.Errorf("action schema description still advertises the renamed describe action: %q", action) + } + + // Syntax the description defers to the parameters must actually be there. + where := schemaPropertyDescription(t, tool, "where") + for _, want := range []string{"Dimension(", "TimeDimension(", "yyyy-mm-dd"} { + if !strings.Contains(where, want) { + t.Errorf("where schema description missing %q: %q", want, where) + } + } + groupBy := schemaPropertyDescription(t, tool, "group_by") + for _, want := range []string{"metric_time__year", "join keys, not group-by values"} { + if !strings.Contains(groupBy, want) { + t.Errorf("group_by schema description missing %q: %q", want, groupBy) + } + } +} + +// TestHelpActionAndDescribeAlias checks the renamed action works and that the +// old name still dispatches, so a client working from a cached schema does not +// hit an Unknown action error. +func TestHelpActionAndDescribeAlias(t *testing.T) { + setupLensTest(t) + + for _, action := range []string{"help", "describe"} { + res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + Action: action, + }) + if err != nil { + t.Fatalf("action %q: unexpected error: %v", action, err) + } + if text := resultText(t, res); !strings.Contains(text, "how to use it") { + t.Errorf("action %q did not return the help overview: %q", action, text) + } + } + + res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + Action: "help", + Target: "query", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // The worked examples are the reason help exists; they must survive the + // move off the description. + if text := resultText(t, res); !strings.Contains(text, "metric_time__year") { + t.Errorf("help query text missing the trend example: %q", text) } } From 74edac4958468038df36fcd745f74f5df751c3f2 Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Wed, 29 Jul 2026 12:03:54 +0200 Subject: [PATCH 3/6] refactor(tools): split the semantic layer into discovery and query tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testing the description against a live client turned up a constraint we had been designing around blind. Clients that defer tool schemas behind a search index — Claude Desktop does — re-serialise the schema and replace OPTIONAL parameter descriptions with a short generated summary. The 459-byte where description arrived as "Filter conditions." and order_by as "Sort order.". Temporarily marking limit required made its real text appear, which is what pinned the cause down. That invalidated the previous commit's main structural move. Syntax had been pushed onto the parameter it governs precisely because each jsonschema description is a separate field with its own budget — true on the wire, but those fields are the ones that get summarised away. Roughly 3,300 bytes of guidance was reaching this client as a paraphrase. Cramming it all back into one description does not fit, and cramming it into the action parameter would have been a workaround rather than a fix. Splitting is the fix: discovery and querying become two tools, so each gets its own 2048 budget, the MetricFlow syntax lives in a tool description rather than on an optional parameter, and metrics becomes genuinely required on the query tool instead of optional — so its multi-metric join rules survive on their own merits. explore_lfx_semantic_layer carries discovery, the covered domains and the routing boundary with query_lfx_lens. query_lfx_semantic_layer carries the query and states its own where/order_by/limit syntax, so a caller never has to call help first. The name and the gate are unchanged, so existing clients keep working; a caller on a cached schema that still sends action=query is told where querying moved rather than getting a bare unknown-action error. TestCriticalGuidanceSurvivesSchemaCompaction is the guard: it walks both tool descriptions plus their required parameters and fails if a token the model cannot guess — the filter syntax, the date format, the limit ceiling, the trend grain — is reachable only through an optional parameter. The optional descriptions stay full and accurate for clients that pass them through; they just are not the only copy. Verified against the live stack: both motivating regional questions return the same data through the new tool, discovery returns metrics with dimensions inline, and the action=query redirect fires. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- README.md | 8 +- cmd/lfx-mcp-server/main.go | 3 + internal/tools/lens.go | 166 ++++++++++++++-------- internal/tools/lens_test.go | 265 ++++++++++++++++++++++++++---------- 4 files changed, 310 insertions(+), 132 deletions(-) diff --git a/README.md b/README.md index 9e42d27..de1648a 100644 --- a/README.md +++ b/README.md @@ -282,9 +282,11 @@ Hitting **Connect** will open a browser window for LFID login. ### LFX Lens -| Tool | Description | -|------------------|-------------------------------------------------------------------------------------------------------| -| `query_lfx_lens` | Ask natural-language questions about a project's data (events, contributors, health, value, and more) | +| Tool | Description | +|------------------------------|-------------------------------------------------------------------------------------------------------| +| `query_lfx_lens` | Ask natural-language questions about a project's data (events, contributors, health, value, and more) | +| `explore_lfx_semantic_layer` | Discover Insights metrics and the dimensions available to them | +| `query_lfx_semantic_layer` | Run a metric query against the Insights Semantic Layer (filter, group, rank, trend) | ### B2B Organizations diff --git a/cmd/lfx-mcp-server/main.go b/cmd/lfx-mcp-server/main.go index 7321e89..d436a46 100644 --- a/cmd/lfx-mcp-server/main.go +++ b/cmd/lfx-mcp-server/main.go @@ -160,6 +160,7 @@ var defaultTools = []string{ "list_email_templates", "send_email", "query_lfx_lens", + "explore_lfx_semantic_layer", "query_lfx_semantic_layer", "search_b2b_orgs", } @@ -797,6 +798,8 @@ func newServer(cfg Config, serviceName string, callerToken *auth.TokenInfo) *mcp if enabledTools["query_lfx_lens"] && canRead && isStaff { tools.RegisterQueryLFXLens(server) } + // RegisterSemanticLayer adds both explore_lfx_semantic_layer and + // query_lfx_semantic_layer; they are a pair and share the same gate. if enabledTools["query_lfx_semantic_layer"] && canRead && isStaff { tools.RegisterSemanticLayer(server) } diff --git a/internal/tools/lens.go b/internal/tools/lens.go index a96fba0..4271a3e 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -150,26 +150,24 @@ func handleQueryLFXLens(ctx context.Context, req *mcp.CallToolRequest, args Quer } // --------------------------------------------------------------------------- -// query_lfx_semantic_layer — structured metric queries +// explore_lfx_semantic_layer / query_lfx_semantic_layer — structured metrics // --------------------------------------------------------------------------- -// semanticLayerDescription is the query_lfx_semantic_layer description. +// Both descriptions are truncated at 2048 characters before the model ever sees +// them, so each must stay under that: anything past the cut is silently +// invisible, which is how earlier guidance (the tlf membership caveat, the +// project_name tip) went unread for as long as it did. +// TestSemanticLayerDescriptions_FitSchemaBudget guards the limit. // -// It is truncated at 2048 characters before the model ever sees it, so it must -// stay under that: anything past the cut is silently invisible, which is how -// earlier guidance (the tlf membership caveat, the project_name tip) went -// unread for as long as it did. TestSemanticLayerDescription_FitsSchemaBudget -// guards the limit. -// -// Detail that does not fit belongs in one of two places, neither of which -// shares this budget: the per-parameter jsonschema descriptions on -// SemanticLayerLFXLensArgs (read at the moment the model fills that field), or -// the help action, whose output is a tool result. Syntax for a parameter goes -// on that parameter; help is a fallback for when a query has already failed, -// not a prerequisite. -const semanticLayerDescription = `LFX Insights Semantic Layer — the query and data-exploration tool for the Linux Foundation data below. - -COVERS (search list_metrics with these words): +// Discovery and querying are split across two tools so that each gets its own +// budget, and so the query's MetricFlow syntax lives in a tool description +// rather than on an optional parameter — see the note on +// QuerySemanticLayerArgs for why that distinction matters. Anything that still +// does not fit belongs in the help action, whose output is a tool result and +// carries no limit; help is a fallback for a failed query, not a prerequisite. +const exploreSemanticLayerDescription = `Discover what the LFX Insights Semantic Layer can measure, then query it with query_lfx_semantic_layer. Start here whenever you do not already know the exact metric and dimension names. + +COVERS (search these words): - contributions — activity, contributor and org counts, commits, PRs, code lines - memberships — revenue, counts, churn, discounts, invoices - events — event, registration, speaker, sponsorship counts and revenue @@ -178,45 +176,90 @@ COVERS (search list_metrics with these words): - project health — health scores, software value, cost - any of the above sliced by country or region — always here, never query_lfx_lens -Pick metrics, then slice them by any dimension those metrics expose: filter, rank, trend over time, or break down by several dimensions at once. Grouping by a name dimension turns a metric into a ranked list of the things behind it — organizations, people, projects — so "who are the top N" questions belong here. List several metrics in one query, even from different domains — they are joined for you on the dimensions they share, such as country, project, event and organization. Queries run globally, across foundations, or scoped to one. +A metric is the number being measured (total_contributors); a dimension is how you slice, filter or list it (country__lf_region, asset_id__membership_tier). Dimension names are entity__field and the prefix differs per metric, so always copy qualified_names from this tool rather than assembling one by hand — e.g. country__lf_region is a person's country, while activity_project_id__organization_lf_region is an organization's HQ. + +ACTIONS +- list_metrics(search): searches metric names and descriptions only, so search a topic word above; a dimension word like "country" matches no metrics. When 15 or fewer match, each comes back with its dimension qualified_names — usually enough to query straight away. If nothing returns, broaden the term. +- get_dimensions(metrics, search): every dimension available to those metrics. Requires at least one metric, so pick a metric first. Passing several returns only the dimensions they share, which is exactly what a cross-domain query can group by. +- help(target): worked query examples. Call it when a query fails or you want a template. + +USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names, and memberships not sliced by country or region.` -Dimension names are entity__field and differ per metric, so copy qualified_names from list_metrics or get_dimensions rather than guessing — e.g. country__lf_region is a person's country, while activity_project_id__organization_lf_region is an organization's HQ. Add metric_time__year (or __quarter, __month, __week, __day) to group_by for a trend. Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. +const querySemanticLayerDescription = `Run a query against the LFX Insights Semantic Layer. Covers contributions, memberships, events, education, maintainers and project health — and is always the right tool for anything sliced by country or region. Use explore_lfx_semantic_layer first if you do not know the metric and dimension names; use query_lfx_lens for narrative or "why" questions that do not reduce to a metric. -USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names, and memberships not sliced by country or region. + metrics (required): comma-separated names. List several to combine them in one result, even across domains — they are joined for you on the dimensions they share. Such a query can only group by dimensions the metrics have in common, and is outer-joined, so a group present in only one domain still appears with NULL for the other. + group_by: dimension qualified_names, comma-separated, copied verbatim from explore_lfx_semantic_layer. Group by a name dimension to turn a metric into a ranked list of organizations, people or projects. For a trend add metric_time__year, or __quarter, __month, __week, __day. + where: MetricFlow filter; this does the actual filtering. + categorical {{ Dimension('country__lf_region') }} = 'Europe' + time {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01' + Dates are yyyy-mm-dd. Region values are exact strings; group by the dimension with no filter to see them. + order_by: comma-separated; each field must also appear in group_by or metrics. Prefix - for descending. Pair with limit for top-N. + limit: ceiling 500. Use 10-20 for top-N, 50-100 for full breakdowns. + project_slug: optional. Omit it for global or cross-foundation questions — the normal case for country and region questions. When given, the where clause must also carry a project filter, validated against that foundation's subtree. -Start with action=list_metrics — it returns dimensions inline when <=15 metrics match, often enough to query straight away. Each parameter's description carries its own syntax.` +Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. The entities listed with a metric are join keys, not group-by values: grouping by one returns raw IDs, so use the matching name dimension.` -// RegisterSemanticLayer registers the query_lfx_semantic_layer tool. The -// registration gate in cmd/lfx-mcp-server limits this tool to staff callers, -// so project scoping is optional here; lfx-lens validates any project filters -// that are provided against the requested foundation's subtree. +// RegisterSemanticLayer registers the two semantic layer tools. The +// registration gate in cmd/lfx-mcp-server limits both to staff callers, so +// project scoping is optional here; lfx-lens validates any project filters that +// are provided against the requested foundation's subtree. +// +// Discovery and querying are separate tools rather than actions on one tool +// because a tool description and its required parameters are the only guidance +// that reaches the model intact — see the note on SemanticLayerLFXLensArgs. +// Splitting gives the query its own description to hold the MetricFlow syntax, +// and makes metrics genuinely required there rather than optional. func RegisterSemanticLayer(server *mcp.Server) { + mcp.AddTool(server, &mcp.Tool{ + Name: "explore_lfx_semantic_layer", + Description: exploreSemanticLayerDescription, + Annotations: &mcp.ToolAnnotations{ + Title: "Explore LFX Semantic Layer", + ReadOnlyHint: true, + }, + }, handleExploreSemanticLayer) + mcp.AddTool(server, &mcp.Tool{ Name: "query_lfx_semantic_layer", - Description: semanticLayerDescription, + Description: querySemanticLayerDescription, Annotations: &mcp.ToolAnnotations{ Title: "Query LFX Semantic Layer", ReadOnlyHint: true, }, - }, handleSemanticLayer) + }, handleQuerySemanticLayer) +} + +// ExploreSemanticLayerArgs defines the input for explore_lfx_semantic_layer. +type ExploreSemanticLayerArgs struct { + Action string `json:"action" jsonschema:"Required. One of: list_metrics, get_dimensions, help."` + Search string `json:"search,omitempty" jsonschema:"For list_metrics, a topic word ('contributor', 'membership', 'event', 'enrollment', 'maintainer', 'health'). For get_dimensions, the slice you are after, e.g. 'region', 'tier', 'name'."` + Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names. Required for get_dimensions; pass several to see only the dimensions they share."` + Target string `json:"target,omitempty" jsonschema:"For action=help only: which action to get examples for (e.g. 'query'). Omit for an overview."` } -// SemanticLayerLFXLensArgs defines the input for the unified semantic layer tool. +// QuerySemanticLayerArgs defines the input for query_lfx_semantic_layer. +// +// Only the tool description and REQUIRED parameters reach the model intact. +// Clients that defer tool schemas behind a search index — Claude Desktop does — +// re-serialise the schema and replace optional parameter descriptions with a +// short generated summary. Verified against a live client: a 459-byte where +// description arrived as "Filter conditions." and order_by as "Sort order.". +// Temporarily marking limit required was enough to make its real description +// appear, which is what pinned the cause down. // -// Each jsonschema description is a separate field from the tool description, so -// syntax lives on the parameter it governs — the model reads it at the moment -// it fills that field, and it costs nothing from semanticLayerDescription's -// budget. TestSemanticLayerArgs_FieldsFitSchemaBudget keeps each one bounded. -type SemanticLayerLFXLensArgs struct { - ProjectSlug string `json:"project_slug,omitempty" jsonschema:"Optional project slug from search_projects (e.g. 'cncf'). Omit it for global or cross-foundation questions — the normal case for country and region questions. When provided, the where clause must also carry a project filter and every project reference is validated against that foundation's subtree."` - Action string `json:"action" jsonschema:"Required. One of: list_metrics, get_dimensions, query, help. list_metrics(search) — start here. Matches metric names and descriptions only, so search a COVERS topic word; a dimension word like 'country' matches no metrics. When 15 or fewer metrics match, each comes back with its dimension qualified_names, usually enough to query straight away. If nothing returns, broaden the term; an unknown metric name is rejected with ranked suggestions, so use those rather than guessing again. get_dimensions(metrics, search) — needs at least one metric; passing several returns only the dimensions they share, which is exactly the set a cross-domain query can group by. query(metrics, group_by, where, order_by, limit) — run the query; syntax is on each parameter. help(target) — worked examples; call it when a query fails or you want a template."` - Target string `json:"target,omitempty" jsonschema:"For action=help only: which action to get examples for (e.g. 'query'). Omit for an overview."` - Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names from list_metrics (required for get_dimensions and query). List several to combine them in one result: metrics from different domains are outer-joined on the dimensions they share, so a group present in only one domain still appears, with NULL for the other metric. You can only group such a query by dimensions the metrics have in common — get_dimensions with several metrics returns exactly that set. Many metrics are already filtered — current_* means active-only, total_contributors excludes bots — so do not repeat those conditions in where."` - Search string `json:"search,omitempty" jsonschema:"Filters results by name and description. For list_metrics use a topic word from COVERS ('contributor', 'membership', 'event', 'enrollment', 'maintainer', 'health'). For get_dimensions use the slice you are after, e.g. 'region', 'country', 'tier', 'name'."` - GroupBy string `json:"group_by,omitempty" jsonschema:"Comma-separated dimension qualified_names, copied verbatim from list_metrics or get_dimensions — they are entity__field and the entity prefix differs per metric, so never assemble one by hand. Group by a name dimension to turn a metric into a ranked list of organizations, people or projects. For a trend add metric_time__year, or __quarter, __month, __week, __day. The entities listed alongside a metric are join keys, not group-by values — grouping by one returns raw IDs, so use the matching name dimension instead."` - Where string `json:"where,omitempty" jsonschema:"MetricFlow filter expression; this clause does the actual data filtering. Categorical: {{ Dimension('country__lf_region') }} = 'Europe'. Time: {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01'. Dates are yyyy-mm-dd. Use the qualified_name exactly as returned by list_metrics or get_dimensions. If you passed project_slug, include a project filter here too — find the matching project_slug or project_name dimension in the dimensions list."` - OrderBy string `json:"order_by,omitempty" jsonschema:"Comma-separated sort fields. Each must also appear in group_by or metrics. Prefix with - for descending, e.g. -current_membership_revenue. Pair with limit for top-N questions."` +// So Metrics is required here — it carries the multi-metric join rules — and +// anything else the model must not get wrong, above all the MetricFlow filter +// syntax it cannot guess, is repeated in querySemanticLayerDescription. The +// optional descriptions below stay full and accurate for clients that pass them +// through unchanged; they just are not the only copy. +// TestCriticalGuidanceSurvivesSchemaCompaction guards that split. +type QuerySemanticLayerArgs struct { + Metrics string `json:"metrics" jsonschema:"Required. Comma-separated metric names from explore_lfx_semantic_layer. List several to combine them in one result, even across domains: they are outer-joined on the dimensions they share, so a group present in only one domain still appears with NULL for the other metric, and you can only group by dimensions they have in common. Many metrics are already filtered — current_* means active-only, total_contributors excludes bots — so do not repeat those conditions in where."` + GroupBy string `json:"group_by,omitempty" jsonschema:"Comma-separated dimension qualified_names, copied verbatim from explore_lfx_semantic_layer — they are entity__field and the prefix differs per metric. Group by a name dimension for a ranked list of organizations, people or projects; add metric_time__year (or __quarter, __month, __week, __day) for a trend."` + Where string `json:"where,omitempty" jsonschema:"MetricFlow filter; this clause does the actual data filtering. Categorical: {{ Dimension('country__lf_region') }} = 'Europe'. Time: {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01'. Dates are yyyy-mm-dd."` + OrderBy string `json:"order_by,omitempty" jsonschema:"Comma-separated sort fields. Each must also appear in group_by or metrics. Prefix with - for descending, e.g. -current_membership_revenue."` Limit int `json:"limit,omitempty" jsonschema:"Maximum rows to return, ceiling 500. Use 10-20 for top-N questions and 50-100 for full breakdowns."` + ProjectSlug string `json:"project_slug,omitempty" jsonschema:"Optional project slug from search_projects (e.g. 'cncf'). Omit it for global or cross-foundation questions. When provided, the where clause must also carry a project filter, validated against that foundation's subtree."` } // lensHelpTexts back the help action. These are tool results, so they carry no @@ -343,25 +386,30 @@ Examples where {{ Dimension('country__lf_region') }} = 'Europe' limit 100` -func handleSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args SemanticLayerLFXLensArgs) (*mcp.CallToolResult, any, error) { +func handleExploreSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args ExploreSemanticLayerArgs) (*mcp.CallToolResult, any, error) { if lensConfig == nil { return nil, nil, fmt.Errorf("LFX Lens tools not configured") } switch args.Action { - // "describe" is the pre-rename name for this action, kept so a caller - // working from a cached schema does not get an Unknown action error. + // "describe" is the pre-rename name for help, kept so a caller working + // from a cached schema does not get an Unknown action error. case "help", "describe": return handleLensHelp(args.Target) case "list_metrics": - return handleLensListMetrics(ctx, args) + return handleLensListMetrics(ctx, args.Search) case "get_dimensions": - return handleLensGetDimensions(ctx, args) + return handleLensGetDimensions(ctx, args.Metrics, args.Search) case "query": - return handleLensQueryMetrics(ctx, args) + // Querying moved to its own tool; a caller on a cached schema would + // otherwise get a bare "unknown action" with nowhere to go. + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "Querying moved to the query_lfx_semantic_layer tool. Call it directly with metrics, group_by, where, order_by and limit."}}, + IsError: true, + }, nil, nil default: return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: list_metrics, get_dimensions, query, help", args.Action)}}, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: list_metrics, get_dimensions, help. To run a query, use the query_lfx_semantic_layer tool.", args.Action)}}, IsError: true, }, nil, nil } @@ -387,16 +435,16 @@ func handleLensHelp(target string) (*mcp.CallToolResult, any, error) { }, nil, nil } -func handleLensListMetrics(ctx context.Context, args SemanticLayerLFXLensArgs) (*mcp.CallToolResult, any, error) { +func handleLensListMetrics(ctx context.Context, search string) (*mcp.CallToolResult, any, error) { params := url.Values{} - if args.Search != "" { - params.Set("search", args.Search) + if search != "" { + params.Set("search", search) } return lensDoGet(ctx, "/lfx-lens/semantic-layer/metrics", params) } -func handleLensGetDimensions(ctx context.Context, args SemanticLayerLFXLensArgs) (*mcp.CallToolResult, any, error) { - metrics := parseCSV(args.Metrics) +func handleLensGetDimensions(ctx context.Context, metricsArg, search string) (*mcp.CallToolResult, any, error) { + metrics := parseCSV(metricsArg) if len(metrics) == 0 { return &mcp.CallToolResult{ Content: []mcp.Content{&mcp.TextContent{Text: "Error: metrics parameter is required for get_dimensions"}}, @@ -406,17 +454,21 @@ func handleLensGetDimensions(ctx context.Context, args SemanticLayerLFXLensArgs) params := url.Values{} params.Set("metrics", strings.Join(metrics, ",")) - if args.Search != "" { - params.Set("search", args.Search) + if search != "" { + params.Set("search", search) } return lensDoGet(ctx, "/lfx-lens/semantic-layer/dimensions", params) } -func handleLensQueryMetrics(ctx context.Context, args SemanticLayerLFXLensArgs) (*mcp.CallToolResult, any, error) { +func handleQuerySemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args QuerySemanticLayerArgs) (*mcp.CallToolResult, any, error) { + if lensConfig == nil { + return nil, nil, fmt.Errorf("LFX Lens tools not configured") + } + metrics := parseCSV(args.Metrics) if len(metrics) == 0 { return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: "Error: metrics parameter is required for query"}}, + Content: []mcp.Content{&mcp.TextContent{Text: "Error: metrics is required. Use explore_lfx_semantic_layer with action=list_metrics to find metric names."}}, IsError: true, }, nil, nil } diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index 3c2bfb4..d9bc901 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -82,8 +82,7 @@ func resultText(t *testing.T, res *mcp.CallToolResult) string { func TestSemanticLayer_GlobalQueryOmitsProjectSlugAndWhere(t *testing.T) { captured := setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ - Action: "query", + res, _, err := handleQuerySemanticLayer(context.Background(), &mcp.CallToolRequest{}, QuerySemanticLayerArgs{ Metrics: "active_maintainers", }) if err != nil { @@ -111,9 +110,8 @@ func TestSemanticLayer_GlobalQueryOmitsProjectSlugAndWhere(t *testing.T) { func TestSemanticLayer_ScopedQuerySendsProjectSlugAndWhere(t *testing.T) { captured := setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleQuerySemanticLayer(context.Background(), &mcp.CallToolRequest{}, QuerySemanticLayerArgs{ ProjectSlug: "cncf", - Action: "query", Metrics: "active_maintainers", Where: "{{ Dimension('maintainer_key__project_slug') }} = 'cncf'", }) @@ -139,7 +137,7 @@ func TestSemanticLayer_ScopedQuerySendsProjectSlugAndWhere(t *testing.T) { func TestSemanticLayer_ListMetricsWithoutProjectSlug(t *testing.T) { captured := setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ Action: "list_metrics", }) if err != nil { @@ -156,7 +154,7 @@ func TestSemanticLayer_ListMetricsWithoutProjectSlug(t *testing.T) { func TestSemanticLayer_GetDimensionsWithoutProjectSlug(t *testing.T) { captured := setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ Action: "get_dimensions", Metrics: "active_maintainers", }) @@ -174,8 +172,7 @@ func TestSemanticLayer_GetDimensionsWithoutProjectSlug(t *testing.T) { func TestSemanticLayer_LimitTooLarge(t *testing.T) { setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ - Action: "query", + res, _, err := handleQuerySemanticLayer(context.Background(), &mcp.CallToolRequest{}, QuerySemanticLayerArgs{ Metrics: "active_maintainers", Limit: 501, }) @@ -190,7 +187,7 @@ func TestSemanticLayer_LimitTooLarge(t *testing.T) { func TestSemanticLayer_DescribeQuery(t *testing.T) { setupLensTest(t) - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ Action: "describe", Target: "query", }) @@ -220,21 +217,29 @@ func TestSemanticLayer_DescribeQuery(t *testing.T) { // budget is enforced against the larger number. const schemaDescriptionBudget = 2048 -// TestSemanticLayerDescription_FitsSchemaBudget guards that limit for the tool -// description itself. -func TestSemanticLayerDescription_FitsSchemaBudget(t *testing.T) { - if got := len(semanticLayerDescription); got > schemaDescriptionBudget { - t.Errorf("description is %d chars; everything past %d is invisible to the model — move detail onto a parameter or into help", - got, schemaDescriptionBudget) +// TestSemanticLayerDescriptions_FitSchemaBudget guards that limit for both +// semantic layer tools. Splitting discovery from querying gave each its own +// budget, which is the point of the split. +func TestSemanticLayerDescriptions_FitSchemaBudget(t *testing.T) { + for name, desc := range map[string]string{ + "explore_lfx_semantic_layer": exploreSemanticLayerDescription, + "query_lfx_semantic_layer": querySemanticLayerDescription, + } { + if got := len(desc); got > schemaDescriptionBudget { + t.Errorf("%s description is %d bytes; everything past %d is invisible to the model — move detail into help", + name, got, schemaDescriptionBudget) + } } } -func TestSemanticLayerDescription(t *testing.T) { +// TestExploreSemanticLayerDescription checks the discovery tool carries the +// routing contract: which domains are ours, and when to use query_lfx_lens. +func TestExploreSemanticLayerDescription(t *testing.T) { for _, want := range []string{ - // The domains this tool owns are named explicitly. Without them the - // routing is one-sided — query_lfx_lens lists concrete triggers while - // this tool describes itself abstractly, so every specific question - // looks like a better match for the other tool. + // The domains are named explicitly. Without them the routing is + // one-sided — query_lfx_lens lists concrete triggers while this tool + // describes itself abstractly, so every specific question looks like a + // better match for the other tool. "contributions —", "memberships —", "events —", @@ -244,18 +249,39 @@ func TestSemanticLayerDescription(t *testing.T) { // Regional questions route here for every topic, memberships included. "any of the above sliced by country or region — always here, never query_lfx_lens", "memberships not sliced by country or region", - // Capabilities that the earlier "pre-aggregated metrics" framing hid. - "ranked list", - "List several metrics in one query", - "metric_time__year", - // Regional dimensions: person's country vs organization HQ. + // Dimension naming, and the regional person-vs-organization split. + "entity__field", "country__lf_region", "activity_project_id__organization_lf_region", - // help is a fallback, not a prerequisite. - "Start with action=list_metrics", + // Discovery must hand off to the query tool by name. + "query_lfx_semantic_layer", + } { + if !strings.Contains(exploreSemanticLayerDescription, want) { + t.Errorf("explore description missing %q", want) + } + } +} + +// TestQuerySemanticLayerDescription checks the query tool is self-sufficient: +// its own description carries the syntax, so a caller never has to call help +// first. +func TestQuerySemanticLayerDescription(t *testing.T) { + for _, want := range []string{ + "metrics (required)", + "Dimension(", + "TimeDimension(", + "yyyy-mm-dd", + "ceiling 500", + "metric_time__year", + "outer-joined", + "ranked list", + "project_slug", + // Both neighbours are named so routing works from this tool too. + "explore_lfx_semantic_layer", + "query_lfx_lens", } { - if !strings.Contains(semanticLayerDescription, want) { - t.Errorf("description missing %q", want) + if !strings.Contains(querySemanticLayerDescription, want) { + t.Errorf("query description missing %q", want) } } for _, unwanted := range []string{ @@ -267,25 +293,70 @@ func TestSemanticLayerDescription(t *testing.T) { "pre-aggregated", "returns numbers, not records", } { - if strings.Contains(semanticLayerDescription, unwanted) { - t.Errorf("description must not contain %q", unwanted) + if strings.Contains(querySemanticLayerDescription, unwanted) { + t.Errorf("query description must not contain %q", unwanted) } } } // TestSemanticLayerArgs_FieldsFitSchemaBudget holds the other half of the -// budget contract. Syntax was deliberately moved out of the tool description -// and onto the parameters it governs; each property description is a separate -// field, so each must independently stay under the limit. +// budget contract: each property description is a separate field, so each must +// independently stay under the limit. func TestSemanticLayerArgs_FieldsFitSchemaBudget(t *testing.T) { - tool := listSemanticLayerTool(t) - for _, property := range []string{ - "project_slug", "action", "target", "metrics", - "search", "group_by", "where", "order_by", "limit", + for _, tc := range []struct { + tool *mcp.Tool + props []string + }{ + {listExploreTool(t), []string{"action", "search", "metrics", "target"}}, + {listQueryTool(t), []string{"metrics", "group_by", "where", "order_by", "limit", "project_slug"}}, } { - if got := len(schemaPropertyDescription(t, tool, property)); got > schemaDescriptionBudget { - t.Errorf("%s description is %d chars; everything past %d is invisible to the model", - property, got, schemaDescriptionBudget) + for _, property := range tc.props { + if got := len(schemaPropertyDescription(t, tc.tool, property)); got > schemaDescriptionBudget { + t.Errorf("%s.%s description is %d bytes; everything past %d is invisible to the model", + tc.tool.Name, property, got, schemaDescriptionBudget) + } + } + } +} + +// TestCriticalGuidanceSurvivesSchemaCompaction is the load-bearing test for +// where guidance is allowed to live. +// +// Clients that defer tool schemas behind a search index re-serialise them and +// replace OPTIONAL parameter descriptions with a short generated summary. This +// was verified against a live client: the 459-byte where description arrived as +// "Filter conditions.", order_by as "Sort order.", and limit as no description +// at all — until limit was temporarily marked required, at which point its real +// text appeared. Only the tool description and required parameters survive. +// +// So syntax the model cannot guess must not live solely on an optional +// parameter. Keeping the full text there is fine and useful for clients that do +// pass it through; it just may not be the only copy. +func TestCriticalGuidanceSurvivesSchemaCompaction(t *testing.T) { + var surviving string + for _, tool := range []*mcp.Tool{listExploreTool(t), listQueryTool(t)} { + surviving += "\n" + tool.Description + for _, name := range schemaRequired(t, tool) { + surviving += "\n" + schemaPropertyDescription(t, tool, name) + } + } + + for _, tc := range []struct { + token string + why string + }{ + {"Dimension(", "categorical filter syntax is unguessable"}, + {"TimeDimension(", "time filter syntax is unguessable"}, + {"yyyy-mm-dd", "date format silently returns wrong rows if guessed"}, + {"ceiling 500", "over-limit requests are rejected outright"}, + {"metric_time__year", "the only way to build a trend"}, + {"entity__field", "dimension names cannot be assembled by hand"}, + {"outer-joined", "explains NULLs in cross-domain results"}, + {"raw IDs", "grouping by an entity silently returns unusable output"}, + } { + if !strings.Contains(surviving, tc.token) { + t.Errorf("%q reaches the model only via an optional parameter, where it gets summarised away (%s). Move it into the tool description or onto a required parameter.", + tc.token, tc.why) } } } @@ -298,6 +369,7 @@ func TestBothLensToolDescriptionsFitBudget(t *testing.T) { name string register func(*mcp.Server) }{ + {"explore_lfx_semantic_layer", RegisterSemanticLayer}, {"query_lfx_semantic_layer", RegisterSemanticLayer}, {"query_lfx_lens", RegisterQueryLFXLens}, } { @@ -313,7 +385,12 @@ func TestBothLensToolDescriptionsFitBudget(t *testing.T) { // Registration / schema // --------------------------------------------------------------------------- -func listSemanticLayerTool(t *testing.T) *mcp.Tool { +func listExploreTool(t *testing.T) *mcp.Tool { + t.Helper() + return listRegisteredTool(t, "explore_lfx_semantic_layer", RegisterSemanticLayer) +} + +func listQueryTool(t *testing.T) *mcp.Tool { t.Helper() return listRegisteredTool(t, "query_lfx_semantic_layer", RegisterSemanticLayer) } @@ -395,30 +472,21 @@ func schemaPropertyDescription(t *testing.T, tool *mcp.Tool, property string) st } func TestRegisterSemanticLayer_Schema(t *testing.T) { - tool := listSemanticLayerTool(t) + explore := listExploreTool(t) + query := listQueryTool(t) - required := schemaRequired(t, tool) - if contains(required, "project_slug") { - t.Errorf("schema required = %v; project_slug must be optional", required) - } - if contains(required, "where") { - t.Errorf("schema required = %v; where must be optional", required) + // Discovery: action is the only required field, and it must name exactly + // the actions the dispatcher accepts — a stale list sends the model to an + // action that errors. Querying lives on the other tool now. + exploreRequired := schemaRequired(t, explore) + if !contains(exploreRequired, "action") { + t.Errorf("explore required = %v; expected to contain action", exploreRequired) } - if !contains(required, "action") { - t.Errorf("schema required = %v; expected to contain action", required) + if contains(exploreRequired, "metrics") { + t.Errorf("explore required = %v; metrics is only needed for get_dimensions", exploreRequired) } - // The optional-scope rule moved onto the parameter it governs, where the - // model reads it while filling the field. - slug := schemaPropertyDescription(t, tool, "project_slug") - if !strings.Contains(slug, "Omit it for global or cross-foundation questions") { - t.Errorf("project_slug schema description missing the optional-scope rule: %q", slug) - } - - // The action property's own guidance ships with tools/list, so it must name - // the same four actions the dispatcher accepts — a stale list here sends - // the model to an action that errors. - action := schemaPropertyDescription(t, tool, "action") - for _, want := range []string{"list_metrics", "get_dimensions", "query", "help"} { + action := schemaPropertyDescription(t, explore, "action") + for _, want := range []string{"list_metrics", "get_dimensions", "help"} { if !strings.Contains(action, want) { t.Errorf("action schema description missing %q: %q", want, action) } @@ -427,18 +495,71 @@ func TestRegisterSemanticLayer_Schema(t *testing.T) { t.Errorf("action schema description still advertises the renamed describe action: %q", action) } - // Syntax the description defers to the parameters must actually be there. - where := schemaPropertyDescription(t, tool, "where") + // Query: metrics is required, so its multi-metric join rules survive schema + // compaction. Everything else stays optional — above all project_slug, + // whose whole point is that global questions omit it. + queryRequired := schemaRequired(t, query) + if !contains(queryRequired, "metrics") { + t.Errorf("query required = %v; metrics must be required so its guidance survives compaction", queryRequired) + } + for _, optional := range []string{"project_slug", "where", "group_by", "order_by", "limit"} { + if contains(queryRequired, optional) { + t.Errorf("query required = %v; %s must stay optional", queryRequired, optional) + } + } + + // The optional descriptions are still expected to be complete, for clients + // that pass them through unchanged. + where := schemaPropertyDescription(t, query, "where") for _, want := range []string{"Dimension(", "TimeDimension(", "yyyy-mm-dd"} { if !strings.Contains(where, want) { t.Errorf("where schema description missing %q: %q", want, where) } } - groupBy := schemaPropertyDescription(t, tool, "group_by") - for _, want := range []string{"metric_time__year", "join keys, not group-by values"} { - if !strings.Contains(groupBy, want) { - t.Errorf("group_by schema description missing %q: %q", want, groupBy) - } + groupBy := schemaPropertyDescription(t, query, "group_by") + if !strings.Contains(groupBy, "metric_time__year") { + t.Errorf("group_by schema description missing the trend grain: %q", groupBy) + } + slug := schemaPropertyDescription(t, query, "project_slug") + if !strings.Contains(slug, "Omit it for global or cross-foundation questions") { + t.Errorf("project_slug schema description missing the optional-scope rule: %q", slug) + } +} + +// TestQueryToolRejectsMissingMetricsWithAPointer keeps the recovery path alive +// for a caller on a cached schema that still sends action=query here. +func TestQueryToolRejectsMissingMetricsWithAPointer(t *testing.T) { + setupLensTest(t) + + res, _, err := handleQuerySemanticLayer(context.Background(), &mcp.CallToolRequest{}, QuerySemanticLayerArgs{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !res.IsError { + t.Fatal("expected an error result when metrics is empty") + } + if text := resultText(t, res); !strings.Contains(text, "explore_lfx_semantic_layer") { + t.Errorf("missing-metrics error should point at the discovery tool: %q", text) + } +} + +// TestExploreToolRedirectsQueryAction covers the other half of that migration: +// a caller still passing action=query to the discovery tool gets told where +// querying moved rather than a bare unknown-action error. +func TestExploreToolRedirectsQueryAction(t *testing.T) { + setupLensTest(t) + + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "query", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !res.IsError { + t.Fatal("expected an error result for action=query on the discovery tool") + } + if text := resultText(t, res); !strings.Contains(text, "query_lfx_semantic_layer") { + t.Errorf("redirect should name the query tool: %q", text) } } @@ -449,7 +570,7 @@ func TestHelpActionAndDescribeAlias(t *testing.T) { setupLensTest(t) for _, action := range []string{"help", "describe"} { - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ Action: action, }) if err != nil { @@ -460,7 +581,7 @@ func TestHelpActionAndDescribeAlias(t *testing.T) { } } - res, _, err := handleSemanticLayer(context.Background(), &mcp.CallToolRequest{}, SemanticLayerLFXLensArgs{ + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ Action: "help", Target: "query", }) From 325e9162b991865eea5add455cb02ae03fb837da Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Wed, 29 Jul 2026 15:04:10 +0200 Subject: [PATCH 4/6] feat(tools): add dimension-value discovery to the semantic layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A where clause naming a real dimension but an unknown literal is not an error: the query succeeds and returns zero rows, which reads as "no such data" rather than "no such spelling". A live client asked which companies to meet in Vietnam and spent five successful-but-wrong queries on that gap — 'APAC' for a region stored as 'Asia Pacific', 'Vietnam' for a country stored as 'Viet Nam' — before escaping via a country code, and the CNCF-scoped half of the question never resolved at all. Add a get_dimension_values action listing the literals a dimension holds, with an optional substring search. It requires metrics: a dimension-only query bypasses the metric allowlist entirely, so without that gate any dimension in the semantic layer — including PII-bearing ones no allowlisted metric exposes — would be dumpable. The action name and the zero-rows rule sit on the required `action` parameter and in both tool descriptions, because clients that defer tool schemas summarise optional parameter descriptions away. Drop the warning that a plural search matches nothing. Lens now falls back to a singular stem, so "memberships" returns 18 metrics and "contributions" 2; the claim was false and was spending budget the new action needed. The asset_id__billing_country trap goes in help, where there is room: it is unnormalized free text holding both 'Viet Nam' and 'Vietnam' alongside 'na', 'US' and 'Untied States', so filtering on it silently drops members filed under another spelling. The clean country__* dimensions return the same members without that risk. LFXV2-2893: https://linuxfoundation.atlassian.net/browse/LFXV2-2893 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- internal/tools/lens.go | 141 +++++++++++++------ internal/tools/lens_test.go | 263 +++++++++++++++++++++++++++++++++--- 2 files changed, 346 insertions(+), 58 deletions(-) diff --git a/internal/tools/lens.go b/internal/tools/lens.go index 4271a3e..4f25735 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -41,8 +41,6 @@ func RegisterQueryLFXLens(server *mcp.Server) { Description: `Ask natural language questions about a project's data using ad-hoc SQL generation. Always use this tool for: -- Membership questions (e.g. "current members", "membership revenue by tier", "churn rate"), EXCEPT country/region - breakdowns, which use query_lfx_semantic_layer - Maintainer names or maintainer+activities data joins, where activities data is the code activities model with code contributions, PRs, commits etc (e.g. "top maintainers by contributions", "who maintains Kubernetes?"). IMPORTANT: activities data (contributors, PRs, code contributions etc) not involving maintainers should use query_lfx_semantic_layer. @@ -55,7 +53,7 @@ Also use this tool for: - Cross-domain joins that the semantic layer cannot do (e.g. maintainers + activities) - Any question where query_lfx_semantic_layer is struggling or returning errors -Important: questions just about contributors/activities (without maintainer joins) should use query_lfx_semantic_layer — it has full contributor data including names, organizations, and activity breakdowns. +Important: contributor, activity and membership questions belong to the semantic layer — explore_lfx_semantic_layer then query_lfx_semantic_layer. Use search_projects first to find the project slug. @@ -72,7 +70,7 @@ Tips: // QueryLFXLensArgs defines the input for query_lfx_lens. type QueryLFXLensArgs struct { ProjectSlug string `json:"project_slug" jsonschema:"Project slug from search_projects (e.g. 'cncf') (required)"` - Input string `json:"input" jsonschema:"Natural language question. Always use for memberships (except country/region breakdowns), maintainer names/trends, open-ended analysis, subproject questions, cross-domain joins, and exploratory questions. Takes 15-30s. (required)"` + Input string `json:"input" jsonschema:"Natural language question. Use for maintainer names/trends, open-ended analysis, subproject questions, cross-domain joins, and exploratory questions. Contributor, activity and membership questions belong to the semantic layer. Takes 15-30s. (required)"` } type lensWorkflowAdditional struct { @@ -165,39 +163,40 @@ func handleQueryLFXLens(ctx context.Context, req *mcp.CallToolRequest, args Quer // QuerySemanticLayerArgs for why that distinction matters. Anything that still // does not fit belongs in the help action, whose output is a tool result and // carries no limit; help is a fallback for a failed query, not a prerequisite. -const exploreSemanticLayerDescription = `Discover what the LFX Insights Semantic Layer can measure, then query it with query_lfx_semantic_layer. Start here whenever you do not already know the exact metric and dimension names. - -COVERS (search these words): -- contributions — activity, contributor and org counts, commits, PRs, code lines -- memberships — revenue, counts, churn, discounts, invoices -- events — event, registration, speaker, sponsorship counts and revenue -- education — enrollment and certification counts -- maintainers — total and active maintainer counts -- project health — health scores, software value, cost +const exploreSemanticLayerDescription = `The LFX Insights Semantic Layer is the query and data-exploration tool for Linux Foundation data. This half discovers what can be measured; query_lfx_semantic_layer runs it. Start here whenever you do not already know the exact metric, dimension and value names. + +COVERS — search one of these topic words: +- contributor, contribution — activity and org counts, commits, PRs +- membership, revenue, churn — counts, discounts, invoices +- event, registration, sponsorship, speaker — counts and revenue +- enrollment, certification — education +- maintainer — total and active counts +- health, project — health scores, software value, cost - any of the above sliced by country or region — always here, never query_lfx_lens -A metric is the number being measured (total_contributors); a dimension is how you slice, filter or list it (country__lf_region, asset_id__membership_tier). Dimension names are entity__field and the prefix differs per metric, so always copy qualified_names from this tool rather than assembling one by hand — e.g. country__lf_region is a person's country, while activity_project_id__organization_lf_region is an organization's HQ. +A metric is the number measured (total_contributors); a dimension is how you slice, filter or list it (country__lf_region). Names are entity__field and the prefix differs per metric, so copy qualified_names from this tool rather than assembling one — country__lf_region is a person's country, activity_project_id__organization_lf_region an organization's HQ. ACTIONS -- list_metrics(search): searches metric names and descriptions only, so search a topic word above; a dimension word like "country" matches no metrics. When 15 or fewer match, each comes back with its dimension qualified_names — usually enough to query straight away. If nothing returns, broaden the term. -- get_dimensions(metrics, search): every dimension available to those metrics. Requires at least one metric, so pick a metric first. Passing several returns only the dimensions they share, which is exactly what a cross-domain query can group by. -- help(target): worked query examples. Call it when a query fails or you want a template. +- list_metrics(search): searches metric names and descriptions only, so search a topic word above, not a dimension word like "country". When 15 or fewer match, each returns its dimension qualified_names — usually enough to query. +- get_dimensions(metrics, search): dimensions available to those metrics; needs at least one. Several returns only the ones they share — what a cross-domain query can group by. +- get_dimension_values(dimension, metrics, search): the literals a dimension holds. Call it before filtering on any value not already seen in output: an unknown literal returns zero rows, not an error, so a wrong guess reads as missing data. Spellings surprise — 'Asia Pacific' not 'APAC', 'Viet Nam' not 'Vietnam'. +- help(target): worked query examples, for when a query fails. -USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names, and memberships not sliced by country or region.` +USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names.` -const querySemanticLayerDescription = `Run a query against the LFX Insights Semantic Layer. Covers contributions, memberships, events, education, maintainers and project health — and is always the right tool for anything sliced by country or region. Use explore_lfx_semantic_layer first if you do not know the metric and dimension names; use query_lfx_lens for narrative or "why" questions that do not reduce to a metric. +const querySemanticLayerDescription = `The LFX Insights Semantic Layer is the query and data-exploration tool for Linux Foundation data; this half runs the query. Covers contributions, memberships, events, education, maintainers and project health — and anything sliced by country or region. ALWAYS call explore_lfx_semantic_layer first unless you already have the exact metric, dimension and entity names — never guess or assemble one: a wrong name errors, and a wrong filter value returns no rows rather than an error, so confirm literals with get_dimension_values. Use query_lfx_lens for narrative or "why" questions that do not reduce to a metric. - metrics (required): comma-separated names. List several to combine them in one result, even across domains — they are joined for you on the dimensions they share. Such a query can only group by dimensions the metrics have in common, and is outer-joined, so a group present in only one domain still appears with NULL for the other. - group_by: dimension qualified_names, comma-separated, copied verbatim from explore_lfx_semantic_layer. Group by a name dimension to turn a metric into a ranked list of organizations, people or projects. For a trend add metric_time__year, or __quarter, __month, __week, __day. - where: MetricFlow filter; this does the actual filtering. + metrics (required): comma-separated names. List several to combine them in one result, even across domains — they are joined on the dimensions they have in common, the only set such a query can group by. The join is outer, so a group in only one domain still appears with NULL for the other. + group_by: dimension qualified_names, comma-separated, copied verbatim. Group by a name dimension for a ranked list of organizations, people or projects. For a trend add metric_time__year, or __quarter, __month, __week, __day. + where: MetricFlow filter; this does the filtering. categorical {{ Dimension('country__lf_region') }} = 'Europe' time {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01' - Dates are yyyy-mm-dd. Region values are exact strings; group by the dimension with no filter to see them. - order_by: comma-separated; each field must also appear in group_by or metrics. Prefix - for descending. Pair with limit for top-N. + Dates are yyyy-mm-dd. + order_by: comma-separated; each field must also appear in group_by or metrics. Prefix - for descending. limit: ceiling 500. Use 10-20 for top-N, 50-100 for full breakdowns. - project_slug: optional. Omit it for global or cross-foundation questions — the normal case for country and region questions. When given, the where clause must also carry a project filter, validated against that foundation's subtree. + project_slug: optional. Omit it for global or cross-foundation questions — the normal case for country and region ones. When given, the where clause must also carry a project filter, validated against that foundation's subtree. -Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. The entities listed with a metric are join keys, not group-by values: grouping by one returns raw IDs, so use the matching name dimension.` +Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. Entities listed with a metric are join keys, not group-by values: grouping by one returns raw IDs, so use the name dimension.` // RegisterSemanticLayer registers the two semantic layer tools. The // registration gate in cmd/lfx-mcp-server limits both to staff callers, so @@ -230,11 +229,17 @@ func RegisterSemanticLayer(server *mcp.Server) { } // ExploreSemanticLayerArgs defines the input for explore_lfx_semantic_layer. +// +// Action is the only required field, so under the schema compaction described +// on QuerySemanticLayerArgs it is the one parameter description that survives +// intact — hence the full action list lives there rather than being split +// across the optional fields. type ExploreSemanticLayerArgs struct { - Action string `json:"action" jsonschema:"Required. One of: list_metrics, get_dimensions, help."` - Search string `json:"search,omitempty" jsonschema:"For list_metrics, a topic word ('contributor', 'membership', 'event', 'enrollment', 'maintainer', 'health'). For get_dimensions, the slice you are after, e.g. 'region', 'tier', 'name'."` - Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names. Required for get_dimensions; pass several to see only the dimensions they share."` - Target string `json:"target,omitempty" jsonschema:"For action=help only: which action to get examples for (e.g. 'query'). Omit for an overview."` + Action string `json:"action" jsonschema:"Required. One of: list_metrics, get_dimensions, get_dimension_values, help. Use get_dimension_values before filtering on any value you have not seen in output: a where clause with a real dimension but an unknown literal returns zero rows instead of an error, so a wrong guess looks exactly like missing data."` + Search string `json:"search,omitempty" jsonschema:"For list_metrics, a topic word ('contributor', 'membership', 'event', 'enrollment', 'maintainer', 'health'). For get_dimensions, the slice you are after, e.g. 'region', 'tier', 'name'. For get_dimension_values, a fragment of the value — keep it short, since the stored spelling often differs from the everyday one."` + Metrics string `json:"metrics,omitempty" jsonschema:"Comma-separated metric names. Required for get_dimensions and get_dimension_values; pass several to get_dimensions to see only the dimensions they share."` + Dimension string `json:"dimension,omitempty" jsonschema:"For action=get_dimension_values only: one dimension qualified_name, copied from get_dimensions (e.g. 'country__lf_region')."` + Target string `json:"target,omitempty" jsonschema:"For action=help only: which action to get examples for (e.g. 'query'). Omit for an overview."` } // QuerySemanticLayerArgs defines the input for query_lfx_semantic_layer. @@ -254,7 +259,7 @@ type ExploreSemanticLayerArgs struct { // through unchanged; they just are not the only copy. // TestCriticalGuidanceSurvivesSchemaCompaction guards that split. type QuerySemanticLayerArgs struct { - Metrics string `json:"metrics" jsonschema:"Required. Comma-separated metric names from explore_lfx_semantic_layer. List several to combine them in one result, even across domains: they are outer-joined on the dimensions they share, so a group present in only one domain still appears with NULL for the other metric, and you can only group by dimensions they have in common. Many metrics are already filtered — current_* means active-only, total_contributors excludes bots — so do not repeat those conditions in where."` + Metrics string `json:"metrics" jsonschema:"Required. Comma-separated metric names taken from explore_lfx_semantic_layer — never guessed. List several to combine them in one result, even across domains: they are outer-joined on the dimensions they share, so a group present in only one domain still appears with NULL for the other metric, and you can only group by dimensions they have in common. Many metrics are already filtered — current_* means active-only, total_contributors excludes bots — so do not repeat those conditions in where."` GroupBy string `json:"group_by,omitempty" jsonschema:"Comma-separated dimension qualified_names, copied verbatim from explore_lfx_semantic_layer — they are entity__field and the prefix differs per metric. Group by a name dimension for a ranked list of organizations, people or projects; add metric_time__year (or __quarter, __month, __week, __day) for a trend."` Where string `json:"where,omitempty" jsonschema:"MetricFlow filter; this clause does the actual data filtering. Categorical: {{ Dimension('country__lf_region') }} = 'Europe'. Time: {{ TimeDimension('asset_id__install_date', 'DAY') }} >= '2024-01-01'. Dates are yyyy-mm-dd."` OrderBy string `json:"order_by,omitempty" jsonschema:"Comma-separated sort fields. Each must also appear in group_by or metrics. Prefix with - for descending, e.g. -current_membership_revenue."` @@ -299,13 +304,39 @@ Passing several metrics returns only the dimensions they SHARE, and that set is much smaller than either metric's own. Those shared dimensions are what a cross-domain query can group by.`, + "get_dimension_values": `get_dimension_values — list the literals a dimension can hold. + + dimension (required): one qualified_name from get_dimensions. + metrics (required): the metric you intend to query. The dimension is + checked against it, so the two must go together. + search (optional): case-insensitive substring. Keep it short — a fragment + like "viet" finds a value however it is spelled. + +Call this before filtering on any value you have not already seen in output. +An unknown literal is not an error: the query succeeds and returns zero rows, +which is indistinguishable from the data genuinely being empty. + +Stored spellings are not the everyday ones: + lf_region 'Asia Pacific', never 'APAC' + country_name 'Viet Nam', 'Korea, Republic of', 'Türkiye' — ISO spellings + +Values come from the dimension's full domain, not just rows carrying the +metric, so a value listed here can still return no rows once other filters are +applied. + +Prefer the country__* dimensions over asset_id__billing_country, which is +unnormalized free text and holds both 'Viet Nam' and 'Vietnam' alongside +entries like 'na', 'US' and 'Untied States'. Filtering on it drops members +filed under a different spelling.`, + "query": lensQueryHelp, } // lensHelpOverview is returned by help with no target. const lensHelpOverview = `LFX Insights Semantic Layer — how to use it -Workflow: list_metrics(search) → get_dimensions (only if you need more) → query. +Workflow: list_metrics(search) → get_dimensions (only if you need more) → +get_dimension_values (before filtering on an unseen value) → query. metric the number being measured dimension an attribute you group, filter or list by @@ -321,7 +352,7 @@ Dimension qualified_names are entity__field. The prefix is the primary key of the metric's own table, so it differs from metric to metric. Always copy the name from list_metrics or get_dimensions. -help targets: query, list_metrics, get_dimensions` +help targets: query, list_metrics, get_dimensions, get_dimension_values` const lensQueryHelp = `query — run a metric query. @@ -371,9 +402,10 @@ Examples order_by -total_contributors limit 10 - Region values are exact strings — group by the dimension with no filter first - to see them. lf_region is one of: North America, Europe, China, India, Japan, - Asia Pacific, Middle East & Africa, Latin America, Other. + Filter values are exact strings. lf_region is one of: North America, Europe, + China, India, Japan, Asia Pacific, Middle East & Africa, Latin America, Other. + For any other dimension use explore_lfx_semantic_layer's get_dimension_values + rather than guessing — a wrong literal returns zero rows, not an error. Contribution against financial involvement, by region, globally metrics total_contributors, total_contributing_organizations, current_membership_revenue @@ -400,6 +432,8 @@ func handleExploreSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, arg return handleLensListMetrics(ctx, args.Search) case "get_dimensions": return handleLensGetDimensions(ctx, args.Metrics, args.Search) + case "get_dimension_values": + return handleLensGetDimensionValues(ctx, args.Dimension, args.Metrics, args.Search) case "query": // Querying moved to its own tool; a caller on a cached schema would // otherwise get a bare "unknown action" with nowhere to go. @@ -409,7 +443,7 @@ func handleExploreSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, arg }, nil, nil default: return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: list_metrics, get_dimensions, help. To run a query, use the query_lfx_semantic_layer tool.", args.Action)}}, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid actions: list_metrics, get_dimensions, get_dimension_values, help. To run a query, use the query_lfx_semantic_layer tool.", args.Action)}}, IsError: true, }, nil, nil } @@ -425,7 +459,7 @@ func handleLensHelp(target string) (*mcp.CallToolResult, any, error) { text, ok := lensHelpTexts[target] if !ok { return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid targets: list_metrics, get_dimensions, query", target)}}, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid targets: list_metrics, get_dimensions, get_dimension_values, query", target)}}, IsError: true, }, nil, nil } @@ -460,6 +494,37 @@ func handleLensGetDimensions(ctx context.Context, metricsArg, search string) (*m return lensDoGet(ctx, "/lfx-lens/semantic-layer/dimensions", params) } +// handleLensGetDimensionValues lists the literals a dimension can hold. +// +// A where clause with a real dimension but an unknown value succeeds and +// returns no rows, so a wrong guess is indistinguishable from an empty result +// and gets read as "no such data". Seen live against 'APAC' (the value is +// 'Asia Pacific') and 'Vietnam' (it is 'Viet Nam'). +func handleLensGetDimensionValues(ctx context.Context, dimension, metricsArg, search string) (*mcp.CallToolResult, any, error) { + if strings.TrimSpace(dimension) == "" { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "Error: dimension is required for get_dimension_values. Pass a qualified_name from get_dimensions, e.g. country__lf_region."}}, + IsError: true, + }, nil, nil + } + + metrics := parseCSV(metricsArg) + if len(metrics) == 0 { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "Error: metrics is required for get_dimension_values — it is what the dimension is checked against. Pass the metric you intend to query."}}, + IsError: true, + }, nil, nil + } + + params := url.Values{} + params.Set("dimension", strings.TrimSpace(dimension)) + params.Set("metrics", strings.Join(metrics, ",")) + if search != "" { + params.Set("search", search) + } + return lensDoGet(ctx, "/lfx-lens/semantic-layer/dimension-values", params) +} + func handleQuerySemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, args QuerySemanticLayerArgs) (*mcp.CallToolResult, any, error) { if lensConfig == nil { return nil, nil, fmt.Errorf("LFX Lens tools not configured") diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index d9bc901..57fb39e 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" @@ -26,6 +27,7 @@ func (stubTokenSource) GetToken(_ context.Context) (string, error) { type capturedLensRequest struct { Method string Path string + Query url.Values Body []byte } @@ -40,6 +42,7 @@ func setupLensTest(t *testing.T) *capturedLensRequest { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { captured.Method = r.Method captured.Path = r.URL.Path + captured.Query = r.URL.Query() body, _ := io.ReadAll(r.Body) captured.Body = body w.Header().Set("Content-Type", "application/json") @@ -240,26 +243,56 @@ func TestExploreSemanticLayerDescription(t *testing.T) { // one-sided — query_lfx_lens lists concrete triggers while this tool // describes itself abstractly, so every specific question looks like a // better match for the other tool. - "contributions —", - "memberships —", - "events —", - "education —", - "maintainers —", - "project health —", + // Search terms must be words the Semantic Layer actually matches. + // The earlier headings were plurals — "contributions", "memberships", + // "education", "project health" all returned zero metrics, and a live + // client followed the instruction, got [], and fell back to + // query_lfx_lens. These singular forms are verified against the API. + "contributor, contribution —", + "membership, revenue, churn —", + "event, registration, sponsorship, speaker —", + "enrollment, certification —", + "maintainer —", + "health, project —", // Regional questions route here for every topic, memberships included. "any of the above sliced by country or region — always here, never query_lfx_lens", - "memberships not sliced by country or region", // Dimension naming, and the regional person-vs-organization split. "entity__field", "country__lf_region", "activity_project_id__organization_lf_region", // Discovery must hand off to the query tool by name. "query_lfx_semantic_layer", + // The value-discovery action, and the reason it exists. A filter naming + // a real dimension but an unknown literal returns zero rows instead of + // erroring, so a wrong guess is indistinguishable from missing data. A + // live client burned five query attempts on 'APAC' and 'Vietnam' before + // escaping via a country code. + "get_dimension_values(dimension, metrics, search)", + "returns zero rows, not an error", + "'Asia Pacific' not 'APAC'", + "'Viet Nam' not 'Vietnam'", + // Either tool can be loaded without the other, so each states what the + // semantic layer is. Here the regional rule sits in COVERS, asserted + // above, rather than in the opening line. + "query and data-exploration tool", } { if !strings.Contains(exploreSemanticLayerDescription, want) { t.Errorf("explore description missing %q", want) } } + + // The description used to warn that a plural search matches nothing. That + // stopped being true once lens learned to fall back to a singular stem: + // "memberships" now returns 18 metrics, "contributions" 2. Telling the model + // otherwise wastes the budget on a false constraint. + for _, unwanted := range []string{ + "a plural like", + "matches nothing", + } { + if strings.Contains(exploreSemanticLayerDescription, unwanted) { + t.Errorf("explore description still warns about plurals, which lens now handles: %q", unwanted) + } + } } // TestQuerySemanticLayerDescription checks the query tool is self-sufficient: @@ -273,12 +306,23 @@ func TestQuerySemanticLayerDescription(t *testing.T) { "yyyy-mm-dd", "ceiling 500", "metric_time__year", - "outer-joined", + "The join is outer", "ranked list", "project_slug", + // Splitting discovery out made it possible to query without ever + // exploring, and a live client did exactly that — going straight to a + // query with guessed names. The rule has to be an instruction, not a + // conditional suggestion. + "ALWAYS call explore_lfx_semantic_layer first", + "never guess", // Both neighbours are named so routing works from this tool too. "explore_lfx_semantic_layer", "query_lfx_lens", + "query and data-exploration tool", + "anything sliced by country or region", + // The silent-zero-rows warning is only actionable if it names the way + // out; without this the model retries the same wrong literal. + "get_dimension_values", } { if !strings.Contains(querySemanticLayerDescription, want) { t.Errorf("query description missing %q", want) @@ -307,7 +351,7 @@ func TestSemanticLayerArgs_FieldsFitSchemaBudget(t *testing.T) { tool *mcp.Tool props []string }{ - {listExploreTool(t), []string{"action", "search", "metrics", "target"}}, + {listExploreTool(t), []string{"action", "search", "metrics", "dimension", "target"}}, {listQueryTool(t), []string{"metrics", "group_by", "where", "order_by", "limit", "project_slug"}}, } { for _, property := range tc.props { @@ -353,6 +397,8 @@ func TestCriticalGuidanceSurvivesSchemaCompaction(t *testing.T) { {"entity__field", "dimension names cannot be assembled by hand"}, {"outer-joined", "explains NULLs in cross-domain results"}, {"raw IDs", "grouping by an entity silently returns unusable output"}, + {"get_dimension_values", "the only recovery from a wrong filter literal"}, + {"zero rows", "a wrong literal is silent, so the model must be told to check first"}, } { if !strings.Contains(surviving, tc.token) { t.Errorf("%q reaches the model only via an optional parameter, where it gets summarised away (%s). Move it into the tool description or onto a required parameter.", @@ -486,7 +532,7 @@ func TestRegisterSemanticLayer_Schema(t *testing.T) { t.Errorf("explore required = %v; metrics is only needed for get_dimensions", exploreRequired) } action := schemaPropertyDescription(t, explore, "action") - for _, want := range []string{"list_metrics", "get_dimensions", "help"} { + for _, want := range []string{"list_metrics", "get_dimensions", "get_dimension_values", "help"} { if !strings.Contains(action, want) { t.Errorf("action schema description missing %q: %q", want, action) } @@ -595,21 +641,34 @@ func TestHelpActionAndDescribeAlias(t *testing.T) { } } -// TestQueryLFXLensDescription_RegionalException guards the other half of the -// routing contract: query_lfx_lens claims memberships, and both its -// description and its input schema ship with tools/list. If they keep saying -// "always use for memberships" unconditionally, clients get instructions that -// contradict the semantic layer's regional carve-out. -func TestQueryLFXLensDescription_RegionalException(t *testing.T) { +// TestQueryLFXLensDoesNotClaimMemberships guards the other half of the routing +// contract. query_lfx_lens used to open with "Always use this tool for: +// Membership questions", carved out only for country/region. Memberships now +// belong to the semantic layer in full — 18 metrics covering revenue, counts, +// churn, discounts and invoices, sliceable and trendable like any other domain +// — so a leftover claim here produces two tools asserting ownership of the same +// question. Both the description and the input schema ship with tools/list. +func TestQueryLFXLensDoesNotClaimMemberships(t *testing.T) { tool := listRegisteredTool(t, "query_lfx_lens", RegisterQueryLFXLens) - if !strings.Contains(tool.Description, "EXCEPT country/region") { - t.Errorf("query_lfx_lens description missing the regional exception: %q", tool.Description) + for _, unwanted := range []string{ + "Always use this tool for:\n- Membership questions", + "EXCEPT country/region", + } { + if strings.Contains(tool.Description, unwanted) { + t.Errorf("query_lfx_lens description still claims memberships: %q", unwanted) + } + } + if !strings.Contains(tool.Description, "contributor, activity and membership questions belong to the semantic layer") { + t.Error("query_lfx_lens description should hand memberships to the semantic layer explicitly") } input := schemaPropertyDescription(t, tool, "input") - if !strings.Contains(input, "memberships (except country/region breakdowns)") { - t.Errorf("query_lfx_lens input schema missing the regional exception: %q", input) + if strings.Contains(input, "Always use for memberships") { + t.Errorf("query_lfx_lens input schema still claims memberships: %q", input) + } + if !strings.Contains(input, "Contributor, activity and membership questions belong to the semantic layer") { + t.Errorf("query_lfx_lens input schema should redirect memberships: %q", input) } } @@ -621,3 +680,167 @@ func contains(list []string, want string) bool { } return false } + +// --------------------------------------------------------------------------- +// get_dimension_values +// +// The action exists because a filter naming a real dimension but an unknown +// literal is not an error: the query succeeds and returns zero rows. Against a +// live client that read as "no such data" and cost five wrong-but-successful +// queries — 'APAC' for a region that is stored as 'Asia Pacific', 'Vietnam' for +// a country stored as 'Viet Nam'. +// --------------------------------------------------------------------------- + +func TestGetDimensionValuesForwardsToTheValuesEndpoint(t *testing.T) { + captured := setupLensTest(t) + + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "get_dimension_values", + Dimension: " country__lf_region ", + Metrics: " total_contributors , current_membership_revenue ", + Search: "asia", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.IsError { + t.Fatalf("unexpected error result: %s", resultText(t, res)) + } + if captured.Path != "/lfx-lens/semantic-layer/dimension-values" { + t.Errorf("unexpected request path: %s", captured.Path) + } + // Whitespace around a copied qualified_name must not reach lens, which + // rejects anything outside [A-Za-z0-9_] rather than trimming it. + if got := captured.Query.Get("dimension"); got != "country__lf_region" { + t.Errorf("dimension = %q; want it trimmed to country__lf_region", got) + } + if got := captured.Query.Get("metrics"); got != "total_contributors,current_membership_revenue" { + t.Errorf("metrics = %q; want the CSV normalised", got) + } + if got := captured.Query.Get("search"); got != "asia" { + t.Errorf("search = %q; want asia", got) + } +} + +func TestGetDimensionValuesOmitsAnEmptySearch(t *testing.T) { + captured := setupLensTest(t) + + _, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "get_dimension_values", + Dimension: "country__lf_region", + Metrics: "total_contributors", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // An empty search must be absent, not sent as "": lens turns a present + // search into an ILIKE '%%' filter and would report zero matches. + if captured.Query.Has("search") { + t.Errorf("search should be omitted when empty, got %q", captured.Query.Get("search")) + } +} + +func TestGetDimensionValuesRejectsMissingArgumentsWithAPointer(t *testing.T) { + for _, tc := range []struct { + name string + args ExploreSemanticLayerArgs + want string + }{ + { + name: "no dimension", + args: ExploreSemanticLayerArgs{Action: "get_dimension_values", Metrics: "total_contributors"}, + want: "country__lf_region", + }, + { + name: "blank dimension", + args: ExploreSemanticLayerArgs{Action: "get_dimension_values", Dimension: " ", Metrics: "total_contributors"}, + want: "country__lf_region", + }, + { + name: "no metrics", + args: ExploreSemanticLayerArgs{Action: "get_dimension_values", Dimension: "country__lf_region"}, + want: "metrics is required", + }, + } { + t.Run(tc.name, func(t *testing.T) { + setupLensTest(t) + + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, tc.args) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !res.IsError { + t.Fatal("expected an error result") + } + if text := resultText(t, res); !strings.Contains(text, tc.want) { + t.Errorf("error should show the way forward (%q): %q", tc.want, text) + } + }) + } +} + +// TestUnknownActionListsTheRealActions guards the recovery message against +// drift: it is what a model reads after guessing an action name, so an action +// missing here is one it will not retry with. +func TestUnknownActionListsTheRealActions(t *testing.T) { + setupLensTest(t) + + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "list_dimension_values", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !res.IsError { + t.Fatal("expected an error result for an unknown action") + } + text := resultText(t, res) + for _, want := range []string{"list_metrics", "get_dimensions", "get_dimension_values", "help"} { + if !strings.Contains(text, want) { + t.Errorf("unknown-action error missing %q: %q", want, text) + } + } +} + +// TestHelpCoversGetDimensionValues checks the long-form guidance is reachable. +// It is the only place that records the billing_country trap, which has no room +// in the 2048-byte description. +func TestHelpCoversGetDimensionValues(t *testing.T) { + setupLensTest(t) + + res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "help", + Target: "get_dimension_values", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.IsError { + t.Fatalf("unexpected error result: %s", resultText(t, res)) + } + text := resultText(t, res) + for _, want := range []string{ + "zero rows", + "'Asia Pacific'", + "Viet Nam", + // asset_id__billing_country is free text holding both spellings, so a + // filter on it drops members filed under the other one. The transcript + // that motivated this work "succeeded" on exactly that dimension. + "asset_id__billing_country", + } { + if !strings.Contains(text, want) { + t.Errorf("get_dimension_values help missing %q", want) + } + } + + // The overview must advertise the target, or nothing points at it. + overview, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{ + Action: "help", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if text := resultText(t, overview); !strings.Contains(text, "get_dimension_values") { + t.Errorf("help overview does not mention get_dimension_values: %q", text) + } +} From 8bb43f0cdf9d74dba9311bd4b28a7bb8faa0f959 Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Wed, 29 Jul 2026 16:41:07 +0200 Subject: [PATCH 5/6] fix(tools): honour each semantic layer tool name, and drop the sponsorship claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot review on #110. Tool selection was broken. Both tools were added by one function behind one gate keyed to "query_lfx_semantic_layer", so LFXMCP_TOOLS=explore_lfx_semantic_layer registered nothing at all, and selecting only the query tool silently exposed both. Split into RegisterExploreSemanticLayer and RegisterQuerySemanticLayer, gated on their own names. They are still meant to be enabled together — each description points at the other — but that is now a recommendation rather than something a name silently overrides. The explore description listed "sponsorship" as a covered topic while query_lfx_lens states that sponsorships belong to it. Two tools claiming the same question is worse than either answer, and query_lfx_lens does sponsorships better, so the carve-out moves to the routing line where the other handoffs already live. A test now fails if the topic list reclaims them. The describe-alias comment claimed more than the code delivers. A caller on the pre-split schema is addressing query_lfx_semantic_layer, which no longer takes an action and cannot reach the alias at all. Restoring that path would mean making metrics optional again, which is the compaction protection the split exists to gain — so the comment and the test now say what is actually covered, and the stale-schema case is left to resolve itself on the client's next tool list refresh. LFXV2-2893: https://linuxfoundation.atlassian.net/browse/LFXV2-2893 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- cmd/lfx-mcp-server/main.go | 10 ++++-- internal/tools/lens.go | 31 ++++++++++++++----- internal/tools/lens_test.go | 62 ++++++++++++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 19 deletions(-) diff --git a/cmd/lfx-mcp-server/main.go b/cmd/lfx-mcp-server/main.go index d436a46..b5a025a 100644 --- a/cmd/lfx-mcp-server/main.go +++ b/cmd/lfx-mcp-server/main.go @@ -798,10 +798,14 @@ func newServer(cfg Config, serviceName string, callerToken *auth.TokenInfo) *mcp if enabledTools["query_lfx_lens"] && canRead && isStaff { tools.RegisterQueryLFXLens(server) } - // RegisterSemanticLayer adds both explore_lfx_semantic_layer and - // query_lfx_semantic_layer; they are a pair and share the same gate. + // Each semantic layer tool honours its own name in LFXMCP_TOOLS. They are + // intended to be enabled together — each description points at the other — + // but gating both on one name made the other name select nothing. + if enabledTools["explore_lfx_semantic_layer"] && canRead && isStaff { + tools.RegisterExploreSemanticLayer(server) + } if enabledTools["query_lfx_semantic_layer"] && canRead && isStaff { - tools.RegisterSemanticLayer(server) + tools.RegisterQuerySemanticLayer(server) } return server diff --git a/internal/tools/lens.go b/internal/tools/lens.go index 4f25735..a9f17bc 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -168,7 +168,7 @@ const exploreSemanticLayerDescription = `The LFX Insights Semantic Layer is the COVERS — search one of these topic words: - contributor, contribution — activity and org counts, commits, PRs - membership, revenue, churn — counts, discounts, invoices -- event, registration, sponsorship, speaker — counts and revenue +- event, registration, speaker — counts and revenue - enrollment, certification — education - maintainer — total and active counts - health, project — health scores, software value, cost @@ -182,7 +182,7 @@ ACTIONS - get_dimension_values(dimension, metrics, search): the literals a dimension holds. Call it before filtering on any value not already seen in output: an unknown literal returns zero rows, not an error, so a wrong guess reads as missing data. Spellings surprise — 'Asia Pacific' not 'APAC', 'Viet Nam' not 'Vietnam'. - help(target): worked query examples, for when a query fails. -USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends/names.` +USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends, event sponsorships.` const querySemanticLayerDescription = `The LFX Insights Semantic Layer is the query and data-exploration tool for Linux Foundation data; this half runs the query. Covers contributions, memberships, events, education, maintainers and project health — and anything sliced by country or region. ALWAYS call explore_lfx_semantic_layer first unless you already have the exact metric, dimension and entity names — never guess or assemble one: a wrong name errors, and a wrong filter value returns no rows rather than an error, so confirm literals with get_dimension_values. Use query_lfx_lens for narrative or "why" questions that do not reduce to a metric. @@ -198,17 +198,24 @@ const querySemanticLayerDescription = `The LFX Insights Semantic Layer is the qu Many metrics are pre-filtered — current_* is active-only, total_contributors excludes bots — so do not re-filter those. Entities listed with a metric are join keys, not group-by values: grouping by one returns raw IDs, so use the name dimension.` -// RegisterSemanticLayer registers the two semantic layer tools. The -// registration gate in cmd/lfx-mcp-server limits both to staff callers, so +// The two semantic layer tools register independently so that LFXMCP_TOOLS can +// select either by name. They are meant to be enabled together — each +// description points at the other — but a shared gate would mean the name +// "explore_lfx_semantic_layer" registered nothing while +// "query_lfx_semantic_layer" silently registered both. +// +// The registration gate in cmd/lfx-mcp-server limits both to staff callers, so // project scoping is optional here; lfx-lens validates any project filters that // are provided against the requested foundation's subtree. // // Discovery and querying are separate tools rather than actions on one tool // because a tool description and its required parameters are the only guidance -// that reaches the model intact — see the note on SemanticLayerLFXLensArgs. +// that reaches the model intact — see the note on QuerySemanticLayerArgs. // Splitting gives the query its own description to hold the MetricFlow syntax, // and makes metrics genuinely required there rather than optional. -func RegisterSemanticLayer(server *mcp.Server) { + +// RegisterExploreSemanticLayer registers the explore_lfx_semantic_layer tool. +func RegisterExploreSemanticLayer(server *mcp.Server) { mcp.AddTool(server, &mcp.Tool{ Name: "explore_lfx_semantic_layer", Description: exploreSemanticLayerDescription, @@ -217,7 +224,10 @@ func RegisterSemanticLayer(server *mcp.Server) { ReadOnlyHint: true, }, }, handleExploreSemanticLayer) +} +// RegisterQuerySemanticLayer registers the query_lfx_semantic_layer tool. +func RegisterQuerySemanticLayer(server *mcp.Server) { mcp.AddTool(server, &mcp.Tool{ Name: "query_lfx_semantic_layer", Description: querySemanticLayerDescription, @@ -424,8 +434,13 @@ func handleExploreSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, arg } switch args.Action { - // "describe" is the pre-rename name for help, kept so a caller working - // from a cached schema does not get an Unknown action error. + // "describe" is the pre-rename name for help. It only helps a caller that + // has this tool but reuses the old action word — a caller still on the + // pre-split schema is addressing query_lfx_semantic_layer, which no longer + // takes an action at all and cannot reach here. Restoring that path would + // mean making metrics optional again on the query tool, which is exactly + // the compaction protection the split exists to get, so the stale-schema + // case is left to resolve itself when the client refreshes its tool list. case "help", "describe": return handleLensHelp(args.Target) case "list_metrics": diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index 57fb39e..76e5074 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -250,7 +250,7 @@ func TestExploreSemanticLayerDescription(t *testing.T) { // query_lfx_lens. These singular forms are verified against the API. "contributor, contribution —", "membership, revenue, churn —", - "event, registration, sponsorship, speaker —", + "event, registration, speaker —", "enrollment, certification —", "maintainer —", "health, project —", @@ -281,6 +281,14 @@ func TestExploreSemanticLayerDescription(t *testing.T) { } } + // Event sponsorships stay with query_lfx_lens, which does them better, so + // this tool must not advertise them. Listing "sponsorship" as a topic here + // put two tools in charge of the same question and contradicted the + // carve-out query_lfx_lens still states. + if strings.Contains(exploreSemanticLayerDescription, "sponsorship,") { + t.Error("explore description claims sponsorships as a topic; query_lfx_lens owns them") + } + // The description used to warn that a plural search matches nothing. That // stopped being true once lens learned to fall back to a singular stem: // "memberships" now returns 18 metrics, "contributions" 2. Telling the model @@ -415,8 +423,8 @@ func TestBothLensToolDescriptionsFitBudget(t *testing.T) { name string register func(*mcp.Server) }{ - {"explore_lfx_semantic_layer", RegisterSemanticLayer}, - {"query_lfx_semantic_layer", RegisterSemanticLayer}, + {"explore_lfx_semantic_layer", RegisterExploreSemanticLayer}, + {"query_lfx_semantic_layer", RegisterQuerySemanticLayer}, {"query_lfx_lens", RegisterQueryLFXLens}, } { tool := listRegisteredTool(t, tc.name, tc.register) @@ -433,16 +441,27 @@ func TestBothLensToolDescriptionsFitBudget(t *testing.T) { func listExploreTool(t *testing.T) *mcp.Tool { t.Helper() - return listRegisteredTool(t, "explore_lfx_semantic_layer", RegisterSemanticLayer) + return listRegisteredTool(t, "explore_lfx_semantic_layer", RegisterExploreSemanticLayer) } func listQueryTool(t *testing.T) *mcp.Tool { t.Helper() - return listRegisteredTool(t, "query_lfx_semantic_layer", RegisterSemanticLayer) + return listRegisteredTool(t, "query_lfx_semantic_layer", RegisterQuerySemanticLayer) } +// listRegisteredTool returns the named tool, failing the test if it is absent. func listRegisteredTool(t *testing.T, name string, register func(*mcp.Server)) *mcp.Tool { t.Helper() + tool := findRegisteredTool(t, name, register) + if tool == nil { + t.Fatalf("%s not found in tool list", name) + } + return tool +} + +// findRegisteredTool returns the named tool, or nil when it is not registered. +func findRegisteredTool(t *testing.T, name string, register func(*mcp.Server)) *mcp.Tool { + t.Helper() server := mcp.NewServer(&mcp.Implementation{ Name: "test-server", @@ -474,7 +493,6 @@ func listRegisteredTool(t *testing.T, name string, register func(*mcp.Server)) * return tool } } - t.Fatalf("%s not found in tool list", name) return nil } @@ -610,8 +628,9 @@ func TestExploreToolRedirectsQueryAction(t *testing.T) { } // TestHelpActionAndDescribeAlias checks the renamed action works and that the -// old name still dispatches, so a client working from a cached schema does not -// hit an Unknown action error. +// old action word still dispatches on this tool. It deliberately does NOT claim +// to cover the pre-split schema: that caller addresses query_lfx_semantic_layer, +// which no longer accepts an action, so no assertion here can exercise it. func TestHelpActionAndDescribeAlias(t *testing.T) { setupLensTest(t) @@ -844,3 +863,30 @@ func TestHelpCoversGetDimensionValues(t *testing.T) { t.Errorf("help overview does not mention get_dimension_values: %q", text) } } + +// TestSemanticLayerToolsRegisterIndependently guards tool selection. +// +// Both tools used to be added by one function behind one gate keyed to +// "query_lfx_semantic_layer", so LFXMCP_TOOLS=explore_lfx_semantic_layer +// registered nothing at all, and selecting only the query tool silently +// exposed both. Each name must control exactly its own tool. +func TestSemanticLayerToolsRegisterIndependently(t *testing.T) { + for _, tc := range []struct { + name string + register func(*mcp.Server) + absent string + }{ + {"explore_lfx_semantic_layer", RegisterExploreSemanticLayer, "query_lfx_semantic_layer"}, + {"query_lfx_semantic_layer", RegisterQuerySemanticLayer, "explore_lfx_semantic_layer"}, + } { + t.Run(tc.name, func(t *testing.T) { + if tool := listRegisteredTool(t, tc.name, tc.register); tool == nil { + t.Fatalf("%s did not register itself", tc.name) + } + if found := findRegisteredTool(t, tc.absent, tc.register); found != nil { + t.Errorf("registering %s also exposed %s; each name must select only its own tool", + tc.name, tc.absent) + } + }) + } +} From 525253bcd9b0325967b2ab613281c82e4509a14b Mon Sep 17 00:00:00 2001 From: Josep Garcia-Reyero Sais Date: Wed, 29 Jul 2026 16:50:05 +0200 Subject: [PATCH 6/6] fix(tools): correct stale references and misleading wording in the lens tools Addresses the five low-confidence findings on the Copilot re-review of #110. All were correct. The action=query branch carried the same overclaim just corrected on the describe alias: a caller on the pre-split schema addresses query_lfx_semantic_layer and cannot reach a branch on the explore tool. Fixing one and leaving the other was an inconsistency, so the comment now points at the describe note rather than repeating a claim that does not hold. The PR description is updated to match. lensHelpTexts referenced semanticLayerDescription, a constant the split removed, so the rule about where first-query guidance lives named a symbol that no longer exists. An unknown help target reported "Unknown action", naming a field that was in fact valid and pointing the caller at the wrong argument to change. "Several returns only the ones they share" was a fragment left by an earlier trim for bytes; a description the model reads to decide what to call should not need parsing twice. Paid for by shortening "subproject exploration" to "subprojects" on the routing line. TestBothLensToolDescriptionsFitBudget covers three tools, so "Both" was wrong. LFXV2-2893: https://linuxfoundation.atlassian.net/browse/LFXV2-2893 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Josep Garcia-Reyero Sais --- internal/tools/lens.go | 16 +++++++++------- internal/tools/lens_test.go | 6 +++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/tools/lens.go b/internal/tools/lens.go index a9f17bc..22e084f 100644 --- a/internal/tools/lens.go +++ b/internal/tools/lens.go @@ -178,11 +178,11 @@ A metric is the number measured (total_contributors); a dimension is how you sli ACTIONS - list_metrics(search): searches metric names and descriptions only, so search a topic word above, not a dimension word like "country". When 15 or fewer match, each returns its dimension qualified_names — usually enough to query. -- get_dimensions(metrics, search): dimensions available to those metrics; needs at least one. Several returns only the ones they share — what a cross-domain query can group by. +- get_dimensions(metrics, search): dimensions available to those metrics; needs at least one. Passing several returns only the ones they share — what a cross-domain query can group by. - get_dimension_values(dimension, metrics, search): the literals a dimension holds. Call it before filtering on any value not already seen in output: an unknown literal returns zero rows, not an error, so a wrong guess reads as missing data. Spellings surprise — 'Asia Pacific' not 'APAC', 'Viet Nam' not 'Vietnam'. - help(target): worked query examples, for when a query fails. -USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subproject exploration, maintainer trends, event sponsorships.` +USE query_lfx_lens INSTEAD for questions that do not reduce to a metric above: narrative or "why", subprojects, maintainer trends, event sponsorships.` const querySemanticLayerDescription = `The LFX Insights Semantic Layer is the query and data-exploration tool for Linux Foundation data; this half runs the query. Covers contributions, memberships, events, education, maintainers and project health — and anything sliced by country or region. ALWAYS call explore_lfx_semantic_layer first unless you already have the exact metric, dimension and entity names — never guess or assemble one: a wrong name errors, and a wrong filter value returns no rows rather than an error, so confirm literals with get_dimension_values. Use query_lfx_lens for narrative or "why" questions that do not reduce to a metric. @@ -279,8 +279,8 @@ type QuerySemanticLayerArgs struct { // lensHelpTexts back the help action. These are tool results, so they carry no // character budget — but they are a fallback, not a prerequisite: everything -// needed to compose a first query lives in semanticLayerDescription and the -// per-parameter descriptions. +// needed to compose a first query lives in exploreSemanticLayerDescription, +// querySemanticLayerDescription and the per-parameter descriptions. var lensHelpTexts = map[string]string{ "list_metrics": `list_metrics — discover metrics. Always the first call. @@ -450,8 +450,10 @@ func handleExploreSemanticLayer(ctx context.Context, _ *mcp.CallToolRequest, arg case "get_dimension_values": return handleLensGetDimensionValues(ctx, args.Dimension, args.Metrics, args.Search) case "query": - // Querying moved to its own tool; a caller on a cached schema would - // otherwise get a bare "unknown action" with nowhere to go. + // Reachable only from a caller that already has this tool and reused + // the old action word; a caller still on the pre-split schema is + // addressing query_lfx_semantic_layer and never lands here. See the + // note on the describe alias above. return &mcp.CallToolResult{ Content: []mcp.Content{&mcp.TextContent{Text: "Querying moved to the query_lfx_semantic_layer tool. Call it directly with metrics, group_by, where, order_by and limit."}}, IsError: true, @@ -474,7 +476,7 @@ func handleLensHelp(target string) (*mcp.CallToolResult, any, error) { text, ok := lensHelpTexts[target] if !ok { return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown action %q. Valid targets: list_metrics, get_dimensions, get_dimension_values, query", target)}}, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Unknown help target %q. Valid targets: list_metrics, get_dimensions, get_dimension_values, query", target)}}, IsError: true, }, nil, nil } diff --git a/internal/tools/lens_test.go b/internal/tools/lens_test.go index 76e5074..8a43d4c 100644 --- a/internal/tools/lens_test.go +++ b/internal/tools/lens_test.go @@ -415,10 +415,10 @@ func TestCriticalGuidanceSurvivesSchemaCompaction(t *testing.T) { } } -// TestBothLensToolDescriptionsFitBudget guards every description that ships in +// TestAllLensToolDescriptionsFitBudget guards every description that ships in // tools/list, not just the semantic layer's. query_lfx_lens has far less -// headroom and is the likelier of the two to drift past the cut unnoticed. -func TestBothLensToolDescriptionsFitBudget(t *testing.T) { +// headroom and is the likeliest to drift past the cut unnoticed. +func TestAllLensToolDescriptionsFitBudget(t *testing.T) { for _, tc := range []struct { name string register func(*mcp.Server)