Repository navigation
fix(tools): tighten seven tool definitions - #652
aliasunder wants to merge 8 commits into
Conversation
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
Gate the remaining not-found remedies behind tool availability
Pre-existing: the not-found remedies in vault_read_note, vault_move_note, and vault_update_properties, plus vault_write_note's already-exists remedy, still name their referenced tool unconditionally, while this PR gives vault_delete_note and the edit tools a spelled-out fallback when the named tool is disabled. Under the corresponding DISABLED_TOOLS configs the served text directs the model to a tool the server never registered — the drift the availability-keying rule and this PR's noteNotFoundVerifyRemedy pattern exist to prevent. The new test asserting each not-found entry keeps a remedy without vault_list_notes covers five edit tools but omits vault_read_note, whose entry cites the same error and the same tool. Failure scenario: DISABLED_TOOLS=vault_list_notes — a client calls vault_read_note with a typo'd path, gets a not-found error, and the tool description's remedy says to verify with vault_list_notes, a tool absent from the served surface, so the remedy dead-ends; likewise vault_write_note's already-exists remedy under DISABLED_TOOLS=vault_patch_note,vault_replace_in_note points only at the disabled vault_replace_in_note. Suggested fixIn a follow-up (this PR's scope rule freezes other tools' text), route these entries through gated remedy strings like the existing noteNotFoundCheckRemedy/noteNotFoundVerifyRemedy: read_note and move_note take the spelling-and-letter-case fallback, and update_properties and write_note's already-exists remedy fall back to tool-free wording when their named tool is disabled, built like replace_in_note's servedPropertyEditors join.umm-actually · z-ai/glm-5.3-flash |
|
umm-actually re-reviewed at 2 new finding(s) posted (7 tracked finding(s) across all runs). Review phase The review deadline expired; results from completed phases are shown. Context notes
umm-actually · deepseek/deepseek-v4.1-flash, z-ai/glm-5.3-flash |
…5.0 tools - vault_search_by_property: one numeric-matching bullet in place of three - vault_find_orphans: one example; shorter daily-notes default sentence - vault_delete_note: shorter Errors entries, each keeping its remedy - vault_search_by_tag: route past the 20-result cap; drop a schema restatement - vault_get_backlinks: Errors as bullets quoting the message - vault_recent_notes: additional_properties marked optional - vault_list_notes: folder path-error entry cut to its remedy - Error-contract tests for vault_list_notes and vault_list_files folder errors Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- vault_search_by_tag: past-cap guidance moves to When to use and covers notes tagged only with the parent; the Parameters bullet names prefix mode - vault_find_orphans: the defaults sentence opens with "With exclude_folders omitted", so "the defaults" has a referent - vault_search_by_property: the array rule is stated once, the key and value schema text drops the discovery routing, additional_properties? optional - vault_get_backlinks: the empty-result remedy notes vault_list_notes lists notes only - Tests for the two gated text branches no snapshot configuration renders Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- vault_get_backlinks: a self-link counts as a backlink here, but vault_find_orphans ignores self-links, so the routing sentence now says find_orphans finds notes nothing else links to - vault_search_by_property: a checkbox is stored as JSON true/false and matches only as the text "1"/"0", never numerically; the bullet no longer implies a checkbox is a stored number - vault_search_by_tag: the vault_search_by_property route returns at most limit notes, so it no longer promises every note Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- findOrphans takes a readonly excludeFolders list, so vault_find_orphans and the vault-orientation prompt pass their resolved lists directly instead of copying them - vault_delete_note handler: the clearStaleTrashEntry comment names the trash_entries row, why an unrecorded move clears it, and that a "Permanently delete" never calls it - The not-found remedy comment states what the remedies fall back to - vault-orientation catch binding: err -> error Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5f191d5 to
f045678
Compare
- vault_search_by_property: key and value matching named separately, number forms shown as examples, 64-bit float comparison, "true" text-only, limit keeps the newest matches and has no upper cap - vault_find_orphans: the override line names the schema default, the default line drops the unset setting, and keeping a default points to vault_get_daily_note for the daily notes folder - vault_search_by_tag: frontmatter tags only, nested tags spelled out, "promoted" replaced; the interim past-cap routing is removed ahead of a limit parameter - vault_get_backlinks: notes and canvases named; canvas paths come from vault_list_files - vault_recent_notes: the created sort's consequence stated; invalid created values are null; sort_by described as the timestamp - vault_delete_note: Behavior, opener and Returns render per Obsidian Sync setting, so a Sync server no longer claims the Deleted files setting applies; remedies that need hidden paths or env vars go to the vault owner or server operator - vault_list_notes: subfolder listing tied to the glob, code-unit sort order, and the path-error remedy offered as an alternative Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- vault_delete_note's broken-links entry with vault_get_backlinks disabled, and its protected-path list under PROTECTED_PATHS. - vault_find_orphans leaves the daily-note hint out under an ORPHAN_EXCLUDE_FOLDERS list. - vault_delete_note's Returns line per OBSIDIAN_SYNC. - The memory-dir protected-path test now asserts the whole entry; its "Profile/" substring also matched the when-to-use line. - findDescriptionLine moves to module scope beside extractDescriptionSection and takes the registered calls. - The obsidian-sync combo comment now says OBSIDIAN_SYNC changes vault_delete_note's text, not only its Errors list. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- vault_delete_note (Sync): the server, not Obsidian Sync, bypasses the Deleted files setting - vault_recent_notes: undated notes can be left out by a small limit, not always - vault_get_backlinks: an empty result means nothing links to the path; a markdown link to a missing note still counts as a backlink Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt remedy The remedy now lists whichever of vault_search, vault_list_notes and vault_list_files the server serves, and falls back to checking the path when none is. A test covers all three served, one served, and none served. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re: umm-actually finding 6021116393 (gate the remaining not-found remedies behind tool availability). Valid. The four remaining ungated references, in vault_read_note, vault_move_note, vault_update_properties and vault_write_note's already-exists remedy, are text in tools this PR doesn't edit. The maintainer approved fixing them together with every other ungated tool-to-tool reference in one dedicated pass, so each of those tools is re-graded once. The same pass covers vault_read_note's missing case in the not-found remedy test. In this PR, vault_get_backlinks' remedy now names only served tools (e08f6f6), and vault_delete_note's not-found remedy already falls back when vault_list_notes is disabled. 🔍 ship-check · pr-monitor · claude-opus-5-5 |
| '- "cannot move to trash …', | ||
| '- any other "cannot move to trash …"', |
There was a problem hiding this comment.
Assert full delete-note error entries, not slices at the first em dash
Low severity · tests · high confidence
Pre-existing: deleteNoteErrorLeads slices each Errors entry at its first ' — ', and for the 'cannot move to trash … — 100 collisions in .trash/' entry that em dash sits inside the quoted message, so the assertion covers only '- “cannot move to trash …' and never that entry's remedy. A dropped or reworded remedy on that entry — the thing the Errors convention requires every bullet to keep — passes the test silently, while a punctuation-only change to the same message would fail it.
Failure scenario: An edit that shortens the collisions entry's remedy (e.g. drops 'ask the vault owner to clear old copies, then retry') leaves the produced line starting with the same '- “cannot move to trash …' prefix, so deleteNoteErrorLeads still equals the expectation and the test stays green with the required remedy gone.
Suggested fix
Assert the whole Errors section between 'Errors:' and '\n\nReturns:' as literal text, the way the new opener/Behavior assertions do, instead of slicing each entry at its first em dash.umm-actually · deepseek/deepseek-v4.1-flash
|
Index the singular tag frontmatter property too
Pre-existing: upsertNote fills the notes.tags column only from the frontmatter Failure scenario: A note declares Suggested fixIn upsertNote, build the stored tags column from both spellings — read frontmatter.tag alongside frontmatter.tags and deduplicate the combined list — and add a fixture note using the singular spelling so the tag tools' Obsidian parity is pinned by a test.umm-actually · z-ai/glm-5.3-flash |
1 participant
Summary
Tightens the definitions of seven tools. Four score below 4.8 on Glama's tool-definition grader, and each change answers the reason the grader gave. The other three are at 5.0 and get completeness fixes. Review findings and a reader's pass then corrected statements that contradicted the code and clarified what a cold reader could not follow. The default tool list shrinks by 662 characters.
vault_search_by_propertylimitkeeps the newest matches and has no upper cap. The array rule is stated oncevault_find_orphansvault_get_daily_notefor the daily notes folder. The override line names the schema default instead of an env var the client can't seevault_delete_notevault_search_by_tag#tagsare not indexed) and spells out nested tags. Drops the sentence the schema already states and the undefined "promoted keys". The cap itself is answered by alimitparameter, approved by the maintainer to ship in the same releasevault_get_backlinks"path must end in …". Names notes and canvases as sources. Routes tovault_find_orphansfor notes nothing else links to, since self-links count as backlinks but not against orphan status. An empty result means nothing links to the path (a markdown link to a missing note still counts). The empty-result remedy names only the path-finding tools the server servesvault_recent_notesadditional_properties?marked optional. States that a small limit can leave out notes without a validcreated.createdis null when missing or invalid.sort_bydescribed as the timestamp to sort byvault_list_notesDefault tool list: 126,205 → 125,543 characters. Only these seven tools change in any of the 18 snapshot configurations (six in the read-only ones, which do not serve
vault_delete_note). Server instructions and prompts are unchanged.Code-only cleanups:
findOrphanstakes a readonly array, so two defensive copies are gone. Comments on the trash bookkeeping and the not-found remedies are clarified.Tests
vault_list_notesandvault_list_filesreject an absolute folder, a folder outside the vault,.and a hidden folder over the real transport. No test covered these listed errors before.tool-definitions.test.ts:vault_delete_note:OBSIDIAN_SYNC, asserted whole;vault_list_notesdisabled;vault_get_backlinksdisabled;PROTECTED_PATHSand memory-folder protected paths, asserted as whole lines.vault_find_orphans:vault_get_daily_notehint with that tool disabled and under an override list.vault_get_backlinks: its empty-result remedy with every, one and no path-finding tool served.vault_recent_notes: its sort line, asserted whole.npm test: 4,735 passed. Build, lint, knip and prettier pass.Review
Not in this change. Each item was reviewed and scheduled by the maintainer:
vault_list_files' folder error entry.vault_read_noteandvault_move_note. They are grouped into one pass so each tool is re-graded once.vault_list_notes;🤖 Generated with Claude Code