Skip to content

refactor(core): thin the CLI — move parsing/resolution into ant-core, route all daemon calls through the client#157

Open
Nic-dorman wants to merge 8 commits into
mainfrom
fix/cli-audit-thinning
Open

refactor(core): thin the CLI — move parsing/resolution into ant-core, route all daemon calls through the client#157
Nic-dorman wants to merge 8 commits into
mainfrom
fix/cli-audit-thinning

Conversation

@Nic-dorman

Copy link
Copy Markdown
Contributor

Summary

Five mechanical fixes from the CLI-architecture audit (the "CLI should be a thin adapter" contract in CLAUDE.md), one commit per finding:

Behavior notes

  • Parse/lookup error texts are preserved (No node found with service name '…', Expected KEY=VALUE, port-range messages near-identical).
  • Daemon-error surfacing for add/reset changes from Daemon returned error: {body} to HTTP request error: {body} (the shared daemon-client convention).
  • node add download progress moves stdout → stderr, consistent with the rest of the progress UI.

Testing

  • cargo test --workspace: 443 passed (new unit tests for PortRange::from_str, parse_env_vars, resolve_bootstrap_peers priority).
  • clippy -D warnings clean, fmt clean.
  • Live smoke against a running daemon (empty registry): name-miss and name-hit resolution for start/stop, reset --force --json no-op, add --json full 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

Nic and others added 6 commits July 23, 2026 09:19
… (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>
@Nic-dorman
Nic-dorman force-pushed the fix/cli-audit-thinning branch from e3c1540 to c8b032b Compare July 23, 2026 08:52

@dirvine dirvine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Remove unrelated generated/vendor artifacts. This PR adds vendor/ant-node/Cargo.toml.orig, vendor/saorsa-core/.cache/saorsa/bootstrap_cache.json, and vendor/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 *.orig artifacts.

  2. Make resolve_bootstrap_peers uphold its new typed-error contract. ant-core/src/config.rs:90-93 says NoBootstrapPeers is returned when no source yields peers, but lines 102-108 return Ok(vec![]) for an empty manifest or when every manifest MultiAddr is 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 return Error::NoBootstrapPeers for 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>
@Nic-dorman

Copy link
Copy Markdown
Contributor Author

Both review items addressed in ec6d5d5:

  1. Vendor artifacts removedvendor/ant-node/Cargo.toml.orig and the vendor/saorsa-core/.cache/saorsa/bootstrap_cache.{json,lock} files are deleted from the branch, and .gitignore now covers nested .cache/ directories and *.orig files so they can't come back.

  2. resolve_bootstrap_peers contract fixed — a selected manifest that is empty, or whose entries are all filtered out (no resolvable socket address), now returns Error::NoBootstrapPeers instead of Ok([]), with no fallback to the public config. The CLI's only call site propagates the error, so startup fails immediately rather than at the first data operation. Added resolve_bootstrap_errors_on_empty_manifest and resolve_bootstrap_errors_when_all_manifest_peers_filtered (the latter uses a Bluetooth MultiAddr whose socket_addr() is None), and tightened the doc comment to say a selected manifest is authoritative.

cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and the ant-core test suite all pass locally.

@dirvine dirvine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ignoring vendor/ wholesale.
  • A selected manifest is now authoritative: empty or fully filtered manifest peers return Error::NoBootstrapPeers before node startup, with no public-config or peer-cache fallback. The new empty-manifest and non-socket MultiAddr regressions 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants