refactor: 清理未使用代码并添加未来功能标记

style: 统一代码格式和注释风格

docs: 更新多个功能文档的完整度和状态

feat(runtime): 添加路径验证工具支持

fix(pipeline): 改进条件判断和变量解析逻辑

test(types): 为ID类型添加全面测试用例

chore: 更新依赖项和Cargo.lock文件

perf(mcp): 优化MCP协议传输和错误处理

plans/whimsical-mapping-patterson-agent-ae164c5c54d22eaf5.md

@@ -0,0 +1,293 @@
+# ZCLAW Rust Crates Code Review Report
+
+## Architecture Evaluation (7.5/10)
+
+The ZCLAW Rust backend demonstrates a well-organized layered architecture with clear separation of concerns. The crate structure follows the documented dependency hierarchy (types -> memory -> runtime -> kernel). However, there are several areas that need improvement in terms of code completeness, security hardening, and consistency.
+
+**Strengths:**
+- Clean layered architecture with proper dependency direction
+- Comprehensive type system with unified error handling
+- Good use of async/await patterns with proper synchronization
+- Security-conscious design with path validation and shell execution controls
+
+**Areas for Improvement:**
+- Stub implementations in core components
+- Incomplete integration of new modules
+- Missing test coverage in critical paths
+- Some security concerns in shell execution
+
+---
+
+## Critical Issues (Critical)
+
+### 1. [Security] Shell Command Timeout Check is Ineffective
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool\builtin\shell_exec.rs`
+**Lines:** 195-209
+
+**Problem:** The timeout check occurs AFTER command execution completes, not during execution. If a command hangs, it will never timeout because the check happens after `output()` returns.
+
+```rust
+// Current (ineffective):
+let output = tokio::task::spawn_blocking(move || {
+    cmd.output()
+})
+.await
+...
+let duration = start.elapsed();
+if duration > Duration::from_secs(timeout_secs) {
+    return Err(...);  // Too late - command already finished!
+}
+```
+
+**Recommendation:** Use `tokio::time::timeout` wrapper around the blocking task, or use async process spawning with `tokio::process::Command` which supports proper timeout handling.
+
+### 2. [Security] Shell Command Parsing is Fragile
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool\builtin\shell_exec.rs`
+**Lines:** 170-177
+
+**Problem:** Simple whitespace splitting fails for commands with quoted arguments:
+- `echo "hello world"` becomes `["echo", "\"hello", "world\""]` instead of `["echo", "hello world"]`
+- This can lead to unexpected behavior and potential security bypasses
+
+**Recommendation:** Use `shlex` crate for proper shell-style quoting, or switch to structured command input (separate program and args in JSON schema).
+
+### 3. [Architecture] TriggerManager Not Integrated with Kernel
+**File:** `G:\ZClaw_openfang\crates\zclaw-kernel\src\kernel.rs`
+**Lines:** 424-482
+
+**Problem:** The `TriggerManager` is defined but never instantiated in `Kernel`. All trigger-related methods in `Kernel` are stub implementations that return empty lists or errors.
+
+```rust
+// All stubs - TriggerManager exists but is not used
+pub async fn list_triggers(&self) -> Vec<crate::trigger_manager::TriggerEntry> {
+    Vec::new()  // Stub!
+}
+```
+
+**Recommendation:** Either:
+1. Complete the integration by adding `TriggerManager` as a field in `Kernel` and wiring up the methods
+2. Remove the stub methods entirely to avoid confusion
+3. Add `#[deprecated]` markers if this is intentionally deferred
+
+### 4. [Architecture] PathValidator Never Passed to Tools
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\loop_runner.rs`
+**Lines:** 80-88, 345-351
+
+**Problem:** The `ToolContext` created in `create_tool_context()` and in `run_streaming()` always sets `path_validator: None`, despite `PathValidator` being implemented for security.
+
+```rust
+fn create_tool_context(&self, session_id: SessionId) -> ToolContext {
+    ToolContext {
+        ...
+        path_validator: None,  // Always None!
+    }
+}
+```
+
+**Impact:** File tools fall back to creating their own validators with default settings, defeating the purpose of centralized security configuration.
+
+**Recommendation:** Add `PathValidator` configuration to `AgentLoop` and pass it through to `ToolContext`.
+
+---
+
+## Important Issues (Important)
+
+### 5. [Concurrency] Potential Deadlock in TriggerManager
+**File:** `G:\ZClaw_openfang\crates\zclaw-kernel\src\trigger_manager.rs`
+**Lines:** 209-221
+
+**Problem:** The `should_fire` method acquires `triggers` read lock, then tries to acquire `states` read lock. Other methods acquire these locks in different orders, which could lead to deadlock.
+
+```rust
+let triggers = self.triggers.read().await;
+// ... then later ...
+let states = self.states.read().await;
+```
+
+**Recommendation:** Always acquire locks in the same order globally, or use a single combined state structure, or use `tokio::sync::RwLock` with `try_read` and retry logic.
+
+### 6. [Error Handling] MCP Transport Silently Discards stderr
+**File:** `G:\ZClaw_openfang\crates\zclaw-protocols\src\mcp_transport.rs`
+**Line:** 131
+
+**Problem:** stderr is redirected to null, making debugging MCP server issues very difficult.
+
+```rust
+cmd.stdin(Stdio::piped())
+    .stdout(Stdio::piped())
+    .stderr(Stdio::null());  // All error output lost!
+```
+
+**Recommendation:** Capture stderr in a separate task and log it via `tracing`, or provide a configuration option to capture it.
+
+### 7. [Error Handling] Missing MCP Process Cleanup
+**File:** `G:\ZClaw_openfang\crates\zclaw-protocols\src\mcp_transport.rs`
+**Lines:** 86-92
+
+**Problem:** The `McpTransport` stores a `Child` process but never implements `Drop` to kill the child process when the transport is dropped. This leads to orphaned processes.
+
+**Recommendation:** Implement `Drop` for `McpTransport` to kill the child process:
+```rust
+impl Drop for McpTransport {
+    fn drop(&mut self) {
+        // Kill child process if running
+    }
+}
+```
+
+### 8. [Code Quality] Unused Assignment in TriggerManager::execute_trigger
+**File:** `G:\ZClaw_openfang\crates\zclaw-kernel\src\trigger_manager.rs`
+**Line:** 317
+
+**Problem:** The return value handling is confusing - it maps the hand result to trigger result but the logic is unclear:
+
+```rust
+hand_result.map(|_| trigger_result)  // Returns trigger_result, not hand result
+```
+
+**Recommendation:** Clarify the intent with a comment or restructure to make the mapping more explicit.
+
+### 9. [Type Safety] Missing Clone Implementation for ToolContext
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool.rs`
+**Lines:** 63-73
+
+**Problem:** Manual `Clone` implementation is provided but `Debug` is also manual. Consider deriving these where possible, or document why manual implementation is needed.
+
+**Recommendation:** If `SkillExecutor` trait object needs special handling, document this. Otherwise, consider restructuring to allow derive macros.
+
+### 10. [Performance] Inefficient Tool Lookup
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool.rs`
+**Lines:** 90-91
+
+**Problem:** Tool lookup is O(n) linear search through the vector:
+
+```rust
+pub fn get(&self, name: &str) -> Option<Arc<dyn Tool>> {
+    self.tools.iter().find(|t| t.name() == name).cloned()
+}
+```
+
+**Recommendation:** Use a `HashMap<String, Arc<dyn Tool>>` for O(1) lookups, especially important since tools are looked up frequently in the agent loop.
+
+---
+
+## Minor Issues (Minor)
+
+### 11. [Style] Inconsistent Default Implementations
+**File:** `G:\ZClaw_openfang\crates\zclaw-kernel\src\trigger_manager.rs`
+**Lines:** 43-45
+
+**Problem:** Default functions defined at module level rather than inline:
+
+```rust
+fn default_max_executions_per_hour() -> u32 { 10 }
+fn default_persist() -> bool { true }
+```
+
+**Recommendation:** Use inline defaults where possible: `#[serde(default = "10")]` doesn't work, but consider using `const` values for clarity.
+
+### 12. [Documentation] Missing Safety Documentation
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool\builtin\path_validator.rs`
+
+**Problem:** The `PathValidator` has good comments but lacks `# Safety` sections for the security-critical functions.
+
+**Recommendation:** Add `# Security` documentation sections explaining what guarantees each validation method provides.
+
+### 13. [Style] Hardcoded Chinese String in Loop Runner
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\loop_runner.rs`
+**Lines:** 117, 238
+
+**Problem:** Hardcoded Chinese error message:
+```rust
+let error_msg = "Reach maximum iterations, please simplify request";
+```
+vs line 238:
+```rust
+let _ = tx.send(LoopEvent::Error("Reach maximum iterations".to_string())).await;
+```
+
+**Recommendation:** Use consistent language (Chinese per project requirements) and consider extracting to constants or i18n.
+
+### 14. [Test Coverage] PathValidator Tests Only Cover Happy Paths
+**File:** `G:\ZClaw_openfang\crates\zclaw-runtime\src\tool\builtin\path_validator.rs`
+**Lines:** 327-365
+
+**Problem:** Tests only check basic functionality. Missing tests for:
+- Symlink blocking
+- Max file size enforcement
+- Blocked path detection on Windows
+- Allowed paths whitelist behavior
+
+**Recommendation:** Add comprehensive security tests, especially for edge cases.
+
+### 15. [Architecture] Kernel::categorize_skills Has Hardcoded Categories
+**File:** `G:\ZClaw_openfang\crates\zclaw-kernel\src\kernel.rs`
+**Lines:** 177-189
+
+**Problem:** Skill categories are hardcoded with Chinese names and specific skill IDs. This makes maintenance difficult.
+
+**Recommendation:** Move category patterns to configuration file or use skill metadata for categorization.
+
+---
+
+## Highlights
+
+### 1. Excellent Path Validator Design
+The `PathValidator` in `path_validator.rs` is well-designed with:
+- Clear separation of concerns
+- Proper handling of symlinks, path traversal, and size limits
+- Good default blocked paths for security
+- Cross-platform considerations (Unix and Windows paths)
+
+### 2. Clean Async Architecture
+The agent loop in `loop_runner.rs` demonstrates excellent use of:
+- Proper async/await patterns
+- Channel-based streaming
+- Multi-turn tool calling support
+- Clean separation between streaming and non-streaming paths
+
+### 3. Comprehensive Error Types
+The `ZclawError` enum in `error.rs` covers all major error categories with proper `thiserror` integration, making error handling consistent across the codebase.
+
+### 4. Well-Structured MCP Protocol Implementation
+The MCP types and transport layer in `mcp_types.rs` and `mcp_transport.rs` provide:
+- Complete JSON-RPC 2.0 implementation
+- Clean type-safe request/response handling
+- Good use of serde for serialization
+
+---
+
+## Summary and Recommendations
+
+### Immediate Actions Required
+
+1. **Fix shell_exec timeout** - This is a security issue that could allow commands to run indefinitely
+2. **Complete TriggerManager integration** - Either integrate it or remove the stub methods
+3. **Wire up PathValidator in AgentLoop** - Security configuration is being bypassed
+
+### Short-term Improvements
+
+4. Add MCP process cleanup on drop
+5. Fix potential deadlock in TriggerManager
+6. Improve tool lookup performance with HashMap
+7. Capture MCP stderr for debugging
+
+### Long-term Considerations
+
+8. Add comprehensive security tests
+9. Move hardcoded configurations to external files
+10. Consider adding metrics/observability hooks
+
+### Code Quality Score Breakdown
+
+| Category | Score | Notes |
+|----------|-------|-------|
+| Architecture | 8/10 | Clean layers, good separation |
+| Security | 6/10 | Good foundation but gaps in implementation |
+| Error Handling | 8/10 | Comprehensive types, consistent usage |
+| Concurrency | 7/10 | Proper async, some lock ordering concerns |
+| Test Coverage | 5/10 | Basic tests, missing edge cases |
+| Documentation | 6/10 | Module docs good, inline docs sparse |
+| Code Style | 7/10 | Generally consistent, some hardcoded values |
+
+**Overall: 7.5/10** - Solid foundation with some critical gaps that need addressing before production use.