feat(cli): standalone distribution with embedded host-service#3298
Merged
Conversation
Bundles the CLI binary + Node.js runtime + host-service + native addons into a single tarball per platform. Users run `superset auth login` and `superset host start --daemon` to expose their machine via the relay. - Implement host start/stop/status commands (were stubs) - Manifest format matches desktop app (shared at ~/superset/host/<orgId>/) - Migrate CLI home from ~/.superset to ~/superset for desktop compat - Store activeOrg locally in config.json (set during OAuth consent) - Host-service Bun build targeting Node.js (native addons external) - Distribution build script: CLI binary + Node + host-service.js + native - CI matrix workflow (macos arm64, macos x64, ubuntu x64) - Bake RELAY_URL and CLOUD_API_URL into the binary at build time
Contributor
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow and local build tooling to produce platform-specific superset CLI tarballs; bundles host-service and Node runtime; and implements local host-service lifecycle management, manifest persistence, and active-organization persistence in the CLI config. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Spawn as spawnHostService
participant OS as Operating System
participant Host as Host Service
participant Health as Health Endpoint
participant Manifest as Manifest Store
User->>CLI: run "host start"
CLI->>CLI: read config & validate activeOrg/auth
CLI->>Spawn: spawnHostService(orgId, sessionToken, port?, daemon?)
Spawn->>Spawn: choose port & generate secret
Spawn->>OS: spawn `superset-host` process (env: NODE, PORT, SECRET, DB, MIGRATIONS)
OS->>Host: process starts
Spawn->>Health: poll http://127.0.0.1:<port>/trpc/health.check (Bearer token)
Health-->>Spawn: responds OK
Spawn->>Manifest: write manifest (pid, endpoint, token, startedAt, orgId)
Spawn-->>CLI: return pid, port, secret
CLI->>User: report host started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR ships the CLI as a standalone, platform-specific tarball that embeds the host-service, a standalone Node.js runtime, and native addons — enabling non-desktop users to expose their machine through the existing relay infrastructure. It implements the previously-stubbed Key changes:
Confidence Score: 3/5Not ready to merge — the access token is written world-readable and the native addon resolution strategy in the tarball is likely broken. The host command logic, manifest lifecycle, CI workflow, and daemon handling are all well-structured. However, the access token is written world-readable (security regression), and the NODE_PATH mechanism used by the superset-host shell wrapper almost certainly cannot resolve require('better-sqlite3') / require('node-pty') from bare .node files — making the primary user path (host start) fail on a fresh install. The stop-command manifest race is a secondary reliability concern. packages/cli/scripts/build-dist.ts (native addon resolution), packages/cli/src/lib/config.ts (file permissions), packages/cli/src/commands/host/stop/command.ts (manifest race)
|
| Filename | Overview |
|---|---|
| packages/cli/scripts/build-dist.ts | New distribution build script; hardcoded native-addon versions will silently break on upgrades, and the NODE_PATH host wrapper strategy is likely insufficient for resolving external native addon packages at runtime. |
| packages/cli/src/lib/config.ts | Migrates config from ~/.superset/ to ~/superset/; introduces activeOrg field — but writeConfig omits mode: 0o600, leaving the access token world-readable. |
| packages/cli/src/commands/host/stop/command.ts | Sends SIGTERM but removes the manifest immediately without waiting for the process to exit, creating a window where a concurrent host start could spawn a duplicate. |
| packages/cli/src/lib/host/spawn.ts | New file; cleanly handles port allocation, health-polling, manifest writing, and daemon detach — logic is sound and health check happens before unref. |
| packages/cli/src/lib/host/manifest.ts | Clean manifest read/write/remove helpers; correctly sets 0o700 dir and 0o600 file permissions, and uses process.kill(pid, 0) for liveness check. |
| packages/cli/src/commands/host/start/command.ts | Checks auth + active org before spawn, detects duplicate via manifest liveness check, handles both foreground and daemon modes correctly. |
| .github/workflows/build-cli.yml | Three-platform CI matrix triggered on cli-v* tags; uploads artifacts and creates a draft GitHub Release — straightforward and correct. |
Sequence Diagram
sequenceDiagram
participant User
participant CLI as superset (CLI binary)
participant HostProc as superset-host (Node process)
participant Relay as relay.superset.sh
participant Client as Desktop / Web Client
User->>CLI: superset auth login
CLI->>CLI: deviceAuth() OAuth flow
CLI->>CLI: writeConfig(accessToken, activeOrg)
User->>CLI: superset host start --daemon
CLI->>CLI: readConfig() check auth + activeOrg
CLI->>CLI: readManifest() check for existing process
CLI->>CLI: findFreePort()
CLI->>CLI: randomBytes(32) secret
CLI->>HostProc: spawn superset-host env AUTH_TOKEN SECRET PORT ORG
CLI->>HostProc: pollHealth() GET /trpc/health.check
HostProc-->>CLI: 200 OK
CLI->>CLI: writeManifest(pid, endpoint, authToken, startedAt)
CLI->>CLI: child.unref() detach daemon
CLI-->>User: Host service started
HostProc->>Relay: connect WebSocket tunnel
Client->>Relay: route request to org host
Relay->>HostProc: forward request
HostProc-->>Client: response via relay
User->>CLI: superset host stop
CLI->>CLI: readManifest()
CLI->>HostProc: process.kill(pid, SIGTERM)
CLI->>CLI: removeManifest()
CLI-->>User: Stopped host service
Comments Outside Diff (2)
-
packages/cli/src/lib/config.ts, line 41-43 (link)Config file written with world-readable permissions
writeConfigwritesconfig.jsonwithout specifying amode, so it defaults to0o644(or whateverumaskallows). This file stores theaccessToken— a sensitive credential. In contrast,writeManifest()correctly usesmode: 0o600. The config file should get the same treatment. -
packages/cli/src/lib/config.ts, line 35-38 (link)No error handling for malformed config JSON
readConfig()callsJSON.parsewithout a try/catch. Ifconfig.jsonbecomes corrupted (e.g., due to a partial write), this throws an unhandled exception instead of a friendly error. ComparereadManifest(), which wraps itsJSON.parsein try/catch and returnsnullon failure.
Reviews (1): Last reviewed commit: "feat(cli): ship standalone distribution ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+28
to
+40
| if (isProcessAlive(manifest.pid)) { | ||
| try { | ||
| process.kill(manifest.pid, "SIGTERM"); | ||
| } catch (error) { | ||
| throw new CLIError( | ||
| `Failed to stop host service (pid ${manifest.pid}): ${ | ||
| error instanceof Error ? error.message : "unknown error" | ||
| }`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| removeManifest(organizationId); |
There was a problem hiding this comment.
SIGTERM is sent and then removeManifest is called immediately without waiting for the process to exit. If the host service is slow to shut down (e.g., draining connections to the relay), a concurrent superset host start invocation could spawn a duplicate.
Suggested change
| if (isProcessAlive(manifest.pid)) { | |
| try { | |
| process.kill(manifest.pid, "SIGTERM"); | |
| } catch (error) { | |
| throw new CLIError( | |
| `Failed to stop host service (pid ${manifest.pid}): ${ | |
| error instanceof Error ? error.message : "unknown error" | |
| }`, | |
| ); | |
| } | |
| } | |
| removeManifest(organizationId); | |
| if (isProcessAlive(manifest.pid)) { | |
| try { | |
| process.kill(manifest.pid, "SIGTERM"); | |
| const deadline = Date.now() + 3_000; | |
| while (isProcessAlive(manifest.pid) && Date.now() < deadline) { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| } | |
| } catch (error) { | |
| throw new CLIError( | |
| `Failed to stop host service (pid ${manifest.pid}): ${ | |
| error instanceof Error ? error.message : "unknown error" | |
| }`, | |
| ); | |
| } | |
| } |
Comment on lines
+114
to
+175
| // better-sqlite3 — bun builds from source into build/Release/ | ||
| const sqliteBuildRoot = join( | ||
| bunPackages, | ||
| "better-sqlite3@12.6.2", | ||
| "node_modules", | ||
| "better-sqlite3", | ||
| ); | ||
| const sqliteNode = findFirstExisting([ | ||
| join(sqliteBuildRoot, "prebuilds", target, "better-sqlite3.node"), | ||
| join(sqliteBuildRoot, "build", "Release", "better_sqlite3.node"), | ||
| ]); | ||
| if (!sqliteNode) { | ||
| throw new Error( | ||
| `better-sqlite3 native binary not found for ${target}. Run 'bun install' first.`, | ||
| ); | ||
| } | ||
| cpSync(sqliteNode, join(destDir, "better_sqlite3.node")); | ||
|
|
||
| // node-pty — prebuild or build/Release/ | ||
| const ptyRoot = join( | ||
| bunPackages, | ||
| "node-pty@1.1.0", | ||
| "node_modules", | ||
| "node-pty", | ||
| ); | ||
| const ptyNode = findFirstExisting([ | ||
| join(ptyRoot, "prebuilds", target, "pty.node"), | ||
| join(ptyRoot, "build", "Release", "pty.node"), | ||
| ]); | ||
| if (!ptyNode) { | ||
| throw new Error( | ||
| `node-pty native binary not found for ${target}. Run 'bun install' first.`, | ||
| ); | ||
| } | ||
| cpSync(ptyNode, join(destDir, "pty.node")); | ||
|
|
||
| if (target.startsWith("darwin")) { | ||
| const spawnHelper = findFirstExisting([ | ||
| join(ptyRoot, "prebuilds", target, "spawn-helper"), | ||
| join(ptyRoot, "build", "Release", "spawn-helper"), | ||
| ]); | ||
| if (spawnHelper) { | ||
| cpSync(spawnHelper, join(destDir, "spawn-helper")); | ||
| chmodSync(join(destDir, "spawn-helper"), 0o755); | ||
| } | ||
| } | ||
|
|
||
| // @parcel/watcher — has per-platform packages under bun's .bun/ dir | ||
| const parcelWatcherRoot = join( | ||
| bunPackages, | ||
| `@parcel+watcher-${target}@2.5.6`, | ||
| "node_modules", | ||
| "@parcel", | ||
| `watcher-${target}`, | ||
| ); | ||
| const watcherNode = findFirstExisting([ | ||
| join(parcelWatcherRoot, "watcher.node"), | ||
| ]); | ||
| if (watcherNode) { | ||
| cpSync(watcherNode, join(destDir, "watcher.node")); | ||
| } else { | ||
| console.warn( |
There was a problem hiding this comment.
The paths for better-sqlite3@12.6.2, node-pty@1.1.0, and @parcel+watcher-${target}@2.5.6 are hardcoded. When any of these packages are bumped in package.json, the build script will throw a "not found" error. At minimum, centralise the versions as named constants:
// Keep in sync with packages/host-service/package.json
const BETTER_SQLITE3_VERSION = "12.6.2";
const NODE_PTY_VERSION = "1.1.0";
const PARCEL_WATCHER_VERSION = "2.5.6";
Comment on lines
+210
to
+218
| function writeHostWrapper(binDir: string): void { | ||
| const wrapper = `#!/bin/sh | ||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| export NODE_PATH="$SCRIPT_DIR/../lib/native" | ||
| exec "$SCRIPT_DIR/../lib/node" "$SCRIPT_DIR/../lib/host-service.js" "$@" | ||
| `; | ||
| const wrapperPath = join(binDir, "superset-host"); | ||
| writeFileSync(wrapperPath, wrapper, { mode: 0o755 }); | ||
| chmodSync(wrapperPath, 0o755); |
There was a problem hiding this comment.
NODE_PATH is insufficient for loading external native addon packages at runtime
The host wrapper sets NODE_PATH=lib/native/, but that directory only contains raw .node binaries. require('better-sqlite3') resolves to the full npm package (JS entry point + native binding). Without the JS wrapper, Node will fail with "Cannot find module" on a fresh install.
Working alternatives:
- Copy full
node_modules/better-sqlite3/(JS + native) into the bundle and pointNODE_PATHat that directory. - Use
module.createRequireinsidehost-service.jsto redirect bare specifiers to explicit absolute paths. - Recreate proper
package.json+ JS shims underlib/node_modules/pointing at the.nodebinaries.
If this was verified working with the full dist tarball (not just superset host status, which does not load native addons), please add a note confirming the mechanism.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/org/switch/command.ts (1)
61-78:⚠️ Potential issue | 🟡 MinorPotential local/server state inconsistency on server failure.
If the server-side sync fails (lines 67-78), the local
config.activeOrghas already been persisted (line 63). This creates a split-brain scenario where the CLI thinks the org is switched, but the server session still references the old org.Consider either:
- Performing the server call first, then persisting locally on success
- Rolling back the local config if the server call fails
- Treating server sync as non-fatal (like login does) if local-only operation is acceptable
♻️ Option 1: Server-first approach
if (org.id === currentOrgId) { return { data: { id: org.id, name: org.name }, message: `Already on ${org.name}`, }; } - // Persist locally so host commands use this org - config.activeOrg = { id: org.id, name: org.name, slug: org.slug }; - writeConfig(config); - // Sync server-side active org for tools that read the session const apiUrl = getApiUrl(config); const res = await fetch(`${apiUrl}/api/auth/organization/set-active`, { method: "POST", headers: { "Content-Type": "application/json", Authorization: `Bearer ${config.auth?.accessToken}`, }, body: JSON.stringify({ organizationId: org.id }), }); if (!res.ok) { throw new CLIError(`Failed to switch organization: ${res.status}`); } + // Persist locally after server confirms + config.activeOrg = { id: org.id, name: org.name, slug: org.slug }; + writeConfig(config); + return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/org/switch/command.ts` around lines 61 - 78, The code persists config.activeOrg via writeConfig before calling getApiUrl/fetch to set the server-side active org, causing local/server inconsistency if the POST fails; fix by either (a) calling getApiUrl and awaiting the fetch POST to /api/auth/organization/set-active first and only on res.ok assign config.activeOrg = { id: org.id, name: org.name, slug: org.slug } and call writeConfig(config), or (b) if you want to keep current order, capture the previous value (e.g., const prevActive = config.activeOrg), perform the fetch inside try/catch and on failure restore config.activeOrg = prevActive and call writeConfig(prevConfig) (or log a non-fatal warning instead of throwing CLIError) so local state never diverges from server state; reference functions/variables: config.activeOrg, writeConfig, getApiUrl, fetch, CLIError.
🧹 Nitpick comments (6)
packages/cli/src/commands/auth/login/command.ts (1)
27-46: Consider consolidatingwriteConfigcalls.
writeConfigis called twice: once after settingauth(line 28) and again after settingactiveOrg(line 42). Since the org lookup is non-fatal, this is fine, but you could reduce I/O by deferring the first write.♻️ Optional consolidation
config.auth = { accessToken: result.token }; - writeConfig(config); s.stop("Authorized!"); // The user picked an org during the OAuth consent screen — read it back // and cache locally so `host start` and other commands know which org to use. try { const api = createApiClient(config); const user = await api.user.me.query(); const org = await api.user.myOrganization.query(); p.log.info(`${user.name} (${user.email})`); if (org) { config.activeOrg = { id: org.id, name: org.name, slug: org.slug }; - writeConfig(config); p.log.info(`Organization: ${org.name}`); } else { p.log.warn("No organization selected."); } } catch { // Non-fatal — login succeeded even if whoami fails } + + writeConfig(config); p.outro("Logged in successfully.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/auth/login/command.ts` around lines 27 - 46, Consolidate the two writeConfig calls by removing the early writeConfig(config) after setting config.auth and instead perform a single writeConfig(config) after the org lookup (or in the catch/finally) so the auth token is persisted even if org resolution fails; update the block around createApiClient(config), api.user.me.query(), and api.user.myOrganization.query() to set config.activeOrg when present, then call writeConfig(config) once and keep the same logging (p.log.info/p.log.warn) and non-fatal behavior on org lookup failure.packages/host-service/build.ts (1)
1-6: Minor: Update docstring to include@parcel/watcher.The docstring mentions
better-sqlite3andnode-ptyas external native addons, but the externals array on line 20 also includes@parcel/watcher. Consider updating the documentation to match.📝 Suggested documentation update
/** * Bundles the host-service entry point into a single JS file that can be * executed by a standalone Node.js runtime. Native addons (better-sqlite3, - * node-pty) are marked external and must be resolved at runtime from + * node-pty, `@parcel/watcher`) are marked external and must be resolved at runtime from * lib/native/ in the distribution bundle. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/build.ts` around lines 1 - 6, The top-of-file docstring in build.ts lists native addons but omits `@parcel/watcher` which is marked external in the externals array; update the comment block (the header/docstring above the module) to also mention `@parcel/watcher` alongside better-sqlite3 and node-pty so the documentation matches the externals configuration.packages/cli/src/lib/host/spawn.ts (1)
29-43: Minor: inherent TOCTOU race in port acquisition.The
findFreePortpattern (bind to port 0, get assigned port, close, return) has a small window where another process could bind the same port before the host-service starts. This is a known limitation of this approach and is generally acceptable for development/CLI tools, but worth noting.An alternative would be to pass port 0 to the host-service and have it report the actual bound port back, but that would require more coordination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/host/spawn.ts` around lines 29 - 43, The findFreePort function has a TOCTOU race: it binds to port 0, closes, and returns the port which can be claimed by another process before the host-service starts; to fix, either modify the host/service startup to accept port 0 and report the actual bound port back (so you avoid pre-allocating a port), or if that coordination isn't feasible, add a clear comment near findFreePort explaining the race and the acceptable risk for CLI/dev usage and optionally retry logic in spawnHost (e.g., retry binding a few times on failure) to mitigate the window; reference the findFreePort function and the host-service startup code path that consumes its result when applying the change.packages/cli/scripts/build-dist.ts (2)
61-73: Theexechelper lacks timeout handling.Long-running or hung commands (e.g.,
curlon a slow network,taron large archives) could block indefinitely. Consider adding a timeout option or usingAbortControllerwith the spawn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 61 - 73, The exec helper (async function exec) needs timeout handling to avoid hanging child processes; modify exec to accept an optional timeoutMs (or an AbortSignal) parameter, start a timer when spawning the child, and if the timer fires call child.kill() and reject with a clear timeout Error; also clear the timer on successful exit, remove/cleanup listeners, and ensure the "error" and "exit" handlers check whether the rejection was due to timeout so you don't double-reject. Use the existing spawn call and error message format (include cmd and args) when rejecting for timeout.
110-179: Hardcoded package versions will require manual updates.The native addon paths include hardcoded versions (
better-sqlite3@12.6.2,node-pty@1.1.0,@parcel+watcher-${target}@2.5.6). When these dependencies are upgraded, the build script will silently fail to find the binaries.Consider extracting these versions to constants at the top of the file, or dynamically resolving them from the lockfile/node_modules structure.
Extract versions to constants for easier maintenance
const VALID_TARGETS: Target[] = ["darwin-arm64", "darwin-x64", "linux-x64"]; const NODE_VERSION = "22.13.0"; +const BETTER_SQLITE3_VERSION = "12.6.2"; +const NODE_PTY_VERSION = "1.1.0"; +const PARCEL_WATCHER_VERSION = "2.5.6"; // ... later in copyNativeAddons: - const sqliteBuildRoot = join( - bunPackages, - "better-sqlite3@12.6.2", + const sqliteBuildRoot = join( + bunPackages, + `better-sqlite3@${BETTER_SQLITE3_VERSION}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 110 - 179, The copyNativeAddons function hardcodes package versions (e.g., "better-sqlite3@12.6.2", "node-pty@1.1.0", `@parcel+watcher-${target}@2.5.6`) which will break when deps are upgraded; update copyNativeAddons to either read those versions from constants defined at the top of the file (e.g., SQLITE_PKG, PTY_PKG, PARCEL_WATCHER_PKG) and use those constants in the join paths, or implement dynamic resolution that searches the bunPackages directory for a matching package folder prefix (e.g., find a directory starting with "better-sqlite3@", "node-pty@", or "@parcel+watcher-" + target) and use the discovered folder name in place of the hardcoded one; ensure the function falls back to a clear error/warning if no matching folder is found and update references to sqliteBuildRoot, ptyRoot, and parcelWatcherRoot accordingly.packages/cli/src/lib/host/manifest.ts (1)
53-54: Consider validating the parsed manifest structure.The JSON is parsed and cast directly to
HostServiceManifestwithout schema validation. If the file is corrupted or manually edited, accessingmanifest.pidormanifest.authTokenin consuming code could produce unexpected values.For a CLI tool this is low-risk, but a lightweight validation (e.g., checking required fields exist and have correct types) could prevent confusing runtime errors.
Example validation approach
export function readManifest( organizationId: string, ): HostServiceManifest | null { const path = manifestPath(organizationId); if (!existsSync(path)) return null; try { - return JSON.parse(readFileSync(path, "utf-8")) as HostServiceManifest; + const data = JSON.parse(readFileSync(path, "utf-8")); + if ( + typeof data.pid === "number" && + typeof data.endpoint === "string" && + typeof data.authToken === "string" && + typeof data.startedAt === "number" && + typeof data.organizationId === "string" + ) { + return data as HostServiceManifest; + } + return null; } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/host/manifest.ts` around lines 53 - 54, The JSON parse in the manifest loader returns a value cast to HostServiceManifest without validation; add lightweight schema checks after JSON.parse(readFileSync(path, "utf-8")) to ensure required fields (e.g., pid is a number, authToken is a non-empty string, and any other HostServiceManifest properties) exist and have correct types, and throw a clear error (or return a well-typed result) if validation fails. Implement this as a small helper like validateHostServiceManifest(manifest) or inline checks in the same function, include the file path in the error message for context, and avoid swallowing the original parse error so consumers don't receive malformed manifest.pid or manifest.authToken values.
🤖 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/build-cli.yml:
- Around line 17-18: The workflow uses an unavailable runner "os: macos-13" for
the job that targets "darwin-x64"; update the runner to a supported macOS runner
(e.g., replace os: macos-13 with macos-15-intel for Intel x86_64 or
macos-14-large / macos-latest-large depending on required VM size) so the job
targeting "darwin-x64" runs on a valid GitHub-hosted macOS runner.
In `@packages/cli/src/lib/config.ts`:
- Around line 25-27: The code introduced SUPERSET_HOME_DIR, CONFIG_PATH and
DEVICE_PATH but forgot to migrate existing files from the old `~/.superset`
location, so update startup/migration logic (before readConfig() is used) to
detect the oldDir = join(homedir(), ".superset") and if oldDir exists and
contains config.json or device.json, copy or move those files into
SUPERSET_HOME_DIR (creating it if needed), preserving permissions and handling
errors; ensure readConfig() then reads from CONFIG_PATH/DEVICE_PATH and that
migration runs only once (skip if target files already exist) and logs any
failures.
---
Outside diff comments:
In `@packages/cli/src/commands/org/switch/command.ts`:
- Around line 61-78: The code persists config.activeOrg via writeConfig before
calling getApiUrl/fetch to set the server-side active org, causing local/server
inconsistency if the POST fails; fix by either (a) calling getApiUrl and
awaiting the fetch POST to /api/auth/organization/set-active first and only on
res.ok assign config.activeOrg = { id: org.id, name: org.name, slug: org.slug }
and call writeConfig(config), or (b) if you want to keep current order, capture
the previous value (e.g., const prevActive = config.activeOrg), perform the
fetch inside try/catch and on failure restore config.activeOrg = prevActive and
call writeConfig(prevConfig) (or log a non-fatal warning instead of throwing
CLIError) so local state never diverges from server state; reference
functions/variables: config.activeOrg, writeConfig, getApiUrl, fetch, CLIError.
---
Nitpick comments:
In `@packages/cli/scripts/build-dist.ts`:
- Around line 61-73: The exec helper (async function exec) needs timeout
handling to avoid hanging child processes; modify exec to accept an optional
timeoutMs (or an AbortSignal) parameter, start a timer when spawning the child,
and if the timer fires call child.kill() and reject with a clear timeout Error;
also clear the timer on successful exit, remove/cleanup listeners, and ensure
the "error" and "exit" handlers check whether the rejection was due to timeout
so you don't double-reject. Use the existing spawn call and error message format
(include cmd and args) when rejecting for timeout.
- Around line 110-179: The copyNativeAddons function hardcodes package versions
(e.g., "better-sqlite3@12.6.2", "node-pty@1.1.0",
`@parcel+watcher-${target}@2.5.6`) which will break when deps are upgraded;
update copyNativeAddons to either read those versions from constants defined at
the top of the file (e.g., SQLITE_PKG, PTY_PKG, PARCEL_WATCHER_PKG) and use
those constants in the join paths, or implement dynamic resolution that searches
the bunPackages directory for a matching package folder prefix (e.g., find a
directory starting with "better-sqlite3@", "node-pty@", or "@parcel+watcher-" +
target) and use the discovered folder name in place of the hardcoded one; ensure
the function falls back to a clear error/warning if no matching folder is found
and update references to sqliteBuildRoot, ptyRoot, and parcelWatcherRoot
accordingly.
In `@packages/cli/src/commands/auth/login/command.ts`:
- Around line 27-46: Consolidate the two writeConfig calls by removing the early
writeConfig(config) after setting config.auth and instead perform a single
writeConfig(config) after the org lookup (or in the catch/finally) so the auth
token is persisted even if org resolution fails; update the block around
createApiClient(config), api.user.me.query(), and
api.user.myOrganization.query() to set config.activeOrg when present, then call
writeConfig(config) once and keep the same logging (p.log.info/p.log.warn) and
non-fatal behavior on org lookup failure.
In `@packages/cli/src/lib/host/manifest.ts`:
- Around line 53-54: The JSON parse in the manifest loader returns a value cast
to HostServiceManifest without validation; add lightweight schema checks after
JSON.parse(readFileSync(path, "utf-8")) to ensure required fields (e.g., pid is
a number, authToken is a non-empty string, and any other HostServiceManifest
properties) exist and have correct types, and throw a clear error (or return a
well-typed result) if validation fails. Implement this as a small helper like
validateHostServiceManifest(manifest) or inline checks in the same function,
include the file path in the error message for context, and avoid swallowing the
original parse error so consumers don't receive malformed manifest.pid or
manifest.authToken values.
In `@packages/cli/src/lib/host/spawn.ts`:
- Around line 29-43: The findFreePort function has a TOCTOU race: it binds to
port 0, closes, and returns the port which can be claimed by another process
before the host-service starts; to fix, either modify the host/service startup
to accept port 0 and report the actual bound port back (so you avoid
pre-allocating a port), or if that coordination isn't feasible, add a clear
comment near findFreePort explaining the race and the acceptable risk for
CLI/dev usage and optionally retry logic in spawnHost (e.g., retry binding a few
times on failure) to mitigate the window; reference the findFreePort function
and the host-service startup code path that consumes its result when applying
the change.
In `@packages/host-service/build.ts`:
- Around line 1-6: The top-of-file docstring in build.ts lists native addons but
omits `@parcel/watcher` which is marked external in the externals array; update
the comment block (the header/docstring above the module) to also mention
`@parcel/watcher` alongside better-sqlite3 and node-pty so the documentation
matches the externals configuration.
🪄 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: f207036a-bfc1-4530-bd7d-d6c85aa3e350
📒 Files selected for processing (14)
.github/workflows/build-cli.ymlpackages/cli/package.jsonpackages/cli/scripts/build-dist.tspackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/host/start/command.tspackages/cli/src/commands/host/status/command.tspackages/cli/src/commands/host/stop/command.tspackages/cli/src/commands/org/switch/command.tspackages/cli/src/lib/config.tspackages/cli/src/lib/env.tspackages/cli/src/lib/host/manifest.tspackages/cli/src/lib/host/spawn.tspackages/host-service/build.tspackages/host-service/package.json
Comment on lines
+17
to
+18
| - os: macos-13 | ||
| target: darwin-x64 |
Contributor
There was a problem hiding this comment.
**Verify macos-13 runner availability.**The macos-13 runner was retired by December 4th, 2025. For x86_64 (Intel) architecture, GitHub recommends using macos-15-intel or macos-14-large/macos-latest-large.
🔧 Proposed fix
include:
- os: macos-14
target: darwin-arm64
- - os: macos-13
+ - os: macos-15-intel
target: darwin-x64
- os: ubuntu-latest
target: linux-x64📝 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
| - os: macos-13 | |
| target: darwin-x64 | |
| - os: macos-15-intel | |
| target: darwin-x64 |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 17-17: label "macos-13" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-intel", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-cli.yml around lines 17 - 18, The workflow uses an
unavailable runner "os: macos-13" for the job that targets "darwin-x64"; update
the runner to a supported macOS runner (e.g., replace os: macos-13 with
macos-15-intel for Intel x86_64 or macos-14-large / macos-latest-large depending
on required VM size) so the job targeting "darwin-x64" runs on a valid
GitHub-hosted macOS runner.
Comment on lines
+25
to
+27
| export const SUPERSET_HOME_DIR = join(homedir(), "superset"); | ||
| const CONFIG_PATH = join(SUPERSET_HOME_DIR, "config.json"); | ||
| const DEVICE_PATH = join(SUPERSET_HOME_DIR, "device.json"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any migration logic elsewhere in the codebase
rg -n "\.superset" --type=ts -g '!node_modules'Repository: superset-sh/superset
Length of output: 26661
🏁 Script executed:
cat -n packages/cli/src/lib/config.ts | head -80Repository: superset-sh/superset
Length of output: 1753
🏁 Script executed:
# Check for migration logic and other references to SUPERSET_HOME_DIR
rg "SUPERSET_HOME_DIR" packages/cli -A 3 -B 3Repository: superset-sh/superset
Length of output: 1705
🏁 Script executed:
# Check for any migration logic related to config paths
rg "\.superset" packages/cli -i -A 2 -B 2Repository: superset-sh/superset
Length of output: 5511
🏁 Script executed:
# Check auth.ts for migration logic
cat -n packages/cli/src/lib/auth.ts | head -150Repository: superset-sh/superset
Length of output: 5023
🏁 Script executed:
# Continue reading auth.ts
cat -n packages/cli/src/lib/auth.ts | tail -30Repository: superset-sh/superset
Length of output: 965
🏁 Script executed:
# Check if there's any initialization or setup code that might handle migration
rg "readConfig|writeConfig" packages/cli/src -A 3 -B 3 | head -80Repository: superset-sh/superset
Length of output: 5284
🏁 Script executed:
# Check if there are tests that verify migration or path handling
find packages/cli -name "*.test.ts" -o -name "*.spec.ts" | xargs rg "config|\.superset|superset" 2>/dev/null | head -50Repository: superset-sh/superset
Length of output: 5211
🏁 Script executed:
# Look for config test files specifically
find packages/cli -type f \( -name "*.test.ts" -o -name "*.spec.ts" \)Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any initialization or setup code in the CLI entry point
find packages/cli/src -name "index.ts" -o -name "main.ts" -o -name "cli.ts" | xargs cat -n 2>/dev/nullRepository: superset-sh/superset
Length of output: 3531
Breaking change: No migration from ~/.superset to ~/superset leaves existing users without access to credentials.
In line 25, SUPERSET_HOME_DIR uses ~/superset (without dot), but existing users have configs at ~/.superset/config.json. The readConfig() function returns an empty object if the new path doesn't exist—it doesn't check for or migrate the old path. Users will lose their authentication token and active org settings after updating.
Add migration logic on first startup to check for the old ~/.superset directory and move or copy config.json and device.json to ~/superset if they exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/config.ts` around lines 25 - 27, The code introduced
SUPERSET_HOME_DIR, CONFIG_PATH and DEVICE_PATH but forgot to migrate existing
files from the old `~/.superset` location, so update startup/migration logic
(before readConfig() is used) to detect the oldDir = join(homedir(),
".superset") and if oldDir exists and contains config.json or device.json, copy
or move those files into SUPERSET_HOME_DIR (creating it if needed), preserving
permissions and handling errors; ensure readConfig() then reads from
CONFIG_PATH/DEVICE_PATH and that migration runs only once (skip if target files
already exist) and logs any failures.
Addresses PR review findings:
- P1: NODE_PATH was pointing at lib/native/ which only had raw .node files.
Node.js can't resolve `require('better-sqlite3')` without the full package
(JS wrapper + bindings resolution). Fix: copy the entire npm package under
lib/node_modules/ and walk transitive deps (picomatch, is-glob, etc.).
Verified locally: better-sqlite3, node-pty, and @parcel/watcher all load.
- P1: host stop removed the manifest before the process exited, allowing
a concurrent host start to race ahead. Fix: wait up to 10s for the process
to exit after SIGTERM, escalate to SIGKILL, then remove the manifest.
- P2: Hardcoded package versions (better-sqlite3@12.6.2, etc.) replaced with
dependency tree walking — reads each package's package.json and recursively
copies transitive runtime deps. No version pins to maintain.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/commands/host/stop/command.ts">
<violation number="1" location="packages/cli/src/commands/host/stop/command.ts:51">
P2: Do not silently swallow SIGKILL failures; this can report a successful stop while the process is still running.
(Based on your team's feedback about handling async/errors explicitly and avoiding silent empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/scripts/build-dist.ts">
<violation number="1" location="packages/cli/scripts/build-dist.ts:194">
P1: Native addon packaging ignores `--target` and copies host-resolved binaries, which can generate broken cross-target distributions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const hostServiceDir = join(repoRoot, "packages", "host-service"); | ||
| for (const pkg of NATIVE_PACKAGES) { | ||
| console.log(`[build-dist] copying ${pkg} (+ deps)`); | ||
| copyPackageWithDeps(pkg, hostServiceDir, repoRoot, destModules, copied); |
Contributor
There was a problem hiding this comment.
P1: Native addon packaging ignores --target and copies host-resolved binaries, which can generate broken cross-target distributions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/scripts/build-dist.ts, line 194:
<comment>Native addon packaging ignores `--target` and copies host-resolved binaries, which can generate broken cross-target distributions.</comment>
<file context>
@@ -100,82 +114,90 @@ async function downloadAndExtractNode(
+ const hostServiceDir = join(repoRoot, "packages", "host-service");
+ for (const pkg of NATIVE_PACKAGES) {
+ console.log(`[build-dist] copying ${pkg} (+ deps)`);
+ copyPackageWithDeps(pkg, hostServiceDir, repoRoot, destModules, copied);
}
+
</file context>
| // Escalate to SIGKILL if it refuses to exit | ||
| try { | ||
| process.kill(manifest.pid, "SIGKILL"); | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow SIGKILL failures; this can report a successful stop while the process is still running.
(Based on your team's feedback about handling async/errors explicitly and avoiding silent empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/host/stop/command.ts, line 51:
<comment>Do not silently swallow SIGKILL failures; this can report a successful stop while the process is still running.
(Based on your team's feedback about handling async/errors explicitly and avoiding silent empty catch blocks.) </comment>
<file context>
@@ -35,6 +35,21 @@ export default command({
+ // Escalate to SIGKILL if it refuses to exit
+ try {
+ process.kill(manifest.pid, "SIGKILL");
+ } catch {}
+ }
}
</file context>
Suggested change
| } catch {} | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException)?.code !== "ESRCH") { | |
| throw new CLIError( | |
| `Failed to force-stop host service (pid ${manifest.pid}): ${error instanceof Error ? error.message : "unknown error"}`, | |
| ); | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/scripts/build-dist.ts (1)
203-205: Avoid duplicating the baked-in endpoint defaults.These literals now have to stay in sync with
packages/cli/src/lib/env.ts. Pull them from a shared constant so a future endpoint change cannot make local-dev and compiled binaries disagree silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 203 - 205, The buildCli function currently hardcodes relayUrl and cloudApiUrl defaults (variables relayUrl and cloudApiUrl); replace these duplicated literals by importing and using the shared defaults exported from the env module (e.g., DEFAULT_RELAY_URL and DEFAULT_CLOUD_API_URL or the module's exported names) so the compiled binary uses the same source-of-truth as the rest of the codebase; update the top of build-dist.ts to import the constants and fall back to them instead of the inline string literals when computing relayUrl/cloudApiUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/scripts/build-dist.ts`:
- Around line 100-107: The script downloads a Node archive via
nodeDownloadUrl(target) into archivePath and immediately extracts it; add a
verification step that fetches the official checksum or signature for that Node
release (or uses a bundled expected checksum lookup keyed by target), compute
the archive's digest (e.g., sha256) and compare it to the expected value, and
only proceed to extracting (the existing tar extraction for extractedPath) if
the checksum/signature verification passes; update the logic around
nodeDownloadUrl(target), archivePath and extractedPath to fail fast with a clear
error when verification fails and to skip extraction when verification is
missing or invalid.
- Around line 185-195: copyNativePackages currently ignores the target and will
copy whatever native binaries exist on the build host; add a validation guard at
the start of copyNativePackages to prevent cross-target mismatches by comparing
the requested target's platform/arch with the build host and failing fast (or
skipping native-packages) rather than copying incompatible .node files.
Specifically, in copyNativePackages (which calls copyPackageWithDeps for
NATIVE_PACKAGES from hostServiceDir into destModules) check target.platform and
target.arch against the host (e.g., process.platform/process.arch or equivalent)
and throw a clear error or abort the build when they differ; if you need
cross-target support later, implement per-package native artifact selection
based on the target before invoking copyPackageWithDeps instead of
unconditionally copying.
---
Nitpick comments:
In `@packages/cli/scripts/build-dist.ts`:
- Around line 203-205: The buildCli function currently hardcodes relayUrl and
cloudApiUrl defaults (variables relayUrl and cloudApiUrl); replace these
duplicated literals by importing and using the shared defaults exported from the
env module (e.g., DEFAULT_RELAY_URL and DEFAULT_CLOUD_API_URL or the module's
exported names) so the compiled binary uses the same source-of-truth as the rest
of the codebase; update the top of build-dist.ts to import the constants and
fall back to them instead of the inline string literals when computing
relayUrl/cloudApiUrl.
🪄 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: 8f3043ca-d1bd-4f7b-9dd1-736a95dc013f
📒 Files selected for processing (2)
packages/cli/scripts/build-dist.tspackages/cli/src/commands/host/stop/command.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/host/stop/command.ts
Comment on lines
+100
to
+107
| if (!existsSync(archivePath)) { | ||
| console.log(`[build-dist] downloading ${nodeDownloadUrl(target)}`); | ||
| await exec("curl", ["-fsSL", "-o", archivePath, nodeDownloadUrl(target)]); | ||
| } | ||
|
|
||
| if (!existsSync(extractedPath)) { | ||
| console.log(`[build-dist] extracting Node.js for ${target}`); | ||
| await exec("tar", ["-xzf", archivePath, "-C", cacheDir]); |
Contributor
There was a problem hiding this comment.
Verify the downloaded Node runtime before extracting it.
This immediately packages an executable fetched over the network. Without a checksum or signature check, a bad mirror, proxy, or partial cache entry can poison the release artifact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/scripts/build-dist.ts` around lines 100 - 107, The script
downloads a Node archive via nodeDownloadUrl(target) into archivePath and
immediately extracts it; add a verification step that fetches the official
checksum or signature for that Node release (or uses a bundled expected checksum
lookup keyed by target), compute the archive's digest (e.g., sha256) and compare
it to the expected value, and only proceed to extracting (the existing tar
extraction for extractedPath) if the checksum/signature verification passes;
update the logic around nodeDownloadUrl(target), archivePath and extractedPath
to fail fast with a clear error when verification fails and to skip extraction
when verification is missing or invalid.
Comment on lines
+185
to
+195
| function copyNativePackages(target: Target, libDir: string): void { | ||
| const repoRoot = resolve(import.meta.dir, "../../.."); | ||
| const destModules = join(libDir, "node_modules"); | ||
| mkdirSync(destModules, { recursive: true }); | ||
| const copied = new Set<string>(); | ||
|
|
||
| const hostServiceDir = join(repoRoot, "packages", "host-service"); | ||
| for (const pkg of NATIVE_PACKAGES) { | ||
| console.log(`[build-dist] copying ${pkg} (+ deps)`); | ||
| copyPackageWithDeps(pkg, hostServiceDir, repoRoot, destModules, copied); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Target handling in packages/cli/scripts/build-dist.ts:\n'
rg -n '\btarget\b|process\.platform|process\.arch' packages/cli/scripts/build-dist.ts
printf '\nNative addon copy block:\n'
sed -n '185,201p' packages/cli/scripts/build-dist.ts
printf '\nRuntime native deps in packages/host-service/package.json:\n'
sed -n '20,40p' packages/host-service/package.json | rg -n 'better-sqlite3|node-pty|@parcel/watcher'Repository: superset-sh/superset
Length of output: 2834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find NATIVE_PACKAGES definition
printf '=== NATIVE_PACKAGES definition ===\n'
rg -n 'NATIVE_PACKAGES' packages/cli/scripts/build-dist.ts -A 5
# Check actual runtime dependencies in host-service package.json
printf '\n=== host-service runtime deps (checking all dependencies) ===\n'
cat packages/host-service/package.json | jq '.dependencies, .optionalDependencies' 2>/dev/null || echo "Checking with grep instead:"
grep -A 30 '"dependencies"' packages/host-service/package.json | head -40
# Check if copyPackageWithDeps validates anything
printf '\n=== copyPackageWithDeps implementation ===\n'
rg -n 'function copyPackageWithDeps|const copyPackageWithDeps' packages/cli/scripts/build-dist.ts -A 15
# Verify target patterns
printf '\n=== VALID_TARGETS definition ===\n'
rg -n 'VALID_TARGETS' packages/cli/scripts/build-dist.ts -A 5Repository: superset-sh/superset
Length of output: 3028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full package.json for `@parcel/watcher`
printf '=== Checking for `@parcel/watcher` in host-service ===\n'
grep -n '@parcel/watcher' packages/host-service/package.json || echo "Not found in direct grep"
# Also check devDependencies and optionalDependencies sections more carefully
printf '\n=== Full host-service package.json sections ===\n'
cat packages/host-service/package.json | jq 'keys'
# Validate proposed guard logic - check all three VALID_TARGETS
printf '\n=== Validating target-to-platform mapping ===\n'
cat > /tmp/validate_targets.js << 'EOF'
const VALID_TARGETS = ["darwin-arm64", "darwin-x64", "linux-x64"];
for (const target of VALID_TARGETS) {
const expectedPlatform = target.startsWith("darwin") ? "darwin" : "linux";
const expectedArch = target.endsWith("arm64") ? "arm64" : "x64";
console.log(`${target} -> platform: ${expectedPlatform}, arch: ${expectedArch}`);
}
EOF
node /tmp/validate_targets.js
# Check if there are any cross-platform build guards elsewhere in the file
printf '\n=== Checking for existing platform/arch guards ===\n'
rg -n 'process\.platform|process\.arch' packages/cli/scripts/build-dist.tsRepository: superset-sh/superset
Length of output: 531
Add platform/arch validation to prevent cross-target native addon mismatches.
copyNativePackages() receives a target parameter but ignores it, copying whatever native binaries (better-sqlite3, node-pty, @parcel/watcher) are installed on the build machine. This causes cross-target builds to produce tarballs with mismatched .node files—for example, building for linux-x64 on a macOS machine will ship Darwin-compiled native modules to a Linux target.
Add a guard at the function start:
Validation guard
function copyNativePackages(target: Target, libDir: string): void {
+ const expectedPlatform = target.startsWith("darwin") ? "darwin" : "linux";
+ const expectedArch = target.endsWith("arm64") ? "arm64" : "x64";
+ if (process.platform !== expectedPlatform || process.arch !== expectedArch) {
+ throw new Error(
+ `Cross-target native addon packaging is unsupported for ${target}; build on ${expectedPlatform}-${expectedArch} or fetch target-specific packages.`,
+ );
+ }
const repoRoot = resolve(import.meta.dir, "../../..");📝 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
| function copyNativePackages(target: Target, libDir: string): void { | |
| const repoRoot = resolve(import.meta.dir, "../../.."); | |
| const destModules = join(libDir, "node_modules"); | |
| mkdirSync(destModules, { recursive: true }); | |
| const copied = new Set<string>(); | |
| const hostServiceDir = join(repoRoot, "packages", "host-service"); | |
| for (const pkg of NATIVE_PACKAGES) { | |
| console.log(`[build-dist] copying ${pkg} (+ deps)`); | |
| copyPackageWithDeps(pkg, hostServiceDir, repoRoot, destModules, copied); | |
| } | |
| function copyNativePackages(target: Target, libDir: string): void { | |
| const expectedPlatform = target.startsWith("darwin") ? "darwin" : "linux"; | |
| const expectedArch = target.endsWith("arm64") ? "arm64" : "x64"; | |
| if (process.platform !== expectedPlatform || process.arch !== expectedArch) { | |
| throw new Error( | |
| `Cross-target native addon packaging is unsupported for ${target}; build on ${expectedPlatform}-${expectedArch} or fetch target-specific packages.`, | |
| ); | |
| } | |
| const repoRoot = resolve(import.meta.dir, "../../.."); | |
| const destModules = join(libDir, "node_modules"); | |
| mkdirSync(destModules, { recursive: true }); | |
| const copied = new Set<string>(); | |
| const hostServiceDir = join(repoRoot, "packages", "host-service"); | |
| for (const pkg of NATIVE_PACKAGES) { | |
| console.log(`[build-dist] copying ${pkg} (+ deps)`); | |
| copyPackageWithDeps(pkg, hostServiceDir, repoRoot, destModules, copied); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/scripts/build-dist.ts` around lines 185 - 195,
copyNativePackages currently ignores the target and will copy whatever native
binaries exist on the build host; add a validation guard at the start of
copyNativePackages to prevent cross-target mismatches by comparing the requested
target's platform/arch with the build host and failing fast (or skipping
native-packages) rather than copying incompatible .node files. Specifically, in
copyNativePackages (which calls copyPackageWithDeps for NATIVE_PACKAGES from
hostServiceDir into destModules) check target.platform and target.arch against
the host (e.g., process.platform/process.arch or equivalent) and throw a clear
error or abort the build when they differ; if you need cross-target support
later, implement per-package native artifact selection based on the target
before invoking copyPackageWithDeps instead of unconditionally copying.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/cli/scripts/build-dist.ts (2)
185-195:⚠️ Potential issue | 🟠 MajorFail fast when the build host does not match the requested native target.
copyNativePackages()always packages the builder's installed.nodebinaries. That makes cross-target builds silently wrong, e.g. copying Darwin addons into alinux-x64tarball or arm64 addons intodarwin-x64.Suggested guard
-function copyNativePackages(libDir: string): void { +function copyNativePackages(target: Target, libDir: string): void { + const expectedPlatform = target.startsWith("darwin") ? "darwin" : "linux"; + const expectedArch = target.endsWith("arm64") ? "arm64" : "x64"; + if (process.platform !== expectedPlatform || process.arch !== expectedArch) { + throw new Error( + `Cross-target native addon packaging is unsupported for ${target}; build on ${expectedPlatform}-${expectedArch} or fetch target-specific native packages.`, + ); + } + const repoRoot = resolve(import.meta.dir, "../../.."); const destModules = join(libDir, "node_modules"); mkdirSync(destModules, { recursive: true }); const copied = new Set<string>(); ... - copyNativePackages(join(stagingRoot, "lib")); + copyNativePackages(target, join(stagingRoot, "lib"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 185 - 195, copyNativePackages currently always copies the builder's installed .node binaries which allows cross-target builds to silently include the wrong native addons; add a fail-fast guard at the start of copyNativePackages that compares the requested native target (e.g. the build config / env var used by the packaging step, such as a NATIVE_TARGET or buildConfig.nativeTarget) with the builder runtime platform/arch (process.platform/process.arch or the platform/arch of hostServiceDir/node_modules) and throw a clear error if they differ; implement the check inside copyNativePackages before iterating NATIVE_PACKAGES and surface the mismatch via an error (so copyPackageWithDeps is never called when targets differ).
100-107:⚠️ Potential issue | 🟠 MajorVerify the downloaded Node archive before extracting it.
Line 102 downloads an executable archive and Line 107 extracts it with no integrity check. A bad mirror, proxy, or stale cache entry here can poison every released tarball.
Suggested hardening
+import { createHash } from "node:crypto"; ... async function downloadAndExtractNode( target: Target, destDir: string, ): Promise<string> { const cacheDir = join(homedir(), ".superset-build-cache"); if (!existsSync(cacheDir)) mkdirSync(cacheDir, { recursive: true }); const archiveName = nodeArchiveName(target); const archivePath = join(cacheDir, `${archiveName}.tar.gz`); const extractedPath = join(cacheDir, archiveName); + const shasumsPath = join(cacheDir, `SHASUMS256-${NODE_VERSION}.txt`); if (!existsSync(archivePath)) { console.log(`[build-dist] downloading ${nodeDownloadUrl(target)}`); await exec("curl", ["-fsSL", "-o", archivePath, nodeDownloadUrl(target)]); } + + if (!existsSync(shasumsPath)) { + await exec("curl", [ + "-fsSL", + "-o", + shasumsPath, + `https://nodejs.org/dist/v${NODE_VERSION}/SHASUMS256.txt`, + ]); + } + + const expectedLine = readFileSync(shasumsPath, "utf-8") + .split("\n") + .find((line) => line.endsWith(` ${archiveName}.tar.gz`)); + if (!expectedLine) { + throw new Error(`Missing checksum for ${archiveName}.tar.gz`); + } + + const expectedSha = expectedLine.split(/\s+/)[0]; + const actualSha = createHash("sha256") + .update(readFileSync(archivePath)) + .digest("hex"); + if (actualSha !== expectedSha) { + throw new Error(`Checksum mismatch for ${archiveName}.tar.gz`); + } if (!existsSync(extractedPath)) {
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cli/scripts/build-dist.ts`:
- Around line 185-195: copyNativePackages currently always copies the builder's
installed .node binaries which allows cross-target builds to silently include
the wrong native addons; add a fail-fast guard at the start of
copyNativePackages that compares the requested native target (e.g. the build
config / env var used by the packaging step, such as a NATIVE_TARGET or
buildConfig.nativeTarget) with the builder runtime platform/arch
(process.platform/process.arch or the platform/arch of
hostServiceDir/node_modules) and throw a clear error if they differ; implement
the check inside copyNativePackages before iterating NATIVE_PACKAGES and surface
the mismatch via an error (so copyPackageWithDeps is never called when targets
differ).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78805453-a44d-4969-b759-69ef88abeede
📒 Files selected for processing (1)
packages/cli/scripts/build-dist.ts
7 tasks
4 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 10, 2026
…et-sh#3298) * feat(cli): ship standalone distribution with embedded host-service Bundles the CLI binary + Node.js runtime + host-service + native addons into a single tarball per platform. Users run `superset auth login` and `superset host start --daemon` to expose their machine via the relay. - Implement host start/stop/status commands (were stubs) - Manifest format matches desktop app (shared at ~/superset/host/<orgId>/) - Migrate CLI home from ~/.superset to ~/superset for desktop compat - Store activeOrg locally in config.json (set during OAuth consent) - Host-service Bun build targeting Node.js (native addons external) - Distribution build script: CLI binary + Node + host-service.js + native - CI matrix workflow (macos arm64, macos x64, ubuntu x64) - Bake RELAY_URL and CLOUD_API_URL into the binary at build time * fix(cli): copy full native addon packages with transitive deps Addresses PR review findings: - P1: NODE_PATH was pointing at lib/native/ which only had raw .node files. Node.js can't resolve `require('better-sqlite3')` without the full package (JS wrapper + bindings resolution). Fix: copy the entire npm package under lib/node_modules/ and walk transitive deps (picomatch, is-glob, etc.). Verified locally: better-sqlite3, node-pty, and @parcel/watcher all load. - P1: host stop removed the manifest before the process exited, allowing a concurrent host start to race ahead. Fix: wait up to 10s for the process to exit after SIGTERM, escalate to SIGKILL, then remove the manifest. - P2: Hardcoded package versions (better-sqlite3@12.6.2, etc.) replaced with dependency tree walking — reads each package's package.json and recursively copies transitive runtime deps. No version pins to maintain. * fix: remove unused target param from copyNativePackages
6 tasks
6 tasks
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
Ships the CLI as a standalone tarball per platform so non-desktop users can run their own host-service that connects to the relay at `relay.superset.sh`. A developer on a remote server, CI box, or cloud VM can now expose their machine to any Superset client.
The relay infrastructure already exists (#3250). This PR makes the CLI actually useful by bundling the host-service + Node runtime + native addons into the distribution.
What's built
/.superset/` → `/superset/` for desktop compat (`SUPERSET_HOME_DIR`)Bundle layout
```
~/superset/
bin/
superset # Bun-compiled CLI
superset-host # Shell wrapper → exec ../lib/node ../lib/host-service.js
lib/
node # Standalone Node.js 22
host-service.js # Bundled entry (external: better-sqlite3, node-pty, @parcel/watcher)
native/
better_sqlite3.node
pty.node
spawn-helper # darwin only
watcher.node # @parcel/watcher
share/
migrations/ # Drizzle SQL files
config.json # Auth token, activeOrg
host// # Per-org host DB + manifest
```
User flow
```bash
tar -xzf superset-darwin-arm64.tar.gz -C ~/superset
export PATH="$HOME/superset/bin:$PATH"
superset auth login # OAuth device flow, org picker in browser
superset host start --daemon # Spawns host-service, connects to relay
superset host status # pid, port, uptime, health
superset host stop # Clean shutdown
```
Verified locally
Test plan
Out of scope
Summary by cubic
Ships a standalone CLI tarball per platform with an embedded
host-serviceand Node.js runtime so servers, CI, and VMs can expose their machine viarelay.superset.sh. Adds reliable host lifecycle commands and a build-and-release pipeline.New Features
host start,host stop, andhost statususing a shared manifest at~/superset/host/<orgId>/manifest.json;host stopwaits up to 10s, escalates to SIGKILL if needed, then removes the manifest.activeOrgin~/superset/config.jsonduringauth login;org switchupdates local config and syncs the server-side active org.build:distassembles the CLI binary, Node.js 22, thehost-servicebundle, native addon packages, and migrations into per-target tarballs;RELAY_URLandCLOUD_API_URLare baked in.cli-v*tags builds for macOS arm64/x64 and Linux x64 and uploads artifacts to a GitHub Release.host-servicetargets Node (bun build --target=node); packaging ships full native addon packages (better-sqlite3,node-pty,@parcel/watcher) with transitive deps underlib/node_modules(resolved viaNODE_PATH).Migration
~/.supersetto~/superset. Move existing config if needed.~/superset/binto your PATH.Written for commit eb968a1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores