From 818453d03e507406b3198baa5fb8409ecb5138cb Mon Sep 17 00:00:00 2001 From: Rajeev Jain Date: Thu, 6 Aug 2026 08:37:26 -0500 Subject: [PATCH] Stop echoing caller file paths in recommended_next_steps Every suggested call interpolated the caller's own absolute grid and data paths back into the string, so a four-step list repeated the same path four times; those echoed paths alone were 28-36% of an inspection result. Steps now name a caller-supplied value by parameter name, spell out only values the server discovered, and bracket what is still missing, which shrinks results 18-34% and raises the computed-answer share from 25-49% to 39-61%. --- CHANGELOG.md | 12 +++ src/uxarray_mcp/next_steps.py | 48 ++++++++++++ src/uxarray_mcp/tools/advanced.py | 30 +++++--- src/uxarray_mcp/tools/inspection.py | 64 +++++++++++----- src/uxarray_mcp/tools/orchestration.py | 46 ++++++++--- tests/test_next_steps_paths.py | 101 +++++++++++++++++++++++++ tests/test_payload_budget.py | 31 ++++---- 7 files changed, 279 insertions(+), 53 deletions(-) create mode 100644 src/uxarray_mcp/next_steps.py create mode 100644 tests/test_next_steps_paths.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a61fd8e..0a88314 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ uses Semantic Versioning for public releases. ## Unreleased +### Changed +- `recommended_next_steps` no longer interpolates the caller's own file paths + into every suggestion. A four-step list used to repeat the same absolute + path four times; on an MPAS QU480 mesh those echoed paths alone were 28% of + an `inspect_mesh` result and 36% of an `inspect_variable` result, more bytes + than every computed number in either reply. Steps now reference a + caller-supplied value by parameter name (`plot_mesh(grid_path)`), spell out + only values the server discovered (`plot_variable(grid_path, data_path, + "temperature")`), and bracket what is still missing (``). Results + shrank 18-34% and the computed answer went from 25-49% of a payload to + 39-61% (#83). + ### Fixed - Preserve the original remote worker exception when synchronous MCP tools run inside an event loop instead of masking it with a nested `asyncio.run` error. diff --git a/src/uxarray_mcp/next_steps.py b/src/uxarray_mcp/next_steps.py new file mode 100644 index 0000000..4f680f0 --- /dev/null +++ b/src/uxarray_mcp/next_steps.py @@ -0,0 +1,48 @@ +"""Compact follow-up suggestions for tool results (#83). + +``recommended_next_steps`` (#30) exists so an agent can chain a workflow +without already knowing the tool vocabulary. That is worth keeping. What was +not worth keeping is *how* it was spelled: every step interpolated the +caller's own absolute file paths back into the string, so a four-step list +repeated the same path four times. + +Measured on an MPAS QU480 mesh before this change, the echoed paths alone +were 28% of an ``inspect_mesh`` result and 36% of an ``inspect_variable`` +result -- more bytes than every computed number in either reply. The caller +supplied those paths in the request it just made, so sending them back +teaches it nothing, and results are re-sent on every later turn. + +The rule is one line: echo a value only when the caller did not already +have it. That gives three ways to render an argument. + +``"grid_path"`` + A value the caller passed in, referenced by parameter name. Bare rather + than quoted, because quoting would read as a literal filename. +``literal(name)`` + A value the server discovered by opening the file, such as a variable + name. Genuinely new, so it is spelled out in full. +``needed("data_path")`` + A value the caller has not supplied yet and must provide to make the + call, rendered ````. + +Tool names are always spelled out: naming the next tool is the whole point. +""" + +from __future__ import annotations + + +def literal(value: str | float) -> str: + """Quote a value the server discovered and the caller may not know.""" + return f'"{value}"' if isinstance(value, str) else str(value) + + +def needed(name: str) -> str: + """Mark an argument the caller must still supply.""" + return f"<{name}>" + + +def call(tool: str, *args: str, note: str | None = None, **kwargs: str) -> str: + """Render one suggested call as ``tool(a, b, k=v) - note``.""" + rendered = list(args) + [f"{key}={value}" for key, value in kwargs.items()] + step = f"{tool}({', '.join(rendered)})" + return f"{step} - {note}" if note else step diff --git a/src/uxarray_mcp/tools/advanced.py b/src/uxarray_mcp/tools/advanced.py index 97c74a8..c9f5273 100644 --- a/src/uxarray_mcp/tools/advanced.py +++ b/src/uxarray_mcp/tools/advanced.py @@ -14,6 +14,7 @@ from uxarray_mcp.domain.mesh import load_dataset, load_grid from uxarray_mcp.domain.remap_coverage import compute_target_coverage +from uxarray_mcp.next_steps import call, needed from uxarray_mcp.provenance import attach_provenance from uxarray_mcp.state import ( OperationTracker, @@ -226,13 +227,15 @@ def subset_bbox( "result_handle": result_handle, } next_steps = [ - f'plot_mesh(grid_path="{resolved_grid}")', - f'export_to_netcdf("", result_handle="{result_handle}")', + call("plot_mesh", grid_path="grid_path"), + # result_handle is echoed as its own key in this same result, so the + # caller can read it there rather than have it repeated in prose. + call("export_to_netcdf", needed("output.nc"), result_handle="result_handle"), ] if resolved_data is not None: next_steps.insert( 0, - f'plot_variable("{resolved_grid}", "{resolved_data}", "")', + call("plot_variable", "grid_path", "data_path", needed("variable_name")), ) result["recommended_next_steps"] = next_steps tracker.succeed("Bounding-box subset complete.") @@ -330,13 +333,15 @@ def subset_polygon( "result_handle": result_handle, } next_steps = [ - f'plot_mesh(grid_path="{resolved_grid}")', - f'export_to_netcdf("", result_handle="{result_handle}")', + call("plot_mesh", grid_path="grid_path"), + # result_handle is echoed as its own key in this same result, so the + # caller can read it there rather than have it repeated in prose. + call("export_to_netcdf", needed("output.nc"), result_handle="result_handle"), ] if resolved_data is not None: next_steps.insert( 0, - f'plot_variable("{resolved_grid}", "{resolved_data}", "")', + call("plot_variable", "grid_path", "data_path", needed("variable_name")), ) result["recommended_next_steps"] = next_steps result = attach_provenance( @@ -426,13 +431,20 @@ def extract_cross_section( "result_handle": result_handle, } next_steps = [ - f'plot_mesh(grid_path="{resolved_grid}")', - f'export_to_netcdf("", result_handle="{result_handle}")', + call("plot_mesh", grid_path="grid_path"), + # result_handle is echoed as its own key in this same result, so the + # caller can read it there rather than have it repeated in prose. + call("export_to_netcdf", needed("output.nc"), result_handle="result_handle"), ] if resolved_data is not None: next_steps.insert( 0, - f'calculate_zonal_mean("{resolved_grid}", "{resolved_data}", "")', + call( + "calculate_zonal_mean", + "grid_path", + "data_path", + needed("variable_name"), + ), ) result["recommended_next_steps"] = next_steps result = attach_provenance( diff --git a/src/uxarray_mcp/tools/inspection.py b/src/uxarray_mcp/tools/inspection.py index 8bfc5ed..e87ba11 100644 --- a/src/uxarray_mcp/tools/inspection.py +++ b/src/uxarray_mcp/tools/inspection.py @@ -15,6 +15,7 @@ load_dataset, load_grid, ) +from uxarray_mcp.next_steps import call, literal, needed from uxarray_mcp.provenance import attach_provenance @@ -91,10 +92,10 @@ def _inspect_mesh_local(file_path: str) -> Dict[str, Any]: "n_max_face_nodes": int(grid.n_max_face_nodes), "file_size_mb": round(file_size_mb, 2), "recommended_next_steps": [ - f'calculate_area("{file_path}")', - f'plot_mesh("{file_path}")', - f'inspect_variable("{file_path}", "")', - f'validate_dataset("{file_path}", "")', + call("calculate_area", "grid_path"), + call("plot_mesh", "grid_path"), + call("inspect_variable", "grid_path", needed("data_path")), + call("validate_dataset", "grid_path", needed("data_path")), ], }, tool="inspect_mesh", @@ -163,15 +164,23 @@ def _inspect_variable_local( next_steps = [] if face_vars: v0 = face_vars[0] + # v0 is discovered by opening the file, so it is spelled out; the + # paths came from the caller and stay as parameter names. next_steps = [ - f'plot_variable("{grid_path}", "{data_path}", "{v0}")', - f'calculate_zonal_mean("{grid_path}", "{data_path}", "{v0}")', - f'validate_dataset("{grid_path}", "{data_path}")', - f'subset_bbox([-60,60], [-30,30], grid_path="{grid_path}", ' - f'data_path="{data_path}", variable_name="{v0}")', + call("plot_variable", "grid_path", "data_path", literal(v0)), + call("calculate_zonal_mean", "grid_path", "data_path", literal(v0)), + call("validate_dataset", "grid_path", "data_path"), + call( + "subset_bbox", + "[-60, 60]", + "[-30, 30]", + grid_path="grid_path", + data_path="data_path", + variable_name=literal(v0), + ), ] else: - next_steps = [f'validate_dataset("{grid_path}", "{data_path}")'] + next_steps = [call("validate_dataset", "grid_path", "data_path")] info["recommended_next_steps"] = next_steps return attach_provenance( info, @@ -234,9 +243,14 @@ def _calculate_area_local(file_path: str) -> Dict[str, Any]: raise RuntimeError(f"Failed to calculate face areas: {str(e)}") result["recommended_next_steps"] = [ - f'plot_mesh("{file_path}")', - f'inspect_variable("{file_path}", "")', - f'calculate_zonal_mean("{file_path}", "", "")', + call("plot_mesh", "grid_path"), + call("inspect_variable", "grid_path", needed("data_path")), + call( + "calculate_zonal_mean", + "grid_path", + needed("data_path"), + needed("variable_name"), + ), ] return attach_provenance( result, tool="calculate_area", inputs={"file_path": file_path} @@ -304,10 +318,15 @@ def _calculate_zonal_mean_local( raise RuntimeError(f"Failed to compute zonal mean: {str(e)}") result["recommended_next_steps"] = [ - f'plot_zonal_mean("{grid_path}", "{data_path}", "{variable_name}")', - f'plot_variable("{grid_path}", "{data_path}", "{variable_name}")', - f'extract_cross_section(latitude=0.0, grid_path="{grid_path}", ' - f'data_path="{data_path}", variable_name="{variable_name}")', + call("plot_zonal_mean", "grid_path", "data_path", "variable_name"), + call("plot_variable", "grid_path", "data_path", "variable_name"), + call( + "extract_cross_section", + "grid_path", + "data_path", + "variable_name", + latitude="0.0", + ), ] return attach_provenance( result, @@ -636,9 +655,14 @@ def validate_dataset(grid_path: str, data_path: str) -> Dict[str, Any]: if overall_passed: result["recommended_next_steps"] = [ - f'inspect_variable("{grid_path}", "{data_path}")', - f'calculate_zonal_mean("{grid_path}", "{data_path}", "")', - f'plot_variable("{grid_path}", "{data_path}", "")', + call("inspect_variable", "grid_path", "data_path"), + call( + "calculate_zonal_mean", + "grid_path", + "data_path", + needed("variable_name"), + ), + call("plot_variable", "grid_path", "data_path", needed("variable_name")), ] else: result["recommended_next_steps"] = [ diff --git a/src/uxarray_mcp/tools/orchestration.py b/src/uxarray_mcp/tools/orchestration.py index 241a7c8..daf9f4d 100644 --- a/src/uxarray_mcp/tools/orchestration.py +++ b/src/uxarray_mcp/tools/orchestration.py @@ -13,6 +13,7 @@ import json from typing import Any, Optional +from uxarray_mcp.next_steps import call, literal, needed from uxarray_mcp.provenance import attach_provenance @@ -280,8 +281,12 @@ def analyze_dataset( next_steps: list[str] = [] if resolved_data is None: next_steps.append( - f'inspect_variable("{resolved_grid}", "") ' - "— rerun with a data file to unlock variable analysis" + call( + "inspect_variable", + "grid_path", + needed("data_path"), + note="rerun with a data file to unlock variable analysis", + ) ) if validation is not None and validation.get("passed") is False: next_steps.append( @@ -289,22 +294,43 @@ def analyze_dataset( "trusting downstream results." ) if selected_variable and resolved_data is not None: + # selected_variable was chosen by the server, so it is spelled out. next_steps.append( - f'plot_zonal_mean("{resolved_grid}", "{resolved_data}", ' - f'"{selected_variable}") — render the zonal profile' + call( + "plot_zonal_mean", + "grid_path", + "data_path", + literal(selected_variable), + note="render the zonal profile", + ) ) next_steps.append( - f'extract_cross_section(latitude=0.0, grid_path="{resolved_grid}", ' - f'data_path="{resolved_data}", variable_name="{selected_variable}")' + call( + "extract_cross_section", + latitude="0.0", + grid_path="grid_path", + data_path="data_path", + variable_name=literal(selected_variable), + ) ) next_steps.append( - f"subset_bbox(lon_bounds=[-180, 180], lat_bounds=[-90, 90], " - f'grid_path="{resolved_grid}", data_path="{resolved_data}", ' - f'variable_name="{selected_variable}") — focus on a region' + call( + "subset_bbox", + lon_bounds="[-180, 180]", + lat_bounds="[-90, 90]", + grid_path="grid_path", + data_path="data_path", + variable_name=literal(selected_variable), + note="focus on a region", + ) ) if not next_steps: next_steps.append( - f'plot_mesh(grid_path="{resolved_grid}") — visualize the mesh wireframe' + call( + "plot_mesh", + grid_path="grid_path", + note="visualize the mesh wireframe", + ) ) result: dict[str, Any] = { diff --git a/tests/test_next_steps_paths.py b/tests/test_next_steps_paths.py new file mode 100644 index 0000000..5470bbd --- /dev/null +++ b/tests/test_next_steps_paths.py @@ -0,0 +1,101 @@ +"""Follow-up hints must not echo the caller's own file paths (#83). + +``recommended_next_steps`` was 38-46% of some results, and most of those +bytes were the grid and data paths interpolated back into every suggested +call -- paths the caller had just sent in the request. Results are carried +forward in the conversation and re-sent on every later turn, so that waste +is paid repeatedly. + +These tests pin the rule rather than the wording: a step may name a tool +and may spell out a value the *server* discovered, but it must never quote +back a filesystem path the caller supplied. +""" + +from __future__ import annotations + +import warnings + +import pytest + +from uxarray_mcp.next_steps import call, literal, needed +from uxarray_mcp.tools.frontdoor import run_analysis + +#: Operations whose results carry follow-up hints, with the extra arguments +#: each one needs beyond ``grid_path``. +_OPERATIONS = { + "inspect_mesh": {}, + "calculate_area": {}, + "inspect_variable": {"variable_name": "temperature"}, + "calculate_zonal_mean": {"variable_name": "temperature"}, + "validate_dataset": {}, +} + + +@pytest.fixture +def results(state_dir, structured_mesh_files): + grid_file, data_file = structured_mesh_files + out = {} + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + for operation, extra in _OPERATIONS.items(): + kwargs = dict(extra) + if operation != "inspect_mesh" and operation != "calculate_area": + kwargs["data_path"] = data_file + out[operation] = ( + run_analysis(operation=operation, grid_path=grid_file, **kwargs), + grid_file, + data_file, + ) + return out + + +class TestNoPathEcho: + @pytest.mark.parametrize("operation", sorted(_OPERATIONS)) + def test_steps_never_echo_caller_paths(self, results, operation): + result, grid_file, data_file = results[operation] + steps = " ".join(result.get("recommended_next_steps", [])) + for path in (grid_file, data_file): + assert path not in steps, ( + f"{operation} echoes the caller-supplied path {path!r} back " + "inside recommended_next_steps. Reference it by parameter " + "name instead; the caller already knows what it passed." + ) + + @pytest.mark.parametrize("operation", sorted(_OPERATIONS)) + def test_steps_still_name_tools(self, results, operation): + """Shrinking the field must not empty it: #30's purpose survives.""" + steps = results[operation][0].get("recommended_next_steps", []) + assert steps, f"{operation} returned no follow-up hints at all" + assert all(isinstance(step, str) and "(" in step for step in steps), ( + f"{operation} hints should still read as callable suggestions" + ) + + @pytest.mark.parametrize("operation", sorted(_OPERATIONS)) + def test_steps_are_a_small_share_of_the_result(self, results, operation): + import json + + result = results[operation][0] + total = len(json.dumps(result, default=str)) + steps = len(json.dumps(result.get("recommended_next_steps", []), default=str)) + assert steps / total <= 0.25, ( + f"{operation} spends {steps / total:.0%} of its payload on " + f"follow-up hints ({steps} of {total} bytes)." + ) + + +class TestRenderers: + def test_caller_supplied_argument_is_a_bare_parameter_name(self): + assert call("plot_mesh", "grid_path") == "plot_mesh(grid_path)" + + def test_discovered_value_is_quoted(self): + assert literal("temperature") == '"temperature"' + + def test_missing_argument_is_bracketed(self): + assert needed("data_path") == "" + + def test_keyword_and_note_render(self): + step = call("subset_bbox", lon_bounds="[-180, 180]", note="focus on a region") + assert step == "subset_bbox(lon_bounds=[-180, 180]) - focus on a region" + + def test_numeric_literal_is_not_quoted(self): + assert literal(0.5) == "0.5" diff --git a/tests/test_payload_budget.py b/tests/test_payload_budget.py index 718f8d2..aad563d 100644 --- a/tests/test_payload_budget.py +++ b/tests/test_payload_budget.py @@ -6,8 +6,8 @@ measured number, because knowing what it grew to is the useful part. The budgets are ratchets, not aspirations -- they sit just above today's -measurements. Tighten them when #83 and #89 land; do not loosen them without -saying why. +measurements. They were tightened when #83 landed; tighten them again when +#89 lands, and do not loosen them without saying why. """ from __future__ import annotations @@ -35,23 +35,26 @@ } #: Upper bound on serialized result bytes, per operation family. Measured -#: values sit roughly 20% below each budget; the slack absorbs the varying +#: values sit roughly 15% below each budget; the slack absorbs the varying #: length of the temporary file paths echoed back in ``_provenance.inputs``. +#: Lowered across the board when #83 removed the caller paths that +#: ``recommended_next_steps`` used to interpolate into every suggestion. RESULT_BYTE_BUDGETS = { - "inspect_mesh": 1600, - # Raised from 1600 for the postcondition block (#84/#90): ~440 bytes - # that took correct verification answers from 11/20 to 20/20 in the - # study, which is the one payload increase we have evidence pays back. - "calculate_area": 1900, - "inspect_variable": 2600, - "calculate_zonal_mean": 2800, - "validate_dataset": 2600, + "inspect_mesh": 1150, + # Kept above inspect_mesh for the postcondition block (#84/#90): ~440 + # bytes that took correct verification answers from 11/20 to 20/20 in + # the study, which is the one payload increase we have evidence for. + "calculate_area": 1550, + "inspect_variable": 1700, + "calculate_zonal_mean": 2050, + "validate_dataset": 2050, } #: Floor on the fraction of a result that is the computed answer plus status. -#: Measured at 0.20-0.36 today. #83 argues for something closer to 0.5; raise -#: this as the catalog and provenance payload shrink. -SIGNAL_FRACTION_FLOOR = 0.15 +#: Measured at 0.37-0.59 after #83, up from 0.25-0.48 before it. #83 argues +#: for something closer to 0.5 everywhere; the remaining gap is +#: ``_provenance``, which is now the largest key in every result. +SIGNAL_FRACTION_FLOOR = 0.30 #: Upper bound on the serialized core tool specification, sent every request. TOOL_SPEC_BYTE_BUDGET = 42000