diff --git a/CHANGELOG.md b/CHANGELOG.md index c3897172c..1a90c2322 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,14 @@ instances, health checks, and crash restart. `LanguageDiscovery` implements 4-layer detection (extension, shebang, UKO, project config). Includes 27 Behave BDD scenarios and 6 Robot integration tests. (#826) +- Added ResourceHandler CRUD and discovery methods: read, write, delete, + list_children, diff, and discover_children. Frozen dataclass result types + (Content, WriteResult, DeleteResult, DiffResult) added to the handler + protocol. GitCheckoutHandler implements all six methods via git plumbing + and filesystem operations. FsDirectoryHandler implements all six via + pathlib/os/difflib. DevcontainerHandler implements read, write, and + discover_children via `devcontainer exec`. DatabaseResourceHandler + inherits NotImplementedError stubs pending connection management. (#827) - Aligned plan lifecycle model with specification: ERRORED is now terminal in `is_terminal`, per-phase state validation enforces APPLIED/CONSTRAINED to APPLY-only and COMPLETE to @@ -73,7 +81,15 @@ `EXECUTE_TYPES`. Code relying on `is_strategize_type` or `is_execute_type` returning `False` for `resource_selection` will see different results. Reclassification aligns with ADR-007 L72 and ADR-033 L74 which permit - resource selection during planning. (#931) + resource selection during planning. (#931) +- Added ResourceHandler CRUD and discovery methods: read, write, delete, + list_children, diff, and discover_children. Frozen dataclass result types + (Content, WriteResult, DeleteResult, DiffResult) added to the handler + protocol. GitCheckoutHandler implements all six methods via git plumbing + and filesystem operations. FsDirectoryHandler implements all six via + pathlib/os/difflib. DevcontainerHandler implements read, write, and + discover_children via `devcontainer exec`. DatabaseResourceHandler + inherits NotImplementedError stubs pending connection management. (#827) - Added built-in deferred virtual resource types: `remote`, `submodule`, and `symlink` with equivalence metadata rules for cross-repo and cross-layer identity tracking. Registry bootstrap includes deferred virtual types but diff --git a/features/resource_handler_crud.feature b/features/resource_handler_crud.feature new file mode 100644 index 000000000..d11177257 --- /dev/null +++ b/features/resource_handler_crud.feature @@ -0,0 +1,181 @@ +Feature: Resource handler CRUD and discovery operations + Tests for read, write, delete, list_children, diff, and discover_children + methods on GitCheckoutHandler, FsDirectoryHandler, and base handler + NotImplementedError defaults. + + Issue #827: ResourceHandler CRUD and discovery methods. + + # ============================================================ + # FsDirectoryHandler CRUD + # ============================================================ + + Scenario: FsDirectory handler read returns file content + Given a temp directory with file "hello.txt" containing "Hello World" + And an fs-directory resource pointing to that directory + When I call read on the fs-directory handler with path "hello.txt" + Then the content data should be "Hello World" + And the content encoding should be "utf-8" + And the content hash should not be empty + + Scenario: FsDirectory handler read of root returns directory listing + Given a temp directory with file "a.txt" containing "aaa" + And a temp sub-file "b.txt" containing "bbb" + And an fs-directory resource pointing to that directory + When I call read on the fs-directory handler for the root + Then the content text should contain "a.txt" + And the content text should contain "b.txt" + + Scenario: FsDirectory handler read of missing path raises FileNotFoundError + Given a temp directory with file "only.txt" containing "data" + And an fs-directory resource pointing to that directory + When I call read on the fs-directory handler with path "missing.txt" + Then a crud FileNotFoundError should be raised + + Scenario: FsDirectory handler write creates a new file + Given an empty temp directory + And an fs-directory resource pointing to that directory + When I call write on the fs-directory handler with path "new.txt" and data "New Content" + Then the write result should be successful + And the write result bytes_written should be 11 + And the file "new.txt" should exist in the temp directory with content "New Content" + + Scenario: FsDirectory handler write creates parent directories + Given an empty temp directory + And an fs-directory resource pointing to that directory + When I call write on the fs-directory handler with path "sub/dir/file.txt" and data "Nested" + Then the write result should be successful + And the file "sub/dir/file.txt" should exist in the temp directory with content "Nested" + + Scenario: FsDirectory handler delete removes a file + Given a temp directory with file "doomed.txt" containing "bye" + And an fs-directory resource pointing to that directory + When I call delete on the fs-directory handler with path "doomed.txt" + Then the delete result should be successful + And the file "doomed.txt" should not exist in the temp directory + + Scenario: FsDirectory handler delete of missing path raises FileNotFoundError + Given an empty temp directory + And an fs-directory resource pointing to that directory + When I call delete on the fs-directory handler with path "ghost.txt" + Then a crud FileNotFoundError should be raised + + Scenario: FsDirectory handler list_children returns sorted entries + Given a temp directory with file "c.txt" containing "c" + And a temp sub-file "a.txt" containing "a" + And a temp sub-directory "subdir" + And an fs-directory resource pointing to that directory + When I call list_children on the fs-directory handler + Then the children list should equal ["a.txt", "c.txt", "subdir"] + + Scenario: FsDirectory handler diff detects changes between directories + Given a temp directory with file "same.txt" containing "same" + And a temp sub-file "changed.txt" containing "original" + And an fs-directory resource pointing to that directory + And a second temp directory with file "same.txt" containing "same" + And a second temp sub-file "changed.txt" containing "modified" + When I call diff on the fs-directory handler against the second directory + Then the diff result should have changes + And the diff files_changed should be 1 + And the diff insertions should be greater than 0 + + Scenario: FsDirectory handler diff detects no changes for identical directories + Given a temp directory with file "same.txt" containing "identical" + And an fs-directory resource pointing to that directory + And a second temp directory with file "same.txt" containing "identical" + When I call diff on the fs-directory handler against the second directory + Then the diff result should have no changes + + Scenario: FsDirectory handler discover_children returns subdirectories + Given a temp directory with file "file.txt" containing "data" + And a temp sub-directory "alpha" + And a temp sub-directory "beta" + And an fs-directory resource pointing to that directory + When I call discover_children on the fs-directory handler + Then the discovered children should have 2 items + And the discovered children names should include "alpha" + And the discovered children names should include "beta" + + # ============================================================ + # GitCheckoutHandler CRUD + # ============================================================ + + Scenario: GitCheckout handler read returns tracked file content + Given a temp git repo with file "readme.md" containing "# Hello" + And a git-checkout resource pointing to that repo + When I call read on the git-checkout handler with path "readme.md" + Then the content data should be "# Hello" + And the content hash should not be empty + + Scenario: GitCheckout handler write creates a new file in the repo + Given a temp git repo with file "initial.txt" containing "start" + And a git-checkout resource pointing to that repo + When I call write on the git-checkout handler with path "added.txt" and data "new file" + Then the write result should be successful + And the file "added.txt" should exist in the temp git repo with content "new file" + + Scenario: GitCheckout handler delete removes a tracked file + Given a temp git repo with file "remove_me.txt" containing "deletable" + And a git-checkout resource pointing to that repo + When I call delete on the git-checkout handler with path "remove_me.txt" + Then the delete result should be successful + And the file "remove_me.txt" should not exist in the temp git repo + + Scenario: GitCheckout handler list_children returns tracked entries + Given a temp git repo with file "a.py" containing "# a" + And a temp git sub-file "b.py" containing "# b" + And a git-checkout resource pointing to that repo + When I call list_children on the git-checkout handler + Then the children list should contain "a.py" + And the children list should contain "b.py" + + Scenario: GitCheckout handler diff detects changes against another location + Given a temp git repo with file "code.py" containing "print('hello')" + And a git-checkout resource pointing to that repo + And a second temp directory with file "code.py" containing "print('world')" + When I call diff on the git-checkout handler against the second directory + Then the diff result should have changes + + Scenario: GitCheckout handler discover_children returns top-level directories + Given a temp git repo with file "root.txt" containing "root" + And a temp git sub-directory "src" with file "main.py" containing "# main" + And a git-checkout resource pointing to that repo + When I call discover_children on the git-checkout handler + Then the discovered children names should include "src" + + # ============================================================ + # BaseResourceHandler NotImplementedError defaults + # ============================================================ + + Scenario: Database handler read raises NotImplementedError + Given a database resource handler + And a dummy database resource + When I call read on the database handler + Then a crud NotImplementedError should be raised with message containing "database" + + Scenario: Database handler write raises NotImplementedError + Given a database resource handler + And a dummy database resource + When I call write on the database handler with path "table" and data "row" + Then a crud NotImplementedError should be raised with message containing "database" + + # ============================================================ + # Security: path traversal rejection + # ============================================================ + + Scenario: FsDirectory handler read rejects path traversal + Given a temp directory with file "safe.txt" containing "safe" + And an fs-directory resource pointing to that directory + When I call read on the fs-directory handler with path "../../etc/passwd" + Then a crud PermissionError should be raised + + Scenario: FsDirectory handler write rejects path traversal + Given a temp directory with file "safe.txt" containing "safe" + And an fs-directory resource pointing to that directory + When I call write on the fs-directory handler with path "../../tmp/evil.txt" and data "pwned" + Then a crud PermissionError should be raised + + Scenario: GitCheckout handler read rejects path traversal + Given a temp git repo with file "safe.txt" containing "safe" + And a git-checkout resource pointing to that repo + When I call read on the git-checkout handler with path "../../etc/passwd" + Then a crud PermissionError should be raised diff --git a/features/steps/resource_handler_crud_steps.py b/features/steps/resource_handler_crud_steps.py new file mode 100644 index 000000000..365e57ba2 --- /dev/null +++ b/features/steps/resource_handler_crud_steps.py @@ -0,0 +1,459 @@ +"""Step definitions for resource_handler_crud.feature. + +Tests CRUD and discovery operations (read, write, delete, list_children, +diff, discover_children) for GitCheckoutHandler and FsDirectoryHandler. + +Issue #827: ResourceHandler CRUD and discovery methods. +""" + +from __future__ import annotations + +import subprocess +import tempfile +from pathlib import Path + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.resource import ( + PhysVirt, + Resource, + ResourceCapabilities, +) +from cleveragents.resource.handlers.database import DatabaseResourceHandler +from cleveragents.resource.handlers.fs_directory import FsDirectoryHandler +from cleveragents.resource.handlers.git_checkout import GitCheckoutHandler + +__all__: list[str] = [] + +_ULID_COUNTER = 0 + + +def _next_ulid() -> str: + """Generate a valid 26-char Crockford Base32 ID for tests.""" + global _ULID_COUNTER + _ULID_COUNTER += 1 + cb32 = "0123456789ABCDEFGHJKMNPQRSTVWXYZ" + n = _ULID_COUNTER + chars: list[str] = [] + for _ in range(26): + chars.append(cb32[n % 32]) + n //= 32 + return "".join(reversed(chars)) + + +def _make_resource(rtype: str, location: str, rid: str | None = None) -> Resource: + return Resource( + resource_id=rid or _next_ulid(), + resource_type_name=rtype, + classification=PhysVirt.PHYSICAL, + location=location, + capabilities=ResourceCapabilities( + readable=True, writable=True, sandboxable=True + ), + ) + + +# --------------------------------------------------------------------------- +# Given: temp directory setup +# --------------------------------------------------------------------------- + + +@given('a temp directory with file "{fname}" containing "{content}"') +def step_given_temp_dir_with_file(context: Context, fname: str, content: str) -> None: + context.crud_tmpdir = tempfile.mkdtemp(prefix="crud_fs_") + Path(context.crud_tmpdir, fname).write_text(content, encoding="utf-8") + + +@given('a temp sub-file "{fname}" containing "{content}"') +def step_given_temp_sub_file(context: Context, fname: str, content: str) -> None: + Path(context.crud_tmpdir, fname).write_text(content, encoding="utf-8") + + +@given('a temp sub-directory "{dirname}"') +def step_given_temp_sub_dir(context: Context, dirname: str) -> None: + Path(context.crud_tmpdir, dirname).mkdir(parents=True, exist_ok=True) + + +@given("an empty temp directory") +def step_given_empty_temp_dir(context: Context) -> None: + context.crud_tmpdir = tempfile.mkdtemp(prefix="crud_fs_") + + +@given("an fs-directory resource pointing to that directory") +def step_given_fs_resource(context: Context) -> None: + context.crud_handler = FsDirectoryHandler() + context.crud_resource = _make_resource("fs-directory", context.crud_tmpdir) + + +# -- Second temp directory for diff scenarios + + +@given('a second temp directory with file "{fname}" containing "{content}"') +def step_given_second_temp_dir(context: Context, fname: str, content: str) -> None: + context.crud_tmpdir2 = tempfile.mkdtemp(prefix="crud_fs2_") + Path(context.crud_tmpdir2, fname).write_text(content, encoding="utf-8") + + +@given('a second temp sub-file "{fname}" containing "{content}"') +def step_given_second_temp_sub_file(context: Context, fname: str, content: str) -> None: + Path(context.crud_tmpdir2, fname).write_text(content, encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Given: temp git repo setup +# --------------------------------------------------------------------------- + + +@given('a temp git repo with file "{fname}" containing "{content}"') +def step_given_temp_git_repo(context: Context, fname: str, content: str) -> None: + context.crud_tmpdir = tempfile.mkdtemp(prefix="crud_git_") + subprocess.run( + ["git", "init"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.email", "test@test.com"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + Path(context.crud_tmpdir, fname).write_text(content, encoding="utf-8") + subprocess.run( + ["git", "add", "."], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", "init"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + + +@given('a temp git sub-file "{fname}" containing "{content}"') +def step_given_temp_git_sub_file(context: Context, fname: str, content: str) -> None: + Path(context.crud_tmpdir, fname).write_text(content, encoding="utf-8") + subprocess.run( + ["git", "add", "."], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", f"add {fname}"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + + +@given( + 'a temp git sub-directory "{dirname}" with file "{fname}" containing "{content}"' +) +def step_given_temp_git_sub_dir( + context: Context, dirname: str, fname: str, content: str +) -> None: + dirpath = Path(context.crud_tmpdir, dirname) + dirpath.mkdir(parents=True, exist_ok=True) + (dirpath / fname).write_text(content, encoding="utf-8") + subprocess.run( + ["git", "add", "."], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", f"add {dirname}/{fname}"], + cwd=context.crud_tmpdir, + capture_output=True, + check=True, + ) + + +@given("a git-checkout resource pointing to that repo") +def step_given_git_resource(context: Context) -> None: + context.crud_handler = GitCheckoutHandler() + context.crud_resource = _make_resource("git-checkout", context.crud_tmpdir) + + +# --------------------------------------------------------------------------- +# Given: database handler (NotImplementedError tests) +# --------------------------------------------------------------------------- + + +@given("a database resource handler") +def step_given_db_handler(context: Context) -> None: + context.crud_handler = DatabaseResourceHandler() + + +@given("a dummy database resource") +def step_given_dummy_db_resource(context: Context) -> None: + context.crud_resource = _make_resource( + "postgres", "/tmp/fake-db", rid="01KJ5C5TPMP8GGX3QC83E2MAQS" + ) + + +# --------------------------------------------------------------------------- +# When: CRUD operations +# --------------------------------------------------------------------------- + + +@when('I call read on the fs-directory handler with path "{path}"') +@when('I call read on the git-checkout handler with path "{path}"') +def step_when_read(context: Context, path: str) -> None: + try: + context.crud_result = context.crud_handler.read( + resource=context.crud_resource, path=path + ) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when("I call read on the fs-directory handler for the root") +def step_when_read_root(context: Context) -> None: + try: + context.crud_result = context.crud_handler.read( + resource=context.crud_resource, path="" + ) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when("I call read on the database handler") +def step_when_read_db(context: Context) -> None: + try: + context.crud_result = context.crud_handler.read(resource=context.crud_resource) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when('I call write on the fs-directory handler with path "{path}" and data "{data}"') +@when('I call write on the git-checkout handler with path "{path}" and data "{data}"') +def step_when_write(context: Context, path: str, data: str) -> None: + try: + context.crud_result = context.crud_handler.write( + resource=context.crud_resource, + path=path, + data=data.encode("utf-8"), + ) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when('I call write on the database handler with path "{path}" and data "{data}"') +def step_when_write_db(context: Context, path: str, data: str) -> None: + try: + context.crud_result = context.crud_handler.write( + resource=context.crud_resource, + path=path, + data=data.encode("utf-8"), + ) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when('I call delete on the fs-directory handler with path "{path}"') +@when('I call delete on the git-checkout handler with path "{path}"') +def step_when_delete(context: Context, path: str) -> None: + try: + context.crud_result = context.crud_handler.delete( + resource=context.crud_resource, path=path + ) + context.crud_error = None + except Exception as exc: + context.crud_result = None + context.crud_error = exc + + +@when("I call list_children on the fs-directory handler") +@when("I call list_children on the git-checkout handler") +def step_when_list_children(context: Context) -> None: + context.crud_result = context.crud_handler.list_children( + resource=context.crud_resource + ) + + +@when("I call diff on the fs-directory handler against the second directory") +@when("I call diff on the git-checkout handler against the second directory") +def step_when_diff(context: Context) -> None: + context.crud_result = context.crud_handler.diff( + resource=context.crud_resource, + other_location=context.crud_tmpdir2, + ) + + +@when("I call discover_children on the fs-directory handler") +@when("I call discover_children on the git-checkout handler") +def step_when_discover(context: Context) -> None: + context.crud_result = context.crud_handler.discover_children( + resource=context.crud_resource + ) + + +# --------------------------------------------------------------------------- +# Then: assertions +# --------------------------------------------------------------------------- + + +@then('the content data should be "{expected}"') +def step_then_content_data(context: Context, expected: str) -> None: + assert context.crud_result is not None, "Expected Content, got None" + actual = context.crud_result.data.decode("utf-8").strip() + assert actual == expected, f"Expected '{expected}', got '{actual}'" + + +@then('the content encoding should be "{expected}"') +def step_then_content_encoding(context: Context, expected: str) -> None: + assert context.crud_result.encoding == expected + + +@then("the content hash should not be empty") +def step_then_content_hash_not_empty(context: Context) -> None: + assert context.crud_result.content_hash, "content_hash is empty" + + +@then('the content text should contain "{substring}"') +def step_then_content_text_contains(context: Context, substring: str) -> None: + text = context.crud_result.text + assert substring in text, f"'{substring}' not in '{text}'" + + +@then("a crud FileNotFoundError should be raised") +def step_then_crud_fnf_error(context: Context) -> None: + assert isinstance(context.crud_error, FileNotFoundError), ( + f"Expected FileNotFoundError, got {type(context.crud_error)}" + ) + + +@then("the write result should be successful") +def step_then_write_success(context: Context) -> None: + assert context.crud_result is not None + assert context.crud_result.success is True, ( + f"Write failed: {context.crud_result.message}" + ) + + +@then("the write result bytes_written should be {count:d}") +def step_then_write_bytes(context: Context, count: int) -> None: + assert context.crud_result.bytes_written == count + + +@then('the file "{path}" should exist in the temp directory with content "{expected}"') +def step_then_file_exists_with_content( + context: Context, path: str, expected: str +) -> None: + target = Path(context.crud_tmpdir) / path + assert target.exists(), f"{path} does not exist" + actual = target.read_text(encoding="utf-8") + assert actual == expected, f"Expected '{expected}', got '{actual}'" + + +@then('the file "{path}" should exist in the temp git repo with content "{expected}"') +def step_then_git_file_exists(context: Context, path: str, expected: str) -> None: + target = Path(context.crud_tmpdir) / path + assert target.exists(), f"{path} does not exist" + actual = target.read_text(encoding="utf-8") + assert actual == expected, f"Expected '{expected}', got '{actual}'" + + +@then("the delete result should be successful") +def step_then_delete_success(context: Context) -> None: + assert context.crud_result is not None + assert context.crud_result.success is True + + +@then('the file "{path}" should not exist in the temp directory') +@then('the file "{path}" should not exist in the temp git repo') +def step_then_file_not_exists(context: Context, path: str) -> None: + target = Path(context.crud_tmpdir) / path + assert not target.exists(), f"{path} still exists" + + +@then("the children list should equal {expected}") +def step_then_children_equal(context: Context, expected: str) -> None: + import ast + + expected_list = ast.literal_eval(expected) + assert context.crud_result == expected_list, ( + f"Expected {expected_list}, got {context.crud_result}" + ) + + +@then('the children list should contain "{item}"') +def step_then_children_contains(context: Context, item: str) -> None: + assert item in context.crud_result, f"'{item}' not in {context.crud_result}" + + +@then("the diff result should have changes") +def step_then_diff_has_changes(context: Context) -> None: + assert context.crud_result.has_changes is True + + +@then("the diff result should have no changes") +def step_then_diff_no_changes(context: Context) -> None: + assert context.crud_result.has_changes is False + + +@then("the diff files_changed should be {count:d}") +def step_then_diff_files_changed(context: Context, count: int) -> None: + assert context.crud_result.files_changed == count + + +@then("the diff insertions should be greater than {count:d}") +def step_then_diff_insertions_gt(context: Context, count: int) -> None: + assert context.crud_result.insertions > count, ( + f"insertions={context.crud_result.insertions} not > {count}" + ) + + +@then("the discovered children should have {count:d} items") +def step_then_discover_count(context: Context, count: int) -> None: + assert len(context.crud_result) == count, ( + f"Expected {count} children, got {len(context.crud_result)}" + ) + + +@then('the discovered children names should include "{name}"') +def step_then_discover_name(context: Context, name: str) -> None: + names = [r.name for r in context.crud_result] + assert name in names, f"'{name}' not in {names}" + + +@then( + 'a crud NotImplementedError should be raised with message containing "{fragment}"' +) +def step_then_crud_not_implemented(context: Context, fragment: str) -> None: + assert isinstance(context.crud_error, NotImplementedError), ( + f"Expected NotImplementedError, got {type(context.crud_error)}" + ) + assert fragment in str(context.crud_error), ( + f"'{fragment}' not in '{context.crud_error}'" + ) + + +@then("a crud PermissionError should be raised") +def step_then_crud_permission_error(context: Context) -> None: + assert isinstance(context.crud_error, PermissionError), ( + f"Expected PermissionError, got {type(context.crud_error)}: {context.crud_error}" + ) diff --git a/robot/helper_resource_handler_crud.py b/robot/helper_resource_handler_crud.py new file mode 100644 index 000000000..cb53ae094 --- /dev/null +++ b/robot/helper_resource_handler_crud.py @@ -0,0 +1,181 @@ +"""Helper script for resource_handler_crud.robot integration tests. + +Exercises the read -> write -> diff cycle on a real temporary git +repository using GitCheckoutHandler, and on a real temporary directory +using FsDirectoryHandler. + +Issue #827: ResourceHandler CRUD and discovery methods. +""" + +from __future__ import annotations + +import subprocess +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import NoReturn + +_ROOT = Path(__file__).resolve().parents[1] +_SRC = str(_ROOT / "src") +# Ensure local source takes precedence over installed package. +# Remove any existing cleveragents modules so importlib re-resolves +# from sys.path[0] (our local src). +sys.path.insert(0, _SRC) +for _mod_name in list(sys.modules): + if _mod_name.startswith("cleveragents"): + del sys.modules[_mod_name] + +from cleveragents.domain.models.core.resource import ( # noqa: E402 + PhysVirt, + Resource, + ResourceCapabilities, +) +from cleveragents.resource.handlers.fs_directory import FsDirectoryHandler # noqa: E402 +from cleveragents.resource.handlers.git_checkout import GitCheckoutHandler # noqa: E402 + + +def _fail(msg: str) -> NoReturn: + print(msg, file=sys.stderr) + sys.exit(1) + + +def _make_resource(rtype: str, location: str) -> Resource: + return Resource( + resource_id="01KJ5C5TPMP8GGX3QC83E2MAQS", + resource_type_name=rtype, + classification=PhysVirt.PHYSICAL, + location=location, + capabilities=ResourceCapabilities( + readable=True, writable=True, sandboxable=True + ), + ) + + +# --------------------------------------------------------------------------- +# fs-read-write-diff: FsDirectoryHandler read -> write -> diff cycle +# --------------------------------------------------------------------------- + + +def fs_read_write_diff() -> None: + """Test read -> write -> diff cycle on FsDirectoryHandler.""" + handler = FsDirectoryHandler() + + # Create two temp dirs: source and comparison + src = tempfile.mkdtemp(prefix="crud_robot_src_") + cmp_dir = tempfile.mkdtemp(prefix="crud_robot_cmp_") + + # Write identical files to both + Path(src, "file.txt").write_text("original\n", encoding="utf-8") + Path(cmp_dir, "file.txt").write_text("original\n", encoding="utf-8") + + resource = _make_resource("fs-directory", src) + + # 1. Read + content = handler.read(resource=resource, path="file.txt") + if content.text.strip() != "original": + _fail(f"read failed: expected 'original', got '{content.text.strip()}'") + + # 2. Write (modify) + result = handler.write(resource=resource, path="file.txt", data=b"modified\n") + if not result.success: + _fail(f"write failed: {result.message}") + + # 3. Diff against comparison dir (should detect change) + diff = handler.diff(resource=resource, other_location=cmp_dir) + if not diff.has_changes: + _fail("diff should detect changes after write") + if diff.files_changed < 1: + _fail(f"diff files_changed={diff.files_changed}, expected >= 1") + + # 4. List children + children = handler.list_children(resource=resource) + if "file.txt" not in children: + _fail(f"list_children missing 'file.txt': {children}") + + print("fs-read-write-diff-ok") + + +# --------------------------------------------------------------------------- +# git-read-write-diff: GitCheckoutHandler read -> write -> diff cycle +# --------------------------------------------------------------------------- + + +def git_read_write_diff() -> None: + """Test read -> write -> diff cycle on GitCheckoutHandler.""" + handler = GitCheckoutHandler() + + # Create a temp git repo + repo = tempfile.mkdtemp(prefix="crud_robot_git_") + subprocess.run(["git", "init"], cwd=repo, capture_output=True, check=True) + subprocess.run( + ["git", "config", "user.email", "test@test.com"], + cwd=repo, + capture_output=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test"], + cwd=repo, + capture_output=True, + check=True, + ) + Path(repo, "code.py").write_text("print('hello')\n", encoding="utf-8") + subprocess.run(["git", "add", "."], cwd=repo, capture_output=True, check=True) + subprocess.run( + ["git", "commit", "-m", "init"], + cwd=repo, + capture_output=True, + check=True, + ) + + resource = _make_resource("git-checkout", repo) + + # Comparison dir + cmp_dir = tempfile.mkdtemp(prefix="crud_robot_cmp_") + Path(cmp_dir, "code.py").write_text("print('hello')\n", encoding="utf-8") + + # 1. Read + content = handler.read(resource=resource, path="code.py") + if "hello" not in content.text: + _fail(f"read failed: 'hello' not in '{content.text}'") + + # 2. Write + result = handler.write(resource=resource, path="code.py", data=b"print('world')\n") + if not result.success: + _fail(f"write failed: {result.message}") + + # 3. Diff (should detect change since we modified the file) + diff = handler.diff(resource=resource, other_location=cmp_dir) + if not diff.has_changes: + _fail("diff should detect changes after write") + + # 4. List children + children = handler.list_children(resource=resource) + if "code.py" not in children: + _fail(f"list_children missing 'code.py': {children}") + + # 5. Discover children (no subdirs expected) + handler.discover_children(resource=resource) + # May or may not have children — just ensure no crash + + print("git-read-write-diff-ok") + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "fs-read-write-diff": fs_read_write_diff, + "git-read-write-diff": git_read_write_diff, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + _COMMANDS[sys.argv[1]]() diff --git a/robot/resource_handler_crud.robot b/robot/resource_handler_crud.robot new file mode 100644 index 000000000..0cb4abcbb --- /dev/null +++ b/robot/resource_handler_crud.robot @@ -0,0 +1,28 @@ +*** Settings *** +Documentation Integration tests for ResourceHandler CRUD operations +... Exercises the read -> write -> diff cycle on real +... temporary directories and git repos. +... Issue #827: ResourceHandler CRUD and discovery methods. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_resource_handler_crud.py + +*** Test Cases *** +FsDirectory Read Write Diff Cycle + [Documentation] FsDirectoryHandler: read original, write modified, diff detects change + ${result}= Run Process ${PYTHON} ${HELPER} fs-read-write-diff cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} fs-read-write-diff-ok + +GitCheckout Read Write Diff Cycle + [Documentation] GitCheckoutHandler: read tracked file, write change, diff detects change + ${result}= Run Process ${PYTHON} ${HELPER} git-read-write-diff cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} git-read-write-diff-ok diff --git a/src/cleveragents/application/services/resource_handler_service.py b/src/cleveragents/application/services/resource_handler_service.py index e09a816ce..a4a7bc338 100644 --- a/src/cleveragents/application/services/resource_handler_service.py +++ b/src/cleveragents/application/services/resource_handler_service.py @@ -34,7 +34,13 @@ from cleveragents.domain.models.core.resource_slot import BindingResult from cleveragents.domain.models.core.resource_type import ResourceTypeSpec from cleveragents.infrastructure.sandbox.factory import SandboxStrategyStr from cleveragents.infrastructure.sandbox.manager import SandboxManager -from cleveragents.resource.handlers.protocol import ResourceHandler +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + ResourceHandler, + WriteResult, +) from cleveragents.resource.handlers.resolver import ( HandlerResolutionError, resolve_handler, @@ -251,12 +257,43 @@ class _DefaultHandler: """Fallback handler when no handler class is specified or resolution fails. Delegates directly to :class:`SandboxManager` using the type's - default sandbox strategy (or resource override). + default sandbox strategy (or resource override). CRUD and + discovery methods raise :class:`NotImplementedError`. """ def __init__(self, type_spec: ResourceTypeSpec) -> None: self._type_spec = type_spec + # -- CRUD stubs (required by ResourceHandler protocol) ----------------- + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Not supported by the default handler.""" + raise NotImplementedError("Default handler does not support read()") + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Not supported by the default handler.""" + raise NotImplementedError("Default handler does not support write()") + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Not supported by the default handler.""" + raise NotImplementedError("Default handler does not support delete()") + + def list_children(self, *, resource: Resource) -> list[str]: + """Not supported by the default handler.""" + raise NotImplementedError("Default handler does not support list_children()") + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Not supported by the default handler.""" + raise NotImplementedError("Default handler does not support diff()") + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Not supported by the default handler.""" + raise NotImplementedError( + "Default handler does not support discover_children()" + ) + + # -- Sandbox resolution ------------------------------------------------ + def resolve( self, *, diff --git a/src/cleveragents/resource/handlers/__init__.py b/src/cleveragents/resource/handlers/__init__.py index 56ada49e6..544606e4f 100644 --- a/src/cleveragents/resource/handlers/__init__.py +++ b/src/cleveragents/resource/handlers/__init__.py @@ -34,7 +34,13 @@ from cleveragents.resource.handlers.database import DatabaseResourceHandler from cleveragents.resource.handlers.devcontainer import DevcontainerHandler from cleveragents.resource.handlers.fs_directory import FsDirectoryHandler from cleveragents.resource.handlers.git_checkout import GitCheckoutHandler -from cleveragents.resource.handlers.protocol import ResourceHandler +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + ResourceHandler, + WriteResult, +) from cleveragents.resource.handlers.resolver import ( HandlerResolutionError, resolve_handler, @@ -42,11 +48,15 @@ from cleveragents.resource.handlers.resolver import ( __all__ = [ "CloudResourceHandler", + "Content", "DatabaseResourceHandler", + "DeleteResult", "DevcontainerHandler", + "DiffResult", "FsDirectoryHandler", "GitCheckoutHandler", "HandlerResolutionError", "ResourceHandler", + "WriteResult", "resolve_handler", ] diff --git a/src/cleveragents/resource/handlers/_base.py b/src/cleveragents/resource/handlers/_base.py index 01770fdd0..a88a06b79 100644 --- a/src/cleveragents/resource/handlers/_base.py +++ b/src/cleveragents/resource/handlers/_base.py @@ -4,23 +4,56 @@ Provides the common ``resolve`` logic used by both :class:`GitCheckoutHandler` and :class:`FsDirectoryHandler`. Subclasses set ``_default_strategy`` and ``_type_label`` to customise behaviour. + +Also provides default (``NotImplementedError``) implementations for +the CRUD and discovery methods added by issue #827. Concrete +handlers override the methods they support. """ from __future__ import annotations +import hashlib import logging +import os +from pathlib import Path from typing import ClassVar, cast from cleveragents.domain.models.core.resource import Resource, SandboxStrategy from cleveragents.infrastructure.sandbox.factory import SandboxStrategyStr from cleveragents.infrastructure.sandbox.manager import SandboxManager +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + WriteResult, +) from cleveragents.tool.context import BoundResource logger = logging.getLogger(__name__) +# Crockford Base32 alphabet (excludes I, L, O, U) +_CB32 = "0123456789ABCDEFGHJKMNPQRSTVWXYZ" + + +def _derive_child_id(parent_id: str, child_name: str) -> str: + """Derive a valid 26-char Crockford Base32 child resource ID. + + Uses a SHA-256 hash of ``parent_id + "/" + child_name``, encoded + in Crockford Base32 to satisfy the ULID pattern constraint on + :class:`Resource.resource_id`. + """ + digest = hashlib.sha256(f"{parent_id}/{child_name}".encode()).digest() + # Take first 16 bytes (128 bits) and encode as 26 Crockford chars + n = int.from_bytes(digest[:16], "big") + chars: list[str] = [] + for _ in range(26): + chars.append(_CB32[n % 32]) + n //= 32 + return "".join(reversed(chars)) + class BaseResourceHandler: - """Base handler with shared resolve logic. + """Base handler with shared resolve logic and CRUD defaults. Subclasses **must** set: @@ -28,11 +61,18 @@ class BaseResourceHandler: the resource has no explicit override. * ``_type_label`` — a human-readable label for error messages (e.g. ``"git-checkout"``). + + Subclasses **may** override the CRUD and discovery methods + (``read``, ``write``, ``delete``, ``list_children``, ``diff``, + ``discover_children``) to provide handler-specific behaviour. + The defaults raise :class:`NotImplementedError`. """ _default_strategy: ClassVar[SandboxStrategy] _type_label: ClassVar[str] + # -- Sandbox resolution (M1) ------------------------------------------- + def resolve( self, *, @@ -97,3 +137,126 @@ class BaseResourceHandler: sandbox_path=sandbox.context.sandbox_path, access=access, ) + + # -- Content CRUD (M6, issue #827) ------------------------------------- + + @staticmethod + def _derive_child_id(parent_id: str, child_name: str) -> str: + """Derive a valid 26-char Crockford Base32 child ID. + + Uses a SHA-256 hash of parent + child name, encoded in + Crockford Base32 to satisfy the ULID pattern constraint. + """ + return _derive_child_id(parent_id, child_name) + + def _require_location(self, resource: Resource) -> str: + """Return *resource.location* or raise ``ValueError``.""" + if not resource.location: + raise ValueError( + f"{self._type_label} resource '{resource.resource_id}' has no location" + ) + return resource.location + + @staticmethod + def _safe_resolve(location: str, path: str) -> Path: + """Resolve *path* within *location*, rejecting traversal escapes. + + Args: + location: The root directory. + path: Relative path within the root. + + Returns: + The resolved :class:`Path`. + + Raises: + PermissionError: If *path* escapes the root via ``..`` or + symlink resolution. + """ + root = Path(location).resolve() + target = (root / path).resolve() + # Use root + os.sep to prevent prefix collision bypass: + # e.g. root=/tmp/foo must not match target=/tmp/foobar/secret + if target != root and not str(target).startswith(str(root) + os.sep): + raise PermissionError(f"Path '{path}' escapes resource root '{location}'") + return target + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Read content from a resource. + + Args: + resource: The resource to read from. + path: Relative path within the resource. + + Raises: + NotImplementedError: Subclass has not implemented ``read``. + """ + raise NotImplementedError(f"{self._type_label} handler does not support read()") + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Write content to a resource. + + Args: + resource: The resource to write to. + path: Relative path within the resource. + data: Raw bytes to write. + + Raises: + NotImplementedError: Subclass has not implemented ``write``. + """ + raise NotImplementedError( + f"{self._type_label} handler does not support write()" + ) + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Delete a resource or a path within it. + + Args: + resource: The resource to delete from. + path: Relative path within the resource. + + Raises: + NotImplementedError: Subclass has not implemented ``delete``. + """ + raise NotImplementedError( + f"{self._type_label} handler does not support delete()" + ) + + def list_children(self, *, resource: Resource) -> list[str]: + """List direct children of a resource. + + Args: + resource: The parent resource. + + Raises: + NotImplementedError: Subclass has not implemented + ``list_children``. + """ + raise NotImplementedError( + f"{self._type_label} handler does not support list_children()" + ) + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Compare a resource against another location. + + Args: + resource: The resource whose current state to compare. + other_location: Filesystem path to compare against. + + Raises: + NotImplementedError: Subclass has not implemented ``diff``. + """ + raise NotImplementedError(f"{self._type_label} handler does not support diff()") + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Auto-discover child resources beneath this resource. + + Args: + resource: The parent resource to scan. + + Raises: + NotImplementedError: Subclass has not implemented + ``discover_children``. + """ + raise NotImplementedError( + f"{self._type_label} handler does not support discover_children()" + ) diff --git a/src/cleveragents/resource/handlers/cloud.py b/src/cleveragents/resource/handlers/cloud.py index 3a0844876..989aeeb29 100644 --- a/src/cleveragents/resource/handlers/cloud.py +++ b/src/cleveragents/resource/handlers/cloud.py @@ -49,6 +49,12 @@ from typing import Any from cleveragents.domain.models.core.resource import Resource from cleveragents.infrastructure.sandbox.manager import SandboxManager +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + WriteResult, +) from cleveragents.shared.redaction import REDACTED, is_sensitive_key from cleveragents.tool.context import BoundResource @@ -467,6 +473,32 @@ class CloudResourceHandler: f"but sandbox provisioning for cloud resources is pending." ) + # -- CRUD stubs (required by ResourceHandler protocol) ----------------- + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support read()") + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support write()") + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support delete()") + + def list_children(self, *, resource: Resource) -> list[str]: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support list_children()") + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support diff()") + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Not supported by the cloud handler.""" + raise NotImplementedError("Cloud handler does not support discover_children()") + # --------------------------------------------------------------------------- # Stubbed sandbox strategies diff --git a/src/cleveragents/resource/handlers/devcontainer.py b/src/cleveragents/resource/handlers/devcontainer.py index 89c319084..abee18ad9 100644 --- a/src/cleveragents/resource/handlers/devcontainer.py +++ b/src/cleveragents/resource/handlers/devcontainer.py @@ -46,6 +46,7 @@ Based on: from __future__ import annotations import logging +import subprocess from cleveragents.domain.models.core.container_lifecycle import ( ContainerLifecycleState, @@ -83,6 +84,7 @@ from cleveragents.resource.handlers.devcontainer_lifecycle import ( rebuild_container, stop_container, ) +from cleveragents.resource.handlers.protocol import Content, WriteResult from cleveragents.tool.context import BoundResource logger = logging.getLogger(__name__) @@ -199,3 +201,148 @@ class DevcontainerHandler(BaseResourceHandler): sandbox_manager=sandbox_manager, access=access, ) + + # -- Content CRUD (issue #827) ----------------------------------------- + # Devcontainer content operations delegate to ``devcontainer exec`` + # where possible. The container must be in RUNNING state. + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Read a file from a running devcontainer via ``devcontainer exec cat``. + + Args: + resource: The devcontainer resource. + path: Path inside the container. + + Returns: + A :class:`Content` with the file bytes. + + Raises: + NotImplementedError: If the resource has no location (cannot + exec into the container). + """ + location = self._require_location(resource) + if not path: + return self._exec_ls(resource, location, ".") + + result = subprocess.run( + ["devcontainer", "exec", "--workspace-folder", location, "cat", path], + capture_output=True, + timeout=30, + check=False, + ) + if result.returncode != 0: + raise FileNotFoundError( + f"Path '{path}' not found in devcontainer at '{location}': " + f"{result.stderr.decode(errors='replace').strip()}" + ) + return Content(data=result.stdout, encoding="utf-8") + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Write a file into a running devcontainer. + + Uses ``devcontainer exec tee`` to write the bytes. + + Args: + resource: The devcontainer resource. + path: Path inside the container. + data: Raw bytes to write. + + Returns: + A :class:`WriteResult` indicating success. + """ + + location = self._require_location(resource) + result = subprocess.run( + [ + "devcontainer", + "exec", + "--workspace-folder", + location, + "tee", + path, + ], + input=data, + capture_output=True, + timeout=30, + check=False, + ) + if result.returncode != 0: + return WriteResult( + success=False, + message=( + f"Write to devcontainer failed: " + f"{result.stderr.decode(errors='replace').strip()}" + ), + ) + return WriteResult( + success=True, + bytes_written=len(data), + message=f"Wrote {len(data)} bytes to {path} in devcontainer", + ) + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Discover directories inside a running devcontainer via ``ls``. + + Args: + resource: The devcontainer resource. + + Returns: + List of child :class:`Resource` objects for top-level + directories in the workspace. + """ + + location = self._require_location(resource) + result = subprocess.run( + [ + "devcontainer", + "exec", + "--workspace-folder", + location, + "ls", + "-1", + ], + capture_output=True, + text=True, + timeout=30, + check=False, + ) + children: list[Resource] = [] + if result.returncode != 0: + return children + + for name in sorted(result.stdout.strip().split("\n")): + name = name.strip() + if not name: + continue + children.append( + Resource( + resource_id=self._derive_child_id(resource.resource_id, name), + name=name, + resource_type_name="fs-directory", + classification=resource.classification, + description=f"'{name}' in devcontainer {resource.resource_id}", + location=f"{location}/{name}", + parents=[resource.resource_id], + ) + ) + return children + + def _exec_ls(self, resource: Resource, location: str, path: str) -> Content: + """Run ``ls`` inside the container and return as Content.""" + + result = subprocess.run( + [ + "devcontainer", + "exec", + "--workspace-folder", + location, + "ls", + "-1", + path, + ], + capture_output=True, + timeout=30, + check=False, + ) + data = result.stdout if result.returncode == 0 else b"" + return Content(data=data, encoding="utf-8") diff --git a/src/cleveragents/resource/handlers/fs_directory.py b/src/cleveragents/resource/handlers/fs_directory.py index 78f1caac4..91e809b71 100644 --- a/src/cleveragents/resource/handlers/fs_directory.py +++ b/src/cleveragents/resource/handlers/fs_directory.py @@ -4,32 +4,255 @@ Resolves ``fs-directory`` resources into sandbox-backed :class:`BoundResource` instances using the ``copy_on_write`` sandbox strategy. -The handler: +Content CRUD operations (issue #827): -1. Validates that the resource has a non-empty ``location``. -2. Determines the sandbox strategy: resource-level override takes - precedence over the default ``copy_on_write``. -3. Calls :meth:`SandboxManager.get_or_create_sandbox` to provision - (or reuse) an isolated copy-on-write directory. -4. Returns a :class:`BoundResource` with ``sandbox_path`` set to the - sandbox root. +- ``read`` — direct filesystem read via :mod:`pathlib` +- ``write`` — atomic file write with parent directory creation +- ``delete`` — ``os.remove`` / ``shutil.rmtree`` +- ``list_children`` — ``os.listdir`` (top-level entries) +- ``diff`` — unified diff via :mod:`difflib` +- ``discover_children`` — ``os.scandir`` for subdirectories Based on: - implementation_plan.md group M1.resource-handlers (L2254-L2271) - Built-in type definition in resource_registry_service.py L99-123 + - Issue #827 — ResourceHandler CRUD and discovery methods """ from __future__ import annotations -from cleveragents.domain.models.core.resource import SandboxStrategy +import difflib +import hashlib +import logging +import os +import shutil +from pathlib import Path + +from cleveragents.domain.models.core.resource import ( + Resource, + SandboxStrategy, +) from cleveragents.resource.handlers._base import BaseResourceHandler +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + WriteResult, +) + +logger = logging.getLogger(__name__) class FsDirectoryHandler(BaseResourceHandler): """Handler for ``fs-directory`` resource types. Provisions a copy-on-write sandbox for a local filesystem directory. + Implements full content CRUD via standard filesystem operations. """ _default_strategy = SandboxStrategy.COPY_ON_WRITE _type_label = "fs-directory" + + # -- Content CRUD (issue #827) ----------------------------------------- + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Read a file or directory listing from the filesystem. + + Args: + resource: The fs-directory resource. + path: Relative path within the directory (empty reads the + directory listing). + + Returns: + A :class:`Content` with the file bytes or directory listing. + + Raises: + FileNotFoundError: If *path* does not exist. + """ + location = self._require_location(resource) + target = ( + self._safe_resolve(location, path) if path else Path(location).resolve() + ) + + if not target.exists(): + raise FileNotFoundError( + f"Path '{path}' not found in fs-directory at '{location}'" + ) + + if target.is_dir(): + children = sorted(os.listdir(target)) + data = "\n".join(children).encode("utf-8") + return Content(data=data, encoding="utf-8") + + data = target.read_bytes() + sha = hashlib.sha256(data).hexdigest() + return Content(data=data, encoding="utf-8", content_hash=sha) + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Write a file into the directory. + + Creates parent directories as needed. + + Args: + resource: The fs-directory resource. + path: Relative path within the directory. + data: Raw bytes to write. + + Returns: + A :class:`WriteResult` indicating success. + """ + location = self._require_location(resource) + target = self._safe_resolve(location, path) + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(data) + sha = hashlib.sha256(data).hexdigest() + return WriteResult( + success=True, + bytes_written=len(data), + content_hash=sha, + message=f"Wrote {len(data)} bytes to {path}", + ) + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Delete a file or subdirectory. + + Args: + resource: The fs-directory resource. + path: Relative path within the directory. + + Returns: + A :class:`DeleteResult` indicating success. + + Raises: + FileNotFoundError: If *path* does not exist. + """ + if not path: + raise PermissionError("Cannot delete the resource root (empty path)") + location = self._require_location(resource) + target = self._safe_resolve(location, path) + + if not target.exists(): + raise FileNotFoundError( + f"Path '{path}' not found in fs-directory at '{location}'" + ) + + if target.is_dir(): + shutil.rmtree(target) + else: + target.unlink() + + return DeleteResult(success=True, message=f"Deleted {path}") + + def list_children(self, *, resource: Resource) -> list[str]: + """List top-level entries in the directory. + + Args: + resource: The fs-directory resource. + + Returns: + Sorted list of entry names. + """ + location = self._require_location(resource) + return sorted(os.listdir(location)) + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Compare the directory against another location. + + Performs a file-by-file unified diff of all files in both + directories. + + Args: + resource: The fs-directory resource. + other_location: Filesystem path to compare against. + + Returns: + A :class:`DiffResult` summarising the differences. + """ + location = self._require_location(resource) + a_path = Path(location) + b_path = Path(other_location) + + # Collect all files from both sides + a_files = {str(p.relative_to(a_path)) for p in a_path.rglob("*") if p.is_file()} + b_files = {str(p.relative_to(b_path)) for p in b_path.rglob("*") if p.is_file()} + all_files = sorted(a_files | b_files) + + unified_parts: list[str] = [] + files_changed = 0 + total_insertions = 0 + total_deletions = 0 + + for rel in all_files: + a_file = a_path / rel + b_file = b_path / rel + + a_lines = ( + a_file.read_text(encoding="utf-8", errors="replace").splitlines( + keepends=True + ) + if a_file.exists() + else [] + ) + b_lines = ( + b_file.read_text(encoding="utf-8", errors="replace").splitlines( + keepends=True + ) + if b_file.exists() + else [] + ) + + diff_lines = list( + difflib.unified_diff( + a_lines, + b_lines, + fromfile=f"a/{rel}", + tofile=f"b/{rel}", + ) + ) + if diff_lines: + files_changed += 1 + unified_parts.extend(diff_lines) + for line in diff_lines: + if line.startswith("+") and not line.startswith("+++"): + total_insertions += 1 + elif line.startswith("-") and not line.startswith("---"): + total_deletions += 1 + + return DiffResult( + has_changes=files_changed > 0, + unified_diff="".join(unified_parts), + files_changed=files_changed, + insertions=total_insertions, + deletions=total_deletions, + ) + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Discover subdirectories as child resources. + + Each immediate subdirectory becomes a child ``fs-directory`` + resource. + + Args: + resource: The parent fs-directory resource. + + Returns: + List of child :class:`Resource` objects. + """ + location = self._require_location(resource) + children: list[Resource] = [] + + for entry in sorted(os.scandir(location), key=lambda e: e.name): + if not entry.is_dir(): + continue + child = Resource( + resource_id=self._derive_child_id(resource.resource_id, entry.name), + name=entry.name, + resource_type_name="fs-directory", + classification=resource.classification, + description=(f"Directory '{entry.name}' in {resource.resource_id}"), + location=entry.path, + parents=[resource.resource_id], + ) + children.append(child) + + return children diff --git a/src/cleveragents/resource/handlers/git_checkout.py b/src/cleveragents/resource/handlers/git_checkout.py index 169ce902e..b82881e77 100644 --- a/src/cleveragents/resource/handlers/git_checkout.py +++ b/src/cleveragents/resource/handlers/git_checkout.py @@ -4,33 +4,340 @@ Resolves ``git-checkout`` resources into sandbox-backed :class:`BoundResource` instances using the ``git_worktree`` sandbox strategy (with fallback to ``copy_on_write``). -The handler: +Content CRUD operations (issue #827): -1. Validates that the resource has a non-empty ``location``. -2. Determines the sandbox strategy: resource-level override takes - precedence over the default ``git_worktree``. -3. Calls :meth:`SandboxManager.get_or_create_sandbox` to provision - (or reuse) an isolated git worktree. -4. Returns a :class:`BoundResource` with ``sandbox_path`` set to the - sandbox root. +- ``read`` — ``git show HEAD:`` +- ``write`` — atomic file write inside the checkout +- ``delete`` — ``os.remove`` + ``git rm --cached`` +- ``list_children`` — ``git ls-tree -r --name-only HEAD`` +- ``diff`` — ``git diff --no-index`` +- ``discover_children`` — ``git ls-tree --name-only HEAD`` Based on: - implementation_plan.md group M1.resource-handlers (L2254-L2271) - Built-in type definition in resource_registry_service.py L62-98 + - Issue #827 — ResourceHandler CRUD and discovery methods """ from __future__ import annotations -from cleveragents.domain.models.core.resource import SandboxStrategy +import hashlib +import logging +import os +import subprocess +from pathlib import Path + +from cleveragents.domain.models.core.resource import ( + Resource, + SandboxStrategy, +) from cleveragents.resource.handlers._base import BaseResourceHandler +from cleveragents.resource.handlers.protocol import ( + Content, + DeleteResult, + DiffResult, + WriteResult, +) + +logger = logging.getLogger(__name__) class GitCheckoutHandler(BaseResourceHandler): """Handler for ``git-checkout`` resource types. Provisions a git-worktree sandbox (or copy-on-write fallback) - for a git repository checkout. + for a git repository checkout. Implements content CRUD via git + plumbing commands where appropriate, falling back to filesystem + operations for write/delete. """ _default_strategy = SandboxStrategy.GIT_WORKTREE _type_label = "git-checkout" + + # -- Content CRUD (issue #827) ----------------------------------------- + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Read a file from the git checkout using ``git show``. + + Falls back to direct filesystem read if ``git show`` fails + (e.g. for untracked files). + + Args: + resource: The git-checkout resource. + path: Relative path within the repo (empty reads repo root + listing). + + Returns: + A :class:`Content` with the file bytes. + + Raises: + FileNotFoundError: If *path* does not exist in the checkout. + """ + location = self._require_location(resource) + + if not path: + children = self.list_children(resource=resource) + data = "\n".join(children).encode("utf-8") + return Content(data=data, encoding="utf-8") + + # Validate path doesn't escape root + target = self._safe_resolve(location, path) + + # Try git show first (works for tracked files) + try: + result = subprocess.run( + ["git", "show", f"HEAD:{path}"], + cwd=location, + capture_output=True, + timeout=30, + check=False, + ) + if result.returncode == 0: + data = result.stdout + sha = hashlib.sha256(data).hexdigest() + # Detect binary: if UTF-8 decode fails, mark as binary + encoding: str | None = "utf-8" + try: + data.decode("utf-8") + except UnicodeDecodeError: + encoding = None + return Content(data=data, encoding=encoding, content_hash=sha) + except (subprocess.TimeoutExpired, OSError): + logger.debug("git show failed for %s, falling back to fs read", path) + + # Fallback: direct filesystem read + if not target.exists(): + raise FileNotFoundError( + f"Path '{path}' not found in git-checkout at '{location}'" + ) + data = target.read_bytes() + sha = hashlib.sha256(data).hexdigest() + encoding = "utf-8" + try: + data.decode("utf-8") + except UnicodeDecodeError: + encoding = None + return Content(data=data, encoding=encoding, content_hash=sha) + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Write a file into the git checkout. + + Writes directly to the filesystem within the checkout location. + Does **not** auto-commit; the caller is responsible for staging + and committing changes. + + Args: + resource: The git-checkout resource. + path: Relative path within the repo. + data: Raw bytes to write. + + Returns: + A :class:`WriteResult` indicating success. + """ + location = self._require_location(resource) + target = self._safe_resolve(location, path) + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(data) + sha = hashlib.sha256(data).hexdigest() + return WriteResult( + success=True, + bytes_written=len(data), + content_hash=sha, + message=f"Wrote {len(data)} bytes to {path}", + ) + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Delete a file from the git checkout. + + Removes the file from the filesystem. If the file is tracked, + also runs ``git rm --cached`` to unstage it. + + Args: + resource: The git-checkout resource. + path: Relative path within the repo. + + Returns: + A :class:`DeleteResult` indicating success. + """ + if not path: + raise PermissionError("Cannot delete the resource root (empty path)") + location = self._require_location(resource) + target = self._safe_resolve(location, path) + + if not target.exists(): + raise FileNotFoundError( + f"Path '{path}' not found in git-checkout at '{location}'" + ) + + if target.is_dir(): + import shutil + + shutil.rmtree(target) + else: + target.unlink() + + # Unstage if tracked + subprocess.run( + ["git", "rm", "--cached", "--ignore-unmatch", "-r", path], + cwd=location, + capture_output=True, + timeout=30, + check=False, + ) + + return DeleteResult(success=True, message=f"Deleted {path}") + + def list_children(self, *, resource: Resource) -> list[str]: + """List tracked files using ``git ls-tree``. + + Returns top-level entries (files and directories) tracked by + git at HEAD. + + Args: + resource: The git-checkout resource. + + Returns: + Sorted list of top-level paths. + """ + location = self._require_location(resource) + + result = subprocess.run( + ["git", "ls-tree", "--name-only", "HEAD"], + cwd=location, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + + if result.returncode != 0: + logger.warning( + "git ls-tree failed (rc=%d) for %s: %s", + result.returncode, + resource.resource_id, + result.stderr.strip(), + ) + # Fallback: filesystem listing + return sorted(os.listdir(location)) + + entries = [line for line in result.stdout.strip().split("\n") if line] + return sorted(entries) + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Compare the git checkout against another location. + + Uses ``git diff --no-index`` which works on arbitrary paths + (not just git-tracked content). + + Args: + resource: The git-checkout resource. + other_location: Filesystem path to compare against. + + Returns: + A :class:`DiffResult` summarising the differences. + """ + location = self._require_location(resource) + + # Force C locale so --shortstat output is always English + env = {**os.environ, "LC_ALL": "C"} + + result = subprocess.run( + ["git", "diff", "--no-index", "--shortstat", location, other_location], + capture_output=True, + text=True, + timeout=60, + check=False, + env=env, + ) + + # git diff --no-index exits 0 = no diff, 1 = has diff, 2+ = error + has_changes = result.returncode == 1 + + # Get unified diff for detail (binary-safe) + unified_result = subprocess.run( + ["git", "diff", "--no-index", location, other_location], + capture_output=True, + timeout=60, + check=False, + env=env, + ) + + unified = ( + unified_result.stdout.decode("utf-8", errors="replace") + if has_changes + else "" + ) + + # Parse --shortstat with named patterns to handle all variants: + # "N files changed, N insertions(+), N deletions(-)" + # "N files changed, N insertions(+)" + # "N files changed, N deletions(-)" + files_changed = 0 + insertions = 0 + deletions = 0 + if has_changes and result.stdout: + import re + + m_files = re.search(r"(\d+) file", result.stdout) + m_ins = re.search(r"(\d+) insertion", result.stdout) + m_del = re.search(r"(\d+) deletion", result.stdout) + if m_files: + files_changed = int(m_files.group(1)) + if m_ins: + insertions = int(m_ins.group(1)) + if m_del: + deletions = int(m_del.group(1)) + + return DiffResult( + has_changes=has_changes, + unified_diff=unified, + files_changed=files_changed, + insertions=insertions, + deletions=deletions, + ) + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Discover child resources via ``git ls-tree``. + + Each top-level directory in the repo becomes a child resource + of type ``fs-directory``. + + Args: + resource: The parent git-checkout resource. + + Returns: + List of child :class:`Resource` objects for top-level + directories. + """ + location = self._require_location(resource) + + result = subprocess.run( + ["git", "ls-tree", "--name-only", "-d", "HEAD"], + cwd=location, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + + children: list[Resource] = [] + if result.returncode != 0: + return children + + for dirname in result.stdout.strip().split("\n"): + dirname = dirname.strip() + if not dirname: + continue + child_path = str(Path(location) / dirname) + child = Resource( + resource_id=self._derive_child_id(resource.resource_id, dirname), + name=dirname, + resource_type_name="fs-directory", + classification=resource.classification, + description=f"Directory '{dirname}' in {resource.resource_id}", + location=child_path, + parents=[resource.resource_id], + ) + children.append(child) + + return children diff --git a/src/cleveragents/resource/handlers/protocol.py b/src/cleveragents/resource/handlers/protocol.py index 84ba4e5fc..ae6a5b9ae 100644 --- a/src/cleveragents/resource/handlers/protocol.py +++ b/src/cleveragents/resource/handlers/protocol.py @@ -2,36 +2,138 @@ Defines the :class:`ResourceHandler` protocol that all resource type handlers must satisfy. A handler bridges a :class:`Resource` domain -object to sandbox provisioning by: +object to sandbox provisioning **and** to content CRUD operations: -1. Reading the resource's ``location`` (the original filesystem path). -2. Determining the sandbox strategy (from resource override or type default). -3. Calling :meth:`SandboxManager.get_or_create_sandbox` to provision an - isolated working directory. -4. Returning a :class:`BoundResource` with ``sandbox_path`` populated. +Sandbox provisioning (M1): + 1. Reading the resource's ``location`` (the original filesystem path). + 2. Determining the sandbox strategy (from resource override or type default). + 3. Calling :meth:`SandboxManager.get_or_create_sandbox` to provision an + isolated working directory. + 4. Returning a :class:`BoundResource` with ``sandbox_path`` populated. + +Content CRUD and discovery (M6): + 5. ``read`` — read resource content from its resolved location. + 6. ``write`` — write/update resource content. + 7. ``delete`` — remove a resource. + 8. ``list_children`` — list direct child resources. + 9. ``diff`` — compare two resource states. + 10. ``discover_children`` — auto-discover child resources. Based on: - implementation_plan.md group M1.resource-handlers (L2254-L2271) - docs/specification.md Resource Handler architecture + - Issue #827 — ResourceHandler CRUD and discovery methods """ from __future__ import annotations +from dataclasses import dataclass, field from typing import Protocol, runtime_checkable from cleveragents.domain.models.core.resource import Resource from cleveragents.infrastructure.sandbox.manager import SandboxManager from cleveragents.tool.context import BoundResource +# --------------------------------------------------------------------------- +# Result types for CRUD and discovery operations +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True, slots=True) +class Content: + """Result of a ``read`` operation. + + Attributes: + data: The raw content bytes. + encoding: Character encoding (e.g. ``"utf-8"``). ``None`` + for binary content. + content_hash: Optional hash (e.g. SHA-256 hex digest) of *data*. + metadata: Arbitrary handler-specific metadata. + """ + + data: bytes + encoding: str | None = "utf-8" + content_hash: str | None = None + metadata: dict[str, str] = field(default_factory=dict) + + @property + def text(self) -> str: + """Decode *data* using *encoding*. + + Raises: + UnicodeDecodeError: If *data* cannot be decoded. + ValueError: If *encoding* is ``None`` (binary content). + """ + if self.encoding is None: + raise ValueError("Cannot decode binary content as text") + return self.data.decode(self.encoding) + + +@dataclass(frozen=True, slots=True) +class WriteResult: + """Result of a ``write`` operation. + + Attributes: + success: Whether the write completed without error. + bytes_written: Number of bytes written. + content_hash: Hash of the written content. + message: Human-readable status message. + """ + + success: bool + bytes_written: int = 0 + content_hash: str | None = None + message: str = "" + + +@dataclass(frozen=True, slots=True) +class DeleteResult: + """Result of a ``delete`` operation. + + Attributes: + success: Whether the deletion completed without error. + message: Human-readable status message. + """ + + success: bool + message: str = "" + + +@dataclass(frozen=True, slots=True) +class DiffResult: + """Result of a ``diff`` operation. + + Attributes: + has_changes: ``True`` if the two states differ. + unified_diff: Unified diff string (empty if no changes). + files_changed: Number of files with differences. + insertions: Number of added lines. + deletions: Number of removed lines. + """ + + has_changes: bool + unified_diff: str = "" + files_changed: int = 0 + insertions: int = 0 + deletions: int = 0 + + +# --------------------------------------------------------------------------- +# Protocol +# --------------------------------------------------------------------------- + @runtime_checkable class ResourceHandler(Protocol): """Protocol for resource type handlers. Each handler knows how to resolve a specific resource type into a - sandboxed working path. Implementations are registered via the - ``handler`` field on :class:`ResourceTypeSpec` using the - ``module:ClassName`` string format. + sandboxed working path, and optionally provides content CRUD and + discovery operations. + + Implementations are registered via the ``handler`` field on + :class:`ResourceTypeSpec` using the ``module:ClassName`` string + format. Lifecycle:: @@ -76,3 +178,92 @@ class ResourceHandler(Protocol): SandboxError: If sandbox creation fails. """ ... + + def read(self, *, resource: Resource, path: str = "") -> Content: + """Read content from a resource. + + Args: + resource: The resource to read from. + path: Relative path within the resource (empty string for + the resource root or a single-file resource). + + Returns: + A :class:`Content` object with the raw bytes and metadata. + + Raises: + FileNotFoundError: If *path* does not exist within the + resource. + PermissionError: If the resource is not readable. + """ + ... + + def write(self, *, resource: Resource, path: str, data: bytes) -> WriteResult: + """Write content to a resource. + + Args: + resource: The resource to write to. + path: Relative path within the resource. + data: Raw bytes to write. + + Returns: + A :class:`WriteResult` indicating success and bytes written. + + Raises: + PermissionError: If the resource is read-only. + OSError: If the write fails at the OS level. + """ + ... + + def delete(self, *, resource: Resource, path: str = "") -> DeleteResult: + """Delete a resource or a path within it. + + Args: + resource: The resource to delete from. + path: Relative path within the resource. Empty string + deletes the resource root. + + Returns: + A :class:`DeleteResult` indicating success. + + Raises: + PermissionError: If the resource is read-only. + FileNotFoundError: If *path* does not exist. + """ + ... + + def list_children(self, *, resource: Resource) -> list[str]: + """List direct children of a resource. + + Args: + resource: The parent resource. + + Returns: + Sorted list of child paths relative to the resource root. + """ + ... + + def diff(self, *, resource: Resource, other_location: str) -> DiffResult: + """Compare a resource against another location. + + Args: + resource: The resource whose current state to compare. + other_location: Filesystem path to compare against. + + Returns: + A :class:`DiffResult` summarising the differences. + """ + ... + + def discover_children(self, *, resource: Resource) -> list[Resource]: + """Auto-discover child resources beneath this resource. + + Used to populate the resource DAG with discovered sub-resources + (e.g. files within a directory, tables within a database). + + Args: + resource: The parent resource to scan. + + Returns: + List of newly discovered child :class:`Resource` objects. + """ + ...