feat: unify devnet logging with offckb logs and a quiet foreground node - #478
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe CLI adds ChangesDevnet logging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant nodeDevnet
participant subscribeToNodeLogs
participant UnifiedLogger
User->>nodeDevnet: start devnet
nodeDevnet->>subscribeToNodeLogs: subscribe to TCP log topic
subscribeToNodeLogs->>UnifiedLogger: emit ckb-script debug entries
User->>UnifiedLogger: run offckb logs
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: 3
🧹 Nitpick comments (4)
src/cli.ts (1)
109-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse commander's
InvalidArgumentErrorinstead of a plainErrorin the--tailparser.Custom option-argument parsers in commander are expected to throw
InvalidArgumentErrorfor validation failures; commander catches that type specifically to print a clean, correctly-formatted error via its own error path. A plainErrorbypasses that and is instead caught generically inrunCli's catch block withcode: 'COMMAND_FAILED', losing commander's usual formatting/exit-code consistency for bad CLI input.🔧 Proposed fix
-import { Command, CommanderError, Option, Argument } from 'commander'; +import { Command, CommanderError, InvalidArgumentError, Option, Argument } from 'commander'; ... .option('--tail <lines>', 'Show the last N lines before following', (value: string) => { const parsed = Number(value); - if (!Number.isInteger(parsed) || parsed < 0) throw new Error('--tail must be a non-negative integer'); + if (!Number.isInteger(parsed) || parsed < 0) { + throw new InvalidArgumentError('--tail must be a non-negative integer'); + } return parsed; })🤖 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/cli.ts` around lines 109 - 113, Update the --tail option parser in the CLI option definition to throw Commander’s InvalidArgumentError for invalid values instead of a plain Error, while preserving the existing non-negative integer validation and message.src/cmd/logs.ts (1)
28-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM overall. Consider adding a
showLogs(..., { follow: true })test for thescripttarget to exercise theinScriptEntrygating + grep combination in follow mode directly — currently only the underlyingfollowLogFileand non-followfilterLinesByTargetpaths are unit tested separately.🤖 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 28 - 52, Add a focused test for showLogs with target set to script and follow enabled, using mixed script/non-script and continuation lines plus a grep filter to verify inScriptEntry gating and grep behavior together in follow mode. Keep the existing followLogFile and non-follow filtering tests unchanged.src/tools/proxy-events.ts (1)
33-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSynchronous file I/O on every proxy event.
event()callsfs.appendFileSyncsynchronously for every RPC request and every JSON-RPC error response, blocking Node's event loop for the duration of the disk write. For a low-traffic local devnet proxy this is tolerable, but it adds unnecessary latency to every RPC round-trip that passes through the proxy (which is the tool's hottest path).♻️ Optional: use a persistent append write stream instead of appendFileSync per call
export function createProxyEventLog(filePath: string): ProxyEventLog { - let dirReady = false; + let stream: fs.WriteStream | null = null; + const ensureStream = () => { + if (stream) return stream; + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + stream = fs.createWriteStream(filePath, { flags: 'a' }); + return stream; + }; return { filePath, event(text: string) { try { - if (!dirReady) { - fs.mkdirSync(path.dirname(filePath), { recursive: true }); - dirReady = true; - } - fs.appendFileSync(filePath, `${new Date().toISOString()} ${text}\n`); + ensureStream().write(`${new Date().toISOString()} ${text}\n`); } catch { - dirReady = false; + stream = null; } }, }; }The path-traversal static-analysis hint on
fs.writeFileSync(txFile, ...)(line 75-76) is a false positive —txHashis a computed hash string fromctx.hashTransaction, not attacker-supplied path input.🤖 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 33 - 50, Update createProxyEventLog and its event method to avoid synchronous appendFileSync on every proxy event; use a persistent asynchronous append write stream, initialized with the required parent directory and reused across calls. Preserve timestamped event output and ensure logging failures remain swallowed so request forwarding is never interrupted.src/devnet/log-subscription.ts (1)
143-156: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRetry logic doesn't distinguish "never connected" from "dropped mid-run".
The
errorhandler retries viasetTimeout(connect, retryDelayMs)wheneverattempts < maxAttempts, regardless of whether the socket had previously connected successfully. The doc comment on theclosehandler states a dropped subscription mid-run needs no reconnect, but that guarantee isn't actually enforced here — onlyclose()(called by the caller'sstopService) prevents further retries, and there's a narrow window where a mid-run drop could trigger a few retry attempts before the caller tears it down. This is self-recovering (bounded bymaxAttempts) but doesn't match the documented intent.♻️ Optional: track whether a connection was ever established
let socket: net.Socket | null = null; let closed = false; let attempts = 0; let failedReported = false; + let everConnected = false; const connect = () => { if (closed || endpoint == null) return; attempts += 1; const conn = net.connect(endpoint.port, endpoint.host); socket = conn; let buffer = ''; conn.on('connect', () => { + everConnected = true; conn.write(JSON.stringify({ id: 1, jsonrpc: '2.0', method: 'subscribe', params: ['log'] }) + '\n'); }); ... conn.on('error', (error) => { if (closed) return; - if (attempts < maxAttempts) { + if (!everConnected && attempts < maxAttempts) { setTimeout(connect, retryDelayMs); } else { fail(new Error(`Log subscription to ${tcpAddress} failed after ${attempts} attempts: ${error.message}`)); } });🤖 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/devnet/log-subscription.ts` around lines 143 - 156, Update the connection state in the subscription logic around the conn error/close handlers to track whether the socket has ever connected successfully. Only schedule retries from the initial connection phase; once a connection has been established, treat later errors or closes as terminal without calling connect, while preserving the existing fail behavior for exhausted initial attempts.
🤖 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/cli.ts`:
- Line 80: Update the `--verbose` option description in the CLI option
definition to accurately state that the default output includes lifecycle
events, transaction hashes, and RPC errors in addition to contract script
output, while verbose mode prints the full raw node/miner output.
In `@src/devnet/log-file.ts`:
- Around line 116-148: Update followLogFile to use a stateful UTF-8 TextDecoder
with streaming enabled when converting each newly-read buffer, preserving
incomplete multi-byte sequences across onChange calls. Reset the decoder
alongside offset and partial when the file is truncated or rotated, and flush
any decoder state appropriately when monitoring ends.
- Around line 83-90: Update resolveLogPath so the rpc target resolves the proxy
log for the active network rather than always using Network.devnet, matching the
network selected by createRPCProxy; alternatively, explicitly document and
enforce that the command is devnet-only if no network-aware context is
available. Preserve the existing named log-file resolution for non-rpc targets.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 109-113: Update the --tail option parser in the CLI option
definition to throw Commander’s InvalidArgumentError for invalid values instead
of a plain Error, while preserving the existing non-negative integer validation
and message.
In `@src/cmd/logs.ts`:
- Around line 28-52: Add a focused test for showLogs with target set to script
and follow enabled, using mixed script/non-script and continuation lines plus a
grep filter to verify inScriptEntry gating and grep behavior together in follow
mode. Keep the existing followLogFile and non-follow filtering tests unchanged.
In `@src/devnet/log-subscription.ts`:
- Around line 143-156: Update the connection state in the subscription logic
around the conn error/close handlers to track whether the socket has ever
connected successfully. Only schedule retries from the initial connection phase;
once a connection has been established, treat later errors or closes as terminal
without calling connect, while preserving the existing fail behavior for
exhausted initial attempts.
In `@src/tools/proxy-events.ts`:
- Around line 33-50: Update createProxyEventLog and its event method to avoid
synchronous appendFileSync on every proxy event; use a persistent asynchronous
append write stream, initialized with the required parent directory and reused
across calls. Preserve timestamped event output and ensure logging failures
remain swallowed so request forwarding is never interrupted.
🪄 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: 17c50b63-9418-4f8e-b6d8-e2eeeafce0be
📒 Files selected for processing (16)
.changeset/logs-command-quiet-node.mdREADME.mdsrc/cli.tssrc/cmd/logs.tssrc/cmd/node.tssrc/cmd/status.tssrc/devnet/log-file.tssrc/devnet/log-subscription.tssrc/tools/proxy-events.tssrc/tools/rpc-proxy.tstests/log-subscription.test.tstests/logs-command.test.tstests/logs.test.tstests/node-quiet-mode.test.tstests/node-supervisor.test.tstests/proxy-events.test.ts
humble-little-bear
force-pushed
the
agent/claude-bear/48973f2a
branch
2 times, most recently
from
July 29, 2026 14:00
319f9fa to
7ea55e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/devnet/log-subscription.ts (1)
143-170: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePending retry timer survives
close().connectre-checksclosed, so no stray connection happens, but thesetTimeouthandle is never tracked or unref'd and can keep the event loop alive after the subscription is torn down.♻️ Proposed refactor
+ let retryTimer: NodeJS.Timeout | null = null; @@ if (attempts < maxAttempts) { - setTimeout(connect, retryDelayMs); + retryTimer = setTimeout(connect, retryDelayMs); + retryTimer.unref?.(); } else { @@ close() { closed = true; + if (retryTimer) clearTimeout(retryTimer); + retryTimer = null; socket?.destroy();🤖 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/devnet/log-subscription.ts` around lines 143 - 170, Track the retry timer created in the error handler of the subscription’s connect flow, and clear it in close() while marking it inactive. Ensure the timer is reset when it fires and does not keep the event loop alive, while preserving the existing closed check and retry behavior.tests/logs-command.test.ts (1)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp directories created by
fixture()are never removed. Track the created roots and clean them inafterEach, as the other suites in this PR do.♻️ Proposed refactor
-function fixture(): { settings: Settings; transport: CapturingTransport } { +const tempRoots: string[] = []; +afterEach(() => { + while (tempRoots.length) fs.rmSync(tempRoots.pop()!, { recursive: true, force: true }); +}); + +function fixture(): { settings: Settings; transport: CapturingTransport } { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'offckb-logs-cmd-')); + tempRoots.push(root);🤖 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/logs-command.test.ts` around lines 28 - 40, Update the test fixture setup around fixture() to track each temporary root it creates, and add afterEach cleanup that removes those roots recursively after every test. Follow the cleanup pattern used by the other test suites, while preserving fixture’s existing settings and transport setup.tests/logs.test.ts (1)
130-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTiming-sensitive follow test. The 500 ms sleep gives only ~2 poll intervals of margin, and
fs.watchFilepolling granularity varies by platform/CI load. Pass a smallintervalMsand poll until the line appears (or time out) instead of a fixed sleep; also movermSyncinto afinally/afterEachso a failed expectation does not leak the temp dir.🤖 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/logs.test.ts` around lines 130 - 146, The followLogFile test should avoid fixed timing and cleanup leaks. Configure followLogFile with a small intervalMs, poll until SCRIPT_LINE is observed or a timeout is reached, then stop watching; move temporary-directory removal into a finally block or afterEach so cleanup runs even when assertions fail.tests/proxy-events.test.ts (1)
67-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
beforeEach/afterEachtemp-dir pattern here. Each test repeatsmkdtempSync/rmSync, which leaks the directory when an expectation fails, and Line 107 points the event log atos.tmpdir()directly (<tmp>/data/logs/proxy.log) rather than an isolated dir.🤖 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/proxy-events.test.ts` around lines 67 - 112, Refactor the handleProxyResponseBody test suite to use shared beforeEach and afterEach hooks for creating and removing an isolated temporary directory. Store the directory and context in suite-scoped variables, reuse the context in each test, and ensure the non-JSON response test also uses the isolated directory rather than os.tmpdir() directly.src/cmd/node.ts (1)
202-222: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueLog subscription is only closed on the supervisor path.
stopServiceruns when CKB/miner exit unexpectedly; a clean shutdown (SIGINT/SIGTERM handling elsewhere) leaves the socket open. Consider closing it from the signal path too so the retry timer/socket cannot hold the event loop.Also applies to: 237-237
🤖 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 202 - 222, The logSubscription created in the node startup flow must also be closed during clean SIGINT/SIGTERM shutdown, not only through stopService. Update the existing signal-handling path to call the subscription’s cleanup/unsubscribe operation when logSubscription is present, ensuring the retry timer and socket are released while preserving the current supervisor cleanup behavior.
🤖 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 208-212: Sanitize entry.message in the subscribeToNodeLogs
callback before passing it to logger.info, removing or escaping C0/C1 control
characters and ANSI/OSC/DCS terminal sequences while preserving normal text.
Keep the existing SCRIPT_LOG_TARGET filter and “CKB-Script:” prefix unchanged.
In `@src/devnet/log-file.ts`:
- Around line 134-144: Update the read loop in the surrounding log-file
processing function to capture the byte count returned by fs.readSync and decode
only the populated portion of buffer, preserving the existing partial-line
handling and onLine behavior.
In `@src/tools/proxy-events.ts`:
- Around line 92-95: Update handleProxyResponseBody to normalize contentType by
removing parameters such as “; charset=utf-8” before comparing it with
application/json. Preserve the existing body and JSON-shape checks, and add a
test covering a charset-bearing application/json response.
- Around line 64-66: Update the RPC event logging in the proxy event handler
around method and response code/message handling to sanitize or serialize all
fields before appending to proxy.log. Prevent embedded newlines, carriage
returns, and terminal control characters from creating forged records or
corrupting offckb logs rpc output, while preserving the existing event content
for safe values.
---
Nitpick comments:
In `@src/cmd/node.ts`:
- Around line 202-222: The logSubscription created in the node startup flow must
also be closed during clean SIGINT/SIGTERM shutdown, not only through
stopService. Update the existing signal-handling path to call the subscription’s
cleanup/unsubscribe operation when logSubscription is present, ensuring the
retry timer and socket are released while preserving the current supervisor
cleanup behavior.
In `@src/devnet/log-subscription.ts`:
- Around line 143-170: Track the retry timer created in the error handler of the
subscription’s connect flow, and clear it in close() while marking it inactive.
Ensure the timer is reset when it fires and does not keep the event loop alive,
while preserving the existing closed check and retry behavior.
In `@tests/logs-command.test.ts`:
- Around line 28-40: Update the test fixture setup around fixture() to track
each temporary root it creates, and add afterEach cleanup that removes those
roots recursively after every test. Follow the cleanup pattern used by the other
test suites, while preserving fixture’s existing settings and transport setup.
In `@tests/logs.test.ts`:
- Around line 130-146: The followLogFile test should avoid fixed timing and
cleanup leaks. Configure followLogFile with a small intervalMs, poll until
SCRIPT_LINE is observed or a timeout is reached, then stop watching; move
temporary-directory removal into a finally block or afterEach so cleanup runs
even when assertions fail.
In `@tests/proxy-events.test.ts`:
- Around line 67-112: Refactor the handleProxyResponseBody test suite to use
shared beforeEach and afterEach hooks for creating and removing an isolated
temporary directory. Store the directory and context in suite-scoped variables,
reuse the context in each test, and ensure the non-JSON response test also uses
the isolated directory rather than os.tmpdir() directly.
🪄 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: 0f7e01c1-ee00-4c64-969f-f3cf242807dd
📒 Files selected for processing (17)
.changeset/logs-command-quiet-node.mdREADME.mdsrc/cli.tssrc/cmd/logs.tssrc/cmd/node.tssrc/cmd/status.tssrc/devnet/log-file.tssrc/devnet/log-subscription.tssrc/tools/proxy-events.tssrc/tools/rpc-proxy.tstests/log-subscription.test.tstests/logs-command.test.tstests/logs.test.tstests/node-quiet-mode.test.tstests/node-supervisor.test.tstests/node-terminal-rpc.test.tstests/proxy-events.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/log-subscription.test.ts
- .changeset/logs-command-quiet-node.md
- src/cmd/logs.ts
- README.md
- tests/node-supervisor.test.ts
- src/tools/rpc-proxy.ts
- tests/node-quiet-mode.test.ts
- src/cli.ts
- src/cmd/status.ts
Add offckb logs [node|script|miner|rpc] [-f] [--grep] [--tail], reading the log files CKB always writes (run.log/miner.log) plus a new proxy event log, so logs are reachable in every run mode and pipe/agent friendly. A foreground offckb node is quiet by default: lifecycle events, live contract script debug output (via the node's TCP log subscription, the same channel ckb-tui uses), send_transaction hashes, and RPC errors still print; --verbose restores the raw stdout relay. The RPC proxy drops per-request lines to debug, warns on JSON-RPC errors in responses, and appends everything to data/logs/proxy.log (viewable via offckb logs rpc). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
humble-little-bear
force-pushed
the
agent/claude-bear/48973f2a
branch
from
July 29, 2026 14:14
7ea55e1 to
89f3e38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/cmd/node.ts (1)
208-212: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winInjection (CWE-117)
Reachability: External
● Entry src/cli.ts:86 startNode │ ▼ ● Sink src/cmd/node.tsScript log output is still forwarded unsanitized.
entry.messagearrives from the node's TCP log stream (which relays contractdebug!output) and goes straight tologger.info, so embedded ANSI/OSC/C0 sequences reach the terminal. The file already hascleanChildOutputfor exactly this on the stdout path; reuse it (or a control-char strip) here.🔒 Proposed fix
- if (entry.target === SCRIPT_LOG_TARGET) logger.info(['CKB-Script:', entry.message]); + if (entry.target === SCRIPT_LOG_TARGET) logger.info(['CKB-Script:', cleanChildOutput(entry.message)]);🤖 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 208 - 212, Sanitize script log messages before forwarding them in the subscribeToNodeLogs callback, reusing the existing cleanChildOutput helper used by the stdout path. Apply it to entry.message while preserving the SCRIPT_LOG_TARGET filter and logger.info call.src/tools/proxy-events.ts (2)
66-66: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winInjection (CWE-117)
Reachability: External
● Entry src/cli.ts:86 startNode │ ▼ ● Hop src/cmd/node.ts │ ▼ ● Hop src/tools/rpc-proxy.ts:12 createRPCProxy: Target RPC server │ ▼ ● Sink src/tools/proxy-events.tsEvent fields are still written to
proxy.logunescaped.methodand the errorcode/messagecome from the proxied payload; an embedded\n/\rsplits one event into forged records and control chars corruptoffckb logs rpcoutput. Strip/escape control characters inevent()(single choke point) rather than at each call site.🔒 Proposed fix
event(text: string) { try { + // One event is one line: neutralize embedded newlines/control chars. + const safe = text.replace(/[\u0000-\u001f\u007f-\u009f]/g, ' '); if (!dirReady) { fs.mkdirSync(path.dirname(filePath), { recursive: true }); dirReady = true; } - fs.appendFileSync(filePath, `${new Date().toISOString()} ${text}\n`); + fs.appendFileSync(filePath, `${new Date().toISOString()} ${safe}\n`);Also applies to: 104-105
🤖 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` at line 66, Update the event() helper in proxy-events.ts to sanitize or escape control characters, including newline and carriage return, in all event fields before writing to proxy.log. Apply the change at this single choke point so both the request method and proxied error code/message emitted by the existing event() calls are protected without modifying individual call sites.
92-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStrict content-type comparison still drops most real responses.
application/json; charset=utf-8is a normal CKB/HTTP response value and failscontentType !== 'application/json', so JSON-RPC errors are never surfaced or written toproxy.log. Normalize the media type before comparing. (Previously flagged and marked addressed, but the exact-match check is still here.)🔧 Proposed fix
- if (contentType !== 'application/json') return; + const mediaType = (contentType ?? '').split(';')[0].trim().toLowerCase(); + if (mediaType !== 'application/json') return;Worth adding a test with
'application/json; charset=utf-8'intests/proxy-events.test.ts.🤖 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 92 - 95, Update handleProxyResponseBody to normalize contentType to its media type before comparing it with application/json, so parameters such as charset=utf-8 are accepted. Preserve the existing empty-body and JSON-shape checks, and add coverage in proxy-events.test.ts for a content type containing application/json; charset=utf-8.src/devnet/log-file.ts (1)
134-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRead-block decoding is still byte-unsafe. Two previously raised issues remain:
fs.readSync's return value is ignored (a short read leaves NUL bytes that get emitted inside lines), and each chunk is decoded independently, so a multi-byte UTF-8 char straddling a read boundary becomes�.🔧 Proposed fix
+ const decoder = new TextDecoder('utf-8'); ... const length = curr.size - offset; const buffer = Buffer.alloc(length); - fs.readSync(fd, buffer, 0, length, offset); - offset = curr.size; - const text = partial + buffer.toString('utf8'); + const bytesRead = fs.readSync(fd, buffer, 0, length, offset); + offset += bytesRead; + const text = partial + decoder.decode(buffer.subarray(0, bytesRead), { stream: 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/devnet/log-file.ts` around lines 134 - 144, Update the read loop around the visible fs.readSync call to use the returned byte count when decoding, so unread Buffer bytes are excluded from text processing. Preserve UTF-8 sequences across chunk boundaries by using a persistent StringDecoder (or equivalent incremental decoder) for the stream, and flush it when the file-following read ends before splitting lines and invoking onLine.
🧹 Nitpick comments (5)
tests/proxy-events.test.ts (1)
67-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
beforeEach/afterEachteardown from the block above. These three testsrmSyncat the end of the test body, so a failing assertion leaks the temp dir; the firstdescribealready has the right pattern.♻️ Suggested cleanup
describe('handleProxyResponseBody', () => { + let dir: string; + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offckb-proxy-')); + }); + afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + it('warns on JSON-RPC errors and records them', () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offckb-proxy-')); const ctx = makeCtx(path.join(dir, 'transactions'));🤖 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/proxy-events.test.ts` around lines 67 - 104, Update the three tests in the handleProxyResponseBody describe block to use the existing beforeEach/afterEach temporary-directory setup and teardown from the surrounding test block. Remove each test body's direct fs.rmSync cleanup while preserving the current assertions and test behavior.src/devnet/log-subscription.ts (1)
143-150: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTrack the retry timer so
close()is fully synchronous. A pendingsetTimeout(connect, retryDelayMs)survivesclose();connect()no-ops thanks toclosed, but the timer can still hold the event loop open for up toretryDelayMsafter shutdown.♻️ Optional cleanup
let attempts = 0; let failedReported = false; + let retryTimer: NodeJS.Timeout | null = null; ... if (attempts < maxAttempts) { - setTimeout(connect, retryDelayMs); + retryTimer = setTimeout(connect, retryDelayMs); + retryTimer.unref?.(); ... close() { closed = true; + if (retryTimer) clearTimeout(retryTimer); + retryTimer = null; socket?.destroy();Also applies to: 164-170
🤖 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/devnet/log-subscription.ts` around lines 143 - 150, Track the retry timeout created in the connection error handler and clear it from close(). Update the surrounding subscription state and close implementation so pending reconnect timers are cancelled during shutdown, while preserving the existing retry behavior and closed guard.tests/logs-command.test.ts (1)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fixture()leaks a temp dir per test. NoafterEachremoves themkdtempSyncroots, so each run leaves six directories inos.tmpdir(). Return the root and clean it up.♻️ Suggested cleanup
-function fixture(): { settings: Settings; transport: CapturingTransport } { +const roots: string[] = []; +afterEach(() => { + while (roots.length) fs.rmSync(roots.pop() as string, { recursive: true, force: true }); +}); + +function fixture(): { settings: Settings; transport: CapturingTransport } { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'offckb-logs-cmd-')); + roots.push(root);🤖 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/logs-command.test.ts` around lines 28 - 40, Update fixture() to return the mkdtempSync root alongside settings and transport, then add per-test cleanup that removes this root recursively after each test. Ensure every test using fixture() registers or performs cleanup so temporary directories do not remain in os.tmpdir().src/tools/proxy-events.ts (1)
33-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding
proxy.log. Every request appends a line synchronously with no size cap or rotation, so a long-running devnet with a chatty indexer grows the file indefinitely and each request pays a blocking write. A size check with a single.1rollover (or dropping the per-requestrequest <method>line and keeping only transactions/errors) would keep it bounded.🤖 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 33 - 50, Update createProxyEventLog so proxy.log growth is bounded: before appending each event, enforce a size limit and perform a single .1 rollover, preserving logging failure isolation and directory creation behavior. Keep the existing event content unless implementing the alternative of removing only per-request request lines.tests/logs.test.ts (1)
141-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a truncation/rotation case. The
curr.size < prev.sizereset branch infollowLogFileis the trickiest part of the offset bookkeeping and is currently uncovered — a secondlisteners[0](smallerStat, largerStat)invocation after rewriting the file would pin it.🤖 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/logs.test.ts` around lines 141 - 174, Add a test case in the followLogFile suite that rewrites or truncates the watched log to a smaller size, then invokes listeners[0] with the smaller current stat and larger previous stat. Assert that followLogFile resets its offset and emits the rewritten file content, while preserving cleanup via stop and the existing mock/file teardown.
🤖 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/tools/rpc-proxy.ts`:
- Line 45: Update the call to handleProxyResponseBody so the Content-Type value
is normalized to its base media type by removing parameters such as charset
before comparison. Preserve the existing behavior for exact application/json
responses while ensuring parameterized JSON types are recognized for RPC error
logging and persistence.
---
Duplicate comments:
In `@src/cmd/node.ts`:
- Around line 208-212: Sanitize script log messages before forwarding them in
the subscribeToNodeLogs callback, reusing the existing cleanChildOutput helper
used by the stdout path. Apply it to entry.message while preserving the
SCRIPT_LOG_TARGET filter and logger.info call.
In `@src/devnet/log-file.ts`:
- Around line 134-144: Update the read loop around the visible fs.readSync call
to use the returned byte count when decoding, so unread Buffer bytes are
excluded from text processing. Preserve UTF-8 sequences across chunk boundaries
by using a persistent StringDecoder (or equivalent incremental decoder) for the
stream, and flush it when the file-following read ends before splitting lines
and invoking onLine.
In `@src/tools/proxy-events.ts`:
- Line 66: Update the event() helper in proxy-events.ts to sanitize or escape
control characters, including newline and carriage return, in all event fields
before writing to proxy.log. Apply the change at this single choke point so both
the request method and proxied error code/message emitted by the existing
event() calls are protected without modifying individual call sites.
- Around line 92-95: Update handleProxyResponseBody to normalize contentType to
its media type before comparing it with application/json, so parameters such as
charset=utf-8 are accepted. Preserve the existing empty-body and JSON-shape
checks, and add coverage in proxy-events.test.ts for a content type containing
application/json; charset=utf-8.
---
Nitpick comments:
In `@src/devnet/log-subscription.ts`:
- Around line 143-150: Track the retry timeout created in the connection error
handler and clear it from close(). Update the surrounding subscription state and
close implementation so pending reconnect timers are cancelled during shutdown,
while preserving the existing retry behavior and closed guard.
In `@src/tools/proxy-events.ts`:
- Around line 33-50: Update createProxyEventLog so proxy.log growth is bounded:
before appending each event, enforce a size limit and perform a single .1
rollover, preserving logging failure isolation and directory creation behavior.
Keep the existing event content unless implementing the alternative of removing
only per-request request lines.
In `@tests/logs-command.test.ts`:
- Around line 28-40: Update fixture() to return the mkdtempSync root alongside
settings and transport, then add per-test cleanup that removes this root
recursively after each test. Ensure every test using fixture() registers or
performs cleanup so temporary directories do not remain in os.tmpdir().
In `@tests/logs.test.ts`:
- Around line 141-174: Add a test case in the followLogFile suite that rewrites
or truncates the watched log to a smaller size, then invokes listeners[0] with
the smaller current stat and larger previous stat. Assert that followLogFile
resets its offset and emits the rewritten file content, while preserving cleanup
via stop and the existing mock/file teardown.
In `@tests/proxy-events.test.ts`:
- Around line 67-104: Update the three tests in the handleProxyResponseBody
describe block to use the existing beforeEach/afterEach temporary-directory
setup and teardown from the surrounding test block. Remove each test body's
direct fs.rmSync cleanup while preserving the current assertions and test
behavior.
🪄 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: 879429b2-a5a3-48c6-ae90-ac0e6e386c1d
📒 Files selected for processing (17)
.changeset/logs-command-quiet-node.mdREADME.mdsrc/cli.tssrc/cmd/logs.tssrc/cmd/node.tssrc/cmd/status.tssrc/devnet/log-file.tssrc/devnet/log-subscription.tssrc/tools/proxy-events.tssrc/tools/rpc-proxy.tstests/log-subscription.test.tstests/logs-command.test.tstests/logs.test.tstests/node-quiet-mode.test.tstests/node-supervisor.test.tstests/node-terminal-rpc.test.tstests/proxy-events.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/node-supervisor.test.ts
- tests/log-subscription.test.ts
- .changeset/logs-command-quiet-node.md
- tests/node-quiet-mode.test.ts
- tests/node-terminal-rpc.test.ts
- README.md
- src/cmd/status.ts
- src/cmd/logs.ts
- node: sanitize relayed script log entries (CSI/OSC/C0/C1) via cleanChildOutput so crafted debug! output cannot inject terminal control sequences - log-file: honor readSync's byte count and decode with a streaming TextDecoder so multi-byte UTF-8 survives chunk boundaries - proxy-events: sanitize event text at the single event() choke point (one event = one line), normalize the response media type before the application/json check (charset params), and bound proxy.log with a single .1 rollover at 10 MB - log-subscription: retry only during the initial connect window and track/unref/clear the retry timer so close() is fully synchronous - cli: throw commander's InvalidArgumentError from the --tail parser and align the --verbose help text with the actual quiet defaults - tests: add follow-mode script/grep, truncation/rotation, UTF-8 split, event sanitization, rollover, charset content-type, and subscription retry cases; move temp-dir handling to afterEach cleanup
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/logs.test.ts (1)
145-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
StatListenertype duplicated across spec files.The identical alias also appears in
tests/logs-command.test.ts. Worth hoisting into a shared test-utils module alongsidecaptureWatchListener/tempLogif more log-following tests get added.🤖 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/logs.test.ts` at line 145, Move the duplicated StatListener type alias from the log-following test files into the shared test-utils module alongside captureWatchListener and tempLog. Update tests/logs.test.ts and tests/logs-command.test.ts to import and reuse the shared StatListener definition, removing their local aliases.
🤖 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/devnet/log-file.ts`:
- Around line 124-129: Update the rotation condition in the onChange callback to
also reset when the file inode changes by including curr.ino !== prev.ino
alongside the existing size checks. Preserve the current offset, partial, and
decoder reset behavior for all detected rotation cases.
In `@src/devnet/log-subscription.ts`:
- Around line 136-137: Treat the JSON-RPC subscription-id response as the
successful connection point instead of the TCP connect event: remove the
terminal/retry-suppression update from the socket connect callback and set it
after parsing the subscription response in src/devnet/log-subscription.ts (lines
136-137). Update tests/log-subscription.test.ts (lines 155-162) to emit that
subscription-id response before asserting a later socket error does not
reconnect.
In `@src/tools/proxy-events.ts`:
- Around line 123-126: Sanitize the stringified JSON-RPC error fields before
retaining or logging them: update the handling around the media-type check in
src/tools/proxy-events.ts lines 123-126 so both code and message passed to
ctx.sink.warn and ctx.events.event use sanitizeEventText. Extend the
charset-bearing JSON-RPC error case in tests/proxy-events.test.ts lines 88-98
with an escape character and assert the warning output contains no escape
character.
---
Nitpick comments:
In `@tests/logs.test.ts`:
- Line 145: Move the duplicated StatListener type alias from the log-following
test files into the shared test-utils module alongside captureWatchListener and
tempLog. Update tests/logs.test.ts and tests/logs-command.test.ts to import and
reuse the shared StatListener definition, removing their local aliases.
🪄 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: 4b50f90a-63ad-4293-ba95-7a1ba202e66c
📒 Files selected for processing (10)
src/cli.tssrc/cmd/node.tssrc/devnet/log-file.tssrc/devnet/log-subscription.tssrc/tools/proxy-events.tstests/log-subscription.test.tstests/logs-command.test.tstests/logs.test.tstests/node-quiet-mode.test.tstests/proxy-events.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/node-quiet-mode.test.ts
Comment on lines
+124
to
+129
| const onChange = (curr: fs.Stats, prev: fs.Stats) => { | ||
| if (curr.size < prev.size || curr.size < offset) { | ||
| // Truncated or rotated: restart from the beginning. | ||
| offset = 0; | ||
| partial = ''; | ||
| decoder = new TextDecoder('utf-8'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect inode changes as log rotation.
A replacement file with the same or larger size bypasses this reset; curr.size === offset can then discard the new file’s initial entries. Include curr.ino !== prev.ino in the rotation condition.
Proposed fix
- if (curr.size < prev.size || curr.size < offset) {
+ if (curr.ino !== prev.ino || curr.size < prev.size || curr.size < offset) {📝 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.
Suggested change
| const onChange = (curr: fs.Stats, prev: fs.Stats) => { | |
| if (curr.size < prev.size || curr.size < offset) { | |
| // Truncated or rotated: restart from the beginning. | |
| offset = 0; | |
| partial = ''; | |
| decoder = new TextDecoder('utf-8'); | |
| const onChange = (curr: fs.Stats, prev: fs.Stats) => { | |
| if (curr.ino !== prev.ino || curr.size < prev.size || curr.size < offset) { | |
| // Truncated or rotated: restart from the beginning. | |
| offset = 0; | |
| partial = ''; | |
| decoder = new TextDecoder('utf-8'); |
🤖 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/devnet/log-file.ts` around lines 124 - 129, Update the rotation condition
in the onChange callback to also reset when the file inode changes by including
curr.ino !== prev.ino alongside the existing size checks. Preserve the current
offset, partial, and decoder reset behavior for all detected rotation cases.
Comment on lines
+136
to
+137
| conn.on('connect', () => { | ||
| everConnected = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat JSON-RPC acknowledgement—not TCP connect—as a live subscription. A TCP connection can reset before the server processes subscribe; suppressing retries at connect permanently drops foreground script logs.
src/devnet/log-subscription.ts#L136-L137: set the terminal/retry-suppression state after parsing the subscription-id response, not in the socketconnectcallback.tests/log-subscription.test.ts#L155-L162: emit the subscription-id response before asserting that a later socket error does not reconnect.
📍 Affects 2 files
src/devnet/log-subscription.ts#L136-L137(this comment)tests/log-subscription.test.ts#L155-L162
🤖 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/devnet/log-subscription.ts` around lines 136 - 137, Treat the JSON-RPC
subscription-id response as the successful connection point instead of the TCP
connect event: remove the terminal/retry-suppression update from the socket
connect callback and set it after parsing the subscription response in
src/devnet/log-subscription.ts (lines 136-137). Update
tests/log-subscription.test.ts (lines 155-162) to emit that subscription-id
response before asserting a later socket error does not reconnect.
Comment on lines
+123
to
+126
| // Real servers answer with parameters attached (application/json; | ||
| // charset=utf-8), so compare the bare media type, not the raw header. | ||
| const mediaType = (contentType ?? '').split(';')[0].trim().toLowerCase(); | ||
| if (mediaType !== 'application/json') return; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Injection (CWE-150)
Reachability: External
● Entry
tests/proxy-events.test.ts
│
▼
● Sink
src/tools/proxy-events.ts
Sanitize JSON-RPC error fields before calling sink.warn.
sanitizeEventText protects proxy.log, but code and message still reach ctx.sink.warn raw. A malicious proxied response can inject terminal control sequences into retained RPC-error output.
src/tools/proxy-events.ts#L123-L126: sanitize stringified error fields before using them in bothctx.sink.warnandctx.events.event.tests/proxy-events.test.ts#L88-L98: include an escape character in the charset-bearing JSON-RPC error and assert the warning receives no escape character.
Proposed fix
- const code = entry.error.code ?? 'unknown';
- const message = entry.error.message ?? 'unknown error';
+ const code = sanitizeEventText(String(entry.error.code ?? 'unknown'));
+ const message = sanitizeEventText(String(entry.error.message ?? 'unknown error'));
ctx.sink.warn(`RPC error: [${code}] ${message}`);
ctx.events.event(`error [${code}] ${message}`);📝 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.
Suggested change
| // Real servers answer with parameters attached (application/json; | |
| // charset=utf-8), so compare the bare media type, not the raw header. | |
| const mediaType = (contentType ?? '').split(';')[0].trim().toLowerCase(); | |
| if (mediaType !== 'application/json') return; | |
| const code = sanitizeEventText(String(entry.error.code ?? 'unknown')); | |
| const message = sanitizeEventText(String(entry.error.message ?? 'unknown error')); | |
| ctx.sink.warn(`RPC error: [${code}] ${message}`); | |
| ctx.events.event(`error [${code}] ${message}`); |
📍 Affects 2 files
src/tools/proxy-events.ts#L123-L126(this comment)tests/proxy-events.test.ts#L88-L98
🤖 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 123 - 126, Sanitize the stringified
JSON-RPC error fields before retaining or logging them: update the handling
around the media-type check in src/tools/proxy-events.ts lines 123-126 so both
code and message passed to ctx.sink.warn and ctx.events.event use
sanitizeEventText. Extend the charset-bearing JSON-RPC error case in
tests/proxy-events.test.ts lines 88-98 with an escape character and assert the
warning output contains no escape character.
Collaborator
Author
|
Review findings addressed in 44f3ef8 (CI green on ubuntu/windows/macos, 301 tests passing). Fixed
Skipped, with reasons
New test coverage: script-entry sanitization, follow-mode script+grep gating, truncation/rotation re-read, multi-byte UTF-8 split reassembly, event-text sanitization, |
This was referenced Jul 31, 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
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.
背景
devnet 日志此前有 3 个源(节点/miner/proxy)× 3 个出口(前台转打、daemon.log、status TUI),行为不一致:前台被全量节点输出刷屏,daemon 模式没有内建查看方式,
status必须 TTY 全屏。本 PR 按讨论的「方案 A」统一:文件为单一日志源,前台默认安静,新增offckb logs命令。改动
1. 新增
offckb logs [target](docker logs心智,可 pipe、配合--json)offckb logs(默认)= 节点日志(run.log)offckb logs script= 合约debug!输出(按 CKB 日志行 target 过滤ckb-script,多行消息的延续行也保留)offckb logs miner/offckb logs rpc-f/--follow(tail -f 流式)、--tail N、--grep <str>daemon不作为概念暴露(它是实现残骸),rpc按用户任务命名而非内部架构词 proxy2. 前台
offckb node默认安静--verbose恢复旧行为ckb-scriptsend_transaction: <hash>、JSON-RPC error warnFollow the full node log with: offckb logs -f3. RPC proxy 降噪 + 落盘
RPC Req:从 info 降为 debug;解析 proxyRes 的 JSON-RPC error 并 warn(此前完全看不到)data/logs/proxy.log(前台/daemon 都写),即offckb logs rpc的数据源status维持现状;README 已补充 logs 用法。验证
tsc --noEmit、eslint 干净(lint 的 4 个 warning 为存量)logs/logs miner/logs rpc输出正确;logs -f实时流出新块日志send_transaction: <hash>,logs rpc记录 request + hashsend_transaction→ 前台 warnRPC error: [-32602] ...,proxy.log 同步记录--verbose恢复CKB:/CKB-Miner:全量输出debug!的合约(secp256k1 不产生 ckb-script 日志),所以「真实 script entry 上屏」这一环靠单测(entry 过滤+打印)和订阅协议实测覆盖,entry 结构与其余 target 完全一致🤖 Generated with Claude Code