Feature/yuchen.liu/mcp skills resources provider

[Leoyzen]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces several significant changes, including the implementation of concurrent execution safety for agents, the addition of slash command support for skills, and updates to the OpenCode server to support multi-question elicitation. My review identified a critical bug where registry_id was removed from the ClaudeCodeAgent constructor but remained in the from_config method, which will cause runtime errors. Additionally, I have concerns about the removal of E2E test artifacts from .gitignore, as this may lead to committing unnecessary or sensitive files.

I am having trouble creating individual review comments. Click here to see my feedback.

src/agentpool/agents/claude_code_agent/claude_code_agent.py (171)

The variable _registry_id was removed from the constructor but not from the from_config method. This will cause a TypeError when from_config is called.

return cls(
            command=config.get_command(),
            args=config.get_args(),
            # Identity
            name=config.name,
            description=config.description,

.gitignore (1-3)

Removing the E2E test artifacts from .gitignore might lead to committing large or sensitive test artifacts to the repository. Please ensure these artifacts are not intended to be ignored.

[Leoyzen]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements the MCP Skills Resources Provider protocol, enabling AgentPool to consume and provide skills via MCP resources and the skill:// URI scheme. While the implementation provides a solid foundation for unified skill access, several issues were identified: the load_skill tool fails to pass the pool context for reference loading and incorrectly handles bare name resolution, the SkillURIResolver has a regex mismatch with the Skill model regarding underscores, and there are priority inversion issues in _sync_commands and the OpenCode route handler. Additionally, performance and correctness improvements are needed for MCP skill discovery and instruction fetching.

[Leoyzen]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements the MCP Skills Resources Provider protocol, enabling unified skill access via skill:// URIs for both local and MCP-sourced skills. It introduces a LocalResourceProvider, extends MCPResourceProvider to support skill discovery, and adds a SkillURIResolver for URI parsing and resolution. My review identified a critical infinite loop in the signal forwarding logic within AgentPool, a documentation gap regarding argument parsing, and a missing import in the documentation examples.

[gemini-code-assist]

The documentation on argument substitution should clarify how arguments containing spaces are handled. The current implementation splits arguments by spaces, which means a value like "Alice Smith" would be treated as two separate arguments, "Alice" and "Smith".

Please add a note explaining this behavior and suggest how to handle multi-word arguments, for example by using quotes if the parser supports it, or by noting it as a current limitation.

[Leoyzen]

/gemini review

[gemini-code-assist]

Warning

Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting /gemini review.

[Leoyzen]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements the skill:// URI scheme for AgentPool, enabling unified access to skills from local filesystems and MCP servers as specified in RFC-0020. It introduces a SkillURIResolver, a LocalResourceProvider, and extends the MCPResourceProvider to support both prompt-based and resource-based skills. The load_skill and list_skills tools have been updated to support URI resolution and argument substitution. Review feedback identified critical security improvements needed for path validation in both the URI resolver and the MCP resource provider, specifically to explicitly reject absolute paths and prevent potential path traversal vulnerabilities.

[gemini-code-assist]

The path validation in read_reference is insufficient and could allow an attacker to read arbitrary files on the server. The current implementation checks for .. but does not properly handle absolute paths. For example, a ref_path of /etc/passwd would bypass the checks because Path('/etc/passwd').resolve() returns an absolute path that doesn't start with ...

The use of Path(path_str).resolve() is also problematic for a remote resource, as it attempts to resolve a path on the local filesystem, which is not the correct approach for a URI-based resource.

Suggested change

	# Path traversal protection
	# Decode URL-encoded characters first
	decoded_path = unquote(ref_path)
	# Check for null bytes
	if "\x00" in decoded_path:
	raise SecurityError("Null bytes not allowed in path")
	# Check for path traversal attempts
	if ".." in decoded_path.split("/"):
	raise SecurityError(f"Path traversal detected: {ref_path}")
	# Normalize the path and ensure it's within references/
	def _validate_path(path_str: str) -> Path:
	"""Validate path and return resolved path."""
	try:
	return Path(path_str).resolve()
	except Exception as e:
	raise SecurityError(f"Invalid path: {ref_path}") from e
	safe_path = _validate_path(decoded_path)
	# The resolved path should not escape the references directory
	if safe_path.parts and safe_path.parts[0] == "..":
	raise SecurityError(f"Path escapes references directory: {ref_path}")
	# Construct the full URI
	# Path traversal protection
	# Decode URL-encoded characters first
	decoded_path = unquote(ref_path)
	# Check for null bytes
	if "\x00" in decoded_path:
	raise SecurityError("Null bytes not allowed in path")
	# Check for path traversal attempts and absolute paths
	if ".." in decoded_path.split("/") or decoded_path.startswith("/"):
	raise SecurityError(f"Path traversal detected: {ref_path}")

[gemini-code-assist]

The path validation for reference_path checks for .. and null bytes, but it does not prevent absolute paths (paths starting with /). While the provider might handle this, it's better to enforce this at the URI parsing level for defense-in-depth. An absolute path in a URI reference is ambiguous and a potential security risk.

Suggested change

	# Validate reference path components if present
	if reference_path is not None:
	ref_parts = reference_path.split("/")
	for part in ref_parts:
	if part == "..":
	msg = f"Path traversal detected in reference path: {uri!r}"
	raise SecurityError(msg)
	if reference_path is not None:
	if ".." in reference_path.split("/") or reference_path.startswith("/"):
	msg = f"Path traversal detected in reference path: {uri!r}"
	raise SecurityError(msg)

[Leoyzen]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements RFC-0020, introducing a unified skill:// URI scheme to load skills from both the local filesystem and MCP servers. Key additions include a SkillURIResolver, a LocalResourceProvider, and extensions to the MCPResourceProvider to support both prompt-based and resource-based skills. The load_skill and list_skills tools were updated to support URIs, reference content, and bash-style argument substitution. Feedback highlights several critical issues, including a path traversal vulnerability in reference loading, sync/async mismatches in provider initialization, and return type inconsistencies in the read_reference interface. Further improvements were suggested regarding URI parsing logic, ensuring skill priority is respected during dynamic re-syncs, and adding missing method definitions to the base ResourceProvider class for better type safety.

[gemini-code-assist]

If SkillsRegistry.discover_skills() is a synchronous method (as suggested by the RFC documentation), calling it with await will fail if it's not a coroutine, or block the event loop if it is a blocking sync call. You should use anyio.to_thread.run_sync() to run it in a separate thread, as described in the RFC's technical design.

[gemini-code-assist]

The ResourceProvider base class is missing definitions for get_skills, get_skill, get_references, and read_reference, even though these are implemented in subclasses and used polymorphically (e.g., in AggregatingResourceProvider and the load_skill tool). Adding these to the base class with default implementations would improve type safety and clarity.