feat(core): add FleetManager.listAgentCommands + re-export SlashCommand (fixes #300)#301
Merged
Merged
Conversation
Reading the slash commands available to an agent (to populate a command
palette / autocomplete) previously required consumers to hand-manage a live
`claude` streaming subprocess: openChatSession → listCommands → close, with the
close guaranteed on every path including errors. That lifecycle care belongs in
the library, not duplicated in each consumer (e.g. Paddock).
Add a one-shot `FleetManager.listAgentCommands(agentName, options?)` that opens
a chat session, reads its command list, and ALWAYS closes the session (in a
`finally`, even if the listing throws):
- Returns the full `SlashCommand[]` — { name, description, argumentHint } per
command (built-ins + project `.claude/commands` + MCP-provided commands).
- Accepts the same `ChatSessionOptions` as openChatSession (workingDirectory,
injectedMcpServers) so the list reflects the intended project context.
- Runs on the SDK runtime regardless of the agent's configured runtime (works
for `cli`-runtime agents) and surfaces StreamingSessionUnsupportedError
unchanged for Docker-wrapped agents.
Also re-export the `SlashCommand` type from `@herdctl/core` so consumers can
`import type { SlashCommand } from "@herdctl/core"` instead of reaching into the
Claude Agent SDK directly.
Adds 5 tests (happy path, close-on-throw, agent-not-found, invalid-state,
docker-unsupported), a fleet-manager library-reference doc entry, and a
changeset (minor).
Fixes #300.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a new ChangeslistAgentCommands API
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FleetManager
participant JobControl
participant RuntimeSession
Caller->>FleetManager: listAgentCommands(agentName, options)
FleetManager->>JobControl: listAgentCommands(agentName, options)
JobControl->>RuntimeSession: openChatSession(agentName, options)
JobControl->>RuntimeSession: listCommands()
RuntimeSession-->>JobControl: SlashCommand[]
JobControl->>RuntimeSession: close()
JobControl-->>FleetManager: SlashCommand[]
FleetManager-->>Caller: SlashCommand[]
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Deploying herdctl with
|
| Latest commit: |
1395fb2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1d137382.herdctl.pages.dev |
| Branch Preview URL: | https://feat-list-agent-commands.herdctl.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/fleet-manager/job-control.ts (1)
399-436: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider guarding
session.close()against masking the original error.If
listCommands()throws andsession.close()also throws in thefinallyblock, theclose()error will replace the original error, making debugging harder. SinceJobControlhas access tothis.ctx.getLogger(), you could swallow and log the close error:🛡️ Optional: prevent close() from masking the original error
async listAgentCommands( agentName: string, options?: ChatSessionOptions, ): Promise<SlashCommand[]> { const session = await this.openChatSession(agentName, options); try { return await session.listCommands(); } finally { - // Guarantee teardown of the underlying subprocess on every path, - // including when listCommands() throws. - await session.close(); + // Guarantee teardown of the underlying subprocess on every path, + // including when listCommands() throws. Swallow close errors so they + // don't mask the original failure from listCommands(). + try { + await session.close(); + } catch (closeError) { + this.ctx.getLogger().warn( + `Failed to close session in listAgentCommands: ${(closeError as Error).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 `@packages/core/src/fleet-manager/job-control.ts` around lines 399 - 436, The `listAgentCommands` method in `JobControl` should not let `session.close()` hide an error from `session.listCommands()`. Update the `finally` cleanup so `session.close()` failures are caught and logged with `this.ctx.getLogger()`, while preserving the original exception from `listCommands()` or `openChatSession()`. Keep the teardown behavior in `listAgentCommands` and `openChatSession` intact, but make close errors non-fatal unless no prior error occurred.packages/core/src/fleet-manager/__tests__/list-agent-commands.test.ts (2)
86-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider hoisting the
yamlimport to module level.Both
createConfigandcreateAgentConfigdynamicallyimport("yaml")on every call. A top-level static import would be cleaner and avoid repeated module resolution overhead.♻️ Suggested refactor
+import yaml from "yaml"; + // ... other imports ... async function createConfig(config: object) { const configPath = join(configDir, "herdctl.yaml"); - const yaml = await import("yaml"); await writeFile(configPath, yaml.stringify(config)); return configPath; } async function createAgentConfig(name: string, config: object) { const agentDir = join(configDir, "agents"); await mkdir(agentDir, { recursive: true }); const agentPath = join(agentDir, `${name}.yaml`); - const yaml = await import("yaml"); await writeFile(agentPath, yaml.stringify(config)); return agentPath; }
Note: If the dynamic import was intentional to avoid loading yaml until needed, the current approach is fine — the overhead is negligible in tests.
🤖 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 `@packages/core/src/fleet-manager/__tests__/list-agent-commands.test.ts` around
lines 86 - 100, The helper functions createConfig and createAgentConfig both
dynamically import yaml on every call, so hoist that import to module scope and
reuse it from both helpers. Update the test file to use a single top-level yaml
import, then keep createConfig and createAgentConfig focused on writing the
serialized config without repeated module resolution.
81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The 50ms setTimeout in afterEach appears unnecessary.
Since the SDK is fully mocked and return() resolves synchronously, there are no lingering subprocesses or async operations that would race with the temp directory cleanup. The delay adds 50ms per test (250ms total) without clear benefit. If this was added to work around a specific flaky cleanup issue, a comment explaining why would help future maintainers.
♻️ Suggested cleanup
afterEach(async () => {
- await new Promise((resolve) => setTimeout(resolve, 50));
await rm(tempDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });
});🤖 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 `@packages/core/src/fleet-manager/__tests__/list-agent-commands.test.ts` around
lines 81 - 84, The `afterEach` cleanup in `list-agent-commands.test.ts` includes
an unnecessary 50ms delay before removing `tempDir`; remove the `setTimeout` and
keep the `rm` cleanup directly in `afterEach` since the mocked SDK and
synchronous `return()` do not need a race buffer. If the delay is intentionally
guarding a flaky cleanup edge case, keep the timing only with a brief comment in
`afterEach` explaining why it is required.
🤖 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.
Nitpick comments:
In `@packages/core/src/fleet-manager/__tests__/list-agent-commands.test.ts`:
- Around line 86-100: The helper functions createConfig and createAgentConfig
both dynamically import yaml on every call, so hoist that import to module scope
and reuse it from both helpers. Update the test file to use a single top-level
yaml import, then keep createConfig and createAgentConfig focused on writing the
serialized config without repeated module resolution.
- Around line 81-84: The `afterEach` cleanup in `list-agent-commands.test.ts`
includes an unnecessary 50ms delay before removing `tempDir`; remove the
`setTimeout` and keep the `rm` cleanup directly in `afterEach` since the mocked
SDK and synchronous `return()` do not need a race buffer. If the delay is
intentionally guarding a flaky cleanup edge case, keep the timing only with a
brief comment in `afterEach` explaining why it is required.
In `@packages/core/src/fleet-manager/job-control.ts`:
- Around line 399-436: The `listAgentCommands` method in `JobControl` should not
let `session.close()` hide an error from `session.listCommands()`. Update the
`finally` cleanup so `session.close()` failures are caught and logged with
`this.ctx.getLogger()`, while preserving the original exception from
`listCommands()` or `openChatSession()`. Keep the teardown behavior in
`listAgentCommands` and `openChatSession` intact, but make close errors
non-fatal unless no prior error occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b60af444-127c-4a7d-aa0d-475902ef3535
📒 Files selected for processing (8)
.changeset/list-agent-commands.mddocs/src/content/docs/library-reference/fleet-manager.mdxpackages/core/src/fleet-manager/__tests__/list-agent-commands.test.tspackages/core/src/fleet-manager/fleet-manager.tspackages/core/src/fleet-manager/job-control.tspackages/core/src/runner/index.tspackages/core/src/runner/runtime/index.tspackages/core/src/runner/runtime/interface.ts
Merged
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
1 participant
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
Adds a lightweight, one-shot
FleetManager.listAgentCommands(agentName, options?)that returns the slash commands available to an agent — opening, querying, and closing the underlying streaming session internally — and re-exports theSlashCommandtype from the@herdctl/corebarrel. Gives consumers (e.g. Paddock) a clean way to populate a slash-command autocomplete without hand-managing a liveclaudesubprocess.Fixes #300.
Why
The capability already exists via
RuntimeSession.listCommands(), but consuming it today means correctly managing subprocess lifecycle on every path:That lifecycle care belongs in the library, not duplicated in each consumer.
What changed
FleetManager.listAgentCommands(agentName, options?)(delegates toJobControl) —openChatSession→session.listCommands()→session.close()in afinally(guaranteed teardown even iflistCommands()throws). Returns the fullSlashCommand[]({ name, description, argumentHint }): built-ins + project.claude/commands+ MCP-provided commands, for the resolved session's cwd/config.ChatSessionOptionsasopenChatSession(notablyworkingDirectoryandinjectedMcpServers). Runs on the SDK runtime regardless of the agent's configuredruntime, so it works forcli-runtime agents; surfacesStreamingSessionUnsupportedErrorunchanged for Docker-wrapped agents.SlashCommandfrom@herdctl/core(via the runtime → runner → core barrel chain) so consumers canimport type { SlashCommand } from "@herdctl/core".library-reference/fleet-manager.mdxentry under Session Management Methods.@herdctl/coreminor.Acceptance (from the issue)
listAgentCommands(agent, opts?)returnsSlashCommand[]and always closes the session — the throw-path test asserts the session's teardown (return()) still runs whenlistCommands()rejects.SlashCommandimportable from@herdctl/core(verified in the builtdistbarrel).Tests
New
list-agent-commands.test.ts(5 tests), mocking the SDK so no real subprocess spawns:SlashCommand[]and closes the sessionlistCommands()throwsAgentNotFoundErrorfor an unknown agent (no session opened)InvalidStateErrorbeforeinitialize()StreamingSessionUnsupportedErrorfor Docker-wrapped agentsVerification
pnpm typecheck— 11/11 passpnpm build— 7/7 pass; docs site builds cleanpnpm test(core) — new 5 pass; full suite green except the pre-existing root-onlydirectory.test.tschmod case (passes under non-root CI), unrelated to this changeNotes
The optional short-lived cache floated in the issue is intentionally left out of v1 — each call spawns/tears down a
claudesubprocess (~seconds), documented in the JSDoc/docs, and consumers are encouraged to cache.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SlashCommandis now available for direct use in type definitions.Documentation
Bug Fixes