Conversation
dengwx2026
left a comment
There was a problem hiding this comment.
TypeScript SDK Review
Overall this is a solid foundation for the TS SDK — good module structure, nice mirroring of the Python SDK patterns, and sensible use of Zod for type definitions. However, there are several critical bugs that will cause runtime failures, plus some architectural concerns worth addressing before merging.
Summary
- 5 Critical issues — will cause runtime failures (stub
uploadDir, broken multipart uploads,__dirnamein ESM, heredoc expansion bug, malformed URL) - 8 Major issues — should be fixed (version mismatch, command injection, infinite loops, unused exception system, etc.)
- 9 Minor issues — code quality improvements (duplication, hardcoded values, dead code)
Architectural Suggestions
-
Use typed exceptions or remove them:
RockException,BadRequestRockError,raiseForCode()etc. are well-designed but never called anywhere. All errors are thrown as genericnew Error(...). Either integrateraiseForCode()into the HTTP layer, or remove the unused exception hierarchy. -
Validate with Zod or drop it: Zod schemas are defined for all request/response types but
.parse()/.safeParse()is never called on API data. They only serve asz.infer<>type extraction — plain TypeScript interfaces would eliminate the Zod dependency with the same result. -
Remove browser compatibility stubs: The SDK imports
fs,crypto,child_process,https,Buffer— it fundamentally cannot work in browsers. TheisBrowser()checks insystem.tsare misleading. -
Consolidate duplicated code:
RunModeType/RunModeare duplicated insandbox/types.tsandcommon/constants.ts.extractNohupPidis duplicated insandbox/utils.tsandutils/http.ts.sleepis duplicated inmodel/client.tsandutils/retry.ts. -
Add cancellation support:
ModelClientpolling loops (while(true)) needAbortControlleror timeout mechanisms to avoid blocking forever.
Minor Issues (not inlined)
utils/retry.ts:26— Defaultbackoff = 1.0means no actual exponential backoff. Should default to2.0.sandbox/client.ts:396—String(e).includes('already exists')for session detection is fragile and will break if the server message changes or is localized.logger.ts:69—loggerCacheMap grows without bound — potential memory leak in long-running processes.common/constants.ts:22-29—Constantsclass with all empty string static properties and@deprecatedtag is dead code.utils/deprecated.ts— Usesconsole.warninstead of the project's Winston logger, and fires on every call (standard practice is warn-once-per-function).
- RunModeType/RunMode: re-export from common/constants.ts in sandbox/types.ts - extractNohupPid: re-export from utils/http.ts in sandbox/utils.ts - sleep: import from utils/retry.ts in model/client.ts instead of private method Addresses code duplication feedback from PR alibaba#492 review.
…etry Default backoff=1.0 resulted in fixed delay intervals (no exponential backoff). Changed to backoff=2.0 so delays double on each retry, which is the standard exponential backoff behavior. Addresses PR alibaba#492 review feedback.
Replace hardcoded default values in sandbox config with environment variables, allowing users to customize defaults without code changes. New environment variables: - ROCK_DEFAULT_IMAGE, ROCK_DEFAULT_MEMORY, ROCK_DEFAULT_CPUS - ROCK_DEFAULT_CLUSTER, ROCK_DEFAULT_AUTO_CLEAR_SECONDS - ROCK_DEFAULT_GROUP_SIZE, ROCK_DEFAULT_START_CONCURRENCY - ROCK_DEFAULT_START_RETRY_TIMES - ROCK_DEFAULT_ARUN_TIMEOUT, ROCK_DEFAULT_NOHUP_WAIT_TIMEOUT - ROCK_DEFAULT_NOHUP_WAIT_INTERVAL, ROCK_DEFAULT_STATUS_CHECK_INTERVAL This addresses PR alibaba#492 review feedback about hardcoded defaults scattered in sandbox/client.ts and config.ts.
… in ModelClient - Add try/catch for JSON.parse in parseRequestLine and parseResponseLine to handle corrupted log files gracefully with meaningful error messages - Replace readFileSync/appendFileSync with async readFile/appendFile from fs/promises to avoid blocking the event loop - Re-throw parse errors immediately in popRequest instead of retrying - Add tests for JSON parse error handling and async file I/O usage Fixes PR alibaba#492 review feedback
- Add module-level sharedHttpsAgent with keepAlive enabled - Prevents creating new https.Agent on every HTTP request - Enables TLS session reuse and connection pooling - Follows Python SDK pattern (_SHARED_SSL_CONTEXT) Fixes PR alibaba#492 reviewer feedback about performance degradation under load.
- RunModeType/RunMode: re-export from common/constants.ts in sandbox/types.ts - extractNohupPid: re-export from utils/http.ts in sandbox/utils.ts - sleep: import from utils/retry.ts in model/client.ts instead of private method Addresses code duplication feedback from PR alibaba#492 review.
…etry Default backoff=1.0 resulted in fixed delay intervals (no exponential backoff). Changed to backoff=2.0 so delays double on each retry, which is the standard exponential backoff behavior. Addresses PR alibaba#492 review feedback.
Replace hardcoded default values in sandbox config with environment variables, allowing users to customize defaults without code changes. New environment variables: - ROCK_DEFAULT_IMAGE, ROCK_DEFAULT_MEMORY, ROCK_DEFAULT_CPUS - ROCK_DEFAULT_CLUSTER, ROCK_DEFAULT_AUTO_CLEAR_SECONDS - ROCK_DEFAULT_GROUP_SIZE, ROCK_DEFAULT_START_CONCURRENCY - ROCK_DEFAULT_START_RETRY_TIMES - ROCK_DEFAULT_ARUN_TIMEOUT, ROCK_DEFAULT_NOHUP_WAIT_TIMEOUT - ROCK_DEFAULT_NOHUP_WAIT_INTERVAL, ROCK_DEFAULT_STATUS_CHECK_INTERVAL This addresses PR alibaba#492 review feedback about hardcoded defaults scattered in sandbox/client.ts and config.ts.
… in ModelClient - Add try/catch for JSON.parse in parseRequestLine and parseResponseLine to handle corrupted log files gracefully with meaningful error messages - Replace readFileSync/appendFileSync with async readFile/appendFile from fs/promises to avoid blocking the event loop - Re-throw parse errors immediately in popRequest instead of retrying - Add tests for JSON parse error handling and async file I/O usage Fixes PR alibaba#492 review feedback
- Add module-level sharedHttpsAgent with keepAlive enabled - Prevents creating new https.Agent on every HTTP request - Enables TLS session reuse and connection pooling - Follows Python SDK pattern (_SHARED_SSL_CONTEXT) Fixes PR alibaba#492 reviewer feedback about performance degradation under load.
1fa3352 to
a95b3e8
Compare
- Add ts-case-convert for automatic case conversion - Implement HTTP layer auto-transform (request → snake_case, response → camelCase) - Refactor all response types to use camelCase - Update examples and documentation BREAKING CHANGE: All response fields now use camelCase instead of snake_case (e.g., sandbox_id → sandboxId, exit_code → exitCode)
- Fix getStatus URL to use sandbox_id instead of sandboxId - Fix logger getLogLevel to use lowercase for level matching
- Rename write_file() to writeFile() - Rename read_file() to readFile() - Add ESLint naming-convention rule to enforce camelCase - Update examples to use new method names
- Modify HttpUtils.get/post/postMultipart to return structured response
with {status, result, headers}
- Add HeaderFieldsSchema with cluster, requestId, eagleeyeTraceid
- Update all Response types to include header-derived fields
- Extract headers in all Sandbox methods and merge into results
- Fix HttpUtils generic type parameter usage in client methods - Fix EnvHubClient to use response.result from new HttpUtils return type
- Replace console.warn with Winston logger for consistency - Implement warn-once pattern using Set to track warned keys - Each deprecated function/class only warns once per session - Add clearDeprecatedWarnings() for testing isolation - Add comprehensive test suite (11 tests)
Replace hardcoded default values in sandbox config with environment variables, allowing users to customize defaults without code changes. New environment variables: - ROCK_DEFAULT_IMAGE, ROCK_DEFAULT_MEMORY, ROCK_DEFAULT_CPUS - ROCK_DEFAULT_CLUSTER, ROCK_DEFAULT_AUTO_CLEAR_SECONDS - ROCK_DEFAULT_GROUP_SIZE, ROCK_DEFAULT_START_CONCURRENCY - ROCK_DEFAULT_START_RETRY_TIMES - ROCK_DEFAULT_ARUN_TIMEOUT, ROCK_DEFAULT_NOHUP_WAIT_TIMEOUT - ROCK_DEFAULT_NOHUP_WAIT_INTERVAL, ROCK_DEFAULT_STATUS_CHECK_INTERVAL This addresses PR alibaba#492 review feedback about hardcoded defaults scattered in sandbox/client.ts and config.ts.
- Move SpeedupType to network.ts where it's used - Update all RunMode/RunModeType imports to use common/constants.ts - Remove unused RuntimeEnvId and AgentType from types.ts - Delete sandbox/types.ts and types.test.ts This addresses the PR review feedback about duplicated type definitions.
…s only - Remove isBrowser() function that implied browser compatibility - Simplify getEnv() and isEnvSet() to directly use process.env - SDK fundamentally cannot run in browsers due to Node.js module imports - Add tests to verify isBrowser is not exported
- Add static factory method RockEnv.create() for async initialization - Call 'make' API to get sandboxId during initialization - Replace raw AxiosInstance with HttpUtils for consistent camelCase/snake_case conversion - Update make() factory function to be async - Add comprehensive tests for RockEnv
… in ModelClient - Add try/catch for JSON.parse in parseRequestLine and parseResponseLine to handle corrupted log files gracefully with meaningful error messages - Replace readFileSync/appendFileSync with async readFile/appendFile from fs/promises to avoid blocking the event loop - Re-throw parse errors immediately in popRequest instead of retrying - Add tests for JSON parse error handling and async file I/O usage Fixes PR alibaba#492 review feedback
- Add module-level sharedHttpsAgent with keepAlive enabled - Prevents creating new https.Agent on every HTTP request - Enables TLS session reuse and connection pooling - Follows Python SDK pattern (_SHARED_SSL_CONTEXT) Fixes PR alibaba#492 reviewer feedback about performance degradation under load.
Replace sync fs.existsSync and fs.readFileSync with async fs/promises.access and fs/promises.readFile to avoid blocking the event loop when uploading large files.
Zod .default(envVars.XXX) captured values at module load time. If environment variables changed after module load, the schema still used stale values. Now using factory functions .default(() => envVars.XXX) to evaluate defaults at parse time, matching Python SDK's Field(default_factory=...) pattern.
Change the default pip index URL from China-specific mirror (mirrors.aliyun.com) to the public PyPI mirror (pypi.org) for better compatibility with international users of this open-source SDK.
Previously, HTTP errors were wrapped in generic Error objects, losing the response property. This prevented callers from detecting specific HTTP status codes (e.g., 401 Unauthorized, 403 Forbidden). Now re-throws the original AxiosError, allowing callers to access error.response.status for proper error handling, consistent with Python SDK behavior.
- Add downloadFile() method for downloading files from sandbox via OSS
- Add uploadMode parameter to uploadByPath() ('auto' | 'direct' | 'oss')
- Add getOssStsCredentials() and isTokenExpired() for OSS STS management
- Add new types: DownloadFileResponse, OssCredentials, UploadMode
- Add ENSURE_OSSUTIL_SCRIPT constant for ossutil installation
- Update documentation for v1.3.0 release
The ali-oss client defaults to HTTP protocol, but OSS buckets typically require HTTPS connections. This caused connection refused errors when uploading files via OSS. Added secure: true to OSS client initialization in setupOss() method.
The ali-oss signatureUrl method requires an options object with 'expires'
property, not a raw number. Passing a number caused TypeError when the
library tried to access options.method.toUpperCase().
Changed: signatureUrl(objectName, 600) -> signatureUrl(objectName, { expires: 600 })
Multi-line scripts passed to arun() with nohup mode caused errors because nohup cannot handle multi-line content directly. The fix wraps multi-line scripts with 'bash -c $'script\'' syntax. Before: nohup <multi-line-script> # fails with 'missing operand' After: nohup bash -c $'<multi-line-script>' # works correctly
ossutil v2 removed the -b flag and requires --region flag. Credentials should be passed via environment variables instead of command-line flags. Changes: - Use OSS_ACCESS_KEY_ID, OSS_ACCESS_KEY_SECRET, OSS_SESSION_TOKEN env vars - Add --region flag (required in v2) - Remove -b flag (deprecated in v2) Fixes errors: 'unknown shorthand flag: b' and 'region must be set in sign version 4'
ossutil v2 does not require separate config command. Credentials should be
passed directly as command-line parameters to ossutil cp, matching Python SDK.
Before: ossutil config + ossutil cp with env vars (incorrect)
After: ossutil cp with --access-key-id, --access-key-secret, --sts-token,
--endpoint, --region parameters (correct)
Fixes 'region must be set in sign version 4' error
… (v1.3.6) Fixed incompatibility between setupOss (ali-oss) and downloadFile (ossutil) by using ROCK_OSS_BUCKET_ENDPOINT env var and normalizing region format. Changes: - Use ROCK_OSS_BUCKET_ENDPOINT if available (format: oss-cn-hangzhou.aliyuncs.com) - Normalize region by removing 'oss-' prefix (oss-cn-hangzhou -> cn-hangzhou) - Matches Python SDK configuration format
When using arun() with nohup mode, the shell is /bin/sh which may not have /usr/local/bin in PATH. Since ossutil is typically installed in /usr/local/bin, this causes 'command not found' errors. Solution: wrap ossutil commands with 'bash -c' to ensure correct PATH. Matches Python SDK implementation using shlex.quote().
Add ROCK_OSS_TIMEOUT environment variable with default 300000ms (5 minutes). uploadByPath and downloadFile now accept optional timeout parameter. Priority: function parameter > environment variable > default Fixes ResponseTimeoutError for large file uploads/downloads.
Large file uploads were failing with 'socket connection was closed unexpectedly' error. Now using multipartUpload for files >= 1MB, matching Python SDK's oss2.resumable_upload behavior. - Files < 1MB: use put() - Files >= 1MB: use multipartUpload() with 1MB parts
Breaking change: uploadByPath and downloadFile now use options object pattern. - Added ProgressInfo, UploadPhase, DownloadPhase types - Added UploadOptions and DownloadOptions interfaces with onProgress callback - OSS operations (multipartUpload, get) report progress percentage (0-100) - Sandbox operations (wget, ossutil) report -1 (progress unavailable) - Updated all tests to use new options object API signature
a95b3e8 to
7b3b333
Compare
关联Issue #495 。
包地址:https://www.npmjs.com/package/rl-rock
支持:
🚀 沙箱管理 - 创建、启动、停止远程容器沙箱
📁 文件系统 - 上传、下载、读取、写入文件
🖥️ 命令执行 - 同步/异步执行 Shell 命令
🔧 运行时环境 - 支持 Python、Node.js 运行时环境管理
🤖 Agent 框架 - 内置 Agent 支持自动化任务
📦 EnvHub - 环境注册与管理
🔄 双模式构建 - 同时支持 ESM 和 CommonJS