fix: address CodeRabbit review findings from v0.4.11 merge (#487) - #488
fix: address CodeRabbit review findings from v0.4.11 merge (#487)#488humble-little-bear wants to merge 1 commit into
Conversation
- logs: scan a wider window before script-filtering the tail so sparse ckb-script entries are not missed; reject unknown log targets instead of silently falling back to node - log-file: detect rotation by inode change in followLogFile, not only by size decrease; test rotation via rename + larger replacement file - init-chain: accept CKB 0.205.0 prereleases for Terminal RPC gating via semver range '>=0.205.0-0' - ckb-tui: treat a binary without the execute bit as a mismatch so it flows into the reinstall path (POSIX only) - proxy-events: keep appending events when a proxy.log rollover fails and retry on the next event; normalize batch JSON-RPC request payloads and guard non-array send_transaction params - rpc-proxy: annotate ctx as ProxyEventContext and wire --verbose to a debug-level sink so per-request lines print during verbose runs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe patch fixes sparse script-log scanning, invalid log-target handling, inode-based log rotation, CKB prerelease detection, non-executable binary validation, RPC proxy batching and warnings, rollover retries, and verbose proxy logging. ChangesCLI fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant nodeDevnet
participant RPCProxy
participant handleOneRequest
participant UnifiedLogger
nodeDevnet->>RPCProxy: createRPCProxy(..., { verbose })
RPCProxy->>UnifiedLogger: select debug sink when verbose
RPCProxy->>handleOneRequest: process each batch payload
handleOneRequest->>UnifiedLogger: log request and validation warning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmd/logs.ts`:
- Around line 37-39: Validate the parsed tail value before calculating
scanWindow in the logs command, rejecting zero (including -0) and any
non-positive value so script mode never reaches slice(-tail) with zero. Keep the
existing positive-tail behavior unchanged.
In `@src/node/init-chain.ts`:
- Around line 76-78: Update supportsTerminalRpcModule so newer Terminal RPC
prereleases, including versions with greater patch or minor values than
TERMINAL_RPC_MIN_CKB_VERSION, satisfy the minimum-version check. Adjust the
semver comparison to include prereleases across newer versions while preserving
the existing true behavior for null or invalid versions.
In `@src/tools/proxy-events.ts`:
- Around line 66-74: Update the rollover logic around the fs.rmSync and
fs.renameSync calls to preserve the existing `${filePath}.1` archive: move it to
a temporary path, attempt the active-log rename, and restore the archive when
that rename fails before retaining append behavior. Add a test covering
renameSync failing after the archive is successfully moved or removed, and
verify the prior archive remains intact.
- Around line 99-100: Update the batch iteration in the proxy event handling
flow around handleOneRequest to validate each member is a non-null object before
dispatching it, skip invalid members, and continue processing subsequent
requests. Add a batch test covering an invalid member before a valid
send_transaction request and verify the valid transaction is still recorded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5021b574-85d7-4da2-934b-4666f77366e7
📒 Files selected for processing (13)
.changeset/review-fixes-v0411.mdsrc/cmd/logs.tssrc/cmd/node.tssrc/devnet/log-file.tssrc/node/init-chain.tssrc/tools/ckb-tui.tssrc/tools/proxy-events.tssrc/tools/rpc-proxy.tstests/ckb-tui-install.test.tstests/init-chain.test.tstests/logs-command.test.tstests/logs.test.tstests/proxy-events.test.ts
| const scanWindow = scriptOnly ? tail * SCRIPT_SCAN_FACTOR : tail; | ||
| let lines = readLogTail(filePath, scanWindow); | ||
| if (scriptOnly) lines = filterLinesByTarget(lines, SCRIPT_LOG_TARGET).slice(-tail); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 --glob '*.ts' 'LogsOptions|--tail|tail\s*=' src testsRepository: ckb-devrel/offckb
Length of output: 4243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/cli.ts option parser ---\n'
sed -n '108,119p' src/cli.ts | cat -n
printf '\n--- src/cmd/logs.ts implementation ---\n'
sed -n '1,70p' src/cmd/logs.ts | cat -n
printf '\n--- tests for zero/negative tail ---\n'
rg -n -C 5 --glob '*.ts' 'tail:\s*0|0x|negative|InvalidArgumentError|--tail' tests src/cli.ts || true
printf '\n--- read-only JS slice behavioral probe ---\n'
node - <<'JS'
const arr = [1,2,3,4,5];
for (const n of [0, 1, -1, -0, Number('-0'), NaN, undefined]) {
try {
console.log(`${JSON.stringify(n)} => ${JSON.stringify(arr.slice(n))} (idx=${arr.slice(n).indexOf(4)})`);
} catch (e) {
console.log(`${JSON.stringify(n)} => throws ${e.name}: ${e.message}`);
}
}
JSRepository: ckb-devrel/offckb
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/cli.ts option parser ---'
sed -n '108,119p' src/cli.ts | cat -n
printf '%s\n' ''
printf '%s\n' '--- src/cmd/logs.ts implementation ---'
sed -n '1,70p' src/cmd/logs.ts | cat -n
printf '%s\n' ''
printf '%s\n' '--- tests for zero/negative tail ---'
rg -n -C 5 --glob '*.ts' 'tail:\s*0|negative|InvalidArgumentError|--tail' tests src/cli.ts || true
printf '%s\n' ''
printf '%s\n' '--- read-only JS slice behavioral probe ---'
node - <<'JS'
const arr = [1,2,3,4,5];
for (const n of [0, 1, -1, -0, Number('-0'), NaN, undefined]) {
try {
console.log(`${JSON.stringify(n)} => ${JSON.stringify(arr.slice(n))}`);
} catch (e) {
console.log(`${JSON.stringify(n)} => throws ${e.name}: ${e.message}`);
}
}
JSRepository: ckb-devrel/offckb
Length of output: 7810
Reject --tail 0 or enforce a minimum before slicing.
--tail accepts 0, and --tail -0 also passes through as 0. In script mode, lines.slice(-0) becomes lines.slice(0), so the command returns all filtered lines instead of omitting output. Reject zero before calculating scanWindow, or require tail > 0.
🤖 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/logs.ts` around lines 37 - 39, Validate the parsed tail value before
calculating scanWindow in the logs command, rejecting zero (including -0) and
any non-positive value so script mode never reaches slice(-tail) with zero. Keep
the existing positive-tail behavior unchanged.
| export function supportsTerminalRpcModule(ckbVersion: string | null | undefined): boolean { | ||
| if (ckbVersion == null || !semver.valid(ckbVersion)) return true; | ||
| return semver.gte(ckbVersion, TERMINAL_RPC_MIN_CKB_VERSION); | ||
| return semver.satisfies(ckbVersion, `>=${TERMINAL_RPC_MIN_CKB_VERSION}-0`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node <<'NODE'
const semver = require('semver');
const range = '>=0.205.0-0';
for (const version of ['0.205.0-rc1', '0.205.1-rc1', '0.206.0-rc1']) {
console.log(version, semver.satisfies(version, range));
}
NODERepository: ckb-devrel/offckb
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked init-chain/node files:\n'
git ls-files | rg '(^|/)init-chain\.ts$|(^|/)node\.ts$|(^|/)package\.json$' || true
printf '\nRelevant init-chain snippet:\n'
if [ -f src/node/init-chain.ts ]; then
nl -ba src/node/init-chain.ts | sed -n '1,130p'
fi
printf '\nRelevant node.ts uses:\n'
if [ -f src/node/node.ts ]; then
rg -n "supportsTerminalRpcModule|Terminal|devnet|includePrerelease|semver\.satisfies" src/node/node.ts -C 4
fi
printf '\nPackage semver declarations:\n'
node - <<'JS'
const fs = require('fs');
for (const p of ['package.json']) {
if (fs.existsSync(p)) {
const pkg = JSON.parse(fs.readFileSync(p, 'utf8'));
console.log(p, 'dependencies.semver=', pkg.dependencies?.semver);
console.log(p, 'devDependencies.semver=', pkg.devDependencies?.semver);
}
}
JS
printf '\nLocal semver version and comparison behavior:\n'
node - <<'JS'
const semver = require('semver');
const range = '>=0.205.0-0';
for (const version of ['0.205.0-rc1', '0.205.1-rc1', '0.205.1', '0.206.0-rc1', '0.206.0']) {
console.log(version, semver.satisfies(version, range), semver.satisfies(version, range, { includePrerelease: true }), semver.gte(version, '0.205.0'));
}
JSRepository: ckb-devrel/offckb
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Relevant init-chain snippet:'
awk '{ printf "%6d\t%s\n", NR, $0 }' src/node/init-chain.ts | sed -n '1,130p'
echo
echo 'Relevant cmd/node.ts matches:'
grep -n -C 5 "supportsTerminalRpcModule\|Terminal\|devnet\|includePrerelease\|semver\.satisfies" src/cmd/node.ts || true
echo
echo 'Package semver declarations:'
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log('dependencies.semver=', pkg.dependencies?.semver);
console.log('devDependencies.semver=', pkg.devDependencies?.semver);
JS
echo
echo 'Local semver version and comparison behavior:'
node - <<'JS'
const semver = require('semver');
const range = '>=0.205.0-0';
console.log('semver version=', require('semver/gte').version);
for (const version of ['0.205.0-rc1', '0.205.1-rc1', '0.205.1', '0.206.0-rc1', '0.206.0']) {
console.log(version, 'satisfies default=', semver.satisfies(version, range), 'satisfies prerelease=', semver.satisfies(version, range, { includePrerelease: true }), 'gte min=', semver.gte(version, '0.205.0'));
}
JSRepository: ckb-devrel/offckb
Length of output: 24141
🌐 Web query:
semver package npm compare prereleases includePrerelease >=X prerelease same major minor patch
💡 Result:
In the npm semver package, the includePrerelease option fundamentally alters how range-matching functions behave regarding prerelease versions, but it does not affect direct comparison functions [1][2]. 1. Range Matching and includePrerelease By default, the semver package excludes prerelease versions from satisfying ranges to prevent unstable versions from being accidentally selected [2][3]. For example, a range like >1.2.3-alpha.3 will not be satisfied by 3.4.5-alpha.9 even though it is technically greater [2][3]. When the includePrerelease: true option is passed to range-matching functions (such as satisfies, maxSatisfying, or minVersion), it treats prerelease versions as if they were standard release versions, allowing them to satisfy the range [2][4]. 2. Important Constraint for Ranges Even when includePrerelease is not used, a prerelease version can only satisfy a range if at least one comparator in the range contains the same [major, minor, patch] tuple and also includes a prerelease tag [2][3]. This prevents a prerelease version from accidentally matching a range that was intended only for stable releases [2][3]. 3. Comparison Functions Comparison functions (such as compare, lt, gt, eq, compareLoose) do not use the includePrerelease option [1]. These functions always compare the prerelease parts of a version if the [major, minor, patch] components are identical [1]. Consequently, they do not require or respect this flag [1]. Summary of Behavior - For range functions (e.g., semver.satisfies(v, r, { includePrerelease: true })): Prerelease tags are treated as normal parts of the version, enabling them to satisfy ranges that would otherwise exclude them [2]. - For direct comparison functions (e.g., semver.lt(v1, v2)): The includePrerelease option has no effect [1]. These functions consistently evaluate version precedence according to SemVer rules, including prerelease identifiers [1][2].
Citations:
- 1: [BUG] semver.lt returns wrong value for pre-release version comparison npm/node-semver#483
- 2: https://github.com/npm/node-semver/tree/refs/heads/main
- 3: https://github.com/npm/node-semver?tab=readme-ov-file
- 4: fix: minVersion returns the true minimum for
>in includePrerelease mode npm/node-semver#890
Allow newer Terminal RPC prereleases to pass the minimum-version check.
With >=0.205.0-0, prereleases are rejected unless they use the same [major, minor, patch] tuple as the comparator. Versions such as 0.205.1-rc1 or 0.206.0-rc1 are newer than 0.205.0, but supportsTerminalRpcModule() returns false; node startup then throws before launching CKB when the config enables Terminal.
Proposed fix
- return semver.satisfies(ckbVersion, `>=${TERMINAL_RPC_MIN_CKB_VERSION}-0`);
+ return semver.satisfies(ckbVersion, `>=${TERMINAL_RPC_MIN_CKB_VERSION}-0`, {
+ includePrerelease: true,
+ });📝 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.
| export function supportsTerminalRpcModule(ckbVersion: string | null | undefined): boolean { | |
| if (ckbVersion == null || !semver.valid(ckbVersion)) return true; | |
| return semver.gte(ckbVersion, TERMINAL_RPC_MIN_CKB_VERSION); | |
| return semver.satisfies(ckbVersion, `>=${TERMINAL_RPC_MIN_CKB_VERSION}-0`); | |
| export function supportsTerminalRpcModule(ckbVersion: string | null | undefined): boolean { | |
| if (ckbVersion == null || !semver.valid(ckbVersion)) return true; | |
| return semver.satisfies(ckbVersion, `>=${TERMINAL_RPC_MIN_CKB_VERSION}-0`, { | |
| includePrerelease: 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 76 - 78, Update
supportsTerminalRpcModule so newer Terminal RPC prereleases, including versions
with greater patch or minor values than TERMINAL_RPC_MIN_CKB_VERSION, satisfy
the minimum-version check. Adjust the semver comparison to include prereleases
across newer versions while preserving the existing true behavior for null or
invalid versions.
| try { | ||
| fs.rmSync(`${filePath}.1`, { force: true }); | ||
| fs.renameSync(filePath, `${filePath}.1`); | ||
| size = 0; | ||
| } catch { | ||
| // Rollover failed (for example the file is locked). Keep | ||
| // appending so events are not lost, and retry on the next event. | ||
| size = maxBytes; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the prior rotated log when the active-log rename fails.
Line 67 deletes proxy.log.1 before Line 68 proves that the active log can replace it. If renameSync(filePath, \${filePath}.1`)` fails after deletion, the catch block keeps new events but permanently loses the prior archive.
Move the existing archive to a temporary path and restore it if the active-log rename fails. Add a test that makes renameSync fail after successful archive removal.
🤖 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/tools/proxy-events.ts` around lines 66 - 74, Update the rollover logic
around the fs.rmSync and fs.renameSync calls to preserve the existing
`${filePath}.1` archive: move it to a temporary path, attempt the active-log
rename, and restore the archive when that rename fails before retaining append
behavior. Add a test covering renameSync failing after the archive is
successfully moved or removed, and verify the prior archive remains intact.
| for (const jsonRpcContent of Array.isArray(parsed) ? parsed : [parsed]) { | ||
| handleOneRequest(jsonRpcContent, ctx); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip malformed batch members without stopping the batch.
JSON.parse accepts null. For [null, { "method": "send_transaction", "params": [tx] }], handleOneRequest dereferences .method on null. The outer catch then exits the loop, so the valid transaction is not recorded.
Validate that each batch member is a non-null object before calling handleOneRequest. Continue after invalid members. Add a batch test with an invalid member before a valid send_transaction request.
🤖 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/tools/proxy-events.ts` around lines 99 - 100, Update the batch iteration
in the proxy event handling flow around handleOneRequest to validate each member
is a non-null object before dispatching it, skip invalid members, and continue
processing subsequent requests. Add a batch test covering an invalid member
before a valid send_transaction request and verify the valid transaction is
still recorded.
1 participant
Fixes all 8 actionable CodeRabbit review comments from #487 (the v0.4.11 develop→master merge).
Changes
Logging (
offckb logs)logs scriptscanned only the lasttaillines before filtering, so sparseckb-scriptentries were usually missed → scan a 100× wider window, trim totailafter filtering (src/cmd/logs.ts)offckb logs scrpit) silently fell back to the node log → now rejected with the accepted values (src/cmd/logs.ts)followLogFiledetected rotation only as a size decrease; a rotated-in file already larger than the old one was read from a stale offset → reset on inode change too, with the test now modeling real rotation (rename + larger replacement) (src/devnet/log-file.ts,tests/logs.test.ts)Chain init
semver.gte('0.205.0-rc1', '0.205.0')is false, so 0.205.0 rc binaries had the Terminal RPC module stripped → usesemver.satisfies(v, '>=0.205.0-0')(src/node/init-chain.ts)ckb-tui install
X_OKcheck on POSIX routes it into the reinstall path (src/tools/ckb-tui.ts)RPC proxy
size = -1and dropped every later event → rollover failure now keeps appending and retries on the next event (src/tools/proxy-events.ts)send_transactiontxs lost), and missing/non-arrayparamsthrew a TypeError misreported as a parse error → normalize payloads to an array, guardparams(src/tools/proxy-events.ts)ctxannotated asProxyEventContext;--verbosenow wires a debug-level sink so per-request "RPC Req" lines print during verbose runs (src/tools/rpc-proxy.ts,src/cmd/node.ts)Tests
tsc --noEmitclean; build passes🤖 Generated with Claude Code