Init fixes#3
Merged
steven-passynkov merged 9 commits intomainfrom Apr 3, 2026
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughRefactors client/service wiring to use transport-backed, Zod-validated request/response shapes (snake_case ↔ camelCase normalization), reworks many service APIs to object-style params and async flows, adds OTEL lifecycle controls, updates models/schemas, examples, tests, and adds a release CI workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Leap0Client
participant Service as ServiceClient
participant Transport as Leap0Transport
participant API as Server/API
Client->>Service: call public method (object-style params)
Service->>Transport: build URL, snake_case payload, options
Transport->>API: HTTP request (JSON or bytes)
API-->>Transport: HTTP response (snake_case JSON / 204 / bytes / SSE)
Transport->>Service: raw response
Service->>Service: normalize(schema, camelizeKeys(response))
Service-->>Client: typed camelCased result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
tests/core/normalize.test.ts (1)
6-25: Consider adding edge-case assertions for non-plain objects.
GivencamelizeKeysexplicitly preservesDateandUint8Array, adding a small assertion for those cases would strengthen this test’s contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/normalize.test.ts` around lines 6 - 25, Add assertions to the "camelizeKeys converts nested API response keys" test to cover non-plain objects: pass a Date instance and a Uint8Array into the input object given to camelizeKeys and assert the returned values preserve the Date (same time/value or instanceof Date) and preserve the Uint8Array contents/type, ensuring camelizeKeys does not convert or clone these special types; update the test block around the existing camelizeKeys invocation and result assertion to include these additional checks.tests/services/ssh.test.ts (1)
28-32: Add an explicitcalls.lengthassertion to tighten this test.Without a total-call check, unexpected additional SSH requests can slip by while current assertions still pass.
Proposed test hardening
await client.regenerateAccess("sb-1"); await client.deleteAccess("sb-1"); + assert.equal(calls.length, 4); assert.equal(calls[0]?.path, "/v1/sandbox/sb-1/ssh/access"); assert.deepEqual(jsonOf(calls[1]!), { id: "ssh-1", password: "secret" }); assert.equal(calls[2]?.path, "/v1/sandbox/sb-1/ssh/regen"); assert.equal(calls[3]?.path, "/v1/sandbox/sb-1/ssh/access");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/ssh.test.ts` around lines 28 - 32, The test inspects the mocked request array "calls" but doesn't assert its total length, so add an explicit assertion like assert.equal(calls.length, 4) (or the expected count) before the existing per-index checks to ensure no extra SSH requests were made; locate the "calls" variable in tests/services/ssh.test.ts and insert the length assertion prior to the existing assert.equal(calls[0]...), assert.deepEqual(calls[1]!), assert.equal(calls[2]...), and assert.equal(calls[3]...) lines.tests/services/process.test.ts (1)
16-17: Assert total call count for stricter request-shape coverage.This test currently validates only the first call payload/path, so unintended extra requests would still pass.
Proposed test hardening
await client.execute("sb-1", { command: "npm test", cwd: "/workspace", timeout: 30 }); + assert.equal(calls.length, 1); assert.equal(calls[0]?.path, "/v1/sandbox/sb-1/process/execute"); assert.deepEqual(jsonOf(calls[0]!), { command: "npm test", cwd: "/workspace", timeout: 30 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/process.test.ts` around lines 16 - 17, The test only checks calls[0] payload and path but doesn't fail if extra requests were made; add a strict assertion on the total number of requests by checking the length of the calls array (the calls variable used alongside the existing assert.equal(...) and assert.deepEqual(...)) to ensure exactly the expected number of requests were issued (e.g., assert that calls.length === 1).src/config/index.ts (1)
48-54: Consider simplifying the nested ternary for readability.The
sdkOtelEnabledresolution logic is correct but the nested ternary can be hard to parse at a glance.♻️ Optional refactor to improve readability
- const sdkOtelEnabled = - input.sdkOtelEnabled ?? - (envOtel === "true" - ? true - : envOtel === "false" - ? false - : Boolean(readEnv("OTEL_EXPORTER_OTLP_ENDPOINT"))); + const sdkOtelEnabled = + input.sdkOtelEnabled ?? resolveSdkOtelFromEnv(envOtel);Add a helper function:
function resolveSdkOtelFromEnv(envOtel: string | undefined): boolean { if (envOtel === "true") return true; if (envOtel === "false") return false; return Boolean(readEnv("OTEL_EXPORTER_OTLP_ENDPOINT")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.ts` around lines 48 - 54, The nested ternary used to compute sdkOtelEnabled is hard to read; extract the resolution into a small helper (e.g., resolveSdkOtelFromEnv(envOtel)) that returns true for "true", false for "false", and otherwise returns Boolean(readEnv("OTEL_EXPORTER_OTLP_ENDPOINT")); then set const sdkOtelEnabled = input.sdkOtelEnabled ?? resolveSdkOtelFromEnv(envOtel) so callers (and future maintainers) see the logic clearly; reference variables/functions: sdkOtelEnabled, input.sdkOtelEnabled, envOtel, readEnv, and the new resolveSdkOtelFromEnv.tests/services/git.test.ts (1)
34-47: Index-based assertions are fragile.The test relies on specific call indices (e.g.,
calls[5],calls[8],calls[13]). If the method call order changes, these assertions will silently verify the wrong payloads. Consider adding inline comments mapping indices to operations, or asserting on path alongside payload:// Example: verify diff call (index 5) const diffCall = calls[5]!; assert.equal(diffCall.path, "/v1/sandbox/sb-1/git/diff"); assert.deepEqual(jsonOf(diffCall), { path: "/workspace/repo", target: "main" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/git.test.ts` around lines 34 - 47, The assertions in tests/services/git.test.ts use hard-coded indices on the mock request array (calls[5], calls[8], calls[12], calls[13]) which is brittle; update each index-based check to locate the correct call by its request path (e.g., find the call whose path equals "/v1/sandbox/sb-1/git/diff", "/v1/sandbox/sb-1/git/checkout", "/v1/sandbox/sb-1/git/ls-files", "/v1/sandbox/sb-1/git/commit") and then assert both the call.path and jsonOf(call) payloads (use the existing jsonOf helper and calls array) so the test verifies the intended operation regardless of call order.tests/services/filesystem.test.ts (1)
43-43: Add an explicit assertion forsetPermissions.This test exercises the new object-form call, but it never verifies the serialized request for it. A regression in the
{ mode, owner, group }mapping would still pass here.Also applies to: 53-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/filesystem.test.ts` at line 43, The test calls client.setPermissions("sb-1", "/tmp/a.txt", { mode: "0755" }) but never asserts the serialized request body; update the test (and the analogous cases at the 53-72 range) to inspect the mock/spy that captured the outgoing request from setPermissions and assert the payload includes the expected object mapping (e.g., mode: "0755" and any owner/group fields when provided) so the serialized request matches the object-form API (targeting the client.setPermissions call and the transport/mock that records requests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 6-7: The release workflow currently triggers on the "v*" tag
pattern and runs npm publish without a dist tag, which causes prerelease tags
(e.g., v1.0.0-beta.1) to be published as latest; update the release job in
release.yml so that the npm publish step adds an explicit --tag for prereleases
(e.g., --tag next or a tag derived from the prerelease identifier) and only
omits --tag for normal releases; detect prerelease by checking the git tag
string for a hyphen/prerelease identifier (the workflow's tag ref matching "v*"
and the npm publish command) and pass the appropriate --tag flag accordingly.
In `@examples/quickstart.ts`:
- Around line 4-15: The client.close() call may be skipped if sandboxes.create()
throws because the create is outside the try; wrap the create and usage in a
try/finally (or add a top-level try) so that client.close() always runs: call
const client = new Leap0Client(); then enter try { const sandbox = await
client.sandboxes.create(); ... } finally { if (sandbox) await sandbox.delete();
await client.close(); } — reference Leap0Client, sandboxes.create,
sandbox.delete, and client.close when making the change and guard
sandbox.delete() for the case where creation failed.
In `@src/client/sandbox.ts`:
- Around line 117-120: The refresh method currently unsafely casts the result of
this.client.sandboxes.get(this.id) to SandboxData; instead, update refresh to
handle both raw SandboxData and wrapped Sandbox instances returned by a factory:
call this.client.sandboxes.get(this.id) into a local variable, detect if the
result is a Sandbox wrapper (e.g., instanceof Sandbox or has a .data/.toJSON()
accessor) and extract the underlying SandboxData accordingly, then call
this.update(...) with that data; alternatively, adjust the SandboxesClient
generic/return type so get(...) is typed as SandboxData to avoid the cast.
Ensure you reference the refresh method, this.client.sandboxes.get, SandboxData
and Sandbox when making the change.
In `@src/core/otel.ts`:
- Around line 60-64: shutdownOtel currently awaits
tracerProviderInstance?.shutdown() and meterProviderInstance?.shutdown() before
clearing the module-level references, which can leave stale references if
shutdown rejects or initOtel runs concurrently; update shutdownOtel so it first
nulls/undefineds the provider refs (tracerProviderInstance and
meterProviderInstance) immediately, then call and await the shutdown() promises
(e.g., capture the promises before clearing or call shutdown after clearing),
and ensure any shutdown rejections are caught/logged so they don't leave the
module in an inconsistent state; keep references to the provider instances or
their shutdown promises using the existing symbol names and ensure initOtel()
checks these refs as before.
In `@src/services/desktop.ts`:
- Around line 405-420: In waitUntilReady, handle events that omit status (per
desktopProcessStatusListSchema) by treating count-only updates: when an event
from statusStream contains running and total and running >= total, return as
ready (in addition to the existing check for status === "running"); also ensure
that when the async iterator completes cleanly (stream closed without ready),
you back off before retrying the outer loop (e.g., await a short delay) to avoid
hammering /api/status/stream; keep existing error handling (lastError and
Leap0Error logic) intact.
In `@src/services/filesystem.ts`:
- Around line 292-305: The editFiles method currently forces params.replace to
"" which loses the distinction between "omit/delete" and "replace with empty
string"; update the request body construction in editFiles (the jsonBody passed
to transport.requestJson for fsPath(..., "edit-files")) to omit the replace
property when params.replace is undefined/null rather than defaulting to an
empty string—i.e., only include replace in the payload if params.replace is
provided so the server can see the intended omitted vs empty-string semantics.
In `@src/services/lsp.ts`:
- Around line 14-25: The json<T> helper currently uses a non-null assertion on
the result of this.transport.requestJson (in json<T>, referencing
transport.requestJson and sandboxIdOf(sandbox)), which will throw an opaque
TypeError if the endpoint returns 204/empty; change it to capture the result
into a variable, explicitly check for undefined/null, and throw a clear,
descriptive error (e.g., "Empty response from /v1/sandbox/.../lsp/{endpoint}")
or return a sensible default before returning the value so callers get a
meaningful message instead of a runtime TypeError.
In `@src/services/templates.ts`:
- Around line 19-41: The create method validates params with
createTemplateParamsSchema.parse but then sends the original params to jsonBody;
change it to use the parsed/coerced result instead so any stripped keys,
coercions or defaults from the schema are preserved on the wire. Specifically,
assign the result of createTemplateParamsSchema.parse(params) to a variable
(e.g., parsed) and pass jsonBody(parsed) into
this.transport.requestJson("/v1/template", { method: "POST", body:
jsonBody(parsed) }, options) and keep normalize(templateDataSchema, data) as-is.
In `@tests/client/client-sandbox.test.ts`:
- Around line 8-10: The test "Leap0Client wires services and supports direct
access" mutates process.env.LEAP0_API_KEY and doesn't restore it; capture the
original value (e.g., const original = process.env.LEAP0_API_KEY) before
assigning, and restore it after the test (either in a finally block inside the
test or via an afterEach that resets process.env.LEAP0_API_KEY = original or
deletes it if original was undefined); apply the same pattern to the other
affected test(s) around lines 36-40 so process-wide env state is always
restored.
In `@tests/quality/docstrings.test.ts`:
- Around line 54-68: The variable relative produced by rel(file) is OS-sensitive
(backslashes on Windows) so strictFiles.has(relative) can fail; normalize
relative before matching by converting it to a consistent POSIX-style path (or
use path.normalize/path.posix) where rel(file) is used or immediately after
(before the strictFiles.has check) so strictFiles membership correctly matches
across platforms; update the code around rel(file), strictFiles.has(relative),
and any other checks that rely on relative path equality (e.g., in the loop that
uses jsDocText and node.name) to use the normalized path.
In `@tests/services/sandboxes-client.test.ts`:
- Around line 68-103: The tests in sandboxes-client.test.ts mutate process.env
keys (OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_HEADERS) and do not
restore them, causing test pollution; update the test file to save the original
values before each test (or in a beforeEach), then restore them after each test
(or in an afterEach) so that tests using makeClient() and client.create() run
with isolated environment variables and do not leak OTEL_* changes to other
tests.
---
Nitpick comments:
In `@src/config/index.ts`:
- Around line 48-54: The nested ternary used to compute sdkOtelEnabled is hard
to read; extract the resolution into a small helper (e.g.,
resolveSdkOtelFromEnv(envOtel)) that returns true for "true", false for "false",
and otherwise returns Boolean(readEnv("OTEL_EXPORTER_OTLP_ENDPOINT")); then set
const sdkOtelEnabled = input.sdkOtelEnabled ?? resolveSdkOtelFromEnv(envOtel) so
callers (and future maintainers) see the logic clearly; reference
variables/functions: sdkOtelEnabled, input.sdkOtelEnabled, envOtel, readEnv, and
the new resolveSdkOtelFromEnv.
In `@tests/core/normalize.test.ts`:
- Around line 6-25: Add assertions to the "camelizeKeys converts nested API
response keys" test to cover non-plain objects: pass a Date instance and a
Uint8Array into the input object given to camelizeKeys and assert the returned
values preserve the Date (same time/value or instanceof Date) and preserve the
Uint8Array contents/type, ensuring camelizeKeys does not convert or clone these
special types; update the test block around the existing camelizeKeys invocation
and result assertion to include these additional checks.
In `@tests/services/filesystem.test.ts`:
- Line 43: The test calls client.setPermissions("sb-1", "/tmp/a.txt", { mode:
"0755" }) but never asserts the serialized request body; update the test (and
the analogous cases at the 53-72 range) to inspect the mock/spy that captured
the outgoing request from setPermissions and assert the payload includes the
expected object mapping (e.g., mode: "0755" and any owner/group fields when
provided) so the serialized request matches the object-form API (targeting the
client.setPermissions call and the transport/mock that records requests).
In `@tests/services/git.test.ts`:
- Around line 34-47: The assertions in tests/services/git.test.ts use hard-coded
indices on the mock request array (calls[5], calls[8], calls[12], calls[13])
which is brittle; update each index-based check to locate the correct call by
its request path (e.g., find the call whose path equals
"/v1/sandbox/sb-1/git/diff", "/v1/sandbox/sb-1/git/checkout",
"/v1/sandbox/sb-1/git/ls-files", "/v1/sandbox/sb-1/git/commit") and then assert
both the call.path and jsonOf(call) payloads (use the existing jsonOf helper and
calls array) so the test verifies the intended operation regardless of call
order.
In `@tests/services/process.test.ts`:
- Around line 16-17: The test only checks calls[0] payload and path but doesn't
fail if extra requests were made; add a strict assertion on the total number of
requests by checking the length of the calls array (the calls variable used
alongside the existing assert.equal(...) and assert.deepEqual(...)) to ensure
exactly the expected number of requests were issued (e.g., assert that
calls.length === 1).
In `@tests/services/ssh.test.ts`:
- Around line 28-32: The test inspects the mocked request array "calls" but
doesn't assert its total length, so add an explicit assertion like
assert.equal(calls.length, 4) (or the expected count) before the existing
per-index checks to ensure no extra SSH requests were made; locate the "calls"
variable in tests/services/ssh.test.ts and insert the length assertion prior to
the existing assert.equal(calls[0]...), assert.deepEqual(calls[1]!),
assert.equal(calls[2]...), and assert.equal(calls[3]...) lines.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59e1e12b-479e-40a7-b936-465056b52ff7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (71)
.github/workflows/release.ymlREADME.mdexamples/code_interpreter_stream.tsexamples/desktop.tsexamples/filesystem_and_git.tsexamples/pty.tsexamples/quickstart.tsexamples/snapshots.tsexamples/ssh.tspackage.jsonsrc/client/index.tssrc/client/sandbox.tssrc/config/constants.tssrc/config/index.tssrc/core/errors.tssrc/core/normalize.tssrc/core/otel.tssrc/core/transport.tssrc/core/utils.tssrc/core/version.tssrc/index.tssrc/models/code-interpreter.tssrc/models/config.tssrc/models/desktop.tssrc/models/filesystem.tssrc/models/git.tssrc/models/index.tssrc/models/lsp.tssrc/models/process.tssrc/models/pty.tssrc/models/refs.tssrc/models/sandbox.tssrc/models/shared.tssrc/models/snapshot.tssrc/models/ssh.tssrc/models/template.tssrc/services/code-interpreter.tssrc/services/desktop.tssrc/services/filesystem.tssrc/services/git.tssrc/services/index.tssrc/services/lsp.tssrc/services/process.tssrc/services/pty.tssrc/services/sandboxes.tssrc/services/shared.tssrc/services/snapshots.tssrc/services/ssh.tssrc/services/templates.tstests/client/client-sandbox.test.tstests/client/index-exports.test.tstests/config/config-utils.test.tstests/config/otel.test.tstests/core/normalize.test.tstests/models/network-policy.test.tstests/models/template-credentials.test.tstests/quality/docstrings.test.tstests/services/code-interpreter.test.tstests/services/desktop.test.tstests/services/filesystem.test.tstests/services/git.test.tstests/services/lsp.test.tstests/services/normalization-cleanup.test.tstests/services/process.test.tstests/services/pty.test.tstests/services/sandboxes-client.test.tstests/services/snapshots.test.tstests/services/ssh.test.tstests/services/transport.test.tstests/utils/helpers.tsvitest.config.ts
Comment on lines
+6
to
+7
| - "v*" | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When npm publishis run without--tag, what dist-tag is used by default, and how should prerelease versions be published to avoid setting latest?
💡 Result:
When npm publish is run without --tag, the default dist-tag used is latest. This is confirmed across multiple official npm documentation pages and changelogs. To publish prerelease versions (e.g., versions with a prerelease identifier like 1.0.0-alpha.1) without setting the latest tag, use the --tag flag with a different tag name, such as beta, alpha, next, or rc. For example: npm publish --tag beta. In npm 11 (current as of 2026), publishing prereleases without an explicit --tag requires specifying a tag explicitly; otherwise, it fails to prevent accidentally setting latest. For non-prerelease versions, the default latest is applied only if the version is greater than the current latest semver version (excluding prereleases).
Citations:
- 1: https://docs.npmjs.com/adding-dist-tags-to-packages/
- 2: https://docs.npmjs.com/cli/v8/commands/npm-publish
- 3: https://docs.npmjs.com/cli/v8/commands/npm-dist-tag/
- 4: feat!: no implicit latest tag on publish when latest > version npm/cli#7939
- 5: https://docs.npmjs.com/adding-dist-tags-to-packages
- 6: https://github.com/npm/cli/blob/latest/CHANGELOG.md
- 7: https://docs.npmjs.com/cli/v8/commands/npm-publish/
🏁 Script executed:
# First, let's find and read the release workflow file
find . -path "./.github/workflows/release.yml" -type fRepository: leap0-dev/leap0-js
Length of output: 1887
Publish prerelease versions with an explicit dist-tag to prevent incorrectly setting latest.
Without --tag, npm publish defaults to the latest dist-tag. Prerelease versions (e.g., v1.0.0-beta.1) trigger on the v* tag pattern but will incorrectly mark themselves as latest for consumers unless explicitly tagged otherwise.
Proposed fix
jobs:
publish:
runs-on: ubuntu-latest
@@
- name: Run tests
run: pnpm test
+ - name: Resolve npm dist-tag
+ id: npm_tag
+ run: |
+ if [[ "${GITHUB_REF_NAME#v}" == *-* ]]; then
+ echo "tag=next" >> "$GITHUB_OUTPUT"
+ else
+ echo "tag=latest" >> "$GITHUB_OUTPUT"
+ fi
+
- name: Publish package
- run: npm publish --provenance --access public
+ run: npm publish --provenance --access public --tag "${{ steps.npm_tag.outputs.tag }}"
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 6 - 7, The release workflow
currently triggers on the "v*" tag pattern and runs npm publish without a dist
tag, which causes prerelease tags (e.g., v1.0.0-beta.1) to be published as
latest; update the release job in release.yml so that the npm publish step adds
an explicit --tag for prereleases (e.g., --tag next or a tag derived from the
prerelease identifier) and only omits --tag for normal releases; detect
prerelease by checking the git tag string for a hyphen/prerelease identifier
(the workflow's tag ref matching "v*" and the npm publish command) and pass the
appropriate --tag flag accordingly.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
src/services/desktop.ts (1)
415-419:⚠️ Potential issue | 🟠 MajorGuard against
0/0count events being treated as ready.Line 418 uses
status.running >= status.total; this is true for0 >= 0and can return early before processes are actually running. Addstatus.total > 0to the readiness condition.Suggested fix
if ( typeof status.running === "number" && typeof status.total === "number" && + status.total > 0 && status.running >= status.total ) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/desktop.ts` around lines 415 - 419, The readiness check currently treats 0/0 as ready because it uses "status.running >= status.total"; update the condition where "status" is inspected (the block checking typeof status.running/total and comparing them) to also require status.total > 0 so the logic becomes: ensure both counts are numbers, status.total > 0, and status.running >= status.total before returning early; modify the condition around the status.running/status.total comparison in the same function to include this extra guard.
🧹 Nitpick comments (2)
src/core/otel.ts (1)
33-37: Consider removing or utilizing the unused_configparameter.The
_configparameter is accepted but not used. If it's reserved for future configuration (e.g., custom OTLP endpoints), consider adding a TODO comment. Otherwise, remove it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/otel.ts` around lines 33 - 37, The initOtel function declares an unused parameter _config (type Leap0ConfigResolved); either remove the unused parameter from initOtel's signature to avoid confusion, or explicitly document its intended future use by adding a TODO comment above the function that mentions reserved usage (e.g., custom OTLP endpoints) and rename _config to config if you want to acknowledge it as intentionally unused. Update any callers if you remove the parameter or adjust typings accordingly so initOtel's signature and usage remain consistent.src/services/templates.ts (1)
22-30: Extract the template-name rule instead of inlining it here.This block now defines the full template-name policy, but
rename()still just forwards{ name }. Pulling these checks into a shared helper or schema would keep both methods aligned and avoid drift over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/templates.ts` around lines 22 - 30, Extract the inline name-validation logic in src/services/templates.ts into a shared helper (e.g., validateTemplateName(name) or a reusable schema) and replace the current inline checks in the block (the conditions on parsed.name: trim, length <=64, no whitespace, not startsWith "system/") with a single call to that helper; then update rename() to call the same validateTemplateName before forwarding { name } so both code paths use the identical rule set and avoid divergence. Ensure the helper throws Leap0Error with the same message when validation fails and export or place it where rename() can import/use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/desktop.ts`:
- Around line 204-227: The response handling in typeText, pressKey, and hotkey
currently coerces arbitrary values to true via Boolean(...); instead, validate
that the response shape has a real boolean ok and return it directly. Update
each method (typeText, pressKey, hotkey) to parse the transport.requestJsonUrl
result against a schema like { ok: z.boolean() } (or at minimum check typeof
data?.ok === 'boolean') and then return parsed.ok; this prevents values like
"false", 1, or {} from being treated as success.
In `@src/services/filesystem.ts`:
- Around line 233-245: The setPermissions method currently allows callers to
pass no permission fields (mode/owner/group) and will send only { path } to the
server; update setPermissions (the async method named setPermissions in this
file that uses fsPath, jsonBody and compact) to validate params synchronously
and throw an Error (reject immediately) if none of params.mode, params.owner, or
params.group are provided so we fail fast on
sandbox.filesystem.setPermissions(path) misuse instead of making a needless
transport.request round-trip.
- Around line 100-107: The request body is needlessly converted to a Node Buffer
which forces Node-only semantics; in the call inside the filesystem service (the
this.transport.request invocation in the method that wraps
this.fsPath("write-file")), remove Buffer.from(content) and pass the Uint8Array
directly as body (e.g., body: content) so the Fetch-compatible runtime can send
it without an extra copy; also remove any Buffer import/usage tied to
writeBytes/write-file so the function remains web-standard (Uint8Array)
compatible and keep the "content-type": "application/octet-stream" header as-is.
In `@src/services/git.ts`:
- Around line 21-32: The json<T> helper currently force-unwraps the
transport.requestJson<T> result with "!" which can mask an undefined response;
modify the json method to capture the result into a local variable (call to
this.transport.requestJson<T>(...)), check if it is undefined, and if so throw a
clear, descriptive error (including endpoint and sandboxIdOf(sandbox)) instead
of returning undefined, otherwise return the value; this ensures callers like
normalize() never receive undefined and makes failures explicit.
In `@src/services/lsp.ts`:
- Around line 58-66: The change reordered optional params and causes
RequestOptions to be misinterpreted; fix didOpen and didOpenPath by preserving
the old options slot and adding runtime compatibility: keep the function
signature with options as the last parameter (options?: RequestOptions) or add
overloads, and inside the function detect argument shapes (e.g. if typeof text
=== "object" treat it as options and set text=undefined; if typeof version ===
"object" treat it as options and set version=1) so callers that pass options in
the old position continue to work; update both didOpen and didOpenPath to use
this argument-normalization logic.
In `@src/services/pty.ts`:
- Around line 22-50: The recv() promise lacks a "close" handler so if the
WebSocket closes before a message the promise can hang; add an onClose event
listener that calls cleanup() and rejects with a Leap0WebSocketError (e.g., "PTY
websocket closed") and ensure cleanup also removes the "close" listener; to
implement, define onClose (or move the cleanup definition after onClose) and
register this.socket.addEventListener("close", onClose, { once: true })
alongside the existing "message" and "error" listeners so every terminal path
calls cleanup and either resolves or rejects the promise.
In `@src/services/sandboxes.ts`:
- Around line 76-80: In create (async create(...)) guard params.templateName
before calling .trim(): validate that params is an object and that
params.templateName is either undefined/null (use DEFAULT_TEMPLATE_NAME) or a
string, and if it's present but not a string throw a Leap0Error with a clear
message; then call .trim() on the verified string (or on DEFAULT_TEMPLATE_NAME).
Update references in the method to use the validated/templateName variable (not
raw params.templateName) so invalid inputs like create(null) or create({
templateName: 123 }) produce a Leap0Error instead of a raw TypeError.
- Around line 60-73: The class currently allows new
SandboxesClient<MyWrapper>(transport) without a SandboxFactory, making
wrap(data: SandboxData) return a plain SandboxData as T; fix by changing the
constructor overloads so a factory is required whenever T is not the default
SandboxData: provide two overload signatures—one for the no-factory case that
fixes T = SandboxData (constructor(transport: Leap0Transport)) and one generic
overload that requires a SandboxFactory<T> (constructor(transport:
Leap0Transport, sandboxFactory: SandboxFactory<T>)); implement a single
constructor body that accepts sandboxFactory optionally but use the overload
types to ensure callers must pass a factory for custom T, and update wrap to
rely on sandboxFactory being present for non-default T (or assert/throw if
absent).
In `@src/services/templates.ts`:
- Around line 46-57: The rename method currently returns Promise<void> and uses
this.transport.request which discards the response; change rename(template:
TemplateRef, params: { name: string }, options: RequestOptions = {}) to return
Promise<TemplateData>, call this.transport.requestJson(...) instead of
this.transport.request inside the withErrorPrefix wrapper, and pass the same
PATCH path, method and jsonBody(params), then normalize the JSON response to
TemplateData before returning it (mirroring get()/list() behavior); ensure you
keep the withErrorPrefix("Failed to rename template: ", ...) wrapper and update
the function signature and any usages/docs to expect TemplateData.
In `@tests/client/client-sandbox.test.ts`:
- Around line 40-45: Move the restoration of process.env.LEAP0_API_KEY to before
awaiting client.close() so the environment is reset even if client.close()
throws; specifically, assign or delete process.env.LEAP0_API_KEY based on
originalApiKey first (using the existing originalApiKey variable and
LEAP0_API_KEY env key), then call await client.close() afterwards to ensure test
state is not leaked.
---
Duplicate comments:
In `@src/services/desktop.ts`:
- Around line 415-419: The readiness check currently treats 0/0 as ready because
it uses "status.running >= status.total"; update the condition where "status" is
inspected (the block checking typeof status.running/total and comparing them) to
also require status.total > 0 so the logic becomes: ensure both counts are
numbers, status.total > 0, and status.running >= status.total before returning
early; modify the condition around the status.running/status.total comparison in
the same function to include this extra guard.
---
Nitpick comments:
In `@src/core/otel.ts`:
- Around line 33-37: The initOtel function declares an unused parameter _config
(type Leap0ConfigResolved); either remove the unused parameter from initOtel's
signature to avoid confusion, or explicitly document its intended future use by
adding a TODO comment above the function that mentions reserved usage (e.g.,
custom OTLP endpoints) and rename _config to config if you want to acknowledge
it as intentionally unused. Update any callers if you remove the parameter or
adjust typings accordingly so initOtel's signature and usage remain consistent.
In `@src/services/templates.ts`:
- Around line 22-30: Extract the inline name-validation logic in
src/services/templates.ts into a shared helper (e.g., validateTemplateName(name)
or a reusable schema) and replace the current inline checks in the block (the
conditions on parsed.name: trim, length <=64, no whitespace, not startsWith
"system/") with a single call to that helper; then update rename() to call the
same validateTemplateName before forwarding { name } so both code paths use the
identical rule set and avoid divergence. Ensure the helper throws Leap0Error
with the same message when validation fails and export or place it where
rename() can import/use it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dd2dd94-f056-4426-91a4-43faf804e3a9
📒 Files selected for processing (24)
examples/filesystem_and_git.tsexamples/quickstart.tssrc/client/sandbox.tssrc/config/index.tssrc/core/otel.tssrc/services/code-interpreter.tssrc/services/desktop.tssrc/services/filesystem.tssrc/services/git.tssrc/services/lsp.tssrc/services/pty.tssrc/services/sandboxes.tssrc/services/templates.tstests/client/client-sandbox.test.tstests/core/normalize.test.tstests/quality/docstrings.test.tstests/services/desktop.test.tstests/services/filesystem.test.tstests/services/git.test.tstests/services/lsp.test.tstests/services/normalization-cleanup.test.tstests/services/process.test.tstests/services/sandboxes-client.test.tstests/services/ssh.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/quality/docstrings.test.ts
- src/config/index.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- examples/filesystem_and_git.ts
- examples/quickstart.ts
- tests/services/git.test.ts
- tests/services/normalization-cleanup.test.ts
- tests/services/process.test.ts
- tests/core/normalize.test.ts
- tests/services/desktop.test.ts
- src/client/sandbox.ts
- tests/services/sandboxes-client.test.ts
- tests/services/filesystem.test.ts
- src/services/code-interpreter.ts
- tests/services/ssh.test.ts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/index.ts (1)
42-66:⚠️ Potential issue | 🟠 MajorWrap ZodError in
Leap0Errorat the config resolution boundary.The constructor calls
resolveConfig()directly without catchingZodErrorexceptions. Consumers expectingLeap0Errorfor invalid client configuration will miss these validation failures. UsesafeParse()with error wrapping, or add a try-catch that transformsZodErrorintoLeap0Error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.ts` around lines 42 - 66, resolveConfig currently lets ZodError bubble up; wrap Zod validation failures in Leap0Error at the config boundary. Change validation to use safeParse (or wrap parse calls in try/catch) for leap0ConfigInputSchema and leap0ConfigResolvedSchema (or the top-level parse around the returned object) and when a ZodError is returned or caught, rethrow a new Leap0Error that includes the ZodError details/message. Ensure the exported resolveConfig function still returns the same Leap0ConfigResolved on success but always converts ZodError -> Leap0Error so callers receive the expected error type.
🧹 Nitpick comments (6)
tests/services/pty.test.ts (3)
10-10: Consider using a more accurate type foroptionsparameter.Using
neveras the type foroptionsis misleading since the function does receive actual options values. While the code works due to theas nevercast on line 26, consider usingunknownor the actualRequestOptionstype for clarity.- requestJson: async (path: string, init: RequestInit, options: never) => { + requestJson: async (path: string, init: RequestInit, options: unknown) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/pty.test.ts` at line 10, The parameter type for the requestJson helper is incorrectly declared as never; change the signature for requestJson (the async function named requestJson) to accept a more accurate type (e.g., options: unknown or the actual RequestOptions type used elsewhere) and remove the need for the `as never` cast where requestJson is called; update any callers or casts to match the new options type to keep type-safety and clarity.
78-80: SameremoveEventListenersignature issue as the previous test.For consistency and accuracy, consider updating the mock signature to match the real API.
- removeEventListener(type: string) { + removeEventListener(type: string, _handler: (event?: { data?: unknown }) => void) { handlers.delete(type); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/pty.test.ts` around lines 78 - 80, The mock removeEventListener implementation in the test (removeEventListener) doesn't match the real DOM/EventTarget signature; update the mock to accept the same parameters as the real API (type and listener), e.g. change removeEventListener(type: string, listener?: EventListenerOrEventListenerObject | null) and use the handlers map to remove only the provided listener (or clear the list for that type when listener is null/undefined), ensuring the mock's addEventListener, removeEventListener, and handlers usage behave consistently with the real EventTarget semantics.
56-58: MockremoveEventListenersignature doesn't match the real WebSocket API.The real
WebSocket.removeEventListener(and the implementation inPtyConnection.recv()) takes bothtypeandhandlerparameters, but this mock only acceptstype. While this works because the Map stores one handler per type, it doesn't validate that the correct handler reference is passed during cleanup.- removeEventListener(type: string) { + removeEventListener(type: string, _handler: (event: { data?: unknown }) => void) { handlers.delete(type); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/pty.test.ts` around lines 56 - 58, The mock removeEventListener currently defined as removeEventListener(type: string) should be changed to match the real WebSocket API by accepting both type and handler (e.g., removeEventListener(type: string, handler: EventListener | null)) and only remove the stored handler if it strictly equals the provided handler; update the mock that uses the handlers Map and adjust any related test expectations accordingly so behavior mirrors PtyConnection.recv() which passes both type and handler.src/models/sandbox.ts (2)
60-73: Schema tightening removes previously typed fields.The schema now omits
templateName,timeoutMin,envVars, andupdatedAtas typed properties. While.catchall(z.unknown())preserves these from API responses, consumers lose type safety and must use bracket notation (e.g.,sandbox["templateName"]) to access them.If these fields are still returned by the API and needed by consumers, consider keeping them as optional typed properties for better DX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/sandbox.ts` around lines 60 - 73, The sandboxDataSchema's use of .catchall(z.unknown()) preserves extra API fields at runtime but removes compile-time typing for properties like templateName, timeoutMin, envVars, and updatedAt; update sandboxDataSchema to explicitly include these fields as optional typed properties (e.g., templateName: z.string().optional(), timeoutMin: z.number().optional(), envVars: z.record(z.string()).optional(), updatedAt: z.string().optional()) so SandboxData has proper types while retaining .catchall(z.unknown()) for any other unknown fields.
16-24: Type safety gap inisSandboxDatatype guard: accepts invalid state values.The
isSandboxDatafunction atsrc/client/sandbox.ts:26only checkstypeof record.state === "string"without validating that the state value is a validSandboxStateenum member. This allows objects with invalid state strings (e.g.,"invalid_state") to pass the type guard even though they fail schema validation later. Consider usingsandboxStateSchema.safeParse(record.state).successor checking enum membership explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/sandbox.ts` around lines 16 - 24, The isSandboxData type guard currently only verifies typeof record.state === "string" and thus accepts arbitrary strings; update isSandboxData (in src/client/sandbox.ts) to validate state against the canonical SandboxState values or the existing sandboxStateSchema (e.g., use sandboxStateSchema.safeParse(record.state).success or check that record.state is one of Object.values(SandboxState)) so only valid enum members pass the guard; ensure the rest of the predicate still checks other fields as before but replace the loose string check with the enum/schema validation to tighten type safety.tests/services/git.test.ts (1)
7-61: Add one assertion for a renamed snake_case key.The risky part of this refactor is camelCase→snake_case mapping, but this test only checks fields that keep the same name. One representative assertion for
commit_id,branch_type,context_lines,set_upstream, or the timestamp fields would cover the new mapping logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/git.test.ts` around lines 7 - 61, The test is missing an assertion that verifies camelCase→snake_case mapping (e.g., setUpstream → set_upstream); update the test to capture the checkout request (find the call where path === "/v1/sandbox/sb-1/git/checkout") after the existing checkoutBranch("sb-1", { path: "/workspace/repo", branch: "feat", setUpstream: true }) invocation (add setUpstream: true to that checkoutBranch call if not already present) and add an assertion like assert.deepEqual(jsonOf(checkoutCall!), { path: "/workspace/repo", branch: "feat", set_upstream: true }) to ensure the transport payload uses snake_case for set_upstream; reference the checkoutBranch invocation and the checkoutCall variable to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/index.ts`:
- Around line 23-31: The function resolveSdkOtelFromEnv should treat any
non-boolean string as a configuration error instead of falling back to
OTEL_EXPORTER_OTLP_ENDPOINT; update resolveSdkOtelFromEnv to accept undefined
(use Boolean(readEnv("OTEL_EXPORTER_OTLP_ENDPOINT"))), accept case-insensitive
"true" and "false" (e.g., envOtel?.toLowerCase()), and for any other non-empty
value throw an explicit Error (or return a rejected/handled config error)
mentioning the invalid LEAP0_SDK_OTEL_ENABLED value so callers can fail fast and
surface the misconfiguration.
In `@src/core/otel.ts`:
- Around line 58-72: The OpenTelemetry shutdown function shutdownOtel is never
invoked; update the lifecycle by calling await shutdownOtel() from
Leap0Client.close() (the same place where initOtel() is logically paired) —
import shutdownOtel into the client module, invoke and await it in
Leap0Client.close(), wrap the call in try/catch to log errors but not throw, and
ensure close() remains idempotent if shutdownOtel() can be called multiple
times; alternatively, if you prefer external lifecycle ownership, add a clear
public lifecycle API that documents calling shutdownOtel() on application
shutdown instead of calling it from Leap0Client.close().
In `@src/models/filesystem.ts`:
- Around line 80-88: Update setPermissionsParamsSchema so empty or
whitespace-only strings are rejected: change each optional field definition
(mode, owner, group) to use z.string().trim().min(1).optional() so blank values
like "" or " " fail validation, and keep the existing .refine that ensures at
least one of mode/owner/group is present; reference the
setPermissionsParamsSchema symbol and the mode/owner/group field names when
applying the change.
In `@src/models/template.ts`:
- Around line 14-22: The schema templateImageConfigSchema currently requires the
keys entrypoint and cmd to exist (even if null); change both to be optional and
nullable like env (e.g., use z.array(z.string()).nullable().optional()) so
templateDataSchema will accept image configs that omit these fields; update the
entrypoint and cmd definitions in templateImageConfigSchema accordingly.
In `@src/services/filesystem.ts`:
- Around line 193-200: The loop in src/services/filesystem.ts that processes
response.formData() currently ignores non-Blob FormData parts, risking silent
data loss; in the for (const [name, value] of formData.entries()) loop (the code
that creates result: Record<string, Uint8Array>), add a guard that throws an
error (including the entry name and type) whenever value is not a Blob/File so
the function fails fast on unexpected string/metadata parts instead of dropping
them; preserve existing Blob handling (await value.arrayBuffer()) and ensure the
thrown error makes it clear it originated while parsing the /read-files
response.
In `@tests/services/desktop.test.ts`:
- Around line 67-83: The test is not exercising the error branches because
statusStream only consumes streamJsonUrl; update or add a test that uses
createRecordedTransport with streamJsonUrl async generator yielding an SSE error
frame (e.g. yield { error: "Desktop request failed" } or yield { message:
"Desktop request failed" }) and then assert that iterating
client.statusStream("sb-1") throws or produces a Leap0Error; reference the
DesktopClient.statusStream method, the streamJsonUrl stub in
createRecordedTransport, and the Leap0Error class to locate where to change the
test and make the assertion.
---
Outside diff comments:
In `@src/config/index.ts`:
- Around line 42-66: resolveConfig currently lets ZodError bubble up; wrap Zod
validation failures in Leap0Error at the config boundary. Change validation to
use safeParse (or wrap parse calls in try/catch) for leap0ConfigInputSchema and
leap0ConfigResolvedSchema (or the top-level parse around the returned object)
and when a ZodError is returned or caught, rethrow a new Leap0Error that
includes the ZodError details/message. Ensure the exported resolveConfig
function still returns the same Leap0ConfigResolved on success but always
converts ZodError -> Leap0Error so callers receive the expected error type.
---
Nitpick comments:
In `@src/models/sandbox.ts`:
- Around line 60-73: The sandboxDataSchema's use of .catchall(z.unknown())
preserves extra API fields at runtime but removes compile-time typing for
properties like templateName, timeoutMin, envVars, and updatedAt; update
sandboxDataSchema to explicitly include these fields as optional typed
properties (e.g., templateName: z.string().optional(), timeoutMin:
z.number().optional(), envVars: z.record(z.string()).optional(), updatedAt:
z.string().optional()) so SandboxData has proper types while retaining
.catchall(z.unknown()) for any other unknown fields.
- Around line 16-24: The isSandboxData type guard currently only verifies typeof
record.state === "string" and thus accepts arbitrary strings; update
isSandboxData (in src/client/sandbox.ts) to validate state against the canonical
SandboxState values or the existing sandboxStateSchema (e.g., use
sandboxStateSchema.safeParse(record.state).success or check that record.state is
one of Object.values(SandboxState)) so only valid enum members pass the guard;
ensure the rest of the predicate still checks other fields as before but replace
the loose string check with the enum/schema validation to tighten type safety.
In `@tests/services/git.test.ts`:
- Around line 7-61: The test is missing an assertion that verifies
camelCase→snake_case mapping (e.g., setUpstream → set_upstream); update the test
to capture the checkout request (find the call where path ===
"/v1/sandbox/sb-1/git/checkout") after the existing checkoutBranch("sb-1", {
path: "/workspace/repo", branch: "feat", setUpstream: true }) invocation (add
setUpstream: true to that checkoutBranch call if not already present) and add an
assertion like assert.deepEqual(jsonOf(checkoutCall!), { path:
"/workspace/repo", branch: "feat", set_upstream: true }) to ensure the transport
payload uses snake_case for set_upstream; reference the checkoutBranch
invocation and the checkoutCall variable to locate the change.
In `@tests/services/pty.test.ts`:
- Line 10: The parameter type for the requestJson helper is incorrectly declared
as never; change the signature for requestJson (the async function named
requestJson) to accept a more accurate type (e.g., options: unknown or the
actual RequestOptions type used elsewhere) and remove the need for the `as
never` cast where requestJson is called; update any callers or casts to match
the new options type to keep type-safety and clarity.
- Around line 78-80: The mock removeEventListener implementation in the test
(removeEventListener) doesn't match the real DOM/EventTarget signature; update
the mock to accept the same parameters as the real API (type and listener), e.g.
change removeEventListener(type: string, listener?:
EventListenerOrEventListenerObject | null) and use the handlers map to remove
only the provided listener (or clear the list for that type when listener is
null/undefined), ensuring the mock's addEventListener, removeEventListener, and
handlers usage behave consistently with the real EventTarget semantics.
- Around line 56-58: The mock removeEventListener currently defined as
removeEventListener(type: string) should be changed to match the real WebSocket
API by accepting both type and handler (e.g., removeEventListener(type: string,
handler: EventListener | null)) and only remove the stored handler if it
strictly equals the provided handler; update the mock that uses the handlers Map
and adjust any related test expectations accordingly so behavior mirrors
PtyConnection.recv() which passes both type and handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 486c5ddb-d9df-4aea-bd96-1cb70db32f4a
📒 Files selected for processing (22)
src/client/index.tssrc/config/index.tssrc/core/otel.tssrc/models/config.tssrc/models/filesystem.tssrc/models/sandbox.tssrc/models/template.tssrc/services/desktop.tssrc/services/filesystem.tssrc/services/git.tssrc/services/lsp.tssrc/services/pty.tssrc/services/sandboxes.tssrc/services/templates.tstests/client/client-sandbox.test.tstests/services/desktop.test.tstests/services/filesystem.test.tstests/services/git.test.tstests/services/lsp.test.tstests/services/normalization-cleanup.test.tstests/services/pty.test.tstests/services/sandboxes-client.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/services/lsp.test.ts
- src/models/config.ts
- tests/client/client-sandbox.test.ts
- src/services/pty.ts
- tests/services/sandboxes-client.test.ts
- src/services/sandboxes.ts
- tests/services/normalization-cleanup.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
tests/services/pty.test.ts (2)
29-35: Assert methods and payload fields, not only pathsThis test is named around request shapes, but it currently verifies only selected paths. Add assertions for HTTP method and key body fields (
id,cols,rows,cwd) so regressions in normalization are caught.Proposed assertion additions
assert.equal(calls[0]?.path, "/v1/sandbox/sb-1/pty"); assert.equal(calls[1]?.path, "/v1/sandbox/sb-1/pty"); assert.equal(calls[3]?.path, "/v1/sandbox/sb-1/pty/sess-1/resize"); + assert.equal(calls[0]?.init.method ?? "GET", "GET"); + assert.equal(calls[1]?.init.method, "POST"); + const createBody = JSON.parse(String(calls[1]?.init.body ?? "{}")); + assert.equal(createBody.id, "sess-1"); + assert.equal(createBody.cwd, "/workspace"); + assert.equal(createBody.cols, 80); + assert.equal(createBody.rows, 24); + assert.equal(calls[3]?.init.method, "POST"); + const resizeBody = JSON.parse(String(calls[3]?.init.body ?? "{}")); + assert.equal(resizeBody.cols, 120); + assert.equal(resizeBody.rows, 40);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/pty.test.ts` around lines 29 - 35, The test only checks request paths but should also assert HTTP methods and request body fields to validate request shapes; update the assertions after invoking client.create/get/resize/delete in tests/services/pty.test.ts to verify calls[0], calls[1], etc. include the correct method (e.g., "POST" for create, "GET" for get, "POST" or "PUT" for resize/delete as appropriate) and that the body contains expected keys and values (id, cols, rows, cwd) for client.create and the resize payload (cols, rows) for client.resize; reference the client.create, client.get, client.resize, client.delete calls and the calls[] entries to locate where to add these assertions.
46-70: Extract duplicated websocket mock setup into a helper
handlers/socketlistener plumbing is duplicated in two tests. A small local helper will keep these tests shorter and reduce maintenance drift.Also applies to: 82-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/pty.test.ts` around lines 46 - 70, Extract the duplicated websocket mock plumbing into a small helper factory (e.g., createMockSocket) that returns the socket object and its associated handlers/state; move the MockListener type, handlers Map and the socket methods (send, close, addEventListener, removeEventListener) into that helper and have tests call createMockSocket() to get { socket, handlers, sent, closed } (or similar) and use those values in both test locations instead of repeating the block; update references to addEventListener/removeEventListener/send/close in the tests to use the returned socket and keep the existing behavior unchanged.src/services/desktop.ts (1)
404-408: Preserve the original SSE error payload.
Leap0Error.bodyis part of the public error contract, but{ error: ... }is stringified and{ message: ... }dropsbodyentirely. That makes the two server error shapes diverge and loses structured payloads.🧰 Suggested cleanup
if (record.error !== undefined) { - throw new Leap0Error("Desktop status stream error", { body: String(record.error) }); + throw new Leap0Error("Desktop status stream error", { body: record.error }); } if (typeof record.message === "string") { - throw new Leap0Error(String(record.message)); + throw new Leap0Error(record.message, { body: record.message }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/desktop.ts` around lines 404 - 408, The SSE error handling currently stringifies record.error and drops structured payload when record.message exists; update the throws to preserve the original payload: when checking record.error use throw new Leap0Error("Desktop status stream error", { body: record.error }) (do not String() it) and when handling typeof record.message === "string" throw new Leap0Error(String(record.message), { body: record.body ?? record }) so the public Leap0Error.body retains the original structured SSE payload (falling back to the whole record if no body field).src/models/desktop.ts (1)
20-80: Make the request schemas strip or reject unknown keys.These are client input validators, but
.catchall(z.unknown())preserves arbitrary fields. Since the service serializes the parsed objects back out, unsupported input still leaks onto the wire.desktopScreenshotParamsSchemaalready strips extras by default; the other request schemas should behave the same way.♻️ Suggested shape hardening
export const desktopSetScreenParamsSchema = z .object({ width: z.number().int().min(320).max(7680), height: z.number().int().min(320).max(4320), - }) - .catchall(z.unknown()); + }); export const desktopClickParamsSchema = z .object({ x: z.number().int().min(0).optional(), y: z.number().int().min(0).optional(), button: z.number().int().min(1).max(3).optional(), }) - .catchall(z.unknown()) .refine((params) => (params.x === undefined) === (params.y === undefined), { message: "x and y must be provided together or both omitted", });Apply the same removal to
desktopScreenshotRegionParamsSchema,desktopDragParamsSchema, anddesktopScrollParamsSchema. If you want hard failures instead of silent stripping,z.strictObject(...)is even better here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/desktop.ts` around lines 20 - 80, Several request schemas (desktopSetScreenParamsSchema, desktopScreenshotRegionParamsSchema, desktopClickParamsSchema, desktopDragParamsSchema, desktopScrollParamsSchema) currently use .catchall(z.unknown()) which preserves arbitrary client fields; remove the .catchall(z.unknown()) calls (so Zod will strip unknown keys by default) or, if you prefer hard failures, replace each .catchall(z.unknown()) with .strict() to reject unknown keys; update those schema definitions accordingly so parsed objects no longer leak unsupported keys.src/services/snapshots.ts (1)
35-48: Move param parsing insidewithErrorPrefix.Right now bad input throws before the wrapper runs, so request-validation failures come back as raw
ZodErrors while transport and response-schema failures get theFailed to ...prefix.Suggested change
async create( sandbox: SandboxRef, params: CreateSnapshotParams = {}, options: RequestOptions = {}, ): Promise<SnapshotData> { - const parsed = createSnapshotParamsSchema.parse(params); return withErrorPrefix("Failed to create snapshot: ", async () => { + const parsed = createSnapshotParamsSchema.parse(params); const data = await this.transport.requestJson<unknown>( `/v1/sandbox/${sandboxIdOf(sandbox)}/snapshot/create`, { method: "POST", body: jsonBody(parsed) }, options, );Apply the same move to
pause()andresume().Also applies to: 52-65, 69-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/snapshots.ts` around lines 35 - 48, The Zod parsing (e.g., createSnapshotParamsSchema.parse(params)) happens before withErrorPrefix so validation errors escape the prefix; move the parse calls into the withErrorPrefix callback for create (function create) so that invalid input errors are wrapped by "Failed to create snapshot: ". Apply the identical change to pause() and resume() (move their param/schema.parse invocations into their withErrorPrefix callbacks and use the appropriate error-prefix messages) and ensure you still pass the parsed result into the transport.requestJson and normalize calls.src/models/template.ts (1)
113-116: ReusetemplateNameSchemafor rename validation.
renameTemplateParamsSchemacurrently accepts names with whitespace or asystem/prefix thatcreateTemplateRequestSchemarejects, so the exported validators disagree about the same field.Suggested change
-export const renameTemplateNameSchema = z.string().min(1).max(64); +export const renameTemplateNameSchema = templateNameSchema;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/template.ts` around lines 113 - 116, The rename validator is looser than the create validator—replace the ad-hoc renameTemplateNameSchema with the existing templateNameSchema so renameTemplateParamsSchema uses the same rules as createTemplateRequestSchema; update renameTemplateParamsSchema to reference templateNameSchema (instead of defining renameTemplateNameSchema) so names with whitespace or a "system/" prefix are rejected consistently.src/models/sandbox.ts (1)
144-189: Use Zod 4’s unifiederroroption here.This block still passes
invalid_type_error, but the repo context documents Zod 4’s unifiederrorparameter instead. If the package is already on Zod 4, these constructors are on the wrong API surface for custom type messages.Suggested change
templateName: z.preprocess( (value) => value ?? DEFAULT_TEMPLATE_NAME, z - .string({ invalid_type_error: "templateName must be a string" }) + .string({ error: "templateName must be a string" }) .trim() .min(1, "templateName must be 1-64 characters") .max(64, "templateName must be 1-64 characters"), ), vcpu: z.preprocess( (value) => value ?? DEFAULT_VCPU, z - .number({ invalid_type_error: "vcpu must be between 1 and 8" }) + .number({ error: "vcpu must be between 1 and 8" }) .int("vcpu must be between 1 and 8") .min(1, "vcpu must be between 1 and 8") .max(8, "vcpu must be between 1 and 8"), ), memoryMib: z.preprocess( (value) => value ?? DEFAULT_MEMORY_MIB, z - .number({ invalid_type_error: "memoryMib must be even and between 512 and 8192" }) + .number({ error: "memoryMib must be even and between 512 and 8192" }) .int("memoryMib must be even and between 512 and 8192") .min(512, "memoryMib must be even and between 512 and 8192") .max(8192, "memoryMib must be even and between 512 and 8192") @@ timeoutMin: z.preprocess( (value) => value ?? DEFAULT_TIMEOUT_MIN, z - .number({ invalid_type_error: "timeoutMin must be between 1 and 480" }) + .number({ error: "timeoutMin must be between 1 and 480" }) .int("timeoutMin must be between 1 and 480") .min(1, "timeoutMin must be between 1 and 480") .max(480, "timeoutMin must be between 1 and 480"), @@ - { invalid_type_error: "params must be an object" }, + { error: "params must be an object" }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/sandbox.ts` around lines 144 - 189, The schema uses the old invalid_type_error option; update all occurrences (in createSandboxRuntimeParamsSchema for the outer z.object and the z.string/z.number constructors for templateName, vcpu, memoryMib, timeoutMin) to use Zod 4’s unified error option instead — replace invalid_type_error: "..." with error: { message: "..." } (preserving each original message string) so the object, string, and number constructors use the unified error shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/index.ts`:
- Around line 76-88: The close() method currently flips this.closed before
awaiting async cleanup so if this.transport.close() throws, shutdownOtel() is
skipped; change close() to only mark this.closed after all cleanup and always
attempt OTEL shutdown regardless of transport errors. Concretely, in close()
(referencing close(), this.transport.close(), this.sdkOtelEnabled,
shutdownOtel(), and this.closed) first check and return if already closed, then
call this.transport.close() inside a try/catch capturing any error, always run
shutdownOtel() when this.sdkOtelEnabled (with its own try/catch), set
this.closed = true after those steps, and finally rethrow the transport error if
one occurred so callers still see the failure.
In `@src/models/sandbox.ts`:
- Around line 60-73: The isValidCidr function accepts strings with multiple '/'
segments because it only destructures the first two tokens; modify isValidCidr
to first check that value contains exactly one '/' (e.g., ensure
value.split("/").length === 2 or that lastIndexOf("/") === indexOf("/")) and
return false if not, then proceed to validate address and prefix as currently
implemented; update validation in function isValidCidr to reject inputs like
"10.0.0.0/8/extra".
In `@src/services/desktop.ts`:
- Around line 179-212: drag() and scroll() currently send payloads without
runtime validation; import desktopDragParamsSchema and desktopScrollParamsSchema
from '@/models/desktop.js' and call desktopDragParamsSchema.parse(payload)
inside drag() and desktopScrollParamsSchema.parse(payload) inside scroll() (use
the parsed result when building jsonBody) to match the validation used by
click() and screenshotRegion() and ensure malformed input is rejected before
calling requestJson("/api/input/drag", ...) and requestJson("/api/input/scroll",
...).
In `@tests/config/config-utils.test.ts`:
- Around line 88-98: The resolveSdkOtelFromEnv path currently throws a plain
Error for invalid LEAP0_SDK_OTEL_ENABLED values causing an inconsistent public
error type; update resolveSdkOtelFromEnv (the function used by resolveConfig) to
throw a Leap0Error with the existing error message when encountering invalid
strings, and then update the test in config-utils.test.ts to assert that
resolveConfig() throws a Leap0Error (not a plain Error) for the "maybe" value so
error types are consistent across config validation.
---
Nitpick comments:
In `@src/models/desktop.ts`:
- Around line 20-80: Several request schemas (desktopSetScreenParamsSchema,
desktopScreenshotRegionParamsSchema, desktopClickParamsSchema,
desktopDragParamsSchema, desktopScrollParamsSchema) currently use
.catchall(z.unknown()) which preserves arbitrary client fields; remove the
.catchall(z.unknown()) calls (so Zod will strip unknown keys by default) or, if
you prefer hard failures, replace each .catchall(z.unknown()) with .strict() to
reject unknown keys; update those schema definitions accordingly so parsed
objects no longer leak unsupported keys.
In `@src/models/sandbox.ts`:
- Around line 144-189: The schema uses the old invalid_type_error option; update
all occurrences (in createSandboxRuntimeParamsSchema for the outer z.object and
the z.string/z.number constructors for templateName, vcpu, memoryMib,
timeoutMin) to use Zod 4’s unified error option instead — replace
invalid_type_error: "..." with error: { message: "..." } (preserving each
original message string) so the object, string, and number constructors use the
unified error shape.
In `@src/models/template.ts`:
- Around line 113-116: The rename validator is looser than the create
validator—replace the ad-hoc renameTemplateNameSchema with the existing
templateNameSchema so renameTemplateParamsSchema uses the same rules as
createTemplateRequestSchema; update renameTemplateParamsSchema to reference
templateNameSchema (instead of defining renameTemplateNameSchema) so names with
whitespace or a "system/" prefix are rejected consistently.
In `@src/services/desktop.ts`:
- Around line 404-408: The SSE error handling currently stringifies record.error
and drops structured payload when record.message exists; update the throws to
preserve the original payload: when checking record.error use throw new
Leap0Error("Desktop status stream error", { body: record.error }) (do not
String() it) and when handling typeof record.message === "string" throw new
Leap0Error(String(record.message), { body: record.body ?? record }) so the
public Leap0Error.body retains the original structured SSE payload (falling back
to the whole record if no body field).
In `@src/services/snapshots.ts`:
- Around line 35-48: The Zod parsing (e.g.,
createSnapshotParamsSchema.parse(params)) happens before withErrorPrefix so
validation errors escape the prefix; move the parse calls into the
withErrorPrefix callback for create (function create) so that invalid input
errors are wrapped by "Failed to create snapshot: ". Apply the identical change
to pause() and resume() (move their param/schema.parse invocations into their
withErrorPrefix callbacks and use the appropriate error-prefix messages) and
ensure you still pass the parsed result into the transport.requestJson and
normalize calls.
In `@tests/services/pty.test.ts`:
- Around line 29-35: The test only checks request paths but should also assert
HTTP methods and request body fields to validate request shapes; update the
assertions after invoking client.create/get/resize/delete in
tests/services/pty.test.ts to verify calls[0], calls[1], etc. include the
correct method (e.g., "POST" for create, "GET" for get, "POST" or "PUT" for
resize/delete as appropriate) and that the body contains expected keys and
values (id, cols, rows, cwd) for client.create and the resize payload (cols,
rows) for client.resize; reference the client.create, client.get, client.resize,
client.delete calls and the calls[] entries to locate where to add these
assertions.
- Around line 46-70: Extract the duplicated websocket mock plumbing into a small
helper factory (e.g., createMockSocket) that returns the socket object and its
associated handlers/state; move the MockListener type, handlers Map and the
socket methods (send, close, addEventListener, removeEventListener) into that
helper and have tests call createMockSocket() to get { socket, handlers, sent,
closed } (or similar) and use those values in both test locations instead of
repeating the block; update references to
addEventListener/removeEventListener/send/close in the tests to use the returned
socket and keep the existing behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab7e7593-750d-4ecc-b054-4452db34e77b
📒 Files selected for processing (24)
src/client/index.tssrc/client/sandbox.tssrc/config/index.tssrc/models/desktop.tssrc/models/filesystem.tssrc/models/sandbox.tssrc/models/snapshot.tssrc/models/template.tssrc/services/desktop.tssrc/services/filesystem.tssrc/services/git.tssrc/services/snapshots.tssrc/services/templates.tstests/client/client-sandbox.test.tstests/config/config-utils.test.tstests/config/otel.test.tstests/models/network-policy.test.tstests/models/template-credentials.test.tstests/services/desktop.test.tstests/services/filesystem.test.tstests/services/git.test.tstests/services/normalization-cleanup.test.tstests/services/pty.test.tstests/services/snapshots.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/services/normalization-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/config/otel.test.ts
- tests/models/network-policy.test.ts
- tests/models/template-credentials.test.ts
- tests/services/snapshots.test.ts
- tests/services/git.test.ts
- src/config/index.ts
- src/models/snapshot.ts
- src/services/templates.ts
- src/client/sandbox.ts
Comment on lines
+88
to
+98
| test("resolveConfig accepts case-insensitive sdk otel env values and rejects invalid strings", () => { | ||
| process.env.LEAP0_API_KEY = "env-key"; | ||
| process.env.LEAP0_SDK_OTEL_ENABLED = "TrUe"; | ||
| assert.equal(resolveConfig().sdkOtelEnabled, true); | ||
|
|
||
| process.env.LEAP0_SDK_OTEL_ENABLED = "FaLsE"; | ||
| assert.equal(resolveConfig().sdkOtelEnabled, false); | ||
|
|
||
| const config = resolveConfig() | ||
| assert.equal(config.sdkOtelEnabled, true) | ||
| }) | ||
| process.env.LEAP0_SDK_OTEL_ENABLED = "maybe"; | ||
| assert.throws(() => resolveConfig(), /Invalid LEAP0_SDK_OTEL_ENABLED value: maybe/); | ||
| }); |
There was a problem hiding this comment.
Unify config error type for invalid LEAP0_SDK_OTEL_ENABLED.
At Line [97], the test only checks message text. With current config code, this path throws a plain Error (from resolveSdkOtelFromEnv) while other invalid config paths are asserted as Leap0Error (Line [109]-[111]). This creates an inconsistent public error contract for resolveConfig.
Please normalize this path to Leap0Error in config code, then assert Leap0Error here too.
Proposed test assertion once config error wrapping is unified
- process.env.LEAP0_SDK_OTEL_ENABLED = "maybe";
- assert.throws(() => resolveConfig(), /Invalid LEAP0_SDK_OTEL_ENABLED value: maybe/);
+ process.env.LEAP0_SDK_OTEL_ENABLED = "maybe";
+ assert.throws(
+ () => resolveConfig(),
+ (error) =>
+ error instanceof Leap0Error &&
+ /Invalid LEAP0_SDK_OTEL_ENABLED value: maybe/.test(error.message),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/config/config-utils.test.ts` around lines 88 - 98, The
resolveSdkOtelFromEnv path currently throws a plain Error for invalid
LEAP0_SDK_OTEL_ENABLED values causing an inconsistent public error type; update
resolveSdkOtelFromEnv (the function used by resolveConfig) to throw a Leap0Error
with the existing error message when encountering invalid strings, and then
update the test in config-utils.test.ts to assert that resolveConfig() throws a
Leap0Error (not a plain Error) for the "maybe" value so error types are
consistent across config validation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/config/index.ts (1)
72-100: Consider unifying the validation pattern.The function mixes direct
.parse()calls (lines 74, 81, 82) withsafeParse+parseConfigOrThrow(lines 73, 87-97). Both work correctly since ZodErrors are caught by the outertry/catch, but using one pattern consistently would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.ts` around lines 72 - 100, Unify the Zod validation pattern by replacing the direct .parse() usages with safeParse + parseConfigOrThrow calls so all validations are handled consistently; specifically change apiKey = apiKeyRequiredSchema.parse(...), timeout = timeoutSchema.parse(...), and authHeader = authHeaderSchema.parse(...) to use parseConfigOrThrow(apiKeyRequiredSchema.safeParse(...)), parseConfigOrThrow(timeoutSchema.safeParse(...)), and parseConfigOrThrow(authHeaderSchema.safeParse(...)) respectively (keep existing inputs like input.apiKey ?? readEnv(...) and DEFAULT_CLIENT_TIMEOUT and the current surrounding try/catch and final leap0ConfigResolvedSchema.safeParse(...) usage intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/desktop.ts`:
- Around line 26-37: The desktopScreenshotParamsSchema currently only enforces
width/height pairing; add a second refine to enforce x/y pairing so that x and y
must be provided together. Specifically, update desktopScreenshotParamsSchema
(the existing z.object with fields format, quality, x, y, width, height) to
include another .refine(...) that checks (params.x === undefined) === (params.y
=== undefined) and provides a clear message like "x and y must be provided
together".
- Around line 128-147: The schemas are too permissive:
desktopProcessStatusSchema currently allows entirely empty objects and
desktopProcessStatusListSchema allows empty lists; tighten them so malformed
payloads fail validation by making key fields required and enforcing non-empty
arrays. Update desktopProcessStatusSchema to require at least name (z.string())
and running (z.boolean()) while keeping pid, stdoutLog, stderrLog optional;
update desktopProcessStatusListSchema to require status (z.enum(...)) and items
as z.array(desktopProcessStatusSchema).min(1) (and optionally keep running/total
optional or make them required if intended). Remove or reduce catchall use so
unknown values don’t mask missing required fields. Ensure types
DesktopProcessStatus and DesktopProcessStatusList derive from the revised
schemas.
---
Nitpick comments:
In `@src/config/index.ts`:
- Around line 72-100: Unify the Zod validation pattern by replacing the direct
.parse() usages with safeParse + parseConfigOrThrow calls so all validations are
handled consistently; specifically change apiKey =
apiKeyRequiredSchema.parse(...), timeout = timeoutSchema.parse(...), and
authHeader = authHeaderSchema.parse(...) to use
parseConfigOrThrow(apiKeyRequiredSchema.safeParse(...)),
parseConfigOrThrow(timeoutSchema.safeParse(...)), and
parseConfigOrThrow(authHeaderSchema.safeParse(...)) respectively (keep existing
inputs like input.apiKey ?? readEnv(...) and DEFAULT_CLIENT_TIMEOUT and the
current surrounding try/catch and final leap0ConfigResolvedSchema.safeParse(...)
usage intact).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 565e9030-3daa-4372-b64e-aae792a9a0fa
📒 Files selected for processing (15)
src/client/index.tssrc/config/index.tssrc/models/desktop.tssrc/models/filesystem.tssrc/models/sandbox.tssrc/models/template.tssrc/services/desktop.tssrc/services/lsp.tssrc/services/sandboxes.tssrc/services/snapshots.tstests/client/client-sandbox.test.tstests/config/config-utils.test.tstests/services/normalization-cleanup.test.tstests/services/pty.test.tstests/services/sandboxes-client.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/services/normalization-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/config/config-utils.test.ts
- tests/services/sandboxes-client.test.ts
- tests/services/pty.test.ts
- src/client/index.ts
- src/models/template.ts
- src/services/sandboxes.ts
- src/models/sandbox.ts
- src/services/desktop.ts
Comment on lines
+128
to
+147
| export const desktopProcessStatusSchema = z | ||
| .object({ | ||
| name: z.string().optional(), | ||
| running: z.boolean().optional(), | ||
| pid: z.number().optional(), | ||
| stdoutLog: z.string().optional(), | ||
| stderrLog: z.string().optional(), | ||
| }) | ||
| .catchall(z.unknown()); | ||
| export type DesktopProcessStatus = z.infer<typeof desktopProcessStatusSchema>; | ||
|
|
||
| export const desktopProcessStatusListSchema = z | ||
| .object({ | ||
| status: z.enum(["running", "degraded", "stopped"]).optional(), | ||
| items: z.array(desktopProcessStatusSchema).optional(), | ||
| running: z.number().optional(), | ||
| total: z.number().optional(), | ||
| }) | ||
| .catchall(z.unknown()); | ||
| export type DesktopProcessStatusList = z.infer<typeof desktopProcessStatusListSchema>; |
There was a problem hiding this comment.
Process status schemas are too permissive and can validate empty events.
With all fields optional, {} and { items: [{}] } pass validation. Because normalization/parsing is direct (src/core/normalize.ts, Line 26) and emitted downstream (src/services/desktop.ts, Line 416), malformed status payloads can slip through without a schema error.
🔧 Proposed tightening (low-breakage refinement approach)
export const desktopProcessStatusSchema = z
.object({
name: z.string().optional(),
running: z.boolean().optional(),
pid: z.number().optional(),
stdoutLog: z.string().optional(),
stderrLog: z.string().optional(),
})
+ .refine(
+ (v) =>
+ v.name !== undefined ||
+ v.running !== undefined ||
+ v.pid !== undefined ||
+ v.stdoutLog !== undefined ||
+ v.stderrLog !== undefined,
+ { message: "process status must contain at least one field" },
+ )
.catchall(z.unknown());
export const desktopProcessStatusListSchema = z
.object({
status: z.enum(["running", "degraded", "stopped"]).optional(),
items: z.array(desktopProcessStatusSchema).optional(),
running: z.number().optional(),
total: z.number().optional(),
})
+ .refine(
+ (v) =>
+ v.status !== undefined ||
+ v.items !== undefined ||
+ v.running !== undefined ||
+ v.total !== undefined,
+ { message: "status list must contain at least one summary field" },
+ )
.catchall(z.unknown());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const desktopProcessStatusSchema = z | |
| .object({ | |
| name: z.string().optional(), | |
| running: z.boolean().optional(), | |
| pid: z.number().optional(), | |
| stdoutLog: z.string().optional(), | |
| stderrLog: z.string().optional(), | |
| }) | |
| .catchall(z.unknown()); | |
| export type DesktopProcessStatus = z.infer<typeof desktopProcessStatusSchema>; | |
| export const desktopProcessStatusListSchema = z | |
| .object({ | |
| status: z.enum(["running", "degraded", "stopped"]).optional(), | |
| items: z.array(desktopProcessStatusSchema).optional(), | |
| running: z.number().optional(), | |
| total: z.number().optional(), | |
| }) | |
| .catchall(z.unknown()); | |
| export type DesktopProcessStatusList = z.infer<typeof desktopProcessStatusListSchema>; | |
| export const desktopProcessStatusSchema = z | |
| .object({ | |
| name: z.string().optional(), | |
| running: z.boolean().optional(), | |
| pid: z.number().optional(), | |
| stdoutLog: z.string().optional(), | |
| stderrLog: z.string().optional(), | |
| }) | |
| .refine( | |
| (v) => | |
| v.name !== undefined || | |
| v.running !== undefined || | |
| v.pid !== undefined || | |
| v.stdoutLog !== undefined || | |
| v.stderrLog !== undefined, | |
| { message: "process status must contain at least one field" }, | |
| ) | |
| .catchall(z.unknown()); | |
| export type DesktopProcessStatus = z.infer<typeof desktopProcessStatusSchema>; | |
| export const desktopProcessStatusListSchema = z | |
| .object({ | |
| status: z.enum(["running", "degraded", "stopped"]).optional(), | |
| items: z.array(desktopProcessStatusSchema).optional(), | |
| running: z.number().optional(), | |
| total: z.number().optional(), | |
| }) | |
| .refine( | |
| (v) => | |
| v.status !== undefined || | |
| v.items !== undefined || | |
| v.running !== undefined || | |
| v.total !== undefined, | |
| { message: "status list must contain at least one summary field" }, | |
| ) | |
| .catchall(z.unknown()); | |
| export type DesktopProcessStatusList = z.infer<typeof desktopProcessStatusListSchema>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/desktop.ts` around lines 128 - 147, The schemas are too
permissive: desktopProcessStatusSchema currently allows entirely empty objects
and desktopProcessStatusListSchema allows empty lists; tighten them so malformed
payloads fail validation by making key fields required and enforcing non-empty
arrays. Update desktopProcessStatusSchema to require at least name (z.string())
and running (z.boolean()) while keeping pid, stdoutLog, stderrLog optional;
update desktopProcessStatusListSchema to require status (z.enum(...)) and items
as z.array(desktopProcessStatusSchema).min(1) (and optionally keep running/total
optional or make them required if intended). Remove or reduce catchall use so
unknown values don’t mask missing required fields. Ensure types
DesktopProcessStatus and DesktopProcessStatusList derive from the revised
schemas.
This was referenced Apr 14, 2026
Merged
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 by CodeRabbit
New Features
Bug Fixes
Breaking Changes