Skip to content

feat(mcp): observe file paths from MCP tools for glob rule matching - #93

Open
dzuyonak wants to merge 3 commits into
frap129:devfrom
dzuyonak:feat/mcp-observations-and-fixes
Open

dzuyonak wants to merge 3 commits into
frap129:devfrom
dzuyonak:feat/mcp-observations-and-fixes

Conversation

@dzuyonak

@dzuyonak dzuyonak commented Nov 4, 2026

Copy link
Copy Markdown

Summary

globs-based rules attach from FileObservation records. Today those records
are only produced by the native file-handling tools (read, write, edit,
apply_patch, lsp).

When an agent reads or searches the workspace through an MCP server (semantic
indexer, ripgrep wrapper, language server, filesystem or memory server), the
returned paths never become observations, so matching rules never attach.

This PR adds a language-agnostic observation normalizer for MCP tool results.
Native tool behavior is unchanged.

Implementation

  1. MCP tool detection

    • mcp__<server>__<tool>.
    • OpenCode's <server>_<tool> naming (for example 1c-index_read_method),
      matched by a whole segment from read, find, search, lookup,
      inspect, info, get, grep, list, scan, query, glob.
    • Native tools are explicitly excluded to avoid double observations.
  2. Path extraction

    • Key/value fields: path=, file=, file_path=, filePath=,
      relativePath=, module_path=, filename=; quoted values, spaces, and
      non-ASCII/Cyrillic paths.
    • Grep/ripgrep-style rows: - src/a.go:10, src/a.go:10:5, → path,
      -> path, and bare path lists.
    • JSON payloads: recursive walk for path-like keys, including arrays under
      results or items.
    • Tool arguments: when a server accepts a target path and returns only the
      file content.
  3. Normalization

    • Windows separators are normalized to /.
    • URLs, .git/, node_modules/, wildcard masks, and version-like tokens
      such as 1.2.3 are rejected.
    • Paths are deduplicated and capped at 5 per call.

Also in this PR

  • Windows: windowsHide: true when resolving the git branch, so the
    background service does not flash a console window on every dispatch.
  • Keywords: Unicode-aware leading boundary instead of ASCII \b, so
    Cyrillic and other non-ASCII keywords match.

Compatibility

  • Native read/write/edit/apply_patch/lsp observations are untouched.
  • No configuration changes and no new dependencies.
  • Only paths already present in model-visible tool output or arguments are
    parsed; no extra filesystem access.

Tests

Added coverage in src/session/file-observation.test.ts for TypeScript,
Python, Go, Rust, Vue, and 1C paths with spaces/Cyrillic names, plus
grep-style rows, JSON payloads, argument fallback, URL/noise rejection, and
the 5-path cap.

Targeted run:

npx tsc --noEmit
npx eslint src/session/file-observation.ts src/session/file-observation.test.ts src/rules/rule-filter.ts src/detection/git-branch.ts
npx prettier --check --end-of-line auto src/session/file-observation.ts src/session/file-observation.test.ts src/rules/rule-filter.ts src/detection/git-branch.ts src/index.rules.test.ts
npx vitest run src/session/file-observation.test.ts src/index.rules.test.ts src/detection/git-branch.test.ts

Result: 137 passed (16 file-observation, 110 index.rules, 11 git-branch).

The full suite still reports 5 pre-existing Windows-only failures in
src/config/plugin-options.test.ts, tui/data/rules.test.ts, and
src/index.runtime.test.ts (POSIX path expectations, file permissions, and a
shell side-effect marker). They are unrelated to this change.

Commits

  1. feat(mcp): observe file paths from MCP tool results — main change.
  2. fix(windows): hide git subprocess console window — no console flashes.
  3. fix(keywords): support non-ASCII keywords — Unicode keyword matching.

Parse workspace paths from MCP tool output and arguments so glob-based rules attach when code is inspected through an MCP server instead of the native read tool. Handles key/value fields, grep-style rows, JSON payloads, and tool arguments for any language.

@frap129 frap129 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Line numbers refer to pr-93 for changed files and main for docs.

Review: request changes

Thanks for the PR. The windowsHide fix (src/detection/git-branch.ts:19) and the Unicode keyword boundary (src/rules/rule-filter.ts:84-86, tests at src/index.rules.test.ts:857-868) are useful. The MCP observation change is the blocker.

Blocking: this reverses the File-observation trust boundary

The repo deliberately narrowed file matching in #38/#58:

  • #58 acceptance criterion 2: “Grep, Glob, shell, custom and MCP tools contribute neither paths nor content, whether successful or failed.”
  • #38 story 30: “I want Grep, Glob, shell, custom, MCP, and other unsupported tools excluded.”
  • #38 implementation decisions: “Only successful Read, Write, Edit, Apply Patch, and LSP events contribute observations.”

normalizeObservations now calls mcpObservations before the native-tool gate (src/session/file-observation.ts:361-365), and mcpObservations admits MCP paths/content (src/session/file-observation.ts:320-358). This is not a missing case; it is the opposite of the settled design. To land it, we need a new issue/spec that explicitly changes #38/#58 and the documentation. Otherwise I would split this PR down to the Windows and keyword fixes.

If the decision is revisited, these need to be fixed:

  1. Make MCP observations path-only. src/session/file-observation.ts:358 sets:

    return [...paths].slice(0, 5).map(path => ({ path, tool, content: output }));

    The contract at src/session/file-observation.ts:1-3 says content is “that file's contribution text.” src/rules/rule-filter.ts:125-127 then applies contentMatchesLiterals to that content. A multi-file MCP result now gives every path the entire result, so globs: src/b.go plus fileContains: TODO can match because src/a.go contained TODO. The goal here is glob matching, so use content: '' unless per-path text can be derived. The new tests currently assert the wrong contract at src/session/file-observation.test.ts:158 and :301.

  2. Use real MCP tool identity. isMcpFileTool at src/session/file-observation.ts:188-195 accepts every mcp__* tool (:189) and any non-denylisted name containing read, find, get, list, info, search, etc. (:191-194). A custom repo_search or get_config now contributes observations, which #58 explicitly excludes. Prefer the connected MCP server/tool data already available to the runtime (src/detection/mcp-tools.ts) over a name heuristic.

  3. Only use argument paths when the output has no paths. src/session/file-observation.ts:334-335 calls argPaths(event.args) unconditionally; the walker is at :272-295. The PR body says arguments are used “when a server accepts a target path and returns only the file content.” As written, an argument path can be paired with another file's output, and a successful no-results search with a file argument still creates an observation.

  4. Harden the parser or narrow its accepted forms.

    • linePath at src/session/file-observation.ts:237-249 accepts prose: linePath('Found in src/a.ts:10') returns Found in src/a.ts, which **/*.ts matches.
    • IGNORED_PATH_PREFIXES at src/session/file-observation.ts:206, used at :304, only rejects .git/ and node_modules/ at the start, so ./node_modules/x.ts and src/node_modules/x.ts pass.
    • The version check at src/session/file-observation.ts:310-315 lets docs/1.2.3 and v1.2.3 through.
  5. Update the docs. README.md:112, :246, :290, :568, and :704 still describe only live Read, Write, Edit, Apply Patch, and LSP as sources. docs/rules.md:72 and :285 say the same. CONTEXT.md:14-16 defines a File observation as the path and file text one event exposes. This is a hard violation of .opencode/rules/11-readme-and-doc-sync.md:19.

  6. Add an end-to-end test. The new coverage is confined to src/session/file-observation.test.ts:139-304. Nothing drives an MCP tool.execute.after event through the runtime and asserts that a glob rule is admitted. The existing native/excluded-tool test is at src/index.runtime.test.ts:549; add the MCP case alongside it.

Other issues

  • -> path is claimed but not parsed. The PR body lists -> path, but src/session/file-observation.ts:238 uses:

    let rest = line.trim().replace(/^(?:[-*>]|→|->)\s*/, '');

    The [-*>] alternative consumes the leading - before -> is tried, leaving > path. normalizePathCandidate then rejects > via ILLEGAL_PATH_CHARS at :207/:307. Reorder the alternatives or use /^(?:->|→|[-*>])\s*/.

  • Keyword fallback. src/rules/rule-filter.ts:84-89 falls back to lowerPrompt.includes(lowerKeyword) when the regex constructor throws. That would match mid-word and contradict the leading-boundary tests at src/index.rules.test.ts:857-868. It is unreachable on ES2022; remove it or use a boundary-preserving fallback.

  • windowsHide is untested. src/detection/git-branch.ts:19 adds the option, but src/detection/git-branch.test.ts:27-39 only asserts cwd, timeout, and killSignal.

Nits

  • collectJsonPaths (src/session/file-observation.ts:252-270) and argPaths (:272-295) are the same recursive walker with a different sink and depth limit; one shared helper would remove the duplication.
  • The extension pattern \.[A-Za-z0-9_-]{1,12} is repeated at src/session/file-observation.ts:204, :241, and :248; make it a named constant.

Verdict: request changes. The Windows and keyword commits look good and can land separately; the MCP observation feature needs a spec decision before code review can approve it.

@frap129
frap129 changed the base branch from main to dev November 12, 2026 16:35
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.

2 participants