Skip to content

fix canary daemon, UDT, and fork reliability#457

Merged
RetricSu merged 8 commits into
developfrom
agent/fix-canary-devrel-findings
Jul 21, 2026
Merged

fix canary daemon, UDT, and fork reliability#457
RetricSu merged 8 commits into
developfrom
agent/fix-canary-devrel-findings

Conversation

@RetricSu

@RetricSu RetricSu commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • fix fresh-install daemon initialization and make node, miner, proxy, PID, and RPC readiness share one supervised lifecycle
  • use the full 32-byte owner lock hash for SUDT type args; reject zero/invalid UDT amounts before client construction and return semantic issuance details
  • harden forked devnets with an explicit source directory, peer-store exclusion, outbound network isolation, spec/genesis/database preflight, copy-only migration, and rollback
  • add offckb devnet info, Indexer lag warnings, fork-aware address prefixes/balances, hidden-by-default private keys, and Mainnet replay-risk banners
  • make Mainnet-fork transfers and deposits fail closed for copied inputs, with an explicit transfer replay-risk override for external keys
  • make --json position-independent with one result object on stdout, NDJSON logs/errors on stderr, stable non-zero failures, no stacks, and EPIPE handling
  • require independently pinned SHA-256 verification for ckb-tui downloads and add --privkey-file / OFFCKB_PRIVATE_KEY support
  • make daemon startup ownership atomic, verify PID identity, preserve startup state through readiness, and clean up every post-spawn failure
  • update README guidance and add a patch changeset

Validation

  • npm test -- --runInBand: 24 suites passed, 181 tests passed, 7 skipped
  • npm run lint: no errors (4 pre-existing no-explicit-any warnings)
  • npm run build: passed
  • real CKB v0.207.0 isolated-config smoke: RPC healthy and get_peers returned []
  • real SUDT issue on a local devnet: transaction committed with 32-byte type args
  • real stopped Mainnet data dry-run: detected Mainnet/genesis, old DB migration, and source peer store without modifying the devnet
  • JSON CLI smoke: exactly one stdout result, structured stderr failure, non-zero invalid-option exit, and clean EPIPE exit

Fixes #456

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The CLI now centralizes async execution and JSON results, strengthens devnet readiness and fork isolation, supports safer private-key inputs, updates UDT validation and issuance results, improves daemon lifecycle handling, and enforces ckb-tui checksum verification. Tests and documentation cover the new behavior.

Changes

Canary DevRel fixes

Layer / File(s) Summary
CLI execution and structured output
src/cli.ts, src/util/logger.ts, src/cmd/*, tests/*
Commands now propagate errors, support runCli(...), and emit stable JSON success or failure records.
Node readiness and lifecycle
src/cmd/node.ts, src/cmd/status.ts, src/devnet/readiness.ts, src/node/init-chain.ts, tests/node-*, tests/readiness*.test.ts
Startup, daemon, stop, status, and initialization flows use RPC readiness checks and coordinated process handling.
Fork discovery, preflight, and isolation
src/devnet/fork.ts, src/util/fs.ts, tests/devnet-fork.test.ts, README.md
Forks support migration checks, dry runs, persistent-data copying, network isolation, and expanded fork state.
Accounts, keys, UDT, and transaction results
src/cmd/*, src/util/*, src/sdk/ckb.ts, tests/*
Commands support file-based key resolution, fork/indexer checks, stricter UDT validation, input-boundary checks, and structured transaction outputs.
TUI verification and documentation
src/tools/ckb-tui.ts, tests/ckb-tui-checksum.test.ts, README.md, .changeset/*
ckb-tui downloads fail closed without a trusted checksum, and CLI, fork, account, and release documentation was updated.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • ckb-devrel/offckb#453 — Shares the devnet fork implementation, fork state handling, and first-run node behavior.

Suggested reviewers: humble-little-bear

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes cover the linked issue's core needs: daemon supervision, 32-byte SUDT args, fork isolation/preflight, JSON output, private keys, and ckb-tui checks.
Out of Scope Changes check ✅ Passed I don't see clear out-of-scope code changes; the extra error-handling updates support the stated reliability and JSON goals.
Title check ✅ Passed The title clearly matches the main focus on daemon, UDT, and fork reliability fixes.
Description check ✅ Passed The description is detailed and directly aligned with the changeset, covering the same reliability and documentation updates.

Comment @coderabbitai help to get the list of available commands.

@RetricSu
RetricSu marked this pull request as ready for review July 20, 2026 08:19

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/tools/ckb-tui.ts (1)

221-244: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Security Misconfiguration (CWE-345)

Reachability: External
● Entry
  src/cmd/status.ts:37
  CKBTui
│
▼
● Sink
  src/tools/ckb-tui.ts

Do not trust checksums-sha256.txt fetched from the same release for arbitrary versions. v0.1.3 is pinned via KNOWN_SHA256, but the fallback path still accepts a manifest that can be replaced together with the archive if that release is compromised. Keep non-default versions pinned here, or verify a signed manifest with an embedded publisher key.

🤖 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/ckb-tui.ts` around lines 221 - 244, Update the checksum
verification flow around checksumUrl and fetchResult so arbitrary non-default
versions cannot trust checksums-sha256.txt fetched from the same release. Reuse
the existing KNOWN_SHA256 pinning for every supported non-default version, or
require verification of the manifest with an embedded publisher key before
accepting expectedHash; preserve assertChecksum only after the checksum source
is independently trusted.
🤖 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/deposit.ts`:
- Around line 30-39: Update the Testnet early-return path in the deposit flow to
capture the hash returned by depositFromTestnetFaucet(), ensure that helper
returns its transferAll transaction hash, and emit logger.result with the same
deposit result shape before returning the hash. Preserve the existing mainnet
transfer reporting and return behavior.

In `@src/cmd/devnet-info.ts`:
- Around line 35-36: Update the readiness logging around indexerLag so positive
lag values use logger.warn instead of logger.info, while retaining logger.info
for zero or non-positive values and the existing null handling.

In `@src/cmd/node.ts`:
- Around line 130-138: Re-check the ckbExited state after createRPCProxy and
before proxy.start() in the miner startup flow. If CKB has already exited, stop
or skip starting the proxy and preserve the existing shutdown/error behavior;
otherwise continue starting the proxy normally.

In `@src/cmd/transfer.ts`:
- Around line 21-28: Update the UDT option handling around udtKind, udtTypeArgs,
and ckb.transfer() so either --udt-kind or --udt-type-args explicitly selects
UDT processing. Require non-empty type arguments, including rejecting an empty
string, before validating and proceeding; only use the CKB transfer path when
neither UDT option is provided.
- Around line 31-33: Update the transfer flow around warnIfMainnetForkSigning
and CKB construction to fail closed on Mainnet forks: require an explicit
replay-risk override when signing with a non-built-in private key, and reject
any selected input created before the fork point. Ensure these validations occur
before transaction construction or signing, while preserving the existing
warning and indexer checks where applicable.

In `@src/util/logger.ts`:
- Around line 72-82: Update the UnifiedLogger constructor initialization to call
setJsonMode(this.jsonMode) immediately after createLogger(), ensuring JSON-mode
Console transports configure stderrLevels before any logs are emitted.

---

Outside diff comments:
In `@src/tools/ckb-tui.ts`:
- Around line 221-244: Update the checksum verification flow around checksumUrl
and fetchResult so arbitrary non-default versions cannot trust
checksums-sha256.txt fetched from the same release. Reuse the existing
KNOWN_SHA256 pinning for every supported non-default version, or require
verification of the manifest with an embedded publisher key before accepting
expectedHash; preserve assertChecksum only after the checksum source is
independently trusted.
🪄 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: 85afcabf-c1bf-47ef-925b-fa8dfebfce74

📥 Commits

Reviewing files that changed from the base of the PR and between 325d378 and 71f10f1.

📒 Files selected for processing (46)
  • .changeset/clean-pandas-report.md
  • README.md
  • src/cli.ts
  • src/cmd/accounts.ts
  • src/cmd/balance.ts
  • src/cmd/clean.ts
  • src/cmd/config.ts
  • src/cmd/create.ts
  • src/cmd/debug.ts
  • src/cmd/deploy.ts
  • src/cmd/deposit.ts
  • src/cmd/devnet-config.ts
  • src/cmd/devnet-fork.ts
  • src/cmd/devnet-info.ts
  • src/cmd/node.ts
  • src/cmd/status.ts
  • src/cmd/transfer-all.ts
  • src/cmd/transfer.ts
  • src/cmd/udt.ts
  • src/devnet/fork.ts
  • src/devnet/readiness.ts
  • src/node/init-chain.ts
  • src/node/install.ts
  • src/sdk/ckb.ts
  • src/tools/ckb-tui.ts
  • src/util/fork-safety.ts
  • src/util/fs.ts
  • src/util/logger.ts
  • src/util/private-key.ts
  • src/util/validator.ts
  • tests/accounts.test.ts
  • tests/ckb-tui-checksum.test.ts
  • tests/devnet-config-command.test.ts
  • tests/devnet-fork.test.ts
  • tests/fork-safety.test.ts
  • tests/init-chain.test.ts
  • tests/logger.test.ts
  • tests/node-command.test.ts
  • tests/node-supervisor.test.ts
  • tests/private-key.test.ts
  • tests/readiness-warning.test.ts
  • tests/readiness.test.ts
  • tests/sdk/ckb.udt.test.ts
  • tests/status.test.ts
  • tests/udt.test.ts
  • tests/validator.test.ts

Comment thread src/cmd/deposit.ts Outdated
Comment thread src/cmd/devnet-info.ts Outdated
Comment thread src/cmd/node.ts
Comment thread src/cmd/transfer.ts
Comment thread src/cmd/transfer.ts Outdated
Comment thread src/util/logger.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/cmd/deposit.ts (1)

32-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Other (CWE-693)

Reachability: External
● Entry
  src/cli.ts:158
  deposit
│
▼
● Sink
  src/cmd/deposit.ts

Enforce the fork boundary before depositing.

This only logs a replay-risk warning. Unlike transfer, it never calls validateMainnetForkSigning or passes rejectInputsAtOrBeforeBlock to CKB.transfer, so a Mainnet-fork deposit can sign copied inputs without the required origin check.

Proposed fix
-  warnIfMainnetForkSigning(network, privateKey);
+  const rejectInputsAtOrBeforeBlock = validateMainnetForkSigning(network, privateKey);
   await warnIfForkIndexerIsBehind(network);
   const txHash = await ckb.transfer({
     toAddress,
     privateKey,
     amountInCKB,
+    rejectInputsAtOrBeforeBlock,
   });
🤖 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/deposit.ts` around lines 32 - 38, Update the deposit flow around
warnIfMainnetForkSigning and ckb.transfer to enforce the fork boundary, not just
warn: validate Mainnet-fork signing with validateMainnetForkSigning and pass the
resulting rejectInputsAtOrBeforeBlock constraint into CKB.transfer, matching the
existing transfer implementation.
src/cmd/node.ts (3)

411-428: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make daemon startup check-and-claim atomic.

Two concurrent startDaemon() calls can both pass the node/PID checks, then spawn competing daemons and overwrite or race on the PID file. Use an OS-level startup lock or atomically reserve the control file before spawning.

🤖 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 411 - 428, Update startDaemon to atomically
acquire an OS-level startup lock or reserve the daemon control/PID file before
spawning the process, so concurrent calls cannot both pass the readiness and
existing-PID checks. Ensure the reservation is created exclusively, reject
startup when another invocation owns it, and release or clean it up on startup
failure and daemon shutdown while preserving the existing duplicate-daemon
errors.

490-522: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Keep the PID file until the daemon exits. terminateProcess() only sends SIGTERM here, so cleanupPidFile() can run while the child is still shutting down. Wait for exit first, and only remove the PID file once termination is confirmed.

🤖 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 490 - 522, Update the failed-readiness cleanup
around terminateProcess and cleanupPidFile to wait until the child process has
exited after sending SIGTERM, then remove the PID file only after termination is
confirmed. Preserve the existing handling for an already-exited child and the
readiness error propagation.

542-564: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Don't treat EPERM as a stale PID in src/cmd/node.ts. isProcessAlive() currently turns any process.kill(pid, 0) error into false, so a live daemon you can’t signal gets its PID file deleted and reported as stale-pid. Only map ESRCH to stale; let EPERM surface as permission denied.

🤖 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 542 - 564, Update isProcessAlive to distinguish
process.kill(pid, 0) errors: return false only for ESRCH, while allowing EPERM
and other errors to propagate as permission or operational failures. Preserve
the existing stale-pid cleanup and result behavior in the node stop flow only
when the process is confirmed absent.
🧹 Nitpick comments (1)
tests/node-supervisor.test.ts (1)

114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the fork-boundary write happens before miner spawn. This test can still pass if miner startup moves ahead of markForkFirstRunComplete; add an ordering assertion between those calls.

🤖 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-supervisor.test.ts` around lines 114 - 126, Update the test around
nodeDevnet to assert that mockMarkForkFirstRunComplete is invoked before
mockProxyStart, using invocation ordering metadata or an equivalent call-order
assertion while preserving the existing argument and startup expectations.
🤖 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/deposit.ts`:
- Around line 25-27: Update the result object in the deposit command after
depositFromTestnetFaucet so it reports the requested amount under
requestedAmount, removes amountInCKB from the amount field, and identifies the
result as the fixed faucet claim instead. Keep the existing txHash, network, and
toAddress fields unchanged.

In `@src/util/fork-safety.ts`:
- Around line 43-46: Update the fork boundary parsing in the shown try block to
validate that the BigInt result is non-negative before returning it. Treat
negative values as invalid and route them through the existing error handling,
while preserving valid non-negative boundary behavior.

---

Outside diff comments:
In `@src/cmd/deposit.ts`:
- Around line 32-38: Update the deposit flow around warnIfMainnetForkSigning and
ckb.transfer to enforce the fork boundary, not just warn: validate Mainnet-fork
signing with validateMainnetForkSigning and pass the resulting
rejectInputsAtOrBeforeBlock constraint into CKB.transfer, matching the existing
transfer implementation.

In `@src/cmd/node.ts`:
- Around line 411-428: Update startDaemon to atomically acquire an OS-level
startup lock or reserve the daemon control/PID file before spawning the process,
so concurrent calls cannot both pass the readiness and existing-PID checks.
Ensure the reservation is created exclusively, reject startup when another
invocation owns it, and release or clean it up on startup failure and daemon
shutdown while preserving the existing duplicate-daemon errors.
- Around line 490-522: Update the failed-readiness cleanup around
terminateProcess and cleanupPidFile to wait until the child process has exited
after sending SIGTERM, then remove the PID file only after termination is
confirmed. Preserve the existing handling for an already-exited child and the
readiness error propagation.
- Around line 542-564: Update isProcessAlive to distinguish process.kill(pid, 0)
errors: return false only for ESRCH, while allowing EPERM and other errors to
propagate as permission or operational failures. Preserve the existing stale-pid
cleanup and result behavior in the node stop flow only when the process is
confirmed absent.

---

Nitpick comments:
In `@tests/node-supervisor.test.ts`:
- Around line 114-126: Update the test around nodeDevnet to assert that
mockMarkForkFirstRunComplete is invoked before mockProxyStart, using invocation
ordering metadata or an equivalent call-order assertion while preserving the
existing argument and startup expectations.
🪄 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: f82d4af5-df9a-4131-9ea9-7a2f964a862d

📥 Commits

Reviewing files that changed from the base of the PR and between 71f10f1 and 0a87ccf.

📒 Files selected for processing (20)
  • README.md
  • src/cli.ts
  • src/cmd/deposit.ts
  • src/cmd/devnet-info.ts
  • src/cmd/node.ts
  • src/cmd/transfer.ts
  • src/devnet/fork.ts
  • src/sdk/ckb.ts
  • src/tools/ckb-tui.ts
  • src/util/fork-safety.ts
  • src/util/logger.ts
  • tests/ckb-tui-checksum.test.ts
  • tests/deposit.test.ts
  • tests/devnet-fork.test.ts
  • tests/devnet-info.test.ts
  • tests/fork-safety.test.ts
  • tests/logger.test.ts
  • tests/node-supervisor.test.ts
  • tests/sdk/ckb.udt.test.ts
  • tests/udt.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • tests/ckb-tui-checksum.test.ts
  • src/cmd/devnet-info.ts
  • src/cmd/transfer.ts
  • README.md
  • src/util/logger.ts
  • src/cli.ts

Comment thread src/cmd/deposit.ts
Comment thread src/util/fork-safety.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
tests/node-command.test.ts (1)

237-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert cleanup happens after termination is confirmed.

This test only proves both events occurred; it passes if the PID file is removed before the liveness retries. Record the liveness count when mockUnlinkSync runs and assert it is at least 3.

Proposed test strengthening
   let livenessChecks = 0;
+  let livenessChecksWhenPidFileRemoved: number | undefined;
+  mockUnlinkSync.mockImplementation((file: string) => {
+    if (file === pidFile) livenessChecksWhenPidFileRemoved = livenessChecks;
+  });
   mockWaitForNodeReady.mockResolvedValueOnce({ ready: false, error: 'proxy unavailable' });
   killSpy.mockImplementation((_pid: number, signal?: NodeJS.Signals | number) => {
...
   expect(killSpy).toHaveBeenCalledWith(-12345, 'SIGTERM');
   expect(livenessChecks).toBeGreaterThanOrEqual(3);
   expect(mockUnlinkSync).toHaveBeenCalledWith(pidFile);
+  expect(livenessChecksWhenPidFileRemoved).toBeGreaterThanOrEqual(3);
🤖 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-command.test.ts` around lines 237 - 256, Strengthen the test
around startNode by recording livenessChecks when mockUnlinkSync executes, then
assert that recorded count is at least 3 after the rejected startup. Keep the
existing SIGTERM, retry-count, and PID-file assertions, ensuring cleanup is
verified only after termination confirmation.
src/cmd/node.ts (1)

554-572: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Escalate to SIGKILL when the failed child won't respond to SIGTERM.

Unlike stopNode (which escalates to SIGKILL after a 5s SIGTERM wait), this readiness-failure path only sends SIGTERM once. If the child ignores it (hung/stuck CKB process), exited stays false, the PID file is deliberately preserved, and the process is left running indefinitely — the caller must manually run offckb node stop to fully clean it up. Mirroring stopNode's SIGTERM→wait→SIGKILL pattern here would make startup-failure cleanup self-sufficient and consistent with the stop path.

♻️ Proposed fix
   if (!readiness.ready) {
     let exited = !isProcessAlive(child.pid);
     try {
       if (!exited) {
         await terminateProcess(child.pid, 'SIGTERM');
         exited = await waitForProcessExit(child.pid, 5000);
+        if (!exited) {
+          await terminateProcess(child.pid, 'SIGKILL');
+          exited = await waitForProcessExit(child.pid, 5000);
+        }
       }
     } catch {
       // The failed child may already have exited.
       exited = !isProcessAlive(child.pid);
     }
🤖 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 554 - 572, Update the readiness-failure cleanup
around terminateProcess and waitForProcessExit to mirror stopNode’s escalation
flow: send SIGTERM, wait up to 5 seconds, then send SIGKILL and wait again if
the child remains alive. Set exited from the final wait result so the existing
PID-file cleanup and error reporting correctly reflect whether the process was
terminated.
🤖 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 554-572: Update the readiness-failure cleanup around
terminateProcess and waitForProcessExit to mirror stopNode’s escalation flow:
send SIGTERM, wait up to 5 seconds, then send SIGKILL and wait again if the
child remains alive. Set exited from the final wait result so the existing
PID-file cleanup and error reporting correctly reflect whether the process was
terminated.

In `@tests/node-command.test.ts`:
- Around line 237-256: Strengthen the test around startNode by recording
livenessChecks when mockUnlinkSync executes, then assert that recorded count is
at least 3 after the rejected startup. Keep the existing SIGTERM, retry-count,
and PID-file assertions, ensuring cleanup is verified only after termination
confirmation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a13a6d89-07d0-46d6-a762-738d245a4868

📥 Commits

Reviewing files that changed from the base of the PR and between 0a87ccf and f1f652b.

📒 Files selected for processing (7)
  • src/cmd/deposit.ts
  • src/cmd/node.ts
  • src/util/fork-safety.ts
  • tests/deposit.test.ts
  • tests/fork-safety.test.ts
  • tests/node-command.test.ts
  • tests/node-supervisor.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/deposit.test.ts
  • tests/fork-safety.test.ts
  • src/util/fork-safety.ts
  • tests/node-supervisor.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/cmd/node.ts (4)

471-486: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Verify daemon identity before treating a live PID as an existing daemon.

A crashed daemon’s PID can be reused by an unrelated process. This branch then refuses every new startup even though no CKB daemon is running. Use verifyDaemonIdentity(existing.pid, existing) before rejecting; if it fails, remove or quarantine only the stale metadata without signaling that process.

🤖 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 471 - 486, Update the existing-PID handling
around readPidFile and isProcessAlive to call verifyDaemonIdentity(existing.pid,
existing) before rejecting a live PID. Only throw the duplicate-daemon errors
when the identity check succeeds; otherwise treat the PID metadata as stale,
clean up or quarantine the PID file, and never signal or affect the unrelated
process.

542-553: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Clean up the detached child on every post-spawn startup failure.

If writePidFile, readForkState, or waitForNodeReady throws, execution bypasses the !readiness.ready cleanup and leaves the detached daemon orphaned with stale PID metadata. Wrap the entire post-spawn readiness phase in a cleanup path that terminates the child, waits/escalates, and removes the PID file before rethrowing.

🤖 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 542 - 553, Wrap the entire post-spawn readiness
phase in the relevant node startup function, including writePidFile,
readForkState, and waitForNodeReady, with error cleanup. On any thrown error,
terminate the detached child using the existing wait/escalation behavior, remove
the PID file, and rethrow the original error; preserve the existing
!readiness.ready cleanup path without duplicating cleanup.

578-591: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep JSON mode to one stdout result.

The success and info logs run before logger.result; in JSON mode they can emit additional records, violating the one-structured-result contract. Suppress these human-readable logs in JSON mode or route them to stderr, leaving logger.result as the only stdout record.

🤖 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 578 - 591, The devnet daemon startup path emits
human-readable success and info logs before logger.result, breaking JSON mode’s
single stdout result contract. Update the logging around the visible
logger.success and logger.info calls to suppress them in JSON mode or route them
to stderr, while keeping logger.result as the only stdout record.

536-542: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep the PID status as starting until readiness succeeds.

The daemon is marked running immediately after spawn, before CKB, miner, and proxy readiness completes. A concurrent stop or second start can therefore bypass the explicit starting handling. Write status: 'running' only after the readiness check passes.

🤖 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 536 - 542, Update the metadata creation in the
node startup flow to write status "starting" after spawning the child, and defer
changing it to "running" until the CKB, miner, and proxy readiness check
succeeds. Ensure the persisted PID metadata is updated after readiness so
concurrent stop and start operations continue to recognize the daemon as
starting beforehand.
🤖 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.

Outside diff comments:
In `@src/cmd/node.ts`:
- Around line 471-486: Update the existing-PID handling around readPidFile and
isProcessAlive to call verifyDaemonIdentity(existing.pid, existing) before
rejecting a live PID. Only throw the duplicate-daemon errors when the identity
check succeeds; otherwise treat the PID metadata as stale, clean up or
quarantine the PID file, and never signal or affect the unrelated process.
- Around line 542-553: Wrap the entire post-spawn readiness phase in the
relevant node startup function, including writePidFile, readForkState, and
waitForNodeReady, with error cleanup. On any thrown error, terminate the
detached child using the existing wait/escalation behavior, remove the PID file,
and rethrow the original error; preserve the existing !readiness.ready cleanup
path without duplicating cleanup.
- Around line 578-591: The devnet daemon startup path emits human-readable
success and info logs before logger.result, breaking JSON mode’s single stdout
result contract. Update the logging around the visible logger.success and
logger.info calls to suppress them in JSON mode or route them to stderr, while
keeping logger.result as the only stdout record.
- Around line 536-542: Update the metadata creation in the node startup flow to
write status "starting" after spawning the child, and defer changing it to
"running" until the CKB, miner, and proxy readiness check succeeds. Ensure the
persisted PID metadata is updated after readiness so concurrent stop and start
operations continue to recognize the daemon as starting beforehand.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a704f7e6-c4f0-4744-89f4-0f04af249805

📥 Commits

Reviewing files that changed from the base of the PR and between 94b2c07 and 5afeb30.

📒 Files selected for processing (2)
  • src/cmd/node.ts
  • tests/node-command.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/node-command.test.ts

@RetricSu
RetricSu merged commit 8f8d0a4 into develop Jul 21, 2026
7 checks passed
@RetricSu
RetricSu deleted the agent/fix-canary-devrel-findings branch July 21, 2026 02:56
RetricSu added a commit that referenced this pull request Jul 21, 2026
* fix canary daemon, UDT, and fork reliability (#457)

* fix canary devrel reliability issues

* fix fork copy isolation on windows

* remove implicit fork source discovery

* address PR review feedback

* address follow-up review feedback

* fix daemon test on Windows runners

* complete failed daemon cleanup

* harden daemon startup recovery

* chore(release): bump version to 0.4.9 (#458)

---------

Co-authored-by: humble-little-bear <retric@ckba.build>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Canary QA] Blocking issues and DevRel feedback for daemon, UDT, JSON/status, and forked devnet

1 participant