feat: run select commands while a node is running [skip-line-limit]#1446
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a local TCP control socket and console abstraction, threads Console through CLI commands, introduces global OnceLock caches for DataStore and EventStoreReader, adds builder flags to share store/eventstore across processes, updates workspace manifests and Dockerfiles, and wires socket-based remote command dispatch into start/main flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User CLI
participant Main as main.rs
participant SocketClient as run_on_socket
participant SocketServer as start_socket_server
participant Handler as Socket Handler
participant Builder as CiphernodeBuilder
participant StoreCache as Global Store Cache
participant EventCache as Global Eventstore Cache
User->>Main: invoke command
Main->>Main: create Console & parse CLI
alt socket available & command convertible
Main->>SocketClient: connect + write RemoteCli JSON
SocketServer->>Handler: accept & deserialize RemoteCli
Handler->>Builder: build(with_shared_store/eventstore)
Builder->>StoreCache: get_cached_store()
StoreCache-->>Builder: DataStore? (maybe)
Builder->>EventCache: get_shared_eventstore()
EventCache-->>Builder: EventStoreReader? (maybe)
Handler->>Handler: execute command, stream output lines
Handler-->>SocketClient: send output lines
else local execution
Main->>Main: run cli.execute(out) locally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
added 10 commits
March 18, 2026 06:00
ryardley
force-pushed
the
ry/1406-socket-serialize
branch
from
167badb to
1cb9956
Compare
ryardley
changed the title
added 2 commits
March 18, 2026 09:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/config/src/app_config.rs (1)
79-85:⚠️ Potential issue | 🟠 MajorFail fast on duplicate control ports.
Every node definition that omits
ctrl_portnow falls back to50505. That means existing multi-node configs can still deserialize successfully, but the second node only fails later whenenclave starttries to bind the control socket. Please validatectrl_portuniqueness during config load and return a clear error.Possible validation in
AppConfig::try_from_unscoped// Deliberately clobber default config.nodes.insert("_default".to_string(), config.node); + + let mut ctrl_ports = HashMap::new(); + for (node_name, node) in &config.nodes { + if let Some(previous) = ctrl_ports.insert(node.ctrl_port, node_name) { + bail!( + "ctrl_port {} is configured for both '{}' and '{}'", + node.ctrl_port, + previous, + node_name + ); + } + } let Some(node) = config.nodes.get(name) else { bail!("Could not find node definition for node '{}'. Did you forget to include it in your configuration?", name); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config/src/app_config.rs` around lines 79 - 85, Multiple NodeDefinition defaults set ctrl_port to 50505 which permits duplicate control ports until runtime; update config validation in AppConfig::try_from_unscoped to detect and fail fast on duplicate ctrl_port values during load. Iterate over the deserialized NodeDefinition list, collect ctrl_port values (treat None as default 50505) and return a clear error if any port appears more than once, referencing NodeDefinition::ctrl_port and AppConfig::try_from_unscoped to locate the logic to modify.
♻️ Duplicate comments (1)
crates/socket-server/src/lib.rs (1)
27-43:⚠️ Potential issue | 🟠 MajorThe socket protocol still can't report command failure.
At Line 39 this helper treats EOF as success and returns
Ok(()). Combined withcrates/cli/src/start.rsLine 69 using?, a failing remote command can close the stream without any explicit error frame, and callers cannot distinguish that from a successful run. The protocol needs an explicit success/error status instead of relying on EOF.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/socket-server/src/lib.rs` around lines 27 - 43, run_on_socket currently treats EOF as success because it logs all lines until EOF and returns Ok(()); change it to expect and parse an explicit status frame first: after sending the payload, read the first line from the reader (use the existing lines iterator), parse it as JSON into a small Response struct (e.g. fields status: String and optional error: String) or serde_json::Value, and if status != "ok" return an anyhow::Error containing the remote error message; only after a successful status should you continue streaming remaining lines to log (and if the first read yields None, return an error like "socket closed before status"). Ensure the function (run_on_socket) returns Err on non-OK status so callers using ? can detect remote command failure.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
475-476: Update the stale matrix comment.The comment above still says the base suite is removed, but this matrix now runs both
baseandpersist. Keeping those in sync avoids a false lead when someone debugs CI failures later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 475 - 476, The comment above the matrix is stale; update the comment text to reflect that the CI matrix now runs both test suites. Edit the comment that currently reads "# TODO removed base test for now" to a concise, accurate message such as "# running both base and persist test suites" so it matches the matrix entry "test-suite: [base, persist]" and avoids confusion during CI debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/start.rs`:
- Around line 63-69: The remote socket handler is calling Cli::execute which
triggers autopassword/autowallet setup; instead add a dedicated remote execution
path that avoids lifecycle setup: change the handler (where RemoteCli is parsed)
to call a new method like RemoteCli::execute_remote(out, config_result) or
Cli::execute_readonly_remote(out, config_result) instead of Cli::execute, and
implement that method in crates/cli/src/cli.rs to only perform command
dispatch/handle the remote request (no autopassword/autowallet/autowallet
prompts, no wallet creation or other write-capable setup). Ensure the new method
uses the same dispatch logic as execute but explicitly skips the
autopassword/autowallet code paths so control-socket requests remain read-only.
In `@examples/CRISP/enclave.config.yaml`:
- Around line 43-45: Duplicate YAML key "autonetkey" in the cn2 node entry makes
the mapping invalid; remove the second occurrence so only a single autonetkey
key remains alongside the ctrl_port setting (keep ctrl_port: 50502 and a single
autonetkey: true), and then validate the file with a YAML linter to ensure the
mapping is unique and parses correctly.
---
Outside diff comments:
In `@crates/config/src/app_config.rs`:
- Around line 79-85: Multiple NodeDefinition defaults set ctrl_port to 50505
which permits duplicate control ports until runtime; update config validation in
AppConfig::try_from_unscoped to detect and fail fast on duplicate ctrl_port
values during load. Iterate over the deserialized NodeDefinition list, collect
ctrl_port values (treat None as default 50505) and return a clear error if any
port appears more than once, referencing NodeDefinition::ctrl_port and
AppConfig::try_from_unscoped to locate the logic to modify.
---
Duplicate comments:
In `@crates/socket-server/src/lib.rs`:
- Around line 27-43: run_on_socket currently treats EOF as success because it
logs all lines until EOF and returns Ok(()); change it to expect and parse an
explicit status frame first: after sending the payload, read the first line from
the reader (use the existing lines iterator), parse it as JSON into a small
Response struct (e.g. fields status: String and optional error: String) or
serde_json::Value, and if status != "ok" return an anyhow::Error containing the
remote error message; only after a successful status should you continue
streaming remaining lines to log (and if the first read yields None, return an
error like "socket closed before status"). Ensure the function (run_on_socket)
returns Err on non-OK status so callers using ? can detect remote command
failure.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 475-476: The comment above the matrix is stale; update the comment
text to reflect that the CI matrix now runs both test suites. Edit the comment
that currently reads "# TODO removed base test for now" to a concise, accurate
message such as "# running both base and persist test suites" so it matches the
matrix entry "test-suite: [base, persist]" and avoids confusion during CI
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 882da445-dfaa-424f-a350-04413b0a7af0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.ymlcrates/cli/src/cli.rscrates/cli/src/main.rscrates/cli/src/start.rscrates/config/src/app_config.rscrates/socket-server/Cargo.tomlcrates/socket-server/src/lib.rsexamples/CRISP/enclave.config.yamltemplates/default/enclave.config.yamltests/integration/enclave.config.yaml
✅ Files skipped from review due to trivial changes (2)
- crates/socket-server/Cargo.toml
- templates/default/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/main.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/net/src/net_sync_manager.rs`:
- Around line 370-373: The DirectRequester builder currently uses
max_retries(10) and retry_timeout(Duration::from_secs(20)) which produces long
inter-retry backoff and can stall sync; update the DirectRequester configuration
(the DirectRequester::builder call where max_retries and retry_timeout are set)
to reduce retry_timeout back toward the default (e.g., Duration::from_secs(5))
and/or lower max_retries (e.g., 4) so per-aggregate backoff is bounded and
sequential loop delays don’t compound; adjust only the parameters on the builder
(max_retries and retry_timeout) to these safer values and leave the rest of the
builder chain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3197638-6962-44d4-a24c-5064b7cc71ef
📒 Files selected for processing (2)
crates/net/src/net_sync_manager.rsexamples/CRISP/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/enclave.config.yaml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
Author
|
I solved the issue with connections being closed here so this is ready for review. |
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.
Closes #1406
This allows us to run select commands during
enclave start.Commands are given with this enum and are generated by using the cli with the appropriate command:
Commands are serializable over json. Eg.
will send this
{"name":null,"otel":null,"quiet":false,"config":null,"verbose":0,"command":"WalletGet"}over
tcp://127.0.0.1:50505Currently I send all the other noise over (otel,quiet etc.) despite not really doing anything with it - we might want to look into using this later.
We can add new commands as required. So far I have only added get commands as changing the database while a node is running could lead dto data corruption.
I anticipate we might want to setup a command in the future to stream events for external consumption for example to get events from the eventstore with a cursor. I exposed a shared eventlog when running ciphernodes to aid in this.
This PR also fixes a bug where quic disconnect messages were not being sent on shutdown which was causing issues when trying to reconnect to nodes.
To test:
then in another terminal
Done
Required for dashboard.
Tidy up
ctrl_portconfigurationSummary by CodeRabbit
New Features
Refactor
Chores / Config