fix(mcp-filesense): enforce navigate writeMode instead of silently ignoring it - #403
fix(mcp-filesense): enforce navigate writeMode instead of silently ignoring it#403ceilf6 wants to merge 1 commit into
Conversation
…noring it navigate 的 schema 声明了 writeMode,tools.ts 也把它传进了 engine,但 engine.navigate() 从未读取该参数——声明了却没接上,调用方无从得知写盘 请求被丢弃。 修法遵循 navigate 的只读定位(executor 的 SecurityManager 把 filesense_navigate 归入 READ_TOOLS,自动放行、不走写审批): - none/cache:维持不写盘,现在是显式契约而非巧合; - workspace:直接拒绝并提示改用 filesense_sync。若真去写 FILES.json,等于给一个只读归类的工具凭空加上免审批的写能力。 同步修正 schema description,使其描述实际行为。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
代码评审报告: fix(mcp-filesense): enforce navigate writeMode instead of silently ignoring it
风险等级: 中
处理建议: 请求修改
决策摘要: 安全论证成立、engine 层拒绝写盘的方向正确,但 writeMode: 'workspace' 是 README 已文档化的环境变量取值,会经 planner-skills.ts:151 注入到每个 navigate 步骤,而 navigate 的工具错误被 executor 当作非致命静默跳过——配置了 workspace 的用户会从"能导航但不写盘"退化为"每次任务完全拿不到导航上下文且无可见报错";合并前需要在配置/schema 边界处理这个取值。
级联分析
- 变更符号:
navigate()(packages/mcp-filesense/src/engine.ts:636)、filesenseNavigateSchema.writeMode描述(tools.ts:179) - 受影响流程:
handleFilesenseTool('filesense_navigate')→engine.navigate()(tools.ts:291-314,throw 被tools.ts:319catch 成{success:false,error});planner-skills.ts:132-156注入的phase.filesense-navigate步骤 →executor.ts:155-178透传step.params→executor-skills.ts:355-364shouldSkipToolError恒为true(仅 debug 日志) - 变更集外调用方:
packages/mcp-file/src/server.ts:155、packages/runtime-node/src/mcp-clients.ts:69均经handleFilesenseTool,不会抛出未捕获异常(text search 确认 enginenavigate无其他直接调用方,与 PR 的 impactedCount 0 一致);配置来源runtime-node/src/config.ts:349-351接受workspace - 置信度: medium (text search + 文件阅读,未使用代码图谱)
问题发现
-
[高] 文档化的
FRONTAGENT_FILESENSE_WRITE_MODE=workspace会让 navigate 步骤整体静默失效- 证据:
README.md:467/docs/README-CN.md:477把workspace列为合法取值;config.ts:350读取该环境变量并由normalizeFilesenseWriteMode(config.ts:150-154) 接受;planner-skills.ts:151把filesense?.writeMode ?? 'cache'放进每个 navigate 步骤的 params;executor.ts:155,177原样传给工具;工具层返回success:false后executor-skills.ts:357-363恒返回true跳过,非 debug 模式下无任何输出。 - 受影响调用方/流程: 所有设置了该环境变量的运行;退化方向与 PR 想修的缺陷同类——从"参数被忽略"变成"整个导航阶段被无声丢弃"。
- 最小可行修复: 在配置或 planner 边界把 navigate 的
workspace降级为只读并打一次显式 warning(例如planner-skills.ts:151处filesense?.writeMode === 'workspace' ? 'none' : (filesense?.writeMode ?? 'cache')),engine 层保留硬拒绝作为最后防线;同时更新 README/README-CN 说明workspace仅对filesense_sync有意义。
- 证据:
-
[中] schema 仍然向模型提供一个必然失败的枚举值
- 证据:
tools.ts:177的enum: ['cache','workspace','none']未变,仅描述改成"会被拒绝"(tools.ts:179)。LLM 生成工具参数时仍可能选中workspace,结果同样落入非致命跳过路径,损失不可见。 - 受影响调用方/流程: LLM 直接构造
filesense_navigate调用的所有路径。 - 最小可行修复: 从
filesenseNavigateSchema的 enum 中移除'workspace'(保留 engine 侧校验),让非法取值在 schema 层就无法被选出。
- 证据:
-
[低] 行为变更未同步到面向用户的文档
- 证据: PR checklist 勾选了"behavior 变更已更新 docs/tests",但
README.md:467、docs/README-CN.md:477仍无差别地列出三个取值。 - 受影响调用方/流程: 按文档配置的使用者。
- 最小可行修复: 在两处注释里标注
workspace只作用于 sync 类工具,navigate 会拒绝。
- 证据: PR checklist 勾选了"behavior 变更已更新 docs/tests",但
-
[低]
cache分支的测试断言没有钉住所声明的语义- 证据:
engine.test.ts:204-213对cache只断言工作区不存在FILES.json,与已有用例engine.test.ts:162-183高度重叠;PR 声称cache是"无缓存位置概念的非写别名",但没有断言任何位置都没有写入。 - 受影响调用方/流程: 未来若真的引入缓存目录,这条测试不会发现语义漂移。
- 最小可行修复: 断言调用前后临时目录(或注入的 boundary)文件集合完全不变,而不仅是单个文件名。
- 证据:
行级发现
- [packages/mcp-filesense/src/engine.ts:640] 硬 throw 在 executor 里会被
executor-skills.ts:357-363无声吞掉;建议只把它作为防线,并在配置/planner 边界把workspace降级为只读 + 一次显式 warning,避免导航阶段整体失效。 - [packages/mcp-filesense/src/tools.ts:179] 描述已说明
workspace会被拒绝,但同一 schema 的 enum 仍提供该值;请从 enum 中移除'workspace',否则模型仍会选中一个必然失败的取值。 - [packages/mcp-filesense/src/engine.test.ts:205]
cache分支只检查工作区FILES.json不存在,无法证明"任何位置都不写";改为比对调用前后目录内容快照更能锁定该契约。
Karpathy 评审
- 假设: PR 假设"默认路径为
cache,故默认不受影响"成立,但漏掉了workspace是文档化配置取值这一前提;也假设 fail-fast 的错误对调用方可见,而 navigate 的错误在 executor 中被设计为非致命跳过。 - 简洁性: 改动很小,没有引入抽象、flag 或新层,实现方式与既有只读定位一致。
- 结构质量: 校验放在 engine 入口、扫描之前,位置合理;没有共享路径特例化、没有薄 wrapper、没有文件规模问题。缺口在于校验层级与配置层未对齐(配置层仍接受该值)。
- 变更范围: 与声明目标一致,无无关重构或格式噪声。
- 验证: 覆盖了 engine 层与工具层两条路径(
tools.security.test.ts:128-142是有价值的端到端参数传递验证);缺的是 planner/executor 这条真正会被配置触发的流程。
缺失覆盖
- planner 注入路径的测试:
filesense.writeMode = 'workspace'配置下,navigate 步骤应仍能返回导航上下文(或至少产生可见 warning),而不是被静默跳过。 - executor 层测试:确认 navigate 因参数被拒绝时的可观测性(当前只有
runtime.debug下的 console 输出,executor-skills.test.ts:222只验证了跳过行为本身)。 - 若采纳移除 enum 中
workspace,需要一条 schema 快照/契约测试防止回退。
| // so it is auto-allowed without write approval. Honouring writeMode:'workspace' | ||
| // here would hand a read-classified tool an unapproved write primitive. | ||
| const writeMode = options.writeMode ?? 'none'; | ||
| if (writeMode === 'workspace') { |
There was a problem hiding this comment.
硬 throw 在 executor 里会被 executor-skills.ts:357-363 无声吞掉;建议只把它作为防线,并在配置/planner 边界把 workspace 降级为只读 + 一次显式 warning,避免导航阶段整体失效。
| enum: ['cache', 'workspace', 'none'], | ||
| description: '写入模式。当前 navigate 默认 none/cache 语义,不写业务目录。', | ||
| description: | ||
| "写入模式,默认 none。navigate 是只读工具:none/cache 均不写盘;'workspace' 会被拒绝,写索引请用 filesense_sync。", |
There was a problem hiding this comment.
描述已说明 workspace 会被拒绝,但同一 schema 的 enum 仍提供该值;请从 enum 中移除 'workspace',否则模型仍会选中一个必然失败的取值。
| await fs.writeFile(path.join(TEST_DIR, 'src', 'main.tsx'), 'export const main = 1;'); | ||
|
|
||
| for (const writeMode of ['none', 'cache'] as const) { | ||
| const result = await navigate(TEST_DIR, { paths: ['.'], depth: 1, writeMode }); |
There was a problem hiding this comment.
cache 分支只检查工作区 FILES.json 不存在,无法证明"任何位置都不写";改为比对调用前后目录内容快照更能锁定该契约。
Linked Issue Or Context
class. This is the same shape of bug: a parameter is declared in the tool
schema, typed, and forwarded — and then dropped on the floor.
Summary
filesense_navigatedeclares awriteModeparameter ('cache' | 'workspace' | 'none') in its MCP schema,types.tstypes it onNavigateOptions, andtools.tsforwards it into the engine. Butengine.navigate()never read it.A caller asking for a write got silence: no write, no warning, no error.
The fix follows navigate's read-only role rather than implementing a write:
none/cache(and the default) — still no disk writes, but now that is anenforced contract instead of an accident.
workspace— rejected with an actionable error pointing atfilesense_sync.Why reject rather than implement. The executor's
SecurityManagerputsfilesense_navigateinREAD_TOOLS(packages/core/src/security.ts), so it isauto-allowed at low risk with no approval prompt, while write tools
(
create_file,apply_patch) andfilesense_syncgo through the write/approvalpath. Making navigate honour
writeMode: 'workspace'would hand aread-classified, approval-exempt tool a workspace write primitive — a security
regression. The repo already encodes this expectation in an existing test,
navigate returns compact summary without writing workspace indexes.If maintainers do want a persisting navigate, the right shape is a separate
approval-gated tool (or reclassifying navigate out of
READ_TOOLS), which is amaintainer-level decision and deliberately not taken here. Likewise,
'cache'is kept as a non-writing alias because the package has no cache-location concept
today; inventing one would be new architecture.
Impact Scope
packages/mcp-filesense/src/engine.ts—navigate()now reads and enforceswriteMode(fails fast, before any scanning).packages/mcp-filesense/src/tools.ts— schema description corrected todescribe actual behaviour.
engine.test.ts(+2),tools.security.test.ts(+1).writeMode: 'workspace'previously succeeded while doingnothing; it now returns
{ success: false, error }from the tool layer.Callers relying on the silent no-op get an explicit error instead — nothing
that previously wrote stops writing.
@frontagent/coredefaults towriteMode: 'cache'(planner-skills.ts), so the default path is unaffected.GitNexus Impact Summary
mcp-boundary(packages/mcp-filesense/src/), with matching in-package tests included.detect_changes --scope compare --base-ref developreports 5 files / 4 symbols / 4 affected execution flows at medium risk;impact navigate --direction upstreamreturns impactedCount 0 at LOW risk. Breakdown below.pnpm quality:precommitexit 0;pnpm quality:local(incl.build:verify) green on pre-push.Breakdown:
detect_changes --scope compare --base-ref develop: 5 files, 4 symbols(
writeMode,depth,navigate,filesenseNavigateSchema), 4 affectedexecution flows, reported risk medium. All four flows are
Navigate → *internals.impact navigate --direction upstream: impactedCount 0, risk LOW —no in-repo callers of the engine function; it is reached through the MCP
tool layer, which the new
tools.security.test.tscase covers end to end.Verification
npx vitest run packages/mcp-filesense— 63 passed (3 new).navigate rejects writeMode workspace instead of silently ignoring itnavigate treats writeMode none and cache as read-onlynavigate rejects a workspace writeMode passed through the tool layer(drives
handleFilesenseTool, so it proves the parameter actually travelsargs → engine, which is the part that was broken)
pnpm quality:precommit— exit 0, turbo 25/25 tasks,test:workflows34/34.pnpm quality:localran on pre-push, includingbuild:verify.Checklist
pnpm quality:precommit, or explained why it could not run.pnpm quality:localfor critical skeleton changes, or explained why it could not run.