P3 — Recipe Semantic Rule Test Coverage#1561
Conversation
Add test classes for block-mcp-tool-budget, block-gh-api-forbidden, block-single-producer, block-entry-exit-reachable, stop-step-message-quality, ci-timeout-minimum, research-output-mode-enum, and constant-step-with-args. Each class covers violation and clean paths, following existing inline Recipe construction patterns and run_semantic_rules() invocation with rule-filtered assertions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: needs_human
@Trecek This PR has findings that require a human decision on design trade-offs before merging.
⚠️ Outside Diff Range
These findings target lines not in the diff and could not be posted as inline comments:
tests/recipe/test_rules_tools.py
- L393 [warning/cohesion]: TestConstantStepWithArgs constructs Recipe inline rather than using the file-local helpers that all other tests rely on. The existing file-pattern uses helpers for recipe construction — inline construction here breaks that pattern. Decision needed: create a wrapper helper or accept the inline approach.
tests/recipe/test_rules_ci.py
- L884 [info/cohesion]: New TestCiTimeoutMinimum methods carry '-> None' return type annotations consistent with the pre-existing module-level test functions, while new test classes in test_rules_actions.py, test_rules_blocks.py, test_rules_inputs.py, and test_rules_tools.py omit '-> None'. Cross-file annotation asymmetry.
tests/recipe/test_rules_blocks.py
- L63 [info/cohesion]: _make_block_recipe is placed after the existing specific helper and a test that uses it, rather than near or before the specialized helper it conceptually supersedes. Consider grouping general factories before specialized ones.
tests/recipe/test_rules_inputs.py
- L176 [info/cohesion]: TestResearchOutputModeEnum constructs Recipe inline across four test methods sharing the same skeleton. A local _make_research_recipe factory would align with the _make_block_recipe pattern introduced in this same PR.
| tool="check_repo_merge_state", block="pre_queue_gate", on_success="step_b" | ||
| ) | ||
| step_b = RecipeStep( | ||
| tool="check_repo_merge_state", block="pre_queue_gate", on_success="done" |
There was a problem hiding this comment.
[warning] cohesion: TestBlockSingleProducer and TestBlockEntryExitReachable mutate step.name via post-construction assignment (step_a.name = "step_a") after building with RecipeStep(). The _make_block_recipe factory is the designated construction boundary for this file, yet name injection is performed outside it — scattered at each call site. If name assignment is required for the rule to work, consider handling it inside the factory or via a parameter.
1 participant
Summary
Add test coverage for 8 untested recipe semantic validation rules across 5 test files. Pure test additions — no source files modified. Each rule gets a dedicated test class with violation and clean paths, following existing test patterns (inline
Recipe/RecipeStepconstruction,run_semantic_rules()invocation, finding-filtered assertions). ~20 test methods total, all xdist-safe.Closes #1539
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260430-081121-610236/.autoskillit/temp/make-plan/p3_recipe_semantic_rule_test_coverage_plan_2026-04-30_081500.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary