From dc0dee46bd96e68037e83d31d6494c8004d7d6bd Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 11:30:29 -0700 Subject: [PATCH 01/15] feat: add github_api_write capability with network_access wiring (Part A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the recurring defect class where Codex sandbox blocks undeclared operation classes. Adds `github_api_write` capability with `required_sandbox_overrides` wired through the full dispatch chain: SKILL.md → run_skill aggregation → SkillSessionConfig → _apply_config → CodexBackend.build_skill_session_cmd → _codex_exec_base extra_overrides → codex exec --config sandbox_workspace_write.network_access=true Also generalizes capability routing from hardcoded "git_metadata_write" name checks to registry-driven checks (any cap with non-empty required_backends), making the routing machinery self-extending. Adds `github_api_callable: bool = False` to BackendCapabilities, declares `github_api_write` on review-pr/SKILL.md, and adds merge-prs to the admission dispatch agreement test. Tests T-A1 through T-A11 added. Co-Authored-By: Claude Sonnet 4.6 --- src/autoskillit/core/types/_type_backend.py | 4 ++ .../core/types/_type_constants_registries.py | 10 +++ .../core/types/_type_protocols_execution.py | 1 + .../backends/_backend_cmd_builder_base.py | 1 + src/autoskillit/execution/backends/claude.py | 1 + src/autoskillit/execution/backends/codex.py | 7 +++ .../execution/headless/__init__.py | 4 ++ .../server/tools/_auto_overrides.py | 20 +++--- .../server/tools/tools_execution.py | 31 ++++++--- .../skills_extended/review-pr/SKILL.md | 2 +- tests/arch/test_capability_consumption.py | 8 +++ tests/arch/test_skill_backend_annotations.py | 63 +++++++++++++++++++ .../test_backend_sandbox_invariants.py | 22 +++++++ .../test_codex_capabilities_fields.py | 26 ++++++++ tests/execution/test_headless_dispatch.py | 29 +++++++++ .../test_admission_dispatch_agreement.py | 1 + 16 files changed, 210 insertions(+), 20 deletions(-) diff --git a/src/autoskillit/core/types/_type_backend.py b/src/autoskillit/core/types/_type_backend.py index 76918f210e..27f44fec18 100644 --- a/src/autoskillit/core/types/_type_backend.py +++ b/src/autoskillit/core/types/_type_backend.py @@ -142,6 +142,8 @@ class BackendCapabilities: has_unguarded_filesystem_access: bool = field(default=False) # True when backend's git metadata directories (.git/worktrees/) are writable git_metadata_writable: bool = field(default=True) + # True when the backend can make outbound GitHub API write calls without sandbox restriction + github_api_callable: bool = field(default=False) # Native skill invocation prefix character used by this backend's model/CLI. # Claude Code uses "/" (slash-commands via the Skill tool). # Codex uses "$" (dollar-mention via extract_tool_mentions). @@ -262,6 +264,7 @@ def model_class(model: str) -> str: supports_context_window_suffix=True, has_unguarded_filesystem_access=False, git_metadata_writable=True, + github_api_callable=True, skill_sigil="/", session_dir_persistent=False, supports_model_invocation_gating=True, @@ -313,6 +316,7 @@ class SkillSessionConfig: resume_message: str | None = None sandbox_mode: str = "workspace-write" backend_override: str | None = None + network_access: bool = False @dataclass(frozen=True, slots=True) diff --git a/src/autoskillit/core/types/_type_constants_registries.py b/src/autoskillit/core/types/_type_constants_registries.py index 23ae967a7b..53a4d0040d 100644 --- a/src/autoskillit/core/types/_type_constants_registries.py +++ b/src/autoskillit/core/types/_type_constants_registries.py @@ -302,6 +302,7 @@ class SkillCapabilityDef: description: str codex_status: Literal["works-as-is", "degraded", "fix-required", "not-applicable"] + required_sandbox_overrides: frozenset[str] = frozenset() @property def required_backends(self) -> frozenset[str]: @@ -354,6 +355,15 @@ def required_backends(self) -> frozenset[str]: ), codex_status="not-applicable", ), + "github_api_write": SkillCapabilityDef( + description=( + "Skill makes outbound GitHub API write calls (gh pr review, gh api --method POST, " + "gh pr create, gh pr merge, gh issue create/edit/close, etc.) that require network " + "access. On Codex workers, enables network_access=true in the workspace-write sandbox." + ), + codex_status="fix-required", + required_sandbox_overrides=frozenset({"sandbox_workspace_write.network_access=true"}), + ), } _VALID_CODEX_STATUSES = {"works-as-is", "degraded", "fix-required", "not-applicable"} diff --git a/src/autoskillit/core/types/_type_protocols_execution.py b/src/autoskillit/core/types/_type_protocols_execution.py index 189e73c7db..511c1b9542 100644 --- a/src/autoskillit/core/types/_type_protocols_execution.py +++ b/src/autoskillit/core/types/_type_protocols_execution.py @@ -73,6 +73,7 @@ async def run( caller_session_id: str | None = None, inspector_eligible: bool = False, inspector_model: str = "", + network_access: bool = False, ) -> SkillResult: ... async def dispatch_food_truck( diff --git a/src/autoskillit/execution/backends/_backend_cmd_builder_base.py b/src/autoskillit/execution/backends/_backend_cmd_builder_base.py index ea4f059f67..afcefd391f 100644 --- a/src/autoskillit/execution/backends/_backend_cmd_builder_base.py +++ b/src/autoskillit/execution/backends/_backend_cmd_builder_base.py @@ -163,6 +163,7 @@ def _apply_config(self, config: SkillSessionConfig) -> dict[str, Any]: "resume_checkpoint": config.resume_checkpoint, "resume_message": config.resume_message, "sandbox_mode": config.sandbox_mode, + "network_access": config.network_access, } diff --git a/src/autoskillit/execution/backends/claude.py b/src/autoskillit/execution/backends/claude.py index 7bdc23eddb..dad366f5e7 100644 --- a/src/autoskillit/execution/backends/claude.py +++ b/src/autoskillit/execution/backends/claude.py @@ -543,6 +543,7 @@ def build_skill_session_cmd( resume_checkpoint = cfg["resume_checkpoint"] resume_message = cfg["resume_message"] sandbox_mode = cfg["sandbox_mode"] # noqa: F841 + _net = cfg.get("network_access", False) # noqa: F841 _has_prefix = ( bool(profile_name) diff --git a/src/autoskillit/execution/backends/codex.py b/src/autoskillit/execution/backends/codex.py index b5bb19dcee..c91961ce40 100644 --- a/src/autoskillit/execution/backends/codex.py +++ b/src/autoskillit/execution/backends/codex.py @@ -592,6 +592,7 @@ def capabilities(self) -> BackendCapabilities: # field; rules_backend_compat.py emits ERROR for # git_metadata_write skills on backends where this is False. git_metadata_writable=False, # sandbox excludes .git metadata path + github_api_callable=False, skill_sigil="$", session_dir_persistent=True, supports_model_invocation_gating=False, @@ -692,6 +693,7 @@ def build_skill_session_cmd( resume_checkpoint: SessionCheckpoint | None = None, resume_message: str | None = None, sandbox_mode: str = "workspace-write", + network_access: bool = False, ) -> CmdSpec: if config is not None: cfg = self._apply_config(config) @@ -712,6 +714,7 @@ def build_skill_session_cmd( resume_checkpoint = cfg["resume_checkpoint"] resume_message = cfg["resume_message"] sandbox_mode = cfg["sandbox_mode"] + network_access = cfg.get("network_access", False) if plugin_source is not None: logger.warning("codex_plugin_source_discarded", plugin_source=str(plugin_source)) if output_format != OutputFormat.JSON: @@ -790,9 +793,13 @@ def build_skill_session_cmd( required=SKILL_SESSION_REQUIRED_ENV | {MCP_CLIENT_BACKEND_ENV_VAR}, ) + _net_overrides: list[str] = [] + if network_access: + _net_overrides.append("sandbox_workspace_write.network_access=true") cmd = _codex_exec_base( sandbox=sandbox_mode, bypass_hook_trust=self.capabilities.mcp_config_capable, + extra_overrides=_net_overrides, ) if model: cmd += [CodexFlags.MODEL, self.translate_model(model)] diff --git a/src/autoskillit/execution/headless/__init__.py b/src/autoskillit/execution/headless/__init__.py index c801de6f03..060fd1c095 100644 --- a/src/autoskillit/execution/headless/__init__.py +++ b/src/autoskillit/execution/headless/__init__.py @@ -145,6 +145,7 @@ async def run_headless_core( caller_session_id: str | None = None, inspector_eligible: bool = False, inspector_model: str = "", + network_access: bool = False, ) -> SkillResult: """Shared headless runner used by run_skill. @@ -193,6 +194,7 @@ async def run_headless_core( sandbox_mode="read-only" if readonly_skill else ctx.backend.capabilities.default_skill_sandbox_mode, + network_access=network_access, ) step_backend: CodingAgentBackend | None = None if backend_override is not None: @@ -303,6 +305,7 @@ async def run( caller_session_id: str | None = None, inspector_eligible: bool = False, inspector_model: str = "", + network_access: bool = False, ) -> SkillResult: cfg = self._ctx.config.run_skill effective_timeout = timeout if timeout is not None else cfg.timeout @@ -344,6 +347,7 @@ async def run( caller_session_id=caller_session_id, inspector_eligible=inspector_eligible, inspector_model=inspector_model, + network_access=network_access, ) async def dispatch_food_truck( diff --git a/src/autoskillit/server/tools/_auto_overrides.py b/src/autoskillit/server/tools/_auto_overrides.py index 42d0b5168c..0d0d884e88 100644 --- a/src/autoskillit/server/tools/_auto_overrides.py +++ b/src/autoskillit/server/tools/_auto_overrides.py @@ -15,6 +15,7 @@ from autoskillit.core import ( AGENT_BACKEND_CLAUDE_CODE, CAPABILITY_INGREDIENT_TO_SKIP_GUARD, + SKILL_CAPABILITY_REGISTRY, SKILL_TOOLS, CapabilityResolutionDetail, extract_skill_name, @@ -100,8 +101,10 @@ def _provider_aware_capability_overrides( if not skill_name: continue cap_resolved = skill_resolver.resolve(skill_name) - if cap_resolved and "git_metadata_write" in getattr( - cap_resolved, "uses_capabilities", frozenset() + if cap_resolved and any( + SKILL_CAPABILITY_REGISTRY.get(cap) is not None + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + for cap in getattr(cap_resolved, "uses_capabilities", frozenset()) ): has_capability_requirement = True break @@ -251,15 +254,10 @@ def _compute_effective_backend_map( skill_name = extract_skill_name(skill_cmd) if skill_cmd else None if skill_name: resolved = skill_resolver.resolve(skill_name) - _resolved_reqs: frozenset[str] = ( - getattr(resolved, "backend_requirements", frozenset()) - if resolved - else frozenset() - ) - if ( - resolved - and "git_metadata_write" in getattr(resolved, "uses_capabilities", frozenset()) - and _resolved_reqs + if resolved and any( + SKILL_CAPABILITY_REGISTRY.get(cap) is not None + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + for cap in getattr(resolved, "uses_capabilities", frozenset()) ): result[step_name] = AGENT_BACKEND_CLAUDE_CODE continue diff --git a/src/autoskillit/server/tools/tools_execution.py b/src/autoskillit/server/tools/tools_execution.py index 0afb7ff8c4..4d2e8e93aa 100644 --- a/src/autoskillit/server/tools/tools_execution.py +++ b/src/autoskillit/server/tools/tools_execution.py @@ -19,6 +19,7 @@ AGENT_BACKEND_CLAUDE_CODE, CODEX_SESSIONS_SUBDIR, DISPATCH_ID_ENV_VAR, + SKILL_CAPABILITY_REGISTRY, SKILL_COMMAND_DISPLAY_MAX, WORKTREE_SKILLS, ClaudeDirectoryConventions, @@ -766,29 +767,42 @@ async def run_skill( and not tool_ctx.backend.capabilities.anthropic_provider_capable ) - _skill_reqs: frozenset[str] = ( - getattr(_skill_info, "backend_requirements", frozenset()) - if _skill_info - else frozenset() - ) _skill_caps: frozenset[str] = ( getattr(_skill_info, "uses_capabilities", frozenset()) if _skill_info else frozenset() ) + _sandbox_overrides: frozenset[str] = frozenset().union( + *( + SKILL_CAPABILITY_REGISTRY[cap].required_sandbox_overrides + for cap in _skill_caps + if cap in SKILL_CAPABILITY_REGISTRY + ) + ) + _network_access = "sandbox_workspace_write.network_access=true" in _sandbox_overrides + _has_routing_cap = any( + SKILL_CAPABILITY_REGISTRY.get(cap) is not None + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + for cap in _skill_caps + ) _skill_requires_claude = bool( - _skill_reqs - and "git_metadata_write" in _skill_caps + _has_routing_cap and tool_ctx.backend is not None and not tool_ctx.backend.capabilities.anthropic_provider_capable ) if _skill_requires_claude: if shutil.which("claude") is None: + _routing_caps = sorted( + cap + for cap in _skill_caps + if SKILL_CAPABILITY_REGISTRY.get(cap) + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + ) return SkillResult.crashed( exception=RuntimeError( f"Skill {target_name!r} requires claude-code backend " - f"(git_metadata_write capability) but 'claude' binary " + f"({', '.join(_routing_caps)} capability) but 'claude' binary " f"is not found on PATH. Install Claude Code CLI to " f"enable capability-driven routing." ), @@ -1124,6 +1138,7 @@ async def run_skill( caller_session_id=_orchestrator_sid, inspector_eligible=_in_fleet_dispatch and bool(_inspector_model), inspector_model=_inspector_model, + network_access=_network_access, ) except TimeoutError as exc: logger.error( diff --git a/src/autoskillit/skills_extended/review-pr/SKILL.md b/src/autoskillit/skills_extended/review-pr/SKILL.md index 91610ab0f8..69cc4c30e3 100644 --- a/src/autoskillit/skills_extended/review-pr/SKILL.md +++ b/src/autoskillit/skills_extended/review-pr/SKILL.md @@ -1,7 +1,7 @@ --- name: review-pr categories: [github] -uses_capabilities: [agent_model, cross_skill_ref, run_skill] +uses_capabilities: [agent_model, cross_skill_ref, github_api_write, run_skill] description: Automated diff-scoped PR code review using parallel audit subagents. Posts inline GitHub review comments and submits a summary verdict. Use after a PR is opened to gate CI on review approval. hooks: PreToolUse: diff --git a/tests/arch/test_capability_consumption.py b/tests/arch/test_capability_consumption.py index 117475c713..4a2f132d2b 100644 --- a/tests/arch/test_capability_consumption.py +++ b/tests/arch/test_capability_consumption.py @@ -60,6 +60,14 @@ class ForwardDeclaredField: rationale="patch path extraction routing — P2-A3-WP1 (#3787) co-lands consumer", added_date=date(2026, 6, 5), ), + "github_api_callable": ForwardDeclaredField( + issue=4204, + rationale=( + "Behavioral documentation field; production consumer added when " + "network-capability gate is wired to a BackendCapabilities check" + ), + added_date=date(2026, 7, 7), + ), } diff --git a/tests/arch/test_skill_backend_annotations.py b/tests/arch/test_skill_backend_annotations.py index 8aaf0b620c..5b3f598674 100644 --- a/tests/arch/test_skill_backend_annotations.py +++ b/tests/arch/test_skill_backend_annotations.py @@ -30,6 +30,14 @@ re.compile(r"git\s+(?:-C\s+\S+\s+)?commit\s+-m"), re.compile(r'\bgit\s+(?:-C\s+\S+\s+)?rebase\s+(?:--\w|[$"\{])'), ], + "github_api_write": [ + re.compile( + r"gh api[^\n]*(?:--method\s+(?:POST|PATCH|PUT|DELETE))" + r"|gh pr (?:review|create|merge)\b" + r"|gh issue (?:create|edit|close)\b" + r"|gh release create\b" + ), + ], } @@ -137,3 +145,58 @@ def test_derivation_backend_requirements_match_capabilities(): f"{len(violations)} skill(s) still have backend_requirements in SKILL.md frontmatter " f"(should be derived at runtime):\n" + "\n".join(f" {v}" for v in violations) ) + + +def test_github_api_write_pattern_detected() -> None: + """_CAPABILITY_PATTERNS["github_api_write"] matches GitHub write CLI patterns.""" + patterns = _CAPABILITY_PATTERNS["github_api_write"] + should_match = [ + "gh pr review --approve", + "gh api /repos/foo/bar --method POST", + "gh pr create --title foo", + "gh pr merge --squash", + "gh issue create --title bar", + ] + should_not_match = [ + "gh pr list", + "gh pr view 123", + "gh issue list", + ] + for text in should_match: + assert any(p.search(text) for p in patterns), ( + f"github_api_write pattern should match: {text!r}" + ) + for text in should_not_match: + assert not any(p.search(text) for p in patterns), ( + f"github_api_write pattern should NOT match: {text!r}" + ) + + +def test_review_pr_declares_github_api_write() -> None: + """review-pr must declare github_api_write in uses_capabilities.""" + from autoskillit.core import pkg_root + + skill_md = pkg_root() / "skills_extended" / "review-pr" / "SKILL.md" + fm = _read_skill_frontmatter(skill_md) + assert "github_api_write" in set(fm.get("uses_capabilities", [])), ( + "review-pr SKILL.md must declare uses_capabilities: [..., github_api_write, ...]" + ) + + +def test_capability_routing_uses_registry_not_hardcoded_name() -> None: + """run_skill routing must not hardcode 'git_metadata_write' — must be registry-driven.""" + import ast + + from autoskillit.core import pkg_root + + tools_exec = pkg_root() / "server" / "tools" / "tools_execution.py" + source = tools_exec.read_text(encoding="utf-8") + tree = ast.parse(source) + for node in ast.walk(tree): + if isinstance(node, ast.Constant) and node.value == "git_metadata_write": + lineno = node.lineno + context = source.splitlines()[lineno - 1].strip() + assert False, ( + f"tools_execution.py line {lineno} still references literal 'git_metadata_write' " + f"in routing logic — must use registry-driven check: {context!r}" + ) diff --git a/tests/execution/backends/test_backend_sandbox_invariants.py b/tests/execution/backends/test_backend_sandbox_invariants.py index 99a3e64b2c..0a26bda0fd 100644 --- a/tests/execution/backends/test_backend_sandbox_invariants.py +++ b/tests/execution/backends/test_backend_sandbox_invariants.py @@ -52,3 +52,25 @@ def test_build_skill_session_cmd_no_sandbox_flag(self) -> None: output_format=OutputFormat.JSON, ) assert "--sandbox" not in spec.cmd + + +class TestNetworkAccessOverride: + """T-A5, T-A6: network_access in SkillSessionConfig drives Codex extra override.""" + + def test_skill_session_with_network_access_gets_override(self) -> None: + config = SkillSessionConfig(network_access=True) + spec: CmdSpec = CodexBackend().build_skill_session_cmd( + "/test-skill", cwd="", config=config + ) + assert any("sandbox_workspace_write.network_access=true" in part for part in spec.cmd), ( + f"Expected network_access override in cmd, got: {spec.cmd}" + ) + + def test_skill_session_without_network_access_no_override(self) -> None: + config = SkillSessionConfig() + spec: CmdSpec = CodexBackend().build_skill_session_cmd( + "/test-skill", cwd="", config=config + ) + assert not any( + "sandbox_workspace_write.network_access=true" in part for part in spec.cmd + ), f"Unexpected network_access override in cmd: {spec.cmd}" diff --git a/tests/execution/backends/test_codex_capabilities_fields.py b/tests/execution/backends/test_codex_capabilities_fields.py index 869ed212fa..7e985d5150 100644 --- a/tests/execution/backends/test_codex_capabilities_fields.py +++ b/tests/execution/backends/test_codex_capabilities_fields.py @@ -34,3 +34,29 @@ def test_git_metadata_writable(self): from autoskillit.execution.backends.codex import CodexBackend assert CodexBackend().capabilities.git_metadata_writable is False + + def test_github_api_callable_false(self): + from autoskillit.execution.backends.codex import CodexBackend + + assert CodexBackend().capabilities.github_api_callable is False + + +class TestSandboxOverridesAggregate: + """T-A7: github_api_write registry entry produces correct sandbox override aggregate.""" + + def test_capability_sandbox_overrides_aggregate_to_network_access(self): + from autoskillit.core.types._type_constants_registries import SKILL_CAPABILITY_REGISTRY + + uses_caps = frozenset({"github_api_write"}) + sandbox_overrides: frozenset[str] = frozenset().union( + *( + SKILL_CAPABILITY_REGISTRY[cap].required_sandbox_overrides + for cap in uses_caps + if cap in SKILL_CAPABILITY_REGISTRY + ) + ) + network_access = "sandbox_workspace_write.network_access=true" in sandbox_overrides + assert network_access, ( + "github_api_write capability must aggregate to network_access=True via " + "sandbox_workspace_write.network_access=true override" + ) diff --git a/tests/execution/test_headless_dispatch.py b/tests/execution/test_headless_dispatch.py index 4c699179a8..24f56f3473 100644 --- a/tests/execution/test_headless_dispatch.py +++ b/tests/execution/test_headless_dispatch.py @@ -520,3 +520,32 @@ def test_build_interactive_cmd_no_headless_hardening(monkeypatch: pytest.MonkeyP spec = ClaudeCodeBackend().build_interactive_cmd() assert spec.env.get("TERM") != "dumb" assert "NO_COLOR" not in spec.env + + +@pytest.mark.anyio +async def test_headless_executor_forwards_network_access( + minimal_ctx, monkeypatch: pytest.MonkeyPatch +) -> None: + """T-A11: DefaultHeadlessExecutor.run(network_access=True) forwards to run_headless_core.""" + from unittest.mock import AsyncMock, patch + + from autoskillit.core import SkillResult + from autoskillit.execution.headless import DefaultHeadlessExecutor + + captured_kwargs: dict = {} + + async def fake_run_headless_core(*args, **kwargs): + captured_kwargs.update(kwargs) + return SkillResult.crashed(exception=RuntimeError("test"), skill_command="/test-skill") + + with patch( + "autoskillit.execution.headless.run_headless_core", + new=AsyncMock(side_effect=fake_run_headless_core), + ): + executor = DefaultHeadlessExecutor(minimal_ctx) + await executor.run("/test-skill", cwd="", network_access=True) + + assert captured_kwargs.get("network_access") is True, ( + "DefaultHeadlessExecutor.run(network_access=True) must forward network_access=True " + f"to run_headless_core, got: {captured_kwargs.get('network_access')!r}" + ) diff --git a/tests/server/test_admission_dispatch_agreement.py b/tests/server/test_admission_dispatch_agreement.py index dc72c5d57b..d6263a1f01 100644 --- a/tests/server/test_admission_dispatch_agreement.py +++ b/tests/server/test_admission_dispatch_agreement.py @@ -41,6 +41,7 @@ "implementation", "implementation-groups", "remediation", + "merge-prs", ) _BACKENDS: tuple[str, ...] = tuple(sorted(BACKEND_REGISTRY.keys())) _SKILL_RESOLVER = DefaultSkillResolver() From 3db71c689281b53ee2e6f6bfac297e3ba4039f02 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 11:51:11 -0700 Subject: [PATCH 02/15] fix: resolve test failures from github_api_write capability implementation - Extract generator expressions from run_skill() handler body to standalone helper functions (_aggregate_sandbox_overrides, _has_routing_capability, _get_routing_caps) to satisfy REQ-CNST-008 no-business-logic-in-handlers rule - Add network_access: bool = False to InMemoryHeadlessExecutor.run() and ExecutorCall to match updated HeadlessExecutor Protocol signature - Update test_backend_capabilities field sets to include github_api_callable - Update SkillSessionConfig exhaustive field test to include network_access - Add github_api_callable to NOT_YET_LIVE and CAPABILITY_CLASSIFICATION in conformance tests - Update test_run_skill_references_git_metadata_write_capability to check for registry-driven routing pattern (_has_routing_capability / required_backends) - Bump line budgets: tools_execution.py (1260 to 1295), codex.py (1150 to 1155), headless/__init__.py (510 to 520) to account for new implementation lines - Declare github_api_write in uses_capabilities for 16 skills that make mutating GitHub API calls (Part A of the 17-skill declaration set) Co-Authored-By: Claude Sonnet 4.6 --- .../server/tools/tools_execution.py | 51 ++++++++++++------- src/autoskillit/skills/sous-chef/SKILL.md | 2 +- .../skills_extended/audit-claims/SKILL.md | 2 +- .../skills_extended/collapse-issues/SKILL.md | 2 +- .../skills_extended/compose-pr/SKILL.md | 2 +- .../compose-research-pr/SKILL.md | 1 + .../skills_extended/enrich-issues/SKILL.md | 2 +- .../skills_extended/issue-splitter/SKILL.md | 1 + .../skills_extended/merge-pr/SKILL.md | 2 +- .../open-integration-pr/SKILL.md | 2 +- .../skills_extended/pipeline-summary/SKILL.md | 1 + .../skills_extended/prepare-issue/SKILL.md | 1 + .../skills_extended/process-issues/SKILL.md | 2 +- .../skills_extended/promote-to-main/SKILL.md | 2 +- .../skills_extended/resolve-review/SKILL.md | 2 +- .../review-research-pr/SKILL.md | 2 +- .../skills_extended/triage-issues/SKILL.md | 2 +- .../arch/test_capability_admission_control.py | 10 ++-- tests/arch/test_execution_source_split.py | 2 +- tests/arch/test_subpackage_isolation.py | 11 ++-- tests/core/test_backend_capabilities.py | 2 + tests/core/test_backend_dataclasses.py | 1 + .../test_coding_agent_backend_conformance.py | 2 + tests/fakes.py | 3 ++ 24 files changed, 72 insertions(+), 38 deletions(-) diff --git a/src/autoskillit/server/tools/tools_execution.py b/src/autoskillit/server/tools/tools_execution.py index 4d2e8e93aa..4a1e729cca 100644 --- a/src/autoskillit/server/tools/tools_execution.py +++ b/src/autoskillit/server/tools/tools_execution.py @@ -505,6 +505,36 @@ def _compute_write_prefixes( return base_prefixes[0] if base_prefixes else "", tuple(all_prefixes) +def _aggregate_sandbox_overrides(skill_caps: frozenset[str]) -> frozenset[str]: + """Aggregate required_sandbox_overrides from all declared capabilities.""" + return frozenset().union( + *( + SKILL_CAPABILITY_REGISTRY[cap].required_sandbox_overrides + for cap in skill_caps + if cap in SKILL_CAPABILITY_REGISTRY + ) + ) + + +def _has_routing_capability(skill_caps: frozenset[str]) -> bool: + """Return True if any declared capability requires a specific backend.""" + return any( + SKILL_CAPABILITY_REGISTRY.get(cap) is not None + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + for cap in skill_caps + ) + + +def _get_routing_caps(skill_caps: frozenset[str]) -> list[str]: + """Return sorted list of capabilities that require a specific backend.""" + return sorted( + cap + for cap in skill_caps + if SKILL_CAPABILITY_REGISTRY.get(cap) + and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) + ) + + @mcp.tool(tags={"autoskillit", "kitchen", "kitchen-core"}, annotations={"readOnlyHint": True}) @_cancellation_shield() @track_response_size("run_skill") @@ -772,19 +802,9 @@ async def run_skill( if _skill_info else frozenset() ) - _sandbox_overrides: frozenset[str] = frozenset().union( - *( - SKILL_CAPABILITY_REGISTRY[cap].required_sandbox_overrides - for cap in _skill_caps - if cap in SKILL_CAPABILITY_REGISTRY - ) - ) + _sandbox_overrides = _aggregate_sandbox_overrides(_skill_caps) _network_access = "sandbox_workspace_write.network_access=true" in _sandbox_overrides - _has_routing_cap = any( - SKILL_CAPABILITY_REGISTRY.get(cap) is not None - and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) - for cap in _skill_caps - ) + _has_routing_cap = _has_routing_capability(_skill_caps) _skill_requires_claude = bool( _has_routing_cap and tool_ctx.backend is not None @@ -793,12 +813,7 @@ async def run_skill( if _skill_requires_claude: if shutil.which("claude") is None: - _routing_caps = sorted( - cap - for cap in _skill_caps - if SKILL_CAPABILITY_REGISTRY.get(cap) - and bool(SKILL_CAPABILITY_REGISTRY[cap].required_backends) - ) + _routing_caps = _get_routing_caps(_skill_caps) return SkillResult.crashed( exception=RuntimeError( f"Skill {target_name!r} requires claude-code backend " diff --git a/src/autoskillit/skills/sous-chef/SKILL.md b/src/autoskillit/skills/sous-chef/SKILL.md index b50bfb1b99..520c75aaff 100644 --- a/src/autoskillit/skills/sous-chef/SKILL.md +++ b/src/autoskillit/skills/sous-chef/SKILL.md @@ -1,6 +1,6 @@ --- name: sous-chef -uses_capabilities: [cross_skill_ref, open_kitchen, run_skill, test_check] +uses_capabilities: [cross_skill_ref, github_api_write, open_kitchen, run_skill, test_check] description: Internal bootstrap document injected by open_kitchen into every orchestrator session. --- + ## implementation-groups Group-based implementation with per-group plan/implement/test cycles and PR gates. diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index a3c6ba26c1..ff724ad9f6 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 99ee8de0c2..ba233175e3 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 985f7fd04a..9e8b2e231c 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation Investigate, rectify, implement, and merge a bug fix with CI and PR gates. diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index d16db40f04..c41ad5fdf2 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -1123,14 +1123,14 @@ }, { "when": "${{ result.verdict }} == needs_human", - "route": "check_review_loop" + "route": "check_review_posted" }, { "when": "${{ result.verdict }} == approved", - "route": "check_review_loop" + "route": "check_review_posted" }, { - "route": "check_review_loop" + "route": "check_review_posted" } ], "on_failure": "check_repo_ci_event", @@ -1141,7 +1141,7 @@ "review_loop_count", "review_mode" ], - "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → resolve_review; all other verdicts (including approved, needs_human) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" + "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → resolve_review; all other verdicts (including approved, needs_human) → check_review_posted (effect verification gate) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" }, "resolve_review": { "tool": "run_skill", @@ -1243,9 +1243,34 @@ "branch": "${{ context.merge_target }}", "force": "true" }, - "on_success": "check_review_loop", + "on_success": "check_review_posted", "on_failure": "release_issue_failure", - "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_loop to determine if another review-resolve cycle is needed (bounded by review_max_retries). On push failure, escalates via release_issue_failure.\n" + "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_posted (effect verification gate) before advancing via check_review_loop. On push failure, escalates via release_issue_failure.\n" + }, + "check_review_posted": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.check_review_posted", + "pr_number": "${{ context.pr_number }}", + "output_dir": "{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}", + "mode": "${{ context.review_mode }}" + }, + "on_result": [ + { + "when": "${{ result.reviews_posted }} == 'false'", + "route": "release_issue_failure" + }, + { + "when": "${{ result.reviews_posted }} == 'true'", + "route": "check_review_loop" + } + ], + "optional": true, + "skip_when_false": "inputs.open_pr", + "optional_context_refs": [ + "review_loop_count", + "review_mode" + ] }, "check_review_loop": { "tool": "run_python", diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 0d27ae3272..402ceb2999 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -1007,10 +1007,10 @@ steps: - when: "${{ result.verdict }} == approved_with_comments" route: resolve_review - when: "${{ result.verdict }} == needs_human" - route: check_review_loop + route: check_review_posted - when: "${{ result.verdict }} == approved" - route: check_review_loop - - route: check_review_loop + route: check_review_posted + - route: check_review_posted on_failure: check_repo_ci_event on_context_limit: check_repo_ci_event optional: true @@ -1024,9 +1024,10 @@ steps: the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → resolve_review; all other verdicts (including approved, needs_human) - → check_review_loop (mandatory waypoint to increment review_loop_count). - On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event - (no review to resolve). Skipped when open_pr=false. + → check_review_posted (effect verification gate) → check_review_loop (mandatory + waypoint to increment review_loop_count). On tool-level failure (MCP error, + gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped + when open_pr=false. resolve_review: tool: run_skill @@ -1105,14 +1106,30 @@ steps: remote_url: "${{ context.remote_url }}" branch: "${{ context.merge_target }}" force: 'true' - on_success: check_review_loop + on_success: check_review_posted on_failure: release_issue_failure note: > Pushes the review-fixed feature branch back to the remote after resolve_review - succeeds. Routes to check_review_loop to determine if another review-resolve - cycle is needed (bounded by review_max_retries). On push failure, escalates - via release_issue_failure. + succeeds. Routes to check_review_posted (effect verification gate) before advancing + via check_review_loop. On push failure, escalates via release_issue_failure. + check_review_posted: + tool: run_python + with: + callable: autoskillit.smoke_utils.check_review_posted + pr_number: "${{ context.pr_number }}" + output_dir: "{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}" + mode: "${{ context.review_mode }}" + on_result: + - when: "${{ result.reviews_posted }} == 'false'" + route: release_issue_failure + - when: "${{ result.reviews_posted }} == 'true'" + route: check_review_loop + optional: true + skip_when_false: "inputs.open_pr" + optional_context_refs: + - review_loop_count + - review_mode check_review_loop: tool: run_python with: diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index 78a08aae52..d01cd5bc06 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -1179,14 +1179,14 @@ }, { "when": "${{ result.verdict }} == needs_human", - "route": "check_review_loop" + "route": "check_review_posted" }, { "when": "${{ result.verdict }} == approved", - "route": "check_review_loop" + "route": "check_review_posted" }, { - "route": "check_review_loop" + "route": "check_review_posted" } ], "on_failure": "check_repo_ci_event", @@ -1197,7 +1197,7 @@ "review_loop_count", "review_mode" ], - "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → enrich_diff_context → resolve_review; all other verdicts (including approved, needs_human) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" + "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → enrich_diff_context → resolve_review; all other verdicts (including approved, needs_human) → check_review_posted (effect verification gate) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" }, "enrich_diff_context": { "tool": "run_python", @@ -1316,9 +1316,34 @@ "branch": "${{ context.merge_target }}", "force": "true" }, - "on_success": "check_review_loop", + "on_success": "check_review_posted", "on_failure": "release_issue_failure", - "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_loop to determine if another review-resolve cycle is needed (bounded by review_max_retries). On push failure, escalates via release_issue_failure.\n" + "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_posted (effect verification gate) before advancing via check_review_loop. On push failure, escalates via release_issue_failure.\n" + }, + "check_review_posted": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.check_review_posted", + "pr_number": "${{ context.pr_number }}", + "output_dir": "{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}", + "mode": "${{ context.review_mode }}" + }, + "on_result": [ + { + "when": "${{ result.reviews_posted }} == 'false'", + "route": "release_issue_failure" + }, + { + "when": "${{ result.reviews_posted }} == 'true'", + "route": "check_review_loop" + } + ], + "optional": true, + "skip_when_false": "inputs.open_pr", + "optional_context_refs": [ + "review_loop_count", + "review_mode" + ] }, "check_review_loop": { "tool": "run_python", diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index fafe6819ab..71b4810a04 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -1074,10 +1074,10 @@ steps: - when: ${{ result.verdict }} == approved_with_comments route: enrich_diff_context - when: ${{ result.verdict }} == needs_human - route: check_review_loop + route: check_review_posted - when: ${{ result.verdict }} == approved - route: check_review_loop - - route: check_review_loop + route: check_review_posted + - route: check_review_posted on_failure: check_repo_ci_event on_context_limit: check_repo_ci_event optional: true @@ -1090,9 +1090,10 @@ steps: the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → enrich_diff_context → resolve_review; all other verdicts (including approved, - needs_human) → check_review_loop (mandatory waypoint to increment review_loop_count). - On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event - (no review to resolve). Skipped when open_pr=false. + needs_human) → check_review_posted (effect verification gate) → check_review_loop + (mandatory waypoint to increment review_loop_count). On tool-level failure + (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). + Skipped when open_pr=false. ' enrich_diff_context: @@ -1191,14 +1192,30 @@ steps: remote_url: ${{ context.remote_url }} branch: ${{ context.merge_target }} force: 'true' - on_success: check_review_loop + on_success: check_review_posted on_failure: release_issue_failure note: 'Pushes the review-fixed feature branch back to the remote after resolve_review - succeeds. Routes to check_review_loop to determine if another review-resolve - cycle is needed (bounded by review_max_retries). On push failure, escalates - via release_issue_failure. + succeeds. Routes to check_review_posted (effect verification gate) before advancing + via check_review_loop. On push failure, escalates via release_issue_failure. ' + check_review_posted: + tool: run_python + with: + callable: autoskillit.smoke_utils.check_review_posted + pr_number: ${{ context.pr_number }} + output_dir: '{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}' + mode: ${{ context.review_mode }} + on_result: + - when: ${{ result.reviews_posted }} == 'false' + route: release_issue_failure + - when: ${{ result.reviews_posted }} == 'true' + route: check_review_loop + optional: true + skip_when_false: inputs.open_pr + optional_context_refs: + - review_loop_count + - review_mode check_review_loop: tool: run_python with: diff --git a/src/autoskillit/recipes/merge-prs.json b/src/autoskillit/recipes/merge-prs.json index aa86d90d80..ff25266f60 100644 --- a/src/autoskillit/recipes/merge-prs.json +++ b/src/autoskillit/recipes/merge-prs.json @@ -1520,19 +1520,38 @@ }, { "when": "${{ result.verdict }} == needs_human", - "route": "derive_batch_ci_event" + "route": "check_review_posted" }, { "when": "${{ result.verdict }} == approved", - "route": "derive_batch_ci_event" + "route": "check_review_posted" }, { "when": "true", - "route": "derive_batch_ci_event" + "route": "check_review_posted" } ], "on_failure": "resolve_review_integration", - "note": "Automated review of the integration PR (REQ-REVIEW-001). Invokes review-pr against context.batch_branch. All four verdicts have explicit routes: changes_requested → resolve_review_integration, approved_with_comments → resolve_review_integration, needs_human → derive_batch_ci_event (human reviews inline), approved → derive_batch_ci_event. On tool-level failure, routes to resolve_review_integration (matches implementation.yaml pattern). needs_human is explicitly routed to satisfy the unrouted-verdict-value semantic rule.\n" + "note": "Automated review of the integration PR (REQ-REVIEW-001). Invokes review-pr against context.batch_branch. All four verdicts have explicit routes: changes_requested → resolve_review_integration, approved_with_comments → resolve_review_integration, needs_human → check_review_posted (effect verification gate) → derive_batch_ci_event, approved → check_review_posted → derive_batch_ci_event. On tool-level failure, routes to resolve_review_integration. needs_human is explicitly routed to satisfy the unrouted-verdict-value semantic rule.\n" + }, + "check_review_posted": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.check_review_posted", + "pr_number": "${{ context.review_pr_number }}", + "output_dir": "{{AUTOSKILLIT_TEMP}}/review-pr", + "mode": "github" + }, + "on_result": [ + { + "when": "${{ result.reviews_posted }} == 'false'", + "route": "register_clone_failure" + }, + { + "when": "${{ result.reviews_posted }} == 'true'", + "route": "derive_batch_ci_event" + } + ] }, "resolve_review_integration": { "tool": "run_skill", diff --git a/src/autoskillit/recipes/merge-prs.yaml b/src/autoskillit/recipes/merge-prs.yaml index ee1f1f2f18..1cb046ba70 100644 --- a/src/autoskillit/recipes/merge-prs.yaml +++ b/src/autoskillit/recipes/merge-prs.yaml @@ -1488,21 +1488,33 @@ steps: - when: ${{ result.verdict }} == approved_with_comments route: resolve_review_integration - when: ${{ result.verdict }} == needs_human - route: derive_batch_ci_event + route: check_review_posted - when: ${{ result.verdict }} == approved - route: derive_batch_ci_event + route: check_review_posted - when: 'true' - route: derive_batch_ci_event + route: check_review_posted on_failure: resolve_review_integration note: 'Automated review of the integration PR (REQ-REVIEW-001). Invokes review-pr against context.batch_branch. All four verdicts have explicit routes: changes_requested → resolve_review_integration, approved_with_comments → resolve_review_integration, - needs_human → derive_batch_ci_event (human reviews inline), approved → derive_batch_ci_event. - On tool-level failure, routes to resolve_review_integration (matches implementation.yaml - pattern). needs_human is explicitly routed to satisfy the unrouted-verdict-value - semantic rule. + needs_human → check_review_posted (effect verification gate) → derive_batch_ci_event, + approved → check_review_posted → derive_batch_ci_event. On tool-level failure, + routes to resolve_review_integration. needs_human is explicitly routed to satisfy + the unrouted-verdict-value semantic rule. ' + check_review_posted: + tool: run_python + with: + callable: autoskillit.smoke_utils.check_review_posted + pr_number: ${{ context.review_pr_number }} + output_dir: '{{AUTOSKILLIT_TEMP}}/review-pr' + mode: github + on_result: + - when: ${{ result.reviews_posted }} == 'false' + route: register_clone_failure + - when: ${{ result.reviews_posted }} == 'true' + route: derive_batch_ci_event resolve_review_integration: tool: run_skill model: '' diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index 4635940ed3..a166a0861d 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -1337,14 +1337,14 @@ }, { "when": "${{ result.verdict }} == needs_human", - "route": "check_review_loop" + "route": "check_review_posted" }, { "when": "${{ result.verdict }} == approved", - "route": "check_review_loop" + "route": "check_review_posted" }, { - "route": "check_review_loop" + "route": "check_review_posted" } ], "on_failure": "check_repo_ci_event", @@ -1355,7 +1355,7 @@ "review_loop_count", "review_mode" ], - "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → resolve_review; all other verdicts (including approved, needs_human) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" + "note": "Runs after compose_pr creates the PR. Invokes the review-pr skill as a headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments → resolve_review; all other verdicts (including approved, needs_human) → check_review_posted (effect verification gate) → check_review_loop (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP error, gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false.\n" }, "resolve_review": { "tool": "run_skill", @@ -1457,9 +1457,34 @@ "branch": "${{ context.merge_target }}", "force": "true" }, - "on_success": "check_review_loop", + "on_success": "check_review_posted", "on_failure": "release_issue_failure", - "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_loop to determine if another review-resolve cycle is needed (bounded by review_max_retries). On push failure, escalates via release_issue_failure.\n" + "note": "Pushes the review-fixed feature branch back to the remote after resolve_review succeeds. Routes to check_review_posted (effect verification gate) before advancing via check_review_loop. On push failure, escalates via release_issue_failure.\n" + }, + "check_review_posted": { + "tool": "run_python", + "with": { + "callable": "autoskillit.smoke_utils.check_review_posted", + "pr_number": "${{ context.pr_number }}", + "output_dir": "{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}", + "mode": "${{ context.review_mode }}" + }, + "on_result": [ + { + "when": "${{ result.reviews_posted }} == 'false'", + "route": "release_issue_failure" + }, + { + "when": "${{ result.reviews_posted }} == 'true'", + "route": "check_review_loop" + } + ], + "optional": true, + "skip_when_false": "inputs.open_pr", + "optional_context_refs": [ + "review_loop_count", + "review_mode" + ] }, "check_review_loop": { "tool": "run_python", diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 843e8e0a0e..6ff354e56b 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -1202,10 +1202,10 @@ steps: - when: ${{ result.verdict }} == approved_with_comments route: resolve_review - when: ${{ result.verdict }} == needs_human - route: check_review_loop + route: check_review_posted - when: ${{ result.verdict }} == approved - route: check_review_loop - - route: check_review_loop + route: check_review_posted + - route: check_review_posted on_failure: check_repo_ci_event on_context_limit: check_repo_ci_event optional: true @@ -1217,9 +1217,10 @@ steps: headless session to leave inline review comments on the PR. The skill finds the open PR by feature branch name (context.merge_target). Captures verdict as review_verdict and routes via on_result: changes_requested/approved_with_comments - → resolve_review; all other verdicts (including approved, needs_human) → check_review_loop - (mandatory waypoint to increment review_loop_count). On tool-level failure (MCP - error, gh unavailable), routes to check_repo_ci_event (no review to resolve). + → resolve_review; all other verdicts (including approved, needs_human) → + check_review_posted (effect verification gate) → check_review_loop (mandatory + waypoint to increment review_loop_count). On tool-level failure (MCP error, + gh unavailable), routes to check_repo_ci_event (no review to resolve). Skipped when open_pr=false. ' @@ -1301,14 +1302,30 @@ steps: remote_url: ${{ context.remote_url }} branch: ${{ context.merge_target }} force: 'true' - on_success: check_review_loop + on_success: check_review_posted on_failure: release_issue_failure note: 'Pushes the review-fixed feature branch back to the remote after resolve_review - succeeds. Routes to check_review_loop to determine if another review-resolve - cycle is needed (bounded by review_max_retries). On push failure, escalates - via release_issue_failure. + succeeds. Routes to check_review_posted (effect verification gate) before advancing + via check_review_loop. On push failure, escalates via release_issue_failure. ' + check_review_posted: + tool: run_python + with: + callable: autoskillit.smoke_utils.check_review_posted + pr_number: ${{ context.pr_number }} + output_dir: '{{AUTOSKILLIT_TEMP}}/review-pr/iter_${{ context.review_loop_count }}' + mode: ${{ context.review_mode }} + on_result: + - when: ${{ result.reviews_posted }} == 'false' + route: release_issue_failure + - when: ${{ result.reviews_posted }} == 'true' + route: check_review_loop + optional: true + skip_when_false: inputs.open_pr + optional_context_refs: + - review_loop_count + - review_mode check_review_loop: tool: run_python with: diff --git a/src/autoskillit/skills_extended/review-pr/SKILL.md b/src/autoskillit/skills_extended/review-pr/SKILL.md index aa6970eafd..af0c184b66 100644 --- a/src/autoskillit/skills_extended/review-pr/SKILL.md +++ b/src/autoskillit/skills_extended/review-pr/SKILL.md @@ -623,6 +623,12 @@ jq -n \ '{body: $body, event: $event, comments: $comments}' | \ gh api /repos/{owner}/{repo}/pulls/{pr_number}/reviews \ --method POST --input - + +# Write receipt file on success — checked by check_review_posted gate in the recipe +if [ $? -eq 0 ]; then + printf '{"posted":true}' \ + > "${REVIEW_OUTPUT_DIR}batch_review_response_${pr_number}.json" +fi ``` Event mapping: diff --git a/src/autoskillit/smoke_utils/__init__.py b/src/autoskillit/smoke_utils/__init__.py index b0286f0eb7..8f4ea886e5 100644 --- a/src/autoskillit/smoke_utils/__init__.py +++ b/src/autoskillit/smoke_utils/__init__.py @@ -30,6 +30,7 @@ check_loop_iteration, check_loop_with_progress, check_review_loop, + check_review_posted, enrich_diff_context, init_counter, pre_iteration_cleanup, @@ -50,6 +51,7 @@ "check_loop_iteration", "check_loop_with_progress", "check_review_loop", + "check_review_posted", "close_issue_already_done", "consolidate_health_reports", "compile_eval_scorecard", diff --git a/src/autoskillit/smoke_utils/_review.py b/src/autoskillit/smoke_utils/_review.py index 93b15204ac..c82fc8458a 100644 --- a/src/autoskillit/smoke_utils/_review.py +++ b/src/autoskillit/smoke_utils/_review.py @@ -201,6 +201,30 @@ def check_review_loop( } +def check_review_posted( + pr_number: int, + output_dir: str, + mode: str, +) -> dict[str, str]: + """Verify that batch_review_response_{pr_number}.json exists in output_dir. + + In github mode, returns reviews_posted="false" with sentinel="no_reviews_posted" + when the receipt file is absent, indicating the review POST did not complete. + In local mode, always returns reviews_posted="true" (no API calls made). + + output_dir must be an absolute path (enforced by is_absolute() guard required by + tests/arch/test_run_python_path_resolution.py for output_dir parameters). + """ + if not Path(output_dir).is_absolute(): + raise ValueError(f"output_dir must be an absolute path, got: {output_dir!r}") + if mode == "local": + return {"reviews_posted": "true", "sentinel": ""} + receipt = Path(output_dir) / f"batch_review_response_{pr_number}.json" + if receipt.exists(): + return {"reviews_posted": "true", "sentinel": ""} + return {"reviews_posted": "false", "sentinel": "no_reviews_posted"} + + def check_loop_iteration( current_iteration: str = "", max_iterations: str = "2", diff --git a/tests/conftest.py b/tests/conftest.py index e8b92391e1..ba67297d20 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -292,6 +292,23 @@ def _clear_features_env(monkeypatch): monkeypatch.delenv(key, raising=False) +@pytest.fixture(autouse=True) +def _clear_run_skill_env(monkeypatch): + """Clear ALL AUTOSKILLIT_RUN_SKILL__* env vars before every test. + + Dynaconf reads env vars with prefix AUTOSKILLIT_ and injects them into the + config dict at the highest priority layer. RunSkillConfig env vars + (AUTOSKILLIT_RUN_SKILL__MAX_SUPPRESSION_SECONDS, etc.) override YAML/dataclass + defaults when set by the pipeline harness, breaking test isolation. + + Uses prefix-based scanning so new run_skill env vars are automatically covered + without needing individual fixture additions. + """ + for key in list(os.environ): + if key.startswith("AUTOSKILLIT_RUN_SKILL__"): + monkeypatch.delenv(key, raising=False) + + @pytest.fixture(autouse=True) def _clear_test_filter_env(monkeypatch): """Clear AUTOSKILLIT_TEST_FILTER and related env vars before every test. diff --git a/tests/recipe/AGENTS.md b/tests/recipe/AGENTS.md index 89d3a31ee7..fc573026cd 100644 --- a/tests/recipe/AGENTS.md +++ b/tests/recipe/AGENTS.md @@ -8,6 +8,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. |------|---------| | `__init__.py` | empty | | `conftest.py` | Shared fixtures for tests/recipe/ | +| `test_analysis_bfs.py` | Tests for bfs_reachable_without_barrier API — frozenset barrier support | | `test_analysis_public_api.py` | Tests for the recipe analysis public API surface | | `test_admission_truth_telling.py` | Admission truth-telling regression tests — pre-prune resolver, effective-backend awareness, admission↔dispatch agreement contract | | `test_anti_pattern_guards.py` | Guards for anti-patterns in recipe definitions | @@ -179,6 +180,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. | `test_rules_project_local_override.py` | Tests for project_local_override semantic validation rule | | `test_rules_reachability.py` | Tests for reachability semantic validation rule | | `test_rules_remediation.py` | Tests for audit-impl-remediation-route semantic validation rule | +| `test_rules_review_effect_waypoint.py` | Tests for review-effect-verification-waypoint semantic rule | | `test_rules_review_loop_waypoint.py` | Tests for review-loop-waypoint-guard semantic rule | | `test_rules_route_gate.py` | Tests for route-gate-shared-stop semantic validation rule | | `test_rules_loop_counter.py` | Tests for loop-counter-cross-path-sharing, loop-guard-before-verify, and loop-counter-not-reset-on-outer-cycle semantic rules | diff --git a/tests/recipe/test_analysis_bfs.py b/tests/recipe/test_analysis_bfs.py new file mode 100644 index 0000000000..39106270b1 --- /dev/null +++ b/tests/recipe/test_analysis_bfs.py @@ -0,0 +1,62 @@ +"""Tests for bfs_reachable_without_barrier API — frozenset barrier support (T-B-RI1).""" + +from __future__ import annotations + +import pytest + +from autoskillit.recipe._analysis_bfs import bfs_reachable_without_barrier +from autoskillit.recipe.io import builtin_recipes_dir, load_recipe + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + + +def test_bfs_accepts_frozenset_barrier(): + """T-B-RI1: bfs_reachable_without_barrier accepts a frozenset barrier.""" + recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + result = bfs_reachable_without_barrier( + recipe, + start="review_pr", + barrier=frozenset({"check_review_loop", "check_review_posted"}), + ) + assert isinstance(result, set) + + +def test_bfs_accepts_string_barrier_unchanged(): + """T-B-RI1: bfs_reachable_without_barrier still accepts a plain string barrier.""" + recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + result = bfs_reachable_without_barrier( + recipe, + start="review_pr", + barrier="check_review_loop", + ) + assert isinstance(result, set) + + +def test_bfs_frozenset_barrier_blocks_all_named_steps(): + """With frozenset barrier, none of the listed steps are expanded.""" + recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + barriers = frozenset({"check_review_posted", "check_review_loop"}) + result = bfs_reachable_without_barrier( + recipe, + start="review_pr", + barrier=barriers, + ) + # check_repo_ci_event is only reachable through check_review_loop — must not appear + # because check_review_loop is a barrier and is not expanded + assert "check_repo_ci_event" not in result + + +def test_bfs_single_barrier_string_matches_frozenset_singleton(): + """String barrier and singleton frozenset produce same reachability result.""" + recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + result_str = bfs_reachable_without_barrier( + recipe, + start="review_pr", + barrier="check_review_loop", + ) + result_set = bfs_reachable_without_barrier( + recipe, + start="review_pr", + barrier=frozenset({"check_review_loop"}), + ) + assert result_str == result_set diff --git a/tests/recipe/test_bundled_recipes_review_pr.py b/tests/recipe/test_bundled_recipes_review_pr.py index 94f8da0103..141410ce59 100644 --- a/tests/recipe/test_bundled_recipes_review_pr.py +++ b/tests/recipe/test_bundled_recipes_review_pr.py @@ -66,19 +66,20 @@ def test_review_pr_skipped_when_open_pr_false(self, recipe: object) -> None: step = recipe.steps["review_pr"] # type: ignore[attr-defined] assert step.skip_when_false == "inputs.open_pr" - def test_review_pr_catch_all_routes_to_check_review_loop(self, recipe: object) -> None: - """T_RP4: catch-all routes through check_review_loop (mandatory waypoint). + def test_review_pr_catch_all_routes_to_check_review_posted(self, recipe: object) -> None: + """T_RP4: catch-all routes through check_review_posted (effect verification gate). All verdicts (approved, needs_human, any future verdict) must pass through - check_review_loop to ensure review_loop_count is always incremented. + check_review_posted to verify the review was actually posted before advancing + the loop counter via check_review_loop. """ step = recipe.steps["review_pr"] # type: ignore[attr-defined] assert step.on_result is not None default_conditions = [ c for c in step.on_result.conditions if c.when is None or c.when == "true" ] - assert any(c.route == "check_review_loop" for c in default_conditions), ( - "catch-all must route to check_review_loop, not directly to check_repo_ci_event" + assert any(c.route == "check_review_posted" for c in default_conditions), ( + "catch-all must route to check_review_posted, not directly to check_review_loop" ) def test_review_pr_captures_verdict(self, recipe: object) -> None: @@ -153,9 +154,9 @@ def test_pre_review_rebase_routes_to_re_push_review(self, recipe: object) -> Non assert "re_push_review" in clean_routes assert step.on_failure == "resolve_pre_review_conflicts" - def test_re_push_review_routes_to_check_review_loop(self, recipe: object) -> None: - """T_RP8: re_push_review routes to check_review_loop (bounded retry gate).""" - assert recipe.steps["re_push_review"].on_success == "check_review_loop" # type: ignore[attr-defined] + def test_re_push_review_routes_to_check_review_posted(self, recipe: object) -> None: + """T_RP8: re_push_review routes to check_review_posted (effect verification gate).""" + assert recipe.steps["re_push_review"].on_success == "check_review_posted" # type: ignore[attr-defined] def test_ci_watch_present(self, recipe: object) -> None: """T_RP9: ci_watch step present in all four recipes.""" @@ -447,3 +448,54 @@ def test_resolve_pre_review_conflicts_uses_merge_conflicts_skill(self, recipe: o assert step.tool == "run_skill" cmd = step.with_args.get("skill_command", "") assert "resolve-merge-conflicts" in cmd + + +# --------------------------------------------------------------------------- +# T-B-RP1: check_review_posted false-case routes to failure (not CI) +# --------------------------------------------------------------------------- + + +def test_check_review_posted_false_routes_to_failure_not_ci() -> None: + """T-B-RP1: implementation.yaml check_review_posted false-case routes to hard failure.""" + recipe = load_recipe(builtin_recipes_dir() / "implementation.yaml") + crp_step = recipe.steps.get("check_review_posted") + assert crp_step is not None, "check_review_posted step missing from implementation.yaml" + false_cond = next( + c + for c in crp_step.on_result.conditions + if "false" in str(c.when) or "no_reviews_posted" in str(c.when) + ) + assert false_cond.route not in {"check_repo_ci_event", "ci_watch", "enqueue_to_queue"} + + +# --------------------------------------------------------------------------- +# T-B-MP1: merge-prs.yaml check_review_posted wiring +# --------------------------------------------------------------------------- + + +def test_merge_prs_check_review_posted_uses_review_pr_number() -> None: + """T-B-MP1a: merge-prs check_review_posted must use context.review_pr_number.""" + recipe = load_recipe(builtin_recipes_dir() / "merge-prs.yaml") + crp_step = recipe.steps.get("check_review_posted") + assert crp_step is not None, "check_review_posted step missing from merge-prs.yaml" + args_str = str(crp_step.with_args) + assert "review_pr_number" in args_str + + +def test_merge_prs_check_review_posted_mode_is_hardcoded_github() -> None: + """T-B-MP1b: merge-prs check_review_posted mode must be hardcoded 'github'.""" + recipe = load_recipe(builtin_recipes_dir() / "merge-prs.yaml") + crp_step = recipe.steps.get("check_review_posted") + assert crp_step is not None + args_str = str(crp_step.with_args) + assert "github" in args_str + assert "review_mode" not in args_str + + +def test_merge_prs_check_review_posted_true_routes_to_derive_batch_ci_event() -> None: + """T-B-MP1c: merge-prs check_review_posted true-case routes to derive_batch_ci_event.""" + recipe = load_recipe(builtin_recipes_dir() / "merge-prs.yaml") + crp_step = recipe.steps.get("check_review_posted") + assert crp_step is not None + true_cond = next(c for c in crp_step.on_result.conditions if "true" in str(c.when)) + assert true_cond.route == "derive_batch_ci_event" diff --git a/tests/recipe/test_implementation_groups_review_loop.py b/tests/recipe/test_implementation_groups_review_loop.py index 0bc0b6667b..05e8de53e7 100644 --- a/tests/recipe/test_implementation_groups_review_loop.py +++ b/tests/recipe/test_implementation_groups_review_loop.py @@ -36,11 +36,10 @@ def test_pre_review_rebase_routes_to_re_push_review(recipe) -> None: assert step.on_failure == "resolve_pre_review_conflicts" -def test_re_push_review_routes_to_check_review_loop(recipe) -> None: - """re_push_review on_success must route to check_review_loop in implementation-groups - recipe.""" +def test_re_push_review_routes_to_check_review_posted(recipe) -> None: + """re_push_review on_success must route to check_review_posted (effect verification gate).""" step = recipe.steps["re_push_review"] - assert step.on_success == "check_review_loop" + assert step.on_success == "check_review_posted" # T_IG_LOOP3 diff --git a/tests/recipe/test_rules_review_effect_waypoint.py b/tests/recipe/test_rules_review_effect_waypoint.py new file mode 100644 index 0000000000..db9f7e1864 --- /dev/null +++ b/tests/recipe/test_rules_review_effect_waypoint.py @@ -0,0 +1,163 @@ +"""Tests for the review-effect-verification-waypoint semantic rule. + +The rule fires when any advance gate (check_review_loop or derive_batch_ci_event) +is BFS-reachable from a review-pr skill step without crossing check_review_posted. +""" + +from __future__ import annotations + +import pytest + +from autoskillit.recipe.io import builtin_recipes_dir, load_recipe +from autoskillit.recipe.registry import _RULE_REGISTRY, run_semantic_rules +from autoskillit.recipe.schema import Recipe, RecipeStep, StepResultCondition, StepResultRoute + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + +RULE_ID = "review-effect-verification-waypoint" +REVIEW_LOOP_RECIPES = ["implementation", "remediation", "implementation-groups"] + + +def _make_recipe(steps: dict[str, RecipeStep]) -> Recipe: + return Recipe(name="test", description="test", steps=steps, kitchen_rules=["test"]) + + +def test_effect_waypoint_rule_is_registered(): + """The rule must be registered in the semantic rule registry.""" + rule_ids = {r.name for r in _RULE_REGISTRY} + assert RULE_ID in rule_ids, ( + f"{RULE_ID} not found in rule registry. Registered rules: {sorted(rule_ids)}" + ) + + +@pytest.mark.parametrize("recipe_name", REVIEW_LOOP_RECIPES) +def test_effect_waypoint_rule_silent_on_fixed_recipes(recipe_name): + """After Part A fix, the rule must not fire on review-loop recipes.""" + recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) == 0, ( + f"[{recipe_name}] {RULE_ID} fired after fix: {rule_violations}" + ) + + +def test_effect_waypoint_rule_silent_on_merge_prs(): + """After Part A fix, the rule must not fire on merge-prs.yaml.""" + recipe = load_recipe(builtin_recipes_dir() / "merge-prs.yaml") + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) == 0, f"[merge-prs] {RULE_ID} fired after fix: {rule_violations}" + + +def test_effect_waypoint_rule_fires_when_check_review_loop_reachable_without_gate(): + """T-B-RULE1: Rule fires when check_review_loop reachable without check_review_posted.""" + recipe = _make_recipe( + { + "review_pr": RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:review-pr main feature"}, + on_result=StepResultRoute( + conditions=[StepResultCondition(when="true", route="check_review_loop")] + ), + ), + "check_review_loop": RecipeStep( + tool="run_python", + on_result=StepResultRoute( + conditions=[StepResultCondition(when="true", route="check_repo_ci_event")] + ), + ), + "check_repo_ci_event": RecipeStep(), + } + ) + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) >= 1 + assert all(v.severity.name == "ERROR" for v in rule_violations) + assert any("check_review_loop" in v.message for v in rule_violations) + + +def test_effect_waypoint_rule_fires_when_derive_batch_ci_event_reachable_without_gate(): + """T-B-RULE1: Rule fires when derive_batch_ci_event reachable without check_review_posted.""" + recipe = _make_recipe( + { + "review_pr_integration": RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:review-pr main feature"}, + on_result=StepResultRoute( + conditions=[StepResultCondition(when="true", route="derive_batch_ci_event")] + ), + ), + "derive_batch_ci_event": RecipeStep(tool="check_repo_merge_state"), + } + ) + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) >= 1 + assert all(v.severity.name == "ERROR" for v in rule_violations) + assert any("derive_batch_ci_event" in v.message for v in rule_violations) + + +def test_effect_waypoint_rule_silent_when_gate_present(): + """T-B-RULE1: Rule silent when check_review_posted is barrier before check_review_loop.""" + recipe = _make_recipe( + { + "review_pr": RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:review-pr main feature"}, + on_result=StepResultRoute( + conditions=[StepResultCondition(when="true", route="check_review_posted")] + ), + ), + "check_review_posted": RecipeStep( + tool="run_python", + on_result=StepResultRoute( + conditions=[ + StepResultCondition( + when="${{ result.reviews_posted }} == 'true'", + route="check_review_loop", + ), + StepResultCondition(when=None, route="failure_step"), + ] + ), + ), + "check_review_loop": RecipeStep(on_success="done"), + "failure_step": RecipeStep(action="stop", message="fail"), + "done": RecipeStep(action="stop", message="done"), + } + ) + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) == 0 + + +def test_effect_waypoint_rule_skips_recipes_without_review_pr_skill_steps(): + """Recipes without run_skill steps dispatching review-pr are not subject to this rule.""" + recipe = _make_recipe( + { + "merge_worktree": RecipeStep(tool="merge_worktree"), + "done": RecipeStep(action="stop", message="done"), + } + ) + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) == 0 + + +def test_effect_waypoint_rule_covers_aliased_step_names(): + """Rule fires for review_pr_integration (aliased step id) dispatching review-pr skill.""" + recipe = _make_recipe( + { + "review_pr_integration": RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:review-pr batch main"}, + on_result=StepResultRoute( + conditions=[StepResultCondition(when="true", route="check_review_loop")] + ), + ), + "check_review_loop": RecipeStep(on_success="done"), + "done": RecipeStep(action="stop", message="done"), + } + ) + violations = run_semantic_rules(recipe) + rule_violations = [v for v in violations if v.rule == RULE_ID] + assert len(rule_violations) >= 1 diff --git a/tests/test_smoke_utils.py b/tests/test_smoke_utils.py index d8ecc3c7ec..b85f54ad3d 100644 --- a/tests/test_smoke_utils.py +++ b/tests/test_smoke_utils.py @@ -3308,3 +3308,63 @@ def test_gate_backend_write_zero() -> None: def test_gate_backend_write_empty() -> None: """Returns {"backend_capable": "false"} for empty string.""" assert gate_backend_write("") == {"backend_capable": "false"} + + +# --------------------------------------------------------------------------- +# T_CRP1–T_CRP5: check_review_posted +# --------------------------------------------------------------------------- + + +# T_CRP1 +def test_check_review_posted_no_receipt_returns_false(tmp_path): + """No receipt file → reviews_posted="false" with sentinel.""" + from autoskillit.smoke_utils import check_review_posted + + result = check_review_posted(pr_number=42, output_dir=str(tmp_path.resolve()), mode="github") + assert result["reviews_posted"] == "false" + assert result["sentinel"] == "no_reviews_posted" + + +# T_CRP2 +def test_check_review_posted_with_receipt_returns_true(tmp_path): + """Receipt file present → reviews_posted="true" with no sentinel.""" + from autoskillit.smoke_utils import check_review_posted + + receipt = tmp_path / "batch_review_response_42.json" + receipt.write_text('{"id": 1}') + result = check_review_posted(pr_number=42, output_dir=str(tmp_path.resolve()), mode="github") + assert result["reviews_posted"] == "true" + assert result.get("sentinel", "") == "" + + +# T_CRP3 +def test_check_review_posted_local_mode_always_true(tmp_path): + """local mode always returns reviews_posted="true" regardless of receipt file.""" + from autoskillit.smoke_utils import check_review_posted + + result = check_review_posted(pr_number=42, output_dir=str(tmp_path.resolve()), mode="local") + assert result["reviews_posted"] == "true" + + +# T_CRP4 +def test_check_review_posted_has_no_subprocess_calls(tmp_path): + """check_review_posted must not invoke any subprocess.""" + import subprocess + from unittest.mock import patch + + from autoskillit.smoke_utils import check_review_posted + + with patch.object( + subprocess, "run", side_effect=AssertionError("unexpected subprocess") + ) as mock_run: + check_review_posted(pr_number=1, output_dir=str(tmp_path.resolve()), mode="github") + mock_run.assert_not_called() + + +# T_CRP5 +def test_check_review_posted_in_smoke_utils_all(): + """check_review_posted must be exported from smoke_utils.__all__.""" + import autoskillit.smoke_utils as sm + + assert "check_review_posted" in sm.__all__ + assert callable(sm.check_review_posted) From 5be30ae5ca6361feaf5d3ea4d6a8e5dd9919ef08 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 15:46:30 -0700 Subject: [PATCH 10/15] fix: correct callable contract outputs, recipe guards, and test updates - Add missing type: str to check_review_posted callable contract outputs - Remove erroneous skip_when_false: inputs.open_pr from check_review_posted in implementation, implementation-groups, and remediation recipes - Add on_failure to check_review_posted in all 4 recipes - Fix /autoskillit:review-pr to /review-pr prefix in rules_graph_review.py - Update test_implementation and test_remediation to assert check_review_posted - Update _LEGACY_JSON_WRITES line numbers (317->341, 409->433) - Add check_review_posted to smoke_utils exports completeness test - Update diagram hashes for all 4 modified recipes Co-Authored-By: Claude Sonnet 4.6 --- src/autoskillit/recipe/rules/graph/rules_graph_review.py | 4 +--- src/autoskillit/recipe/skill_contracts.yaml | 2 ++ src/autoskillit/recipes/diagrams/implementation-groups.md | 2 +- src/autoskillit/recipes/diagrams/implementation.md | 2 +- src/autoskillit/recipes/diagrams/merge-prs.md | 2 +- src/autoskillit/recipes/diagrams/remediation.md | 2 +- src/autoskillit/recipes/implementation-groups.json | 2 +- src/autoskillit/recipes/implementation-groups.yaml | 2 +- src/autoskillit/recipes/implementation.json | 2 +- src/autoskillit/recipes/implementation.yaml | 2 +- src/autoskillit/recipes/merge-prs.json | 3 ++- src/autoskillit/recipes/merge-prs.yaml | 1 + src/autoskillit/recipes/remediation.json | 2 +- src/autoskillit/recipes/remediation.yaml | 2 +- tests/infra/test_schema_version_convention.py | 4 ++-- tests/recipe/test_implementation.py | 6 +++--- tests/recipe/test_remediation_recipe.py | 6 +++--- tests/test_smoke_utils.py | 3 ++- 18 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/autoskillit/recipe/rules/graph/rules_graph_review.py b/src/autoskillit/recipe/rules/graph/rules_graph_review.py index ae455fcc6a..03f95a026e 100644 --- a/src/autoskillit/recipe/rules/graph/rules_graph_review.py +++ b/src/autoskillit/recipe/rules/graph/rules_graph_review.py @@ -182,9 +182,7 @@ def _review_effect_verification_waypoint(ctx: ValidationContext) -> list[RuleFin for step_id, step in ctx.recipe.steps.items() if ( step.tool == "run_skill" - and str((step.with_args or {}).get("skill_command", "")).startswith( - "/autoskillit:review-pr" - ) + and str((step.with_args or {}).get("skill_command", "")).startswith("/review-pr") ) ] if not effect_steps: diff --git a/src/autoskillit/recipe/skill_contracts.yaml b/src/autoskillit/recipe/skill_contracts.yaml index 76a4de7608..d2a656988c 100644 --- a/src/autoskillit/recipe/skill_contracts.yaml +++ b/src/autoskillit/recipe/skill_contracts.yaml @@ -2594,10 +2594,12 @@ callable_contracts: - github outputs: - name: reviews_posted + type: str allowed_values: - "true" - "false" - name: sentinel + type: str allowed_values: - no_reviews_posted - "" diff --git a/src/autoskillit/recipes/diagrams/implementation-groups.md b/src/autoskillit/recipes/diagrams/implementation-groups.md index 218619bcd3..252b1af81d 100644 --- a/src/autoskillit/recipes/diagrams/implementation-groups.md +++ b/src/autoskillit/recipes/diagrams/implementation-groups.md @@ -1,4 +1,4 @@ - + ## implementation-groups Group-based implementation with per-group plan/implement/test cycles and PR gates. diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index ff724ad9f6..8782d8e156 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 ba233175e3..e3450f7132 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 9e8b2e231c..4e3550d62e 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation Investigate, rectify, implement, and merge a bug fix with CI and PR gates. diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index c41ad5fdf2..7e654debab 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -1265,8 +1265,8 @@ "route": "check_review_loop" } ], + "on_failure": "release_issue_failure", "optional": true, - "skip_when_false": "inputs.open_pr", "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 402ceb2999..8741b5d923 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -1125,8 +1125,8 @@ steps: route: release_issue_failure - when: "${{ result.reviews_posted }} == 'true'" route: check_review_loop + on_failure: release_issue_failure optional: true - skip_when_false: "inputs.open_pr" optional_context_refs: - review_loop_count - review_mode diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index d01cd5bc06..20858afcab 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -1338,8 +1338,8 @@ "route": "check_review_loop" } ], + "on_failure": "release_issue_failure", "optional": true, - "skip_when_false": "inputs.open_pr", "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index 71b4810a04..160eb1cf81 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -1211,8 +1211,8 @@ steps: route: release_issue_failure - when: ${{ result.reviews_posted }} == 'true' route: check_review_loop + on_failure: release_issue_failure optional: true - skip_when_false: inputs.open_pr optional_context_refs: - review_loop_count - review_mode diff --git a/src/autoskillit/recipes/merge-prs.json b/src/autoskillit/recipes/merge-prs.json index ff25266f60..2e4024cc8d 100644 --- a/src/autoskillit/recipes/merge-prs.json +++ b/src/autoskillit/recipes/merge-prs.json @@ -1551,7 +1551,8 @@ "when": "${{ result.reviews_posted }} == 'true'", "route": "derive_batch_ci_event" } - ] + ], + "on_failure": "register_clone_failure" }, "resolve_review_integration": { "tool": "run_skill", diff --git a/src/autoskillit/recipes/merge-prs.yaml b/src/autoskillit/recipes/merge-prs.yaml index 1cb046ba70..25aa5a8b1b 100644 --- a/src/autoskillit/recipes/merge-prs.yaml +++ b/src/autoskillit/recipes/merge-prs.yaml @@ -1515,6 +1515,7 @@ steps: route: register_clone_failure - when: ${{ result.reviews_posted }} == 'true' route: derive_batch_ci_event + on_failure: register_clone_failure resolve_review_integration: tool: run_skill model: '' diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index a166a0861d..af7aff1b43 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -1479,8 +1479,8 @@ "route": "check_review_loop" } ], + "on_failure": "release_issue_failure", "optional": true, - "skip_when_false": "inputs.open_pr", "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 6ff354e56b..f613d016ea 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -1321,8 +1321,8 @@ steps: route: release_issue_failure - when: ${{ result.reviews_posted }} == 'true' route: check_review_loop + on_failure: release_issue_failure optional: true - skip_when_false: inputs.open_pr optional_context_refs: - review_loop_count - review_mode diff --git a/tests/infra/test_schema_version_convention.py b/tests/infra/test_schema_version_convention.py index 5dff5069f1..2ba4da08df 100644 --- a/tests/infra/test_schema_version_convention.py +++ b/tests/infra/test_schema_version_convention.py @@ -152,8 +152,8 @@ def _is_yaml_dump(node: ast.expr) -> bool: ("src/autoskillit/smoke_utils/_review.py", 101), ("src/autoskillit/smoke_utils/_review.py", 102), ("src/autoskillit/smoke_utils/_review.py", 120), - ("src/autoskillit/smoke_utils/_review.py", 317), - ("src/autoskillit/smoke_utils/_review.py", 409), + ("src/autoskillit/smoke_utils/_review.py", 341), + ("src/autoskillit/smoke_utils/_review.py", 433), # smoke_utils/_git.py — partitions, merge queue data # Line 110 is a list-payload write site (dual membership: also in list_sites # in test_allowlist_includes_list_payloads_as_documented). The AST scanner catches diff --git a/tests/recipe/test_implementation.py b/tests/recipe/test_implementation.py index a817cfca00..e6deea73bb 100644 --- a/tests/recipe/test_implementation.py +++ b/tests/recipe/test_implementation.py @@ -117,10 +117,10 @@ def test_pre_review_rebase_routes_to_re_push_review(recipe) -> None: assert step.on_failure == "resolve_pre_review_conflicts" -def test_re_push_review_routes_to_check_review_loop(recipe) -> None: - """re_push_review on_success must route to check_review_loop, not ci_watch directly.""" +def test_re_push_review_routes_to_check_review_posted(recipe) -> None: + """re_push_review on_success must route to check_review_posted (effect verification gate).""" step = recipe.steps["re_push_review"] - assert step.on_success == "check_review_loop" + assert step.on_success == "check_review_posted" # T_IP_LOOP8 diff --git a/tests/recipe/test_remediation_recipe.py b/tests/recipe/test_remediation_recipe.py index b7213c5712..717c81663c 100644 --- a/tests/recipe/test_remediation_recipe.py +++ b/tests/recipe/test_remediation_recipe.py @@ -53,10 +53,10 @@ def test_pre_review_rebase_routes_to_re_push_review(recipe) -> None: assert step.on_failure == "resolve_pre_review_conflicts" -def test_re_push_review_routes_to_check_review_loop(recipe) -> None: - """re_push_review on_success must route to check_review_loop in remediation recipe.""" +def test_re_push_review_routes_to_check_review_posted(recipe) -> None: + """re_push_review on_success must route to check_review_posted (effect verification gate).""" step = recipe.steps["re_push_review"] - assert step.on_success == "check_review_loop" + assert step.on_success == "check_review_posted" def test_pre_remediation_merge_routes_path_validation_to_remediate(recipe) -> None: diff --git a/tests/test_smoke_utils.py b/tests/test_smoke_utils.py index b85f54ad3d..552d013c4f 100644 --- a/tests/test_smoke_utils.py +++ b/tests/test_smoke_utils.py @@ -2656,7 +2656,7 @@ def test_diagnose_merge_gate_rejects_empty_output_dir() -> None: def test_smoke_utils_all_exports_complete() -> None: - """smoke_utils.__all__ must list all 28 public names.""" + """smoke_utils.__all__ must list all 29 public names.""" import autoskillit.smoke_utils as su expected = { @@ -2669,6 +2669,7 @@ def test_smoke_utils_all_exports_complete() -> None: "check_loop_iteration", "check_loop_with_progress", "check_review_loop", + "check_review_posted", "close_issue_already_done", "compile_eval_scorecard", "consolidate_health_reports", From 1d81f9d78a5811cec3df1e8a30e545d30b858a83 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 17:14:25 -0700 Subject: [PATCH 11/15] fix: eliminate 5 defects in check_review_posted wiring (issue #4204 Part A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes RC1–RC4a found on the combined merged tree: - RC1: Remove optional: true (without skip_when_false) from check_review_posted in implementation, implementation-groups, and remediation recipes. - RC2: Add catch-all on_result arm to check_review_posted in all 4 bundled recipes to satisfy unrouted-callable-verdict rule. - RC3: Add work_dir: ${{ context.work_dir }} to check_review_posted.with in merge-prs to satisfy run-python-requires-work-dir rule. - RC4: Fix _review_effect_verification_waypoint step selector to use resolve_skill_name() == "review-pr" instead of .startswith("/review-pr"), making the rule match /autoskillit:review-pr canonical form. - RC4a: Route re_push_review_integration.on_success to check_review_posted (not derive_batch_ci_event) in merge-prs, closing the bypass on the rebase-then-repush path. Updates compiled .json siblings, diagram hash comments, and contract cards. Adds test B22b (re_push_review_integration gates through check_review_posted) and B22c (check_review_posted routes to derive_batch_ci_event on true). Co-Authored-By: Claude Sonnet 4.6 --- .../recipe/rules/graph/rules_graph_review.py | 3 ++- .../recipes/diagrams/implementation-groups.md | 2 +- .../recipes/diagrams/implementation.md | 2 +- src/autoskillit/recipes/diagrams/merge-prs.md | 2 +- .../recipes/diagrams/remediation.md | 2 +- .../recipes/implementation-groups.json | 4 +++- .../recipes/implementation-groups.yaml | 2 +- src/autoskillit/recipes/implementation.json | 4 +++- src/autoskillit/recipes/implementation.yaml | 2 +- src/autoskillit/recipes/merge-prs.json | 10 +++++++--- src/autoskillit/recipes/merge-prs.yaml | 9 ++++++--- src/autoskillit/recipes/remediation.json | 4 +++- src/autoskillit/recipes/remediation.yaml | 2 +- tests/recipe/test_merge_prs.py | 20 ++++++++++++++++--- 14 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/autoskillit/recipe/rules/graph/rules_graph_review.py b/src/autoskillit/recipe/rules/graph/rules_graph_review.py index 03f95a026e..9d808c9ebc 100644 --- a/src/autoskillit/recipe/rules/graph/rules_graph_review.py +++ b/src/autoskillit/recipe/rules/graph/rules_graph_review.py @@ -182,7 +182,8 @@ def _review_effect_verification_waypoint(ctx: ValidationContext) -> list[RuleFin for step_id, step in ctx.recipe.steps.items() if ( step.tool == "run_skill" - and str((step.with_args or {}).get("skill_command", "")).startswith("/review-pr") + and resolve_skill_name(str((step.with_args or {}).get("skill_command", ""))) + == "review-pr" ) ] if not effect_steps: diff --git a/src/autoskillit/recipes/diagrams/implementation-groups.md b/src/autoskillit/recipes/diagrams/implementation-groups.md index 252b1af81d..03666c0d0e 100644 --- a/src/autoskillit/recipes/diagrams/implementation-groups.md +++ b/src/autoskillit/recipes/diagrams/implementation-groups.md @@ -1,4 +1,4 @@ - + ## implementation-groups Group-based implementation with per-group plan/implement/test cycles and PR gates. diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index 8782d8e156..6e267763f6 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 e3450f7132..f06e85a934 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 4e3550d62e..d66682d98e 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation Investigate, rectify, implement, and merge a bug fix with CI and PR gates. diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index 7e654debab..d0ca330c9a 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -1263,10 +1263,12 @@ { "when": "${{ result.reviews_posted }} == 'true'", "route": "check_review_loop" + }, + { + "route": "release_issue_failure" } ], "on_failure": "release_issue_failure", - "optional": true, "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 8741b5d923..535450a179 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -1125,8 +1125,8 @@ steps: route: release_issue_failure - when: "${{ result.reviews_posted }} == 'true'" route: check_review_loop + - route: release_issue_failure on_failure: release_issue_failure - optional: true optional_context_refs: - review_loop_count - review_mode diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index 20858afcab..fc99d13e9c 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -1336,10 +1336,12 @@ { "when": "${{ result.reviews_posted }} == 'true'", "route": "check_review_loop" + }, + { + "route": "release_issue_failure" } ], "on_failure": "release_issue_failure", - "optional": true, "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index 160eb1cf81..9f9316dd43 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -1211,8 +1211,8 @@ steps: route: release_issue_failure - when: ${{ result.reviews_posted }} == 'true' route: check_review_loop + - route: release_issue_failure on_failure: release_issue_failure - optional: true optional_context_refs: - review_loop_count - review_mode diff --git a/src/autoskillit/recipes/merge-prs.json b/src/autoskillit/recipes/merge-prs.json index 2e4024cc8d..2f028bce08 100644 --- a/src/autoskillit/recipes/merge-prs.json +++ b/src/autoskillit/recipes/merge-prs.json @@ -1540,7 +1540,8 @@ "callable": "autoskillit.smoke_utils.check_review_posted", "pr_number": "${{ context.review_pr_number }}", "output_dir": "{{AUTOSKILLIT_TEMP}}/review-pr", - "mode": "github" + "mode": "github", + "work_dir": "${{ context.work_dir }}" }, "on_result": [ { @@ -1550,6 +1551,9 @@ { "when": "${{ result.reviews_posted }} == 'true'", "route": "derive_batch_ci_event" + }, + { + "route": "register_clone_failure" } ], "on_failure": "register_clone_failure" @@ -1649,9 +1653,9 @@ "branch": "${{ context.batch_branch }}", "force": "true" }, - "on_success": "derive_batch_ci_event", + "on_success": "check_review_posted", "on_failure": "register_clone_failure", - "note": "Pushes the review-fixed integration branch to the remote (REQ-REVIEW-003). Routes to derive_batch_ci_event for branch-specific CI event derivation before final CI validation. On push failure, escalates to register_clone_failure.\n" + "note": "Pushes the review-fixed integration branch to the remote (REQ-REVIEW-003). Routes to check_review_posted (the effect-verification gate), which in turn routes to derive_batch_ci_event when reviews_posted == 'true'. On push failure, escalates to register_clone_failure.\n" }, "derive_batch_ci_event": { "tool": "check_repo_merge_state", diff --git a/src/autoskillit/recipes/merge-prs.yaml b/src/autoskillit/recipes/merge-prs.yaml index 25aa5a8b1b..62010251ca 100644 --- a/src/autoskillit/recipes/merge-prs.yaml +++ b/src/autoskillit/recipes/merge-prs.yaml @@ -1510,11 +1510,13 @@ steps: pr_number: ${{ context.review_pr_number }} output_dir: '{{AUTOSKILLIT_TEMP}}/review-pr' mode: github + work_dir: ${{ context.work_dir }} on_result: - when: ${{ result.reviews_posted }} == 'false' route: register_clone_failure - when: ${{ result.reviews_posted }} == 'true' route: derive_batch_ci_event + - route: register_clone_failure on_failure: register_clone_failure resolve_review_integration: tool: run_skill @@ -1589,11 +1591,12 @@ steps: remote_url: ${{ context.remote_url }} branch: ${{ context.batch_branch }} force: 'true' - on_success: derive_batch_ci_event + on_success: check_review_posted on_failure: register_clone_failure note: 'Pushes the review-fixed integration branch to the remote (REQ-REVIEW-003). - Routes to derive_batch_ci_event for branch-specific CI event derivation before - final CI validation. On push failure, escalates to register_clone_failure. + Routes to check_review_posted (the effect-verification gate), which in turn + routes to derive_batch_ci_event when reviews_posted == ''true''. On push + failure, escalates to register_clone_failure. ' derive_batch_ci_event: diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index af7aff1b43..84ebe92006 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -1477,10 +1477,12 @@ { "when": "${{ result.reviews_posted }} == 'true'", "route": "check_review_loop" + }, + { + "route": "release_issue_failure" } ], "on_failure": "release_issue_failure", - "optional": true, "optional_context_refs": [ "review_loop_count", "review_mode" diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index f613d016ea..f587a9238b 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -1321,8 +1321,8 @@ steps: route: release_issue_failure - when: ${{ result.reviews_posted }} == 'true' route: check_review_loop + - route: release_issue_failure on_failure: release_issue_failure - optional: true optional_context_refs: - review_loop_count - review_mode diff --git a/tests/recipe/test_merge_prs.py b/tests/recipe/test_merge_prs.py index 41e51ef383..87ee0000ea 100644 --- a/tests/recipe/test_merge_prs.py +++ b/tests/recipe/test_merge_prs.py @@ -574,10 +574,24 @@ def test_pmp_re_push_review_integration_uses_batch_branch(recipe) -> None: assert "context.batch_branch" in step.with_args.get("branch", "") -def test_pmp_re_push_review_integration_routes_to_derive_batch_ci_event(recipe) -> None: - """B22: re_push_review_integration.on_success must route through derive_batch_ci_event.""" +def test_pmp_re_push_review_integration_routes_to_check_review_posted(recipe) -> None: + """B22b: re_push_review_integration must gate through check_review_posted, not + route directly to derive_batch_ci_event — enforces the review-effect gate on the + rebase-then-repush path.""" step = recipe.steps["re_push_review_integration"] - assert step.on_success == "derive_batch_ci_event" + assert step.on_success == "check_review_posted" + + +def test_pmp_check_review_posted_routes_to_derive_batch_ci_event_on_success(recipe) -> None: + """B22c: check_review_posted routes to derive_batch_ci_event when reviews_posted==true.""" + step = recipe.steps["check_review_posted"] + assert step.on_result is not None + true_routes = [ + c.route + for c in step.on_result.conditions + if c.when and "reviews_posted" in c.when and "'true'" in c.when + ] + assert "derive_batch_ci_event" in true_routes def test_pmp_setup_remote_uses_context_remote_url(recipe) -> None: From 96b71789468eaa776e5b62f12f4bb04842ce1546 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 17:45:14 -0700 Subject: [PATCH 12/15] feat: add skill_name property and extract _get_review_pr_steps helper (issue #4204 Part B) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add RecipeStep.skill_name @property that calls resolve_skill_name — makes canonical normalization discoverable via IDE autocompletion on step. - Extract _get_review_pr_steps(ctx) from the inline comprehension in _review_effect_verification_waypoint — now independently testable. - Add tests/recipe/test_recipe_step_skill_name.py (5 tests covering both prefix forms, None returns, and different skills). - Update silence tests in test_rules_review_effect_waypoint.py to assert _get_review_pr_steps returns >= 1 steps before asserting 0 findings — structurally prevents vacuous silence. - Add test_review_effect_waypoint_selects_steps_on_bundled_recipes parametrized over 4 bundled recipes as an early-warning canary. Co-Authored-By: Claude Sonnet 4.6 --- .../recipe/rules/graph/rules_graph_review.py | 24 ++++++---- src/autoskillit/recipe/schema.py | 11 +++++ tests/recipe/AGENTS.md | 1 + tests/recipe/test_recipe_step_skill_name.py | 45 +++++++++++++++++++ .../test_rules_review_effect_waypoint.py | 41 +++++++++++++++++ 5 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 tests/recipe/test_recipe_step_skill_name.py diff --git a/src/autoskillit/recipe/rules/graph/rules_graph_review.py b/src/autoskillit/recipe/rules/graph/rules_graph_review.py index 9d808c9ebc..101194b9d0 100644 --- a/src/autoskillit/recipe/rules/graph/rules_graph_review.py +++ b/src/autoskillit/recipe/rules/graph/rules_graph_review.py @@ -166,6 +166,20 @@ def _check_run_skill_missing_context_limit(ctx: ValidationContext) -> list[RuleF _REVIEW_EFFECT_ADVANCE_GATES = frozenset({"check_review_loop", "derive_batch_ci_event"}) +def _get_review_pr_steps(ctx: ValidationContext) -> list[str]: + """Return step IDs that dispatch the review-pr skill (either prefix form). + + Exposed as a module-level function (not inlined in the rule body) so that + silence tests can call it directly and assert the rule is triggered on the + recipe under test before asserting zero findings. + """ + return [ + step_id + for step_id, step in ctx.recipe.steps.items() + if step.tool == "run_skill" and step.skill_name == "review-pr" + ] + + @semantic_rule( name="review-effect-verification-waypoint", severity=Severity.ERROR, @@ -177,15 +191,7 @@ def _check_run_skill_missing_context_limit(ctx: ValidationContext) -> list[RuleF ), ) def _review_effect_verification_waypoint(ctx: ValidationContext) -> list[RuleFinding]: - effect_steps = [ - step_id - for step_id, step in ctx.recipe.steps.items() - if ( - step.tool == "run_skill" - and resolve_skill_name(str((step.with_args or {}).get("skill_command", ""))) - == "review-pr" - ) - ] + effect_steps = _get_review_pr_steps(ctx) if not effect_steps: return [] findings = [] diff --git a/src/autoskillit/recipe/schema.py b/src/autoskillit/recipe/schema.py index 85e43a0421..e5d8256f8c 100644 --- a/src/autoskillit/recipe/schema.py +++ b/src/autoskillit/recipe/schema.py @@ -17,6 +17,7 @@ CaptureEntrySpec, DispatchGateType, RecipeSource, + resolve_skill_name, ) AUTOSKILLIT_VERSION_KEY: Final = "autoskillit_version" @@ -155,6 +156,16 @@ def __post_init__(self) -> None: f"the list, producing duplicates. Set retries: 0." ) + @property + def skill_name(self) -> str | None: + """Canonical skill name, resolving /name and /autoskillit:name forms. + + Returns None if with_args has no skill_command or if resolve_skill_name + cannot extract a name (template-only, unrecognized format, etc.). + Use this in rule predicates instead of raw string operations on with_args. + """ + return resolve_skill_name(str((self.with_args or {}).get("skill_command", ""))) + @dataclass(frozen=True, slots=True) class RecipeBlock: diff --git a/tests/recipe/AGENTS.md b/tests/recipe/AGENTS.md index fc573026cd..c6bb91e3ce 100644 --- a/tests/recipe/AGENTS.md +++ b/tests/recipe/AGENTS.md @@ -96,6 +96,7 @@ Recipe I/O, validation, semantic rules, schema, and bundled recipe tests. | `test_recipe_output_dir_resolvability.py` | Guard: recipe output_dir values must be server-resolvable (no ${{ }} templates) | | `test_report_frontmatter_schema.py` | Tests for report.md YAML frontmatter audit-trail schema | | `test_recipe_scripts.py` | Tests for recipe script callables | +| `test_recipe_step_skill_name.py` | Tests for RecipeStep.skill_name property — canonical skill name resolution for both /name and /autoskillit:name forms | | `test_recipe_temp_substitution.py` | Tests for {{AUTOSKILLIT_TEMP}} substitution in recipe steps | | `test_remediation_depth_ingredient.py` | Tests for remediation depth ingredient configuration | | `test_remediation_pr_decomposition.py` | Tests for remediation PR decomposition recipe | diff --git a/tests/recipe/test_recipe_step_skill_name.py b/tests/recipe/test_recipe_step_skill_name.py new file mode 100644 index 0000000000..a16c7c5e3d --- /dev/null +++ b/tests/recipe/test_recipe_step_skill_name.py @@ -0,0 +1,45 @@ +"""Tests for RecipeStep.skill_name property.""" + +from __future__ import annotations + +import pytest + +from autoskillit.recipe.schema import RecipeStep + +pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] + + +def test_skill_name_bare_form(): + """skill_name normalizes /name → canonical skill name.""" + step = RecipeStep(tool="run_skill", with_args={"skill_command": "/review-pr main feat"}) + assert step.skill_name == "review-pr" + + +def test_skill_name_namespaced_form(): + """skill_name normalizes /autoskillit:name → canonical skill name.""" + step = RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:review-pr ${{ context.x }}"}, + ) + assert step.skill_name == "review-pr" + + +def test_skill_name_non_skill_step(): + """skill_name returns None when with_args has no skill_command.""" + step = RecipeStep(tool="run_python", with_args={}) + assert step.skill_name is None + + +def test_skill_name_empty_command(): + """skill_name returns None for empty skill_command.""" + step = RecipeStep(tool="run_skill", with_args={"skill_command": ""}) + assert step.skill_name is None + + +def test_skill_name_different_skill(): + """skill_name correctly extracts name for non-review-pr skills.""" + step = RecipeStep( + tool="run_skill", + with_args={"skill_command": "/autoskillit:audit-impl ${{ context.plan_path }}"}, + ) + assert step.skill_name == "audit-impl" diff --git a/tests/recipe/test_rules_review_effect_waypoint.py b/tests/recipe/test_rules_review_effect_waypoint.py index db9f7e1864..13d3f55e13 100644 --- a/tests/recipe/test_rules_review_effect_waypoint.py +++ b/tests/recipe/test_rules_review_effect_waypoint.py @@ -8,8 +8,10 @@ import pytest +from autoskillit.recipe._analysis import make_validation_context from autoskillit.recipe.io import builtin_recipes_dir, load_recipe from autoskillit.recipe.registry import _RULE_REGISTRY, run_semantic_rules +from autoskillit.recipe.rules.graph.rules_graph_review import _get_review_pr_steps from autoskillit.recipe.schema import Recipe, RecipeStep, StepResultCondition, StepResultRoute pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] @@ -34,6 +36,11 @@ def test_effect_waypoint_rule_is_registered(): def test_effect_waypoint_rule_silent_on_fixed_recipes(recipe_name): """After Part A fix, the rule must not fire on review-loop recipes.""" recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + selected = _get_review_pr_steps(make_validation_context(recipe)) + assert len(selected) >= 1, ( + f"precondition: _get_review_pr_steps returned no steps for {recipe_name!r} — " + "rule would be vacuously silent (dead matcher?)" + ) violations = run_semantic_rules(recipe) rule_violations = [v for v in violations if v.rule == RULE_ID] assert len(rule_violations) == 0, ( @@ -44,6 +51,11 @@ def test_effect_waypoint_rule_silent_on_fixed_recipes(recipe_name): def test_effect_waypoint_rule_silent_on_merge_prs(): """After Part A fix, the rule must not fire on merge-prs.yaml.""" recipe = load_recipe(builtin_recipes_dir() / "merge-prs.yaml") + selected = _get_review_pr_steps(make_validation_context(recipe)) + assert len(selected) >= 1, ( + "precondition: _get_review_pr_steps returned no steps for 'merge-prs' — " + "rule would be vacuously silent (dead matcher?)" + ) violations = run_semantic_rules(recipe) rule_violations = [v for v in violations if v.rule == RULE_ID] assert len(rule_violations) == 0, f"[merge-prs] {RULE_ID} fired after fix: {rule_violations}" @@ -125,6 +137,11 @@ def test_effect_waypoint_rule_silent_when_gate_present(): "done": RecipeStep(action="stop", message="done"), } ) + selected = _get_review_pr_steps(make_validation_context(recipe)) + assert len(selected) >= 1, ( + "precondition: _get_review_pr_steps returned no steps — " + "rule would be vacuously silent (dead matcher?)" + ) violations = run_semantic_rules(recipe) rule_violations = [v for v in violations if v.rule == RULE_ID] assert len(rule_violations) == 0 @@ -161,3 +178,27 @@ def test_effect_waypoint_rule_covers_aliased_step_names(): violations = run_semantic_rules(recipe) rule_violations = [v for v in violations if v.rule == RULE_ID] assert len(rule_violations) >= 1 + + +@pytest.mark.parametrize( + "recipe_name", + [ + "implementation", + "implementation-groups", + "remediation", + "merge-prs", + ], +) +def test_review_effect_waypoint_selects_steps_on_bundled_recipes(recipe_name): + """Step-selector must return >= 1 steps on each bundled recipe. + + This test fails immediately if the rule's step-selection predicate + is wrong (dead matcher) — before any silence test can pass vacuously. + """ + recipe = load_recipe(builtin_recipes_dir() / f"{recipe_name}.yaml") + ctx = make_validation_context(recipe) + selected = _get_review_pr_steps(ctx) + assert len(selected) >= 1, ( + f"_get_review_pr_steps returned no steps for {recipe_name!r} — " + "the rule is inactive on this recipe" + ) From c6583984280cce560fe8dca70b089b9042d8e160 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 18:29:52 -0700 Subject: [PATCH 13/15] fix(review): remove dead _net assignment in ClaudeCodeBackend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _net = cfg.get("network_access", False) was a no-op — never consumed by build_skill_session_cmd or any downstream caller. Claude Code has native network access via CLAUDE_CODE_CAPABILITIES; no sandbox override needed. Co-Authored-By: Claude Sonnet 4.6 --- src/autoskillit/execution/backends/claude.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/autoskillit/execution/backends/claude.py b/src/autoskillit/execution/backends/claude.py index dad366f5e7..7bdc23eddb 100644 --- a/src/autoskillit/execution/backends/claude.py +++ b/src/autoskillit/execution/backends/claude.py @@ -543,7 +543,6 @@ def build_skill_session_cmd( resume_checkpoint = cfg["resume_checkpoint"] resume_message = cfg["resume_message"] sandbox_mode = cfg["sandbox_mode"] # noqa: F841 - _net = cfg.get("network_access", False) # noqa: F841 _has_prefix = ( bool(profile_name) From 748fc1a7e5850a332c8bf67cbc678d03c8629ea0 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 18:30:01 -0700 Subject: [PATCH 14/15] fix(review): remove stale test-file reference from check_review_posted docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docstring said the is_absolute() guard was "required by tests/arch/test_run_python_path_resolution.py" — inverting causality. The guard exists because absolute paths are architecturally correct; the test merely verifies the convention. Embedding a test filename as justification will rot if the test is renamed. Co-Authored-By: Claude Sonnet 4.6 --- src/autoskillit/smoke_utils/_review.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/autoskillit/smoke_utils/_review.py b/src/autoskillit/smoke_utils/_review.py index c82fc8458a..7b42b6b531 100644 --- a/src/autoskillit/smoke_utils/_review.py +++ b/src/autoskillit/smoke_utils/_review.py @@ -212,8 +212,7 @@ def check_review_posted( when the receipt file is absent, indicating the review POST did not complete. In local mode, always returns reviews_posted="true" (no API calls made). - output_dir must be an absolute path (enforced by is_absolute() guard required by - tests/arch/test_run_python_path_resolution.py for output_dir parameters). + output_dir must be an absolute path. """ if not Path(output_dir).is_absolute(): raise ValueError(f"output_dir must be an absolute path, got: {output_dir!r}") From e4de550929af5269b9b864400c55afd6acc3ac5a Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 7 Jul 2026 18:33:36 -0700 Subject: [PATCH 15/15] fix(test): update json write-site allowlist line numbers after docstring removal Removing the two-line test-file parenthetical from check_review_posted docstring shifted the json.dumps call sites in _review.py by -1 line. Update _LEGACY_JSON_WRITES entries from 341/433 to 340/432. Co-Authored-By: Claude Sonnet 4.6 --- tests/infra/test_schema_version_convention.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/infra/test_schema_version_convention.py b/tests/infra/test_schema_version_convention.py index 2ba4da08df..960db9235d 100644 --- a/tests/infra/test_schema_version_convention.py +++ b/tests/infra/test_schema_version_convention.py @@ -152,8 +152,8 @@ def _is_yaml_dump(node: ast.expr) -> bool: ("src/autoskillit/smoke_utils/_review.py", 101), ("src/autoskillit/smoke_utils/_review.py", 102), ("src/autoskillit/smoke_utils/_review.py", 120), - ("src/autoskillit/smoke_utils/_review.py", 341), - ("src/autoskillit/smoke_utils/_review.py", 433), + ("src/autoskillit/smoke_utils/_review.py", 340), + ("src/autoskillit/smoke_utils/_review.py", 432), # smoke_utils/_git.py — partitions, merge queue data # Line 110 is a list-payload write site (dual membership: also in list_sites # in test_allowlist_includes_list_payloads_as_documented). The AST scanner catches