Skip to content

fix(filesystem): note allowed-directories scope on read_file - #4569

Open
yash161004 wants to merge 1 commit into
modelcontextprotocol:mainfrom
yash161004:fix/read-file-allowed-directories-note
Open

fix(filesystem): note allowed-directories scope on read_file#4569
yash161004 wants to merge 1 commit into
modelcontextprotocol:mainfrom
yash161004:fix/read-file-allowed-directories-note

Conversation

@yash161004

Copy link
Copy Markdown

What

Adds the access-boundary sentence to read_file's description in the filesystem
server, matching every other tool it ships with.

-    description: "Read the complete contents of a file as text. DEPRECATED: Use read_text_file instead.",
+    description: "Read the complete contents of a file as text. DEPRECATED: Use read_text_file instead. Only works within allowed directories.",

Why

All 14 filesystem tools end their description with "Only works within allowed
directories."
— except read_file, the deprecated alias:

Tool States the boundary
read_file no
read_text_file, read_media_file, read_multiple_files, write_file, edit_file, create_directory, list_directory, list_directory_with_sizes, directory_tree, move_file, search_files, get_file_info, list_allowed_directories yes

read_file is registered with ReadTextFileArgsSchema.shape and dispatches to
readTextFileHandler — the same schema and the same handler as
read_text_file. The boundary is therefore enforced identically; the two differ
only in how they are described.

This is a documentation gap, not a security issue. Path validation is
unchanged and works correctly. Nothing here suggests unrestricted access.

It's worth fixing because the description is the part an LLM agent reasons over
when selecting a tool. read_text_file tells the model it is constrained to
allowed directories; its alias, backed by the identical code path, does not — so
the same operation is presented to the model two different ways. Restating the
constraint costs one sentence and removes the discrepancy.

Notes

  • One-line change to a string literal; no behaviour, schema, or handler changes.
  • No test asserts this string (it appears exactly once in the tree).
  • Consistent with the recent metadata tidy-up in fix(filesystem): declare openWorldHint on all tools #4480, which brought
    openWorldHint into line across the same tool set.

Found while auditing MCP tool definitions for description/behaviour drift.
Prepared with assistance from Claude.

Every filesystem tool description ends by stating the access boundary --
"Only works within allowed directories." -- except read_file, which is the
one deprecated alias.

read_file is registered with ReadTextFileArgsSchema.shape and dispatches to
readTextFileHandler, so it is the same schema and the same handler as
read_text_file and enforces the same boundary. Only the description differs,
which makes this a documentation gap rather than a behavioural one.

It matters because the description is the part an LLM agent reasons over when
choosing a tool. read_text_file states the constraint; its alias does not, so
the same underlying operation is described to the model two different ways.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
Verifying all six candidate findings against upstream source showed only one
was real. Two of the other five were the linter's fault, and both are fixed
here. On the same 85 tools across 11 public servers, missing-scope findings go
from 10 to 2, with no false positives left.

1. states_scope() only recognised filesystem phrasing. A tool that says it acts
   "from the knowledge graph" or "on the SQLite database" states its boundary
   just as precisely as "within allowed directories" does, but was treated as
   unscoped -- so every in-memory and database server looked like it was hiding
   something. It now recognises a named subsystem, whether it appears after a
   preposition or as a direct object ("Read the entire knowledge graph").
   Generic objects are deliberately still not boundaries: "on the host" and
   "in the system" name no limit at all.

2. DESTRUCTIVE_VERBS ignored word sense, so "drop" matched the Playwright
   drag-and-drop tools. Ambiguous verbs move to CONTEXTUAL_VERBS and count only
   next to a qualifying object -- "drop the users table" does, "drop a file onto
   the page" does not. upload and transfer are removed outright: moving data is
   not destroying it.

Also requires 8 tools before inferring a boundary convention. On a three-tool
server two agreeing is 67% and indicts the third; only one convention departure
survived verification across all 85 tools, on a 14-tool server, so a real sample
costs nothing and removes the small-server noise.

The regression tests use the real upstream description strings, not invented
ones, and assert the constraint that matters: read_file on the filesystem server
must keep firing. It is the finding that verification confirmed and that PR
modelcontextprotocol/servers#4569 fixes. A change that silences it has gone too
far, whatever else it improves.

Separately, the sweep script wrote its output under docs/, which is published.
It now writes to the gitignored .private/ -- that output pairs named third-party
servers with asserted findings, and committing it would breach
docs/DISCLOSURE.md.

145 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
HANDOFF still said the findings were unverified and that deciding whether to
disclose them was the next task. Both are now false: all six were verified
against upstream on 2026-07-29, only F-001 was real, and it is fixed by PR
modelcontextprotocol/servers#4569. A fresh agent reading HANDOFF and TASKS
together would have got contradictory instructions.

Also updates the test count and points the next-work section at TASKS.md rather
than duplicating it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
PR #4 added REMAINING_WORK.md while I was adding TASKS.md, leaving two competing
task documents that disagreed. REMAINING_WORK.md wins: it is broader, covering
fixtura and OpenEval as well as mcplock. TASKS.md is deleted and its unique
content folded in.

Its section 3.1 was stale in a way that mattered -- it said the findings were
unverified and whether to disclose was undecided. Both were resolved on
2026-07-29: all six were verified against upstream source, only F-001 was real,
and it is fixed by PR modelcontextprotocol/servers#4569. Two of the other five
were mcplock's own false positives, both since fixed, taking the sweep from 10
scope findings to 2 with none false. Left as written, a fresh agent would have
redone finished verification work and possibly sent findings that were retracted.

Adds the three missing Phase 6 items -- running the Action in a real external
repo, the writeup, and registry investigation -- with the real numbers the
writeup should use, and the note that the honest framing is that most candidates
did not survive verification.

Also corrects paths from docs/ to .private/ and the test count to 145.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
The anonymized version of the internal draft. Names only
@modelcontextprotocol/server-filesystem, which is already public through
PR modelcontextprotocol/servers#4569; every other server in the sweep is
referred to by capability only, because no other finding cleared the bar in
docs/DISCLOSURE.md.

Four factual corrections were made against the source data before publishing,
rather than against the previous draft:

- The two post-fix scope findings are on two different servers, not both on
  the filesystem server. The earlier text would have attributed a second
  defect to maintainers who do not own it.
- Ambiguity findings are counted sweep-wide (6, four of them on the
  filesystem server), matching how the scope bullet beside it counts.
- The attestations claim is now accurate: Trusted Publishing is configured
  and verified, but no version currently on PyPI carries provenance. The
  previous wording contradicted the supply-chain section of the same document.
- The OIDC failure count is exact: five attempts across two tagged releases,
  confirmed from the Actions history rather than from notes.

docs/ is excluded from the sdist allowlist, so this does not ship to PyPI;
tests/test_packaging.py covers that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
Verifying all six candidate findings against upstream source showed only one
was real. Two of the other five were the linter's fault, and both are fixed
here. On the same 85 tools across 11 public servers, missing-scope findings go
from 10 to 2, with no false positives left.

1. states_scope() only recognised filesystem phrasing. A tool that says it acts
   "from the knowledge graph" or "on the SQLite database" states its boundary
   just as precisely as "within allowed directories" does, but was treated as
   unscoped -- so every in-memory and database server looked like it was hiding
   something. It now recognises a named subsystem, whether it appears after a
   preposition or as a direct object ("Read the entire knowledge graph").
   Generic objects are deliberately still not boundaries: "on the host" and
   "in the system" name no limit at all.

2. DESTRUCTIVE_VERBS ignored word sense, so "drop" matched the Playwright
   drag-and-drop tools. Ambiguous verbs move to CONTEXTUAL_VERBS and count only
   next to a qualifying object -- "drop the users table" does, "drop a file onto
   the page" does not. upload and transfer are removed outright: moving data is
   not destroying it.

Also requires 8 tools before inferring a boundary convention. On a three-tool
server two agreeing is 67% and indicts the third; only one convention departure
survived verification across all 85 tools, on a 14-tool server, so a real sample
costs nothing and removes the small-server noise.

The regression tests use the real upstream description strings, not invented
ones, and assert the constraint that matters: read_file on the filesystem server
must keep firing. It is the finding that verification confirmed and that PR
modelcontextprotocol/servers#4569 fixes. A change that silences it has gone too
far, whatever else it improves.

Separately, the sweep script wrote its output under docs/, which is published.
It now writes to the gitignored .private/ -- that output pairs named third-party
servers with asserted findings, and committing it would breach
docs/DISCLOSURE.md.

145 tests pass.
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
HANDOFF still said the findings were unverified and that deciding whether to
disclose them was the next task. Both are now false: all six were verified
against upstream on 2026-07-29, only F-001 was real, and it is fixed by PR
modelcontextprotocol/servers#4569. A fresh agent reading HANDOFF and TASKS
together would have got contradictory instructions.

Also updates the test count and points the next-work section at TASKS.md rather
than duplicating it.
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
PR #4 added REMAINING_WORK.md while I was adding TASKS.md, leaving two competing
task documents that disagreed. REMAINING_WORK.md wins: it is broader, covering
fixtura and OpenEval as well as mcplock. TASKS.md is deleted and its unique
content folded in.

Its section 3.1 was stale in a way that mattered -- it said the findings were
unverified and whether to disclose was undecided. Both were resolved on
2026-07-29: all six were verified against upstream source, only F-001 was real,
and it is fixed by PR modelcontextprotocol/servers#4569. Two of the other five
were mcplock's own false positives, both since fixed, taking the sweep from 10
scope findings to 2 with none false. Left as written, a fresh agent would have
redone finished verification work and possibly sent findings that were retracted.

Adds the three missing Phase 6 items -- running the Action in a real external
repo, the writeup, and registry investigation -- with the real numbers the
writeup should use, and the note that the honest framing is that most candidates
did not survive verification.

Also corrects paths from docs/ to .private/ and the test count to 145.
yash161004 added a commit to yash161004/mcplock that referenced this pull request Jul 29, 2026
The anonymized version of the internal draft. Names only
@modelcontextprotocol/server-filesystem, which is already public through
PR modelcontextprotocol/servers#4569; every other server in the sweep is
referred to by capability only, because no other finding cleared the bar in
docs/DISCLOSURE.md.

Four factual corrections were made against the source data before publishing,
rather than against the previous draft:

- The two post-fix scope findings are on two different servers, not both on
  the filesystem server. The earlier text would have attributed a second
  defect to maintainers who do not own it.
- Ambiguity findings are counted sweep-wide (6, four of them on the
  filesystem server), matching how the scope bullet beside it counts.
- The attestations claim is now accurate: Trusted Publishing is configured
  and verified, but no version currently on PyPI carries provenance. The
  previous wording contradicted the supply-chain section of the same document.
- The OIDC failure count is exact: five attempts across two tagged releases,
  confirmed from the Actions history rather than from notes.

docs/ is excluded from the sdist allowlist, so this does not ship to PyPI;
tests/test_packaging.py covers that.
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