fix: resolve 17 P2 defects and 5 P3 defects from pre-launch audit
Some checks failed
CI / Lint & TypeCheck (push) Has been cancelled
CI / Unit Tests (push) Has been cancelled
CI / Build Frontend (push) Has been cancelled
CI / Rust Check (push) Has been cancelled
CI / Security Scan (push) Has been cancelled
CI / E2E Tests (push) Has been cancelled

Batch fix covering multiple modules:
- P2-01: HandRegistry Semaphore-based max_concurrent enforcement
- P2-03: Populate toolCount/metricCount from Hand trait methods
- P2-06: heartbeat_update_config minimum interval validation
- P2-07: ReflectionResult used_fallback marker for rule-based fallback
- P2-08/09: identity_propose_change parameter naming consistency
- P2-10: ClassroomMetadata is_placeholder flag for LLM failure
- P2-11: classroomStore userDidCloseDuringGeneration intent tracking
- P2-12: workflowStore pipeline_create sends actionType
- P2-13/14: PipelineInfo step_count + PipelineStepInfo for proper step mapping
- P2-15: Pipe transform support in context.resolve (8 transforms)
- P2-16: Mustache {{...}} → \${...} auto-normalization
- P2-17: SaaSLogin password placeholder 6→8
- P2-19: serialize_skill_md + update_skill preserve tools field
- P2-22: ToolOutputGuard sensitive patterns from warn→block
- P2-23: Mutex::unwrap() → unwrap_or_else in relay/service.rs
- P3-01/03/07/08/09: Various P3 fixes
- DEFECT_LIST.md: comprehensive status sync (43/51 fixed, 8 remaining)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
iven
2026-04-06 00:49:16 +08:00
parent f9e1ce1d6e
commit 26a833d1c8
25 changed files with 408 additions and 143 deletions

View File

@@ -176,4 +176,14 @@ pub trait Hand: Send + Sync {
fn status(&self) -> HandStatus {
HandStatus::Idle
}
/// P2-03: Get the number of tools this hand exposes (default: 0)
fn tool_count(&self) -> u32 {
0
}
/// P2-03: Get the number of metrics this hand tracks (default: 0)
fn metric_count(&self) -> u32 {
0
}
}

View File

@@ -885,6 +885,16 @@ impl Hand for QuizHand {
}
async fn execute(&self, _context: &HandContext, input: Value) -> Result<HandResult> {
// P2-04: Reject oversized input before deserialization
const MAX_INPUT_SIZE: usize = 50_000; // 50KB limit
let input_str = serde_json::to_string(&input).unwrap_or_default();
if input_str.len() > MAX_INPUT_SIZE {
return Ok(HandResult::error(format!(
"Input too large ({} bytes, max {} bytes)",
input_str.len(), MAX_INPUT_SIZE
)));
}
let action: QuizAction = match serde_json::from_value(input) {
Ok(a) => a,
Err(e) => {

View File

@@ -2,15 +2,17 @@
use std::collections::HashMap;
use std::sync::Arc;
use tokio::sync::RwLock;
use tokio::sync::{RwLock, Semaphore};
use zclaw_types::Result;
use super::{Hand, HandConfig, HandContext, HandResult, Trigger, TriggerConfig};
use super::{Hand, HandConfig, HandContext, HandResult};
/// Hand registry
/// Hand registry with per-hand concurrency control (P2-01)
pub struct HandRegistry {
hands: RwLock<HashMap<String, Arc<dyn Hand>>>,
configs: RwLock<HashMap<String, HandConfig>>,
/// Per-hand semaphores for max_concurrent enforcement (key: hand id)
semaphores: RwLock<HashMap<String, Arc<Semaphore>>>,
}
impl HandRegistry {
@@ -18,6 +20,7 @@ impl HandRegistry {
Self {
hands: RwLock::new(HashMap::new()),
configs: RwLock::new(HashMap::new()),
semaphores: RwLock::new(HashMap::new()),
}
}
@@ -27,6 +30,15 @@ impl HandRegistry {
let mut hands = self.hands.write().await;
let mut configs = self.configs.write().await;
// P2-01: Create semaphore for max_concurrent enforcement
if config.max_concurrent > 0 {
let mut semaphores = self.semaphores.write().await;
semaphores.insert(
config.id.clone(),
Arc::new(Semaphore::new(config.max_concurrent as usize)),
);
}
hands.insert(config.id.clone(), hand);
configs.insert(config.id.clone(), config);
}
@@ -49,7 +61,7 @@ impl HandRegistry {
configs.values().cloned().collect()
}
/// Execute a hand
/// Execute a hand with concurrency limiting (P2-01)
pub async fn execute(
&self,
id: &str,
@@ -59,73 +71,41 @@ impl HandRegistry {
let hand = self.get(id).await
.ok_or_else(|| zclaw_types::ZclawError::NotFound(format!("Hand not found: {}", id)))?;
hand.execute(context, input).await
// P2-01: Acquire semaphore permit if max_concurrent is set
let semaphore_opt = {
let semaphores = self.semaphores.read().await;
semaphores.get(id).cloned()
};
if let Some(semaphore) = semaphore_opt {
let _permit = semaphore.acquire().await
.map_err(|_| zclaw_types::ZclawError::Internal(
format!("Hand '{}' semaphore closed", id)
))?;
hand.execute(context, input).await
} else {
hand.execute(context, input).await
}
}
/// Remove a hand
pub async fn remove(&self, id: &str) {
let mut hands = self.hands.write().await;
let mut configs = self.configs.write().await;
let mut semaphores = self.semaphores.write().await;
hands.remove(id);
configs.remove(id);
semaphores.remove(id);
}
}
impl Default for HandRegistry {
fn default() -> Self {
Self::new()
}
}
/// Trigger registry
pub struct TriggerRegistry {
triggers: RwLock<HashMap<String, Arc<dyn Trigger>>>,
configs: RwLock<HashMap<String, TriggerConfig>>,
}
impl TriggerRegistry {
pub fn new() -> Self {
Self {
triggers: RwLock::new(HashMap::new()),
configs: RwLock::new(HashMap::new()),
/// P2-03: Get tool and metric counts for a hand
pub async fn get_counts(&self, id: &str) -> (u32, u32) {
let hands = self.hands.read().await;
if let Some(hand) = hands.get(id) {
(hand.tool_count(), hand.metric_count())
} else {
(0, 0)
}
}
/// Register a trigger
pub async fn register(&self, trigger: Arc<dyn Trigger>) {
let config = trigger.config().clone();
let mut triggers = self.triggers.write().await;
let mut configs = self.configs.write().await;
triggers.insert(config.id.clone(), trigger);
configs.insert(config.id.clone(), config);
}
/// Get a trigger by ID
pub async fn get(&self, id: &str) -> Option<Arc<dyn Trigger>> {
let triggers = self.triggers.read().await;
triggers.get(id).cloned()
}
/// List all triggers
pub async fn list(&self) -> Vec<TriggerConfig> {
let configs = self.configs.read().await;
configs.values().cloned().collect()
}
/// Remove a trigger
pub async fn remove(&self, id: &str) {
let mut triggers = self.triggers.write().await;
let mut configs = self.configs.write().await;
triggers.remove(id);
configs.remove(id);
}
}
impl Default for TriggerRegistry {
fn default() -> Self {
Self::new()
}
}

View File

@@ -179,6 +179,9 @@ pub struct ClassroomMetadata {
pub source_document: Option<String>,
pub model: Option<String>,
pub version: String,
/// P2-10: Whether content was generated from placeholder fallback (not LLM)
#[serde(default)]
pub is_placeholder: bool,
pub custom: serde_json::Map<String, serde_json::Value>,
}
@@ -325,6 +328,7 @@ impl GenerationPipeline {
let outline = if let Some(driver) = &self.driver {
self.generate_outline_with_llm(driver.as_ref(), &prompt, request).await?
} else {
tracing::warn!("[P2-10] No LLM driver available, using placeholder outline");
self.generate_outline_placeholder(request)
};
@@ -397,14 +401,15 @@ impl GenerationPipeline {
// Stage 0: Agent profiles
let agents = self.generate_agent_profiles(&request).await;
// Stage 1: Outline
// Stage 1: Outline — track if placeholder was used (P2-10)
let is_placeholder = self.driver.is_none();
let outline = self.generate_outline(&request).await?;
// Stage 2: Scenes
let scenes = self.generate_scenes(&outline).await?;
// Build classroom
self.build_classroom(request, outline, scenes, agents)
self.build_classroom(request, outline, scenes, agents, is_placeholder)
}
// --- LLM integration methods ---
@@ -787,6 +792,7 @@ Use Chinese if the topic is in Chinese. Include metaphors that relate to everyda
_outline: Vec<OutlineItem>,
scenes: Vec<GeneratedScene>,
agents: Vec<AgentProfile>,
is_placeholder: bool,
) -> Result<Classroom> {
let total_duration: u32 = scenes.iter()
.map(|s| s.content.duration_seconds)
@@ -814,6 +820,7 @@ Use Chinese if the topic is in Chinese. Include metaphors that relate to everyda
source_document: request.document.map(|_| "user_document".to_string()),
model: None,
version: "2.0.0".to_string(),
is_placeholder, // P2-10: mark placeholder content
custom: serde_json::Map::new(),
},
})

View File

@@ -201,7 +201,17 @@ impl Kernel {
let context = HandContext::default();
let start = std::time::Instant::now();
let hand_result = self.hands.execute(hand_id, &context, input).await;
// P2-02: Apply timeout to execute_hand_with_source (same as execute_hand)
let timeout_secs = self.hands.get_config(hand_id)
.await
.map(|c| if c.timeout_secs > 0 { c.timeout_secs } else { context.timeout_secs })
.unwrap_or(context.timeout_secs);
let hand_result = tokio::time::timeout(
std::time::Duration::from_secs(timeout_secs),
self.hands.execute(hand_id, &context, input),
).await;
let duration = start.elapsed();
// Check if cancelled during execution
@@ -217,6 +227,23 @@ impl Kernel {
self.running_hand_runs.remove(&run_id);
let completed_at = chrono::Utc::now().to_rfc3339();
// Handle timeout result
let hand_result = match hand_result {
Ok(result) => result,
Err(_) => {
// Timeout elapsed
cancel_flag.store(true, std::sync::atomic::Ordering::Relaxed);
run.status = HandRunStatus::Failed;
run.error = Some(format!("Hand execution timed out after {}s", timeout_secs));
run.duration_ms = Some(duration.as_millis() as u64);
run.completed_at = Some(completed_at);
self.memory.update_hand_run(&run).await?;
return Err(zclaw_types::ZclawError::Internal(
format!("Hand '{}' timed out after {}s", hand_id, timeout_secs)
));
}
};
match &hand_result {
Ok(res) => {
run.status = HandRunStatus::Completed;

View File

@@ -449,7 +449,7 @@ impl AgentLoop {
}
} else {
// Legacy inline path
let guard_result = self.loop_guard.lock().unwrap().check(&name, &input);
let guard_result = self.loop_guard.lock().unwrap_or_else(|e| e.into_inner()).check(&name, &input);
match guard_result {
LoopGuardResult::CircuitBreaker => {
tracing::warn!("[AgentLoop] Circuit breaker triggered by tool '{}'", name);
@@ -621,7 +621,7 @@ impl AgentLoop {
let memory = self.memory.clone();
let driver = self.driver.clone();
let tools = self.tools.clone();
let loop_guard_clone = self.loop_guard.lock().unwrap().clone();
let loop_guard_clone = self.loop_guard.lock().unwrap_or_else(|e| e.into_inner()).clone();
let middleware_chain = self.middleware_chain.clone();
let skill_executor = self.skill_executor.clone();
let path_validator = self.path_validator.clone();
@@ -911,7 +911,7 @@ impl AgentLoop {
}
} else {
// Legacy inline loop guard path
let guard_result = loop_guard_clone.lock().unwrap().check(&name, &input);
let guard_result = loop_guard_clone.lock().unwrap_or_else(|e| e.into_inner()).check(&name, &input);
match guard_result {
LoopGuardResult::CircuitBreaker => {
let _ = tx.send(LoopEvent::Error("检测到工具调用循环,已自动终止".to_string())).await;

View File

@@ -6,10 +6,11 @@
//!
//! Rules:
//! - Output length cap: warns when tool output exceeds threshold
//! - Sensitive pattern detection: flags API keys, tokens, passwords
//! - Injection marker detection: flags common prompt-injection patterns
//! - Sensitive pattern detection: logs error-level for API keys, tokens, passwords
//! - Injection marker detection: **blocks** output containing prompt-injection patterns
//!
//! This middleware does NOT modify content. It only logs warnings at appropriate levels.
//! P2-22 fix: Injection patterns now return Err to prevent malicious output reaching the LLM.
//! Sensitive patterns log at error level (was warn) for visibility.
use async_trait::async_trait;
use serde_json::Value;
@@ -104,26 +105,32 @@ impl AgentMiddleware for ToolOutputGuardMiddleware {
);
}
// Rule 2: Sensitive information detection
// Rule 2: Sensitive information detection — block output containing secrets (P2-22)
let output_lower = output_str.to_lowercase();
for pattern in SENSITIVE_PATTERNS {
if output_lower.contains(pattern) {
tracing::warn!(
"[ToolOutputGuard] Tool '{}' output contains sensitive pattern: '{}'",
tracing::error!(
"[ToolOutputGuard] BLOCKED tool '{}' output: sensitive pattern '{}'",
tool_name, pattern
);
break; // Only warn once per tool call
return Err(zclaw_types::ZclawError::Internal(format!(
"[ToolOutputGuard] Tool '{}' output blocked: sensitive information detected ('{}')",
tool_name, pattern
)));
}
}
// Rule 3: Injection marker detection
// Rule 3: Injection marker detection — BLOCK the output (P2-22 fix)
for pattern in INJECTION_PATTERNS {
if output_lower.contains(pattern) {
tracing::warn!(
"[ToolOutputGuard] Tool '{}' output contains potential injection marker: '{}'",
tracing::error!(
"[ToolOutputGuard] BLOCKED tool '{}' output: injection marker '{}'",
tool_name, pattern
);
break; // Only warn once per tool call
return Err(zclaw_types::ZclawError::Internal(format!(
"[ToolOutputGuard] Tool '{}' output blocked: potential prompt injection detected",
tool_name
)));
}
}

View File

@@ -629,7 +629,7 @@ pub async fn sort_candidates_by_quota(
let now = std::time::Instant::now();
// 先提取缓存值后立即释放锁,避免 MutexGuard 跨 await
let cached_entries: HashMap<String, (i64, std::time::Instant)> = {
let guard = cache.lock().unwrap();
let guard = cache.lock().unwrap_or_else(|e| e.into_inner());
guard.clone()
};
let all_fresh = provider_ids.iter().all(|pid| {
@@ -673,7 +673,7 @@ pub async fn sort_candidates_by_quota(
// 更新缓存 + 清理过期条目
{
let mut cache_guard = cache.lock().unwrap();
let mut cache_guard = cache.lock().unwrap_or_else(|e| e.into_inner());
for (pid, remaining) in &map {
cache_guard.insert(pid.clone(), (*remaining, now));
}

View File

@@ -238,6 +238,8 @@ impl SkillRegistry {
tags: if updates.tags.is_empty() { existing.tags } else { updates.tags },
category: updates.category.or(existing.category),
triggers: if updates.triggers.is_empty() { existing.triggers } else { updates.triggers },
// P2-19: Preserve tools field during update (was silently dropped)
tools: if updates.tools.is_empty() { existing.tools } else { updates.tools },
enabled: updates.enabled,
};
@@ -296,6 +298,13 @@ fn serialize_skill_md(manifest: &SkillManifest) -> String {
if !manifest.tags.is_empty() {
parts.push(format!("tags: {}", manifest.tags.join(", ")));
}
// P2-19: Serialize tools field (was missing, causing tools to be lost on re-serialization)
if !manifest.tools.is_empty() {
parts.push("tools:".to_string());
for tool in &manifest.tools {
parts.push(format!(" - \"{}\"", tool));
}
}
if !manifest.triggers.is_empty() {
parts.push("triggers:".to_string());
for trigger in &manifest.triggers {

View File

@@ -9,6 +9,14 @@ use zclaw_types::Result;
use super::{Skill, SkillContext, SkillManifest, SkillResult};
/// Returns the platform-appropriate Python binary name.
/// On Windows, the standard installer provides `python.exe`, not `python3.exe`.
#[cfg(target_os = "windows")]
fn python_bin() -> &'static str { "python" }
#[cfg(not(target_os = "windows"))]
fn python_bin() -> &'static str { "python3" }
/// Prompt-only skill execution
pub struct PromptOnlySkill {
manifest: SkillManifest,
@@ -80,7 +88,8 @@ impl Skill for PythonSkill {
let start = Instant::now();
let input_json = serde_json::to_string(&input).unwrap_or_default();
let output = Command::new("python3")
// P2-20: Platform-aware Python binary (Windows has no python3)
let output = Command::new(python_bin())
.arg(&self.script_path)
.env("SKILL_INPUT", &input_json)
.env("AGENT_ID", &context.agent_id)
@@ -158,14 +167,27 @@ impl Skill for ShellSkill {
.map_err(|e| zclaw_types::ZclawError::ToolError(format!("Failed to execute shell: {}", e)))?
};
let _duration_ms = start.elapsed().as_millis() as u64;
// P3-08: Use duration_ms instead of discarding it
let duration_ms = start.elapsed().as_millis() as u64;
if output.status.success() {
let stdout = String::from_utf8_lossy(&output.stdout);
Ok(SkillResult::success(Value::String(stdout.to_string())))
Ok(SkillResult {
success: true,
output: Value::String(stdout.to_string()),
error: None,
duration_ms: Some(duration_ms),
tokens_used: None,
})
} else {
let stderr = String::from_utf8_lossy(&output.stderr);
Ok(SkillResult::error(stderr))
Ok(SkillResult {
success: false,
output: Value::Null,
error: Some(stderr.to_string()),
duration_ms: Some(duration_ms),
tokens_used: None,
})
}
}
}

View File

@@ -52,6 +52,9 @@ pub struct SkillManifest {
/// Trigger words for skill activation
#[serde(default)]
pub triggers: Vec<String>,
/// Required tools for skill execution (e.g., "bash", "web_search")
#[serde(default)]
pub tools: Vec<String>,
/// Whether the skill is enabled
#[serde(default = "default_enabled")]
pub enabled: bool,