docs: lift Glama TDQS scores on the lowest-scoring tools#66
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds modification-time-based result ordering to folder searches and expands documentation across several vault tools to clarify semantics around deletion permanence, memory updates, link handling, and folder matching behavior. ChangesSearch Ordering and Tool Documentation Updates
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Add parameter-semantics, behavior, and usage detail to the six lowest-scoring tools (find_orphans, delete_note, get_backlinks, get_outgoing_links, search_by_folder, update_memory) so descriptions explain non-obvious intent beyond the schema. https://claude.ai/code/session_01KnvnMoDuU6QkYrCna54CqT
The query omitted ORDER BY, so results came back in arbitrary order despite the tool documenting "sorted by most recently modified." Add ORDER BY mtime DESC to match sibling metadata tools. https://claude.ai/code/session_01KnvnMoDuU6QkYrCna54CqT
aliasunder
force-pushed
the
claude/vault-bootstrap-setup-kzwu2
branch
from
3a0c0eb to
6f3c871
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vault-mcp/search/search-index.ts`:
- Around line 637-638: The SQL ORDER BY clause currently uses only "ORDER BY
mtime DESC" which yields non-deterministic ordering for rows with equal mtime;
update the query in src/vault-mcp/search/search-index.ts to add a deterministic
secondary sort key (e.g., change the ORDER BY to "ORDER BY mtime DESC, path
ASC") so results are stable when mtime ties occur (the clause near the LIMIT ?
should be adjusted accordingly).
In `@src/vault-mcp/tool-definitions.ts`:
- Around line 698-710: The vault_search_by_folder tool description is missing an
explicit "Errors:" section; update its description string to include an Errors:
paragraph (e.g., "Errors: returns empty array for empty/nonexistent folders;
other failures will throw an error with remediation guidance such as checking
folder path and permissions.") and do the same for the other updated tool
descriptions in this file (the neighboring tools referenced in the review
ranges), ensuring each description includes "Errors:" with a short remediation
note while keeping existing Example:, When to use:, Obsidian syntax: (for write
tools), and Returns: sections intact; search for the vault_search_by_folder
entry name to locate the block and mirror the Errors: pattern into the other
affected tool description constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 102ce834-37f9-426f-b93c-262800b9d6c2
📒 Files selected for processing (4)
README.mdsrc/vault-mcp/search/__tests__/search-index.test.tssrc/vault-mcp/search/search-index.tssrc/vault-mcp/tool-definitions.ts
Soften vault_delete_note's deletion claim — the server has no undo, but recovery may be possible via backups/sync history, so drop the absolute "irreversible / cannot be recovered" wording. Add concise Errors sections clarifying the empty-result-is-not-an-error contract on the link/folder/orphan query tools, and reconcile the AGENTS.md Errors convention to match (required only where a tool can fail or has a no-match contract worth stating). https://claude.ai/code/session_01KnvnMoDuU6QkYrCna54CqT
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 178-183: Test coverage is missing an assertion for the mandatory
"Returns:" section in tool descriptions; update the unit test in
src/vault-mcp/__tests__/tool-definitions.test.ts (the test that iterates over
toolDescriptions / the relevant it block) to assert that each tool description
string includes "Returns:" (similar to the existing checks for "Example:" and
"When to use:"), and ensure the check runs for all tools (including write tools
alongside the existing "Obsidian syntax:" assertion) so any tool missing
"Returns:" will fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f056d26c-5b03-4a0c-ade8-526eaec88f69
📒 Files selected for processing (2)
AGENTS.mdsrc/vault-mcp/tool-definitions.ts
✅ Files skipped from review due to trivial changes (1)
- src/vault-mcp/tool-definitions.ts
AGENTS.md mandates a Returns: section on every MCP tool description, but the suite only enforced Example: and When to use. Add the matching assertion so a future tool can't omit Returns. https://claude.ai/code/session_01KnvnMoDuU6QkYrCna54CqT
aliasunder
changed the title
aliasunder
changed the title
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
2 participants
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.
Summary
Glama scored all 23 tools (Tool Definition Quality grade: A). Six tools sat below the pack, dragged by the same dimensions — Parameters (descriptions restated the schema instead of explaining intent) and, on several, Behavior. This enriches those six descriptions with non-obvious parameter semantics, behavioral consequences, and tighter usage guidance; fixes one latent bug surfaced during the audit; and tightens the description-quality conventions + test coverage off the back of review.
Targeted tools and their weak dimensions:
vault_find_orphansvault_delete_notepath)vault_get_backlinksvault_get_outgoing_linksvault_search_by_foldervault_update_memoryNotable additions, all verified against the data layer (not invented):
find_orphans:exclude_foldersreplaces the defaults rather than merging (a real footgun), and matches by recursive folder prefix.delete_note: deletion is permanent on disk (no trash) with no server-side undo — recovery depends on the user's own backups or sync history (claim scoped to the server; not asserted as absolutely irreversible, since synced vaults retain version history). Inbound links break on delete; added anErrors:section.get_backlinks/get_outgoing_links:pathis exact + case-sensitive incl..md; a non-matching path returns an empty array, not an error (now stated underErrors:); self-links and code-fence exclusion documented.update_memory: additive (never overwrites; repeat calls duplicate); trimmed the triple-repeated "call list_memory_files first" that capped Conciseness.Bug fix (same audit)
vault_search_by_folderdocumented "sorted by most recently modified" but its SQL omittedORDER BY, returning results in arbitrary order. AddedORDER BY mtime DESCto match every sibling metadata tool, with a test asserting the ordering. (Declined CodeRabbit'spath ASCtie-breaker here —recentNotesandfindOrphansalso sort by baremtime DESC; a tie-breaker should be applied uniformly in a separate change, not piecemeal.)Review follow-ups (conventions + tests)
Errors:sections, where relevant. Added the empty-result-is-not-an-error contract tosearch_by_folder,get_backlinks,get_outgoing_links,find_orphans;delete_notecarries its real errors.update_memoryis intentionally left without one (auto-creates; no failure/no-match contract — a boilerplate "Errors: none" would just cost tokens).Errors:is required where a tool has failure modes or a no-match/empty-result contract worth clarifying, optional for tools that can't meaningfully fail.tool-definitions.test.tsnow asserts every tool description includes aReturns:section (alongside the existingExample:/When to usechecks) so a future tool can't omit it.Scope
Description text + one-line SQL sort fix + a doc-convention update + two tests. No tool behavior changes beyond the folder-search ordering.
Test plan
npm run build— cleannpm run lint— cleannpm test— 499 passing (incl.searchByFolderordering test +Returns:section test)https://claude.ai/code/session_01KnvnMoDuU6QkYrCna54CqT
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
[[Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/aliasunder/vault-cortex/pull/66?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)