diff --git a/INSTRUCTIONS.md b/INSTRUCTIONS.md index 4eff86fe..ee3b49d8 100644 --- a/INSTRUCTIONS.md +++ b/INSTRUCTIONS.md @@ -137,7 +137,7 @@ See [MCP Servers](#mcp-servers) for available tools and [docs/mcp-servers.md](do | Variable | Default | Description | | -------------------- | ------------------ | -------------------------------------------------------------------------- | -| `STIRRUP_CODE_IMAGE` | `python:3.12-slim` | Docker image for the code sandbox (build `assetops-code` for numpy/pandas/scipy) | +| `STIRRUP_CODE_IMAGE` | `assetops-code` | Docker image for the Stirrup code sandbox; build the bundled image for NumPy, pandas, and SciPy | | `DOCKER_HOST` | *(SDK default)* | Daemon socket if non-default (e.g. Rancher: `unix:////.rd/docker.sock`) | --- diff --git a/benchmarks/scenario_suite/README.md b/benchmarks/scenario_suite/README.md index 05d03159..5461c682 100644 --- a/benchmarks/scenario_suite/README.md +++ b/benchmarks/scenario_suite/README.md @@ -19,12 +19,16 @@ profile contains: | Category | Scenario IDs | | -------- | ------------ | -| CAR | 151, 152, 153, 156, 167, 178, 180, 182, 183, 193 | +| CAR | 151, 152, 153, 156 | | FCC | 301, 303, 305, 308, 314, 316, 320, 323, 325, 327 | -| FMSR | 902, 904, 905, 906, 915, 916, 920, 923, 928, 932 | -| Health | 401–410 | -| TSFM | 1001–1005 | -| WOSR | 5, 9, 13, 20, 24, 31, 43, 50, 61, 66 | +| FMSR | 902, 904, 905, 906 | +| Health | 401–404 | +| TSFM | `[]` (none) | +| WOSR | 5, 9, 13, 20 | + +Individual categories may use an empty list, such as `tsfm: []`; the profile as +a whole must contain at least one scenario ID. Profile shorthands such as `lite` +simply skip empty categories. Examples: diff --git a/benchmarks/scenario_suite/lite.yaml b/benchmarks/scenario_suite/lite.yaml index b977e989..cb734ec3 100644 --- a/benchmarks/scenario_suite/lite.yaml +++ b/benchmarks/scenario_suite/lite.yaml @@ -3,12 +3,6 @@ car: - 152 - 153 - 156 - - 167 - - 178 - - 180 - - 182 - - 183 - - 193 fcc: - 301 - 303 @@ -25,37 +19,14 @@ fmsr: - 904 - 905 - 906 - - 915 - - 916 - - 920 - - 923 - - 928 - - 932 health: - 401 - 402 - 403 - 404 - - 405 - - 406 - - 407 - - 408 - - 409 - - 410 -tsfm: - - 1001 - - 1002 - - 1003 - - 1004 - - 1005 +tsfm: [] wosr: - 5 - 9 - 13 - 20 - - 24 - - 31 - - 43 - - 50 - - 61 - - 66 diff --git a/docs/evaluation.md b/docs/evaluation.md index c53394b9..70174791 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -89,6 +89,8 @@ uv run evaluate \ --judge-model litellm_proxy/azure/gpt-5.4 ``` +Evaluation scores the top-level `answer` field in each trajectory. + Output: ``` diff --git a/docs/static-json-evaluation.md b/docs/static-json-evaluation.md index 147e81a2..706d87a9 100644 --- a/docs/static-json-evaluation.md +++ b/docs/static-json-evaluation.md @@ -75,6 +75,7 @@ Expected folder layout: scenarios_data/ scenario_11/ groundtruth.txt + groundtruth_eval.json # optional scenario_12/ groundtruth.txt scenario_13/ @@ -95,6 +96,46 @@ scenario_11 → 11 The evaluator then joins trajectories and ground truth by scenario id. +### Optional evaluation metadata + +Scenario folders may also include `groundtruth_eval.json`. This file provides +scorer-specific metadata while keeping the canonical answer in `groundtruth.txt`. +It is currently used by clarification-abstain-response (CAR) scenarios, where +the exact wording can vary but the response mode and key domain terms must be +correct. + +Example `groundtruth.txt`: + +```json +{ + "clarification": "Which asset do you mean by 'the main unit'?" +} +``` + +Example `groundtruth_eval.json`: + +```json +{ + "mode": "clarification", + "required_terms": ["main unit"], + "optional_terms": ["asset"], + "must_have_exactly_one_mode_key": true +} +``` + +Supported CAR metadata fields: + +| Field | Meaning | +| -------------------------------- | ----------------------------------------------------------------------- | +| `mode` | Expected top-level key: `response`, `clarification`, or `abstain` | +| `required_terms` | Terms that indicate the model addressed the required ambiguity or fact | +| `optional_terms` | Informative terms reported for analysis, but not required for passing | +| `must_have_exactly_one_mode_key` | Whether the model must return exactly one top-level CAR mode key | + +If `groundtruth_eval.json` is absent, the scorer derives CAR metadata from +`groundtruth.txt` for one-key answers using `response`, `clarification`, or +`abstain`. + --- ## How Matching Works @@ -180,6 +221,11 @@ The scorer also handles simple noisy count-only answers such as: The answer is 34. ``` +For noisy structured answers, the parser prefers a final JSON/Python structure +over earlier explanatory parenthetical text. This avoids evaluating prose such +as `(no asset is specified)` when the model later returns the actual final JSON +object. + --- ## Metrics @@ -200,6 +246,45 @@ The answer is 34. The scorer marks a scenario as passed only when the structured answer is a strict exact match. Partial correctness is still available through `score`, `f1`, `partial_exact_match_accuracy`, and detailed key-level results. +### CAR metrics and pass criteria + +For CAR scenarios, the scorer evaluates the answer as a mode-selection task plus +required-term coverage. The model answer should be a single JSON object using +exactly one of these top-level keys: + +```text +response | clarification | abstain +``` + +A CAR scenario passes when: + +1. the model returns exactly one top-level CAR mode key, +2. that key matches the expected `mode`, and +3. at least one `required_terms` entry appears in the model answer value. + +The CAR soft score is reported as: + +```text +car_score = 0.6 * mode_key_match + 0.4 * mode_term_coverage +``` + +where `mode_term_coverage` is the fraction of required terms found in the answer +value. Optional terms are reported for diagnostics and do not affect pass/fail. + +Additional CAR report fields: + +| Metric | Meaning | +| ----------------------------------- | ------------------------------------------------------------------ | +| `mode_key_match` | 1.0 when the returned mode key matches the expected mode | +| `mode_exactly_one_key` | 1.0 when the answer has exactly one top-level key | +| `mode_required_terms` | Required terms used for this scenario | +| `mode_matched_terms` | Required terms found in the model answer | +| `mode_term_coverage` | Fraction of required terms found | +| `mode_optional_terms` | Optional diagnostic terms | +| `mode_matched_optional_terms` | Optional diagnostic terms found | +| `mode_optional_term_coverage` | Fraction of optional terms found, or null if none were configured | +| `car_score` | Weighted CAR soft score | + --- ## Example diff --git a/docs/stirrup-agent.md b/docs/stirrup-agent.md index 4c2d2117..71bb79f6 100644 --- a/docs/stirrup-agent.md +++ b/docs/stirrup-agent.md @@ -113,18 +113,19 @@ The runner's default model is | `--model-id` prefix | Client | Notes | | ----------------------- | ------------------------------- | ----------------------------------------------------------- | | `/` | Stirrup `LiteLLMClient` | Native LiteLLM. `watsonx/...`, `anthropic/...`, etc. work directly. | -| `litellm_proxy/` or `tokenrouter/` | `OpenResponsesClient`, then `ChatCompletionsClient` | Prefer Responses; fall back when unsupported or unavailable after retries. | +| `litellm_proxy/` or `tokenrouter/` | `ChatCompletionsClient` | Uses only the OpenAI-compatible Chat Completions API. | Required env vars match the rest of the repo: the standard watsonx vars for the native route, `LITELLM_BASE_URL` / `LITELLM_API_KEY` for the LiteLLM proxy, or `TOKENROUTER_BASE_URL` / `TOKENROUTER_API_KEY` for TokenRouter. -For context management, the runner assumes every configured model has a -1,000,000-token context window. A client adapter reports that value to -Stirrup's summarization logic while leaving each underlying client's 64,000 -maximum-output-token default unchanged. The runner requests summarization at -85% context usage, or approximately 850,000 tokens. This is a benchmark -assumption rather than model metadata supplied by TokenRouter. +For context management, the runner gives Stirrup a 100,000-token working-context +budget while leaving each underlying client's 64,000 maximum-output-token default +unchanged. The runner requests summarization at 75% of that budget, or +approximately 75,000 tokens. This budget controls context compaction and is not +model metadata supplied by TokenRouter. When summarization occurs, the complete +generated summary is printed during the run rather than the truncated preview +used by Stirrup's default logger. --- @@ -149,12 +150,14 @@ Backends (`--code-backend`): > inputs you control; prefer `docker` for unattended or untrusted runs. When code execution is enabled, the runner appends backend-specific guidance to -the shared agent prompt. It directs the model to retrieve domain data through -the MCP tools, use `code_exec` for computation and validation, keep work inside -the persistent execution workspace, and include verified conclusions in the -Stirrup `finish` reason. Docker runs also identify `/workspace` and the default -image's package availability; local runs warn that commands execute with the -current user's host permissions. `--no-code` keeps the shared prompt unchanged. +the shared agent prompt. It directs the model to answer directly when domain +knowledge, MCP results, or basic reasoning are sufficient; reserve `code_exec` +for necessary computation, data processing, workspace inspection, or validation; +and never use it for planning, comments, placeholders, or empty scripts. When +code is needed, the prompt prefers one bounded inspect/analyze/verify script and +small outputs. Docker runs identify `/workspace` and the installed NumPy, pandas, +and SciPy packages; local runs warn that commands execute with the current user's +host permissions. `--no-code` keeps the shared prompt unchanged. --- @@ -176,19 +179,21 @@ Rancher Desktop must use the **dockerd (moby)** engine, not containerd. ### 2. Sandbox image with the scientific stack -`python:3.12-slim` has no numpy/pandas/scipy. Build the bundled image, which adds them: +The default image name is `assetops-code`. Build it once from the bundled +Dockerfile, which adds NumPy, pandas, and SciPy to `python:3.12-slim`: ```bash docker build -f src/agent/stirrup_agent/Dockerfile.code -t assetops-code . -export STIRRUP_CODE_IMAGE=assetops-code ``` +Set `STIRRUP_CODE_IMAGE` only when using a different tag. + `Dockerfile.code`: ```dockerfile FROM python:3.12-slim RUN pip install --no-cache-dir \ - "numpy>=1.24" "pandas>=2.0" "scipy>=1.10" "matplotlib>=3.7" + "numpy>=1.24" "pandas>=2.0" "scipy>=1.10" ``` ### 3. Run @@ -209,6 +214,45 @@ call and the right answer (479001600). --- +## Reading tool-produced files + +On the code track, MCP text results larger than 100 KiB (102,400 UTF-8 bytes) are +automatically written under `mcp_results/` in the active code workspace. The +tool response returned to the model is a compact JSON handle containing the +relative path, tool arguments, byte count, and SHA-256 digest. `code_exec` can +read that path directly without copying the original response through another +model turn. +The file contains the complete, unmodified MCP response—including null fields— +rather than a projected subset. Code should inspect only the schema, counts, a +small sample, or the specific rows or fields needed, then process the artifact +in place. For artifacts larger than 200 KiB, extract and process the relevant +subset in bounded batches instead of printing the full payload. + +Within one agent run, identical read calls reuse an intact existing artifact. +Successful work-order mutations, catalog mutations, and TSFM run-producing tools +clear the read cache so later reads observe their changes. This is a run-local +snapshot cache; it does not detect updates made externally during the run. +Artifacts are content-addressed, so a refreshed response does not overwrite an +earlier snapshot. Smaller responses remain inline, and persistence failures +safely fall back to the original inline MCP result. + +For example: + +```bash +python3 - <<'PY' +import json + +with open("mcp_results/wo__list_workorders__.json") as f: + result = json.load(f) + +print(len(result["work_orders"])) +PY +``` + +The workspace is temporary unless `--preserve-workspace` is enabled. + +--- + ## Preserving code workspaces By default, Stirrup creates a temporary execution directory for `code_exec` and @@ -249,7 +293,7 @@ In addition to the [common flags](../INSTRUCTIONS.md#common-flags) (`--model-id` | `--workspace-dir PATH` | Host base directory for Docker/local code-execution workspaces. | | `--preserve-workspace` | Copy final code-execution files into `--workspace-dir` before cleanup. | -Environment variable: `STIRRUP_CODE_IMAGE` (Docker image; default `python:3.12-slim`). +Environment variable: `STIRRUP_CODE_IMAGE` (Docker image; default `assetops-code`). --- @@ -315,7 +359,7 @@ uv run stirrup-agent --no-code --run-id stirrup-smoke --scenario-id 101 \ | ------- | ----------- | | `docker.errors.DockerException: Error while fetching server API version ... FileNotFoundError` | SDK can't find the daemon socket. `export DOCKER_HOST=unix://`. | | Docker connects but `code_exec` hits `ModuleNotFoundError` | Sandbox image lacks the library; build/point `STIRRUP_CODE_IMAGE` at `assetops-code` (or an image with the stack). | -| Agent's code can't open a tool-produced file path in Docker | Expected — the host path isn't in the sandbox. Use `--code-backend local` for file-reading scenarios. | +| Agent code cannot open a server-returned dataset or result file pointer in Docker | Those host-side pointers are separate from automatically bridged `mcp_results/` snapshots. Use an MCP tool that consumes the pointer, or use `--code-backend local` when direct host-file access is required. | | `uv sync` fails to resolve | A pin (e.g. `litellm==...`) clashing with Stirrup's range; relax to a compatible range. | | `stirrup` import errors | `stirrup[mcp,litellm,docker]` not installed; re-run `uv sync`. | | A server missing from the tool list | Its subprocess failed to start (missing CouchDB creds for `iot`/`wo`/`vibration`, or model load for `tsfm`). | @@ -324,9 +368,10 @@ uv run stirrup-agent --no-code --run-id stirrup-smoke --scenario-id 101 \ ## What was added -- `src/agent/stirrup_agent/` — `runner.py`, `cli.py`, `trajectory.py`, `__init__.py`, - `Dockerfile.code`, and `tests/test_runner.py`. +- `src/agent/stirrup_agent/` — runner, trajectory mapping, workspace bridging, + CLI, Docker image, and focused tests. +- `src/agent/_prompts.py` — concise enforcement of user-requested response formats. - `pyproject.toml` — `stirrup[mcp,litellm,docker]` dependency and the `stirrup-agent` entry point. -No other parts of the repo are modified; the MCP servers are unchanged. +The MCP servers themselves are unchanged. diff --git a/src/agent/_prompts.py b/src/agent/_prompts.py index 6fec7ea9..b6dcdae5 100644 --- a/src/agent/_prompts.py +++ b/src/agent/_prompts.py @@ -12,5 +12,6 @@ forecasting models, and work order management. Answer the user's question concisely and accurately using the available tools. -When you retrieve data, include the key numbers or names in your answer. +Follow the user's requested output format exactly. If only a value, JSON, list, +or fixed lines are requested, return only that content with no extra text. """ diff --git a/src/agent/stirrup_agent/Dockerfile.code b/src/agent/stirrup_agent/Dockerfile.code index 1be3f6e5..5c72d812 100644 --- a/src/agent/stirrup_agent/Dockerfile.code +++ b/src/agent/stirrup_agent/Dockerfile.code @@ -1,3 +1,3 @@ FROM python:3.12-slim RUN pip install --no-cache-dir \ - "numpy>=1.24" "pandas>=2.0" "scipy>=1.10" "matplotlib>=3.7" \ No newline at end of file + "numpy>=1.24" "pandas>=2.0" "scipy>=1.10" \ No newline at end of file diff --git a/src/agent/stirrup_agent/cli.py b/src/agent/stirrup_agent/cli.py index 478593e4..4f9d5711 100644 --- a/src/agent/stirrup_agent/cli.py +++ b/src/agent/stirrup_agent/cli.py @@ -31,8 +31,7 @@ def _build_parser() -> argparse.ArgumentParser: directly here, e.g. watsonx/meta-llama/llama-4-maverick-17b-128e-instruct-fp8 -Router models use the Responses API first and fall back to Chat Completions -when Responses is unsupported or remains unavailable after retries. +Router models use the Chat Completions API. tracks: --code-enabled (default) Add a sandboxed code-execution tool (code track). @@ -44,7 +43,7 @@ def _build_parser() -> argparse.ArgumentParser: TOKENROUTER_API_KEY TokenRouter API key (for tokenrouter/* models) TOKENROUTER_BASE_URL TokenRouter base URL (e.g. https://api.tokenrouter.com/v1) WATSONX_APIKEY/... Standard LiteLLM watsonx vars (for watsonx/* models) - STIRRUP_CODE_IMAGE Docker image for the code track (default python:3.12-slim) + STIRRUP_CODE_IMAGE Docker image for the code track (default assetops-code) examples: stirrup-agent "What assets are at site MAIN?" diff --git a/src/agent/stirrup_agent/finish_tool.py b/src/agent/stirrup_agent/finish_tool.py new file mode 100644 index 00000000..48349060 --- /dev/null +++ b/src/agent/stirrup_agent/finish_tool.py @@ -0,0 +1,94 @@ +"""Custom Stirrup finish tool that captures the final response explicitly.""" + +from __future__ import annotations + +from pydantic import BaseModel, Field, field_validator +from stirrup.constants import DEFAULT_FINISH_TOOL_NAME +from stirrup.core.models import Tool, ToolResult, ToolUseCountMetadata + + +class AssetOpsFinishParams(BaseModel): + """Arguments the agent supplies when ending an AssetOps task.""" + + answer: str = Field( + min_length=1, + description=( + "Complete response to send directly to the user. Match any output " + "format specified in the original request. Include only the requested " + "content, with no status update, internal reasoning, or additional " + "commentary unless the user asked for it." + ), + ) + reason: str = Field( + default="", + description=( + "Optional internal note explaining why the run is ending. " + "This is operational metadata and is not shown to the user." + ), + ) + paths: list[str] = Field( + default_factory=list, + description=( + "Paths of files created or modified for the user. List individual " + "files only; do not list directories." + ), + ) + + @field_validator("answer") + @classmethod + def _answer_must_contain_content(cls, value: str) -> str: + if not value.strip(): + raise ValueError("answer must include at least one non-whitespace character") + return value + + +async def _finish_executor( + params: AssetOpsFinishParams, +) -> ToolResult[ToolUseCountMetadata]: + """Accept completion only when every reported output file exists.""" + from stirrup.core.agent import _SESSION_STATE + + try: + state = _SESSION_STATE.get(None) + exec_env = state.exec_env if state else None + except LookupError: + exec_env = None + + if exec_env and params.paths: + missing = [path for path in params.paths if not await exec_env.file_exists(path)] + if missing: + return ToolResult( + content=( + f"Cannot finish: reported output files do not exist: {missing}. " + "Check each path and save the files before trying again." + ), + metadata=ToolUseCountMetadata(), + success=False, + ) + + return ToolResult( + content=params.answer, + metadata=ToolUseCountMetadata(), + success=True, + ) + + +ASSETOPS_FINISH_TOOL: Tool[AssetOpsFinishParams, ToolUseCountMetadata] = Tool( + name=DEFAULT_FINISH_TOOL_NAME, + description=( + "End the task when all available work is complete or no further progress " + "is possible. Put the complete response for the user in answer, an " + "optional internal completion note in reason, and any created or modified " + "file paths in paths. Call this tool alone on the final turn." + ), + parameters=AssetOpsFinishParams, + executor=_finish_executor, +) + + +def structured_finish_answer(finish_params: object) -> str | None: + """Return the final response, or ``None`` for non-custom finish data.""" + answer = getattr(finish_params, "answer", None) + if isinstance(answer, str) and answer.strip(): + return answer.strip() + return None diff --git a/src/agent/stirrup_agent/runner.py b/src/agent/stirrup_agent/runner.py index 8df70ad2..46272278 100644 --- a/src/agent/stirrup_agent/runner.py +++ b/src/agent/stirrup_agent/runner.py @@ -7,9 +7,7 @@ :class:`~agent.models.Trajectory`. Model routing: - * OpenAI-compatible router prefixes -> Stirrup ``OpenResponsesClient`` first, - with a sticky fallback to ``ChatCompletionsClient`` when the endpoint does - not support the Responses interface. + * OpenAI-compatible router prefixes -> Stirrup ``ChatCompletionsClient``. * ``/`` -> Stirrup ``LiteLLMClient``, which reaches Anthropic, watsonx, Bedrock, etc. natively through LiteLLM. This means ``watsonx/...`` models work directly here, without the proxy detour Goose @@ -44,6 +42,7 @@ from .._prompts import AGENT_SYSTEM_PROMPT from ..models import AgentResult, Trajectory from ..runner import AgentRunner +from .finish_tool import ASSETOPS_FINISH_TOOL from .trajectory import build_trajectory, classify_tool, final_answer _log = logging.getLogger(__name__) @@ -51,31 +50,31 @@ _REPO_ROOT = Path(__file__).parent.parent.parent.parent _DEFAULT_MODEL = "watsonx/meta-llama/llama-4-maverick-17b-128e-instruct-fp8" # A code-track image needs the scientific stack the WO/vibration analyses use. -_DEFAULT_CODE_IMAGE = os.environ.get("STIRRUP_CODE_IMAGE", "python:3.12-slim") -_ASSUMED_CONTEXT_WINDOW = 1_000_000 -_CONTEXT_SUMMARIZATION_CUTOFF = 0.85 -_RESPONSES_FALLBACK_STATUS_CODES = frozenset( - {400, 404, 405, 415, 422, 500, 501, 502, 503, 504} -) +_DEFAULT_CODE_IMAGE = os.environ.get("STIRRUP_CODE_IMAGE", "assetops-code") +_WORKING_CONTEXT_BUDGET = 100_000 +_CONTEXT_SUMMARIZATION_CUTOFF = 0.75 _CODE_EXEC_SYSTEM_PROMPT = """\ -Code execution guidance: -- Treat the MCP tools as the authoritative source for asset and domain data. - Do not query CouchDB or other backing services directly from code. -- Use code_exec for calculations, data transformations, workspace file - inspection, and result validation. Do not use it instead of an available MCP - tool for retrieving domain data. -- Use relative paths within the current execution workspace. Files and other - workspace state persist across code_exec calls during this run. -- Run non-interactive, bounded commands. Check that a package is installed - before relying on it, and use the Python standard library when practical. -- Verify computed results before answering. Put the answer and its key evidence - in the finish reason; stdout and workspace files are not part of the final - answer by themselves. +Code execution: +- MCP tools and their definitions are authoritative for domain data and semantics. + Never use code to query backing services or bypass an available MCP tool. +- Do not overuse code_exec. Answer directly from MCP results, domain knowledge, + and basic reasoning or arithmetic when sufficient. Use code_exec only for + necessary computation, data processing, workspace inspection, or validation. + Never use it for planning, comments, placeholders, or empty scripts. +- Prefer one complete script that inspects, analyzes, and verifies. Do not repeat + equivalent experiments; correct failures directly. +- Stay inside the execution workspace and use relative paths. Workspace state + persists across code_exec calls. +- For artifacts, inspect only the schema, counts, a small sample, or the specific + rows or fields needed, then process in place. If an artifact exceeds 200 KiB, + never print it in full; extract and process the relevant subset in bounded + batches. Avoid large record lists and verbose diagnostics. Reuse snapshots + unless domain state has changed. """ _DOCKER_CODE_EXEC_SYSTEM_PROMPT = """\ The Docker execution workspace is /workspace. Host filesystem paths are not -available inside the container. The image might include scientific packages -such as numpy, pandas, scipy, or matplotlib. Verify them before relying on them. +available inside the container. NumPy, pandas, and SciPy are installed; check +availability before using other packages. """ _LOCAL_CODE_EXEC_SYSTEM_PROMPT = """\ The local execution workspace is a temporary directory, but commands run on the @@ -84,28 +83,14 @@ """ -def _responses_error_status_code(exc: Exception) -> int | None: - """Return an HTTP status, including errors wrapped by Tenacity retries.""" - status_code = getattr(exc, "status_code", None) - if isinstance(status_code, int): - return status_code - - last_attempt = getattr(exc, "last_attempt", None) - if last_attempt is None: - return None - nested = last_attempt.exception() - nested_status = getattr(nested, "status_code", None) - return nested_status if isinstance(nested_status, int) else None - - class _ContextWindowClient: - """Report the assumed context size without changing the output-token cap. + """Report the working context budget without changing the output-token cap. Stirrup currently reads ``LLMClient.max_tokens`` both when configuring the provider's maximum output and when deciding whether to summarize context. Keeping the provider client behind this adapter lets it retain its native - 64k output default while the agent loop uses the benchmark's 1M-context - assumption. + 64k output default while the agent loop uses a lower working-context budget + for earlier summarization. """ def __init__(self, client: Any) -> None: @@ -113,7 +98,7 @@ def __init__(self, client: Any) -> None: @property def max_tokens(self) -> int: - return _ASSUMED_CONTEXT_WINDOW + return _WORKING_CONTEXT_BUDGET @property def model_slug(self) -> str: @@ -125,42 +110,19 @@ async def generate( return await self._client.generate(messages, tools) -class _ResponsesThenChatClient: - """Prefer Open Responses, then stick to Chat Completions if unavailable.""" +def _build_full_summary_logger(): + """Return a Stirrup logger that displays generated summaries without truncation.""" + from rich.text import Text + from stirrup.utils.logging import AgentLogger, console - def __init__(self, responses_client: Any, chat_client: Any) -> None: - self._responses_client = responses_client - self._chat_client = chat_client - self._use_chat_completions = False + class _FullSummaryLogger(AgentLogger): + def context_summarization_complete( + self, summary: str, bridge: str + ) -> None: + console.print(Text("✓ Summary Generated", style="bold green")) + console.print(summary, markup=False, soft_wrap=True) - @property - def max_tokens(self) -> int: - return self._responses_client.max_tokens - - @property - def model_slug(self) -> str: - return self._responses_client.model_slug - - async def generate( - self, messages: list[Any], tools: dict[str, Any] - ) -> Any: - if self._use_chat_completions: - return await self._chat_client.generate(messages, tools) - - try: - return await self._responses_client.generate(messages, tools) - except Exception as exc: - status_code = _responses_error_status_code(exc) - if status_code not in _RESPONSES_FALLBACK_STATUS_CODES: - raise - self._use_chat_completions = True - _log.warning( - "Responses API unavailable for model %s (HTTP %s); " - "using Chat Completions for the remainder of the run", - self.model_slug, - status_code, - ) - return await self._chat_client.generate(messages, tools) + return _FullSummaryLogger() def _copy_workspace_contents(source: Path, destination: Path) -> None: @@ -269,7 +231,6 @@ def _build_client(self): creds = resolve_router_creds(self._model_id) if creds is not None: from stirrup.clients.chat_completions_client import ChatCompletionsClient - from stirrup.clients.open_responses_client import OpenResponsesClient common_kwargs = { "model": resolve_model(self._model_id), @@ -278,10 +239,7 @@ def _build_client(self): "reasoning_effort": self._reasoning_effort, "kwargs": client_kwargs, } - client = _ResponsesThenChatClient( - responses_client=OpenResponsesClient(**common_kwargs), - chat_client=ChatCompletionsClient(**common_kwargs), - ) + client = ChatCompletionsClient(**common_kwargs) else: from stirrup.clients.litellm_client import LiteLLMClient @@ -292,13 +250,13 @@ def _build_client(self): ) return _ContextWindowClient(client) - def _build_mcp_provider(self): - """Build a Stirrup ``MCPToolProvider`` for the AssetOpsBench servers. + def _build_mcp_config(self): + """Build the Stirrup MCP configuration for AssetOpsBench servers. Each server is a stdio process launched exactly as the other runners launch it: ``uv run --directory ``. """ - from stirrup.tools.mcp import MCPConfig, MCPToolProvider + from stirrup.tools.mcp import MCPConfig servers: dict[str, dict] = {} for name, spec in self._server_paths.items(): @@ -308,8 +266,22 @@ def _build_mcp_provider(self): "args": ["run", "--directory", str(_REPO_ROOT), cmd_arg], "cwd": str(_REPO_ROOT), } - config = MCPConfig.model_validate({"mcpServers": servers}) - return MCPToolProvider(config=config) + return MCPConfig.model_validate({"mcpServers": servers}) + + def _build_mcp_provider(self, *, exec_env=None): + """Build the MCP provider, bridging large results when code is enabled.""" + config = self._build_mcp_config() + if exec_env is None: + from stirrup.tools.mcp import MCPToolProvider + + return MCPToolProvider(config=config) + + from .workspace_bridge import WorkspaceBridgedMCPToolProvider + + return WorkspaceBridgedMCPToolProvider( + config=config, + exec_env=exec_env, + ) def _build_code_provider(self): """Build the sandboxed code-execution provider for the code track.""" @@ -338,11 +310,14 @@ def _build_code_provider(self): ) def _build_tools(self) -> list: - tools: list = [] - if self._code_enabled: - tools.append(self._build_code_provider()) - tools.append(self._build_mcp_provider()) - return tools + if not self._code_enabled: + return [self._build_mcp_provider()] + + code_provider = self._build_code_provider() + return [ + code_provider, + self._build_mcp_provider(exec_env=code_provider), + ] def _build_system_prompt(self) -> str: """Append code-execution guidance when the code track is enabled.""" @@ -367,13 +342,16 @@ async def run(self, question: str) -> AgentResult: run_started = time.perf_counter() started_at = _dt.datetime.now(_dt.UTC).isoformat() + client = self._build_client() agent = Agent( - client=self._build_client(), + client=client, name="assetops", system_prompt=self._build_system_prompt(), tools=self._build_tools(), + finish_tool=ASSETOPS_FINISH_TOOL, max_turns=self._max_turns, context_summarization_cutoff=_CONTEXT_SUMMARIZATION_CUTOFF, + logger=_build_full_summary_logger(), ) _log.info( diff --git a/src/agent/stirrup_agent/tests/test_finish_tool.py b/src/agent/stirrup_agent/tests/test_finish_tool.py new file mode 100644 index 00000000..0f66c33d --- /dev/null +++ b/src/agent/stirrup_agent/tests/test_finish_tool.py @@ -0,0 +1,91 @@ +"""Tests for the AssetOps-specific Stirrup finish contract.""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError +from stirrup import Agent +from stirrup.core.models import AssistantMessage, ToolCall + +from agent.stirrup_agent.finish_tool import ( + ASSETOPS_FINISH_TOOL, + AssetOpsFinishParams, + structured_finish_answer, +) + + +def test_finish_params_require_a_non_empty_answer(): + with pytest.raises(ValidationError): + AssetOpsFinishParams(answer="") + with pytest.raises(ValidationError): + AssetOpsFinishParams(answer=" \n") + + +def test_finish_params_keep_reason_and_paths_optional(): + params = AssetOpsFinishParams(answer='{"count":7}') + + assert params.answer == '{"count":7}' + assert params.reason == "" + assert params.paths == [] + + +@pytest.mark.anyio +async def test_finish_tool_returns_the_user_facing_answer(): + params = AssetOpsFinishParams( + answer='["PMP-01"]', + reason="Located the requested pump.", + ) + + result = await ASSETOPS_FINISH_TOOL.executor(params) + + assert result.success is True + assert result.content == '["PMP-01"]' + + +def test_structured_finish_answer_rejects_legacy_params(): + class _LegacyFinish: + reason = "done" + + assert structured_finish_answer(_LegacyFinish()) is None + + +def test_structured_finish_answer_strips_boundary_whitespace(): + params = AssetOpsFinishParams(answer=" 42\n") + + assert structured_finish_answer(params) == "42" + + +@pytest.mark.anyio +async def test_stirrup_agent_returns_custom_finish_params(): + class _Client: + max_tokens = 100_000 + model_slug = "fake/custom-finish" + + async def generate(self, messages, tools): + assert tools["finish"].parameters is AssetOpsFinishParams + return AssistantMessage( + content="", + tool_calls=[ + ToolCall( + name="finish", + arguments=( + '{"answer":"[1,2]","reason":"done","paths":[]}' + ), + tool_call_id="finish-1", + ) + ], + ) + + agent = Agent( + client=_Client(), + name="assetops-test", + tools=[], + finish_tool=ASSETOPS_FINISH_TOOL, + max_turns=2, + ) + + finish_params, history, _metadata = await agent.run("Return a JSON array.") + + assert isinstance(finish_params, AssetOpsFinishParams) + assert finish_params.answer == "[1,2]" + assert history[-1][-1].content == "[1,2]" diff --git a/src/agent/stirrup_agent/tests/test_runner.py b/src/agent/stirrup_agent/tests/test_runner.py index 78eb1d63..c64430c2 100644 --- a/src/agent/stirrup_agent/tests/test_runner.py +++ b/src/agent/stirrup_agent/tests/test_runner.py @@ -7,15 +7,20 @@ from __future__ import annotations +import importlib from dataclasses import dataclass, field from pathlib import Path +from unittest.mock import MagicMock import pytest from agent._prompts import AGENT_SYSTEM_PROMPT +from agent.stirrup_agent.finish_tool import ASSETOPS_FINISH_TOOL from agent.stirrup_agent.runner import ( StirrupAgentRunner, - _ResponsesThenChatClient, + _CONTEXT_SUMMARIZATION_CUTOFF, + _WORKING_CONTEXT_BUDGET, + _build_full_summary_logger, _copy_workspace_contents, ) from agent.stirrup_agent.trajectory import ( @@ -23,6 +28,7 @@ classify_tool, final_answer, ) +from agent.stirrup_agent.workspace_bridge import WorkspaceBridgedMCPToolProvider _DOMAIN = {"iot", "utilities", "fmsr", "tsfm", "wo", "vibration"} @@ -70,6 +76,13 @@ class _Finish: reason: str +@dataclass +class _StructuredFinish: + answer: str + reason: str = "" + paths: list[str] = field(default_factory=list) + + def test_classify_tool(): assert classify_tool("wo__get_work_order", _DOMAIN) == "domain" assert classify_tool("vibration__compute_fft", _DOMAIN) == "domain" @@ -102,32 +115,19 @@ def test_stirrup_runner_rejects_unsupported_code_backend(): StirrupAgentRunner(code_backend="e2b") -def test_stirrup_runner_uses_shared_prompt_when_code_is_disabled(): - runner = StirrupAgentRunner(code_enabled=False) +def test_stirrup_runner_bridges_mcp_results_when_code_is_enabled(): + runner = StirrupAgentRunner(code_backend="local") - assert runner._build_system_prompt() == AGENT_SYSTEM_PROMPT + code_provider, mcp_provider = runner._build_tools() + assert isinstance(mcp_provider, WorkspaceBridgedMCPToolProvider) + assert mcp_provider._exec_env is code_provider -def test_stirrup_runner_appends_docker_code_guidance(): - prompt = StirrupAgentRunner(code_backend="docker")._build_system_prompt() - assert prompt.startswith(AGENT_SYSTEM_PROMPT) - assert "Treat the MCP tools as the authoritative source" in prompt - assert "finish reason" in prompt - assert "/workspace" in prompt - assert "scientific packages" in prompt - assert "Verify them before relying on them" in prompt - assert "host with the current user's permissions" not in prompt - - -def test_stirrup_runner_appends_local_code_guidance(): - prompt = StirrupAgentRunner(code_backend="local")._build_system_prompt() +def test_stirrup_runner_uses_shared_prompt_when_code_is_disabled(): + runner = StirrupAgentRunner(code_enabled=False) - assert prompt.startswith(AGENT_SYSTEM_PROMPT) - assert "code_exec" in prompt - assert "host with the current user's permissions" in prompt - assert "use relative paths" in prompt - assert "/workspace" not in prompt + assert runner._build_system_prompt() == AGENT_SYSTEM_PROMPT def test_stirrup_runner_forwards_temperature_to_litellm_client(): @@ -143,7 +143,7 @@ def test_stirrup_runner_forwards_temperature_to_litellm_client(): assert provider_client._kwargs == {"temperature": 0.2} assert provider_client._reasoning_effort == "high" assert provider_client.max_tokens == 64_000 - assert client.max_tokens == 1_000_000 + assert client.max_tokens == 100_000 def test_stirrup_runner_forwards_temperature_to_router_client( @@ -161,96 +161,29 @@ def test_stirrup_runner_forwards_temperature_to_router_client( client = runner._build_client() from stirrup.clients.chat_completions_client import ChatCompletionsClient - from stirrup.clients.open_responses_client import OpenResponsesClient router_client = client._client - assert isinstance(router_client._responses_client, OpenResponsesClient) - assert isinstance(router_client._chat_client, ChatCompletionsClient) - assert router_client._responses_client._kwargs == {"temperature": 0.2} - assert router_client._chat_client._kwargs == {"temperature": 0.2} - assert router_client._responses_client._reasoning_effort == "medium" - assert router_client._chat_client._reasoning_effort == "medium" + assert isinstance(router_client, ChatCompletionsClient) + assert router_client._kwargs == {"temperature": 0.2} + assert router_client._reasoning_effort == "medium" assert router_client.max_tokens == 64_000 - assert client.max_tokens == 1_000_000 - -class _FakeClient: - def __init__(self, *, result=None, error: Exception | None = None) -> None: - self.result = result - self.error = error - self.calls = 0 - self.max_tokens = 64_000 - self.model_slug = "test-model" - - async def generate(self, messages, tools): - self.calls += 1 - if self.error is not None: - raise self.error - return self.result - - -class _StatusError(Exception): - def __init__(self, status_code: int) -> None: - super().__init__(f"HTTP {status_code}") - self.status_code = status_code - - -class _LastAttempt: - def __init__(self, error: Exception) -> None: - self._error = error - - def exception(self) -> Exception: - return self._error - - -class _RetryError(Exception): - def __init__(self, error: Exception) -> None: - super().__init__("retries exhausted") - self.last_attempt = _LastAttempt(error) - - -@pytest.mark.anyio -async def test_router_client_prefers_open_responses(): - responses = _FakeClient(result="responses result") - chat = _FakeClient(result="chat response") - client = _ResponsesThenChatClient(responses, chat) - - assert await client.generate([], {}) == "responses result" - assert responses.calls == 1 - assert chat.calls == 0 + assert client.max_tokens == 100_000 -@pytest.mark.anyio -async def test_router_client_falls_back_to_chat_and_sticks_to_it(): - responses = _FakeClient(error=_StatusError(404)) - chat = _FakeClient(result="chat response") - client = _ResponsesThenChatClient(responses, chat) +def test_stirrup_runner_uses_75k_summarization_trigger(): + assert _WORKING_CONTEXT_BUDGET == 100_000 + assert _CONTEXT_SUMMARIZATION_CUTOFF == 0.75 + assert _WORKING_CONTEXT_BUDGET * _CONTEXT_SUMMARIZATION_CUTOFF == 75_000 - assert await client.generate([], {}) == "chat response" - assert await client.generate([], {}) == "chat response" - assert responses.calls == 1 - assert chat.calls == 2 +def test_full_summary_logger_does_not_truncate(capsys: pytest.CaptureFixture[str]): + marker = "SUMMARY_END_MARKER_1234567890" + summary = "x" * 900 + marker -@pytest.mark.anyio -async def test_router_client_falls_back_after_retried_server_error(): - responses = _FakeClient(error=_RetryError(_StatusError(500))) - chat = _FakeClient(result="chat response") - client = _ResponsesThenChatClient(responses, chat) + logger = _build_full_summary_logger() + logger.context_summarization_complete(summary, "unused bridge") - assert await client.generate([], {}) == "chat response" - assert responses.calls == 1 - assert chat.calls == 1 - - -@pytest.mark.anyio -async def test_router_client_does_not_mask_non_interface_errors(): - responses = _FakeClient(error=RuntimeError("request failed")) - chat = _FakeClient(result="chat response") - client = _ResponsesThenChatClient(responses, chat) - - with pytest.raises(RuntimeError, match="request failed"): - await client.generate([], {}) - assert chat.calls == 0 + assert marker in capsys.readouterr().out def test_build_trajectory_maps_turns_calls_and_outputs(): @@ -315,6 +248,19 @@ def test_final_answer_prefers_finish_turn_content_over_finish_reason(): ) +def test_final_answer_prefers_structured_finish_answer_over_turn_content(): + history = [ + [ + _Assistant( + content="Task complete.", + tool_calls=[_TC("finish", '{"answer":"42"}', "f1")], + ) + ] + ] + + assert final_answer(history, _StructuredFinish(answer="42")) == "42" + + def test_final_answer_uses_finish_reason_when_finish_turn_content_is_empty(): history = [ [_Assistant(content="I will inspect the work orders.")], @@ -346,3 +292,104 @@ def test_arguments_parsed_when_already_dict(): ] traj = build_trajectory(history) assert traj.all_tool_calls[0].input == {"asset": "CH6"} + + +@pytest.mark.anyio +async def test_run_persists_legacy_final_answer( + monkeypatch: pytest.MonkeyPatch, +): + runner_module = importlib.import_module("agent.stirrup_agent.runner") + client = object() + history = [ + [ + _Assistant( + content="calculating", + tool_calls=[_TC("code_exec", '{"cmd":"calculate"}', "c1")], + ), + _Tool(content="FINAL=7", tool_call_id="c1", name="code_exec"), + ], + [ + _Assistant( + content='The requested result is {"count":7}.', + tool_calls=[_TC("finish", '{"reason":"done"}', "f1")], + ) + ], + ] + + class _FakeAgent: + def __init__(self, **kwargs): + assert kwargs["client"] is client + assert kwargs["finish_tool"] is ASSETOPS_FINISH_TOOL + + def session(self): + return self + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc_value, traceback): + return None + + async def run(self, question): + assert question == "Return a JSON object." + return _Finish("run complete"), history, {} + + persist = MagicMock() + monkeypatch.setattr("stirrup.Agent", _FakeAgent) + monkeypatch.setattr(runner_module, "persist_trajectory", persist) + + runner = StirrupAgentRunner(server_paths={}, code_enabled=False) + monkeypatch.setattr(runner, "_build_client", lambda: client) + monkeypatch.setattr(runner, "_build_tools", lambda: []) + + result = await runner.run("Return a JSON object.") + + assert result.answer == 'The requested result is {"count":7}.' + persist.assert_called_once() + assert persist.call_args.kwargs["answer"] == result.answer + + +@pytest.mark.anyio +async def test_run_uses_structured_finish_without_repair_call( + monkeypatch: pytest.MonkeyPatch, +): + runner_module = importlib.import_module("agent.stirrup_agent.runner") + client = object() + history = [ + [ + _Assistant( + content="Task complete.", + tool_calls=[_TC("finish", '{"answer":"[1,2]"}', "f1")], + ) + ] + ] + + class _FakeAgent: + def __init__(self, **kwargs): + assert kwargs["client"] is client + assert kwargs["finish_tool"] is ASSETOPS_FINISH_TOOL + + def session(self): + return self + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc_value, traceback): + return None + + async def run(self, question): + return _StructuredFinish(answer="[1,2]"), history, {} + + persist = MagicMock() + monkeypatch.setattr("stirrup.Agent", _FakeAgent) + monkeypatch.setattr(runner_module, "persist_trajectory", persist) + + runner = StirrupAgentRunner(server_paths={}, code_enabled=False) + monkeypatch.setattr(runner, "_build_client", lambda: client) + monkeypatch.setattr(runner, "_build_tools", lambda: []) + + result = await runner.run("Return a JSON array.") + + assert result.answer == "[1,2]" + assert persist.call_args.kwargs["answer"] == "[1,2]" diff --git a/src/agent/stirrup_agent/tests/test_workspace_bridge.py b/src/agent/stirrup_agent/tests/test_workspace_bridge.py new file mode 100644 index 00000000..9fcf1003 --- /dev/null +++ b/src/agent/stirrup_agent/tests/test_workspace_bridge.py @@ -0,0 +1,249 @@ +"""Tests for automatic persistence of large MCP tool results.""" + +from __future__ import annotations + +import hashlib +import json + +import pytest +from pydantic import BaseModel +from stirrup.core.models import Tool, ToolResult, ToolUseCountMetadata +from stirrup.tools.mcp import MCPConfig + +from agent.stirrup_agent.workspace_bridge import ( + WorkspaceBridgedMCPToolProvider, +) + + +class _Params(BaseModel): + page_size: int = 0 + failure_code: str | None = None + + +class _FakeExecEnvironment: + def __init__(self, *, fail_writes: bool = False) -> None: + self.files: dict[str, bytes] = {} + self.fail_writes = fail_writes + + async def write_file_bytes(self, path: str, content: bytes) -> None: + if self.fail_writes: + raise RuntimeError("write failed") + self.files[path] = content + + async def file_exists(self, path: str) -> bool: + return path in self.files + + async def read_file_bytes(self, path: str) -> bytes: + if path not in self.files: + raise FileNotFoundError(path) + return self.files[path] + + +def _provider( + exec_env: _FakeExecEnvironment, + *, + threshold: int = 32, +) -> WorkspaceBridgedMCPToolProvider: + config = MCPConfig.model_validate({"mcpServers": {}}) + return WorkspaceBridgedMCPToolProvider( + config=config, + exec_env=exec_env, # type: ignore[arg-type] + persist_threshold_bytes=threshold, + ) + + +def _tool(name: str, content: str, calls: list[str]) -> Tool: + async def executor(params: _Params) -> ToolResult[ToolUseCountMetadata]: + calls.append(name) + return ToolResult( + content=content, + metadata=ToolUseCountMetadata(), + ) + + return Tool( + name=name, + description="Test MCP tool", + parameters=_Params, + executor=executor, + ) + + +@pytest.mark.anyio +async def test_large_result_is_persisted_and_reused() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + calls: list[str] = [] + content = json.dumps({"work_orders": [{"description": "x" * 200}]}) + tool = provider._wrap_tool(_tool("wo__list_workorders", content, calls)) + + assert tool.description == "Test MCP tool" + + first = await tool.executor(_Params()) + first_handle = json.loads(first.content) + + assert calls == ["wo__list_workorders"] + assert first_handle["cached"] is False + assert first_handle["workspace_file"].startswith( + "mcp_results/wo__list_workorders_" + ) + assert exec_env.files[first_handle["workspace_file"]] == content.encode() + assert first_handle["bytes"] == len(content.encode()) + assert first_handle["sha256"] == hashlib.sha256(content.encode()).hexdigest() + assert first_handle["arguments"]["failure_code"] is None + assert "never dump the entire file" in first_handle["instruction"] + + second = await tool.executor(_Params()) + second_handle = json.loads(second.content) + + assert calls == ["wo__list_workorders"] + assert second_handle["cached"] is True + assert second_handle["workspace_file"] == first_handle["workspace_file"] + + +@pytest.mark.anyio +async def test_small_result_remains_inline() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env, threshold=1_000) + calls: list[str] = [] + tool = provider._wrap_tool(_tool("wo__get_failure_codes", "{}", calls)) + + result = await tool.executor(_Params()) + + assert result.content == "{}" + assert calls == ["wo__get_failure_codes"] + assert exec_env.files == {} + + +@pytest.mark.anyio +async def test_modified_artifact_is_refetched() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + calls: list[str] = [] + content = json.dumps({"work_orders": ["x" * 200]}) + tool = provider._wrap_tool(_tool("wo__list_workorders", content, calls)) + + first = await tool.executor(_Params()) + handle = json.loads(first.content) + exec_env.files[handle["workspace_file"]] = b"modified" + + await tool.executor(_Params()) + + assert calls == ["wo__list_workorders", "wo__list_workorders"] + assert exec_env.files[handle["workspace_file"]] == content.encode() + + +@pytest.mark.anyio +async def test_persistence_failure_falls_back_to_inline_result() -> None: + exec_env = _FakeExecEnvironment(fail_writes=True) + provider = _provider(exec_env) + calls: list[str] = [] + content = "x" * 200 + tool = provider._wrap_tool(_tool("wo__list_workorders", content, calls)) + + result = await tool.executor(_Params()) + + assert result.content == content + assert calls == ["wo__list_workorders"] + + +@pytest.mark.anyio +async def test_work_order_mutation_invalidates_cached_reads() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + read_calls: list[str] = [] + mutation_calls: list[str] = [] + read_content = json.dumps({"work_orders": ["x" * 200]}) + read_tool = provider._wrap_tool( + _tool("wo__list_workorders", read_content, read_calls) + ) + mutation_tool = provider._wrap_tool( + _tool("wo__update_workorder", '{"success": true}', mutation_calls) + ) + + await read_tool.executor(_Params()) + await read_tool.executor(_Params()) + assert len(read_calls) == 1 + + await mutation_tool.executor(_Params()) + await read_tool.executor(_Params()) + + assert mutation_calls == ["wo__update_workorder"] + assert len(read_calls) == 2 + + +@pytest.mark.anyio +async def test_refetched_snapshot_does_not_overwrite_prior_artifact() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + responses = iter( + [ + json.dumps({"work_orders": ["old" * 100]}), + json.dumps({"work_orders": ["new" * 100]}), + ] + ) + + async def read_executor( + params: _Params, + ) -> ToolResult[ToolUseCountMetadata]: + return ToolResult( + content=next(responses), + metadata=ToolUseCountMetadata(), + ) + + read_tool = provider._wrap_tool( + Tool( + name="wo__list_workorders", + description="Test MCP tool", + parameters=_Params, + executor=read_executor, + ) + ) + mutation_tool = provider._wrap_tool( + _tool("wo__update_workorder", '{"success": true}', []) + ) + + first = json.loads((await read_tool.executor(_Params())).content) + await mutation_tool.executor(_Params()) + second = json.loads((await read_tool.executor(_Params())).content) + + assert first["workspace_file"] != second["workspace_file"] + assert b"old" in exec_env.files[first["workspace_file"]] + assert b"new" in exec_env.files[second["workspace_file"]] + + +@pytest.mark.anyio +async def test_non_work_order_mutation_invalidates_cached_reads() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + read_calls: list[str] = [] + read_tool = provider._wrap_tool( + _tool( + "tsfm__list_models", + json.dumps({"models": ["x" * 200]}), + read_calls, + ) + ) + mutation_tool = provider._wrap_tool( + _tool("tsfm__register_model", '{"status": "registered"}', []) + ) + + await read_tool.executor(_Params()) + await read_tool.executor(_Params()) + await mutation_tool.executor(_Params()) + await read_tool.executor(_Params()) + + assert len(read_calls) == 2 + + +@pytest.mark.anyio +async def test_mutation_calls_are_never_cached() -> None: + exec_env = _FakeExecEnvironment() + provider = _provider(exec_env) + calls: list[str] = [] + content = json.dumps({"work_order": "x" * 200}) + tool = provider._wrap_tool(_tool("wo__update_workorder", content, calls)) + + await tool.executor(_Params()) + await tool.executor(_Params()) + + assert calls == ["wo__update_workorder", "wo__update_workorder"] diff --git a/src/agent/stirrup_agent/trajectory.py b/src/agent/stirrup_agent/trajectory.py index 6ac5cb06..ba8b9bc3 100644 --- a/src/agent/stirrup_agent/trajectory.py +++ b/src/agent/stirrup_agent/trajectory.py @@ -152,11 +152,16 @@ def build_trajectory(history: Iterable[Any]) -> Trajectory: def final_answer(history: Iterable[Any], finish_params: Any) -> str: """Return the user-facing answer from a completed Stirrup run. - ``finish.reason`` describes why the agent stopped and is not necessarily - the answer shown to the user. Prefer non-empty assistant content emitted - alongside the ``finish`` call, while retaining the reason as a fallback for - agents that put no content on that turn. + Prefer the explicit ``finish.answer`` supplied by the AssetOps finish tool. + For legacy/default finish tools, ``finish.reason`` describes why the agent + stopped and is not necessarily the answer shown to the user, so prefer + non-empty assistant content emitted alongside the ``finish`` call while + retaining the reason as a fallback. """ + structured_answer = getattr(finish_params, "answer", None) + if isinstance(structured_answer, str) and structured_answer.strip(): + return structured_answer.strip() + messages = _flatten(history) for msg in reversed(messages): diff --git a/src/agent/stirrup_agent/workspace_bridge.py b/src/agent/stirrup_agent/workspace_bridge.py new file mode 100644 index 00000000..4e6920b3 --- /dev/null +++ b/src/agent/stirrup_agent/workspace_bridge.py @@ -0,0 +1,210 @@ +"""Persist large MCP results in the active Stirrup code workspace. + +The LLM should not have to copy a large MCP response into ``code_exec`` just to +analyze it. This provider wraps Stirrup's MCP tools so oversized text results +are written to the existing code-execution environment and replaced in the +conversation with a compact, cacheable artifact handle. +""" + +from __future__ import annotations + +import hashlib +import json +import logging +import re +from dataclasses import asdict, dataclass +from typing import Any + +from pydantic import BaseModel +from stirrup.core.models import Tool, ToolResult, ToolUseCountMetadata +from stirrup.tools.code_backends.base import CodeExecToolProvider +from stirrup.tools.mcp import MCPConfig, MCPToolProvider + +_log = logging.getLogger(__name__) + +DEFAULT_PERSIST_THRESHOLD_BYTES = 100 * 1024 +_ARTIFACT_DIRECTORY = "mcp_results" +_MUTATING_TOOLS = { + "fmsr__add_failure_modes", + "tsfm__deprecate_feature", + "tsfm__deprecate_model", + "tsfm__new_feature_version", + "tsfm__new_model_version", + "tsfm__register_feature", + "tsfm__register_finetuned", + "tsfm__register_model", + "tsfm__run_plan", + "tsfm__run_recipe", + "tsfm__run_tabular_recipe", + "tsfm__update_feature", + "tsfm__update_model", + "wo__generate_work_order", + "wo__update_workorder", + "wo__approve_workorder", + "wo__assign_technician", + "wo__close_workorder", + "wo__cancel_workorder", +} + + +@dataclass(frozen=True) +class MCPResultArtifact: + """A durable workspace snapshot of one MCP tool result.""" + + workspace_file: str + tool: str + arguments: dict[str, Any] + bytes: int + sha256: str + + def tool_content(self, *, cached: bool = False) -> str: + payload = { + **asdict(self), + "artifact_type": "mcp_result", + "cached": cached, + "instruction": ( + "Treat workspace_file as the complete, read-only MCP snapshot. " + "Process it in place with code_exec and print only required " + "fields or aggregates; never dump the entire file. Do not repeat " + "this MCP read unless the underlying domain state has changed." + ), + } + return json.dumps(payload, ensure_ascii=False, sort_keys=True) + + +def _normalized_arguments(params: BaseModel) -> dict[str, Any]: + return params.model_dump(mode="json") + + +def _query_key(tool_name: str, arguments: dict[str, Any]) -> str: + normalized = json.dumps( + arguments, + ensure_ascii=False, + sort_keys=True, + separators=(",", ":"), + ) + return hashlib.sha256(f"{tool_name}:{normalized}".encode()).hexdigest() + + +def _artifact_extension(content: str) -> str: + try: + json.loads(content) + except json.JSONDecodeError: + return "txt" + return "json" + + +def _artifact_path( + tool_name: str, + query_key: str, + content_hash: str, + extension: str, +) -> str: + safe_name = re.sub(r"[^A-Za-z0-9_.-]+", "_", tool_name) + return ( + f"{_ARTIFACT_DIRECTORY}/{safe_name}_{query_key[:12]}_" + f"{content_hash[:12]}.{extension}" + ) + + +class WorkspaceBridgedMCPToolProvider(MCPToolProvider): + """MCP provider that spills oversized text results into ``code_exec``.""" + + def __init__( + self, + config: MCPConfig, + *, + exec_env: CodeExecToolProvider, + persist_threshold_bytes: int = DEFAULT_PERSIST_THRESHOLD_BYTES, + ) -> None: + if persist_threshold_bytes <= 0: + raise ValueError("persist_threshold_bytes must be positive") + super().__init__(config=config) + self._exec_env = exec_env + self._persist_threshold_bytes = persist_threshold_bytes + self._artifacts: dict[str, MCPResultArtifact] = {} + + async def _artifact_is_intact(self, artifact: MCPResultArtifact) -> bool: + try: + payload = await self._exec_env.read_file_bytes(artifact.workspace_file) + except Exception: + _log.debug( + "Unable to validate cached MCP artifact %s", + artifact.workspace_file, + exc_info=True, + ) + return False + return hashlib.sha256(payload).hexdigest() == artifact.sha256 + + async def __aenter__(self) -> list[Tool[Any, ToolUseCountMetadata]]: + tools = await super().__aenter__() + return [self._wrap_tool(tool) for tool in tools] + + def _wrap_tool( + self, tool: Tool[Any, ToolUseCountMetadata] + ) -> Tool[Any, ToolUseCountMetadata]: + original_executor = tool.executor + + async def executor( + params: BaseModel, + ) -> ToolResult[ToolUseCountMetadata]: + arguments = _normalized_arguments(params) + query_key = _query_key(tool.name, arguments) + cacheable = tool.name not in _MUTATING_TOOLS + artifact = self._artifacts.get(query_key) if cacheable else None + + if artifact is not None and await self._artifact_is_intact(artifact): + return ToolResult( + content=artifact.tool_content(cached=True), + metadata=ToolUseCountMetadata(), + ) + + result = await original_executor(params) + if result.success and tool.name in _MUTATING_TOOLS: + self._artifacts.clear() + + if not result.success or not isinstance(result.content, str): + return result + + payload = result.content.encode("utf-8") + if len(payload) <= self._persist_threshold_bytes: + return result + + content_hash = hashlib.sha256(payload).hexdigest() + path = _artifact_path( + tool.name, + query_key, + content_hash, + _artifact_extension(result.content), + ) + try: + await self._exec_env.write_file_bytes(path, payload) + except Exception: + _log.warning( + "Failed to persist large MCP result from %s; returning it inline", + tool.name, + exc_info=True, + ) + return result + + artifact = MCPResultArtifact( + workspace_file=path, + tool=tool.name, + arguments=arguments, + bytes=len(payload), + sha256=content_hash, + ) + if cacheable: + self._artifacts[query_key] = artifact + return ToolResult( + content=artifact.tool_content(), + success=result.success, + metadata=result.metadata, + ) + + return Tool( + name=tool.name, + description=tool.description, + parameters=tool.parameters, + executor=executor, + ) diff --git a/src/benchmark/scenario_suite_runner.py b/src/benchmark/scenario_suite_runner.py index 08868b87..ad5808f3 100644 --- a/src/benchmark/scenario_suite_runner.py +++ b/src/benchmark/scenario_suite_runner.py @@ -75,10 +75,9 @@ def load_scenario_profile(path: Path) -> dict[str, tuple[str, ...]]: profile: dict[str, tuple[str, ...]] = {} for category in SCENARIO_CATEGORY_ORDER: raw_ids = raw_profile[category] - if not isinstance(raw_ids, list) or not raw_ids: + if not isinstance(raw_ids, list): raise ValueError( - f"Scenario profile category {category!r} must be a non-empty list: " - f"{path}" + f"Scenario profile category {category!r} must be a list: {path}" ) scenario_ids: list[str] = [] @@ -100,6 +99,11 @@ def load_scenario_profile(path: Path) -> dict[str, tuple[str, ...]]: ) profile[category] = tuple(scenario_ids) + if not any(profile.values()): + raise ValueError( + f"Scenario profile must contain at least one scenario id: {path}" + ) + return profile diff --git a/src/benchmark/tests/test_scenario_suite_runner.py b/src/benchmark/tests/test_scenario_suite_runner.py index 19b9a8dd..84116317 100644 --- a/src/benchmark/tests/test_scenario_suite_runner.py +++ b/src/benchmark/tests/test_scenario_suite_runner.py @@ -60,39 +60,10 @@ def test_scenario_mappings_cover_expected_categories() -> None: str(scenario_id) for scenario_id in range(1, 67) ) assert all( - len(mr.SCENARIO_IDS_LITE[category]) == 10 - for category in {"car", "fcc", "fmsr", "health", "wosr"} - ) - assert mr.SCENARIO_IDS_LITE["car"] == ( - "151", - "152", - "153", - "156", - "167", - "178", - "180", - "182", - "183", - "193", - ) - assert mr.SCENARIO_IDS_LITE["health"] == tuple( - str(scenario_id) for scenario_id in range(401, 411) - ) - assert mr.SCENARIO_IDS_LITE["tsfm"] == tuple( - str(scenario_id) for scenario_id in range(1001, 1006) - ) - assert mr.SCENARIO_IDS_LITE["wosr"] == ( - "5", - "9", - "13", - "20", - "24", - "31", - "43", - "50", - "61", - "66", + mr.SCENARIO_IDS_LITE[category] + for category in expected - {"tsfm"} ) + assert mr.SCENARIO_IDS_LITE["tsfm"] == () def test_scenario_profiles_are_loaded_from_yaml() -> None: @@ -177,6 +148,43 @@ def test_load_scenario_profile_rejects_missing_category(tmp_path: Path) -> None: mr.load_scenario_profile(path) +def test_load_scenario_profile_accepts_empty_category(tmp_path: Path) -> None: + path = tmp_path / "profile.yaml" + path.write_text( + """ +car: [151] +fcc: [301] +fmsr: [902] +health: [401] +tsfm: [] +wosr: [1] +""".strip(), + encoding="utf-8", + ) + + profile = mr.load_scenario_profile(path) + + assert profile["tsfm"] == () + + +def test_load_scenario_profile_rejects_empty_profile(tmp_path: Path) -> None: + path = tmp_path / "profile.yaml" + path.write_text( + """ +car: [] +fcc: [] +fmsr: [] +health: [] +tsfm: [] +wosr: [] +""".strip(), + encoding="utf-8", + ) + + with pytest.raises(ValueError, match="at least one scenario id"): + mr.load_scenario_profile(path) + + def test_resolve_scenario_ids_raises_for_missing_file_path(tmp_path: Path) -> None: with pytest.raises(FileNotFoundError, match="Scenario id file not found"): mr.resolve_scenario_ids(tmp_path / "missing.txt") diff --git a/src/evaluation/loader.py b/src/evaluation/loader.py index 04278e1a..c8fbf6dd 100644 --- a/src/evaluation/loader.py +++ b/src/evaluation/loader.py @@ -100,6 +100,12 @@ def _load_scenario_dir(path: Path) -> list[Scenario]: scenario_id = scenario_id.removeprefix("scenario_") expected_answer = groundtruth_path.read_text(encoding="utf-8").strip() + eval_metadata_path = child / "groundtruth_eval.json" + evaluation_metadata = None + if eval_metadata_path.exists(): + evaluation_metadata = json.loads( + eval_metadata_path.read_text(encoding="utf-8") + ) question_path = child / "question.txt" text = ( @@ -115,6 +121,7 @@ def _load_scenario_dir(path: Path) -> list[Scenario]: "text": text, "type": "structured", "expected_answer": expected_answer, + "evaluation_metadata": evaluation_metadata, "scoring_method": "static_json", } ) diff --git a/src/evaluation/models.py b/src/evaluation/models.py index 353619f2..7042f4c9 100644 --- a/src/evaluation/models.py +++ b/src/evaluation/models.py @@ -23,6 +23,7 @@ class Scenario(BaseModel): category: str = "" characteristic_form: str | None = None expected_answer: str | None = None + evaluation_metadata: dict[str, Any] | None = None scoring_method: str | None = None @classmethod diff --git a/src/evaluation/report.py b/src/evaluation/report.py index 6738cfff..7a80884b 100644 --- a/src/evaluation/report.py +++ b/src/evaluation/report.py @@ -57,6 +57,8 @@ def _aggregate_score_summary(results: list[ScenarioResult]) -> dict[str, Any]: "mode_key_match", "mode_exactly_one_key", "mode_term_coverage", + "mode_optional_term_coverage", + "car_score", ] score_values: dict[str, list[float]] = {name: [] for name in metric_names} @@ -127,6 +129,10 @@ def _aggregate_score_summary(results: list[ScenarioResult]) -> dict[str, Any]: "mode_key_match_avg": _avg(score_values["mode_key_match"]), "mode_exactly_one_key_avg": _avg(score_values["mode_exactly_one_key"]), "mode_term_coverage_avg": _avg(score_values["mode_term_coverage"]), + "mode_optional_term_coverage_avg": _avg( + score_values["mode_optional_term_coverage"] + ), + "car_score_avg": _avg(score_values["car_score"]), "missing_keys_total": missing_keys_total, "extra_keys_total": extra_keys_total, "detail_entries_total": detail_entries_total, diff --git a/src/evaluation/scorers/static_json.py b/src/evaluation/scorers/static_json.py index fb719dc7..5b80a970 100644 --- a/src/evaluation/scorers/static_json.py +++ b/src/evaluation/scorers/static_json.py @@ -78,6 +78,10 @@ class StaticJsonScore: mode_required_terms: list[str] = field(default_factory=list) mode_matched_terms: list[str] = field(default_factory=list) mode_term_coverage: float | None = None + mode_optional_terms: list[str] = field(default_factory=list) + mode_matched_optional_terms: list[str] = field(default_factory=list) + mode_optional_term_coverage: float | None = None + car_score: float | None = None def to_dict(self) -> dict[str, Any]: """Return a JSON-serializable dictionary.""" @@ -125,56 +129,76 @@ def _strip_markdown_fence(content: str) -> str: return content -def _extract_balanced_structure(content: str) -> str: - """Extract first balanced {...}, [...], or (...) from noisy text.""" +def _balanced_structure_candidates( + content: str, + open_chars: set[str], +) -> list[str]: + """Return balanced structured substrings from noisy text. + + Parentheses are handled separately from JSON-like braces/brackets because + natural-language answers often contain explanatory parentheticals before + the final JSON object. + """ content = content.strip() + close_for_open = {"{": "}", "[": "]", "(": ")"} + candidates: list[str] = [] - candidates = [ - (content.find("{"), "{", "}"), - (content.find("["), "[", "]"), - (content.find("("), "(", ")"), - ] - candidates = [ - (idx, open_ch, close_ch) - for idx, open_ch, close_ch in candidates - if idx != -1 - ] + for start, open_ch in enumerate(content): + if open_ch not in open_chars: + continue - if not candidates: - return content + close_ch = close_for_open[open_ch] + depth = 0 + in_string = False + quote_char = "" + escaped = False + + for index in range(start, len(content)): + ch = content[index] + + if in_string: + if escaped: + escaped = False + elif ch == "\\": + escaped = True + elif ch == quote_char: + in_string = False + continue - start, open_ch, close_ch = min(candidates, key=lambda item: item[0]) + if ch in {"'", '"'}: + in_string = True + quote_char = ch + continue - depth = 0 - in_string = False - quote_char = "" - escaped = False + if ch == open_ch: + depth += 1 + elif ch == close_ch: + depth -= 1 + if depth == 0: + candidates.append(content[start : index + 1].strip()) + break - for index in range(start, len(content)): - ch = content[index] + return candidates - if in_string: - if escaped: - escaped = False - elif ch == "\\": - escaped = True - elif ch == quote_char: - in_string = False - continue - if ch in {"'", '"'}: - in_string = True - quote_char = ch - continue +def _parse_structured_candidate(content: str) -> tuple[bool, Any]: + """Try supported structured parsers for a candidate answer.""" + try: + return True, json.loads(content) + except json.JSONDecodeError: + pass - if ch == open_ch: - depth += 1 - elif ch == close_ch: - depth -= 1 - if depth == 0: - return content[start : index + 1].strip() + try: + return True, ast.literal_eval(content) + except (ValueError, SyntaxError): + pass - return content[start:].strip() + try: + return True, json.loads(content.replace("'", '"')) + except json.JSONDecodeError: + pass + + return False, None def _extract_count_from_text(content: str) -> int | float | None: @@ -268,22 +292,19 @@ def parse_structured_answer(value: Any) -> Any: content = extract_answer_text(value) content = _strip_markdown_fence(content) - content = _extract_balanced_structure(content) + parsed, result = _parse_structured_candidate(content) + if parsed: + return result - try: - return json.loads(content) - except json.JSONDecodeError: - pass + for candidate in _balanced_structure_candidates(content, {"{", "["}): + parsed, result = _parse_structured_candidate(candidate) + if parsed: + return result - try: - return ast.literal_eval(content) - except (ValueError, SyntaxError): - pass - - try: - return json.loads(content.replace("'", '"')) - except json.JSONDecodeError: - pass + for candidate in _balanced_structure_candidates(content, {"("}): + parsed, result = _parse_structured_candidate(candidate) + if parsed: + return result count = _extract_count_from_text(content) if count is not None: @@ -494,12 +515,44 @@ def _is_mode_gold_answer(value: Any) -> bool: return key in _MODE_KEYS -def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: - gold = parse_structured_answer(gold_answer) - model = parse_structured_answer(model_answer) +def _as_terms(value: Any) -> list[str]: + if value is None: + return [] + if isinstance(value, str): + value = [value] + if not isinstance(value, list): + return [] + return _dedupe_terms([str(item) for item in value]) + +def _mode_metadata_from_gold(gold_answer: Any) -> dict[str, Any]: + gold = parse_structured_answer(gold_answer) gold_key = str(next(iter(gold))).strip().lower() gold_value = next(iter(gold.values())) + return { + "mode": gold_key, + "required_terms": _extract_required_mode_terms(gold_value), + "optional_terms": [], + "must_have_exactly_one_mode_key": True, + } + + +def _evaluate_mode_json( + gold_answer: Any, + model_answer: Any, + evaluation_metadata: dict[str, Any] | None = None, +) -> StaticJsonScore: + gold = parse_structured_answer(gold_answer) + model = parse_structured_answer(model_answer) + + metadata = _mode_metadata_from_gold(gold_answer) + if evaluation_metadata: + metadata.update(evaluation_metadata) + + gold_key = str(metadata.get("mode") or next(iter(gold))).strip().lower() + required_terms = _as_terms(metadata.get("required_terms")) + optional_terms = _as_terms(metadata.get("optional_terms")) + must_have_one_key = bool(metadata.get("must_have_exactly_one_mode_key", True)) model_is_dict = isinstance(model, dict) model_keys = [str(key).strip().lower() for key in model.keys()] if model_is_dict else [] @@ -508,7 +561,6 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: model_value = next(iter(model.values())) if model_is_dict and model_exactly_one_key else "" key_match = model_exactly_one_key and model_key == gold_key - required_terms = _extract_required_mode_terms(gold_value) model_text = _normalize_text_for_terms(model_value) matched_terms = [ term for term in required_terms if f" {term} " in f" {model_text} " @@ -516,6 +568,12 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: term_coverage = ( len(matched_terms) / len(required_terms) if required_terms else 1.0 ) + matched_optional_terms = [ + term for term in optional_terms if f" {term} " in f" {model_text} " + ] + optional_term_coverage = ( + len(matched_optional_terms) / len(optional_terms) if optional_terms else None + ) details = [ KeyComparison( @@ -543,13 +601,27 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: ) ) + for term in optional_terms: + matched = term in matched_optional_terms + details.append( + KeyComparison( + key=f"answer.optional_term.{term}", + gold_value=term, + model_value=term if matched else "MISSING", + exact=matched, + match_type="optional_term_present" if matched else "optional_term_missing", + similarity=1.0 if matched else 0.0, + accepted=matched, + ) + ) + missing_keys = [] if key_match else [f"answer.{gold_key}"] extra_keys = [] if model_is_dict: extra_keys = [ f"answer.{key}" for key in model_keys - if key not in {gold_key} or not model_exactly_one_key + if key not in {gold_key} or (must_have_one_key and not model_exactly_one_key) ] else: extra_keys = ["answer"] if model is not None else [] @@ -557,6 +629,10 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: total_gold_keys = 1 + len(required_terms) total_model_keys = 1 + len(required_terms) + len(extra_keys) exact_matches = (1 if key_match else 0) + len(matched_terms) + required_details = details[:total_gold_keys] + required_term_score = term_coverage + required_term_pass = bool(matched_terms) if required_terms else True + car_score = (0.6 * (1.0 if key_match else 0.0)) + (0.4 * required_term_score) precision = exact_matches / total_model_keys if total_model_keys else 0.0 recall = exact_matches / total_gold_keys if total_gold_keys else 0.0 @@ -565,13 +641,19 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: if precision + recall > 0 else 0.0 ) - strict_exact = 1.0 if key_match and term_coverage == 1.0 and not extra_keys else 0.0 + strict_exact = ( + 1.0 + if key_match + and required_term_pass + and (model_exactly_one_key or not must_have_one_key) + else 0.0 + ) return StaticJsonScore( partial_match_accuracy=recall, partial_exact_match_accuracy=recall, strict_exact_match_accuracy=strict_exact, - partial_similarity_score=sum(item.similarity for item in details) + partial_similarity_score=sum(item.similarity for item in required_details) / total_gold_keys, partial_numeric_match_accuracy=0.0, range_match_accuracy=0.0, @@ -598,6 +680,10 @@ def _evaluate_mode_json(gold_answer: Any, model_answer: Any) -> StaticJsonScore: mode_required_terms=required_terms, mode_matched_terms=matched_terms, mode_term_coverage=term_coverage, + mode_optional_terms=optional_terms, + mode_matched_optional_terms=matched_optional_terms, + mode_optional_term_coverage=optional_term_coverage, + car_score=car_score, ) @@ -784,8 +870,12 @@ def evaluate_static_json( model_answer: Any, *, similarity_threshold: float = 0.0, + evaluation_metadata: dict[str, Any] | None = None, ) -> StaticJsonScore: """Evaluate one structured gold answer against one model answer.""" + if evaluation_metadata and str(evaluation_metadata.get("mode", "")).lower() in _MODE_KEYS: + return _evaluate_mode_json(gold_answer, model_answer, evaluation_metadata) + if _is_mode_gold_answer(gold_answer): return _evaluate_mode_json(gold_answer, model_answer) @@ -1013,13 +1103,20 @@ def __call__( ), ) - static_score = evaluate_static_json(gold_answer, answer) + static_score = evaluate_static_json( + gold_answer, + answer, + evaluation_metadata=scenario.evaluation_metadata, + ) passed = static_score.strict_exact_match_accuracy == 1.0 + score_value = static_score.car_score + if score_value is None: + score_value = static_score.f1 return ScorerResult( scorer=self.name, passed=passed, - score=round(static_score.f1, 3), + score=round(score_value, 3), rationale=( "strict structured match" if passed diff --git a/src/evaluation/tests/test_evaluator.py b/src/evaluation/tests/test_evaluator.py index 66ebe822..17392e6b 100644 --- a/src/evaluation/tests/test_evaluator.py +++ b/src/evaluation/tests/test_evaluator.py @@ -5,6 +5,8 @@ import json from pathlib import Path +import pytest + from evaluation import scorers as registry from evaluation.evaluator import Evaluator from evaluation.models import Scenario, ScorerResult diff --git a/src/evaluation/tests/test_loader.py b/src/evaluation/tests/test_loader.py index e47b0741..b7841530 100644 --- a/src/evaluation/tests/test_loader.py +++ b/src/evaluation/tests/test_loader.py @@ -123,3 +123,25 @@ def test_load_scenarios_from_groundtruth_folders(tmp_path): assert scenarios[0].id == "11" assert scenarios[0].expected_answer == "{'energy': 14, 'material': 48}" assert scenarios[0].scoring_method == "static_json" + + +def test_load_scenarios_reads_groundtruth_eval_metadata(tmp_path): + scenario_dir = tmp_path / "scenario_151" + scenario_dir.mkdir() + (scenario_dir / "groundtruth.txt").write_text( + '{"clarification": "Which asset do you mean by the main unit?"}', + encoding="utf-8", + ) + (scenario_dir / "groundtruth_eval.json").write_text( + '{"mode": "clarification", "required_terms": ["main unit"]}', + encoding="utf-8", + ) + + scenarios = load_scenarios(tmp_path) + + assert len(scenarios) == 1 + assert scenarios[0].id == "151" + assert scenarios[0].evaluation_metadata == { + "mode": "clarification", + "required_terms": ["main unit"], + } diff --git a/src/evaluation/tests/test_static_json_scorer.py b/src/evaluation/tests/test_static_json_scorer.py index c5de0049..deef8a97 100644 --- a/src/evaluation/tests/test_static_json_scorer.py +++ b/src/evaluation/tests/test_static_json_scorer.py @@ -24,6 +24,29 @@ def test_parse_fenced_json_response_key_from_noisy_answer(): } +def test_parse_json_after_parenthetical_prose(): + raw = ( + 'The registry has generic descriptions (all descriptions are "Asset PMPxxxxx").\n\n' + '{"clarification": "Which pump do you mean by the big pump out the back?"}' + ) + + assert parse_structured_answer(raw) == { + "clarification": "Which pump do you mean by the big pump out the back?" + } + + +def test_parse_json_after_parenthetical_with_number(): + raw = ( + 'Based on the repair history (asset PMP42144 has 17 work orders), ' + 'the answer is:\n' + '{"clarification": "Which asset do you mean by the main unit?"}' + ) + + assert parse_structured_answer(raw) == { + "clarification": "Which asset do you mean by the main unit?" + } + + def test_parse_python_style_dict(): raw = "{'energy': 14, 'material': 48}" @@ -327,6 +350,97 @@ def test_mode_requires_exactly_one_top_level_key(): assert score.extra_keys == ["answer.clarification", "answer.response"] +def test_car_metadata_passes_on_mode_and_required_terms_only(): + score = evaluate_static_json( + {"clarification": "Which asset do you mean by 'the main unit'?"}, + {"clarification": "Which unit are you referring to as the main unit?"}, + evaluation_metadata={ + "mode": "clarification", + "required_terms": ["main unit"], + "optional_terms": ["asset"], + }, + ) + + assert score.strict_exact_match_accuracy == 1.0 + assert score.car_score == 1.0 + assert score.mode_key_match == 1.0 + assert score.mode_required_terms == ["main unit"] + assert score.mode_matched_terms == ["main unit"] + assert score.mode_optional_terms == ["asset"] + assert score.mode_matched_optional_terms == [] + assert score.mode_optional_term_coverage == 0.0 + + +def test_car_optional_terms_do_not_inflate_similarity(): + score = evaluate_static_json( + {"clarification": "Which asset do you mean by 'the main unit'?"}, + { + "clarification": ( + "Which asset do you mean by the main unit? Please provide the " + "asset tag." + ) + }, + evaluation_metadata={ + "mode": "clarification", + "required_terms": ["main unit"], + "optional_terms": ["asset", "asset tag"], + }, + ) + + assert score.strict_exact_match_accuracy == 1.0 + assert score.partial_similarity_score == 1.0 + assert score.mode_optional_term_coverage == 1.0 + + +def test_car_metadata_passes_when_any_required_term_matches(): + score = evaluate_static_json( + {"abstain": "Cannot determine because date or time is missing."}, + {"abstain": "Cannot determine this from the available date fields."}, + evaluation_metadata={ + "mode": "abstain", + "required_terms": ["lately", "date", "time"], + }, + ) + + assert score.strict_exact_match_accuracy == 1.0 + assert score.car_score == 0.7333333333333333 + assert score.mode_key_match == 1.0 + assert score.mode_matched_terms == ["date"] + assert score.mode_term_coverage == 1 / 3 + + +def test_car_metadata_fails_when_required_term_is_missing(): + score = evaluate_static_json( + {"clarification": "Which asset do you mean by 'the main unit'?"}, + {"clarification": "Which asset should I analyze?"}, + evaluation_metadata={ + "mode": "clarification", + "required_terms": ["main unit"], + }, + ) + + assert score.strict_exact_match_accuracy == 0.0 + assert score.car_score == 0.6 + assert score.mode_key_match == 1.0 + assert score.mode_term_coverage == 0.0 + + +def test_car_metadata_fails_when_mode_key_is_wrong_even_with_required_term(): + score = evaluate_static_json( + {"clarification": "Which asset do you mean by 'the main unit'?"}, + {"response": "The main unit appears to be PMP42144."}, + evaluation_metadata={ + "mode": "clarification", + "required_terms": ["main unit"], + }, + ) + + assert score.strict_exact_match_accuracy == 0.0 + assert score.car_score == 0.4 + assert score.mode_key_match == 0.0 + assert score.mode_term_coverage == 1.0 + + def test_count_only_exact_match(): score = evaluate_static_json("34", "The answer is 34.") @@ -372,3 +486,29 @@ def test_static_json_scorer_wrapper_exact_match(): assert result.passed is True assert result.score == 1.0 assert result.details["strict_exact_match_accuracy"] == 1.0 + + +def test_static_json_scorer_uses_car_metadata_score(): + scenario = Scenario.from_raw( + { + "id": "151", + "text": "Clarify main unit.", + "expected_answer": '{"clarification": "Which asset do you mean by the main unit?"}', + "evaluation_metadata": { + "mode": "clarification", + "required_terms": ["main unit"], + }, + "scoring_method": "static_json", + } + ) + + scorer = StaticJsonScorer() + result = scorer( + scenario, + '{"clarification": "Which asset is the main unit?"}', + "", + ) + + assert result.passed is True + assert result.score == 1.0 + assert result.details["car_score"] == 1.0 diff --git a/src/observability/persistence.py b/src/observability/persistence.py index 49f7443e..a47825a1 100644 --- a/src/observability/persistence.py +++ b/src/observability/persistence.py @@ -77,7 +77,6 @@ def persist_trajectory( "answer": answer, "trajectory": _serialize_trajectory(trajectory), } - try: out_path.write_text(json.dumps(record, indent=2, default=str), encoding="utf-8") except OSError: