From 42170a06bcd3a45b1b352ddbb41b2d314f28f441 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 11 Jun 2026 07:13:18 -0700 Subject: [PATCH 1/2] Rectify: Surface LoadRecipeResult Error Channels in Validation Envelope (#4060) Fix _recipe_validation_error_response to merge error-severity suggestions from result["suggestions"] into the user-visible message, preventing the "unknown structural error" fallback when valid=False is driven by semantic or contract findings (not structural errors). Changes: - tools_kitchen.py: merge errors + error-severity findings into error_parts; use generic "validation" label when semantic findings are present - tests/contracts/test_validation_error_envelope_coverage.py: contract test asserting both channels are surfaced in the envelope - tests/server/test_tools_kitchen_envelope.py: direct unit test, parametrized invariant, end-to-end open_kitchen test, and malformed-suggestion resilience - tests/contracts/AGENTS.md: register new contract test (REQ-ENVELOPE-001) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/autoskillit/server/tools/tools_kitchen.py | 17 ++- tests/contracts/AGENTS.md | 1 + ...test_validation_error_envelope_coverage.py | 35 +++++ tests/server/test_tools_kitchen_envelope.py | 131 ++++++++++++++++++ 4 files changed, 180 insertions(+), 4 deletions(-) create mode 100644 tests/contracts/test_validation_error_envelope_coverage.py diff --git a/src/autoskillit/server/tools/tools_kitchen.py b/src/autoskillit/server/tools/tools_kitchen.py index 8eb804a922..424d34276d 100644 --- a/src/autoskillit/server/tools/tools_kitchen.py +++ b/src/autoskillit/server/tools/tools_kitchen.py @@ -99,14 +99,23 @@ def _kitchen_failure_envelope( def _recipe_validation_error_response(name: str, result: dict[str, Any]) -> str: _errs: list[str] = result.get("errors", []) - _error_detail = "; ".join(_errs[:3]) if _errs else "unknown structural error" + _error_findings = [ + s + for s in result.get("suggestions", []) + if isinstance(s, dict) and s.get("severity") == "error" + ] + _finding_strs = [f"[{f.get('rule', '')}] {f.get('message', '')}" for f in _error_findings] + if _errs and _finding_strs: + _error_parts = _errs[:2] + _finding_strs[:1] + else: + _error_parts = (_errs + _finding_strs)[:3] + _error_detail = "; ".join(_error_parts) if _error_parts else "unknown structural error" + _label = "structural validation" if (_errs and not _error_findings) else "validation" return json.dumps( { "success": False, "kitchen": "failed", - "user_visible_message": ( - f"Recipe '{name}' failed structural validation: {_error_detail}" - ), + "user_visible_message": (f"Recipe '{name}' failed {_label}: {_error_detail}"), "error": f"Recipe '{name}' failed validation: {_error_detail}", "stage": "recipe_validation", "errors": _errs, diff --git a/tests/contracts/AGENTS.md b/tests/contracts/AGENTS.md index 374ff249aa..5ff65e8ace 100644 --- a/tests/contracts/AGENTS.md +++ b/tests/contracts/AGENTS.md @@ -39,6 +39,7 @@ Protocol satisfaction, package gateway, and skill contract compliance tests. | `test_github_ops.py` | Contract tests: GitHub operation semantics in SKILL.md files | | `test_hook_bridge_coverage.py` | REQ-BRIDGE-001: quota guard hook config bridge must produce exactly the keys that resolve_quota_settings() reads | | `test_implement_experiment_contracts.py` | Contract tests for implement-experiment SKILL.md — test infrastructure requirements | +| `test_validation_error_envelope_coverage.py` | REQ-ENVELOPE-001: validation error envelope must surface all error channels from compute_recipe_validity | | `test_input_type_semantic_correctness.py` | Cross-validate skill_contracts.yaml path input types against SKILL.md content | | `test_instruction_surface.py` | Contract tests: every instruction surface must carry the pipeline tool restriction | | `test_issue_body_discipline.py` | Cross-skill contract: no SKILL.md may append validation summaries to issue bodies | diff --git a/tests/contracts/test_validation_error_envelope_coverage.py b/tests/contracts/test_validation_error_envelope_coverage.py new file mode 100644 index 0000000000..531f7ef91d --- /dev/null +++ b/tests/contracts/test_validation_error_envelope_coverage.py @@ -0,0 +1,35 @@ +"""REQ-ENVELOPE-001: _recipe_validation_error_response must surface all error channels +from compute_recipe_validity (structural errors + semantic/contract findings). + +Mirrors the test_hook_bridge_coverage pattern: when a new error channel is added to +LoadRecipeResult, this test forces the envelope to surface it. +""" + +from __future__ import annotations + +import json + +import pytest + +pytestmark = [pytest.mark.layer("contracts"), pytest.mark.small] + + +def test_envelope_surfaces_both_structural_and_semantic_channels(): + """Envelope must reference at least one item from errors AND from suggestions.""" + from autoskillit.server.tools.tools_kitchen import _recipe_validation_error_response + + result = { + "valid": False, + "errors": ["structural problem"], + "suggestions": [ + { + "severity": "error", + "rule": "semantic-rule", + "message": "semantic problem", + "step": "s", + }, + ], + } + response = json.loads(_recipe_validation_error_response("demo", result)) + assert "structural problem" in response["user_visible_message"] + assert "semantic-rule" in response["user_visible_message"] diff --git a/tests/server/test_tools_kitchen_envelope.py b/tests/server/test_tools_kitchen_envelope.py index 5db4d602d4..95326c5393 100644 --- a/tests/server/test_tools_kitchen_envelope.py +++ b/tests/server/test_tools_kitchen_envelope.py @@ -883,3 +883,134 @@ def test_every_return_path_parses_as_json_and_has_boolean_success(stage): envelope = json.loads(_kitchen_failure_envelope(RuntimeError("test"), stage=stage)) assert isinstance(envelope["success"], bool) + + +def test_recipe_validation_error_response_surfaces_semantic_errors(): + """_recipe_validation_error_response must merge error-severity suggestions + into the user-visible message when errors is empty.""" + from autoskillit.server.tools.tools_kitchen import _recipe_validation_error_response + + result = { + "valid": False, + "errors": [], + "suggestions": [ + { + "severity": "error", + "rule": "backend-incompatible-skill", + "message": "Step 'deploy' uses skill 'merge_worktree'", + "step": "deploy", + }, + ], + } + response = json.loads(_recipe_validation_error_response("demo", result)) + assert response["success"] is False + assert response["kitchen"] == "failed" + assert response["stage"] == "recipe_validation" + assert "unknown structural error" not in response["user_visible_message"] + assert "backend-incompatible-skill" in response["user_visible_message"] + assert "merge_worktree" in response["error"] + + +@pytest.mark.parametrize( + "result,expected_substring", + [ + ( + {"valid": False, "errors": ["schema violation"], "suggestions": []}, + "schema violation", + ), + ( + { + "valid": False, + "errors": [], + "suggestions": [ + { + "severity": "error", + "rule": "rule-x", + "message": "bad step", + "step": "s", + } + ], + }, + "rule-x", + ), + ( + { + "valid": False, + "errors": [], + "suggestions": [ + { + "severity": "error", + "rule": "contract-y", + "message": "stale", + "step": "c", + } + ], + }, + "contract-y", + ), + ], +) +def test_validation_error_envelope_always_names_cause(result, expected_substring): + """Envelope must always name a cause when valid=False, never 'unknown structural error'.""" + from autoskillit.server.tools.tools_kitchen import _recipe_validation_error_response + + response = json.loads(_recipe_validation_error_response("demo", result)) + assert response["success"] is False + assert response["kitchen"] == "failed" + assert response["stage"] == "recipe_validation" + assert "unknown structural error" not in response["user_visible_message"] + assert expected_substring in response["user_visible_message"] + + +@pytest.mark.anyio +async def test_open_kitchen_fails_on_semantic_errors_only(tmp_path, monkeypatch): + """open_kitchen must surface semantic error findings when errors=[].""" + monkeypatch.chdir(tmp_path) + mock_ctx = _make_mock_ctx() + mock_ctx.enable_components = AsyncMock() + mock_ctx.recipes = MagicMock() + mock_ctx.recipes.load_and_validate.return_value = { + "content": "name: demo\nsteps:\n do:\n tool: run_cmd\n", + "valid": False, + "errors": [], + "suggestions": [ + { + "severity": "error", + "rule": "backend-incompatible-skill", + "message": "Step 'deploy' uses skill 'merge_worktree'", + "step": "deploy", + }, + ], + } + mock_ctx.config.migration.suppressed = [] + + with patch("autoskillit.server._get_ctx", return_value=mock_ctx): + with patch("autoskillit.server.logger"): + with patch( + "autoskillit.server.tools.tools_kitchen._prime_quota_cache", new=AsyncMock() + ): + with patch("autoskillit.server.tools.tools_kitchen._write_hook_config"): + from autoskillit.server.tools.tools_kitchen import open_kitchen + + result_str = await open_kitchen(name="demo", ctx=mock_ctx) + + parsed = json.loads(result_str) + assert parsed["success"] is False + assert "unknown structural error" not in parsed["user_visible_message"] + assert "backend-incompatible-skill" in parsed["user_visible_message"] + + +def test_recipe_validation_error_response_handles_malformed_suggestions(): + """_recipe_validation_error_response must not crash on suggestions missing rule/message.""" + from autoskillit.server.tools.tools_kitchen import _recipe_validation_error_response + + result = { + "valid": False, + "errors": [], + "suggestions": [ + {"severity": "error", "step": "some-step"}, + ], + } + response = json.loads(_recipe_validation_error_response("demo", result)) + assert "unknown structural error" not in response["user_visible_message"] + assert response["success"] is False From dd0bdc7dc088bcf8f8503ba5b225079e9bf8e579 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 11 Jun 2026 07:24:44 -0700 Subject: [PATCH 2/2] fix: update _LEGACY_JSON_WRITES allowlist line numbers for tools_kitchen.py The prior commit shifted 5 json.dumps call sites by +9 lines. Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/infra/test_schema_version_convention.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/infra/test_schema_version_convention.py b/tests/infra/test_schema_version_convention.py index 15b425d58f..fc5aeecb7e 100644 --- a/tests/infra/test_schema_version_convention.py +++ b/tests/infra/test_schema_version_convention.py @@ -119,11 +119,11 @@ def _is_yaml_dump(node: ast.expr) -> bool: # _lifespan.py — hooks.json self-heal on startup drift (co-owned with Claude plugin system) ("src/autoskillit/server/_lifespan.py", 87), # tools_kitchen.py — hook config, quota guard, git_ops_policy, ingredient locks overlay - ("src/autoskillit/server/tools/tools_kitchen.py", 160), - ("src/autoskillit/server/tools/tools_kitchen.py", 179), - ("src/autoskillit/server/tools/tools_kitchen.py", 213), - ("src/autoskillit/server/tools/tools_kitchen.py", 863), - ("src/autoskillit/server/tools/tools_kitchen.py", 923), + ("src/autoskillit/server/tools/tools_kitchen.py", 169), + ("src/autoskillit/server/tools/tools_kitchen.py", 188), + ("src/autoskillit/server/tools/tools_kitchen.py", 222), + ("src/autoskillit/server/tools/tools_kitchen.py", 872), + ("src/autoskillit/server/tools/tools_kitchen.py", 932), # tools_pipeline_tracker.py — tracker_data dict ("src/autoskillit/server/tools/tools_pipeline_tracker.py", 166), # tools_status.py — mcp_data dict