-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(sandbox): migrate to object-based API parameters with backwa… #6
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
Conversation
…rd compatibility
This commit refactors the entire sandbox API to use object-based parameter passing
instead of positional arguments. All methods now accept parameters as objects
with { input, config } pattern while maintaining backward compatibility through
deprecation warnings for old signatures.
The changes affect:
- Client methods (createTemplate, deleteTemplate, etc.)
- Sandbox creation and management methods
- Template operations
- All sandbox subclasses (AIO, Browser, CodeInterpreter, Custom)
- Test suites updated for new parameter patterns
Additionally, numeric field normalization is added to templates to ensure
consistent data types across string and number representations.
BREAKING CHANGE: Old parameter patterns are deprecated and will be removed in future versions.
此提交重构了整个沙箱 API 以使用基于对象的参数传递,
而不是位置参数。所有方法现在都接受 { input, config } 模式的对象参数,
同时通过弃用警告保留旧签名的向后兼容性。
更改影响:
- 客户端方法(createTemplate、deleteTemplate 等)
- 沙箱创建和管理方法
- 模板操作
- 所有沙箱子类(AIO、Browser、CodeInterpreter、Custom)
- 更新测试套件以适应新的参数模式
此外,向模板添加数字字段规范化,以确保字符串和数字表示形式的数据类型一致。
重大变更:旧参数模式已被弃用,并将在未来版本中移除。
Change-Id: I73e043e97ae2c8363a0f1172eed14da27a777e7c
Signed-off-by: OhYee <oyohyee@oyohyee.com>
Remove unused path import and unnecessary Sandbox type import from the sandbox example file to clean up code and reduce dependencies. 移除未使用的导入以清理代码示例并减少依赖项。 Change-Id: If120626c1e173339e8a0e438eb9b9eba4a82c205 Signed-off-by: OhYee <oyohyee@oyohyee.com>
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.
Pull request overview
This pull request refactors the entire sandbox SDK API from positional parameters to object-based parameter passing using a { input, config } pattern. The refactoring maintains backward compatibility through TypeScript overloads and runtime deprecation warnings for the old API signatures.
Changes:
- All client methods (Template and Sandbox operations) now accept object parameters with
{ input, config }or{ id/name, config }patterns - Added runtime parameter detection logic to support both new and legacy API signatures
- All sandbox subclasses (AIO, Browser, CodeInterpreter, Custom) updated to use the new
Sandbox.createsignature withtemplateTypeparameter - Added numeric field normalization to Template class to handle string-to-number conversions
- Added
JAVASCRIPTtoCodeLanguageenum - Comprehensive test updates to match new API patterns and add legacy compatibility tests
- Example code updated to demonstrate new API usage
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sandbox/client.ts | Refactored all client methods to support both object-based and legacy positional parameters with deprecation warnings |
| src/sandbox/sandbox.ts | Updated static methods (create, delete, stop, get, list) to use object parameters with overloads for backward compatibility |
| src/sandbox/template.ts | Updated static methods to use object parameters and added normalizeNumericFields method |
| src/sandbox/aio-sandbox.ts | Updated createFromTemplate to use new Sandbox.create API with templateType |
| src/sandbox/browser-sandbox.ts | Updated createFromTemplate to use new Sandbox.create API with templateType |
| src/sandbox/code-interpreter-sandbox.ts | Updated createFromTemplate to use new Sandbox.create API with templateType |
| src/sandbox/custom-sandbox.ts | Updated createFromTemplate to use new Sandbox.create API with templateType |
| src/sandbox/model.ts | Added JAVASCRIPT enum value to CodeLanguage |
| tests/unittests/sandbox/client.test.ts | Added comprehensive legacy compatibility tests and updated mock setup |
| tests/unittests/sandbox/sandbox.test.ts | Updated test expectations to match new parameter passing behavior |
| tests/unittests/sandbox/custom-sandbox.test.ts | Updated tests to match new Sandbox.create signature and added mock setup to avoid circular dependencies |
| tests/unittests/sandbox/template.test.ts | Added tests for numeric field normalization functionality |
| examples/sandbox.ts | Updated example code to use new object-based API patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… API parameters This commit refactors the SandboxClient and Sandbox classes to use object-based API parameters instead of the legacy string-based parameters. The changes include updating method signatures, parameter handling, and maintaining backward compatibility for deprecated APIs. Additionally, the template tests have been updated to use direct SandboxControlAPI mocking instead of client mocking. The parameter migration affects listTemplates, deleteSandbox, stopSandbox, and listSandboxes methods to accept object parameters for better type safety and flexibility while maintaining support for legacy string parameter usage. refactor(sandbox): 将客户端和沙箱类迁移到基于对象的 API 参数 此提交重构了 SandboxClient 和 Sandbox 类,以使用基于对象的 API 参数, 而不是传统的基于字符串的参数。更改包括更新方法签名、参数处理, 并维护对已弃用 API 的向后兼容性。此外,模板测试已更新为直接使用 SandboxControlAPI 模拟,而不是客户端模拟。 参数迁移影响了 listTemplates、deleteSandbox、stopSandbox 和 listSandboxes 方法,使其接受对象参数以实现更好的类型安全性和灵活性, 同时保持对传统字符串参数用法的支持。 Change-Id: I9d1b8be721a41b38851d1280727b892590d085c5 Signed-off-by: OhYee <oyohyee@oyohyee.com>
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
tests/unittests/sandbox/sandbox.test.ts:230
- The Sandbox class methods (create, delete, stop, get, list) have deprecated legacy API signatures, but there are no test cases in sandbox.test.ts to verify that these legacy signatures still work correctly and emit deprecation warnings. This is a gap in test coverage compared to client.test.ts which has comprehensive legacy compatibility tests.
Consider adding test cases for the legacy API patterns to ensure backward compatibility is maintained.
describe('delete (static)', () => {
it('should delete sandbox successfully', async () => {
const mockSandbox = new Sandbox({
sandboxId: 'sandbox-123',
state: SandboxState.DELETING,
});
mockClientDeleteSandbox.mockResolvedValue(mockSandbox);
const result = await Sandbox.delete({ id: 'sandbox-123' });
expect(result.sandboxId).toBe('sandbox-123');
expect(mockClientDeleteSandbox).toHaveBeenCalledWith({
id: 'sandbox-123',
});
});
it('should throw ClientError when deletion fails', async () => {
mockClientDeleteSandbox.mockRejectedValue(
new ClientError(404, 'Sandbox not found')
);
await expect(Sandbox.delete({ id: 'non-existent' })).rejects.toThrow(
ClientError
);
});
});
describe('stop (static)', () => {
it('should stop sandbox successfully', async () => {
const mockSandbox = new Sandbox({
sandboxId: 'sandbox-123',
state: SandboxState.STOPPED,
});
mockClientStopSandbox.mockResolvedValue(mockSandbox);
const result = await Sandbox.stop({ id: 'sandbox-123' });
expect(result.sandboxId).toBe('sandbox-123');
expect(mockClientStopSandbox).toHaveBeenCalledWith({
id: 'sandbox-123',
});
});
it('should throw ClientError when stop fails', async () => {
mockClientStopSandbox.mockRejectedValue(
new ClientError(404, 'Sandbox not found')
);
await expect(Sandbox.stop({ id: 'non-existent' })).rejects.toThrow(
ClientError
);
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…h backward compatibility
This commit refactors the sandbox client and template classes to use object-based API parameters instead of positional arguments. The changes maintain backward compatibility by supporting both the new object-based API and legacy parameter formats, while adding deprecation warnings for the old usage patterns.
The client methods now accept parameters as objects ({ input, config }) rather than positional arguments (input, config), improving type safety and parameter clarity. The template class numeric field normalization was also improved to properly handle invalid numeric values by setting them to undefined.
Additionally, the test suite was updated to mock the SandboxClient directly instead of the lower-level control API, providing better test isolation and more accurate testing of the actual API usage patterns.
refactor(sandbox): 将API方法迁移到基于对象的参数并保持向后兼容性
此提交重构了sandbox客户端和模板类,使用基于对象的API参数而不是位置参数。这些更改通过支持新的基于对象的API和旧的参数格式来保持向后兼容性,同时为旧的使用模式添加弃用警告。
客户端方法现在接受参数作为对象({ input, config})而不是位置参数(input, config),提高了类型安全性和参数清晰度。模板类数值字段规范化也得到了改进,通过将无效的数值设置为undefined来正确处理它们。
此外,测试套件已更新为直接模拟SandboxClient而不是底层控制API,提供更好的测试隔离和更准确的实际API使用模式测试。
Change-Id: I8fd2bad5dac5c898328fe64d6d7f40b82ee00fe4
Signed-off-by: OhYee <oyohyee@oyohyee.com>
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should set invalid numeric strings to undefined', () => { | ||
| const template = new Template({ | ||
| templateId: 'template-456', | ||
| templateName: 'test-template-invalid', | ||
| cpu: 'not-a-number' as unknown as number, | ||
| }); | ||
|
|
||
| (template as unknown as { normalizeNumericFields: () => void }).normalizeNumericFields(); | ||
|
|
||
| expect(template.cpu).toBeUndefined(); | ||
| }); |
Copilot
AI
Jan 29, 2026
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.
The test manually calls the private method normalizeNumericFields() through type casting. While this works, it would be better to test this behavior through the public API by checking that numeric fields are properly normalized when creating a Template instance with string values, without needing to expose or manually call the private method.
| */ | ||
| export enum CodeLanguage { | ||
| PYTHON = "python", | ||
| JAVASCRIPT = "javascript", |
Copilot
AI
Jan 29, 2026
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.
Adding a new enum value JAVASCRIPT to CodeLanguage is a good addition. Ensure that all code that handles CodeLanguage enum values (such as switch statements or validation logic) properly handles this new value.
| networkConfiguration: | ||
| finalInput.networkConfiguration ? | ||
| new $AgentRun.NetworkConfiguration({ |
Copilot
AI
Jan 29, 2026
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.
The ternary operator formatting has been changed to use a newline after the question mark. While this is syntactically valid, it's unusual in JavaScript/TypeScript. The more conventional formatting would be to either keep it on one line or place the question mark at the end of the condition line. Consider using a more standard formatting pattern for better readability and consistency with common style guides.
| networkConfiguration: | |
| finalInput.networkConfiguration ? | |
| new $AgentRun.NetworkConfiguration({ | |
| networkConfiguration: finalInput.networkConfiguration | |
| ? new $AgentRun.NetworkConfiguration({ |
| input.networkConfiguration ? | ||
| new $AgentRun.NetworkConfiguration({ | ||
| ...input.networkConfiguration, | ||
| }) | ||
| : undefined, |
Copilot
AI
Jan 29, 2026
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.
The ternary operator formatting has been changed to use a newline after the question mark. While this is syntactically valid, it's unusual in JavaScript/TypeScript. The more conventional formatting would be to either keep it on one line or place the question mark at the end of the condition line. Consider using a more standard formatting pattern for better readability and consistency with common style guides.
| input.networkConfiguration ? | |
| new $AgentRun.NetworkConfiguration({ | |
| ...input.networkConfiguration, | |
| }) | |
| : undefined, | |
| input.networkConfiguration | |
| ? new $AgentRun.NetworkConfiguration({ | |
| ...input.networkConfiguration, | |
| }) | |
| : undefined, |
| if (templateType === TemplateType.CODE_INTERPRETER) | ||
| return new CodeInterpreterSandbox(sb, cfg); | ||
| else if (templateType === TemplateType.BROWSER) | ||
| return new BrowserSandbox(sb, cfg); | ||
| else if (templateType === TemplateType.AIO) | ||
| return new AioSandbox(sb, cfg); | ||
| else if (templateType === TemplateType.CUSTOM) | ||
| return new CustomSandbox(sb, cfg); |
Copilot
AI
Jan 29, 2026
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.
The if-else-if chain for returning sandbox instances lacks braces around the return statements. While this is syntactically valid, it's less readable and more error-prone for future modifications. Consider adding braces for consistency and better maintainability.
| if (templateType === TemplateType.CODE_INTERPRETER) | |
| return new CodeInterpreterSandbox(sb, cfg); | |
| else if (templateType === TemplateType.BROWSER) | |
| return new BrowserSandbox(sb, cfg); | |
| else if (templateType === TemplateType.AIO) | |
| return new AioSandbox(sb, cfg); | |
| else if (templateType === TemplateType.CUSTOM) | |
| return new CustomSandbox(sb, cfg); | |
| if (templateType === TemplateType.CODE_INTERPRETER) { | |
| return new CodeInterpreterSandbox(sb, cfg); | |
| } else if (templateType === TemplateType.BROWSER) { | |
| return new BrowserSandbox(sb, cfg); | |
| } else if (templateType === TemplateType.AIO) { | |
| return new AioSandbox(sb, cfg); | |
| } else if (templateType === TemplateType.CUSTOM) { | |
| return new CustomSandbox(sb, cfg); | |
| } |
| it('should normalize numeric fields from strings', () => { | ||
| const template = new Template({ | ||
| templateId: 'template-123', | ||
| templateName: 'test-template', | ||
| sandboxIdleTimeoutInSeconds: '600' as unknown as number, | ||
| sandboxTtlInSeconds: '3600' as unknown as number, | ||
| shareConcurrencyLimitPerSandbox: '3' as unknown as number, | ||
| cpu: '2' as unknown as number, | ||
| memory: 4096, | ||
| diskSize: '512' as unknown as number, | ||
| }); | ||
|
|
||
| (template as unknown as { normalizeNumericFields: () => void }).normalizeNumericFields(); | ||
|
|
||
| expect(template.sandboxIdleTimeoutInSeconds).toBe(600); | ||
| expect(template.sandboxTtlInSeconds).toBe(3600); | ||
| expect(template.shareConcurrencyLimitPerSandbox).toBe(3); | ||
| expect(template.cpu).toBe(2); | ||
| expect(template.memory).toBe(4096); | ||
| expect(template.diskSize).toBe(512); | ||
| }); |
Copilot
AI
Jan 29, 2026
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.
The test manually calls the private method normalizeNumericFields() through type casting. While this works, it would be better to test this behavior through the public API by checking that numeric fields are properly normalized when creating a Template instance with string values, without needing to expose or manually call the private method.
…ove deprecated test update test file to fix formatting issues, reorder imports, add proper comma separators in function calls, and remove a deprecated test case that is no longer needed 修复格式问题,重新排序导入,添加适当的逗号分隔符,并删除不再需要的已弃用测试用例 Change-Id: I8ed28372c047f15e0a49d882e47d0a8911a542a1 Signed-off-by: OhYee <oyohyee@oyohyee.com>
…rd compatibility
This commit refactors the entire sandbox API to use object-based parameter passing instead of positional arguments. All methods now accept parameters as objects with { input, config } pattern while maintaining backward compatibility through deprecation warnings for old signatures.
The changes affect:
Additionally, numeric field normalization is added to templates to ensure consistent data types across string and number representations.
BREAKING CHANGE: Old parameter patterns are deprecated and will be removed in future versions.
此提交重构了整个沙箱 API 以使用基于对象的参数传递,
而不是位置参数。所有方法现在都接受 { input, config } 模式的对象参数,
同时通过弃用警告保留旧签名的向后兼容性。
更改影响:
此外,向模板添加数字字段规范化,以确保字符串和数字表示形式的数据类型一致。
重大变更:旧参数模式已被弃用,并将在未来版本中移除。
Change-Id: I73e043e97ae2c8363a0f1172eed14da27a777e7c
Fix bugs
Bug detail
Pull request tasks
Update docs
Reason for update
Pull request tasks
Add contributor
Contributed content
Content detail
Others
Reason for update