feat(core): agent self-scheduling via MCP server#192
Open
oheckmann74 wants to merge 26 commits into
Open
Conversation
Allow agents to create, update, and delete their own schedules at runtime through a capability-gated MCP server. This solves the problem where agents couldn't reliably set up dynamic schedules (e.g., FedEx tracking) because they didn't know the correct config file path. - Add `self_scheduling` config field (enabled, max_schedules, min_interval) - Create per-agent YAML store at .herdctl/dynamic-schedules/<agent>.yaml - Implement herdctl-scheduler stdio MCP server with 4 tools - Auto-inject MCP server into agents with self_scheduling.enabled - Merge dynamic schedules with static ones in scheduler (static wins) - Add source: "static" | "dynamic" tag to ScheduleInfo for visibility - Safety: namespace isolation, count limits, min interval, TTL support Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix qualified names with dots (e.g., "fleet.agent") rejected by file path validation — allow dots but block ".." path traversal sequences - Fix getSchedule/enableSchedule/disableSchedule not finding dynamic schedules — fall back to dynamic schedule lookup from disk - Fix validateMinInterval passing interval value as schedule name in MinIntervalViolationError - Add prompt max length (10,000 chars) to MCP tool schema to match storage schema validation - Add console.warn for corrupt dynamic schedule files instead of silent catch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds agent self-scheduling: an auto-injected Changes
Sequence DiagramsequenceDiagram
participant AgentConfig as Agent Config
participant FleetMgr as Fleet Manager
participant MCP as herdctl-scheduler MCP
participant Store as Dynamic Store (.herdctl/dynamic-schedules)
participant Scheduler as Scheduler
AgentConfig->>FleetMgr: initialize (self_scheduling.enabled)
FleetMgr->>FleetMgr: determine stateDir
FleetMgr->>MCP: inject herdctl-scheduler (env: HERDCTL_AGENT_NAME, HERDCTL_STATE_DIR, HERDCTL_MAX_SCHEDULES, HERDCTL_MIN_INTERVAL, HERDCTL_STATIC_SCHEDULES)
MCP->>Store: create/list/update/delete schedule
Store-->>MCP: return result
Scheduler->>Store: loadAllDynamicSchedules() (periodic)
Store-->>Scheduler: return dynamic schedules
Scheduler->>Scheduler: merge dynamic + static (static wins) and evaluate schedules
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 10
🧹 Nitpick comments (2)
packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts (1)
1-29: Consider mocking filesystem calls for unit-test isolation.These tests currently exercise real disk I/O, which makes the suite closer to integration behavior and less isolated than unit-level expectations.
As per coding guidelines:
Mock external dependencies (SDK, file system, GitHub API) in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts` around lines 1 - 29, Tests are performing real filesystem I/O; replace those with mocked fs calls to isolate unit tests by using Vitest's mocking (vi.mock) for "node:fs/promises" or by injecting a test-controlled directory into the code under test. Specifically, in the tests that call createTempDir and functions like getDynamicSchedulesDir, readDynamicSchedules, createDynamicSchedule, updateDynamicSchedule, deleteDynamicSchedule and listDynamicSchedules, stub/make fakes for mkdir, realpath, readFile, writeFile, rm (and any other fs/promises functions used) to return deterministic values and simulate file contents (YAML strings) or errors (e.g., for MinIntervalViolationError or ScheduleLimitExceededError scenarios); alternatively refactor the code to accept an injectable base directory and pass a fake/in-memory path from tests so no real disk I/O occurs. Ensure mocks are reset/cleared in beforeEach/afterEach.packages/core/src/config/__tests__/self-scheduling.test.ts (1)
117-129: Add one test for default self-scheduling fallback values.You verify custom values, but a case with
{ enabled: true }would lock in the intended defaults (10and"5m").✅ Suggested test addition
+ it("uses default max_schedules and min_interval when omitted", () => { + const agents = [ + makeAgent({ + self_scheduling: { enabled: true } as ResolvedAgent["self_scheduling"], + }), + ]; + + injectSchedulerMcpServers(agents, "/tmp/.herdctl"); + + const env = agents[0].mcp_servers!["herdctl-scheduler"].env!; + expect(env.HERDCTL_MAX_SCHEDULES).toBe("10"); + expect(env.HERDCTL_MIN_INTERVAL).toBe("5m"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/__tests__/self-scheduling.test.ts` around lines 117 - 129, Add a new test that verifies default self-scheduling fallback values when only { enabled: true } is provided: create an agent via makeAgent({ self_scheduling: { enabled: true } }), call injectSchedulerMcpServers(agents, "/tmp/.herdctl"), then read the env from agents[0].mcp_servers!["herdctl-scheduler"].env! and assert HERDCTL_MAX_SCHEDULES equals "10" and HERDCTL_MIN_INTERVAL equals "5m"; this mirrors the existing custom-values test but checks the default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/package.json`:
- Line 22: The package.json was updated to add the "@modelcontextprotocol/sdk"
dependency but the pnpm-lock.yaml is out of sync; regenerate the lockfile by
running pnpm install (or pnpm install --lockfile-only) locally so pnpm-lock.yaml
includes the new "@modelcontextprotocol/sdk" entry, then commit the updated
pnpm-lock.yaml alongside the package.json change so CI (which runs pnpm install
--frozen-lockfile) will succeed.
In `@packages/core/src/config/schema.ts`:
- Around line 860-867: SelfSchedulingSchema currently allows any string for
min_interval; tighten validation by replacing the loose z.string() on the
min_interval field with a validated string using z.string().refine(...) (or a
custom Zod transformer) that enforces the allowed duration format (e.g., a regex
like /^\d+\s*[smhd]$/ or by calling the existing duration parsing helper if one
exists) while preserving the optional/default("5m") behavior; update the
validation message to be descriptive so invalid config files fail at parse time.
In `@packages/core/src/fleet-manager/schedule-management.ts`:
- Around line 56-60: The code swallows errors when calling
loadAllDynamicSchedules(stateDir) (and the analogous call later) which obscures
recoverable I/O/parse failures; update the two catch blocks that wrap
loadAllDynamicSchedules to log a warning including the caught error (e.g., via
your module logger or processLogger) and context (that dynamic schedules failed
to load and stateDir path), instead of silently ignoring the exception, so
callers can see the underlying cause while still falling back to static
schedules.
- Around line 162-165: The thrown ScheduleNotFoundError currently passes only
the static keys from agent.schedules (availableSchedules) which omits any
dynamically-resolved schedule names; update the code that throws
ScheduleNotFoundError so it collects dynamic candidates (e.g., results from your
dynamic schedule lookup/resolution logic), merge them with
Object.keys(agent.schedules) into a single combined list (preserving
uniqueness), and pass that combined list as the availableSchedules argument to
ScheduleNotFoundError so the error message includes both static and dynamic
schedule names.
In `@packages/core/src/mcp/scheduler-mcp.ts`:
- Around line 40-44: The env parsing for AGENT_NAME, STATE_DIR, MAX_SCHEDULES,
MIN_INTERVAL, and STATIC_SCHEDULES is unvalidated (parseInt can produce NaN) and
can silently break scheduleOptions; create a Zod schema that describes and
validates these env vars (types, defaults, min/max for MAX_SCHEDULES, format for
MIN_INTERVAL, array for STATIC_SCHEDULES), replace direct reads/parseInt with a
single validated parse step using that schema, ensure scheduleOptions consumes
the validated values and explicitly handle/report validation errors instead of
allowing NaN or undefined values to propagate (refer to the constants
AGENT_NAME, STATE_DIR, MAX_SCHEDULES, MIN_INTERVAL, STATIC_SCHEDULES and the
scheduleOptions construction).
- Around line 47-53: Replace raw console.error calls with the core logger:
import createLogger from '@herdctl/core', initialize e.g. const logger =
createLogger('scheduler-mcp'), and change the console.error("HERDCTL_AGENT_NAME
environment variable is required") and console.error("HERDCTL_STATE_DIR
environment variable is required") calls to logger.error(...) (preserve the
process.exit(1) behavior). Also find and replace any other console.error usages
in this module (the ones around the same runtime checks and the error at the
later shutdown/validation point) with logger.error to conform to the
createLogger(prefix) guideline.
In `@packages/core/src/scheduler/dynamic-schedules.ts`:
- Around line 282-293: createDynamicSchedule and updateDynamicSchedule are
writing schedule objects that can violate DynamicScheduleSchema (e.g., overlong
prompt, non-positive ttl_hours) which can break later reads; before persisting
the schedule constructed in the DynamicSchedule object, validate/parse it
against DynamicScheduleSchema (use schema.parse or safeParse) and reject with a
clear error if validation fails, ensuring ttl_hours, prompt length,
cron/interval exclusivity, and expires_at consistency are enforced; apply the
same validation step to the analogous code path around the 340-361 block so only
schema-compliant DynamicSchedule instances are written to disk.
- Around line 68-94: Replace the Error base with the shared FleetManagerError
for all dynamic schedule errors: import FleetManagerError from
fleet-manager/errors.ts and change each class to extend FleetManagerError
(update DynamicScheduleError, ScheduleLimitExceededError,
MinIntervalViolationError, ScheduleNameConflictError), keep the existing
descriptive messages passed to super in each constructor, preserve setting
this.name to the specific class name, and ensure constructors follow the
FleetManagerError signature (add metadata if required by FleetManagerError) so
these errors integrate with the core typed error hierarchy.
- Around line 447-449: Replace the raw console.warn call in dynamic-schedules.ts
with the core logger: import createLogger from `@herdctl/core` (or use the
existing createLogger helper), instantiate a logger for this module (e.g.,
createLogger('dynamic-schedules') or similar) and call logger.warn with the same
message including agentName and msg; remove the console.warn usage so runtime
logging follows the project guideline of NEVER using raw console methods.
In `@packages/core/src/scheduler/scheduler.ts`:
- Around line 118-120: dynamic schedules are currently skipped for the first
DYNAMIC_SCHEDULE_RELOAD_INTERVAL ticks because dynamicScheduleLastLoad starts at
0 and isn't reset on restart; update initialization and lifecycle to force an
immediate refresh: set dynamicScheduleLastLoad to a value that makes the next
tick consider the schedule stale (e.g., initialize to
DYNAMIC_SCHEDULE_RELOAD_INTERVAL or -Infinity) and, inside start(), reset
dynamicScheduleLastLoad and clear any dynamic schedule cache so a reload happens
immediately after start; apply the same reset logic where schedules are reloaded
(see the related logic around lines ~320-335) so stop/start won't reuse stale
cached data.
---
Nitpick comments:
In `@packages/core/src/config/__tests__/self-scheduling.test.ts`:
- Around line 117-129: Add a new test that verifies default self-scheduling
fallback values when only { enabled: true } is provided: create an agent via
makeAgent({ self_scheduling: { enabled: true } }), call
injectSchedulerMcpServers(agents, "/tmp/.herdctl"), then read the env from
agents[0].mcp_servers!["herdctl-scheduler"].env! and assert
HERDCTL_MAX_SCHEDULES equals "10" and HERDCTL_MIN_INTERVAL equals "5m"; this
mirrors the existing custom-values test but checks the default behavior.
In `@packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts`:
- Around line 1-29: Tests are performing real filesystem I/O; replace those with
mocked fs calls to isolate unit tests by using Vitest's mocking (vi.mock) for
"node:fs/promises" or by injecting a test-controlled directory into the code
under test. Specifically, in the tests that call createTempDir and functions
like getDynamicSchedulesDir, readDynamicSchedules, createDynamicSchedule,
updateDynamicSchedule, deleteDynamicSchedule and listDynamicSchedules, stub/make
fakes for mkdir, realpath, readFile, writeFile, rm (and any other fs/promises
functions used) to return deterministic values and simulate file contents (YAML
strings) or errors (e.g., for MinIntervalViolationError or
ScheduleLimitExceededError scenarios); alternatively refactor the code to accept
an injectable base directory and pass a fake/in-memory path from tests so no
real disk I/O occurs. Ensure mocks are reset/cleared in beforeEach/afterEach.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/agent-self-scheduling.mdpackages/core/package.jsonpackages/core/src/config/__tests__/self-scheduling.test.tspackages/core/src/config/index.tspackages/core/src/config/schema.tspackages/core/src/config/self-scheduling.tspackages/core/src/fleet-manager/fleet-manager.tspackages/core/src/fleet-manager/schedule-management.tspackages/core/src/fleet-manager/status-queries.tspackages/core/src/fleet-manager/types.tspackages/core/src/mcp/scheduler-mcp.tspackages/core/src/scheduler/__tests__/dynamic-schedules.test.tspackages/core/src/scheduler/dynamic-schedules.tspackages/core/src/scheduler/index.tspackages/core/src/scheduler/scheduler.ts
Comment on lines
+47
to
+53
| console.error("HERDCTL_AGENT_NAME environment variable is required"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (!STATE_DIR) { | ||
| console.error("HERDCTL_STATE_DIR environment variable is required"); | ||
| process.exit(1); |
Contributor
There was a problem hiding this comment.
Replace raw console.error with core logger.
Lines 47, 52, and 315 use console.error directly in runtime code.
Suggested fix
import { z } from "zod";
+import { createLogger } from "../utils/logger.js";
@@
+const logger = createLogger("mcp-scheduler");
@@
if (!AGENT_NAME) {
- console.error("HERDCTL_AGENT_NAME environment variable is required");
+ logger.error("HERDCTL_AGENT_NAME environment variable is required");
process.exit(1);
}
@@
if (!STATE_DIR) {
- console.error("HERDCTL_STATE_DIR environment variable is required");
+ logger.error("HERDCTL_STATE_DIR environment variable is required");
process.exit(1);
}
@@
main().catch((error) => {
- console.error("Failed to start herdctl-scheduler MCP server:", error);
+ logger.error(
+ `Failed to start herdctl-scheduler MCP server: ${error instanceof Error ? error.message : String(error)}`,
+ );
process.exit(1);
});As per coding guidelines: NEVER use raw console.log/warn/error/debug for runtime logging - use createLogger(prefix) from @herdctl/core instead.
Also applies to: 315-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/mcp/scheduler-mcp.ts` around lines 47 - 53, Replace raw
console.error calls with the core logger: import createLogger from
'@herdctl/core', initialize e.g. const logger = createLogger('scheduler-mcp'),
and change the console.error("HERDCTL_AGENT_NAME environment variable is
required") and console.error("HERDCTL_STATE_DIR environment variable is
required") calls to logger.error(...) (preserve the process.exit(1) behavior).
Also find and replace any other console.error usages in this module (the ones
around the same runtime checks and the error at the later shutdown/validation
point) with logger.error to conform to the createLogger(prefix) guideline.
Comment on lines
+68
to
+94
| export class DynamicScheduleError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = "DynamicScheduleError"; | ||
| } | ||
| } | ||
|
|
||
| export class ScheduleLimitExceededError extends DynamicScheduleError { | ||
| constructor(agentName: string, max: number) { | ||
| super(`Agent "${agentName}" has reached the maximum of ${max} dynamic schedules`); | ||
| this.name = "ScheduleLimitExceededError"; | ||
| } | ||
| } | ||
|
|
||
| export class MinIntervalViolationError extends DynamicScheduleError { | ||
| constructor(scheduleName: string, actual: string, minimum: string) { | ||
| super(`Schedule "${scheduleName}" interval "${actual}" is below the minimum "${minimum}"`); | ||
| this.name = "MinIntervalViolationError"; | ||
| } | ||
| } | ||
|
|
||
| export class ScheduleNameConflictError extends DynamicScheduleError { | ||
| constructor(scheduleName: string, reason: string) { | ||
| super(`Cannot create schedule "${scheduleName}": ${reason}`); | ||
| this.name = "ScheduleNameConflictError"; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align dynamic schedule errors with the core typed error base.
These classes currently extend Error directly. In core runtime code, this should use the shared FleetManagerError hierarchy for consistent error handling and metadata.
As per coding guidelines: Extend FleetManagerError from fleet-manager/errors.ts for all errors and always include actionable error messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 68 - 94,
Replace the Error base with the shared FleetManagerError for all dynamic
schedule errors: import FleetManagerError from fleet-manager/errors.ts and
change each class to extend FleetManagerError (update DynamicScheduleError,
ScheduleLimitExceededError, MinIntervalViolationError,
ScheduleNameConflictError), keep the existing descriptive messages passed to
super in each constructor, preserve setting this.name to the specific class
name, and ensure constructors follow the FleetManagerError signature (add
metadata if required by FleetManagerError) so these errors integrate with the
core typed error hierarchy.
- Fix TTL on update: expires_at now computed from Date.now() instead of created_at — setting ttl_hours: 24 during an update expires 24h from now, not 24h from original creation - Fix read/write race on TTL cleanup: reads no longer write back expired entries. Expired schedules are filtered in-memory during reads and cleaned up during mutations (create/update/delete) which already perform a read-modify-write cycle - Fix dynamic schedules missing from getAgentInfo(): buildAgentInfo now accepts and merges dynamic schedules so they appear in CLI status and web dashboard views Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/core/src/scheduler/dynamic-schedules.ts (3)
301-319:⚠️ Potential issue | 🔴 CriticalValidate mutation payloads before writing dynamic schedules to disk.
Lines 301-319 and 361-385 write objects without runtime schema validation. Invalid values (for example
promptlength or non-positivettl_hours) can be persisted and later make reads fail for that agent file.As per coding guidelines: `packages/core/**/*.{ts,tsx}: Use Zod schemas for validation of all external input including config files, CLI arguments, and API payloads`.🛡️ Suggested change
- const schedule: DynamicSchedule = { + const schedule = DynamicScheduleSchema.parse({ type: input.type, ...(input.cron && { cron: input.cron }), ...(input.interval && { interval: input.interval }), prompt: input.prompt, enabled: input.enabled ?? true, created_at: now, ...(input.ttl_hours != null && { ttl_hours: input.ttl_hours, expires_at: new Date(Date.now() + input.ttl_hours * 3600000).toISOString(), }), - }; + }); @@ - file.schedules[scheduleName] = updated; + const validated = DynamicScheduleSchema.parse(updated); + file.schedules[scheduleName] = validated; await writeDynamicSchedules(stateDir, agentName, file); - return updated; + return validated;Also applies to: 361-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 301 - 319, Validate the constructed DynamicSchedule before persisting by adding a Zod schema and parsing the input/schedule object (validate fields like prompt length, non-negative/positive ttl_hours, cron/interval formats, type, and enabled) and throw/return a clear error if parsing fails; apply the same validation in the later block (lines around where schedule is built again) so you validate the input and the final schedule object prior to assigning to file.schedules and calling writeDynamicSchedules (refer to the schedule variable, input object, DynamicSchedule type, and the writeDynamicSchedules call to locate where to integrate Zod parsing).
470-472:⚠️ Potential issue | 🟠 MajorReplace raw
console.warnwith the core logger.Line 472 uses
console.warnin core runtime code; usecreateLogger("dynamic-schedules")andlogger.warn(...)instead.As per coding guidelines: `**/*.{ts,tsx}: NEVER use raw console.log/warn/error/debug for runtime logging - use createLogger(prefix) from `@herdctl/core` instead`.🪵 Suggested change
import { parse as parseYaml } from "yaml"; import { z } from "zod"; +import { createLogger } from "../utils/logger.js"; import { atomicWriteYaml } from "../state/utils/atomic.js"; ... +const logger = createLogger("dynamic-schedules"); ... - console.warn(`[dynamic-schedules] Failed to load schedules for "${agentName}": ${msg}`); + logger.warn(`Failed to load schedules for "${agentName}": ${msg}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 470 - 472, Replace the raw console.warn call in dynamic-schedules with the core logger: import createLogger from `@herdctl/core` (or use existing createLogger), create a module-level logger via createLogger("dynamic-schedules"), and change the console.warn(`[dynamic-schedules] Failed to load schedules for "${agentName}": ${msg}`) to logger.warn(...) using the same message and variables (agentName, msg); ensure the new logger is used wherever console.* is present in this module to comply with logging guidelines.
68-94:⚠️ Potential issue | 🟠 MajorUse
FleetManagerErroras the base for core scheduler errors.Lines 68-94 currently extend
Error, which bypasses the core typed-error handling path.As per coding guidelines: `packages/core/src/**/*.{ts,tsx}: Use typed error classes extending FleetManagerError for error handling`.♻️ Suggested change
+import { FleetManagerError } from "../fleet-manager/errors.js"; ... -export class DynamicScheduleError extends Error { +export class DynamicScheduleError extends FleetManagerError { constructor(message: string) { super(message); this.name = "DynamicScheduleError"; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 68 - 94, Replace the Error base class with FleetManagerError for the scheduler error types so they follow the core typed-error handling; update the declaration of DynamicScheduleError to extend FleetManagerError (and import FleetManagerError), and keep ScheduleLimitExceededError, MinIntervalViolationError, and ScheduleNameConflictError extending DynamicScheduleError (preserving their constructors and .name assignments) so the typed-error path is used consistently across DynamicScheduleError, ScheduleLimitExceededError, MinIntervalViolationError, and ScheduleNameConflictError.
🧹 Nitpick comments (2)
packages/core/src/fleet-manager/status-queries.ts (2)
11-13: Use a per-agent read path ingetAgentInfoByNameinstead of scanning all agents.Line 203 currently loads all dynamic schedule files just to return one agent. Switching to
listDynamicSchedules(stateDir, agent.qualifiedName)avoids unnecessary directory-wide I/O for single-agent status queries.⚡ Suggested change
import type { DynamicSchedule } from "../scheduler/dynamic-schedules.js"; -import { loadAllDynamicSchedules } from "../scheduler/dynamic-schedules.js"; +import { + listDynamicSchedules, + loadAllDynamicSchedules, +} from "../scheduler/dynamic-schedules.js"; @@ - const dynamic = await loadAllDynamicSchedules(this.ctx.getStateDir()); - agentDynamic = dynamic.get(agent.qualifiedName); + agentDynamic = await listDynamicSchedules(this.ctx.getStateDir(), agent.qualifiedName);Also applies to: 200-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fleet-manager/status-queries.ts` around lines 11 - 13, getAgentInfoByName is currently calling loadAllDynamicSchedules which scans the whole state dir; change it to use the per-agent listing API by calling listDynamicSchedules(stateDir, agent.qualifiedName) so we only read the targeted agent's schedule files. Replace the loadAllDynamicSchedules(...) usage in getAgentInfoByName with listDynamicSchedules(...) (import it from "../scheduler/dynamic-schedules.js" if not already), adjust any code that expects the full-array shape to work with the per-agent result, and ensure stateDir and agent.qualifiedName are passed through to the new call.
150-154: Avoid silent fallback when dynamic schedule loading fails.These catches hide parse/read issues and make operator diagnosis harder. Log a
warnwith context before falling back to static schedules.As per coding guidelines: `Choose appropriate log levels: debug for internal details, info for significant events, warn for recoverable issues, error for failures`.♻️ Suggested change
- } catch { - // Ignore — show static schedules at minimum + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + this.ctx + .getLogger() + .warn(`Failed to load dynamic schedules; falling back to static schedules: ${msg}`); } @@ - } catch { - // Ignore — show static schedules at minimum + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + this.ctx + .getLogger() + .warn(`Failed to load dynamic schedules for "${agent.qualifiedName}": ${msg}`); }Also applies to: 200-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fleet-manager/status-queries.ts` around lines 150 - 154, The catch that swallows errors from loadAllDynamicSchedules should log a warning with context and the caught error before falling back to static schedules; update both occurrences (the try/catch around loadAllDynamicSchedules(this.ctx.getStateDir()) and the similar block at lines ~200-207) to catch (err) and call this.ctx.logger.warn or a similar logger, including the state dir (this.ctx.getStateDir()) and err.message/stack so operators can diagnose the parse/read failure while still proceeding to show static schedules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/scheduler/dynamic-schedules.ts`:
- Around line 301-319: Validate the constructed DynamicSchedule before
persisting by adding a Zod schema and parsing the input/schedule object
(validate fields like prompt length, non-negative/positive ttl_hours,
cron/interval formats, type, and enabled) and throw/return a clear error if
parsing fails; apply the same validation in the later block (lines around where
schedule is built again) so you validate the input and the final schedule object
prior to assigning to file.schedules and calling writeDynamicSchedules (refer to
the schedule variable, input object, DynamicSchedule type, and the
writeDynamicSchedules call to locate where to integrate Zod parsing).
- Around line 470-472: Replace the raw console.warn call in dynamic-schedules
with the core logger: import createLogger from `@herdctl/core` (or use existing
createLogger), create a module-level logger via
createLogger("dynamic-schedules"), and change the
console.warn(`[dynamic-schedules] Failed to load schedules for "${agentName}":
${msg}`) to logger.warn(...) using the same message and variables (agentName,
msg); ensure the new logger is used wherever console.* is present in this module
to comply with logging guidelines.
- Around line 68-94: Replace the Error base class with FleetManagerError for the
scheduler error types so they follow the core typed-error handling; update the
declaration of DynamicScheduleError to extend FleetManagerError (and import
FleetManagerError), and keep ScheduleLimitExceededError,
MinIntervalViolationError, and ScheduleNameConflictError extending
DynamicScheduleError (preserving their constructors and .name assignments) so
the typed-error path is used consistently across DynamicScheduleError,
ScheduleLimitExceededError, MinIntervalViolationError, and
ScheduleNameConflictError.
---
Nitpick comments:
In `@packages/core/src/fleet-manager/status-queries.ts`:
- Around line 11-13: getAgentInfoByName is currently calling
loadAllDynamicSchedules which scans the whole state dir; change it to use the
per-agent listing API by calling listDynamicSchedules(stateDir,
agent.qualifiedName) so we only read the targeted agent's schedule files.
Replace the loadAllDynamicSchedules(...) usage in getAgentInfoByName with
listDynamicSchedules(...) (import it from "../scheduler/dynamic-schedules.js" if
not already), adjust any code that expects the full-array shape to work with the
per-agent result, and ensure stateDir and agent.qualifiedName are passed through
to the new call.
- Around line 150-154: The catch that swallows errors from
loadAllDynamicSchedules should log a warning with context and the caught error
before falling back to static schedules; update both occurrences (the try/catch
around loadAllDynamicSchedules(this.ctx.getStateDir()) and the similar block at
lines ~200-207) to catch (err) and call this.ctx.logger.warn or a similar
logger, including the state dir (this.ctx.getStateDir()) and err.message/stack
so operators can diagnose the parse/read failure while still proceeding to show
static schedules.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/fleet-manager/status-queries.tspackages/core/src/scheduler/dynamic-schedules.ts
- Validate min_interval at config parse time via Zod refine - Validate MCP env vars via Zod schema (prevents NaN from parseInt) - Validate schedule payloads via DynamicScheduleSchema.parse() before writing - Replace console.warn with createLogger in dynamic-schedules - Log warnings instead of silently swallowing dynamic schedule load errors - Include dynamic schedule names in ScheduleNotFoundError context - Force immediate dynamic schedule load on scheduler start (reset cache) - Update pnpm-lock.yaml with @modelcontextprotocol/sdk dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/core/src/scheduler/dynamic-schedules.ts (2)
32-41: Schema allows type/field mismatch; validated at runtime.The schema permits
type: "cron"without acronfield. Runtime validation increateDynamicSchedule(lines 261-274) catches this, but a Zod refinement could enforce it at the schema level for earlier, more consistent rejection.♻️ Optional: Add Zod refinement for type/field consistency
export const DynamicScheduleSchema = z.object({ type: z.enum(["cron", "interval"]), cron: z.string().optional(), interval: z.string().optional(), prompt: z.string().max(10000), enabled: z.boolean().default(true), created_at: z.string(), ttl_hours: z.number().positive().optional(), expires_at: z.string().nullable().optional(), -}); +}).refine( + (data) => (data.type === "cron" ? !!data.cron : !!data.interval), + { message: "Cron schedules require 'cron', interval schedules require 'interval'" } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 32 - 41, DynamicScheduleSchema currently allows a type/field mismatch (e.g., type: "cron" without a cron value); add a Zod refinement to DynamicScheduleSchema that enforces when type === "cron" the cron field is present/non-empty, and when type === "interval" the interval field is present/non-empty, so the schema itself rejects inconsistent objects before reaching createDynamicSchedule; reference DynamicScheduleSchema for the change and keep createDynamicSchedule validation in sync or remove redundant runtime checks once schema-level enforcement is added.
520-523: Silent catch may hide unexpected errors during cron gap estimation.The catch block only re-throws
MinIntervalViolationErrorand silently ignores all other errors. While cron syntax is validated earlier, unexpected errors during gap calculation could be masked.♻️ Consider logging unexpected errors
} catch (error) { // Re-throw our own errors, ignore cron parsing failures if (error instanceof MinIntervalViolationError) throw error; + // Log unexpected errors for debugging (cron syntax validated earlier) + if (!(error instanceof Error && error.message.includes("cron"))) { + logger.debug(`Unexpected error in cron gap estimation: ${error}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` around lines 520 - 523, The catch block in dynamic-schedules.ts currently only re-throws MinIntervalViolationError and silently ignores all other exceptions; update that catch to log unexpected errors with context (e.g., schedule id/cron expression and the function performing gap estimation) using the project's logger (e.g., logger.error or processLogger.error) before continuing to ignore them, while preserving the existing re-throw for MinIntervalViolationError so behavior doesn't change for that specific error.packages/core/src/scheduler/scheduler.ts (1)
329-335: Consider usingDynamicScheduletype directly for the cache.The type assertion casts the loaded
DynamicSchedulerecords to a narrower inline type. While this works, it's fragile—ifDynamicSchedulechanges, the cast silently hides mismatches.♻️ Suggested refactor
- // Dynamic schedule cache — re-read from disk every N ticks to avoid excessive I/O - private dynamicScheduleCache: Map< - string, - Record< - string, - { type: string; interval?: string; cron?: string; prompt?: string; enabled?: boolean } - > - > = new Map(); + // Dynamic schedule cache — re-read from disk every N ticks to avoid excessive I/O + private dynamicScheduleCache: Map< + string, + Record<string, DynamicSchedule> + > = new Map();Then import
DynamicScheduletype from./dynamic-schedules.jsand remove the cast at line 329.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/scheduler.ts` around lines 329 - 335, Replace the inline type-cast on this.dynamicScheduleCache with the actual DynamicSchedule type: import the DynamicSchedule type from ./dynamic-schedules.js and assign loaded to this.dynamicScheduleCache using that type (remove the explicit inline Record<...> cast). Update references to dynamicScheduleCache if needed so the compiler uses the imported DynamicSchedule type instead of the fragile inline assertion; ensure the assignment uses the loaded variable as a Map<string, DynamicSchedule> (or Map<string, Record<string, DynamicSchedule>> if the structure is nested) to preserve runtime shape and prevent silent mismatches.packages/core/src/fleet-manager/status-queries.ts (1)
201-211: Inefficient: loads all agents' dynamic schedules to get one agent.
loadAllDynamicSchedulesreads every agent's YAML file, then line 205 extracts just one agent. UselistDynamicSchedulesdirectly for single-agent queries.♻️ Proposed fix
// Load dynamic schedules for this agent let agentDynamic: Record<string, DynamicSchedule> | undefined; try { - const dynamic = await loadAllDynamicSchedules(this.ctx.getStateDir()); - agentDynamic = dynamic.get(agent.qualifiedName); + agentDynamic = await listDynamicSchedules(this.ctx.getStateDir(), agent.qualifiedName); + if (Object.keys(agentDynamic).length === 0) { + agentDynamic = undefined; + } } catch (error) { const msg = error instanceof Error ? error.message : String(error); this.ctx .getLogger() .warn(`Failed to load dynamic schedules for ${agent.qualifiedName}: ${msg}`); }You'll need to import
listDynamicSchedulesif not already imported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fleet-manager/status-queries.ts` around lines 201 - 211, The code currently calls loadAllDynamicSchedules and then picks a single agent (agent.qualifiedName), which is inefficient; replace that with a direct single-agent call using listDynamicSchedules to avoid reading every YAML file. Import listDynamicSchedules if missing, call it with this.ctx.getStateDir() (and the agent identifier/qualifiedName per listDynamicSchedules' signature) to get only this agent's DynamicSchedule, and update the try/catch to assign the result to agentDynamic and preserve the existing warning via this.ctx.getLogger().warn on error.packages/core/src/fleet-manager/schedule-management.ts (1)
77-88: Minor duplication in ScheduleInfo construction.The dynamic schedule ScheduleInfo construction at lines 77-88 and 148-159 is nearly identical. Consider extracting a helper if this pattern appears elsewhere.
Also applies to: 148-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fleet-manager/schedule-management.ts` around lines 77 - 88, The two nearly identical ScheduleInfo constructions (the objects pushed into allSchedules in the allSchedules.push block and the similar block later) should be consolidated into a small helper function (e.g., buildScheduleInfo or makeScheduleInfo) that accepts (name, agent, schedule, scheduleState, source = "dynamic") and returns the ScheduleInfo-shaped object with fields name, agentName (agent.qualifiedName), type, interval, cron, status (scheduleState?.status ?? "idle"), lastRunAt, nextRunAt, lastError, and source; replace both inline object literals with calls to this helper so both locations reuse the same logic and reduce duplication while preserving types and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/fleet-manager/schedule-management.ts`:
- Around line 77-88: The two nearly identical ScheduleInfo constructions (the
objects pushed into allSchedules in the allSchedules.push block and the similar
block later) should be consolidated into a small helper function (e.g.,
buildScheduleInfo or makeScheduleInfo) that accepts (name, agent, schedule,
scheduleState, source = "dynamic") and returns the ScheduleInfo-shaped object
with fields name, agentName (agent.qualifiedName), type, interval, cron, status
(scheduleState?.status ?? "idle"), lastRunAt, nextRunAt, lastError, and source;
replace both inline object literals with calls to this helper so both locations
reuse the same logic and reduce duplication while preserving types and behavior.
In `@packages/core/src/fleet-manager/status-queries.ts`:
- Around line 201-211: The code currently calls loadAllDynamicSchedules and then
picks a single agent (agent.qualifiedName), which is inefficient; replace that
with a direct single-agent call using listDynamicSchedules to avoid reading
every YAML file. Import listDynamicSchedules if missing, call it with
this.ctx.getStateDir() (and the agent identifier/qualifiedName per
listDynamicSchedules' signature) to get only this agent's DynamicSchedule, and
update the try/catch to assign the result to agentDynamic and preserve the
existing warning via this.ctx.getLogger().warn on error.
In `@packages/core/src/scheduler/dynamic-schedules.ts`:
- Around line 32-41: DynamicScheduleSchema currently allows a type/field
mismatch (e.g., type: "cron" without a cron value); add a Zod refinement to
DynamicScheduleSchema that enforces when type === "cron" the cron field is
present/non-empty, and when type === "interval" the interval field is
present/non-empty, so the schema itself rejects inconsistent objects before
reaching createDynamicSchedule; reference DynamicScheduleSchema for the change
and keep createDynamicSchedule validation in sync or remove redundant runtime
checks once schema-level enforcement is added.
- Around line 520-523: The catch block in dynamic-schedules.ts currently only
re-throws MinIntervalViolationError and silently ignores all other exceptions;
update that catch to log unexpected errors with context (e.g., schedule id/cron
expression and the function performing gap estimation) using the project's
logger (e.g., logger.error or processLogger.error) before continuing to ignore
them, while preserving the existing re-throw for MinIntervalViolationError so
behavior doesn't change for that specific error.
In `@packages/core/src/scheduler/scheduler.ts`:
- Around line 329-335: Replace the inline type-cast on this.dynamicScheduleCache
with the actual DynamicSchedule type: import the DynamicSchedule type from
./dynamic-schedules.js and assign loaded to this.dynamicScheduleCache using that
type (remove the explicit inline Record<...> cast). Update references to
dynamicScheduleCache if needed so the compiler uses the imported DynamicSchedule
type instead of the fragile inline assertion; ensure the assignment uses the
loaded variable as a Map<string, DynamicSchedule> (or Map<string, Record<string,
DynamicSchedule>> if the structure is nested) to preserve runtime shape and
prevent silent mismatches.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/core/src/config/schema.tspackages/core/src/fleet-manager/schedule-management.tspackages/core/src/fleet-manager/status-queries.tspackages/core/src/mcp/scheduler-mcp.tspackages/core/src/scheduler/dynamic-schedules.tspackages/core/src/scheduler/scheduler.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/config/schema.ts
- packages/core/src/mcp/scheduler-mcp.ts
- Log unexpected errors in cron gap estimation instead of silencing - Use DynamicSchedule type directly for scheduler cache (remove fragile cast) - Use listDynamicSchedules for single-agent lookup in getAgentInfoByName Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/scheduler/dynamic-schedules.ts (1)
447-447: Consider using a top-level import forreaddir.The dynamic import of
readdirat runtime is unusual when otherfs/promisesfunctions (mkdir,readFile,unlink) are imported statically at the top of the file.♻️ Proposed refactor
-import { mkdir, readFile, unlink } from "node:fs/promises"; +import { mkdir, readdir, readFile, unlink } from "node:fs/promises"; ... export async function loadAllDynamicSchedules( stateDir: string, ): Promise<Map<string, Record<string, DynamicSchedule>>> { - const { readdir } = await import("node:fs/promises"); const dir = getDynamicSchedulesDir(stateDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/scheduler/dynamic-schedules.ts` at line 447, Replace the runtime dynamic import that extracts readdir (const { readdir } = await import("node:fs/promises");) with a top-level static import alongside the existing mkdir, readFile, unlink imports; update the import list at the top of the module to include readdir so the scheduler/dynamic-schedules.ts code uses the statically imported readdir symbol instead of performing an await import at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/scheduler/dynamic-schedules.ts`:
- Line 447: Replace the runtime dynamic import that extracts readdir (const {
readdir } = await import("node:fs/promises");) with a top-level static import
alongside the existing mkdir, readFile, unlink imports; update the import list
at the top of the module to include readdir so the
scheduler/dynamic-schedules.ts code uses the statically imported readdir symbol
instead of performing an await import at runtime.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/fleet-manager/status-queries.tspackages/core/src/scheduler/dynamic-schedules.tspackages/core/src/scheduler/scheduler.ts
Adds a `typing_indicator` boolean to `DiscordOutputSchema` (default: true)
that controls whether the bot shows a typing indicator while processing.
The typing indicator can cause "An unknown error occurred" messages in
Discord on long-running jobs, because Discord's client shows an error
when the sendTyping() API call fails or races with rate limits during
extended processing.
Setting `typing_indicator: false` in the agent's Discord output config
disables the indicator entirely:
```yaml
chat:
discord:
output:
typing_indicator: false
```
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ile upload
Four Discord/Slack integration improvements:
1. Message deduplication (Discord + Slack)
- Skip intermediate JSONL snapshots (stop_reason: null) that lack
complete text content
- Deliver only finalized snapshots, deduplicate by message.id
- Fixes "completed the task but no response" fallback message
2. Acknowledgement emoji reaction (Discord)
- React with configurable emoji (default: eyes) on message receipt
- Remove reaction after response is sent
- New output config: acknowledge_emoji
3. Voice message transcription (Discord)
- Detect Discord voice messages via MessageFlags.IsVoiceMessage
- Download audio, transcribe via OpenAI Whisper API
- New config: chat.discord.voice (enabled, provider, api_key_env,
model, language)
- New module: voice-transcriber.ts
4. File upload / file sender MCP (Discord)
- Add uploadFile() to DiscordConnector using AttachmentBuilder
- Wire FileSenderContext + createFileSenderDef in Discord manager
- Pass injectedMcpServers to trigger() so agents can send files
back to the originating channel
- Mirrors existing Slack file upload support
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…prompts, and progress indicators Discord conversations with Claude Code agents are verbose — they narrate tool usage, emit intermediate reasoning, and produce full diffs. This adds three features that work together to make the Discord experience feel like texting a coworker: - **concise_mode** (default: true): Appends a system prompt telling the agent to skip narration and give only its final answer. Threaded through a new `systemPromptAppend` field on TriggerOptions → RunnerOptions → RuntimeExecuteOptions, applied in both SDK (preset append) and CLI (--system-prompt) runtimes. - **final_answer_only** (default: true): Buffers all assistant messages during execution and sends only the last one (the actual answer). Suppresses tool result embeds and system status embeds in this mode. - **progress_indicator** (default: true): Shows a single "Working..." embed that updates in-place with tool names as they run, throttled to 2s intervals. Deleted on job completion. Powered by a new `replyWithRef` function on the Discord connector that returns edit/delete handles. All three options default to the new quiet behavior. Set to `false` in agent config to restore verbose output. Slack, Web, and existing configs are unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add resultText fallback from SDK result message when all assistant turns are tool-only (no text blocks). Update concise mode system prompt to instruct Claude to always end with a text summary. Add config merge support for new discord output fields. Add 10 handleMessage pipeline tests covering dedup, buffering, fallback, tool result embeds, concise prompt injection, and session storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nded footer
Redesign all Discord embeds for a calm, premium feel:
- Remove titles from all embeds (progress, tool results, errors, status, summary)
- Add signature footer ("herdctl · agent-name") to all embeds
- Refine color palette: soft violet for progress, emerald for success, cool gray for system, sky blue for commands
- Compact tool result embeds: collapse title + fields into single description line
- Horizontal result summary with centered-dot separators instead of inline fields
- Convert /help, /status, /reset slash commands from plain text to styled embeds
- Add syntax highlighting (ansi) for Bash tool output
- Make DiscordReplyEmbed.title optional to support title-less embeds
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add processing for non-voice file attachments (images, PDFs, text/code files) uploaded alongside Discord messages. Text files are inlined into the prompt; images and PDFs are saved to disk so the agent can view them via its Read tool. Configurable via chat.discord.attachments with size limits, allowed types, and automatic cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stant_messages enum
Replace two blunt output controls with a single `assistant_messages` enum
("answers" | "all") that classifies turns by whether they contain tool_use
blocks. Answer turns (no tool_use) are sent immediately; reasoning turns
(text + tool_use) are suppressed in "answers" mode. This eliminates message
buffering, removes the concise-mode system prompt injection that degraded
answer quality, and fixes the common "no additional output to share" fallback.
Also flips `result_summary` default to `true` so task stats are always shown.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Agents with multiple MCP servers (e.g. gmail, calendar, bear, gdrive, google-contacts) can exceed 15s during initialization, causing the session file watcher to time out before Claude CLI creates the session. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CLI runtime silently ignored injectedMcpServers (file sender for Discord/Slack uploads). Reuse existing mcp-http-bridge infrastructure: start HTTP bridges on localhost for each injected server and pass them via --mcp-config as HTTP-type MCP servers. Also includes: - Fix --mcp-config wrapper (mcpServers key was missing, caused CLI hang) - Synthetic result message for CLI runtime usage tracking - Discord embed content extraction in conversation context builder - discord-changes.md documenting all fork changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Files uploaded by agents via MCP are now buffered and attached to the next answer message instead of sent as standalone messages above it. Also updates discord-changes.md with corrected descriptions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Critical: - Restore process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT in finally block to prevent leaking state across concurrent jobs Important: - Clean up partial HTTP bridges if a bridge fails to start midway - Use wrapper object for progressEmbedHandle to fix TypeScript narrowing - Add voice transcription prefix "[Voice message transcription]:" as documented - Cap toolNamesRun to last 50 entries to prevent unbounded memory growth - Wrap finally block async operations in try/catch to prevent error masking - Document allowedTools behavior for injected MCP tools Minor: - Add 30s timeout to attachment downloads and voice message fetch - Add 60s timeout to Whisper API transcription call - Cap embed text extraction to 4000 chars in mention handler - Use broader channel type cast (TextChannel | DMChannel) for uploadFile - Remove discord-changes.md from repo (belongs in PR description, not tree) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Wrap ack reaction in try/catch so failure doesn't abort message handling - Add path traversal validation on download_dir schema (reject '..' and absolute paths) - Validate Whisper API response payload before returning - Move progress embed cleanup to finally block (cleanup on error path too) - Use path.dirname() instead of manual lastIndexOf/substring - Add vi.unstubAllGlobals() safety net in attachments test afterEach Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When self_scheduling is enabled, append a system prompt snippet that teaches agents how to use the herdctl scheduling tools. This prevents agents from trying to edit config files directly (as happened with juliusagent) and provides clear guidelines on schedule creation, frequency limits, TTL usage, and prompt authoring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/config/self-scheduling.ts`:
- Around line 80-103: The loop currently continues when "herdctl-scheduler"
exists, skipping the self-scheduling prompt; instead, change the flow so you
only skip creating/overwriting agent.mcp_servers["herdctl-scheduler"] but always
append the scheduling guidance: check "herdctl-scheduler" in agent.mcp_servers
and only assign the mcp server when absent, then unconditionally call
buildSchedulingPrompt(selfScheduling) and merge it into agent.system_prompt
(using the existing concatenation logic) so agents with an existing
herdctl-scheduler still receive the prompt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baa13bec-ecdd-455b-8e6a-fccec1a1534f
📒 Files selected for processing (2)
packages/core/src/config/__tests__/self-scheduling.test.tspackages/core/src/config/self-scheduling.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/config/tests/self-scheduling.test.ts
When an operator explicitly declares a custom herdctl-scheduler MCP server, the system prompt guidance should still be appended. Previously the `continue` statement skipped both MCP injection and prompt injection. Addresses CodeRabbit review feedback on PR edspencer#192. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
self_scheduling.enabled: truein agent config — theherdctl-schedulerMCP server is auto-injected.herdctl/dynamic-schedules/<agent>.yaml— one file per agent, no cross-agent contentionmax_schedules(default 10),min_intervalenforcement (default 5m), optional TTL with lazy expirationsource: "static" | "dynamic"tag on ScheduleInfo, schedules inspectable on diskWhat it looks like for the operator
What it looks like for the agent
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Tests