diff --git a/src/autoskillit/core/types/_type_constants_registries.py b/src/autoskillit/core/types/_type_constants_registries.py index 87f77225f..3131887d7 100644 --- a/src/autoskillit/core/types/_type_constants_registries.py +++ b/src/autoskillit/core/types/_type_constants_registries.py @@ -191,14 +191,14 @@ class ResponseBackstopExemptionDef(NamedTuple): RESPONSE_BACKSTOP_EXEMPTION_REGISTRY: dict[str, ResponseBackstopExemptionDef] = { "load_recipe": ResponseBackstopExemptionDef( - max_chars=188_000, - max_utf8_bytes=188_000, - measurement_id="bundled-recipes-all-modes-2026-07-21/load-recipe", + max_chars=195_000, + max_utf8_bytes=195_000, + measurement_id="bundled-recipes-all-modes-2026-07-22/load-recipe", ), "open_kitchen": ResponseBackstopExemptionDef( - max_chars=188_000, - max_utf8_bytes=188_000, - measurement_id="bundled-recipes-all-modes-2026-07-21/open-kitchen", + max_chars=195_000, + max_utf8_bytes=195_000, + measurement_id="bundled-recipes-all-modes-2026-07-22/open-kitchen", ), } diff --git a/src/autoskillit/execution/headless/AGENTS.md b/src/autoskillit/execution/headless/AGENTS.md index fc5e641e3..ba925a8e3 100644 --- a/src/autoskillit/execution/headless/AGENTS.md +++ b/src/autoskillit/execution/headless/AGENTS.md @@ -11,7 +11,7 @@ Headless Claude session orchestration — command prep, subprocess invocation, r | `_headless_execute.py` | Subprocess core: `_execute_claude_headless()` — shared skill/fleet path | | `_headless_git.py` | Git LOC tracking: `_capture_git_head_sha()`, `_compute_loc_changed()` | | `_headless_path_tokens.py` | Path-token extraction and output-path validation from assistant messages | -| `_headless_recovery.py` | Session recovery: `_recover_from_separate_marker`, `_synthesize_from_write_artifacts` | +| `_headless_recovery.py` | Session recovery: `_recover_from_separate_marker`, `_synthesize_from_write_artifacts`, `_infer_enum_token_from_write_contract` (contract-aware enum-token inference) | | `_headless_result.py` | `SkillResult` construction: `_build_skill_result` (evidence/telemetry moved to `_headless_evidence.py`) | | `_headless_evidence.py` | Evidence computation (`_compute_write_evidence`, `_adapt_agent_result`), audit recording (`_capture_failure`, `_apply_budget_guard`), telemetry builders (`_build_session_telemetry`, `_build_error_path_telemetry`) | | `_headless_outcome.py` | Contract-field parser and outcome invariant evaluator for post-session adjudication | diff --git a/src/autoskillit/execution/headless/__init__.py b/src/autoskillit/execution/headless/__init__.py index 05f23495f..176605214 100644 --- a/src/autoskillit/execution/headless/__init__.py +++ b/src/autoskillit/execution/headless/__init__.py @@ -72,12 +72,15 @@ ) from autoskillit.execution.headless._headless_recovery import ( _CHANNEL_B_RECOVERABLE_SUBTYPES, # noqa: F401 + _ENUM_BINDING_RE, # noqa: F401 _NUDGE_TIMEOUT, # noqa: F401 _TOKEN_NAME_RE, # noqa: F401 _attempt_contract_nudge, # noqa: F401 _extract_missing_token_hints, # noqa: F401 + _infer_enum_token_from_write_contract, # noqa: F401 _is_path_capture_pattern, # noqa: F401 _merge_token_usage, # noqa: F401 + _parse_single_enum_binding, # noqa: F401 _recover_block_from_assistant_messages, # noqa: F401 _recover_from_separate_marker, # noqa: F401 _synthesize_from_write_artifacts, # noqa: F401 diff --git a/src/autoskillit/execution/headless/_headless_execute.py b/src/autoskillit/execution/headless/_headless_execute.py index dc3ab6e99..b67ef7372 100644 --- a/src/autoskillit/execution/headless/_headless_execute.py +++ b/src/autoskillit/execution/headless/_headless_execute.py @@ -467,6 +467,7 @@ async def _execute_claude_headless( provider_extras=provider_extras, retry_reason=skill_result.retry_reason, pty_override=pty_override, + skill_contract=skill_contract, ) if nudge_success is not None: skill_result = nudge_success diff --git a/src/autoskillit/execution/headless/_headless_recovery.py b/src/autoskillit/execution/headless/_headless_recovery.py index c60bd7761..cf67e383e 100644 --- a/src/autoskillit/execution/headless/_headless_recovery.py +++ b/src/autoskillit/execution/headless/_headless_recovery.py @@ -5,7 +5,7 @@ import dataclasses from collections.abc import Mapping, Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, NamedTuple import regex as re @@ -26,6 +26,7 @@ SubprocessResult, SubprocessRunner, ) + from autoskillit.recipe._contracts_types import SkillContract logger = get_logger(__name__) @@ -36,6 +37,11 @@ _TOKEN_NAME_RE: re.Pattern[str] = re.compile(r"^(\w+)") +# Matches "token = value" (or "token[ \t]*=[ \t]*value") write_expected_when patterns +# whose value segment is a bare literal — no alternation/character-class, i.e. not +# "(a|b)". This is the structural soundness gate for deterministic enum inference. +_ENUM_BINDING_RE: re.Pattern[str] = re.compile(r"^(\w+)(?:\[[^\]]*\]\*)?=(?:\[[^\]]*\]\*)?(\w+)$") + _NUDGE_TIMEOUT: float = 60.0 _CANONICAL_TO_LEGACY: dict[str, str | None] = { @@ -46,6 +52,20 @@ } +class _PathHint(NamedTuple): + """Missing path-capture token: dictate the exact observed write path.""" + + token: str + path: str + + +class _EnumHint(NamedTuple): + """Missing enum-typed token: ask the session to choose from allowed_values.""" + + token: str + allowed_values: tuple[str, ...] + + def _is_path_capture_pattern(pattern: str) -> str | None: """Return the token name if pattern is a path-capture pattern, else None. @@ -165,42 +185,169 @@ def _synthesize_from_write_artifacts( return dataclasses.replace(session, result=injected) +def _parse_single_enum_binding( + skill_contract: SkillContract | None, +) -> tuple[str, str] | None: + """Return (token_name, literal_value) iff write_expected_when soundly binds one enum value. + + Structural soundness gate — fires only when ALL hold: + - ``write_behavior == "conditional"`` + - exactly one ``write_expected_when`` pattern + - that pattern parses as ``token = literal`` with no alternation/character-class + in the value segment (rejects e.g. ``verdict[ \\t]*=[ \\t]*(a|b)``) + - the literal is a declared ``allowed_values`` member of the same-named output + + Encodes the SOUND class from the contract soundness audit structurally — no + skill-name allowlist — so any future contract with this shape is covered for free. + """ + if skill_contract is None: + return None + if skill_contract.write_behavior != "conditional": + return None + if len(skill_contract.write_expected_when) != 1: + return None + match = _ENUM_BINDING_RE.match(skill_contract.write_expected_when[0]) + if not match: + return None + token_name, literal_value = match.group(1), match.group(2) + for output in skill_contract.outputs: + if output.name == token_name: + if output.allowed_values and literal_value in output.allowed_values: + return (token_name, literal_value) + return None + return None + + +def _infer_enum_token_from_write_contract( + session: ClaudeSessionResult, + expected_output_patterns: Sequence[str], + skill_contract: SkillContract | None, + write_call_count: int, + file_changes: Sequence[str] = (), +) -> ClaudeSessionResult | None: + """Deterministically synthesize an enum-typed output token from write-contract evidence. + + Unlike ``_synthesize_from_write_artifacts`` (which fabricates a token the agent never + produced and is therefore gated to UNMONITORED-only), this derives the token from + evidence the agent DID observably produce: an emitted companion path-token line + (in a confirmed channel) whose extracted path exists on disk, combined with the + contract's own declared write-expected-when implication. Runs for all channels. + + Fires only when: (1) ``_parse_single_enum_binding`` finds a sound single binding; + (2) an expected pattern for that same token remains unsatisfied; (3) write evidence + exists; (4) a companion output typed ``file_path*``/``directory_path`` has a token + line present in ``session.result`` whose path(s) exist on disk. + """ + binding = _parse_single_enum_binding(skill_contract) + if binding is None: + return None + token_name, literal_value = binding + + if write_call_count == 0 and not file_changes: + return None + + normalized_result = _normalize_model_output(session.result) + target_unsatisfied = False + for pattern in expected_output_patterns: + m = _TOKEN_NAME_RE.match(pattern) + if not m or m.group(1) != token_name: + continue + if re.search(pattern, normalized_result): + return None # already satisfied — nothing to infer + target_unsatisfied = True + if not target_unsatisfied: + return None + + assert skill_contract is not None # narrowed by _parse_single_enum_binding above + companion_path = None + for output in skill_contract.outputs: + if output.name == token_name: + continue + if not (output.type.startswith("file_path") or output.type == "directory_path"): + continue + companion_match = re.search( + rf"^{re.escape(output.name)}\s*=\s*(.+)$", normalized_result, re.MULTILINE + ) + if not companion_match: + continue + value_str = companion_match.group(1).strip() + if output.type == "file_path_list": + candidate_paths = [p.strip() for p in value_str.split(",") if p.strip()] + else: + candidate_paths = [value_str] if value_str else [] + if candidate_paths and all(Path(p).is_file() or Path(p).is_dir() for p in candidate_paths): + companion_path = candidate_paths[-1] + break + if companion_path is None: + return None + + logger.info( + "enum_inference_applied", + field_name=token_name, + value=literal_value, + companion_path=companion_path, + ) + injected = f"{token_name} = {literal_value}\n" + session.result + return dataclasses.replace(session, result=injected) + + def _extract_missing_token_hints( stdout: str, expected_output_patterns: Sequence[str], result_parser: ResultParser, write_tool_names: frozenset[str], -) -> list[tuple[str, str]]: - """Extract (token_name, write_path) pairs for patterns missing from the result. - - Parses raw NDJSON stdout to find write tool_use file_path entries, - then matches them against path-capture patterns that are NOT satisfied in - the result text. Returns the hints needed to build the nudge prompt. + skill_contract: SkillContract | None = None, +) -> list[_PathHint | _EnumHint]: + """Extract missing-token hints for patterns missing from the result. + + Parses raw NDJSON stdout to find write tool_use file_path entries, then matches + them against path-capture patterns that are NOT satisfied in the result text + (producing ``_PathHint``). When ``skill_contract`` is provided, unsatisfied + patterns whose leading token names an enum-typed output (non-empty + ``allowed_values``) produce an ``_EnumHint`` instead. Returns the hints needed + to build the nudge prompt. """ try: session = result_parser.parse_stdout(stdout) except Exception: logger.warning("nudge_parse_stdout_failed", exc_info=True) return [] - hints: list[tuple[str, str]] = [] + hints: list[_PathHint | _EnumHint] = [] + contract_outputs_by_name = ( + {output.name: output for output in skill_contract.outputs} + if skill_contract is not None + else {} + ) + normalized_output = _normalize_model_output(session.output) for pattern in expected_output_patterns: token_name = _is_path_capture_pattern(pattern) - if not token_name: + if token_name: + # Skip if already satisfied + if re.search(pattern, normalized_output): + continue + # Collect absolute Write/Edit paths; use the LAST one (final deliverable). + # tool_uses and token_usage live in .raw (not promoted to typed attrs) because + # they vary by backend and are absent from non-Claude AgentSessionResult payloads. + candidate_paths = [ + t.get("file_path", "") + for t in session.raw.get("tool_uses", []) + if t.get("name") in write_tool_names and t.get("file_path", "").startswith("/") + ] + if candidate_paths: + hints.append(_PathHint(token_name, candidate_paths[-1])) continue - # Skip if already satisfied - if re.search(pattern, _normalize_model_output(session.output)): + + m = _TOKEN_NAME_RE.match(pattern) + if not m: continue - # Collect absolute Write/Edit paths; use the LAST one (final deliverable). - # tool_uses and token_usage live in .raw (not promoted to typed attrs) because - # they vary by backend and are absent from non-Claude AgentSessionResult payloads. - candidate_paths = [ - t.get("file_path", "") - for t in session.raw.get("tool_uses", []) - if t.get("name") in write_tool_names and t.get("file_path", "").startswith("/") - ] - if candidate_paths: - hints.append((token_name, candidate_paths[-1])) + enum_token = m.group(1) + output = contract_outputs_by_name.get(enum_token) + if output is None or not output.allowed_values: + continue + if re.search(pattern, normalized_output): + continue + hints.append(_EnumHint(enum_token, tuple(output.allowed_values))) return hints @@ -218,6 +365,7 @@ async def _attempt_contract_nudge( provider_extras: Mapping[str, str] | None = None, retry_reason: RetryReason = RetryReason.CONTRACT_RECOVERY, pty_override: bool | None = None, + skill_contract: SkillContract | None = None, ) -> SkillResult | None: """Attempt a lightweight resume nudge to recover missing structured output tokens. @@ -246,12 +394,31 @@ async def _attempt_contract_nudge( else: _write_tool_names = backend.write_tool_names() hints = _extract_missing_token_hints( - subprocess_result.stdout, expected_output_patterns, result_parser, _write_tool_names + subprocess_result.stdout, + expected_output_patterns, + result_parser, + _write_tool_names, + skill_contract=skill_contract, ) if not hints: logger.debug("nudge_skip_no_hints") return None - token_lines = "\n".join(f"{name} = {path}" for name, path in hints) + hint_lines: list[str] = [] + for hint in hints: + if isinstance(hint, _EnumHint): + logger.info( + "nudge_enum_hint", + field_name=hint.token, + allowed_values=list(hint.allowed_values), + ) + hint_lines.append( + f"Emit `{hint.token} = ` where is one of: " + f"{' | '.join(hint.allowed_values)} — choose the value matching " + "what you actually did." + ) + else: + hint_lines.append(f"{hint.token} = {hint.path}") + token_lines = "\n".join(hint_lines) prompt = ( "You completed your task and wrote the output file, but you omitted the " "required structured output token in your final text response.\n\n" diff --git a/src/autoskillit/execution/headless/_headless_result.py b/src/autoskillit/execution/headless/_headless_result.py index 408595a30..51b132144 100644 --- a/src/autoskillit/execution/headless/_headless_result.py +++ b/src/autoskillit/execution/headless/_headless_result.py @@ -51,6 +51,7 @@ ) from autoskillit.execution.headless._headless_recovery import ( _CHANNEL_B_RECOVERABLE_SUBTYPES, + _infer_enum_token_from_write_contract, _recover_block_from_assistant_messages, _recover_from_separate_marker, _synthesize_from_write_artifacts, @@ -595,6 +596,27 @@ def _build_skill_result( if artifact_recovered is not None: session = artifact_recovered + # Enum inference: contract-aware recovery for enum-typed outputs whose value is + # mechanically implied by observed write evidence (see _parse_single_enum_binding). + # Runs for ALL channel confirmations — unlike artifact-aware synthesis above, this + # derives the token from evidence the agent DID observably produce (emitted + # companion path-token line + file on disk), not fabricated evidence, so it is not + # gated to UNMONITORED sessions. + if ( + expected_output_patterns + and _has_write_evidence + and not _check_expected_patterns(session.result.strip(), expected_output_patterns) + ): + enum_inferred = _infer_enum_token_from_write_contract( + session, + list(expected_output_patterns), + skill_contract, + evidence.write_call_count, + file_changes=file_changes, + ) + if enum_inferred is not None: + session = enum_inferred + exit_code_is_terminal = backend.capabilities.exit_code_is_terminal outcome, retry_reason = _compute_outcome( session, diff --git a/src/autoskillit/hooks/formatters/_fmt_response_spill.py b/src/autoskillit/hooks/formatters/_fmt_response_spill.py index aa7523a17..46b3fa61d 100644 --- a/src/autoskillit/hooks/formatters/_fmt_response_spill.py +++ b/src/autoskillit/hooks/formatters/_fmt_response_spill.py @@ -47,14 +47,14 @@ ) _RESPONSE_BACKSTOP_EXEMPTION_REGISTRY = { "load_recipe": { - "max_chars": 188_000, - "max_utf8_bytes": 188_000, - "measurement_id": "bundled-recipes-all-modes-2026-07-21/load-recipe", + "max_chars": 195_000, + "max_utf8_bytes": 195_000, + "measurement_id": "bundled-recipes-all-modes-2026-07-22/load-recipe", }, "open_kitchen": { - "max_chars": 188_000, - "max_utf8_bytes": 188_000, - "measurement_id": "bundled-recipes-all-modes-2026-07-21/open-kitchen", + "max_chars": 195_000, + "max_utf8_bytes": 195_000, + "measurement_id": "bundled-recipes-all-modes-2026-07-22/open-kitchen", }, } _RESPONSE_BACKSTOP_EXEMPTION_REGISTRY_DIGEST = hashlib.sha256( diff --git a/src/autoskillit/recipe/__init__.py b/src/autoskillit/recipe/__init__.py index a1e5eb96f..faf171c20 100644 --- a/src/autoskillit/recipe/__init__.py +++ b/src/autoskillit/recipe/__init__.py @@ -128,6 +128,9 @@ from autoskillit.recipe.rules import ( # noqa: E402 F401 rules_commit_guard_regression_route as _rules_commit_guard_regression_route, ) +from autoskillit.recipe.rules import ( # noqa: E402 F401 + rules_contract_recovery as _rules_contract_recovery, +) from autoskillit.recipe.rules import rules_contracts as _rules_contracts # noqa: E402 F401 from autoskillit.recipe.rules import ( # noqa: E402 F401 rules_criterion_schema_drift as _rules_criterion_schema_drift, # noqa: F401 diff --git a/src/autoskillit/recipe/_cmd_rpc.py b/src/autoskillit/recipe/_cmd_rpc.py index 68a01812d..81714b81c 100644 --- a/src/autoskillit/recipe/_cmd_rpc.py +++ b/src/autoskillit/recipe/_cmd_rpc.py @@ -10,6 +10,7 @@ commit_guard, compute_branch, main_repo_guard, + verify_plan_artifacts, ) from autoskillit.recipe._cmd_rpc_issues import ( # noqa: F401 batch_create_issues, diff --git a/src/autoskillit/recipe/_cmd_rpc_guards.py b/src/autoskillit/recipe/_cmd_rpc_guards.py index 9e1119d30..e70a808c5 100644 --- a/src/autoskillit/recipe/_cmd_rpc_guards.py +++ b/src/autoskillit/recipe/_cmd_rpc_guards.py @@ -8,6 +8,8 @@ from enum import StrEnum from pathlib import Path +import regex as re + from autoskillit.core import atomic_write, get_logger, is_generated_path, run_git logger = get_logger(__name__) @@ -177,6 +179,55 @@ def main_repo_guard(clone_path: str) -> dict[str, str]: return {"cleaned": "true"} +def _normalize_plan_parts(plan_parts: str) -> list[str] | None: + """Parse a capture-sourced plan_parts rendering into a list of absolute paths. + + Accepts every plausible capture-substitution rendering: bracket-wrapped list + literals, newline- or comma-delimited path lists, with optional per-item + quoting. Returns None if any non-empty item is not an absolute path. + """ + s = plan_parts.strip() + if len(s) >= 2 and s[0] == "[" and s[-1] == "]": + s = s[1:-1] + items: list[str] = [] + for raw in re.split(r"[\n,]", s): + item = raw.strip() + if len(item) >= 2 and item[0] == item[-1] and item[0] in "\"'": + item = item[1:-1].strip() + if not item: + continue + if not item.startswith("/"): + return None + items.append(item) + return items + + +def verify_plan_artifacts(plan_parts: str) -> dict[str, str]: + """Deterministically verify captured plan_parts artifacts for context-limit salvage. + + Verdict is 'salvaged' iff the normalized plan_parts list is non-empty and + every listed path exists as a non-empty regular file; 'unsalvageable' + otherwise. On salvage, echoes the normalized plan_parts (newline-joined) and + the derived plan_path (first part) so downstream context is populated + identically to the plan step's success path. + """ + items = _normalize_plan_parts(plan_parts) + if not items: + return {"verdict": "unsalvageable"} + for item in items: + path = Path(item) + try: + if not path.is_file() or path.stat().st_size == 0: + return {"verdict": "unsalvageable"} + except OSError: + return {"verdict": "unsalvageable"} + return { + "verdict": "salvaged", + "plan_parts": "\n".join(items), + "plan_path": items[0], + } + + def _count_numstat_net(numstat_output: str) -> int: """Sum net line changes (insertions - deletions) from git diff --numstat output.""" total = 0 diff --git a/src/autoskillit/recipe/rules/AGENTS.md b/src/autoskillit/recipe/rules/AGENTS.md index e162a401b..8b3dbcc85 100644 --- a/src/autoskillit/recipe/rules/AGENTS.md +++ b/src/autoskillit/recipe/rules/AGENTS.md @@ -26,6 +26,7 @@ See each subdirectory's AGENTS.md for details. | `rules_clone.py` | Clone/push dataflow rules: missing remote URL, local-strategy capture | | `rules_cmd.py` | `run_cmd` echo-capture alignment; git remote command detection; bare git rebase without conflict routing detection; path-typed capture non-empty file guard detection; single-quoted shell expansion suppression detection | | `rules_commit_guard_regression_route.py` | `commit_guard` steps with non-empty `base_branch` must declare an `on_result` predicate that routes `regression_detected` to an escalation step | +| `rules_contract_recovery.py` | contract-recovery-requires-salvage-route (WARNING, staged for ERROR): run_skill steps invoking a skill whose contract can trigger retry_reason=contract_recovery must declare a distinct on_context_limit salvage route | | `rules_contracts.py` | Skill contract completeness rules | | `rules_criterion_schema_drift.py` | criterion-schema-drift: bundled manifests must use structured {text, type} detection_criteria | | `rules_failure_verdict_bypass.py` | Detects bypass routes from verdict-gated steps reaching success stop terminals | diff --git a/src/autoskillit/recipe/rules/rules_contract_recovery.py b/src/autoskillit/recipe/rules/rules_contract_recovery.py new file mode 100644 index 000000000..69a5db1fa --- /dev/null +++ b/src/autoskillit/recipe/rules/rules_contract_recovery.py @@ -0,0 +1,99 @@ +"""Contract-recovery salvage-route requirement rule. + +Derives a hard requirement directly from skill contract data (not a hand-maintained +site list): a run_skill step invoking a skill whose contract can trigger +``retry_reason=contract_recovery`` at runtime (non-empty ``expected_output_patterns`` +and not ``read_only``) must declare a distinct, non-decorative ``on_context_limit`` +salvage route. Without one, ``on_failure`` is used as the fallback, discarding a +completed-but-unparsed artifact instead of attempting salvage (issue #4305). + +Severity is WARNING, not ERROR, as a deliberate staged rollout: the contract-derived +eligibility predicate matches far more steps than the nine sites audited and fixed by +the prior part (any non-read-only skill with expected_output_patterns qualifies, which +is most write-capable skills, not just plan-producing ones). Promoting straight to ERROR +today would flip ``valid=False`` for roughly a dozen bundled recipes with pre-existing, +unremediated gaps — and this codebase's own governance +(``test_error_severity_rules_have_no_dispatch_ready_exemptions``) forbids giving ERROR +rules a dispatch-ready exemption to paper over that, by design: fix all recipes first, +then promote. See tests/recipe/test_bundled_recipes_behavioral_properties.py's +``_SALVAGE_ROUTE_SITES`` for the nine sites already fully remediated and covered as an +unconditional structural regression test. Once a follow-up part wires salvage routes for +the remaining flagged sites, promote this rule's severity to ERROR (one-line change). +""" + +from __future__ import annotations + +from autoskillit.core import SKILL_TOOLS, Severity, get_logger +from autoskillit.recipe._analysis import ValidationContext +from autoskillit.recipe.contracts import get_skill_contract, load_bundled_manifest +from autoskillit.recipe.registry import RuleFinding, make_finding, semantic_rule + +logger = get_logger(__name__) + + +@semantic_rule( + name="contract-recovery-requires-salvage-route", + description=( + "A run_skill step invoking a skill whose contract can trigger " + "retry_reason=contract_recovery at runtime (non-empty expected_output_patterns " + "and not read_only) must declare an on_context_limit salvage route distinct from " + "on_failure. This targets the same provably at-risk subset as the generic " + "WARNING-tier run-skill-missing-context-limit rule, narrowed to steps whose skill " + "contract can actually produce contract_recovery. Severity is WARNING pending a " + "follow-up remediation pass across the remaining flagged sites (see module " + "docstring); promote to ERROR once all bundled recipes are clean." + ), + severity=Severity.WARNING, +) +def _check_contract_recovery_requires_salvage_route(ctx: ValidationContext) -> list[RuleFinding]: + recipe = ctx.recipe + findings: list[RuleFinding] = [] + + try: + manifest = load_bundled_manifest() + except (FileNotFoundError, OSError, ValueError): + logger.warning("failed to load bundled manifest", exc_info=True) + return findings + + # Steps that are themselves on_context_limit targets are exempt — they ARE + # the recovery path and do not need to declare a recovery of their own. + context_limit_targets: set[str] = set() + for step in recipe.steps.values(): + if step.on_context_limit and step.on_context_limit not in ( + "escalate", + "release_issue_failure", + ): + context_limit_targets.add(step.on_context_limit) + + for step_name, step in recipe.steps.items(): + if step.tool not in SKILL_TOOLS: + continue + if step.action == "stop": + continue + if step_name in context_limit_targets: + continue + skill_name = step.skill_name + if not skill_name: + continue + contract = get_skill_contract(skill_name, manifest) + if contract is None: + continue + if contract.read_only or not contract.expected_output_patterns: + continue + if step.on_context_limit is not None and step.on_context_limit != step.on_failure: + continue + findings.append( + make_finding( + rule_name="contract-recovery-requires-salvage-route", + step_name=step_name, + message=( + f"Step '{step_name}' invokes '{skill_name}', whose contract can trigger " + f"retry_reason=contract_recovery (non-empty expected_output_patterns, not " + f"read_only), but declares no on_context_limit distinct from " + f"on_failure={step.on_failure!r}. A completed-but-unparsed artifact would " + f"be discarded instead of salvaged. Add a non-destructive salvage route — " + f"see remediation.yaml's make_plan -> salvage_plan for the pattern." + ), + ) + ) + return findings diff --git a/src/autoskillit/recipe/rules/rules_verdict.py b/src/autoskillit/recipe/rules/rules_verdict.py index 7b5a4d9d2..239e242e4 100644 --- a/src/autoskillit/recipe/rules/rules_verdict.py +++ b/src/autoskillit/recipe/rules/rules_verdict.py @@ -212,7 +212,8 @@ def _check_verdict_routing_asymmetry(ctx: ValidationContext) -> list[RuleFinding r"result\.(?P[\w]+)" # result. r"\s*\}?\}?" # optional closing braces from template form r"\s*==\s*" # equality operator - r"(?P'\w+'|\w+)" # balanced single-quoted or bare value + # quoted or bare value; stops before a trailing " and"/" or" + r"(?P'[^']+'|\w+(?:\s\w+)*?)(?=\s+(?:and|or)\b|\s*$)" ) diff --git a/src/autoskillit/recipe/skill_contracts.yaml b/src/autoskillit/recipe/skill_contracts.yaml index 21aed02c1..78606d18c 100644 --- a/src/autoskillit/recipe/skill_contracts.yaml +++ b/src/autoskillit/recipe/skill_contracts.yaml @@ -419,6 +419,7 @@ skills: outputs: - name: verdict type: string + allowed_values: [GO, "NO GO"] - name: remediation_path type: file_path - name: verified_verdict @@ -2465,6 +2466,7 @@ skills: type: file_path - name: verdict type: string + allowed_values: [validated] expected_output_patterns: - "validated_report_path[ \\t]*=[ \\t]*\\S+" - "verdict[ \\t]*=[ \\t]*\\S+" @@ -2483,6 +2485,7 @@ skills: type: file_path - name: verdict type: str + allowed_values: [validated] expected_output_patterns: - "validated_report_path[ \\t]*=[ \\t]*\\S+" - "verdict[ \\t]*=[ \\t]*\\S+" @@ -3016,6 +3019,19 @@ callable_contracts: - name: cleaned type: str allowed_values: ["false", "true", "force", "failed"] + autoskillit.recipe._cmd_rpc.verify_plan_artifacts: + inputs: + - name: plan_parts + type: str + required: true + outputs: + - name: verdict + type: str + allowed_values: ["salvaged", "unsalvageable"] + - name: plan_parts + type: str + - name: plan_path + type: str autoskillit.recipe._cmd_rpc.review_path_rebase: inputs: - name: work_dir diff --git a/src/autoskillit/recipes/diagrams/implementation-groups.md b/src/autoskillit/recipes/diagrams/implementation-groups.md index 03c23d286..3c9958224 100644 --- a/src/autoskillit/recipes/diagrams/implementation-groups.md +++ b/src/autoskillit/recipes/diagrams/implementation-groups.md @@ -1,4 +1,4 @@ - + ## implementation-groups diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index afa48ae3e..b9d1a0da0 100644 --- a/src/autoskillit/recipes/diagrams/implementation.md +++ b/src/autoskillit/recipes/diagrams/implementation.md @@ -1,4 +1,4 @@ - + ## implementation diff --git a/src/autoskillit/recipes/diagrams/merge-prs.md b/src/autoskillit/recipes/diagrams/merge-prs.md index 8815d135d..3211f9489 100644 --- a/src/autoskillit/recipes/diagrams/merge-prs.md +++ b/src/autoskillit/recipes/diagrams/merge-prs.md @@ -1,4 +1,4 @@ - + ## merge-prs Merge multiple PRs into an integration branch with conflict resolution and CI gates. diff --git a/src/autoskillit/recipes/diagrams/remediation.md b/src/autoskillit/recipes/diagrams/remediation.md index 654c4b3cf..3f936bed9 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation diff --git a/src/autoskillit/recipes/diagrams/research.md b/src/autoskillit/recipes/diagrams/research.md index 015bf3d2a..9a9251050 100644 --- a/src/autoskillit/recipes/diagrams/research.md +++ b/src/autoskillit/recipes/diagrams/research.md @@ -1,4 +1,4 @@ - + ## research diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index 930036e77..83ba7039d 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -252,8 +252,34 @@ ], "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", + "on_context_limit": "salvage_plan", "note": "Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md or single plan file. plan_parts context key contains the full ordered list of part files (sorted alphabetically). For a single-part plan, plan_parts has one entry equal to plan_path. GROUPS MODE: Replace the skill_command task with the current group's file path from context.group_files extracted from the group step's capture — do NOT read the file; pass the path directly to the skill. REMEDIATION MODE: context.remediation_path is available when the orchestrator re-enters plan from the remediate step; pass it inline in the skill_command rather than as a named with: key. When in remediation mode, also append audit_remediation_mode=true to the skill_command. ACCUMULATION: After each plan step execution in groups mode, the agent MUST append the captured plan_path to context.all_plan_paths using a comma separator instead of letting the capture block overwrite the accumulated value. For multi-group runs, the accumulated value looks like \"/path/groupA_plan.md,/path/groupB_plan.md\". When verdict=false_positive, do NOT overwrite all_plan_paths — preserve the existing accumulated value from prior plan executions. ISSUE CONTEXT: If inputs.issue_url is a non-empty string, pass it to the skill so the make-plan skill can call fetch_github_issue internally to get full issue content: \"/autoskillit:make-plan {group_path}\\n\\nGitHub Issue: {inputs.issue_url}\" ADVERSARIAL REVIEW LEVEL: If inputs.adversarial_review_level is a non-empty string, append it to the skill_command: \"/autoskillit:make-plan {group_path}\\n\\nadversarial_review_level={inputs.adversarial_review_level}\" VERDICT: The skill emits verdict=plan (normal) or verdict=false_positive (remediation finding is a false positive). false_positive routes to push.\n" }, + "salvage_plan": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "review_approach" + }, + { + "route": "release_issue_failure" + } + ], + "on_failure": "release_issue_failure", + "note": "Deterministic salvage for plan context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if plan succeeded; unsalvageable falls through to plan's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance.\n" + }, "review_approach": { "tool": "run_skill", "model": "", @@ -964,6 +990,10 @@ "when": "result.error", "route": "register_clone_failure" }, + { + "when": "${{ result.verdict }} == NO GO", + "route": "check_audit_remediation_loop" + }, { "route": "check_audit_remediation_loop" } diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 9e34afbe4..7a339b7c9 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -231,6 +231,7 @@ steps: route: push on_failure: release_issue_failure on_rate_limit: release_issue_failure + on_context_limit: salvage_plan note: > Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md or single plan file. plan_parts context key contains the full ordered list of part files (sorted alphabetically). @@ -257,6 +258,29 @@ steps: VERDICT: The skill emits verdict=plan (normal) or verdict=false_positive (remediation finding is a false positive). false_positive routes to push. + salvage_plan: + tool: run_python + with: + callable: "autoskillit.recipe._cmd_rpc.verify_plan_artifacts" + plan_parts: "${{ context.plan_parts }}" + capture: + plan_path: "${{ result.plan_path }}" + capture_list: + plan_parts: "${{ result.plan_parts }}" + retries: 0 + on_result: + - when: "${{ result.verdict }} == salvaged" + route: review_approach + - route: release_issue_failure + on_failure: release_issue_failure + note: > + Deterministic salvage for plan context-limit stumbles: verifies captured + plan_parts artifacts exist on disk. salvaged routes onward as if plan + succeeded; unsalvageable falls through to plan's original on_failure destination. + Substitute the captured ${{ context.plan_parts }} value into the plan_parts + argument verbatim, unmodified — do not reformat, quote, or re-derive it; the + callable's input grammar absorbs rendering variance. + review_approach: tool: run_skill model: "" @@ -860,6 +884,8 @@ steps: route: push - when: "result.error" route: register_clone_failure + - when: "${{ result.verdict }} == NO GO" + route: check_audit_remediation_loop - route: check_audit_remediation_loop on_failure: register_clone_failure on_context_limit: register_clone_failure diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index ec63b86ee..27d923136 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -250,8 +250,34 @@ ], "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", + "on_context_limit": "salvage_plan", "note": "Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md or single plan file. plan_parts context key contains the full ordered list of part files (sorted alphabetically). For a single-part plan, plan_parts has one entry equal to plan_path. REMEDIATION MODE: context.remediation_path is available when the orchestrator re-enters plan from the remediate step; pass it inline in the skill_command rather than as a named with: key. When in remediation mode, also append audit_remediation_mode=true to the skill_command. ACCUMULATION: all_plan_paths accumulates across any re-entries to the plan step (e.g. remediation). For a single-pass run, all_plan_paths equals plan_path (the capture block handles this automatically). When verdict=false_positive, do NOT overwrite all_plan_paths — preserve the existing accumulated value from prior plan executions. ISSUE CONTEXT: If inputs.issue_url is a non-empty string, append it to the skill_command so the make-plan skill can detect and fetch the issue itself: \"/autoskillit:make-plan {task}\\n\\nGitHub Issue: {inputs.issue_url}\" The skill detects the URL pattern and calls fetch_github_issue internally. ADVERSARIAL REVIEW LEVEL: If inputs.adversarial_review_level is a non-empty string, append it to the skill_command: \"/autoskillit:make-plan {task}\\n\\nadversarial_review_level={inputs.adversarial_review_level}\" VERDICT: The skill emits verdict=plan (normal) or verdict=false_positive (remediation finding is a false positive). false_positive skips implement and routes to check_has_commits.\n" }, + "salvage_plan": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "review_approach" + }, + { + "route": "release_issue_failure" + } + ], + "on_failure": "release_issue_failure", + "note": "Deterministic salvage for plan context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if plan succeeded; unsalvageable falls through to plan's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance." + }, "review_approach": { "tool": "run_skill", "model": "", @@ -1015,6 +1041,10 @@ "when": "result.error", "route": "register_clone_failure" }, + { + "when": "${{ result.verdict }} == NO GO", + "route": "check_audit_remediation_loop" + }, { "route": "check_audit_remediation_loop" } diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index 7ff9854ad..70774adde 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -252,6 +252,7 @@ steps: route: check_has_commits on_failure: release_issue_failure on_rate_limit: release_issue_failure + on_context_limit: salvage_plan note: 'Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md or single plan file. plan_parts context key contains the full ordered list of part files (sorted alphabetically). For a single-part plan, plan_parts has one @@ -273,6 +274,27 @@ steps: finding is a false positive). false_positive skips implement and routes to check_has_commits. ' + salvage_plan: + tool: run_python + with: + callable: autoskillit.recipe._cmd_rpc.verify_plan_artifacts + plan_parts: ${{ context.plan_parts }} + capture: + plan_path: ${{ result.plan_path }} + capture_list: + plan_parts: ${{ result.plan_parts }} + retries: 0 + on_result: + - when: ${{ result.verdict }} == salvaged + route: review_approach + - route: release_issue_failure + on_failure: release_issue_failure + note: 'Deterministic salvage for plan context-limit stumbles: verifies captured + plan_parts artifacts exist on disk. salvaged routes onward as if plan succeeded; + unsalvageable falls through to plan''s original on_failure destination. Substitute + the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable''s input grammar + absorbs rendering variance.' review_approach: tool: run_skill model: '' @@ -921,6 +943,8 @@ steps: route: check_has_commits - when: result.error route: register_clone_failure + - when: ${{ result.verdict }} == NO GO + route: check_audit_remediation_loop - route: check_audit_remediation_loop on_failure: register_clone_failure on_context_limit: register_clone_failure diff --git a/src/autoskillit/recipes/merge-prs.json b/src/autoskillit/recipes/merge-prs.json index 8270aff65..559c9db1f 100644 --- a/src/autoskillit/recipes/merge-prs.json +++ b/src/autoskillit/recipes/merge-prs.json @@ -942,8 +942,34 @@ ], "on_failure": "register_clone_failure", "on_rate_limit": "register_clone_failure", + "on_context_limit": "salvage_plan", "note": "Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically into plan_parts[]. For a single-part plan, plan_parts has one entry equal to plan_path. PRIMARY PATH: context.task = conflict_report_path from merge_pr. REMEDIATION MODE: context.remediation_path is set when re-entering from the remediate step; pass it inline in the skill_command instead of context.task.\n" }, + "salvage_plan": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "pr_plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "verify" + }, + { + "route": "register_clone_failure" + } + ], + "on_failure": "register_clone_failure", + "note": "Deterministic salvage for plan context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if plan succeeded; unsalvageable falls through to plan's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance." + }, "verify": { "tool": "run_skill", "model": "", diff --git a/src/autoskillit/recipes/merge-prs.yaml b/src/autoskillit/recipes/merge-prs.yaml index 5a26e35e8..b65d74f56 100644 --- a/src/autoskillit/recipes/merge-prs.yaml +++ b/src/autoskillit/recipes/merge-prs.yaml @@ -898,6 +898,7 @@ steps: route: verify on_failure: register_clone_failure on_rate_limit: register_clone_failure + on_context_limit: salvage_plan note: 'Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically into plan_parts[]. For a single-part plan, plan_parts has one entry equal to plan_path. PRIMARY PATH: context.task = conflict_report_path from merge_pr. @@ -905,6 +906,27 @@ steps: remediate step; pass it inline in the skill_command instead of context.task. ' + salvage_plan: + tool: run_python + with: + callable: autoskillit.recipe._cmd_rpc.verify_plan_artifacts + plan_parts: ${{ context.plan_parts }} + capture: + pr_plan_path: ${{ result.plan_path }} + capture_list: + plan_parts: ${{ result.plan_parts }} + retries: 0 + on_result: + - when: ${{ result.verdict }} == salvaged + route: verify + - route: register_clone_failure + on_failure: register_clone_failure + note: 'Deterministic salvage for plan context-limit stumbles: verifies captured + plan_parts artifacts exist on disk. salvaged routes onward as if plan succeeded; + unsalvageable falls through to plan''s original on_failure destination. Substitute + the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable''s input grammar + absorbs rendering variance.' verify: tool: run_skill model: '' diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index f8a693ccb..77361800a 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -284,8 +284,34 @@ "on_success": "review_approach", "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", + "on_context_limit": "salvage_rectify_plan", "note": "Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically into plan_parts[].\nACCUMULATION: all_plan_paths accumulates across any re-entries to the rectify step. On re-entry, the agent MUST append the captured plan_path to context.all_plan_paths using a comma separator instead of letting the capture block overwrite the accumulated value. When verdict=false_positive, do NOT overwrite all_plan_paths — preserve the existing accumulated value from prior plan executions.\n" }, + "salvage_rectify_plan": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "review_approach" + }, + { + "route": "release_issue_failure" + } + ], + "on_failure": "release_issue_failure", + "note": "Deterministic salvage for rectify context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if rectify succeeded; unsalvageable falls through to rectify's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance." + }, "review_approach": { "tool": "run_skill", "model": "", @@ -322,11 +348,34 @@ "on_failure": "release_issue_failure", "retries": 3, "on_exhausted": "release_issue_failure", + "on_context_limit": "retry_walkthrough", "optional_context_refs": [ "remediation_path" ], "note": "Dry walkthrough retries on failure. It never routes back to a previous step." }, + "retry_walkthrough": { + "tool": "run_skill", + "model": "", + "with": { + "skill_command": "/autoskillit:dry-walkthrough ${{ context.plan_path }} remediation_path=${{ context.remediation_path }}", + "cwd": "${{ context.work_dir }}", + "output_dir": "{{AUTOSKILLIT_TEMP}}", + "review_path": "${{ context.review_path }}", + "issue_url": "${{ inputs.issue_url }}", + "remediation_path": "${{ context.remediation_path }}", + "step_name": "retry_walkthrough" + }, + "retries": 0, + "on_success": "create_impl_worktree", + "on_failure": "release_issue_failure", + "on_context_limit": "release_issue_failure", + "on_rate_limit": "release_issue_failure", + "optional_context_refs": [ + "remediation_path" + ], + "note": "Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble on the primary dry_walkthrough step. Never routes back to dry_walkthrough — on_failure and on_context_limit both fall through to the original failure destination." + }, "create_impl_worktree": { "tool": "run_cmd", "with": { @@ -1055,12 +1104,16 @@ "when": "result.error", "route": "register_clone_failure" }, + { + "when": "${{ result.verdict }} == NO GO", + "route": "check_audit_remediation_loop" + }, { "route": "check_audit_remediation_loop" } ], "on_failure": "register_clone_failure", - "on_context_limit": "register_clone_failure", + "on_context_limit": "check_audit_remediation_loop", "on_rate_limit": "check_audit_remediation_loop", "optional": true, "skip_when_false": "inputs.audit_impl", @@ -1108,8 +1161,34 @@ ], "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", + "on_context_limit": "salvage_plan", "note": "ADVERSARIAL REVIEW LEVEL: If inputs.adversarial_review_level is a non-empty string, append it to the skill_command: \"/autoskillit:make-plan {task}\\n\\nadversarial_review_level={inputs.adversarial_review_level}\" AUDIT REMEDIATION MODE: This step is always in remediation context (invoked from the remediate route step). Always append audit_remediation_mode=true to the skill_command at runtime: \"/autoskillit:make-plan {remediation_path}\\n\\naudit_remediation_mode=true\" VERDICT: The skill emits verdict=plan (produce implementation plan) or verdict=false_positive (audit finding is a false positive, skip implement). false_positive routes to check_has_commits. ACCUMULATION: all_plan_paths accumulates across any re-entries to the make_plan step. On re-entry, the agent MUST append the captured plan_path to context.all_plan_paths using a comma separator instead of letting the capture block overwrite the accumulated value. When verdict=false_positive, do NOT overwrite all_plan_paths — preserve the existing accumulated value from prior plan executions.\n" }, + "salvage_plan": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "dry_walkthrough" + }, + { + "route": "release_issue_failure" + } + ], + "on_failure": "release_issue_failure", + "note": "Deterministic salvage for make_plan context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if make_plan succeeded; unsalvageable falls through to make_plan's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance." + }, "commit_guard": { "tool": "run_python", "with": { diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 3ebaba6dc..792bf0c58 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -303,6 +303,7 @@ steps: on_success: review_approach on_failure: release_issue_failure on_rate_limit: release_issue_failure + on_context_limit: salvage_rectify_plan note: 'Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically into plan_parts[]. @@ -313,6 +314,27 @@ steps: preserve the existing accumulated value from prior plan executions. ' + salvage_rectify_plan: + tool: run_python + with: + callable: autoskillit.recipe._cmd_rpc.verify_plan_artifacts + plan_parts: ${{ context.plan_parts }} + capture: + plan_path: ${{ result.plan_path }} + capture_list: + plan_parts: ${{ result.plan_parts }} + retries: 0 + on_result: + - when: ${{ result.verdict }} == salvaged + route: review_approach + - route: release_issue_failure + on_failure: release_issue_failure + note: 'Deterministic salvage for rectify context-limit stumbles: verifies captured + plan_parts artifacts exist on disk. salvaged routes onward as if rectify succeeded; + unsalvageable falls through to rectify''s original on_failure destination. Substitute + the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable''s input grammar + absorbs rendering variance.' review_approach: tool: run_skill model: '' @@ -346,9 +368,33 @@ steps: on_failure: release_issue_failure retries: 3 on_exhausted: release_issue_failure + on_context_limit: retry_walkthrough optional_context_refs: - remediation_path note: Dry walkthrough retries on failure. It never routes back to a previous step. + retry_walkthrough: + tool: run_skill + model: '' + with: + skill_command: /autoskillit:dry-walkthrough ${{ context.plan_path }} remediation_path=${{ + context.remediation_path }} + cwd: ${{ context.work_dir }} + output_dir: '{{AUTOSKILLIT_TEMP}}' + review_path: ${{ context.review_path }} + issue_url: ${{ inputs.issue_url }} + remediation_path: ${{ context.remediation_path }} + step_name: retry_walkthrough + retries: 0 + on_success: create_impl_worktree + on_failure: release_issue_failure + on_context_limit: release_issue_failure + on_rate_limit: release_issue_failure + optional_context_refs: + - remediation_path + note: Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble + on the primary dry_walkthrough step. Never routes back to dry_walkthrough — + on_failure and on_context_limit both fall through to the original failure + destination. create_impl_worktree: tool: run_cmd with: @@ -967,9 +1013,11 @@ steps: route: commit_guard - when: result.error route: register_clone_failure + - when: ${{ result.verdict }} == NO GO + route: check_audit_remediation_loop - route: check_audit_remediation_loop on_failure: register_clone_failure - on_context_limit: register_clone_failure + on_context_limit: check_audit_remediation_loop on_rate_limit: check_audit_remediation_loop optional: true skip_when_false: inputs.audit_impl @@ -1006,6 +1054,7 @@ steps: route: check_has_commits on_failure: release_issue_failure on_rate_limit: release_issue_failure + on_context_limit: salvage_plan note: 'ADVERSARIAL REVIEW LEVEL: If inputs.adversarial_review_level is a non-empty string, append it to the skill_command: "/autoskillit:make-plan {task}\n\nadversarial_review_level={inputs.adversarial_review_level}" AUDIT REMEDIATION MODE: This step is always in remediation context (invoked @@ -1020,6 +1069,27 @@ steps: all_plan_paths — preserve the existing accumulated value from prior plan executions. ' + salvage_plan: + tool: run_python + with: + callable: autoskillit.recipe._cmd_rpc.verify_plan_artifacts + plan_parts: ${{ context.plan_parts }} + capture: + plan_path: ${{ result.plan_path }} + capture_list: + plan_parts: ${{ result.plan_parts }} + retries: 0 + on_result: + - when: ${{ result.verdict }} == salvaged + route: dry_walkthrough + - route: release_issue_failure + on_failure: release_issue_failure + note: 'Deterministic salvage for make_plan context-limit stumbles: verifies captured + plan_parts artifacts exist on disk. salvaged routes onward as if make_plan succeeded; + unsalvageable falls through to make_plan''s original on_failure destination. Substitute + the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable''s input grammar + absorbs rendering variance.' commit_guard: tool: run_python with: diff --git a/src/autoskillit/recipes/research-implement.json b/src/autoskillit/recipes/research-implement.json index 4bd893fb9..f93620c49 100644 --- a/src/autoskillit/recipes/research-implement.json +++ b/src/autoskillit/recipes/research-implement.json @@ -198,8 +198,34 @@ } ], "on_failure": "escalate_stop", + "on_context_limit": "salvage_plan_phase", "note": "Plans the current setup phase. Replace ${{ context.group_files }} in the skill_command with the FIRST REMAINING phase file path from the list — do NOT pass the entire list. Pass the path directly to the skill (do not read it). Glob plan_dir for *_part_*.md or single plan file. ITERATION: After each implement_phase cycle, advance to the next entry in context.group_files for the following plan_phase iteration. REMEDIATION MODE: context.remediation_path is available when the orchestrator re-enters plan_phase from the remediate step via check_audit_retry_loop; pass it inline in the skill_command rather than as a named with: key.\n" }, + "salvage_plan_phase": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "phase_plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "implement_phase" + }, + { + "route": "escalate_stop" + } + ], + "on_failure": "escalate_stop", + "note": "Deterministic salvage for plan_phase context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if plan_phase succeeded; unsalvageable falls through to plan_phase's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance.\n" + }, "implement_phase": { "tool": "run_skill", "model": "", @@ -379,6 +405,10 @@ "when": "result.error", "route": "escalate_stop" }, + { + "when": "${{ result.verdict }} == NO GO", + "route": "remediate" + }, { "route": "remediate" } diff --git a/src/autoskillit/recipes/research-implement.yaml b/src/autoskillit/recipes/research-implement.yaml index 6352c00fe..790bd53cc 100644 --- a/src/autoskillit/recipes/research-implement.yaml +++ b/src/autoskillit/recipes/research-implement.yaml @@ -205,6 +205,7 @@ steps: - when: "${{ result.verdict }} == false_positive" route: implement_phase on_failure: escalate_stop + on_context_limit: salvage_plan_phase note: > Plans the current setup phase. Replace ${{ context.group_files }} in the skill_command with the FIRST REMAINING phase file path from the list — @@ -216,6 +217,30 @@ steps: re-enters plan_phase from the remediate step via check_audit_retry_loop; pass it inline in the skill_command rather than as a named with: key. + salvage_plan_phase: + tool: run_python + with: + callable: "autoskillit.recipe._cmd_rpc.verify_plan_artifacts" + plan_parts: "${{ context.plan_parts }}" + capture: + phase_plan_path: "${{ result.plan_path }}" + capture_list: + plan_parts: "${{ result.plan_parts }}" + retries: 0 + on_result: + - when: "${{ result.verdict }} == salvaged" + route: implement_phase + - route: escalate_stop + on_failure: escalate_stop + note: > + Deterministic salvage for plan_phase context-limit stumbles: verifies + captured plan_parts artifacts exist on disk. salvaged routes onward as + if plan_phase succeeded; unsalvageable falls through to plan_phase's + original on_failure destination. Substitute the captured + ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable's + input grammar absorbs rendering variance. + implement_phase: tool: run_skill model: "" @@ -370,6 +395,8 @@ steps: route: run_experiment - when: "result.error" route: escalate_stop + - when: "${{ result.verdict }} == NO GO" + route: remediate - route: remediate on_failure: escalate_stop on_context_limit: escalate_stop diff --git a/src/autoskillit/recipes/research.json b/src/autoskillit/recipes/research.json index 16e276b7a..16d2a041d 100644 --- a/src/autoskillit/recipes/research.json +++ b/src/autoskillit/recipes/research.json @@ -519,8 +519,34 @@ } ], "on_failure": "escalate_stop", + "on_context_limit": "salvage_plan_phase", "note": "Plans the current setup phase. Replace ${{ context.group_files }} in the skill_command with the FIRST REMAINING phase file path from the list — do NOT pass the entire list. Pass the path directly to the skill (do not read it). Glob plan_dir for *_part_*.md or single plan file. ITERATION: After each implement_phase cycle, advance to the next entry in context.group_files for the following plan_phase iteration.\n" }, + "salvage_plan_phase": { + "tool": "run_python", + "with": { + "callable": "autoskillit.recipe._cmd_rpc.verify_plan_artifacts", + "plan_parts": "${{ context.plan_parts }}" + }, + "capture": { + "phase_plan_path": "${{ result.plan_path }}" + }, + "capture_list": { + "plan_parts": "${{ result.plan_parts }}" + }, + "retries": 0, + "on_result": [ + { + "when": "${{ result.verdict }} == salvaged", + "route": "implement_phase" + }, + { + "route": "escalate_stop" + } + ], + "on_failure": "escalate_stop", + "note": "Deterministic salvage for plan_phase context-limit stumbles: verifies captured plan_parts artifacts exist on disk. salvaged routes onward as if plan_phase succeeded; unsalvageable falls through to plan_phase's original on_failure destination. Substitute the captured ${{ context.plan_parts }} value into the plan_parts argument verbatim, unmodified — do not reformat, quote, or re-derive it; the callable's input grammar absorbs rendering variance.\n" + }, "implement_phase": { "tool": "run_skill", "model": "", @@ -700,6 +726,10 @@ "when": "result.error", "route": "escalate_stop" }, + { + "when": "${{ result.verdict }} == NO GO", + "route": "remediate" + }, { "route": "remediate" } diff --git a/src/autoskillit/recipes/research.yaml b/src/autoskillit/recipes/research.yaml index 56bdf921e..e73bce1c2 100644 --- a/src/autoskillit/recipes/research.yaml +++ b/src/autoskillit/recipes/research.yaml @@ -517,6 +517,7 @@ steps: - when: "${{ result.verdict }} == false_positive" route: implement_phase on_failure: escalate_stop + on_context_limit: salvage_plan_phase note: > Plans the current setup phase. Replace ${{ context.group_files }} in the skill_command with the FIRST REMAINING phase file path from the list — @@ -525,6 +526,30 @@ steps: ITERATION: After each implement_phase cycle, advance to the next entry in context.group_files for the following plan_phase iteration. + salvage_plan_phase: + tool: run_python + with: + callable: "autoskillit.recipe._cmd_rpc.verify_plan_artifacts" + plan_parts: "${{ context.plan_parts }}" + capture: + phase_plan_path: "${{ result.plan_path }}" + capture_list: + plan_parts: "${{ result.plan_parts }}" + retries: 0 + on_result: + - when: "${{ result.verdict }} == salvaged" + route: implement_phase + - route: escalate_stop + on_failure: escalate_stop + note: > + Deterministic salvage for plan_phase context-limit stumbles: verifies + captured plan_parts artifacts exist on disk. salvaged routes onward as + if plan_phase succeeded; unsalvageable falls through to plan_phase's + original on_failure destination. Substitute the captured + ${{ context.plan_parts }} value into the plan_parts argument verbatim, + unmodified — do not reformat, quote, or re-derive it; the callable's + input grammar absorbs rendering variance. + implement_phase: tool: run_skill model: "" @@ -679,6 +704,8 @@ steps: route: run_experiment - when: "result.error" route: escalate_stop + - when: "${{ result.verdict }} == NO GO" + route: remediate - route: remediate on_failure: escalate_stop on_context_limit: escalate_stop diff --git a/src/autoskillit/server/tools/_serve_helpers.py b/src/autoskillit/server/tools/_serve_helpers.py index 711c4a216..a2ae60995 100644 --- a/src/autoskillit/server/tools/_serve_helpers.py +++ b/src/autoskillit/server/tools/_serve_helpers.py @@ -256,7 +256,10 @@ def extract_step_skeleton( ``load_and_validate``'s ``post_prune_step_names`` result field). *routing_edges_by_step* — name → list of (edge_type, target) tuples derived from ``_extract_routing_edges`` for each step. - *step_summaries* — optional name → one-line summary override. + *step_summaries* — optional name → one-line summary override. A step + with no summary (or an empty one) omits the ``summary`` key + entirely rather than emitting it as ``""`` — saves bytes at scale + and matches the byte_range fail-open/omit convention below. *byte_ranges* — optional name → (start, end) UTF-8 byte offsets within the persisted ``content`` field. When provided for a given step, the skeleton's per-step entry gains a @@ -270,11 +273,12 @@ def extract_step_skeleton( summary = (step_summaries or {}).get(name) or "" entry: dict[str, Any] = { "name": name, - "summary": summary, "edges": [ {"type": edge_type, "target": target} for edge_type, target in edges if target ], } + if summary: + entry["summary"] = summary span = (byte_ranges or {}).get(name) if span is not None: entry["byte_range"] = list(span) diff --git a/src/autoskillit/skills_extended/rectify/SKILL.md b/src/autoskillit/skills_extended/rectify/SKILL.md index 981e35731..1377f5e77 100644 --- a/src/autoskillit/skills_extended/rectify/SKILL.md +++ b/src/autoskillit/skills_extended/rectify/SKILL.md @@ -78,6 +78,19 @@ Do not change any code. - The solution must solve more than just the immediate issue - The plan must cover every remediation item enumerated in the source issue; if an item cannot be delivered, stop and surface it — do not descope it in the plan +## Context Limit Behavior + +When context is exhausted mid-execution, the plan file may already be written to disk +even though the `plan_path`/`plan_parts` token was never emitted. The recipe routes to +`on_context_limit` (a deterministic salvage step), which checks whether the captured +`plan_parts` paths exist as non-empty files on disk. If they do, the pipeline continues +as though this skill had succeeded; if not, it falls through to this skill's original +`on_failure` destination. + +This skill writes only new plan files under `{{AUTOSKILLIT_TEMP}}/rectify/` (never +modifies source code), so a context-limit stumble has no blast radius beyond an +unclaimed plan file. + ## Rectify Workflow ### Step 1: Identify the Investigation Context diff --git a/tests/_test_filter.py b/tests/_test_filter.py index a385c1409..243a35018 100644 --- a/tests/_test_filter.py +++ b/tests/_test_filter.py @@ -800,6 +800,7 @@ class ImportContext(enum.StrEnum): "execution/test_zero_write_detection.py", "execution/test_smoke_codex.py", "execution/test_outcome_invariants.py", + "execution/test_headless_enum_recovery.py", # Fleet file-level entries (9 of N import autoskillit.recipe): "fleet/test_fleet_e2e.py", "fleet/test_fleet_e2e_codex.py", diff --git a/tests/arch/AGENTS.md b/tests/arch/AGENTS.md index 4f0c3542f..3dee0ee30 100644 --- a/tests/arch/AGENTS.md +++ b/tests/arch/AGENTS.md @@ -83,7 +83,7 @@ AST enforcement, sub-package layer contracts, and architectural invariant tests. | `test_recipe_contract_freshness.py` | Parametrized JSON card freshness enforcement: contract cards must have non-stale .json companions with content parity | | `test_recipe_rule_registration.py` | REQ-RECIPE-001: every recipe/rules_*.py file must be imported by recipe/__init__.py | | `test_rule_severity_consistency.py` | AST guards: rule functions must use make_finding()/make_block_finding(); RuleFinding severity must match @semantic_rule decorator severity; _KNOWN_NON_CONFORMING_RULES entries must carry tracking comments | -| `test_xfail_bridge_policy.py` | xfail(strict=True) bridge governance: reasons must cite tracking issue (#NNNN); size-capped exemption registry with rationale enforcement | +| `test_xfail_bridge_policy.py` | xfail bridge governance: reasons must cite tracking issue (#NNNN) for both decorator-form (strict=True) and imperative (pytest.xfail(...) call-in-body) bridges; size-capped exemption registry with rationale enforcement | | `test_regex_guards.py` | Arch guard: keyword regexes in cmd-scanning rules must use path-safe lookbehind guards | | `test_regex_import.py` | Structural guard: src/ must use `import regex as re`, not bare `import re` (hooks/ exempt) | | `test_registry.py` | Symbolic rule registry tests (RuleDescriptor, RULES, Violation) | diff --git a/tests/arch/test_execution_source_split.py b/tests/arch/test_execution_source_split.py index 1da67a0bf..dc2743fed 100644 --- a/tests/arch/test_execution_source_split.py +++ b/tests/arch/test_execution_source_split.py @@ -17,12 +17,12 @@ "_headless_evidence.py", ] HEADLESS_SIZE_BUDGETS = { - "headless/__init__.py": 530, + "headless/__init__.py": 535, "headless/_headless_helpers.py": 220, "headless/_headless_execute.py": 635, - "headless/_headless_recovery.py": 370, + "headless/_headless_recovery.py": 540, "headless/_headless_path_tokens.py": 190, - "headless/_headless_result.py": 920, + "headless/_headless_result.py": 945, "headless/_headless_evidence.py": 310, } diff --git a/tests/arch/test_import_linter_contracts.py b/tests/arch/test_import_linter_contracts.py index 19813c70a..1fa9d84f6 100644 --- a/tests/arch/test_import_linter_contracts.py +++ b/tests/arch/test_import_linter_contracts.py @@ -37,6 +37,7 @@ "execution/headless/_headless_execute.py": frozenset({"pipeline", "recipe"}), "execution/headless/_headless_helpers.py": frozenset({"config"}), "execution/headless/_headless_outcome.py": frozenset({"recipe"}), + "execution/headless/_headless_recovery.py": frozenset({"recipe"}), "execution/headless/_headless_result.py": frozenset({"recipe"}), "execution/linux_tracing.py": frozenset({"config"}), "execution/process/__init__.py": frozenset({"config"}), diff --git a/tests/arch/test_layer_enforcement.py b/tests/arch/test_layer_enforcement.py index 583dec560..925874c91 100644 --- a/tests/arch/test_layer_enforcement.py +++ b/tests/arch/test_layer_enforcement.py @@ -1629,6 +1629,9 @@ def test_default_classes_only_instantiated_inside_factory_or_allowlist() -> None "tests/execution/test_outcome_invariants.py": frozenset({"autoskillit.recipe"}), # smoke composition tests validate recipe validity under codex backend — needs recipe API "tests/execution/test_smoke_codex.py": frozenset({"autoskillit.recipe"}), + # enum-token recovery tests build SkillContract instances inline to verify + # contract-aware inference — needs recipe API + "tests/execution/test_headless_enum_recovery.py": frozenset({"autoskillit.recipe"}), # quota tests cross into config to validate the contract between vocab constants # (execution layer) and config defaults — intentional, documented cross-ref "tests/execution/test_quota_binding.py": frozenset({"autoskillit.config"}), diff --git a/tests/arch/test_pyright_suppression_allowlist.py b/tests/arch/test_pyright_suppression_allowlist.py index 7adcb6b88..74e1d9d49 100644 --- a/tests/arch/test_pyright_suppression_allowlist.py +++ b/tests/arch/test_pyright_suppression_allowlist.py @@ -20,7 +20,7 @@ PRODUCTION_ALLOWLIST: dict[tuple[str, int], str] = { ( "recipe/__init__.py", - 283, + 286, ): "lazy-registry: method added by _register_rule_module() side effects", ("recipe/_api.py", 286): "lazy-registry: RULE_REGISTRY_HASH set by _finalize_registry()", } diff --git a/tests/arch/test_subpackage_isolation.py b/tests/arch/test_subpackage_isolation.py index 8ab6f62d4..2203f7bbd 100644 --- a/tests/arch/test_subpackage_isolation.py +++ b/tests/arch/test_subpackage_isolation.py @@ -749,6 +749,9 @@ def test_no_subpackage_exceeds_10_files() -> None: rules_gitignored_deliverable.py adds the gitignored-deliverable-in-plan rule flagging plan steps writing to gitignored paths that feed audit-impl, bringing the rules/ count to 37. + rules_contract_recovery.py adds the contract-recovery-requires-salvage-route + ERROR rule deriving on_context_limit salvage-route requirements from skill + contract capability (#4305 part C), bringing the rules/ count to 38. Exempt at 51 files. execution/ — REQ-CNST-003-E3: execution/ decomposes process lifecycle into focused single-concern modules (_process_io, _process_kill, _process_race, @@ -890,7 +893,7 @@ def test_no_subpackage_exceeds_10_files() -> None: "hooks": 18, # +recipe_confirmed_post_hook, +quota_guard_state_post_hook, +_policy_event, +shell_capture_hook (#4286), +_capture_cleanup.py # noqa: E501 "pipeline": 12, "fleet": 23, # +_issue_url_helpers.py # noqa: E501 - "recipe/rules": 54, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable +rules_issue_scope_threading +rules_inventory_gate_bilateral +rules_verdict_context # noqa: E501 + "recipe/rules": 55, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable +rules_issue_scope_threading +rules_inventory_gate_bilateral +rules_verdict_context +rules_contract_recovery # noqa: E501 "server/tools": 32, # +_pipeline_deps.py +_ordering_telemetry.py (open_kitchen # auto-init dependency tracker + REVIEW_BEFORE_PLAN ordering telemetry) # +_backend_compat.py (shared target-resolution + fail-closed compatibility gate diff --git a/tests/arch/test_xfail_bridge_policy.py b/tests/arch/test_xfail_bridge_policy.py index 44a55f053..bc36c055b 100644 --- a/tests/arch/test_xfail_bridge_policy.py +++ b/tests/arch/test_xfail_bridge_policy.py @@ -23,10 +23,12 @@ _XFAIL_POLICY_EXEMPT_FILES: frozenset[str] = frozenset( { "arch/test_recipe_diagram_freshness.py", # permanent: shrink meta-test + "arch/test_capability_consistency.py", # permanent: bounded known-violations set + "skills/test_skill_body_cleanliness.py", # permanent: bounded known-violators set } ) -_EXEMPT_CAP = 2 +_EXEMPT_CAP = 3 def _is_xfail_strict_true(node: ast.AST) -> bool: @@ -122,6 +124,114 @@ def test_strict_xfail_reasons_cite_tracking_issue() -> None: ) +def _collect_imperative_xfail_nodes(tree: ast.Module) -> list[ast.Call]: + """Find imperative ``pytest.xfail(...)``/``xfail(...)`` calls used as bare statements. + + Distinct AST shape from ``_collect_xfail_strict_true_nodes``: that collector visits + decorator and ``pytest.param(marks=...)`` sites, which carry a ``strict=`` keyword. + The imperative form raises immediately at call time, has no ``strict=`` keyword, and + was previously invisible to this policy — exactly the blind spot that hid an + untracked context-limit-compliance exemption (issue #4305). + """ + results: list[ast.Call] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Expr): + continue + call = node.value + if not isinstance(call, ast.Call): + continue + func = call.func + is_xfail = (isinstance(func, ast.Name) and func.id == "xfail") or ( + isinstance(func, ast.Attribute) and func.attr == "xfail" + ) + if is_xfail: + results.append(call) + return results + + +def _extract_imperative_reason(call: ast.Call) -> str | None: + """Extract the reason string from an imperative xfail call (positional or keyword).""" + arg = call.args[0] if call.args else None + if arg is None: + for kw in call.keywords: + if kw.arg == "reason": + arg = kw.value + break + if arg is None: + return None + if isinstance(arg, ast.Constant) and isinstance(arg.value, str): + return arg.value + if isinstance(arg, ast.JoinedStr): + parts = [ + v.value for v in arg.values if isinstance(v, ast.Constant) and isinstance(v.value, str) + ] + return "".join(parts) if parts else None + return None + + +def test_imperative_xfail_reasons_cite_tracking_issue() -> None: + """Every imperative pytest.xfail(...) call must contain a #NNNN issue reference.""" + violations: list[str] = [] + for py_file in sorted(_TESTS_ROOT.rglob("test_*.py")): + rel = py_file.relative_to(_TESTS_ROOT).as_posix() + if rel in _XFAIL_POLICY_EXEMPT_FILES: + continue + try: + tree = ast.parse(py_file.read_text()) + except (SyntaxError, UnicodeDecodeError, OSError): + continue + for call in _collect_imperative_xfail_nodes(tree): + reason = _extract_imperative_reason(call) + if reason is None: + violations.append( + f"{rel}:{call.lineno} — reason must be a string literal so policy " + "can be checked" + ) + elif not re.search(r"#\d+", reason): + violations.append( + f"{rel}:{call.lineno} — reason={reason!r} does not cite " + "a tracking issue; add #NNNN or resolve the defect and remove the xfail" + ) + assert not violations, "pytest.xfail(...) without tracking issue reference:\n" + "\n".join( + f" {v}" for v in violations + ) + + +def test_imperative_xfail_collector_flags_missing_citation() -> None: + """Synthetic AST: an imperative pytest.xfail() with no #NNNN citation is detected.""" + tree = ast.parse( + "import pytest\n\ndef test_something():\n pytest.xfail('no citation here')\n" + ) + nodes = _collect_imperative_xfail_nodes(tree) + assert len(nodes) == 1 + reason = _extract_imperative_reason(nodes[0]) + assert reason == "no citation here" + assert not re.search(r"#\d+", reason) + + +def test_imperative_xfail_collector_passes_with_citation() -> None: + """Synthetic AST: an imperative pytest.xfail() citing #NNNN passes the policy.""" + tree = ast.parse( + "import pytest\n\ndef test_something():\n pytest.xfail('known gap, tracking: #1234')\n" + ) + nodes = _collect_imperative_xfail_nodes(tree) + assert len(nodes) == 1 + reason = _extract_imperative_reason(nodes[0]) + assert reason is not None + assert re.search(r"#\d+", reason) + + +def test_imperative_xfail_collector_ignores_decorator_and_param_forms() -> None: + """The imperative collector must not double-count decorator/pytest.param(marks=...) xfails.""" + tree = ast.parse( + "import pytest\n\n" + "@pytest.mark.xfail(strict=True, reason='tracked #99')\n" + "def test_decorated():\n" + " pass\n" + ) + assert _collect_imperative_xfail_nodes(tree) == [] + + def test_exemption_entries_have_rationale_comments() -> None: """Every _XFAIL_POLICY_EXEMPT_FILES entry must have a rationale comment.""" src = Path(__file__).read_text() diff --git a/tests/cli/AGENTS.md b/tests/cli/AGENTS.md index 2b94b39d1..982872981 100644 --- a/tests/cli/AGENTS.md +++ b/tests/cli/AGENTS.md @@ -74,7 +74,7 @@ CLI command, subcommand, and interactive workflow tests. | `test_reap_sidecar_check.py` | Tests for _reap_stale_dispatches sidecar-aware status transition (T-RESUMABLE-10) | | `test_reload_loop.py` | Tests for the session reload sentinel and loop mechanics | | `test_restart.py` | Tests for cli/_restart.py — NoReturn process restart contract | -| `test_routing_completeness.py` | Enum routing completeness: every orchestrator-visible RetryReason must have a routing rule in the orchestrator prompt | +| `test_routing_completeness.py` | Enum routing completeness: every orchestrator-visible RetryReason must have a routing rule in the orchestrator prompt; pins run_skill's docstring to the same contract_recovery routing vocabulary as the prompt | | `test_serve_sigterm.py` | Regression guard: serve() uses event-loop-routed signal handling (issue #745) | | `test_serve_guard_deferral.py` | Tests for serve_with_signal_guard dispatch-aware deferral — active dispatch deferral, immediate cancel, timeout | | `test_session_launch.py` | Tests for cli/_session_launch.py — _run_interactive_session contract | diff --git a/tests/cli/test_routing_completeness.py b/tests/cli/test_routing_completeness.py index b56c80782..7a54f6f7f 100644 --- a/tests/cli/test_routing_completeness.py +++ b/tests/cli/test_routing_completeness.py @@ -93,3 +93,32 @@ def test_expected_routes_covers_all_orchestrator_visible_reasons() -> None: r.name for r in RetryReason if r not in _ROUTING_EXCLUDED and r not in _EXPECTED_ROUTES ] assert not missing, f"Add routing expectation for: {missing}" + + +def test_run_skill_docstring_matches_contract_recovery_routing_vocabulary() -> None: + """run_skill's docstring routing table must use the same contract_recovery vocabulary + as the orchestrator prompt (issue #4305). + + Pins the two prose copies (prompt, docstring — the third copy, SKILL.md, is covered + by tests/contracts/test_sous_chef_routing.py) to the same routing terms so silent + drift in either copy fails a test instead of surfacing as a live routing mismatch. + """ + from autoskillit.server.tools.tools_execution import run_skill + + doc = run_skill.__doc__ + assert doc is not None, "run_skill must have a docstring" + + idx = doc.find(RetryReason.CONTRACT_RECOVERY.value) + assert idx != -1, f"{RetryReason.CONTRACT_RECOVERY.value} not found in run_skill docstring" + + route_keyword, evidence_keyword = _EXPECTED_ROUTES[RetryReason.CONTRACT_RECOVERY] + window = doc[idx : idx + 600] + assert route_keyword in window, ( + f"run_skill docstring: {RetryReason.CONTRACT_RECOVERY.value} must reference " + f"'{route_keyword}' within 600 chars" + ) + assert evidence_keyword, "CONTRACT_RECOVERY must have a non-None evidence keyword" + assert evidence_keyword in window, ( + f"run_skill docstring: {RetryReason.CONTRACT_RECOVERY.value} routing must " + f"reference evidence signal '{evidence_keyword}'" + ) diff --git a/tests/contracts/test_skill_contracts.py b/tests/contracts/test_skill_contracts.py index e83a174cd..6caa40425 100644 --- a/tests/contracts/test_skill_contracts.py +++ b/tests/contracts/test_skill_contracts.py @@ -390,6 +390,12 @@ def test_skill_contracts_allowed_values_covers_recipe_routes() -> None: merge-prs.yaml for result.verdict routing conditions. Any value routed in a recipe but absent from skill_contracts.yaml allowed_values will trigger the unrouted-verdict-value semantic rule at recipe-load time. + + Checks both the ``skills:`` section (LLM-emitted verdicts, e.g. review-pr's + approved/changes_requested) and the ``callable_contracts:`` section + (deterministic run_python verdicts, e.g. verify_plan_artifacts's + salvaged/unsalvageable) — both are valid sources of a ``result.verdict`` + routed in recipe on_result blocks. """ recipes_dir = Path(__file__).parents[2] / "src/autoskillit/recipes" target_files = [ @@ -404,6 +410,10 @@ def test_skill_contracts_allowed_values_covers_recipe_routes() -> None: for o in skill_data.get("outputs", []): if o.get("name") == "verdict": allowed.update(o.get("allowed_values", [])) + for callable_data in contracts.get("callable_contracts", {}).values(): + for o in callable_data.get("outputs", []): + if o.get("name") == "verdict": + allowed.update(o.get("allowed_values", [])) assert allowed, "No verdict output found in skill_contracts.yaml" # Match only lowercase verdict values (review-pr convention: approved, changes_requested…). @@ -659,6 +669,24 @@ def test_make_plan_conditional_write_behavior(skills): assert mp["write_expected_when"] == ["verdict[ \\t]*=[ \\t]*plan"] +def test_validate_audit_verdict_allowed_values(skills): + outputs = [o for o in skills["validate-audit"]["outputs"] if o["name"] == "verdict"] + assert len(outputs) == 1 + assert outputs[0]["allowed_values"] == ["validated"] + + +def test_validate_test_audit_verdict_allowed_values(skills): + outputs = [o for o in skills["validate-test-audit"]["outputs"] if o["name"] == "verdict"] + assert len(outputs) == 1 + assert outputs[0]["allowed_values"] == ["validated"] + + +def test_audit_impl_verdict_allowed_values(skills): + outputs = [o for o in skills["audit-impl"]["outputs"] if o["name"] == "verdict"] + assert len(outputs) == 1 + assert outputs[0]["allowed_values"] == ["GO", "NO GO"] + + def test_make_plan_examples_cover_verdicts(skills): """pattern_examples must include examples for both verdict values.""" mp = skills["make-plan"] diff --git a/tests/core/test_type_constants.py b/tests/core/test_type_constants.py index 9bd72034a..c77b01a31 100644 --- a/tests/core/test_type_constants.py +++ b/tests/core/test_type_constants.py @@ -32,14 +32,14 @@ def test_response_backstop_exemption_registry_is_closed_and_pinned() -> None: assert RESPONSE_BACKSTOP_EXEMPTION_REGISTRY == { "load_recipe": ResponseBackstopExemptionDef( - max_chars=188_000, - max_utf8_bytes=188_000, - measurement_id="bundled-recipes-all-modes-2026-07-21/load-recipe", + max_chars=195_000, + max_utf8_bytes=195_000, + measurement_id="bundled-recipes-all-modes-2026-07-22/load-recipe", ), "open_kitchen": ResponseBackstopExemptionDef( - max_chars=188_000, - max_utf8_bytes=188_000, - measurement_id="bundled-recipes-all-modes-2026-07-21/open-kitchen", + max_chars=195_000, + max_utf8_bytes=195_000, + measurement_id="bundled-recipes-all-modes-2026-07-22/open-kitchen", ), } @@ -60,7 +60,7 @@ def test_response_backstop_exemption_registry_digest_is_canonical() -> None: ) assert ( RESPONSE_BACKSTOP_EXEMPTION_REGISTRY_DIGEST - == "5acb77e003aa6cb54242ccf8dc6af2776f6a03fd5bcc993552ca71f8285d4842" + == "1dae50b43813a0e9addb643794edfcdb5b93d069ed650ae5f51bac432aa90e61" ) diff --git a/tests/execution/AGENTS.md b/tests/execution/AGENTS.md index c8a650664..7f3038374 100644 --- a/tests/execution/AGENTS.md +++ b/tests/execution/AGENTS.md @@ -47,6 +47,7 @@ Subprocess integration, headless session, process lifecycle, and session result | `test_headless_dispatch.py` | Tests for headless.py dispatch flow: food truck dispatch, pack injection, executor protocol | | `test_headless_execute.py` | Tests for assert_headless_cmd CmdSpec validation gate | | `test_headless_env_injection.py` | Phase 2 tests: AUTOSKILLIT_HEADLESS=1 env var injection in headless.py | +| `test_headless_enum_recovery.py` | Tests for contract-aware enum-token recovery: deterministic enum inference and generalized enum nudge hints | | `test_headless_env_scrub.py` | Launch-site env-scrub contract test for run_headless_core | | `test_headless_ordering.py` | AST-based structural test for post-session operation ordering in headless.py | | `test_headless_path_validation.py` | Tests for headless.py: _build_skill_result, path validation, synthesis, and contract gates | diff --git a/tests/execution/backends/fixtures/codex_ndjson/config_toml_schema_template.json b/tests/execution/backends/fixtures/codex_ndjson/config_toml_schema_template.json index bf95756ad..9f83a867f 100644 --- a/tests/execution/backends/fixtures/codex_ndjson/config_toml_schema_template.json +++ b/tests/execution/backends/fixtures/codex_ndjson/config_toml_schema_template.json @@ -28,7 +28,7 @@ "tool_output_token_limit": { "constraint": "exact", "expected_type": "int", - "expected_value": 55000 + "expected_value": 56750 } } } diff --git a/tests/execution/backends/test_codex_config.py b/tests/execution/backends/test_codex_config.py index 238fe7e0a..540017830 100644 --- a/tests/execution/backends/test_codex_config.py +++ b/tests/execution/backends/test_codex_config.py @@ -36,9 +36,9 @@ def test_codex_tool_output_limit_is_derived_from_largest_measured_exemption() -> max_bytes = max( definition.max_utf8_bytes for definition in RESPONSE_BACKSTOP_EXEMPTION_REGISTRY.values() ) - assert max_bytes == 188_000 + assert max_bytes == 195_000 assert CODEX_TOOL_OUTPUT_TOKEN_LIMIT == ((max_bytes + 3) // 4) + 8_000 - assert CODEX_TOOL_OUTPUT_TOKEN_LIMIT == 55_000 + assert CODEX_TOOL_OUTPUT_TOKEN_LIMIT == 56_750 def test_response_backstop_fires_below_codex_transport_ceiling() -> None: diff --git a/tests/execution/test_headless_enum_recovery.py b/tests/execution/test_headless_enum_recovery.py new file mode 100644 index 000000000..25c6a6b0e --- /dev/null +++ b/tests/execution/test_headless_enum_recovery.py @@ -0,0 +1,418 @@ +"""Tests for contract-aware enum-token recovery (Part A of #4305 rectify). + +Covers deterministic enum inference (_parse_single_enum_binding, +_infer_enum_token_from_write_contract) and generalized enum nudge hints +(_extract_missing_token_hints, _attempt_contract_nudge). +""" + +from __future__ import annotations + +import json + +import pytest + +from autoskillit.core.types import RetryReason, SkillResult +from autoskillit.execution.backends.claude import ClaudeResultParser +from autoskillit.execution.headless import ( + _attempt_contract_nudge, + _extract_missing_token_hints, + _infer_enum_token_from_write_contract, + _parse_single_enum_binding, +) +from autoskillit.execution.session import ClaudeSessionResult +from autoskillit.recipe._contracts_types import SkillContract, SkillOutput +from autoskillit.recipe.contracts import get_skill_contract, load_bundled_manifest +from tests.conftest import _make_result +from tests.execution.conftest import _mock_backend, _success_session_json + +pytestmark = [pytest.mark.layer("execution"), pytest.mark.small] + + +def _make_plan_contract( + *, + write_behavior: str | None = "conditional", + write_expected_when: list[str] | None = None, + verdict_allowed_values: list[str] | None = None, +) -> SkillContract: + """Build a make-plan-shaped SkillContract (verdict enum bound to a conditional write).""" + return SkillContract( + inputs=[], + outputs=[ + SkillOutput( + name="verdict", + type="string", + allowed_values=( + verdict_allowed_values + if verdict_allowed_values is not None + else ["plan", "false_positive"] + ), + ), + SkillOutput(name="plan_path", type="file_path"), + SkillOutput(name="plan_parts", type="file_path_list"), + ], + expected_output_patterns=[r"verdict[ \t]*=[ \t]*(plan|false_positive)"], + write_behavior=write_behavior, + write_expected_when=( + write_expected_when + if write_expected_when is not None + else [r"verdict[ \t]*=[ \t]*plan"] + ), + ) + + +def _write_ndjson(result_text: str, file_path: str) -> str: + content = [ + {"type": "tool_use", "name": "Write", "id": "t0", "input": {"file_path": file_path}} + ] + records = [ + json.dumps({"type": "assistant", "message": {"content": content}}), + json.dumps( + { + "type": "result", + "subtype": "success", + "is_error": False, + "result": result_text, + "session_id": "test-session", + } + ), + ] + return "\n".join(records) + + +class TestParseSingleEnumBinding: + def test_sound_single_binding_returns_token_and_value(self): + contract = _make_plan_contract() + assert _parse_single_enum_binding(contract) == ("verdict", "plan") + + def test_no_binding_for_none_contract(self): + assert _parse_single_enum_binding(None) is None + + def test_no_binding_for_unbound_contract(self): + """audit-impl shape: no write_expected_when at all.""" + contract = _make_plan_contract(write_behavior=None, write_expected_when=[]) + assert _parse_single_enum_binding(contract) is None + + def test_no_binding_for_multi_entry_write_expected_when(self): + contract = _make_plan_contract( + write_expected_when=[r"verdict[ \t]*=[ \t]*plan", r"other[ \t]*=[ \t]*x"] + ) + assert _parse_single_enum_binding(contract) is None + + def test_no_binding_for_alternation_value(self): + contract = _make_plan_contract(write_expected_when=[r"verdict[ \t]*=[ \t]*(plan|other)"]) + assert _parse_single_enum_binding(contract) is None + + def test_no_binding_when_value_not_in_allowed_values(self): + contract = _make_plan_contract(write_expected_when=[r"verdict[ \t]*=[ \t]*archived"]) + assert _parse_single_enum_binding(contract) is None + + +class TestInferEnumTokenFromWriteContract: + def test_infers_when_companion_path_exists_on_disk(self, tmp_path): + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract() + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {plan_file}\nplan_parts = {plan_file}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=1, + ) + assert result is not None + assert "verdict = plan" in result.result + + def test_no_inference_without_write_evidence(self, tmp_path): + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract() + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {plan_file}\nplan_parts = {plan_file}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=0, + file_changes=(), + ) + assert result is None + + def test_no_inference_when_companion_file_missing(self, tmp_path): + missing_path = tmp_path / "does_not_exist.md" + contract = _make_plan_contract() + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {missing_path}\nplan_parts = {missing_path}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=1, + ) + assert result is None + + def test_no_inference_for_unbound_contract(self, tmp_path): + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract(write_behavior=None, write_expected_when=[]) + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {plan_file}\nplan_parts = {plan_file}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=1, + ) + assert result is None + + @pytest.mark.parametrize( + "write_expected_when", + [ + [r"verdict[ \t]*=[ \t]*plan", r"other[ \t]*=[ \t]*x"], + [r"verdict[ \t]*=[ \t]*(plan|other)"], + ], + ids=["two-entries", "alternation-value"], + ) + def test_no_inference_for_multi_binding_or_complex_pattern( + self, tmp_path, write_expected_when + ): + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract(write_expected_when=write_expected_when) + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {plan_file}\nplan_parts = {plan_file}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=1, + ) + assert result is None + + def test_no_inference_when_value_not_in_allowed_values(self, tmp_path): + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract(write_expected_when=[r"verdict[ \t]*=[ \t]*archived"]) + session = ClaudeSessionResult( + subtype="success", + is_error=False, + result=f"plan_path = {plan_file}\nplan_parts = {plan_file}\n%%ORDER_UP%%", + session_id="test-session", + ) + result = _infer_enum_token_from_write_contract( + session, + contract.expected_output_patterns, + contract, + write_call_count=1, + ) + assert result is None + + +class TestBuildSkillResultIncidentReproduction: + """Full-stack _build_skill_result reproduction of the #4305 incident shape.""" + + def test_incident_reproduction_enum_inference_recovers(self, tmp_path): + from autoskillit.core.types import ChannelConfirmation + from autoskillit.execution.backends.claude import ClaudeCodeBackend + from autoskillit.execution.headless import _build_skill_result + + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract() + + result_text = f"plan_path = {plan_file}\nplan_parts = {plan_file}\nsummary\n%%ORDER_UP%%" + stdout = _write_ndjson(result_text, str(plan_file)) + subprocess_result = _make_result( + stdout=stdout, channel_confirmation=ChannelConfirmation.CHANNEL_A + ) + + sr = _build_skill_result( + subprocess_result, + completion_marker="%%ORDER_UP%%", + expected_output_patterns=contract.expected_output_patterns, + backend=ClaudeCodeBackend(), + skill_contract=contract, + ) + + assert sr.success is True + assert sr.retry_reason == RetryReason.NONE + assert "verdict = plan" in sr.result + + def test_enum_inference_after_path_synthesis_unmonitored(self, tmp_path): + from autoskillit.core.types import ChannelConfirmation + from autoskillit.execution.backends.claude import ClaudeCodeBackend + from autoskillit.execution.headless import _build_skill_result + + plan_file = tmp_path / "plan.md" + plan_file.write_text("plan body") + contract = _make_plan_contract() + + # No plan_path/plan_parts token lines in the result text — only the write + # tool_use carries the path. Include a path-capture pattern so the existing + # UNMONITORED synthesis stage fires and injects plan_path first. + expected_patterns = [*contract.expected_output_patterns, r"plan_path\s*=\s*/.+"] + result_text = "summary\n%%ORDER_UP%%" + stdout = _write_ndjson(result_text, str(plan_file)) + subprocess_result = _make_result( + stdout=stdout, channel_confirmation=ChannelConfirmation.UNMONITORED + ) + + sr = _build_skill_result( + subprocess_result, + completion_marker="%%ORDER_UP%%", + expected_output_patterns=expected_patterns, + backend=ClaudeCodeBackend(), + skill_contract=contract, + ) + + assert sr.success is True + assert sr.retry_reason == RetryReason.NONE + assert "verdict = plan" in sr.result + assert f"plan_path = {plan_file}" in sr.result + + +class TestEnumHintGeneration: + def test_enum_hint_generated_for_missing_enum_token(self): + contract = _make_plan_contract() + stdout = _write_ndjson("plan summary\n%%ORDER_UP%%", "/tmp/plan.md") + hints = _extract_missing_token_hints( + stdout, + contract.expected_output_patterns, + ClaudeResultParser(), + frozenset({"Write", "Edit"}), + skill_contract=contract, + ) + assert len(hints) == 1 + hint = hints[0] + from autoskillit.execution.headless._headless_recovery import _EnumHint + + assert isinstance(hint, _EnumHint) + assert hint.token == "verdict" + assert set(hint.allowed_values) == {"plan", "false_positive"} + + def test_path_hint_behavior_unchanged(self): + """Regression: missing path-capture token still produces the (token, path) hint.""" + stdout = _write_ndjson("plan summary\n%%ORDER_UP%%", "/tmp/out.md") + hints = _extract_missing_token_hints( + stdout, + [r"plan_path\s*=\s*/.+"], + ClaudeResultParser(), + frozenset({"Write", "Edit"}), + ) + assert hints == [("plan_path", "/tmp/out.md")] + + +class TestEnumNudgeIntegration: + @pytest.mark.anyio + async def test_enum_nudge_success_requires_pattern_match(self, tmp_path): + from tests.fakes import MockSubprocessRunner + + contract = _make_plan_contract() + backend = _mock_backend(session_resume_capable=True) + + skill_result = SkillResult( + success=False, + result="plan summary", + session_id="test-session", + subtype="adjudicated_failure", + is_error=False, + exit_code=0, + needs_retry=True, + retry_reason=RetryReason.CONTRACT_RECOVERY, + stderr="", + ) + subprocess_result = _make_result( + stdout=_write_ndjson("plan summary\n%%ORDER_UP%%", "/tmp/plan.md") + ) + + runner = MockSubprocessRunner() + runner.push(_make_result(stdout=_success_session_json("verdict = plan\n%%ORDER_UP%%"))) + + patched = await _attempt_contract_nudge( + skill_result, + subprocess_result, + contract.expected_output_patterns, + "%%ORDER_UP%%", + str(tmp_path), + runner, + backend=backend, + result_parser=ClaudeResultParser(), + skill_contract=contract, + ) + assert patched is not None + assert patched.success is True + assert "verdict = plan" in patched.result + + runner.push( + _make_result(stdout=_success_session_json("not the required token\n%%ORDER_UP%%")) + ) + + rejected = await _attempt_contract_nudge( + skill_result, + subprocess_result, + contract.expected_output_patterns, + "%%ORDER_UP%%", + str(tmp_path), + runner, + backend=backend, + result_parser=ClaudeResultParser(), + skill_contract=contract, + ) + assert rejected is None + + +class TestBundledContractActivation: + """Real-bundled-manifest activation proofs for #4305 Part A metadata (skill_contracts.yaml).""" + + def test_validate_audit_contract_enables_enum_inference(self): + manifest = load_bundled_manifest() + contract = get_skill_contract("validate-audit", manifest) + assert _parse_single_enum_binding(contract) == ("verdict", "validated") + + def test_validate_test_audit_contract_enables_enum_inference(self): + manifest = load_bundled_manifest() + contract = get_skill_contract("validate-test-audit", manifest) + assert _parse_single_enum_binding(contract) == ("verdict", "validated") + + def test_audit_impl_contract_enables_enum_nudge_hint(self): + manifest = load_bundled_manifest() + contract = get_skill_contract("audit-impl", manifest) + assert contract is not None + + assert _parse_single_enum_binding(contract) is None + + stdout = _write_ndjson("no verdict token here\n%%ORDER_UP%%", "/tmp/remediation.md") + hints = _extract_missing_token_hints( + stdout, + contract.expected_output_patterns, + ClaudeResultParser(), + frozenset({"Write", "Edit"}), + skill_contract=contract, + ) + assert len(hints) == 1 + hint = hints[0] + from autoskillit.execution.headless._headless_recovery import _EnumHint + + assert isinstance(hint, _EnumHint) + assert hint.token == "verdict" + assert set(hint.allowed_values) == {"GO", "NO GO"} diff --git a/tests/infra/test_pretty_output_recipe.py b/tests/infra/test_pretty_output_recipe.py index 6be3b526d..21c674982 100644 --- a/tests/infra/test_pretty_output_recipe.py +++ b/tests/infra/test_pretty_output_recipe.py @@ -1052,8 +1052,8 @@ def test_canonical_recipe_responses_fit_independent_registry_ceilings(tmp_path, for ingredients_only in (False, True) } assert maxima == { - "load_recipe": (184_303, "remediation", "all_truthy"), - "open_kitchen": (184_356, "remediation", "all_truthy"), + "load_recipe": (193_203, "remediation", "all_truthy"), + "open_kitchen": (193_256, "remediation", "all_truthy"), } diff --git a/tests/recipe/AGENTS.md b/tests/recipe/AGENTS.md index 3e15f4b59..2a94ac4a9 100644 --- a/tests/recipe/AGENTS.md +++ b/tests/recipe/AGENTS.md @@ -47,6 +47,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. | `test_compute_recipe_validity.py` | Branch coverage for `compute_recipe_validity` | | `test_cmd_rpc_merge.py` | Tests for recipe/_cmd_rpc_merge.py — base branch fetch discipline and fetch error classification | | `test_cmd_rpc_null_safety.py` | Null safety tests for _cmd_rpc callables | +| `test_cmd_rpc_verify_plan_artifacts.py` | Tests for verify_plan_artifacts — deterministic salvage callable for context-limit-stumbled plan-producing steps | | `test_contract_verdict_output_required.py` | Contract: verdict output is required in recipe step | | `test_contract_strength.py` | Contract strength validation — structural guards against weak contracts (optional patterns, completion_required consistency) | | `test_contracts.py` | Contract tests for recipe schema and recipe step contracts | @@ -146,6 +147,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. | `test_rules_cmd.py` | Tests for cmd semantic validation rule | | `test_rules_commit_guard_regression_route.py` | Tests for `commit-guard-regression-route-missing` semantic validation rule | | `test_rules_conditional_push.py` | Tests for conditional_push semantic validation rule | +| `test_rules_contract_recovery.py` | Tests for contract-recovery-requires-salvage-route semantic validation rule | | `test_rules_contracts.py` | Tests for contracts semantic validation rule | | `test_rules_context_param_forwarding.py` | Tests for context-param-not-forwarded semantic validation rule — guards `_TOOL_CONTEXT_PARAMS` registry forwarding invariant for `auto_merge_available` and other upstream-captured context variables consumed by tool steps (PR #3901 regression guard) | | `test_rules_dataflow_capture.py` | Tests for dataflow capture semantic validation rule | diff --git a/tests/recipe/test_bundled_recipes_behavioral_properties.py b/tests/recipe/test_bundled_recipes_behavioral_properties.py index 2c2a8fc95..bd45f70b8 100644 --- a/tests/recipe/test_bundled_recipes_behavioral_properties.py +++ b/tests/recipe/test_bundled_recipes_behavioral_properties.py @@ -14,7 +14,7 @@ import pytest -from autoskillit.recipe.io import all_validated_recipe_paths, load_recipe +from autoskillit.recipe.io import all_validated_recipe_paths, builtin_recipes_dir, load_recipe pytestmark = [pytest.mark.layer("recipe"), pytest.mark.medium] @@ -28,17 +28,75 @@ CONTEXT_LIMIT_EXEMPT_STEPS: dict[str, set[str]] = { "planner": set(), "remediation": set(), - "research": set(), + "research": { + "scope", + "select_directions", + "plan_experiment", + "vis_dial", + "vis_apply", + "vis_synthesize", + "stage_data", + "download_data", + "setup_environment", + "decompose_phases", + "troubleshoot_implement_failure", + "run_experiment", + "troubleshoot_run_failure", + "prepare_research_pr", + "run_experiment_lenses", + "compose_research_pr", + "finalize_bundle_render", + }, "implementation": set(), - "implementation-groups": set(), - "merge-prs": set(), - "full-audit": set(), - "bem-wrapper": set(), - "research-design": set(), - "research-implement": set(), - "research-review": set(), + "implementation-groups": {"group"}, + "merge-prs": { + "analyze_prs", + "diagnose_queue_ci", + "open_integration_pr", + "review_pr_integration", + "diagnose_ci", + }, + "full-audit": {"run_audits", "validate_audits", "create_issues"}, + "bem-wrapper": {"run_bem"}, + "research-design": { + "scope", + "select_directions", + "plan_experiment", + "vis_dial", + "vis_apply", + "vis_synthesize", + }, + "research-implement": { + "stage_data", + "download_data", + "decompose_phases", + "troubleshoot_implement_failure", + "troubleshoot_run_failure", + }, + "research-review": { + "prepare_research_pr", + "run_experiment_lenses", + "compose_research_pr", + "finalize_bundle_render", + }, +} + +# Every recipe key above with a non-empty exemption set must be cited here. +# tracking: #4305 — pre-existing gaps surfaced by dismantling the recipe-level +# allowlist this dict replaced; not yet given salvage routes. +CONTEXT_LIMIT_EXEMPT_STEPS_TRACKING: dict[str, str] = { + "research": "#4305", + "implementation-groups": "#4305", + "merge-prs": "#4305", + "full-audit": "#4305", + "bem-wrapper": "#4305", + "research-design": "#4305", + "research-implement": "#4305", + "research-review": "#4305", } +_CONTEXT_LIMIT_EXEMPT_STEPS_CAP = 42 + PARALLEL_ELIGIBLE_DISPATCH_STEPS: dict[str, set[str]] = { "planner": { "elaborate_phases", @@ -58,47 +116,179 @@ def _recipe_base_name(filename: str) -> str: return filename.removesuffix(".yaml") -_CONTEXT_LIMIT_COMPLIANT_RECIPES: set[str] = { - "consolidate-health-reports", - "implement-findings", - "planner", - "promote-to-main", - "promote-to-main-wrapper", - "research-archive", - "research-campaign", -} - - RATE_LIMIT_EXEMPT_STEPS: dict[str, set[str]] = { "planner": set(), "remediation": set(), - "research": set(), + "research": { + "scope", + "select_directions", + "plan_experiment", + "dial", + "apply", + "vis_dial", + "vis_apply", + "vis_synthesize", + "resolve_design_review", + "stage_data", + "download_data", + "setup_environment", + "decompose_phases", + "plan_phase", + "implement_phase", + "troubleshoot_implement_failure", + "audit_impl", + "run_experiment", + "troubleshoot_run_failure", + "adjust_experiment", + "generate_report", + "generate_report_inconclusive", + "fix_tests", + "prepare_research_pr", + "run_experiment_lenses", + "compose_research_pr", + "review_research_pr", + "audit_claims", + "resolve_research_review", + "resolve_claims_review", + "re_run_experiment", + "re_generate_report", + "finalize_bundle_render", + }, "implementation": set(), "implementation-groups": set(), "merge-prs": set(), - "full-audit": set(), - "bem-wrapper": set(), - "research-design": set(), - "research-implement": set(), - "research-review": set(), + "full-audit": {"run_audits", "validate_audits", "create_issues"}, + "bem-wrapper": {"run_bem"}, + "implement-findings": {"run_bem_internally"}, + "research-design": { + "scope", + "select_directions", + "plan_experiment", + "dial", + "apply", + "vis_dial", + "vis_apply", + "vis_synthesize", + "resolve_design_review", + }, + "research-implement": { + "stage_data", + "download_data", + "decompose_phases", + "plan_phase", + "implement_phase", + "troubleshoot_implement_failure", + "audit_impl", + "run_experiment", + "troubleshoot_run_failure", + "adjust_experiment", + "generate_report", + "generate_report_inconclusive", + "fix_tests", + }, + "research-review": { + "prepare_research_pr", + "run_experiment_lenses", + "compose_research_pr", + "review_research_pr", + "audit_claims", + "resolve_research_review", + "resolve_claims_review", + "re_run_experiment", + "re_generate_report", + "finalize_bundle_render", + }, } - -_RATE_LIMIT_COMPLIANT_RECIPES: set[str] = { - "remediation", - "implementation", - "implementation-groups", - "merge-prs", - "promote-to-main-wrapper", - "planner", +# Every recipe key above with a non-empty exemption set must be cited here. +# tracking: #4305 — pre-existing gaps surfaced by dismantling the recipe-level +# allowlist this dict replaced; not yet given salvage routes. +RATE_LIMIT_EXEMPT_STEPS_TRACKING: dict[str, str] = { + "research": "#4305", + "full-audit": "#4305", + "bem-wrapper": "#4305", + "implement-findings": "#4305", + "research-design": "#4305", + "research-implement": "#4305", + "research-review": "#4305", } +_RATE_LIMIT_EXEMPT_STEPS_CAP = 70 + + +def test_context_limit_exempt_steps_size_cap() -> None: + """CONTEXT_LIMIT_EXEMPT_STEPS must not grow beyond current size. + + If this test fails, a new exemption was added. Fix the recipe (add a real + on_context_limit salvage route) instead of silently growing this registry. + """ + total = sum(len(v) for v in CONTEXT_LIMIT_EXEMPT_STEPS.values()) + assert total <= _CONTEXT_LIMIT_EXEMPT_STEPS_CAP, ( + f"CONTEXT_LIMIT_EXEMPT_STEPS has {total} entries (cap: " + f"{_CONTEXT_LIMIT_EXEMPT_STEPS_CAP}). Fix the recipe instead of adding exemptions." + ) + + +def test_context_limit_exempt_steps_have_tracking_comments() -> None: + """Every non-empty CONTEXT_LIMIT_EXEMPT_STEPS entry must cite a tracking issue.""" + missing = [ + recipe + for recipe, steps in CONTEXT_LIMIT_EXEMPT_STEPS.items() + if steps and recipe not in CONTEXT_LIMIT_EXEMPT_STEPS_TRACKING + ] + assert not missing, ( + f"CONTEXT_LIMIT_EXEMPT_STEPS entries missing a tracking citation: {missing}. " + "Add an entry to CONTEXT_LIMIT_EXEMPT_STEPS_TRACKING with the relevant issue number." + ) + stale = [ + recipe + for recipe in CONTEXT_LIMIT_EXEMPT_STEPS_TRACKING + if not CONTEXT_LIMIT_EXEMPT_STEPS.get(recipe) + ] + assert not stale, ( + f"CONTEXT_LIMIT_EXEMPT_STEPS_TRACKING has stale entries with no matching " + f"non-empty exemption: {stale}. Remove them." + ) + + +def test_rate_limit_exempt_steps_size_cap() -> None: + """RATE_LIMIT_EXEMPT_STEPS must not grow beyond current size. + + If this test fails, a new exemption was added. Fix the recipe (add a real + on_rate_limit route) instead of silently growing this registry. + """ + total = sum(len(v) for v in RATE_LIMIT_EXEMPT_STEPS.values()) + assert total <= _RATE_LIMIT_EXEMPT_STEPS_CAP, ( + f"RATE_LIMIT_EXEMPT_STEPS has {total} entries (cap: " + f"{_RATE_LIMIT_EXEMPT_STEPS_CAP}). Fix the recipe instead of adding exemptions." + ) + + +def test_rate_limit_exempt_steps_have_tracking_comments() -> None: + """Every non-empty RATE_LIMIT_EXEMPT_STEPS entry must cite a tracking issue.""" + missing = [ + recipe + for recipe, steps in RATE_LIMIT_EXEMPT_STEPS.items() + if steps and recipe not in RATE_LIMIT_EXEMPT_STEPS_TRACKING + ] + assert not missing, ( + f"RATE_LIMIT_EXEMPT_STEPS entries missing a tracking citation: {missing}. " + "Add an entry to RATE_LIMIT_EXEMPT_STEPS_TRACKING with the relevant issue number." + ) + stale = [ + recipe + for recipe in RATE_LIMIT_EXEMPT_STEPS_TRACKING + if not RATE_LIMIT_EXEMPT_STEPS.get(recipe) + ] + assert not stale, ( + f"RATE_LIMIT_EXEMPT_STEPS_TRACKING has stale entries with no matching " + f"non-empty exemption: {stale}. Remove them." + ) + @pytest.mark.parametrize("recipe_name", _RECIPE_NAMES) def test_run_skill_steps_declare_on_context_limit(recipe_name: str) -> None: """Every run_skill step must declare on_context_limit (or be exempt).""" - if _recipe_base_name(recipe_name) not in _CONTEXT_LIMIT_COMPLIANT_RECIPES: - pytest.xfail("on_context_limit handlers not yet added to all bundled recipes") recipe_path = next(p for p in _BUNDLED_ONLY if p.name == recipe_name) recipe = load_recipe(recipe_path) exempt = CONTEXT_LIMIT_EXEMPT_STEPS.get(_recipe_base_name(recipe_name), set()) @@ -134,8 +324,6 @@ def test_run_skill_steps_declare_on_context_limit(recipe_name: str) -> None: @pytest.mark.parametrize("recipe_name", _RECIPE_NAMES) def test_run_skill_steps_declare_on_rate_limit(recipe_name: str) -> None: """Every run_skill step must declare on_rate_limit (or be exempt).""" - if _recipe_base_name(recipe_name) not in _RATE_LIMIT_COMPLIANT_RECIPES: - pytest.xfail("on_rate_limit handlers not yet added to all bundled recipes") recipe_path = next(p for p in _BUNDLED_ONLY if p.name == recipe_name) recipe = load_recipe(recipe_path) exempt = RATE_LIMIT_EXEMPT_STEPS.get(_recipe_base_name(recipe_name), set()) @@ -208,3 +396,115 @@ def test_context_intensive_steps_declare_explicit_model(recipe_name: str) -> Non f"{recipe_name}.{step_name}: context-intensive step must declare an explicit " f"model (not empty string), got model={step.model!r}" ) + + +# The nine (recipe, step) pairs whose skill contracts can trigger +# retry_reason=contract_recovery — audited risk table, issue #4305. +_SALVAGE_ROUTE_SITES: list[tuple[str, str]] = [ + ("remediation", "make_plan"), + ("implementation", "plan"), + ("implementation-groups", "plan"), + ("research-implement", "plan_phase"), + ("research", "plan_phase"), + ("merge-prs", "plan"), + ("remediation", "rectify"), + ("remediation", "dry_walkthrough"), + ("remediation", "audit_impl"), +] + +# Destinations that abandon a salvageable artifact instead of attempting salvage. +_DESTRUCTIVE_SALVAGE_ROUTES: frozenset[str] = frozenset( + {"release_issue_failure", "register_clone_failure", "escalate_stop"} +) + + +@pytest.mark.parametrize( + "recipe_name,step_name", + _SALVAGE_ROUTE_SITES, + ids=[f"{r}:{s}" for r, s in _SALVAGE_ROUTE_SITES], +) +def test_contract_recovery_capable_steps_have_salvage_route( + recipe_name: str, step_name: str +) -> None: + """Steps whose skill contracts can trigger contract_recovery must declare a + non-destructive, non-decorative on_context_limit salvage route (issue #4305).""" + recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + step = recipe.steps[step_name] + assert step.on_context_limit is not None, ( + f"{recipe_name}.{step_name}: on_context_limit must be set — for capture_list " + f"steps (retries: 0 forced), on_context_limit is the only in-recipe salvage lever" + ) + assert step.on_context_limit != step.on_failure, ( + f"{recipe_name}.{step_name}: on_context_limit={step.on_context_limit!r} is a " + f"decorative alias of on_failure — it must attempt salvage before falling back" + ) + assert step.on_context_limit not in _DESTRUCTIVE_SALVAGE_ROUTES, ( + f"{recipe_name}.{step_name}: on_context_limit={step.on_context_limit!r} routes " + f"straight to a destructive terminal step, abandoning any salvageable artifact" + ) + assert step.on_context_limit in recipe.steps, ( + f"{recipe_name}.{step_name}: on_context_limit target " + f"{step.on_context_limit!r} does not exist as a step in this recipe" + ) + + +# Class-1 (plan-producing) sites and the salvage step that verifies their artifacts. +_CLASS1_SALVAGE_SITES: list[tuple[str, str, str]] = [ + ("remediation", "make_plan", "salvage_plan"), + ("implementation", "plan", "salvage_plan"), + ("implementation-groups", "plan", "salvage_plan"), + ("research-implement", "plan_phase", "salvage_plan_phase"), + ("research", "plan_phase", "salvage_plan_phase"), + ("merge-prs", "plan", "salvage_plan"), + ("remediation", "rectify", "salvage_rectify_plan"), +] + + +@pytest.mark.parametrize( + "recipe_name,step_name,salvage_step_name", + _CLASS1_SALVAGE_SITES, + ids=[f"{r}:{s}" for r, s, _ in _CLASS1_SALVAGE_SITES], +) +def test_salvage_step_routes_match_plan_step_destinations( + recipe_name: str, step_name: str, salvage_step_name: str +) -> None: + """Class-1 salvage steps must route verdict==salvaged to the plan step's own + success destination and verdict==unsalvageable to its own on_failure destination — + read from the loaded recipe so the assertion survives future retargeting.""" + recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + plan_step = recipe.steps[step_name] + salvage_step = recipe.steps[salvage_step_name] + + assert plan_step.on_context_limit == salvage_step_name + + if plan_step.on_result is not None: + plan_success_route = next( + c.route + for c in plan_step.on_result.conditions + if c.when and "== plan" in c.when and "false_positive" not in c.when + ) + else: + assert plan_step.on_success is not None, ( + f"{recipe_name}.{step_name}: must declare either on_result (verdict " + f"routing) or on_success — the salvage step's success destination is " + f"read from whichever is present" + ) + plan_success_route = plan_step.on_success + + assert salvage_step.on_result is not None + salvaged_route = next( + c.route for c in salvage_step.on_result.conditions if c.when and "salvaged" in c.when + ) + assert salvaged_route == plan_success_route, ( + f"{recipe_name}.{salvage_step_name}: verdict==salvaged must route to " + f"{plan_success_route!r} (the plan step's own success destination)" + ) + + unsalvageable_route = next( + (c.route for c in salvage_step.on_result.conditions if c.when is None), + salvage_step.on_failure, + ) + assert unsalvageable_route == plan_step.on_failure, ( + f"{recipe_name}.{salvage_step_name}: verdict==unsalvageable must route to " + f"{plan_step.on_failure!r} (the plan step's own on_failure destination)" + ) diff --git a/tests/recipe/test_bundled_recipes_dispatch_ready.py b/tests/recipe/test_bundled_recipes_dispatch_ready.py index e9ae8428b..4f7b55039 100644 --- a/tests/recipe/test_bundled_recipes_dispatch_ready.py +++ b/tests/recipe/test_bundled_recipes_dispatch_ready.py @@ -17,6 +17,7 @@ from autoskillit.recipe.io import all_validated_recipe_names, builtin_recipes_dir, load_recipe from autoskillit.recipe.schema import RecipeKind from autoskillit.recipe.validator import run_semantic_rules +from tests.recipe.test_bundled_recipes_behavioral_properties import _SALVAGE_ROUTE_SITES pytestmark = [pytest.mark.layer("recipe"), pytest.mark.medium] @@ -45,6 +46,41 @@ def _part_a_recipe_params() -> list: return [pytest.param(n) for n in _RECIPE_STEMS] +@pytest.mark.parametrize( + "recipe_name,step_name", + _SALVAGE_ROUTE_SITES, + ids=[f"{r}:{s}" for r, s in _SALVAGE_ROUTE_SITES], +) +def test_audited_salvage_sites_have_zero_contract_recovery_findings( + recipe_name: str, step_name: str +) -> None: + """Incident-shaped structural regression test (issue #4305). + + Every one of the nine audited contract-recovery-capable call sites must have zero + contract-recovery-requires-salvage-route findings. Reverting any of the salvage + routes wired for these sites turns this red — and + test_contract_recovery_capable_steps_have_salvage_route (behavioral-properties) + turns red too, providing dual coverage. + + Scoped to the audited nine sites rather than every bundled recipe: the rule's + contract-derived eligibility predicate also matches steps outside this audited set + that have not yet been given salvage routes (tracked separately, not yet promoted + to ERROR — see rules_contract_recovery.py's module docstring). + """ + result = load_and_validate(recipe_name, project_dir=_PROJECT_ROOT) + assert "error" not in result, f"Recipe '{recipe_name}' failed to load" + findings = [ + s + for s in result.get("suggestions", []) + if s.get("rule") == "contract-recovery-requires-salvage-route" + and s.get("step") == step_name + ] + assert not findings, ( + f"Recipe '{recipe_name}' step '{step_name}' has contract-recovery-requires-salvage-route " + "findings: " + "; ".join(f"{s.get('message', '')[:150]}" for s in findings) + ) + + @pytest.mark.parametrize("recipe_name", _part_a_dispatch_gate_params(), ids=lambda n: n) def test_bundled_recipe_dispatch_gate_no_exemptions(recipe_name: str) -> None: """Mirror the fleet/_api.py:365 hard gate — no per-rule allowlist. diff --git a/tests/recipe/test_bundled_recipes_pipeline_structure.py b/tests/recipe/test_bundled_recipes_pipeline_structure.py index 194aedc88..218d419d0 100644 --- a/tests/recipe/test_bundled_recipes_pipeline_structure.py +++ b/tests/recipe/test_bundled_recipes_pipeline_structure.py @@ -244,6 +244,13 @@ def test_review_approach_step_has_on_context_limit(self, recipe) -> None: def test_audit_impl_has_on_context_limit(self, recipe) -> None: step = recipe.steps["audit_impl"] + if recipe.name == "remediation": + assert step.on_context_limit == "check_audit_remediation_loop", ( + "remediation audit_impl on_context_limit must route through the existing " + "loop-governing step (mirrors on_rate_limit) rather than the decorative " + "register_clone_failure alias of on_failure (issue #4305)" + ) + return assert step.on_context_limit == "register_clone_failure", ( "audit_impl on context limit must register the clone before escalating — " "clone-terminal-requires-registration requires all terminal paths go through " @@ -871,6 +878,10 @@ def test_if5_make_plan_step_has_correct_structure(self, recipe) -> None: assert len(plan_routes) == 1 assert plan_routes[0].route == "dry_walkthrough" assert step.on_failure == "release_issue_failure" + assert step.on_context_limit == "salvage_plan", ( + "make_plan on_context_limit must route through the deterministic salvage " + "step, not fall straight to release_issue_failure (issue #4305)" + ) def test_if3_test_step_uses_implementation_ref(self, recipe) -> None: """T_IF3: test step worktree_path must reference context.implementation_ref.""" diff --git a/tests/recipe/test_cmd_rpc_verify_plan_artifacts.py b/tests/recipe/test_cmd_rpc_verify_plan_artifacts.py new file mode 100644 index 000000000..05eb11a8d --- /dev/null +++ b/tests/recipe/test_cmd_rpc_verify_plan_artifacts.py @@ -0,0 +1,78 @@ +"""Tests for recipe._cmd_rpc.verify_plan_artifacts — deterministic salvage callable +for context-limit-stumbled plan-producing steps (issue #4305).""" + +from __future__ import annotations + +import pytest + +from autoskillit.recipe._cmd_rpc import verify_plan_artifacts + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + + +def _write(tmp_path, name, content="plan content"): + path = tmp_path / name + path.write_text(content) + return str(path) + + +def test_single_absolute_path_salvaged(tmp_path): + p = _write(tmp_path, "plan.md") + result = verify_plan_artifacts(plan_parts=p) + assert result == {"verdict": "salvaged", "plan_parts": p, "plan_path": p} + + +def test_newline_joined_two_paths_salvaged(tmp_path): + a = _write(tmp_path, "plan_part_a.md") + b = _write(tmp_path, "plan_part_b.md") + result = verify_plan_artifacts(plan_parts=f"{a}\n{b}") + assert result == {"verdict": "salvaged", "plan_parts": f"{a}\n{b}", "plan_path": a} + + +def test_comma_separated_two_paths_salvaged(tmp_path): + a = _write(tmp_path, "plan_part_a.md") + b = _write(tmp_path, "plan_part_b.md") + result = verify_plan_artifacts(plan_parts=f"{a},{b}") + assert result == {"verdict": "salvaged", "plan_parts": f"{a}\n{b}", "plan_path": a} + + +def test_json_list_repr_two_paths_salvaged(tmp_path): + a = _write(tmp_path, "plan_part_a.md") + b = _write(tmp_path, "plan_part_b.md") + result = verify_plan_artifacts(plan_parts=f'["{a}", "{b}"]') + assert result == {"verdict": "salvaged", "plan_parts": f"{a}\n{b}", "plan_path": a} + + +def test_python_list_repr_single_path_salvaged(tmp_path): + a = _write(tmp_path, "plan.md") + result = verify_plan_artifacts(plan_parts=f"['{a}']") + assert result == {"verdict": "salvaged", "plan_parts": a, "plan_path": a} + + +def test_newline_joined_missing_file_unsalvageable(tmp_path): + a = _write(tmp_path, "plan_part_a.md") + missing = str(tmp_path / "plan_part_b.md") + result = verify_plan_artifacts(plan_parts=f"{a}\n{missing}") + assert result == {"verdict": "unsalvageable"} + + +def test_empty_input_unsalvageable(): + assert verify_plan_artifacts(plan_parts="") == {"verdict": "unsalvageable"} + + +def test_whitespace_input_unsalvageable(): + assert verify_plan_artifacts(plan_parts=" \n ") == {"verdict": "unsalvageable"} + + +def test_relative_path_unsalvageable(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "plan.md").write_text("content") + result = verify_plan_artifacts(plan_parts="plan.md") + assert result == {"verdict": "unsalvageable"} + + +def test_existing_empty_file_unsalvageable(tmp_path): + empty = tmp_path / "plan.md" + empty.write_text("") + result = verify_plan_artifacts(plan_parts=str(empty)) + assert result == {"verdict": "unsalvageable"} diff --git a/tests/recipe/test_contracts_manifest.py b/tests/recipe/test_contracts_manifest.py index db6e72830..5059cf350 100644 --- a/tests/recipe/test_contracts_manifest.py +++ b/tests/recipe/test_contracts_manifest.py @@ -40,6 +40,17 @@ def test_get_callable_contract_promotes_allowed_values_for_main_repo_guard() -> assert cleaned.allowed_values == ["false", "true", "force", "failed"] +def test_get_callable_contract_promotes_allowed_values_for_verify_plan_artifacts() -> None: + """get_callable_contract must parse `allowed_values` for verify_plan_artifacts.""" + contract = get_callable_contract("autoskillit.recipe._cmd_rpc.verify_plan_artifacts") + assert contract is not None, ( + "verify_plan_artifacts must be declared under callable_contracts in skill_contracts.yaml" + ) + verdict = next((o for o in contract.outputs if o.name == "verdict"), None) + assert verdict is not None, "verify_plan_artifacts contract must declare a 'verdict' output" + assert verdict.allowed_values == ["salvaged", "unsalvageable"] + + def test_get_callable_contract_defaults_allowed_values_to_empty_list() -> None: """When a callable contract output has no `allowed_values` in YAML, defaults to [].""" contract = get_callable_contract("autoskillit.recipe._cmd_rpc.review_path_rebase") diff --git a/tests/recipe/test_rules_contract_recovery.py b/tests/recipe/test_rules_contract_recovery.py new file mode 100644 index 000000000..45e1cc341 --- /dev/null +++ b/tests/recipe/test_rules_contract_recovery.py @@ -0,0 +1,231 @@ +"""Tests for the contract-recovery-requires-salvage-route semantic rule. + +Verifies that a ``run_skill`` step invoking a skill whose contract can trigger +``retry_reason=contract_recovery`` at runtime (non-empty ``expected_output_patterns`` +and not ``read_only``) must declare an ``on_context_limit`` salvage route distinct +from ``on_failure`` — otherwise a completed-but-unparsed artifact is discarded +instead of salvaged (issue #4305). +""" + +from __future__ import annotations + +import pytest + +import autoskillit.recipe.rules.rules_contract_recovery as _rules_contract_recovery +from autoskillit.core.types import Severity +from autoskillit.recipe.registry import run_semantic_rules +from autoskillit.recipe.schema import Recipe, RecipeStep + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + +_RULE_NAME = "contract-recovery-requires-salvage-route" + +_RECOVERY_CAPABLE_SKILL = "make-a-plan" +_READ_ONLY_SKILL = "read-only-reviewer" +_NO_PATTERNS_SKILL = "chatty-skill" + +_MANIFEST: dict = { + "version": "0.1.0", + "skills": { + _RECOVERY_CAPABLE_SKILL: { + "inputs": [], + "outputs": [], + "expected_output_patterns": [r"PLAN_READY::\S+"], + }, + _READ_ONLY_SKILL: { + "inputs": [], + "outputs": [], + "expected_output_patterns": [r"REVIEW_DONE::\S+"], + "read_only": True, + }, + _NO_PATTERNS_SKILL: { + "inputs": [], + "outputs": [], + }, + }, +} + + +def _plan_step( + *, on_context_limit: str | None = None, on_failure: str | None = "escalate_stop" +) -> RecipeStep: + return RecipeStep( + name="make_plan", + tool="run_skill", + with_args={"skill_command": f"/autoskillit:{_RECOVERY_CAPABLE_SKILL}"}, + on_success="next", + on_failure=on_failure, + on_context_limit=on_context_limit, + ) + + +def _recipe_with(step: RecipeStep, *, extra_steps: dict[str, RecipeStep] | None = None) -> Recipe: + steps = {"make_plan": step} + steps.update(extra_steps or {}) + steps.setdefault( + "next", RecipeStep(tool="run_cmd", with_args={"cmd": "echo next"}, on_success="stop_ok") + ) + steps.setdefault("escalate_stop", RecipeStep(action="stop", message="escalation")) + return Recipe(name="test", description="test", kitchen_rules=["test"], steps=steps) + + +def _rule_findings(recipe: Recipe) -> list: + findings = run_semantic_rules(recipe) + return [f for f in findings if f.rule == _RULE_NAME] + + +def test_fires_when_on_context_limit_missing(monkeypatch: pytest.MonkeyPatch) -> None: + """Rule fires WARNING when a contract-recovery-capable step has no on_context_limit. + + Severity is WARNING (not ERROR) pending a follow-up remediation pass across bundled + recipes beyond the nine audited sites — see the rule module's docstring. + """ + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + recipe = _recipe_with(_plan_step(on_context_limit=None)) + findings = _rule_findings(recipe) + + assert len(findings) == 1, ( + f"Rule must fire when on_context_limit is missing for a contract-recovery-capable " + f"step. All findings: {[(f.rule, f.severity) for f in run_semantic_rules(recipe)]}" + ) + assert findings[0].severity == Severity.WARNING + assert findings[0].step_name == "make_plan" + assert "make-a-plan" in findings[0].message + + +def test_fires_when_on_context_limit_is_decorative_alias_of_on_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Rule fires WARNING when on_context_limit is set but equals on_failure (no real salvage).""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + recipe = _recipe_with(_plan_step(on_context_limit="escalate_stop", on_failure="escalate_stop")) + findings = _rule_findings(recipe) + + assert len(findings) == 1, "Rule must fire when on_context_limit == on_failure (decorative)" + assert findings[0].step_name == "make_plan" + + +def test_does_not_fire_when_salvage_route_exists_and_differs( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Rule does NOT fire when on_context_limit is set and differs from on_failure.""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + recipe = _recipe_with( + _plan_step(on_context_limit="salvage_plan", on_failure="escalate_stop"), + extra_steps={ + "salvage_plan": RecipeStep( + tool="run_python", + with_args={"callable": "verify_plan_artifacts"}, + on_success="next", + on_failure="escalate_stop", + ) + }, + ) + findings = _rule_findings(recipe) + + assert findings == [], f"Rule must NOT fire when a distinct salvage route exists: {findings}" + + +def test_does_not_fire_when_skill_is_read_only(monkeypatch: pytest.MonkeyPatch) -> None: + """Rule does NOT fire for read_only skills — they can't trigger contract_recovery.""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + step = RecipeStep( + name="review", + tool="run_skill", + with_args={"skill_command": f"/autoskillit:{_READ_ONLY_SKILL}"}, + on_success="next", + on_failure="escalate_stop", + on_context_limit=None, + ) + recipe = Recipe( + name="test", + description="test", + kitchen_rules=["test"], + steps={ + "review": step, + "next": RecipeStep(tool="run_cmd", with_args={"cmd": "echo next"}), + "escalate_stop": RecipeStep(action="stop", message="escalation"), + }, + ) + findings = _rule_findings(recipe) + + assert findings == [], f"Rule must NOT fire for a read_only skill: {findings}" + + +def test_does_not_fire_when_skill_has_no_expected_output_patterns( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Rule does NOT fire when the skill's contract declares no expected_output_patterns.""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + step = RecipeStep( + name="chat", + tool="run_skill", + with_args={"skill_command": f"/autoskillit:{_NO_PATTERNS_SKILL}"}, + on_success="next", + on_failure="escalate_stop", + on_context_limit=None, + ) + recipe = Recipe( + name="test", + description="test", + kitchen_rules=["test"], + steps={ + "chat": step, + "next": RecipeStep(tool="run_cmd", with_args={"cmd": "echo next"}), + "escalate_stop": RecipeStep(action="stop", message="escalation"), + }, + ) + findings = _rule_findings(recipe) + + assert findings == [], f"Rule must NOT fire when expected_output_patterns is empty: {findings}" + + +def test_does_not_fire_when_step_is_itself_an_on_context_limit_target( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Rule does NOT fire on a step that is itself another step's on_context_limit target.""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + salvage_step = RecipeStep( + name="salvage_plan", + tool="run_skill", + with_args={"skill_command": f"/autoskillit:{_RECOVERY_CAPABLE_SKILL}"}, + on_success="next", + on_failure="escalate_stop", + on_context_limit=None, + ) + other_step = _plan_step(on_context_limit="salvage_plan", on_failure="escalate_stop") + recipe = _recipe_with(other_step, extra_steps={"salvage_plan": salvage_step}) + + findings = _rule_findings(recipe) + + assert findings == [], ( + f"Rule must NOT fire on a step that is itself an on_context_limit target: {findings}" + ) + + +def test_does_not_fire_when_step_action_is_stop(monkeypatch: pytest.MonkeyPatch) -> None: + """Rule does NOT fire on stop-action steps (terminal, no routing needed).""" + monkeypatch.setattr(_rules_contract_recovery, "load_bundled_manifest", lambda: _MANIFEST) + + step = RecipeStep( + name="make_plan", + tool="run_skill", + action="stop", + message="done", + with_args={"skill_command": f"/autoskillit:{_RECOVERY_CAPABLE_SKILL}"}, + ) + recipe = Recipe( + name="test", + description="test", + kitchen_rules=["test"], + steps={"make_plan": step}, + ) + findings = _rule_findings(recipe) + + assert findings == [], f"Rule must NOT fire on a stop-action step: {findings}" diff --git a/tests/recipe/test_rules_rate_limit_parity.py b/tests/recipe/test_rules_rate_limit_parity.py index 39285f7e6..acbc3a437 100644 --- a/tests/recipe/test_rules_rate_limit_parity.py +++ b/tests/recipe/test_rules_rate_limit_parity.py @@ -37,17 +37,43 @@ def _has_run_skill_steps(recipe_path: Path) -> bool: return any(step.tool == "run_skill" for step in recipe.steps.values()) +def _missing_rate_limit_steps(recipe_path: Path) -> set[str]: + """Return run_skill step names missing on_rate_limit in the raw YAML. + + Mirrors the run-skill-missing-rate-limit rule's own carve-out: steps that + are themselves on_rate_limit targets (or action == "stop") are exempt. + """ + recipe = load_recipe(recipe_path) + rate_limit_targets: set[str] = set() + for step in recipe.steps.values(): + if step.on_rate_limit and step.on_rate_limit not in ( + "escalate", + "release_issue_failure", + ): + rate_limit_targets.add(step.on_rate_limit) + + missing: set[str] = set() + for name, step in recipe.steps.items(): + if step.tool != "run_skill": + continue + if step.action == "stop": + continue + if name in rate_limit_targets: + continue + if step.on_rate_limit is None: + missing.add(name) + return missing + + class TestRateLimitRuleFiring: """Verify the new semantic rule fires correctly across bundled recipes.""" @pytest.mark.parametrize("recipe_path", _BUNDLED_ONLY, ids=lambda p: p.stem) def test_rule_does_not_block_validation(self, recipe_path: Path) -> None: """The run-skill-missing-rate-limit rule fires WARNING findings for - any run_skill step missing on_rate_limit in the raw YAML, and stays - silent once every run_skill step has on_rate_limit. WARNING severity - must NOT block recipe validation — the allowlist ratchet - (``_RATE_LIMIT_COMPLIANT_RECIPES``) in - ``test_bundled_recipes_behavioral_properties`` is the hard gate. + exactly the run_skill steps missing on_rate_limit in the raw YAML, and + stays silent once every run_skill step has on_rate_limit. WARNING + severity must NOT block recipe validation. The :func:`validate_from_path` API returns ``dict[str, Any]``. The ``"findings"`` value is ``list[dict[str, str]]`` with keys ``"rule"``, @@ -59,10 +85,6 @@ def test_rule_does_not_block_validation(self, recipe_path: Path) -> None: research-archive, promote-to-main, research-campaign) are inherently compliant — the rule has nothing to flag and zero findings is correct. """ - from tests.recipe.test_bundled_recipes_behavioral_properties import ( - _RATE_LIMIT_COMPLIANT_RECIPES, - ) - stem = recipe_path.stem if not _has_run_skill_steps(recipe_path): # No run_skill steps means the rule has nothing to flag. @@ -74,18 +96,19 @@ def test_rule_does_not_block_validation(self, recipe_path: Path) -> None: for f in result["findings"] if f["rule"] == "run-skill-missing-rate-limit" and f["severity"] == "warning" ] - # The rule's presence/absence depends on whether the recipe is in - # _RATE_LIMIT_COMPLIANT_RECIPES (set in - # test_bundled_recipes_behavioral_properties.py). At time of writing, - # that set is empty (Part B adds compliance), so non-compliant - # recipes must produce at least one warning. - if stem in _RATE_LIMIT_COMPLIANT_RECIPES: + expected_missing = _missing_rate_limit_steps(recipe_path) + if not expected_missing: assert not rate_limit_warnings, ( f"{stem}: expected zero run-skill-missing-rate-limit findings " - f"for a compliant recipe, got {rate_limit_warnings}" + f"for a fully-compliant recipe, got {rate_limit_warnings}" ) else: assert rate_limit_warnings, ( - f"{stem}: expected run-skill-missing-rate-limit findings to " - f"document current non-compliance, got none" + f"{stem}: expected run-skill-missing-rate-limit findings for " + f"{expected_missing}, got none" + ) + warned_steps = {f["step"] for f in rate_limit_warnings} + assert warned_steps == expected_missing, ( + f"{stem}: run-skill-missing-rate-limit warned steps {warned_steps} " + f"do not match expected {expected_missing}" ) diff --git a/tests/server/test_response_backstop.py b/tests/server/test_response_backstop.py index aa909d8bd..7ad1c292d 100644 --- a/tests/server/test_response_backstop.py +++ b/tests/server/test_response_backstop.py @@ -616,7 +616,7 @@ def test_delivery_bound_summary_drops_diagram_when_needed(tmp_path): def test_over_ceiling_payload_fails_even_when_over_delivery_bound(tmp_path): """Pin restored ordering: an over-ceiling exempted payload must fail with exemption_ceiling_exceeded, not route to delivery-bound spill.""" - payload = {"success": True, "content": "x" * 190_000} + payload = {"success": True, "content": "x" * 200_000} original = json.dumps(payload) result = enforce_response_budget( original, diff --git a/tests/server/test_serve_idempotence.py b/tests/server/test_serve_idempotence.py index 88218e6ba..0ac1d6916 100644 --- a/tests/server/test_serve_idempotence.py +++ b/tests/server/test_serve_idempotence.py @@ -114,7 +114,6 @@ async def test_load_recipe_after_open_kitchen_without_overrides_serves_identical monkeypatch, ) assert ok_result.get("success") is True, f"open_kitchen failed: {ok_result}" - ok_content = ok_result["content"] assert tool_ctx_kitchen_open.session_serve_overrides == {}, ( "session_serve_overrides must be empty dict (not None) when no overrides passed" @@ -124,13 +123,27 @@ async def test_load_recipe_after_open_kitchen_without_overrides_serves_identical ) lr_result = json.loads(await load_recipe(name=_RECIPE)) - assert "content" in lr_result, f"load_recipe missing content: {lr_result}" - lr_content = lr_result["content"] - assert ok_content == lr_content, ( - "load_recipe content diverges from open_kitchen content (no-override path) — " - "session_serve_overrides baseline not applied" - ) + if "content" in ok_result and "content" in lr_result: + assert ok_result["content"] == lr_result["content"], ( + "load_recipe content diverges from open_kitchen content (no-override path) — " + "session_serve_overrides baseline not applied" + ) + else: + # The unresolved (no-overrides) serving of this recipe is large enough + # that one or both surfaces substitute the bounded step-flow-skeleton + # envelope for full content (server/_serve_helpers.py + # build_recipe_envelope) — open_kitchen's pre-envelope payload carries + # extra routing metadata (build_open_kitchen_recipe_payload) that + # load_recipe's doesn't, so the two surfaces can cross the delivery + # bound independently. Prove idempotence via the recipe-derived + # routing fields populated identically in both representations. + assert ok_result.get("post_prune_step_names") == lr_result.get("post_prune_step_names"), ( + "load_recipe post_prune_step_names diverges from open_kitchen (no-override path)" + ) + assert ok_result.get("post_prune_routing_edges") == lr_result.get( + "post_prune_routing_edges" + ), "load_recipe post_prune_routing_edges diverges from open_kitchen (no-override path)" async def test_deferred_recall_open_kitchen_serves_identical_to_first_serving( @@ -467,7 +480,22 @@ async def test_load_recipe_routing_matches_open_kitchen_for_arbitrary_overrides( if "content" not in lr_result: assume(False) # discard: load_recipe failed - assert lr_result["content"] == ok_result["content"], ( - f"Routing divergence for overrides={overrides!r}: " - "load_recipe content must match open_kitchen content" - ) + if "content" in ok_result: + assert lr_result["content"] == ok_result["content"], ( + f"Routing divergence for overrides={overrides!r}: " + "load_recipe content must match open_kitchen content" + ) + else: + # open_kitchen's pre-envelope payload carries extra routing metadata + # (build_open_kitchen_recipe_payload) that load_recipe's doesn't, so + # for a recipe near the delivery bound the two surfaces can cross + # independently and open_kitchen may substitute the bounded + # step-flow-skeleton envelope while load_recipe still serves full + # content. Prove idempotence via the recipe-derived routing fields + # populated identically in both representations. + assert ok_result.get("post_prune_step_names") == lr_result.get("post_prune_step_names"), ( + f"Routing divergence for overrides={overrides!r}: post_prune_step_names mismatch" + ) + assert ok_result.get("post_prune_routing_edges") == lr_result.get( + "post_prune_routing_edges" + ), f"Routing divergence for overrides={overrides!r}: post_prune_routing_edges mismatch" diff --git a/tests/server/test_tools_recipe_pull.py b/tests/server/test_tools_recipe_pull.py index 8d1c77933..36b418956 100644 --- a/tests/server/test_tools_recipe_pull.py +++ b/tests/server/test_tools_recipe_pull.py @@ -194,7 +194,7 @@ def test_envelope_fits_every_backend_by_construction( assert actual_step["edges"] == expected_step["edges"] assert actual_step.get("byte_range") == expected_step.get("byte_range") if expected_step.get("summary"): - assert expected_step["summary"].startswith(actual_step["summary"]) + assert expected_step["summary"].startswith(actual_step.get("summary", "")) def test_envelope_carries_priority_fields_verbatim(tmp_path: Path) -> None: