refactor(core): thin the CLI — move parsing/resolution into ant-core, route all daemon calls through the client#157
refactor(core): thin the CLI — move parsing/resolution into ant-core, route all daemon calls through the client#157Nic-dorman wants to merge 8 commits into
Conversation
… (V2-744)
Port-range parsing ("12000" / "12000-12004") lived in ant-cli while the
PortRange type lives in ant-core, so every frontend had to reimplement
it. Parse via the standard FromStr trait next to the type instead; the
CLI now just calls .parse().
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every frontend passing env vars to AddNodeOpts needs the same KEY=VALUE parsing the CLI hand-rolled; expose it as AddNodeOpts::parse_env_vars. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(V2-742) The explicit-peers > devnet-manifest > bootstrap_peers.toml priority logic (with manifest-peer socket-addr filtering) lived in ant-cli, but it's business logic every frontend needs. Move it to config::resolve_bootstrap_peers; the no-source case is now a typed Error::NoBootstrapPeers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ant node add / ant node reset hand-rolled reqwest POSTs against the daemon while every other endpoint goes through ant_core::node::daemon::client. Add client::add_node and client::reset and use them from the CLI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ant node start/stop --service-name read node_registry.json directly to translate name -> ID even while the daemon (the registry's owner) is running, so the daemon could mutate the registry between the CLI's read and the API call. Add client::resolve_node_id_by_name, which resolves through GET /nodes/status, and drop the CLI's direct registry read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CliProgress (node add, unicode bar to stdout) and CliUpdateProgress (self-update, plain percent to stderr) were near-identical impls of the same trait. Keep one CliProgress in ant-cli/src/progress.rs, writing to stderr like the rest of the progress UI. Also fixes: ant node add --json no longer interleaves download-progress output with the JSON result (NoopProgress in JSON mode, matching ant update). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e3c1540 to
c8b032b
Compare
dirvine
left a comment
There was a problem hiding this comment.
Reviewed current head c8b032b10b63bd4f7a0c0497e4f4f4a1bdbd2ae8. The CLI/core moves, daemon routes, name resolution and JSON progress handling look sound, and all 12 CI checks are green. I found two items to address before merge:
-
Remove unrelated generated/vendor artifacts. This PR adds
vendor/ant-node/Cargo.toml.orig,vendor/saorsa-core/.cache/saorsa/bootstrap_cache.json, andvendor/saorsa-core/.cache/saorsa/bootstrap_cache.lock. The cache includes a machine/runtime instance ID and timestamp; none of these files belong to the CLI-thinning change. Please remove them and consider ignoring nested.cache/and*.origartifacts. -
Make
resolve_bootstrap_peersuphold its new typed-error contract.ant-core/src/config.rs:90-93saysNoBootstrapPeersis returned when no source yields peers, but lines 102-108 returnOk(vec![])for an empty manifest or when every manifestMultiAddris filtered out. Because selecting a manifest also disables the public peer cache, the CLI can start apparently successfully with zero peers and fail only during the data operation. Please returnError::NoBootstrapPeersfor an empty filtered manifest result and add tests for an empty manifest and a fully filtered manifest. Do not fall back to the public config when a manifest was explicitly selected.
The empty-manifest behaviour existed in the old CLI helper, so this is not a newly introduced runtime regression; it is a mismatch in the new public ant-core API and misses the purpose of the typed error.
Verified locally: cargo fmt --all -- --check, focused node::types and config tests, cargo check --all-targets; independent review also ran the ant-core library and daemon integration tests successfully.
…tifacts resolve_bootstrap_peers now returns NoBootstrapPeers when a selected manifest is empty or every entry is filtered out, instead of Ok([]) — a selected manifest never falls back to the public config. Also removes accidentally committed vendor cache/.orig files and ignores them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both review items addressed in ec6d5d5:
|
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed current head ec6d5d5d7992cd1a1f2296dec26568137b3d1076 against the prior findings. Both are addressed:
- The stray
vendor.orig/.cache/lock artifacts are removed from the tracked tree, and the ignore rules cover the generated artifact shapes without ignoringvendor/wholesale. - A selected manifest is now authoritative: empty or fully filtered manifest peers return
Error::NoBootstrapPeersbefore node startup, with no public-config or peer-cache fallback. The new empty-manifest and non-socketMultiAddrregressions exercise both paths.
No new code blocker found. Verified locally on this exact head: cargo fmt --all -- --check, four focused bootstrap resolver tests, and cargo check --all-targets all pass. Independent adversarial probes also confirmed explicit peers retain priority and an empty manifest cannot fall back to configured public peers.
Current CI is 9/12 green with no failures; macOS/Ubuntu E2E and Ubuntu Merkle E2E are still in progress. Code review is clear; merge readiness remains conditional on those jobs finishing green.
Summary
Five mechanical fixes from the CLI-architecture audit (the "CLI should be a thin adapter" contract in CLAUDE.md), one commit per finding:
PortRangeparsing (audit docs: comprehensive API documentation for data, wallet, devnet, and library usage #7):impl FromStr for PortRangein ant-core next to the type; CLI uses.parse()instead of its ownparse_port_range.AddNodeOpts::parse_env_varsownsKEY=VALUEparsing; CLI helper removed.bootstrap_peers.tomlpriority (with manifest-peer filtering) moved toant_core::config::resolve_bootstrap_peers; no-source case is now typedError::NoBootstrapPeers.node add/node resetno longer hand-roll reqwest against the daemon — newdaemon::client::add_node/client::resetmatching every other endpoint.node start/stop --service-nameresolved the node ID by readingnode_registry.jsondirectly while the daemon (the registry owner) was running. Newclient::resolve_node_id_by_namegoes throughGET /nodes/status; the CLI no longer touches the registry file at all.ProgressReporterimpls (CliProgressin add.rs,CliUpdateProgressin update.rs) collapsed into oneprogress::CliProgresson stderr. Side fix:ant node add --jsonno longer interleaves download progress with the JSON result (NoopProgress in JSON mode, matchingant update).Behavior notes
No node found with service name '…',Expected KEY=VALUE, port-range messages near-identical).Daemon returned error: {body}toHTTP request error: {body}(the shared daemon-client convention).node adddownload progress moves stdout → stderr, consistent with the rest of the progress UI.Testing
cargo test --workspace: 443 passed (new unit tests forPortRange::from_str,parse_env_vars,resolve_bootstrap_peerspriority).-D warningsclean, fmt clean.reset --force --jsonno-op,add --jsonfull round-trip with a stub binary (then dismissed), client-side parse failures for inverted port range and malformed--env— all outputs byte-compatible with the old CLI.🤖 Generated with Claude Code