fix: use importlib.resources for template access (#17)

[ckrough]

Summary

Fixes Issue #17 where agentspaces fails to find templates when run from a different directory.

The root cause was that design.py and skills.py used Path(__file__).parent.parent.parent.parent to locate templates, which only works when running from source but fails when the package is installed.

Changes

• Move templates/ into src/agentspaces/templates/ as package data
• Add resources.py module using importlib.resources for package resource access
• Update design.py and skills.py to use new resources module
• Configure pyproject.toml to include template files in wheel
• Add comprehensive tests for resources module (100% coverage)
• Update CLAUDE.md to reflect new template location

Test plan

• All 330 tests pass
• resources.py has 100% test coverage
• agentspaces docs list works correctly
• Template paths resolve to package location

[claude]

Pull Request Review: fix: use importlib.resources for template access (#17)

Summary

This PR successfully fixes Issue #17 by migrating from fragile __file__-based path resolution to the standard importlib.resources API. The implementation is well-structured with comprehensive test coverage (100% for the new module).

---

✅ Strengths

1. Correct Solution Architecture

• Using importlib.resources.files() is the standard Python 3.9+ approach for package resources
• Properly handles both source and installed package scenarios
• Clean separation of concerns with a dedicated resources.py module

2. Code Quality

• Excellent adherence to project conventions (type hints, docstrings, error handling)
• Proper exception chaining with from e throughout
• Clean integration into existing modules via exception translation pattern
• Follows existing patterns in design.py and skills.py (src/agentspaces/infrastructure/design.py:79-82)

3. Test Coverage

• Comprehensive test suite with 100% coverage of resources.py
• Tests include edge cases (missing directories, module not found, type errors)
• Integration verified through existing tests passing

4. Documentation

• CLAUDE.md properly updated to reflect new structure
• Clear docstrings explaining purpose and behavior
• Good inline comments explaining design decisions (src/agentspaces/infrastructure/resources.py:40-42)

---

⚠️ Issues & Recommendations

🔴 Critical: Package Data Configuration Issue

Location: pyproject.toml:60-63

Problem: The [tool.hatch.build] include configuration is insufficient and may not work correctly:

[tool.hatch.build]
include = [
    "src/agentspaces/**/*.py",
    "src/agentspaces/templates/**/*.md",
]

Issues:

1. The **/*.py pattern is redundant - hatch already includes Python files from the packages directive
2. This only includes .md files but the templates directory contains README files without extensions (visible in the diff showing moved files like .claude/agents/README.md)
3. The configuration should use [tool.hatch.build.targets.wheel] section, not the bare [tool.hatch.build]

Recommended Fix:

[tool.hatch.build.targets.wheel]
packages = ["src/agentspaces"]

# Include all template files as package data
[tool.hatch.build.targets.wheel.force-include]
"src/agentspaces/templates" = "agentspaces/templates"

Or alternatively, use the simpler approach:

[tool.hatch.build.targets.wheel]
packages = ["src/agentspaces"]

[tool.hatch.build.targets.wheel.shared-data]
"src/agentspaces/templates" = "agentspaces/templates"

Testing needed: Build a wheel (uv build) and verify template files are included:

uv build
unzip -l dist/agentspaces-*.whl | grep templates

---

🟡 Medium: Zip-Safe Compatibility Concern

Location: src/agentspaces/infrastructure/resources.py:43

Problem: The code converts the traversable to a Path directly:

templates_path = Path(str(templates_ref))

The comment mentions as_file context manager but doesn't use it. For zip-imported packages (e.g., when installed in certain environments), this might fail because the resource may not exist as a filesystem path.

Recommendation: Use the recommended pattern for Python 3.9+:

from importlib.resources import files, as_file

def _get_templates_dir() -> Path:
    try:
        templates_ref = files("agentspaces.templates")
        # For most cases, direct path access works, but as_file ensures
        # compatibility with zip-imported packages
        with as_file(templates_ref) as templates_path:
            if not templates_path.exists():
                raise ResourceError(
                    f"Templates directory not found: {templates_path}"
                )
            # Return the path - it remains valid after context exit
            # for directory resources
            return Path(templates_path)
    except ModuleNotFoundError as e:
        raise ResourceError(...) from e

However, there's a structural problem: as_file is a context manager, so the path may not be valid after the function returns. A better approach:

Option 1: Return the Traversable object and use it directly (recommended):

from importlib.resources.abc import Traversable

def _get_templates_resource() -> Traversable:
    """Get templates as a traversable resource."""
    try:
        return files("agentspaces.templates")
    except ModuleNotFoundError as e:
        raise ResourceError(...) from e

Then adapt consumers to work with Traversable instead of Path.

Option 2: Keep current approach but document the limitation:
Add a comment noting this won't work for zip-imported packages but is simpler and works for the common installation methods.

Given the project is a CLI tool (not a library that might be zip-imported), Option 2 is acceptable - just add a clear comment.

---

🟡 Medium: Security Consideration Removed

Location: src/agentspaces/infrastructure/design.py:70-83, src/agentspaces/infrastructure/skills.py:53-74

Observation: The old code included path traversal protection:

# Ensure templates_dir is within package root (prevent symlink attacks)
if not str(resolved).startswith(str(package_resolved)):
    raise DesignError("Templates directory escapes package root")

This was removed in the new implementation.

Analysis: This is likely acceptable because:

• importlib.resources provides paths from the package manager, which are already validated
• The attack vector (malicious symlinks in templates directory) is less relevant when using package resources
• If someone can modify the installed package, they can already execute arbitrary code

Recommendation: No action needed, but consider adding a comment in resources.py explaining that path validation is handled by the package manager.

---

🟢 Minor: Error Message Consistency

Location: src/agentspaces/infrastructure/resources.py:46-47

The error messages use f"... {templates_path}" which exposes internal paths to end users. Consider whether these should be more user-friendly:

raise ResourceError(
    "Templates directory not found. Ensure agentspaces is installed correctly."
)

Current approach is fine for debugging but might confuse end users.

---

🟢 Minor: Test Coverage Gap

Location: tests/unit/infrastructure/test_resources.py

The tests mock _get_templates_dir in some places but don't test the actual importlib.resources.files() call with the real package.

Suggestion: Add an integration-style test:

def test_actual_package_resource_access() -> None:
    """Should successfully access real package resources."""
    from importlib.resources import files
    
    templates_ref = files("agentspaces.templates")
    assert templates_ref is not None
    # Verify it's accessible
    assert (templates_ref / "skeleton").is_dir()
    assert (templates_ref / "skills").is_dir()

This ensures the package structure is actually correct.

---

🔒 Security Review

• ✅ Input sanitization maintained in skills.py (_sanitize_for_markdown)
• ✅ No new injection vulnerabilities introduced
• ✅ Exception chaining properly maintains error context without exposing sensitive data
• ✅ No hardcoded credentials or secrets

---

📊 Performance Considerations

• ✅ No performance concerns - resource access is cached by Python
• ✅ File I/O patterns unchanged from previous implementation
• ✅ No new loops or expensive operations

---

🧪 Test Coverage

• ✅ 330 tests passing (per PR description)
• ✅ 100% coverage on new resources.py module
• ✅ Edge cases well covered (missing dirs, module not found, type errors)
• ⚠️ Missing: End-to-end test of built wheel installation

---

📋 Checklist Before Merge

1. CRITICAL: Fix pyproject.toml package data configuration and verify with uv build
2. RECOMMENDED: Test actual wheel installation: uv build && pip install dist/*.whl && agentspaces docs list
3. OPTIONAL: Consider zip-safe compatibility (document limitation or use as_file)
4. OPTIONAL: Add integration test for actual package resource access

---

🎯 Verdict

Conditional Approval - The architecture and implementation are excellent, but there's a critical configuration issue that must be fixed before merging.

Required Changes:

• Fix the pyproject.toml include configuration to ensure all template files (including READMEs) are bundled
• Verify with a wheel build and installation test

Recommended Changes:

• Add comment about zip-import limitations (or implement as_file pattern)
• Consider more user-friendly error messages
• Add integration test for package resource access

Great work overall! The migration to importlib.resources is the right approach and the code quality is very high. The test coverage is exemplary. Just need to fix that packaging configuration.

---

Reviewed with ❤️ by Claude Code