feat: add session-scoped agent trace dashboard - #534
Conversation
wanglongan587
left a comment
There was a problem hiding this comment.
🤖 OCR 行级代码检视已完成：共 76 条意见，其中 74 条已作为行内评论提交。
| self.reload(existing.instance)?; | ||
| self.focus(&existing); | ||
| return Ok(*existing); |
There was a problem hiding this comment.
[HIGH] bug · apps/desktop/src-tauri/src/surface/service.rs L163-165
reload 失败时通过 ? 提前返回，但此前已成功的 trace grant（无论是 line 124 对 prepared_context 的首次授权，还是 line 160 对已存在 context_id 的重新授权）不会被撤销，导致 trace grant（会话级追踪数据访问权限）泄漏——操作失败后仍保留了对目标会话追踪数据的访问权限。建议在 reload 失败的返回路径上撤销当前生效的 context（context_id != prepared_context 分支撤销 context_id，其余分支撤销 prepared_context），或将已授权 context 的撤销逻辑统一抽取为 RAII（Resource Acquisition Is Initialization：将资源清理与对象生命周期绑定）守卫，避免逐路径手写撤销而遗漏。
建议修复
if let Err(error) = self.reload(existing.instance) {
let revoke_target = if context_id != prepared_context {
&context_id
} else {
&prepared_context
};
self.gateway.revoke_invocation_context(revoke_target);
return Err(error);
}
self.focus(&existing);
return Ok(*existing);
| Ok(sessions | ||
| .into_iter() | ||
| .filter(|session| session.workspace_id == selected.workspace_id) | ||
| .filter_map(|session| self.trace_binding_for_session(session).ok()) | ||
| .collect()) |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/agent_runtime.rs L309-313
trace_binding_for_session 的错误通过 .ok() 被静默丢弃，但其中包含两类性质不同的失败：(1) plugin_for_agent 返回 None——根据 for_agent 的文档注释（connection.rs 第 252–253 行），这是"正常的运行时状态"（会话可能比其插件存活更久），合理地应跳过；(2) workspace_cwd 失败——可能表示工作区行已被删除或其路径不再可用，属于真实数据异常。两类错误被同一 filter_map 吞没后，后者的故障对用户和调试者完全不可见。建议至少将 workspace_cwd 的失败以日志方式记录，使运行时异常不会被无声丢失。
建议修复
Ok(sessions
.into_iter()
.filter(|session| session.workspace_id == selected.workspace_id)
.filter_map(|session| {
self.trace_binding_for_session(session)
.map_err(|error| {
ora_warn!(error = %error, "failed to resolve trace session binding");
error
})
.ok()
})
.collect())
| label: session | ||
| .title | ||
| .as_ref() | ||
| .map(|title| title.as_str().to_string()) | ||
| .unwrap_or_else(|| "未命名会话".to_string()), |
There was a problem hiding this comment.
[MEDIUM] maintainability · crates/backend/src/agent_runtime.rs L338-342
"未命名会话" 是硬编码的中文字符串。label 字段经 TraceSessionGrant（context.rs 第 20 行文档明确写道 "label is the only user-facing session identity"）→ AuthorizedTrace.label → trace_metadata JSON → 插件 SDK TraceResource.label 直接传递到前端 UI。项目已具备 i18n（国际化）基础设施（packages/app-shell/src/i18n/i18n-instance.ts），非中文用户将看到未翻译的文本。建议使用语言无关的占位符（如空字符串或 "Untitled session"），由前端根据用户语言进行本地化展示。
建议修复
label: session
.title
.as_ref()
.map(|title| title.as_str().to_string())
.unwrap_or_default(),
| fn trace_binding_for_session( | ||
| &self, | ||
| session: Session, | ||
| ) -> Result<TraceSessionBinding, BackendError> { | ||
| let provider_plugin_id = self | ||
| .inner | ||
| .connections | ||
| .plugin_for_agent(&session.agent_ref) | ||
| .ok_or_else(|| { | ||
| runtime_internal( | ||
| "trace_provider_unavailable", | ||
| format!( | ||
| "{} no longer supplies an installed trace provider", | ||
| session.agent_ref | ||
| ), | ||
| ) | ||
| })?; | ||
| Ok(TraceSessionBinding { | ||
| ora_session_id: session.id.as_ref().to_string(), | ||
| provider_plugin_id, | ||
| provider_session_id: session.agent_session_id, | ||
| workspace_root: self.workspace_cwd(&session.workspace_id)?, | ||
| label: session | ||
| .title | ||
| .as_ref() | ||
| .map(|title| title.as_str().to_string()) | ||
| .unwrap_or_else(|| "未命名会话".to_string()), | ||
| updated_at_ms: session.audit_fields.updated_at, | ||
| }) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] performance · crates/backend/src/agent_runtime.rs L316-345
trace_session_catalog 先按 workspace_id 过滤，此后所有剩余会话共享同一个 workspace_id，但 trace_binding_for_session 对每个会话都调用 self.workspace_cwd(&session.workspace_id)。workspace_cwd 内部通过 resolve_workspace_cwd（task.rs 第 256–272 行）执行一次数据库查询（SqliteWorkspaceRepository::find_workspace）和文件系统路径解析，因此 N 个会话导致 N 次冗余查询。应在过滤后预先解析一次工作区根目录并复用。
建议修复
fn trace_binding_for_session(
&self,
session: Session,
workspace_root: &Path,
) -> Result<TraceSessionBinding, BackendError> {
let provider_plugin_id = self
.inner
.connections
.plugin_for_agent(&session.agent_ref)
.ok_or_else(|| {
runtime_internal(
"trace_provider_unavailable",
format!(
"{} no longer supplies an installed trace provider",
session.agent_ref
),
)
})?;
Ok(TraceSessionBinding {
ora_session_id: session.id.as_ref().to_string(),
provider_plugin_id,
provider_session_id: session.agent_session_id,
workspace_root: workspace_root.to_path_buf(),
label: session
.title
.as_ref()
.map(|title| title.as_str().to_string())
.unwrap_or_default(),
updated_at_ms: session.audit_fields.updated_at,
})
}
| let selected = self.find_session(ora_session_id)?; | ||
| let sessions = SqliteSessionRepository::new(self.inner.pool.clone()) | ||
| .list_sessions() | ||
| .map_err(|source| BackendError::internal("failed to list trace sessions", source))?; |
There was a problem hiding this comment.
[LOW] performance · crates/backend/src/agent_runtime.rs L305-308
list_sessions() 加载所有工作区的未删除会话后再在内存中按 workspace_id 过滤。SqliteSessionRepository 未提供按工作区范围查询的方法（crates/db/src/repository/session.rs 第 80 行的 SQL 无 WHERE workspace_id = ? 条件）。当会话数量随使用增长时，此查询模式的开销会不必要地增加。建议在仓库层增加按 workspace_id 过滤的查询变体，使查询在数据库侧完成过滤。
建议修复
let selected = self.find_session(ora_session_id)?;
let sessions = SqliteSessionRepository::new(self.inner.pool.clone())
.list_sessions_by_workspace(&selected.workspace_id)
.map_err(|source| BackendError::internal("failed to list trace sessions", source))?;
| if provider.locator.recursive { | ||
| discover_recursive( | ||
| &directory, | ||
| &file_name, | ||
| provider, | ||
| &session.label, | ||
| session.ora_session_id == grant.current_ora_session_id, | ||
| &mut context.trace_ids, | ||
| &mut traces, | ||
| )?; | ||
| } else { |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/trace.rs L170-180
trace.rs 第 271 行 add_trace_if_file 在 discover_recursive 中被调用时，传入的 containment_root 是递归发现的根目录。但 add_trace_if_file 第 291 行对 path 做了 canonicalize 后检查 canonical.starts_with(containment_root)，而 discover_recursive 传入的 root 参数是经过 canonicalize 的（trace.rs:159-160 let root = root.canonicalize().map_err(map_io)?;），但递归发现的子目录路径来自 entry.path()，它可能因符号链接而逃逸。discover_recursive 第 196 行检查了 file_type.is_symlink() 会 continue，但 read_dir 返回的条目可能包含指向目录的硬链接或绑定挂载点，is_symlink() 只会跳过符号链接。如果存在绑定挂载（bind mount）或硬链接目录，entry.path() 会指向该目录，其子文件会在后续迭代中被发现，而这些文件的 canonicalize 路径可能不在 containment_root 内，此时 add_trace_if_file 的 containment 检查会捕获并返回错误——这是防御性失败而非泄漏，所以不是安全问题。但递归发现中的绑定挂载场景可能导致 trace 列表整个失败而非跳过。这个限制可以在文档中说明。
建议修复
// 当前行为是安全的：containment 检查会在绑定挂载场景下返回错误而非泄漏。
// 若希望更健壮，可在 discover_recursive 中对子目录也做 canonicalize + starts_with 检查：
//
// if file_type.is_dir() {
// let sub = entry.path();
// if let Ok(canon) = sub.canonicalize() {
// if canon.starts_with(root) {
// pending.push((sub, depth + 1));
// }
// }
// }
| let mut file = File::open(&trace.path).map_err(map_io)?; | ||
| file.seek(SeekFrom::Start(offset)).map_err(map_io)?; | ||
| let available = metadata.len().saturating_sub(offset); | ||
| let wanted = available.min(max_bytes as u64) as usize; | ||
| let mut bytes = vec![0; wanted]; | ||
| file.read_exact(&mut bytes).map_err(map_io)?; |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L116-121
read 方法中，file.read_exact(&mut bytes) 在读取到 EOF（文件末尾）但未填满缓冲区时会返回 UnexpectedEof 错误。虽然前面通过 metadata.len() 计算了 wanted，但 metadata 的采样与实际读取之间存在 TOCTOU（time-of-check to time-of-use，即“检查时刻与使用时刻”竞态）窗口：如果在此间文件被截断，read_exact 会因实际可读字节少于 wanted 而失败。该错误被 map_io 统一映射为 "trace file is unavailable"，错误语义不够精确，插件页面无法区分文件不存在与文件被截断。建议改用 read（非精确读取）并返回实际读取字节数，与基于实际读取量的 eof 判断保持一致。
建议修复
let mut file = File::open(&trace.path).map_err(map_io)?;
file.seek(SeekFrom::Start(offset)).map_err(map_io)?;
let available = metadata.len().saturating_sub(offset);
let wanted = available.min(max_bytes as u64) as usize;
let mut bytes = vec![0; wanted];
let read_bytes = file.read(&mut bytes).map_err(map_io)?;
bytes.truncate(read_bytes);
| async move { | ||
| runtime.wait_for_exit().await; | ||
| contexts.revoke_generation(&plugin_id, generation); |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/runtime.rs L215-217
退出观察任务在 runtime.wait_for_exit().await 之后立即调用 contexts.revoke_generation(&plugin_id, generation)。由于 PluginTraceHost 持有自己的 contexts clone（共享同一 Arc<Mutex<...>>），如果此时还有正在处理中的 trace 请求（例如 list 正在执行递归目录扫描），revoke_generation 会与 with_context 竞争同一把 Mutex。由于 with_context 是同步的（在 handle 中不跨 .await），revoke_generation 会等待 with_context 释放锁后再执行，因此不会导致数据损坏或 panic。但需要注意：wait_for_exit 返回后进程已退出，此时正在处理的 trace 请求实际上是通过主机侧的 JSON-RPC 处理器执行的，与子进程无关。但子进程已退出意味着该插件 generation 的所有 context 应该被撤销。当前逻辑是安全的，因为 with_context 和 revoke_generation 都通过同一把 Mutex 序列化，不存在真正的并发冲突。
建议修复
// 当前逻辑安全：Mutex 序列化了 with_context 和 revoke_generation。
// 若未来 trace 处理变为异步或在锁外持有 context 引用，需重新评估。
// 可考虑在 revoke 后添加日志以辅助调试：
// ora_debug!(plugin_id = %plugin_id, generation = generation.0, "revoked trace contexts for stopped generation");
| fn map_io(error: std::io::Error) -> HostRequestError { | ||
| let kind = if error.kind() == std::io::ErrorKind::NotFound { | ||
| "trace_not_found" | ||
| } else { | ||
| "io" | ||
| }; | ||
| trace_error(kind, "trace file is unavailable") | ||
| } |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L358-365
map_io 函数将所有非 NotFound 的 IO 错误统一映射为 kind = "io"、消息 "trace file is unavailable"。这意味着 UnexpectedEof（文件被截断）、PermissionDenied（权限不足）等完全不同的情况都会被映射为相同的错误响应，调用方无法区分“文件不存在”、“文件被截断导致 cursor 过期”和“权限问题”。特别是 UnexpectedEof 应当映射为 stale_cursor 以允许插件页面重新执行 list 获取最新 trace 列表，而非笼统的 io 错误。
建议修复
fn map_io(error: std::io::Error) -> HostRequestError {
let kind = match error.kind() {
std::io::ErrorKind::NotFound => "trace_not_found",
std::io::ErrorKind::UnexpectedEof => "stale_cursor",
_ => "io",
};
trace_error(kind, "trace file is unavailable")
}
| fn verify_containment(trace: &AuthorizedTrace) -> Result<(), HostRequestError> { | ||
| let canonical = trace.path.canonicalize().map_err(map_io)?; | ||
| if canonical != trace.path || !canonical.starts_with(&trace.containment_root) { | ||
| return Err(trace_error( | ||
| "trace_unavailable", | ||
| "trace path changed after authorization", | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/trace.rs L274-283
verify_containment 第 276 行检查 canonical != trace.path，意图是检测 trace 文件在授权后被替换。但 trace.path 已在 add_trace_if_file 中通过 canonicalize 设置为规范路径（trace.rs:266 path: canonical）。canonicalize 在大多数平台上会解析符号链接并返回绝对路径。如果同一路径上的文件被删除后重新创建（不同 inode），canonicalize 仍然返回相同的路径字符串，因此 canonical != trace.path 不会触发——该检查无法检测文件替换。这本身不是安全漏洞（containment 仍然有效），但该检查名不副实，可能给维护者虚假的安全感。
建议修复
// 当前检查对文件替换场景效果有限，但 containment 检查仍然有效。
// 若要检测文件替换，可比较 inode/file handle，但跨平台实现复杂。
// 建议更新注释明确说明此检查的范围：
fn verify_containment(trace: &AuthorizedTrace) -> Result<(), HostRequestError> {
// Re-canonicalize to detect symlink redirection or path escape; does not
// detect same-path file replacement (use cursor for that).
let canonical = trace.path.canonicalize().map_err(map_io)?;
if !canonical.starts_with(&trace.containment_root) {
return Err(trace_error(
"trace_unavailable",
"trace path changed after authorization",
));
}
Ok(())
}
| Ok(sessions | ||
| .into_iter() | ||
| .filter(|session| session.workspace_id == selected.workspace_id) | ||
| .filter_map(|session| self.trace_binding_for_session(session).ok()) | ||
| .collect()) |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/agent_runtime.rs L309-313
trace_binding_for_session 的错误通过 .ok() 被静默丢弃，但其中包含两类性质不同的失败：(1) plugin_for_agent 返回 None——根据 for_agent 的文档注释（connection.rs 第 252–253 行），这是"正常的运行时状态"（会话可能比其插件存活更久），合理地应跳过；(2) workspace_cwd 失败——可能表示工作区行已被删除或其路径不再可用，属于真实数据异常。两类错误被同一 filter_map 吞没后，后者的故障对用户和调试者完全不可见。建议至少将 workspace_cwd 的失败以日志方式记录，使运行时异常不会被无声丢失。
建议修复
Ok(sessions
.into_iter()
.filter(|session| session.workspace_id == selected.workspace_id)
.filter_map(|session| {
self.trace_binding_for_session(session)
.map_err(|error| {
ora_warn!(error = %error, "failed to resolve trace session binding");
error
})
.ok()
})
.collect())
| label: session | ||
| .title | ||
| .as_ref() | ||
| .map(|title| title.as_str().to_string()) | ||
| .unwrap_or_else(|| "未命名会话".to_string()), |
There was a problem hiding this comment.
[MEDIUM] maintainability · crates/backend/src/agent_runtime.rs L338-342
"未命名会话" 是硬编码的中文字符串。label 字段经 TraceSessionGrant（context.rs 第 20 行文档明确写道 "label is the only user-facing session identity"）→ AuthorizedTrace.label → trace_metadata JSON → 插件 SDK TraceResource.label 直接传递到前端 UI。项目已具备 i18n（国际化）基础设施（packages/app-shell/src/i18n/i18n-instance.ts），非中文用户将看到未翻译的文本。建议使用语言无关的占位符（如空字符串或 "Untitled session"），由前端根据用户语言进行本地化展示。
建议修复
label: session
.title
.as_ref()
.map(|title| title.as_str().to_string())
.unwrap_or_default(),
| fn trace_binding_for_session( | ||
| &self, | ||
| session: Session, | ||
| ) -> Result<TraceSessionBinding, BackendError> { | ||
| let provider_plugin_id = self | ||
| .inner | ||
| .connections | ||
| .plugin_for_agent(&session.agent_ref) | ||
| .ok_or_else(|| { | ||
| runtime_internal( | ||
| "trace_provider_unavailable", | ||
| format!( | ||
| "{} no longer supplies an installed trace provider", | ||
| session.agent_ref | ||
| ), | ||
| ) | ||
| })?; | ||
| Ok(TraceSessionBinding { | ||
| ora_session_id: session.id.as_ref().to_string(), | ||
| provider_plugin_id, | ||
| provider_session_id: session.agent_session_id, | ||
| workspace_root: self.workspace_cwd(&session.workspace_id)?, | ||
| label: session | ||
| .title | ||
| .as_ref() | ||
| .map(|title| title.as_str().to_string()) | ||
| .unwrap_or_else(|| "未命名会话".to_string()), | ||
| updated_at_ms: session.audit_fields.updated_at, | ||
| }) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] performance · crates/backend/src/agent_runtime.rs L316-345
trace_session_catalog 先按 workspace_id 过滤，此后所有剩余会话共享同一个 workspace_id，但 trace_binding_for_session 对每个会话都调用 self.workspace_cwd(&session.workspace_id)。workspace_cwd 内部通过 resolve_workspace_cwd（task.rs 第 256–272 行）执行一次数据库查询（SqliteWorkspaceRepository::find_workspace）和文件系统路径解析，因此 N 个会话导致 N 次冗余查询。应在过滤后预先解析一次工作区根目录并复用。
建议修复
fn trace_binding_for_session(
&self,
session: Session,
workspace_root: &Path,
) -> Result<TraceSessionBinding, BackendError> {
let provider_plugin_id = self
.inner
.connections
.plugin_for_agent(&session.agent_ref)
.ok_or_else(|| {
runtime_internal(
"trace_provider_unavailable",
format!(
"{} no longer supplies an installed trace provider",
session.agent_ref
),
)
})?;
Ok(TraceSessionBinding {
ora_session_id: session.id.as_ref().to_string(),
provider_plugin_id,
provider_session_id: session.agent_session_id,
workspace_root: workspace_root.to_path_buf(),
label: session
.title
.as_ref()
.map(|title| title.as_str().to_string())
.unwrap_or_default(),
updated_at_ms: session.audit_fields.updated_at,
})
}
| let selected = self.find_session(ora_session_id)?; | ||
| let sessions = SqliteSessionRepository::new(self.inner.pool.clone()) | ||
| .list_sessions() | ||
| .map_err(|source| BackendError::internal("failed to list trace sessions", source))?; |
There was a problem hiding this comment.
[LOW] performance · crates/backend/src/agent_runtime.rs L305-308
list_sessions() 加载所有工作区的未删除会话后再在内存中按 workspace_id 过滤。SqliteSessionRepository 未提供按工作区范围查询的方法（crates/db/src/repository/session.rs 第 80 行的 SQL 无 WHERE workspace_id = ? 条件）。当会话数量随使用增长时，此查询模式的开销会不必要地增加。建议在仓库层增加按 workspace_id 过滤的查询变体，使查询在数据库侧完成过滤。
建议修复
let selected = self.find_session(ora_session_id)?;
let sessions = SqliteSessionRepository::new(self.inner.pool.clone())
.list_sessions_by_workspace(&selected.workspace_id)
.map_err(|source| BackendError::internal("failed to list trace sessions", source))?;
| let candidate = AgentRef::parse(plugin.name).ok()?; | ||
| (candidate == *agent_ref) | ||
| .then(|| PluginId::parse(&plugin.id).ok()) | ||
| .flatten() |
There was a problem hiding this comment.
[CRITICAL] bug · crates/backend/src/agent_runtime/connection.rs L243-246
plugin_for_agent 使用 plugin.name（仅名称段，如 "example"）构造 AgentRef，再与传入的 agent_ref 比较。但 agent_ref 是由 agent_identity() 通过 plugin_id.canonical() 生成的完整 <namespace>/<name> 形式（如 "official/example"）。AgentRef 的相等性是字符串逐字节比较，名称段缺少命名空间前缀，因此该比较永远不会命中，导致方法对所有已安装 agent 均返回 None。这会使 trace_binding_for_session 的调用方将正常会话误报为 provider 不可用（trace_provider_unavailable）。
修复方式应参照同文件中 agent_for_plugin 的写法，使用 plugin.id（即 canonical 形式）而非 plugin.name 来构造候选 AgentRef。
建议修复
let candidate = AgentRef::parse(&plugin.id).ok()?;
(candidate == *agent_ref)
.then(|| PluginId::parse(&plugin.id).ok())
.flatten()
| pub(crate) fn with_context<Result>( | ||
| &self, | ||
| context_id: &str, | ||
| consumer_plugin_id: &PluginId, | ||
| consumer_generation: PluginGenerationKey, | ||
| operation: impl FnOnce(&mut StoredContext) -> Result, | ||
| ) -> Option<Result> { | ||
| let mut contexts = self.inner.lock().unwrap_or_else(PoisonError::into_inner); | ||
| let context = contexts.get_mut(context_id)?; | ||
| if context.consumer_plugin_id != *consumer_plugin_id | ||
| || context.consumer_generation != consumer_generation | ||
| { | ||
| return None; | ||
| } | ||
| Some(operation(context)) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] performance · crates/plugin-lifecycle/src/context.rs L120-135
with_context 在持有 Mutex 守卫的整个生命周期内执行 operation 闭包。根据 trace.rs（第 48–60 行）中的调用方，该闭包会分派到 list、stat 或 read 方法，其中 read 执行同步文件 I/O（File::open、file.seek、file.read_exact），而 list 执行同步目录遍历（read_dir、canonicalize）。这意味着在磁盘 I/O 期间，整个进程的 PluginInvocationContexts 表对所有其他插件调用（invocation）都是锁定的。在并发 trace 请求下，这会导致线程饥饿（thread starvation，即线程长时间无法获取锁而无法推进工作），并可能在有界线程池上引发尾延迟（tail latency）问题。建议将授权检查（验证 consumer_plugin_id / consumer_generation）与实际 I/O 操作分离：在锁内做权限校验并克隆所需的 AuthorizedTrace 数据，释放锁后再执行文件读取。
建议修复
/// Verifies caller identity and returns a cloned snapshot of the matching context for
/// use outside the lock. Callers that need to mutate should use `with_context` instead.
pub(crate) fn authorize(
&self,
context_id: &str,
consumer_plugin_id: &PluginId,
consumer_generation: PluginGenerationKey,
) -> Option<StoredContext> {
let contexts = self.inner.lock().unwrap_or_else(PoisonError::into_inner);
let context = contexts.get(context_id)?;
if context.consumer_plugin_id != *consumer_plugin_id
|| context.consumer_generation != consumer_generation
{
return None;
}
Some(context.clone())
}
pub(crate) fn with_context<Result>(
&self,
context_id: &str,
consumer_plugin_id: &PluginId,
consumer_generation: PluginGenerationKey,
operation: impl FnOnce(&mut StoredContext) -> Result,
) -> Option<Result> {
let mut contexts = self.inner.lock().unwrap_or_else(PoisonError::into_inner);
let context = contexts.get_mut(context_id)?;
if context.consumer_plugin_id != *consumer_plugin_id
|| context.consumer_generation != consumer_generation
{
return None;
}
Some(operation(context))
}
| pub updated_at_ms: i64, | ||
| } | ||
|
|
||
| use crate::connection::PluginGenerationKey; |
There was a problem hiding this comment.
[LOW] maintainability · crates/plugin-lifecycle/src/context.rs L33
use crate::connection::PluginGenerationKey; 位于两个结构体定义之间（第 21–31 行的 TraceSessionGrant 与第 35–44 行的 AuthorizedTrace 之间），而非文件顶部与其他 use 语句（第 3–8 行）放在一起。这降低了可读性，且与 Rust 社区惯例（将 use 统一置于文件顶部）不一致。将此行移至顶部 import 块即可修复。
建议修复
// 将此行移至文件顶部第 3–8 行的 use 块中:
// use crate::connection::PluginGenerationKey;
// use ora_domain::PluginId;
// use ora_plugin_runtime::PluginTraceProvider;
// use std::collections::HashMap;
// use std::path::PathBuf;
// use std::sync::{Arc, Mutex, PoisonError};
// use uuid::Uuid;
| let provider = self | ||
| .plugin | ||
| .lifecycle() | ||
| .connection(&binding.provider_plugin_id) | ||
| .ok()?; |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/plugin_gateway.rs L127-131
filter_map 闭包中使用 .ok()? 将 connection() 返回的 ConnectionError 静默丢弃。当某个 provider 插件连接失败（如 NotRunning、Failed 等），该 binding 会被跳过且不留任何诊断痕迹；若全部连接失败，最终返回的错误信息 "no trace providers are currently available" 无法区分"没有配置 provider"与"provider 连接出错"两种情况，增加排查难度。建议收集连接错误并在全部失败时将其包含在返回错误中，或至少记录日志。
建议修复
let provider = match self
.plugin
.lifecycle()
.connection(&binding.provider_plugin_id)
{
Ok(provider) => provider,
Err(error) => {
tracing::warn!(
plugin_id = %binding.provider_plugin_id,
error = %error,
"skipping trace provider: connection unavailable"
);
return None;
}
};
| let current = self | ||
| .agent_runtime | ||
| .trace_session_binding(ora_session_id) | ||
| .map_err(|error| GatewayError::TraceContext(error.to_string()))?; | ||
| let mut bindings = self | ||
| .agent_runtime | ||
| .trace_session_catalog(ora_session_id) | ||
| .map_err(|error| GatewayError::TraceContext(error.to_string()))?; |
There was a problem hiding this comment.
[LOW] maintainability · crates/backend/src/plugin_gateway.rs L110-117
使用 error.to_string() 将 BackendError 展平为纯字符串存入 GatewayError::TraceContext(String)。BackendError 的 Display 实现仅输出 context 字段（见 error.rs 第 116 行），完整的 source chain（即底层根因）在 to_string() 时被丢弃，导致调用方无法通过 source() 链追溯真正的失败原因。BackendError 已实现 std::error::Error 并保留了 source，建议将 TraceContext 变体改为 #[source] BackendError 或至少在 to_string 时附加 source 信息以保留诊断链。
建议修复
let current = self
.agent_runtime
.trace_session_binding(ora_session_id)
.map_err(GatewayError::TraceContext)?;
let mut bindings = self
.agent_runtime
.trace_session_catalog(ora_session_id)
.map_err(GatewayError::TraceContext)?;
| if !bindings | ||
| .iter() | ||
| .any(|binding| binding.ora_session_id == current.ora_session_id) | ||
| { | ||
| bindings.push(current); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/plugin_gateway.rs L118-123
去重逻辑仅比较 ora_session_id 是否相等。当 catalog 中已存在同一 ora_session_id 的 binding 但其 provider_plugin_id、provider_session_id 等字段与 current（由 trace_session_binding 即时解析）不一致时（例如两次数据库读取之间发生了 agent 重新绑定到不同插件，即竞态条件——两个并发操作对共享状态的交错执行导致读取到不一致视图），current 这一权威且最新的 binding 会被静默丢弃，catalog 中的过时条目被保留，最终 grant 的 trace context 可能指向错误的 provider。建议当 ora_session_id 已存在时用 current 替换旧条目而非跳过。
建议修复
bindings.retain(|binding| binding.ora_session_id != current.ora_session_id);
bindings.push(current);
| fn validate_portable_relative_directory(directory: &str) -> Result<(), String> { | ||
| if directory.starts_with('/') || directory.starts_with('\\') || directory.contains('\\') { | ||
| return Err("trace provider directory must be a portable relative path".to_string()); | ||
| } | ||
| if directory | ||
| .split('/') | ||
| .any(|segment| segment.is_empty() || matches!(segment, "." | "..")) | ||
| { | ||
| return Err("trace provider directory contains an unsafe segment".to_string()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] security · crates/plugin-protocol/src/registration.rs L176-187
validate_portable_relative_directory 未拒绝 Windows 盘符路径（path traversal，即路径遍历攻击，指攻击者构造文件路径以访问预期目录之外的文件）。例如 C:/foo 或 C:foo 能通过全部现有检查：不以 / 或 \ 开头、不含 \，且按 / 拆分后的段（segment，路径中以分隔符切分的部分）["C:", "foo"] 均非空、非 ./..。在 Windows 上 PathBuf::join 会将盘符路径视为绝对路径并丢弃已有 root，使插件可将 trace 目录指向 home/workspace 之外的任意位置。trace.rs 中的 canonicalize() + starts_with(&root) 运行时校验能拦截此情况，但注册契约的文档注释声明其为 "path-safe registration contract"（路径安全注册契约），且防御纵深（defense in depth，即多层独立的安全校验叠加以降低单层失效的风险）原则要求注册层自身应完整。此外，该函数同样未拒绝嵌入的 null 字节（\0），可能导致 JSON 序列化或日志输出时出现截断或损坏。
建议修复
fn validate_portable_relative_directory(directory: &str) -> Result<(), String> {
if directory.starts_with('/') || directory.starts_with('\\') || directory.contains('\\') {
return Err("trace provider directory must be a portable relative path".to_string());
}
if directory.contains('\0') {
return Err("trace provider directory must not contain null bytes".to_string());
}
// Reject Windows drive-letter paths (e.g., "C:/foo" or "C:foo") which are
// treated as absolute by PathBuf::join on Windows, bypassing the declared root.
if directory.len() >= 2 {
let bytes = directory.as_bytes();
if bytes[0].is_ascii_alphabetic() && bytes[1] == b':' {
return Err("trace provider directory must not contain a drive letter".to_string());
}
}
if directory
.split('/')
.any(|segment| segment.is_empty() || matches!(segment, "." | ".."))
{
| let entries = value | ||
| .as_array() | ||
| .ok_or_else(|| "plugin registration field traceProviders must be an array".to_string())?; | ||
| let mut provider_ids = HashSet::new(); |
There was a problem hiding this comment.
[MEDIUM] performance · crates/plugin-protocol/src/registration.rs L100-103
parse_trace_providers 未对 trace provider（trace 提供者，声明 trace 文件位置的插件注册项）数量设置上限。每个 provider 在 trace.rs 的 discover 中会触发 canonicalize() 调用，且若 recursive: true 还会触发递归 read_dir 扫描。尽管 trace.rs 对单次扫描设有 MAX_DISCOVERY_ENTRIES（10 万）和 MAX_DISCOVERY_DEPTH（8 层）限制，但这些限制是按每个 provider 独立计算的。恶意插件可注册数千个 provider，导致资源耗尽（resource exhaustion，即不受控地消耗 CPU、内存或文件句柄等系统资源）。建议在注册解析阶段设置合理的上限（如 64 个）。
建议修复
let entries = value
.as_array()
.ok_or_else(|| "plugin registration field traceProviders must be an array".to_string())?;
const MAX_TRACE_PROVIDERS: usize = 64;
if entries.len() > MAX_TRACE_PROVIDERS {
return Err(format!(
"plugin registered {} trace providers, exceeding the limit of {}",
entries.len(),
MAX_TRACE_PROVIDERS
));
}
let mut provider_ids = HashSet::new();
| let (unsafe_inner, _inbound) = test_inner(); | ||
| let error = handle_message( | ||
| &unsafe_inner, | ||
| json!({ | ||
| "jsonrpc": "2.0", | ||
| "method": "ora/register", | ||
| "params": { | ||
| "methods": [], | ||
| "traceProviders": [{ | ||
| "providerId": "bad", | ||
| "format": "trace.v1", | ||
| "locator": { | ||
| "root": "home", | ||
| "directory": "../private", | ||
| "fileNameTemplate": "{provider_session_id}.jsonl" | ||
| } | ||
| }] | ||
| } | ||
| }), | ||
| ) | ||
| .await | ||
| .expect_err("reject traversal"); | ||
| assert_eq!(error, "trace provider directory contains an unsafe segment"); | ||
| } |
There was a problem hiding this comment.
[HIGH] test · crates/plugin-runtime/src/tests.rs L203-226
该测试是 plugin-protocol crate 中路径安全校验逻辑（validate_portable_relative_directory 与 validate_trace_file_name_template）的唯一测试覆盖，但实际只验证了多种拒绝路径中的一种（.. 段），其余安全关键分支完全未覆盖。生产代码中 validate_portable_relative_directory 还会拒绝绝对路径（以 / 开头）、包含反斜杠的路径、空段（如 foo//bar），这些分支均无测试。更严重的是 validate_trace_file_name_template——它负责拒绝文件名模板中的路径分隔符和占位符注入——完全没有被任何负向测试用例覆盖。由于 trace provider 最终会访问用户主目录下的文件系统，校验逻辑的任何回归都可能导致恶意插件读取任意文件。建议补充以下场景的负向测试：绝对路径（/etc/passwd）、反斜杠路径（C:\Users）、空段、模板含路径分隔符、模板缺少占位符、模板仅有占位符。
建议修复
// 除了现有的 ../private 拒绝测试外，建议补充以下场景：
// 1. 绝对路径被拒绝
let (abs_inner, _) = test_inner();
let err = handle_message(
&abs_inner,
json!({
"jsonrpc": "2.0",
"method": "ora/register",
"params": {
"methods": [],
"traceProviders": [{
"providerId": "bad",
"format": "trace.v1",
"locator": {
"root": "home",
"directory": "/etc/passwd",
"fileNameTemplate": "{provider_session_id}.jsonl"
}
}]
}
}),
)
.await
.expect_err("reject absolute path");
assert_eq!(err, "trace provider directory must be a portable relative path");
// 2. fileNameTemplate 含路径分隔符被拒绝
let (tmpl_inner, _) = test_inner();
let err = handle_message(
&tmpl_inner,
json!({
"jsonrpc": "2.0",
"method": "ora/register",
"params": {
"methods": [],
"traceProviders": [{
"providerId": "bad",
"format": "trace.v1",
"locator": {
"root": "home",
"directory": "logs",
"fileNameTemplate": "../{provider_session_id}.jsonl"
}
}]
}
}),
)
.await
.expect_err("reject template with path separator");
assert_eq!(
err,
"trace provider fileNameTemplate must be one file name containing exactly one provider session placeholder"
);
| pub fn bind_workbench_context(&self, instance: SurfaceInstanceId, context: String) -> bool { | ||
| let mut inner = self.lock(); | ||
| let Some(record) = inner.instances.get(&instance) else { | ||
| return false; | ||
| }; | ||
| if !matches!(record.definition.source, SurfaceSource::Workbench(_)) { | ||
| return false; | ||
| } | ||
| inner.workbench_contexts.insert(instance, context); | ||
| true | ||
| } |
There was a problem hiding this comment.
[MEDIUM] security · crates/surface/src/registry.rs L223-233
bind_workbench_context 使用 HashMap::insert 静默覆盖已有 context，调用方无法感知覆盖是否发生。当前调用路径中（service.rs 第 181 行），此方法仅在新打开的实例上调用，此时不应已有 context，因此实际风险较低。但若未来调用顺序变化（例如 workbench_context 先于 bind_workbench_context 被调用），旧 context 会被静默替换且 gateway 侧不会收到 revoke 通知，导致授权泄漏（即旧 context 在网关层仍然有效，但注册表中已丢失引用）。建议返回是否发生了覆盖（如返回 Option 表示被替换的旧值），或在插入前检查并拒绝覆盖。
建议修复
pub fn bind_workbench_context(&self, instance: SurfaceInstanceId, context: String) -> Option<Option<String>> {
let mut inner = self.lock();
let Some(record) = inner.instances.get(&instance) else {
return None;
};
if !matches!(record.definition.source, SurfaceSource::Workbench(_)) {
return None;
}
Some(inner.workbench_contexts.insert(instance, context))
}
| if let Some(context) = inner.workbench_contexts.get(&instance) { | ||
| return Some(context.clone()); | ||
| } | ||
| let context = issue(); | ||
| inner.workbench_contexts.insert(instance, context.clone()); | ||
| Some(context) |
There was a problem hiding this comment.
[MEDIUM] other · crates/surface/src/registry.rs L214-219
workbench_context 在持有 registry Mutex 期间调用用户传入的 issue 闭包。当前调用方（workbench_bridge.rs 第 152 行）传入的闭包仅生成 UUID 并写入另一个 Mutex 保护的 HashMap，开销很小，不会造成实际问题。但作为公开 API，持有锁期间执行外部代码是一个潜在的死锁/竞争风险（deadlock：两个或多个执行流互相等待对方释放锁，导致永久阻塞）——若未来调用方传入涉及 I/O 或获取其他锁的闭包，会阻塞所有并发注册表操作。建议先在锁内检查缓存命中，未命中时释放锁后调用 issue，再重新加锁写入。
建议修复
if let Some(context) = inner.workbench_contexts.get(&instance) {
return Some(context.clone());
}
drop(inner);
let context = issue();
let mut inner = self.lock();
if let Some(existing) = inner.workbench_contexts.get(&instance) {
return Some(existing.clone());
}
inner.workbench_contexts.insert(instance, context.clone());
Some(context)
| pub fn take_workbench_context(&self, instance: SurfaceInstanceId) -> Option<String> { | ||
| self.lock().workbench_contexts.remove(&instance) | ||
| } |
There was a problem hiding this comment.
[LOW] maintainability · crates/surface/src/registry.rs L236-238
take_workbench_context 未像 workbench_context 和 bind_workbench_context 那样检查 SurfaceSource::Workbench，存在一致性缺陷。当前功能上无害——非 workbench 实例在 workbench_contexts 中没有条目，remove 返回 None。但缺少此守卫意味着如果未来其他代码路径为非 workbench 实例意外写入 context，此方法会静默移除它而不报错，违背了仅 workbench 实例拥有 context 的设计意图。建议补充来源检查以保持三个方法的一致性。
建议修复
pub fn take_workbench_context(&self, instance: SurfaceInstanceId) -> Option<String> {
let mut inner = self.lock();
let record = inner.instances.get(&instance)?;
if !matches!(record.definition.source, SurfaceSource::Workbench(_)) {
return None;
}
inner.workbench_contexts.remove(&instance)
}
| onClick={() => { | ||
| const { embeddedSupported, setSidePanelInstance, applyEvent } = | ||
| useSurfaceStore.getState(); | ||
| const target = embeddedSupported ? "embedded" : "windowed"; | ||
| void surfaces | ||
| .openSessionTraceDashboard(sessionId, target) | ||
| .then((record) => { | ||
| applyEvent({ type: "opened", ...record }); | ||
| if (record.target === "embedded") { | ||
| setSidePanelInstance(record.instance); | ||
| } | ||
| }) | ||
| .catch(() => toast.error(t("surface.openFailed"))); | ||
| }} |
There was a problem hiding this comment.
[MEDIUM] maintainability · packages/app-shell/src/features/surface/session-dashboard-button.tsx L33-46
该 onClick 处理器将与同一功能目录下 useOpenSurface hook 中已抽取的 open-apply-catch 逻辑高度重复（读取 store 状态 → 判断 target → 调用平台方法 → applyEvent → 条件性 setSidePanelInstance → catch 弹 toast）。此外，此处使用 .then().catch() Promise 链，而 useOpenSurface 使用 async/await + try/catch，两者风格不一致。如果后续需要修改后处理逻辑（例如增加对过期会话的守卫、调整错误处理方式），必须在两处分别修改，容易遗漏导致行为不一致。建议将该公共逻辑抽取为共享 helper 或泛化 useOpenSurface hook，使其接受平台 open 方法作为参数，从而复用同一套后处理与错误处理路径。
建议修复
// 方案一：泛化 useOpenSurface，使其接受 open 函数参数
// use-open-surface.ts 中新增：
// export function useOpenSurfaceRecord() {
// const { t } = useTranslation();
// return useCallback(async (
// open: (target: SurfaceTarget) => Promise<SurfaceRecord>,
// ) => {
// const { embeddedSupported, setSidePanelInstance, applyEvent } =
// useSurfaceStore.getState();
// const target = embeddedSupported ? "embedded" : "windowed";
// try {
// const record = await open(target);
// applyEvent({ type: "opened", ...record });
// if (record.target === "embedded") {
// setSidePanelInstance(record.instance);
// }
// } catch {
// toast.error(t("surface.openFailed"));
// }
// }, [t]);
// }
// session-dashboard-button.tsx 中使用：
// const openSurfaceRecord = useOpenSurfaceRecord();
// onClick={() =>
// void openSurfaceRecord((target) =>
// surfaces.openSessionTraceDashboard(sessionId, target),
// )
// }
| #[test] | ||
| fn binds_and_takes_workbench_invocation_context() { | ||
| let registry = SurfaceRegistry::default(); | ||
| let (record, _) = registry | ||
| .open(workbench_definition("acme.panel"), MountTarget::Windowed) | ||
| .expect("open"); | ||
|
|
||
| let first = registry.workbench_context(record.instance, || "context-1".to_string()); | ||
| let second = registry.workbench_context(record.instance, || "context-2".to_string()); | ||
| let taken = registry.take_workbench_context(record.instance); | ||
| let after_take = registry.workbench_context(record.instance, || "context-3".to_string()); | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
[MEDIUM] test · crates/surface/src/registry.rs L587-599
测试方法名为 binds_and_takes_workbench_invocation_context，但实际仅调用了 workbench_context（懒初始化路径）和 take_workbench_context，从未调用 bind_workbench_context。作为公开 API 且涉及授权（authorization，即验证某调用方是否有权执行操作）的 bind_workbench_context 完全没有测试覆盖，存在维护性风险——未来修改该方法时无法被测试捕获回归（regression，即代码修改后原本正确的功能意外失效）。此外，三个新方法的 SurfaceSource::Workbench 守卫（guard，即前置条件检查）也未被任何测试验证，例如对 RemoteSite 实例调用应返回 None/false。建议补充对 bind_workbench_context 的测试，包括：正常绑定路径、未知实例返回 false、非 workbench 来源返回 false，以及 bind_workbench_context 后 workbench_context 返回已绑定值且不调用 issue 闭包。
建议修复
#[test]
fn binds_and_takes_workbench_invocation_context() {
let registry = SurfaceRegistry::default();
let (record, _) = registry
.open(workbench_definition("acme.panel"), MountTarget::Windowed)
.expect("open");
let first = registry.workbench_context(record.instance, || "context-1".to_string());
let second = registry.workbench_context(record.instance, || "context-2".to_string());
let taken = registry.take_workbench_context(record.instance);
let after_take = registry.workbench_context(record.instance, || "context-3".to_string());
assert_eq!(
(first, second, taken, after_take),
(
Some("context-1".to_string()),
Some("context-1".to_string()),
Some("context-1".to_string()),
Some("context-3".to_string()),
)
);
}
/// `bind_workbench_context` installs context that `workbench_context` returns without issuing.
#[test]
fn binds_context_and_lazy_read_returns_it() {
let registry = SurfaceRegistry::default();
let (record, _) = registry
.open(workbench_definition("acme.panel"), MountTarget::Windowed)
.expect("open");
let bound = registry.bind_workbench_context(record.instance, "host-ctx".to_string());
let read = registry.workbench_context(record.instance, || panic!("issue should not be called"));
assert_eq!((bound, read), (true, Some("host-ctx".to_string())));
}
/// Non-workbench sources and unknown instances are rejected.
#[test]
fn refuses_context_for_non_workbench_and_unknown() {
let registry = SurfaceRegistry::default();
let (remote, _) = registry
.open(definition("acme.hub"), MountTarget::Windowed)
.expect("open");
let ctx = registry.workbench_context(remote.instance, || "ctx".to_string());
let bound = registry.bind_workbench_context(remote.instance, "ctx".to_string());
assert_eq!((ctx, bound), (None, false));
}
| let max_bytes = required_u64(params, "max_bytes")? as usize; | ||
| if max_bytes == 0 || max_bytes > MAX_TRACE_CHUNK_BYTES { |
There was a problem hiding this comment.
[HIGH] bug · crates/plugin-lifecycle/src/trace.rs L89-90
max_bytes 在 as usize 转型后才进行范围校验，在 32 位平台上会因截断（truncation，即高位被丢弃）而绕过上限检查。required_u64 返回 u64，当值如 0x1_0000_0004（即 4GiB+4）在 32 位 usize 上被截断为 4，不仅通过了 > MAX_TRACE_CHUNK_BYTES 校验，还会导致实际读取量远小于请求值且无错误提示。应在转型前以 u64 值进行校验。
建议修复
let max_bytes_u64 = required_u64(params, "max_bytes")?;
if max_bytes_u64 == 0 || max_bytes_u64 > MAX_TRACE_CHUNK_BYTES as u64 {
return Err(invalid_params(format!(
"max_bytes must be between 1 and {MAX_TRACE_CHUNK_BYTES}"
)));
}
let max_bytes = max_bytes_u64 as usize;
| .with_context( | ||
| context_id, | ||
| &self.caller_plugin_id, | ||
| self.caller_generation, | ||
| |context| match method { | ||
| TRACE_LIST_METHOD => self.list(context), | ||
| TRACE_STAT_METHOD => self.stat(context, ¶ms), | ||
| TRACE_READ_METHOD => self.read(context, ¶ms), | ||
| _ => Err(HostRequestError::method_not_found(method)), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
[HIGH] performance · crates/plugin-lifecycle/src/trace.rs L49-59
handle 是 async fn，但 with_context 在持锁状态下同步调用闭包，而闭包内（list/stat/read）执行的是阻塞式文件系统 I/O（canonicalize、read_dir、File::open、seek、read_exact）。这带来两个问题：其一，异步执行器线程被阻塞，在单线程运行时（single-threaded runtime）上会导致整个事件循环停滞；其二，PluginInvocationContexts 的 Mutex 在整个 I/O 期间被持有，导致同一进程中所有插件的 trace 请求被串行化。同 crate 的 storage.rs 已用 tokio::task::spawn_blocking 将阻塞 I/O 移出异步线程，此处应遵循相同模式。
建议修复
// 在锁内仅提取所需数据后立即释放锁，再通过 spawn_blocking 执行 I/O：
// 1. with_context 中克隆出 trace 授权信息（grant / AuthorizedTrace 的 path 等）
// 2. drop 锁
// 3. tokio::task::spawn_blocking(move || { /* 执行 canonicalize / File::open / read 等 */ }).await
// 4. 若 list 需要写回 context.traces，再获取一次锁写入结果
| verify_containment(trace)?; | ||
| let metadata = trace.path.metadata().map_err(map_io)?; |
There was a problem hiding this comment.
[MEDIUM] security · crates/plugin-lifecycle/src/trace.rs L99-100
read 方法中 verify_containment 对 trace.path 重新规范化（canonicalize，即解析所有符号链接得到绝对真实路径）并验证路径未越界，但随后 File::open(&trace.path) 使用原始路径打开文件。在这两步之间存在 TOCTOU（Time-of-Check-to-Time-of-Use，检查与使用之间的竞态条件）窗口：若具有文件系统访问权限的攻击者在检查通过后、打开文件前将 trace.path 处的文件替换为指向 containment root 之外的符号链接（symlink），File::open 会跟随该链接读取宿主机上的非授权文件。修复方式是先 File::open 获取文件描述符，再对已打开的句柄调用 file.metadata()（底层为 fstat，不跟随符号链接）验证文件未发生替换。
建议修复
// 先打开文件获取句柄，再用 fstat（file.metadata）验证，消除 TOCTOU 窗口
let mut file = File::open(&trace.path).map_err(map_io)?;
let metadata = file.metadata().map_err(map_io)?;
let canonical = trace.path.canonicalize().map_err(map_io)?;
if canonical != trace.path || !canonical.starts_with(&trace.containment_root) {
return Err(trace_error(
"trace_unavailable",
"trace path changed after authorization",
));
}
| if depth > MAX_DISCOVERY_DEPTH || visited >= MAX_DISCOVERY_ENTRIES { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[LOW] performance · crates/plugin-lifecycle/src/trace.rs L209-211
当 visited >= MAX_DISCOVERY_ENTRIES 时使用 continue，这会从 pending 弹出下一个目录并再次命中同样的条件后跳过，直到 pending 耗尽为止——产生无意义的空转循环。应改为 break 直接终止整个 while let 遍历。
建议修复
if depth > MAX_DISCOVERY_DEPTH || visited >= MAX_DISCOVERY_ENTRIES {
break;
}
| let provider = self | ||
| .plugin | ||
| .lifecycle() | ||
| .connection(&binding.provider_plugin_id) | ||
| .ok()?; |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/plugin_gateway.rs L127-131
filter_map 闭包中使用 .ok()? 将 connection() 返回的 ConnectionError 静默丢弃。当某个 provider 插件连接失败（如 NotRunning、Failed 等），该 binding 会被跳过且不留任何诊断痕迹；若全部连接失败，最终返回的错误信息 "no trace providers are currently available" 无法区分"没有配置 provider"与"provider 连接出错"两种情况，增加排查难度。建议收集连接错误并在全部失败时将其包含在返回错误中，或至少记录日志。
建议修复
let provider = match self
.plugin
.lifecycle()
.connection(&binding.provider_plugin_id)
{
Ok(provider) => provider,
Err(error) => {
tracing::warn!(
plugin_id = %binding.provider_plugin_id,
error = %error,
"skipping trace provider: connection unavailable"
);
return None;
}
};
| let current = self | ||
| .agent_runtime | ||
| .trace_session_binding(ora_session_id) | ||
| .map_err(|error| GatewayError::TraceContext(error.to_string()))?; | ||
| let mut bindings = self | ||
| .agent_runtime | ||
| .trace_session_catalog(ora_session_id) | ||
| .map_err(|error| GatewayError::TraceContext(error.to_string()))?; |
There was a problem hiding this comment.
[LOW] maintainability · crates/backend/src/plugin_gateway.rs L110-117
使用 error.to_string() 将 BackendError 展平为纯字符串存入 GatewayError::TraceContext(String)。BackendError 的 Display 实现仅输出 context 字段（见 error.rs 第 116 行），完整的 source chain（即底层根因）在 to_string() 时被丢弃，导致调用方无法通过 source() 链追溯真正的失败原因。BackendError 已实现 std::error::Error 并保留了 source，建议将 TraceContext 变体改为 #[source] BackendError 或至少在 to_string 时附加 source 信息以保留诊断链。
建议修复
let current = self
.agent_runtime
.trace_session_binding(ora_session_id)
.map_err(GatewayError::TraceContext)?;
let mut bindings = self
.agent_runtime
.trace_session_catalog(ora_session_id)
.map_err(GatewayError::TraceContext)?;
| if !bindings | ||
| .iter() | ||
| .any(|binding| binding.ora_session_id == current.ora_session_id) | ||
| { | ||
| bindings.push(current); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] bug · crates/backend/src/plugin_gateway.rs L118-123
去重逻辑仅比较 ora_session_id 是否相等。当 catalog 中已存在同一 ora_session_id 的 binding 但其 provider_plugin_id、provider_session_id 等字段与 current（由 trace_session_binding 即时解析）不一致时（例如两次数据库读取之间发生了 agent 重新绑定到不同插件，即竞态条件——两个并发操作对共享状态的交错执行导致读取到不一致视图），current 这一权威且最新的 binding 会被静默丢弃，catalog 中的过时条目被保留，最终 grant 的 trace context 可能指向错误的 provider。建议当 ora_session_id 已存在时用 current 替换旧条目而非跳过。
建议修复
bindings.retain(|binding| binding.ora_session_id != current.ora_session_id);
bindings.push(current);
| fn validate_portable_relative_directory(directory: &str) -> Result<(), String> { | ||
| if directory.starts_with('/') || directory.starts_with('\\') || directory.contains('\\') { | ||
| return Err("trace provider directory must be a portable relative path".to_string()); | ||
| } | ||
| if directory | ||
| .split('/') | ||
| .any(|segment| segment.is_empty() || matches!(segment, "." | "..")) | ||
| { | ||
| return Err("trace provider directory contains an unsafe segment".to_string()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] security · crates/plugin-protocol/src/registration.rs L176-187
validate_portable_relative_directory 未拒绝 Windows 盘符路径（path traversal，即路径遍历攻击，指攻击者构造文件路径以访问预期目录之外的文件）。例如 C:/foo 或 C:foo 能通过全部现有检查：不以 / 或 \ 开头、不含 \，且按 / 拆分后的段（segment，路径中以分隔符切分的部分）["C:", "foo"] 均非空、非 ./..。在 Windows 上 PathBuf::join 会将盘符路径视为绝对路径并丢弃已有 root，使插件可将 trace 目录指向 home/workspace 之外的任意位置。trace.rs 中的 canonicalize() + starts_with(&root) 运行时校验能拦截此情况，但注册契约的文档注释声明其为 "path-safe registration contract"（路径安全注册契约），且防御纵深（defense in depth，即多层独立的安全校验叠加以降低单层失效的风险）原则要求注册层自身应完整。此外，该函数同样未拒绝嵌入的 null 字节（\0），可能导致 JSON 序列化或日志输出时出现截断或损坏。
建议修复
fn validate_portable_relative_directory(directory: &str) -> Result<(), String> {
if directory.starts_with('/') || directory.starts_with('\\') || directory.contains('\\') {
return Err("trace provider directory must be a portable relative path".to_string());
}
if directory.contains('\0') {
return Err("trace provider directory must not contain null bytes".to_string());
}
// Reject Windows drive-letter paths (e.g., "C:/foo" or "C:foo") which are
// treated as absolute by PathBuf::join on Windows, bypassing the declared root.
if directory.len() >= 2 {
let bytes = directory.as_bytes();
if bytes[0].is_ascii_alphabetic() && bytes[1] == b':' {
return Err("trace provider directory must not contain a drive letter".to_string());
}
}
if directory
.split('/')
.any(|segment| segment.is_empty() || matches!(segment, "." | ".."))
{
| let entries = value | ||
| .as_array() | ||
| .ok_or_else(|| "plugin registration field traceProviders must be an array".to_string())?; | ||
| let mut provider_ids = HashSet::new(); |
There was a problem hiding this comment.
[MEDIUM] performance · crates/plugin-protocol/src/registration.rs L100-103
parse_trace_providers 未对 trace provider（trace 提供者，声明 trace 文件位置的插件注册项）数量设置上限。每个 provider 在 trace.rs 的 discover 中会触发 canonicalize() 调用，且若 recursive: true 还会触发递归 read_dir 扫描。尽管 trace.rs 对单次扫描设有 MAX_DISCOVERY_ENTRIES（10 万）和 MAX_DISCOVERY_DEPTH（8 层）限制，但这些限制是按每个 provider 独立计算的。恶意插件可注册数千个 provider，导致资源耗尽（resource exhaustion，即不受控地消耗 CPU、内存或文件句柄等系统资源）。建议在注册解析阶段设置合理的上限（如 64 个）。
建议修复
let entries = value
.as_array()
.ok_or_else(|| "plugin registration field traceProviders must be an array".to_string())?;
const MAX_TRACE_PROVIDERS: usize = 64;
if entries.len() > MAX_TRACE_PROVIDERS {
return Err(format!(
"plugin registered {} trace providers, exceeding the limit of {}",
entries.len(),
MAX_TRACE_PROVIDERS
));
}
let mut provider_ids = HashSet::new();
| let (unsafe_inner, _inbound) = test_inner(); | ||
| let error = handle_message( | ||
| &unsafe_inner, | ||
| json!({ | ||
| "jsonrpc": "2.0", | ||
| "method": "ora/register", | ||
| "params": { | ||
| "methods": [], | ||
| "traceProviders": [{ | ||
| "providerId": "bad", | ||
| "format": "trace.v1", | ||
| "locator": { | ||
| "root": "home", | ||
| "directory": "../private", | ||
| "fileNameTemplate": "{provider_session_id}.jsonl" | ||
| } | ||
| }] | ||
| } | ||
| }), | ||
| ) | ||
| .await | ||
| .expect_err("reject traversal"); | ||
| assert_eq!(error, "trace provider directory contains an unsafe segment"); | ||
| } |
There was a problem hiding this comment.
[HIGH] test · crates/plugin-runtime/src/tests.rs L203-226
该测试是 plugin-protocol crate 中路径安全校验逻辑（validate_portable_relative_directory 与 validate_trace_file_name_template）的唯一测试覆盖，但实际只验证了多种拒绝路径中的一种（.. 段），其余安全关键分支完全未覆盖。生产代码中 validate_portable_relative_directory 还会拒绝绝对路径（以 / 开头）、包含反斜杠的路径、空段（如 foo//bar），这些分支均无测试。更严重的是 validate_trace_file_name_template——它负责拒绝文件名模板中的路径分隔符和占位符注入——完全没有被任何负向测试用例覆盖。由于 trace provider 最终会访问用户主目录下的文件系统，校验逻辑的任何回归都可能导致恶意插件读取任意文件。建议补充以下场景的负向测试：绝对路径（/etc/passwd）、反斜杠路径（C:\Users）、空段、模板含路径分隔符、模板缺少占位符、模板仅有占位符。
建议修复
// 除了现有的 ../private 拒绝测试外，建议补充以下场景：
// 1. 绝对路径被拒绝
let (abs_inner, _) = test_inner();
let err = handle_message(
&abs_inner,
json!({
"jsonrpc": "2.0",
"method": "ora/register",
"params": {
"methods": [],
"traceProviders": [{
"providerId": "bad",
"format": "trace.v1",
"locator": {
"root": "home",
"directory": "/etc/passwd",
"fileNameTemplate": "{provider_session_id}.jsonl"
}
}]
}
}),
)
.await
.expect_err("reject absolute path");
assert_eq!(err, "trace provider directory must be a portable relative path");
// 2. fileNameTemplate 含路径分隔符被拒绝
let (tmpl_inner, _) = test_inner();
let err = handle_message(
&tmpl_inner,
json!({
"jsonrpc": "2.0",
"method": "ora/register",
"params": {
"methods": [],
"traceProviders": [{
"providerId": "bad",
"format": "trace.v1",
"locator": {
"root": "home",
"directory": "logs",
"fileNameTemplate": "../{provider_session_id}.jsonl"
}
}]
}
}),
)
.await
.expect_err("reject template with path separator");
assert_eq!(
err,
"trace provider fileNameTemplate must be one file name containing exactly one provider session placeholder"
);
| pub fn bind_workbench_context(&self, instance: SurfaceInstanceId, context: String) -> bool { | ||
| let mut inner = self.lock(); | ||
| let Some(record) = inner.instances.get(&instance) else { | ||
| return false; | ||
| }; | ||
| if !matches!(record.definition.source, SurfaceSource::Workbench(_)) { | ||
| return false; | ||
| } | ||
| inner.workbench_contexts.insert(instance, context); | ||
| true | ||
| } |
There was a problem hiding this comment.
[MEDIUM] security · crates/surface/src/registry.rs L223-233
bind_workbench_context 使用 HashMap::insert 静默覆盖已有 context，调用方无法感知覆盖是否发生。当前调用路径中（service.rs 第 181 行），此方法仅在新打开的实例上调用，此时不应已有 context，因此实际风险较低。但若未来调用顺序变化（例如 workbench_context 先于 bind_workbench_context 被调用），旧 context 会被静默替换且 gateway 侧不会收到 revoke 通知，导致授权泄漏（即旧 context 在网关层仍然有效，但注册表中已丢失引用）。建议返回是否发生了覆盖（如返回 Option 表示被替换的旧值），或在插入前检查并拒绝覆盖。
建议修复
pub fn bind_workbench_context(&self, instance: SurfaceInstanceId, context: String) -> Option<Option<String>> {
let mut inner = self.lock();
let Some(record) = inner.instances.get(&instance) else {
return None;
};
if !matches!(record.definition.source, SurfaceSource::Workbench(_)) {
return None;
}
Some(inner.workbench_contexts.insert(instance, context))
}
| if let Some(context) = inner.workbench_contexts.get(&instance) { | ||
| return Some(context.clone()); | ||
| } | ||
| let context = issue(); | ||
| inner.workbench_contexts.insert(instance, context.clone()); | ||
| Some(context) |
There was a problem hiding this comment.
[MEDIUM] other · crates/surface/src/registry.rs L214-219
workbench_context 在持有 registry Mutex 期间调用用户传入的 issue 闭包。当前调用方（workbench_bridge.rs 第 152 行）传入的闭包仅生成 UUID 并写入另一个 Mutex 保护的 HashMap，开销很小，不会造成实际问题。但作为公开 API，持有锁期间执行外部代码是一个潜在的死锁/竞争风险（deadlock：两个或多个执行流互相等待对方释放锁，导致永久阻塞）——若未来调用方传入涉及 I/O 或获取其他锁的闭包，会阻塞所有并发注册表操作。建议先在锁内检查缓存命中，未命中时释放锁后调用 issue，再重新加锁写入。
建议修复
if let Some(context) = inner.workbench_contexts.get(&instance) {
return Some(context.clone());
}
drop(inner);
let context = issue();
let mut inner = self.lock();
if let Some(existing) = inner.workbench_contexts.get(&instance) {
return Some(existing.clone());
}
inner.workbench_contexts.insert(instance, context.clone());
Some(context)
| pub fn take_workbench_context(&self, instance: SurfaceInstanceId) -> Option<String> { | ||
| self.lock().workbench_contexts.remove(&instance) | ||
| } |
There was a problem hiding this comment.
[LOW] maintainability · crates/surface/src/registry.rs L236-238
take_workbench_context 未像 workbench_context 和 bind_workbench_context 那样检查 SurfaceSource::Workbench，存在一致性缺陷。当前功能上无害——非 workbench 实例在 workbench_contexts 中没有条目，remove 返回 None。但缺少此守卫意味着如果未来其他代码路径为非 workbench 实例意外写入 context，此方法会静默移除它而不报错，违背了仅 workbench 实例拥有 context 的设计意图。建议补充来源检查以保持三个方法的一致性。
建议修复
pub fn take_workbench_context(&self, instance: SurfaceInstanceId) -> Option<String> {
let mut inner = self.lock();
let record = inner.instances.get(&instance)?;
if !matches!(record.definition.source, SurfaceSource::Workbench(_)) {
return None;
}
inner.workbench_contexts.remove(&instance)
}
| onClick={() => { | ||
| const { embeddedSupported, setSidePanelInstance, applyEvent } = | ||
| useSurfaceStore.getState(); | ||
| const target = embeddedSupported ? "embedded" : "windowed"; | ||
| void surfaces | ||
| .openSessionTraceDashboard(sessionId, target) | ||
| .then((record) => { | ||
| applyEvent({ type: "opened", ...record }); | ||
| if (record.target === "embedded") { | ||
| setSidePanelInstance(record.instance); | ||
| } | ||
| }) | ||
| .catch(() => toast.error(t("surface.openFailed"))); | ||
| }} |
There was a problem hiding this comment.
[MEDIUM] maintainability · packages/app-shell/src/features/surface/session-dashboard-button.tsx L33-46
该 onClick 处理器将与同一功能目录下 useOpenSurface hook 中已抽取的 open-apply-catch 逻辑高度重复（读取 store 状态 → 判断 target → 调用平台方法 → applyEvent → 条件性 setSidePanelInstance → catch 弹 toast）。此外，此处使用 .then().catch() Promise 链，而 useOpenSurface 使用 async/await + try/catch，两者风格不一致。如果后续需要修改后处理逻辑（例如增加对过期会话的守卫、调整错误处理方式），必须在两处分别修改，容易遗漏导致行为不一致。建议将该公共逻辑抽取为共享 helper 或泛化 useOpenSurface hook，使其接受平台 open 方法作为参数，从而复用同一套后处理与错误处理路径。
建议修复
// 方案一：泛化 useOpenSurface，使其接受 open 函数参数
// use-open-surface.ts 中新增：
// export function useOpenSurfaceRecord() {
// const { t } = useTranslation();
// return useCallback(async (
// open: (target: SurfaceTarget) => Promise<SurfaceRecord>,
// ) => {
// const { embeddedSupported, setSidePanelInstance, applyEvent } =
// useSurfaceStore.getState();
// const target = embeddedSupported ? "embedded" : "windowed";
// try {
// const record = await open(target);
// applyEvent({ type: "opened", ...record });
// if (record.target === "embedded") {
// setSidePanelInstance(record.instance);
// }
// } catch {
// toast.error(t("surface.openFailed"));
// }
// }, [t]);
// }
// session-dashboard-button.tsx 中使用：
// const openSurfaceRecord = useOpenSurfaceRecord();
// onClick={() =>
// void openSurfaceRecord((target) =>
// surfaces.openSessionTraceDashboard(sessionId, target),
// )
// }
| #[test] | ||
| fn binds_and_takes_workbench_invocation_context() { | ||
| let registry = SurfaceRegistry::default(); | ||
| let (record, _) = registry | ||
| .open(workbench_definition("acme.panel"), MountTarget::Windowed) | ||
| .expect("open"); | ||
|
|
||
| let first = registry.workbench_context(record.instance, || "context-1".to_string()); | ||
| let second = registry.workbench_context(record.instance, || "context-2".to_string()); | ||
| let taken = registry.take_workbench_context(record.instance); | ||
| let after_take = registry.workbench_context(record.instance, || "context-3".to_string()); | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
[MEDIUM] test · crates/surface/src/registry.rs L587-599
测试方法名为 binds_and_takes_workbench_invocation_context，但实际仅调用了 workbench_context（懒初始化路径）和 take_workbench_context，从未调用 bind_workbench_context。作为公开 API 且涉及授权（authorization，即验证某调用方是否有权执行操作）的 bind_workbench_context 完全没有测试覆盖，存在维护性风险——未来修改该方法时无法被测试捕获回归（regression，即代码修改后原本正确的功能意外失效）。此外，三个新方法的 SurfaceSource::Workbench 守卫（guard，即前置条件检查）也未被任何测试验证，例如对 RemoteSite 实例调用应返回 None/false。建议补充对 bind_workbench_context 的测试，包括：正常绑定路径、未知实例返回 false、非 workbench 来源返回 false，以及 bind_workbench_context 后 workbench_context 返回已绑定值且不调用 issue 闭包。
建议修复
#[test]
fn binds_and_takes_workbench_invocation_context() {
let registry = SurfaceRegistry::default();
let (record, _) = registry
.open(workbench_definition("acme.panel"), MountTarget::Windowed)
.expect("open");
let first = registry.workbench_context(record.instance, || "context-1".to_string());
let second = registry.workbench_context(record.instance, || "context-2".to_string());
let taken = registry.take_workbench_context(record.instance);
let after_take = registry.workbench_context(record.instance, || "context-3".to_string());
assert_eq!(
(first, second, taken, after_take),
(
Some("context-1".to_string()),
Some("context-1".to_string()),
Some("context-1".to_string()),
Some("context-3".to_string()),
)
);
}
/// `bind_workbench_context` installs context that `workbench_context` returns without issuing.
#[test]
fn binds_context_and_lazy_read_returns_it() {
let registry = SurfaceRegistry::default();
let (record, _) = registry
.open(workbench_definition("acme.panel"), MountTarget::Windowed)
.expect("open");
let bound = registry.bind_workbench_context(record.instance, "host-ctx".to_string());
let read = registry.workbench_context(record.instance, || panic!("issue should not be called"));
assert_eq!((bound, read), (true, Some("host-ctx".to_string())));
}
/// Non-workbench sources and unknown instances are rejected.
#[test]
fn refuses_context_for_non_workbench_and_unknown() {
let registry = SurfaceRegistry::default();
let (remote, _) = registry
.open(definition("acme.hub"), MountTarget::Windowed)
.expect("open");
let ctx = registry.workbench_context(remote.instance, || "ctx".to_string());
let bound = registry.bind_workbench_context(remote.instance, "ctx".to_string());
assert_eq!((ctx, bound), (None, false));
}
| let max_bytes = required_u64(params, "max_bytes")? as usize; | ||
| if max_bytes == 0 || max_bytes > MAX_TRACE_CHUNK_BYTES { |
There was a problem hiding this comment.
[HIGH] bug · crates/plugin-lifecycle/src/trace.rs L89-90
max_bytes 在 as usize 转型后才进行范围校验，在 32 位平台上会因截断（truncation，即高位被丢弃）而绕过上限检查。required_u64 返回 u64，当值如 0x1_0000_0004（即 4GiB+4）在 32 位 usize 上被截断为 4，不仅通过了 > MAX_TRACE_CHUNK_BYTES 校验，还会导致实际读取量远小于请求值且无错误提示。应在转型前以 u64 值进行校验。
建议修复
let max_bytes_u64 = required_u64(params, "max_bytes")?;
if max_bytes_u64 == 0 || max_bytes_u64 > MAX_TRACE_CHUNK_BYTES as u64 {
return Err(invalid_params(format!(
"max_bytes must be between 1 and {MAX_TRACE_CHUNK_BYTES}"
)));
}
let max_bytes = max_bytes_u64 as usize;
| .with_context( | ||
| context_id, | ||
| &self.caller_plugin_id, | ||
| self.caller_generation, | ||
| |context| match method { | ||
| TRACE_LIST_METHOD => self.list(context), | ||
| TRACE_STAT_METHOD => self.stat(context, ¶ms), | ||
| TRACE_READ_METHOD => self.read(context, ¶ms), | ||
| _ => Err(HostRequestError::method_not_found(method)), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
[HIGH] performance · crates/plugin-lifecycle/src/trace.rs L49-59
handle 是 async fn，但 with_context 在持锁状态下同步调用闭包，而闭包内（list/stat/read）执行的是阻塞式文件系统 I/O（canonicalize、read_dir、File::open、seek、read_exact）。这带来两个问题：其一，异步执行器线程被阻塞，在单线程运行时（single-threaded runtime）上会导致整个事件循环停滞；其二，PluginInvocationContexts 的 Mutex 在整个 I/O 期间被持有，导致同一进程中所有插件的 trace 请求被串行化。同 crate 的 storage.rs 已用 tokio::task::spawn_blocking 将阻塞 I/O 移出异步线程，此处应遵循相同模式。
建议修复
// 在锁内仅提取所需数据后立即释放锁，再通过 spawn_blocking 执行 I/O：
// 1. with_context 中克隆出 trace 授权信息（grant / AuthorizedTrace 的 path 等）
// 2. drop 锁
// 3. tokio::task::spawn_blocking(move || { /* 执行 canonicalize / File::open / read 等 */ }).await
// 4. 若 list 需要写回 context.traces，再获取一次锁写入结果
| verify_containment(trace)?; | ||
| let metadata = trace.path.metadata().map_err(map_io)?; |
There was a problem hiding this comment.
[MEDIUM] security · crates/plugin-lifecycle/src/trace.rs L99-100
read 方法中 verify_containment 对 trace.path 重新规范化（canonicalize，即解析所有符号链接得到绝对真实路径）并验证路径未越界，但随后 File::open(&trace.path) 使用原始路径打开文件。在这两步之间存在 TOCTOU（Time-of-Check-to-Time-of-Use，检查与使用之间的竞态条件）窗口：若具有文件系统访问权限的攻击者在检查通过后、打开文件前将 trace.path 处的文件替换为指向 containment root 之外的符号链接（symlink），File::open 会跟随该链接读取宿主机上的非授权文件。修复方式是先 File::open 获取文件描述符，再对已打开的句柄调用 file.metadata()（底层为 fstat，不跟随符号链接）验证文件未发生替换。
建议修复
// 先打开文件获取句柄，再用 fstat（file.metadata）验证，消除 TOCTOU 窗口
let mut file = File::open(&trace.path).map_err(map_io)?;
let metadata = file.metadata().map_err(map_io)?;
let canonical = trace.path.canonicalize().map_err(map_io)?;
if canonical != trace.path || !canonical.starts_with(&trace.containment_root) {
return Err(trace_error(
"trace_unavailable",
"trace path changed after authorization",
));
}
| if depth > MAX_DISCOVERY_DEPTH || visited >= MAX_DISCOVERY_ENTRIES { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[LOW] performance · crates/plugin-lifecycle/src/trace.rs L209-211
当 visited >= MAX_DISCOVERY_ENTRIES 时使用 continue，这会从 pending 弹出下一个目录并再次命中同样的条件后跳过，直到 pending 耗尽为止——产生无意义的空转循环。应改为 break 直接终止整个 while let 遍历。
建议修复
if depth > MAX_DISCOVERY_DEPTH || visited >= MAX_DISCOVERY_ENTRIES {
break;
}
| openSessionTraceDashboard: vi.fn( | ||
| async (_sessionId: string, mount: "embedded" | "windowed") => ({ | ||
| instance: 1, | ||
| pluginId: "official/ora-space.agent-dashboard", | ||
| kind: "workbench" as const, | ||
| title: "Agent Dashboard", | ||
| target: mount, | ||
| state: "open" as const, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
[LOW] maintainability · packages/app-shell/src/test/surface-test-platform.ts L29-38
新增的 openSessionTraceDashboard mock 与已有的 open mock 在返回结构上高度重复（instance / pluginId / kind / title / target / state），两处各自手写字面量，后续若 SurfaceRecord 字段变更需同步修改多处。建议提取一个 helper 函数生成 mock 的 SurfaceRecord，降低重复并防止字段漂移。
建议修复
openSessionTraceDashboard: vi.fn(
async (_sessionId: string, mount: "embedded" | "windowed") => ({
...makeMockSurfaceRecord({
pluginId: "official/ora-space.agent-dashboard",
kind: "workbench",
title: "Agent Dashboard",
}),
target: mount,
}),
),
| openSessionTraceDashboard: vi.fn( | ||
| async (_sessionId: string, mount: "embedded" | "windowed") => ({ | ||
| instance: 1, | ||
| pluginId: "official/ora-space.agent-dashboard", | ||
| kind: "workbench" as const, | ||
| title: "Agent Dashboard", | ||
| target: mount, | ||
| state: "open" as const, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
[MEDIUM] maintainability · packages/app-shell/src/test/surface-test-platform.ts L29-38
openSessionTraceDashboard mock 与 stub-platform.ts 中同名实现返回的 instance 分别为 1 和 0，两者不一致。虽然同为测试桩、目前未发现直接断言该值的用例，但若后续测试依赖 instance 编号做侧边栏独占判断（setSidePanelInstance(record.instance)），不一致的初始值可能产生误报或漏检。建议统一使用同一常量或同值。
建议修复
openSessionTraceDashboard: vi.fn(
async (_sessionId: string, mount: "embedded" | "windowed") => ({
instance: 0,
pluginId: "official/ora-space.agent-dashboard",
kind: "workbench" as const,
title: "Agent Dashboard",
target: mount,
state: "open" as const,
}),
),
| if ( | ||
| provider.providerId.length === 0 || provider.format.length === 0 || | ||
| provider.directory.length === 0 || | ||
| provider.fileNameTemplate.length === 0 | ||
| ) { | ||
| throw new Error("Trace provider fields cannot be empty"); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] maintainability · packages/plugin-sdk/src/plugin.ts L165-171
declareTraceProvider 声明了 fileNameTemplate 必须包含且仅包含一个 {provider_session_id} 占位符（见 TraceProviderDeclaration 注释），但方法内部仅校验该字段非空，未验证占位符的存在性、唯一性或是否包含路径分隔符。虽然宿主端（host-side，即 Rust 侧）的 validate_trace_file_name_template() 会在注册阶段做完整校验，但 SDK 侧缺少前置校验会导致插件开发者只能在运行时收到宿主返回的注册拒绝错误，错误信息可能不够直观且远离调用点。建议在 declareTraceProvider 中增加对 {provider_session_id} 占位符的数量校验以及对路径分隔符的拒绝。
建议修复
if (
provider.providerId.length === 0 || provider.format.length === 0 ||
provider.directory.length === 0 ||
provider.fileNameTemplate.length === 0
) {
throw new Error("Trace provider fields cannot be empty");
}
const placeholder = "{provider_session_id}";
const placeholderCount =
provider.fileNameTemplate.split(placeholder).length - 1;
if (
placeholderCount !== 1 ||
provider.fileNameTemplate.includes("/") ||
provider.fileNameTemplate.includes("\\")
) {
throw new Error(
"Trace provider fileNameTemplate must contain exactly one " +
"{provider_session_id} placeholder and no path separators",
);
}
| if ( | ||
| provider.providerId.length === 0 || provider.format.length === 0 || | ||
| provider.directory.length === 0 || | ||
| provider.fileNameTemplate.length === 0 | ||
| ) { | ||
| throw new Error("Trace provider fields cannot be empty"); | ||
| } |
There was a problem hiding this comment.
[LOW] maintainability · packages/plugin-sdk/src/plugin.ts L165-171
TraceProviderDeclaration.directory 的文档注释为「Safe slash-separated directory below root」，但 declareTraceProvider 未校验路径安全性（如 .. 段、前导 /、反斜杠 \）。宿主端 validate_portable_relative_directory() 会做完整校验，但 SDK 侧增加前置校验可作为纵深防御（defense-in-depth，即在多个层级重复实施安全检查以提升整体防护能力），同时为插件开发者提供更早、更清晰的错误反馈。
建议修复
if (
provider.providerId.length === 0 || provider.format.length === 0 ||
provider.directory.length === 0 ||
provider.fileNameTemplate.length === 0
) {
throw new Error("Trace provider fields cannot be empty");
}
if (
provider.directory.startsWith("/") ||
provider.directory.includes("\\") ||
provider.directory.split("/").some((segment) =>
segment === "" || segment === "." || segment === ".."
)
) {
throw new Error(
"Trace provider directory must be a safe relative path without .. segments",
);
}
| render={ | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| aria-label={t("chat.dashboard.open")} | ||
| title={t("chat.dashboard.open")} | ||
| onClick={() => { | ||
| const { embeddedSupported, setSidePanelInstance, applyEvent } = | ||
| useSurfaceStore.getState(); | ||
| const target = embeddedSupported ? "embedded" : "windowed"; | ||
| void surfaces | ||
| .openSessionTraceDashboard(sessionId, target) | ||
| .then((record) => { | ||
| applyEvent({ type: "opened", ...record }); | ||
| if (record.target === "embedded") { | ||
| setSidePanelInstance(record.instance); | ||
| } | ||
| }) | ||
| .catch(() => toast.error(t("surface.openFailed"))); | ||
| }} | ||
| > | ||
| <IconChartHistogram /> | ||
| </Button> | ||
| } | ||
| /> | ||
| <TooltipContent>{t("chat.dashboard.open")}</TooltipContent> |
There was a problem hiding this comment.
[LOW] maintainability · packages/app-shell/src/features/surface/session-dashboard-button.tsx L26-52
图标放置在 render 属性内的 Button 子节点中，而非 TooltipTrigger 的直接子节点。整个代码库中其他所有 TooltipTrigger（以及 DropdownMenuTrigger、PopoverTrigger）的用法都遵循同一约定：render 元素使用自闭合标签（如 <Button ... />），图标/文本作为触发器组件的直接子节点传入。此处偏离了该约定，可能让后续维护者产生困惑，也可能与 Base UI 合并 props 时的预期行为不一致。建议将 render 元素改为自闭合，把图标移到 TooltipTrigger 的子节点。
建议修复
render={
<Button
type="button"
variant="ghost"
size="icon"
aria-label={t("chat.dashboard.open")}
title={t("chat.dashboard.open")}
onClick={() => {
const { embeddedSupported, setSidePanelInstance, applyEvent } =
useSurfaceStore.getState();
const target = embeddedSupported ? "embedded" : "windowed";
void surfaces
.openSessionTraceDashboard(sessionId, target)
.then((record) => {
applyEvent({ type: "opened", ...record });
if (record.target === "embedded") {
setSidePanelInstance(record.instance);
}
})
.catch(() => toast.error(t("surface.openFailed")));
}}
/>
}
>
<IconChartHistogram />
</TooltipTrigger>
<TooltipContent>{t("chat.dashboard.open")}</TooltipContent>
| aria-label={t("chat.dashboard.open")} | ||
| title={t("chat.dashboard.open")} | ||
| onClick={() => { |
There was a problem hiding this comment.
[LOW] maintainability · packages/app-shell/src/features/surface/session-dashboard-button.tsx L31-33
Button 上同时设置了 title 属性和同文本的 TooltipContent，会导致原生浏览器 tooltip（即鼠标悬停延迟后出现的系统级提示框）与自定义样式 tooltip 同时出现，形成视觉上的双重提示。代码库中其他 TooltipTrigger + Button 组合（如 workspace-sidebar.tsx、workflow-manager.tsx）在存在 TooltipContent 时不使用 title 属性。建议移除 title 以保持一致并避免双重提示。
建议修复
aria-label={t("chat.dashboard.open")}
onClick={() => {
| let mut file = File::open(&trace.path).map_err(map_io)?; | ||
| file.seek(SeekFrom::Start(offset)).map_err(map_io)?; | ||
| let available = metadata.len().saturating_sub(offset); | ||
| let wanted = available.min(max_bytes as u64) as usize; | ||
| let mut bytes = vec![0; wanted]; | ||
| file.read_exact(&mut bytes).map_err(map_io)?; |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L116-121
read 方法（trace.rs:116-121）在计算 wanted 后使用 read_exact 读取字节，但 metadata.len() 是在 File::open 之前通过 trace.path.metadata() 获取的。在 metadata 采样与实际文件读取之间存在 TOCTOU（time-of-check to time-of-use，即“检查时刻与使用时刻”竞态）窗口：如果在此间文件被截断或替换，read_exact 在实际读取字节数小于 wanted 时会返回 UnexpectedEof，该错误被 map_io 统一映射为 "trace file is unavailable"。虽然不会导致安全问题（路径已在 verify_containment 中验证），但错误语义不够精确——插件页面可能因此错误地认为文件不存在而非被截断，无法正确触发 cursor 重试流程。建议改用 read（非精确读取）并返回实际读取字节数，与基于实际读取量的 eof 判断保持一致。
建议修复
let mut file = File::open(&trace.path).map_err(map_io)?;
file.seek(SeekFrom::Start(offset)).map_err(map_io)?;
let available = metadata.len().saturating_sub(offset);
let wanted = available.min(max_bytes as u64) as usize;
let mut bytes = vec![0; wanted];
let read_bytes = file.read(&mut bytes).map_err(map_io)?;
bytes.truncate(read_bytes);
| /// Returns the registry shared with every launched process handler. | ||
| pub fn invocation_contexts(&self) -> PluginInvocationContexts { | ||
| self.contexts.clone() | ||
| } |
There was a problem hiding this comment.
[LOW] maintainability · crates/plugin-lifecycle/src/runtime.rs L117-120
invocation_contexts() 方法返回的是 self.contexts.clone()。查看 context.rs:56-60，PluginInvocationContexts 是 Arc<Mutex<...>> 的包装，因此 clone() 实际上只是增加引用计数，返回的是共享句柄。方法文档注释仅写 "Returns the registry shared with every launched process handler"，但没有明确告知调用方返回的是共享句柄而非独立副本，调用方无法从签名和注释推断出对其的修改会影响所有已启动的进程。虽然当前唯一调用方（crates/backend/src/plugin.rs:118）确实在使用共享语义，但为了防止未来误用，建议在文档注释中明确说明返回的是共享句柄。
建议修复
/// Returns a shared handle to the invocation-context registry.
///
/// The returned [`PluginInvocationContexts`] shares its inner state (via `Arc`) with every
/// process launched by this launcher. Mutations (issue, grant, revoke) are immediately visible
/// to all live handlers and future launches.
pub fn invocation_contexts(&self) -> PluginInvocationContexts {
self.contexts.clone()
}
| pub async fn handle(&self, method: &str, params: Value) -> Result<Value, HostRequestError> { | ||
| let context_id = required_string(¶ms, "context_id")?; | ||
| self.contexts | ||
| .with_context( | ||
| context_id, | ||
| &self.caller_plugin_id, | ||
| self.caller_generation, | ||
| |context| match method { | ||
| TRACE_LIST_METHOD => self.list(context), | ||
| TRACE_STAT_METHOD => self.stat(context, ¶ms), | ||
| TRACE_READ_METHOD => self.read(context, ¶ms), | ||
| _ => Err(HostRequestError::method_not_found(method)), | ||
| }, | ||
| ) | ||
| .ok_or_else(|| trace_error("context_unavailable", "trace context is unavailable"))? | ||
| } |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L46-61
PluginTraceHost::handle 中调用 with_context 并传入一个同步闭包（match method { ... }），但该闭包内部执行了同步文件 I/O 操作（list → discover → std::fs::read_dir / File::metadata；read → File::open / read_exact）。with_context（context.rs:120-135）在持有 Mutex 守卫期间执行闭包，这意味着所有同步文件 I/O 都在持有进程级别的 Mutex 时进行。当多个插件进程同时发起 trace 请求时，一个插件的大规模目录扫描（递归最多 100,000 个条目 × 深度 8）会阻塞所有其他插件的 trace 请求和 context 管理操作（issue、grant_trace、revoke）。此外 Mutex 是同步锁且在 async handle 中跨 .await 持有，但在检查后发现 with_context 是同步的（不包含 .await），所以不会触发 cancel safety（取消安全性）问题，但持有同步锁期间执行可能耗时的文件 I/O 仍然会导致性能瓶颈。
建议修复
// 建议将 context 查找与文件 I/O 分离：先在锁内拷贝出所需的 grant/trace 信息，
// 释放锁后再执行文件 I/O。例如：
//
// let grant = self.contexts.with_context(..., |ctx| ctx.trace.clone())?;
// // 在锁外执行 discover / read
//
// 这需要调整 with_context 的调用模式，避免在持有锁期间做文件操作。
| if provider.locator.recursive { | ||
| discover_recursive( | ||
| &directory, | ||
| &file_name, | ||
| provider, | ||
| &session.label, | ||
| session.ora_session_id == grant.current_ora_session_id, | ||
| &mut context.trace_ids, | ||
| &mut traces, | ||
| )?; | ||
| } else { |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/trace.rs L170-180
trace.rs 第 271 行 add_trace_if_file 在 discover_recursive 中被调用时，传入的 containment_root 是递归发现的根目录。但 add_trace_if_file 第 291 行对 path 做了 canonicalize 后检查 canonical.starts_with(containment_root)，而 discover_recursive 传入的 root 参数是经过 canonicalize 的（trace.rs:159-160 let root = root.canonicalize().map_err(map_io)?;），但递归发现的子目录路径来自 entry.path()，它可能因符号链接而逃逸。discover_recursive 第 196 行检查了 file_type.is_symlink() 会 continue，但 read_dir 返回的条目可能包含指向目录的硬链接或绑定挂载点，is_symlink() 只会跳过符号链接。如果存在绑定挂载（bind mount）或硬链接目录，entry.path() 会指向该目录，其子文件会在后续迭代中被发现，而这些文件的 canonicalize 路径可能不在 containment_root 内，此时 add_trace_if_file 的 containment 检查会捕获并返回错误——这是防御性失败而非泄漏，所以不是安全问题。但递归发现中的绑定挂载场景可能导致 trace 列表整个失败而非跳过。这个限制可以在文档中说明。
建议修复
// 当前行为是安全的：containment 检查会在绑定挂载场景下返回错误而非泄漏。
// 若希望更健壮，可在 discover_recursive 中对子目录也做 canonicalize + starts_with 检查：
//
// if file_type.is_dir() {
// let sub = entry.path();
// if let Ok(canon) = sub.canonicalize() {
// if canon.starts_with(root) {
// pending.push((sub, depth + 1));
// }
// }
// }
| let mut file = File::open(&trace.path).map_err(map_io)?; | ||
| file.seek(SeekFrom::Start(offset)).map_err(map_io)?; | ||
| let available = metadata.len().saturating_sub(offset); | ||
| let wanted = available.min(max_bytes as u64) as usize; | ||
| let mut bytes = vec![0; wanted]; | ||
| file.read_exact(&mut bytes).map_err(map_io)?; |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L116-121
read 方法中，file.read_exact(&mut bytes) 在读取到 EOF（文件末尾）但未填满缓冲区时会返回 UnexpectedEof 错误。虽然前面通过 metadata.len() 计算了 wanted，但 metadata 的采样与实际读取之间存在 TOCTOU（time-of-check to time-of-use，即“检查时刻与使用时刻”竞态）窗口：如果在此间文件被截断，read_exact 会因实际可读字节少于 wanted 而失败。该错误被 map_io 统一映射为 "trace file is unavailable"，错误语义不够精确，插件页面无法区分文件不存在与文件被截断。建议改用 read（非精确读取）并返回实际读取字节数，与基于实际读取量的 eof 判断保持一致。
建议修复
let mut file = File::open(&trace.path).map_err(map_io)?;
file.seek(SeekFrom::Start(offset)).map_err(map_io)?;
let available = metadata.len().saturating_sub(offset);
let wanted = available.min(max_bytes as u64) as usize;
let mut bytes = vec![0; wanted];
let read_bytes = file.read(&mut bytes).map_err(map_io)?;
bytes.truncate(read_bytes);
| async move { | ||
| runtime.wait_for_exit().await; | ||
| contexts.revoke_generation(&plugin_id, generation); |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/runtime.rs L215-217
退出观察任务在 runtime.wait_for_exit().await 之后立即调用 contexts.revoke_generation(&plugin_id, generation)。由于 PluginTraceHost 持有自己的 contexts clone（共享同一 Arc<Mutex<...>>），如果此时还有正在处理中的 trace 请求（例如 list 正在执行递归目录扫描），revoke_generation 会与 with_context 竞争同一把 Mutex。由于 with_context 是同步的（在 handle 中不跨 .await），revoke_generation 会等待 with_context 释放锁后再执行，因此不会导致数据损坏或 panic。但需要注意：wait_for_exit 返回后进程已退出，此时正在处理的 trace 请求实际上是通过主机侧的 JSON-RPC 处理器执行的，与子进程无关。但子进程已退出意味着该插件 generation 的所有 context 应该被撤销。当前逻辑是安全的，因为 with_context 和 revoke_generation 都通过同一把 Mutex 序列化，不存在真正的并发冲突。
建议修复
// 当前逻辑安全：Mutex 序列化了 with_context 和 revoke_generation。
// 若未来 trace 处理变为异步或在锁外持有 context 引用，需重新评估。
// 可考虑在 revoke 后添加日志以辅助调试：
// ora_debug!(plugin_id = %plugin_id, generation = generation.0, "revoked trace contexts for stopped generation");
| fn map_io(error: std::io::Error) -> HostRequestError { | ||
| let kind = if error.kind() == std::io::ErrorKind::NotFound { | ||
| "trace_not_found" | ||
| } else { | ||
| "io" | ||
| }; | ||
| trace_error(kind, "trace file is unavailable") | ||
| } |
There was a problem hiding this comment.
[MEDIUM] bug · crates/plugin-lifecycle/src/trace.rs L358-365
map_io 函数将所有非 NotFound 的 IO 错误统一映射为 kind = "io"、消息 "trace file is unavailable"。这意味着 UnexpectedEof（文件被截断）、PermissionDenied（权限不足）等完全不同的情况都会被映射为相同的错误响应，调用方无法区分“文件不存在”、“文件被截断导致 cursor 过期”和“权限问题”。特别是 UnexpectedEof 应当映射为 stale_cursor 以允许插件页面重新执行 list 获取最新 trace 列表，而非笼统的 io 错误。
建议修复
fn map_io(error: std::io::Error) -> HostRequestError {
let kind = match error.kind() {
std::io::ErrorKind::NotFound => "trace_not_found",
std::io::ErrorKind::UnexpectedEof => "stale_cursor",
_ => "io",
};
trace_error(kind, "trace file is unavailable")
}
| fn verify_containment(trace: &AuthorizedTrace) -> Result<(), HostRequestError> { | ||
| let canonical = trace.path.canonicalize().map_err(map_io)?; | ||
| if canonical != trace.path || !canonical.starts_with(&trace.containment_root) { | ||
| return Err(trace_error( | ||
| "trace_unavailable", | ||
| "trace path changed after authorization", | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[LOW] bug · crates/plugin-lifecycle/src/trace.rs L274-283
verify_containment 第 276 行检查 canonical != trace.path，意图是检测 trace 文件在授权后被替换。但 trace.path 已在 add_trace_if_file 中通过 canonicalize 设置为规范路径（trace.rs:266 path: canonical）。canonicalize 在大多数平台上会解析符号链接并返回绝对路径。如果同一路径上的文件被删除后重新创建（不同 inode），canonicalize 仍然返回相同的路径字符串，因此 canonical != trace.path 不会触发——该检查无法检测文件替换。这本身不是安全漏洞（containment 仍然有效），但该检查名不副实，可能给维护者虚假的安全感。
建议修复
// 当前检查对文件替换场景效果有限，但 containment 检查仍然有效。
// 若要检测文件替换，可比较 inode/file handle，但跨平台实现复杂。
// 建议更新注释明确说明此检查的范围：
fn verify_containment(trace: &AuthorizedTrace) -> Result<(), HostRequestError> {
// Re-canonicalize to detect symlink redirection or path escape; does not
// detect same-path file replacement (use cursor for that).
let canonical = trace.path.canonicalize().map_err(map_io)?;
if !canonical.starts_with(&trace.containment_root) {
return Err(trace_error(
"trace_unavailable",
"trace path changed after authorization",
));
}
Ok(())
}
🤖 OCR 行级代码检视
共发现 76 条意见：74 条已作为行内评论提交，2 条因不在本次 diff 范围内见下表。
未定位到变更行的意见（2）1. [low]
|
Summary
Dependency and CI status
This is intentionally a Draft PR. The cross-repository rollout depends on publishing the updated @ora-space/plugin-sdk 0.9.0 API used by the companion agent/dashboard plugins. Related integration CI may remain temporarily red until that SDK is published and downstream consumers are updated.
Validation
Notes
The branch is rebased onto ora-space/desktop main at 67d07a4. The six commits are split by CSP, protocol, data plane, agent runtime, Surface host, and app-shell integration for review.