fix: address CodeRabbit review findings from v0.4.11 merge (#487)

.changeset/review-fixes-v0411.md

@@ -0,0 +1,5 @@
+---
+'@offckb/cli': patch
+---
+
+Fix review findings from the v0.4.11 release merge. `offckb logs script` now scans a wider window before filtering so sparse script entries are not missed, and an unknown `offckb logs` target is rejected instead of silently reading the node log. `offckb logs -f` detects log rotation by inode change, so a rotated-in file that is already larger than the old one is re-read from the start. `offckb logs --tail` rejects zero and negative values instead of printing the entire filtered log. CKB 0.205.0 prerelease binaries (rc builds), including prereleases of newer versions, are now correctly treated as Terminal-RPC capable during chain init. A ckb-tui binary that lost its execute bit is reinstalled instead of failing at spawn time. The RPC proxy keeps appending events when a proxy.log rollover fails (previously all further events were dropped) and preserves the previous archive when the rollover's rename fails, records batched JSON-RPC requests including batched `send_transaction`, skips malformed batch members without aborting the rest of the batch, and warns instead of misreporting a parse error when `send_transaction` has no usable params. `--verbose` on `offckb node` now also enables the proxy's per-request debug lines.

src/cli.ts

@@ -111,8 +111,8 @@ program
   .option('--grep <pattern>', 'Only show lines containing the given text')
   .option('--tail <lines>', 'Show the last N lines before following', (value: string) => {
     const parsed = Number(value);
-    if (!Number.isInteger(parsed) || parsed < 0) {
-      throw new InvalidArgumentError('--tail must be a non-negative integer');
+    if (!Number.isInteger(parsed) || parsed <= 0) {
+      throw new InvalidArgumentError('--tail must be a positive integer');
     }
     return parsed;
   })

src/cmd/logs.ts

@@ -19,6 +19,10 @@ export interface LogsOptions {
 }
 
 const DEFAULT_TAIL = 100;
+// Script entries are sparse in run.log (a devnet node writes many non-script
+// lines per second), so a script-filtered tail scans a much wider window and
+// trims back to `tail` after filtering.
+const SCRIPT_SCAN_FACTOR = 100;
 
 /**
  * Print (and optionally follow) a devnet log file. The core is synchronous so
@@ -28,10 +32,16 @@ const DEFAULT_TAIL = 100;
 export function showLogs(target: LogTarget, options: LogsOptions, settings: Settings, logger: UnifiedLogger): void {
   const filePath = resolveLogPath(target, settings);
   const tail = options.tail ?? DEFAULT_TAIL;
-
-  let lines = readLogTail(filePath, tail);
+  // A zero tail is rejected, not "print nothing": slice(-0) is slice(0), so
+  // script mode would otherwise dump every filtered line it scanned.
+  if (!Number.isInteger(tail) || tail <= 0) {
+    throw new Error(`--tail must be a positive integer (got ${options.tail})`);
+  }
   const scriptOnly = target === 'script';
-  if (scriptOnly) lines = filterLinesByTarget(lines, SCRIPT_LOG_TARGET);
+
+  const scanWindow = scriptOnly ? tail * SCRIPT_SCAN_FACTOR : tail;
+  let lines = readLogTail(filePath, scanWindow);
+  if (scriptOnly) lines = filterLinesByTarget(lines, SCRIPT_LOG_TARGET).slice(-tail);
   if (options.grep) lines = grepLines(lines, options.grep);
   for (const line of lines) logger.info(line);
 
@@ -52,6 +62,9 @@ export function showLogs(target: LogTarget, options: LogsOptions, settings: Sett
 }
 
 export function logsCommand(target: string | undefined, options: LogsOptions): void {
-  const resolved: LogTarget = LOG_TARGETS.includes(target as LogTarget) ? (target as LogTarget) : 'node';
+  if (target != null && !LOG_TARGETS.includes(target as LogTarget)) {
+    throw new Error(`Unknown log target '${target}'. Use one of: ${LOG_TARGETS.join(', ')}.`);
+  }
+  const resolved: LogTarget = (target as LogTarget) ?? 'node';
   showLogs(resolved, options, readSettings(), defaultLogger);
 }

src/cmd/node.ts

@@ -203,7 +203,7 @@ export async function nodeDevnet({ version, binaryPath, daemon, verbose }: NodeP
     throw new Error('CKB devnet exited while the miner was starting.');
   }
 
-  const proxy = createRPCProxy(Network.devnet, settings.devnet.rpcUrl, settings.devnet.rpcProxyPort);
+  const proxy = createRPCProxy(Network.devnet, settings.devnet.rpcUrl, settings.devnet.rpcProxyPort, { verbose });
   proxy.start();
 
   // Contract script debug output (debug! in scripts) streams live over the

src/devnet/log-file.ts

@@ -122,7 +122,10 @@ export function followLogFile(filePath: string, onLine: (line: string) => void,
   let decoder = new TextDecoder('utf-8');
 
   const onChange = (curr: fs.Stats, prev: fs.Stats) => {
-    if (curr.size < prev.size || curr.size < offset) {
+    // Rotation swaps in a new inode at the same path; a replacement that is
+    // already larger than the old file fools the size checks below, so the
+    // inode change must also reset the offset.
+    if (curr.size < prev.size || curr.size < offset || curr.ino !== prev.ino) {
       // Truncated or rotated: restart from the beginning.
       offset = 0;
       partial = '';

src/node/init-chain.ts

@@ -70,10 +70,14 @@ export const TERMINAL_RPC_MIN_CKB_VERSION = '0.205.0';
 
 // Unknown (null/undefined/unparseable) versions keep the historical behavior
 // of assuming support — a custom binary whose version cannot be probed must
-// not lose functionality it may actually have.
+// not lose functionality it may actually have. The `-0` range suffix opts
+// 0.205.0 prereleases (rc builds already carry the Terminal module) into the
+// match; plain semver.gte would exclude them. includePrerelease extends that
+// to prereleases of NEWER versions (0.205.1-rc1, 0.206.0-rc1, ...), which the
+// tuple rule would otherwise reject even though they are past the minimum.
 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`, { includePrerelease: true });
 }
 
 // Whether the chain's ckb.toml currently enables the Terminal RPC module.

src/tools/ckb-tui.ts

@@ -127,6 +127,11 @@ export class CKBTui {
       // Metadata alone does not prove readability; an unreadable binary must
       // flow into the reinstall path (which republishes with correct modes).
       fs.accessSync(binaryPath, fs.constants.R_OK);
+      // The binary is spawned directly, so a lost execute bit must also count
+      // as a mismatch. Windows reports X_OK for any readable file.
+      if (process.platform !== 'win32') {
+        fs.accessSync(binaryPath, fs.constants.X_OK);
+      }
       if (!expected) {
         return true;
       }

src/tools/proxy-events.ts

@@ -63,9 +63,28 @@ export function createProxyEventLog(filePath: string, maxBytes = PROXY_LOG_MAX_B
         const line = `${new Date().toISOString()} ${sanitizeEventText(text)}\n`;
         if (size < 0) size = fs.existsSync(filePath) ? fs.statSync(filePath).size : 0;
         if (size > 0 && size + Buffer.byteLength(line) > maxBytes) {
-          fs.rmSync(`${filePath}.1`, { force: true });
-          fs.renameSync(filePath, `${filePath}.1`);
-          size = 0;
+          try {
+            // The previous archive is moved aside rather than deleted up
+            // front: if the active-log rename then fails, the archive is
+            // restored instead of permanently lost.
+            const archivePath = `${filePath}.1`;
+            const backupPath = `${filePath}.1.bak`;
+            fs.rmSync(backupPath, { force: true });
+            const hadArchive = fs.existsSync(archivePath);
+            if (hadArchive) fs.renameSync(archivePath, backupPath);
+            try {
+              fs.renameSync(filePath, archivePath);
+            } catch (error) {
+              if (hadArchive) fs.renameSync(backupPath, archivePath);
+              throw error;
+            }
+            fs.rmSync(backupPath, { force: true });
+            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;
+          }
         }
         fs.appendFileSync(filePath, line);
         size += Buffer.byteLength(line);
@@ -87,33 +106,52 @@ export function handleProxyRequestBody(reqData: string, ctx: ProxyEventContext):
   if (reqData.length === 0) return;
 
   try {
-    const jsonRpcContent = JSON.parse(reqData) as JsonRpcRequestPayload;
-    const method = jsonRpcContent.method;
-    const params = jsonRpcContent.params;
-    ctx.sink.debug('RPC Req: ', method);
-    if (typeof method === 'string') {
-      ctx.events.event(`request ${method}`);
-    }
-
-    if (method === 'send_transaction') {
-      const tx = (params as unknown[])[0];
-      const txHash = ctx.hashTransaction(tx);
-      if (!fs.existsSync(ctx.transactionsPath)) {
-        fs.mkdirSync(ctx.transactionsPath, { recursive: true });
+    // A JSON-RPC batch request is an array of payloads; normalize so batched
+    // send_transaction calls are recorded like single ones.
+    const parsed = JSON.parse(reqData) as JsonRpcRequestPayload | JsonRpcRequestPayload[];
+    for (const jsonRpcContent of Array.isArray(parsed) ? parsed : [parsed]) {
+      // Batch members are user input: JSON.parse happily yields null, strings,
+      // or nested arrays. Skip anything that is not a plain object so one
+      // malformed member cannot abort the rest of the batch.
+      if (jsonRpcContent == null || typeof jsonRpcContent !== 'object' || Array.isArray(jsonRpcContent)) {
+        ctx.sink.warn('skipping malformed JSON-RPC batch member');
+        continue;
       }
-      const txFile = path.resolve(ctx.transactionsPath, `${txHash}.json`);
-      fs.writeFileSync(txFile, JSON.stringify(tx, null, 2));
-      // The hash line mirrors the RPC method name on purpose: at request time
-      // the proxy does not yet know whether the node will accept the tx, and
-      // any rejection surfaces separately as an RPC error line.
-      ctx.sink.info(`send_transaction: ${txHash}`);
-      ctx.events.event(`send_transaction ${txHash}`);
+      handleOneRequest(jsonRpcContent, ctx);
     }
   } catch (err) {
     ctx.sink.error('Error parsing JSON-RPC req content:', (err as Error).message);
   }
 }
 
+function handleOneRequest(jsonRpcContent: JsonRpcRequestPayload, ctx: ProxyEventContext): void {
+  const method = jsonRpcContent.method;
+  const params = jsonRpcContent.params;
+  ctx.sink.debug('RPC Req: ', method);
+  if (typeof method === 'string') {
+    ctx.events.event(`request ${method}`);
+  }
+
+  if (method === 'send_transaction') {
+    if (!Array.isArray(params) || params.length === 0) {
+      ctx.sink.warn('send_transaction request has no params; skipping tx dump');
+      return;
+    }
+    const tx = params[0];
+    const txHash = ctx.hashTransaction(tx);
+    if (!fs.existsSync(ctx.transactionsPath)) {
+      fs.mkdirSync(ctx.transactionsPath, { recursive: true });
+    }
+    const txFile = path.resolve(ctx.transactionsPath, `${txHash}.json`);
+    fs.writeFileSync(txFile, JSON.stringify(tx, null, 2));
+    // The hash line mirrors the RPC method name on purpose: at request time
+    // the proxy does not yet know whether the node will accept the tx, and
+    // any rejection surfaces separately as an RPC error line.
+    ctx.sink.info(`send_transaction: ${txHash}`);
+    ctx.events.event(`send_transaction ${txHash}`);
+  }
+}
+
 interface JsonRpcErrorPayload {
   error?: { code?: unknown; message?: unknown };
 }

src/tools/rpc-proxy.ts

@@ -2,18 +2,32 @@ import httpProxy from 'http-proxy';
 import http from 'http';
 import { Network } from '../type/base';
 import { readSettings } from '../cfg/setting';
-import { logger } from '../util/logger';
+import { logger, UnifiedLogger } from '../util/logger';
 import { proxyLogPathForNetwork } from '../devnet/log-file';
-import { createProxyEventLog, handleProxyRequestBody, handleProxyResponseBody } from './proxy-events';
+import {
+  createProxyEventLog,
+  handleProxyRequestBody,
+  handleProxyResponseBody,
+  ProxyEventContext,
+} from './proxy-events';
 
 // todo: if we use import this throws error in tsc building
 const { cccA } = require('@ckb-ccc/core/advanced');
 
-export function createRPCProxy(network: Network, targetRpcUrl: string, port: number) {
+export interface RPCProxyOptions {
+  // --verbose also lifts the proxy's per-request "RPC Req" lines from debug
+  // to the console; without it they stay in proxy.log only.
+  verbose?: boolean;
+}
+
+export function createRPCProxy(network: Network, targetRpcUrl: string, port: number, options: RPCProxyOptions = {}) {
   const settings = readSettings();
   const events = createProxyEventLog(proxyLogPathForNetwork(network, settings));
-  const ctx = {
-    sink: logger,
+  // The global logger sits at info level, so a debug-level sink is needed for
+  // verbose runs to actually see the per-request lines.
+  const sink = options.verbose ? UnifiedLogger.create({ level: 'debug', showLevel: false }) : logger;
+  const ctx: ProxyEventContext = {
+    sink,
     events,
     transactionsPath: settings[network].transactionsPath,
     hashTransaction: (tx: unknown) => {

tests/ckb-tui-install.test.ts

@@ -97,7 +97,9 @@ describe('ckb-tui installed-binary verification', () => {
     )[assetName()];
 
   it('keeps an existing binary whose digest matches the configured release', () => {
-    fs.writeFileSync(binaryPath, 'installed ckb-tui');
+    // A real install publishes with mode 0o755; the execute bit is part of
+    // what the verification checks.
+    fs.writeFileSync(binaryPath, 'installed ckb-tui', { mode: 0o755 });
     jest.spyOn(crypto, 'createHash').mockReturnValue({
       update: () => ({ digest: () => pinnedDigest() }),
     } as unknown as crypto.Hash);
@@ -145,7 +147,8 @@ describe('ckb-tui installed-binary verification', () => {
 
   it('falls back to presence-only detection for a release without a pinned binary digest', () => {
     mockVersion.current = 'v9.9.9';
-    fs.writeFileSync(binaryPath, 'any content at all');
+    // Presence-only still means a usable binary: regular, readable, executable.
+    fs.writeFileSync(binaryPath, 'any content at all', { mode: 0o755 });
 
     expect(CKBTui.ensureInstalled()).toBe(binaryPath);
     expect(installSpy).not.toHaveBeenCalled();
@@ -181,6 +184,14 @@ describe('ckb-tui installed-binary verification', () => {
 
     expect(internals.installedBinaryMatches(binaryPath)).toBe(false);
   });
+
+  itPosix('treats a binary without the execute bit as a mismatch', () => {
+    mockVersion.current = 'v9.9.9'; // Unpinned: presence alone must not suffice.
+    // No execute bit anywhere, so X_OK fails even for root.
+    fs.writeFileSync(binaryPath, 'execute-bit-lost ckb-tui', { mode: 0o644 });
+
+    expect(internals.installedBinaryMatches(binaryPath)).toBe(false);
+  });
 });
 
 describe('ckb-tui binary publishing', () => {

tests/init-chain.test.ts

@@ -223,7 +223,12 @@ describe('Terminal RPC module version gating', () => {
     expect(supportsTerminalRpcModule('0.207.0')).toBe(true);
     expect(supportsTerminalRpcModule('0.120.0')).toBe(false);
     expect(supportsTerminalRpcModule('0.204.9')).toBe(false);
-    expect(supportsTerminalRpcModule('0.205.0-rc1')).toBe(false);
+    // 0.205.0 prereleases already ship the Terminal module.
+    expect(supportsTerminalRpcModule('0.205.0-rc1')).toBe(true);
+    expect(supportsTerminalRpcModule('0.204.9-rc1')).toBe(false);
+    // Prereleases of newer versions are past the minimum too.
+    expect(supportsTerminalRpcModule('0.205.1-rc1')).toBe(true);
+    expect(supportsTerminalRpcModule('0.206.0-rc1')).toBe(true);
   });
 
   it('strips Terminal from a freshly initialized template when the binary is too old', async () => {

tests/logs-command.test.ts

@@ -2,7 +2,7 @@ import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
 import winston from 'winston';
-import { showLogs } from '../src/cmd/logs';
+import { logsCommand, showLogs } from '../src/cmd/logs';
 import { defaultSettings, Settings } from '../src/cfg/setting';
 import { UnifiedLogger } from '../src/util/logger';
 
@@ -70,6 +70,17 @@ describe('showLogs', () => {
     expect(transport.logs).toEqual([SCRIPT_LINE]);
   });
 
+  it('finds sparse script entries beyond the raw tail window', () => {
+    const { settings, transport } = fixture();
+    const runLog = path.join(settings.devnet.dataPath, 'logs', 'run.log');
+    // The only script entry sits above the last `tail` lines, so a plain
+    // filter-after-tail would print nothing.
+    fs.writeFileSync(runLog, [SCRIPT_LINE, NODE_LINE, ERROR_LINE, NODE_LINE].join('\n') + '\n');
+    const log = UnifiedLogger.create({ transports: [transport], showLevel: false });
+    showLogs('script', { tail: 2 }, settings, log);
+    expect(transport.logs).toEqual([SCRIPT_LINE]);
+  });
+
   it('reads miner.log for the miner target', () => {
     const { settings, transport } = fixture();
     const log = UnifiedLogger.create({ transports: [transport], showLevel: false });
@@ -91,13 +102,23 @@ describe('showLogs', () => {
     expect(transport.logs).toEqual([ERROR_LINE]);
   });
 
+  it('rejects a zero or negative tail instead of dumping the whole filtered log', () => {
+    const { settings, transport } = fixture();
+    const log = UnifiedLogger.create({ transports: [transport], showLevel: false });
+    // slice(-0) is slice(0): without validation, script mode would print every
+    // filtered line it scanned instead of nothing.
+    expect(() => showLogs('script', { tail: 0 }, settings, log)).toThrow(/positive integer/);
+    expect(() => showLogs('script', { tail: -0 }, settings, log)).toThrow(/positive integer/);
+    expect(() => showLogs('node', { tail: -5 }, settings, log)).toThrow(/positive integer/);
+    expect(transport.logs).toEqual([]);
+  });
+
   it('throws a helpful error when the log file does not exist', () => {
     const settings = JSON.parse(JSON.stringify(defaultSettings)) as Settings;
     settings.devnet.dataPath = '/nonexistent';
     const log = UnifiedLogger.create({ transports: [new CapturingTransport()] });
     expect(() => showLogs('node', { tail: 100 }, settings, log)).toThrow(/log file not found/i);
   });
-
   it('in follow mode gates script entries and applies grep to streamed lines', () => {
     const { settings, transport } = fixture();
     type StatListener = (curr: fs.Stats, prev: fs.Stats) => void;
@@ -131,3 +152,10 @@ describe('showLogs', () => {
     }
   });
 });
+
+describe('logsCommand', () => {
+  it('rejects an unknown log target instead of falling back to node', () => {
+    expect(() => logsCommand('scrpit', {})).toThrow(/unknown log target 'scrpit'/i);
+    expect(() => logsCommand('scrpit', {})).toThrow(/node, script, miner, rpc/);
+  });
+});

tests/logs.test.ts

@@ -194,11 +194,25 @@ describe('followLogFile', () => {
       listeners[0](grown, grown);
       expect(seen).toEqual([SCRIPT_LINE]);
 
-      // Rotation replaces the file with a shorter one; the next notification
-      // must reset the offset instead of seeking past the end.
+      // Truncation shrinks the file in place; the next notification must
+      // reset the offset instead of seeking past the end.
       fs.writeFileSync(file, `${ERROR_LINE}\n`);
       listeners[0](fs.statSync(file), grown);
       expect(seen).toEqual([SCRIPT_LINE, ERROR_LINE]);
+
+      fs.appendFileSync(file, `${NODE_LINE}\n`);
+      const regrown = fs.statSync(file);
+      listeners[0](regrown, regrown);
+      expect(seen).toEqual([SCRIPT_LINE, ERROR_LINE, NODE_LINE]);
+
+      // Rotation replaces the file: the old path is renamed away and a new
+      // file appears at the same path, possibly already larger than the old
+      // one. The size checks alone would keep the stale offset, so the inode
+      // change must restart the read from offset 0.
+      fs.renameSync(file, `${file}.1`);
+      fs.writeFileSync(file, [SCRIPT_LINE, ERROR_LINE, NODE_LINE, SCRIPT_LINE].join('\n') + '\n');
+      listeners[0](fs.statSync(file), regrown);
+      expect(seen).toEqual([SCRIPT_LINE, ERROR_LINE, NODE_LINE, SCRIPT_LINE, ERROR_LINE, NODE_LINE, SCRIPT_LINE]);
       stop();
     } finally {
       mockWatchFile.mockReset();