Skip to content

Commit c433274

Browse files
Trecekclaude
andcommitted
[FIX] load_and_validate fail-open on steps-less YAML recipes
Restructure load_and_validate so validity is derived unconditionally from the validation pipeline output (matching the fail-closed pattern used by validate_from_path). Previously, valid was initialized to True and the validation pipeline was gated behind a "steps" in data check, causing steps-less YAML recipes to bypass all validation and be served as valid. Changes: - _api.py: change valid = True to valid = False (fail-closed default) - _api.py: remove the "steps" in data guard so the validation pipeline runs unconditionally on any successfully-parsed dict - _api.py: remove the dead else branch (now unreachable) - io.py: add type guard in _parse_recipe for non-dict steps values (converts AttributeError into clean ValueError) - tools_kitchen.py: get_recipe MCP resource returns error JSON for invalid recipes instead of raw content - tools_recipe.py: load_recipe tool includes validation_failed flag when the recipe fails validation - tests: add 9 regression tests covering steps-less YAML, cached results, sentinel-based structural guarantee, empty steps, non-dict root, non-dict steps value, campaign recipes, and downstream server-side guards Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ade4afb commit c433274

7 files changed

Lines changed: 300 additions & 5 deletions

File tree

src/autoskillit/recipe/_api.py

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ def load_and_validate(
312312
raw = substitute_temp_placeholder(raw, _temp_relpath)
313313
raw = substitute_scripts_placeholder(raw)
314314
suggestions: list[dict[str, Any]] = []
315-
valid = True
315+
valid = False
316316
errors: list[str] = []
317317
recipe = None
318318
active_recipe = None
@@ -330,7 +330,7 @@ def load_and_validate(
330330
data = _load_recipe_dict(match.path, raw_text=raw, temp_dir_relpath=_temp_relpath)
331331
t0 = _t("yaml_parse", t0, name)
332332

333-
if isinstance(data, dict) and "steps" in data:
333+
if isinstance(data, dict):
334334
recipe = _parse_recipe(data)
335335

336336
from autoskillit.recipe.identity import compute_composite_hash # noqa: PLC0415
@@ -469,8 +469,6 @@ def load_and_validate(
469469
t0 = _t("diagram", t0, name)
470470

471471
valid = compute_recipe_validity(errors, semantic_findings, contract_findings)
472-
else:
473-
t0 = _t("yaml_parse", t0, name)
474472

475473
except YAMLError as exc:
476474
logger.warning("Recipe YAML parse error", name=name, exc_info=True)

src/autoskillit/recipe/io.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,10 @@ def _parse_recipe(data: dict[str, Any]) -> Recipe:
502502
)
503503

504504
steps: dict[str, RecipeStep] = {}
505-
for step_name, step_data in (data.get("steps") or {}).items():
505+
_steps_raw = data.get("steps")
506+
if _steps_raw is not None and not isinstance(_steps_raw, dict):
507+
raise ValueError(f"'steps' must be a mapping, got {type(_steps_raw).__name__!r}")
508+
for step_name, step_data in (_steps_raw or {}).items():
506509
if isinstance(step_data, dict):
507510
step = _parse_step(step_data)
508511
step.name = step_name

src/autoskillit/server/tools/tools_kitchen.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,15 @@ def get_recipe(name: str) -> str:
418418
except Exception:
419419
logger.warning("get_recipe_failure", recipe=name, stage="load_and_validate", exc_info=True)
420420
return json.dumps({"error": f"Recipe '{name}' composition failed."})
421+
if not result.get("valid", False):
422+
logger.warning("get_recipe_invalid", recipe=name, errors=result.get("errors", []))
423+
return json.dumps(
424+
{
425+
"error": f"Recipe '{name}' failed validation.",
426+
"errors": result.get("errors", []),
427+
"suggestions": result.get("suggestions", []),
428+
}
429+
)
421430
return result.get("content", json.dumps({"error": "Recipe composition failed."}))
422431

423432

src/autoskillit/server/tools/tools_recipe.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,8 @@ async def load_recipe(
234234
)
235235
recipe_info = tool_ctx.recipes.find(name, tool_ctx.project_dir)
236236
result = await _apply_triage_gate(result, name, recipe_info=recipe_info)
237+
if not result.get("valid", False):
238+
result["validation_failed"] = True
237239
if ingredients_only:
238240
result = strip_ingredients_only_keys(result)
239241
return json.dumps(result)

tests/recipe/test_api.py

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,3 +1262,211 @@ def test_validate_from_path_codex_backend_valid_after_pruning(tmp_path: Path) ->
12621262
if s.get("severity") == Severity.ERROR
12631263
)
12641264
)
1265+
1266+
1267+
# ---------------------------------------------------------------------------
1268+
# load_and_validate fail-closed tests (regression for steps-less YAML bug)
1269+
# ---------------------------------------------------------------------------
1270+
1271+
1272+
_RECIPE_NO_STEPS = """\
1273+
name: test-no-steps
1274+
description: A test recipe with no steps key
1275+
autoskillit_version: "0.3.0"
1276+
kitchen_rules:
1277+
- "Test rule"
1278+
"""
1279+
1280+
_RECIPE_EMPTY_STEPS = """\
1281+
name: test-empty-steps
1282+
description: A test recipe with empty steps
1283+
autoskillit_version: "0.3.0"
1284+
kitchen_rules:
1285+
- "Test rule"
1286+
steps: {}
1287+
"""
1288+
1289+
_RECIPE_NON_DICT_STEPS = """\
1290+
name: test-list-steps
1291+
description: A test recipe with steps as a list
1292+
autoskillit_version: "0.3.0"
1293+
kitchen_rules:
1294+
- "Test rule"
1295+
steps:
1296+
- foo
1297+
- bar
1298+
"""
1299+
1300+
_RECIPE_NON_DICT_ROOT = """\
1301+
- item1
1302+
- item2
1303+
"""
1304+
1305+
_RECIPE_CAMPAIGN_NO_STEPS = """\
1306+
name: test-campaign-no-steps
1307+
description: A campaign recipe with dispatches and no steps
1308+
autoskillit_version: "0.3.0"
1309+
kind: campaign
1310+
kitchen_rules:
1311+
- "Campaign rule"
1312+
dispatches:
1313+
- name: dispatch1
1314+
recipe: some-recipe
1315+
gate: some_gate
1316+
"""
1317+
1318+
1319+
# 1a: steps-less YAML returns valid=False
1320+
def test_load_and_validate_steps_less_yaml_returns_invalid(tmp_path: Path) -> None:
1321+
"""A YAML recipe without a 'steps' key must produce valid=False."""
1322+
import autoskillit.recipe._api_cache as cache_mod
1323+
1324+
cache_mod._LOAD_CACHE.clear()
1325+
_setup_project_recipe(tmp_path, "test-no-steps", _RECIPE_NO_STEPS)
1326+
1327+
from autoskillit.recipe._api import load_and_validate
1328+
1329+
result = load_and_validate("test-no-steps", project_dir=tmp_path)
1330+
1331+
assert result["valid"] is False, (
1332+
f"Steps-less YAML must return valid=False; got valid={result.get('valid')}. "
1333+
f"Errors: {result.get('errors', [])}"
1334+
)
1335+
assert len(result["errors"]) > 0, "Expected at least one error for steps-less YAML"
1336+
assert any("step" in e.lower() for e in result["errors"]), (
1337+
f"Expected an error mentioning steps; got: {result['errors']}"
1338+
)
1339+
assert any(s.get("severity") == Severity.ERROR for s in result.get("suggestions", [])), (
1340+
"Expected at least one ERROR severity suggestion"
1341+
)
1342+
1343+
1344+
# 1b: cached result also returns valid=False
1345+
def test_load_and_validate_steps_less_yaml_not_cached_as_valid(tmp_path: Path) -> None:
1346+
"""After calling load_and_validate with steps-less YAML, the cache must not serve valid=True.""" # noqa: E501
1347+
import autoskillit.recipe._api_cache as cache_mod
1348+
1349+
cache_mod._LOAD_CACHE.clear()
1350+
_setup_project_recipe(tmp_path, "test-no-steps", _RECIPE_NO_STEPS)
1351+
1352+
from autoskillit.recipe._api import load_and_validate
1353+
1354+
result1 = load_and_validate("test-no-steps", project_dir=tmp_path)
1355+
result2 = load_and_validate("test-no-steps", project_dir=tmp_path)
1356+
1357+
assert result1["valid"] is False
1358+
assert result2["valid"] is False, (
1359+
f"Cached result must also be valid=False; got valid={result2.get('valid')}"
1360+
)
1361+
1362+
1363+
# 1c: compute_recipe_validity is always the source of valid
1364+
def test_load_and_validate_always_invokes_compute_recipe_validity(
1365+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
1366+
) -> None:
1367+
"""Monkey-patch compute_recipe_validity with a sentinel and verify it flows through
1368+
for BOTH with-steps and without-steps recipes. This structurally guarantees that
1369+
no code path through load_and_validate can return a valid value that didn't come
1370+
from compute_recipe_validity.
1371+
"""
1372+
import autoskillit.recipe._api as api_mod
1373+
import autoskillit.recipe._api_cache as cache_mod
1374+
1375+
cache_mod._LOAD_CACHE.clear()
1376+
1377+
_setup_project_recipe(tmp_path, "test-no-steps", _RECIPE_NO_STEPS)
1378+
_setup_project_recipe(tmp_path, "test-recipe-with-rules", _RECIPE_WITH_RULES)
1379+
1380+
sentinel_valid = object()
1381+
monkeypatch.setattr(api_mod, "compute_recipe_validity", lambda *a, **kw: sentinel_valid)
1382+
1383+
from autoskillit.recipe._api import load_and_validate
1384+
1385+
result_no_steps = load_and_validate("test-no-steps", project_dir=tmp_path)
1386+
assert result_no_steps["valid"] is sentinel_valid, (
1387+
f"Steps-less recipe must get valid from compute_recipe_validity sentinel; "
1388+
f"got valid={result_no_steps.get('valid')!r}"
1389+
)
1390+
1391+
cache_mod._LOAD_CACHE.clear()
1392+
result_with_steps = load_and_validate("test-recipe-with-rules", project_dir=tmp_path)
1393+
assert result_with_steps["valid"] is sentinel_valid, (
1394+
f"With-steps recipe must get valid from compute_recipe_validity sentinel; "
1395+
f"got valid={result_with_steps.get('valid')!r}"
1396+
)
1397+
1398+
1399+
# 1d: empty-steps recipe returns valid=False
1400+
def test_load_and_validate_empty_steps_returns_invalid(tmp_path: Path) -> None:
1401+
"""A YAML with steps: {} (present but empty) must produce valid=False."""
1402+
import autoskillit.recipe._api_cache as cache_mod
1403+
1404+
cache_mod._LOAD_CACHE.clear()
1405+
_setup_project_recipe(tmp_path, "test-empty-steps", _RECIPE_EMPTY_STEPS)
1406+
1407+
from autoskillit.recipe._api import load_and_validate
1408+
1409+
result = load_and_validate("test-empty-steps", project_dir=tmp_path)
1410+
1411+
assert result["valid"] is False, (
1412+
f"Empty-steps YAML must return valid=False; got valid={result.get('valid')}. "
1413+
f"Errors: {result.get('errors', [])}"
1414+
)
1415+
assert any("step" in e.lower() for e in result["errors"]), (
1416+
f"Expected error about steps; got: {result['errors']}"
1417+
)
1418+
1419+
1420+
# 1e: non-dict YAML returns valid=False
1421+
def test_load_and_validate_non_dict_root_returns_invalid(tmp_path: Path) -> None:
1422+
"""A YAML that parses to a list (non-dict root) must produce valid=False."""
1423+
import autoskillit.recipe._api_cache as cache_mod
1424+
1425+
cache_mod._LOAD_CACHE.clear()
1426+
_setup_project_recipe(tmp_path, "test-list-root", _RECIPE_NON_DICT_ROOT)
1427+
1428+
from autoskillit.recipe._api import load_and_validate
1429+
1430+
result = load_and_validate("test-list-root", project_dir=tmp_path)
1431+
1432+
assert result["valid"] is False, (
1433+
f"Non-dict root YAML must return valid=False; got valid={result.get('valid')}"
1434+
)
1435+
1436+
1437+
# 1f: non-dict steps value returns valid=False
1438+
def test_load_and_validate_non_dict_steps_value_returns_invalid(tmp_path: Path) -> None:
1439+
"""A YAML with steps: [foo, bar] (list instead of mapping) must produce valid=False
1440+
with a clear error, not an uncaught AttributeError.
1441+
"""
1442+
import autoskillit.recipe._api_cache as cache_mod
1443+
1444+
cache_mod._LOAD_CACHE.clear()
1445+
_setup_project_recipe(tmp_path, "test-list-steps", _RECIPE_NON_DICT_STEPS)
1446+
1447+
from autoskillit.recipe._api import load_and_validate
1448+
1449+
result = load_and_validate("test-list-steps", project_dir=tmp_path)
1450+
1451+
assert result["valid"] is False, (
1452+
f"Non-dict steps value must return valid=False; got valid={result.get('valid')}. "
1453+
f"Suggestions: {result.get('suggestions', [])}"
1454+
)
1455+
1456+
1457+
# 1g: campaign recipe with dispatches and no steps validates correctly
1458+
def test_load_and_validate_campaign_no_steps_returns_valid(tmp_path: Path) -> None:
1459+
"""A campaign recipe (kind=campaign) with dispatches and no steps must return valid=True."""
1460+
import autoskillit.recipe._api_cache as cache_mod
1461+
1462+
cache_mod._LOAD_CACHE.clear()
1463+
_setup_project_recipe(tmp_path, "test-campaign-no-steps", _RECIPE_CAMPAIGN_NO_STEPS)
1464+
1465+
from autoskillit.recipe._api import load_and_validate
1466+
1467+
result = load_and_validate("test-campaign-no-steps", project_dir=tmp_path)
1468+
1469+
assert result["valid"] is True, (
1470+
f"Campaign recipe with dispatches and no steps must be valid=True; "
1471+
f"got valid={result.get('valid')}. Errors: {result.get('errors', [])}"
1472+
)

tests/server/test_tools_kitchen_gate_features.py

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,3 +417,41 @@ def test_recipe_resource_returns_composed_content():
417417
assert result == ("name: test-recipe\nsteps:\n stop:\n action: stop\n message: done\n")
418418
assert "optional: true" not in result
419419
assert "skip_when_false:" not in result
420+
421+
422+
# 1h: get_recipe resource returns error for invalid recipe
423+
def test_recipe_resource_returns_error_for_invalid_recipe():
424+
"""When load_and_validate returns valid=False, get_recipe must return an error
425+
JSON instead of raw content.
426+
"""
427+
mock_ctx = _make_mock_ctx()
428+
mock_ctx.recipes = MagicMock()
429+
mock_ctx.recipes.find.return_value = MagicMock()
430+
mock_ctx.recipes.load_and_validate.return_value = {
431+
"content": "name: bad-recipe\ndescription: missing steps\n",
432+
"valid": False,
433+
"errors": ["Recipe must have at least one step."],
434+
"suggestions": [],
435+
}
436+
mock_ctx.backend = None
437+
438+
with (
439+
patch("autoskillit.server._state._get_ctx_or_none", return_value=mock_ctx),
440+
patch(
441+
"autoskillit.server.tools.tools_kitchen.resolve_ingredient_defaults",
442+
return_value={},
443+
),
444+
patch(
445+
"autoskillit.server.tools.tools_kitchen.build_config_authoritative_layer",
446+
return_value={},
447+
),
448+
):
449+
from autoskillit.server.tools.tools_kitchen import get_recipe
450+
451+
result = get_recipe("bad-recipe")
452+
453+
parsed = json.loads(result)
454+
assert "error" in parsed, f"Expected error JSON for invalid recipe; got: {result}"
455+
assert "validation" in parsed["error"].lower() or "invalid" in parsed["error"].lower(), (
456+
f"Error should mention validation/invalid; got: {parsed['error']}"
457+
)

tests/server/test_tools_load_recipe.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,3 +543,40 @@ async def test_load_recipe_ingredients_only_strips_content(self, tmp_path, monke
543543
assert "orchestration_rules" not in result
544544
assert "stop_step_semantics" not in result
545545
assert "suggestions" in result
546+
547+
548+
# 1i: load_recipe tool surfaces validation failure
549+
class TestLoadRecipeSurfacesValidationFailure:
550+
"""When load_and_validate returns valid=False, the load_recipe tool must include
551+
a field indicating the recipe failed validation.
552+
"""
553+
554+
@pytest.fixture(autouse=True)
555+
def _ensure_ctx(self, tool_ctx_kitchen_open):
556+
"""Ensure server context is initialized with gate open."""
557+
558+
@pytest.mark.anyio
559+
async def test_load_recipe_surfaces_validation_failure(
560+
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
561+
) -> None:
562+
"""When load_and_validate returns valid=False, the response must include
563+
a validation_failed indicator so callers know the recipe is invalid.
564+
"""
565+
from autoskillit.recipe._api_cache import _LOAD_CACHE
566+
567+
monkeypatch.chdir(tmp_path)
568+
recipes_dir = tmp_path / ".autoskillit" / "recipes"
569+
recipes_dir.mkdir(parents=True)
570+
(recipes_dir / "no-steps.yaml").write_text(
571+
"name: no-steps\ndescription: Missing steps\nkitchen_rules:\n - 'rule'\n"
572+
)
573+
574+
_LOAD_CACHE.clear()
575+
576+
result = json.loads(await load_recipe(name="no-steps"))
577+
assert result.get("valid") is False
578+
assert result.get("validation_failed") is True, (
579+
f"Expected validation_failed=True in response; got keys: {list(result.keys())}"
580+
)
581+
assert "errors" in result
582+
assert len(result["errors"]) > 0

0 commit comments

Comments
 (0)