feat(vscode): discover workspace-local nodes from <workspace>/nodes/#1037
Open
dsapandora wants to merge 7 commits into
Open
feat(vscode): discover workspace-local nodes from <workspace>/nodes/#1037dsapandora wants to merge 7 commits into
dsapandora wants to merge 7 commits into
Conversation
dsapandora
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEngineLocal now enables development prototyping of local nodes by symlinking workspace-level node directories into the engine's catalog at startup, tracks created symlinks per instance, and removes them on stop/dispose; accompanying docs describe layout, constraints, and promotion workflow. ChangesLocal node symlink support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/vscode/src/engine/local/engine-local.ts`:
- Around line 518-555: The symlink ownership race happens because
setupLocalNodes() unlinks any symlink at targetDir and cleanupLocalNodes()
removes symlinks indiscriminately; change to an ownership scheme keyed by this
window/instance so you only touch symlinks you created: when creating a symlink
in setupLocalNodes() write a small owner marker (e.g., targetDir + '.owner'
containing a unique this.windowId) and only unlink an existing symlink if its
owner marker matches; likewise, when catching an existing symbolic link skip or
warn if owner differs; in cleanupLocalNodes() only unlink symlinks whose owner
marker matches this.windowId and remove their owner files, and ensure
linkedNodes only records targets you successfully owned. Include helper
functions like createOwnerMarker/readOwnerMarker/removeOwnerMarker and reference
setupLocalNodes(), cleanupLocalNodes(), linkedNodes, targetDir, sourceDir when
making changes.
- Around line 496-517: The code currently sets targetRoot = path.join(engineDir,
'nodes') which links local nodes into the wrong directory; change the target to
the engine catalog contract path (path.join(engineDir, 'nodes', 'src', 'nodes'))
so discovered local node dirs are placed where the engine expects them; update
the directory creation and subsequent references that use targetRoot (the block
that mkdirSync(targetRoot, { recursive: true }) and the loop that computes
targetDir) to use the new path, preserving existing error logging via
this.logger.output and the checks against localNodesDir and services.json.
In `@docs/README-nodes.md`:
- Around line 163-181: The two fenced code blocks showing the workspace tree and
the engine output log lack language tags and trigger MD040; update those
backticks to include the text language (change ``` to ```text) for the workspace
tree block (the my-workspace/ tree) and the engine output block (the "✅ Local
nodes registered (1): hello_world" log) so the markdown linter passes.
- Around line 183-189: The docs still tell users to use the VS Code "Restart
engine" command which was removed; update README-nodes.md to remove the stale
"Use the "Restart engine" command in VS Code." sentence and replace it with the
supported refresh flow: explain that after editing or adding a node (under .pipe
references to nodes/<node>/), users must restart the engine by stopping and then
starting it using the extension's new lifecycle commands (e.g., "Stop engine"
then "Start engine") or, if applicable, by reloading the window/extension host
as documented by the extension—ensure you reference the current commands that
actually exist in the extension instead of the removed "Restart engine" command.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c9913a0-3217-4ca1-8cf0-9e00df1d8f5e
📒 Files selected for processing (2)
apps/vscode/src/engine/local/engine-local.tsdocs/README-nodes.md
dsapandora
force-pushed
the
feat/local-nodes-discovery
branch
from
85dd9a8 to
a49e47c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/vscode/src/engine/local/engine-local.ts (2)
496-498:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLink local nodes into the actual engine catalog path.
The PR contract is
<engineDir>/nodes/src/nodes/<node>. CreatingtargetRootas<engineDir>/nodesputs the symlinks one level too high, so the engine will not discover them.Proposed fix
- const targetRoot = path.join(engineDir, 'nodes'); + const targetRoot = path.join(engineDir, 'nodes', 'src', 'nodes');Also applies to: 516-516
🤖 Prompt for 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. In `@apps/vscode/src/engine/local/engine-local.ts` around lines 496 - 498, The code currently sets targetRoot to path.join(engineDir, 'nodes') which places symlinks one level too high; change targetRoot to path.join(engineDir, 'nodes', 'src', 'nodes') so links land under the engine contract (<engineDir>/nodes/src/nodes/<node>); ensure fs.mkdirSync(targetRoot, { recursive: true }) is used and that this updated targetRoot is the path used later when creating the symlinks (referencing the targetRoot variable, engineDir, and the symlink creation logic around the existing code at lines ~496 and ~516).
518-555:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDo not let one VS Code window delete another window’s links.
This unlinks any existing symlink at
targetDirand later removes every tracked symlink with no ownership check. If two windows share the same installed engine directory, one window can replace or delete the other window’s local-node registration. It also means orphan cleanup from crashed runs cannot be done safely. This needs per-window isolation or an ownership marker before release.🤖 Prompt for 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. In `@apps/vscode/src/engine/local/engine-local.ts` around lines 518 - 555, When creating and removing local-node symlinks (in the setupLocalNodes symlink-creation block and cleanupLocalNodes method) add a per-window ownership marker and check it before unlinking: generate a unique owner id for this VS Code window (e.g., UUID or PID+UUID) at startup, write that id to a small marker file next to the symlink or inside the linked directory when you successfully create the symlink (update the symlink creation code path that pushes to this.linkedNodes to also record/set the marker), and when encountering an existing symlink (the fs.lstatSync branch) or when cleaning up in cleanupLocalNodes only unlink if the marker matches this window’s id; otherwise skip and leave the link alone. Ensure the linkedNodes tracking contains only symlinks you own so cleanupLocalNodes checks ownership (marker match) before fs.unlinkSync.
🤖 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 `@apps/vscode/src/engine/local/engine-local.ts`:
- Around line 164-166: setupLocalNodes mutates shared linkedNodes before
spawnProcess, causing leaked workspace-local links if spawnProcess fails or
start() is retried; change setupLocalNodes to return an undo/cleanup function
(or a list of created links) and wrap the call + await spawnProcess(...) in a
try/catch/finally block inside start(): call the undo function if spawnProcess
throws or if a later start is attempted to roll back any created links, and
ensure stop()/dispose() idempotently cleans linkedNodes so repeated starts don't
reuse stale links (refer to setupLocalNodes, spawnProcess, linkedNodes, start,
stop, dispose).
---
Duplicate comments:
In `@apps/vscode/src/engine/local/engine-local.ts`:
- Around line 496-498: The code currently sets targetRoot to
path.join(engineDir, 'nodes') which places symlinks one level too high; change
targetRoot to path.join(engineDir, 'nodes', 'src', 'nodes') so links land under
the engine contract (<engineDir>/nodes/src/nodes/<node>); ensure
fs.mkdirSync(targetRoot, { recursive: true }) is used and that this updated
targetRoot is the path used later when creating the symlinks (referencing the
targetRoot variable, engineDir, and the symlink creation logic around the
existing code at lines ~496 and ~516).
- Around line 518-555: When creating and removing local-node symlinks (in the
setupLocalNodes symlink-creation block and cleanupLocalNodes method) add a
per-window ownership marker and check it before unlinking: generate a unique
owner id for this VS Code window (e.g., UUID or PID+UUID) at startup, write that
id to a small marker file next to the symlink or inside the linked directory
when you successfully create the symlink (update the symlink creation code path
that pushes to this.linkedNodes to also record/set the marker), and when
encountering an existing symlink (the fs.lstatSync branch) or when cleaning up
in cleanupLocalNodes only unlink if the marker matches this window’s id;
otherwise skip and leave the link alone. Ensure the linkedNodes tracking
contains only symlinks you own so cleanupLocalNodes checks ownership (marker
match) before fs.unlinkSync.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9274a0a5-70cc-422b-b66e-d387e4acbf6f
📒 Files selected for processing (2)
apps/vscode/src/engine/local/engine-local.tsdocs/README-nodes.md
The extension now mirrors each <workspace>/nodes/<node>/ subdir into the engine's nodes/src/nodes/ via symlinks before spawning the engine binary, so contributors can prototype a new node next to their .pipe without touching the installed engine. Symlinks are cleaned up on engine stop; stale ones from crashed runs are refreshed on next start. Engine official nodes are unaffected setupLocalNodes() used to blindly unlink any existing symlink at the target before re-creating it, which could clobber a symlink another VS Code/Cursor window had installed for the same node name. Now inspect the existing link first: - target matches our sourceDir → reuse (no I/O, just track), - target gone (truly stale from a crashed run) → refresh, - target still points elsewhere → log and skip; never clobber. cleanupLocalNodes() already only touches symlinks tracked in linkedNodes, so this round-trip keeps other windows' claims intact. Also add `text` language tags to the two fenced code blocks in docs/README-nodes.md to satisfy MD040. Addresses CodeRabbit findings on PR #1037.
dsapandora
force-pushed
the
feat/local-nodes-discovery
branch
from
a49e47c to
afed25b
Compare
…fore setup If start() ran a second time on the same backend (retry after a failed start, or a manual recovery flow), setupLocalNodes() would accumulate entries in linkedNodes from the prior attempt — leaving symlinks for nodes removed from the workspace and potentially leaking links from runs that never made it past spawnProcess. Add a cleanup at the top of the start path and wrap the spawn in try/catch so a failed spawn restores the install dir to its prior state. Addresses CodeRabbit Major finding on PR #1037.
The section described how the extension wires symlinks under the hood and how to refresh the catalog. Implementation detail readers don't need; the "Reload Window after changes" guidance already lives in the Quick start.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/vscode/src/engine/local/engine-local.ts (1)
55-56: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDocumentation path mismatch with actual code.
The comment on line 55 and the docstring on line 488 both reference
nodes/src/nodes/, but the actual code at line 507 correctly uses the flat layoutpath.join(engineDir, 'nodes')per the packaged engine's structure.Suggested fix
- /** Local-node symlinks created under <engineDir>/nodes/src/nodes/ for this run; removed on stop. */ + /** Local-node symlinks created under <engineDir>/nodes/ for this run; removed on stop. */ private linkedNodes: string[] = [];And for the docstring:
/** * Symlinks each <workspaceRoot>/nodes/<node>/ subdir (must contain a - * services.json) into <engineDir>/nodes/src/nodes/<node>/. + * services.json) into <engineDir>/nodes/<node>/. *Also applies to: 487-488
🤖 Prompt for 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. In `@apps/vscode/src/engine/local/engine-local.ts` around lines 55 - 56, The inline comment for the linkedNodes field and the class/docstring references that mention the outdated nested path "nodes/src/nodes/" must be updated to match the actual packaged layout used by the code (path.join(engineDir, 'nodes')), so find the linkedNodes declaration (private linkedNodes: string[] = []) and any docstring paragraphs referencing "nodes/src/nodes/" and replace those descriptions with the flat "nodes" directory (e.g., "Local-node symlinks created under <engineDir>/nodes for this run; removed on stop"), ensuring any examples or explanatory text reference path.join(engineDir, 'nodes') or "<engineDir>/nodes" instead of the nested path.
🤖 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.
Outside diff comments:
In `@apps/vscode/src/engine/local/engine-local.ts`:
- Around line 55-56: The inline comment for the linkedNodes field and the
class/docstring references that mention the outdated nested path
"nodes/src/nodes/" must be updated to match the actual packaged layout used by
the code (path.join(engineDir, 'nodes')), so find the linkedNodes declaration
(private linkedNodes: string[] = []) and any docstring paragraphs referencing
"nodes/src/nodes/" and replace those descriptions with the flat "nodes"
directory (e.g., "Local-node symlinks created under <engineDir>/nodes for this
run; removed on stop"), ensuring any examples or explanatory text reference
path.join(engineDir, 'nodes') or "<engineDir>/nodes" instead of the nested path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b99a64a-fb58-4e39-af62-b651f9f2f21a
📒 Files selected for processing (1)
apps/vscode/src/engine/local/engine-local.ts
- Replace the redundant folder-tree example with a pointer to the canonical "Adding a New Node" section (steps 1-5 already cover services.json, Python interfaces, SVG, requirements.txt). - Drop the `### Path convention` subsection — saying it's "same as built-in nodes" already implies the same `"path"` convention. - Trim the Promoting tail; "ships as a unit" plus the earlier self-contained line is enough.
Same behavior, less code: - Drop the `reuseExisting` flag and the post-try `if (reuseExisting)` block — inline the reuse push + continue. - Invert the symlink check to early-skip the collision case, removing the else branch and one indent level. - Collapse the inner readlink try/catch (5 lines → 2). - Compact cleanupLocalNodes loop body. - Correct the JSDoc — the target path is `<engineDir>/nodes/<node>/`, not `<engineDir>/nodes/src/nodes/<node>/` (the latter is only the source repo layout, not the installed engine). No behavior change.
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
1 participant
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
The extension now mirrors each /nodes// subdir into the engine's nodes/src/nodes/ via symlinks before spawning the engine binary, so contributors can prototype a new node next to their .pipe without touching the installed engine. Symlinks are cleaned up on engine stop; stale ones from crashed runs are refreshed on next start.
I added the Restart optoo
Engine official nodes are unaffected
Type
feat
Testing
./builder testpassesChecklist
Linked Issue
Fixes #
Summary by CodeRabbit
New Features
Documentation