From bc0baae7778b448b982141c0c06858875b1116b8 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 10 Apr 2026 00:50:54 +0000 Subject: [PATCH 1/8] fix(resource): trigger auto-discovery when adding resource (#6464) ISSUES CLOSED: #6464 --- features/resource_cli.feature | 11 ++- features/steps/resource_cli_steps.py | 20 ++++- .../services/_resource_registry_ops.py | 7 +- src/cleveragents/cli/commands/resource.py | 84 ++++++++++++++++++- .../infrastructure/database/repositories.py | 4 + 5 files changed, 119 insertions(+), 7 deletions(-) diff --git a/features/resource_cli.feature b/features/resource_cli.feature index ed2da316e..55cac7a48 100644 --- a/features/resource_cli.feature +++ b/features/resource_cli.feature @@ -69,9 +69,18 @@ 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 a resource with non-existent type When I run resource add "nonexistent" "local/test" with path "/tmp" Then the resource command should fail diff --git a/features/steps/resource_cli_steps.py b/features/steps/resource_cli_steps.py index a47217153..6b6a32df3 100644 --- a/features/steps/resource_cli_steps.py +++ b/features/steps/resource_cli_steps.py @@ -269,13 +269,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 +291,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 +335,12 @@ 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 + + # ---- Resource List ---- @@ -488,7 +497,12 @@ 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") # --------------------------------------------------------------------------- diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index 0d54a1b04..a61eeb57a 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__ = [ @@ -152,7 +153,10 @@ class ResourceInstanceMixin: session.flush() session.commit() - return Resource( + repository = ResourceRepository(self._session) + repository.auto_discover_children(resource_id) + + resource = Resource( resource_id=resource_id, name=name, resource_type_name=type_name, @@ -165,6 +169,7 @@ class ResourceInstanceMixin: created_at=datetime.fromisoformat(now_iso), updated_at=datetime.fromisoformat(now_iso), ) + return resource except (NotFoundError, ValidationError): session.rollback() raise 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..7a1ada0b3 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -2811,8 +2811,10 @@ class ResourceRepository: ) created.append(child_resource) + session.commit() return created except ResourceNotFoundRepoError: + session.rollback() raise except ( OperationalError, @@ -2822,6 +2824,8 @@ class ResourceRepository: raise DatabaseError( f"Failed to auto-discover children for '{resource_id}': {exc}" ) from exc + finally: + session.close() @staticmethod def _get_ancestors(session: Session, resource_id: str) -> set[str]: -- 2.52.0 From 58cb75e5b8fff50a83cfdd1b35407990a25eb315 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 10 Apr 2026 19:42:54 +0000 Subject: [PATCH 2/8] fix(resource): keep register resource atomic Ensure the resource registry removes the parent record when auto-discovery raises so the operation remains atomic. Refs: #6464 --- ...resource_registry_service_coverage.feature | 7 ++ ...esource_registry_service_coverage_steps.py | 96 ++++++++++++++++++- .../services/_resource_registry_ops.py | 29 +++++- 3 files changed, 130 insertions(+), 2 deletions(-) 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_registry_service_coverage_steps.py b/features/steps/resource_registry_service_coverage_steps.py index 9f4789fdd..c57b0f057 100644 --- a/features/steps/resource_registry_service_coverage_steps.py +++ b/features/steps/resource_registry_service_coverage_steps.py @@ -12,7 +12,7 @@ from __future__ import annotations import json import tempfile 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 +30,7 @@ from cleveragents.domain.models.core.resource_type import ( ) from cleveragents.infrastructure.database.models import ( Base, + ResourceModel, ResourceTypeModel, ) @@ -210,6 +211,99 @@ 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() + from datetime import UTC, datetime + + 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 # --------------------------------------------------------------------------- diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index a61eeb57a..6e6cead18 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -154,7 +154,34 @@ class ResourceInstanceMixin: session.commit() repository = ResourceRepository(self._session) - repository.auto_discover_children(resource_id) + try: + repository.auto_discover_children(resource_id) + except Exception as auto_exc: + logger.error( + "Auto-discovery failed; rolling back parent resource", + extra={"resource_id": resource_id, "type_name": type_name}, + ) + cleanup_session = self._session() + try: + cleanup_session.query(ResourceModel).filter_by( + resource_id=resource_id + ).delete() + cleanup_session.commit() + except Exception as cleanup_exc: + cleanup_session.rollback() + logger.exception( + "Failed to clean up parent resource after auto-discovery failure", + extra={ + "resource_id": resource_id, + "type_name": type_name, + }, + ) + raise cleanup_exc from auto_exc + finally: + cleanup_session.close() + + session.rollback() + raise resource = Resource( resource_id=resource_id, -- 2.52.0 From 5df4d2c4bcac04caaa0a37407f8b428f73a8a937 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 10 Apr 2026 20:31:53 +0000 Subject: [PATCH 3/8] fix(resource-registry): keep resource auto discovery atomic --- .../services/_resource_registry_ops.py | 28 +++++-------------- .../infrastructure/database/repositories.py | 26 +++++++++++++---- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index 6e6cead18..e93f8aa14 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -151,38 +151,24 @@ class ResourceInstanceMixin: session.add(db_resource) session.flush() - session.commit() repository = ResourceRepository(self._session) try: - repository.auto_discover_children(resource_id) + repository.auto_discover_children( + resource_id, + session=session, + commit=False, + ) except Exception as auto_exc: logger.error( "Auto-discovery failed; rolling back parent resource", extra={"resource_id": resource_id, "type_name": type_name}, ) - cleanup_session = self._session() - try: - cleanup_session.query(ResourceModel).filter_by( - resource_id=resource_id - ).delete() - cleanup_session.commit() - except Exception as cleanup_exc: - cleanup_session.rollback() - logger.exception( - "Failed to clean up parent resource after auto-discovery failure", - extra={ - "resource_id": resource_id, - "type_name": type_name, - }, - ) - raise cleanup_exc from auto_exc - finally: - cleanup_session.close() - session.rollback() raise + session.commit() + resource = Resource( resource_id=resource_id, name=name, diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 7a1ada0b3..41f90eeb8 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,21 +2821,25 @@ class ResourceRepository: ) created.append(child_resource) - session.commit() + if commit: + session.commit() return created except ResourceNotFoundRepoError: - session.rollback() + if commit or own_session: + session.rollback() raise except ( OperationalError, SQLAlchemyDatabaseError, ) as exc: - session.rollback() + if commit or own_session: + session.rollback() raise DatabaseError( f"Failed to auto-discover children for '{resource_id}': {exc}" ) from exc finally: - session.close() + if own_session: + session.close() @staticmethod def _get_ancestors(session: Session, resource_id: str) -> set[str]: -- 2.52.0 From 5c9002c540f99a706c5215874f352165fa706e6f Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 11 Apr 2026 03:30:50 +0000 Subject: [PATCH 4/8] test(resource-cli): expect auto-discovered children Update resource CLI tree Behave scenario to expect auto-discovery output and add a reusable assertion for minimum child counts. Refs: #6464 --- features/resource_cli_tree.feature | 4 ++-- features/steps/resource_cli_tree_steps.py | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/features/resource_cli_tree.feature b/features/resource_cli_tree.feature index 1df921b94..917e538b0 100644 --- a/features/resource_cli_tree.feature +++ b/features/resource_cli_tree.feature @@ -219,11 +219,11 @@ 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 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/steps/resource_cli_tree_steps.py b/features/steps/resource_cli_tree_steps.py index 8177a525f..f2b53e88c 100644 --- a/features/steps/resource_cli_tree_steps.py +++ b/features/steps/resource_cli_tree_steps.py @@ -459,6 +459,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.""" -- 2.52.0 From 682902a11c57d89a49962e941b9356c52698f599 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 12 Apr 2026 04:15:07 +0000 Subject: [PATCH 5/8] fix(resource): ensure resource add remains atomic when discovery fails --- features/resource_cli.feature | 7 + features/steps/resource_cli_steps.py | 30 +++- .../services/_resource_registry_ops.py | 155 +++++++++--------- 3 files changed, 112 insertions(+), 80 deletions(-) diff --git a/features/resource_cli.feature b/features/resource_cli.feature index 55cac7a48..d55436f8a 100644 --- a/features/resource_cli.feature +++ b/features/resource_cli.feature @@ -86,6 +86,13 @@ Feature: Resource CLI commands 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/steps/resource_cli_steps.py b/features/steps/resource_cli_steps.py index 6b6a32df3..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 @@ -341,6 +345,18 @@ def step_set_resource_cli_format(context: Context, fmt: str) -> None: 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 ---- @@ -505,6 +521,18 @@ def step_resource_json_has_key(context: Context, key: str) -> None: 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") + + # --------------------------------------------------------------------------- # Additional steps for resource_cli_coverage.feature # --------------------------------------------------------------------------- diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index e93f8aa14..476b97b42 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -101,88 +101,85 @@ class ResourceInstanceMixin: """ session = self._session() try: - type_row = ( - session.query(ResourceTypeModel).filter_by(name=type_name).first() - ) - if type_row is None: - raise NotFoundError( - resource_type="resource_type", - resource_id=type_name, + with session.begin(): + type_row = ( + session.query(ResourceTypeModel).filter_by(name=type_name).first() + ) + if type_row is None: + raise NotFoundError( + resource_type="resource_type", + resource_id=type_name, + ) + + if not type_row.user_addable: + raise ValidationError( + f"Resource type '{type_name}' is not user-addable. " + "Only types with user_addable=true can be instantiated.", + ) + + resource_kind_str: str = str(type_row.resource_kind) + resource_id = str(ULID()) + + # Determine namespace from the resource name + namespace: str | None = None + if name is not None: + parts = name.split("/", 1) + if len(parts) == 2: + namespace = parts[0] + + now_iso = datetime.now(tz=UTC).isoformat() + properties_json: str | None = None + if properties: + properties_json = json.dumps(properties) + + db_resource = ResourceModel( + resource_id=resource_id, + namespaced_name=name, + namespace=namespace, + type_name=type_name, + resource_kind=resource_kind_str, + location=location, + description=description, + read_only=read_only, + auto_discovered=False, + sandbox_strategy=None, + content_hash=None, + properties_json=properties_json, + metadata_json=None, + created_at=now_iso, + updated_at=now_iso, ) - if not type_row.user_addable: - raise ValidationError( - f"Resource type '{type_name}' is not user-addable. " - "Only types with user_addable=true can be instantiated.", + session.add(db_resource) + session.flush() + + repository = ResourceRepository(self._session) + try: + repository.auto_discover_children( + resource_id, + session=session, + commit=False, + ) + except Exception as auto_exc: + logger.error( + "Auto-discovery failed; rolling back parent resource", + extra={"resource_id": resource_id, "type_name": type_name}, + ) + raise + + return Resource( + resource_id=resource_id, + name=name, + resource_type_name=type_name, + classification=PhysVirt(resource_kind_str), + description=description, + properties=properties or {}, + location=location, + content_hash=None, + sandbox_strategy=None, + created_at=datetime.fromisoformat(now_iso), + updated_at=datetime.fromisoformat(now_iso), ) - - resource_kind_str: str = str(type_row.resource_kind) - resource_id = str(ULID()) - - # Determine namespace from the resource name - namespace: str | None = None - if name is not None: - parts = name.split("/", 1) - if len(parts) == 2: - namespace = parts[0] - - now_iso = datetime.now(tz=UTC).isoformat() - properties_json: str | None = None - if properties: - properties_json = json.dumps(properties) - - db_resource = ResourceModel( - resource_id=resource_id, - namespaced_name=name, - namespace=namespace, - type_name=type_name, - resource_kind=resource_kind_str, - location=location, - description=description, - read_only=read_only, - auto_discovered=False, - sandbox_strategy=None, - content_hash=None, - properties_json=properties_json, - metadata_json=None, - created_at=now_iso, - updated_at=now_iso, - ) - - session.add(db_resource) - session.flush() - - repository = ResourceRepository(self._session) - try: - repository.auto_discover_children( - resource_id, - session=session, - commit=False, - ) - except Exception as auto_exc: - logger.error( - "Auto-discovery failed; rolling back parent resource", - extra={"resource_id": resource_id, "type_name": type_name}, - ) - session.rollback() - raise - - session.commit() - - resource = Resource( - resource_id=resource_id, - name=name, - resource_type_name=type_name, - classification=PhysVirt(resource_kind_str), - description=description, - properties=properties or {}, - location=location, - content_hash=None, - sandbox_strategy=None, - created_at=datetime.fromisoformat(now_iso), - updated_at=datetime.fromisoformat(now_iso), - ) - return resource except (NotFoundError, ValidationError): session.rollback() raise -- 2.52.0 From 7bcc212de587cb14a860e05014ab0148dd91a3de Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 5 May 2026 19:45:05 +0000 Subject: [PATCH 6/8] fix(resource): address reviewer feedback on auto-discovery atomicity - Always rollback session unconditionally in auto_discover_children except blocks (both ResourceNotFoundRepoError and OperationalError/ SQLAlchemyDatabaseError), regardless of commit/own_session flags. This ensures @database_retry retries with a clean session and callers continue to see the original DatabaseError instead of sqlalchemy.exc.PendingRollbackError. - Remove unused 'auto_exc' binding in register_resource's auto-discovery exception handler (use bare 'except Exception:' instead). - Move all 'from datetime import UTC, datetime' imports from inside function bodies to module-level in resource_registry_service_coverage_steps.py. ISSUES CLOSED: #6464 --- .../steps/resource_registry_service_coverage_steps.py | 9 +-------- .../application/services/_resource_registry_ops.py | 2 +- src/cleveragents/infrastructure/database/repositories.py | 6 ++---- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/features/steps/resource_registry_service_coverage_steps.py b/features/steps/resource_registry_service_coverage_steps.py index c57b0f057..9008a4086 100644 --- a/features/steps/resource_registry_service_coverage_steps.py +++ b/features/steps/resource_registry_service_coverage_steps.py @@ -11,6 +11,7 @@ from __future__ import annotations import json import tempfile +from datetime import UTC, datetime from typing import Any from unittest.mock import MagicMock, patch @@ -227,8 +228,6 @@ def step_auto_fail_type_exists(context: Any, type_name: str) -> None: namespace = type_name.split("/", 1)[0] if "/" in type_name else "local" session = context.cov_rt_factory() - from datetime import UTC, datetime - now_iso = datetime.now(tz=UTC).isoformat() row = ResourceTypeModel( name=type_name, @@ -395,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", @@ -449,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", @@ -567,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 476b97b42..c88c5898b 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -160,7 +160,7 @@ class ResourceInstanceMixin: session=session, commit=False, ) - except Exception as auto_exc: + except Exception: logger.error( "Auto-discovery failed; rolling back parent resource", extra={"resource_id": resource_id, "type_name": type_name}, diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 41f90eeb8..f840676c2 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -2825,15 +2825,13 @@ class ResourceRepository: session.commit() return created except ResourceNotFoundRepoError: - if commit or own_session: - session.rollback() + session.rollback() raise except ( OperationalError, SQLAlchemyDatabaseError, ) as exc: - if commit or own_session: - session.rollback() + session.rollback() raise DatabaseError( f"Failed to auto-discover children for '{resource_id}': {exc}" ) from exc -- 2.52.0 From bbf1915d54a70b0236ea48b8294d89520251e322 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 31 May 2026 19:21:50 -0400 Subject: [PATCH 7/8] fix(resource): preserve atomicity in register_resource without breaking shared-session callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous attempt wrapped `register_resource` in `with session.begin():` to guarantee parent + auto-discovered children commit atomically. That pattern raises `sqlalchemy.exc.InvalidRequestError: A transaction is already begun on this Session.` whenever a caller (e.g. the WF05 integration helper at `robot/helper_int_wf05_db_migration.py`) reuses a single Session across multiple service calls — autobegin has already opened the implicit transaction by the time `register_resource` runs. This rewrites the flow to keep the simpler `session.commit()` pattern that worked with shared sessions, but moves the commit to AFTER `auto_discover_children` so any failure between `session.add(parent)` and `session.commit()` rolls the whole transaction back via the existing `except`/`session.rollback()` handlers. Atomicity is preserved (parent is never persisted on an auto-discovery failure) and shared-session callers no longer get the `InvalidRequestError`. Also stabilises the `Service get_children returns auto-discovered children for directory` scenario in `features/resource_cli_tree.feature` by adding an explicit `Given a seeded directory exists at "/tmp/gcl"` step that creates the directory and writes a sentinel file. Without this seed the scenario depended on whatever happened to exist at `/tmp/gcl` in the CI environment. ISSUES CLOSED: #6464 --- features/resource_cli_tree.feature | 1 + features/steps/resource_cli_tree_steps.py | 15 ++ .../services/_resource_registry_ops.py | 163 ++++++++++-------- 3 files changed, 103 insertions(+), 76 deletions(-) diff --git a/features/resource_cli_tree.feature b/features/resource_cli_tree.feature index 917e538b0..08dfe3900 100644 --- a/features/resource_cli_tree.feature +++ b/features/resource_cli_tree.feature @@ -221,6 +221,7 @@ Feature: Resource CLI tree, inspect, link-child, and unlink-child commands 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 at least 1 child diff --git a/features/steps/resource_cli_tree_steps.py b/features/steps/resource_cli_tree_steps.py index f2b53e88c..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.""" diff --git a/src/cleveragents/application/services/_resource_registry_ops.py b/src/cleveragents/application/services/_resource_registry_ops.py index c88c5898b..f9965405b 100644 --- a/src/cleveragents/application/services/_resource_registry_ops.py +++ b/src/cleveragents/application/services/_resource_registry_ops.py @@ -101,85 +101,96 @@ class ResourceInstanceMixin: """ session = self._session() try: - with session.begin(): - type_row = ( - session.query(ResourceTypeModel).filter_by(name=type_name).first() - ) - if type_row is None: - raise NotFoundError( - resource_type="resource_type", - resource_id=type_name, - ) - - if not type_row.user_addable: - raise ValidationError( - f"Resource type '{type_name}' is not user-addable. " - "Only types with user_addable=true can be instantiated.", - ) - - resource_kind_str: str = str(type_row.resource_kind) - resource_id = str(ULID()) - - # Determine namespace from the resource name - namespace: str | None = None - if name is not None: - parts = name.split("/", 1) - if len(parts) == 2: - namespace = parts[0] - - now_iso = datetime.now(tz=UTC).isoformat() - properties_json: str | None = None - if properties: - properties_json = json.dumps(properties) - - db_resource = ResourceModel( - resource_id=resource_id, - namespaced_name=name, - namespace=namespace, - type_name=type_name, - resource_kind=resource_kind_str, - location=location, - description=description, - read_only=read_only, - auto_discovered=False, - sandbox_strategy=None, - content_hash=None, - properties_json=properties_json, - metadata_json=None, - created_at=now_iso, - updated_at=now_iso, + type_row = ( + session.query(ResourceTypeModel).filter_by(name=type_name).first() + ) + if type_row is None: + raise NotFoundError( + resource_type="resource_type", + resource_id=type_name, ) - session.add(db_resource) - session.flush() - - 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 - - return Resource( - resource_id=resource_id, - name=name, - resource_type_name=type_name, - classification=PhysVirt(resource_kind_str), - description=description, - properties=properties or {}, - location=location, - content_hash=None, - sandbox_strategy=None, - created_at=datetime.fromisoformat(now_iso), - updated_at=datetime.fromisoformat(now_iso), + if not type_row.user_addable: + raise ValidationError( + f"Resource type '{type_name}' is not user-addable. " + "Only types with user_addable=true can be instantiated.", ) + + resource_kind_str: str = str(type_row.resource_kind) + resource_id = str(ULID()) + + # Determine namespace from the resource name + namespace: str | None = None + if name is not None: + parts = name.split("/", 1) + if len(parts) == 2: + namespace = parts[0] + + now_iso = datetime.now(tz=UTC).isoformat() + properties_json: str | None = None + if properties: + properties_json = json.dumps(properties) + + db_resource = ResourceModel( + resource_id=resource_id, + namespaced_name=name, + namespace=namespace, + type_name=type_name, + resource_kind=resource_kind_str, + location=location, + description=description, + read_only=read_only, + auto_discovered=False, + sandbox_strategy=None, + content_hash=None, + properties_json=properties_json, + metadata_json=None, + created_at=now_iso, + updated_at=now_iso, + ) + + 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( + resource_id=resource_id, + name=name, + resource_type_name=type_name, + classification=PhysVirt(resource_kind_str), + description=description, + properties=properties or {}, + location=location, + content_hash=None, + sandbox_strategy=None, + created_at=datetime.fromisoformat(now_iso), + updated_at=datetime.fromisoformat(now_iso), + ) except (NotFoundError, ValidationError): session.rollback() raise -- 2.52.0 From ba8c4248971db33f893255717bd74b7d3c12b0f7 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 6 Jun 2026 03:40:12 -0400 Subject: [PATCH 8/8] test(resource-cli): cover auto-discovered children rich output path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a Behave scenario that adds an fs-directory resource at a seeded directory via the CLI. This exercises the if-children branch in resource_add (lines 857-887 of resource.py), including _short_resource_id, _format_child_status, and the Rich child table rendering — all previously uncovered because no existing scenario produced auto-discovered children through the resource add command path. ISSUES CLOSED: #6464 --- features/resource_cli.feature | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/features/resource_cli.feature b/features/resource_cli.feature index d55436f8a..0d937cbd7 100644 --- a/features/resource_cli.feature +++ b/features/resource_cli.feature @@ -81,6 +81,13 @@ Feature: Resource CLI commands 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 -- 2.52.0