Repository navigation
fix(cli): use the daemon distro for remote planning and MCP discovery - #626
Conversation
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Thanks, @QinXi-ai, and fast: #618 went up Monday and this covers all of it. All five call sites go through TargetDistro, the #[serde(default)] field keeps both directions compatible, and the fallback warns instead of guessing. Reviewed at 69564c7.
What I checked:
- On the head,
distro::detectsurvives only insideTargetDistro's fallback and the daemon's own detection. - Old daemon, new client: a missing or null
host_distro, or an unknown variant (I sent{"Arch": …}), resolves toNoneand falls back with the warning. New daemon, old client: the CLI and the shell read named fields, so the extra field is ignored. - Trust: the reported distro steers the prompt, the tool list and the client-side routing pre-check.
validate_action_platformstill decides on the daemon side from its own detection, andDistroId::Otherstrings reach the prompt throughnormalise_free_textlike before. - End to end: I ran your binary as
sysknife mcp-serverinsidefedora:41with no network, against a mock daemon on a mounted socket. Ubuntu 24.04 reported → 61 tools includingsysknife_ufw_status. Fedora reported → 38. No field, or an unknown variant → 38 plus the fallback warning. That's the issue's split, fixed. - Reverting the runner lookup makes
planning_and_doctor_use_the_remote_distributionfail atremote_distro.rs:118. - fmt, clippy
--all-targetsand doc pass. The redrustjob is only the test count (1,997 vs the recorded 1,993). CONTRIBUTING says I refresh that at merge, so leave it.
One thing to fix first. Reverting the MCP lookup in SysknifeMcpServer::new doesn't make tools_list_uses_the_daemons_distro_and_falls_back_for_old_daemons fail; it hangs. The mock daemon's thread blocks in listener.accept() (mcp_stdio.rs:134) and the test join()s it at :176. On an Ubuntu host the Ubuntu case passes its assertion without the server ever dialling, then the join never returns. ci.yml sets no timeout-minutes, so CI would sit for six hours instead of going red. Either accept with a deadline, the way remote_distro.rs already does, or assert the mock saw a query_state before joining. Putting the Fedora case first would also make a revert fail on an Ubuntu runner.
Smaller notes, none blocking:
- The fallback warning says "unreachable or too old", and it also fires for an unknown variant or a failed daemon-side detection.
- The MCP server now makes a blocking
QueryState(a fullcollect_state, up to the 10 s socket timeout) before it serves stdio, andrun_intentadds a second one per run. Fine for now; worth one call later. - The unit tests near
mcp_server.rs:2642that callSysknifeMcpServer::new()now dial the default socket on the developer's machine.
#628 fixed the red security-audit on main this afternoon, so updating the branch clears it.
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Thanks for syncing the count, @QinXi-ai. The board is green at 54e6a5c now, rust and security-audit included. Re-reviewed at 54e6a5c.
The new commit touches four files, and none of them is a test:
$ git diff --stat 69564c7 54e6a5c
README.md | 2 +-
docs/distro-support.md | 2 +-
docs/introduction.md | 2 +-
tests/evidence/workspace-tests.json | 2 +-
So the one blocking item from my last review still stands. apps/sysknife-cli/tests/mcp_stdio.rs is byte-identical to 69564c7, which means reverting the MCP lookup in SysknifeMcpServer::new still leaves tools_list_uses_the_daemons_distro_and_falls_back_for_old_daemons blocked in the mock's listener.accept() (mcp_stdio.rs:134) with the test waiting on join() at :176. On a runner where the host distro matches the reported one, the test hangs instead of failing. Either of the two fixes I suggested works: accept with a deadline the way remote_distro.rs already does, or assert the mock saw a query_state before joining.
Everything else from that review holds, and the smaller notes there stay optional. Once the test fails cleanly with the lookup reverted, this is ready from my side.
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Thanks for sticking with this one. The non-blocking accept loop with the FRAME_TIMEOUT deadline resolves the hang I flagged: a reverted lookup now fails the test instead of stalling it.
I proved it at 586178a6 in a no-network container. tools_list_uses_the_daemons_distro_and_falls_back_for_old_daemons passes as written. I then swapped the lookup in SysknifeMcpServer::new back to local detection:
s/DaemonClient::new(resolve_socket_target()).target_distro();/crate::client::TargetDistro::resolve(None);/
and the same test FAILED. I also traced the other three paths to the same lookup: run_intent, run_doctor and plan_intent_inner. The daemon still decides eligibility in validate_action_platform, so a wrong host_distro can only shape the prompt and the tool list. The #[serde(default)] field plus the unknown-variant fallback keeps an old daemon working, and your test covers that None case. The count of 2002 matches across the artifact and the three docs.
Two leftovers I'm happy to take as follow-ups rather than hold this:
SysknifeMcpServer::new().get_info()in the unit tests atmcp_server.rs:2642and:2649now dials the developer's default socket. It's harmless, butfor_distro(None)would keep those tests offline.- The warning still says "unreachable or too old", while the docs add "or cannot identify it". Worth one wording pass someday.
Approving and merging. This closes #618, and I'll write the CHANGELOG line at release. I'll look at #684 next.
Main reached 2002 with lacs-project#626; this branch adds two tests (every_prompt_action_is_offered_by_its_plan_schema and other_prompt_preserves_examples_and_has_an_available_preference_finish), so the artifact and the three published figures move together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UpyrhbDTW1gBwzGAC44GYU
* fix(brain): keep other-host prompts within the action schema * test: move the workspace baseline to 2004 for the two new prompt tests Main reached 2002 with #626; this branch adds two tests (every_prompt_action_is_offered_by_its_plan_schema and other_prompt_preserves_examples_and_has_an_available_preference_finish), so the artifact and the three published figures move together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UpyrhbDTW1gBwzGAC44GYU --------- Co-authored-by: hushen <190065939+918154429@users.noreply.github.com> Co-authored-by: Vladimir Rotariu <rotariuvladimir@gmail.com> Co-authored-by: Vladimir Rotariu <sysknife-development@protonmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Main moved to 2,004 Rust tests (lacs-project#626, lacs-project#684); this branch adds seven, so the artifact and the three published figures read 2,011. No other conflict. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UpyrhbDTW1gBwzGAC44GYU
Minor release. #627 keys request_hash with an HMAC under the audit key (chain_version 4; previews stored by an older release fail closed after upgrading) and #639 changes the history request a newer CLI sends, so a caller relying on the old behaviour notices: the middle digit moves. Also: the planner sees only its host's distribution (#626, #656, #658, #684), checkpoint and the MCP audit tools share the audit-store resolver (#636), every workflow job has a time limit (#662), a weekly cargo audit (#640), release hardening (#635, #637, #660), and every published figure is rendered from its source (#685). Claude-Session: https://claude.ai/code/session_01UpyrhbDTW1gBwzGAC44GYU Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Fixes #618. CLI planning, routing, MCP discovery and doctor use the target daemon's distribution instead of the controller's. A Fedora controller connected to an Ubuntu daemon retains Ubuntu read tools and the Debian planning prompt. The optional, serde-defaulted
host_distrofield preserves compatibility with older daemons; unavailable or older daemons still fall back locally with a warning.The MCP regression mock now accepts connections with a deadline and bounds socket reads and writes. Reverting the MCP daemon-distro lookup on an Ubuntu host produces a clear failure after about 20 seconds instead of hanging on
accept()/join(). This addresses review 5632185343. Existing startup fallback warnings and restart guidance remain intact.Validation
8532cf9, themcp_stdioandremote_distrointegration targets pass: 6 tests, 0 skipped.MCP server did not connect to query the daemon's distro. Restoring production code returns all 6 integration tests to passing.cargo clippy --locked --offline -p sysknife-cli --all-targets --all-features -- -D warnings, formatting and whitespace checks pass.test_baseline: 2002 rust tests, matching the recorded baseline.All current-head checks have finished successfully except the intentionally skippedcontainer-smokejob. The full local board is not claimed green.Local validation limits
scripts/ci-local.sh --no-postgreswas run after synchronizing main. Workspace Clippy, rustdoc and nextest are blocked by missing GLib/GIO/GObject development libraries in WSL. Frontend/setup checks encounter local npm/Node execution and dependency-install problems. Missing lint/security helper tools leave other checks incomplete, and the live Postgres check was explicitly skipped locally. The GitHub frontend, Postgres, dependency-review, security-audit, scripts-lint and secret-scan jobs passed on the tested code head.This contribution is AI-assisted.
Local CI summary