diff --git a/src/autoskillit/recipe/rules/rules_loop_counter.py b/src/autoskillit/recipe/rules/rules_loop_counter.py index 8c2dbfdfa..05e045eae 100644 --- a/src/autoskillit/recipe/rules/rules_loop_counter.py +++ b/src/autoskillit/recipe/rules/rules_loop_counter.py @@ -197,6 +197,21 @@ def _check_loop_guard_before_verify(ctx: ValidationContext) -> list[RuleFinding] return findings +_WRAPPER_LOOP_EXEMPT_COUNTERS: frozenset[str] = frozenset( + { + "group_iteration_count", + } +) +"""Counters exempt from the cross-cycle reset requirement. + +These counters guard run-wide safety ceilings (e.g. group_iteration_count caps the +total number of recipe-group iterations across the entire pipeline run) and must +NOT be reset per audit-remediation cycle — doing so would defeat the safety +ceiling and allow indefinite repetition. Counter names are stable across +recipes; step names vary, so we key the exemption on counter variable rather +than step name.""" + + @semantic_rule( name="loop-counter-not-reset-on-outer-cycle", description=( @@ -204,7 +219,7 @@ def _check_loop_guard_before_verify(ctx: ValidationContext) -> list[RuleFinding] "from an outer check_loop_iteration guard's non-max_exceeded route " "without passing through a step that resets the inner counter" ), - severity=Severity.WARNING, + severity=Severity.ERROR, ) def _check_loop_counter_not_reset_on_outer_cycle(ctx: ValidationContext) -> list[RuleFinding]: """Detect temporal counter sharing across outer audit-remediation cycles. @@ -218,6 +233,12 @@ def _check_loop_counter_not_reset_on_outer_cycle(ctx: ValidationContext) -> list remediation cycle (audit_remediation_count) — other outer/inner guard relationships (e.g. merge_fix wrapping merge_rebase) have separate reset mechanisms and are out of scope for this rule. + + Bilateral cycle-membership: an inner guard is only considered in-scope + when it is BOTH forward-reachable from the outer guard's non-exit route + AND backward-reachable to the outer guard. This structurally excludes + post-audit terminal guards (CI watch, stall recovery, etc.) that lie + downstream of the audit-remediation cycle but cannot return to it. """ findings: list[RuleFinding] = [] recipe = ctx.recipe @@ -244,38 +265,39 @@ def _check_loop_counter_not_reset_on_outer_cycle(ctx: ValidationContext) -> list if not audit_outer_guards: return findings - reverse_graph: dict[str, set[str]] = {} - for src, targets in graph.items(): - for tgt in targets: - reverse_graph.setdefault(tgt, set()).add(src) - - for inner_name, inner_counter in guard_steps.items(): - if inner_name in audit_outer_guards: + for outer_name in audit_outer_guards: + outer_step = recipe.steps[outer_name] + if outer_step.on_result is None: continue - for outer_name in audit_outer_guards: - if inner_name == outer_name: + non_exit_target: str | None = None + for cond in outer_step.on_result.conditions: + if cond.when and "max_exceeded" in cond.when: continue + non_exit_target = cond.route + break - outer_step = recipe.steps[outer_name] - if outer_step.on_result is None: - continue + if non_exit_target is None or non_exit_target not in recipe.steps: + continue - non_exit_target: str | None = None - for cond in outer_step.on_result.conditions: - if cond.when and "max_exceeded" in cond.when: - continue - non_exit_target = cond.route - break + forward_reachable = bfs_reachable(graph, non_exit_target) + forward_reachable.add(non_exit_target) + cycle_candidates = bfs_reachable(ctx.predecessors, outer_name) - if non_exit_target is None or non_exit_target not in recipe.steps: + for inner_name, inner_counter in guard_steps.items(): + if inner_name in audit_outer_guards: + continue + + if inner_counter in _WRAPPER_LOOP_EXEMPT_COUNTERS: continue - forward_reachable = bfs_reachable(graph, non_exit_target) if inner_name not in forward_reachable: continue - backward_reachable = bfs_reachable(reverse_graph, inner_name) + if inner_name not in cycle_candidates: + continue + + backward_reachable = bfs_reachable(ctx.predecessors, inner_name) on_path = forward_reachable & backward_reachable on_path.add(non_exit_target) on_path.discard(inner_name) @@ -293,13 +315,14 @@ def _check_loop_counter_not_reset_on_outer_cycle(ctx: ValidationContext) -> list rule_name="loop-counter-not-reset-on-outer-cycle", step_name=inner_name, message=( - f"Inner guard '{inner_name}' uses counter '{inner_counter}' " - f"but is reachable from audit-remediation guard " - f"'{outer_name}' via '{non_exit_target}' without a reset " - f"step. Add a step using " - f"'autoskillit.smoke_utils.init_counter' to capture " - f"'{inner_counter}' on this path so each audit-remediation " - f"cycle gets a fresh budget." + f"Inner guard '{inner_name}' uses counter " + f"'{inner_counter}' but is reachable from " + f"audit-remediation guard '{outer_name}' via " + f"'{non_exit_target}' without a reset step. Add " + f"a step using " + f"'autoskillit.smoke_utils.init_counter' to " + f"capture '{inner_counter}' on this path so each " + f"audit-remediation cycle gets a fresh budget." ), ) ) diff --git a/src/autoskillit/recipes/diagrams/implementation-groups.md b/src/autoskillit/recipes/diagrams/implementation-groups.md index 5c7adb79c..3a25aca98 100644 --- a/src/autoskillit/recipes/diagrams/implementation-groups.md +++ b/src/autoskillit/recipes/diagrams/implementation-groups.md @@ -1,4 +1,4 @@ - + ## implementation-groups Group-based implementation with per-group plan/implement/test cycles and PR gates. @@ -18,7 +18,8 @@ group → plan → review_approach (optional) │ fix (on failure) → next_or_done └────┘ | -+-- audit_impl → reset_test_fix_counter → remediate (optional) ++-- audit_impl → reset_test_fix_counter → reset_merge_test_fix_counter + → reset_ref_push_counter → remediate (optional) | +-- [open-pr] (optional): | prepare_pr → compose_pr → review_pr → resolve_review diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index 9d1d1f2a0..a3c6ba26c 100644 --- a/src/autoskillit/recipes/diagrams/implementation.md +++ b/src/autoskillit/recipes/diagrams/implementation.md @@ -1,4 +1,4 @@ - + # implementation @@ -19,7 +19,8 @@ | +----+ | - +-- audit_impl → reset_test_fix_counter → remediate (optional) + +-- audit_impl → reset_test_fix_counter → reset_merge_test_fix_counter + → reset_ref_push_counter → remediate (optional) | +-- [open-pr] (optional): | prepare_pr → compose_pr → review_pr → resolve_review diff --git a/src/autoskillit/recipes/diagrams/remediation.md b/src/autoskillit/recipes/diagrams/remediation.md index 41711bd26..985f7fd04 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation Investigate, rectify, implement, and merge a bug fix with CI and PR gates. @@ -14,7 +14,8 @@ dry_walkthrough → implement ↔ [retry_worktree on context limit] | test → assess | -+-- audit_impl → reset_test_fix_counter → pre_remediation_merge → remediate (optional) ++-- audit_impl → reset_test_fix_counter → reset_merge_test_fix_counter + → reset_ref_push_counter → pre_remediation_merge → remediate (optional) | make_plan → commit_guard → merge → push | diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index 4767da291..d16db40f0 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -540,9 +540,35 @@ "capture": { "test_fix_loop_count": "${{ result.value }}" }, + "on_success": "reset_merge_test_fix_counter", + "on_failure": "reset_merge_test_fix_counter", + "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles." + }, + "reset_merge_test_fix_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "merge_test_fix_loop_count": "${{ result.value }}" + }, + "on_success": "reset_ref_push_counter", + "on_failure": "reset_ref_push_counter", + "note": "Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh merge-gate fix budget." + }, + "reset_ref_push_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "ref_push_count": "${{ result.value }}" + }, "on_success": "remediate", "on_failure": "remediate", - "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles." + "note": "Resets ref_push_count to 0 so each audit-remediation cycle gets a fresh ref-push budget." }, "commit_guard": { "tool": "run_python", diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 9539a13bb..0d27ae327 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -500,13 +500,39 @@ steps: counter_value: "" capture: test_fix_loop_count: "${{ result.value }}" - on_success: remediate - on_failure: remediate + on_success: reset_merge_test_fix_counter + on_failure: reset_merge_test_fix_counter note: >- Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles. + reset_merge_test_fix_counter: + tool: run_python + with: + callable: "autoskillit.smoke_utils.init_counter" + counter_value: "" + capture: + merge_test_fix_loop_count: "${{ result.value }}" + on_success: reset_ref_push_counter + on_failure: reset_ref_push_counter + note: >- + Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle + gets a fresh merge-gate fix budget. + + reset_ref_push_counter: + tool: run_python + with: + callable: "autoskillit.smoke_utils.init_counter" + counter_value: "" + capture: + ref_push_count: "${{ result.value }}" + on_success: remediate + on_failure: remediate + note: >- + Resets ref_push_count to 0 so each audit-remediation cycle gets a + fresh ref-push budget. + commit_guard: tool: run_python diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index 13dd3b3b4..78a08aae5 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -582,9 +582,35 @@ "capture": { "test_fix_loop_count": "${{ result.value }}" }, + "on_success": "reset_merge_test_fix_counter", + "on_failure": "reset_merge_test_fix_counter", + "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles." + }, + "reset_merge_test_fix_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "merge_test_fix_loop_count": "${{ result.value }}" + }, + "on_success": "reset_ref_push_counter", + "on_failure": "reset_ref_push_counter", + "note": "Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh merge-gate fix budget." + }, + "reset_ref_push_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "ref_push_count": "${{ result.value }}" + }, "on_success": "remediate", "on_failure": "remediate", - "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles." + "note": "Resets ref_push_count to 0 so each audit-remediation cycle gets a fresh ref-push budget." }, "commit_guard": { "tool": "run_python", diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index de1653106..fafe6819a 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -554,13 +554,39 @@ steps: counter_value: '' capture: test_fix_loop_count: ${{ result.value }} - on_success: remediate - on_failure: remediate + on_success: reset_merge_test_fix_counter + on_failure: reset_merge_test_fix_counter note: >- Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. The outer audit_remediation_count already bounds total cycles. + reset_merge_test_fix_counter: + tool: run_python + with: + callable: autoskillit.smoke_utils.init_counter + counter_value: '' + capture: + merge_test_fix_loop_count: ${{ result.value }} + on_success: reset_ref_push_counter + on_failure: reset_ref_push_counter + note: >- + Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle + gets a fresh merge-gate fix budget. + + reset_ref_push_counter: + tool: run_python + with: + callable: autoskillit.smoke_utils.init_counter + counter_value: '' + capture: + ref_push_count: ${{ result.value }} + on_success: remediate + on_failure: remediate + note: >- + Resets ref_push_count to 0 so each audit-remediation cycle gets a + fresh ref-push budget. + commit_guard: tool: run_python with: diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index 4118a233e..4635940ed 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -619,9 +619,35 @@ "capture": { "test_fix_loop_count": "${{ result.value }}" }, + "on_success": "reset_merge_test_fix_counter", + "on_failure": "reset_merge_test_fix_counter", + "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. Routes to pre_remediation_merge to preserve the worktree-merge step before starting a new remediation cycle." + }, + "reset_merge_test_fix_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "merge_test_fix_loop_count": "${{ result.value }}" + }, + "on_success": "reset_ref_push_counter", + "on_failure": "reset_ref_push_counter", + "note": "Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh merge-gate fix budget." + }, + "reset_ref_push_counter": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "" + }, + "capture": { + "ref_push_count": "${{ result.value }}" + }, "on_success": "pre_remediation_merge", "on_failure": "pre_remediation_merge", - "note": "Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. Routes to pre_remediation_merge to preserve the worktree-merge step before starting a new remediation cycle." + "note": "Resets ref_push_count to 0 so each audit-remediation cycle gets a fresh ref-push budget." }, "pre_remediation_merge": { "tool": "merge_worktree", diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 81057d812..843e8e0a0 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -594,13 +594,39 @@ steps: counter_value: '' capture: test_fix_loop_count: ${{ result.value }} - on_success: pre_remediation_merge - on_failure: pre_remediation_merge + on_success: reset_merge_test_fix_counter + on_failure: reset_merge_test_fix_counter note: >- Resets test_fix_loop_count to 0 so each audit-remediation cycle gets a fresh pre-merge fix budget. Routes to pre_remediation_merge to preserve the worktree-merge step before starting a new remediation cycle. + reset_merge_test_fix_counter: + tool: run_python + with: + callable: autoskillit.smoke_utils.init_counter + counter_value: '' + capture: + merge_test_fix_loop_count: ${{ result.value }} + on_success: reset_ref_push_counter + on_failure: reset_ref_push_counter + note: >- + Resets merge_test_fix_loop_count to 0 so each audit-remediation cycle + gets a fresh merge-gate fix budget. + + reset_ref_push_counter: + tool: run_python + with: + callable: autoskillit.smoke_utils.init_counter + counter_value: '' + capture: + ref_push_count: ${{ result.value }} + on_success: pre_remediation_merge + on_failure: pre_remediation_merge + note: >- + Resets ref_push_count to 0 so each audit-remediation cycle gets a + fresh ref-push budget. + pre_remediation_merge: tool: merge_worktree with: diff --git a/tests/recipe/test_rules_integration_predicate.py b/tests/recipe/test_rules_integration_predicate.py index 64f1f3df6..6a12e9669 100644 --- a/tests/recipe/test_rules_integration_predicate.py +++ b/tests/recipe/test_rules_integration_predicate.py @@ -30,6 +30,7 @@ class TestRecipeIntegrationPredicateRouting: def _load_recipes(self, request) -> None: request.cls.if_recipe = load_recipe(builtin_recipes_dir() / "remediation.yaml") request.cls.ip_recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + request.cls.ig_recipe = load_recipe(builtin_recipes_dir() / "implementation-groups.yaml") def test_investigate_first_merge_step_has_predicate_on_result(self) -> None: """The merge step in remediation.yaml has predicate on_result.""" @@ -130,11 +131,12 @@ def test_both_recipes_validate_cleanly(self) -> None: ip_errors = validate_recipe_structure(self.ip_recipe) assert ip_errors == [], f"implementation.yaml has validation errors: {ip_errors}" - def test_both_recipes_no_error_semantic_findings(self) -> None: - """Both recipes pass semantic rules with no ERROR-severity findings.""" + def test_all_recipes_no_error_semantic_findings(self) -> None: + """All bundled implementation-family recipes have no ERROR-severity findings.""" for recipe, name in [ (self.if_recipe, "remediation"), (self.ip_recipe, "implementation"), + (self.ig_recipe, "implementation-groups"), ]: findings = run_semantic_rules(recipe) errors = [f for f in findings if f.severity == Severity.ERROR] diff --git a/tests/recipe/test_rules_loop_counter.py b/tests/recipe/test_rules_loop_counter.py index cfec92ffb..10b8b7a48 100644 --- a/tests/recipe/test_rules_loop_counter.py +++ b/tests/recipe/test_rules_loop_counter.py @@ -1,4 +1,5 @@ -"""Tests for loop-counter-cross-path-sharing and loop-guard-before-verify semantic rules.""" +"""Tests for loop-counter-cross-path-sharing, loop-guard-before-verify, and +loop-counter-not-reset-on-outer-cycle semantic rules.""" from __future__ import annotations @@ -196,3 +197,104 @@ def test_bundled_recipes_no_guard_before_verify(self, recipe_name: str) -> None: findings = run_semantic_rules(recipe) gbv = [f for f in findings if f.rule == "loop-guard-before-verify"] assert gbv == [], f"{recipe_name}: {[(f.step_name, f.message) for f in gbv]}" + + +def _audit_outer_guard(counter_var: str, *, non_exit: str, exit_route: str) -> RecipeStep: + """A check_loop_iteration guard whose counter contains 'audit_remediation'.""" + return _guard_step(counter_var, non_exit=non_exit, exit_route=exit_route) + + +class TestLoopCounterNotResetOnOuterCycle: + def test_missing_reset_fires(self) -> None: + """Inner guard reachable from audit_remediation outer without reset step fires.""" + steps = { + "outer_guard": _audit_outer_guard( + "audit_remediation_count", non_exit="work", exit_route="done" + ), + "work": RecipeStep(tool="run_skill", on_success="inner_guard", on_failure="done"), + "inner_guard": _guard_step("fix_count", non_exit="fix", exit_route="done"), + "fix": RecipeStep(tool="run_skill", on_success="test", on_failure="done"), + "test": RecipeStep(tool="test_check", on_success="outer_guard", on_failure="fix"), + "done": RecipeStep(action="stop", message="Done. Emit sentinel: {}"), + } + recipe = _make_recipe(steps) + findings = run_semantic_rules(recipe) + rule_findings = [f for f in findings if f.rule == "loop-counter-not-reset-on-outer-cycle"] + assert len(rule_findings) == 1 + assert rule_findings[0].severity == Severity.ERROR + assert rule_findings[0].step_name == "inner_guard" + + def test_reset_present_does_not_fire(self) -> None: + """A reset step on the path suppresses the finding.""" + steps = { + "outer_guard": _audit_outer_guard( + "audit_remediation_count", non_exit="reset_fix", exit_route="done" + ), + "reset_fix": RecipeStep( + tool="run_python", + with_args={ + "callable": "autoskillit.smoke_utils.init_counter", + "counter_value": "", + }, + capture={"fix_count": "${{ result.value }}"}, + on_success="work", + on_failure="work", + ), + "work": RecipeStep(tool="run_skill", on_success="inner_guard", on_failure="done"), + "inner_guard": _guard_step("fix_count", non_exit="fix", exit_route="done"), + "fix": RecipeStep(tool="run_skill", on_success="test", on_failure="done"), + "test": RecipeStep(tool="test_check", on_success="outer_guard", on_failure="fix"), + "done": RecipeStep(action="stop", message="Done. Emit sentinel: {}"), + } + recipe = _make_recipe(steps) + findings = run_semantic_rules(recipe) + rule_findings = [f for f in findings if f.rule == "loop-counter-not-reset-on-outer-cycle"] + assert rule_findings == [] + + def test_post_audit_terminal_guard_excluded(self) -> None: + """A guard downstream with no path back to outer is excluded by bilateral check.""" + steps = { + "outer_guard": _audit_outer_guard( + "audit_remediation_count", non_exit="work", exit_route="done" + ), + "work": RecipeStep(tool="run_skill", on_success="push", on_failure="done"), + "push": RecipeStep(tool="push_to_remote", on_success="ci_guard", on_failure="done"), + "ci_guard": _guard_step("ci_count", non_exit="ci_watch", exit_route="done"), + "ci_watch": RecipeStep(action="stop", message="Done. Emit sentinel: {}"), + "done": RecipeStep(action="stop", message="Done. Emit sentinel: {}"), + } + recipe = _make_recipe(steps) + findings = run_semantic_rules(recipe) + rule_findings = [f for f in findings if f.rule == "loop-counter-not-reset-on-outer-cycle"] + assert rule_findings == [] + + def test_wrapper_loop_exempt_counter_excluded(self) -> None: + """group_iteration_count counter is exempt even when in the bilateral cycle.""" + steps = { + "outer_guard": _audit_outer_guard( + "audit_remediation_count", non_exit="work", exit_route="done" + ), + "work": RecipeStep(tool="run_skill", on_success="inner_guard", on_failure="done"), + "inner_guard": _guard_step( + "group_iteration_count", non_exit="process", exit_route="done" + ), + "process": RecipeStep(tool="run_skill", on_success="test", on_failure="done"), + "test": RecipeStep(tool="test_check", on_success="outer_guard", on_failure="process"), + "done": RecipeStep(action="stop", message="Done. Emit sentinel: {}"), + } + recipe = _make_recipe(steps) + findings = run_semantic_rules(recipe) + rule_findings = [f for f in findings if f.rule == "loop-counter-not-reset-on-outer-cycle"] + assert rule_findings == [] + + @pytest.mark.parametrize( + "recipe_name", + ("remediation", "implementation", "implementation-groups"), + ) + def test_bundled_recipes_no_missing_reset(self, recipe_name: str) -> None: + recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + findings = run_semantic_rules(recipe) + rule_findings = [f for f in findings if f.rule == "loop-counter-not-reset-on-outer-cycle"] + assert rule_findings == [], ( + f"{recipe_name}: {[(f.step_name, f.message) for f in rule_findings]}" + )