feat(tools): split the semantic layer into discovery and query tools, and add value discovery [LFXV2-2893] by josep-reyero · Pull Request #110 · linuxfoundation/lfx-mcp

josep-reyero · 2026-07-29T14:24:48Z

Jira: LFXV2-2893 — under epic LFXV2-2891 (LFX MCP: migrate away from text to SQL)

Depends on linuxfoundation/lfx-lens#28 — already merged and deployed to prod as lfx-lens 0.5.5, so this is unblocked.

Why

Four separate failures were pushing callers off the semantic layer and back onto query_lfx_lens, i.e. back onto text-to-SQL — the exact fallback this epic exists to remove. Each was found by watching a live client fail, not by reading the code.

1. Descriptions were silently truncated

The production query_lfx_semantic_layer description was 3,662 characters against a 2,048 limit — 44% of it never reached the model. That is how earlier guidance (the tlf membership caveat, the project_name tip) went unread for as long as it did.

2. Optional parameter descriptions are summarised away

Clients that defer tool schemas behind a search index 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.", order_by as "Sort order.", and limit had none at all — until limit was temporarily marked required, at which point its real text appeared. That experiment is what pinned the cause down.

Only the tool description and required parameters survive intact. So MetricFlow filter syntax, which the model cannot guess, must not live solely on an optional parameter.

3. The documented search terms matched no metrics

Four of six domain headings — contributions, memberships, education, project health — returned zero metrics. A live client followed the instruction, got [], and fell back to the SQL agent.

4. A wrong filter literal fails silently

A where clause naming a real dimension but a value that does not exist is not an error: the query succeeds and returns zero rows. An ED question about Vietnam burned five wrong-but-successful queries on 'APAC' (stored 'Asia Pacific') and 'Vietnam' (stored 'Viet Nam').

What changed

Split into two tools (74edac4) so each gets its own 2,048-byte budget, and so the query syntax lives in a tool description rather than on an optional parameter:

	required	description
`explore_lfx_semantic_layer`	`action`	2,043 bytes
`query_lfx_semantic_layer`	`metrics`	2,032 bytes

action and metrics are required precisely because they are the fields that survive compaction — they carry the action list, the silent-zero-rows rule, and the multi-metric join semantics.

New get_dimension_values action (325e916) calling the lens endpoint shipped in lfx-lens#28. Lists the literals a dimension holds, with substring search.

Verified search terms replacing the four headings that returned nothing.

Explicit explore-first rule, after a live client went straight to querying with guessed names.

Memberships routed wholly to the semantic layer — the query_lfx_lens carve-out is removed. 18 metrics covering revenue, counts, churn, discounts and invoices, sliceable and trendable like any other domain.

Plural-search warning dropped — lens now falls back to a singular stem, so the claim was false and was spending budget the new action needed.

Deliberately left in query_lfx_lens: event sponsorships, and maintainers joined to activities. The latter compiles in the semantic layer but the shared dimensions carry no maintainer identity, so the SQL agent genuinely does it better.

Result

The Vietnam question, before and after:

	Before	After
CNCF members in Vietnam	3 attempts, wrong dimension	`country__country_name = 'Viet Nam'` → AgileOps, Viettel
Vietnamese LF contributors	3 attempts → 0 rows, escaped via country code	first try → FPT 113, Viettel 37, FPT University 32

Five rounds collapse to two value lookups and two queries, each right first time. Confirmed in Claude Desktop: the model reached for get_dimension_values on its own rather than guessing.

Testing

go build, go vet, gofmt and the full suite clean. Notable tests:

TestSemanticLayerDescriptions_FitSchemaBudget — enforces the limit in bytes, not chars (em-dashes cost 3, which is how a 2,040-char description came in at 2,070 bytes).
TestCriticalGuidanceSurvivesSchemaCompaction — walks both tool descriptions plus required parameters and fails if guidance the model cannot guess is reachable only via an optional parameter. This is the load-bearing test for where guidance is allowed to live.
TestQueryLFXLensDoesNotClaimMemberships — stops two tools asserting ownership of the same question.
Retired action words (describe, action=query) redirect rather than erroring — but only for a caller that already has the new tool and reused the old word. A caller still on the pre-split schema addresses query_lfx_semantic_layer, which no longer takes an action, so it cannot reach those branches. Supporting it would mean making metrics optional again and forfeiting the compaction protection the split exists to gain, so that case is left to resolve itself on the client's next tool-list refresh.

🤖 Generated with Claude Code

…s dimension 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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

Copilot

Pull request overview

Splits semantic-layer discovery from querying and adds dimension-value discovery to prevent incorrect silent-zero results.

Changes:

Adds separate explore and query tools.
Adds get_dimension_values and updated routing guidance.
Adds schema-budget and compatibility tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`README.md`	Documents both semantic-layer tools.
`internal/tools/lens.go`	Implements split tools, value discovery, and guidance.
`internal/tools/lens_test.go`	Expands schema, routing, and endpoint tests.
`cmd/lfx-mcp-server/main.go`	Enables and registers the explore tool.

josep-reyero · 2026-07-29T14:41:56Z

+type QuerySemanticLayerArgs struct {
+	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."`


Half agreed, and fixed as a documentation correction in 8bb43f0 rather than a code change.

You are right that the claim does not hold: a caller on the pre-split schema is addressing query_lfx_semantic_layer, which no longer takes an action at all, so the alias on the explore tool cannot receive that call.

I did not take the first branch of your suggestion. Preserving a compatibility path on query_lfx_semantic_layer means keeping metrics optional, and optional parameter descriptions are exactly what gets summarised away by clients that defer tool schemas — verified live, a 459-byte where description arrived as "Filter conditions.". That protection is the reason for the split, so trading it back for a transient window costs more than it saves. MCP clients refresh their tool list, so the stale schema self-heals.

So I took the second branch: the comment now states what the alias actually covers (a caller with this tool reusing the old action word) and why the pre-split case is deliberately not handled.

josep-reyero · 2026-07-29T14:41:54Z

+	// 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)


Fixed in 8bb43f0. This was the real bug of the five. Split into RegisterExploreSemanticLayer and RegisterQuerySemanticLayer, each gated on its own name.

TestSemanticLayerToolsRegisterIndependently covers both directions: each name registers its own tool, and does not register the other. They are still meant to be enabled together, but that is now a recommendation in a comment rather than something a name silently overrides.

josep-reyero · 2026-07-29T14:41:58Z

+	for _, action := range []string{"help", "describe"} {
+		res, _, err := handleExploreSemanticLayer(context.Background(), &mcp.CallToolRequest{}, ExploreSemanticLayerArgs{
+			Action: action,
+		})


Fixed in 8bb43f0 as a scope correction. You are right that the test did not cover what the comment claimed. Rather than assert the old payload against the query tool — which cannot pass, since metrics is required by design — the doc comment now says explicitly that this test does not cover the pre-split schema and why no assertion here can. See the reply on lens.go:262 for the reasoning.

…rship claim 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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

internal/tools/lens.go:283

This comment references semanticLayerDescription, which was removed by the split. The first-query guidance now lives across the two tool descriptions, so naming the deleted constant makes the maintenance rule misleading.

// 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.

internal/tools/lens.go:456

The PR description still says the stale-schema action=query path redirects, but the production pre-split schema addresses query_lfx_semantic_layer; this branch belongs to explore_lfx_semantic_layer and cannot receive that payload. Lines 437–443 correctly document that pre-split clients are intentionally unsupported, so update the PR description to avoid promising compatibility that is not present.

		// 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."}},

internal/tools/lens_test.go:421

This test now covers three registered Lens tools, so Both is inaccurate and will become more confusing as the set grows.

// 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) {

internal/tools/lens.go:477

At this point the action is valid (help); only its target is unknown. Reporting “Unknown action” misidentifies the invalid field and can prompt callers to change the wrong argument.

	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)}},

internal/tools/lens.go:181

“Several returns” is grammatically incomplete and leaves it unclear that this refers to passing multiple metrics.

- 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.

…ns 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) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

josep-reyero and others added 4 commits July 28, 2026 19:11

Copilot AI review requested due to automatic review settings July 29, 2026 14:24

claude Bot reviewed Jul 29, 2026

View reviewed changes

josep-reyero added the ai-assisted A task or activity that was supported by AI, such as CoPilot, ChatGPT, or other AI technology. label Jul 29, 2026

Copilot started reviewing on behalf of josep-reyero July 29, 2026 14:25 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 29, 2026 14:41

Copilot started reviewing on behalf of josep-reyero July 29, 2026 14:42 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 29, 2026 14:50

Copilot started reviewing on behalf of josep-reyero July 29, 2026 14:50 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

emsearcy approved these changes Jul 29, 2026

View reviewed changes

josep-reyero merged commit 9df6dd0 into main Jul 30, 2026
8 checks passed

josep-reyero deleted the feature/LFXV2-2893-semantic-layer-tool-guidance branch July 30, 2026 11:37

Navigation Menu

Uh oh!

feat(tools): split the semantic layer into discovery and query tools, and add value discovery [LFXV2-2893] - #110

feat(tools): split the semantic layer into discovery and query tools, and add value discovery [LFXV2-2893]#110
josep-reyero merged 6 commits into
mainfrom
feature/LFXV2-2893-semantic-layer-tool-guidance

josep-reyero commented Jul 29, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

josep-reyero Jul 29, 2026

Uh oh!

josep-reyero Jul 29, 2026

Uh oh!

josep-reyero Jul 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		type QuerySemanticLayerArgs struct {
		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."`

Uh oh!

Conversation

josep-reyero commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

1. Descriptions were silently truncated

2. Optional parameter descriptions are summarised away

3. The documented search terms matched no metrics

4. A wrong filter literal fails silently

What changed

Result

Testing

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

josep-reyero Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

josep-reyero Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

josep-reyero Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josep-reyero commented Jul 29, 2026 •

edited

Loading