fix(filesystem): note allowed-directories scope on read_file - #4569
Open
yash161004 wants to merge 1 commit into
Open
fix(filesystem): note allowed-directories scope on read_file#4569yash161004 wants to merge 1 commit into
yash161004 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the access-boundary sentence to
read_file's description in the filesystemserver, matching every other tool it ships with.
Why
All 14 filesystem tools end their description with "Only works within allowed
directories." — except
read_file, the deprecated alias:read_fileread_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_directoriesread_fileis registered withReadTextFileArgsSchema.shapeand dispatches toreadTextFileHandler— the same schema and the same handler asread_text_file. The boundary is therefore enforced identically; the two differonly 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_filetells the model it is constrained toallowed 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
openWorldHintinto line across the same tool set.Found while auditing MCP tool definitions for description/behaviour drift.
Prepared with assistance from Claude.