Rectify: Recipe Validation Envelope Hides Semantic Errors Behind "unknown structural error"

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,

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 |

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"]

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

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