Spawn typeberry nodes in docker containers according to network specification#110
Spawn typeberry nodes in docker containers according to network specification#110
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a new CLI start command that runs a local JAM network in a Docker container, along with required changes to the genesis state structure from array to object format for JSON compatibility. Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant StartCmd as Start Command
participant Docker as Docker API
participant Filesystem as Filesystem
participant Container as JAM Container
User->>StartCmd: Run start command
StartCmd->>Filesystem: Verify genesis.json exists
Filesystem-->>StartCmd: ✓ File found
StartCmd->>StartCmd: Read & filter genesis_state
StartCmd->>Filesystem: Write filtered genesis to temp
Filesystem-->>StartCmd: Temp file path
StartCmd->>Docker: Pull JAM network image
Docker-->>StartCmd: Image ready
StartCmd->>Docker: Run container with volumes
Docker->>Container: Start container
Container-->>Docker: Container running
Docker-->>StartCmd: Process handle
StartCmd->>Filesystem: Create logs directory
Filesystem-->>StartCmd: Logs dir ready
StartCmd->>StartCmd: Wire signal handlers
User->>StartCmd: Send SIGINT/SIGTERM
StartCmd->>Container: Terminate gracefully
StartCmd->>Filesystem: Clean up temp files
Filesystem-->>StartCmd: Cleanup complete
StartCmd-->>User: Exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
bin/cli/src/commands/start-command.ts (2)
53-53: Usejoin()for path construction.For consistency with the rest of the file and best practices, use
join()fromnode:pathinstead of string interpolation.♻️ Suggested fix
- const logsPath = `${process.cwd()}/logs`; + const logsPath = join(process.cwd(), "logs");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` at line 53, The code constructs logsPath using string interpolation ("const logsPath = `${process.cwd()}/logs`;") which is brittle; change it to use path.join (imported from 'node:path' or 'path') and set logsPath = join(process.cwd(), 'logs') and add the join import if missing so path separators are handled correctly across platforms and consistent with other usages in this file.
17-30: Add JSDoc documentation for the public function.As per coding guidelines, public APIs should have JSDoc comments. Consider adding documentation similar to the other exported functions.
📝 Suggested documentation
+/** + * Creates a filtered copy of the genesis file with validator keys removed. + * `@param` genesisPath - Path to the source genesis.json file + * `@returns` Path to the temporary filtered genesis file + */ export async function createFilteredGenesis(genesisPath: string): Promise<string> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 17 - 30, The exported function createFilteredGenesis lacks JSDoc; add a JSDoc block above the createFilteredGenesis declaration that documents the function purpose, parameters, return value and behavior (mention it reads a genesis JSON, removes keys listed in GENESIS_STATE_DROP_KEYS from genesis.genesis_state if present, writes a temp file with formatted JSON, and returns the temp path), and include `@param` {string} genesisPath and `@returns` {Promise<string}> annotations and any thrown errors or side effects for consistency with other exported functions.bin/cli/index.ts (1)
29-38: Consider addingstartto the examples list.The help text examples include
new,build,test, anddeploy, but not the newstartcommand.📝 Suggested update
Examples: $ jammin new [options] $ jammin build [options] $ jammin test [options] $ jammin deploy [options] + $ jammin start For more information, visit: https://fluffylabs.dev/jammin/`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/index.ts` around lines 29 - 38, The help text added via program.addHelpText currently lists example commands (new, build, test, deploy) but omits the new start command; update the template string passed to program.addHelpText to include an example line for start (e.g., "$ jammin start [options]") so the CLI help shows the start usage alongside the other examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli/src/commands/start-command.ts`:
- Around line 36-46: The code tries to read proc.stderr while spawning Docker
with stderr: "inherit", which makes proc.stderr null; either stop reading stderr
or make stderr readable: change the Bun.spawn call for the Docker pull (the
const proc created by Bun.spawn and the DOCKER_IMAGE usage) to use stderr:
"pipe" (and keep stdout either "inherit" or "pipe") and then await new
Response(proc.stderr).text() alongside proc.exited to include the captured
stderr in the thrown Error, or remove the new Response(proc.stderr).text() read
and the stderr element from the Promise.all and throw the Error without stderr
since output is already visible; update the error message construction
accordingly.
In `@packages/jammin-sdk/genesis-state-generator.ts`:
- Around line 71-76: The tests assume genesis_state is an array but
toJip4Schema() now returns genesis_state as an object (built with
Object.fromEntries); update the test assertions in
genesis-state-generator.test.ts to reflect an object contract: replace
Array.isArray checks with typeof genesis.genesis_state === 'object' and use
Object.keys(genesis.genesis_state).length > 0 or
Object.values(genesis.genesis_state) instead of .length and .map, and when
iterating use Object.entries(genesis.genesis_state) or
Object.values(genesis.genesis_state) to access values; alternatively, if the
required contract is still an array, revert the change in toJip4Schema() to
return [...genesis.genesisState.entries()].map(([k,v]) =>
[k.toString().substring(2), v.toString().substring(2)]) so tests can remain
unchanged.
---
Nitpick comments:
In `@bin/cli/index.ts`:
- Around line 29-38: The help text added via program.addHelpText currently lists
example commands (new, build, test, deploy) but omits the new start command;
update the template string passed to program.addHelpText to include an example
line for start (e.g., "$ jammin start [options]") so the CLI help shows the
start usage alongside the other examples.
In `@bin/cli/src/commands/start-command.ts`:
- Line 53: The code constructs logsPath using string interpolation ("const
logsPath = `${process.cwd()}/logs`;") which is brittle; change it to use
path.join (imported from 'node:path' or 'path') and set logsPath =
join(process.cwd(), 'logs') and add the join import if missing so path
separators are handled correctly across platforms and consistent with other
usages in this file.
- Around line 17-30: The exported function createFilteredGenesis lacks JSDoc;
add a JSDoc block above the createFilteredGenesis declaration that documents the
function purpose, parameters, return value and behavior (mention it reads a
genesis JSON, removes keys listed in GENESIS_STATE_DROP_KEYS from
genesis.genesis_state if present, writes a temp file with formatted JSON, and
returns the temp path), and include `@param` {string} genesisPath and `@returns`
{Promise<string}> annotations and any thrown errors or side effects for
consistency with other exported functions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bin/cli/src/commands/start-command.ts (1)
36-46:⚠️ Potential issue | 🔴 Critical
proc.stderrisnullwhenstderr: "inherit"is used.When
stderr: "inherit"is set, Bun pipes stderr directly to the parent process andproc.stderrbecomesnull. Line 42 attempts to read fromproc.stderr, which won't capture error output as intended.🔧 Option 1: Remove stderr read (simpler, output is already visible)
- const [stderr, exitCode] = await Promise.all([new Response(proc.stderr).text(), proc.exited]); + const exitCode = await proc.exited; if (exitCode !== 0) { - throw new Error(`Failed to pull image ${DOCKER_IMAGE} with exit code ${exitCode}: ${stderr}`); + throw new Error(`Failed to pull image ${DOCKER_IMAGE} with exit code ${exitCode}`); }🔧 Option 2: Capture stderr for error message
const proc = Bun.spawn(["docker", "pull", "--platform=linux/amd64", DOCKER_IMAGE], { stdout: "inherit", - stderr: "inherit", + stderr: "pipe", cwd: process.cwd(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 36 - 46, The code reads proc.stderr even though Bun.spawn was called with stderr: "inherit", which makes proc.stderr null; update the Bun.spawn call in start-command.ts so stderr is "pipe" (or remove the stderr read and its use) and then await Promise.all([new Response(proc.stderr).text(), proc.exited]) to capture the error text; specifically modify the Bun.spawn options for the proc created in this file (the variable proc that pulls DOCKER_IMAGE) to use stderr: "pipe" if you want to include stderr in the thrown Error, or remove the new Response(proc.stderr).text() read and adjust the error path to rely on exitCode only if you choose the simpler option.
🧹 Nitpick comments (1)
bin/cli/src/commands/start-command.ts (1)
75-81: Signal handlers are registered but never removed.The
cleanupfunction is added toSIGINT,SIGTERM, andexitevents but is never removed after the container exits. In long-running processes or whenstartContaineris called multiple times, this could lead to listener accumulation.♻️ Proposed fix to remove listeners after use
const cleanup = () => { proc.kill(); }; process.on("SIGINT", cleanup); process.on("SIGTERM", cleanup); process.on("exit", cleanup); - await proc.exited; + try { + await proc.exited; + } finally { + process.off("SIGINT", cleanup); + process.off("SIGTERM", cleanup); + process.off("exit", cleanup); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 75 - 81, The signal handlers registered with process.on("SIGINT"/"SIGTERM"/"exit") use the cleanup function (which calls proc.kill()) but are never removed, causing listener accumulation; update the cleanup to be idempotent and, after the container/process finishes, call process.off("SIGINT", cleanup), process.off("SIGTERM", cleanup) and process.off("exit", cleanup) (or removeListener) so listeners are removed, or alternatively remove those listeners inside cleanup once it runs; reference the existing cleanup function and the proc variable when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli/src/commands/start-command.test.ts`:
- Around line 138-140: The test uses expect(pullImage()).rejects.toThrow(...)
without awaiting the rejects matcher, so the rejection assertions may not run;
update the test around the pullImage() assertions to await the promises (e.g.,
add await before expect(pullImage()).rejects.toThrow for both "Failed to pull
image" and "exit code 1") so the test properly awaits and verifies the rejection
from pullImage.
- Around line 172-176: The afterEach cleanup currently calls unlink(testCwd)
(and resets process.cwd to "/"), which fails for directories and may break cwd
for other tests; update the afterEach in start-command.test (the afterEach
block) to await a recursive directory removal (use fs.rm or fs.promises.rm with
recursive:true, force:true) on testCwd instead of unlink, and capture the
original cwd before tests run (store originalCwd in beforeEach or top-level) so
you can restore process.cwd = originalCwd rather than hardcoding "/"; also
continue restoring Bun.spawn = originalSpawn as before.
---
Duplicate comments:
In `@bin/cli/src/commands/start-command.ts`:
- Around line 36-46: The code reads proc.stderr even though Bun.spawn was called
with stderr: "inherit", which makes proc.stderr null; update the Bun.spawn call
in start-command.ts so stderr is "pipe" (or remove the stderr read and its use)
and then await Promise.all([new Response(proc.stderr).text(), proc.exited]) to
capture the error text; specifically modify the Bun.spawn options for the proc
created in this file (the variable proc that pulls DOCKER_IMAGE) to use stderr:
"pipe" if you want to include stderr in the thrown Error, or remove the new
Response(proc.stderr).text() read and adjust the error path to rely on exitCode
only if you choose the simpler option.
---
Nitpick comments:
In `@bin/cli/src/commands/start-command.ts`:
- Around line 75-81: The signal handlers registered with
process.on("SIGINT"/"SIGTERM"/"exit") use the cleanup function (which calls
proc.kill()) but are never removed, causing listener accumulation; update the
cleanup to be idempotent and, after the container/process finishes, call
process.off("SIGINT", cleanup), process.off("SIGTERM", cleanup) and
process.off("exit", cleanup) (or removeListener) so listeners are removed, or
alternatively remove those listeners inside cleanup once it runs; reference the
existing cleanup function and the proc variable when making changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/cli/index.tsbin/cli/src/commands/start-command.test.tsbin/cli/src/commands/start-command.tspackages/jammin-sdk/utils/genesis-state-generator.test.tspackages/jammin-sdk/utils/genesis-state-generator.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/cli/src/commands/start-command.ts (1)
36-46:⚠️ Potential issue | 🔴 Critical
stderris unavailable when set to"inherit"— readingproc.stderrwill fail or return empty.
Bun.spawnwithstderr: "inherit"pipes stderr directly to the parent, makingproc.stderrnull. Thenew Response(proc.stderr).text()on line 42 won't capture meaningful output.Either change to
stderr: "pipe"to capture the error text, or drop the stderr read since output is already visible to the user:Option A: Capture stderr for the error message
const proc = Bun.spawn(["docker", "pull", "--platform=linux/amd64", DOCKER_IMAGE], { stdout: "inherit", - stderr: "inherit", + stderr: "pipe", cwd: process.cwd(), });Option B: Drop the stderr read (simpler)
- const [stderr, exitCode] = await Promise.all([new Response(proc.stderr).text(), proc.exited]); + const exitCode = await proc.exited; if (exitCode !== 0) { - throw new Error(`Failed to pull image ${DOCKER_IMAGE} with exit code ${exitCode}: ${stderr}`); + throw new Error(`Failed to pull image ${DOCKER_IMAGE} with exit code ${exitCode}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 36 - 46, The pull logic reads proc.stderr but sets stderr: "inherit", so proc.stderr is null; update the Bun.spawn call in start-command.ts (where proc and DOCKER_IMAGE are used) to either change stderr to "pipe" and keep reading new Response(proc.stderr).text() for inclusion in the thrown Error, or remove the stderr read entirely and rely on the inherited output and exitCode to throw a simpler Error; modify the Bun.spawn options and the subsequent Promise.all/read of proc.stderr accordingly so you don't attempt to read a null proc.stderr.
🧹 Nitpick comments (5)
bin/cli/index.ts (1)
29-38: Help text examples don't include the newstartcommand.The examples section lists
new,build,test, anddeploybut omitsstart. I see there's already a TODO on line 28 about this, so this is just a heads-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/index.ts` around lines 29 - 38, The help examples passed to program.addHelpText are missing the new start command; update the string literal supplied to program.addHelpText to include a line showing the start command (e.g., "$ jammin start [options]") alongside the existing examples so the CLI help reflects the new start command (refer to program.addHelpText where the Examples block is defined).bin/cli/src/commands/start-command.ts (3)
17-30: Missing JSDoc comment on exported function.
createFilteredGenesisis exported and public-facing but lacks a JSDoc comment, unlike the other exported functions in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 17 - 30, Add a JSDoc block above the exported function createFilteredGenesis describing its purpose, parameters, return value, and behavior: note that it reads a genesis JSON from genesisPath, removes keys listed in GENESIS_STATE_DROP_KEYS from genesis.genesis_state (if present), writes the filtered JSON to a temporary file (named with jammin-genesis-<timestamp>.json) and returns the temp file path as a Promise<string>; include `@param` {string} genesisPath and `@returns` {Promise<string>} and mention potential side effects (creates temp file) and thrown errors when parsing or writing fail so the public API is documented.
75-83: Signal listeners are never removed andproc.kill()may throw after exit.After
proc.exitedresolves, theSIGINT/SIGTERM/exitlisteners remain attached. If the CLI process continues (orstartContaineris called again), listeners accumulate. Additionally, callingproc.kill()on an already-exited process can throw.Suggested fix: remove listeners after exit and guard proc.kill
const cleanup = () => { - proc.kill(); + try { proc.kill(); } catch {} }; process.on("SIGINT", cleanup); process.on("SIGTERM", cleanup); process.on("exit", cleanup); await proc.exited; + + process.off("SIGINT", cleanup); + process.off("SIGTERM", cleanup); + process.off("exit", cleanup);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` around lines 75 - 83, The cleanup listener currently registered for "SIGINT"/"SIGTERM"/"exit" should be removed after awaiting proc.exited and proc.kill should be called safely; update the code around the cleanup function and the process.on registrations to (1) ensure cleanup removes the three listeners (use process.off/removeListener for the same handler) once proc.exited resolves and (2) guard the call to proc.kill so it won’t throw if the child already exited (either check a boolean like proc.killed or wrap proc.kill in a try/catch). Reference the existing symbols: cleanup, proc.kill, and proc.exited when making these changes.
53-53: Usejoin()for path construction instead of string interpolation.Line 53 uses template-literal concatenation for the logs path, while
joinfromnode:pathis already imported and used elsewhere in this file. This is inconsistent and won't handle edge cases (e.g., trailing slashes oncwd()).Suggested fix
- const logsPath = `${process.cwd()}/logs`; + const logsPath = join(process.cwd(), "logs");Note: You'll need to add
jointo the import fromnode:pathon line 3 — it's already imported there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.ts` at line 53, Replace the string-interpolated logsPath (`const logsPath = `${process.cwd()}/logs`;`) with a proper path.join call to handle edge cases: use join(process.cwd(), 'logs') for the logsPath variable (ensure join is imported from 'node:path' alongside other imports); update the declaration of logsPath to reference join so path separators and trailing slashes are handled correctly.bin/cli/src/commands/start-command.test.ts (1)
138-141: Double invocation ofpullImage()in error test — consider consolidating.Each
expect(pullImage()).rejects.toThrow(...)call invokespullImage()separately, triggering two mock spawn calls. While thereturn Promise.all(...)pattern correctly awaits the assertions (addressing the past review about missingawait), you could combine both substring checks in a single call for clarity:Suggested simplification
- return Promise.all([ - expect(pullImage()).rejects.toThrow("Failed to pull image"), - expect(pullImage()).rejects.toThrow("exit code 1"), - ]); + const err = await pullImage().catch((e: Error) => e); + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toContain("Failed to pull image"); + expect((err as Error).message).toContain("exit code 1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/start-command.test.ts` around lines 138 - 141, The test currently calls pullImage() twice inside Promise.all which triggers two mock spawn calls; change it to invoke pullImage() once, await the single rejected promise (e.g., await expect(pullImage()).rejects.toThrow(...) or await expect(pullImage()).rejects.toThrowErrorMatching(/Failed to pull image.*exit code 1/)) and assert both substrings/regex against that single error — update the assertions around pullImage() in start-command.test.ts so only one call to pullImage() is made and both error conditions are checked on that single rejected result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli/src/commands/start-command.test.ts`:
- Around line 83-97: The test's mockSpawn returns a ReadableStream for stderr
which hides a production bug in pullImage where Bun.spawn is invoked with
stderr: "inherit" and proc.stderr is therefore null; update the test mock to
mirror runtime behavior by making (Bun as any).spawn (mockSpawn) produce an
object where stderr is null (or absent) and exited resolves as before, and
adjust the mock name mockSpawn and references to Bun.spawn/pullImage to ensure
the test fails if pullImage still reads proc.stderr when stderr is inherited.
---
Duplicate comments:
In `@bin/cli/src/commands/start-command.ts`:
- Around line 36-46: The pull logic reads proc.stderr but sets stderr:
"inherit", so proc.stderr is null; update the Bun.spawn call in start-command.ts
(where proc and DOCKER_IMAGE are used) to either change stderr to "pipe" and
keep reading new Response(proc.stderr).text() for inclusion in the thrown Error,
or remove the stderr read entirely and rely on the inherited output and exitCode
to throw a simpler Error; modify the Bun.spawn options and the subsequent
Promise.all/read of proc.stderr accordingly so you don't attempt to read a null
proc.stderr.
---
Nitpick comments:
In `@bin/cli/index.ts`:
- Around line 29-38: The help examples passed to program.addHelpText are missing
the new start command; update the string literal supplied to program.addHelpText
to include a line showing the start command (e.g., "$ jammin start [options]")
alongside the existing examples so the CLI help reflects the new start command
(refer to program.addHelpText where the Examples block is defined).
In `@bin/cli/src/commands/start-command.test.ts`:
- Around line 138-141: The test currently calls pullImage() twice inside
Promise.all which triggers two mock spawn calls; change it to invoke pullImage()
once, await the single rejected promise (e.g., await
expect(pullImage()).rejects.toThrow(...) or await
expect(pullImage()).rejects.toThrowErrorMatching(/Failed to pull image.*exit
code 1/)) and assert both substrings/regex against that single error — update
the assertions around pullImage() in start-command.test.ts so only one call to
pullImage() is made and both error conditions are checked on that single
rejected result.
In `@bin/cli/src/commands/start-command.ts`:
- Around line 17-30: Add a JSDoc block above the exported function
createFilteredGenesis describing its purpose, parameters, return value, and
behavior: note that it reads a genesis JSON from genesisPath, removes keys
listed in GENESIS_STATE_DROP_KEYS from genesis.genesis_state (if present),
writes the filtered JSON to a temporary file (named with
jammin-genesis-<timestamp>.json) and returns the temp file path as a
Promise<string>; include `@param` {string} genesisPath and `@returns`
{Promise<string>} and mention potential side effects (creates temp file) and
thrown errors when parsing or writing fail so the public API is documented.
- Around line 75-83: The cleanup listener currently registered for
"SIGINT"/"SIGTERM"/"exit" should be removed after awaiting proc.exited and
proc.kill should be called safely; update the code around the cleanup function
and the process.on registrations to (1) ensure cleanup removes the three
listeners (use process.off/removeListener for the same handler) once proc.exited
resolves and (2) guard the call to proc.kill so it won’t throw if the child
already exited (either check a boolean like proc.killed or wrap proc.kill in a
try/catch). Reference the existing symbols: cleanup, proc.kill, and proc.exited
when making these changes.
- Line 53: Replace the string-interpolated logsPath (`const logsPath =
`${process.cwd()}/logs`;`) with a proper path.join call to handle edge cases:
use join(process.cwd(), 'logs') for the logsPath variable (ensure join is
imported from 'node:path' alongside other imports); update the declaration of
logsPath to reference join so path separators and trailing slashes are handled
correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/cli/index.tsbin/cli/src/commands/start-command.test.tsbin/cli/src/commands/start-command.tspackages/jammin-sdk/utils/genesis-state-generator.test.tspackages/jammin-sdk/utils/genesis-state-generator.ts
Resolves #12