diff --git a/features/resource_cli.feature b/features/resource_cli.feature index ed2da316e..0d937cbd7 100644 --- a/features/resource_cli.feature +++ b/features/resource_cli.feature @@ -69,14 +69,37 @@ Feature: Resource CLI commands Scenario: Add a git-checkout resource Given built-in types are bootstrapped When I run resource add "git-checkout" "local/my-repo" with path "/tmp/repo" - Then the resource output should contain "Added resource" + Then the resource output should contain "Resource registered" + And the resource output should contain "Auto-discovered Children" And the resource output should contain "local/my-repo" + Scenario: Add a git-checkout resource in JSON format + Given built-in types are bootstrapped + And the resource CLI output format is "json" + When I run resource add "git-checkout" "local/json-add" with path "/tmp/json-add" + Then the resource output should be valid JSON + And the resource JSON output should contain key "children" + And the resource JSON output should contain key "children_count" + + Scenario: Add an fs-directory resource shows auto-discovered children in rich output + Given built-in types are bootstrapped + And a seeded directory exists at "/tmp/fs-dir-cli-add" + When I run resource add "fs-directory" "local/fs-dir-cli-add" with path "/tmp/fs-dir-cli-add" + Then the resource output should contain "Auto-discovered Children" + And the resource output should contain "child resource" + Scenario: Add a resource with non-existent type When I run resource add "nonexistent" "local/test" with path "/tmp" Then the resource command should fail And the resource output should contain "not found" + Scenario: Resource add rolls back when auto discovery fails + Given built-in types are bootstrapped + And auto discovery will fail during resource registration + When I run resource add "git-checkout" "local/atomic-failure" with path "/tmp/atomic" + Then the resource command should fail + And the resource registry should not contain resource "local/atomic-failure" + # ---- Resource List ---- Scenario: List resources when empty diff --git a/features/resource_cli_tree.feature b/features/resource_cli_tree.feature index 1df921b94..08dfe3900 100644 --- a/features/resource_cli_tree.feature +++ b/features/resource_cli_tree.feature @@ -219,11 +219,12 @@ Feature: Resource CLI tree, inspect, link-child, and unlink-child commands Then the children list should have 2 items And the first child name should be "local/gc-a-child" - Scenario: Service get_children returns empty list for leaf + Scenario: Service get_children returns auto-discovered children for directory Given resource tree built-in types are bootstrapped + And a seeded directory exists at "/tmp/gcl" And a resource tree resource "fs-directory" named "local/gc-leaf" at "/tmp/gcl" When I get children of resource "local/gc-leaf" - Then the children list should have 0 items + Then the children list should have at least 1 child # ---- Inspect with no properties ---- diff --git a/features/resource_registry_service_coverage.feature b/features/resource_registry_service_coverage.feature index 9e53d8dd3..de8026900 100644 --- a/features/resource_registry_service_coverage.feature +++ b/features/resource_registry_service_coverage.feature @@ -24,6 +24,13 @@ Feature: Resource registry service coverage gaps Then the register_resource faulty session should have been rolled back And the original generic exception should propagate from register_resource + Scenario: register_resource cleans up after auto-discovery failure + Given a real in-memory resource registry service is initialised + And a user-addable resource type "covns/auto-fail" exists for auto-discovery failure coverage + When I register that resource and auto-discovery raises + Then the resource should not remain persisted after auto-discovery failure + And the auto-discovery exception should propagate from register_resource + # ── _spec_to_db auto_discovery and equivalence ──────────────────── Scenario: _spec_to_db serialises auto_discovery to JSON diff --git a/features/steps/resource_cli_steps.py b/features/steps/resource_cli_steps.py index a47217153..09ec005da 100644 --- a/features/steps/resource_cli_steps.py +++ b/features/steps/resource_cli_steps.py @@ -18,7 +18,11 @@ from sqlalchemy.orm import sessionmaker from cleveragents.application.services.resource_registry_service import ( ResourceRegistryService, ) -from cleveragents.core.exceptions import CleverAgentsError, ValidationError +from cleveragents.core.exceptions import ( + CleverAgentsError, + NotFoundError, + ValidationError, +) from cleveragents.infrastructure.database.models import Base @@ -269,13 +273,14 @@ def _do_resource_add( clone_into: str | None = None, read_only: bool = False, update: bool = False, - fmt: str = "rich", + fmt: str | None = None, ) -> None: """Run resource add command (shared impl).""" from cleveragents.cli.commands.resource import resource_add orig = _patch_service(context) try: + resolved_fmt = fmt or getattr(context, "resource_cli_format", "rich") output, failed = _capture_output( resource_add, type_name=type_name, @@ -290,11 +295,13 @@ def _do_resource_add( clone_into=clone_into, read_only=read_only, update=update, - fmt=fmt, + fmt=resolved_fmt, ) context.resource_cli_output = output context.resource_cli_failed = failed finally: + if fmt is None and hasattr(context, "resource_cli_format"): + delattr(context, "resource_cli_format") _unpatch_service(orig) @@ -332,6 +339,24 @@ def step_run_resource_add_path_url( _do_resource_add(context, type_name, name, path, url=url_value) +@given('the resource CLI output format is "{fmt}"') +def step_set_resource_cli_format(context: Context, fmt: str) -> None: + """Set the desired output format for the next resource CLI command.""" + context.resource_cli_format = fmt + + +@given("auto discovery will fail during resource registration") +def step_auto_discovery_failure(context: Context) -> None: + """Force ResourceRepository.auto_discover_children to raise during the next call.""" + + patcher = patch( + "cleveragents.infrastructure.database.repositories.ResourceRepository.auto_discover_children", + side_effect=RuntimeError("Simulated auto-discovery failure"), + ) + patcher.start() + context.add_cleanup(patcher.stop) + + # ---- Resource List ---- @@ -488,7 +513,24 @@ def step_resource_json_has_key(context: Context, key: str) -> None: assert len(data) > 0, "JSON output is an empty list" assert key in data[0], f"Key '{key}' not found in first item of JSON list" else: - assert key in data, f"Key '{key}' not found in JSON output" + if key in data: + return + payload = data.get("data") if isinstance(data, dict) else None + if isinstance(payload, dict) and key in payload: + return + raise AssertionError(f"Key '{key}' not found in JSON output") + + +@then('the resource registry should not contain resource "{name}"') +def step_registry_missing_resource(context: Context, name: str) -> None: + """Ensure that a resource was not persisted in the registry.""" + + service = _make_service(context) + try: + service.show_resource(name) + except NotFoundError: + return + raise AssertionError(f"Resource '{name}' was persisted despite failure") # --------------------------------------------------------------------------- diff --git a/features/steps/resource_cli_tree_steps.py b/features/steps/resource_cli_tree_steps.py index 8177a525f..0a84d74f2 100644 --- a/features/steps/resource_cli_tree_steps.py +++ b/features/steps/resource_cli_tree_steps.py @@ -149,6 +149,21 @@ def step_tree_create_temp_file(context: Context, filename: str, content: str) -> filepath.write_text(content, encoding="utf-8") +@given('a seeded directory exists at "{path}"') +def step_tree_seed_directory(context: Context, path: str) -> None: + """Create the directory and drop a sentinel file inside. + + Used by scenarios that exercise filesystem auto-discovery on an + ``fs-directory`` resource — without an explicit seed the scenario + would be CI-environment-dependent (empty / missing ``path`` ⇒ + zero discovered children ⇒ flake). + """ + target = Path(path) + target.mkdir(parents=True, exist_ok=True) + sentinel = target / ".cleveragents-tree-sentinel" + sentinel.write_text("seed", encoding="utf-8") + + @given('resource tree child "{child}" is linked to parent "{parent}"') def step_tree_link_child(context: Context, child: str, parent: str) -> None: """Link a child to a parent.""" @@ -459,6 +474,14 @@ def step_children_count(context: Context, count: int) -> None: ) +@then("the children list should have at least {count:d} child") +@then("the children list should have at least {count:d} children") +def step_children_count_at_least(context: Context, count: int) -> None: + """Assert children list length is at least *count*.""" + actual = len(context.tree_children_result) + assert actual >= count, f"Expected at least {count} children, got {actual}" + + @then('the first child name should be "{name}"') def step_first_child_name(context: Context, name: str) -> None: """Assert the first child's name.""" diff --git a/features/steps/resource_registry_service_coverage_steps.py b/features/steps/resource_registry_service_coverage_steps.py index 9f4789fdd..9008a4086 100644 --- a/features/steps/resource_registry_service_coverage_steps.py +++ b/features/steps/resource_registry_service_coverage_steps.py @@ -11,8 +11,9 @@ from __future__ import annotations import json import tempfile +from datetime import UTC, datetime from typing import Any -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from behave import given, then, when # type: ignore[import-untyped] from sqlalchemy import create_engine @@ -30,6 +31,7 @@ from cleveragents.domain.models.core.resource_type import ( ) from cleveragents.infrastructure.database.models import ( Base, + ResourceModel, ResourceTypeModel, ) @@ -210,6 +212,97 @@ def step_regrsc_exception_propagates(context: Any) -> None: assert "filesystem gone" in str(context.regrsc_exception) +# --------------------------------------------------------------------------- +# Scenario: register_resource cleans up after auto-discovery failure +# --------------------------------------------------------------------------- + + +@given( + 'a user-addable resource type "{type_name}" exists for auto-discovery failure coverage' +) +def step_auto_fail_type_exists(context: Any, type_name: str) -> None: + if not hasattr(context, "cov_rt_factory"): + raise AssertionError( + "In-memory service factory must be initialised before adding type" + ) + + namespace = type_name.split("/", 1)[0] if "/" in type_name else "local" + session = context.cov_rt_factory() + now_iso = datetime.now(tz=UTC).isoformat() + row = ResourceTypeModel( + name=type_name, + namespace=namespace, + description="Auto-discovery failure coverage type", + resource_kind="physical", + sandbox_strategy="git_worktree", + user_addable=True, + handler_ref=None, + args_schema_json=None, + allowed_parent_types_json=None, + allowed_child_types_json=None, + auto_discover_json=json.dumps( + { + "enabled": True, + "rules": [ + { + "type": f"{type_name}-child", + } + ], + } + ), + capabilities_json=json.dumps( + {"read": True, "write": True, "sandbox": False, "checkpoint": False} + ), + equivalence_json=None, + source="test", + created_at=now_iso, + updated_at=now_iso, + ) + session.add(row) + session.commit() + context.cov_auto_fail_type = type_name + + +@when("I register that resource and auto-discovery raises") +def step_register_resource_auto_failure(context: Any) -> None: + context.auto_disc_exception = None + context.cov_auto_fail_resource_name = "local/auto-fail" + + patcher = patch( + "cleveragents.infrastructure.database.repositories." + "ResourceRepository.auto_discover_children", + side_effect=RuntimeError("auto-discovery failure"), + ) + patcher.start() + try: + context.cov_rt_svc.register_resource( + type_name=context.cov_auto_fail_type, + name=context.cov_auto_fail_resource_name, + location=None, + ) + except RuntimeError as exc: + context.auto_disc_exception = exc + finally: + patcher.stop() + + +@then("the resource should not remain persisted after auto-discovery failure") +def step_verify_cleanup(context: Any) -> None: + session = context.cov_rt_factory() + row = ( + session.query(ResourceModel) + .filter_by(namespaced_name=context.cov_auto_fail_resource_name) + .first() + ) + assert row is None, "Resource should have been removed after auto-discovery failure" + + +@then("the auto-discovery exception should propagate from register_resource") +def step_verify_auto_disc_exception(context: Any) -> None: + assert context.auto_disc_exception is not None + assert "auto-discovery failure" in str(context.auto_disc_exception) + + # --------------------------------------------------------------------------- # Scenario: _spec_to_db serialises auto_discovery to JSON # --------------------------------------------------------------------------- @@ -301,8 +394,6 @@ def step_db_row_with_auto_discover(context: Any) -> None: context.ad_original = {"glob": "**/*.py", "depth": 3} session = factory() - from datetime import UTC, datetime - now = datetime.now(tz=UTC).isoformat() row = ResourceTypeModel( name="git-checkout", @@ -355,8 +446,6 @@ def step_db_row_with_equivalence(context: Any) -> None: context.eq_original = {"criteria": ["hash"], "description": "Exact match"} session = factory() - from datetime import UTC, datetime - now = datetime.now(tz=UTC).isoformat() row = ResourceTypeModel( name="testns/virt-eq", @@ -473,8 +562,6 @@ def step_db_row_with_namespace_and_name( context.ns_builtin_factory = factory session = factory() - from datetime import UTC, datetime - now = datetime.now(tz=UTC).isoformat() row = ResourceTypeModel( name=name, diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index 0d54a1b04..f9965405b 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -28,6 +28,7 @@ from cleveragents.infrastructure.database.models import ( ResourceModel, ResourceTypeModel, ) +from cleveragents.infrastructure.database.repositories import ResourceRepository from cleveragents.resource.inheritance import find_subtypes __all__ = [ @@ -150,6 +151,31 @@ class ResourceInstanceMixin: session.add(db_resource) session.flush() + + # Run auto-discovery using the SAME session with commit=False so + # the parent + every auto-discovered child are committed atomically + # by the single ``session.commit()`` below. Any exception inside + # auto_discover_children propagates to the outer ``except`` blocks, + # which roll the whole transaction back — the parent is never + # persisted on a failure. We intentionally avoid the + # ``with session.begin():`` pattern here because callers (e.g. + # the WF05 integration helper) reuse one Session across many ops + # and that pattern raises ``InvalidRequestError`` when an outer + # transaction is already begun. + repository = ResourceRepository(self._session) + try: + repository.auto_discover_children( + resource_id, + session=session, + commit=False, + ) + except Exception: + logger.error( + "Auto-discovery failed; rolling back parent resource", + extra={"resource_id": resource_id, "type_name": type_name}, + ) + raise + session.commit() return Resource( diff --git a/src/cleveragents/cli/commands/resource.py b/src/cleveragents/cli/commands/resource.py index bc4d6a754..153a5e39c 100644 --- a/src/cleveragents/cli/commands/resource.py +++ b/src/cleveragents/cli/commands/resource.py @@ -56,6 +56,7 @@ from __future__ import annotations import json import logging import re +from collections import Counter from pathlib import Path from typing import Annotated, Any @@ -198,6 +199,21 @@ def _resource_dict(resource: Any) -> dict[str, object]: } +def _short_resource_id(resource_id: str, *, length: int = 12) -> str: + """Return a truncated resource ID for concise table display.""" + if len(resource_id) <= length: + return resource_id + return f"{resource_id[:length]}…" + + +def _format_child_status(resource: Any) -> str: + """Derive a human-readable status label for an auto-discovered child.""" + status = _get_lifecycle_state_str(resource) + if status: + return status + return "created" + + # --------------------------------------------------------------------------- # Resource Type commands # --------------------------------------------------------------------------- @@ -811,14 +827,78 @@ def resource_add( properties=properties if properties else None, ) + children = service.get_children(resource.resource_id) + display_name = resource.name or "(unnamed)" + summary_line = f"Added resource: {display_name} (id: {resource.resource_id})" + if fmt != OutputFormat.RICH.value: data = _resource_dict(resource) + data["children"] = [_resource_dict(child) for child in children] + data["children_count"] = len(children) console.print(format_output(data, fmt)) return + details = Table.grid(padding=(0, 1)) + details.add_column(justify="right", style="cyan") + details.add_column(style="white") + details.add_row("Name", resource.name or "(unnamed)") + details.add_row("ID", resource.resource_id) + details.add_row("Type", resource.resource_type_name) + details.add_row("Kind", str(resource.classification)) + if resource.location: + details.add_row("Location", resource.location) + if resource.description: + details.add_row("Description", resource.description) + if resource.properties: + details.add_row("Properties", json.dumps(resource.properties)) + + console.print(Panel(details, title="Resource", expand=False)) + + if children: + child_table = Table( + title="Auto-discovered Children", + show_header=True, + header_style="cyan", + ) + child_table.add_column("ID", style="dim") + child_table.add_column("Type", style="cyan") + child_table.add_column("Status", style="green") + + max_detailed_rows = 5 + for child in children[:max_detailed_rows]: + child_table.add_row( + _short_resource_id(child.resource_id), + child.resource_type_name, + _format_child_status(child), + ) + + if len(children) > max_detailed_rows: + remaining = children[max_detailed_rows:] + counts = Counter(child.resource_type_name for child in remaining) + for type_name, count in sorted( + counts.items(), key=lambda item: (-item[1], item[0]) + ): + label = ( + f"+ {count} {type_name}" + f" {'resource' if count == 1 else 'resources'}" + ) + child_table.add_row("", label, "") + + console.print(child_table) + else: + console.print( + Panel( + "No auto-discovered children", + title="Auto-discovered Children", + expand=False, + ) + ) + + console.print(summary_line) + child_count = len(children) + noun = "child resource" if child_count == 1 else "child resources" console.print( - f"[green]Added resource:[/green] {resource.name} " - f"(id: {resource.resource_id})" + f"[green]✓ OK[/green] Resource registered ({child_count} {noun} discovered)" ) except NotFoundError as exc: diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 6ae44b067..f840676c2 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -2679,7 +2679,13 @@ class ResourceRepository: ) from exc @database_retry - def auto_discover_children(self, resource_id: str) -> list[Any]: + def auto_discover_children( + self, + resource_id: str, + *, + session: Session | None = None, + commit: bool = True, + ) -> list[Any]: """Materialize child resources per type auto-discovery. Looks up the resource's type, checks auto_discovery config, @@ -2688,6 +2694,9 @@ class ResourceRepository: Args: resource_id: ULID of the parent resource. + session: Optional SQLAlchemy session to reuse. When provided, + the caller manages commit/rollback/close semantics. + commit: Whether this method should commit the transaction. Returns: List of newly created child ``Resource`` domain objects. @@ -2704,7 +2713,8 @@ class ResourceRepository: ResourceCapabilities, ) - session = self._session() + own_session = session is None + session = session or self._session() try: parent_row = ( session.query(ResourceModel).filter_by(resource_id=resource_id).first() @@ -2811,8 +2821,11 @@ class ResourceRepository: ) created.append(child_resource) + if commit: + session.commit() return created except ResourceNotFoundRepoError: + session.rollback() raise except ( OperationalError, @@ -2822,6 +2835,9 @@ class ResourceRepository: raise DatabaseError( f"Failed to auto-discover children for '{resource_id}': {exc}" ) from exc + finally: + if own_session: + session.close() @staticmethod def _get_ancestors(session: Session, resource_id: str) -> set[str]: