docs(orchestrating-agent-relay): fleet node enrollment + availability diagnostics - #93
Conversation
… diagnostics Adds the two things the skill's fleet section was missing, both learned while recovering a Mac mini fleet node after a reboot. "Is the node actually available?" — online is not the same as available for placement. Documents checking the spawn:* capability list rather than the status field, the two listing traps (fleet nodes hides offline records; output truncates at 64KB through a pipe), and an end-to-end placement proof that spawns from a different machine and verifies the process on the target host. "Enrolling a new machine as a fleet node" — the mint/redeem API flow (POST /api/v1/fleet/enrollment-tokens then /api/v1/fleet/register), linking cloud/dev-stack/fleet-node-bootstrap/README.md as authoritative rather than duplicating it. Calls out the mandatory reaper guard: node up kills every broker whose CWD is the resolved project root, which is relay#1328. Plus five Common Mistakes rows for the CLI behaviours that cost real time, including node agent subcommands ignoring --state-dir and AGENT_RELAY_DATA_DIR (filed separately as relay#1446). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe ChangesFleet relay skill update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 7fc22ed0ec
ℹ️ 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".
| The node-side script and the authoritative reference is | ||
| `cloud/dev-stack/fleet-node-bootstrap/README.md` (supports Daytona, CF | ||
| Containers, the local dev-stack runner, and Mac minis). Inputs are env-only so |
There was a problem hiding this comment.
The published orchestrating-agent-relay package ships only this SKILL.md (prpm.json's files list), while neither cloud/dev-stack/fleet-node-bootstrap/README.md nor sandbox-node-bootstrap.sh exists anywhere in this repository. Consequently, users installing the skill cannot access the claimed authoritative instructions or the script required by the enrollment command; provide a resolvable repository URL and a way to obtain the script.
Useful? React with 👍 / 👎.
| # on the target host: | ||
| pgrep -fl placement-proof # broker pty + CLI process must be present | ||
| agent-relay node agent release placement-proof |
There was a problem hiding this comment.
For the newly enrolled LaunchAgent-backed node described below, this cleanup command will not work: the new Common Mistakes row at line 536 explicitly says node agent release reads only the default connection file, while com.agentrelay.fleet-node starts with --state-dir. Thus the recommended end-to-end proof leaves placement-proof running unless the user independently discovers and applies the later symlink workaround; include that workaround here or use a fleet-side release operation.
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: 3
🤖 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 `@skills/orchestrating-agent-relay/SKILL.md`:
- Line 477: Update the target-side release procedure around the placement-proof
command to work with fleet nodes configured via --state-dir: document a
supported control-plane release path or a safe symlink workaround that verifies
the default connection path is absent, compares its metadata with the target
broker, preserves any pre-existing file, and removes only the symlink created by
the procedure.
- Around line 473-474: Update the relay enrollment examples around the
placement-proof command and the corresponding lines near the later enrollment
example to avoid putting RELAY_AGENT_TOKEN directly in shell history or visible
process arguments. Use a silent read or protected environment-file flow to
populate RELAY_AGENT_TOKEN before invoking agent-relay, while preserving the
existing command behavior.
- Around line 463-465: Update the healthy placement target requirements to
require only the capability matching the requested agent type, rather than every
spawn:* capability. Keep release and relay:delivery-cursor-v1 as separate
lifecycle requirements, and preserve the behavior that records without any spawn
capability cannot be placed on.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01be1b30-5c6a-4f7b-96c7-cab28fb595f8
📒 Files selected for processing (3)
README.mdprpm.jsonskills/orchestrating-agent-relay/SKILL.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Four review findings, all valid and all fixed. Capability requirement was overstated: a node advertising only spawn:claude is a valid target for `fleet spawn claude`. Now requires the spawn:<agent-type> matching the request, with release and relay:delivery-cursor-v1 called out as separate lifecycle capabilities. Tokens no longer appear inline. Both the placement-proof and enrollment examples now populate the token with a silent `read` plus an unset trap, so it stays out of shell history as well as out of ps argv. The enrollment prose claimed env-only inputs while the example contradicted it. Release path was self-contradictory: the proof used `node agent release` on the target host while the same document states that subcommand cannot reach a --state-dir broker. Now uses the control-plane `agent-relay fleet release`, which needs no local connection file. The symlink workaround was actively unsafe. It used `ln -sf`, which destroys a pre-existing connection.json -- and that file is a real regular file on at least one host in this fleet. Replaced with a guarded form that refuses when the path exists and removes only a link it created. Verified both branches: refuses and preserves on the host with the real file, links/runs/cleans on the host without. Bootstrap assets are not shipped with this skill, so the bare relative path was unresolvable for anyone installing it. Now names the AgentWorkforce/cloud repo and states the access requirement. Skipped: SkillSpector npx-pinning warnings (lines 54/75/85/549) are pre-existing and outside this PR's diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four findings were valid and are fixed in c7927c6. Thanks — two of them were real defects, not just wording. 1. Capability requirement overstated (CodeRabbit, L465) — correct, I generalised from a fleet where all four nodes happen to advertise an identical set. Now requires the 2. Tokens in shell history (CodeRabbit L474, Codex) — valid, and the doc contradicted itself: the enrollment prose said "inputs are env-only so secrets never reach 3. Release path unreachable for 4. Symlink workaround was actively unsafe — this one deserves calling out, because it was worse than "document a guard". The row used Validated both branches rather than assuming: 5. Unresolvable bootstrap reference (Codex L490) — valid. The published package ships only Skipped: the SkillSpector The underlying |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/orchestrating-agent-relay/SKILL.md">
<violation number="1" location="skills/orchestrating-agent-relay/SKILL.md:477">
P3: The new silent `read -r -s -p` steps will block (or fail on EOF) when this SKILL.md is executed in a non-interactive / harnessed agent run, which is the primary way an orchestrator consumes this doc — while the change successfully keeps the token out of shell history, it trades that for an interactive stdin prompt. Recommend documenting that the token must be injected via environment in automated contexts (and that `read` is only for interactive/human use), e.g. use `${RELAY_AGENT_TOKEN:?set the token in your environment}` and the `read` snippet only as the interactive fallback, so the spawn/enroll flows stay runnable headlessly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| ```bash | ||
| # Read the token without leaving it in shell history or `ps` argv. | ||
| read -r -s -p 'Agent token: ' RELAY_AGENT_TOKEN; printf '\n' |
There was a problem hiding this comment.
P3: The new silent read -r -s -p steps will block (or fail on EOF) when this SKILL.md is executed in a non-interactive / harnessed agent run, which is the primary way an orchestrator consumes this doc — while the change successfully keeps the token out of shell history, it trades that for an interactive stdin prompt. Recommend documenting that the token must be injected via environment in automated contexts (and that read is only for interactive/human use), e.g. use ${RELAY_AGENT_TOKEN:?set the token in your environment} and the read snippet only as the interactive fallback, so the spawn/enroll flows stay runnable headlessly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/orchestrating-agent-relay/SKILL.md, line 477:
<comment>The new silent `read -r -s -p` steps will block (or fail on EOF) when this SKILL.md is executed in a non-interactive / harnessed agent run, which is the primary way an orchestrator consumes this doc — while the change successfully keeps the token out of shell history, it trades that for an interactive stdin prompt. Recommend documenting that the token must be injected via environment in automated contexts (and that `read` is only for interactive/human use), e.g. use `${RELAY_AGENT_TOKEN:?set the token in your environment}` and the `read` snippet only as the interactive fallback, so the spawn/enroll flows stay runnable headlessly.</comment>
<file context>
@@ -460,21 +460,34 @@ python3 -c 'import json;raw=open("/tmp/nodes.raw").read();d=json.loads(raw[raw.f
-pgrep -fl placement-proof # broker pty + CLI process must be present
-agent-relay node agent release placement-proof
+# Read the token without leaving it in shell history or `ps` argv.
+read -r -s -p 'Agent token: ' RELAY_AGENT_TOKEN; printf '\n'
+export RELAY_AGENT_TOKEN
+trap 'unset RELAY_AGENT_TOKEN' EXIT
</file context>
Proof recipe now demonstrates the check it promised. The prose told readers to spawn from a different machine and confirm dispatchedNodeId, then showed neither. Now numbered STEP 0-3: an explicit different-machine instruction, a parser that extracts dispatchedNodeId from the response (which carries a human-readable preamble before the JSON, so it parses from the first brace), the on-host pgrep, and the release. Adds a note that steps 1 and 2 are separate claims -- control plane records dispatch whether or not anything ran, which is exactly how a broken node looks healthy. Moves the --state-dir workaround out of the table cell into its own subsection. It had outgrown a single row, and a multi-line guarded procedure could not be written safely inside one. The subsection leads with the control-plane path and treats the symlink as the fallback it is. Adds mkdir -p for the parent directory, which may not exist on a freshly provisioned node, and states explicitly why ln -sf must never be used here. Table row shrinks to a pointer at that subsection. Validated, not assumed: the parser against a real response shape, and both guard branches on real hosts -- refuses and preserves on the host with a real connection file (untouched, still dated 2026-08-03), links/runs/cleans on the host without one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Second round addressed in 3532a5e. Both cubic findings on P3, L472 — the recipe didn't demonstrate the check it promised. Correct, and a fair hit: the prose said "spawn from a different machine" and "confirm Note for anyone implementing this: I also added a line making explicit why both steps exist: the control plane records dispatch whether or not anything ran. Step 1 passing alone is precisely how a broken node looks healthy — which is the failure this whole section exists to catch. P2, L559 —
Restructured rather than patched. The workaround had outgrown a table cell — a multi-line guarded procedure can't be written safely inside one, which is how the original Validated both guard branches on real hosts, verbatim from the doc: Already resolved: cubic's L477 finding on No review from |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Parsers no longer crash on unexpected output. Both the roster and STEP 1 probes
used raw.find("{"), which assumes the CLI preamble contains no brace and that the
call succeeded. Neither holds. Now they locate the first brace at start-of-line
via regex and degrade to printing the raw output.
The STEP 1 case mattered most: a traceback there exits non-zero and skips the
STEP 3 release, leaking the running agent the recipe just spawned -- a diagnostic
that leaks the thing it is diagnosing. The new form always exits 0.
Roster parser now prints node id. STEP 1 told readers to compare dispatchedNodeId
against the node's id while the roster command printed only name, so the mandated
comparison was impossible. Also states that dispatchedNodeId is an id, not a name.
Symlink guard now tests -L as well as -e. A dangling link is what an interrupted
run leaves behind, and -e is false for one, so the guard passed and ln -s then
failed with "File exists" while the subcommand silently never ran.
Validated on real hosts, Python 3.9.6 (stock macOS):
- failed-spawn response with a brace in the preamble: old parser raised
JSONDecodeError, new one reports the failure and exits 0
- planted dangling symlink: [ -e ] false, [ -L ] true, new guard refuses
- roster parser prints id=node_d4190c4c... for finn-mini, matching the
dispatchedNodeId observed in a real spawn
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Third round addressed in 1a0fb00. All three valid — the first one was the most serious defect in this PR so far. P2, L490 — brittle parse. Correct, and worse than "prints a traceback". A Reproduced your exact scenario — a preamble containing a P3, L490 — id vs name. Valid, and it made the mandated comparison literally impossible: STEP 1 said "compare P3, L569 — dangling symlink. Valid, and your reasoning is exactly right: a dangling link is precisely what an interrupted run leaves behind, since cleanup is only reached on the happy path. Took the suggested Verified by planting one: All checks validated on a real host running stock Python 3.9.6, so the parser syntax is 3.9-compatible rather than only working on a modern local interpreter. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/orchestrating-agent-relay/SKILL.md`:
- Around line 495-497: Replace the fixed placement-proof name in the
placement-proof workflow with one generated unique identifier, and reuse that
identifier consistently for --name, pgrep checks, and fleet release across the
related examples. Prefer releasing by the returned invocation ID when supported,
while preserving the existing spawn and verification behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c31025e6-07a5-4f25-8459-fe9f35393b21
📒 Files selected for processing (1)
skills/orchestrating-agent-relay/SKILL.md
| agent-relay fleet spawn claude \ | ||
| --name placement-proof --node <node> --channel general \ | ||
| --task "Run hostname -s and reply with its output only." > /tmp/spawn.json |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a unique name for each placement proof.
The fixed name placement-proof can collide with a worker from a previous run or another operator. If spawning fails, agent-relay fleet release placement-proof can still terminate that unrelated worker. A stale process can also satisfy pgrep -fl placement-proof.
Generate one unique name and reuse it for --name, pgrep, and fleet release. Release by the returned invocation ID instead if the CLI supports it.
Proposed fix
+SPAWN_NAME="placement-proof-$(date +%s)-$$"
+
agent-relay fleet spawn claude \
- --name placement-proof --node <node> --channel general \
+ --name "$SPAWN_NAME" --node <node> --channel general \
--task "Run hostname -s and reply with its output only." > /tmp/spawn.json
...
-pgrep -fl placement-proof
+pgrep -fl "$SPAWN_NAME"
...
-agent-relay fleet release placement-proof
+agent-relay fleet release "$SPAWN_NAME"Also applies to: 524-524, 529-529, 641-641
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 54: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 75: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 85: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 652: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 652: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🤖 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 `@skills/orchestrating-agent-relay/SKILL.md` around lines 495 - 497, Replace
the fixed placement-proof name in the placement-proof workflow with one
generated unique identifier, and reuse that identifier consistently for --name,
pgrep checks, and fleet release across the related examples. Prefer releasing by
the returned invocation ID when supported, while preserving the existing spawn
and verification behavior.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/orchestrating-agent-relay/SKILL.md">
<violation number="1" location="skills/orchestrating-agent-relay/SKILL.md:465">
P3: The roster parser's `json.loads` is unguarded, unlike the STEP 1 parser in the same diff that catches `ValueError` to avoid a traceback. If the redirected `/tmp/nodes.raw` is empty or truncated (the very case the surrounding comments warn about), this snippet fails with a raw Python traceback instead of the graceful 'No JSON in output' message — a minor inconsistency with the robustness the PR otherwise added.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| m = re.search(r"^\{", raw, re.M) # first brace at start of a line, not inside the preamble | ||
| if not m: | ||
| raise SystemExit("No JSON in output. Raw:\n" + raw[:500]) | ||
| for n in json.loads(raw[m.start():]).get("nodes", []): |
There was a problem hiding this comment.
P3: The roster parser's json.loads is unguarded, unlike the STEP 1 parser in the same diff that catches ValueError to avoid a traceback. If the redirected /tmp/nodes.raw is empty or truncated (the very case the surrounding comments warn about), this snippet fails with a raw Python traceback instead of the graceful 'No JSON in output' message — a minor inconsistency with the robustness the PR otherwise added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/orchestrating-agent-relay/SKILL.md, line 465:
<comment>The roster parser's `json.loads` is unguarded, unlike the STEP 1 parser in the same diff that catches `ValueError` to avoid a traceback. If the redirected `/tmp/nodes.raw` is empty or truncated (the very case the surrounding comments warn about), this snippet fails with a raw Python traceback instead of the graceful 'No JSON in output' message — a minor inconsistency with the robustness the PR otherwise added.</comment>
<file context>
@@ -456,10 +456,21 @@ still never receive a spawn. Check the capability list, not the status field:
+m = re.search(r"^\{", raw, re.M) # first brace at start of a line, not inside the preamble
+if not m:
+ raise SystemExit("No JSON in output. Raw:\n" + raw[:500])
+for n in json.loads(raw[m.start():]).get("nodes", []):
+ caps = [c["name"] for c in n.get("capabilities", [])]
+ print(f'{n.get("name")} id={n.get("id")} {n.get("status")} live={n.get("live")} {caps}')
</file context>
1 participant
Extends
orchestrating-agent-relay(2.2.0 → 2.3.0) with the two fleet-node gaps found while recovering a Mac mini fleet node after a reboot. No new skill — the existing fleet section already owned this area, and a second skill would have split discovery.What's added
"Is the node actually available?" —
onlineis not the same as available for placement. A node can be live and never receive a spawn. Documents checking thespawn:*capability list rather than the status field, plus an end-to-end placement proof that spawns from a different machine (so it tests placement, not a local spawn), matchesdispatchedNodeId, verifies the process on the target host withpgrep, and releases."Enrolling a new machine as a fleet node" — the two-step API flow (
POST /api/v1/fleet/enrollment-tokens→POST /api/v1/fleet/register), linkingcloud/dev-stack/fleet-node-bootstrap/README.mdas authoritative rather than duplicating it, since that will drift. The reaper guard is called out prominently:agent-relay node upterminates every broker whose CWD is the resolved project root, and a$HOME-rooted workdir reaps every$HOME-rooted broker — relay#1328, a real incident on a shared machine. Also notes enrollment survives reboots via thecom.agentrelay.fleet-nodeLaunchAgent, so a rebooted machine does not need re-enrolling.Five Common Mistakes rows for CLI behaviour that isn't documented anywhere:
fleet nodeshides offline/non-fleet records by default — it hid 385 of 390 on a real workspace, which is why a node that exists appears missingnode agent list/releaseonly read the defaultconnection.json, rejecting--state-dirand ignoringAGENT_RELAY_DATA_DIRfleet spawnrequiresRELAY_AGENT_TOKEN, and--taskis mandatory — the errors surface one at a timeonline≠ availableNotes
--state-diritem is a genuine CLI defect, filed separately as node agent subcommands ignore --state-dir and AGENT_RELAY_DATA_DIR, making --state-dir brokers unmanageable relay#1446. Onlyattachregisters the option; the other tennode agentsubcommands do not. The workaround documented here (temporarily symlinking the connection file) is a footgun and should be replaced by the fix.🤖 Generated with Claude Code