Skip to content

fix(mcp-filesense): enforce navigate writeMode instead of silently ignoring it - #403

Open
ceilf6 wants to merge 1 commit into
developfrom
fix/filesense-write-mode
Open

fix(mcp-filesense): enforce navigate writeMode instead of silently ignoring it#403
ceilf6 wants to merge 1 commit into
developfrom
fix/filesense-write-mode

Conversation

@ceilf6

@ceilf6 ceilf6 commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Linked Issue Or Context

Summary

filesense_navigate declares a writeMode parameter ('cache' | 'workspace' | 'none') in its MCP schema, types.ts types it on NavigateOptions, and
tools.ts forwards it into the engine. But engine.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 an
    enforced contract instead of an accident.
  • workspace — rejected with an actionable error pointing at filesense_sync.

Why reject rather than implement. The executor's SecurityManager puts
filesense_navigate in READ_TOOLS (packages/core/src/security.ts), so it is
auto-allowed at low risk with no approval prompt, while write tools
(create_file, apply_patch) and filesense_sync go through the write/approval
path. Making navigate honour writeMode: 'workspace' would hand a
read-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 a
maintainer-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.tsnavigate() now reads and enforces
    writeMode (fails fast, before any scanning).
  • packages/mcp-filesense/src/tools.ts — schema description corrected to
    describe actual behaviour.
  • Tests: engine.test.ts (+2), tools.security.test.ts (+1).
  • Behaviour change: writeMode: 'workspace' previously succeeded while doing
    nothing; 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/core defaults to
    writeMode: 'cache' (planner-skills.ts), so the default path is unaffected.

GitNexus Impact Summary

  • Risk level: LOW
  • Critical skeleton changes: yes — mcp-boundary (packages/mcp-filesense/src/), with matching in-package tests included.
  • GitNexus impact: detect_changes --scope compare --base-ref develop reports 5 files / 4 symbols / 4 affected execution flows at medium risk; impact navigate --direction upstream returns impactedCount 0 at LOW risk. Breakdown below.
  • Verification: pnpm quality:precommit exit 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 affected
    execution 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.ts case covers end to end.

Verification

  • npx vitest run packages/mcp-filesense — 63 passed (3 new).
    • navigate rejects writeMode workspace instead of silently ignoring it
    • navigate treats writeMode none and cache as read-only
    • navigate rejects a workspace writeMode passed through the tool layer
      (drives handleFilesenseTool, so it proves the parameter actually travels
      args → engine, which is the part that was broken)
  • pnpm quality:precommit — exit 0, turbo 25/25 tasks, test:workflows 34/34.
  • pnpm quality:local ran on pre-push, including build:verify.

Checklist

  • I have linked an issue or explained why this PR stands alone.
  • I have kept the diff focused on the stated change.
  • I have run pnpm quality:precommit, or explained why it could not run.
  • I have run pnpm quality:local for critical skeleton changes, or explained why it could not run.
  • I have updated docs or tests when behavior, public APIs, or Harness contracts changed.
  • For critical skeleton changes, I have filled the GitNexus impact summary with concrete results.

…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>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ ceilf6/repo-guard

代码评审报告: 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:319 catch 成 {success:false,error});planner-skills.ts:132-156 注入的 phase.filesense-navigate 步骤 → executor.ts:155-178 透传 step.paramsexecutor-skills.ts:355-364 shouldSkipToolError 恒为 true(仅 debug 日志)
  • 变更集外调用方: packages/mcp-file/src/server.ts:155packages/runtime-node/src/mcp-clients.ts:69 均经 handleFilesenseTool,不会抛出未捕获异常(text search 确认 engine navigate 无其他直接调用方,与 PR 的 impactedCount 0 一致);配置来源 runtime-node/src/config.ts:349-351 接受 workspace
  • 置信度: medium (text search + 文件阅读,未使用代码图谱)

问题发现

  1. [高] 文档化的 FRONTAGENT_FILESENSE_WRITE_MODE=workspace 会让 navigate 步骤整体静默失效

    • 证据: README.md:467 / docs/README-CN.md:477workspace 列为合法取值;config.ts:350 读取该环境变量并由 normalizeFilesenseWriteMode(config.ts:150-154) 接受;planner-skills.ts:151filesense?.writeMode ?? 'cache' 放进每个 navigate 步骤的 params;executor.ts:155,177 原样传给工具;工具层返回 success:falseexecutor-skills.ts:357-363 恒返回 true 跳过,非 debug 模式下无任何输出。
    • 受影响调用方/流程: 所有设置了该环境变量的运行;退化方向与 PR 想修的缺陷同类——从"参数被忽略"变成"整个导航阶段被无声丢弃"。
    • 最小可行修复: 在配置或 planner 边界把 navigate 的 workspace 降级为只读并打一次显式 warning(例如 planner-skills.ts:151filesense?.writeMode === 'workspace' ? 'none' : (filesense?.writeMode ?? 'cache')),engine 层保留硬拒绝作为最后防线;同时更新 README/README-CN 说明 workspace 仅对 filesense_sync 有意义。
  2. [中] schema 仍然向模型提供一个必然失败的枚举值

    • 证据: tools.ts:177enum: ['cache','workspace','none'] 未变,仅描述改成"会被拒绝"(tools.ts:179)。LLM 生成工具参数时仍可能选中 workspace,结果同样落入非致命跳过路径,损失不可见。
    • 受影响调用方/流程: LLM 直接构造 filesense_navigate 调用的所有路径。
    • 最小可行修复: 从 filesenseNavigateSchema 的 enum 中移除 'workspace'(保留 engine 侧校验),让非法取值在 schema 层就无法被选出。
  3. [低] 行为变更未同步到面向用户的文档

    • 证据: PR checklist 勾选了"behavior 变更已更新 docs/tests",但 README.md:467docs/README-CN.md:477 仍无差别地列出三个取值。
    • 受影响调用方/流程: 按文档配置的使用者。
    • 最小可行修复: 在两处注释里标注 workspace 只作用于 sync 类工具,navigate 会拒绝。
  4. [低] cache 分支的测试断言没有钉住所声明的语义

    • 证据: engine.test.ts:204-213cache 只断言工作区不存在 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') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

硬 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。",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

描述已说明 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 });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cache 分支只检查工作区 FILES.json 不存在,无法证明"任何位置都不写";改为比对调用前后目录内容快照更能锁定该契约。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant