fix(node): adapt devnet Terminal RPC module to the CKB version - #477
Open
humble-little-bear wants to merge 1 commit into
Open
fix(node): adapt devnet Terminal RPC module to the CKB version#477humble-little-bear wants to merge 1 commit into
humble-little-bear wants to merge 1 commit into
Conversation
The devnet ckb.toml template enables the Terminal RPC module, which only exists since CKB v0.205.0 (nervosnetwork/ckb#4989). Older binaries abort at startup with an opaque serde "unknown variant `Terminal`" error, and migrateLegacyDevnetRpcConfig re-added the module on every `offckb node` start even after users removed it by hand — an unbreakable crash loop for anyone pinned to an old CKB. - initChainIfNeeded takes the effective CKB version: fresh chains for a pre-0.205.0 binary are initialized from the template with Terminal stripped (tcp_listen_address, which predates 0.205.0, stays), and the legacy-config migration no longer re-adds Terminal for such binaries. - nodeDevnet resolves the effective version (managed binaries know it; a custom --binary-path is probed via getVersionFromBinary) and fails fast with an actionable error when the existing config enables Terminal but the binary is too old, instead of letting CKB dump the serde error. - A custom binary whose version cannot be probed keeps the historical behavior; if it then crashes with the tell-tale "unknown variant `Terminal`" stderr, the startup error now points at the actual cause. - README's status section notes the CKB >= 0.205.0 requirement for the TUI's system-metric panels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughDevnet startup now detects the selected CKB version, gates the Terminal RPC module in generated configurations, prevents incompatible legacy migration, and reports actionable errors for unsupported configurations or Terminal-related startup failures. Documentation and tests cover the new behavior. ChangesTerminal RPC compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant nodeDevnet
participant CKBBinary
participant initChainIfNeeded
participant CKBDevnet
nodeDevnet->>CKBBinary: probe or select CKB version
nodeDevnet->>initChainIfNeeded: initialize ckb.toml with ckbVersion
initChainIfNeeded-->>nodeDevnet: version-compatible configuration
nodeDevnet->>CKBDevnet: spawn devnet with ckb.toml
CKBDevnet-->>nodeDevnet: readiness or stderr result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/init-chain.test.ts (1)
209-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! Solid coverage of the version-gating matrix (unknown/old/new versions × fresh-init/migration/pre-existing-config paths).
One minor gap:
removeTerminalModule's multi-line-array branch (init-chain.ts Lines 166-174) is never exercised — the bundled template being stripped in these tests is single-line, and the multi-line fixture here is only used to assert "never edit an existing file," not fresh-init stripping. Consider adding a case where the bundled template itself is (or is rewritten to) multi-line before init, if that branch is meant to be reachable in practice.🤖 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 `@tests/init-chain.test.ts` around lines 209 - 330, Add coverage for the multi-line-array branch in removeTerminalModule by making the fresh-init template’s RPC modules multi-line before invoking initChainIfNeeded with an old CKB version. Assert Terminal is removed while other modules remain, ensuring this path tests actual template stripping rather than preservation of an existing ckb.toml.src/node/init-chain.ts (1)
146-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
TERMINAL_RPC_MODULEinstead of the hardcoded"Terminal"literal.
removeTerminalModule's regexes (Lines 158-160, 168) hardcode the string"Terminal"even thoughTERMINAL_RPC_MODULEalready exists as the canonical constant (used elsewhere in this file, e.g.devnetConfigHasTerminalRpc,addTerminalModule). If the module name ever changes, these regexes would silently stop matching.♻️ Proposed fix
if (lines[modulesStart].includes(']')) { const line = lines[modulesStart]; // Terminal last (the bundled template), Terminal first, or Terminal alone. - let updated = line.replace(/,\s*"Terminal"/, ''); - if (updated === line) updated = line.replace(/"Terminal"\s*,\s*/, ''); - if (updated === line) updated = line.replace(/\[\s*"Terminal"\s*\]/, '[]'); + let updated = line.replace(new RegExp(`,\\s*"${TERMINAL_RPC_MODULE}"`), ''); + if (updated === line) updated = line.replace(new RegExp(`"${TERMINAL_RPC_MODULE}"\\s*,\\s*`), ''); + if (updated === line) updated = line.replace(new RegExp(`\\[\\s*"${TERMINAL_RPC_MODULE}"\\s*\\]`), '[]'); if (updated === line) return false; lines[modulesStart] = updated; return true; } // Multi-line array: drop the line holding the Terminal entry. for (let i = modulesStart + 1; i < section.end; i++) { - if (/^\s*"Terminal",?\s*$/.test(lines[i])) { + if (new RegExp(`^\\s*"${TERMINAL_RPC_MODULE}",?\\s*$`).test(lines[i])) { lines.splice(i, 1); return true; }🤖 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 `@src/node/init-chain.ts` around lines 146 - 197, Update removeTerminalModule to derive all Terminal-entry matching and replacement patterns from the existing TERMINAL_RPC_MODULE constant instead of hardcoding "Terminal". Preserve the current handling for single-line and multi-line module arrays, including quoted TOML entries.src/cmd/node.ts (1)
71-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating Terminal compatibility before
installCKBBinary, not after.For the managed-binary path,
effectiveCkbVersionis already known asckbVersionbeforeinstallCKBBinaryruns — nothing about the compatibility check (Lines 96-108) actually depends on the binary being installed or the chain being initialized first. Moving thedevnetConfigHasTerminalRpc/supportsTerminalRpcModulecheck earlier would fail fast without a potentially slow binary download when the run is going to error out anyway.[reliability_and_resilience]
♻️ Proposed reordering
const settings = readSettings(); const ckbVersion = version || settings.bins.defaultCKBVersion; let ckbBinPath = ''; let effectiveCkbVersion: string | null = null; + const devnetConfigPath = settings.devnet.configPath; if (binaryPath) { ckbBinPath = binaryPath; logger.info(`Using custom CKB binary path: ${ckbBinPath}`); effectiveCkbVersion = getVersionFromBinary(ckbBinPath); } else { - await installCKBBinary(ckbVersion); - ckbBinPath = getCKBBinaryPath(ckbVersion); effectiveCkbVersion = ckbVersion; } - await initChainIfNeeded({ ckbVersion: effectiveCkbVersion }); - const devnetConfigPath = settings.devnet.configPath; - if (!supportsTerminalRpcModule(effectiveCkbVersion) && devnetConfigHasTerminalRpc(devnetConfigPath)) { + const terminalSupported = supportsTerminalRpcModule(effectiveCkbVersion); + if (!terminalSupported && devnetConfigHasTerminalRpc(devnetConfigPath)) { throw new Error(/* ... */); } - if (!supportsTerminalRpcModule(effectiveCkbVersion)) { + if (!terminalSupported) { logger.info(/* ... */); } + + if (!binaryPath) { + await installCKBBinary(ckbVersion); + ckbBinPath = getCKBBinaryPath(ckbVersion); + } + await initChainIfNeeded({ ckbVersion: effectiveCkbVersion });Category note: this doubles as for the
supportsTerminalRpcModulede-duplication.🤖 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 `@src/cmd/node.ts` around lines 71 - 109, Move the Terminal compatibility validation using devnetConfigHasTerminalRpc and supportsTerminalRpcModule before installCKBBinary in the managed-binary path, using the already-known ckbVersion as the selected version. Preserve the existing error message and logging behavior, avoid duplicating the validation after initialization, and retain the custom-binary path’s probed-version validation.
🤖 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.
Nitpick comments:
In `@src/cmd/node.ts`:
- Around line 71-109: Move the Terminal compatibility validation using
devnetConfigHasTerminalRpc and supportsTerminalRpcModule before installCKBBinary
in the managed-binary path, using the already-known ckbVersion as the selected
version. Preserve the existing error message and logging behavior, avoid
duplicating the validation after initialization, and retain the custom-binary
path’s probed-version validation.
In `@src/node/init-chain.ts`:
- Around line 146-197: Update removeTerminalModule to derive all Terminal-entry
matching and replacement patterns from the existing TERMINAL_RPC_MODULE constant
instead of hardcoding "Terminal". Preserve the current handling for single-line
and multi-line module arrays, including quoted TOML entries.
In `@tests/init-chain.test.ts`:
- Around line 209-330: Add coverage for the multi-line-array branch in
removeTerminalModule by making the fresh-init template’s RPC modules multi-line
before invoking initChainIfNeeded with an old CKB version. Assert Terminal is
removed while other modules remain, ensuring this path tests actual template
stripping rather than preservation of an existing ckb.toml.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 598c041d-d5e1-4eed-9117-50e10791b9d7
📒 Files selected for processing (7)
.changeset/terminal-rpc-version-compat.mdREADME.mdsrc/cmd/node.tssrc/node/init-chain.tstests/init-chain.test.tstests/node-supervisor.test.tstests/node-terminal-rpc.test.ts
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.
Problem
The devnet
ckb.tomltemplate enables theTerminalRPC module, which only exists since CKB v0.205.0 (nervosnetwork/ckb#4989). For anyone running the devnet with an older CKB (offckb node <旧版本>, a pinneddefaultCKBVersion, or--binary-path), the node aborts at startup with an opaque serde error:Worse,
migrateLegacyDevnetRpcConfigre-addedTerminalon everyoffckb nodestart, so manually removing it from the config didn't help — an unbreakable crash loop that documentation alone could not fix.What this PR does
Node startup now resolves the effective CKB version (managed binaries know it by construction; a custom
--binary-pathis probed via the existinggetVersionFromBinary()) and adapts:Terminalstripped (tcp_listen_addresspredates 0.205.0 and stays), with a log line noting theoffckb statuspanels need ≥ 0.205.0.Terminalfor such binaries — breaking the remove/re-add loop — while still enablingtcp_listen_address.Terminal+ old binary fails fast before spawning CKB, with an actionable error naming the file, the required version, and the two remedies — replacing the serde dump.unknown variantTerminal`` stderr, the startup error now appends a hint pointing at the actual cause (bounded 4 KB stderr tail).statussection notes the CKB >= 0.205.0 requirement for the TUI's system-metric panels.Pre-existing user configs are never silently rewritten — the strip applies only to a pristine, freshly copied template; everything else surfaces as a clear error.
Test plan
pnpm test— 29 suites, 237 passed (21 ininit-chain, 9 new innode-terminal-rpccovering version gating, fail-fast, probe fallback, and the stderr hint)tsc --noEmitclean,eslintclean,pnpm buildsucceeds0.120.0produces a valid ckb.toml withoutTerminal, withtcp_listen_addressintact)🤖 Generated with Claude Code