feat: add Bun TypeScript SDK as a workspace package - #18
Merged
Conversation
`ignite serve` gave `--host` an auto-derived `-h`, which collides with
clap's generated `--help`. Under debug assertions that panics before a
single argument is parsed, so `cargo run -- serve` never started; release
builds silently rebound `-h` to `--host` instead. Move host to `-H`, which
is what the man page already documented, and add a regression test that
runs clap's `debug_assert()` over the whole command tree.
Default the daemon to port 9847 in both the CLI and the standalone HTTP
binary. 3000 is heavily contested on a developer machine, and the port is
now shared with the TypeScript SDK's default so neither side needs
configuring.
`execute_service` is synchronous and runs for the entire lifetime of the
microVM, but was called directly from an async handler. That parked a
Tokio worker thread for the whole execution, so enough concurrent calls
starved the runtime and stalled every other route including /health. Move
it onto `spawn_blocking` and surface `JoinError` rather than reporting a
success with no metrics.
`list_services` swallowed `read_dir` errors and returned `{"services":[]}`
with HTTP 200, making a misconfigured `--services` path indistinguishable
from an empty directory. Report the failure with the path and cause, and
sort results so the endpoint is deterministic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds @ignite/sdk under sdk/ts, wired up as a Bun workspace at the repo
root so `bun install` produces a committed lockfile and dependencies
resolve inside the repository.
Type resolution is pinned via typeRoots. Without it tsc walks past the
repository root and can satisfy @types/bun from a global node_modules,
which makes `typecheck` pass locally and fail in CI. With node_modules
removed the typecheck now fails as it should.
The package emits real build output: `exports`/`types`/`files` plus a
tsconfig.build.json emitting dist/ with declarations and sourcemaps.
Relative imports carry .js extensions so the emitted ESM resolves under
plain Node, not just Bun.
The client covers what the daemon actually does:
- Timeouts and AbortSignal on every method. executeService boots a
microVM so it gets a 5 minute budget against 30s for metadata calls,
both overridable, 0 to disable. A timeout raises IgniteTimeoutError
distinct from a caller abort, because a client-side timeout does not
mean the guest stopped running.
- Both daemon error shapes: the ExecuteResponse body and the bare
{"error"} used for 401 and 429, which carries no success field.
- Service names validated locally against the same rules as
validate_service_name, rejecting before spending a round trip.
- input typed as JsonValue. It was Record<string, unknown> | unknown,
a union that collapses to unknown and checked nothing.
Tests cover auth and rate-limit shapes, non-JSON error bodies,
success:false on HTTP 200, timeouts, aborts, and header handling. An
opt-in suite gated on IGNITE_TEST_BASE_URL runs against a live daemon,
since the unit tests assert against hand-written JSON and only that
suite proves the types match what the daemon emits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI and HTTP server now default to port 9847 and improve service listing and execution handling. A new TypeScript SDK provides typed client methods, errors, timeouts, cancellation, examples, tests, and package documentation. ChangesIgnite HTTP API and TypeScript SDK
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Example
participant IgniteClient
participant IgniteHTTP
participant IgniteService
Example->>IgniteClient: Check health and list services
IgniteClient->>IgniteHTTP: Send JSON request
IgniteHTTP-->>IgniteClient: Return API response
Example->>IgniteClient: Execute selected service
IgniteClient->>IgniteHTTP: Send execution request
IgniteHTTP->>IgniteService: Run validated service
IgniteService-->>IgniteHTTP: Return execution report
IgniteHTTP-->>IgniteClient: Return result or error
IgniteClient-->>Example: Report metrics, output, or typed failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
package.json (1)
9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise
engines.bunto the version that supportsbun run --filter.The workspace scripts use
bun run --filter '*' ..., and Bun added--filterin v1.1.4. Keeppackage.jsonfrom accepting Bun 1.0.x unless the SDK scripts are changed to use only flags available in that floor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 9 - 16, Update the package.json engines.bun constraint to require Bun 1.1.4 or newer, matching the --filter usage in the build, test, and typecheck scripts; do not alter the scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ignite-http/src/server.rs`:
- Around line 170-182: Update the directory iteration in the service-listing
handler to process each ReadDir result explicitly instead of using
entries.flatten(). On any iteration io::Error, return the same HTTP 500 response
path used for the initial read_dir failure; otherwise preserve the existing
directory filtering and deterministic services.sort() behavior.
- Around line 453-470: Update the test
list_services_reports_an_unreadable_directory_instead_of_an_empty_list to create
a tempfile::tempdir() and derive a guaranteed-missing child path from it,
replacing the hard-coded absolute PathBuf while preserving the existing
assertions.
- Around line 259-286: Extend ServerState with a shared semaphore sized to the
allowed concurrent service executions, and initialize it wherever the state is
constructed. In execute_service_handler, call try_acquire_owned() before
spawn_blocking; return the established 429 or 503 response when no permit is
available. Move the owned permit into the blocking closure so it remains held
through execute_service completion, including error and panic paths.
In `@sdk/ts/src/client.ts`:
- Line 232: Update executeService in sdk/ts/src/client.ts:232-232 to resolve
timeouts using request options first, then the explicitly provided constructor
timeout (preserving timeoutMs: 0), and finally the default; ensure
explicitTimeoutMs records whether the constructor option was supplied and add a
unit test asserting IgniteTimeoutError.timeoutMs. Update the timeoutMs
documentation in sdk/ts/src/types.ts:101-107 to describe the actual precedence,
and revise sdk/ts/README.md:95-105 to state that execution budgets are
configurable per request.
- Around line 167-177: Update the health() method to validate that body is a
valid HealthResponse-shaped object before casting or returning it, matching the
parsed-shape guards used by listServices and executeService. Treat undefined,
null, and non-object bodies as an error and preserve the existing IgniteApiError
response details.
---
Nitpick comments:
In `@package.json`:
- Around line 9-16: Update the package.json engines.bun constraint to require
Bun 1.1.4 or newer, matching the --filter usage in the build, test, and
typecheck scripts; do not alter the scripts.
🪄 Autofix
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 Plus
Run ID: f8a2603e-383a-4a56-8fcd-74a4a5d69197
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.mddocs/api.mddocs/getting-started.mddocs/man/ignite.1docs/walkthrough.mdignite-cli/src/main.rsignite-http/Cargo.tomlignite-http/src/main.rsignite-http/src/server.rspackage.jsonsdk/ts/README.mdsdk/ts/example.tssdk/ts/package.jsonsdk/ts/src/client.tssdk/ts/src/errors.tssdk/ts/src/index.tssdk/ts/src/types.tssdk/ts/test/client.test.tssdk/ts/test/integration.test.tssdk/ts/tsconfig.build.jsonsdk/ts/tsconfig.json
Comment on lines
170
to
+182
| let mut services = Vec::new(); | ||
| if let Ok(entries) = fs::read_dir(&state.services_path) { | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if let Some(name) = path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .filter(|_| path.is_dir()) | ||
| { | ||
| services.push(name.to_string()); | ||
| } | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if let Some(name) = path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .filter(|_| path.is_dir()) | ||
| { | ||
| services.push(name.to_string()); | ||
| } | ||
| } | ||
| Json(serde_json::json!({ "services": services })) | ||
| // Directory order is filesystem-dependent; sort so the API is deterministic. | ||
| services.sort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return an error for directory iteration failures.
Line 171 discards ReadDir errors with flatten(). If iteration fails after the directory opens, this endpoint returns a partial service list with HTTP 200. Handle each Result<DirEntry, io::Error> and return the same HTTP 500 response used for the initial read_dir failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 170 - 182, Update the directory
iteration in the service-listing handler to process each ReadDir result
explicitly instead of using entries.flatten(). On any iteration io::Error,
return the same HTTP 500 response path used for the initial read_dir failure;
otherwise preserve the existing directory filtering and deterministic
services.sort() behavior.
Comment on lines
+259
to
+286
| // `execute_service` is synchronous and runs for the entire lifetime of the | ||
| // microVM. Calling it directly from an async handler parks a Tokio worker | ||
| // thread for that whole duration, so enough concurrent executions starve | ||
| // the runtime and stall every other route, including `/health`. | ||
| let result = | ||
| tokio::task::spawn_blocking(move || execute_service(&service_dir, options, None, None)) | ||
| .await; | ||
|
|
||
| let result = match result { | ||
| Ok(result) => result, | ||
| Err(join_err) => { | ||
| // The blocking task panicked or was cancelled. Surface it rather | ||
| // than reporting success with no metrics. | ||
| tracing::error!(error = %join_err, "service execution task failed to complete"); | ||
| return ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(ExecuteResponse { | ||
| success: false, | ||
| service_name, | ||
| metrics: None, | ||
| preflight: None, | ||
| error: Some(format!("Execution task failed to complete: {join_err}")), | ||
| }), | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| match result { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the declared Tokio version and all execution-admission controls.
rg -n -C 4 \
-g 'Cargo.toml' -g 'Cargo.lock' -g '*.rs' \
'tokio|spawn_blocking|Semaphore|execute_service_handler|struct ServerState' .Repository: dev-dami/ignite
Length of output: 12060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo workspace Tokio dependency =="
sed -n '1,30p' Cargo.toml
echo
echo "== ServerState definition and execute handler outline =="
sed -n '1,45p' ignite-http/src/server.rs
echo
sed -n '200,295p' ignite-http/src/server.rs
echo
echo "== Existing Semaphore/Concurrency controls =="
rg -n '\bSemaphore\b|semaphore|try_acquire|acquire_owned|tokio::sync' ignite-http/src/server.rs ignition-http ignite-shared ignite-core ignite-cli || true
echo
echo "== Runtime/blocking-task settings around Tokio main and serve =="
sed -n '1,90p' ignite-http/src/main.rs
echo
echo "== Static probe: count spawn_blocking under execute_service_handler and ServerState fields =="
python3 - <<'PY'
from pathlib import Path
p=Path('ignite-http/src/server.rs')
text=p.read_text()
start=text.find(' async fn execute_service_handler')
end=text.find('\n pub fn create_router', start)
body=text[start:end] if start!=-1 and end!=-1 else ''
print("spawn_blocking_count=", body.count('spawn_blocking'))
print("ServerState fields in file:")
for i,line in enumerate(Path('ignite-http/src/server.rs').read_text().splitlines(),1):
if 'pub struct ServerState' in line or 'Semaphore' in line or 'RateLimiter' in line:
print(f"{i}: {line}")
PYRepository: dev-dami/ignite
Length of output: 8973
Bound concurrent service executions before spawn_blocking.
ServerState only keeps the request rate limiter and has no execution semaphore. execute_service_handler queues a blocking microVM task for every accepted request, with no Tokio maximum-blocking limit configured. Add a shared bounded execution semaphore, use try_acquire_owned() before scheduling work, return 429 or 503 when capacity is full, and keep the permit until execute_service completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 259 - 286, Extend ServerState with a
shared semaphore sized to the allowed concurrent service executions, and
initialize it wherever the state is constructed. In execute_service_handler,
call try_acquire_owned() before spawn_blocking; return the established 429 or
503 response when no permit is available. Move the owned permit into the
blocking closure so it remains held through execute_service completion,
including error and panic paths.
Comment on lines
+453
to
+470
| #[tokio::test] | ||
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | ||
| // Previously this swallowed the error and returned `{"services": []}` | ||
| // with HTTP 200, making a misconfigured path look like an empty one. | ||
| let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist"); | ||
|
|
||
| let (status, body) = get(test_state(missing), "/services").await; | ||
|
|
||
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | ||
| assert!( | ||
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | ||
| "expected a read failure message, got {body}" | ||
| ); | ||
| assert!( | ||
| body.get("services").is_none(), | ||
| "a failed listing must not report a services array" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a temporary path for the missing-directory test.
Line 457 embeds an absolute host path. The path can exist on a test host and makes the test depend on host filesystem layout. Create a missing child under tempfile::tempdir() instead.
As per coding guidelines: “Never introduce secrets, tokens, or host-specific paths into committed Rust code.”
Proposed fix
- let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist");
+ let dir = tempfile::tempdir().unwrap();
+ let missing = dir.path().join("missing");📝 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
| #[tokio::test] | |
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | |
| // Previously this swallowed the error and returned `{"services": []}` | |
| // with HTTP 200, making a misconfigured path look like an empty one. | |
| let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist"); | |
| let (status, body) = get(test_state(missing), "/services").await; | |
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | |
| assert!( | |
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | |
| "expected a read failure message, got {body}" | |
| ); | |
| assert!( | |
| body.get("services").is_none(), | |
| "a failed listing must not report a services array" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | |
| // Previously this swallowed the error and returned `{"services": []}` | |
| // with HTTP 200, making a misconfigured path look like an empty one. | |
| let dir = tempfile::tempdir().unwrap(); | |
| let missing = dir.path().join("missing"); | |
| let (status, body) = get(test_state(missing), "/services").await; | |
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | |
| assert!( | |
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | |
| "expected a read failure message, got {body}" | |
| ); | |
| assert!( | |
| body.get("services").is_none(), | |
| "a failed listing must not report a services array" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 453 - 470, Update the test
list_services_reports_an_unreadable_directory_instead_of_an_empty_list to create
a tempfile::tempdir() and derive a guaranteed-missing child path from it,
replacing the hard-coded absolute PathBuf while preserving the existing
assertions.
Source: Coding guidelines
Comment on lines
+167
to
+177
| const body = await IgniteClient.readBody(response); | ||
| if (!response.ok) { | ||
| throw new IgniteApiError( | ||
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | ||
| response.status, | ||
| undefined, | ||
| body, | ||
| ); | ||
| } | ||
| return body as HealthResponse; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the health body before the cast.
readBody returns undefined for an empty body. If the daemon answers 200 with an empty or non-object body, health() resolves to undefined while the declared return type is HealthResponse. The caller then throws a TypeError on health.status, as in sdk/ts/example.ts line 20.
listServices and executeService already guard the parsed shape. Apply the same guard here.
🛡️ Proposed fix
const body = await IgniteClient.readBody(response);
if (!response.ok) {
throw new IgniteApiError(
IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`),
response.status,
undefined,
body,
);
}
+ if (!body || typeof body !== 'object') {
+ throw new IgniteApiError(
+ 'Invalid JSON response from server during health check',
+ response.status,
+ undefined,
+ body,
+ );
+ }
return body as HealthResponse;📝 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
| const body = await IgniteClient.readBody(response); | |
| if (!response.ok) { | |
| throw new IgniteApiError( | |
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| return body as HealthResponse; | |
| } | |
| const body = await IgniteClient.readBody(response); | |
| if (!response.ok) { | |
| throw new IgniteApiError( | |
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| if (!body || typeof body !== 'object') { | |
| throw new IgniteApiError( | |
| 'Invalid JSON response from server during health check', | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| return body as HealthResponse; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/ts/src/client.ts` around lines 167 - 177, Update the health() method to
validate that body is a valid HealthResponse-shaped object before casting or
returning it, matching the parsed-shape guards used by listServices and
executeService. Treat undefined, null, and non-object bodies as an error and
preserve the existing IgniteApiError response details.
| audit: options.audit ?? false, | ||
| }), | ||
| }, | ||
| options.timeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constructor timeoutMs never applies to executeService. The root cause is at sdk/ts/src/client.ts line 232: executeService resolves its budget from options.timeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS and skips this.timeoutMs. A caller who sets new IgniteClient({ timeoutMs: 0 }) still gets a 5-minute execution timeout, and a caller who sets a shorter client budget still waits 5 minutes. The documentation in two other files states the opposite.
sdk/ts/src/client.ts#L232-L232: use the constructor value when it is set, for exampleoptions.timeoutMs ?? this.explicitTimeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS, whereexplicitTimeoutMsrecords whether the caller passedtimeoutMs. Add a unit test that sets a client-leveltimeoutMsand asserts the reportedIgniteTimeoutError.timeoutMsforexecuteService.sdk/ts/src/types.ts#L101-L107: correct thetimeoutMsdoc comment so it matches the chosen precedence, instead of stating that the value "Applies to every call".sdk/ts/README.md#L95-L105: correct line 98, which states that both defaults are configurable. State that the execution budget is configurable per request, or update it after the precedence fix lands.
📍 Affects 3 files
sdk/ts/src/client.ts#L232-L232(this comment)sdk/ts/src/types.ts#L101-L107sdk/ts/README.md#L95-L105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/ts/src/client.ts` at line 232, Update executeService in
sdk/ts/src/client.ts:232-232 to resolve timeouts using request options first,
then the explicitly provided constructor timeout (preserving timeoutMs: 0), and
finally the default; ensure explicitTimeoutMs records whether the constructor
option was supplied and add a unit test asserting IgniteTimeoutError.timeoutMs.
Update the timeoutMs documentation in sdk/ts/src/types.ts:101-107 to describe
the actual precedence, and revise sdk/ts/README.md:95-105 to state that
execution budgets are configurable per request.
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.
Description
Brief description of changes.
Type of Change
Checklist
Related Issues
Fixes #(issue number)
Summary by CodeRabbit
ignite servenow uses port 9847 by default; host selection uses-H.