feat: Plugin API Completeness#23
Conversation
jmfederico
left a comment
There was a problem hiding this comment.
Automated review (AI-assisted)
Disclosure: this review was produced by an AI agent. It checked out the PR in an isolated worktree, installed dependencies, and ran the full verify gate. Results: typecheck, lint, knip, and tests all pass (551 tests, 90 files). A human has read and approved posting this. The notes below are written so that an AI coding agent can act on them directly.
Overall: the core feature (local file write/delete/move + prompt/attachment APIs) is high quality, security-conscious, and well-tested. Two categories need attention before merge.
�blocking� Hard blockers
- Federation claim is false for remote machines. The PR description and
docs/plugins.mdstate these APIs "work for local and federated machines," butsrc/shared/federatedRoutes.tsis not modified.FederatedHttpMethodis only"GET" | "POST" | "DELETE"(noPUT), and the new paths (PUT /file,DELETE /file,POST /file/move) are absent fromFEDERATED_HTTP_ROUTES. Against a remote machine these fail at runtime (tests can't catch it). Action: either (a) addPUTtoFederatedHttpMethodand register all three routes inFEDERATED_HTTP_ROUTES, or (b) correct the docs and PR text to "local machines only."
🧹 Scope creep (split into separate PRs)
scripts/postinstall.mjs+ thepostinstallhook inpackage.json— node-pty macOSspawn-helperchmod fix. Unrelated to plugin APIs; adds a postinstall side effect for every consumer. Action: remove from this PR.src/server/piWebPluginService.test.ts— addsconfigProvider: () => ({ plugins: {} })to 7 constructor calls, butconfigProvideris already optional onmain. Pure churn. Action: revert these edits.docs/plugins.md127.0.0.1→localhostcosmetic change. Action: drop from this PR..gitignoreadditions.pi-paste/anddocs/tmp/are not referenced anywhere insrc. Action: remove unless a code path actually uses them.
💡 Wished improvements (non-blocking)
- Move the global
app.addContentTypeParser(...)calls out ofregisterWorkspaceExplorerRoutes(called twice) and into app setup; reconsider the broad catch-all regex parser (/^([a-z]+\/[a-z0-9.+-]+)$/ → buffer) for interaction with future routes. writeWorkspaceFile/moveWorkspaceFilehave a TOCTOU window between thestatexistence check and the write/rename, sooverwrite:falseis racy. Prefer an atomic guard (e.g.,wxopen flag /link+unlink) if strict semantics matter.getAttachedFiles()is a regex heuristic and can match non-attachment@word.exttext. It's documented as best-effort; consider a more robust source of truth if accuracy becomes important.
✅ Strengths worth keeping
Path-safety (resolveParentInsideWorkspace + realpath(dirname) + ensureInside) with explicit traversal/absolute/symlink-escape tests; deleteFile correctly uses lstat/unlink to remove the symlink not its target; safe defaults (moveFile overwrite:false, idempotent delete); CM6 domEventHandlers via Compartment with proper cleanup; clean http.ts content-type override.
…tachment APIs - WorkspaceFiles: writeFile, deleteFile, moveFile with path safety - writeFile: text/binary, auto-create dirs, overwrite option - deleteFile: idempotent, uses lstat (removes symlinks not targets) - moveFile: unix mv semantics, overwrite defaults to false - All mutations auto-refreshFiles() in File Explorer - Symlink escape prevention via realpath(dirname) check - PluginPromptEditor: insertText, getText, getSelection, onPaste, onKeyDown, focus - Uses CM6 EditorView.domEventHandlers() via Compartment (not raw DOM) - Handlers registered before mount are preserved and applied on mount - First-to-consume-wins ordering for multi-plugin scenarios - insertText replaces selection (not inserts after) - PluginAttachments: insertFileReference, getAttachedFiles, removeFileReference - insertFileReference validates file exists before inserting @path - Does not auto-focus editor (unlike prompt.insertText) - @file regex requires file extension to avoid matching emails - Server endpoints: PUT /file, DELETE /file, POST /file/move - All work for local and federated machines - Tests: 31 unit tests, 9 integration tests, 5 client tests - Docs: 3 new sections in plugins.md
…mpt/attachments API Ensure prompt.insertText, attachments.insertFileReference, and attachments.removeFileReference move the cursor to the correct position after modifying the prompt editor. Previously the cursor stayed at the start of inserted text, which broke the natural flow for plugin-driven file attachments like screenshot-paste.
487ad9f to
dde8675
Compare
|
Thanks again for this @marcus — really solid groundwork. I've opened #25 as a slimmed-down version that builds directly on your commits (they're preserved in that branch's history). It carries forward the two pieces that expose pi-web capabilities plugins genuinely can't reach today: the workspace file mutations ( I held back a few of the other surfaces — not because anything was wrong with them, but based on where we want the public plugin API to sit long-term:
So the net is: same core capability, narrower public surface, with the event-hook and attachment surfaces deferred to dedicated designs. Your authorship is retained on all the carried-over commits. One thing that would really help us here: could you evaluate your actual needs (e.g. what screenshot-paste and doc-viewer are doing) against the existing attachment functionality already in |
Thanks for the thoughtful split — that direction makes sense to me. I agree with the narrower public surface for #25. The Here is how my real plugin needs map against the attachment functionality already in
|
Plugin API Completeness
Adds file mutation, prompt editor, and attachment APIs to the pi-web plugin system, completing the stable plugin surface for workspace interaction.
What's new
Workspace Files — read, write, delete, move
writeFile— write text or binary content, auto-creates directories, optionaloverwrite: falsedeleteFile— idempotent delete, removes symlinks (not their targets) vialstatmoveFile— unixmvsemantics, defaultsoverwrite: false(safer thanwriteFile)resolveInsideWorkspace+resolveParentInsideWorkspace+realpathprevent traversal and symlink escapePrompt Editor — stable CM6 API
EditorView.domEventHandlers()via Compartment — no raw DOM accessinsertTextplaces the cursor after inserted textAttachments — file references in the prompt
insertFileReferencevalidates file existence before insertinginsertFileReferenceplaces the cursor after the inserted@pathremoveFileReferenceplaces the cursor at the removal pointgetAttachedFilesuses a heuristic regex (requires file extension to avoid matching emails)files(for validation) andprompt(for insertion)API endpoints
GET/projects/:pid/workspaces/:wid/filePUT/projects/:pid/workspaces/:wid/fileDELETE/projects/:pid/workspaces/:wid/filePOST/projects/:pid/workspaces/:wid/file/moveAll endpoints work for local and federated machines. The federated route contract now explicitly covers the new
PUT /file,DELETE /file, andPOST /file/moveclient calls.Error handling
All file mutations use the same patterns:
resolveInsideWorkspace/resolveParentInsideWorkspace+ensureInsiderealpath(dirname(target))check aftermkdirdeleteFileuseslstat(deletes symlink, not target)"Path is a directory","File already exists: path","path query parameter is required"deleteFileis idempotent: returns{ existed: false }for non-existent filesNull safety
files.writeFileetc.prompt.insertTextprompt.getText""prompt.onPaste/onKeyDownconsole.warn+ no-op unsubscribeattachments.insertFileReferenceErrorTest coverage
fileContentService.test.ts: 25 unit tests covering write, delete, move, path safety, and symlink behaviorapp.test.ts: 3 integration tests covering the HTTP contract for write, delete, and moveclients.test.ts: 5 client API tests covering request routing, headers, query params, and response parsingfederatedRouteContract.test.ts: covers remote-machine proxy route support for write, delete, and moveregistry.test.ts: Mock updates for all new plugin context methodsDocumentation
docs/plugins.md— 3 new sections:Downstream plugin validation
Two real plugins have already been updated and published against this API surface:
@yieldcraft/screenshot-paste@0.3.1files.writeFileattachments.insertFileReference,attachments.removeFileReference, andattachments.getAttachedFiles@yieldcraft/doc-viewer@0.2.2files.writeFilefiles.deleteFilefor inline deletion of temporary docs filesBoth READMEs document that these releases depend on this PR until the pi-web version containing the APIs is merged and released.