feat(skill): spawn-cloud-swarm skill + connect-claude installer (PR 7b relay-side)#866
Open
kjgbot wants to merge 1 commit into
Open
feat(skill): spawn-cloud-swarm skill + connect-claude installer (PR 7b relay-side)#866kjgbot wants to merge 1 commit into
kjgbot wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds MCP preflight validation for cloud.local-mount tools and extends the ChangesCloud Swarm Skill Guidance and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/cli/commands/cloud.ts`:
- Around line 362-365: The catch block that logs install failure for the
"spawn-cloud-swarm" skill should not assume the thrown value is an Error; update
the deps.log call inside the catch(skillErr) to derive the message safely (e.g.,
use skillErr instanceof Error ? skillErr.message : String(skillErr)) so
non-Error throwables (null, string, etc.) won't cause a secondary exception;
adjust the log line that currently casts to Error and reference the same context
message used today.
In `@test/skills/spawn-cloud-swarm.test.ts`:
- Around line 141-146: The polling loop exits too early because it breaks when
either the mount is stopped OR when no agents are running; change the
termination to require both conditions before breaking. In the loop that calls
harness.status() and harness.list() (variables mountStatus and agents, loop
using opts.pollIterations), replace the two separate if-break checks with a
single combined check that breaks only when mountStatus.running is false AND
agents.every((a) => a.status !== 'running') is true so the poll continues until
both the mount is not running and no agents remain running.
- Around line 125-139: The test uses a single shared backoffWaits counter
causing earlier calls to harness.spawn to consume retries for all workers; move
the retry budget to be per-worker by declaring and initializing backoffWaits
inside the worker loop (e.g., inside the for loop body) or otherwise reset it
before each spawn attempt, and keep the same catch logic that checks code ===
'QUOTA_EXCEEDED' and calls await opts.sleep(10_000) up to three times per
worker.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3f3b482-046e-475e-a69a-6306a6930468
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdpackage.jsonprpm.jsonskills/spawn-cloud-swarm/SKILL.mdsrc/cli/commands/cloud.test.tssrc/cli/commands/cloud.tssrc/cli/lib/install-skill.test.tssrc/cli/lib/install-skill.tssrc/cli/lib/mcp-preflight.test.tssrc/cli/lib/mcp-preflight.tstest/skills/spawn-cloud-swarm.test.ts
Comment on lines
+362
to
+365
| } catch (skillErr) { | ||
| deps.log( | ||
| `warning: failed to install spawn-cloud-swarm skill (${(skillErr as Error).message ?? 'unknown error'}); connect succeeded.` | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: locate unsafe casted .message access in catch paths.
rg -n -C2 '\(.* as Error\)\.message' src/cli/commands/cloud.tsRepository: AgentWorkforce/relay
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Search for other catch blocks with unsafe casts in the same file
rg -n 'catch|instanceof|as Error' src/cli/commands/cloud.ts | head -30Repository: AgentWorkforce/relay
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Check the context around line 677 to confirm the safe pattern is used similarly
sed -n '673,680p' src/cli/commands/cloud.tsRepository: AgentWorkforce/relay
Length of output: 403
Guard the warning path against non-Error throwables.
Line 364 dereferences .message from a cast value. If installSkill throws null or a string, this catch block can throw and break the fail-open behavior. The codebase already uses the safe pattern elsewhere (line 677): err instanceof Error ? err.message : String(err).
💡 Suggested fix
} catch (skillErr) {
+ const skillMessage =
+ skillErr instanceof Error ? skillErr.message : String(skillErr ?? 'unknown error');
deps.log(
- `warning: failed to install spawn-cloud-swarm skill (${(skillErr as Error).message ?? 'unknown error'}); connect succeeded.`
+ `warning: failed to install spawn-cloud-swarm skill (${skillMessage}); connect succeeded.`
);
}📝 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.
Suggested change
| } catch (skillErr) { | |
| deps.log( | |
| `warning: failed to install spawn-cloud-swarm skill (${(skillErr as Error).message ?? 'unknown error'}); connect succeeded.` | |
| ); | |
| } catch (skillErr) { | |
| const skillMessage = | |
| skillErr instanceof Error ? skillErr.message : String(skillErr ?? 'unknown error'); | |
| deps.log( | |
| `warning: failed to install spawn-cloud-swarm skill (${skillMessage}); connect succeeded.` | |
| ); |
🤖 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 `@src/cli/commands/cloud.ts` around lines 362 - 365, The catch block that logs
install failure for the "spawn-cloud-swarm" skill should not assume the thrown
value is an Error; update the deps.log call inside the catch(skillErr) to derive
the message safely (e.g., use skillErr instanceof Error ? skillErr.message :
String(skillErr)) so non-Error throwables (null, string, etc.) won't cause a
secondary exception; adjust the log line that currently casts to Error and
reference the same context message used today.
Comment on lines
+125
to
+139
| let backoffWaits = 0; | ||
| for (let i = 0; i < opts.workers; ) { | ||
| try { | ||
| await harness.spawn({ workspaceId: 'ws-1' }); | ||
| i += 1; | ||
| } catch (err) { | ||
| const code = (err as Error & { code?: string }).code; | ||
| if (code === 'QUOTA_EXCEEDED' && backoffWaits < 3) { | ||
| backoffWaits += 1; | ||
| await opts.sleep(10_000); | ||
| continue; | ||
| } | ||
| throw err; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reset quota retry budget per worker, not globally.
At Line 125, backoffWaits is shared across all workers. That means earlier workers can consume retries and later workers may fail before getting their own 3 backoff cycles.
🔧 Proposed fix
- let backoffWaits = 0;
- for (let i = 0; i < opts.workers; ) {
- try {
- await harness.spawn({ workspaceId: 'ws-1' });
- i += 1;
- } catch (err) {
- const code = (err as Error & { code?: string }).code;
- if (code === 'QUOTA_EXCEEDED' && backoffWaits < 3) {
- backoffWaits += 1;
- await opts.sleep(10_000);
- continue;
- }
- throw err;
- }
- }
+ for (let i = 0; i < opts.workers; i += 1) {
+ let backoffWaits = 0;
+ // one retry budget per worker spawn
+ // eslint-disable-next-line no-constant-condition
+ while (true) {
+ try {
+ await harness.spawn({ workspaceId: 'ws-1' });
+ break;
+ } catch (err) {
+ const code = (err as Error & { code?: string }).code;
+ if (code === 'QUOTA_EXCEEDED' && backoffWaits < 3) {
+ backoffWaits += 1;
+ await opts.sleep(10_000);
+ continue;
+ }
+ throw err;
+ }
+ }
+ }🤖 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 `@test/skills/spawn-cloud-swarm.test.ts` around lines 125 - 139, The test uses
a single shared backoffWaits counter causing earlier calls to harness.spawn to
consume retries for all workers; move the retry budget to be per-worker by
declaring and initializing backoffWaits inside the worker loop (e.g., inside the
for loop body) or otherwise reset it before each spawn attempt, and keep the
same catch logic that checks code === 'QUOTA_EXCEEDED' and calls await
opts.sleep(10_000) up to three times per worker.
Comment on lines
+141
to
+146
| for (let i = 0; i < opts.pollIterations; i += 1) { | ||
| const mountStatus = await harness.status({ localDir: opts.localDir }); | ||
| const agents = await harness.list(); | ||
| if (!mountStatus.running) break; | ||
| if (agents.every((a) => a.status !== 'running')) break; | ||
| } |
Contributor
There was a problem hiding this comment.
Polling termination condition is too permissive.
Line 144 and Line 145 break on either condition. The procedure contract in this PR context is to poll until mount is not running and no agents are running; current logic can exit early.
🔧 Proposed fix
- if (!mountStatus.running) break;
- if (agents.every((a) => a.status !== 'running')) break;
+ if (!mountStatus.running && agents.every((a) => a.status !== 'running')) break;📝 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.
Suggested change
| for (let i = 0; i < opts.pollIterations; i += 1) { | |
| const mountStatus = await harness.status({ localDir: opts.localDir }); | |
| const agents = await harness.list(); | |
| if (!mountStatus.running) break; | |
| if (agents.every((a) => a.status !== 'running')) break; | |
| } | |
| for (let i = 0; i < opts.pollIterations; i += 1) { | |
| const mountStatus = await harness.status({ localDir: opts.localDir }); | |
| const agents = await harness.list(); | |
| if (!mountStatus.running && agents.every((a) => a.status !== 'running')) break; | |
| } |
🤖 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 `@test/skills/spawn-cloud-swarm.test.ts` around lines 141 - 146, The polling
loop exits too early because it breaks when either the mount is stopped OR when
no agents are running; change the termination to require both conditions before
breaking. In the loop that calls harness.status() and harness.list() (variables
mountStatus and agents, loop using opts.pollIterations), replace the two
separate if-break checks with a single combined check that breaks only when
mountStatus.running is false AND agents.every((a) => a.status !== 'running') is
true so the poll continues until both the mount is not running and no agents
remain running.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+40
to
+46
| 4. The relaycast MCP surface exposes every tool this skill drives. Before | ||
| the prereq checks above, confirm `cloud.local-mount.ensure`, | ||
| `cloud.local-mount.status`, and `cloud.local-mount.stop` are all | ||
| registered on the MCP client. If any are missing, surface | ||
| `MCP_LOCAL_MOUNT_TOOLS_MISSING` and the verbatim remediation: "Upgrade | ||
| `@relaycast/mcp` to a build that includes `cloud.local-mount.*` (see | ||
| relaycast PR `feat/cloud-local-mount-tools`)." Then stop. |
Contributor
There was a problem hiding this comment.
🟡 SKILL.md prereq 4 contradicts its own ordering — MCP tool check listed last but says "before the prereq checks above"
Prereq 4 (line 40) explicitly says "Before the prereq checks above, confirm cloud.local-mount.ensure, cloud.local-mount.status, and cloud.local-mount.stop are all registered on the MCP client." However, it is numbered as step 4 — after steps 1–3. This is directly contradictory: an agent following the numbered list will execute step 3 (which calls cloud.local-mount.status) before step 4 confirms that tool is even registered. If the MCP tools are missing, the agent gets a confusing tool-not-found error at step 3 instead of the intended MCP_LOCAL_MOUNT_TOOLS_MISSING remediation from step 4. Since this SKILL.md is an LLM-consumed instruction set, the contradictory ordering is a functional bug in the agent's procedure.
Prompt for agents
The MCP tool availability check (currently prereq 4) explicitly says it should run "before the prereq checks above" but is listed as the last numbered step. This contradicts the numbered ordering and causes step 3 to call cloud.local-mount.status before step 4 confirms it exists.
To fix: Move the MCP tool availability check to be prereq 1 (renumber current 1→2, 2→3, 3→4), or restructure the text so the "before" instruction is removed and the check naturally fits as a later step that doesn't depend on ordering. The simplest fix is to renumber it as step 0 or move it above the current step 1.
Files to change: skills/spawn-cloud-swarm/SKILL.md, specifically the Prereqs the Skill Enforces section (lines 22-49).
Was this helpful? React with 👍 or 👎 to provide feedback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 12 files
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/spawn-cloud-swarm/SKILL.md">
<violation number="1" location="skills/spawn-cloud-swarm/SKILL.md:40">
P2: Prereq 4 says it should be checked "before the prereq checks above" but is listed last. Move this check to position 1 (or renumber) so tool-availability is verified before any MCP calls are attempted in the subsequent prereqs.</violation>
</file>
<file name="src/cli/commands/cloud.ts">
<violation number="1" location="src/cli/commands/cloud.ts:364">
P2: Unsafe cast `(skillErr as Error).message` will throw a `TypeError` if `installSkill` rejects with `null` or a non-object value, breaking the fail-open intent (connect should still succeed). Use the safe pattern already in this file: `skillErr instanceof Error ? skillErr.message : String(skillErr)`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic
| `cloud.local-mount.status { localDir: CWD }` — if the MCP reports the | ||
| directory is not registered, surface `NEEDS_RELAYFILE_SETUP` and instruct | ||
| the user to run `relayfile setup --local-dir <CWD>`. Then stop. | ||
| 4. The relaycast MCP surface exposes every tool this skill drives. Before |
There was a problem hiding this comment.
P2: Prereq 4 says it should be checked "before the prereq checks above" but is listed last. Move this check to position 1 (or renumber) so tool-availability is verified before any MCP calls are attempted in the subsequent prereqs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/spawn-cloud-swarm/SKILL.md, line 40:
<comment>Prereq 4 says it should be checked "before the prereq checks above" but is listed last. Move this check to position 1 (or renumber) so tool-availability is verified before any MCP calls are attempted in the subsequent prereqs.</comment>
<file context>
@@ -0,0 +1,113 @@
+ `cloud.local-mount.status { localDir: CWD }` — if the MCP reports the
+ directory is not registered, surface `NEEDS_RELAYFILE_SETUP` and instruct
+ the user to run `relayfile setup --local-dir <CWD>`. Then stop.
+4. The relaycast MCP surface exposes every tool this skill drives. Before
+ the prereq checks above, confirm `cloud.local-mount.ensure`,
+ `cloud.local-mount.status`, and `cloud.local-mount.stop` are all
</file context>
| } | ||
| } catch (skillErr) { | ||
| deps.log( | ||
| `warning: failed to install spawn-cloud-swarm skill (${(skillErr as Error).message ?? 'unknown error'}); connect succeeded.` |
There was a problem hiding this comment.
P2: Unsafe cast (skillErr as Error).message will throw a TypeError if installSkill rejects with null or a non-object value, breaking the fail-open intent (connect should still succeed). Use the safe pattern already in this file: skillErr instanceof Error ? skillErr.message : String(skillErr).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/commands/cloud.ts, line 364:
<comment>Unsafe cast `(skillErr as Error).message` will throw a `TypeError` if `installSkill` rejects with `null` or a non-object value, breaking the fail-open intent (connect should still succeed). Use the safe pattern already in this file: `skillErr instanceof Error ? skillErr.message : String(skillErr)`.</comment>
<file context>
@@ -330,6 +342,29 @@ export function registerCloudCommands(program: Command, overrides: Partial<Cloud
+ }
+ } catch (skillErr) {
+ deps.log(
+ `warning: failed to install spawn-cloud-swarm skill (${(skillErr as Error).message ?? 'unknown error'}); connect succeeded.`
+ );
+ }
</file context>
kjgbot
force-pushed
the
feat/spawn-cloud-swarm-skill
branch
from
22c8af0 to
b1c47c9
Compare
kjgbot
force-pushed
the
feat/spawn-cloud-swarm-skill
branch
from
c684d4e to
f04ddb4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/cloud.ts (1)
361-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
cloud_auth.skill_installedin connect telemetry to match the stated contract.The connect telemetry payload currently omits
skill_installed, so downstream analytics cannot distinguish “already installed” vs “guidance shown”.Suggested fix
const started = Date.now(); let success = false; let errorClass: string | undefined; const trackedProvider = normalizeProvider(providerArg); + let skillInstalledState: boolean | undefined; try { const result = await connectProvider({ @@ success = result.success; if (success && trackedProvider === 'anthropic') { try { const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled; - if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) { + skillInstalledState = skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME); + if (!skillInstalledState) { deps.log( `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}` ); } @@ track('cloud_auth', { action: 'connect', success, duration_ms: Date.now() - started, provider: trackedProvider, + ...(trackedProvider === 'anthropic' ? { skill_installed: skillInstalledState } : {}), ...(errorClass ? { error_class: errorClass } : {}), });🤖 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 `@src/cli/commands/cloud.ts` around lines 361 - 379, The telemetry payload sent by track('cloud_auth', ...) is missing the skill_installed flag; update the finally block to set a boolean skill_installed and include it in the tracked properties (e.g., compute skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled and call skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME) when provider is 'anthropic' or set false otherwise), then add ...(typeof skill_installed !== 'undefined' ? { skill_installed: skill_installed } : {}) to the object passed to track so the connect event includes cloud_auth.skill_installed.
🧹 Nitpick comments (1)
src/cli/commands/cloud.test.ts (1)
389-435: ⚡ Quick winAdd regression tests for “skill probe throws” and connect telemetry payload.
Current new tests cover install guidance visibility, but they don’t lock in fail-open behavior or
skill_installedtelemetry oncloud_auth.🤖 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 `@src/cli/commands/cloud.test.ts` around lines 389 - 435, Add two regression tests: one that simulates the skill probe throwing and asserts the CLI still proceeds (fail-open) by mocking skillInstalled to throw and calling program.parseAsync(['node','agent-relay','cloud','connect','claude']) and verifying connect still calls connectProviderMock (or logs success) rather than crashing; and another that invokes the cloud auth flow (program.parseAsync(['node','agent-relay','cloud','auth', ...]) or the existing cloud connect path where telemetry is emitted) with skillInstalled mocked true/false and asserts deps.telemetry.track was called with a payload containing the key "skill_installed" (boolean) to lock in telemetry behavior. Use the existing helpers/IDs from the diff: createHarness, connectProviderMock, skillInstalled, program.parseAsync and deps.telemetry (or deps) to locate where to add these tests.
🤖 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 `@src/cli/commands/cloud.ts`:
- Around line 361-368: The Claude skill probe (skillInstalled /
hasClaudeSkillInstalled) can throw and bubble up turning a successful connect
into failure; wrap the probe call inside a try/catch when running the anthopic
branch (the block that checks success && trackedProvider === 'anthropic') so any
exception is treated as "fail-open" (assume installed or at least do not throw)
and optionally log a warning via deps.log; specifically protect the call to
skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME) (and the fallback
hasClaudeSkillInstalled) so an exception does not propagate out of the connect
flow and only logs the error without changing the overall success state.
---
Outside diff comments:
In `@src/cli/commands/cloud.ts`:
- Around line 361-379: The telemetry payload sent by track('cloud_auth', ...) is
missing the skill_installed flag; update the finally block to set a boolean
skill_installed and include it in the tracked properties (e.g., compute
skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled and call
skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME) when provider is 'anthropic' or set
false otherwise), then add ...(typeof skill_installed !== 'undefined' ? {
skill_installed: skill_installed } : {}) to the object passed to track so the
connect event includes cloud_auth.skill_installed.
---
Nitpick comments:
In `@src/cli/commands/cloud.test.ts`:
- Around line 389-435: Add two regression tests: one that simulates the skill
probe throwing and asserts the CLI still proceeds (fail-open) by mocking
skillInstalled to throw and calling
program.parseAsync(['node','agent-relay','cloud','connect','claude']) and
verifying connect still calls connectProviderMock (or logs success) rather than
crashing; and another that invokes the cloud auth flow
(program.parseAsync(['node','agent-relay','cloud','auth', ...]) or the existing
cloud connect path where telemetry is emitted) with skillInstalled mocked
true/false and asserts deps.telemetry.track was called with a payload containing
the key "skill_installed" (boolean) to lock in telemetry behavior. Use the
existing helpers/IDs from the diff: createHarness, connectProviderMock,
skillInstalled, program.parseAsync and deps.telemetry (or deps) to locate where
to add these tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b3b578e-43ac-4ce9-a3d6-af6875a440ad
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdsrc/cli/commands/cloud.test.tssrc/cli/commands/cloud.tssrc/cli/lib/mcp-preflight.test.tssrc/cli/lib/mcp-preflight.ts
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- README.md
Comment on lines
+361
to
+368
| if (success && trackedProvider === 'anthropic') { | ||
| const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled; | ||
| if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) { | ||
| deps.log( | ||
| `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}` | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make the Claude skill probe fail-open so connect success is not downgraded to failure.
If skillInstalled(...) throws (override bug, filesystem edge case), the exception bubbles to the outer catch and turns a successful connect into a failed command.
Suggested fix
success = result.success;
if (success && trackedProvider === 'anthropic') {
- const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled;
- if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) {
- deps.log(
- `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}`
- );
- }
+ try {
+ const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled;
+ if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) {
+ deps.log(
+ `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}`
+ );
+ }
+ } catch (skillErr) {
+ const message = skillErr instanceof Error ? skillErr.message : String(skillErr ?? 'unknown error');
+ deps.log(`warning: unable to verify Claude skill installation (${message})`);
+ }
}📝 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.
Suggested change
| if (success && trackedProvider === 'anthropic') { | |
| const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled; | |
| if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) { | |
| deps.log( | |
| `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}` | |
| ); | |
| } | |
| } | |
| if (success && trackedProvider === 'anthropic') { | |
| try { | |
| const skillInstalled = deps.skillInstalled ?? hasClaudeSkillInstalled; | |
| if (!skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME)) { | |
| deps.log( | |
| `Install ${SPAWN_CLOUD_SWARM_SKILL_NAME} before spawning cloud swarms: ${SPAWN_CLOUD_SWARM_SKILL_INSTALL_COMMAND}` | |
| ); | |
| } | |
| } catch (skillErr) { | |
| const message = skillErr instanceof Error ? skillErr.message : String(skillErr ?? 'unknown error'); | |
| deps.log(`warning: unable to verify Claude skill installation (${message})`); | |
| } | |
| } |
🤖 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 `@src/cli/commands/cloud.ts` around lines 361 - 368, The Claude skill probe
(skillInstalled / hasClaudeSkillInstalled) can throw and bubble up turning a
successful connect into failure; wrap the probe call inside a try/catch when
running the anthopic branch (the block that checks success && trackedProvider
=== 'anthropic') so any exception is treated as "fail-open" (assume installed or
at least do not throw) and optionally log a warning via deps.log; specifically
protect the call to skillInstalled(SPAWN_CLOUD_SWARM_SKILL_NAME) (and the
fallback hasClaudeSkillInstalled) so an exception does not propagate out of the
connect flow and only logs the error without changing the overall success state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spawn-cloud-swarmskill and thecloud connect claudeauto-install path from relaycloud connect clauderemind users to installspawn-cloud-swarmfrom AgentWorkforce/skills before using cloud swarmscloud.local-mount.*tool surfaceFeedback addressed
conflictCount; skill guidance points users to inspect.relay/conflicts/Validation
npm exec vitest run src/cli/commands/cloud.test.ts src/cli/lib/mcp-preflight.test.tsnpm run typechecknpm run lint(passes with existing warnings)Merge order