allow user to setNode - #166
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
test/util.ts (2)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a single source for the REPL prompt string.
REPL_PROMPTduplicatesPROMPTinsrc/index.ts.test/replMenu.test.tsline 44 counts occurrences of this exact string, so a change insrc/index.tsweakens that assertion silently instead of failing it. Move the literal into a small shared module (for examplesrc/constants.ts) and import it in both places.🤖 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 `@test/util.ts` around lines 46 - 48, Move the duplicated REPL prompt literal from test/util.ts and src/index.ts into a shared constant exported by a small module such as constants.ts. Update both the PROMPT usage in src/index.ts and REPL_PROMPT usage in test/util.ts to import that shared constant, preserving the existing prompt value and test behavior.
88-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTerminate the child process when
runRepltimes out.
runReplonly updates its own promise onclose, whiletest/setNode.test.tssets a 120s Mocha timeout. If thenpx tsxCLI hangs, Mocha may abandon the test while the child process remains alive because no childSIGKILLtimer is installed. Add a boundedtimeout/killSignaltospawnand handlecloseso the non-zero timeout code becomes a failing result instead of leaving an orphan child.🤖 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 `@test/util.ts` around lines 88 - 104, The runRepl child process lacks timeout-based termination and timeout-result handling. Update the spawn call and its close handling in runRepl to apply a bounded timeout with a kill signal, and ensure termination produces a non-zero result that rejects or otherwise fails the run instead of leaving the process alive.src/cli.ts (1)
127-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider skipping the startup reachability probe in one-shot mode.
createCLI()awaitsvalidateNode()on every invocation. In one-shot mode (AVOID_LOOP_RUN=true) this adds a blocking status request before the requested command runs, and an unreachable node costs the full 10 s HTTP or 30 s P2P timeout on each scripted call. The message "Commands may fail" adds little value there, because the command itself reports its own failure. Gate the probe onprocess.env.AVOID_LOOP_RUN !== "true", matching the eagerstartP2Pdecision above.♻️ Proposed change
- if (hasNode()) { + // Interactive only: a one-shot run gets its reachability signal from the command + // it was asked to run, so probing first only adds latency. + if (hasNode() && process.env.AVOID_LOOP_RUN !== "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/cli.ts` around lines 127 - 144, Skip the startup reachability probe in createCLI when process.env.AVOID_LOOP_RUN === "true"; only call validateNode and print its success or unreachable-node messages in interactive mode, while preserving the existing behavior otherwise.src/nodeConnection.ts (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
as anyon thesetupP2Poptions object.The setup options still need a non-
anytype; narrow the cast only to thelibp2pproperty if@oceanprotocol/lib9.0.0-next.6 does not accept it at the options level.🤖 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/nodeConnection.ts` around lines 82 - 90, Remove the broad as any cast from the options passed to ProviderInstance.setupP2P, preserving type checking for the full setup options object. If the libp2p configuration remains incompatible with the library type, narrow the cast to only the libp2p property while keeping bootstrapPeers and the surrounding options strongly typed.Source: Coding guidelines
🤖 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 `@CLAUDE.md`:
- Line 168: Update the startP2P description in CLAUDE.md to state that eager
startup occurs for normal invocations but is skipped when AVOID_LOOP_RUN="true",
matching the condition in cli.ts. Preserve the existing details about non-lazy
startup, awaiting behavior, and readiness handling.
In `@README.md`:
- Around line 90-92: Update the fenced code block containing export
NODE_URL='XXXX' to specify the bash language, matching the neighboring fenced
blocks and satisfying markdownlint MD040.
In `@src/index.ts`:
- Around line 245-258: Update the catch block to set process.exitCode to 1
instead of immediately calling process.exit(1), allowing the existing finally
cleanup to flush output. In the finally block, ensure the stopP2P() false path
explicitly flushes output and exits after cleanup, while preserving the existing
stopP2P() true behavior.
In `@test/setNode.test.ts`:
- Around line 104-115: Update both affected tests in test/setNode.test.ts at
lines 104-115 and 124-133 to include RPC: LIVE_RPC in the runRepl environment
overrides alongside their existing NODE_URL values, ensuring
Commands.initializeSigner() reaches the live Barge endpoint and the assertions
execute.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 127-144: Skip the startup reachability probe in createCLI when
process.env.AVOID_LOOP_RUN === "true"; only call validateNode and print its
success or unreachable-node messages in interactive mode, while preserving the
existing behavior otherwise.
In `@src/nodeConnection.ts`:
- Around line 82-90: Remove the broad as any cast from the options passed to
ProviderInstance.setupP2P, preserving type checking for the full setup options
object. If the libp2p configuration remains incompatible with the library type,
narrow the cast to only the libp2p property while keeping bootstrapPeers and the
surrounding options strongly typed.
In `@test/util.ts`:
- Around line 46-48: Move the duplicated REPL prompt literal from test/util.ts
and src/index.ts into a shared constant exported by a small module such as
constants.ts. Update both the PROMPT usage in src/index.ts and REPL_PROMPT usage
in test/util.ts to import that shared constant, preserving the existing prompt
value and test behavior.
- Around line 88-104: The runRepl child process lacks timeout-based termination
and timeout-result handling. Update the spawn call and its close handling in
runRepl to apply a bounded timeout with a kill signal, and ensure termination
produces a non-zero result that rejects or otherwise fails the run instead of
leaving the process alive.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9054ec-b61d-4f39-b100-1b3067cd44ce
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdsrc/cli.tssrc/index.tssrc/nodeConnection.tstest/replMenu.test.tstest/setNode.test.tstest/util.ts
| ``` | ||
| export NODE_URL='XXXX' | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint reports MD040 for the block at line 90. Add bash to match the neighboring blocks at lines 96 and 106.
📝 Proposed fix
-```
+```bash
export NODE_URL='XXXX'</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` around lines 90 - 92, Update the fenced code block containing
export NODE_URL='XXXX' to specify the bash language, matching the neighboring
fenced blocks and satisfying markdownlint MD040.
Source: Linters/SAST tools
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR introduces a well-designed enhancement for node selection by allowing the CLI to start without a predefined node and enabling on-the-fly node switching. It decouples the libp2p transport lifecycle into a dedicated nodeConnection.ts module, implementing non-blocking background warm-up for interactive mode and safe teardown logic.
Comments:
• [WARNING][bug] The positional argument is marked as required by using <nodeUrl>. This means running setNode -n <url> without a positional argument will cause Commander to throw a "missing required argument" error. Consider making the positional argument optional [nodeUrl] and validating its presence in the action, or simply remove the -n, --node option as the positional argument alone is sufficient.
- .argument("<nodeUrl>", "HTTP(S) URL, peer id or full multiaddr of the node")
+ .argument("[nodeUrl]", "HTTP(S) URL, peer id or full multiaddr of the node")• [INFO][style] If a provider or indexer object happens to lack a chainId property, String(p.chainId) will yield the literal string "undefined". It is safer to filter out falsy values before mapping to a string.
- ...(status.provider || []).map((p) => String(p.chainId)),
- ...(status.indexer || []).map((i) => String(i.chainId)),
+ ...(status.provider || []).map((p) => p.chainId).filter(Boolean).map(String),
+ ...(status.indexer || []).map((i) => i.chainId).filter(Boolean).map(String),• [INFO][other] Excellent design pattern here. Catching the rejection of the fire-and-forget promise and storing it in p2pFailure to be surfaced later in ensureP2PReady() elegantly prevents unhandled promise rejection crashes while keeping the startup non-blocking.
• [INFO][other] The deliberate use of process.exit(1) in the catch block to bypass the finally block—thus skipping a potentially hanging orderly shutdown on fatal errors—is a thoughtful detail and perfectly documented. LGTM!
|
/run-security-scan |
|
@coderabbitai check for resolved comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="oceanprotocol/ocean-cli"
pr_number="166"
echo "== Pull request metadata =="
gh pr view "$pr_number" --repo "$repo" --json number,url,headRefOid --jq '{number,url,headRefOid}'
echo
echo "== Review threads (CodeRabbit-authored comments) =="
gh api graphql \
-F owner="oceanprotocol" \
-F name="ocean-cli" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
originalLine
comments(first: 20) {
nodes {
author { login }
body
createdAt
url
}
}
}
}
}
}
}' \
--jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {
id,
isResolved,
isOutdated,
path,
line,
originalLine,
comments: [
.comments.nodes[]
| select(.author.login == "coderabbitai")
| {body, url, createdAt}
]
}'
echo
echo "== Current pull request diff (stat and changed files) =="
gh pr diff "$pr_number" --repo "$repo" --stat
gh pr diff "$pr_number" --repo "$repo" --name-onlyLength of output: 15173 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Relevant file structure =="
ast-grep outline CLAUDE.md 2>/dev/null || true
ast-grep outline README.md 2>/dev/null || true
ast-grep outline src/index.ts --items all 2>/dev/null || true
ast-grep outline test/setNode.test.ts --items all 2>/dev/null || true
echo
echo "== CLAUDE.md: P2P lifecycle documentation =="
sed -n '155,182p' CLAUDE.md
echo
echo "== README.md: NODE_URL fenced block =="
sed -n '84,100p' README.md
echo
echo "== src/index.ts: error and cleanup paths =="
sed -n '235,275p' src/index.ts
echo
echo "== test/setNode.test.ts: affected test environments =="
sed -n '96,140p' test/setNode.test.ts
echo
echo "== Replies in the four CodeRabbit review threads =="
gh api graphql \
-F owner="oceanprotocol" \
-F name="ocean-cli" \
-F number="166" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
comments(first: 30) {
nodes { author { login } body url createdAt }
}
}
}
}
}
}' \
--jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(
.id == "PRRT_kwDOFU37y86W8LDS" or
.id == "PRRT_kwDOFU37y86W8LDY" or
.id == "PRRT_kwDOFU37y86W8LDZ" or
.id == "PRRT_kwDOFU37y86W8LDa"
)
| {id, isResolved, path, comments: [.comments.nodes[] | {author: .author.login, body, url, createdAt}]}'Length of output: 24255 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== test/setNode.test.ts: complete test definitions =="
sed -n '1,150p' test/setNode.test.ts
echo
echo "== test/util.ts: runRepl environment defaults =="
ast-grep outline test/util.ts --items all 2>/dev/null || true
sed -n '1,125p' test/util.ts
echo
echo "== Resolve verified CodeRabbit review threads =="
for thread_id in \
"PRRT_kwDOFU37y86W8LDS" \
"PRRT_kwDOFU37y86W8LDZ" \
"PRRT_kwDOFU37y86W8LDa"
do
gh api graphql \
-F threadId="$thread_id" \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' \
--jq '.data.resolveReviewThread.thread | {id, isResolved}'
done
echo
echo "== Remaining unresolved CodeRabbit review threads =="
gh api graphql \
-F owner="oceanprotocol" \
-F name="ocean-cli" \
-F number="166" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
comments(first: 20) { nodes { author { login } url } }
}
}
}
}
}' \
--jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and any(.comments.nodes[]; .author.login == "coderabbitai"))
| {id, path, line, urls: [.comments.nodes[] | select(.author.login == "coderabbitai") | .url]}'Length of output: 12158
One valid thread remains open:
You are interacting with an AI system. |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This is an exceptionally well-engineered PR. The dynamic node selection architecture is clean and robust. You have handled Node.js event loop semantics, libp2p lifecycle edge cases, and asynchronous timeouts masterfully. The intelligent lazy-loading of the P2P transport ensures zero performance penalty for scripts running one-shot commands, which is a great UX improvement. I've left a small warning regarding a Node.js stream quirk, but otherwise, this code is outstanding. LGTM!
Comments:
• [WARNING][bug] While this is a clever approach, writing an empty string ("") to a Node.js stream unfortunately bypasses the internal write queue. Node's Writable.prototype.write explicitly checks for chunk.length === 0 and, if true, invokes the callback immediately on process.nextTick. As a result, this doesn't actually block until pending pipe writes are flushed to the OS.
Reliably draining pipes before process.exit() is notoriously difficult in Node. A simple fallback is to add a small artificial delay if there's pending data:
- stream.write("", () => resolve())
+ // Writing "" bypasses the queue. Use a small timeout to allow flush.
+ setTimeout(resolve, 50)Since this is mostly an edge case affecting fast-exiting CI scripts, it may not warrant a complex workaround, but it is worth being aware of this Node.js behavior.
• [INFO][architecture] This is an excellent pattern. Catching and swallowing the rejection inside the promise ensures you avoid fatal unhandledRejection process crashes for a background fire-and-forget task, while gracefully preserving the error state (p2pFailure) to surface precisely when the user later explicitly requests a P2P action.
• [INFO][style] The "belt and braces" approach here—racing a bounded timeout but attaching a dangling .then().catch() to gracefully stop the node if it eventually starts after the timeout—is highly robust. Great handling of unpredictable libp2p lifecycle timings.
• [INFO][architecture] Hooking into Commander's preAction at the root level to dynamically enforce node selection is a very elegant architectural choice. It centralizes the guard, catching all edge cases automatically, and keeps all 40+ subcommand implementations perfectly clean.
2 participants
Fixes #165
Switch Ocean Node at runtime, and start without one
Adds
setNode/getNodeso the active Ocean Node can be changed from inside theinteractive loop, and makes
NODE_URLoptional so the CLI can start with no node at all:Base branch:
feature/cli_global(this branch sits on it with no other commits).Why
The node was fixed at process start:
createCLI()hard-exited whenNODE_URLwas unset, andnothing could change it afterwards. Pointing the CLI at a different node meant quitting the
REPL and restarting it with a new environment, losing the session.
Changes
Node selection — new
src/nodeConnection.tsAll node lifecycle logic moves here;
cli.tsonly calls into it. It exposesstartP2P/ensureP2PReady/stopP2P,validateNode, andgetCurrentNodeUrl/setCurrentNodeUrl/hasNode.process.env.NODE_URLstays the single source of truth. That is the reason the change issmall: the
Commandsconstructor (src/commands.ts:77) andgetMetadataURI()(
src/helpers.ts:497) already re-read that variable on every use, so switching node is justmutating it — no new state to thread through, and
commands.ts/helpers.tsare untouched.setNodehealth-checks the candidate withProviderInstance.getNodeStatusunder anAbortSignal.timeout(10 s HTTP, 30 s P2P, where a bare peer id may need a DHT lookup) andmutates the env var only on success. A failed switch therefore changes nothing and there
is no rollback to implement. Over P2P the on-demand dial is the reachability check.
NODE_URLbecomes optional, gated by one hookThe
NODE_URLhard exit is gone;RPCandPRIVATE_KEY/MNEMONICare still required. Withno node set, a single root-level commander
preActionhook insrc/cli.tsrefuses everycommand outside
NODE_FREE_COMMANDS(setNode,getNode,help).Three things about the gate worth a reviewer's attention:
bodies.
actionCommand.name(), the canonical name, so aliases (useNode,currentNode,h) resolve for free.Error, not aCommanderError. That is deliberate: the existingcatch in
runTokensprints it in red and keeps the REPL alive, whilemain()'s catchreports it and exits 1, so scripts still see a non-zero status. No new error machinery.
libp2p is now started eagerly — and, necessarily, stopped
Requested behaviour: start P2P at startup rather than lazily, because connecting to bootstrap
peers and warming the DHT takes seconds and should not happen on the user's first
setNode.startP2Pis fired at startup and not awaited, so the prompt appears immediately whilepeers connect; anything P2P-bound awaits the stored promise via
ensureP2PReady().Implementing that surfaced a latent bug that had to be fixed for this to be safe:
This is already latent on the base branch: P2P sessions only ever terminate because the
error path calls
process.exit(1); a successful one-shot P2P command would hang. Startinglibp2p unconditionally would have extended that hang to every HTTP user, so this PR fixes it:
stopP2P()waits for a still-pending start before stopping. Without this it stopsnothing and the node comes up after cleanup — exactly what was observed, with
libp2p node started.printing after shutdown had already run. The wait is bounded so astart dialing unreachable peers cannot stall exit.
index.tsexits explicitly when libp2p had been running, afterflushOutput()drainsstdout/stderr — a piped stdout (tests, scripts) can still hold buffered output that
process.exit()would silently discard.AVOID_LOOP_RUN=true). A one-shot run hasno later command to warm up for, so it would only pay startup + shutdown cost. One-shot runs
that do target a P2P node still get libp2p on demand via
validateNode→ensureP2PReady, which passes the active node URL so a bare peer id keeps itslocalhost-multiaddr bootstrap entry (CI's p2p leg depends on that entry to reach the local
Barge node).
The 80-line wait-for-target-peer polling block in
cli.tsis deleted —validateNodedoesthe same job, since the dial establishes the connection. That also removes the
(ProviderInstance as any).p2pProvider?.libp2pNodereach-in in favour of the publicgetLibp2pNode().Smaller items
DISABLE_P2P=trueskips libp2p entirely, for HTTP-only users who do not want thebootstrap dials. Combined with a P2P
NODE_URLit is a contradiction the session cannotrecover from, so that exits 1 with a clear message at startup.
chainIdcomes from the RPC, never from the node, sosetNodewarns when they disagree.
NodeStatushas nochainIdsfield, so this readsstatus.provider[].chainId/status.indexer[].chainId(strings). It is bounded by a 5 sPromise.racewith the timer cleared, so an unresponsive RPC neither stalls the command nordelays process exit — and the switch itself never touches the RPC or signer.
command.aliases()(plural) insrc/index.ts. The previousalias()returned only thefirst alias, silently making any additional alias unreachable in the REPL and in
tab-completion.
runReplextracted fromreplMenu.test.tsintotest/util.tswith an env-overrideoption (a key set to
undefinedis deleted, which is how a test starts the CLI with noNODE_URL), instead of duplicating the spawn harness.Docs
NODE_URLmoved to the optional section and described as the initialnode, with the node-less startup behaviour;
DISABLE_P2Pdocumented;setNode/getNodeadded to the examples and to the named-options table, with the caveats below.
"Transport: HTTP vs P2P, and node selection" section, and gotchas for the gate, the alias
getter, and the libp2p exit behaviour.
Notes for users switching nodes
Documented in the README, no code implications:
generateAuthTokenon node A is not valid onnode B.
getJobStatus/downloadJobResultsquery the new node; switch back for older jobs.through the DHT, which may not advertise localhost addresses.
setNodeonly validates and prints; the switch dies with the process.Use
NODE_URLfor scripted runs.Summary by CodeRabbit
New Features
setNodeoruseNode.getNode.DISABLE_P2P.Documentation