feat(1463-pr-a): bootstrap-node.sh -- idempotent node bootstrap - #1488
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNew Bash script ChangesNode Bootstrap Mesh Enrollment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17173a8758
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [[ "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; } | ||
| NODE_ID="${2:-}"; shift 2 ;; | ||
| --profile) | ||
| [[ "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; } | ||
| PROFILE="${2:-}"; shift 2 ;; |
There was a problem hiding this comment.
The parser only rejects values that start with --, so a trailing flag like --node-id or --profile with no following token falls through to shift 2 and exits immediately under set -e without the intended requires a value message. This makes common invocation mistakes hard to diagnose and bypasses the script’s own input-validation UX.
Useful? React with 👍 / 👎.
| else | ||
| echo -e " ${RED}✗${RST} ERROR: Profile not found: pmoves/config/profiles/${PROFILE}.yaml" >&2 | ||
| echo "" >&2 | ||
| echo " Run: python deploy/provision/json-to-profile.py --node-id $NODE_ID --profile $PROFILE to generate it" >&2 |
There was a problem hiding this comment.
The error hint tells operators to run deploy/provision/json-to-profile.py, but that file path does not exist in this repository, so the recommended recovery step fails with a second error when a profile is missing. This leaves the bootstrap flow without a valid remediation path at exactly the failure point where guidance is needed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pmoves/scripts/bootstrap-node.sh (1)
121-123: 💤 Low valueConsider stricter hostname matching to avoid false positives.
grep -qF "$NODE_ID"matches substrings, so if NODE_ID isfooand the status output contains a hostfoobar, it would incorrectly reportfooas already enrolled. Using word-boundary matching would be more precise.Proposed fix using word boundaries
ts_status=$(tailscale status 2>/dev/null || true) - if echo "$ts_status" | grep -qF "$NODE_ID"; then + if echo "$ts_status" | grep -qwF "$NODE_ID"; then ok "Tailscale: $NODE_ID already enrolled"The
-wflag adds word-boundary matching to prevent substring false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pmoves/scripts/bootstrap-node.sh` around lines 121 - 123, The current check uses grep -qF "$NODE_ID" which matches substrings (e.g., "foo" in "foobar"); update the matching to use word-boundary matching by changing the grep invocation that checks ts_status for NODE_ID (the line using ts_status and NODE_ID after calling tailscale status) to use grep's -w flag (e.g., grep -qwF "$NODE_ID") so only whole-word matches are considered and false positives are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pmoves/scripts/bootstrap-node.sh`:
- Around line 67-72: The flag handling for --node-id and --profile should
explicitly detect a missing value (empty "${2:-}") before checking for a next
flag; update the blocks that set NODE_ID and PROFILE so they first test -z
"${2:-}" and emit the existing "ERROR: --node-id/--profile requires a value" to
stderr and exit 1, otherwise proceed to assign NODE_ID="${2:-}" (or
PROFILE="${2:-}") and shift 2; apply the same change to both the --node-id and
--profile branches so shift 2 is never called when the value is missing.
- Around line 183-191: Replace the invalid NATS diagnostic command and ensure
piped publishes read stdin: in the connectivity check block that currently calls
"nats server ping --server ${NATS_URL...}" (inside the if command -v nats ...),
call "nats rtt --server ${NATS_URL...}" instead; and wherever JSON is piped into
"nats pub --server ${NATS_URL...} <topic>" (the publish code around the nats pub
usage), add the "--force-stdin" flag so the CLI reads from standard input when
publishing piped JSON.
---
Nitpick comments:
In `@pmoves/scripts/bootstrap-node.sh`:
- Around line 121-123: The current check uses grep -qF "$NODE_ID" which matches
substrings (e.g., "foo" in "foobar"); update the matching to use word-boundary
matching by changing the grep invocation that checks ts_status for NODE_ID (the
line using ts_status and NODE_ID after calling tailscale status) to use grep's
-w flag (e.g., grep -qwF "$NODE_ID") so only whole-word matches are considered
and false positives are avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e315ff2d-dc09-44bf-83d8-3e2609f723a7
📒 Files selected for processing (1)
pmoves/scripts/bootstrap-node.sh
| --node-id) | ||
| [[ "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; } | ||
| NODE_ID="${2:-}"; shift 2 ;; | ||
| --profile) | ||
| [[ "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; } | ||
| PROFILE="${2:-}"; shift 2 ;; |
There was a problem hiding this comment.
Handle missing argument value more gracefully.
When --node-id or --profile is the last argument with no value, the check [[ "${2:-}" == --* ]] passes (empty string doesn't start with --), but then shift 2 fails with an unclear shell error due to set -e. Adding a check for empty values would provide the intended error message.
Proposed fix
--node-id)
- [[ "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; }
+ [[ -z "${2:-}" || "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; }
NODE_ID="${2:-}"; shift 2 ;;
--profile)
- [[ "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; }
+ [[ -z "${2:-}" || "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; }
PROFILE="${2:-}"; shift 2 ;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --node-id) | |
| [[ "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; } | |
| NODE_ID="${2:-}"; shift 2 ;; | |
| --profile) | |
| [[ "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; } | |
| PROFILE="${2:-}"; shift 2 ;; | |
| --node-id) | |
| [[ -z "${2:-}" || "${2:-}" == --* ]] && { echo "ERROR: --node-id requires a value" >&2; exit 1; } | |
| NODE_ID="${2:-}"; shift 2 ;; | |
| --profile) | |
| [[ -z "${2:-}" || "${2:-}" == --* ]] && { echo "ERROR: --profile requires a value" >&2; exit 1; } | |
| PROFILE="${2:-}"; shift 2 ;; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pmoves/scripts/bootstrap-node.sh` around lines 67 - 72, The flag handling for
--node-id and --profile should explicitly detect a missing value (empty
"${2:-}") before checking for a next flag; update the blocks that set NODE_ID
and PROFILE so they first test -z "${2:-}" and emit the existing "ERROR:
--node-id/--profile requires a value" to stderr and exit 1, otherwise proceed to
assign NODE_ID="${2:-}" (or PROFILE="${2:-}") and shift 2; apply the same change
to both the --node-id and --profile branches so shift 2 is never called when the
value is missing.
| if command -v nats >/dev/null 2>&1; then | ||
| if nats server ping --server "${NATS_URL:-nats://localhost:4222}" 2>/dev/null; then | ||
| ok "NATS connectivity: server ping OK" | ||
| else | ||
| warn "NATS server ping failed — NATS may not be running (non-fatal)" | ||
| fi | ||
| else | ||
| warn "nats CLI not found — skipping NATS connectivity probe" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nats CLI server ping command syntax
💡 Result:
The NATS CLI command to ping NATS servers and measure network round-trip time (RTT) is: nats server ping This command provides a list of available servers and their respective RTTs, followed by summary statistics including the number of replies, maximum, minimum, and average latency [1][2]. Note that this is distinct from the internal NATS client PING/PONG protocol, which is a mechanism used by client applications to maintain and verify active connections to the NATS service [3]. The nats server ping command is a diagnostic tool provided by the NATS CLI for administrative and monitoring purposes [1][2].
Citations:
- 1: https://github.com/nats-io/natscli
- 2: https://pkg.go.dev/github.com/bwerthmann/natscli
- 3: https://docs.nats.io/using-nats/developer/connecting/pingpong
🌐 Web query:
nats CLI --server flag nats server ping
💡 Result:
The NATS CLI (nats) uses the --server flag to specify the URL of the NATS server you wish to interact with [1]. This flag is a global option available for most nats commands [1]. Regarding your request to "ping" a NATS server, the NATS CLI does not have a direct command named nats ping. However, you can achieve similar functionality using the nats rtt command, which measures the round-trip time (RTT) to the server [1]. To check the RTT to a specific server, you can use: nats rtt --server If you have configured a NATS context, you can simply run nats rtt to use the default server defined in your context [2][3][1]. Key points: 1. The --server flag (or -s) is used to define the target NATS server URL (e.g., nats://localhost:4222) [1]. 2. There is no nats ping command; nats rtt is the standard tool for checking connectivity and latency [1]. 3. You can also set the server URL via the NATS_URL environment variable or by using NATS contexts (nats context) to avoid typing the flag repeatedly [2][3][1].
Citations:
- 1: https://docs.nats.io/using-nats/nats-tools/nats_cli
- 2: https://github.com/nats-io/natscli
- 3: https://dev.to/karanpratapsingh/introduction-to-nats-cli-33nk
🌐 Web query:
nats pub command syntax piping JSON
💡 Result:
To publish JSON data using the NATS CLI, you can pipe the JSON content directly into the nats pub command [1]. The NATS CLI treats the message body as a string or raw bytes, so you can provide the JSON payload via standard input (stdin) [1]. To force the NATS CLI to read from stdin, use the --force-stdin flag [1]. Example syntax: echo '{"key": "value"}' | nats pub --force-stdin Alternatively, if you have your JSON stored in a file, you can redirect the file content into the command: nats pub --force-stdin < data.json The nats pub command accepts the subject as a required argument [1]. While the CLI does not have a built-in JSON validator, it will transmit whatever data is provided as the message body [1][2]. Ensure your JSON is correctly formatted before piping it, as NATS treats the payload as opaque bytes [3][2].
Citations:
- 1: https://github.com/nats-io/natscli/blob/3e40e78f/cli/pub_command.go
- 2: How to publish and consume JSON data? nats-io/nats-server#5554
- 3: https://docs.nats.io/using-nats/developer/sending/structure
🌐 Web query:
NATS CLI nats pub --server flag arguments
💡 Result:
In the NATS CLI, the --server flag is a global configuration flag used to specify the URL of the NATS server to connect to [1]. It accepts a URL string (e.g., nats://localhost:4222) as its argument [1]. Key details regarding the --server flag: 1. Usage: It can be used with any NATS CLI command, including nats pub, to override the default connection settings [1]. 2. Precedence: The NATS CLI determines the server connection in the following order of precedence: - The --server CLI flag [1]. - The NATS_URL environment variable [1][2]. - The active NATS Context (if configured) [1]. 3. Alternative: Instead of passing the flag every time, it is common practice to use nats context save to store server configurations and switch between them using nats context select [1][3]. Example: nats pub --server nats://your-server:4222 "your message"
Citations:
- 1: https://docs.nats.io/using-nats/nats-tools/nats_cli
- 2: https://natsbyexample.com/examples/messaging/pub-sub/cli
- 3: https://github.com/nats-io/natscli
nats server ping is not a valid NATS CLI command — use nats rtt instead, and add --force-stdin flag to the nats pub command when piping JSON.
Line 185 uses nats server ping --server URL to check NATS connectivity. The NATS CLI does not have a ping subcommand under server; the correct diagnostic tool is nats rtt (measures round-trip time). Similarly, line 221 (and related code in 219–227) pipes JSON to nats pub --server URL topic, but piping to nats pub requires the --force-stdin flag to read from standard input. The --server flag itself is correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pmoves/scripts/bootstrap-node.sh` around lines 183 - 191, Replace the invalid
NATS diagnostic command and ensure piped publishes read stdin: in the
connectivity check block that currently calls "nats server ping --server
${NATS_URL...}" (inside the if command -v nats ...), call "nats rtt --server
${NATS_URL...}" instead; and wherever JSON is piped into "nats pub --server
${NATS_URL...} <topic>" (the publish code around the nats pub usage), add the
"--force-stdin" flag so the CLI reads from standard input when publishing piped
JSON.
Six-step mesh enrollment: Tailscale check, profile validation, Docker network pre-flight, NATS subscribe test, runner label hint, mesh announce. Safe to re-run; warns on missing optional tooling, exits 1 on missing profile, exits 2 if Docker unavailable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace `timeout 3 nats sub` (always exits 124) with `nats server ping` for a real connectivity probe; add --profile flag to the json-to-profile error hint so the suggested command is complete and runnable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…trap-node.sh - Guard tailscale status with || true to prevent spurious re-enrollment - Exit immediately after Docker network create failure (no false mesh announces) - Validate --node-id/--profile values are not flags - Validate ID chars are safe for JSON payload - Use warn() for no-op runner step (green checkmark was misleading) - Remove dead --help pre-scan loop Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…announce Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
65d22bb to
3874c7e
Compare
1 participant
Summary
pmoves/scripts/bootstrap-node.sh— idempotent fleet node bootstrap, safe to re-runaudit_network_reality.shstyle (ANSI helpers, counters, summary block)Step details
|| true)json-to-profile.pyhintpmoves_busDocker network (172.30.3.0/24)nats server ping)mesh.node.announce.v1publishQuality notes
|| trueguard ontailscale statusprevents spurious re-enrollment when daemon is unhealthy--node-id/--profilevalues validated as[a-zA-Z0-9_-]+before any step runswarn()notok()— no misleading green checkmarks for no-opsTest plan
bash pmoves/scripts/bootstrap-node.sh --helpexits 0--node-id --profile fooexits 1 with "requires a value" errorjson-to-profile.pyhint including--profile🤖 Generated with Claude Code
Summary by CodeRabbit