From 6289fa5e500c5ece8a4559370c35acaa7e31489a Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 19 Aug 2026 15:50:09 +0000 Subject: [PATCH 1/5] Name the right namespace when a measure or column does not resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Columns and saved measures share one namespace but take opposite syntax: a column needs a colon suffix, a saved measure must not carry one. Neither error said which kind the name actually was, so a caller that guessed wrong was told to try the syntax that fails for the other kind. Observed in production. An agent asked for `csat`, which does not exist — the measure is `csat_pct`. The bare-name error told it to use colon syntax, so it retried `nps:avg`, then applied `:sum` to every field, which broke the saved measures that had been resolving correctly. Both messages now consult the other namespace before giving advice: * a bare name that is no saved measure suggests the closest one, rather than asserting that colon syntax is the fix; * `measure:agg` on a saved measure says to drop the suffix, instead of reporting the measure as a missing column; * an unknown column suggests the closest column or measure. Threading the saved-measure names into the parser also covers the transform and mixed-arithmetic paths, so the suggestion works at any nesting depth. Also drops the "`advanced_search` extra not installed" warning from search responses. It reports a deployment configuration the caller cannot act on, so it goes to the operator log instead. Co-Authored-By: Claude Opus 5 (1M context) --- slayer/core/formula.py | 53 ++++++++++++++++++-------- slayer/engine/enrichment.py | 29 +++++++++++++- slayer/search/retrievers/embeddings.py | 14 ++++--- tests/integration/test_integration.py | 4 +- tests/integration/test_mcp_inspect.py | 2 +- tests/test_formula.py | 8 ++-- tests/test_named_measures.py | 40 ++++++++++++++++++- 7 files changed, 119 insertions(+), 31 deletions(-) diff --git a/slayer/core/formula.py b/slayer/core/formula.py index d41dd5a2..e3a7fa07 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -13,6 +13,7 @@ """ import ast +import difflib import io import re import tokenize @@ -600,13 +601,38 @@ def parse_formula( except SyntaxError as e: raise ValueError(f"Invalid formula syntax: {formula!r} — {e}") - return _parse_node(tree.body, original=formula, agg_refs=agg_refs) + return _parse_node( + tree.body, + original=formula, + agg_refs=agg_refs, + known_measures=frozenset(named_measures or ()), + ) + + +def _bare_name_message(name: str, known_measures: frozenset[str]) -> str: + """Explain a bare name that expanded to no saved measure. + + A bare name is only ever a saved measure, and by this point expansion has + already failed, so the name is either misspelled or a column. Suggesting a + real measure first stops the reader from adding a colon suffix that a + measure would reject anyway. + """ + suggestion = difflib.get_close_matches( + word=name, possibilities=sorted(known_measures), n=1 + ) + hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" + return ( + f"'{name}' is not a saved measure.{hint} Reference a saved measure by its " + f"bare name, or aggregate a column with colon syntax (e.g., '{name}:sum'). " + f"For COUNT(*), use '*:count'." + ) def _parse_node( node: ast.AST, original: str, agg_refs: dict[str, AggregatedMeasureRef] | None = None, + known_measures: frozenset[str] = frozenset(), ) -> FieldSpec: """Recursively parse an AST node into a FieldSpec.""" if agg_refs is None: @@ -616,12 +642,7 @@ def _parse_node( if isinstance(node, ast.Name): if node.id in agg_refs: return agg_refs[node.id] - name = node.id - raise ValueError( - f"Bare measure name '{name}' is not valid. " - f"Use colon syntax (e.g., '{name}:sum', '{name}:avg'). " - f"For COUNT(*), use '*:count'." - ) + raise ValueError(_bare_name_message(node.id, known_measures)) # Dotted name → cross-model measure must include aggregation if isinstance(node, ast.Attribute) and isinstance(node.value, ast.Name): @@ -643,7 +664,7 @@ def _parse_node( # _parse_mixed_arithmetic so inner aggregated refs and nested # transforms are registered/extracted. _validate_scalar_call(node, original) - return _parse_mixed_arithmetic(node, original, agg_refs) + return _parse_mixed_arithmetic(node, original, agg_refs, known_measures) if category != "transform": raise ValueError( f"Unknown function '{func_name}' in formula {original!r}. " @@ -656,7 +677,7 @@ def _parse_node( raise ValueError(f"Transform '{func_name}' requires at least one argument (the measure)") # First arg is the measure/expression being transformed - inner = _parse_node(node.args[0], original, agg_refs) + inner = _parse_node(node.args[0], original, agg_refs, known_measures) # Remaining positional args are transform parameters (offset, granularity, etc.) # The rank family is keyword-only after the measure; reject extra positionals @@ -682,7 +703,7 @@ def _parse_node( # Binary/unary/comparison/boolean operation → check if it contains transform calls if isinstance(node, (ast.BinOp, ast.UnaryOp, ast.Compare, ast.BoolOp)): if _contains_call(node): - return _parse_mixed_arithmetic(node, original, agg_refs) + return _parse_mixed_arithmetic(node, original, agg_refs, known_measures) measure_names = _collect_names(node) # Reject bare measure names (not from colon syntax preprocessing) for mname in measure_names: @@ -692,11 +713,7 @@ def _parse_node( f"Cross-model measure '{mname}' must include an aggregation " f"(e.g., '{mname}:sum')." ) - raise ValueError( - f"Bare measure name '{mname}' is not valid. " - f"Use colon syntax (e.g., '{mname}:sum', '{mname}:avg'). " - f"For COUNT(*), use '*:count'." - ) + raise ValueError(_bare_name_message(mname, known_measures)) field_agg_refs = {n: agg_refs[n] for n in measure_names if n in agg_refs} return ArithmeticField( sql=ast.unparse(node), @@ -728,12 +745,13 @@ def _replace_calls_in_arith( counter: list[int], agg_refs: dict[str, AggregatedMeasureRef], original: str, + known_measures: frozenset[str] = frozenset(), ) -> ast.AST: """Walk the AST, replacing transform Call nodes with Name placeholders.""" if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id in ALL_TRANSFORMS: placeholder = f"_t{counter[0]}" counter[0] += 1 - transform = _parse_node(node, original, agg_refs) + transform = _parse_node(node, original, agg_refs, known_measures) sub_transforms.append((placeholder, transform)) return ast.Name(id=placeholder, ctx=ast.Load()) @@ -743,6 +761,7 @@ def _replace_calls_in_arith( "counter": counter, "agg_refs": agg_refs, "original": original, + "known_measures": known_measures, } if isinstance(node, ast.Name): @@ -795,6 +814,7 @@ def _parse_mixed_arithmetic( node: ast.AST, original: str, agg_refs: dict[str, AggregatedMeasureRef] | None = None, + known_measures: frozenset[str] = frozenset(), ) -> MixedArithmeticField: """Parse arithmetic that contains transform calls. @@ -815,6 +835,7 @@ def _parse_mixed_arithmetic( counter=counter, agg_refs=agg_refs, original=original, + known_measures=known_measures, ) modified_sql = ast.unparse(modified) diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index bae49204..eed17517 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -170,6 +170,29 @@ def _public_field_name(qfield: Any) -> str: ) +def _unknown_column_message( + *, model: Any, measure_name: str, aggregation_name: str +) -> str: + """Explain why ``measure_name:aggregation_name`` did not resolve. + + Columns and saved measures share one namespace but take opposite syntax: + a column needs the colon suffix, a measure must not carry one. Naming the + kind that does exist turns a dead end into a one-step correction. + """ + if model.get_measure(measure_name) is not None: + return ( + f"'{measure_name}' is a saved measure on model '{model.name}', not a " + f"column, so it takes no aggregation. Reference it as '{measure_name}' " + f"instead of '{measure_name}:{aggregation_name}'." + ) + known = sorted( + {c.name for c in model.columns} | {m.name for m in model.measures} + ) + suggestion = difflib.get_close_matches(word=measure_name, possibilities=known, n=1) + hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" + return f"Column '{measure_name}' not found in model '{model.name}'.{hint}" + + async def enrich_query( query: SlayerQuery, model: SlayerModel, @@ -461,7 +484,11 @@ async def _ensure_aggregated_measure( measure_def = model.get_column(measure_name) if measure_def is None: raise ValueError( - f"Column '{measure_name}' not found in model '{model.name}'" + _unknown_column_message( + model=model, + measure_name=measure_name, + aggregation_name=aggregation_name, + ) ) # DEV-1576 §3: distinguish "unknown aggregation name" from "known # but not allowed for this column type". The name check runs BEFORE diff --git a/slayer/search/retrievers/embeddings.py b/slayer/search/retrievers/embeddings.py index 44000031..9c755ef6 100644 --- a/slayer/search/retrievers/embeddings.py +++ b/slayer/search/retrievers/embeddings.py @@ -192,10 +192,9 @@ async def retrieve( and entity rankings from a single ``fetch_corpus`` + ``embed_question`` + dim-check (Codex Finding 1). - Skipped (with a warning) when: + Skipped when ``question`` is blank or the ``advanced_search`` extra is + unavailable, and skipped with a warning when: - * ``question`` is blank, - * the ``advanced_search`` extra is not installed, * the active model has no embedding rows in storage, * the query embedding call fails, * dim mismatch between query vec and corpus. @@ -203,11 +202,14 @@ async def retrieve( if corpus is None or not question or not question.strip(): return RetrievalResult() if not embedding_client.is_available(): - return RetrievalResult(warnings=[ + # Deployment configuration, not something the caller can act on: + # log it for the operator rather than returning it as a warning. + _log.warning( "embedding channel skipped: `advanced_search` extra not " "installed or no API key configured for the active " - "embedding model.", - ]) + "embedding model." + ) + return RetrievalResult() rows = await self.fetch_corpus() if datasource is not None: diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index 77163e0d..e95b1f74 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -3844,7 +3844,7 @@ async def test_round_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="round(amount, 2)", name="r")], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): await engine.execute(query) @@ -3855,5 +3855,5 @@ async def test_abs_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="abs(amount)", name="a")], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): await engine.execute(query) diff --git a/tests/integration/test_mcp_inspect.py b/tests/integration/test_mcp_inspect.py index c69301bd..3aeb001b 100644 --- a/tests/integration/test_mcp_inspect.py +++ b/tests/integration/test_mcp_inspect.py @@ -315,7 +315,7 @@ async def test_full_response(self, env) -> None: # No leaked error artefacts from the old implementation assert "sample_data_error" not in result - assert "Bare measure name" not in result + assert "is not a saved measure" not in result async def test_no_longer_json(self, env) -> None: server = create_mcp_server(storage=env["storage"]) diff --git a/tests/test_formula.py b/tests/test_formula.py index 07e8d434..3aac090e 100644 --- a/tests/test_formula.py +++ b/tests/test_formula.py @@ -20,11 +20,11 @@ class TestFormulaParser: def test_bare_measure_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("count") def test_bare_measure_in_arithmetic_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("revenue / count") def test_aggregated_measure(self) -> None: @@ -350,7 +350,7 @@ def test_not_substituted_when_called(self) -> None: assert result.inner.measure_name == "revenue" def test_unknown_bare_name_still_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula( "unknown_thing", named_measures={"aov": "revenue:sum"} ) @@ -360,7 +360,7 @@ def test_no_named_measures_preserves_old_behavior(self) -> None: existing bare-name rejection — no regression for callers that don't opt in. """ - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("aov") diff --git a/tests/test_named_measures.py b/tests/test_named_measures.py index 3193a845..916d527b 100644 --- a/tests/test_named_measures.py +++ b/tests/test_named_measures.py @@ -173,7 +173,45 @@ async def test_unknown_bare_name_still_errors(self) -> None: measures=[{"formula": "nonexistent", "name": "result"}], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): + await _generate(query, model) + + async def test_near_miss_bare_name_suggests_the_saved_measure(self) -> None: + """A misspelled saved measure names the real one.""" + model = _orders_model( + measures=[ModelMeasure(name="aov_net", formula="revenue:sum")] + ) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "aov_nett", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'aov_net'"): + await _generate(query, model) + + async def test_aggregating_a_saved_measure_says_to_drop_the_suffix(self) -> None: + """``measure:agg`` used to report the measure as a missing column.""" + model = _orders_model( + measures=[ModelMeasure(name="aov", formula="revenue:sum")] + ) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "aov:sum", "name": "result"}], + ) + + with pytest.raises( + ValueError, match="is a saved measure.*takes no aggregation" + ): + await _generate(query, model) + + async def test_unknown_column_suggests_a_close_column(self) -> None: + model = _orders_model() + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "revenu:sum", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'revenue'"): await _generate(query, model) async def test_duplicate_saved_measure_name_rejected_in_enrichment(self) -> None: From 0bd14621556578750e8d55f29c83a28d14cdafb9 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 19 Aug 2026 16:23:54 +0000 Subject: [PATCH 2/5] Address review: skip unnamed measures, cover the expression path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from CodeRabbit on #315, both correct. `ModelMeasure.name` is optional, so building the close-match candidates from every `m.name` could sort `None` against strings. A model validator rejects unnamed measures at construction, but direct mutation of `model.measures` bypasses it — the same route an existing test uses. The candidate set now drops unnamed measures, via a helper both messages share. A third copy of the old wording lived in the mixed-arithmetic path, which this PR had missed: `round(amount, 2)` reached it and still reported "Bare measure name ... Use colon syntax". It now names what the column needs instead, and the two integration expectations follow. Co-Authored-By: Claude Opus 5 (1M context) --- slayer/engine/enrichment.py | 43 +++++++++++++++++++++++---- tests/integration/test_integration.py | 4 +-- tests/test_named_measures.py | 31 +++++++++++++++++++ 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index eed17517..f327be04 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -170,6 +170,20 @@ def _public_field_name(qfield: Any) -> str: ) +def _close_name_hint(name: str, model: Any) -> str: + """A ' Did you mean ...?' clause drawn from every name the model offers. + + ``ModelMeasure.name`` is optional, so unnamed measures are dropped rather + than sorted against strings. + """ + known = sorted( + {c.name for c in model.columns} + | {m.name for m in model.measures if m.name is not None} + ) + suggestion = difflib.get_close_matches(word=name, possibilities=known, n=1) + return f" Did you mean '{suggestion[0]}'?" if suggestion else "" + + def _unknown_column_message( *, model: Any, measure_name: str, aggregation_name: str ) -> str: @@ -185,14 +199,29 @@ def _unknown_column_message( f"column, so it takes no aggregation. Reference it as '{measure_name}' " f"instead of '{measure_name}:{aggregation_name}'." ) - known = sorted( - {c.name for c in model.columns} | {m.name for m in model.measures} - ) - suggestion = difflib.get_close_matches(word=measure_name, possibilities=known, n=1) - hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" + hint = _close_name_hint(measure_name, model) return f"Column '{measure_name}' not found in model '{model.name}'.{hint}" +def _bare_name_in_expression_message(*, model: Any, name: str) -> str: + """Explain a bare name inside an arithmetic expression. + + Saved measures are inlined before this point, so a name that survives is + either a column that forgot its aggregation or nothing at all. + """ + if model.get_column(name) is not None: + return ( + f"'{name}' is a column on model '{model.name}', so it needs an " + f"aggregation inside an expression — write '{name}:sum', or another " + f"aggregation." + ) + hint = _close_name_hint(name, model) + return ( + f"'{name}' is not a saved measure on model '{model.name}'.{hint} " + f"Aggregate a column with colon syntax (e.g., '{name}:sum')." + ) + + async def enrich_query( query: SlayerQuery, model: SlayerModel, @@ -897,7 +926,9 @@ async def _ensure_measure_from_spec(mname: str, agg_refs: dict | None = None): agg_kwargs=ref.agg_kwargs, ) else: - raise ValueError(f"Bare measure name '{mname}' in expression is not valid. Use colon syntax.") + raise ValueError( + _bare_name_in_expression_message(model=model, name=mname) + ) async def _resolve_inner_alias(inner_spec, fallback_name: str) -> str: """Flatten a transform's inner spec to a measure alias. diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index e95b1f74..8e4c47f7 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -3844,7 +3844,7 @@ async def test_round_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="round(amount, 2)", name="r")], ) - with pytest.raises(ValueError, match="is not a saved measure"): + with pytest.raises(ValueError, match="needs an aggregation inside an expression"): await engine.execute(query) @@ -3855,5 +3855,5 @@ async def test_abs_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="abs(amount)", name="a")], ) - with pytest.raises(ValueError, match="is not a saved measure"): + with pytest.raises(ValueError, match="needs an aggregation inside an expression"): await engine.execute(query) diff --git a/tests/test_named_measures.py b/tests/test_named_measures.py index 916d527b..5d8d0758 100644 --- a/tests/test_named_measures.py +++ b/tests/test_named_measures.py @@ -204,6 +204,37 @@ async def test_aggregating_a_saved_measure_says_to_drop_the_suffix(self) -> None ): await _generate(query, model) + async def test_bare_column_in_expression_asks_for_an_aggregation(self) -> None: + """The mixed-arithmetic path reaches its own error site.""" + model = _orders_model() + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "round(revenue, 2)", "name": "result"}], + ) + + with pytest.raises( + ValueError, match="needs an aggregation inside an expression" + ): + await _generate(query, model) + + async def test_unnamed_measure_does_not_break_the_suggestion(self) -> None: + """``ModelMeasure.name`` is optional; unnamed measures are skipped. + + The model validator rejects unnamed measures at construction, so this + reaches the helper the way a post-construction mutation would. + """ + model = _orders_model( + measures=[ModelMeasure(name="aov", formula="revenue:sum")] + ) + model.measures.append(ModelMeasure(formula="revenue:avg")) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "revenu:sum", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'revenue'"): + await _generate(query, model) + async def test_unknown_column_suggests_a_close_column(self) -> None: model = _orders_model() query = SlayerQuery( From c9493bc9a83c921804df0df3f6de0880afd6df7f Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 19 Aug 2026 16:26:15 +0000 Subject: [PATCH 3/5] Address review: log the numpy import failure too The missing-numpy branch reports the same packaging gap as the is_available() check, reached by a different route, so it belongs in the operator log rather than the response. The three warnings that survive are the ones a caller can act on: no embedding rows, embed failure, and dim mismatch. Co-Authored-By: Claude Opus 5 (1M context) --- slayer/search/retrievers/embeddings.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/slayer/search/retrievers/embeddings.py b/slayer/search/retrievers/embeddings.py index 9c755ef6..4135e7d1 100644 --- a/slayer/search/retrievers/embeddings.py +++ b/slayer/search/retrievers/embeddings.py @@ -232,10 +232,9 @@ async def retrieve( ]) # Inline imports: ``numpy`` and ``slayer.embeddings.ranker`` - # require the optional ``advanced_search`` extra. When the extra - # is not installed, we fall through to a soft warning instead of - # raising at module import time so the rest of slayer keeps - # working without the extra. + # require the optional ``advanced_search`` extra. Importing here + # rather than at module scope keeps the rest of slayer working + # without the extra. try: import numpy as np from slayer.embeddings.ranker import ( @@ -244,10 +243,13 @@ async def retrieve( top_k_cosine, ) except ImportError: - return RetrievalResult(warnings=[ + # Same packaging gap as the is_available() check above, reached by + # a different route: an operator concern, not a caller's. + _log.warning( "embedding channel skipped: numpy not installed " - "(reinstall with the `advanced_search` extra).", - ]) + "(reinstall with the `advanced_search` extra)." + ) + return RetrievalResult() query_vec = await self.embed_question(question or "") if query_vec is None: From e4e4cb2146315d9c4426d52f2cf5855123e63ace Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 19 Aug 2026 16:29:19 +0000 Subject: [PATCH 4/5] Address review: keyword arguments for the message helpers Both new helpers take more than one parameter, which the coding guideline says must be passed by keyword. Made them keyword-only so the call sites cannot drift back. Co-Authored-By: Claude Opus 5 (1M context) --- slayer/core/formula.py | 12 +++++++++--- slayer/engine/enrichment.py | 6 +++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/slayer/core/formula.py b/slayer/core/formula.py index e3a7fa07..fd01ed45 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -609,7 +609,7 @@ def parse_formula( ) -def _bare_name_message(name: str, known_measures: frozenset[str]) -> str: +def _bare_name_message(*, name: str, known_measures: frozenset[str]) -> str: """Explain a bare name that expanded to no saved measure. A bare name is only ever a saved measure, and by this point expansion has @@ -642,7 +642,9 @@ def _parse_node( if isinstance(node, ast.Name): if node.id in agg_refs: return agg_refs[node.id] - raise ValueError(_bare_name_message(node.id, known_measures)) + raise ValueError( + _bare_name_message(name=node.id, known_measures=known_measures) + ) # Dotted name → cross-model measure must include aggregation if isinstance(node, ast.Attribute) and isinstance(node.value, ast.Name): @@ -713,7 +715,11 @@ def _parse_node( f"Cross-model measure '{mname}' must include an aggregation " f"(e.g., '{mname}:sum')." ) - raise ValueError(_bare_name_message(mname, known_measures)) + raise ValueError( + _bare_name_message( + name=mname, known_measures=known_measures + ) + ) field_agg_refs = {n: agg_refs[n] for n in measure_names if n in agg_refs} return ArithmeticField( sql=ast.unparse(node), diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index f327be04..55196e72 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -170,7 +170,7 @@ def _public_field_name(qfield: Any) -> str: ) -def _close_name_hint(name: str, model: Any) -> str: +def _close_name_hint(*, name: str, model: Any) -> str: """A ' Did you mean ...?' clause drawn from every name the model offers. ``ModelMeasure.name`` is optional, so unnamed measures are dropped rather @@ -199,7 +199,7 @@ def _unknown_column_message( f"column, so it takes no aggregation. Reference it as '{measure_name}' " f"instead of '{measure_name}:{aggregation_name}'." ) - hint = _close_name_hint(measure_name, model) + hint = _close_name_hint(name=measure_name, model=model) return f"Column '{measure_name}' not found in model '{model.name}'.{hint}" @@ -215,7 +215,7 @@ def _bare_name_in_expression_message(*, model: Any, name: str) -> str: f"aggregation inside an expression — write '{name}:sum', or another " f"aggregation." ) - hint = _close_name_hint(name, model) + hint = _close_name_hint(name=name, model=model) return ( f"'{name}' is not a saved measure on model '{model.name}'.{hint} " f"Aggregate a column with colon syntax (e.g., '{name}:sum')." From 5df28bf00ce90299a3af0d3e12a67cc9513cb585 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 19 Aug 2026 16:34:25 +0000 Subject: [PATCH 5/5] Assert the extra-missing path degrades quietly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_question_only_warns_when_extra_missing encoded the behaviour this branch removes: it asserted the missing extra reaches the caller's warnings. It now asserts the opposite contract — no advanced_search warning in the response, the message in the operator log, and the search still returning tantivy + BM25 results. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_search_three_channel.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/test_search_three_channel.py b/tests/test_search_three_channel.py index 79fda6ca..7db57764 100644 --- a/tests/test_search_three_channel.py +++ b/tests/test_search_three_channel.py @@ -9,6 +9,7 @@ from __future__ import annotations +import logging import tempfile from collections.abc import Iterator @@ -69,20 +70,28 @@ async def _seed_basic_corpus(storage: YAMLStorage) -> None: # --------------------------------------------------------------------------- -async def test_question_only_warns_when_extra_missing( - storage: YAMLStorage, monkeypatch: pytest.MonkeyPatch, +async def test_question_only_degrades_quietly_when_extra_missing( + storage: YAMLStorage, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, ) -> None: """If the extra isn't installed (or no API key is configured), the - embedding channel emits a warning and the search still returns - whatever tantivy + BM25 found. The session-wide autouse fixture - already stubs ``is_available`` to ``False`` — this test relies on - that default rather than re-patching it.""" + embedding channel is skipped and the search still returns whatever + tantivy + BM25 found. That is a deployment gap the caller cannot act + on, so it reaches the operator log rather than the response. The + session-wide autouse fixture already stubs ``is_available`` to + ``False`` — this test relies on that default rather than re-patching + it.""" await _seed_basic_corpus(storage) service = SearchService(storage=storage) - response = await service.search(question="how do I look up purchases?") - assert any( + with caplog.at_level( + logging.WARNING, logger="slayer.search.retrievers.embeddings" + ): + response = await service.search(question="how do I look up purchases?") + assert not any( "advanced_search" in w for w in response.warnings ), response.warnings + assert "advanced_search" in caplog.text async def test_question_only_warns_when_no_embeddings_persisted(