Add type field to RecipeIngredient and enforce integer-default consistency#2171
Merged
Trecek merged 4 commits intoMay 7, 2026
Merged
Conversation
…default consistency Add `type: str | None` field to RecipeIngredient schema, parsed from YAML `type:` key. Add semantic rule `ingredient-type-default-invalid` that rejects integer-typed ingredients with `default: ""` (auto-detect sentinel) or non-parseable defaults. Update `local_review_rounds` in implementation.yaml, remediation.yaml, and implementation-groups.yaml to use explicit `type: integer` and `default: "3"` instead of the shadow default `""`. Add `local_review_rounds: "0"` to merge-prs.yaml annotate_pr_diff step to make github-review mode explicit. Fixes GitHub Issue #2166 — local_review_rounds default="" bypassed type semantics and caused reviews to never post to GitHub. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The T5 test imported resolve_ingredient_defaults from autoskillit.config, violating the layer boundary (tests/recipe only allows autoskillit.core and autoskillit.recipe). Additionally, resolve_ingredient_defaults returns "0" in test environments without project config (exception fallback), causing a false mismatch with the explicit "3" default. Replace with an in-layer assertion that local_review_rounds declares type=integer, which is now enforced by the ingredient-type-default-invalid semantic rule. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt in integer-default rule Bind ValueError as exc and include str(exc) in the finding message for non-parseable integer defaults. Move continue inside the try block after int(default) so control flow intent is explicit on both success and error paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bare int(ing.default) produces pytest ERROR status on ValueError instead of FAILED, making CI output harder to diagnose. Use try/except with pytest.fail() for a clear assertion failure message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trecek
deleted the
local-review-rounds-ingredient-default-bypasses-type-semanti/2166
branch
Trecek
added a commit
that referenced
this pull request
May 8, 2026
…tency (#2171) ## Summary The `RecipeIngredient` schema has no `type` field — every ingredient default is stored as `str | None` with no type semantics. The convention of `default: ""` means "auto-detect from config" but only feeds the display layer (`resolve_ingredient_defaults` → `build_ingredient_rows`), not the runtime template engine. When `${{ inputs.local_review_rounds }}` is not explicitly supplied, the runtime receives `""` while the LLM sees `"3"` in the ingredients table. This creates a **shadow default** where the display-default and runtime-default are two separate values with no enforcement they agree. The fix adds a `type` field to `RecipeIngredient` and a semantic rule that rejects `type: integer` + `default: ""` at recipe load time, eliminating the ambiguity for all future recipe authors. ## Requirements **Expected Fix:** 1. Change the recipe ingredient `default: ""` to an explicit integer default (e.g., `default: "3"`) that matches the config value, eliminating the LLM interpretation ambiguity. 2. Ensure the final review cycle always posts to GitHub regardless of `local_review_rounds` (so reviews that resolve within the local budget still get GitHub visibility). 3. Consider whether `default: ""` should be disallowed for ingredients that map to integer config fields — a schema-level typing constraint. ## Changed Files ### Modified (●): - `src/autoskillit/recipe/io.py` - `src/autoskillit/recipe/rules/rules_inputs.py` - `src/autoskillit/recipe/schema.py` - `src/autoskillit/recipes/implementation-groups.yaml` - `src/autoskillit/recipes/implementation.yaml` - `src/autoskillit/recipes/merge-prs.yaml` - `src/autoskillit/recipes/remediation.yaml` - `tests/recipe/test_bundled_recipes_review_pr.py` - `tests/recipe/test_io_parsing.py` - `tests/recipe/test_merge_prs.py` - `tests/recipe/test_rules_inputs.py` - `tests/recipe/test_schema.py` ## Implementation Plan Plan file: `/home/talon/projects/autoskillit-runs/remediation-20260507-084045-358772/.autoskillit/temp/rectify/rectify_ingredient_type_enforcement_2026-05-07_090000.md` 🤖 Generated with [Claude Code](https://claude.com/claude-code) via AutoSkillit <!-- autoskillit:pipeline-signature steps=prepare_pr,run_arch_lenses,compose_pr,annotate_pr_diff,review_pr --> ## Token Usage Summary | Step | Model | count | uncached | output | cache_read | peak_ctx | turns | cache_write | time | |------|-------|-------|----------|--------|------------|----------|-------|-------------|------| | rectify | claude-sonnet-4-6 | 1 | 3.2k | 18.0k | 666.7k | 93.6k | 172 | 50.6k | 7m 38s | | dry_walkthrough | claude-opus-4-6 | 1 | 1.3k | 10.3k | 1.5M | 69.5k | 99 | 56.4k | 6m 19s | | implement* | MiniMax-M2.7-highspeed | 1 | 2.9M | 18.6k | 2.1M | 64.1k | 169 | 66.5k | 8m 29s | | assess | claude-opus-4-6 | 1 | 53 | 7.3k | 1.3M | 67.6k | 43 | 54.7k | 4m 40s | | prepare_pr* | MiniMax-M2.7-highspeed | 1 | 65.2k | 4.1k | 183.8k | 35.0k | 21 | 51.8k | 1m 41s | | compose_pr* | MiniMax-M2.7-highspeed | 1 | 121.3k | 1.9k | 354.3k | 29.8k | 25 | 15.0k | 1m 9s | | **Total** | | | 3.1M | 60.2k | 6.1M | 93.6k | | 295.1k | 30m 0s | \* *Step used a non-Anthropic provider; caching behavior may differ.* ## Token Efficiency | Step | LoC Changed | cache_read/LoC | cache_write/LoC | output/LoC | |------|-------------|----------------|-----------------|------------| | rectify | 0 | — | — | — | | dry_walkthrough | 0 | — | — | — | | implement | 383 | 5534.1 | 173.5 | 48.6 | | assess | 27 | 48170.0 | 2027.5 | 270.9 | | prepare_pr | 0 | — | — | — | | compose_pr | 0 | — | — | — | | **Total** | **410** | 14960.2 | 719.8 | 146.8 | ## Model Usage Breakdown | Model | steps | uncached | output | cache_read | cache_write | time | |-------|-------|----------|--------|------------|-------------|------| | claude-sonnet-4-6 | 1 | 3.2k | 18.0k | 666.7k | 50.6k | 7m 38s | | claude-opus-4-6 | 2 | 1.4k | 17.6k | 2.8M | 111.1k | 11m 0s | | MiniMax-M2.7-highspeed | 3 | 3.1M | 24.6k | 2.7M | 133.3k | 11m 21s | --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
RecipeIngredientschema has notypefield — every ingredient default is stored asstr | Nonewith no type semantics. The convention ofdefault: ""means "auto-detect from config" but only feeds the display layer (resolve_ingredient_defaults→build_ingredient_rows), not the runtime template engine. When${{ inputs.local_review_rounds }}is not explicitly supplied, the runtime receives""while the LLM sees"3"in the ingredients table. This creates a shadow default where the display-default and runtime-default are two separate values with no enforcement they agree. The fix adds atypefield toRecipeIngredientand a semantic rule that rejectstype: integer+default: ""at recipe load time, eliminating the ambiguity for all future recipe authors.Requirements
Expected Fix:
default: ""to an explicit integer default (e.g.,default: "3") that matches the config value, eliminating the LLM interpretation ambiguity.local_review_rounds(so reviews that resolve within the local budget still get GitHub visibility).default: ""should be disallowed for ingredients that map to integer config fields — a schema-level typing constraint.Changed Files
Modified (●):
src/autoskillit/recipe/io.pysrc/autoskillit/recipe/rules/rules_inputs.pysrc/autoskillit/recipe/schema.pysrc/autoskillit/recipes/implementation-groups.yamlsrc/autoskillit/recipes/implementation.yamlsrc/autoskillit/recipes/merge-prs.yamlsrc/autoskillit/recipes/remediation.yamltests/recipe/test_bundled_recipes_review_pr.pytests/recipe/test_io_parsing.pytests/recipe/test_merge_prs.pytests/recipe/test_rules_inputs.pytests/recipe/test_schema.pyImplementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260507-084045-358772/.autoskillit/temp/rectify/rectify_ingredient_type_enforcement_2026-05-07_090000.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary
* Step used a non-Anthropic provider; caching behavior may differ.
Token Efficiency
Model Usage Breakdown