Repository navigation
test: prune and rewrite SDK tests flagged by the current-main audit rerun - #2142
devin-ai-integration[bot] wants to merge 5 commits into
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…ling Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…lic seam Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…public seams Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Package ArtifactsBuilt from 3d0716a. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.1.3-devin-1794604210-prune-sdk-tests-rerun.0.tgz ./e2b-2.64.1-devin-1794604210-prune-sdk-tests-rerun.0.tgzCLI ( npm install ./e2b-cli-2.24.1-devin-1794604210-prune-sdk-tests-rerun.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.9.2-devin-1794604210-prune-sdk-tests-rerun.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.5.1-devin-1794604210-prune-sdk-tests-rerun.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.1.2+devin.1794604210.prune.sdk.tests.rerun-py3-none-any.whl ./e2b-2.64.0+devin.1794604210.prune.sdk.tests.rerun-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.11.10+devin.1794604210.prune.sdk.tests.rerun-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.7.2+devin.1794604210.prune.sdk.tests.rerun-py3-none-any.whl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| test('should hash file content spanning multiple read chunks', async () => { | ||
| const content = randomBytes(3 * 1024 * 1024 + 123) | ||
| const filePath = join(testDir, 'big.bin') | ||
| const hash = () => | ||
| calculateFilesHash('big.bin', '/app', testDir, [], false, undefined) | ||
|
|
||
| await writeFile(filePath, content) | ||
| const stats = statSync(filePath) | ||
|
|
||
| const expected = createHash('sha256') | ||
| .update('COPY big.bin /app') | ||
| .update('big.bin') | ||
| .update(stats.mode.toString()) | ||
| .update(stats.size.toString()) | ||
| .update(content) | ||
| .digest('hex') | ||
|
|
||
| expect( | ||
| await calculateFilesHash('big.bin', '/app', testDir, [], false, undefined) | ||
| ).toBe(expected) | ||
| }) | ||
| const original = await hash() | ||
|
|
||
| test('should not read whole files into memory', async () => { | ||
| await writeFile(join(testDir, 'big.bin'), randomBytes(2 * 1024 * 1024)) | ||
| const readFileSync = vi.spyOn(fs, 'readFileSync') | ||
| // Flip one byte in the final, partial chunk; same size and mode. | ||
| content[content.length - 1] ^= 0xff | ||
| await writeFile(filePath, content) | ||
| const changed = await hash() | ||
|
|
||
| await calculateFilesHash('big.bin', '/app', testDir, [], false, undefined) | ||
| content[content.length - 1] ^= 0xff | ||
| await writeFile(filePath, content) | ||
|
|
||
| expect(readFileSync).not.toHaveBeenCalled() | ||
| expect(changed).not.toBe(original) | ||
| expect(await hash()).toBe(original) |
There was a problem hiding this comment.
🟡 (optional) After this change, no test in either SDK pins the template files-hash value. A recipe change that silently invalidates every user's build cache would now pass CI. The new multi-chunk test only checks that flipping the last byte changes the hash. It still passes if earlier chunks are skipped or if hashFileContent hashes the whole buffer instead of subarray(0, bytesRead). Fix: give both SDKs deterministic content spanning more than one 1 MiB chunk and assert one hard-coded hex digest, shared by JS and Python. The Python twin is at test_get_all_files_in_path.py:576-599.
Why this was flagged
The removed tests recomputed the sha256 recipe (COPY line, relative path, mode, size, content) in both getAllFilesInPath.test.ts and test_get_all_files_in_path.py. They pinned the cache key and kept the two SDKs computing the same digest. The replacement test (getAllFilesInPath.test.ts:584-602, test_get_all_files_in_path.py:576-599) uses random content and only compares hashes against each other. Three regressions now pass in both languages: a loop that hashes only the final chunk, a change to field order or encoding, and in JS hash.update(buffer) without subarray(0, bytesRead) at src/template/utils.ts:297. That last one leaks stale bytes from the reused Buffer.allocUnsafe(HASH_CHUNK_SIZE), so small-file hashes become nondeterministic. Grep finds no 64-hex golden digest under either SDK's tests/template, so nothing else catches these. The base branch failed on all of them. A literal golden digest comes from an independent source, so it does not have the tautology problem that the PR set out to remove.
Verification: The diff deletes the only assertions in either SDK that compared the files hash against an independently computed sha256 of the full content. The new test (getAllFilesInPath.test.ts:584-602; test_get_all_files_in_path.py:576-599) uses random content. It flips only content[content.length - 1]. No hex digest or sha256 recomputation remains anywhere.
| def test_pty_stream_input_sends_pty_data_under_the_stream_timeout( | ||
| fake_envd_sandbox: Sandbox, | ||
| ): | ||
| fake_envd_sandbox.pty.stream_input(42, ["a", b"b"], timeout=5) | ||
|
|
||
| [(path, headers, messages)] = _FakeEnvdHandler.requests | ||
| assert path == "/process.Process/StreamInput" | ||
| assert headers["connect-timeout-ms"] == "5000" | ||
| assert messages[0] == {"start": {"process": {"pid": 42}}} | ||
| data = [ | ||
| base64.b64decode(message["data"]["input"]["pty"]) for message in messages[1:] | ||
| ] | ||
| assert data == [b"a", b"b"] |
There was a problem hiding this comment.
🟡 (optional) Deleting test_stream_input_sync.py leaves no test showing that stream_input rejects a single bytearray chunk, and no offline test of single-chunk rejection at all. The deleted test_rejects_single_chunk was parametrized over "data", b"data" and bytearray(b"data"). Only live-sandbox tests remain, covering bytes (commands) and str (pty). Fix: add an offline single-chunk test on fake_envd_sandbox for commands and pty. It should cover str, bytes and bytearray (and memoryview, which the guard also lists) and assert that no request reaches the fake envd.
Why this was flagged
The guard is at e2b/sandbox/commands/stream_input.py:52: isinstance(data, (str, bytes, bytearray, memoryview)). It raises InvalidArgumentException so that a lone chunk is not streamed byte by byte as ints. Before this PR, tests/test_stream_input_sync.py ran it offline for "data", b"data" and bytearray(b"data"). This PR deletes that file. The only remaining tests are tests/sync/sandbox_sync/commands/test_stream_input.py:37 (b"ls\n") and tests/sync/sandbox_sync/pty/test_stream_input.py:22 ("ls\n"), and both need a live sandbox fixture. A grep for bytearray in the Python tests finds no other stream_input use. The new pty test at lines 145-157 only adds data and deadline coverage. So dropping bytearray from the guard would now pass CI, and an offline run checks no single-chunk path at all. Neither was true on the base branch.
Verification: The PR deletes packages/python-sdk/tests/test_stream_input_sync.py. The guard is at packages/python-sdk/e2b/sandbox/commands/stream_input.py:52. Only two single-chunk tests remain, and both need a live sandbox. No test in packages/python-sdk/tests passes a bytearray (or memoryview) as one chunk, so a future edit that drops bytearray from the guard tuple goes uncaught.
Summary
Fourth PR in the SDK test cleanup stack (#2048 ← #2054 ← #2052 ← this). It applies the high- and medium-confidence findings from rerunning the TDD-skill audit against current
main. Low-confidence findings are not included (they stay in SDK-414). Test-only, so there's no changeset.JS type assertions now actually run.
expectTypeOfincommands/run.test.ts/connect.test.tswas never checked: tsconfig only includessrc, and vitest typecheck mode was off. A deliberately wrong assertion still passed. Those assertions now live intests/sandbox/commands/retainOutput.test-d.ts, checked by a newtypesvitest project (typecheck.only,tsconfig.typecheck.json=src+tests/**/*.test-d.ts). Bun, deno and browser select projects explicitly, so they don't run it. I checked that a wrong assertion now fails withTypeCheckError.Deleted (duplicate, tests its own fake, or can't fail):
paginator throws once exhausted, which asserted an error thrown by the test's ownFakePaginator. Alsorun retains output by default, which passedretainOutput: trueexplicitly.run with non-array shell. ASetgoes through the same!shellOpt[0]branch as[], so the test still passes withArray.isArrayremoved.send stdin via command handle, a strict subset ofclose stdin via command handle. Alsopause sandbox, a strict prefix ofresume sandbox.snapshot preserves file system statetests, which repeatcreate sandbox from snapshot.calculateFilesHash … should not read whole files into memory, which only pinsfs.readFileSync/open().read(size). In JS it still passed when the code read the whole file.test_throws_for_none_values, which makes the same call asnormalize_build_arguments(), andtest_handles_name_without_tag, which goes through the same branch as the string-name test.test_get_timeout(sync/async), which only does anisinstanceon a typed dataclass field, andtest_volume_token_defaults_to_none, which only checks a constructor default.test_chunk_check_records_the_validation_error(internalChunkCheck.erroridentity).test_sync_stream_close_reaches_transport_generator, which still passed with_close_stream(inner)removed, because CPython refcounting closes the generator anyway.test_stream_input_sync.py, which only called the internalstream_input_synchelper. The public-seam fake-envd test now also coverspty.stream_inputdata and theconnect-timeout-msdeadline.Merged: the
{}rows of the CPU-option tests (JSboundOpts, Pythontest_bound_api_params) duplicated the min-free-disk{}row. The JSapiDefaultsmaxCpuCounttests duplicated the publicTemplate.buildInBackgroundtests. Both become a singlecpuCount-only row.Rewritten at a public seam:
calculateFilesHashmulti-chunk test (JS/Py): it used to rebuild the hash with the implementation's own recipe. Now it flips a byte in the last partial chunk and expects the hash to change, then flip back and the hash to be restored.test_commands_connect_applies_request_timeoutnow uses the realcreate_rpc_clientagainst an unresponsive HTTP/2 frame server instead of mocking the factory.test_connect_sends_stable_host_routing_headers_to_envd(sync/async) checks the headers envd actually receives onfiles.read, instead of mocking_cls_connect/_integrationand readingsandbox_headers.envdUnresponsiveAtPauseis now checked throughSandbox.get_info/Sandbox.list(sync/async) against a local API, instead of the privateSandboxInfo._from_*converters.on_resumeinstance form callssandbox.connect(300, "reboot")and expects aTypeError, instead of inspecting the signature.Link to Devin session: https://app.devin.ai/sessions/98def51a61574e17908881f74f2f7ded
Open in Devin Desktop: https://app.devin.ai/desktop/session/98def51a61574e17908881f74f2f7ded?variant=devin
Requested by: @mishushakov
🟢 Devin Review found nothing to fix · View findings