fix: rename mainnet-fork override flag and apply leftover 0.4.9 review fixes#466
Merged
Conversation
…w fixes - Rename --allow-mainnet-replay-risk to --allow-external-key-on-mainnet-fork (#460) - Enforce the Mainnet-fork replay guard in transfer-all, udt issue/destroy, and deploy, threading the fork boundary into input selection (#462) - Validate --tx-hash before it is used in debug cache paths - Only read the fork boundary after the spawned process binds the RPC port - Reject symlinked entries when copying fork source chain data - Accept extended xUDT type args (owner hash + flags/extension) - Per-kind UDT scan budgets, deep-cloned settings fallbacks, accurate config-set errors, preserved devnet-config error, execFile process lookup, aligned ckb-tui download timeouts, EXDEV-safe install, README TOC entry 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR renames the external-key mainnet-fork override, enforces replay guards across transaction and deployment flows, validates transaction and UDT inputs, hardens fork-data copying and devnet startup, preserves configuration errors, and improves ckb-tui installation behavior. ChangesMainnet fork safety and validation
Runtime reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Command
participant ForkSafety
participant SDK
participant CKB
CLI->>Command: invoke fork-sensitive operation
Command->>ForkSafety: validate signing key and fork state
ForkSafety-->>Command: return input rejection boundary
Command->>SDK: pass rejection boundary
SDK->>CKB: assert input origins and submit transaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 @.changeset/tidy-mugs-repair.md:
- Around line 2-5: Update the CLI option rename described in the changeset so
--allow-mainnet-replay-risk remains supported as a deprecated alias for
--allow-external-key-on-mainnet-fork, or classify the removal as a breaking
release instead of a patch. Ensure existing scripts continue to function while
the new flag is adopted.
In `@src/cmd/node.ts`:
- Around line 193-207: Update isProcessListeningOnPort so lsof probe failures
other than a missing executable return null, representing an indeterminate
ownership check rather than false. Preserve true for a successful
listening-process match and ensure callers can fall back to weaker signals
whenever the inspection cannot reliably run.
In `@src/devnet/fork.ts`:
- Around line 272-282: Validate the sourceData root with assertNoSymlink before
readdirSync enumerates its contents, then retain the existing per-entry and
recursive filter checks around fs.cpSync. This ensures a symlinked data
directory is rejected before traversal.
In `@src/tools/ckb-tui.ts`:
- Around line 187-199: Update the EXDEV fallback in the installation flow around
the renameSync call to copy extractedBinary to a unique staging file inside
binDir, then atomically rename that staged file to this.binaryPath and remove
the original temporary binary. Ensure cleanup handles the staging file
appropriately while preserving the existing direct rename path for
same-filesystem moves.
🪄 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: ca894959-bcbe-482d-8c63-1a91f10372e8
📒 Files selected for processing (25)
.changeset/tidy-mugs-repair.mdREADME.mdsrc/cfg/setting.tssrc/cli.tssrc/cmd/config.tssrc/cmd/debug.tssrc/cmd/deploy.tssrc/cmd/devnet-config.tssrc/cmd/node.tssrc/cmd/transfer-all.tssrc/cmd/transfer.tssrc/cmd/udt.tssrc/deploy/index.tssrc/devnet/fork.tssrc/sdk/ckb.tssrc/tools/ckb-tui.tssrc/util/fork-safety.tssrc/util/validator.tstests/debug-tx-file.test.tstests/devnet-fork.test.tstests/fork-safety.test.tstests/node-command.test.tstests/transfer-all.test.tstests/udt.test.tstests/validator.test.ts
- Keep --allow-mainnet-replay-risk as a hidden deprecated alias folded into --allow-external-key-on-mainnet-fork (with a deprecation warning) so 0.4.9 scripts keep working under a patch release - Treat lsof probe failures with stderr output as indeterminate (null) instead of "not listening"; only an empty-stderr exit is a genuine no-match, so permission errors fall back to the weaker genesis signal - Reject a symlinked data root before enumerating source chain data - Stage cross-device ckb-tui installs inside binDir and publish with an atomic rename, so concurrent installs never see a truncated binary Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/node-listener.test.ts (1)
48-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact
lsofinvocation.These tests verify outcome mapping but never verify the PID/port filters and
LISTENselector required bysrc/cmd/node.ts:198-211. A wrong command could still leave the suite green and allow readiness checks to inspect the wrong process.Proposed test assertion
expect(isProcessListeningOnPort(1234, 8114)).toBe(true); + expect(mockExecFileSync).toHaveBeenCalledWith( + 'lsof', + ['-a', '-p', '1234', '-iTCP:8114', '-sTCP:LISTEN'], + { stdio: ['ignore', 'pipe', 'pipe'] }, + );🤖 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/node-listener.test.ts` around lines 48 - 50, Update the isProcessListeningOnPort test cases to assert mockExecFileSync receives the exact lsof executable, PID/port filters, and LISTEN selector used by the implementation. Cover the successful invocation assertion alongside the existing true outcome, and apply the same command verification consistently to relevant result-mapping tests.
🤖 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/node.ts`:
- Around line 201-209: Update the synchronous lsof probe in
clearForkFirstRunWhenNodeUp to pass a short timeout option to execFileSync. In
its catch handling, detect timeout failures and return null so the existing
fallback path proceeds, while preserving the current ENOENT and stderr-based
behavior for other failures.
---
Nitpick comments:
In `@tests/node-listener.test.ts`:
- Around line 48-50: Update the isProcessListeningOnPort test cases to assert
mockExecFileSync receives the exact lsof executable, PID/port filters, and
LISTEN selector used by the implementation. Cover the successful invocation
assertion alongside the existing true outcome, and apply the same command
verification consistently to relevant result-mapping tests.
🪄 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: 8a7e43ce-487d-4583-8d3c-90efec99fbfb
📒 Files selected for processing (11)
.changeset/tidy-mugs-repair.mdREADME.mdsrc/cli.tssrc/cmd/node.tssrc/devnet/fork.tssrc/tools/ckb-tui.tssrc/util/fork-safety.tstests/cli-mainnet-fork-flag.test.tstests/devnet-fork.test.tstests/fork-safety.test.tstests/node-listener.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/tidy-mugs-repair.md
- README.md
A hung lsof would block execFileSync (and with it daemon startup) indefinitely, and its empty-stderr timeout error would be misread as a genuine no-match. Cap the probe at 5s and classify ETIMEDOUT as indeterminate (null) so the genesis fallback proceeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/node-listener.test.ts`:
- Around line 81-88: Update the timeout assertion in the “bounds the lsof probe
with a timeout” test to require exactly 5000 milliseconds instead of accepting
any number, while preserving the existing lsof arguments and invocation checks.
🪄 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: 55e36f0b-5f50-4e4e-9018-57844c45db9d
📒 Files selected for processing (2)
src/cmd/node.tstests/node-listener.test.ts
isProcessListeningOnPort short-circuits to null on win32, so the lsof outcome-mapping tests failed on the Windows CI runner (mock never called). Force a unix platform for the lsof-probing cases, cover the win32 short-circuit explicitly, and pin the probe timeout to exactly 5000 ms per review feedback.
This was referenced Jul 24, 2026
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
3 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.
Closes #460
Closes #462
What
#460 — parameter naming:
--allow-mainnet-replay-riskis renamed to--allow-external-key-on-mainnet-fork(CLI flag, option types, error messages, README, tests).#462 — leftover review items from #461 (0.4.9): the PR was merged before the CodeRabbit review comments were addressed; this applies the still-valid ones:
transfer-all,udt issue,udt destroy, anddeploynow go throughvalidateMainnetForkSigningand thread the returned fork boundary into the SDK, which rejects inputs created at or before the boundary viaassertInputsCreatedAfter— mirroringtransfer/deposit. The renamed--allow-external-key-on-mainnet-forkflag is now available on all four commands.debug --tx-hashpath traversal: the hash is validated as a 0x-prefixed 32-byte hex string before it is interpolated into cache paths.get_tip_block_numberonce the spawned CKB process is the RPC listener (the fork keeps the source chain's genesis hash, so genesis equality alone doesn't identify our node). Falls back to the genesis check where the listener can't be determined (Windows / nolsof).copySourceDatarejects symlinked entries (top-level and nested) instead of silently copying data from outside the source directory.maxCellsbudget so a large SUDT set can't silently drop xUDT balances.readSettingsfallbacks return deep clones of the defaults;config settry/catch narrowed so I/O errors aren't mislabeled as validation failures (and no doubled prefix);devnet configrethrows the originalInitializationError; process command-line lookup usesexecFilewith arg arrays; ckb-tui download timeout constants aligned and cross-device (EXDEV) installs handled; README TOC gains the missing step-7 fork entry; unused test binding removed.New/updated tests cover the flag rename, boundary threading for
transfer-all/udt issue/udt destroy, tx-hash rejection, symlink rejection, and extended xUDT args.pnpm typecheck,pnpm lint,pnpm test(25 suites) andpnpm buildall pass.Changeset included (patch).