diff --git a/docs/README.md b/docs/README.md index 0ac0e2b8c5..d91eabd3d9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -2,7 +2,7 @@ AutoSkillit is a Claude Code plugin that runs YAML recipes through a multi-level orchestrator. The bundled recipes implement issue → plan → worktree -→ tests → PR → merge pipelines using 58 MCP tools and 142 bundled skills. +→ tests → PR → merge pipelines using 59 MCP tools and 142 bundled skills. ## Start here diff --git a/docs/execution/architecture.md b/docs/execution/architecture.md index ed7a8b5d29..02f43e83e1 100644 --- a/docs/execution/architecture.md +++ b/docs/execution/architecture.md @@ -4,7 +4,7 @@ How AutoSkillit runs a recipe end to end: orchestrator, kitchen gating, clone an ## Overview -AutoSkillit is a Claude Code plugin that orchestrates automated workflows using headless sessions. It provides 58 MCP tools and 142 bundled skills, organized into a gated visibility system. +AutoSkillit is a Claude Code plugin that orchestrates automated workflows using headless sessions. It provides 59 MCP tools and 142 bundled skills, organized into a gated visibility system. ## Core Concepts @@ -68,7 +68,7 @@ AutoSkillit supports four session modes with different tool and skill visibility `$ claude`); `/open-kitchen` reveals kitchen tools. - **`$ autoskillit order`**: Pipeline orchestrator session. Kitchen is pre-opened at startup — - all 58 MCP tools are available immediately. All skill tiers are accessible. The orchestrator + all 59 MCP tools are available immediately. All skill tiers are accessible. The orchestrator delegates work through `run_skill` (headless sessions) and `run_cmd` (shell commands). - **`run_skill` (headless)**: Worker sessions launched by the orchestrator. Sees 4 Free Range diff --git a/docs/execution/tool-access.md b/docs/execution/tool-access.md index 2ed5fddb5e..3263e25240 100644 --- a/docs/execution/tool-access.md +++ b/docs/execution/tool-access.md @@ -1,6 +1,6 @@ # MCP Tool Access Control -AutoSkillit provides 58 MCP tools organized into three access levels that control which +AutoSkillit provides 59 MCP tools organized into three access levels that control which session types can see each tool. ## Three Access Levels @@ -86,7 +86,7 @@ missing kitchen visibility. ## Complete MCP Tool Access Control Map -All 58 tools with their access level, tags, source file, and functional category. +All 59 tools with their access level, tags, source file, and functional category. **Tag abbreviations**: AS = `autoskillit`, K = `kitchen`, HL = `headless`, GH = `github`, CI = `ci`, CL = `clone`, TL = `telemetry`, FL = `fleet` diff --git a/docs/skills/visibility.md b/docs/skills/visibility.md index da2ad3dd9f..fc59e9840f 100644 --- a/docs/skills/visibility.md +++ b/docs/skills/visibility.md @@ -96,7 +96,7 @@ and `/autoskillit:close-kitchen`. Skills in `skills_extended/` are never seen. Order is similar to cook: AutoSkillit launches Claude Code with access to all tiers. The key difference is the orchestrator (`sous-chef` skill) is injected and the kitchen -is pre-opened so all 58 MCP tools are available from the start. +is pre-opened so all 59 MCP tools are available from the start. ### Headless session (launched by `run_skill`) diff --git a/src/autoskillit/cli/_prompts_orchestrator.py b/src/autoskillit/cli/_prompts_orchestrator.py index 62f78f741e..0de34122a3 100644 --- a/src/autoskillit/cli/_prompts_orchestrator.py +++ b/src/autoskillit/cli/_prompts_orchestrator.py @@ -243,6 +243,10 @@ def _build_orchestrator_prompt( via CLI). Partial progress may exist on disk. Follow on_context_limit. - If the progress signal is false OR the step has no on_context_limit: fall through to on_failure — no recoverable progress evidence. +- When run_skill returns "needs_retry: true" AND "retry_reason: outcome_invariant": + - The skill's contract-declared outcome invariant was violated (e.g. accept_count > 0 + but fix_failures > 0). This is a logical failure — the skill ran but its core function + failed. Always fall through to on_failure. Do NOT route to on_context_limit. - When run_skill returns "needs_retry: true" AND "retry_reason: contract_recovery": - The model ran to completion and wrote artifacts but the structured output tokens failed pattern validation. Infrastructure nudge was attempted but could not recover. diff --git a/src/autoskillit/config/ingredient_defaults.py b/src/autoskillit/config/ingredient_defaults.py index 25d8824b8b..1dce9b5b11 100644 --- a/src/autoskillit/config/ingredient_defaults.py +++ b/src/autoskillit/config/ingredient_defaults.py @@ -30,6 +30,7 @@ "create_and_publish_branch", "check_pr_mergeable", "set_commit_status", + "commit_files", ), ), ("Recipes", ("migrate_recipe", "list_recipes", "load_recipe", "validate_recipe")), diff --git a/src/autoskillit/core/__init__.pyi b/src/autoskillit/core/__init__.pyi index 2687eab6ae..46736c186f 100644 --- a/src/autoskillit/core/__init__.pyi +++ b/src/autoskillit/core/__init__.pyi @@ -362,6 +362,7 @@ from .types import SessionTelemetry as SessionTelemetry from .types import SessionType as SessionType from .types import Severity as Severity from .types import SkillCapabilityDef as SkillCapabilityDef +from .types import SkillContractResolver as SkillContractResolver from .types import SkillFamilyDef as SkillFamilyDef from .types import SkillLister as SkillLister from .types import SkillResolver as SkillResolver diff --git a/src/autoskillit/core/types/_type_constants_registries.py b/src/autoskillit/core/types/_type_constants_registries.py index 0ffd65e6cc..eea0ce5c23 100644 --- a/src/autoskillit/core/types/_type_constants_registries.py +++ b/src/autoskillit/core/types/_type_constants_registries.py @@ -124,7 +124,7 @@ } ) -HEADLESS_TOOLS: frozenset[str] = frozenset({"test_check", "unlock_agent_pack"}) +HEADLESS_TOOLS: frozenset[str] = frozenset({"test_check", "unlock_agent_pack", "commit_files"}) FLEET_TOOLS: frozenset[str] = frozenset( { @@ -323,6 +323,7 @@ class AgentPackDef(NamedTuple): "reset_test_dir": frozenset({"kitchen-core"}), "reset_workspace": frozenset({"kitchen-core"}), "classify_fix": frozenset({"kitchen-core"}), + "commit_files": frozenset({"kitchen-core"}), "list_recipes": frozenset({"kitchen-core", "fleet-dispatch"}), "load_recipe": frozenset({"kitchen-core", "fleet-dispatch"}), "validate_recipe": frozenset({"kitchen-core"}), @@ -433,6 +434,10 @@ class HardCapabilityMismatch(NamedTuple): codex_status="not-applicable", worker_routable=True, ), + "commit_files": SkillCapabilityDef( + description="commit_files MCP tool — server-side git stage/commit", + codex_status="works-as-is", + ), "git_metadata_write": SkillCapabilityDef( description=( "Requires .git/ metadata write access (git commit, git rebase, " diff --git a/src/autoskillit/core/types/_type_enums.py b/src/autoskillit/core/types/_type_enums.py index 193df51d3d..365c788796 100644 --- a/src/autoskillit/core/types/_type_enums.py +++ b/src/autoskillit/core/types/_type_enums.py @@ -62,6 +62,9 @@ class RetryReason(StrEnum): IDLE_STALL = "idle_stall" # stdout idle watchdog kill — session may have partial progress RATE_LIMITED = "rate_limited" # transient HTTP 429 or rate-limit pattern — wait-and-retry CANCELLED = "cancelled" + OUTCOME_INVARIANT = ( + "outcome_invariant" # skill-emitted outcome fields violated a contract invariant + ) class InfraExitCategory(StrEnum): diff --git a/src/autoskillit/core/types/_type_protocols_execution.py b/src/autoskillit/core/types/_type_protocols_execution.py index b29e5d439a..f2f0701e03 100644 --- a/src/autoskillit/core/types/_type_protocols_execution.py +++ b/src/autoskillit/core/types/_type_protocols_execution.py @@ -4,7 +4,7 @@ from collections.abc import Callable, Mapping, Sequence from pathlib import Path -from typing import Protocol, runtime_checkable +from typing import Any, Protocol, runtime_checkable from ._type_checkpoint import SessionCheckpoint # noqa: F401, TC001 from ._type_results import ( @@ -84,6 +84,7 @@ async def run( network_access: bool = False, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: Any | None = None, ) -> SkillResult: ... async def dispatch_food_truck( diff --git a/src/autoskillit/core/types/_type_protocols_recipe.py b/src/autoskillit/core/types/_type_protocols_recipe.py index 31fbb6f817..1697c310e8 100644 --- a/src/autoskillit/core/types/_type_protocols_recipe.py +++ b/src/autoskillit/core/types/_type_protocols_recipe.py @@ -7,6 +7,7 @@ from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable if TYPE_CHECKING: + from autoskillit.recipe._contracts_types import SkillContract from autoskillit.recipe.schema import Recipe, RecipeInfo from ._type_backend import BackendCapabilities @@ -24,6 +25,7 @@ "MigrationService", "DatabaseReader", "ReadOnlyResolver", + "SkillContractResolver", "ServeOverridesSnapshot", ] @@ -131,3 +133,10 @@ class ReadOnlyResolver(Protocol): """Protocol for resolving whether a skill is read-only from skill contracts.""" def __call__(self, skill_command: str) -> bool: ... + + +@runtime_checkable +class SkillContractResolver(Protocol): + """Protocol for resolving a skill's full contract from skill contracts.""" + + def __call__(self, skill_command: str) -> SkillContract | None: ... diff --git a/src/autoskillit/core/types/_type_results.py b/src/autoskillit/core/types/_type_results.py index b7125273b3..5f8e8d4f5f 100644 --- a/src/autoskillit/core/types/_type_results.py +++ b/src/autoskillit/core/types/_type_results.py @@ -36,6 +36,7 @@ "ValidatedAddDir", "ValidatedWorktreePath", "VALID_INPUT_SPEC_TYPES", + "OutcomeInvariantSpec", "WriteBehaviorSpec", "WriteEvidence", "FailureRecord", @@ -146,6 +147,20 @@ def is_dir(self) -> bool: return Path(self.path).is_dir() +@dataclass(frozen=True, slots=True) +class OutcomeInvariantSpec: + """Outcome invariant evaluated against skill-emitted output fields. + + when: predicate expression, e.g. "accept_count > 0" + require: requirement expression, e.g. "fix_failures == 0" + Both use grammar: + with ops: >, >=, ==, !=, <=, < + """ + + when: str + require: str + + @dataclass(frozen=True, slots=True) class WriteBehaviorSpec: """Write-expectation metadata resolved from skill contracts. @@ -550,6 +565,9 @@ class SkillResult: ndjson_drift: NdjsonDriftOutcome = field(default_factory=NdjsonDriftOutcome) """NDJSON parser vocabulary drift counters — populated by Codex sessions.""" completion_required: bool = False + outcome_fields: dict[str, int | str] | None = None + outcome_invariant_violated: bool = False + outcome_qualifier: str | None = None def to_json(self) -> str: data: dict[str, Any] = { @@ -828,4 +846,7 @@ class SessionIndexEntry(TypedDict): api_retry_last_status: int | None ndjson_unknown_event_count: int ndjson_unknown_item_count: int + outcome_fields: dict[str, int | str] | None + outcome_invariant_violated: bool + outcome_qualifier: str | None schema_version: int diff --git a/src/autoskillit/execution/headless/AGENTS.md b/src/autoskillit/execution/headless/AGENTS.md index b570d72194..fc5e641e39 100644 --- a/src/autoskillit/execution/headless/AGENTS.md +++ b/src/autoskillit/execution/headless/AGENTS.md @@ -14,6 +14,7 @@ Headless Claude session orchestration — command prep, subprocess invocation, r | `_headless_recovery.py` | Session recovery: `_recover_from_separate_marker`, `_synthesize_from_write_artifacts` | | `_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 | | `_headless_scan.py` | `_scan_jsonl_write_paths()` — scans stdout JSONL for Write/Edit/Bash tool calls; uses `core/bash_write_targets` for precise write-target extraction | ## Architecture Notes diff --git a/src/autoskillit/execution/headless/__init__.py b/src/autoskillit/execution/headless/__init__.py index 3170796ebf..05f23495fe 100644 --- a/src/autoskillit/execution/headless/__init__.py +++ b/src/autoskillit/execution/headless/__init__.py @@ -92,6 +92,7 @@ if TYPE_CHECKING: from autoskillit.pipeline.context import ToolContext + from autoskillit.recipe._contracts_types import SkillContract __all__ = [ "DefaultHeadlessExecutor", @@ -148,6 +149,7 @@ async def run_headless_core( network_access: bool = False, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: SkillContract | None = None, ) -> SkillResult: """Shared headless runner used by run_skill. @@ -263,6 +265,7 @@ async def run_headless_core( backend_override_source=backend_override_source, closure_spec=closure_spec, closure_report_root=closure_report_root, + skill_contract=skill_contract, ) @@ -314,6 +317,7 @@ async def run( network_access: bool = False, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: SkillContract | None = None, ) -> SkillResult: cfg = self._ctx.config.run_skill effective_timeout = timeout if timeout is not None else cfg.timeout @@ -359,6 +363,7 @@ async def run( network_access=network_access, closure_spec=closure_spec, closure_report_root=closure_report_root, + skill_contract=skill_contract, ) async def dispatch_food_truck( diff --git a/src/autoskillit/execution/headless/_headless_execute.py b/src/autoskillit/execution/headless/_headless_execute.py index 59a430cf87..dc3ab6e995 100644 --- a/src/autoskillit/execution/headless/_headless_execute.py +++ b/src/autoskillit/execution/headless/_headless_execute.py @@ -66,6 +66,7 @@ if TYPE_CHECKING: from autoskillit.core import SubprocessResult from autoskillit.pipeline.context import ToolContext + from autoskillit.recipe._contracts_types import SkillContract logger = get_logger(__name__) @@ -116,6 +117,7 @@ async def _execute_claude_headless( on_session_id_resolved: Callable[[str], None] | None = None, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: SkillContract | None = None, ) -> SkillResult: """Shared subprocess execution for headless Claude sessions. @@ -444,6 +446,7 @@ async def _execute_claude_headless( readonly_skill=_readonly_skill, closure_spec=closure_spec, closure_report_root=closure_report_root, + skill_contract=skill_contract, ) if ( @@ -588,6 +591,9 @@ async def _execute_claude_headless( backend=cast(Literal["claude-code", "codex"], _step_backend.name), channel_b_capable=_step_backend.capabilities.channel_b_capable, backend_override_source=backend_override_source, + outcome_fields=skill_result.outcome_fields, + outcome_invariant_violated=skill_result.outcome_invariant_violated, + outcome_qualifier=skill_result.outcome_qualifier, ) except Exception: logger.debug("session_log_flush_failed", exc_info=True) diff --git a/src/autoskillit/execution/headless/_headless_outcome.py b/src/autoskillit/execution/headless/_headless_outcome.py new file mode 100644 index 0000000000..019598d2af --- /dev/null +++ b/src/autoskillit/execution/headless/_headless_outcome.py @@ -0,0 +1,155 @@ +"""Contract-field parser and outcome invariant evaluator. + +Extracts declared output fields from session result text as ``KEY = value`` +lines, typed per the contract's output declarations. Evaluates outcome +invariants and success qualifiers against parsed fields. +""" + +from __future__ import annotations + +import operator +from typing import TYPE_CHECKING, Any + +import regex as re + +from autoskillit.core import get_logger + +if TYPE_CHECKING: + from autoskillit.recipe._contracts_types import ( + OutcomeInvariantEntry, + SkillContract, + SuccessQualifierEntry, + ) + +logger = get_logger(__name__) + +_FIELD_RE = re.compile(r"^(\w+)\s*=\s*(.+)$", re.MULTILINE) + +_OPS: dict[str, Any] = { + ">": operator.gt, + ">=": operator.ge, + "==": operator.eq, + "!=": operator.ne, + "<=": operator.le, + "<": operator.lt, +} + +_EXPR_RE = re.compile(r"^(\w+)\s*(>=|<=|!=|==|>|<)\s*(\d+)$") + + +def parse_outcome_fields( + result_text: str, + contract: SkillContract, +) -> dict[str, int | str]: + """Extract declared output fields from the trailing contiguous block. + + Only fields declared in the contract's ``outputs`` are extracted. + Walks backward from the end of the text, skipping non-field trailing + lines (gate tags, blank lines), then collects the contiguous run of + ``KEY = value`` lines. This ensures earlier prose containing field-like + syntax cannot overwrite the authoritative final output block. + + Integer-typed fields are parsed to int; malformed integer values are + recorded as the raw string (the invariant evaluator treats missing/ + malformed fields as violations when referenced by a ``require``). + """ + declared = {o.name: o.type for o in contract.outputs} + lines = result_text.splitlines() + field_lines: list[str] = [] + collecting = False + for line in reversed(lines): + if not collecting: + if _FIELD_RE.match(line): + collecting = True + field_lines.append(line) + # else: skip trailing non-field lines (gate tags, blanks) + else: + if _FIELD_RE.match(line): + field_lines.append(line) + else: + break + + parsed: dict[str, int | str] = {} + for line in field_lines: + m = _FIELD_RE.match(line) + if not m: + continue + name, raw_value = m.group(1), m.group(2).strip() + if name not in declared: + continue + if declared[name] == "integer": + try: + parsed[name] = int(raw_value) + except ValueError: + parsed[name] = raw_value + else: + parsed[name] = raw_value + return parsed + + +def _eval_expr(expr: str, fields: dict[str, int | str]) -> bool | None: + """Evaluate a simple comparison expression against parsed fields. + + Returns True/False on successful evaluation, None if the referenced + field is missing or non-numeric. + """ + m = _EXPR_RE.match(expr) + if not m: + return None + field_name, op_str, literal = m.group(1), m.group(2), int(m.group(3)) + value = fields.get(field_name) + if not isinstance(value, int): + return None + return _OPS[op_str](value, literal) + + +def _eval_compound_expr(expr: str, fields: dict[str, int | str]) -> bool | None: + """Evaluate a compound expression with ``and`` connectives. + + Each sub-expression is a simple comparison. All must be true for the + compound to be true. Returns None if any sub-expression references a + missing/non-numeric field. + """ + parts = [p.strip() for p in expr.split(" and ")] + results = [_eval_expr(p, fields) for p in parts] + if any(r is None for r in results): + return None + return all(results) + + +def evaluate_outcome_invariants( + fields: dict[str, int | str], + invariants: list[OutcomeInvariantEntry], +) -> tuple[bool, str]: + """Evaluate outcome invariants against parsed fields. + + Returns (violated, detail). ``violated`` is True if any invariant's + ``when`` predicate is satisfied but its ``require`` condition is not. + A missing ``require`` field when ``when`` is true is a violation + (fail-closed). A missing ``when`` field causes the invariant to be + skipped (the field was never emitted — legitimate no-PR-found exit). + """ + for inv in invariants: + when_result = _eval_compound_expr(inv.when, fields) + if when_result is None or not when_result: + continue + require_result = _eval_compound_expr(inv.require, fields) + if require_result is None or not require_result: + return True, f"invariant violated: when '{inv.when}' require '{inv.require}'" + return False, "" + + +def evaluate_success_qualifier( + fields: dict[str, int | str], + qualifiers: list[SuccessQualifierEntry], +) -> str | None: + """Evaluate success qualifiers against parsed fields. + + Returns the qualifier string if any qualifier's ``when`` predicate + matches, or None if no qualifier applies. + """ + for sq in qualifiers: + when_result = _eval_compound_expr(sq.when, fields) + if when_result is not None and when_result: + return sq.qualifier + return None diff --git a/src/autoskillit/execution/headless/_headless_result.py b/src/autoskillit/execution/headless/_headless_result.py index 3a235cf64a..408595a307 100644 --- a/src/autoskillit/execution/headless/_headless_result.py +++ b/src/autoskillit/execution/headless/_headless_result.py @@ -36,6 +36,10 @@ _extract_file_changes, _stdout_mentions_write_tools, ) +from autoskillit.execution.headless._headless_outcome import ( + evaluate_outcome_invariants, + parse_outcome_fields, +) from autoskillit.execution.headless._headless_path_tokens import ( _extract_branch_name, _extract_output_paths, @@ -68,6 +72,7 @@ if TYPE_CHECKING: from autoskillit.core import AuditLog, CodingAgentBackend, SubprocessResult + from autoskillit.recipe._contracts_types import SkillContract logger = get_logger(__name__) @@ -160,6 +165,54 @@ def _has_out_of_cwd_file_change(file_changes: Sequence[str], cwd: str) -> bool: return False +def _apply_post_session_adjudication( + sr: SkillResult, + evidence: WriteEvidence, + write_behavior: WriteBehaviorSpec | None, + skill_contract: SkillContract | None, +) -> SkillResult: + """Consolidated post-session adjudication: zero-write gate + outcome invariants. + + Invoked as the last adjudication step before each success-finalizing + return. Makes "a success path that skips adjudication" unrepresentable. + """ + if not sr.success: + return sr + + if not evidence.has_implementation_evidence and write_behavior is not None: + write_expected = False + if write_behavior.mode == "always": + write_expected = True + elif write_behavior.mode == "conditional" and write_behavior.expected_when: + write_expected = _check_expected_patterns( + sr.result, + write_behavior.expected_when, + ) + if write_expected: + return dataclasses.replace( + sr, + success=False, + subtype="zero_writes", + needs_retry=True, + retry_reason=RetryReason.ZERO_WRITES, + ) + + if skill_contract is not None and skill_contract.outcome_invariants: + fields = parse_outcome_fields(sr.result, skill_contract) + violated, detail = evaluate_outcome_invariants(fields, skill_contract.outcome_invariants) + if violated: + logger.warning("outcome_invariant_violated", detail=detail) + return dataclasses.replace( + sr, + success=False, + subtype="outcome_invariant_violation", + needs_retry=True, + retry_reason=RetryReason.OUTCOME_INVARIANT, + ) + + return sr + + def _build_skill_result( result: SubprocessResult, completion_marker: str = "", @@ -181,9 +234,11 @@ def _build_skill_result( readonly_skill: bool = False, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: SkillContract | None = None, ) -> SkillResult: """Route SubprocessResult fields into the standard run_skill response.""" file_changes = _extract_file_changes(result.stdout, backend) + branch = ( "idle_stall" if result.termination == TerminationReason.IDLE_STALL @@ -249,26 +304,9 @@ def _build_skill_result( provider_used=provider_used, api_retry=stale_api_retry, ) - if ( - _stale_success_sr.success - and write_behavior is not None - and not stale_evidence.has_implementation_evidence - ): - _stale_write_expected = write_behavior.mode == "always" or ( - write_behavior.mode == "conditional" - and write_behavior.expected_when - and _check_expected_patterns( - _stale_success_sr.result, write_behavior.expected_when - ) - ) - if _stale_write_expected: - _stale_success_sr = dataclasses.replace( - _stale_success_sr, - success=False, - subtype="zero_writes", - needs_retry=True, - retry_reason=RetryReason.ZERO_WRITES, - ) + _stale_success_sr = _apply_post_session_adjudication( + _stale_success_sr, stale_evidence, write_behavior, skill_contract + ) return _stale_success_sr # No valid result in stdout — fall through to original stale response _stale_is_rate_limited = has_rate_limit_signal(stale_session, result) @@ -356,26 +394,9 @@ def _build_skill_result( provider_used=provider_used, api_retry=idle_api_retry, ) - if ( - _idle_success_sr.success - and write_behavior is not None - and not idle_evidence.has_implementation_evidence - ): - _idle_write_expected = write_behavior.mode == "always" or ( - write_behavior.mode == "conditional" - and write_behavior.expected_when - and _check_expected_patterns( - _idle_success_sr.result, write_behavior.expected_when - ) - ) - if _idle_write_expected: - _idle_success_sr = dataclasses.replace( - _idle_success_sr, - success=False, - subtype="zero_writes", - needs_retry=True, - retry_reason=RetryReason.ZERO_WRITES, - ) + _idle_success_sr = _apply_post_session_adjudication( + _idle_success_sr, idle_evidence, write_behavior, skill_contract + ) return _idle_success_sr _idle_is_rate_limited = has_rate_limit_signal(idle_session, result) _idle_is_api_error = idle_session.api_retry_exhausted or ( @@ -647,18 +668,6 @@ def _build_skill_result( needs_retry = False retry_reason = RetryReason.NONE - if ( - success - and write_behavior is not None - and write_behavior.mode == "always" - and not evidence.has_implementation_evidence - ): - normalized_subtype = "zero_writes" - outcome = SessionOutcome.RETRIABLE - success = False - needs_retry = True - retry_reason = RetryReason.ZERO_WRITES - # Invariant: TIMED_OUT sessions must produce subtype='timeout'. if ( result.termination == TerminationReason.TIMED_OUT @@ -836,26 +845,7 @@ def _build_skill_result( ) sr = _apply_budget_guard(sr, skill_command, audit, max_consecutive_retries) - # Zero-write gate: demote success to retriable failure when a write-expected - # skill produced zero Edit/Write calls (silent degradation detection). - # Write expectation is resolved from skill_contracts.yaml via WriteBehaviorSpec. - if sr.success and not evidence.has_implementation_evidence and write_behavior is not None: - write_expected = False - if write_behavior.mode == "always": - write_expected = True - elif write_behavior.mode == "conditional" and write_behavior.expected_when: - write_expected = _check_expected_patterns( - sr.result, - write_behavior.expected_when, - ) - if write_expected: - sr = dataclasses.replace( - sr, - success=False, - subtype="zero_writes", - needs_retry=True, - retry_reason=RetryReason.ZERO_WRITES, - ) + sr = _apply_post_session_adjudication(sr, evidence, write_behavior, skill_contract) # Closure verification gate: when a ClosureAuthoritySpec is active, independently # verify the canonical closure report. On failure, demote to execution error so @@ -896,6 +886,24 @@ def _build_skill_result( ) sr = _apply_budget_guard(sr, skill_command, audit, max_consecutive_retries) + if skill_contract is not None and skill_contract.outputs: + _parsed_fields = parse_outcome_fields(sr.result, skill_contract) + _qualifier: str | None = None + if sr.success and skill_contract.success_qualifiers: + from autoskillit.execution.headless._headless_outcome import ( + evaluate_success_qualifier, + ) + + _qualifier = evaluate_success_qualifier( + _parsed_fields, skill_contract.success_qualifiers + ) + sr = dataclasses.replace( + sr, + outcome_fields=_parsed_fields if _parsed_fields else None, + outcome_invariant_violated=sr.retry_reason == RetryReason.OUTCOME_INVARIANT, + outcome_qualifier=_qualifier, + ) + logger.debug( "build_skill_result_exit", success=sr.success, diff --git a/src/autoskillit/execution/session_log.py b/src/autoskillit/execution/session_log.py index 5daddbf7e7..8e3d04f0f2 100644 --- a/src/autoskillit/execution/session_log.py +++ b/src/autoskillit/execution/session_log.py @@ -158,6 +158,9 @@ def flush_session_log( channel_b_capable: bool = True, backend_override_source: str | None = None, telemetry: SessionTelemetry, + outcome_fields: dict[str, int | str] | None = None, + outcome_invariant_violated: bool = False, + outcome_qualifier: str | None = None, ) -> None: """Flush session diagnostics to disk. @@ -588,10 +591,13 @@ def flush_session_log( "api_retry_last_status": api_retry_last_status, "ndjson_unknown_event_count": ndjson_unknown_event_count, "ndjson_unknown_item_count": ndjson_unknown_item_count, + "outcome_fields": outcome_fields, + "outcome_invariant_violated": outcome_invariant_violated, + "outcome_qualifier": outcome_qualifier, "model_identifier": effective_model_id, "configured_model": model_identity.configured_model, "profile_name": model_identity.profile_name, - "schema_version": 4, + "schema_version": 5, } index_path = log_root / "sessions.jsonl" with index_path.open("a") as f: diff --git a/src/autoskillit/pipeline/context.py b/src/autoskillit/pipeline/context.py index e9072d9b0f..04eac17cf3 100644 --- a/src/autoskillit/pipeline/context.py +++ b/src/autoskillit/pipeline/context.py @@ -38,6 +38,7 @@ RecipeRepository, ServeOverridesSnapshot, SessionSkillManager, + SkillContractResolver, SkillResolver, SubprocessRunner, TestRunner, @@ -105,6 +106,8 @@ class ToolContext: skill contracts. completion_required_resolver: CompletionRequiredResolver — resolves whether a skill requires the completion marker. + skill_contract_resolver: SkillContractResolver — resolves a skill's full contract + from skill contracts. quota_refresh_task: QuotaRefreshTask — cancellable handle for the kitchen-scoped quota refresh background task. fleet_lock: FleetLock — semaphore-style guard for concurrent fleet dispatch. @@ -162,6 +165,7 @@ class ToolContext: read_only_resolver: ReadOnlyResolver | None = field(default=None) input_contract_resolver: InputContractResolver | None = field(default=None) completion_required_resolver: CompletionRequiredResolver | None = field(default=None) + skill_contract_resolver: SkillContractResolver | None = field(default=None) backend: CodingAgentBackend | None = field(default=None) session_skill_manager: SessionSkillManager | None = field(default=None) skill_resolver: SkillResolver | None = field(default=None) diff --git a/src/autoskillit/recipe/__init__.py b/src/autoskillit/recipe/__init__.py index d19d9aceb8..3ecee72062 100644 --- a/src/autoskillit/recipe/__init__.py +++ b/src/autoskillit/recipe/__init__.py @@ -23,7 +23,11 @@ format_ingredients_table, ) from autoskillit.recipe.contracts import ( # noqa: E402 + OutcomeInvariantEntry, + SkillContract, + SkillOutput, StaleItem, + SuccessQualifierEntry, check_contract_staleness, generate_recipe_card, get_skill_contract, @@ -316,6 +320,10 @@ "resolve_input_specs", "resolve_skill_name", "validate_recipe_cards", + "OutcomeInvariantEntry", + "SkillContract", + "SkillOutput", + "SuccessQualifierEntry", "DefaultRecipeRepository", "parse_recipe_metadata", "load_and_validate", diff --git a/src/autoskillit/recipe/_contracts_manifest.py b/src/autoskillit/recipe/_contracts_manifest.py index be8423982b..eeb9999225 100644 --- a/src/autoskillit/recipe/_contracts_manifest.py +++ b/src/autoskillit/recipe/_contracts_manifest.py @@ -20,10 +20,12 @@ _CONTEXT_REF_RE, _TEMPLATE_REF_RE, INPUT_REF_RE, + OutcomeInvariantEntry, ResultFieldSpec, SkillContract, SkillInput, SkillOutput, + SuccessQualifierEntry, ToolOutputContractSpec, ToolOutputFieldSpec, ) @@ -82,6 +84,40 @@ def get_skill_contract(skill_name: str, manifest: dict[str, Any]) -> SkillContra raise KeyError( f"Malformed result_fields entry for skill '{skill_name}': missing key {exc}" ) from exc + + output_names = {o.name for o in outputs} + int_output_names = {o.name for o in outputs if o.type == "integer"} + + outcome_invariants: list[OutcomeInvariantEntry] = [] + for inv in skill_data.get("outcome_invariants", []): + w, r = inv.get("when", ""), inv.get("require", "") + if not w or not r: + raise ValueError( + f"outcome_invariants entry for skill '{skill_name}' missing 'when' or 'require'" + ) + for expr_label, expr in [("when", w), ("require", r)]: + field_name = expr.split()[0] if expr.split() else "" + if field_name not in output_names: + raise ValueError( + f"outcome_invariants.{expr_label} references undeclared output " + f"'{field_name}' in skill '{skill_name}'" + ) + if field_name not in int_output_names: + raise ValueError( + f"outcome_invariants.{expr_label} references non-integer output " + f"'{field_name}' in skill '{skill_name}'" + ) + outcome_invariants.append(OutcomeInvariantEntry(when=w, require=r)) + + success_qualifiers: list[SuccessQualifierEntry] = [] + for sq in skill_data.get("success_qualifiers", []): + w, q = sq.get("when", ""), sq.get("qualifier", "") + if not w or not q: + raise ValueError( + f"success_qualifiers entry for skill '{skill_name}' missing 'when' or 'qualifier'" + ) + success_qualifiers.append(SuccessQualifierEntry(when=w, qualifier=q)) + return SkillContract( inputs=inputs, outputs=outputs, @@ -92,6 +128,8 @@ def get_skill_contract(skill_name: str, manifest: dict[str, Any]) -> SkillContra read_only=read_only, completion_required=completion_required, result_fields=result_fields, + outcome_invariants=outcome_invariants, + success_qualifiers=success_qualifiers, ) diff --git a/src/autoskillit/recipe/_contracts_types.py b/src/autoskillit/recipe/_contracts_types.py index 88003eed48..731e7ba1fa 100644 --- a/src/autoskillit/recipe/_contracts_types.py +++ b/src/autoskillit/recipe/_contracts_types.py @@ -35,6 +35,18 @@ class ResultFieldSpec: required: bool = True +@dataclasses.dataclass +class OutcomeInvariantEntry: + when: str + require: str + + +@dataclasses.dataclass +class SuccessQualifierEntry: + when: str + qualifier: str + + @dataclasses.dataclass class SkillContract: inputs: list[SkillInput] @@ -46,6 +58,8 @@ class SkillContract: read_only: bool = False completion_required: bool = False result_fields: list[ResultFieldSpec] = dataclasses.field(default_factory=list) + outcome_invariants: list[OutcomeInvariantEntry] = dataclasses.field(default_factory=list) + success_qualifiers: list[SuccessQualifierEntry] = dataclasses.field(default_factory=list) @dataclasses.dataclass(frozen=True, slots=True) diff --git a/src/autoskillit/recipe/contracts.py b/src/autoskillit/recipe/contracts.py index 545c9e0468..df2ec1904c 100644 --- a/src/autoskillit/recipe/contracts.py +++ b/src/autoskillit/recipe/contracts.py @@ -35,12 +35,14 @@ RESULT_CAPTURE_RE, BlockFingerprint, DataFlowEntry, + OutcomeInvariantEntry, RecipeCard, ResultFieldSpec, SkillContract, SkillInput, SkillOutput, StaleItem, + SuccessQualifierEntry, ToolOutputContractSpec, ToolOutputFieldSpec, ) diff --git a/src/autoskillit/recipe/rules/rules_backend_compat.py b/src/autoskillit/recipe/rules/rules_backend_compat.py index bab97bd059..f2ad111fb5 100644 --- a/src/autoskillit/recipe/rules/rules_backend_compat.py +++ b/src/autoskillit/recipe/rules/rules_backend_compat.py @@ -44,6 +44,18 @@ def _check_backend_incompatible_skill(ctx: ValidationContext) -> list[RuleFindin skill_info = ctx.skill_resolver.resolve(skill_name) if skill_info is None: continue + if getattr(skill_info, "invalid_reason", None): + findings.append( + make_finding( + rule_name="backend-incompatible-skill", + step_name=step_name, + message=( + f"step '{step_name}': skill '{skill_name}' has invalid capabilities: " + f"{skill_info.invalid_reason}" + ), + ) + ) + continue # Per-step effective backend: when providers route a single step to # a different backend (ANTHROPIC_BASE_URL → claude-code), the global # ctx.backend_name would incorrectly flag that covered step. diff --git a/src/autoskillit/recipe/skill_contracts.yaml b/src/autoskillit/recipe/skill_contracts.yaml index 6ff8862559..21aed02c12 100644 --- a/src/autoskillit/recipe/skill_contracts.yaml +++ b/src/autoskillit/recipe/skill_contracts.yaml @@ -175,6 +175,10 @@ skills: - ci_only_failure - name: fixes_applied type: integer + - name: accept_count + type: integer + - name: fix_failures + type: integer - name: deferred_observations_path type: string description: "Written when mode=local (accumulated critical/warning DISCUSS findings, excluding info-severity) or when mode=github (posted from prior local rounds)." @@ -186,6 +190,12 @@ skills: write_behavior: conditional write_expected_when: - "verdict[ \\t]*=[ \\t]*real_fix" + outcome_invariants: + - when: "accept_count > 0" + require: "fix_failures == 0" + success_qualifiers: + - when: "accept_count > 0 and fixes_applied == 0 and fix_failures == 0" + qualifier: "accepted_without_changes" dry-walkthrough: inputs: - name: plan_path @@ -1904,16 +1914,26 @@ skills: - ci_only_failure - name: fixes_applied type: integer + - name: accept_count + type: integer + - name: fix_failures + type: integer expected_output_patterns: - "needs_rerun[ \\t]*=[ \\t]*(true|false)" pattern_examples: - - "needs_rerun = true\nverdict = real_fix\nfixes_applied = 2\n%%ORDER_UP%%" - - "needs_rerun = false\nverdict = already_green\nfixes_applied = 0\n%%ORDER_UP%%" - - "needs_rerun = true\nverdict = flake_suspected\nfixes_applied = 0\n%%ORDER_UP%%" - - "needs_rerun = false\nverdict = ci_only_failure\nfixes_applied = 0\n%%ORDER_UP%%" + - "needs_rerun = true\nverdict = real_fix\nfixes_applied = 2\naccept_count = 2\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = false\nverdict = already_green\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = true\nverdict = flake_suspected\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = false\nverdict = ci_only_failure\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" write_behavior: conditional write_expected_when: - "verdict[ \\t]*=[ \\t]*real_fix" + outcome_invariants: + - when: "accept_count > 0" + require: "fix_failures == 0" + success_qualifiers: + - when: "accept_count > 0 and fixes_applied == 0 and fix_failures == 0" + qualifier: "accepted_without_changes" resolve-research-review: inputs: @@ -1935,16 +1955,26 @@ skills: - ci_only_failure - name: fixes_applied type: integer + - name: accept_count + type: integer + - name: fix_failures + type: integer expected_output_patterns: - "needs_rerun[ \\t]*=[ \\t]*(true|false)" pattern_examples: - - "needs_rerun = true\nverdict = real_fix\nfixes_applied = 2\n%%ORDER_UP%%" - - "needs_rerun = false\nverdict = already_green\nfixes_applied = 0\n%%ORDER_UP%%" - - "needs_rerun = true\nverdict = flake_suspected\nfixes_applied = 0\n%%ORDER_UP%%" - - "needs_rerun = false\nverdict = ci_only_failure\nfixes_applied = 0\n%%ORDER_UP%%" + - "needs_rerun = true\nverdict = real_fix\nfixes_applied = 2\naccept_count = 2\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = false\nverdict = already_green\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = true\nverdict = flake_suspected\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" + - "needs_rerun = false\nverdict = ci_only_failure\nfixes_applied = 0\naccept_count = 0\nfix_failures = 0\n%%ORDER_UP%%" write_behavior: conditional write_expected_when: - "verdict[ \\t]*=[ \\t]*real_fix" + outcome_invariants: + - when: "accept_count > 0" + require: "fix_failures == 0" + success_qualifiers: + - when: "accept_count > 0 and fixes_applied == 0 and fix_failures == 0" + qualifier: "accepted_without_changes" vis-lens-always-on: inputs: diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index 793efa38c5..6002f8423c 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 dec13da47a..28a3552002 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 11e97ed38b..56ef04f673 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/implementation.json b/src/autoskillit/recipes/implementation.json index 9098bcca62..1f7641bcca 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -1267,7 +1267,8 @@ "step_name": "resolve_review" }, "capture": { - "verdict": "${{ result.verdict }}" + "verdict": "${{ result.verdict }}", + "fixes_applied": "${{ result.fixes_applied }}" }, "retries": 2, "on_exhausted": "release_issue_failure", @@ -1295,8 +1296,6 @@ } ], "on_failure": "release_issue_failure", - "optional": true, - "skip_when_false": "inputs.backend_supports_git_write", "optional_context_refs": [ "review_mode" ], diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index 49b743ecdb..d458f886de 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -1164,6 +1164,7 @@ steps: step_name: resolve_review capture: verdict: ${{ result.verdict }} + fixes_applied: ${{ result.fixes_applied }} retries: 2 on_exhausted: release_issue_failure on_context_limit: release_issue_failure @@ -1179,8 +1180,6 @@ steps: route: pre_review_rebase - route: release_issue_failure on_failure: release_issue_failure - optional: true - skip_when_false: inputs.backend_supports_git_write optional_context_refs: - review_mode note: 'Runs when review_pr reports issues (verdict=changes_requested). The clone diff --git a/src/autoskillit/recipes/merge-prs.json b/src/autoskillit/recipes/merge-prs.json index fa4d2c46ad..4f211af5f3 100644 --- a/src/autoskillit/recipes/merge-prs.json +++ b/src/autoskillit/recipes/merge-prs.json @@ -1596,7 +1596,8 @@ "step_name": "resolve_review_integration" }, "capture": { - "verdict": "${{ result.verdict }}" + "verdict": "${{ result.verdict }}", + "fixes_applied": "${{ result.fixes_applied }}" }, "retries": 2, "on_exhausted": "register_clone_failure", @@ -1624,7 +1625,6 @@ } ], "on_failure": "register_clone_failure", - "skip_when_false": "inputs.backend_supports_git_write", "note": "Resolves review findings on the integration PR (REQ-REVIEW-002). Fetches inline and summary review comments from GitHub and applies fixes. Retries up to 2 times (3 total attempts) before escalating to register_clone_failure (REQ-REVIEW-005). Routes via on_result: verdict dispatch — real_fix/already_green route through pre_review_rebase_integration (fetch+rebase before push); flake_suspected/ ci_only_failure escalate to register_clone_failure.\n" }, "pre_review_rebase_integration": { diff --git a/src/autoskillit/recipes/merge-prs.yaml b/src/autoskillit/recipes/merge-prs.yaml index 1087f9e39b..2926334cd8 100644 --- a/src/autoskillit/recipes/merge-prs.yaml +++ b/src/autoskillit/recipes/merge-prs.yaml @@ -1555,6 +1555,7 @@ steps: step_name: resolve_review_integration capture: verdict: ${{ result.verdict }} + fixes_applied: ${{ result.fixes_applied }} retries: 2 on_exhausted: register_clone_failure on_context_limit: pre_review_rebase_integration @@ -1570,7 +1571,6 @@ steps: route: pre_review_rebase_integration - route: register_clone_failure on_failure: register_clone_failure - skip_when_false: inputs.backend_supports_git_write note: 'Resolves review findings on the integration PR (REQ-REVIEW-002). Fetches inline and summary review comments from GitHub and applies fixes. Retries up to 2 times (3 total attempts) before escalating to register_clone_failure (REQ-REVIEW-005). diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index 746e9de1ee..f2bd212820 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -1482,7 +1482,8 @@ "step_name": "resolve_review" }, "capture": { - "verdict": "${{ result.verdict }}" + "verdict": "${{ result.verdict }}", + "fixes_applied": "${{ result.fixes_applied }}" }, "retries": 2, "on_exhausted": "release_issue_failure", @@ -1510,9 +1511,7 @@ } ], "on_failure": "release_issue_failure", - "optional": true, - "skip_when_false": "inputs.backend_supports_git_write", - "note": "Runs when review_pr reports issues (verdict=changes_requested). The clone (context.work_dir) has the feature branch (context.merge_target) checked out. resolve-review fetches inline and summary review comments via the GitHub API, applies fixes for each actionable finding (critical + warning severity), and commits each fix. Routes via on_result: verdict dispatch — real_fix/already_green/ flake_suspected/ci_only_failure all route to pre_review_rebase (fetch+rebase before push); ci_only_failure cannot be emitted without CI context.\n", + "note": "Runs when review_pr reports issues (verdict=changes_requested). The clone (context.work_dir) has the feature branch (context.merge_target) checked out. resolve-review fetches inline and summary review comments via the GitHub API, applies fixes for each actionable finding (critical + warning severity), and commits each fix via the commit_files MCP tool.\n", "optional_context_refs": [ "review_mode" ] diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 61986a6c05..8b7cf58218 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -1362,6 +1362,7 @@ steps: step_name: resolve_review capture: verdict: ${{ result.verdict }} + fixes_applied: ${{ result.fixes_applied }} retries: 2 on_exhausted: release_issue_failure on_context_limit: release_issue_failure @@ -1377,15 +1378,11 @@ steps: route: pre_review_rebase - route: release_issue_failure on_failure: release_issue_failure - optional: true - skip_when_false: inputs.backend_supports_git_write note: 'Runs when review_pr reports issues (verdict=changes_requested). The clone (context.work_dir) has the feature branch (context.merge_target) checked out. resolve-review fetches inline and summary review comments via the GitHub API, applies fixes for each actionable finding (critical + warning severity), and - commits each fix. Routes via on_result: verdict dispatch — real_fix/already_green/ - flake_suspected/ci_only_failure all route to pre_review_rebase (fetch+rebase - before push); ci_only_failure cannot be emitted without CI context. + commits each fix via the commit_files MCP tool. ' optional_context_refs: diff --git a/src/autoskillit/recipes/research-review.json b/src/autoskillit/recipes/research-review.json index c172f988e4..06ec2a7fc9 100644 --- a/src/autoskillit/recipes/research-review.json +++ b/src/autoskillit/recipes/research-review.json @@ -333,8 +333,7 @@ "on_exhausted": "route_claims_resolve", "on_context_limit": "route_claims_resolve", "on_success": "route_claims_resolve", - "on_failure": "route_claims_resolve", - "skip_when_false": "inputs.backend_supports_git_write" + "on_failure": "route_claims_resolve" }, "route_claims_resolve": { "action": "route", @@ -368,8 +367,7 @@ "on_exhausted": "merge_escalations", "on_context_limit": "merge_escalations", "on_success": "merge_escalations", - "on_failure": "merge_escalations", - "skip_when_false": "inputs.backend_supports_git_write" + "on_failure": "merge_escalations" }, "merge_escalations": { "action": "route", diff --git a/src/autoskillit/recipes/research-review.yaml b/src/autoskillit/recipes/research-review.yaml index 4daaf67b95..99c08eae2b 100644 --- a/src/autoskillit/recipes/research-review.yaml +++ b/src/autoskillit/recipes/research-review.yaml @@ -306,7 +306,6 @@ steps: on_context_limit: route_claims_resolve on_success: route_claims_resolve on_failure: route_claims_resolve - skip_when_false: "inputs.backend_supports_git_write" route_claims_resolve: action: route @@ -338,7 +337,6 @@ steps: on_context_limit: merge_escalations on_success: merge_escalations on_failure: merge_escalations - skip_when_false: "inputs.backend_supports_git_write" merge_escalations: action: route diff --git a/src/autoskillit/server/_factory.py b/src/autoskillit/server/_factory.py index c5bb6e2b70..0100527cd3 100644 --- a/src/autoskillit/server/_factory.py +++ b/src/autoskillit/server/_factory.py @@ -62,6 +62,7 @@ ) from autoskillit.recipe import ( DefaultRecipeRepository, + SkillContract, get_skill_contract, load_bundled_manifest, resolve_input_specs, @@ -392,10 +393,17 @@ def _resolve_completion_required(skill_command: str) -> bool: contract = get_skill_contract(name, load_bundled_manifest()) return contract.completion_required if contract else False + def _resolve_skill_contract(skill_command: str) -> SkillContract | None: + name = resolve_skill_name(skill_command) + if not name: + return None + return get_skill_contract(name, load_bundled_manifest()) + ctx.output_pattern_resolver = _resolve_output_patterns ctx.write_expected_resolver = _resolve_write_behavior ctx.read_only_resolver = _resolve_read_only ctx.completion_required_resolver = _resolve_completion_required + ctx.skill_contract_resolver = _resolve_skill_contract ctx.input_contract_resolver = resolve_input_specs ctx.token_factory = token_factory ctx.build_protected_campaign_ids = build_protected_campaign_ids diff --git a/src/autoskillit/server/git.py b/src/autoskillit/server/git.py index 7f12abfbd1..3d1be5bc53 100644 --- a/src/autoskillit/server/git.py +++ b/src/autoskillit/server/git.py @@ -89,6 +89,24 @@ def preview(text: str) -> str: return shaped +def validate_commit_paths(cwd: str, paths: list[str]) -> str | None: + """Validate that every path resolves inside cwd and has no ``.git`` component. + + Returns an error message string on the first violation found, or None if + every path is valid. Uses realpath containment (same algorithm as + ``core.path_containment.resolve_contained_path``) without the file-existence, + hardlink, and size guards that are inappropriate for staging-time validation. + """ + resolved_cwd = os.path.realpath(cwd) + for p in paths: + if ".git" in Path(p).parts: + return f"path contains .git component: {p}" + full = os.path.realpath(os.path.join(resolved_cwd, p)) + if not full.startswith(resolved_cwd + os.sep) and full != resolved_cwd: + return f"path escapes cwd: {p}" + return None + + def _filter_changed_files(stdout: str, prefixes: list[str]) -> tuple[list[str], list[str]]: """Parse git diff stdout into (changed_files, critical_files). diff --git a/src/autoskillit/server/tools/tools_execution.py b/src/autoskillit/server/tools/tools_execution.py index 278d038d89..d545576053 100644 --- a/src/autoskillit/server/tools/tools_execution.py +++ b/src/autoskillit/server/tools/tools_execution.py @@ -1034,6 +1034,10 @@ async def run_skill( if tool_ctx.write_expected_resolver: write_spec = tool_ctx.write_expected_resolver(skill_command) + _skill_contract = None + if tool_ctx.skill_contract_resolver: + _skill_contract = tool_ctx.skill_contract_resolver(skill_command) + # Resolve closure spec from explicit MCP tool parameters. # Closure args are first-class parameters (not embedded in skill_command text) # because the skill_command string is prompt text consumed by the LLM session, @@ -1375,6 +1379,7 @@ async def run_skill( network_access=_network_access, closure_spec=closure_spec, closure_report_root=closure_report_root, + skill_contract=_skill_contract, ) except TimeoutError as exc: logger.error( diff --git a/src/autoskillit/server/tools/tools_git.py b/src/autoskillit/server/tools/tools_git.py index c8554650ab..c20252b6fa 100644 --- a/src/autoskillit/server/tools/tools_git.py +++ b/src/autoskillit/server/tools/tools_git.py @@ -1,11 +1,13 @@ -"""MCP tool handlers: merge_worktree, classify_fix, create_and_publish_branch.""" +"""MCP tool handlers: merge_worktree, classify_fix, create_and_publish_branch, commit_files.""" from __future__ import annotations import asyncio import json import os +import shutil import time +from pathlib import Path from typing import Any import structlog @@ -627,3 +629,112 @@ async def create_and_publish_branch( except Exception as exc: logger.error("create_and_publish_branch unhandled exception", exc_info=True) return json.dumps({"error": f"{type(exc).__name__}: {exc}"}) + + +@mcp.tool( + tags={"autoskillit", "kitchen", "kitchen-core", "headless"}, + annotations={"readOnlyHint": True}, +) +@_cancellation_shield() +@track_response_size("commit_files") +async def commit_files( + paths: list[str], + message: str, + cwd: str, + step_name: str = "", + ctx: Context = CurrentContext(), +) -> str: + """Stage and commit specified files in a worktree via the server process. + + Runs pre-commit hooks on the staged files before committing. If hooks + auto-fix files, re-stages and retries the commit once. On hard hook + failure, returns an error envelope without committing. + + Args: + paths: List of file paths (relative to cwd) to stage and commit. + message: Commit message. + cwd: Absolute path to the git worktree. + step_name: Optional YAML step key for wall-clock timing accumulation. + + Never raises. + """ + try: + with structlog.contextvars.bound_contextvars(tool="commit_files", cwd=cwd): + logger.info("commit_files", path_count=len(paths), cwd=cwd) + + if not cwd or not os.path.isdir(cwd): + return json.dumps( + {"success": False, "error": f"cwd does not exist or is not a directory: {cwd}"} + ) + if not paths: + return json.dumps({"success": False, "error": "paths list is empty"}) + + from autoskillit.server.git import validate_commit_paths # circular-break + + if (path_error := validate_commit_paths(cwd, paths)) is not None: + return json.dumps({"success": False, "error": path_error}) + + from autoskillit.server import _get_ctx # circular-break + + tool_ctx = _get_ctx() + _start = time.monotonic() + + try: + rc, stdout, stderr = await _run_subprocess( + ["git", "-C", cwd, "add", "--"] + paths, cwd=cwd, timeout=30 + ) + if rc != 0: + return json.dumps( + {"success": False, "error": f"git add failed: {stderr.strip()}"} + ) + + pre_commit_bin = shutil.which("pre-commit", path=os.environ.get("PATH", "")) + uv_bin = shutil.which("uv", path=os.environ.get("PATH", "")) + hook_cmd: list[str] | None = None + if (Path(cwd) / ".pre-commit-config.yaml").exists(): + if uv_bin and (Path(cwd) / "uv.lock").exists(): + hook_cmd = [uv_bin, "run", "pre-commit", "run", "--files"] + paths + elif pre_commit_bin: + hook_cmd = [pre_commit_bin, "run", "--files"] + paths + else: + return json.dumps( + { + "success": False, + "error": "pre-commit config exists but no pre-commit binary found", + } + ) + + if hook_cmd is not None: + rc, stdout, stderr = await _run_subprocess(hook_cmd, cwd=cwd, timeout=120) + if rc != 0: + rc2, _, _ = await _run_subprocess( + ["git", "-C", cwd, "add", "--"] + paths, cwd=cwd, timeout=30 + ) + if rc2 != 0: + _err = f"pre-commit + re-add failed: {stderr.strip()}" + return json.dumps({"success": False, "error": _err}) + rc3, _, stderr3 = await _run_subprocess(hook_cmd, cwd=cwd, timeout=120) + if rc3 != 0: + _err = f"pre-commit retry failed: {stderr3.strip()}" + return json.dumps({"success": False, "error": _err}) + + rc, stdout, stderr = await _run_subprocess( + ["git", "-C", cwd, "commit", "-m", message], cwd=cwd, timeout=30 + ) + if rc != 0: + return json.dumps( + {"success": False, "error": f"git commit failed: {stderr.strip()}"} + ) + + rc, stdout, stderr = await _run_subprocess( + ["git", "-C", cwd, "rev-parse", "HEAD"], cwd=cwd, timeout=10 + ) + commit_sha = stdout.strip() + + return json.dumps({"success": True, "commit_sha": commit_sha}) + finally: + if step_name: + tool_ctx.timing_log.record(step_name, time.monotonic() - _start) + except Exception as exc: + logger.error("commit_files unhandled exception", exc_info=True) + return json.dumps({"success": False, "error": f"{type(exc).__name__}: {exc}"}) diff --git a/src/autoskillit/skills/sous-chef/SKILL.md b/src/autoskillit/skills/sous-chef/SKILL.md index ac26b17a1d..717b979d35 100644 --- a/src/autoskillit/skills/sous-chef/SKILL.md +++ b/src/autoskillit/skills/sous-chef/SKILL.md @@ -110,6 +110,7 @@ When `run_skill` returns `needs_retry=true` for **any step**: defines `on_context_limit`** → follow `on_context_limit`. The model made filesystem contact but made no Write/Edit tool calls. Partial progress may exist on disk. - **If `retry_reason: zero_writes` AND `has_progress_evidence` is false** → fall through to `on_failure`. +- **If `retry_reason: outcome_invariant`** → fall through to `on_failure`. The skill's contract-declared outcome invariant was violated (e.g. fix application failures detected). This is a logical failure, not infrastructure — do not route to `on_context_limit`. - **If `retry_reason: stale`** → decrement the `retries` counter for this step. Re-execute the same step if retries remain. If retries are exhausted, fall through to `on_failure`. Do NOT route to `on_context_limit` — stale is a transient failure, @@ -157,6 +158,7 @@ Summary: `needs_retry=true` + `retry_reason=resume` + `subtype=stale` → re-exe `needs_retry=true` + `retry_reason=early_stop` + `has_progress_evidence=false` → `on_failure`. `needs_retry=true` + `retry_reason=zero_writes` + `has_progress_evidence=true` + step has `on_context_limit` → follow `on_context_limit`. `needs_retry=true` + `retry_reason=zero_writes` + `has_progress_evidence=false` → `on_failure`. + `needs_retry=true` + `retry_reason=outcome_invariant` → `on_failure`. `needs_retry=true` + `retry_reason=stale` → decrement retries counter → `on_failure` when exhausted (no partial progress, not a context limit). `needs_retry=true` + `retry_reason=stale` + worktree-creating step → one-shot re-execute (bypasses retries budget; on_failure if repeated stale). diff --git a/src/autoskillit/skills_extended/resolve-claims-review/SKILL.md b/src/autoskillit/skills_extended/resolve-claims-review/SKILL.md index 1e5a987ece..5e85c1effb 100644 --- a/src/autoskillit/skills_extended/resolve-claims-review/SKILL.md +++ b/src/autoskillit/skills_extended/resolve-claims-review/SKILL.md @@ -1,7 +1,7 @@ --- name: resolve-claims-review categories: [research] -uses_capabilities: [agent_model, git_metadata_write, github_api_write] +uses_capabilities: [agent_model, commit_files, github_api_write] description: > Fetch claim findings from audit-claims, run citation-aware intent validation (ACCEPT/REJECT/DISCUSS), apply targeted citation fixes, escalate findings @@ -311,13 +311,13 @@ For each ACCEPT finding, route by `fix_strategy`: 3. Continue processing — escalation does not change the exit code (exit code remains 0) **`add_citation` / `qualify_claim` / `remove_claim` → apply edit → commit:** -```bash -git -C "$worktree_path" add {file} -# If pre-commit hooks exist: -pre-commit run --files {file} && git -C "$worktree_path" add {file} -git -C "$worktree_path" commit -m "fix(claims-review): {description} [{dimension}]" -# Do NOT use --amend — always create new commits. ``` +commit_files(paths=["{file}"], message="fix(claims-review): {description} [{dimension}]", cwd="{work_dir}") +``` +The tool runs pre-commit hooks, handles auto-fix re-staging, and returns +`{"success": true, "commit_sha": "..."}` or `{"success": false, "error": "..."}`. +A finding counts toward `fixes_applied` ONLY when `commit_files` returns `success: true`. +A failed call increments `fix_failures`. Do NOT use `--amend` — always create new commits. Append `thread_node_id` to `addressed_thread_ids` (if not `None`). **Classification gate — REJECT/DISCUSS bypass:** @@ -483,6 +483,8 @@ output tokens: needs_rerun = {true|false} verdict = {real_fix|already_green} fixes_applied = {N} +accept_count = {N} +fix_failures = {N} ``` - **`needs_rerun = true`**: At least one finding was classified as `rerun_required` in diff --git a/src/autoskillit/skills_extended/resolve-research-review/SKILL.md b/src/autoskillit/skills_extended/resolve-research-review/SKILL.md index 4ec1b554a1..aa85a7b7c7 100644 --- a/src/autoskillit/skills_extended/resolve-research-review/SKILL.md +++ b/src/autoskillit/skills_extended/resolve-research-review/SKILL.md @@ -1,7 +1,7 @@ --- name: resolve-research-review categories: [research] -uses_capabilities: [agent_model, git_metadata_write, github_api_write] +uses_capabilities: [agent_model, commit_files, github_api_write] description: > Fetch PR review comments from review-research-pr, run research-aware intent validation (ACCEPT/REJECT/DISCUSS), apply targeted fixes, escalate unrerunnable @@ -298,13 +298,13 @@ For each ACCEPT finding, route by `fix_strategy`: 3. Continue processing — escalation does not change the exit code (exit code remains 0) **`config_fix` / `script_fix` / `report_edit` → apply edit → commit:** -```bash -git -C "$worktree_path" add {file} -# If pre-commit hooks exist: -pre-commit run --files {file} && git -C "$worktree_path" add {file} -git -C "$worktree_path" commit -m "fix(research-review): {description} [{dimension}]" -# Do NOT use --amend — always create new commits. ``` +commit_files(paths=["{file}"], message="fix(research-review): {description} [{dimension}]", cwd="{work_dir}") +``` +The tool runs pre-commit hooks, handles auto-fix re-staging, and returns +`{"success": true, "commit_sha": "..."}` or `{"success": false, "error": "..."}`. +A finding counts toward `fixes_applied` ONLY when `commit_files` returns `success: true`. +A failed call increments `fix_failures`. Do NOT use `--amend` — always create new commits. Append `thread_node_id` to `addressed_thread_ids` (if not `None`). **Classification gate — REJECT/DISCUSS bypass:** @@ -464,6 +464,8 @@ output tokens: needs_rerun = {true|false} verdict = {real_fix|already_green} fixes_applied = {N} +accept_count = {N} +fix_failures = {N} ``` - **`needs_rerun = true`**: At least one finding was classified as `rerun_required` in diff --git a/src/autoskillit/skills_extended/resolve-review/SKILL.md b/src/autoskillit/skills_extended/resolve-review/SKILL.md index 68f4da7642..cf352ca8e1 100644 --- a/src/autoskillit/skills_extended/resolve-review/SKILL.md +++ b/src/autoskillit/skills_extended/resolve-review/SKILL.md @@ -1,7 +1,7 @@ --- name: resolve-review categories: [github] -uses_capabilities: [agent_model, git_metadata_write, github_api_write, run_skill, test_check] +uses_capabilities: [agent_model, commit_files, github_api_write, run_skill, test_check] description: Fetch PR review comments, run intent validation (ACCEPT/REJECT/DISCUSS) before applying fixes, and post inline replies. MCP-only — used exclusively by recipe orchestration via run_skill after review_pr reports changes_requested or needs_human verdict. hooks: PreToolUse: @@ -72,7 +72,7 @@ normal commit protocol. **Before every test run and before emitting structured output tokens:** 1. Run `git -C {work_dir} status --porcelain` -2. If any files are dirty: `git -C {work_dir} add -- && git -C {work_dir} commit -m "fix: commit pending review changes"` — do NOT use `--amend`. +2. If any files are dirty: call `commit_files(paths=[], message="fix: commit pending review changes", cwd="{work_dir}")`. Count any failed call toward `fix_failures`. 3. Only then proceed with the test or structured output This ensures that even if context exhaustion interrupts the fix loop, all applied @@ -608,14 +608,14 @@ For each finding where the classification map shows `verdict = ACCEPT` the edit — the pre-built context covers understanding only, not the write. 2. Understand what the reviewer is requesting 3. Apply the fix -4. Stage and commit: - ```bash - git add {file} - # If pre-commit hooks are configured: - pre-commit run --files {file} && git add {file} - git commit -m "fix(review): {brief description of reviewer's request}" - # Do NOT use --amend — always create new commits. +4. Stage and commit via the `commit_files` MCP tool: + ``` + commit_files(paths=["{file}"], message="fix(review): {brief description of reviewer's request}", cwd="{work_dir}") ``` + The tool runs pre-commit hooks, handles auto-fix re-staging, and returns + `{"success": true, "commit_sha": "..."}` or `{"success": false, "error": "..."}`. + A finding counts toward `fixes_applied` ONLY when `commit_files` returns `success: true`. + A failed call increments `fix_failures`. Do NOT use `--amend` — always create new commits. **Classification gate — REJECT/DISCUSS bypass:** For findings where the classification map shows `verdict = REJECT` or `verdict = DISCUSS`: @@ -867,12 +867,17 @@ Then determine and emit the structured output tokens (required for the ``` verdict = {verdict} fixes_applied = {accept_count - skipped_in_fix_phase} +accept_count = {accept_count} +fix_failures = {fix_failures} ``` Where: - `{verdict}` is `real_fix` if fixes were applied, `already_green` otherwise - `{accept_count - skipped_in_fix_phase}` is the number of ACCEPT findings where code changes were actually committed +- `{accept_count}` is the total number of ACCEPT-classified findings +- `{fix_failures}` is the number of ACCEPT findings whose apply/commit attempt + errored (distinct from `skipped_in_fix_phase` which counts legitimate pre-attempt skips) The Step 1 graceful degradation exit must NOT emit these tokens — no tokens when skipping due to no PR found. @@ -886,9 +891,13 @@ When a PR is processed, the following structured output tokens are emitted: ``` verdict = real_fix|already_green fixes_applied = {N} +accept_count = {N} +fix_failures = {N} ``` -Where `{N}` is the count of ACCEPT findings where code changes were committed. +Where `fixes_applied` is the count of ACCEPT findings where code changes were +committed. `accept_count` is the total number of ACCEPT-classified findings. +`fix_failures` is the count of ACCEPT findings whose apply/commit attempt failed. `verdict = real_fix` means fixes were applied; `verdict = already_green` means all review findings were already addressed and no code changes were needed. diff --git a/src/autoskillit/workspace/skills.py b/src/autoskillit/workspace/skills.py index 035c7e678b..82c3a2045e 100644 --- a/src/autoskillit/workspace/skills.py +++ b/src/autoskillit/workspace/skills.py @@ -28,6 +28,7 @@ class SkillInfo: categories: frozenset[str] = frozenset() backend_requirements: frozenset[str] = frozenset() uses_capabilities: frozenset[str] = frozenset() + invalid_reason: str | None = None def _read_skill_frontmatter(path: Path) -> dict[str, Any]: @@ -142,7 +143,12 @@ def _skill_info_from_frontmatter(name: str, source: SkillSource, skill_path: Pat frozenset(str(c) for c in caps_raw) if isinstance(caps_raw, list) else frozenset() ) unknown_caps = uses_capabilities - frozenset(SKILL_CAPABILITY_REGISTRY) + _invalid_reason: str | None = None if unknown_caps: + _invalid_reason = ( + f"unknown uses_capabilities: {sorted(unknown_caps)} " + f"(valid: {sorted(SKILL_CAPABILITY_REGISTRY)})" + ) logger.warning( "unrecognized_uses_capabilities", invalid=sorted(unknown_caps), @@ -166,6 +172,7 @@ def _skill_info_from_frontmatter(name: str, source: SkillSource, skill_path: Pat categories=categories, backend_requirements=backend_requirements, uses_capabilities=uses_capabilities, + invalid_reason=_invalid_reason, ) diff --git a/tests/_test_filter.py b/tests/_test_filter.py index eee5891d6d..3f4f32fb7f 100644 --- a/tests/_test_filter.py +++ b/tests/_test_filter.py @@ -795,6 +795,7 @@ class ImportContext(enum.StrEnum): "execution/test_headless_path_validation.py", "execution/test_zero_write_detection.py", "execution/test_smoke_codex.py", + "execution/test_outcome_invariants.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/test_execution_source_split.py b/tests/arch/test_execution_source_split.py index ea7cf1623f..1da67a0bf0 100644 --- a/tests/arch/test_execution_source_split.py +++ b/tests/arch/test_execution_source_split.py @@ -17,9 +17,9 @@ "_headless_evidence.py", ] HEADLESS_SIZE_BUDGETS = { - "headless/__init__.py": 525, + "headless/__init__.py": 530, "headless/_headless_helpers.py": 220, - "headless/_headless_execute.py": 630, + "headless/_headless_execute.py": 635, "headless/_headless_recovery.py": 370, "headless/_headless_path_tokens.py": 190, "headless/_headless_result.py": 920, diff --git a/tests/arch/test_import_linter_contracts.py b/tests/arch/test_import_linter_contracts.py index b9389f0644..19813c70a5 100644 --- a/tests/arch/test_import_linter_contracts.py +++ b/tests/arch/test_import_linter_contracts.py @@ -33,9 +33,11 @@ EXPECTED_CROSS_LAYER_GUARDS: dict[str, frozenset[str]] = { "core/types/_type_protocols_recipe.py": frozenset({"recipe"}), - "execution/headless/__init__.py": frozenset({"pipeline"}), - "execution/headless/_headless_execute.py": frozenset({"pipeline"}), + "execution/headless/__init__.py": frozenset({"pipeline", "recipe"}), + "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_result.py": frozenset({"recipe"}), "execution/linux_tracing.py": frozenset({"config"}), "execution/process/__init__.py": frozenset({"config"}), "execution/testing.py": frozenset({"config"}), diff --git a/tests/arch/test_layer_enforcement.py b/tests/arch/test_layer_enforcement.py index 1e6bd262f2..4a8ba0dc5b 100644 --- a/tests/arch/test_layer_enforcement.py +++ b/tests/arch/test_layer_enforcement.py @@ -638,8 +638,11 @@ def test_il2_no_deferred_upward_imports(pkg_name: str) -> None: source = py_file.read_text(encoding="utf-8") tree = ast.parse(source, filename=str(py_file)) deferred_nodes = _collect_deferred_imports(tree) + tc_lines = _type_checking_lines(tree) for node in deferred_nodes: + if node.lineno in tc_lines: + continue stems_to_check: list[str] = [] if isinstance(node, ast.ImportFrom) and node.module: parts = node.module.split(".") @@ -1616,6 +1619,9 @@ def test_default_classes_only_instantiated_inside_factory_or_allowlist() -> None ), # write detection sync guard validates recipe contract patterns against test fixtures "tests/execution/test_zero_write_detection.py": frozenset({"autoskillit.recipe"}), + # outcome invariant tests verify _apply_post_session_adjudication against + # SkillContract/OutcomeInvariantEntry/SuccessQualifierEntry definitions + "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"}), # quota tests cross into config to validate the contract between vocab constants diff --git a/tests/arch/test_pyright_suppression_allowlist.py b/tests/arch/test_pyright_suppression_allowlist.py index 3d32f1cabd..0663dddd73 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", - 274, + 278, ): "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_skill_backend_annotations.py b/tests/arch/test_skill_backend_annotations.py index 7854ed33a1..3f7288556b 100644 --- a/tests/arch/test_skill_backend_annotations.py +++ b/tests/arch/test_skill_backend_annotations.py @@ -25,6 +25,7 @@ "run_skill": [re.compile(r"\brun_skill\b")], "test_check": [re.compile(r"\btest_check\b")], "claude_dir": [re.compile(r"\.claude/")], + "commit_files": [re.compile(r"\bcommit_files\s*\(")], "git_metadata_write": [ re.compile(r"create_impl_worktree\.sh|git worktree add\b[ \t]+\S|git checkout -b"), re.compile(r"git\s+(?:-C\s+\S+\s+)?commit\s+-m"), diff --git a/tests/arch/test_subpackage_structure.py b/tests/arch/test_subpackage_structure.py index 1d3fc591b5..fa692b8b55 100644 --- a/tests/arch/test_subpackage_structure.py +++ b/tests/arch/test_subpackage_structure.py @@ -109,6 +109,7 @@ def test_headless_has_expected_modules(self): "_headless_execute", "_headless_git", "_headless_helpers", + "_headless_outcome", "_headless_path_tokens", "_headless_recovery", "_headless_result", diff --git a/tests/cli/test_doctor_standing_pins.py b/tests/cli/test_doctor_standing_pins.py index 77e1f78b6e..1158a67d70 100644 --- a/tests/cli/test_doctor_standing_pins.py +++ b/tests/cli/test_doctor_standing_pins.py @@ -27,7 +27,7 @@ def test_infeasible_pin_reports_error( config_dir = tmp_path / "home" / ".autoskillit" _write_config( config_dir / "config.yaml", - "agent_backend:\n recipe_overrides:\n remediation:\n resolve_review: codex\n", + "agent_backend:\n recipe_overrides:\n remediation:\n assess: codex\n", ) project_dir = tmp_path / "project" @@ -37,7 +37,7 @@ def test_infeasible_pin_reports_error( errors = [r for r in results if r.severity == Severity.ERROR] assert errors, f"Expected at least one ERROR result, got: {results}" msg = errors[0].message - assert "resolve_review" in msg and "codex" in msg + assert "assess" in msg and "codex" in msg def test_feasible_pin_reports_ok( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/tests/cli/test_routing_completeness.py b/tests/cli/test_routing_completeness.py index eaf6f0a978..b56c80782f 100644 --- a/tests/cli/test_routing_completeness.py +++ b/tests/cli/test_routing_completeness.py @@ -39,6 +39,7 @@ RetryReason.CONTRACT_RECOVERY: ("on_context_limit", "has_progress_evidence"), RetryReason.CLONE_CONTAMINATION: ("on_failure", "pre_contamination_retry_reason"), RetryReason.RATE_LIMITED: ("on_rate_limit", None), + RetryReason.OUTCOME_INVARIANT: ("on_failure", None), } diff --git a/tests/core/test_type_constants.py b/tests/core/test_type_constants.py index 2abde2b675..58f7015698 100644 --- a/tests/core/test_type_constants.py +++ b/tests/core/test_type_constants.py @@ -398,7 +398,7 @@ def test_provider_profile_in_private_env_vars() -> None: def test_headless_tools_contains_expected_names(): from autoskillit.core.types import HEADLESS_TOOLS - assert HEADLESS_TOOLS == {"test_check", "unlock_agent_pack"} + assert HEADLESS_TOOLS == {"test_check", "unlock_agent_pack", "commit_files"} def test_free_range_tools_contains_expected_names(): diff --git a/tests/core/test_type_protocol_shards.py b/tests/core/test_type_protocol_shards.py index 6ec0c6a615..806ffc14dc 100644 --- a/tests/core/test_type_protocol_shards.py +++ b/tests/core/test_type_protocol_shards.py @@ -56,6 +56,7 @@ def test_recipe_shard_all(): "MigrationService", "DatabaseReader", "ReadOnlyResolver", + "SkillContractResolver", "ServeOverridesSnapshot", } diff --git a/tests/core/test_types.py b/tests/core/test_types.py index 4064b031e5..5e051b307f 100644 --- a/tests/core/test_types.py +++ b/tests/core/test_types.py @@ -62,6 +62,7 @@ def test_retry_reason_values(): RetryReason.IDLE_STALL, RetryReason.RATE_LIMITED, RetryReason.CANCELLED, + RetryReason.OUTCOME_INVARIANT, } assert RetryReason.NONE.value == "none" diff --git a/tests/docs/test_doc_counts.py b/tests/docs/test_doc_counts.py index d59b32c87b..1c3e8b3113 100644 --- a/tests/docs/test_doc_counts.py +++ b/tests/docs/test_doc_counts.py @@ -202,9 +202,9 @@ def _count_semantic_rule_files() -> int: # ----- tests ------------------------------------------------------------------ -def test_kitchen_tagged_tool_count_is_39() -> None: +def test_kitchen_tagged_tool_count_is_40() -> None: count = _count_kitchen_tools() - assert count == 39, f"Expected 39 kitchen-tagged tools; found {count}" + assert count == 40, f"Expected 40 kitchen-tagged tools; found {count}" def test_free_range_tool_count_is_17() -> None: @@ -213,9 +213,9 @@ def test_free_range_tool_count_is_17() -> None: ) -def test_headless_tool_count_is_2() -> None: - assert _count_headless_tools() == 2, ( - f"Expected 2 headless-tagged tools; found {_count_headless_tools()}" +def test_headless_tool_count_is_3() -> None: + assert _count_headless_tools() == 3, ( + f"Expected 3 headless-tagged tools; found {_count_headless_tools()}" ) @@ -269,9 +269,9 @@ def test_bundled_recipe_count_is_15() -> None: assert recipes == expected, f"Recipes drifted: {recipes}" -def test_retry_reason_value_count_is_16() -> None: +def test_retry_reason_value_count_is_17() -> None: values = _retry_reason_values() - assert len(values) == 16, f"RetryReason has {len(values)} values: {values}" + assert len(values) == 17, f"RetryReason has {len(values)} values: {values}" def test_semantic_rule_family_count_is_current() -> None: @@ -297,8 +297,8 @@ def _assert_doc_states_number(doc: Path, label: str, expected: int) -> None: DOCS_DIR / "execution" / "tool-access.md", ], ) -def test_docs_state_57_mcp_tools(doc_path: Path) -> None: - _assert_doc_states_number(doc_path, "MCP tools", 58) +def test_docs_state_59_mcp_tools(doc_path: Path) -> None: + _assert_doc_states_number(doc_path, "MCP tools", 59) @pytest.mark.parametrize( diff --git a/tests/execution/AGENTS.md b/tests/execution/AGENTS.md index 0d9288279e..637997ee8a 100644 --- a/tests/execution/AGENTS.md +++ b/tests/execution/AGENTS.md @@ -126,6 +126,7 @@ Subprocess integration, headless session, process lifecycle, and session result | `test_trace_target_resolver.py` | Tests for resolve_trace_target — descendant-walk and basename-match contract | | `test_write_evidence.py` | Write evidence: multi-directory fs snapshot and write_watch_dirs plumbing | | `test_write_evidence_invariants.py` | Write-evidence invariants: 'no work done' retry reasons must be overridden by write evidence | +| `test_outcome_invariants.py` | Outcome-contract adjudication: invariant evaluation, field parsing, success qualifiers | | `test_zero_write_detection.py` | Contract: sessions expected to write must actually write (behavioral write-count gate) | | `test_closure_gate.py` | Execution-layer closure verification gate tests — demote unverifiable closure reports, pass valid reports, no-op without spec | diff --git a/tests/execution/test_outcome_invariants.py b/tests/execution/test_outcome_invariants.py new file mode 100644 index 0000000000..64b045ac57 --- /dev/null +++ b/tests/execution/test_outcome_invariants.py @@ -0,0 +1,683 @@ +"""Contract: outcome-field invariants catch lying-model degradation. + +Verifies the outcome-contract adjudication layer that parses declared +KEY = value output fields from session result text and demotes sessions +whose self-reported outcome tokens violate a contract invariant — e.g. a +skill claiming "verdict = already_green" (or "real_fix") while also +reporting fix_failures > 0. Covers RECT-011 through RECT-018. +""" + +from __future__ import annotations + +import dataclasses +import json +import re + +import pytest + +from autoskillit.core import RetryReason, WriteBehaviorSpec +from autoskillit.core.types import KillReason +from autoskillit.core.types._type_results import ApiRetryOutcome, SkillResult, WriteEvidence +from autoskillit.execution.backends.claude import ClaudeCodeBackend +from autoskillit.execution.headless import _build_skill_result +from autoskillit.execution.headless._headless_outcome import ( + evaluate_outcome_invariants, + evaluate_success_qualifier, + parse_outcome_fields, +) +from autoskillit.execution.headless._headless_result import _apply_post_session_adjudication +from autoskillit.recipe import ( + OutcomeInvariantEntry, + SkillContract, + SkillOutput, + SuccessQualifierEntry, + get_skill_contract, + load_bundled_manifest, +) +from tests.conftest import _make_result + +pytestmark = [pytest.mark.layer("execution"), pytest.mark.small] + + +def _resolve_review_contract() -> SkillContract: + manifest = load_bundled_manifest() + contract = get_skill_contract("resolve-review", manifest) + assert contract is not None, "resolve-review missing from skill_contracts.yaml" + assert contract.outcome_invariants, "resolve-review must declare outcome_invariants" + return contract + + +def _e6_result_text( + *, verdict: str, accept_count: int, fixes_applied: int, fix_failures: int +) -> str: + return ( + f"verdict = {verdict}\n" + f"fixes_applied = {fixes_applied}\n" + f"accept_count = {accept_count}\n" + f"fix_failures = {fix_failures}\n" + "%%ORDER_UP%%" + ) + + +def _result_record(result_text: str, session_id: str = "test-sess") -> str: + return json.dumps( + { + "type": "result", + "subtype": "success", + "is_error": False, + "result": result_text, + "session_id": session_id, + } + ) + + +def _stdout_with_edit_evidence(result_text: str, session_id: str = "test-sess") -> str: + """Build NDJSON stdout with an Edit tool_use plus a success result record. + + Used for ``verdict = real_fix`` fixtures: the write-expectation gate + (checked before outcome invariants) requires write evidence when the + conditional pattern matches, so these fixtures must carry an Edit call + to reach the outcome-invariant adjudication being tested. + """ + assistant = { + "type": "assistant", + "message": { + "content": [{"type": "tool_use", "name": "Edit", "id": "tu_0"}], + }, + } + return "\n".join([json.dumps(assistant), _result_record(result_text, session_id)]) + + +# --------------------------------------------------------------------------- +# RECT-011 / RECT-012: E6-shaped demotion via full _build_skill_result pipeline +# --------------------------------------------------------------------------- + + +class TestOutcomeInvariantDemotion: + """Synthetic 'already_green with fix_failures' sessions must be demoted.""" + + def test_already_green_with_fix_failures_demoted(self) -> None: + """RECT-011: verdict=already_green, accept_count=3, fixes_applied=0, + fix_failures=3, with file_changes_count=1 evidence must be demoted.""" + stdout = _result_record( + _e6_result_text( + verdict="already_green", accept_count=3, fixes_applied=0, fix_failures=3 + ) + ) + sr = _build_skill_result( + _make_result(returncode=0, stdout=stdout), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.success is False, ( + "already_green with fix_failures=3 (accept_count > 0) must be demoted" + ) + assert sr.subtype == "outcome_invariant_violation" + assert sr.needs_retry is True + assert sr.retry_reason == RetryReason.OUTCOME_INVARIANT + + def test_lying_model_real_fix_with_fix_failures_demoted(self) -> None: + """RECT-012: verdict=real_fix, fixes_applied=0, fix_failures=3 demotes identically. + + Carries Edit-tool write evidence so the case reaches outcome-invariant + adjudication rather than being intercepted by the zero-write gate + (verdict=real_fix alone triggers the write-expectation pattern). + """ + stdout = _stdout_with_edit_evidence( + _e6_result_text(verdict="real_fix", accept_count=3, fixes_applied=0, fix_failures=3) + ) + sr = _build_skill_result( + _make_result(returncode=0, stdout=stdout), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.success is False, ( + "real_fix with fix_failures=3 (accept_count > 0) must be demoted identically" + ) + assert sr.subtype == "outcome_invariant_violation" + assert sr.needs_retry is True + assert sr.retry_reason == RetryReason.OUTCOME_INVARIANT + + +class TestOutcomeInvariantCounterCases: + """RECT-013: legitimate outcome shapes must NOT be demoted.""" + + def test_all_reject_stays_success(self) -> None: + """accept_count=0 → invariant's 'when' is false → skipped → success preserved.""" + stdout = _result_record( + _e6_result_text( + verdict="already_green", accept_count=0, fixes_applied=0, fix_failures=0 + ) + ) + sr = _build_skill_result( + _make_result(returncode=0, stdout=stdout), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.success is True + assert sr.subtype != "outcome_invariant_violation" + + def test_full_success_stays_unqualified_success(self) -> None: + """accept_count=3, fixes_applied=3, fix_failures=0 → invariant satisfied → success. + + Carries Edit-tool write evidence to satisfy the write-expectation gate + (verdict=real_fix triggers it) so this exercises outcome-invariant + adjudication rather than being intercepted upstream. + """ + stdout = _stdout_with_edit_evidence( + _e6_result_text(verdict="real_fix", accept_count=3, fixes_applied=3, fix_failures=0) + ) + sr = _build_skill_result( + _make_result(returncode=0, stdout=stdout), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.success is True + assert sr.subtype != "outcome_invariant_violation" + assert sr.outcome_qualifier is None, "Full success must NOT carry a qualifier" + + def test_legitimate_all_skipped_already_green_qualified_not_demoted(self) -> None: + """accept_count=1, fixes_applied=0, fix_failures=0 → success WITH qualifier + 'accepted_without_changes' (not demoted).""" + stdout = _result_record( + _e6_result_text( + verdict="already_green", accept_count=1, fixes_applied=0, fix_failures=0 + ) + ) + sr = _build_skill_result( + _make_result(returncode=0, stdout=stdout), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.success is True, ( + "Legitimate all-skipped already_green (fix_failures=0) must NOT be demoted" + ) + assert sr.subtype != "outcome_invariant_violation" + assert sr.outcome_qualifier == "accepted_without_changes" + + +class TestOutcomeInvariantRecoveryPaths: + """RECT-014: E6-shaped output arriving via recovered-STALE / recovered-IDLE_STALL + infra paths must demote identically to the normal-completion path.""" + + def test_recovered_stale_demotes(self) -> None: + from autoskillit.core.types._type_enums import TerminationReason + + stdout = _result_record( + _e6_result_text( + verdict="already_green", accept_count=3, fixes_applied=0, fix_failures=3 + ) + ) + sr = _build_skill_result( + _make_result( + returncode=0, + stdout=stdout, + termination_reason=TerminationReason.STALE, + ), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.subtype != "recovered_from_stale", ( + "Recovery subtype must be overwritten by the invariant-violation subtype" + ) + assert sr.success is False + assert sr.subtype == "outcome_invariant_violation" + assert sr.needs_retry is True + assert sr.retry_reason == RetryReason.OUTCOME_INVARIANT + + def test_recovered_idle_stall_demotes(self) -> None: + """Carries Edit-tool write evidence: verdict=real_fix triggers the + write-expectation gate, which must be satisfied to reach outcome-invariant + adjudication on the recovered-IDLE_STALL path.""" + from autoskillit.core.types._type_enums import TerminationReason + + stdout = _stdout_with_edit_evidence( + _e6_result_text(verdict="real_fix", accept_count=3, fixes_applied=0, fix_failures=3) + ) + sr = _build_skill_result( + _make_result( + returncode=0, + stdout=stdout, + termination_reason=TerminationReason.IDLE_STALL, + ), + skill_command="/autoskillit:resolve-review feature-branch main", + write_behavior=WriteBehaviorSpec( + mode="conditional", + expected_when=(r"verdict[ \t]*=[ \t]*real_fix",), + ), + skill_contract=_resolve_review_contract(), + backend=ClaudeCodeBackend(), + ) + assert sr.subtype != "recovered_from_idle_stall", ( + "Recovery subtype must be overwritten by the invariant-violation subtype" + ) + assert sr.success is False + assert sr.subtype == "outcome_invariant_violation" + assert sr.needs_retry is True + assert sr.retry_reason == RetryReason.OUTCOME_INVARIANT + + +# --------------------------------------------------------------------------- +# RECT-015: parse_outcome_fields unit tests +# --------------------------------------------------------------------------- + + +class TestParseOutcomeFields: + """Contract-field parser: declared fields only, typed per contract.""" + + def _contract(self) -> SkillContract: + return SkillContract( + inputs=[], + outputs=[ + SkillOutput(name="verdict", type="string"), + SkillOutput(name="accept_count", type="integer"), + SkillOutput(name="fixes_applied", type="integer"), + SkillOutput(name="fix_failures", type="integer"), + ], + ) + + def test_declared_integer_field_parses_to_int(self) -> None: + fields = parse_outcome_fields("accept_count = 5\n", self._contract()) + assert fields["accept_count"] == 5 + assert isinstance(fields["accept_count"], int) + + def test_malformed_integer_field_stays_raw_string(self) -> None: + fields = parse_outcome_fields("accept_count = not_a_number\n", self._contract()) + assert fields["accept_count"] == "not_a_number" + + def test_undeclared_tokens_ignored(self) -> None: + fields = parse_outcome_fields( + "accept_count = 2\nsome_undeclared_field = 99\n", self._contract() + ) + assert "some_undeclared_field" not in fields + assert fields["accept_count"] == 2 + + def test_realistic_step7_output_parses_all_fields(self) -> None: + result_text = _e6_result_text( + verdict="real_fix", accept_count=4, fixes_applied=4, fix_failures=0 + ) + fields = parse_outcome_fields(result_text, self._contract()) + assert fields["verdict"] == "real_fix" + assert fields["accept_count"] == 4 + assert fields["fixes_applied"] == 4 + assert fields["fix_failures"] == 0 + + def test_adversarial_prose_before_block_does_not_overwrite(self) -> None: + """Trailing-block parser ignores field-like lines in earlier prose.""" + result_text = ( + "I set verdict = already_green because there was nothing to fix.\n" + "The accept_count = 2 findings were all rejected.\n" + "\n" + "resolve-review complete\n" + "Fixes applied: 3\n" + "\n" + "verdict = real_fix\n" + "fixes_applied = 3\n" + "accept_count = 4\n" + "fix_failures = 0\n" + "%%ORDER_UP%%" + ) + fields = parse_outcome_fields(result_text, self._contract()) + assert fields["verdict"] == "real_fix" + assert fields["fixes_applied"] == 3 + assert fields["accept_count"] == 4 + assert fields["fix_failures"] == 0 + + def test_trailing_gate_tag_skipped(self) -> None: + """Gate tags after the field block don't break contiguous detection.""" + result_text = ( + "verdict = real_fix\n" + "fixes_applied = 1\n" + "accept_count = 1\n" + "fix_failures = 0\n" + "%%ORDER_UP::abc123%%" + ) + fields = parse_outcome_fields(result_text, self._contract()) + assert fields["verdict"] == "real_fix" + assert fields["fixes_applied"] == 1 + + def test_fields_only_no_trailing_tag(self) -> None: + """Parser works when no gate tag follows the field block.""" + result_text = "verdict = already_green\nfixes_applied = 0\n" + fields = parse_outcome_fields(result_text, self._contract()) + assert fields["verdict"] == "already_green" + assert fields["fixes_applied"] == 0 + + +# --------------------------------------------------------------------------- +# RECT-016: token-emission sync — SKILL.md must declare every invariant field +# --------------------------------------------------------------------------- + + +class TestTokenEmissionSync: + """Every field referenced by an outcome_invariants entry must be both a + declared output in skill_contracts.yaml AND appear as an emitted + ``field = `` line inside the resolve-review SKILL.md Output section.""" + + @staticmethod + def _skill_md_output_section_text() -> str: + from autoskillit.core import pkg_root + + skill_md = pkg_root() / "skills_extended" / "resolve-review" / "SKILL.md" + content = skill_md.read_text(encoding="utf-8") + marker = "\n## Output\n" + idx = content.find(marker) + assert idx != -1, "resolve-review SKILL.md missing '## Output' section" + return content[idx:] + + def test_invariant_fields_declared_and_emitted(self) -> None: + contract = _resolve_review_contract() + declared_names = {o.name for o in contract.outputs} + output_section = self._skill_md_output_section_text() + + referenced_fields: set[str] = set() + for inv in contract.outcome_invariants: + for expr in (inv.when, inv.require): + field_name = expr.split()[0] if expr.split() else "" + referenced_fields.add(field_name) + + assert referenced_fields, "outcome_invariants must reference at least one field" + + for field_name in referenced_fields: + assert field_name in declared_names, ( + f"outcome_invariants references {field_name!r} which is not a declared " + "output in skill_contracts.yaml" + ) + line_pattern = re.compile(rf"^{re.escape(field_name)}\s*=", re.MULTILINE) + assert line_pattern.search(output_section), ( + f"outcome_invariants field {field_name!r} has no '{field_name} = ' line " + "in resolve-review SKILL.md Output section" + ) + + +# --------------------------------------------------------------------------- +# RECT-017: contract loader validation +# --------------------------------------------------------------------------- + + +class TestContractLoaderValidation: + """get_skill_contract must reject malformed outcome_invariants entries.""" + + def test_undeclared_field_reference_fails_load(self) -> None: + manifest = { + "skills": { + "fake-skill": { + "outputs": [{"name": "accept_count", "type": "integer"}], + "outcome_invariants": [ + {"when": "accept_count > 0", "require": "undeclared_field == 0"} + ], + } + } + } + with pytest.raises(ValueError, match="undeclared output"): + get_skill_contract("fake-skill", manifest) + + def test_non_numeric_declared_field_fails_load(self) -> None: + manifest = { + "skills": { + "fake-skill": { + "outputs": [ + {"name": "accept_count", "type": "integer"}, + {"name": "verdict", "type": "string"}, + ], + "outcome_invariants": [ + {"when": "accept_count > 0", "require": "verdict == 0"} + ], + } + } + } + with pytest.raises(ValueError, match="non-integer output"): + get_skill_contract("fake-skill", manifest) + + def test_missing_when_or_require_fails_load(self) -> None: + manifest = { + "skills": { + "fake-skill": { + "outputs": [{"name": "accept_count", "type": "integer"}], + "outcome_invariants": [{"when": "accept_count > 0"}], + } + } + } + with pytest.raises(ValueError, match="missing 'when' or 'require'"): + get_skill_contract("fake-skill", manifest) + + def test_well_formed_invariant_loads_successfully(self) -> None: + manifest = { + "skills": { + "fake-skill": { + "outputs": [ + {"name": "accept_count", "type": "integer"}, + {"name": "fix_failures", "type": "integer"}, + ], + "outcome_invariants": [ + {"when": "accept_count > 0", "require": "fix_failures == 0"} + ], + } + } + } + contract = get_skill_contract("fake-skill", manifest) + assert contract is not None + assert len(contract.outcome_invariants) == 1 + assert contract.outcome_invariants[0].when == "accept_count > 0" + assert contract.outcome_invariants[0].require == "fix_failures == 0" + + +# --------------------------------------------------------------------------- +# RECT-018: evidence semantics regression +# --------------------------------------------------------------------------- + + +class TestEvidenceSemanticsRegression: + """has_implementation_evidence must remain unchanged by this feature — + git_writes_detected alone does NOT satisfy it.""" + + def test_git_writes_alone_does_not_satisfy_implementation_evidence(self) -> None: + evidence = WriteEvidence( + write_call_count=0, + fs_writes_detected=False, + git_writes_detected=True, + file_changes_count=0, + ) + assert evidence.has_implementation_evidence is False + assert evidence.has_evidence is True, ( + "git_writes_detected alone still satisfies the broader has_evidence signal" + ) + + def test_fs_writes_alone_does_not_satisfy_implementation_evidence(self) -> None: + evidence = WriteEvidence( + write_call_count=0, + fs_writes_detected=True, + git_writes_detected=False, + file_changes_count=0, + ) + assert evidence.has_implementation_evidence is False + + def test_write_call_count_satisfies_implementation_evidence(self) -> None: + evidence = WriteEvidence( + write_call_count=1, + fs_writes_detected=False, + git_writes_detected=False, + file_changes_count=0, + ) + assert evidence.has_implementation_evidence is True + + def test_file_changes_count_satisfies_implementation_evidence(self) -> None: + evidence = WriteEvidence( + write_call_count=0, + fs_writes_detected=False, + git_writes_detected=False, + file_changes_count=1, + ) + assert evidence.has_implementation_evidence is True + + +# --------------------------------------------------------------------------- +# Direct unit tests: evaluate_outcome_invariants / evaluate_success_qualifier +# --------------------------------------------------------------------------- + + +class TestEvaluateOutcomeInvariantsUnit: + """Direct unit coverage of evaluate_outcome_invariants against parsed fields.""" + + def _invariants(self) -> list[OutcomeInvariantEntry]: + return [OutcomeInvariantEntry(when="accept_count > 0", require="fix_failures == 0")] + + def test_violation_detected(self) -> None: + fields = {"accept_count": 3, "fix_failures": 3} + violated, detail = evaluate_outcome_invariants(fields, self._invariants()) + assert violated is True + assert "accept_count > 0" in detail + + def test_no_violation_when_require_satisfied(self) -> None: + fields = {"accept_count": 3, "fix_failures": 0} + violated, _ = evaluate_outcome_invariants(fields, self._invariants()) + assert violated is False + + def test_skipped_when_when_field_missing(self) -> None: + """A missing 'when' field means the token was never emitted — legitimate + no-PR-found exit — the invariant must be skipped, not violated.""" + fields: dict[str, int | str] = {} + violated, _ = evaluate_outcome_invariants(fields, self._invariants()) + assert violated is False + + def test_fail_closed_when_require_field_missing(self) -> None: + """'when' true but 'require' field absent → fail-closed violation.""" + fields: dict[str, int | str] = {"accept_count": 1} + violated, _ = evaluate_outcome_invariants(fields, self._invariants()) + assert violated is True + + def test_no_invariants_never_violates(self) -> None: + violated, detail = evaluate_outcome_invariants({"accept_count": 5}, []) + assert violated is False + assert detail == "" + + +class TestEvaluateSuccessQualifierUnit: + """Direct unit coverage of evaluate_success_qualifier.""" + + def _qualifiers(self) -> list[SuccessQualifierEntry]: + return [ + SuccessQualifierEntry( + when="accept_count > 0 and fixes_applied == 0 and fix_failures == 0", + qualifier="accepted_without_changes", + ) + ] + + def test_qualifier_matches(self) -> None: + fields = {"accept_count": 1, "fixes_applied": 0, "fix_failures": 0} + result = evaluate_success_qualifier(fields, self._qualifiers()) + assert result == "accepted_without_changes" + + def test_no_qualifier_when_no_match(self) -> None: + fields = {"accept_count": 3, "fixes_applied": 3, "fix_failures": 0} + result = evaluate_success_qualifier(fields, self._qualifiers()) + assert result is None + + def test_no_qualifiers_returns_none(self) -> None: + result = evaluate_success_qualifier({"accept_count": 1}, []) + assert result is None + + +# --------------------------------------------------------------------------- +# Direct unit tests: _apply_post_session_adjudication +# --------------------------------------------------------------------------- + + +def _base_skill_result(*, success: bool = True, result_text: str = "") -> SkillResult: + return SkillResult( + success=success, + result=result_text, + session_id="test-sess", + subtype="success", + is_error=False, + exit_code=0, + needs_retry=False, + retry_reason=RetryReason.NONE, + stderr="", + kill_reason=KillReason.NATURAL_EXIT, + api_retry=ApiRetryOutcome(), + ) + + +class TestApplyPostSessionAdjudicationUnit: + """Direct unit coverage of _apply_post_session_adjudication.""" + + def test_non_success_passthrough(self) -> None: + sr = _base_skill_result(success=False) + result = _apply_post_session_adjudication( + sr, WriteEvidence.none_observed(), None, _resolve_review_contract() + ) + assert result is sr + + def test_no_contract_passthrough(self) -> None: + sr = _base_skill_result( + result_text=_e6_result_text( + verdict="already_green", accept_count=3, fixes_applied=0, fix_failures=3 + ) + ) + result = _apply_post_session_adjudication(sr, WriteEvidence.none_observed(), None, None) + assert result.success is True + assert result.subtype != "outcome_invariant_violation" + + def test_violated_invariant_demotes_directly(self) -> None: + sr = _base_skill_result( + result_text=_e6_result_text( + verdict="already_green", accept_count=3, fixes_applied=0, fix_failures=3 + ) + ) + evidence = WriteEvidence( + write_call_count=0, + fs_writes_detected=False, + git_writes_detected=False, + file_changes_count=1, + ) + result = dataclasses.replace(sr) # sanity: replace works on SkillResult + result = _apply_post_session_adjudication( + result, evidence, None, _resolve_review_contract() + ) + assert result.success is False + assert result.subtype == "outcome_invariant_violation" + assert result.needs_retry is True + assert result.retry_reason == RetryReason.OUTCOME_INVARIANT + + def test_satisfied_invariant_preserves_success(self) -> None: + sr = _base_skill_result( + result_text=_e6_result_text( + verdict="real_fix", accept_count=3, fixes_applied=3, fix_failures=0 + ) + ) + result = _apply_post_session_adjudication( + sr, WriteEvidence.none_observed(), None, _resolve_review_contract() + ) + assert result.success is True + assert result.subtype != "outcome_invariant_violation" diff --git a/tests/execution/test_session_log_backend_source.py b/tests/execution/test_session_log_backend_source.py index f1e8c606b6..25c101bae3 100644 --- a/tests/execution/test_session_log_backend_source.py +++ b/tests/execution/test_session_log_backend_source.py @@ -71,7 +71,10 @@ def test_session_index_entry_default_value(self) -> None: "api_retry_last_status": None, "ndjson_unknown_event_count": 0, "ndjson_unknown_item_count": 0, - "schema_version": 4, + "outcome_fields": None, + "outcome_invariant_violated": False, + "outcome_qualifier": None, + "schema_version": 5, } assert entry["backend_override_source"] is None diff --git a/tests/execution/test_session_log_flush.py b/tests/execution/test_session_log_flush.py index bec3d01079..4de5fd0d56 100644 --- a/tests/execution/test_session_log_flush.py +++ b/tests/execution/test_session_log_flush.py @@ -568,12 +568,12 @@ def test_flush_index_includes_step_name_and_token_fields(tmp_path): assert entry["cache_read_tokens"] == 80 -def test_flush_index_includes_schema_version_4(tmp_path): - """sessions.jsonl entry must contain schema_version: 4.""" +def test_flush_index_includes_schema_version_5(tmp_path): + """sessions.jsonl entry must contain schema_version: 5.""" _flush(tmp_path) index_path = tmp_path / "sessions.jsonl" entry = json.loads(index_path.read_text().strip().split("\n")[-1]) - assert entry["schema_version"] == 4 + assert entry["schema_version"] == 5 def test_flush_index_token_fields_zero_when_no_step(tmp_path): diff --git a/tests/fakes.py b/tests/fakes.py index 4268601eb5..be6ab738c4 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -104,6 +104,7 @@ class ExecutorCall: network_access: bool = False closure_spec: ClosureAuthoritySpec | None = None closure_report_root: Path | None = None + skill_contract: Any | None = None @dataclasses.dataclass @@ -214,6 +215,7 @@ async def run( network_access: bool = False, closure_spec: ClosureAuthoritySpec | None = None, closure_report_root: Path | None = None, + skill_contract: Any | None = None, ) -> SkillResult: self.calls.append( ExecutorCall( @@ -256,6 +258,7 @@ async def run( network_access=network_access, closure_spec=closure_spec, closure_report_root=closure_report_root, + skill_contract=skill_contract, ) ) if self._queue: diff --git a/tests/infra/test_pretty_output_recipe.py b/tests/infra/test_pretty_output_recipe.py index ea45c03762..1f9e73b82e 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": (182_916, "remediation", "all_truthy"), - "open_kitchen": (182_975, "remediation", "all_truthy"), + "load_recipe": (182_759, "remediation", "all_truthy"), + "open_kitchen": (182_818, "remediation", "all_truthy"), } diff --git a/tests/recipe/test_rules_backend_compat.py b/tests/recipe/test_rules_backend_compat.py index f454299ebb..4db7902f12 100644 --- a/tests/recipe/test_rules_backend_compat.py +++ b/tests/recipe/test_rules_backend_compat.py @@ -552,7 +552,6 @@ class TestBundledRecipeStepGuards: "merge-prs", "research-implement", "research", - "research-review", ], ids=lambda x: x, ) diff --git a/tests/server/AGENTS.md b/tests/server/AGENTS.md index 5222c07184..77c317eaf2 100644 --- a/tests/server/AGENTS.md +++ b/tests/server/AGENTS.md @@ -71,6 +71,7 @@ Server tool handler unit tests — kitchen, execution, CI, clone, workspace tool | `test_tools_ci_split.py` | CI split structural guard | | `test_tools_ci_watch.py` | Tests for wait_for_ci event validation, null coercion, and auto_trigger recovery | | `test_tools_clone.py` | Tests for autoskillit server clone tools | +| `test_tools_commit_files.py` | Tests for commit_files MCP tool: path containment, pre-commit hooks, error envelopes | | `test_tools_dispatch.py` | Tests for dispatch_food_truck execution lifecycle: lock, success envelope, PID, quota, cleanup | | `test_tools_dispatch_halt.py` | Tests for dispatch_food_truck campaign halt enforcement gate | | `test_tools_dispatch_params.py` | Tests for dispatch_food_truck parameter passthrough: resume, idle_timeout, marketplace | diff --git a/tests/server/test_admission_dispatch_agreement.py b/tests/server/test_admission_dispatch_agreement.py index 21a1d96c65..6e174946c6 100644 --- a/tests/server/test_admission_dispatch_agreement.py +++ b/tests/server/test_admission_dispatch_agreement.py @@ -676,6 +676,7 @@ def test_admission_dispatch_agreement_with_explicit_pin( skill_info.name = target_skill_name skill_info.backend_requirements = frozenset() skill_info.uses_capabilities = frozenset({"git_metadata_write"}) + skill_info.invalid_reason = None # Path pointing at a non-existent file so the SKILL.md read in # `_check_undefined_bash_placeholder` raises OSError (caught by the rule) # instead of returning a MagicMock that crashes `extract_bash_blocks`. diff --git a/tests/server/test_server_tool_registration.py b/tests/server/test_server_tool_registration.py index ea7916765f..54bb66a8ff 100644 --- a/tests/server/test_server_tool_registration.py +++ b/tests/server/test_server_tool_registration.py @@ -33,7 +33,7 @@ def test_no_skills_directory_provider(self): class TestToolRegistration: - """All 54 tools are registered on the MCP server.""" + """All tools are registered on the MCP server.""" @pytest.mark.anyio async def test_all_tools_exist(self, kitchen_enabled): @@ -55,6 +55,7 @@ async def test_all_tools_exist(self, kitchen_enabled): "test_check", "reset_test_dir", "classify_fix", + "commit_files", "reset_workspace", "merge_worktree", "read_db", diff --git a/tests/server/test_tools_commit_files.py b/tests/server/test_tools_commit_files.py new file mode 100644 index 0000000000..14cb27ffd7 --- /dev/null +++ b/tests/server/test_tools_commit_files.py @@ -0,0 +1,490 @@ +"""Tests for the commit_files MCP tool and validate_commit_paths helper.""" + +from __future__ import annotations + +import json +import shutil + +import pytest + +from autoskillit.server.git import validate_commit_paths +from autoskillit.server.tools.tools_git import commit_files +from tests.conftest import _make_result + +pytestmark = [pytest.mark.layer("server"), pytest.mark.small] + + +# --------------------------------------------------------------------------- +# validate_commit_paths — pure function tests +# --------------------------------------------------------------------------- + + +class TestValidateCommitPaths: + def test_relative_path_inside_cwd_is_valid(self, tmp_path): + (tmp_path / "src").mkdir() + (tmp_path / "src" / "file.py").touch() + assert validate_commit_paths(str(tmp_path), ["src/file.py"]) is None + + def test_nested_relative_path_is_valid(self, tmp_path): + assert validate_commit_paths(str(tmp_path), ["a/b/c.py"]) is None + + def test_relative_path_escaping_cwd_is_rejected(self, tmp_path): + result = validate_commit_paths(str(tmp_path), ["../outside.py"]) + assert result is not None + assert "escapes cwd" in result + + def test_relative_path_with_multiple_parent_traversals_is_rejected(self, tmp_path): + # Nested cwd so ../../.. actually escapes the filesystem root ancestry too + nested = tmp_path / "a" / "b" + nested.mkdir(parents=True) + result = validate_commit_paths(str(nested), ["../../../etc/passwd"]) + assert result is not None + assert "escapes cwd" in result + + def test_absolute_path_outside_worktree_is_rejected(self, tmp_path): + outside = tmp_path.parent / "definitely_outside_dir" / "file.py" + result = validate_commit_paths(str(tmp_path), [str(outside)]) + assert result is not None + assert "escapes cwd" in result + + def test_absolute_path_inside_worktree_is_valid(self, tmp_path): + inside = tmp_path / "file.py" + assert validate_commit_paths(str(tmp_path), [str(inside)]) is None + + def test_path_with_dot_git_component_is_rejected(self, tmp_path): + result = validate_commit_paths(str(tmp_path), [".git/config"]) + assert result is not None + assert ".git component" in result + + def test_path_with_dot_git_component_nested_is_rejected(self, tmp_path): + result = validate_commit_paths(str(tmp_path), ["sub/.git/hooks/pre-commit"]) + assert result is not None + assert ".git component" in result + + def test_first_violation_is_reported_when_multiple_paths_given(self, tmp_path): + result = validate_commit_paths(str(tmp_path), ["ok/file.py", "../escape.py"]) + assert result is not None + assert "escape.py" in result + + def test_valid_paths_all_pass(self, tmp_path): + assert validate_commit_paths(str(tmp_path), ["a.py", "b/c.py", "d/e/f.py"]) is None + + def test_cwd_itself_is_a_valid_path(self, tmp_path): + assert validate_commit_paths(str(tmp_path), ["."]) is None + + +# --------------------------------------------------------------------------- +# commit_files tool — happy path +# --------------------------------------------------------------------------- + + +class TestCommitFilesHappyPath: + @pytest.mark.anyio + async def test_stages_commits_and_returns_sha(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) # git add + tool_ctx.runner.push(_make_result(0, "", "")) # git commit + tool_ctx.runner.push(_make_result(0, "abc123def456\n", "")) # rev-parse HEAD + + result = json.loads( + await commit_files(paths=["file.py"], message="fix: thing", cwd=str(wt)) + ) + + assert result == {"success": True, "commit_sha": "abc123def456"} + + @pytest.mark.anyio + async def test_git_add_invoked_with_correct_paths(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "sha\n", "")) + + await commit_files(paths=["a.py", "b.py"], message="msg", cwd=str(wt)) + + add_call = tool_ctx.runner.call_args_list[0][0] + assert add_call == ["git", "-C", str(wt), "add", "--", "a.py", "b.py"] + + @pytest.mark.anyio + async def test_git_commit_invoked_with_message(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "sha\n", "")) + + await commit_files(paths=["a.py"], message="fix: the bug", cwd=str(wt)) + + commit_call = tool_ctx.runner.call_args_list[1][0] + assert commit_call == ["git", "-C", str(wt), "commit", "-m", "fix: the bug"] + assert "--no-verify" not in commit_call + + +# --------------------------------------------------------------------------- +# commit_files tool — containment rejections (never invokes subprocess) +# --------------------------------------------------------------------------- + + +class TestCommitFilesContainmentRejections: + @pytest.mark.anyio + async def test_cwd_missing_returns_error(self, tool_ctx, tmp_path): + missing = tmp_path / "does-not-exist" + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(missing))) + assert result["success"] is False + assert "cwd does not exist" in result["error"] + + @pytest.mark.anyio + async def test_empty_paths_returns_error(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + result = json.loads(await commit_files(paths=[], message="msg", cwd=str(wt))) + assert result["success"] is False + assert "paths list is empty" in result["error"] + + @pytest.mark.anyio + async def test_relative_path_escaping_cwd_is_rejected(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + result = json.loads(await commit_files(paths=["../escape.py"], message="msg", cwd=str(wt))) + assert result["success"] is False + assert "escapes cwd" in result["error"] + assert tool_ctx.runner.call_args_list == [] + + @pytest.mark.anyio + async def test_absolute_path_outside_worktree_is_rejected(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + outside = tmp_path / "outside" / "file.py" + result = json.loads(await commit_files(paths=[str(outside)], message="msg", cwd=str(wt))) + assert result["success"] is False + assert "escapes cwd" in result["error"] + assert tool_ctx.runner.call_args_list == [] + + @pytest.mark.anyio + async def test_dot_git_component_path_is_rejected(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + result = json.loads(await commit_files(paths=[".git/config"], message="msg", cwd=str(wt))) + assert result["success"] is False + assert ".git component" in result["error"] + assert tool_ctx.runner.call_args_list == [] + + +# --------------------------------------------------------------------------- +# commit_files tool — git add failure +# --------------------------------------------------------------------------- + + +class TestCommitFilesGitAddFailure: + @pytest.mark.anyio + async def test_git_add_failure_returns_error_without_committing(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(1, "", "fatal: pathspec did not match")) + + result = json.loads(await commit_files(paths=["missing.py"], message="msg", cwd=str(wt))) + + assert result["success"] is False + assert "git add failed" in result["error"] + assert len(tool_ctx.runner.call_args_list) == 1 + + +# --------------------------------------------------------------------------- +# commit_files tool — pre-commit invocation +# --------------------------------------------------------------------------- + + +class TestCommitFilesPreCommitInvocation: + @pytest.mark.anyio + async def test_pre_commit_invoked_when_config_present(self, tool_ctx, tmp_path, monkeypatch): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []\n") + + monkeypatch.setattr( + shutil, + "which", + lambda cmd, **kw: "/usr/bin/pre-commit" if cmd == "pre-commit" else None, + ) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add + tool_ctx.runner.push(_make_result(0, "", "")) # pre-commit run --files (pass) + tool_ctx.runner.push(_make_result(0, "", "")) # git commit + tool_ctx.runner.push(_make_result(0, "sha123\n", "")) # rev-parse HEAD + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert result["success"] is True + pre_commit_call = tool_ctx.runner.call_args_list[1][0] + assert pre_commit_call == ["/usr/bin/pre-commit", "run", "--files", "a.py"] + assert "--no-verify" not in pre_commit_call + + @pytest.mark.anyio + async def test_pre_commit_not_invoked_without_config(self, tool_ctx, tmp_path, monkeypatch): + wt = tmp_path / "wt" + wt.mkdir() + # No .pre-commit-config.yaml present. + monkeypatch.setattr( + shutil, + "which", + lambda cmd, **kw: "/usr/bin/pre-commit" if cmd == "pre-commit" else None, + ) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add + tool_ctx.runner.push(_make_result(0, "", "")) # git commit + tool_ctx.runner.push(_make_result(0, "sha\n", "")) # rev-parse HEAD + + await commit_files(paths=["a.py"], message="msg", cwd=str(wt)) + + # Only 3 subprocess calls: add, commit, rev-parse — no pre-commit invocation. + assert len(tool_ctx.runner.call_args_list) == 3 + assert tool_ctx.runner.call_args_list[1][0] == [ + "git", + "-C", + str(wt), + "commit", + "-m", + "msg", + ] + + @pytest.mark.anyio + async def test_pre_commit_via_uv_run_when_uv_lock_present( + self, tool_ctx, tmp_path, monkeypatch + ): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []\n") + (wt / "uv.lock").write_text("") + + def _which(cmd, **kw): + if cmd == "uv": + return "/usr/bin/uv" + if cmd == "pre-commit": + return "/usr/bin/pre-commit" + return None + + monkeypatch.setattr(shutil, "which", _which) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add + tool_ctx.runner.push(_make_result(0, "", "")) # uv run pre-commit run --files + tool_ctx.runner.push(_make_result(0, "", "")) # git commit + tool_ctx.runner.push(_make_result(0, "sha\n", "")) # rev-parse HEAD + + await commit_files(paths=["a.py"], message="msg", cwd=str(wt)) + + pre_commit_call = tool_ctx.runner.call_args_list[1][0] + assert pre_commit_call == ["/usr/bin/uv", "run", "pre-commit", "run", "--files", "a.py"] + + +# --------------------------------------------------------------------------- +# commit_files tool — hook auto-fix retry path +# --------------------------------------------------------------------------- + + +class TestCommitFilesHookAutoFixRetry: + @pytest.mark.anyio + async def test_hook_autofix_re_adds_and_retries_commit_once( + self, tool_ctx, tmp_path, monkeypatch + ): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []\n") + monkeypatch.setattr( + shutil, + "which", + lambda cmd, **kw: "/usr/bin/pre-commit" if cmd == "pre-commit" else None, + ) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add (initial) + tool_ctx.runner.push( + _make_result(1, "", "files were modified by this hook") + ) # pre-commit fails (auto-fix) + tool_ctx.runner.push(_make_result(0, "", "")) # git add (re-add after auto-fix) + tool_ctx.runner.push(_make_result(0, "", "")) # pre-commit run --files (retry, passes) + tool_ctx.runner.push(_make_result(0, "", "")) # git commit + tool_ctx.runner.push(_make_result(0, "sha456\n", "")) # rev-parse HEAD + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert result == {"success": True, "commit_sha": "sha456"} + calls = [c[0] for c in tool_ctx.runner.call_args_list] + assert calls == [ + ["git", "-C", str(wt), "add", "--", "a.py"], + ["/usr/bin/pre-commit", "run", "--files", "a.py"], + ["git", "-C", str(wt), "add", "--", "a.py"], + ["/usr/bin/pre-commit", "run", "--files", "a.py"], + ["git", "-C", str(wt), "commit", "-m", "msg"], + ["git", "-C", str(wt), "rev-parse", "HEAD"], + ] + + +# --------------------------------------------------------------------------- +# commit_files tool — hard hook failure +# --------------------------------------------------------------------------- + + +class TestCommitFilesHardHookFailure: + @pytest.mark.anyio + async def test_re_add_failure_after_hook_failure_returns_error( + self, tool_ctx, tmp_path, monkeypatch + ): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []\n") + monkeypatch.setattr( + shutil, + "which", + lambda cmd, **kw: "/usr/bin/pre-commit" if cmd == "pre-commit" else None, + ) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add (initial) + tool_ctx.runner.push(_make_result(1, "", "hook failed hard")) # pre-commit fails + tool_ctx.runner.push(_make_result(1, "", "fatal: pathspec")) # re-add fails + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert result["success"] is False + assert "pre-commit + re-add failed" in result["error"] + # Never reaches git commit. + assert len(tool_ctx.runner.call_args_list) == 3 + + @pytest.mark.anyio + async def test_retry_pre_commit_still_failing_returns_error_no_commit( + self, tool_ctx, tmp_path, monkeypatch + ): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []\n") + monkeypatch.setattr( + shutil, + "which", + lambda cmd, **kw: "/usr/bin/pre-commit" if cmd == "pre-commit" else None, + ) + + tool_ctx.runner.push(_make_result(0, "", "")) # git add (initial) + tool_ctx.runner.push(_make_result(1, "", "lint error")) # pre-commit fails + tool_ctx.runner.push(_make_result(0, "", "")) # re-add succeeds + tool_ctx.runner.push(_make_result(1, "", "lint error persists")) # retry still fails + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert result["success"] is False + assert "pre-commit retry failed" in result["error"] + assert "lint error persists" in result["error"] + # No commit call occurred — only 4 subprocess calls total. + assert len(tool_ctx.runner.call_args_list) == 4 + for call in tool_ctx.runner.call_args_list: + assert call[0][:3] != ["git", "-C", str(wt)] or "commit" not in call[0] + + +# --------------------------------------------------------------------------- +# commit_files tool — git commit failure (post pre-commit) +# --------------------------------------------------------------------------- + + +class TestCommitFilesGitCommitFailure: + @pytest.mark.anyio + async def test_git_commit_failure_returns_error(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) # git add + tool_ctx.runner.push(_make_result(1, "", "nothing to commit")) # git commit fails + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert result["success"] is False + assert "git commit failed" in result["error"] + assert "nothing to commit" in result["error"] + + +# --------------------------------------------------------------------------- +# commit_files tool — envelope shape parity with push_to_remote +# --------------------------------------------------------------------------- + + +class TestCommitFilesEnvelopeShape: + @pytest.mark.anyio + async def test_success_envelope_has_success_true_and_commit_sha(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "deadbeef\n", "")) + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert set(result.keys()) == {"success", "commit_sha"} + assert result["success"] is True + assert isinstance(result["commit_sha"], str) + + @pytest.mark.anyio + async def test_failure_envelope_has_success_false_and_error(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(1, "", "boom")) + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + + assert set(result.keys()) == {"success", "error"} + assert result["success"] is False + assert isinstance(result["error"], str) + + @pytest.mark.anyio + async def test_validation_failure_envelope_matches_shape(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + + result = json.loads(await commit_files(paths=["../escape.py"], message="msg", cwd=str(wt))) + + assert set(result.keys()) == {"success", "error"} + assert result["success"] is False + + +# --------------------------------------------------------------------------- +# commit_files tool — timing +# --------------------------------------------------------------------------- + + +class TestCommitFilesTiming: + @pytest.mark.anyio + async def test_step_name_records_timing(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "sha\n", "")) + + await commit_files(paths=["a.py"], message="msg", cwd=str(wt), step_name="commit") + + report = tool_ctx.timing_log.get_report() + assert any(e["step_name"] == "commit" for e in report) + + @pytest.mark.anyio + async def test_empty_step_name_skips_timing(self, tool_ctx, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "", "")) + tool_ctx.runner.push(_make_result(0, "sha\n", "")) + + await commit_files(paths=["a.py"], message="msg", cwd=str(wt)) + + assert tool_ctx.timing_log.get_report() == [] + + @pytest.mark.anyio + async def test_pre_commit_config_present_but_binary_missing_returns_error( + self, tool_ctx, tmp_path, monkeypatch + ): + wt = tmp_path / "wt" + wt.mkdir() + (wt / ".pre-commit-config.yaml").write_text("repos: []") + (wt / "a.py").write_text("x = 1\n") + tool_ctx.runner.push(_make_result(0, "", "")) + + monkeypatch.setattr(shutil, "which", lambda *a, **kw: None) + + result = json.loads(await commit_files(paths=["a.py"], message="msg", cwd=str(wt))) + assert result["success"] is False + assert "pre-commit" in result["error"] + assert "binary" in result["error"] or "not found" in result["error"] diff --git a/tests/workspace/test_skills.py b/tests/workspace/test_skills.py index 2d45501d1a..16a53d9732 100644 --- a/tests/workspace/test_skills.py +++ b/tests/workspace/test_skills.py @@ -815,6 +815,8 @@ def test_unknown_capability_pruned_with_warning(self, tmp_path) -> None: with structlog.testing.capture_logs() as logs: info = _skill_info_from_frontmatter("test", SkillSource.BUNDLED, skill_md) assert info.backend_requirements == frozenset() + assert info.invalid_reason is not None + assert "nonexistent_cap" in info.invalid_reason assert any(log["event"] == "unrecognized_uses_capabilities" for log in logs) def test_mixed_capabilities_derive_backend_requirements(self, tmp_path) -> None: @@ -868,7 +870,7 @@ def test_all_skillinfo_fields_parsed_by_frontmatter_function(self) -> None: dc_fields = {f.name for f in dataclasses.fields(SkillInfo)} constructor_only = {"name", "source", "path"} - derived_fields = {"backend_requirements"} + derived_fields = {"backend_requirements", "invalid_reason"} parseable_fields = dc_fields - constructor_only - derived_fields source = inspect.getsource(_skill_info_from_frontmatter)