diff --git a/.autoskillit/recipes/eval/agent-eval.yaml b/.autoskillit/recipes/eval/agent-eval.yaml index dd5e621223..97f87df3f1 100644 --- a/.autoskillit/recipes/eval/agent-eval.yaml +++ b/.autoskillit/recipes/eval/agent-eval.yaml @@ -78,9 +78,6 @@ steps: output_dir: "${{ inputs.output_dir }}" capture: eval_run_dir: "${{ result.eval_run_dir }}" - canary_count: "${{ result.canary_count }}" - variant_count: "${{ result.variant_count }}" - manifest_index_path: "${{ result.manifest_index_path }}" on_success: run_canaries on_failure: escalate note: >- @@ -100,7 +97,7 @@ steps: Instead, follow these instructions using MCP tool calls directly: - 1. Read the manifest index: run_cmd cat {context.manifest_index_path} + 1. Read the manifest index: run_cmd cat {context.eval_run_dir}/manifest_index.json Parse the JSON to get canary_ids, variant_ids, and directory paths. @@ -175,4 +172,5 @@ steps: (${{ context.passed_runs }}/${{ context.total_runs }}). Vacuous passes: ${{ context.vacuous_passes }}. Effective pass rate: ${{ context.effective_pass_rate }}. - Scorecard: ${{ context.scorecard_path }} + Scorecard: ${{ context.scorecard_path }}. + Emit the L3 sentinel with success status. diff --git a/.autoskillit/recipes/eval/skill-eval.yaml b/.autoskillit/recipes/eval/skill-eval.yaml index 59484282d4..fd8e2ab110 100644 --- a/.autoskillit/recipes/eval/skill-eval.yaml +++ b/.autoskillit/recipes/eval/skill-eval.yaml @@ -65,9 +65,6 @@ steps: output_dir: "${{ inputs.output_dir }}" capture: eval_run_dir: "${{ result.eval_run_dir }}" - canary_count: "${{ result.canary_count }}" - variant_count: "${{ result.variant_count }}" - manifest_index_path: "${{ result.manifest_index_path }}" on_success: run_canaries on_failure: escalate note: >- @@ -86,7 +83,7 @@ steps: Instead, follow these instructions using MCP tool calls directly: - 1. Read the manifest index: run_cmd cat {context.manifest_index_path} + 1. Read the manifest index: run_cmd cat {context.eval_run_dir}/manifest_index.json Parse the JSON to get canary_ids, variant_ids, and directory paths. @@ -161,4 +158,5 @@ steps: (${{ context.passed_runs }}/${{ context.total_runs }}). Vacuous passes: ${{ context.vacuous_passes }}. Effective pass rate: ${{ context.effective_pass_rate }}. - Scorecard: ${{ context.scorecard_path }} \ No newline at end of file + Scorecard: ${{ context.scorecard_path }}. + Emit the L3 sentinel with success status. \ No newline at end of file diff --git a/src/autoskillit/fleet/_api.py b/src/autoskillit/fleet/_api.py index f2b23e528b..e9e543271a 100644 --- a/src/autoskillit/fleet/_api.py +++ b/src/autoskillit/fleet/_api.py @@ -391,9 +391,13 @@ async def _run_dispatch( error_findings = [ s for s in validation_result.get("suggestions", []) if s.get("severity") == "error" ] + total_errors = len(structural_errors) + len(error_findings) error_parts = structural_errors[:3] + [ f"[{f['rule']}] {f['message']}" for f in error_findings[:3] ] + shown = len(error_parts) + if total_errors > shown: + error_parts.append(f"+{total_errors - shown} more errors") return DispatchResult( DispatchRejected( error_code=FleetErrorCode.FLEET_RECIPE_INVALID, diff --git a/src/autoskillit/recipe/_api.py b/src/autoskillit/recipe/_api.py index e65168ae26..5d516f50ad 100644 --- a/src/autoskillit/recipe/_api.py +++ b/src/autoskillit/recipe/_api.py @@ -462,11 +462,47 @@ def load_and_validate( t0 = _t("semantic_rules", t0, name) if _skip_resolutions and any(v is False for v in _skip_resolutions.values()): + # Note: the capture-inversion-detection rule is intentionally NOT + # filtered here. Its strict forward-path dominance check (R1) in + # _check_capture_inversion subsumes what the filter would mask for + # this rule, so the filter would be a no-op for it by construction + # (the pre-prune baseline is structurally blind for that rule due + # to bypass edges emptying the BFS fact intersection). The filter + # remains useful for other rules like dead-output where pruning + # genuinely introduces new findings. + # + # Also: _skip_resolutions values are tristate — True (kept), + # False (pruned), None (deferred when defer_unresolved=True). + # The `is False` identity comparison correctly excludes None. semantic_findings = filter_pruning_false_positives( semantic_findings, _pre_prune_findings ) semantic_suggestions = findings_to_dicts(semantic_findings) + # Defense-in-depth diagnostic: surface any post-prune finding from + # a graph-aware rule that did NOT appear in the pre-prune baseline. + # If a future rule has a gap similar to the original capture-inversion + # bug, this surfaces it during development rather than masking it + # silently via the filter. Logged at debug level (no production impact). + _graph_aware_rules = {"capture-inversion-detection", "dead-output"} + _pre_prune_keys = {(f.rule, f.step_name) for f in _pre_prune_findings} + for _f in semantic_findings: + if ( + _f.rule in _graph_aware_rules + and ( + _f.rule, + _f.step_name, + ) + not in _pre_prune_keys + ): + logger.debug( + "pruning_filter_new_finding", + recipe=name, + rule=_f.rule, + step=_f.step_name, + message=_f.message, + ) + _suppressed = suppressed or [] if name in _suppressed: semantic_suggestions = filter_version_rule(semantic_suggestions) diff --git a/src/autoskillit/recipe/_api_listing.py b/src/autoskillit/recipe/_api_listing.py index a41e1d9a97..32dc87b01f 100644 --- a/src/autoskillit/recipe/_api_listing.py +++ b/src/autoskillit/recipe/_api_listing.py @@ -147,6 +147,11 @@ def validate_from_path( report = ctx.dataflow semantic_findings = run_semantic_rules(ctx) if _skip_resolutions and any(v is False for v in _skip_resolutions.values()): + # Note: the capture-inversion-detection rule is intentionally NOT + # filtered here. Its strict forward-path dominance check (R1) in + # _check_capture_inversion subsumes what the filter would mask for + # this rule. The filter remains useful for other rules like + # dead-output where pruning genuinely introduces new findings. semantic_findings = filter_pruning_false_positives(semantic_findings, _pre_prune_findings) quality = build_quality_dict(report) diff --git a/src/autoskillit/recipe/rules/rules_reachability.py b/src/autoskillit/recipe/rules/rules_reachability.py index 3a335fc9d8..e3bbd151df 100644 --- a/src/autoskillit/recipe/rules/rules_reachability.py +++ b/src/autoskillit/recipe/rules/rules_reachability.py @@ -27,6 +27,7 @@ _bfs_with_facts, bfs_reachable, ) +from autoskillit.recipe._analysis_bfs import bfs_reachable_without_barrier from autoskillit.recipe.registry import RuleFinding, make_finding, semantic_rule # Regex to find ${{ context.X }} references anywhere in a string value. @@ -129,6 +130,24 @@ def _check_capture_inversion(ctx: ValidationContext) -> list[RuleFinding]: if not producers: continue # no producer anywhere — not an inversion, different bug + # Strict forward-path dominance: exonerate if any producer strictly + # dominates step_name on success paths — every success-path from entry + # to step_name crosses the producer, so var is always captured before + # step_name runs. Using backward reachability (_ancestors) here would + # accept "on some path" producers that don't dominate fork-join readers, + # producing false negatives in real inversions. bfs_reachable_without_barrier + # builds its own success-path graph from ctx.recipe (post-prune) and + # returns all steps reachable from entry without crossing the barrier. + # If step_name is NOT in this set, the producer strictly dominates it. + exonerated = False + for _producer in producers: + _reachable_without = bfs_reachable_without_barrier(ctx.recipe, entry, _producer) + if step_name not in _reachable_without: + exonerated = True + break + if exonerated: + continue + findings.append( make_finding( rule_name="capture-inversion-detection", diff --git a/src/autoskillit/server/tools/tools_kitchen.py b/src/autoskillit/server/tools/tools_kitchen.py index e1c3922c43..6cb020c9da 100644 --- a/src/autoskillit/server/tools/tools_kitchen.py +++ b/src/autoskillit/server/tools/tools_kitchen.py @@ -116,12 +116,17 @@ def _recipe_validation_error_response(name: str, result: dict[str, Any]) -> str: _structural_errs: list[str] = result.get("errors", []) if _structural_errs: _error_parts = _structural_errs[:3] + if len(_structural_errs) > 3: + _error_parts.append(f"+{len(_structural_errs) - 3} more errors") else: - _error_parts = [ + _all_errors = [ f"[{s.get('rule', 'unknown-rule')}] {s.get('message', '')}" for s in result.get("suggestions", []) if isinstance(s, dict) and s.get("severity") == "error" - ][:3] + ] + _error_parts = _all_errors[:3] + if len(_all_errors) > 3: + _error_parts.append(f"+{len(_all_errors) - 3} more errors") _error_detail = "; ".join(_error_parts) if _error_parts else "unknown structural error" _label = "structural validation" if _structural_errs else "validation" return json.dumps( diff --git a/tests/arch/test_rule_severity_consistency.py b/tests/arch/test_rule_severity_consistency.py index 62b0941517..8519c6c3e7 100644 --- a/tests/arch/test_rule_severity_consistency.py +++ b/tests/arch/test_rule_severity_consistency.py @@ -270,13 +270,6 @@ def _collect_error_severity_rules() -> set[str]: return error_rules -@pytest.mark.xfail( - strict=True, - reason=( - "2 ERROR-severity rules still in allowlist — fix agent-eval and skill-eval " - "before removing xfail" - ), -) def test_error_severity_rules_have_no_dispatch_ready_exemptions() -> None: """Every ERROR-severity rule must have zero entries in the dispatch-ready allowlist. diff --git a/tests/fleet/AGENTS.md b/tests/fleet/AGENTS.md index cb688af0a9..f2393e0652 100644 --- a/tests/fleet/AGENTS.md +++ b/tests/fleet/AGENTS.md @@ -39,6 +39,7 @@ Fleet campaign dispatch, state persistence, and sidecar tests. | `test_findings_rpc.py` | Tests for autoskillit.fleet._findings_rpc (T15–T21) | | `test_fleet.py` | Tests for fleet package | | `test_fleet_e2e.py` | Fleet Group O: end-to-end test suite for fleet dispatch loop | +| `test_fleet_error_count_surfacing.py` | Tests for fleet-path +N more errors indicator in dispatch validation (R4) | | `test_fleet_e2e_codex.py` | Fleet Group O-codex: end-to-end codex-backend dispatch via shim | | `test_fleet_rename_integrity.py` | Fleet rename integrity guard | | `test_fleet_semaphore.py` | Unit tests for FleetSemaphore (FleetLock semaphore implementation) | diff --git a/tests/fleet/test_fleet_error_count_surfacing.py b/tests/fleet/test_fleet_error_count_surfacing.py new file mode 100644 index 0000000000..50c1c56ade --- /dev/null +++ b/tests/fleet/test_fleet_error_count_surfacing.py @@ -0,0 +1,90 @@ +"""Tests for fleet-path error count surfacing — the +N more indicator in dispatch validation. + +R4: when fleet dispatch validation produces more errors than the display cap, +the DispatchRejected message must include a '+N more errors' indicator. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from tests.fleet._helpers import _no_sleep_quota_checker, _noop_quota_refresher + +pytestmark = [pytest.mark.layer("fleet"), pytest.mark.small, pytest.mark.feature("fleet")] + + +def _make_suggestions(n: int) -> list[dict[str, str]]: + return [ + {"rule": f"rule-{i}", "severity": "error", "message": f"error number {i}"} + for i in range(n) + ] + + +@pytest.mark.anyio +async def test_fleet_dispatch_surfaces_plus_n_more_for_combined_overflow(tool_ctx): + """5 structural + 5 semantic errors → shown=6, overflow='+4 more errors'.""" + tool_ctx.recipes = MagicMock() + tool_ctx.recipes.find.return_value = MagicMock() + tool_ctx.recipes.load.return_value = MagicMock() + tool_ctx.recipes.load_and_validate.return_value = { + "valid": False, + "errors": [f"structural error {i}" for i in range(5)], + "suggestions": _make_suggestions(5), + } + + with patch("autoskillit.fleet._api.execute_dispatch", new_callable=AsyncMock): + from autoskillit.fleet._api import _run_dispatch + from autoskillit.fleet.state_types import DispatchRejected + + result = await _run_dispatch( + tool_ctx=tool_ctx, + recipe="test-recipe", + task="test task", + ingredients={}, + dispatch_name=None, + timeout_sec=None, + prompt_builder=lambda **kw: "prompt", + quota_checker=_no_sleep_quota_checker, + quota_refresher=_noop_quota_refresher, + ) + + assert isinstance(result.outcome, DispatchRejected) + assert "+4 more errors" in result.outcome.message, ( + f"Expected '+4 more errors' for 5+5 combined, got: {result.outcome.message!r}" + ) + + +@pytest.mark.anyio +async def test_fleet_dispatch_no_indicator_at_exactly_six(tool_ctx): + """3 structural + 3 semantic errors → shown=6, no overflow indicator.""" + tool_ctx.recipes = MagicMock() + tool_ctx.recipes.find.return_value = MagicMock() + tool_ctx.recipes.load.return_value = MagicMock() + tool_ctx.recipes.load_and_validate.return_value = { + "valid": False, + "errors": [f"structural error {i}" for i in range(3)], + "suggestions": _make_suggestions(3), + } + + with patch("autoskillit.fleet._api.execute_dispatch", new_callable=AsyncMock): + from autoskillit.fleet._api import _run_dispatch + from autoskillit.fleet.state_types import DispatchRejected + + result = await _run_dispatch( + tool_ctx=tool_ctx, + recipe="test-recipe", + task="test task", + ingredients={}, + dispatch_name=None, + timeout_sec=None, + prompt_builder=lambda **kw: "prompt", + quota_checker=_no_sleep_quota_checker, + quota_refresher=_noop_quota_refresher, + ) + + assert isinstance(result.outcome, DispatchRejected) + assert "more errors" not in result.outcome.message, ( + f"Did not expect overflow indicator for exactly 6 shown, got: {result.outcome.message!r}" + ) diff --git a/tests/infra/test_schema_version_convention.py b/tests/infra/test_schema_version_convention.py index cb0073f17f..5dff5069f1 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", 90), # tools_kitchen.py — hook config, quota guard, git_ops_policy, ingredient locks overlay - ("src/autoskillit/server/tools/tools_kitchen.py", 228), - ("src/autoskillit/server/tools/tools_kitchen.py", 247), - ("src/autoskillit/server/tools/tools_kitchen.py", 281), - ("src/autoskillit/server/tools/tools_kitchen.py", 1119), - ("src/autoskillit/server/tools/tools_kitchen.py", 1179), + ("src/autoskillit/server/tools/tools_kitchen.py", 233), + ("src/autoskillit/server/tools/tools_kitchen.py", 252), + ("src/autoskillit/server/tools/tools_kitchen.py", 286), + ("src/autoskillit/server/tools/tools_kitchen.py", 1124), + ("src/autoskillit/server/tools/tools_kitchen.py", 1184), # tools_pipeline_tracker.py — tracker_data dict ("src/autoskillit/server/tools/tools_pipeline_tracker.py", 166), # tools_status.py — mcp_data dict diff --git a/tests/recipe/AGENTS.md b/tests/recipe/AGENTS.md index 824db26df0..ce37f7938a 100644 --- a/tests/recipe/AGENTS.md +++ b/tests/recipe/AGENTS.md @@ -26,6 +26,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. | `test_bundled_recipes_general.py` | General structural tests for all bundled recipes | | `test_bundled_recipes_dispatch_ready.py` | Universal dispatch-readiness gate for all bundled recipes and contract cards | | `test_bundled_recipes_no_inversions.py` | Guard: no inversion step order violations in bundled recipes | +| `test_bundled_recipes_all_truthy.py` | Regression guard: every bundled recipe validates cleanly under all-truthy and default ingredient configs (R1, R3) | | `test_bundled_recipes_pipeline_structure.py` | Pipeline structure tests for bundled recipes | | `test_bundled_recipes_research.py` | Tests for research bundled recipe structure | | `test_bundled_recipes_research_design.py` | Tests for research-design bundled recipe structure | diff --git a/tests/recipe/test_bundled_recipes_all_truthy.py b/tests/recipe/test_bundled_recipes_all_truthy.py new file mode 100644 index 0000000000..29f65adecb --- /dev/null +++ b/tests/recipe/test_bundled_recipes_all_truthy.py @@ -0,0 +1,104 @@ +"""Bundled-recipe regression guard: every bundled recipe must validate cleanly. + +Reproduces the capture-inversion false positive bug that blocked open_kitchen +when every skip_when_false ingredient resolves to "true". With R1's strict +forward-path dominance check, these recipes validate cleanly. R3 adds the +complementary default-pruned configuration to guard against regressions in the +non-truthy path. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any + +import pytest + +from autoskillit.recipe import all_validated_recipe_names, load_and_validate + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent + +_ALL_TRUTHY_SKIP_INGREDIENTS: dict[str, str] = { + "is_fleet_dispatch": "true", + "adversarial_review_level": "true", + "local_review_rounds": "true", + "base_branch": "true", + "post_run_diagnostics": "true", +} + + +def _all_truthy_overrides() -> dict[str, Any]: + """Build overrides dict that forces every skip_when_false ingredient truthy.""" + overrides: dict[str, Any] = dict(_ALL_TRUTHY_SKIP_INGREDIENTS) + overrides["task"] = "test task" + overrides["issue_url"] = "https://github.com/test/test/issues/1" + overrides["source_dir"] = str(_PROJECT_ROOT) + return overrides + + +def _default_overrides() -> dict[str, Any]: + """Build overrides dict with only required fields — skip_when_false ingredients at defaults.""" + return { + "task": "test task", + "issue_url": "https://github.com/test/test/issues/1", + "source_dir": str(_PROJECT_ROOT), + } + + +def _recipe_names() -> list[str]: + return sorted(all_validated_recipe_names(_PROJECT_ROOT)) + + +@pytest.mark.parametrize("recipe_name", _recipe_names(), ids=lambda n: n) +def test_bundled_recipe_validates_with_all_truthy_ingredients( + recipe_name: str, tmp_path: Path +) -> None: + """Every bundled recipe must validate under the all-truthy config.""" + cwd_before = Path.cwd() + try: + os.chdir(tmp_path) + result = load_and_validate( + recipe_name, + project_dir=_PROJECT_ROOT, + ingredient_overrides=_all_truthy_overrides(), + ) + errors = [s for s in result.get("suggestions", []) if s.get("severity") == "error"] + assert result["valid"] is True, ( + f"{recipe_name} produced errors under all-truthy config: {errors}\n" + f"result keys: {sorted(result.keys())}" + ) + assert not errors, ( + f"{recipe_name} has error-severity findings under all-truthy config:\n" + + "\n".join(f" [{s.get('rule')}] {s.get('message')}" for s in errors) + ) + finally: + os.chdir(cwd_before) + + +@pytest.mark.parametrize("recipe_name", _recipe_names(), ids=lambda n: n) +def test_bundled_recipe_validates_with_default_ingredients( + recipe_name: str, tmp_path: Path +) -> None: + """Every bundled recipe must validate under the default (non-truthy) config.""" + cwd_before = Path.cwd() + try: + os.chdir(tmp_path) + result = load_and_validate( + recipe_name, + project_dir=_PROJECT_ROOT, + ingredient_overrides=_default_overrides(), + ) + errors = [s for s in result.get("suggestions", []) if s.get("severity") == "error"] + assert result["valid"] is True, ( + f"{recipe_name} produced errors under default config: {errors}\n" + f"result keys: {sorted(result.keys())}" + ) + assert not errors, ( + f"{recipe_name} has error-severity findings under default config:\n" + + "\n".join(f" [{s.get('rule')}] {s.get('message')}" for s in errors) + ) + finally: + os.chdir(cwd_before) diff --git a/tests/recipe/test_bundled_recipes_dispatch_ready.py b/tests/recipe/test_bundled_recipes_dispatch_ready.py index 5b578083df..8fd9d945e2 100644 --- a/tests/recipe/test_bundled_recipes_dispatch_ready.py +++ b/tests/recipe/test_bundled_recipes_dispatch_ready.py @@ -28,16 +28,7 @@ _CONTRACT_STEMS = sorted(p.stem for p in _CONTRACTS_DIR.glob("*.yaml")) -_KNOWN_NON_CONFORMING_RULES: dict[str, set[str]] = { - "agent-eval": { # tracking: #4069 - "all-dispatchable-stops-have-sentinel", - "dead-output", - }, - "skill-eval": { # tracking: #4069 - "all-dispatchable-stops-have-sentinel", - "dead-output", - }, -} +_KNOWN_NON_CONFORMING_RULES: dict[str, set[str]] = {} _RECIPES_WITH_OTHER_ERROR_RULES: frozenset[str] = frozenset( k for k, v in _KNOWN_NON_CONFORMING_RULES.items() if v diff --git a/tests/recipe/test_recipe_backend_composition_matrix.py b/tests/recipe/test_recipe_backend_composition_matrix.py index 22a272fa5d..5df8dcb553 100644 --- a/tests/recipe/test_recipe_backend_composition_matrix.py +++ b/tests/recipe/test_recipe_backend_composition_matrix.py @@ -42,20 +42,7 @@ # -- Known-broken combos (xfail strict) ------------------------------------- -KNOWN_BROKEN: dict[tuple[str, str], str] = { - ("agent-eval", "claude-code"): ( - "tracking: #4069 -- violates all-dispatchable-stops-have-sentinel + dead-output" - ), - ("agent-eval", "codex"): ( - "tracking: #4069 -- violates all-dispatchable-stops-have-sentinel + dead-output" - ), - ("skill-eval", "claude-code"): ( - "tracking: #4069 -- violates all-dispatchable-stops-have-sentinel + dead-output" - ), - ("skill-eval", "codex"): ( - "tracking: #4069 -- violates all-dispatchable-stops-have-sentinel + dead-output" - ), -} +KNOWN_BROKEN: dict[tuple[str, str], str] = {} _SKILL_RESOLVER = DefaultSkillResolver() diff --git a/tests/recipe/test_rules_reachability.py b/tests/recipe/test_rules_reachability.py index ae6ddb68f2..73bf4c96af 100644 --- a/tests/recipe/test_rules_reachability.py +++ b/tests/recipe/test_rules_reachability.py @@ -10,9 +10,14 @@ import pytest -from autoskillit.recipe._analysis import _bfs_with_facts, _build_step_graph +from autoskillit.recipe._analysis import ( + _bfs_with_facts, + _build_step_graph, + make_validation_context, +) from autoskillit.recipe.io import builtin_recipes_dir, load_recipe from autoskillit.recipe.registry import run_semantic_rules +from autoskillit.recipe.rules.rules_reachability import _check_capture_inversion from autoskillit.recipe.schema import ( Recipe, RecipeStep, @@ -178,3 +183,177 @@ def _make_branching_recipe() -> Recipe: "join": join, }, ) + + +# --------------------------------------------------------------------------- +# Dominance exoneration tests (R1) +# --------------------------------------------------------------------------- + + +def _make_dominating_producer_recipe() -> Recipe: + """Recipe where producer B unconditionally captures var_x and dominates reader D. + + Layout: + entry ─has_gate→ has_gate (skip_when_false) → producer → router_x → reader + bypass: entry → router_x (skips producer when skip_when_false) + + The bypass edge from skip_when_false creates a path that does NOT go through + the producer, causing the fact intersection to drop var_x from known_vars + at the reader — but the producer dominates the reader on every success + path, so the reader is safe. + """ + entry = RecipeStep(name="entry", action="route", on_success="has_gate") + has_gate = RecipeStep( + name="has_gate", + action="route", + skip_when_false="some_gate", + on_success="producer", + on_result=StepResultRoute( + conditions=[ + StepResultCondition(route="router_x", when="context.some_gate == 'true'"), + StepResultCondition(route="router_x"), + ] + ), + ) + producer = RecipeStep( + name="producer", + tool="run_cmd", + with_args={"cmd": "echo capturing var_x"}, + capture={"var_x": "${{ result.output }}"}, + on_success="router_x", + ) + router_x = RecipeStep( + name="router_x", + action="route", + on_result=StepResultRoute( + conditions=[ + StepResultCondition(route="consumer", when="context.var_x == 'yes'"), + StepResultCondition(route="consumer"), + ] + ), + ) + consumer = RecipeStep( + name="consumer", + tool="run_cmd", + with_args={"cmd": "echo ${{ context.var_x }}"}, + ) + return Recipe( + name="synthetic-dominating-producer", + description="producer dominates reader via success-path barrier", + steps={ + "entry": entry, + "has_gate": has_gate, + "producer": producer, + "router_x": router_x, + "consumer": consumer, + }, + ) + + +def test_dominating_producer_exonerates_reader(): + """R1: an unconditional producer that strictly dominates the reader exonerates it. + + The skip_when_false bypass creates a path that bypasses the producer in the + pre-prune graph, dropping var_x from known_vars at the reader. The strict + forward-path dominance check correctly exonerates the reader because the + producer is on every success path. + """ + recipe = _make_dominating_producer_recipe() + ctx = make_validation_context(recipe) + findings = _check_capture_inversion(ctx) + consumer_findings = [f for f in findings if "consumer" in f.message] + assert consumer_findings == [], ( + "Dominating producer should exonerate consumer, got: " + + ", ".join(f.message for f in consumer_findings) + ) + + +def _make_non_dominating_producer_recipe() -> Recipe: + """Recipe where producer is on only ONE branch of a fork (not a dominator). + + Layout: + entry → fork → branch_producer (captures var_x) → joiner + → branch_other (no capture) → joiner + joiner → check_x (conditional on var_x) → reader + → reader (fallback) + reader reads var_x + + The producer is on one branch only — does NOT dominate the reader. Should + produce a capture-inversion finding. + + The joiner routes to two DIFFERENT targets (check_x vs reader) so the + conditional edge fact (var_x, yes) only reaches reader via the check_x + path, while the fallback path carries no fact. The intersection at reader + drops var_x from known_vars, exercising the dominance check. + """ + entry = RecipeStep( + name="entry", + action="route", + on_result=StepResultRoute( + conditions=[ + StepResultCondition(route="branch_producer", when="context.flag == 'yes'"), + StepResultCondition(route="branch_other"), + ] + ), + ) + branch_producer = RecipeStep( + name="branch_producer", + tool="run_cmd", + with_args={"cmd": "echo producing"}, + capture={"var_x": "${{ result.output }}"}, + on_success="joiner", + ) + branch_other = RecipeStep( + name="branch_other", + tool="run_cmd", + with_args={"cmd": "echo nothing"}, + on_success="joiner", + ) + joiner = RecipeStep( + name="joiner", + action="route", + on_result=StepResultRoute( + conditions=[ + StepResultCondition(route="check_x", when="context.var_x == 'yes'"), + StepResultCondition(route="reader"), + ] + ), + ) + check_x = RecipeStep( + name="check_x", + tool="run_cmd", + with_args={"cmd": "echo check"}, + on_success="reader", + ) + reader = RecipeStep( + name="reader", + tool="run_cmd", + with_args={"cmd": "echo ${{ context.var_x }}"}, + ) + return Recipe( + name="synthetic-non-dominating", + description="producer on one branch only — not a dominator", + steps={ + "entry": entry, + "branch_producer": branch_producer, + "branch_other": branch_other, + "joiner": joiner, + "check_x": check_x, + "reader": reader, + }, + ) + + +def test_non_dominating_producer_does_not_exonerate_reader(): + """Negative test: producer on one branch only should NOT exonerate reader. + + Distinguishes "on some path" (ancestor) from "on every path" (dominator). + """ + recipe = _make_non_dominating_producer_recipe() + ctx = make_validation_context(recipe) + findings = _check_capture_inversion(ctx) + reader_findings = [f for f in findings if "reader" in f.message] + assert reader_findings, ( + "Non-dominating producer should flag reader with capture-inversion, " + "but no findings were produced." + ) diff --git a/tests/server/AGENTS.md b/tests/server/AGENTS.md index c488b8a0e0..932bc4a23a 100644 --- a/tests/server/AGENTS.md +++ b/tests/server/AGENTS.md @@ -97,6 +97,7 @@ Server tool handler unit tests — kitchen, execution, CI, clone, workspace tool | `test_tools_kitchen_preflight.py` | Tests for dispatch-feasibility preflight in open_kitchen and shared _check_dispatch_feasibility function | | `test_tools_fleet_dispatch_preflight.py` | Tests for dispatch_food_truck preflight wiring — preflight before execute_dispatch | | `test_tools_kitchen_helpers.py` | Tests for tools_kitchen.py helper functions: _recipe_validation_error_response semantic-finding surfacing | +| `test_error_count_surfacing.py` | Tests for the +N more errors indicator in validation error responses (R4) | | `test_tools_kitchen_gate.py` | Tests for tools_kitchen.py: gate toggle, review gate cleanup, kitchen_id, misc | | `test_tools_kitchen_gate_features.py` | Tests for tools_kitchen.py: recipe packs, quota refresh, ingredients_only, project_dir | | `test_tools_kitchen_gate_hook_config.py` | Tests for tools_kitchen.py: hook config lifecycle, overlay, and quota guard tool | diff --git a/tests/server/test_error_count_surfacing.py b/tests/server/test_error_count_surfacing.py new file mode 100644 index 0000000000..72ac0ed72b --- /dev/null +++ b/tests/server/test_error_count_surfacing.py @@ -0,0 +1,69 @@ +"""Tests for error count surfacing — the +N more indicator in validation responses. + +R4: ensure that when validation produces >3 error-severity findings, the +user_visible_message includes a '+N more errors' indicator instead of silently +truncating beyond the first three. +""" + +from __future__ import annotations + +import json + +import pytest + +from autoskillit.server.tools.tools_kitchen import _recipe_validation_error_response + +pytestmark = [pytest.mark.layer("server"), pytest.mark.small] + + +def _make_suggestions(n: int) -> list[dict[str, str]]: + return [ + { + "rule": f"rule-{i}", + "severity": "error", + "message": f"error number {i}", + } + for i in range(n) + ] + + +def test_validation_error_response_surfaces_plus_n_more_for_excess_semantic_errors(): + """When >3 error-severity suggestions exist, message should indicate overflow.""" + suggestions = _make_suggestions(5) + result: dict[str, object] = {"errors": [], "suggestions": suggestions} + envelope = json.loads(_recipe_validation_error_response("recipe_x", result)) + msg = envelope["user_visible_message"] + assert "+2 more errors" in msg, f"Expected '+2 more errors' indicator, got: {msg!r}" + + +def test_validation_error_response_no_indicator_at_exactly_three(): + """At exactly 3 errors, no +N more indicator is needed.""" + suggestions = _make_suggestions(3) + result: dict[str, object] = {"errors": [], "suggestions": suggestions} + envelope = json.loads(_recipe_validation_error_response("recipe_x", result)) + msg = envelope["user_visible_message"] + assert "more errors" not in msg, f"Did not expect overflow indicator, got: {msg!r}" + + +def test_validation_error_response_counts_structural_errors_too(): + """Structural errors also contribute to the overflow count.""" + result: dict[str, object] = { + "errors": ["e1", "e2", "e3", "e4", "e5"], + "suggestions": [], + } + envelope = json.loads(_recipe_validation_error_response("recipe_x", result)) + msg = envelope["user_visible_message"] + assert "+2 more errors" in msg, ( + f"Expected '+2 more errors' for 5 structural errors, got: {msg!r}" + ) + + +def test_validation_error_response_includes_one_when_4_structural_errors(): + """Boundary case: 4 structural errors yields '+1 more errors'.""" + result: dict[str, object] = { + "errors": ["e1", "e2", "e3", "e4"], + "suggestions": [], + } + envelope = json.loads(_recipe_validation_error_response("recipe_x", result)) + msg = envelope["user_visible_message"] + assert "+1 more errors" in msg, f"Expected '+1 more errors' for 4 structural, got: {msg!r}" diff --git a/tests/server/test_tools_recipe.py b/tests/server/test_tools_recipe.py index bd200378e3..2b5b80907f 100644 --- a/tests/server/test_tools_recipe.py +++ b/tests/server/test_tools_recipe.py @@ -488,10 +488,12 @@ async def test_list_recipes_mcp_tool_hides_campaign_when_fleet_disabled( @pytest.mark.anyio -async def test_list_recipes_returns_error_string_when_context_missing() -> None: +async def test_list_recipes_returns_error_string_when_context_missing(monkeypatch) -> None: """list_recipes must return an error message string (not []) when tool_ctx is None.""" + from autoskillit.server import _state from autoskillit.server.tools.tools_recipe import list_recipes + monkeypatch.setattr(_state, "_ctx", None) result = await list_recipes() assert isinstance(result, str) parsed = json.loads(result)