docs: add inventory file for multi-server-devnet skill - #584
Conversation
🤖 Codex Code Review
No Rust consensus paths are touched in this PR, so I did not find any fork-choice, attestation, STF, XMSS, or SSZ-specific concerns in the diff itself. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewThis PR introduces operational tooling (bash/AWK scripts) for multi-server devnet inventory management. It does not modify consensus-critical Rust code. Review Summary: The implementation is robust with excellent error handling (typo-resistant tag validation, strict exit codes) and clear documentation. No security vulnerabilities or correctness issues identified. Minor Suggestions:
Correctness Verification:
Documentation: The Security: No injection vectors detected; user input is passed to AWK via Approve with minor non-blocking suggestions. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewReview: PR 584 —
|
Six ways the selector could hand a caller the wrong host list, all of them silent, which is the failure mode the script exists to prevent: - `--tag ""` matched nothing, so the filter was dropped and the ENTIRE fleet came back at exit 0. An unset `$DEVNET` in `--tag "$DEVNET"` would act on hosts on other chains, the mirror image of the typo case already guarded. - The unknown-argument path printed help to stdout, and the documented idiom captures stdout: a mistyped flag fed 1.4 KB of usage text into a host loop. - `--not-tag T` hard-failed when no host carried T, contradicting the script's own `--tag validator` == `--tag <devnet> --not-tag aggregator` equivalence. An exclusion matching nothing is well-defined; only `--tag` is checked now. - Nothing validated nodes/subnets, so one space in the tags column shifted every later field: a role landed in `nodes`, `--tag validator` returned the aggregator, and a word reached a caller expecting a count. The file is now refused with the row named, rather than answered from. - Tag arguments were glob-expanded against the cwd, so `--tag 'devnet-*'` could silently resolve to a different group than typed. - `usage()` sliced help from a hardcoded line range; it now ends at the first non-comment line, so editing the header cannot truncate `--help`. The documented recipes were also unrunnable as written: `inventory.sh` is not on PATH, and the `| tr` pipeline discarded the exit code the script sets so carefully, turning any error into an empty-but-successful host list.
Follow-up to the input-validation pass, closing the rest of the review: - `validator` was special-cased in three places (a branch in hastag, plus two exemptions in the unknown-tag check) with `devnet-` hardcoded inside the branch, so a second derived tag meant touching all three and a fleet that names chains differently got a silent empty answer. It is now one table entry (prefix + disqualifying tag), and DEVNET_TAG_PREFIX names the prefix. - A literal `validator` in the file was silently ignored, which is exactly the disagreement the derivation exists to rule out: the row said one thing and the computation another. It now names the line and refuses. - `--tag validator` against a file carrying no chain tag at all said only "no host matches", when the actionable fact is that the tag can never match there. - The file-lookup ladder was a copy of devnet-env.sh's, comment included, and had already drifted (return-0 vs exit-2 on nothing found). Both now call one `devnet_find_file`, so the two config files of a deployment cannot end up with two ideas of where they live. - The known-tag list, whose whole job is to be read next to the operator's typo, printed in awk's hash order. Docs: devnet.inventory is the record of which hosts exist and `SERVERS` is a working set filled from a query against it, rather than two hand-kept host lists with no stated precedence. Also notes that the `for h in $SERVERS` workflows rely on bash word-splitting, which zsh does not do, and drops the em-dashes this branch added.
Nothing read `SERVERS`: no script did, and a shell variable can't survive from one command to the next anyway, so as a stored config value it was only ever a second host list to keep in step with devnet.inventory. The workflows now query the inventory in the same command that loops over it, guarded with `|| exit` so a typo'd tag aborts instead of leaving a loop that does nothing and reports success, and `$(echo $hosts)` because zsh doesn't split parameter expansions. `SSH_USER` stays: it isn't a host, the inventory has no column for it, and every documented ssh needs it. Its section is named for what it holds now.
…evnet.env Narrows the previous commit: replacing the `for h in $SERVERS` loops with an inline inventory query per command went further than intended. Which hosts a command runs against is the caller's to pass, and `SERVERS` is how they pass it, so the workflows are back to taking it from the caller and the query that fills it stays documented where inventory.sh is introduced. What devnet.env loses stands: no script reads SERVERS or SSH_USER, so storing them there only created a host list to keep in step with devnet.inventory.
2 participants
🗒️ Description / Motivation
The
devnet.envexample included a list of servers inside a comment. This PR moves that information to a new file, with scripts to query it.What Changed
Correctness / Behavior Guarantees
Tests Added / Run
Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(cargo test --workspace --profile release-fast) — all passing