feat: add referenceURL lookup and Vitest integration test suite#28
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the DEVONthink integration by enabling more flexible record identification through Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new lookup method using referenceURL and significantly improves the project's quality by adding a comprehensive Vitest integration test suite. While the new referenceURL feature is well-implemented with proper escaping for string inputs, a critical JXA injection vulnerability was identified in the existing id lookup logic. The id parameter, expected to be a number, is interpolated directly into the JXA script without validation, allowing for arbitrary code execution if a malicious string is provided. This requires immediate remediation by ensuring runtime validation of the id parameter. Furthermore, I've identified two potential issues in the getRecordByIdentifier implementation: a possible bug related to UUID case-sensitivity and a significant performance concern with the fallback lookup mechanism.
54b5e64 to
cb8097b
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces referenceURL lookup capabilities and refactors the testing approach into a robust Vitest integration suite. While the new test suite is well-structured and get_record_by_identifier follows JXA best practices, a critical security vulnerability exists: the existing id lookup logic remains vulnerable to JXA injection. The id parameter is directly interpolated without validation, potentially leading to Remote Code Execution (RCE) via prompt injection. It is strongly recommended to use the formatValueForJXA helper for all interpolated values. Furthermore, the referenceURL lookup implementation may not work as intended for non-UUID URLs and lacks sufficient test coverage.
5af997e to
0d413a9
Compare
|
@gemini-code-assist review All feedback addressed:
|
There was a problem hiding this comment.
Code Review
This pull request introduces two major improvements: a new Vitest integration test suite and the implementation of referenceURL lookup in get_record_by_identifier. A security audit focused on the new referenceURL lookup logic, specifically addressing potential JXA (JavaScript for Automation) Injection due to user-supplied inputs being injected into scripts executed via osascript. However, the implementation uses robust escaping and validation, including isJXASafeString, escapeStringForJXA, formatValueForJXA, and secure execution with execFile. No vulnerabilities meeting the criteria for reporting (medium severity or higher) were identified. There is one minor suggestion to improve the efficiency of the fallback lookup logic.
…t integration suite Add referenceURL parameter to get_record_by_identifier for x-devonthink-item:// URL lookup, supporting both UUID and non-UUID formats (e.g. imported emails with message-ID-based reference URLs). Uses three-tier lookup: UUID fast path, lookupRecordsWithURL scan, and brute-force referenceURL comparison fallback. Replace monolithic regression-test.ts with a proper Vitest integration test suite split by domain (connectivity, CRUD, identification, organization, transformation, network, AI). Tests use globalSetup/globalTeardown with an isolated temp database, ensuring no impact on production data. Covers 28 tests across all 27 MCP tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0d413a9 to
7e31473
Compare
Email records have referenceURLs with percent-encoded message-IDs (e.g. %3C, %40, %3E) that aren't UUIDs. lookupRecordsWithURL searches the "url" property, not "referenceURL", so these never matched. The url case now detects x-devonthink-item:// URLs, decodes the identifier, and uses getRecordWithUuid — matching the pattern in getRecordByIdentifier.ts. Regular URLs still use lookupRecordsWithURL with decoding applied. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the PR! Why did you close and delete this @darylrobbins ? |
Claude got a bit overzealous when I asked it to merge the changes into my fork. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces two major improvements: it adds support for referenceURL lookup in the get_record_by_identifier tool and completely refactors the testing infrastructure to a modern Vitest-based integration suite. The implementation for referenceURL lookup is robust, handling both standard UUID-based and non-UUID (e.g., email message-ID) x-devonthink-item:// URLs. The accompanying fix in lookup_record to correctly handle these URLs is also a valuable addition. The new integration test suite is a significant enhancement, providing isolated, comprehensive, and reliable testing for all tools against a live DEVONthink instance. The code quality is high, adhering to the project's JXA scripting best practices, and the documentation has been updated accordingly. Overall, this is an excellent contribution that improves both functionality and maintainability. I have not found any issues that meet the required severity threshold for comments.
|
@dvcrn This one should be ready to go. |
|
Thanks, looks good to me! Could you run the formatter, then I'll merge this |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@dvcrn Formatted |
Summary
referenceURLparameter toget_record_by_identifierforx-devonthink-item://URL lookup, supporting both UUID and non-UUID formats (e.g. imported emails with message-ID-based reference URLs). Uses two-tier lookup: UUID fast path →lookupRecordsWithURLscan across databases.regression-test.tswith a proper Vitest integration test suite split by domain (connectivity, CRUD, identification, organization, transformation, network, AI). Tests useglobalSetup/globalTeardownwith an isolated temp database, ensuring no impact on production data. Covers 28 tests across all 27 MCP tools.Test plan
npm test— unit tests passnpm run test:integration— 28 integration tests pass (1 skipped:selected_recordsrequires UI)get_record_by_identifierwithreferenceURLfor UUID-format URLsget_record_by_identifierwithreferenceURLfor email message-ID URLs🤖 Generated with Claude Code