fix: address CodeRabbit review findings from v0.4.11 merge (#487) - #488
Merged
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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe patch updates log validation and scanning, inode-based rotation handling, RPC proxy batching and rollover, verbose logging, Terminal RPC prerelease detection, and executable binary checks. ChangesCLI fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant nodeDevnet
participant RPCProxy
participant proxyEvents
participant UnifiedLogger
nodeDevnet->>RPCProxy: createRPCProxy(..., { verbose })
RPCProxy->>UnifiedLogger: select debug sink when verbose
RPCProxy->>proxyEvents: process single or batch request members
proxyEvents->>UnifiedLogger: log requests and validation warnings
Possibly related PRs
🚥 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
- logs: reject --tail 0/negative (slice(-0) dumped the whole filtered log) - init-chain: includePrerelease so 0.205.1-rc1/0.206.0-rc1 pass the Terminal RPC minimum-version check - proxy-events: preserve the previous proxy.log.1 archive when the active-log rename fails during rollover - proxy-events: skip malformed JSON-RPC batch members instead of aborting the rest of the batch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
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