feat(mcp): accept training-data-friendly parameter aliases#766
feat(mcp): accept training-data-friendly parameter aliases#766phernandez merged 3 commits intomainfrom
Conversation
Models reach for common parameter names from training data (offset/limit
instead of page/page_size, find/old_text instead of find_text, etc.) on
first use of MCP tools. Pydantic validation errors fire, the model loses
trust in the tool, and falls back to filesystem reads — bypassing the
knowledge graph entirely.
Use Pydantic v2 AliasChoices on tool parameters so the canonical names
are still the only thing advertised in the JSON schema, but common
alternatives are silently accepted at validation time.
Aliases added across 12 tools:
- pagination: page←offset/page_number, page_size←limit/per_page
- search: query←q/search/text, after_date←since/after/from_date,
min_similarity←threshold/similarity_threshold,
note_types←note_type/types
- time: timeframe←since/time_range/lookback
- paths: directory←folder/dir/path, dir_name←directory/folder/...,
path←file_path/filepath/file
- move: destination_path←to/dest_path/new_path/destination
- edit: find_text←find/old_text/old_content/search,
content←new_content/replacement/replace_with,
section←section_heading/heading
- flags: overwrite←force/replace, is_directory←is_dir
- context: url←uri/memory_url, max_related←max_results
Closes #690
Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 19s —— View job PR Review: feat(mcp): accept training-data-friendly parameter aliases
SummaryThis is a well-motivated, cleanly executed PR. The use of Code Quality & Standards ✅
The inline comments in # find_text is the highest-frequency miss per the issue: models reach for
# find/old_text/old_content/search before find_text every time.Testing & Documentation ✅ (with minor gaps)
Three coverage gaps:
Architecture Compliance ✅
Security & Performance ✅
The aliases are accepted only at Pydantic validation time and are then assigned to the canonical Python parameter, so all downstream validation (path traversal, type coercion, etc.) is completely unaffected. Compatibility ✅
Minor Observations (not blockers)
Checklist
Overall: ✅ Approve with minor suggestions. The two missing functional alias tests for |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87285adf12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| page_size: int = 10, | ||
| page: Annotated[ | ||
| int, | ||
| Field(default=1, validation_alias=AliasChoices("page", "offset", "page_number")), |
There was a problem hiding this comment.
Mapping offset directly onto page changes query semantics and returns the wrong slice for common inputs. In this codebase, downstream search pagination is page-based (offset = (page - 1) * page_size in the API layer), so a call like offset=20, limit=10 now fetches page 20 instead of items 21–30, and offset=0 can produce invalid page handling in other tools that validate page >= 1. If offset is accepted as an alias, it needs translation to a 1-indexed page value rather than direct binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 07798a5. Dropped offset as an alias for page across all five paginated tools (read_note, search_notes, recent_activity, view_note, build_context) since the semantics differ (item-index vs. 1-indexed page-number). Kept the safe synonyms (page_number, limit, per_page). Added test_offset_is_not_aliased_to_page as a regression guard so this doesn't sneak back in.
Codex review caught a real bug: aliasing `offset` to `page` silently returns the wrong slice. `offset` is item-indexed (skip N items); `page` is 1-indexed page-number. A call like `offset=20, limit=10` would expect items 21-30 but our binding turned it into page 20 → items 191-200. The original issue (#690) listed `offset` as an alias for `page`, but the semantics are different enough that silent acceptance is worse than rejection. Dropped the alias across all five paginated tools (read_note, search_notes, recent_activity, view_note, build_context) and added a regression-guard test so a future contributor doesn't reintroduce it. Other review feedback addressed: - dropped `limit_related` alias for `max_related` (not a natural convention, `max_results` already covers the use case) - added functional integration test for view_note pagination aliases - added functional integration test for delete_note `is_dir` alias Kept aliases that are true synonyms: `page_number` (≡ page), `limit`/`per_page` (≡ page_size, both mean "max items per response"). Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 1m 49s —— View job PR Review: feat(mcp): accept training-data-friendly parameter aliases
SummaryThis sync addresses all three concerns from the previous round:
Code Quality & Standards ✅
The inline comment is consistent and precise across all five paginated tools ( # `offset` is intentionally NOT aliased: it has different semantics
# (item-indexed vs. 1-indexed page-number).Testing & Documentation ✅
One minor observation (not a blocker): Architecture Compliance ✅
Security & Performance ✅
Compatibility ✅
Checklist
Overall: ✅ Approve. All issues from the previous review have been addressed. The |
|
Addressed both reviews in 07798a5: Codex (P2 — bug): Dropped Claude review minors:
Test counts: 22 passed in the alias suite (was 19), full MCP integration suite still green. |
…liases Codex/Claude review noted an asymmetry: the schema-sanity test's must_not_have lists include `offset` (for page) and `limit_related` (for max_related) even though those aliases were never accepted in the code. That's intentional — listing rejected aliases here acts as a regression guard so a future contributor can't reintroduce them without the test catching it. Made that intent explicit in the docstring so the entries don't read as dead code. No behavior change; doc-only. Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 14s —— View job PR Review: feat(mcp): accept training-data-friendly parameter aliases
SummaryThis is the third pass. Both blocking issues from previous rounds are resolved: the Code Quality & Standards ✅
The # `offset` is intentionally NOT aliased: it has different semantics
# (item-indexed vs. 1-indexed page-number).Testing & Documentation ✅
One trivial observation (not a blocker): Architecture Compliance ✅
Security & Performance ✅
Compatibility ✅
Checklist
Overall: ✅ Approve. All issues from prior reviews are resolved. The PR is well-motivated, cleanly implemented, and thoroughly tested. Ready to merge. |
1 participant
Summary
offset/limit,find/old_text,folder/dir, etc. on first use of our MCP tools. Pydantic validation errors fire, the model loses trust in the tool, and falls back to raw filesystem reads — bypassing the knowledge graph entirely. (See Add Pydantic AliasChoices to MCP tool parameters #690 for the full diagnosis.)AliasChoicesto parameters across 12 MCP tools. Canonical names remain the only thing advertised in the JSON schema, so models still learn the right name — but common alternatives are silently accepted at validation time as a safety net.Tools updated
read_note,edit_note,search_notes,recent_activity,list_directory,write_note,move_note,delete_note,read_content,view_note,build_context,canvasAliases (selected highlights)
page/page_sizeoffset,limit,page_number,per_pagequeryq,search,textqis the HTTP conventionfind_textfind,old_text,old_content,searchcontent(in edit_note)new_content,replacement,replace_withcontentis ambiguous as "replacement text"sectionsection_heading,headingdirectory/dir_namefolder,dir,pathdestination_pathto,dest_path,new_path,destinationtimeframesince,time_range,lookbackis_directoryis_dirurl(build_context)uri,memory_urlThe full alias map is in the commit message.
Design note
The advertised JSON schema continues to show only the canonical names. Aliases live entirely on the validation layer — silent safety net, not advertised API. This preserves the ability to deprecate aliases later without a breaking schema change. A schema-sanity test (
test_aliases_not_advertised_in_schema) enforces this invariant.Test plan
test-int/mcp/test_param_aliases_integration.pycovering each tool's aliasesruff checkandty checkcleanCloses #690
🤖 Generated with Claude Code