Skip to content

zotero-skill - #57

Open
jasonzou wants to merge 2 commits into
cookjohn:mainfrom
jasonzou:main
Open

zotero-skill#57
jasonzou wants to merge 2 commits into
cookjohn:mainfrom
jasonzou:main

Conversation

@jasonzou

@jasonzou jasonzou commented Apr 7, 2026

Copy link
Copy Markdown

Thanks for the plugin and saved lots of time!!!!
I added a small enhancement:

  • can import pdf - via 23119 api
  • create an agent skill using the plugin
  • my openclaw runs in a docker; and zotero runs in the host machine.

@cookjohn

Copy link
Copy Markdown
Owner

Thanks a lot for this contribution — PDF import is one of the most-requested capabilities, and the agent skill + Python CLI are a nice demonstration of driving the plugin end-to-end (including from a Docker container). A few changes are needed before we can merge:

  1. Write gating (required): import_pdf modifies the library, but the new connector tools are added after the write-tool filter and marked "always available", and the call handler has no write.enabled check. All other write operations (write_note, create_collection, …) are gated both in tools/list and at call time. Please gate import_pdf the same way so users who disabled write operations aren't surprised.
  2. Dead code: handlePdfUpload() in streamableMCPServer.ts is defined but never routed or called. Please remove it (or wire it into the HTTP server if you intended an upload endpoint).
  3. Personal metadata: zotero-skill/_meta.json contains a marketplace ownerId/publishedAt — please drop this file from the PR.
  4. list_collections duplication: the MCP server already exposes get_collections (and collection write ops since v1.4.6). Could you either reuse it in the skill or explain why the Connector-API variant is needed? Two tools returning collections with different shapes will confuse clients.
  5. Placement: we'd prefer the skill live under examples/zotero-skill/ (with a README pointer) rather than at repo root — keeps the plugin source tree focused. Open to discussion.
  6. Heads-up — overlap with feat: write_item 新增 import action — 支持将本地文件导入为 Zotero 附件 #76: write_item just gained an import action on main (local filePath → attachment, ships next release). Your base64 import_pdf still adds value for remote/container clients that cannot write to the host filesystem, but please coordinate the docs/skill so the two paths are clearly distinguished.

Smaller notes: callZoteroImport hardcodes port 23119 while the other two tools accept a port arg; and base64-encoding PDFs through JSON-RPC inflates payloads ~33%, so a documented size limit or a clearer error for huge files would help. (We verified fetch/atob are available in Zotero's plugin sandbox, so the platform usage is fine — though Zotero.HTTP.request is the convention elsewhere in the codebase.)

Also, could you fix the PR title typo ("zotoer" → "zotero")? Happy to merge once the gating and cleanup items are addressed.

@jasonzou jasonzou changed the title zotoer-skill zotero-skill Aug 21, 2026
@cookjohn

cookjohn commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Hi @jasonzou — following up: the June review requested six changes (gating import_pdf behind write.enabled, removing the unused handlePdfUpload, dropping zotero-skill/_meta.json, resolving the list_collections duplication, moving the skill under examples/, and the title typo). The branch merged upstream main in August but none of the review items have corresponding changes yet.

Also worth noting: the landscape has moved since June — write_item now has a local-file import action (v1.5.0) and add_by_identifier landed in v1.6.0, so the skill's docs would want updating either way.

If you'd like to continue, please respond or update within two weeks (by 2026-09-17) — happy to help with any of the items. If we don't hear back we'll close this PR to keep the queue tidy; you can reopen anytime, and the work remains referenced. The agent-skill idea itself is still welcome, especially as an examples/ contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants