diff --git a/Cargo.lock b/Cargo.lock index 34cdd836a2..f51fa3f1b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1272,6 +1272,7 @@ dependencies = [ "serde_json", "sha2", "tokio", + "url", "which 8.0.5", ] diff --git a/docs/architecture/cli-product-line-design.md b/docs/architecture/cli-product-line-design.md index 9922e7f7ae..d643bab464 100644 --- a/docs/architecture/cli-product-line-design.md +++ b/docs/architecture/cli-product-line-design.md @@ -494,10 +494,10 @@ Configuration 只能覆盖产品定义明确允许的默认值;用户插件只 Plugin/Tool、可执行 Skill/Command、MCP/LSP/Formatter、远程 Reference 等 L2/L3 内容在 OC-R2 完成归属模块保护 前只发现和展示;完成后仍须在首次启用或能力扩大时确认。它们无需先迁移; 显式导入用于用户希望取得 BitFun 独立管理快照的场景。当前只落地两个经过评审的窄切片:Desktop 与 -`bitfun mcp import` 可以预览 OpenCode / Claude Code 中语义等价的 MCP 安全声明,只有显式 `--apply` 才原子写入现有 +`bitfun mcp import` 可以预览 OpenCode、Claude Code 与 Codex 中语义等价的 MCP 安全声明,只有显式 `--apply` 才原子写入现有 BitFun MCP 配置;`bitfun hooks` 和统一 `/hooks` 可预览 Claude Code / Codex 中受支持的同步 command Hook,并用精确 计划指纹确认后复制到现有原生 Hook 层。两者都不写回来源文件,也不表示通用 Canonical Config 导入已进入 CLI-P1。 -MCP 的 Codex 投影、凭据、header、env、cwd、通用导入记录和 undo 均未实现;Hook 的 OpenCode、非 command 或依赖 +MCP 的凭据、header、env、cwd、通用导入记录和 undo 均未实现;Hook 的 OpenCode、非 command 或依赖 外部 Runtime 的 handler 仍只静态展示。其他资产在 CLI-P0 仍截止到 Dry-run,只有各自经过评审的 apply 切片才能写入: ```text @@ -512,6 +512,13 @@ Hook C0:脱敏发现 -> 精确命令预览 | 指纹确认 -> 原子发布本 返回类型化 `action-required`;无关待办只进入结构化状态或 `stderr` 摘要,不等待不可见输入,也不自动批准。 当前只能静态预览的 custom tool 名称只显示“已发现,未执行”。 +Codex MCP 快照沿用同一 `bitfun mcp import` 心智:local stdio 只在没有 environment 且无需 effective working directory +时接纳;显式 cwd 或当前 workspace 形成的隐式 cwd 都返回“需要设置”,避免导入后改用 BitFun 进程目录。remote 只接纳无 +userinfo/query/fragment/header/bearer 和额外 OAuth 语义的纯 HTTPS URL;其他字段返回“需要设置”或“不支持”,不做降级 +复制。导入结果仍为 disabled、`autoStart: false`。Desktop 在已有导入卡内默认选中全部 eligible 项并允许逐项取消; +plan stale 后只保留旧选择与新 eligible 项的交集,新候选不自动选中。CLI 保持默认全量和重复 `--candidate` 缩小集合, +仅在展示名重复时附带既有 candidate ID 消歧,不新增生态专用命令。 + 导入预览只使用四种用户可读结论:可直接使用、需要转换、会发生功能降级、输入无效。每项同时说明是原地 引用、写入 BitFun 配置、继续保持外部来源还是不支持;不得用“已映射”推导为已写入、已信任或已启用。 @@ -545,7 +552,8 @@ Hook C0:脱敏发现 -> 精确命令预览 | 指纹确认 -> 原子发布本 报告也不属于当前实现。 现有对 `.claude/.codex/.opencode/.agents` Skill 根的直接发现已经保留来源身份和全局/项目使用范围,并在 GUI/TUI -展示来源和默认覆盖状态,模式配置再展示实际采用项;固定根顺序保持为 Skill Registry 的独立回归契约。 +展示来源和默认覆盖状态,模式配置再展示实际采用项;固定根顺序保持为 Skill Registry 的独立回归契约。Registry 仅按 +既有 source slot 在内部选择来源方言,不向用户增加主选择器,也不让本地与 Remote 分支各自猜测路径。 Skill Registry 还保留来源资产声明的隐式调用意图:Claude `SKILL.md` 的 `disable-model-invocation: true` 与 Codex `agents/openai.yaml` 的 `policy.allow_implicit_invocation: false` 都会让 Skill 不进入模型自动目录,但不影响 `/skills`、 模式配置和显式加载。Claude `user-invocable: false` 与上述模型调用策略相互独立:它只让 Skill 不进入 Web/CLI 的用户 @@ -558,13 +566,20 @@ Skill Registry 还保留来源资产声明的隐式调用意图:Claude `SKILL. 分组以及 `\$` 转义;缺失的位置参数保留原占位符,模板没有未转义占位符时才追加 `ARGUMENTS:` 段。该展开器只处理 字符串,不执行命令、脚本或动态变量。未携带 `arguments` 的旧工具调用保持原 Skill 正文不变。 +Claude Skill 使用目录名作为稳定调用身份;frontmatter `description` 可缺省并回退正文首个非空段落,可选 +`when_to_use` 只能与已有描述合并,合并后限制为 1536 个 Unicode 字符。`arguments` 可声明为空白分隔字符串或字符串列表,并按顺序把 +`$name` 绑定到同一个调用参数列表;缺失命名参数展开为空,位置参数兼容规则不变。Codex Skill 在缺少 `name` 时回退 +目录名,但仍要求 `description`。`.agents/.opencode/.bitfun/.cursor` 继续使用原有严格语义。Claude `allowed-tools` 不授予 +预批准;`context`/`fork`、`agent`、`model`、`effort`、`hooks`、`paths`、`shell`、`runtime` 及动态 shell/runtime 变量等未接通行为会 +整体拒绝加载,而不是静默忽略后部分执行。 + 这项能力不新增导入记录、来源图、后台 watcher 或第二套刷新生命周期。用户只需要一个手动入口:`/reload` 同时刷新 Skill Registry 并失效当前 Session 的 Workspace Instructions 缓存;`/reload skills` 与 `/reload instructions` 用于只刷新 一类内容。Desktop、Embedded CLI 与 Shared TUI 共用同一 core 协调入口,但 Skill Registry 刷新和 Session `UserContext` 缓存失效仍由各自既有 owner 完成。指令变更从下一条消息开始生效;运行期不承诺文件监听或当前生成中的 消息热替换。缓存 generation 会拒绝活动 Turn 在失效之后写回的旧构建结果;旧 `/reload-skills` 输入仅作为隐藏兼容别名 映射到 `/reload skills`,不增加第二个命令入口。 -本切片也不实现 `allowed-tools`、`context`、`fork`、`agent`、`model`、命名参数、动态 shell/runtime 变量、URL、祖先目录 +本切片也不实现 `allowed-tools` 的权限预批准、`context`、`fork`、`agent`、`model`、动态 shell/runtime 变量、URL、祖先目录 级联、插件 Runtime 或 OpenCode 复杂 Hook。后续只有在存在稳定消费方和独立安全边界时才扩展这些语义。 Skill 说明和索引可按 L1 处理,脚本、URL 和外部依赖按 L2 确认;显式导入仍不得复制凭据值。MCP 启用状态按 diff --git a/docs/architecture/extensions/external-ai-work-sources-design.md b/docs/architecture/extensions/external-ai-work-sources-design.md index ad234bdd42..d42dddf106 100644 --- a/docs/architecture/extensions/external-ai-work-sources-design.md +++ b/docs/architecture/extensions/external-ai-work-sources-design.md @@ -23,16 +23,16 @@ MCP 安全子集,以及 Codex Subagent、MCP 安全子集;三种生态使用 Codex/Claude Code 运行时适配、primary agent 替换和外部 Subagent 续接仍属于后续阶段,不能因来源被识别就宣称已经可用。OpenCode、Claude Code 与 Codex 的本地 Hook 脱敏目录 已作为独立只读切片接入;在此之上,Claude Code 与 Codex 的同步 command 子集可经精确命令审阅复制为 BitFun 管理的 原生 Hook 层,仍由唯一 `AgentHookEngine` 执行。OpenCode handler、非 command/异步 handler 和未审阅声明仍不可执行。 -独立的 MCP C0a 快照导入复用上述来源与现有 MCP 配置 owner:Desktop 和根 CLI 可预览 OpenCode / Claude Code -中语义等价的安全声明,并在用户显式确认后原子写入 disabled 原生条目。Codex 导入投影、凭据/header/env/cwd -迁移、通用导入记录、undo、Peer/Remote 写入均未实现;这不改变外部 MCP 持续兼容来源的运行路径。 +独立的 MCP C0a 快照导入复用上述来源与现有 MCP 配置 owner:Desktop 和根 CLI 可预览 OpenCode、Claude Code +与 Codex 中语义等价的安全声明,并在用户显式确认后原子写入 disabled 原生条目。凭据/header/env/cwd 迁移、 +通用导入记录、undo、Peer/Remote 写入均未实现;这不改变外部 MCP 持续兼容来源的运行路径。 ## 0. 当前 MCP 快照导入契约(C0a) 快照导入是显式复制,不是持续同步,也不改变现有外部 MCP 兼容来源。Desktop 与根 CLI 只负责展示脱敏预览并发送 -typed intent;OpenCode / Claude Code sibling adapter 复用各自已合并的解析结果生成私有安全投影,外部来源协调器固定 -当前 candidate 与行为版本,core 负责重新规划,最终仍由唯一 MCP 配置 service 校验并写入 `mcp_servers`。 -Codex 继续参与现有只读发现,但当前没有导入投影。 +typed intent;OpenCode、Claude Code 与 Codex sibling adapter 复用各自已合并的解析结果生成私有安全投影,外部来源 +协调器固定当前 candidate 与行为版本,core 负责重新规划,最终仍由唯一 MCP 配置 service 校验并写入 +`mcp_servers`。Codex 的投影与其运行准备共用同一当前 candidate/version fencing,不建立第二套解析或缓存。 公开的 versioned plan/apply DTO 只包含 schema version、plan fingerprint、candidate ID、display name、transport、建议 native ID、disposition 和稳定 reason code,不包含 command arguments、URL、原始 JSON、凭据、environment/header 值或 @@ -42,10 +42,15 @@ native ID、disposition 和稳定 reason code,不包含 command arguments、UR 当前只复制能够与原生配置保持等价语义的声明: - 无显式 environment/cwd 的 local stdio command 与 adapter 已解析 arguments; -- 无 userinfo、query、fragment、header 或 provider OAuth 变化的 HTTPS streamable HTTP URL。 +- 无 userinfo、query、fragment、header、bearer token 或 provider OAuth 变化的 HTTPS streamable HTTP URL。 environment 值或引用、header/authorization、cwd、未知字段和其他 transport 不猜测、不复制、不记录。导入条目始终为 `enabled: false` 与 `autoStart: false`;local 条目不继承完整父进程环境,只保留 MCP runtime owner 提供的安全环境。 +Codex 的 legacy `name` 是上游忽略的展示字段,不进入导入结果或行为版本;`startup_timeout_sec`、`tool_timeout_sec`、 +`enabled_tools`、`disabled_tools`、approval、environment/scopes/OAuth 与并行调用等运行敏感字段仍按不支持处理,不能因 +静态发现成功而丢弃语义后导入。 +Codex 未显式声明 cwd 时,其兼容运行投影仍会把当前 workspace 作为 effective cwd;现有原生快照格式不会保留这项隐式 +语义,因此 workspace 场景的 local 声明返回“需要设置”,不能以“没有 cwd 字段”为由导入后继承 BitFun 进程目录。 native ID 优先使用外部 logical name,再使用稳定生态后缀和最小可用数字后缀;超长名称使用 bounded digest,已有条目 永不覆盖。plan fingerprint 同时绑定脱敏 plan、私有投影和当前原生 MCP 配置摘要。apply 会重新发现并重建 plan;来源或 @@ -59,6 +64,11 @@ native ID 优先使用外部 logical name,再使用稳定生态后缀和最小 写入口、导入 journal、tombstone、undo、外部应用回写或插件安装/激活策略;导入后仍由既有 MCP manager 完成复核、编辑、 启用和删除。 +Desktop 的导入卡默认选中当前 plan 中全部 eligible 项,用户可在原卡片内取消个别条目;每项同时显示来源生态和 +用户/项目使用范围,不增加新的向导或主选择器。apply 只发送当前选中 candidate。若并发来源或目标配置变化导致 plan +stale,界面替换为服务端返回的新 plan,并只保留“旧选择与新 eligible candidate 的交集”;新出现的 candidate 不自动 +勾选,避免一次旧确认扩大到用户未见过的内容。取消、完成或切换作用域会清空这份易失选择。 + ## 1. 产品判断与竞品启示 竞品事实与 BitFun 的产品判断分开记录: @@ -268,7 +278,7 @@ OpenCode Subagent 属于 L2:adapter 只读取声明,不执行外部代码; | Subagent | 用户/项目声明的安全子集 | 用户/项目 `agents/**/*.md` 的安全子集 | 用户/项目 `[agents]`、角色文件与安全配置层子集 | prompt、描述、精确模型和可表达工具请求进入既有归属模块;权限、私有 MCP/Hook、推理/并发等没有对应实现的字段会阻止激活。 | | MCP | 用户/显式目录/项目配置的安全子集 | user/project/local 原生层的安全子集 | 用户与项目 `config.toml` 原生层的安全子集 | 支持可表达的 stdio 与 HTTPS Streamable HTTP;发现不启动 Server,首次激活继续经 BitFun MCP 审批。OAuth、remote executor、per-tool policy 等不完整语义明确降级。 | | Standalone Tool | 已有单文件 JavaScript 子集 | 无稳定的 runtime-free standalone Tool 来源 | 无稳定的 runtime-free standalone Tool 来源 | TypeScript、package/plugin Tool 与动态工具注册依赖独立 Plugin Host,不在声明式 adapter 中猜测。 | -| Skill | 由现有 Skill 加载模块发现 `.opencode` 等标准根 | 由现有 Skill 加载模块发现 `.claude` 标准根 | 由现有 Skill 加载模块发现 `.codex`、`.agents` 标准根 | Skill 的加载、覆盖、模式开关与执行仍由同一个 Skill 模块负责,不复制进外部来源管理模块。 | +| Skill | 由现有 Skill 加载模块发现 `.opencode` 等标准根 | 由现有 Skill 加载模块发现 `.claude` 标准根;目录名是调用身份,描述可回退正文首段,`when_to_use` 合入索引,声明参数可做纯文本命名展开 | 由现有 Skill 加载模块发现 `.codex`、`.agents` 标准根;`.codex` 缺少 `name` 时回退目录名 | Skill 的加载、覆盖、模式开关与执行仍由同一个 Skill 模块负责,不复制进外部来源管理模块;未接通的运行时字段整体拒绝。 | | Hook | 静态目录 | 脱敏目录;同步 command 子集可审阅导入 | 脱敏目录;同步 command 子集可审阅导入 | 仅复制到私有原生快照并由 `AgentHookEngine` 执行;OpenCode、非 command、异步、未知或依赖未观察激活语义的 handler 不导入。 | 生态原生语义由各 adapter 以契约测试固定,不抽象成全局优先级: @@ -277,6 +287,15 @@ OpenCode Subagent 属于 L2:adapter 只读取声明,不执行外部代码; `/frontend:component` 的原生命名空间;同层重名无效,遵循 Claude Code 当前“personal 覆盖 project”的 Skill/legacy Command 规则,同名 Skill 仅通过有界名称索引遮蔽 Command。 只展开 `$ARGUMENTS`、`$ARGUMENTS[N]` 和 `$N` 纯文本参数;shell、文件引用和改变 Agent、模型、工具或 Hook 的字段整体阻止激活。 +- Skill Registry 继续拥有所有根的发现、覆盖、显式加载与刷新,只用既有稳定 source slot 在内部选择格式方言,不向用户 + 暴露主选择器,也不按路径字符串临时猜测。`.claude` Skill 的调用名固定为目录名;`description` 缺失时取正文首个 + 非空段落,并与可选 `when_to_use` 合并为最多 1536 个 Unicode 字符的模型索引说明。`arguments` 可为以空白分隔的名称 + 字符串或字符串列表;名称按参数顺序绑定并由现有纯文本参数展开器处理,缺失命名参数展开为空,既有缺失位置参数仍保留 + 占位符。`.codex` Skill 只增加上游已有的目录名 fallback,`description` 仍必填;`.agents`、`.opencode`、`.bitfun` 和 + `.cursor` 的严格格式不变。本地与 Remote 发现及实际加载必须使用同一方言映射,避免目录显示可用而执行时重新解析失败。 +- Claude `allowed-tools` 不能授予 BitFun 工具预批准,因此安全降级为无额外权限;`context`/`fork`、`agent`、`model`、 + `effort`、`hooks`、`paths`、`shell`、`runtime` 等会改变执行行为而当前没有等价 owner 的字段阻止加载。Claude runtime 变量与动态 + shell 注入也不执行。此切片不增加插件 Skill、祖先活动目录、文件 watcher、URL 来源或另一条 reload 命令。 - Claude Subagent 扫描用户与逐层项目 `.claude/agents/**/*.md`,近工作目录定义整项覆盖;Claude MCP 保留 `local > project > user` 的整项覆盖,local 只读取与规范化当前工作区严格匹配的项目项。 - Codex Subagent 从用户与逐层项目 `[agents]`、角色文件合并,缺失字段按 Codex 层级继承;`enabled`、默认模型等已支持 diff --git a/src/apps/cli/README.md b/src/apps/cli/README.md index c88af90671..dc06e02031 100644 --- a/src/apps/cli/README.md +++ b/src/apps/cli/README.md @@ -20,7 +20,7 @@ bitfun sessions list bitfun usage bitfun doctor bitfun health -bitfun mcp import # preview safe OpenCode / Claude Code MCP declarations +bitfun mcp import # preview safe OpenCode / Claude Code / Codex MCP declarations bitfun mcp import --apply # copy eligible declarations as disabled native entries bitfun mcp import --apply --candidate # repeat to select a subset bitfun mcp import --apply --candidate --native-id @@ -30,11 +30,11 @@ bitfun update --check # report only; do not install ``` `bitfun mcp import` is an explicit snapshot operation, not continuous sync. It -does not copy credentials, headers, environment values, or working directories, -and Codex MCP import is not supported in the current slice. Apply revalidates the -preview and never overwrites an existing native entry; imported entries remain -disabled until reviewed and enabled through the existing MCP manager. Use -`--format json` for the versioned machine-readable plan or result. +does not copy credentials, headers, environment values, or explicit working +directories. Apply revalidates the preview and never overwrites an existing +native entry; imported entries remain disabled until reviewed and enabled +through the existing MCP manager. Use `--format json` for the versioned +machine-readable plan or result. Official Linux archive installations check for updates before interactive TUI startup at most once every six hours. That check only fetches the release diff --git a/src/apps/cli/src/mcp_import.rs b/src/apps/cli/src/mcp_import.rs index 67d9954160..ef68047f60 100644 --- a/src/apps/cli/src/mcp_import.rs +++ b/src/apps/cli/src/mcp_import.rs @@ -5,6 +5,7 @@ use bitfun_product_domains::external_sources::{ EXTERNAL_MCP_IMPORT_SCHEMA_V1, }; use clap::ValueEnum; +use std::collections::BTreeMap; #[derive(Debug, Clone, Copy, PartialEq, Eq, ValueEnum)] pub(crate) enum McpImportOutputFormat { @@ -117,10 +118,27 @@ fn render_plan(plan: &ExternalMcpImportPlanV1) -> String { "{} external MCP server(s) can be imported:", eligible.len() )]; + let display_name_counts = eligible.iter().fold(BTreeMap::new(), |mut counts, item| { + *counts.entry(item.display_name.as_str()).or_insert(0usize) += 1; + counts + }); for item in eligible { + let display_name = crate::plugin_diagnostics::escape_terminal_text(&item.display_name); + let display_name = if display_name_counts + .get(item.display_name.as_str()) + .is_some_and(|count| *count > 1) + { + format!( + "{} [{}]", + display_name, + crate::plugin_diagnostics::escape_terminal_text(&item.candidate_id) + ) + } else { + display_name + }; lines.push(format!( "- {} -> {}", - crate::plugin_diagnostics::escape_terminal_text(&item.display_name), + display_name, crate::plugin_diagnostics::escape_terminal_text( item.proposed_native_id.as_deref().unwrap_or("unavailable") ) @@ -147,3 +165,62 @@ fn operation_error( ) -> anyhow::Error { anyhow!("{}: {}", error.code.as_str(), error.detail) } + +#[cfg(test)] +mod tests { + use super::*; + use bitfun_product_domains::external_sources::{ + ExternalMcpImportPlanItemV1, ExternalMcpTransportKind, + }; + + fn item( + candidate_id: &str, + display_name: &str, + native_id: &str, + ) -> ExternalMcpImportPlanItemV1 { + ExternalMcpImportPlanItemV1 { + candidate_id: candidate_id.to_string(), + display_name: display_name.to_string(), + transport: ExternalMcpTransportKind::LocalStdio, + proposed_native_id: Some(native_id.to_string()), + disposition: ExternalMcpImportDispositionV1::Eligible, + reason_code: None, + } + } + + #[test] + fn render_plan_adds_candidate_ids_only_for_duplicate_display_names() { + let plan = ExternalMcpImportPlanV1 { + schema_version: EXTERNAL_MCP_IMPORT_SCHEMA_V1, + plan_fingerprint: "plan".to_string(), + items: vec![ + item("opencode::user::docs", "Docs", "docs"), + item("codex::project::docs", "Docs", "docs-codex"), + item("claude::user::search", "Search", "search"), + ], + }; + + let rendered = render_plan(&plan); + + assert!(rendered.contains("- Docs [opencode::user::docs] -> docs")); + assert!(rendered.contains("- Docs [codex::project::docs] -> docs-codex")); + assert!(rendered.contains("- Search -> search")); + assert!(!rendered.contains("Search [claude::user::search]")); + } + + #[test] + fn render_plan_ignores_ineligible_duplicate_names_when_deciding_labels() { + let mut ineligible = item("old::docs", "Docs", "docs-old"); + ineligible.disposition = ExternalMcpImportDispositionV1::AlreadyImported; + let plan = ExternalMcpImportPlanV1 { + schema_version: EXTERNAL_MCP_IMPORT_SCHEMA_V1, + plan_fingerprint: "plan".to_string(), + items: vec![item("codex::docs", "Docs", "docs"), ineligible], + }; + + let rendered = render_plan(&plan); + + assert!(rendered.contains("- Docs -> docs")); + assert!(!rendered.contains("Docs [codex::docs]")); + } +} diff --git a/src/crates/adapters/claude-code-adapter/src/mcp_source.rs b/src/crates/adapters/claude-code-adapter/src/mcp_source.rs index 8806386349..47f9535b83 100644 --- a/src/crates/adapters/claude-code-adapter/src/mcp_source.rs +++ b/src/crates/adapters/claude-code-adapter/src/mcp_source.rs @@ -894,9 +894,7 @@ fn prepare_import_projection( } if environment.is_empty() && working_directory.is_none() => { PreparedExternalMcpImportTransport::Local { command, args } } - PreparedTransportTemplate::Remote { url, headers } - if headers.is_empty() && import_safe_https_url(&url) => - { + PreparedTransportTemplate::Remote { url, headers } if headers.is_empty() => { PreparedExternalMcpImportTransport::Remote { url } } _ => { @@ -1043,17 +1041,6 @@ fn sanitized_https_origin(value: &str) -> Result { Ok(url.to_string()) } -fn import_safe_https_url(value: &str) -> bool { - url::Url::parse(value).is_ok_and(|url| { - url.scheme() == "https" - && url.host_str().is_some() - && url.username().is_empty() - && url.password().is_none() - && url.query().is_none() - && url.fragment().is_none() - }) -} - fn provider_error(suffix: &str, message: &str, transient: bool) -> ExternalSourceProviderError { ExternalSourceProviderError::new(format!("claude.mcp.{suffix}"), message, transient) } diff --git a/src/crates/adapters/codex-adapter/src/mcp_source.rs b/src/crates/adapters/codex-adapter/src/mcp_source.rs index f36ac54a14..7252d1f144 100644 --- a/src/crates/adapters/codex-adapter/src/mcp_source.rs +++ b/src/crates/adapters/codex-adapter/src/mcp_source.rs @@ -4,8 +4,8 @@ use bitfun_product_domains::external_sources::{ ExternalMcpStaticStatus, ExternalMcpTransportKind, ExternalSourceAssetKind, ExternalSourceContext, ExternalSourceDiagnostic, ExternalSourceHealth, ExternalSourceProviderError, ExternalSourceRecord, ExternalSourceScope, ExternalWatchRoot, - PreparedExternalMcpServer, PreparedExternalMcpTransport, SecretValue, SourceKey, - SourceQualifiedMcpServerId, + PreparedExternalMcpImportServer, PreparedExternalMcpImportTransport, PreparedExternalMcpServer, + PreparedExternalMcpTransport, SecretValue, SourceKey, SourceQualifiedMcpServerId, }; use bitfun_static_hook_support::{ read_bounded_text, redacted_executable_preview, resolve_bounded_regular_file, @@ -26,6 +26,7 @@ const MAX_MAP_ENTRIES: usize = 128; const MAX_RUNTIME_TEXT_BYTES: usize = 64 * 1024; const SUPPORTED_FIELDS: &[&str] = &[ + "name", "command", "args", "env", @@ -236,33 +237,14 @@ impl CodexMcpProvider { .map_err(|error| provider_error("snapshot_invalid", &error.to_string(), false))?; Ok(MaterializedSnapshot { snapshot, prepared }) } -} - -impl Default for CodexMcpProvider { - fn default() -> Self { - Self::new(CodexMcpProviderOptions::default()) - } -} -impl ExternalMcpSourceProvider for CodexMcpProvider { - fn identity(&self) -> ExternalMcpProviderIdentity { - ExternalMcpProviderIdentity::new(PROVIDER_ID, ECOSYSTEM_ID, "Codex") - .expect("static Codex MCP provider identity must be valid") - } - - fn discover( - &self, - input: &ExternalMcpDiscoveryInput, - ) -> Result { - self.materialize(input).map(|result| result.snapshot) - } - - fn prepare_server( + fn current_preparation( &self, input: &ExternalMcpDiscoveryInput, server_id: &SourceQualifiedMcpServerId, expected_behavior_version: &str, - ) -> Result { + ) -> Result<(ExternalMcpServerDefinition, PreparedTransportTemplate), ExternalSourceProviderError> + { if server_id.source.provider_id.as_str() != PROVIDER_ID { return Err(provider_error( "identity_mismatch", @@ -276,6 +258,7 @@ impl ExternalMcpSourceProvider for CodexMcpProvider { .servers .iter() .find(|definition| &definition.id == server_id) + .cloned() .ok_or_else(|| { provider_error( "stale_revision", @@ -286,14 +269,16 @@ impl ExternalMcpSourceProvider for CodexMcpProvider { if definition.behavior_version != expected_behavior_version { return Err(provider_error( "stale_revision", - "MCP server behavior changed before activation", + "MCP server behavior changed before preparation", true, )); } - if !matches!(definition.static_status, ExternalMcpStaticStatus::Ready) { + if !definition.source_enabled + || !matches!(definition.static_status, ExternalMcpStaticStatus::Ready) + { return Err(provider_error( "not_activatable", - "MCP server is disabled, unsupported, or invalid", + "MCP server is disabled or unsupported", false, )); } @@ -304,13 +289,55 @@ impl ExternalMcpSourceProvider for CodexMcpProvider { .ok_or_else(|| { provider_error( "preparation_missing", - "MCP runtime preparation is unavailable", + "MCP preparation is unavailable", false, ) })?; + Ok((definition, template)) + } +} + +impl Default for CodexMcpProvider { + fn default() -> Self { + Self::new(CodexMcpProviderOptions::default()) + } +} + +impl ExternalMcpSourceProvider for CodexMcpProvider { + fn identity(&self) -> ExternalMcpProviderIdentity { + ExternalMcpProviderIdentity::new(PROVIDER_ID, ECOSYSTEM_ID, "Codex") + .expect("static Codex MCP provider identity must be valid") + } + + fn discover( + &self, + input: &ExternalMcpDiscoveryInput, + ) -> Result { + self.materialize(input).map(|result| result.snapshot) + } + + fn prepare_server( + &self, + input: &ExternalMcpDiscoveryInput, + server_id: &SourceQualifiedMcpServerId, + expected_behavior_version: &str, + ) -> Result { + let (_, template) = + self.current_preparation(input, server_id, expected_behavior_version)?; prepare_transport(template, server_id.clone(), expected_behavior_version) } + fn prepare_import( + &self, + input: &ExternalMcpDiscoveryInput, + server_id: &SourceQualifiedMcpServerId, + expected_behavior_version: &str, + ) -> Result { + let (definition, template) = + self.current_preparation(input, server_id, expected_behavior_version)?; + prepare_import_projection(definition, template) + } + fn watch_roots(&self, context: &ExternalSourceContext) -> Vec { let mut roots = BTreeMap::from([(self.options.codex_home.clone(), true)]); if self.options.project_config_enabled { @@ -353,6 +380,7 @@ enum PreparedTransportTemplate { environment: BTreeMap, environment_refs: BTreeMap, working_directory: Option, + working_directory_explicit: bool, }, Remote { url: String, @@ -495,6 +523,9 @@ fn materialize_server( .filter(|field| !SUPPORTED_FIELDS.contains(&field.as_str())) .map(|field| format!("Codex MCP field '{field}' is not supported")) .collect::>(); + if object.get("name").is_some_and(|value| !value.is_str()) { + reasons.push("Codex MCP name must be a string".to_string()); + } let enabled = match object.get("enabled") { None => true, Some(Value::Boolean(value)) => *value, @@ -589,6 +620,7 @@ fn materialize_local( } let environment = string_map(object.get("env"), "env", &mut reasons); let environment_refs = environment_refs(object.get("env_vars"), &mut reasons); + let working_directory_explicit = object.contains_key("cwd"); let cwd = string_value_optional(object.get("cwd"), "cwd", &mut reasons).map(PathBuf::from); let cwd = cwd.or_else(|| context.workspace_root.clone()); enforce_size( @@ -637,6 +669,7 @@ fn materialize_local( environment, environment_refs, working_directory: cwd, + working_directory_explicit, }, diagnostics, }) @@ -783,6 +816,7 @@ fn unsupported_local( environment: BTreeMap::new(), environment_refs: BTreeMap::new(), working_directory: None, + working_directory_explicit: false, }, diagnostics: Vec::new(), } @@ -800,6 +834,7 @@ fn prepare_transport( mut environment, environment_refs, working_directory, + working_directory_explicit: _, } => { for (key, reference) in environment_refs { environment.insert(key, resolve_environment(&reference)?); @@ -864,6 +899,61 @@ fn prepare_transport( }) } +fn prepare_import_projection( + definition: ExternalMcpServerDefinition, + template: PreparedTransportTemplate, +) -> Result { + let transport = match template { + PreparedTransportTemplate::Local { + command, + args, + environment, + environment_refs, + working_directory, + working_directory_explicit, + } if environment.is_empty() + && environment_refs.is_empty() + && working_directory.is_none() + && !working_directory_explicit => + { + PreparedExternalMcpImportTransport::Local { command, args } + } + PreparedTransportTemplate::Remote { + url, + headers, + header_refs, + bearer_token_env_var, + oauth_enabled, + } if headers.is_empty() + && header_refs.is_empty() + && bearer_token_env_var.is_none() + && oauth_enabled => + { + PreparedExternalMcpImportTransport::Remote { url } + } + _ => { + return Err(ExternalSourceProviderError::new( + "external_mcp.import_setup_required", + "MCP declaration contains fields that cannot be imported safely", + false, + )); + } + }; + let prepared = PreparedExternalMcpImportServer { + id: definition.id, + behavior_version: definition.behavior_version, + transport, + }; + prepared.validate().map_err(|_| { + ExternalSourceProviderError::new( + "external_mcp.import_setup_required", + "MCP declaration contains fields that cannot be imported safely", + false, + ) + })?; + Ok(prepared) +} + fn string_value(value: Option<&Value>, field: &str, reasons: &mut Vec) -> String { string_value_optional(value, field, reasons).unwrap_or_default() } @@ -1086,6 +1176,9 @@ fn behavior_version( if table.get("required").is_some_and(Value::is_bool) { table.remove("required"); } + if table.get("name").is_some_and(Value::is_str) { + table.remove("name"); + } } let encoded = toml::to_string(&behavior).unwrap_or_default(); revision_key.opaque_revision( diff --git a/src/crates/adapters/codex-adapter/tests/mcp_source.rs b/src/crates/adapters/codex-adapter/tests/mcp_source.rs index c5e4735b4f..698c1c58a9 100644 --- a/src/crates/adapters/codex-adapter/tests/mcp_source.rs +++ b/src/crates/adapters/codex-adapter/tests/mcp_source.rs @@ -2,7 +2,7 @@ use bitfun_codex_adapter::{CodexMcpProvider, CodexMcpProviderOptions}; use bitfun_product_domains::external_sources::{ ExecutionDomainId, ExternalMcpDiscoveryInput, ExternalMcpRevisionKey, ExternalMcpSourceProvider, ExternalMcpStaticStatus, ExternalMcpTransportKind, - ExternalSourceContext, ExternalSourceScope, + ExternalSourceContext, ExternalSourceScope, PreparedExternalMcpImportTransport, }; use std::collections::BTreeSet; use std::fs; @@ -51,6 +51,17 @@ impl Fixture { revision_key: ExternalMcpRevisionKey::new([7; 32]), } } + + fn input_without_workspace(&self) -> ExternalMcpDiscoveryInput { + ExternalMcpDiscoveryInput { + context: ExternalSourceContext { + workspace_root: None, + execution_domain_id: ExecutionDomainId::new("local-user").unwrap(), + }, + suppressed_sources: BTreeSet::new(), + revision_key: ExternalMcpRevisionKey::new([7; 32]), + } + } } fn write(path: impl AsRef, contents: &str) { @@ -178,12 +189,39 @@ X-Env = "CODEX_MCP_MISSING_HEADER" } #[test] -fn native_import_remains_explicitly_unsupported_for_codex_in_c0a() { +fn safe_local_import_preserves_command_arguments_and_ignores_legacy_name() { let fixture = Fixture::new(); write( fixture.codex_home.join("config.toml"), r#"[mcp_servers.docs] command = "docs-mcp" +args = ["--stdio"] +name = "Ignored display label" +"#, + ); + let provider = fixture.provider(); + let input = fixture.input_without_workspace(); + let snapshot = provider.discover(&input).unwrap(); + let server = &snapshot.servers[0]; + + let prepared = provider + .prepare_import(&input, &server.id, &server.behavior_version) + .unwrap(); + assert!(matches!( + prepared.transport, + PreparedExternalMcpImportTransport::Local { ref command, ref args } + if command == "docs-mcp" && args == &["--stdio"] + )); +} + +#[test] +fn workspace_implicit_cwd_requires_setup_instead_of_changing_local_behavior() { + let fixture = Fixture::new(); + write( + fixture.codex_home.join("config.toml"), + r#"[mcp_servers.docs] +command = "node" +args = ["./server.js"] "#, ); let provider = fixture.provider(); @@ -194,7 +232,108 @@ command = "docs-mcp" let error = provider .prepare_import(&input, &server.id, &server.behavior_version) .unwrap_err(); - assert_eq!(error.code, "external_mcp.import_unsupported"); + + assert_eq!(error.code, "external_mcp.import_setup_required"); +} + +#[test] +fn safe_remote_import_preserves_a_clean_https_url() { + let fixture = Fixture::new(); + write( + fixture.codex_home.join("config.toml"), + r#"[mcp_servers.docs] +url = "https://docs.example.test/mcp" +"#, + ); + let provider = fixture.provider(); + let input = fixture.input(); + let snapshot = provider.discover(&input).unwrap(); + let server = &snapshot.servers[0]; + + let prepared = provider + .prepare_import(&input, &server.id, &server.behavior_version) + .unwrap(); + assert!(matches!( + prepared.transport, + PreparedExternalMcpImportTransport::Remote { ref url } + if url == "https://docs.example.test/mcp" + )); +} + +#[test] +fn local_environment_references_and_explicit_cwd_require_setup() { + let fixture = Fixture::new(); + write( + fixture.codex_home.join("config.toml"), + r#"[mcp_servers.literal_env] +command = "literal-env" +env = { TOKEN = "secret" } + +[mcp_servers.referenced_env] +command = "referenced-env" +env_vars = ["TOKEN"] + +[mcp_servers.explicit_cwd] +command = "cwd-server" +cwd = "." +"#, + ); + let provider = fixture.provider(); + let input = fixture.input(); + let snapshot = provider.discover(&input).unwrap(); + + for name in ["literal_env", "referenced_env", "explicit_cwd"] { + let server = snapshot + .servers + .iter() + .find(|server| server.name == name) + .unwrap(); + let error = provider + .prepare_import(&input, &server.id, &server.behavior_version) + .unwrap_err(); + assert_eq!(error.code, "external_mcp.import_setup_required"); + assert!(!error.message.contains("secret")); + } +} + +#[test] +fn remote_headers_bearer_and_unsafe_url_parts_require_setup() { + let fixture = Fixture::new(); + write( + fixture.codex_home.join("config.toml"), + r#"[mcp_servers.literal_header] +url = "https://docs.example.test/mcp" +http_headers = { X-Secret = "secret" } + +[mcp_servers.referenced_header] +url = "https://docs.example.test/mcp" +env_http_headers = { X-Token = "TOKEN" } + +[mcp_servers.bearer] +url = "https://docs.example.test/mcp" +bearer_token_env_var = "TOKEN" + +[mcp_servers.query] +url = "https://docs.example.test/mcp?token=secret" + +[mcp_servers.fragment] +url = "https://docs.example.test/mcp#private" + +[mcp_servers.userinfo] +url = "https://user:secret@docs.example.test/mcp" +"#, + ); + let provider = fixture.provider(); + let input = fixture.input(); + let snapshot = provider.discover(&input).unwrap(); + + for server in &snapshot.servers { + let error = provider + .prepare_import(&input, &server.id, &server.behavior_version) + .unwrap_err(); + assert_eq!(error.code, "external_mcp.import_setup_required"); + assert!(!error.message.contains("secret")); + } } #[test] @@ -277,23 +416,71 @@ required = false } #[test] -fn fields_rejected_by_current_codex_are_not_silently_accepted() { +fn legacy_name_is_ignored_but_invalid_or_runtime_sensitive_fields_stay_unsupported() { let fixture = Fixture::new(); write( fixture.codex_home.join("config.toml"), r#"[mcp_servers.shared] command = "server" name = "Invented display label" + +[mcp_servers.invalid_name] +command = "server" +name = 42 + +[mcp_servers.runtime_control] +command = "server" +tool_timeout_sec = 15 +disabled_tools = ["write"] "#, ); let snapshot = fixture.provider().discover(&fixture.input()).unwrap(); - + let by_name = |name: &str| { + snapshot + .servers + .iter() + .find(|server| server.name == name) + .unwrap() + }; + assert_eq!( + by_name("shared").static_status, + ExternalMcpStaticStatus::Ready + ); assert!(matches!( - &snapshot.servers[0].static_status, + &by_name("invalid_name").static_status, ExternalMcpStaticStatus::Unsupported { reason } - if reason.contains("field 'name' is not supported") + if reason.contains("name must be a string") )); + assert!(matches!( + by_name("runtime_control").static_status, + ExternalMcpStaticStatus::Unsupported { .. } + )); +} + +#[test] +fn ignored_legacy_name_does_not_change_behavior_version() { + let fixture = Fixture::new(); + let config = fixture.codex_home.join("config.toml"); + write( + &config, + r#"[mcp_servers.shared] +command = "server" +name = "First display label" +"#, + ); + let first = fixture.provider().discover(&fixture.input()).unwrap(); + let version = first.servers[0].behavior_version.clone(); + + write( + &config, + r#"[mcp_servers.shared] +command = "server" +name = "Second display label" +"#, + ); + let second = fixture.provider().discover(&fixture.input()).unwrap(); + assert_eq!(second.servers[0].behavior_version, version); } #[test] diff --git a/src/crates/adapters/opencode-adapter/src/mcp_source.rs b/src/crates/adapters/opencode-adapter/src/mcp_source.rs index 58ae481544..226a99acf2 100644 --- a/src/crates/adapters/opencode-adapter/src/mcp_source.rs +++ b/src/crates/adapters/opencode-adapter/src/mcp_source.rs @@ -841,7 +841,7 @@ fn prepare_import_projection( url, headers, oauth_enabled, - } if headers.is_empty() && oauth_enabled && import_safe_https_url(&url) => { + } if headers.is_empty() && oauth_enabled => { PreparedExternalMcpImportTransport::Remote { url } } _ => { @@ -1037,17 +1037,6 @@ fn sanitized_https_url(value: &str) -> Result { Ok(url.to_string()) } -fn import_safe_https_url(value: &str) -> bool { - url::Url::parse(value).is_ok_and(|url| { - url.scheme() == "https" - && url.host_str().is_some() - && url.username().is_empty() - && url.password().is_none() - && url.query().is_none() - && url.fragment().is_none() - }) -} - struct ConfigLayer { path: PathBuf, scope: ExternalSourceScope, @@ -1066,32 +1055,28 @@ fn parse_config_layer( path: &Path, ) -> ParsedConfigLayer { match read_bounded_text(path, MAX_CONFIG_FILE_BYTES) { - Ok(BoundedTextRead::TooLarge) => { - ParsedConfigLayer { - servers: BTreeMap::new(), - diagnostics: vec![ExternalSourceDiagnostic::error( - "opencode.mcp.config_too_large", - "OpenCode config exceeds the 1 MiB compatibility limit", - None, - ) - .with_asset_kind(ExternalSourceAssetKind::Mcp)], - content_version: "too-large".to_string(), - fatal: true, - } - } - Ok(BoundedTextRead::InvalidUtf8) => { - ParsedConfigLayer { - servers: BTreeMap::new(), - diagnostics: vec![ExternalSourceDiagnostic::error( - "opencode.mcp.config_invalid_utf8", - "OpenCode config is not valid UTF-8", - None, - ) - .with_asset_kind(ExternalSourceAssetKind::Mcp)], - content_version: "invalid-utf8".to_string(), - fatal: true, - } - } + Ok(BoundedTextRead::TooLarge) => ParsedConfigLayer { + servers: BTreeMap::new(), + diagnostics: vec![ExternalSourceDiagnostic::error( + "opencode.mcp.config_too_large", + "OpenCode config exceeds the 1 MiB compatibility limit", + None, + ) + .with_asset_kind(ExternalSourceAssetKind::Mcp)], + content_version: "too-large".to_string(), + fatal: true, + }, + Ok(BoundedTextRead::InvalidUtf8) => ParsedConfigLayer { + servers: BTreeMap::new(), + diagnostics: vec![ExternalSourceDiagnostic::error( + "opencode.mcp.config_invalid_utf8", + "OpenCode config is not valid UTF-8", + None, + ) + .with_asset_kind(ExternalSourceAssetKind::Mcp)], + content_version: "invalid-utf8".to_string(), + fatal: true, + }, Ok(BoundedTextRead::Content(content)) => { let content_version = content_version(revision_key, path, content.as_bytes()); let value = match serde_json::from_str::(&strip_jsonc(&content)) { @@ -1137,19 +1122,17 @@ fn parse_config_layer( fatal: false, } } - Err(error) => { - ParsedConfigLayer { - servers: BTreeMap::new(), - diagnostics: vec![ExternalSourceDiagnostic::error( - "opencode.mcp.config_unreadable", - format!("Failed to read OpenCode MCP config: {error}"), - None, - ) - .with_asset_kind(ExternalSourceAssetKind::Mcp)], - content_version: "unreadable".to_string(), - fatal: true, - } - } + Err(error) => ParsedConfigLayer { + servers: BTreeMap::new(), + diagnostics: vec![ExternalSourceDiagnostic::error( + "opencode.mcp.config_unreadable", + format!("Failed to read OpenCode MCP config: {error}"), + None, + ) + .with_asset_kind(ExternalSourceAssetKind::Mcp)], + content_version: "unreadable".to_string(), + fatal: true, + }, } } diff --git a/src/crates/assembly/core/src/agentic/deep_review/capabilities.rs b/src/crates/assembly/core/src/agentic/deep_review/capabilities.rs index ee5444023a..a31e438bf7 100644 --- a/src/crates/assembly/core/src/agentic/deep_review/capabilities.rs +++ b/src/crates/assembly/core/src/agentic/deep_review/capabilities.rs @@ -335,8 +335,14 @@ async fn load_discovered_review_skill( .await .map_err(|error| BitFunError::tool(format!("Failed to read review skill: {error}")))? }; - let mut data = SkillData::from_markdown(info.path.clone(), &markdown, info.level, true) - .map_err(|error| BitFunError::tool(error.to_string()))?; + let mut data = SkillData::from_markdown_for_source_slot( + info.path.clone(), + &markdown, + info.level, + true, + &info.source_slot, + ) + .map_err(|error| BitFunError::tool(error.to_string()))?; data.key = info.key.clone(); data.source_slot = info.source_slot.clone(); data.dir_name = info.dir_name.clone(); @@ -515,6 +521,30 @@ mod tests { assert!(!rendered.contains("full guidance")); } + #[tokio::test] + async fn catalog_loads_claude_review_skill_with_source_semantics() { + let temp = tempfile::tempdir().expect("temporary workspace"); + let skill_dir = temp + .path() + .join(".claude") + .join("skills") + .join("code-review-claude"); + std::fs::create_dir_all(&skill_dir).expect("skill directory"); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\narguments: target\n---\nReview $target for Claude compatibility.\n", + ) + .expect("skill markdown"); + let context = local_tool_context(temp.path().to_path_buf()); + + let descriptor = review_capability_catalog(&context) + .await + .into_iter() + .find(|descriptor| descriptor.key().contains("code-review-claude")); + + assert!(descriptor.is_some()); + } + #[tokio::test] async fn resolve_rejects_skill_when_implicit_policy_changed_after_catalog() { let temp = tempfile::tempdir().expect("temporary workspace"); diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs index b6a7e72282..05bc5acfec 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs @@ -8,7 +8,7 @@ use crate::agentic::tools::framework::{ }; use crate::util::errors::{BitFunError, BitFunResult}; use async_trait::async_trait; -use bitfun_services_core::markdown::expand_prompt_template_arguments; +use bitfun_services_core::markdown::expand_prompt_template_arguments_with_names; use log::debug; use serde_json::{json, Value}; @@ -300,7 +300,11 @@ impl Tool for SkillTool { }; if let Some(arguments) = input.get("arguments").and_then(Value::as_str) { - skill_data.content = expand_prompt_template_arguments(&skill_data.content, arguments); + skill_data.content = expand_prompt_template_arguments_with_names( + &skill_data.content, + arguments, + &skill_data.argument_names, + ); } let location_str = skill_data.location.as_str(); let result_for_assistant = render_loaded_skill_for_assistant(&skill_data, use_stable_key); @@ -424,6 +428,56 @@ Use the remote project skill. } } + struct ClaudeRemoteFs; + + #[async_trait] + impl WorkspaceFileSystem for ClaudeRemoteFs { + async fn read_file(&self, path: &str) -> anyhow::Result> { + Ok(self.read_file_text(path).await?.into_bytes()) + } + + async fn read_file_text(&self, path: &str) -> anyhow::Result { + if path == "/remote/project/.claude/skills/remote-review/SKILL.md" { + return Ok( + "---\ndescription: Review a remote target.\narguments: target focus\n---\n\nReview $target for $focus.\n" + .to_string(), + ); + } + anyhow::bail!("not found: {}", path) + } + + async fn write_file(&self, _path: &str, _contents: &[u8]) -> anyhow::Result<()> { + Ok(()) + } + + async fn exists(&self, path: &str) -> anyhow::Result { + Ok(self.is_dir(path).await? || self.is_file(path).await?) + } + + async fn is_file(&self, path: &str) -> anyhow::Result { + Ok(path == "/remote/project/.claude/skills/remote-review/SKILL.md") + } + + async fn is_dir(&self, path: &str) -> anyhow::Result { + Ok(matches!( + path, + "/remote/project/.claude/skills" | "/remote/project/.claude/skills/remote-review" + )) + } + + async fn read_dir(&self, path: &str) -> anyhow::Result> { + if path == "/remote/project/.claude/skills" { + return Ok(vec![WorkspaceDirEntry { + name: "remote-review".to_string(), + path: "/remote/project/.claude/skills/remote-review".to_string(), + is_dir: true, + is_symlink: false, + }]); + } + Ok(vec![]) + } + } + fn local_context(root: PathBuf) -> crate::agentic::tools::framework::ToolUseContext { crate::agentic::tools::framework::ToolUseContext { tool_call_id: None, @@ -487,6 +541,64 @@ Use the remote project skill. .contains(expected)); } + #[tokio::test] + async fn local_claude_skill_uses_source_semantics_for_discovery_load_and_arguments() { + let temp = tempfile::tempdir().expect("tempdir"); + let skill_dir = temp.path().join(".claude/skills/deploy-service"); + fs::create_dir_all(&skill_dir).expect("skill directory"); + fs::write( + skill_dir.join("SKILL.md"), + "---\ndescription: Deploy a service.\narguments: service environment\n---\n\nDeploy $service to $environment.\n", + ) + .expect("skill markdown"); + let context = local_context(temp.path().to_path_buf()); + + let visible = SkillRegistry::global() + .get_resolved_skills_for_workspace(Some(temp.path()), None) + .await; + assert!(visible + .iter() + .any(|skill| { skill.name == "deploy-service" && skill.source_slot == "claude" })); + + let results = SkillTool::new() + .call_impl( + &json!({ + "command": "deploy-service", + "arguments": "api staging" + }), + &context, + ) + .await + .expect("Claude skill should load with the discovery dialect"); + let ToolResult::Result { data, .. } = &results[0] else { + panic!("expected result payload"); + }; + assert_eq!(data["content"], "Deploy api to staging."); + } + + #[tokio::test] + async fn remote_claude_skill_uses_the_same_dialect_for_discovery_and_load() { + let registry = SkillRegistry::global(); + let visible = registry + .get_resolved_skills_for_remote_workspace(&ClaudeRemoteFs, "/remote/project", None) + .await; + assert!(visible + .iter() + .any(|skill| { skill.name == "remote-review" && skill.source_slot == "claude" })); + + let loaded = registry + .find_and_load_skill_for_remote_workspace( + "remote-review", + &ClaudeRemoteFs, + "/remote/project", + None, + ) + .await + .expect("remote Claude skill should load with the discovery dialect"); + assert_eq!(loaded.name, "remote-review"); + assert_eq!(loaded.argument_names, ["target", "focus"]); + } + #[tokio::test] async fn remote_description_indexes_project_skills_through_workspace_services() { let identity = diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs index 9f44480360..cacd1e1ffd 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs @@ -75,6 +75,16 @@ impl SkillRegistry { } } + fn parse_skill_markdown( + path: String, + content: &str, + location: SkillLocation, + with_content: bool, + source_slot: &str, + ) -> Result { + SkillData::from_markdown_for_source_slot(path, content, location, with_content, source_slot) + } + pub fn global() -> &'static Self { SKILL_REGISTRY.get_or_init(Self::new) } @@ -309,11 +319,12 @@ impl SkillRegistry { } match fs::read_to_string(&skill_md_path).await { - Ok(content) => match SkillData::from_markdown( + Ok(content) => match Self::parse_skill_markdown( path.to_string_lossy().to_string(), &content, entry.level, false, + entry.slot, ) { Ok(mut skill_data) => { Self::apply_local_openai_policy(&mut skill_data, &path).await; @@ -402,11 +413,12 @@ impl SkillRegistry { } match fs.read_file_text(&skill_md_path).await { - Ok(content) => match SkillData::from_markdown( + Ok(content) => match Self::parse_skill_markdown( item.path.clone(), &content, SkillLocation::Project, false, + entry.slot, ) { Ok(mut skill_data) => { Self::apply_remote_openai_policy(&mut skill_data, fs, &item.path).await; @@ -807,8 +819,14 @@ impl SkillRegistry { .await .map_err(|error| BitFunError::tool(format!("Failed to read skill file: {}", error)))?; - let mut data = SkillData::from_markdown(info.path.clone(), &content, info.level, true) - .map_err(|error| BitFunError::tool(error.to_string()))?; + let mut data = Self::parse_skill_markdown( + info.path.clone(), + &content, + info.level, + true, + &info.source_slot, + ) + .map_err(|error| BitFunError::tool(error.to_string()))?; data.key = info.key; data.source_slot = info.source_slot; data.dir_name = info.dir_name; @@ -843,8 +861,14 @@ impl SkillRegistry { .await .map_err(|error| BitFunError::tool(format!("Failed to read skill file: {}", error)))?; - let mut data = SkillData::from_markdown(info.path.clone(), &content, info.level, true) - .map_err(|error| BitFunError::tool(error.to_string()))?; + let mut data = Self::parse_skill_markdown( + info.path.clone(), + &content, + info.level, + true, + &info.source_slot, + ) + .map_err(|error| BitFunError::tool(error.to_string()))?; data.key = info.key; data.source_slot = info.source_slot; data.dir_name = info.dir_name; @@ -868,8 +892,14 @@ impl SkillRegistry { .await?; let content = Self::read_skill_md_for_remote_merge(&info, fs).await?; - let mut data = SkillData::from_markdown(info.path.clone(), &content, info.level, true) - .map_err(|error| BitFunError::tool(error.to_string()))?; + let mut data = Self::parse_skill_markdown( + info.path.clone(), + &content, + info.level, + true, + &info.source_slot, + ) + .map_err(|error| BitFunError::tool(error.to_string()))?; data.key = info.key; data.source_slot = info.source_slot; data.dir_name = info.dir_name; @@ -901,8 +931,14 @@ impl SkillRegistry { })?; let content = Self::read_skill_md_for_remote_merge(&info, fs).await?; - let mut data = SkillData::from_markdown(info.path.clone(), &content, info.level, true) - .map_err(|error| BitFunError::tool(error.to_string()))?; + let mut data = Self::parse_skill_markdown( + info.path.clone(), + &content, + info.level, + true, + &info.source_slot, + ) + .map_err(|error| BitFunError::tool(error.to_string()))?; data.key = info.key; data.source_slot = info.source_slot; data.dir_name = info.dir_name; diff --git a/src/crates/contracts/product-domains/Cargo.toml b/src/crates/contracts/product-domains/Cargo.toml index 4274175d55..3d6be7baad 100644 --- a/src/crates/contracts/product-domains/Cargo.toml +++ b/src/crates/contracts/product-domains/Cargo.toml @@ -38,13 +38,14 @@ sha2 = { workspace = true, optional = true } hex = { workspace = true, optional = true } hmac = { workspace = true, optional = true } which = { workspace = true, optional = true } +url = { workspace = true, optional = true } [features] default = [] plugin-source = ["hex", "sha2"] miniapp = ["dirs", "hex", "sha2", "which"] function-agents = ["log"] -external-sources = ["hex", "hmac", "sha2"] +external-sources = ["hex", "hmac", "sha2", "url"] product-full = ["plugin-source", "miniapp", "function-agents", "external-sources"] [dev-dependencies] diff --git a/src/crates/contracts/product-domains/src/external_sources.rs b/src/crates/contracts/product-domains/src/external_sources.rs index 1d607872de..e7712b2225 100644 --- a/src/crates/contracts/product-domains/src/external_sources.rs +++ b/src/crates/contracts/product-domains/src/external_sources.rs @@ -876,6 +876,20 @@ impl PreparedExternalMcpImportServer { } PreparedExternalMcpImportTransport::Remote { url } => { validate_text(url, "prepared MCP import URL")?; + let parsed = url::Url::parse(url).map_err(|_| { + ExternalSourceContractError::InvalidIdentifier("prepared MCP import URL") + })?; + if parsed.scheme() != "https" + || parsed.host_str().is_none() + || !parsed.username().is_empty() + || parsed.password().is_some() + || parsed.query().is_some() + || parsed.fragment().is_some() + { + return Err(ExternalSourceContractError::InvalidIdentifier( + "prepared MCP import URL", + )); + } } } Ok(()) diff --git a/src/crates/contracts/product-domains/tests/external_source_contracts.rs b/src/crates/contracts/product-domains/tests/external_source_contracts.rs index fcc7d7cdd7..ae8e691695 100644 --- a/src/crates/contracts/product-domains/tests/external_source_contracts.rs +++ b/src/crates/contracts/product-domains/tests/external_source_contracts.rs @@ -85,6 +85,37 @@ fn external_mcp_import_contract_keeps_private_values_out_of_debug_and_requests() assert!(!encoded.contains("argument")); } +#[test] +fn external_mcp_import_contract_rejects_urls_that_cannot_be_copied_losslessly() { + let prepared = |url: &str| PreparedExternalMcpImportServer { + id: SourceQualifiedMcpServerId::new( + SourceKey::new("codex.mcp", "user-config").unwrap(), + "docs", + ) + .unwrap(), + behavior_version: "sha256:behavior-v1".to_string(), + transport: PreparedExternalMcpImportTransport::Remote { + url: url.to_string(), + }, + }; + + prepared("https://docs.example.test/mcp") + .validate() + .unwrap(); + for url in [ + "http://docs.example.test/mcp", + "https://user@docs.example.test/mcp", + "https://user:secret@docs.example.test/mcp", + "https://docs.example.test/mcp?token=secret", + "https://docs.example.test/mcp#private", + ] { + assert!( + prepared(url).validate().is_err(), + "unexpectedly safe: {url}" + ); + } +} + fn source(provider_id: &str, ecosystem_id: &str, source_id: &str) -> ExternalSourceRecord { ExternalSourceRecord { key: SourceKey::new(provider_id, source_id).expect("valid source key"), diff --git a/src/crates/execution/agent-runtime/src/skills/roots.rs b/src/crates/execution/agent-runtime/src/skills/roots.rs index c30490701a..cd63b48046 100644 --- a/src/crates/execution/agent-runtime/src/skills/roots.rs +++ b/src/crates/execution/agent-runtime/src/skills/roots.rs @@ -1,5 +1,7 @@ use std::path::{Path, PathBuf}; +use super::types::SkillSourceDialect; + pub const USER_SKILL_KEY_PREFIX: &str = "user"; pub const PROJECT_SKILL_KEY_PREFIX: &str = "project"; pub const BITFUN_USER_SKILL_SLOT: &str = "bitfun"; @@ -110,6 +112,14 @@ pub const USER_CONFIG_SKILL_ROOTS: &[SkillRootSpec] = &[SkillRootSpec { source_label: "OpenCode", }]; +pub(crate) fn skill_source_dialect(source_slot: &str) -> SkillSourceDialect { + match source_slot { + "claude" | "home.claude" => SkillSourceDialect::ClaudeCode, + "codex" | "home.codex" => SkillSourceDialect::Codex, + _ => SkillSourceDialect::AgentSkills, + } +} + pub fn resolve_user_config_skill_root( spec: &SkillRootSpec, config_dir: &Path, diff --git a/src/crates/execution/agent-runtime/src/skills/types.rs b/src/crates/execution/agent-runtime/src/skills/types.rs index 94c62c5d54..04a43b47c7 100644 --- a/src/crates/execution/agent-runtime/src/skills/types.rs +++ b/src/crates/execution/agent-runtime/src/skills/types.rs @@ -1,8 +1,19 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use serde_yaml::Value; +use std::collections::HashSet; use std::path::Path; +const CLAUDE_DESCRIPTION_MAX_CHARS: usize = 1536; +const CLAUDE_ARGUMENT_NAMES_MAX: usize = 32; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SkillSourceDialect { + AgentSkills, + ClaudeCode, + Codex, +} + #[derive(Debug, Clone, thiserror::Error, PartialEq, Eq)] pub enum SkillParseError { #[error("Invalid SKILL.md format: {0}")] @@ -108,6 +119,7 @@ pub struct SkillData { pub allow_implicit_invocation: bool, pub allow_user_invocation: bool, pub argument_hint: Option, + pub argument_names: Vec, } fn default_allow_implicit_invocation() -> bool { @@ -190,26 +202,219 @@ fn parse_front_matter_markdown(content: &str) -> Result<(Value, String), SkillPa Ok((metadata, markdown_body.to_string())) } +fn directory_name(path: &str) -> Result { + Path::new(path) + .file_name() + .and_then(|value| value.to_str()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) + .ok_or_else(|| SkillParseError::InvalidPath(path.to_string())) +} + +fn first_markdown_paragraph(body: &str) -> Option { + body.replace("\r\n", "\n") + .split("\n\n") + .map(str::trim) + .find(|paragraph| !paragraph.is_empty()) + .map(str::to_string) +} + +fn truncate_chars(value: String, max_chars: usize) -> String { + if value.chars().count() <= max_chars { + return value; + } + value.chars().take(max_chars).collect() +} + +fn claude_description(metadata: &Value, body: &str) -> Result { + let description = optional_string(metadata, "description")? + .filter(|value| !value.trim().is_empty()) + .or_else(|| first_markdown_paragraph(body)) + .unwrap_or_default(); + let when_to_use = + optional_string(metadata, "when_to_use")?.filter(|value| !value.trim().is_empty()); + let description = description.trim(); + if description.is_empty() { + return Err(SkillParseError::MissingField("description")); + } + let combined = match when_to_use.as_deref().map(str::trim) { + None => description.to_string(), + Some(when_to_use) => { + format!("{description}\n\nWhen to use: {when_to_use}") + } + }; + Ok(truncate_chars(combined, CLAUDE_DESCRIPTION_MAX_CHARS)) +} + +fn claude_argument_names(metadata: &Value) -> Result, SkillParseError> { + let Some(value) = metadata.get("arguments") else { + return Ok(Vec::new()); + }; + let names = match value { + Value::String(value) => value.split_whitespace().map(str::to_string).collect(), + Value::Sequence(values) => values + .iter() + .map(|value| { + value.as_str().map(str::to_string).ok_or_else(|| { + SkillParseError::InvalidFormat( + "Field 'arguments' must contain only strings".to_string(), + ) + }) + }) + .collect::, _>>()?, + _ => { + return Err(SkillParseError::InvalidFormat( + "Field 'arguments' must be a string or string list".to_string(), + )); + } + }; + if names.len() > CLAUDE_ARGUMENT_NAMES_MAX { + return Err(SkillParseError::InvalidFormat(format!( + "Field 'arguments' cannot contain more than {CLAUDE_ARGUMENT_NAMES_MAX} names" + ))); + } + + let mut seen = HashSet::with_capacity(names.len()); + for name in &names { + let mut bytes = name.bytes(); + let Some(first) = bytes.next() else { + return Err(SkillParseError::InvalidFormat( + "Field 'arguments' contains an empty name".to_string(), + )); + }; + if name.len() > 64 + || !(first.is_ascii_alphabetic() || first == b'_') + || !bytes.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-')) + { + return Err(SkillParseError::InvalidFormat(format!( + "Invalid argument name '{name}'" + ))); + } + if !seen.insert(name.as_str()) { + return Err(SkillParseError::InvalidFormat(format!( + "Duplicate argument name '{name}'" + ))); + } + } + Ok(names) +} + +fn reject_unsupported_claude_semantics( + metadata: &Value, + body: &str, +) -> Result<(), SkillParseError> { + const UNSUPPORTED_FIELDS: &[&str] = &[ + "context", + "agent", + "model", + "effort", + "hooks", + "paths", + "shell", + "runtime", + "background", + "disallowed-tools", + ]; + if let Some(field) = UNSUPPORTED_FIELDS + .iter() + .find(|field| metadata.get(**field).is_some()) + { + return Err(SkillParseError::InvalidFormat(format!( + "Claude field '{field}' is not supported" + ))); + } + + const DYNAMIC_MARKERS: &[&str] = &[ + "${CLAUDE_SESSION_ID}", + "${CLAUDE_EFFORT}", + "${CLAUDE_SKILL_DIR}", + "!`", + ]; + if let Some(marker) = DYNAMIC_MARKERS + .iter() + .find(|marker| body.contains(**marker)) + { + return Err(SkillParseError::InvalidFormat(format!( + "Claude dynamic expression '{marker}' is not supported" + ))); + } + Ok(()) +} + impl SkillData { pub fn from_markdown( path: String, content: &str, location: SkillLocation, with_content: bool, + ) -> Result { + Self::from_markdown_with_dialect( + path, + content, + location, + with_content, + SkillSourceDialect::AgentSkills, + ) + } + + pub fn from_markdown_for_source_slot( + path: String, + content: &str, + location: SkillLocation, + with_content: bool, + source_slot: &str, + ) -> Result { + Self::from_markdown_with_dialect( + path, + content, + location, + with_content, + super::roots::skill_source_dialect(source_slot), + ) + } + + fn from_markdown_with_dialect( + path: String, + content: &str, + location: SkillLocation, + with_content: bool, + dialect: SkillSourceDialect, ) -> Result { let (metadata, body) = parse_front_matter_markdown(content)?; + let dir_name = directory_name(&path)?; - let name = metadata + let declared_name = metadata .get("name") - .and_then(|value| value.as_str()) - .map(str::to_string) - .ok_or(SkillParseError::MissingField("name"))?; + .map(|value| { + value.as_str().map(str::to_string).ok_or_else(|| { + SkillParseError::InvalidFormat("Field 'name' must be a string".to_string()) + }) + }) + .transpose()?; + let name = match dialect { + SkillSourceDialect::ClaudeCode => dir_name.clone(), + SkillSourceDialect::Codex => declared_name.unwrap_or_else(|| dir_name.clone()), + SkillSourceDialect::AgentSkills => { + declared_name.ok_or(SkillParseError::MissingField("name"))? + } + }; - let description = metadata - .get("description") - .and_then(|value| value.as_str()) - .map(str::to_string) - .ok_or(SkillParseError::MissingField("description"))?; + let description = if dialect == SkillSourceDialect::ClaudeCode { + claude_description(&metadata, &body)? + } else { + metadata + .get("description") + .and_then(|value| value.as_str()) + .map(str::to_string) + .ok_or(SkillParseError::MissingField("description"))? + }; + let argument_names = if dialect == SkillSourceDialect::ClaudeCode { + reject_unsupported_claude_semantics(&metadata, &body)?; + claude_argument_names(&metadata)? + } else { + Vec::new() + }; let allow_implicit_invocation = !optional_claude_bool(&metadata, "disable-model-invocation")?.unwrap_or(false); @@ -218,12 +423,6 @@ impl SkillData { let argument_hint = optional_string(&metadata, "argument-hint")?; let skill_content = if with_content { body } else { String::new() }; - let dir_name = Path::new(&path) - .file_name() - .and_then(|value| value.to_str()) - .ok_or_else(|| SkillParseError::InvalidPath(path.clone()))? - .to_string(); - Ok(SkillData { key: String::new(), name, @@ -236,6 +435,7 @@ impl SkillData { allow_implicit_invocation, allow_user_invocation, argument_hint, + argument_names, }) } diff --git a/src/crates/execution/agent-runtime/tests/skill_contracts.rs b/src/crates/execution/agent-runtime/tests/skill_contracts.rs index 7f5e356474..a58234dded 100644 --- a/src/crates/execution/agent-runtime/tests/skill_contracts.rs +++ b/src/crates/execution/agent-runtime/tests/skill_contracts.rs @@ -8,9 +8,10 @@ use bitfun_agent_runtime::skills::{ resolve_default_hidden_builtin_for_explicit_invocation, resolve_skill_default_enabled_for_mode, resolve_skill_state_for_mode, resolve_user_config_skill_root, resolve_visible_skills, sort_skills, ExplicitSkillInvocationResolution, ModeSkillStateReason, SkillCandidate, - SkillData, SkillInfo, SkillLocation, UserModeSkillOverrides, BITFUN_SYSTEM_SKILL_DIR, - BITFUN_SYSTEM_SKILL_SLOT, BITFUN_USER_SKILL_SLOT, PROJECT_SKILL_KEY_PREFIX, - PROJECT_SKILL_ROOTS, USER_CONFIG_SKILL_ROOTS, USER_HOME_SKILL_ROOTS, USER_SKILL_KEY_PREFIX, + SkillData, SkillInfo, SkillLocation, SkillParseError, UserModeSkillOverrides, + BITFUN_SYSTEM_SKILL_DIR, BITFUN_SYSTEM_SKILL_SLOT, BITFUN_USER_SKILL_SLOT, + PROJECT_SKILL_KEY_PREFIX, PROJECT_SKILL_ROOTS, USER_CONFIG_SKILL_ROOTS, USER_HOME_SKILL_ROOTS, + USER_SKILL_KEY_PREFIX, }; fn builtin_skill(dir_name: &str) -> SkillInfo { @@ -55,6 +56,207 @@ fn custom_user_skill(dir_name: &str) -> SkillInfo { } } +#[test] +fn skill_source_dialect_is_derived_from_the_stable_source_slot() { + let markdown = "---\ndescription: Directory fallback.\n---\n\nBody.\n"; + for source_slot in ["claude", "home.claude", "codex", "home.codex"] { + let parsed = SkillData::from_markdown_for_source_slot( + "/workspace/root/slot-fallback".to_string(), + markdown, + SkillLocation::Project, + false, + source_slot, + ) + .unwrap_or_else(|error| panic!("unexpected dialect for {source_slot}: {error}")); + assert_eq!(parsed.name, "slot-fallback"); + } + for source_slot in ["bitfun", "cursor", "opencode", "agents"] { + assert!(SkillData::from_markdown_for_source_slot( + "/workspace/root/strict".to_string(), + markdown, + SkillLocation::Project, + false, + source_slot, + ) + .is_err()); + } +} + +#[test] +fn claude_skill_uses_directory_identity_and_static_metadata_fallbacks() { + let markdown = r#"--- +name: ignored-display-name +when_to_use: Use for focused security reviews. +arguments: + - target + - focus +allowed-tools: Read, Grep +--- + +Review a change without modifying it. + +Additional workflow details. +"#; + + let data = SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/security-review".to_string(), + markdown, + SkillLocation::Project, + true, + "claude", + ) + .expect("supported Claude skill should parse"); + + assert_eq!(data.name, "security-review"); + assert!(data + .description + .starts_with("Review a change without modifying it.")); + assert!(data + .description + .contains("Use for focused security reviews.")); + assert!(data.description.chars().count() <= 1536); + assert_eq!(data.argument_names, ["target", "focus"]); + assert!(data.content.contains("Additional workflow details.")); +} + +#[test] +fn claude_skill_accepts_whitespace_argument_names_and_explicit_description() { + let markdown = r#"--- +description: Deploy a selected service. +arguments: service environment +--- + +Deploy $service to $environment. +"#; + + let data = SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/deploy".to_string(), + markdown, + SkillLocation::Project, + true, + "claude", + ) + .expect("Claude string arguments should parse"); + + assert_eq!(data.name, "deploy"); + assert_eq!(data.description, "Deploy a selected service."); + assert_eq!(data.argument_names, ["service", "environment"]); +} + +#[test] +fn claude_when_to_use_cannot_replace_a_missing_description() { + let error = SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/empty".to_string(), + "---\nwhen_to_use: Use for empty inputs.\n---\n", + SkillLocation::Project, + false, + "claude", + ) + .expect_err("when_to_use only supplements a description"); + + assert_eq!(error, SkillParseError::MissingField("description")); +} + +#[test] +fn claude_skill_rejects_unavailable_runtime_semantics() { + for field in [ + "context: fork", + "agent: Explore", + "model: opus", + "effort: high", + "hooks: {}", + "paths: src/**", + "shell: bash", + "runtime: node", + "background: true", + "disallowed-tools: Write", + ] { + let markdown = + format!("---\ndescription: Unsupported behavior.\n{field}\n---\n\nDo work.\n"); + let error = SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/unsafe".to_string(), + &markdown, + SkillLocation::Project, + true, + "claude", + ) + .expect_err("unsupported Claude behavior must fail closed"); + assert!(matches!(error, SkillParseError::InvalidFormat(_))); + } + + for body in [ + "Use ${CLAUDE_SESSION_ID}.", + "Use ${CLAUDE_EFFORT}.", + "Read ${CLAUDE_SKILL_DIR}/data.", + "Run !`git status` before continuing.", + ] { + let markdown = format!("---\ndescription: Dynamic behavior.\n---\n\n{body}\n"); + assert!(SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/dynamic".to_string(), + &markdown, + SkillLocation::Project, + true, + "claude", + ) + .is_err()); + } +} + +#[test] +fn claude_skill_validates_argument_names_without_a_generic_schema() { + for arguments in [ + "arguments: target target", + "arguments: target/path", + "arguments:\n - target\n - 42", + ] { + let markdown = format!("---\ndescription: Invalid arguments.\n{arguments}\n---\n\nBody.\n"); + assert!(SkillData::from_markdown_for_source_slot( + "/workspace/.claude/skills/invalid-arguments".to_string(), + &markdown, + SkillLocation::Project, + false, + "claude", + ) + .is_err()); + } +} + +#[test] +fn codex_skill_falls_back_to_directory_name_but_keeps_description_required() { + let data = SkillData::from_markdown_for_source_slot( + "/workspace/.codex/skills/review".to_string(), + "---\ndescription: Review a change.\n---\n\nReview carefully.\n", + SkillLocation::Project, + true, + "codex", + ) + .expect("Codex skill should use its directory name"); + assert_eq!(data.name, "review"); + assert_eq!(data.description, "Review a change."); + + let missing_description = SkillData::from_markdown_for_source_slot( + "/workspace/.codex/skills/review".to_string(), + "---\nname: review\n---\n\nReview carefully.\n", + SkillLocation::Project, + false, + "codex", + ) + .expect_err("Codex description remains required"); + assert_eq!( + missing_description, + SkillParseError::MissingField("description") + ); + + let strict = SkillData::from_markdown( + "/workspace/.agents/skills/review".to_string(), + "---\ndescription: Review a change.\n---\n\nReview carefully.\n", + SkillLocation::Project, + false, + ) + .expect_err("Agent Skills roots keep strict name requirements"); + assert_eq!(strict, SkillParseError::MissingField("name")); +} + fn project_skill(dir_name: &str) -> SkillInfo { SkillInfo { key: format!("project::bitfun::{}", dir_name), diff --git a/src/crates/services/services-core/src/markdown.rs b/src/crates/services/services-core/src/markdown.rs index 09c361269a..cbb4e3a621 100644 --- a/src/crates/services/services-core/src/markdown.rs +++ b/src/crates/services/services-core/src/markdown.rs @@ -13,6 +13,16 @@ static PROMPT_ARGUMENT_REGEX: LazyLock = LazyLock::new(|| { /// Expands Claude-compatible prompt arguments without executing dynamic content. pub fn expand_prompt_template_arguments(template: &str, arguments: &str) -> String { + expand_prompt_template_arguments_with_names(template, arguments, &[]) +} + +/// Expands positional and explicitly declared named prompt arguments without +/// executing dynamic content. +pub fn expand_prompt_template_arguments_with_names( + template: &str, + arguments: &str, + argument_names: &[String], +) -> String { let arguments_by_position = PROMPT_ARGUMENT_REGEX .find_iter(arguments) .map(|item| { @@ -39,7 +49,7 @@ pub fn expand_prompt_template_arguments(template: &str, arguments: &str) -> Stri continue; } if remaining.starts_with(r"\$") { - if let Some(length) = prompt_placeholder_length(&remaining[1..]) { + if let Some(length) = prompt_placeholder_length(&remaining[1..], argument_names) { expanded.push_str(&remaining[1..length + 1]); cursor += length + 1; } else { @@ -64,6 +74,14 @@ pub fn expand_prompt_template_arguments(template: &str, arguments: &str) -> Stri cursor += length; continue; } + if let Some((length, position)) = named_placeholder(remaining, argument_names) { + used_placeholder = true; + if let Some(argument) = arguments_by_position.get(position) { + expanded.push_str(argument); + } + cursor += length; + continue; + } let character = remaining .chars() @@ -80,10 +98,11 @@ pub fn expand_prompt_template_arguments(template: &str, arguments: &str) -> Stri expanded.trim().to_string() } -fn prompt_placeholder_length(value: &str) -> Option { +fn prompt_placeholder_length(value: &str, argument_names: &[String]) -> Option { positional_placeholder(value) .map(|(length, _)| length) .or_else(|| full_arguments_placeholder_length(value)) + .or_else(|| named_placeholder(value, argument_names).map(|(length, _)| length)) } fn full_arguments_placeholder_length(value: &str) -> Option { @@ -114,6 +133,26 @@ fn positional_placeholder(value: &str) -> Option<(usize, Option)> { Some((length + 1, indexed[..length].parse::().ok())) } +fn named_placeholder(value: &str, argument_names: &[String]) -> Option<(usize, usize)> { + let value = value.strip_prefix('$')?; + argument_names + .iter() + .enumerate() + .filter(|(_, name)| value.starts_with(name.as_str())) + .filter(|(_, name)| { + value[name.len()..] + .bytes() + .next() + .is_none_or(|byte| !is_argument_name_byte(byte)) + }) + .max_by_key(|(_, name)| name.len()) + .map(|(position, name)| (name.len() + 1, position)) +} + +fn is_argument_name_byte(byte: u8) -> bool { + byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-') +} + /// Parses and writes Markdown files with YAML front matter. pub struct FrontMatterMarkdown; diff --git a/src/crates/services/services-core/tests/markdown_owner_contracts.rs b/src/crates/services/services-core/tests/markdown_owner_contracts.rs index a09358d58f..963b93193c 100644 --- a/src/crates/services/services-core/tests/markdown_owner_contracts.rs +++ b/src/crates/services/services-core/tests/markdown_owner_contracts.rs @@ -1,4 +1,7 @@ -use bitfun_services_core::markdown::{expand_prompt_template_arguments, FrontMatterMarkdown}; +use bitfun_services_core::markdown::{ + expand_prompt_template_arguments, expand_prompt_template_arguments_with_names, + FrontMatterMarkdown, +}; use std::fs; #[test] @@ -84,3 +87,47 @@ fn prompt_arguments_preserve_backslashes_before_non_placeholders() { "Keep \\$HOME, \\$ARGUMENTS[foo], and \\${CLAUDE_SESSION_ID}\n\nARGUMENTS: alpha" ); } + +#[test] +fn prompt_arguments_expand_declared_names_from_the_existing_argument_list() { + assert_eq!( + expand_prompt_template_arguments_with_names( + "Deploy $service to $environment; positional $0 / $1.", + "api \"staging west\"", + &["service".to_string(), "environment".to_string()], + ), + "Deploy api to staging west; positional api / staging west." + ); +} + +#[test] +fn prompt_arguments_expand_missing_names_to_empty_and_preserve_unknown_names() { + assert_eq!( + expand_prompt_template_arguments_with_names( + "Known: <$target>; missing: <$focus>; unknown: <$owner>.", + "src/lib.rs", + &["target".to_string(), "focus".to_string()], + ), + "Known: ; missing: <>; unknown: <$owner>." + ); +} + +#[test] +fn prompt_arguments_preserve_escaped_declared_names() { + assert_eq!( + expand_prompt_template_arguments_with_names( + r"Use $target and show \$target; keep \\$target expandable.", + "src/lib.rs", + &["target".to_string()], + ), + r"Use src/lib.rs and show $target; keep \\src/lib.rs expandable." + ); +} + +#[test] +fn existing_prompt_argument_api_remains_compatible_without_declared_names() { + assert_eq!( + expand_prompt_template_arguments("Keep $target and use $0", "alpha"), + "Keep $target and use alpha" + ); +} diff --git a/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.test.tsx b/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.test.tsx index a01f1afeb9..dcf7b23e17 100644 --- a/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.test.tsx +++ b/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.test.tsx @@ -222,6 +222,217 @@ describe('ExternalMcpOverview', () => { expect(importArea.textContent).toContain('external.import.applied'); }); + it('locks import choices while an apply request is in flight', async () => { + let resolveApply!: (value: unknown) => void; + applyMcpImportMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveApply = resolve; + })); + await act(async () => { + root.render(); + await Promise.resolve(); + await Promise.resolve(); + }); + const importArea = container.querySelector('[data-testid="external-mcp-import"]')!; + await act(async () => { + (importArea.querySelector('button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + const checkbox = importArea.querySelector('input[type="checkbox"]') as HTMLInputElement; + + await act(async () => { + (importArea.querySelector('.bitfun-mcp-tools__import-actions button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + expect(checkbox.disabled).toBe(true); + + await act(async () => { + resolveApply({ + schemaVersion: 1, + outcome: { + status: 'applied', + imported: [{ candidateId: 'opencode-project-docs', nativeId: 'docs' }], + }, + }); + await Promise.resolve(); + }); + }); + + it('defaults eligible imports to selected and allows a compact per-item choice with source scope', async () => { + const codexSource = { + stableKey: 'codex-mcp-user', + lifecycle: 'available', + record: { + key: { providerId: 'codex.mcp', sourceId: 'user' }, + ecosystemId: 'codex', + displayName: 'Codex user MCP', + sourceKind: 'mcp', + scope: 'user_global', + location: '~/.codex/config.toml', + executionDomainId: 'local:test', + health: 'available', + contentVersion: '1', + }, + }; + getSnapshotMock.mockResolvedValue({ + ...snapshot, + sources: [...snapshot.sources, codexSource], + mcpServers: [ + snapshot.mcpServers[0], + { + ...snapshot.mcpServers[0], + candidateId: 'codex-user-search', + definition: { + ...snapshot.mcpServers[0].definition, + id: { + source: { providerId: 'codex.mcp', sourceId: 'user' }, + localId: 'search', + }, + provenance: [{ providerId: 'codex.mcp', sourceId: 'user' }], + name: 'search', + }, + }, + ], + integrationPolicy: { + ...snapshot.integrationPolicy, + registeredEcosystems: [ + ...snapshot.integrationPolicy.registeredEcosystems, + { + ecosystemId: 'codex', + displayName: 'Codex', + adapterRevision: '1', + capabilities: [], + }, + ], + }, + }); + planMcpImportMock.mockResolvedValue({ + schemaVersion: 1, + planFingerprint: 'sha256:multi', + items: [ + { + candidateId: 'opencode-project-docs', + displayName: 'docs', + transport: 'local_stdio', + proposedNativeId: 'docs', + disposition: 'eligible', + }, + { + candidateId: 'codex-user-search', + displayName: 'search', + transport: 'local_stdio', + proposedNativeId: 'search', + disposition: 'eligible', + }, + ], + }); + + await act(async () => { + root.render(); + await Promise.resolve(); + await Promise.resolve(); + }); + const importArea = container.querySelector('[data-testid="external-mcp-import"]')!; + await act(async () => { + (importArea.querySelector('button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + + const checkboxes = Array.from(importArea.querySelectorAll('input[type="checkbox"]')); + expect(checkboxes).toHaveLength(2); + expect(checkboxes.every((checkbox) => checkbox.checked)).toBe(true); + expect(importArea.textContent).toContain('OpenCode'); + expect(importArea.textContent).toContain('external.scope.project'); + expect(importArea.textContent).toContain('Codex'); + expect(importArea.textContent).toContain('external.scope.userGlobal'); + + await act(async () => { + checkboxes[1].click(); + }); + await act(async () => { + (importArea.querySelector('.bitfun-mcp-tools__import-actions button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + expect(applyMcpImportMock).toHaveBeenCalledWith( + 'D:/workspace/project', + expect.objectContaining({ planFingerprint: 'sha256:multi' }), + [{ candidateId: 'opencode-project-docs' }], + ); + }); + + it('keeps only still-eligible previous selections when a stale plan is refreshed', async () => { + planMcpImportMock.mockResolvedValue({ + schemaVersion: 1, + planFingerprint: 'sha256:old', + items: [ + { + candidateId: 'opencode-project-docs', + displayName: 'docs', + transport: 'local_stdio', + proposedNativeId: 'docs', + disposition: 'eligible', + }, + { + candidateId: 'old-candidate', + displayName: 'old', + transport: 'local_stdio', + proposedNativeId: 'old', + disposition: 'eligible', + }, + ], + }); + applyMcpImportMock.mockResolvedValue({ + schemaVersion: 1, + outcome: { + status: 'stale', + refreshedPlan: { + schemaVersion: 1, + planFingerprint: 'sha256:new', + items: [ + { + candidateId: 'opencode-project-docs', + displayName: 'docs', + transport: 'local_stdio', + proposedNativeId: 'docs', + disposition: 'eligible', + }, + { + candidateId: 'new-candidate', + displayName: 'new', + transport: 'local_stdio', + proposedNativeId: 'new', + disposition: 'eligible', + }, + ], + }, + }, + }); + + await act(async () => { + root.render(); + await Promise.resolve(); + await Promise.resolve(); + }); + const importArea = container.querySelector('[data-testid="external-mcp-import"]')!; + await act(async () => { + (importArea.querySelector('button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + let checkboxes = Array.from(importArea.querySelectorAll('input[type="checkbox"]')); + await act(async () => { + checkboxes[1].click(); + }); + await act(async () => { + (importArea.querySelector('.bitfun-mcp-tools__import-actions button') as HTMLButtonElement).click(); + await Promise.resolve(); + }); + + checkboxes = Array.from(importArea.querySelectorAll('input[type="checkbox"]')); + expect(checkboxes).toHaveLength(2); + expect(checkboxes[0].checked).toBe(true); + expect(checkboxes[1].checked).toBe(false); + expect(importArea.textContent).toContain('external.import.stale'); + }); + it('does not expose import mutation in Peer or remote workspace mode', async () => { peerState.deviceId = 'peer-a'; await act(async () => { diff --git a/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.tsx b/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.tsx index b0a8679c7e..7bd2ff942a 100644 --- a/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.tsx +++ b/src/web-ui/src/infrastructure/config/components/ExternalMcpOverview.tsx @@ -134,6 +134,9 @@ const ExternalMcpOverview: React.FC = () => { const [loading, setLoading] = useState(true); const [loadFailed, setLoadFailed] = useState(false); const [importPlan, setImportPlan] = useState(null); + const [selectedImportCandidateIds, setSelectedImportCandidateIds] = useState>( + () => new Set(), + ); const [importBusy, setImportBusy] = useState(false); const [importNotice, setImportNotice] = useState<'applied' | 'stale' | 'empty' | 'failed' | null>(null); const snapshot = snapshotState?.scope === requestScope ? snapshotState.snapshot : null; @@ -175,6 +178,7 @@ const ExternalMcpOverview: React.FC = () => { useEffect(() => { importRequestIdRef.current += 1; setImportPlan(null); + setSelectedImportCandidateIds(new Set()); setImportNotice(null); setImportBusy(false); }, [requestScope]); @@ -228,6 +232,9 @@ const ExternalMcpOverview: React.FC = () => { return scopeRank(leftSource?.record.scope) - scopeRank(rightSource?.record.scope) || left.definition.name.localeCompare(right.definition.name); }), [snapshot?.mcpServers, sourceByKey]); + const mcpEntryByCandidateId = useMemo(() => new Map( + (snapshot?.mcpServers ?? []).map((entry) => [entry.candidateId, entry]), + ), [snapshot?.mcpServers]); const hostReadOnly = snapshot !== null && !snapshot.hostCapabilities.canMutatePolicy @@ -240,6 +247,9 @@ const ExternalMcpOverview: React.FC = () => { const eligibleImportItems = (importPlan?.items ?? []).filter((item) => ( item.disposition === 'eligible' || item.disposition === 'automatic_rename' )); + const selectedImportItems = eligibleImportItems.filter((item) => ( + selectedImportCandidateIds.has(item.candidateId) + )); const previewImport = async () => { if (!importSupported) return; @@ -253,6 +263,13 @@ const ExternalMcpOverview: React.FC = () => { item.disposition === 'eligible' || item.disposition === 'automatic_rename' )); setImportPlan(hasEligible ? plan : null); + setSelectedImportCandidateIds(new Set( + plan.items + .filter((item) => ( + item.disposition === 'eligible' || item.disposition === 'automatic_rename' + )) + .map((item) => item.candidateId), + )); if (!hasEligible) setImportNotice('empty'); } catch (error) { if (requestId !== importRequestIdRef.current) return; @@ -264,7 +281,7 @@ const ExternalMcpOverview: React.FC = () => { }; const applyImport = async () => { - if (!importSupported || !importPlan || eligibleImportItems.length === 0) return; + if (!importSupported || !importPlan || selectedImportItems.length === 0) return; const requestId = ++importRequestIdRef.current; setImportBusy(true); setImportNotice(null); @@ -272,14 +289,26 @@ const ExternalMcpOverview: React.FC = () => { const result = await externalSourcesAPI.applyMcpImport( workspacePath || undefined, importPlan, - eligibleImportItems.map((item) => ({ candidateId: item.candidateId })), + selectedImportItems.map((item) => ({ candidateId: item.candidateId })), ); if (requestId !== importRequestIdRef.current) return; if (result.outcome.status === 'stale') { - setImportPlan(result.outcome.refreshedPlan); + const refreshedPlan = result.outcome.refreshedPlan; + const refreshedEligibleIds = new Set( + refreshedPlan.items + .filter((item) => ( + item.disposition === 'eligible' || item.disposition === 'automatic_rename' + )) + .map((item) => item.candidateId), + ); + setSelectedImportCandidateIds((current) => new Set( + [...current].filter((candidateId) => refreshedEligibleIds.has(candidateId)), + )); + setImportPlan(refreshedPlan); setImportNotice('stale'); } else { setImportPlan(null); + setSelectedImportCandidateIds(new Set()); setImportNotice('applied'); } } catch (error) { @@ -291,6 +320,22 @@ const ExternalMcpOverview: React.FC = () => { } }; + const cancelImport = () => { + setImportPlan(null); + setSelectedImportCandidateIds(new Set()); + setImportNotice(null); + }; + + const toggleImportCandidate = (candidateId: string) => { + if (importBusy) return; + setSelectedImportCandidateIds((current) => { + const next = new Set(current); + if (next.has(candidateId)) next.delete(candidateId); + else next.add(candidateId); + return next; + }); + }; + const scopeLabel = (scope: ExternalSourceScope | undefined): string => { switch (scope) { case 'user_global': return t('external.scope.userGlobal'); @@ -434,24 +479,56 @@ const ExternalMcpOverview: React.FC = () => { {entries.length > 0 && !hostReadOnly && importSupported ? (
{importPlan ? ( - <> -

{t('external.import.confirm', { count: eligibleImportItems.length })}

-
    - {eligibleImportItems.map((item) => ( -
  • - {item.displayName} → {item.proposedNativeId} -
  • - ))} +
    +

    {t('external.import.confirm', { count: selectedImportItems.length })}

    +
      + {eligibleImportItems.map((item) => { + const catalogEntry = mcpEntryByCandidateId.get(item.candidateId); + const source = catalogEntry ? sourceByKey.get(sourceKey( + catalogEntry.definition.id.source.providerId, + catalogEntry.definition.id.source.sourceId, + )) : undefined; + const ecosystemLabel = source + ? ecosystemLabels.get(source.record.ecosystemId) ?? source.record.ecosystemId + : catalogEntry?.definition.id.source.providerId ?? t('external.unknown'); + const candidateScopeLabel = scopeLabel(source?.record.scope); + return ( +
    • + +
    • + ); + })}
    - -
    - +
    ) : (