-
-
Notifications
You must be signed in to change notification settings - Fork 373
Add node-transfer skill: High-speed file transfer with native streaming #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@EisonMe is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 3 comments
| "send.js": null, | ||
| "receive.js": null, | ||
| "ensure-installed.js": null | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integrity check always fails
version.js ships with files hashes set to null, but ensure-installed.js treats any mismatch as “needs deploy” (hashFile(...) !== expectedHash). With expectedHash === null, every non-null actual hash will fail, so nodes will never be considered up-to-date (and will constantly request redeploy). Do you intend to compute and commit the 12-char SHA256 prefixes in version.js at release time, or should ensure-installed.js treat null hashes as “skip hash validation”?
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/node-transfer/version.js
Line: 14:17
Comment:
**Integrity check always fails**
`version.js` ships with `files` hashes set to `null`, but `ensure-installed.js` treats any mismatch as “needs deploy” (`hashFile(...) !== expectedHash`). With `expectedHash === null`, every non-null actual hash will fail, so nodes will *never* be considered up-to-date (and will constantly request redeploy). Do you intend to compute and commit the 12-char SHA256 prefixes in `version.js` at release time, or should `ensure-installed.js` treat `null` hashes as “skip hash validation”?
How can I resolve this? If you propose a fix, please make it concise.| psScript += `\n\n`; | ||
| psScript += `# Deploy ${file}\n`; | ||
| psScript += `$b64 = "${encoded}"\n`; | ||
| psScript += `[System.IO.File]::WriteAllBytes("${targetPath}", [System.Convert]::FromBase64String($b64))`; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PowerShell script may be invalid
Each deployed file is embedded as $b64 = "<...>" inside the generated script (deploy.js:105-107). Base64 output includes / and + (fine), but can also be extremely long; more importantly, the script never resets/clears $b64 safely between files and doesn’t guard against PowerShell command-length/quoting limits when executed via nodes.invoke(['powershell','-Command', ...]). In practice this can truncate the script and produce corrupted files (exactly the “encoding issues / failed deployments” this skill is trying to avoid).
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/node-transfer/deploy.js
Line: 103:107
Comment:
**PowerShell script may be invalid**
Each deployed file is embedded as `$b64 = "<...>"` inside the generated script (`deploy.js:105-107`). Base64 output includes `/` and `+` (fine), but can also be extremely long; more importantly, the script never resets/clears `$b64` safely between files and doesn’t guard against PowerShell command-length/quoting limits when executed via `nodes.invoke(['powershell','-Command', ...])`. In practice this can truncate the script and produce corrupted files (exactly the “encoding issues / failed deployments” this skill is trying to avoid).
How can I resolve this? If you propose a fix, please make it concise.| // Build full URL with token | ||
| const fullUrl = `${args.url}?token=${encodeURIComponent(args.token)}`; | ||
|
|
||
| // Choose http or https module | ||
| const client = parsedUrl.protocol === 'https:' ? https : http; | ||
|
|
||
| // Create write stream for output file | ||
| const writeStream = fs.createWriteStream(args.outputPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token URL concatenation
The code appends the token via raw string concatenation (receive.js:193-200). If the provided URL already has a query component, the resulting URL becomes invalid and the sender won’t see the token, causing an auth failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/node-transfer/receive.js
Line: 193:200
Comment:
**Token URL concatenation**
The code appends the token via raw string concatenation (receive.js:193-200). If the provided URL already has a query component, the resulting URL becomes invalid and the sender won’t see the token, causing an auth failure.
How can I resolve this? If you propose a fix, please make it concise.
🐛 Critical Bug Fix: Replace Base64 File Transfer with Native Streaming
Problem Summary
The current
nodes.invokemechanism for file transfer has critical limitations that cause production issues:Performance Comparison
Root Cause
The current implementation:
nodes.invokeThis causes OOM errors for files larger than available RAM and creates unacceptable delays.
Solution: node-transfer
A native Node.js stream-based file transfer using HTTP streaming:
Architecture
Security Model
Proposed Integration
Add a
nodes.transfercommand to the corenodestool:Files Included
This contribution includes:
send.jsreceive.jsensure-installed.jsversion.jsdeploy.jsSKILL.mdpackage.jsonREADME.mdTesting Results
Validated on Windows nodes with various file sizes:
Migration Path
Phase 1: Submit as skill (this PR)
Phase 2: Add
nodes.transfer()to SDK with auto-deploymentPhase 3: Native protocol implementation
Checklist
SKILL.md)Related: This fixes critical issues with the current Base64-over-JSON approach and enables reliable large file transfers between OpenClaw nodes.
Greptile Overview
Greptile Summary
This PR adds a new
skills/node-transferskill implementing node-to-node file transfer using native Node.js streaming over HTTP. It includes sender/receiver scripts (send.js,receive.js), a deployment script generator (deploy.js) intended to install the scripts onto nodes once, and anensure-installed.jschecker that is supposed to detect whether the installed scripts are up-to-date based on aversion.jsmanifest.Key functional risk areas are the install/up-to-date detection and the deployment mechanism: as currently written, the version manifest ships with
nullhashes, which makes the integrity check always report “needs deploy”, and the generated PowerShell deployment script can be too large/fragile when executed vianodes.invoke(which undermines the “install once” workflow). There is also a concrete URL construction bug inreceive.jswhen the sender URL already contains query parameters.Confidence Score: 2/5
version.jsprovidesnullexpected hashes soensure-installed.jswill always mark installations as mismatched, andreceive.jshas a deterministic URL concatenation bug for URLs with existing query strings. Additionally, the deployment mechanism relies on very large inline base64 strings in a PowerShell-Commandcontext, which is prone to truncation/corruption in real usage.(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
Context used:
dashboard- AGENTS.md (source)